public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Wu, Hao A" <hao.a.wu@intel.com>
To: Jeff Brasen <jbrasen@nvidia.com>,
	"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: Edgar Handal <ehandal@nvidia.com>
Subject: Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit IO widths
Date: Fri, 1 Feb 2019 07:54:08 +0000	[thread overview]
Message-ID: <B80AF82E9BFB8E4FBD8C89DA810C6A093C894549@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <CY4PR12MB11432ED9D447ECA6A5284889CB920@CY4PR12MB1143.namprd12.prod.outlook.com>

> -----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.
> -----------------------------------------------------------------------------------


  reply	other threads:[~2019-02-01  7:54 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=B80AF82E9BFB8E4FBD8C89DA810C6A093C894549@SHSMSX104.ccr.corp.intel.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox