Re: [PATCH] clk: clk-hi3670: Add CLK_IGNORE_UNUSED flag

[Mauro Carvalho Chehab]

Em Sat, 15 Aug 2020 19:33:31 -0700
Stephen Boyd <sboyd@xxxxxxxxxx> escreveu:

> Please send patches To: somebody. Sending them to nobody causes my MUA
> pain.

Ok. Should I send it to you or to someone else?
> 
> Quoting Mauro Carvalho Chehab (2020-08-14 07:16:20)
> > There are several clocks that are required for Kirin 970 to
> > work. Without them, the system hangs. However, most of
> > the clocks defined at clk-hi3670 aren't specified on its
> > device tree, nor at Hikey 970 one.
> > 
> > A few of them are defined at the Linaro's official tree
> > for Hikey 970, but, even there, distros use
> > 
> >         clk_ignore_unused=true
> > 
> > as a boot option.
> > 
> > So, instead, let's modify the driver to use CLK_IGNORE_UNUSED
> > flags, removing the need for this boot parameter.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx>
> > ---
> >  drivers/clk/hisilicon/clk-hi3670.c | 731 +++++++++++++++++------------
> >  1 file changed, 425 insertions(+), 306 deletions(-)  
> 
> This is very many. Are all of these clks actually enabled out of boot
> and are getting turned off at late init?

That's a very good question. Unfortunately, I don't know.

There are some documentation at:
	https://www.96boards.org/documentation/consumer/hikey/hikey970/hardware-docs/

Including schematics for HiKey 970, but it doesn't seem to show all
the clock lines, plus the clock names at the driver don't seem to match
what's there at the datasheet.

> Is there some set of clks that can be marked as CLK_IS_CRITICAL instead?

Maybe, but identifying those would require a huge amount of work. See,
this patch marks 306 clock lines with CLK_IGNORE_UNUSED:

	$ git grep CLK_IGNORE_UNUSED drivers/clk/hisilicon/clk-hi3670.c|wc -l
	306

At vanilla Kernel 5.8, there are 49 known clock lines:

	$ git grep -E 'HI\S+CLK' arch/arm64/boot/dts/hisilicon/*70*|wc -l
	49

As I'm porting several drivers in order to support DRM and hopefully USB,
this count should increase as drivers get merged.

At downstream 4.9 Kernel, there are 99 known clock lines:

	$ git grep -E 'KI\S+CLK' arch/arm64/boot/dts/hisilicon/*70*|wc -l
	99

In other words, there are still 207 lines that we have no clue about
them. What among those are critical or not is a very good question.


> The CLK_IGNORE_UNUSED flag shouldn't be used very much at all. Instead,
> drivers should be using the CLK_IS_CRITICAL flag. We have a lot of
> CLK_IGNORE_UNUSED in the kernel right now, but the hope is that we can
> get rid of this flag one day.

I see the point. Yet, I can't see any solution for that, except not letting 
PM to disable unused clocks on this chipset. See, the only way to use the
HiKey 970 board (which is the only one with DT bindings upstream for this
chipset) is to boot the Kernel with clk_ignore_unused=true.

Ok, if someone has enough time and some robot infrastructure that would
automatically be patching the driver, detect broken boots, and powering
down/up the device after each attempt, he could be disabling each one of 
the clock lines that are not specified at the DT, identifying the
critical ones.

Then, he may need to port more drivers, together with their DT bindings,
from the downstream tree.

That is a lot of work for, IMHO, not much gain.

Thanks,
Mauro