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

[Ashish Singhal <ashishsingha@nvidia.com>]

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.
-----------------------------------------------------------------------------------