* [PATCH 0/1] UART Dynamic clock freq Support
@ 2020-02-19 13:31 Pankaj Bansal
2020-02-19 13:31 ` [PATCH 1/1] MdeModulePkg: " Pankaj Bansal
0 siblings, 1 reply; 18+ messages in thread
From: Pankaj Bansal @ 2020-02-19 13:31 UTC (permalink / raw)
To: Jian J Wang, Hao A Wu, Ray Ni, Maurice Ma, Guo Dong, Benjamin You,
Leif Lindholm, Meenakshi Aggarwal, Varun Sethi
Cc: devel, Pankaj Bansal
From: Pankaj Bansal <pankaj.bansal@nxp.com>
This patch adds dynamic freqency support for UART.
This patch is modelled on below patch
(cbba5ca104fb ArmPlatformPkg: PL011 Dynamic clock freq Support )
i have to ask, after introducing the generic library in MdeModulePkg
shouldn't the PL011 specific library PL011UartClockLib be removed from
ArmPlatformPkg?
And also, shouldn't the PL011UartClkInHz be replaced with more generic
PcdSerialClockRate?
Pankaj Bansal (1):
MdeModulePkg: UART Dynamic clock freq Support
.../Include/Library/SerialUartClockLib.h | 21 +++++++++++++++
.../BaseSerialPortLib16550.c | 9 ++++---
.../BaseSerialPortLib16550.inf | 2 +-
.../BaseSerialUartClockLib.c | 24 +++++++++++++++++
.../BaseSerialUartClockLib.inf | 27 +++++++++++++++++++
MdeModulePkg/MdeModulePkg.dsc | 2 ++
SourceLevelDebugPkg/SourceLevelDebugPkg.dsc | 1 +
UefiPayloadPkg/UefiPayloadPkgIa32.dsc | 1 +
UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc | 1 +
9 files changed, 83 insertions(+), 5 deletions(-)
create mode 100644 MdeModulePkg/Include/Library/SerialUartClockLib.h
create mode 100644 MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.c
create mode 100644 MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
--
2.17.1
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq Support
2020-02-19 13:31 [PATCH 0/1] UART Dynamic clock freq Support Pankaj Bansal
@ 2020-02-19 13:31 ` Pankaj Bansal
2020-03-19 13:40 ` [edk2-devel] " Samer El-Haj-Mahmoud
2020-03-19 15:09 ` Ni, Ray
0 siblings, 2 replies; 18+ messages in thread
From: Pankaj Bansal @ 2020-02-19 13:31 UTC (permalink / raw)
To: Jian J Wang, Hao A Wu, Ray Ni, Maurice Ma, Guo Dong, Benjamin You,
Leif Lindholm, Meenakshi Aggarwal, Varun Sethi
Cc: devel, Pankaj Bansal
From: Pankaj Bansal <pankaj.bansal@nxp.com>
Some platform support dynamic clocking, which is controlled by some
jumper setting or hardware registers. Result of that is that PCD
PcdSerialClockRate would need to be updated for frequency change.
This patch implements support for dynamic frequency for Uart.
This patch implements default lib, which is using Pcd. Platform which
needs dynamic clocking needs implement SerialUartClockLib
This patch is based on ArmPlatformPkg/Library/PL011UartClockLib
Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
---
.../Include/Library/SerialUartClockLib.h | 21 +++++++++++++++
.../BaseSerialPortLib16550.c | 9 ++++---
.../BaseSerialPortLib16550.inf | 2 +-
.../BaseSerialUartClockLib.c | 24 +++++++++++++++++
.../BaseSerialUartClockLib.inf | 27 +++++++++++++++++++
MdeModulePkg/MdeModulePkg.dsc | 2 ++
SourceLevelDebugPkg/SourceLevelDebugPkg.dsc | 1 +
UefiPayloadPkg/UefiPayloadPkgIa32.dsc | 1 +
UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc | 1 +
9 files changed, 83 insertions(+), 5 deletions(-)
create mode 100644 MdeModulePkg/Include/Library/SerialUartClockLib.h
create mode 100644 MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.c
create mode 100644 MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
diff --git a/MdeModulePkg/Include/Library/SerialUartClockLib.h b/MdeModulePkg/Include/Library/SerialUartClockLib.h
new file mode 100644
index 000000000000..b6b16f71d4cf
--- /dev/null
+++ b/MdeModulePkg/Include/Library/SerialUartClockLib.h
@@ -0,0 +1,21 @@
+/** @file
+*
+* Copyright 2020 NXP
+*
+* SPDX-License-Identifier: BSD-2-Clause-Patent
+*
+**/
+
+#ifndef SERIAL_UART_CLOCK_LIB_H__
+#define SERIAL_UART_CLOCK_LIB_H__
+
+/**
+ Return clock in for Uart IP
+**/
+UINT32
+EFIAPI
+BaseSerialPortGetClock (
+ VOID
+ );
+
+#endif
diff --git a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c
index 9cb50dd80d56..2e0c05d5789e 100644
--- a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c
+++ b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c
@@ -16,6 +16,7 @@
#include <Library/IoLib.h>
#include <Library/PciLib.h>
#include <Library/PlatformHookLib.h>
+#include <Library/SerialUartClockLib.h>
#include <Library/BaseLib.h>
//
@@ -501,8 +502,8 @@ SerialPortInitialize (
// Calculate divisor for baud generator
// Ref_Clk_Rate / Baud_Rate / 16
//
- Divisor = PcdGet32 (PcdSerialClockRate) / (PcdGet32 (PcdSerialBaudRate) * 16);
- if ((PcdGet32 (PcdSerialClockRate) % (PcdGet32 (PcdSerialBaudRate) * 16)) >= PcdGet32 (PcdSerialBaudRate) * 8) {
+ Divisor = BaseSerialPortGetClock () / (PcdGet32 (PcdSerialBaudRate) * 16);
+ if ((BaseSerialPortGetClock () % (PcdGet32 (PcdSerialBaudRate) * 16)) >= PcdGet32 (PcdSerialBaudRate) * 8) {
Divisor++;
}
@@ -1080,8 +1081,8 @@ SerialPortSetAttributes (
// Calculate divisor for baud generator
// Ref_Clk_Rate / Baud_Rate / 16
//
- Divisor = PcdGet32 (PcdSerialClockRate) / (SerialBaudRate * 16);
- if ((PcdGet32 (PcdSerialClockRate) % (SerialBaudRate * 16)) >= SerialBaudRate * 8) {
+ Divisor = BaseSerialPortGetClock () / (SerialBaudRate * 16);
+ if ((BaseSerialPortGetClock () % (SerialBaudRate * 16)) >= SerialBaudRate * 8) {
Divisor++;
}
diff --git a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
index 8b4ae3f1d4ee..b4c16504f211 100644
--- a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
+++ b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
@@ -24,6 +24,7 @@ [LibraryClasses]
IoLib
PlatformHookLib
PciLib
+ SerialUartClockLib
[Sources]
BaseSerialPortLib16550.c
@@ -37,7 +38,6 @@ [Pcd]
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialBaudRate ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialLineControl ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialFifoControl ## CONSUMES
- gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialPciDeviceInfo ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialExtendedTxFifoSize ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterStride ## CONSUMES
diff --git a/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.c b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.c
new file mode 100644
index 000000000000..7a0d0427cc4e
--- /dev/null
+++ b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.c
@@ -0,0 +1,24 @@
+/** @file
+*
+* Copyright 2020 NXP
+*
+* SPDX-License-Identifier: BSD-2-Clause-Patent
+*
+**/
+
+#include <Base.h>
+#include <Library/PcdLib.h>
+
+/**
+ Return clock in for Uart IP
+
+ @return Pcd PcdSerialClockRate
+**/
+UINT32
+EFIAPI
+BaseSerialPortGetClock (
+ VOID
+ )
+{
+ return PcdGet32 (PcdSerialClockRate);
+}
diff --git a/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
new file mode 100644
index 000000000000..91ba69436ed6
--- /dev/null
+++ b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
@@ -0,0 +1,27 @@
+#/* @file
+# Copyright 2020 NXP
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+#*/
+
+[Defines]
+ INF_VERSION = 0x0001001A
+ BASE_NAME = BaseSerialUartClockLib
+ FILE_GUID = fa65495e-d3c8-4ea3-9737-994e9ccbaf11
+ MODULE_TYPE = BASE
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = SerialUartClockLib
+
+[Packages]
+ MdePkg/MdePkg.dec
+ MdeModulePkg/MdeModulePkg.dec
+
+[Sources.common]
+ BaseSerialUartClockLib.c
+
+[LibraryClasses]
+ PcdLib
+
+[Pcd]
+ gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate ## CONSUMES
diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.dsc
index f7dbb27ce25d..d581ca797b3b 100644
--- a/MdeModulePkg/MdeModulePkg.dsc
+++ b/MdeModulePkg/MdeModulePkg.dsc
@@ -65,6 +65,7 @@ [LibraryClasses]
UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
SecurityManagementLib|MdeModulePkg/Library/DxeSecurityManagementLib/DxeSecurityManagementLib.inf
TimerLib|MdePkg/Library/BaseTimerLibNullTemplate/BaseTimerLibNullTemplate.inf
+ SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
SerialPortLib|MdePkg/Library/BaseSerialPortLibNull/BaseSerialPortLibNull.inf
CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
@@ -292,6 +293,7 @@ [Components]
MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/RuntimeDxeReportStatusCodeLib.inf
MdeModulePkg/Library/RuntimeResetSystemLib/RuntimeResetSystemLib.inf
+ MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
MdeModulePkg/Library/BasePlatformHookLibNull/BasePlatformHookLibNull.inf
MdeModulePkg/Library/DxeDebugPrintErrorLevelLib/DxeDebugPrintErrorLevelLib.inf
diff --git a/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc b/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc
index a1a1b81d03cb..c0ad88f26341 100644
--- a/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc
+++ b/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc
@@ -33,6 +33,7 @@ [LibraryClasses.common]
SynchronizationLib|MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
LocalApicLib|UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.inf
PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/BasePeCoffGetEntryPointLib.inf
+ SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
PeCoffExtraActionLib|SourceLevelDebugPkg/Library/PeCoffExtraActionLibDebug/PeCoffExtraActionLibDebug.inf
TimerLib|UefiCpuPkg/Library/SecPeiDxeTimerLibUefiCpu/SecPeiDxeTimerLibUefiCpu.inf
diff --git a/UefiPayloadPkg/UefiPayloadPkgIa32.dsc b/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
index d52945442e0e..a556a32b272c 100644
--- a/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
+++ b/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
@@ -174,6 +174,7 @@ [LibraryClasses]
#
TimerLib|UefiPayloadPkg/Library/AcpiTimerLib/AcpiTimerLib.inf
ResetSystemLib|UefiPayloadPkg/Library/ResetSystemLib/ResetSystemLib.inf
+ SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
PlatformHookLib|UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib.inf
PlatformBootManagerLib|UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
diff --git a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
index 0736cd995476..7e86375fe57d 100644
--- a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
+++ b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
@@ -175,6 +175,7 @@ [LibraryClasses]
#
TimerLib|UefiPayloadPkg/Library/AcpiTimerLib/AcpiTimerLib.inf
ResetSystemLib|UefiPayloadPkg/Library/ResetSystemLib/ResetSystemLib.inf
+ SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
PlatformHookLib|UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib.inf
PlatformBootManagerLib|UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
--
2.17.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq Support
2020-02-19 13:31 ` [PATCH 1/1] MdeModulePkg: " Pankaj Bansal
@ 2020-03-19 13:40 ` Samer El-Haj-Mahmoud
2020-03-19 15:09 ` Ni, Ray
1 sibling, 0 replies; 18+ messages in thread
From: Samer El-Haj-Mahmoud @ 2020-03-19 13:40 UTC (permalink / raw)
To: devel@edk2.groups.io, pankaj.bansal@oss.nxp.com, Jian J Wang,
Hao A Wu, Ray Ni, Maurice Ma, Guo Dong, Benjamin You,
Leif Lindholm, Meenakshi Aggarwal (meenakshi.aggarwal@nxp.com),
V Sethi (v.sethi@nxp.com)
Cc: Pankaj Bansal, Samer El-Haj-Mahmoud
Any help from maintainers in reviewing this MdeModulePkg patch please?
Thanks,
--Samer
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Pankaj Bansal via Groups.Io
Sent: Wednesday, February 19, 2020 8:32 AM
To: Jian J Wang <jian.j.wang@intel.com>; Hao A Wu <hao.a.wu@intel.com>; Ray Ni <ray.ni@intel.com>; Maurice Ma <maurice.ma@intel.com>; Guo Dong <guo.dong@intel.com>; Benjamin You <benjamin.you@intel.com>; Leif Lindholm <leif@nuviainc.com>; Meenakshi Aggarwal (meenakshi.aggarwal@nxp.com) <meenakshi.aggarwal@nxp.com>; V Sethi (v.sethi@nxp.com) <v.sethi@nxp.com>
Cc: devel@edk2.groups.io; Pankaj Bansal <pankaj.bansal@nxp.com>
Subject: [edk2-devel] [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq Support
From: Pankaj Bansal <pankaj.bansal@nxp.com>
Some platform support dynamic clocking, which is controlled by some jumper setting or hardware registers. Result of that is that PCD PcdSerialClockRate would need to be updated for frequency change.
This patch implements support for dynamic frequency for Uart.
This patch implements default lib, which is using Pcd. Platform which needs dynamic clocking needs implement SerialUartClockLib
This patch is based on ArmPlatformPkg/Library/PL011UartClockLib
Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
---
.../Include/Library/SerialUartClockLib.h | 21 +++++++++++++++
.../BaseSerialPortLib16550.c | 9 ++++---
.../BaseSerialPortLib16550.inf | 2 +-
.../BaseSerialUartClockLib.c | 24 +++++++++++++++++
.../BaseSerialUartClockLib.inf | 27 +++++++++++++++++++
MdeModulePkg/MdeModulePkg.dsc | 2 ++
SourceLevelDebugPkg/SourceLevelDebugPkg.dsc | 1 +
UefiPayloadPkg/UefiPayloadPkgIa32.dsc | 1 +
UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc | 1 +
9 files changed, 83 insertions(+), 5 deletions(-) create mode 100644 MdeModulePkg/Include/Library/SerialUartClockLib.h
create mode 100644 MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.c
create mode 100644 MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
diff --git a/MdeModulePkg/Include/Library/SerialUartClockLib.h b/MdeModulePkg/Include/Library/SerialUartClockLib.h
new file mode 100644
index 000000000000..b6b16f71d4cf
--- /dev/null
+++ b/MdeModulePkg/Include/Library/SerialUartClockLib.h
@@ -0,0 +1,21 @@
+/** @file
+*
+* Copyright 2020 NXP
+*
+* SPDX-License-Identifier: BSD-2-Clause-Patent
+*
+**/
+
+#ifndef SERIAL_UART_CLOCK_LIB_H__
+#define SERIAL_UART_CLOCK_LIB_H__
+
+/**
+ Return clock in for Uart IP
+**/
+UINT32
+EFIAPI
+BaseSerialPortGetClock (
+ VOID
+ );
+
+#endif
diff --git a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c
index 9cb50dd80d56..2e0c05d5789e 100644
--- a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c
+++ b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550
+++ .c
@@ -16,6 +16,7 @@
#include <Library/IoLib.h>
#include <Library/PciLib.h>
#include <Library/PlatformHookLib.h>
+#include <Library/SerialUartClockLib.h>
#include <Library/BaseLib.h>
//
@@ -501,8 +502,8 @@ SerialPortInitialize (
// Calculate divisor for baud generator
// Ref_Clk_Rate / Baud_Rate / 16
//
- Divisor = PcdGet32 (PcdSerialClockRate) / (PcdGet32 (PcdSerialBaudRate) * 16);
- if ((PcdGet32 (PcdSerialClockRate) % (PcdGet32 (PcdSerialBaudRate) * 16)) >= PcdGet32 (PcdSerialBaudRate) * 8) {
+ Divisor = BaseSerialPortGetClock () / (PcdGet32 (PcdSerialBaudRate) *
+ 16); if ((BaseSerialPortGetClock () % (PcdGet32 (PcdSerialBaudRate) *
+ 16)) >= PcdGet32 (PcdSerialBaudRate) * 8) {
Divisor++;
}
@@ -1080,8 +1081,8 @@ SerialPortSetAttributes (
// Calculate divisor for baud generator
// Ref_Clk_Rate / Baud_Rate / 16
//
- Divisor = PcdGet32 (PcdSerialClockRate) / (SerialBaudRate * 16);
- if ((PcdGet32 (PcdSerialClockRate) % (SerialBaudRate * 16)) >= SerialBaudRate * 8) {
+ Divisor = BaseSerialPortGetClock () / (SerialBaudRate * 16); if
+ ((BaseSerialPortGetClock () % (SerialBaudRate * 16)) >= SerialBaudRate
+ * 8) {
Divisor++;
}
diff --git a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
index 8b4ae3f1d4ee..b4c16504f211 100644
--- a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
+++ b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550
+++ .inf
@@ -24,6 +24,7 @@ [LibraryClasses]
IoLib
PlatformHookLib
PciLib
+ SerialUartClockLib
[Sources]
BaseSerialPortLib16550.c
@@ -37,7 +38,6 @@ [Pcd]
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialBaudRate ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialLineControl ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialFifoControl ## CONSUMES
- gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialPciDeviceInfo ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialExtendedTxFifoSize ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterStride ## CONSUMES
diff --git a/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.c b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.c
new file mode 100644
index 000000000000..7a0d0427cc4e
--- /dev/null
+++ b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib
+++ .c
@@ -0,0 +1,24 @@
+/** @file
+*
+* Copyright 2020 NXP
+*
+* SPDX-License-Identifier: BSD-2-Clause-Patent
+*
+**/
+
+#include <Base.h>
+#include <Library/PcdLib.h>
+
+/**
+ Return clock in for Uart IP
+
+ @return Pcd PcdSerialClockRate
+**/
+UINT32
+EFIAPI
+BaseSerialPortGetClock (
+ VOID
+ )
+{
+ return PcdGet32 (PcdSerialClockRate); }
diff --git a/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
new file mode 100644
index 000000000000..91ba69436ed6
--- /dev/null
+++ b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib
+++ .inf
@@ -0,0 +1,27 @@
+#/* @file
+# Copyright 2020 NXP
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent # #*/
+
+[Defines]
+ INF_VERSION = 0x0001001A
+ BASE_NAME = BaseSerialUartClockLib
+ FILE_GUID = fa65495e-d3c8-4ea3-9737-994e9ccbaf11
+ MODULE_TYPE = BASE
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = SerialUartClockLib
+
+[Packages]
+ MdePkg/MdePkg.dec
+ MdeModulePkg/MdeModulePkg.dec
+
+[Sources.common]
+ BaseSerialUartClockLib.c
+
+[LibraryClasses]
+ PcdLib
+
+[Pcd]
+ gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate ## CONSUMES
diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.dsc index f7dbb27ce25d..d581ca797b3b 100644
--- a/MdeModulePkg/MdeModulePkg.dsc
+++ b/MdeModulePkg/MdeModulePkg.dsc
@@ -65,6 +65,7 @@ [LibraryClasses]
UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
SecurityManagementLib|MdeModulePkg/Library/DxeSecurityManagementLib/DxeSecurityManagementLib.inf
TimerLib|MdePkg/Library/BaseTimerLibNullTemplate/BaseTimerLibNullTemplate.inf
+
+ SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSer
+ ialUartClockLib.inf
SerialPortLib|MdePkg/Library/BaseSerialPortLibNull/BaseSerialPortLibNull.inf
CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
@@ -292,6 +293,7 @@ [Components]
MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/RuntimeDxeReportStatusCodeLib.inf
MdeModulePkg/Library/RuntimeResetSystemLib/RuntimeResetSystemLib.inf
+
+ MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
MdeModulePkg/Library/BasePlatformHookLibNull/BasePlatformHookLibNull.inf
MdeModulePkg/Library/DxeDebugPrintErrorLevelLib/DxeDebugPrintErrorLevelLib.inf
diff --git a/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc b/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc
index a1a1b81d03cb..c0ad88f26341 100644
--- a/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc
+++ b/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc
@@ -33,6 +33,7 @@ [LibraryClasses.common]
SynchronizationLib|MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
LocalApicLib|UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.inf
PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/BasePeCoffGetEntryPointLib.inf
+
+ SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSer
+ ialUartClockLib.inf
SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
PeCoffExtraActionLib|SourceLevelDebugPkg/Library/PeCoffExtraActionLibDebug/PeCoffExtraActionLibDebug.inf
TimerLib|UefiCpuPkg/Library/SecPeiDxeTimerLibUefiCpu/SecPeiDxeTimerLibUefiCpu.inf
diff --git a/UefiPayloadPkg/UefiPayloadPkgIa32.dsc b/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
index d52945442e0e..a556a32b272c 100644
--- a/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
+++ b/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
@@ -174,6 +174,7 @@ [LibraryClasses]
#
TimerLib|UefiPayloadPkg/Library/AcpiTimerLib/AcpiTimerLib.inf
ResetSystemLib|UefiPayloadPkg/Library/ResetSystemLib/ResetSystemLib.inf
+
+ SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSer
+ ialUartClockLib.inf
SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
PlatformHookLib|UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib.inf
PlatformBootManagerLib|UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
diff --git a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
index 0736cd995476..7e86375fe57d 100644
--- a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
+++ b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
@@ -175,6 +175,7 @@ [LibraryClasses]
#
TimerLib|UefiPayloadPkg/Library/AcpiTimerLib/AcpiTimerLib.inf
ResetSystemLib|UefiPayloadPkg/Library/ResetSystemLib/ResetSystemLib.inf
+
+ SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSer
+ ialUartClockLib.inf
SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
PlatformHookLib|UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib.inf
PlatformBootManagerLib|UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
--
2.17.1
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq Support
2020-02-19 13:31 ` [PATCH 1/1] MdeModulePkg: " Pankaj Bansal
2020-03-19 13:40 ` [edk2-devel] " Samer El-Haj-Mahmoud
@ 2020-03-19 15:09 ` Ni, Ray
2020-03-19 15:30 ` Leif Lindholm
1 sibling, 1 reply; 18+ messages in thread
From: Ni, Ray @ 2020-03-19 15:09 UTC (permalink / raw)
To: Pankaj Bansal, Wang, Jian J, Wu, Hao A, Ma, Maurice, Dong, Guo,
You, Benjamin, Leif Lindholm, Meenakshi Aggarwal, Varun Sethi
Cc: devel@edk2.groups.io, Pankaj Bansal
It seems this change requires all platforms DSC to include the new library class/instance.
Is there a way that can avoid the platform impact?
Thanks,
Ray
> -----Original Message-----
> From: Pankaj Bansal <pankaj.bansal@oss.nxp.com>
> Sent: Wednesday, February 19, 2020 9:32 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Ma, Maurice
> <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>; You, Benjamin <benjamin.you@intel.com>; Leif Lindholm
> <leif@nuviainc.com>; Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; Varun Sethi <V.Sethi@nxp.com>
> Cc: devel@edk2.groups.io; Pankaj Bansal <pankaj.bansal@nxp.com>
> Subject: [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq Support
>
> From: Pankaj Bansal <pankaj.bansal@nxp.com>
>
> Some platform support dynamic clocking, which is controlled by some
> jumper setting or hardware registers. Result of that is that PCD
> PcdSerialClockRate would need to be updated for frequency change.
>
> This patch implements support for dynamic frequency for Uart.
>
> This patch implements default lib, which is using Pcd. Platform which
> needs dynamic clocking needs implement SerialUartClockLib
>
> This patch is based on ArmPlatformPkg/Library/PL011UartClockLib
>
> Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> ---
> .../Include/Library/SerialUartClockLib.h | 21 +++++++++++++++
> .../BaseSerialPortLib16550.c | 9 ++++---
> .../BaseSerialPortLib16550.inf | 2 +-
> .../BaseSerialUartClockLib.c | 24 +++++++++++++++++
> .../BaseSerialUartClockLib.inf | 27 +++++++++++++++++++
> MdeModulePkg/MdeModulePkg.dsc | 2 ++
> SourceLevelDebugPkg/SourceLevelDebugPkg.dsc | 1 +
> UefiPayloadPkg/UefiPayloadPkgIa32.dsc | 1 +
> UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc | 1 +
> 9 files changed, 83 insertions(+), 5 deletions(-)
> create mode 100644 MdeModulePkg/Include/Library/SerialUartClockLib.h
> create mode 100644 MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.c
> create mode 100644 MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
>
> diff --git a/MdeModulePkg/Include/Library/SerialUartClockLib.h b/MdeModulePkg/Include/Library/SerialUartClockLib.h
> new file mode 100644
> index 000000000000..b6b16f71d4cf
> --- /dev/null
> +++ b/MdeModulePkg/Include/Library/SerialUartClockLib.h
> @@ -0,0 +1,21 @@
> +/** @file
> +*
> +* Copyright 2020 NXP
> +*
> +* SPDX-License-Identifier: BSD-2-Clause-Patent
> +*
> +**/
> +
> +#ifndef SERIAL_UART_CLOCK_LIB_H__
> +#define SERIAL_UART_CLOCK_LIB_H__
> +
> +/**
> + Return clock in for Uart IP
> +**/
> +UINT32
> +EFIAPI
> +BaseSerialPortGetClock (
> + VOID
> + );
> +
> +#endif
> diff --git a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c
> b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c
> index 9cb50dd80d56..2e0c05d5789e 100644
> --- a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c
> +++ b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c
> @@ -16,6 +16,7 @@
> #include <Library/IoLib.h>
> #include <Library/PciLib.h>
> #include <Library/PlatformHookLib.h>
> +#include <Library/SerialUartClockLib.h>
> #include <Library/BaseLib.h>
>
> //
> @@ -501,8 +502,8 @@ SerialPortInitialize (
> // Calculate divisor for baud generator
> // Ref_Clk_Rate / Baud_Rate / 16
> //
> - Divisor = PcdGet32 (PcdSerialClockRate) / (PcdGet32 (PcdSerialBaudRate) * 16);
> - if ((PcdGet32 (PcdSerialClockRate) % (PcdGet32 (PcdSerialBaudRate) * 16)) >= PcdGet32 (PcdSerialBaudRate) * 8) {
> + Divisor = BaseSerialPortGetClock () / (PcdGet32 (PcdSerialBaudRate) * 16);
> + if ((BaseSerialPortGetClock () % (PcdGet32 (PcdSerialBaudRate) * 16)) >= PcdGet32 (PcdSerialBaudRate) * 8) {
> Divisor++;
> }
>
> @@ -1080,8 +1081,8 @@ SerialPortSetAttributes (
> // Calculate divisor for baud generator
> // Ref_Clk_Rate / Baud_Rate / 16
> //
> - Divisor = PcdGet32 (PcdSerialClockRate) / (SerialBaudRate * 16);
> - if ((PcdGet32 (PcdSerialClockRate) % (SerialBaudRate * 16)) >= SerialBaudRate * 8) {
> + Divisor = BaseSerialPortGetClock () / (SerialBaudRate * 16);
> + if ((BaseSerialPortGetClock () % (SerialBaudRate * 16)) >= SerialBaudRate * 8) {
> Divisor++;
> }
>
> diff --git a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
> b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
> index 8b4ae3f1d4ee..b4c16504f211 100644
> --- a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
> +++ b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
> @@ -24,6 +24,7 @@ [LibraryClasses]
> IoLib
> PlatformHookLib
> PciLib
> + SerialUartClockLib
>
> [Sources]
> BaseSerialPortLib16550.c
> @@ -37,7 +38,6 @@ [Pcd]
> gEfiMdeModulePkgTokenSpaceGuid.PcdSerialBaudRate ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdSerialLineControl ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdSerialFifoControl ## CONSUMES
> - gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdSerialPciDeviceInfo ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdSerialExtendedTxFifoSize ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterStride ## CONSUMES
> diff --git a/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.c
> b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.c
> new file mode 100644
> index 000000000000..7a0d0427cc4e
> --- /dev/null
> +++ b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.c
> @@ -0,0 +1,24 @@
> +/** @file
> +*
> +* Copyright 2020 NXP
> +*
> +* SPDX-License-Identifier: BSD-2-Clause-Patent
> +*
> +**/
> +
> +#include <Base.h>
> +#include <Library/PcdLib.h>
> +
> +/**
> + Return clock in for Uart IP
> +
> + @return Pcd PcdSerialClockRate
> +**/
> +UINT32
> +EFIAPI
> +BaseSerialPortGetClock (
> + VOID
> + )
> +{
> + return PcdGet32 (PcdSerialClockRate);
> +}
> diff --git a/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
> b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
> new file mode 100644
> index 000000000000..91ba69436ed6
> --- /dev/null
> +++ b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
> @@ -0,0 +1,27 @@
> +#/* @file
> +# Copyright 2020 NXP
> +#
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +#*/
> +
> +[Defines]
> + INF_VERSION = 0x0001001A
> + BASE_NAME = BaseSerialUartClockLib
> + FILE_GUID = fa65495e-d3c8-4ea3-9737-994e9ccbaf11
> + MODULE_TYPE = BASE
> + VERSION_STRING = 1.0
> + LIBRARY_CLASS = SerialUartClockLib
> +
> +[Packages]
> + MdePkg/MdePkg.dec
> + MdeModulePkg/MdeModulePkg.dec
> +
> +[Sources.common]
> + BaseSerialUartClockLib.c
> +
> +[LibraryClasses]
> + PcdLib
> +
> +[Pcd]
> + gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate ## CONSUMES
> diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.dsc
> index f7dbb27ce25d..d581ca797b3b 100644
> --- a/MdeModulePkg/MdeModulePkg.dsc
> +++ b/MdeModulePkg/MdeModulePkg.dsc
> @@ -65,6 +65,7 @@ [LibraryClasses]
> UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
> SecurityManagementLib|MdeModulePkg/Library/DxeSecurityManagementLib/DxeSecurityManagementLib.inf
> TimerLib|MdePkg/Library/BaseTimerLibNullTemplate/BaseTimerLibNullTemplate.inf
> + SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
> SerialPortLib|MdePkg/Library/BaseSerialPortLibNull/BaseSerialPortLibNull.inf
> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
> PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
> @@ -292,6 +293,7 @@ [Components]
> MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
> MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/RuntimeDxeReportStatusCodeLib.inf
> MdeModulePkg/Library/RuntimeResetSystemLib/RuntimeResetSystemLib.inf
> + MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
> MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
> MdeModulePkg/Library/BasePlatformHookLibNull/BasePlatformHookLibNull.inf
> MdeModulePkg/Library/DxeDebugPrintErrorLevelLib/DxeDebugPrintErrorLevelLib.inf
> diff --git a/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc b/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc
> index a1a1b81d03cb..c0ad88f26341 100644
> --- a/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc
> +++ b/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc
> @@ -33,6 +33,7 @@ [LibraryClasses.common]
> SynchronizationLib|MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
> LocalApicLib|UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.inf
> PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/BasePeCoffGetEntryPointLib.inf
> + SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
> SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
> PeCoffExtraActionLib|SourceLevelDebugPkg/Library/PeCoffExtraActionLibDebug/PeCoffExtraActionLibDebug.inf
> TimerLib|UefiCpuPkg/Library/SecPeiDxeTimerLibUefiCpu/SecPeiDxeTimerLibUefiCpu.inf
> diff --git a/UefiPayloadPkg/UefiPayloadPkgIa32.dsc b/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
> index d52945442e0e..a556a32b272c 100644
> --- a/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
> +++ b/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
> @@ -174,6 +174,7 @@ [LibraryClasses]
> #
> TimerLib|UefiPayloadPkg/Library/AcpiTimerLib/AcpiTimerLib.inf
> ResetSystemLib|UefiPayloadPkg/Library/ResetSystemLib/ResetSystemLib.inf
> + SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
> SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
> PlatformHookLib|UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib.inf
> PlatformBootManagerLib|UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> diff --git a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> index 0736cd995476..7e86375fe57d 100644
> --- a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> +++ b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> @@ -175,6 +175,7 @@ [LibraryClasses]
> #
> TimerLib|UefiPayloadPkg/Library/AcpiTimerLib/AcpiTimerLib.inf
> ResetSystemLib|UefiPayloadPkg/Library/ResetSystemLib/ResetSystemLib.inf
> + SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
> SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
> PlatformHookLib|UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib.inf
> PlatformBootManagerLib|UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> --
> 2.17.1
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq Support
2020-03-19 15:09 ` Ni, Ray
@ 2020-03-19 15:30 ` Leif Lindholm
2020-03-23 5:31 ` Pankaj Bansal
0 siblings, 1 reply; 18+ messages in thread
From: Leif Lindholm @ 2020-03-19 15:30 UTC (permalink / raw)
To: Ni, Ray
Cc: Pankaj Bansal, Wang, Jian J, Wu, Hao A, Ma, Maurice, Dong, Guo,
You, Benjamin, Meenakshi Aggarwal, Varun Sethi,
devel@edk2.groups.io, Pankaj Bansal
Hi Ray,
I agree it would be nice, but if not - this lets us get rid of
otherwise needless driver duplication. To me, that makes it worth a
little trivial churn.
/
Leif
On Thu, Mar 19, 2020 at 15:09:19 +0000, Ni, Ray wrote:
> It seems this change requires all platforms DSC to include the new library class/instance.
>
> Is there a way that can avoid the platform impact?
>
> Thanks,
> Ray
>
> > -----Original Message-----
> > From: Pankaj Bansal <pankaj.bansal@oss.nxp.com>
> > Sent: Wednesday, February 19, 2020 9:32 PM
> > To: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Ma, Maurice
> > <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>; You, Benjamin <benjamin.you@intel.com>; Leif Lindholm
> > <leif@nuviainc.com>; Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; Varun Sethi <V.Sethi@nxp.com>
> > Cc: devel@edk2.groups.io; Pankaj Bansal <pankaj.bansal@nxp.com>
> > Subject: [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq Support
> >
> > From: Pankaj Bansal <pankaj.bansal@nxp.com>
> >
> > Some platform support dynamic clocking, which is controlled by some
> > jumper setting or hardware registers. Result of that is that PCD
> > PcdSerialClockRate would need to be updated for frequency change.
> >
> > This patch implements support for dynamic frequency for Uart.
> >
> > This patch implements default lib, which is using Pcd. Platform which
> > needs dynamic clocking needs implement SerialUartClockLib
> >
> > This patch is based on ArmPlatformPkg/Library/PL011UartClockLib
> >
> > Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> > ---
> > .../Include/Library/SerialUartClockLib.h | 21 +++++++++++++++
> > .../BaseSerialPortLib16550.c | 9 ++++---
> > .../BaseSerialPortLib16550.inf | 2 +-
> > .../BaseSerialUartClockLib.c | 24 +++++++++++++++++
> > .../BaseSerialUartClockLib.inf | 27 +++++++++++++++++++
> > MdeModulePkg/MdeModulePkg.dsc | 2 ++
> > SourceLevelDebugPkg/SourceLevelDebugPkg.dsc | 1 +
> > UefiPayloadPkg/UefiPayloadPkgIa32.dsc | 1 +
> > UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc | 1 +
> > 9 files changed, 83 insertions(+), 5 deletions(-)
> > create mode 100644 MdeModulePkg/Include/Library/SerialUartClockLib.h
> > create mode 100644 MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.c
> > create mode 100644 MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
> >
> > diff --git a/MdeModulePkg/Include/Library/SerialUartClockLib.h b/MdeModulePkg/Include/Library/SerialUartClockLib.h
> > new file mode 100644
> > index 000000000000..b6b16f71d4cf
> > --- /dev/null
> > +++ b/MdeModulePkg/Include/Library/SerialUartClockLib.h
> > @@ -0,0 +1,21 @@
> > +/** @file
> > +*
> > +* Copyright 2020 NXP
> > +*
> > +* SPDX-License-Identifier: BSD-2-Clause-Patent
> > +*
> > +**/
> > +
> > +#ifndef SERIAL_UART_CLOCK_LIB_H__
> > +#define SERIAL_UART_CLOCK_LIB_H__
> > +
> > +/**
> > + Return clock in for Uart IP
> > +**/
> > +UINT32
> > +EFIAPI
> > +BaseSerialPortGetClock (
> > + VOID
> > + );
> > +
> > +#endif
> > diff --git a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c
> > b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c
> > index 9cb50dd80d56..2e0c05d5789e 100644
> > --- a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c
> > +++ b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c
> > @@ -16,6 +16,7 @@
> > #include <Library/IoLib.h>
> > #include <Library/PciLib.h>
> > #include <Library/PlatformHookLib.h>
> > +#include <Library/SerialUartClockLib.h>
> > #include <Library/BaseLib.h>
> >
> > //
> > @@ -501,8 +502,8 @@ SerialPortInitialize (
> > // Calculate divisor for baud generator
> > // Ref_Clk_Rate / Baud_Rate / 16
> > //
> > - Divisor = PcdGet32 (PcdSerialClockRate) / (PcdGet32 (PcdSerialBaudRate) * 16);
> > - if ((PcdGet32 (PcdSerialClockRate) % (PcdGet32 (PcdSerialBaudRate) * 16)) >= PcdGet32 (PcdSerialBaudRate) * 8) {
> > + Divisor = BaseSerialPortGetClock () / (PcdGet32 (PcdSerialBaudRate) * 16);
> > + if ((BaseSerialPortGetClock () % (PcdGet32 (PcdSerialBaudRate) * 16)) >= PcdGet32 (PcdSerialBaudRate) * 8) {
> > Divisor++;
> > }
> >
> > @@ -1080,8 +1081,8 @@ SerialPortSetAttributes (
> > // Calculate divisor for baud generator
> > // Ref_Clk_Rate / Baud_Rate / 16
> > //
> > - Divisor = PcdGet32 (PcdSerialClockRate) / (SerialBaudRate * 16);
> > - if ((PcdGet32 (PcdSerialClockRate) % (SerialBaudRate * 16)) >= SerialBaudRate * 8) {
> > + Divisor = BaseSerialPortGetClock () / (SerialBaudRate * 16);
> > + if ((BaseSerialPortGetClock () % (SerialBaudRate * 16)) >= SerialBaudRate * 8) {
> > Divisor++;
> > }
> >
> > diff --git a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
> > b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
> > index 8b4ae3f1d4ee..b4c16504f211 100644
> > --- a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
> > +++ b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
> > @@ -24,6 +24,7 @@ [LibraryClasses]
> > IoLib
> > PlatformHookLib
> > PciLib
> > + SerialUartClockLib
> >
> > [Sources]
> > BaseSerialPortLib16550.c
> > @@ -37,7 +38,6 @@ [Pcd]
> > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialBaudRate ## CONSUMES
> > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialLineControl ## CONSUMES
> > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialFifoControl ## CONSUMES
> > - gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate ## CONSUMES
> > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialPciDeviceInfo ## CONSUMES
> > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialExtendedTxFifoSize ## CONSUMES
> > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterStride ## CONSUMES
> > diff --git a/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.c
> > b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.c
> > new file mode 100644
> > index 000000000000..7a0d0427cc4e
> > --- /dev/null
> > +++ b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.c
> > @@ -0,0 +1,24 @@
> > +/** @file
> > +*
> > +* Copyright 2020 NXP
> > +*
> > +* SPDX-License-Identifier: BSD-2-Clause-Patent
> > +*
> > +**/
> > +
> > +#include <Base.h>
> > +#include <Library/PcdLib.h>
> > +
> > +/**
> > + Return clock in for Uart IP
> > +
> > + @return Pcd PcdSerialClockRate
> > +**/
> > +UINT32
> > +EFIAPI
> > +BaseSerialPortGetClock (
> > + VOID
> > + )
> > +{
> > + return PcdGet32 (PcdSerialClockRate);
> > +}
> > diff --git a/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
> > b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
> > new file mode 100644
> > index 000000000000..91ba69436ed6
> > --- /dev/null
> > +++ b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
> > @@ -0,0 +1,27 @@
> > +#/* @file
> > +# Copyright 2020 NXP
> > +#
> > +# SPDX-License-Identifier: BSD-2-Clause-Patent
> > +#
> > +#*/
> > +
> > +[Defines]
> > + INF_VERSION = 0x0001001A
> > + BASE_NAME = BaseSerialUartClockLib
> > + FILE_GUID = fa65495e-d3c8-4ea3-9737-994e9ccbaf11
> > + MODULE_TYPE = BASE
> > + VERSION_STRING = 1.0
> > + LIBRARY_CLASS = SerialUartClockLib
> > +
> > +[Packages]
> > + MdePkg/MdePkg.dec
> > + MdeModulePkg/MdeModulePkg.dec
> > +
> > +[Sources.common]
> > + BaseSerialUartClockLib.c
> > +
> > +[LibraryClasses]
> > + PcdLib
> > +
> > +[Pcd]
> > + gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate ## CONSUMES
> > diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.dsc
> > index f7dbb27ce25d..d581ca797b3b 100644
> > --- a/MdeModulePkg/MdeModulePkg.dsc
> > +++ b/MdeModulePkg/MdeModulePkg.dsc
> > @@ -65,6 +65,7 @@ [LibraryClasses]
> > UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
> > SecurityManagementLib|MdeModulePkg/Library/DxeSecurityManagementLib/DxeSecurityManagementLib.inf
> > TimerLib|MdePkg/Library/BaseTimerLibNullTemplate/BaseTimerLibNullTemplate.inf
> > + SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
> > SerialPortLib|MdePkg/Library/BaseSerialPortLibNull/BaseSerialPortLibNull.inf
> > CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
> > PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
> > @@ -292,6 +293,7 @@ [Components]
> > MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
> > MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/RuntimeDxeReportStatusCodeLib.inf
> > MdeModulePkg/Library/RuntimeResetSystemLib/RuntimeResetSystemLib.inf
> > + MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
> > MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
> > MdeModulePkg/Library/BasePlatformHookLibNull/BasePlatformHookLibNull.inf
> > MdeModulePkg/Library/DxeDebugPrintErrorLevelLib/DxeDebugPrintErrorLevelLib.inf
> > diff --git a/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc b/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc
> > index a1a1b81d03cb..c0ad88f26341 100644
> > --- a/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc
> > +++ b/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc
> > @@ -33,6 +33,7 @@ [LibraryClasses.common]
> > SynchronizationLib|MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
> > LocalApicLib|UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.inf
> > PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/BasePeCoffGetEntryPointLib.inf
> > + SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
> > SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
> > PeCoffExtraActionLib|SourceLevelDebugPkg/Library/PeCoffExtraActionLibDebug/PeCoffExtraActionLibDebug.inf
> > TimerLib|UefiCpuPkg/Library/SecPeiDxeTimerLibUefiCpu/SecPeiDxeTimerLibUefiCpu.inf
> > diff --git a/UefiPayloadPkg/UefiPayloadPkgIa32.dsc b/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
> > index d52945442e0e..a556a32b272c 100644
> > --- a/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
> > +++ b/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
> > @@ -174,6 +174,7 @@ [LibraryClasses]
> > #
> > TimerLib|UefiPayloadPkg/Library/AcpiTimerLib/AcpiTimerLib.inf
> > ResetSystemLib|UefiPayloadPkg/Library/ResetSystemLib/ResetSystemLib.inf
> > + SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
> > SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
> > PlatformHookLib|UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib.inf
> > PlatformBootManagerLib|UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> > diff --git a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > index 0736cd995476..7e86375fe57d 100644
> > --- a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > +++ b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > @@ -175,6 +175,7 @@ [LibraryClasses]
> > #
> > TimerLib|UefiPayloadPkg/Library/AcpiTimerLib/AcpiTimerLib.inf
> > ResetSystemLib|UefiPayloadPkg/Library/ResetSystemLib/ResetSystemLib.inf
> > + SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
> > SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
> > PlatformHookLib|UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib.inf
> > PlatformBootManagerLib|UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> > --
> > 2.17.1
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq Support
2020-03-19 15:30 ` Leif Lindholm
@ 2020-03-23 5:31 ` Pankaj Bansal
2020-03-26 14:13 ` [edk2-devel] " Guomin Jiang
0 siblings, 1 reply; 18+ messages in thread
From: Pankaj Bansal @ 2020-03-23 5:31 UTC (permalink / raw)
To: Leif Lindholm, Ni, Ray
Cc: Pankaj Bansal (OSS), Wang, Jian J, Wu, Hao A, Ma, Maurice,
Dong, Guo, You, Benjamin, Meenakshi Aggarwal, Varun Sethi,
devel@edk2.groups.io
Hi Ray,
I had thought of making PcdSerialClockRate as Dynamic PCD
I had made changes for it too.
But it doesn't work.
SerialPortInitalize, which uses PcdSerialClockRate, gets called from DebugLib constructor.
DebugLib is used by every module to print any info onto console.
For DxeCore and PcdDxe, for which PcdLib instance is NULL, this results in Assert :
https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BasePcdLibNull/PcdLib.c#L123
The other approach that I thought of was, to copy BaseSerialPortLib16550 for our platform and simply return EFI_SUCCESS from SerialPortInitalize.
But as Leif pointed out, this results in code duplication.
Regards,
Pankaj Bansal
> -----Original Message-----
> From: Leif Lindholm <leif@nuviainc.com>
> Sent: Thursday, March 19, 2020 9:01 PM
> To: Ni, Ray <ray.ni@intel.com>
> Cc: Pankaj Bansal (OSS) <pankaj.bansal@oss.nxp.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ma, Maurice
> <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>; You, Benjamin
> <benjamin.you@intel.com>; Meenakshi Aggarwal
> <meenakshi.aggarwal@nxp.com>; Varun Sethi <V.Sethi@nxp.com>;
> devel@edk2.groups.io; Pankaj Bansal <pankaj.bansal@nxp.com>
> Subject: Re: [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq Support
>
> Hi Ray,
>
> I agree it would be nice, but if not - this lets us get rid of
> otherwise needless driver duplication. To me, that makes it worth a
> little trivial churn.
>
> /
> Leif
>
> On Thu, Mar 19, 2020 at 15:09:19 +0000, Ni, Ray wrote:
> > It seems this change requires all platforms DSC to include the new library
> class/instance.
> >
> > Is there a way that can avoid the platform impact?
> >
> > Thanks,
> > Ray
> >
> > > -----Original Message-----
> > > From: Pankaj Bansal <pankaj.bansal@oss.nxp.com>
> > > Sent: Wednesday, February 19, 2020 9:32 PM
> > > To: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Ma, Maurice
> > > <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>; You,
> Benjamin <benjamin.you@intel.com>; Leif Lindholm
> > > <leif@nuviainc.com>; Meenakshi Aggarwal
> <meenakshi.aggarwal@nxp.com>; Varun Sethi <V.Sethi@nxp.com>
> > > Cc: devel@edk2.groups.io; Pankaj Bansal <pankaj.bansal@nxp.com>
> > > Subject: [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq Support
> > >
> > > From: Pankaj Bansal <pankaj.bansal@nxp.com>
> > >
> > > Some platform support dynamic clocking, which is controlled by some
> > > jumper setting or hardware registers. Result of that is that PCD
> > > PcdSerialClockRate would need to be updated for frequency change.
> > >
> > > This patch implements support for dynamic frequency for Uart.
> > >
> > > This patch implements default lib, which is using Pcd. Platform which
> > > needs dynamic clocking needs implement SerialUartClockLib
> > >
> > > This patch is based on ArmPlatformPkg/Library/PL011UartClockLib
> > >
> > > Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> > > ---
> > > .../Include/Library/SerialUartClockLib.h | 21 +++++++++++++++
> > > .../BaseSerialPortLib16550.c | 9 ++++---
> > > .../BaseSerialPortLib16550.inf | 2 +-
> > > .../BaseSerialUartClockLib.c | 24 +++++++++++++++++
> > > .../BaseSerialUartClockLib.inf | 27 +++++++++++++++++++
> > > MdeModulePkg/MdeModulePkg.dsc | 2 ++
> > > SourceLevelDebugPkg/SourceLevelDebugPkg.dsc | 1 +
> > > UefiPayloadPkg/UefiPayloadPkgIa32.dsc | 1 +
> > > UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc | 1 +
> > > 9 files changed, 83 insertions(+), 5 deletions(-)
> > > create mode 100644 MdeModulePkg/Include/Library/SerialUartClockLib.h
> > > create mode 100644
> MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.c
> > > create mode 100644
> MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
> > >
> > > diff --git a/MdeModulePkg/Include/Library/SerialUartClockLib.h
> b/MdeModulePkg/Include/Library/SerialUartClockLib.h
> > > new file mode 100644
> > > index 000000000000..b6b16f71d4cf
> > > --- /dev/null
> > > +++ b/MdeModulePkg/Include/Library/SerialUartClockLib.h
> > > @@ -0,0 +1,21 @@
> > > +/** @file
> > > +*
> > > +* Copyright 2020 NXP
> > > +*
> > > +* SPDX-License-Identifier: BSD-2-Clause-Patent
> > > +*
> > > +**/
> > > +
> > > +#ifndef SERIAL_UART_CLOCK_LIB_H__
> > > +#define SERIAL_UART_CLOCK_LIB_H__
> > > +
> > > +/**
> > > + Return clock in for Uart IP
> > > +**/
> > > +UINT32
> > > +EFIAPI
> > > +BaseSerialPortGetClock (
> > > + VOID
> > > + );
> > > +
> > > +#endif
> > > diff --git
> a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c
> > > b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c
> > > index 9cb50dd80d56..2e0c05d5789e 100644
> > > ---
> a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c
> > > +++
> b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c
> > > @@ -16,6 +16,7 @@
> > > #include <Library/IoLib.h>
> > > #include <Library/PciLib.h>
> > > #include <Library/PlatformHookLib.h>
> > > +#include <Library/SerialUartClockLib.h>
> > > #include <Library/BaseLib.h>
> > >
> > > //
> > > @@ -501,8 +502,8 @@ SerialPortInitialize (
> > > // Calculate divisor for baud generator
> > > // Ref_Clk_Rate / Baud_Rate / 16
> > > //
> > > - Divisor = PcdGet32 (PcdSerialClockRate) / (PcdGet32 (PcdSerialBaudRate) *
> 16);
> > > - if ((PcdGet32 (PcdSerialClockRate) % (PcdGet32 (PcdSerialBaudRate) *
> 16)) >= PcdGet32 (PcdSerialBaudRate) * 8) {
> > > + Divisor = BaseSerialPortGetClock () / (PcdGet32 (PcdSerialBaudRate) * 16);
> > > + if ((BaseSerialPortGetClock () % (PcdGet32 (PcdSerialBaudRate) * 16)) >=
> PcdGet32 (PcdSerialBaudRate) * 8) {
> > > Divisor++;
> > > }
> > >
> > > @@ -1080,8 +1081,8 @@ SerialPortSetAttributes (
> > > // Calculate divisor for baud generator
> > > // Ref_Clk_Rate / Baud_Rate / 16
> > > //
> > > - Divisor = PcdGet32 (PcdSerialClockRate) / (SerialBaudRate * 16);
> > > - if ((PcdGet32 (PcdSerialClockRate) % (SerialBaudRate * 16)) >=
> SerialBaudRate * 8) {
> > > + Divisor = BaseSerialPortGetClock () / (SerialBaudRate * 16);
> > > + if ((BaseSerialPortGetClock () % (SerialBaudRate * 16)) >= SerialBaudRate *
> 8) {
> > > Divisor++;
> > > }
> > >
> > > diff --git
> a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
> > >
> b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
> > > index 8b4ae3f1d4ee..b4c16504f211 100644
> > > ---
> a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
> > > +++
> b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
> > > @@ -24,6 +24,7 @@ [LibraryClasses]
> > > IoLib
> > > PlatformHookLib
> > > PciLib
> > > + SerialUartClockLib
> > >
> > > [Sources]
> > > BaseSerialPortLib16550.c
> > > @@ -37,7 +38,6 @@ [Pcd]
> > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialBaudRate ##
> CONSUMES
> > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialLineControl ##
> CONSUMES
> > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialFifoControl ##
> CONSUMES
> > > - gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate ##
> CONSUMES
> > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialPciDeviceInfo ##
> CONSUMES
> > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialExtendedTxFifoSize ##
> CONSUMES
> > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterStride ##
> CONSUMES
> > > diff --git
> a/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.c
> > > b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.c
> > > new file mode 100644
> > > index 000000000000..7a0d0427cc4e
> > > --- /dev/null
> > > +++
> b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.c
> > > @@ -0,0 +1,24 @@
> > > +/** @file
> > > +*
> > > +* Copyright 2020 NXP
> > > +*
> > > +* SPDX-License-Identifier: BSD-2-Clause-Patent
> > > +*
> > > +**/
> > > +
> > > +#include <Base.h>
> > > +#include <Library/PcdLib.h>
> > > +
> > > +/**
> > > + Return clock in for Uart IP
> > > +
> > > + @return Pcd PcdSerialClockRate
> > > +**/
> > > +UINT32
> > > +EFIAPI
> > > +BaseSerialPortGetClock (
> > > + VOID
> > > + )
> > > +{
> > > + return PcdGet32 (PcdSerialClockRate);
> > > +}
> > > diff --git
> a/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
> > >
> b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
> > > new file mode 100644
> > > index 000000000000..91ba69436ed6
> > > --- /dev/null
> > > +++
> b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
> > > @@ -0,0 +1,27 @@
> > > +#/* @file
> > > +# Copyright 2020 NXP
> > > +#
> > > +# SPDX-License-Identifier: BSD-2-Clause-Patent
> > > +#
> > > +#*/
> > > +
> > > +[Defines]
> > > + INF_VERSION = 0x0001001A
> > > + BASE_NAME = BaseSerialUartClockLib
> > > + FILE_GUID = fa65495e-d3c8-4ea3-9737-994e9ccbaf11
> > > + MODULE_TYPE = BASE
> > > + VERSION_STRING = 1.0
> > > + LIBRARY_CLASS = SerialUartClockLib
> > > +
> > > +[Packages]
> > > + MdePkg/MdePkg.dec
> > > + MdeModulePkg/MdeModulePkg.dec
> > > +
> > > +[Sources.common]
> > > + BaseSerialUartClockLib.c
> > > +
> > > +[LibraryClasses]
> > > + PcdLib
> > > +
> > > +[Pcd]
> > > + gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate ##
> CONSUMES
> > > diff --git a/MdeModulePkg/MdeModulePkg.dsc
> b/MdeModulePkg/MdeModulePkg.dsc
> > > index f7dbb27ce25d..d581ca797b3b 100644
> > > --- a/MdeModulePkg/MdeModulePkg.dsc
> > > +++ b/MdeModulePkg/MdeModulePkg.dsc
> > > @@ -65,6 +65,7 @@ [LibraryClasses]
> > > UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
> > >
> SecurityManagementLib|MdeModulePkg/Library/DxeSecurityManagementLib/D
> xeSecurityManagementLib.inf
> > >
> TimerLib|MdePkg/Library/BaseTimerLibNullTemplate/BaseTimerLibNullTemplat
> e.inf
> > > +
> SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialU
> artClockLib.inf
> > >
> SerialPortLib|MdePkg/Library/BaseSerialPortLibNull/BaseSerialPortLibNull.inf
> > >
> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
> > > PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
> > > @@ -292,6 +293,7 @@ [Components]
> > >
> MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
> > >
> MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/RuntimeDxeReportSt
> atusCodeLib.inf
> > >
> MdeModulePkg/Library/RuntimeResetSystemLib/RuntimeResetSystemLib.inf
> > > +
> MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
> > >
> MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
> > >
> MdeModulePkg/Library/BasePlatformHookLibNull/BasePlatformHookLibNull.inf
> > >
> MdeModulePkg/Library/DxeDebugPrintErrorLevelLib/DxeDebugPrintErrorLevelLi
> b.inf
> > > diff --git a/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc
> b/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc
> > > index a1a1b81d03cb..c0ad88f26341 100644
> > > --- a/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc
> > > +++ b/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc
> > > @@ -33,6 +33,7 @@ [LibraryClasses.common]
> > >
> SynchronizationLib|MdePkg/Library/BaseSynchronizationLib/BaseSynchronizati
> onLib.inf
> > > LocalApicLib|UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.inf
> > >
> PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/BasePeC
> offGetEntryPointLib.inf
> > > +
> SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialU
> artClockLib.inf
> > >
> SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib
> 16550.inf
> > >
> PeCoffExtraActionLib|SourceLevelDebugPkg/Library/PeCoffExtraActionLibDebu
> g/PeCoffExtraActionLibDebug.inf
> > >
> TimerLib|UefiCpuPkg/Library/SecPeiDxeTimerLibUefiCpu/SecPeiDxeTimerLibUe
> fiCpu.inf
> > > diff --git a/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
> b/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
> > > index d52945442e0e..a556a32b272c 100644
> > > --- a/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
> > > +++ b/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
> > > @@ -174,6 +174,7 @@ [LibraryClasses]
> > > #
> > > TimerLib|UefiPayloadPkg/Library/AcpiTimerLib/AcpiTimerLib.inf
> > >
> ResetSystemLib|UefiPayloadPkg/Library/ResetSystemLib/ResetSystemLib.inf
> > > +
> SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialU
> artClockLib.inf
> > >
> SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib
> 16550.inf
> > >
> PlatformHookLib|UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib.in
> f
> > >
> PlatformBootManagerLib|UefiPayloadPkg/Library/PlatformBootManagerLib/Pl
> atformBootManagerLib.inf
> > > diff --git a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > > index 0736cd995476..7e86375fe57d 100644
> > > --- a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > > +++ b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > > @@ -175,6 +175,7 @@ [LibraryClasses]
> > > #
> > > TimerLib|UefiPayloadPkg/Library/AcpiTimerLib/AcpiTimerLib.inf
> > >
> ResetSystemLib|UefiPayloadPkg/Library/ResetSystemLib/ResetSystemLib.inf
> > > +
> SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialU
> artClockLib.inf
> > >
> SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib
> 16550.inf
> > >
> PlatformHookLib|UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib.in
> f
> > >
> PlatformBootManagerLib|UefiPayloadPkg/Library/PlatformBootManagerLib/Pl
> atformBootManagerLib.inf
> > > --
> > > 2.17.1
> >
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq Support
2020-03-23 5:31 ` Pankaj Bansal
@ 2020-03-26 14:13 ` Guomin Jiang
2020-03-28 12:36 ` Pankaj Bansal
0 siblings, 1 reply; 18+ messages in thread
From: Guomin Jiang @ 2020-03-26 14:13 UTC (permalink / raw)
To: devel@edk2.groups.io, pankaj.bansal@nxp.com, Leif Lindholm,
Ni, Ray
Cc: Pankaj Bansal (OSS), Wang, Jian J, Wu, Hao A, Ma, Maurice,
Dong, Guo, You, Benjamin, Meenakshi Aggarwal, Varun Sethi
It is a good idea,
I have a question:
1. When can detect the jumper signal or register?
2. If the jumper use GPIO, SerialUartClockLib will depend on GpioLib?
3. If the register is inside SIO, how to dispose the dependence?
If it is too complex to implement the SerialUartClockLib, the project owner will choose use BaseSerialUartClockLib rather implement it. and in this case, it make no sense after import complexity.
I have also reviewed the ArmPlatformPkg/Library/PL011UartClockLib code, it still use the fixed data rather than dynamically detect clock.
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Pankaj
> Bansal
> Sent: Monday, March 23, 2020 1:31 PM
> To: Leif Lindholm <leif@nuviainc.com>; Ni, Ray <ray.ni@intel.com>
> Cc: Pankaj Bansal (OSS) <pankaj.bansal@oss.nxp.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ma, Maurice
> <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>; You,
> Benjamin <benjamin.you@intel.com>; Meenakshi Aggarwal
> <meenakshi.aggarwal@nxp.com>; Varun Sethi <V.Sethi@nxp.com>;
> devel@edk2.groups.io
> Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: UART Dynamic clock
> freq Support
>
> Hi Ray,
>
> I had thought of making PcdSerialClockRate as Dynamic PCD I had made
> changes for it too.
> But it doesn't work.
> SerialPortInitalize, which uses PcdSerialClockRate, gets called from DebugLib
> constructor.
> DebugLib is used by every module to print any info onto console.
> For DxeCore and PcdDxe, for which PcdLib instance is NULL, this results in
> Assert :
>
> https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BasePcdLi
> bNull/PcdLib.c#L123
>
> The other approach that I thought of was, to copy BaseSerialPortLib16550 for
> our platform and simply return EFI_SUCCESS from SerialPortInitalize.
> But as Leif pointed out, this results in code duplication.
>
> Regards,
> Pankaj Bansal
>
> > -----Original Message-----
> > From: Leif Lindholm <leif@nuviainc.com>
> > Sent: Thursday, March 19, 2020 9:01 PM
> > To: Ni, Ray <ray.ni@intel.com>
> > Cc: Pankaj Bansal (OSS) <pankaj.bansal@oss.nxp.com>; Wang, Jian J
> > <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ma, Maurice
> > <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>; You,
> Benjamin
> > <benjamin.you@intel.com>; Meenakshi Aggarwal
> > <meenakshi.aggarwal@nxp.com>; Varun Sethi <V.Sethi@nxp.com>;
> > devel@edk2.groups.io; Pankaj Bansal <pankaj.bansal@nxp.com>
> > Subject: Re: [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq
> Support
> >
> > Hi Ray,
> >
> > I agree it would be nice, but if not - this lets us get rid of
> > otherwise needless driver duplication. To me, that makes it worth a
> > little trivial churn.
> >
> > /
> > Leif
> >
> > On Thu, Mar 19, 2020 at 15:09:19 +0000, Ni, Ray wrote:
> > > It seems this change requires all platforms DSC to include the new
> > > library
> > class/instance.
> > >
> > > Is there a way that can avoid the platform impact?
> > >
> > > Thanks,
> > > Ray
> > >
> > > > -----Original Message-----
> > > > From: Pankaj Bansal <pankaj.bansal@oss.nxp.com>
> > > > Sent: Wednesday, February 19, 2020 9:32 PM
> > > > To: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Ma, Maurice
> > > > <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>; You,
> > Benjamin <benjamin.you@intel.com>; Leif Lindholm
> > > > <leif@nuviainc.com>; Meenakshi Aggarwal
> > <meenakshi.aggarwal@nxp.com>; Varun Sethi <V.Sethi@nxp.com>
> > > > Cc: devel@edk2.groups.io; Pankaj Bansal <pankaj.bansal@nxp.com>
> > > > Subject: [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq
> Support
> > > >
> > > > From: Pankaj Bansal <pankaj.bansal@nxp.com>
> > > >
> > > > Some platform support dynamic clocking, which is controlled by
> > > > some jumper setting or hardware registers. Result of that is that
> > > > PCD PcdSerialClockRate would need to be updated for frequency
> change.
> > > >
> > > > This patch implements support for dynamic frequency for Uart.
> > > >
> > > > This patch implements default lib, which is using Pcd. Platform
> > > > which needs dynamic clocking needs implement SerialUartClockLib
> > > >
> > > > This patch is based on ArmPlatformPkg/Library/PL011UartClockLib
> > > >
> > > > Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> > > > ---
> > > > .../Include/Library/SerialUartClockLib.h | 21 +++++++++++++++
> > > > .../BaseSerialPortLib16550.c | 9 ++++---
> > > > .../BaseSerialPortLib16550.inf | 2 +-
> > > > .../BaseSerialUartClockLib.c | 24 +++++++++++++++++
> > > > .../BaseSerialUartClockLib.inf | 27 +++++++++++++++++++
> > > > MdeModulePkg/MdeModulePkg.dsc | 2 ++
> > > > SourceLevelDebugPkg/SourceLevelDebugPkg.dsc | 1 +
> > > > UefiPayloadPkg/UefiPayloadPkgIa32.dsc | 1 +
> > > > UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc | 1 +
> > > > 9 files changed, 83 insertions(+), 5 deletions(-) create mode
> > > > 100644 MdeModulePkg/Include/Library/SerialUartClockLib.h
> > > > create mode 100644
> > MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.c
> > > > create mode 100644
> > MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
> > > >
> > > > diff --git a/MdeModulePkg/Include/Library/SerialUartClockLib.h
> > b/MdeModulePkg/Include/Library/SerialUartClockLib.h
> > > > new file mode 100644
> > > > index 000000000000..b6b16f71d4cf
> > > > --- /dev/null
> > > > +++ b/MdeModulePkg/Include/Library/SerialUartClockLib.h
> > > > @@ -0,0 +1,21 @@
> > > > +/** @file
> > > > +*
> > > > +* Copyright 2020 NXP
> > > > +*
> > > > +* SPDX-License-Identifier: BSD-2-Clause-Patent
> > > > +*
> > > > +**/
> > > > +
> > > > +#ifndef SERIAL_UART_CLOCK_LIB_H__ #define
> > > > +SERIAL_UART_CLOCK_LIB_H__
> > > > +
> > > > +/**
> > > > + Return clock in for Uart IP
> > > > +**/
> > > > +UINT32
> > > > +EFIAPI
> > > > +BaseSerialPortGetClock (
> > > > + VOID
> > > > + );
> > > > +
> > > > +#endif
> > > > diff --git
> >
> a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c
> > > >
> b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib165
> > > > 50.c index 9cb50dd80d56..2e0c05d5789e 100644
> > > > ---
> >
> a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c
> > > > +++
> >
> b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c
> > > > @@ -16,6 +16,7 @@
> > > > #include <Library/IoLib.h>
> > > > #include <Library/PciLib.h>
> > > > #include <Library/PlatformHookLib.h>
> > > > +#include <Library/SerialUartClockLib.h>
> > > > #include <Library/BaseLib.h>
> > > >
> > > > //
> > > > @@ -501,8 +502,8 @@ SerialPortInitialize (
> > > > // Calculate divisor for baud generator
> > > > // Ref_Clk_Rate / Baud_Rate / 16
> > > > //
> > > > - Divisor = PcdGet32 (PcdSerialClockRate) / (PcdGet32
> > > > (PcdSerialBaudRate) *
> > 16);
> > > > - if ((PcdGet32 (PcdSerialClockRate) % (PcdGet32
> > > > (PcdSerialBaudRate) *
> > 16)) >= PcdGet32 (PcdSerialBaudRate) * 8) {
> > > > + Divisor = BaseSerialPortGetClock () / (PcdGet32
> > > > + (PcdSerialBaudRate) * 16); if ((BaseSerialPortGetClock () %
> > > > + (PcdGet32 (PcdSerialBaudRate) * 16)) >=
> > PcdGet32 (PcdSerialBaudRate) * 8) {
> > > > Divisor++;
> > > > }
> > > >
> > > > @@ -1080,8 +1081,8 @@ SerialPortSetAttributes (
> > > > // Calculate divisor for baud generator
> > > > // Ref_Clk_Rate / Baud_Rate / 16
> > > > //
> > > > - Divisor = PcdGet32 (PcdSerialClockRate) / (SerialBaudRate *
> > > > 16);
> > > > - if ((PcdGet32 (PcdSerialClockRate) % (SerialBaudRate * 16)) >=
> > SerialBaudRate * 8) {
> > > > + Divisor = BaseSerialPortGetClock () / (SerialBaudRate * 16);
> > > > + if ((BaseSerialPortGetClock () % (SerialBaudRate * 16)) >=
> > > > + SerialBaudRate *
> > 8) {
> > > > Divisor++;
> > > > }
> > > >
> > > > diff --git
> > a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.i
> > nf
> > > >
> > b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.i
> > nf
> > > > index 8b4ae3f1d4ee..b4c16504f211 100644
> > > > ---
> > a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.i
> > nf
> > > > +++
> > b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.i
> > nf
> > > > @@ -24,6 +24,7 @@ [LibraryClasses]
> > > > IoLib
> > > > PlatformHookLib
> > > > PciLib
> > > > + SerialUartClockLib
> > > >
> > > > [Sources]
> > > > BaseSerialPortLib16550.c
> > > > @@ -37,7 +38,6 @@ [Pcd]
> > > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialBaudRate ##
> > CONSUMES
> > > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialLineControl ##
> > CONSUMES
> > > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialFifoControl ##
> > CONSUMES
> > > > - gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate ##
> > CONSUMES
> > > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialPciDeviceInfo ##
> > CONSUMES
> > > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialExtendedTxFifoSize
> ##
> > CONSUMES
> > > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterStride ##
> > CONSUMES
> > > > diff --git
> > a/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.c
> > > > b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockL
> > > > ib.c
> > > > new file mode 100644
> > > > index 000000000000..7a0d0427cc4e
> > > > --- /dev/null
> > > > +++
> > b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.c
> > > > @@ -0,0 +1,24 @@
> > > > +/** @file
> > > > +*
> > > > +* Copyright 2020 NXP
> > > > +*
> > > > +* SPDX-License-Identifier: BSD-2-Clause-Patent
> > > > +*
> > > > +**/
> > > > +
> > > > +#include <Base.h>
> > > > +#include <Library/PcdLib.h>
> > > > +
> > > > +/**
> > > > + Return clock in for Uart IP
> > > > +
> > > > + @return Pcd PcdSerialClockRate
> > > > +**/
> > > > +UINT32
> > > > +EFIAPI
> > > > +BaseSerialPortGetClock (
> > > > + VOID
> > > > + )
> > > > +{
> > > > + return PcdGet32 (PcdSerialClockRate); }
> > > > diff --git
> > a/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.i
> > nf
> > > >
> > b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.i
> > nf
> > > > new file mode 100644
> > > > index 000000000000..91ba69436ed6
> > > > --- /dev/null
> > > > +++
> > b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.i
> > nf
> > > > @@ -0,0 +1,27 @@
> > > > +#/* @file
> > > > +# Copyright 2020 NXP
> > > > +#
> > > > +# SPDX-License-Identifier: BSD-2-Clause-Patent # #*/
> > > > +
> > > > +[Defines]
> > > > + INF_VERSION = 0x0001001A
> > > > + BASE_NAME = BaseSerialUartClockLib
> > > > + FILE_GUID = fa65495e-d3c8-4ea3-9737-994e9ccbaf11
> > > > + MODULE_TYPE = BASE
> > > > + VERSION_STRING = 1.0
> > > > + LIBRARY_CLASS = SerialUartClockLib
> > > > +
> > > > +[Packages]
> > > > + MdePkg/MdePkg.dec
> > > > + MdeModulePkg/MdeModulePkg.dec
> > > > +
> > > > +[Sources.common]
> > > > + BaseSerialUartClockLib.c
> > > > +
> > > > +[LibraryClasses]
> > > > + PcdLib
> > > > +
> > > > +[Pcd]
> > > > + gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate ##
> > CONSUMES
> > > > diff --git a/MdeModulePkg/MdeModulePkg.dsc
> > b/MdeModulePkg/MdeModulePkg.dsc
> > > > index f7dbb27ce25d..d581ca797b3b 100644
> > > > --- a/MdeModulePkg/MdeModulePkg.dsc
> > > > +++ b/MdeModulePkg/MdeModulePkg.dsc
> > > > @@ -65,6 +65,7 @@ [LibraryClasses]
> > > > UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
> > > >
> >
> SecurityManagementLib|MdeModulePkg/Library/DxeSecurityManagementL
> ib/D
> > xeSecurityManagementLib.inf
> > > >
> >
> TimerLib|MdePkg/Library/BaseTimerLibNullTemplate/BaseTimerLibNullTem
> pl
> > TimerLib|at
> > e.inf
> > > > +
> >
> SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSer
> > SerialUartClockLib|ialU
> > artClockLib.inf
> > > >
> > SerialPortLib|MdePkg/Library/BaseSerialPortLibNull/BaseSerialPortLibNu
> > SerialPortLib|ll.inf
> > > >
> >
> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.i
> n
> > CapsuleLib|f
> > > > PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
> > > > @@ -292,6 +293,7 @@ [Components]
> > > >
> >
> MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.
> inf
> > > >
> >
> MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/RuntimeDxeRep
> ortSt
> > atusCodeLib.inf
> > > >
> >
> MdeModulePkg/Library/RuntimeResetSystemLib/RuntimeResetSystemLib.i
> nf
> > > > +
> > MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
> > > >
> > MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
> > > >
> >
> MdeModulePkg/Library/BasePlatformHookLibNull/BasePlatformHookLibNull
> .i
> > nf
> > > >
> >
> MdeModulePkg/Library/DxeDebugPrintErrorLevelLib/DxeDebugPrintErrorLe
> ve
> > lLi
> > b.inf
> > > > diff --git a/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc
> > b/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc
> > > > index a1a1b81d03cb..c0ad88f26341 100644
> > > > --- a/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc
> > > > +++ b/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc
> > > > @@ -33,6 +33,7 @@ [LibraryClasses.common]
> > > >
> >
> SynchronizationLib|MdePkg/Library/BaseSynchronizationLib/BaseSynchroni
> > SynchronizationLib|zati
> > onLib.inf
> > > > LocalApicLib|UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.inf
> > > >
> >
> PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/Base
> P
> > PeCoffGetEntryPointLib|eC
> > offGetEntryPointLib.inf
> > > > +
> >
> SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSer
> > SerialUartClockLib|ialU
> > artClockLib.inf
> > > >
> > SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPo
> > SerialPortLib|rtLib
> > 16550.inf
> > > >
> >
> PeCoffExtraActionLib|SourceLevelDebugPkg/Library/PeCoffExtraActionLibD
> > PeCoffExtraActionLib|ebu
> > g/PeCoffExtraActionLibDebug.inf
> > > >
> >
> TimerLib|UefiCpuPkg/Library/SecPeiDxeTimerLibUefiCpu/SecPeiDxeTimerLi
> b
> > TimerLib|Ue
> > fiCpu.inf
> > > > diff --git a/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
> > b/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
> > > > index d52945442e0e..a556a32b272c 100644
> > > > --- a/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
> > > > +++ b/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
> > > > @@ -174,6 +174,7 @@ [LibraryClasses]
> > > > #
> > > > TimerLib|UefiPayloadPkg/Library/AcpiTimerLib/AcpiTimerLib.inf
> > > >
> >
> ResetSystemLib|UefiPayloadPkg/Library/ResetSystemLib/ResetSystemLib.in
> > ResetSystemLib|f
> > > > +
> >
> SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSer
> > SerialUartClockLib|ialU
> > artClockLib.inf
> > > >
> > SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPo
> > SerialPortLib|rtLib
> > 16550.inf
> > > >
> >
> PlatformHookLib|UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib
> > PlatformHookLib|.in
> > f
> > > >
> >
> PlatformBootManagerLib|UefiPayloadPkg/Library/PlatformBootManagerLib/
> P
> > PlatformBootManagerLib|l
> > atformBootManagerLib.inf
> > > > diff --git a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > > > index 0736cd995476..7e86375fe57d 100644
> > > > --- a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > > > +++ b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > > > @@ -175,6 +175,7 @@ [LibraryClasses]
> > > > #
> > > > TimerLib|UefiPayloadPkg/Library/AcpiTimerLib/AcpiTimerLib.inf
> > > >
> >
> ResetSystemLib|UefiPayloadPkg/Library/ResetSystemLib/ResetSystemLib.in
> > ResetSystemLib|f
> > > > +
> >
> SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSer
> > SerialUartClockLib|ialU
> > artClockLib.inf
> > > >
> > SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPo
> > SerialPortLib|rtLib
> > 16550.inf
> > > >
> >
> PlatformHookLib|UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib
> > PlatformHookLib|.in
> > f
> > > >
> >
> PlatformBootManagerLib|UefiPayloadPkg/Library/PlatformBootManagerLib/
> P
> > PlatformBootManagerLib|l
> > atformBootManagerLib.inf
> > > > --
> > > > 2.17.1
> > >
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq Support
2020-03-26 14:13 ` [edk2-devel] " Guomin Jiang
@ 2020-03-28 12:36 ` Pankaj Bansal
2020-03-30 1:20 ` Guomin Jiang
0 siblings, 1 reply; 18+ messages in thread
From: Pankaj Bansal @ 2020-03-28 12:36 UTC (permalink / raw)
To: Jiang, Guomin, devel@edk2.groups.io, Pankaj Bansal, Leif Lindholm,
Ni, Ray, Ard Biesheuvel
Cc: Wang, Jian J, Wu, Hao A, Ma, Maurice, Dong, Guo, You, Benjamin,
Meenakshi Aggarwal, Varun Sethi, Samer El-Haj-Mahmoud
Hello Jiang,
> -----Original Message-----
> From: Jiang, Guomin <guomin.jiang@intel.com>
> Sent: Thursday, March 26, 2020 7:44 PM
> To: devel@edk2.groups.io; Pankaj Bansal <pankaj.bansal@nxp.com>; Leif
> Lindholm <leif@nuviainc.com>; Ni, Ray <ray.ni@intel.com>
> Cc: Pankaj Bansal (OSS) <pankaj.bansal@oss.nxp.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ma, Maurice
> <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>; You, Benjamin
> <benjamin.you@intel.com>; Meenakshi Aggarwal
> <meenakshi.aggarwal@nxp.com>; Varun Sethi <V.Sethi@nxp.com>
> Subject: RE: [edk2-devel] [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq
> Support
>
> It is a good idea,
>
> I have a question:
> 1. When can detect the jumper signal or register?
-> This depends on platform. Ideally as early as possible in boot process (in SEC phase), because PEI Core and onwards, all modules start making call to DebugLib constructor.
> 2. If the jumper use GPIO, SerialUartClockLib will depend on GpioLib?
-> Yes, SerialUartClockLib can depend on other libraries as well. E.g. in https://edk2.groups.io/g/devel/message/56009, SerialUartClockLib depends on ArmPlatformLib
> 3. If the register is inside SIO, how to dispose the dependence?
-> I did not understand term SIO. Quick google search throws up Intel Serial IO host controller driver. Intel Serial IO driver is required if you plan to use the I2C, UART, or GPIO host controllers.
Is that what you are referring to ? I assume you mean that what happens if the SerialUartClockLib, tries to read registers that require SerialPortLib ? i.e. circular dependency i.e deadlock ?
In that case, some kind of intervention is required on Platform owner part. May be implement a version of SerialUartClockLib for SEC phase that doesn't depend on SerialPortLib, which lets
the serial port to initialize. For all other module types, implement another version of SerialUartClockLib.
>
> If it is too complex to implement the SerialUartClockLib, the project owner will
> choose use BaseSerialUartClockLib rather implement it. and in this case, it make
> no sense after import complexity.
Complexity is platform specific. We have implemented BaseSerialUartClockLib in https://edk2.groups.io/g/devel/message/56009.
>
> I have also reviewed the ArmPlatformPkg/Library/PL011UartClockLib code, it
> still use the fixed data rather than dynamically detect clock.
Right. As I said, making PcdSerialClockRate as Dynamic PCD, we cannot use this in modules for which PcdLib is BasePcdLibNull.
It results in ASSERT.
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Pankaj
> > Bansal
> > Sent: Monday, March 23, 2020 1:31 PM
> > To: Leif Lindholm <leif@nuviainc.com>; Ni, Ray <ray.ni@intel.com>
> > Cc: Pankaj Bansal (OSS) <pankaj.bansal@oss.nxp.com>; Wang, Jian J
> > <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ma, Maurice
> > <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>; You,
> > Benjamin <benjamin.you@intel.com>; Meenakshi Aggarwal
> > <meenakshi.aggarwal@nxp.com>; Varun Sethi <V.Sethi@nxp.com>;
> > devel@edk2.groups.io
> > Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: UART Dynamic clock
> > freq Support
> >
> > Hi Ray,
> >
> > I had thought of making PcdSerialClockRate as Dynamic PCD I had made
> > changes for it too.
> > But it doesn't work.
> > SerialPortInitalize, which uses PcdSerialClockRate, gets called from DebugLib
> > constructor.
> > DebugLib is used by every module to print any info onto console.
> > For DxeCore and PcdDxe, for which PcdLib instance is NULL, this results in
> > Assert :
> >
> > https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BasePcdLi
> > bNull/PcdLib.c#L123
> >
> > The other approach that I thought of was, to copy BaseSerialPortLib16550 for
> > our platform and simply return EFI_SUCCESS from SerialPortInitalize.
> > But as Leif pointed out, this results in code duplication.
> >
> > Regards,
> > Pankaj Bansal
> >
> > > -----Original Message-----
> > > From: Leif Lindholm <leif@nuviainc.com>
> > > Sent: Thursday, March 19, 2020 9:01 PM
> > > To: Ni, Ray <ray.ni@intel.com>
> > > Cc: Pankaj Bansal (OSS) <pankaj.bansal@oss.nxp.com>; Wang, Jian J
> > > <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ma, Maurice
> > > <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>; You,
> > Benjamin
> > > <benjamin.you@intel.com>; Meenakshi Aggarwal
> > > <meenakshi.aggarwal@nxp.com>; Varun Sethi <V.Sethi@nxp.com>;
> > > devel@edk2.groups.io; Pankaj Bansal <pankaj.bansal@nxp.com>
> > > Subject: Re: [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq
> > Support
> > >
> > > Hi Ray,
> > >
> > > I agree it would be nice, but if not - this lets us get rid of
> > > otherwise needless driver duplication. To me, that makes it worth a
> > > little trivial churn.
> > >
> > > /
> > > Leif
> > >
> > > On Thu, Mar 19, 2020 at 15:09:19 +0000, Ni, Ray wrote:
> > > > It seems this change requires all platforms DSC to include the new
> > > > library
> > > class/instance.
> > > >
> > > > Is there a way that can avoid the platform impact?
> > > >
> > > > Thanks,
> > > > Ray
> > > >
> > > > > -----Original Message-----
> > > > > From: Pankaj Bansal <pankaj.bansal@oss.nxp.com>
> > > > > Sent: Wednesday, February 19, 2020 9:32 PM
> > > > > To: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Ma, Maurice
> > > > > <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>; You,
> > > Benjamin <benjamin.you@intel.com>; Leif Lindholm
> > > > > <leif@nuviainc.com>; Meenakshi Aggarwal
> > > <meenakshi.aggarwal@nxp.com>; Varun Sethi <V.Sethi@nxp.com>
> > > > > Cc: devel@edk2.groups.io; Pankaj Bansal <pankaj.bansal@nxp.com>
> > > > > Subject: [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq
> > Support
> > > > >
> > > > > From: Pankaj Bansal <pankaj.bansal@nxp.com>
> > > > >
> > > > > Some platform support dynamic clocking, which is controlled by
> > > > > some jumper setting or hardware registers. Result of that is that
> > > > > PCD PcdSerialClockRate would need to be updated for frequency
> > change.
> > > > >
> > > > > This patch implements support for dynamic frequency for Uart.
> > > > >
> > > > > This patch implements default lib, which is using Pcd. Platform
> > > > > which needs dynamic clocking needs implement SerialUartClockLib
> > > > >
> > > > > This patch is based on ArmPlatformPkg/Library/PL011UartClockLib
> > > > >
> > > > > Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> > > > > ---
> > > > > .../Include/Library/SerialUartClockLib.h | 21 +++++++++++++++
> > > > > .../BaseSerialPortLib16550.c | 9 ++++---
> > > > > .../BaseSerialPortLib16550.inf | 2 +-
> > > > > .../BaseSerialUartClockLib.c | 24 +++++++++++++++++
> > > > > .../BaseSerialUartClockLib.inf | 27 +++++++++++++++++++
> > > > > MdeModulePkg/MdeModulePkg.dsc | 2 ++
> > > > > SourceLevelDebugPkg/SourceLevelDebugPkg.dsc | 1 +
> > > > > UefiPayloadPkg/UefiPayloadPkgIa32.dsc | 1 +
> > > > > UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc | 1 +
> > > > > 9 files changed, 83 insertions(+), 5 deletions(-) create mode
> > > > > 100644 MdeModulePkg/Include/Library/SerialUartClockLib.h
> > > > > create mode 100644
> > > MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.c
> > > > > create mode 100644
> > > MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
> > > > >
> > > > > diff --git a/MdeModulePkg/Include/Library/SerialUartClockLib.h
> > > b/MdeModulePkg/Include/Library/SerialUartClockLib.h
> > > > > new file mode 100644
> > > > > index 000000000000..b6b16f71d4cf
> > > > > --- /dev/null
> > > > > +++ b/MdeModulePkg/Include/Library/SerialUartClockLib.h
> > > > > @@ -0,0 +1,21 @@
> > > > > +/** @file
> > > > > +*
> > > > > +* Copyright 2020 NXP
> > > > > +*
> > > > > +* SPDX-License-Identifier: BSD-2-Clause-Patent
> > > > > +*
> > > > > +**/
> > > > > +
> > > > > +#ifndef SERIAL_UART_CLOCK_LIB_H__ #define
> > > > > +SERIAL_UART_CLOCK_LIB_H__
> > > > > +
> > > > > +/**
> > > > > + Return clock in for Uart IP
> > > > > +**/
> > > > > +UINT32
> > > > > +EFIAPI
> > > > > +BaseSerialPortGetClock (
> > > > > + VOID
> > > > > + );
> > > > > +
> > > > > +#endif
> > > > > diff --git
> > >
> > a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c
> > > > >
> > b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib165
> > > > > 50.c index 9cb50dd80d56..2e0c05d5789e 100644
> > > > > ---
> > >
> > a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c
> > > > > +++
> > >
> > b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.c
> > > > > @@ -16,6 +16,7 @@
> > > > > #include <Library/IoLib.h>
> > > > > #include <Library/PciLib.h>
> > > > > #include <Library/PlatformHookLib.h>
> > > > > +#include <Library/SerialUartClockLib.h>
> > > > > #include <Library/BaseLib.h>
> > > > >
> > > > > //
> > > > > @@ -501,8 +502,8 @@ SerialPortInitialize (
> > > > > // Calculate divisor for baud generator
> > > > > // Ref_Clk_Rate / Baud_Rate / 16
> > > > > //
> > > > > - Divisor = PcdGet32 (PcdSerialClockRate) / (PcdGet32
> > > > > (PcdSerialBaudRate) *
> > > 16);
> > > > > - if ((PcdGet32 (PcdSerialClockRate) % (PcdGet32
> > > > > (PcdSerialBaudRate) *
> > > 16)) >= PcdGet32 (PcdSerialBaudRate) * 8) {
> > > > > + Divisor = BaseSerialPortGetClock () / (PcdGet32
> > > > > + (PcdSerialBaudRate) * 16); if ((BaseSerialPortGetClock () %
> > > > > + (PcdGet32 (PcdSerialBaudRate) * 16)) >=
> > > PcdGet32 (PcdSerialBaudRate) * 8) {
> > > > > Divisor++;
> > > > > }
> > > > >
> > > > > @@ -1080,8 +1081,8 @@ SerialPortSetAttributes (
> > > > > // Calculate divisor for baud generator
> > > > > // Ref_Clk_Rate / Baud_Rate / 16
> > > > > //
> > > > > - Divisor = PcdGet32 (PcdSerialClockRate) / (SerialBaudRate *
> > > > > 16);
> > > > > - if ((PcdGet32 (PcdSerialClockRate) % (SerialBaudRate * 16)) >=
> > > SerialBaudRate * 8) {
> > > > > + Divisor = BaseSerialPortGetClock () / (SerialBaudRate * 16);
> > > > > + if ((BaseSerialPortGetClock () % (SerialBaudRate * 16)) >=
> > > > > + SerialBaudRate *
> > > 8) {
> > > > > Divisor++;
> > > > > }
> > > > >
> > > > > diff --git
> > > a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.i
> > > nf
> > > > >
> > > b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.i
> > > nf
> > > > > index 8b4ae3f1d4ee..b4c16504f211 100644
> > > > > ---
> > > a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.i
> > > nf
> > > > > +++
> > > b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.i
> > > nf
> > > > > @@ -24,6 +24,7 @@ [LibraryClasses]
> > > > > IoLib
> > > > > PlatformHookLib
> > > > > PciLib
> > > > > + SerialUartClockLib
> > > > >
> > > > > [Sources]
> > > > > BaseSerialPortLib16550.c
> > > > > @@ -37,7 +38,6 @@ [Pcd]
> > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialBaudRate ##
> > > CONSUMES
> > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialLineControl ##
> > > CONSUMES
> > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialFifoControl ##
> > > CONSUMES
> > > > > - gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate ##
> > > CONSUMES
> > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialPciDeviceInfo ##
> > > CONSUMES
> > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialExtendedTxFifoSize
> > ##
> > > CONSUMES
> > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterStride ##
> > > CONSUMES
> > > > > diff --git
> > > a/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.c
> > > > > b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockL
> > > > > ib.c
> > > > > new file mode 100644
> > > > > index 000000000000..7a0d0427cc4e
> > > > > --- /dev/null
> > > > > +++
> > > b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.c
> > > > > @@ -0,0 +1,24 @@
> > > > > +/** @file
> > > > > +*
> > > > > +* Copyright 2020 NXP
> > > > > +*
> > > > > +* SPDX-License-Identifier: BSD-2-Clause-Patent
> > > > > +*
> > > > > +**/
> > > > > +
> > > > > +#include <Base.h>
> > > > > +#include <Library/PcdLib.h>
> > > > > +
> > > > > +/**
> > > > > + Return clock in for Uart IP
> > > > > +
> > > > > + @return Pcd PcdSerialClockRate
> > > > > +**/
> > > > > +UINT32
> > > > > +EFIAPI
> > > > > +BaseSerialPortGetClock (
> > > > > + VOID
> > > > > + )
> > > > > +{
> > > > > + return PcdGet32 (PcdSerialClockRate); }
> > > > > diff --git
> > > a/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.i
> > > nf
> > > > >
> > > b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.i
> > > nf
> > > > > new file mode 100644
> > > > > index 000000000000..91ba69436ed6
> > > > > --- /dev/null
> > > > > +++
> > > b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.i
> > > nf
> > > > > @@ -0,0 +1,27 @@
> > > > > +#/* @file
> > > > > +# Copyright 2020 NXP
> > > > > +#
> > > > > +# SPDX-License-Identifier: BSD-2-Clause-Patent # #*/
> > > > > +
> > > > > +[Defines]
> > > > > + INF_VERSION = 0x0001001A
> > > > > + BASE_NAME = BaseSerialUartClockLib
> > > > > + FILE_GUID = fa65495e-d3c8-4ea3-9737-994e9ccbaf11
> > > > > + MODULE_TYPE = BASE
> > > > > + VERSION_STRING = 1.0
> > > > > + LIBRARY_CLASS = SerialUartClockLib
> > > > > +
> > > > > +[Packages]
> > > > > + MdePkg/MdePkg.dec
> > > > > + MdeModulePkg/MdeModulePkg.dec
> > > > > +
> > > > > +[Sources.common]
> > > > > + BaseSerialUartClockLib.c
> > > > > +
> > > > > +[LibraryClasses]
> > > > > + PcdLib
> > > > > +
> > > > > +[Pcd]
> > > > > + gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate ##
> > > CONSUMES
> > > > > diff --git a/MdeModulePkg/MdeModulePkg.dsc
> > > b/MdeModulePkg/MdeModulePkg.dsc
> > > > > index f7dbb27ce25d..d581ca797b3b 100644
> > > > > --- a/MdeModulePkg/MdeModulePkg.dsc
> > > > > +++ b/MdeModulePkg/MdeModulePkg.dsc
> > > > > @@ -65,6 +65,7 @@ [LibraryClasses]
> > > > > UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
> > > > >
> > >
> > SecurityManagementLib|MdeModulePkg/Library/DxeSecurityManagementL
> > ib/D
> > > xeSecurityManagementLib.inf
> > > > >
> > >
> > TimerLib|MdePkg/Library/BaseTimerLibNullTemplate/BaseTimerLibNullTem
> > pl
> > > TimerLib|at
> > > e.inf
> > > > > +
> > >
> > SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSer
> > > SerialUartClockLib|ialU
> > > artClockLib.inf
> > > > >
> > > SerialPortLib|MdePkg/Library/BaseSerialPortLibNull/BaseSerialPortLibNu
> > > SerialPortLib|ll.inf
> > > > >
> > >
> > CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.i
> > n
> > > CapsuleLib|f
> > > > > PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
> > > > > @@ -292,6 +293,7 @@ [Components]
> > > > >
> > >
> > MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.
> > inf
> > > > >
> > >
> > MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/RuntimeDxeRep
> > ortSt
> > > atusCodeLib.inf
> > > > >
> > >
> > MdeModulePkg/Library/RuntimeResetSystemLib/RuntimeResetSystemLib.i
> > nf
> > > > > +
> > > MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib.inf
> > > > >
> > > MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
> > > > >
> > >
> > MdeModulePkg/Library/BasePlatformHookLibNull/BasePlatformHookLibNull
> > .i
> > > nf
> > > > >
> > >
> > MdeModulePkg/Library/DxeDebugPrintErrorLevelLib/DxeDebugPrintErrorLe
> > ve
> > > lLi
> > > b.inf
> > > > > diff --git a/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc
> > > b/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc
> > > > > index a1a1b81d03cb..c0ad88f26341 100644
> > > > > --- a/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc
> > > > > +++ b/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc
> > > > > @@ -33,6 +33,7 @@ [LibraryClasses.common]
> > > > >
> > >
> > SynchronizationLib|MdePkg/Library/BaseSynchronizationLib/BaseSynchroni
> > > SynchronizationLib|zati
> > > onLib.inf
> > > > > LocalApicLib|UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.inf
> > > > >
> > >
> > PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/Base
> > P
> > > PeCoffGetEntryPointLib|eC
> > > offGetEntryPointLib.inf
> > > > > +
> > >
> > SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSer
> > > SerialUartClockLib|ialU
> > > artClockLib.inf
> > > > >
> > > SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPo
> > > SerialPortLib|rtLib
> > > 16550.inf
> > > > >
> > >
> > PeCoffExtraActionLib|SourceLevelDebugPkg/Library/PeCoffExtraActionLibD
> > > PeCoffExtraActionLib|ebu
> > > g/PeCoffExtraActionLibDebug.inf
> > > > >
> > >
> > TimerLib|UefiCpuPkg/Library/SecPeiDxeTimerLibUefiCpu/SecPeiDxeTimerLi
> > b
> > > TimerLib|Ue
> > > fiCpu.inf
> > > > > diff --git a/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
> > > b/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
> > > > > index d52945442e0e..a556a32b272c 100644
> > > > > --- a/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
> > > > > +++ b/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
> > > > > @@ -174,6 +174,7 @@ [LibraryClasses]
> > > > > #
> > > > > TimerLib|UefiPayloadPkg/Library/AcpiTimerLib/AcpiTimerLib.inf
> > > > >
> > >
> > ResetSystemLib|UefiPayloadPkg/Library/ResetSystemLib/ResetSystemLib.in
> > > ResetSystemLib|f
> > > > > +
> > >
> > SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSer
> > > SerialUartClockLib|ialU
> > > artClockLib.inf
> > > > >
> > > SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPo
> > > SerialPortLib|rtLib
> > > 16550.inf
> > > > >
> > >
> > PlatformHookLib|UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib
> > > PlatformHookLib|.in
> > > f
> > > > >
> > >
> > PlatformBootManagerLib|UefiPayloadPkg/Library/PlatformBootManagerLib/
> > P
> > > PlatformBootManagerLib|l
> > > atformBootManagerLib.inf
> > > > > diff --git a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > > b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > > > > index 0736cd995476..7e86375fe57d 100644
> > > > > --- a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > > > > +++ b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > > > > @@ -175,6 +175,7 @@ [LibraryClasses]
> > > > > #
> > > > > TimerLib|UefiPayloadPkg/Library/AcpiTimerLib/AcpiTimerLib.inf
> > > > >
> > >
> > ResetSystemLib|UefiPayloadPkg/Library/ResetSystemLib/ResetSystemLib.in
> > > ResetSystemLib|f
> > > > > +
> > >
> > SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseSer
> > > SerialUartClockLib|ialU
> > > artClockLib.inf
> > > > >
> > > SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPo
> > > SerialPortLib|rtLib
> > > 16550.inf
> > > > >
> > >
> > PlatformHookLib|UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib
> > > PlatformHookLib|.in
> > > f
> > > > >
> > >
> > PlatformBootManagerLib|UefiPayloadPkg/Library/PlatformBootManagerLib/
> > P
> > > PlatformBootManagerLib|l
> > > atformBootManagerLib.inf
> > > > > --
> > > > > 2.17.1
> > > >
> >
> >
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq Support
2020-03-28 12:36 ` Pankaj Bansal
@ 2020-03-30 1:20 ` Guomin Jiang
2020-03-30 7:35 ` Leif Lindholm
0 siblings, 1 reply; 18+ messages in thread
From: Guomin Jiang @ 2020-03-30 1:20 UTC (permalink / raw)
To: devel@edk2.groups.io, pankaj.bansal@nxp.com, Leif Lindholm,
Ni, Ray, Ard Biesheuvel
Cc: Wang, Jian J, Wu, Hao A, Ma, Maurice, Dong, Guo, You, Benjamin,
Meenakshi Aggarwal, Varun Sethi, Samer El-Haj-Mahmoud
Hi Pankaj,
I know your consideration.
My consideration is that we provide an interface, it should be better for user, it not, the user won't use it and if no nobody use it, why provide it?
In other word, the effort for copying the SerialPortLib and the effort for implementing SerialUartClockLib are same, why consumer should learn the new interface?
Thanks.
> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Pankaj Bansal
> Sent: Saturday, March 28, 2020 8:37 PM
> To: Jiang, Guomin <guomin.jiang@intel.com>; devel@edk2.groups.io; Pankaj
> Bansal <pankaj.bansal@nxp.com>; Leif Lindholm <leif@nuviainc.com>; Ni,
> Ray <ray.ni@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Ma, Maurice <maurice.ma@intel.com>; Dong, Guo
> <guo.dong@intel.com>; You, Benjamin <benjamin.you@intel.com>;
> Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; Varun Sethi
> <V.Sethi@nxp.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-
> Mahmoud@arm.com>
> Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: UART Dynamic clock
> freq Support
>
> Hello Jiang,
>
> > -----Original Message-----
> > From: Jiang, Guomin <guomin.jiang@intel.com>
> > Sent: Thursday, March 26, 2020 7:44 PM
> > To: devel@edk2.groups.io; Pankaj Bansal <pankaj.bansal@nxp.com>; Leif
> > Lindholm <leif@nuviainc.com>; Ni, Ray <ray.ni@intel.com>
> > Cc: Pankaj Bansal (OSS) <pankaj.bansal@oss.nxp.com>; Wang, Jian J
> > <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ma, Maurice
> > <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>; You,
> Benjamin
> > <benjamin.you@intel.com>; Meenakshi Aggarwal
> > <meenakshi.aggarwal@nxp.com>; Varun Sethi <V.Sethi@nxp.com>
> > Subject: RE: [edk2-devel] [PATCH 1/1] MdeModulePkg: UART Dynamic
> clock
> > freq Support
> >
> > It is a good idea,
> >
> > I have a question:
> > 1. When can detect the jumper signal or register?
> -> This depends on platform. Ideally as early as possible in boot
> process (in SEC phase), because PEI Core and onwards, all modules start
> making call to DebugLib constructor.
> > 2. If the jumper use GPIO, SerialUartClockLib will depend on GpioLib?
> -> Yes, SerialUartClockLib can depend on other libraries as well. E.g. in
> https://edk2.groups.io/g/devel/message/56009, SerialUartClockLib depends
> on ArmPlatformLib
> > 3. If the register is inside SIO, how to dispose the dependence?
> -> I did not understand term SIO. Quick google search throws up Intel
> Serial IO host controller driver. Intel Serial IO driver is required if you plan to
> use the I2C, UART, or GPIO host controllers.
> Is that what you are referring to ? I assume you mean that what
> happens if the SerialUartClockLib, tries to read registers that require
> SerialPortLib ? i.e. circular dependency i.e deadlock ?
> In that case, some kind of intervention is required on Platform
> owner part. May be implement a version of SerialUartClockLib for SEC phase
> that doesn't depend on SerialPortLib, which lets
> the serial port to initialize. For all other module types, implement
> another version of SerialUartClockLib.
> >
> > If it is too complex to implement the SerialUartClockLib, the project
> > owner will choose use BaseSerialUartClockLib rather implement it. and
> > in this case, it make no sense after import complexity.
>
> Complexity is platform specific. We have implemented
> BaseSerialUartClockLib in https://edk2.groups.io/g/devel/message/56009.
>
> >
> > I have also reviewed the ArmPlatformPkg/Library/PL011UartClockLib
> > code, it still use the fixed data rather than dynamically detect clock.
>
> Right. As I said, making PcdSerialClockRate as Dynamic PCD, we cannot use
> this in modules for which PcdLib is BasePcdLibNull.
> It results in ASSERT.
>
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > Pankaj Bansal
> > > Sent: Monday, March 23, 2020 1:31 PM
> > > To: Leif Lindholm <leif@nuviainc.com>; Ni, Ray <ray.ni@intel.com>
> > > Cc: Pankaj Bansal (OSS) <pankaj.bansal@oss.nxp.com>; Wang, Jian J
> > > <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ma,
> Maurice
> > > <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>; You,
> > > Benjamin <benjamin.you@intel.com>; Meenakshi Aggarwal
> > > <meenakshi.aggarwal@nxp.com>; Varun Sethi <V.Sethi@nxp.com>;
> > > devel@edk2.groups.io
> > > Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: UART Dynamic
> > > clock freq Support
> > >
> > > Hi Ray,
> > >
> > > I had thought of making PcdSerialClockRate as Dynamic PCD I had made
> > > changes for it too.
> > > But it doesn't work.
> > > SerialPortInitalize, which uses PcdSerialClockRate, gets called from
> > > DebugLib constructor.
> > > DebugLib is used by every module to print any info onto console.
> > > For DxeCore and PcdDxe, for which PcdLib instance is NULL, this
> > > results in Assert :
> > >
> > >
> https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BasePcd
> > > Li
> > > bNull/PcdLib.c#L123
> > >
> > > The other approach that I thought of was, to copy
> > > BaseSerialPortLib16550 for our platform and simply return EFI_SUCCESS
> from SerialPortInitalize.
> > > But as Leif pointed out, this results in code duplication.
> > >
> > > Regards,
> > > Pankaj Bansal
> > >
> > > > -----Original Message-----
> > > > From: Leif Lindholm <leif@nuviainc.com>
> > > > Sent: Thursday, March 19, 2020 9:01 PM
> > > > To: Ni, Ray <ray.ni@intel.com>
> > > > Cc: Pankaj Bansal (OSS) <pankaj.bansal@oss.nxp.com>; Wang, Jian J
> > > > <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ma,
> > > > Maurice <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>;
> > > > You,
> > > Benjamin
> > > > <benjamin.you@intel.com>; Meenakshi Aggarwal
> > > > <meenakshi.aggarwal@nxp.com>; Varun Sethi <V.Sethi@nxp.com>;
> > > > devel@edk2.groups.io; Pankaj Bansal <pankaj.bansal@nxp.com>
> > > > Subject: Re: [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq
> > > Support
> > > >
> > > > Hi Ray,
> > > >
> > > > I agree it would be nice, but if not - this lets us get rid of
> > > > otherwise needless driver duplication. To me, that makes it worth
> > > > a little trivial churn.
> > > >
> > > > /
> > > > Leif
> > > >
> > > > On Thu, Mar 19, 2020 at 15:09:19 +0000, Ni, Ray wrote:
> > > > > It seems this change requires all platforms DSC to include the
> > > > > new library
> > > > class/instance.
> > > > >
> > > > > Is there a way that can avoid the platform impact?
> > > > >
> > > > > Thanks,
> > > > > Ray
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Pankaj Bansal <pankaj.bansal@oss.nxp.com>
> > > > > > Sent: Wednesday, February 19, 2020 9:32 PM
> > > > > > To: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > > > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Ma, Maurice
> > > > > > <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>; You,
> > > > Benjamin <benjamin.you@intel.com>; Leif Lindholm
> > > > > > <leif@nuviainc.com>; Meenakshi Aggarwal
> > > > <meenakshi.aggarwal@nxp.com>; Varun Sethi <V.Sethi@nxp.com>
> > > > > > Cc: devel@edk2.groups.io; Pankaj Bansal
> > > > > > <pankaj.bansal@nxp.com>
> > > > > > Subject: [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq
> > > Support
> > > > > >
> > > > > > From: Pankaj Bansal <pankaj.bansal@nxp.com>
> > > > > >
> > > > > > Some platform support dynamic clocking, which is controlled by
> > > > > > some jumper setting or hardware registers. Result of that is
> > > > > > that PCD PcdSerialClockRate would need to be updated for
> > > > > > frequency
> > > change.
> > > > > >
> > > > > > This patch implements support for dynamic frequency for Uart.
> > > > > >
> > > > > > This patch implements default lib, which is using Pcd.
> > > > > > Platform which needs dynamic clocking needs implement
> > > > > > SerialUartClockLib
> > > > > >
> > > > > > This patch is based on
> > > > > > ArmPlatformPkg/Library/PL011UartClockLib
> > > > > >
> > > > > > Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> > > > > > ---
> > > > > > .../Include/Library/SerialUartClockLib.h | 21 +++++++++++++++
> > > > > > .../BaseSerialPortLib16550.c | 9 ++++---
> > > > > > .../BaseSerialPortLib16550.inf | 2 +-
> > > > > > .../BaseSerialUartClockLib.c | 24 +++++++++++++++++
> > > > > > .../BaseSerialUartClockLib.inf | 27 +++++++++++++++++++
> > > > > > MdeModulePkg/MdeModulePkg.dsc | 2 ++
> > > > > > SourceLevelDebugPkg/SourceLevelDebugPkg.dsc | 1 +
> > > > > > UefiPayloadPkg/UefiPayloadPkgIa32.dsc | 1 +
> > > > > > UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc | 1 +
> > > > > > 9 files changed, 83 insertions(+), 5 deletions(-) create
> > > > > > mode
> > > > > > 100644 MdeModulePkg/Include/Library/SerialUartClockLib.h
> > > > > > create mode 100644
> > > > MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib
> > > > .c
> > > > > > create mode 100644
> > > > MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib
> > > > .inf
> > > > > >
> > > > > > diff --git a/MdeModulePkg/Include/Library/SerialUartClockLib.h
> > > > b/MdeModulePkg/Include/Library/SerialUartClockLib.h
> > > > > > new file mode 100644
> > > > > > index 000000000000..b6b16f71d4cf
> > > > > > --- /dev/null
> > > > > > +++ b/MdeModulePkg/Include/Library/SerialUartClockLib.h
> > > > > > @@ -0,0 +1,21 @@
> > > > > > +/** @file
> > > > > > +*
> > > > > > +* Copyright 2020 NXP
> > > > > > +*
> > > > > > +* SPDX-License-Identifier: BSD-2-Clause-Patent
> > > > > > +*
> > > > > > +**/
> > > > > > +
> > > > > > +#ifndef SERIAL_UART_CLOCK_LIB_H__ #define
> > > > > > +SERIAL_UART_CLOCK_LIB_H__
> > > > > > +
> > > > > > +/**
> > > > > > + Return clock in for Uart IP **/
> > > > > > +UINT32
> > > > > > +EFIAPI
> > > > > > +BaseSerialPortGetClock (
> > > > > > + VOID
> > > > > > + );
> > > > > > +
> > > > > > +#endif
> > > > > > diff --git
> > > >
> > >
> a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550
> > > .c
> > > > > >
> > > b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib165
> > > > > > 50.c index 9cb50dd80d56..2e0c05d5789e 100644
> > > > > > ---
> > > >
> > >
> a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550
> > > .c
> > > > > > +++
> > > >
> > >
> b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550
> > > .c
> > > > > > @@ -16,6 +16,7 @@
> > > > > > #include <Library/IoLib.h>
> > > > > > #include <Library/PciLib.h>
> > > > > > #include <Library/PlatformHookLib.h>
> > > > > > +#include <Library/SerialUartClockLib.h>
> > > > > > #include <Library/BaseLib.h>
> > > > > >
> > > > > > //
> > > > > > @@ -501,8 +502,8 @@ SerialPortInitialize (
> > > > > > // Calculate divisor for baud generator
> > > > > > // Ref_Clk_Rate / Baud_Rate / 16
> > > > > > //
> > > > > > - Divisor = PcdGet32 (PcdSerialClockRate) / (PcdGet32
> > > > > > (PcdSerialBaudRate) *
> > > > 16);
> > > > > > - if ((PcdGet32 (PcdSerialClockRate) % (PcdGet32
> > > > > > (PcdSerialBaudRate) *
> > > > 16)) >= PcdGet32 (PcdSerialBaudRate) * 8) {
> > > > > > + Divisor = BaseSerialPortGetClock () / (PcdGet32
> > > > > > + (PcdSerialBaudRate) * 16); if ((BaseSerialPortGetClock () %
> > > > > > + (PcdGet32 (PcdSerialBaudRate) * 16)) >=
> > > > PcdGet32 (PcdSerialBaudRate) * 8) {
> > > > > > Divisor++;
> > > > > > }
> > > > > >
> > > > > > @@ -1080,8 +1081,8 @@ SerialPortSetAttributes (
> > > > > > // Calculate divisor for baud generator
> > > > > > // Ref_Clk_Rate / Baud_Rate / 16
> > > > > > //
> > > > > > - Divisor = PcdGet32 (PcdSerialClockRate) / (SerialBaudRate *
> > > > > > 16);
> > > > > > - if ((PcdGet32 (PcdSerialClockRate) % (SerialBaudRate * 16))
> > > > > > >=
> > > > SerialBaudRate * 8) {
> > > > > > + Divisor = BaseSerialPortGetClock () / (SerialBaudRate *
> > > > > > + 16); if ((BaseSerialPortGetClock () % (SerialBaudRate * 16))
> > > > > > + >= SerialBaudRate *
> > > > 8) {
> > > > > > Divisor++;
> > > > > > }
> > > > > >
> > > > > > diff --git
> > > > a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib165
> > > > 50.i
> > > > nf
> > > > > >
> > > >
> b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib165
> > > > 50.i
> > > > nf
> > > > > > index 8b4ae3f1d4ee..b4c16504f211 100644
> > > > > > ---
> > > > a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib165
> > > > 50.i
> > > > nf
> > > > > > +++
> > > >
> b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib165
> > > > 50.i
> > > > nf
> > > > > > @@ -24,6 +24,7 @@ [LibraryClasses]
> > > > > > IoLib
> > > > > > PlatformHookLib
> > > > > > PciLib
> > > > > > + SerialUartClockLib
> > > > > >
> > > > > > [Sources]
> > > > > > BaseSerialPortLib16550.c
> > > > > > @@ -37,7 +38,6 @@ [Pcd]
> > > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialBaudRate ##
> > > > CONSUMES
> > > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialLineControl
> ##
> > > > CONSUMES
> > > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialFifoControl ##
> > > > CONSUMES
> > > > > > - gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate ##
> > > > CONSUMES
> > > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialPciDeviceInfo
> ##
> > > > CONSUMES
> > > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialExtendedTxFifoSize
> > > ##
> > > > CONSUMES
> > > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterStride
> ##
> > > > CONSUMES
> > > > > > diff --git
> > > > a/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockL
> > > > ib.c
> > > > > > b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartCl
> > > > > > ockL
> > > > > > ib.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..7a0d0427cc4e
> > > > > > --- /dev/null
> > > > > > +++
> > > > b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockL
> > > > ib.c
> > > > > > @@ -0,0 +1,24 @@
> > > > > > +/** @file
> > > > > > +*
> > > > > > +* Copyright 2020 NXP
> > > > > > +*
> > > > > > +* SPDX-License-Identifier: BSD-2-Clause-Patent
> > > > > > +*
> > > > > > +**/
> > > > > > +
> > > > > > +#include <Base.h>
> > > > > > +#include <Library/PcdLib.h>
> > > > > > +
> > > > > > +/**
> > > > > > + Return clock in for Uart IP
> > > > > > +
> > > > > > + @return Pcd PcdSerialClockRate **/
> > > > > > +UINT32
> > > > > > +EFIAPI
> > > > > > +BaseSerialPortGetClock (
> > > > > > + VOID
> > > > > > + )
> > > > > > +{
> > > > > > + return PcdGet32 (PcdSerialClockRate); }
> > > > > > diff --git
> > > > a/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockL
> > > > ib.i
> > > > nf
> > > > > >
> > > > b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockL
> > > > ib.i
> > > > nf
> > > > > > new file mode 100644
> > > > > > index 000000000000..91ba69436ed6
> > > > > > --- /dev/null
> > > > > > +++
> > > > b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockL
> > > > ib.i
> > > > nf
> > > > > > @@ -0,0 +1,27 @@
> > > > > > +#/* @file
> > > > > > +# Copyright 2020 NXP
> > > > > > +#
> > > > > > +# SPDX-License-Identifier: BSD-2-Clause-Patent # #*/
> > > > > > +
> > > > > > +[Defines]
> > > > > > + INF_VERSION = 0x0001001A
> > > > > > + BASE_NAME = BaseSerialUartClockLib
> > > > > > + FILE_GUID = fa65495e-d3c8-4ea3-9737-994e9ccbaf11
> > > > > > + MODULE_TYPE = BASE
> > > > > > + VERSION_STRING = 1.0
> > > > > > + LIBRARY_CLASS = SerialUartClockLib
> > > > > > +
> > > > > > +[Packages]
> > > > > > + MdePkg/MdePkg.dec
> > > > > > + MdeModulePkg/MdeModulePkg.dec
> > > > > > +
> > > > > > +[Sources.common]
> > > > > > + BaseSerialUartClockLib.c
> > > > > > +
> > > > > > +[LibraryClasses]
> > > > > > + PcdLib
> > > > > > +
> > > > > > +[Pcd]
> > > > > > + gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate
> ##
> > > > CONSUMES
> > > > > > diff --git a/MdeModulePkg/MdeModulePkg.dsc
> > > > b/MdeModulePkg/MdeModulePkg.dsc
> > > > > > index f7dbb27ce25d..d581ca797b3b 100644
> > > > > > --- a/MdeModulePkg/MdeModulePkg.dsc
> > > > > > +++ b/MdeModulePkg/MdeModulePkg.dsc
> > > > > > @@ -65,6 +65,7 @@ [LibraryClasses]
> > > > > > UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
> > > > > >
> > > >
> > >
> SecurityManagementLib|MdeModulePkg/Library/DxeSecurityManagementL
> > > ib/D
> > > > xeSecurityManagementLib.inf
> > > > > >
> > > >
> > >
> TimerLib|MdePkg/Library/BaseTimerLibNullTemplate/BaseTimerLibNullTem
> > > pl
> > > > TimerLib|at
> > > > e.inf
> > > > > > +
> > > >
> > > SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseS
> > > SerialUartClockLib|er
> > > > SerialUartClockLib|ialU
> > > > artClockLib.inf
> > > > > >
> > > > SerialPortLib|MdePkg/Library/BaseSerialPortLibNull/BaseSerialPortL
> > > > SerialPortLib|ibNu
> > > > SerialPortLib|ll.inf
> > > > > >
> > > >
> > >
> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.
> > > CapsuleLib|i
> > > n
> > > > CapsuleLib|f
> > > > > > PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
> > > > > > @@ -292,6 +293,7 @@ [Components]
> > > > > >
> > > >
> > >
> MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.
> > > inf
> > > > > >
> > > >
> > >
> MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/RuntimeDxeRep
> > > ortSt
> > > > atusCodeLib.inf
> > > > > >
> > > >
> > >
> MdeModulePkg/Library/RuntimeResetSystemLib/RuntimeResetSystemLib.i
> > > nf
> > > > > > +
> > > > MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib
> > > > .inf
> > > > > >
> > > >
> MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550
> > > > .inf
> > > > > >
> > > >
> > >
> MdeModulePkg/Library/BasePlatformHookLibNull/BasePlatformHookLibNull
> > > .i
> > > > nf
> > > > > >
> > > >
> > >
> MdeModulePkg/Library/DxeDebugPrintErrorLevelLib/DxeDebugPrintErrorLe
> > > ve
> > > > lLi
> > > > b.inf
> > > > > > diff --git a/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc
> > > > b/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc
> > > > > > index a1a1b81d03cb..c0ad88f26341 100644
> > > > > > --- a/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc
> > > > > > +++ b/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc
> > > > > > @@ -33,6 +33,7 @@ [LibraryClasses.common]
> > > > > >
> > > >
> > >
> SynchronizationLib|MdePkg/Library/BaseSynchronizationLib/BaseSynchro
> > > SynchronizationLib|ni
> > > > SynchronizationLib|zati
> > > > onLib.inf
> > > > > >
> > > > > > LocalApicLib|UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.inf
> > > > > >
> > > >
> > >
> PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/Bas
> > > PeCoffGetEntryPointLib|e
> > > P
> > > > PeCoffGetEntryPointLib|eC
> > > > offGetEntryPointLib.inf
> > > > > > +
> > > >
> > > SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseS
> > > SerialUartClockLib|er
> > > > SerialUartClockLib|ialU
> > > > artClockLib.inf
> > > > > >
> > > > SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSeri
> > > > SerialPortLib|alPo
> > > > SerialPortLib|rtLib
> > > > 16550.inf
> > > > > >
> > > >
> > > PeCoffExtraActionLib|SourceLevelDebugPkg/Library/PeCoffExtraActionLi
> > > PeCoffExtraActionLib|bD
> > > > PeCoffExtraActionLib|ebu
> > > > g/PeCoffExtraActionLibDebug.inf
> > > > > >
> > > >
> > >
> TimerLib|UefiCpuPkg/Library/SecPeiDxeTimerLibUefiCpu/SecPeiDxeTimerL
> > > TimerLib|i
> > > b
> > > > TimerLib|Ue
> > > > fiCpu.inf
> > > > > > diff --git a/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
> > > > b/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
> > > > > > index d52945442e0e..a556a32b272c 100644
> > > > > > --- a/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
> > > > > > +++ b/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
> > > > > > @@ -174,6 +174,7 @@ [LibraryClasses]
> > > > > > #
> > > > > >
> > > > > > TimerLib|UefiPayloadPkg/Library/AcpiTimerLib/AcpiTimerLib.inf
> > > > > >
> > > >
> > >
> ResetSystemLib|UefiPayloadPkg/Library/ResetSystemLib/ResetSystemLib.
> > > ResetSystemLib|in
> > > > ResetSystemLib|f
> > > > > > +
> > > >
> > > SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseS
> > > SerialUartClockLib|er
> > > > SerialUartClockLib|ialU
> > > > artClockLib.inf
> > > > > >
> > > > SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSeri
> > > > SerialPortLib|alPo
> > > > SerialPortLib|rtLib
> > > > 16550.inf
> > > > > >
> > > >
> > >
> PlatformHookLib|UefiPayloadPkg/Library/PlatformHookLib/PlatformHookL
> > > PlatformHookLib|ib
> > > > PlatformHookLib|.in
> > > > f
> > > > > >
> > > >
> > >
> PlatformBootManagerLib|UefiPayloadPkg/Library/PlatformBootManagerLib
> > > PlatformBootManagerLib|/
> > > P
> > > > PlatformBootManagerLib|l
> > > > atformBootManagerLib.inf
> > > > > > diff --git a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > > > b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > > > > > index 0736cd995476..7e86375fe57d 100644
> > > > > > --- a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > > > > > +++ b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > > > > > @@ -175,6 +175,7 @@ [LibraryClasses]
> > > > > > #
> > > > > >
> > > > > > TimerLib|UefiPayloadPkg/Library/AcpiTimerLib/AcpiTimerLib.inf
> > > > > >
> > > >
> > >
> ResetSystemLib|UefiPayloadPkg/Library/ResetSystemLib/ResetSystemLib.
> > > ResetSystemLib|in
> > > > ResetSystemLib|f
> > > > > > +
> > > >
> > > SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseS
> > > SerialUartClockLib|er
> > > > SerialUartClockLib|ialU
> > > > artClockLib.inf
> > > > > >
> > > > SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSeri
> > > > SerialPortLib|alPo
> > > > SerialPortLib|rtLib
> > > > 16550.inf
> > > > > >
> > > >
> > >
> PlatformHookLib|UefiPayloadPkg/Library/PlatformHookLib/PlatformHookL
> > > PlatformHookLib|ib
> > > > PlatformHookLib|.in
> > > > f
> > > > > >
> > > >
> > >
> PlatformBootManagerLib|UefiPayloadPkg/Library/PlatformBootManagerLib
> > > PlatformBootManagerLib|/
> > > P
> > > > PlatformBootManagerLib|l
> > > > atformBootManagerLib.inf
> > > > > > --
> > > > > > 2.17.1
> > > > >
> > >
> > >
>
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq Support
2020-03-30 1:20 ` Guomin Jiang
@ 2020-03-30 7:35 ` Leif Lindholm
2020-03-30 7:44 ` Ard Biesheuvel
0 siblings, 1 reply; 18+ messages in thread
From: Leif Lindholm @ 2020-03-30 7:35 UTC (permalink / raw)
To: Jiang, Guomin
Cc: devel@edk2.groups.io, pankaj.bansal@nxp.com, Ni, Ray,
Ard Biesheuvel, Wang, Jian J, Wu, Hao A, Ma, Maurice, Dong, Guo,
You, Benjamin, Meenakshi Aggarwal, Varun Sethi,
Samer El-Haj-Mahmoud
Hi Jiang,
It is not a question of effort of copying a driver, it is a question
that copying drivers is something that should be avoided wherever
practically possible. I did not think this topic was still under
debate.
If the existing 16550 SerialPortLib is overspecialised to the point
where it only works on a subset of 16550 implementations, then it
should change. There are going to be more non-PC systems turning up
with 16550 UARTs - should they each copy/modify their drivers?
If there are better ways of solving that problem, please suggest.
But more duplicated drivers is not the answer.
/
Leif
On Mon, Mar 30, 2020 at 01:20:03 +0000, Jiang, Guomin wrote:
> Hi Pankaj,
>
> I know your consideration.
>
> My consideration is that we provide an interface, it should be
> better for user, it not, the user won't use it and if no nobody use
> it, why provide it?
> In other word, the effort for copying the SerialPortLib and the
> effort for implementing SerialUartClockLib are same, why consumer
> should learn the new interface?
>
> Thanks.
> > -----Original Message-----
> > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> > Pankaj Bansal
> > Sent: Saturday, March 28, 2020 8:37 PM
> > To: Jiang, Guomin <guomin.jiang@intel.com>; devel@edk2.groups.io; Pankaj
> > Bansal <pankaj.bansal@nxp.com>; Leif Lindholm <leif@nuviainc.com>; Ni,
> > Ray <ray.ni@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > <hao.a.wu@intel.com>; Ma, Maurice <maurice.ma@intel.com>; Dong, Guo
> > <guo.dong@intel.com>; You, Benjamin <benjamin.you@intel.com>;
> > Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; Varun Sethi
> > <V.Sethi@nxp.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-
> > Mahmoud@arm.com>
> > Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: UART Dynamic clock
> > freq Support
> >
> > Hello Jiang,
> >
> > > -----Original Message-----
> > > From: Jiang, Guomin <guomin.jiang@intel.com>
> > > Sent: Thursday, March 26, 2020 7:44 PM
> > > To: devel@edk2.groups.io; Pankaj Bansal <pankaj.bansal@nxp.com>; Leif
> > > Lindholm <leif@nuviainc.com>; Ni, Ray <ray.ni@intel.com>
> > > Cc: Pankaj Bansal (OSS) <pankaj.bansal@oss.nxp.com>; Wang, Jian J
> > > <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ma, Maurice
> > > <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>; You,
> > Benjamin
> > > <benjamin.you@intel.com>; Meenakshi Aggarwal
> > > <meenakshi.aggarwal@nxp.com>; Varun Sethi <V.Sethi@nxp.com>
> > > Subject: RE: [edk2-devel] [PATCH 1/1] MdeModulePkg: UART Dynamic
> > clock
> > > freq Support
> > >
> > > It is a good idea,
> > >
> > > I have a question:
> > > 1. When can detect the jumper signal or register?
> > -> This depends on platform. Ideally as early as possible in boot
> > process (in SEC phase), because PEI Core and onwards, all modules start
> > making call to DebugLib constructor.
> > > 2. If the jumper use GPIO, SerialUartClockLib will depend on GpioLib?
> > -> Yes, SerialUartClockLib can depend on other libraries as well. E.g. in
> > https://edk2.groups.io/g/devel/message/56009, SerialUartClockLib depends
> > on ArmPlatformLib
> > > 3. If the register is inside SIO, how to dispose the dependence?
> > -> I did not understand term SIO. Quick google search throws up Intel
> > Serial IO host controller driver. Intel Serial IO driver is required if you plan to
> > use the I2C, UART, or GPIO host controllers.
> > Is that what you are referring to ? I assume you mean that what
> > happens if the SerialUartClockLib, tries to read registers that require
> > SerialPortLib ? i.e. circular dependency i.e deadlock ?
> > In that case, some kind of intervention is required on Platform
> > owner part. May be implement a version of SerialUartClockLib for SEC phase
> > that doesn't depend on SerialPortLib, which lets
> > the serial port to initialize. For all other module types, implement
> > another version of SerialUartClockLib.
> > >
> > > If it is too complex to implement the SerialUartClockLib, the project
> > > owner will choose use BaseSerialUartClockLib rather implement it. and
> > > in this case, it make no sense after import complexity.
> >
> > Complexity is platform specific. We have implemented
> > BaseSerialUartClockLib in https://edk2.groups.io/g/devel/message/56009.
> >
> > >
> > > I have also reviewed the ArmPlatformPkg/Library/PL011UartClockLib
> > > code, it still use the fixed data rather than dynamically detect clock.
> >
> > Right. As I said, making PcdSerialClockRate as Dynamic PCD, we cannot use
> > this in modules for which PcdLib is BasePcdLibNull.
> > It results in ASSERT.
> >
> > >
> > > > -----Original Message-----
> > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > > Pankaj Bansal
> > > > Sent: Monday, March 23, 2020 1:31 PM
> > > > To: Leif Lindholm <leif@nuviainc.com>; Ni, Ray <ray.ni@intel.com>
> > > > Cc: Pankaj Bansal (OSS) <pankaj.bansal@oss.nxp.com>; Wang, Jian J
> > > > <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ma,
> > Maurice
> > > > <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>; You,
> > > > Benjamin <benjamin.you@intel.com>; Meenakshi Aggarwal
> > > > <meenakshi.aggarwal@nxp.com>; Varun Sethi <V.Sethi@nxp.com>;
> > > > devel@edk2.groups.io
> > > > Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: UART Dynamic
> > > > clock freq Support
> > > >
> > > > Hi Ray,
> > > >
> > > > I had thought of making PcdSerialClockRate as Dynamic PCD I had made
> > > > changes for it too.
> > > > But it doesn't work.
> > > > SerialPortInitalize, which uses PcdSerialClockRate, gets called from
> > > > DebugLib constructor.
> > > > DebugLib is used by every module to print any info onto console.
> > > > For DxeCore and PcdDxe, for which PcdLib instance is NULL, this
> > > > results in Assert :
> > > >
> > > >
> > https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BasePcd
> > > > Li
> > > > bNull/PcdLib.c#L123
> > > >
> > > > The other approach that I thought of was, to copy
> > > > BaseSerialPortLib16550 for our platform and simply return EFI_SUCCESS
> > from SerialPortInitalize.
> > > > But as Leif pointed out, this results in code duplication.
> > > >
> > > > Regards,
> > > > Pankaj Bansal
> > > >
> > > > > -----Original Message-----
> > > > > From: Leif Lindholm <leif@nuviainc.com>
> > > > > Sent: Thursday, March 19, 2020 9:01 PM
> > > > > To: Ni, Ray <ray.ni@intel.com>
> > > > > Cc: Pankaj Bansal (OSS) <pankaj.bansal@oss.nxp.com>; Wang, Jian J
> > > > > <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ma,
> > > > > Maurice <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>;
> > > > > You,
> > > > Benjamin
> > > > > <benjamin.you@intel.com>; Meenakshi Aggarwal
> > > > > <meenakshi.aggarwal@nxp.com>; Varun Sethi <V.Sethi@nxp.com>;
> > > > > devel@edk2.groups.io; Pankaj Bansal <pankaj.bansal@nxp.com>
> > > > > Subject: Re: [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq
> > > > Support
> > > > >
> > > > > Hi Ray,
> > > > >
> > > > > I agree it would be nice, but if not - this lets us get rid of
> > > > > otherwise needless driver duplication. To me, that makes it worth
> > > > > a little trivial churn.
> > > > >
> > > > > /
> > > > > Leif
> > > > >
> > > > > On Thu, Mar 19, 2020 at 15:09:19 +0000, Ni, Ray wrote:
> > > > > > It seems this change requires all platforms DSC to include the
> > > > > > new library
> > > > > class/instance.
> > > > > >
> > > > > > Is there a way that can avoid the platform impact?
> > > > > >
> > > > > > Thanks,
> > > > > > Ray
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Pankaj Bansal <pankaj.bansal@oss.nxp.com>
> > > > > > > Sent: Wednesday, February 19, 2020 9:32 PM
> > > > > > > To: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > > > > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Ma, Maurice
> > > > > > > <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>; You,
> > > > > Benjamin <benjamin.you@intel.com>; Leif Lindholm
> > > > > > > <leif@nuviainc.com>; Meenakshi Aggarwal
> > > > > <meenakshi.aggarwal@nxp.com>; Varun Sethi <V.Sethi@nxp.com>
> > > > > > > Cc: devel@edk2.groups.io; Pankaj Bansal
> > > > > > > <pankaj.bansal@nxp.com>
> > > > > > > Subject: [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq
> > > > Support
> > > > > > >
> > > > > > > From: Pankaj Bansal <pankaj.bansal@nxp.com>
> > > > > > >
> > > > > > > Some platform support dynamic clocking, which is controlled by
> > > > > > > some jumper setting or hardware registers. Result of that is
> > > > > > > that PCD PcdSerialClockRate would need to be updated for
> > > > > > > frequency
> > > > change.
> > > > > > >
> > > > > > > This patch implements support for dynamic frequency for Uart.
> > > > > > >
> > > > > > > This patch implements default lib, which is using Pcd.
> > > > > > > Platform which needs dynamic clocking needs implement
> > > > > > > SerialUartClockLib
> > > > > > >
> > > > > > > This patch is based on
> > > > > > > ArmPlatformPkg/Library/PL011UartClockLib
> > > > > > >
> > > > > > > Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> > > > > > > ---
> > > > > > > .../Include/Library/SerialUartClockLib.h | 21 +++++++++++++++
> > > > > > > .../BaseSerialPortLib16550.c | 9 ++++---
> > > > > > > .../BaseSerialPortLib16550.inf | 2 +-
> > > > > > > .../BaseSerialUartClockLib.c | 24 +++++++++++++++++
> > > > > > > .../BaseSerialUartClockLib.inf | 27 +++++++++++++++++++
> > > > > > > MdeModulePkg/MdeModulePkg.dsc | 2 ++
> > > > > > > SourceLevelDebugPkg/SourceLevelDebugPkg.dsc | 1 +
> > > > > > > UefiPayloadPkg/UefiPayloadPkgIa32.dsc | 1 +
> > > > > > > UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc | 1 +
> > > > > > > 9 files changed, 83 insertions(+), 5 deletions(-) create
> > > > > > > mode
> > > > > > > 100644 MdeModulePkg/Include/Library/SerialUartClockLib.h
> > > > > > > create mode 100644
> > > > > MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib
> > > > > .c
> > > > > > > create mode 100644
> > > > > MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib
> > > > > .inf
> > > > > > >
> > > > > > > diff --git a/MdeModulePkg/Include/Library/SerialUartClockLib.h
> > > > > b/MdeModulePkg/Include/Library/SerialUartClockLib.h
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..b6b16f71d4cf
> > > > > > > --- /dev/null
> > > > > > > +++ b/MdeModulePkg/Include/Library/SerialUartClockLib.h
> > > > > > > @@ -0,0 +1,21 @@
> > > > > > > +/** @file
> > > > > > > +*
> > > > > > > +* Copyright 2020 NXP
> > > > > > > +*
> > > > > > > +* SPDX-License-Identifier: BSD-2-Clause-Patent
> > > > > > > +*
> > > > > > > +**/
> > > > > > > +
> > > > > > > +#ifndef SERIAL_UART_CLOCK_LIB_H__ #define
> > > > > > > +SERIAL_UART_CLOCK_LIB_H__
> > > > > > > +
> > > > > > > +/**
> > > > > > > + Return clock in for Uart IP **/
> > > > > > > +UINT32
> > > > > > > +EFIAPI
> > > > > > > +BaseSerialPortGetClock (
> > > > > > > + VOID
> > > > > > > + );
> > > > > > > +
> > > > > > > +#endif
> > > > > > > diff --git
> > > > >
> > > >
> > a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550
> > > > .c
> > > > > > >
> > > > b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib165
> > > > > > > 50.c index 9cb50dd80d56..2e0c05d5789e 100644
> > > > > > > ---
> > > > >
> > > >
> > a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550
> > > > .c
> > > > > > > +++
> > > > >
> > > >
> > b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550
> > > > .c
> > > > > > > @@ -16,6 +16,7 @@
> > > > > > > #include <Library/IoLib.h>
> > > > > > > #include <Library/PciLib.h>
> > > > > > > #include <Library/PlatformHookLib.h>
> > > > > > > +#include <Library/SerialUartClockLib.h>
> > > > > > > #include <Library/BaseLib.h>
> > > > > > >
> > > > > > > //
> > > > > > > @@ -501,8 +502,8 @@ SerialPortInitialize (
> > > > > > > // Calculate divisor for baud generator
> > > > > > > // Ref_Clk_Rate / Baud_Rate / 16
> > > > > > > //
> > > > > > > - Divisor = PcdGet32 (PcdSerialClockRate) / (PcdGet32
> > > > > > > (PcdSerialBaudRate) *
> > > > > 16);
> > > > > > > - if ((PcdGet32 (PcdSerialClockRate) % (PcdGet32
> > > > > > > (PcdSerialBaudRate) *
> > > > > 16)) >= PcdGet32 (PcdSerialBaudRate) * 8) {
> > > > > > > + Divisor = BaseSerialPortGetClock () / (PcdGet32
> > > > > > > + (PcdSerialBaudRate) * 16); if ((BaseSerialPortGetClock () %
> > > > > > > + (PcdGet32 (PcdSerialBaudRate) * 16)) >=
> > > > > PcdGet32 (PcdSerialBaudRate) * 8) {
> > > > > > > Divisor++;
> > > > > > > }
> > > > > > >
> > > > > > > @@ -1080,8 +1081,8 @@ SerialPortSetAttributes (
> > > > > > > // Calculate divisor for baud generator
> > > > > > > // Ref_Clk_Rate / Baud_Rate / 16
> > > > > > > //
> > > > > > > - Divisor = PcdGet32 (PcdSerialClockRate) / (SerialBaudRate *
> > > > > > > 16);
> > > > > > > - if ((PcdGet32 (PcdSerialClockRate) % (SerialBaudRate * 16))
> > > > > > > >=
> > > > > SerialBaudRate * 8) {
> > > > > > > + Divisor = BaseSerialPortGetClock () / (SerialBaudRate *
> > > > > > > + 16); if ((BaseSerialPortGetClock () % (SerialBaudRate * 16))
> > > > > > > + >= SerialBaudRate *
> > > > > 8) {
> > > > > > > Divisor++;
> > > > > > > }
> > > > > > >
> > > > > > > diff --git
> > > > > a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib165
> > > > > 50.i
> > > > > nf
> > > > > > >
> > > > >
> > b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib165
> > > > > 50.i
> > > > > nf
> > > > > > > index 8b4ae3f1d4ee..b4c16504f211 100644
> > > > > > > ---
> > > > > a/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib165
> > > > > 50.i
> > > > > nf
> > > > > > > +++
> > > > >
> > b/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib165
> > > > > 50.i
> > > > > nf
> > > > > > > @@ -24,6 +24,7 @@ [LibraryClasses]
> > > > > > > IoLib
> > > > > > > PlatformHookLib
> > > > > > > PciLib
> > > > > > > + SerialUartClockLib
> > > > > > >
> > > > > > > [Sources]
> > > > > > > BaseSerialPortLib16550.c
> > > > > > > @@ -37,7 +38,6 @@ [Pcd]
> > > > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialBaudRate ##
> > > > > CONSUMES
> > > > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialLineControl
> > ##
> > > > > CONSUMES
> > > > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialFifoControl ##
> > > > > CONSUMES
> > > > > > > - gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate ##
> > > > > CONSUMES
> > > > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialPciDeviceInfo
> > ##
> > > > > CONSUMES
> > > > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialExtendedTxFifoSize
> > > > ##
> > > > > CONSUMES
> > > > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterStride
> > ##
> > > > > CONSUMES
> > > > > > > diff --git
> > > > > a/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockL
> > > > > ib.c
> > > > > > > b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartCl
> > > > > > > ockL
> > > > > > > ib.c
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..7a0d0427cc4e
> > > > > > > --- /dev/null
> > > > > > > +++
> > > > > b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockL
> > > > > ib.c
> > > > > > > @@ -0,0 +1,24 @@
> > > > > > > +/** @file
> > > > > > > +*
> > > > > > > +* Copyright 2020 NXP
> > > > > > > +*
> > > > > > > +* SPDX-License-Identifier: BSD-2-Clause-Patent
> > > > > > > +*
> > > > > > > +**/
> > > > > > > +
> > > > > > > +#include <Base.h>
> > > > > > > +#include <Library/PcdLib.h>
> > > > > > > +
> > > > > > > +/**
> > > > > > > + Return clock in for Uart IP
> > > > > > > +
> > > > > > > + @return Pcd PcdSerialClockRate **/
> > > > > > > +UINT32
> > > > > > > +EFIAPI
> > > > > > > +BaseSerialPortGetClock (
> > > > > > > + VOID
> > > > > > > + )
> > > > > > > +{
> > > > > > > + return PcdGet32 (PcdSerialClockRate); }
> > > > > > > diff --git
> > > > > a/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockL
> > > > > ib.i
> > > > > nf
> > > > > > >
> > > > > b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockL
> > > > > ib.i
> > > > > nf
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..91ba69436ed6
> > > > > > > --- /dev/null
> > > > > > > +++
> > > > > b/MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockL
> > > > > ib.i
> > > > > nf
> > > > > > > @@ -0,0 +1,27 @@
> > > > > > > +#/* @file
> > > > > > > +# Copyright 2020 NXP
> > > > > > > +#
> > > > > > > +# SPDX-License-Identifier: BSD-2-Clause-Patent # #*/
> > > > > > > +
> > > > > > > +[Defines]
> > > > > > > + INF_VERSION = 0x0001001A
> > > > > > > + BASE_NAME = BaseSerialUartClockLib
> > > > > > > + FILE_GUID = fa65495e-d3c8-4ea3-9737-994e9ccbaf11
> > > > > > > + MODULE_TYPE = BASE
> > > > > > > + VERSION_STRING = 1.0
> > > > > > > + LIBRARY_CLASS = SerialUartClockLib
> > > > > > > +
> > > > > > > +[Packages]
> > > > > > > + MdePkg/MdePkg.dec
> > > > > > > + MdeModulePkg/MdeModulePkg.dec
> > > > > > > +
> > > > > > > +[Sources.common]
> > > > > > > + BaseSerialUartClockLib.c
> > > > > > > +
> > > > > > > +[LibraryClasses]
> > > > > > > + PcdLib
> > > > > > > +
> > > > > > > +[Pcd]
> > > > > > > + gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate
> > ##
> > > > > CONSUMES
> > > > > > > diff --git a/MdeModulePkg/MdeModulePkg.dsc
> > > > > b/MdeModulePkg/MdeModulePkg.dsc
> > > > > > > index f7dbb27ce25d..d581ca797b3b 100644
> > > > > > > --- a/MdeModulePkg/MdeModulePkg.dsc
> > > > > > > +++ b/MdeModulePkg/MdeModulePkg.dsc
> > > > > > > @@ -65,6 +65,7 @@ [LibraryClasses]
> > > > > > > UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
> > > > > > >
> > > > >
> > > >
> > SecurityManagementLib|MdeModulePkg/Library/DxeSecurityManagementL
> > > > ib/D
> > > > > xeSecurityManagementLib.inf
> > > > > > >
> > > > >
> > > >
> > TimerLib|MdePkg/Library/BaseTimerLibNullTemplate/BaseTimerLibNullTem
> > > > pl
> > > > > TimerLib|at
> > > > > e.inf
> > > > > > > +
> > > > >
> > > > SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseS
> > > > SerialUartClockLib|er
> > > > > SerialUartClockLib|ialU
> > > > > artClockLib.inf
> > > > > > >
> > > > > SerialPortLib|MdePkg/Library/BaseSerialPortLibNull/BaseSerialPortL
> > > > > SerialPortLib|ibNu
> > > > > SerialPortLib|ll.inf
> > > > > > >
> > > > >
> > > >
> > CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.
> > > > CapsuleLib|i
> > > > n
> > > > > CapsuleLib|f
> > > > > > > PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
> > > > > > > @@ -292,6 +293,7 @@ [Components]
> > > > > > >
> > > > >
> > > >
> > MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.
> > > > inf
> > > > > > >
> > > > >
> > > >
> > MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/RuntimeDxeRep
> > > > ortSt
> > > > > atusCodeLib.inf
> > > > > > >
> > > > >
> > > >
> > MdeModulePkg/Library/RuntimeResetSystemLib/RuntimeResetSystemLib.i
> > > > nf
> > > > > > > +
> > > > > MdeModulePkg/Library/BaseSerialUartClockLib/BaseSerialUartClockLib
> > > > > .inf
> > > > > > >
> > > > >
> > MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550
> > > > > .inf
> > > > > > >
> > > > >
> > > >
> > MdeModulePkg/Library/BasePlatformHookLibNull/BasePlatformHookLibNull
> > > > .i
> > > > > nf
> > > > > > >
> > > > >
> > > >
> > MdeModulePkg/Library/DxeDebugPrintErrorLevelLib/DxeDebugPrintErrorLe
> > > > ve
> > > > > lLi
> > > > > b.inf
> > > > > > > diff --git a/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc
> > > > > b/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc
> > > > > > > index a1a1b81d03cb..c0ad88f26341 100644
> > > > > > > --- a/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc
> > > > > > > +++ b/SourceLevelDebugPkg/SourceLevelDebugPkg.dsc
> > > > > > > @@ -33,6 +33,7 @@ [LibraryClasses.common]
> > > > > > >
> > > > >
> > > >
> > SynchronizationLib|MdePkg/Library/BaseSynchronizationLib/BaseSynchro
> > > > SynchronizationLib|ni
> > > > > SynchronizationLib|zati
> > > > > onLib.inf
> > > > > > >
> > > > > > > LocalApicLib|UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.inf
> > > > > > >
> > > > >
> > > >
> > PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/Bas
> > > > PeCoffGetEntryPointLib|e
> > > > P
> > > > > PeCoffGetEntryPointLib|eC
> > > > > offGetEntryPointLib.inf
> > > > > > > +
> > > > >
> > > > SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseS
> > > > SerialUartClockLib|er
> > > > > SerialUartClockLib|ialU
> > > > > artClockLib.inf
> > > > > > >
> > > > > SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSeri
> > > > > SerialPortLib|alPo
> > > > > SerialPortLib|rtLib
> > > > > 16550.inf
> > > > > > >
> > > > >
> > > > PeCoffExtraActionLib|SourceLevelDebugPkg/Library/PeCoffExtraActionLi
> > > > PeCoffExtraActionLib|bD
> > > > > PeCoffExtraActionLib|ebu
> > > > > g/PeCoffExtraActionLibDebug.inf
> > > > > > >
> > > > >
> > > >
> > TimerLib|UefiCpuPkg/Library/SecPeiDxeTimerLibUefiCpu/SecPeiDxeTimerL
> > > > TimerLib|i
> > > > b
> > > > > TimerLib|Ue
> > > > > fiCpu.inf
> > > > > > > diff --git a/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
> > > > > b/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
> > > > > > > index d52945442e0e..a556a32b272c 100644
> > > > > > > --- a/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
> > > > > > > +++ b/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
> > > > > > > @@ -174,6 +174,7 @@ [LibraryClasses]
> > > > > > > #
> > > > > > >
> > > > > > > TimerLib|UefiPayloadPkg/Library/AcpiTimerLib/AcpiTimerLib.inf
> > > > > > >
> > > > >
> > > >
> > ResetSystemLib|UefiPayloadPkg/Library/ResetSystemLib/ResetSystemLib.
> > > > ResetSystemLib|in
> > > > > ResetSystemLib|f
> > > > > > > +
> > > > >
> > > > SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseS
> > > > SerialUartClockLib|er
> > > > > SerialUartClockLib|ialU
> > > > > artClockLib.inf
> > > > > > >
> > > > > SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSeri
> > > > > SerialPortLib|alPo
> > > > > SerialPortLib|rtLib
> > > > > 16550.inf
> > > > > > >
> > > > >
> > > >
> > PlatformHookLib|UefiPayloadPkg/Library/PlatformHookLib/PlatformHookL
> > > > PlatformHookLib|ib
> > > > > PlatformHookLib|.in
> > > > > f
> > > > > > >
> > > > >
> > > >
> > PlatformBootManagerLib|UefiPayloadPkg/Library/PlatformBootManagerLib
> > > > PlatformBootManagerLib|/
> > > > P
> > > > > PlatformBootManagerLib|l
> > > > > atformBootManagerLib.inf
> > > > > > > diff --git a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > > > > b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > > > > > > index 0736cd995476..7e86375fe57d 100644
> > > > > > > --- a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > > > > > > +++ b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > > > > > > @@ -175,6 +175,7 @@ [LibraryClasses]
> > > > > > > #
> > > > > > >
> > > > > > > TimerLib|UefiPayloadPkg/Library/AcpiTimerLib/AcpiTimerLib.inf
> > > > > > >
> > > > >
> > > >
> > ResetSystemLib|UefiPayloadPkg/Library/ResetSystemLib/ResetSystemLib.
> > > > ResetSystemLib|in
> > > > > ResetSystemLib|f
> > > > > > > +
> > > > >
> > > > SerialUartClockLib|MdeModulePkg/Library/BaseSerialUartClockLib/BaseS
> > > > SerialUartClockLib|er
> > > > > SerialUartClockLib|ialU
> > > > > artClockLib.inf
> > > > > > >
> > > > > SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSeri
> > > > > SerialPortLib|alPo
> > > > > SerialPortLib|rtLib
> > > > > 16550.inf
> > > > > > >
> > > > >
> > > >
> > PlatformHookLib|UefiPayloadPkg/Library/PlatformHookLib/PlatformHookL
> > > > PlatformHookLib|ib
> > > > > PlatformHookLib|.in
> > > > > f
> > > > > > >
> > > > >
> > > >
> > PlatformBootManagerLib|UefiPayloadPkg/Library/PlatformBootManagerLib
> > > > PlatformBootManagerLib|/
> > > > P
> > > > > PlatformBootManagerLib|l
> > > > > atformBootManagerLib.inf
> > > > > > > --
> > > > > > > 2.17.1
> > > > > >
> > > >
> > > >
> >
> >
> >
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq Support
2020-03-30 7:35 ` Leif Lindholm
@ 2020-03-30 7:44 ` Ard Biesheuvel
2020-03-31 1:53 ` Ni, Ray
0 siblings, 1 reply; 18+ messages in thread
From: Ard Biesheuvel @ 2020-03-30 7:44 UTC (permalink / raw)
To: Leif Lindholm
Cc: Jiang, Guomin, devel@edk2.groups.io, pankaj.bansal@nxp.com,
Ni, Ray, Wang, Jian J, Wu, Hao A, Ma, Maurice, Dong, Guo,
You, Benjamin, Meenakshi Aggarwal, Varun Sethi,
Samer El-Haj-Mahmoud
On Mon, 30 Mar 2020 at 09:35, Leif Lindholm <leif@nuviainc.com> wrote:
>
> Hi Jiang,
>
> It is not a question of effort of copying a driver, it is a question
> that copying drivers is something that should be avoided wherever
> practically possible. I did not think this topic was still under
> debate.
>
> If the existing 16550 SerialPortLib is overspecialised to the point
> where it only works on a subset of 16550 implementations, then it
> should change. There are going to be more non-PC systems turning up
> with 16550 UARTs - should they each copy/modify their drivers?
>
> If there are better ways of solving that problem, please suggest.
> But more duplicated drivers is not the answer.
>
Could we use a GUIDed HOB? If it exists, we use its contents, and if
it doesn't, we use the default set by the FixedPCD.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq Support
2020-03-30 7:44 ` Ard Biesheuvel
@ 2020-03-31 1:53 ` Ni, Ray
2020-03-31 9:22 ` [edk2-devel] API breakages and their implications. Was: " Leif Lindholm
2020-04-11 11:54 ` [edk2-devel] " Pankaj Bansal
0 siblings, 2 replies; 18+ messages in thread
From: Ni, Ray @ 2020-03-31 1:53 UTC (permalink / raw)
To: Ard Biesheuvel, Leif Lindholm
Cc: Jiang, Guomin, devel@edk2.groups.io, pankaj.bansal@nxp.com,
Wang, Jian J, Wu, Hao A, Ma, Maurice, Dong, Guo, You, Benjamin,
Meenakshi Aggarwal, Varun Sethi, Samer El-Haj-Mahmoud
Leif,
Please understand that the concern of this change is all the platforms that uses
this serial port lib must be changed otherwise build breaks.
Ard,
Using Guided HOB sounds a good idea to me: )
The benefits of using HOB is:
Length field in the HOB header can be used for extension if more parameters are needed.
DXE can have the HOB access as well.
EFI_SEC_HOB_DATA_PPI can be used to return the new Guided HOB from SEC phase if needed.
Thanks,
Ray
> -----Original Message-----
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Sent: Monday, March 30, 2020 3:45 PM
> To: Leif Lindholm <leif@nuviainc.com>
> Cc: Jiang, Guomin <guomin.jiang@intel.com>; devel@edk2.groups.io; pankaj.bansal@nxp.com; Ni, Ray <ray.ni@intel.com>;
> Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ma, Maurice <maurice.ma@intel.com>; Dong,
> Guo <guo.dong@intel.com>; You, Benjamin <benjamin.you@intel.com>; Meenakshi Aggarwal
> <meenakshi.aggarwal@nxp.com>; Varun Sethi <V.Sethi@nxp.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-
> Mahmoud@arm.com>
> Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq Support
>
> On Mon, 30 Mar 2020 at 09:35, Leif Lindholm <leif@nuviainc.com> wrote:
> >
> > Hi Jiang,
> >
> > It is not a question of effort of copying a driver, it is a question
> > that copying drivers is something that should be avoided wherever
> > practically possible. I did not think this topic was still under
> > debate.
> >
> > If the existing 16550 SerialPortLib is overspecialised to the point
> > where it only works on a subset of 16550 implementations, then it
> > should change. There are going to be more non-PC systems turning up
> > with 16550 UARTs - should they each copy/modify their drivers?
> >
> > If there are better ways of solving that problem, please suggest.
> > But more duplicated drivers is not the answer.
> >
>
> Could we use a GUIDed HOB? If it exists, we use its contents, and if
> it doesn't, we use the default set by the FixedPCD.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] API breakages and their implications. Was: [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq Support
2020-03-31 1:53 ` Ni, Ray
@ 2020-03-31 9:22 ` Leif Lindholm
2020-03-31 12:11 ` Ni, Ray
2020-03-31 13:23 ` Laszlo Ersek
2020-04-11 11:54 ` [edk2-devel] " Pankaj Bansal
1 sibling, 2 replies; 18+ messages in thread
From: Leif Lindholm @ 2020-03-31 9:22 UTC (permalink / raw)
To: Ni, Ray
Cc: Ard Biesheuvel, Jiang, Guomin, devel@edk2.groups.io, Andrew Fish,
Laszlo Ersek, Michael D Kinney
On Tue, Mar 31, 2020 at 01:53:21 +0000, Ni, Ray wrote:
> Leif,
> Please understand that the concern of this change is all the platforms that uses
> this serial port lib must be changed otherwise build breaks.
Yes. This is the nature of collaborative development.
This is something we on the ARM side have lived with since we got
involved with EDK2, being the less-deployed user of the codebase, and
that is fine.
TianoCore specifically produced the UDK to make life easier for those
who are unable to track upstream, and we have followed that up with
stable tags. I would highly recommend that any team that feels that
this change would be too much effort to move to edk2-stable202002. Of
course, they would then need to manually cherry-pick any improvements
and security fixes from master. That is their choice.
Nevertheless, breaking existing platforms is a side effect, not a
goal. So if an alternative solution can be found (which is not a hack
that is going to affect the codebase negatively over time and simply
need to be fixed properly at a later date), then clearly that is
preferable.
"This breaks many platforms" is a good argument for seeing if a
solution can be found that does not break (as) many platforms. It is
not an argument for duplicating drivers when the change needed for
those platforms is trivial.
These days, Linux tends to deal with API breakages (and other things)
using a semantic patch tool called Coccinelle[1]. It would not be
unreasonable, and indeed it would be helpful to us on the non-x86 side
if something similar was adopted (and semantic patches mandated) for
API breakages in EDK2.
[1] http://coccinelle.lip6.fr/sp.php
Regards,
Leif
> Ard,
> Using Guided HOB sounds a good idea to me: )
> The benefits of using HOB is:
> Length field in the HOB header can be used for extension if more parameters are needed.
> DXE can have the HOB access as well.
>
> EFI_SEC_HOB_DATA_PPI can be used to return the new Guided HOB from SEC phase if needed.
>
> Thanks,
> Ray
>
>
> > -----Original Message-----
> > From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Sent: Monday, March 30, 2020 3:45 PM
> > To: Leif Lindholm <leif@nuviainc.com>
> > Cc: Jiang, Guomin <guomin.jiang@intel.com>; devel@edk2.groups.io; pankaj.bansal@nxp.com; Ni, Ray <ray.ni@intel.com>;
> > Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ma, Maurice <maurice.ma@intel.com>; Dong,
> > Guo <guo.dong@intel.com>; You, Benjamin <benjamin.you@intel.com>; Meenakshi Aggarwal
> > <meenakshi.aggarwal@nxp.com>; Varun Sethi <V.Sethi@nxp.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-
> > Mahmoud@arm.com>
> > Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq Support
> >
> > On Mon, 30 Mar 2020 at 09:35, Leif Lindholm <leif@nuviainc.com> wrote:
> > >
> > > Hi Jiang,
> > >
> > > It is not a question of effort of copying a driver, it is a question
> > > that copying drivers is something that should be avoided wherever
> > > practically possible. I did not think this topic was still under
> > > debate.
> > >
> > > If the existing 16550 SerialPortLib is overspecialised to the point
> > > where it only works on a subset of 16550 implementations, then it
> > > should change. There are going to be more non-PC systems turning up
> > > with 16550 UARTs - should they each copy/modify their drivers?
> > >
> > > If there are better ways of solving that problem, please suggest.
> > > But more duplicated drivers is not the answer.
> > >
> >
> > Could we use a GUIDed HOB? If it exists, we use its contents, and if
> > it doesn't, we use the default set by the FixedPCD.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] API breakages and their implications. Was: [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq Support
2020-03-31 9:22 ` [edk2-devel] API breakages and their implications. Was: " Leif Lindholm
@ 2020-03-31 12:11 ` Ni, Ray
2020-03-31 12:59 ` Leif Lindholm
2020-03-31 13:23 ` Laszlo Ersek
1 sibling, 1 reply; 18+ messages in thread
From: Ni, Ray @ 2020-03-31 12:11 UTC (permalink / raw)
To: Leif Lindholm
Cc: Ard Biesheuvel, Jiang, Guomin, devel@edk2.groups.io, Andrew Fish,
Laszlo Ersek, Kinney, Michael D
Leif,
Thanks for introducing such an interesting tool.
I see this too is very useful for code refactoring.
It's a game changing tool😊
To help me understand, do you suggest MAYBE when incompatible changes like this
happen, the change owners propose the semantic patches for all platforms?
Thanks,
Ray
> -----Original Message-----
> From: Leif Lindholm <leif@nuviainc.com>
> Sent: Tuesday, March 31, 2020 5:22 PM
> To: Ni, Ray <ray.ni@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Jiang, Guomin <guomin.jiang@intel.com>; devel@edk2.groups.io; Andrew
> Fish <afish@apple.com>; Laszlo Ersek <lersek@redhat.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: Re: [edk2-devel] API breakages and their implications. Was: [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq
> Support
>
> On Tue, Mar 31, 2020 at 01:53:21 +0000, Ni, Ray wrote:
> > Leif,
> > Please understand that the concern of this change is all the platforms that uses
> > this serial port lib must be changed otherwise build breaks.
>
> Yes. This is the nature of collaborative development.
> This is something we on the ARM side have lived with since we got
> involved with EDK2, being the less-deployed user of the codebase, and
> that is fine.
>
> TianoCore specifically produced the UDK to make life easier for those
> who are unable to track upstream, and we have followed that up with
> stable tags. I would highly recommend that any team that feels that
> this change would be too much effort to move to edk2-stable202002. Of
> course, they would then need to manually cherry-pick any improvements
> and security fixes from master. That is their choice.
>
> Nevertheless, breaking existing platforms is a side effect, not a
> goal. So if an alternative solution can be found (which is not a hack
> that is going to affect the codebase negatively over time and simply
> need to be fixed properly at a later date), then clearly that is
> preferable.
>
> "This breaks many platforms" is a good argument for seeing if a
> solution can be found that does not break (as) many platforms. It is
> not an argument for duplicating drivers when the change needed for
> those platforms is trivial.
>
> These days, Linux tends to deal with API breakages (and other things)
> using a semantic patch tool called Coccinelle[1]. It would not be
> unreasonable, and indeed it would be helpful to us on the non-x86 side
> if something similar was adopted (and semantic patches mandated) for
> API breakages in EDK2.
>
> [1] http://coccinelle.lip6.fr/sp.php
>
> Regards,
>
> Leif
>
> > Ard,
> > Using Guided HOB sounds a good idea to me: )
> > The benefits of using HOB is:
> > Length field in the HOB header can be used for extension if more parameters are needed.
> > DXE can have the HOB access as well.
> >
> > EFI_SEC_HOB_DATA_PPI can be used to return the new Guided HOB from SEC phase if needed.
> >
> > Thanks,
> > Ray
> >
> >
> > > -----Original Message-----
> > > From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > Sent: Monday, March 30, 2020 3:45 PM
> > > To: Leif Lindholm <leif@nuviainc.com>
> > > Cc: Jiang, Guomin <guomin.jiang@intel.com>; devel@edk2.groups.io; pankaj.bansal@nxp.com; Ni, Ray
> <ray.ni@intel.com>;
> > > Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ma, Maurice <maurice.ma@intel.com>; Dong,
> > > Guo <guo.dong@intel.com>; You, Benjamin <benjamin.you@intel.com>; Meenakshi Aggarwal
> > > <meenakshi.aggarwal@nxp.com>; Varun Sethi <V.Sethi@nxp.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-
> > > Mahmoud@arm.com>
> > > Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq Support
> > >
> > > On Mon, 30 Mar 2020 at 09:35, Leif Lindholm <leif@nuviainc.com> wrote:
> > > >
> > > > Hi Jiang,
> > > >
> > > > It is not a question of effort of copying a driver, it is a question
> > > > that copying drivers is something that should be avoided wherever
> > > > practically possible. I did not think this topic was still under
> > > > debate.
> > > >
> > > > If the existing 16550 SerialPortLib is overspecialised to the point
> > > > where it only works on a subset of 16550 implementations, then it
> > > > should change. There are going to be more non-PC systems turning up
> > > > with 16550 UARTs - should they each copy/modify their drivers?
> > > >
> > > > If there are better ways of solving that problem, please suggest.
> > > > But more duplicated drivers is not the answer.
> > > >
> > >
> > > Could we use a GUIDed HOB? If it exists, we use its contents, and if
> > > it doesn't, we use the default set by the FixedPCD.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] API breakages and their implications. Was: [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq Support
2020-03-31 12:11 ` Ni, Ray
@ 2020-03-31 12:59 ` Leif Lindholm
0 siblings, 0 replies; 18+ messages in thread
From: Leif Lindholm @ 2020-03-31 12:59 UTC (permalink / raw)
To: Ni, Ray
Cc: Ard Biesheuvel, Jiang, Guomin, devel@edk2.groups.io, Andrew Fish,
Laszlo Ersek, Kinney, Michael D
Hi Ray,
I think it's good to start doing it voluntarily, or for changes
expected to affect many platforms. Over time, as people become more
familiar with the tool, it would make sense to make it first
recommended and then mandatory.
For Linux, the coccinelle diff is frequently included in the commit
message, or (especially when used for changing deprecated interfaces
or poor coding practice) they are copmmitted to the repository (under
scripts/coccinelle).
Regards,
Leif
On Tue, Mar 31, 2020 at 12:11:03 +0000, Ni, Ray wrote:
> Leif,
> Thanks for introducing such an interesting tool.
> I see this too is very useful for code refactoring.
> It's a game changing tool😊
>
> To help me understand, do you suggest MAYBE when incompatible changes like this
> happen, the change owners propose the semantic patches for all platforms?
>
>
> Thanks,
> Ray
>
>
> > -----Original Message-----
> > From: Leif Lindholm <leif@nuviainc.com>
> > Sent: Tuesday, March 31, 2020 5:22 PM
> > To: Ni, Ray <ray.ni@intel.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Jiang, Guomin <guomin.jiang@intel.com>; devel@edk2.groups.io; Andrew
> > Fish <afish@apple.com>; Laszlo Ersek <lersek@redhat.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> > Subject: Re: [edk2-devel] API breakages and their implications. Was: [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq
> > Support
> >
> > On Tue, Mar 31, 2020 at 01:53:21 +0000, Ni, Ray wrote:
> > > Leif,
> > > Please understand that the concern of this change is all the platforms that uses
> > > this serial port lib must be changed otherwise build breaks.
> >
> > Yes. This is the nature of collaborative development.
> > This is something we on the ARM side have lived with since we got
> > involved with EDK2, being the less-deployed user of the codebase, and
> > that is fine.
> >
> > TianoCore specifically produced the UDK to make life easier for those
> > who are unable to track upstream, and we have followed that up with
> > stable tags. I would highly recommend that any team that feels that
> > this change would be too much effort to move to edk2-stable202002. Of
> > course, they would then need to manually cherry-pick any improvements
> > and security fixes from master. That is their choice.
> >
> > Nevertheless, breaking existing platforms is a side effect, not a
> > goal. So if an alternative solution can be found (which is not a hack
> > that is going to affect the codebase negatively over time and simply
> > need to be fixed properly at a later date), then clearly that is
> > preferable.
> >
> > "This breaks many platforms" is a good argument for seeing if a
> > solution can be found that does not break (as) many platforms. It is
> > not an argument for duplicating drivers when the change needed for
> > those platforms is trivial.
> >
> > These days, Linux tends to deal with API breakages (and other things)
> > using a semantic patch tool called Coccinelle[1]. It would not be
> > unreasonable, and indeed it would be helpful to us on the non-x86 side
> > if something similar was adopted (and semantic patches mandated) for
> > API breakages in EDK2.
> >
> > [1] http://coccinelle.lip6.fr/sp.php
> >
> > Regards,
> >
> > Leif
> >
> > > Ard,
> > > Using Guided HOB sounds a good idea to me: )
> > > The benefits of using HOB is:
> > > Length field in the HOB header can be used for extension if more parameters are needed.
> > > DXE can have the HOB access as well.
> > >
> > > EFI_SEC_HOB_DATA_PPI can be used to return the new Guided HOB from SEC phase if needed.
> > >
> > > Thanks,
> > > Ray
> > >
> > >
> > > > -----Original Message-----
> > > > From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > > Sent: Monday, March 30, 2020 3:45 PM
> > > > To: Leif Lindholm <leif@nuviainc.com>
> > > > Cc: Jiang, Guomin <guomin.jiang@intel.com>; devel@edk2.groups.io; pankaj.bansal@nxp.com; Ni, Ray
> > <ray.ni@intel.com>;
> > > > Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ma, Maurice <maurice.ma@intel.com>; Dong,
> > > > Guo <guo.dong@intel.com>; You, Benjamin <benjamin.you@intel.com>; Meenakshi Aggarwal
> > > > <meenakshi.aggarwal@nxp.com>; Varun Sethi <V.Sethi@nxp.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-
> > > > Mahmoud@arm.com>
> > > > Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq Support
> > > >
> > > > On Mon, 30 Mar 2020 at 09:35, Leif Lindholm <leif@nuviainc.com> wrote:
> > > > >
> > > > > Hi Jiang,
> > > > >
> > > > > It is not a question of effort of copying a driver, it is a question
> > > > > that copying drivers is something that should be avoided wherever
> > > > > practically possible. I did not think this topic was still under
> > > > > debate.
> > > > >
> > > > > If the existing 16550 SerialPortLib is overspecialised to the point
> > > > > where it only works on a subset of 16550 implementations, then it
> > > > > should change. There are going to be more non-PC systems turning up
> > > > > with 16550 UARTs - should they each copy/modify their drivers?
> > > > >
> > > > > If there are better ways of solving that problem, please suggest.
> > > > > But more duplicated drivers is not the answer.
> > > > >
> > > >
> > > > Could we use a GUIDed HOB? If it exists, we use its contents, and if
> > > > it doesn't, we use the default set by the FixedPCD.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] API breakages and their implications. Was: [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq Support
2020-03-31 9:22 ` [edk2-devel] API breakages and their implications. Was: " Leif Lindholm
2020-03-31 12:11 ` Ni, Ray
@ 2020-03-31 13:23 ` Laszlo Ersek
2020-04-01 12:55 ` Leif Lindholm
1 sibling, 1 reply; 18+ messages in thread
From: Laszlo Ersek @ 2020-03-31 13:23 UTC (permalink / raw)
To: Leif Lindholm, Ni, Ray
Cc: Ard Biesheuvel, Jiang, Guomin, devel@edk2.groups.io, Andrew Fish,
Michael D Kinney
On 03/31/20 11:22, Leif Lindholm wrote:
> On Tue, Mar 31, 2020 at 01:53:21 +0000, Ni, Ray wrote:
>> Leif,
>> Please understand that the concern of this change is all the platforms that uses
>> this serial port lib must be changed otherwise build breaks.
>
> Yes. This is the nature of collaborative development.
> This is something we on the ARM side have lived with since we got
> involved with EDK2, being the less-deployed user of the codebase, and
> that is fine.
>
> TianoCore specifically produced the UDK to make life easier for those
> who are unable to track upstream, and we have followed that up with
> stable tags. I would highly recommend that any team that feels that
> this change would be too much effort to move to edk2-stable202002. Of
> course, they would then need to manually cherry-pick any improvements
> and security fixes from master. That is their choice.
>
> Nevertheless, breaking existing platforms is a side effect, not a
> goal. So if an alternative solution can be found (which is not a hack
> that is going to affect the codebase negatively over time and simply
> need to be fixed properly at a later date), then clearly that is
> preferable.
>
> "This breaks many platforms" is a good argument for seeing if a
> solution can be found that does not break (as) many platforms. It is
> not an argument for duplicating drivers when the change needed for
> those platforms is trivial.
>
> These days, Linux tends to deal with API breakages (and other things)
> using a semantic patch tool called Coccinelle[1]. It would not be
> unreasonable, and indeed it would be helpful to us on the non-x86 side
> if something similar was adopted (and semantic patches mandated) for
> API breakages in EDK2.
>
> [1] http://coccinelle.lip6.fr/sp.php
Two comments:
(1) One of the reasons why I would like to keep all platforms in a
single tree is to deal with API changes like this. That way, someone
proposing an API change would at least have the chance to fix up all the
consumer sites. Of course it would require diligent review from the
other pkg maintainers, but it could be implemented without any temprary
breakage in the git history even.
(2) Specifically about this problem. The vendor GUID approach is not a
bad one. How about the following alternative:
(2.1) Patch#1:
in the "MdeModulePkg/Library/BaseSerialPortLib16550" directory,
introduce a header file called "GetClock.h". This header file should
declare:
UINT32
GetClock (
VOID
);
Note: EFIAPI not needed.
The header file should be added to the existent
"BaseSerialPortLib16550.inf" file, in the [Sources] section.
Furthermore, in the same patch, introduce a new source file called
"GetClockPcd.c". (Add this file to the INF file as well.) The source
file should do:
#include <Library/PcdLib.h>
#include "GetClock.h"
UINT32
GetClock (
VOID
)
{
return PcdGet32 (PcdSerialClockRate);
}
Finally, in the same patch#1, modify "BaseSerialPortLib16550.c" to
#include "GetClock.h", and to call GetClock() in place of the current
PcdGet32 (PcdSerialClockRate) calls.
This patch basically splits the PcdGet32 (PcdSerialClockRate) call to a
separate translation unit, hiding it behind the more abstract GetClock()
API.
(2.2) Patch#2:
In the *same* "MdeModulePkg/Library/BaseSerialPortLib16550" directory,
introduce three new files:
- BaseSerialPortLibDynamicFrequency16550.inf
- BaseSerialPortLibDynamicFrequency16550.uni
- GetClockDynamicFrequency.c
I think the contents of these files must be fairly obvious at this
point, but anyway:
(2.2.1) INF file:
- modified copy of the INF file from (2.1)
- Update file-top comment, copyright notice, BASE_NAME, MODULE_UNI_FILE,
FILE_GUID.
- [LibraryClasses]: add dependency on SerialUartClockLib
- [Sources]: replace "GetClockPcd.c" with "GetClockDynamicFrequency.c"
- [Pcd]: drop PcdSerialClockRate
(2.2.2) UNI file: copy and modify the existent UNI file as needed.
(2.2.3) GetClockDynamicFrequency.c:
#include <Library/SerialUartClockLib.h>
#include "GetClock.h"
UINT32
GetClock (
VOID
)
{
return BaseSerialPortGetClock ();
}
This way, platforms currently consuming
"MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf"
will see no change whatsoever.
Platforms needing the dynamic frequency can resolve SerialPortLib to
"MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLibDynamicFrequency16550.inf",
*and* also resolve SerialUartClockLib to an instance that matches their
needs.
All implementation except the GetClock() one will be shared between the
two lib instances "BaseSerialPortLib16550" and
"BaseSerialPortLibDynamicFrequency16550".
This is a frequent pattern in edk2 -- refer to the various module
directories that contain two or more INF files. For example:
$ git ls-files -- \
MdePkg/*inf \
MdeModulePkg/*inf \
NetworkPkg/*inf \
PcAtChipsetPkg/*inf \
SecurityPkg/*inf \
ShellPkg/*inf \
UefiCpuPkg/*inf \
| rev \
| cut -f 2- -d / \
| rev \
| sort \
| uniq -d
This will produce a list of directories where each directory contains at
least two INF files:
MdeModulePkg/Bus/I2c/I2cDxe
MdeModulePkg/Core/PiSmmCore
MdeModulePkg/Library/DxeCapsuleLibFmp
MdeModulePkg/Library/DxeCoreMemoryAllocationLib
MdeModulePkg/Library/DxeResetSystemLib/UnitTest
MdeModulePkg/Library/LzmaCustomDecompressLib
MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib
MdeModulePkg/Library/SmmLockBoxLib
MdeModulePkg/Logo
MdeModulePkg/Universal/CapsulePei
MdeModulePkg/Universal/EbcDxe
MdeModulePkg/Universal/FaultTolerantWriteDxe
MdeModulePkg/Universal/Variable/RuntimeDxe
MdePkg/Library/BaseIoLibIntrinsic
MdePkg/Library/BaseUefiDecompressLib
MdePkg/Library/PciSegmentLibSegmentInfo
MdePkg/Library/UefiDevicePathLib
MdePkg/Test/UnitTest/Library/BaseLib
MdePkg/Test/UnitTest/Library/BaseSafeIntLib
PcAtChipsetPkg/Library/AcpiTimerLib
SecurityPkg/HddPassword
SecurityPkg/Library/HashLibBaseCryptoRouter
SecurityPkg/Library/Tpm2DeviceLibDTpm
SecurityPkg/Library/Tpm2DeviceLibRouter
SecurityPkg/Tcg/Opal/OpalPassword
SecurityPkg/Tcg/Tcg2Config
ShellPkg/DynamicCommand/DpDynamicCommand
ShellPkg/DynamicCommand/TftpDynamicCommand
UefiCpuPkg/CpuFeatures
UefiCpuPkg/Library/CpuExceptionHandlerLib
UefiCpuPkg/Library/CpuTimerLib
UefiCpuPkg/Library/MpInitLib
UefiCpuPkg/Library/RegisterCpuFeaturesLib
UefiCpuPkg/Library/SmmCpuFeaturesLib
UefiCpuPkg/PiSmmCommunication
If you look at INF files inside any given such directory, you'll find
that the [Sources] sections between them have non-empty intersections,
but also differences. That's the exact same pattern we should use here, IMO.
(I intentionally used the core packages in this example.)
Thanks
Laszlo
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] API breakages and their implications. Was: [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq Support
2020-03-31 13:23 ` Laszlo Ersek
@ 2020-04-01 12:55 ` Leif Lindholm
0 siblings, 0 replies; 18+ messages in thread
From: Leif Lindholm @ 2020-04-01 12:55 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Ni, Ray, Ard Biesheuvel, Jiang, Guomin, devel@edk2.groups.io,
Andrew Fish, Michael D Kinney
On Tue, Mar 31, 2020 at 15:23:24 +0200, Laszlo Ersek wrote:
> On 03/31/20 11:22, Leif Lindholm wrote:
> > "This breaks many platforms" is a good argument for seeing if a
> > solution can be found that does not break (as) many platforms. It is
> > not an argument for duplicating drivers when the change needed for
> > those platforms is trivial.
> >
> > These days, Linux tends to deal with API breakages (and other things)
> > using a semantic patch tool called Coccinelle[1]. It would not be
> > unreasonable, and indeed it would be helpful to us on the non-x86 side
> > if something similar was adopted (and semantic patches mandated) for
> > API breakages in EDK2.
> >
> > [1] http://coccinelle.lip6.fr/sp.php
>
> Two comments:
>
> (1) One of the reasons why I would like to keep all platforms in a
> single tree is to deal with API changes like this.
Agreed.
> That way, someone
> proposing an API change would at least have the chance to fix up all the
> consumer sites. Of course it would require diligent review from the
> other pkg maintainers, but it could be implemented without any temprary
> breakage in the git history even.
And a daily CI job could spot breakages and send out alerts to
platform owners.
It would also provide more incentive for actually upstreaming platform
ports.
> (2) Specifically about this problem. The vendor GUID approach is not a
> bad one. How about the following alternative:
I have no strong comment on your alternative. It seems perfectly
feasible, and I agree there is precedent. Thanks for providing it.
I will let the MdeModulePkg maintainers specify their preference, or
provide other alternative solutions.
Best Regards,
Leif
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq Support
2020-03-31 1:53 ` Ni, Ray
2020-03-31 9:22 ` [edk2-devel] API breakages and their implications. Was: " Leif Lindholm
@ 2020-04-11 11:54 ` Pankaj Bansal
1 sibling, 0 replies; 18+ messages in thread
From: Pankaj Bansal @ 2020-04-11 11:54 UTC (permalink / raw)
To: 'Ni, Ray', 'Ard Biesheuvel',
'Leif Lindholm'
Cc: 'Jiang, Guomin', 'devel@edk2.groups.io',
'Wang, Jian J', 'Wu, Hao A',
'Ma, Maurice', 'Dong, Guo',
'You, Benjamin', Meenakshi Aggarwal, Varun Sethi,
'Samer El-Haj-Mahmoud', Pankaj Bansal (OSS)
>
> Leif,
> Please understand that the concern of this change is all the platforms that uses
> this serial port lib must be changed otherwise build breaks.
>
> Ard,
> Using Guided HOB sounds a good idea to me: )
> The benefits of using HOB is:
> Length field in the HOB header can be used for extension if more parameters
> are needed.
This sounds really appealing to me as well. Because in the past we had received request
from our customers about changing the UART baud rate in production system, without
having to recompile the firmware.
Up until now, the only method we have is UEFI shell, whose changes would take effect
after BDS phase. With GUID HOB, we can update the baud rate as early as PEI phase.
> DXE can have the HOB access as well.
>
> EFI_SEC_HOB_DATA_PPI can be used to return the new Guided HOB from SEC
> phase if needed.
So I made changes that pass EFI_SEC_HOB_DATA_PPI from SEC to PEI core.
I had to remove SerialPortInitialize call from SEC phase, as in SEC phase we don't have HOB support.
I am able to get the clock in PEI phase. However this fails in DXE phase.
This is happening because the dependency of SerialPortConstructor on HobLibConstructor.
This is the Autogen.h file of PcdDxe:
VOID
EFIAPI
ProcessLibraryConstructorList (
IN EFI_HANDLE ImageHandle,
IN EFI_SYSTEM_TABLE *SystemTable
)
{
EFI_STATUS Status;
Status = BaseDebugLibSerialPortConstructor ();
ASSERT_RETURN_ERROR (Status);
Status = UefiBootServicesTableLibConstructor (ImageHandle, SystemTable);
ASSERT_EFI_ERROR (Status);
Status = UefiRuntimeServicesTableLibConstructor (ImageHandle, SystemTable);
ASSERT_EFI_ERROR (Status);
Status = UefiLibConstructor (ImageHandle, SystemTable);
ASSERT_EFI_ERROR (Status);
Status = HobLibConstructor (ImageHandle, SystemTable);
ASSERT_EFI_ERROR (Status);
}
As you can see, the SerialPortConstructor is called first, which tries to retrieve the Guided HOB.
This in turn calls the EfiGetSystemConfigurationTable(&gEfiHobListGuid, &mHobList),
Which relies on gST pointer being set. Which would be set in UefiBootServicesTableLibConstructor.
How can we ensure that the SerialPortConstructor is called after HobLibConstructor ?
>
> Thanks,
> Ray
>
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2020-04-11 11:55 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-02-19 13:31 [PATCH 0/1] UART Dynamic clock freq Support Pankaj Bansal
2020-02-19 13:31 ` [PATCH 1/1] MdeModulePkg: " Pankaj Bansal
2020-03-19 13:40 ` [edk2-devel] " Samer El-Haj-Mahmoud
2020-03-19 15:09 ` Ni, Ray
2020-03-19 15:30 ` Leif Lindholm
2020-03-23 5:31 ` Pankaj Bansal
2020-03-26 14:13 ` [edk2-devel] " Guomin Jiang
2020-03-28 12:36 ` Pankaj Bansal
2020-03-30 1:20 ` Guomin Jiang
2020-03-30 7:35 ` Leif Lindholm
2020-03-30 7:44 ` Ard Biesheuvel
2020-03-31 1:53 ` Ni, Ray
2020-03-31 9:22 ` [edk2-devel] API breakages and their implications. Was: " Leif Lindholm
2020-03-31 12:11 ` Ni, Ray
2020-03-31 12:59 ` Leif Lindholm
2020-03-31 13:23 ` Laszlo Ersek
2020-04-01 12:55 ` Leif Lindholm
2020-04-11 11:54 ` [edk2-devel] " Pankaj Bansal
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox