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

[Simon Glass]

Hi Peter,

On Fri, 11 Mar 2022 at 20:32, Peter Geis <pgwipeout at gmail.com> wrote:
>
> 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

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