* [PATCH] EmbeddedPkg/MmcDxe: Add non-DDR timing mode support
@ 2017-06-08 8:12 Jun Nie
2017-06-08 8:39 ` Haojian Zhuang
2017-06-08 16:51 ` Leif Lindholm
0 siblings, 2 replies; 5+ messages in thread
From: Jun Nie @ 2017-06-08 8:12 UTC (permalink / raw)
To: olivier.martin, haojian.zhuang, edk2-devel; +Cc: shawn.guo, jason.liu, Jun Nie
Only DDR mode is support for 8bit mode currently. Add
non-DDR case when configuring ECSD.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jun Nie <jun.nie@linaro.org>
---
EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
index 574a77e..5c0d7e7 100644
--- a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
+++ b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
@@ -286,7 +286,10 @@ InitializeEmmcDevice (
}
Status = Host->SetIos (Host, BusClockFreq, 8, TimingMode[Idx]);
if (!EFI_ERROR (Status)) {
- Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_BUS_WIDTH, EMMC_BUS_WIDTH_DDR_8BIT);
+ if (Idx < 2)
+ Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_BUS_WIDTH, EMMC_BUS_WIDTH_DDR_8BIT);
+ else
+ Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_BUS_WIDTH, EMMC_BUS_WIDTH_8BIT);
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_ERROR, "InitializeEmmcDevice(): Failed to set EXTCSD bus width, Status:%r\n", Status));
}
--
1.9.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] EmbeddedPkg/MmcDxe: Add non-DDR timing mode support
2017-06-08 8:12 [PATCH] EmbeddedPkg/MmcDxe: Add non-DDR timing mode support Jun Nie
@ 2017-06-08 8:39 ` Haojian Zhuang
2017-06-08 16:55 ` Leif Lindholm
2017-06-08 16:51 ` Leif Lindholm
1 sibling, 1 reply; 5+ messages in thread
From: Haojian Zhuang @ 2017-06-08 8:39 UTC (permalink / raw)
To: Jun Nie, olivier.martin, edk2-devel; +Cc: shawn.guo, jason.liu
On 2017/6/8 16:12, Jun Nie wrote:
> Only DDR mode is support for 8bit mode currently. Add
> non-DDR case when configuring ECSD.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
> EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> index 574a77e..5c0d7e7 100644
> --- a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> +++ b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> @@ -286,7 +286,10 @@ InitializeEmmcDevice (
> }
> Status = Host->SetIos (Host, BusClockFreq, 8, TimingMode[Idx]);
> if (!EFI_ERROR (Status)) {
> - Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_BUS_WIDTH, EMMC_BUS_WIDTH_DDR_8BIT);
> + if (Idx < 2)
It's better to avoid hardcoded value at here. Maybe you can use switch &
case on TimingMode array at here.
> + Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_BUS_WIDTH, EMMC_BUS_WIDTH_DDR_8BIT);
> + else
> + Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_BUS_WIDTH, EMMC_BUS_WIDTH_8BIT);
> if (EFI_ERROR (Status)) {
> DEBUG ((DEBUG_ERROR, "InitializeEmmcDevice(): Failed to set EXTCSD bus width, Status:%r\n", Status));
> }
>
Best Regards
Haojian
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] EmbeddedPkg/MmcDxe: Add non-DDR timing mode support
2017-06-08 8:12 [PATCH] EmbeddedPkg/MmcDxe: Add non-DDR timing mode support Jun Nie
2017-06-08 8:39 ` Haojian Zhuang
@ 2017-06-08 16:51 ` Leif Lindholm
1 sibling, 0 replies; 5+ messages in thread
From: Leif Lindholm @ 2017-06-08 16:51 UTC (permalink / raw)
To: Jun Nie; +Cc: haojian.zhuang, edk2-devel, jason.liu, shawn.guo, ard.biesheuvel
Hi Jun,
Don't forget to cc the package maintainers on your patch submission,
as found in top-level Maintainers.txt.
On Thu, Jun 08, 2017 at 04:12:27PM +0800, Jun Nie wrote:
> Only DDR mode is support for 8bit mode currently. Add
> non-DDR case when configuring ECSD.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
> EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> index 574a77e..5c0d7e7 100644
> --- a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> +++ b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> @@ -286,7 +286,10 @@ InitializeEmmcDevice (
> }
> Status = Host->SetIos (Host, BusClockFreq, 8, TimingMode[Idx]);
> if (!EFI_ERROR (Status)) {
> - Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_BUS_WIDTH, EMMC_BUS_WIDTH_DDR_8BIT);
> + if (Idx < 2)
While currently programatically correct, this would break if anyone
changed the order of entries in TimingMode.
A less fragile test would be
if (TimingMode[Idx] & (EMMCHS52DDR1V2 | EMMCHS52DDR1V8))
(If we ever need to support HS400 devices in this function, I would
suggest a rewrite without fixed indexes and with a helper macro to
determine DDR-ness.)
Regards,
Leif
> + Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_BUS_WIDTH, EMMC_BUS_WIDTH_DDR_8BIT);
> + else
> + Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_BUS_WIDTH, EMMC_BUS_WIDTH_8BIT);
> if (EFI_ERROR (Status)) {
> DEBUG ((DEBUG_ERROR, "InitializeEmmcDevice(): Failed to set EXTCSD bus width, Status:%r\n", Status));
> }
> --
> 1.9.1
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] EmbeddedPkg/MmcDxe: Add non-DDR timing mode support
2017-06-08 8:39 ` Haojian Zhuang
@ 2017-06-08 16:55 ` Leif Lindholm
2017-06-09 3:54 ` Jun Nie
0 siblings, 1 reply; 5+ messages in thread
From: Leif Lindholm @ 2017-06-08 16:55 UTC (permalink / raw)
To: Haojian Zhuang; +Cc: Jun Nie, edk2-devel, jason.liu, shawn.guo
On Thu, Jun 08, 2017 at 04:39:44PM +0800, Haojian Zhuang wrote:
> On 2017/6/8 16:12, Jun Nie wrote:
> >Only DDR mode is support for 8bit mode currently. Add
> >non-DDR case when configuring ECSD.
> >
> >Contributed-under: TianoCore Contribution Agreement 1.0
> >Signed-off-by: Jun Nie <jun.nie@linaro.org>
> >---
> > EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> >diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> >index 574a77e..5c0d7e7 100644
> >--- a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> >+++ b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> >@@ -286,7 +286,10 @@ InitializeEmmcDevice (
> > }
> > Status = Host->SetIos (Host, BusClockFreq, 8, TimingMode[Idx]);
> > if (!EFI_ERROR (Status)) {
> >- Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_BUS_WIDTH, EMMC_BUS_WIDTH_DDR_8BIT);
> >+ if (Idx < 2)
>
> It's better to avoid hardcoded value at here. Maybe you can use switch &
> case on TimingMode array at here.
Yes, that would also work.
Indeed, if any other possible values than EMMC_BUS_WIDTH_DDR_8BIT or
EMMC_BUS_WIDTH_8BIT could be likely in the future, that would be the
preferable solution.
Regards,
Leif
> >+ Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_BUS_WIDTH, EMMC_BUS_WIDTH_DDR_8BIT);
> >+ else
> >+ Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_BUS_WIDTH, EMMC_BUS_WIDTH_8BIT);
> > if (EFI_ERROR (Status)) {
> > DEBUG ((DEBUG_ERROR, "InitializeEmmcDevice(): Failed to set EXTCSD bus width, Status:%r\n", Status));
> > }
> >
>
> Best Regards
> Haojian
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] EmbeddedPkg/MmcDxe: Add non-DDR timing mode support
2017-06-08 16:55 ` Leif Lindholm
@ 2017-06-09 3:54 ` Jun Nie
0 siblings, 0 replies; 5+ messages in thread
From: Jun Nie @ 2017-06-09 3:54 UTC (permalink / raw)
To: Leif Lindholm; +Cc: Haojian Zhuang, edk2-devel, Jason Liu, Shawn Guo
2017-06-09 0:55 GMT+08:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Thu, Jun 08, 2017 at 04:39:44PM +0800, Haojian Zhuang wrote:
>> On 2017/6/8 16:12, Jun Nie wrote:
>> >Only DDR mode is support for 8bit mode currently. Add
>> >non-DDR case when configuring ECSD.
>> >
>> >Contributed-under: TianoCore Contribution Agreement 1.0
>> >Signed-off-by: Jun Nie <jun.nie@linaro.org>
>> >---
>> > EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 5 ++++-
>> > 1 file changed, 4 insertions(+), 1 deletion(-)
>> >
>> >diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
>> >index 574a77e..5c0d7e7 100644
>> >--- a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
>> >+++ b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
>> >@@ -286,7 +286,10 @@ InitializeEmmcDevice (
>> > }
>> > Status = Host->SetIos (Host, BusClockFreq, 8, TimingMode[Idx]);
>> > if (!EFI_ERROR (Status)) {
>> >- Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_BUS_WIDTH, EMMC_BUS_WIDTH_DDR_8BIT);
>> >+ if (Idx < 2)
>>
>> It's better to avoid hardcoded value at here. Maybe you can use switch &
>> case on TimingMode array at here.
>
> Yes, that would also work.
>
> Indeed, if any other possible values than EMMC_BUS_WIDTH_DDR_8BIT or
> EMMC_BUS_WIDTH_8BIT could be likely in the future, that would be the
> preferable solution.
>
> Regards,
>
> Leif
Yes, follow switch/case way is more extendable. Will add it and add
maintainer in next version.
Jun
>
>> >+ Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_BUS_WIDTH, EMMC_BUS_WIDTH_DDR_8BIT);
>> >+ else
>> >+ Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_BUS_WIDTH, EMMC_BUS_WIDTH_8BIT);
>> > if (EFI_ERROR (Status)) {
>> > DEBUG ((DEBUG_ERROR, "InitializeEmmcDevice(): Failed to set EXTCSD bus width, Status:%r\n", Status));
>> > }
>> >
>>
>> Best Regards
>> Haojian
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-06-09 3:53 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-06-08 8:12 [PATCH] EmbeddedPkg/MmcDxe: Add non-DDR timing mode support Jun Nie
2017-06-08 8:39 ` Haojian Zhuang
2017-06-08 16:55 ` Leif Lindholm
2017-06-09 3:54 ` Jun Nie
2017-06-08 16:51 ` Leif Lindholm
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox