| Message ID | b59df199b373191791afcccd627d900ce07afa71.1400670427.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 21/05/14 12:10, Viresh Kumar wrote: > All drivers expecting CPU's OPPs from device tree initialize OPP table using > of_init_opp_table() and there is nothing driver specific in that. They all do it > in the same way adding to code redundancy. > > It would be better if we can get rid of code redundancy by initializing CPU OPPs > from core code for all CPUs that have a "operating-points" property defined in > their node. > > This patch initializes OPPs as soon as CPU device is registered in > register_cpu(). > > 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..853e99e 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_info(&cpu->dev, "%s: created OPP table for cpu: %d\n", > + __func__, cpu->dev.id); [Nit] Not sure if we need this logging on each cpu, may be dev_dbg if you still fancy one ? :) > + /* 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 > + IMO this could be more generic and applicable for any device if you replace "struct cpu" with "struct device". I know this is currently used by cpu but we can extend it's use for any device if needed in future. You can then move this to pm_opp.h as of_init_dev_opp_table may be. Regards, Sudeep -- 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 21 May 2014 19:15, Sudeep Holla <sudeep.holla@arm.com> wrote: > [Nit] Not sure if we need this logging on each cpu, may be dev_dbg if you > still > fancy one ? :) It wouldn't happen on each CPU, but one CPU per policy as others would return -ENODEV. I added dev_dbg earlier but then I thought dev_info is better as we may better show this to everybody as it about the most important device, i.e. CPU :) >> + /* 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 >> + > > > IMO this could be more generic and applicable for any device if you replace > "struct cpu" with "struct device". I know this is currently used by cpu but > we can extend it's use for any device if needed in future. You can then move > this to pm_opp.h as of_init_dev_opp_table may be. Probably we will call of_init_opp_table() directly for other devices, as this function doesn't do anything else, apart from some prints.. So, probably leave is as is for now, unless a real need arises ? -- 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 21/05/14 15:01, Viresh Kumar wrote: > On 21 May 2014 19:15, Sudeep Holla <sudeep.holla@arm.com> wrote: >> [Nit] Not sure if we need this logging on each cpu, may be dev_dbg if you >> still >> fancy one ? :) > > It wouldn't happen on each CPU, but one CPU per policy as others > would return -ENODEV. > Hmm agreed, but there are SoCs that support per CPU DVFS ;) > I added dev_dbg earlier but then I thought dev_info is better as we may > better show this to everybody as it about the most important device, > i.e. CPU :) > >>> + /* 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 >>> + >> >> >> IMO this could be more generic and applicable for any device if you replace >> "struct cpu" with "struct device". I know this is currently used by cpu but >> we can extend it's use for any device if needed in future. You can then move >> this to pm_opp.h as of_init_dev_opp_table may be. > > Probably we will call of_init_opp_table() directly for other devices, as this > function doesn't do anything else, apart from some prints.. So, probably > leave is as is for now, unless a real need arises ? > I don't see anything cpu specific there, but that's just my opinion. Regards, Sudeep -- 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 21 May 2014 20:12, Sudeep Holla <sudeep.holla@arm.com> wrote: > Hmm agreed, but there are SoCs that support per CPU DVFS ;) Lets see what's Nishanth/Rafael's view on this. I am more or less in favor of it. But isn't a big thing. Can convert it to dbg if it annoys you :) >> Probably we will call of_init_opp_table() directly for other devices, as >> this >> function doesn't do anything else, apart from some prints.. So, probably >> leave is as is for now, unless a real need arises ? > I don't see anything cpu specific there, but that's just my opinion. I never said that it has anything cpu specific.. What I said was that this routine wouldn't have existed if Rafael 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. :) But in case it is, we can add it later. -- 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 21/05/14 12:10, Viresh Kumar wrote: > All drivers expecting CPU's OPPs from device tree initialize OPP table using > of_init_opp_table() and there is nothing driver specific in that. They all do it > in the same way adding to code redundancy. > > It would be better if we can get rid of code redundancy by initializing CPU OPPs > from core code for all CPUs that have a "operating-points" property defined in > their node. > > This patch initializes OPPs as soon as CPU device is registered in > register_cpu(). > > 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..853e99e 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_info(&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) {} Sorry I missed this earlier, main idea of this wrapper is not to have any config dependency and hide error handling details for non-DT platforms. Since of_init_opp_table has dummy implementation, you really don't need this dummy implementation again here. Regards, Sudeep -- 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 21 May 2014 22:56, Sudeep Holla <sudeep.holla@arm.com> wrote: > Sorry I missed this earlier, main idea of this wrapper is not to have any > config dependency and hide error handling details for non-DT platforms. > Since > of_init_opp_table has dummy implementation, you really don't need this dummy > implementation again here. x-( (That's my angry face ..) I still added it so that the next few conditional statements (error checking) also go away for non-DT platforms.. So, lets keep for now. We may have more additions into it in future.. -- 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/
diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c index 006b1bc..853e99e 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_info(&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; }
All drivers expecting CPU's OPPs from device tree initialize OPP table using of_init_opp_table() and there is nothing driver specific in that. They all do it in the same way adding to code redundancy. It would be better if we can get rid of code redundancy by initializing CPU OPPs from core code for all CPUs that have a "operating-points" property defined in their node. This patch initializes OPPs as soon as CPU device is registered in register_cpu(). 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(-)