[PATCH 0/2] clk: qcom: fix disp_cc_mdss_mdp_clk_src issues on sdm845

[PATCH 0/2] clk: qcom: fix disp_cc_mdss_mdp_clk_src issues on sdm845

[Dmitry Baryshkov]

This is an alternative approach to the issue that Bjorn proposed in
https://lore.kernel.org/linux-arm-msm/20211203035436.3505743-1-bjorn.andersson@linaro.org/

The disp_cc_mdss_mdp_clk_src clock can become stuck during the boot
process for reasons other than just disabling the clocks in
clock_disable_unused phase. For example other drivers during the boot
procedure can toggle parent of the clock, disabling it for some reason.

So instead of enforcing clock parking during the clock_disable_unused,
park them during the driver probe. This can break the splash screen
display, however loosing the splash screen for few seconds is considered
to be lesser evil compared to possibly loosing the display at all
(because the RCG gets stuck).

----------------------------------------------------------------
Dmitry Baryshkov (2):
      clk: qcom: add API to safely park RCG2 sources
      clk: qcom: dispcc-sdm845: park disp_cc_mdss_mdp_clk_src

 drivers/clk/qcom/clk-rcg.h       |  2 ++
 drivers/clk/qcom/clk-rcg2.c      | 34 ++++++++++++++++++++++++++++++++++
 drivers/clk/qcom/dispcc-sdm845.c |  3 +++
 3 files changed, 39 insertions(+)

[PATCH 1/2] clk: qcom: add API to safely park RCG2 sources

[Dmitry Baryshkov]

Some of RCG2 clocks can become stuck during the boot process, when
device drivers are enabling and disabling the RCG2's parent clocks.
To prevernt such outcome of driver probe sequences, add API to park
clocks to the safe clock source (typically TCXO).

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/clk-rcg.h  |  2 ++
 drivers/clk/qcom/clk-rcg2.c | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/drivers/clk/qcom/clk-rcg.h b/drivers/clk/qcom/clk-rcg.h
index 99efcc7f8d88..9708b8799b01 100644
--- a/drivers/clk/qcom/clk-rcg.h
+++ b/drivers/clk/qcom/clk-rcg.h
@@ -172,6 +172,8 @@ extern const struct clk_ops clk_gfx3d_ops;
 extern const struct clk_ops clk_rcg2_shared_ops;
 extern const struct clk_ops clk_dp_ops;
 
+int clk_rcg2_park_safely(struct regmap *regmap, u32 offset, unsigned int safe_src);
+
 struct clk_rcg_dfs_data {
 	struct clk_rcg2 *rcg;
 	struct clk_init_data *init;
diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
index e1b1b426fae4..230b04a7427c 100644
--- a/drivers/clk/qcom/clk-rcg2.c
+++ b/drivers/clk/qcom/clk-rcg2.c
@@ -1036,6 +1036,40 @@ static void clk_rcg2_shared_disable(struct clk_hw *hw)
 	regmap_write(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, cfg);
 }
 
+int clk_rcg2_park_safely(struct regmap *regmap, u32 offset, unsigned int safe_src)
+{
+	unsigned int val, ret, count;
+
+	ret = regmap_read(regmap, offset + CFG_REG, &val);
+	if (ret)
+		return ret;
+
+	/* assume safe source is 0 */
+	if ((val & CFG_SRC_SEL_MASK) == (safe_src << CFG_SRC_SEL_SHIFT))
+		return 0;
+
+	regmap_write(regmap, offset + CFG_REG, safe_src << CFG_SRC_SEL_SHIFT);
+
+	ret = regmap_update_bits(regmap, offset + CMD_REG,
+				 CMD_UPDATE, CMD_UPDATE);
+	if (ret)
+		return ret;
+
+	/* Wait for update to take effect */
+	for (count = 500; count > 0; count--) {
+		ret = regmap_read(regmap, offset + CMD_REG, &val);
+		if (ret)
+			return ret;
+		if (!(val & CMD_UPDATE))
+			return 0;
+		udelay(1);
+	}
+
+	WARN(1, "the rcg didn't update its configuration.");
+	return -EBUSY;
+}
+EXPORT_SYMBOL_GPL(clk_rcg2_park_safely);
+
 const struct clk_ops clk_rcg2_shared_ops = {
 	.enable = clk_rcg2_shared_enable,
 	.disable = clk_rcg2_shared_disable,
-- 
2.33.0

[PATCH 2/2] clk: qcom: dispcc-sdm845: park disp_cc_mdss_mdp_clk_src

[Dmitry Baryshkov]

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);
+
 	/* Enable hardware clock gating for DSI and MDP clocks */
 	regmap_update_bits(regmap, 0x8000, 0x7f0, 0x7f0);
 
-- 
2.33.0

Re: [PATCH 1/2] clk: qcom: add API to safely park RCG2 sources

[Stephen Boyd]

Quoting Dmitry Baryshkov (2021-12-07 18:22:09)
> Some of RCG2 clocks can become stuck during the boot process, when
> device drivers are enabling and disabling the RCG2's parent clocks.
> To prevernt such outcome of driver probe sequences, add API to park

s/prevernt/prevent/

> clocks to the safe clock source (typically TCXO).
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

I'd prefer this approach vs. adding a new clk flag. The clk framework
doesn't handle handoff properly today so we shouldn't try to bandage
that in the core.

> diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
> index e1b1b426fae4..230b04a7427c 100644
> --- a/drivers/clk/qcom/clk-rcg2.c
> +++ b/drivers/clk/qcom/clk-rcg2.c
> @@ -1036,6 +1036,40 @@ static void clk_rcg2_shared_disable(struct clk_hw *hw)
>         regmap_write(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, cfg);
>  }
>  
> +int clk_rcg2_park_safely(struct regmap *regmap, u32 offset, unsigned int safe_src)

Please add kernel doc as it's an exported symbol.

> +{
> +       unsigned int val, ret, count;
> +
> +       ret = regmap_read(regmap, offset + CFG_REG, &val);
> +       if (ret)
> +               return ret;
> +
> +       /* assume safe source is 0 */

Are we assuming safe source is 0 here? It looks like we pass it in now?

> +       if ((val & CFG_SRC_SEL_MASK) == (safe_src << CFG_SRC_SEL_SHIFT))
> +               return 0;
> +
> +       regmap_write(regmap, offset + CFG_REG, safe_src << CFG_SRC_SEL_SHIFT);
> +
> +       ret = regmap_update_bits(regmap, offset + CMD_REG,
> +                                CMD_UPDATE, CMD_UPDATE);
> +       if (ret)
> +               return ret;
> +
> +       /* Wait for update to take effect */
> +       for (count = 500; count > 0; count--) {
> +               ret = regmap_read(regmap, offset + CMD_REG, &val);
> +               if (ret)
> +                       return ret;
> +               if (!(val & CMD_UPDATE))
> +                       return 0;
> +               udelay(1);
> +       }
> +
> +       WARN(1, "the rcg didn't update its configuration.");

Add a newline?

> +       return -EBUSY;
> +}
> +EXPORT_SYMBOL_GPL(clk_rcg2_park_safely);
> +

Re: [PATCH 2/2] clk: qcom: dispcc-sdm845: park disp_cc_mdss_mdp_clk_src

[Stephen Boyd]

Quoting Dmitry Baryshkov (2021-12-07 18:22:10)
> 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>
> ---

Reviewed-by: Stephen Boyd <sboyd@kernel.org>

Re: [PATCH 2/2] clk: qcom: dispcc-sdm845: park disp_cc_mdss_mdp_clk_src

[Robert Foss]

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>

Re: [PATCH 2/2] clk: qcom: dispcc-sdm845: park disp_cc_mdss_mdp_clk_src

[Dmitry Baryshkov]

On Thu, 9 Dec 2021 at 17:11, Robert Foss <robert.foss@linaro.org> wrote:
>
> 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.

Nice idea!

>
> > +
> >         /* Enable hardware clock gating for DSI and MDP clocks */
> >         regmap_update_bits(regmap, 0x8000, 0x7f0, 0x7f0);
> >
>
> Reviewed-by: Robert Foss <robert.foss@linaro.org>



-- 
With best wishes
Dmitry

Re: [PATCH 1/2] clk: qcom: add API to safely park RCG2 sources

[Bjorn Andersson]

On Thu 09 Dec 00:37 PST 2021, Stephen Boyd wrote:

> Quoting Dmitry Baryshkov (2021-12-07 18:22:09)
> > Some of RCG2 clocks can become stuck during the boot process, when
> > device drivers are enabling and disabling the RCG2's parent clocks.
> > To prevernt such outcome of driver probe sequences, add API to park
> 
> s/prevernt/prevent/
> 
> > clocks to the safe clock source (typically TCXO).
> > 
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> I'd prefer this approach vs. adding a new clk flag. The clk framework
> doesn't handle handoff properly today so we shouldn't try to bandage
> that in the core.
> 

I'm not against putting this responsibility in the drivers, but I don't
think we can blindly park all the RCGs that may or may not be used.

Note that we should do this for all RCGs that downstream are marked as
enable_safe_config (upstream should be using clk_rcg2_shared_ops)
and disabling some of those probe time won't be appreciated by the
hardware.


If you don't like the flag passed to clk_disable_unused (which is like a
very reasonable objection to have), we need to make progress towards a
proper solution that replaces clk_disable_unused().

> > diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
> > index e1b1b426fae4..230b04a7427c 100644
> > --- a/drivers/clk/qcom/clk-rcg2.c
> > +++ b/drivers/clk/qcom/clk-rcg2.c
> > @@ -1036,6 +1036,40 @@ static void clk_rcg2_shared_disable(struct clk_hw *hw)
> >         regmap_write(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, cfg);
> >  }
> >  
> > +int clk_rcg2_park_safely(struct regmap *regmap, u32 offset, unsigned int safe_src)

This seems to just duplicate clk_rcg2_shared_disable()?

Regards,
Bjorn

> 
> Please add kernel doc as it's an exported symbol.
> 
> > +{
> > +       unsigned int val, ret, count;
> > +
> > +       ret = regmap_read(regmap, offset + CFG_REG, &val);
> > +       if (ret)
> > +               return ret;
> > +
> > +       /* assume safe source is 0 */
> 
> Are we assuming safe source is 0 here? It looks like we pass it in now?
> 
> > +       if ((val & CFG_SRC_SEL_MASK) == (safe_src << CFG_SRC_SEL_SHIFT))
> > +               return 0;
> > +
> > +       regmap_write(regmap, offset + CFG_REG, safe_src << CFG_SRC_SEL_SHIFT);
> > +
> > +       ret = regmap_update_bits(regmap, offset + CMD_REG,
> > +                                CMD_UPDATE, CMD_UPDATE);
> > +       if (ret)
> > +               return ret;
> > +
> > +       /* Wait for update to take effect */
> > +       for (count = 500; count > 0; count--) {
> > +               ret = regmap_read(regmap, offset + CMD_REG, &val);
> > +               if (ret)
> > +                       return ret;
> > +               if (!(val & CMD_UPDATE))
> > +                       return 0;
> > +               udelay(1);
> > +       }
> > +
> > +       WARN(1, "the rcg didn't update its configuration.");
> 
> Add a newline?
> 
> > +       return -EBUSY;
> > +}
> > +EXPORT_SYMBOL_GPL(clk_rcg2_park_safely);
> > +

Re: [PATCH 2/2] clk: qcom: dispcc-sdm845: park disp_cc_mdss_mdp_clk_src

[Bjorn Andersson]

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
> 

Re: [PATCH 2/2] clk: qcom: dispcc-sdm845: park disp_cc_mdss_mdp_clk_src

[Dmitry Baryshkov]

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
>>


-- 
With best wishes
Dmitry

Re: [PATCH 1/2] clk: qcom: add API to safely park RCG2 sources

[Dmitry Baryshkov]

On 09/12/2021 21:36, Bjorn Andersson wrote:
> On Thu 09 Dec 00:37 PST 2021, Stephen Boyd wrote:
> 
>> Quoting Dmitry Baryshkov (2021-12-07 18:22:09)
>>> Some of RCG2 clocks can become stuck during the boot process, when
>>> device drivers are enabling and disabling the RCG2's parent clocks.
>>> To prevernt such outcome of driver probe sequences, add API to park
>>
>> s/prevernt/prevent/
>>
>>> clocks to the safe clock source (typically TCXO).
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>
>> I'd prefer this approach vs. adding a new clk flag. The clk framework
>> doesn't handle handoff properly today so we shouldn't try to bandage
>> that in the core.
>>
> 
> I'm not against putting this responsibility in the drivers, but I don't
> think we can blindly park all the RCGs that may or may not be used.
> 
> Note that we should do this for all RCGs that downstream are marked as
> enable_safe_config (upstream should be using clk_rcg2_shared_ops)
> and disabling some of those probe time won't be appreciated by the
> hardware.

Only for the hardware as crazy, as displays. And maybe gmu_clk_src. I 
don't think we expect venus or camcc to be really clocking when kernel 
boots.

> 
> 
> If you don't like the flag passed to clk_disable_unused (which is like a
> very reasonable objection to have), we need to make progress towards a
> proper solution that replaces clk_disable_unused().

The issue is that at the time of clk_disable_unused() it can be too 
late, for example because msm being built-in into the kernel has already 
tried to play with PLLs/GDSCs and thus made RCG stuck. This is what I 
was observing on RB3 if the msm driver is built in and the splash screen 
is enabled.

> 
>>> diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
>>> index e1b1b426fae4..230b04a7427c 100644
>>> --- a/drivers/clk/qcom/clk-rcg2.c
>>> +++ b/drivers/clk/qcom/clk-rcg2.c
>>> @@ -1036,6 +1036,40 @@ static void clk_rcg2_shared_disable(struct clk_hw *hw)
>>>          regmap_write(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, cfg);
>>>   }
>>>   
>>> +int clk_rcg2_park_safely(struct regmap *regmap, u32 offset, unsigned int safe_src)
> 
> This seems to just duplicate clk_rcg2_shared_disable()?

A light version of it. It does not do force_on/_off. And also it can not 
rely on clkr->regmap or clock name being set. Initially I used 
clk_rcg2_shared_disable + several patches to stop it from crashing if it 
is used on the non-registered clock. Then I just decided to write 
special helper.

> 
> Regards,
> Bjorn
> 
>>
>> Please add kernel doc as it's an exported symbol.

Ack

>>
>>> +{
>>> +       unsigned int val, ret, count;
>>> +
>>> +       ret = regmap_read(regmap, offset + CFG_REG, &val);
>>> +       if (ret)
>>> +               return ret;
>>> +
>>> +       /* assume safe source is 0 */
>>
>> Are we assuming safe source is 0 here? It looks like we pass it in now?

Leftover, will remove if/when posting v2.

>>
>>> +       if ((val & CFG_SRC_SEL_MASK) == (safe_src << CFG_SRC_SEL_SHIFT))
>>> +               return 0;
>>> +
>>> +       regmap_write(regmap, offset + CFG_REG, safe_src << CFG_SRC_SEL_SHIFT);
>>> +
>>> +       ret = regmap_update_bits(regmap, offset + CMD_REG,
>>> +                                CMD_UPDATE, CMD_UPDATE);
>>> +       if (ret)
>>> +               return ret;
>>> +
>>> +       /* Wait for update to take effect */
>>> +       for (count = 500; count > 0; count--) {
>>> +               ret = regmap_read(regmap, offset + CMD_REG, &val);
>>> +               if (ret)
>>> +                       return ret;
>>> +               if (!(val & CMD_UPDATE))
>>> +                       return 0;
>>> +               udelay(1);
>>> +       }
>>> +
>>> +       WARN(1, "the rcg didn't update its configuration.");
>>
>> Add a newline?

Ack.

>>
>>> +       return -EBUSY;
>>> +}
>>> +EXPORT_SYMBOL_GPL(clk_rcg2_park_safely);
>>> +


-- 
With best wishes
Dmitry

Re: [PATCH 2/2] clk: qcom: dispcc-sdm845: park disp_cc_mdss_mdp_clk_src

[Dmitry Baryshkov]

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/

-- 
With best wishes
Dmitry

Re: [PATCH 2/2] clk: qcom: dispcc-sdm845: park disp_cc_mdss_mdp_clk_src

[Stephen Boyd]

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.

Re: [PATCH 2/2] clk: qcom: dispcc-sdm845: park disp_cc_mdss_mdp_clk_src

[Dmitry Baryshkov]

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.



-- 
With best wishes
Dmitry

Re: [PATCH 2/2] clk: qcom: dispcc-sdm845: park disp_cc_mdss_mdp_clk_src

[Bjorn Andersson]

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

Re: [PATCH 2/2] clk: qcom: dispcc-sdm845: park disp_cc_mdss_mdp_clk_src

[Stephen Boyd]

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.

Re: [PATCH 1/2] clk: qcom: add API to safely park RCG2 sources

[Bjorn Andersson]

On Wed 15 Dec 13:14 PST 2021, Dmitry Baryshkov wrote:

> On 09/12/2021 21:36, Bjorn Andersson wrote:
> > On Thu 09 Dec 00:37 PST 2021, Stephen Boyd wrote:
> > 
> > > Quoting Dmitry Baryshkov (2021-12-07 18:22:09)
> > > > Some of RCG2 clocks can become stuck during the boot process, when
> > > > device drivers are enabling and disabling the RCG2's parent clocks.
> > > > To prevernt such outcome of driver probe sequences, add API to park
> > > 
> > > s/prevernt/prevent/
> > > 
> > > > clocks to the safe clock source (typically TCXO).
> > > > 
> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > 
> > > I'd prefer this approach vs. adding a new clk flag. The clk framework
> > > doesn't handle handoff properly today so we shouldn't try to bandage
> > > that in the core.
> > > 
> > 
> > I'm not against putting this responsibility in the drivers, but I don't
> > think we can blindly park all the RCGs that may or may not be used.
> > 
> > Note that we should do this for all RCGs that downstream are marked as
> > enable_safe_config (upstream should be using clk_rcg2_shared_ops)
> > and disabling some of those probe time won't be appreciated by the
> > hardware.
> 
> Only for the hardware as crazy, as displays. And maybe gmu_clk_src. I don't
> think we expect venus or camcc to be really clocking when kernel boots.
> 

SM8350 GCC has 44 clocks marked as such downstream.

> > 
> > 
> > If you don't like the flag passed to clk_disable_unused (which is like a
> > very reasonable objection to have), we need to make progress towards a
> > proper solution that replaces clk_disable_unused().
> 
> The issue is that at the time of clk_disable_unused() it can be too late,
> for example because msm being built-in into the kernel has already tried to
> play with PLLs/GDSCs and thus made RCG stuck.

Makes sense, so this logic will have to consider both the hardware state
(or make assumptions thereof) and the clock votes in the kernel.

> This is what I was observing
> on RB3 if the msm driver is built in and the splash screen is enabled.
> 

Which clock was this?

We should be able to assume that the bootloader will hand us a clock
tree that's functionally configured, so reparenting etc of the RCGs
should not cause issues because the old parent will be ticking and we
will explicitly start the new parent.

One case that might not be handled though is the externally sourced
clocks, where if you reconfigure the DSI phy without first parking the
RCG you might lock up the RCG. So I think that whenever we mess with
those clocks we need to make sure that the downstream RCGs are not
ticking off them.

> > 
> > > > diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
> > > > index e1b1b426fae4..230b04a7427c 100644
> > > > --- a/drivers/clk/qcom/clk-rcg2.c
> > > > +++ b/drivers/clk/qcom/clk-rcg2.c
> > > > @@ -1036,6 +1036,40 @@ static void clk_rcg2_shared_disable(struct clk_hw *hw)
> > > >          regmap_write(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, cfg);
> > > >   }
> > > > +int clk_rcg2_park_safely(struct regmap *regmap, u32 offset, unsigned int safe_src)
> > 
> > This seems to just duplicate clk_rcg2_shared_disable()?
> 
> A light version of it. It does not do force_on/_off. And also it can not
> rely on clkr->regmap or clock name being set. Initially I used
> clk_rcg2_shared_disable + several patches to stop it from crashing if it is
> used on the non-registered clock. Then I just decided to write special
> helper.
> 

Okay, makes sense then. But I don't think we want to shoot down clocks
at clock probe time.

Regards,
Bjorn

> > 
> > Regards,
> > Bjorn
> > 
> > > 
> > > Please add kernel doc as it's an exported symbol.
> 
> Ack
> 
> > > 
> > > > +{
> > > > +       unsigned int val, ret, count;
> > > > +
> > > > +       ret = regmap_read(regmap, offset + CFG_REG, &val);
> > > > +       if (ret)
> > > > +               return ret;
> > > > +
> > > > +       /* assume safe source is 0 */
> > > 
> > > Are we assuming safe source is 0 here? It looks like we pass it in now?
> 
> Leftover, will remove if/when posting v2.
> 
> > > 
> > > > +       if ((val & CFG_SRC_SEL_MASK) == (safe_src << CFG_SRC_SEL_SHIFT))
> > > > +               return 0;
> > > > +
> > > > +       regmap_write(regmap, offset + CFG_REG, safe_src << CFG_SRC_SEL_SHIFT);
> > > > +
> > > > +       ret = regmap_update_bits(regmap, offset + CMD_REG,
> > > > +                                CMD_UPDATE, CMD_UPDATE);
> > > > +       if (ret)
> > > > +               return ret;
> > > > +
> > > > +       /* Wait for update to take effect */
> > > > +       for (count = 500; count > 0; count--) {
> > > > +               ret = regmap_read(regmap, offset + CMD_REG, &val);
> > > > +               if (ret)
> > > > +                       return ret;
> > > > +               if (!(val & CMD_UPDATE))
> > > > +                       return 0;
> > > > +               udelay(1);
> > > > +       }
> > > > +
> > > > +       WARN(1, "the rcg didn't update its configuration.");
> > > 
> > > Add a newline?
> 
> Ack.
> 
> > > 
> > > > +       return -EBUSY;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(clk_rcg2_park_safely);
> > > > +
> 
> 
> -- 
> With best wishes
> Dmitry