[PATCH v2 0/4] SdMmc fixes

[PATCH v2 0/4] SdMmc fixes

[Marcin Wojtas]

Hi,

Answering the review request, I extracted SdMmcPciHcDxe driver fixes
from SdMmcOverride protocol modification. Comparing to v1,
patches are rebased onto the newest master branch and also a macro
is used instead of the raw value in SdMmcHcReset.

Patches are available in the github:
https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/sdmmc-fixes-r20180907

I'm looking forward to the comments and remarks.

Best regards,
Marcin

Changelog:
v1 -> v2
 * rebase on top of the newest master
 * resolve conflicts after taking fixes out from new features
 * 3/4 - use macro instead of raw value in SdMmcHcReset

Marcin Wojtas (3):
  MdeModulePkg/SdMmcPciHcDxe: Fix HS200 operation
  MdeModulePkg/SdMmcPciHcDxe: Adjust eMMC clock and bus width sequence
  MdeModulePkg/SdMmcPciHcDxe: Execute card detect only for RemovableSlot

Tomasz Michalec (1):
  MdeModulePkg/SdMmcPciHcDxe: Fix SdMmcHcReset to set only necesery bits

 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h   | 10 +++++
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c    | 39 +++++---------------
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c | 18 ++++++---
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   |  6 +--
 4 files changed, 34 insertions(+), 39 deletions(-)

-- 
2.7.4

[PATCH v2 1/4] MdeModulePkg/SdMmcPciHcDxe: Fix HS200 operation

[Marcin Wojtas]

When switching to any of high speed modes (HS, HS200, HS400)
there is need to set HS_ENABLE bit in Host Control 1 register
which allow Host Controller to output CMD and DAT lines on
both edges of clock. In Linux it is done after switching bus
width in sdhci_set_ios().

Also according to JESD84-B50-1 chapter 6.6.4 "HS200 timing mode
selection" (documentation about eMMC4.5 standard) there is
no need to disable clock when switching to HS200.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h |  5 ++++
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c  | 30 +++-----------------
 2 files changed, 9 insertions(+), 26 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
index e389d52..e3fadb5 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
@@ -63,6 +63,11 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #define SD_MMC_HC_CTRL_VER            0xFE
 
 //
