| Message ID | 5800397.rNERvs0XYE@wuerfel (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 24-03-16, 14:36, Arnd Bergmann wrote: > I've tried implementing something similar when we last discussed it, > but didn't get far enough to test it our properly, and I had trouble > integrating some of the more complex platforms (omap, imx, sunxi) > that have lots of root compatible strings to fit in nicely. > > I think having a lookup table as in your patch 2 makes sense here, > but I still would prefer not having a device at all. See the patch We don't have any other option really. The platform-device logic was added to make -EPROBE-DEFER work :) > Another idea I had was to make dt_cpufreq_init() a global function > that can simply be called by platforms that pass non-NULL platform > data today, avoiding the need for the pdata in a global location. I think its high time that we kill platform data if we don't care about old DT compatibility with new kernel for mvebu platform, if it hasn't gone into product already. We can do everything with opp-v2 now, and pdata doesn't do anything special. > diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c > index f951f911786e..b3817cc86597 100644 > --- a/drivers/cpufreq/cpufreq-dt.c > +++ b/drivers/cpufreq/cpufreq-dt.c > @@ -356,7 +356,7 @@ static struct cpufreq_driver dt_cpufreq_driver = { > .suspend = cpufreq_generic_suspend, > }; > > -static int dt_cpufreq_probe(struct platform_device *pdev) > +static int (void *data) dt_cpufreq_init ? :) > { > int ret; > > @@ -371,15 +371,22 @@ static int dt_cpufreq_probe(struct platform_device *pdev) > if (ret) > return ret; > > - dt_cpufreq_driver.driver_data = dev_get_platdata(&pdev->dev); > + dt_cpufreq_driver.driver_data = data; > > ret = cpufreq_register_driver(&dt_cpufreq_driver); > if (ret) > - dev_err(&pdev->dev, "failed register driver: %d\n", ret); > + pr_err("failed register driver: %d\n", ret); > > return ret; > } > > +/* a minimal fake platform device for platforms that still call > + * platform_device_create(). Don't use */ > +static int dt_cpufreq_probe(struct platform_device *pdev) > +{ > + return dt_cpufreq_init(dev_get_platdata(&pdev->dev)); > +} > + > static int dt_cpufreq_remove(struct platform_device *pdev) > { > cpufreq_unregister_driver(&dt_cpufreq_driver); > @@ -393,7 +400,24 @@ static struct platform_driver dt_cpufreq_platdrv = { > .probe = dt_cpufreq_probe, > .remove = dt_cpufreq_remove, > }; > -module_platform_driver(dt_cpufreq_platdrv); > + > +static int __init cpufreq_dt_init(void) > +{ > + if (of_machine_is_compatible("marvell,berlin")) > + return dt_cpufreq_init(NULL); > + > + return platform_driver_register(&dt_cpufreq_platdrv); > +} > +module_init(cpufreq_dt_init); > + > +static void cpufreq_dt_exit(void) > +{ > + if (dt_cpufreq_platdrv.driver.bus) I didn't like this as well :(
On Thursday 24 March 2016 19:36:00 Viresh Kumar wrote: > On 24-03-16, 14:36, Arnd Bergmann wrote: > > I've tried implementing something similar when we last discussed it, > > but didn't get far enough to test it our properly, and I had trouble > > integrating some of the more complex platforms (omap, imx, sunxi) > > that have lots of root compatible strings to fit in nicely. > > > > I think having a lookup table as in your patch 2 makes sense here, > > but I still would prefer not having a device at all. See the patch > > We don't have any other option really. The platform-device logic was > added to make -EPROBE-DEFER work :) Hmm, can you give an example where this is needed? I assume you are thinking of some clock controller or regulator that gets initialized after the cpufreq driver, right? Are there any that are in loadable modules, rather than just later in the init sequence for built-in drivers? > > Another idea I had was to make dt_cpufreq_init() a global function > > that can simply be called by platforms that pass non-NULL platform > > data today, avoiding the need for the pdata in a global location. > > I think its high time that we kill platform data if we don't care > about old DT compatibility with new kernel for mvebu platform, if it > hasn't gone into product already. We can do everything with opp-v2 > now, and pdata doesn't do anything special. I think we do care about compatibility there, but there are clearly other ways to handle the special case for opp-v1 if we want to. > > diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c > > index f951f911786e..b3817cc86597 100644 > > --- a/drivers/cpufreq/cpufreq-dt.c > > +++ b/drivers/cpufreq/cpufreq-dt.c > > @@ -356,7 +356,7 @@ static struct cpufreq_driver dt_cpufreq_driver = { > > .suspend = cpufreq_generic_suspend, > > }; > > > > -static int dt_cpufreq_probe(struct platform_device *pdev) > > +static int (void *data) > > dt_cpufreq_init ? :) Yes, I must have removed the string from my email when copying the text above. Arnd -- 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 24-03-16, 15:16, Arnd Bergmann wrote: > Hmm, can you give an example where this is needed? I don't remember them, but I am quite sure that I have seen people asking for that support again and again. > I assume you are > thinking of some clock controller or regulator that gets initialized > after the cpufreq driver, right? Yeah. > Are there any that are in loadable modules, rather than just later > in the init sequence for built-in drivers? Maybe both, don't remember. Sorry. Over that, independent of what the users are doing today, I think its important to support defer probing for anyone that wants to use it. > I think we do care about compatibility there, but there are clearly > other ways to handle the special case for opp-v1 if we want to. Okay, and what are those ? There is only one platform which has used the pdata, mvebu.
Ping !! On 25-03-16, 10:11, Viresh Kumar wrote: > On 24-03-16, 15:16, Arnd Bergmann wrote: > > Hmm, can you give an example where this is needed? > > I don't remember them, but I am quite sure that I have seen people > asking for that support again and again. > > > I assume you are > > thinking of some clock controller or regulator that gets initialized > > after the cpufreq driver, right? > > Yeah. > > > Are there any that are in loadable modules, rather than just later > > in the init sequence for built-in drivers? > > Maybe both, don't remember. Sorry. > > Over that, independent of what the users are doing today, I think its > important to support defer probing for anyone that wants to use it. > > > I think we do care about compatibility there, but there are clearly > > other ways to handle the special case for opp-v1 if we want to. > > Okay, and what are those ? There is only one platform which has used > the pdata, mvebu.
On Friday 25 March 2016 10:11:24 Viresh Kumar wrote: > On 24-03-16, 15:16, Arnd Bergmann wrote: > > Hmm, can you give an example where this is needed? > > I don't remember them, but I am quite sure that I have seen people > asking for that support again and again. > > > I assume you are > > thinking of some clock controller or regulator that gets initialized > > after the cpufreq driver, right? > > Yeah. > > > Are there any that are in loadable modules, rather than just later > > in the init sequence for built-in drivers? > > Maybe both, don't remember. Sorry. > > Over that, independent of what the users are doing today, I think its > important to support defer probing for anyone that wants to use it. Ok. > > I think we do care about compatibility there, but there are clearly > > other ways to handle the special case for opp-v1 if we want to. > > Okay, and what are those ? There is only one platform which has used > the pdata, mvebu. The platform already calls dev_pm_opp_add(), so it directly interacts with the cpufreq infrastructure. One way to do it would be to add another function call to set the independent_clocks flag in a static variable owned by that. Another way would be to have a separate function call for doing dev_pm_opp_add() with independent clocks. As far as I can tell, the two (dev_pm_opp_add and independent_clocks) are already tightly coupled interfaces, so it makes sense to combine them. Arnd -- 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 29-03-16, 15:22, Arnd Bergmann wrote: > The platform already calls dev_pm_opp_add(), so it directly interacts > with the cpufreq infrastructure. One way to do it would be to add > another function call to set the independent_clocks flag in a static > variable owned by that. Another way would be to have a separate > function call for doing dev_pm_opp_add() with independent clocks. > As far as I can tell, the two (dev_pm_opp_add and independent_clocks) > are already tightly coupled interfaces, so it makes sense to > combine them. That looks like a good idea. Apart from that I hope you don't have any issues with rest of the V2 series.
diff --git a/arch/arm/mach-berlin/berlin.c b/arch/arm/mach-berlin/berlin.c index 25d73870ccca..ac181c6797ee 100644 --- a/arch/arm/mach-berlin/berlin.c +++ b/arch/arm/mach-berlin/berlin.c @@ -18,11 +18,6 @@ #include <asm/hardware/cache-l2x0.h> #include <asm/mach/arch.h> -static void __init berlin_init_late(void) -{ - platform_device_register_simple("cpufreq-dt", -1, NULL, 0); -} - static const char * const berlin_dt_compat[] = { "marvell,berlin", NULL, @@ -30,7 +25,6 @@ static const char * const berlin_dt_compat[] = { DT_MACHINE_START(BERLIN_DT, "Marvell Berlin") .dt_compat = berlin_dt_compat, - .init_late = berlin_init_late, /* * with DT probing for L2CCs, berlin_init_machine can be removed. * Note: 88DE3005 (Armada 1500-mini) uses pl310 l2cc diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c index f951f911786e..b3817cc86597 100644 --- a/drivers/cpufreq/cpufreq-dt.c +++ b/drivers/cpufreq/cpufreq-dt.c @@ -356,7 +356,7 @@ static struct cpufreq_driver dt_cpufreq_driver = { .suspend = cpufreq_generic_suspend, }; -static int dt_cpufreq_probe(struct platform_device *pdev) +static int (void *data) { int ret; @@ -371,15 +371,22 @@ static int dt_cpufreq_probe(struct platform_device *pdev) if (ret) return ret; - dt_cpufreq_driver.driver_data = dev_get_platdata(&pdev->dev); + dt_cpufreq_driver.driver_data = data; ret = cpufreq_register_driver(&dt_cpufreq_driver); if (ret) - dev_err(&pdev->dev, "failed register driver: %d\n", ret); + pr_err("failed register driver: %d\n", ret); return ret; } +/* a minimal fake platform device for platforms that still call + * platform_device_create(). Don't use */ +static int dt_cpufreq_probe(struct platform_device *pdev) +{ + return dt_cpufreq_init(dev_get_platdata(&pdev->dev)); +} + static int dt_cpufreq_remove(struct platform_device *pdev) { cpufreq_unregister_driver(&dt_cpufreq_driver); @@ -393,7 +400,24 @@ static struct platform_driver dt_cpufreq_platdrv = { .probe = dt_cpufreq_probe, .remove = dt_cpufreq_remove, }; -module_platform_driver(dt_cpufreq_platdrv); + +static int __init cpufreq_dt_init(void) +{ + if (of_machine_is_compatible("marvell,berlin")) + return dt_cpufreq_init(NULL); + + return platform_driver_register(&dt_cpufreq_platdrv); +} +module_init(cpufreq_dt_init); + +static void cpufreq_dt_exit(void) +{ + if (dt_cpufreq_platdrv.driver.bus) + platform_driver_unregister(&dt_cpufreq_platdrv); + else + cpufreq_unregister_driver(&dt_cpufreq_driver); +} +module_exit(cpufreq_dt_exit); MODULE_ALIAS("platform:cpufreq-dt"); MODULE_AUTHOR("Viresh Kumar <viresh.kumar@linaro.org>");