On Sunday, September 27, 2015 01:15:20 PM Ashwin Chaugule wrote:
> Hi Rafael,
>
>
> On 25 September 2015 at 20:05, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote:
> > On Wednesday, September 09, 2015 04:27:04 PM Ashwin Chaugule wrote:
> >> CPPC stands for Collaborative Processor Performance Controls
> >> and is defined in the ACPI v5.0+ spec. It describes CPU
> >> performance controls on an abstract and continuous scale
> >> allowing the platform (e.g. remote power processor) to flexibly
> >> optimize CPU performance with its knowledge of power budgets
> >> and other architecture specific knowledge.
> >>
> >> This patch adds a shim which exports commonly used functions
> >> to get and set CPPC specific controls for each CPU. This enables
> >> CPUFreq drivers to gather per CPU performance data and use
> >> with exisiting governors or even allows for customized governors
> >> which are implemented inside CPUFreq drivers.
> >>
> >> Signed-off-by: Ashwin Chaugule <ashwin.chaugule@xxxxxxxxxx>
> >> Reviewed-by: Al Stone <al.stone@xxxxxxxxxx>
> >> ---
> >
> > [cut]
> >
> >> +
> >> +/**
> >> + * acpi_get_psd_map - Map the CPUs in a common freq domain as indicated
> >> + * in the respective _PSD entries.
> >
> > One minor nit: this needs to be one-line in a proper kerneldoc.
> >
> >> + * @all_cpu_data: This array of pointers contains all the CPU specific
> >> + * details to access CPPC registers and freq domain information for each
> >> + * CPU.
> >
> > Ditto.
> >
> > You can add more description below.
>
> Ok. Will do.
>
> >
> > [cut]
> >
> >> +int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
> >> +{
> >> + struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
> >> + struct cpc_register_resource *delivered_reg, *reference_reg;
> >> + u64 delivered, reference;
> >> + int ret = 0;
> >> +
> >> + if (!cpc_desc) {
> >> + pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
> >> + return -ENODEV;
> >> + }
> >> +
> >> + delivered_reg = &cpc_desc->cpc_regs[DELIVERED_CTR];
> >> + reference_reg = &cpc_desc->cpc_regs[REFERENCE_CTR];
> >> +
> >> + spin_lock(&pcc_lock);
> >
> > I still have the question about this lock.
> >
> > Why does it have to be a spinlock?
>
> The idea is to let CPUs send in their requests as quickly as possible.
> The PCC transaction only relays the request. So the CPUs don't really
> wait for all the clk + voltage toggling etc. I think any other
> primitive, might cause unnecessary delays in sending requests.
It might if there was contention.
> Also FWIW, this is another thing I was influenced by from the pcc-cpufreq.c
> driver. :)
OK, so this sounds like an optimization, which is fine.
Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
