Re: [PATCH 3/6] dt-bindings: display/msm/gmu: Document A612 RGMU

[Krzysztof Kozlowski <krzk@kernel.org>]

On 21/10/2025 17:51, Akhil P Oommen wrote:
> On 10/19/2025 2:43 PM, Krzysztof Kozlowski wrote:
>> On 17/10/2025 19:08, Akhil P Oommen wrote:
>>> RGMU a.k.a Reduced Graphics Management Unit is a small state machine
>>> with the sole purpose of providing IFPC (Inter Frame Power Collapse)
>>> support. Compared to GMU, it doesn't manage GPU clock, voltage
>>> scaling, bw voting or any other functionalities. All it does is detect
>>> an idle GPU and toggle the GDSC switch. As it doesn't access DDR space,
>>> it doesn't require iommu.
>>>
>>> So far, only Adreno 612 GPU has an RGMU core. Document RGMU in the GMU's
>>> schema.
>>>
>>> Signed-off-by: Akhil P Oommen <akhilpo@oss.qualcomm.com>
>>> ---
>>>  .../devicetree/bindings/display/msm/gmu.yaml       | 98 +++++++++++++++++-----
>>>  1 file changed, 79 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/msm/gmu.yaml b/Documentation/devicetree/bindings/display/msm/gmu.yaml
>>> index afc1879357440c137cadeb2d9a74ae8459570a25..a262d41755f09f21f607bf7a1fd567f386595f39 100644
>>> --- a/Documentation/devicetree/bindings/display/msm/gmu.yaml
>>> +++ b/Documentation/devicetree/bindings/display/msm/gmu.yaml
>>> @@ -26,6 +26,9 @@ properties:
>>>        - items:
>>>            - pattern: '^qcom,adreno-gmu-x[1-9][0-9][0-9]\.[0-9]$'
>>>            - const: qcom,adreno-gmu
>>> +      - items:
>>> +          - const: qcom,adreno-rgmu-612.0
>>> +          - const: qcom,adreno-rgmu
>>>        - const: qcom,adreno-gmu-wrapper
>>>  
>>>    reg:
>>> @@ -45,24 +48,30 @@ properties:
>>>      maxItems: 7
>>>  
>>>    interrupts:
>>> -    items:
>>> -      - description: GMU HFI interrupt
>>> -      - description: GMU interrupt
>>
>>
>> Both stay, just explain what is the first interrupt. You should not drop
>> descriptions here. Look at every other binding - of course except that
>> terrible Adreno GPU which is anti-example.
> 
> Do you mean we should use a OneOf and list both combo? Or elaborate the
> description of the first interrupt to include OOB too? Something like:
> 
> - description: HFI interrupt on GMU and OOB interrupt on RGMU.

Yes, like that.

> 
> Sorry, I am a bit confused.
> 
>>
>>> +    minItems: 2
>>> +    maxItems: 2
>>>  
>>>    interrupt-names:
>>> -    items:
>>> -      - const: hfi
>>> -      - const: gmu
>>> +    oneOf:
>>> +      - items:
>>> +          - const: hfi
>>> +            description: GMU HFI interrupt
>>
>> No, descriptions never go to xxx-names, but to xxx.
>>
>>> +          - const: gmu
>>> +            description: GMU interrupt
>>> +      - items:
>>> +          - const: oob
>>> +            description: GMU OOB interrupt
>>> +          - const: gmu
>>> +            description: GMU interrupt
>>> +
>>>  
>>>    power-domains:
>>> -    items:
>>> -      - description: CX power domain
>>> -      - description: GX power domain
>>> +    minItems: 2
>>> +    maxItems: 3
>>
>> No.
> I will keep the 'description'. Here, RGMU has 3 power domains instead of
> 2. I suppose we should add the description for the 3rd power domain here
> and keep 'minItems: 2' property to override the default 3?

Yes.

> 
>>
>>>  
>>>    power-domain-names:
>>> -    items:
>>> -      - const: cx
>>> -      - const: gx
>>> +    minItems: 2
>>> +    maxItems: 3
>>
>>
>> No. Why?
> Same as above.
> 
>>
>>>  
>>>    iommus:
>>>      maxItems: 1
>>> @@ -86,6 +95,44 @@ required:
>>>  additionalProperties: false
>>>  
>>>  allOf:
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            const: qcom,adreno-rgmu-612.0
>>> +    then:
>>> +      properties:
>>> +        reg:
>>> +          items:
>>> +            - description: Core RGMU registers
>>> +        reg-names:
>>> +          items:
>>> +            - const: gmu
>>> +        clocks:
>>> +          items:
>>> +            - description: GMU clock
>>> +            - description: GPU CX clock
>>> +            - description: GPU AXI clock
>>> +            - description: GPU MEMNOC clock
>>> +            - description: GPU SMMU vote clock
>>> +        clock-names:
>>> +          items:
>>> +            - const: gmu
>>> +            - const: cxo
>>> +            - const: axi
>>> +            - const: memnoc
>>> +            - const: smmu_vote
>>> +        power-domains:
>>> +          items:
>>> +            - description: CX power domain
>>> +            - description: GX power domain
>>> +            - description: VDD_CX power domain
>>> +        power-domain-names:
>>> +          items:
>>> +            - const: cx
>>> +            - const: gx
>>> +            - const: vdd_cx
>>
>> This does not make even sense. Why did you remove the the common list
>> from  power-domain-names?
>>
>>> +
>>>    - if:
>>>        properties:
>>>          compatible:
>>> @@ -313,13 +360,26 @@ allOf:
>>>            items:
>>>              - const: gmu
>>>      else:
>>> -      required:
>>> -        - clocks
>>> -        - clock-names
>>> -        - interrupts
>>> -        - interrupt-names
>>> -        - iommus
>>> -        - operating-points-v2
>>> +      if:
>>> +        properties:
>>> +          compatible:
>>> +            contains:
>>> +              const: qcom,adreno-rgmu
>>> +      then:
>>> +        required:
>>> +          - clocks
>>> +          - clock-names
>>> +          - interrupts
>>> +          - interrupt-names
>>> +          - operating-points-v2
>>> +      else:
>>
>> No. Don't nest multiple ifs.
> 
> I guess we should split this. I will add a 'required' constraint to the
> rgmu constraints above. And apply the below 'required' constraint
> specifically to 'qcom,adreno-gmu' instead of the 'else' fallback case.
> 
> Please let me know if you have any suggestion.

Maybe the binding is getting to complicated and RGMU should have its own.


Best regards,
Krzysztof