Hi Cédric,
On Tue, 7 Jan 2020 at 21:20, Cédric Le Goater clg@kaod.org wrote:
On 12/30/19 5:19 AM, Simon Glass wrote:
At present the clock driver reads its ofdata in the probe() method. This is not correct although it is often harmless.
However in this case it causes a problem, something like this:
- ast_get_scu() is called (from somewhere) to get the SCI address
- this probes the clock
- first sets up ofdata (which does nothing at present)
- DM marks clock device as active
- DM calls pinctrl
- pinctrl probes and calls ast_get_scu() in ast2500_pinctrl_probe()
- ast_get_scu() probes the clock, but sees it already marked as probed
- ast_get_scu() accesses the clock's private data, with scu as NULL
- DM calls clock probe function ast2500_clk_probe() which reads scu
By putting the read of scu into the correct method, scu is read as part of ofdata setup, and everything is OK.
Note: This problem did not matter until now since DM always probed all parents before reading a child's ofdata. The fact that pinctrl is a child of clock seems to trigger this strange bug.
Did you find it with QEMU ?
Yes that's right.
Signed-off-by: Simon Glass sjg@chromium.org
Reviewed-by: Cédric Le Goater clg@kaod.org
C.
drivers/clk/aspeed/clk_ast2500.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/aspeed/clk_ast2500.c b/drivers/clk/aspeed/clk_ast2500.c index 9249cf9cdf..b3a3f3d4dd 100644 --- a/drivers/clk/aspeed/clk_ast2500.c +++ b/drivers/clk/aspeed/clk_ast2500.c @@ -490,7 +490,7 @@ struct clk_ops ast2500_clk_ops = { .enable = ast2500_clk_enable, };
-static int ast2500_clk_probe(struct udevice *dev) +static int ast2500_clk_ofdata_to_platdata(struct udevice *dev) { struct ast2500_clk_priv *priv = dev_get_priv(dev);
@@ -525,5 +525,5 @@ U_BOOT_DRIVER(aspeed_ast2500_scu) = { .priv_auto_alloc_size = sizeof(struct ast2500_clk_priv), .ops = &ast2500_clk_ops, .bind = ast2500_clk_bind,
.probe = ast2500_clk_probe,
.ofdata_to_platdata = ast2500_clk_ofdata_to_platdata,};
Regards, Simon