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

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Wu,
> Hao A
> Sent: Friday, April 26, 2019 4:35 PM
> To: Loh, Tien Hock; devel@edk2.groups.io; thloh85@gmail.com
> Cc: Wang, Jian J
> Subject: Re: [edk2-devel] [PATCH v5 1/1] MdeModulePkg:
> BaseSerialPortLib16550: Add Mmio32 support
> 
> > -----Original Message-----
> > From: Loh, Tien Hock
> > Sent: Friday, April 26, 2019 2:27 PM
> > To: devel@edk2.groups.io; thloh85@gmail.com
> > Cc: Loh, Tien Hock; Wang, Jian J; Wu, Hao A
> > Subject: [PATCH v5 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
> >
> > 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>
> >
> > ---
> > v5
> > - Updates function header comments
> > - Change the implementation to eliminate unnecessary else clause
> 
> Looks good to me,
> Reviewed-by: Hao Wu <hao.a.wu@intel.com>
> 
> I will wait some time before pushing to see if there are additional
> comments from others.

Thanks for the contribution.
Push via commit 8a472b1915.

Best Regards,
Hao Wu

> 
> Best Regards,
> Hao Wu
> 
> > v4
> > - Updates Pcd name to a better name: PcdSerialRegisterAccessWidth
> > v3
> > - Updates the Pcd to be UINT8 to allow more options such as 16 bits access
> > in the future
> > - Updated copyright date
> > v2
> > - Updates the Pcd name to PcdSerialMmio32BitAccess and access 32 bits
> > register if PcdSerialUseMmio and PcdSerialMmio32BitAccess is set
> > ---
> >  .../BaseSerialPortLib16550/BaseSerialPortLib16550.c        | 14 +++++++++++-
> --
> >  .../BaseSerialPortLib16550/BaseSerialPortLib16550.inf      |  3 ++-
> >  MdeModulePkg/MdeModulePkg.dec                              |  7 +++++++
> >  3 files changed, 20 insertions(+), 4 deletions(-)
> >
> > diff --git
> > a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c
> > b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c
> > index 34df34d..bbae379 100644
> > ---
> a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c
> > +++
> > b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c
> > @@ -2,7 +2,7 @@
> >    16550 UART Serial Port library functions
> >
> >    (C) Copyright 2014 Hewlett-Packard Development Company, L.P.<BR>
> > -  Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> > +  Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
> >    Copyright (c) 2018, AMD Incorporated. All rights reserved.<BR>
> >
> >    SPDX-License-Identifier: BSD-2-Clause-Patent
> > @@ -62,7 +62,8 @@ typedef struct {
> >    Read an 8-bit 16550 register.  If PcdSerialUseMmio is TRUE, then the value
> is
> > read from
> >    MMIO space.  If PcdSerialUseMmio is FALSE, then the value is read from
> I/O
> > space.  The
> >    parameter Offset is added to the base address of the 16550 registers that is
> > specified
> > -  by PcdSerialRegisterBase.
> > +  by PcdSerialRegisterBase. PcdSerialRegisterAccessWidth specifies the
> MMIO
> > space access
> > +  width and defaults to 8 bit access, and supports 8 or 32 bit access.
> >
> >    @param  Base    The base address register of UART device.
> >    @param  Offset  The offset of the 16550 register to read.
> > @@ -77,6 +78,9 @@ SerialPortReadRegister (
> >    )
> >  {
> >    if (PcdGetBool (PcdSerialUseMmio)) {
> > +    if (PcdGet8 (PcdSerialRegisterAccessWidth) == 32) {
> > +      return (UINT8) MmioRead32 (Base + Offset * PcdGet32
> > (PcdSerialRegisterStride));
> > +    }
> >      return MmioRead8 (Base + Offset * PcdGet32 (PcdSerialRegisterStride));
> >    } else {
> >      return IoRead8 (Base + Offset * PcdGet32 (PcdSerialRegisterStride));
> > @@ -87,7 +91,8 @@ SerialPortReadRegister (
> >    Write an 8-bit 16550 register.  If PcdSerialUseMmio is TRUE, then the value
> is
> > written to
> >    MMIO space.  If PcdSerialUseMmio is FALSE, then the value is written to
> I/O
> > space.  The
> >    parameter Offset is added to the base address of the 16550 registers that is
> > specified
> > -  by PcdSerialRegisterBase.
> > +  by PcdSerialRegisterBase. PcdSerialRegisterAccessWidth specifies the
> MMIO
> > space access
> > +  width and defaults to 8 bit access, and supports 8 or 32 bit access.
> >
> >    @param  Base    The base address register of UART device.
> >    @param  Offset  The offset of the 16550 register to write.
> > @@ -104,6 +109,9 @@ SerialPortWriteRegister (
> >    )
> >  {
> >    if (PcdGetBool (PcdSerialUseMmio)) {
> > +    if (PcdGet8 (PcdSerialRegisterAccessWidth) == 32) {
> > +      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/BaseSerialPortLib16550.inf
> >
> b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
> > index b60779c..8b4ae3f 100644
> > ---
> >
> a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
> > +++
> >
> b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
> > @@ -1,7 +1,7 @@
> >  ## @file
> >  #  SerialPortLib instance for 16550 UART.
> >  #
> > -#  Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> > +#  Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
> >  #  SPDX-License-Identifier: BSD-2-Clause-Patent
> >  #
> >  ##
> > @@ -29,6 +29,7 @@
> >    BaseSerialPortLib16550.c
> >
> >  [Pcd]
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterAccessWidth     ##
> > SOMETIMES_CONSUMES
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio                 ##
> > CONSUMES
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseHardwareFlowControl  ##
> > CONSUMES
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdSerialDetectCable             ##
> > SOMETIMES_CONSUMES
> > diff --git a/MdeModulePkg/MdeModulePkg.dec
> > b/MdeModulePkg/MdeModulePkg.dec
> > index be84916..2ef48f2 100644
> > --- a/MdeModulePkg/MdeModulePkg.dec
> > +++ b/MdeModulePkg/MdeModulePkg.dec
> > @@ -1170,6 +1170,13 @@
> >    # @Prompt Serial port registers use MMIO.
> >
> >
> gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio|FALSE|BOOLEAN|0x00
> > 020000
> >
> > +  ## 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|0x
> > 00020007
> > +
> >    ## 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>
> >    #   FALSE - 16550 serial port hardware flow control will be disabled.<BR>
> > --
> > 2.2.2
> 
> 
>