[v6,08/18] cpufreq: exynos: Use device tree to determine if cpufreq cooling should be registered
diff mbox

Message ID 1422015260-14225-1-git-send-email-l.majewski@samsung.com
State Accepted
Delegated to: Eduardo Valentin
Headers show

Commit Message

Lukasz Majewski Jan. 23, 2015, 12:14 p.m. UTC
With thermal subsystem rework it is necessary to tune current cpufreq code
to use cpu frequency change as a potential cooling device.

Now the cpu cooling device is registered only when proper nodes and properties
are available in device tree. Lack of them, however, will not prevent
cpufreq for normal operation.

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
---
Changes for v6:
- New patch

---
 drivers/cpufreq/exynos-cpufreq.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

Comments

Viresh Kumar Jan. 23, 2015, 12:59 p.m. UTC | #1
On 23 January 2015 at 17:44, Lukasz Majewski <l.majewski@samsung.com> wrote:
> +       cpus = of_find_node_by_path("/cpus");
> +       if (!cpus) {
> +               pr_err("failed to find cpus node\n");
> +               return 0;
> +       }
> +
> +       np = of_get_next_child(cpus, NULL);
> +       if (!np) {
> +               pr_err("failed to find cpus child node\n");
> +               of_node_put(cpus);
>                 return 0;
> +       }

Why making it complex? Just get device node for cpu 0 and
do cpu_dev->np.
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lukasz Majewski Jan. 23, 2015, 1:57 p.m. UTC | #2
Hi Viresh,

> On 23 January 2015 at 17:44, Lukasz Majewski <l.majewski@samsung.com>
> wrote:
> > +       cpus = of_find_node_by_path("/cpus");
> > +       if (!cpus) {
> > +               pr_err("failed to find cpus node\n");
> > +               return 0;
> > +       }
> > +
> > +       np = of_get_next_child(cpus, NULL);
> > +       if (!np) {
> > +               pr_err("failed to find cpus child node\n");
> > +               of_node_put(cpus);
> >                 return 0;
> > +       }
> 
> Why making it complex? Just get device node for cpu 0 and
> do cpu_dev->np.

Please pay a note about following problem:

Previously we got: cpu0: cpu@0 for all Exynos devices.

Now, however, cpu numbering has changed (due to GIC rework).
For example:

Exynos4412:
	cpus {
		cpu0: cpu@A00 {
			...
			#cooling-cells = <2>; /* min followed by max */
		};

		cpu@A01 {
		};

		cpu@A02 {
		};

		cpu@A03 {
		};
	}

Exynos 4210:
	cpus {
		cpu0: cpu@900 {
			#cooling-cells = <2>; /* min followed by max */
		};

		cpu@901 {
		};
	};

Exynos 5250:
	cpus {
		cpu0: cpu@0 {
			#cooling-cells = <2>; /* min followed by max */
		};

		cpu@1 {
		};
	};
	

As you can see different cpu@XXY nodes we have and simply calling cpu@0
won't work.
Viresh Kumar Jan. 25, 2015, 2:01 p.m. UTC | #3
On 23 January 2015 at 19:27, Lukasz Majewski <l.majewski@samsung.com> wrote:
> Please pay a note about following problem:
>
> Previously we got: cpu0: cpu@0 for all Exynos devices.
>
> Now, however, cpu numbering has changed (due to GIC rework).
> For example:
>
> Exynos4412:
>         cpus {
>                 cpu0: cpu@A00 {
>                         ...
>                         #cooling-cells = <2>; /* min followed by max */
>                 };
>
>                 cpu@A01 {
>                 };
>
>                 cpu@A02 {
>                 };
>
>                 cpu@A03 {
>                 };
>         }
>
> Exynos 4210:
>         cpus {
>                 cpu0: cpu@900 {
>                         #cooling-cells = <2>; /* min followed by max */
>                 };
>
>                 cpu@901 {
>                 };
>         };
>
> Exynos 5250:
>         cpus {
>                 cpu0: cpu@0 {
>                         #cooling-cells = <2>; /* min followed by max */
>                 };
>
>                 cpu@1 {
>                 };
>         };
>
>
> As you can see different cpu@XXY nodes we have and simply calling cpu@0
> won't work.

I wasn't asked you to get the cpu0 node from dt but this:

cpu_dev = get_cpu_dev(0);
np = of_node_get(cpu_dev->of_node);

