rtc: ac100: Fix ac100 determine rate bug
PATCH v3 1/1 rtc: ac100: Fix ac100 determine rate bug
This patch fixes a bug, that prevents the Allwinner A83T and the A80 from a successful boot.
The bug is there since v4.16-rc1 and appeared after the clk branch was merged.
You can find the shortend trace below:
Unable to handle kernel NULL pointer dereference at virtual address 00000000 pgd = (ptrval) [00000000] *pgd=00000000 Internal error: Oops: 5 [#1] SMP ARM Modules linked in: CPU: 0 PID: 49 Comm: kworker/0:1 Not tainted 4.15.0-10190-gb89e32ccd1be #2 Hardware name: Allwinner sun8i Family Workqueue: events deferred_probe_work_func PC is at clk_hw_get_rate+0x0/0x34 LR is at ac100_clkout_determine_rate+0x48/0x19c
[ ... ]
(clk_hw_get_rate) from (ac100_clkout_determine_rate+0x48/0x19c) (ac100_clkout_determine_rate) from (clk_core_set_rate_nolock+0x3c/0x1a0) (clk_core_set_rate_nolock) from (clk_set_rate+0x30/0x88) (clk_set_rate) from (of_clk_set_defaults+0x200/0x364) (of_clk_set_defaults) from (platform_drv_probe+0x18/0xb0)
To fix that bug, we first check if the return of the clk_hw_get_parent_by_index is non zero. If it is zero we skip that clock parent.
The BUG report could be found here: https://lkml.org/lkml/2018/2/10/198
---
Changes in v3:
* add information when the bug appeared
* make the comment more clear
Changes in v2:
* add tag Fixes: ... to commit message
* add comment to if statement why we are doing this check
drivers/rtc/rtc-ac100.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-)
drivers/rtc/rtc-ac100.c +18 -1
@@ -183,7 +183,24 @@ static int ac100_clkout_determine_rate(struct clk_hw *hw,for (i =0 ; i < num_parents; i++) {struct clk_hw *parent =clk_hw_get_parent_by_index (hw, i); -unsigned long tmp, prate =clk_hw_get_rate (parent); +unsigned long tmp, prate; + +/* +* The clock has two parents, one is a fixed clock which is +* internally registered by the ac100 driver. The other parent +* is a clock from the codec side of the chip, which we +* properly declare and reference in the devicetree and is +* not implemented in any driver right now. +* If the clock core looks for the parent of that second +* missing clock, it can't one that is registered and +* returns NULL. +* Thus we need to check if the parent exists before +* we get the parent rate. +*/ +if (!parent) +continue ; + + prate =clk_hw_get_rate (parent); tmp =ac100_clkout_round_rate (hw, req->rate, prate);
Signature · 1 line
2.11.0
Hi Phillipp,
Quoted 58 lines · Philipp Rossak
This patch fixes a bug, that prevents the Allwinner A83T and the A80 from a successful boot.
The bug is there since v4.16-rc1 and appeared after the clk branch was merged.
You can find the shortend trace below:
Unable to handle kernel NULL pointer dereference at virtual address 00000000 pgd = (ptrval) [00000000] *pgd=00000000 Internal error: Oops: 5 [#1] SMP ARM Modules linked in: CPU: 0 PID: 49 Comm: kworker/0:1 Not tainted 4.15.0-10190-gb89e32ccd1be #2 Hardware name: Allwinner sun8i Family Workqueue: events deferred_probe_work_func PC is at clk_hw_get_rate+0x0/0x34 LR is at ac100_clkout_determine_rate+0x48/0x19c[ ... ]
(
clk_hw_get_rate) from (ac100_clkout_determine_rate+0x48/0x19c) (ac100_clkout_determine_rate) from (clk_core_set_rate_nolock+0x3c/0x1a0) (clk_core_set_rate_nolock) from (clk_set_rate+0x30/0x88) (clk_set_rate) from (of_clk_set_defaults+0x200/0x364) (of_clk_set_defaults) from (platform_drv_probe+0x18/0xb0)To fix that bug, we first check if the return of the
clk_hw_get_parent_by_indexis non zero. If it is zero we skip that clock parent.The BUG report could be found here: https://lkml.org/lkml/2018/2/10/198
Fixes 04940631b8d2 ("rtc: ac100: Add clk output support")Signed-off-by Philipp Rossak <[email protected]>---
Changes in v3:
* add information when the bug appeared
* make the comment more clear
Changes in v2:
* add tag Fixes: ... to commit message
* add comment to if statement why we are doing this checkdrivers/rtc/rtc-ac100.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-)drivers/rtc/rtc-ac100.c +0
-1@@ -183,7 +183,24 @@ static int ac100_clkout_determine_rate(struct clk_hw *hw,for (i =0 ; i < num_parents; i++) {struct clk_hw *parent =clk_hw_get_parent_by_index (hw, i); -unsigned long tmp, prate =clk_hw_get_rate (parent);
drivers/rtc/rtc-ac100.c static int ac100_clkout_determine_rate(struct clk_hw *hw, · line 195
Quoted 10 lines
drivers/rtc/rtc-ac100.c +10
-0+unsigned long tmp, prate; + +/* +* The clock has two parents, one is a fixed clock which is +* internally registered by the ac100 driver. The other parent +* is a clock from the codec side of the chip, which we +* properly declare and reference in the devicetree and is +* not implemented in any driver right now. +* If the clock core looks for the parent of that second +* missing clock, it can't one that is registered and
You've missed the word "find" between "it can't" and "one that is registered".
Thanks,
Signature · 4 lines
Julian Calaby Email: [email protected] Profile: http://www.google.com/profiles/julian.calaby/
Quoted 5 lines · Philipp Rossak
This patch fixes a bug, that prevents the Allwinner A83T and the A80 from a successful boot.
The bug is there since v4.16-rc1 and appeared after the clk branch was merged.
Out of curiosity, which patch has introduced this? I couldn't find any obvious match.
Quoted 58 lines
You can find the shortend trace below:
Unable to handle kernel NULL pointer dereference at virtual address 00000000 pgd = (ptrval) [00000000] *pgd=00000000 Internal error: Oops: 5 [#1] SMP ARM Modules linked in: CPU: 0 PID: 49 Comm: kworker/0:1 Not tainted 4.15.0-10190-gb89e32ccd1be #2 Hardware name: Allwinner sun8i Family Workqueue: events deferred_probe_work_func PC is at clk_hw_get_rate+0x0/0x34 LR is at ac100_clkout_determine_rate+0x48/0x19c[ ... ]
(
clk_hw_get_rate) from (ac100_clkout_determine_rate+0x48/0x19c) (ac100_clkout_determine_rate) from (clk_core_set_rate_nolock+0x3c/0x1a0) (clk_core_set_rate_nolock) from (clk_set_rate+0x30/0x88) (clk_set_rate) from (of_clk_set_defaults+0x200/0x364) (of_clk_set_defaults) from (platform_drv_probe+0x18/0xb0)To fix that bug, we first check if the return of the
clk_hw_get_parent_by_indexis non zero. If it is zero we skip that clock parent.The BUG report could be found here: https://lkml.org/lkml/2018/2/10/198
Fixes 04940631b8d2 ("rtc: ac100: Add clk output support")Signed-off-by Philipp Rossak <[email protected]>---
Changes in v3:
* add information when the bug appeared
* make the comment more clear
Changes in v2:
* add tag Fixes: ... to commit message
* add comment to if statement why we are doing this checkdrivers/rtc/rtc-ac100.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-)drivers/rtc/rtc-ac100.c +6
-1@@ -183,7 +183,24 @@ static int ac100_clkout_determine_rate(struct clk_hw *hw,for (i =0 ; i < num_parents; i++) {struct clk_hw *parent =clk_hw_get_parent_by_index (hw, i); -unsigned long tmp, prate =clk_hw_get_rate (parent); +unsigned long tmp, prate; + +/* +* The clock has two parents, one is a fixed clock which is +* internally registered by the ac100 driver. The other parent +* is a clock from the codec side of the chip, which we
drivers/rtc/rtc-ac100.c static int ac100_clkout_determine_rate(struct clk_hw *hw, · line 201
Quoted 10 lines
drivers/rtc/rtc-ac100.c +10
-0+* properly declare and reference in the devicetree and is +* not implemented in any driver right now. +* If the clock core looks for the parent of that second +* missing clock, it can't one that is registered and +* returns NULL. +* Thus we need to check if the parent exists before +* we get the parent rate. +*/ +if (!parent) +continue ;
I'm sorry, but I still don't get it. When you register that clock, you will give it two parents. Why would that change during the life of the clock?
This really looks like a workaround rather than an actual fix.
Maxime
Signature · 3 lines
Maxime Ripard, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering http://bootlin.com
Quoted 10 lines · Maxime Ripard
Quoted 5 lines · Philipp Rossak
This patch fixes a bug, that prevents the Allwinner A83T and the A80 from a successful boot.
The bug is there since v4.16-rc1 and appeared after the clk branch was merged.
Out of curiosity, which patch has introduced this? I couldn't find any obvious match.
I wasn't also n
Quoted 73 lines
Quoted 58 lines
You can find the shortend trace below:
Unable to handle kernel NULL pointer dereference at virtual address 00000000 pgd = (ptrval) [00000000] *pgd=00000000 Internal error: Oops: 5 [#1] SMP ARM Modules linked in: CPU: 0 PID: 49 Comm: kworker/0:1 Not tainted 4.15.0-10190-gb89e32ccd1be #2 Hardware name: Allwinner sun8i Family Workqueue: events deferred_probe_work_func PC is at clk_hw_get_rate+0x0/0x34 LR is at ac100_clkout_determine_rate+0x48/0x19c[ ... ]
(
clk_hw_get_rate) from (ac100_clkout_determine_rate+0x48/0x19c) (ac100_clkout_determine_rate) from (clk_core_set_rate_nolock+0x3c/0x1a0) (clk_core_set_rate_nolock) from (clk_set_rate+0x30/0x88) (clk_set_rate) from (of_clk_set_defaults+0x200/0x364) (of_clk_set_defaults) from (platform_drv_probe+0x18/0xb0)To fix that bug, we first check if the return of the
clk_hw_get_parent_by_indexis non zero. If it is zero we skip that clock parent.The BUG report could be found here: https://lkml.org/lkml/2018/2/10/198
Fixes 04940631b8d2 ("rtc: ac100: Add clk output support")Signed-off-by Philipp Rossak <[email protected]>---
Changes in v3:
* add information when the bug appeared
* make the comment more clear
Changes in v2:
* add tag Fixes: ... to commit message
* add comment to if statement why we are doing this checkdrivers/rtc/rtc-ac100.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-)drivers/rtc/rtc-ac100.c +6
-1@@ -183,7 +183,24 @@ static int ac100_clkout_determine_rate(struct clk_hw *hw,for (i =0 ; i < num_parents; i++) {struct clk_hw *parent =clk_hw_get_parent_by_index (hw, i); -unsigned long tmp, prate =clk_hw_get_rate (parent); +unsigned long tmp, prate; + +/* +* The clock has two parents, one is a fixed clock which is +* internally registered by the ac100 driver. The other parent +* is a clock from the codec side of the chip, which we drivers/rtc/rtc-ac100.c static int ac100_clkout_determine_rate(struct clk_hw *hw, · line 201
Quoted 10 lines
drivers/rtc/rtc-ac100.c +10
-0+* properly declare and reference in the devicetree and is +* not implemented in any driver right now. +* If the clock core looks for the parent of that second +* missing clock, it can't one that is registered and +* returns NULL. +* Thus we need to check if the parent exists before +* we get the parent rate. +*/ +if (!parent) +continue ;I'm sorry, but I still don't get it. When you register that clock, you will give it two parents. Why would that change during the life of the clock?
Quoted 4 lines
This really looks like a workaround rather than an actual fix.
Maxime
I agree this is more a workaround!
A proper solution/fix would be to define the devicetree correct like this:
arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts +8 -1
@@ -175,11 +175,18 @@ compatible = "x-powers,ac100-rtc"; interrupt-parent = <&r_intc>; interrupts = <0 IRQ_TYPE_LEVEL_LOW>; - clocks = <&ac100_codec>; + clocks = <&ac100_rtc_32k>; #clock-cells = <1>; clock-output-names = "cko1_rtc", "cko2_rtc", "cko3_rtc"; + + ac100_rtc_32k: rtc-32k-oscillator { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <32768>; + clock-output-names = "ac100-rtc-32k"; + }; }; }; };
What do you think about that solution?
I already tested it and it looks like it is working.
Philipp
Quoted 13 lines · Philipp Rossak
Quoted 10 lines · Maxime Ripard
Quoted 6 lines · Philipp Rossak
This patch fixes a bug, that prevents the Allwinner A83T and the A80 from a successful boot.
The bug is there since v4.16-rc1 and appeared after the clk branch was merged.
Quoted 5 lines
Quoted 3 lines
Out of curiosity, which patch has introduced this? I couldn't find any obvious match.
I wasn't also n
To be honest, I'm not sure why this is hitting you and not me. I have both A83T boards that have assigned-clock-rates set for the ac100 clock outputs for WiFi. I have them running 4.16-rc1 and have not seen this. The device tree patches that add these are in 4.15.
Quoted 113 lines
Quoted 80 lines
Quoted 65 lines
You can find the shortend trace below:
Unable to handle kernel NULL pointer dereference at virtual address 00000000 pgd = (ptrval) [00000000] *pgd=00000000 Internal error: Oops: 5 [#1] SMP ARM Modules linked in: CPU: 0 PID: 49 Comm: kworker/0:1 Not tainted 4.15.0-10190-gb89e32ccd1be #2 Hardware name: Allwinner sun8i Family Workqueue: events deferred_probe_work_func PC is at clk_hw_get_rate+0x0/0x34 LR is at ac100_clkout_determine_rate+0x48/0x19c[ ... ]
(
clk_hw_get_rate) from (ac100_clkout_determine_rate+0x48/0x19c) (ac100_clkout_determine_rate) from (clk_core_set_rate_nolock+0x3c/0x1a0) (clk_core_set_rate_nolock) from (clk_set_rate+0x30/0x88) (clk_set_rate) from (of_clk_set_defaults+0x200/0x364) (of_clk_set_defaults) from (platform_drv_probe+0x18/0xb0)To fix that bug, we first check if the return of the
clk_hw_get_parent_by_indexis non zero. If it is zero we skip that clock parent.The BUG report could be found here: https://lkml.org/lkml/2018/2/10/198
Fixes 04940631b8d2 ("rtc: ac100: Add clk output support")Signed-off-by Philipp Rossak <[email protected]>---
Changes in v3:
* add information when the bug appeared
* make the comment more clear
Changes in v2:
* add tag Fixes: ... to commit message
* add comment to if statement why we are doing this checkdrivers/rtc/rtc-ac100.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-)drivers/rtc/rtc-ac100.c +4
-1@@ -183,7 +183,24 @@ static int ac100_clkout_determine_rate(struct clk_hw *hw, for (i = 0; i < num_parents; i++) {struct clk_hw *parent =clk_hw_get_parent_by_index (hw, i); -unsigned long tmp, prate =clk_hw_get_rate (parent); +unsigned long tmp, prate; + +/* +* The clock has two parents, one is a fixed clock which is
+ * internally registered by the ac100 driver. The other
parentdrivers/rtc/rtc-ac100.c +2
-0+* is a clock from the codec side of the chip, which we +* properly declare and reference in the devicetree and is
drivers/rtc/rtc-ac100.c
Quoted 9 lines
drivers/rtc/rtc-ac100.c +9
-0+* not implemented in any driver right now. +* If the clock core looks for the parent of that second +* missing clock, it can't one that is registered and +* returns NULL. +* Thus we need to check if the parent exists before +* we get the parent rate. +*/ +if (!parent) +continue ;I'm sorry, but I still don't get it. When you register that clock, you will give it two parents. Why would that change during the life of the clock?
Quoted 4 lines
This really looks like a workaround rather than an actual fix.
Maxime
I agree this is more a workaround!
A proper solution/fix would be to define the devicetree correct like this:arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts +8
-1@@ -175,11 +175,18 @@ compatible = "x-powers,ac100-rtc"; interrupt-parent = <&r_intc>; interrupts = <0 IRQ_TYPE_LEVEL_LOW>; - clocks = <&ac100_codec>; + clocks = <&ac100_rtc_32k>; #clock-cells = <1>; clock-output-names = "cko1_rtc", "cko2_rtc", "cko3_rtc"; + + ac100_rtc_32k: rtc-32k-oscillator { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <32768>; + clock-output-names = "ac100-rtc-32k"; + }; }; }; };
Quoted 1 line
What do you think about that solution?
That's not quite right either. As I mentioned before, the RTC block has two clock inputs, one 4MHz signal from the codec block, and one 32.768 kHz signal from an external crystal. The original device tree binding describes the first one, and the 32.768 kHz clock was registered by the RTC driver internally.
If you're going to add the crystal clock, you still need to keep the codec one. Note that this does not fix what Maxime is asking you. I've already provided an explanation:
The clock core allows registering clocks with not-yet-existing clock parents. Parents are matches by string names. If no clock by that name is registered yet, the clock core simply orphans the new clock if the unregistered parent is its current parent or simply ignores that parent if its not the current parent. This is entirely valid and is what we are counting on here, as we haven't implemented the codec-side driver.
Note we also sort of depend on this behavior for sunxi-ng clocks, where in some cases we get circular dependencies between clock blocks.
BTW, since this is mostly related to the clk subsystem, please CC the clk maintainers as well.
Quoted 2 lines
I already tested it and it looks like it is working.
There is no point in testing things if it doesn't match the hardware. Yes your code works and doesn't crash. No it does not represent what the hardware is actually doing.
Regards
ChenYu
Quoted 18 lines · Chen-Yu Tsai
Quoted 17 lines · Philipp Rossak
Quoted 13 lines · Maxime Ripard
Quoted 6 lines · Philipp Rossak
This patch fixes a bug, that prevents the Allwinner A83T and the A80 from a successful boot.
The bug is there since v4.16-rc1 and appeared after the clk branch was merged.
Out of curiosity, which patch has introduced this? I couldn't find any obvious match.
Quoted 8 lines
Quoted 1 line
I wasn't also n
To be honest, I'm not sure why this is hitting you and not me. I have both A83T boards that have assigned-clock-rates set for the ac100 clock outputs for WiFi. I have them running 4.16-rc1 and have not seen this. The device tree patches that add these are in 4.15.
Now it is getting curious ... .
I already mentioned that bug in the sunxi-irc and someone else was hitting that problem also...
I tested it also with the same toolchain you are using (gcc 7.3.0-1 Debian), but that didn't made any difference.
I don't think that issue is related with the Hardware, but to be on the save side: Which Hardware version of the BPI-M3 do you have? I have version 1.2.
Can someone else can confirm this bug?
Quoted 135 lines
Quoted 113 lines
Quoted 80 lines
Quoted 65 lines
You can find the shortend trace below:
Unable to handle kernel NULL pointer dereference at virtual address 00000000 pgd = (ptrval) [00000000] *pgd=00000000 Internal error: Oops: 5 [#1] SMP ARM Modules linked in: CPU: 0 PID: 49 Comm: kworker/0:1 Not tainted 4.15.0-10190-gb89e32ccd1be #2 Hardware name: Allwinner sun8i Family Workqueue: events deferred_probe_work_func PC is at clk_hw_get_rate+0x0/0x34 LR is at ac100_clkout_determine_rate+0x48/0x19c[ ... ]
(
clk_hw_get_rate) from (ac100_clkout_determine_rate+0x48/0x19c) (ac100_clkout_determine_rate) from (clk_core_set_rate_nolock+0x3c/0x1a0) (clk_core_set_rate_nolock) from (clk_set_rate+0x30/0x88) (clk_set_rate) from (of_clk_set_defaults+0x200/0x364) (of_clk_set_defaults) from (platform_drv_probe+0x18/0xb0)To fix that bug, we first check if the return of the
clk_hw_get_parent_by_indexis non zero. If it is zero we skip that clock parent.The BUG report could be found here: https://lkml.org/lkml/2018/2/10/198
Fixes 04940631b8d2 ("rtc: ac100: Add clk output support")Signed-off-by Philipp Rossak <[email protected]>---
Changes in v3:
* add information when the bug appeared
* make the comment more clear
Changes in v2:
* add tag Fixes: ... to commit message
* add comment to if statement why we are doing this checkdrivers/rtc/rtc-ac100.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-)drivers/rtc/rtc-ac100.c +4
-1@@ -183,7 +183,24 @@ static int ac100_clkout_determine_rate(struct clk_hw *hw, for (i = 0; i < num_parents; i++) {struct clk_hw *parent =clk_hw_get_parent_by_index (hw, i); -unsigned long tmp, prate =clk_hw_get_rate (parent); +unsigned long tmp, prate; + +/* +* The clock has two parents, one is a fixed clock which is
+ * internally registered by the ac100 driver. The other
parentdrivers/rtc/rtc-ac100.c +2
-0+* is a clock from the codec side of the chip, which we +* properly declare and reference in the devicetree and is
drivers/rtc/rtc-ac100.c
Quoted 9 lines
drivers/rtc/rtc-ac100.c +9
-0+* not implemented in any driver right now. +* If the clock core looks for the parent of that second +* missing clock, it can't one that is registered and +* returns NULL. +* Thus we need to check if the parent exists before +* we get the parent rate. +*/ +if (!parent) +continue ;I'm sorry, but I still don't get it. When you register that clock, you will give it two parents. Why would that change during the life of the clock?
Quoted 4 lines
This really looks like a workaround rather than an actual fix.
Maxime
I agree this is more a workaround!
A proper solution/fix would be to define the devicetree correct like this:arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts +8
-1@@ -175,11 +175,18 @@ compatible = "x-powers,ac100-rtc"; interrupt-parent = <&r_intc>; interrupts = <0 IRQ_TYPE_LEVEL_LOW>; - clocks = <&ac100_codec>; + clocks = <&ac100_rtc_32k>; #clock-cells = <1>; clock-output-names = "cko1_rtc", "cko2_rtc", "cko3_rtc"; + + ac100_rtc_32k: rtc-32k-oscillator { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <32768>; + clock-output-names = "ac100-rtc-32k"; + }; }; }; };Quoted 1 line
What do you think about that solution?
That's not quite right either. As I mentioned before, the RTC block has two clock inputs, one 4MHz signal from the codec block, and one 32.768 kHz signal from an external crystal. The original device tree binding describes the first one, and the 32.768 kHz clock was registered by the RTC driver internally.
If you're going to add the crystal clock, you still need to keep the codec one. Note that this does not fix what Maxime is asking you. I've already provided an explanation:
The clock core allows registering clocks with not-yet-existing clock parents. Parents are matches by string names. If no clock by that name is registered yet, the clock core simply orphans the new clock if the unregistered parent is its current parent or simply ignores that parent if its not the current parent. This is entirely valid and is what we are counting on here, as we haven't implemented the codec-side driver.
Quoted 7 lines
Note we also sort of depend on this behavior for sunxi-ng clocks, where in some cases we get circular dependencies between clock blocks.
BTW, since this is mostly related to the clk subsystem, please CC the clk maintainers as well.
Ok, I added also now the clk mailing list and maintainers. As reference this here is the boot log [1].
Im not sure if this in relation to this bug:
But, already with the 4.15 kernel I get from time to time some oops during runtime on the a31s (bpi-m2) and all are clock related and causes a crash of the system.
Philipp
Quoted 42 lines · Philipp Rossak
Quoted 23 lines · Chen-Yu Tsai
Quoted 21 lines · Philipp Rossak
Quoted 16 lines · Maxime Ripard
Quoted 7 lines · Philipp Rossak
This patch fixes a bug, that prevents the Allwinner A83T and the A80 from a successful boot.
The bug is there since v4.16-rc1 and appeared after the clk branch was merged.
Out of curiosity, which patch has introduced this? I couldn't find any obvious match.
Quoted 9 lines
Quoted 1 line
I wasn't also n
To be honest, I'm not sure why this is hitting you and not me. I have both A83T boards that have assigned-clock-rates set for the ac100 clock outputs for WiFi. I have them running 4.16-rc1 and have not seen this. The device tree patches that add these are in 4.15.
Now it is getting curious ... .
I already mentioned that bug in the sunxi-irc and someone else was hitting that problem also...
I tested it also with the same toolchain you are using (gcc 7.3.0-1 Debian), but that didn't made any difference.
Quoted 4 lines
I don't think that issue is related with the Hardware, but to be on the save
side: Which Hardware version of the BPI-M3 do you have? I have version 1.2.Can someone else can confirm this bug?
So I might have remembered wrong, as I just realized I have your patch in my a83t branches. I don't hit this on the A80, which also has the AC100, but doesn't use assigned-clock-rates in the device tree.
Could you try rolling back to 4.15 and see if you still hit it?
Quoted 152 lines
Quoted 144 lines
Quoted 121 lines
Quoted 87 lines
Quoted 73 lines
You can find the shortend trace below:
Unable to handle kernel NULL pointer dereference at virtual address 00000000 pgd = (ptrval) [00000000] *pgd=00000000 Internal error: Oops: 5 [#1] SMP ARM Modules linked in: CPU: 0 PID: 49 Comm: kworker/0:1 Not tainted 4.15.0-10190-gb89e32ccd1be #2 Hardware name: Allwinner sun8i Family Workqueue: events deferred_probe_work_func PC is at clk_hw_get_rate+0x0/0x34 LR is at ac100_clkout_determine_rate+0x48/0x19c[ ... ]
(
clk_hw_get_rate) from (ac100_clkout_determine_rate+0x48/0x19c) (ac100_clkout_determine_rate) from (clk_core_set_rate_nolock+0x3c/0x1a0) (clk_core_set_rate_nolock) from (clk_set_rate+0x30/0x88) (clk_set_rate) from (of_clk_set_defaults+0x200/0x364) (of_clk_set_defaults) from (platform_drv_probe+0x18/0xb0)To fix that bug, we first check if the return of the
clk_hw_get_parent_by_indexis non zero. If it is zero we skip that clock parent.The BUG report could be found here: https://lkml.org/lkml/2018/2/10/198
Fixes 04940631b8d2 ("rtc: ac100: Add clk output support")Signed-off-by Philipp Rossak <[email protected]>---
Changes in v3:
* add information when the bug appeared
* make the comment more clear
Changes in v2:
* add tag Fixes: ... to commit message
* add comment to if statement why we are doing this checkdrivers/rtc/rtc-ac100.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-)drivers/rtc/rtc-ac100.c +0
-0@@ -183,7 +183,24 @@ static int ac100_clkout_determine_rate(struct
clk_hw
*hw,
for (i = 0; i <num_parents; i++) {
structclk_hw*parent = clk_hw_get_parent_by_index(hw,
i);drivers/rtc/rtc-ac100.c +4
-1-unsigned long tmp, prate =clk_hw_get_rate (parent); +unsigned long tmp, prate; + +/* +* The clock has two parents, one is a fixed clock which
is
+ * internally registered by the ac100 driver. The other
parentdrivers/rtc/rtc-ac100.c +2
-0+* is a clock from the codec side of the chip, which we +* properly declare and reference in the devicetree and is
drivers/rtc/rtc-ac100.c +2
-0+* not implemented in any driver right now. +* If the clock core looks for the parent of that second
drivers/rtc/rtc-ac100.c
Quoted 7 lines
drivers/rtc/rtc-ac100.c +7
-0+* missing clock, it can't one that is registered and +* returns NULL. +* Thus we need to check if the parent exists before +* we get the parent rate. +*/ +if (!parent) +continue ;I'm sorry, but I still don't get it. When you register that clock, you will give it two parents. Why would that change during the life of the clock?
Quoted 4 lines
This really looks like a workaround rather than an actual fix.
Maxime
I agree this is more a workaround!
A proper solution/fix would be to define the devicetree correct like this:arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts +8
-1@@ -175,11 +175,18 @@ compatible = "x-powers,ac100-rtc"; interrupt-parent = <&r_intc>; interrupts = <0 IRQ_TYPE_LEVEL_LOW>; - clocks = <&ac100_codec>; + clocks = <&ac100_rtc_32k>; #clock-cells = <1>; clock-output-names = "cko1_rtc", "cko2_rtc", "cko3_rtc"; + + ac100_rtc_32k: rtc-32k-oscillator { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <32768>; + clock-output-names = "ac100-rtc-32k"; + }; }; }; };Quoted 1 line
What do you think about that solution?
That's not quite right either. As I mentioned before, the RTC block has two clock inputs, one 4MHz signal from the codec block, and one 32.768 kHz signal from an external crystal. The original device tree binding describes the first one, and the 32.768 kHz clock was registered by the RTC driver internally.
If you're going to add the crystal clock, you still need to keep the codec one. Note that this does not fix what Maxime is asking you. I've already provided an explanation:
The clock core allows registering clocks with not-yet-existing clock parents. Parents are matches by string names. If no clock by that name is registered yet, the clock core simply orphans the new clock if the unregistered parent is its current parent or simply ignores that parent if its not the current parent. This is entirely valid and is what we are counting on here, as we haven't implemented the codec-side driver.
Quoted 7 lines
Note we also sort of depend on this behavior for sunxi-ng clocks, where in some cases we get circular dependencies between clock blocks.
BTW, since this is mostly related to the clk subsystem, please CC the clk maintainers as well.
Quoted 7 lines
Ok, I added also now the clk mailing list and maintainers. As reference this here is the boot log [1].
Im not sure if this in relation to this bug:
But, already with the 4.15 kernel I get from time to time some oops during runtime on the a31s (bpi-m2) and all are clock related and causes a crash of the system.
Are you referring to the A31 CLK_OUT bug? That is an entirely separate bug in the A31/A31s CCU driver. The bug was there from the beginning, but did not cause a crash as the incompatible data structures had lined up in a way that the embedded data structure that was used by the incorrect clk ops was actually compatible. This changed with 4.16-rc1, as a new field was added to some of the clock types, breaking the alignment. I made some changes but have yet to write the commit log for it. Also I don't have any A31/A31s boards to test it on right now. I can send you the patch later.
ChenYu
Quoted 6 lines
Philipp
Quoted 46 lines · Chen-Yu Tsai
Quoted 45 lines · Philipp Rossak
Quoted 23 lines · Chen-Yu Tsai
Quoted 21 lines · Philipp Rossak
Quoted 16 lines · Maxime Ripard
Quoted 7 lines · Philipp Rossak
This patch fixes a bug, that prevents the Allwinner A83T and the A80 from a successful boot.
The bug is there since v4.16-rc1 and appeared after the clk branch was merged.
Out of curiosity, which patch has introduced this? I couldn't find any obvious match.
Quoted 9 lines
Quoted 1 line
I wasn't also n
To be honest, I'm not sure why this is hitting you and not me. I have both A83T boards that have assigned-clock-rates set for the ac100 clock outputs for WiFi. I have them running 4.16-rc1 and have not seen this. The device tree patches that add these are in 4.15.
Now it is getting curious ... .
I already mentioned that bug in the sunxi-irc and someone else was hitting that problem also...
I tested it also with the same toolchain you are using (gcc 7.3.0-1 Debian), but that didn't made any difference.I don't think that issue is related with the Hardware, but to be on the save
side: Which Hardware version of the BPI-M3 do you have? I have version 1.2.
Quoted 9 lines
Quoted 1 line
Can someone else can confirm this bug?
So I might have remembered wrong, as I just realized I have your patch in my a83t branches. I don't hit this on the A80, which also has the AC100, but doesn't use assigned-clock-rates in the device tree.
Could you try rolling back to 4.15 and see if you still hit it?
Clean 4.15, no patches, just cloned before building: No issues.
Quoted 160 lines
Quoted 152 lines
Quoted 144 lines
Quoted 121 lines
Quoted 87 lines
Quoted 73 lines
You can find the shortend trace below:
Unable to handle kernel NULL pointer dereference at virtual address 00000000 pgd = (ptrval) [00000000] *pgd=00000000 Internal error: Oops: 5 [#1] SMP ARM Modules linked in: CPU: 0 PID: 49 Comm: kworker/0:1 Not tainted 4.15.0-10190-gb89e32ccd1be #2 Hardware name: Allwinner sun8i Family Workqueue: events deferred_probe_work_func PC is at clk_hw_get_rate+0x0/0x34 LR is at ac100_clkout_determine_rate+0x48/0x19c[ ... ]
(
clk_hw_get_rate) from (ac100_clkout_determine_rate+0x48/0x19c) (ac100_clkout_determine_rate) from (clk_core_set_rate_nolock+0x3c/0x1a0) (clk_core_set_rate_nolock) from (clk_set_rate+0x30/0x88) (clk_set_rate) from (of_clk_set_defaults+0x200/0x364) (of_clk_set_defaults) from (platform_drv_probe+0x18/0xb0)To fix that bug, we first check if the return of the
clk_hw_get_parent_by_indexis non zero. If it is zero we skip that clock parent.The BUG report could be found here: https://lkml.org/lkml/2018/2/10/198
Fixes 04940631b8d2 ("rtc: ac100: Add clk output support")Signed-off-by Philipp Rossak <[email protected]>---
Changes in v3:
* add information when the bug appeared
* make the comment more clear
Changes in v2:
* add tag Fixes: ... to commit message
* add comment to if statement why we are doing this check
drivers/rtc/rtc-ac100.c| 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-)drivers/rtc/rtc-ac100.c +0
-0@@ -183,7 +183,24 @@ static int ac100_clkout_determine_rate(struct
clk_hw
*hw,
for (i = 0; i <num_parents; i++) {
structclk_hw*parent = clk_hw_get_parent_by_index(hw,
i);drivers/rtc/rtc-ac100.c +4
-1-unsigned long tmp, prate =clk_hw_get_rate (parent); +unsigned long tmp, prate; + +/* +* The clock has two parents, one is a fixed clock which
is
+ * internally registered by the ac100 driver. The other
parentdrivers/rtc/rtc-ac100.c +2
-0+* is a clock from the codec side of the chip, which we +* properly declare and reference in the devicetree and is
drivers/rtc/rtc-ac100.c +2
-0+* not implemented in any driver right now. +* If the clock core looks for the parent of that second
drivers/rtc/rtc-ac100.c
Quoted 7 lines
drivers/rtc/rtc-ac100.c +7
-0+* missing clock, it can't one that is registered and +* returns NULL. +* Thus we need to check if the parent exists before +* we get the parent rate. +*/ +if (!parent) +continue ;I'm sorry, but I still don't get it. When you register that clock, you will give it two parents. Why would that change during the life of the clock?
Quoted 4 lines
This really looks like a workaround rather than an actual fix.
Maxime
I agree this is more a workaround!
A proper solution/fix would be to define the devicetree correct like this:arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts +8
-1@@ -175,11 +175,18 @@ compatible = "x-powers,ac100-rtc"; interrupt-parent = <&r_intc>; interrupts = <0 IRQ_TYPE_LEVEL_LOW>; - clocks = <&ac100_codec>; + clocks = <&ac100_rtc_32k>; #clock-cells = <1>; clock-output-names = "cko1_rtc", "cko2_rtc", "cko3_rtc"; + + ac100_rtc_32k: rtc-32k-oscillator { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <32768>; + clock-output-names = "ac100-rtc-32k"; + }; }; }; };Quoted 1 line
What do you think about that solution?
That's not quite right either. As I mentioned before, the RTC block has two clock inputs, one 4MHz signal from the codec block, and one 32.768 kHz signal from an external crystal. The original device tree binding describes the first one, and the 32.768 kHz clock was registered by the RTC driver internally.
If you're going to add the crystal clock, you still need to keep the codec one. Note that this does not fix what Maxime is asking you. I've already provided an explanation:
The clock core allows registering clocks with not-yet-existing clock parents. Parents are matches by string names. If no clock by that name is registered yet, the clock core simply orphans the new clock if the unregistered parent is its current parent or simply ignores that parent if its not the current parent. This is entirely valid and is what we are counting on here, as we haven't implemented the codec-side driver.
Quoted 7 lines
Note we also sort of depend on this behavior for sunxi-ng clocks, where in some cases we get circular dependencies between clock blocks.
BTW, since this is mostly related to the clk subsystem, please CC the clk maintainers as well.
Quoted 7 lines
Ok, I added also now the clk mailing list and maintainers. As reference this here is the boot log [1].
Im not sure if this in relation to this bug:
But, already with the 4.15 kernel I get from time to time some oops during runtime on the a31s (bpi-m2) and all are clock related and causes a crash of the system.
Quoted 9 lines
Are you referring to the A31
CLK_OUTbug? That is an entirely separate bug in the A31/A31s CCU driver. The bug was there from the beginning, but did not cause a crash as the incompatible data structures had lined up in a way that the embedded data structure that was used by the incorrect clk ops was actually compatible. This changed with 4.16-rc1, as a new field was added to some of the clock types, breaking the alignment. I made some changes but have yet to write the commit log for it. Also I don't have any A31/A31s boards to test it on right now. I can send you the patch later.
No I'm talking about an other bug. But, right now I'm not sure if this is related to the CLK_OUT bug. My first plan was to wait until you submitted the CLK_OUT patch, to clarify that. But I thought it would be good to mention.
That one I'm talking about is very strange.
So you can run your hardware for hours and nothing happens. Then when it appears the system crashs and reliable booting afterwards is not possible (for a few minutes). But that is not related to the thermal behavior of the soc. I'm still trying to find out under which conditions it crashes...
I also ran some test with cpuburn and cpufreq-ljt-stress-test but everything looks good, related to those tests.
Philipp
Quoted 39 lines · Chen-Yu Tsai
Quoted 26 lines
arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts +8
-1@@ -175,11 +175,18 @@ compatible = "x-powers,ac100-rtc"; interrupt-parent = <&r_intc>; interrupts = <0 IRQ_TYPE_LEVEL_LOW>; - clocks = <&ac100_codec>; + clocks = <&ac100_rtc_32k>; #clock-cells = <1>; clock-output-names = "cko1_rtc", "cko2_rtc", "cko3_rtc"; + + ac100_rtc_32k: rtc-32k-oscillator { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <32768>; + clock-output-names = "ac100-rtc-32k"; + }; }; }; };Quoted 1 line
What do you think about that solution?
That's not quite right either. As I mentioned before, the RTC block has two clock inputs, one 4MHz signal from the codec block, and one 32.768 kHz signal from an external crystal. The original device tree binding describes the first one, and the 32.768 kHz clock was registered by the RTC driver internally.
If you're going to add the crystal clock, you still need to keep the codec one. Note that this does not fix what Maxime is asking you. I've already provided an explanation:
Quoted 8 lines
The clock core allows registering clocks with not-yet-existing clock parents. Parents are matches by string names. If no clock by that name is registered yet, the clock core simply orphans the new clock if the unregistered parent is its current parent or simply ignores that parent if its not the current parent. This is entirely valid and is what we are counting on here, as we haven't implemented the codec-side driver.
So, we end up in a situation where clk_hw_get_num_parents returns the amount of clocks we can be parented to (orphans or not), but clk_hw_get_parent_by_index will not return the orphan clocks?
That's pretty bad :/
Is there a way to test before registering that all our parents are actually there? clk_get?
Maxime
Signature · 3 lines
Maxime Ripard, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering http://bootlin.com
Quoted 49 lines · Maxime Ripard
Quoted 39 lines · Chen-Yu Tsai
Quoted 26 lines
arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts +8
-1@@ -175,11 +175,18 @@ compatible = "x-powers,ac100-rtc"; interrupt-parent = <&r_intc>; interrupts = <0 IRQ_TYPE_LEVEL_LOW>; - clocks = <&ac100_codec>; + clocks = <&ac100_rtc_32k>; #clock-cells = <1>; clock-output-names = "cko1_rtc", "cko2_rtc", "cko3_rtc"; + + ac100_rtc_32k: rtc-32k-oscillator { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <32768>; + clock-output-names = "ac100-rtc-32k"; + }; }; }; };Quoted 1 line
What do you think about that solution?
That's not quite right either. As I mentioned before, the RTC block has two clock inputs, one 4MHz signal from the codec block, and one 32.768 kHz signal from an external crystal. The original device tree binding describes the first one, and the 32.768 kHz clock was registered by the RTC driver internally.
If you're going to add the crystal clock, you still need to keep the codec one. Note that this does not fix what Maxime is asking you. I've already provided an explanation:
Quoted 8 lines
The clock core allows registering clocks with not-yet-existing clock parents. Parents are matches by string names. If no clock by that name is registered yet, the clock core simply orphans the new clock if the unregistered parent is its current parent or simply ignores that parent if its not the current parent. This is entirely valid and is what we are counting on here, as we haven't implemented the codec-side driver.
Quoted 3 lines
So, we end up in a situation where
clk_hw_get_num_parentsreturns the amount of clocks we can be parented to (orphans or not), butclk_hw_get_parent_by_indexwill not return the orphan clocks?
There is no placeholder for missing parents, unlike the regulator subsystem that has a dummy regulator for this purpose.
Quoted 1 line
That's pretty bad :/
Yeah. I didn't expect this to happen. But to be fair, I should have done the check on clk_hw_get_parent_by_index.
Quoted 2 lines
Is there a way to test before registering that all our parents are actually there?
clk_get?
That's probably the way to do it. However in the AC100 RTC case, I left it open to be missing on purpose, so we could use the RTC without waiting for the codec to be supported.
ChenYu
Quoted 63 lines · Chen-Yu Tsai
Quoted 49 lines · Maxime Ripard
Quoted 39 lines · Chen-Yu Tsai
Quoted 26 lines
arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts +8
-1@@ -175,11 +175,18 @@ compatible = "x-powers,ac100-rtc"; interrupt-parent = <&r_intc>; interrupts = <0 IRQ_TYPE_LEVEL_LOW>; - clocks = <&ac100_codec>; + clocks = <&ac100_rtc_32k>; #clock-cells = <1>; clock-output-names = "cko1_rtc", "cko2_rtc", "cko3_rtc"; + + ac100_rtc_32k: rtc-32k-oscillator { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <32768>; + clock-output-names = "ac100-rtc-32k"; + }; }; }; };Quoted 1 line
What do you think about that solution?
That's not quite right either. As I mentioned before, the RTC block has two clock inputs, one 4MHz signal from the codec block, and one 32.768 kHz signal from an external crystal. The original device tree binding describes the first one, and the 32.768 kHz clock was registered by the RTC driver internally.
If you're going to add the crystal clock, you still need to keep the codec one. Note that this does not fix what Maxime is asking you. I've already provided an explanation:
Quoted 8 lines
The clock core allows registering clocks with not-yet-existing clock parents. Parents are matches by string names. If no clock by that name is registered yet, the clock core simply orphans the new clock if the unregistered parent is its current parent or simply ignores that parent if its not the current parent. This is entirely valid and is what we are counting on here, as we haven't implemented the codec-side driver.
Quoted 3 lines
So, we end up in a situation where
clk_hw_get_num_parentsreturns the amount of clocks we can be parented to (orphans or not), butclk_hw_get_parent_by_indexwill not return the orphan clocks?There is no placeholder for missing parents, unlike the regulator subsystem that has a dummy regulator for this purpose.
Quoted 1 line
That's pretty bad :/
Yeah. I didn't expect this to happen. But to be fair, I should have done the check on
clk_hw_get_parent_by_index.
Quoted 9 lines
Quoted 2 lines
Is there a way to test before registering that all our parents are actually there?
clk_get?That's probably the way to do it. However in the AC100 RTC case, I left it open to be missing on purpose, so we could use the RTC without waiting for the codec to be supported.
ChenYu
So how should we proceed with this issue?
Should I send a new version with a fixed comment or should I implement the check in clk_get function?
For the second option I will need about 3 weeks to submit a proper patch since I have the next two weeks some other stuff to do. If a proper fix is required earlier, it might be better if someone else is taking care about a fix.
Philipp
Quoted 76 lines · Philipp Rossak
Quoted 63 lines · Chen-Yu Tsai
Quoted 49 lines · Maxime Ripard
Quoted 39 lines · Chen-Yu Tsai
Quoted 26 lines
arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts +8
-1@@ -175,11 +175,18 @@ compatible = "x-powers,ac100-rtc"; interrupt-parent = <&r_intc>; interrupts = <0 IRQ_TYPE_LEVEL_LOW>; - clocks = <&ac100_codec>; + clocks = <&ac100_rtc_32k>; #clock-cells = <1>; clock-output-names = "cko1_rtc", "cko2_rtc", "cko3_rtc"; + + ac100_rtc_32k: rtc-32k-oscillator { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <32768>; + clock-output-names = "ac100-rtc-32k"; + }; }; }; };Quoted 1 line
What do you think about that solution?
That's not quite right either. As I mentioned before, the RTC block has two clock inputs, one 4MHz signal from the codec block, and one 32.768 kHz signal from an external crystal. The original device tree binding describes the first one, and the 32.768 kHz clock was registered by the RTC driver internally.
If you're going to add the crystal clock, you still need to keep the codec one. Note that this does not fix what Maxime is asking you. I've already provided an explanation:
Quoted 8 lines
The clock core allows registering clocks with not-yet-existing clock parents. Parents are matches by string names. If no clock by that name is registered yet, the clock core simply orphans the new clock if the unregistered parent is its current parent or simply ignores that parent if its not the current parent. This is entirely valid and is what we are counting on here, as we haven't implemented the codec-side driver.
Quoted 3 lines
So, we end up in a situation where
clk_hw_get_num_parentsreturns the amount of clocks we can be parented to (orphans or not), butclk_hw_get_parent_by_indexwill not return the orphan clocks?There is no placeholder for missing parents, unlike the regulator subsystem that has a dummy regulator for this purpose.
Quoted 1 line
That's pretty bad :/
Yeah. I didn't expect this to happen. But to be fair, I should have done the check on
clk_hw_get_parent_by_index.Quoted 9 lines
Quoted 2 lines
Is there a way to test before registering that all our parents are actually there?
clk_get?That's probably the way to do it. However in the AC100 RTC case, I left it open to be missing on purpose, so we could use the RTC without waiting for the codec to be supported.
ChenYu
So how should we proceed with this issue?
Quoted 7 lines
Should I send a new version with a fixed comment or should I implement the check in
clk_getfunction?For the second option I will need about 3 weeks to submit a proper patch since I have the next two weeks some other stuff to do.
If a proper fix is required earlier, it might be better if someone else is taking care about a fix.
A better comment will do.
Maxime
Signature · 3 lines
Maxime Ripard, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com