| Message ID | ce68a1ba333ab080a2428242d8fa4d4629064a4d.1401191054.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 05/27/2014 06:50 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 calls of_init_opp_table() right after CPU device is registered in > register_cpu(). of_init_opp_table() also has a dummy implementation which simply > returns -ENOSYS when CONFIG_OPP or CONFIG_OF isn't supported by some platform. > > 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> > --- > drivers/base/cpu.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c > index 006b1bc..790183f 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,10 +350,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)); The change so far is a improvement in error handling -> which, personally I find nice, but not necessarily related to the $subject or covered in commit message. I suggest splitting that specific change out as a patch of it's own. > + of_init_opp_table(&cpu->dev); tricky... :) I finally did catchup on previous discussions https://patchwork.kernel.org/patch/4199601/ https://patchwork.kernel.org/patch/4199741/ https://patchwork.kernel.org/patch/4215901/ https://patchwork.kernel.org/patch/4220431/ So, will let Rafael comment better here. > > return error; > } >
On 30 May 2014 04:11, Nishanth Menon <nm@ti.com> wrote: > The change so far is a improvement in error handling -> which, > personally I find nice, but not necessarily related to the $subject or > covered in commit message. I suggest splitting that specific change > out as a patch of it's own. I would be required to do this here: if (!error) of_init_opp_table(**); And so did this change as part of this patch :), probably rafael doesn't feel a need of this and so has asked me to do a resend of a single patch (reported by Sachin) .. So, will leave it as is unless he wants me to send it again :) For this and other patches, thanks for your review. -- 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:20:53 PM 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 calls of_init_opp_table() right after CPU device is registered in > register_cpu(). of_init_opp_table() also has a dummy implementation which simply > returns -ENOSYS when CONFIG_OPP or CONFIG_OF isn't supported by some platform. > > 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> > --- > drivers/base/cpu.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c > index 006b1bc..790183f 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,10 +350,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_opp_table(&cpu->dev); Having thought a bit more about that I don't really like this. In the ACPI land we have a processor driver that binds to CPU devices and carries out similar initialization. I kind of prefer that to littering core code with platform-specific stuff. 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..790183f 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,10 +350,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_opp_table(&cpu->dev); 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 calls of_init_opp_table() right after CPU device is registered in register_cpu(). of_init_opp_table() also has a dummy implementation which simply returns -ENOSYS when CONFIG_OPP or CONFIG_OF isn't supported by some platform. 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> --- drivers/base/cpu.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)