All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Yuan, Perry" <Perry.Yuan@amd.com>
To: "Rafael J. Wysocki" <rafael@kernel.org>,
	"Limonciello, Mario" <Mario.Limonciello@amd.com>
Cc: "rafael.j.wysocki@intel.com" <rafael.j.wysocki@intel.com>,
	"Huang, Ray" <Ray.Huang@amd.com>,
	"viresh.kumar@linaro.org" <viresh.kumar@linaro.org>,
	"Sharma, Deepak" <Deepak.Sharma@amd.com>,
	"Fontenot, Nathan" <Nathan.Fontenot@amd.com>,
	"Deucher, Alexander" <Alexander.Deucher@amd.com>,
	"Huang, Shimmer" <Shimmer.Huang@amd.com>,
	"Du, Xiaojian" <Xiaojian.Du@amd.com>,
	"Meng, Li (Jassmine)" <Li.Meng@amd.com>,
	"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v3 1/8] ACPI: CPPC: Add AMD pstate energy performance preference cppc control
Date: Thu, 10 Nov 2022 15:51:57 +0000	[thread overview]
Message-ID: <DM4PR12MB527881961B4BE3F74F503CFE9C019@DM4PR12MB5278.namprd12.prod.outlook.com> (raw)
In-Reply-To: <CAJZ5v0ik68V6D2tipGH4tepaAmy5bpSy2nZUyAHn=Qia9SCLzA@mail.gmail.com>

[AMD Official Use Only - General]



