On 2/29/2024 18:49, Ilpo Järvinen wrote:
> On Wed, 28 Feb 2024, Shyam Sundar S K wrote:
>
>>
>>
>> On 2/27/2024 18:58, Ilpo Järvinen wrote:
>>> On Tue, 27 Feb 2024, Shyam Sundar S K wrote:
>>>
>>>> Update the APMF function index 2 for family 1Ah, that gets the
>>>> information of SBIOS requests (like the pending requests from BIOS,
>>>
>>> extra space.
>>>
>>>> custom notifications, updation of power limits etc).
>>>>
>>>> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@xxxxxxx>
>>>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@xxxxxxx>
>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx>
>>>> ---
>>>> drivers/platform/x86/amd/pmf/acpi.c | 6 ++++++
>>>> drivers/platform/x86/amd/pmf/pmf.h | 13 +++++++++++++
>>>> 2 files changed, 19 insertions(+)
>>>>
>>>> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
>>>> index 1f287a147c57..1b2a099c0cef 100644
>>>> --- a/drivers/platform/x86/amd/pmf/acpi.c
>>>> +++ b/drivers/platform/x86/amd/pmf/acpi.c
>>>> @@ -166,6 +166,12 @@ int apmf_get_auto_mode_def(struct amd_pmf_dev *pdev, struct apmf_auto_mode *data
>>>> return apmf_if_call_store_buffer(pdev, APMF_FUNC_AUTO_MODE, data, sizeof(*data));
>>>> }
>>>>
>>>> +int apmf_get_sbios_requests_v2(struct amd_pmf_dev *pdev, struct apmf_sbios_req_v2 *req)
>>>> +{
>>>> + return apmf_if_call_store_buffer(pdev, APMF_FUNC_SBIOS_REQUESTS,
>>>> + req, sizeof(*req));
>>>
>>> Fix the alignment please.
>>>
>>>> +}
>>>> +
>>>> int apmf_get_sbios_requests(struct amd_pmf_dev *pdev, struct apmf_sbios_req *req)
>>>> {
>>>> return apmf_if_call_store_buffer(pdev, APMF_FUNC_SBIOS_REQUESTS,
>>>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
>>>> index 4364af72a7a3..f11d2a348696 100644
>>>> --- a/drivers/platform/x86/amd/pmf/pmf.h
>>>> +++ b/drivers/platform/x86/amd/pmf/pmf.h
>>>> @@ -116,6 +116,18 @@ struct apmf_sbios_req {
>>>> u8 skin_temp_hs2;
>>>> } __packed;
>>>>
>>>> +struct apmf_sbios_req_v2 {
>>>> + u16 size;
>>>> + u32 pending_req;
>>>> + u8 rsvd;
>>>> + u32 update_ppt_pmf;
>>>> + u32 update_ppt_pmf_apu_only;
>>>> + u32 update_stt_min;
>>>> + u8 update_stt_apu;
>>>> + u8 update_stt_hs2;
>>>
>>> Is it intentional that these do not match with the names in struct
>>> apmf_sbios_req? I mean some of the fields look suspiciously close in name
>>> so is the purpose still the same and somebody just invented new names for the
>>> same field?
>>
>> The idea is to optimize certain fields in the BIOS menu that OEMs have
>> to feed in while making the right choices for the power settings for
>> different features.
>>
>> The entire series is targeted towards that where the interface between
>> the driver and the BIOS is improvised so that:
>>
>> - Multiple features can link to one state, so OEMs doesn’t need to
>> program same parameters in multiple locations.
>> - If we need to add new power controller limits , we don’t have to
>> touch APMF functions, its more expandable adding new fields in APS
>> methods.
>>
>> To answers to your question of apmf_sbios_req vs apmf_sbios_req_v2:
>>
>> It calls for a new struct _v2, because:
>> - AMT support has been dropped so there shall be no pending events
>> from DYTC (like the CQL and AMT)
>> - As per the new design, the PMFW has given control to set PPT and STT
>> limits and no pending requests on updating SPL limits.
>>
>> But as per names, I don't think there is no new invention :-)
>
> Hi again,
>
> I'm fine with adding _v2 struct (I could see they're not identical).
>
> What I'm still left unsure if we spoke past each other so let me state
> the question in more concrete terms:
>
> - apmf_sbios_req has a field called stt_min_limit
> - apmf_sbios_req_v2 has a field called update_stt_min
>
> My question is, are those just the same but only named differently for
> some reason, or does the "limit" and/or "update" difference actually imply
> there's change in how that field is used?
Hi Ilpo,
Apologies for the long delay.
Your question is valid and hence I had to go back to my FW
counterparts to get my basics right before responding back.
So the crux is, for each of the power controller within the CPU
infrastructure, like the Slow PPT, Fast PPT, STAPM, TDC SOC, EDC VDD
etc., all of them are guarded by two parameters:
- one, "limit", a max threshold a software can set
- two, "value", that can be updated to based on the changing system
dynamics.
So, atleast in the PMF driver context the field names can remain
constant. The field names in apmf_sbios_req looks apt here, so in the
next revision I will make fields in apmf_sbios_req and
apmf_sbios_req_v2 look the same (w.r.t the naming).
Before respin, can you have a look at the other patches and see if you
have remarks?
Thanks,
Shyam
>
> Similar question applies to the other fields which look close but not
> identical. There's no need for you to itemize and explain each field for
> me specifically in the reply, I just prefer the same thing called the same
> in both structs if that's the case.
>
> It could be you tried to answer this with your second bullet but I just
> don't understand its meaning deeply enough, thus I'm asking again, please
> bear with me.
>
>> FYI, here PPT means Power Packaging Tracking, so it could be SPPT
>> (Slow PPT) or FPPT (Fast PPT) and SST means Skin Temperature Tracking.
>
>
