Hi Mike, Hao, Replies inlined.
On Wed, 2019-04-24 at 03:45 +0000, Michael D Kinney wrote: > One issue I see is using a FeatureFlag PCD. > These PCDs can only be TRUE or FALSE, so they can not be > extended later. A FixedAtBuild PCD of type BOOL has the > same issue. > > It would be better to use a UINTx FixedAtBuild PCD, and > define a bit or a value for 32-bit access. That way, if > there is a device that requires 16-bit access, it can be > added in the same PCD. > OK noted. I'll update with UINT8, and set 0x0 to default 8bits and 0x1 to 32 bits access > Also, should the new PCD be limited to MMIO? It could be > an access width PCD that could be applied to I/O or MMIO > Access. > I'm not sure if this should affect the I/O. I would separate the I/O access with MMIO access with different PCD, I'll skip this for now as I don't have platform that uses I/O space, thus I'm unable to test the changes. > Mike > > > -----Original Message----- > > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] > > On Behalf Of Wu, Hao A > > Sent: Tuesday, April 23, 2019 8:20 PM > > To: Loh, Tien Hock <tien.hock....@intel.com>; > > devel@edk2.groups.io; thlo...@gmail.com > > Cc: Wang, Jian J <jian.j.w...@intel.com> > > Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: > > BaseSerialPortLib16550: Add Mmio32 support > > > > > -----Original Message----- > > > From: Loh, Tien Hock > > > Sent: Wednesday, April 24, 2019 11:05 AM > > > To: devel@edk2.groups.io; thlo...@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....@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. > > Yes sorry I wasn't aware I'm making patch on top of a patch. > > > > Some minor comments: > > A. Please help to update the copyright year for the > > files: > > BaseSerialPortLib16550.c > > BaseSerialPortLib16550.inf > > OK noted. I'll update the copyright year. > > B. For BaseSerialPortLib16550.inf, maybe: > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialMmio32BitAccess > > ## SOMETIMES_CONSUMES > > OK noted. > > is more appropriate here. > > > > Best Regards, > > Hao Wu > > > > > > > > Signed-off-by: "Tien Hock, Loh" > > > > <tien.hock....@intel.com> > > > Cc: Jian J Wang <jian.j.w...@intel.com> > > > Cc: Hao Wu <hao.a...@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/BaseSerialP > > ortLib16550.c > > > > > > > b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialP > > ortLib16550.c > > > index b242b23..f90fb55 100644 > > > --- > > > > a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialP > > ortLib16550.c > > > +++ > > > > > > > b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialP > > ortLib16550.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/BaseSerialP > > ortLib16550.inf > > > > > > > b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialP > > ortLib16550.inf > > > index 575728a..c03d90d 100644 > > > --- > > > > > > > a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialP > > ortLib16550.inf > > > +++ > > > > > > > b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialP > > ortLib16550.inf > > > @@ -29,7 +29,7 @@ > > > BaseSerialPortLib16550.c > > > > > > [Pcd] > > > - gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio32 > > > > ## > > > CONSUMES > > > + > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialMmio32BitAccess > > ## > > > CONSUMES > > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio > > > > ## > > > CONSUMES > > > > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseHardwareFlowCo > > ntrol ## > > > 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|BOO > > LEAN|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|B > > OOLEAN|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|F > > ALSE|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 > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#39466): https://edk2.groups.io/g/devel/message/39466 Mute This Topic: https://groups.io/mt/31318235/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
