Hi, Can we change the name of the access size PCD so it does not use Mmio in the name?
I am ok with not implementing support for different access widths for I/O, but it would be good if the PCD name and description is for the access width so it could potentially be used for I/O in the future if there is a need. The default access width can be 8-bits so it is a compatible change. How about the following name and description in the DEC file? ## 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|0x00020007 Thanks, Mike > -----Original Message----- > From: Loh, Tien Hock > Sent: Tuesday, April 23, 2019 11:58 PM > To: Kinney, Michael D <michael.d.kin...@intel.com>; Wu, > Hao A <hao.a...@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 > > 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 (#39512): https://edk2.groups.io/g/devel/message/39512 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] -=-=-=-=-=-=-=-=-=-=-=-
