Re: [PATCH 3/7] platform/x86/amd/pmf: Add support to get sbios requests in PMF driver

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 5875 bytes --]

On Wed, 6 Mar 2024, Shyam Sundar S K wrote:
> 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@amd.com>
> >>>>>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> >>>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> >>>>>> ---
> >>>>>>  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?

I'm handling for-next for this cycle so review-ilpo.

-- 
 i.