> -----Original Message-----
> From: Rafael J. Wysocki <rafael@kernel.org>
> Sent: Thursday, November 10, 2022 10:50 PM
> To: Limonciello, Mario <Mario.Limonciello@amd.com>; Yuan, Perry
> <Perry.Yuan@amd.com>
> Cc: rafael.j.wysocki@intel.com; Huang, Ray <Ray.Huang@amd.com>;
> viresh.kumar@linaro.org; Sharma, Deepak <Deepak.Sharma@amd.com>;
> Fontenot, Nathan <Nathan.Fontenot@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Huang, Shimmer
> <Shimmer.Huang@amd.com>; Du, Xiaojian <Xiaojian.Du@amd.com>; Meng,
> Li (Jassmine) <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v3 1/8] ACPI: CPPC: Add AMD pstate energy
> performance preference cppc control
> 
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
> 
> 
> On Mon, Nov 7, 2022 at 7:44 PM Limonciello, Mario
> <mario.limonciello@amd.com> wrote:
> >
> > On 11/7/2022 11:56, Perry Yuan wrote:
> > > Add the EPP(Energy Performance Preference) support for the AMD SoCs
> > > without the dedicated CPPC MSR, those SoCs need to add this cppc
> > > acpi functions to update EPP values and desired perf value.
> >
> > As far as I can tell this is generic code.  Although the reason you're
> > submitting it is for enabling AMD SoCs, the commit message should be
> > worded as such.
> >
> > >
> > > In order to get EPP worked, cppc_get_epp_caps() will query EPP
> > > preference value and cppc_set_epp_perf() will set EPP new value.
> > > Before the EPP works, pstate driver will use cppc_set_auto_epp() to
> > > enable EPP function from firmware firstly.
> >
> > This could more succinctly say:
> >
> > "Add support for setting and querying EPP preferences to the generic
> > CPPC driver.  This enables downstream drivers such as amd-pstate to
> > discover and use these values."
> >
> > >
> > > Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
> > > ---
> > >   drivers/acpi/cppc_acpi.c | 126
> +++++++++++++++++++++++++++++++++++++++
> > >   include/acpi/cppc_acpi.h |  17 ++++++
> > >   2 files changed, 143 insertions(+)
> > >
> > > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> > > index 093675b1a1ff..d9c38dee1f48 100644
> > > --- a/drivers/acpi/cppc_acpi.c
> > > +++ b/drivers/acpi/cppc_acpi.c
> > > @@ -1365,6 +1365,132 @@ int cppc_get_perf_ctrs(int cpunum, struct
> cppc_perf_fb_ctrs *perf_fb_ctrs)
> > >   }
> > >   EXPORT_SYMBOL_GPL(cppc_get_perf_ctrs);
> > >
> > > +/**
> > > + * cppc_get_epp_caps - Get the energy preference register value.
> > > + * @cpunum: CPU from which to get epp preference level.
> > > + * @perf_caps: Return address.
> > > + *
> > > + * Return: 0 for success, -EIO otherwise.
> > > + */
> > > +int cppc_get_epp_caps(int cpunum, struct cppc_perf_caps *perf_caps)
> > > +{
> > > +     struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
> > > +     struct cpc_register_resource *energy_perf_reg;
> > > +     u64 energy_perf;
> > > +
> > > +     if (!cpc_desc) {
> > > +             pr_warn("No CPC descriptor for CPU:%d\n", cpunum);
> > > +             return -ENODEV;
> > > +     }
> > > +
> > > +     energy_perf_reg = &cpc_desc->cpc_regs[ENERGY_PERF];
> > > +
> > > +     if (!CPC_SUPPORTED(energy_perf_reg))
> > > +             pr_warn("energy perf reg update is unsupported!\n");
> >
> > No need to add a explanation point at the end.
> >
> > As this is a per-CPU message I wonder if this would be better as
> > pr_warn_once()?  Othewrise some systems with large numbers of cores
> > might potentially show this message quite a few times.
> 
> pr_info_once() would suffice IMO.

Fixed in V4. 

> 
> > > +
> > > +     if (CPC_IN_PCC(energy_perf_reg)) {
> > > +             int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
> > > +             struct cppc_pcc_data *pcc_ss_data = NULL;
> > > +             int ret = 0;
> > > +
> > > +             if (pcc_ss_id < 0)
> > > +                     return -ENODEV;
> > > +
> > > +             pcc_ss_data = pcc_data[pcc_ss_id];
> > > +
> > > +             down_write(&pcc_ss_data->pcc_lock);
> > > +
> > > +             if (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0) {
> > > +                     cpc_read(cpunum, energy_perf_reg, &energy_perf);
> > > +                     perf_caps->energy_perf = energy_perf;
> > > +             } else {
> > > +                     ret = -EIO;
> > > +             }
> > > +
> > > +             up_write(&pcc_ss_data->pcc_lock);
> > > +
> > > +             return ret;
> > > +     }
> 
> What if CPC is not in PCC?
> 
> Would returning 0 then work for all users?

Fixed in V4

> 
> > > +
> > > +     return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cppc_get_epp_caps);
> > > +
> > > +int cppc_set_auto_epp(int cpu, bool enable) {
> > > +     int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
> > > +     struct cpc_register_resource *auto_sel_reg;
> > > +     struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
> > > +     struct cppc_pcc_data *pcc_ss_data = NULL;
> > > +     int ret = -EINVAL;
> > > +
> > > +     if (!cpc_desc) {
> > > +             pr_warn("No CPC descriptor for CPU:%d\n", cpu);
> >
> > Is this actually warn worthy?  I would think it's fine a debug like we
> > have for the other _CPC missing messages.
> >
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE];
> > > +
> > > +     if (CPC_IN_PCC(auto_sel_reg)) {
> > > +             if (pcc_ss_id < 0)
> > > +                     return -ENODEV;
> > > +
> > > +             ret = cpc_write(cpu, auto_sel_reg, enable);
> > > +             if (ret)
> > > +                     return ret;
> > > +
> > > +             pcc_ss_data = pcc_data[pcc_ss_id];
> > > +
> > > +             down_write(&pcc_ss_data->pcc_lock);
> > > +             /* after writing CPC, transfer the ownership of PCC to platform */
> > > +             ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
> > > +             up_write(&pcc_ss_data->pcc_lock);
> > > +             return ret;
> > > +     }
> > > +
> > > +     return cpc_write(cpu, auto_sel_reg, enable); }
> > > +EXPORT_SYMBOL_GPL(cppc_set_auto_epp);
> > > +
> > > +/*
> > > + * Set Energy Performance Preference Register value through
> > > + * Performance Controls Interface
> > > + */
> > > +int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
> > > +{
> > > +     int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
> > > +     struct cpc_register_resource *epp_set_reg;
> > > +     struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
> > > +     struct cppc_pcc_data *pcc_ss_data = NULL;
> > > +     int ret = -EINVAL;
> > > +
> > > +     if (!cpc_desc) {
> > > +             pr_warn("No CPC descriptor for CPU:%d\n", cpu);
> >
> > Is this actually warn worthy?  I would think it's fine a debug like we
> > have for the other _CPC missing messages.
> >
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     epp_set_reg = &cpc_desc->cpc_regs[ENERGY_PERF];
> > > +
> > > +     if (CPC_IN_PCC(epp_set_reg)) {
> > > +             if (pcc_ss_id < 0)
> > > +                     return -ENODEV;
> > > +
> > > +             ret = cpc_write(cpu, epp_set_reg, perf_ctrls->energy_perf);
> > > +             if (ret)
> > > +                     return ret;
> > > +
> > > +             pcc_ss_data = pcc_data[pcc_ss_id];
> > > +
> > > +             down_write(&pcc_ss_data->pcc_lock);
> > > +             /* after writing CPC, transfer the ownership of PCC to platform */
> > > +             ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
> > > +             up_write(&pcc_ss_data->pcc_lock);
> >
> > cppc_set_auto_epp and cppc_set_epp_perf have nearly the same code in
> > the if block.  I wonder if it's worth having a static helper function
> > for this purpose that takes "reg" and "value" as arguments?
> >
> > > +     }
> 
> And what about the non-PCC case here?

I merge the  cppc_set_auto_epp and cppc_set_epp_perf in V4. 
For the non-PCC case, we canno set the EPP value to FW, then just returned 
Error code.  Is it Ok ?


> 
> > > +
> > > +     return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cppc_set_epp_perf);
> > > +
> > >   /**
> > >    * cppc_set_enable - Set to enable CPPC on the processor by writing the
> > >    * Continuous Performance Control package EnableRegister field.
> > > diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
> > > index c5614444031f..10d91aeedaca 100644
> > > --- a/include/acpi/cppc_acpi.h
> > > +++ b/include/acpi/cppc_acpi.h
> > > @@ -108,12 +108,14 @@ struct cppc_perf_caps {
> > >       u32 lowest_nonlinear_perf;
> > >       u32 lowest_freq;
> > >       u32 nominal_freq;
> > > +     u32 energy_perf;
> > >   };
> > >
> > >   struct cppc_perf_ctrls {
> > >       u32 max_perf;
> > >       u32 min_perf;
> > >       u32 desired_perf;
> > > +     u32 energy_perf;
> > >   };
> > >
> > >   struct cppc_perf_fb_ctrs {
> > > @@ -149,6 +151,9 @@ extern bool cpc_ffh_supported(void);
> > >   extern bool cpc_supported_by_cpu(void);
> > >   extern int cpc_read_ffh(int cpunum, struct cpc_reg *reg, u64 *val);
> > >   extern int cpc_write_ffh(int cpunum, struct cpc_reg *reg, u64
> > > val);
> > > +extern int cppc_set_auto_epp(int cpu, bool enable); extern int
> > > +cppc_get_epp_caps(int cpunum, struct cppc_perf_caps *perf_caps);
> > > +extern int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls
> > > +*perf_ctrls);
> > >   #else /* !CONFIG_ACPI_CPPC_LIB */
> > >   static inline int cppc_get_desired_perf(int cpunum, u64 *desired_perf)
> > >   {
> > > @@ -202,6 +207,18 @@ static inline int cpc_write_ffh(int cpunum, struct
> cpc_reg *reg, u64 val)
> > >   {
> > >       return -ENOTSUPP;
> > >   }
> > > +static inline int cppc_set_auto_epp(int cpu, bool enable) {
> > > +     return -ENOTSUPP;
> > > +}
> > > +static inline int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls
> > > +*perf_ctrls) {
> > > +     return -ENOTSUPP;
> > > +}
> > > +static inline int cppc_get_epp_caps(int cpunum, struct
> > > +cppc_perf_caps *perf_caps) {
> > > +     return -ENOTSUPP;
> > > +}
> > >   #endif /* !CONFIG_ACPI_CPPC_LIB */
> > >
> > >   #endif /* _CPPC_ACPI_H*/
> >

  reply	other threads:[~2022-11-10 15:52 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-07 17:56 [PATCH v3 0/8] Implement AMD Pstate EPP Driver Perry Yuan
2022-11-07 17:56 ` [PATCH v3 1/8] ACPI: CPPC: Add AMD pstate energy performance preference cppc control Perry Yuan
2022-11-07 18:44   ` Limonciello, Mario
2022-11-10 14:49     ` Rafael J. Wysocki
2022-11-10 15:51       ` Yuan, Perry [this message]
2022-11-10 15:55         ` Rafael J. Wysocki
2022-11-10 16:23           ` Yuan, Perry
2022-11-13 16:28     ` Yuan, Perry
2022-11-07 17:56 ` [PATCH v3 2/8] Documentation: amd-pstate: add EPP profiles introduction Perry Yuan
2022-11-07 18:33   ` Limonciello, Mario
2022-11-13 16:23     ` Yuan, Perry
2022-11-10 14:57   ` Rafael J. Wysocki
2022-11-10 15:07     ` Yuan, Perry
2022-11-07 17:57 ` [PATCH v3 3/8] cpufreq: amd-pstate: change driver to be built-in type Perry Yuan
2022-11-07 18:29   ` Limonciello, Mario
2022-11-13 16:21     ` Yuan, Perry
2022-11-07 17:57 ` [PATCH v3 4/8] cpufreq: amd_pstate: add AMD Pstate EPP support for the MSR based processors Perry Yuan
2022-11-07 20:32   ` Limonciello, Mario
2022-11-10 15:59     ` Nathan Fontenot
2022-11-10 16:22       ` Yuan, Perry
2022-11-08  7:21   ` kernel test robot
2022-11-09  5:55   ` kernel test robot
2022-11-07 17:57 ` [PATCH v3 5/8] cpufreq: amd_pstate: implement amd pstate cpu online and offline callback Perry Yuan
2022-11-07 18:22   ` Limonciello, Mario
2022-11-13 16:19     ` Yuan, Perry
2022-11-07 17:57 ` [PATCH v3 6/8] cpufreq: amd-pstate: implement suspend and resume callbacks Perry Yuan
2022-11-07 18:18   ` Limonciello, Mario
2022-11-13 16:19     ` Yuan, Perry
2022-11-10 16:19   ` Nathan Fontenot
2022-11-07 17:57 ` [PATCH v3 7/8] cpufreq: amd-pstate: add frequency dynamic boost sysfs control Perry Yuan
2022-11-07 18:16   ` Limonciello, Mario
2022-11-07 19:09     ` Limonciello, Mario
2022-11-07 17:57 ` [PATCH v3 8/8] cpufreq: amd_pstate: add driver working mode status sysfs entry Perry Yuan
2022-11-07 18:10   ` Limonciello, Mario
2022-11-13 16:18     ` Yuan, Perry
2022-11-10 16:06   ` Nathan Fontenot
2022-11-10 16:49     ` Yuan, Perry

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DM4PR12MB527881961B4BE3F74F503CFE9C019@DM4PR12MB5278.namprd12.prod.outlook.com \
    --to=perry.yuan@amd.com \
    --cc=Alexander.Deucher@amd.com \
    --cc=Deepak.Sharma@amd.com \
    --cc=Li.Meng@amd.com \
    --cc=Mario.Limonciello@amd.com \
    --cc=Nathan.Fontenot@amd.com \
    --cc=Ray.Huang@amd.com \
    --cc=Shimmer.Huang@amd.com \
    --cc=Xiaojian.Du@amd.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=rafael.j.wysocki@intel.com \
    --cc=rafael@kernel.org \
    --cc=viresh.kumar@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.