Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: BaseSerialPortLib16550: Add Mmio32 support

[tien.hock.loh@intel.com]

Hi Mike, Hao,

Replies inlined.

On Wed, 2019-04-24 at 03:45 +0000, Michael D Kinney wrote:
> One issue I see is using a FeatureFlag PCD.
> These PCDs can only be TRUE or FALSE, so they can not be
> extended later.  A FixedAtBuild PCD of type BOOL has the 
> same issue.
> 
> It would be better to use a UINTx FixedAtBuild PCD, and 
> define a bit or a value for 32-bit access.  That way, if
> there is a device that requires 16-bit access, it can be
> added in the same PCD.
> 
OK noted. I'll update with UINT8, and set 0x0 to default 8bits and 0x1
to 32 bits access

> Also, should the new PCD be limited to MMIO?  It could be
> an access width PCD that could be applied to I/O or MMIO
> Access.
> 
I'm not sure if this should affect the I/O. I would separate the I/O
access with MMIO access with different PCD, I'll skip this for now as I
don't have platform that uses I/O space, thus I'm unable to test the
changes. 

> Mike
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io]
> > On Behalf Of Wu, Hao A
> > Sent: Tuesday, April 23, 2019 8:20 PM
> > To: Loh, Tien Hock <tien.hock.loh@intel.com>;
> > devel@edk2.groups.io; thloh85@gmail.com
> > Cc: Wang, Jian J <jian.j.wang@intel.com>
> > Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg:
> > BaseSerialPortLib16550: Add Mmio32 support
> > 
> > > -----Original Message-----
> > > From: Loh, Tien Hock
> > > Sent: Wednesday, April 24, 2019 11:05 AM
> > > To: devel@edk2.groups.io; thloh85@gmail.com
> > > Cc: Loh, Tien Hock; Wang, Jian J; Wu, Hao A
> > > Subject: [PATCH 1/1] MdeModulePkg:
> > 
> > BaseSerialPortLib16550: Add Mmio32
> > > support
> > > 
> > > From: "Tien Hock, Loh" <tien.hock.loh@intel.com>
> > > 
> > > Some busses doesn't allow 8 bit MMIO read/write, this
> > 
> > adds support for
> > > 32 bits read/write
> > 
> > Hello Tien Hock,
> > 
> > Your V2 patch seems to be based on your V1 patch.
> > 
> > Could you help to update the V2 patch to base on the tip
> > of the edk2
> > master branch? Thanks.
> > 
> > One easy way to do this is to squash the 2 commits into
> > one.
> > 
Yes sorry I wasn't aware I'm making patch on top of a patch. 
> > 
> > Some minor comments:
> > A. Please help to update the copyright year for the
> > files:
> >    BaseSerialPortLib16550.c
> >    BaseSerialPortLib16550.inf
> > 
OK noted. I'll update the copyright year.

> > B. For BaseSerialPortLib16550.inf, maybe:
> > 
> > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialMmio32BitAccess
> > ## SOMETIMES_CONSUMES
> > 
OK noted.

