| Message ID | ca7cc76c49806ce1ac8f9617fa4266cfc29d1ea1.1400480592.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Monday, May 19, 2014 11:59:11 AM 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> > Cc: Sudeep Holla <sudeep.holla@arm.com> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > V1-V2: > A colleague spotted some extra debug prints in my first mail :( > > Replace > + pr_err("****%s: failed to init OPP table for cpu%d, err: %d\n", > with > + pr_err("%s: failed to init OPP table for cpu%d, err: %d\n", > > drivers/base/cpu.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c > index 006b1bc..74ce944 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" > > @@ -349,11 +350,20 @@ 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) What about if (error) return error; and then you'd save an indentation level? Anyway, I find adding of_node* stuff directly to the driver core this way kind of disgusting as there still are platforms that don't use it. Can we have a call to a function that will change into an empty stub on such platforms here, please? > + if (!error) { > per_cpu(cpu_sys_devices, num) = &cpu->dev; > - if (!error) > register_cpu_under_node(num, cpu_to_node(num)); > > + /* Initialize CPUs OPP table */ > + if (of_node_get(cpu->dev.of_node)) { > + error = of_init_opp_table(&cpu->dev); > + if (error && error != -ENODEV) > + pr_err("%s: failed to init OPP table for cpu%d, err: %d\n", > + __func__, num, error); > + of_node_put(cpu->dev.of_node); > + } > + } > + > return error; > } > >
On Monday, May 19, 2014 11:13:24 PM Rafael J. Wysocki wrote: > On Monday, May 19, 2014 11:59:11 AM 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> > > Cc: Sudeep Holla <sudeep.holla@arm.com> > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> Do patches [2-5/5] depend on this one BTW?
On 20 May 2014 02:43, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > What about > > if (error) > return error; > > and then you'd save an indentation level? Yes. > Anyway, I find adding of_node* stuff directly to the driver core this way > kind of disgusting as there still are platforms that don't use it. > > Can we have a call to a function that will change into an empty stub on such > platforms here, please? Okay. But can you explain a bit more about how and where those stubs would be implemented? -- 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 20 May 2014 02:46, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> Do patches [2-5/5] depend on this one BTW?
Yes.
--
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 19/05/14 07:29, 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(). > This is really nice getting rid of all duplicate code. > 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> > Cc: Sudeep Holla <sudeep.holla@arm.com> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > V1-V2: > A colleague spotted some extra debug prints in my first mail :( > > Replace > + pr_err("****%s: failed to init OPP table for cpu%d, err: %d\n", > with > + pr_err("%s: failed to init OPP table for cpu%d, err: %d\n", > > drivers/base/cpu.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c > index 006b1bc..74ce944 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" > > @@ -349,11 +350,20 @@ 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) > + if (!error) { > per_cpu(cpu_sys_devices, num) = &cpu->dev; > - if (!error) > register_cpu_under_node(num, cpu_to_node(num)); > > + /* Initialize CPUs OPP table */ > + if (of_node_get(cpu->dev.of_node)) { > + error = of_init_opp_table(&cpu->dev); > + if (error && error != -ENODEV) As Rafael mentioned it's better to have a wrapper function to hide these details. You should consider the fact that of_init_opp_table returns -EINVAL if CONFIG_PM_OPP not defined as well as when the list is invalid in the DT. IMO we can return -ENOSYS if not implemented(i.e. !CONFIG_PM_OPP) > + pr_err("%s: failed to init OPP table for cpu%d, err: %d\n", > + __func__, num, error); > + of_node_put(cpu->dev.of_node); > + } > + } > + > return error; > } > 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 15:05, Sudeep Holla <sudeep.holla@arm.com> wrote: > As Rafael mentioned it's better to have a wrapper function to hide these > details. You should consider the fact that of_init_opp_table returns -EINVAL > if > CONFIG_PM_OPP not defined as well as when the list is invalid in the DT. > IMO we can return -ENOSYS if not implemented(i.e. !CONFIG_PM_OPP) I didn't understood Rafael's comment as I couldn't figure out if he is just pointing to CONFIG_** or some arch specific thing.. But it looks more obvious that he asked me something similar to what you are saying :) Why do we need to return anything? Let that function have return type 'void'? Also would it make sense to move this into it as well? cpu->dev.of_node = of_get_cpu_node(num, NULL); -- 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 10:41, Viresh Kumar wrote: > On 21 May 2014 15:05, Sudeep Holla <sudeep.holla@arm.com> wrote: >> As Rafael mentioned it's better to have a wrapper function to hide these >> details. You should consider the fact that of_init_opp_table returns -EINVAL >> if >> CONFIG_PM_OPP not defined as well as when the list is invalid in the DT. >> IMO we can return -ENOSYS if not implemented(i.e. !CONFIG_PM_OPP) > > I didn't understood Rafael's comment as I couldn't figure out if he is just > pointing to CONFIG_** or some arch specific thing.. > > But it looks more obvious that he asked me something similar to what you > are saying :) > I believe so, mainly the non-DT case, since you are checking for error, it will end up with spurious messages as the return value is -EINVAL. Hence I was suggesting return -ENOSYS(which means Function not implemented) > Why do we need to return anything? Let that function have return type 'void'? Hmm, don't we still need to throw error if DT has invalid OPP ? It doesn't may sense to me if no errors is returned and still CPUFreq fails later. > Also would it make sense to move this into it as well? > > cpu->dev.of_node = of_get_cpu_node(num, NULL); > I don't quite understand what you mean 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 15:18, Sudeep Holla <sudeep.holla@arm.com> wrote: >> Why do we need to return anything? Let that function have return type >> 'void'? > > > Hmm, don't we still need to throw error if DT has invalid OPP ? > It doesn't may sense to me if no errors is returned and still CPUFreq fails > later. I wasn't sure if we should fail CPU's registration if it had bad OPPs. Maybe just print a good message and go ahead. CPU is usable atleast :) >> Also would it make sense to move this into it as well? >> >> cpu->dev.of_node = of_get_cpu_node(num, NULL); >> > > I don't quite understand what you mean here ? This piece of code was added by you earlier to register_cpu(). As this was also DT specific, should I move this as well to the new routine we are creating? -- 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 Tuesday, May 20, 2014 08:08:11 AM Viresh Kumar wrote: > On 20 May 2014 02:43, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > What about > > > > if (error) > > return error; > > > > and then you'd save an indentation level? > > Yes. > > > Anyway, I find adding of_node* stuff directly to the driver core this way > > kind of disgusting as there still are platforms that don't use it. > > > > Can we have a call to a function that will change into an empty stub on such > > platforms here, please? > > Okay. But can you explain a bit more about how and where those stubs > would be implemented? In a header file included by cpu.c. Something like #if defined(CONFIG_OF) && defined(CONFIG_PM_OPP) <function header> #else static inline <stub> #endif
On 22 May 2014 05:27, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > In a header file included by cpu.c. Something like > > #if defined(CONFIG_OF) && defined(CONFIG_PM_OPP) > <function header> > #else > static inline <stub> > #endif Thanks, Sudeep already helped me in understanding that :) .. Already implemented that in cpu.c only .. -- 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..74ce944 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" @@ -349,11 +350,20 @@ 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) + if (!error) { per_cpu(cpu_sys_devices, num) = &cpu->dev; - if (!error) register_cpu_under_node(num, cpu_to_node(num)); + /* Initialize CPUs OPP table */ + if (of_node_get(cpu->dev.of_node)) { + error = of_init_opp_table(&cpu->dev); + if (error && error != -ENODEV) + pr_err("%s: failed to init OPP table for cpu%d, err: %d\n", + __func__, num, error); + of_node_put(cpu->dev.of_node); + } + } + 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> Cc: Sudeep Holla <sudeep.holla@arm.com> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- V1-V2: A colleague spotted some extra debug prints in my first mail :( Replace + pr_err("****%s: failed to init OPP table for cpu%d, err: %d\n", with + pr_err("%s: failed to init OPP table for cpu%d, err: %d\n", drivers/base/cpu.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)