From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id BCBEFAC0207 for ; Tue, 12 Dec 2023 03:46:10 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=vKvPIfQqEli1eYBWfdbUUIqKpU1xMhHH5j3KQC9hgKE=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:User-Agent:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1702352769; v=1; b=J+IEabw+IlGDmcS44Bf//WQXK/zAM2txSheq34mqbKumSP02jhuQt59iboBIxld0nCwKarmN pGjK5rhoZlcXDza0kNJbvKGh1DEY0us4azu706MYcSx5Uvdi5vLhvw9WZXpThEIuWIHJuoaYDew pqn2K8RtSOMRQapEnTp5ISWk= X-Received: by 127.0.0.2 with SMTP id YGkzYY7687511xk2Nm88rWLc; Mon, 11 Dec 2023 19:46:09 -0800 X-Received: from mail.loongson.cn (mail.loongson.cn [114.242.206.163]) by mx.groups.io with SMTP id smtpd.web11.5416.1702352767409018029 for ; Mon, 11 Dec 2023 19:46:08 -0800 X-Received: from loongson.cn (unknown [10.40.24.149]) by gateway (Coremail) with SMTP id _____8DxS+l213dl7j4AAA--.1568S3; Tue, 12 Dec 2023 11:45:59 +0800 (CST) X-Received: from [10.40.24.149] (unknown [10.40.24.149]) by localhost.localdomain (Coremail) with SMTP id AQAAf8DxkuNx13dlxx0AAA--.1042S3; Tue, 12 Dec 2023 11:45:53 +0800 (CST) Message-ID: Date: Tue, 12 Dec 2023 11:45:53 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [edk2-devel] [PATCH v3 11/39] UefiCpuPkg: Add LoongArch64 CPU Timer library To: devel@edk2.groups.io, lersek@redhat.com, Liming Gao , Michael D Kinney , "Ni, Ray" Cc: Eric Dong , Rahul Kumar , Gerd Hoffmann References: <20231117095742.3605778-1-lichao@loongs> <20231117100007.3609080-1-lichao@loongson.cn> <2e42db7c-a927-f47b-7d80-632895b11e1b@redhat.com> <1069d005-ef80-46c6-89fa-f3fed316e4ad@loongson.cn> <8c11be85-051e-382e-14a2-736d7565fb54@redhat.com> From: "Chao Li" In-Reply-To: <8c11be85-051e-382e-14a2-736d7565fb54@redhat.com> X-CM-TRANSID: AQAAf8DxkuNx13dlxx0AAA--.1042S3 X-CM-SenderInfo: xolfxt3r6o00pqjv00gofq/1tbiAQADCGV3wy0BCgABs8 X-Coremail-Antispam: 1Uk129KBj93XoWfJr4Utw15Jw48ZF4Dtr47Awc_yoWDtF1rpF 4UtFsrWFn7t3WSyryxA3yjqFWYkFs5Aa45GF15Kryqyr1kJFn2g3Z7trZ0gFy3u393Zr4I vr4jgwsxuF4DGagCm3ZEXasCq-sJn29KB7ZKAUJUUUU5529EdanIXcx71UUUUU7KY7ZEXa sCq-sGcSsGvfJ3UbIjqfuFe4nvWSU5nxnvy29KBjDU0xBIdaVrnRJUUUkEb4IE77IF4wAF F20E14v26r1j6r4UM7CY07I20VC2zVCF04k26cxKx2IYs7xG6rWj6s0DM7CIcVAFz4kK6r 1j6r18M28lY4IEw2IIxxk0rwA2F7IY1VAKz4vEj48ve4kI8wA2z4x0Y4vE2Ix0cI8IcVAF wI0_JFI_Gr1l84ACjcxK6xIIjxv20xvEc7CjxVAFwI0_Gr0_Cr1l84ACjcxK6I8E87Iv67 AKxVW8Jr0_Cr1UM28EF7xvwVC2z280aVCY1x0267AKxVW8Jr0_Cr1UM2AIxVAIcxkEcVAq 07x20xvEncxIr21l57IF6xkI12xvs2x26I8E6xACxx1lYx0E2Ix0cI8IcVAFwI0_Jrv_JF 1lYx0Ex4A2jsIE14v26r1j6r4UMcvjeVCFs4IE7xkEbVWUJVW8JwACjcxG0xvEwIxGrwCj r7xvwVCIw2I0I7xG6c02F41lc7CjxVAaw2AFwI0_JF0_Jw1l42xK82IYc2Ij64vIr41l4I 8I3I0E4IkC6x0Yz7v_Jr0_Gr1lx2IqxVAqx4xG67AKxVWUGVWUWwC20s026x8GjcxK67AK xVWUGVWUWwC2zVAF1VAY17CE14v26r1q6r43MIIYrxkI7VAKI48JMIIF0xvE2Ix0cI8IcV AFwI0_Jr0_JF4lIxAIcVC0I7IYx2IY6xkF7I0E14v26r1j6r4UMIIF0xvE42xK8VAvwI8I cIk0rVWUJVWUCwCI42IY6I8E87Iv67AKxVWUJVW8JwCI42IY6I8E87Iv6xkF7I0E14v26r 1j6r4UYxBIdaVFxhVjvjDU0xZFpf9x07UAUUUUUUUU= Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lichao@loongson.cn List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: UshnhmlbfGjDhSIWR1Oi6HoAx7686176AA= Content-Type: multipart/alternative; boundary="------------YsUA9FGcduy0A0KBrn4c6NVF" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=J+IEabw+; dmarc=none; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io --------------YsUA9FGcduy0A0KBrn4c6NVF Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Hi Laszlo, Thanks for reviewing again this patch while you are sick and please take=20 care. My opinion is as follow: Thanks, Chao On 2023/12/12 01:16, Laszlo Ersek wrote: > 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=3D4584 >>>> >>>> Cc: Eric Dong >>>> Cc: Ray Ni >>>> Cc: Rahul Kumar >>>> Cc: Gerd Hoffmann >>>> Signed-off-by: Chao Li >>>> --- >>>> .../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/Bas= eLoongArch64CpuTimerLib.inf >>>> create mode 100644 UefiCpuPkg/Library/BaseLoongArch64CpuTimerLib/Bas= eLoongArch64CpuTimerLib.uni >>>> create mode 100644 UefiCpuPkg/Library/BaseLoongArch64CpuTimerLib/Cpu= TimerLib.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: >>> >>> >>> >>> 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/blo= b/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 > ([]). > > Your naming seems to follow the first alternative > ([]). OK. > > However, that's still not a perfect match. For , you have "Base". > For , you have "LoongArch64". But for , you > have "CpuTimerLib", and that's wrong. We don't have a "CpuTimerLib" > class; instead it's the "TimerLib" class. The part of "CpuTimerLib" uses the name located at=20 UefiCpuPkg/Library/CpuTimerLib, so this library is named=20 BaseLoongArch64CpuTimerLib. > > 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 [], as a suffix, under the first > naming scheme alternative: "BaseLoongArch64TimerLibCpu". I think the name "CpuTimerLib" highlights the "CPU", because this=20 library relies on the CPU interal timer. So should I use the=20 "CpuTimerLib" as ? 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 *=3D *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.i= nf > 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.in= f > > 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=20 instance that implements dependencies should be under the UefiCpuPkg,=20 and the architecture common library or driver should be under the MdePkg. Liming, Mike and Ray, what suggestions do you have? > >>>> + 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 =3D 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 agree. > >>>> + 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. Ok, I will add the CpuDeadLoop in the V4. > >>> (25) This proves that, generally speaking, the final "+=3D" 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 "+=3D" to the SafeIntLib way? >> Something like: NanoSeconds =3D 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. I have noticed. > > ... 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). Please take care. > > Thanks > Laszlo > > > >=20 > -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112355): https://edk2.groups.io/g/devel/message/112355 Mute This Topic: https://groups.io/mt/102644766/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- --------------YsUA9FGcduy0A0KBrn4c6NVF Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable

