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: 5369 bytes --]

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.

-- 
 i.