clk & clock-controller@ DT nodes: __clk_core_init: clk clock-controller already initialized

clk & clock-controller@ DT nodes: __clk_core_init: clk clock-controller already initialized

[Rafał Miłecki]

Hi,

I'm working on Northstar support (ARM platform with multiple clocks
blocks).

I've just noticed that my clocks don't work anymore after the commit:
[PATCH] ARM: dts: BCM5301X: update CRU block description
https://patchwork.kernel.org/project/linux-arm-kernel/patch/20211029160523.2812-1-zajec5@gmail.com/

[    0.000000] __clk_core_init: clk clock-controller already initialized
[    0.000000] ------------[ cut here ]------------
[    0.000000] WARNING: CPU: 0 PID: 0 at drivers/clk/bcm/clk-iproc-pll.c:802 iproc_pll_clk_setup+0x4c8/0x4f4
[    0.000000] Modules linked in:
[    0.000000] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.10.80 #0
[    0.000000] Hardware name: BCM5301X
[    0.000000] [<c0108410>] (unwind_backtrace) from [<c0104bc4>] (show_stack+0x10/0x14)
[    0.000000] [<c0104bc4>] (show_stack) from [<c03dca28>] (dump_stack+0x94/0xa8)
[    0.000000] [<c03dca28>] (dump_stack) from [<c0118440>] (__warn+0xb8/0x114)
[    0.000000] [<c0118440>] (__warn) from [<c0118504>] (warn_slowpath_fmt+0x68/0x78)
[    0.000000] [<c0118504>] (warn_slowpath_fmt) from [<c043281c>] (iproc_pll_clk_setup+0x4c8/0x4f4)
[    0.000000] [<c043281c>] (iproc_pll_clk_setup) from [<c0818c04>] (nsp_genpll_clk_init+0x30/0x38)
[    0.000000] [<c0818c04>] (nsp_genpll_clk_init) from [<c0818634>] (of_clk_init+0x118/0x1f8)
[    0.000000] [<c0818634>] (of_clk_init) from [<c08039b0>] (time_init+0x24/0x30)
[    0.000000] [<c08039b0>] (time_init) from [<c0800d14>] (start_kernel+0x398/0x50c)
[    0.000000] [<c0800d14>] (start_kernel) from [<00000000>] (0x0)
[    0.000000] ---[ end trace fe236bfe9559ee50 ]---

It seems that Linux's clock name is derived from DT node. With multiple
clock-controller@ nodes there is a name conflict as seen above.

 From DT perspective I don't think there is anything wrong in having
multiple nodes using the same name prefix.

Could clk subsystem be somehow improved to be more /creative/ at picking
clock names? ;) Would someone be kind to take a look at this, please?

Re: clk & clock-controller@ DT nodes: __clk_core_init: clk clock-controller already initialized

[Rafał Miłecki]

On 23.11.2021 11:09, Rafał Miłecki wrote:
> [    0.000000] __clk_core_init: clk clock-controller already initialized
> [    0.000000] ------------[ cut here ]------------
> [    0.000000] WARNING: CPU: 0 PID: 0 at drivers/clk/bcm/clk-iproc-pll.c:802 iproc_pll_clk_setup+0x4c8/0x4f4
> [    0.000000] Modules linked in:
> [    0.000000] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.10.80 #0
> [    0.000000] Hardware name: BCM5301X
> [    0.000000] [<c0108410>] (unwind_backtrace) from [<c0104bc4>] (show_stack+0x10/0x14)
> [    0.000000] [<c0104bc4>] (show_stack) from [<c03dca28>] (dump_stack+0x94/0xa8)
> [    0.000000] [<c03dca28>] (dump_stack) from [<c0118440>] (__warn+0xb8/0x114)
> [    0.000000] [<c0118440>] (__warn) from [<c0118504>] (warn_slowpath_fmt+0x68/0x78)
> [    0.000000] [<c0118504>] (warn_slowpath_fmt) from [<c043281c>] (iproc_pll_clk_setup+0x4c8/0x4f4)
> [    0.000000] [<c043281c>] (iproc_pll_clk_setup) from [<c0818c04>] (nsp_genpll_clk_init+0x30/0x38)
> [    0.000000] [<c0818c04>] (nsp_genpll_clk_init) from [<c0818634>] (of_clk_init+0x118/0x1f8)
> [    0.000000] [<c0818634>] (of_clk_init) from [<c08039b0>] (time_init+0x24/0x30)
> [    0.000000] [<c08039b0>] (time_init) from [<c0800d14>] (start_kernel+0x398/0x50c)
> [    0.000000] [<c0800d14>] (start_kernel) from [<00000000>] (0x0)
> [    0.000000] ---[ end trace fe236bfe9559ee50 ]---
> 
> It seems that Linux's clock name is derived from DT node. With multiple
> clock-controller@ nodes there is a name conflict as seen above.
> 
>  From DT perspective I don't think there is anything wrong in having
> multiple nodes using the same name prefix.
> 
> Could clk subsystem be somehow improved to be more /creative/ at picking
> clock names? ;) Would someone be kind to take a look at this, please?

