Message-ID: <ebc257025ebd641e624ef506ea09c800.sboyd@kernel.org>
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <20221227204528.1899863-1-abel.vesa@linaro.org>
References: <20221227204528.1899863-1-abel.vesa@linaro.org>
Subject: Re: [PATCH v3 1/2] clk: Add generic sync_state callback for disabling unused clocks
From:   Stephen Boyd <sboyd@kernel.org>
Cc:     linux-clk@vger.kernel.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-arm-msm@vger.kernel.org, mka@chromium.org
To:     Abel Vesa <abel.vesa@linaro.org>, Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Mike Turquette <mturquette@baylibre.com>
Date:   Fri, 17 Feb 2023 21:38:22 -0800
User-Agent: alot/0.10
Precedence: bulk

Quoting Abel Vesa (2022-12-27 12:45:27)
> There are unused clocks that need to remain untouched by clk_disable_unus=
ed,
> and most likely could be disabled later on sync_state. So provide a gener=
ic
> sync_state callback for the clock providers that register such clocks.
> Then, use the same mechanism as clk_disable_unused from that generic
> callback, but pass the device to make sure only the clocks belonging to
> the current clock provider get disabled, if unused. Also, during the
> default clk_disable_unused, if the driver that registered the clock has
> the generic clk_sync_state_disable_unused callback set for sync_state,
> skip disabling its clocks.

How does that avoid disabling clks randomly in the clk tree? I'm
concerned about disabling an unused clk in the middle of the tree
because it doesn't have a driver using sync state, while the clk is the
parent of an unused clk that is backed by sync state.

   clk A -->  clk B=20

clk A: No sync state
clk B: sync state

clk B is left on by the bootloader. __clk_disable_unused(NULL) is called
from late init. Imagine clk A is the root of the tree.

	clk_disable_unused_subtree(clk_core A)
	  clk_disable_unused_subtree(clk_core B)
	    if (from_sync_state && core->dev !=3D dev)
	      return;
	  ...
	  clk core A->ops->disable()

clk core B is off now?

Also sync_state seems broken right now. I saw mka mentioned that if you
have a device node enabled in your DT but never enable a driver for it
in the kernel we'll never get sync_state called. This is another
problem, but it concerns me that sync_state would make the unused clk
disabling happen at some random time or not at all.

Can the problem be approached more directly? If this is about fixing
continuous splash screen, then I wonder why we can't list out the clks
that we know are enabled by the bootloader in some new DT binding, e.g.:

	clock-controller {
		#clock-cells =3D <1>;
		boot-handoff-clocks =3D <&consumer_device "clock cells for this clk provi=
der">;
	};

Then mark those as "critical/don't turn off" all the way up the clk tree
when the clk driver probes by essentially incrementing the
prepare/enable count but not actually touching the hardware, and when
the clks are acquired by clk_get() for that device that's using them
from boot we make the first clk_prepare_enable() do nothing and not
increment the count at all. We can probably stick some flag into the
'struct clk' for this when we create the handle in clk_get() so that the
prepare and enable functions can special case and skip over.

The sync_state hook operates on a driver level, which is too large when
you consider that a single clk driver may register hundreds of clks that
are not related. We want to target a solution at the clk level so that
any damage from keeping on all the clks provided by the controller is
limited to just the drivers that aren't probed and ready to handle their
clks. If sync_state could be called whenever a clk consumer consumes a
clk it may work? Technically we already have that by the clk_hw_provider
function but there isn't enough information being passed there, like the
getting device.

> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 842e72a5348f..cf1adfeaf257 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -720,6 +720,7 @@ struct clk *clk_register_divider_table(struct device =
*dev, const char *name,
>                 void __iomem *reg, u8 shift, u8 width,
>                 u8 clk_divider_flags, const struct clk_div_table *table,
>                 spinlock_t *lock);
> +void clk_sync_state_disable_unused(struct device *dev);

This is a weird place to put this. Why not in the helper functions
section?

>  /**
>   * clk_register_divider - register a divider clock with the clock framew=
ork
>   * @dev: device registering this clock