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

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

Hi,

Can we change the name of the access size PCD so it
does not use Mmio in the name?

I am ok with not implementing support for different
access widths for I/O, but it would be good if the PCD
name and description is for the access width so it 
could potentially be used for I/O in the future if 
there is a need.  The default access width can be 8-bits
so it is a compatible change.

How about the following name and description in the DEC file?

  ## Indicates the access width for 16550 serial port registers.
  # Default is 8-bit access mode.<BR><BR>
  #   8  - 16550 serial port registers are accessed in 8-bit width.<BR>
  #   32 - 16550 serial port registers are accessed in 32-bit width.<BR>
  # @Prompt Serial port register access width.
  gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterAccessWidth|8|UINT8|0x00020007

Thanks,

Mike

> -----Original Message-----
> From: Loh, Tien Hock
> Sent: Tuesday, April 23, 2019 11:58 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; Wu,
> Hao A <hao.a.wu@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
> 
> 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
> > >
> > >
> > >
> >
> >
> > 
> >