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

["Loh, Tien Hock" <tien.hock.loh@intel.com>]

> -----Original Message-----
> From: Wu, Hao A
> Sent: Friday, April 26, 2019 1:36 PM
> To: thloh85@gmail.com; devel@edk2.groups.io; Loh, Tien Hock
> <tien.hock.loh@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>
> Subject: RE: [PATCH 1/1] MdeModulePkg: BaseSerialPortLib16550: Add
> Mmio32 support
> 
> > -----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.
No problem, thanks for reviewing the patch.

> 
> 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?
> 
I'm OK either way, I'll make the changes.
> 
> Also, could you help to update the function description comments for
> 
> * SerialPortReadRegister()
> * SerialPortWriteRegister()
> 
> to reflect their behavior with regard to the new PCD?
OK noted.

> 
> 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"'.
OK noted.
> 
> 
> 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.i
> > nf
> >
> b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.i
> > nf
> > index b60779c..8b4ae3f 100644
> > ---
> > a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.i
> > nf
> > +++
> >
> b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.i
> > nf
> > @@ -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|0x
> 00
> > 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