Eugene,
We do not have support for V4 64b DMA right now but it can be added later if
needed. I am trying to understand the reason behind changing the check from AND
to OR. Right now, we disable 64b DMA Support in PCI if the controller cannot
support 64b DMA in V3 as well as V4. If either of these support 64b DMA, we do
not disable it. In the code, we set Support64BitDma to TRUE by default and
change it to FALSE only if any of the controller does not support it in V3 as
well as V4. If all controllers support it in V3 or V4 we keep 64b DMA support
enabled.
//
// 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));
}
}
Thanks
Ashish
From: Cohen, Eugene <eug...@hp.com>
Sent: Thursday, February 28, 2019 12:56 PM
To: Ashish Singhal <ashishsin...@nvidia.com>; Wu, Hao A <hao.a...@intel.com>;
edk2-devel@lists.01.org
Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit
systems
Ashish,
* With my change, if any of the controller did not support 64b DMA in V3 as
well as V4 capability, we are not enabling it in PCI layer.
The logic is:
if (Private->Capability[Slot].SysBus64V3 == 0 &&
Private->Capability[Slot].SysBus64V4 == 0) {
Support64BitDma = FALSE;
}
which means that for a SDHC v3 controller you have SysBus64V3=1 and
SysBus64V4=0 the FALSE assignment is never done - this is not correct. Perhaps
you intended an OR instead of an AND? Either way changing this to an || or
using my patch is the same logical result because a V3 controller will use
32-bit DMA and V4 controller will use 64-bit DMA (a V4 controller should have
the V3 bit set). I really saw no reason to be checking the V3 bit since the
driver was unprepared to do V3 64-bit DMA operations anyways.
Eugene
From: Ashish Singhal <ashishsin...@nvidia.com<mailto:ashishsin...@nvidia.com>>
Sent: Thursday, February 28, 2019 12:15 PM
To: Cohen, Eugene <eug...@hp.com<mailto:eug...@hp.com>>; Wu, Hao A
<hao.a...@intel.com<mailto:hao.a...@intel.com>>;
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit
systems
Hello Eugene,
My patch enabled support for SDHC 4.0 and above in general including support
for 64b ADMA descriptor. The check for V3 capability for 64b DMA was already
there and similar check was implemented for V4 capability for 64b DMA. Earlier,
if any of the V3 controller did not support 64b DMA, we were not enabling it in
PCI layer. With my change, if any of the controller did not support 64b DMA in
V3 as well as V4 capability, we are not enabling it in PCI layer.
This check in my opinion is better because we only disable 64b DMA PCI support
when both V3 and V4 have it disabled.
Thanks
Ashish
-----Original Message-----
From: Cohen, Eugene <eug...@hp.com<mailto:eug...@hp.com>>
Sent: Thursday, February 28, 2019 4:24 AM
To: Wu, Hao A <hao.a...@intel.com<mailto:hao.a...@intel.com>>;
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Cc: Ashish Singhal <ashishsin...@nvidia.com<mailto:ashishsin...@nvidia.com>>
Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit
systems
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...@intel.com<mailto:hao.a...@intel.com>>
Sent: Wednesday, February 27, 2019 8:25 PM
To: Cohen, Eugene <eug...@hp.com<mailto:eug...@hp.com>>;
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Cc: Ashish Singhal <ashishsin...@nvidia.com<mailto:ashishsin...@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-boun...@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...@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eugene Cohen <mailto:eug...@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
-----------------------------------------------------------------------------------
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
