* [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems @ 2019-02-27 10:58 Cohen, Eugene 2019-02-28 3:24 ` Wu, Hao A 0 siblings, 1 reply; 24+ messages in thread From: Cohen, Eugene @ 2019-02-27 10:58 UTC (permalink / raw) To: edk2-devel@lists.01.org, Hao Wu 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. Cc: Hao Wu <hao.a.wu@intel.com> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Eugene Cohen <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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems 2019-02-27 10:58 [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems Cohen, Eugene @ 2019-02-28 3:24 ` Wu, Hao A 2019-02-28 11:23 ` Cohen, Eugene 0 siblings, 1 reply; 24+ messages in thread From: Wu, Hao A @ 2019-02-28 3:24 UTC (permalink / raw) To: Cohen, Eugene, edk2-devel@lists.01.org; +Cc: Ashish Singhal 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: 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 <hao.a.wu@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eugene Cohen <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 > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems 2019-02-28 3:24 ` Wu, Hao A @ 2019-02-28 11:23 ` Cohen, Eugene 2019-02-28 19:15 ` Ashish Singhal 0 siblings, 1 reply; 24+ messages in thread From: Cohen, Eugene @ 2019-02-28 11:23 UTC (permalink / raw) To: Wu, Hao A, edk2-devel@lists.01.org; +Cc: Ashish Singhal 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems 2019-02-28 11:23 ` Cohen, Eugene @ 2019-02-28 19:15 ` Ashish Singhal 2019-02-28 19:56 ` Cohen, Eugene 0 siblings, 1 reply; 24+ messages in thread From: Ashish Singhal @ 2019-02-28 19:15 UTC (permalink / raw) To: Cohen, Eugene, Wu, Hao A, edk2-devel@lists.01.org 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 <eugene@hp.com> Sent: Thursday, February 28, 2019 4:24 AM To: Wu, Hao A <hao.a.wu@intel.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 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 ----------------------------------------------------------------------------------- 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. ----------------------------------------------------------------------------------- ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems 2019-02-28 19:15 ` Ashish Singhal @ 2019-02-28 19:56 ` Cohen, Eugene 2019-02-28 21:27 ` Ashish Singhal 0 siblings, 1 reply; 24+ messages in thread From: Cohen, Eugene @ 2019-02-28 19:56 UTC (permalink / raw) To: Ashish Singhal, Wu, Hao A, edk2-devel@lists.01.org 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 <ashishsingha@nvidia.com> Sent: Thursday, February 28, 2019 12:15 PM To: Cohen, Eugene <eugene@hp.com>; Wu, Hao A <hao.a.wu@intel.com>; 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 <eugene@hp.com<mailto:eugene@hp.com>> Sent: Thursday, February 28, 2019 4:24 AM To: Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> Cc: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@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.wu@intel.com<mailto:hao.a.wu@intel.com>> Sent: Wednesday, February 27, 2019 8:25 PM To: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> Cc: Ashish Singhal <ashishsingha@nvidia.com<mailto: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<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. ----------------------------------------------------------------------------------- ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems 2019-02-28 19:56 ` Cohen, Eugene @ 2019-02-28 21:27 ` Ashish Singhal 2019-02-28 21:58 ` Cohen, Eugene 0 siblings, 1 reply; 24+ messages in thread From: Ashish Singhal @ 2019-02-28 21:27 UTC (permalink / raw) To: Cohen, Eugene, Wu, Hao A, edk2-devel@lists.01.org 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 <eugene@hp.com> Sent: Thursday, February 28, 2019 12:56 PM To: Ashish Singhal <ashishsingha@nvidia.com>; Wu, Hao A <hao.a.wu@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 <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>> Sent: Thursday, February 28, 2019 12:15 PM To: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@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 <eugene@hp.com<mailto:eugene@hp.com>> Sent: Thursday, February 28, 2019 4:24 AM To: Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> Cc: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@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.wu@intel.com<mailto:hao.a.wu@intel.com>> Sent: Wednesday, February 27, 2019 8:25 PM To: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> Cc: Ashish Singhal <ashishsingha@nvidia.com<mailto: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 ----------------------------------------------------------------------------------- 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. ----------------------------------------------------------------------------------- ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems 2019-02-28 21:27 ` Ashish Singhal @ 2019-02-28 21:58 ` Cohen, Eugene 2019-02-28 22:20 ` Ashish Singhal 0 siblings, 1 reply; 24+ messages in thread From: Cohen, Eugene @ 2019-02-28 21:58 UTC (permalink / raw) To: Ashish Singhal, Wu, Hao A, edk2-devel@lists.01.org, Ard Biesheuvel Ashish, Ø 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. That is precisely the problem. An SDHC v3 controller might support 64b DMA in V3 but not in V4 mode. The current code will leave 64b DMA support enabled resulting in the issuing of the PCI DUAL_ADDRESS_CYCLE attribute ( see https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c#L738 ) which then causes buffers to be allocated that cannot be DMAed. For reference look at this snippet of the NonDiscoverablePciDeviceIo driver: https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c#L622 and you can see that bounce buffering will only occur if DUAL_ADDRESS_CYCLE is clear. So since we do not have V3 64b DMA (96-bit descriptor) support in place we must not allow the DUAL_ADDRESS_CYCLE attribute to be set or we will fail with this check: https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c#L1426 I've added Ard who updated the driver with DUAL_ADDRESS_CYCLE support. Eugene From: Ashish Singhal <ashishsingha@nvidia.com> Sent: Thursday, February 28, 2019 2:28 PM To: Cohen, Eugene <eugene@hp.com>; Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems 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 <eugene@hp.com<mailto:eugene@hp.com>> Sent: Thursday, February 28, 2019 12:56 PM To: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@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 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 <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>> Sent: Thursday, February 28, 2019 12:15 PM To: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@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 <eugene@hp.com<mailto:eugene@hp.com>> Sent: Thursday, February 28, 2019 4:24 AM To: Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> Cc: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@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.wu@intel.com<mailto:hao.a.wu@intel.com>> Sent: Wednesday, February 27, 2019 8:25 PM To: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> Cc: Ashish Singhal <ashishsingha@nvidia.com<mailto: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<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. ----------------------------------------------------------------------------------- ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems 2019-02-28 21:58 ` Cohen, Eugene @ 2019-02-28 22:20 ` Ashish Singhal 2019-02-28 22:40 ` Cohen, Eugene 0 siblings, 1 reply; 24+ messages in thread From: Ashish Singhal @ 2019-02-28 22:20 UTC (permalink / raw) To: Cohen, Eugene, Wu, Hao A, edk2-devel@lists.01.org, Ard Biesheuvel Eugene, Thanks for the explanation. The problem is valid and is more clear to me now. How about we do this: Instead of: if (Private->Capability[Slot].SysBus64V3 == 0 && Private->Capability[Slot].SysBus64V4 == 0) { Support64BitDma = FALSE; } What do you think about: if ((Private->ControllerVersion[Slot] == SD_MMC_HC_CTRL_VER_300 && Private->Capability[Slot].SysBus64V3 == 0) || (Private->ControllerVersion[Slot] >= SD_MMC_HC_CTRL_VER_400 && Private->Capability[Slot].SysBus64V4 == 0)) { Support64BitDma = FALSE; } With this, we would be checking 64b capability based on the version we are using and not for something we may not be using despite of being advertised in the controller. Thanks Ashish From: Cohen, Eugene <eugene@hp.com> Sent: Thursday, February 28, 2019 2:59 PM To: Ashish Singhal <ashishsingha@nvidia.com>; Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Ard Biesheuvel <ard.biesheuvel@linaro.org> Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems Ashish, * 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. That is precisely the problem. An SDHC v3 controller might support 64b DMA in V3 but not in V4 mode. The current code will leave 64b DMA support enabled resulting in the issuing of the PCI DUAL_ADDRESS_CYCLE attribute ( see https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c#L738 ) which then causes buffers to be allocated that cannot be DMAed. For reference look at this snippet of the NonDiscoverablePciDeviceIo driver: https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c#L622 and you can see that bounce buffering will only occur if DUAL_ADDRESS_CYCLE is clear. So since we do not have V3 64b DMA (96-bit descriptor) support in place we must not allow the DUAL_ADDRESS_CYCLE attribute to be set or we will fail with this check: https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c#L1426 I've added Ard who updated the driver with DUAL_ADDRESS_CYCLE support. Eugene From: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>> Sent: Thursday, February 28, 2019 2:28 PM To: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@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 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 <eugene@hp.com<mailto:eugene@hp.com>> Sent: Thursday, February 28, 2019 12:56 PM To: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@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 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 <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>> Sent: Thursday, February 28, 2019 12:15 PM To: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@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 <eugene@hp.com<mailto:eugene@hp.com>> Sent: Thursday, February 28, 2019 4:24 AM To: Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> Cc: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@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.wu@intel.com<mailto:hao.a.wu@intel.com>> Sent: Wednesday, February 27, 2019 8:25 PM To: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> Cc: Ashish Singhal <ashishsingha@nvidia.com<mailto: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 ----------------------------------------------------------------------------------- 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. ----------------------------------------------------------------------------------- ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems 2019-02-28 22:20 ` Ashish Singhal @ 2019-02-28 22:40 ` Cohen, Eugene 2019-02-28 23:58 ` Ashish Singhal 0 siblings, 1 reply; 24+ messages in thread From: Cohen, Eugene @ 2019-02-28 22:40 UTC (permalink / raw) To: Ashish Singhal, Wu, Hao A, edk2-devel@lists.01.org, Ard Biesheuvel Ashish, I think that code will still fail for our use case. We are version 3 with 64-bit support so Private->Capability[Slot].SysBus64V3 == 0 will evaluate to FALSE. Since we are V3 Private->ControllerVersion[Slot] >= SD_MMC_HC_CTRL_VER_400 will also evaluate to FALSE. Therefore Support64BitDma will still be TRUE resulting in DUAL_ADDRESS_CYCLE being set which disables bounce buffering. Since no code is in place to do V3 64b DMA we will still hit the same problem, specifically namely that buffers that are not DMAable will be allocated and we will still fail the check here<https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c#L1426>. Until such time that V3 64b DMA support is in place I believe only the V4 bit should be evaluated. Eugene From: Ashish Singhal <ashishsingha@nvidia.com> Sent: Thursday, February 28, 2019 3:21 PM To: Cohen, Eugene <eugene@hp.com>; Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Ard Biesheuvel <ard.biesheuvel@linaro.org> Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems Eugene, Thanks for the explanation. The problem is valid and is more clear to me now. How about we do this: Instead of: if (Private->Capability[Slot].SysBus64V3 == 0 && Private->Capability[Slot].SysBus64V4 == 0) { Support64BitDma = FALSE; } What do you think about: if ((Private->ControllerVersion[Slot] == SD_MMC_HC_CTRL_VER_300 && Private->Capability[Slot].SysBus64V3 == 0) || (Private->ControllerVersion[Slot] >= SD_MMC_HC_CTRL_VER_400 && Private->Capability[Slot].SysBus64V4 == 0)) { Support64BitDma = FALSE; } With this, we would be checking 64b capability based on the version we are using and not for something we may not be using despite of being advertised in the controller. Thanks Ashish From: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>> Sent: Thursday, February 28, 2019 2:59 PM To: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>> Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems Ashish, Ø 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. That is precisely the problem. An SDHC v3 controller might support 64b DMA in V3 but not in V4 mode. The current code will leave 64b DMA support enabled resulting in the issuing of the PCI DUAL_ADDRESS_CYCLE attribute ( see https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c#L738<https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c#L738> ) which then causes buffers to be allocated that cannot be DMAed. For reference look at this snippet of the NonDiscoverablePciDeviceIo driver: https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c#L622<https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c#L622> and you can see that bounce buffering will only occur if DUAL_ADDRESS_CYCLE is clear. So since we do not have V3 64b DMA (96-bit descriptor) support in place we must not allow the DUAL_ADDRESS_CYCLE attribute to be set or we will fail with this check: https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c#L1426<https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c#L1426> I've added Ard who updated the driver with DUAL_ADDRESS_CYCLE support. Eugene From: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>> Sent: Thursday, February 28, 2019 2:28 PM To: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@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 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 <eugene@hp.com<mailto:eugene@hp.com>> Sent: Thursday, February 28, 2019 12:56 PM To: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@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 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 <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>> Sent: Thursday, February 28, 2019 12:15 PM To: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@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 <eugene@hp.com<mailto:eugene@hp.com>> Sent: Thursday, February 28, 2019 4:24 AM To: Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> Cc: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@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.wu@intel.com<mailto:hao.a.wu@intel.com>> Sent: Wednesday, February 27, 2019 8:25 PM To: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> Cc: Ashish Singhal <ashishsingha@nvidia.com<mailto: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<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. ----------------------------------------------------------------------------------- ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems 2019-02-28 22:40 ` Cohen, Eugene @ 2019-02-28 23:58 ` Ashish Singhal 2019-03-01 0:10 ` Cohen, Eugene 0 siblings, 1 reply; 24+ messages in thread From: Ashish Singhal @ 2019-02-28 23:58 UTC (permalink / raw) To: Cohen, Eugene, Wu, Hao A, edk2-devel@lists.01.org, Ard Biesheuvel Eugene, Thanks for pointing that out. This is a use case we are not covering as of now. I see two options to this: 1. Do not enable 64b DMA support in PCI based on V3 as driver does not support V3 64b ADMA. This is a quick fix. 2. Enable V3 64b ADMA support to add the missing feature. This will take maybe a day or two and can be done. Thanks Ashish From: Cohen, Eugene <eugene@hp.com> Sent: Thursday, February 28, 2019 3:40 PM To: Ashish Singhal <ashishsingha@nvidia.com>; Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Ard Biesheuvel <ard.biesheuvel@linaro.org> Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems Ashish, I think that code will still fail for our use case. We are version 3 with 64-bit support so Private->Capability[Slot].SysBus64V3 == 0 will evaluate to FALSE. Since we are V3 Private->ControllerVersion[Slot] >= SD_MMC_HC_CTRL_VER_400 will also evaluate to FALSE. Therefore Support64BitDma will still be TRUE resulting in DUAL_ADDRESS_CYCLE being set which disables bounce buffering. Since no code is in place to do V3 64b DMA we will still hit the same problem, specifically namely that buffers that are not DMAable will be allocated and we will still fail the check here<https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c#L1426>. Until such time that V3 64b DMA support is in place I believe only the V4 bit should be evaluated. Eugene From: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>> Sent: Thursday, February 28, 2019 3:21 PM To: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>> Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems Eugene, Thanks for the explanation. The problem is valid and is more clear to me now. How about we do this: Instead of: if (Private->Capability[Slot].SysBus64V3 == 0 && Private->Capability[Slot].SysBus64V4 == 0) { Support64BitDma = FALSE; } What do you think about: if ((Private->ControllerVersion[Slot] == SD_MMC_HC_CTRL_VER_300 && Private->Capability[Slot].SysBus64V3 == 0) || (Private->ControllerVersion[Slot] >= SD_MMC_HC_CTRL_VER_400 && Private->Capability[Slot].SysBus64V4 == 0)) { Support64BitDma = FALSE; } With this, we would be checking 64b capability based on the version we are using and not for something we may not be using despite of being advertised in the controller. Thanks Ashish From: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>> Sent: Thursday, February 28, 2019 2:59 PM To: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>> Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems Ashish, * 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. That is precisely the problem. An SDHC v3 controller might support 64b DMA in V3 but not in V4 mode. The current code will leave 64b DMA support enabled resulting in the issuing of the PCI DUAL_ADDRESS_CYCLE attribute ( see https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c#L738 ) which then causes buffers to be allocated that cannot be DMAed. For reference look at this snippet of the NonDiscoverablePciDeviceIo driver: https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c#L622 and you can see that bounce buffering will only occur if DUAL_ADDRESS_CYCLE is clear. So since we do not have V3 64b DMA (96-bit descriptor) support in place we must not allow the DUAL_ADDRESS_CYCLE attribute to be set or we will fail with this check: https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c#L1426 I've added Ard who updated the driver with DUAL_ADDRESS_CYCLE support. Eugene From: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>> Sent: Thursday, February 28, 2019 2:28 PM To: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@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 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 <eugene@hp.com<mailto:eugene@hp.com>> Sent: Thursday, February 28, 2019 12:56 PM To: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@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 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 <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>> Sent: Thursday, February 28, 2019 12:15 PM To: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@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 <eugene@hp.com<mailto:eugene@hp.com>> Sent: Thursday, February 28, 2019 4:24 AM To: Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> Cc: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@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.wu@intel.com<mailto:hao.a.wu@intel.com>> Sent: Wednesday, February 27, 2019 8:25 PM To: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> Cc: Ashish Singhal <ashishsingha@nvidia.com<mailto: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 ----------------------------------------------------------------------------------- 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. ----------------------------------------------------------------------------------- ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems 2019-02-28 23:58 ` Ashish Singhal @ 2019-03-01 0:10 ` Cohen, Eugene 2019-03-01 0:19 ` Ashish Singhal 0 siblings, 1 reply; 24+ messages in thread From: Cohen, Eugene @ 2019-03-01 0:10 UTC (permalink / raw) To: Ashish Singhal, Wu, Hao A, edk2-devel@lists.01.org, Ard Biesheuvel Ashish, Agreed - #2 would be better in the long run since this will have better performance by eliminating the bounce buffering. My original intent in submitting the patch was to fix the logic the current implementation with minimal impact. Thanks, Eugene From: Ashish Singhal <ashishsingha@nvidia.com> Sent: Thursday, February 28, 2019 4:58 PM To: Cohen, Eugene <eugene@hp.com>; Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Ard Biesheuvel <ard.biesheuvel@linaro.org> Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems Eugene, Thanks for pointing that out. This is a use case we are not covering as of now. I see two options to this: 1. Do not enable 64b DMA support in PCI based on V3 as driver does not support V3 64b ADMA. This is a quick fix. 2. Enable V3 64b ADMA support to add the missing feature. This will take maybe a day or two and can be done. Thanks Ashish From: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>> Sent: Thursday, February 28, 2019 3:40 PM To: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>> Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems Ashish, I think that code will still fail for our use case. We are version 3 with 64-bit support so Private->Capability[Slot].SysBus64V3 == 0 will evaluate to FALSE. Since we are V3 Private->ControllerVersion[Slot] >= SD_MMC_HC_CTRL_VER_400 will also evaluate to FALSE. Therefore Support64BitDma will still be TRUE resulting in DUAL_ADDRESS_CYCLE being set which disables bounce buffering. Since no code is in place to do V3 64b DMA we will still hit the same problem, specifically namely that buffers that are not DMAable will be allocated and we will still fail the check here<https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c#L1426>. Until such time that V3 64b DMA support is in place I believe only the V4 bit should be evaluated. Eugene From: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>> Sent: Thursday, February 28, 2019 3:21 PM To: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>> Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems Eugene, Thanks for the explanation. The problem is valid and is more clear to me now. How about we do this: Instead of: if (Private->Capability[Slot].SysBus64V3 == 0 && Private->Capability[Slot].SysBus64V4 == 0) { Support64BitDma = FALSE; } What do you think about: if ((Private->ControllerVersion[Slot] == SD_MMC_HC_CTRL_VER_300 && Private->Capability[Slot].SysBus64V3 == 0) || (Private->ControllerVersion[Slot] >= SD_MMC_HC_CTRL_VER_400 && Private->Capability[Slot].SysBus64V4 == 0)) { Support64BitDma = FALSE; } With this, we would be checking 64b capability based on the version we are using and not for something we may not be using despite of being advertised in the controller. Thanks Ashish From: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>> Sent: Thursday, February 28, 2019 2:59 PM To: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>> Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems Ashish, Ø 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. That is precisely the problem. An SDHC v3 controller might support 64b DMA in V3 but not in V4 mode. The current code will leave 64b DMA support enabled resulting in the issuing of the PCI DUAL_ADDRESS_CYCLE attribute ( see https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c#L738<https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c#L738> ) which then causes buffers to be allocated that cannot be DMAed. For reference look at this snippet of the NonDiscoverablePciDeviceIo driver: https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c#L622<https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c#L622> and you can see that bounce buffering will only occur if DUAL_ADDRESS_CYCLE is clear. So since we do not have V3 64b DMA (96-bit descriptor) support in place we must not allow the DUAL_ADDRESS_CYCLE attribute to be set or we will fail with this check: https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c#L1426<https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c#L1426> I've added Ard who updated the driver with DUAL_ADDRESS_CYCLE support. Eugene From: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>> Sent: Thursday, February 28, 2019 2:28 PM To: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@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 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 <eugene@hp.com<mailto:eugene@hp.com>> Sent: Thursday, February 28, 2019 12:56 PM To: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@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 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 <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>> Sent: Thursday, February 28, 2019 12:15 PM To: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@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 <eugene@hp.com<mailto:eugene@hp.com>> Sent: Thursday, February 28, 2019 4:24 AM To: Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> Cc: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@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.wu@intel.com<mailto:hao.a.wu@intel.com>> Sent: Wednesday, February 27, 2019 8:25 PM To: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> Cc: Ashish Singhal <ashishsingha@nvidia.com<mailto: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<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. ----------------------------------------------------------------------------------- ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems 2019-03-01 0:10 ` Cohen, Eugene @ 2019-03-01 0:19 ` Ashish Singhal 2019-03-01 10:32 ` Ard Biesheuvel 2019-03-01 11:02 ` Cohen, Eugene 0 siblings, 2 replies; 24+ messages in thread From: Ashish Singhal @ 2019-03-01 0:19 UTC (permalink / raw) To: Cohen, Eugene, Wu, Hao A, edk2-devel@lists.01.org, Ard Biesheuvel Eugene, Small question. Did the issue appear after the V4 patch went in? Looking at the code before that patch, we were enabling 64b dma in pci based on capability register already despite of driver supporting only 32b dma. Thanks Ashish Get Outlook for iOS<https://aka.ms/o0ukef> ________________________________ From: Cohen, Eugene <eugene@hp.com> Sent: Thursday, February 28, 2019 5:11 PM To: Ashish Singhal; Wu, Hao A; edk2-devel@lists.01.org; Ard Biesheuvel Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems Ashish, Agreed - #2 would be better in the long run since this will have better performance by eliminating the bounce buffering. My original intent in submitting the patch was to fix the logic the current implementation with minimal impact. Thanks, Eugene From: Ashish Singhal <ashishsingha@nvidia.com> Sent: Thursday, February 28, 2019 4:58 PM To: Cohen, Eugene <eugene@hp.com>; Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Ard Biesheuvel <ard.biesheuvel@linaro.org> Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems Eugene, Thanks for pointing that out. This is a use case we are not covering as of now. I see two options to this: 1. Do not enable 64b DMA support in PCI based on V3 as driver does not support V3 64b ADMA. This is a quick fix. 2. Enable V3 64b ADMA support to add the missing feature. This will take maybe a day or two and can be done. Thanks Ashish From: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>> Sent: Thursday, February 28, 2019 3:40 PM To: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>> Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems Ashish, I think that code will still fail for our use case. We are version 3 with 64-bit support so Private->Capability[Slot].SysBus64V3 == 0 will evaluate to FALSE. Since we are V3 Private->ControllerVersion[Slot] >= SD_MMC_HC_CTRL_VER_400 will also evaluate to FALSE. Therefore Support64BitDma will still be TRUE resulting in DUAL_ADDRESS_CYCLE being set which disables bounce buffering. Since no code is in place to do V3 64b DMA we will still hit the same problem, specifically namely that buffers that are not DMAable will be allocated and we will still fail the check here<https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c#L1426>. Until such time that V3 64b DMA support is in place I believe only the V4 bit should be evaluated. Eugene From: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>> Sent: Thursday, February 28, 2019 3:21 PM To: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>> Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems Eugene, Thanks for the explanation. The problem is valid and is more clear to me now. How about we do this: Instead of: if (Private->Capability[Slot].SysBus64V3 == 0 && Private->Capability[Slot].SysBus64V4 == 0) { Support64BitDma = FALSE; } What do you think about: if ((Private->ControllerVersion[Slot] == SD_MMC_HC_CTRL_VER_300 && Private->Capability[Slot].SysBus64V3 == 0) || (Private->ControllerVersion[Slot] >= SD_MMC_HC_CTRL_VER_400 && Private->Capability[Slot].SysBus64V4 == 0)) { Support64BitDma = FALSE; } With this, we would be checking 64b capability based on the version we are using and not for something we may not be using despite of being advertised in the controller. Thanks Ashish From: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>> Sent: Thursday, February 28, 2019 2:59 PM To: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>> Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems Ashish, Ø 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. That is precisely the problem. An SDHC v3 controller might support 64b DMA in V3 but not in V4 mode. The current code will leave 64b DMA support enabled resulting in the issuing of the PCI DUAL_ADDRESS_CYCLE attribute ( see https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c#L738 ) which then causes buffers to be allocated that cannot be DMAed. For reference look at this snippet of the NonDiscoverablePciDeviceIo driver: https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c#L622 and you can see that bounce buffering will only occur if DUAL_ADDRESS_CYCLE is clear. So since we do not have V3 64b DMA (96-bit descriptor) support in place we must not allow the DUAL_ADDRESS_CYCLE attribute to be set or we will fail with this check: https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c#L1426 I've added Ard who updated the driver with DUAL_ADDRESS_CYCLE support. Eugene From: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>> Sent: Thursday, February 28, 2019 2:28 PM To: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@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 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 <eugene@hp.com<mailto:eugene@hp.com>> Sent: Thursday, February 28, 2019 12:56 PM To: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@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 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 <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>> Sent: Thursday, February 28, 2019 12:15 PM To: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@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 <eugene@hp.com<mailto:eugene@hp.com>> Sent: Thursday, February 28, 2019 4:24 AM To: Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> Cc: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@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.wu@intel.com<mailto:hao.a.wu@intel.com>> Sent: Wednesday, February 27, 2019 8:25 PM To: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> Cc: Ashish Singhal <ashishsingha@nvidia.com<mailto: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 ----------------------------------------------------------------------------------- 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. ----------------------------------------------------------------------------------- ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems 2019-03-01 0:19 ` Ashish Singhal @ 2019-03-01 10:32 ` Ard Biesheuvel 2019-03-01 10:34 ` Ard Biesheuvel 2019-03-01 10:54 ` Cohen, Eugene 2019-03-01 11:02 ` Cohen, Eugene 1 sibling, 2 replies; 24+ messages in thread From: Ard Biesheuvel @ 2019-03-01 10:32 UTC (permalink / raw) To: Ashish Singhal; +Cc: Cohen, Eugene, Wu, Hao A, edk2-devel@lists.01.org On Fri, 1 Mar 2019 at 01:19, Ashish Singhal <ashishsingha@nvidia.com> wrote: > > Eugene, > > Small question. Did the issue appear after the V4 patch went in? Looking at the code before that patch, we were enabling 64b dma in pci based on capability register already despite of driver supporting only 32b dma. > I think this may have been an oversight on my part when I originally added the DUAL_ADDRESS_CYCLE handling. The following commit added EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE to the host bridge driver commit e58a71d9c50ba641b5ab19f5ce2cbf772187de4d Author: Ard Biesheuvel <ard.biesheuvel@linaro.org> Date: Mon Sep 5 09:55:16 2016 +0100 MdeModulePkg/PciHostBridgeDxe: restrict 64-bit DMA to devices that support it Currently, the EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute is completely ignored by the PCI host bridge driver, which means that, on an implementation that supports DMA above 4 GB, allocations above 4 GB may be provided to devices that have not expressed support for it. and the SDHCI driver was fixed accordingly in Author: Ard Biesheuvel <ard.biesheuvel@linaro.org> Date: Mon Sep 5 09:51:48 2016 +0100 MdeModulePkg/SdMmcPciHcDxe: enable 64-bit PCI DMA PCI controller drivers must set the EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute if the controller supports 64-bit DMA addressing. So before these changes, we were in the exact same situation, but since PC platforms never enable DMA above 4 GB in the first place, nobody ever noticed until we started running this code on arm64 platforms that have no 32-bit addressable DRAM to begin with. The obvious conclusion is that the driver should not set the EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute if the device does not support it, or, which seems to be our case, if the driver does not implement the 64-bit DMA mode that the driver does support. However, since there are platforms for which bounce buffering is not an option (since there is no 32-bit addressable memory to bounce to), this is not just a performance optimization, and so it would be useful to fix the code so it can drive all 64-bit DMA capable hardware. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems 2019-03-01 10:32 ` Ard Biesheuvel @ 2019-03-01 10:34 ` Ard Biesheuvel 2019-03-01 10:54 ` Cohen, Eugene 1 sibling, 0 replies; 24+ messages in thread From: Ard Biesheuvel @ 2019-03-01 10:34 UTC (permalink / raw) To: Ashish Singhal; +Cc: Cohen, Eugene, Wu, Hao A, edk2-devel@lists.01.org On Fri, 1 Mar 2019 at 11:32, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > On Fri, 1 Mar 2019 at 01:19, Ashish Singhal <ashishsingha@nvidia.com> wrote: > > > > Eugene, > > > > Small question. Did the issue appear after the V4 patch went in? Looking at the code before that patch, we were enabling 64b dma in pci based on capability register already despite of driver supporting only 32b dma. > > > > I think this may have been an oversight on my part when I originally > added the DUAL_ADDRESS_CYCLE handling. > > The following commit added EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE to the > host bridge driver > > commit e58a71d9c50ba641b5ab19f5ce2cbf772187de4d > Author: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Date: Mon Sep 5 09:55:16 2016 +0100 > > MdeModulePkg/PciHostBridgeDxe: restrict 64-bit DMA to devices that > support it > > Currently, the EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute is completely > ignored by the PCI host bridge driver, which means that, on an > implementation > that supports DMA above 4 GB, allocations above 4 GB may be provided to > devices that have not expressed support for it. > > and the SDHCI driver was fixed accordingly in > > Author: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Date: Mon Sep 5 09:51:48 2016 +0100 > > MdeModulePkg/SdMmcPciHcDxe: enable 64-bit PCI DMA > > PCI controller drivers must set the EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE > attribute if the controller supports 64-bit DMA addressing. > > So before these changes, we were in the exact same situation, but > since PC platforms never enable DMA above 4 GB in the first place, > nobody ever noticed until we started running this code on arm64 > platforms that have no 32-bit addressable DRAM to begin with. > > The obvious conclusion is that the driver should not set the > EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute if the device does > not support it, or, which seems to be our case, if the driver does not > implement the 64-bit DMA mode that the driver does support. Correction: that the *device* does support. > However, > since there are platforms for which bounce buffering is not an option > (since there is no 32-bit addressable memory to bounce to), this is > not just a performance optimization, and so it would be useful to fix > the code so it can drive all 64-bit DMA capable hardware. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems 2019-03-01 10:32 ` Ard Biesheuvel 2019-03-01 10:34 ` Ard Biesheuvel @ 2019-03-01 10:54 ` Cohen, Eugene 2019-03-01 11:38 ` Ard Biesheuvel 1 sibling, 1 reply; 24+ messages in thread From: Cohen, Eugene @ 2019-03-01 10:54 UTC (permalink / raw) To: Ard Biesheuvel, Ashish Singhal Cc: Wu, Hao A, edk2-devel@lists.01.org, Kim, Sangwoo (김상우 SW1Lab.) Ard, > So before these changes, we were in the exact same situation, but since PC > platforms never enable DMA above 4 GB in the first place, nobody ever > noticed until we started running this code on arm64 platforms that have no > 32-bit addressable DRAM to begin with. Interesting - I did not realize that there were designs that were crazy enough to have no addressable DRAM below 4G. > The obvious conclusion is that the driver should not set the > EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute if the device does > not support it, or, which seems to be our case, if the driver does not > implement the 64-bit DMA mode that the driver does support. However, > since there are platforms for which bounce buffering is not an option (since > there is no 32-bit addressable memory to bounce to), this is not just a > performance optimization, and so it would be useful to fix the code so it can > drive all 64-bit DMA capable hardware. Okay, that's a great reason - let's get V3 64b ADMA2 in! Any objection to committing the original patch in the short term? Thanks, Eugene ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems 2019-03-01 10:54 ` Cohen, Eugene @ 2019-03-01 11:38 ` Ard Biesheuvel 2019-03-01 12:31 ` Ashish Singhal 2019-03-01 15:25 ` Ashish Singhal 0 siblings, 2 replies; 24+ messages in thread From: Ard Biesheuvel @ 2019-03-01 11:38 UTC (permalink / raw) To: Cohen, Eugene Cc: Ashish Singhal, Wu, Hao A, edk2-devel@lists.01.org, Kim, Sangwoo (김상우 SW1Lab.) On Fri, 1 Mar 2019 at 11:54, Cohen, Eugene <eugene@hp.com> wrote: > > Ard, > > > So before these changes, we were in the exact same situation, but since PC > > platforms never enable DMA above 4 GB in the first place, nobody ever > > noticed until we started running this code on arm64 platforms that have no > > 32-bit addressable DRAM to begin with. > > Interesting - I did not realize that there were designs that were crazy enough to have no addressable DRAM below 4G. > You must be new here :-) But seriously, it does make sense for an implementation to, say, put all peripherals, PCIe resource windows etc in the bottom half and all DRAM in the top half of a 40-bit address space, which is how the AMD Seattle SoC ended with its system memory at address 0x80_0000_0000. Note that on this platform, we can still use 32-bit DMA if we want to with the help of the SMMUs, but we haven't wired those up in UEFI (and the generic host bridge driver did not have the IOMMU hooks at the time) > > The obvious conclusion is that the driver should not set the > > EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute if the device does > > not support it, or, which seems to be our case, if the driver does not > > implement the 64-bit DMA mode that the driver does support. However, > > since there are platforms for which bounce buffering is not an option (since > > there is no 32-bit addressable memory to bounce to), this is not just a > > performance optimization, and so it would be useful to fix the code so it can > > drive all 64-bit DMA capable hardware. > > Okay, that's a great reason - let's get V3 64b ADMA2 in! > > Any objection to committing the original patch in the short term? > not at all Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems 2019-03-01 11:38 ` Ard Biesheuvel @ 2019-03-01 12:31 ` Ashish Singhal 2019-03-01 18:31 ` Ashish Singhal 2019-03-01 15:25 ` Ashish Singhal 1 sibling, 1 reply; 24+ messages in thread From: Ashish Singhal @ 2019-03-01 12:31 UTC (permalink / raw) To: Ard Biesheuvel, Cohen, Eugene Cc: Wu, Hao A, edk2-devel@lists.01.org, Kim, Sangwoo (김상우 SW1Lab.) I've already started refactoring the driver to support V3 64b ADMA2. Meanwhile, I'm OK with the proposed patch. Thanks Ashish Get Outlook for iOS<https://aka.ms/o0ukef> ________________________________ From: Ard Biesheuvel <ard.biesheuvel@linaro.org> Sent: Friday, March 1, 2019 4:40 AM To: Cohen, Eugene Cc: Ashish Singhal; Wu, Hao A; edk2-devel@lists.01.org; Kim, Sangwoo (김상우 SW1Lab.) Subject: Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems On Fri, 1 Mar 2019 at 11:54, Cohen, Eugene <eugene@hp.com> wrote: > > Ard, > > > So before these changes, we were in the exact same situation, but since PC > > platforms never enable DMA above 4 GB in the first place, nobody ever > > noticed until we started running this code on arm64 platforms that have no > > 32-bit addressable DRAM to begin with. > > Interesting - I did not realize that there were designs that were crazy enough to have no addressable DRAM below 4G. > You must be new here :-) But seriously, it does make sense for an implementation to, say, put all peripherals, PCIe resource windows etc in the bottom half and all DRAM in the top half of a 40-bit address space, which is how the AMD Seattle SoC ended with its system memory at address 0x80_0000_0000. Note that on this platform, we can still use 32-bit DMA if we want to with the help of the SMMUs, but we haven't wired those up in UEFI (and the generic host bridge driver did not have the IOMMU hooks at the time) > > The obvious conclusion is that the driver should not set the > > EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute if the device does > > not support it, or, which seems to be our case, if the driver does not > > implement the 64-bit DMA mode that the driver does support. However, > > since there are platforms for which bounce buffering is not an option (since > > there is no 32-bit addressable memory to bounce to), this is not just a > > performance optimization, and so it would be useful to fix the code so it can > > drive all 64-bit DMA capable hardware. > > Okay, that's a great reason - let's get V3 64b ADMA2 in! > > Any objection to committing the original patch in the short term? > not at all Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> ----------------------------------------------------------------------------------- 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. ----------------------------------------------------------------------------------- ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems 2019-03-01 12:31 ` Ashish Singhal @ 2019-03-01 18:31 ` Ashish Singhal 0 siblings, 0 replies; 24+ messages in thread From: Ashish Singhal @ 2019-03-01 18:31 UTC (permalink / raw) To: Ard Biesheuvel, Cohen, Eugene Cc: Wu, Hao A, edk2-devel@lists.01.org, Kim, Sangwoo (김상우 SW1Lab.) Eugene, I have submitted a patch for supporting 64b DMA on V3 controllers. Can you please validate it at your end as well? Thanks Ashish From: Ashish Singhal <ashishsingha@nvidia.com> Sent: Friday, March 1, 2019 5:31 AM To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Cohen, Eugene <eugene@hp.com> Cc: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Kim, Sangwoo (김상우 SW1Lab.) <sangwoo.kim@hp.com> Subject: Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems I've already started refactoring the driver to support V3 64b ADMA2. Meanwhile, I'm OK with the proposed patch. Thanks Ashish Get Outlook for iOS<https://aka.ms/o0ukef> ________________________________ From: Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>> Sent: Friday, March 1, 2019 4:40 AM To: Cohen, Eugene Cc: Ashish Singhal; Wu, Hao A; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Kim, Sangwoo (김상우 SW1Lab.) Subject: Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems On Fri, 1 Mar 2019 at 11:54, Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>> wrote: > > Ard, > > > So before these changes, we were in the exact same situation, but since PC > > platforms never enable DMA above 4 GB in the first place, nobody ever > > noticed until we started running this code on arm64 platforms that have no > > 32-bit addressable DRAM to begin with. > > Interesting - I did not realize that there were designs that were crazy enough to have no addressable DRAM below 4G. > You must be new here :-) But seriously, it does make sense for an implementation to, say, put all peripherals, PCIe resource windows etc in the bottom half and all DRAM in the top half of a 40-bit address space, which is how the AMD Seattle SoC ended with its system memory at address 0x80_0000_0000. Note that on this platform, we can still use 32-bit DMA if we want to with the help of the SMMUs, but we haven't wired those up in UEFI (and the generic host bridge driver did not have the IOMMU hooks at the time) > > The obvious conclusion is that the driver should not set the > > EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute if the device does > > not support it, or, which seems to be our case, if the driver does not > > implement the 64-bit DMA mode that the driver does support. However, > > since there are platforms for which bounce buffering is not an option (since > > there is no 32-bit addressable memory to bounce to), this is not just a > > performance optimization, and so it would be useful to fix the code so it can > > drive all 64-bit DMA capable hardware. > > Okay, that's a great reason - let's get V3 64b ADMA2 in! > > Any objection to committing the original patch in the short term? > not at all Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>> ----------------------------------------------------------------------------------- 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. ----------------------------------------------------------------------------------- ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems 2019-03-01 11:38 ` Ard Biesheuvel 2019-03-01 12:31 ` Ashish Singhal @ 2019-03-01 15:25 ` Ashish Singhal 2019-03-04 2:39 ` Wu, Hao A 1 sibling, 1 reply; 24+ messages in thread From: Ashish Singhal @ 2019-03-01 15:25 UTC (permalink / raw) To: Ard Biesheuvel, Cohen, Eugene Cc: Wu, Hao A, edk2-devel@lists.01.org, Kim, Sangwoo (김상우 SW1Lab.) Acked-by: Ashish Singhal <ashishsingha@nvidia.com> -----Original Message----- From: Ard Biesheuvel <ard.biesheuvel@linaro.org> Sent: Friday, March 1, 2019 4:39 AM To: Cohen, Eugene <eugene@hp.com> Cc: Ashish Singhal <ashishsingha@nvidia.com>; Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Kim, Sangwoo (김상우 SW1Lab.) <sangwoo.kim@hp.com> Subject: Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems On Fri, 1 Mar 2019 at 11:54, Cohen, Eugene <eugene@hp.com> wrote: > > Ard, > > > So before these changes, we were in the exact same situation, but > > since PC platforms never enable DMA above 4 GB in the first place, > > nobody ever noticed until we started running this code on arm64 > > platforms that have no 32-bit addressable DRAM to begin with. > > Interesting - I did not realize that there were designs that were crazy enough to have no addressable DRAM below 4G. > You must be new here :-) But seriously, it does make sense for an implementation to, say, put all peripherals, PCIe resource windows etc in the bottom half and all DRAM in the top half of a 40-bit address space, which is how the AMD Seattle SoC ended with its system memory at address 0x80_0000_0000. Note that on this platform, we can still use 32-bit DMA if we want to with the help of the SMMUs, but we haven't wired those up in UEFI (and the generic host bridge driver did not have the IOMMU hooks at the time) > > The obvious conclusion is that the driver should not set the > > EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute if the device does > > not support it, or, which seems to be our case, if the driver does > > not implement the 64-bit DMA mode that the driver does support. > > However, since there are platforms for which bounce buffering is not > > an option (since there is no 32-bit addressable memory to bounce > > to), this is not just a performance optimization, and so it would be > > useful to fix the code so it can drive all 64-bit DMA capable hardware. > > Okay, that's a great reason - let's get V3 64b ADMA2 in! > > Any objection to committing the original patch in the short term? > not at all Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> ----------------------------------------------------------------------------------- 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. ----------------------------------------------------------------------------------- ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems 2019-03-01 15:25 ` Ashish Singhal @ 2019-03-04 2:39 ` Wu, Hao A 2019-03-04 4:00 ` Ashish Singhal 0 siblings, 1 reply; 24+ messages in thread From: Wu, Hao A @ 2019-03-04 2:39 UTC (permalink / raw) To: Cohen, Eugene, Ashish Singhal, Ard Biesheuvel Cc: edk2-devel@lists.01.org, Kim, Sangwoo (??? SW1Lab.) Hi Eugene, Ashish and Ard Sorry for the delayed response, I was out of office in the previous several days. According to the discussion, my understanding is that (quote the comments from Ard): > Driver should not set the EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute > 1. If the device does not support it; > 2. If the driver does not implement the 64-bit DMA mode that the device does > support. Thus, for the current implementation of the SdMmcPciHcDxe driver (including the V4 ADMA descriptor support from Ashish): * The driver should set the DUAL_ADDRESS_CYCLE attribute only when 'SysBus64V4' bit set, because of the statement 2 above. And for the previous implementation (before the change from Ashish): * The driver should not set the DUAL_ADDRESS_CYCLE attribute at all, since the implementation was written to support only the 32b ADMA descriptor. If this is true, I am fine with your proposed fix. Eugene, Could you help to state the reason for the fix a bit more clear in the commit log? Also, I have filed a Bugzilla tracker for this one: https://bugzilla.tianocore.org/show_bug.cgi?id=1583 Could you help to add this information into the commit log as well? Thanks. Best Regards, Hao Wu > -----Original Message----- > From: Ashish Singhal [mailto:ashishsingha@nvidia.com] > Sent: Friday, March 01, 2019 11:25 PM > To: Ard Biesheuvel; Cohen, Eugene > Cc: Wu, Hao A; edk2-devel@lists.01.org; Kim, Sangwoo (김상우 SW1Lab.) > Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 > 64-bit systems > > Acked-by: Ashish Singhal <ashishsingha@nvidia.com> > > -----Original Message----- > From: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Sent: Friday, March 1, 2019 4:39 AM > To: Cohen, Eugene <eugene@hp.com> > Cc: Ashish Singhal <ashishsingha@nvidia.com>; Wu, Hao A > <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Kim, Sangwoo (김상우 > SW1Lab.) <sangwoo.kim@hp.com> > Subject: Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 > 64-bit systems > > On Fri, 1 Mar 2019 at 11:54, Cohen, Eugene <eugene@hp.com> wrote: > > > > Ard, > > > > > So before these changes, we were in the exact same situation, but > > > since PC platforms never enable DMA above 4 GB in the first place, > > > nobody ever noticed until we started running this code on arm64 > > > platforms that have no 32-bit addressable DRAM to begin with. > > > > Interesting - I did not realize that there were designs that were crazy > enough to have no addressable DRAM below 4G. > > > > You must be new here :-) > > But seriously, it does make sense for an implementation to, say, put all > peripherals, PCIe resource windows etc in the bottom half and all DRAM in > the top half of a 40-bit address space, which is how the AMD Seattle SoC > ended with its system memory at address 0x80_0000_0000. > Note that on this platform, we can still use 32-bit DMA if we want to with the > help of the SMMUs, but we haven't wired those up in UEFI (and the generic > host bridge driver did not have the IOMMU hooks at the > time) > > > > The obvious conclusion is that the driver should not set the > > > EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute if the device > does > > > not support it, or, which seems to be our case, if the driver does > > > not implement the 64-bit DMA mode that the driver does support. > > > However, since there are platforms for which bounce buffering is not > > > an option (since there is no 32-bit addressable memory to bounce > > > to), this is not just a performance optimization, and so it would be > > > useful to fix the code so it can drive all 64-bit DMA capable hardware. > > > > Okay, that's a great reason - let's get V3 64b ADMA2 in! > > > > Any objection to committing the original patch in the short term? > > > > not at all > > Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > ----------------------------------------------------------------------------------- > 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. > ----------------------------------------------------------------------------------- ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems 2019-03-04 2:39 ` Wu, Hao A @ 2019-03-04 4:00 ` Ashish Singhal 2019-03-04 4:26 ` Wu, Hao A 2019-03-05 11:58 ` Cohen, Eugene 0 siblings, 2 replies; 24+ messages in thread From: Ashish Singhal @ 2019-03-04 4:00 UTC (permalink / raw) To: Wu, Hao A, Cohen, Eugene, Ard Biesheuvel Cc: edk2-devel@lists.01.org, Kim, Sangwoo (??? SW1Lab.) Hi Hao, I agree that there has been a bug all along which got exposed just now. We should submit the patch as proposed by Eugene. Also, I have submitted the patch for enabling 64b DMA for V3. Please take that into consideration once the freeze is over so that we can fix the issue in real sense. Eugene, Please let me know once you have tried my patch on your board. Thanks Ashish -----Original Message----- From: Wu, Hao A <hao.a.wu@intel.com> Sent: Sunday, March 3, 2019 7:39 PM To: Cohen, Eugene <eugene@hp.com>; Ashish Singhal <ashishsingha@nvidia.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: edk2-devel@lists.01.org; Kim, Sangwoo (??? SW1Lab.) <sangwoo.kim@hp.com> Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems Hi Eugene, Ashish and Ard Sorry for the delayed response, I was out of office in the previous several days. According to the discussion, my understanding is that (quote the comments from Ard): > Driver should not set the EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE > attribute 1. If the device does not support it; 2. If the driver does > not implement the 64-bit DMA mode that the device does > support. Thus, for the current implementation of the SdMmcPciHcDxe driver (including the V4 ADMA descriptor support from Ashish): * The driver should set the DUAL_ADDRESS_CYCLE attribute only when 'SysBus64V4' bit set, because of the statement 2 above. And for the previous implementation (before the change from Ashish): * The driver should not set the DUAL_ADDRESS_CYCLE attribute at all, since the implementation was written to support only the 32b ADMA descriptor. If this is true, I am fine with your proposed fix. Eugene, Could you help to state the reason for the fix a bit more clear in the commit log? Also, I have filed a Bugzilla tracker for this one: https://bugzilla.tianocore.org/show_bug.cgi?id=1583 Could you help to add this information into the commit log as well? Thanks. Best Regards, Hao Wu > -----Original Message----- > From: Ashish Singhal [mailto:ashishsingha@nvidia.com] > Sent: Friday, March 01, 2019 11:25 PM > To: Ard Biesheuvel; Cohen, Eugene > Cc: Wu, Hao A; edk2-devel@lists.01.org; Kim, Sangwoo (김상우 SW1Lab.) > Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 > 64-bit systems > > Acked-by: Ashish Singhal <ashishsingha@nvidia.com> > > -----Original Message----- > From: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Sent: Friday, March 1, 2019 4:39 AM > To: Cohen, Eugene <eugene@hp.com> > Cc: Ashish Singhal <ashishsingha@nvidia.com>; Wu, Hao A > <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Kim, Sangwoo (김상우 > SW1Lab.) <sangwoo.kim@hp.com> > Subject: Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 > 64-bit systems > > On Fri, 1 Mar 2019 at 11:54, Cohen, Eugene <eugene@hp.com> wrote: > > > > Ard, > > > > > So before these changes, we were in the exact same situation, but > > > since PC platforms never enable DMA above 4 GB in the first place, > > > nobody ever noticed until we started running this code on arm64 > > > platforms that have no 32-bit addressable DRAM to begin with. > > > > Interesting - I did not realize that there were designs that were > > crazy > enough to have no addressable DRAM below 4G. > > > > You must be new here :-) > > But seriously, it does make sense for an implementation to, say, put > all peripherals, PCIe resource windows etc in the bottom half and all > DRAM in the top half of a 40-bit address space, which is how the AMD > Seattle SoC ended with its system memory at address 0x80_0000_0000. > Note that on this platform, we can still use 32-bit DMA if we want to > with the help of the SMMUs, but we haven't wired those up in UEFI (and > the generic host bridge driver did not have the IOMMU hooks at the > time) > > > > The obvious conclusion is that the driver should not set the > > > EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute if the device > does > > > not support it, or, which seems to be our case, if the driver does > > > not implement the 64-bit DMA mode that the driver does support. > > > However, since there are platforms for which bounce buffering is > > > not an option (since there is no 32-bit addressable memory to > > > bounce to), this is not just a performance optimization, and so it > > > would be useful to fix the code so it can drive all 64-bit DMA capable hardware. > > > > Okay, that's a great reason - let's get V3 64b ADMA2 in! > > > > Any objection to committing the original patch in the short term? > > > > not at all > > Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > ---------------------------------------------------------------------- > ------------- 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. > ----------------------------------------------------------------------------------- ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems 2019-03-04 4:00 ` Ashish Singhal @ 2019-03-04 4:26 ` Wu, Hao A 2019-03-05 11:58 ` Cohen, Eugene 1 sibling, 0 replies; 24+ messages in thread From: Wu, Hao A @ 2019-03-04 4:26 UTC (permalink / raw) To: Ashish Singhal, Cohen, Eugene, Ard Biesheuvel Cc: edk2-devel@lists.01.org, Kim, Sangwoo (??? SW1Lab.) > -----Original Message----- > From: Ashish Singhal [mailto:ashishsingha@nvidia.com] > Sent: Monday, March 04, 2019 12:00 PM > To: Wu, Hao A; Cohen, Eugene; Ard Biesheuvel > Cc: edk2-devel@lists.01.org; Kim, Sangwoo (??? SW1Lab.) > Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 > 64-bit systems > > Hi Hao, > > I agree that there has been a bug all along which got exposed just now. We > should submit the patch as proposed by Eugene. Yes. I will co-work with Eugene to address this soon. > > Also, I have submitted the patch for enabling 64b DMA for V3. Please take > that into consideration once the freeze is over so that we can fix the issue in > real sense. Sure. I will take a look at your proposed patch. Please grant me some time for the review and test. Best Regards, Hao Wu > > Eugene, > > Please let me know once you have tried my patch on your board. > > Thanks > Ashish > > -----Original Message----- > From: Wu, Hao A <hao.a.wu@intel.com> > Sent: Sunday, March 3, 2019 7:39 PM > To: Cohen, Eugene <eugene@hp.com>; Ashish Singhal > <ashishsingha@nvidia.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: edk2-devel@lists.01.org; Kim, Sangwoo (??? SW1Lab.) > <sangwoo.kim@hp.com> > Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 > 64-bit systems > > Hi Eugene, Ashish and Ard > > Sorry for the delayed response, I was out of office in the previous several > days. > > According to the discussion, my understanding is that (quote the comments > from > Ard): > > > Driver should not set the EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE > > attribute 1. If the device does not support it; 2. If the driver does > > not implement the 64-bit DMA mode that the device does > > support. > > Thus, for the current implementation of the SdMmcPciHcDxe driver > (including the > V4 ADMA descriptor support from Ashish): > > * The driver should set the DUAL_ADDRESS_CYCLE attribute only when > 'SysBus64V4' > bit set, because of the statement 2 above. > > And for the previous implementation (before the change from Ashish): > > * The driver should not set the DUAL_ADDRESS_CYCLE attribute at all, since > the > implementation was written to support only the 32b ADMA descriptor. > > If this is true, I am fine with your proposed fix. > > > Eugene, > > Could you help to state the reason for the fix a bit more clear in the commit > log? > > Also, I have filed a Bugzilla tracker for this one: > https://bugzilla.tianocore.org/show_bug.cgi?id=1583 > > Could you help to add this information into the commit log as well? Thanks. > > Best Regards, > Hao Wu > > > -----Original Message----- > > From: Ashish Singhal [mailto:ashishsingha@nvidia.com] > > Sent: Friday, March 01, 2019 11:25 PM > > To: Ard Biesheuvel; Cohen, Eugene > > Cc: Wu, Hao A; edk2-devel@lists.01.org; Kim, Sangwoo (김상우 SW1Lab.) > > Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC > v3 > > 64-bit systems > > > > Acked-by: Ashish Singhal <ashishsingha@nvidia.com> > > > > -----Original Message----- > > From: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > Sent: Friday, March 1, 2019 4:39 AM > > To: Cohen, Eugene <eugene@hp.com> > > Cc: Ashish Singhal <ashishsingha@nvidia.com>; Wu, Hao A > > <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Kim, Sangwoo (김상우 > > SW1Lab.) <sangwoo.kim@hp.com> > > Subject: Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC > v3 > > 64-bit systems > > > > On Fri, 1 Mar 2019 at 11:54, Cohen, Eugene <eugene@hp.com> wrote: > > > > > > Ard, > > > > > > > So before these changes, we were in the exact same situation, but > > > > since PC platforms never enable DMA above 4 GB in the first place, > > > > nobody ever noticed until we started running this code on arm64 > > > > platforms that have no 32-bit addressable DRAM to begin with. > > > > > > Interesting - I did not realize that there were designs that were > > > crazy > > enough to have no addressable DRAM below 4G. > > > > > > > You must be new here :-) > > > > But seriously, it does make sense for an implementation to, say, put > > all peripherals, PCIe resource windows etc in the bottom half and all > > DRAM in the top half of a 40-bit address space, which is how the AMD > > Seattle SoC ended with its system memory at address 0x80_0000_0000. > > Note that on this platform, we can still use 32-bit DMA if we want to > > with the help of the SMMUs, but we haven't wired those up in UEFI (and > > the generic host bridge driver did not have the IOMMU hooks at the > > time) > > > > > > The obvious conclusion is that the driver should not set the > > > > EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute if the device > > does > > > > not support it, or, which seems to be our case, if the driver does > > > > not implement the 64-bit DMA mode that the driver does support. > > > > However, since there are platforms for which bounce buffering is > > > > not an option (since there is no 32-bit addressable memory to > > > > bounce to), this is not just a performance optimization, and so it > > > > would be useful to fix the code so it can drive all 64-bit DMA capable > hardware. > > > > > > Okay, that's a great reason - let's get V3 64b ADMA2 in! > > > > > > Any objection to committing the original patch in the short term? > > > > > > > not at all > > > > Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > > > ---------------------------------------------------------------------- > > ------------- 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. > > ----------------------------------------------------------------------------------- ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems 2019-03-04 4:00 ` Ashish Singhal 2019-03-04 4:26 ` Wu, Hao A @ 2019-03-05 11:58 ` Cohen, Eugene 1 sibling, 0 replies; 24+ messages in thread From: Cohen, Eugene @ 2019-03-05 11:58 UTC (permalink / raw) To: Ashish Singhal, Wu, Hao A, Ard Biesheuvel Cc: edk2-devel@lists.01.org, Kim, Sangwoo (김상우 SW1Lab.) [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 6438 bytes --] Ashish, Thanks - I haven't forgotten. We are still doing tests with the 32-bit ADMA driver and running into issues on our newer platform - once we have those resolved we will test the patch. (We are seeing a strange issue where Read Multiple Block works but Write Multiple Block does not.) Eugene From: Ashish Singhal <ashishsingha@nvidia.com> Sent: Sunday, March 3, 2019 9:00 PM To: Wu, Hao A <hao.a.wu@intel.com>; Cohen, Eugene <eugene@hp.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: edk2-devel@lists.01.org; Kim, Sangwoo (ê¹ìì° SW1Lab.) <sangwoo.kim@hp.com> Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems Hi Hao, I agree that there has been a bug all along which got exposed just now. We should submit the patch as proposed by Eugene. Also, I have submitted the patch for enabling 64b DMA for V3. Please take that into consideration once the freeze is over so that we can fix the issue in real sense. Eugene, Please let me know once you have tried my patch on your board. Thanks Ashish -----Original Message----- From: Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>> Sent: Sunday, March 3, 2019 7:39 PM To: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>>; Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>>; Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>> Cc: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Kim, Sangwoo (??? SW1Lab.) <sangwoo.kim@hp.com<mailto:sangwoo.kim@hp.com>> Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems Hi Eugene, Ashish and Ard Sorry for the delayed response, I was out of office in the previous several days. According to the discussion, my understanding is that (quote the comments from Ard): > Driver should not set the EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE > attribute 1. If the device does not support it; 2. If the driver does > not implement the 64-bit DMA mode that the device does > support. Thus, for the current implementation of the SdMmcPciHcDxe driver (including the V4 ADMA descriptor support from Ashish): * The driver should set the DUAL_ADDRESS_CYCLE attribute only when 'SysBus64V4' bit set, because of the statement 2 above. And for the previous implementation (before the change from Ashish): * The driver should not set the DUAL_ADDRESS_CYCLE attribute at all, since the implementation was written to support only the 32b ADMA descriptor. If this is true, I am fine with your proposed fix. Eugene, Could you help to state the reason for the fix a bit more clear in the commit log? Also, I have filed a Bugzilla tracker for this one: https://bugzilla.tianocore.org/show_bug.cgi?id=1583<https://bugzilla.tianocore.org/show_bug.cgi?id=1583> Could you help to add this information into the commit log as well? Thanks. Best Regards, Hao Wu > -----Original Message----- > From: Ashish Singhal [mailto:ashishsingha@nvidia.com] > Sent: Friday, March 01, 2019 11:25 PM > To: Ard Biesheuvel; Cohen, Eugene > Cc: Wu, Hao A; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Kim, Sangwoo (ê¹ìì° SW1Lab.) > Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 > 64-bit systems > > Acked-by: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>> > > -----Original Message----- > From: Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>> > Sent: Friday, March 1, 2019 4:39 AM > To: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>> > Cc: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>>; Wu, Hao A > <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Kim, Sangwoo (ê¹ìì° > SW1Lab.) <sangwoo.kim@hp.com<mailto:sangwoo.kim@hp.com>> > Subject: Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 > 64-bit systems > > On Fri, 1 Mar 2019 at 11:54, Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>> wrote: > > > > Ard, > > > > > So before these changes, we were in the exact same situation, but > > > since PC platforms never enable DMA above 4 GB in the first place, > > > nobody ever noticed until we started running this code on arm64 > > > platforms that have no 32-bit addressable DRAM to begin with. > > > > Interesting - I did not realize that there were designs that were > > crazy > enough to have no addressable DRAM below 4G. > > > > You must be new here :-) > > But seriously, it does make sense for an implementation to, say, put > all peripherals, PCIe resource windows etc in the bottom half and all > DRAM in the top half of a 40-bit address space, which is how the AMD > Seattle SoC ended with its system memory at address 0x80_0000_0000. > Note that on this platform, we can still use 32-bit DMA if we want to > with the help of the SMMUs, but we haven't wired those up in UEFI (and > the generic host bridge driver did not have the IOMMU hooks at the > time) > > > > The obvious conclusion is that the driver should not set the > > > EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute if the device > does > > > not support it, or, which seems to be our case, if the driver does > > > not implement the 64-bit DMA mode that the driver does support. > > > However, since there are platforms for which bounce buffering is > > > not an option (since there is no 32-bit addressable memory to > > > bounce to), this is not just a performance optimization, and so it > > > would be useful to fix the code so it can drive all 64-bit DMA capable hardware. > > > > Okay, that's a great reason - let's get V3 64b ADMA2 in! > > > > Any objection to committing the original patch in the short term? > > > > not at all > > Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>> > > ---------------------------------------------------------------------- > ------------- 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. > ----------------------------------------------------------------------------------- \x16º& ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems 2019-03-01 0:19 ` Ashish Singhal 2019-03-01 10:32 ` Ard Biesheuvel @ 2019-03-01 11:02 ` Cohen, Eugene 1 sibling, 0 replies; 24+ messages in thread From: Cohen, Eugene @ 2019-03-01 11:02 UTC (permalink / raw) To: Ashish Singhal, Wu, Hao A, edk2-devel@lists.01.org, Ard Biesheuvel Ashish, Yes this issue existed before V4 support came in. The previous code would test the (V3) SysBus64 bit: https://github.com/tianocore/edk2/blob/7f3b0bad4bbb3cb24014d2e6216615896ea09dbf/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c#L652 And then return an error building the ADMA descriptor table because the address is not within 32-bit DMA range: https://github.com/tianocore/edk2/blob/7f3b0bad4bbb3cb24014d2e6216615896ea09dbf/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c#L1295 Eugene From: Ashish Singhal <ashishsingha@nvidia.com> Sent: Thursday, February 28, 2019 5:19 PM To: Cohen, Eugene <eugene@hp.com>; Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Ard Biesheuvel <ard.biesheuvel@linaro.org> Subject: Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems Eugene, Small question. Did the issue appear after the V4 patch went in? Looking at the code before that patch, we were enabling 64b dma in pci based on capability register already despite of driver supporting only 32b dma. Thanks Ashish Get Outlook for iOS<https://aka.ms/o0ukef> ________________________________ From: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>> Sent: Thursday, February 28, 2019 5:11 PM To: Ashish Singhal; Wu, Hao A; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Ard Biesheuvel Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems Ashish, Agreed - #2 would be better in the long run since this will have better performance by eliminating the bounce buffering. My original intent in submitting the patch was to fix the logic the current implementation with minimal impact. Thanks, Eugene From: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>> Sent: Thursday, February 28, 2019 4:58 PM To: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>> Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems Eugene, Thanks for pointing that out. This is a use case we are not covering as of now. I see two options to this: 1. Do not enable 64b DMA support in PCI based on V3 as driver does not support V3 64b ADMA. This is a quick fix. 2. Enable V3 64b ADMA support to add the missing feature. This will take maybe a day or two and can be done. Thanks Ashish From: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>> Sent: Thursday, February 28, 2019 3:40 PM To: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>> Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems Ashish, I think that code will still fail for our use case. We are version 3 with 64-bit support so Private->Capability[Slot].SysBus64V3 == 0 will evaluate to FALSE. Since we are V3 Private->ControllerVersion[Slot] >= SD_MMC_HC_CTRL_VER_400 will also evaluate to FALSE. Therefore Support64BitDma will still be TRUE resulting in DUAL_ADDRESS_CYCLE being set which disables bounce buffering. Since no code is in place to do V3 64b DMA we will still hit the same problem, specifically namely that buffers that are not DMAable will be allocated and we will still fail the check here<https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c#L1426>. Until such time that V3 64b DMA support is in place I believe only the V4 bit should be evaluated. Eugene From: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>> Sent: Thursday, February 28, 2019 3:21 PM To: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>> Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems Eugene, Thanks for the explanation. The problem is valid and is more clear to me now. How about we do this: Instead of: if (Private->Capability[Slot].SysBus64V3 == 0 && Private->Capability[Slot].SysBus64V4 == 0) { Support64BitDma = FALSE; } What do you think about: if ((Private->ControllerVersion[Slot] == SD_MMC_HC_CTRL_VER_300 && Private->Capability[Slot].SysBus64V3 == 0) || (Private->ControllerVersion[Slot] >= SD_MMC_HC_CTRL_VER_400 && Private->Capability[Slot].SysBus64V4 == 0)) { Support64BitDma = FALSE; } With this, we would be checking 64b capability based on the version we are using and not for something we may not be using despite of being advertised in the controller. Thanks Ashish From: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>> Sent: Thursday, February 28, 2019 2:59 PM To: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>> Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems Ashish, > 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. That is precisely the problem. An SDHC v3 controller might support 64b DMA in V3 but not in V4 mode. The current code will leave 64b DMA support enabled resulting in the issuing of the PCI DUAL_ADDRESS_CYCLE attribute ( see https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c#L738<https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c#L738> ) which then causes buffers to be allocated that cannot be DMAed. For reference look at this snippet of the NonDiscoverablePciDeviceIo driver: https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c#L622<https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c#L622> and you can see that bounce buffering will only occur if DUAL_ADDRESS_CYCLE is clear. So since we do not have V3 64b DMA (96-bit descriptor) support in place we must not allow the DUAL_ADDRESS_CYCLE attribute to be set or we will fail with this check: https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c#L1426<https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c#L1426> I've added Ard who updated the driver with DUAL_ADDRESS_CYCLE support. Eugene From: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>> Sent: Thursday, February 28, 2019 2:28 PM To: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@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 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 <eugene@hp.com<mailto:eugene@hp.com>> Sent: Thursday, February 28, 2019 12:56 PM To: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@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 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 <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>> Sent: Thursday, February 28, 2019 12:15 PM To: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@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 <eugene@hp.com<mailto:eugene@hp.com>> Sent: Thursday, February 28, 2019 4:24 AM To: Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> Cc: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@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.wu@intel.com<mailto:hao.a.wu@intel.com>> Sent: Wednesday, February 27, 2019 8:25 PM To: Cohen, Eugene <eugene@hp.com<mailto:eugene@hp.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> Cc: Ashish Singhal <ashishsingha@nvidia.com<mailto: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<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. ----------------------------------------------------------------------------------- ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2019-03-05 11:58 UTC | newest] Thread overview: 24+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-02-27 10:58 [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems Cohen, Eugene 2019-02-28 3:24 ` Wu, Hao A 2019-02-28 11:23 ` Cohen, Eugene 2019-02-28 19:15 ` Ashish Singhal 2019-02-28 19:56 ` Cohen, Eugene 2019-02-28 21:27 ` Ashish Singhal 2019-02-28 21:58 ` Cohen, Eugene 2019-02-28 22:20 ` Ashish Singhal 2019-02-28 22:40 ` Cohen, Eugene 2019-02-28 23:58 ` Ashish Singhal 2019-03-01 0:10 ` Cohen, Eugene 2019-03-01 0:19 ` Ashish Singhal 2019-03-01 10:32 ` Ard Biesheuvel 2019-03-01 10:34 ` Ard Biesheuvel 2019-03-01 10:54 ` Cohen, Eugene 2019-03-01 11:38 ` Ard Biesheuvel 2019-03-01 12:31 ` Ashish Singhal 2019-03-01 18:31 ` Ashish Singhal 2019-03-01 15:25 ` Ashish Singhal 2019-03-04 2:39 ` Wu, Hao A 2019-03-04 4:00 ` Ashish Singhal 2019-03-04 4:26 ` Wu, Hao A 2019-03-05 11:58 ` Cohen, Eugene 2019-03-01 11:02 ` Cohen, Eugene
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox