[PATCH v1 01/11] clk: rockchip: rk3568: fix reset handler

[Peter Geis]

On Fri, Mar 11, 2022 at 9:25 PM Simon Glass <sjg at chromium.org> wrote:
>
> Hi Peter,
>
> On Mon, 21 Feb 2022 at 18:31, Peter Geis <pgwipeout at 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 at 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_rk3399.c#L1431
https://elixir.bootlin.com/u-boot/latest/source/drivers/clk/rockchip/clk_rk3368.c#L625
https://elixir.bootlin.com/u-boot/latest/source/drivers/clk/rockchip/clk_rk3328.c#L827

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.

>
> 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