[PATCH v2 1/2] MdeModulePkg/SdMmcPciHcDxe: Declare V4 64 bit address capability

[PATCH v2 1/2] MdeModulePkg/SdMmcPciHcDxe: Declare V4 64 bit address capability

[Ashish Singhal]

Add capability declaration for V4.x 64 bit system address support.
This would be used for host controllers working in version 4. Enable
64 bit DMA support in PCI layer if V3 or V4 64 bit support is
enabled in host capability register.

The usage of this new field does not need a guard for version check as
spec for previous SDMMC versions defines this field as reserved with
default value of 0.

Change-Id: I64fcb674ec566c46a37ea6597ae06cb194625cae
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ashish Singhal <ashishsingha@nvidia.com>
---
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c |  4 ++--
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   |  3 ++-
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h   | 10 +++++-----
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
index bf9869d..1c18ea4 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
@@ -617,7 +617,6 @@ SdMmcPciHcDriverBindingStart (
     }
   }
 
-  Support64BitDma = TRUE;
   for (Slot = FirstBar; Slot < (FirstBar + SlotNum); Slot++) {
     Private->Slot[Slot].Enable = TRUE;
 
@@ -638,7 +637,8 @@ SdMmcPciHcDriverBindingStart (
     }
     DumpCapabilityReg (Slot, &Private->Capability[Slot]);
 
-    Support64BitDma &= Private->Capability[Slot].SysBus64;
+    Support64BitDma = (Private->Capability[Slot].SysBus64V3 |
+                       Private->Capability[Slot].SysBus64V4);
 
     Status = SdMmcHcGetMaxCurrent (PciIo, Slot, &Private->MaxCurrent[Slot]);
     if (EFI_ERROR (Status)) {
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
index bedc968..e506875 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
@@ -45,7 +45,8 @@ DumpCapabilityReg (
   DEBUG ((DEBUG_INFO, "   Voltage 3.3       %a\n", Capability->Voltage33 ? "TRUE" : "FALSE"));
   DEBUG ((DEBUG_INFO, "   Voltage 3.0       %a\n", Capability->Voltage30 ? "TRUE" : "FALSE"));
   DEBUG ((DEBUG_INFO, "   Voltage 1.8       %a\n", Capability->Voltage18 ? "TRUE" : "FALSE"));
-  DEBUG ((DEBUG_INFO, "   64-bit Sys Bus    %a\n", Capability->SysBus64 ? "TRUE" : "FALSE"));
+  DEBUG ((DEBUG_INFO, "   V4 64-bit Sys Bus %a\n", Capability->SysBus64V4 ? "TRUE" : "FALSE"));
+  DEBUG ((DEBUG_INFO, "   V3 64-bit Sys Bus %a\n", Capability->SysBus64V3 ? "TRUE" : "FALSE"));
   DEBUG ((DEBUG_INFO, "   Async Interrupt   %a\n", Capability->AsyncInt ? "TRUE" : "FALSE"));
   DEBUG ((DEBUG_INFO, "   SlotType          "));
   if (Capability->SlotType == 0x00) {
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
index 7e3f588..cc138fc 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
@@ -114,24 +114,24 @@ typedef struct {
   UINT32   Voltage33:1;       // bit 24
   UINT32   Voltage30:1;       // bit 25
   UINT32   Voltage18:1;       // bit 26
-  UINT32   Reserved3:1;       // bit 27
-  UINT32   SysBus64:1;        // bit 28
+  UINT32   SysBus64V4:1;      // bit 27
+  UINT32   SysBus64V3:1;      // bit 28
   UINT32   AsyncInt:1;        // bit 29
   UINT32   SlotType:2;        // bit 30:31
   UINT32   Sdr50:1;           // bit 32
   UINT32   Sdr104:1;          // bit 33
   UINT32   Ddr50:1;           // bit 34
-  UINT32   Reserved4:1;       // bit 35
+  UINT32   Reserved3:1;       // bit 35
   UINT32   DriverTypeA:1;     // bit 36
   UINT32   DriverTypeC:1;     // bit 37
   UINT32   DriverTypeD:1;     // bit 38
   UINT32   DriverType4:1;     // bit 39
   UINT32   TimerCount:4;      // bit 40:43
-  UINT32   Reserved5:1;       // bit 44
+  UINT32   Reserved4:1;       // bit 44
   UINT32   TuningSDR50:1;     // bit 45
   UINT32   RetuningMod:2;     // bit 46:47
   UINT32   ClkMultiplier:8;   // bit 48:55
-  UINT32   Reserved6:7;       // bit 56:62
+  UINT32   Reserved5:7;       // bit 56:62
   UINT32   Hs400:1;           // bit 63
 } SD_MMC_HC_SLOT_CAP;
 
-- 
2.7.4

[PATCH v2 2/2] MdeModulePkg/SdMmcPciHcDxe: Add V4 64bit SDMA and ADMA2 support.

[Ashish Singhal]

If V4 64 bit address mode is enabled in compatibility register,
program controller to enable V4 host mode.
Use appropriate ADMA2 descriptors supporting 64 bit addresses.
Use appropriate registers for SDMA mode operation.

Change-Id: I1f6c984368988e51999eb289aa29677f9b0cdf49
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ashish Singhal <ashishsingha@nvidia.com>
---
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h |   4 +-
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   | 273 +++++++++++++++++----
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h   |  28 ++-
 3 files changed, 260 insertions(+), 45 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
index c683600..22795df 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
@@ -2,6 +2,7 @@
 
   Provides some data structure definitions used by the SD/MMC host controller driver.
 
+Copyright (c) 2018, NVIDIA CORPORATION. All rights reserved.
 Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD License
@@ -144,7 +145,8 @@ typedef struct {
   BOOLEAN                             Started;
   UINT64                              Timeout;
 
-  SD_MMC_HC_ADMA_DESC_LINE            *AdmaDesc;
+  SD_MMC_HC_ADMA_32_DESC_LINE         *Adma32Desc;
+  SD_MMC_HC_ADMA_64_DESC_LINE         *Adma64Desc;
   EFI_PHYSICAL_ADDRESS                AdmaDescPhy;
   VOID                                *AdmaMap;
   UINT32                              AdmaPages;
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
index e506875..9fef3fb 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
@@ -4,6 +4,7 @@
 
   It would expose EFI_SD_MMC_PASS_THRU_PROTOCOL for upper layer use.
 
+  Copyright (c) 2018, NVIDIA CORPORATION. All rights reserved.
   Copyright (c) 2015 - 2017, Intel Corporation. All rights reserved.<BR>
   This program and the accompanying materials
   are licensed and made available under the terms and conditions of the BSD License
@@ -418,6 +419,36 @@ SdMmcHcWaitMmioSet (
 }
 
 /**
+  Get the controller version information from the specified slot.
+
+  @param[in]  PciIo           The PCI IO protocol instance.
+  @param[in]  Slot            The slot number of the SD card to send the command to.
+  @param[out] Version         The buffer to store the version information.
+
+  @retval EFI_SUCCESS         The operation executes successfully.
+  @retval Others              The operation fails.
+
+**/
+EFI_STATUS
+SdMmcHcGetControllerVersion (
+  IN     EFI_PCI_IO_PROTOCOL  *PciIo,
+  IN     UINT8                Slot,
+     OUT UINT16               *Version
+  )
+{
+  EFI_STATUS                Status;
+
+  Status = SdMmcHcRwMmio (PciIo, Slot, SD_MMC_HC_CTRL_VER, TRUE, sizeof (UINT16), Version);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  *Version &= 0xFF;
+
+  return EFI_SUCCESS;
+}
+
+/**
   Software reset the specified SD/MMC host controller and enable all interrupts.
 
   @param[in] Private        A pointer to the SD_MMC_HC_PRIVATE_DATA instance.
@@ -776,18 +807,18 @@ SdMmcHcClockSupply (
 
   DEBUG ((DEBUG_INFO, "BaseClkFreq %dMHz Divisor %d ClockFreq %dKhz\n", BaseClkFreq, Divisor, ClockFreq));
 
-  Status = SdMmcHcRwMmio (PciIo, Slot, SD_MMC_HC_CTRL_VER, TRUE, sizeof (ControllerVer), &ControllerVer);
+  Status = SdMmcHcGetControllerVersion (PciIo, Slot, &ControllerVer);
   if (EFI_ERROR (Status)) {
     return Status;
   }
   //
   // Set SDCLK Frequency Select and Internal Clock Enable fields in Clock Control register.
   //
-  if (((ControllerVer & 0xFF) >= SD_MMC_HC_CTRL_VER_300) &&
-      ((ControllerVer & 0xFF) <= SD_MMC_HC_CTRL_VER_420)) {
+  if ((ControllerVer >= SD_MMC_HC_CTRL_VER_300) &&
+      (ControllerVer <= SD_MMC_HC_CTRL_VER_420)) {
     ASSERT (Divisor <= 0x3FF);
     ClockCtrl = ((Divisor & 0xFF) << 8) | ((Divisor & 0x300) >> 2);
-  } else if (((ControllerVer & 0xFF) == 0) || ((ControllerVer & 0xFF) == 1)) {
+  } else if ((ControllerVer == 0) || (ControllerVer == 1)) {
     //
     // Only the most significant bit can be used as divisor.
     //
@@ -935,6 +966,54 @@ SdMmcHcSetBusWidth (
 }
 
 /**
+  Configure V4 64 bit system address support at initialization.
+
+  @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.
+
+  @retval EFI_SUCCESS       The clock is supplied successfully.
+
+**/
+EFI_STATUS
+SdMmcHcV4Init64BitSupport (
+  IN EFI_PCI_IO_PROTOCOL    *PciIo,
+  IN UINT8                  Slot,
+  IN SD_MMC_HC_SLOT_CAP     Capability
+  )
+{
+  EFI_STATUS                Status;
+  UINT16                    ControllerVer;
+  UINT16                    HostCtrl2;
+
+  //
+  // Check if V4 64bit support is available
+  //
+  Status = SdMmcHcGetControllerVersion (PciIo, Slot, &ControllerVer);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  if (ControllerVer >= SD_MMC_HC_CTRL_VER_400) {
+    HostCtrl2 = SD_MMC_HC_V4_EN;
+    //
+    // Check if V4 64bit support is available
+    //
+    if (Capability.SysBus64V4 == TRUE) {
+      HostCtrl2 |= SD_MMC_HC_64_ADDR_EN;
+      DEBUG ((DEBUG_INFO, "Enabled V4 64 bit system bus support\n"));
+    }
+    Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
+    if (EFI_ERROR (Status)) {
+      return Status;
+    }
+    DEBUG ((DEBUG_INFO, "Enabled V4 64 bit system bus support\n"));
+  }
+
+  return EFI_SUCCESS;
+}
+
+/**
   Supply SD/MMC card with lowest clock frequency at initialization.
 
   @param[in] PciIo          The PCI IO protocol instance.
@@ -1101,6 +1180,11 @@ SdMmcHcInitHost (
   PciIo = Private->PciIo;
   Capability = Private->Capability[Slot];
 
+  Status = SdMmcHcV4Init64BitSupport (PciIo, Slot, Capability);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
   Status = SdMmcHcInitClockFreq (PciIo, Slot, Capability);
   if (EFI_ERROR (Status)) {
     return Status;
@@ -1169,7 +1253,7 @@ SdMmcHcLedOnOff (
 /**
   Build ADMA descriptor table for transfer.
 
-  Refer to SD Host Controller Simplified spec 3.0 Section 1.13 for details.
+  Refer to SD Host Controller Simplified spec 4.2 Section 1.13 for details.
 
   @param[in] Trb            The pointer to the SD_MMC_HC_TRB instance.
 
@@ -1187,49 +1271,69 @@ BuildAdmaDescTable (
   UINT64                    Entries;
   UINT32                    Index;
   UINT64                    Remaining;
-  UINT32                    Address;
+  UINT64                    Address;
   UINTN                     TableSize;
   EFI_PCI_IO_PROTOCOL       *PciIo;
   EFI_STATUS                Status;
   UINTN                     Bytes;
+  UINT16                    ControllerVer;
+  BOOLEAN                   AddressingMode64 = FALSE;
+  UINTN                     DescSize = sizeof (SD_MMC_HC_ADMA_32_DESC_LINE);
+  VOID                      *AdmaDesc = NULL;
 
   Data    = Trb->DataPhy;
   DataLen = Trb->DataLen;
   PciIo   = Trb->Private->PciIo;
+
+  //
+  // Detect whether 64bit addressing is supported.
   //
-  // Only support 32bit ADMA Descriptor Table
+  Status = SdMmcHcGetControllerVersion (PciIo, Trb->Slot, &ControllerVer);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+  if (ControllerVer >= SD_MMC_HC_CTRL_VER_400) {
+    Status = SdMmcHcCheckMmioSet(PciIo, Trb->Slot, SD_MMC_HC_HOST_CTRL2, 0x2,
+                                 SD_MMC_HC_V4_EN|SD_MMC_HC_64_ADDR_EN, SD_MMC_HC_V4_EN|SD_MMC_HC_64_ADDR_EN);
+    if (!EFI_ERROR (Status)) {
+      AddressingMode64 = TRUE;
+      DescSize = sizeof (SD_MMC_HC_ADMA_64_DESC_LINE);
+    }
+  }
   //
-  if ((Data >= 0x100000000ul) || ((Data + DataLen) > 0x100000000ul)) {
+  // Check for valid ranges in 32bit ADMA Descriptor Table
+  //
+  if (AddressingMode64 == FALSE &&
+      ((Data >= 0x100000000ul) || ((Data + DataLen) > 0x100000000ul))) {
     return EFI_INVALID_PARAMETER;
   }
   //
   // Address field shall be set on 32-bit boundary (Lower 2-bit is always set to 0)
-  // for 32-bit address descriptor table.
   //
   if ((Data & (BIT0 | BIT1)) != 0) {
     DEBUG ((DEBUG_INFO, "The buffer [0x%x] to construct ADMA desc is not aligned to 4 bytes boundary!\n", Data));
   }
 
   Entries   = DivU64x32 ((DataLen + ADMA_MAX_DATA_PER_LINE - 1), ADMA_MAX_DATA_PER_LINE);
-  TableSize = (UINTN)MultU64x32 (Entries, sizeof (SD_MMC_HC_ADMA_DESC_LINE));
+  TableSize = (UINTN)MultU64x32 (Entries, DescSize);
   Trb->AdmaPages = (UINT32)EFI_SIZE_TO_PAGES (TableSize);
   Status = PciIo->AllocateBuffer (
                     PciIo,
                     AllocateAnyPages,
                     EfiBootServicesData,
                     EFI_SIZE_TO_PAGES (TableSize),
-                    (VOID **)&Trb->AdmaDesc,
+                    (VOID **)&AdmaDesc,
                     0
                     );
   if (EFI_ERROR (Status)) {
     return EFI_OUT_OF_RESOURCES;
   }
-  ZeroMem (Trb->AdmaDesc, TableSize);
+  ZeroMem (AdmaDesc, TableSize);
   Bytes  = TableSize;
   Status = PciIo->Map (
                     PciIo,
                     EfiPciIoOperationBusMasterCommonBuffer,
-                    Trb->AdmaDesc,
+                    AdmaDesc,
                     &Bytes,
                     &Trb->AdmaDescPhy,
                     &Trb->AdmaMap
@@ -1242,12 +1346,13 @@ BuildAdmaDescTable (
     PciIo->FreeBuffer (
              PciIo,
              EFI_SIZE_TO_PAGES (TableSize),
-             Trb->AdmaDesc
+             AdmaDesc
              );
     return EFI_OUT_OF_RESOURCES;
   }
 
-  if ((UINT64)(UINTN)Trb->AdmaDescPhy > 0x100000000ul) {
+  if ((AddressingMode64 == FALSE) &&
+      (UINT64)(UINTN)Trb->AdmaDescPhy > 0x100000000ul) {
     //
     // The ADMA doesn't support 64bit addressing.
     //
@@ -1258,25 +1363,49 @@ BuildAdmaDescTable (
     PciIo->FreeBuffer (
       PciIo,
       EFI_SIZE_TO_PAGES (TableSize),
-      Trb->AdmaDesc
+      AdmaDesc
     );
     return EFI_DEVICE_ERROR;
   }
 
   Remaining = DataLen;
-  Address   = (UINT32)Data;
+  Address   = Data;
+  if (AddressingMode64 == FALSE) {
+    Trb->Adma32Desc = AdmaDesc;
+    Trb->Adma64Desc = NULL;
+  } else {
+    Trb->Adma64Desc = AdmaDesc;
+    Trb->Adma32Desc = NULL;
+  }
   for (Index = 0; Index < Entries; Index++) {
-    if (Remaining <= ADMA_MAX_DATA_PER_LINE) {
-      Trb->AdmaDesc[Index].Valid = 1;
-      Trb->AdmaDesc[Index].Act   = 2;
-      Trb->AdmaDesc[Index].Length  = (UINT16)Remaining;
-      Trb->AdmaDesc[Index].Address = Address;
-      break;
+    if (AddressingMode64 == FALSE) {
+      if (Remaining < ADMA_MAX_DATA_PER_LINE) {
+        Trb->Adma32Desc[Index].Valid = 1;
+        Trb->Adma32Desc[Index].Act   = 2;
+        Trb->Adma32Desc[Index].Length  = (UINT16)Remaining;
+        Trb->Adma32Desc[Index].Address = (UINT32)Address;
+        break;
+      } else {
+        Trb->Adma32Desc[Index].Valid = 1;
+        Trb->Adma32Desc[Index].Act   = 2;
+        Trb->Adma32Desc[Index].Length  = 0;
+        Trb->Adma32Desc[Index].Address = (UINT32)Address;
+      }
     } else {
-      Trb->AdmaDesc[Index].Valid = 1;
-      Trb->AdmaDesc[Index].Act   = 2;
-      Trb->AdmaDesc[Index].Length  = 0;
-      Trb->AdmaDesc[Index].Address = Address;
+      if (Remaining < ADMA_MAX_DATA_PER_LINE) {
+        Trb->Adma64Desc[Index].Valid = 1;
+        Trb->Adma64Desc[Index].Act   = 2;
+        Trb->Adma64Desc[Index].Length  = (UINT16)Remaining;
+        Trb->Adma64Desc[Index].LowerAddress = (UINT32)(Address & MAX_UINT32);
+        Trb->Adma64Desc[Index].UpperAddress = (UINT32)(Address>>32);
+        break;
+      } else {
+        Trb->Adma64Desc[Index].Valid = 1;
+        Trb->Adma64Desc[Index].Act   = 2;
+        Trb->Adma64Desc[Index].Length  = 0;
+        Trb->Adma64Desc[Index].LowerAddress = (UINT32)(Address & MAX_UINT32);
+        Trb->Adma64Desc[Index].UpperAddress = (UINT32)(Address>>32);
+      }
     }
 
     Remaining -= ADMA_MAX_DATA_PER_LINE;
@@ -1286,7 +1415,7 @@ BuildAdmaDescTable (
   //
   // Set the last descriptor line as end of descriptor table
   //
-  Trb->AdmaDesc[Index].End = 1;
+  AddressingMode64 ? (Trb->Adma64Desc[Index].End = 1) : (Trb->Adma32Desc[Index].End = 1);
   return EFI_SUCCESS;
 }
 
@@ -1430,11 +1559,18 @@ SdMmcFreeTrb (
       Trb->AdmaMap
     );
   }
-  if (Trb->AdmaDesc != NULL) {
+  if (Trb->Adma32Desc != NULL) {
     PciIo->FreeBuffer (
       PciIo,
       Trb->AdmaPages,
-      Trb->AdmaDesc
+      Trb->Adma32Desc
+    );
+  }
+  if (Trb->Adma64Desc != NULL) {
+    PciIo->FreeBuffer (
+      PciIo,
+      Trb->AdmaPages,
+      Trb->Adma64Desc
     );
   }
   if (Trb->DataMap != NULL) {
@@ -1574,12 +1710,14 @@ SdMmcExecTrb (
   UINT16                              Cmd;
   UINT16                              IntStatus;
   UINT32                              Argument;
-  UINT16                              BlkCount;
+  UINT32                              BlkCount;
   UINT16                              BlkSize;
   UINT16                              TransMode;
   UINT8                               HostCtrl1;
-  UINT32                              SdmaAddr;
+  UINT64                              SdmaAddr;
   UINT64                              AdmaAddr;
+  UINT16                              ControllerVer;
+  BOOLEAN                             AddressingMode64 = FALSE;
 
   Packet = Trb->Packet;
   PciIo  = Trb->Private->PciIo;
@@ -1612,13 +1750,33 @@ SdMmcExecTrb (
 
   SdMmcHcLedOnOff (PciIo, Trb->Slot, TRUE);
 
+  Status = SdMmcHcGetControllerVersion (PciIo, Trb->Slot, &ControllerVer);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+  if (ControllerVer >= SD_MMC_HC_CTRL_VER_400) {
+    Status = SdMmcHcCheckMmioSet(PciIo, Trb->Slot, SD_MMC_HC_HOST_CTRL2, 0x2,
+                                 SD_MMC_HC_V4_EN|SD_MMC_HC_64_ADDR_EN, SD_MMC_HC_V4_EN|SD_MMC_HC_64_ADDR_EN);
+    if (!EFI_ERROR (Status)) {
+      AddressingMode64 = TRUE;
+    }
+  }
+
   if (Trb->Mode == SdMmcSdmaMode) {
-    if ((UINT64)(UINTN)Trb->DataPhy >= 0x100000000ul) {
+    if ((AddressingMode64 == FALSE) &&
+        ((UINT64)(UINTN)Trb->DataPhy >= 0x100000000ul)) {
       return EFI_INVALID_PARAMETER;
     }
 
-    SdmaAddr = (UINT32)(UINTN)Trb->DataPhy;
-    Status   = SdMmcHcRwMmio (PciIo, Trb->Slot, SD_MMC_HC_SDMA_ADDR, FALSE, sizeof (SdmaAddr), &SdmaAddr);
+    SdmaAddr = (UINT64)(UINTN)Trb->DataPhy;
+
+    if (ControllerVer >= SD_MMC_HC_CTRL_VER_400) {
+      Status = SdMmcHcRwMmio (PciIo, Trb->Slot, SD_MMC_HC_ADMA_SYS_ADDR, FALSE, sizeof (UINT64), &SdmaAddr);
+    }
+    else {
+      Status = SdMmcHcRwMmio (PciIo, Trb->Slot, SD_MMC_HC_SDMA_ADDR, FALSE, sizeof (UINT32), &SdmaAddr);
+    }
+
     if (EFI_ERROR (Status)) {
       return Status;
     }
@@ -1648,9 +1806,14 @@ SdMmcExecTrb (
     //
     // Calcuate Block Count.
     //
-    BlkCount = (UINT16)(Trb->DataLen / Trb->BlockSize);
+    BlkCount = (Trb->DataLen / Trb->BlockSize);
+  }
+  if (ControllerVer >= SD_MMC_HC_CTRL_VER_410) {
+    Status = SdMmcHcRwMmio (PciIo, Trb->Slot, SD_MMC_HC_SDMA_ADDR, FALSE, sizeof (UINT32), &BlkCount);
+  }
+  else {
+    Status = SdMmcHcRwMmio (PciIo, Trb->Slot, SD_MMC_HC_BLK_COUNT, FALSE, sizeof (UINT16), &BlkCount);
   }
-  Status   = SdMmcHcRwMmio (PciIo, Trb->Slot, SD_MMC_HC_BLK_COUNT, FALSE, sizeof (BlkCount), &BlkCount);
   if (EFI_ERROR (Status)) {
     return Status;
   }
@@ -1746,10 +1909,11 @@ SdMmcCheckTrbResult (
   EFI_SD_MMC_PASS_THRU_COMMAND_PACKET *Packet;
   UINT16                              IntStatus;
   UINT32                              Response[4];
-  UINT32                              SdmaAddr;
+  UINT64                              SdmaAddr;
   UINT8                               Index;
   UINT8                               SwReset;
   UINT32                              PioLength;
+  UINT16                              ControllerVer;
 
   SwReset = 0;
   Packet  = Trb->Packet;
@@ -1870,19 +2034,42 @@ SdMmcCheckTrbResult (
     //
     // Update SDMA Address register.
     //
-    SdmaAddr = SD_MMC_SDMA_ROUND_UP ((UINT32)(UINTN)Trb->DataPhy, SD_MMC_SDMA_BOUNDARY);
-    Status   = SdMmcHcRwMmio (
+    SdmaAddr = SD_MMC_SDMA_ROUND_UP ((UINTN)Trb->DataPhy, SD_MMC_SDMA_BOUNDARY);
+
+    Status = SdMmcHcGetControllerVersion (
+               Private->PciIo,
+               Trb->Slot,
+               &ControllerVer
+               );
+    if (EFI_ERROR (Status)) {
+      return Status;
+    }
+
+    if (ControllerVer >= SD_MMC_HC_CTRL_VER_400) {
+      Status = SdMmcHcRwMmio (
                  Private->PciIo,
                  Trb->Slot,
-                 SD_MMC_HC_SDMA_ADDR,
+                 SD_MMC_HC_ADMA_SYS_ADDR,
                  FALSE,
-                 sizeof (UINT32),
+                 sizeof (UINT64),
                  &SdmaAddr
                  );
+    }
+    else {
+        Status = SdMmcHcRwMmio (
+                   Private->PciIo,
+                   Trb->Slot,
+                   SD_MMC_HC_SDMA_ADDR,
+                   FALSE,
+                   sizeof (UINT32),
+                   &SdmaAddr
+                   );
+    }
+
     if (EFI_ERROR (Status)) {
       goto Done;
     }
-    Trb->DataPhy = (UINT32)(UINTN)SdmaAddr;
+    Trb->DataPhy = (UINT64)(UINTN)SdmaAddr;
   }
 
   if ((Packet->SdMmcCmdBlk->CommandType != SdMmcCommandTypeAdtc) &&
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
index cc138fc..a6234f1 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
@@ -2,6 +2,7 @@
 
   Provides some data structure definitions used by the SD/MMC host controller driver.
 
+Copyright (c) 2018, NVIDIA CORPORATION. All rights reserved.
 Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD License
@@ -78,6 +79,9 @@ typedef enum {
 //
 #define ADMA_MAX_DATA_PER_LINE     0x10000
 
+//
+// ADMA descriptor for 32b addressing.
+//
 typedef struct {
   UINT32 Valid:1;
   UINT32 End:1;
@@ -87,7 +91,23 @@ typedef struct {
   UINT32 Reserved1:10;
   UINT32 Length:16;
   UINT32 Address;
-} SD_MMC_HC_ADMA_DESC_LINE;
+} SD_MMC_HC_ADMA_32_DESC_LINE;
+
+//
+// ADMA descriptor for 64b addressing.
+//
+typedef struct {
+  UINT32 Valid:1;
+  UINT32 End:1;
+  UINT32 Int:1;
+  UINT32 Reserved:1;
+  UINT32 Act:2;
+  UINT32 Reserved1:10;
+  UINT32 Length:16;
+  UINT32 LowerAddress;
+  UINT32 UpperAddress;
+  UINT32 Reserved2;
+} SD_MMC_HC_ADMA_64_DESC_LINE;
 
 #define SD_MMC_SDMA_BOUNDARY          512 * 1024
 #define SD_MMC_SDMA_ROUND_UP(x, n)    (((x) + n) & ~(n - 1))
@@ -145,6 +165,12 @@ typedef struct {
 #define SD_MMC_HC_CTRL_VER_410      0x04
 #define SD_MMC_HC_CTRL_VER_420      0x05
 
+//
+// SD Host controller V4 Support
+//
+#define SD_MMC_HC_V4_EN             BIT12
+#define SD_MMC_HC_64_ADDR_EN        BIT13
+
 /**
   Dump the content of SD/MMC host controller's Capability Register.
 
-- 
2.7.4

Re: [PATCH v3 2/2] MdeModulePkg/SdMmcPciHcDxe: Add V4 64bit SDMA and ADMA2 support.

[Ashish Singhal]

Hello,

Any feedback on this patch yet?

Thanks
Ashish

-----Original Message-----
From: Ashish Singhal <ashishsingha@nvidia.com> 
Sent: Monday, November 19, 2018 1:59 PM
To: edk2-devel@lists.01.org
Cc: Ashish Singhal <ashishsingha@nvidia.com>
Subject: [PATCH v3 2/2] MdeModulePkg/SdMmcPciHcDxe: Add V4 64bit SDMA and ADMA2 support.

If V4 64 bit address mode is enabled in compatibility register, program controller to enable V4 host mode.
Use appropriate ADMA2 descriptors supporting 64 bit addresses.
Use appropriate registers for SDMA mode operation.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ashish Singhal <ashishsingha@nvidia.com>
---
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h |   4 +-
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   | 273 +++++++++++++++++----
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h   |  28 ++-
 3 files changed, 260 insertions(+), 45 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
index c683600..22795df 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
@@ -2,6 +2,7 @@
 
   Provides some data structure definitions used by the SD/MMC host controller driver.
 
+Copyright (c) 2018, NVIDIA CORPORATION. All rights reserved.
 Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>  This program and the accompanying materials  are licensed and made available under the terms and conditions of the BSD License @@ -144,7 +145,8 @@ typedef struct {
   BOOLEAN                             Started;
   UINT64                              Timeout;
 
-  SD_MMC_HC_ADMA_DESC_LINE            *AdmaDesc;
+  SD_MMC_HC_ADMA_32_DESC_LINE         *Adma32Desc;
+  SD_MMC_HC_ADMA_64_DESC_LINE         *Adma64Desc;
   EFI_PHYSICAL_ADDRESS                AdmaDescPhy;
   VOID                                *AdmaMap;
   UINT32                              AdmaPages;
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
index e506875..9fef3fb 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
@@ -4,6 +4,7 @@
 
   It would expose EFI_SD_MMC_PASS_THRU_PROTOCOL for upper layer use.
 
+  Copyright (c) 2018, NVIDIA CORPORATION. All rights reserved.
   Copyright (c) 2015 - 2017, Intel Corporation. All rights reserved.<BR>
   This program and the accompanying materials
   are licensed and made available under the terms and conditions of the BSD License @@ -418,6 +419,36 @@ SdMmcHcWaitMmioSet (  }
 
 /**
+  Get the controller version information from the specified slot.
+
+  @param[in]  PciIo           The PCI IO protocol instance.
+  @param[in]  Slot            The slot number of the SD card to send the command to.
+  @param[out] Version         The buffer to store the version information.
+
+  @retval EFI_SUCCESS         The operation executes successfully.
+  @retval Others              The operation fails.
+
+**/
+EFI_STATUS
+SdMmcHcGetControllerVersion (
+  IN     EFI_PCI_IO_PROTOCOL  *PciIo,
+  IN     UINT8                Slot,
+     OUT UINT16               *Version
+  )
+{
+  EFI_STATUS                Status;
+
+  Status = SdMmcHcRwMmio (PciIo, Slot, SD_MMC_HC_CTRL_VER, TRUE, sizeof 
+ (UINT16), Version);  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  *Version &= 0xFF;
+
+  return EFI_SUCCESS;
+}
+
+/**
   Software reset the specified SD/MMC host controller and enable all interrupts.
 
   @param[in] Private        A pointer to the SD_MMC_HC_PRIVATE_DATA instance.
@@ -776,18 +807,18 @@ SdMmcHcClockSupply (
 
   DEBUG ((DEBUG_INFO, "BaseClkFreq %dMHz Divisor %d ClockFreq %dKhz\n", BaseClkFreq, Divisor, ClockFreq));
 
-  Status = SdMmcHcRwMmio (PciIo, Slot, SD_MMC_HC_CTRL_VER, TRUE, sizeof (ControllerVer), &ControllerVer);
+  Status = SdMmcHcGetControllerVersion (PciIo, Slot, &ControllerVer);
   if (EFI_ERROR (Status)) {
     return Status;
   }
   //
   // Set SDCLK Frequency Select and Internal Clock Enable fields in Clock Control register.
   //
-  if (((ControllerVer & 0xFF) >= SD_MMC_HC_CTRL_VER_300) &&
-      ((ControllerVer & 0xFF) <= SD_MMC_HC_CTRL_VER_420)) {
+  if ((ControllerVer >= SD_MMC_HC_CTRL_VER_300) &&
+      (ControllerVer <= SD_MMC_HC_CTRL_VER_420)) {
     ASSERT (Divisor <= 0x3FF);
     ClockCtrl = ((Divisor & 0xFF) << 8) | ((Divisor & 0x300) >> 2);
-  } else if (((ControllerVer & 0xFF) == 0) || ((ControllerVer & 0xFF) == 1)) {
+  } else if ((ControllerVer == 0) || (ControllerVer == 1)) {
     //
     // Only the most significant bit can be used as divisor.
     //
@@ -935,6 +966,54 @@ SdMmcHcSetBusWidth (  }
 
 /**
+  Configure V4 64 bit system address support at initialization.
+
+  @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.
+
+  @retval EFI_SUCCESS       The clock is supplied successfully.
+
+**/
+EFI_STATUS
+SdMmcHcV4Init64BitSupport (
+  IN EFI_PCI_IO_PROTOCOL    *PciIo,
+  IN UINT8                  Slot,
+  IN SD_MMC_HC_SLOT_CAP     Capability
+  )
+{
+  EFI_STATUS                Status;
+  UINT16                    ControllerVer;
+  UINT16                    HostCtrl2;
+
+  //
+  // Check if V4 64bit support is available  //  Status = 
+ SdMmcHcGetControllerVersion (PciIo, Slot, &ControllerVer);  if 
+ (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  if (ControllerVer >= SD_MMC_HC_CTRL_VER_400) {
+    HostCtrl2 = SD_MMC_HC_V4_EN;
+    //
+    // Check if V4 64bit support is available
+    //
+    if (Capability.SysBus64V4 == TRUE) {
+      HostCtrl2 |= SD_MMC_HC_64_ADDR_EN;
+      DEBUG ((DEBUG_INFO, "Enabled V4 64 bit system bus support\n"));
+    }
+    Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
+    if (EFI_ERROR (Status)) {
+      return Status;
+    }
+    DEBUG ((DEBUG_INFO, "Enabled V4 64 bit system bus support\n"));  }
+
+  return EFI_SUCCESS;
+}
+
+/**
   Supply SD/MMC card with lowest clock frequency at initialization.
 
   @param[in] PciIo          The PCI IO protocol instance.
@@ -1101,6 +1180,11 @@ SdMmcHcInitHost (
   PciIo = Private->PciIo;
   Capability = Private->Capability[Slot];
 
+  Status = SdMmcHcV4Init64BitSupport (PciIo, Slot, Capability);  if 
+ (EFI_ERROR (Status)) {
+    return Status;
+  }
+
   Status = SdMmcHcInitClockFreq (PciIo, Slot, Capability);
   if (EFI_ERROR (Status)) {
     return Status;
@@ -1169,7 +1253,7 @@ SdMmcHcLedOnOff (
 /**
   Build ADMA descriptor table for transfer.
 
-  Refer to SD Host Controller Simplified spec 3.0 Section 1.13 for details.
+  Refer to SD Host Controller Simplified spec 4.2 Section 1.13 for details.
 
   @param[in] Trb            The pointer to the SD_MMC_HC_TRB instance.
 
@@ -1187,49 +1271,69 @@ BuildAdmaDescTable (
   UINT64                    Entries;
   UINT32                    Index;
   UINT64                    Remaining;
-  UINT32                    Address;
+  UINT64                    Address;
   UINTN                     TableSize;
   EFI_PCI_IO_PROTOCOL       *PciIo;
   EFI_STATUS                Status;
   UINTN                     Bytes;
+  UINT16                    ControllerVer;
+  BOOLEAN                   AddressingMode64 = FALSE;
+  UINTN                     DescSize = sizeof (SD_MMC_HC_ADMA_32_DESC_LINE);
+  VOID                      *AdmaDesc = NULL;
 
   Data    = Trb->DataPhy;
   DataLen = Trb->DataLen;
   PciIo   = Trb->Private->PciIo;
+
+  //
+  // Detect whether 64bit addressing is supported.
   //
-  // Only support 32bit ADMA Descriptor Table
+  Status = SdMmcHcGetControllerVersion (PciIo, Trb->Slot, 
+ &ControllerVer);  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+  if (ControllerVer >= SD_MMC_HC_CTRL_VER_400) {
+    Status = SdMmcHcCheckMmioSet(PciIo, Trb->Slot, SD_MMC_HC_HOST_CTRL2, 0x2,
+                                 SD_MMC_HC_V4_EN|SD_MMC_HC_64_ADDR_EN, SD_MMC_HC_V4_EN|SD_MMC_HC_64_ADDR_EN);
+    if (!EFI_ERROR (Status)) {
+      AddressingMode64 = TRUE;
+      DescSize = sizeof (SD_MMC_HC_ADMA_64_DESC_LINE);
+    }
+  }
   //
-  if ((Data >= 0x100000000ul) || ((Data + DataLen) > 0x100000000ul)) {
+  // Check for valid ranges in 32bit ADMA Descriptor Table  //  if 
+ (AddressingMode64 == FALSE &&
+      ((Data >= 0x100000000ul) || ((Data + DataLen) > 0x100000000ul))) 
+ {
     return EFI_INVALID_PARAMETER;
   }
   //
   // Address field shall be set on 32-bit boundary (Lower 2-bit is always set to 0)
-  // for 32-bit address descriptor table.
   //
   if ((Data & (BIT0 | BIT1)) != 0) {
     DEBUG ((DEBUG_INFO, "The buffer [0x%x] to construct ADMA desc is not aligned to 4 bytes boundary!\n", Data));
   }
 
   Entries   = DivU64x32 ((DataLen + ADMA_MAX_DATA_PER_LINE - 1), ADMA_MAX_DATA_PER_LINE);
-  TableSize = (UINTN)MultU64x32 (Entries, sizeof (SD_MMC_HC_ADMA_DESC_LINE));
+  TableSize = (UINTN)MultU64x32 (Entries, DescSize);
   Trb->AdmaPages = (UINT32)EFI_SIZE_TO_PAGES (TableSize);
   Status = PciIo->AllocateBuffer (
                     PciIo,
                     AllocateAnyPages,
                     EfiBootServicesData,
                     EFI_SIZE_TO_PAGES (TableSize),
-                    (VOID **)&Trb->AdmaDesc,
+                    (VOID **)&AdmaDesc,
                     0
                     );
   if (EFI_ERROR (Status)) {
     return EFI_OUT_OF_RESOURCES;
   }
-  ZeroMem (Trb->AdmaDesc, TableSize);
+  ZeroMem (AdmaDesc, TableSize);
   Bytes  = TableSize;
   Status = PciIo->Map (
                     PciIo,
                     EfiPciIoOperationBusMasterCommonBuffer,
-                    Trb->AdmaDesc,
+                    AdmaDesc,
                     &Bytes,
                     &Trb->AdmaDescPhy,
                     &Trb->AdmaMap
@@ -1242,12 +1346,13 @@ BuildAdmaDescTable (
     PciIo->FreeBuffer (
              PciIo,
              EFI_SIZE_TO_PAGES (TableSize),
-             Trb->AdmaDesc
+             AdmaDesc
              );
     return EFI_OUT_OF_RESOURCES;
   }
 
-  if ((UINT64)(UINTN)Trb->AdmaDescPhy > 0x100000000ul) {
+  if ((AddressingMode64 == FALSE) &&
+      (UINT64)(UINTN)Trb->AdmaDescPhy > 0x100000000ul) {
     //
     // The ADMA doesn't support 64bit addressing.
     //
@@ -1258,25 +1363,49 @@ BuildAdmaDescTable (
     PciIo->FreeBuffer (
       PciIo,
       EFI_SIZE_TO_PAGES (TableSize),
-      Trb->AdmaDesc
+      AdmaDesc
     );
     return EFI_DEVICE_ERROR;
   }
 
   Remaining = DataLen;
-  Address   = (UINT32)Data;
+  Address   = Data;
+  if (AddressingMode64 == FALSE) {
+    Trb->Adma32Desc = AdmaDesc;
+    Trb->Adma64Desc = NULL;
+  } else {
+    Trb->Adma64Desc = AdmaDesc;
+    Trb->Adma32Desc = NULL;
+  }
   for (Index = 0; Index < Entries; Index++) {
-    if (Remaining <= ADMA_MAX_DATA_PER_LINE) {
-      Trb->AdmaDesc[Index].Valid = 1;
-      Trb->AdmaDesc[Index].Act   = 2;
-      Trb->AdmaDesc[Index].Length  = (UINT16)Remaining;
-      Trb->AdmaDesc[Index].Address = Address;
-      break;
+    if (AddressingMode64 == FALSE) {
+      if (Remaining < ADMA_MAX_DATA_PER_LINE) {
+        Trb->Adma32Desc[Index].Valid = 1;
+        Trb->Adma32Desc[Index].Act   = 2;
+        Trb->Adma32Desc[Index].Length  = (UINT16)Remaining;
+        Trb->Adma32Desc[Index].Address = (UINT32)Address;
+        break;
+      } else {
+        Trb->Adma32Desc[Index].Valid = 1;
+        Trb->Adma32Desc[Index].Act   = 2;
+        Trb->Adma32Desc[Index].Length  = 0;
+        Trb->Adma32Desc[Index].Address = (UINT32)Address;
+      }
     } else {
-      Trb->AdmaDesc[Index].Valid = 1;
-      Trb->AdmaDesc[Index].Act   = 2;
-      Trb->AdmaDesc[Index].Length  = 0;
-      Trb->AdmaDesc[Index].Address = Address;
+      if (Remaining < ADMA_MAX_DATA_PER_LINE) {
+        Trb->Adma64Desc[Index].Valid = 1;
+        Trb->Adma64Desc[Index].Act   = 2;
+        Trb->Adma64Desc[Index].Length  = (UINT16)Remaining;
+        Trb->Adma64Desc[Index].LowerAddress = (UINT32)(Address & MAX_UINT32);
+        Trb->Adma64Desc[Index].UpperAddress = (UINT32)(Address>>32);
+        break;
+      } else {
+        Trb->Adma64Desc[Index].Valid = 1;
+        Trb->Adma64Desc[Index].Act   = 2;
+        Trb->Adma64Desc[Index].Length  = 0;
+        Trb->Adma64Desc[Index].LowerAddress = (UINT32)(Address & MAX_UINT32);
+        Trb->Adma64Desc[Index].UpperAddress = (UINT32)(Address>>32);
+      }
     }
 
     Remaining -= ADMA_MAX_DATA_PER_LINE; @@ -1286,7 +1415,7 @@ BuildAdmaDescTable (
   //
   // Set the last descriptor line as end of descriptor table
   //
-  Trb->AdmaDesc[Index].End = 1;
+  AddressingMode64 ? (Trb->Adma64Desc[Index].End = 1) : 
+ (Trb->Adma32Desc[Index].End = 1);
   return EFI_SUCCESS;
 }
 
@@ -1430,11 +1559,18 @@ SdMmcFreeTrb (
       Trb->AdmaMap
     );
   }
-  if (Trb->AdmaDesc != NULL) {
+  if (Trb->Adma32Desc != NULL) {
     PciIo->FreeBuffer (
       PciIo,
       Trb->AdmaPages,
-      Trb->AdmaDesc
+      Trb->Adma32Desc
+    );
+  }
+  if (Trb->Adma64Desc != NULL) {
+    PciIo->FreeBuffer (
+      PciIo,
+      Trb->AdmaPages,
+      Trb->Adma64Desc
     );
   }
   if (Trb->DataMap != NULL) {
@@ -1574,12 +1710,14 @@ SdMmcExecTrb (
   UINT16                              Cmd;
   UINT16                              IntStatus;
   UINT32                              Argument;
-  UINT16                              BlkCount;
+  UINT32                              BlkCount;
   UINT16                              BlkSize;
   UINT16                              TransMode;
   UINT8                               HostCtrl1;
-  UINT32                              SdmaAddr;
+  UINT64                              SdmaAddr;
   UINT64                              AdmaAddr;
+  UINT16                              ControllerVer;
+  BOOLEAN                             AddressingMode64 = FALSE;
 
   Packet = Trb->Packet;
   PciIo  = Trb->Private->PciIo;
@@ -1612,13 +1750,33 @@ SdMmcExecTrb (
 
   SdMmcHcLedOnOff (PciIo, Trb->Slot, TRUE);
 
+  Status = SdMmcHcGetControllerVersion (PciIo, Trb->Slot, 
+ &ControllerVer);  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+  if (ControllerVer >= SD_MMC_HC_CTRL_VER_400) {
+    Status = SdMmcHcCheckMmioSet(PciIo, Trb->Slot, SD_MMC_HC_HOST_CTRL2, 0x2,
+                                 SD_MMC_HC_V4_EN|SD_MMC_HC_64_ADDR_EN, SD_MMC_HC_V4_EN|SD_MMC_HC_64_ADDR_EN);
+    if (!EFI_ERROR (Status)) {
+      AddressingMode64 = TRUE;
+    }
+  }
+
   if (Trb->Mode == SdMmcSdmaMode) {
-    if ((UINT64)(UINTN)Trb->DataPhy >= 0x100000000ul) {
+    if ((AddressingMode64 == FALSE) &&
+        ((UINT64)(UINTN)Trb->DataPhy >= 0x100000000ul)) {
       return EFI_INVALID_PARAMETER;
     }
 
-    SdmaAddr = (UINT32)(UINTN)Trb->DataPhy;
-    Status   = SdMmcHcRwMmio (PciIo, Trb->Slot, SD_MMC_HC_SDMA_ADDR, FALSE, sizeof (SdmaAddr), &SdmaAddr);
+    SdmaAddr = (UINT64)(UINTN)Trb->DataPhy;
+
+    if (ControllerVer >= SD_MMC_HC_CTRL_VER_400) {
+      Status = SdMmcHcRwMmio (PciIo, Trb->Slot, SD_MMC_HC_ADMA_SYS_ADDR, FALSE, sizeof (UINT64), &SdmaAddr);
+    }
+    else {
+      Status = SdMmcHcRwMmio (PciIo, Trb->Slot, SD_MMC_HC_SDMA_ADDR, FALSE, sizeof (UINT32), &SdmaAddr);
+    }
+
     if (EFI_ERROR (Status)) {
       return Status;
     }
@@ -1648,9 +1806,14 @@ SdMmcExecTrb (
     //
     // Calcuate Block Count.
     //
-    BlkCount = (UINT16)(Trb->DataLen / Trb->BlockSize);
+    BlkCount = (Trb->DataLen / Trb->BlockSize);  }  if (ControllerVer 
+ >= SD_MMC_HC_CTRL_VER_410) {
+    Status = SdMmcHcRwMmio (PciIo, Trb->Slot, SD_MMC_HC_SDMA_ADDR, 
+ FALSE, sizeof (UINT32), &BlkCount);  }  else {
+    Status = SdMmcHcRwMmio (PciIo, Trb->Slot, SD_MMC_HC_BLK_COUNT, 
+ FALSE, sizeof (UINT16), &BlkCount);
   }
-  Status   = SdMmcHcRwMmio (PciIo, Trb->Slot, SD_MMC_HC_BLK_COUNT, FALSE, sizeof (BlkCount), &BlkCount);
   if (EFI_ERROR (Status)) {
     return Status;
   }
@@ -1746,10 +1909,11 @@ SdMmcCheckTrbResult (
   EFI_SD_MMC_PASS_THRU_COMMAND_PACKET *Packet;
   UINT16                              IntStatus;
   UINT32                              Response[4];
-  UINT32                              SdmaAddr;
+  UINT64                              SdmaAddr;
   UINT8                               Index;
   UINT8                               SwReset;
   UINT32                              PioLength;
+  UINT16                              ControllerVer;
 
   SwReset = 0;
   Packet  = Trb->Packet;
@@ -1870,19 +2034,42 @@ SdMmcCheckTrbResult (
     //
     // Update SDMA Address register.
     //
-    SdmaAddr = SD_MMC_SDMA_ROUND_UP ((UINT32)(UINTN)Trb->DataPhy, SD_MMC_SDMA_BOUNDARY);
-    Status   = SdMmcHcRwMmio (
+    SdmaAddr = SD_MMC_SDMA_ROUND_UP ((UINTN)Trb->DataPhy, 
+ SD_MMC_SDMA_BOUNDARY);
+
+    Status = SdMmcHcGetControllerVersion (
+               Private->PciIo,
+               Trb->Slot,
+               &ControllerVer
+               );
+    if (EFI_ERROR (Status)) {
+      return Status;
+    }
+
+    if (ControllerVer >= SD_MMC_HC_CTRL_VER_400) {
+      Status = SdMmcHcRwMmio (
                  Private->PciIo,
                  Trb->Slot,
-                 SD_MMC_HC_SDMA_ADDR,
+                 SD_MMC_HC_ADMA_SYS_ADDR,
                  FALSE,
-                 sizeof (UINT32),
+                 sizeof (UINT64),
                  &SdmaAddr
                  );
+    }
+    else {
+        Status = SdMmcHcRwMmio (
+                   Private->PciIo,
+                   Trb->Slot,
+                   SD_MMC_HC_SDMA_ADDR,
+                   FALSE,
+                   sizeof (UINT32),
+                   &SdmaAddr
+                   );
+    }
+
     if (EFI_ERROR (Status)) {
       goto Done;
     }
-    Trb->DataPhy = (UINT32)(UINTN)SdmaAddr;
+    Trb->DataPhy = (UINT64)(UINTN)SdmaAddr;
   }
 
   if ((Packet->SdMmcCmdBlk->CommandType != SdMmcCommandTypeAdtc) && diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
index cc138fc..a6234f1 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
@@ -2,6 +2,7 @@
 
   Provides some data structure definitions used by the SD/MMC host controller driver.
 
+Copyright (c) 2018, NVIDIA CORPORATION. All rights reserved.
 Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>  This program and the accompanying materials  are licensed and made available under the terms and conditions of the BSD License @@ -78,6 +79,9 @@ typedef enum {  //
 #define ADMA_MAX_DATA_PER_LINE     0x10000
 
+//
+// ADMA descriptor for 32b addressing.
+//
 typedef struct {
   UINT32 Valid:1;
   UINT32 End:1;
@@ -87,7 +91,23 @@ typedef struct {
   UINT32 Reserved1:10;
   UINT32 Length:16;
   UINT32 Address;
-} SD_MMC_HC_ADMA_DESC_LINE;
+} SD_MMC_HC_ADMA_32_DESC_LINE;
+
+//
+// ADMA descriptor for 64b addressing.
+//
+typedef struct {
+  UINT32 Valid:1;
+  UINT32 End:1;
+  UINT32 Int:1;
+  UINT32 Reserved:1;
+  UINT32 Act:2;
+  UINT32 Reserved1:10;
+  UINT32 Length:16;
+  UINT32 LowerAddress;
+  UINT32 UpperAddress;
+  UINT32 Reserved2;
+} SD_MMC_HC_ADMA_64_DESC_LINE;
 
 #define SD_MMC_SDMA_BOUNDARY          512 * 1024
 #define SD_MMC_SDMA_ROUND_UP(x, n)    (((x) + n) & ~(n - 1))
@@ -145,6 +165,12 @@ typedef struct {
 #define SD_MMC_HC_CTRL_VER_410      0x04
 #define SD_MMC_HC_CTRL_VER_420      0x05
 
+//
+// SD Host controller V4 Support
+//
+#define SD_MMC_HC_V4_EN             BIT12
+#define SD_MMC_HC_64_ADDR_EN        BIT13
+
 /**
   Dump the content of SD/MMC host controller's Capability Register.
 
--
2.7.4

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Re: [PATCH v3 2/2] MdeModulePkg/SdMmcPciHcDxe: Add V4 64bit SDMA and ADMA2 support.

[Wu, Hao A]

Hello,

Sorry for the delayed response.

Apart from inserting the Bugzilla tracker information, several more general level
comments:

1. Cannot access the files (content) on SD & eMMC devices

After applying (rebasing onto the latest codes) your patches, I found that
though the SD & eMMC devices can be detected, yet the content cannot be
accessed.

There are 1 SD card and 1 embedded eMMC device within the system. Under Shell,
I can see 2 file systems on SD & eMMC devices being detected, but when I try to
access the files on those file systems by using the 'ls' command, it fails,
saying "ls: File Not Found - 'FS0:\'". I can confirm that files can be listed
successfully without applying this patch.

I also tried the 'dblk' command for display the data via BlockIO protocols
created on those devices. I found that for SD, the command works fine. But for
eMMC, the command fails with message "dblk: Read file error - 'BlockIo'".

For the hardware I own, the controllers are all version 3.0. I tested on both
IA32 and X64 arch image. The results were the same as described above.

Unfortunately, I do not have access to boards with SD controller with version
newer than 3.0. So I am not able to perform those tests on my side for Version
4.0 or greater SD controllers.

Do you have any hardware with SD controller of version 3.0? Is it working on
your side?

Please let me know if additional information is needed.


2. On SdMmcHcGetControllerVersion()

I found that there is a 'ControllerVersion' version in the data structure
SD_MMC_HC_PRIVATE_DATA. But currently, this field seems never used.

I think we can get the controller version information only once and use this
field to store it. Hence, this new function can be eliminated and also can
avoid calling it multiple times.


3. Setting the SD_MMC_HC_64_ADDR_EN bit in SdMmcHcV4Init64BitSupport()

The setting of the SD_MMC_HC_64_ADDR_EN bit is decided by the 'SysBus64V4'
field in the CAP register. For me, additional dependency on the 64-bit DMA
support in the PCI layer is needed as well.

In function SdMmcPciHcDriverBindingStart():

  //
  // Enable 64-bit DMA support in the PCI layer if this controller
  // supports it.
  //
  if (Support64BitDma) {
    Status = PciIo->Attributes (
                      PciIo,
                      EfiPciIoAttributeOperationEnable,
                      EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE,
                      NULL
                      );
    if (EFI_ERROR (Status)) {
      DEBUG ((DEBUG_WARN, "SdMmcPciHcDriverBindingStart: failed to enable 64-bit DMA (%r)\n", Status));
    }
  }

If the above PCI attribute setting fails, the PCI layer will not support the
64-bit DMA.

Hence, I am thinking to introduce a new BOOLEAN field in the
"SD_MMC_HC_PRIVATE_DATA" data structure (maybe putting the 'Support64BitDma'
into the data structure). If the above PCI operation succeeds, the BOOLEAN
field will be set to TRUE, otherwise, FALSE.

And the setting of the SD_MMC_HC_64_ADDR_EN bit should depend on the BOOLEAN
field as well.


4. Please help to rebase the patch upon the latest master branch.

Some changes within the SdMmcPciHcDxe have been pushed recently. Could you help
to rebase this patch upon the latest changes. Thanks in advance.


Also, some inline comments below.


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Ashish Singhal
> Sent: Tuesday, November 20, 2018 4:59 AM
> To: edk2-devel@lists.01.org
> Cc: Ashish Singhal
> Subject: [edk2] [PATCH v3 2/2] MdeModulePkg/SdMmcPciHcDxe: Add V4
> 64bit SDMA and ADMA2 support.
> 
> If V4 64 bit address mode is enabled in compatibility register,
> program controller to enable V4 host mode.
> Use appropriate ADMA2 descriptors supporting 64 bit addresses.
> Use appropriate registers for SDMA mode operation.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ashish Singhal <ashishsingha@nvidia.com>
> ---
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h |   4 +-
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   | 273
> +++++++++++++++++----
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h   |  28 ++-
>  3 files changed, 260 insertions(+), 45 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> index c683600..22795df 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> @@ -2,6 +2,7 @@
> 
>    Provides some data structure definitions used by the SD/MMC host
> controller driver.
> 
> +Copyright (c) 2018, NVIDIA CORPORATION. All rights reserved.
>  Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD
> License
> @@ -144,7 +145,8 @@ typedef struct {
>    BOOLEAN                             Started;
>    UINT64                              Timeout;
> 
> -  SD_MMC_HC_ADMA_DESC_LINE            *AdmaDesc;
> +  SD_MMC_HC_ADMA_32_DESC_LINE         *Adma32Desc;
> +  SD_MMC_HC_ADMA_64_DESC_LINE         *Adma64Desc;
>    EFI_PHYSICAL_ADDRESS                AdmaDescPhy;
>    VOID                                *AdmaMap;
>    UINT32                              AdmaPages;
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> index e506875..9fef3fb 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> @@ -4,6 +4,7 @@
> 
>    It would expose EFI_SD_MMC_PASS_THRU_PROTOCOL for upper layer use.
> 
> +  Copyright (c) 2018, NVIDIA CORPORATION. All rights reserved.
>    Copyright (c) 2015 - 2017, Intel Corporation. All rights reserved.<BR>
>    This program and the accompanying materials
>    are licensed and made available under the terms and conditions of the BSD
> License
> @@ -418,6 +419,36 @@ SdMmcHcWaitMmioSet (
>  }
> 
>  /**
> +  Get the controller version information from the specified slot.
> +
> +  @param[in]  PciIo           The PCI IO protocol instance.
> +  @param[in]  Slot            The slot number of the SD card to send the
> command to.
> +  @param[out] Version         The buffer to store the version information.
> +
> +  @retval EFI_SUCCESS         The operation executes successfully.
> +  @retval Others              The operation fails.
> +
> +**/
> +EFI_STATUS
> +SdMmcHcGetControllerVersion (
> +  IN     EFI_PCI_IO_PROTOCOL  *PciIo,
> +  IN     UINT8                Slot,
> +     OUT UINT16               *Version
> +  )
> +{
> +  EFI_STATUS                Status;
> +
> +  Status = SdMmcHcRwMmio (PciIo, Slot, SD_MMC_HC_CTRL_VER, TRUE,
> sizeof (UINT16), Version);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  *Version &= 0xFF;
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
>    Software reset the specified SD/MMC host controller and enable all
> interrupts.
> 
>    @param[in] Private        A pointer to the SD_MMC_HC_PRIVATE_DATA
> instance.
> @@ -776,18 +807,18 @@ SdMmcHcClockSupply (
> 
>    DEBUG ((DEBUG_INFO, "BaseClkFreq %dMHz Divisor %d
> ClockFreq %dKhz\n", BaseClkFreq, Divisor, ClockFreq));
> 
> -  Status = SdMmcHcRwMmio (PciIo, Slot, SD_MMC_HC_CTRL_VER, TRUE,
> sizeof (ControllerVer), &ControllerVer);
> +  Status = SdMmcHcGetControllerVersion (PciIo, Slot, &ControllerVer);
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
>    //
>    // Set SDCLK Frequency Select and Internal Clock Enable fields in Clock
> Control register.
>    //
> -  if (((ControllerVer & 0xFF) >= SD_MMC_HC_CTRL_VER_300) &&
> -      ((ControllerVer & 0xFF) <= SD_MMC_HC_CTRL_VER_420)) {
> +  if ((ControllerVer >= SD_MMC_HC_CTRL_VER_300) &&
> +      (ControllerVer <= SD_MMC_HC_CTRL_VER_420)) {
>      ASSERT (Divisor <= 0x3FF);
>      ClockCtrl = ((Divisor & 0xFF) << 8) | ((Divisor & 0x300) >> 2);
> -  } else if (((ControllerVer & 0xFF) == 0) || ((ControllerVer & 0xFF) == 1)) {
> +  } else if ((ControllerVer == 0) || (ControllerVer == 1)) {

Could you help to update the above 'else if' statement to use the below version
definitions?

SD_MMC_HC_CTRL_VER_100
SD_MMC_HC_CTRL_VER_200

>      //
>      // Only the most significant bit can be used as divisor.
>      //
> @@ -935,6 +966,54 @@ SdMmcHcSetBusWidth (
>  }
> 
>  /**
> +  Configure V4 64 bit system address support at initialization.
> +
> +  @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.
> +
> +  @retval EFI_SUCCESS       The clock is supplied successfully.
> +
> +**/
> +EFI_STATUS
> +SdMmcHcV4Init64BitSupport (
> +  IN EFI_PCI_IO_PROTOCOL    *PciIo,
> +  IN UINT8                  Slot,
> +  IN SD_MMC_HC_SLOT_CAP     Capability
> +  )
> +{
> +  EFI_STATUS                Status;
> +  UINT16                    ControllerVer;
> +  UINT16                    HostCtrl2;
> +
> +  //
> +  // Check if V4 64bit support is available
> +  //
> +  Status = SdMmcHcGetControllerVersion (PciIo, Slot, &ControllerVer);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  if (ControllerVer >= SD_MMC_HC_CTRL_VER_400) {
> +    HostCtrl2 = SD_MMC_HC_V4_EN;
> +    //
> +    // Check if V4 64bit support is available
> +    //
> +    if (Capability.SysBus64V4 == TRUE) {

Apart from the additional check needed comments above, could you help to refine
the above check to:

if (Capability.SysBus64V4 != 0) {

> +      HostCtrl2 |= SD_MMC_HC_64_ADDR_EN;
> +      DEBUG ((DEBUG_INFO, "Enabled V4 64 bit system bus support\n"));
> +    }
> +    Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof
> (HostCtrl2), &HostCtrl2);
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +    DEBUG ((DEBUG_INFO, "Enabled V4 64 bit system bus support\n"));
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
>    Supply SD/MMC card with lowest clock frequency at initialization.
> 
>    @param[in] PciIo          The PCI IO protocol instance.
> @@ -1101,6 +1180,11 @@ SdMmcHcInitHost (
>    PciIo = Private->PciIo;
>    Capability = Private->Capability[Slot];
> 
> +  Status = SdMmcHcV4Init64BitSupport (PciIo, Slot, Capability);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
>    Status = SdMmcHcInitClockFreq (PciIo, Slot, Capability);
>    if (EFI_ERROR (Status)) {
>      return Status;
> @@ -1169,7 +1253,7 @@ SdMmcHcLedOnOff (
>  /**
>    Build ADMA descriptor table for transfer.
> 
> -  Refer to SD Host Controller Simplified spec 3.0 Section 1.13 for details.
> +  Refer to SD Host Controller Simplified spec 4.2 Section 1.13 for details.
> 
>    @param[in] Trb            The pointer to the SD_MMC_HC_TRB instance.
> 
> @@ -1187,49 +1271,69 @@ BuildAdmaDescTable (
>    UINT64                    Entries;
>    UINT32                    Index;
>    UINT64                    Remaining;
> -  UINT32                    Address;
> +  UINT64                    Address;
>    UINTN                     TableSize;
>    EFI_PCI_IO_PROTOCOL       *PciIo;
>    EFI_STATUS                Status;
>    UINTN                     Bytes;
> +  UINT16                    ControllerVer;
> +  BOOLEAN                   AddressingMode64 = FALSE;
> +  UINTN                     DescSize = sizeof (SD_MMC_HC_ADMA_32_DESC_LINE);
> +  VOID                      *AdmaDesc = NULL;

Please help to separate the variable declaration and initial value assignment.

Also, please update the type of variable 'DescSize' from UINTN to UINT32. The
Visual Studio 2015 complier complains for:

" TableSize = (UINTN)MultU64x32 (Entries, DescSize); ":

conversion from 'UINTN' to 'UINT32', possible loss of data.

> 
>    Data    = Trb->DataPhy;
>    DataLen = Trb->DataLen;
>    PciIo   = Trb->Private->PciIo;
> +
> +  //
> +  // Detect whether 64bit addressing is supported.
>    //
> -  // Only support 32bit ADMA Descriptor Table
> +  Status = SdMmcHcGetControllerVersion (PciIo, Trb->Slot, &ControllerVer);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +  if (ControllerVer >= SD_MMC_HC_CTRL_VER_400) {
> +    Status = SdMmcHcCheckMmioSet(PciIo, Trb->Slot,
> SD_MMC_HC_HOST_CTRL2, 0x2,
> +                                 SD_MMC_HC_V4_EN|SD_MMC_HC_64_ADDR_EN,
> SD_MMC_HC_V4_EN|SD_MMC_HC_64_ADDR_EN);
> +    if (!EFI_ERROR (Status)) {
> +      AddressingMode64 = TRUE;
> +      DescSize = sizeof (SD_MMC_HC_ADMA_64_DESC_LINE);
> +    }
> +  }
>    //
> -  if ((Data >= 0x100000000ul) || ((Data + DataLen) > 0x100000000ul)) {
> +  // Check for valid ranges in 32bit ADMA Descriptor Table
> +  //
> +  if (AddressingMode64 == FALSE &&
> +      ((Data >= 0x100000000ul) || ((Data + DataLen) > 0x100000000ul))) {

Please help to update the above check to:

  if ((!AddressingMode64) &&
      ((Data >= 0x100000000ul) || ((Data + DataLen) > 0x100000000ul))) {

>      return EFI_INVALID_PARAMETER;
>    }
>    //
>    // Address field shall be set on 32-bit boundary (Lower 2-bit is always set to
> 0)
> -  // for 32-bit address descriptor table.
>    //
>    if ((Data & (BIT0 | BIT1)) != 0) {
>      DEBUG ((DEBUG_INFO, "The buffer [0x%x] to construct ADMA desc is not
> aligned to 4 bytes boundary!\n", Data));
>    }

I found that the above check (also the comments) should be updated. For 32-bit
addressing the address of ADMA desc is 4-bytes aligned; for 64-bit case, the
requirement is 8-byte.

Seems the align check for 64-bit addressing is missing here.

> 
>    Entries   = DivU64x32 ((DataLen + ADMA_MAX_DATA_PER_LINE - 1),
> ADMA_MAX_DATA_PER_LINE);
> -  TableSize = (UINTN)MultU64x32 (Entries, sizeof
> (SD_MMC_HC_ADMA_DESC_LINE));
> +  TableSize = (UINTN)MultU64x32 (Entries, DescSize);
>    Trb->AdmaPages = (UINT32)EFI_SIZE_TO_PAGES (TableSize);
>    Status = PciIo->AllocateBuffer (
>                      PciIo,
>                      AllocateAnyPages,
>                      EfiBootServicesData,
>                      EFI_SIZE_TO_PAGES (TableSize),
> -                    (VOID **)&Trb->AdmaDesc,
> +                    (VOID **)&AdmaDesc,
>                      0
>                      );
>    if (EFI_ERROR (Status)) {
>      return EFI_OUT_OF_RESOURCES;
>    }
> -  ZeroMem (Trb->AdmaDesc, TableSize);
> +  ZeroMem (AdmaDesc, TableSize);
>    Bytes  = TableSize;
>    Status = PciIo->Map (
>                      PciIo,
>                      EfiPciIoOperationBusMasterCommonBuffer,
> -                    Trb->AdmaDesc,
> +                    AdmaDesc,
>                      &Bytes,
>                      &Trb->AdmaDescPhy,
>                      &Trb->AdmaMap
> @@ -1242,12 +1346,13 @@ BuildAdmaDescTable (
>      PciIo->FreeBuffer (
>               PciIo,
>               EFI_SIZE_TO_PAGES (TableSize),
> -             Trb->AdmaDesc
> +             AdmaDesc
>               );
>      return EFI_OUT_OF_RESOURCES;
>    }
> 
> -  if ((UINT64)(UINTN)Trb->AdmaDescPhy > 0x100000000ul) {
> +  if ((AddressingMode64 == FALSE) &&
> +      (UINT64)(UINTN)Trb->AdmaDescPhy > 0x100000000ul) {

Please update the above check to:

  if ((!AddressingMode64) &&
      (UINT64)(UINTN)Trb->AdmaDescPhy > 0x100000000ul) {

>      //
>      // The ADMA doesn't support 64bit addressing.
>      //
> @@ -1258,25 +1363,49 @@ BuildAdmaDescTable (
>      PciIo->FreeBuffer (
>        PciIo,
>        EFI_SIZE_TO_PAGES (TableSize),
> -      Trb->AdmaDesc
> +      AdmaDesc
>      );
>      return EFI_DEVICE_ERROR;
>    }
> 
>    Remaining = DataLen;
> -  Address   = (UINT32)Data;
> +  Address   = Data;
> +  if (AddressingMode64 == FALSE) {

Please help to update the above check to:

  if (!AddressingMode64) {

> +    Trb->Adma32Desc = AdmaDesc;
> +    Trb->Adma64Desc = NULL;
> +  } else {
> +    Trb->Adma64Desc = AdmaDesc;
> +    Trb->Adma32Desc = NULL;
> +  }
>    for (Index = 0; Index < Entries; Index++) {
> -    if (Remaining <= ADMA_MAX_DATA_PER_LINE) {
> -      Trb->AdmaDesc[Index].Valid = 1;
> -      Trb->AdmaDesc[Index].Act   = 2;
> -      Trb->AdmaDesc[Index].Length  = (UINT16)Remaining;
> -      Trb->AdmaDesc[Index].Address = Address;
> -      break;
> +    if (AddressingMode64 == FALSE) {

Please help to update the above check to:

  if (!AddressingMode64) {

> +      if (Remaining < ADMA_MAX_DATA_PER_LINE) {
> +        Trb->Adma32Desc[Index].Valid = 1;
> +        Trb->Adma32Desc[Index].Act   = 2;
> +        Trb->Adma32Desc[Index].Length  = (UINT16)Remaining;
> +        Trb->Adma32Desc[Index].Address = (UINT32)Address;
> +        break;
> +      } else {
> +        Trb->Adma32Desc[Index].Valid = 1;
> +        Trb->Adma32Desc[Index].Act   = 2;
> +        Trb->Adma32Desc[Index].Length  = 0;
> +        Trb->Adma32Desc[Index].Address = (UINT32)Address;
> +      }
>      } else {
> -      Trb->AdmaDesc[Index].Valid = 1;
> -      Trb->AdmaDesc[Index].Act   = 2;
> -      Trb->AdmaDesc[Index].Length  = 0;
> -      Trb->AdmaDesc[Index].Address = Address;
> +      if (Remaining < ADMA_MAX_DATA_PER_LINE) {
> +        Trb->Adma64Desc[Index].Valid = 1;
> +        Trb->Adma64Desc[Index].Act   = 2;
> +        Trb->Adma64Desc[Index].Length  = (UINT16)Remaining;
> +        Trb->Adma64Desc[Index].LowerAddress = (UINT32)(Address &
> MAX_UINT32);
> +        Trb->Adma64Desc[Index].UpperAddress = (UINT32)(Address>>32);
> +        break;
> +      } else {
> +        Trb->Adma64Desc[Index].Valid = 1;
> +        Trb->Adma64Desc[Index].Act   = 2;
> +        Trb->Adma64Desc[Index].Length  = 0;
> +        Trb->Adma64Desc[Index].LowerAddress = (UINT32)(Address &
> MAX_UINT32);

I think the above 2 " & MAX_UINT32" can be dropped.


> +        Trb->Adma64Desc[Index].UpperAddress = (UINT32)(Address>>32);
> +      }
>      }
> 
>      Remaining -= ADMA_MAX_DATA_PER_LINE;
> @@ -1286,7 +1415,7 @@ BuildAdmaDescTable (
>    //
>    // Set the last descriptor line as end of descriptor table
>    //
> -  Trb->AdmaDesc[Index].End = 1;
> +  AddressingMode64 ? (Trb->Adma64Desc[Index].End = 1) : (Trb-
> >Adma32Desc[Index].End = 1);
>    return EFI_SUCCESS;
>  }
> 
> @@ -1430,11 +1559,18 @@ SdMmcFreeTrb (
>        Trb->AdmaMap
>      );
>    }
> -  if (Trb->AdmaDesc != NULL) {
> +  if (Trb->Adma32Desc != NULL) {
>      PciIo->FreeBuffer (
>        PciIo,
>        Trb->AdmaPages,
> -      Trb->AdmaDesc
> +      Trb->Adma32Desc
> +    );
> +  }
> +  if (Trb->Adma64Desc != NULL) {
> +    PciIo->FreeBuffer (
> +      PciIo,
> +      Trb->AdmaPages,
> +      Trb->Adma64Desc
>      );
>    }
>    if (Trb->DataMap != NULL) {
> @@ -1574,12 +1710,14 @@ SdMmcExecTrb (
>    UINT16                              Cmd;
>    UINT16                              IntStatus;
>    UINT32                              Argument;
> -  UINT16                              BlkCount;
> +  UINT32                              BlkCount;
>    UINT16                              BlkSize;
>    UINT16                              TransMode;
>    UINT8                               HostCtrl1;
> -  UINT32                              SdmaAddr;
> +  UINT64                              SdmaAddr;
>    UINT64                              AdmaAddr;
> +  UINT16                              ControllerVer;
> +  BOOLEAN                             AddressingMode64 = FALSE;

Please help to separate the variable declaration and initial value assignment.

> 
>    Packet = Trb->Packet;
>    PciIo  = Trb->Private->PciIo;
> @@ -1612,13 +1750,33 @@ SdMmcExecTrb (
> 
>    SdMmcHcLedOnOff (PciIo, Trb->Slot, TRUE);
> 
> +  Status = SdMmcHcGetControllerVersion (PciIo, Trb->Slot, &ControllerVer);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +  if (ControllerVer >= SD_MMC_HC_CTRL_VER_400) {
> +    Status = SdMmcHcCheckMmioSet(PciIo, Trb->Slot,
> SD_MMC_HC_HOST_CTRL2, 0x2,
> +                                 SD_MMC_HC_V4_EN|SD_MMC_HC_64_ADDR_EN,
> SD_MMC_HC_V4_EN|SD_MMC_HC_64_ADDR_EN);
> +    if (!EFI_ERROR (Status)) {
> +      AddressingMode64 = TRUE;
> +    }
> +  }
> +
>    if (Trb->Mode == SdMmcSdmaMode) {
> -    if ((UINT64)(UINTN)Trb->DataPhy >= 0x100000000ul) {
> +    if ((AddressingMode64 == FALSE) &&
> +        ((UINT64)(UINTN)Trb->DataPhy >= 0x100000000ul)) {

Please help to update the above check to:

    if ((!AddressingMode64) &&
        ((UINT64)(UINTN)Trb->DataPhy >= 0x100000000ul)) {

>        return EFI_INVALID_PARAMETER;
>      }
> 
> -    SdmaAddr = (UINT32)(UINTN)Trb->DataPhy;
> -    Status   = SdMmcHcRwMmio (PciIo, Trb->Slot, SD_MMC_HC_SDMA_ADDR,
> FALSE, sizeof (SdmaAddr), &SdmaAddr);
> +    SdmaAddr = (UINT64)(UINTN)Trb->DataPhy;
> +
> +    if (ControllerVer >= SD_MMC_HC_CTRL_VER_400) {
> +      Status = SdMmcHcRwMmio (PciIo, Trb->Slot,
> SD_MMC_HC_ADMA_SYS_ADDR, FALSE, sizeof (UINT64), &SdmaAddr);
> +    }
> +    else {

Minor coding style comment for the above line:

} else {

> +      Status = SdMmcHcRwMmio (PciIo, Trb->Slot,
> SD_MMC_HC_SDMA_ADDR, FALSE, sizeof (UINT32), &SdmaAddr);
> +    }
> +
>      if (EFI_ERROR (Status)) {
>        return Status;
>      }
> @@ -1648,9 +1806,14 @@ SdMmcExecTrb (
>      //
>      // Calcuate Block Count.
>      //
> -    BlkCount = (UINT16)(Trb->DataLen / Trb->BlockSize);
> +    BlkCount = (Trb->DataLen / Trb->BlockSize);
> +  }

For the below 'Block Count' settings, I think it would be better to add
comments that quote the SD Host Controller Spec to mention that the 32-bit
block count support for all operations in SD mode was introduced at Version
4.10.

> +  if (ControllerVer >= SD_MMC_HC_CTRL_VER_410) {
> +    Status = SdMmcHcRwMmio (PciIo, Trb->Slot, SD_MMC_HC_SDMA_ADDR,
> FALSE, sizeof (UINT32), &BlkCount);
> +  }
> +  else {

Minor coding style comment for the above line:

} else {

> +    Status = SdMmcHcRwMmio (PciIo, Trb->Slot, SD_MMC_HC_BLK_COUNT,
> FALSE, sizeof (UINT16), &BlkCount);
>    }
> -  Status   = SdMmcHcRwMmio (PciIo, Trb->Slot, SD_MMC_HC_BLK_COUNT,
> FALSE, sizeof (BlkCount), &BlkCount);
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
> @@ -1746,10 +1909,11 @@ SdMmcCheckTrbResult (
>    EFI_SD_MMC_PASS_THRU_COMMAND_PACKET *Packet;
>    UINT16                              IntStatus;
>    UINT32                              Response[4];
> -  UINT32                              SdmaAddr;
> +  UINT64                              SdmaAddr;
>    UINT8                               Index;
>    UINT8                               SwReset;
>    UINT32                              PioLength;
> +  UINT16                              ControllerVer;
> 
>    SwReset = 0;
>    Packet  = Trb->Packet;
> @@ -1870,19 +2034,42 @@ SdMmcCheckTrbResult (
>      //
>      // Update SDMA Address register.
>      //
> -    SdmaAddr = SD_MMC_SDMA_ROUND_UP ((UINT32)(UINTN)Trb-
> >DataPhy, SD_MMC_SDMA_BOUNDARY);
> -    Status   = SdMmcHcRwMmio (
> +    SdmaAddr = SD_MMC_SDMA_ROUND_UP ((UINTN)Trb->DataPhy,
> SD_MMC_SDMA_BOUNDARY);
> +
> +    Status = SdMmcHcGetControllerVersion (
> +               Private->PciIo,
> +               Trb->Slot,
> +               &ControllerVer
> +               );
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +
> +    if (ControllerVer >= SD_MMC_HC_CTRL_VER_400) {
> +      Status = SdMmcHcRwMmio (
>                   Private->PciIo,
>                   Trb->Slot,
> -                 SD_MMC_HC_SDMA_ADDR,
> +                 SD_MMC_HC_ADMA_SYS_ADDR,
>                   FALSE,
> -                 sizeof (UINT32),
> +                 sizeof (UINT64),
>                   &SdmaAddr
>                   );
> +    }
> +    else {
> +        Status = SdMmcHcRwMmio (
> +                   Private->PciIo,
> +                   Trb->Slot,
> +                   SD_MMC_HC_SDMA_ADDR,
> +                   FALSE,
> +                   sizeof (UINT32),
> +                   &SdmaAddr
> +                   );
> +    }

Minor coding style comments for the above line:

1. } else {
2. Please help to refine the space indent of the above SdMmcHcRwMmio() call


Best Regards,
Hao Wu

> +
>      if (EFI_ERROR (Status)) {
>        goto Done;
>      }
> -    Trb->DataPhy = (UINT32)(UINTN)SdmaAddr;
> +    Trb->DataPhy = (UINT64)(UINTN)SdmaAddr;
>    }
> 
>    if ((Packet->SdMmcCmdBlk->CommandType != SdMmcCommandTypeAdtc)
> &&
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> index cc138fc..a6234f1 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> @@ -2,6 +2,7 @@
> 
>    Provides some data structure definitions used by the SD/MMC host
> controller driver.
> 
> +Copyright (c) 2018, NVIDIA CORPORATION. All rights reserved.
>  Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD
> License
> @@ -78,6 +79,9 @@ typedef enum {
>  //
>  #define ADMA_MAX_DATA_PER_LINE     0x10000
> 
> +//
> +// ADMA descriptor for 32b addressing.
> +//
>  typedef struct {
>    UINT32 Valid:1;
>    UINT32 End:1;
> @@ -87,7 +91,23 @@ typedef struct {
>    UINT32 Reserved1:10;
>    UINT32 Length:16;
>    UINT32 Address;
> -} SD_MMC_HC_ADMA_DESC_LINE;
> +} SD_MMC_HC_ADMA_32_DESC_LINE;
> +
> +//
> +// ADMA descriptor for 64b addressing.
> +//
> +typedef struct {
> +  UINT32 Valid:1;
> +  UINT32 End:1;
> +  UINT32 Int:1;
> +  UINT32 Reserved:1;
> +  UINT32 Act:2;
> +  UINT32 Reserved1:10;
> +  UINT32 Length:16;
> +  UINT32 LowerAddress;
> +  UINT32 UpperAddress;
> +  UINT32 Reserved2;
> +} SD_MMC_HC_ADMA_64_DESC_LINE;
> 
>  #define SD_MMC_SDMA_BOUNDARY          512 * 1024
>  #define SD_MMC_SDMA_ROUND_UP(x, n)    (((x) + n) & ~(n - 1))
> @@ -145,6 +165,12 @@ typedef struct {
>  #define SD_MMC_HC_CTRL_VER_410      0x04
>  #define SD_MMC_HC_CTRL_VER_420      0x05
> 
> +//
> +// SD Host controller V4 Support
> +//
> +#define SD_MMC_HC_V4_EN             BIT12
> +#define SD_MMC_HC_64_ADDR_EN        BIT13
> +
>  /**
>    Dump the content of SD/MMC host controller's Capability Register.
> 
> --
> 2.7.4
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH v3 2/2] MdeModulePkg/SdMmcPciHcDxe: Add V4 64bit SDMA and ADMA2 support.

[Ashish Singhal]

Hello Hao,

Answers to your comments:

1. Patch 5 fixes the issue with accessing SD as well as eMMC files.
2. Using Private variable for controller version would mean I can only access it in functions having an instance of the installed protocol which is not possible in some functions for example where we set clock dividers. We either need to use what I have or pass Private or controller version as an argument to functions using it.
3. I think there is a bigger issue here. During host controller initialization we need to setup 64b addressing which happens before we initialize 64b DMA in PCI layer. So what you are suggesting may not be that helpful. Also, what we are doing right now is that we go through all slots and if controller on any slot does not support 64b, we do not enable 64b DMA in PCI layer. What is the likelihood of all controllers on an SOC not supporting similar address width? Also, shouldn't we be enabling 64b DMA in PCI layer if any of the controller supports 64b addressing? Wouldn't 64b DMA enabled mean 32b is enabled by default?
4. Patch 5 has been rebased on tip of edk2.

Thanks
Ashish

-----Original Message-----
From: Wu, Hao A <hao.a.wu@intel.com> 
Sent: Wednesday, November 28, 2018 12:25 AM
To: Ashish Singhal <ashishsingha@nvidia.com>; edk2-devel@lists.01.org
Subject: RE: [edk2] [PATCH v3 2/2] MdeModulePkg/SdMmcPciHcDxe: Add V4 64bit SDMA and ADMA2 support.

Hello,

Sorry for the delayed response.

Apart from inserting the Bugzilla tracker information, several more general level
comments:

1. Cannot access the files (content) on SD & eMMC devices

After applying (rebasing onto the latest codes) your patches, I found that though the SD & eMMC devices can be detected, yet the content cannot be accessed.

There are 1 SD card and 1 embedded eMMC device within the system. Under Shell, I can see 2 file systems on SD & eMMC devices being detected, but when I try to access the files on those file systems by using the 'ls' command, it fails, saying "ls: File Not Found - 'FS0:\'". I can confirm that files can be listed successfully without applying this patch.

I also tried the 'dblk' command for display the data via BlockIO protocols created on those devices. I found that for SD, the command works fine. But for eMMC, the command fails with message "dblk: Read file error - 'BlockIo'".

For the hardware I own, the controllers are all version 3.0. I tested on both
IA32 and X64 arch image. The results were the same as described above.

Unfortunately, I do not have access to boards with SD controller with version newer than 3.0. So I am not able to perform those tests on my side for Version
4.0 or greater SD controllers.

Do you have any hardware with SD controller of version 3.0? Is it working on your side?

Please let me know if additional information is needed.


2. On SdMmcHcGetControllerVersion()

I found that there is a 'ControllerVersion' version in the data structure SD_MMC_HC_PRIVATE_DATA. But currently, this field seems never used.

I think we can get the controller version information only once and use this field to store it. Hence, this new function can be eliminated and also can avoid calling it multiple times.


3. Setting the SD_MMC_HC_64_ADDR_EN bit in SdMmcHcV4Init64BitSupport()

The setting of the SD_MMC_HC_64_ADDR_EN bit is decided by the 'SysBus64V4'
field in the CAP register. For me, additional dependency on the 64-bit DMA support in the PCI layer is needed as well.

In function SdMmcPciHcDriverBindingStart():

  //
  // Enable 64-bit DMA support in the PCI layer if this controller
  // supports it.
  //
  if (Support64BitDma) {
    Status = PciIo->Attributes (
                      PciIo,
                      EfiPciIoAttributeOperationEnable,
                      EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE,
                      NULL
                      );
    if (EFI_ERROR (Status)) {
      DEBUG ((DEBUG_WARN, "SdMmcPciHcDriverBindingStart: failed to enable 64-bit DMA (%r)\n", Status));
    }
  }

If the above PCI attribute setting fails, the PCI layer will not support the 64-bit DMA.

Hence, I am thinking to introduce a new BOOLEAN field in the "SD_MMC_HC_PRIVATE_DATA" data structure (maybe putting the 'Support64BitDma'
into the data structure). If the above PCI operation succeeds, the BOOLEAN field will be set to TRUE, otherwise, FALSE.

And the setting of the SD_MMC_HC_64_ADDR_EN bit should depend on the BOOLEAN field as well.


4. Please help to rebase the patch upon the latest master branch.

Some changes within the SdMmcPciHcDxe have been pushed recently. Could you help to rebase this patch upon the latest changes. Thanks in advance.


Also, some inline comments below.


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 
> Ashish Singhal
> Sent: Tuesday, November 20, 2018 4:59 AM
> To: edk2-devel@lists.01.org
> Cc: Ashish Singhal
> Subject: [edk2] [PATCH v3 2/2] MdeModulePkg/SdMmcPciHcDxe: Add V4 
> 64bit SDMA and ADMA2 support.
> 
> If V4 64 bit address mode is enabled in compatibility register, 
> program controller to enable V4 host mode.
> Use appropriate ADMA2 descriptors supporting 64 bit addresses.
> Use appropriate registers for SDMA mode operation.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ashish Singhal <ashishsingha@nvidia.com>
> ---
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h |   4 +-
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   | 273
> +++++++++++++++++----
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h   |  28 ++-
>  3 files changed, 260 insertions(+), 45 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> index c683600..22795df 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> @@ -2,6 +2,7 @@
> 
>    Provides some data structure definitions used by the SD/MMC host 
> controller driver.
> 
> +Copyright (c) 2018, NVIDIA CORPORATION. All rights reserved.
>  Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>  This 
> program and the accompanying materials  are licensed and made 
> available under the terms and conditions of the BSD License @@ -144,7 
> +145,8 @@ typedef struct {
>    BOOLEAN                             Started;
>    UINT64                              Timeout;
> 
> -  SD_MMC_HC_ADMA_DESC_LINE            *AdmaDesc;
> +  SD_MMC_HC_ADMA_32_DESC_LINE         *Adma32Desc;
> +  SD_MMC_HC_ADMA_64_DESC_LINE         *Adma64Desc;
>    EFI_PHYSICAL_ADDRESS                AdmaDescPhy;
>    VOID                                *AdmaMap;
>    UINT32                              AdmaPages;
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> index e506875..9fef3fb 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> @@ -4,6 +4,7 @@
> 
>    It would expose EFI_SD_MMC_PASS_THRU_PROTOCOL for upper layer use.
> 
> +  Copyright (c) 2018, NVIDIA CORPORATION. All rights reserved.
>    Copyright (c) 2015 - 2017, Intel Corporation. All rights reserved.<BR>
>    This program and the accompanying materials
>    are licensed and made available under the terms and conditions of 
> the BSD License @@ -418,6 +419,36 @@ SdMmcHcWaitMmioSet (  }
> 
>  /**
> +  Get the controller version information from the specified slot.
> +
> +  @param[in]  PciIo           The PCI IO protocol instance.
> +  @param[in]  Slot            The slot number of the SD card to send the
> command to.
> +  @param[out] Version         The buffer to store the version information.
> +
> +  @retval EFI_SUCCESS         The operation executes successfully.
> +  @retval Others              The operation fails.
> +
> +**/
> +EFI_STATUS
> +SdMmcHcGetControllerVersion (
> +  IN     EFI_PCI_IO_PROTOCOL  *PciIo,
> +  IN     UINT8                Slot,
> +     OUT UINT16               *Version
> +  )
> +{
> +  EFI_STATUS                Status;
> +
> +  Status = SdMmcHcRwMmio (PciIo, Slot, SD_MMC_HC_CTRL_VER, TRUE,
> sizeof (UINT16), Version);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  *Version &= 0xFF;
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
>    Software reset the specified SD/MMC host controller and enable all 
> interrupts.
> 
>    @param[in] Private        A pointer to the SD_MMC_HC_PRIVATE_DATA
> instance.
> @@ -776,18 +807,18 @@ SdMmcHcClockSupply (
> 
>    DEBUG ((DEBUG_INFO, "BaseClkFreq %dMHz Divisor %d ClockFreq 
> %dKhz\n", BaseClkFreq, Divisor, ClockFreq));
> 
> -  Status = SdMmcHcRwMmio (PciIo, Slot, SD_MMC_HC_CTRL_VER, TRUE, 
> sizeof (ControllerVer), &ControllerVer);
> +  Status = SdMmcHcGetControllerVersion (PciIo, Slot, &ControllerVer);
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
>    //
>    // Set SDCLK Frequency Select and Internal Clock Enable fields in 
> Clock Control register.
>    //
> -  if (((ControllerVer & 0xFF) >= SD_MMC_HC_CTRL_VER_300) &&
> -      ((ControllerVer & 0xFF) <= SD_MMC_HC_CTRL_VER_420)) {
> +  if ((ControllerVer >= SD_MMC_HC_CTRL_VER_300) &&
> +      (ControllerVer <= SD_MMC_HC_CTRL_VER_420)) {
>      ASSERT (Divisor <= 0x3FF);
>      ClockCtrl = ((Divisor & 0xFF) << 8) | ((Divisor & 0x300) >> 2);
> -  } else if (((ControllerVer & 0xFF) == 0) || ((ControllerVer & 0xFF) 
> == 1)) {
> +  } else if ((ControllerVer == 0) || (ControllerVer == 1)) {

Could you help to update the above 'else if' statement to use the below version definitions?

SD_MMC_HC_CTRL_VER_100
SD_MMC_HC_CTRL_VER_200

>      //
>      // Only the most significant bit can be used as divisor.
>      //
> @@ -935,6 +966,54 @@ SdMmcHcSetBusWidth (  }
> 
>  /**
> +  Configure V4 64 bit system address support at initialization.
> +
> +  @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.
> +
> +  @retval EFI_SUCCESS       The clock is supplied successfully.
> +
> +**/
> +EFI_STATUS
> +SdMmcHcV4Init64BitSupport (
> +  IN EFI_PCI_IO_PROTOCOL    *PciIo,
> +  IN UINT8                  Slot,
> +  IN SD_MMC_HC_SLOT_CAP     Capability
> +  )
> +{
> +  EFI_STATUS                Status;
> +  UINT16                    ControllerVer;
> +  UINT16                    HostCtrl2;
> +
> +  //
> +  // Check if V4 64bit support is available  //  Status = 
> + SdMmcHcGetControllerVersion (PciIo, Slot, &ControllerVer);  if 
> + (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  if (ControllerVer >= SD_MMC_HC_CTRL_VER_400) {
> +    HostCtrl2 = SD_MMC_HC_V4_EN;
> +    //
> +    // Check if V4 64bit support is available
> +    //
> +    if (Capability.SysBus64V4 == TRUE) {

Apart from the additional check needed comments above, could you help to refine the above check to:

if (Capability.SysBus64V4 != 0) {

> +      HostCtrl2 |= SD_MMC_HC_64_ADDR_EN;
> +      DEBUG ((DEBUG_INFO, "Enabled V4 64 bit system bus support\n"));
> +    }
> +    Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof
> (HostCtrl2), &HostCtrl2);
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +    DEBUG ((DEBUG_INFO, "Enabled V4 64 bit system bus support\n"));  
> + }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
>    Supply SD/MMC card with lowest clock frequency at initialization.
> 
>    @param[in] PciIo          The PCI IO protocol instance.
> @@ -1101,6 +1180,11 @@ SdMmcHcInitHost (
>    PciIo = Private->PciIo;
>    Capability = Private->Capability[Slot];
> 
> +  Status = SdMmcHcV4Init64BitSupport (PciIo, Slot, Capability);  if 
> + (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
>    Status = SdMmcHcInitClockFreq (PciIo, Slot, Capability);
>    if (EFI_ERROR (Status)) {
>      return Status;
> @@ -1169,7 +1253,7 @@ SdMmcHcLedOnOff (
>  /**
>    Build ADMA descriptor table for transfer.
> 
> -  Refer to SD Host Controller Simplified spec 3.0 Section 1.13 for details.
> +  Refer to SD Host Controller Simplified spec 4.2 Section 1.13 for details.
> 
>    @param[in] Trb            The pointer to the SD_MMC_HC_TRB instance.
> 
> @@ -1187,49 +1271,69 @@ BuildAdmaDescTable (
>    UINT64                    Entries;
>    UINT32                    Index;
>    UINT64                    Remaining;
> -  UINT32                    Address;
> +  UINT64                    Address;
>    UINTN                     TableSize;
>    EFI_PCI_IO_PROTOCOL       *PciIo;
>    EFI_STATUS                Status;
>    UINTN                     Bytes;
> +  UINT16                    ControllerVer;
> +  BOOLEAN                   AddressingMode64 = FALSE;
> +  UINTN                     DescSize = sizeof (SD_MMC_HC_ADMA_32_DESC_LINE);
> +  VOID                      *AdmaDesc = NULL;

Please help to separate the variable declaration and initial value assignment.

Also, please update the type of variable 'DescSize' from UINTN to UINT32. The Visual Studio 2015 complier complains for:

" TableSize = (UINTN)MultU64x32 (Entries, DescSize); ":

conversion from 'UINTN' to 'UINT32', possible loss of data.

> 
>    Data    = Trb->DataPhy;
>    DataLen = Trb->DataLen;
>    PciIo   = Trb->Private->PciIo;
> +
> +  //
> +  // Detect whether 64bit addressing is supported.
>    //
> -  // Only support 32bit ADMA Descriptor Table
> +  Status = SdMmcHcGetControllerVersion (PciIo, Trb->Slot, 
> + &ControllerVer);  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +  if (ControllerVer >= SD_MMC_HC_CTRL_VER_400) {
> +    Status = SdMmcHcCheckMmioSet(PciIo, Trb->Slot,
> SD_MMC_HC_HOST_CTRL2, 0x2,
> +                                 
> + SD_MMC_HC_V4_EN|SD_MMC_HC_64_ADDR_EN,
> SD_MMC_HC_V4_EN|SD_MMC_HC_64_ADDR_EN);
> +    if (!EFI_ERROR (Status)) {
> +      AddressingMode64 = TRUE;
> +      DescSize = sizeof (SD_MMC_HC_ADMA_64_DESC_LINE);
> +    }
> +  }
>    //
> -  if ((Data >= 0x100000000ul) || ((Data + DataLen) > 0x100000000ul)) 
> {
> +  // Check for valid ranges in 32bit ADMA Descriptor Table  //  if 
> + (AddressingMode64 == FALSE &&
> +      ((Data >= 0x100000000ul) || ((Data + DataLen) > 
> + 0x100000000ul))) {

Please help to update the above check to:

  if ((!AddressingMode64) &&
      ((Data >= 0x100000000ul) || ((Data + DataLen) > 0x100000000ul))) {

>      return EFI_INVALID_PARAMETER;
>    }
>    //
>    // Address field shall be set on 32-bit boundary (Lower 2-bit is 
> always set to
> 0)
> -  // for 32-bit address descriptor table.
>    //
>    if ((Data & (BIT0 | BIT1)) != 0) {
>      DEBUG ((DEBUG_INFO, "The buffer [0x%x] to construct ADMA desc is 
> not aligned to 4 bytes boundary!\n", Data));
>    }

I found that the above check (also the comments) should be updated. For 32-bit addressing the address of ADMA desc is 4-bytes aligned; for 64-bit case, the requirement is 8-byte.

Seems the align check for 64-bit addressing is missing here.

> 
>    Entries   = DivU64x32 ((DataLen + ADMA_MAX_DATA_PER_LINE - 1),
> ADMA_MAX_DATA_PER_LINE);
> -  TableSize = (UINTN)MultU64x32 (Entries, sizeof 
> (SD_MMC_HC_ADMA_DESC_LINE));
> +  TableSize = (UINTN)MultU64x32 (Entries, DescSize);
>    Trb->AdmaPages = (UINT32)EFI_SIZE_TO_PAGES (TableSize);
>    Status = PciIo->AllocateBuffer (
>                      PciIo,
>                      AllocateAnyPages,
>                      EfiBootServicesData,
>                      EFI_SIZE_TO_PAGES (TableSize),
> -                    (VOID **)&Trb->AdmaDesc,
> +                    (VOID **)&AdmaDesc,
>                      0
>                      );
>    if (EFI_ERROR (Status)) {
>      return EFI_OUT_OF_RESOURCES;
>    }
> -  ZeroMem (Trb->AdmaDesc, TableSize);
> +  ZeroMem (AdmaDesc, TableSize);
>    Bytes  = TableSize;
>    Status = PciIo->Map (
>                      PciIo,
>                      EfiPciIoOperationBusMasterCommonBuffer,
> -                    Trb->AdmaDesc,
> +                    AdmaDesc,
>                      &Bytes,
>                      &Trb->AdmaDescPhy,
>                      &Trb->AdmaMap
> @@ -1242,12 +1346,13 @@ BuildAdmaDescTable (
>      PciIo->FreeBuffer (
>               PciIo,
>               EFI_SIZE_TO_PAGES (TableSize),
> -             Trb->AdmaDesc
> +             AdmaDesc
>               );
>      return EFI_OUT_OF_RESOURCES;
>    }
> 
> -  if ((UINT64)(UINTN)Trb->AdmaDescPhy > 0x100000000ul) {
> +  if ((AddressingMode64 == FALSE) &&
> +      (UINT64)(UINTN)Trb->AdmaDescPhy > 0x100000000ul) {

Please update the above check to:

  if ((!AddressingMode64) &&
      (UINT64)(UINTN)Trb->AdmaDescPhy > 0x100000000ul) {

>      //
>      // The ADMA doesn't support 64bit addressing.
>      //
> @@ -1258,25 +1363,49 @@ BuildAdmaDescTable (
>      PciIo->FreeBuffer (
>        PciIo,
>        EFI_SIZE_TO_PAGES (TableSize),
> -      Trb->AdmaDesc
> +      AdmaDesc
>      );
>      return EFI_DEVICE_ERROR;
>    }
> 
>    Remaining = DataLen;
> -  Address   = (UINT32)Data;
> +  Address   = Data;
> +  if (AddressingMode64 == FALSE) {

Please help to update the above check to:

  if (!AddressingMode64) {

> +    Trb->Adma32Desc = AdmaDesc;
> +    Trb->Adma64Desc = NULL;
> +  } else {
> +    Trb->Adma64Desc = AdmaDesc;
> +    Trb->Adma32Desc = NULL;
> +  }
>    for (Index = 0; Index < Entries; Index++) {
> -    if (Remaining <= ADMA_MAX_DATA_PER_LINE) {
> -      Trb->AdmaDesc[Index].Valid = 1;
> -      Trb->AdmaDesc[Index].Act   = 2;
> -      Trb->AdmaDesc[Index].Length  = (UINT16)Remaining;
> -      Trb->AdmaDesc[Index].Address = Address;
> -      break;
> +    if (AddressingMode64 == FALSE) {

Please help to update the above check to:

  if (!AddressingMode64) {

> +      if (Remaining < ADMA_MAX_DATA_PER_LINE) {
> +        Trb->Adma32Desc[Index].Valid = 1;
> +        Trb->Adma32Desc[Index].Act   = 2;
> +        Trb->Adma32Desc[Index].Length  = (UINT16)Remaining;
> +        Trb->Adma32Desc[Index].Address = (UINT32)Address;
> +        break;
> +      } else {
> +        Trb->Adma32Desc[Index].Valid = 1;
> +        Trb->Adma32Desc[Index].Act   = 2;
> +        Trb->Adma32Desc[Index].Length  = 0;
> +        Trb->Adma32Desc[Index].Address = (UINT32)Address;
> +      }
>      } else {
> -      Trb->AdmaDesc[Index].Valid = 1;
> -      Trb->AdmaDesc[Index].Act   = 2;
> -      Trb->AdmaDesc[Index].Length  = 0;
> -      Trb->AdmaDesc[Index].Address = Address;
> +      if (Remaining < ADMA_MAX_DATA_PER_LINE) {
> +        Trb->Adma64Desc[Index].Valid = 1;
> +        Trb->Adma64Desc[Index].Act   = 2;
> +        Trb->Adma64Desc[Index].Length  = (UINT16)Remaining;
> +        Trb->Adma64Desc[Index].LowerAddress = (UINT32)(Address &
> MAX_UINT32);
> +        Trb->Adma64Desc[Index].UpperAddress = (UINT32)(Address>>32);
> +        break;
> +      } else {
> +        Trb->Adma64Desc[Index].Valid = 1;
> +        Trb->Adma64Desc[Index].Act   = 2;
> +        Trb->Adma64Desc[Index].Length  = 0;
> +        Trb->Adma64Desc[Index].LowerAddress = (UINT32)(Address &
> MAX_UINT32);

I think the above 2 " & MAX_UINT32" can be dropped.


> +        Trb->Adma64Desc[Index].UpperAddress = (UINT32)(Address>>32);
> +      }
>      }
> 
>      Remaining -= ADMA_MAX_DATA_PER_LINE; @@ -1286,7 +1415,7 @@ 
> BuildAdmaDescTable (
>    //
>    // Set the last descriptor line as end of descriptor table
>    //
> -  Trb->AdmaDesc[Index].End = 1;
> +  AddressingMode64 ? (Trb->Adma64Desc[Index].End = 1) : (Trb-
> >Adma32Desc[Index].End = 1);
>    return EFI_SUCCESS;
>  }
> 
> @@ -1430,11 +1559,18 @@ SdMmcFreeTrb (
>        Trb->AdmaMap
>      );
>    }
> -  if (Trb->AdmaDesc != NULL) {
> +  if (Trb->Adma32Desc != NULL) {
>      PciIo->FreeBuffer (
>        PciIo,
>        Trb->AdmaPages,
> -      Trb->AdmaDesc
> +      Trb->Adma32Desc
> +    );
> +  }
> +  if (Trb->Adma64Desc != NULL) {
> +    PciIo->FreeBuffer (
> +      PciIo,
> +      Trb->AdmaPages,
> +      Trb->Adma64Desc
>      );
>    }
>    if (Trb->DataMap != NULL) {
> @@ -1574,12 +1710,14 @@ SdMmcExecTrb (
>    UINT16                              Cmd;
>    UINT16                              IntStatus;
>    UINT32                              Argument;
> -  UINT16                              BlkCount;
> +  UINT32                              BlkCount;
>    UINT16                              BlkSize;
>    UINT16                              TransMode;
>    UINT8                               HostCtrl1;
> -  UINT32                              SdmaAddr;
> +  UINT64                              SdmaAddr;
>    UINT64                              AdmaAddr;
> +  UINT16                              ControllerVer;
> +  BOOLEAN                             AddressingMode64 = FALSE;

Please help to separate the variable declaration and initial value assignment.

> 
>    Packet = Trb->Packet;
>    PciIo  = Trb->Private->PciIo;
> @@ -1612,13 +1750,33 @@ SdMmcExecTrb (
> 
>    SdMmcHcLedOnOff (PciIo, Trb->Slot, TRUE);
> 
> +  Status = SdMmcHcGetControllerVersion (PciIo, Trb->Slot, 
> + &ControllerVer);  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +  if (ControllerVer >= SD_MMC_HC_CTRL_VER_400) {
> +    Status = SdMmcHcCheckMmioSet(PciIo, Trb->Slot,
> SD_MMC_HC_HOST_CTRL2, 0x2,
> +                                 
> + SD_MMC_HC_V4_EN|SD_MMC_HC_64_ADDR_EN,
> SD_MMC_HC_V4_EN|SD_MMC_HC_64_ADDR_EN);
> +    if (!EFI_ERROR (Status)) {
> +      AddressingMode64 = TRUE;
> +    }
> +  }
> +
>    if (Trb->Mode == SdMmcSdmaMode) {
> -    if ((UINT64)(UINTN)Trb->DataPhy >= 0x100000000ul) {
> +    if ((AddressingMode64 == FALSE) &&
> +        ((UINT64)(UINTN)Trb->DataPhy >= 0x100000000ul)) {

Please help to update the above check to:

    if ((!AddressingMode64) &&
        ((UINT64)(UINTN)Trb->DataPhy >= 0x100000000ul)) {

>        return EFI_INVALID_PARAMETER;
>      }
> 
> -    SdmaAddr = (UINT32)(UINTN)Trb->DataPhy;
> -    Status   = SdMmcHcRwMmio (PciIo, Trb->Slot, SD_MMC_HC_SDMA_ADDR,
> FALSE, sizeof (SdmaAddr), &SdmaAddr);
> +    SdmaAddr = (UINT64)(UINTN)Trb->DataPhy;
> +
> +    if (ControllerVer >= SD_MMC_HC_CTRL_VER_400) {
> +      Status = SdMmcHcRwMmio (PciIo, Trb->Slot,
> SD_MMC_HC_ADMA_SYS_ADDR, FALSE, sizeof (UINT64), &SdmaAddr);
> +    }
> +    else {

Minor coding style comment for the above line:

} else {

> +      Status = SdMmcHcRwMmio (PciIo, Trb->Slot,
> SD_MMC_HC_SDMA_ADDR, FALSE, sizeof (UINT32), &SdmaAddr);
> +    }
> +
>      if (EFI_ERROR (Status)) {
>        return Status;
>      }
> @@ -1648,9 +1806,14 @@ SdMmcExecTrb (
>      //
>      // Calcuate Block Count.
>      //
> -    BlkCount = (UINT16)(Trb->DataLen / Trb->BlockSize);
> +    BlkCount = (Trb->DataLen / Trb->BlockSize);  }

For the below 'Block Count' settings, I think it would be better to add comments that quote the SD Host Controller Spec to mention that the 32-bit block count support for all operations in SD mode was introduced at Version 4.10.

> +  if (ControllerVer >= SD_MMC_HC_CTRL_VER_410) {
> +    Status = SdMmcHcRwMmio (PciIo, Trb->Slot, SD_MMC_HC_SDMA_ADDR,
> FALSE, sizeof (UINT32), &BlkCount);
> +  }
> +  else {

Minor coding style comment for the above line:

} else {

> +    Status = SdMmcHcRwMmio (PciIo, Trb->Slot, SD_MMC_HC_BLK_COUNT,
> FALSE, sizeof (UINT16), &BlkCount);
>    }
> -  Status   = SdMmcHcRwMmio (PciIo, Trb->Slot, SD_MMC_HC_BLK_COUNT,
> FALSE, sizeof (BlkCount), &BlkCount);
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
> @@ -1746,10 +1909,11 @@ SdMmcCheckTrbResult (
>    EFI_SD_MMC_PASS_THRU_COMMAND_PACKET *Packet;
>    UINT16                              IntStatus;
>    UINT32                              Response[4];
> -  UINT32                              SdmaAddr;
> +  UINT64                              SdmaAddr;
>    UINT8                               Index;
>    UINT8                               SwReset;
>    UINT32                              PioLength;
> +  UINT16                              ControllerVer;
> 
>    SwReset = 0;
>    Packet  = Trb->Packet;
> @@ -1870,19 +2034,42 @@ SdMmcCheckTrbResult (
>      //
>      // Update SDMA Address register.
>      //
> -    SdmaAddr = SD_MMC_SDMA_ROUND_UP ((UINT32)(UINTN)Trb-
> >DataPhy, SD_MMC_SDMA_BOUNDARY);
> -    Status   = SdMmcHcRwMmio (
> +    SdmaAddr = SD_MMC_SDMA_ROUND_UP ((UINTN)Trb->DataPhy,
> SD_MMC_SDMA_BOUNDARY);
> +
> +    Status = SdMmcHcGetControllerVersion (
> +               Private->PciIo,
> +               Trb->Slot,
> +               &ControllerVer
> +               );
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +
> +    if (ControllerVer >= SD_MMC_HC_CTRL_VER_400) {
> +      Status = SdMmcHcRwMmio (
>                   Private->PciIo,
>                   Trb->Slot,
> -                 SD_MMC_HC_SDMA_ADDR,
> +                 SD_MMC_HC_ADMA_SYS_ADDR,
>                   FALSE,
> -                 sizeof (UINT32),
> +                 sizeof (UINT64),
>                   &SdmaAddr
>                   );
> +    }
> +    else {
> +        Status = SdMmcHcRwMmio (
> +                   Private->PciIo,
> +                   Trb->Slot,
> +                   SD_MMC_HC_SDMA_ADDR,
> +                   FALSE,
> +                   sizeof (UINT32),
> +                   &SdmaAddr
> +                   );
> +    }

Minor coding style comments for the above line:

1. } else {
2. Please help to refine the space indent of the above SdMmcHcRwMmio() call


Best Regards,
Hao Wu

> +
>      if (EFI_ERROR (Status)) {
>        goto Done;
>      }
> -    Trb->DataPhy = (UINT32)(UINTN)SdmaAddr;
> +    Trb->DataPhy = (UINT64)(UINTN)SdmaAddr;
>    }
> 
>    if ((Packet->SdMmcCmdBlk->CommandType != SdMmcCommandTypeAdtc) && 
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> index cc138fc..a6234f1 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> @@ -2,6 +2,7 @@
> 
>    Provides some data structure definitions used by the SD/MMC host 
> controller driver.
> 
> +Copyright (c) 2018, NVIDIA CORPORATION. All rights reserved.
>  Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>  This 
> program and the accompanying materials  are licensed and made 
> available under the terms and conditions of the BSD License @@ -78,6 
> +79,9 @@ typedef enum {  //
>  #define ADMA_MAX_DATA_PER_LINE     0x10000
> 
> +//
> +// ADMA descriptor for 32b addressing.
> +//
>  typedef struct {
>    UINT32 Valid:1;
>    UINT32 End:1;
> @@ -87,7 +91,23 @@ typedef struct {
>    UINT32 Reserved1:10;
>    UINT32 Length:16;
>    UINT32 Address;
> -} SD_MMC_HC_ADMA_DESC_LINE;
> +} SD_MMC_HC_ADMA_32_DESC_LINE;
> +
> +//
> +// ADMA descriptor for 64b addressing.
> +//
> +typedef struct {
> +  UINT32 Valid:1;
> +  UINT32 End:1;
> +  UINT32 Int:1;
> +  UINT32 Reserved:1;
> +  UINT32 Act:2;
> +  UINT32 Reserved1:10;
> +  UINT32 Length:16;
> +  UINT32 LowerAddress;
> +  UINT32 UpperAddress;
> +  UINT32 Reserved2;
> +} SD_MMC_HC_ADMA_64_DESC_LINE;
> 
>  #define SD_MMC_SDMA_BOUNDARY          512 * 1024
>  #define SD_MMC_SDMA_ROUND_UP(x, n)    (((x) + n) & ~(n - 1))
> @@ -145,6 +165,12 @@ typedef struct {
>  #define SD_MMC_HC_CTRL_VER_410      0x04
>  #define SD_MMC_HC_CTRL_VER_420      0x05
> 
> +//
> +// SD Host controller V4 Support
> +//
> +#define SD_MMC_HC_V4_EN             BIT12
> +#define SD_MMC_HC_64_ADDR_EN        BIT13
> +
>  /**
>    Dump the content of SD/MMC host controller's Capability Register.
> 
> --
> 2.7.4
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Re: [PATCH v3 2/2] MdeModulePkg/SdMmcPciHcDxe: Add V4 64bit SDMA and ADMA2 support.

[Ashish Singhal]

Missed one of your suggested compiler related change in Patch 5. Have posted patch 6.

Thanks
Ashish Singhal

-----Original Message-----
From: Ashish Singhal 
Sent: Thursday, November 29, 2018 11:48 AM
To: 'Wu, Hao A' <hao.a.wu@intel.com>; edk2-devel@lists.01.org
Subject: RE: [edk2] [PATCH v3 2/2] MdeModulePkg/SdMmcPciHcDxe: Add V4 64bit SDMA and ADMA2 support.

Hello Hao,

Answers to your comments:

1. Patch 5 fixes the issue with accessing SD as well as eMMC files.
2. Using Private variable for controller version would mean I can only access it in functions having an instance of the installed protocol which is not possible in some functions for example where we set clock dividers. We either need to use what I have or pass Private or controller version as an argument to functions using it.
3. I think there is a bigger issue here. During host controller initialization we need to setup 64b addressing which happens before we initialize 64b DMA in PCI layer. So what you are suggesting may not be that helpful. Also, what we are doing right now is that we go through all slots and if controller on any slot does not support 64b, we do not enable 64b DMA in PCI layer. What is the likelihood of all controllers on an SOC not supporting similar address width? Also, shouldn't we be enabling 64b DMA in PCI layer if any of the controller supports 64b addressing? Wouldn't 64b DMA enabled mean 32b is enabled by default?
4. Patch 5 has been rebased on tip of edk2.

Thanks
Ashish

-----Original Message-----
From: Wu, Hao A <hao.a.wu@intel.com>
Sent: Wednesday, November 28, 2018 12:25 AM
To: Ashish Singhal <ashishsingha@nvidia.com>; edk2-devel@lists.01.org
Subject: RE: [edk2] [PATCH v3 2/2] MdeModulePkg/SdMmcPciHcDxe: Add V4 64bit SDMA and ADMA2 support.

Hello,

Sorry for the delayed response.

Apart from inserting the Bugzilla tracker information, several more general level
comments:

1. Cannot access the files (content) on SD & eMMC devices

After applying (rebasing onto the latest codes) your patches, I found that though the SD & eMMC devices can be detected, yet the content cannot be accessed.

There are 1 SD card and 1 embedded eMMC device within the system. Under Shell, I can see 2 file systems on SD & eMMC devices being detected, but when I try to access the files on those file systems by using the 'ls' command, it fails, saying "ls: File Not Found - 'FS0:\'". I can confirm that files can be listed successfully without applying this patch.

I also tried the 'dblk' command for display the data via BlockIO protocols created on those devices. I found that for SD, the command works fine. But for eMMC, the command fails with message "dblk: Read file error - 'BlockIo'".

For the hardware I own, the controllers are all version 3.0. I tested on both
IA32 and X64 arch image. The results were the same as described above.

Unfortunately, I do not have access to boards with SD controller with version newer than 3.0. So I am not able to perform those tests on my side for Version
4.0 or greater SD controllers.

Do you have any hardware with SD controller of version 3.0? Is it working on your side?

Please let me know if additional information is needed.


2. On SdMmcHcGetControllerVersion()

I found that there is a 'ControllerVersion' version in the data structure SD_MMC_HC_PRIVATE_DATA. But currently, this field seems never used.

I think we can get the controller version information only once and use this field to store it. Hence, this new function can be eliminated and also can avoid calling it multiple times.


3. Setting the SD_MMC_HC_64_ADDR_EN bit in SdMmcHcV4Init64BitSupport()

The setting of the SD_MMC_HC_64_ADDR_EN bit is decided by the 'SysBus64V4'
field in the CAP register. For me, additional dependency on the 64-bit DMA support in the PCI layer is needed as well.

In function SdMmcPciHcDriverBindingStart():

  //
  // Enable 64-bit DMA support in the PCI layer if this controller
  // supports it.
  //
  if (Support64BitDma) {
    Status = PciIo->Attributes (
                      PciIo,
                      EfiPciIoAttributeOperationEnable,
                      EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE,
                      NULL
                      );
    if (EFI_ERROR (Status)) {
      DEBUG ((DEBUG_WARN, "SdMmcPciHcDriverBindingStart: failed to enable 64-bit DMA (%r)\n", Status));
    }
  }

If the above PCI attribute setting fails, the PCI layer will not support the 64-bit DMA.

Hence, I am thinking to introduce a new BOOLEAN field in the "SD_MMC_HC_PRIVATE_DATA" data structure (maybe putting the 'Support64BitDma'
into the data structure). If the above PCI operation succeeds, the BOOLEAN field will be set to TRUE, otherwise, FALSE.

And the setting of the SD_MMC_HC_64_ADDR_EN bit should depend on the BOOLEAN field as well.


4. Please help to rebase the patch upon the latest master branch.

Some changes within the SdMmcPciHcDxe have been pushed recently. Could you help to rebase this patch upon the latest changes. Thanks in advance.


Also, some inline comments below.


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 
> Ashish Singhal
> Sent: Tuesday, November 20, 2018 4:59 AM
> To: edk2-devel@lists.01.org
> Cc: Ashish Singhal
> Subject: [edk2] [PATCH v3 2/2] MdeModulePkg/SdMmcPciHcDxe: Add V4 
> 64bit SDMA and ADMA2 support.
> 
> If V4 64 bit address mode is enabled in compatibility register, 
> program controller to enable V4 host mode.
> Use appropriate ADMA2 descriptors supporting 64 bit addresses.
> Use appropriate registers for SDMA mode operation.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ashish Singhal <ashishsingha@nvidia.com>
> ---
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h |   4 +-
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   | 273
> +++++++++++++++++----
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h   |  28 ++-
>  3 files changed, 260 insertions(+), 45 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> index c683600..22795df 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> @@ -2,6 +2,7 @@
> 
>    Provides some data structure definitions used by the SD/MMC host 
> controller driver.
> 
> +Copyright (c) 2018, NVIDIA CORPORATION. All rights reserved.
>  Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>  This 
> program and the accompanying materials  are licensed and made 
> available under the terms and conditions of the BSD License @@ -144,7
> +145,8 @@ typedef struct {
>    BOOLEAN                             Started;
>    UINT64                              Timeout;
> 
> -  SD_MMC_HC_ADMA_DESC_LINE            *AdmaDesc;
> +  SD_MMC_HC_ADMA_32_DESC_LINE         *Adma32Desc;
> +  SD_MMC_HC_ADMA_64_DESC_LINE         *Adma64Desc;
>    EFI_PHYSICAL_ADDRESS                AdmaDescPhy;
>    VOID                                *AdmaMap;
>    UINT32                              AdmaPages;
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> index e506875..9fef3fb 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> @@ -4,6 +4,7 @@
> 
>    It would expose EFI_SD_MMC_PASS_THRU_PROTOCOL for upper layer use.
> 
> +  Copyright (c) 2018, NVIDIA CORPORATION. All rights reserved.
>    Copyright (c) 2015 - 2017, Intel Corporation. All rights reserved.<BR>
>    This program and the accompanying materials
>    are licensed and made available under the terms and conditions of 
> the BSD License @@ -418,6 +419,36 @@ SdMmcHcWaitMmioSet (  }
> 
>  /**
> +  Get the controller version information from the specified slot.
> +
> +  @param[in]  PciIo           The PCI IO protocol instance.
> +  @param[in]  Slot            The slot number of the SD card to send the
> command to.
> +  @param[out] Version         The buffer to store the version information.
> +
> +  @retval EFI_SUCCESS         The operation executes successfully.
> +  @retval Others              The operation fails.
> +
> +**/
> +EFI_STATUS
> +SdMmcHcGetControllerVersion (
> +  IN     EFI_PCI_IO_PROTOCOL  *PciIo,
> +  IN     UINT8                Slot,
> +     OUT UINT16               *Version
> +  )
> +{
> +  EFI_STATUS                Status;
> +
> +  Status = SdMmcHcRwMmio (PciIo, Slot, SD_MMC_HC_CTRL_VER, TRUE,
> sizeof (UINT16), Version);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  *Version &= 0xFF;
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
>    Software reset the specified SD/MMC host controller and enable all 
> interrupts.
> 
>    @param[in] Private        A pointer to the SD_MMC_HC_PRIVATE_DATA
> instance.
> @@ -776,18 +807,18 @@ SdMmcHcClockSupply (
> 
>    DEBUG ((DEBUG_INFO, "BaseClkFreq %dMHz Divisor %d ClockFreq 
> %dKhz\n", BaseClkFreq, Divisor, ClockFreq));
> 
> -  Status = SdMmcHcRwMmio (PciIo, Slot, SD_MMC_HC_CTRL_VER, TRUE, 
> sizeof (ControllerVer), &ControllerVer);
> +  Status = SdMmcHcGetControllerVersion (PciIo, Slot, &ControllerVer);
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
>    //
>    // Set SDCLK Frequency Select and Internal Clock Enable fields in 
> Clock Control register.
>    //
> -  if (((ControllerVer & 0xFF) >= SD_MMC_HC_CTRL_VER_300) &&
> -      ((ControllerVer & 0xFF) <= SD_MMC_HC_CTRL_VER_420)) {
> +  if ((ControllerVer >= SD_MMC_HC_CTRL_VER_300) &&
> +      (ControllerVer <= SD_MMC_HC_CTRL_VER_420)) {
>      ASSERT (Divisor <= 0x3FF);
>      ClockCtrl = ((Divisor & 0xFF) << 8) | ((Divisor & 0x300) >> 2);
> -  } else if (((ControllerVer & 0xFF) == 0) || ((ControllerVer & 0xFF) 
> == 1)) {
> +  } else if ((ControllerVer == 0) || (ControllerVer == 1)) {

Could you help to update the above 'else if' statement to use the below version definitions?

SD_MMC_HC_CTRL_VER_100
SD_MMC_HC_CTRL_VER_200

>      //
>      // Only the most significant bit can be used as divisor.
>      //
> @@ -935,6 +966,54 @@ SdMmcHcSetBusWidth (  }
> 
>  /**
> +  Configure V4 64 bit system address support at initialization.
> +
> +  @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.
> +
> +  @retval EFI_SUCCESS       The clock is supplied successfully.
> +
> +**/
> +EFI_STATUS
> +SdMmcHcV4Init64BitSupport (
> +  IN EFI_PCI_IO_PROTOCOL    *PciIo,
> +  IN UINT8                  Slot,
> +  IN SD_MMC_HC_SLOT_CAP     Capability
> +  )
> +{
> +  EFI_STATUS                Status;
> +  UINT16                    ControllerVer;
> +  UINT16                    HostCtrl2;
> +
> +  //
> +  // Check if V4 64bit support is available  //  Status = 
> + SdMmcHcGetControllerVersion (PciIo, Slot, &ControllerVer);  if 
> + (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  if (ControllerVer >= SD_MMC_HC_CTRL_VER_400) {
> +    HostCtrl2 = SD_MMC_HC_V4_EN;
> +    //
> +    // Check if V4 64bit support is available
> +    //
> +    if (Capability.SysBus64V4 == TRUE) {

Apart from the additional check needed comments above, could you help to refine the above check to:

if (Capability.SysBus64V4 != 0) {

> +      HostCtrl2 |= SD_MMC_HC_64_ADDR_EN;
> +      DEBUG ((DEBUG_INFO, "Enabled V4 64 bit system bus support\n"));
> +    }
> +    Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof
> (HostCtrl2), &HostCtrl2);
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +    DEBUG ((DEBUG_INFO, "Enabled V4 64 bit system bus support\n")); }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
>    Supply SD/MMC card with lowest clock frequency at initialization.
> 
>    @param[in] PciIo          The PCI IO protocol instance.
> @@ -1101,6 +1180,11 @@ SdMmcHcInitHost (
>    PciIo = Private->PciIo;
>    Capability = Private->Capability[Slot];
> 
> +  Status = SdMmcHcV4Init64BitSupport (PciIo, Slot, Capability);  if 
> + (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
>    Status = SdMmcHcInitClockFreq (PciIo, Slot, Capability);
>    if (EFI_ERROR (Status)) {
>      return Status;
> @@ -1169,7 +1253,7 @@ SdMmcHcLedOnOff (
>  /**
>    Build ADMA descriptor table for transfer.
> 
> -  Refer to SD Host Controller Simplified spec 3.0 Section 1.13 for details.
> +  Refer to SD Host Controller Simplified spec 4.2 Section 1.13 for details.
> 
>    @param[in] Trb            The pointer to the SD_MMC_HC_TRB instance.
> 
> @@ -1187,49 +1271,69 @@ BuildAdmaDescTable (
>    UINT64                    Entries;
>    UINT32                    Index;
>    UINT64                    Remaining;
> -  UINT32                    Address;
> +  UINT64                    Address;
>    UINTN                     TableSize;
>    EFI_PCI_IO_PROTOCOL       *PciIo;
>    EFI_STATUS                Status;
>    UINTN                     Bytes;
> +  UINT16                    ControllerVer;
> +  BOOLEAN                   AddressingMode64 = FALSE;
> +  UINTN                     DescSize = sizeof (SD_MMC_HC_ADMA_32_DESC_LINE);
> +  VOID                      *AdmaDesc = NULL;

Please help to separate the variable declaration and initial value assignment.

Also, please update the type of variable 'DescSize' from UINTN to UINT32. The Visual Studio 2015 complier complains for:

" TableSize = (UINTN)MultU64x32 (Entries, DescSize); ":

conversion from 'UINTN' to 'UINT32', possible loss of data.

> 
>    Data    = Trb->DataPhy;
>    DataLen = Trb->DataLen;
>    PciIo   = Trb->Private->PciIo;
> +
> +  //
> +  // Detect whether 64bit addressing is supported.
>    //
> -  // Only support 32bit ADMA Descriptor Table
> +  Status = SdMmcHcGetControllerVersion (PciIo, Trb->Slot, 
> + &ControllerVer);  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +  if (ControllerVer >= SD_MMC_HC_CTRL_VER_400) {
> +    Status = SdMmcHcCheckMmioSet(PciIo, Trb->Slot,
> SD_MMC_HC_HOST_CTRL2, 0x2,
> +                                 
> + SD_MMC_HC_V4_EN|SD_MMC_HC_64_ADDR_EN,
> SD_MMC_HC_V4_EN|SD_MMC_HC_64_ADDR_EN);
> +    if (!EFI_ERROR (Status)) {
> +      AddressingMode64 = TRUE;
> +      DescSize = sizeof (SD_MMC_HC_ADMA_64_DESC_LINE);
> +    }
> +  }
>    //
> -  if ((Data >= 0x100000000ul) || ((Data + DataLen) > 0x100000000ul)) 
> {
> +  // Check for valid ranges in 32bit ADMA Descriptor Table  //  if
> + (AddressingMode64 == FALSE &&
> +      ((Data >= 0x100000000ul) || ((Data + DataLen) >
> + 0x100000000ul))) {

Please help to update the above check to:

  if ((!AddressingMode64) &&
      ((Data >= 0x100000000ul) || ((Data + DataLen) > 0x100000000ul))) {

>      return EFI_INVALID_PARAMETER;
>    }
>    //
>    // Address field shall be set on 32-bit boundary (Lower 2-bit is 
> always set to
> 0)
> -  // for 32-bit address descriptor table.
>    //
>    if ((Data & (BIT0 | BIT1)) != 0) {
>      DEBUG ((DEBUG_INFO, "The buffer [0x%x] to construct ADMA desc is 
> not aligned to 4 bytes boundary!\n", Data));
>    }

I found that the above check (also the comments) should be updated. For 32-bit addressing the address of ADMA desc is 4-bytes aligned; for 64-bit case, the requirement is 8-byte.

Seems the align check for 64-bit addressing is missing here.

> 
>    Entries   = DivU64x32 ((DataLen + ADMA_MAX_DATA_PER_LINE - 1),
> ADMA_MAX_DATA_PER_LINE);
> -  TableSize = (UINTN)MultU64x32 (Entries, sizeof 
> (SD_MMC_HC_ADMA_DESC_LINE));
> +  TableSize = (UINTN)MultU64x32 (Entries, DescSize);
>    Trb->AdmaPages = (UINT32)EFI_SIZE_TO_PAGES (TableSize);
>    Status = PciIo->AllocateBuffer (
>                      PciIo,
>                      AllocateAnyPages,
>                      EfiBootServicesData,
>                      EFI_SIZE_TO_PAGES (TableSize),
> -                    (VOID **)&Trb->AdmaDesc,
> +                    (VOID **)&AdmaDesc,
>                      0
>                      );
>    if (EFI_ERROR (Status)) {
>      return EFI_OUT_OF_RESOURCES;
>    }
> -  ZeroMem (Trb->AdmaDesc, TableSize);
> +  ZeroMem (AdmaDesc, TableSize);
>    Bytes  = TableSize;
>    Status = PciIo->Map (
>                      PciIo,
>                      EfiPciIoOperationBusMasterCommonBuffer,
> -                    Trb->AdmaDesc,
> +                    AdmaDesc,
>                      &Bytes,
>                      &Trb->AdmaDescPhy,
>                      &Trb->AdmaMap
> @@ -1242,12 +1346,13 @@ BuildAdmaDescTable (
>      PciIo->FreeBuffer (
>               PciIo,
>               EFI_SIZE_TO_PAGES (TableSize),
> -             Trb->AdmaDesc
> +             AdmaDesc
>               );
>      return EFI_OUT_OF_RESOURCES;
>    }
> 
> -  if ((UINT64)(UINTN)Trb->AdmaDescPhy > 0x100000000ul) {
> +  if ((AddressingMode64 == FALSE) &&
> +      (UINT64)(UINTN)Trb->AdmaDescPhy > 0x100000000ul) {

Please update the above check to:

  if ((!AddressingMode64) &&
      (UINT64)(UINTN)Trb->AdmaDescPhy > 0x100000000ul) {

>      //
>      // The ADMA doesn't support 64bit addressing.
>      //
> @@ -1258,25 +1363,49 @@ BuildAdmaDescTable (
>      PciIo->FreeBuffer (
>        PciIo,
>        EFI_SIZE_TO_PAGES (TableSize),
> -      Trb->AdmaDesc
> +      AdmaDesc
>      );
>      return EFI_DEVICE_ERROR;
>    }
> 
>    Remaining = DataLen;
> -  Address   = (UINT32)Data;
> +  Address   = Data;
> +  if (AddressingMode64 == FALSE) {

Please help to update the above check to:

  if (!AddressingMode64) {

> +    Trb->Adma32Desc = AdmaDesc;
> +    Trb->Adma64Desc = NULL;
> +  } else {
> +    Trb->Adma64Desc = AdmaDesc;
> +    Trb->Adma32Desc = NULL;
> +  }
>    for (Index = 0; Index < Entries; Index++) {
> -    if (Remaining <= ADMA_MAX_DATA_PER_LINE) {
> -      Trb->AdmaDesc[Index].Valid = 1;
> -      Trb->AdmaDesc[Index].Act   = 2;
> -      Trb->AdmaDesc[Index].Length  = (UINT16)Remaining;
> -      Trb->AdmaDesc[Index].Address = Address;
> -      break;
> +    if (AddressingMode64 == FALSE) {

Please help to update the above check to:

  if (!AddressingMode64) {

> +      if (Remaining < ADMA_MAX_DATA_PER_LINE) {
> +        Trb->Adma32Desc[Index].Valid = 1;
> +        Trb->Adma32Desc[Index].Act   = 2;
> +        Trb->Adma32Desc[Index].Length  = (UINT16)Remaining;
> +        Trb->Adma32Desc[Index].Address = (UINT32)Address;
> +        break;
> +      } else {
> +        Trb->Adma32Desc[Index].Valid = 1;
> +        Trb->Adma32Desc[Index].Act   = 2;
> +        Trb->Adma32Desc[Index].Length  = 0;
> +        Trb->Adma32Desc[Index].Address = (UINT32)Address;
> +      }
>      } else {
> -      Trb->AdmaDesc[Index].Valid = 1;
> -      Trb->AdmaDesc[Index].Act   = 2;
> -      Trb->AdmaDesc[Index].Length  = 0;
> -      Trb->AdmaDesc[Index].Address = Address;
> +      if (Remaining < ADMA_MAX_DATA_PER_LINE) {
> +        Trb->Adma64Desc[Index].Valid = 1;
> +        Trb->Adma64Desc[Index].Act   = 2;
> +        Trb->Adma64Desc[Index].Length  = (UINT16)Remaining;
> +        Trb->Adma64Desc[Index].LowerAddress = (UINT32)(Address &
> MAX_UINT32);
> +        Trb->Adma64Desc[Index].UpperAddress = (UINT32)(Address>>32);
> +        break;
> +      } else {
> +        Trb->Adma64Desc[Index].Valid = 1;
> +        Trb->Adma64Desc[Index].Act   = 2;
> +        Trb->Adma64Desc[Index].Length  = 0;
> +        Trb->Adma64Desc[Index].LowerAddress = (UINT32)(Address &
> MAX_UINT32);

I think the above 2 " & MAX_UINT32" can be dropped.


> +        Trb->Adma64Desc[Index].UpperAddress = (UINT32)(Address>>32);
> +      }
>      }
> 
>      Remaining -= ADMA_MAX_DATA_PER_LINE; @@ -1286,7 +1415,7 @@ 
> BuildAdmaDescTable (
>    //
>    // Set the last descriptor line as end of descriptor table
>    //
> -  Trb->AdmaDesc[Index].End = 1;
> +  AddressingMode64 ? (Trb->Adma64Desc[Index].End = 1) : (Trb-
> >Adma32Desc[Index].End = 1);
>    return EFI_SUCCESS;
>  }
> 
> @@ -1430,11 +1559,18 @@ SdMmcFreeTrb (
>        Trb->AdmaMap
>      );
>    }
> -  if (Trb->AdmaDesc != NULL) {
> +  if (Trb->Adma32Desc != NULL) {
>      PciIo->FreeBuffer (
>        PciIo,
>        Trb->AdmaPages,
> -      Trb->AdmaDesc
> +      Trb->Adma32Desc
> +    );
> +  }
> +  if (Trb->Adma64Desc != NULL) {
> +    PciIo->FreeBuffer (
> +      PciIo,
> +      Trb->AdmaPages,
> +      Trb->Adma64Desc
>      );
>    }
>    if (Trb->DataMap != NULL) {
> @@ -1574,12 +1710,14 @@ SdMmcExecTrb (
>    UINT16                              Cmd;
>    UINT16                              IntStatus;
>    UINT32                              Argument;
> -  UINT16                              BlkCount;
> +  UINT32                              BlkCount;
>    UINT16                              BlkSize;
>    UINT16                              TransMode;
>    UINT8                               HostCtrl1;
> -  UINT32                              SdmaAddr;
> +  UINT64                              SdmaAddr;
>    UINT64                              AdmaAddr;
> +  UINT16                              ControllerVer;
> +  BOOLEAN                             AddressingMode64 = FALSE;

Please help to separate the variable declaration and initial value assignment.

> 
>    Packet = Trb->Packet;
>    PciIo  = Trb->Private->PciIo;
> @@ -1612,13 +1750,33 @@ SdMmcExecTrb (
> 
>    SdMmcHcLedOnOff (PciIo, Trb->Slot, TRUE);
> 
> +  Status = SdMmcHcGetControllerVersion (PciIo, Trb->Slot, 
> + &ControllerVer);  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +  if (ControllerVer >= SD_MMC_HC_CTRL_VER_400) {
> +    Status = SdMmcHcCheckMmioSet(PciIo, Trb->Slot,
> SD_MMC_HC_HOST_CTRL2, 0x2,
> +                                 
> + SD_MMC_HC_V4_EN|SD_MMC_HC_64_ADDR_EN,
> SD_MMC_HC_V4_EN|SD_MMC_HC_64_ADDR_EN);
> +    if (!EFI_ERROR (Status)) {
> +      AddressingMode64 = TRUE;
> +    }
> +  }
> +
>    if (Trb->Mode == SdMmcSdmaMode) {
> -    if ((UINT64)(UINTN)Trb->DataPhy >= 0x100000000ul) {
> +    if ((AddressingMode64 == FALSE) &&
> +        ((UINT64)(UINTN)Trb->DataPhy >= 0x100000000ul)) {

Please help to update the above check to:

    if ((!AddressingMode64) &&
        ((UINT64)(UINTN)Trb->DataPhy >= 0x100000000ul)) {

>        return EFI_INVALID_PARAMETER;
>      }
> 
> -    SdmaAddr = (UINT32)(UINTN)Trb->DataPhy;
> -    Status   = SdMmcHcRwMmio (PciIo, Trb->Slot, SD_MMC_HC_SDMA_ADDR,
> FALSE, sizeof (SdmaAddr), &SdmaAddr);
> +    SdmaAddr = (UINT64)(UINTN)Trb->DataPhy;
> +
> +    if (ControllerVer >= SD_MMC_HC_CTRL_VER_400) {
> +      Status = SdMmcHcRwMmio (PciIo, Trb->Slot,
> SD_MMC_HC_ADMA_SYS_ADDR, FALSE, sizeof (UINT64), &SdmaAddr);
> +    }
> +    else {

Minor coding style comment for the above line:

} else {

> +      Status = SdMmcHcRwMmio (PciIo, Trb->Slot,
> SD_MMC_HC_SDMA_ADDR, FALSE, sizeof (UINT32), &SdmaAddr);
> +    }
> +
>      if (EFI_ERROR (Status)) {
>        return Status;
>      }
> @@ -1648,9 +1806,14 @@ SdMmcExecTrb (
>      //
>      // Calcuate Block Count.
>      //
> -    BlkCount = (UINT16)(Trb->DataLen / Trb->BlockSize);
> +    BlkCount = (Trb->DataLen / Trb->BlockSize);  }

For the below 'Block Count' settings, I think it would be better to add comments that quote the SD Host Controller Spec to mention that the 32-bit block count support for all operations in SD mode was introduced at Version 4.10.

> +  if (ControllerVer >= SD_MMC_HC_CTRL_VER_410) {
> +    Status = SdMmcHcRwMmio (PciIo, Trb->Slot, SD_MMC_HC_SDMA_ADDR,
> FALSE, sizeof (UINT32), &BlkCount);
> +  }
> +  else {

Minor coding style comment for the above line:

} else {

> +    Status = SdMmcHcRwMmio (PciIo, Trb->Slot, SD_MMC_HC_BLK_COUNT,
> FALSE, sizeof (UINT16), &BlkCount);
>    }
> -  Status   = SdMmcHcRwMmio (PciIo, Trb->Slot, SD_MMC_HC_BLK_COUNT,
> FALSE, sizeof (BlkCount), &BlkCount);
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
> @@ -1746,10 +1909,11 @@ SdMmcCheckTrbResult (
>    EFI_SD_MMC_PASS_THRU_COMMAND_PACKET *Packet;
>    UINT16                              IntStatus;
>    UINT32                              Response[4];
> -  UINT32                              SdmaAddr;
> +  UINT64                              SdmaAddr;
>    UINT8                               Index;
>    UINT8                               SwReset;
>    UINT32                              PioLength;
> +  UINT16                              ControllerVer;
> 
>    SwReset = 0;
>    Packet  = Trb->Packet;
> @@ -1870,19 +2034,42 @@ SdMmcCheckTrbResult (
>      //
>      // Update SDMA Address register.
>      //
> -    SdmaAddr = SD_MMC_SDMA_ROUND_UP ((UINT32)(UINTN)Trb-
> >DataPhy, SD_MMC_SDMA_BOUNDARY);
> -    Status   = SdMmcHcRwMmio (
> +    SdmaAddr = SD_MMC_SDMA_ROUND_UP ((UINTN)Trb->DataPhy,
> SD_MMC_SDMA_BOUNDARY);
> +
> +    Status = SdMmcHcGetControllerVersion (
> +               Private->PciIo,
> +               Trb->Slot,
> +               &ControllerVer
> +               );
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +
> +    if (ControllerVer >= SD_MMC_HC_CTRL_VER_400) {
> +      Status = SdMmcHcRwMmio (
>                   Private->PciIo,
>                   Trb->Slot,
> -                 SD_MMC_HC_SDMA_ADDR,
> +                 SD_MMC_HC_ADMA_SYS_ADDR,
>                   FALSE,
> -                 sizeof (UINT32),
> +                 sizeof (UINT64),
>                   &SdmaAddr
>                   );
> +    }
> +    else {
> +        Status = SdMmcHcRwMmio (
> +                   Private->PciIo,
> +                   Trb->Slot,
> +                   SD_MMC_HC_SDMA_ADDR,
> +                   FALSE,
> +                   sizeof (UINT32),
> +                   &SdmaAddr
> +                   );
> +    }

Minor coding style comments for the above line:

1. } else {
2. Please help to refine the space indent of the above SdMmcHcRwMmio() call


Best Regards,
Hao Wu

> +
>      if (EFI_ERROR (Status)) {
>        goto Done;
>      }
> -    Trb->DataPhy = (UINT32)(UINTN)SdmaAddr;
> +    Trb->DataPhy = (UINT64)(UINTN)SdmaAddr;
>    }
> 
>    if ((Packet->SdMmcCmdBlk->CommandType != SdMmcCommandTypeAdtc) && 
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> index cc138fc..a6234f1 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> @@ -2,6 +2,7 @@
> 
>    Provides some data structure definitions used by the SD/MMC host 
> controller driver.
> 
> +Copyright (c) 2018, NVIDIA CORPORATION. All rights reserved.
>  Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>  This 
> program and the accompanying materials  are licensed and made 
> available under the terms and conditions of the BSD License @@ -78,6
> +79,9 @@ typedef enum {  //
>  #define ADMA_MAX_DATA_PER_LINE     0x10000
> 
> +//
> +// ADMA descriptor for 32b addressing.
> +//
>  typedef struct {
>    UINT32 Valid:1;
>    UINT32 End:1;
> @@ -87,7 +91,23 @@ typedef struct {
>    UINT32 Reserved1:10;
>    UINT32 Length:16;
>    UINT32 Address;
> -} SD_MMC_HC_ADMA_DESC_LINE;
> +} SD_MMC_HC_ADMA_32_DESC_LINE;
> +
> +//
> +// ADMA descriptor for 64b addressing.
> +//
> +typedef struct {
> +  UINT32 Valid:1;
> +  UINT32 End:1;
> +  UINT32 Int:1;
> +  UINT32 Reserved:1;
> +  UINT32 Act:2;
> +  UINT32 Reserved1:10;
> +  UINT32 Length:16;
> +  UINT32 LowerAddress;
> +  UINT32 UpperAddress;
> +  UINT32 Reserved2;
> +} SD_MMC_HC_ADMA_64_DESC_LINE;
> 
>  #define SD_MMC_SDMA_BOUNDARY          512 * 1024
>  #define SD_MMC_SDMA_ROUND_UP(x, n)    (((x) + n) & ~(n - 1))
> @@ -145,6 +165,12 @@ typedef struct {
>  #define SD_MMC_HC_CTRL_VER_410      0x04
>  #define SD_MMC_HC_CTRL_VER_420      0x05
> 
> +//
> +// SD Host controller V4 Support
> +//
> +#define SD_MMC_HC_V4_EN             BIT12
> +#define SD_MMC_HC_64_ADDR_EN        BIT13
> +
>  /**
>    Dump the content of SD/MMC host controller's Capability Register.
> 
> --
> 2.7.4
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Re: [PATCH v3 2/2] MdeModulePkg/SdMmcPciHcDxe: Add V4 64bit SDMA and ADMA2 support.

[Wu, Hao A]

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Ashish Singhal
> Sent: Friday, November 30, 2018 2:48 AM
> To: Wu, Hao A; edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH v3 2/2] MdeModulePkg/SdMmcPciHcDxe: Add
> V4 64bit SDMA and ADMA2 support.
> 
> Hello Hao,
> 
> Answers to your comments:
> 
> 1. Patch 5 fixes the issue with accessing SD as well as eMMC files.

Hello,

The issue still exists on my side.
Please refer to the comments within the V6 2/2 patch.

> 2. Using Private variable for controller version would mean I can only access it
> in functions having an instance of the installed protocol which is not possible
> in some functions for example where we set clock dividers. We either need
> to use what I have or pass Private or controller version as an argument to
> functions using it.

The solution for this is not very complex.

I checked all the callers of SdMmcHcGetControllerVersion() in this patch:

SdMmcHcClockSupply()
SdMmcHcInitV4Enhancements()
BuildAdmaDescTable()
SdMmcExecTrb()
SdMmcCheckTrbResult()

For SdMmcExecTrb() and SdMmcCheckTrbResult(), they already have the access to
'Private'.

For SdMmcHcInitV4Enhancements() and BuildAdmaDescTable(), their callers all
have the access to 'Private'. So we can either:

1. Add 'Private' to the input parameter
2. Add 'ControllerVersion' to the input parameter

for the above 2 functions. Personally, I prefer the 2nd option.

As for SdMmcHcClockSupply(), among its callers, only SdMmcHcInitClockFreq()
does not have direct access to 'Private'. However, SdMmcHcInitClockFreq() is
able to get the ControllerVersion or 'Private' from its caller.

Hence, ultimately, we can call function SdMmcHcGetControllerVersion() only once
at function SdMmcPciHcDriverBindingStart() right after the
SdMmcHcGetMaxCurrent() call. The result can be saved in the 'ControllerVersion'
field of the "SD_MMC_HC_PRIVATE_DATA" structure.

Also, the type of field 'ControllerVersion' in "SD_MMC_HC_PRIVATE_DATA" can be
updated to UINT16 from UINT32.

> 3. I think there is a bigger issue here. During host controller initialization we
> need to setup 64b addressing which happens before we initialize 64b DMA in
> PCI layer. So what you are suggesting may not be that helpful. Also, what we
> are doing right now is that we go through all slots and if controller on any slot
> does not support 64b, we do not enable 64b DMA in PCI layer. What is the
> likelihood of all controllers on an SOC not supporting similar address width?

I am not sure on this one.

All the devices I own seem to only have 1 slot on the controller. And all the
SD host controllers in a system seem to have the same address width support.

But since this driver is a general one, let us assume there will be such a
case here.

> Also, shouldn't we be enabling 64b DMA in PCI layer if any of the controller
> supports 64b addressing? Wouldn't 64b DMA enabled mean 32b is enabled
> by default?

I think your handling in the proposed v6 1/2 patch is good.

Since the Pci IO protocol is managing all the slots on one SD controller, when
64-bit DMA is enabled in the PCI layer, there will be potential issues if one
or more slots only support 32-bit system addressing.

But since this driver is a general one, let us assume there will be such a
case that controllers may have different address width here.

> 4. Patch 5 has been rebased on tip of edk2.

Thanks.

Best Regards,
Hao Wu

> 
> Thanks
> Ashish
> 
> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Wednesday, November 28, 2018 12:25 AM
> To: Ashish Singhal <ashishsingha@nvidia.com>; edk2-devel@lists.01.org
> Subject: RE: [edk2] [PATCH v3 2/2] MdeModulePkg/SdMmcPciHcDxe: Add V4
> 64bit SDMA and ADMA2 support.
> 
> Hello,
> 
> Sorry for the delayed response.
> 
> Apart from inserting the Bugzilla tracker information, several more general
> level
> comments:
> 
> 1. Cannot access the files (content) on SD & eMMC devices
> 
> After applying (rebasing onto the latest codes) your patches, I found that
> though the SD & eMMC devices can be detected, yet the content cannot be
> accessed.
> 
> There are 1 SD card and 1 embedded eMMC device within the system. Under
> Shell, I can see 2 file systems on SD & eMMC devices being detected, but
> when I try to access the files on those file systems by using the 'ls' command,
> it fails, saying "ls: File Not Found - 'FS0:\'". I can confirm that files can be listed
> successfully without applying this patch.
> 
> I also tried the 'dblk' command for display the data via BlockIO protocols
> created on those devices. I found that for SD, the command works fine. But
> for eMMC, the command fails with message "dblk: Read file error - 'BlockIo'".
> 
> For the hardware I own, the controllers are all version 3.0. I tested on both
> IA32 and X64 arch image. The results were the same as described above.
> 
> Unfortunately, I do not have access to boards with SD controller with version
> newer than 3.0. So I am not able to perform those tests on my side for
> Version
> 4.0 or greater SD controllers.
> 
> Do you have any hardware with SD controller of version 3.0? Is it working on
> your side?
> 
> Please let me know if additional information is needed.
> 
> 
> 2. On SdMmcHcGetControllerVersion()
> 
> I found that there is a 'ControllerVersion' version in the data structure
> SD_MMC_HC_PRIVATE_DATA. But currently, this field seems never used.
> 
> I think we can get the controller version information only once and use this
> field to store it. Hence, this new function can be eliminated and also can
> avoid calling it multiple times.
> 
> 
> 3. Setting the SD_MMC_HC_64_ADDR_EN bit in
> SdMmcHcV4Init64BitSupport()
> 
> The setting of the SD_MMC_HC_64_ADDR_EN bit is decided by the
> 'SysBus64V4'
> field in the CAP register. For me, additional dependency on the 64-bit DMA
> support in the PCI layer is needed as well.
> 
> In function SdMmcPciHcDriverBindingStart():
> 
>   //
>   // Enable 64-bit DMA support in the PCI layer if this controller
>   // supports it.
>   //
>   if (Support64BitDma) {
>     Status = PciIo->Attributes (
>                       PciIo,
>                       EfiPciIoAttributeOperationEnable,
>                       EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE,
>                       NULL
>                       );
>     if (EFI_ERROR (Status)) {
>       DEBUG ((DEBUG_WARN, "SdMmcPciHcDriverBindingStart: failed to
> enable 64-bit DMA (%r)\n", Status));
>     }
>   }
> 
> If the above PCI attribute setting fails, the PCI layer will not support the 64-bit
> DMA.
> 
> Hence, I am thinking to introduce a new BOOLEAN field in the
> "SD_MMC_HC_PRIVATE_DATA" data structure (maybe putting the
> 'Support64BitDma'
> into the data structure). If the above PCI operation succeeds, the BOOLEAN
> field will be set to TRUE, otherwise, FALSE.
> 
> And the setting of the SD_MMC_HC_64_ADDR_EN bit should depend on the
> BOOLEAN field as well.
> 
> 
> 4. Please help to rebase the patch upon the latest master branch.
> 
> Some changes within the SdMmcPciHcDxe have been pushed recently. Could
> you help to rebase this patch upon the latest changes. Thanks in advance.
> 
> 
> Also, some inline comments below.
> 
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> > Ashish Singhal
> > Sent: Tuesday, November 20, 2018 4:59 AM
> > To: edk2-devel@lists.01.org
> > Cc: Ashish Singhal
> > Subject: [edk2] [PATCH v3 2/2] MdeModulePkg/SdMmcPciHcDxe: Add V4
> > 64bit SDMA and ADMA2 support.
> >
> > If V4 64 bit address mode is enabled in compatibility register,
> > program controller to enable V4 host mode.
> > Use appropriate ADMA2 descriptors supporting 64 bit addresses.
> > Use appropriate registers for SDMA mode operation.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ashish Singhal <ashishsingha@nvidia.com>
> > ---
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h |   4 +-
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   | 273
> > +++++++++++++++++----
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h   |  28 ++-
> >  3 files changed, 260 insertions(+), 45 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> > index c683600..22795df 100644
> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> > @@ -2,6 +2,7 @@
> >
> >    Provides some data structure definitions used by the SD/MMC host
> > controller driver.
> >
> > +Copyright (c) 2018, NVIDIA CORPORATION. All rights reserved.
> >  Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>  This
> > program and the accompanying materials  are licensed and made
> > available under the terms and conditions of the BSD License @@ -144,7
> > +145,8 @@ typedef struct {
> >    BOOLEAN                             Started;
> >    UINT64                              Timeout;
> >
> > -  SD_MMC_HC_ADMA_DESC_LINE            *AdmaDesc;
> > +  SD_MMC_HC_ADMA_32_DESC_LINE         *Adma32Desc;
> > +  SD_MMC_HC_ADMA_64_DESC_LINE         *Adma64Desc;
> >    EFI_PHYSICAL_ADDRESS                AdmaDescPhy;
> >    VOID                                *AdmaMap;
> >    UINT32                              AdmaPages;
> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > index e506875..9fef3fb 100644
> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > @@ -4,6 +4,7 @@
> >
> >    It would expose EFI_SD_MMC_PASS_THRU_PROTOCOL for upper layer
> use.
> >
> > +  Copyright (c) 2018, NVIDIA CORPORATION. All rights reserved.
> >    Copyright (c) 2015 - 2017, Intel Corporation. All rights reserved.<BR>
> >    This program and the accompanying materials
> >    are licensed and made available under the terms and conditions of
> > the BSD License @@ -418,6 +419,36 @@ SdMmcHcWaitMmioSet (  }
> >
> >  /**
> > +  Get the controller version information from the specified slot.
> > +
> > +  @param[in]  PciIo           The PCI IO protocol instance.
> > +  @param[in]  Slot            The slot number of the SD card to send the
> > command to.
> > +  @param[out] Version         The buffer to store the version information.
> > +
> > +  @retval EFI_SUCCESS         The operation executes successfully.
> > +  @retval Others              The operation fails.
> > +
> > +**/
> > +EFI_STATUS
> > +SdMmcHcGetControllerVersion (
> > +  IN     EFI_PCI_IO_PROTOCOL  *PciIo,
> > +  IN     UINT8                Slot,
> > +     OUT UINT16               *Version
> > +  )
> > +{
> > +  EFI_STATUS                Status;
> > +
> > +  Status = SdMmcHcRwMmio (PciIo, Slot, SD_MMC_HC_CTRL_VER, TRUE,
> > sizeof (UINT16), Version);
> > +  if (EFI_ERROR (Status)) {
> > +    return Status;
> > +  }
> > +
> > +  *Version &= 0xFF;
> > +
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +/**
> >    Software reset the specified SD/MMC host controller and enable all
> > interrupts.
> >
> >    @param[in] Private        A pointer to the SD_MMC_HC_PRIVATE_DATA
> > instance.
> > @@ -776,18 +807,18 @@ SdMmcHcClockSupply (
> >
> >    DEBUG ((DEBUG_INFO, "BaseClkFreq %dMHz Divisor %d ClockFreq
> > %dKhz\n", BaseClkFreq, Divisor, ClockFreq));
> >
> > -  Status = SdMmcHcRwMmio (PciIo, Slot, SD_MMC_HC_CTRL_VER, TRUE,
> > sizeof (ControllerVer), &ControllerVer);
> > +  Status = SdMmcHcGetControllerVersion (PciIo, Slot, &ControllerVer);
> >    if (EFI_ERROR (Status)) {
> >      return Status;
> >    }
> >    //
> >    // Set SDCLK Frequency Select and Internal Clock Enable fields in
> > Clock Control register.
> >    //
> > -  if (((ControllerVer & 0xFF) >= SD_MMC_HC_CTRL_VER_300) &&
> > -      ((ControllerVer & 0xFF) <= SD_MMC_HC_CTRL_VER_420)) {
> > +  if ((ControllerVer >= SD_MMC_HC_CTRL_VER_300) &&
> > +      (ControllerVer <= SD_MMC_HC_CTRL_VER_420)) {
> >      ASSERT (Divisor <= 0x3FF);
> >      ClockCtrl = ((Divisor & 0xFF) << 8) | ((Divisor & 0x300) >> 2);
> > -  } else if (((ControllerVer & 0xFF) == 0) || ((ControllerVer & 0xFF)
> > == 1)) {
> > +  } else if ((ControllerVer == 0) || (ControllerVer == 1)) {
> 
> Could you help to update the above 'else if' statement to use the below
> version definitions?
> 
> SD_MMC_HC_CTRL_VER_100
> SD_MMC_HC_CTRL_VER_200
> 
> >      //
> >      // Only the most significant bit can be used as divisor.
> >      //
> > @@ -935,6 +966,54 @@ SdMmcHcSetBusWidth (  }
> >
> >  /**
> > +  Configure V4 64 bit system address support at initialization.
> > +
> > +  @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.
> > +
> > +  @retval EFI_SUCCESS       The clock is supplied successfully.
> > +
> > +**/
> > +EFI_STATUS
> > +SdMmcHcV4Init64BitSupport (
> > +  IN EFI_PCI_IO_PROTOCOL    *PciIo,
> > +  IN UINT8                  Slot,
> > +  IN SD_MMC_HC_SLOT_CAP     Capability
> > +  )
> > +{
> > +  EFI_STATUS                Status;
> > +  UINT16                    ControllerVer;
> > +  UINT16                    HostCtrl2;
> > +
> > +  //
> > +  // Check if V4 64bit support is available  //  Status =
> > + SdMmcHcGetControllerVersion (PciIo, Slot, &ControllerVer);  if
> > + (EFI_ERROR (Status)) {
> > +    return Status;
> > +  }
> > +
> > +  if (ControllerVer >= SD_MMC_HC_CTRL_VER_400) {
> > +    HostCtrl2 = SD_MMC_HC_V4_EN;
> > +    //
> > +    // Check if V4 64bit support is available
> > +    //
> > +    if (Capability.SysBus64V4 == TRUE) {
> 
> Apart from the additional check needed comments above, could you help to
> refine the above check to:
> 
> if (Capability.SysBus64V4 != 0) {
> 
> > +      HostCtrl2 |= SD_MMC_HC_64_ADDR_EN;
> > +      DEBUG ((DEBUG_INFO, "Enabled V4 64 bit system bus support\n"));
> > +    }
> > +    Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2,
> sizeof
> > (HostCtrl2), &HostCtrl2);
> > +    if (EFI_ERROR (Status)) {
> > +      return Status;
> > +    }
> > +    DEBUG ((DEBUG_INFO, "Enabled V4 64 bit system bus support\n"));
> > + }
> > +
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +/**
> >    Supply SD/MMC card with lowest clock frequency at initialization.
> >
> >    @param[in] PciIo          The PCI IO protocol instance.
> > @@ -1101,6 +1180,11 @@ SdMmcHcInitHost (
> >    PciIo = Private->PciIo;
> >    Capability = Private->Capability[Slot];
> >
> > +  Status = SdMmcHcV4Init64BitSupport (PciIo, Slot, Capability);  if
> > + (EFI_ERROR (Status)) {
> > +    return Status;
> > +  }
> > +
> >    Status = SdMmcHcInitClockFreq (PciIo, Slot, Capability);
> >    if (EFI_ERROR (Status)) {
> >      return Status;
> > @@ -1169,7 +1253,7 @@ SdMmcHcLedOnOff (
> >  /**
> >    Build ADMA descriptor table for transfer.
> >
> > -  Refer to SD Host Controller Simplified spec 3.0 Section 1.13 for details.
> > +  Refer to SD Host Controller Simplified spec 4.2 Section 1.13 for details.
> >
> >    @param[in] Trb            The pointer to the SD_MMC_HC_TRB instance.
> >
> > @@ -1187,49 +1271,69 @@ BuildAdmaDescTable (
> >    UINT64                    Entries;
> >    UINT32                    Index;
> >    UINT64                    Remaining;
> > -  UINT32                    Address;
> > +  UINT64                    Address;
> >    UINTN                     TableSize;
> >    EFI_PCI_IO_PROTOCOL       *PciIo;
> >    EFI_STATUS                Status;
> >    UINTN                     Bytes;
> > +  UINT16                    ControllerVer;
> > +  BOOLEAN                   AddressingMode64 = FALSE;
> > +  UINTN                     DescSize = sizeof
> (SD_MMC_HC_ADMA_32_DESC_LINE);
> > +  VOID                      *AdmaDesc = NULL;
> 
> Please help to separate the variable declaration and initial value assignment.
> 
> Also, please update the type of variable 'DescSize' from UINTN to UINT32.
> The Visual Studio 2015 complier complains for:
> 
> " TableSize = (UINTN)MultU64x32 (Entries, DescSize); ":
> 
> conversion from 'UINTN' to 'UINT32', possible loss of data.
> 
> >
> >    Data    = Trb->DataPhy;
> >    DataLen = Trb->DataLen;
> >    PciIo   = Trb->Private->PciIo;
> > +
> > +  //
> > +  // Detect whether 64bit addressing is supported.
> >    //
> > -  // Only support 32bit ADMA Descriptor Table
> > +  Status = SdMmcHcGetControllerVersion (PciIo, Trb->Slot,
> > + &ControllerVer);  if (EFI_ERROR (Status)) {
> > +    return Status;
> > +  }
> > +  if (ControllerVer >= SD_MMC_HC_CTRL_VER_400) {
> > +    Status = SdMmcHcCheckMmioSet(PciIo, Trb->Slot,
> > SD_MMC_HC_HOST_CTRL2, 0x2,
> > +
> > + SD_MMC_HC_V4_EN|SD_MMC_HC_64_ADDR_EN,
> > SD_MMC_HC_V4_EN|SD_MMC_HC_64_ADDR_EN);
> > +    if (!EFI_ERROR (Status)) {
> > +      AddressingMode64 = TRUE;
> > +      DescSize = sizeof (SD_MMC_HC_ADMA_64_DESC_LINE);
> > +    }
> > +  }
> >    //
> > -  if ((Data >= 0x100000000ul) || ((Data + DataLen) > 0x100000000ul))
> > {
> > +  // Check for valid ranges in 32bit ADMA Descriptor Table  //  if
> > + (AddressingMode64 == FALSE &&
> > +      ((Data >= 0x100000000ul) || ((Data + DataLen) >
> > + 0x100000000ul))) {
> 
> Please help to update the above check to:
> 
>   if ((!AddressingMode64) &&
>       ((Data >= 0x100000000ul) || ((Data + DataLen) > 0x100000000ul))) {
> 
> >      return EFI_INVALID_PARAMETER;
> >    }
> >    //
> >    // Address field shall be set on 32-bit boundary (Lower 2-bit is
> > always set to
> > 0)
> > -  // for 32-bit address descriptor table.
> >    //
> >    if ((Data & (BIT0 | BIT1)) != 0) {
> >      DEBUG ((DEBUG_INFO, "The buffer [0x%x] to construct ADMA desc is
> > not aligned to 4 bytes boundary!\n", Data));
> >    }
> 
> I found that the above check (also the comments) should be updated. For
> 32-bit addressing the address of ADMA desc is 4-bytes aligned; for 64-bit case,
> the requirement is 8-byte.
> 
> Seems the align check for 64-bit addressing is missing here.
> 
> >
> >    Entries   = DivU64x32 ((DataLen + ADMA_MAX_DATA_PER_LINE - 1),
> > ADMA_MAX_DATA_PER_LINE);
> > -  TableSize = (UINTN)MultU64x32 (Entries, sizeof
> > (SD_MMC_HC_ADMA_DESC_LINE));
> > +  TableSize = (UINTN)MultU64x32 (Entries, DescSize);
> >    Trb->AdmaPages = (UINT32)EFI_SIZE_TO_PAGES (TableSize);
> >    Status = PciIo->AllocateBuffer (
> >                      PciIo,
> >                      AllocateAnyPages,
> >                      EfiBootServicesData,
> >                      EFI_SIZE_TO_PAGES (TableSize),
> > -                    (VOID **)&Trb->AdmaDesc,
> > +                    (VOID **)&AdmaDesc,
> >                      0
> >                      );
> >    if (EFI_ERROR (Status)) {
> >      return EFI_OUT_OF_RESOURCES;
> >    }
> > -  ZeroMem (Trb->AdmaDesc, TableSize);
> > +  ZeroMem (AdmaDesc, TableSize);
> >    Bytes  = TableSize;
> >    Status = PciIo->Map (
> >                      PciIo,
> >                      EfiPciIoOperationBusMasterCommonBuffer,
> > -                    Trb->AdmaDesc,
> > +                    AdmaDesc,
> >                      &Bytes,
> >                      &Trb->AdmaDescPhy,
> >                      &Trb->AdmaMap
> > @@ -1242,12 +1346,13 @@ BuildAdmaDescTable (
> >      PciIo->FreeBuffer (
> >               PciIo,
> >               EFI_SIZE_TO_PAGES (TableSize),
> > -             Trb->AdmaDesc
> > +             AdmaDesc
> >               );
> >      return EFI_OUT_OF_RESOURCES;
> >    }
> >
> > -  if ((UINT64)(UINTN)Trb->AdmaDescPhy > 0x100000000ul) {
> > +  if ((AddressingMode64 == FALSE) &&
> > +      (UINT64)(UINTN)Trb->AdmaDescPhy > 0x100000000ul) {
> 
> Please update the above check to:
> 
>   if ((!AddressingMode64) &&
>       (UINT64)(UINTN)Trb->AdmaDescPhy > 0x100000000ul) {
> 
> >      //
> >      // The ADMA doesn't support 64bit addressing.
> >      //
> > @@ -1258,25 +1363,49 @@ BuildAdmaDescTable (
> >      PciIo->FreeBuffer (
> >        PciIo,
> >        EFI_SIZE_TO_PAGES (TableSize),
> > -      Trb->AdmaDesc
> > +      AdmaDesc
> >      );
> >      return EFI_DEVICE_ERROR;
> >    }
> >
> >    Remaining = DataLen;
> > -  Address   = (UINT32)Data;
> > +  Address   = Data;
> > +  if (AddressingMode64 == FALSE) {
> 
> Please help to update the above check to:
> 
>   if (!AddressingMode64) {
> 
> > +    Trb->Adma32Desc = AdmaDesc;
> > +    Trb->Adma64Desc = NULL;
> > +  } else {
> > +    Trb->Adma64Desc = AdmaDesc;
> > +    Trb->Adma32Desc = NULL;
> > +  }
> >    for (Index = 0; Index < Entries; Index++) {
> > -    if (Remaining <= ADMA_MAX_DATA_PER_LINE) {
> > -      Trb->AdmaDesc[Index].Valid = 1;
> > -      Trb->AdmaDesc[Index].Act   = 2;
> > -      Trb->AdmaDesc[Index].Length  = (UINT16)Remaining;
> > -      Trb->AdmaDesc[Index].Address = Address;
> > -      break;
> > +    if (AddressingMode64 == FALSE) {
> 
> Please help to update the above check to:
> 
>   if (!AddressingMode64) {
> 
> > +      if (Remaining < ADMA_MAX_DATA_PER_LINE) {
> > +        Trb->Adma32Desc[Index].Valid = 1;
> > +        Trb->Adma32Desc[Index].Act   = 2;
> > +        Trb->Adma32Desc[Index].Length  = (UINT16)Remaining;
> > +        Trb->Adma32Desc[Index].Address = (UINT32)Address;
> > +        break;
> > +      } else {
> > +        Trb->Adma32Desc[Index].Valid = 1;
> > +        Trb->Adma32Desc[Index].Act   = 2;
> > +        Trb->Adma32Desc[Index].Length  = 0;
> > +        Trb->Adma32Desc[Index].Address = (UINT32)Address;
> > +      }
> >      } else {
> > -      Trb->AdmaDesc[Index].Valid = 1;
> > -      Trb->AdmaDesc[Index].Act   = 2;
> > -      Trb->AdmaDesc[Index].Length  = 0;
> > -      Trb->AdmaDesc[Index].Address = Address;
> > +      if (Remaining < ADMA_MAX_DATA_PER_LINE) {
> > +        Trb->Adma64Desc[Index].Valid = 1;
> > +        Trb->Adma64Desc[Index].Act   = 2;
> > +        Trb->Adma64Desc[Index].Length  = (UINT16)Remaining;
> > +        Trb->Adma64Desc[Index].LowerAddress = (UINT32)(Address &
> > MAX_UINT32);
> > +        Trb->Adma64Desc[Index].UpperAddress = (UINT32)(Address>>32);
> > +        break;
> > +      } else {
> > +        Trb->Adma64Desc[Index].Valid = 1;
> > +        Trb->Adma64Desc[Index].Act   = 2;
> > +        Trb->Adma64Desc[Index].Length  = 0;
> > +        Trb->Adma64Desc[Index].LowerAddress = (UINT32)(Address &
> > MAX_UINT32);
> 
> I think the above 2 " & MAX_UINT32" can be dropped.
> 
> 
> > +        Trb->Adma64Desc[Index].UpperAddress = (UINT32)(Address>>32);
> > +      }
> >      }
> >
> >      Remaining -= ADMA_MAX_DATA_PER_LINE; @@ -1286,7 +1415,7 @@
> > BuildAdmaDescTable (
> >    //
> >    // Set the last descriptor line as end of descriptor table
> >    //
> > -  Trb->AdmaDesc[Index].End = 1;
> > +  AddressingMode64 ? (Trb->Adma64Desc[Index].End = 1) : (Trb-
> > >Adma32Desc[Index].End = 1);
> >    return EFI_SUCCESS;
> >  }
> >
> > @@ -1430,11 +1559,18 @@ SdMmcFreeTrb (
> >        Trb->AdmaMap
> >      );
> >    }
> > -  if (Trb->AdmaDesc != NULL) {
> > +  if (Trb->Adma32Desc != NULL) {
> >      PciIo->FreeBuffer (
> >        PciIo,
> >        Trb->AdmaPages,
> > -      Trb->AdmaDesc
> > +      Trb->Adma32Desc
> > +    );
> > +  }
> > +  if (Trb->Adma64Desc != NULL) {
> > +    PciIo->FreeBuffer (
> > +      PciIo,
> > +      Trb->AdmaPages,
> > +      Trb->Adma64Desc
> >      );
> >    }
> >    if (Trb->DataMap != NULL) {
> > @@ -1574,12 +1710,14 @@ SdMmcExecTrb (
> >    UINT16                              Cmd;
> >    UINT16                              IntStatus;
> >    UINT32                              Argument;
> > -  UINT16                              BlkCount;
> > +  UINT32                              BlkCount;
> >    UINT16                              BlkSize;
> >    UINT16                              TransMode;
> >    UINT8                               HostCtrl1;
> > -  UINT32                              SdmaAddr;
> > +  UINT64                              SdmaAddr;
> >    UINT64                              AdmaAddr;
> > +  UINT16                              ControllerVer;
> > +  BOOLEAN                             AddressingMode64 = FALSE;
> 
> Please help to separate the variable declaration and initial value assignment.
> 
> >
> >    Packet = Trb->Packet;
> >    PciIo  = Trb->Private->PciIo;
> > @@ -1612,13 +1750,33 @@ SdMmcExecTrb (
> >
> >    SdMmcHcLedOnOff (PciIo, Trb->Slot, TRUE);
> >
> > +  Status = SdMmcHcGetControllerVersion (PciIo, Trb->Slot,
> > + &ControllerVer);  if (EFI_ERROR (Status)) {
> > +    return Status;
> > +  }
> > +  if (ControllerVer >= SD_MMC_HC_CTRL_VER_400) {
> > +    Status = SdMmcHcCheckMmioSet(PciIo, Trb->Slot,
> > SD_MMC_HC_HOST_CTRL2, 0x2,
> > +
> > + SD_MMC_HC_V4_EN|SD_MMC_HC_64_ADDR_EN,
> > SD_MMC_HC_V4_EN|SD_MMC_HC_64_ADDR_EN);
> > +    if (!EFI_ERROR (Status)) {
> > +      AddressingMode64 = TRUE;
> > +    }
> > +  }
> > +
> >    if (Trb->Mode == SdMmcSdmaMode) {
> > -    if ((UINT64)(UINTN)Trb->DataPhy >= 0x100000000ul) {
> > +    if ((AddressingMode64 == FALSE) &&
> > +        ((UINT64)(UINTN)Trb->DataPhy >= 0x100000000ul)) {
> 
> Please help to update the above check to:
> 
>     if ((!AddressingMode64) &&
>         ((UINT64)(UINTN)Trb->DataPhy >= 0x100000000ul)) {
> 
> >        return EFI_INVALID_PARAMETER;
> >      }
> >
> > -    SdmaAddr = (UINT32)(UINTN)Trb->DataPhy;
> > -    Status   = SdMmcHcRwMmio (PciIo, Trb->Slot,
> SD_MMC_HC_SDMA_ADDR,
> > FALSE, sizeof (SdmaAddr), &SdmaAddr);
> > +    SdmaAddr = (UINT64)(UINTN)Trb->DataPhy;
> > +
> > +    if (ControllerVer >= SD_MMC_HC_CTRL_VER_400) {
> > +      Status = SdMmcHcRwMmio (PciIo, Trb->Slot,
> > SD_MMC_HC_ADMA_SYS_ADDR, FALSE, sizeof (UINT64), &SdmaAddr);
> > +    }
> > +    else {
> 
> Minor coding style comment for the above line:
> 
> } else {
> 
> > +      Status = SdMmcHcRwMmio (PciIo, Trb->Slot,
> > SD_MMC_HC_SDMA_ADDR, FALSE, sizeof (UINT32), &SdmaAddr);
> > +    }
> > +
> >      if (EFI_ERROR (Status)) {
> >        return Status;
> >      }
> > @@ -1648,9 +1806,14 @@ SdMmcExecTrb (
> >      //
> >      // Calcuate Block Count.
> >      //
> > -    BlkCount = (UINT16)(Trb->DataLen / Trb->BlockSize);
> > +    BlkCount = (Trb->DataLen / Trb->BlockSize);  }
> 
> For the below 'Block Count' settings, I think it would be better to add
> comments that quote the SD Host Controller Spec to mention that the 32-bit
> block count support for all operations in SD mode was introduced at Version
> 4.10.
> 
> > +  if (ControllerVer >= SD_MMC_HC_CTRL_VER_410) {
> > +    Status = SdMmcHcRwMmio (PciIo, Trb->Slot,
> SD_MMC_HC_SDMA_ADDR,
> > FALSE, sizeof (UINT32), &BlkCount);
> > +  }
> > +  else {
> 
> Minor coding style comment for the above line:
> 
> } else {
> 
> > +    Status = SdMmcHcRwMmio (PciIo, Trb->Slot,
> SD_MMC_HC_BLK_COUNT,
> > FALSE, sizeof (UINT16), &BlkCount);
> >    }
> > -  Status   = SdMmcHcRwMmio (PciIo, Trb->Slot, SD_MMC_HC_BLK_COUNT,
> > FALSE, sizeof (BlkCount), &BlkCount);
> >    if (EFI_ERROR (Status)) {
> >      return Status;
> >    }
> > @@ -1746,10 +1909,11 @@ SdMmcCheckTrbResult (
> >    EFI_SD_MMC_PASS_THRU_COMMAND_PACKET *Packet;
> >    UINT16                              IntStatus;
> >    UINT32                              Response[4];
> > -  UINT32                              SdmaAddr;
> > +  UINT64                              SdmaAddr;
> >    UINT8                               Index;
> >    UINT8                               SwReset;
> >    UINT32                              PioLength;
> > +  UINT16                              ControllerVer;
> >
> >    SwReset = 0;
> >    Packet  = Trb->Packet;
> > @@ -1870,19 +2034,42 @@ SdMmcCheckTrbResult (
> >      //
> >      // Update SDMA Address register.
> >      //
> > -    SdmaAddr = SD_MMC_SDMA_ROUND_UP ((UINT32)(UINTN)Trb-
> > >DataPhy, SD_MMC_SDMA_BOUNDARY);
> > -    Status   = SdMmcHcRwMmio (
> > +    SdmaAddr = SD_MMC_SDMA_ROUND_UP ((UINTN)Trb->DataPhy,
> > SD_MMC_SDMA_BOUNDARY);
> > +
> > +    Status = SdMmcHcGetControllerVersion (
> > +               Private->PciIo,
> > +               Trb->Slot,
> > +               &ControllerVer
> > +               );
> > +    if (EFI_ERROR (Status)) {
> > +      return Status;
> > +    }
> > +
> > +    if (ControllerVer >= SD_MMC_HC_CTRL_VER_400) {
> > +      Status = SdMmcHcRwMmio (
> >                   Private->PciIo,
> >                   Trb->Slot,
> > -                 SD_MMC_HC_SDMA_ADDR,
> > +                 SD_MMC_HC_ADMA_SYS_ADDR,
> >                   FALSE,
> > -                 sizeof (UINT32),
> > +                 sizeof (UINT64),
> >                   &SdmaAddr
> >                   );
> > +    }
> > +    else {
> > +        Status = SdMmcHcRwMmio (
> > +                   Private->PciIo,
> > +                   Trb->Slot,
> > +                   SD_MMC_HC_SDMA_ADDR,
> > +                   FALSE,
> > +                   sizeof (UINT32),
> > +                   &SdmaAddr
> > +                   );
> > +    }
> 
> Minor coding style comments for the above line:
> 
> 1. } else {
> 2. Please help to refine the space indent of the above SdMmcHcRwMmio()
> call
> 
> 
> Best Regards,
> Hao Wu
> 
> > +
> >      if (EFI_ERROR (Status)) {
> >        goto Done;
> >      }
> > -    Trb->DataPhy = (UINT32)(UINTN)SdmaAddr;
> > +    Trb->DataPhy = (UINT64)(UINTN)SdmaAddr;
> >    }
> >
> >    if ((Packet->SdMmcCmdBlk->CommandType !=
> SdMmcCommandTypeAdtc) &&
> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> > index cc138fc..a6234f1 100644
> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> > @@ -2,6 +2,7 @@
> >
> >    Provides some data structure definitions used by the SD/MMC host
> > controller driver.
> >
> > +Copyright (c) 2018, NVIDIA CORPORATION. All rights reserved.
> >  Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>  This
> > program and the accompanying materials  are licensed and made
> > available under the terms and conditions of the BSD License @@ -78,6
> > +79,9 @@ typedef enum {  //
> >  #define ADMA_MAX_DATA_PER_LINE     0x10000
> >
> > +//
> > +// ADMA descriptor for 32b addressing.
> > +//
> >  typedef struct {
> >    UINT32 Valid:1;
> >    UINT32 End:1;
> > @@ -87,7 +91,23 @@ typedef struct {
> >    UINT32 Reserved1:10;
> >    UINT32 Length:16;
> >    UINT32 Address;
> > -} SD_MMC_HC_ADMA_DESC_LINE;
> > +} SD_MMC_HC_ADMA_32_DESC_LINE;
> > +
> > +//
> > +// ADMA descriptor for 64b addressing.
> > +//
> > +typedef struct {
> > +  UINT32 Valid:1;
> > +  UINT32 End:1;
> > +  UINT32 Int:1;
> > +  UINT32 Reserved:1;
> > +  UINT32 Act:2;
> > +  UINT32 Reserved1:10;
> > +  UINT32 Length:16;
> > +  UINT32 LowerAddress;
> > +  UINT32 UpperAddress;
> > +  UINT32 Reserved2;
> > +} SD_MMC_HC_ADMA_64_DESC_LINE;
> >
> >  #define SD_MMC_SDMA_BOUNDARY          512 * 1024
> >  #define SD_MMC_SDMA_ROUND_UP(x, n)    (((x) + n) & ~(n - 1))
> > @@ -145,6 +165,12 @@ typedef struct {
> >  #define SD_MMC_HC_CTRL_VER_410      0x04
> >  #define SD_MMC_HC_CTRL_VER_420      0x05
> >
> > +//
> > +// SD Host controller V4 Support
> > +//
> > +#define SD_MMC_HC_V4_EN             BIT12
> > +#define SD_MMC_HC_64_ADDR_EN        BIT13
> > +
> >  /**
> >    Dump the content of SD/MMC host controller's Capability Register.
> >
> > --
> > 2.7.4
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> -----------------------------------------------------------------------------------
> This email message is for the sole use of the intended recipient(s) and may
> contain
> confidential information.  Any unauthorized review, use, disclosure or
> distribution
> is prohibited.  If you are not the intended recipient, please contact the
> sender by
> reply email and destroy all copies of the original message.
> -----------------------------------------------------------------------------------
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH v3 1/2] MdeModulePkg/SdMmcPciHcDxe: Declare V4 64 bit address capability

[Wu, Hao A]

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Ashish Singhal
> Sent: Tuesday, November 20, 2018 4:59 AM
> To: edk2-devel@lists.01.org
> Cc: Ashish Singhal
> Subject: [edk2] [PATCH v3 1/2] MdeModulePkg/SdMmcPciHcDxe: Declare V4
> 64 bit address capability
> 
> Add capability declaration for V4.x 64 bit system address support.
> This would be used for host controllers working in version 4. Enable
> 64 bit DMA support in PCI layer if V3 or V4 64 bit support is
> enabled in host capability register.
> 
> The usage of this new field does not need a guard for version check as
> spec for previous SDMMC versions defines this field as reserved with
> default value of 0.

Hello,

Sorry for the delayed response.

I have filed a Tianocore Feature Requests Bugzilla tracker for the 64-bit
SDMA/ADMA support for Sd/MMC host controller driver:

https://bugzilla.tianocore.org/show_bug.cgi?id=1359

Could you help to include this Bugzilla tracker message in your 2 proposed
patches?

> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ashish Singhal <ashishsingha@nvidia.com>
> ---
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c |  4 ++--
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   |  3 ++-
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h   | 10 +++++-----
>  3 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> index bf9869d..1c18ea4 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> @@ -617,7 +617,6 @@ SdMmcPciHcDriverBindingStart (
>      }
>    }
> 
> -  Support64BitDma = TRUE;

Please keep the above line, otherwise GCC compiler (I am testing with GCC4.9)
seems not happy with it.

>    for (Slot = FirstBar; Slot < (FirstBar + SlotNum); Slot++) {
>      Private->Slot[Slot].Enable = TRUE;
> 
> @@ -638,7 +637,8 @@ SdMmcPciHcDriverBindingStart (
>      }
>      DumpCapabilityReg (Slot, &Private->Capability[Slot]);
> 
> -    Support64BitDma &= Private->Capability[Slot].SysBus64;
> +    Support64BitDma = (Private->Capability[Slot].SysBus64V3 |
> +                       Private->Capability[Slot].SysBus64V4);

For the above statement, how about:

    Support64BitDma &= (Private->Capability[Slot].SysBus64V3 |
                        Private->Capability[Slot].SysBus64V4);

The Visual Studio 2015 complier build fails for your current proposed change.


Best Regards,
Hao Wu

> 
>      Status = SdMmcHcGetMaxCurrent (PciIo, Slot, &Private-
> >MaxCurrent[Slot]);
>      if (EFI_ERROR (Status)) {
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> index bedc968..e506875 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> @@ -45,7 +45,8 @@ DumpCapabilityReg (
>    DEBUG ((DEBUG_INFO, "   Voltage 3.3       %a\n", Capability->Voltage33 ?
> "TRUE" : "FALSE"));
>    DEBUG ((DEBUG_INFO, "   Voltage 3.0       %a\n", Capability->Voltage30 ?
> "TRUE" : "FALSE"));
>    DEBUG ((DEBUG_INFO, "   Voltage 1.8       %a\n", Capability->Voltage18 ?
> "TRUE" : "FALSE"));
> -  DEBUG ((DEBUG_INFO, "   64-bit Sys Bus    %a\n", Capability->SysBus64 ?
> "TRUE" : "FALSE"));
> +  DEBUG ((DEBUG_INFO, "   V4 64-bit Sys Bus %a\n", Capability-
> >SysBus64V4 ? "TRUE" : "FALSE"));
> +  DEBUG ((DEBUG_INFO, "   V3 64-bit Sys Bus %a\n", Capability-
> >SysBus64V3 ? "TRUE" : "FALSE"));
>    DEBUG ((DEBUG_INFO, "   Async Interrupt   %a\n", Capability->AsyncInt ?
> "TRUE" : "FALSE"));
>    DEBUG ((DEBUG_INFO, "   SlotType          "));
>    if (Capability->SlotType == 0x00) {
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> index 7e3f588..cc138fc 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> @@ -114,24 +114,24 @@ typedef struct {
>    UINT32   Voltage33:1;       // bit 24
>    UINT32   Voltage30:1;       // bit 25
>    UINT32   Voltage18:1;       // bit 26
> -  UINT32   Reserved3:1;       // bit 27
> -  UINT32   SysBus64:1;        // bit 28
> +  UINT32   SysBus64V4:1;      // bit 27
> +  UINT32   SysBus64V3:1;      // bit 28
>    UINT32   AsyncInt:1;        // bit 29
>    UINT32   SlotType:2;        // bit 30:31
>    UINT32   Sdr50:1;           // bit 32
>    UINT32   Sdr104:1;          // bit 33
>    UINT32   Ddr50:1;           // bit 34
> -  UINT32   Reserved4:1;       // bit 35
> +  UINT32   Reserved3:1;       // bit 35
>    UINT32   DriverTypeA:1;     // bit 36
>    UINT32   DriverTypeC:1;     // bit 37
>    UINT32   DriverTypeD:1;     // bit 38
>    UINT32   DriverType4:1;     // bit 39
>    UINT32   TimerCount:4;      // bit 40:43
> -  UINT32   Reserved5:1;       // bit 44
> +  UINT32   Reserved4:1;       // bit 44
>    UINT32   TuningSDR50:1;     // bit 45
>    UINT32   RetuningMod:2;     // bit 46:47
>    UINT32   ClkMultiplier:8;   // bit 48:55
> -  UINT32   Reserved6:7;       // bit 56:62
> +  UINT32   Reserved5:7;       // bit 56:62
>    UINT32   Hs400:1;           // bit 63
>  } SD_MMC_HC_SLOT_CAP;
> 
> --
> 2.7.4
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH v3 1/2] MdeModulePkg/SdMmcPciHcDxe: Declare V4 64 bit address capability

[Ashish Singhal]

Separate V4 1/2 patch has been submitted with changes made as per feedback.

Thanks
Ashish

-----Original Message-----
From: Wu, Hao A <hao.a.wu@intel.com> 
Sent: Wednesday, November 28, 2018 12:25 AM
To: Ashish Singhal <ashishsingha@nvidia.com>; edk2-devel@lists.01.org
Subject: RE: [edk2] [PATCH v3 1/2] MdeModulePkg/SdMmcPciHcDxe: Declare V4 64 bit address capability

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 
> Ashish Singhal
> Sent: Tuesday, November 20, 2018 4:59 AM
> To: edk2-devel@lists.01.org
> Cc: Ashish Singhal
> Subject: [edk2] [PATCH v3 1/2] MdeModulePkg/SdMmcPciHcDxe: Declare V4
> 64 bit address capability
> 
> Add capability declaration for V4.x 64 bit system address support.
> This would be used for host controllers working in version 4. Enable
> 64 bit DMA support in PCI layer if V3 or V4 64 bit support is enabled 
> in host capability register.
> 
> The usage of this new field does not need a guard for version check as 
> spec for previous SDMMC versions defines this field as reserved with 
> default value of 0.

Hello,

Sorry for the delayed response.

I have filed a Tianocore Feature Requests Bugzilla tracker for the 64-bit SDMA/ADMA support for Sd/MMC host controller driver:

https://bugzilla.tianocore.org/show_bug.cgi?id=1359

Could you help to include this Bugzilla tracker message in your 2 proposed patches?

> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ashish Singhal <ashishsingha@nvidia.com>
> ---
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c |  4 ++--
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   |  3 ++-
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h   | 10 +++++-----
>  3 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> index bf9869d..1c18ea4 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> @@ -617,7 +617,6 @@ SdMmcPciHcDriverBindingStart (
>      }
>    }
> 
> -  Support64BitDma = TRUE;

Please keep the above line, otherwise GCC compiler (I am testing with GCC4.9) seems not happy with it.

>    for (Slot = FirstBar; Slot < (FirstBar + SlotNum); Slot++) {
>      Private->Slot[Slot].Enable = TRUE;
> 
> @@ -638,7 +637,8 @@ SdMmcPciHcDriverBindingStart (
>      }
>      DumpCapabilityReg (Slot, &Private->Capability[Slot]);
> 
> -    Support64BitDma &= Private->Capability[Slot].SysBus64;
> +    Support64BitDma = (Private->Capability[Slot].SysBus64V3 |
> +                       Private->Capability[Slot].SysBus64V4);

For the above statement, how about:

    Support64BitDma &= (Private->Capability[Slot].SysBus64V3 |
                        Private->Capability[Slot].SysBus64V4);

The Visual Studio 2015 complier build fails for your current proposed change.


Best Regards,
Hao Wu

> 
>      Status = SdMmcHcGetMaxCurrent (PciIo, Slot, &Private-
> >MaxCurrent[Slot]);
>      if (EFI_ERROR (Status)) {
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> index bedc968..e506875 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> @@ -45,7 +45,8 @@ DumpCapabilityReg (
>    DEBUG ((DEBUG_INFO, "   Voltage 3.3       %a\n", Capability->Voltage33 ?
> "TRUE" : "FALSE"));
>    DEBUG ((DEBUG_INFO, "   Voltage 3.0       %a\n", Capability->Voltage30 ?
> "TRUE" : "FALSE"));
>    DEBUG ((DEBUG_INFO, "   Voltage 1.8       %a\n", Capability->Voltage18 ?
> "TRUE" : "FALSE"));
> -  DEBUG ((DEBUG_INFO, "   64-bit Sys Bus    %a\n", Capability->SysBus64 ?
> "TRUE" : "FALSE"));
> +  DEBUG ((DEBUG_INFO, "   V4 64-bit Sys Bus %a\n", Capability-
> >SysBus64V4 ? "TRUE" : "FALSE"));
> +  DEBUG ((DEBUG_INFO, "   V3 64-bit Sys Bus %a\n", Capability-
> >SysBus64V3 ? "TRUE" : "FALSE"));
>    DEBUG ((DEBUG_INFO, "   Async Interrupt   %a\n", Capability->AsyncInt ?
> "TRUE" : "FALSE"));
>    DEBUG ((DEBUG_INFO, "   SlotType          "));
>    if (Capability->SlotType == 0x00) { diff --git 
> a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> index 7e3f588..cc138fc 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> @@ -114,24 +114,24 @@ typedef struct {
>    UINT32   Voltage33:1;       // bit 24
>    UINT32   Voltage30:1;       // bit 25
>    UINT32   Voltage18:1;       // bit 26
> -  UINT32   Reserved3:1;       // bit 27
> -  UINT32   SysBus64:1;        // bit 28
> +  UINT32   SysBus64V4:1;      // bit 27
> +  UINT32   SysBus64V3:1;      // bit 28
>    UINT32   AsyncInt:1;        // bit 29
>    UINT32   SlotType:2;        // bit 30:31
>    UINT32   Sdr50:1;           // bit 32
>    UINT32   Sdr104:1;          // bit 33
>    UINT32   Ddr50:1;           // bit 34
> -  UINT32   Reserved4:1;       // bit 35
> +  UINT32   Reserved3:1;       // bit 35
>    UINT32   DriverTypeA:1;     // bit 36
>    UINT32   DriverTypeC:1;     // bit 37
>    UINT32   DriverTypeD:1;     // bit 38
>    UINT32   DriverType4:1;     // bit 39
>    UINT32   TimerCount:4;      // bit 40:43
> -  UINT32   Reserved5:1;       // bit 44
> +  UINT32   Reserved4:1;       // bit 44
>    UINT32   TuningSDR50:1;     // bit 45
>    UINT32   RetuningMod:2;     // bit 46:47
>    UINT32   ClkMultiplier:8;   // bit 48:55
> -  UINT32   Reserved6:7;       // bit 56:62
> +  UINT32   Reserved5:7;       // bit 56:62
>    UINT32   Hs400:1;           // bit 63
>  } SD_MMC_HC_SLOT_CAP;
> 
> --
> 2.7.4
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------