Re: [PATCH v4 4/7] dt-bindings: gpu: v3d: Add per-compatible register restrictions

[Krzysztof Kozlowski <krzk@kernel.org>]

On 13/03/2025 15:43, Maíra Canal wrote:
> In order to enforce per-SoC register rules, add per-compatible
> restrictions. V3D 3.3 (represented by brcm,7268-v3d) has a cache
> controller (GCA), which is not present in other V3D generations.
> Declaring these differences helps ensure the DTB accurately reflect
> the hardware design.
> 
> While not ideal, this commit keeps the register order flexible for
> brcm,7268-v3d with the goal to keep the ABI backwards compatible.
> 
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>  .../devicetree/bindings/gpu/brcm,bcm-v3d.yaml      | 73 ++++++++++++++++++----
>  1 file changed, 61 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/gpu/brcm,bcm-v3d.yaml b/Documentation/devicetree/bindings/gpu/brcm,bcm-v3d.yaml
> index dc078ceeca9ac3447ba54a7c8830821f0b2a7f9f..9867b617c60c6fe34a0f88a3ee2f581a94b69a5c 100644
> --- a/Documentation/devicetree/bindings/gpu/brcm,bcm-v3d.yaml
> +++ b/Documentation/devicetree/bindings/gpu/brcm,bcm-v3d.yaml
> @@ -22,20 +22,10 @@ properties:
>        - brcm,7278-v3d
>  
>    reg:
> -    items:
> -      - description: hub register (required)
> -      - description: core0 register (required)
> -      - description: GCA cache controller register (if GCA controller present)
> -      - description: bridge register (if no external reset controller)
> -    minItems: 2

Widest constraints always stay here, so you cannot remove minItems.

> +    maxItems: 4

>  
>    reg-names:
> -    items:
> -      - const: hub
> -      - const: core0
> -      - enum: [ bridge, gca ]
> -      - enum: [ bridge, gca ]
> -    minItems: 2

Same problem.

> +    maxItems: 4
>  
>    interrupts:
>      items:
> @@ -58,6 +48,65 @@ required:
>    - reg-names
>    - interrupts

...

> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: brcm,7268-v3d
> +    then:
> +      properties:
> +        reg:
> +          items:
> +            - description: hub register
> +            - description: core0 register
> +            - description: GCA cache controller register
> +            - description: bridge register (if no external reset controller)
> +          minItems: 3
> +        reg-names:
> +          items:
> +            - const: hub
> +            - const: core0
> +            - enum: [ bridge, gca ]

So GCA is always there? Then this should be just 'gca'. Your list for
'reg' already says that third item must be GCA. I understand that you do
not want to affect the ABI, but it already kind of is with enforcing GCA
in 'reg'.

I anyway do not understand why 'gca' or 'bridge' are supposed to be
optional. Does the given SoC differ between boards? What is the external
reset controller here? External like outside of SoC?

Best regards,
Krzysztof