+// SD Host Control 1 Register bits description
+//
+#define SD_MMC_HC_HOST_CTRL1_HS_ENABLE  (1 << 2)
+
+//
 // The transfer modes supported by SD Host Controller
 // Simplified Spec 3.0 Table 1-2
 //
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
index c5fd214..b3903b4 100755
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
@@ -816,8 +816,8 @@ EmmcSwitchToHS200 (
 {
   EFI_STATUS          Status;
   UINT8               HsTiming;
+  UINT8               HostCtrl1;
   UINT8               HostCtrl2;
-  UINT16              ClockCtrl;
 
   if ((BusWidth != 4) && (BusWidth != 8)) {
     return EFI_INVALID_PARAMETER;
@@ -828,12 +828,10 @@ EmmcSwitchToHS200 (
     return Status;
   }
   //
-  // Set to HS200/SDR104 timing
-  //
-  //
-  // Stop bus clock at first
+  // Set to High Speed timing
   //
-  Status = SdMmcHcStopClock (PciIo, Slot);
+  HostCtrl1 = SD_MMC_HC_HOST_CTRL1_HS_ENABLE;
+  Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL1, sizeof (HostCtrl1), &HostCtrl1);
   if (EFI_ERROR (Status)) {
     return Status;
   }
@@ -853,26 +851,6 @@ EmmcSwitchToHS200 (
   if (EFI_ERROR (Status)) {
     return Status;
   }
-  //
-  // Wait Internal Clock Stable in the Clock Control register to be 1 before set SD Clock Enable bit
-  //
-  Status = SdMmcHcWaitMmioSet (
-             PciIo,
-             Slot,
-             SD_MMC_HC_CLOCK_CTRL,
-             sizeof (ClockCtrl),
-             BIT1,
-             BIT1,
-             SD_MMC_HC_GENERIC_TIMEOUT
-             );
-  if (EFI_ERROR (Status)) {
-    return Status;
-  }
-  //
-  // Set SD Clock Enable in the Clock Control register to 1
-  //
-  ClockCtrl = BIT2;
-  Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_CLOCK_CTRL, sizeof (ClockCtrl), &ClockCtrl);
 
   HsTiming = 2;
   Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming, ClockFreq);
-- 
2.7.4

[PATCH v2 2/4] MdeModulePkg/SdMmcPciHcDxe: Adjust eMMC clock and bus width sequence

[Marcin Wojtas]

According to JESD84-B50-1 chapter A.6 (documentation about eMMC4.5
standard) step "Changing the data bus width" (A.6.3) should be
execute after step "Switching to high-speed mode" (A.6.2).

This patch fixes the bus-width/clock-setting sequence
in EmmcSwitchToHighSpeed ().

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
index b3903b4..87027b8 100755
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
@@ -745,10 +745,6 @@ EmmcSwitchToHighSpeed (
   UINT8               HostCtrl1;
   UINT8               HostCtrl2;
 
-  Status = EmmcSwitchBusWidth (PciIo, PassThru, Slot, Rca, IsDdr, BusWidth);
-  if (EFI_ERROR (Status)) {
-    return Status;
-  }
   //
   // Set to Hight Speed timing
   //
@@ -783,6 +779,11 @@ EmmcSwitchToHighSpeed (
 
   HsTiming = 1;
   Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming, ClockFreq);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  Status = EmmcSwitchBusWidth (PciIo, PassThru, Slot, Rca, IsDdr, BusWidth);
 
   return Status;
 }
-- 
2.7.4

[PATCH v2 3/4] MdeModulePkg/SdMmcPciHcDxe: Fix SdMmcHcReset to set only necesery bits

[Marcin Wojtas]

From: Tomasz Michalec <tm@semihalf.com>

SdMmcHcReset used to set all bits of Software Reset Register to 1
including reserved ones.

Now only first bit is set which means "Software Reset for All".

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h | 5 +++++
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 6 +++---
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
index e3fadb5..ec90d1e 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
@@ -68,6 +68,11 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #define SD_MMC_HC_HOST_CTRL1_HS_ENABLE  (1 << 2)
 
 //
+// SD Software Reset Register bits description
+//
+#define SD_MMC_HC_SW_RST_ALL            (1 << 0)
+
+//
 // The transfer modes supported by SD Host Controller
 // Simplified Spec 3.0 Table 1-2
 //
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
index 9672b5b..9d9bca8 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
@@ -454,8 +454,8 @@ SdMmcHcReset (
   }
 
   PciIo   = Private->PciIo;
-  SwReset = 0xFF;
-  Status  = SdMmcHcRwMmio (PciIo, Slot, SD_MMC_HC_SW_RST, FALSE, sizeof (SwReset), &SwReset);
+  SwReset = SD_MMC_HC_SW_RST_ALL;
+  Status  = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_SW_RST, sizeof (SwReset), &SwReset);
 
   if (EFI_ERROR (Status)) {
     DEBUG ((DEBUG_ERROR, "SdMmcHcReset: write full 1 fails: %r\n", Status));
@@ -467,7 +467,7 @@ SdMmcHcReset (
              Slot,
              SD_MMC_HC_SW_RST,
              sizeof (SwReset),
-             0xFF,
+             SD_MMC_HC_SW_RST_ALL,
              0x00,
              SD_MMC_HC_GENERIC_TIMEOUT
              );
-- 
2.7.4

[PATCH v2 4/4] MdeModulePkg/SdMmcPciHcDxe: Execute card detect only for RemovableSlot

[Marcin Wojtas]

Some devices can be non removable (such as eMMC) and checking
Present State Register on host controller may falsely return
an information that device is not present. Execute this
check conditionally on the SloType field value.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
index f923930..bf9869d 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
@@ -661,12 +661,18 @@ SdMmcPciHcDriverBindingStart (
     //
     // Check whether there is a SD/MMC card attached
     //
-    Status = SdMmcHcCardDetect (PciIo, Slot, &MediaPresent);
-    if (EFI_ERROR (Status) && (Status != EFI_MEDIA_CHANGED)) {
-      continue;
-    } else if (!MediaPresent) {
-      DEBUG ((DEBUG_INFO, "SdMmcHcCardDetect: No device attached in Slot[%d]!!!\n", Slot));
-      continue;
+    if (Private->Slot[Slot].SlotType == RemovableSlot) {
+      Status = SdMmcHcCardDetect (PciIo, Slot, &MediaPresent);
+      if (EFI_ERROR (Status) && (Status != EFI_MEDIA_CHANGED)) {
+        continue;
+      } else if (!MediaPresent) {
+        DEBUG ((
+          DEBUG_INFO,
+          "SdMmcHcCardDetect: No device attached in Slot[%d]!!!\n",
+          Slot
+          ));
+        continue;
+      }
     }
 
     Status = SdMmcHcInitHost (Private, Slot);
-- 
2.7.4

Re: [PATCH v2 0/4] SdMmc fixes

[Ard Biesheuvel]

+Hao

On 7 September 2018 at 11:10, Marcin Wojtas <mw@semihalf.com> wrote:
> Hi,
>
> Answering the review request, I extracted SdMmcPciHcDxe driver fixes
> from SdMmcOverride protocol modification. Comparing to v1,
> patches are rebased onto the newest master branch and also a macro
> is used instead of the raw value in SdMmcHcReset.
>
> Patches are available in the github:
> https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/sdmmc-fixes-r20180907
>
> I'm looking forward to the comments and remarks.
>
> Best regards,
> Marcin
>
> Changelog:
> v1 -> v2
>  * rebase on top of the newest master
>  * resolve conflicts after taking fixes out from new features
>  * 3/4 - use macro instead of raw value in SdMmcHcReset
>
> Marcin Wojtas (3):
>   MdeModulePkg/SdMmcPciHcDxe: Fix HS200 operation
>   MdeModulePkg/SdMmcPciHcDxe: Adjust eMMC clock and bus width sequence
>   MdeModulePkg/SdMmcPciHcDxe: Execute card detect only for RemovableSlot
>
> Tomasz Michalec (1):
>   MdeModulePkg/SdMmcPciHcDxe: Fix SdMmcHcReset to set only necesery bits
>
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h   | 10 +++++
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c    | 39 +++++---------------
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c | 18 ++++++---
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   |  6 +--
>  4 files changed, 34 insertions(+), 39 deletions(-)
>
> --
> 2.7.4
>

Re: [PATCH v2 3/4] MdeModulePkg/SdMmcPciHcDxe: Fix SdMmcHcReset to set only necesery bits

[Ard Biesheuvel]

On 7 September 2018 at 11:10, Marcin Wojtas <mw@semihalf.com> wrote:
> From: Tomasz Michalec <tm@semihalf.com>
>
> SdMmcHcReset used to set all bits of Software Reset Register to 1
> including reserved ones.
>
> Now only first bit is set which means "Software Reset for All".
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h | 5 +++++
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 6 +++---
>  2 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> index e3fadb5..ec90d1e 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> @@ -68,6 +68,11 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  #define SD_MMC_HC_HOST_CTRL1_HS_ENABLE  (1 << 2)
>
>  //
> +// SD Software Reset Register bits description
> +//
> +#define SD_MMC_HC_SW_RST_ALL            (1 << 0)

Please use BIT0 here

> +
> +//
>  // The transfer modes supported by SD Host Controller
>  // Simplified Spec 3.0 Table 1-2
>  //
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> index 9672b5b..9d9bca8 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> @@ -454,8 +454,8 @@ SdMmcHcReset (
>    }
>
>    PciIo   = Private->PciIo;
> -  SwReset = 0xFF;
> -  Status  = SdMmcHcRwMmio (PciIo, Slot, SD_MMC_HC_SW_RST, FALSE, sizeof (SwReset), &SwReset);
> +  SwReset = SD_MMC_HC_SW_RST_ALL;
> +  Status  = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_SW_RST, sizeof (SwReset), &SwReset);
>
>    if (EFI_ERROR (Status)) {
>      DEBUG ((DEBUG_ERROR, "SdMmcHcReset: write full 1 fails: %r\n", Status));
> @@ -467,7 +467,7 @@ SdMmcHcReset (
>               Slot,
>               SD_MMC_HC_SW_RST,
>               sizeof (SwReset),
> -             0xFF,
> +             SD_MMC_HC_SW_RST_ALL,
>               0x00,
>               SD_MMC_HC_GENERIC_TIMEOUT
>               );
> --
> 2.7.4
>

Re: [PATCH v2 0/4] SdMmc fixes

[Marcin Wojtas]

Hi,

So far there was one minor remark from Ard (about macro definition) - I
would appreciate any comments before I can submit v3.

Thanks,
Marcin

pt., 7 wrz 2018 o 13:04 Ard Biesheuvel <ard.biesheuvel@linaro.org>
napisał(a):

> +Hao
>
> On 7 September 2018 at 11:10, Marcin Wojtas <mw@semihalf.com> wrote:
> > Hi,
> >
> > Answering the review request, I extracted SdMmcPciHcDxe driver fixes
> > from SdMmcOverride protocol modification. Comparing to v1,
> > patches are rebased onto the newest master branch and also a macro
> > is used instead of the raw value in SdMmcHcReset.
> >
> > Patches are available in the github:
> >
> https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/sdmmc-fixes-r20180907
> >
> > I'm looking forward to the comments and remarks.
> >
> > Best regards,
> > Marcin
> >
> > Changelog:
> > v1 -> v2
> >  * rebase on top of the newest master
> >  * resolve conflicts after taking fixes out from new features
> >  * 3/4 - use macro instead of raw value in SdMmcHcReset
> >
> > Marcin Wojtas (3):
> >   MdeModulePkg/SdMmcPciHcDxe: Fix HS200 operation
> >   MdeModulePkg/SdMmcPciHcDxe: Adjust eMMC clock and bus width sequence
> >   MdeModulePkg/SdMmcPciHcDxe: Execute card detect only for RemovableSlot
> >
> > Tomasz Michalec (1):
> >   MdeModulePkg/SdMmcPciHcDxe: Fix SdMmcHcReset to set only necesery bits
> >
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h   | 10 +++++
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c    | 39
> +++++---------------
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c | 18 ++++++---
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   |  6 +--
> >  4 files changed, 34 insertions(+), 39 deletions(-)
> >
> > --
> > 2.7.4
> >
>

Re: [PATCH v2 1/4] MdeModulePkg/SdMmcPciHcDxe: Fix HS200 operation

[Wu, Hao A]

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Marcin Wojtas
> Sent: Friday, September 07, 2018 5:10 PM
> To: edk2-devel@lists.01.org
> Cc: Tian, Feng; tm@semihalf.com; nadavh@marvell.com; Gao, Liming; Kinney,
> Michael D
> Subject: [edk2] [PATCH v2 1/4] MdeModulePkg/SdMmcPciHcDxe: Fix HS200
> operation
> 
> When switching to any of high speed modes (HS, HS200, HS400)
> there is need to set HS_ENABLE bit in Host Control 1 register
> which allow Host Controller to output CMD and DAT lines on
> both edges of clock. In Linux it is done after switching bus
> width in sdhci_set_ios().

I am checking both the SD and eMMC specs for this. As far as I can tell,
since the eMMC cards that support HS200 & HS400 will have 1.8V/1.2V
signaling. This makes a match within the SD specs for UHS-I mode (rather
than High Speed mode which is 3.3V signaling).

Switching the host controller to UHS-I mode requires configuring the '1.8V
Signaling Enable' (BIT3) and 'UHS Mode Select' (BIT2~0) fields of the Host
Control 2 Register. And the setting of the 'High Speed Enable' (BIT2)
field of the Host Control 1 Register seems not required to me.

Do you met with issues when switching the eMMC device to HS200/HS400 mode
when not setting BIT2 of the Host Control Register?

> 
> Also according to JESD84-B50-1 chapter 6.6.4 "HS200 timing mode
> selection" (documentation about eMMC4.5 standard) there is
> no need to disable clock when switching to HS200.

Yes, you are right that the eMMC spec does not require such a reset of the
reset SD Clock Enable (BIT2) of the Clock Control Register.

I think the code here is taking reference to the SD Host Controller
Simplified Spec 3.0:

Under the description of 'High Speed Enable' (BIT2) of the Host Control 1
Register (Table 2-16):

* If Preset Value Enable in the Host Control 2 register is set to 1, Host
* Driver needs to reset SD Clock Enable before changing this field to
* avoid generating clock glitches. After setting this field, the Host
* Driver sets SD Clock Enable again.

The spec makes very clear that the reset SD Clock Enable is only needed
when Preset Value Enable in the Host Control 2 register is set to 1.

But for the description of 'UHS Mode Select' (BIT2~0) of the Host Control
2 Register (Table 2-32):

* If Preset Value Enable in the Host Control 2 register is set to 1, Host
* Controller sets SDCLK Frequency Select, Clock Generator Select in the
* Clock Control register and Driver Strength Select according to Preset
* Value registers. In this case, one of preset value registers is selected
* by this field. Host Driver needs to reset SD Clock Enable before changing
* this field to avoid generating clock glitch. After setting this field,
* Host Driver sets SD Clock Enable again.

It's not that clear whether this reset is needed under all cases. So the
driver just play it safe here to added reset of the SD Clock Enable bit
during the switch of HS200 mode for eMMC devices.

So do you met with issues here for this? If not, I prefer to keep the
re-enabling of the clock logic here for stability consideration.

> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h |  5 ++++
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c  | 30 +++----------------
> -
>  2 files changed, 9 insertions(+), 26 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> index e389d52..e3fadb5 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> @@ -63,6 +63,11 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> KIND, EITHER EXPRESS OR IMPLIED.
>  #define SD_MMC_HC_CTRL_VER            0xFE
> 
>  //
> +// SD Host Control 1 Register bits description
> +//
> +#define SD_MMC_HC_HOST_CTRL1_HS_ENABLE  (1 << 2)
> +
> +//
>  // The transfer modes supported by SD Host Controller
>  // Simplified Spec 3.0 Table 1-2
>  //
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> index c5fd214..b3903b4 100755
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> @@ -816,8 +816,8 @@ EmmcSwitchToHS200 (
>  {
>    EFI_STATUS          Status;
>    UINT8               HsTiming;
> +  UINT8               HostCtrl1;
>    UINT8               HostCtrl2;
> -  UINT16              ClockCtrl;
> 
>    if ((BusWidth != 4) && (BusWidth != 8)) {
>      return EFI_INVALID_PARAMETER;
> @@ -828,12 +828,10 @@ EmmcSwitchToHS200 (
>      return Status;
>    }
>    //
> -  // Set to HS200/SDR104 timing
> -  //
> -  //
> -  // Stop bus clock at first
> +  // Set to High Speed timing
>    //
> -  Status = SdMmcHcStopClock (PciIo, Slot);
> +  HostCtrl1 = SD_MMC_HC_HOST_CTRL1_HS_ENABLE;
> +  Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL1, sizeof
> (HostCtrl1), &HostCtrl1);
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
> @@ -853,26 +851,6 @@ EmmcSwitchToHS200 (
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
> -  //
> -  // Wait Internal Clock Stable in the Clock Control register to be 1 before set
> SD Clock Enable bit
> -  //
> -  Status = SdMmcHcWaitMmioSet (
> -             PciIo,
> -             Slot,
> -             SD_MMC_HC_CLOCK_CTRL,
> -             sizeof (ClockCtrl),
> -             BIT1,
> -             BIT1,
> -             SD_MMC_HC_GENERIC_TIMEOUT
> -             );
> -  if (EFI_ERROR (Status)) {
> -    return Status;
> -  }
> -  //
> -  // Set SD Clock Enable in the Clock Control register to 1
> -  //
> -  ClockCtrl = BIT2;
> -  Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_CLOCK_CTRL, sizeof
> (ClockCtrl), &ClockCtrl);
> 
>    HsTiming = 2;
>    Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming,
> ClockFreq);
> --
> 2.7.4
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH v2 0/4] SdMmc fixes

[Wu, Hao A]

Hi Marcin,

Please give me some time to go through the series. I will provide feedbacks on these patches.
Sorry for the possible delay.

Best Regards,
Hao Wu

From: Marcin Wojtas [mailto:mw@semihalf.com]
Sent: Wednesday, September 12, 2018 3:29 PM
To: edk2-devel-01
Cc: Wu, Hao A; Ard Biesheuvel; Tian, Feng; Kinney, Michael D; Gao, Liming; Leif Lindholm; nadavh@marvell.com; jsd@semihalf.com; Tomasz Michalec
Subject: Re: [PATCH v2 0/4] SdMmc fixes

Hi,

So far there was one minor remark from Ard (about macro definition) - I would appreciate any comments before I can submit v3.

Thanks,
Marcin

pt., 7 wrz 2018 o 13:04 Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>> napisał(a):
+Hao

On 7 September 2018 at 11:10, Marcin Wojtas <mw@semihalf.com<mailto:mw@semihalf.com>> wrote:
> Hi,
>
> Answering the review request, I extracted SdMmcPciHcDxe driver fixes
> from SdMmcOverride protocol modification. Comparing to v1,
> patches are rebased onto the newest master branch and also a macro
> is used instead of the raw value in SdMmcHcReset.
>
> Patches are available in the github:
> https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/sdmmc-fixes-r20180907
>
> I'm looking forward to the comments and remarks.
>
> Best regards,
> Marcin
>
> Changelog:
> v1 -> v2
>  * rebase on top of the newest master
>  * resolve conflicts after taking fixes out from new features
>  * 3/4 - use macro instead of raw value in SdMmcHcReset
>
> Marcin Wojtas (3):
>   MdeModulePkg/SdMmcPciHcDxe: Fix HS200 operation
>   MdeModulePkg/SdMmcPciHcDxe: Adjust eMMC clock and bus width sequence
>   MdeModulePkg/SdMmcPciHcDxe: Execute card detect only for RemovableSlot
>
> Tomasz Michalec (1):
>   MdeModulePkg/SdMmcPciHcDxe: Fix SdMmcHcReset to set only necesery bits
>
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h   | 10 +++++
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c    | 39 +++++---------------
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c | 18 ++++++---
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   |  6 +--
>  4 files changed, 34 insertions(+), 39 deletions(-)
>
> --
> 2.7.4
>

Re: [PATCH v2 1/4] MdeModulePkg/SdMmcPciHcDxe: Fix HS200 operation

[Marcin Wojtas]

Hi Hao,

Thank you for your input. In v3, I'll withdraw this patch. I tested
and I can use HS200 successfully on my boards without it. Both changes
(HS enable bit and removing clock disabling) were residues from an old
debug, whereas the acutal fix was an additional HW reconfiguration
after switching the clock frequency.

Best regards,
Marcin

czw., 13 wrz 2018 o 16:43 Wu, Hao A <hao.a.wu@intel.com> napisał(a):
>
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> > Marcin Wojtas
> > Sent: Friday, September 07, 2018 5:10 PM
> > To: edk2-devel@lists.01.org
> > Cc: Tian, Feng; tm@semihalf.com; nadavh@marvell.com; Gao, Liming; Kinney,
> > Michael D
> > Subject: [edk2] [PATCH v2 1/4] MdeModulePkg/SdMmcPciHcDxe: Fix HS200
> > operation
> >
> > When switching to any of high speed modes (HS, HS200, HS400)
> > there is need to set HS_ENABLE bit in Host Control 1 register
> > which allow Host Controller to output CMD and DAT lines on
> > both edges of clock. In Linux it is done after switching bus
> > width in sdhci_set_ios().
>
> I am checking both the SD and eMMC specs for this. As far as I can tell,
> since the eMMC cards that support HS200 & HS400 will have 1.8V/1.2V
> signaling. This makes a match within the SD specs for UHS-I mode (rather
> than High Speed mode which is 3.3V signaling).
>
> Switching the host controller to UHS-I mode requires configuring the '1.8V
> Signaling Enable' (BIT3) and 'UHS Mode Select' (BIT2~0) fields of the Host
> Control 2 Register. And the setting of the 'High Speed Enable' (BIT2)
> field of the Host Control 1 Register seems not required to me.
>
> Do you met with issues when switching the eMMC device to HS200/HS400 mode
> when not setting BIT2 of the Host Control Register?
>
> >
> > Also according to JESD84-B50-1 chapter 6.6.4 "HS200 timing mode
> > selection" (documentation about eMMC4.5 standard) there is
> > no need to disable clock when switching to HS200.
>
> Yes, you are right that the eMMC spec does not require such a reset of the
> reset SD Clock Enable (BIT2) of the Clock Control Register.
>
> I think the code here is taking reference to the SD Host Controller
> Simplified Spec 3.0:
>
> Under the description of 'High Speed Enable' (BIT2) of the Host Control 1
> Register (Table 2-16):
>
> * If Preset Value Enable in the Host Control 2 register is set to 1, Host
> * Driver needs to reset SD Clock Enable before changing this field to
> * avoid generating clock glitches. After setting this field, the Host
> * Driver sets SD Clock Enable again.
>
> The spec makes very clear that the reset SD Clock Enable is only needed
> when Preset Value Enable in the Host Control 2 register is set to 1.
>
> But for the description of 'UHS Mode Select' (BIT2~0) of the Host Control
> 2 Register (Table 2-32):
>
> * If Preset Value Enable in the Host Control 2 register is set to 1, Host
> * Controller sets SDCLK Frequency Select, Clock Generator Select in the
> * Clock Control register and Driver Strength Select according to Preset
> * Value registers. In this case, one of preset value registers is selected
> * by this field. Host Driver needs to reset SD Clock Enable before changing
> * this field to avoid generating clock glitch. After setting this field,
> * Host Driver sets SD Clock Enable again.
>
> It's not that clear whether this reset is needed under all cases. So the
> driver just play it safe here to added reset of the SD Clock Enable bit
> during the switch of HS200 mode for eMMC devices.
>
> So do you met with issues here for this? If not, I prefer to keep the
> re-enabling of the clock logic here for stability consideration.
>
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> > ---
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h |  5 ++++
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c  | 30 +++----------------
> > -
> >  2 files changed, 9 insertions(+), 26 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> > index e389d52..e3fadb5 100644
> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> > @@ -63,6 +63,11 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> > KIND, EITHER EXPRESS OR IMPLIED.
> >  #define SD_MMC_HC_CTRL_VER            0xFE
> >
> >  //
> > +// SD Host Control 1 Register bits description
> > +//
> > +#define SD_MMC_HC_HOST_CTRL1_HS_ENABLE  (1 << 2)
> > +
> > +//
> >  // The transfer modes supported by SD Host Controller
> >  // Simplified Spec 3.0 Table 1-2
> >  //
> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > index c5fd214..b3903b4 100755
> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > @@ -816,8 +816,8 @@ EmmcSwitchToHS200 (
> >  {
> >    EFI_STATUS          Status;
> >    UINT8               HsTiming;
> > +  UINT8               HostCtrl1;
> >    UINT8               HostCtrl2;
> > -  UINT16              ClockCtrl;
> >
> >    if ((BusWidth != 4) && (BusWidth != 8)) {
> >      return EFI_INVALID_PARAMETER;
> > @@ -828,12 +828,10 @@ EmmcSwitchToHS200 (
> >      return Status;
> >    }
> >    //
> > -  // Set to HS200/SDR104 timing
> > -  //
> > -  //
> > -  // Stop bus clock at first
> > +  // Set to High Speed timing
> >    //
> > -  Status = SdMmcHcStopClock (PciIo, Slot);
> > +  HostCtrl1 = SD_MMC_HC_HOST_CTRL1_HS_ENABLE;
> > +  Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL1, sizeof
> > (HostCtrl1), &HostCtrl1);
> >    if (EFI_ERROR (Status)) {
> >      return Status;
> >    }
> > @@ -853,26 +851,6 @@ EmmcSwitchToHS200 (
> >    if (EFI_ERROR (Status)) {
> >      return Status;
> >    }
> > -  //
> > -  // Wait Internal Clock Stable in the Clock Control register to be 1 before set
> > SD Clock Enable bit
> > -  //
> > -  Status = SdMmcHcWaitMmioSet (
> > -             PciIo,
> > -             Slot,
> > -             SD_MMC_HC_CLOCK_CTRL,
> > -             sizeof (ClockCtrl),
> > -             BIT1,
> > -             BIT1,
> > -             SD_MMC_HC_GENERIC_TIMEOUT
> > -             );
> > -  if (EFI_ERROR (Status)) {
> > -    return Status;
> > -  }
> > -  //
> > -  // Set SD Clock Enable in the Clock Control register to 1
> > -  //
> > -  ClockCtrl = BIT2;
> > -  Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_CLOCK_CTRL, sizeof
> > (ClockCtrl), &ClockCtrl);
> >
> >    HsTiming = 2;
> >    Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming,
> > ClockFreq);
> > --
> > 2.7.4
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel