* Re: [PATCH v4 12/19] cpufreq: cpufreq-cpu0: remove device tree parsing for cpu nodes
[not found] ` <1376991021-12160-13-git-send-email-Sudeep.KarkadaNagesha@arm.com>
@ 2013-09-06 13:44 ` Guennadi Liakhovetski
2013-09-09 9:24 ` Sudeep KarkadaNagesha
0 siblings, 1 reply; 7+ messages in thread
From: Guennadi Liakhovetski @ 2013-09-06 13:44 UTC (permalink / raw)
To: Sudeep KarkadaNagesha
Cc: linux-arm-kernel, linux-kernel, linux-pm, devicetree,
linuxppc-dev, Viresh Kumar, Greg Kroah-Hartman,
Benjamin Herrenschmidt, Jonas Bonn, Michal Simek,
Rafael J. Wysocki, Grant Likely, Rob Herring
Hi
On Tue, 20 Aug 2013, Sudeep KarkadaNagesha wrote:
> From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
>
> Now that the cpu device registration initialises the of_node(if available)
> appropriately for all the cpus, parsing here is redundant.
>
> This patch removes all DT parsing and uses cpu->of_node instead.
>
> Acked-by: Shawn Guo <shawn.guo@linaro.org>
> Acked-by: Rob Herring <rob.herring@calxeda.com>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
> ---
> drivers/cpufreq/cpufreq-cpu0.c | 23 ++++-------------------
> 1 file changed, 4 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.=
> c
> index ad1fde2..5b05c26 100644
> --- a/drivers/cpufreq/cpufreq-cpu0.c
> +++ b/drivers/cpufreq/cpufreq-cpu0.c
> @@ -174,29 +174,17 @@ static struct cpufreq_driver cpu0_cpufreq_driver =3D =
> {
> =20
> static int cpu0_cpufreq_probe(struct platform_device *pdev)
> {
> -=09struct device_node *np, *parent;
> +=09struct device_node *np;
> =09int ret;
> =20
> -=09parent =3D of_find_node_by_path("/cpus");
> -=09if (!parent) {
> -=09=09pr_err("failed to find OF /cpus\n");
> -=09=09return -ENOENT;
> -=09}
> -
> -=09for_each_child_of_node(parent, np) {
> -=09=09if (of_get_property(np, "operating-points", NULL))
> -=09=09=09break;
> -=09}
> +=09cpu_dev =3D &pdev->dev;
> =20
> +=09np =3D of_node_get(cpu_dev->of_node);
Has this actually been tested? This seems to break cpufreq-cpu0. The
reason is, that this probe function is called not for the DT CPU node, but
for a special virtual cpufreq-cpu0 platform device, typically created by
platforms, using
platform_device_register_simple("cpufreq-cpu0", -1, NULL, 0);
which then of course doesn't have on .of_node associated with it.
Thanks
Guennadi
> =09if (!np) {
> =09=09pr_err("failed to find cpu0 node\n");
> -=09=09ret =3D -ENOENT;
> -=09=09goto out_put_parent;
> +=09=09return -ENOENT;
> =09}
> =20
> -=09cpu_dev =3D &pdev->dev;
> -=09cpu_dev->of_node =3D np;
> -
> =09cpu_reg =3D devm_regulator_get(cpu_dev, "cpu0");
> =09if (IS_ERR(cpu_reg)) {
> =09=09/*
> @@ -269,15 +257,12 @@ static int cpu0_cpufreq_probe(struct platform_device =
> *pdev)
> =09}
> =20
> =09of_node_put(np);
> -=09of_node_put(parent);
> =09return 0;
> =20
> out_free_table:
> =09opp_free_cpufreq_table(cpu_dev, &freq_table);
> out_put_node:
> =09of_node_put(np);
> -out_put_parent:
> -=09of_node_put(parent);
> =09return ret;
> }
> =20
> --=20
> 1.8.1.2
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 12/19] cpufreq: cpufreq-cpu0: remove device tree parsing for cpu nodes
2013-09-06 13:44 ` [PATCH v4 12/19] cpufreq: cpufreq-cpu0: remove device tree parsing for cpu nodes Guennadi Liakhovetski
@ 2013-09-09 9:24 ` Sudeep KarkadaNagesha
2013-09-09 14:32 ` Shawn Guo
0 siblings, 1 reply; 7+ messages in thread
From: Sudeep KarkadaNagesha @ 2013-09-09 9:24 UTC (permalink / raw)
To: Guennadi Liakhovetski, Shawn Guo
Cc: Sudeep KarkadaNagesha, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
devicetree@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
Viresh Kumar, Greg Kroah-Hartman, Benjamin Herrenschmidt,
Jonas Bonn, Michal Simek, Rafael J. Wysocki,
grant.likely@linaro.org, rob.herring@calxeda.com
On 06/09/13 14:44, Guennadi Liakhovetski wrote:
> Hi
>
> On Tue, 20 Aug 2013, Sudeep KarkadaNagesha wrote:
>
>> From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
>>
>> Now that the cpu device registration initialises the of_node(if available)
>> appropriately for all the cpus, parsing here is redundant.
>>
>> This patch removes all DT parsing and uses cpu->of_node instead.
>>
>> Acked-by: Shawn Guo <shawn.guo@linaro.org>
>> Acked-by: Rob Herring <rob.herring@calxeda.com>
>> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>> Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
>> ---
>> drivers/cpufreq/cpufreq-cpu0.c | 23 ++++-------------------
>> 1 file changed, 4 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.=
>> c
>> index ad1fde2..5b05c26 100644
>> --- a/drivers/cpufreq/cpufreq-cpu0.c
>> +++ b/drivers/cpufreq/cpufreq-cpu0.c
>> @@ -174,29 +174,17 @@ static struct cpufreq_driver cpu0_cpufreq_driver =3D =
>> {
>> =20
>> static int cpu0_cpufreq_probe(struct platform_device *pdev)
>> {
>> -=09struct device_node *np, *parent;
>> +=09struct device_node *np;
>> =09int ret;
>> =20
>> -=09parent =3D of_find_node_by_path("/cpus");
>> -=09if (!parent) {
>> -=09=09pr_err("failed to find OF /cpus\n");
>> -=09=09return -ENOENT;
>> -=09}
>> -
>> -=09for_each_child_of_node(parent, np) {
>> -=09=09if (of_get_property(np, "operating-points", NULL))
>> -=09=09=09break;
>> -=09}
>> +=09cpu_dev =3D &pdev->dev;
>> =20
>> +=09np =3D of_node_get(cpu_dev->of_node);
>
> Has this actually been tested? This seems to break cpufreq-cpu0. The
> reason is, that this probe function is called not for the DT CPU node, but
> for a special virtual cpufreq-cpu0 platform device, typically created by
> platforms, using
>
> platform_device_register_simple("cpufreq-cpu0", -1, NULL, 0);
>
> which then of course doesn't have on .of_node associated with it.
>
Hi Guennadi,
Based on my understanding of the original code:
cpu_dev = &pdev->dev;
...
ret = of_init_opp_table(cpu_dev);
of_init_opp_table needs cpu_dev to be get_cpu_device(0). My
understanding was that platform using cpufreq-cpu0 sets &pdev->dev to
get_cpu_device(0). But looks like that's not the case.
Hi Shawn,
Can you please clarify ? The fix would be as below but I would like to
know if setting cpu_dev to get_cpu_device(0) instead of &pdev->dev has
any impact on other parts of code using cpu_dev ?
diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c
index cbfffa9..871c336 100644
--- a/drivers/cpufreq/cpufreq-cpu0.c
+++ b/drivers/cpufreq/cpufreq-cpu0.c
@@ -177,7 +177,7 @@ static int cpu0_cpufreq_probe(struct platform_device
*pdev)
struct device_node *np;
int ret;
- cpu_dev = &pdev->dev;
+ cpu_dev = get_cpu_device(0);
np = of_node_get(cpu_dev->of_node);
if (!np) {
Regards,
Sudeep
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v4 12/19] cpufreq: cpufreq-cpu0: remove device tree parsing for cpu nodes
2013-09-09 9:24 ` Sudeep KarkadaNagesha
@ 2013-09-09 14:32 ` Shawn Guo
2013-09-09 15:24 ` Sudeep KarkadaNagesha
0 siblings, 1 reply; 7+ messages in thread
From: Shawn Guo @ 2013-09-09 14:32 UTC (permalink / raw)
To: Sudeep KarkadaNagesha
Cc: Guennadi Liakhovetski, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
devicetree@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
Viresh Kumar, Greg Kroah-Hartman, Benjamin Herrenschmidt,
Jonas Bonn, Michal Simek, Rafael J. Wysocki,
grant.likely@linaro.org, rob.herring@calxeda.com
Hi Sudeep,
On Mon, Sep 09, 2013 at 10:24:39AM +0100, Sudeep KarkadaNagesha wrote:
> Hi Shawn,
>
> Can you please clarify ? The fix would be as below but I would like to
> know if setting cpu_dev to get_cpu_device(0) instead of &pdev->dev has
> any impact on other parts of code using cpu_dev ?
I'm sorry. I should have given it a test on hardware before ACKing the
changes.
The fix below should not have other impact except the prefix of dev_err
[info, dbg] message output ('cpufreq-cpu0:' to 'cpu cpu0:'), which
shouldn't be a problem.
>
> diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c
> index cbfffa9..871c336 100644
> --- a/drivers/cpufreq/cpufreq-cpu0.c
> +++ b/drivers/cpufreq/cpufreq-cpu0.c
> @@ -177,7 +177,7 @@ static int cpu0_cpufreq_probe(struct platform_device
> *pdev)
> struct device_node *np;
> int ret;
>
> - cpu_dev = &pdev->dev;
> + cpu_dev = get_cpu_device(0);
>
> np = of_node_get(cpu_dev->of_node);
> if (!np) {
>
The imx6q-cpufreq driver needs a similar fixing. Please include the
following changes into your fixing patches. Thanks.
Shawn
---8<---------
diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
index 85a1b51..69fd4b6 100644
--- a/arch/arm/mach-imx/mach-imx6q.c
+++ b/arch/arm/mach-imx/mach-imx6q.c
@@ -233,9 +233,10 @@ put_node:
of_node_put(np);
}
-static void __init imx6q_opp_init(struct device *cpu_dev)
+static void __init imx6q_opp_init(void)
{
struct device_node *np;
+ struct device *cpu_dev = get_cpu_device(0);
np = of_node_get(cpu_dev->of_node);
if (!np) {
@@ -268,7 +269,7 @@ static void __init imx6q_init_late(void)
imx6q_cpuidle_init();
if (IS_ENABLED(CONFIG_ARM_IMX6Q_CPUFREQ)) {
- imx6q_opp_init(&imx6q_cpufreq_pdev.dev);
+ imx6q_opp_init();
platform_device_register(&imx6q_cpufreq_pdev);
}
}
diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index 3e39654..d7ebd91 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -7,6 +7,7 @@
*/
#include <linux/clk.h>
+#include <linux/cpu.h>
#include <linux/cpufreq.h>
#include <linux/delay.h>
#include <linux/err.h>
@@ -202,7 +203,7 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
unsigned long min_volt, max_volt;
int num, ret;
- cpu_dev = &pdev->dev;
+ cpu_dev = get_cpu_device(0);
np = of_node_get(cpu_dev->of_node);
if (!np) {
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v4 12/19] cpufreq: cpufreq-cpu0: remove device tree parsing for cpu nodes
2013-09-09 14:32 ` Shawn Guo
@ 2013-09-09 15:24 ` Sudeep KarkadaNagesha
2013-09-10 2:44 ` Shawn Guo
0 siblings, 1 reply; 7+ messages in thread
From: Sudeep KarkadaNagesha @ 2013-09-09 15:24 UTC (permalink / raw)
To: Shawn Guo
Cc: Sudeep KarkadaNagesha, Guennadi Liakhovetski,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
devicetree@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
Viresh Kumar, Greg Kroah-Hartman, Benjamin Herrenschmidt,
Jonas Bonn, Michal Simek, Rafael J. Wysocki,
grant.likely@linaro.org, rob.herring@calxeda.com
On 09/09/13 15:32, Shawn Guo wrote:
> Hi Sudeep,
>
> On Mon, Sep 09, 2013 at 10:24:39AM +0100, Sudeep KarkadaNagesha wrote:
>> Hi Shawn,
>>
>> Can you please clarify ? The fix would be as below but I would like to
>> know if setting cpu_dev to get_cpu_device(0) instead of &pdev->dev has
>> any impact on other parts of code using cpu_dev ?
>
> I'm sorry. I should have given it a test on hardware before ACKing the
> changes.
>
> The fix below should not have other impact except the prefix of dev_err
> [info, dbg] message output ('cpufreq-cpu0:' to 'cpu cpu0:'), which
> shouldn't be a problem.
>
Hi Shawn,
Ok. But I am bit suspicious about devm_clk_get(cpu_dev, NULL).
I don't understand completely as how the clock are registered(whether
with dev_id or with connection_id).
A quick grep revealed that i.mx and shmobile is using conection id while
registering. If the clock is registered with connection id and retrieved
with cpu_dev(now dev_id is cpu0 and not cpufreq-cpu0), IIUC that would
break. If we pass pdev->dev for clk_get, it should be fine but again
IIUC it breaks highbank which gets all the information from DT.
So only solution I can think of is to continue to have the code
assigning (&pdev->dev)->of_node with cpu device node which is not clean
and arguable as incorrect since there is no DT node for cpufreq-cpu0.
I don't have a strong opinion though.
Let me know how would you like to fix this.
>>
>> diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c
>> index cbfffa9..871c336 100644
>> --- a/drivers/cpufreq/cpufreq-cpu0.c
>> +++ b/drivers/cpufreq/cpufreq-cpu0.c
>> @@ -177,7 +177,7 @@ static int cpu0_cpufreq_probe(struct platform_device
>> *pdev)
>> struct device_node *np;
>> int ret;
>>
>> - cpu_dev = &pdev->dev;
>> + cpu_dev = get_cpu_device(0);
>>
>> np = of_node_get(cpu_dev->of_node);
>> if (!np) {
>>
>
> The imx6q-cpufreq driver needs a similar fixing. Please include the
> following changes into your fixing patches. Thanks.
>
Ok no problem I can post the fix based on response for the above question.
Regard,
Sudeep
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 12/19] cpufreq: cpufreq-cpu0: remove device tree parsing for cpu nodes
2013-09-09 15:24 ` Sudeep KarkadaNagesha
@ 2013-09-10 2:44 ` Shawn Guo
2013-09-10 10:56 ` Sudeep KarkadaNagesha
0 siblings, 1 reply; 7+ messages in thread
From: Shawn Guo @ 2013-09-10 2:44 UTC (permalink / raw)
To: Sudeep KarkadaNagesha
Cc: Guennadi Liakhovetski, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
devicetree@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
Viresh Kumar, Greg Kroah-Hartman, Benjamin Herrenschmidt,
Jonas Bonn, Michal Simek, Rafael J. Wysocki,
grant.likely@linaro.org, rob.herring@calxeda.com
On Mon, Sep 09, 2013 at 04:24:18PM +0100, Sudeep KarkadaNagesha wrote:
> Hi Shawn,
>
> Ok. But I am bit suspicious about devm_clk_get(cpu_dev, NULL).
> I don't understand completely as how the clock are registered(whether
> with dev_id or with connection_id).
As the connection_id of devm_clk_get() call here is NULL, the clock
lookup should be registered with a proper dev_id in clk_register_clkdev()
call. And that's what you have seen with imx and shmobile code.
> A quick grep revealed that i.mx and shmobile is using conection id while
> registering.
They are using dev_id.
> If the clock is registered with connection id and retrieved
> with cpu_dev(now dev_id is cpu0 and not cpufreq-cpu0), IIUC that would
> break. If we pass pdev->dev for clk_get, it should be fine but again
> IIUC it breaks highbank which gets all the information from DT.
If the clock lookup is from DT, we should be just fine, since it will
work as long as the DT node with 'clocks' property (/cpus/cpu@0 in this
case) is attached to the struct device pointer of devm_clk_get() call.
> So only solution I can think of is to continue to have the code
> assigning (&pdev->dev)->of_node with cpu device node which is not clean
> and arguable as incorrect since there is no DT node for cpufreq-cpu0.
> I don't have a strong opinion though.
>
> Let me know how would you like to fix this.
So we only need to change all clkdev registration to use "cpu0" as
dev_id intstead of "cpufreq-cpu0.0", something like below.
And for imx, it should work even without the changes, because we have
device tree lookup ready there, and those clk_register_clkdev() calls
can just be removed now. But I prefer to include the change and leave
the cleanup to another patch for keeping the change log clear.
Shawn
---8<----------
diff --git a/arch/arm/mach-imx/clk-imx27.c b/arch/arm/mach-imx/clk-imx27.c
index c3cfa41..c6b40f3 100644
--- a/arch/arm/mach-imx/clk-imx27.c
+++ b/arch/arm/mach-imx/clk-imx27.c
@@ -285,7 +285,7 @@ int __init mx27_clocks_init(unsigned long fref)
clk_register_clkdev(clk[ata_ahb_gate], "ata", NULL);
clk_register_clkdev(clk[rtc_ipg_gate], NULL, "imx21-rtc");
clk_register_clkdev(clk[scc_ipg_gate], "scc", NULL);
- clk_register_clkdev(clk[cpu_div], NULL, "cpufreq-cpu0.0");
+ clk_register_clkdev(clk[cpu_div], NULL, "cpu0");
clk_register_clkdev(clk[emi_ahb_gate], "emi_ahb" , NULL);
mxc_timer_init(MX27_IO_ADDRESS(MX27_GPT1_BASE_ADDR), MX27_INT_GPT1);
diff --git a/arch/arm/mach-imx/clk-imx51-imx53.c b/arch/arm/mach-imx/clk-imx51-imx53.c
index 1a56a33..de1964c 100644
--- a/arch/arm/mach-imx/clk-imx51-imx53.c
+++ b/arch/arm/mach-imx/clk-imx51-imx53.c
@@ -328,7 +328,7 @@ static void __init mx5_clocks_common_init(unsigned long rate_ckil,
clk_register_clkdev(clk[ssi2_ipg_gate], NULL, "imx-ssi.1");
clk_register_clkdev(clk[ssi3_ipg_gate], NULL, "imx-ssi.2");
clk_register_clkdev(clk[sdma_gate], NULL, "imx35-sdma");
- clk_register_clkdev(clk[cpu_podf], NULL, "cpufreq-cpu0.0");
+ clk_register_clkdev(clk[cpu_podf], NULL, "cpu0");
clk_register_clkdev(clk[iim_gate], "iim", NULL);
clk_register_clkdev(clk[dummy], NULL, "imx2-wdt.0");
clk_register_clkdev(clk[dummy], NULL, "imx2-wdt.1");
diff --git a/arch/arm/mach-shmobile/clock-r8a73a4.c b/arch/arm/mach-shmobile/clock-r8a73a4.c
index 8ea5ef6..5bd2e85 100644
--- a/arch/arm/mach-shmobile/clock-r8a73a4.c
+++ b/arch/arm/mach-shmobile/clock-r8a73a4.c
@@ -555,7 +555,7 @@ static struct clk_lookup lookups[] = {
CLKDEV_CON_ID("pll2h", &pll2h_clk),
/* CPU clock */
- CLKDEV_DEV_ID("cpufreq-cpu0", &z_clk),
+ CLKDEV_DEV_ID("cpu0", &z_clk),
/* DIV6 */
CLKDEV_CON_ID("zb", &div6_clks[DIV6_ZB]),
diff --git a/arch/arm/mach-shmobile/clock-sh73a0.c b/arch/arm/mach-shmobile/clock-sh73a0.c
index 1942eae..c92c023 100644
--- a/arch/arm/mach-shmobile/clock-sh73a0.c
+++ b/arch/arm/mach-shmobile/clock-sh73a0.c
@@ -616,7 +616,7 @@ static struct clk_lookup lookups[] = {
CLKDEV_DEV_ID("smp_twd", &twd_clk), /* smp_twd */
/* DIV4 clocks */
- CLKDEV_DEV_ID("cpufreq-cpu0", &div4_clks[DIV4_Z]),
+ CLKDEV_DEV_ID("cpu0", &div4_clks[DIV4_Z]),
/* DIV6 clocks */
CLKDEV_CON_ID("vck1_clk", &div6_clks[DIV6_VCK1]),
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v4 12/19] cpufreq: cpufreq-cpu0: remove device tree parsing for cpu nodes
2013-09-10 2:44 ` Shawn Guo
@ 2013-09-10 10:56 ` Sudeep KarkadaNagesha
2013-09-10 11:19 ` Shawn Guo
0 siblings, 1 reply; 7+ messages in thread
From: Sudeep KarkadaNagesha @ 2013-09-10 10:56 UTC (permalink / raw)
To: Shawn Guo
Cc: Sudeep KarkadaNagesha, Guennadi Liakhovetski,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
devicetree@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
Viresh Kumar, Greg Kroah-Hartman, Benjamin Herrenschmidt,
Jonas Bonn, Michal Simek, Rafael J. Wysocki,
grant.likely@linaro.org, rob.herring@calxeda.com
On 10/09/13 03:44, Shawn Guo wrote:
> On Mon, Sep 09, 2013 at 04:24:18PM +0100, Sudeep KarkadaNagesha wrote:
>> Hi Shawn,
>>
>> Ok. But I am bit suspicious about devm_clk_get(cpu_dev, NULL).
>> I don't understand completely as how the clock are registered(whether
>> with dev_id or with connection_id).
>
> As the connection_id of devm_clk_get() call here is NULL, the clock
> lookup should be registered with a proper dev_id in clk_register_clkdev()
> call. And that's what you have seen with imx and shmobile code.
>
>
>> A quick grep revealed that i.mx and shmobile is using conection id while
>> registering.
>
> They are using dev_id.
>
Yes correct. I misunderstood, was expecting caller to pass dev and clk
layer extract dev_name so that even when device name is changed the clk
registration need not be changed.
>> If the clock is registered with connection id and retrieved
>> with cpu_dev(now dev_id is cpu0 and not cpufreq-cpu0), IIUC that would
>> break. If we pass pdev->dev for clk_get, it should be fine but again
>> IIUC it breaks highbank which gets all the information from DT.
>
> If the clock lookup is from DT, we should be just fine, since it will
> work as long as the DT node with 'clocks' property (/cpus/cpu@0 in this
> case) is attached to the struct device pointer of devm_clk_get() call.
>
This can be ignored if we are registering with "cpu0" as below in your
patch.
>> So only solution I can think of is to continue to have the code
>> assigning (&pdev->dev)->of_node with cpu device node which is not clean
>> and arguable as incorrect since there is no DT node for cpufreq-cpu0.
>> I don't have a strong opinion though.
>>
>> Let me know how would you like to fix this.
>
> So we only need to change all clkdev registration to use "cpu0" as
> dev_id intstead of "cpufreq-cpu0.0", something like below.
>
> And for imx, it should work even without the changes, because we have
> device tree lookup ready there, and those clk_register_clkdev() calls
> can just be removed now. But I prefer to include the change and leave
> the cleanup to another patch for keeping the change log clear.
>
Ok makes sense, do you want me to include this patch also as fix.
I can send a series to fix this if you OK:
1. Fix in cpufreq-cpu0
2. Fix in i.MX driver and platform file
3. Patch below
Regards,
Sudeep
>
> ---8<----------
>
> diff --git a/arch/arm/mach-imx/clk-imx27.c b/arch/arm/mach-imx/clk-imx27.c
> index c3cfa41..c6b40f3 100644
> --- a/arch/arm/mach-imx/clk-imx27.c
> +++ b/arch/arm/mach-imx/clk-imx27.c
> @@ -285,7 +285,7 @@ int __init mx27_clocks_init(unsigned long fref)
> clk_register_clkdev(clk[ata_ahb_gate], "ata", NULL);
> clk_register_clkdev(clk[rtc_ipg_gate], NULL, "imx21-rtc");
> clk_register_clkdev(clk[scc_ipg_gate], "scc", NULL);
> - clk_register_clkdev(clk[cpu_div], NULL, "cpufreq-cpu0.0");
> + clk_register_clkdev(clk[cpu_div], NULL, "cpu0");
> clk_register_clkdev(clk[emi_ahb_gate], "emi_ahb" , NULL);
>
> mxc_timer_init(MX27_IO_ADDRESS(MX27_GPT1_BASE_ADDR), MX27_INT_GPT1);
> diff --git a/arch/arm/mach-imx/clk-imx51-imx53.c b/arch/arm/mach-imx/clk-imx51-imx53.c
> index 1a56a33..de1964c 100644
> --- a/arch/arm/mach-imx/clk-imx51-imx53.c
> +++ b/arch/arm/mach-imx/clk-imx51-imx53.c
> @@ -328,7 +328,7 @@ static void __init mx5_clocks_common_init(unsigned long rate_ckil,
> clk_register_clkdev(clk[ssi2_ipg_gate], NULL, "imx-ssi.1");
> clk_register_clkdev(clk[ssi3_ipg_gate], NULL, "imx-ssi.2");
> clk_register_clkdev(clk[sdma_gate], NULL, "imx35-sdma");
> - clk_register_clkdev(clk[cpu_podf], NULL, "cpufreq-cpu0.0");
> + clk_register_clkdev(clk[cpu_podf], NULL, "cpu0");
> clk_register_clkdev(clk[iim_gate], "iim", NULL);
> clk_register_clkdev(clk[dummy], NULL, "imx2-wdt.0");
> clk_register_clkdev(clk[dummy], NULL, "imx2-wdt.1");
> diff --git a/arch/arm/mach-shmobile/clock-r8a73a4.c b/arch/arm/mach-shmobile/clock-r8a73a4.c
> index 8ea5ef6..5bd2e85 100644
> --- a/arch/arm/mach-shmobile/clock-r8a73a4.c
> +++ b/arch/arm/mach-shmobile/clock-r8a73a4.c
> @@ -555,7 +555,7 @@ static struct clk_lookup lookups[] = {
> CLKDEV_CON_ID("pll2h", &pll2h_clk),
>
> /* CPU clock */
> - CLKDEV_DEV_ID("cpufreq-cpu0", &z_clk),
> + CLKDEV_DEV_ID("cpu0", &z_clk),
>
> /* DIV6 */
> CLKDEV_CON_ID("zb", &div6_clks[DIV6_ZB]),
> diff --git a/arch/arm/mach-shmobile/clock-sh73a0.c b/arch/arm/mach-shmobile/clock-sh73a0.c
> index 1942eae..c92c023 100644
> --- a/arch/arm/mach-shmobile/clock-sh73a0.c
> +++ b/arch/arm/mach-shmobile/clock-sh73a0.c
> @@ -616,7 +616,7 @@ static struct clk_lookup lookups[] = {
> CLKDEV_DEV_ID("smp_twd", &twd_clk), /* smp_twd */
>
> /* DIV4 clocks */
> - CLKDEV_DEV_ID("cpufreq-cpu0", &div4_clks[DIV4_Z]),
> + CLKDEV_DEV_ID("cpu0", &div4_clks[DIV4_Z]),
>
> /* DIV6 clocks */
> CLKDEV_CON_ID("vck1_clk", &div6_clks[DIV6_VCK1]),
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 12/19] cpufreq: cpufreq-cpu0: remove device tree parsing for cpu nodes
2013-09-10 10:56 ` Sudeep KarkadaNagesha
@ 2013-09-10 11:19 ` Shawn Guo
0 siblings, 0 replies; 7+ messages in thread
From: Shawn Guo @ 2013-09-10 11:19 UTC (permalink / raw)
To: Sudeep KarkadaNagesha
Cc: Guennadi Liakhovetski, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
devicetree@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
Viresh Kumar, Greg Kroah-Hartman, Benjamin Herrenschmidt,
Jonas Bonn, Michal Simek, Rafael J. Wysocki,
grant.likely@linaro.org, rob.herring@calxeda.com
On Tue, Sep 10, 2013 at 11:56:17AM +0100, Sudeep KarkadaNagesha wrote:
> > So we only need to change all clkdev registration to use "cpu0" as
> > dev_id intstead of "cpufreq-cpu0.0", something like below.
> >
> > And for imx, it should work even without the changes, because we have
> > device tree lookup ready there, and those clk_register_clkdev() calls
> > can just be removed now. But I prefer to include the change and leave
> > the cleanup to another patch for keeping the change log clear.
> >
> Ok makes sense, do you want me to include this patch also as fix.
> I can send a series to fix this if you OK:
> 1. Fix in cpufreq-cpu0
> 2. Fix in i.MX driver and platform file
> 3. Patch below
Yes, please. Thanks.
Shawn
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-09-10 11:19 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1374492747-13879-1-git-send-email-Sudeep.KarkadaNagesha@arm.com>
[not found] ` <1376991021-12160-1-git-send-email-Sudeep.KarkadaNagesha@arm.com>
[not found] ` <1376991021-12160-13-git-send-email-Sudeep.KarkadaNagesha@arm.com>
2013-09-06 13:44 ` [PATCH v4 12/19] cpufreq: cpufreq-cpu0: remove device tree parsing for cpu nodes Guennadi Liakhovetski
2013-09-09 9:24 ` Sudeep KarkadaNagesha
2013-09-09 14:32 ` Shawn Guo
2013-09-09 15:24 ` Sudeep KarkadaNagesha
2013-09-10 2:44 ` Shawn Guo
2013-09-10 10:56 ` Sudeep KarkadaNagesha
2013-09-10 11:19 ` Shawn Guo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).