[PATCH 1/2] rockchip: rk3399: Re-init clocks in U-Boot proper

[Simon Glass]

Hi Alper,

On Sat, 24 Oct 2020 at 16:29, Alper Nebi Yasak <alpernebiyasak at gmail.com> wrote:
>
> On 24/10/2020 03:05, Simon Glass wrote:
> > Hi Alper,
> >
> > On Thu, 22 Oct 2020 at 14:37, Alper Nebi Yasak <alpernebiyasak at gmail.com> wrote:
> >>
> >> It's possible to chainload U-Boot proper from the vendor firmware in
> >> rk3399 chromebooks, but the way the vendor firmware sets up clocks is
> >> somehow different than what U-Boot expects. This causes the display to
> >> stay devoid of content even though vidconsole claims to work (with
> >> patches in process of being upstreamed).
> >>
> >> This is meant to be a rk3399 version of commit d3cb46aa8c41 ("rockchip:
> >> Init clocks again when chain-loading") which can detect the discrepancy,
> >> but this patch can not so it always re-inits.
> >>
> >> Signed-off-by: Alper Nebi Yasak <alpernebiyasak at gmail.com>
> >> ---
> >> The rk3288 version has rockchip_vop_set_clk in #ifndef CONFIG_BUILD_SPL,
> >> and checks if that setup is already done before that's called. I think I
> >> can do the #ifndef to avoid unnecessary re-inits for rk3399 as well, but
> >> the vop clocks are set differently for each chip so I don't know how
> >> correct that'd be.
> >>
> >> The pmucru setup is #ifndef CONFIG_BUILD_SPL on rk3399, so I think I can
> >> also check for that, but that's technically in a different driver and I
> >> don't know how best to do that.
> >>
> >>  drivers/clk/rockchip/clk_rk3399.c | 7 +------
> >>  1 file changed, 1 insertion(+), 6 deletions(-)
> >
> > Could we make this conditional on SPL not running? We could check that
> > by enabling CONFIG_HANDOFF, for example.
>
> Using CONFIG_HANDOFF in the probe function like this works on my side:
>
>     static int rk3399_clk_probe(struct udevice *dev)
>     {
>             struct rk3399_clk_priv *priv = dev_get_priv(dev);
>             bool init_clocks = false;
>
>     #if CONFIG_IS_ENABLED(OF_PLATDATA)
>             struct rk3399_clk_plat *plat = dev_get_platdata(dev);
>
>             priv->cru = map_sysmem(plat->dtd.reg[0], plat->dtd.reg[1]);
>     #endif
>
>     #if defined(CONFIG_SPL_BUILD)
>             init_clocks = true;
>     #elif CONFIG_IS_ENABLED(HANDOFF)
>             if (!(gd->spl_handoff))
>                     init_clocks = true;
>     #endif
>
>             if (init_clocks)
>                     rkclk_init(priv->cru);
>
>             return 0;
>     }
>
> The clk_rk3288 code also checks for GD_FLG_RELOC, adding that didn't
> look like it did anything good/bad for me. Should I add that too? I
> mean:
>
>     #elif CONFIG_IS_ENABLED(HANDOFF)
>             if (!(gd->flags & GD_FLG_RELOC)) {
>                     if (!(gd->spl_handoff))
>                             init_clocks = true;
>             }
>     #endif
>
> Also, HANDOFF depends on BLOBLIST (maybe it shouldn't?), so I added
> these values from chromebook_coral to my config, are they OK?
>
>     CONFIG_BLOBLIST=y
>     CONFIG_BLOBLIST_SIZE=0x30000
>     CONFIG_BLOBLIST_ADDR=0x100000

Yes that's right. The handoff struct is stored in the blblist. It
probably only needs to be small though.

Regards,
Simon