Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124
On Thu, Jul 24, 2014 at 05:43:40AM +0530, Viresh Kumar wrote:
> On 24 July 2014 00:47, Tuomas Tynkkynen wrote:
> > It's this:
> >
> > +static int tegra124_cpufreq_probe(struct platform_device *pdev)
> > +{
> > [...]
> > +
> > + dfll_clk = of_clk_get_by_name(cpu_dev->of_node, "dfll");
> > + if (IS_ERR(dfll_clk)) {
> > + ret = PTR_ERR(dfll_clk);
> > + goto out_put_cpu_clk;
> > + }
>
> This would search for clocks passed via DT, right? Why would we
> get EPROBE_DEFER for that? Sorry for the stupid question.
of_clk_get_by_name() can return -EPROBE_DEFER, which will happen if the
clock provider hasn't been registered yet.
Thierry
pgpvB32qZjywd.pgp
Description: PGP signature
Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124
On Thu, Jul 24, 2014 at 05:43:40AM +0530, Viresh Kumar wrote:
On 24 July 2014 00:47, Tuomas Tynkkynen ttynkky...@nvidia.com wrote:
It's this:
+static int tegra124_cpufreq_probe(struct platform_device *pdev)
+{
[...]
+
+ dfll_clk = of_clk_get_by_name(cpu_dev-of_node, dfll);
+ if (IS_ERR(dfll_clk)) {
+ ret = PTR_ERR(dfll_clk);
+ goto out_put_cpu_clk;
+ }
This would search for clocks passed via DT, right? Why would we
get EPROBE_DEFER for that? Sorry for the stupid question.
of_clk_get_by_name() can return -EPROBE_DEFER, which will happen if the
clock provider hasn't been registered yet.
Thierry
pgpvB32qZjywd.pgp
Description: PGP signature
Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124
On 24 July 2014 00:47, Tuomas Tynkkynen wrote:
> It's this:
>
> +static int tegra124_cpufreq_probe(struct platform_device *pdev)
> +{
> [...]
> +
> + dfll_clk = of_clk_get_by_name(cpu_dev->of_node, "dfll");
> + if (IS_ERR(dfll_clk)) {
> + ret = PTR_ERR(dfll_clk);
> + goto out_put_cpu_clk;
> + }
This would search for clocks passed via DT, right? Why would we
get EPROBE_DEFER for that? Sorry for the stupid question.
--
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124
On 23/07/14 19:50, Viresh Kumar wrote:
> On 23 July 2014 17:27, Tuomas Tynkkynen wrote:
>> The platform device is required for the deferred probe that can happen if the
>> DFLL driver hasn't initialized yet, and module_init() callbacks don't seem to
>> respect -EPROBE_DEFER.
>
> Oh, which call in this file will return EPROBE_DEFER? I couldn't make out
> which one will depend on DFLL driver.
>
It's this:
+static int tegra124_cpufreq_probe(struct platform_device *pdev)
+{
[...]
+
+ dfll_clk = of_clk_get_by_name(cpu_dev->of_node, "dfll");
+ if (IS_ERR(dfll_clk)) {
+ ret = PTR_ERR(dfll_clk);
+ goto out_put_cpu_clk;
+ }
--
nvpublic
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124
On 23 July 2014 17:27, Tuomas Tynkkynen wrote: > The platform device is required for the deferred probe that can happen if the > DFLL driver hasn't initialized yet, and module_init() callbacks don't seem to > respect -EPROBE_DEFER. Oh, which call in this file will return EPROBE_DEFER? I couldn't make out which one will depend on DFLL driver. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124
On Wed, Jul 23, 2014 at 03:35:46PM +0300, Tuomas Tynkkynen wrote:
> On 23/07/14 10:09, Thierry Reding wrote:
> > On Mon, Jul 21, 2014 at 06:39:00PM +0300, Tuomas Tynkkynen wrote:
[...]
> >> diff --git a/drivers/cpufreq/tegra124-cpufreq.c
> >> b/drivers/cpufreq/tegra124-cpufreq.c
[...]
> >> + cpu_clk = of_clk_get_by_name(cpu_dev->of_node, "cpu_g");
> >> + if (IS_ERR(cpu_clk))
> >> + return PTR_ERR(cpu_clk);
[...]
> >> + pllp_clk = of_clk_get_by_name(cpu_dev->of_node, "pll_p");
> >> + if (IS_ERR(pllp_clk)) {
> >> + ret = PTR_ERR(pllp_clk);
> >> + goto out_put_pllx_clk;
> >> + }
> >
> > Can the above not be devm_clk_get(cpu_dev, "...") so that you can remove
> > all the clk_put() calls in the cleanup code below?
>
> That would allocate the clks under the cpu_dev's devres list, i.e. all the
> clk_puts wouldn't happen when the cpufreq driver goes away, but only when
> cpu_dev itself goes away.
I don't think so. devres_release_all() is called on driver detach as
well.
> > But is there even a reason why we need that? Couldn't we make the
> > driver's .remove() undo what .probe() did so that the driver can be
> > unloaded?
>
> I guess that could be done, though to fully undo everything the regulator
> voltage would also need to be saved/restored.
That would certainly be my prefered approach. that way the driver can
simply be unloaded, leaving the CPU in the same state as it was after
boot.
> > Otherwise it probably makes more sense not to use a driver (and dummy
> > device) at all as Viresh already mentioned.
> >
>
> The dummy platform device is only required for probe deferral, if that
> could be solved in a different way then yes.
I don't think it can. Probe deferral is pretty closely tied to devices
so it's unlikely to ever get implemented for regular initcalls. And in
this case I really think making the driver removable is a good thing.
> >> +};
> >> +
> >> +static const struct of_device_id soc_of_matches[] = {
> >> + { .compatible = "nvidia,tegra124", },
> >> + {}
> >> +};
> >> +
> >> +static int __init tegra_cpufreq_init(void)
> >> +{
> >> + int ret;
> >> + struct platform_device *pdev;
> >> +
> >> + if (!of_find_matching_node(NULL, soc_of_matches))
> >> + return -ENODEV;
> >
> > I think this could be of_machine_is_compatible() since there's only a
> > single entry in the match table. If there's a good chance that we may
> > end up with more entries, perhaps now would be a good time to add an
> > of_match_machine() function?
>
> I think this driver should work on Tegra132 without modifications.
> of_match_machine() does sound useful for some of the other cpufreq
> drivers as well and likely for your soc_is_tegra() from the PMC
> series as well.
Yes, indeed. I'll give it a shot if you don't beat me to it with this
series.
Thierry
pgplM2WI6Ijtv.pgp
Description: PGP signature
Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124
On Wed, Jul 23, 2014 at 01:55:57PM +0530, Viresh Kumar wrote: > On 23 July 2014 12:54, Thierry Reding wrote: > > ARM_TEGRA_CPUFREQ is still optional, so the select only applies when the > > Tegra cpufreq driver is enabled. This is mostly just out of convenience, > > though. The Tegra cpufreq driver uses the generic CPU0 cpufreq driver so > > a select will automatically pull in the necessary dependency. With a > > Not necessarily. cpufreq-cpu0 can have few unmet dependency. And so > there are chances that tegra driver is compiled but cpufreq-cpu0 isn't as > we didn't mention it as a *hard* dependency. > > And so at boot, there wouldn't be any cpufreq support even when tegra's > cpufreq driver is available. > > Though, menuconfig may give some warnings no such situations. > > > "depends on" the Tegra cpufreq driver only becomes available after > > you've selected GENERIC_CPUFREQ_CPU0, which is somewhat unintuitive. > > > > To illustrate with an example: as a user, I want to enable CPU frequency > > scaling on Tegra. So I use menuconfig to navigate to the "CPU Frequency > > scaling" menu (enable it if not available yet) and look for an entry > > that says "Tegra". But I can't find it because it's hidden due to the > > lack of GENERIC_CPUFREQ_CPU0. That the Tegra CPU frequency driver uses a > > generic driver is an implementation detail that users shouldn't have to > > be aware of. > > Don't know, the guy compiling out stuff should be knowledgeable enough to > have a look why tegra cpufreq entry isn't shown in menu. As, probably the > above problem I mentioned looks to be of more significance than this one, > atleast to me :) > > And, another thing to mention is that CONFIG_TEGRA_CPUFREQ is valid > for earlier platforms as well and so a select/depends wouldn't be valid for > earlier platforms. We probably need another Kconfig entry here. Yes, sounds like a new Kconfig entry for this specific driver would be a better approach and should remove all the above concerns. Thierry pgpmzsPVkt9FV.pgp Description: PGP signature
Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124
On 23/07/14 10:09, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Mon, Jul 21, 2014 at 06:39:00PM +0300, Tuomas Tynkkynen wrote:
> [...]
>> diff --git a/drivers/cpufreq/tegra124-cpufreq.c
>> b/drivers/cpufreq/tegra124-cpufreq.c
> [...]
>> +static int tegra124_cpu_switch_to_dfll(void)
>> +{
>> +struct clk *original_cpu_clk_parent;
>
> Maybe just "parent"?
>
>> +unsigned long rate;
>> +struct dev_pm_opp *opp;
>> +int ret;
>> +
>> +rate = clk_get_rate(cpu_clk);
>> +opp = dev_pm_opp_find_freq_ceil(cpu_dev, );
>> +if (IS_ERR(opp))
>> +return PTR_ERR(opp);
>> +
>> +ret = clk_set_rate(dfll_clk, rate);
>> +if (ret)
>> +return ret;
>> +
>> +original_cpu_clk_parent = clk_get_parent(cpu_clk);
>> +clk_set_parent(cpu_clk, pllp_clk);
>> +if (ret)
>> +return ret;
>> +
>> +ret = clk_prepare_enable(dfll_clk);
>> +if (ret)
>> +goto out_switch_to_original_parent;
>
> This could simply be "out" or "err" or anything else shorter than the
> above.
>
>> +
>> +clk_set_parent(cpu_clk, dfll_clk);
>> +
>> +return 0;
>> +
>> +out_switch_to_original_parent:
>> +clk_set_parent(cpu_clk, original_cpu_clk_parent);
>> +
>> +return ret;
>> +}
>> +
>> +static struct platform_device_info cpufreq_cpu0_devinfo = {
>> +.name = "cpufreq-cpu0",
>> +};
>> +
>> +static int tegra124_cpufreq_probe(struct platform_device *pdev)
>> +{
>> +int ret;
>> +
>> +cpu_dev = get_cpu_device(0);
>> +if (!cpu_dev)
>> +return -ENODEV;
>> +
>> +cpu_clk = of_clk_get_by_name(cpu_dev->of_node, "cpu_g");
>> +if (IS_ERR(cpu_clk))
>> +return PTR_ERR(cpu_clk);
>> +
>> +dfll_clk = of_clk_get_by_name(cpu_dev->of_node, "dfll");
>> +if (IS_ERR(dfll_clk)) {
>> +ret = PTR_ERR(dfll_clk);
>> +goto out_put_cpu_clk;
>> +}
>> +
>> +pllx_clk = of_clk_get_by_name(cpu_dev->of_node, "pll_x");
>> +if (IS_ERR(pllx_clk)) {
>> +ret = PTR_ERR(pllx_clk);
>> +goto out_put_dfll_clk;
>> +}
>> +
>> +pllp_clk = of_clk_get_by_name(cpu_dev->of_node, "pll_p");
>> +if (IS_ERR(pllp_clk)) {
>> +ret = PTR_ERR(pllp_clk);
>> +goto out_put_pllx_clk;
>> +}
>
> Can the above not be devm_clk_get(cpu_dev, "...") so that you can remove
> all the clk_put() calls in the cleanup code below?
That would allocate the clks under the cpu_dev's devres list, i.e. all the
clk_puts wouldn't happen when the cpufreq driver goes away, but only when
cpu_dev itself goes away.
>
>> +
>> +ret = dev_pm_opp_init_cpufreq_table(cpu_dev, _table);
>> +if (ret)
>> +goto out_put_pllp_clk;
>> +
>> +ret = tegra124_cpu_switch_to_dfll();
>> +if (ret)
>> +goto out_free_table;
>> +
>> +platform_device_register_full(_cpu0_devinfo);
>
> Should the cpufreq_cpu0_devinfo device perhaps be a child of pdev?
Yeah, I suppose it should.
>> +
>> +return 0;
>> +
>> +out_free_table:
>> +dev_pm_opp_free_cpufreq_table(cpu_dev, _table);
>> +out_put_pllp_clk:
>> +clk_put(pllp_clk);
>> +out_put_pllx_clk:
>> +clk_put(pllx_clk);
>> +out_put_dfll_clk:
>> +clk_put(dfll_clk);
>> +out_put_cpu_clk:
>> +clk_put(cpu_clk);
>> +
>> +return ret;
>> +}
>> +
>> +static struct platform_driver tegra124_cpufreq_platdrv = {
>> +.driver = {
>> +.name = "cpufreq-tegra124",
>> +.owner = THIS_MODULE,
>> +},
>> +.probe = tegra124_cpufreq_probe,
>
> Note that simply leaving out .remove() here doesn't guarantee that the
> driver won't be unloaded. Also building it into the kernel doesn't
> prevent that. You can still unbind the driver via sysfs. So you'd need
> to add a .suppress_bind_attrs = true above.
I hadn't heard about suppress_bind_attrs before, it indeed sounds useful.
> But is there even a reason why we need that? Couldn't we make the
> driver's .remove() undo what .probe() did so that the driver can be
> unloaded?
I guess that could be done, though to fully undo everything the regulator
voltage would also need to be saved/restored.
> Otherwise it probably makes more sense not to use a driver (and dummy
> device) at all as Viresh already mentioned.
>
The dummy platform device is only required for probe deferral, if that
could be solved in a different way then yes.
>> +};
>> +
>> +static const struct of_device_id soc_of_matches[] = {
>> +{ .compatible = "nvidia,tegra124", },
>> +{}
>> +};
>> +
>> +static int __init tegra_cpufreq_init(void)
>> +{
>> +int ret;
>> +struct platform_device *pdev;
>> +
>> +if (!of_find_matching_node(NULL, soc_of_matches))
>> +return -ENODEV;
>
> I think this could be of_machine_is_compatible() since there's only a
> single entry in the match table. If there's a good chance that we may
> end up with more entries, perhaps now would be a good time to add an
> of_match_machine() function?
I think
Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124
On 23/07/14 07:44, Viresh Kumar wrote:
> On 21 July 2014 21:09, Tuomas Tynkkynen wrote:
>
>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>> index 7364a53..df3c73e 100644
>> --- a/drivers/cpufreq/Kconfig.arm
>> +++ b/drivers/cpufreq/Kconfig.arm
>> @@ -244,6 +244,7 @@ config ARM_SPEAR_CPUFREQ
>> config ARM_TEGRA_CPUFREQ
>> bool "TEGRA CPUFreq support"
>> depends on ARCH_TEGRA
>> + depends on GENERIC_CPUFREQ_CPU0
>
> Wouldn't this also disturb the existing cpufreq driver for earlier
> tegra platforms? i.e. we don't need cpufreq-cpu0 for them
> atleast as of now.
>
>> default y
>> help
>> This adds the CPUFreq driver support for TEGRA SOCs.
>> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
>> index db6d9a2..3437d24 100644
>> --- a/drivers/cpufreq/Makefile
>> +++ b/drivers/cpufreq/Makefile
>> @@ -76,6 +76,7 @@ obj-$(CONFIG_ARM_SA1100_CPUFREQ) += sa1100-cpufreq.o
>> obj-$(CONFIG_ARM_SA1110_CPUFREQ) += sa1110-cpufreq.o
>> obj-$(CONFIG_ARM_SPEAR_CPUFREQ)+= spear-cpufreq.o
>> obj-$(CONFIG_ARM_TEGRA_CPUFREQ)+= tegra-cpufreq.o
>> +obj-$(CONFIG_ARM_TEGRA_CPUFREQ)+= tegra124-cpufreq.o
>
> Maybe, you can update the same line if you want.
Oh, right.
>
>> obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o
>>
>>
>> ##
>> diff --git a/drivers/cpufreq/tegra124-cpufreq.c
>> b/drivers/cpufreq/tegra124-cpufreq.c
>
>> +static struct cpufreq_frequency_table *freq_table;
>> +
>> +static struct device *cpu_dev;
>> +static struct clk *cpu_clk;
>> +static struct clk *pllp_clk;
>> +static struct clk *pllx_clk;
>> +static struct clk *dfll_clk;
>
> The routines in this file are going to be called just once at boot, right?
> In that case we are actually wasting some memory by creating globals.
> Probably just move all these in a struct and allocate it at runtime.
>
Sure.
[...]
>> +
>> + ret = dev_pm_opp_init_cpufreq_table(cpu_dev, _table);
>> + if (ret)
>> + goto out_put_pllp_clk;
>
> Why do you need this? cpufreq-cpu0 also does it and this freq_table is
> just not getting used at all then.
>
Oops, yes I forgot to remove that part when making the conversion.
>> +
>> + pdev = platform_device_register_simple("cpufreq-tegra124", -1, NULL,
>> 0);
>> + if (IS_ERR(pdev)) {
>> + platform_driver_unregister(_cpufreq_platdrv);
>> + return PTR_ERR(pdev);
>> + }
>
> Why create another unnecessary platform-device/driver here? If
> of_find_matching_node() passes and you really need to probe cpufreq-cpu0
> here, then just remove the other two calls and move probe's implementation
> here only.
>
The platform device is required for the deferred probe that can happen if the
DFLL driver hasn't initialized yet, and module_init() callbacks don't seem to
respect -EPROBE_DEFER.
[...]
--
nvpublic
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124
On 23 July 2014 12:54, Thierry Reding wrote: > ARM_TEGRA_CPUFREQ is still optional, so the select only applies when the > Tegra cpufreq driver is enabled. This is mostly just out of convenience, > though. The Tegra cpufreq driver uses the generic CPU0 cpufreq driver so > a select will automatically pull in the necessary dependency. With a Not necessarily. cpufreq-cpu0 can have few unmet dependency. And so there are chances that tegra driver is compiled but cpufreq-cpu0 isn't as we didn't mention it as a *hard* dependency. And so at boot, there wouldn't be any cpufreq support even when tegra's cpufreq driver is available. Though, menuconfig may give some warnings no such situations. > "depends on" the Tegra cpufreq driver only becomes available after > you've selected GENERIC_CPUFREQ_CPU0, which is somewhat unintuitive. > > To illustrate with an example: as a user, I want to enable CPU frequency > scaling on Tegra. So I use menuconfig to navigate to the "CPU Frequency > scaling" menu (enable it if not available yet) and look for an entry > that says "Tegra". But I can't find it because it's hidden due to the > lack of GENERIC_CPUFREQ_CPU0. That the Tegra CPU frequency driver uses a > generic driver is an implementation detail that users shouldn't have to > be aware of. Don't know, the guy compiling out stuff should be knowledgeable enough to have a look why tegra cpufreq entry isn't shown in menu. As, probably the above problem I mentioned looks to be of more significance than this one, atleast to me :) And, another thing to mention is that CONFIG_TEGRA_CPUFREQ is valid for earlier platforms as well and so a select/depends wouldn't be valid for earlier platforms. We probably need another Kconfig entry here. > But we're using cpu_dev->of_node, so we need to make sure cpu_dev > doesn't go away suddenly. Simply keeping a reference to ->of_node > won't ensure that. Oh, yeah I completely agree, but don't see that as a normal code style people follow. Probably they take cpu for granted, which doesn't look right :) > I guess technically it would be better if get_cpu_device() already > incremented the reference count on the returned struct device. Currently > it would theoretically still be possible for the device to disappear > between the call to get_cpu_device() and a call to get_device(). I agree again. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124
On Wed, Jul 23, 2014 at 12:28:21PM +0530, Viresh Kumar wrote:
> On 23 July 2014 12:24, Thierry Reding wrote:
> > On Wed, Jul 23, 2014 at 10:14:44AM +0530, Viresh Kumar wrote:
> >> On 21 July 2014 21:09, Tuomas Tynkkynen wrote:
> >>
> >> > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> >> > index 7364a53..df3c73e 100644
> >> > --- a/drivers/cpufreq/Kconfig.arm
> >> > +++ b/drivers/cpufreq/Kconfig.arm
> >> > @@ -244,6 +244,7 @@ config ARM_SPEAR_CPUFREQ
> >> > config ARM_TEGRA_CPUFREQ
> >> > bool "TEGRA CPUFreq support"
> >> > depends on ARCH_TEGRA
> >> > + depends on GENERIC_CPUFREQ_CPU0
> >>
> >> Wouldn't this also disturb the existing cpufreq driver for earlier
> >> tegra platforms? i.e. we don't need cpufreq-cpu0 for them
> >> atleast as of now.
> >
> > Perhaps this should be "select" rather than "depends on"?
>
> Don't know, its not optionaly for tegra124 and so a "depends on"
> might fit better ?
ARM_TEGRA_CPUFREQ is still optional, so the select only applies when the
Tegra cpufreq driver is enabled. This is mostly just out of convenience,
though. The Tegra cpufreq driver uses the generic CPU0 cpufreq driver so
a select will automatically pull in the necessary dependency. With a
"depends on" the Tegra cpufreq driver only becomes available after
you've selected GENERIC_CPUFREQ_CPU0, which is somewhat unintuitive.
To illustrate with an example: as a user, I want to enable CPU frequency
scaling on Tegra. So I use menuconfig to navigate to the "CPU Frequency
scaling" menu (enable it if not available yet) and look for an entry
that says "Tegra". But I can't find it because it's hidden due to the
lack of GENERIC_CPUFREQ_CPU0. That the Tegra CPU frequency driver uses a
generic driver is an implementation detail that users shouldn't have to
be aware of.
> >> > +static int tegra124_cpufreq_probe(struct platform_device *pdev)
> >> > +{
> >> > + int ret;
> >> > +
> >> > + cpu_dev = get_cpu_device(0);
> >> > + if (!cpu_dev)
> >> > + return -ENODEV;
> >> > +
> >>
> >> Shouldn't we do a of_node_get() here?
> >
> > I think this would need to be get_device() since it's the struct device
> > that's being used subsequently.
>
> Probably I didn't write it well..
>
> What I meant was after doing a get_cpu_device() we might also need
> to do of_node_get(cpu_dev->of_node) as we would be using of_node
> in further code.
But we're using cpu_dev->of_node, so we need to make sure cpu_dev
doesn't go away suddenly. Simply keeping a reference to ->of_node
won't ensure that.
I guess technically it would be better if get_cpu_device() already
incremented the reference count on the returned struct device. Currently
it would theoretically still be possible for the device to disappear
between the call to get_cpu_device() and a call to get_device().
Thierry
pgp4zNkZ72U4Q.pgp
Description: PGP signature
Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124
On Mon, Jul 21, 2014 at 9:09 PM, Tuomas Tynkkynen wrote:
> Add a new cpufreq driver for Tegra124. Instead of using the PLLX as
> +
> +static int tegra124_cpu_switch_to_dfll(void)
> +{
> + struct clk *original_cpu_clk_parent;
> + unsigned long rate;
> + struct dev_pm_opp *opp;
> + int ret;
> +
> + rate = clk_get_rate(cpu_clk);
> + opp = dev_pm_opp_find_freq_ceil(cpu_dev, );
> + if (IS_ERR(opp))
> + return PTR_ERR(opp);
> +
> + ret = clk_set_rate(dfll_clk, rate);
> + if (ret)
> + return ret;
> +
> + original_cpu_clk_parent = clk_get_parent(cpu_clk);
> + clk_set_parent(cpu_clk, pllp_clk);
Needs return check.
> + if (ret)
> + return ret;
Which 'ret' is being checked here? nothing is assigned here. May be
fixing previous comment will fix this.
> +
> + ret = clk_prepare_enable(dfll_clk);
> Please read the FAQ at http://www.tux.org/lkml/
--
Thanks and Regards
Pramod
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124
On Mon, Jul 21, 2014 at 06:39:00PM +0300, Tuomas Tynkkynen wrote:
[...]
> diff --git a/drivers/cpufreq/tegra124-cpufreq.c
> b/drivers/cpufreq/tegra124-cpufreq.c
[...]
> +static int tegra124_cpu_switch_to_dfll(void)
> +{
> + struct clk *original_cpu_clk_parent;
Maybe just "parent"?
> + unsigned long rate;
> + struct dev_pm_opp *opp;
> + int ret;
> +
> + rate = clk_get_rate(cpu_clk);
> + opp = dev_pm_opp_find_freq_ceil(cpu_dev, );
> + if (IS_ERR(opp))
> + return PTR_ERR(opp);
> +
> + ret = clk_set_rate(dfll_clk, rate);
> + if (ret)
> + return ret;
> +
> + original_cpu_clk_parent = clk_get_parent(cpu_clk);
> + clk_set_parent(cpu_clk, pllp_clk);
> + if (ret)
> + return ret;
> +
> + ret = clk_prepare_enable(dfll_clk);
> + if (ret)
> + goto out_switch_to_original_parent;
This could simply be "out" or "err" or anything else shorter than the
above.
> +
> + clk_set_parent(cpu_clk, dfll_clk);
> +
> + return 0;
> +
> +out_switch_to_original_parent:
> + clk_set_parent(cpu_clk, original_cpu_clk_parent);
> +
> + return ret;
> +}
> +
> +static struct platform_device_info cpufreq_cpu0_devinfo = {
> + .name = "cpufreq-cpu0",
> +};
> +
> +static int tegra124_cpufreq_probe(struct platform_device *pdev)
> +{
> + int ret;
> +
> + cpu_dev = get_cpu_device(0);
> + if (!cpu_dev)
> + return -ENODEV;
> +
> + cpu_clk = of_clk_get_by_name(cpu_dev->of_node, "cpu_g");
> + if (IS_ERR(cpu_clk))
> + return PTR_ERR(cpu_clk);
> +
> + dfll_clk = of_clk_get_by_name(cpu_dev->of_node, "dfll");
> + if (IS_ERR(dfll_clk)) {
> + ret = PTR_ERR(dfll_clk);
> + goto out_put_cpu_clk;
> + }
> +
> + pllx_clk = of_clk_get_by_name(cpu_dev->of_node, "pll_x");
> + if (IS_ERR(pllx_clk)) {
> + ret = PTR_ERR(pllx_clk);
> + goto out_put_dfll_clk;
> + }
> +
> + pllp_clk = of_clk_get_by_name(cpu_dev->of_node, "pll_p");
> + if (IS_ERR(pllp_clk)) {
> + ret = PTR_ERR(pllp_clk);
> + goto out_put_pllx_clk;
> + }
Can the above not be devm_clk_get(cpu_dev, "...") so that you can remove
all the clk_put() calls in the cleanup code below?
> +
> + ret = dev_pm_opp_init_cpufreq_table(cpu_dev, _table);
> + if (ret)
> + goto out_put_pllp_clk;
> +
> + ret = tegra124_cpu_switch_to_dfll();
> + if (ret)
> + goto out_free_table;
> +
> + platform_device_register_full(_cpu0_devinfo);
Should the cpufreq_cpu0_devinfo device perhaps be a child of pdev?
> +
> + return 0;
> +
> +out_free_table:
> + dev_pm_opp_free_cpufreq_table(cpu_dev, _table);
> +out_put_pllp_clk:
> + clk_put(pllp_clk);
> +out_put_pllx_clk:
> + clk_put(pllx_clk);
> +out_put_dfll_clk:
> + clk_put(dfll_clk);
> +out_put_cpu_clk:
> + clk_put(cpu_clk);
> +
> + return ret;
> +}
> +
> +static struct platform_driver tegra124_cpufreq_platdrv = {
> + .driver = {
> + .name = "cpufreq-tegra124",
> + .owner = THIS_MODULE,
> + },
> + .probe = tegra124_cpufreq_probe,
Note that simply leaving out .remove() here doesn't guarantee that the
driver won't be unloaded. Also building it into the kernel doesn't
prevent that. You can still unbind the driver via sysfs. So you'd need
to add a .suppress_bind_attrs = true above.
But is there even a reason why we need that? Couldn't we make the
driver's .remove() undo what .probe() did so that the driver can be
unloaded?
Otherwise it probably makes more sense not to use a driver (and dummy
device) at all as Viresh already mentioned.
> +};
> +
> +static const struct of_device_id soc_of_matches[] = {
> + { .compatible = "nvidia,tegra124", },
> + {}
> +};
> +
> +static int __init tegra_cpufreq_init(void)
> +{
> + int ret;
> + struct platform_device *pdev;
> +
> + if (!of_find_matching_node(NULL, soc_of_matches))
> + return -ENODEV;
I think this could be of_machine_is_compatible() since there's only a
single entry in the match table. If there's a good chance that we may
end up with more entries, perhaps now would be a good time to add an
of_match_machine() function?
> +
> + ret = platform_driver_register(_cpufreq_platdrv);
> + if (ret)
> + return ret;
> +
> + pdev = platform_device_register_simple("cpufreq-tegra124", -1, NULL, 0);
> + if (IS_ERR(pdev)) {
> + platform_driver_unregister(_cpufreq_platdrv);
> + return PTR_ERR(pdev);
> + }
> +
> + return 0;
> +}
> +
> +MODULE_AUTHOR("Tuomas Tynkkynen ");
> +MODULE_DESCRIPTION("cpufreq driver for nVIDIA Tegra124");
We use "NVIDIA" everywhere nowadays.
> +MODULE_LICENSE("GPLv2");
The correct license string is "GPL v2".
> +module_init(tegra_cpufreq_init);
The placement of this is unusual. It should go immediately below the
tegra_cpufreq_init() function.
Thierry
Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124
On 23 July 2014 12:24, Thierry Reding wrote:
> On Wed, Jul 23, 2014 at 10:14:44AM +0530, Viresh Kumar wrote:
>> On 21 July 2014 21:09, Tuomas Tynkkynen wrote:
>>
>> > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>> > index 7364a53..df3c73e 100644
>> > --- a/drivers/cpufreq/Kconfig.arm
>> > +++ b/drivers/cpufreq/Kconfig.arm
>> > @@ -244,6 +244,7 @@ config ARM_SPEAR_CPUFREQ
>> > config ARM_TEGRA_CPUFREQ
>> > bool "TEGRA CPUFreq support"
>> > depends on ARCH_TEGRA
>> > + depends on GENERIC_CPUFREQ_CPU0
>>
>> Wouldn't this also disturb the existing cpufreq driver for earlier
>> tegra platforms? i.e. we don't need cpufreq-cpu0 for them
>> atleast as of now.
>
> Perhaps this should be "select" rather than "depends on"?
Don't know, its not optionaly for tegra124 and so a "depends on"
might fit better ?
>> > +static int tegra124_cpufreq_probe(struct platform_device *pdev)
>> > +{
>> > + int ret;
>> > +
>> > + cpu_dev = get_cpu_device(0);
>> > + if (!cpu_dev)
>> > + return -ENODEV;
>> > +
>>
>> Shouldn't we do a of_node_get() here?
>
> I think this would need to be get_device() since it's the struct device
> that's being used subsequently.
Probably I didn't write it well..
What I meant was after doing a get_cpu_device() we might also need
to do of_node_get(cpu_dev->of_node) as we would be using of_node
in further code.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124
On Wed, Jul 23, 2014 at 10:14:44AM +0530, Viresh Kumar wrote:
> On 21 July 2014 21:09, Tuomas Tynkkynen wrote:
>
> > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> > index 7364a53..df3c73e 100644
> > --- a/drivers/cpufreq/Kconfig.arm
> > +++ b/drivers/cpufreq/Kconfig.arm
> > @@ -244,6 +244,7 @@ config ARM_SPEAR_CPUFREQ
> > config ARM_TEGRA_CPUFREQ
> > bool "TEGRA CPUFreq support"
> > depends on ARCH_TEGRA
> > + depends on GENERIC_CPUFREQ_CPU0
>
> Wouldn't this also disturb the existing cpufreq driver for earlier
> tegra platforms? i.e. we don't need cpufreq-cpu0 for them
> atleast as of now.
Perhaps this should be "select" rather than "depends on"?
> > +static int tegra124_cpufreq_probe(struct platform_device *pdev)
> > +{
> > + int ret;
> > +
> > + cpu_dev = get_cpu_device(0);
> > + if (!cpu_dev)
> > + return -ENODEV;
> > +
>
> Shouldn't we do a of_node_get() here?
I think this would need to be get_device() since it's the struct device
that's being used subsequently.
Thierry
pgp3vOPVTVbVH.pgp
Description: PGP signature
Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124
On Wed, Jul 23, 2014 at 10:14:44AM +0530, Viresh Kumar wrote:
On 21 July 2014 21:09, Tuomas Tynkkynen ttynkky...@nvidia.com wrote:
diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 7364a53..df3c73e 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -244,6 +244,7 @@ config ARM_SPEAR_CPUFREQ
config ARM_TEGRA_CPUFREQ
bool TEGRA CPUFreq support
depends on ARCH_TEGRA
+ depends on GENERIC_CPUFREQ_CPU0
Wouldn't this also disturb the existing cpufreq driver for earlier
tegra platforms? i.e. we don't need cpufreq-cpu0 for them
atleast as of now.
Perhaps this should be select rather than depends on?
+static int tegra124_cpufreq_probe(struct platform_device *pdev)
+{
+ int ret;
+
+ cpu_dev = get_cpu_device(0);
+ if (!cpu_dev)
+ return -ENODEV;
+
Shouldn't we do a of_node_get() here?
I think this would need to be get_device() since it's the struct device
that's being used subsequently.
Thierry
pgp3vOPVTVbVH.pgp
Description: PGP signature
Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124
On 23 July 2014 12:24, Thierry Reding thierry.red...@gmail.com wrote:
On Wed, Jul 23, 2014 at 10:14:44AM +0530, Viresh Kumar wrote:
On 21 July 2014 21:09, Tuomas Tynkkynen ttynkky...@nvidia.com wrote:
diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 7364a53..df3c73e 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -244,6 +244,7 @@ config ARM_SPEAR_CPUFREQ
config ARM_TEGRA_CPUFREQ
bool TEGRA CPUFreq support
depends on ARCH_TEGRA
+ depends on GENERIC_CPUFREQ_CPU0
Wouldn't this also disturb the existing cpufreq driver for earlier
tegra platforms? i.e. we don't need cpufreq-cpu0 for them
atleast as of now.
Perhaps this should be select rather than depends on?
Don't know, its not optionaly for tegra124 and so a depends on
might fit better ?
+static int tegra124_cpufreq_probe(struct platform_device *pdev)
+{
+ int ret;
+
+ cpu_dev = get_cpu_device(0);
+ if (!cpu_dev)
+ return -ENODEV;
+
Shouldn't we do a of_node_get() here?
I think this would need to be get_device() since it's the struct device
that's being used subsequently.
Probably I didn't write it well..
What I meant was after doing a get_cpu_device() we might also need
to do of_node_get(cpu_dev-of_node) as we would be using of_node
in further code.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124
On Mon, Jul 21, 2014 at 06:39:00PM +0300, Tuomas Tynkkynen wrote:
[...]
diff --git a/drivers/cpufreq/tegra124-cpufreq.c
b/drivers/cpufreq/tegra124-cpufreq.c
[...]
+static int tegra124_cpu_switch_to_dfll(void)
+{
+ struct clk *original_cpu_clk_parent;
Maybe just parent?
+ unsigned long rate;
+ struct dev_pm_opp *opp;
+ int ret;
+
+ rate = clk_get_rate(cpu_clk);
+ opp = dev_pm_opp_find_freq_ceil(cpu_dev, rate);
+ if (IS_ERR(opp))
+ return PTR_ERR(opp);
+
+ ret = clk_set_rate(dfll_clk, rate);
+ if (ret)
+ return ret;
+
+ original_cpu_clk_parent = clk_get_parent(cpu_clk);
+ clk_set_parent(cpu_clk, pllp_clk);
+ if (ret)
+ return ret;
+
+ ret = clk_prepare_enable(dfll_clk);
+ if (ret)
+ goto out_switch_to_original_parent;
This could simply be out or err or anything else shorter than the
above.
+
+ clk_set_parent(cpu_clk, dfll_clk);
+
+ return 0;
+
+out_switch_to_original_parent:
+ clk_set_parent(cpu_clk, original_cpu_clk_parent);
+
+ return ret;
+}
+
+static struct platform_device_info cpufreq_cpu0_devinfo = {
+ .name = cpufreq-cpu0,
+};
+
+static int tegra124_cpufreq_probe(struct platform_device *pdev)
+{
+ int ret;
+
+ cpu_dev = get_cpu_device(0);
+ if (!cpu_dev)
+ return -ENODEV;
+
+ cpu_clk = of_clk_get_by_name(cpu_dev-of_node, cpu_g);
+ if (IS_ERR(cpu_clk))
+ return PTR_ERR(cpu_clk);
+
+ dfll_clk = of_clk_get_by_name(cpu_dev-of_node, dfll);
+ if (IS_ERR(dfll_clk)) {
+ ret = PTR_ERR(dfll_clk);
+ goto out_put_cpu_clk;
+ }
+
+ pllx_clk = of_clk_get_by_name(cpu_dev-of_node, pll_x);
+ if (IS_ERR(pllx_clk)) {
+ ret = PTR_ERR(pllx_clk);
+ goto out_put_dfll_clk;
+ }
+
+ pllp_clk = of_clk_get_by_name(cpu_dev-of_node, pll_p);
+ if (IS_ERR(pllp_clk)) {
+ ret = PTR_ERR(pllp_clk);
+ goto out_put_pllx_clk;
+ }
Can the above not be devm_clk_get(cpu_dev, ...) so that you can remove
all the clk_put() calls in the cleanup code below?
+
+ ret = dev_pm_opp_init_cpufreq_table(cpu_dev, freq_table);
+ if (ret)
+ goto out_put_pllp_clk;
+
+ ret = tegra124_cpu_switch_to_dfll();
+ if (ret)
+ goto out_free_table;
+
+ platform_device_register_full(cpufreq_cpu0_devinfo);
Should the cpufreq_cpu0_devinfo device perhaps be a child of pdev?
+
+ return 0;
+
+out_free_table:
+ dev_pm_opp_free_cpufreq_table(cpu_dev, freq_table);
+out_put_pllp_clk:
+ clk_put(pllp_clk);
+out_put_pllx_clk:
+ clk_put(pllx_clk);
+out_put_dfll_clk:
+ clk_put(dfll_clk);
+out_put_cpu_clk:
+ clk_put(cpu_clk);
+
+ return ret;
+}
+
+static struct platform_driver tegra124_cpufreq_platdrv = {
+ .driver = {
+ .name = cpufreq-tegra124,
+ .owner = THIS_MODULE,
+ },
+ .probe = tegra124_cpufreq_probe,
Note that simply leaving out .remove() here doesn't guarantee that the
driver won't be unloaded. Also building it into the kernel doesn't
prevent that. You can still unbind the driver via sysfs. So you'd need
to add a .suppress_bind_attrs = true above.
But is there even a reason why we need that? Couldn't we make the
driver's .remove() undo what .probe() did so that the driver can be
unloaded?
Otherwise it probably makes more sense not to use a driver (and dummy
device) at all as Viresh already mentioned.
+};
+
+static const struct of_device_id soc_of_matches[] = {
+ { .compatible = nvidia,tegra124, },
+ {}
+};
+
+static int __init tegra_cpufreq_init(void)
+{
+ int ret;
+ struct platform_device *pdev;
+
+ if (!of_find_matching_node(NULL, soc_of_matches))
+ return -ENODEV;
I think this could be of_machine_is_compatible() since there's only a
single entry in the match table. If there's a good chance that we may
end up with more entries, perhaps now would be a good time to add an
of_match_machine() function?
+
+ ret = platform_driver_register(tegra124_cpufreq_platdrv);
+ if (ret)
+ return ret;
+
+ pdev = platform_device_register_simple(cpufreq-tegra124, -1, NULL, 0);
+ if (IS_ERR(pdev)) {
+ platform_driver_unregister(tegra124_cpufreq_platdrv);
+ return PTR_ERR(pdev);
+ }
+
+ return 0;
+}
+
+MODULE_AUTHOR(Tuomas Tynkkynen ttynkky...@nvidia.com);
+MODULE_DESCRIPTION(cpufreq driver for nVIDIA Tegra124);
We use NVIDIA everywhere nowadays.
+MODULE_LICENSE(GPLv2);
The correct license string is GPL v2.
+module_init(tegra_cpufreq_init);
The placement of this is unusual. It should go immediately below the
tegra_cpufreq_init() function.
Thierry
pgpiKfSNDIHoa.pgp
Description: PGP signature
Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124
On Mon, Jul 21, 2014 at 9:09 PM, Tuomas Tynkkynen ttynkky...@nvidia.com wrote:
Add a new cpufreq driver for Tegra124. Instead of using the PLLX as
snip
+
+static int tegra124_cpu_switch_to_dfll(void)
+{
+ struct clk *original_cpu_clk_parent;
+ unsigned long rate;
+ struct dev_pm_opp *opp;
+ int ret;
+
+ rate = clk_get_rate(cpu_clk);
+ opp = dev_pm_opp_find_freq_ceil(cpu_dev, rate);
+ if (IS_ERR(opp))
+ return PTR_ERR(opp);
+
+ ret = clk_set_rate(dfll_clk, rate);
+ if (ret)
+ return ret;
+
+ original_cpu_clk_parent = clk_get_parent(cpu_clk);
+ clk_set_parent(cpu_clk, pllp_clk);
Needs return check.
+ if (ret)
+ return ret;
Which 'ret' is being checked here? nothing is assigned here. May be
fixing previous comment will fix this.
+
+ ret = clk_prepare_enable(dfll_clk);
snip
Please read the FAQ at http://www.tux.org/lkml/
--
Thanks and Regards
Pramod
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124
On Wed, Jul 23, 2014 at 12:28:21PM +0530, Viresh Kumar wrote:
On 23 July 2014 12:24, Thierry Reding thierry.red...@gmail.com wrote:
On Wed, Jul 23, 2014 at 10:14:44AM +0530, Viresh Kumar wrote:
On 21 July 2014 21:09, Tuomas Tynkkynen ttynkky...@nvidia.com wrote:
diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 7364a53..df3c73e 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -244,6 +244,7 @@ config ARM_SPEAR_CPUFREQ
config ARM_TEGRA_CPUFREQ
bool TEGRA CPUFreq support
depends on ARCH_TEGRA
+ depends on GENERIC_CPUFREQ_CPU0
Wouldn't this also disturb the existing cpufreq driver for earlier
tegra platforms? i.e. we don't need cpufreq-cpu0 for them
atleast as of now.
Perhaps this should be select rather than depends on?
Don't know, its not optionaly for tegra124 and so a depends on
might fit better ?
ARM_TEGRA_CPUFREQ is still optional, so the select only applies when the
Tegra cpufreq driver is enabled. This is mostly just out of convenience,
though. The Tegra cpufreq driver uses the generic CPU0 cpufreq driver so
a select will automatically pull in the necessary dependency. With a
depends on the Tegra cpufreq driver only becomes available after
you've selected GENERIC_CPUFREQ_CPU0, which is somewhat unintuitive.
To illustrate with an example: as a user, I want to enable CPU frequency
scaling on Tegra. So I use menuconfig to navigate to the CPU Frequency
scaling menu (enable it if not available yet) and look for an entry
that says Tegra. But I can't find it because it's hidden due to the
lack of GENERIC_CPUFREQ_CPU0. That the Tegra CPU frequency driver uses a
generic driver is an implementation detail that users shouldn't have to
be aware of.
+static int tegra124_cpufreq_probe(struct platform_device *pdev)
+{
+ int ret;
+
+ cpu_dev = get_cpu_device(0);
+ if (!cpu_dev)
+ return -ENODEV;
+
Shouldn't we do a of_node_get() here?
I think this would need to be get_device() since it's the struct device
that's being used subsequently.
Probably I didn't write it well..
What I meant was after doing a get_cpu_device() we might also need
to do of_node_get(cpu_dev-of_node) as we would be using of_node
in further code.
But we're using cpu_dev-of_node, so we need to make sure cpu_dev
doesn't go away suddenly. Simply keeping a reference to -of_node
won't ensure that.
I guess technically it would be better if get_cpu_device() already
incremented the reference count on the returned struct device. Currently
it would theoretically still be possible for the device to disappear
between the call to get_cpu_device() and a call to get_device().
Thierry
pgp4zNkZ72U4Q.pgp
Description: PGP signature
Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124
On 23 July 2014 12:54, Thierry Reding thierry.red...@gmail.com wrote: ARM_TEGRA_CPUFREQ is still optional, so the select only applies when the Tegra cpufreq driver is enabled. This is mostly just out of convenience, though. The Tegra cpufreq driver uses the generic CPU0 cpufreq driver so a select will automatically pull in the necessary dependency. With a Not necessarily. cpufreq-cpu0 can have few unmet dependency. And so there are chances that tegra driver is compiled but cpufreq-cpu0 isn't as we didn't mention it as a *hard* dependency. And so at boot, there wouldn't be any cpufreq support even when tegra's cpufreq driver is available. Though, menuconfig may give some warnings no such situations. depends on the Tegra cpufreq driver only becomes available after you've selected GENERIC_CPUFREQ_CPU0, which is somewhat unintuitive. To illustrate with an example: as a user, I want to enable CPU frequency scaling on Tegra. So I use menuconfig to navigate to the CPU Frequency scaling menu (enable it if not available yet) and look for an entry that says Tegra. But I can't find it because it's hidden due to the lack of GENERIC_CPUFREQ_CPU0. That the Tegra CPU frequency driver uses a generic driver is an implementation detail that users shouldn't have to be aware of. Don't know, the guy compiling out stuff should be knowledgeable enough to have a look why tegra cpufreq entry isn't shown in menu. As, probably the above problem I mentioned looks to be of more significance than this one, atleast to me :) And, another thing to mention is that CONFIG_TEGRA_CPUFREQ is valid for earlier platforms as well and so a select/depends wouldn't be valid for earlier platforms. We probably need another Kconfig entry here. But we're using cpu_dev-of_node, so we need to make sure cpu_dev doesn't go away suddenly. Simply keeping a reference to -of_node won't ensure that. Oh, yeah I completely agree, but don't see that as a normal code style people follow. Probably they take cpu for granted, which doesn't look right :) I guess technically it would be better if get_cpu_device() already incremented the reference count on the returned struct device. Currently it would theoretically still be possible for the device to disappear between the call to get_cpu_device() and a call to get_device(). I agree again. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124
On 23/07/14 07:44, Viresh Kumar wrote:
On 21 July 2014 21:09, Tuomas Tynkkynen ttynkky...@nvidia.com wrote:
diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 7364a53..df3c73e 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -244,6 +244,7 @@ config ARM_SPEAR_CPUFREQ
config ARM_TEGRA_CPUFREQ
bool TEGRA CPUFreq support
depends on ARCH_TEGRA
+ depends on GENERIC_CPUFREQ_CPU0
Wouldn't this also disturb the existing cpufreq driver for earlier
tegra platforms? i.e. we don't need cpufreq-cpu0 for them
atleast as of now.
default y
help
This adds the CPUFreq driver support for TEGRA SOCs.
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index db6d9a2..3437d24 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -76,6 +76,7 @@ obj-$(CONFIG_ARM_SA1100_CPUFREQ) += sa1100-cpufreq.o
obj-$(CONFIG_ARM_SA1110_CPUFREQ) += sa1110-cpufreq.o
obj-$(CONFIG_ARM_SPEAR_CPUFREQ)+= spear-cpufreq.o
obj-$(CONFIG_ARM_TEGRA_CPUFREQ)+= tegra-cpufreq.o
+obj-$(CONFIG_ARM_TEGRA_CPUFREQ)+= tegra124-cpufreq.o
Maybe, you can update the same line if you want.
Oh, right.
obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o
##
diff --git a/drivers/cpufreq/tegra124-cpufreq.c
b/drivers/cpufreq/tegra124-cpufreq.c
+static struct cpufreq_frequency_table *freq_table;
+
+static struct device *cpu_dev;
+static struct clk *cpu_clk;
+static struct clk *pllp_clk;
+static struct clk *pllx_clk;
+static struct clk *dfll_clk;
The routines in this file are going to be called just once at boot, right?
In that case we are actually wasting some memory by creating globals.
Probably just move all these in a struct and allocate it at runtime.
Sure.
[...]
+
+ ret = dev_pm_opp_init_cpufreq_table(cpu_dev, freq_table);
+ if (ret)
+ goto out_put_pllp_clk;
Why do you need this? cpufreq-cpu0 also does it and this freq_table is
just not getting used at all then.
Oops, yes I forgot to remove that part when making the conversion.
+
+ pdev = platform_device_register_simple(cpufreq-tegra124, -1, NULL,
0);
+ if (IS_ERR(pdev)) {
+ platform_driver_unregister(tegra124_cpufreq_platdrv);
+ return PTR_ERR(pdev);
+ }
Why create another unnecessary platform-device/driver here? If
of_find_matching_node() passes and you really need to probe cpufreq-cpu0
here, then just remove the other two calls and move probe's implementation
here only.
The platform device is required for the deferred probe that can happen if the
DFLL driver hasn't initialized yet, and module_init() callbacks don't seem to
respect -EPROBE_DEFER.
[...]
--
nvpublic
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124
On 23/07/14 10:09, Thierry Reding wrote:
* PGP Signed by an unknown key
On Mon, Jul 21, 2014 at 06:39:00PM +0300, Tuomas Tynkkynen wrote:
[...]
diff --git a/drivers/cpufreq/tegra124-cpufreq.c
b/drivers/cpufreq/tegra124-cpufreq.c
[...]
+static int tegra124_cpu_switch_to_dfll(void)
+{
+struct clk *original_cpu_clk_parent;
Maybe just parent?
+unsigned long rate;
+struct dev_pm_opp *opp;
+int ret;
+
+rate = clk_get_rate(cpu_clk);
+opp = dev_pm_opp_find_freq_ceil(cpu_dev, rate);
+if (IS_ERR(opp))
+return PTR_ERR(opp);
+
+ret = clk_set_rate(dfll_clk, rate);
+if (ret)
+return ret;
+
+original_cpu_clk_parent = clk_get_parent(cpu_clk);
+clk_set_parent(cpu_clk, pllp_clk);
+if (ret)
+return ret;
+
+ret = clk_prepare_enable(dfll_clk);
+if (ret)
+goto out_switch_to_original_parent;
This could simply be out or err or anything else shorter than the
above.
+
+clk_set_parent(cpu_clk, dfll_clk);
+
+return 0;
+
+out_switch_to_original_parent:
+clk_set_parent(cpu_clk, original_cpu_clk_parent);
+
+return ret;
+}
+
+static struct platform_device_info cpufreq_cpu0_devinfo = {
+.name = cpufreq-cpu0,
+};
+
+static int tegra124_cpufreq_probe(struct platform_device *pdev)
+{
+int ret;
+
+cpu_dev = get_cpu_device(0);
+if (!cpu_dev)
+return -ENODEV;
+
+cpu_clk = of_clk_get_by_name(cpu_dev-of_node, cpu_g);
+if (IS_ERR(cpu_clk))
+return PTR_ERR(cpu_clk);
+
+dfll_clk = of_clk_get_by_name(cpu_dev-of_node, dfll);
+if (IS_ERR(dfll_clk)) {
+ret = PTR_ERR(dfll_clk);
+goto out_put_cpu_clk;
+}
+
+pllx_clk = of_clk_get_by_name(cpu_dev-of_node, pll_x);
+if (IS_ERR(pllx_clk)) {
+ret = PTR_ERR(pllx_clk);
+goto out_put_dfll_clk;
+}
+
+pllp_clk = of_clk_get_by_name(cpu_dev-of_node, pll_p);
+if (IS_ERR(pllp_clk)) {
+ret = PTR_ERR(pllp_clk);
+goto out_put_pllx_clk;
+}
Can the above not be devm_clk_get(cpu_dev, ...) so that you can remove
all the clk_put() calls in the cleanup code below?
That would allocate the clks under the cpu_dev's devres list, i.e. all the
clk_puts wouldn't happen when the cpufreq driver goes away, but only when
cpu_dev itself goes away.
+
+ret = dev_pm_opp_init_cpufreq_table(cpu_dev, freq_table);
+if (ret)
+goto out_put_pllp_clk;
+
+ret = tegra124_cpu_switch_to_dfll();
+if (ret)
+goto out_free_table;
+
+platform_device_register_full(cpufreq_cpu0_devinfo);
Should the cpufreq_cpu0_devinfo device perhaps be a child of pdev?
Yeah, I suppose it should.
+
+return 0;
+
+out_free_table:
+dev_pm_opp_free_cpufreq_table(cpu_dev, freq_table);
+out_put_pllp_clk:
+clk_put(pllp_clk);
+out_put_pllx_clk:
+clk_put(pllx_clk);
+out_put_dfll_clk:
+clk_put(dfll_clk);
+out_put_cpu_clk:
+clk_put(cpu_clk);
+
+return ret;
+}
+
+static struct platform_driver tegra124_cpufreq_platdrv = {
+.driver = {
+.name = cpufreq-tegra124,
+.owner = THIS_MODULE,
+},
+.probe = tegra124_cpufreq_probe,
Note that simply leaving out .remove() here doesn't guarantee that the
driver won't be unloaded. Also building it into the kernel doesn't
prevent that. You can still unbind the driver via sysfs. So you'd need
to add a .suppress_bind_attrs = true above.
I hadn't heard about suppress_bind_attrs before, it indeed sounds useful.
But is there even a reason why we need that? Couldn't we make the
driver's .remove() undo what .probe() did so that the driver can be
unloaded?
I guess that could be done, though to fully undo everything the regulator
voltage would also need to be saved/restored.
Otherwise it probably makes more sense not to use a driver (and dummy
device) at all as Viresh already mentioned.
The dummy platform device is only required for probe deferral, if that
could be solved in a different way then yes.
+};
+
+static const struct of_device_id soc_of_matches[] = {
+{ .compatible = nvidia,tegra124, },
+{}
+};
+
+static int __init tegra_cpufreq_init(void)
+{
+int ret;
+struct platform_device *pdev;
+
+if (!of_find_matching_node(NULL, soc_of_matches))
+return -ENODEV;
I think this could be of_machine_is_compatible() since there's only a
single entry in the match table. If there's a good chance that we may
end up with more entries, perhaps now would be a good time to add an
of_match_machine() function?
I think this driver should work on Tegra132 without modifications.
of_match_machine() does sound useful for some of the other cpufreq
drivers as well and likely for your soc_is_tegra() from the PMC
series as well.
+
+ret = platform_driver_register(tegra124_cpufreq_platdrv);
Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124
On Wed, Jul 23, 2014 at 01:55:57PM +0530, Viresh Kumar wrote: On 23 July 2014 12:54, Thierry Reding thierry.red...@gmail.com wrote: ARM_TEGRA_CPUFREQ is still optional, so the select only applies when the Tegra cpufreq driver is enabled. This is mostly just out of convenience, though. The Tegra cpufreq driver uses the generic CPU0 cpufreq driver so a select will automatically pull in the necessary dependency. With a Not necessarily. cpufreq-cpu0 can have few unmet dependency. And so there are chances that tegra driver is compiled but cpufreq-cpu0 isn't as we didn't mention it as a *hard* dependency. And so at boot, there wouldn't be any cpufreq support even when tegra's cpufreq driver is available. Though, menuconfig may give some warnings no such situations. depends on the Tegra cpufreq driver only becomes available after you've selected GENERIC_CPUFREQ_CPU0, which is somewhat unintuitive. To illustrate with an example: as a user, I want to enable CPU frequency scaling on Tegra. So I use menuconfig to navigate to the CPU Frequency scaling menu (enable it if not available yet) and look for an entry that says Tegra. But I can't find it because it's hidden due to the lack of GENERIC_CPUFREQ_CPU0. That the Tegra CPU frequency driver uses a generic driver is an implementation detail that users shouldn't have to be aware of. Don't know, the guy compiling out stuff should be knowledgeable enough to have a look why tegra cpufreq entry isn't shown in menu. As, probably the above problem I mentioned looks to be of more significance than this one, atleast to me :) And, another thing to mention is that CONFIG_TEGRA_CPUFREQ is valid for earlier platforms as well and so a select/depends wouldn't be valid for earlier platforms. We probably need another Kconfig entry here. Yes, sounds like a new Kconfig entry for this specific driver would be a better approach and should remove all the above concerns. Thierry pgpmzsPVkt9FV.pgp Description: PGP signature
Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124
On Wed, Jul 23, 2014 at 03:35:46PM +0300, Tuomas Tynkkynen wrote:
On 23/07/14 10:09, Thierry Reding wrote:
On Mon, Jul 21, 2014 at 06:39:00PM +0300, Tuomas Tynkkynen wrote:
[...]
diff --git a/drivers/cpufreq/tegra124-cpufreq.c
b/drivers/cpufreq/tegra124-cpufreq.c
[...]
+ cpu_clk = of_clk_get_by_name(cpu_dev-of_node, cpu_g);
+ if (IS_ERR(cpu_clk))
+ return PTR_ERR(cpu_clk);
[...]
+ pllp_clk = of_clk_get_by_name(cpu_dev-of_node, pll_p);
+ if (IS_ERR(pllp_clk)) {
+ ret = PTR_ERR(pllp_clk);
+ goto out_put_pllx_clk;
+ }
Can the above not be devm_clk_get(cpu_dev, ...) so that you can remove
all the clk_put() calls in the cleanup code below?
That would allocate the clks under the cpu_dev's devres list, i.e. all the
clk_puts wouldn't happen when the cpufreq driver goes away, but only when
cpu_dev itself goes away.
I don't think so. devres_release_all() is called on driver detach as
well.
But is there even a reason why we need that? Couldn't we make the
driver's .remove() undo what .probe() did so that the driver can be
unloaded?
I guess that could be done, though to fully undo everything the regulator
voltage would also need to be saved/restored.
That would certainly be my prefered approach. that way the driver can
simply be unloaded, leaving the CPU in the same state as it was after
boot.
Otherwise it probably makes more sense not to use a driver (and dummy
device) at all as Viresh already mentioned.
The dummy platform device is only required for probe deferral, if that
could be solved in a different way then yes.
I don't think it can. Probe deferral is pretty closely tied to devices
so it's unlikely to ever get implemented for regular initcalls. And in
this case I really think making the driver removable is a good thing.
+};
+
+static const struct of_device_id soc_of_matches[] = {
+ { .compatible = nvidia,tegra124, },
+ {}
+};
+
+static int __init tegra_cpufreq_init(void)
+{
+ int ret;
+ struct platform_device *pdev;
+
+ if (!of_find_matching_node(NULL, soc_of_matches))
+ return -ENODEV;
I think this could be of_machine_is_compatible() since there's only a
single entry in the match table. If there's a good chance that we may
end up with more entries, perhaps now would be a good time to add an
of_match_machine() function?
I think this driver should work on Tegra132 without modifications.
of_match_machine() does sound useful for some of the other cpufreq
drivers as well and likely for your soc_is_tegra() from the PMC
series as well.
Yes, indeed. I'll give it a shot if you don't beat me to it with this
series.
Thierry
pgplM2WI6Ijtv.pgp
Description: PGP signature
Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124
On 23 July 2014 17:27, Tuomas Tynkkynen ttynkky...@nvidia.com wrote: The platform device is required for the deferred probe that can happen if the DFLL driver hasn't initialized yet, and module_init() callbacks don't seem to respect -EPROBE_DEFER. Oh, which call in this file will return EPROBE_DEFER? I couldn't make out which one will depend on DFLL driver. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124
On 23/07/14 19:50, Viresh Kumar wrote:
On 23 July 2014 17:27, Tuomas Tynkkynen ttynkky...@nvidia.com wrote:
The platform device is required for the deferred probe that can happen if the
DFLL driver hasn't initialized yet, and module_init() callbacks don't seem to
respect -EPROBE_DEFER.
Oh, which call in this file will return EPROBE_DEFER? I couldn't make out
which one will depend on DFLL driver.
It's this:
+static int tegra124_cpufreq_probe(struct platform_device *pdev)
+{
[...]
+
+ dfll_clk = of_clk_get_by_name(cpu_dev-of_node, dfll);
+ if (IS_ERR(dfll_clk)) {
+ ret = PTR_ERR(dfll_clk);
+ goto out_put_cpu_clk;
+ }
--
nvpublic
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124
On 24 July 2014 00:47, Tuomas Tynkkynen ttynkky...@nvidia.com wrote:
It's this:
+static int tegra124_cpufreq_probe(struct platform_device *pdev)
+{
[...]
+
+ dfll_clk = of_clk_get_by_name(cpu_dev-of_node, dfll);
+ if (IS_ERR(dfll_clk)) {
+ ret = PTR_ERR(dfll_clk);
+ goto out_put_cpu_clk;
+ }
This would search for clocks passed via DT, right? Why would we
get EPROBE_DEFER for that? Sorry for the stupid question.
--
viresh
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124
On 21 July 2014 21:09, Tuomas Tynkkynen wrote:
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 7364a53..df3c73e 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -244,6 +244,7 @@ config ARM_SPEAR_CPUFREQ
> config ARM_TEGRA_CPUFREQ
> bool "TEGRA CPUFreq support"
> depends on ARCH_TEGRA
> + depends on GENERIC_CPUFREQ_CPU0
Wouldn't this also disturb the existing cpufreq driver for earlier
tegra platforms? i.e. we don't need cpufreq-cpu0 for them
atleast as of now.
> default y
> help
> This adds the CPUFreq driver support for TEGRA SOCs.
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index db6d9a2..3437d24 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -76,6 +76,7 @@ obj-$(CONFIG_ARM_SA1100_CPUFREQ) += sa1100-cpufreq.o
> obj-$(CONFIG_ARM_SA1110_CPUFREQ) += sa1110-cpufreq.o
> obj-$(CONFIG_ARM_SPEAR_CPUFREQ)+= spear-cpufreq.o
> obj-$(CONFIG_ARM_TEGRA_CPUFREQ)+= tegra-cpufreq.o
> +obj-$(CONFIG_ARM_TEGRA_CPUFREQ)+= tegra124-cpufreq.o
Maybe, you can update the same line if you want.
> obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o
>
>
> ##
> diff --git a/drivers/cpufreq/tegra124-cpufreq.c
> b/drivers/cpufreq/tegra124-cpufreq.c
> +static struct cpufreq_frequency_table *freq_table;
> +
> +static struct device *cpu_dev;
> +static struct clk *cpu_clk;
> +static struct clk *pllp_clk;
> +static struct clk *pllx_clk;
> +static struct clk *dfll_clk;
The routines in this file are going to be called just once at boot, right?
In that case we are actually wasting some memory by creating globals.
Probably just move all these in a struct and allocate it at runtime.
> +static int tegra124_cpu_switch_to_dfll(void)
> +{
> + struct clk *original_cpu_clk_parent;
> + unsigned long rate;
> + struct dev_pm_opp *opp;
> + int ret;
> +
> + rate = clk_get_rate(cpu_clk);
> + opp = dev_pm_opp_find_freq_ceil(cpu_dev, );
> + if (IS_ERR(opp))
> + return PTR_ERR(opp);
> +
> + ret = clk_set_rate(dfll_clk, rate);
> + if (ret)
> + return ret;
> +
> + original_cpu_clk_parent = clk_get_parent(cpu_clk);
> + clk_set_parent(cpu_clk, pllp_clk);
> + if (ret)
> + return ret;
> +
> + ret = clk_prepare_enable(dfll_clk);
> + if (ret)
> + goto out_switch_to_original_parent;
> +
> + clk_set_parent(cpu_clk, dfll_clk);
> +
> + return 0;
> +
> +out_switch_to_original_parent:
> + clk_set_parent(cpu_clk, original_cpu_clk_parent);
> +
> + return ret;
> +}
> +
> +static struct platform_device_info cpufreq_cpu0_devinfo = {
> + .name = "cpufreq-cpu0",
> +};
> +
> +static int tegra124_cpufreq_probe(struct platform_device *pdev)
> +{
> + int ret;
> +
> + cpu_dev = get_cpu_device(0);
> + if (!cpu_dev)
> + return -ENODEV;
> +
Shouldn't we do a of_node_get() here?
> + cpu_clk = of_clk_get_by_name(cpu_dev->of_node, "cpu_g");
> + if (IS_ERR(cpu_clk))
> + return PTR_ERR(cpu_clk);
> +
> + dfll_clk = of_clk_get_by_name(cpu_dev->of_node, "dfll");
> + if (IS_ERR(dfll_clk)) {
> + ret = PTR_ERR(dfll_clk);
> + goto out_put_cpu_clk;
> + }
> +
> + pllx_clk = of_clk_get_by_name(cpu_dev->of_node, "pll_x");
> + if (IS_ERR(pllx_clk)) {
> + ret = PTR_ERR(pllx_clk);
> + goto out_put_dfll_clk;
> + }
> +
> + pllp_clk = of_clk_get_by_name(cpu_dev->of_node, "pll_p");
> + if (IS_ERR(pllp_clk)) {
> + ret = PTR_ERR(pllp_clk);
> + goto out_put_pllx_clk;
> + }
> +
> + ret = dev_pm_opp_init_cpufreq_table(cpu_dev, _table);
> + if (ret)
> + goto out_put_pllp_clk;
Why do you need this? cpufreq-cpu0 also does it and this freq_table is
just not getting used at all then.
> +
> + ret = tegra124_cpu_switch_to_dfll();
> + if (ret)
> + goto out_free_table;
> +
> + platform_device_register_full(_cpu0_devinfo);
> +
> + return 0;
> +
> +out_free_table:
> + dev_pm_opp_free_cpufreq_table(cpu_dev, _table);
> +out_put_pllp_clk:
> + clk_put(pllp_clk);
> +out_put_pllx_clk:
> + clk_put(pllx_clk);
> +out_put_dfll_clk:
> + clk_put(dfll_clk);
> +out_put_cpu_clk:
> + clk_put(cpu_clk);
> +
> + return ret;
> +}
> +
> +static struct platform_driver tegra124_cpufreq_platdrv = {
> + .driver = {
> + .name = "cpufreq-tegra124",
> + .owner = THIS_MODULE,
> + },
> + .probe = tegra124_cpufreq_probe,
> +};
> +
> +static const struct of_device_id soc_of_matches[] = {
> + {
Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124
On 21 July 2014 21:09, Tuomas Tynkkynen ttynkky...@nvidia.com wrote:
diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 7364a53..df3c73e 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -244,6 +244,7 @@ config ARM_SPEAR_CPUFREQ
config ARM_TEGRA_CPUFREQ
bool TEGRA CPUFreq support
depends on ARCH_TEGRA
+ depends on GENERIC_CPUFREQ_CPU0
Wouldn't this also disturb the existing cpufreq driver for earlier
tegra platforms? i.e. we don't need cpufreq-cpu0 for them
atleast as of now.
default y
help
This adds the CPUFreq driver support for TEGRA SOCs.
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index db6d9a2..3437d24 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -76,6 +76,7 @@ obj-$(CONFIG_ARM_SA1100_CPUFREQ) += sa1100-cpufreq.o
obj-$(CONFIG_ARM_SA1110_CPUFREQ) += sa1110-cpufreq.o
obj-$(CONFIG_ARM_SPEAR_CPUFREQ)+= spear-cpufreq.o
obj-$(CONFIG_ARM_TEGRA_CPUFREQ)+= tegra-cpufreq.o
+obj-$(CONFIG_ARM_TEGRA_CPUFREQ)+= tegra124-cpufreq.o
Maybe, you can update the same line if you want.
obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o
##
diff --git a/drivers/cpufreq/tegra124-cpufreq.c
b/drivers/cpufreq/tegra124-cpufreq.c
+static struct cpufreq_frequency_table *freq_table;
+
+static struct device *cpu_dev;
+static struct clk *cpu_clk;
+static struct clk *pllp_clk;
+static struct clk *pllx_clk;
+static struct clk *dfll_clk;
The routines in this file are going to be called just once at boot, right?
In that case we are actually wasting some memory by creating globals.
Probably just move all these in a struct and allocate it at runtime.
+static int tegra124_cpu_switch_to_dfll(void)
+{
+ struct clk *original_cpu_clk_parent;
+ unsigned long rate;
+ struct dev_pm_opp *opp;
+ int ret;
+
+ rate = clk_get_rate(cpu_clk);
+ opp = dev_pm_opp_find_freq_ceil(cpu_dev, rate);
+ if (IS_ERR(opp))
+ return PTR_ERR(opp);
+
+ ret = clk_set_rate(dfll_clk, rate);
+ if (ret)
+ return ret;
+
+ original_cpu_clk_parent = clk_get_parent(cpu_clk);
+ clk_set_parent(cpu_clk, pllp_clk);
+ if (ret)
+ return ret;
+
+ ret = clk_prepare_enable(dfll_clk);
+ if (ret)
+ goto out_switch_to_original_parent;
+
+ clk_set_parent(cpu_clk, dfll_clk);
+
+ return 0;
+
+out_switch_to_original_parent:
+ clk_set_parent(cpu_clk, original_cpu_clk_parent);
+
+ return ret;
+}
+
+static struct platform_device_info cpufreq_cpu0_devinfo = {
+ .name = cpufreq-cpu0,
+};
+
+static int tegra124_cpufreq_probe(struct platform_device *pdev)
+{
+ int ret;
+
+ cpu_dev = get_cpu_device(0);
+ if (!cpu_dev)
+ return -ENODEV;
+
Shouldn't we do a of_node_get() here?
+ cpu_clk = of_clk_get_by_name(cpu_dev-of_node, cpu_g);
+ if (IS_ERR(cpu_clk))
+ return PTR_ERR(cpu_clk);
+
+ dfll_clk = of_clk_get_by_name(cpu_dev-of_node, dfll);
+ if (IS_ERR(dfll_clk)) {
+ ret = PTR_ERR(dfll_clk);
+ goto out_put_cpu_clk;
+ }
+
+ pllx_clk = of_clk_get_by_name(cpu_dev-of_node, pll_x);
+ if (IS_ERR(pllx_clk)) {
+ ret = PTR_ERR(pllx_clk);
+ goto out_put_dfll_clk;
+ }
+
+ pllp_clk = of_clk_get_by_name(cpu_dev-of_node, pll_p);
+ if (IS_ERR(pllp_clk)) {
+ ret = PTR_ERR(pllp_clk);
+ goto out_put_pllx_clk;
+ }
+
+ ret = dev_pm_opp_init_cpufreq_table(cpu_dev, freq_table);
+ if (ret)
+ goto out_put_pllp_clk;
Why do you need this? cpufreq-cpu0 also does it and this freq_table is
just not getting used at all then.
+
+ ret = tegra124_cpu_switch_to_dfll();
+ if (ret)
+ goto out_free_table;
+
+ platform_device_register_full(cpufreq_cpu0_devinfo);
+
+ return 0;
+
+out_free_table:
+ dev_pm_opp_free_cpufreq_table(cpu_dev, freq_table);
+out_put_pllp_clk:
+ clk_put(pllp_clk);
+out_put_pllx_clk:
+ clk_put(pllx_clk);
+out_put_dfll_clk:
+ clk_put(dfll_clk);
+out_put_cpu_clk:
+ clk_put(cpu_clk);
+
+ return ret;
+}
+
+static struct platform_driver tegra124_cpufreq_platdrv = {
+ .driver = {
+ .name = cpufreq-tegra124,
+ .owner = THIS_MODULE,
+ },
+ .probe = tegra124_cpufreq_probe,
+};
+
+static const struct of_device_id soc_of_matches[] = {
+ { .compatible = nvidia,tegra124, },
+ {}
+};
+
+static int __init tegra_cpufreq_init(void)
+{
+ int ret;
Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124
On Monday, July 21, 2014 06:39:00 PM Tuomas Tynkkynen wrote:
> Add a new cpufreq driver for Tegra124. Instead of using the PLLX as
> the CPU clocksource, switch immediately to the DFLL. It allows the use
> of higher clock rates, and will automatically scale the CPU voltage as
> well. Besides the CPU clocksource switch, we let the cpufreq-cpu0 driver
> for all the cpufreq operations.
>
> This driver also relies on the DFLL driver to fill the OPP table for the
> CPU0 device, so that the cpufreq-cpu0 driver knows what frequencies to
> use.
>
> This driver is a completely independent of the old cpufreq driver
> (tegra-cpufreq), which is only used on Tegra20.
>
> Signed-off-by: Tuomas Tynkkynen
I'm leaving this to Viresh. If he AKCs it for you, please feel free to push
this through whatever tree you think is appropriate.
> ---
> v2 changes:
> - use the cpufreq-cpu0 driver
>
> drivers/cpufreq/Kconfig.arm| 1 +
> drivers/cpufreq/Makefile | 1 +
> drivers/cpufreq/tegra124-cpufreq.c | 169
> +
> 3 files changed, 171 insertions(+)
> create mode 100644 drivers/cpufreq/tegra124-cpufreq.c
>
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 7364a53..df3c73e 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -244,6 +244,7 @@ config ARM_SPEAR_CPUFREQ
> config ARM_TEGRA_CPUFREQ
> bool "TEGRA CPUFreq support"
> depends on ARCH_TEGRA
> + depends on GENERIC_CPUFREQ_CPU0
> default y
> help
> This adds the CPUFreq driver support for TEGRA SOCs.
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index db6d9a2..3437d24 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -76,6 +76,7 @@ obj-$(CONFIG_ARM_SA1100_CPUFREQ)+= sa1100-cpufreq.o
> obj-$(CONFIG_ARM_SA1110_CPUFREQ) += sa1110-cpufreq.o
> obj-$(CONFIG_ARM_SPEAR_CPUFREQ) += spear-cpufreq.o
> obj-$(CONFIG_ARM_TEGRA_CPUFREQ) += tegra-cpufreq.o
> +obj-$(CONFIG_ARM_TEGRA_CPUFREQ) += tegra124-cpufreq.o
> obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o
>
>
> ##
> diff --git a/drivers/cpufreq/tegra124-cpufreq.c
> b/drivers/cpufreq/tegra124-cpufreq.c
> new file mode 100644
> index 000..65ff428
> --- /dev/null
> +++ b/drivers/cpufreq/tegra124-cpufreq.c
> @@ -0,0 +1,169 @@
> +/*
> + * Tegra 124 cpufreq driver
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +static struct cpufreq_frequency_table *freq_table;
> +
> +static struct device *cpu_dev;
> +static struct clk *cpu_clk;
> +static struct clk *pllp_clk;
> +static struct clk *pllx_clk;
> +static struct clk *dfll_clk;
> +
> +static int tegra124_cpu_switch_to_dfll(void)
> +{
> + struct clk *original_cpu_clk_parent;
> + unsigned long rate;
> + struct dev_pm_opp *opp;
> + int ret;
> +
> + rate = clk_get_rate(cpu_clk);
> + opp = dev_pm_opp_find_freq_ceil(cpu_dev, );
> + if (IS_ERR(opp))
> + return PTR_ERR(opp);
> +
> + ret = clk_set_rate(dfll_clk, rate);
> + if (ret)
> + return ret;
> +
> + original_cpu_clk_parent = clk_get_parent(cpu_clk);
> + clk_set_parent(cpu_clk, pllp_clk);
> + if (ret)
> + return ret;
> +
> + ret = clk_prepare_enable(dfll_clk);
> + if (ret)
> + goto out_switch_to_original_parent;
> +
> + clk_set_parent(cpu_clk, dfll_clk);
> +
> + return 0;
> +
> +out_switch_to_original_parent:
> + clk_set_parent(cpu_clk, original_cpu_clk_parent);
> +
> + return ret;
> +}
> +
> +static struct platform_device_info cpufreq_cpu0_devinfo = {
> + .name = "cpufreq-cpu0",
> +};
> +
> +static int tegra124_cpufreq_probe(struct platform_device *pdev)
> +{
> + int ret;
> +
> + cpu_dev = get_cpu_device(0);
> + if (!cpu_dev)
> + return -ENODEV;
> +
> + cpu_clk = of_clk_get_by_name(cpu_dev->of_node, "cpu_g");
> + if (IS_ERR(cpu_clk))
> + return PTR_ERR(cpu_clk);
> +
> + dfll_clk = of_clk_get_by_name(cpu_dev->of_node, "dfll");
> + if (IS_ERR(dfll_clk)) {
> + ret = PTR_ERR(dfll_clk);
> + goto out_put_cpu_clk;
> + }
> +
> +
[PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124
Add a new cpufreq driver for Tegra124. Instead of using the PLLX as
the CPU clocksource, switch immediately to the DFLL. It allows the use
of higher clock rates, and will automatically scale the CPU voltage as
well. Besides the CPU clocksource switch, we let the cpufreq-cpu0 driver
for all the cpufreq operations.
This driver also relies on the DFLL driver to fill the OPP table for the
CPU0 device, so that the cpufreq-cpu0 driver knows what frequencies to
use.
This driver is a completely independent of the old cpufreq driver
(tegra-cpufreq), which is only used on Tegra20.
Signed-off-by: Tuomas Tynkkynen
---
v2 changes:
- use the cpufreq-cpu0 driver
drivers/cpufreq/Kconfig.arm| 1 +
drivers/cpufreq/Makefile | 1 +
drivers/cpufreq/tegra124-cpufreq.c | 169 +
3 files changed, 171 insertions(+)
create mode 100644 drivers/cpufreq/tegra124-cpufreq.c
diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 7364a53..df3c73e 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -244,6 +244,7 @@ config ARM_SPEAR_CPUFREQ
config ARM_TEGRA_CPUFREQ
bool "TEGRA CPUFreq support"
depends on ARCH_TEGRA
+ depends on GENERIC_CPUFREQ_CPU0
default y
help
This adds the CPUFreq driver support for TEGRA SOCs.
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index db6d9a2..3437d24 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -76,6 +76,7 @@ obj-$(CONFIG_ARM_SA1100_CPUFREQ) += sa1100-cpufreq.o
obj-$(CONFIG_ARM_SA1110_CPUFREQ) += sa1110-cpufreq.o
obj-$(CONFIG_ARM_SPEAR_CPUFREQ)+= spear-cpufreq.o
obj-$(CONFIG_ARM_TEGRA_CPUFREQ)+= tegra-cpufreq.o
+obj-$(CONFIG_ARM_TEGRA_CPUFREQ)+= tegra124-cpufreq.o
obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o
##
diff --git a/drivers/cpufreq/tegra124-cpufreq.c
b/drivers/cpufreq/tegra124-cpufreq.c
new file mode 100644
index 000..65ff428
--- /dev/null
+++ b/drivers/cpufreq/tegra124-cpufreq.c
@@ -0,0 +1,169 @@
+/*
+ * Tegra 124 cpufreq driver
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#define pr_fmt(fmt)KBUILD_MODNAME ": " fmt
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+static struct cpufreq_frequency_table *freq_table;
+
+static struct device *cpu_dev;
+static struct clk *cpu_clk;
+static struct clk *pllp_clk;
+static struct clk *pllx_clk;
+static struct clk *dfll_clk;
+
+static int tegra124_cpu_switch_to_dfll(void)
+{
+ struct clk *original_cpu_clk_parent;
+ unsigned long rate;
+ struct dev_pm_opp *opp;
+ int ret;
+
+ rate = clk_get_rate(cpu_clk);
+ opp = dev_pm_opp_find_freq_ceil(cpu_dev, );
+ if (IS_ERR(opp))
+ return PTR_ERR(opp);
+
+ ret = clk_set_rate(dfll_clk, rate);
+ if (ret)
+ return ret;
+
+ original_cpu_clk_parent = clk_get_parent(cpu_clk);
+ clk_set_parent(cpu_clk, pllp_clk);
+ if (ret)
+ return ret;
+
+ ret = clk_prepare_enable(dfll_clk);
+ if (ret)
+ goto out_switch_to_original_parent;
+
+ clk_set_parent(cpu_clk, dfll_clk);
+
+ return 0;
+
+out_switch_to_original_parent:
+ clk_set_parent(cpu_clk, original_cpu_clk_parent);
+
+ return ret;
+}
+
+static struct platform_device_info cpufreq_cpu0_devinfo = {
+ .name = "cpufreq-cpu0",
+};
+
+static int tegra124_cpufreq_probe(struct platform_device *pdev)
+{
+ int ret;
+
+ cpu_dev = get_cpu_device(0);
+ if (!cpu_dev)
+ return -ENODEV;
+
+ cpu_clk = of_clk_get_by_name(cpu_dev->of_node, "cpu_g");
+ if (IS_ERR(cpu_clk))
+ return PTR_ERR(cpu_clk);
+
+ dfll_clk = of_clk_get_by_name(cpu_dev->of_node, "dfll");
+ if (IS_ERR(dfll_clk)) {
+ ret = PTR_ERR(dfll_clk);
+ goto out_put_cpu_clk;
+ }
+
+ pllx_clk = of_clk_get_by_name(cpu_dev->of_node, "pll_x");
+ if (IS_ERR(pllx_clk)) {
+ ret = PTR_ERR(pllx_clk);
+ goto out_put_dfll_clk;
+ }
+
+ pllp_clk = of_clk_get_by_name(cpu_dev->of_node, "pll_p");
+ if (IS_ERR(pllp_clk)) {
+ ret = PTR_ERR(pllp_clk);
+ goto out_put_pllx_clk;
+ }
+
+ ret =
[PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124
Add a new cpufreq driver for Tegra124. Instead of using the PLLX as
the CPU clocksource, switch immediately to the DFLL. It allows the use
of higher clock rates, and will automatically scale the CPU voltage as
well. Besides the CPU clocksource switch, we let the cpufreq-cpu0 driver
for all the cpufreq operations.
This driver also relies on the DFLL driver to fill the OPP table for the
CPU0 device, so that the cpufreq-cpu0 driver knows what frequencies to
use.
This driver is a completely independent of the old cpufreq driver
(tegra-cpufreq), which is only used on Tegra20.
Signed-off-by: Tuomas Tynkkynen ttynkky...@nvidia.com
---
v2 changes:
- use the cpufreq-cpu0 driver
drivers/cpufreq/Kconfig.arm| 1 +
drivers/cpufreq/Makefile | 1 +
drivers/cpufreq/tegra124-cpufreq.c | 169 +
3 files changed, 171 insertions(+)
create mode 100644 drivers/cpufreq/tegra124-cpufreq.c
diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 7364a53..df3c73e 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -244,6 +244,7 @@ config ARM_SPEAR_CPUFREQ
config ARM_TEGRA_CPUFREQ
bool TEGRA CPUFreq support
depends on ARCH_TEGRA
+ depends on GENERIC_CPUFREQ_CPU0
default y
help
This adds the CPUFreq driver support for TEGRA SOCs.
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index db6d9a2..3437d24 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -76,6 +76,7 @@ obj-$(CONFIG_ARM_SA1100_CPUFREQ) += sa1100-cpufreq.o
obj-$(CONFIG_ARM_SA1110_CPUFREQ) += sa1110-cpufreq.o
obj-$(CONFIG_ARM_SPEAR_CPUFREQ)+= spear-cpufreq.o
obj-$(CONFIG_ARM_TEGRA_CPUFREQ)+= tegra-cpufreq.o
+obj-$(CONFIG_ARM_TEGRA_CPUFREQ)+= tegra124-cpufreq.o
obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o
##
diff --git a/drivers/cpufreq/tegra124-cpufreq.c
b/drivers/cpufreq/tegra124-cpufreq.c
new file mode 100644
index 000..65ff428
--- /dev/null
+++ b/drivers/cpufreq/tegra124-cpufreq.c
@@ -0,0 +1,169 @@
+/*
+ * Tegra 124 cpufreq driver
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#define pr_fmt(fmt)KBUILD_MODNAME : fmt
+
+#include linux/clk.h
+#include linux/cpufreq.h
+#include linux/cpu.h
+#include linux/err.h
+#include linux/init.h
+#include linux/kernel.h
+#include linux/module.h
+#include linux/of.h
+#include linux/platform_device.h
+#include linux/pm_opp.h
+#include linux/types.h
+
+static struct cpufreq_frequency_table *freq_table;
+
+static struct device *cpu_dev;
+static struct clk *cpu_clk;
+static struct clk *pllp_clk;
+static struct clk *pllx_clk;
+static struct clk *dfll_clk;
+
+static int tegra124_cpu_switch_to_dfll(void)
+{
+ struct clk *original_cpu_clk_parent;
+ unsigned long rate;
+ struct dev_pm_opp *opp;
+ int ret;
+
+ rate = clk_get_rate(cpu_clk);
+ opp = dev_pm_opp_find_freq_ceil(cpu_dev, rate);
+ if (IS_ERR(opp))
+ return PTR_ERR(opp);
+
+ ret = clk_set_rate(dfll_clk, rate);
+ if (ret)
+ return ret;
+
+ original_cpu_clk_parent = clk_get_parent(cpu_clk);
+ clk_set_parent(cpu_clk, pllp_clk);
+ if (ret)
+ return ret;
+
+ ret = clk_prepare_enable(dfll_clk);
+ if (ret)
+ goto out_switch_to_original_parent;
+
+ clk_set_parent(cpu_clk, dfll_clk);
+
+ return 0;
+
+out_switch_to_original_parent:
+ clk_set_parent(cpu_clk, original_cpu_clk_parent);
+
+ return ret;
+}
+
+static struct platform_device_info cpufreq_cpu0_devinfo = {
+ .name = cpufreq-cpu0,
+};
+
+static int tegra124_cpufreq_probe(struct platform_device *pdev)
+{
+ int ret;
+
+ cpu_dev = get_cpu_device(0);
+ if (!cpu_dev)
+ return -ENODEV;
+
+ cpu_clk = of_clk_get_by_name(cpu_dev-of_node, cpu_g);
+ if (IS_ERR(cpu_clk))
+ return PTR_ERR(cpu_clk);
+
+ dfll_clk = of_clk_get_by_name(cpu_dev-of_node, dfll);
+ if (IS_ERR(dfll_clk)) {
+ ret = PTR_ERR(dfll_clk);
+ goto out_put_cpu_clk;
+ }
+
+ pllx_clk = of_clk_get_by_name(cpu_dev-of_node, pll_x);
+ if (IS_ERR(pllx_clk)) {
+ ret = PTR_ERR(pllx_clk);
+ goto out_put_dfll_clk;
+ }
+
+ pllp_clk = of_clk_get_by_name(cpu_dev-of_node, pll_p);
+
Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124
On Monday, July 21, 2014 06:39:00 PM Tuomas Tynkkynen wrote:
Add a new cpufreq driver for Tegra124. Instead of using the PLLX as
the CPU clocksource, switch immediately to the DFLL. It allows the use
of higher clock rates, and will automatically scale the CPU voltage as
well. Besides the CPU clocksource switch, we let the cpufreq-cpu0 driver
for all the cpufreq operations.
This driver also relies on the DFLL driver to fill the OPP table for the
CPU0 device, so that the cpufreq-cpu0 driver knows what frequencies to
use.
This driver is a completely independent of the old cpufreq driver
(tegra-cpufreq), which is only used on Tegra20.
Signed-off-by: Tuomas Tynkkynen ttynkky...@nvidia.com
I'm leaving this to Viresh. If he AKCs it for you, please feel free to push
this through whatever tree you think is appropriate.
---
v2 changes:
- use the cpufreq-cpu0 driver
drivers/cpufreq/Kconfig.arm| 1 +
drivers/cpufreq/Makefile | 1 +
drivers/cpufreq/tegra124-cpufreq.c | 169
+
3 files changed, 171 insertions(+)
create mode 100644 drivers/cpufreq/tegra124-cpufreq.c
diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 7364a53..df3c73e 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -244,6 +244,7 @@ config ARM_SPEAR_CPUFREQ
config ARM_TEGRA_CPUFREQ
bool TEGRA CPUFreq support
depends on ARCH_TEGRA
+ depends on GENERIC_CPUFREQ_CPU0
default y
help
This adds the CPUFreq driver support for TEGRA SOCs.
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index db6d9a2..3437d24 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -76,6 +76,7 @@ obj-$(CONFIG_ARM_SA1100_CPUFREQ)+= sa1100-cpufreq.o
obj-$(CONFIG_ARM_SA1110_CPUFREQ) += sa1110-cpufreq.o
obj-$(CONFIG_ARM_SPEAR_CPUFREQ) += spear-cpufreq.o
obj-$(CONFIG_ARM_TEGRA_CPUFREQ) += tegra-cpufreq.o
+obj-$(CONFIG_ARM_TEGRA_CPUFREQ) += tegra124-cpufreq.o
obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o
##
diff --git a/drivers/cpufreq/tegra124-cpufreq.c
b/drivers/cpufreq/tegra124-cpufreq.c
new file mode 100644
index 000..65ff428
--- /dev/null
+++ b/drivers/cpufreq/tegra124-cpufreq.c
@@ -0,0 +1,169 @@
+/*
+ * Tegra 124 cpufreq driver
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME : fmt
+
+#include linux/clk.h
+#include linux/cpufreq.h
+#include linux/cpu.h
+#include linux/err.h
+#include linux/init.h
+#include linux/kernel.h
+#include linux/module.h
+#include linux/of.h
+#include linux/platform_device.h
+#include linux/pm_opp.h
+#include linux/types.h
+
+static struct cpufreq_frequency_table *freq_table;
+
+static struct device *cpu_dev;
+static struct clk *cpu_clk;
+static struct clk *pllp_clk;
+static struct clk *pllx_clk;
+static struct clk *dfll_clk;
+
+static int tegra124_cpu_switch_to_dfll(void)
+{
+ struct clk *original_cpu_clk_parent;
+ unsigned long rate;
+ struct dev_pm_opp *opp;
+ int ret;
+
+ rate = clk_get_rate(cpu_clk);
+ opp = dev_pm_opp_find_freq_ceil(cpu_dev, rate);
+ if (IS_ERR(opp))
+ return PTR_ERR(opp);
+
+ ret = clk_set_rate(dfll_clk, rate);
+ if (ret)
+ return ret;
+
+ original_cpu_clk_parent = clk_get_parent(cpu_clk);
+ clk_set_parent(cpu_clk, pllp_clk);
+ if (ret)
+ return ret;
+
+ ret = clk_prepare_enable(dfll_clk);
+ if (ret)
+ goto out_switch_to_original_parent;
+
+ clk_set_parent(cpu_clk, dfll_clk);
+
+ return 0;
+
+out_switch_to_original_parent:
+ clk_set_parent(cpu_clk, original_cpu_clk_parent);
+
+ return ret;
+}
+
+static struct platform_device_info cpufreq_cpu0_devinfo = {
+ .name = cpufreq-cpu0,
+};
+
+static int tegra124_cpufreq_probe(struct platform_device *pdev)
+{
+ int ret;
+
+ cpu_dev = get_cpu_device(0);
+ if (!cpu_dev)
+ return -ENODEV;
+
+ cpu_clk = of_clk_get_by_name(cpu_dev-of_node, cpu_g);
+ if (IS_ERR(cpu_clk))
+ return PTR_ERR(cpu_clk);
+
+ dfll_clk = of_clk_get_by_name(cpu_dev-of_node, dfll);
+ if (IS_ERR(dfll_clk)) {
+ ret = PTR_ERR(dfll_clk);
+ goto out_put_cpu_clk;
+ }
+
+
