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@xxxxxxxxxx>
>>
>> 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@xxxxxxxxx>
>>>>>>> ---
>>>>>>>
>>>>>>>   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,
> 
> 
> 
>