* [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit IO widths
@ 2019-01-30 23:58 Jeff Brasen
2019-02-01 5:55 ` Wu, Hao A
0 siblings, 1 reply; 9+ messages in thread
From: Jeff Brasen @ 2019-01-30 23:58 UTC (permalink / raw)
To: edk2-devel; +Cc: Edgar Handal, Jeff Brasen
From: Edgar Handal <ehandal@nvidia.com>
Use 16-bit and 32-bit IO widths for SDMMC MMIO to prevent all register
accesses from being split up into 8-bit accesses.
The SDHCI specification states that the registers shall be accessable in
byte, word, and double word accesses.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
---
MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 25 ++++++++++++++++++++----
1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
index 5aec8c6..82f4493 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
@@ -152,19 +152,36 @@ SdMmcHcRwMmio (
)
{
EFI_STATUS Status;
+ EFI_PCI_IO_PROTOCOL_WIDTH Width;
if ((PciIo == NULL) || (Data == NULL)) {
return EFI_INVALID_PARAMETER;
}
- if ((Count != 1) && (Count != 2) && (Count != 4) && (Count != 8)) {
- return EFI_INVALID_PARAMETER;
+ switch (Count) {
+ case 1:
+ Width = EfiPciIoWidthUint8;
+ break;
+ case 2:
+ Width = EfiPciIoWidthUint16;
+ Count = 1;
+ break;
+ case 4:
+ Width = EfiPciIoWidthUint32;
+ Count = 1;
+ break;
+ case 8:
+ Width = EfiPciIoWidthUint32;
+ Count = 2;
+ break;
+ default:
+ return EFI_INVALID_PARAMETER;
}
if (Read) {
Status = PciIo->Mem.Read (
PciIo,
- EfiPciIoWidthUint8,
+ Width,
BarIndex,
(UINT64) Offset,
Count,
@@ -173,7 +190,7 @@ SdMmcHcRwMmio (
} else {
Status = PciIo->Mem.Write (
PciIo,
- EfiPciIoWidthUint8,
+ Width,
BarIndex,
(UINT64) Offset,
Count,
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit IO widths
2019-01-30 23:58 [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit IO widths Jeff Brasen
@ 2019-02-01 5:55 ` Wu, Hao A
2019-02-01 7:12 ` Jeff Brasen
2019-02-03 12:39 ` Ard Biesheuvel
0 siblings, 2 replies; 9+ messages in thread
From: Wu, Hao A @ 2019-02-01 5:55 UTC (permalink / raw)
To: Jeff Brasen, edk2-devel@lists.01.org; +Cc: Edgar Handal
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Jeff
> Brasen
> Sent: Thursday, January 31, 2019 7:59 AM
> To: edk2-devel@lists.01.org
> Cc: Edgar Handal; Jeff Brasen
> Subject: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit IO
> widths
>
> From: Edgar Handal <ehandal@nvidia.com>
>
> Use 16-bit and 32-bit IO widths for SDMMC MMIO to prevent all register
> accesses from being split up into 8-bit accesses.
>
> The SDHCI specification states that the registers shall be accessable in
> byte, word, and double word accesses.
Hi,
Thanks for the contribution. The change seems good to me.
Just curious, if the accesses are always slit into byte(8-bit), is there any
issue or performance impact is encountered during your usage?
It will be helpful to get more information on the purpose of the patch.
Thanks.
Best Regards,
Hao Wu
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> ---
> MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 25
> ++++++++++++++++++++----
> 1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> index 5aec8c6..82f4493 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> @@ -152,19 +152,36 @@ SdMmcHcRwMmio (
> )
> {
> EFI_STATUS Status;
> + EFI_PCI_IO_PROTOCOL_WIDTH Width;
>
> if ((PciIo == NULL) || (Data == NULL)) {
> return EFI_INVALID_PARAMETER;
> }
>
> - if ((Count != 1) && (Count != 2) && (Count != 4) && (Count != 8)) {
> - return EFI_INVALID_PARAMETER;
> + switch (Count) {
> + case 1:
> + Width = EfiPciIoWidthUint8;
> + break;
> + case 2:
> + Width = EfiPciIoWidthUint16;
> + Count = 1;
> + break;
> + case 4:
> + Width = EfiPciIoWidthUint32;
> + Count = 1;
> + break;
> + case 8:
> + Width = EfiPciIoWidthUint32;
> + Count = 2;
> + break;
> + default:
> + return EFI_INVALID_PARAMETER;
> }
>
> if (Read) {
> Status = PciIo->Mem.Read (
> PciIo,
> - EfiPciIoWidthUint8,
> + Width,
> BarIndex,
> (UINT64) Offset,
> Count,
> @@ -173,7 +190,7 @@ SdMmcHcRwMmio (
> } else {
> Status = PciIo->Mem.Write (
> PciIo,
> - EfiPciIoWidthUint8,
> + Width,
> BarIndex,
> (UINT64) Offset,
> Count,
> --
> 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] 9+ messages in thread
* Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit IO widths
2019-02-01 5:55 ` Wu, Hao A
@ 2019-02-01 7:12 ` Jeff Brasen
2019-02-01 7:54 ` Wu, Hao A
2019-02-03 12:39 ` Ard Biesheuvel
1 sibling, 1 reply; 9+ messages in thread
From: Jeff Brasen @ 2019-02-01 7:12 UTC (permalink / raw)
To: Wu, Hao A, edk2-devel@lists.01.org; +Cc: Edgar Handal
-----Original Message-----
From: Wu, Hao A <hao.a.wu@intel.com>
Sent: Thursday, January 31, 2019 10:56 PM
To: Jeff Brasen <jbrasen@nvidia.com>; edk2-devel@lists.01.org
Cc: Edgar Handal <ehandal@nvidia.com>
Subject: RE: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit IO widths
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Jeff Brasen
> Sent: Thursday, January 31, 2019 7:59 AM
> To: edk2-devel@lists.01.org
> Cc: Edgar Handal; Jeff Brasen
> Subject: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit IO
> widths
>
> From: Edgar Handal <ehandal@nvidia.com>
>
> Use 16-bit and 32-bit IO widths for SDMMC MMIO to prevent all register
> accesses from being split up into 8-bit accesses.
>
> The SDHCI specification states that the registers shall be accessable
> in byte, word, and double word accesses.
Hi,
Thanks for the contribution. The change seems good to me.
Just curious, if the accesses are always slit into byte(8-bit), is there any issue or performance impact is encountered during your usage?
It will be helpful to get more information on the purpose of the patch.
Thanks.
Best Regards,
Hao Wu
[JMB] We were working with a simulation module that has some issues when accessing 16 or 32 bit registers by byte (This should be supported per the SDHCI specification.), and this patch resolves this while we work on getting the model fixed. This should also optimize performance as there will less read/write instructions (My guess is this is a marginal improvement as most of the SD access time would be DMA operations.)
Thanks,
Jeff
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> ---
> MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 25
> ++++++++++++++++++++----
> 1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> index 5aec8c6..82f4493 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> @@ -152,19 +152,36 @@ SdMmcHcRwMmio (
> )
> {
> EFI_STATUS Status;
> + EFI_PCI_IO_PROTOCOL_WIDTH Width;
>
> if ((PciIo == NULL) || (Data == NULL)) {
> return EFI_INVALID_PARAMETER;
> }
>
> - if ((Count != 1) && (Count != 2) && (Count != 4) && (Count != 8)) {
> - return EFI_INVALID_PARAMETER;
> + switch (Count) {
> + case 1:
> + Width = EfiPciIoWidthUint8;
> + break;
> + case 2:
> + Width = EfiPciIoWidthUint16;
> + Count = 1;
> + break;
> + case 4:
> + Width = EfiPciIoWidthUint32;
> + Count = 1;
> + break;
> + case 8:
> + Width = EfiPciIoWidthUint32;
> + Count = 2;
> + break;
> + default:
> + return EFI_INVALID_PARAMETER;
> }
>
> if (Read) {
> Status = PciIo->Mem.Read (
> PciIo,
> - EfiPciIoWidthUint8,
> + Width,
> BarIndex,
> (UINT64) Offset,
> Count,
> @@ -173,7 +190,7 @@ SdMmcHcRwMmio (
> } else {
> Status = PciIo->Mem.Write (
> PciIo,
> - EfiPciIoWidthUint8,
> + Width,
> BarIndex,
> (UINT64) Offset,
> Count,
> --
> 2.7.4
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information. Any unauthorized review, use, disclosure or distribution
is prohibited. If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit IO widths
2019-02-01 7:12 ` Jeff Brasen
@ 2019-02-01 7:54 ` Wu, Hao A
2019-02-01 17:52 ` Jeff Brasen
0 siblings, 1 reply; 9+ messages in thread
From: Wu, Hao A @ 2019-02-01 7:54 UTC (permalink / raw)
To: Jeff Brasen, edk2-devel@lists.01.org; +Cc: Edgar Handal
> -----Original Message-----
> From: Jeff Brasen [mailto:jbrasen@nvidia.com]
> Sent: Friday, February 01, 2019 3:12 PM
> To: Wu, Hao A; edk2-devel@lists.01.org
> Cc: Edgar Handal
> Subject: RE: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit IO
> widths
>
>
>
> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Thursday, January 31, 2019 10:56 PM
> To: Jeff Brasen <jbrasen@nvidia.com>; edk2-devel@lists.01.org
> Cc: Edgar Handal <ehandal@nvidia.com>
> Subject: RE: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit IO
> widths
>
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> > Jeff Brasen
> > Sent: Thursday, January 31, 2019 7:59 AM
> > To: edk2-devel@lists.01.org
> > Cc: Edgar Handal; Jeff Brasen
> > Subject: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit IO
> > widths
> >
> > From: Edgar Handal <ehandal@nvidia.com>
> >
> > Use 16-bit and 32-bit IO widths for SDMMC MMIO to prevent all register
> > accesses from being split up into 8-bit accesses.
> >
> > The SDHCI specification states that the registers shall be accessable
> > in byte, word, and double word accesses.
>
> Hi,
>
> Thanks for the contribution. The change seems good to me.
>
> Just curious, if the accesses are always slit into byte(8-bit), is there any issue or
> performance impact is encountered during your usage?
>
> It will be helpful to get more information on the purpose of the patch.
> Thanks.
>
> Best Regards,
> Hao Wu
>
> [JMB] We were working with a simulation module that has some issues when
> accessing 16 or 32 bit registers by byte (This should be supported per the SDHCI
> specification.), and this patch resolves this while we work on getting the model
> fixed. This should also optimize performance as there will less read/write
> instructions (My guess is this is a marginal improvement as most of the SD
> access time would be DMA operations.)
Thanks Jeff,
Got it. May I know is it possible for you to collect some performance data
for the change?
Meanwhile, I will test this patch with the boards I own and try to collect
some data. Please grant me some time for this. Since the current
implementation does not violate the spec, let us evaluate whether better
performance will be brought.
Best Regards,
Hao Wu
>
> Thanks,
> Jeff
>
>
>
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> > ---
> > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 25
> > ++++++++++++++++++++----
> > 1 file changed, 21 insertions(+), 4 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > index 5aec8c6..82f4493 100644
> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > @@ -152,19 +152,36 @@ SdMmcHcRwMmio (
> > )
> > {
> > EFI_STATUS Status;
> > + EFI_PCI_IO_PROTOCOL_WIDTH Width;
> >
> > if ((PciIo == NULL) || (Data == NULL)) {
> > return EFI_INVALID_PARAMETER;
> > }
> >
> > - if ((Count != 1) && (Count != 2) && (Count != 4) && (Count != 8)) {
> > - return EFI_INVALID_PARAMETER;
> > + switch (Count) {
> > + case 1:
> > + Width = EfiPciIoWidthUint8;
> > + break;
> > + case 2:
> > + Width = EfiPciIoWidthUint16;
> > + Count = 1;
> > + break;
> > + case 4:
> > + Width = EfiPciIoWidthUint32;
> > + Count = 1;
> > + break;
> > + case 8:
> > + Width = EfiPciIoWidthUint32;
> > + Count = 2;
> > + break;
> > + default:
> > + return EFI_INVALID_PARAMETER;
> > }
> >
> > if (Read) {
> > Status = PciIo->Mem.Read (
> > PciIo,
> > - EfiPciIoWidthUint8,
> > + Width,
> > BarIndex,
> > (UINT64) Offset,
> > Count,
> > @@ -173,7 +190,7 @@ SdMmcHcRwMmio (
> > } else {
> > Status = PciIo->Mem.Write (
> > PciIo,
> > - EfiPciIoWidthUint8,
> > + Width,
> > BarIndex,
> > (UINT64) Offset,
> > Count,
> > --
> > 2.7.4
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> -----------------------------------------------------------------------------------
> This email message is for the sole use of the intended recipient(s) and may
> contain
> confidential information. Any unauthorized review, use, disclosure or
> distribution
> is prohibited. If you are not the intended recipient, please contact the sender
> by
> reply email and destroy all copies of the original message.
> -----------------------------------------------------------------------------------
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit IO widths
2019-02-01 7:54 ` Wu, Hao A
@ 2019-02-01 17:52 ` Jeff Brasen
2019-02-18 2:49 ` Wu, Hao A
0 siblings, 1 reply; 9+ messages in thread
From: Jeff Brasen @ 2019-02-01 17:52 UTC (permalink / raw)
To: Wu, Hao A, edk2-devel@lists.01.org; +Cc: Edgar Handal
________________________________
From: Wu, Hao A <hao.a.wu@intel.com>
Sent: Friday, February 1, 2019 12:54 AM
To: Jeff Brasen; edk2-devel@lists.01.org
Cc: Edgar Handal
Subject: RE: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit IO widths
> -----Original Message-----
> From: Jeff Brasen [mailto:jbrasen@nvidia.com]
> Sent: Friday, February 01, 2019 3:12 PM
> To: Wu, Hao A; edk2-devel@lists.01.org
> Cc: Edgar Handal
> Subject: RE: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit IO
> widths
>
>
>
> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Thursday, January 31, 2019 10:56 PM
> To: Jeff Brasen <jbrasen@nvidia.com>; edk2-devel@lists.01.org
> Cc: Edgar Handal <ehandal@nvidia.com>
> Subject: RE: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit IO
> widths
>
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> > Jeff Brasen
> > Sent: Thursday, January 31, 2019 7:59 AM
> > To: edk2-devel@lists.01.org
> > Cc: Edgar Handal; Jeff Brasen
> > Subject: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit IO
> > widths
> >
> > From: Edgar Handal <ehandal@nvidia.com>
> >
> > Use 16-bit and 32-bit IO widths for SDMMC MMIO to prevent all register
> > accesses from being split up into 8-bit accesses.
> >
> > The SDHCI specification states that the registers shall be accessable
> > in byte, word, and double word accesses.
>
> Hi,
>
> Thanks for the contribution. The change seems good to me.
>
> Just curious, if the accesses are always slit into byte(8-bit), is there any issue or
> performance impact is encountered during your usage?
>
> It will be helpful to get more information on the purpose of the patch.
> Thanks.
>
> Best Regards,
> Hao Wu
>
> [JMB] We were working with a simulation module that has some issues when
> accessing 16 or 32 bit registers by byte (This should be supported per the SDHCI
> specification.), and this patch resolves this while we work on getting the model
> fixed. This should also optimize performance as there will less read/write
> instructions (My guess is this is a marginal improvement as most of the SD
> access time would be DMA operations.)
Thanks Jeff,
Got it. May I know is it possible for you to collect some performance data
for the change?
[JMB] Sure, did a several boots on silicon platform and see an average boot time improvement of 15ms. With a range of 10-20ms and never saw the performance get worse.
Meanwhile, I will test this patch with the boards I own and try to collect
some data. Please grant me some time for this. Since the current
implementation does not violate the spec, let us evaluate whether better
performance will be brought.
Best Regards,
Hao Wu
>
> Thanks,
> Jeff
>
>
>
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> > ---
> > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 25
> > ++++++++++++++++++++----
> > 1 file changed, 21 insertions(+), 4 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > index 5aec8c6..82f4493 100644
> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > @@ -152,19 +152,36 @@ SdMmcHcRwMmio (
> > )
> > {
> > EFI_STATUS Status;
> > + EFI_PCI_IO_PROTOCOL_WIDTH Width;
> >
> > if ((PciIo == NULL) || (Data == NULL)) {
> > return EFI_INVALID_PARAMETER;
> > }
> >
> > - if ((Count != 1) && (Count != 2) && (Count != 4) && (Count != 8)) {
> > - return EFI_INVALID_PARAMETER;
> > + switch (Count) {
> > + case 1:
> > + Width = EfiPciIoWidthUint8;
> > + break;
> > + case 2:
> > + Width = EfiPciIoWidthUint16;
> > + Count = 1;
> > + break;
> > + case 4:
> > + Width = EfiPciIoWidthUint32;
> > + Count = 1;
> > + break;
> > + case 8:
> > + Width = EfiPciIoWidthUint32;
> > + Count = 2;
> > + break;
> > + default:
> > + return EFI_INVALID_PARAMETER;
> > }
> >
> > if (Read) {
> > Status = PciIo->Mem.Read (
> > PciIo,
> > - EfiPciIoWidthUint8,
> > + Width,
> > BarIndex,
> > (UINT64) Offset,
> > Count,
> > @@ -173,7 +190,7 @@ SdMmcHcRwMmio (
> > } else {
> > Status = PciIo->Mem.Write (
> > PciIo,
> > - EfiPciIoWidthUint8,
> > + Width,
> > BarIndex,
> > (UINT64) Offset,
> > Count,
> > --
> > 2.7.4
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> -----------------------------------------------------------------------------------
> This email message is for the sole use of the intended recipient(s) and may
> contain
> confidential information. Any unauthorized review, use, disclosure or
> distribution
> is prohibited. If you are not the intended recipient, please contact the sender
> by
> reply email and destroy all copies of the original message.
> -----------------------------------------------------------------------------------
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit IO widths
2019-02-01 5:55 ` Wu, Hao A
2019-02-01 7:12 ` Jeff Brasen
@ 2019-02-03 12:39 ` Ard Biesheuvel
2019-02-20 1:19 ` Wu, Hao A
1 sibling, 1 reply; 9+ messages in thread
From: Ard Biesheuvel @ 2019-02-03 12:39 UTC (permalink / raw)
To: Wu, Hao A
Cc: Jeff Brasen, edk2-devel@lists.01.org, Edgar Handal, Marcin Wojtas
On Fri, 1 Feb 2019 at 06:55, Wu, Hao A <hao.a.wu@intel.com> wrote:
>
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Jeff
> > Brasen
> > Sent: Thursday, January 31, 2019 7:59 AM
> > To: edk2-devel@lists.01.org
> > Cc: Edgar Handal; Jeff Brasen
> > Subject: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit IO
> > widths
> >
> > From: Edgar Handal <ehandal@nvidia.com>
> >
> > Use 16-bit and 32-bit IO widths for SDMMC MMIO to prevent all register
> > accesses from being split up into 8-bit accesses.
> >
> > The SDHCI specification states that the registers shall be accessable in
> > byte, word, and double word accesses.
>
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
I think we should always prefer accessing MMIO registers using their
native size, unless there are pressing reasons not to.
> Hi,
>
> Thanks for the contribution. The change seems good to me.
>
> Just curious, if the accesses are always slit into byte(8-bit), is there any
> issue or performance impact is encountered during your usage?
>
> It will be helpful to get more information on the purpose of the patch.
> Thanks.
>
> Best Regards,
> Hao Wu
>
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> > ---
> > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 25
> > ++++++++++++++++++++----
> > 1 file changed, 21 insertions(+), 4 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > index 5aec8c6..82f4493 100644
> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > @@ -152,19 +152,36 @@ SdMmcHcRwMmio (
> > )
> > {
> > EFI_STATUS Status;
> > + EFI_PCI_IO_PROTOCOL_WIDTH Width;
> >
> > if ((PciIo == NULL) || (Data == NULL)) {
> > return EFI_INVALID_PARAMETER;
> > }
> >
> > - if ((Count != 1) && (Count != 2) && (Count != 4) && (Count != 8)) {
> > - return EFI_INVALID_PARAMETER;
> > + switch (Count) {
> > + case 1:
> > + Width = EfiPciIoWidthUint8;
> > + break;
> > + case 2:
> > + Width = EfiPciIoWidthUint16;
> > + Count = 1;
> > + break;
> > + case 4:
> > + Width = EfiPciIoWidthUint32;
> > + Count = 1;
> > + break;
> > + case 8:
> > + Width = EfiPciIoWidthUint32;
> > + Count = 2;
> > + break;
> > + default:
> > + return EFI_INVALID_PARAMETER;
> > }
> >
> > if (Read) {
> > Status = PciIo->Mem.Read (
> > PciIo,
> > - EfiPciIoWidthUint8,
> > + Width,
> > BarIndex,
> > (UINT64) Offset,
> > Count,
> > @@ -173,7 +190,7 @@ SdMmcHcRwMmio (
> > } else {
> > Status = PciIo->Mem.Write (
> > PciIo,
> > - EfiPciIoWidthUint8,
> > + Width,
> > BarIndex,
> > (UINT64) Offset,
> > Count,
> > --
> > 2.7.4
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit IO widths
2019-02-01 17:52 ` Jeff Brasen
@ 2019-02-18 2:49 ` Wu, Hao A
0 siblings, 0 replies; 9+ messages in thread
From: Wu, Hao A @ 2019-02-18 2:49 UTC (permalink / raw)
To: Jeff Brasen, edk2-devel@lists.01.org; +Cc: Edgar Handal
Hello Jeff,
Sorry for the delayed response.
I have verified your patch with the board on my side and it is working
fine. And I saw around 3~4 milliseconds performance boost. So I think the
patch will bring performance benefit to the driver.
Some minor inline comments below:
From: Jeff Brasen [mailto:jbrasen@nvidia.com]
Sent: Saturday, February 02, 2019 1:53 AM
To: Wu, Hao A; edk2-devel@lists.01.org
Cc: Edgar Handal
Subject: Re: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit IO widths
________________________________
From: Wu, Hao A <hao.a.wu@intel.com>
Sent: Friday, February 1, 2019 12:54 AM
To: Jeff Brasen; edk2-devel@lists.01.org
Cc: Edgar Handal
Subject: RE: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit IO widths
> -----Original Message-----
> From: Jeff Brasen [mailto:jbrasen@nvidia.com]
> Sent: Friday, February 01, 2019 3:12 PM
> To: Wu, Hao A; edk2-devel@lists.01.org
> Cc: Edgar Handal
> Subject: RE: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit IO
> widths
>
>
>
> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Thursday, January 31, 2019 10:56 PM
> To: Jeff Brasen <jbrasen@nvidia.com>; edk2-devel@lists.01.org
> Cc: Edgar Handal <ehandal@nvidia.com>
> Subject: RE: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit IO
> widths
>
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> > Jeff Brasen
> > Sent: Thursday, January 31, 2019 7:59 AM
> > To: edk2-devel@lists.01.org
> > Cc: Edgar Handal; Jeff Brasen
> > Subject: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit IO
> > widths
> >
Could you help to file a Bugzilla tracker to state the purpose of this
patch and make a reference in the commit log message here? Thanks.
> > From: Edgar Handal <ehandal@nvidia.com>
> >
> > Use 16-bit and 32-bit IO widths for SDMMC MMIO to prevent all register
> > accesses from being split up into 8-bit accesses.
> >
> > The SDHCI specification states that the registers shall be accessable
> > in byte, word, and double word accesses.
'accessable' -> 'accessible'.
Also, could you help to quote the detailed context from the SD HCI spec
for the above statement?
Lastly, could you help to add your performance evaluation data in the
commit log message as well?
With the above 3 minor comments handled:
Reviewed-by: Hao Wu <hao.a.wu@intel.com>
Best Regards,
Hao Wu
>
> Hi,
>
> Thanks for the contribution. The change seems good to me.
>
> Just curious, if the accesses are always slit into byte(8-bit), is there any issue or
> performance impact is encountered during your usage?
>
> It will be helpful to get more information on the purpose of the patch.
> Thanks.
>
> Best Regards,
> Hao Wu
>
> [JMB] We were working with a simulation module that has some issues when
> accessing 16 or 32 bit registers by byte (This should be supported per the SDHCI
> specification.), and this patch resolves this while we work on getting the model
> fixed. This should also optimize performance as there will less read/write
> instructions (My guess is this is a marginal improvement as most of the SD
> access time would be DMA operations.)
Thanks Jeff,
Got it. May I know is it possible for you to collect some performance data
for the change?
[JMB] Sure, did a several boots on silicon platform and see an average boot time improvement of 15ms. With a range of 10-20ms and never saw the performance get worse.
Meanwhile, I will test this patch with the boards I own and try to collect
some data. Please grant me some time for this. Since the current
implementation does not violate the spec, let us evaluate whether better
performance will be brought.
Best Regards,
Hao Wu
>
> Thanks,
> Jeff
>
>
>
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> > ---
> > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 25
> > ++++++++++++++++++++----
> > 1 file changed, 21 insertions(+), 4 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > index 5aec8c6..82f4493 100644
> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > @@ -152,19 +152,36 @@ SdMmcHcRwMmio (
> > )
> > {
> > EFI_STATUS Status;
> > + EFI_PCI_IO_PROTOCOL_WIDTH Width;
> >
> > if ((PciIo == NULL) || (Data == NULL)) {
> > return EFI_INVALID_PARAMETER;
> > }
> >
> > - if ((Count != 1) && (Count != 2) && (Count != 4) && (Count != 8)) {
> > - return EFI_INVALID_PARAMETER;
> > + switch (Count) {
> > + case 1:
> > + Width = EfiPciIoWidthUint8;
> > + break;
> > + case 2:
> > + Width = EfiPciIoWidthUint16;
> > + Count = 1;
> > + break;
> > + case 4:
> > + Width = EfiPciIoWidthUint32;
> > + Count = 1;
> > + break;
> > + case 8:
> > + Width = EfiPciIoWidthUint32;
> > + Count = 2;
> > + break;
> > + default:
> > + return EFI_INVALID_PARAMETER;
> > }
> >
> > if (Read) {
> > Status = PciIo->Mem.Read (
> > PciIo,
> > - EfiPciIoWidthUint8,
> > + Width,
> > BarIndex,
> > (UINT64) Offset,
> > Count,
> > @@ -173,7 +190,7 @@ SdMmcHcRwMmio (
> > } else {
> > Status = PciIo->Mem.Write (
> > PciIo,
> > - EfiPciIoWidthUint8,
> > + Width,
> > BarIndex,
> > (UINT64) Offset,
> > Count,
> > --
> > 2.7.4
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> -----------------------------------------------------------------------------------
> This email message is for the sole use of the intended recipient(s) and may
> contain
> confidential information. Any unauthorized review, use, disclosure or
> distribution
> is prohibited. If you are not the intended recipient, please contact the sender
> by
> reply email and destroy all copies of the original message.
> -----------------------------------------------------------------------------------
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit IO widths
2019-02-03 12:39 ` Ard Biesheuvel
@ 2019-02-20 1:19 ` Wu, Hao A
2019-02-20 11:04 ` Ard Biesheuvel
0 siblings, 1 reply; 9+ messages in thread
From: Wu, Hao A @ 2019-02-20 1:19 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Jeff Brasen, edk2-devel@lists.01.org, Edgar Handal, Marcin Wojtas
> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Sunday, February 03, 2019 8:39 PM
> To: Wu, Hao A
> Cc: Jeff Brasen; edk2-devel@lists.01.org; Edgar Handal; Marcin Wojtas
> Subject: Re: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit
> IO widths
>
> On Fri, 1 Feb 2019 at 06:55, Wu, Hao A <hao.a.wu@intel.com> wrote:
> >
> > > -----Original Message-----
> > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Jeff
> > > Brasen
> > > Sent: Thursday, January 31, 2019 7:59 AM
> > > To: edk2-devel@lists.01.org
> > > Cc: Edgar Handal; Jeff Brasen
> > > Subject: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit
> IO
> > > widths
> > >
> > > From: Edgar Handal <ehandal@nvidia.com>
> > >
> > > Use 16-bit and 32-bit IO widths for SDMMC MMIO to prevent all register
> > > accesses from being split up into 8-bit accesses.
> > >
> > > The SDHCI specification states that the registers shall be accessable in
> > > byte, word, and double word accesses.
> >
>
> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Ard,
Really sorry for missing your 'Acked-by' tag.
It came to me after I pushed the commit.
Sorry again for this.
Best Regards,
Hao Wu
>
> I think we should always prefer accessing MMIO registers using their
> native size, unless there are pressing reasons not to.
>
>
> > Hi,
> >
> > Thanks for the contribution. The change seems good to me.
> >
> > Just curious, if the accesses are always slit into byte(8-bit), is there any
> > issue or performance impact is encountered during your usage?
> >
> > It will be helpful to get more information on the purpose of the patch.
> > Thanks.
> >
> > Best Regards,
> > Hao Wu
> >
> > >
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> > > ---
> > > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 25
> > > ++++++++++++++++++++----
> > > 1 file changed, 21 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > > index 5aec8c6..82f4493 100644
> > > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > > @@ -152,19 +152,36 @@ SdMmcHcRwMmio (
> > > )
> > > {
> > > EFI_STATUS Status;
> > > + EFI_PCI_IO_PROTOCOL_WIDTH Width;
> > >
> > > if ((PciIo == NULL) || (Data == NULL)) {
> > > return EFI_INVALID_PARAMETER;
> > > }
> > >
> > > - if ((Count != 1) && (Count != 2) && (Count != 4) && (Count != 8)) {
> > > - return EFI_INVALID_PARAMETER;
> > > + switch (Count) {
> > > + case 1:
> > > + Width = EfiPciIoWidthUint8;
> > > + break;
> > > + case 2:
> > > + Width = EfiPciIoWidthUint16;
> > > + Count = 1;
> > > + break;
> > > + case 4:
> > > + Width = EfiPciIoWidthUint32;
> > > + Count = 1;
> > > + break;
> > > + case 8:
> > > + Width = EfiPciIoWidthUint32;
> > > + Count = 2;
> > > + break;
> > > + default:
> > > + return EFI_INVALID_PARAMETER;
> > > }
> > >
> > > if (Read) {
> > > Status = PciIo->Mem.Read (
> > > PciIo,
> > > - EfiPciIoWidthUint8,
> > > + Width,
> > > BarIndex,
> > > (UINT64) Offset,
> > > Count,
> > > @@ -173,7 +190,7 @@ SdMmcHcRwMmio (
> > > } else {
> > > Status = PciIo->Mem.Write (
> > > PciIo,
> > > - EfiPciIoWidthUint8,
> > > + Width,
> > > BarIndex,
> > > (UINT64) Offset,
> > > Count,
> > > --
> > > 2.7.4
> > >
> > > _______________________________________________
> > > edk2-devel mailing list
> > > edk2-devel@lists.01.org
> > > https://lists.01.org/mailman/listinfo/edk2-devel
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit IO widths
2019-02-20 1:19 ` Wu, Hao A
@ 2019-02-20 11:04 ` Ard Biesheuvel
0 siblings, 0 replies; 9+ messages in thread
From: Ard Biesheuvel @ 2019-02-20 11:04 UTC (permalink / raw)
To: Wu, Hao A
Cc: Jeff Brasen, edk2-devel@lists.01.org, Edgar Handal, Marcin Wojtas
On Wed, 20 Feb 2019 at 02:19, Wu, Hao A <hao.a.wu@intel.com> wrote:
>
> > -----Original Message-----
> > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> > Sent: Sunday, February 03, 2019 8:39 PM
> > To: Wu, Hao A
> > Cc: Jeff Brasen; edk2-devel@lists.01.org; Edgar Handal; Marcin Wojtas
> > Subject: Re: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit
> > IO widths
> >
> > On Fri, 1 Feb 2019 at 06:55, Wu, Hao A <hao.a.wu@intel.com> wrote:
> > >
> > > > -----Original Message-----
> > > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> > Jeff
> > > > Brasen
> > > > Sent: Thursday, January 31, 2019 7:59 AM
> > > > To: edk2-devel@lists.01.org
> > > > Cc: Edgar Handal; Jeff Brasen
> > > > Subject: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit
> > IO
> > > > widths
> > > >
> > > > From: Edgar Handal <ehandal@nvidia.com>
> > > >
> > > > Use 16-bit and 32-bit IO widths for SDMMC MMIO to prevent all register
> > > > accesses from being split up into 8-bit accesses.
> > > >
> > > > The SDHCI specification states that the registers shall be accessable in
> > > > byte, word, and double word accesses.
> > >
> >
> > Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> Ard,
>
> Really sorry for missing your 'Acked-by' tag.
> It came to me after I pushed the commit.
>
> Sorry again for this.
>
No worries
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-02-20 11:05 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-01-30 23:58 [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit IO widths Jeff Brasen
2019-02-01 5:55 ` Wu, Hao A
2019-02-01 7:12 ` Jeff Brasen
2019-02-01 7:54 ` Wu, Hao A
2019-02-01 17:52 ` Jeff Brasen
2019-02-18 2:49 ` Wu, Hao A
2019-02-03 12:39 ` Ard Biesheuvel
2019-02-20 1:19 ` Wu, Hao A
2019-02-20 11:04 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox