Hi Laszlo,
Thanks for reviewing again this patch while you are sick and please take care.
My opinion is as follow:
The part of "CpuTimerLib" uses the name located at UefiCpuPkg/Library/CpuTimerLib, so this library is named BaseLoongArch64CpuTimerLib.On 11/23/23 12:43, Chao Li wrote:On 2023/11/23 00:12, Laszlo Ersek wrote:On 11/17/23 11:00, Chao Li wrote:Add the LoongArch64 CPU Timer library, using CPUCFG 0x4 and 0x5 for Stable Counter frequency. BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4584 Cc: Eric Dong <eric.dong@intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Rahul Kumar <rahul1.kumar@intel.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Signed-off-by: Chao Li <lichao@loongson.cn> --- .../BaseLoongArch64CpuTimerLib.inf | 30 +++ .../BaseLoongArch64CpuTimerLib.uni | 15 ++ .../BaseLoongArch64CpuTimerLib/CpuTimerLib.c | 226 ++++++++++++++++++ UefiCpuPkg/UefiCpuPkg.dsc | 3 + 4 files changed, 274 insertions(+) create mode 100644 UefiCpuPkg/Library/BaseLoongArch64CpuTimerLib/BaseLoongArch64CpuTimerLib.inf create mode 100644 UefiCpuPkg/Library/BaseLoongArch64CpuTimerLib/BaseLoongArch64CpuTimerLib.uni create mode 100644 UefiCpuPkg/Library/BaseLoongArch64CpuTimerLib/CpuTimerLib.c(1) sorry about the annoying comment, but the library should be called (preferably) BaseTimerLibLoongArch64Cpu. We're frequently not consistent with the following library instance naming scheme, but in theory anyway, library instances should be named as follows: <firmware module type><lib class name><library instance identifier> Thus, in this case, Base + TimerLib + LoongArch64Cpu. update UNI file name, INF file name, directory name, BASE_NAME and MODULE_UNI_FILE accordingly (if you agree)There has a SPEC for naming style: https://github.com/tianocore-docs/edk2-CCodingStandardsSpecification/blob/master/4_naming_conventions/42_directory_names.md Please check 4.2.3 EDKII Library directory, and most directory naming follows this SPEC.I didn't remember (or know about) this part of the coding standards spec; thanks for the pointer. It seems like my suggestion matches the second alternative (<Phase><LibraryClassName>[<Dependency>]). Your naming seems to follow the first alternative (<Phase>[<CpuArch>]<LibraryClassName>). OK. However, that's still not a perfect match. For <Phase>, you have "Base". For <CpuArch>, you have "LoongArch64". But for <LibraryClassName>, you have "CpuTimerLib", and that's wrong. We don't have a "CpuTimerLib" class; instead it's the "TimerLib" class.
The LIBRARY_CLASS define in your INF file also says "TimerLib", so "CpuTimerLib" is wrong / inconsistent in the lib instance name. "BaseLoongArch64TimerLib" should be fine. If you want to add "Cpu", I guess you could add it as [<Dependency>], as a suffix, under the first naming scheme alternative: "BaseLoongArch64TimerLibCpu".
I think the name "CpuTimerLib" highlights the "CPU", because this library relies on the CPU interal timer. So should I use the "CpuTimerLib" as <LibraryClassName>?
Ray, what do you suggest?
(5) Relatedly: the TimerLib class is declared in "MdePkg.dec". Thus, if you indeed don't need anything from "UefiCpuPkg.dec", I'd suggest moving this library instance under MdePkg.It is inappropriate to place this library in MdePkg, because the MdePkg doesn't have any CpuTimerLib instance, and this class library are HW implementation-related, so it more appropriate to place it in UefiCpuPkg.This ties in with my above comment. The reason for MdePkg not having a "CpuTimerLib" instance is that there is no such lib class in edk2. The library class is called "TimerLib". We have multiple instances in edk2: $ git grep -l 'LIBRARY_CLASS *= *TimerLib\>' -- '*inf' ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.inf EmulatorPkg/Library/DxeCoreTimerLib/DxeCoreTimerLib.inf EmulatorPkg/Library/DxeTimerLib/DxeTimerLib.inf EmulatorPkg/Library/PeiTimerLib/PeiTimerLib.inf MdePkg/Library/BaseTimerLibNullTemplate/BaseTimerLibNullTemplate.inf MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLibBhyve.inf OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.inf OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf PcAtChipsetPkg/Library/AcpiTimerLib/PeiAcpiTimerLib.inf PcAtChipsetPkg/Library/AcpiTimerLib/StandaloneMmAcpiTimerLib.inf UefiCpuPkg/Library/BaseRiscV64CpuTimerLib/BaseRiscV64CpuTimerLib.inf UefiCpuPkg/Library/CpuTimerLib/BaseCpuTimerLib.inf UefiCpuPkg/Library/SecPeiDxeTimerLibUefiCpu/SecPeiDxeTimerLibUefiCpu.inf UefiPayloadPkg/Library/AcpiTimerLib/AcpiTimerLib.inf This "prior art" suggests the new library instance could go in MdePkg or UefiCpuPkg alike. That's where my comment about "UefiCpuPkg.dec" dependencies becomes relevant. Compare: - MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf - UefiCpuPkg/Library/SecPeiDxeTimerLibUefiCpu/SecPeiDxeTimerLibUefiCpu.inf The difference between these library instances (as the INF files explain) is xAPIC vs. x2APIC support. The latter instance supports x2APIC. And, for x2APIC support, the latter instance depends on the LocalApicLib class. The LocalApicLib class is declared in "UefiCpuPkg.dec". And that is the dependency that forces the second library instance to exist under UefiCpuPkg. Thus, my original point stands -- if you need nothing from "UefiCpuPkg", then your library instance "BaseLoongArch64TimerLib" (or "BaseLoongArch64TimerLibCpu", if you like) should go under MdePkg, in my opinion anyway. (A different example: "UefiCpuPkg/Library/CpuTimerLib/BaseCpuTimerLib.inf" lives under UefiCpuPkg due to depending on "PcdCpuCoreCrystalClockFrequency", which also comes from "UefiCpuPkg.dec".)
I'm confused after you said it. In my opion, the libraries or driver instance that implements dependencies should be under the UefiCpuPkg, and the architecture common library or driver should be under the MdePkg.
Liming, Mike and Ray, what suggestions do you have?
Ok, I agree.+ VOID + ) +{ + UINT32 BaseFreq; + UINT32 ClockMultiplier; + UINT32 ClockDivide; + CPUCFG_REG4_INFO_DATA CCFreq;(10) Per edk2 coding style, when acronyms and abbreviations are grouped together, each individual acronym itself is supposed to be CamelCase. For example, when we use "PCI" in identifiers, it becomes "Pci". So I think this variable should be called "CcFreq", where "CC" stands for "CPU Config". If I'm wrong about "CC", then please ignore this point.The two letter 'C' means: first 'C' means Constant, and the second means Crystal. In LoongArch SPEC, this area is named CC_FREQ, which means: Constant frequency timer and the crystal frequency corresponding to the clock used by the timer. So I think CCFreq is more suitable."PCI" is also an acronym ("Peripheral Component Interconnect"), so "CC" for "Constant Crystal" is no different, from this perspective. The same way we write PciRead8(), the variable should be CcFreq.
Ok, I agree.+ CPUCFG_REG5_INFO_DATA CpucfgReg5Data; + UINT32 StableTimerFreq; + + // + // Get the the crystal frequency corresponding to the constant + // frequency timer and the clock used by the timer. + // + AsmCpucfg (CPUCFG_REG4_INFO, &CCFreq.Uint32); + + // + // Get the multiplication factor and frequency division factor + // corresponding to the constant frequency timer and the clock + // used by the timer. + // + AsmCpucfg (CPUCFG_REG5_INFO, &CpucfgReg5Data.Uint32); + + BaseFreq = CCFreq.Bits.CC_FREQ;(11) My comment here probably affects the header file where you introduced the CC_FREQ field name. It should be CcFreq. Same for MUL and DIV below.The same to above.I understand the LoongArch spec may call this "CC_FREQ", but that's just not how edk2 spells field names. "CC_FREQ" is not idiomatic, the field too should be called "CcFreq". Anyway, I don't insist, just wanted to make you aware.
Ok, I will add the CpuDeadLoop in the V4.+ ASSERT (0);(15) Should be ASSERT (FALSE). (16) I recommend that a CpuDeadLoop() call be added here too, because ASSERT()s are compiled out of RELEASE builds.For double (13), (14), (15), (16) and (17), I agree, will fix them in V4. But for 15, did the CpuDeadLoop() be needed? I think if the DEBUG mode doesn't work, the RELEASE mode can not be created.You are not asserting an algorithmic invariant, but an environmental circumstance (namely that the clock does not show unreasonable properties). Meaning that the predicate can be falsified due to environmental reasons, even if there is no programming or design error in the code. Therefore, in my opinion, such *environment* checks are best preserved in RELEASE builds as well. Either way, up to you.
I have noticed.(25) This proves that, generally speaking, the final "+=" operation (increment) is unsafe -- in every TimerLib instance in edk2 that uses this calculation --, and should be replaced with an addition from SafeIntLib.Do you recommend changing the final "+=" to the SafeIntLib way? Something like: NanoSeconds = SafeUint64Add (NanoSeconds, DivU64x64Remainder (MultU64x32 (Remainder, 1000000000u), Frequency, NULL), &NanoSeconds).Something like that, yes, please. While at it, using some helper variables could improve readability, plus AFAIR, SafeUint64Add() returns a RETURN_STATUS vaule.
Please take care.... sorry about the late followup. I've been out sick for two weeks, and have returned now for only a few days again, before the year ends. I think it unlikely that I can continue reviewing v3 in any timely manner, so feel free to post v4 whenever you're ready to do so (assuming no-one else wants to continue reviewing v3).
Thanks Laszlo