[PATCH v4 0/4] SdMmcOverride extension

[PATCH v4 0/4] SdMmcOverride extension

[Marcin Wojtas]

Hi,

Although I could've waited for Hao's remarks, I think it may
be better if he takes a look at much cleaner code, which
addresses v3 review comments.
The newest version of the patchset cleans-up significantly
patches 2&3 by removing code duplication and other minor
improvements.

Patches are available in the github:
https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/sdmmc-override-upstream-r20181109

Please note that extending SdMmcOverride protocol was impacting
so far the only user of it (Synquacer controller). In paralel
edk2-platforms patchset, a patch can be found:
("Silicon/SynQuacer/PlatformDxe: adjust to updated SdMmcOverride")
which adjust to the new API.
https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/xenon-upstream-r20181109

I'm looking forward to the comments and remarks.

Best regards,
Marcin

Changelog:
v3->v4
* 2/4:
  - avoid duplication by calling SdMmcOverride callback in SdMmcHcUhsSignaling

* 3/4:
  - avoid duplication by calling SdMmcOverride callback in EmmcSwitchClockFreq

* 4/4:
  - add Ard's RB

v2->v3
* 1/4:
  - rename new parameter to PhaseData
  - add Ard's RB

* 2/4:
  - s/Controler/Controller/
  - remove all references to MMC_SDR_50 mode
  - rename and reorder MMC bus modes
  - rename enum: s/SD_MMC_UHS_TIMING/SD_MMC_BUS_MODE/
    and move it to protocol header in order to drop including private one
  - fix if condition in EmmcSwitchToHighSpeed
  - call SdMmcHcUhsSignaling unconditionally before SdMmcOverride
    callback, so that protocol producer can optionally modify only quirky
    timing mode values.

*4/4
  - bump protocol version to 2
  - remove redundant assert from SdMmcPciHcDriverBindingStart
    (BaseClkFreq is already checked in SdMmcHcInitClockFreq)
  - update comment in SdMmcHcInitClockFreq
  - restore original DumpCapabilityReg and append 

v1 -> v2
* Rebase onto newest master
* 1/4 [new patch] - preparation for extending NotifyPhase
* 2/4 - UhsSignaling as a part of NotifyPhase
* 3/4 - SwitchClockFreqPost as a part of NotifyPhase
* 4/4 - Allow updating BaseClkFreq via Capability instead of the
        independent callback.


Marcin Wojtas (2):
  MdeModulePkg/SdMmcPciHcDxe: Add an optional parameter in NotifyPhase
  MdeModulePkg/SdMmcPciHcDxe: Allow overriding base clock frequency

Tomasz Michalec (2):
  MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol
  MdeModulePkg/SdMmcPciHcDxe: Add SwitchClockFreqPost to SdMmcOverride

 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h |   6 +
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h   |  42 ++++++-
 MdeModulePkg/Include/Protocol/SdMmcOverride.h      |  29 ++++-
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c    | 121 ++++++++++---------
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c      |  35 ++++--
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c |  13 +-
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   | 124 +++++++++++++++++---
 7 files changed, 280 insertions(+), 90 deletions(-)

-- 
2.7.4

[PATCH v4 1/4] MdeModulePkg/SdMmcPciHcDxe: Add an optional parameter in NotifyPhase

[Marcin Wojtas]

In order to ensure bigger flexibility in the NotifyPhase
routine of the SdMmcOverride protocol, enable using an
optional phase-specific data. This will allow to exchange
more information between the protocol producer driver
and SdMmcPciHcDxe in the newly added callbacks.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 MdeModulePkg/Include/Protocol/SdMmcOverride.h    |  4 +++-
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 12 ++++++++----
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
index 0766252..8a7669e 100644
--- a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
+++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
@@ -63,6 +63,7 @@ EFI_STATUS
   @param[in]      PhaseType             The type of operation and whether the
                                         hook is invoked right before (pre) or
                                         right after (post)
+  @param[in,out]  PhaseData             The pointer to a phase-specific data.
 
   @retval EFI_SUCCESS           The override function completed successfully.
   @retval EFI_NOT_FOUND         The specified controller or slot does not exist.
