Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1246877 > unrolled thread

Re: [PATCH 14/26] clk: mvebu: Convert to clk_hw based provider APIs

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 14/26] clk: mvebu: Convert to clk_hw based provider APIs Thomas Petazzoni <thomas.petazzoni@free-electrons.com> - 2015-10-14 17:10 +0200
    Re: [PATCH 14/26] clk: mvebu: Convert to clk_hw based provider APIs Stephen Boyd <sboyd@codeaurora.org> - 2015-10-14 20:30 +0200
      Re: [PATCH 14/26] clk: mvebu: Convert to clk_hw based provider APIs Thomas Petazzoni <thomas.petazzoni@free-electrons.com> - 2015-10-14 22:20 +0200
        Re: [PATCH 14/26] clk: mvebu: Convert to clk_hw based provider APIs Stephen Boyd <sboyd@codeaurora.org> - 2015-10-14 23:10 +0200
          Re: [PATCH 14/26] clk: mvebu: Convert to clk_hw based provider APIs Thomas Petazzoni <thomas.petazzoni@free-electrons.com> - 2015-10-15 10:50 +0200
            Re: [PATCH 14/26] clk: mvebu: Convert to clk_hw based provider APIs Stephen Boyd <sboyd@codeaurora.org> - 2015-10-15 20:10 +0200
              Re: [PATCH 14/26] clk: mvebu: Convert to clk_hw based provider APIs Thomas Petazzoni <thomas.petazzoni@free-electrons.com> - 2015-10-15 22:00 +0200
                [PATCH] clk: Make of_clk_get_parent_name() robust with #clock-cells = 1 Stephen Boyd <sboyd@codeaurora.org> - 2015-10-16 01:30 +0200
                  Re: [PATCH] clk: Make of_clk_get_parent_name() robust with #clock-cells = 1 Michael Turquette <mturquette@baylibre.com> - 2015-10-16 15:00 +0200
                    Re: [PATCH] clk: Make of_clk_get_parent_name() robust with  #clock-cells = 1 Geert Uytterhoeven <geert@linux-m68k.org> - 2015-10-16 15:10 +0200
      Re: [PATCH 14/26] clk: mvebu: Convert to clk_hw based provider APIs Thomas Petazzoni <thomas.petazzoni@free-electrons.com> - 2015-10-15 10:30 +0200
        Re: [PATCH 14/26] clk: mvebu: Convert to clk_hw based provider APIs Stephen Boyd <sboyd@codeaurora.org> - 2015-10-15 20:10 +0200

#1246877 — Re: [PATCH 14/26] clk: mvebu: Convert to clk_hw based provider APIs

Stephen, Mike,

