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

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

> -----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/BaseSerialPortLib16550.c
> b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c
> index b242b23..f90fb55 100644
> --- a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c
> +++
> b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.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/BaseSerialPortLib16550.inf
> b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
> index 575728a..c03d90d 100644
> ---
> a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
> +++
> b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
> @@ -29,7 +29,7 @@
>    BaseSerialPortLib16550.c
> 
>  [Pcd]
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio32               ##
> CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdSerialMmio32BitAccess         ##
> CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio                 ##
> CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseHardwareFlowControl  ##
> 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|BOOLEAN|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|BOOLEAN|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|FALSE|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