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

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

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

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