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

[Alper Nebi Yasak]

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