* [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