Re: [PATCHv2 2/2] clk: meson: g12a: set cpub_clk flags to CLK_IS_CRITICAL

[Kevin Hilman]

Anand Moon <linux.amoon@xxxxxxxxx> writes:

> On Odroid n2, cpub_clk is not geting enable, which lead the stalling
> at booting of the device,

First, how is the CPU_B clk related to the SD card issue described in
the cover letter?  I think this patch is attempting to fix something
unrelated to the SD card.  Please separate from this series (or describe
in detail how it's related to the SD card booting.)

Also, we're missing lots of details here to be able to help.  Are you
using the u-boot from hardkernel?  your own?  something else?  What's
the version?

Can you share logs (including u-boot logs) showing how your kernel is
booting and full kernel boot log (including the stalls.)

> updating flags to CLK_IS_CRITICAL which help enable all the parent for
> cpub_clk.

With current mainline, I've tested DVFS using CPUfreq on both clusters
on odroid-n2, and both clusters are booting, so I don't understand the
need for this patch.  

It's not related to your problem (I don't think) but for the regulators
used by each cluster, the PWM driver is needed, and there's a bug/race
in the probing of the PWM regulators used for CPU_B.  If you make the
PWM regulators, built-in this problem goes away for CPUfreq.

Just for kicks, can you build your kernel with CONFIG_PWM_MESON=y
(currently defaults to =n) and see if you have any better results with
booting.

And FYI, any use of CLK_IS_CRITICAL will be very highly scrutinized.
You will need detailed justification for adding this flag since it most
often is just masking some other bug.

Kevin

> Fixes: ffae8475b90c (clk: meson: g12a: add notifiers to handle cpu clock change);
> Cc: Martin Blumenstingl <martin.blumenstingl@xxxxxxxxxxxxxx>
> Cc: Jerome Brunet <jbrunet@xxxxxxxxxxxx>
> Cc: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
> Suggested-by: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
> Signed-off-by: Anand Moon <linux.amoon@xxxxxxxxx>
> ---
> Previous changes
> 	fix the commit $subject and $message as previously I was
>         wrong on the my findings.
>         Added the Fixed tags to the commit.
>
> Following Neil's suggestion, I have prepared this patch.
> https://patchwork.kernel.org/patch/11177441/#22964889
> ---
>  drivers/clk/meson/g12a.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c
> index d2760a021301..7237d08b4112 100644
> --- a/drivers/clk/meson/g12a.c
> +++ b/drivers/clk/meson/g12a.c
> @@ -681,7 +681,7 @@ static struct clk_regmap g12b_cpub_clk = {
>  			&g12a_sys_pll.hw
>  		},
>  		.num_parents = 2,
> -		.flags = CLK_SET_RATE_PARENT,
> +		.flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
>  	},
>  };
>  
> -- 
> 2.25.1