[PATCH v2 4/6] dt-bindings: gpu: v3d: Add SMS to the registers' list

[Maíra Canal]

Hi Krzysztof,

On 3/10/25 09:55, Krzysztof Kozlowski wrote:
> On 10/03/2025 12:57, Maíra Canal wrote:
>>>
>>>> Signed-off-by: Maíra Canal <mcanal at igalia.com>
>>>> ---
>>>>    .../devicetree/bindings/gpu/brcm,bcm-v3d.yaml      | 60 ++++++++++++++++++++--
>>>>    1 file changed, 55 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/gpu/brcm,bcm-v3d.yaml b/Documentation/devicetree/bindings/gpu/brcm,bcm-v3d.yaml
>>>> index dc078ceeca9ac3447ba54a7c8830821f0b2a7f9f..c0caee055e8c18dbcac0e51aa192951996545695 100644
>>>> --- a/Documentation/devicetree/bindings/gpu/brcm,bcm-v3d.yaml
>>>> +++ b/Documentation/devicetree/bindings/gpu/brcm,bcm-v3d.yaml
>>>> @@ -27,15 +27,12 @@ properties:
>>>>          - description: core0 register (required)
>>>>          - description: GCA cache controller register (if GCA controller present)
>>>>          - description: bridge register (if no external reset controller)
>>>> +      - description: SMS register (if SMS controller present)
>>>
>>> This lists five items, but you say you have max 4?
>>
>> V3D 3.1 uses hub, core0, gca, and bridge (optional)
>> V3D 4.1 and 4.2 uses hub, core, and bridge (optional)
>> V3D 7.1 uses hub, core0, sms, and bridge (optional)
>>
>> Therefore, for a given DT, you will have 4 items max.
> 
> And how many items do you have here?
> 
>>
>>>
>>>>        minItems: 2
>>>>    
>>>>      reg-names:
>>>> -    items:
>>>> -      - const: hub
>>>> -      - const: core0
>>>> -      - enum: [ bridge, gca ]
>>>> -      - enum: [ bridge, gca ]
>>>>        minItems: 2
>>>> +    maxItems: 4
>>>
>>> So here 4, but earlier 5? These must come in sync.
>>
>> I added maxItems for reg in the allOf section.
> 
> I don't think you answer the comments. I said you listed earlier 5 items
> and then you answered with saying devices have 4 items. Here I said
> these properties must be synced and you said why you added maxItems...
> Not related, read again the feedback.

I'm sorry, I don't usually write DTBs. I believe what I need is to
specify the reg items for each compatible, right?

> 
> 
>>
>>>
>>>>    
>>>>      interrupts:
>>>>        items:
>>>> @@ -60,6 +57,59 @@ required:
>>>>    
>>>>    additionalProperties: false
>>>>    
>>>> +allOf:
>>>
>>> This goes above additionalProperties.
>>
>> Got it.
>>
>>>
>>>> +  - if:
>>>> +      properties:
>>>> +        compatible:
>>>> +          contains:
>>>> +            enum:
>>>> +              - brcm,2711-v3d
>>>> +              - brcm,7278-v3d
>>>> +    then:
>>>> +      properties:
>>>> +        reg:
>>>> +          minItems: 2
>>>> +          maxItems: 3
>>>> +        reg-names:
>>>> +          items:
>>>> +            - const: hub
>>>> +            - const: core0
>>>> +            - const: bridge
>>>
>>> Again un-synced lists.
>>
>> Sorry, what do you mean by un-synced lists?
> 
> xxx and xxx-names must have the same constraints. They do not have here.
> You have two different constraints and you can test your DTS to see that.
 >

I used `make dt_binding_check` to test it and it didn't catch any
errors.

>>
>>>
>>>> +  - if:
>>>> +      properties:
>>>> +        compatible:
>>>> +          contains:
>>>> +            const: brcm,2712-v3d
>>>> +    then:
>>>> +      properties:
>>>> +        reg:
>>>> +          minItems: 3
>>>> +          maxItems: 4
>>>> +        reg-names:
>>>> +          items:
>>>> +            - const: hub
>>>> +            - const: core0
>>>> +            - enum: [ bridge, sms ]
>>>> +            - enum: [ bridge, sms ]
>>>> +          minItems: 3
>>>
>>> Why is this flexible?
>>
>> I cannot guarantee the order and bridge is optional.
> 
> Hm? You must guarantee the order and I do not understand why this needs
> some sort of exception from all other bindings that only here you cannot
> guarantee the order.

I'm trying to keep backwards compatibility. This binding exists for many
years and it always used "enum: [ bridge, gca ]".

Best Regards,
- Maíra

> 
> 
> Best regards,
> Krzysztof