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

["Michael D Kinney" <michael.d.kinney@intel.com>]

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.

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.

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.
> 
> 
> Some minor comments:
> A. Please help to update the copyright year for the
> files:
>    BaseSerialPortLib16550.c
>    BaseSerialPortLib16550.inf
> 
> B. For BaseSerialPortLib16550.inf, maybe:
> 
> gEfiMdeModulePkgTokenSpaceGuid.PcdSerialMmio32BitAccess
> ## SOMETIMES_CONSUMES
> 
> 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
> 
> 
>