Quoting Dmitry Baryshkov (2021-12-15 19:34:11) > On Thu, 16 Dec 2021 at 04:38, Stephen Boyd <sboyd@xxxxxxxxxx> 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@xxxxxxxxxx> > > > >> --- > > > >> 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.
