Re: [linux-sunxi] [PATCH v2] rtc: ac100: Fix ac100 determine rate bug
From: Philipp Rossak
Date: Tue Feb 13 2018 - 10:23:09 EST
On 13.02.2018 14:44, Chen-Yu Tsai wrote:
On Tue, Feb 13, 2018 at 9:32 PM, Maxime Ripard
<maxime.ripard@xxxxxxxxxxx> wrote:
On Tue, Feb 13, 2018 at 01:14:14PM +0100, Philipp Rossak wrote:
This patch fixes a bug, that prevents the Allwinner A83T and the A80
from a successful boot. You can find the shortend trace below:
Since when is it there?
The bug is there since v4.16-rc1 and appeared after the clk branch was
merged.
^^ Should I add this info also in the commit message?
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
Fixes: 04940631b8d2 ("rtc: ac100: Add clk output support")
Should it be sent to stable?
Signed-off-by: Philipp Rossak <embed3d@xxxxxxxxx>
---
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 | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/rtc/rtc-ac100.c b/drivers/rtc/rtc-ac100.c
index 8ff9dc3fe5bf..ba73201d8cc1 100644
--- a/drivers/rtc/rtc-ac100.c
+++ b/drivers/rtc/rtc-ac100.c
@@ -183,7 +183,17 @@ 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;
+
+ /*
+ * We purposefully left open the possibility to use the clock
+ * from the codec side but it is not implemented right now.
+ * Thus we need to check if the parent exists.
+ */
+ if (!parent)
+ continue;
+
+ prate = clk_hw_get_rate(parent);
clk_hw_get_num_parents should return the exact number of parents,
which is going to be 1 if you only have one parent, like all DTS seems
to have.
If not, then it should be explained in the comment and / or fixed
properly.
The clock has two parents. One is a fixed clock internally registered
by the driver. This is actually an external crystal, and we should
probably add a device node and the works for it. The other parent
is a clock from the codec side, which we properly declare and
reference in the device tree. This clock, though defined, is not
implemented in any driver (because we don't have any ATM).
This second missing clock is what's causing issues here. The clk core
looks for the parent by name, can't find one that is registered, and
returns NULL.
I guess the comment above is still not clear enough?
I can get more detailed in the comment. I thought about this:
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.
Is that ok for you?
Philipp
ChenYu
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com