| Message ID | 6379a5109bf6b4875cc1656b025f9b8186bbb91a.1400736536.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Thursday, May 22, 2014 11:07:28 AM Viresh Kumar wrote: > Drivers expecting CPU's OPPs from device tree initialize OPP table themselves by > calling of_init_opp_table() and there is nothing driver specific in that. They > all do it in the same redundant way. > > It would be better if we can get rid of redundancy by initializing CPU OPPs from > CPU core code for all CPUs (that have a "operating-points" property defined in > their node). > > This patch adds another routine in cpu.c: of_init_cpu_opp_table() and calls it > right after CPU device is registered in register_cpu(). A dummy implementation > is also provided to make it lightweight for platforms that don't need it. > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Amit Daniel Kachhap <amit.daniel@samsung.com> > Cc: Kukjin Kim <kgene.kim@samsung.com> > Cc: Shawn Guo <shawn.guo@linaro.org> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/base/cpu.c | 30 ++++++++++++++++++++++++++---- > 1 file changed, 26 insertions(+), 4 deletions(-) > > diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c > index 006b1bc..818cfe8 100644 > --- a/drivers/base/cpu.c > +++ b/drivers/base/cpu.c > @@ -16,6 +16,7 @@ > #include <linux/acpi.h> > #include <linux/of.h> > #include <linux/cpufeature.h> > +#include <linux/pm_opp.h> > > #include "base.h" > > @@ -322,6 +323,25 @@ static int cpu_uevent(struct device *dev, struct kobj_uevent_env *env) > } > #endif > > +#if defined(CONFIG_PM_OPP) && defined(CONFIG_OF) > +static inline void of_init_cpu_opp_table(struct cpu *cpu) Do you actually use cpu anywere in this function for anything other than just accessing cpu->dev? If not, why not to pass cpu->dev to it and move it somewhere in the OPP core? > +{ > + int error; > + > + /* Initialize CPU's OPP table */ > + error = of_init_opp_table(&cpu->dev); > + if (!error) > + dev_dbg(&cpu->dev, "%s: created OPP table for cpu: %d\n", > + __func__, cpu->dev.id); > + /* Print error only if there is an issue with OPP table */ > + else if (error != -ENOSYS && error != -ENODEV) > + dev_err(&cpu->dev, "%s: failed to init OPP table for cpu%d, err: %d\n", > + __func__, cpu->dev.id, error); > +} > +#else > +static inline void of_init_cpu_opp_table(struct cpu *cpu) {} > +#endif > + > /* > * register_cpu - Setup a sysfs device for a CPU. > * @cpu - cpu->hotpluggable field set to 1 will generate a control file in > @@ -349,10 +369,12 @@ int register_cpu(struct cpu *cpu, int num) > if (cpu->hotpluggable) > cpu->dev.groups = hotplugable_cpu_attr_groups; > error = device_register(&cpu->dev); > - if (!error) > - per_cpu(cpu_sys_devices, num) = &cpu->dev; > - if (!error) > - register_cpu_under_node(num, cpu_to_node(num)); > + if (error) > + return error; > + > + per_cpu(cpu_sys_devices, num) = &cpu->dev; > + register_cpu_under_node(num, cpu_to_node(num)); > + of_init_cpu_opp_table(cpu); > > return error; > } >
On 27 May 2014 05:02, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > Do you actually use cpu anywere in this function for anything other than > just accessing cpu->dev? If not, why not to pass cpu->dev to it and > move it somewhere in the OPP core? We are also using it for cpu->dev_id, but that's not so important. This routine wouldn't have existed if you wouldn't have asked for it. It is just a wrapper over of_init_opp_table, which also has a dummy implementation when its not supported. So, it might not be worth enough for any other code to use it. :) And so I added it here. Let me know how/where do you want it and I will resend it quickly. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 27 May 2014 05:34, Viresh Kumar <viresh.kumar@linaro.org> wrote: > We are also using it for cpu->dev_id, but that's not so important. This > routine wouldn't have existed if you wouldn't have asked for it. It is > just a wrapper over of_init_opp_table, which also has a dummy > implementation when its not supported. > > So, it might not be worth enough for any other code to use it. :) > And so I added it here. > > Let me know how/where do you want it and I will resend it quickly. There is another option here, add these print messages in of_init_opp_table() ? That's the only difference new wrapper has. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, May 27, 2014 05:48:27 AM Viresh Kumar wrote: > On 27 May 2014 05:34, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > We are also using it for cpu->dev_id, but that's not so important. This > > routine wouldn't have existed if you wouldn't have asked for it. It is > > just a wrapper over of_init_opp_table, which also has a dummy > > implementation when its not supported. > > > > So, it might not be worth enough for any other code to use it. :) > > And so I added it here. > > > > Let me know how/where do you want it and I will resend it quickly. > > There is another option here, add these print messages in > of_init_opp_table() ? That's the only difference new wrapper has. Yeah, put them into of_init_opp_table() rather I'd say. Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c index 006b1bc..818cfe8 100644 --- a/drivers/base/cpu.c +++ b/drivers/base/cpu.c @@ -16,6 +16,7 @@ #include <linux/acpi.h> #include <linux/of.h> #include <linux/cpufeature.h> +#include <linux/pm_opp.h> #include "base.h" @@ -322,6 +323,25 @@ static int cpu_uevent(struct device *dev, struct kobj_uevent_env *env) } #endif +#if defined(CONFIG_PM_OPP) && defined(CONFIG_OF) +static inline void of_init_cpu_opp_table(struct cpu *cpu) +{ + int error; + + /* Initialize CPU's OPP table */ + error = of_init_opp_table(&cpu->dev); + if (!error) + dev_dbg(&cpu->dev, "%s: created OPP table for cpu: %d\n", + __func__, cpu->dev.id); + /* Print error only if there is an issue with OPP table */ + else if (error != -ENOSYS && error != -ENODEV) + dev_err(&cpu->dev, "%s: failed to init OPP table for cpu%d, err: %d\n", + __func__, cpu->dev.id, error); +} +#else +static inline void of_init_cpu_opp_table(struct cpu *cpu) {} +#endif + /* * register_cpu - Setup a sysfs device for a CPU. * @cpu - cpu->hotpluggable field set to 1 will generate a control file in @@ -349,10 +369,12 @@ int register_cpu(struct cpu *cpu, int num) if (cpu->hotpluggable) cpu->dev.groups = hotplugable_cpu_attr_groups; error = device_register(&cpu->dev); - if (!error) - per_cpu(cpu_sys_devices, num) = &cpu->dev; - if (!error) - register_cpu_under_node(num, cpu_to_node(num)); + if (error) + return error; + + per_cpu(cpu_sys_devices, num) = &cpu->dev; + register_cpu_under_node(num, cpu_to_node(num)); + of_init_cpu_opp_table(cpu); return error; }
Drivers expecting CPU's OPPs from device tree initialize OPP table themselves by calling of_init_opp_table() and there is nothing driver specific in that. They all do it in the same redundant way. It would be better if we can get rid of redundancy by initializing CPU OPPs from CPU core code for all CPUs (that have a "operating-points" property defined in their node). This patch adds another routine in cpu.c: of_init_cpu_opp_table() and calls it right after CPU device is registered in register_cpu(). A dummy implementation is also provided to make it lightweight for platforms that don't need it. Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Amit Daniel Kachhap <amit.daniel@samsung.com> Cc: Kukjin Kim <kgene.kim@samsung.com> Cc: Shawn Guo <shawn.guo@linaro.org> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/base/cpu.c | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-)