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

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

> -----Original Message-----
> From: Loh, Tien Hock
> Sent: Thursday, April 25, 2019 2:13 PM
> To: thloh85@gmail.com devel@edk2.groups.io; Kinney, Michael D
> 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
> 
> 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>
> 
> ---
> 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
> ---
>  .../Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c  | 12
> +++++++++++-
>  .../BaseSerialPortLib16550/BaseSerialPortLib16550.inf        |  3 ++-
>  MdeModulePkg/MdeModulePkg.dec                                |  7 +++++++
>  3 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git
> a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c
> b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c
> index 34df34d..5910606 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
> @@ -77,6 +77,11 @@ SerialPortReadRegister (
>    )
>  {
>    if (PcdGetBool (PcdSerialUseMmio)) {
> +    if (PcdGet8 (PcdSerialRegisterAccessWidth) == 8) {
> +      return MmioRead8 (Base + Offset * PcdGet32 (PcdSerialRegisterStride));
> +    } else if (PcdGet8 (PcdSerialRegisterAccessWidth) == 32) {
> +      return (UINT8) MmioRead32 (Base + Offset * PcdGet32
> (PcdSerialRegisterStride));
> +    }
>      return MmioRead8 (Base + Offset * PcdGet32 (PcdSerialRegisterStride));

Hello Tien Hock,

Sorry for the delayed response.

The above codes deal with 3 cases (also true for SerialPortWriteRegister):

1. PcdSerialRegisterAccessWidth is set to 8
2. PcdSerialRegisterAccessWidth is set to 32
3. PcdSerialRegisterAccessWidth is set to values other than 8 or 32
(For 3, the behavior falls back to default, which is okay to me)

For case 1 and 3, they end up calling MmioRead8(). So IMO, maybe we can
simplifies the logic to:

  if (PcdGet8 (PcdSerialRegisterAccessWidth) == 32) {
    MmioRead32 (...);
  } else {
    MmioRead8 (...);
  }

What do you think?


Also, could you help to update the function description comments for

* SerialPortReadRegister()
* SerialPortWriteRegister()

to reflect their behavior with regard to the new PCD?

Lastly, when sending a new version of the patch, could you help to append
option '--subject-prefix="PATCH vN"' to the 'git-format-patch' command. So
for this v4 patch, it will be '--subject-prefix="PATCH v4"'.


Best Regards,
Hao Wu

>    } else {
>      return IoRead8 (Base + Offset * PcdGet32 (PcdSerialRegisterStride));
> @@ -104,6 +109,11 @@ SerialPortWriteRegister (
>    )
>  {
>    if (PcdGetBool (PcdSerialUseMmio)) {
> +    if (PcdGet8 (PcdSerialRegisterAccessWidth) == 8) {
> +      return MmioWrite8 (Base + Offset * PcdGet32 (PcdSerialRegisterStride),
> Value);
> +    } else 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