On Fri, 31 Jul 2015 10:03:54 -0700, Stephen Boyd wrote:
> We're removing struct clk from the clk provider API, so switch
> this code to using the clk_hw based provider APIs. This also
> removes a clk_get() in this driver that can just as easily use
> of_clk_get_parent_name() instead.
> 
> Cc: Gregory CLEMENT <gregory.clement@free-electrons.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> ---
>  drivers/clk/mvebu/clk-cpu.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/mvebu/clk-cpu.c b/drivers/clk/mvebu/clk-cpu.c
> index 86888a658d4c..5837eb8a212f 100644
> --- a/drivers/clk/mvebu/clk-cpu.c
> +++ b/drivers/clk/mvebu/clk-cpu.c
> @@ -121,7 +121,7 @@ static int clk_cpu_on_set_rate(struct clk_hw *hwclk, unsigned long rate,
>  	if (!cpuclk->pmu_dfs)
>  		return -ENODEV;
>  
> -	cur_rate = __clk_get_rate(hwclk->clk);
> +	cur_rate = clk_hw_get_rate(hwclk);
>  
>  	reg = readl(cpuclk->reg_base + SYS_CTRL_CLK_DIVIDER_CTRL2_OFFSET);
>  	fabric_div = (reg >> SYS_CTRL_CLK_DIVIDER_CTRL2_NBCLK_RATIO_SHIFT) &
> @@ -197,7 +197,6 @@ static void __init of_cpu_clk_setup(struct device_node *node)
>  	for_each_node_by_type(dn, "cpu") {
>  		struct clk_init_data init;
>  		struct clk *clk;
> -		struct clk *parent_clk;
>  		char *clk_name = kzalloc(5, GFP_KERNEL);
>  		int cpu, err;
>  
> @@ -209,9 +208,8 @@ static void __init of_cpu_clk_setup(struct device_node *node)
>  			goto bail_out;
>  
>  		sprintf(clk_name, "cpu%d", cpu);
> -		parent_clk = of_clk_get(node, 0);
>  
> -		cpuclk[cpu].parent_name = __clk_get_name(parent_clk);
> +		cpuclk[cpu].parent_name = of_clk_get_parent_name(node, 0);
>  		cpuclk[cpu].clk_name = clk_name;
>  		cpuclk[cpu].cpu = cpu;
>  		cpuclk[cpu].reg_base = clock_complex_base;

Sorry to chime in only right now, but this patch causes a regression on
Armada XP: the cpu clocks are no longer seen as child of their parent
and therefore their rate is always 0. This breaks cpufreq on this
platform.

For the record, our DT looks like this:

			coreclk: mvebu-sar@18230 {
				compatible = "marvell,armada-xp-core-clock";
				reg = <0x18230 0x08>;
				#clock-cells = <1>;
			};

			cpuclk: clock-complex@18700 {
				#clock-cells = <1>;
				compatible = "marvell,armada-xp-cpu-clock";
				reg = <0x18700 0x24>, <0x1c054 0x10>;
				clocks = <&coreclk 1>;
			};

The driver for the cpuclk registers n clocks, one for each CPU, where
each clock has as its parent the second "core clock", i.e <&coreclk 1>.

In the code before your patch, we were doing an of_clk_get(), which was
doing a complete "resolution" of the parent clock, and so
cpuclk[cpu].parent_name is "cpuclk". This allows the clk framework to
properly find "cpuclk" as the parent for the cpu[0-3] clocks, and we
get the appropriate behavior:

  cpuclk                                   4            4  1333000000          0 0  
     cpu3                                  1            1  1333000000          0 0  
     cpu2                                  1            1  1333000000          0 0  
     cpu1                                  1            1  1333000000          0 0  
     cpu0                                  3            3  1333000000          0 0  
     dramclk                               0            0   666500000          0 0  
     hclk                                  0            0   333250000          0 0  
     nbclk                                 0            0   666500000          0 0  

With your patch, since we don't use clock-output-names in the Device
Tree, the of_clk_get_parent_name() helper function returns simply the
name of the Device Tree node of the parent, in our case just
"mvebu-sar". Which obviously doesn't match any clock name, with the
consequence that cpu[0-3] are no longer parented to cpuclk, and
therefore their rate is 0 because they don't have a parent:

 cpuclk                                   0            0  1333000000          0 0  
    dramclk                               0            0   666500000          0 0  
    hclk                                  0            0   333250000          0 0  
    nbclk                                 0            0   666500000          0 0  
 cpu3                                     1            1           0          0 0  
 cpu2                                     1            1           0          0 0  
 cpu1                                     1            1           0          0 0  
 cpu0                                     3            3           0          0 0  

Stephen, what do you suggest to fix this issue?

The easiest solution is to add a clock-output-names property to the
coreclk node. This way, of_clk_get_parent_name() will properly resolve
the clock name to its correct name (i.e, "cpuclk" in our case) and
everything works fine (I've tested). The drawback of this solution is
that it breaks backward compatibility with old DTs: a 4.2 DT for Armada
XP would no longer work with a >= 4.3 kernel.

Do you have some other suggestions to make ?

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
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/

[toc] | [next] | [standalone]

#1247061

On 10/14, Thomas Petazzoni wrote:
> On Fri, 31 Jul 2015 10:03:54 -0700, Stephen Boyd wrote:
> > @@ -209,9 +208,8 @@ static void __init of_cpu_clk_setup(struct device_node *node)
> >  			goto bail_out;
> >  
> >  		sprintf(clk_name, "cpu%d", cpu);
> > -		parent_clk = of_clk_get(node, 0);
> >  
> > -		cpuclk[cpu].parent_name = __clk_get_name(parent_clk);
> > +		cpuclk[cpu].parent_name = of_clk_get_parent_name(node, 0);
> >  		cpuclk[cpu].clk_name = clk_name;
> >  		cpuclk[cpu].cpu = cpu;
> >  		cpuclk[cpu].reg_base = clock_complex_base;
> 
> Sorry to chime in only right now, but this patch causes a regression on
> Armada XP: the cpu clocks are no longer seen as child of their parent
> and therefore their rate is always 0. This breaks cpufreq on this
> platform.
> 
[...]
> 
> Stephen, what do you suggest to fix this issue?

Simplest fix is to revert this hunk. Longer term, we should look
into making of_clk_get_parent_name() use whatever string has been
used when registering the clock, or completely change how we do
parent child linkages so that strings aren't necessary.

> 
> The easiest solution is to add a clock-output-names property to the
> coreclk node. This way, of_clk_get_parent_name() will properly resolve
> the clock name to its correct name (i.e, "cpuclk" in our case) and
> everything works fine (I've tested). The drawback of this solution is
> that it breaks backward compatibility with old DTs: a 4.2 DT for Armada
> XP would no longer work with a >= 4.3 kernel.
> 
> Do you have some other suggestions to make ?
> 

How about this patch?

---8<----
diff --git a/drivers/clk/mvebu/clk-cpu.c b/drivers/clk/mvebu/clk-cpu.c
index 5837eb8a212f..85da8b983256 100644
--- a/drivers/clk/mvebu/clk-cpu.c
+++ b/drivers/clk/mvebu/clk-cpu.c
@@ -197,6 +197,7 @@ static void __init of_cpu_clk_setup(struct device_node *node)
 	for_each_node_by_type(dn, "cpu") {
 		struct clk_init_data init;
 		struct clk *clk;
+		struct clk *parent_clk;
 		char *clk_name = kzalloc(5, GFP_KERNEL);
 		int cpu, err;
 
@@ -208,8 +209,9 @@ static void __init of_cpu_clk_setup(struct device_node *node)
 			goto bail_out;
 
 		sprintf(clk_name, "cpu%d", cpu);
+		parent_clk = of_clk_get(node, 0);
 
-		cpuclk[cpu].parent_name = of_clk_get_parent_name(node, 0);
+		cpuclk[cpu].parent_name = __clk_get_name(parent_clk);
 		cpuclk[cpu].clk_name = clk_name;
 		cpuclk[cpu].cpu = cpu;
 		cpuclk[cpu].reg_base = clock_complex_base;
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/

[toc] | [prev] | [next] | [standalone]

#1247133

Hello,

Thanks for your feedback!

On Wed, 14 Oct 2015 11:21:38 -0700, Stephen Boyd wrote:

> > Stephen, what do you suggest to fix this issue?
> 
> Simplest fix is to revert this hunk.

Indeed, this is the simplest fix. But it's going backward with what you
were trying to do originally, so it isn't really nice either.

> Longer term, we should look into making of_clk_get_parent_name() use
> whatever string has been used when registering the clock,

I am not sure how of_clk_get_parent_name() can know the name of the
clock without clock-output-names in the DT. Without clock-output-names,
the only way to know the name of the parent clock is to actually ask
the driver of the parent clock, no?

> or completely change how we do parent child linkages so that strings
> aren't necessary.

Keep the reference to the DT node + "arguments". Not super simple
either.

> > The easiest solution is to add a clock-output-names property to the
> > coreclk node. This way, of_clk_get_parent_name() will properly
> > resolve the clock name to its correct name (i.e, "cpuclk" in our
> > case) and everything works fine (I've tested). The drawback of this
> > solution is that it breaks backward compatibility with old DTs: a
> > 4.2 DT for Armada XP would no longer work with a >= 4.3 kernel.
> > 
> > Do you have some other suggestions to make ?
> > 
> 
> How about this patch?

I'll give it a try tomorrow, but since it's essentially a revert of
your patch, I believe it should work.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
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/

[toc] | [prev] | [next] | [standalone]

#1247158

On 10/14, Thomas Petazzoni wrote:
> On Wed, 14 Oct 2015 11:21:38 -0700, Stephen Boyd wrote:
> 
> > > Stephen, what do you suggest to fix this issue?
> > 
> > Simplest fix is to revert this hunk.
> 
> Indeed, this is the simplest fix. But it's going backward with what you
> were trying to do originally, so it isn't really nice either.
> 
> > Longer term, we should look into making of_clk_get_parent_name() use
> > whatever string has been used when registering the clock,
> 
> I am not sure how of_clk_get_parent_name() can know the name of the
> clock without clock-output-names in the DT. Without clock-output-names,
> the only way to know the name of the parent clock is to actually ask
> the driver of the parent clock, no?

Yes that's the plan. We would try to resolve the name with
of_clk_get() and __clk_get_name() if there isn't a
clock-output-names property. If #clock-cells > 1 and of_clk_get()
fails, we would return NULL, otherwise for #clock-cells == 0 we
could fallback on the node name as a last resort. The change here
is that we don't use the node name for providers with
#clock-cells > 1 because that doesn't make any sense. We'll also
try a clk_get() in the case with #clock-cells = 0 and no
clock-output-names.

Eventually once we add the ability for OF clk providers to
register clk_hw pointers instead of clk pointers we'll be able to
"simplify" the of_clk_get() + __clk_get_name() path with a
provider clk_hw lookup and clk_hw_get_name() combination, which
should be faster and avoid any allocations.

Here's that untested patch, which we can throw into clk-next for
v4.4

-----8<----
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index b005f666e3a1..16b86a551bcb 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3055,6 +3055,7 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
 	u32 pv;
 	int rc;
 	int count;
+	struct clk *clk;
 
 	if (index < 0)
 		return NULL;
@@ -3080,8 +3081,25 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
 
 	if (of_property_read_string_index(clkspec.np, "clock-output-names",
 					  index,
-					  &clk_name) < 0)
-		clk_name = clkspec.np->name;
+					  &clk_name) < 0) {
+		/*
+		 * Best effort to get the name if the clock has been
+		 * registered with the framework. If the clock isn't
+		 * registered, we return the node name as the name of
+		 * the clock as long as #clock-cells = 0.
+		 */
+		clk = of_clk_get(np, index);
+		if (IS_ERR(clk)) {
+			if (clkspec.args_count == 0)
+				clk_name = clkspec.np->name;
+			else
+				clk_name = NULL;
+		} else {
+			clk_name = __clk_get_name(clk);
+			clk_put(clk);
+		}
+	}
+
 
 	of_node_put(clkspec.np);
 	return clk_name;

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/

[toc] | [prev] | [next] | [standalone]

#1247575

Stephen,

On Wed, 14 Oct 2015 14:08:59 -0700, Stephen Boyd wrote:

> Here's that untested patch, which we can throw into clk-next for
> v4.4
> 
> -----8<----
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index b005f666e3a1..16b86a551bcb 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3055,6 +3055,7 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
>  	u32 pv;
>  	int rc;
>  	int count;
> +	struct clk *clk;
>  
>  	if (index < 0)
>  		return NULL;
> @@ -3080,8 +3081,25 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
>  
>  	if (of_property_read_string_index(clkspec.np, "clock-output-names",
>  					  index,
> -					  &clk_name) < 0)
> -		clk_name = clkspec.np->name;
> +					  &clk_name) < 0) {
> +		/*
> +		 * Best effort to get the name if the clock has been
> +		 * registered with the framework. If the clock isn't
> +		 * registered, we return the node name as the name of
> +		 * the clock as long as #clock-cells = 0.
> +		 */
> +		clk = of_clk_get(np, index);
> +		if (IS_ERR(clk)) {
> +			if (clkspec.args_count == 0)
> +				clk_name = clkspec.np->name;
> +			else
> +				clk_name = NULL;
> +		} else {
> +			clk_name = __clk_get_name(clk);
> +			clk_put(clk);
> +		}
> +	}
> +
>  
>  	of_node_put(clkspec.np);
>  	return clk_name;

It almost worked, but not completely. The issue is that by the time you
call of_clk_get(np, index), index is no longer equal to the value
passed as argument to of_clk_get_parent_name(), it has been modified to
indicate the index of the *parent* clock.

So, after changing your patch to:

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 0ebcf44..60d2f62 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3052,9 +3052,11 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
        struct property *prop;
        const char *clk_name;
        const __be32 *vp;
+       int parent_index;
        u32 pv;
        int rc;
        int count;
+       struct clk *clk;
 
        if (index < 0)
                return NULL;
@@ -3064,14 +3066,14 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
        if (rc)
                return NULL;
 
-       index = clkspec.args_count ? clkspec.args[0] : 0;
+       parent_index = clkspec.args_count ? clkspec.args[0] : 0;
        count = 0;
 
        /* if there is an indices property, use it to transfer the index
         * specified into an array offset for the clock-output-names property.
         */
        of_property_for_each_u32(clkspec.np, "clock-indices", prop, vp, pv) {
-               if (index == pv) {
+               if (parent_index == pv) {
                        index = count;
                        break;
                }
@@ -3079,9 +3081,26 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
        }
 
        if (of_property_read_string_index(clkspec.np, "clock-output-names",
-                                         index,
-                                         &clk_name) < 0)
-               clk_name = clkspec.np->name;
+                                         parent_index,
+                                         &clk_name) < 0) {
+               /*
+                * Best effort to get the name if the clock has been
+                * registered with the framework. If the clock isn't
+                * registered, we return the node name as the name of
+                * the clock as long as #clock-cells = 0.
+                */
+               clk = of_clk_get(np, index);
+               if (IS_ERR(clk)) {
+                       if (clkspec.args_count == 0)
+                               clk_name = clkspec.np->name;
+                       else
+                               clk_name = NULL;
+               } else {
+                       clk_name = __clk_get_name(clk);
+                       clk_put(clk);
+               }
+       }
+
 
        of_node_put(clkspec.np);
        return clk_name;

It does work properly for me, on top of 4.3-rc5, without any change to
my Device Tree files.

So, what is the plan now ?

 - Have the minimal fix in drivers/clk/mvebu/clk-cpu.c for 4.3.

 - Have the patch improving the of_clk_get_parent_name() logic merged
   in 4.4 + a revert of the fix done for 4.3 in clk-cpu.c. Unless maybe
   you don't want to clutter the core of the clock framework to handle
   this specific case?

 - Add the clock-output-names to the Marvell EBU Device Tree files, so
   that in a couple of kernel releases we can remove the hacks and rely
   on the generic logic of of_clk_get_parent_name().

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
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/

[toc] | [prev] | [next] | [standalone]

#1248054

On 10/15, Thomas Petazzoni wrote:
> Stephen,
> 
> On Wed, 14 Oct 2015 14:08:59 -0700, Stephen Boyd wrote:
> 
> > Here's that untested patch, which we can throw into clk-next for
> > v4.4
> 
> It almost worked, but not completely. The issue is that by the time you
> call of_clk_get(np, index), index is no longer equal to the value
> passed as argument to of_clk_get_parent_name(), it has been modified to
> indicate the index of the *parent* clock.
> 
> So, after changing your patch to:
[...]
> It does work properly for me, on top of 4.3-rc5, without any change to
> my Device Tree files.

Good catch! Except we don't need to do all that complicated
stuff, right? We can use of_clk_get_from_provider() instead.

---8<----
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 16b86a551bcb..648fb2c8904d 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3088,7 +3088,7 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
 		 * registered, we return the node name as the name of
 		 * the clock as long as #clock-cells = 0.
 		 */
-		clk = of_clk_get(np, index);
+		clk = of_clk_get_from_provider(clkspec);
 		if (IS_ERR(clk)) {
 			if (clkspec.args_count == 0)
 				clk_name = clkspec.np->name;

> 
> So, what is the plan now ?
> 
>  - Have the minimal fix in drivers/clk/mvebu/clk-cpu.c for 4.3.

Yep.

> 
>  - Have the patch improving the of_clk_get_parent_name() logic merged
>    in 4.4 + a revert of the fix done for 4.3 in clk-cpu.c. Unless maybe
>    you don't want to clutter the core of the clock framework to handle
>    this specific case?

Assuming that the patch doesn't cause problems for other
platforms, then I think we'll go with this approach.

> 
>  - Add the clock-output-names to the Marvell EBU Device Tree files, so
>    that in a couple of kernel releases we can remove the hacks and rely
>    on the generic logic of of_clk_get_parent_name().
> 

Naw, let's not change any DT files for this.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/

[toc] | [prev] | [next] | [standalone]

#1248123

Stephen,

On Thu, 15 Oct 2015 11:09:03 -0700, Stephen Boyd wrote:

> Good catch! Except we don't need to do all that complicated
> stuff, right? We can use of_clk_get_from_provider() instead.
> 
> ---8<----
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 16b86a551bcb..648fb2c8904d 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3088,7 +3088,7 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
>  		 * registered, we return the node name as the name of
>  		 * the clock as long as #clock-cells = 0.
>  		 */
> -		clk = of_clk_get(np, index);
> +		clk = of_clk_get_from_provider(clkspec);
>  		if (IS_ERR(clk)) {
>  			if (clkspec.args_count == 0)
>  				clk_name = clkspec.np->name;

I'll re-test this solution tomorrow, just to make sure.

> > So, what is the plan now ?
> > 
> >  - Have the minimal fix in drivers/clk/mvebu/clk-cpu.c for 4.3.
> 
> Yep.

I see in your other e-mail that you already applied the fix, so good.

> >  - Have the patch improving the of_clk_get_parent_name() logic merged
> >    in 4.4 + a revert of the fix done for 4.3 in clk-cpu.c. Unless maybe
> >    you don't want to clutter the core of the clock framework to handle
> >    this specific case?
> 
> Assuming that the patch doesn't cause problems for other
> platforms, then I think we'll go with this approach.

Ok. Can you send the patch "properly" so that I can give a formal
Tested-by ?

> >  - Add the clock-output-names to the Marvell EBU Device Tree files, so
> >    that in a couple of kernel releases we can remove the hacks and rely
> >    on the generic logic of of_clk_get_parent_name().
> > 
> 
> Naw, let's not change any DT files for this.

ACK.

Thanks for your help!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
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/

[toc] | [prev] | [next] | [standalone]

#1248248 — [PATCH] clk: Make of_clk_get_parent_name() robust with #clock-cells = 1

If a clock provider has #clock-cells = 1 and we call
of_clk_get_parent_name() on it we may end up returning the name
of the provider node if the provider doesn't have a
clock-output-names property. This doesn't make sense, especially
when you consider that calling of_clk_get_parent_name() on such a
node with different indices will return the same name each time.

Let's try getting the clock from the framework via of_clk_get()
instead, and only fallback to the node name if we have a provider
with #clock-cells = 0. This way, we can't hand out the same name
for different clocks when we don't actually know their names.

Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/clk/clk.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index b005f666e3a1..a0fe9ca1f0af 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3055,6 +3055,7 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
 	u32 pv;
 	int rc;
 	int count;
+	struct clk *clk;
 
 	if (index < 0)
 		return NULL;
@@ -3080,8 +3081,25 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
 
 	if (of_property_read_string_index(clkspec.np, "clock-output-names",
 					  index,
-					  &clk_name) < 0)
-		clk_name = clkspec.np->name;
+					  &clk_name) < 0) {
+		/*
+		 * Best effort to get the name if the clock has been
+		 * registered with the framework. If the clock isn't
+		 * registered, we return the node name as the name of
+		 * the clock as long as #clock-cells = 0.
+		 */
+		clk = of_clk_get_from_provider(&clkspec);
+		if (IS_ERR(clk)) {
+			if (clkspec.args_count == 0)
+				clk_name = clkspec.np->name;
+			else
+				clk_name = NULL;
+		} else {
+			clk_name = __clk_get_name(clk);
+			clk_put(clk);
+		}
+	}
+
 
 	of_node_put(clkspec.np);
 	return clk_name;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

--
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/

[toc] | [prev] | [next] | [standalone]

#1248717 — Re: [PATCH] clk: Make of_clk_get_parent_name() robust with #clock-cells = 1

Cc'ing Geert.

Quoting Stephen Boyd (2015-10-15 16:19:38)
> If a clock provider has #clock-cells = 1 and we call
> of_clk_get_parent_name() on it we may end up returning the name
> of the provider node if the provider doesn't have a
> clock-output-names property. This doesn't make sense, especially
> when you consider that calling of_clk_get_parent_name() on such a
> node with different indices will return the same name each time.
> 
> Let's try getting the clock from the framework via of_clk_get()
> instead, and only fallback to the node name if we have a provider
> with #clock-cells = 0. This way, we can't hand out the same name
> for different clocks when we don't actually know their names.
> 
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>

Geert,

Does this approach fix the issues you had removing clock-output-names
from your clock provider DTS?

Regards,
Mike

> ---
>  drivers/clk/clk.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index b005f666e3a1..a0fe9ca1f0af 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3055,6 +3055,7 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
>         u32 pv;
>         int rc;
>         int count;
> +       struct clk *clk;
>  
>         if (index < 0)
>                 return NULL;
> @@ -3080,8 +3081,25 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
>  
>         if (of_property_read_string_index(clkspec.np, "clock-output-names",
>                                           index,
> -                                         &clk_name) < 0)
> -               clk_name = clkspec.np->name;
> +                                         &clk_name) < 0) {
> +               /*
> +                * Best effort to get the name if the clock has been
> +                * registered with the framework. If the clock isn't
> +                * registered, we return the node name as the name of
> +                * the clock as long as #clock-cells = 0.
> +                */
> +               clk = of_clk_get_from_provider(&clkspec);
> +               if (IS_ERR(clk)) {
> +                       if (clkspec.args_count == 0)
> +                               clk_name = clkspec.np->name;
> +                       else
> +                               clk_name = NULL;
> +               } else {
> +                       clk_name = __clk_get_name(clk);
> +                       clk_put(clk);
> +               }
> +       }
> +
>  
>         of_node_put(clkspec.np);
>         return clk_name;
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 
--
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/

