From: Jinzhou Su <[email protected]>
Add a new function to enable CPPC feature. This function
will write Continuous Performance Control package
EnableRegister field on the processor.
Signed-off-by: Jinzhou Su <[email protected]>
Signed-off-by: Huang Rui <[email protected]>
---
drivers/acpi/cppc_acpi.c | 48 ++++++++++++++++++++++++++++++++++++++++
include/acpi/cppc_acpi.h | 5 +++++
2 files changed, 53 insertions(+)
diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 2efe2ba97d96..b285960c35e7 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -1220,6 +1220,54 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
}
EXPORT_SYMBOL_GPL(cppc_get_perf_ctrs);
+/**
+ * cppc_set_enable - Set to enable CPPC on the processor by writing the
+ * Continuous Performance Control package EnableRegister feild.
+ * @cpu: CPU for which to enable CPPC register.
+ * @enable: 0 - disable, 1 - enable CPPC feature on the processor.
+ *
+ * Return: 0 for success, -ERRNO or -EIO otherwise.
+ */
+int cppc_set_enable(int cpu, u32 enable)
+{
+ int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
+ struct cpc_register_resource *enable_reg;
+ struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
+ struct cppc_pcc_data *pcc_ss_data = NULL;
+ int ret = -1;
+
+ /* check the input value*/
+ if (cpu < 0 || cpu > num_possible_cpus() - 1 || enable > 1)
+ return -ENODEV;
+
+ if (!cpc_desc) {
+ pr_debug("No CPC descriptor for CPU:%d\n", cpu);
+ return -ENODEV;
+ }
+
+ enable_reg = &cpc_desc->cpc_regs[ENABLE];
+
+ if (CPC_IN_PCC(enable_reg)) {
+
+ if (pcc_ss_id < 0)
+ return -EIO;
+
+ ret = cpc_write(cpu, enable_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 platfrom */
+ ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
+ up_write(&pcc_ss_data->pcc_lock);
+ }
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(cppc_set_enable);
+
/**
* cppc_set_perf - Set a CPU's performance controls.
* @cpu: CPU for which to set performance controls.
diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
index 9f4985b4d64d..3fdae40a75fc 100644
--- a/include/acpi/cppc_acpi.h
+++ b/include/acpi/cppc_acpi.h
@@ -137,6 +137,7 @@ struct cppc_cpudata {
extern int cppc_get_desired_perf(int cpunum, u64 *desired_perf);
extern int cppc_get_perf_ctrs(int cpu, struct cppc_perf_fb_ctrs *perf_fb_ctrs);
extern int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls);
+extern int cppc_set_enable(int cpu, u32 enable);
extern int cppc_get_perf_caps(int cpu, struct cppc_perf_caps *caps);
extern bool acpi_cpc_valid(void);
extern int acpi_get_psd_map(unsigned int cpu, struct cppc_cpudata *cpu_data);
@@ -157,6 +158,10 @@ static inline int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
{
return -ENOTSUPP;
}
+static inline int cppc_set_enable(int cpu, u32 enable)
+{
+ return -ENOTSUPP;
+}
static inline int cppc_get_perf_caps(int cpu, struct cppc_perf_caps *caps)
{
return -ENOTSUPP;
--
2.25.1
On 9/26/2021 4:05 AM, Huang Rui wrote:
> From: Jinzhou Su <[email protected]>
>
> Add a new function to enable CPPC feature. This function
> will write Continuous Performance Control package
> EnableRegister field on the processor.
>
> Signed-off-by: Jinzhou Su <[email protected]>
> Signed-off-by: Huang Rui <[email protected]>
> ---
> drivers/acpi/cppc_acpi.c | 48 ++++++++++++++++++++++++++++++++++++++++
> include/acpi/cppc_acpi.h | 5 +++++
> 2 files changed, 53 insertions(+)
>
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index 2efe2ba97d96..b285960c35e7 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -1220,6 +1220,54 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
> }
> EXPORT_SYMBOL_GPL(cppc_get_perf_ctrs);
>
> +/**
> + * cppc_set_enable - Set to enable CPPC on the processor by writing the
> + * Continuous Performance Control package EnableRegister feild.
> + * @cpu: CPU for which to enable CPPC register.
> + * @enable: 0 - disable, 1 - enable CPPC feature on the processor.
> + *
> + * Return: 0 for success, -ERRNO or -EIO otherwise.
> + */
> +int cppc_set_enable(int cpu, u32 enable)
This should take a bool instead of u32 for enable, you can only enable
or diable cppc. The only caller I see is in patch 7/21 in which the
enable arg is already a bool that's converted to a u32. This also allows
for the removal of the enable value check.
You should consider merging this patch with patch 7/21. This patch adds
the cppc_set_enable() routine but has no users. The only caller I find is
in patch 7/21.
-Nathan
> +{
> + int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
> + struct cpc_register_resource *enable_reg;
> + struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
> + struct cppc_pcc_data *pcc_ss_data = NULL;
> + int ret = -1;
> +
> + /* check the input value*/
> + if (cpu < 0 || cpu > num_possible_cpus() - 1 || enable > 1)
> + return -ENODEV;
> +
> + if (!cpc_desc) {
> + pr_debug("No CPC descriptor for CPU:%d\n", cpu);
> + return -ENODEV;
> + }
> +
> + enable_reg = &cpc_desc->cpc_regs[ENABLE];
> +
> + if (CPC_IN_PCC(enable_reg)) {
> +
> + if (pcc_ss_id < 0)
> + return -EIO;
> +
> + ret = cpc_write(cpu, enable_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 platfrom */
> + ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
> + up_write(&pcc_ss_data->pcc_lock);
> + }
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(cppc_set_enable);
> +
> /**
> * cppc_set_perf - Set a CPU's performance controls.
> * @cpu: CPU for which to set performance controls.
> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
> index 9f4985b4d64d..3fdae40a75fc 100644
> --- a/include/acpi/cppc_acpi.h
> +++ b/include/acpi/cppc_acpi.h
> @@ -137,6 +137,7 @@ struct cppc_cpudata {
> extern int cppc_get_desired_perf(int cpunum, u64 *desired_perf);
> extern int cppc_get_perf_ctrs(int cpu, struct cppc_perf_fb_ctrs *perf_fb_ctrs);
> extern int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls);
> +extern int cppc_set_enable(int cpu, u32 enable);
> extern int cppc_get_perf_caps(int cpu, struct cppc_perf_caps *caps);
> extern bool acpi_cpc_valid(void);
> extern int acpi_get_psd_map(unsigned int cpu, struct cppc_cpudata *cpu_data);
> @@ -157,6 +158,10 @@ static inline int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
> {
> return -ENOTSUPP;
> }
> +static inline int cppc_set_enable(int cpu, u32 enable)
> +{
> + return -ENOTSUPP;
> +}
> static inline int cppc_get_perf_caps(int cpu, struct cppc_perf_caps *caps)
> {
> return -ENOTSUPP;
> --
> 2.25.1
>
[AMD Official Use Only]
Hi all,
Sorry to late response, I am just back from vacation.
> -----Original Message-----
> From: Fontenot, Nathan <[email protected]>
> Sent: Wednesday, September 29, 2021 1:06 AM
> To: Huang, Ray <[email protected]>; Rafael J . Wysocki
> <[email protected]>; Viresh Kumar <[email protected]>;
> Shuah Khan <[email protected]>; Borislav Petkov <[email protected]>;
> Peter Zijlstra <[email protected]>; Ingo Molnar <[email protected]>;
> [email protected]
> Cc: Sharma, Deepak <[email protected]>; Deucher, Alexander
> <[email protected]>; Limonciello, Mario
> <[email protected]>; Su, Jinzhou (Joe) <[email protected]>;
> Du, Xiaojian <[email protected]>; [email protected];
> [email protected]
> Subject: Re: [PATCH v2 04/21] ACPI: CPPC: add cppc enable register function
>
> On 9/26/2021 4:05 AM, Huang Rui wrote:
> > From: Jinzhou Su <[email protected]>
> >
> > Add a new function to enable CPPC feature. This function
> > will write Continuous Performance Control package
> > EnableRegister field on the processor.
> >
> > Signed-off-by: Jinzhou Su <[email protected]>
> > Signed-off-by: Huang Rui <[email protected]>
> > ---
> > drivers/acpi/cppc_acpi.c | 48
> ++++++++++++++++++++++++++++++++++++++++
> > include/acpi/cppc_acpi.h | 5 +++++
> > 2 files changed, 53 insertions(+)
> >
> > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> > index 2efe2ba97d96..b285960c35e7 100644
> > --- a/drivers/acpi/cppc_acpi.c
> > +++ b/drivers/acpi/cppc_acpi.c
> > @@ -1220,6 +1220,54 @@ int cppc_get_perf_ctrs(int cpunum, struct
> cppc_perf_fb_ctrs *perf_fb_ctrs)
> > }
> > EXPORT_SYMBOL_GPL(cppc_get_perf_ctrs);
> >
> > +/**
> > + * cppc_set_enable - Set to enable CPPC on the processor by writing the
> > + * Continuous Performance Control package EnableRegister feild.
> > + * @cpu: CPU for which to enable CPPC register.
> > + * @enable: 0 - disable, 1 - enable CPPC feature on the processor.
> > + *
> > + * Return: 0 for success, -ERRNO or -EIO otherwise.
> > + */
> > +int cppc_set_enable(int cpu, u32 enable)
>
> This should take a bool instead of u32 for enable, you can only enable
> or diable cppc. The only caller I see is in patch 7/21 in which the
> enable arg is already a bool that's converted to a u32. This also allows
> for the removal of the enable value check.
>
Yes, the bool type should be better than u32 here. Will update it in V3.
> You should consider merging this patch with patch 7/21. This patch adds
> the cppc_set_enable() routine but has no users. The only caller I find is
> in patch 7/21.
>
I am looking back again, this patch is to provide the new helper in cppc_acpi library under ACPI subsystem.
However, patch 7 is to enable the shared memory APIs in amd-pstate driver.
They are actually the different function implementations. I prefer using the separated patch here.
Thanks,
Ray
On Sun, Sep 26, 2021 at 11:06 AM Huang Rui <[email protected]> wrote:
>
> From: Jinzhou Su <[email protected]>
>
> Add a new function to enable CPPC feature. This function
> will write Continuous Performance Control package
> EnableRegister field on the processor.
And what is going to take place after this write?
Also, it would be good to mention that the user of this function will
be added subsequently.
> Signed-off-by: Jinzhou Su <[email protected]>
> Signed-off-by: Huang Rui <[email protected]>
> ---
> drivers/acpi/cppc_acpi.c | 48 ++++++++++++++++++++++++++++++++++++++++
> include/acpi/cppc_acpi.h | 5 +++++
> 2 files changed, 53 insertions(+)
>
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index 2efe2ba97d96..b285960c35e7 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -1220,6 +1220,54 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
> }
> EXPORT_SYMBOL_GPL(cppc_get_perf_ctrs);
>
> +/**
> + * cppc_set_enable - Set to enable CPPC on the processor by writing the
> + * Continuous Performance Control package EnableRegister feild.
> + * @cpu: CPU for which to enable CPPC register.
> + * @enable: 0 - disable, 1 - enable CPPC feature on the processor.
> + *
> + * Return: 0 for success, -ERRNO or -EIO otherwise.
> + */
> +int cppc_set_enable(int cpu, u32 enable)
> +{
> + int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
> + struct cpc_register_resource *enable_reg;
> + struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
> + struct cppc_pcc_data *pcc_ss_data = NULL;
> + int ret = -1;
> +
> + /* check the input value*/
> + if (cpu < 0 || cpu > num_possible_cpus() - 1 || enable > 1)
Why not use cpu_possible()? And why enable > 1 is a problem?
> + return -ENODEV;
-EINVAL
> +
> + if (!cpc_desc) {
if this is checked, the cpu_possible() check above is redundant.
> + pr_debug("No CPC descriptor for CPU:%d\n", cpu);
> + return -ENODEV;
> + }
> +
> + enable_reg = &cpc_desc->cpc_regs[ENABLE];
> +
> + if (CPC_IN_PCC(enable_reg)) {
> +
> + if (pcc_ss_id < 0)
> + return -EIO;
> +
> + ret = cpc_write(cpu, enable_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 platfrom */
> + ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
> + up_write(&pcc_ss_data->pcc_lock);
> + }
Does it really need to do nothing if the register is not in PCC? If
so, then why?
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(cppc_set_enable);
> +
> /**
> * cppc_set_perf - Set a CPU's performance controls.
> * @cpu: CPU for which to set performance controls.
> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
> index 9f4985b4d64d..3fdae40a75fc 100644
> --- a/include/acpi/cppc_acpi.h
> +++ b/include/acpi/cppc_acpi.h
> @@ -137,6 +137,7 @@ struct cppc_cpudata {
> extern int cppc_get_desired_perf(int cpunum, u64 *desired_perf);
> extern int cppc_get_perf_ctrs(int cpu, struct cppc_perf_fb_ctrs *perf_fb_ctrs);
> extern int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls);
> +extern int cppc_set_enable(int cpu, u32 enable);
> extern int cppc_get_perf_caps(int cpu, struct cppc_perf_caps *caps);
> extern bool acpi_cpc_valid(void);
> extern int acpi_get_psd_map(unsigned int cpu, struct cppc_cpudata *cpu_data);
> @@ -157,6 +158,10 @@ static inline int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
> {
> return -ENOTSUPP;
> }
> +static inline int cppc_set_enable(int cpu, u32 enable)
> +{
> + return -ENOTSUPP;
> +}
> static inline int cppc_get_perf_caps(int cpu, struct cppc_perf_caps *caps)
> {
> return -ENOTSUPP;
> --
> 2.25.1
>
[AMD Official Use Only]
> -----Original Message-----
> From: Rafael J. Wysocki <[email protected]>
> Sent: Wednesday, October 20, 2021 1:00 AM
> To: Huang, Ray <[email protected]>
> Cc: Rafael J . Wysocki <[email protected]>; Viresh Kumar
> <[email protected]>; Shuah Khan <[email protected]>;
> Borislav Petkov <[email protected]>; Peter Zijlstra <[email protected]>; Ingo
> Molnar <[email protected]>; Linux PM <[email protected]>;
> Sharma, Deepak <[email protected]>; Deucher, Alexander
> <[email protected]>; Limonciello, Mario
> <[email protected]>; Fontenot, Nathan
> <[email protected]>; Su, Jinzhou (Joe) <[email protected]>;
> Du, Xiaojian <[email protected]>; Linux Kernel Mailing List <linux-
> [email protected]>; the arch/x86 maintainers <[email protected]>
> Subject: Re: [PATCH v2 04/21] ACPI: CPPC: add cppc enable register function
>
> On Sun, Sep 26, 2021 at 11:06 AM Huang Rui <[email protected]> wrote:
> >
> > From: Jinzhou Su <[email protected]>
> >
> > Add a new function to enable CPPC feature. This function will write
> > Continuous Performance Control package EnableRegister field on the
> > processor.
>
> And what is going to take place after this write?
>
> Also, it would be good to mention that the user of this function will be added
> subsequently.
After the enable flag is set, the processor hardware can accept the performance goals such as desired perf that programed by kernel and control the processor frequency according to the performance value.
I will mention this in the comment in V3.
>
> > Signed-off-by: Jinzhou Su <[email protected]>
> > Signed-off-by: Huang Rui <[email protected]>
> > ---
> > drivers/acpi/cppc_acpi.c | 48
> > ++++++++++++++++++++++++++++++++++++++++
> > include/acpi/cppc_acpi.h | 5 +++++
> > 2 files changed, 53 insertions(+)
> >
> > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c index
> > 2efe2ba97d96..b285960c35e7 100644
> > --- a/drivers/acpi/cppc_acpi.c
> > +++ b/drivers/acpi/cppc_acpi.c
> > @@ -1220,6 +1220,54 @@ int cppc_get_perf_ctrs(int cpunum, struct
> > cppc_perf_fb_ctrs *perf_fb_ctrs) }
> > EXPORT_SYMBOL_GPL(cppc_get_perf_ctrs);
> >
> > +/**
> > + * cppc_set_enable - Set to enable CPPC on the processor by writing
> > +the
> > + * Continuous Performance Control package EnableRegister feild.
> > + * @cpu: CPU for which to enable CPPC register.
> > + * @enable: 0 - disable, 1 - enable CPPC feature on the processor.
> > + *
> > + * Return: 0 for success, -ERRNO or -EIO otherwise.
> > + */
> > +int cppc_set_enable(int cpu, u32 enable) {
> > + int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
> > + struct cpc_register_resource *enable_reg;
> > + struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
> > + struct cppc_pcc_data *pcc_ss_data = NULL;
> > + int ret = -1;
> > +
> > + /* check the input value*/
> > + if (cpu < 0 || cpu > num_possible_cpus() - 1 || enable > 1)
>
> Why not use cpu_possible()? And why enable > 1 is a problem?
>
Yes, you're right, cpu_possible() is better here.
Will remove "enable > 1", and yes, we should support "disable" as well.
> > + return -ENODEV;
>
> -EINVAL
>
Updated.
> > +
> > + if (!cpc_desc) {
>
> if this is checked, the cpu_possible() check above is redundant.
Hmm, if acpi_cppc_processor_probe got failed, some one outside acpi driver would like to call this helper.
Is that possible we get a null cpc descriptor here? Or anything I missed.
>
> > + pr_debug("No CPC descriptor for CPU:%d\n", cpu);
> > + return -ENODEV;
> > + }
> > +
> > + enable_reg = &cpc_desc->cpc_regs[ENABLE];
> > +
> > + if (CPC_IN_PCC(enable_reg)) {
> > +
> > + if (pcc_ss_id < 0)
> > + return -EIO;
> > +
> > + ret = cpc_write(cpu, enable_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 platfrom */
> > + ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
> > + up_write(&pcc_ss_data->pcc_lock);
> > + }
>
> Does it really need to do nothing if the register is not in PCC? If so, then why?
>
Hmm, do you mean we should take care the cases for enabling behavior if register in other spaces such as SYSTEM_MEMORY or FIXED_HARDWARE on different kinds of SBIOS implementation?
Thanks,
Ray
On Wed, Oct 20, 2021 at 1:13 PM Huang, Ray <[email protected]> wrote:
>
> [AMD Official Use Only]
>
> > -----Original Message-----
> > From: Rafael J. Wysocki <[email protected]>
> > Sent: Wednesday, October 20, 2021 1:00 AM
> > To: Huang, Ray <[email protected]>
> > Cc: Rafael J . Wysocki <[email protected]>; Viresh Kumar
> > <[email protected]>; Shuah Khan <[email protected]>;
> > Borislav Petkov <[email protected]>; Peter Zijlstra <[email protected]>; Ingo
> > Molnar <[email protected]>; Linux PM <[email protected]>;
> > Sharma, Deepak <[email protected]>; Deucher, Alexander
> > <[email protected]>; Limonciello, Mario
> > <[email protected]>; Fontenot, Nathan
> > <[email protected]>; Su, Jinzhou (Joe) <[email protected]>;
> > Du, Xiaojian <[email protected]>; Linux Kernel Mailing List <linux-
> > [email protected]>; the arch/x86 maintainers <[email protected]>
> > Subject: Re: [PATCH v2 04/21] ACPI: CPPC: add cppc enable register function
> >
> > On Sun, Sep 26, 2021 at 11:06 AM Huang Rui <[email protected]> wrote:
> > >
> > > From: Jinzhou Su <[email protected]>
> > >
> > > Add a new function to enable CPPC feature. This function will write
> > > Continuous Performance Control package EnableRegister field on the
> > > processor.
> >
> > And what is going to take place after this write?
> >
> > Also, it would be good to mention that the user of this function will be added
> > subsequently.
>
> After the enable flag is set, the processor hardware can accept the performance goals such as desired perf that programed by kernel and control the processor frequency according to the performance value.
Is this the CPPC EnableRegister register described in Section 8.4.7.1
of ACPI 6.4? If so, it would be good to provide this information in
the changelog either.
> I will mention this in the comment in V3.
>
> >
> > > Signed-off-by: Jinzhou Su <[email protected]>
> > > Signed-off-by: Huang Rui <[email protected]>
> > > ---
> > > drivers/acpi/cppc_acpi.c | 48
> > > ++++++++++++++++++++++++++++++++++++++++
> > > include/acpi/cppc_acpi.h | 5 +++++
> > > 2 files changed, 53 insertions(+)
> > >
> > > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c index
> > > 2efe2ba97d96..b285960c35e7 100644
> > > --- a/drivers/acpi/cppc_acpi.c
> > > +++ b/drivers/acpi/cppc_acpi.c
> > > @@ -1220,6 +1220,54 @@ int cppc_get_perf_ctrs(int cpunum, struct
> > > cppc_perf_fb_ctrs *perf_fb_ctrs) }
> > > EXPORT_SYMBOL_GPL(cppc_get_perf_ctrs);
> > >
> > > +/**
> > > + * cppc_set_enable - Set to enable CPPC on the processor by writing
> > > +the
> > > + * Continuous Performance Control package EnableRegister feild.
> > > + * @cpu: CPU for which to enable CPPC register.
> > > + * @enable: 0 - disable, 1 - enable CPPC feature on the processor.
> > > + *
> > > + * Return: 0 for success, -ERRNO or -EIO otherwise.
> > > + */
> > > +int cppc_set_enable(int cpu, u32 enable) {
> > > + int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
> > > + struct cpc_register_resource *enable_reg;
> > > + struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
> > > + struct cppc_pcc_data *pcc_ss_data = NULL;
> > > + int ret = -1;
> > > +
> > > + /* check the input value*/
> > > + if (cpu < 0 || cpu > num_possible_cpus() - 1 || enable > 1)
> >
> > Why not use cpu_possible()? And why enable > 1 is a problem?
> >
>
> Yes, you're right, cpu_possible() is better here.
> Will remove "enable > 1", and yes, we should support "disable" as well.
>
>
> > > + return -ENODEV;
> >
> > -EINVAL
> >
>
> Updated.
>
> > > +
> > > + if (!cpc_desc) {
> >
> > if this is checked, the cpu_possible() check above is redundant.
>
> Hmm, if acpi_cppc_processor_probe got failed, some one outside acpi driver would like to call this helper.
> Is that possible we get a null cpc descriptor here? Or anything I missed.
if cpu_possible(cpu) is false, then cpc_desc for cpu will be NULL. If
you check the latter, there's no need to check the former. Of course,
cpc_desc may be NULL for other reasons, but you're checking it anyway.
> >
> > > + pr_debug("No CPC descriptor for CPU:%d\n", cpu);
> > > + return -ENODEV;
> > > + }
> > > +
> > > + enable_reg = &cpc_desc->cpc_regs[ENABLE];
> > > +
> > > + if (CPC_IN_PCC(enable_reg)) {
> > > +
> > > + if (pcc_ss_id < 0)
> > > + return -EIO;
> > > +
> > > + ret = cpc_write(cpu, enable_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 platfrom */
> > > + ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
> > > + up_write(&pcc_ss_data->pcc_lock);
> > > + }
> >
> > Does it really need to do nothing if the register is not in PCC? If so, then why?
> >
>
> Hmm, do you mean we should take care the cases for enabling behavior if register in other spaces such as SYSTEM_MEMORY or FIXED_HARDWARE on different kinds of SBIOS implementation?
This is a generic interface and it should cover all of the valid use
cases, so yes.
[AMD Official Use Only]
> -----Original Message-----
> From: Rafael J. Wysocki <[email protected]>
> Sent: Wednesday, October 20, 2021 9:32 PM
> To: Huang, Ray <[email protected]>
> Cc: Rafael J. Wysocki <[email protected]>; Rafael J . Wysocki
> <[email protected]>; Viresh Kumar <[email protected]>;
> Shuah Khan <[email protected]>; Borislav Petkov <[email protected]>;
> Peter Zijlstra <[email protected]>; Ingo Molnar <[email protected]>;
> Linux PM <[email protected]>; Sharma, Deepak
> <[email protected]>; Deucher, Alexander
> <[email protected]>; Limonciello, Mario
> <[email protected]>; Fontenot, Nathan
> <[email protected]>; Su, Jinzhou (Joe) <[email protected]>;
> Du, Xiaojian <[email protected]>; Linux Kernel Mailing List <linux-
> [email protected]>; the arch/x86 maintainers <[email protected]>
> Subject: Re: [PATCH v2 04/21] ACPI: CPPC: add cppc enable register function
>
> On Wed, Oct 20, 2021 at 1:13 PM Huang, Ray <[email protected]>
> wrote:
> >
> > [AMD Official Use Only]
> >
> > > -----Original Message-----
> > > From: Rafael J. Wysocki <[email protected]>
> > > Sent: Wednesday, October 20, 2021 1:00 AM
> > > To: Huang, Ray <[email protected]>
> > > Cc: Rafael J . Wysocki <[email protected]>; Viresh Kumar
> > > <[email protected]>; Shuah Khan <[email protected]>;
> > > Borislav Petkov <[email protected]>; Peter Zijlstra <[email protected]>;
> > > Ingo Molnar <[email protected]>; Linux PM <[email protected]>;
> > > Sharma, Deepak <[email protected]>; Deucher, Alexander
> > > <[email protected]>; Limonciello, Mario
> > > <[email protected]>; Fontenot, Nathan
> > > <[email protected]>; Su, Jinzhou (Joe)
> <[email protected]>;
> > > Du, Xiaojian <[email protected]>; Linux Kernel Mailing List
> > > <linux- [email protected]>; the arch/x86 maintainers
> > > <[email protected]>
> > > Subject: Re: [PATCH v2 04/21] ACPI: CPPC: add cppc enable register
> > > function
> > >
> > > On Sun, Sep 26, 2021 at 11:06 AM Huang Rui <[email protected]>
> wrote:
> > > >
> > > > From: Jinzhou Su <[email protected]>
> > > >
> > > > Add a new function to enable CPPC feature. This function will
> > > > write Continuous Performance Control package EnableRegister field
> > > > on the processor.
> > >
> > > And what is going to take place after this write?
> > >
> > > Also, it would be good to mention that the user of this function
> > > will be added subsequently.
> >
> > After the enable flag is set, the processor hardware can accept the
> performance goals such as desired perf that programed by kernel and control
> the processor frequency according to the performance value.
>
> Is this the CPPC EnableRegister register described in Section 8.4.7.1 of ACPI
> 6.4? If so, it would be good to provide this information in the changelog
> either.
>
I see, yes. We should follow the spec definition for general CPPC function helper.
> > I will mention this in the comment in V3.
> >
> > >
> > > > Signed-off-by: Jinzhou Su <[email protected]>
> > > > Signed-off-by: Huang Rui <[email protected]>
> > > > ---
> > > > drivers/acpi/cppc_acpi.c | 48
> > > > ++++++++++++++++++++++++++++++++++++++++
> > > > include/acpi/cppc_acpi.h | 5 +++++
> > > > 2 files changed, 53 insertions(+)
> > > >
> > > > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> > > > index
> > > > 2efe2ba97d96..b285960c35e7 100644
> > > > --- a/drivers/acpi/cppc_acpi.c
> > > > +++ b/drivers/acpi/cppc_acpi.c
> > > > @@ -1220,6 +1220,54 @@ int cppc_get_perf_ctrs(int cpunum, struct
> > > > cppc_perf_fb_ctrs *perf_fb_ctrs) }
> > > > EXPORT_SYMBOL_GPL(cppc_get_perf_ctrs);
> > > >
> > > > +/**
> > > > + * cppc_set_enable - Set to enable CPPC on the processor by
> > > > +writing the
> > > > + * Continuous Performance Control package EnableRegister feild.
> > > > + * @cpu: CPU for which to enable CPPC register.
> > > > + * @enable: 0 - disable, 1 - enable CPPC feature on the processor.
> > > > + *
> > > > + * Return: 0 for success, -ERRNO or -EIO otherwise.
> > > > + */
> > > > +int cppc_set_enable(int cpu, u32 enable) {
> > > > + int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
> > > > + struct cpc_register_resource *enable_reg;
> > > > + struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
> > > > + struct cppc_pcc_data *pcc_ss_data = NULL;
> > > > + int ret = -1;
> > > > +
> > > > + /* check the input value*/
> > > > + if (cpu < 0 || cpu > num_possible_cpus() - 1 || enable >
> > > > + 1)
> > >
> > > Why not use cpu_possible()? And why enable > 1 is a problem?
> > >
> >
> > Yes, you're right, cpu_possible() is better here.
> > Will remove "enable > 1", and yes, we should support "disable" as well.
> >
> >
> > > > + return -ENODEV;
> > >
> > > -EINVAL
> > >
> >
> > Updated.
> >
> > > > +
> > > > + if (!cpc_desc) {
> > >
> > > if this is checked, the cpu_possible() check above is redundant.
> >
> > Hmm, if acpi_cppc_processor_probe got failed, some one outside acpi
> driver would like to call this helper.
> > Is that possible we get a null cpc descriptor here? Or anything I missed.
>
> if cpu_possible(cpu) is false, then cpc_desc for cpu will be NULL. If you check
> the latter, there's no need to check the former. Of course, cpc_desc may be
> NULL for other reasons, but you're checking it anyway.
>
Yes. If the cpc_desc is initialized, the cpu has to be in possible mask. I will clean it up in V3.
> > >
> > > > + pr_debug("No CPC descriptor for CPU:%d\n", cpu);
> > > > + return -ENODEV;
> > > > + }
> > > > +
> > > > + enable_reg = &cpc_desc->cpc_regs[ENABLE];
> > > > +
> > > > + if (CPC_IN_PCC(enable_reg)) {
> > > > +
> > > > + if (pcc_ss_id < 0)
> > > > + return -EIO;
> > > > +
> > > > + ret = cpc_write(cpu, enable_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
> platfrom */
> > > > + ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
> > > > + up_write(&pcc_ss_data->pcc_lock);
> > > > + }
> > >
> > > Does it really need to do nothing if the register is not in PCC? If so, then
> why?
> > >
> >
> > Hmm, do you mean we should take care the cases for enabling behavior if
> register in other spaces such as SYSTEM_MEMORY or FIXED_HARDWARE on
> different kinds of SBIOS implementation?
>
> This is a generic interface and it should cover all of the valid use cases, so yes.
I got it, thanks to reminder!
Thanks,
Ray