* Re: [PATCH 1/1] MdeModulePkg: BaseSerialPortLib16550: Add Mmio32 support
[not found] <1556075112-185791-1-git-send-email-tien.hock.loh@intel.com>
@ 2019-04-24 3:19 ` Wu, Hao A
2019-04-24 3:45 ` [edk2-devel] " Michael D Kinney
0 siblings, 1 reply; 4+ messages in thread
From: Wu, Hao A @ 2019-04-24 3:19 UTC (permalink / raw)
To: Loh, Tien Hock, devel@edk2.groups.io, thloh85@gmail.com; +Cc: Wang, Jian J
> -----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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: BaseSerialPortLib16550: Add Mmio32 support
2019-04-24 3:19 ` [PATCH 1/1] MdeModulePkg: BaseSerialPortLib16550: Add Mmio32 support Wu, Hao A
@ 2019-04-24 3:45 ` Michael D Kinney
2019-04-24 6:58 ` tien.hock.loh
0 siblings, 1 reply; 4+ messages in thread
From: Michael D Kinney @ 2019-04-24 3:45 UTC (permalink / raw)
To: devel@edk2.groups.io, Wu, Hao A, Loh, Tien Hock,
thloh85@gmail.com, Kinney, Michael D
Cc: Wang, Jian J
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.
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.
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.loh@intel.com>;
> devel@edk2.groups.io; thloh85@gmail.com
> Cc: Wang, Jian J <jian.j.wang@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; 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/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
>
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: BaseSerialPortLib16550: Add Mmio32 support
2019-04-24 3:45 ` [edk2-devel] " Michael D Kinney
@ 2019-04-24 6:58 ` tien.hock.loh
2019-04-24 16:50 ` Michael D Kinney
0 siblings, 1 reply; 4+ messages in thread
From: tien.hock.loh @ 2019-04-24 6:58 UTC (permalink / raw)
To: Kinney, Michael D, Wu, Hao A, devel@edk2.groups.io,
thloh85@gmail.com
Cc: Wang, Jian J
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.loh@intel.com>;
> > devel@edk2.groups.io; thloh85@gmail.com
> > Cc: Wang, Jian J <jian.j.wang@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; 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.
> >
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.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/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
> >
> >
> >
>
>
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: BaseSerialPortLib16550: Add Mmio32 support
2019-04-24 6:58 ` tien.hock.loh
@ 2019-04-24 16:50 ` Michael D Kinney
0 siblings, 0 replies; 4+ messages in thread
From: Michael D Kinney @ 2019-04-24 16:50 UTC (permalink / raw)
To: Loh, Tien Hock, Wu, Hao A, devel@edk2.groups.io,
thloh85@gmail.com, Kinney, Michael D
Cc: Wang, Jian J
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.kinney@intel.com>; Wu,
> Hao A <hao.a.wu@intel.com>; devel@edk2.groups.io;
> thloh85@gmail.com
> Cc: Wang, Jian J <jian.j.wang@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.loh@intel.com>;
> > > devel@edk2.groups.io; thloh85@gmail.com
> > > Cc: Wang, Jian J <jian.j.wang@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; 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.
> > >
> 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.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/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
> > >
> > >
> > >
> >
> >
> >
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-04-24 16:50 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1556075112-185791-1-git-send-email-tien.hock.loh@intel.com>
2019-04-24 3:19 ` [PATCH 1/1] MdeModulePkg: BaseSerialPortLib16550: Add Mmio32 support Wu, Hao A
2019-04-24 3:45 ` [edk2-devel] " Michael D Kinney
2019-04-24 6:58 ` tien.hock.loh
2019-04-24 16:50 ` Michael D Kinney
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox