[PATCH v2 4/6] dt-bindings: gpu: v3d: Add SMS to the registers' list
Maíra Canal
mcanal at igalia.com
Mon Mar 10 20:07:31 UTC 2025
Hi Krzysztof,
On 3/10/25 14:34, Krzysztof Kozlowski wrote:
> On 10/03/2025 14:15, Maíra Canal wrote:
>> 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?
>
> You need to list four items in 'reg' and last items' description would
> need to describe two different sets.
>
> And commit msg must explain why now this device needs to use sms, not
> gca. Why you cannot use the gca range instead of sms? So many questions.
I believe I understood the issue now. From my point of view, the idea is
preserving the semantics of the register bank names. GCA is a cache
controller that it is only available in V3D 3.3 (so, only brcm,7268-v3d
has it). SMS is a power controller that it is only available in V3D 7.1
(so, only brcm,2712-v3d has it).
Each one of those compatibles represent different V3D generations (3.3,
4.1, 4.2, 7.1).
From my understanding, I'm keeping the ABI preserved, as brcm,7268-v3d
needs to have a GCA register (otherwise, you won't be able to flush the
cache) and brcm,2711-v3d doesn't even have this piece of hardware.
I understand that now I'm imposing per-compatible restrictions that
didn't exist in the spec, but the new restrictions are compatible to the
hardware specification. I'd like to understand if I can:
1. Use two register items (gca and sms) to preserve the semantics of the
register names.
2. Can I add per-compatible restrictions to the DT even if the original
bindings didn't do it? If not, can I just add a new register to the
register list? Like:
reg-names:
items:
- const: hub
- const: core0
- - enum: [ bridge, gca ]
- - enum: [ bridge, gca ]
+ - enum: [ bridge, gca, sms ]
+ - enum: [ bridge, gca, sms ]
+ - enum: [ bridge, gca, sms ]
minItems: 2
Best Regards,
- Maíra
>
>>
>>>
>>>
>>>>
>>>>>bcm
>>>>>>
>>>>>> 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.
>
> So change the example to have one list with two items and second list
> with three items. Is it correct DTS? No. Does this pass tests? Yes.
>
>>
>>>>
>>>>>
>>>>>> + - 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 ]".
>
> But it is now sms, not gca,, so I do not see how the ABI is preserved.
>
> Best regards,
> Krzysztof
More information about the dri-devel
mailing list