Quoting Dmitry Baryshkov (2021-12-15 19:34:11)

On Thu, 16 Dec 2021 at 04:38, Stephen Boyd [off-list ref] wrote:

quoted

Quoting Dmitry Baryshkov (2021-12-15 14:17:40)

quoted

On 09/12/2021 21:40, Bjorn Andersson wrote:

quoted

On Tue 07 Dec 18:22 PST 2021, Dmitry Baryshkov wrote:

quoted

To stop disp_cc_mdss_mdp_clk_src from getting stuck during boot if it
was enabled by the bootloader, part it to the TCXO clock source.

Signed-off-by: Dmitry Baryshkov <redacted>
---
  drivers/clk/qcom/dispcc-sdm845.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/drivers/clk/qcom/dispcc-sdm845.c b/drivers/clk/qcom/dispcc-sdm845.c
index 735adfefc379..f2afbba7bc72 100644
--- a/drivers/clk/qcom/dispcc-sdm845.c
+++ b/drivers/clk/qcom/dispcc-sdm845.c

@@ -858,6 +858,9 @@ static int disp_cc_sdm845_probe(struct platform_device *pdev)

     clk_fabia_pll_configure(&disp_cc_pll0, regmap, &disp_cc_pll0_config);

+    /* Park disp_cc_mdss_mdp_clk_src */
+    clk_rcg2_park_safely(regmap, 0x2088, 0);

Today booting the system with "clk_ignore_unused" will give you a
working efifb up until the point where the display driver kicks in and
reinitializes the hardware state - which during development might be
indefinite.

During development one can introduce a dispcc parameter. Maybe we should
add qcom-common parameter telling dispcc drivers to skip parking these
clocks.

quoted

If we blindly cut the mdp_clk_src here that will no longer be possible.

I think we have several separate tasks here:

1) Support developing code. This is what you have in mind with EFIFB +
clk_ignore_unused.

2) Get display to work stable and rock solid. This can include
completely tearing down the display pipeline for the sake of getting
MDP/MDSS/DSI to work with as few hacks as possible.

3) Gracious handover of display/framebuffer from bootloader to the Linux
kernel.

For the task #1, you can hack the dispcc as you wish or set any
additional parameters, as you are already passing clk_ignore_unused.
This will all end up as #1 transitions to #2.

I was targetting task#2. Disable everything to let dpu/dsi/dp start from
the scratch. If I understand correctly, this approach would also help
you with your boot-clock-too-high-for-the-minimum-opp issue. Is my
assumption correct?

For the task #3 we need collaboration between dispcc, clock core and
dpu/dsi drivers. Just marking the clocks for the clk_disable_unused() is
the least of the problems that we have here. I think [1] is a bit closer
to what I'd expect.

I have a similar but slightly different idea of how this can be made to
work. I'd do the following (excuse me for the hand waving, no code at hand):

- Add clk_ops->inherit_state callback, which can check if the clock is
enabled already or not. If it is, set the enable_count to 1, set special
CLOCK_INHERITED flag, read back the state, etc.

- Make of_clk_set_defaults() ignore clocks with CLOCK_INHERITED flag.
Maybe it should return special status telling that some of the clocks
were not updated.

This sounds an awful lot like the CLK_HANDOFF flag that never
materialized. We know we have a problem where the enable state of a clk
isn't understood at registration time (although we do know the frequency
of the clk). So far it's been put largely on clk providers to figure out
that their clk is enabled and avoid doing something if it is. But that's
run into problems where clk flags that want us to not do something if
the clk is enabled fail to detect this, see CLK_SET_RATE_GATE for
example. This should be fixed; patches welcome.

Within the clk framework we don't really want to care about a clk already
being enabled and keeping track of that via the enable_count. Trying to
figure out when to "hand that off" is complex, and what exactly is the
point to it? Drivers still need to call clk_enable to enable the clk, so
all that really matters is that we know the clk is on at boot and to
respect the clk flags.

It's a pity. Tracking the pre-enabled clocks status would keep the
clock running till the driver is actually able to pick it up.

I have no problem determining the prepare/enable state at clk
registration time and then using that to make the clk flags work
properly and to skip calling down into the prepare and enable clk_ops.
It needs to be disjoint from the counts though so that the possibility
of handing off the count is removed.

quoted

quoted

- Add clk_get_inherit() call, which would drop the CLOCK_INHERITED flag
and return previous flag state to calling driver. The driver now assumes
ownership of this clock with the enable_count of 1. This way the driver
can adjust itself to the current clock state (e.g. drop the frequency,
disable the clock and then call of_clk_set_defaults() again to
reparent/reclock clocks as necessary, etc). If the parent chain is not
fully available, clk_get_inherit must return an error for INHERITED
clocks, so that the driver will not cause reparenting of the orphaned
clocks.

Please god no more clk_get() APIs! The driver shouldn't care that the
clk is already enabled when clk_get() returns. The driver must call
clk_enable() if it wants the clk to be enabled.

What about clk_get returning the clock and clk_enable transferring the
ownership?

No? Why can't the caller of clk_get() call clk_enable()?

I see that Michael Turquette had more or less the same ideas in 2015-2016.

Yes

It would ensure that the clock chain stays on till msm takes over the
efifb/splash/etc.

Who is turning off the clk? Some driver or the disable unused code?

quoted

Buried in here is the question of if we should allow clk_get() to
succeed if the clk is an orphan. I recall that rockchip had some problem
if we didn't allow orphans to be handed out but it's been years and I've
forgotten the details. But from a purely high-level we should definitely not
hand out orphan clks via clk_get() because the clk isn't operable
outside of clk_set_rate() or clk_set_parent().

And there's more work to do here first by getting rid of the .get_parent
clk_op and having it return a clk_hw pointer (see my two or three year
old clk_get_hw series).

Could you please point me to it?

https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/log/?h=clk-parent-rewrite

My god it's been three years.