| Message ID | 20211208022210.1300773-3-dmitry.baryshkov@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | clk: qcom: fix disp_cc_mdss_mdp_clk_src issues on sdm845 | expand |
On Wed, 8 Dec 2021 at 03:22, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> 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); Could the hardcoded number be replaced with disp_cc_mdss_mdp_clk_src.cmd_rcgr just to make this easier to read? Maybe the comment isn't needed with this change. > + > /* Enable hardware clock gating for DSI and MDP clocks */ > regmap_update_bits(regmap, 0x8000, 0x7f0, 0x7f0); > Reviewed-by: Robert Foss <robert.foss@linaro.org>
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. If we blindly cut the mdp_clk_src here that will no longer be possible. Regards, Bjorn > + > /* Enable hardware clock gating for DSI and MDP clocks */ > regmap_update_bits(regmap, 0x8000, 0x7f0, 0x7f0); > > -- > 2.33.0 >
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. > > If we blindly cut the mdp_clk_src here that will no longer be possible. Yep. There is an opposite issue. I was getting the rcg2 stuck messages _before_ the clk_disable_unused kicks in. So we definitely need something more complex than both our proposals. Not to mention that Steev tested that this does not fix the issue on C630. > > Regards, > Bjorn > >> + >> /* Enable hardware clock gating for DSI and MDP clocks */ >> regmap_update_bits(regmap, 0x8000, 0x7f0, 0x7f0); >> >> -- >> 2.33.0 >>
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. - 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@gmail.com/
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. > > - 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.
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. > > - 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? I see that Michael Turquette had more or less the same ideas in 2015-2016. It would ensure that the clock chain stays on till msm takes over the efifb/splash/etc. > > 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? > 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.
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@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. > 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@gmail.com/ > > -- > With best wishes > Dmitry
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.
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); + /* Enable hardware clock gating for DSI and MDP clocks */ regmap_update_bits(regmap, 0x8000, 0x7f0, 0x7f0);
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(+)