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

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

[Alper Nebi Yasak]

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@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(-)

diff --git a/drivers/clk/rockchip/clk_rk3399.c b/drivers/clk/rockchip/clk_rk3399.c
index 1ea41f3c5b2e..dd629c26b8f0 100644
--- a/drivers/clk/rockchip/clk_rk3399.c
+++ b/drivers/clk/rockchip/clk_rk3399.c
@@ -50,10 +50,9 @@ struct pll_div {
 	.fbdiv = (u32)((u64)hz * _refdiv * _postdiv1 * _postdiv2 / OSC_HZ),\
 	.postdiv1 = _postdiv1, .postdiv2 = _postdiv2};
 
-#if defined(CONFIG_SPL_BUILD)
 static const struct pll_div gpll_init_cfg = PLL_DIVISORS(GPLL_HZ, 2, 2, 1);
 static const struct pll_div cpll_init_cfg = PLL_DIVISORS(CPLL_HZ, 1, 2, 2);
-#else
+#if !defined(CONFIG_SPL_BUILD)
 static const struct pll_div ppll_init_cfg = PLL_DIVISORS(PPLL_HZ, 2, 2, 1);
 #endif
 
@@ -1274,7 +1273,6 @@ static struct clk_ops rk3399_clk_ops = {
 	.disable = rk3399_clk_disable,
 };
 
-#ifdef CONFIG_SPL_BUILD
 static void rkclk_init(struct rockchip_cru *cru)
 {
 	u32 aclk_div;
@@ -1352,11 +1350,9 @@ static void rkclk_init(struct rockchip_cru *cru)
 		     hclk_div << HCLK_PERILP1_DIV_CON_SHIFT |
 		     HCLK_PERILP1_PLL_SEL_GPLL << HCLK_PERILP1_PLL_SEL_SHIFT);
 }
-#endif
 
 static int rk3399_clk_probe(struct udevice *dev)
 {
-#ifdef CONFIG_SPL_BUILD
 	struct rk3399_clk_priv *priv = dev_get_priv(dev);
 
 #if CONFIG_IS_ENABLED(OF_PLATDATA)
@@ -1365,7 +1361,6 @@ static int rk3399_clk_probe(struct udevice *dev)
 	priv->cru = map_sysmem(plat->dtd.reg[0], plat->dtd.reg[1]);
 #endif
 	rkclk_init(priv->cru);
-#endif
 	return 0;
 }
 
-- 
2.28.0

[PATCH 2/2] rockchip: gru: Allow setting up clocks in U-Boot proper

[Alper Nebi Yasak]

Commit fe974716326c ("rockchip: rk3288: Allow setting up clocks in
U-Boot proper") fixes some clock issues when chainloading U-Boot on
rk3288 chromebooks. Part of that change is still available in veyron's
board_early_init_r() function. Since chain-loading U-Boot proper from
vendor firmware is possible on gru boards as well, do the same thing for
them too.

Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
---

 board/google/gru/gru.c           | 23 +++++++++++++++++++++++
 configs/chromebook_bob_defconfig |  1 +
 2 files changed, 24 insertions(+)

diff --git a/board/google/gru/gru.c b/board/google/gru/gru.c
index 7dfbc3ac8676..441a1a376a9a 100644
--- a/board/google/gru/gru.c
+++ b/board/google/gru/gru.c
@@ -4,6 +4,7 @@
  */
 
 #include <common.h>
+#include <dm.h>
 #include <init.h>
 
 #ifdef CONFIG_SPL_BUILD
@@ -31,3 +32,25 @@ int board_early_init_f(void)
 	return 0;
 }
 #endif