Hi Laszlo,

Thanks for reviewing again this patch while you are sick and please take care.

My opinion is as follow:


=
Thanks,
Chao
On 2023/12/12 01:16, Laszlo Ersek wrote:
On 11/23/23 12:43, Chao Li wro=
te:

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=
=3D4584

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/BaseLoong=
Arch64CpuTimerLib.inf
 create mode 100644 UefiCpuPkg/Library/BaseLoongArch64CpuTimerLib/BaseLoong=
Arch64CpuTimerLib.uni
 create mode 100644 UefiCpuPkg/Library/BaseLoongArch64CpuTimerLib/CpuTimerL=
ib.c

(1) sorry about the annoyi=
ng 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 id=
entifier>

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-CCodingStandardsSp=
ecification/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 "Bas=
e".
For <CpuArch>, you have "LoongArch64". But for <LibraryClassName&g=
t;, you
have "CpuTimerLib", and that's wrong. We don't have a "CpuTimerLib"
class; instead it's the "TimerLib" class.
The part of "CpuTimerLib" uses the name located at UefiCpuPkg/Library/CpuTimerLib, so this library is named BaseLoongArch64CpuTimerLib. 

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 firs=
t
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 TimerLi=
b 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 *=3D *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?

    

    

      



      

        

          

            
+  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: fi=
rst '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        =3D CCFreq.Bits.CC_FREQ;


          

        

      

      

      

        

          
(11) My comment here proba=
bly 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 agree.

    

      



      

        

          

            
+    ASSERT (0);


          

          
(15) Should be ASSERT (FAL=
SE).

(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.

    

    Ok, I will add the CpuDeadLoop in the V4.

    

      



      

        

          
(25) This proves that, gen=
erally speaking, the final "+=3D" 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 th=
e final "+=3D" to the SafeIntLib way?
Something like: NanoSeconds =3D 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.

    

    I have noticed.

    

      

... 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).

    

    Please take care.

    

      

Thanks
Laszlo







    

  




_._,_._,_





Groups.io Links:



 =20
    You receive all messages sent to this group.
 =20
 =20




View/Reply Online (#112355) |


 =20

|

  Mute=
 This Topic


| New Topic







Your Subscriptio=
n |
Contact Group Owner |

Unsubscribe

 [rebecca@openfw.io]


_._,_._,_



--------------YsUA9FGcduy0A0KBrn4c6NVF--