Re: [edk2] [PATCH v3 1/2] MdeModulePkg/SdMmcPciHcDxe: Declare V4 64 bit address capability
Separate V4 1/2 patch has been submitted with changes made as per feedback. Thanks Ashish -Original Message- From: Wu, Hao A Sent: Wednesday, November 28, 2018 12:25 AM To: Ashish Singhal ; 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-boun...@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 > --- > 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 >
Re: [edk2] [PATCH v3 1/2] MdeModulePkg/SdMmcPciHcDxe: Declare V4 64 bit address capability
> -Original Message- > From: edk2-devel [mailto:edk2-devel-boun...@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 > --- > 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 >UIN