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.
- 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.
- 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.
But as this is a complex solution and will take several iterations, I
suggest teaching dispcc to park clocks at boot.
[1]
https://lore.kernel.org/linux-arm-msm/20190630150230.7878-1-robdclark@xxxxxxxxx/
--
With best wishes
Dmitry