Ah, I've just realized it's not clk subsystem but actually a clk driver
that picks up a name based on DT node name.

iproc_pll_clk_setup() does:
init.name = node->name;

I should have double check that before sending my e-mail.

I can't see any helper picking a clock unique name based on DT node.
Any advise how to proceed with that?

Re: clk & clock-controller@ DT nodes: __clk_core_init: clk clock-controller already initialized

[Rafał Miłecki]

On 23.11.2021 11:20, Rafał Miłecki wrote:
> On 23.11.2021 11:09, Rafał Miłecki wrote:
>> [    0.000000] __clk_core_init: clk clock-controller already initialized
>> [    0.000000] ------------[ cut here ]------------
>> [    0.000000] WARNING: CPU: 0 PID: 0 at drivers/clk/bcm/clk-iproc-pll.c:802 iproc_pll_clk_setup+0x4c8/0x4f4
>> [    0.000000] Modules linked in:
>> [    0.000000] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.10.80 #0
>> [    0.000000] Hardware name: BCM5301X
>> [    0.000000] [<c0108410>] (unwind_backtrace) from [<c0104bc4>] (show_stack+0x10/0x14)
>> [    0.000000] [<c0104bc4>] (show_stack) from [<c03dca28>] (dump_stack+0x94/0xa8)
>> [    0.000000] [<c03dca28>] (dump_stack) from [<c0118440>] (__warn+0xb8/0x114)
>> [    0.000000] [<c0118440>] (__warn) from [<c0118504>] (warn_slowpath_fmt+0x68/0x78)
>> [    0.000000] [<c0118504>] (warn_slowpath_fmt) from [<c043281c>] (iproc_pll_clk_setup+0x4c8/0x4f4)
>> [    0.000000] [<c043281c>] (iproc_pll_clk_setup) from [<c0818c04>] (nsp_genpll_clk_init+0x30/0x38)
>> [    0.000000] [<c0818c04>] (nsp_genpll_clk_init) from [<c0818634>] (of_clk_init+0x118/0x1f8)
>> [    0.000000] [<c0818634>] (of_clk_init) from [<c08039b0>] (time_init+0x24/0x30)
>> [    0.000000] [<c08039b0>] (time_init) from [<c0800d14>] (start_kernel+0x398/0x50c)
>> [    0.000000] [<c0800d14>] (start_kernel) from [<00000000>] (0x0)
>> [    0.000000] ---[ end trace fe236bfe9559ee50 ]---
>>
>> It seems that Linux's clock name is derived from DT node. With multiple
>> clock-controller@ nodes there is a name conflict as seen above.
>>
>>  From DT perspective I don't think there is anything wrong in having
>> multiple nodes using the same name prefix.
>>
>> Could clk subsystem be somehow improved to be more /creative/ at picking
>> clock names? ;) Would someone be kind to take a look at this, please?
> 
> Ah, I've just realized it's not clk subsystem but actually a clk driver
> that picks up a name based on DT node name.
> 
> iproc_pll_clk_setup() does:
> init.name = node->name;
> 
> I should have double check that before sending my e-mail.
> 
> I can't see any helper picking a clock unique name based on DT node.
> Any advise how to proceed with that?

I've tried a simple patch switching from "name" to "full_name".

That results in registering "clock-controller@100" and
"clock-controller@160" clocks (I can see them in /sys/kernel/debug/clk/)

They seem to work fine:
# cat /sys/kernel/debug/clk/clock-controller@140/clk_rate
2000000000
# cat /sys/kernel/debug/clk/clock-controller@100/clk_rate
3200000000

It seems however that output clocks are broken:
# cat /sys/kernel/debug/clk/usbclk/clk_rate
0

For some reason iproc_clk_recalc_rate() gets called with "parent_rate"
argument 0 whenever clocks aren't named "lcpll0" and "genpll". I'm not
sure why it happens. I don't see any code referencing clocks by
hardcoded names "lcpll0" and "genpll".


--- 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_n
  	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);