[v3,1/8] ACPI: CPPC: Add AMD pstate energy performance preference cppc control

  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.

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.

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(+)
  

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.

> +
> +	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;
> +	}
> +
> +	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?

> +	}
> +
> +	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*/
  

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.

> > +
> > +     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?

> > +
> > +     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?

> > +
> > +     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*/
>
  

[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*/
> >
  

On Thu, Nov 10, 2022 at 4:52 PM Yuan, Perry <Perry.Yuan@amd.com> wrote:
>
> [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 ?

Yes, if it cannot be updated, it should be treated the same way as
unsupported IMV.
  

[AMD Official Use Only - General]

Hi Rafael. 

> -----Original Message-----
> From: Rafael J. Wysocki <rafael@kernel.org>
> Sent: Thursday, November 10, 2022 11:56 PM
> To: Yuan, Perry <Perry.Yuan@amd.com>
> Cc: Rafael J. Wysocki <rafael@kernel.org>; Limonciello, Mario
> <Mario.Limonciello@amd.com>; 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 Thu, Nov 10, 2022 at 4:52 PM Yuan, Perry <Perry.Yuan@amd.com> wrote:
> >
> > [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 ?
> 
> Yes, if it cannot be updated, it should be treated the same way as
> unsupported IMV.

Make sense, I will make change for this in V4 and you can help to take a look at V4.
Thanks. 

Perry .
  

[AMD Official Use Only - General]

Hi Mario,

> -----Original Message-----
> From: Limonciello, Mario <Mario.Limonciello@amd.com>
> Sent: Tuesday, November 8, 2022 2:45 AM
> To: Yuan, Perry <Perry.Yuan@amd.com>; rafael.j.wysocki@intel.com; Huang,
> Ray <Ray.Huang@amd.com>; viresh.kumar@linaro.org
> Cc: 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
> 
> 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.
> 

Thanks for your suggestions, fixed in V4. 

> >
> > 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."
> 

Changed in v4 as you suggested. 

> >
> > 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.


I made some new changes and combined the two Epp call functions.
Remove some unnecessary log printing.
Please help to take a look at the V4 if you have any concerns.

Perry .


> 
> > +
> > +	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;
> > +	}
> > +
> > +	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.
> 


Fixed in V4.

> > +		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.

Fixed in V4.


> 
> > +		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?


Good idea, Ray also suggested to merge them.
I combined the two calls into single in V4.
Please take a look.

Perry. 

> 
> > +	}
> > +
> > +	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*/