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. > > - 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. 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). Once we do that we'll know if we can hand out an orphan clk because it may one day be reparented via clk_set_parent() or clk_set_rate() vs. the case where we shouldn't hand it out via clk_get() because we'll never be able to parent it because the parent(s) doesn't exist.
