On Wed 15 Dec 14:17 PST 2021, Dmitry Baryshkov wrote: > 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. > Do you see a particular need to add a specific clock? Can't we just rely on clk_ignore_unused as a global indicator for this as well? > > > > 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. > There's a case here that we don't handle today; say you have two clocks A and B, which are both parented by the same RCG. Some driver enables clock A for a while and then disables it, at this point there's nothing indicating that some piece of hardware (with a not yet probed driver) is using clock B and that it must not be turned off until said driver has had a chance to "park" the hardware or cast its vote. > 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. > This is a good goal. But I don't think we should do this solely by cutting the clocks as the clock driver probes. I do think we need to allow the various drivers to turn off the hardware nicely before cutting any remaining clocks. > 3) Gracious handover of display/framebuffer from bootloader to the Linux > kernel. > Right, this is a stretch goal for now. > 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? > I am somewhat surprised that we haven't yet seen problems as we try out the approach of just cutting clocks without first shutting down the client driver. It might work for display, but I would not be surprised if cutting clocks like this will cause problem for the clocks that feed some external entity and then comes back in (e.g. PHY clocks). Also not that while this might work for dispcc, we should ensure to park all clocks that are flagged enable_safe_config downstream before we turn off their parents. In SM8350 GCC there are 44 of these, several of them you won't be able to just turn off - e.g. there's the UART. > 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. > The idea that Qualcomm (and Google) builds on for this is to rely on sync_state in the clock drivers and essentially 1) ensure that clocks aren't turned off until that point 2) clean up unused clocks when all known clients have probed. The problem with sync_state is that clocks resolved by their global name is not known beforehand, so sync_state will fire prematurely. > 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. > That's fine for clocks where you can test that, but we have several clocks where you can't read the state. One such set of clocks are clk-smd-rpm, which we can't query and we may not turn off until all the client driver have probed - as they provide e.g. clocking for the busses. > - 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. > > - 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. > This problem is not limited to the display driver, so adding a new clk_get() won't scale. > - If the driver decides for some reason to abandon the device for some > reason (because of the probe() failure or because of the remove() callback > being called) it will disable all clocks as expected, effectively parking > them (but not marking them as inherited). This way next driver probe() > attempt will start from the scratch, without inherited state. > So how do you handle the case when the display driver probe defers? I think you're trying to reinvent the sync_state callback. > But as this is a complex solution and will take several iterations, I > suggest teaching dispcc to park clocks at boot. > I agree, but it seems that we need to be more correct in order to support the newer platforms (SM8350/8450 etc)... Regards, Bjorn > > [1] https://lore.kernel.org/linux-arm-msm/20190630150230.7878-1-robdclark@xxxxxxxxx/ > > -- > With best wishes > Dmitry
