[PATCH] clk: qcom: mmcc-msm8974: Add OXILICX_GDSC for msm8226

[PATCH] clk: qcom: mmcc-msm8974: Add OXILICX_GDSC for msm8226

[Luca Weiss]

On msm8226 we also have OXILICX_GDSC but we need a slighly different
config, with a .cxcs defined for clock but with no parent.

Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
---
 drivers/clk/qcom/mmcc-msm8974.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/clk/qcom/mmcc-msm8974.c b/drivers/clk/qcom/mmcc-msm8974.c
index 4273fce9a4a4..39ee3953567c 100644
--- a/drivers/clk/qcom/mmcc-msm8974.c
+++ b/drivers/clk/qcom/mmcc-msm8974.c
@@ -2443,6 +2443,16 @@ static struct gdsc oxilicx_gdsc = {
 	.pwrsts = PWRSTS_OFF_ON,
 };
 
+static struct gdsc oxilicx_gdsc_msm8226 = {
+	.gdscr = 0x4034,
+	.cxcs = (unsigned int []){ 0x4028 },
+	.cxc_count = 1,
+	.pd = {
+		.name = "oxilicx",
+	},
+	.pwrsts = PWRSTS_OFF_ON,
+};
+
 static struct clk_regmap *mmcc_msm8226_clocks[] = {
 	[MMSS_AHB_CLK_SRC] = &mmss_ahb_clk_src.clkr,
 	[MMSS_AXI_CLK_SRC] = &mmss_axi_clk_src.clkr,
@@ -2533,6 +2543,7 @@ static struct gdsc *mmcc_msm8226_gdscs[] = {
 	[MDSS_GDSC] = &mdss_gdsc,
 	[CAMSS_JPEG_GDSC] = &camss_jpeg_gdsc,
 	[CAMSS_VFE_GDSC] = &camss_vfe_gdsc,
+	[OXILICX_GDSC] = &oxilicx_gdsc_msm8226,
 };
 
 static const struct regmap_config mmcc_msm8226_regmap_config = {

---
base-commit: dd9e11d6477a52ede9ebe575c83285e79e823889
change-id: 20230506-msm8226-oxilicx-7f3f0f8e491d

Best regards,
-- 
Luca Weiss <luca@z3ntu.xyz>

Re: [PATCH] clk: qcom: mmcc-msm8974: Add OXILICX_GDSC for msm8226

[Konrad Dybcio]

On 6.05.2023 23:20, Luca Weiss wrote:
> On msm8226 we also have OXILICX_GDSC but we need a slighly different
> config, with a .cxcs defined for clock but with no parent.
Hm, on newer (a5xx+) GPUs, CX needs to be turned on first and
to achieve that, we sometimes define it to be the GX's (also
implicitly known as "oxili-non-CX" in before-a6xx-times) parent..

Roughly speaking CX powers the "GPU hardware owned by the broader
SoC that may not need the GPU core clock to be up" and GX powers
the "GPU hardware owned strictly by the GPU that needs at least some
GPU clocks to be enabled"

Maybe 8974 simply has a bug in the driver that would do the reverse?
Could you (and perhaps Dmitry on his shiny new 13yo board) test that
theory, preferably on both SoCs?

--- a/drivers/clk/qcom/mmcc-msm8974.c
+++ b/drivers/clk/qcom/mmcc-msm8974.c
@@ -2431,6 +2431,7 @@ static struct gdsc oxili_gdsc = {
        .pd = {
                .name = "oxili",
        },
+       .parent = &oxili_gdsc.pd,
        .pwrsts = PWRSTS_OFF_ON,
 };
 
@@ -2439,7 +2440,6 @@ static struct gdsc oxilicx_gdsc = {
        .pd = {
                .name = "oxilicx",
        },
-       .parent = &oxili_gdsc.pd,
        .pwrsts = PWRSTS_OFF_ON,
 };

Konrad
> 
> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> ---
>  drivers/clk/qcom/mmcc-msm8974.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/clk/qcom/mmcc-msm8974.c b/drivers/clk/qcom/mmcc-msm8974.c
> index 4273fce9a4a4..39ee3953567c 100644
> --- a/drivers/clk/qcom/mmcc-msm8974.c
> +++ b/drivers/clk/qcom/mmcc-msm8974.c
> @@ -2443,6 +2443,16 @@ static struct gdsc oxilicx_gdsc = {
>  	.pwrsts = PWRSTS_OFF_ON,
>  };
>  
> +static struct gdsc oxilicx_gdsc_msm8226 = {
> +	.gdscr = 0x4034,
> +	.cxcs = (unsigned int []){ 0x4028 },
> +	.cxc_count = 1,
> +	.pd = {
> +		.name = "oxilicx",
> +	},
> +	.pwrsts = PWRSTS_OFF_ON,
> +};
> +
>  static struct clk_regmap *mmcc_msm8226_clocks[] = {
>  	[MMSS_AHB_CLK_SRC] = &mmss_ahb_clk_src.clkr,
>  	[MMSS_AXI_CLK_SRC] = &mmss_axi_clk_src.clkr,
> @@ -2533,6 +2543,7 @@ static struct gdsc *mmcc_msm8226_gdscs[] = {
>  	[MDSS_GDSC] = &mdss_gdsc,
>  	[CAMSS_JPEG_GDSC] = &camss_jpeg_gdsc,
>  	[CAMSS_VFE_GDSC] = &camss_vfe_gdsc,
> +	[OXILICX_GDSC] = &oxilicx_gdsc_msm8226,
>  };
>  
>  static const struct regmap_config mmcc_msm8226_regmap_config = {
> 
> ---
> base-commit: dd9e11d6477a52ede9ebe575c83285e79e823889
> change-id: 20230506-msm8226-oxilicx-7f3f0f8e491d
> 
> Best regards,

Re: [PATCH] clk: qcom: mmcc-msm8974: Add OXILICX_GDSC for msm8226

[Dmitry Baryshkov]

On 08/05/2023 10:23, Konrad Dybcio wrote:
> 
> 
> On 6.05.2023 23:20, Luca Weiss wrote:
>> On msm8226 we also have OXILICX_GDSC but we need a slighly different
>> config, with a .cxcs defined for clock but with no parent.
> Hm, on newer (a5xx+) GPUs, CX needs to be turned on first and
> to achieve that, we sometimes define it to be the GX's (also
> implicitly known as "oxili-non-CX" in before-a6xx-times) parent..
> 
> Roughly speaking CX powers the "GPU hardware owned by the broader
> SoC that may not need the GPU core clock to be up" and GX powers
> the "GPU hardware owned strictly by the GPU that needs at least some
> GPU clocks to be enabled"
> 
> Maybe 8974 simply has a bug in the driver that would do the reverse?
> Could you (and perhaps Dmitry on his shiny new 13yo board) test that
> theory, preferably on both SoCs?
> 
> --- a/drivers/clk/qcom/mmcc-msm8974.c
> +++ b/drivers/clk/qcom/mmcc-msm8974.c
> @@ -2431,6 +2431,7 @@ static struct gdsc oxili_gdsc = {
>          .pd = {
>                  .name = "oxili",
>          },
> +       .parent = &oxili_gdsc.pd,
>          .pwrsts = PWRSTS_OFF_ON,
>   };

Are you declaring oxili_gdsc to be a parent of itself?

>   
> @@ -2439,7 +2440,6 @@ static struct gdsc oxilicx_gdsc = {
>          .pd = {
>                  .name = "oxilicx",
>          },
> -       .parent = &oxili_gdsc.pd,
>          .pwrsts = PWRSTS_OFF_ON,
>   };
> 
> Konrad
>>
>> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
>> ---
>>   drivers/clk/qcom/mmcc-msm8974.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/mmcc-msm8974.c b/drivers/clk/qcom/mmcc-msm8974.c
>> index 4273fce9a4a4..39ee3953567c 100644
>> --- a/drivers/clk/qcom/mmcc-msm8974.c
>> +++ b/drivers/clk/qcom/mmcc-msm8974.c
>> @@ -2443,6 +2443,16 @@ static struct gdsc oxilicx_gdsc = {
>>   	.pwrsts = PWRSTS_OFF_ON,
>>   };
>>   
>> +static struct gdsc oxilicx_gdsc_msm8226 = {
>> +	.gdscr = 0x4034,
>> +	.cxcs = (unsigned int []){ 0x4028 },
>> +	.cxc_count = 1,
>> +	.pd = {
>> +		.name = "oxilicx",
>> +	},
>> +	.pwrsts = PWRSTS_OFF_ON,
>> +};
>> +
>>   static struct clk_regmap *mmcc_msm8226_clocks[] = {
>>   	[MMSS_AHB_CLK_SRC] = &mmss_ahb_clk_src.clkr,
>>   	[MMSS_AXI_CLK_SRC] = &mmss_axi_clk_src.clkr,
>> @@ -2533,6 +2543,7 @@ static struct gdsc *mmcc_msm8226_gdscs[] = {
>>   	[MDSS_GDSC] = &mdss_gdsc,
>>   	[CAMSS_JPEG_GDSC] = &camss_jpeg_gdsc,
>>   	[CAMSS_VFE_GDSC] = &camss_vfe_gdsc,
>> +	[OXILICX_GDSC] = &oxilicx_gdsc_msm8226,
>>   };
>>   
>>   static const struct regmap_config mmcc_msm8226_regmap_config = {
>>
>> ---
>> base-commit: dd9e11d6477a52ede9ebe575c83285e79e823889
>> change-id: 20230506-msm8226-oxilicx-7f3f0f8e491d
>>
>> Best regards,

-- 
With best wishes
Dmitry

Re: [PATCH] clk: qcom: mmcc-msm8974: Add OXILICX_GDSC for msm8226

[Konrad Dybcio]

On 8.05.2023 13:32, Dmitry Baryshkov wrote:
> On 08/05/2023 10:23, Konrad Dybcio wrote:
>>
>>
>> On 6.05.2023 23:20, Luca Weiss wrote:
>>> On msm8226 we also have OXILICX_GDSC but we need a slighly different
>>> config, with a .cxcs defined for clock but with no parent.
>> Hm, on newer (a5xx+) GPUs, CX needs to be turned on first and
>> to achieve that, we sometimes define it to be the GX's (also
>> implicitly known as "oxili-non-CX" in before-a6xx-times) parent..
>>
>> Roughly speaking CX powers the "GPU hardware owned by the broader
>> SoC that may not need the GPU core clock to be up" and GX powers
>> the "GPU hardware owned strictly by the GPU that needs at least some
>> GPU clocks to be enabled"
>>
>> Maybe 8974 simply has a bug in the driver that would do the reverse?
>> Could you (and perhaps Dmitry on his shiny new 13yo board) test that
>> theory, preferably on both SoCs?
>>
>> --- a/drivers/clk/qcom/mmcc-msm8974.c
>> +++ b/drivers/clk/qcom/mmcc-msm8974.c
>> @@ -2431,6 +2431,7 @@ static struct gdsc oxili_gdsc = {
>>          .pd = {
>>                  .name = "oxili",
>>          },
>> +       .parent = &oxili_gdsc.pd,
>>          .pwrsts = PWRSTS_OFF_ON,
>>   };
> 
> Are you declaring oxili_gdsc to be a parent of itself?
lol.. nice catch of course this line should have been 

+       .parent = &oxilicx_gdsc.pd,

and the definitions would need to be swapped

Konrad
> 
>>   @@ -2439,7 +2440,6 @@ static struct gdsc oxilicx_gdsc = {
>>          .pd = {
>>                  .name = "oxilicx",
>>          },
>> -       .parent = &oxili_gdsc.pd,
>>          .pwrsts = PWRSTS_OFF_ON,
>>   };
>>
>> Konrad
>>>
>>> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
>>> ---
>>>   drivers/clk/qcom/mmcc-msm8974.c | 11 +++++++++++
>>>   1 file changed, 11 insertions(+)
>>>
>>> diff --git a/drivers/clk/qcom/mmcc-msm8974.c b/drivers/clk/qcom/mmcc-msm8974.c
>>> index 4273fce9a4a4..39ee3953567c 100644
>>> --- a/drivers/clk/qcom/mmcc-msm8974.c
>>> +++ b/drivers/clk/qcom/mmcc-msm8974.c
>>> @@ -2443,6 +2443,16 @@ static struct gdsc oxilicx_gdsc = {
>>>       .pwrsts = PWRSTS_OFF_ON,
>>>   };
>>>   +static struct gdsc oxilicx_gdsc_msm8226 = {
>>> +    .gdscr = 0x4034,
>>> +    .cxcs = (unsigned int []){ 0x4028 },
>>> +    .cxc_count = 1,
>>> +    .pd = {
>>> +        .name = "oxilicx",
>>> +    },
>>> +    .pwrsts = PWRSTS_OFF_ON,
>>> +};
>>> +
>>>   static struct clk_regmap *mmcc_msm8226_clocks[] = {
>>>       [MMSS_AHB_CLK_SRC] = &mmss_ahb_clk_src.clkr,
>>>       [MMSS_AXI_CLK_SRC] = &mmss_axi_clk_src.clkr,
>>> @@ -2533,6 +2543,7 @@ static struct gdsc *mmcc_msm8226_gdscs[] = {
>>>       [MDSS_GDSC] = &mdss_gdsc,
>>>       [CAMSS_JPEG_GDSC] = &camss_jpeg_gdsc,
>>>       [CAMSS_VFE_GDSC] = &camss_vfe_gdsc,
>>> +    [OXILICX_GDSC] = &oxilicx_gdsc_msm8226,
>>>   };
>>>     static const struct regmap_config mmcc_msm8226_regmap_config = {
>>>
>>> ---
>>> base-commit: dd9e11d6477a52ede9ebe575c83285e79e823889
>>> change-id: 20230506-msm8226-oxilicx-7f3f0f8e491d
>>>
>>> Best regards,
> 

Re: [PATCH] clk: qcom: mmcc-msm8974: Add OXILICX_GDSC for msm8226

[Luca Weiss]

On Montag, 8. Mai 2023 13:35:07 CEST Konrad Dybcio wrote:
> On 8.05.2023 13:32, Dmitry Baryshkov wrote:
> > On 08/05/2023 10:23, Konrad Dybcio wrote:
> >> On 6.05.2023 23:20, Luca Weiss wrote:
> >>> On msm8226 we also have OXILICX_GDSC but we need a slighly different
> >>> config, with a .cxcs defined for clock but with no parent.
> >> 
> >> Hm, on newer (a5xx+) GPUs, CX needs to be turned on first and
> >> to achieve that, we sometimes define it to be the GX's (also
> >> implicitly known as "oxili-non-CX" in before-a6xx-times) parent..
> >> 
> >> Roughly speaking CX powers the "GPU hardware owned by the broader
> >> SoC that may not need the GPU core clock to be up" and GX powers
> >> the "GPU hardware owned strictly by the GPU that needs at least some
> >> GPU clocks to be enabled"
> >> 
> >> Maybe 8974 simply has a bug in the driver that would do the reverse?
> >> Could you (and perhaps Dmitry on his shiny new 13yo board) test that
> >> theory, preferably on both SoCs?
> >> 
> >> --- a/drivers/clk/qcom/mmcc-msm8974.c
> >> +++ b/drivers/clk/qcom/mmcc-msm8974.c
> >> @@ -2431,6 +2431,7 @@ static struct gdsc oxili_gdsc = {
> >>          .pd = {
> >>                  .name = "oxili",
> >>          },
> >> +       .parent = &oxili_gdsc.pd,
> >>          .pwrsts = PWRSTS_OFF_ON,
> >>   };
> > 
> > Are you declaring oxili_gdsc to be a parent of itself?
> 
> lol.. nice catch of course this line should have been
> 
> +       .parent = &oxilicx_gdsc.pd,
> 
> and the definitions would need to be swapped

The 0x4024 oxili_gdsc (downstream name gdsc_oxili_gx) is disabled in 8226 dts.

Only in downstream msm8974.dtsi this gdsc gets "parent-supply = 
<&pm8841_s4_corner>;", on 8226 there's no parent-supply. And the gdsc parent 
doesn't even seem to be described there.

Should I still try?

> 
> Konrad
> 
> >>   @@ -2439,7 +2440,6 @@ static struct gdsc oxilicx_gdsc = {
> >>          .pd = {
> >>                  .name = "oxilicx",
> >>          },
> >> -       .parent = &oxili_gdsc.pd,
> >>          .pwrsts = PWRSTS_OFF_ON,
> >>   };
> >> 
> >> Konrad
> >> 
> >>> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> >>> ---
> >>>   drivers/clk/qcom/mmcc-msm8974.c | 11 +++++++++++
> >>>   1 file changed, 11 insertions(+)
> >>> 
> >>> diff --git a/drivers/clk/qcom/mmcc-msm8974.c
> >>> b/drivers/clk/qcom/mmcc-msm8974.c index 4273fce9a4a4..39ee3953567c
> >>> 100644
> >>> --- a/drivers/clk/qcom/mmcc-msm8974.c
> >>> +++ b/drivers/clk/qcom/mmcc-msm8974.c
> >>> @@ -2443,6 +2443,16 @@ static struct gdsc oxilicx_gdsc = {
> >>>       .pwrsts = PWRSTS_OFF_ON,
> >>>   };
> >>>   +static struct gdsc oxilicx_gdsc_msm8226 = {
> >>> +    .gdscr = 0x4034,
> >>> +    .cxcs = (unsigned int []){ 0x4028 },
> >>> +    .cxc_count = 1,
> >>> +    .pd = {
> >>> +        .name = "oxilicx",
> >>> +    },
> >>> +    .pwrsts = PWRSTS_OFF_ON,
> >>> +};
> >>> +
> >>>   static struct clk_regmap *mmcc_msm8226_clocks[] = {
> >>>       [MMSS_AHB_CLK_SRC] = &mmss_ahb_clk_src.clkr,
> >>>       [MMSS_AXI_CLK_SRC] = &mmss_axi_clk_src.clkr,
> >>> @@ -2533,6 +2543,7 @@ static struct gdsc *mmcc_msm8226_gdscs[] = {
> >>>       [MDSS_GDSC] = &mdss_gdsc,
> >>>       [CAMSS_JPEG_GDSC] = &camss_jpeg_gdsc,
> >>>       [CAMSS_VFE_GDSC] = &camss_vfe_gdsc,
> >>> +    [OXILICX_GDSC] = &oxilicx_gdsc_msm8226,
> >>>   };
> >>>     static const struct regmap_config mmcc_msm8226_regmap_config = {
> >>> 
> >>> ---
> >>> base-commit: dd9e11d6477a52ede9ebe575c83285e79e823889
> >>> change-id: 20230506-msm8226-oxilicx-7f3f0f8e491d
> >>> 
> >>> Best regards,

Re: [PATCH] clk: qcom: mmcc-msm8974: Add OXILICX_GDSC for msm8226

[Konrad Dybcio]

On 9.05.2023 18:57, Luca Weiss wrote:
> On Montag, 8. Mai 2023 13:35:07 CEST Konrad Dybcio wrote:
>> On 8.05.2023 13:32, Dmitry Baryshkov wrote:
>>> On 08/05/2023 10:23, Konrad Dybcio wrote:
>>>> On 6.05.2023 23:20, Luca Weiss wrote:
>>>>> On msm8226 we also have OXILICX_GDSC but we need a slighly different
>>>>> config, with a .cxcs defined for clock but with no parent.
>>>>
>>>> Hm, on newer (a5xx+) GPUs, CX needs to be turned on first and
>>>> to achieve that, we sometimes define it to be the GX's (also
>>>> implicitly known as "oxili-non-CX" in before-a6xx-times) parent..
>>>>
>>>> Roughly speaking CX powers the "GPU hardware owned by the broader
>>>> SoC that may not need the GPU core clock to be up" and GX powers
>>>> the "GPU hardware owned strictly by the GPU that needs at least some
>>>> GPU clocks to be enabled"
>>>>
>>>> Maybe 8974 simply has a bug in the driver that would do the reverse?
>>>> Could you (and perhaps Dmitry on his shiny new 13yo board) test that
>>>> theory, preferably on both SoCs?
>>>>
>>>> --- a/drivers/clk/qcom/mmcc-msm8974.c
>>>> +++ b/drivers/clk/qcom/mmcc-msm8974.c
>>>> @@ -2431,6 +2431,7 @@ static struct gdsc oxili_gdsc = {
>>>>          .pd = {
>>>>                  .name = "oxili",
>>>>          },
>>>> +       .parent = &oxili_gdsc.pd,
>>>>          .pwrsts = PWRSTS_OFF_ON,
>>>>   };
>>>
>>> Are you declaring oxili_gdsc to be a parent of itself?
>>
>> lol.. nice catch of course this line should have been
>>
>> +       .parent = &oxilicx_gdsc.pd,
>>
>> and the definitions would need to be swapped
> 
> The 0x4024 oxili_gdsc (downstream name gdsc_oxili_gx) is disabled in 8226 dts.
> 
> Only in downstream msm8974.dtsi this gdsc gets "parent-supply = 
> <&pm8841_s4_corner>;", on 8226 there's no parent-supply. And the gdsc parent 
> doesn't even seem to be described there.
> 
> Should I still try?
No, nevermind, this SoC is cut down more than I had initially thought.

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

with a minor nit: oxilicx -> oxili_cx

Konrad
> 
>>
>> Konrad
>>
>>>>   @@ -2439,7 +2440,6 @@ static struct gdsc oxilicx_gdsc = {
>>>>          .pd = {
>>>>                  .name = "oxilicx",
>>>>          },
>>>> -       .parent = &oxili_gdsc.pd,
>>>>          .pwrsts = PWRSTS_OFF_ON,
>>>>   };
>>>>
>>>> Konrad
>>>>
>>>>> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
>>>>> ---
>>>>>   drivers/clk/qcom/mmcc-msm8974.c | 11 +++++++++++
>>>>>   1 file changed, 11 insertions(+)
>>>>>
>>>>> diff --git a/drivers/clk/qcom/mmcc-msm8974.c
>>>>> b/drivers/clk/qcom/mmcc-msm8974.c index 4273fce9a4a4..39ee3953567c
>>>>> 100644
>>>>> --- a/drivers/clk/qcom/mmcc-msm8974.c
>>>>> +++ b/drivers/clk/qcom/mmcc-msm8974.c
>>>>> @@ -2443,6 +2443,16 @@ static struct gdsc oxilicx_gdsc = {
>>>>>       .pwrsts = PWRSTS_OFF_ON,
>>>>>   };
>>>>>   +static struct gdsc oxilicx_gdsc_msm8226 = {
>>>>> +    .gdscr = 0x4034,
>>>>> +    .cxcs = (unsigned int []){ 0x4028 },
>>>>> +    .cxc_count = 1,
>>>>> +    .pd = {
>>>>> +        .name = "oxilicx",
>>>>> +    },
>>>>> +    .pwrsts = PWRSTS_OFF_ON,
>>>>> +};
>>>>> +
>>>>>   static struct clk_regmap *mmcc_msm8226_clocks[] = {
>>>>>       [MMSS_AHB_CLK_SRC] = &mmss_ahb_clk_src.clkr,
>>>>>       [MMSS_AXI_CLK_SRC] = &mmss_axi_clk_src.clkr,
>>>>> @@ -2533,6 +2543,7 @@ static struct gdsc *mmcc_msm8226_gdscs[] = {
>>>>>       [MDSS_GDSC] = &mdss_gdsc,
>>>>>       [CAMSS_JPEG_GDSC] = &camss_jpeg_gdsc,
>>>>>       [CAMSS_VFE_GDSC] = &camss_vfe_gdsc,
>>>>> +    [OXILICX_GDSC] = &oxilicx_gdsc_msm8226,
>>>>>   };
>>>>>     static const struct regmap_config mmcc_msm8226_regmap_config = {
>>>>>
>>>>> ---
>>>>> base-commit: dd9e11d6477a52ede9ebe575c83285e79e823889
>>>>> change-id: 20230506-msm8226-oxilicx-7f3f0f8e491d
>>>>>
>>>>> Best regards,
> 
> 
> 
> 

Re: [PATCH] clk: qcom: mmcc-msm8974: Add OXILICX_GDSC for msm8226

[Luca Weiss]

On Dienstag, 16. Mai 2023 02:15:06 CEST Konrad Dybcio wrote:
> On 9.05.2023 18:57, Luca Weiss wrote:
> > On Montag, 8. Mai 2023 13:35:07 CEST Konrad Dybcio wrote:
> >> On 8.05.2023 13:32, Dmitry Baryshkov wrote:
> >>> On 08/05/2023 10:23, Konrad Dybcio wrote:
> >>>> On 6.05.2023 23:20, Luca Weiss wrote:
> >>>>> On msm8226 we also have OXILICX_GDSC but we need a slighly different
> >>>>> config, with a .cxcs defined for clock but with no parent.
> >>>> 
> >>>> Hm, on newer (a5xx+) GPUs, CX needs to be turned on first and
> >>>> to achieve that, we sometimes define it to be the GX's (also
> >>>> implicitly known as "oxili-non-CX" in before-a6xx-times) parent..
> >>>> 
> >>>> Roughly speaking CX powers the "GPU hardware owned by the broader
> >>>> SoC that may not need the GPU core clock to be up" and GX powers
> >>>> the "GPU hardware owned strictly by the GPU that needs at least some
> >>>> GPU clocks to be enabled"
> >>>> 
> >>>> Maybe 8974 simply has a bug in the driver that would do the reverse?
> >>>> Could you (and perhaps Dmitry on his shiny new 13yo board) test that
> >>>> theory, preferably on both SoCs?
> >>>> 
> >>>> --- a/drivers/clk/qcom/mmcc-msm8974.c
> >>>> +++ b/drivers/clk/qcom/mmcc-msm8974.c
> >>>> @@ -2431,6 +2431,7 @@ static struct gdsc oxili_gdsc = {
> >>>> 
> >>>>          .pd = {
> >>>>          
> >>>>                  .name = "oxili",
> >>>>          
> >>>>          },
> >>>> 
> >>>> +       .parent = &oxili_gdsc.pd,
> >>>> 
> >>>>          .pwrsts = PWRSTS_OFF_ON,
> >>>>   
> >>>>   };
> >>> 
> >>> Are you declaring oxili_gdsc to be a parent of itself?
> >> 
> >> lol.. nice catch of course this line should have been
> >> 
> >> +       .parent = &oxilicx_gdsc.pd,
> >> 
> >> and the definitions would need to be swapped
> > 
> > The 0x4024 oxili_gdsc (downstream name gdsc_oxili_gx) is disabled in 8226
> > dts.
> > 
> > Only in downstream msm8974.dtsi this gdsc gets "parent-supply =
> > <&pm8841_s4_corner>;", on 8226 there's no parent-supply. And the gdsc
> > parent doesn't even seem to be described there.
> > 
> > Should I still try?
> 
> No, nevermind, this SoC is cut down more than I had initially thought.
> 
> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> 
> with a minor nit: oxilicx -> oxili_cx

Hi Konrad,

where do you want this changed? Just the .name field? But even that one is now 
matching the other oxilicx variant. And there's also gdscs like 
oxilicx_ahb_clk.

Let me know.

Regards
Luca

> 
> Konrad
> 
> >> Konrad
> >> 
> >>>>   @@ -2439,7 +2440,6 @@ static struct gdsc oxilicx_gdsc = {
> >>>>   
> >>>>          .pd = {
> >>>>          
> >>>>                  .name = "oxilicx",
> >>>>          
> >>>>          },
> >>>> 
> >>>> -       .parent = &oxili_gdsc.pd,
> >>>> 
> >>>>          .pwrsts = PWRSTS_OFF_ON,
> >>>>   
> >>>>   };
> >>>> 
> >>>> Konrad
> >>>> 
> >>>>> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> >>>>> ---
> >>>>> 
> >>>>>   drivers/clk/qcom/mmcc-msm8974.c | 11 +++++++++++
> >>>>>   1 file changed, 11 insertions(+)
> >>>>> 
> >>>>> diff --git a/drivers/clk/qcom/mmcc-msm8974.c
> >>>>> b/drivers/clk/qcom/mmcc-msm8974.c index 4273fce9a4a4..39ee3953567c
> >>>>> 100644
> >>>>> --- a/drivers/clk/qcom/mmcc-msm8974.c
> >>>>> +++ b/drivers/clk/qcom/mmcc-msm8974.c
> >>>>> @@ -2443,6 +2443,16 @@ static struct gdsc oxilicx_gdsc = {
> >>>>> 
> >>>>>       .pwrsts = PWRSTS_OFF_ON,
> >>>>>   
> >>>>>   };
> >>>>>   +static struct gdsc oxilicx_gdsc_msm8226 = {
> >>>>> 
> >>>>> +    .gdscr = 0x4034,
> >>>>> +    .cxcs = (unsigned int []){ 0x4028 },
> >>>>> +    .cxc_count = 1,
> >>>>> +    .pd = {
> >>>>> +        .name = "oxilicx",
> >>>>> +    },
> >>>>> +    .pwrsts = PWRSTS_OFF_ON,
> >>>>> +};
> >>>>> +
> >>>>> 
> >>>>>   static struct clk_regmap *mmcc_msm8226_clocks[] = {
> >>>>>   
> >>>>>       [MMSS_AHB_CLK_SRC] = &mmss_ahb_clk_src.clkr,
> >>>>>       [MMSS_AXI_CLK_SRC] = &mmss_axi_clk_src.clkr,
> >>>>> 
> >>>>> @@ -2533,6 +2543,7 @@ static struct gdsc *mmcc_msm8226_gdscs[] = {
> >>>>> 
> >>>>>       [MDSS_GDSC] = &mdss_gdsc,
> >>>>>       [CAMSS_JPEG_GDSC] = &camss_jpeg_gdsc,
> >>>>>       [CAMSS_VFE_GDSC] = &camss_vfe_gdsc,
> >>>>> 
> >>>>> +    [OXILICX_GDSC] = &oxilicx_gdsc_msm8226,
> >>>>> 
> >>>>>   };
> >>>>>   
> >>>>>     static const struct regmap_config mmcc_msm8226_regmap_config = {
> >>>>> 
> >>>>> ---
> >>>>> base-commit: dd9e11d6477a52ede9ebe575c83285e79e823889
> >>>>> change-id: 20230506-msm8226-oxilicx-7f3f0f8e491d
> >>>>> 
> >>>>> Best regards,

Re: [PATCH] clk: qcom: mmcc-msm8974: Add OXILICX_GDSC for msm8226

[Konrad Dybcio]

On 23.05.2023 22:44, Luca Weiss wrote:
> On Dienstag, 16. Mai 2023 02:15:06 CEST Konrad Dybcio wrote:
>> On 9.05.2023 18:57, Luca Weiss wrote:
>>> On Montag, 8. Mai 2023 13:35:07 CEST Konrad Dybcio wrote:
>>>> On 8.05.2023 13:32, Dmitry Baryshkov wrote:
>>>>> On 08/05/2023 10:23, Konrad Dybcio wrote:
>>>>>> On 6.05.2023 23:20, Luca Weiss wrote:
>>>>>>> On msm8226 we also have OXILICX_GDSC but we need a slighly different
>>>>>>> config, with a .cxcs defined for clock but with no parent.
>>>>>>
>>>>>> Hm, on newer (a5xx+) GPUs, CX needs to be turned on first and
>>>>>> to achieve that, we sometimes define it to be the GX's (also
>>>>>> implicitly known as "oxili-non-CX" in before-a6xx-times) parent..
>>>>>>
>>>>>> Roughly speaking CX powers the "GPU hardware owned by the broader
>>>>>> SoC that may not need the GPU core clock to be up" and GX powers
>>>>>> the "GPU hardware owned strictly by the GPU that needs at least some
>>>>>> GPU clocks to be enabled"
>>>>>>
>>>>>> Maybe 8974 simply has a bug in the driver that would do the reverse?
>>>>>> Could you (and perhaps Dmitry on his shiny new 13yo board) test that
>>>>>> theory, preferably on both SoCs?
>>>>>>
>>>>>> --- a/drivers/clk/qcom/mmcc-msm8974.c
>>>>>> +++ b/drivers/clk/qcom/mmcc-msm8974.c
>>>>>> @@ -2431,6 +2431,7 @@ static struct gdsc oxili_gdsc = {
>>>>>>
>>>>>>          .pd = {
>>>>>>          
>>>>>>                  .name = "oxili",
>>>>>>          
>>>>>>          },
>>>>>>
>>>>>> +       .parent = &oxili_gdsc.pd,
>>>>>>
>>>>>>          .pwrsts = PWRSTS_OFF_ON,
>>>>>>   
>>>>>>   };
>>>>>
>>>>> Are you declaring oxili_gdsc to be a parent of itself?
>>>>
>>>> lol.. nice catch of course this line should have been
>>>>
>>>> +       .parent = &oxilicx_gdsc.pd,
>>>>
>>>> and the definitions would need to be swapped
>>>
>>> The 0x4024 oxili_gdsc (downstream name gdsc_oxili_gx) is disabled in 8226
>>> dts.
>>>
>>> Only in downstream msm8974.dtsi this gdsc gets "parent-supply =
>>> <&pm8841_s4_corner>;", on 8226 there's no parent-supply. And the gdsc
>>> parent doesn't even seem to be described there.
>>>
>>> Should I still try?
>>
>> No, nevermind, this SoC is cut down more than I had initially thought.
>>
>> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>
>> with a minor nit: oxilicx -> oxili_cx
> 
> Hi Konrad,
> 
> where do you want this changed? Just the .name field?
Yes and maybe the struct name. We shouldn't be messing with bindings
since it's cosmetic.

 But even that one is now 
> matching the other oxilicx variant. And there's also gdscs like 
> oxilicx_ahb_clk.
oxilicx literally means "the CX side of OXILI", or translating from
Qualcommish to English "GPU registers accessible from the AP, not
necessarily the GPU itself"

Konrad
> 
> Let me know.
> 
> Regards
> Luca
> 
>>
>> Konrad
>>
>>>> Konrad
>>>>
>>>>>>   @@ -2439,7 +2440,6 @@ static struct gdsc oxilicx_gdsc = {
>>>>>>   
>>>>>>          .pd = {
>>>>>>          
>>>>>>                  .name = "oxilicx",
>>>>>>          
>>>>>>          },
>>>>>>
>>>>>> -       .parent = &oxili_gdsc.pd,
>>>>>>
>>>>>>          .pwrsts = PWRSTS_OFF_ON,
>>>>>>   
>>>>>>   };
>>>>>>
>>>>>> Konrad
>>>>>>
>>>>>>> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
>>>>>>> ---
>>>>>>>
>>>>>>>   drivers/clk/qcom/mmcc-msm8974.c | 11 +++++++++++
>>>>>>>   1 file changed, 11 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/clk/qcom/mmcc-msm8974.c
>>>>>>> b/drivers/clk/qcom/mmcc-msm8974.c index 4273fce9a4a4..39ee3953567c
>>>>>>> 100644
>>>>>>> --- a/drivers/clk/qcom/mmcc-msm8974.c
>>>>>>> +++ b/drivers/clk/qcom/mmcc-msm8974.c
>>>>>>> @@ -2443,6 +2443,16 @@ static struct gdsc oxilicx_gdsc = {
>>>>>>>
>>>>>>>       .pwrsts = PWRSTS_OFF_ON,
>>>>>>>   
>>>>>>>   };
>>>>>>>   +static struct gdsc oxilicx_gdsc_msm8226 = {
>>>>>>>
>>>>>>> +    .gdscr = 0x4034,
>>>>>>> +    .cxcs = (unsigned int []){ 0x4028 },
>>>>>>> +    .cxc_count = 1,
>>>>>>> +    .pd = {
>>>>>>> +        .name = "oxilicx",
>>>>>>> +    },
>>>>>>> +    .pwrsts = PWRSTS_OFF_ON,
>>>>>>> +};
>>>>>>> +
>>>>>>>
>>>>>>>   static struct clk_regmap *mmcc_msm8226_clocks[] = {
>>>>>>>   
>>>>>>>       [MMSS_AHB_CLK_SRC] = &mmss_ahb_clk_src.clkr,
>>>>>>>       [MMSS_AXI_CLK_SRC] = &mmss_axi_clk_src.clkr,
>>>>>>>
>>>>>>> @@ -2533,6 +2543,7 @@ static struct gdsc *mmcc_msm8226_gdscs[] = {
>>>>>>>
>>>>>>>       [MDSS_GDSC] = &mdss_gdsc,
>>>>>>>       [CAMSS_JPEG_GDSC] = &camss_jpeg_gdsc,
>>>>>>>       [CAMSS_VFE_GDSC] = &camss_vfe_gdsc,
>>>>>>>
>>>>>>> +    [OXILICX_GDSC] = &oxilicx_gdsc_msm8226,
>>>>>>>
>>>>>>>   };
>>>>>>>   
>>>>>>>     static const struct regmap_config mmcc_msm8226_regmap_config = {
>>>>>>>
>>>>>>> ---
>>>>>>> base-commit: dd9e11d6477a52ede9ebe575c83285e79e823889
>>>>>>> change-id: 20230506-msm8226-oxilicx-7f3f0f8e491d
>>>>>>>
>>>>>>> Best regards,
> 
> 
> 
>