Re: [PATCH 2/2] clk: qcom: dispcc-sdm845: park disp_cc_mdss_mdp_clk_src

[Stephen Boyd <sboyd@kernel.org>]

Quoting Dmitry Baryshkov (2021-12-15 19:34:11)
> On Thu, 16 Dec 2021 at 04:38, Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Dmitry Baryshkov (2021-12-15 14:17:40)
> > > On 09/12/2021 21:40, Bjorn Andersson wrote:
> > > > On Tue 07 Dec 18:22 PST 2021, Dmitry Baryshkov wrote:
> > > >
> > > >> 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 <dmitry.baryshkov@linaro.org>
> > > >> ---
> > > >>   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.
> > >
> > > >
> > > > 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.

> 
> > > - 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?

> 
> >
> > 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.