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

Donald Kuo
 

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.