Messages in this thread |  | | From | "Huang, Ray" <> | Subject | RE: [PATCH v2 04/21] ACPI: CPPC: add cppc enable register function | Date | Thu, 21 Oct 2021 03:41:28 +0000 |
| |
[AMD Official Use Only]
> -----Original Message----- > From: Rafael J. Wysocki <rafael@kernel.org> > Sent: Wednesday, October 20, 2021 9:32 PM > To: Huang, Ray <Ray.Huang@amd.com> > Cc: Rafael J. Wysocki <rafael@kernel.org>; Rafael J . Wysocki > <rafael.j.wysocki@intel.com>; Viresh Kumar <viresh.kumar@linaro.org>; > Shuah Khan <skhan@linuxfoundation.org>; Borislav Petkov <bp@suse.de>; > Peter Zijlstra <peterz@infradead.org>; Ingo Molnar <mingo@kernel.org>; > Linux PM <linux-pm@vger.kernel.org>; Sharma, Deepak > <Deepak.Sharma@amd.com>; Deucher, Alexander > <Alexander.Deucher@amd.com>; Limonciello, Mario > <Mario.Limonciello@amd.com>; Fontenot, Nathan > <Nathan.Fontenot@amd.com>; Su, Jinzhou (Joe) <Jinzhou.Su@amd.com>; > Du, Xiaojian <Xiaojian.Du@amd.com>; Linux Kernel Mailing List <linux- > kernel@vger.kernel.org>; the arch/x86 maintainers <x86@kernel.org> > 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@amd.com> > wrote: > > > > [AMD Official Use Only] > > > > > -----Original Message----- > > > From: Rafael J. Wysocki <rafael@kernel.org> > > > Sent: Wednesday, October 20, 2021 1:00 AM > > > To: Huang, Ray <Ray.Huang@amd.com> > > > Cc: Rafael J . Wysocki <rafael.j.wysocki@intel.com>; Viresh Kumar > > > <viresh.kumar@linaro.org>; Shuah Khan <skhan@linuxfoundation.org>; > > > Borislav Petkov <bp@suse.de>; Peter Zijlstra <peterz@infradead.org>; > > > Ingo Molnar <mingo@kernel.org>; Linux PM <linux-pm@vger.kernel.org>; > > > Sharma, Deepak <Deepak.Sharma@amd.com>; Deucher, Alexander > > > <Alexander.Deucher@amd.com>; Limonciello, Mario > > > <Mario.Limonciello@amd.com>; Fontenot, Nathan > > > <Nathan.Fontenot@amd.com>; Su, Jinzhou (Joe) > <Jinzhou.Su@amd.com>; > > > Du, Xiaojian <Xiaojian.Du@amd.com>; Linux Kernel Mailing List > > > <linux- kernel@vger.kernel.org>; the arch/x86 maintainers > > > <x86@kernel.org> > > > 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@amd.com> > wrote: > > > > > > > > From: Jinzhou Su <Jinzhou.Su@amd.com> > > > > > > > > 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@amd.com> > > > > Signed-off-by: Huang Rui <ray.huang@amd.com> > > > > --- > > > > 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 |  |