Re: [PATCH v1 10/11] cpufreq: tegra20: Wrap cpufreq into platform driver
On 18.05.2018 12:07, Thierry Reding wrote: > On Thu, May 17, 2018 at 09:00:55PM +0300, Dmitry Osipenko wrote: >> Currently tegra20-cpufreq kernel module isn't getting autoloaded because >> there is no device associated with the module, this is one of two patches >> that resolves the module autoloading. This patch adds a module alias that >> will associate the tegra20-cpufreq kernel module with the platform device, >> other patch will instantiate the actual platform device. And now it makes >> sense to wrap cpufreq driver into a platform driver for consistency. >> >> Signed-off-by: Dmitry Osipenko >> --- >> drivers/cpufreq/tegra20-cpufreq.c | 116 +++--- >> 1 file changed, 73 insertions(+), 43 deletions(-) >> >> diff --git a/drivers/cpufreq/tegra20-cpufreq.c >> b/drivers/cpufreq/tegra20-cpufreq.c >> index c0a7b5a78aa6..f9d02a28df9e 100644 >> --- a/drivers/cpufreq/tegra20-cpufreq.c >> +++ b/drivers/cpufreq/tegra20-cpufreq.c >> @@ -19,7 +19,7 @@ >> #include >> #include >> #include >> -#include >> +#include >> >> static struct cpufreq_frequency_table freq_table[] = { >> { .frequency = 216000 }, >> @@ -33,15 +33,19 @@ static struct cpufreq_frequency_table freq_table[] = { >> { .frequency = CPUFREQ_TABLE_END }, >> }; >> >> -static struct clk *cpu_clk; >> -static struct clk *pll_x_clk; >> -static struct clk *pll_p_clk; >> -static bool pll_x_prepared; >> +struct tegra20_cpufreq_data { > > Nit: I'm not a big fan of _data suffixes because they are completely > redundant. Any data structure by definition hosts data, so I'd just drop > that. Okay, I'll drop it in v2. > [...] >> @@ -152,55 +161,76 @@ static struct cpufreq_driver tegra_cpufreq_driver = { >> .suspend= cpufreq_generic_suspend, >> }; >> >> -static int __init tegra_cpufreq_init(void) >> +static int tegra20_cpufreq_probe(struct platform_device *pdev) >> { >> +struct tegra20_cpufreq_data *data; >> int err; >> >> -if (!of_machine_is_compatible("nvidia,tegra20")) >> -return -ENODEV; >> +data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); >> +if (!data) >> +return -ENOMEM; >> >> -cpu_clk = clk_get_sys(NULL, "cclk"); >> -if (IS_ERR(cpu_clk)) >> -return PTR_ERR(cpu_clk); >> +data->cpu_clk = clk_get_sys(NULL, "cclk"); >> +if (IS_ERR(data->cpu_clk)) >> +return PTR_ERR(data->cpu_clk); >> >> -pll_x_clk = clk_get_sys(NULL, "pll_x"); >> -if (IS_ERR(pll_x_clk)) { >> -err = PTR_ERR(pll_x_clk); >> +data->pll_x_clk = clk_get_sys(NULL, "pll_x"); >> +if (IS_ERR(data->pll_x_clk)) { >> +err = PTR_ERR(data->pll_x_clk); >> goto put_cpu; >> } >> >> -pll_p_clk = clk_get_sys(NULL, "pll_p"); >> -if (IS_ERR(pll_p_clk)) { >> -err = PTR_ERR(pll_p_clk); >> +data->pll_p_clk = clk_get_sys(NULL, "pll_p"); >> +if (IS_ERR(data->pll_p_clk)) { >> +err = PTR_ERR(data->pll_p_clk); >> goto put_pll_x; >> } >> >> +data->dev = &pdev->dev; >> + >> +tegra_cpufreq_driver.driver_data = data; > > Couldn't this be embedded into struct tegra20_cpufreq_data? Moving > everything but this into a per-device data structure seems half-baked. That's a good suggestions, thank you.
Re: [PATCH v1 10/11] cpufreq: tegra20: Wrap cpufreq into platform driver
On Thu, May 17, 2018 at 09:00:55PM +0300, Dmitry Osipenko wrote: > Currently tegra20-cpufreq kernel module isn't getting autoloaded because > there is no device associated with the module, this is one of two patches > that resolves the module autoloading. This patch adds a module alias that > will associate the tegra20-cpufreq kernel module with the platform device, > other patch will instantiate the actual platform device. And now it makes > sense to wrap cpufreq driver into a platform driver for consistency. > > Signed-off-by: Dmitry Osipenko > --- > drivers/cpufreq/tegra20-cpufreq.c | 116 +++--- > 1 file changed, 73 insertions(+), 43 deletions(-) > > diff --git a/drivers/cpufreq/tegra20-cpufreq.c > b/drivers/cpufreq/tegra20-cpufreq.c > index c0a7b5a78aa6..f9d02a28df9e 100644 > --- a/drivers/cpufreq/tegra20-cpufreq.c > +++ b/drivers/cpufreq/tegra20-cpufreq.c > @@ -19,7 +19,7 @@ > #include > #include > #include > -#include > +#include > > static struct cpufreq_frequency_table freq_table[] = { > { .frequency = 216000 }, > @@ -33,15 +33,19 @@ static struct cpufreq_frequency_table freq_table[] = { > { .frequency = CPUFREQ_TABLE_END }, > }; > > -static struct clk *cpu_clk; > -static struct clk *pll_x_clk; > -static struct clk *pll_p_clk; > -static bool pll_x_prepared; > +struct tegra20_cpufreq_data { Nit: I'm not a big fan of _data suffixes because they are completely redundant. Any data structure by definition hosts data, so I'd just drop that. [...] > @@ -152,55 +161,76 @@ static struct cpufreq_driver tegra_cpufreq_driver = { > .suspend= cpufreq_generic_suspend, > }; > > -static int __init tegra_cpufreq_init(void) > +static int tegra20_cpufreq_probe(struct platform_device *pdev) > { > + struct tegra20_cpufreq_data *data; > int err; > > - if (!of_machine_is_compatible("nvidia,tegra20")) > - return -ENODEV; > + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > > - cpu_clk = clk_get_sys(NULL, "cclk"); > - if (IS_ERR(cpu_clk)) > - return PTR_ERR(cpu_clk); > + data->cpu_clk = clk_get_sys(NULL, "cclk"); > + if (IS_ERR(data->cpu_clk)) > + return PTR_ERR(data->cpu_clk); > > - pll_x_clk = clk_get_sys(NULL, "pll_x"); > - if (IS_ERR(pll_x_clk)) { > - err = PTR_ERR(pll_x_clk); > + data->pll_x_clk = clk_get_sys(NULL, "pll_x"); > + if (IS_ERR(data->pll_x_clk)) { > + err = PTR_ERR(data->pll_x_clk); > goto put_cpu; > } > > - pll_p_clk = clk_get_sys(NULL, "pll_p"); > - if (IS_ERR(pll_p_clk)) { > - err = PTR_ERR(pll_p_clk); > + data->pll_p_clk = clk_get_sys(NULL, "pll_p"); > + if (IS_ERR(data->pll_p_clk)) { > + err = PTR_ERR(data->pll_p_clk); > goto put_pll_x; > } > > + data->dev = &pdev->dev; > + > + tegra_cpufreq_driver.driver_data = data; Couldn't this be embedded into struct tegra20_cpufreq_data? Moving everything but this into a per-device data structure seems half-baked. Thierry signature.asc Description: PGP signature
Re: [PATCH v1 10/11] cpufreq: tegra20: Wrap cpufreq into platform driver
On 18-05-18, 11:09, Dmitry Osipenko wrote: > On 18.05.2018 05:07, Viresh Kumar wrote: > > On 17-05-18, 21:00, Dmitry Osipenko wrote: > >> -static int __init tegra_cpufreq_init(void) > >> +static int tegra20_cpufreq_probe(struct platform_device *pdev) > >> { > >> + struct tegra20_cpufreq_data *data; > >>int err; > >> > >> - if (!of_machine_is_compatible("nvidia,tegra20")) > >> - return -ENODEV; > > > > So this stuff wasn't really required as you are getting rid of that in > > the same series. Should we really add it then ? Maybe .. > > > > It's not strictly needed, but I'd prefer to keep that stuff for clarity as it > kinda shows the way that led to the final result. Okay. -- viresh
Re: [PATCH v1 10/11] cpufreq: tegra20: Wrap cpufreq into platform driver
On 18.05.2018 05:07, Viresh Kumar wrote: > On 17-05-18, 21:00, Dmitry Osipenko wrote: >> -static int __init tegra_cpufreq_init(void) >> +static int tegra20_cpufreq_probe(struct platform_device *pdev) >> { >> +struct tegra20_cpufreq_data *data; >> int err; >> >> -if (!of_machine_is_compatible("nvidia,tegra20")) >> -return -ENODEV; > > So this stuff wasn't really required as you are getting rid of that in > the same series. Should we really add it then ? Maybe .. > It's not strictly needed, but I'd prefer to keep that stuff for clarity as it kinda shows the way that led to the final result. [snip]
Re: [PATCH v1 10/11] cpufreq: tegra20: Wrap cpufreq into platform driver
On 17-05-18, 21:00, Dmitry Osipenko wrote: > -static int __init tegra_cpufreq_init(void) > +static int tegra20_cpufreq_probe(struct platform_device *pdev) > { > + struct tegra20_cpufreq_data *data; > int err; > > - if (!of_machine_is_compatible("nvidia,tegra20")) > - return -ENODEV; So this stuff wasn't really required as you are getting rid of that in the same series. Should we really add it then ? Maybe .. > + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > > - cpu_clk = clk_get_sys(NULL, "cclk"); > - if (IS_ERR(cpu_clk)) > - return PTR_ERR(cpu_clk); > + data->cpu_clk = clk_get_sys(NULL, "cclk"); > + if (IS_ERR(data->cpu_clk)) > + return PTR_ERR(data->cpu_clk); > > - pll_x_clk = clk_get_sys(NULL, "pll_x"); > - if (IS_ERR(pll_x_clk)) { > - err = PTR_ERR(pll_x_clk); > + data->pll_x_clk = clk_get_sys(NULL, "pll_x"); > + if (IS_ERR(data->pll_x_clk)) { > + err = PTR_ERR(data->pll_x_clk); > goto put_cpu; > } > > - pll_p_clk = clk_get_sys(NULL, "pll_p"); > - if (IS_ERR(pll_p_clk)) { > - err = PTR_ERR(pll_p_clk); > + data->pll_p_clk = clk_get_sys(NULL, "pll_p"); > + if (IS_ERR(data->pll_p_clk)) { > + err = PTR_ERR(data->pll_p_clk); > goto put_pll_x; > } > > + data->dev = &pdev->dev; > + > + tegra_cpufreq_driver.driver_data = data; > + > err = cpufreq_register_driver(&tegra_cpufreq_driver); > if (err) > goto put_pll_p; > > + platform_set_drvdata(pdev, data); > + > return 0; > > put_pll_p: > - clk_put(pll_p_clk); > + clk_put(data->pll_p_clk); > put_pll_x: > - clk_put(pll_x_clk); > + clk_put(data->pll_x_clk); > put_cpu: > - clk_put(cpu_clk); > + clk_put(data->cpu_clk); > > return err; > } > > -static void __exit tegra_cpufreq_exit(void) > +static int tegra20_cpufreq_remove(struct platform_device *pdev) > { > + struct tegra20_cpufreq_data *data = platform_get_drvdata(pdev); > + > cpufreq_unregister_driver(&tegra_cpufreq_driver); > - clk_put(pll_p_clk); > - clk_put(pll_x_clk); > - clk_put(cpu_clk); > + > + clk_put(data->pll_p_clk); > + clk_put(data->pll_x_clk); > + clk_put(data->cpu_clk); > + > + return 0; > } > > +static struct platform_driver tegra20_cpufreq_driver = { > + .probe = tegra20_cpufreq_probe, > + .remove = tegra20_cpufreq_remove, > + .driver = { > + .name = "tegra20-cpufreq", > + }, > +}; > +module_platform_driver(tegra20_cpufreq_driver); > + > MODULE_AUTHOR("Colin Cross "); > +MODULE_ALIAS("platform:tegra20-cpufreq"); > MODULE_DESCRIPTION("NVIDIA Tegra20 cpufreq driver"); > MODULE_LICENSE("GPL"); > -module_init(tegra_cpufreq_init); > -module_exit(tegra_cpufreq_exit); Acked-by: Viresh Kumar -- viresh
[PATCH v1 10/11] cpufreq: tegra20: Wrap cpufreq into platform driver
Currently tegra20-cpufreq kernel module isn't getting autoloaded because there is no device associated with the module, this is one of two patches that resolves the module autoloading. This patch adds a module alias that will associate the tegra20-cpufreq kernel module with the platform device, other patch will instantiate the actual platform device. And now it makes sense to wrap cpufreq driver into a platform driver for consistency. Signed-off-by: Dmitry Osipenko --- drivers/cpufreq/tegra20-cpufreq.c | 116 +++--- 1 file changed, 73 insertions(+), 43 deletions(-) diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c index c0a7b5a78aa6..f9d02a28df9e 100644 --- a/drivers/cpufreq/tegra20-cpufreq.c +++ b/drivers/cpufreq/tegra20-cpufreq.c @@ -19,7 +19,7 @@ #include #include #include -#include +#include static struct cpufreq_frequency_table freq_table[] = { { .frequency = 216000 }, @@ -33,15 +33,19 @@ static struct cpufreq_frequency_table freq_table[] = { { .frequency = CPUFREQ_TABLE_END }, }; -static struct clk *cpu_clk; -static struct clk *pll_x_clk; -static struct clk *pll_p_clk; -static bool pll_x_prepared; +struct tegra20_cpufreq_data { + struct device *dev; + struct clk *cpu_clk; + struct clk *pll_x_clk; + struct clk *pll_p_clk; + bool pll_x_prepared; +}; static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy, unsigned int index) { - unsigned int ifreq = clk_get_rate(pll_p_clk) / 1000; + struct tegra20_cpufreq_data *data = cpufreq_get_driver_data(); + unsigned int ifreq = clk_get_rate(data->pll_p_clk) / 1000; /* * Don't switch to intermediate freq if: @@ -57,6 +61,7 @@ static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy, static int tegra_target_intermediate(struct cpufreq_policy *policy, unsigned int index) { + struct tegra20_cpufreq_data *data = cpufreq_get_driver_data(); int ret; /* @@ -69,21 +74,22 @@ static int tegra_target_intermediate(struct cpufreq_policy *policy, * Also, we wouldn't be using pll_x anymore and must not take extra * reference to it, as it can be disabled now to save some power. */ - clk_prepare_enable(pll_x_clk); + clk_prepare_enable(data->pll_x_clk); - ret = clk_set_parent(cpu_clk, pll_p_clk); + ret = clk_set_parent(data->cpu_clk, data->pll_p_clk); if (ret) - clk_disable_unprepare(pll_x_clk); + clk_disable_unprepare(data->pll_x_clk); else - pll_x_prepared = true; + data->pll_x_prepared = true; return ret; } static int tegra_target(struct cpufreq_policy *policy, unsigned int index) { + struct tegra20_cpufreq_data *data = cpufreq_get_driver_data(); unsigned long rate = freq_table[index].frequency; - unsigned int ifreq = clk_get_rate(pll_p_clk) / 1000; + unsigned int ifreq = clk_get_rate(data->pll_p_clk) / 1000; int ret; /* @@ -91,14 +97,14 @@ static int tegra_target(struct cpufreq_policy *policy, unsigned int index) * as it isn't used anymore. */ if (rate == ifreq) - return clk_set_parent(cpu_clk, pll_p_clk); + return clk_set_parent(data->cpu_clk, data->pll_p_clk); - ret = clk_set_rate(pll_x_clk, rate * 1000); + ret = clk_set_rate(data->pll_x_clk, rate * 1000); /* Restore to earlier frequency on error, i.e. pll_x */ if (ret) - pr_err("Failed to change pll_x to %lu\n", rate); + dev_err(data->dev, "Failed to change pll_x to %lu\n", rate); - ret = clk_set_parent(cpu_clk, pll_x_clk); + ret = clk_set_parent(data->cpu_clk, data->pll_x_clk); /* This shouldn't fail while changing or restoring */ WARN_ON(ret); @@ -106,9 +112,9 @@ static int tegra_target(struct cpufreq_policy *policy, unsigned int index) * Drop count to pll_x clock only if we switched to intermediate freq * earlier while transitioning to a target frequency. */ - if (pll_x_prepared) { - clk_disable_unprepare(pll_x_clk); - pll_x_prepared = false; + if (data->pll_x_prepared) { + clk_disable_unprepare(data->pll_x_clk); + data->pll_x_prepared = false; } return ret; @@ -116,25 +122,28 @@ static int tegra_target(struct cpufreq_policy *policy, unsigned int index) static int tegra_cpu_init(struct cpufreq_policy *policy) { + struct tegra20_cpufreq_data *data = cpufreq_get_driver_data(); int ret; - clk_prepare_enable(cpu_clk); + clk_prepare_enable(data->cpu_clk); /* FIXME: what's the actual transition time? */ ret = cpufreq_generic_init(policy, freq_ta