[toc] | [prev] | [next] | [standalone]

#1248739 — Re: [PATCH] clk: Make of_clk_get_parent_name() robust with #clock-cells = 1

Hi Mike,

On Fri, Oct 16, 2015 at 2:55 PM, Michael Turquette
<mturquette@baylibre.com> wrote:
> Quoting Stephen Boyd (2015-10-15 16:19:38)
>> If a clock provider has #clock-cells = 1 and we call
>> of_clk_get_parent_name() on it we may end up returning the name
>> of the provider node if the provider doesn't have a
>> clock-output-names property. This doesn't make sense, especially
>> when you consider that calling of_clk_get_parent_name() on such a
>> node with different indices will return the same name each time.
>>
>> Let's try getting the clock from the framework via of_clk_get()
>> instead, and only fallback to the node name if we have a provider
>> with #clock-cells = 0. This way, we can't hand out the same name
>> for different clocks when we don't actually know their names.
>>
>> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
>
> Geert,
>
> Does this approach fix the issues you had removing clock-output-names
> from your clock provider DTS?

I already saw this patch in clk-next, and I think it would fix that issue.
However, we're no longer seeing the issue, as we no longer have
fixed-factor-clocks in DT.

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
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/

[toc] | [prev] | [next] | [standalone]

#1247559

Stephen,

On Wed, 14 Oct 2015 11:21:38 -0700, Stephen Boyd wrote:

> How about this patch?
> 
> ---8<----
> diff --git a/drivers/clk/mvebu/clk-cpu.c b/drivers/clk/mvebu/clk-cpu.c
> index 5837eb8a212f..85da8b983256 100644
> --- a/drivers/clk/mvebu/clk-cpu.c
> +++ b/drivers/clk/mvebu/clk-cpu.c
> @@ -197,6 +197,7 @@ static void __init of_cpu_clk_setup(struct device_node *node)
>  	for_each_node_by_type(dn, "cpu") {
>  		struct clk_init_data init;
>  		struct clk *clk;
> +		struct clk *parent_clk;
>  		char *clk_name = kzalloc(5, GFP_KERNEL);
>  		int cpu, err;
>  
> @@ -208,8 +209,9 @@ static void __init of_cpu_clk_setup(struct device_node *node)
>  			goto bail_out;
>  
>  		sprintf(clk_name, "cpu%d", cpu);
> +		parent_clk = of_clk_get(node, 0);
>  
> -		cpuclk[cpu].parent_name = of_clk_get_parent_name(node, 0);
> +		cpuclk[cpu].parent_name = __clk_get_name(parent_clk);
>  		cpuclk[cpu].clk_name = clk_name;
>  		cpuclk[cpu].cpu = cpu;
>  		cpuclk[cpu].reg_base = clock_complex_base;

Applied on top of 4.3-rc5, this patch fixes the problem for me, without
any change to the Device Tree files.

It's probably the easiest option for 4.3. Do you want me to submit a
patch, or will you apply it directly to your clk tree?

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
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/

[toc] | [prev] | [next] | [standalone]

#1248055

On 10/15, Thomas Petazzoni wrote:
> 
> Applied on top of 4.3-rc5, this patch fixes the problem for me, without
> any change to the Device Tree files.
> 
> It's probably the easiest option for 4.3. Do you want me to submit a
> patch, or will you apply it directly to your clk tree?

I've already applied it to clk-fixes, I'll send it off tomorrow
or the next day.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web