Wouldn't this work? You only need to guarantee that the cooling-cells is added
onto the boot CPUs node.
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lukasz Majewski Jan. 25, 2015, 4:27 p.m. UTC | #4
On Sun, 25 Jan 2015 19:31:14 +0530
Viresh Kumar <viresh.kumar@linaro.org> wrote:

> On 23 January 2015 at 19:27, Lukasz Majewski <l.majewski@samsung.com>
> wrote:
> > Please pay a note about following problem:
> >
> > Previously we got: cpu0: cpu@0 for all Exynos devices.
> >
> > Now, however, cpu numbering has changed (due to GIC rework).
> > For example:
> >
> > Exynos4412:
> >         cpus {
> >                 cpu0: cpu@A00 {
> >                         ...
> >                         #cooling-cells = <2>; /* min followed by
> > max */ };
> >
> >                 cpu@A01 {
> >                 };
> >
> >                 cpu@A02 {
> >                 };
> >
> >                 cpu@A03 {
> >                 };
> >         }
> >
> > Exynos 4210:
> >         cpus {
> >                 cpu0: cpu@900 {
> >                         #cooling-cells = <2>; /* min followed by
> > max */ };
> >
> >                 cpu@901 {
> >                 };
> >         };
> >
> > Exynos 5250:
> >         cpus {
> >                 cpu0: cpu@0 {
> >                         #cooling-cells = <2>; /* min followed by
> > max */ };
> >
> >                 cpu@1 {
> >                 };
> >         };
> >
> >
> > As you can see different cpu@XXY nodes we have and simply calling
> > cpu@0 won't work.
> 
> I wasn't asked you to get the cpu0 node from dt but this:
> 
> cpu_dev = get_cpu_dev(0);
> np = of_node_get(cpu_dev->of_node);

Ingenious simplicity :-)

> 
> Wouldn't this work? You only need to guarantee that the cooling-cells
> is added onto the boot CPUs node.

I will test it tomorrow and share results.

Viresh, thanks for tip.

Best regards,
Lukasz Majewski
Eduardo Valentin Jan. 25, 2015, 4:46 p.m. UTC | #5
On Sun, Jan 25, 2015 at 07:31:14PM +0530, Viresh Kumar wrote:
> On 23 January 2015 at 19:27, Lukasz Majewski <l.majewski@samsung.com> wrote:
> > Please pay a note about following problem:
> >
> > Previously we got: cpu0: cpu@0 for all Exynos devices.
> >
> > Now, however, cpu numbering has changed (due to GIC rework).
> > For example:
> >
> > Exynos4412:
> >         cpus {
> >                 cpu0: cpu@A00 {
> >                         ...
> >                         #cooling-cells = <2>; /* min followed by max */
> >                 };
> >
> >                 cpu@A01 {
> >                 };
> >
> >                 cpu@A02 {
> >                 };
> >
> >                 cpu@A03 {
> >                 };
> >         }
> >
> > Exynos 4210:
> >         cpus {
> >                 cpu0: cpu@900 {
> >                         #cooling-cells = <2>; /* min followed by max */
> >                 };
> >
> >                 cpu@901 {
> >                 };
> >         };
> >
> > Exynos 5250:
> >         cpus {
> >                 cpu0: cpu@0 {
> >                         #cooling-cells = <2>; /* min followed by max */
> >                 };
> >
> >                 cpu@1 {
> >                 };
> >         };
> >
> >
> > As you can see different cpu@XXY nodes we have and simply calling cpu@0
> > won't work.
> 
> I wasn't asked you to get the cpu0 node from dt but this:
> 
> cpu_dev = get_cpu_dev(0);
> np = of_node_get(cpu_dev->of_node);
> 
> Wouldn't this work? You only need to guarantee that the cooling-cells is added
> onto the boot CPUs node.

Lukasz,

I agree with Viresh here, you can simplify your code.

I, somehow, missed this conversation and already applied v6 of this
patch in my -fixes branch. Can you please fix this by sending a
differential patch on top of this one applying Viresh's commit?

Viresh, my bad, I missed your comments.

Thanks


Eduardo
Lukasz Majewski Jan. 25, 2015, 9:55 p.m. UTC | #6
On Sun, 25 Jan 2015 12:46:21 -0400
Eduardo Valentin <edubezval@gmail.com> wrote:

