Hi Peter,
On Fri, 11 Mar 2022 at 20:32, Peter Geis pgwipeout@gmail.com wrote:
On Fri, Mar 11, 2022 at 9:25 PM Simon Glass sjg@chromium.org wrote:
Hi Peter,
On Mon, 21 Feb 2022 at 18:31, Peter Geis pgwipeout@gmail.com wrote:
The reset handler for rk3568 is missing its private data. This leads to an abort when a reset is triggered.
Add the missing dev_set_priv to the rk3568 clk driver.
Fixes: 4a262feba3a5 ("rockchip: rk3568: add clock driver")
Signed-off-by: Peter Geis pgwipeout@gmail.com
drivers/clk/rockchip/clk_rk3568.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/clk/rockchip/clk_rk3568.c b/drivers/clk/rockchip/clk_rk3568.c index d5e45e7602c7..c83ae22dc302 100644 --- a/drivers/clk/rockchip/clk_rk3568.c +++ b/drivers/clk/rockchip/clk_rk3568.c @@ -14,6 +14,7 @@ #include <asm/arch-rockchip/clock.h> #include <asm/arch-rockchip/hardware.h> #include <asm/io.h> +#include <dm/device-internal.h> #include <dm/lists.h> #include <dt-bindings/clock/rk3568-cru.h>
@@ -2934,6 +2935,7 @@ static int rk3568_clk_bind(struct udevice *dev) glb_srst_fst); priv->glb_srst_snd_value = offsetof(struct rk3568_cru, glb_srsr_snd);
dev_set_priv(sys_child, priv); }#if CONFIG_IS_ENABLED(RESET_ROCKCHIP)
2.25.1
You must not access priv data in the bind() handler as it doesn't exist yet. The malloc() in that function is incorrect.
Strange, this makes this driver match literally every other rockchip clock driver. A few examples: https://elixir.bootlin.com/u-boot/latest/source/drivers/clk/rockchip/clk_rk3... https://elixir.bootlin.com/u-boot/latest/source/drivers/clk/rockchip/clk_rk3... https://elixir.bootlin.com/u-boot/latest/source/drivers/clk/rockchip/clk_rk3...
Oops.
It's funny though, I thought this should reside completely within the clock driver, because the sysreset handler is touching private cru registers and I planned on moving it here. Then I found this in the other drivers and went with this method for consistency.
I think it is OK for the sysreset driver to touch the cru, if it needs to. We sometimes find we have multiple drivers for a single IP block, particular the 'core' ones.
You certainly must not set the priv data in there. See the lifecycle docs in driver model for how things are supposed to work.
You can use plat data, so could move glb_srst_fst_value and glb_srst_snd_value to struct rk3568_clk_plat, although oddly that struct only exists if OF_PLATDATA is used.
Quite confusing.
Regards, Simon