| Message ID | 1359428020-21068-1-git-send-email-shawn.guo@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jan 29, 2013 at 8:23 AM, Shawn Guo <shawn.guo@linaro.org> wrote: > As multiplatform build is being adopted by more and more ARM platforms, > initcall function should be used very carefully. For example, when > GENERIC_CPUFREQ_CPU0 is built in the kernel, cpu0_cpufreq_driver_init() > will be called on all the platforms to initialize cpufreq-cpu0 driver. > > To eliminate this undesired the effect, the patch changes cpufreq-cpu0 > driver to have it instantiated as a platform_driver. Then it will only > run on platforms that create the platform_device "cpufreq-cpu0". > > Along with the change, it also changes cpu_dev to be &pdev->dev, > so that managed functions can start working, and module build gets > supported too. > > The existing users of cpufreq-cpu0 driver highbank and am33xx are also > updated accordingly to adapt the changes. > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > Cc: Mark Langsdorf <mark.langsdorf@calxeda.com> > Cc: AnilKumar Ch <anilkumar@ti.com> > Cc: Tony Lindgren <tony@atomide.com> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
On Tue, Jan 29, 2013 at 08:23:40, Shawn Guo wrote: > As multiplatform build is being adopted by more and more ARM platforms, > initcall function should be used very carefully. For example, when > GENERIC_CPUFREQ_CPU0 is built in the kernel, cpu0_cpufreq_driver_init() > will be called on all the platforms to initialize cpufreq-cpu0 driver. > > To eliminate this undesired the effect, the patch changes cpufreq-cpu0 > driver to have it instantiated as a platform_driver. Then it will only > run on platforms that create the platform_device "cpufreq-cpu0". > > Along with the change, it also changes cpu_dev to be &pdev->dev, > so that managed functions can start working, and module build gets > supported too. > > The existing users of cpufreq-cpu0 driver highbank and am33xx are also > updated accordingly to adapt the changes. > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > Cc: Mark Langsdorf <mark.langsdorf@calxeda.com> > Cc: AnilKumar Ch <anilkumar@ti.com> > Cc: Tony Lindgren <tony@atomide.com> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > Changes since v1: > * Migrate cpufreq-cpu0 users in the same patch > > Rafael, > > The patch is based on Mark's highbank-cpufreq series and Nishanth's > "PM / OPP : export symbol consolidation" sereis. > > Mark, AnilKumar, > > I only compile-tested it on highbank and omap2. Please give it a test > no hardware to make sure cpufreq-cpu0 still works for you. Thanks. Hi Shawn, I hope this is based on linux-omap/master, to test the driver I have to add some patches on top of this patch, because of recent changes. I will provide the test details once I am done. Thanks AnilKumar
On Wednesday, January 30, 2013 07:10:20 AM AnilKumar, Chimata wrote: > On Tue, Jan 29, 2013 at 08:23:40, Shawn Guo wrote: > > As multiplatform build is being adopted by more and more ARM platforms, > > initcall function should be used very carefully. For example, when > > GENERIC_CPUFREQ_CPU0 is built in the kernel, cpu0_cpufreq_driver_init() > > will be called on all the platforms to initialize cpufreq-cpu0 driver. > > > > To eliminate this undesired the effect, the patch changes cpufreq-cpu0 > > driver to have it instantiated as a platform_driver. Then it will only > > run on platforms that create the platform_device "cpufreq-cpu0". > > > > Along with the change, it also changes cpu_dev to be &pdev->dev, > > so that managed functions can start working, and module build gets > > supported too. > > > > The existing users of cpufreq-cpu0 driver highbank and am33xx are also > > updated accordingly to adapt the changes. > > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > > Cc: Mark Langsdorf <mark.langsdorf@calxeda.com> > > Cc: AnilKumar Ch <anilkumar@ti.com> > > Cc: Tony Lindgren <tony@atomide.com> > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > Changes since v1: > > * Migrate cpufreq-cpu0 users in the same patch > > > > Rafael, > > > > The patch is based on Mark's highbank-cpufreq series and Nishanth's > > "PM / OPP : export symbol consolidation" sereis. > > > > Mark, AnilKumar, > > > > I only compile-tested it on highbank and omap2. Please give it a test > > no hardware to make sure cpufreq-cpu0 still works for you. Thanks. > > Hi Shawn, > > I hope this is based on linux-omap/master, to test the driver I have > to add some patches on top of this patch, because of recent changes. > I will provide the test details once I am done. Shawn, does it mean I need to wait with applying the patch or will OMAP2 work as is? Rafael
On Wed, Jan 30, 2013 at 02:40:35PM +0100, Rafael J. Wysocki wrote: > > > Mark, AnilKumar, > > > > > > I only compile-tested it on highbank and omap2. Please give it a test > > > no hardware to make sure cpufreq-cpu0 still works for you. Thanks. > > > > Hi Shawn, > > > > I hope this is based on linux-omap/master, to test the driver I have > > to add some patches on top of this patch, because of recent changes. > > I will provide the test details once I am done. > > Shawn, does it mean I need to wait with applying the patch or will OMAP2 > work as is? > I would expect that omap2/am33xx should work as it is, but I do not have hardware to confirm that. Also as the patch touches a few files in arch/arm/mach-omap2, we may need an ACK from Tony. Actually, I prefer to move omap2 bits into a separate patch and have it go via omap tree to avoid the possible conflict with cpufreq and arm-soc tree. What do you say? If you agree, I will re-iterate the patch to do so. Shawn
On Wednesday, January 30, 2013 09:49:41 PM Shawn Guo wrote: > On Wed, Jan 30, 2013 at 02:40:35PM +0100, Rafael J. Wysocki wrote: > > > > Mark, AnilKumar, > > > > > > > > I only compile-tested it on highbank and omap2. Please give it a test > > > > no hardware to make sure cpufreq-cpu0 still works for you. Thanks. > > > > > > Hi Shawn, > > > > > > I hope this is based on linux-omap/master, to test the driver I have > > > to add some patches on top of this patch, because of recent changes. > > > I will provide the test details once I am done. > > > > Shawn, does it mean I need to wait with applying the patch or will OMAP2 > > work as is? > > > I would expect that omap2/am33xx should work as it is, but I do not > have hardware to confirm that. > > Also as the patch touches a few files in arch/arm/mach-omap2, we may > need an ACK from Tony. > > Actually, I prefer to move omap2 bits into a separate patch and have > it go via omap tree to avoid the possible conflict with cpufreq and > arm-soc tree. What do you say? If you agree, I will re-iterate the > patch to do so. Yes, please do as you think is appropriate. Thanks, Rafael
On Tue, 29 Jan 2013, Shawn Guo wrote: > As multiplatform build is being adopted by more and more ARM platforms, > initcall function should be used very carefully. For example, when > GENERIC_CPUFREQ_CPU0 is built in the kernel, cpu0_cpufreq_driver_init() > will be called on all the platforms to initialize cpufreq-cpu0 driver. > > To eliminate this undesired the effect, the patch changes cpufreq-cpu0 > driver to have it instantiated as a platform_driver. Then it will only > run on platforms that create the platform_device "cpufreq-cpu0". Sorry, confused. Before this used to be a generic cpufreq driver, usable on all (DT-enabled only) platforms. You just had to provide an OPP table, a clock, a regulator, similar to this http://thread.gmane.org/gmane.linux.kernel.cpufreq/9510/focus=9509 (also see the complete thread for more information). Now this won't work obviously. Instead we now need a pseudo platform device to instantiate cpufreq-cpu0. This device cannot be put in DT, because it doesn't describe real hardware. So, we have to add register it from the board specific .c code, which we actually want to get rid of... Is this really what we want? What about other cpufreq drivers? They have the same problem on multiplatform builds, right? Say, s3c2416-cpufreq.c. It also just initialises itself and starts looking for a clock, names "msysclk" with a NULL device pointer etc. Don't we need a common approach for cpufreq driver initialisation? The decision which cpufreq driver to use is SoC-specific, right? We're unlikely to have several boards, using the same SoC, wishing to use different cpufreq drivers? The decision _whether_ or not to enable it and _which_ resources to use might be board-specific. So, how about adding a cpufreq call something like cpufreq_driver_request("cpufreq-driver-name"); to be called by SoC-specific code. You can say it is not much different from adding a virtual device, but firstly I think such a use of a platform device is really an overkill. Secondly you still run a danger, that several platforms, built into a single image, register several devices for different cpufreq drivers, or even for one... With a special call you know there can be only one and you return -EBUSY to all further calls to that function. Thanks Guennadi > > Along with the change, it also changes cpu_dev to be &pdev->dev, > so that managed functions can start working, and module build gets > supported too. > > The existing users of cpufreq-cpu0 driver highbank and am33xx are also > updated accordingly to adapt the changes. > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > Cc: Mark Langsdorf <mark.langsdorf@calxeda.com> > Cc: AnilKumar Ch <anilkumar@ti.com> > Cc: Tony Lindgren <tony@atomide.com> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > Changes since v1: > * Migrate cpufreq-cpu0 users in the same patch > > Rafael, > > The patch is based on Mark's highbank-cpufreq series and Nishanth's > "PM / OPP : export symbol consolidation" sereis. > > Mark, AnilKumar, > > I only compile-tested it on highbank and omap2. Please give it a test > no hardware to make sure cpufreq-cpu0 still works for you. Thanks. > > Shawn > > arch/arm/mach-omap2/board-generic.c | 1 + > arch/arm/mach-omap2/cclock33xx_data.c | 2 +- > arch/arm/mach-omap2/common.h | 1 + > arch/arm/mach-omap2/io.c | 7 +++++++ > drivers/cpufreq/Kconfig | 2 +- > drivers/cpufreq/cpufreq-cpu0.c | 35 ++++++++++++++++++++++----------- > drivers/cpufreq/highbank-cpufreq.c | 5 +++++ > 7 files changed, 39 insertions(+), 14 deletions(-) > > diff --git a/arch/arm/mach-omap2/board-generic.c b/arch/arm/mach-omap2/board-generic.c > index 53cb380b..b945480 100644 > --- a/arch/arm/mach-omap2/board-generic.c > +++ b/arch/arm/mach-omap2/board-generic.c > @@ -139,6 +139,7 @@ DT_MACHINE_START(AM33XX_DT, "Generic AM33XX (Flattened Device Tree)") > .init_irq = omap_intc_of_init, > .handle_irq = omap3_intc_handle_irq, > .init_machine = omap_generic_init, > + .init_late = am33xx_init_late, > .timer = &omap3_am33xx_timer, > .dt_compat = am33xx_boards_compat, > MACHINE_END > diff --git a/arch/arm/mach-omap2/cclock33xx_data.c b/arch/arm/mach-omap2/cclock33xx_data.c > index ea64ad6..acb1620 100644 > --- a/arch/arm/mach-omap2/cclock33xx_data.c > +++ b/arch/arm/mach-omap2/cclock33xx_data.c > @@ -850,7 +850,7 @@ static struct omap_clk am33xx_clks[] = { > CLK(NULL, "dpll_core_m5_ck", &dpll_core_m5_ck, CK_AM33XX), > CLK(NULL, "dpll_core_m6_ck", &dpll_core_m6_ck, CK_AM33XX), > CLK(NULL, "dpll_mpu_ck", &dpll_mpu_ck, CK_AM33XX), > - CLK("cpu0", NULL, &dpll_mpu_ck, CK_AM33XX), > + CLK("cpufreq-cpu0.0", NULL, &dpll_mpu_ck, CK_AM33XX), > CLK(NULL, "dpll_mpu_m2_ck", &dpll_mpu_m2_ck, CK_AM33XX), > CLK(NULL, "dpll_ddr_ck", &dpll_ddr_ck, CK_AM33XX), > CLK(NULL, "dpll_ddr_m2_ck", &dpll_ddr_m2_ck, CK_AM33XX), > diff --git a/arch/arm/mach-omap2/common.h b/arch/arm/mach-omap2/common.h > index 948bcaa..e3355df5 100644 > --- a/arch/arm/mach-omap2/common.h > +++ b/arch/arm/mach-omap2/common.h > @@ -106,6 +106,7 @@ void omap2430_init_late(void); > void omap3430_init_late(void); > void omap35xx_init_late(void); > void omap3630_init_late(void); > +void am33xx_init_late(void); > void am35xx_init_late(void); > void ti81xx_init_late(void); > void omap4430_init_late(void); > diff --git a/arch/arm/mach-omap2/io.c b/arch/arm/mach-omap2/io.c > index 2c3fdd6..7acfb8a 100644 > --- a/arch/arm/mach-omap2/io.c > +++ b/arch/arm/mach-omap2/io.c > @@ -535,6 +535,13 @@ void __init omap3630_init_late(void) > omap2_clk_enable_autoidle_all(); > } > > +void __init am33xx_init_late(void) > +{ > + struct platform_device_info devinfo = { .name = "cpufreq-cpu0", }; > + > + platform_device_register_full(&devinfo); > +} > + > void __init am35xx_init_late(void) > { > omap_mux_late_init(); > diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig > index ea512f4..774dc1c 100644 > --- a/drivers/cpufreq/Kconfig > +++ b/drivers/cpufreq/Kconfig > @@ -180,7 +180,7 @@ config CPU_FREQ_GOV_CONSERVATIVE > If in doubt, say N. > > config GENERIC_CPUFREQ_CPU0 > - bool "Generic CPU0 cpufreq driver" > + tristate "Generic CPU0 cpufreq driver" > depends on HAVE_CLK && REGULATOR && PM_OPP && OF > select CPU_FREQ_TABLE > help > diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c > index 90e9d73..519c2f7 100644 > --- a/drivers/cpufreq/cpufreq-cpu0.c > +++ b/drivers/cpufreq/cpufreq-cpu0.c > @@ -12,12 +12,12 @@ > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > #include <linux/clk.h> > -#include <linux/cpu.h> > #include <linux/cpufreq.h> > #include <linux/err.h> > #include <linux/module.h> > #include <linux/of.h> > #include <linux/opp.h> > +#include <linux/platform_device.h> > #include <linux/regulator/consumer.h> > #include <linux/slab.h> > > @@ -174,7 +174,7 @@ static struct cpufreq_driver cpu0_cpufreq_driver = { > .attr = cpu0_cpufreq_attr, > }; > > -static int cpu0_cpufreq_driver_init(void) > +static int cpu0_cpufreq_probe(struct platform_device *pdev) > { > struct device_node *np; > int ret; > @@ -189,23 +189,17 @@ static int cpu0_cpufreq_driver_init(void) > return -ENOENT; > } > > - cpu_dev = get_cpu_device(0); > - if (!cpu_dev) { > - pr_err("failed to get cpu0 device\n"); > - ret = -ENODEV; > - goto out_put_node; > - } > - > + cpu_dev = &pdev->dev; > cpu_dev->of_node = np; > > - cpu_clk = clk_get(cpu_dev, NULL); > + cpu_clk = devm_clk_get(cpu_dev, NULL); > if (IS_ERR(cpu_clk)) { > ret = PTR_ERR(cpu_clk); > pr_err("failed to get cpu0 clock: %d\n", ret); > goto out_put_node; > } > > - cpu_reg = regulator_get(cpu_dev, "cpu0"); > + cpu_reg = devm_regulator_get(cpu_dev, "cpu0"); > if (IS_ERR(cpu_reg)) { > pr_warn("failed to get cpu0 regulator\n"); > cpu_reg = NULL; > @@ -266,7 +260,24 @@ out_put_node: > of_node_put(np); > return ret; > } > -late_initcall(cpu0_cpufreq_driver_init); > + > +static int cpu0_cpufreq_remove(struct platform_device *pdev) > +{ > + cpufreq_unregister_driver(&cpu0_cpufreq_driver); > + opp_free_cpufreq_table(cpu_dev, &freq_table); > + > + return 0; > +} > + > +static struct platform_driver cpu0_cpufreq_platdrv = { > + .driver = { > + .name = "cpufreq-cpu0", > + .owner = THIS_MODULE, > + }, > + .probe = cpu0_cpufreq_probe, > + .remove = cpu0_cpufreq_remove, > +}; > +module_platform_driver(cpu0_cpufreq_platdrv); > > MODULE_AUTHOR("Shawn Guo <shawn.guo@linaro.org>"); > MODULE_DESCRIPTION("Generic CPU0 cpufreq driver"); > diff --git a/drivers/cpufreq/highbank-cpufreq.c b/drivers/cpufreq/highbank-cpufreq.c > index 2ea6276..0491f1f 100644 > --- a/drivers/cpufreq/highbank-cpufreq.c > +++ b/drivers/cpufreq/highbank-cpufreq.c > @@ -20,6 +20,7 @@ > #include <linux/err.h> > #include <linux/of.h> > #include <linux/mailbox.h> > +#include <linux/platform_device.h> > > #define HB_CPUFREQ_CHANGE_NOTE 0x80000001 > #define HB_CPUFREQ_IPC_LEN 7 > @@ -65,6 +66,7 @@ static struct notifier_block hb_cpufreq_clk_nb = { > > static int hb_cpufreq_driver_init(void) > { > + struct platform_device_info devinfo = { .name = "cpufreq-cpu0", }; > struct device *cpu_dev; > struct clk *cpu_clk; > struct device_node *np; > @@ -104,6 +106,9 @@ static int hb_cpufreq_driver_init(void) > goto out_put_node; > } > > + /* Instantiate cpufreq-cpu0 */ > + platform_device_register_full(&devinfo); > + > out_put_node: > of_node_put(np); > return ret; > -- > 1.7.9.5 > > > -- > 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 > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/
-Anil as the mail id does not seem to be accepted by TI mail server anymore. On 16:47-20130322, Guennadi Liakhovetski wrote: > On Tue, 29 Jan 2013, Shawn Guo wrote: > > > As multiplatform build is being adopted by more and more ARM platforms, > > initcall function should be used very carefully. For example, when > > GENERIC_CPUFREQ_CPU0 is built in the kernel, cpu0_cpufreq_driver_init() > > will be called on all the platforms to initialize cpufreq-cpu0 driver. > > > > To eliminate this undesired the effect, the patch changes cpufreq-cpu0 > > driver to have it instantiated as a platform_driver. Then it will only > > run on platforms that create the platform_device "cpufreq-cpu0". > > Sorry, confused. Before this used to be a generic cpufreq driver, usable > on all (DT-enabled only) platforms. You just had to provide an OPP table, > a clock, a regulator, similar to this > > http://thread.gmane.org/gmane.linux.kernel.cpufreq/9510/focus=9509 > > (also see the complete thread for more information). Now this won't work > obviously. Instead we now need a pseudo platform device to instantiate > cpufreq-cpu0. This device cannot be put in DT, because it doesn't describe > real hardware. So, we have to add register it from the board specific .c > code, which we actually want to get rid of... Is this really what we want? > > What about other cpufreq drivers? They have the same problem on > multiplatform builds, right? Say, s3c2416-cpufreq.c. It also just > initialises itself and starts looking for a clock, names "msysclk" with a > NULL device pointer etc. Don't we need a common approach for cpufreq > driver initialisation? > > The decision which cpufreq driver to use is SoC-specific, right? We're > unlikely to have several boards, using the same SoC, wishing to use > different cpufreq drivers? The decision _whether_ or not to enable it and > _which_ resources to use might be board-specific. So, how about adding a > cpufreq call something like > > cpufreq_driver_request("cpufreq-driver-name"); > > to be called by SoC-specific code. You can say it is not much different > from adding a virtual device, but firstly I think such a use of a platform > device is really an overkill. Secondly you still run a danger, that > several platforms, built into a single image, register several devices for > different cpufreq drivers, or even for one... With a special call you know > there can be only one and you return -EBUSY to all further calls to that > function. Just an note on this specific patch - I made this generic handling for OMAP OPP handling in my series: http://marc.info/?l=linux-omap&m=136371580826031&w=2 Handling non-DT and DT enabled boots are key for us as well here.
On Fri, Mar 22, 2013 at 04:47:17PM +0100, Guennadi Liakhovetski wrote: > Sorry, confused. Before this used to be a generic cpufreq driver, usable > on all (DT-enabled only) platforms. You just had to provide an OPP table, > a clock, a regulator, similar to this > > http://thread.gmane.org/gmane.linux.kernel.cpufreq/9510/focus=9509 > > (also see the complete thread for more information). Now this won't work > obviously. Instead we now need a pseudo platform device to instantiate > cpufreq-cpu0. This device cannot be put in DT, because it doesn't describe > real hardware. So, we have to add register it from the board specific .c > code, which we actually want to get rid of... Is this really what we want? > Yes, that's what we want. We are moving to multiplatform build, and we do not want the driver get instantiated on very single platform that is part of multiplatform. Some of them may not support cpufreq at all and others may have operating-points defined in device tree but need to work with platform specific cpufreq driver. In an ideal DT world, there shouldn't be any board specific .c code but only SoC specific code. Whether or not support cpufreq is a SoC specific decision, and it's perfectly fine to make the decision in SoC specific code. > What about other cpufreq drivers? They have the same problem on > multiplatform builds, right? Say, s3c2416-cpufreq.c. It also just > initialises itself and starts looking for a clock, names "msysclk" with a > NULL device pointer etc. Don't we need a common approach for cpufreq > driver initialisation? > The s3c2416 is not part of multiplatform build yet. And any platform that wants to be part of multiplatform build needs to fix those global initcalls. Two approaches are being used to do that. - Call of_machine_is_compatible() to check target platform at the beginning of the initcall, and return immediately if it's not the target platform. See drivers/cpufreq/highbank-cpufreq.c for example. - Remove global initcalls and use platform_device/driver mechanism for instantiation. See drivers/cpufreq/imx6q-cpufreq.c for example. I like the second one which is more scalable, and choose to use it for cpufreq-cpu0 driver. > The decision which cpufreq driver to use is SoC-specific, right? We're > unlikely to have several boards, using the same SoC, wishing to use > different cpufreq drivers? The decision _whether_ or not to enable it and > _which_ resources to use might be board-specific. So, how about adding a > cpufreq call something like > > cpufreq_driver_request("cpufreq-driver-name"); > > to be called by SoC-specific code. You can say it is not much different > from adding a virtual device, but firstly I think such a use of a platform > device is really an overkill. I do not see why this is an overkill. It's pretty common to use the approach for instantiating particular driver on particular target. > Secondly you still run a danger, that > several platforms, built into a single image, register several devices for > different cpufreq drivers, or even for one... With a special call you know > there can be only one and you return -EBUSY to all further calls to that > function. I do not see how this could happen. The cpufreq device gets added in target specific init function which will only be invoked when the kernel is running on this target. Check arch/arm/mach-imx/mach-imx6q.c or the OMAP example given by Nishanth to see how this should be done. Shawn
On Sat, 23 Mar 2013, Shawn Guo wrote: > On Fri, Mar 22, 2013 at 04:47:17PM +0100, Guennadi Liakhovetski wrote: [snip] > > Secondly you still run a danger, that > > several platforms, built into a single image, register several devices for > > different cpufreq drivers, or even for one... With a special call you know > > there can be only one and you return -EBUSY to all further calls to that > > function. > > I do not see how this could happen. The cpufreq device gets added in > target specific init function which will only be invoked when the kernel > is running on this target. Check arch/arm/mach-imx/mach-imx6q.c or the > OMAP example given by Nishanth to see how this should be done. Sorry, I meant buggy implementations, where an initcall is added without checking, whether it's running on supported hardware. Already before your patch for cpufreq-cpu0 to instantiate "mistakenly" on unsupported hardware you had to have an "operating-points" property in your "cpus" node, and you needed a clock attached to your cpu0 device. Do you really think this was likely? Anyway, I do find this an overkill and an abuse, but I'm not going to fight over it. If I'm the only one with this impression - no problem, just forget my ranting :) Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/
On Sun, 24 Mar 2013, Guennadi Liakhovetski wrote: > On Sat, 23 Mar 2013, Shawn Guo wrote: > > > On Fri, Mar 22, 2013 at 04:47:17PM +0100, Guennadi Liakhovetski wrote: > > [snip] > > > > Secondly you still run a danger, that > > > several platforms, built into a single image, register several devices for > > > different cpufreq drivers, or even for one... With a special call you know > > > there can be only one and you return -EBUSY to all further calls to that > > > function. > > > > I do not see how this could happen. The cpufreq device gets added in > > target specific init function which will only be invoked when the kernel > > is running on this target. Check arch/arm/mach-imx/mach-imx6q.c or the > > OMAP example given by Nishanth to see how this should be done. > > Sorry, I meant buggy implementations, where an initcall is added without > checking, whether it's running on supported hardware. > > Already before your patch for cpufreq-cpu0 to instantiate "mistakenly" on > unsupported hardware you had to have an "operating-points" property in > your "cpus" node, and you needed a clock attached to your cpu0 device. Do > you really think this was likely? > > Anyway, I do find this an overkill and an abuse, Look at this: in your now platform driver static int cpu0_cpufreq_probe(struct platform_device *pdev) { ... you get a DT node from "/cpus" with an "operating-points" property: for_each_child_of_node(of_find_node_by_path("/cpus"), np) { if (of_get_property(np, "operating-points", NULL)) break; } and then you patch your pseudo-, virtual platform device, that you're currently probing with this found DT node: cpu_dev = &pdev->dev; cpu_dev->of_node = np; huh... > but I'm not going to > fight over it. If I'm the only one with this impression - no problem, just > forget my ranting :) Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/
diff --git a/arch/arm/mach-omap2/board-generic.c b/arch/arm/mach-omap2/board-generic.c index 53cb380b..b945480 100644 --- a/arch/arm/mach-omap2/board-generic.c +++ b/arch/arm/mach-omap2/board-generic.c @@ -139,6 +139,7 @@ DT_MACHINE_START(AM33XX_DT, "Generic AM33XX (Flattened Device Tree)") .init_irq = omap_intc_of_init, .handle_irq = omap3_intc_handle_irq, .init_machine = omap_generic_init, + .init_late = am33xx_init_late, .timer = &omap3_am33xx_timer, .dt_compat = am33xx_boards_compat, MACHINE_END diff --git a/arch/arm/mach-omap2/cclock33xx_data.c b/arch/arm/mach-omap2/cclock33xx_data.c index ea64ad6..acb1620 100644 --- a/arch/arm/mach-omap2/cclock33xx_data.c +++ b/arch/arm/mach-omap2/cclock33xx_data.c @@ -850,7 +850,7 @@ static struct omap_clk am33xx_clks[] = { CLK(NULL, "dpll_core_m5_ck", &dpll_core_m5_ck, CK_AM33XX), CLK(NULL, "dpll_core_m6_ck", &dpll_core_m6_ck, CK_AM33XX), CLK(NULL, "dpll_mpu_ck", &dpll_mpu_ck, CK_AM33XX), - CLK("cpu0", NULL, &dpll_mpu_ck, CK_AM33XX), + CLK("cpufreq-cpu0.0", NULL, &dpll_mpu_ck, CK_AM33XX), CLK(NULL, "dpll_mpu_m2_ck", &dpll_mpu_m2_ck, CK_AM33XX), CLK(NULL, "dpll_ddr_ck", &dpll_ddr_ck, CK_AM33XX), CLK(NULL, "dpll_ddr_m2_ck", &dpll_ddr_m2_ck, CK_AM33XX), diff --git a/arch/arm/mach-omap2/common.h b/arch/arm/mach-omap2/common.h index 948bcaa..e3355df5 100644 --- a/arch/arm/mach-omap2/common.h +++ b/arch/arm/mach-omap2/common.h @@ -106,6 +106,7 @@ void omap2430_init_late(void); void omap3430_init_late(void); void omap35xx_init_late(void); void omap3630_init_late(void); +void am33xx_init_late(void); void am35xx_init_late(void); void ti81xx_init_late(void); void omap4430_init_late(void); diff --git a/arch/arm/mach-omap2/io.c b/arch/arm/mach-omap2/io.c index 2c3fdd6..7acfb8a 100644 --- a/arch/arm/mach-omap2/io.c +++ b/arch/arm/mach-omap2/io.c @@ -535,6 +535,13 @@ void __init omap3630_init_late(void) omap2_clk_enable_autoidle_all(); } +void __init am33xx_init_late(void) +{ + struct platform_device_info devinfo = { .name = "cpufreq-cpu0", }; + + platform_device_register_full(&devinfo); +} + void __init am35xx_init_late(void) { omap_mux_late_init(); diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig index ea512f4..774dc1c 100644 --- a/drivers/cpufreq/Kconfig +++ b/drivers/cpufreq/Kconfig @@ -180,7 +180,7 @@ config CPU_FREQ_GOV_CONSERVATIVE If in doubt, say N. config GENERIC_CPUFREQ_CPU0 - bool "Generic CPU0 cpufreq driver" + tristate "Generic CPU0 cpufreq driver" depends on HAVE_CLK && REGULATOR && PM_OPP && OF select CPU_FREQ_TABLE help diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c index 90e9d73..519c2f7 100644 --- a/drivers/cpufreq/cpufreq-cpu0.c +++ b/drivers/cpufreq/cpufreq-cpu0.c @@ -12,12 +12,12 @@ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt #include <linux/clk.h> -#include <linux/cpu.h> #include <linux/cpufreq.h> #include <linux/err.h> #include <linux/module.h> #include <linux/of.h> #include <linux/opp.h> +#include <linux/platform_device.h> #include <linux/regulator/consumer.h> #include <linux/slab.h> @@ -174,7 +174,7 @@ static struct cpufreq_driver cpu0_cpufreq_driver = { .attr = cpu0_cpufreq_attr, }; -static int cpu0_cpufreq_driver_init(void) +static int cpu0_cpufreq_probe(struct platform_device *pdev) { struct device_node *np; int ret; @@ -189,23 +189,17 @@ static int cpu0_cpufreq_driver_init(void) return -ENOENT; } - cpu_dev = get_cpu_device(0); - if (!cpu_dev) { - pr_err("failed to get cpu0 device\n"); - ret = -ENODEV; - goto out_put_node; - } - + cpu_dev = &pdev->dev; cpu_dev->of_node = np; - cpu_clk = clk_get(cpu_dev, NULL); + cpu_clk = devm_clk_get(cpu_dev, NULL); if (IS_ERR(cpu_clk)) { ret = PTR_ERR(cpu_clk); pr_err("failed to get cpu0 clock: %d\n", ret); goto out_put_node; } - cpu_reg = regulator_get(cpu_dev, "cpu0"); + cpu_reg = devm_regulator_get(cpu_dev, "cpu0"); if (IS_ERR(cpu_reg)) { pr_warn("failed to get cpu0 regulator\n"); cpu_reg = NULL; @@ -266,7 +260,24 @@ out_put_node: of_node_put(np); return ret; } -late_initcall(cpu0_cpufreq_driver_init); + +static int cpu0_cpufreq_remove(struct platform_device *pdev) +{ + cpufreq_unregister_driver(&cpu0_cpufreq_driver); + opp_free_cpufreq_table(cpu_dev, &freq_table); + + return 0; +} + +static struct platform_driver cpu0_cpufreq_platdrv = { + .driver = { + .name = "cpufreq-cpu0", + .owner = THIS_MODULE, + }, + .probe = cpu0_cpufreq_probe, + .remove = cpu0_cpufreq_remove, +}; +module_platform_driver(cpu0_cpufreq_platdrv); MODULE_AUTHOR("Shawn Guo <shawn.guo@linaro.org>"); MODULE_DESCRIPTION("Generic CPU0 cpufreq driver"); diff --git a/drivers/cpufreq/highbank-cpufreq.c b/drivers/cpufreq/highbank-cpufreq.c index 2ea6276..0491f1f 100644 --- a/drivers/cpufreq/highbank-cpufreq.c +++ b/drivers/cpufreq/highbank-cpufreq.c @@ -20,6 +20,7 @@ #include <linux/err.h> #include <linux/of.h> #include <linux/mailbox.h> +#include <linux/platform_device.h> #define HB_CPUFREQ_CHANGE_NOTE 0x80000001 #define HB_CPUFREQ_IPC_LEN 7 @@ -65,6 +66,7 @@ static struct notifier_block hb_cpufreq_clk_nb = { static int hb_cpufreq_driver_init(void) { + struct platform_device_info devinfo = { .name = "cpufreq-cpu0", }; struct device *cpu_dev; struct clk *cpu_clk; struct device_node *np; @@ -104,6 +106,9 @@ static int hb_cpufreq_driver_init(void) goto out_put_node; } + /* Instantiate cpufreq-cpu0 */ + platform_device_register_full(&devinfo); + out_put_node: of_node_put(np); return ret;
As multiplatform build is being adopted by more and more ARM platforms, initcall function should be used very carefully. For example, when GENERIC_CPUFREQ_CPU0 is built in the kernel, cpu0_cpufreq_driver_init() will be called on all the platforms to initialize cpufreq-cpu0 driver. To eliminate this undesired the effect, the patch changes cpufreq-cpu0 driver to have it instantiated as a platform_driver. Then it will only run on platforms that create the platform_device "cpufreq-cpu0". Along with the change, it also changes cpu_dev to be &pdev->dev, so that managed functions can start working, and module build gets supported too. The existing users of cpufreq-cpu0 driver highbank and am33xx are also updated accordingly to adapt the changes. Signed-off-by: Shawn Guo <shawn.guo@linaro.org> Cc: Mark Langsdorf <mark.langsdorf@calxeda.com> Cc: AnilKumar Ch <anilkumar@ti.com> Cc: Tony Lindgren <tony@atomide.com> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> --- Changes since v1: * Migrate cpufreq-cpu0 users in the same patch Rafael, The patch is based on Mark's highbank-cpufreq series and Nishanth's "PM / OPP : export symbol consolidation" sereis. Mark, AnilKumar, I only compile-tested it on highbank and omap2. Please give it a test no hardware to make sure cpufreq-cpu0 still works for you. Thanks. Shawn arch/arm/mach-omap2/board-generic.c | 1 + arch/arm/mach-omap2/cclock33xx_data.c | 2 +- arch/arm/mach-omap2/common.h | 1 + arch/arm/mach-omap2/io.c | 7 +++++++ drivers/cpufreq/Kconfig | 2 +- drivers/cpufreq/cpufreq-cpu0.c | 35 ++++++++++++++++++++++----------- drivers/cpufreq/highbank-cpufreq.c | 5 +++++ 7 files changed, 39 insertions(+), 14 deletions(-)