+
+#ifndef CONFIG_SPL_BUILD
+int board_early_init_r(void)
+{
+	struct udevice *clk;
+	int ret;
+
+	/*
+	 * This init is done in SPL, but when chain-loading U-Boot SPL will
+	 * have been skipped. Allow the clock driver to check if it needs
+	 * setting up.
+	 */
+	ret = uclass_get_device_by_driver(UCLASS_CLK,
+					  DM_GET_DRIVER(clk_rk3399), &clk);
+	if (ret) {
+		debug("%s: CLK init failed: %d\n", __func__, ret);
+		return ret;
+	}
+
+	return 0;
+}
+#endif
diff --git a/configs/chromebook_bob_defconfig b/configs/chromebook_bob_defconfig
index 4608892fb567..23b5a1a904e1 100644
--- a/configs/chromebook_bob_defconfig
+++ b/configs/chromebook_bob_defconfig
@@ -19,6 +19,7 @@ CONFIG_DEBUG_UART=y
 CONFIG_DEFAULT_FDT_FILE="rockchip/rk3399-gru-bob.dtb"
 # CONFIG_DISPLAY_CPUINFO is not set
 CONFIG_DISPLAY_BOARDINFO_LATE=y
+CONFIG_BOARD_EARLY_INIT_R=y
 # CONFIG_SPL_RAW_IMAGE_SUPPORT is not set
 CONFIG_SPL_STACK_R=y
 CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x4000
-- 
2.28.0

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

[Simon Glass]

Hi Alper,

On Thu, 22 Oct 2020 at 14:37, Alper Nebi Yasak <alpernebiyasak@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@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.

Regards,
Simon

[PATCH 2/2] rockchip: gru: Allow setting up clocks in U-Boot proper

[Simon Glass]

Hi Alper,

On Thu, 22 Oct 2020 at 14:38, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> Commit fe974716326c ("rockchip: rk3288: Allow setting up clocks in
> U-Boot proper") fixes some clock issues when chainloading U-Boot on
> rk3288 chromebooks. Part of that change is still available in veyron's
> board_early_init_r() function. Since chain-loading U-Boot proper from
> vendor firmware is possible on gru boards as well, do the same thing for
> them too.
>
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> ---
>
>  board/google/gru/gru.c           | 23 +++++++++++++++++++++++
>  configs/chromebook_bob_defconfig |  1 +
>  2 files changed, 24 insertions(+)

Similar comment here.

Regards,
Simon

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

[PATCH 2/2] rockchip: gru: Allow setting up 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:38, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>>
>> Commit fe974716326c ("rockchip: rk3288: Allow setting up clocks in
>> U-Boot proper") fixes some clock issues when chainloading U-Boot on
>> rk3288 chromebooks. Part of that change is still available in veyron's
>> board_early_init_r() function. Since chain-loading U-Boot proper from
>> vendor firmware is possible on gru boards as well, do the same thing for
>> them too.
>>
>> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
>> ---
>>
>>  board/google/gru/gru.c           | 23 +++++++++++++++++++++++
>>  configs/chromebook_bob_defconfig |  1 +
>>  2 files changed, 24 insertions(+)
> 
> Similar comment here.

I can do the same HANDOFF checks here, but to me it looks like this
wouldn't need them if we're already checking things in the probe
function.

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

[PATCH 2/2] rockchip: gru: Allow setting up clocks in U-Boot proper

[Simon Glass]

Hi Alper,

On Sat, 24 Oct 2020 at 16:29, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> On 24/10/2020 03:05, Simon Glass wrote:
> > Hi Alper,
> >
> > On Thu, 22 Oct 2020 at 14:38, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
> >>
> >> Commit fe974716326c ("rockchip: rk3288: Allow setting up clocks in
> >> U-Boot proper") fixes some clock issues when chainloading U-Boot on
> >> rk3288 chromebooks. Part of that change is still available in veyron's
> >> board_early_init_r() function. Since chain-loading U-Boot proper from
> >> vendor firmware is possible on gru boards as well, do the same thing for
> >> them too.
> >>
> >> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> >> ---
> >>
> >>  board/google/gru/gru.c           | 23 +++++++++++++++++++++++
> >>  configs/chromebook_bob_defconfig |  1 +
> >>  2 files changed, 24 insertions(+)
> >
> > Similar comment here.
>
> I can do the same HANDOFF checks here, but to me it looks like this
> wouldn't need them if we're already checking things in the probe
> function.

Yes I thnk so.

Regards,
Simon