[PATCH 1/2] clk: qcom: mmcc-msm8974: use clk_rcg2_shared_ops for mdp_clk_src clock

[PATCH 1/2] clk: qcom: mmcc-msm8974: use clk_rcg2_shared_ops for mdp_clk_src clock

[Dmitry Baryshkov]

The mdp_clk_src clock should not be turned off. Instead it should be
'parked' to the XO, as most of other mdp_clk_src clocks. Fix that by
using the clk_rcg2_shared_ops.

Fixes: d8b212014e69 ("clk: qcom: Add support for MSM8974's multimedia clock controller (MMCC)")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/mmcc-msm8974.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/mmcc-msm8974.c b/drivers/clk/qcom/mmcc-msm8974.c
index 4273fce9a4a4..aa29c79fcd55 100644
--- a/drivers/clk/qcom/mmcc-msm8974.c
+++ b/drivers/clk/qcom/mmcc-msm8974.c
@@ -485,7 +485,7 @@ static struct clk_rcg2 mdp_clk_src = {
 		.name = "mdp_clk_src",
 		.parent_data = mmcc_xo_mmpll0_dsi_hdmi_gpll0,
 		.num_parents = ARRAY_SIZE(mmcc_xo_mmpll0_dsi_hdmi_gpll0),
-		.ops = &clk_rcg2_ops,
+		.ops = &clk_rcg2_shared_ops,
 	},
 };
 
-- 
2.39.2

[PATCH 2/2] clk: qcom: mmcc-msm8974: fix MDSS_GDSC power flags

[Dmitry Baryshkov]

Using PWRSTS_RET on msm8974's MDSS_GDSC causes display to stop working.
The gdsc doesn't fully come out of retention mode. Change it's pwrsts
flags to PWRSTS_OFF_ON.

