RE: [PATCH v2 04/21] ACPI: CPPC: add cppc enable register function

[Huang, Ray]

[AMD Official Use Only]

> -----Original Message-----
> From: Rafael J. Wysocki <rafael@xxxxxxxxxx>
> Sent: Wednesday, October 20, 2021 9:32 PM
> To: Huang, Ray <Ray.Huang@xxxxxxx>
> Cc: Rafael J. Wysocki <rafael@xxxxxxxxxx>; Rafael J . Wysocki
> <rafael.j.wysocki@xxxxxxxxx>; Viresh Kumar <viresh.kumar@xxxxxxxxxx>;
> Shuah Khan <skhan@xxxxxxxxxxxxxxxxxxx>; Borislav Petkov <bp@xxxxxxx>;
> Peter Zijlstra <peterz@xxxxxxxxxxxxx>; Ingo Molnar <mingo@xxxxxxxxxx>;
> Linux PM <linux-pm@xxxxxxxxxxxxxxx>; Sharma, Deepak
> <Deepak.Sharma@xxxxxxx>; Deucher, Alexander
> <Alexander.Deucher@xxxxxxx>; Limonciello, Mario
> <Mario.Limonciello@xxxxxxx>; Fontenot, Nathan
> <Nathan.Fontenot@xxxxxxx>; Su, Jinzhou (Joe) <Jinzhou.Su@xxxxxxx>;
> Du, Xiaojian <Xiaojian.Du@xxxxxxx>; Linux Kernel Mailing List <linux-
> kernel@xxxxxxxxxxxxxxx>; the arch/x86 maintainers <x86@xxxxxxxxxx>
> Subject: Re: [PATCH v2 04/21] ACPI: CPPC: add cppc enable register function
> 
> On Wed, Oct 20, 2021 at 1:13 PM Huang, Ray <Ray.Huang@xxxxxxx>
> wrote:
> >
> > [AMD Official Use Only]
> >
> > > -----Original Message-----
> > > From: Rafael J. Wysocki <rafael@xxxxxxxxxx>
> > > Sent: Wednesday, October 20, 2021 1:00 AM
> > > To: Huang, Ray <Ray.Huang@xxxxxxx>
> > > Cc: Rafael J . Wysocki <rafael.j.wysocki@xxxxxxxxx>; Viresh Kumar
> > > <viresh.kumar@xxxxxxxxxx>; Shuah Khan <skhan@xxxxxxxxxxxxxxxxxxx>;
> > > Borislav Petkov <bp@xxxxxxx>; Peter Zijlstra <peterz@xxxxxxxxxxxxx>;
> > > Ingo Molnar <mingo@xxxxxxxxxx>; Linux PM <linux-pm@xxxxxxxxxxxxxxx>;
> > > Sharma, Deepak <Deepak.Sharma@xxxxxxx>; Deucher, Alexander
> > > <Alexander.Deucher@xxxxxxx>; Limonciello, Mario
> > > <Mario.Limonciello@xxxxxxx>; Fontenot, Nathan
> > > <Nathan.Fontenot@xxxxxxx>; Su, Jinzhou (Joe)
> <Jinzhou.Su@xxxxxxx>;
> > > Du, Xiaojian <Xiaojian.Du@xxxxxxx>; Linux Kernel Mailing List
> > > <linux- kernel@xxxxxxxxxxxxxxx>; the arch/x86 maintainers
> > > <x86@xxxxxxxxxx>
> > > Subject: Re: [PATCH v2 04/21] ACPI: CPPC: add cppc enable register
> > > function
> > >
> > > On Sun, Sep 26, 2021 at 11:06 AM Huang Rui <ray.huang@xxxxxxx>
> wrote:
> > > >
> > > > From: Jinzhou Su <Jinzhou.Su@xxxxxxx>
> > > >
> > > > 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 <Jinzhou.Su@xxxxxxx>
> > > > Signed-off-by: Huang Rui <ray.huang@xxxxxxx>
> > > > ---
> > > >  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