> > is more appropriate here.
> > 
> > Best Regards,
> > Hao Wu
> > 
> > > 
> > > Signed-off-by: "Tien Hock, Loh"
> > 
> > <tien.hock.loh@intel.com>
> > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > Cc: Hao Wu <hao.a.wu@intel.com>
> > > 
> > > --
> > > v2:
> > > - Updates the Pcd name to PcdSerialMmio32BitAccess and
> > 
> > access 32 bits
> > > register if PcdSerialUseMmio and
> > 
> > PcdSerialMmio32BitAccess is set
> > > ---
> > >  .../BaseSerialPortLib16550/BaseSerialPortLib16550.c
> > > 16 ++++++++--------
> > >  .../BaseSerialPortLib16550/BaseSerialPortLib16550.inf
> > >  2 +-
> > >  MdeModulePkg/MdeModulePkg.dec
> > > 12 +++++++-----
> > >  3 files changed, 16 insertions(+), 14 deletions(-)
> > > 
> > > diff --git
> > > 
> > 
> > a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialP
> > ortLib16550.c
> > > 
> > 
> > b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialP
> > ortLib16550.c
> > > index b242b23..f90fb55 100644
> > > ---
> > 
> > a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialP
> > ortLib16550.c
> > > +++
> > > 
> > 
> > b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialP
> > ortLib16550.c
> > > @@ -76,10 +76,10 @@ SerialPortReadRegister (
> > >    UINTN  Offset
> > >    )
> > >  {
> > > -  if (PcdGetBool (PcdSerialUseMmio32)) {
> > > -    return (UINT8) MmioRead32 (Base + Offset *
> > 
> > PcdGet32
> > > (PcdSerialRegisterStride));
> > > -  }
> > > -  else if (PcdGetBool (PcdSerialUseMmio)) {
> > > +  if (PcdGetBool (PcdSerialUseMmio)) {
> > > +    if (PcdGetBool (PcdSerialMmio32BitAccess)) {
> > > +      return (UINT8) MmioRead32 (Base + Offset *
> > 
> > PcdGet32
> > > (PcdSerialRegisterStride));
> > > +    }
> > >      return MmioRead8 (Base + Offset * PcdGet32
> > 
> > (PcdSerialRegisterStride));
> > >    } else {
> > >      return IoRead8 (Base + Offset * PcdGet32
> > 
> > (PcdSerialRegisterStride));
> > > @@ -106,10 +106,10 @@ SerialPortWriteRegister (
> > >    UINT8  Value
> > >    )
> > >  {
> > > -  if (PcdGetBool (PcdSerialUseMmio32)) {
> > > -    return MmioWrite32 (Base + Offset * PcdGet32
> > 
> > (PcdSerialRegisterStride),
> > > (UINT8)Value);
> > > -  }
> > > -  else if (PcdGetBool (PcdSerialUseMmio)) {
> > > +  if (PcdGetBool (PcdSerialUseMmio)) {
> > > +    if (PcdGetBool (PcdSerialMmio32BitAccess)) {
> > > +      return (UINT8) MmioWrite32 (Base + Offset *
> > 
> > PcdGet32
> > > (PcdSerialRegisterStride), (UINT8)Value);
> > > +    }
> > >      return MmioWrite8 (Base + Offset * PcdGet32
> > 
> > (PcdSerialRegisterStride),
> > > Value);
> > >    } else {
> > >      return IoWrite8 (Base + Offset * PcdGet32
> > 
> > (PcdSerialRegisterStride), Value);
> > > diff --git
> > > 
> > 
> > a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialP
> > ortLib16550.inf
> > > 
> > 
> > b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialP
> > ortLib16550.inf
> > > index 575728a..c03d90d 100644
> > > ---
> > > 
> > 
> > a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialP
> > ortLib16550.inf
> > > +++
> > > 
> > 
> > b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialP
> > ortLib16550.inf
> > > @@ -29,7 +29,7 @@
> > >    BaseSerialPortLib16550.c
> > > 
> > >  [Pcd]
> > > -  gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio32
> > 
> > ##
> > > CONSUMES
> > > +
> > 
> > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialMmio32BitAccess
> > ##
> > > CONSUMES
> > >    gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio
> > 
> > ##
> > > CONSUMES
> > > 
> > 
> > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseHardwareFlowCo
> > ntrol  ##
> > > CONSUMES
> > >    gEfiMdeModulePkgTokenSpaceGuid.PcdSerialDetectCable
> > 
> > ##
> > > SOMETIMES_CONSUMES
> > > diff --git a/MdeModulePkg/MdeModulePkg.dec
> > > b/MdeModulePkg/MdeModulePkg.dec
> > > index 4e53625..f868850 100644
> > > --- a/MdeModulePkg/MdeModulePkg.dec
> > > +++ b/MdeModulePkg/MdeModulePkg.dec
> > > @@ -1170,11 +1170,13 @@
> > >    # @Prompt Serial port registers use MMIO.
> > > 
> > > 
> > 
> > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio|FALSE|BOO
> > LEAN|0x00
> > > 020000
> > > 
> > > -  ## Indicates the 16550 serial port registers are in
> > 
> > MMIO 32 bit space, or in
> > > I/O space. Default is I/O space.<BR><BR>
> > > -  #   TRUE  - 16550 serial port registers are in MMIO
> > 
> > 32 bit space.<BR>
> > > -  #   FALSE - 16550 serial port registers are in I/O
> > 
> > space.<BR>
> > > -  # @Prompt Serial port registers use MMIO.
> > > -
> > > 
> > 
> > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio32|FALSE|B
> > OOLEAN|0x
> > > 00020007
> > > +  ## Indicates the access mode for 16550 serial port
> > 
> > registers when they are in
> > > MMIO space.
> > > +  # The PCD is only valid if PcdSerialUseMmio is set
> > 
> > to TRUE.
> > > +  # Default is 8-bit access mode.<BR><BR>
> > > +  #   TRUE  - 16550 serial port MMIO registers are
> > 
> > accessed in 32-bit
> > > width.<BR>
> > > +  #   FALSE - 16550 serial port MMIO registers are
> > 
> > accessed in 8-bit width.<BR>
> > > +  # @Prompt Serial port MMIO registers access mode.
> > > +
> > > 
> > 
> > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialMmio32BitAccess|F
> > ALSE|BOOLE
> > > AN|0x00020007
> > > 
> > >    ## Indicates if the 16550 serial port hardware flow
> > 
> > control will be enabled.
> > > Default is FALSE.<BR><BR>
> > >    #   TRUE  - 16550 serial port hardware flow control
> > 
> > will be enabled.<BR>
> > > --
> > > 2.2.2
> > 
> > 
> > 
> 
> 
> 
>