On 3/6/2024 16:04, Ilpo Järvinen wrote:
> On Wed, 6 Mar 2024, Shyam Sundar S K wrote:
>> 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?
>
> Hi,
>
> I already looked at them briefly and didn't come across other things to
> say except that the use arrays made things cleaner. :-) So please just
> respin.
>
Sure. You want this to be rebased to review-hans or review-ilpo tree?
Thanks,
Shyam