@@ -74,7 +75,8 @@ EFI_STATUS
 (EFIAPI * EDKII_SD_MMC_NOTIFY_PHASE) (
   IN      EFI_HANDLE                      ControllerHandle,
   IN      UINT8                           Slot,
-  IN      EDKII_SD_MMC_PHASE_TYPE         PhaseType
+  IN      EDKII_SD_MMC_PHASE_TYPE         PhaseType,
+  IN OUT  VOID                           *PhaseData
   );
 
 struct _EDKII_SD_MMC_OVERRIDE {
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
index bedc968..923c55b 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
@@ -444,7 +444,8 @@ SdMmcHcReset (
     Status = mOverride->NotifyPhase (
                           Private->ControllerHandle,
                           Slot,
-                          EdkiiSdMmcResetPre);
+                          EdkiiSdMmcResetPre,
+                          NULL);
     if (EFI_ERROR (Status)) {
       DEBUG ((DEBUG_WARN,
         "%a: SD/MMC pre reset notifier callback failed - %r\n",
@@ -494,7 +495,8 @@ SdMmcHcReset (
     Status = mOverride->NotifyPhase (
                           Private->ControllerHandle,
                           Slot,
-                          EdkiiSdMmcResetPost);
+                          EdkiiSdMmcResetPost,
+                          NULL);
     if (EFI_ERROR (Status)) {
       DEBUG ((DEBUG_WARN,
         "%a: SD/MMC post reset notifier callback failed - %r\n",
@@ -1088,7 +1090,8 @@ SdMmcHcInitHost (
     Status = mOverride->NotifyPhase (
                           Private->ControllerHandle,
                           Slot,
-                          EdkiiSdMmcInitHostPre);
+                          EdkiiSdMmcInitHostPre,
+                          NULL);
     if (EFI_ERROR (Status)) {
       DEBUG ((DEBUG_WARN,
         "%a: SD/MMC pre init notifier callback failed - %r\n",
@@ -1123,7 +1126,8 @@ SdMmcHcInitHost (
     Status = mOverride->NotifyPhase (
                           Private->ControllerHandle,
                           Slot,
-                          EdkiiSdMmcInitHostPost);
+                          EdkiiSdMmcInitHostPost,
+                          NULL);
     if (EFI_ERROR (Status)) {
       DEBUG ((DEBUG_WARN,
         "%a: SD/MMC post init notifier callback failed - %r\n",
-- 
2.7.4

[PATCH v4 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol

[Marcin Wojtas]

From: Tomasz Michalec <tm@semihalf.com>

Some SD Host Controllers use different values in Host Control 2 Register
to select UHS Mode. This patch adds a new UhsSignaling type routine to
the NotifyPhase of the SdMmcOverride protocol.

UHS signaling configuration is moved to a common, default routine
(SdMmcHcUhsSignaling). After it is executed, the protocol producer
can override the values if needed.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h | 34 ++++++++
 MdeModulePkg/Include/Protocol/SdMmcOverride.h    | 17 ++++
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c  | 86 ++++++++-----------
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c    | 15 ++--
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 89 ++++++++++++++++++++
 5 files changed, 181 insertions(+), 60 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
index 7e3f588..b47b0df 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
@@ -63,6 +63,21 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #define SD_MMC_HC_CTRL_VER            0xFE
 
 //
+// SD Host Controller bits to HOST_CTRL2 register
+//
+#define SD_MMC_HC_CTRL_UHS_MASK       0x0007
+#define SD_MMC_HC_CTRL_UHS_SDR12      0x0000
+#define SD_MMC_HC_CTRL_UHS_SDR25      0x0001
+#define SD_MMC_HC_CTRL_UHS_SDR50      0x0002
+#define SD_MMC_HC_CTRL_UHS_SDR104     0x0003
+#define SD_MMC_HC_CTRL_UHS_DDR50      0x0004
+#define SD_MMC_HC_CTRL_MMC_LEGACY     0x0000
+#define SD_MMC_HC_CTRL_MMC_HS_SDR     0x0001
+#define SD_MMC_HC_CTRL_MMC_HS_DDR     0x0004
+#define SD_MMC_HC_CTRL_MMC_HS200      0x0003
+#define SD_MMC_HC_CTRL_MMC_HS400      0x0005
+
+//
 // The transfer modes supported by SD Host Controller
 // Simplified Spec 3.0 Table 1-2
 //
@@ -518,4 +533,23 @@ SdMmcHcInitTimeoutCtrl (
   IN UINT8                  Slot
   );
 
+/**
+  Set SD Host Controller control 2 registry according to selected speed.
+
+  @param[in] ControllerHandle The handle of the controller.
+  @param[in] PciIo            The PCI IO protocol instance.
+  @param[in] Slot             The slot number of the SD card to send the command to.
+  @param[in] Timing           The timing to select.
+
+  @retval EFI_SUCCESS         The timing is set successfully.
+  @retval Others              The timing isn't set successfully.
+**/
+EFI_STATUS
+SdMmcHcUhsSignaling (
+  IN EFI_HANDLE             ControllerHandle,
+  IN EFI_PCI_IO_PROTOCOL    *PciIo,
+  IN UINT8                  Slot,
+  IN SD_MMC_BUS_MODE        Timing
+  );
+
 #endif
diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
index 8a7669e..f948bef 100644
--- a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
+++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
@@ -26,11 +26,28 @@
 
 typedef struct _EDKII_SD_MMC_OVERRIDE EDKII_SD_MMC_OVERRIDE;
 
+//
+// Bus timing modes
+//
+typedef enum {
+  SdMmcUhsSdr12,
+  SdMmcUhsSdr25,
+  SdMmcUhsSdr50,
+  SdMmcUhsSdr104,
+  SdMmcUhsDdr50,
+  SdMmcMmcLegacy,
+  SdMmcMmcHsSdr,
+  SdMmcMmcHsDdr,
+  SdMmcMmcHs200,
+  SdMmcMmcHs400,
+} SD_MMC_BUS_MODE;
+
 typedef enum {
   EdkiiSdMmcResetPre,
   EdkiiSdMmcResetPost,
   EdkiiSdMmcInitHostPre,
   EdkiiSdMmcInitHostPost,
+  EdkiiSdMmcUhsSignaling,
 } EDKII_SD_MMC_PHASE_TYPE;
 
 /**
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
index c5fd214..db4e8a1 100755
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
@@ -740,10 +740,13 @@ EmmcSwitchToHighSpeed (
   IN UINT8                              BusWidth
   )
 {
-  EFI_STATUS          Status;
-  UINT8               HsTiming;
-  UINT8               HostCtrl1;
-  UINT8               HostCtrl2;
+  EFI_STATUS              Status;
+  UINT8                   HsTiming;
+  UINT8                   HostCtrl1;
+  SD_MMC_BUS_MODE         Timing;
+  SD_MMC_HC_PRIVATE_DATA  *Private;
+
+  Private = SD_MMC_HC_PRIVATE_FROM_THIS (PassThru);
 
   Status = EmmcSwitchBusWidth (PciIo, PassThru, Slot, Rca, IsDdr, BusWidth);
   if (EFI_ERROR (Status)) {
@@ -758,25 +761,15 @@ EmmcSwitchToHighSpeed (
     return Status;
   }
 
-  //
-  // Clean UHS Mode Select field of Host Control 2 reigster before update
-  //
-  HostCtrl2 = (UINT8)~0x7;
-  Status = SdMmcHcAndMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
-  if (EFI_ERROR (Status)) {
-    return Status;
-  }
-  //
-  // Set UHS Mode Select field of Host Control 2 reigster to SDR12/25/50
-  //
   if (IsDdr) {
-    HostCtrl2 = BIT2;
+    Timing = SdMmcMmcHsDdr;
   } else if (ClockFreq == 52) {
-    HostCtrl2 = BIT0;
+    Timing = SdMmcMmcHsSdr;
   } else {
-    HostCtrl2 = 0;
+    Timing = SdMmcMmcLegacy;
   }
-  Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
+
+  Status = SdMmcHcUhsSignaling (Private->ControllerHandle, PciIo, Slot, Timing);
   if (EFI_ERROR (Status)) {
     return Status;
   }
@@ -814,10 +807,13 @@ EmmcSwitchToHS200 (
   IN UINT8                              BusWidth
   )
 {
-  EFI_STATUS          Status;
-  UINT8               HsTiming;
-  UINT8               HostCtrl2;
-  UINT16              ClockCtrl;
+  EFI_STATUS               Status;
+  UINT8                    HsTiming;
+  UINT16                   ClockCtrl;
+  SD_MMC_BUS_MODE          Timing;
+  SD_MMC_HC_PRIVATE_DATA  *Private;
+
+  Private = SD_MMC_HC_PRIVATE_FROM_THIS (PassThru);
 
   if ((BusWidth != 4) && (BusWidth != 8)) {
     return EFI_INVALID_PARAMETER;
@@ -837,22 +833,14 @@ EmmcSwitchToHS200 (
   if (EFI_ERROR (Status)) {
     return Status;
   }
-  //
-  // Clean UHS Mode Select field of Host Control 2 reigster before update
-  //
-  HostCtrl2 = (UINT8)~0x7;
-  Status = SdMmcHcAndMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
-  if (EFI_ERROR (Status)) {
-    return Status;
-  }
-  //
-  // Set UHS Mode Select field of Host Control 2 reigster to SDR104
-  //
-  HostCtrl2 = BIT0 | BIT1;
-  Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
+
+  Timing = SdMmcMmcHs200;
+
+  Status = SdMmcHcUhsSignaling (Private->ControllerHandle, PciIo, Slot, Timing);
   if (EFI_ERROR (Status)) {
     return Status;
   }
+
   //
   // Wait Internal Clock Stable in the Clock Control register to be 1 before set SD Clock Enable bit
   //
@@ -910,9 +898,12 @@ EmmcSwitchToHS400 (
   IN UINT32                             ClockFreq
   )
 {
-  EFI_STATUS          Status;
-  UINT8               HsTiming;
-  UINT8               HostCtrl2;
+  EFI_STATUS                 Status;
+  UINT8                      HsTiming;
+  SD_MMC_BUS_MODE            Timing;
+  SD_MMC_HC_PRIVATE_DATA     *Private;
+
+  Private = SD_MMC_HC_PRIVATE_FROM_THIS (PassThru);
 
   Status = EmmcSwitchToHS200 (PciIo, PassThru, Slot, Rca, ClockFreq, 8);
   if (EFI_ERROR (Status)) {
@@ -933,19 +924,10 @@ EmmcSwitchToHS400 (
   if (EFI_ERROR (Status)) {
     return Status;
   }
-  //
-  // Clean UHS Mode Select field of Host Control 2 reigster before update
-  //
-  HostCtrl2 = (UINT8)~0x7;
-  Status = SdMmcHcAndMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
-  if (EFI_ERROR (Status)) {
-    return Status;
-  }
-  //
-  // Set UHS Mode Select field of Host Control 2 reigster to HS400
-  //
-  HostCtrl2 = BIT0 | BIT2;
-  Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
+
+  Timing = SdMmcMmcHs400;
+
+  Status = SdMmcHcUhsSignaling (Private->ControllerHandle, PciIo, Slot, Timing);
   if (EFI_ERROR (Status)) {
     return Status;
   }
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
index 6ee9ed7..55b3564 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
@@ -784,8 +784,8 @@ SdCardSetBusMode (
   UINT8                        BusWidth;
   UINT8                        AccessMode;
   UINT8                        HostCtrl1;
-  UINT8                        HostCtrl2;
   UINT8                        SwitchResp[64];
+  SD_MMC_BUS_MODE              Timing;
   SD_MMC_HC_PRIVATE_DATA       *Private;
 
   Private = SD_MMC_HC_PRIVATE_FROM_THIS (PassThru);
@@ -817,18 +817,23 @@ SdCardSetBusMode (
   if (S18A && (Capability->Sdr104 != 0) && ((SwitchResp[13] & BIT3) != 0)) {
     ClockFreq = 208;
     AccessMode = 3;
+    Timing = SdMmcUhsSdr104;
   } else if (S18A && (Capability->Sdr50 != 0) && ((SwitchResp[13] & BIT2) != 0)) {
     ClockFreq = 100;
     AccessMode = 2;
+    Timing = SdMmcUhsSdr50;
   } else if (S18A && (Capability->Ddr50 != 0) && ((SwitchResp[13] & BIT4) != 0)) {
     ClockFreq = 50;
     AccessMode = 4;
+    Timing = SdMmcUhsDdr50;
   } else if ((SwitchResp[13] & BIT1) != 0) {
     ClockFreq = 50;
     AccessMode = 1;
+    Timing = SdMmcUhsSdr25;
   } else {
     ClockFreq = 25;
     AccessMode = 0;
+    Timing = SdMmcUhsSdr12;
   }
 
   Status = SdCardSwitch (PassThru, Slot, AccessMode, 0xF, 0xF, 0xF, TRUE, SwitchResp);
@@ -854,13 +859,7 @@ SdCardSetBusMode (
     }
   }
 
-  HostCtrl2 = (UINT8)~0x7;
-  Status = SdMmcHcAndMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
-  if (EFI_ERROR (Status)) {
-    return Status;
-  }
-  HostCtrl2 = AccessMode;
-  Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
+  Status = SdMmcHcUhsSignaling (Private->ControllerHandle, PciIo, Slot, Timing);
   if (EFI_ERROR (Status)) {
     return Status;
   }
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
index 923c55b..c3eec8b 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
@@ -1138,6 +1138,95 @@ SdMmcHcInitHost (
 }
 
 /**
+  Set SD Host Controler control 2 registry according to selected speed.
+
+  @param[in] ControllerHandle The handle of the controller.
+  @param[in] PciIo            The PCI IO protocol instance.
+  @param[in] Slot             The slot number of the SD card to send the command to.
+  @param[in] Timing           The timing to select.
+
+  @retval EFI_SUCCESS         The timing is set successfully.
+  @retval Others              The timing isn't set successfully.
+**/
+EFI_STATUS
+SdMmcHcUhsSignaling (
+  IN EFI_HANDLE             ControllerHandle,
+  IN EFI_PCI_IO_PROTOCOL    *PciIo,
+  IN UINT8                  Slot,
+  IN SD_MMC_BUS_MODE        Timing
+  )
+{
+  EFI_STATUS                 Status;
+  UINT8                      HostCtrl2;
+
+  HostCtrl2 = (UINT8)~SD_MMC_HC_CTRL_UHS_MASK;
+  Status = SdMmcHcAndMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  switch (Timing) {
+    case SdMmcUhsSdr12:
+      HostCtrl2 = SD_MMC_HC_CTRL_UHS_SDR12;
+      break;
+    case SdMmcUhsSdr25:
+      HostCtrl2 = SD_MMC_HC_CTRL_UHS_SDR25;
+      break;
+    case SdMmcUhsSdr50:
+      HostCtrl2 = SD_MMC_HC_CTRL_UHS_SDR50;
+      break;
+    case SdMmcUhsSdr104:
+      HostCtrl2 = SD_MMC_HC_CTRL_UHS_SDR104;
+      break;
+    case SdMmcUhsDdr50:
+      HostCtrl2 = SD_MMC_HC_CTRL_UHS_DDR50;
+      break;
+    case SdMmcMmcLegacy:
+      HostCtrl2 = SD_MMC_HC_CTRL_MMC_LEGACY;
+      break;
+    case SdMmcMmcHsSdr:
+      HostCtrl2 = SD_MMC_HC_CTRL_MMC_HS_SDR;
+      break;
+    case SdMmcMmcHsDdr:
+      HostCtrl2 = SD_MMC_HC_CTRL_MMC_HS_DDR;
+      break;
+    case SdMmcMmcHs200:
+      HostCtrl2 = SD_MMC_HC_CTRL_MMC_HS200;
+      break;
+    case SdMmcMmcHs400:
+      HostCtrl2 = SD_MMC_HC_CTRL_MMC_HS400;
+      break;
+    default:
+     HostCtrl2 = 0;
+     break;
+  }
+  Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
+    Status = mOverride->NotifyPhase (
+                          ControllerHandle,
+                          Slot,
+                          EdkiiSdMmcUhsSignaling,
+                          &Timing
+                          );
+    if (EFI_ERROR (Status)) {
+      DEBUG ((
+        DEBUG_ERROR,
+        "%a: SD/MMC uhs signaling notifier callback failed - %r\n",
+        __FUNCTION__,
+        Status
+        ));
+      return Status;
+    }
+  }
+
+  return EFI_SUCCESS;
+}
+
+/**
   Turn on/off LED.
 
   @param[in] PciIo          The PCI IO protocol instance.
-- 
2.7.4

[PATCH v4 3/4] MdeModulePkg/SdMmcPciHcDxe: Add SwitchClockFreqPost to SdMmcOverride

[Marcin Wojtas]

From: Tomasz Michalec <tm@semihalf.com>

Some SD Host Controlers need to do additional opperations after clock
frequency switch.

This patch add new callback type to NotifyPhase of the SdMmcOverride
protocol. It is called after SdMmcHcClockSupply.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 MdeModulePkg/Include/Protocol/SdMmcOverride.h   |  1 +
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c | 31 +++++++++++++++++---
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c   | 18 ++++++++++++
 3 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
index f948bef..6160b5b 100644
--- a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
+++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
@@ -48,6 +48,7 @@ typedef enum {
   EdkiiSdMmcInitHostPre,
   EdkiiSdMmcInitHostPost,
   EdkiiSdMmcUhsSignaling,
+  EdkiiSdMmcSwitchClockFreqPost,
 } EDKII_SD_MMC_PHASE_TYPE;
 
 /**
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
index db4e8a1..b75a9bb 100755
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
@@ -651,6 +651,7 @@ EmmcSwitchBusWidth (
   @param[in] Slot           The slot number of the SD card to send the command to.
   @param[in] Rca            The relative device address to be assigned.
   @param[in] HsTiming       The value to be written to HS_TIMING field of EXT_CSD register.
+  @param[in] Timing         The bus mode timing indicator.
   @param[in] ClockFreq      The max clock frequency to be set, the unit is MHz.
 
   @retval EFI_SUCCESS       The operation is done correctly.
@@ -664,6 +665,7 @@ EmmcSwitchClockFreq (
   IN UINT8                              Slot,
   IN UINT16                             Rca,
   IN UINT8                              HsTiming,
+  IN SD_MMC_BUS_MODE                    Timing,
   IN UINT32                             ClockFreq
   )
 {
@@ -706,6 +708,27 @@ EmmcSwitchClockFreq (
   // Convert the clock freq unit from MHz to KHz.
   //
   Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Private->Capability[Slot]);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
+    Status = mOverride->NotifyPhase (
+                          Private->ControllerHandle,
+                          Slot,
+                          EdkiiSdMmcSwitchClockFreqPost,
+                          &Timing
+                          );
+    if (EFI_ERROR (Status)) {
+      DEBUG ((
+        DEBUG_ERROR,
+        "%a: SD/MMC switch clock freq post notifier callback failed - %r\n",
+        __FUNCTION__,
+        Status
+        ));
+      return Status;
+    }
+  }
 
   return Status;
 }
@@ -775,7 +798,7 @@ EmmcSwitchToHighSpeed (
   }
 
   HsTiming = 1;
-  Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming, ClockFreq);
+  Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming, Timing, ClockFreq);
 
   return Status;
 }
@@ -863,7 +886,7 @@ EmmcSwitchToHS200 (
   Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_CLOCK_CTRL, sizeof (ClockCtrl), &ClockCtrl);
 
   HsTiming = 2;
-  Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming, ClockFreq);
+  Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming, Timing, ClockFreq);
   if (EFI_ERROR (Status)) {
     return Status;
   }
@@ -913,7 +936,7 @@ EmmcSwitchToHS400 (
   // Set to Hight Speed timing and set the clock frequency to a value less than 52MHz.
   //
   HsTiming = 1;
-  Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming, 52);
+  Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming, SdMmcMmcHsSdr, 52);
   if (EFI_ERROR (Status)) {
     return Status;
   }
@@ -933,7 +956,7 @@ EmmcSwitchToHS400 (
   }
 
   HsTiming = 3;
-  Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming, ClockFreq);
+  Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming, Timing, ClockFreq);
 
   return Status;
 }
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
index 55b3564..32fd416 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
@@ -869,6 +869,24 @@ SdCardSetBusMode (
     return Status;
   }
 
+  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
+    Status = mOverride->NotifyPhase (
+                          Private->ControllerHandle,
+                          Slot,
+                          EdkiiSdMmcSwitchClockFreqPost,
+                          &Timing
+                          );
+    if (EFI_ERROR (Status)) {
+      DEBUG ((
+        DEBUG_ERROR,
+        "%a: SD/MMC switch clock freq post notifier callback failed - %r\n",
+        __FUNCTION__,
+        Status
+        ));
+      return Status;
+    }
+  }
+
   if ((AccessMode == 3) || ((AccessMode == 2) && (Capability->TuningSDR50 != 0))) {
     Status = SdCardTuningClock (PciIo, PassThru, Slot);
     if (EFI_ERROR (Status)) {
-- 
2.7.4

[PATCH v4 4/4] MdeModulePkg/SdMmcPciHcDxe: Allow overriding base clock frequency

[Marcin Wojtas]

Some SdMmc host controllers are run by clocks with different
frequency than it is reflected in Capabilities Register 1.
It is allowed by SDHCI specification ver. 4.2 - if BaseClkFreq
field value of the Capability Register 1 is zero, the clock
frequency must be obtained via another method.

Because the bitfield is only 8 bits wide, a maximum value
that could be obtained from hardware is 255MHz.
In case the actual frequency exceeds 255MHz, the 8-bit BaseClkFreq
member of SD_MMC_HC_SLOT_CAP structure occurs to be not sufficient
to be used for setting the clock speed in SdMmcHcClockSupply
function.

This patch adds new UINT32 array ('BaseClkFreq[]') to
SD_MMC_HC_PRIVATE_DATA structure for specifying
the input clock speed for each slot of the host controller.
All routines that are used for clock configuration are
updated accordingly.

This patch also adds new IN OUT BaseClockFreq field
in the Capability callback of the SdMmcOverride,
protocol which allows to update BaseClkFreq value.

The patch reuses original commit from edk2-platforms:
20f6f144d3a8 ("Marvell/Drivers: XenonDxe: Allow overriding base clock
frequency")

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h |  6 +++++
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h   |  8 +++----
 MdeModulePkg/Include/Protocol/SdMmcOverride.h      |  7 ++++--
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c    |  4 ++--
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c      |  4 ++--
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c | 13 ++++++++++-
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   | 23 ++++++++++----------
 7 files changed, 43 insertions(+), 22 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
index c683600..8c1a589 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
@@ -118,6 +118,12 @@ typedef struct {
   UINT64                              MaxCurrent[SD_MMC_HC_MAX_SLOT];
 
   UINT32                              ControllerVersion;
+
+  //
+  // Some controllers may require to override base clock frequency
+  // value stored in Capabilities Register 1.
+  //
+  UINT32                              BaseClkFreq[SD_MMC_HC_MAX_SLOT];
 } SD_MMC_HC_PRIVATE_DATA;
 
 #define SD_MMC_HC_TRB_SIG             SIGNATURE_32 ('T', 'R', 'B', 'T')
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
index b47b0df..dd45cbd 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
@@ -423,7 +423,7 @@ SdMmcHcStopClock (
   @param[in] PciIo          The PCI IO protocol instance.
   @param[in] Slot           The slot number of the SD card to send the command to.
   @param[in] ClockFreq      The max clock frequency to be set. The unit is KHz.
-  @param[in] Capability     The capability of the slot.
+  @param[in] BaseClkFreq    The base clock frequency of host controller in MHz.
 
   @retval EFI_SUCCESS       The clock is supplied successfully.
   @retval Others            The clock isn't supplied successfully.
@@ -434,7 +434,7 @@ SdMmcHcClockSupply (
   IN EFI_PCI_IO_PROTOCOL    *PciIo,
   IN UINT8                  Slot,
   IN UINT64                 ClockFreq,
-  IN SD_MMC_HC_SLOT_CAP     Capability
+  IN UINT32                 BaseClkFreq
   );
 
 /**
@@ -482,7 +482,7 @@ SdMmcHcSetBusWidth (
 
   @param[in] PciIo          The PCI IO protocol instance.
   @param[in] Slot           The slot number of the SD card to send the command to.
-  @param[in] Capability     The capability of the slot.
+  @param[in] BaseClkFreq    The base clock frequency of host controller in MHz.
 
   @retval EFI_SUCCESS       The clock is supplied successfully.
   @retval Others            The clock isn't supplied successfully.
@@ -492,7 +492,7 @@ EFI_STATUS
 SdMmcHcInitClockFreq (
   IN EFI_PCI_IO_PROTOCOL    *PciIo,
   IN UINT8                  Slot,
-  IN SD_MMC_HC_SLOT_CAP     Capability
+  IN UINT32                 BaseClkFreq
   );
 
 /**
diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
index 6160b5b..0aaf258 100644
--- a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
+++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
@@ -22,7 +22,7 @@
 #define EDKII_SD_MMC_OVERRIDE_PROTOCOL_GUID \
   { 0xeaf9e3c1, 0xc9cd, 0x46db, { 0xa5, 0xe5, 0x5a, 0x12, 0x4c, 0x83, 0x23, 0x23 } }
 
-#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_VERSION    0x1
+#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_VERSION    0x2
 
 typedef struct _EDKII_SD_MMC_OVERRIDE EDKII_SD_MMC_OVERRIDE;
 
@@ -58,6 +58,8 @@ typedef enum {
   @param[in]      ControllerHandle      The EFI_HANDLE of the controller.
   @param[in]      Slot                  The 0 based slot index.
   @param[in,out]  SdMmcHcSlotCapability The SDHCI capability structure.
+  @param[in,out]  BaseClkFreq           The base clock frequency value that
+                                        optionally can be updated.
 
   @retval EFI_SUCCESS           The override function completed successfully.
   @retval EFI_NOT_FOUND         The specified controller or slot does not exist.
@@ -69,7 +71,8 @@ EFI_STATUS
 (EFIAPI * EDKII_SD_MMC_CAPABILITY) (
   IN      EFI_HANDLE                      ControllerHandle,
   IN      UINT8                           Slot,
-  IN  OUT VOID                            *SdMmcHcSlotCapability
+  IN OUT  VOID                            *SdMmcHcSlotCapability,
+  IN OUT  UINT32                          *BaseClkFreq
   );
 
 /**
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
index b75a9bb..2d3fb68 100755
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
@@ -707,7 +707,7 @@ EmmcSwitchClockFreq (
   //
   // Convert the clock freq unit from MHz to KHz.
   //
-  Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Private->Capability[Slot]);
+  Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Private->BaseClkFreq[Slot]);
   if (EFI_ERROR (Status)) {
     return Status;
   }
@@ -1007,7 +1007,7 @@ EmmcSetBusMode (
     return Status;
   }
 
-  ASSERT (Private->Capability[Slot].BaseClkFreq != 0);
+  ASSERT (Private->BaseClkFreq[Slot] != 0);
   //
   // Check if the Host Controller support 8bits bus width.
   //
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
index 32fd416..68485c8 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
@@ -864,7 +864,7 @@ SdCardSetBusMode (
     return Status;
   }
 
-  Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, *Capability);
+  Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Private->BaseClkFreq[Slot]);
   if (EFI_ERROR (Status)) {
     return Status;
   }
@@ -1064,7 +1064,7 @@ SdCardIdentification (
         goto Error;
       }
 
-      SdMmcHcInitClockFreq (PciIo, Slot, Private->Capability[Slot]);
+      SdMmcHcInitClockFreq (PciIo, Slot, Private->BaseClkFreq[Slot]);
 
       gBS->Stall (1000);
 
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
index bf9869d..a87f8de 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
@@ -625,11 +625,16 @@ SdMmcPciHcDriverBindingStart (
     if (EFI_ERROR (Status)) {
       continue;
     }
+
+    Private->BaseClkFreq[Slot] = Private->Capability[Slot].BaseClkFreq;
+
     if (mOverride != NULL && mOverride->Capability != NULL) {
       Status = mOverride->Capability (
                             Controller,
                             Slot,
-                            &Private->Capability[Slot]);
+                            &Private->Capability[Slot],
+                            &Private->BaseClkFreq[Slot]
+                            );
       if (EFI_ERROR (Status)) {
         DEBUG ((DEBUG_WARN, "%a: Failed to override capability - %r\n",
           __FUNCTION__, Status));
@@ -637,6 +642,12 @@ SdMmcPciHcDriverBindingStart (
       }
     }
     DumpCapabilityReg (Slot, &Private->Capability[Slot]);
+    DEBUG ((
+      DEBUG_INFO,
+      "Slot[%d] Base Clock Frequency: %dMHz\n",
+      Slot,
+      Private->BaseClkFreq[Slot]
+      ));
 
     Support64BitDma &= Private->Capability[Slot].SysBus64;
 
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
index c3eec8b..ddf6dcf 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
@@ -721,7 +721,7 @@ SdMmcHcStopClock (
   @param[in] PciIo          The PCI IO protocol instance.
   @param[in] Slot           The slot number of the SD card to send the command to.
   @param[in] ClockFreq      The max clock frequency to be set. The unit is KHz.
-  @param[in] Capability     The capability of the slot.
+  @param[in] BaseClkFreq    The base clock frequency of host controller in MHz.
 
   @retval EFI_SUCCESS       The clock is supplied successfully.
   @retval Others            The clock isn't supplied successfully.
@@ -732,11 +732,10 @@ SdMmcHcClockSupply (
   IN EFI_PCI_IO_PROTOCOL    *PciIo,
   IN UINT8                  Slot,
   IN UINT64                 ClockFreq,
-  IN SD_MMC_HC_SLOT_CAP     Capability
+  IN UINT32                 BaseClkFreq
   )
 {
   EFI_STATUS                Status;
-  UINT32                    BaseClkFreq;
   UINT32                    SettingFreq;
   UINT32                    Divisor;
   UINT32                    Remainder;
@@ -746,9 +745,8 @@ SdMmcHcClockSupply (
   //
   // Calculate a divisor for SD clock frequency
   //
-  ASSERT (Capability.BaseClkFreq != 0);
+  ASSERT (BaseClkFreq != 0);
 
-  BaseClkFreq = Capability.BaseClkFreq;
   if (ClockFreq == 0) {
     return EFI_INVALID_PARAMETER;
   }
@@ -940,7 +938,7 @@ SdMmcHcSetBusWidth (
 
   @param[in] PciIo          The PCI IO protocol instance.
   @param[in] Slot           The slot number of the SD card to send the command to.
-  @param[in] Capability     The capability of the slot.
+  @param[in] BaseClkFreq    The base clock frequency of host controller in MHz.
 
   @retval EFI_SUCCESS       The clock is supplied successfully.
   @retval Others            The clock isn't supplied successfully.
@@ -950,16 +948,19 @@ EFI_STATUS
 SdMmcHcInitClockFreq (
   IN EFI_PCI_IO_PROTOCOL    *PciIo,
   IN UINT8                  Slot,
-  IN SD_MMC_HC_SLOT_CAP     Capability
+  IN UINT32                 BaseClkFreq
   )
 {
   EFI_STATUS                Status;
   UINT32                    InitFreq;
 
   //
-  // Calculate a divisor for SD clock frequency
+  // According to SDHCI specification ver. 4.2, BaseClkFreq field value of
+  // the Capability Register 1 can be zero, which means a need for obtaining
+  // the clock frequency via another method. Fail in case it is not updated
+  // by SW at this point.
   //
-  if (Capability.BaseClkFreq == 0) {
+  if (BaseClkFreq == 0) {
     //
     // Don't support get Base Clock Frequency information via another method
     //
@@ -969,7 +970,7 @@ SdMmcHcInitClockFreq (
   // Supply 400KHz clock frequency at initialization phase.
   //
   InitFreq = 400;
-  Status = SdMmcHcClockSupply (PciIo, Slot, InitFreq, Capability);
+  Status = SdMmcHcClockSupply (PciIo, Slot, InitFreq, BaseClkFreq);
   return Status;
 }
 
@@ -1103,7 +1104,7 @@ SdMmcHcInitHost (
   PciIo = Private->PciIo;
   Capability = Private->Capability[Slot];
 
-  Status = SdMmcHcInitClockFreq (PciIo, Slot, Capability);
+  Status = SdMmcHcInitClockFreq (PciIo, Slot, Private->BaseClkFreq[Slot]);
   if (EFI_ERROR (Status)) {
     return Status;
   }
-- 
2.7.4

Re: [PATCH v4 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol

[Ard Biesheuvel]

On Sat, 10 Nov 2018 at 00:02, Marcin Wojtas <mw@semihalf.com> wrote:
>
> From: Tomasz Michalec <tm@semihalf.com>
>
> Some SD Host Controllers use different values in Host Control 2 Register
> to select UHS Mode. This patch adds a new UhsSignaling type routine to
> the NotifyPhase of the SdMmcOverride protocol.
>
> UHS signaling configuration is moved to a common, default routine
> (SdMmcHcUhsSignaling). After it is executed, the protocol producer
> can override the values if needed.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h | 34 ++++++++
>  MdeModulePkg/Include/Protocol/SdMmcOverride.h    | 17 ++++
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c  | 86 ++++++++-----------
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c    | 15 ++--
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 89 ++++++++++++++++++++
>  5 files changed, 181 insertions(+), 60 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> index 7e3f588..b47b0df 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> @@ -63,6 +63,21 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  #define SD_MMC_HC_CTRL_VER            0xFE
>
>  //
> +// SD Host Controller bits to HOST_CTRL2 register
> +//
> +#define SD_MMC_HC_CTRL_UHS_MASK       0x0007
> +#define SD_MMC_HC_CTRL_UHS_SDR12      0x0000
> +#define SD_MMC_HC_CTRL_UHS_SDR25      0x0001
> +#define SD_MMC_HC_CTRL_UHS_SDR50      0x0002
> +#define SD_MMC_HC_CTRL_UHS_SDR104     0x0003
> +#define SD_MMC_HC_CTRL_UHS_DDR50      0x0004
> +#define SD_MMC_HC_CTRL_MMC_LEGACY     0x0000
> +#define SD_MMC_HC_CTRL_MMC_HS_SDR     0x0001
> +#define SD_MMC_HC_CTRL_MMC_HS_DDR     0x0004
> +#define SD_MMC_HC_CTRL_MMC_HS200      0x0003
> +#define SD_MMC_HC_CTRL_MMC_HS400      0x0005
> +
> +//
>  // The transfer modes supported by SD Host Controller
>  // Simplified Spec 3.0 Table 1-2
>  //
> @@ -518,4 +533,23 @@ SdMmcHcInitTimeoutCtrl (
>    IN UINT8                  Slot
>    );
>
> +/**
> +  Set SD Host Controller control 2 registry according to selected speed.
> +
> +  @param[in] ControllerHandle The handle of the controller.
> +  @param[in] PciIo            The PCI IO protocol instance.
> +  @param[in] Slot             The slot number of the SD card to send the command to.
> +  @param[in] Timing           The timing to select.
> +
> +  @retval EFI_SUCCESS         The timing is set successfully.
> +  @retval Others              The timing isn't set successfully.
> +**/
> +EFI_STATUS
> +SdMmcHcUhsSignaling (
> +  IN EFI_HANDLE             ControllerHandle,
> +  IN EFI_PCI_IO_PROTOCOL    *PciIo,
> +  IN UINT8                  Slot,
> +  IN SD_MMC_BUS_MODE        Timing
> +  );
> +
>  #endif
> diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> index 8a7669e..f948bef 100644
> --- a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> @@ -26,11 +26,28 @@
>
>  typedef struct _EDKII_SD_MMC_OVERRIDE EDKII_SD_MMC_OVERRIDE;
>
> +//
> +// Bus timing modes
> +//
> +typedef enum {
> +  SdMmcUhsSdr12,
> +  SdMmcUhsSdr25,
> +  SdMmcUhsSdr50,
> +  SdMmcUhsSdr104,
> +  SdMmcUhsDdr50,
> +  SdMmcMmcLegacy,
> +  SdMmcMmcHsSdr,
> +  SdMmcMmcHsDdr,
> +  SdMmcMmcHs200,
> +  SdMmcMmcHs400,
> +} SD_MMC_BUS_MODE;
> +
>  typedef enum {
>    EdkiiSdMmcResetPre,
>    EdkiiSdMmcResetPost,
>    EdkiiSdMmcInitHostPre,
>    EdkiiSdMmcInitHostPost,
> +  EdkiiSdMmcUhsSignaling,
>  } EDKII_SD_MMC_PHASE_TYPE;
>
>  /**
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> index c5fd214..db4e8a1 100755
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> @@ -740,10 +740,13 @@ EmmcSwitchToHighSpeed (
>    IN UINT8                              BusWidth
>    )
>  {
> -  EFI_STATUS          Status;
> -  UINT8               HsTiming;
> -  UINT8               HostCtrl1;
> -  UINT8               HostCtrl2;
> +  EFI_STATUS              Status;
> +  UINT8                   HsTiming;
> +  UINT8                   HostCtrl1;
> +  SD_MMC_BUS_MODE         Timing;
> +  SD_MMC_HC_PRIVATE_DATA  *Private;
> +
> +  Private = SD_MMC_HC_PRIVATE_FROM_THIS (PassThru);
>
>    Status = EmmcSwitchBusWidth (PciIo, PassThru, Slot, Rca, IsDdr, BusWidth);
>    if (EFI_ERROR (Status)) {
> @@ -758,25 +761,15 @@ EmmcSwitchToHighSpeed (
>      return Status;
>    }
>
> -  //
> -  // Clean UHS Mode Select field of Host Control 2 reigster before update
> -  //
> -  HostCtrl2 = (UINT8)~0x7;
> -  Status = SdMmcHcAndMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
> -  if (EFI_ERROR (Status)) {
> -    return Status;
> -  }
> -  //
> -  // Set UHS Mode Select field of Host Control 2 reigster to SDR12/25/50
> -  //
>    if (IsDdr) {
> -    HostCtrl2 = BIT2;
> +    Timing = SdMmcMmcHsDdr;
>    } else if (ClockFreq == 52) {
> -    HostCtrl2 = BIT0;
> +    Timing = SdMmcMmcHsSdr;
>    } else {
> -    HostCtrl2 = 0;
> +    Timing = SdMmcMmcLegacy;
>    }
> -  Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
> +
> +  Status = SdMmcHcUhsSignaling (Private->ControllerHandle, PciIo, Slot, Timing);
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
> @@ -814,10 +807,13 @@ EmmcSwitchToHS200 (
>    IN UINT8                              BusWidth
>    )
>  {
> -  EFI_STATUS          Status;
> -  UINT8               HsTiming;
> -  UINT8               HostCtrl2;
> -  UINT16              ClockCtrl;
> +  EFI_STATUS               Status;
> +  UINT8                    HsTiming;
> +  UINT16                   ClockCtrl;
> +  SD_MMC_BUS_MODE          Timing;
> +  SD_MMC_HC_PRIVATE_DATA  *Private;
> +
> +  Private = SD_MMC_HC_PRIVATE_FROM_THIS (PassThru);
>
>    if ((BusWidth != 4) && (BusWidth != 8)) {
>      return EFI_INVALID_PARAMETER;
> @@ -837,22 +833,14 @@ EmmcSwitchToHS200 (
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
> -  //
> -  // Clean UHS Mode Select field of Host Control 2 reigster before update
> -  //
> -  HostCtrl2 = (UINT8)~0x7;
> -  Status = SdMmcHcAndMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
> -  if (EFI_ERROR (Status)) {
> -    return Status;
> -  }
> -  //
> -  // Set UHS Mode Select field of Host Control 2 reigster to SDR104
> -  //
> -  HostCtrl2 = BIT0 | BIT1;
> -  Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
> +
> +  Timing = SdMmcMmcHs200;
> +
> +  Status = SdMmcHcUhsSignaling (Private->ControllerHandle, PciIo, Slot, Timing);
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
> +
>    //
>    // Wait Internal Clock Stable in the Clock Control register to be 1 before set SD Clock Enable bit
>    //
> @@ -910,9 +898,12 @@ EmmcSwitchToHS400 (
>    IN UINT32                             ClockFreq
>    )
>  {
> -  EFI_STATUS          Status;
> -  UINT8               HsTiming;
> -  UINT8               HostCtrl2;
> +  EFI_STATUS                 Status;
> +  UINT8                      HsTiming;
> +  SD_MMC_BUS_MODE            Timing;
> +  SD_MMC_HC_PRIVATE_DATA     *Private;
> +
> +  Private = SD_MMC_HC_PRIVATE_FROM_THIS (PassThru);
>
>    Status = EmmcSwitchToHS200 (PciIo, PassThru, Slot, Rca, ClockFreq, 8);
>    if (EFI_ERROR (Status)) {
> @@ -933,19 +924,10 @@ EmmcSwitchToHS400 (
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
> -  //
> -  // Clean UHS Mode Select field of Host Control 2 reigster before update
> -  //
> -  HostCtrl2 = (UINT8)~0x7;
> -  Status = SdMmcHcAndMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
> -  if (EFI_ERROR (Status)) {
> -    return Status;
> -  }
> -  //
> -  // Set UHS Mode Select field of Host Control 2 reigster to HS400
> -  //
> -  HostCtrl2 = BIT0 | BIT2;
> -  Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
> +
> +  Timing = SdMmcMmcHs400;
> +
> +  Status = SdMmcHcUhsSignaling (Private->ControllerHandle, PciIo, Slot, Timing);
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> index 6ee9ed7..55b3564 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> @@ -784,8 +784,8 @@ SdCardSetBusMode (
>    UINT8                        BusWidth;
>    UINT8                        AccessMode;
>    UINT8                        HostCtrl1;
> -  UINT8                        HostCtrl2;
>    UINT8                        SwitchResp[64];
> +  SD_MMC_BUS_MODE              Timing;
>    SD_MMC_HC_PRIVATE_DATA       *Private;
>
>    Private = SD_MMC_HC_PRIVATE_FROM_THIS (PassThru);
> @@ -817,18 +817,23 @@ SdCardSetBusMode (
>    if (S18A && (Capability->Sdr104 != 0) && ((SwitchResp[13] & BIT3) != 0)) {
>      ClockFreq = 208;
>      AccessMode = 3;
> +    Timing = SdMmcUhsSdr104;
>    } else if (S18A && (Capability->Sdr50 != 0) && ((SwitchResp[13] & BIT2) != 0)) {
>      ClockFreq = 100;
>      AccessMode = 2;
> +    Timing = SdMmcUhsSdr50;
>    } else if (S18A && (Capability->Ddr50 != 0) && ((SwitchResp[13] & BIT4) != 0)) {
>      ClockFreq = 50;
>      AccessMode = 4;
> +    Timing = SdMmcUhsDdr50;
>    } else if ((SwitchResp[13] & BIT1) != 0) {
>      ClockFreq = 50;
>      AccessMode = 1;
> +    Timing = SdMmcUhsSdr25;
>    } else {
>      ClockFreq = 25;
>      AccessMode = 0;
> +    Timing = SdMmcUhsSdr12;
>    }
>
>    Status = SdCardSwitch (PassThru, Slot, AccessMode, 0xF, 0xF, 0xF, TRUE, SwitchResp);
> @@ -854,13 +859,7 @@ SdCardSetBusMode (
>      }
>    }
>
> -  HostCtrl2 = (UINT8)~0x7;
> -  Status = SdMmcHcAndMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
> -  if (EFI_ERROR (Status)) {
> -    return Status;
> -  }
> -  HostCtrl2 = AccessMode;
> -  Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
> +  Status = SdMmcHcUhsSignaling (Private->ControllerHandle, PciIo, Slot, Timing);
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> index 923c55b..c3eec8b 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> @@ -1138,6 +1138,95 @@ SdMmcHcInitHost (
>  }
>
>  /**
> +  Set SD Host Controler control 2 registry according to selected speed.
> +
> +  @param[in] ControllerHandle The handle of the controller.
> +  @param[in] PciIo            The PCI IO protocol instance.
> +  @param[in] Slot             The slot number of the SD card to send the command to.
> +  @param[in] Timing           The timing to select.
> +
> +  @retval EFI_SUCCESS         The timing is set successfully.
> +  @retval Others              The timing isn't set successfully.
> +**/
> +EFI_STATUS
> +SdMmcHcUhsSignaling (
> +  IN EFI_HANDLE             ControllerHandle,
> +  IN EFI_PCI_IO_PROTOCOL    *PciIo,
> +  IN UINT8                  Slot,
> +  IN SD_MMC_BUS_MODE        Timing
> +  )
> +{
> +  EFI_STATUS                 Status;
> +  UINT8                      HostCtrl2;
> +
> +  HostCtrl2 = (UINT8)~SD_MMC_HC_CTRL_UHS_MASK;
> +  Status = SdMmcHcAndMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  switch (Timing) {
> +    case SdMmcUhsSdr12:
> +      HostCtrl2 = SD_MMC_HC_CTRL_UHS_SDR12;
> +      break;
> +    case SdMmcUhsSdr25:
> +      HostCtrl2 = SD_MMC_HC_CTRL_UHS_SDR25;
> +      break;
> +    case SdMmcUhsSdr50:
> +      HostCtrl2 = SD_MMC_HC_CTRL_UHS_SDR50;
> +      break;
> +    case SdMmcUhsSdr104:
> +      HostCtrl2 = SD_MMC_HC_CTRL_UHS_SDR104;
> +      break;
> +    case SdMmcUhsDdr50:
> +      HostCtrl2 = SD_MMC_HC_CTRL_UHS_DDR50;
> +      break;
> +    case SdMmcMmcLegacy:
> +      HostCtrl2 = SD_MMC_HC_CTRL_MMC_LEGACY;
> +      break;
> +    case SdMmcMmcHsSdr:
> +      HostCtrl2 = SD_MMC_HC_CTRL_MMC_HS_SDR;
> +      break;
> +    case SdMmcMmcHsDdr:
> +      HostCtrl2 = SD_MMC_HC_CTRL_MMC_HS_DDR;
> +      break;
> +    case SdMmcMmcHs200:
> +      HostCtrl2 = SD_MMC_HC_CTRL_MMC_HS200;
> +      break;
> +    case SdMmcMmcHs400:
> +      HostCtrl2 = SD_MMC_HC_CTRL_MMC_HS400;
> +      break;
> +    default:
> +     HostCtrl2 = 0;
> +     break;
> +  }
> +  Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> +    Status = mOverride->NotifyPhase (
> +                          ControllerHandle,
> +                          Slot,
> +                          EdkiiSdMmcUhsSignaling,
> +                          &Timing
> +                          );
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((
> +        DEBUG_ERROR,
> +        "%a: SD/MMC uhs signaling notifier callback failed - %r\n",
> +        __FUNCTION__,
> +        Status
> +        ));
> +      return Status;
> +    }
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
>    Turn on/off LED.
>
>    @param[in] PciIo          The PCI IO protocol instance.
> --
> 2.7.4
>

Re: [PATCH v4 3/4] MdeModulePkg/SdMmcPciHcDxe: Add SwitchClockFreqPost to SdMmcOverride

[Ard Biesheuvel]

On Sat, 10 Nov 2018 at 00:02, Marcin Wojtas <mw@semihalf.com> wrote:
>
> From: Tomasz Michalec <tm@semihalf.com>
>
> Some SD Host Controlers need to do additional opperations

operations

> after clock
> frequency switch.
>
> This patch add new callback type to NotifyPhase of the SdMmcOverride
> protocol. It is called after SdMmcHcClockSupply.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>


> ---
>  MdeModulePkg/Include/Protocol/SdMmcOverride.h   |  1 +
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c | 31 +++++++++++++++++---
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c   | 18 ++++++++++++
>  3 files changed, 46 insertions(+), 4 deletions(-)
>
> diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> index f948bef..6160b5b 100644
> --- a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> @@ -48,6 +48,7 @@ typedef enum {
>    EdkiiSdMmcInitHostPre,
>    EdkiiSdMmcInitHostPost,
>    EdkiiSdMmcUhsSignaling,
> +  EdkiiSdMmcSwitchClockFreqPost,
>  } EDKII_SD_MMC_PHASE_TYPE;
>
>  /**
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> index db4e8a1..b75a9bb 100755
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> @@ -651,6 +651,7 @@ EmmcSwitchBusWidth (
>    @param[in] Slot           The slot number of the SD card to send the command to.
>    @param[in] Rca            The relative device address to be assigned.
>    @param[in] HsTiming       The value to be written to HS_TIMING field of EXT_CSD register.
> +  @param[in] Timing         The bus mode timing indicator.
>    @param[in] ClockFreq      The max clock frequency to be set, the unit is MHz.
>
>    @retval EFI_SUCCESS       The operation is done correctly.
> @@ -664,6 +665,7 @@ EmmcSwitchClockFreq (
>    IN UINT8                              Slot,
>    IN UINT16                             Rca,
>    IN UINT8                              HsTiming,
> +  IN SD_MMC_BUS_MODE                    Timing,
>    IN UINT32                             ClockFreq
>    )
>  {
> @@ -706,6 +708,27 @@ EmmcSwitchClockFreq (
>    // Convert the clock freq unit from MHz to KHz.
>    //
>    Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Private->Capability[Slot]);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> +    Status = mOverride->NotifyPhase (
> +                          Private->ControllerHandle,
> +                          Slot,
> +                          EdkiiSdMmcSwitchClockFreqPost,
> +                          &Timing
> +                          );
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((
> +        DEBUG_ERROR,
> +        "%a: SD/MMC switch clock freq post notifier callback failed - %r\n",
> +        __FUNCTION__,
> +        Status
> +        ));
> +      return Status;
> +    }
> +  }
>
>    return Status;
>  }
> @@ -775,7 +798,7 @@ EmmcSwitchToHighSpeed (
>    }
>
>    HsTiming = 1;
> -  Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming, ClockFreq);
> +  Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming, Timing, ClockFreq);
>
>    return Status;
>  }
> @@ -863,7 +886,7 @@ EmmcSwitchToHS200 (
>    Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_CLOCK_CTRL, sizeof (ClockCtrl), &ClockCtrl);
>
>    HsTiming = 2;
> -  Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming, ClockFreq);
> +  Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming, Timing, ClockFreq);
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
> @@ -913,7 +936,7 @@ EmmcSwitchToHS400 (
>    // Set to Hight Speed timing and set the clock frequency to a value less than 52MHz.
>    //
>    HsTiming = 1;
> -  Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming, 52);
> +  Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming, SdMmcMmcHsSdr, 52);
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
> @@ -933,7 +956,7 @@ EmmcSwitchToHS400 (
>    }
>
>    HsTiming = 3;
> -  Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming, ClockFreq);
> +  Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming, Timing, ClockFreq);
>
>    return Status;
>  }
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> index 55b3564..32fd416 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> @@ -869,6 +869,24 @@ SdCardSetBusMode (
>      return Status;
>    }
>
> +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> +    Status = mOverride->NotifyPhase (
> +                          Private->ControllerHandle,
> +                          Slot,
> +                          EdkiiSdMmcSwitchClockFreqPost,
> +                          &Timing
> +                          );
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((
> +        DEBUG_ERROR,
> +        "%a: SD/MMC switch clock freq post notifier callback failed - %r\n",
> +        __FUNCTION__,
> +        Status
> +        ));
> +      return Status;
> +    }
> +  }
> +
>    if ((AccessMode == 3) || ((AccessMode == 2) && (Capability->TuningSDR50 != 0))) {
>      Status = SdCardTuningClock (PciIo, PassThru, Slot);
>      if (EFI_ERROR (Status)) {
> --
> 2.7.4
>

Re: [PATCH v4 0/4] SdMmcOverride extension

[Wu, Hao A]

Hi Marcin,

The code changes look good to me.

Could you please grant me some time for some additional tests for these
patches?
I will inform you with the results sometime next week. Thanks in advance.

Best Regards,
Hao Wu


> -----Original Message-----
> From: Marcin Wojtas [mailto:mw@semihalf.com]
> Sent: Saturday, November 10, 2018 7:01 AM
> To: edk2-devel@lists.01.org
> Cc: leif.lindholm@linaro.org; Wu, Hao A; Kinney, Michael D; Gao, Liming;
> ard.biesheuvel@linaro.org; nadavh@marvell.com; mw@semihalf.com;
> jsd@semihalf.com; tm@semihalf.com; jaz@semihalf.com
> Subject: [PATCH v4 0/4] SdMmcOverride extension
> 
> Hi,
> 
> Although I could've waited for Hao's remarks, I think it may
> be better if he takes a look at much cleaner code, which
> addresses v3 review comments.
> The newest version of the patchset cleans-up significantly
> patches 2&3 by removing code duplication and other minor
> improvements.
> 
> Patches are available in the github:
> https://github.com/MarvellEmbeddedProcessors/edk2-open-
> platform/commits/sdmmc-override-upstream-r20181109
> 
> Please note that extending SdMmcOverride protocol was impacting
> so far the only user of it (Synquacer controller). In paralel
> edk2-platforms patchset, a patch can be found:
> ("Silicon/SynQuacer/PlatformDxe: adjust to updated SdMmcOverride")
> which adjust to the new API.
> https://github.com/MarvellEmbeddedProcessors/edk2-open-
> platform/commits/xenon-upstream-r20181109
> 
> I'm looking forward to the comments and remarks.
> 
> Best regards,
> Marcin
> 
> Changelog:
> v3->v4
> * 2/4:
>   - avoid duplication by calling SdMmcOverride callback in
> SdMmcHcUhsSignaling
> 
> * 3/4:
>   - avoid duplication by calling SdMmcOverride callback in
> EmmcSwitchClockFreq
> 
> * 4/4:
>   - add Ard's RB
> 
> v2->v3
> * 1/4:
>   - rename new parameter to PhaseData
>   - add Ard's RB
> 
> * 2/4:
>   - s/Controler/Controller/
>   - remove all references to MMC_SDR_50 mode
>   - rename and reorder MMC bus modes
>   - rename enum: s/SD_MMC_UHS_TIMING/SD_MMC_BUS_MODE/
>     and move it to protocol header in order to drop including private one
>   - fix if condition in EmmcSwitchToHighSpeed
>   - call SdMmcHcUhsSignaling unconditionally before SdMmcOverride
>     callback, so that protocol producer can optionally modify only quirky
>     timing mode values.
> 
> *4/4
>   - bump protocol version to 2
>   - remove redundant assert from SdMmcPciHcDriverBindingStart
>     (BaseClkFreq is already checked in SdMmcHcInitClockFreq)
>   - update comment in SdMmcHcInitClockFreq
>   - restore original DumpCapabilityReg and append
> 
> v1 -> v2
> * Rebase onto newest master
> * 1/4 [new patch] - preparation for extending NotifyPhase
> * 2/4 - UhsSignaling as a part of NotifyPhase
> * 3/4 - SwitchClockFreqPost as a part of NotifyPhase
> * 4/4 - Allow updating BaseClkFreq via Capability instead of the
>         independent callback.
> 
> 
> Marcin Wojtas (2):
>   MdeModulePkg/SdMmcPciHcDxe: Add an optional parameter in NotifyPhase
>   MdeModulePkg/SdMmcPciHcDxe: Allow overriding base clock frequency
> 
> Tomasz Michalec (2):
>   MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride
> protocol
>   MdeModulePkg/SdMmcPciHcDxe: Add SwitchClockFreqPost to
> SdMmcOverride
> 
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h |   6 +
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h   |  42 ++++++-
>  MdeModulePkg/Include/Protocol/SdMmcOverride.h      |  29 ++++-
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c    | 121 ++++++++++-
> --------
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c      |  35 ++++--
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c |  13 +-
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   | 124
> +++++++++++++++++---
>  7 files changed, 280 insertions(+), 90 deletions(-)
> 
> --
> 2.7.4

Re: [PATCH v4 0/4] SdMmcOverride extension

[Marcin Wojtas]

Hi Hao,

wt., 13 lis 2018 o 08:38 Wu, Hao A <hao.a.wu@intel.com> napisał(a):
>
> Hi Marcin,
>
> The code changes look good to me.
>
> Could you please grant me some time for some additional tests for these
> patches?

Sure.

> I will inform you with the results sometime next week. Thanks in advance.
>

Ard gave his RB to 2/4 and 3/4. Moreover he pointed a typo in 3/4
commit message - should I repost, or could those be included when
applying the patches (unless you don't request any code change, of
course)?

Best regards,
Marcin

> Best Regards,
> Hao Wu
>
>
> > -----Original Message-----
> > From: Marcin Wojtas [mailto:mw@semihalf.com]
> > Sent: Saturday, November 10, 2018 7:01 AM
> > To: edk2-devel@lists.01.org
> > Cc: leif.lindholm@linaro.org; Wu, Hao A; Kinney, Michael D; Gao, Liming;
> > ard.biesheuvel@linaro.org; nadavh@marvell.com; mw@semihalf.com;
> > jsd@semihalf.com; tm@semihalf.com; jaz@semihalf.com
> > Subject: [PATCH v4 0/4] SdMmcOverride extension
> >
> > Hi,
> >
> > Although I could've waited for Hao's remarks, I think it may
> > be better if he takes a look at much cleaner code, which
> > addresses v3 review comments.
> > The newest version of the patchset cleans-up significantly
> > patches 2&3 by removing code duplication and other minor
> > improvements.
> >
> > Patches are available in the github:
> > https://github.com/MarvellEmbeddedProcessors/edk2-open-
> > platform/commits/sdmmc-override-upstream-r20181109
> >
> > Please note that extending SdMmcOverride protocol was impacting
> > so far the only user of it (Synquacer controller). In paralel
> > edk2-platforms patchset, a patch can be found:
> > ("Silicon/SynQuacer/PlatformDxe: adjust to updated SdMmcOverride")
> > which adjust to the new API.
> > https://github.com/MarvellEmbeddedProcessors/edk2-open-
> > platform/commits/xenon-upstream-r20181109
> >
> > I'm looking forward to the comments and remarks.
> >
> > Best regards,
> > Marcin
> >
> > Changelog:
> > v3->v4
> > * 2/4:
> >   - avoid duplication by calling SdMmcOverride callback in
> > SdMmcHcUhsSignaling
> >
> > * 3/4:
> >   - avoid duplication by calling SdMmcOverride callback in
> > EmmcSwitchClockFreq
> >
> > * 4/4:
> >   - add Ard's RB
> >
> > v2->v3
> > * 1/4:
> >   - rename new parameter to PhaseData
> >   - add Ard's RB
> >
> > * 2/4:
> >   - s/Controler/Controller/
> >   - remove all references to MMC_SDR_50 mode
> >   - rename and reorder MMC bus modes
> >   - rename enum: s/SD_MMC_UHS_TIMING/SD_MMC_BUS_MODE/
> >     and move it to protocol header in order to drop including private one
> >   - fix if condition in EmmcSwitchToHighSpeed
> >   - call SdMmcHcUhsSignaling unconditionally before SdMmcOverride
> >     callback, so that protocol producer can optionally modify only quirky
> >     timing mode values.
> >
> > *4/4
> >   - bump protocol version to 2
> >   - remove redundant assert from SdMmcPciHcDriverBindingStart
> >     (BaseClkFreq is already checked in SdMmcHcInitClockFreq)
> >   - update comment in SdMmcHcInitClockFreq
> >   - restore original DumpCapabilityReg and append
> >
> > v1 -> v2
> > * Rebase onto newest master
> > * 1/4 [new patch] - preparation for extending NotifyPhase
> > * 2/4 - UhsSignaling as a part of NotifyPhase
> > * 3/4 - SwitchClockFreqPost as a part of NotifyPhase
> > * 4/4 - Allow updating BaseClkFreq via Capability instead of the
> >         independent callback.
> >
> >
> > Marcin Wojtas (2):
> >   MdeModulePkg/SdMmcPciHcDxe: Add an optional parameter in NotifyPhase
> >   MdeModulePkg/SdMmcPciHcDxe: Allow overriding base clock frequency
> >
> > Tomasz Michalec (2):
> >   MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride
> > protocol
> >   MdeModulePkg/SdMmcPciHcDxe: Add SwitchClockFreqPost to
> > SdMmcOverride
> >
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h |   6 +
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h   |  42 ++++++-
> >  MdeModulePkg/Include/Protocol/SdMmcOverride.h      |  29 ++++-
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c    | 121 ++++++++++-
> > --------
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c      |  35 ++++--
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c |  13 +-
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   | 124
> > +++++++++++++++++---
> >  7 files changed, 280 insertions(+), 90 deletions(-)
> >
> > --
> > 2.7.4
>

Re: [PATCH v4 0/4] SdMmcOverride extension

[Wu, Hao A]

> -----Original Message-----
> From: Marcin Wojtas [mailto:mw@semihalf.com]
> Sent: Tuesday, November 13, 2018 4:25 PM
> To: Wu, Hao A
> Cc: edk2-devel-01; Leif Lindholm; Kinney, Michael D; Gao, Liming; Ard
> Biesheuvel; nadavh@marvell.com; jsd@semihalf.com; Tomasz Michalec;
> Grzegorz Jaszczyk
> Subject: Re: [PATCH v4 0/4] SdMmcOverride extension
> 
> Hi Hao,
> 
> wt., 13 lis 2018 o 08:38 Wu, Hao A <hao.a.wu@intel.com> napisał(a):
> >
> > Hi Marcin,
> >
> > The code changes look good to me.
> >
> > Could you please grant me some time for some additional tests for these
> > patches?
> 
> Sure.
> 
> > I will inform you with the results sometime next week. Thanks in advance.
> >
> 
> Ard gave his RB to 2/4 and 3/4. Moreover he pointed a typo in 3/4
> commit message - should I repost, or could those be included when
> applying the patches (unless you don't request any code change, of
> course)?

If no other issues, I will help to address the typo issue in patch 3/4.

A little concern on the commit author information if I modify the commit, but
I will let you know if a repost from you is needed.

Best Regards,
Hao Wu

> 
> Best regards,
> Marcin
> 
> > Best Regards,
> > Hao Wu
> >
> >
> > > -----Original Message-----
> > > From: Marcin Wojtas [mailto:mw@semihalf.com]
> > > Sent: Saturday, November 10, 2018 7:01 AM
> > > To: edk2-devel@lists.01.org
> > > Cc: leif.lindholm@linaro.org; Wu, Hao A; Kinney, Michael D; Gao, Liming;
> > > ard.biesheuvel@linaro.org; nadavh@marvell.com; mw@semihalf.com;
> > > jsd@semihalf.com; tm@semihalf.com; jaz@semihalf.com
> > > Subject: [PATCH v4 0/4] SdMmcOverride extension
> > >
> > > Hi,
> > >
> > > Although I could've waited for Hao's remarks, I think it may
> > > be better if he takes a look at much cleaner code, which
> > > addresses v3 review comments.
> > > The newest version of the patchset cleans-up significantly
> > > patches 2&3 by removing code duplication and other minor
> > > improvements.
> > >
> > > Patches are available in the github:
> > > https://github.com/MarvellEmbeddedProcessors/edk2-open-
> > > platform/commits/sdmmc-override-upstream-r20181109
> > >
> > > Please note that extending SdMmcOverride protocol was impacting
> > > so far the only user of it (Synquacer controller). In paralel
> > > edk2-platforms patchset, a patch can be found:
> > > ("Silicon/SynQuacer/PlatformDxe: adjust to updated SdMmcOverride")
> > > which adjust to the new API.
> > > https://github.com/MarvellEmbeddedProcessors/edk2-open-
> > > platform/commits/xenon-upstream-r20181109
> > >
> > > I'm looking forward to the comments and remarks.
> > >
> > > Best regards,
> > > Marcin
> > >
> > > Changelog:
> > > v3->v4
> > > * 2/4:
> > >   - avoid duplication by calling SdMmcOverride callback in
> > > SdMmcHcUhsSignaling
> > >
> > > * 3/4:
> > >   - avoid duplication by calling SdMmcOverride callback in
> > > EmmcSwitchClockFreq
> > >
> > > * 4/4:
> > >   - add Ard's RB
> > >
> > > v2->v3
> > > * 1/4:
> > >   - rename new parameter to PhaseData
> > >   - add Ard's RB
> > >
> > > * 2/4:
> > >   - s/Controler/Controller/
> > >   - remove all references to MMC_SDR_50 mode
> > >   - rename and reorder MMC bus modes
> > >   - rename enum: s/SD_MMC_UHS_TIMING/SD_MMC_BUS_MODE/
> > >     and move it to protocol header in order to drop including private one
> > >   - fix if condition in EmmcSwitchToHighSpeed
> > >   - call SdMmcHcUhsSignaling unconditionally before SdMmcOverride
> > >     callback, so that protocol producer can optionally modify only quirky
> > >     timing mode values.
> > >
> > > *4/4
> > >   - bump protocol version to 2
> > >   - remove redundant assert from SdMmcPciHcDriverBindingStart
> > >     (BaseClkFreq is already checked in SdMmcHcInitClockFreq)
> > >   - update comment in SdMmcHcInitClockFreq
> > >   - restore original DumpCapabilityReg and append
> > >
> > > v1 -> v2
> > > * Rebase onto newest master
> > > * 1/4 [new patch] - preparation for extending NotifyPhase
> > > * 2/4 - UhsSignaling as a part of NotifyPhase
> > > * 3/4 - SwitchClockFreqPost as a part of NotifyPhase
> > > * 4/4 - Allow updating BaseClkFreq via Capability instead of the
> > >         independent callback.
> > >
> > >
> > > Marcin Wojtas (2):
> > >   MdeModulePkg/SdMmcPciHcDxe: Add an optional parameter in
> NotifyPhase
> > >   MdeModulePkg/SdMmcPciHcDxe: Allow overriding base clock frequency
> > >
> > > Tomasz Michalec (2):
> > >   MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride
> > > protocol
> > >   MdeModulePkg/SdMmcPciHcDxe: Add SwitchClockFreqPost to
> > > SdMmcOverride
> > >
> > >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h |   6 +
> > >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h   |  42 ++++++-
> > >  MdeModulePkg/Include/Protocol/SdMmcOverride.h      |  29 ++++-
> > >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c    | 121
> ++++++++++-
> > > --------
> > >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c      |  35 ++++--
> > >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c |  13 +-
> > >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   | 124
> > > +++++++++++++++++---
> > >  7 files changed, 280 insertions(+), 90 deletions(-)
> > >
> > > --
> > > 2.7.4
> >

Re: [PATCH v4 0/4] SdMmcOverride extension

[Wu, Hao A]

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Wu, Hao A
> Sent: Tuesday, November 13, 2018 4:29 PM
> To: Marcin Wojtas
> Cc: Tomasz Michalec; Grzegorz Jaszczyk; edk2-devel-01; Gao, Liming;
> nadavh@marvell.com; Kinney, Michael D
> Subject: Re: [edk2] [PATCH v4 0/4] SdMmcOverride extension
> 
> > -----Original Message-----
> > From: Marcin Wojtas [mailto:mw@semihalf.com]
> > Sent: Tuesday, November 13, 2018 4:25 PM
> > To: Wu, Hao A
> > Cc: edk2-devel-01; Leif Lindholm; Kinney, Michael D; Gao, Liming; Ard
> > Biesheuvel; nadavh@marvell.com; jsd@semihalf.com; Tomasz Michalec;
> > Grzegorz Jaszczyk
> > Subject: Re: [PATCH v4 0/4] SdMmcOverride extension
> >
> > Hi Hao,
> >
> > wt., 13 lis 2018 o 08:38 Wu, Hao A <hao.a.wu@intel.com> napisał(a):
> > >
> > > Hi Marcin,
> > >
> > > The code changes look good to me.
> > >
> > > Could you please grant me some time for some additional tests for these
> > > patches?
> >
> > Sure.
> >
> > > I will inform you with the results sometime next week. Thanks in advance.
> > >
> >
> > Ard gave his RB to 2/4 and 3/4. Moreover he pointed a typo in 3/4
> > commit message - should I repost, or could those be included when
> > applying the patches (unless you don't request any code change, of
> > course)?
> 
> If no other issues, I will help to address the typo issue in patch 3/4.
> 
> A little concern on the commit author information if I modify the commit, but
> I will let you know if a repost from you is needed.
> 
> Best Regards,
> Hao Wu

For the series:
Reviewed-by: Hao Wu <hao.a.wu@intel.com>

Pushed at 49c9953425..7f3b0bad4b


Best Regards,
Hao Wu

> 
> >
> > Best regards,
> > Marcin
> >
> > > Best Regards,
> > > Hao Wu
> > >
> > >
> > > > -----Original Message-----
> > > > From: Marcin Wojtas [mailto:mw@semihalf.com]
> > > > Sent: Saturday, November 10, 2018 7:01 AM
> > > > To: edk2-devel@lists.01.org
> > > > Cc: leif.lindholm@linaro.org; Wu, Hao A; Kinney, Michael D; Gao, Liming;
> > > > ard.biesheuvel@linaro.org; nadavh@marvell.com; mw@semihalf.com;
> > > > jsd@semihalf.com; tm@semihalf.com; jaz@semihalf.com
> > > > Subject: [PATCH v4 0/4] SdMmcOverride extension
> > > >
> > > > Hi,
> > > >
> > > > Although I could've waited for Hao's remarks, I think it may
> > > > be better if he takes a look at much cleaner code, which
> > > > addresses v3 review comments.
> > > > The newest version of the patchset cleans-up significantly
> > > > patches 2&3 by removing code duplication and other minor
> > > > improvements.
> > > >
> > > > Patches are available in the github:
> > > > https://github.com/MarvellEmbeddedProcessors/edk2-open-
> > > > platform/commits/sdmmc-override-upstream-r20181109
> > > >
> > > > Please note that extending SdMmcOverride protocol was impacting
> > > > so far the only user of it (Synquacer controller). In paralel
> > > > edk2-platforms patchset, a patch can be found:
> > > > ("Silicon/SynQuacer/PlatformDxe: adjust to updated SdMmcOverride")
> > > > which adjust to the new API.
> > > > https://github.com/MarvellEmbeddedProcessors/edk2-open-
> > > > platform/commits/xenon-upstream-r20181109
> > > >
> > > > I'm looking forward to the comments and remarks.
> > > >
> > > > Best regards,
> > > > Marcin
> > > >
> > > > Changelog:
> > > > v3->v4
> > > > * 2/4:
> > > >   - avoid duplication by calling SdMmcOverride callback in
> > > > SdMmcHcUhsSignaling
> > > >
> > > > * 3/4:
> > > >   - avoid duplication by calling SdMmcOverride callback in
> > > > EmmcSwitchClockFreq
> > > >
> > > > * 4/4:
> > > >   - add Ard's RB
> > > >
> > > > v2->v3
> > > > * 1/4:
> > > >   - rename new parameter to PhaseData
> > > >   - add Ard's RB
> > > >
> > > > * 2/4:
> > > >   - s/Controler/Controller/
> > > >   - remove all references to MMC_SDR_50 mode
> > > >   - rename and reorder MMC bus modes
> > > >   - rename enum: s/SD_MMC_UHS_TIMING/SD_MMC_BUS_MODE/
> > > >     and move it to protocol header in order to drop including private one
> > > >   - fix if condition in EmmcSwitchToHighSpeed
> > > >   - call SdMmcHcUhsSignaling unconditionally before SdMmcOverride
> > > >     callback, so that protocol producer can optionally modify only quirky
> > > >     timing mode values.
> > > >
> > > > *4/4
> > > >   - bump protocol version to 2
> > > >   - remove redundant assert from SdMmcPciHcDriverBindingStart
> > > >     (BaseClkFreq is already checked in SdMmcHcInitClockFreq)
> > > >   - update comment in SdMmcHcInitClockFreq
> > > >   - restore original DumpCapabilityReg and append
> > > >
> > > > v1 -> v2
> > > > * Rebase onto newest master
> > > > * 1/4 [new patch] - preparation for extending NotifyPhase
> > > > * 2/4 - UhsSignaling as a part of NotifyPhase
> > > > * 3/4 - SwitchClockFreqPost as a part of NotifyPhase
> > > > * 4/4 - Allow updating BaseClkFreq via Capability instead of the
> > > >         independent callback.
> > > >
> > > >
> > > > Marcin Wojtas (2):
> > > >   MdeModulePkg/SdMmcPciHcDxe: Add an optional parameter in
> > NotifyPhase
> > > >   MdeModulePkg/SdMmcPciHcDxe: Allow overriding base clock
> frequency
> > > >
> > > > Tomasz Michalec (2):
> > > >   MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to
> SdMmcOverride
> > > > protocol
> > > >   MdeModulePkg/SdMmcPciHcDxe: Add SwitchClockFreqPost to
> > > > SdMmcOverride
> > > >
> > > >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h |   6 +
> > > >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h   |  42
> ++++++-
> > > >  MdeModulePkg/Include/Protocol/SdMmcOverride.h      |  29 ++++-
> > > >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c    | 121
> > ++++++++++-
> > > > --------
> > > >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c      |  35 ++++--
> > > >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c |  13 +-
> > > >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   | 124
> > > > +++++++++++++++++---
> > > >  7 files changed, 280 insertions(+), 90 deletions(-)
> > > >
> > > > --
> > > > 2.7.4
> > >
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH v4 0/4] SdMmcOverride extension

[Marcin Wojtas]

wt., 20 lis 2018 o 06:59 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
> > Wu, Hao A
> > Sent: Tuesday, November 13, 2018 4:29 PM
> > To: Marcin Wojtas
> > Cc: Tomasz Michalec; Grzegorz Jaszczyk; edk2-devel-01; Gao, Liming;
> > nadavh@marvell.com; Kinney, Michael D
> > Subject: Re: [edk2] [PATCH v4 0/4] SdMmcOverride extension
> >
> > > -----Original Message-----
> > > From: Marcin Wojtas [mailto:mw@semihalf.com]
> > > Sent: Tuesday, November 13, 2018 4:25 PM
> > > To: Wu, Hao A
> > > Cc: edk2-devel-01; Leif Lindholm; Kinney, Michael D; Gao, Liming; Ard
> > > Biesheuvel; nadavh@marvell.com; jsd@semihalf.com; Tomasz Michalec;
> > > Grzegorz Jaszczyk
> > > Subject: Re: [PATCH v4 0/4] SdMmcOverride extension
> > >
> > > Hi Hao,
> > >
> > > wt., 13 lis 2018 o 08:38 Wu, Hao A <hao.a.wu@intel.com> napisał(a):
> > > >
> > > > Hi Marcin,
> > > >
> > > > The code changes look good to me.
> > > >
> > > > Could you please grant me some time for some additional tests for these
> > > > patches?
> > >
> > > Sure.
> > >
> > > > I will inform you with the results sometime next week. Thanks in advance.
> > > >
> > >
> > > Ard gave his RB to 2/4 and 3/4. Moreover he pointed a typo in 3/4
> > > commit message - should I repost, or could those be included when
> > > applying the patches (unless you don't request any code change, of
> > > course)?
> >
> > If no other issues, I will help to address the typo issue in patch 3/4.
> >
> > A little concern on the commit author information if I modify the commit, but
> > I will let you know if a repost from you is needed.
> >
> > Best Regards,
> > Hao Wu
>
> For the series:
> Reviewed-by: Hao Wu <hao.a.wu@intel.com>
>
> Pushed at 49c9953425..7f3b0bad4b
>

Thanks a lot!

Marcin

>
> Best Regards,
> Hao Wu
>
> >
> > >
> > > Best regards,
> > > Marcin
> > >
> > > > Best Regards,
> > > > Hao Wu
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Marcin Wojtas [mailto:mw@semihalf.com]
> > > > > Sent: Saturday, November 10, 2018 7:01 AM
> > > > > To: edk2-devel@lists.01.org
> > > > > Cc: leif.lindholm@linaro.org; Wu, Hao A; Kinney, Michael D; Gao, Liming;
> > > > > ard.biesheuvel@linaro.org; nadavh@marvell.com; mw@semihalf.com;
> > > > > jsd@semihalf.com; tm@semihalf.com; jaz@semihalf.com
> > > > > Subject: [PATCH v4 0/4] SdMmcOverride extension
> > > > >
> > > > > Hi,
> > > > >
> > > > > Although I could've waited for Hao's remarks, I think it may
> > > > > be better if he takes a look at much cleaner code, which
> > > > > addresses v3 review comments.
> > > > > The newest version of the patchset cleans-up significantly
> > > > > patches 2&3 by removing code duplication and other minor
> > > > > improvements.
> > > > >
> > > > > Patches are available in the github:
> > > > > https://github.com/MarvellEmbeddedProcessors/edk2-open-
> > > > > platform/commits/sdmmc-override-upstream-r20181109
> > > > >
> > > > > Please note that extending SdMmcOverride protocol was impacting
> > > > > so far the only user of it (Synquacer controller). In paralel
> > > > > edk2-platforms patchset, a patch can be found:
> > > > > ("Silicon/SynQuacer/PlatformDxe: adjust to updated SdMmcOverride")
> > > > > which adjust to the new API.
> > > > > https://github.com/MarvellEmbeddedProcessors/edk2-open-
> > > > > platform/commits/xenon-upstream-r20181109
> > > > >
> > > > > I'm looking forward to the comments and remarks.
> > > > >
> > > > > Best regards,
> > > > > Marcin
> > > > >
> > > > > Changelog:
> > > > > v3->v4
> > > > > * 2/4:
> > > > >   - avoid duplication by calling SdMmcOverride callback in
> > > > > SdMmcHcUhsSignaling
> > > > >
> > > > > * 3/4:
> > > > >   - avoid duplication by calling SdMmcOverride callback in
> > > > > EmmcSwitchClockFreq
> > > > >
> > > > > * 4/4:
> > > > >   - add Ard's RB
> > > > >
> > > > > v2->v3
> > > > > * 1/4:
> > > > >   - rename new parameter to PhaseData
> > > > >   - add Ard's RB
> > > > >
> > > > > * 2/4:
> > > > >   - s/Controler/Controller/
> > > > >   - remove all references to MMC_SDR_50 mode
> > > > >   - rename and reorder MMC bus modes
> > > > >   - rename enum: s/SD_MMC_UHS_TIMING/SD_MMC_BUS_MODE/
> > > > >     and move it to protocol header in order to drop including private one
> > > > >   - fix if condition in EmmcSwitchToHighSpeed
> > > > >   - call SdMmcHcUhsSignaling unconditionally before SdMmcOverride
> > > > >     callback, so that protocol producer can optionally modify only quirky
> > > > >     timing mode values.
> > > > >
> > > > > *4/4
> > > > >   - bump protocol version to 2
> > > > >   - remove redundant assert from SdMmcPciHcDriverBindingStart
> > > > >     (BaseClkFreq is already checked in SdMmcHcInitClockFreq)
> > > > >   - update comment in SdMmcHcInitClockFreq
> > > > >   - restore original DumpCapabilityReg and append
> > > > >
> > > > > v1 -> v2
> > > > > * Rebase onto newest master
> > > > > * 1/4 [new patch] - preparation for extending NotifyPhase
> > > > > * 2/4 - UhsSignaling as a part of NotifyPhase
> > > > > * 3/4 - SwitchClockFreqPost as a part of NotifyPhase
> > > > > * 4/4 - Allow updating BaseClkFreq via Capability instead of the
> > > > >         independent callback.
> > > > >
> > > > >
> > > > > Marcin Wojtas (2):
> > > > >   MdeModulePkg/SdMmcPciHcDxe: Add an optional parameter in
> > > NotifyPhase
> > > > >   MdeModulePkg/SdMmcPciHcDxe: Allow overriding base clock
> > frequency
> > > > >
> > > > > Tomasz Michalec (2):
> > > > >   MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to
> > SdMmcOverride
> > > > > protocol
> > > > >   MdeModulePkg/SdMmcPciHcDxe: Add SwitchClockFreqPost to
> > > > > SdMmcOverride
> > > > >
> > > > >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h |   6 +
> > > > >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h   |  42
> > ++++++-
> > > > >  MdeModulePkg/Include/Protocol/SdMmcOverride.h      |  29 ++++-
> > > > >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c    | 121
> > > ++++++++++-
> > > > > --------
> > > > >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c      |  35 ++++--
> > > > >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c |  13 +-
> > > > >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   | 124
> > > > > +++++++++++++++++---
> > > > >  7 files changed, 280 insertions(+), 90 deletions(-)
> > > > >
> > > > > --
> > > > > 2.7.4
> > > >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel