[PATCH] clk: iproc: Do not rely on node name for correct PLL setup

[PATCH] clk: iproc: Do not rely on node name for correct PLL setup

[Florian Fainelli]

After commit 31fd9b79dc58 ("ARM: dts: BCM5301X: update CRU block
description") a warning from clk-iproc-pll.c was generated due to a
duplicate PLL name as well as the console stopped working. Upon closer
inspection it became clear that iproc_pll_clk_setup() used the Device
Tree node unit name as an unique identifier as well as a parent name to
parent all clocks under the PLL.

BCM5301X was the first platform on which that got noticed because of the
DT node unit name renaming but the same assumptions hold true for any
user of the iproc_pll_clk_setup() function.

The first 'clock-output-names' property is always guaranteed to be
unique as well as providing the actual desired PLL clock name, so we
utilize that to register the PLL and as a parent name of all children
clock.

Fixes: 5fe225c105fd ("clk: iproc: add initial common clock support")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Rafal,

This is a replacement for this patch that you checked into OpenWrt:

https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/bcm53xx/patches-5.15/320-ARM-dts-BCM5301X-Switch-back-to-old-clock-nodes-name.patch;h=cee37732ab9e2ac8bc2a399a53d01b9ead756cb8;hb=HEAD


 drivers/clk/bcm/clk-iproc-pll.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/bcm/clk-iproc-pll.c b/drivers/clk/bcm/clk-iproc-pll.c
index 33da30f99c79..92be88eb1d11 100644
--- a/drivers/clk/bcm/clk-iproc-pll.c
+++ b/drivers/clk/bcm/clk-iproc-pll.c
@@ -736,6 +736,7 @@ void iproc_pll_clk_setup(struct device_node *node,
 	const char *parent_name;
 	struct iproc_clk *iclk_array;
 	struct clk_hw_onecell_data *clk_data;
+	const char *clk_name;
 
 	if (WARN_ON(!pll_ctrl) || WARN_ON(!clk_ctrl))
 		return;
@@ -783,7 +784,12 @@ void iproc_pll_clk_setup(struct device_node *node,
 	iclk = &iclk_array[0];
 	iclk->pll = pll;
 
-	init.name = node->name;
+	ret = of_property_read_string_index(node, "clock-output-names",
+					    0, &clk_name);
+	if (WARN_ON(ret))
+		goto err_pll_register;
+
+	init.name = clk_name;
 	init.ops = &iproc_pll_ops;
 	init.flags = 0;
 	parent_name = of_clk_get_parent_name(node, 0);
@@ -803,13 +809,12 @@ void iproc_pll_clk_setup(struct device_node *node,
 		goto err_pll_register;
 
 	clk_data->hws[0] = &iclk->hw;
+	parent_name = clk_name;
 
 	/* now initialize and register all leaf clocks */
 	for (i = 1; i < num_clks; i++) {
-		const char *clk_name;
 
 		memset(&init, 0, sizeof(init));
-		parent_name = node->name;
 
 		ret = of_property_read_string_index(node, "clock-output-names",
 						    i, &clk_name);
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] clk: iproc: Do not rely on node name for correct PLL setup

[Florian Fainelli]

On 8/2/2022 7:58 PM, Florian Fainelli wrote:
> After commit 31fd9b79dc58 ("ARM: dts: BCM5301X: update CRU block
> description") a warning from clk-iproc-pll.c was generated due to a
> duplicate PLL name as well as the console stopped working. Upon closer
> inspection it became clear that iproc_pll_clk_setup() used the Device
> Tree node unit name as an unique identifier as well as a parent name to
> parent all clocks under the PLL.
> 
> BCM5301X was the first platform on which that got noticed because of the
> DT node unit name renaming but the same assumptions hold true for any
> user of the iproc_pll_clk_setup() function.
> 
> The first 'clock-output-names' property is always guaranteed to be
> unique as well as providing the actual desired PLL clock name, so we
> utilize that to register the PLL and as a parent name of all children
> clock.
> 
> Fixes: 5fe225c105fd ("clk: iproc: add initial common clock support")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Stephen,

Can we get this applied for an upcoming 6.0-rc? Thanks
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] clk: iproc: Do not rely on node name for correct PLL setup

[Rafał Miłecki]

On 2022-08-03 04:58, Florian Fainelli wrote:
> After commit 31fd9b79dc58 ("ARM: dts: BCM5301X: update CRU block
> description") a warning from clk-iproc-pll.c was generated due to a
> duplicate PLL name as well as the console stopped working. Upon closer
> inspection it became clear that iproc_pll_clk_setup() used the Device
> Tree node unit name as an unique identifier as well as a parent name to
> parent all clocks under the PLL.
> 
> BCM5301X was the first platform on which that got noticed because of 
> the
> DT node unit name renaming but the same assumptions hold true for any
> user of the iproc_pll_clk_setup() function.
> 
> The first 'clock-output-names' property is always guaranteed to be
> unique as well as providing the actual desired PLL clock name, so we
> utilize that to register the PLL and as a parent name of all children
> clock.
> 
> Fixes: 5fe225c105fd ("clk: iproc: add initial common clock support")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Acked-by: Rafał Miłecki <rafal@milecki.pl>


Thanks for looking into this!

In the past I debugged this too and even developed a simple fix:
clk & clock-controller@ DT nodes: __clk_core_init: clk clock-controller 
already initialized
https://www.spinics.net/lists/linux-clk/msg63855.html

For some reason my old fix didn't work with usbclk clock.

I just tested your patch agains kernel 5.15 and it works!

root@OpenWrt:/# uname -r
5.15.64
root@OpenWrt:/# cat /sys/kernel/debug/clk/usbclk/clk_rate
40000000



> ---
> Rafal,
> 
> This is a replacement for this patch that you checked into OpenWrt:
> 
> https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/bcm53xx/patches-5.15/320-ARM-dts-BCM5301X-Switch-back-to-old-clock-nodes-name.patch;h=cee37732ab9e2ac8bc2a399a53d01b9ead756cb8;hb=HEAD
> 
> 
>  drivers/clk/bcm/clk-iproc-pll.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/bcm/clk-iproc-pll.c 
> b/drivers/clk/bcm/clk-iproc-pll.c
> index 33da30f99c79..92be88eb1d11 100644
> --- a/drivers/clk/bcm/clk-iproc-pll.c
> +++ b/drivers/clk/bcm/clk-iproc-pll.c
> @@ -736,6 +736,7 @@ void iproc_pll_clk_setup(struct device_node *node,
>  	const char *parent_name;
>  	struct iproc_clk *iclk_array;
>  	struct clk_hw_onecell_data *clk_data;
> +	const char *clk_name;
> 
>  	if (WARN_ON(!pll_ctrl) || WARN_ON(!clk_ctrl))
>  		return;
> @@ -783,7 +784,12 @@ void iproc_pll_clk_setup(struct device_node *node,
>  	iclk = &iclk_array[0];
>  	iclk->pll = pll;
> 
> -	init.name = node->name;
> +	ret = of_property_read_string_index(node, "clock-output-names",
> +					    0, &clk_name);
> +	if (WARN_ON(ret))
> +		goto err_pll_register;
> +
> +	init.name = clk_name;
>  	init.ops = &iproc_pll_ops;
>  	init.flags = 0;
>  	parent_name = of_clk_get_parent_name(node, 0);
> @@ -803,13 +809,12 @@ void iproc_pll_clk_setup(struct device_node 
> *node,
>  		goto err_pll_register;
> 
>  	clk_data->hws[0] = &iclk->hw;
> +	parent_name = clk_name;
> 
>  	/* now initialize and register all leaf clocks */
>  	for (i = 1; i < num_clks; i++) {
> -		const char *clk_name;
> 
>  		memset(&init, 0, sizeof(init));
> -		parent_name = node->name;
> 
>  		ret = of_property_read_string_index(node, "clock-output-names",
>  						    i, &clk_name);

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] clk: iproc: Do not rely on node name for correct PLL setup

[Rafał Miłecki]

On 2022-09-04 17:00, Rafał Miłecki wrote:
> On 2022-08-03 04:58, Florian Fainelli wrote:
>> After commit 31fd9b79dc58 ("ARM: dts: BCM5301X: update CRU block
>> description") a warning from clk-iproc-pll.c was generated due to a
>> duplicate PLL name as well as the console stopped working. Upon closer
>> inspection it became clear that iproc_pll_clk_setup() used the Device
>> Tree node unit name as an unique identifier as well as a parent name 
>> to
>> parent all clocks under the PLL.
>> 
>> BCM5301X was the first platform on which that got noticed because of 
>> the
>> DT node unit name renaming but the same assumptions hold true for any
>> user of the iproc_pll_clk_setup() function.
>> 
>> The first 'clock-output-names' property is always guaranteed to be
>> unique as well as providing the actual desired PLL clock name, so we
>> utilize that to register the PLL and as a parent name of all children
>> clock.
>> 
>> Fixes: 5fe225c105fd ("clk: iproc: add initial common clock support")
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> 
> Acked-by: Rafał Miłecki <rafal@milecki.pl>
> 
> 
> Thanks for looking into this!
> 
> In the past I debugged this too and even developed a simple fix:
> clk & clock-controller@ DT nodes: __clk_core_init: clk
> clock-controller already initialized
> https://www.spinics.net/lists/linux-clk/msg63855.html
> 
> For some reason my old fix didn't work with usbclk clock.

I compared your changes with my old attempt and found the missing bit. I
forgot about updating parent_name.

FWIW something like below would work too.

Thanks again for taking care of this old regression.


diff --git a/drivers/clk/bcm/clk-iproc-pll.c 
b/drivers/clk/bcm/clk-iproc-pll.c
index 33da30f99..af9ca32b8 100644
--- a/drivers/clk/bcm/clk-iproc-pll.c
+++ b/drivers/clk/bcm/clk-iproc-pll.c
@@ -783,7 +783,7 @@ void iproc_pll_clk_setup(struct device_node *node,
  	iclk = &iclk_array[0];
  	iclk->pll = pll;

-	init.name = node->name;
+	init.name = node->full_name;
  	init.ops = &iproc_pll_ops;
  	init.flags = 0;
  	parent_name = of_clk_get_parent_name(node, 0);
@@ -809,7 +809,7 @@ void iproc_pll_clk_setup(struct device_node *node,
  		const char *clk_name;

  		memset(&init, 0, sizeof(init));
-		parent_name = node->name;
+		parent_name = node->full_name;

  		ret = of_property_read_string_index(node, "clock-output-names",
  						    i, &clk_name);

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] clk: iproc: Do not rely on node name for correct PLL setup

[Rafał Miłecki]

On 2022-08-03 04:58, Florian Fainelli wrote:
> After commit 31fd9b79dc58 ("ARM: dts: BCM5301X: update CRU block
> description") a warning from clk-iproc-pll.c was generated due to a
> duplicate PLL name as well as the console stopped working. Upon closer
> inspection it became clear that iproc_pll_clk_setup() used the Device
> Tree node unit name as an unique identifier as well as a parent name to
> parent all clocks under the PLL.
> 
> BCM5301X was the first platform on which that got noticed because of 
> the
> DT node unit name renaming but the same assumptions hold true for any
> user of the iproc_pll_clk_setup() function.
> 
> The first 'clock-output-names' property is always guaranteed to be
> unique as well as providing the actual desired PLL clock name, so we
> utilize that to register the PLL and as a parent name of all children
> clock.
> 
> Fixes: 5fe225c105fd ("clk: iproc: add initial common clock support")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

One nitpick below.


> ---
> Rafal,
> 
> This is a replacement for this patch that you checked into OpenWrt:
> 
> https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/bcm53xx/patches-5.15/320-ARM-dts-BCM5301X-Switch-back-to-old-clock-nodes-name.patch;h=cee37732ab9e2ac8bc2a399a53d01b9ead756cb8;hb=HEAD
> 
> 
>  drivers/clk/bcm/clk-iproc-pll.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/bcm/clk-iproc-pll.c 
> b/drivers/clk/bcm/clk-iproc-pll.c
> index 33da30f99c79..92be88eb1d11 100644
> --- a/drivers/clk/bcm/clk-iproc-pll.c
> +++ b/drivers/clk/bcm/clk-iproc-pll.c
> @@ -736,6 +736,7 @@ void iproc_pll_clk_setup(struct device_node *node,
>  	const char *parent_name;
>  	struct iproc_clk *iclk_array;
>  	struct clk_hw_onecell_data *clk_data;
> +	const char *clk_name;
> 
>  	if (WARN_ON(!pll_ctrl) || WARN_ON(!clk_ctrl))
>  		return;
> @@ -783,7 +784,12 @@ void iproc_pll_clk_setup(struct device_node *node,
>  	iclk = &iclk_array[0];
>  	iclk->pll = pll;
> 
> -	init.name = node->name;
> +	ret = of_property_read_string_index(node, "clock-output-names",
> +					    0, &clk_name);
> +	if (WARN_ON(ret))
> +		goto err_pll_register;
> +
> +	init.name = clk_name;
>  	init.ops = &iproc_pll_ops;
>  	init.flags = 0;
>  	parent_name = of_clk_get_parent_name(node, 0);
> @@ -803,13 +809,12 @@ void iproc_pll_clk_setup(struct device_node 
> *node,
>  		goto err_pll_register;
> 
>  	clk_data->hws[0] = &iclk->hw;
> +	parent_name = clk_name;
> 
>  	/* now initialize and register all leaf clocks */
>  	for (i = 1; i < num_clks; i++) {
> -		const char *clk_name;
> 
>  		memset(&init, 0, sizeof(init));

You could probably get rid of that empty line above memset().


> -		parent_name = node->name;
> 
>  		ret = of_property_read_string_index(node, "clock-output-names",
>  						    i, &clk_name);

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel