Re: [PATCH] UefiCpuPkg: Adding a new TSC library by using CPUID(0x15) TSC leaf

Donald Kuo
 

Hi Liming,

Done.

Patch is attached to https://bugzilla.tianocore.org/show_bug.cgi?id=1909

Another BZ to apply CpuTimerLib will be tracking on: https://bugzilla.tianocore.org/show_bug.cgi?id=2096

Thanks,
Donald

-----Original Message-----
From: Gao, Liming
Sent: Tuesday, August 20, 2019 2:51 PM
To: Kuo, Donald <donald.kuo@...>; devel@edk2.groups.io;
lersek@...; Dong, Eric <eric.dong@...>
Cc: Ni, Ray <ray.ni@...>; Zeng, Star <star.zeng@...>; Chan,
Amy <amy.chan@...>; Chaganty, Rangasai V
<rangasai.v.chaganty@...>; Lai, Luke <luke.lai@...>; Li, Kevin
Y <kevin.y.li@...>; leif.lindholm@...; afish@...; Kinney,
Michael D <michael.d.kinney@...>
Subject: RE: [edk2-devel] [PATCH] UefiCpuPkg: Adding a new TSC library by
using CPUID(0x15) TSC leaf

Donald:
Please also attach the patch linker in BZs.

And, please submit another BZ for edk2-
platforms\Platform\Intel\KabylakeOpenBoardPkg to apply this new library
instance.

Thanks
Liming
-----Original Message-----
From: Kuo, Donald
Sent: Tuesday, August 20, 2019 10:44 AM
To: devel@edk2.groups.io; lersek@...; Gao, Liming
<liming.gao@...>; Dong, Eric <eric.dong@...>
Cc: Ni, Ray <ray.ni@...>; Zeng, Star <star.zeng@...>; Chan,
Amy <amy.chan@...>; Chaganty, Rangasai V
<rangasai.v.chaganty@...>; Lai, Luke <luke.lai@...>; Li,
Kevin Y <kevin.y.li@...>; leif.lindholm@...;
afish@...; Kinney, Michael D <michael.d.kinney@...>
Subject: RE: [edk2-devel] [PATCH] UefiCpuPkg: Adding a new TSC library
by using CPUID(0x15) TSC leaf

Thanks Laszlo help to review and great feedbacks. That we did miss to fulfil
BZ.

I had updated Bugzilla
https://bugzilla.tianocore.org/show_bug.cgi?id=1909
for more documentation.

As I know for the edk2-platforms should be consumed as KBL (7th
Generation) platform in Client, and this feature based on SDM is
supported on SKL (6th Generation, Family 06h) onwards. So it's ok to
use as TimerLib instances for edk2-platforms.

And I think the library is new instances for TimerLib for supported
CPU, and those non-supported CPU will still keep using AcpiTimerlib as
TimerLib instances.

Thanks,
Donald

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
Laszlo Ersek
Sent: Saturday, August 17, 2019 4:40 AM
To: Gao, Liming <liming.gao@...>; Kuo, Donald
<donald.kuo@...>; Dong, Eric <eric.dong@...>;
devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@...>; Zeng, Star <star.zeng@...>;
Chan, Amy <amy.chan@...>; Chaganty, Rangasai V
<rangasai.v.chaganty@...>; Lai, Luke <luke.lai@...>; Li,
Kevin Y <kevin.y.li@...>; leif.lindholm@...;
afish@...;
Kinney,
Michael D <michael.d.kinney@...>
Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Adding a new TSC
library by using CPUID(0x15) TSC leaf

On 08/16/19 18:16, Laszlo Ersek wrote:
On 08/15/19 06:02, Gao, Liming wrote:
Donald: This change is a new feature. Now, it is not in edk2
feature planning list. If you want to catch it into 201908 stable
tag, please get approve from Stewards first. I have cc this mail to all
Stewards.
- I don't mind adding a new feature, as long as it gets properly
reviewed by package owners before we enter the soft feature freeze.

- Looking at the BZ
<https://bugzilla.tianocore.org/show_bug.cgi?id=1909>, a bit more
documentation would be nice.

- On the negative side, I'm very much *not* a fan of adding
features to the open source edk2 tree without actually *consuming*
the feature in an open source tree. Are the new library instances
going to be put to use in edk2-platforms, perhaps?

We discussed this topic earlier on some of the stewards' calls. On
one hand, it's not uncommon to see library instances from Intel
enter core
edk2 packages without any dependent platform code, or even a
detailed problem statement / purpose description (see e.g. commit
5c9bb86f171c and its surrounding commits). On the other hand,
attempts in the past, to add libraries with well demonstrated and
direct in-tree use cases, to
edk2 core, have been rejected, from other submitters. (Here's one
example: <https://bugzilla.tianocore.org/show_bug.cgi?id=957>.) I'm
not prying at proprietary platform information, but a new library
added to
edk2 core *should* be well-justified.

The commit message on this patch is empty. It only references
<https://bugzilla.tianocore.org/show_bug.cgi?id=1909>. And if I
open the BZ, this is all I get:

Need a new TSC library to check the CPUID leaf (EAX=0x15) for TSC.
For new platform (start from SKL) can use CPUID and retire/remove
the current override from AcpiTimerLib.

Does this read like an actual feature request? (TimerLib is an
MdePkg library class, so not exactly "niche".)
In comparison, the following email does read like a feature request:

[edk2-devel] Determining TSC frequency programmatically
https://edk2.groups.io/g/devel/message/45750
http://mid.mail-archive.com/8EC14D0D-DFA5-412D-A4E1-
4D641576D58E@...

If the posting is related to TianoCore#1909, then I urge the BZ
assignee to please reference the message in the TianoCore BZ.

Thanks
Laszlo

Join devel@edk2.groups.io to automatically receive all group messages.