Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems

["Cohen, Eugene" <eugene@hp.com>]

Hao,

> I remember the commit b5547b9ce97e80c3127682a2a5d4b9bd14af353e from Ashish
> only handles the controllers with version greater or equal to 4.00.

Right - that commit added support for SDHC 4.0 and above.  The original driver supported SDHC 3.0 albeit only with SDMA and 32-bit ADMA support.

With that commit two descriptor types are supported the 32-bit ADMA descriptor (SD_MMC_HC_ADMA_32_DESC_LINE which is 64-bits in size) and the V4 64-bit ADMA descriptor (SD_MMC_HC_ADMA_64_DESC_LINE which is 128-bits in size).

However the commit mistakenly added a check for the V3 capability for 64-bit DMA and used it to set the PCI DUAL_ADDRESS_CYCLE attributre which then does not the 32-bit compatible bounce buffer mechanism.  Later, when we attempt an ADMA data transfer we hit an ASSERT because the PCI DMA subsystem is not using bounce buffers to provide 32-bit DMA compatible memory.  So the patch I submitted simply removes the unnecessary check of the V3 64-bit DMA capability check so the PCI DUAL_ADDRESS_CYCLE attribute is not set allowing 32-bit DMA to succeed on these platforms.

> And the ADMA2 (96-bit Descriptor) mode for V3 controllers is selected by
> setting the 'DMA Select' filed in the Host Control 1 Register to 11b. But
> the currently behavior of the driver is setting the field to 10b, which I
> think will not switch to the ADMA2 (96-bit Descriptor) mode for V3.

Correct, right now for a V3 controller only 32-bit DMA is supported.  An enhancement for V3 64-bit ADMA would improve performance on controllers that support that mode by eliminating the bounce buffer and associated memory copies.  I think we should file a BZ for SD HCI V3 64-bit ADMA support - if you agree I would be happy to do that.

I should point out that we have done extensive testing of this change on our host controller.

Thanks,

Eugene

---

From: Wu, Hao A <hao.a.wu@intel.com> 
Sent: Wednesday, February 27, 2019 8:25 PM
To: Cohen, Eugene <eugene@hp.com>; edk2-devel@lists.01.org
Cc: Ashish Singhal <ashishsingha@nvidia.com>
Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems

Loop Ashish in.
Some comments below.

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Cohen, Eugene
> Sent: Wednesday, February 27, 2019 6:59 PM
> To: mailto:edk2-devel@lists.01.org; Wu, Hao A
> Subject: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC
> v3 64-bit systems
> 
> The SdMmcPciHcDriverBindingStart function was checking two
> different capability bits in determining whether 64-bit DMA
> modes were supported, one mode is defined in the SDHC version
> 3 specification (using 96-bit descriptors) and another is
> defined in the SDHC version 4 specification (using 128-bit
> descriptors). Since the currently implementation of 64-bit
> ADMA2 only supports the SDHC version 4 implementation it is
> incorrect to check the V3 64-bit capability bit since this
> will activate V4 ADMA2 on V3 controllers.

I remember the commit b5547b9ce97e80c3127682a2a5d4b9bd14af353e from Ashish
only handles the controllers with version greater or equal to 4.00.

And the ADMA2 (96-bit Descriptor) mode for V3 controllers is selected by
setting the 'DMA Select' filed in the Host Control 1 Register to 11b. But
the currently behavior of the driver is setting the field to 10b, which I
think will not switch to the ADMA2 (96-bit Descriptor) mode for V3.

Maybe there is something I miss here. Could you help to provide some more
detail on the issue you met? Thanks.

Best Regards,
Hao Wu

> 
> Cc: Hao Wu <mailto:hao.a.wu@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eugene Cohen <mailto:eugene@hp.com>
> ---
> MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> index b474f8d..5bc91c5 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> @@ -666,8 +666,7 @@ SdMmcPciHcDriverBindingStart (
> // If any of the slots does not support 64b system bus
> // do not enable 64b DMA in the PCI layer.
> //
> - if (Private->Capability[Slot].SysBus64V3 == 0 &&
> - Private->Capability[Slot].SysBus64V4 == 0) {
> + if (Private->Capability[Slot].SysBus64V4 == 0) {
> Support64BitDma = FALSE;
> }
> 
> --
> 2.7.4
> _______________________________________________
> edk2-devel mailing list
> mailto:edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel