* [PATCH 1/1] MdeModulePkg: BaseSerialPortLib16550: Add Mmio32 support
@ 2019-04-24 3:08 tien.hock.loh
0 siblings, 0 replies; 5+ messages in thread
From: tien.hock.loh @ 2019-04-24 3:08 UTC (permalink / raw)
To: devel, thloh85; +Cc: Tien Hock, Loh, Jian J Wang, Hao Wu
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>
--
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|0x00020000
- ## 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|0x00020007
+ ## 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|BOOLEAN|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 related [flat|nested] 5+ messages in thread
* 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
0 siblings, 0 replies; 5+ 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] 5+ messages in thread
* [PATCH 1/1] MdeModulePkg: BaseSerialPortLib16550: Add Mmio32 support
@ 2019-04-24 8:31 Loh, Tien Hock
0 siblings, 0 replies; 5+ messages in thread
From: Loh, Tien Hock @ 2019-04-24 8:31 UTC (permalink / raw)
To: devel, thloh85; +Cc: Tien Hock, Loh, Jian J Wang, Hao Wu
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>
--
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 | 8 ++++++++
3 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c
index 34df34d..3a811c9 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 (PcdSerialMmioAccessBits) == 8) {
+ return MmioRead8 (Base + Offset * PcdGet32 (PcdSerialRegisterStride));
+ } else if (PcdGet8 (PcdSerialMmioAccessBits) == 32) {
+ return (UINT8) MmioRead32 (Base + Offset * PcdGet32 (PcdSerialRegisterStride));
+ }
return MmioRead8 (Base + Offset * PcdGet32 (PcdSerialRegisterStride));
} else {
return IoRead8 (Base + Offset * PcdGet32 (PcdSerialRegisterStride));
@@ -104,6 +109,11 @@ SerialPortWriteRegister (
)
{
if (PcdGetBool (PcdSerialUseMmio)) {
+ if (PcdGet8 (PcdSerialMmioAccessBits) == 8) {
+ return MmioWrite8 (Base + Offset * PcdGet32 (PcdSerialRegisterStride), Value);
+ } else if (PcdGet8 (PcdSerialMmioAccessBits) == 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..d51921f 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.PcdSerialMmioAccessBits ## SOMETIMES_CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseHardwareFlowControl ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialDetectCable ## SOMETIMES_CONSUMES
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index be84916..b1813a6 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -1170,6 +1170,14 @@
# @Prompt Serial port registers use MMIO.
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio|FALSE|BOOLEAN|0x00020000
+ ## 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>
+ # 8 - 16550 serial port MMIO registers are accessed in 8-bit width.<BR>
+ # 32 - 16550 serial port MMIO registers are accessed in 32-bit width.<BR>
+ # @Prompt Serial port MMIO registers access mode.
+ gEfiMdeModulePkgTokenSpaceGuid.PcdSerialMmioAccessBits|32|UINT8|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>
# FALSE - 16550 serial port hardware flow control will be disabled.<BR>
--
2.2.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] MdeModulePkg: BaseSerialPortLib16550: Add Mmio32 support
[not found] <1556172774-134132-1-git-send-email-tien.hock.loh@intel.com>
@ 2019-04-26 5:36 ` Wu, Hao A
2019-04-26 6:01 ` Loh, Tien Hock
0 siblings, 1 reply; 5+ messages in thread
From: Wu, Hao A @ 2019-04-26 5:36 UTC (permalink / raw)
To: thloh85@gmail.com, devel@edk2.groups.io, Loh, Tien Hock,
Kinney, Michael D
Cc: Wang, Jian J
> -----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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] MdeModulePkg: BaseSerialPortLib16550: Add Mmio32 support
2019-04-26 5:36 ` Wu, Hao A
@ 2019-04-26 6:01 ` Loh, Tien Hock
0 siblings, 0 replies; 5+ messages in thread
From: Loh, Tien Hock @ 2019-04-26 6:01 UTC (permalink / raw)
To: Wu, Hao A, thloh85@gmail.com, devel@edk2.groups.io,
Kinney, Michael D
Cc: Wang, Jian J
> -----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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-04-26 6:02 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-04-24 8:31 [PATCH 1/1] MdeModulePkg: BaseSerialPortLib16550: Add Mmio32 support Loh, Tien Hock
[not found] <1556172774-134132-1-git-send-email-tien.hock.loh@intel.com>
2019-04-26 5:36 ` Wu, Hao A
2019-04-26 6:01 ` Loh, Tien Hock
[not found] <1556075112-185791-1-git-send-email-tien.hock.loh@intel.com>
2019-04-24 3:19 ` Wu, Hao A
-- strict thread matches above, loose matches on Subject: below --
2019-04-24 3:08 tien.hock.loh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox