[v1 1/2] clk: qcom: gdsc: Use the default transition delay for GDSCs

[v1 1/2] clk: qcom: gdsc: Use the default transition delay for GDSCs

[Taniya Das]

Do not update the transition delay and use the default reset values.

Fixes: 45dd0e55317cc ("clk: qcom: Add support for GDSCs)
Signed-off-by: Taniya Das <tdas@codeaurora.org>
---
 drivers/clk/qcom/gdsc.c | 6 +++++-
 drivers/clk/qcom/gdsc.h | 1 +
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index 7e1dd8ccfa38..e7b213450640 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -380,7 +380,11 @@ static int gdsc_init(struct gdsc *sc)
 	 */
 	mask = HW_CONTROL_MASK | SW_OVERRIDE_MASK |
 	       EN_REST_WAIT_MASK | EN_FEW_WAIT_MASK | CLK_DIS_WAIT_MASK;
-	val = EN_REST_WAIT_VAL | EN_FEW_WAIT_VAL | CLK_DIS_WAIT_VAL;
+
+	regmap_read(sc->regmap, sc->gdscr, &val);
+
+	if (!(sc->flags & DEFAULT_TRANSITION_DELAY))
+		val |= EN_REST_WAIT_VAL | EN_FEW_WAIT_VAL | CLK_DIS_WAIT_VAL;
 	ret = regmap_update_bits(sc->regmap, sc->gdscr, mask, val);
 	if (ret)
 		return ret;
diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
index d7cc4c21a9d4..1bd3ecdd0b0a 100644
--- a/drivers/clk/qcom/gdsc.h
+++ b/drivers/clk/qcom/gdsc.h
@@ -53,6 +53,7 @@ struct gdsc {
 #define ALWAYS_ON	BIT(6)
 #define RETAIN_FF_ENABLE	BIT(7)
 #define NO_RET_PERIPH	BIT(8)
+#define DEFAULT_TRANSITION_DELAY	BIT(9)
 	struct reset_controller_dev	*rcdev;
 	unsigned int			*resets;
 	unsigned int			reset_count;
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.

[v1 2/2] clk: qcom: dispcc: Update gdsc flag for display GDSC

[Taniya Das]

Update the mdss gdsc flag for SC7180/SC7280/SM8150/SM8250
to not program the transition delay.

Fixes: dd3d06622138 ("clk: qcom: Add display clock controller driver for SC7180")
Fixes: 1a00c962f9cd ("clk: qcom: Add display clock controller driver for SC7280")
Fixes: 80a18f4a8567 ("clk: qcom: Add display clock controller driver for SM8150 and SM8250")
Signed-off-by: Taniya Das <tdas@codeaurora.org>
---
 drivers/clk/qcom/dispcc-sc7180.c | 2 +-
 drivers/clk/qcom/dispcc-sc7280.c | 2 +-
 drivers/clk/qcom/dispcc-sm8250.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/qcom/dispcc-sc7180.c b/drivers/clk/qcom/dispcc-sc7180.c
index 538e4963c915..c203888ab2ca 100644
--- a/drivers/clk/qcom/dispcc-sc7180.c
+++ b/drivers/clk/qcom/dispcc-sc7180.c
@@ -629,7 +629,7 @@ static struct gdsc mdss_gdsc = {
 		.name = "mdss_gdsc",
 	},
 	.pwrsts = PWRSTS_OFF_ON,
-	.flags = HW_CTRL,
+	.flags = HW_CTRL |  DEFAULT_TRANSITION_DELAY,
 };

 static struct gdsc *disp_cc_sc7180_gdscs[] = {
diff --git a/drivers/clk/qcom/dispcc-sc7280.c b/drivers/clk/qcom/dispcc-sc7280.c
index 4ef4ae231794..8e1a2d4a8120 100644
--- a/drivers/clk/qcom/dispcc-sc7280.c
+++ b/drivers/clk/qcom/dispcc-sc7280.c
@@ -791,7 +791,7 @@ static struct gdsc disp_cc_mdss_core_gdsc = {
 		.name = "disp_cc_mdss_core_gdsc",
 	},
 	.pwrsts = PWRSTS_OFF_ON,
-	.flags = HW_CTRL | RETAIN_FF_ENABLE,
+	.flags = HW_CTRL | RETAIN_FF_ENABLE | DEFAULT_TRANSITION_DELAY,
 };

 static struct clk_regmap *disp_cc_sc7280_clocks[] = {
diff --git a/drivers/clk/qcom/dispcc-sm8250.c b/drivers/clk/qcom/dispcc-sm8250.c
index 566fdfa0a15b..4d64d499a285 100644
--- a/drivers/clk/qcom/dispcc-sm8250.c
+++ b/drivers/clk/qcom/dispcc-sm8250.c
@@ -1130,7 +1130,7 @@ static struct gdsc mdss_gdsc = {
 		.name = "mdss_gdsc",
 	},
 	.pwrsts = PWRSTS_OFF_ON,
-	.flags = HW_CTRL,
+	.flags = HW_CTRL | DEFAULT_TRANSITION_DELAY,
 	.supply = "mmcx",
 };

--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.

Re: [v1 1/2] clk: qcom: gdsc: Use the default transition delay for GDSCs

[Bjorn Andersson]

On Wed 09 Feb 11:25 CST 2022, Taniya Das wrote:

> Do not update the transition delay and use the default reset values.
> 

Can you help me understand what "the transition delay" is? In particular
I would like to know which other platforms, past, present and future
should have this flag set - or not set.

> Fixes: 45dd0e55317cc ("clk: qcom: Add support for GDSCs)
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> ---
>  drivers/clk/qcom/gdsc.c | 6 +++++-
>  drivers/clk/qcom/gdsc.h | 1 +
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index 7e1dd8ccfa38..e7b213450640 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -380,7 +380,11 @@ static int gdsc_init(struct gdsc *sc)
>  	 */
>  	mask = HW_CONTROL_MASK | SW_OVERRIDE_MASK |
>  	       EN_REST_WAIT_MASK | EN_FEW_WAIT_MASK | CLK_DIS_WAIT_MASK;
> -	val = EN_REST_WAIT_VAL | EN_FEW_WAIT_VAL | CLK_DIS_WAIT_VAL;
> +
> +	regmap_read(sc->regmap, sc->gdscr, &val);
> +
> +	if (!(sc->flags & DEFAULT_TRANSITION_DELAY))

Why is it a good idea to not have a transition delay other than the
default one on all the other gdscs?

Regards,
Bjorn

> +		val |= EN_REST_WAIT_VAL | EN_FEW_WAIT_VAL | CLK_DIS_WAIT_VAL;
>  	ret = regmap_update_bits(sc->regmap, sc->gdscr, mask, val);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
> index d7cc4c21a9d4..1bd3ecdd0b0a 100644
> --- a/drivers/clk/qcom/gdsc.h
> +++ b/drivers/clk/qcom/gdsc.h
> @@ -53,6 +53,7 @@ struct gdsc {
>  #define ALWAYS_ON	BIT(6)
>  #define RETAIN_FF_ENABLE	BIT(7)
>  #define NO_RET_PERIPH	BIT(8)
> +#define DEFAULT_TRANSITION_DELAY	BIT(9)
>  	struct reset_controller_dev	*rcdev;
>  	unsigned int			*resets;
>  	unsigned int			reset_count;
> --
> Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
> of the Code Aurora Forum, hosted by the  Linux Foundation.
> 

Re: [v1 2/2] clk: qcom: dispcc: Update gdsc flag for display GDSC

[Steev Klimaszewski]

Hi Taniya,

On 2/9/22 11:25 AM, Taniya Das wrote:
> Update the mdss gdsc flag for SC7180/SC7280/SM8150/SM8250
> to not program the transition delay.
>
> Fixes: dd3d06622138 ("clk: qcom: Add display clock controller driver for SC7180")
> Fixes: 1a00c962f9cd ("clk: qcom: Add display clock controller driver for SC7280")
> Fixes: 80a18f4a8567 ("clk: qcom: Add display clock controller driver for SM8150 and SM8250")
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> ---
>   drivers/clk/qcom/dispcc-sc7180.c | 2 +-
>   drivers/clk/qcom/dispcc-sc7280.c | 2 +-
>   drivers/clk/qcom/dispcc-sm8250.c | 2 +-
>   3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/qcom/dispcc-sc7180.c b/drivers/clk/qcom/dispcc-sc7180.c
> index 538e4963c915..c203888ab2ca 100644
> --- a/drivers/clk/qcom/dispcc-sc7180.c
> +++ b/drivers/clk/qcom/dispcc-sc7180.c
> @@ -629,7 +629,7 @@ static struct gdsc mdss_gdsc = {
>   		.name = "mdss_gdsc",
>   	},
>   	.pwrsts = PWRSTS_OFF_ON,
> -	.flags = HW_CTRL,
> +	.flags = HW_CTRL |  DEFAULT_TRANSITION_DELAY,
>   };
There's an extra space after the |
>   static struct gdsc *disp_cc_sc7180_gdscs[] = {
> diff --git a/drivers/clk/qcom/dispcc-sc7280.c b/drivers/clk/qcom/dispcc-sc7280.c
> index 4ef4ae231794..8e1a2d4a8120 100644
> --- a/drivers/clk/qcom/dispcc-sc7280.c
> +++ b/drivers/clk/qcom/dispcc-sc7280.c
> @@ -791,7 +791,7 @@ static struct gdsc disp_cc_mdss_core_gdsc = {
>   		.name = "disp_cc_mdss_core_gdsc",
>   	},
>   	.pwrsts = PWRSTS_OFF_ON,
> -	.flags = HW_CTRL | RETAIN_FF_ENABLE,
> +	.flags = HW_CTRL | RETAIN_FF_ENABLE | DEFAULT_TRANSITION_DELAY,
>   };
>
>   static struct clk_regmap *disp_cc_sc7280_clocks[] = {
> diff --git a/drivers/clk/qcom/dispcc-sm8250.c b/drivers/clk/qcom/dispcc-sm8250.c
> index 566fdfa0a15b..4d64d499a285 100644
> --- a/drivers/clk/qcom/dispcc-sm8250.c
> +++ b/drivers/clk/qcom/dispcc-sm8250.c
> @@ -1130,7 +1130,7 @@ static struct gdsc mdss_gdsc = {
>   		.name = "mdss_gdsc",
>   	},
>   	.pwrsts = PWRSTS_OFF_ON,
> -	.flags = HW_CTRL,
> +	.flags = HW_CTRL | DEFAULT_TRANSITION_DELAY,
>   	.supply = "mmcx",
>   };
>
> --
> Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
> of the Code Aurora Forum, hosted by the  Linux Foundation.
>
-- steev

Re: [v1 1/2] clk: qcom: gdsc: Use the default transition delay for GDSCs

[Bjorn Andersson]

On Wed 09 Feb 11:25 CST 2022, Taniya Das wrote:

> Do not update the transition delay and use the default reset values.
> 
> Fixes: 45dd0e55317cc ("clk: qcom: Add support for GDSCs)
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> ---
>  drivers/clk/qcom/gdsc.c | 6 +++++-
>  drivers/clk/qcom/gdsc.h | 1 +
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index 7e1dd8ccfa38..e7b213450640 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -380,7 +380,11 @@ static int gdsc_init(struct gdsc *sc)
>  	 */
>  	mask = HW_CONTROL_MASK | SW_OVERRIDE_MASK |
>  	       EN_REST_WAIT_MASK | EN_FEW_WAIT_MASK | CLK_DIS_WAIT_MASK;
> -	val = EN_REST_WAIT_VAL | EN_FEW_WAIT_VAL | CLK_DIS_WAIT_VAL;
> +
> +	regmap_read(sc->regmap, sc->gdscr, &val);
> +
> +	if (!(sc->flags & DEFAULT_TRANSITION_DELAY))

I dug a little bit more into this and noticed that on various platforms
CLK_DIS_WAIT_VAL for the GPU_CX GDSC is supposed to be 8 (whereas both
hw default and CLK_DIS_WAIT_VAL is 2).

I'm not able to find anything helpful in the git log describing what the
value does, but it seems that a "just use hw default" flag won't cut it
for this scenario.

Regards,
Bjorn

> +		val |= EN_REST_WAIT_VAL | EN_FEW_WAIT_VAL | CLK_DIS_WAIT_VAL;
>  	ret = regmap_update_bits(sc->regmap, sc->gdscr, mask, val);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
> index d7cc4c21a9d4..1bd3ecdd0b0a 100644
> --- a/drivers/clk/qcom/gdsc.h
> +++ b/drivers/clk/qcom/gdsc.h
> @@ -53,6 +53,7 @@ struct gdsc {
>  #define ALWAYS_ON	BIT(6)
>  #define RETAIN_FF_ENABLE	BIT(7)
>  #define NO_RET_PERIPH	BIT(8)
> +#define DEFAULT_TRANSITION_DELAY	BIT(9)
>  	struct reset_controller_dev	*rcdev;
>  	unsigned int			*resets;
>  	unsigned int			reset_count;
> --
> Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
> of the Code Aurora Forum, hosted by the  Linux Foundation.
> 

Re: [v1 1/2] clk: qcom: gdsc: Use the default transition delay for GDSCs

[Stephen Boyd]

Quoting Taniya Das (2022-02-09 09:25:12)
> Do not update the transition delay and use the default reset values.

Why? I know why because I'm CCed on the internal bug but nobody else
knows what's going on here. Please clearly describe what you're fixing,
i.e. that you're seeing black screens on sc7180 because the gdsc is
being enabled/disabled so rapidly that the hardware is getting borked.
Lots more detail please. Otherwise we're going to ignore this patch
series as nonsense because we don't have enough information to realize
that it is important.

Re: [v1 1/2] clk: qcom: gdsc: Use the default transition delay for GDSCs

[Stephen Boyd]

Quoting Bjorn Andersson (2022-02-09 14:35:08)
> On Wed 09 Feb 11:25 CST 2022, Taniya Das wrote:
> 
> > Do not update the transition delay and use the default reset values.
> > 
> > Fixes: 45dd0e55317cc ("clk: qcom: Add support for GDSCs)
> > Signed-off-by: Taniya Das <tdas@codeaurora.org>
> > ---
> >  drivers/clk/qcom/gdsc.c | 6 +++++-
> >  drivers/clk/qcom/gdsc.h | 1 +
> >  2 files changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> > index 7e1dd8ccfa38..e7b213450640 100644
> > --- a/drivers/clk/qcom/gdsc.c
> > +++ b/drivers/clk/qcom/gdsc.c
> > @@ -380,7 +380,11 @@ static int gdsc_init(struct gdsc *sc)
> >        */
> >       mask = HW_CONTROL_MASK | SW_OVERRIDE_MASK |
> >              EN_REST_WAIT_MASK | EN_FEW_WAIT_MASK | CLK_DIS_WAIT_MASK;
> > -     val = EN_REST_WAIT_VAL | EN_FEW_WAIT_VAL | CLK_DIS_WAIT_VAL;
> > +
> > +     regmap_read(sc->regmap, sc->gdscr, &val);
> > +
> > +     if (!(sc->flags & DEFAULT_TRANSITION_DELAY))
> 
> I dug a little bit more into this and noticed that on various platforms
> CLK_DIS_WAIT_VAL for the GPU_CX GDSC is supposed to be 8 (whereas both
> hw default and CLK_DIS_WAIT_VAL is 2).
> 
> I'm not able to find anything helpful in the git log describing what the
> value does, but it seems that a "just use hw default" flag won't cut it
> for this scenario.
> 

I'd prefer we invert the logic so that we don't need to litter this flag
all over the place. I recall that the wait values were incorrect a long
time ago on early gdsc using designs but hopefully they've been fixed
now and we can simply use the default power on reset (POR) values.

Re: [v1 1/2] clk: qcom: gdsc: Use the default transition delay for GDSCs

[Bjorn Andersson]

On Wed 09 Feb 11:25 CST 2022, Taniya Das wrote:

> Do not update the transition delay and use the default reset values.
> 
> Fixes: 45dd0e55317cc ("clk: qcom: Add support for GDSCs)
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> ---
>  drivers/clk/qcom/gdsc.c | 6 +++++-
>  drivers/clk/qcom/gdsc.h | 1 +
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index 7e1dd8ccfa38..e7b213450640 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -380,7 +380,11 @@ static int gdsc_init(struct gdsc *sc)
>  	 */
>  	mask = HW_CONTROL_MASK | SW_OVERRIDE_MASK |
>  	       EN_REST_WAIT_MASK | EN_FEW_WAIT_MASK | CLK_DIS_WAIT_MASK;
> -	val = EN_REST_WAIT_VAL | EN_FEW_WAIT_VAL | CLK_DIS_WAIT_VAL;
> +
> +	regmap_read(sc->regmap, sc->gdscr, &val);
> +
> +	if (!(sc->flags & DEFAULT_TRANSITION_DELAY))
> +		val |= EN_REST_WAIT_VAL | EN_FEW_WAIT_VAL | CLK_DIS_WAIT_VAL;

Forgot to point out this yesterday, but these are multi-bit values, so
you need to mask out whatever bits are already set before filling in the
new ones.

Regards,
Bjorn

>  	ret = regmap_update_bits(sc->regmap, sc->gdscr, mask, val);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
> index d7cc4c21a9d4..1bd3ecdd0b0a 100644
> --- a/drivers/clk/qcom/gdsc.h
> +++ b/drivers/clk/qcom/gdsc.h
> @@ -53,6 +53,7 @@ struct gdsc {
>  #define ALWAYS_ON	BIT(6)
>  #define RETAIN_FF_ENABLE	BIT(7)
>  #define NO_RET_PERIPH	BIT(8)
> +#define DEFAULT_TRANSITION_DELAY	BIT(9)
>  	struct reset_controller_dev	*rcdev;
>  	unsigned int			*resets;
>  	unsigned int			reset_count;
> --
> Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
> of the Code Aurora Forum, hosted by the  Linux Foundation.
> 

Re: [v1 1/2] clk: qcom: gdsc: Use the default transition delay for GDSCs

[Bjorn Andersson]

On Thu 10 Feb 01:28 CST 2022, Stephen Boyd wrote:

> Quoting Bjorn Andersson (2022-02-09 14:35:08)
> > On Wed 09 Feb 11:25 CST 2022, Taniya Das wrote:
> > 
> > > Do not update the transition delay and use the default reset values.
> > > 
> > > Fixes: 45dd0e55317cc ("clk: qcom: Add support for GDSCs)
> > > Signed-off-by: Taniya Das <tdas@codeaurora.org>
> > > ---
> > >  drivers/clk/qcom/gdsc.c | 6 +++++-
> > >  drivers/clk/qcom/gdsc.h | 1 +
> > >  2 files changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> > > index 7e1dd8ccfa38..e7b213450640 100644
> > > --- a/drivers/clk/qcom/gdsc.c
> > > +++ b/drivers/clk/qcom/gdsc.c
> > > @@ -380,7 +380,11 @@ static int gdsc_init(struct gdsc *sc)
> > >        */
> > >       mask = HW_CONTROL_MASK | SW_OVERRIDE_MASK |
> > >              EN_REST_WAIT_MASK | EN_FEW_WAIT_MASK | CLK_DIS_WAIT_MASK;
> > > -     val = EN_REST_WAIT_VAL | EN_FEW_WAIT_VAL | CLK_DIS_WAIT_VAL;
> > > +
> > > +     regmap_read(sc->regmap, sc->gdscr, &val);
> > > +
> > > +     if (!(sc->flags & DEFAULT_TRANSITION_DELAY))
> > 
> > I dug a little bit more into this and noticed that on various platforms
> > CLK_DIS_WAIT_VAL for the GPU_CX GDSC is supposed to be 8 (whereas both
> > hw default and CLK_DIS_WAIT_VAL is 2).
> > 
> > I'm not able to find anything helpful in the git log describing what the
> > value does, but it seems that a "just use hw default" flag won't cut it
> > for this scenario.
> > 
> 
> I'd prefer we invert the logic so that we don't need to litter this flag
> all over the place. I recall that the wait values were incorrect a long
> time ago on early gdsc using designs but hopefully they've been fixed
> now and we can simply use the default power on reset (POR) values.

Are you suggesting that we make it the default to not update the values
and then provide means to specify it where needed?

I like that suggestion.

But as mentioned in my reply yesterday, GPU_CX on several platforms
needs a different CLK_DIS_WAIT_VAL - and not the same value.

Are these values ever 0? Or could we simply add the three numbers to
struct gdsc and have 0 denote "use hw default"?

Regards,
Bjorn

Re: [v1 1/2] clk: qcom: gdsc: Use the default transition delay for GDSCs

[Stephen Boyd]

Quoting Bjorn Andersson (2022-02-10 11:32:03)
> On Thu 10 Feb 01:28 CST 2022, Stephen Boyd wrote:
> 
> > I'd prefer we invert the logic so that we don't need to litter this flag
> > all over the place. I recall that the wait values were incorrect a long
> > time ago on early gdsc using designs but hopefully they've been fixed
> > now and we can simply use the default power on reset (POR) values.
> 
> Are you suggesting that we make it the default to not update the values
> and then provide means to specify it where needed?

Sure!

> 
> I like that suggestion.
> 
> But as mentioned in my reply yesterday, GPU_CX on several platforms
> needs a different CLK_DIS_WAIT_VAL - and not the same value.
> 
> Are these values ever 0? Or could we simply add the three numbers to
> struct gdsc and have 0 denote "use hw default"?

I don't think they're ever set to zero. That would most likely just
break things because they need a few clk cycles to wait between
transitioning states. Otherwise the GDSC gets stuck and the clks don't
work.

Re: [v1 1/2] clk: qcom: gdsc: Use the default transition delay for GDSCs

[Taniya Das]

Hi Stephen, Bjorn,

Thanks for your comments.

On 2/10/2022 12:58 PM, Stephen Boyd wrote:
> Quoting Bjorn Andersson (2022-02-09 14:35:08)
>> On Wed 09 Feb 11:25 CST 2022, Taniya Das wrote:
>>
>>> Do not update the transition delay and use the default reset values.
>>>
>>> Fixes: 45dd0e55317cc ("clk: qcom: Add support for GDSCs)
>>> Signed-off-by: Taniya Das <tdas@codeaurora.org>
>>> ---
>>>   drivers/clk/qcom/gdsc.c | 6 +++++-
>>>   drivers/clk/qcom/gdsc.h | 1 +
>>>   2 files changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
>>> index 7e1dd8ccfa38..e7b213450640 100644
>>> --- a/drivers/clk/qcom/gdsc.c
>>> +++ b/drivers/clk/qcom/gdsc.c
>>> @@ -380,7 +380,11 @@ static int gdsc_init(struct gdsc *sc)
>>>         */
>>>        mask = HW_CONTROL_MASK | SW_OVERRIDE_MASK |
>>>               EN_REST_WAIT_MASK | EN_FEW_WAIT_MASK | CLK_DIS_WAIT_MASK;
>>> -     val = EN_REST_WAIT_VAL | EN_FEW_WAIT_VAL | CLK_DIS_WAIT_VAL;
>>> +
>>> +     regmap_read(sc->regmap, sc->gdscr, &val);
>>> +
>>> +     if (!(sc->flags & DEFAULT_TRANSITION_DELAY))
>>
>> I dug a little bit more into this and noticed that on various platforms
>> CLK_DIS_WAIT_VAL for the GPU_CX GDSC is supposed to be 8 (whereas both
>> hw default and CLK_DIS_WAIT_VAL is 2).
>>

Yes, only for certain GPU_CC these would be updated and that too in case 
the design team suggests. Downstream we would set the value from probe 
itself, or we can pick these from device tree as required and suggested.

>> I'm not able to find anything helpful in the git log describing what the
>> value does, but it seems that a "just use hw default" flag won't cut it
>> for this scenario.
>>

This value is used for the number of clock cycles it would wait before 
the GDSCR ACK signals/halting the clock.

> 
> I'd prefer we invert the logic so that we don't need to litter this flag
> all over the place. I recall that the wait values were incorrect a long
> time ago on early gdsc using designs but hopefully they've been fixed
> now and we can simply use the default power on reset (POR) values.

I am okay to invert the logic, but I am not sure if they could cause any 
issues to the older targets. They were broken in HW design long back, 
but got fixed in most families of target and most GDSCs.

As mentioned if explicitly they need to be updated, it is best to do 
from the probe.
This was done in SC7180 GPUCC driver.
         /* Configure clk_dis_wait for gpu_cx_gdsc */
         regmap_update_bits(regmap, 0x106c, CLK_DIS_WAIT_MASK,
                                         8 << CLK_DIS_WAIT_SHIFT);


Please let me know if we are okay to add the invert logic.

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

--

Re: [v1 1/2] clk: qcom: gdsc: Use the default transition delay for GDSCs

[Bjorn Andersson]

On Mon 21 Feb 08:55 PST 2022, Taniya Das wrote:

> Hi Stephen, Bjorn,
> 
> Thanks for your comments.
> 
> On 2/10/2022 12:58 PM, Stephen Boyd wrote:
> > Quoting Bjorn Andersson (2022-02-09 14:35:08)
> > > On Wed 09 Feb 11:25 CST 2022, Taniya Das wrote:
> > > 
> > > > Do not update the transition delay and use the default reset values.
> > > > 
> > > > Fixes: 45dd0e55317cc ("clk: qcom: Add support for GDSCs)
> > > > Signed-off-by: Taniya Das <tdas@codeaurora.org>
> > > > ---
> > > >   drivers/clk/qcom/gdsc.c | 6 +++++-
> > > >   drivers/clk/qcom/gdsc.h | 1 +
> > > >   2 files changed, 6 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> > > > index 7e1dd8ccfa38..e7b213450640 100644
> > > > --- a/drivers/clk/qcom/gdsc.c
> > > > +++ b/drivers/clk/qcom/gdsc.c
> > > > @@ -380,7 +380,11 @@ static int gdsc_init(struct gdsc *sc)
> > > >         */
> > > >        mask = HW_CONTROL_MASK | SW_OVERRIDE_MASK |
> > > >               EN_REST_WAIT_MASK | EN_FEW_WAIT_MASK | CLK_DIS_WAIT_MASK;
> > > > -     val = EN_REST_WAIT_VAL | EN_FEW_WAIT_VAL | CLK_DIS_WAIT_VAL;
> > > > +
> > > > +     regmap_read(sc->regmap, sc->gdscr, &val);
> > > > +
> > > > +     if (!(sc->flags & DEFAULT_TRANSITION_DELAY))
> > > 
> > > I dug a little bit more into this and noticed that on various platforms
> > > CLK_DIS_WAIT_VAL for the GPU_CX GDSC is supposed to be 8 (whereas both
> > > hw default and CLK_DIS_WAIT_VAL is 2).
> > > 
> 
> Yes, only for certain GPU_CC these would be updated and that too in case the
> design team suggests. Downstream we would set the value from probe itself,
> or we can pick these from device tree as required and suggested.
> 

I don't expect that value to be "configurable", so pushing it to DT
doesn't seem like the proper solution.

> > > I'm not able to find anything helpful in the git log describing what the
> > > value does, but it seems that a "just use hw default" flag won't cut it
> > > for this scenario.
> > > 
> 
> This value is used for the number of clock cycles it would wait before the
> GDSCR ACK signals/halting the clock.
> 

That makes sense.

> > 
> > I'd prefer we invert the logic so that we don't need to litter this flag
> > all over the place. I recall that the wait values were incorrect a long
> > time ago on early gdsc using designs but hopefully they've been fixed
> > now and we can simply use the default power on reset (POR) values.
> 
> I am okay to invert the logic, but I am not sure if they could cause any
> issues to the older targets. They were broken in HW design long back, but
> got fixed in most families of target and most GDSCs.
> 

I don't fancy us having a flag with the purpose of "don't set the
timings to 2, 8 and 2" and then rely on open coded writes in probe to
set it to something else where needed.

So I definitely would prefer to flip this around, to make the cases
where we want to write different values explicit.

But as you say, unless we make sure that all existing platforms do write
2, 8 and 2 we risk introducing regressions from the current behavior.

> As mentioned if explicitly they need to be updated, it is best to do from
> the probe.
> This was done in SC7180 GPUCC driver.
>         /* Configure clk_dis_wait for gpu_cx_gdsc */
>         regmap_update_bits(regmap, 0x106c, CLK_DIS_WAIT_MASK,
>                                         8 << CLK_DIS_WAIT_SHIFT);

But we call regmap_update_bits() from probe, which sets the CLK_DIS_WAIT
to 8, then we call qcom_cc_really_probe() which will end up in
gdsc_init() which replaces that with a 2.

Perhaps I'm missing something?

> 
> 
> Please let me know if we are okay to add the invert logic.
> 

I'm still favoring a scheme where we add 3 integers to struct gdsc and
in gdsc_init() we check if they are non-zero we write the value to the
register.

Although being a big patch, we could maintain the existing behaviour by
giving all existing struct gdsc definitions the values 2, 8 and 2 to
avoid regressions and we (everyone) can then go through the platforms
one by one and remove the unnecessary assignments - but more
importantly, double check with downstream if they need a different
value.

This will also have the side effect going forward that it will be more
explicit and people writing gdsc definitions should double check these
values.

Regards,
Bjorn

Re: [v1 1/2] clk: qcom: gdsc: Use the default transition delay for GDSCs

[Taniya Das]

Hello Bjorn,

Thanks for your comments.

On 2/22/2022 2:12 AM, Bjorn Andersson wrote:
> On Mon 21 Feb 08:55 PST 2022, Taniya Das wrote:
> 
>> Hi Stephen, Bjorn,
>>
>> Thanks for your comments.
>>
>> On 2/10/2022 12:58 PM, Stephen Boyd wrote:
>>> Quoting Bjorn Andersson (2022-02-09 14:35:08)
>>>> On Wed 09 Feb 11:25 CST 2022, Taniya Das wrote:
>>>>
>>>>> Do not update the transition delay and use the default reset values.
>>>>>
>>>>> Fixes: 45dd0e55317cc ("clk: qcom: Add support for GDSCs)
>>>>> Signed-off-by: Taniya Das <tdas@codeaurora.org>
>>>>> ---
>>>>>    drivers/clk/qcom/gdsc.c | 6 +++++-
>>>>>    drivers/clk/qcom/gdsc.h | 1 +
>>>>>    2 files changed, 6 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
>>>>> index 7e1dd8ccfa38..e7b213450640 100644
>>>>> --- a/drivers/clk/qcom/gdsc.c
>>>>> +++ b/drivers/clk/qcom/gdsc.c
>>>>> @@ -380,7 +380,11 @@ static int gdsc_init(struct gdsc *sc)
>>>>>          */
>>>>>         mask = HW_CONTROL_MASK | SW_OVERRIDE_MASK |
>>>>>                EN_REST_WAIT_MASK | EN_FEW_WAIT_MASK | CLK_DIS_WAIT_MASK;
>>>>> -     val = EN_REST_WAIT_VAL | EN_FEW_WAIT_VAL | CLK_DIS_WAIT_VAL;
>>>>> +
>>>>> +     regmap_read(sc->regmap, sc->gdscr, &val);
>>>>> +
>>>>> +     if (!(sc->flags & DEFAULT_TRANSITION_DELAY))
>>>>
>>>> I dug a little bit more into this and noticed that on various platforms
>>>> CLK_DIS_WAIT_VAL for the GPU_CX GDSC is supposed to be 8 (whereas both
>>>> hw default and CLK_DIS_WAIT_VAL is 2).
>>>>
>>
>> Yes, only for certain GPU_CC these would be updated and that too in case the
>> design team suggests. Downstream we would set the value from probe itself,
>> or we can pick these from device tree as required and suggested.
>>
> 
> I don't expect that value to be "configurable", so pushing it to DT
> doesn't seem like the proper solution.
> 
>>>> I'm not able to find anything helpful in the git log describing what the
>>>> value does, but it seems that a "just use hw default" flag won't cut it
>>>> for this scenario.
>>>>
>>
>> This value is used for the number of clock cycles it would wait before the
>> GDSCR ACK signals/halting the clock.
>>
> 
> That makes sense.
> 
>>>
>>> I'd prefer we invert the logic so that we don't need to litter this flag
>>> all over the place. I recall that the wait values were incorrect a long
>>> time ago on early gdsc using designs but hopefully they've been fixed
>>> now and we can simply use the default power on reset (POR) values.
>>
>> I am okay to invert the logic, but I am not sure if they could cause any
>> issues to the older targets. They were broken in HW design long back, but
>> got fixed in most families of target and most GDSCs.
>>
> 
> I don't fancy us having a flag with the purpose of "don't set the
> timings to 2, 8 and 2" and then rely on open coded writes in probe to
> set it to something else where needed.
> 
> So I definitely would prefer to flip this around, to make the cases
> where we want to write different values explicit.
> 
> But as you say, unless we make sure that all existing platforms do write
> 2, 8 and 2 we risk introducing regressions from the current behavior.
> 
>> As mentioned if explicitly they need to be updated, it is best to do from
>> the probe.
>> This was done in SC7180 GPUCC driver.
>>          /* Configure clk_dis_wait for gpu_cx_gdsc */
>>          regmap_update_bits(regmap, 0x106c, CLK_DIS_WAIT_MASK,
>>                                          8 << CLK_DIS_WAIT_SHIFT);
> 
> But we call regmap_update_bits() from probe, which sets the CLK_DIS_WAIT
> to 8, then we call qcom_cc_really_probe() which will end up in
> gdsc_init() which replaces that with a 2.
> 
> Perhaps I'm missing something?
> 

It was my miss when I did a cleanup to move the DIS_WAIT_VAL before 
registering the clocks.

>>
>>
>> Please let me know if we are okay to add the invert logic.
>>
> 
> I'm still favoring a scheme where we add 3 integers to struct gdsc and
> in gdsc_init() we check if they are non-zero we write the value to the
> register.
> 
Sure, will update the gdsc_init() to default "2, 8, 2" in case of 
non-zero value.

> Although being a big patch, we could maintain the existing behaviour by
> giving all existing struct gdsc definitions the values 2, 8 and 2 to
> avoid regressions and we (everyone) can then go through the platforms
> one by one and remove the unnecessary assignments - but more
> importantly, double check with downstream if they need a different
> value.
> 

Sure, that would help.

> This will also have the side effect going forward that it will be more
> explicit and people writing gdsc definitions should double check these
> values.
> 
> Regards,
> Bjorn

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

--