* [PATCH] clk: register fixed-clock only if #clock-cells property is present
@ 2014-03-26 18:22 Sylwester Nawrocki
2014-03-26 18:33 ` Fabio Estevam
2014-03-27 10:55 ` Sylwester Nawrocki
0 siblings, 2 replies; 14+ messages in thread
From: Sylwester Nawrocki @ 2014-03-26 18:22 UTC (permalink / raw)
To: linux-arm-kernel
After commit 1771b10d605d26ccee771a7fb4b08718c124097a
"clk: respect the clock dependencies in of_clk_init"
the order of registering clock providers and their corresponding
clocks may change. This commit currently causes a regression
on Exynos4 platforms, where fixed clocks are now being registered
through the standard "fixed-clock" compatible device driver, rather
than through a custom mechanism within the clk-exynos4 driver.
Then any attempts to register the fixed clocks in clk-exynos4
fail, since clocks named "xxti", "xusbxti" are already registered.
This means that some clock specifiers do not work any more, i.e.
those with phandle to the main clock controller node and fixed
indexes. This in turn causes wrong frequency of the root clocks
and various division by zero errors.
Hence for old dtbs, where #clock-cells property is missing in the
fixed-clock DT nodes for exynos4 this patch has a positive side
effect that the fixed clocks will not get registered through the
standard mechanism and the required clock names won't be taken
at the time the main clock controller driver's init callback is
called.
To make exynos use the standard fixed-clock registration mechanism,
related dts files could be updated, i.e. #clock-cells property
added together with required dependencies, that is a 'clocks'
property with phandles to the fixed clock nodes listed in it.
The dependencies specified in devicetree would then ensure proper
order of clocks registration.
Now the issue is that this patch affects not only exynos, but also
imx and at91 platforms. However, those use a custom compatible
string in addition to "fixed-clock" and I suspect registering
fixed clocks through the drivers/clk/clk-fixed-rate.c driver has
been failing silently for them anyway. This patch could be
actually fixing similar issue as for exynos, seen after commit
"clk: respect the clock dependencies in of_clk_init".
There is one board that will likely break after this patch, i.e.
arch/arm/boot/dts/vf610-twr.dts. Nonetheless, it doesn't specify
the #clock-cells property, which is documented as mandatory in
Documentation/devicetree/bindings/clock/fixed-clock.txt.
So perhaps we could add the missing property to dts as a fix.
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
drivers/clk/clk-fixed-rate.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c
index 0fc56ab..cbbef2b 100644
--- a/drivers/clk/clk-fixed-rate.c
+++ b/drivers/clk/clk-fixed-rate.c
@@ -122,6 +122,11 @@ void of_fixed_clk_setup(struct device_node *node)
if (of_property_read_u32(node, "clock-frequency", &rate))
return;
+ if (!of_find_property(node, "#clock-cells", NULL)) {
+ pr_warn("clk: missing #clock-cells property at %s\n",
+ node->full_name);
+ return;
+ }
of_property_read_u32(node, "clock-accuracy", &accuracy);
of_property_read_string(node, "clock-output-names", &clk_name);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH] clk: register fixed-clock only if #clock-cells property is present
2014-03-26 18:22 [PATCH] clk: register fixed-clock only if #clock-cells property is present Sylwester Nawrocki
@ 2014-03-26 18:33 ` Fabio Estevam
2014-03-26 19:57 ` Sylwester Nawrocki
2014-03-27 10:55 ` Sylwester Nawrocki
1 sibling, 1 reply; 14+ messages in thread
From: Fabio Estevam @ 2014-03-26 18:33 UTC (permalink / raw)
To: linux-arm-kernel
Hi Sylwester,
On Wed, Mar 26, 2014 at 3:22 PM, Sylwester Nawrocki
<s.nawrocki@samsung.com> wrote:
> After commit 1771b10d605d26ccee771a7fb4b08718c124097a
> "clk: respect the clock dependencies in of_clk_init"
> the order of registering clock providers and their corresponding
> clocks may change. This commit currently causes a regression
> on Exynos4 platforms, where fixed clocks are now being registered
> through the standard "fixed-clock" compatible device driver, rather
> than through a custom mechanism within the clk-exynos4 driver.
> Then any attempts to register the fixed clocks in clk-exynos4
> fail, since clocks named "xxti", "xusbxti" are already registered.
> This means that some clock specifiers do not work any more, i.e.
> those with phandle to the main clock controller node and fixed
> indexes. This in turn causes wrong frequency of the root clocks
> and various division by zero errors.
>
> Hence for old dtbs, where #clock-cells property is missing in the
> fixed-clock DT nodes for exynos4 this patch has a positive side
> effect that the fixed clocks will not get registered through the
> standard mechanism and the required clock names won't be taken
> at the time the main clock controller driver's init callback is
> called.
>
> To make exynos use the standard fixed-clock registration mechanism,
> related dts files could be updated, i.e. #clock-cells property
> added together with required dependencies, that is a 'clocks'
> property with phandles to the fixed clock nodes listed in it.
> The dependencies specified in devicetree would then ensure proper
> order of clocks registration.
>
> Now the issue is that this patch affects not only exynos, but also
> imx and at91 platforms. However, those use a custom compatible
> string in addition to "fixed-clock" and I suspect registering
> fixed clocks through the drivers/clk/clk-fixed-rate.c driver has
> been failing silently for them anyway. This patch could be
> actually fixing similar issue as for exynos, seen after commit
> "clk: respect the clock dependencies in of_clk_init".
>
> There is one board that will likely break after this patch, i.e.
> arch/arm/boot/dts/vf610-twr.dts. Nonetheless, it doesn't specify
> the #clock-cells property, which is documented as mandatory in
> Documentation/devicetree/bindings/clock/fixed-clock.txt.
> So perhaps we could add the missing property to dts as a fix.
>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Still not able to boot with this patch applied:
Starting kernel ...
Uncompressing Linux... done, booting the kernel.
Booting Linux on physical CPU 0x0
Linux version 3.14.0-rc8-next-20140326+ (fabio at fabio-Latitude-E6410)
(gcc version 4.7.3 (Ubuntu/Linaro 4.7.3-1ubuntu1) ) #948 SMP Wed
Mar 26 15:30:47 BRT 2014
CPU: ARMv7 Processor [412fc09a] revision 10 (ARMv7), cr=10c5387d
CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing instruction cache
Machine model: Wandboard i.MX6 Quad Board
bootconsole [earlycon0] enabled
Memory policy: Data cache writealloc
PERCPU: Embedded 8 pages/cpu @ee7a4000 s8960 r8192 d15616 u32768
Built 1 zonelists in Zone order, mobility grouping on. Total pages: 520720
Kernel command line: console=ttymxc0,115200 root=/dev/nfs ip=dhcp
nfsroot=192.168.0.2:/tftpboot/rfs,v3,tcp earlyprintk
PID hash table entries: 4096 (order: 2, 16384 bytes)
Dentry cache hash table entries: 262144 (order: 8, 1048576 bytes)
Inode-cache hash table entries: 131072 (order: 7, 524288 bytes)
Memory: 2063904K/2097152K available (6455K kernel code, 402K rwdata,
2200K rodata, 320K init, 5513K bss, 33248K reserved, 270336K high
mem)
Virtual kernel memory layout:
vector : 0xffff0000 - 0xffff1000 ( 4 kB)
fixmap : 0xfff00000 - 0xfffe0000 ( 896 kB)
vmalloc : 0xf0000000 - 0xff000000 ( 240 MB)
lowmem : 0x80000000 - 0xef800000 (1784 MB)
pkmap : 0x7fe00000 - 0x80000000 ( 2 MB)
modules : 0x7f000000 - 0x7fe00000 ( 14 MB)
.text : 0x80008000 - 0x8087bfdc (8656 kB)
.init : 0x8087c000 - 0x808cc300 ( 321 kB)
.data : 0x808ce000 - 0x80932980 ( 403 kB)
.bss : 0x80932988 - 0x80e950f8 (5514 kB)
SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=4, Nodes=1
Hierarchical RCU implementation.
NR_IRQS:16 nr_irqs:16 16
L310 cache controller enabled
l2x0: 16 ways, CACHE_ID 0x410000c7, AUX_CTRL 0x32070000, Cache size: 1024 kB
Switching to timer-based delay loop
Division by zero in kernel.
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.14.0-rc8-next-20140326+ #948
Backtrace:
[<80011cd4>] (dump_backtrace) from [<80011e70>] (show_stack+0x18/0x1c)
r6:00000000 r5:00000000 r4:00000000 r3:00000000
[<80011e58>] (show_stack) from [<8064cc90>] (dump_stack+0x88/0xa4)
[<8064cc08>] (dump_stack) from [<80011a94>] (__div0+0x18/0x20)
r5:00000000 r4:00000000
[<80011a7c>] (__div0) from [<8029582c>] (Ldiv0_64+0x8/0x18)
[<8007e854>] (clocks_calc_mult_shift) from [<8089a8d0>]
(sched_clock_register+0x60/0x238)
r10:f0018074 r9:00000000 r8:80020ba0 r7:00000020 r6:00000000 r5:80932eac
r4:00000000
[<8089a870>] (sched_clock_register) from [<80884588>]
(mxc_timer_init+0xf8/0x190)
r10:f0018074 r9:00000000 r8:00000057 r7:f0020024 r6:ee022b80 r5:80932eac
r4:00000000
[<80884490>] (mxc_timer_init) from [<808910b0>]
(imx6q_clocks_init+0x2c00/0x2d24)
r8:00000004 r7:809334d0 r6:808c7310 r5:ee7ccbe8 r4:f0020000
[<8088e4b0>] (imx6q_clocks_init) from [<808b024c>] (of_clk_init+0xd8/0x198)
r10:80923594 r9:809234d4 r8:00000000 r7:ee002ac0 r6:ee002b00 r5:ee7ce5b0
r4:00000001
[<808b0174>] (of_clk_init) from [<80880364>] (time_init+0x2c/0x38)
r10:ef7fc9c0 r9:412fc09a r8:808d6880 r7:808bd790 r6:ffffffff r5:809329c0
r4:00000001
[<80880338>] (time_init) from [<8087ca2c>] (start_kernel+0x1f8/0x388)
[<8087c834>] (start_kernel) from [<10008074>] (0x10008074)
r10:00000000 r8:1000406a r7:808db644 r6:808bd78c r5:808d692c r4:10c5387d
sched_clock: 32 bits at 0 Hz, resolution 0ns, wraps every 0ns
Division by zero in kernel.
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.14.0-rc8-next-20140326+ #948
Backtrace:
[<80011cd4>] (dump_backtrace) from [<80011e70>] (show_stack+0x18/0x1c)
r6:ee011a00 r5:00000000 r4:00000000 r3:00000000
[<80011e58>] (show_stack) from [<8064cc90>] (dump_stack+0x88/0xa4)
[<8064cc08>] (dump_stack) from [<80011a94>] (__div0+0x18/0x20)
r5:00000000 r4:00000000
[<80011a7c>] (__div0) from [<8029582c>] (Ldiv0_64+0x8/0x18)
[<8007ea84>] (__clocksource_updatefreq_scale) from [<8007ec3c>]
(__clocksource_register_scale+0x14/0x54)
r10:000000c8 r9:00000000 r8:f0020024 r7:807b3e58 r6:ee0119c0 r5:00000000
r4:ee011a00
[<8007ec28>] (__clocksource_register_scale) from [<808ae6f8>]
(clocksource_mmio_init+0x8c/0xa8)
r4:ffffffff r3:00000001
[<808ae66c>] (clocksource_mmio_init) from [<808845ac>]
(mxc_timer_init+0x11c/0x190)
r10:f0018074 r8:00000057 r7:f0020024 r6:ee022b80 r5:80932eac r4:808dc900
[<80884490>] (mxc_timer_init) from [<808910b0>]
(imx6q_clocks_init+0x2c00/0x2d24)
r8:00000004 r7:809334d0 r6:808c7310 r5:ee7ccbe8 r4:f0020000
[<8088e4b0>] (imx6q_clocks_init) from [<808b024c>] (of_clk_init+0xd8/0x198)
r10:80923594 r9:809234d4 r8:00000000 r7:ee002ac0 r6:ee002b00 r5:ee7ce5b0
r4:00000001
[<808b0174>] (of_clk_init) from [<80880364>] (time_init+0x2c/0x38)
r10:ef7fc9c0 r9:412fc09a r8:808d6880 r7:808bd790 r6:ffffffff r5:809329c0
r4:00000001
[<80880338>] (time_init) from [<8087ca2c>] (start_kernel+0x1f8/0x388)
[<8087c834>] (start_kernel) from [<10008074>] (0x10008074)
r10:00000000 r8:1000406a r7:808db644 r6:808bd78c r5:808d692c r4:10c5387d
Division by zero in kernel.
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.14.0-rc8-next-20140326+ #948
Backtrace:
[<80011cd4>] (dump_backtrace) from [<80011e70>] (show_stack+0x18/0x1c)
r6:ee011a00 r5:00000000 r4:00000000 r3:00000000
[<80011e58>] (show_stack) from [<8064cc90>] (dump_stack+0x88/0xa4)
[<8064cc08>] (dump_stack) from [<80011a94>] (__div0+0x18/0x20)
r5:00000000 r4:00000000
[<80011a7c>] (__div0) from [<8029582c>] (Ldiv0_64+0x8/0x18)
[<8007e854>] (clocks_calc_mult_shift) from [<8007eb28>]
(__clocksource_updatefreq_scale+0xa4/0x1a4)
r10:00000001 r9:a3d70a3d r8:70a3d70a r7:0000000b r6:ee011a00 r5:00000001
r4:00000001
[<8007ea84>] (__clocksource_updatefreq_scale) from [<8007ec3c>]
(__clocksource_register_scale+0x14/0x54)
r10:000000c8 r9:00000000 r8:f0020024 r7:807b3e58 r6:ee0119c0 r5:00000000
r4:ee011a00
[<8007ec28>] (__clocksource_register_scale) from [<808ae6f8>]
(clocksource_mmio_init+0x8c/0xa8)
r4:ffffffff r3:00000001
[<808ae66c>] (clocksource_mmio_init) from [<808845ac>]
(mxc_timer_init+0x11c/0x190)
r10:f0018074 r8:00000057 r7:f0020024 r6:ee022b80 r5:80932eac r4:808dc900
[<80884490>] (mxc_timer_init) from [<808910b0>]
(imx6q_clocks_init+0x2c00/0x2d24)
r8:00000004 r7:809334d0 r6:808c7310 r5:ee7ccbe8 r4:f0020000
[<8088e4b0>] (imx6q_clocks_init) from [<808b024c>] (of_clk_init+0xd8/0x198)
r10:80923594 r9:809234d4 r8:00000000 r7:ee002ac0 r6:ee002b00 r5:ee7ce5b0
r4:00000001
[<808b0174>] (of_clk_init) from [<80880364>] (time_init+0x2c/0x38)
r10:ef7fc9c0 r9:412fc09a r8:808d6880 r7:808bd790 r6:ffffffff r5:809329c0
r4:00000001
[<80880338>] (time_init) from [<8087ca2c>] (start_kernel+0x1f8/0x388)
[<8087c834>] (start_kernel) from [<10008074>] (0x10008074)
r10:00000000 r8:1000406a r7:808db644 r6:808bd78c r5:808d692c r4:10c5387d
Division by zero in kernel.
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.14.0-rc8-next-20140326+ #948
Backtrace:
[<80011cd4>] (dump_backtrace) from [<80011e70>] (show_stack+0x18/0x1c)
r6:ee022b80 r5:00000000 r4:00000000 r3:00000000
[<80011e58>] (show_stack) from [<8064cc90>] (dump_stack+0x88/0xa4)
[<8064cc08>] (dump_stack) from [<80011a94>] (__div0+0x18/0x20)
r5:808dc900 r4:00000000
[<80011a7c>] (__div0) from [<8029582c>] (Ldiv0_64+0x8/0x18)
[<80082314>] (clockevents_config) from [<800823b4>]
(clockevents_config_and_register+0x1c/0x28)
r5:80932eac r4:808dc900
[<80082398>] (clockevents_config_and_register) from [<808845d8>]
(mxc_timer_init+0x148/0x190)
r4:808dc900 r3:fffffffe
[<80884490>] (mxc_timer_init) from [<808910b0>]
(imx6q_clocks_init+0x2c00/0x2d24)
r8:00000004 r7:809334d0 r6:808c7310 r5:ee7ccbe8 r4:f0020000
[<8088e4b0>] (imx6q_clocks_init) from [<808b024c>] (of_clk_init+0xd8/0x198)
r10:80923594 r9:809234d4 r8:00000000 r7:ee002ac0 r6:ee002b00 r5:ee7ce5b0
r4:00000001
[<808b0174>] (of_clk_init) from [<80880364>] (time_init+0x2c/0x38)
r10:ef7fc9c0 r9:412fc09a r8:808d6880 r7:808bd790 r6:ffffffff r5:809329c0
r4:00000001
[<80880338>] (time_init) from [<8087ca2c>] (start_kernel+0x1f8/0x388)
[<8087c834>] (start_kernel) from [<10008074>] (0x10008074)
r10:00000000 r8:1000406a r7:808db644 r6:808bd78c r5:808d692c r4:10c5387d
------------[ cut here ]------------
WARNING: CPU: 0 PID: 0 at kernel/time/clockevents.c:44 cev_delta2ns+0xe8/0x104()
Modules linked in:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.14.0-rc8-next-20140326+ #948
Backtrace:
[<80011cd4>] (dump_backtrace) from [<80011e70>] (show_stack+0x18/0x1c)
r6:80081cb8 r5:00000000 r4:00000000 r3:00000000
[<80011e58>] (show_stack) from [<8064cc90>] (dump_stack+0x88/0xa4)
[<8064cc08>] (dump_stack) from [<80028f78>] (warn_slowpath_common+0x70/0x94)
r5:00000009 r4:00000000
[<80028f08>] (warn_slowpath_common) from [<80028fc0>]
(warn_slowpath_null+0x24/0x2c)
r8:000000ff r7:000000ff r6:00000000 r5:808dc900 r4:00000000
[<80028f9c>] (warn_slowpath_null) from [<80081cb8>] (cev_delta2ns+0xe8/0x104)
[<80081bd0>] (cev_delta2ns) from [<80082374>] (clockevents_config+0x60/0x84)
r9:00000000 r8:00000057 r7:f0020024 r6:ee022b80 r5:808dc900 r4:00000000
[<80082314>] (clockevents_config) from [<800823b4>]
(clockevents_config_and_register+0x1c/0x28)
r5:80932eac r4:808dc900
[<80082398>] (clockevents_config_and_register) from [<808845d8>]
(mxc_timer_init+0x148/0x190)
r4:808dc900 r3:fffffffe
[<80884490>] (mxc_timer_init) from [<808910b0>]
(imx6q_clocks_init+0x2c00/0x2d24)
r8:00000004 r7:809334d0 r6:808c7310 r5:ee7ccbe8 r4:f0020000
[<8088e4b0>] (imx6q_clocks_init) from [<808b024c>] (of_clk_init+0xd8/0x198)
r10:80923594 r9:809234d4 r8:00000000 r7:ee002ac0 r6:ee002b00 r5:ee7ce5b0
r4:00000001
[<808b0174>] (of_clk_init) from [<80880364>] (time_init+0x2c/0x38)
r10:ef7fc9c0 r9:412fc09a r8:808d6880 r7:808bd790 r6:ffffffff r5:809329c0
r4:00000001
[<80880338>] (time_init) from [<8087ca2c>] (start_kernel+0x1f8/0x388)
[<8087c834>] (start_kernel) from [<10008074>] (0x10008074)
r10:00000000 r8:1000406a r7:808db644 r6:808bd78c r5:808d692c r4:10c5387d
---[ end trace 3406ff24bd97382e ]---
clk: missing #clock-cells property at /clocks/osc
clk: missing #clock-cells property at /clocks/ckih1
clk: missing #clock-cells property at /clocks/ckil
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] clk: register fixed-clock only if #clock-cells property is present
2014-03-26 18:33 ` Fabio Estevam
@ 2014-03-26 19:57 ` Sylwester Nawrocki
2014-03-26 20:14 ` Fabio Estevam
0 siblings, 1 reply; 14+ messages in thread
From: Sylwester Nawrocki @ 2014-03-26 19:57 UTC (permalink / raw)
To: linux-arm-kernel
Hi Fabio,
On 03/26/2014 07:33 PM, Fabio Estevam wrote:
> On Wed, Mar 26, 2014 at 3:22 PM, Sylwester Nawrocki
> <s.nawrocki@samsung.com> wrote:
[...]
> Still not able to boot with this patch applied:
Perhaps a change as below helps ?
From 85ee85e4a92b42442354f3f2454be50c173e1c59 Mon Sep 17 00:00:00 2001
From: Sylwester Nawrocki <s.nawrocki@samsung.com>
Date: Wed, 26 Mar 2014 20:54:13 +0100
Subject: [PATCH] clk: reverse default clk provider initialization order
in of_clk_init()
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
drivers/clk/clk.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index fb3c40b..d30809c 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2608,7 +2608,7 @@ void __init of_clk_init(const struct of_device_id
*matches)
parent->clk_init_cb = match->data;
parent->np = np;
- list_add(&parent->node, &clk_provider_list);
+ list_add_tail(&parent->node, &clk_provider_list);
}
while (!list_empty(&clk_provider_list)) {
--
1.7.9.5
This should keep the clock providers initialization order as before,
rather than reversing it.
Anyway, I think the clock dependencies need to be specified in
the device tree explicitly to avoid surprises like this when,
e.g. clock nodes get reordered in dts or by dtc.
--
Regards,
Sylwester
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH] clk: register fixed-clock only if #clock-cells property is present
2014-03-26 19:57 ` Sylwester Nawrocki
@ 2014-03-26 20:14 ` Fabio Estevam
2014-03-26 20:25 ` Gregory CLEMENT
2014-03-27 7:58 ` Boris BREZILLON
0 siblings, 2 replies; 14+ messages in thread
From: Fabio Estevam @ 2014-03-26 20:14 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Mar 26, 2014 at 4:57 PM, Sylwester Nawrocki
<sylvester.nawrocki@gmail.com> wrote:
> Perhaps a change as below helps ?
>
> From 85ee85e4a92b42442354f3f2454be50c173e1c59 Mon Sep 17 00:00:00 2001
> From: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Date: Wed, 26 Mar 2014 20:54:13 +0100
> Subject: [PATCH] clk: reverse default clk provider initialization order in
> of_clk_init()
>
>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
> drivers/clk/clk.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index fb3c40b..d30809c 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2608,7 +2608,7 @@ void __init of_clk_init(const struct of_device_id
> *matches)
>
> parent->clk_init_cb = match->data;
> parent->np = np;
> - list_add(&parent->node, &clk_provider_list);
> + list_add_tail(&parent->node, &clk_provider_list);
> }
>
> while (!list_empty(&clk_provider_list)) {
Thanks, Sylwester!
This makes my imx6q-wandboard to boot again.
Tested-by: Fabio Estevam <fabio.estevam@freescale.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] clk: register fixed-clock only if #clock-cells property is present
2014-03-26 20:14 ` Fabio Estevam
@ 2014-03-26 20:25 ` Gregory CLEMENT
2014-03-26 20:34 ` Fabio Estevam
2014-03-27 7:58 ` Boris BREZILLON
1 sibling, 1 reply; 14+ messages in thread
From: Gregory CLEMENT @ 2014-03-26 20:25 UTC (permalink / raw)
To: linux-arm-kernel
On 26/03/2014 21:14, Fabio Estevam wrote:
> On Wed, Mar 26, 2014 at 4:57 PM, Sylwester Nawrocki
> <sylvester.nawrocki@gmail.com> wrote:
>
>> Perhaps a change as below helps ?
>>
>> From 85ee85e4a92b42442354f3f2454be50c173e1c59 Mon Sep 17 00:00:00 2001
>> From: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> Date: Wed, 26 Mar 2014 20:54:13 +0100
>> Subject: [PATCH] clk: reverse default clk provider initialization order in
>> of_clk_init()
>>
>>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> ---
>> drivers/clk/clk.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index fb3c40b..d30809c 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -2608,7 +2608,7 @@ void __init of_clk_init(const struct of_device_id
>> *matches)
>>
>> parent->clk_init_cb = match->data;
>> parent->np = np;
>> - list_add(&parent->node, &clk_provider_list);
>> + list_add_tail(&parent->node, &clk_provider_list);
>> }
>>
>> while (!list_empty(&clk_provider_list)) {
>
> Thanks, Sylwester!
>
> This makes my imx6q-wandboard to boot again.
>
> Tested-by: Fabio Estevam <fabio.estevam@freescale.com>
>
Hi Fabio, Sylwester,
I am happy there was a solution!
However, Fabio, could you apply the following patch without the patch
"clk: reverse default clk provider initialization order in
of_clk_init()" and then sent me the traces. I would like to be sure
that there is not any side effect.
Thanks!
---
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index fb3c40b4fbe2..0824cf38f79a 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2558,6 +2558,7 @@ static int parent_ready(struct device_node *np)
while (true) {
struct clk *clk = of_clk_get(np, i);
+ pr_warn("%s: Testing %s \n", __func__, np->name);
/* this parent is ready we can check the next one */
if (!IS_ERR(clk)) {
@@ -2567,8 +2568,10 @@ static int parent_ready(struct device_node *np)
}
/* at least one parent is not ready, we exit now */
- if (PTR_ERR(clk) == -EPROBE_DEFER)
+ if (PTR_ERR(clk) == -EPROBE_DEFER) {
+ pr_warn("%s: Not ready \n", __func__);
return 0;
+ }
/*
* Here we make assumption that the device tree is
@@ -2578,6 +2581,7 @@ static int parent_ready(struct device_node *np)
* parent, no need to wait for them, then we can
* consider their absence as being ready
*/
+ pr_warn("%s: Ready \n", __func__);
return 1;
}
}
@@ -2616,6 +2620,7 @@ void __init of_clk_init(const struct of_device_id *matches)
list_for_each_entry_safe(clk_provider, next,
&clk_provider_list, node) {
if (force || parent_ready(clk_provider->np)) {
+ pr_warn("%s: Initializing %s \n", __func__, clk_provider->np->name);
clk_provider->clk_init_cb(clk_provider->np);
list_del(&clk_provider->node);
kfree(clk_provider);
@@ -2629,8 +2634,10 @@ void __init of_clk_init(const struct of_device_id *matches)
* initialize all the remaining ones unconditionally
* in case the clock parent was not mandatory
*/
- if (!is_init_done)
+ if (!is_init_done) {
+ pr_warn("%s: Now force the initialization of the remaning clocks\n", __func__);
force = true;
+ }
}
}
--
1.8.1.2
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH] clk: register fixed-clock only if #clock-cells property is present
2014-03-26 20:25 ` Gregory CLEMENT
@ 2014-03-26 20:34 ` Fabio Estevam
0 siblings, 0 replies; 14+ messages in thread
From: Fabio Estevam @ 2014-03-26 20:34 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Mar 26, 2014 at 5:25 PM, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:
> However, Fabio, could you apply the following patch without the patch
> "clk: reverse default clk provider initialization order in
> of_clk_init()" and then sent me the traces. I would like to be sure
> that there is not any side effect.
Here is the result, thanks.
Hierarchical RCU implementation.
NR_IRQS:16 nr_irqs:16 16
L310 cache controller enabled
l2x0: 16 ways, CACHE_ID 0x410000c7, AUX_CTRL 0x32070000, Cache size: 1024 kB
parent_ready: Testing ccm
parent_ready: Ready
of_clk_init: Initializing ccm
Switching to timer-based delay loop
Division by zero in kernel.
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.14.0-rc8-next-20140326+ #951
Backtrace:
[<80011cd4>] (dump_backtrace) from [<80011e70>] (show_stack+0x18/0x1c)
r6:00000000 r5:00000000 r4:00000000 r3:00000000
[<80011e58>] (show_stack) from [<8064cc60>] (dump_stack+0x88/0xa4)
[<8064cbd8>] (dump_stack) from [<80011a94>] (__div0+0x18/0x20)
r5:00000000 r4:00000000
[<80011a7c>] (__div0) from [<8029582c>] (Ldiv0_64+0x8/0x18)
[<8007e854>] (clocks_calc_mult_shift) from [<8089a8d0>]
(sched_clock_register+0x60/0x238)
r10:f0018074 r9:00000000 r8:80020ba0 r7:00000020 r6:00000000 r5:80932eac
r4:00000000
[<8089a870>] (sched_clock_register) from [<80884588>]
(mxc_timer_init+0xf8/0x190)
r10:f0018074 r9:00000000 r8:00000057 r7:f0020024 r6:ee022b80 r5:80932eac
r4:00000000
[<80884490>] (mxc_timer_init) from [<808910b0>]
(imx6q_clocks_init+0x2c00/0x2d24)
r8:00000004 r7:809334d0 r6:808c7380 r5:ee7ccbe8 r4:f0020000
[<8088e4b0>] (imx6q_clocks_init) from [<808b02c4>] (of_clk_init+0x150/0x208)
r10:809234d4 r9:00000000 r8:ee7ce5b0 r7:ee002ac0 r6:ee002b00 r5:fffffffe
r4:00000001
[<808b0174>] (of_clk_init) from [<80880364>] (time_init+0x2c/0x38)
r10:ef7fc9c0 r9:412fc09a r8:808d6880 r7:808bd800 r6:ffffffff r5:809329c0
r4:00000001
[<80880338>] (time_init) from [<8087ca2c>] (start_kernel+0x1f8/0x388)
[<8087c834>] (start_kernel) from [<10008074>] (0x10008074)
r10:00000000 r8:1000406a r7:808db644 r6:808bd7fc r5:808d692c r4:10c5387d
sched_clock: 32 bits at 0 Hz, resolution 0ns, wraps every 0ns
Division by zero in kernel.
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.14.0-rc8-next-20140326+ #951
Backtrace:
[<80011cd4>] (dump_backtrace) from [<80011e70>] (show_stack+0x18/0x1c)
r6:ee011a00 r5:00000000 r4:00000000 r3:00000000
[<80011e58>] (show_stack) from [<8064cc60>] (dump_stack+0x88/0xa4)
[<8064cbd8>] (dump_stack) from [<80011a94>] (__div0+0x18/0x20)
r5:00000000 r4:00000000
[<80011a7c>] (__div0) from [<8029582c>] (Ldiv0_64+0x8/0x18)
[<8007ea84>] (__clocksource_updatefreq_scale) from [<8007ec3c>]
(__clocksource_register_scale+0x14/0x54)
r10:000000c8 r9:00000000 r8:f0020024 r7:807b3e68 r6:ee0119c0 r5:00000000
r4:ee011a00
[<8007ec28>] (__clocksource_register_scale) from [<808ae6f8>]
(clocksource_mmio_init+0x8c/0xa8)
r4:ffffffff r3:00000001
[<808ae66c>] (clocksource_mmio_init) from [<808845ac>]
(mxc_timer_init+0x11c/0x190)
r10:f0018074 r8:00000057 r7:f0020024 r6:ee022b80 r5:80932eac r4:808dc900
[<80884490>] (mxc_timer_init) from [<808910b0>]
(imx6q_clocks_init+0x2c00/0x2d24)
r8:00000004 r7:809334d0 r6:808c7380 r5:ee7ccbe8 r4:f0020000
[<8088e4b0>] (imx6q_clocks_init) from [<808b02c4>] (of_clk_init+0x150/0x208)
r10:809234d4 r9:00000000 r8:ee7ce5b0 r7:ee002ac0 r6:ee002b00 r5:fffffffe
r4:00000001
[<808b0174>] (of_clk_init) from [<80880364>] (time_init+0x2c/0x38)
r10:ef7fc9c0 r9:412fc09a r8:808d6880 r7:808bd800 r6:ffffffff r5:809329c0
r4:00000001
[<80880338>] (time_init) from [<8087ca2c>] (start_kernel+0x1f8/0x388)
[<8087c834>] (start_kernel) from [<10008074>] (0x10008074)
r10:00000000 r8:1000406a r7:808db644 r6:808bd7fc r5:808d692c r4:10c5387d
Division by zero in kernel.
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.14.0-rc8-next-20140326+ #951
Backtrace:
[<80011cd4>] (dump_backtrace) from [<80011e70>] (show_stack+0x18/0x1c)
r6:ee011a00 r5:00000000 r4:00000000 r3:00000000
[<80011e58>] (show_stack) from [<8064cc60>] (dump_stack+0x88/0xa4)
[<8064cbd8>] (dump_stack) from [<80011a94>] (__div0+0x18/0x20)
r5:00000000 r4:00000000
[<80011a7c>] (__div0) from [<8029582c>] (Ldiv0_64+0x8/0x18)
[<8007e854>] (clocks_calc_mult_shift) from [<8007eb28>]
(__clocksource_updatefreq_scale+0xa4/0x1a4)
r10:00000001 r9:a3d70a3d r8:70a3d70a r7:0000000b r6:ee011a00 r5:00000001
r4:00000001
[<8007ea84>] (__clocksource_updatefreq_scale) from [<8007ec3c>]
(__clocksource_register_scale+0x14/0x54)
r10:000000c8 r9:00000000 r8:f0020024 r7:807b3e68 r6:ee0119c0 r5:00000000
r4:ee011a00
[<8007ec28>] (__clocksource_register_scale) from [<808ae6f8>]
(clocksource_mmio_init+0x8c/0xa8)
r4:ffffffff r3:00000001
[<808ae66c>] (clocksource_mmio_init) from [<808845ac>]
(mxc_timer_init+0x11c/0x190)
r10:f0018074 r8:00000057 r7:f0020024 r6:ee022b80 r5:80932eac r4:808dc900
[<80884490>] (mxc_timer_init) from [<808910b0>]
(imx6q_clocks_init+0x2c00/0x2d24)
r8:00000004 r7:809334d0 r6:808c7380 r5:ee7ccbe8 r4:f0020000
[<8088e4b0>] (imx6q_clocks_init) from [<808b02c4>] (of_clk_init+0x150/0x208)
r10:809234d4 r9:00000000 r8:ee7ce5b0 r7:ee002ac0 r6:ee002b00 r5:fffffffe
r4:00000001
[<808b0174>] (of_clk_init) from [<80880364>] (time_init+0x2c/0x38)
r10:ef7fc9c0 r9:412fc09a r8:808d6880 r7:808bd800 r6:ffffffff r5:809329c0
r4:00000001
[<80880338>] (time_init) from [<8087ca2c>] (start_kernel+0x1f8/0x388)
[<8087c834>] (start_kernel) from [<10008074>] (0x10008074)
r10:00000000 r8:1000406a r7:808db644 r6:808bd7fc r5:808d692c r4:10c5387d
Division by zero in kernel.
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.14.0-rc8-next-20140326+ #951
Backtrace:
[<80011cd4>] (dump_backtrace) from [<80011e70>] (show_stack+0x18/0x1c)
r6:ee022b80 r5:00000000 r4:00000000 r3:00000000
[<80011e58>] (show_stack) from [<8064cc60>] (dump_stack+0x88/0xa4)
[<8064cbd8>] (dump_stack) from [<80011a94>] (__div0+0x18/0x20)
r5:808dc900 r4:00000000
[<80011a7c>] (__div0) from [<8029582c>] (Ldiv0_64+0x8/0x18)
[<80082314>] (clockevents_config) from [<800823b4>]
(clockevents_config_and_register+0x1c/0x28)
r5:80932eac r4:808dc900
[<80082398>] (clockevents_config_and_register) from [<808845d8>]
(mxc_timer_init+0x148/0x190)
r4:808dc900 r3:fffffffe
[<80884490>] (mxc_timer_init) from [<808910b0>]
(imx6q_clocks_init+0x2c00/0x2d24)
r8:00000004 r7:809334d0 r6:808c7380 r5:ee7ccbe8 r4:f0020000
[<8088e4b0>] (imx6q_clocks_init) from [<808b02c4>] (of_clk_init+0x150/0x208)
r10:809234d4 r9:00000000 r8:ee7ce5b0 r7:ee002ac0 r6:ee002b00 r5:fffffffe
r4:00000001
[<808b0174>] (of_clk_init) from [<80880364>] (time_init+0x2c/0x38)
r10:ef7fc9c0 r9:412fc09a r8:808d6880 r7:808bd800 r6:ffffffff r5:809329c0
r4:00000001
[<80880338>] (time_init) from [<8087ca2c>] (start_kernel+0x1f8/0x388)
[<8087c834>] (start_kernel) from [<10008074>] (0x10008074)
r10:00000000 r8:1000406a r7:808db644 r6:808bd7fc r5:808d692c r4:10c5387d
------------[ cut here ]------------
WARNING: CPU: 0 PID: 0 at kernel/time/clockevents.c:44 cev_delta2ns+0xe8/0x104()
Modules linked in:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.14.0-rc8-next-20140326+ #951
Backtrace:
[<80011cd4>] (dump_backtrace) from [<80011e70>] (show_stack+0x18/0x1c)
r6:80081cb8 r5:00000000 r4:00000000 r3:00000000
[<80011e58>] (show_stack) from [<8064cc60>] (dump_stack+0x88/0xa4)
[<8064cbd8>] (dump_stack) from [<80028f78>] (warn_slowpath_common+0x70/0x94)
r5:00000009 r4:00000000
[<80028f08>] (warn_slowpath_common) from [<80028fc0>]
(warn_slowpath_null+0x24/0x2c)
r8:000000ff r7:000000ff r6:00000000 r5:808dc900 r4:00000000
[<80028f9c>] (warn_slowpath_null) from [<80081cb8>] (cev_delta2ns+0xe8/0x104)
[<80081bd0>] (cev_delta2ns) from [<80082374>] (clockevents_config+0x60/0x84)
r9:00000000 r8:00000057 r7:f0020024 r6:ee022b80 r5:808dc900 r4:00000000
[<80082314>] (clockevents_config) from [<800823b4>]
(clockevents_config_and_register+0x1c/0x28)
r5:80932eac r4:808dc900
[<80082398>] (clockevents_config_and_register) from [<808845d8>]
(mxc_timer_init+0x148/0x190)
r4:808dc900 r3:fffffffe
[<80884490>] (mxc_timer_init) from [<808910b0>]
(imx6q_clocks_init+0x2c00/0x2d24)
r8:00000004 r7:809334d0 r6:808c7380 r5:ee7ccbe8 r4:f0020000
[<8088e4b0>] (imx6q_clocks_init) from [<808b02c4>] (of_clk_init+0x150/0x208)
r10:809234d4 r9:00000000 r8:ee7ce5b0 r7:ee002ac0 r6:ee002b00 r5:fffffffe
r4:00000001
[<808b0174>] (of_clk_init) from [<80880364>] (time_init+0x2c/0x38)
r10:ef7fc9c0 r9:412fc09a r8:808d6880 r7:808bd800 r6:ffffffff r5:809329c0
r4:00000001
[<80880338>] (time_init) from [<8087ca2c>] (start_kernel+0x1f8/0x388)
[<8087c834>] (start_kernel) from [<10008074>] (0x10008074)
r10:00000000 r8:1000406a r7:808db644 r6:808bd7fc r5:808d692c r4:10c5387d
---[ end trace 3406ff24bd97382e ]---
parent_ready: Testing osc
parent_ready: Ready
of_clk_init: Initializing osc
parent_ready: Testing ckih1
parent_ready: Ready
of_clk_init: Initializing ckih1
parent_ready: Testing ckil
parent_ready: Ready
of_clk_init: Initializing ckil
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] clk: register fixed-clock only if #clock-cells property is present
2014-03-26 20:14 ` Fabio Estevam
2014-03-26 20:25 ` Gregory CLEMENT
@ 2014-03-27 7:58 ` Boris BREZILLON
2014-03-27 8:11 ` [PATCH] ARM: imx6/dt: add ccm dependency upon ckil, ckih1 and osc clocks Boris BREZILLON
2014-03-27 10:01 ` [PATCH] clk: register fixed-clock only if #clock-cells property is present Sylwester Nawrocki
1 sibling, 2 replies; 14+ messages in thread
From: Boris BREZILLON @ 2014-03-27 7:58 UTC (permalink / raw)
To: linux-arm-kernel
Le 26/03/2014 21:14, Fabio Estevam a ?crit :
> On Wed, Mar 26, 2014 at 4:57 PM, Sylwester Nawrocki
> <sylvester.nawrocki@gmail.com> wrote:
>
>> Perhaps a change as below helps ?
>>
>> From 85ee85e4a92b42442354f3f2454be50c173e1c59 Mon Sep 17 00:00:00 2001
>> From: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> Date: Wed, 26 Mar 2014 20:54:13 +0100
>> Subject: [PATCH] clk: reverse default clk provider initialization order in
>> of_clk_init()
>>
>>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> ---
>> drivers/clk/clk.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index fb3c40b..d30809c 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -2608,7 +2608,7 @@ void __init of_clk_init(const struct of_device_id
>> *matches)
>>
>> parent->clk_init_cb = match->data;
>> parent->np = np;
>> - list_add(&parent->node, &clk_provider_list);
>> + list_add_tail(&parent->node, &clk_provider_list);
>> }
>>
>> while (!list_empty(&clk_provider_list)) {
> Thanks, Sylwester!
>
> This makes my imx6q-wandboard to boot again.
>
> Tested-by: Fabio Estevam <fabio.estevam@freescale.com>
This solution solve the problem for this specific case because clks are
declared in the correct order in imx DTs.
But, even with your patch I think we could see similar issues by
reordering DT
nodes...
The real problem here is that imx platform does not declare the CCM clocks
dependencies upon ckil, ckih1 and osc fixed clocks within the DT [1], and
retrieve these clocks when initializing the CCM clocks ([2] and [3]).
We should try to a add these dependencies in the DT and see if it works.
[1] http://lxr.free-electrons.com/source/arch/arm/boot/dts/imx6sl.dtsi#L379
[2] http://lxr.free-electrons.com/source/arch/arm/mach-imx/clk-imx6q.c#L151
[3] http://lxr.free-electrons.com/source/arch/arm/mach-imx/clk.c#L30
Best Regards,
Boris
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] ARM: imx6/dt: add ccm dependency upon ckil, ckih1 and osc clocks
2014-03-27 7:58 ` Boris BREZILLON
@ 2014-03-27 8:11 ` Boris BREZILLON
2014-03-27 8:39 ` Boris BREZILLON
2014-03-27 11:19 ` Fabio Estevam
2014-03-27 10:01 ` [PATCH] clk: register fixed-clock only if #clock-cells property is present Sylwester Nawrocki
1 sibling, 2 replies; 14+ messages in thread
From: Boris BREZILLON @ 2014-03-27 8:11 UTC (permalink / raw)
To: linux-arm-kernel
Signed-off-by: Boris BREZILLON <b.brezillon.dev@gmail.com>
---
arch/arm/boot/dts/imx6qdl.dtsi | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
index cfc85be..060e94c 100644
--- a/arch/arm/boot/dts/imx6qdl.dtsi
+++ b/arch/arm/boot/dts/imx6qdl.dtsi
@@ -62,16 +62,19 @@
ckil {
compatible = "fsl,imx-ckil", "fixed-clock";
+ #clock-cells = <0>;
clock-frequency = <32768>;
};
ckih1 {
compatible = "fsl,imx-ckih1", "fixed-clock";
+ #clock-cells = <0>;
clock-frequency = <0>;
};
osc {
compatible = "fsl,imx-osc", "fixed-clock";
+ #clock-cells = <0>;
clock-frequency = <24000000>;
};
};
@@ -489,6 +492,7 @@
interrupts = <0 87 IRQ_TYPE_LEVEL_HIGH>,
<0 88 IRQ_TYPE_LEVEL_HIGH>;
#clock-cells = <1>;
+ clocks = <&ckil &ckih1 &osc>;
};
anatop: anatop at 020c8000 {
--
1.7.9.5
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH] ARM: imx6/dt: add ccm dependency upon ckil, ckih1 and osc clocks
2014-03-27 8:11 ` [PATCH] ARM: imx6/dt: add ccm dependency upon ckil, ckih1 and osc clocks Boris BREZILLON
@ 2014-03-27 8:39 ` Boris BREZILLON
2014-03-27 11:19 ` Fabio Estevam
1 sibling, 0 replies; 14+ messages in thread
From: Boris BREZILLON @ 2014-03-27 8:39 UTC (permalink / raw)
To: linux-arm-kernel
Le 27/03/2014 09:11, Boris BREZILLON a ?crit :
> Signed-off-by: Boris BREZILLON <b.brezillon.dev@gmail.com>
> ---
> arch/arm/boot/dts/imx6qdl.dtsi | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
> index cfc85be..060e94c 100644
> --- a/arch/arm/boot/dts/imx6qdl.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl.dtsi
> @@ -62,16 +62,19 @@
>
> ckil {
Oops.
You'll have to change this line into the line below in order to be able
to reference this clk.
ckil: ckil {
> compatible = "fsl,imx-ckil", "fixed-clock";
> + #clock-cells = <0>;
> clock-frequency = <32768>;
> };
>
> ckih1 {
ditto
> compatible = "fsl,imx-ckih1", "fixed-clock";
> + #clock-cells = <0>;
> clock-frequency = <0>;
> };
>
> osc {
ditto
> compatible = "fsl,imx-osc", "fixed-clock";
> + #clock-cells = <0>;
> clock-frequency = <24000000>;
> };
> };
> @@ -489,6 +492,7 @@
> interrupts = <0 87 IRQ_TYPE_LEVEL_HIGH>,
> <0 88 IRQ_TYPE_LEVEL_HIGH>;
> #clock-cells = <1>;
> + clocks = <&ckil &ckih1 &osc>;
> };
>
> anatop: anatop at 020c8000 {
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] clk: register fixed-clock only if #clock-cells property is present
2014-03-27 7:58 ` Boris BREZILLON
2014-03-27 8:11 ` [PATCH] ARM: imx6/dt: add ccm dependency upon ckil, ckih1 and osc clocks Boris BREZILLON
@ 2014-03-27 10:01 ` Sylwester Nawrocki
2014-03-27 11:14 ` Boris BREZILLON
1 sibling, 1 reply; 14+ messages in thread
From: Sylwester Nawrocki @ 2014-03-27 10:01 UTC (permalink / raw)
To: linux-arm-kernel
Hi Boris,
On 27/03/14 08:58, Boris BREZILLON wrote:
> This solution solve the problem for this specific case because clks are
> declared in the correct order in imx DTs.
> But, even with your patch I think we could see similar issues by
> reordering DT nodes...
>
> The real problem here is that imx platform does not declare the CCM clocks
> dependencies upon ckil, ckih1 and osc fixed clocks within the DT [1], and
> retrieve these clocks when initializing the CCM clocks ([2] and [3]).
>
> We should try to a add these dependencies in the DT and see if it works.
While presumably all of us agree the dependencies should be correctly
specified in dts I think we should minimize possible regressions by
keeping the clocks registration order as before, i.e. as parsed by the
kernel from DT. Rather than explicitly reversing it, which does not gain
us anything AFAICS. Instead we are seeing regressions where new kernels
stop working with old dtbs.
I'm going to resend the patch replacing list_add() with list_add_tail(),
with this the mvebu platform would work and there should be no regression
on imx and exynos.
Please note that specifying dependencies between CCM on imx and the fixed
clocks might not be enough. If the fixed clocks get matched on "fixed-clock"
compatible some clock specifiers (i.e. those using phandle to the CCM) could
get invalid, since the clocks won't get registered by the ccm driver, but by
the regular fixed clock driver. That means a phandle to different node would
need to be used to reference the fixed clock. I'm not sure if this is the case
for imx, but changes may be needed all over various dts files.
In addition, we should make sure the kernel works with current and modified
dtbs.
> [1] http://lxr.free-electrons.com/source/arch/arm/boot/dts/imx6sl.dtsi#L379
> [2] http://lxr.free-electrons.com/source/arch/arm/mach-imx/clk-imx6q.c#L151
> [3] http://lxr.free-electrons.com/source/arch/arm/mach-imx/clk.c#L30
--
Thanks,
Sylwester
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] clk: register fixed-clock only if #clock-cells property is present
2014-03-26 18:22 [PATCH] clk: register fixed-clock only if #clock-cells property is present Sylwester Nawrocki
2014-03-26 18:33 ` Fabio Estevam
@ 2014-03-27 10:55 ` Sylwester Nawrocki
1 sibling, 0 replies; 14+ messages in thread
From: Sylwester Nawrocki @ 2014-03-27 10:55 UTC (permalink / raw)
To: linux-arm-kernel
Mike, please ignore this patch for now. It turns out a less intrusive
change [1] is enough to fix the regressions on both imx and exynos.
I'm going to address the issue properly for next kernel release and
make exynos use regular fixed-clock driver, rather than registering
the external clock generators within the SoC main clock controller
driver.
--
Thanks,
Sylwester
[1] http://permalink.gmane.org/gmane.linux.kernel/1673639
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] clk: register fixed-clock only if #clock-cells property is present
2014-03-27 10:01 ` [PATCH] clk: register fixed-clock only if #clock-cells property is present Sylwester Nawrocki
@ 2014-03-27 11:14 ` Boris BREZILLON
0 siblings, 0 replies; 14+ messages in thread
From: Boris BREZILLON @ 2014-03-27 11:14 UTC (permalink / raw)
To: linux-arm-kernel
Hi Sylwester,
Le 27/03/2014 11:01, Sylwester Nawrocki a ?crit :
> Hi Boris,
>
> On 27/03/14 08:58, Boris BREZILLON wrote:
>> This solution solve the problem for this specific case because clks are
>> declared in the correct order in imx DTs.
>> But, even with your patch I think we could see similar issues by
>> reordering DT nodes...
>>
>> The real problem here is that imx platform does not declare the CCM clocks
>> dependencies upon ckil, ckih1 and osc fixed clocks within the DT [1], and
>> retrieve these clocks when initializing the CCM clocks ([2] and [3]).
>>
>> We should try to a add these dependencies in the DT and see if it works.
> While presumably all of us agree the dependencies should be correctly
> specified in dts I think we should minimize possible regressions by
> keeping the clocks registration order as before, i.e. as parsed by the
> kernel from DT. Rather than explicitly reversing it, which does not gain
> us anything AFAICS. Instead we are seeing regressions where new kernels
> stop working with old dtbs.
I totally agree with you on this point: my patch is not a replacement of
yours.
I just wanted to point out that we need to fix DT definitions to avoid these
kind of issues in the future.
>
> I'm going to resend the patch replacing list_add() with list_add_tail(),
> with this the mvebu platform would work and there should be no regression
> on imx and exynos.
>
> Please note that specifying dependencies between CCM on imx and the fixed
> clocks might not be enough. If the fixed clocks get matched on "fixed-clock"
> compatible some clock specifiers (i.e. those using phandle to the CCM) could
> get invalid, since the clocks won't get registered by the ccm driver, but by
> the regular fixed clock driver. That means a phandle to different node would
> need to be used to reference the fixed clock. I'm not sure if this is the case
> for imx, but changes may be needed all over various dts files.
> In addition, we should make sure the kernel works with current and modified
> dtbs.
>
>> [1] http://lxr.free-electrons.com/source/arch/arm/boot/dts/imx6sl.dtsi#L379
>> [2] http://lxr.free-electrons.com/source/arch/arm/mach-imx/clk-imx6q.c#L151
>> [3] http://lxr.free-electrons.com/source/arch/arm/mach-imx/clk.c#L30
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] ARM: imx6/dt: add ccm dependency upon ckil, ckih1 and osc clocks
2014-03-27 8:11 ` [PATCH] ARM: imx6/dt: add ccm dependency upon ckil, ckih1 and osc clocks Boris BREZILLON
2014-03-27 8:39 ` Boris BREZILLON
@ 2014-03-27 11:19 ` Fabio Estevam
2014-03-27 13:27 ` Boris BREZILLON
1 sibling, 1 reply; 14+ messages in thread
From: Fabio Estevam @ 2014-03-27 11:19 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Mar 27, 2014 at 5:11 AM, Boris BREZILLON
<brezillonboris@gmail.com> wrote:
> Signed-off-by: Boris BREZILLON <b.brezillon.dev@gmail.com>
Please provide a commit message, thanks.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] ARM: imx6/dt: add ccm dependency upon ckil, ckih1 and osc clocks
2014-03-27 11:19 ` Fabio Estevam
@ 2014-03-27 13:27 ` Boris BREZILLON
0 siblings, 0 replies; 14+ messages in thread
From: Boris BREZILLON @ 2014-03-27 13:27 UTC (permalink / raw)
To: linux-arm-kernel
Le 27/03/2014 12:19, Fabio Estevam a ?crit :
> On Thu, Mar 27, 2014 at 5:11 AM, Boris BREZILLON
> <brezillonboris@gmail.com> wrote:
>> Signed-off-by: Boris BREZILLON <b.brezillon.dev@gmail.com>
> Please provide a commit message, thanks.
This was sent as a test to verify my assumptions.
But if you want a commit message, how about this one ?
Define ccm clocks dependencies upon ckil, ckih1 and osc fixed clocks
to enforce probing order.
Anyway this patch is buggy (see my previous answer) and I'm not sure
I'm one that should submit it :-).
Best Regards,
Boris
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2014-03-27 13:27 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-03-26 18:22 [PATCH] clk: register fixed-clock only if #clock-cells property is present Sylwester Nawrocki
2014-03-26 18:33 ` Fabio Estevam
2014-03-26 19:57 ` Sylwester Nawrocki
2014-03-26 20:14 ` Fabio Estevam
2014-03-26 20:25 ` Gregory CLEMENT
2014-03-26 20:34 ` Fabio Estevam
2014-03-27 7:58 ` Boris BREZILLON
2014-03-27 8:11 ` [PATCH] ARM: imx6/dt: add ccm dependency upon ckil, ckih1 and osc clocks Boris BREZILLON
2014-03-27 8:39 ` Boris BREZILLON
2014-03-27 11:19 ` Fabio Estevam
2014-03-27 13:27 ` Boris BREZILLON
2014-03-27 10:01 ` [PATCH] clk: register fixed-clock only if #clock-cells property is present Sylwester Nawrocki
2014-03-27 11:14 ` Boris BREZILLON
2014-03-27 10:55 ` Sylwester Nawrocki
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).