by Viresh Kumar

[permalink] [raw]

Subject: Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124

On 21 July 2014 21:09, Tuomas Tynkkynen <[email protected]> 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;
> + struct platform_device *pdev;
> +
> + if (!of_find_matching_node(NULL, soc_of_matches))
> + return -ENODEV;
> +
> + 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);
> + }

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.

> + return 0;
> +}
> +
> +MODULE_AUTHOR("Tuomas Tynkkynen <[email protected]>");
> +MODULE_DESCRIPTION("cpufreq driver for nVIDIA Tegra124");
> +MODULE_LICENSE("GPLv2");
> +module_init(tegra_cpufreq_init);
> --
> 1.8.1.5
>

2014-07-23 06:54:21

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 <[email protected]>");
> +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

Attachments:

(No filename) (4.48 kB)
(No filename) (819.00 B)
Download all attachments

2014-07-23 07:21:48

by Pramod Gurav

[permalink] [raw]

Subject: Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124

On Mon, Jul 21, 2014 at 9:09 PM, Tuomas Tynkkynen <[email protected]> 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

2014-07-23 07:24:51

On Wed, Jul 23, 2014 at 01:55:57PM +0530, Viresh Kumar wrote:
> On 23 July 2014 12:54, Thierry Reding <[email protected]> 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

Attachments:

(No filename) (1.99 kB)
(No filename) (819.00 B)
Download all attachments

2014-07-23 13:59:29

On Mon, Jul 21, 2014 at 06:38:48PM +0300, Tuomas Tynkkynen wrote:

> Add helper functions that allow regulator consumers to obtain low-level
> details about the regulator hardware, like the voltage selector register
> address and such. These details can be useful when configuring hardware
> or firmware that want to do low-level access to regulators, with no
> involvement from the kernel.

Applied, thanks. I've also created a signed tag for this one:

The following changes since commit 7171511eaec5bf23fb06078f59784a3a0626b38f:

Linux 3.16-rc1 (2014-06-15 17:45:28 -1000)

are available in the git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git tags/getreg

for you to fetch changes up to 04eca28cde52cdf9eb91e127cc358ad79a8ec53b:

regulator: Add helpers for low-level register access (2014-07-25 18:43:48 +0100)

----------------------------------------------------------------
regulator: Allow clients to retrieve voltage selection register
information

Some systems have embedded controllers that can automatically manage the
voltage of a regulator. Help client drivers support this in a generic
fashion by providing access to the register information we have in the
framework.

----------------------------------------------------------------
Tuomas Tynkkynen (1):
regulator: Add helpers for low-level register access

Documentation/power/regulator/consumer.txt | 35 +++++++++++++++
drivers/regulator/core.c | 71 ++++++++++++++++++++++++++++++
include/linux/regulator/consumer.h | 26 +++++++++++
3 files changed, 132 insertions(+)

Attachments:

(No filename) (1.58 kB)
signature.asc (819.00 B)
Digital signature Download all attachments