Fixes: d399723950c4 ("clk: qcom: gdsc: Fix the handling of PWRSTS_RET support")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/mmcc-msm8974.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/mmcc-msm8974.c b/drivers/clk/qcom/mmcc-msm8974.c
index aa29c79fcd55..277ef0065aae 100644
--- a/drivers/clk/qcom/mmcc-msm8974.c
+++ b/drivers/clk/qcom/mmcc-msm8974.c
@@ -2401,7 +2401,7 @@ static struct gdsc mdss_gdsc = {
 	.pd = {
 		.name = "mdss",
 	},
-	.pwrsts = PWRSTS_RET_ON,
+	.pwrsts = PWRSTS_OFF_ON,
 };
 
 static struct gdsc camss_jpeg_gdsc = {
-- 
2.39.2

Re: [PATCH 2/2] clk: qcom: mmcc-msm8974: fix MDSS_GDSC power flags

[Konrad Dybcio]

On 7.05.2023 19:53, Dmitry Baryshkov wrote:
> Using PWRSTS_RET on msm8974's MDSS_GDSC causes display to stop working.
> The gdsc doesn't fully come out of retention mode. Change it's pwrsts
> flags to PWRSTS_OFF_ON.
> 
> Fixes: d399723950c4 ("clk: qcom: gdsc: Fix the handling of PWRSTS_RET support")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
This is a stopgap solution, not exactly a fix, all signs on Heaven and
Earth say that 8974 should support retention on this GDSC!

*although*

pre-v2.2 chips don't

Konrad
>  drivers/clk/qcom/mmcc-msm8974.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/mmcc-msm8974.c b/drivers/clk/qcom/mmcc-msm8974.c
> index aa29c79fcd55..277ef0065aae 100644
> --- a/drivers/clk/qcom/mmcc-msm8974.c
> +++ b/drivers/clk/qcom/mmcc-msm8974.c
> @@ -2401,7 +2401,7 @@ static struct gdsc mdss_gdsc = {
>  	.pd = {
>  		.name = "mdss",
>  	},
> -	.pwrsts = PWRSTS_RET_ON,
> +	.pwrsts = PWRSTS_OFF_ON,
>  };
>  
>  static struct gdsc camss_jpeg_gdsc = {

Re: [PATCH 2/2] clk: qcom: mmcc-msm8974: fix MDSS_GDSC power flags

[Dmitry Baryshkov]

On 08/05/2023 11:27, Konrad Dybcio wrote:
> 
> 
> On 7.05.2023 19:53, Dmitry Baryshkov wrote:
>> Using PWRSTS_RET on msm8974's MDSS_GDSC causes display to stop working.
>> The gdsc doesn't fully come out of retention mode. Change it's pwrsts
>> flags to PWRSTS_OFF_ON.
>>
>> Fixes: d399723950c4 ("clk: qcom: gdsc: Fix the handling of PWRSTS_RET support")
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
> This is a stopgap solution, not exactly a fix, all signs on Heaven and
> Earth say that 8974 should support retention on this GDSC!
> 
> *although*
> 
> pre-v2.2 chips don't

Mine is apq8074 v2.2, if I'm not mistaken.

> 
> Konrad
>>   drivers/clk/qcom/mmcc-msm8974.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/qcom/mmcc-msm8974.c b/drivers/clk/qcom/mmcc-msm8974.c
>> index aa29c79fcd55..277ef0065aae 100644
>> --- a/drivers/clk/qcom/mmcc-msm8974.c
>> +++ b/drivers/clk/qcom/mmcc-msm8974.c
>> @@ -2401,7 +2401,7 @@ static struct gdsc mdss_gdsc = {
>>   	.pd = {
>>   		.name = "mdss",
>>   	},
>> -	.pwrsts = PWRSTS_RET_ON,
>> +	.pwrsts = PWRSTS_OFF_ON,
>>   };
>>   
>>   static struct gdsc camss_jpeg_gdsc = {

-- 
With best wishes
Dmitry

Re: [PATCH 2/2] clk: qcom: mmcc-msm8974: fix MDSS_GDSC power flags

[Luca Weiss]

On Sonntag, 7. Mai 2023 19:53:35 CEST Dmitry Baryshkov wrote:
> Using PWRSTS_RET on msm8974's MDSS_GDSC causes display to stop working.
> The gdsc doesn't fully come out of retention mode. Change it's pwrsts
> flags to PWRSTS_OFF_ON.

+CC Rajendra & Manivannan

Following the mails at [0] this was the suggested fix, which I haven't
made into a patch myself yet.

With this (and without the revert of the linked patch) panel init is fine
again on FP2. Thanks!

[0] https://lore.kernel.org/linux-arm-msm/5897497.lOV4Wx5bFT@g550jk/

Tested-by: Luca Weiss <luca@z3ntu.xyz>

> 
> Fixes: d399723950c4 ("clk: qcom: gdsc: Fix the handling of PWRSTS_RET
> support") Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/clk/qcom/mmcc-msm8974.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/mmcc-msm8974.c
> b/drivers/clk/qcom/mmcc-msm8974.c index aa29c79fcd55..277ef0065aae 100644
> --- a/drivers/clk/qcom/mmcc-msm8974.c
> +++ b/drivers/clk/qcom/mmcc-msm8974.c
> @@ -2401,7 +2401,7 @@ static struct gdsc mdss_gdsc = {
>  	.pd = {
>  		.name = "mdss",
>  	},
> -	.pwrsts = PWRSTS_RET_ON,
> +	.pwrsts = PWRSTS_OFF_ON,
>  };
> 
>  static struct gdsc camss_jpeg_gdsc = {

Re: [PATCH 2/2] clk: qcom: mmcc-msm8974: fix MDSS_GDSC power flags

[Rajendra Nayak]

On 5/9/2023 2:17 AM, Luca Weiss wrote:
> On Sonntag, 7. Mai 2023 19:53:35 CEST Dmitry Baryshkov wrote:
>> Using PWRSTS_RET on msm8974's MDSS_GDSC causes display to stop working.
>> The gdsc doesn't fully come out of retention mode. Change it's pwrsts
>> flags to PWRSTS_OFF_ON.
> 
> +CC Rajendra & Manivannan
> 
> Following the mails at [0] this was the suggested fix, which I haven't
> made into a patch myself yet.
> 
> With this (and without the revert of the linked patch) panel init is fine
> again on FP2. Thanks!
> 
> [0] https://lore.kernel.org/linux-arm-msm/5897497.lOV4Wx5bFT@g550jk/
> 
> Tested-by: Luca Weiss <luca@z3ntu.xyz>

Reviewed-by: Rajendra Nayak <quic_rjendra@quicinc.com>

> 
>>
>> Fixes: d399723950c4 ("clk: qcom: gdsc: Fix the handling of PWRSTS_RET
>> support") Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/clk/qcom/mmcc-msm8974.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/qcom/mmcc-msm8974.c
>> b/drivers/clk/qcom/mmcc-msm8974.c index aa29c79fcd55..277ef0065aae 100644
>> --- a/drivers/clk/qcom/mmcc-msm8974.c
>> +++ b/drivers/clk/qcom/mmcc-msm8974.c
>> @@ -2401,7 +2401,7 @@ static struct gdsc mdss_gdsc = {
>>   	.pd = {
>>   		.name = "mdss",
>>   	},
>> -	.pwrsts = PWRSTS_RET_ON,
>> +	.pwrsts = PWRSTS_OFF_ON,
>>   };
>>
>>   static struct gdsc camss_jpeg_gdsc = {
> 
> 
> 
> 

Re: [PATCH 2/2] clk: qcom: mmcc-msm8974: fix MDSS_GDSC power flags

[Manivannan Sadhasivam]

On Sun, May 07, 2023 at 08:53:35PM +0300, Dmitry Baryshkov wrote:
> Using PWRSTS_RET on msm8974's MDSS_GDSC causes display to stop working.
> The gdsc doesn't fully come out of retention mode. Change it's pwrsts
> flags to PWRSTS_OFF_ON.
> 

What does "stop working" implies? Does it work during boot and randomly stopped
working or it stopped working after resume from suspend?

Even though reverting to non-retention mode works, I think the issue might be
somewhere else. Like the vote might be missing to get the GDSC out of retention
mode.

- Mani

> Fixes: d399723950c4 ("clk: qcom: gdsc: Fix the handling of PWRSTS_RET support")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/clk/qcom/mmcc-msm8974.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/mmcc-msm8974.c b/drivers/clk/qcom/mmcc-msm8974.c
> index aa29c79fcd55..277ef0065aae 100644
> --- a/drivers/clk/qcom/mmcc-msm8974.c
> +++ b/drivers/clk/qcom/mmcc-msm8974.c
> @@ -2401,7 +2401,7 @@ static struct gdsc mdss_gdsc = {
>  	.pd = {
>  		.name = "mdss",
>  	},
> -	.pwrsts = PWRSTS_RET_ON,
> +	.pwrsts = PWRSTS_OFF_ON,
>  };
>  
>  static struct gdsc camss_jpeg_gdsc = {
> -- 
> 2.39.2
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 2/2] clk: qcom: mmcc-msm8974: fix MDSS_GDSC power flags

[Dmitry Baryshkov]

On Tue, 9 May 2023 at 08:50, Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Sun, May 07, 2023 at 08:53:35PM +0300, Dmitry Baryshkov wrote:
> > Using PWRSTS_RET on msm8974's MDSS_GDSC causes display to stop working.
> > The gdsc doesn't fully come out of retention mode. Change it's pwrsts
> > flags to PWRSTS_OFF_ON.
> >
>
> What does "stop working" implies? Does it work during boot and randomly stopped
> working or it stopped working after resume from suspend?

It stops working during the boot. I observed the MDP not starting up
properly. Mea culpa, I did not look deep enough into the details, just
stomped upon this change which fixes the problem for me.

>
> Even though reverting to non-retention mode works, I think the issue might be
> somewhere else. Like the vote might be missing to get the GDSC out of retention
> mode.

I don't think there is a vote missing. The driver votes on MDSS_GDSC
before enabling access to any of the registers from the MDSS region.

>
> - Mani
>
> > Fixes: d399723950c4 ("clk: qcom: gdsc: Fix the handling of PWRSTS_RET support")
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  drivers/clk/qcom/mmcc-msm8974.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/clk/qcom/mmcc-msm8974.c b/drivers/clk/qcom/mmcc-msm8974.c
> > index aa29c79fcd55..277ef0065aae 100644
> > --- a/drivers/clk/qcom/mmcc-msm8974.c
> > +++ b/drivers/clk/qcom/mmcc-msm8974.c
> > @@ -2401,7 +2401,7 @@ static struct gdsc mdss_gdsc = {
> >       .pd = {
> >               .name = "mdss",
> >       },
> > -     .pwrsts = PWRSTS_RET_ON,
> > +     .pwrsts = PWRSTS_OFF_ON,
> >  };
> >
> >  static struct gdsc camss_jpeg_gdsc = {
> > --
> > 2.39.2
> >
>
> --
> மணிவண்ணன் சதாசிவம்



-- 
With best wishes
Dmitry

Re: [PATCH 2/2] clk: qcom: mmcc-msm8974: fix MDSS_GDSC power flags

[Manivannan Sadhasivam]

On Tue, May 09, 2023 at 02:20:07PM +0300, Dmitry Baryshkov wrote:
> On Tue, 9 May 2023 at 08:50, Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > On Sun, May 07, 2023 at 08:53:35PM +0300, Dmitry Baryshkov wrote:
> > > Using PWRSTS_RET on msm8974's MDSS_GDSC causes display to stop working.
> > > The gdsc doesn't fully come out of retention mode. Change it's pwrsts
> > > flags to PWRSTS_OFF_ON.
> > >
> >
> > What does "stop working" implies? Does it work during boot and randomly stopped
> > working or it stopped working after resume from suspend?
> 
> It stops working during the boot. I observed the MDP not starting up
> properly. Mea culpa, I did not look deep enough into the details, just
> stomped upon this change which fixes the problem for me.
> 

IIUC, GDSC will be transitioned to retention mode only if the parent domain goes
to low power mode. So if the MDSS GDSC goes to retention mode during boot, then
it suggests that the parent domain is not voted properly. Unless I misunderstood
something...

Or is the GDSC behavior changes between RPM and RPMh?

- Mani

> >
> > Even though reverting to non-retention mode works, I think the issue might be
> > somewhere else. Like the vote might be missing to get the GDSC out of retention
> > mode.
> 
> I don't think there is a vote missing. The driver votes on MDSS_GDSC
> before enabling access to any of the registers from the MDSS region.
> 
> >
> > - Mani
> >
> > > Fixes: d399723950c4 ("clk: qcom: gdsc: Fix the handling of PWRSTS_RET support")
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > ---
> > >  drivers/clk/qcom/mmcc-msm8974.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/clk/qcom/mmcc-msm8974.c b/drivers/clk/qcom/mmcc-msm8974.c
> > > index aa29c79fcd55..277ef0065aae 100644
> > > --- a/drivers/clk/qcom/mmcc-msm8974.c
> > > +++ b/drivers/clk/qcom/mmcc-msm8974.c
> > > @@ -2401,7 +2401,7 @@ static struct gdsc mdss_gdsc = {
> > >       .pd = {
> > >               .name = "mdss",
> > >       },
> > > -     .pwrsts = PWRSTS_RET_ON,
> > > +     .pwrsts = PWRSTS_OFF_ON,
> > >  };
> > >
> > >  static struct gdsc camss_jpeg_gdsc = {
> > > --
> > > 2.39.2
> > >
> >
> > --
> > மணிவண்ணன் சதாசிவம்
> 
> 
> 
> -- 
> With best wishes
> Dmitry

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 2/2] clk: qcom: mmcc-msm8974: fix MDSS_GDSC power flags

[Dmitry Baryshkov]

On 11/05/2023 11:15, Manivannan Sadhasivam wrote:
> On Tue, May 09, 2023 at 02:20:07PM +0300, Dmitry Baryshkov wrote:
>> On Tue, 9 May 2023 at 08:50, Manivannan Sadhasivam
>> <manivannan.sadhasivam@linaro.org> wrote:
>>>
>>> On Sun, May 07, 2023 at 08:53:35PM +0300, Dmitry Baryshkov wrote:
>>>> Using PWRSTS_RET on msm8974's MDSS_GDSC causes display to stop working.
>>>> The gdsc doesn't fully come out of retention mode. Change it's pwrsts
>>>> flags to PWRSTS_OFF_ON.
>>>>
>>>
>>> What does "stop working" implies? Does it work during boot and randomly stopped
>>> working or it stopped working after resume from suspend?
>>
>> It stops working during the boot. I observed the MDP not starting up
>> properly. Mea culpa, I did not look deep enough into the details, just
>> stomped upon this change which fixes the problem for me.
>>
> 
> IIUC, GDSC will be transitioned to retention mode only if the parent domain goes
> to low power mode. So if the MDSS GDSC goes to retention mode during boot, then
> it suggests that the parent domain is not voted properly. Unless I misunderstood
> something...

Not sure, what is the parent domain here. Note, it is a pretty old 
implementation.

> 
> Or is the GDSC behavior changes between RPM and RPMh?
> 
> - Mani
> 
>>>
>>> Even though reverting to non-retention mode works, I think the issue might be
>>> somewhere else. Like the vote might be missing to get the GDSC out of retention
>>> mode.
>>
>> I don't think there is a vote missing. The driver votes on MDSS_GDSC
>> before enabling access to any of the registers from the MDSS region.
>>
>>>
>>> - Mani
>>>
>>>> Fixes: d399723950c4 ("clk: qcom: gdsc: Fix the handling of PWRSTS_RET support")
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> ---
>>>>   drivers/clk/qcom/mmcc-msm8974.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/clk/qcom/mmcc-msm8974.c b/drivers/clk/qcom/mmcc-msm8974.c
>>>> index aa29c79fcd55..277ef0065aae 100644
>>>> --- a/drivers/clk/qcom/mmcc-msm8974.c
>>>> +++ b/drivers/clk/qcom/mmcc-msm8974.c
>>>> @@ -2401,7 +2401,7 @@ static struct gdsc mdss_gdsc = {
>>>>        .pd = {
>>>>                .name = "mdss",
>>>>        },
>>>> -     .pwrsts = PWRSTS_RET_ON,
>>>> +     .pwrsts = PWRSTS_OFF_ON,
>>>>   };
>>>>
>>>>   static struct gdsc camss_jpeg_gdsc = {
>>>> --
>>>> 2.39.2
>>>>
>>>
>>> --
>>> மணிவண்ணன் சதாசிவம்
>>
>>
>>
>> -- 
>> With best wishes
>> Dmitry
> 

-- 
With best wishes
Dmitry

Re: [PATCH 2/2] clk: qcom: mmcc-msm8974: fix MDSS_GDSC power flags

[Dmitry Baryshkov]

On Tue, 30 May 2023 at 02:47, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On 11/05/2023 11:15, Manivannan Sadhasivam wrote:
> > On Tue, May 09, 2023 at 02:20:07PM +0300, Dmitry Baryshkov wrote:
> >> On Tue, 9 May 2023 at 08:50, Manivannan Sadhasivam
> >> <manivannan.sadhasivam@linaro.org> wrote:
> >>>
> >>> On Sun, May 07, 2023 at 08:53:35PM +0300, Dmitry Baryshkov wrote:
> >>>> Using PWRSTS_RET on msm8974's MDSS_GDSC causes display to stop working.
> >>>> The gdsc doesn't fully come out of retention mode. Change it's pwrsts
> >>>> flags to PWRSTS_OFF_ON.
> >>>>
> >>>
> >>> What does "stop working" implies? Does it work during boot and randomly stopped
> >>> working or it stopped working after resume from suspend?
> >>
> >> It stops working during the boot. I observed the MDP not starting up
> >> properly. Mea culpa, I did not look deep enough into the details, just
> >> stomped upon this change which fixes the problem for me.
> >>
> >
> > IIUC, GDSC will be transitioned to retention mode only if the parent domain goes
> > to low power mode. So if the MDSS GDSC goes to retention mode during boot, then
> > it suggests that the parent domain is not voted properly. Unless I misunderstood
> > something...
>
> Not sure, what is the parent domain here. Note, it is a pretty old
> implementation.

Colleagues, any further feedback on this? I'd like to note that all
other platforms use PWRSTS_OFF_ON for the MDSS_GDSC,
I do not think that msm8974 is particularly different here.

>
> >
> > Or is the GDSC behavior changes between RPM and RPMh?
> >
> > - Mani
> >
> >>>
> >>> Even though reverting to non-retention mode works, I think the issue might be
> >>> somewhere else. Like the vote might be missing to get the GDSC out of retention
> >>> mode.
> >>
> >> I don't think there is a vote missing. The driver votes on MDSS_GDSC
> >> before enabling access to any of the registers from the MDSS region.
> >>
> >>>
> >>> - Mani
> >>>
> >>>> Fixes: d399723950c4 ("clk: qcom: gdsc: Fix the handling of PWRSTS_RET support")
> >>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>>> ---
> >>>>   drivers/clk/qcom/mmcc-msm8974.c | 2 +-
> >>>>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/clk/qcom/mmcc-msm8974.c b/drivers/clk/qcom/mmcc-msm8974.c
> >>>> index aa29c79fcd55..277ef0065aae 100644
> >>>> --- a/drivers/clk/qcom/mmcc-msm8974.c
> >>>> +++ b/drivers/clk/qcom/mmcc-msm8974.c
> >>>> @@ -2401,7 +2401,7 @@ static struct gdsc mdss_gdsc = {
> >>>>        .pd = {
> >>>>                .name = "mdss",
> >>>>        },
> >>>> -     .pwrsts = PWRSTS_RET_ON,
> >>>> +     .pwrsts = PWRSTS_OFF_ON,
> >>>>   };
> >>>>
> >>>>   static struct gdsc camss_jpeg_gdsc = {
> >>>> --
> >>>> 2.39.2
> >>>>
> >>>
> >>> --
> >>> மணிவண்ணன் சதாசிவம்
> >>
> >>
> >>
> >> --
> >> With best wishes
> >> Dmitry
> >
>
> --
> With best wishes
> Dmitry
>


-- 
With best wishes
Dmitry

Re: [PATCH 2/2] clk: qcom: mmcc-msm8974: fix MDSS_GDSC power flags

[Konrad Dybcio]

On 14.06.2023 08:05, Dmitry Baryshkov wrote:
> On Tue, 30 May 2023 at 02:47, Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
>>
>> On 11/05/2023 11:15, Manivannan Sadhasivam wrote:
>>> On Tue, May 09, 2023 at 02:20:07PM +0300, Dmitry Baryshkov wrote:
>>>> On Tue, 9 May 2023 at 08:50, Manivannan Sadhasivam
>>>> <manivannan.sadhasivam@linaro.org> wrote:
>>>>>
>>>>> On Sun, May 07, 2023 at 08:53:35PM +0300, Dmitry Baryshkov wrote:
>>>>>> Using PWRSTS_RET on msm8974's MDSS_GDSC causes display to stop working.
>>>>>> The gdsc doesn't fully come out of retention mode. Change it's pwrsts
>>>>>> flags to PWRSTS_OFF_ON.
>>>>>>
>>>>>
>>>>> What does "stop working" implies? Does it work during boot and randomly stopped
>>>>> working or it stopped working after resume from suspend?
>>>>
>>>> It stops working during the boot. I observed the MDP not starting up
>>>> properly. Mea culpa, I did not look deep enough into the details, just
>>>> stomped upon this change which fixes the problem for me.
>>>>
>>>
>>> IIUC, GDSC will be transitioned to retention mode only if the parent domain goes
>>> to low power mode. So if the MDSS GDSC goes to retention mode during boot, then
>>> it suggests that the parent domain is not voted properly. Unless I misunderstood
>>> something...
>>
>> Not sure, what is the parent domain here. Note, it is a pretty old
>> implementation.
> 
> Colleagues, any further feedback on this? I'd like to note that all
> other platforms use PWRSTS_OFF_ON for the MDSS_GDSC,
> I do not think that msm8974 is particularly different here.
I'd say it's ok for now.

Konrad
> 
>>
>>>
>>> Or is the GDSC behavior changes between RPM and RPMh?
>>>
>>> - Mani
>>>
>>>>>
>>>>> Even though reverting to non-retention mode works, I think the issue might be
>>>>> somewhere else. Like the vote might be missing to get the GDSC out of retention
>>>>> mode.
>>>>
>>>> I don't think there is a vote missing. The driver votes on MDSS_GDSC
>>>> before enabling access to any of the registers from the MDSS region.
>>>>
>>>>>
>>>>> - Mani
>>>>>
>>>>>> Fixes: d399723950c4 ("clk: qcom: gdsc: Fix the handling of PWRSTS_RET support")
>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>> ---
>>>>>>   drivers/clk/qcom/mmcc-msm8974.c | 2 +-
>>>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/clk/qcom/mmcc-msm8974.c b/drivers/clk/qcom/mmcc-msm8974.c
>>>>>> index aa29c79fcd55..277ef0065aae 100644
>>>>>> --- a/drivers/clk/qcom/mmcc-msm8974.c
>>>>>> +++ b/drivers/clk/qcom/mmcc-msm8974.c
>>>>>> @@ -2401,7 +2401,7 @@ static struct gdsc mdss_gdsc = {
>>>>>>        .pd = {
>>>>>>                .name = "mdss",
>>>>>>        },
>>>>>> -     .pwrsts = PWRSTS_RET_ON,
>>>>>> +     .pwrsts = PWRSTS_OFF_ON,
>>>>>>   };
>>>>>>
>>>>>>   static struct gdsc camss_jpeg_gdsc = {
>>>>>> --
>>>>>> 2.39.2
>>>>>>
>>>>>
>>>>> --
>>>>> மணிவண்ணன் சதாசிவம்
>>>>
>>>>
>>>>
>>>> --
>>>> With best wishes
>>>> Dmitry
>>>
>>
>> --
>> With best wishes
>> Dmitry
>>
> 
> 

Re: [PATCH 1/2] clk: qcom: mmcc-msm8974: use clk_rcg2_shared_ops for mdp_clk_src clock

[Konrad Dybcio]

On 7.05.2023 19:53, Dmitry Baryshkov wrote:
> The mdp_clk_src clock should not be turned off. Instead it should be
> 'parked' to the XO, as most of other mdp_clk_src clocks. Fix that by
> using the clk_rcg2_shared_ops.
> 
> Fixes: d8b212014e69 ("clk: qcom: Add support for MSM8974's multimedia clock controller (MMCC)")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
I am inclined to think it's true, however I can't see any evidence
of this being done on the vendor kernel.. I am however not a licensed
archaeologist so I might have missed it.. Hence I'll do:

Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
>  drivers/clk/qcom/mmcc-msm8974.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/mmcc-msm8974.c b/drivers/clk/qcom/mmcc-msm8974.c
> index 4273fce9a4a4..aa29c79fcd55 100644
> --- a/drivers/clk/qcom/mmcc-msm8974.c
> +++ b/drivers/clk/qcom/mmcc-msm8974.c
> @@ -485,7 +485,7 @@ static struct clk_rcg2 mdp_clk_src = {
>  		.name = "mdp_clk_src",
>  		.parent_data = mmcc_xo_mmpll0_dsi_hdmi_gpll0,
>  		.num_parents = ARRAY_SIZE(mmcc_xo_mmpll0_dsi_hdmi_gpll0),
> -		.ops = &clk_rcg2_ops,
> +		.ops = &clk_rcg2_shared_ops,
>  	},
>  };
>  

Re: [PATCH 1/2] clk: qcom: mmcc-msm8974: use clk_rcg2_shared_ops for mdp_clk_src clock

[Dmitry Baryshkov]

On 08/05/2023 10:55, Konrad Dybcio wrote:
> 
> 
> On 7.05.2023 19:53, Dmitry Baryshkov wrote:
>> The mdp_clk_src clock should not be turned off. Instead it should be
>> 'parked' to the XO, as most of other mdp_clk_src clocks. Fix that by
>> using the clk_rcg2_shared_ops.
>>
>> Fixes: d8b212014e69 ("clk: qcom: Add support for MSM8974's multimedia clock controller (MMCC)")
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
> I am inclined to think it's true, however I can't see any evidence
> of this being done on the vendor kernel.. I am however not a licensed
> archaeologist so I might have missed it.. Hence I'll do:

I have been observing the 'clock stuck at off' without this change. The 
warning goes away with this patch. Thus I suppose it is correct.

> 
> Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> 
> Konrad
>>   drivers/clk/qcom/mmcc-msm8974.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/qcom/mmcc-msm8974.c b/drivers/clk/qcom/mmcc-msm8974.c
>> index 4273fce9a4a4..aa29c79fcd55 100644
>> --- a/drivers/clk/qcom/mmcc-msm8974.c
>> +++ b/drivers/clk/qcom/mmcc-msm8974.c
>> @@ -485,7 +485,7 @@ static struct clk_rcg2 mdp_clk_src = {
>>   		.name = "mdp_clk_src",
>>   		.parent_data = mmcc_xo_mmpll0_dsi_hdmi_gpll0,
>>   		.num_parents = ARRAY_SIZE(mmcc_xo_mmpll0_dsi_hdmi_gpll0),
>> -		.ops = &clk_rcg2_ops,
>> +		.ops = &clk_rcg2_shared_ops,
>>   	},
>>   };
>>   

-- 
With best wishes
Dmitry

Re: [PATCH 1/2] clk: qcom: mmcc-msm8974: use clk_rcg2_shared_ops for mdp_clk_src clock

[Luca Weiss]

On Sonntag, 7. Mai 2023 19:53:34 CEST Dmitry Baryshkov wrote:
> The mdp_clk_src clock should not be turned off. Instead it should be
> 'parked' to the XO, as most of other mdp_clk_src clocks. Fix that by
> using the clk_rcg2_shared_ops.

While I don't recall seeing problems with this clock before, with this change 
it also seems to be work fine on FP2.

Tested-by: Luca Weiss <luca@z3ntu.xyz>

> 
> Fixes: d8b212014e69 ("clk: qcom: Add support for MSM8974's multimedia clock
> controller (MMCC)") Signed-off-by: Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org>
> ---
>  drivers/clk/qcom/mmcc-msm8974.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/mmcc-msm8974.c
> b/drivers/clk/qcom/mmcc-msm8974.c index 4273fce9a4a4..aa29c79fcd55 100644
> --- a/drivers/clk/qcom/mmcc-msm8974.c
> +++ b/drivers/clk/qcom/mmcc-msm8974.c
> @@ -485,7 +485,7 @@ static struct clk_rcg2 mdp_clk_src = {
>  		.name = "mdp_clk_src",
>  		.parent_data = mmcc_xo_mmpll0_dsi_hdmi_gpll0,
>  		.num_parents = ARRAY_SIZE(mmcc_xo_mmpll0_dsi_hdmi_gpll0),
> -		.ops = &clk_rcg2_ops,
> +		.ops = &clk_rcg2_shared_ops,
>  	},
>  };

Re: (subset) [PATCH 1/2] clk: qcom: mmcc-msm8974: use clk_rcg2_shared_ops for mdp_clk_src clock

[Bjorn Andersson]

On Sun, 7 May 2023 20:53:34 +0300, Dmitry Baryshkov wrote:
> The mdp_clk_src clock should not be turned off. Instead it should be
> 'parked' to the XO, as most of other mdp_clk_src clocks. Fix that by
> using the clk_rcg2_shared_ops.
> 
> 

Applied, thanks!

[1/2] clk: qcom: mmcc-msm8974: use clk_rcg2_shared_ops for mdp_clk_src clock
      commit: 8fd492e77ff71f68f7311c22f7bc960182465cd7

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>

Re: (subset) [PATCH 1/2] clk: qcom: mmcc-msm8974: use clk_rcg2_shared_ops for mdp_clk_src clock

[Bjorn Andersson]

On Sun, 07 May 2023 20:53:34 +0300, Dmitry Baryshkov wrote:
> The mdp_clk_src clock should not be turned off. Instead it should be
> 'parked' to the XO, as most of other mdp_clk_src clocks. Fix that by
> using the clk_rcg2_shared_ops.
> 
> 

Applied, thanks!

[2/2] clk: qcom: mmcc-msm8974: fix MDSS_GDSC power flags
      commit: 4e13c7a55cf752887f2b8d8008711dbbc64ea796

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>