diff mbox series

clk: qcom: mmcc-msm8974: Add OXILICX_GDSC for msm8226

Message ID 20230506-msm8226-oxilicx-v1-1-52e34b94ff22@z3ntu.xyz
State New
Headers
Series clk: qcom: mmcc-msm8974: Add OXILICX_GDSC for msm8226 |

Commit Message

Luca Weiss May 6, 2023, 9:20 p.m. UTC 
  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(+)


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

Best regards,

Comments

Konrad Dybcio May 8, 2023, 7:23 a.m. UTC | #1 
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,
Dmitry Baryshkov May 8, 2023, 11:32 a.m. UTC | #2 
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,
Konrad Dybcio May 8, 2023, 11:35 a.m. UTC | #3 
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,
>
Luca Weiss May 9, 2023, 4:57 p.m. UTC | #4 
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,
Konrad Dybcio May 16, 2023, 12:15 a.m. UTC | #5 
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,
> 
> 
> 
>
Luca Weiss May 23, 2023, 8:44 p.m. UTC | #6 
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,
Konrad Dybcio May 26, 2023, 7:41 p.m. UTC | #7 
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,
> 
> 
> 
>
diff mbox series

Patch

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