> On Sun, Jan 25, 2015 at 07:31:14PM +0530, Viresh Kumar wrote:
> > On 23 January 2015 at 19:27, Lukasz Majewski
> > <l.majewski@samsung.com> wrote:
> > > Please pay a note about following problem:
> > >
> > > Previously we got: cpu0: cpu@0 for all Exynos devices.
> > >
> > > Now, however, cpu numbering has changed (due to GIC rework).
> > > For example:
> > >
> > > Exynos4412:
> > >         cpus {
> > >                 cpu0: cpu@A00 {
> > >                         ...
> > >                         #cooling-cells = <2>; /* min followed by
> > > max */ };
> > >
> > >                 cpu@A01 {
> > >                 };
> > >
> > >                 cpu@A02 {
> > >                 };
> > >
> > >                 cpu@A03 {
> > >                 };
> > >         }
> > >
> > > Exynos 4210:
> > >         cpus {
> > >                 cpu0: cpu@900 {
> > >                         #cooling-cells = <2>; /* min followed by
> > > max */ };
> > >
> > >                 cpu@901 {
> > >                 };
> > >         };
> > >
> > > Exynos 5250:
> > >         cpus {
> > >                 cpu0: cpu@0 {
> > >                         #cooling-cells = <2>; /* min followed by
> > > max */ };
> > >
> > >                 cpu@1 {
> > >                 };
> > >         };
> > >
> > >
> > > As you can see different cpu@XXY nodes we have and simply calling
> > > cpu@0 won't work.
> > 
> > I wasn't asked you to get the cpu0 node from dt but this:
> > 
> > cpu_dev = get_cpu_dev(0);
> > np = of_node_get(cpu_dev->of_node);
> > 
> > Wouldn't this work? You only need to guarantee that the
> > cooling-cells is added onto the boot CPUs node.
> 
> Lukasz,
> 
> I agree with Viresh here, you can simplify your code.
> 
> I, somehow, missed this conversation and already applied v6 of this
> patch in my -fixes branch. Can you please fix this by sending a
> differential patch on top of this one applying Viresh's commit?

No problem, I will check this in my office tomorrow.

Thanks.

> 
> Viresh, my bad, I missed your comments.
> 
> Thanks
> 
> 
> Eduardo

Patch
diff mbox

diff --git a/drivers/cpufreq/exynos-cpufreq.c b/drivers/cpufreq/exynos-cpufreq.c
index f99a0b0..5e98c6b 100644
--- a/drivers/cpufreq/exynos-cpufreq.c
+++ b/drivers/cpufreq/exynos-cpufreq.c
@@ -18,10 +18,13 @@ 
 #include <linux/cpufreq.h>
 #include <linux/platform_device.h>
 #include <linux/of.h>
+#include <linux/cpu_cooling.h>
+#include <linux/cpu.h>
 
 #include "exynos-cpufreq.h"
 
 static struct exynos_dvfs_info *exynos_info;
+static struct thermal_cooling_device *cdev;
 static struct regulator *arm_regulator;
 static unsigned int locking_frequency;
 
@@ -156,6 +159,7 @@  static struct cpufreq_driver exynos_driver = {
 
 static int exynos_cpufreq_probe(struct platform_device *pdev)
 {
+	struct device_node *cpus, *np;
 	int ret = -EINVAL;
 
 	exynos_info = kzalloc(sizeof(*exynos_info), GFP_KERNEL);
@@ -198,9 +202,36 @@  static int exynos_cpufreq_probe(struct platform_device *pdev)
 	/* Done here as we want to capture boot frequency */
 	locking_frequency = clk_get_rate(exynos_info->cpu_clk) / 1000;
 
-	if (!cpufreq_register_driver(&exynos_driver))
+	ret = cpufreq_register_driver(&exynos_driver);
+	if (ret)
+		goto err_cpufreq_reg;
+
+	cpus = of_find_node_by_path("/cpus");
+	if (!cpus) {
+		pr_err("failed to find cpus node\n");
+		return 0;
+	}
+
+	np = of_get_next_child(cpus, NULL);
+	if (!np) {
+		pr_err("failed to find cpus child node\n");
+		of_node_put(cpus);
 		return 0;
+	}
+
+	if (of_find_property(np, "#cooling-cells", NULL)) {
+		cdev = of_cpufreq_cooling_register(np,
+						   cpu_present_mask);
+		if (IS_ERR(cdev))
+			pr_err("running cpufreq without cooling device: %ld\n",
+			       PTR_ERR(cdev));
+	}
+	of_node_put(np);
+	of_node_put(cpus);
+
+	return 0;
 
+err_cpufreq_reg:
 	dev_err(&pdev->dev, "failed to register cpufreq driver\n");
 	regulator_put(arm_regulator);
 err_vdd_arm: