| Message ID | ad55cd1a03e712e2c0c49701e87a37199cb8376e.1454992187.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Rafael Wysocki |
| Headers | show |
On 02/09, Viresh Kumar wrote: > OPP core can handle the regulators by itself, and but it needs to know > the name of the regulator to fetch. Add support for that. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
Hi Viresh, CC device-tree folks Replying to an old email, because that's the most accurate reference I could find. On Tue, Feb 9, 2016 at 6:06 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > OPP core can handle the regulators by itself, and but it needs to know > the name of the regulator to fetch. Add support for that. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/cpufreq/cpufreq-dt.c | 46 ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 46 insertions(+) > > diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c > index 4c9f8a828f6f..2af75f8088bb 100644 > --- a/drivers/cpufreq/cpufreq-dt.c > +++ b/drivers/cpufreq/cpufreq-dt.c > @@ -119,6 +120,30 @@ static int set_target(struct cpufreq_policy *policy, unsigned int index) > return ret; > } > > +/* > + * An earlier version of opp-v1 bindings used to name the regulator > + * "cpu0-supply", we still need to handle that for backwards compatibility. > + */ > +static const char *find_supply_name(struct device *dev, struct device_node *np) > +{ > + struct property *pp; > + int cpu = dev->id; > + > + /* Try "cpu0" for older DTs */ > + if (!cpu) { > + pp = of_find_property(np, "cpu0-supply", NULL); > + if (pp) > + return "cpu0"; > + } > + > + pp = of_find_property(np, "cpu-supply", NULL); > + if (pp) > + return "cpu"; Despite the existence of lots of users of these properties, I couldn't find both the "earlier version" and the "current version" of the opp-v1 bindings documenting the "cpu0-supply" and "cpu-supply" properties? Even for opp-v2, they are not documented in Documentation/devicetree/bindings/opp/opp.txt, but cpu-supply is used in the examples? For v2, I did find "[PATCH 01/16] PM / OPP: Add 'supply-names' binding" https://lore.kernel.org/lkml/2b87b162eabd1570ae2311e1ef8655acda72f678.1441972771.git.viresh.kumar@linaro.org/ but presumably that's an even further evolution? Can you please document these properties? Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 17-07-18, 09:46, Geert Uytterhoeven wrote: > Hi Viresh, > > CC device-tree folks > > Replying to an old email, because that's the most accurate reference I > could find. > > On Tue, Feb 9, 2016 at 6:06 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > OPP core can handle the regulators by itself, and but it needs to know > > the name of the regulator to fetch. Add support for that. > > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > --- > > drivers/cpufreq/cpufreq-dt.c | 46 ++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 46 insertions(+) > > > > diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c > > index 4c9f8a828f6f..2af75f8088bb 100644 > > --- a/drivers/cpufreq/cpufreq-dt.c > > +++ b/drivers/cpufreq/cpufreq-dt.c > > > @@ -119,6 +120,30 @@ static int set_target(struct cpufreq_policy *policy, unsigned int index) > > return ret; > > } > > > > +/* > > + * An earlier version of opp-v1 bindings used to name the regulator > > + * "cpu0-supply", we still need to handle that for backwards compatibility. > > + */ > > +static const char *find_supply_name(struct device *dev, struct device_node *np) > > +{ > > + struct property *pp; > > + int cpu = dev->id; > > + > > + /* Try "cpu0" for older DTs */ > > + if (!cpu) { > > + pp = of_find_property(np, "cpu0-supply", NULL); > > + if (pp) > > + return "cpu0"; > > + } > > + > > + pp = of_find_property(np, "cpu-supply", NULL); > > + if (pp) > > + return "cpu"; > > Despite the existence of lots of users of these properties, I couldn't find > both the "earlier version" and the "current version" of the opp-v1 bindings > documenting the "cpu0-supply" and "cpu-supply" properties? They are part of the device nodes and don't fall under the jurisdiction of OPP tables and so aren't defined there. We rely on the "<name>-supply" property from the regulator bindings for the devices. > Even for opp-v2, they are not documented in > Documentation/devicetree/bindings/opp/opp.txt, but cpu-supply is used in > the examples? Same reasoning here as well. > For v2, I did find "[PATCH 01/16] PM / OPP: Add 'supply-names' binding" > https://lore.kernel.org/lkml/2b87b162eabd1570ae2311e1ef8655acda72f678.1441972771.git.viresh.kumar@linaro.org/ > but presumably that's an even further evolution? Yeah, that never made it to mainline is abandoned. > Can you please document these properties? I don't think we need to, do we ?
Hi Viresh, On Wed, Jul 18, 2018 at 7:50 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 17-07-18, 09:46, Geert Uytterhoeven wrote: > > CC device-tree folks > > > > Replying to an old email, because that's the most accurate reference I > > could find. > > > > On Tue, Feb 9, 2016 at 6:06 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > OPP core can handle the regulators by itself, and but it needs to know > > > the name of the regulator to fetch. Add support for that. > > > > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > > --- > > > drivers/cpufreq/cpufreq-dt.c | 46 ++++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 46 insertions(+) > > > > > > diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c > > > index 4c9f8a828f6f..2af75f8088bb 100644 > > > --- a/drivers/cpufreq/cpufreq-dt.c > > > +++ b/drivers/cpufreq/cpufreq-dt.c > > > > > @@ -119,6 +120,30 @@ static int set_target(struct cpufreq_policy *policy, unsigned int index) > > > return ret; > > > } > > > > > > +/* > > > + * An earlier version of opp-v1 bindings used to name the regulator > > > + * "cpu0-supply", we still need to handle that for backwards compatibility. > > > + */ > > > +static const char *find_supply_name(struct device *dev, struct device_node *np) > > > +{ > > > + struct property *pp; > > > + int cpu = dev->id; > > > + > > > + /* Try "cpu0" for older DTs */ > > > + if (!cpu) { > > > + pp = of_find_property(np, "cpu0-supply", NULL); > > > + if (pp) > > > + return "cpu0"; > > > + } > > > + > > > + pp = of_find_property(np, "cpu-supply", NULL); > > > + if (pp) > > > + return "cpu"; > > > > Despite the existence of lots of users of these properties, I couldn't find > > both the "earlier version" and the "current version" of the opp-v1 bindings > > documenting the "cpu0-supply" and "cpu-supply" properties? > > They are part of the device nodes and don't fall under the > jurisdiction of OPP tables and so aren't defined there. We rely on the > "<name>-supply" property from the regulator bindings for the devices. > > > Even for opp-v2, they are not documented in > > Documentation/devicetree/bindings/opp/opp.txt, but cpu-supply is used in > > the examples? > > Same reasoning here as well. Right, they are part of the cpu nodes, not of the operating-points properties (which are part of the cpu nodes) or the opp_table nodes (for v2). > > Can you please document these properties? > > I don't think we need to, do we ? IMHO they should be documented somewhere, with a link to Documentation/devicetree/bindings/regulator/regulator.txt. How else can a DTS writer know he/she needs them? Without cpu-supply (and clock!) properties, cpufreq can't work. Candidates: - Documentation/devicetree/bindings/cpufreq/cpufreq-dt.txt This has operating points without clocks nor cpu-supply, but with clock-latency (for which clock? ;-) - Documentation/devicetree/bindings/cpufreq/arm_big_little_dt.txt Again, operating points without clocks nor cpu-supply, but with clock-latency. Thanks! Gr{oetje,eeting}s, Geert
On 18-07-18, 09:09, Geert Uytterhoeven wrote: > IMHO they should be documented somewhere, with a link to > Documentation/devicetree/bindings/regulator/regulator.txt. > How else can a DTS writer know he/she needs them? > Without cpu-supply (and clock!) properties, cpufreq can't work. > > Candidates: > - Documentation/devicetree/bindings/cpufreq/cpufreq-dt.txt > This has operating points without clocks nor cpu-supply, but with > clock-latency (for which clock? ;-) > - Documentation/devicetree/bindings/cpufreq/arm_big_little_dt.txt > Again, operating points without clocks nor cpu-supply, but with > clock-latency. Sounds reasonable. Will cook patches for that soon. Thanks.
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c index 4c9f8a828f6f..2af75f8088bb 100644 --- a/drivers/cpufreq/cpufreq-dt.c +++ b/drivers/cpufreq/cpufreq-dt.c @@ -34,6 +34,7 @@ struct private_data { struct regulator *cpu_reg; struct thermal_cooling_device *cdev; unsigned int voltage_tolerance; /* in percentage */ + const char *reg_name; }; static struct freq_attr *cpufreq_dt_attr[] = { @@ -119,6 +120,30 @@ static int set_target(struct cpufreq_policy *policy, unsigned int index) return ret; } +/* + * An earlier version of opp-v1 bindings used to name the regulator + * "cpu0-supply", we still need to handle that for backwards compatibility. + */ +static const char *find_supply_name(struct device *dev, struct device_node *np) +{ + struct property *pp; + int cpu = dev->id; + + /* Try "cpu0" for older DTs */ + if (!cpu) { + pp = of_find_property(np, "cpu0-supply", NULL); + if (pp) + return "cpu0"; + } + + pp = of_find_property(np, "cpu-supply", NULL); + if (pp) + return "cpu"; + + dev_dbg(dev, "no regulator for cpu%d\n", cpu); + return NULL; +} + static int allocate_resources(int cpu, struct device **cdev, struct regulator **creg, struct clk **cclk) { @@ -200,6 +225,7 @@ static int cpufreq_init(struct cpufreq_policy *policy) unsigned long min_uV = ~0, max_uV = 0; unsigned int transition_latency; bool opp_v1 = false; + const char *name; int ret; ret = allocate_resources(policy->cpu, &cpu_dev, &cpu_reg, &cpu_clk); @@ -229,6 +255,20 @@ static int cpufreq_init(struct cpufreq_policy *policy) } /* + * OPP layer will be taking care of regulators now, but it needs to know + * the name of the regulator first. + */ + name = find_supply_name(cpu_dev, np); + if (name) { + ret = dev_pm_opp_set_regulator(cpu_dev, name); + if (ret) { + dev_err(cpu_dev, "Failed to set regulator for cpu%d: %d\n", + policy->cpu, ret); + goto out_node_put; + } + } + + /* * Initialize OPP tables for all policy->cpus. They will be shared by * all CPUs which have marked their CPUs shared with OPP bindings. * @@ -273,6 +313,7 @@ static int cpufreq_init(struct cpufreq_policy *policy) goto out_free_opp; } + priv->reg_name = name; of_property_read_u32(np, "voltage-tolerance", &priv->voltage_tolerance); transition_latency = dev_pm_opp_get_max_clock_latency(cpu_dev); @@ -366,6 +407,8 @@ static int cpufreq_init(struct cpufreq_policy *policy) kfree(priv); out_free_opp: dev_pm_opp_of_cpumask_remove_table(policy->cpus); + if (name) + dev_pm_opp_put_regulator(cpu_dev); out_node_put: of_node_put(np); out_put_reg_clk: @@ -383,6 +426,9 @@ static int cpufreq_exit(struct cpufreq_policy *policy) cpufreq_cooling_unregister(priv->cdev); dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table); dev_pm_opp_of_cpumask_remove_table(policy->related_cpus); + if (priv->reg_name) + dev_pm_opp_put_regulator(priv->cpu_dev); + clk_put(policy->clk); if (!IS_ERR(priv->cpu_reg)) regulator_put(priv->cpu_reg);
OPP core can handle the regulators by itself, and but it needs to know the name of the regulator to fetch. Add support for that. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/cpufreq/cpufreq-dt.c | 46 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+)