Re: [PATCH 2/2] cpufreq: arm_big_little: use generic OPP functions for {init,free}_opp_table

[Viresh Kumar]

On 28-04-16, 11:25, Sudeep Holla wrote:
> Currently when performing random hotplugs and suspend-to-ram(S2R) on
> systems using arm_big_little cpufreq driver, we get warnings similar to:
> 
> cpu cpu1: _opp_add: duplicate OPPs detected. Existing: freq: 600000000,
> 	volt: 800000, enabled: 1. New: freq: 600000000, volt: 800000, enabled: 1
> 
> This is mainly because the OPPs for the shared cpus are not set. We can
> just use dev_pm_opp_of_cpumask_add_table in case the OPPs are obtained
> from DT(arm_big_little_dt.c) or use dev_pm_opp_set_sharing_cpus if the
> OPPs are obtained by other means like firmware(e.g. scpi-cpufreq.c)
> 
> Also now that the generic dev_pm_opp_cpumask_remove_table can handle
> removal of opp table and entries for all associated CPUs, we can reuse
> dev_pm_opp_cpumask_remove_table as free_opp_table in cpufreq_arm_bL_ops.
> 
> This patch makes necessary changes to reuse the generic OPP functions for
> {init,free}_opp_table and thereby eliminating the warnings.
> 
> Cc: Viresh Kumar <vireshk@xxxxxxxxxx>
> Cc: "Rafael J. Wysocki" <rjw@xxxxxxxxxxxxx>
> Cc: linux-pm@xxxxxxxxxxxxxxx
> Signed-off-by: Sudeep Holla <sudeep.holla@xxxxxxx>
> ---
>  drivers/cpufreq/arm_big_little.c    | 54 ++++++++++++++++++++-----------------
>  drivers/cpufreq/arm_big_little.h    |  4 +--
>  drivers/cpufreq/arm_big_little_dt.c | 21 ++-------------
>  drivers/cpufreq/scpi-cpufreq.c      | 24 ++++++++++++++---
>  4 files changed, 54 insertions(+), 49 deletions(-)
> 
> Hi Viresh,
> 
> Why is dynamic OPPs not deleted in dev_pm_opp_{,cpumask_}remove_table ?
> It would remove some code in scpi-cpufreq.c but I would like to understand.
> Is there any use in not deleting them there ?

That was intentional, consider this case:
- OPPs are added dynamically from platform code
- insmod cpufreq-dt.ko (add static (dt) OPPs as well, mostly fail)
- rmmod  cpufreq-dt.ko (remove all OPPs)

- insmod cpufreq-dt.ko (no more OPPs available, as we removed them both).

That's bad ?

Now, how to fix platforms which don't add dynamic OPPs from platform code? But
something like the scpi driver, which can get inserted/removed again.

This is what I would suggest:
- Even if dev_pm_opp_of_cpumask_remove_table() isn't doing any OF specific
  stuff, lets not update its name and move it out of the ifdef, as it will be
  used only if the user has created OPPs using DT earlier.
- But rather make those two routines call two new routines in the core (which
  don't depend on OF) and implement what these of-remove routines do.
- Add two more routines for removing OPPs created dynamically and call the two
  routines created in previous step.

I hope you followed that :)

> -static void scpi_free_opp_table(struct device *cpu_dev)
> +static void scpi_free_opp_table(cpumask_var_t cpumask)
>  {
> +	struct device *cpu_dev = get_cpu_device(cpumask_first(cpumask));
> +
> +	cpumask_clear_cpu(cpu_dev->id, cpumask);
> +	dev_pm_opp_cpumask_remove_table(cpumask);
> +	cpumask_set_cpu(cpu_dev->id, cpumask);
> +
>  	scpi_opp_table_ops(cpu_dev, true);
>  }

This was a *really* crazy idea :)

And btw, I think you better move to cpufreq-dt, instead of struggling with this
one :)

-- 
viresh