[PATCH v2 0/6] drm/v3d: Fix GPU reset issues on the Raspberry Pi 5

[PATCH v2 0/6] drm/v3d: Fix GPU reset issues on the Raspberry Pi 5

[Maíra Canal]

This series addresses GPU reset issues reported in [1], where running a
long compute job would trigger repeated GPU resets, leading to a UI
freeze.

Patches #1 and #2 prevent the same faulty job from being resubmitted in a
loop, mitigating the first cause of the issue.

However, the issue isn't entirely solved. Even with only a single GPU
reset, the UI still freezes on the Raspberry Pi 5, indicating a GPU hang.
Patches #3 to #5 address this by properly configuring the V3D_SMS
registers, which are required for power management and resets in V3D 7.1.

Patch #6 updates the DT maintainership, replacing Emma with the current
v3d driver maintainer.

[1] https://github.com/raspberrypi/linux/issues/6660

Best Regards,
- Maíra

---
v1 -> v2:
- [1/6, 2/6, 5/6] Add Iago's R-b (Iago Toral)
- [3/6] Use V3D_GEN_* macros consistently throughout the driver (Phil Elwell)
- [3/6] Don't add Iago's R-b in 3/6 due to changes in the patch
- [4/6] Add per-compatible restrictions to enforce per‐SoC register rules (Conor Dooley)
- [6/6] Add Emma's A-b, collected through IRC (Emma Anholt)
- [6/6] Add Rob's A-b (Rob Herring)
- Link to v1: https://lore.kernel.org/r/20250226-v3d-gpu-reset-fixes-v1-0-83a969fdd9c1@igalia.com

---
Maíra Canal (6):
      drm/v3d: Don't run jobs that have errors flagged in its fence
      drm/v3d: Set job pointer to NULL when the job's fence has an error
      drm/v3d: Associate a V3D tech revision to all supported devices
      dt-bindings: gpu: v3d: Add SMS to the registers' list
      drm/v3d: Use V3D_SMS registers for power on/off and reset on V3D 7.x
      dt-bindings: gpu: Add V3D driver maintainer as DT maintainer

 .../devicetree/bindings/gpu/brcm,bcm-v3d.yaml      |  62 +++++++++-
 drivers/gpu/drm/v3d/v3d_debugfs.c                  | 126 ++++++++++-----------
 drivers/gpu/drm/v3d/v3d_drv.c                      |  62 +++++++++-
 drivers/gpu/drm/v3d/v3d_drv.h                      |  22 +++-
 drivers/gpu/drm/v3d/v3d_gem.c                      |  27 ++++-
 drivers/gpu/drm/v3d/v3d_irq.c                      |   6 +-
 drivers/gpu/drm/v3d/v3d_perfmon.c                  |   4 +-
 drivers/gpu/drm/v3d/v3d_regs.h                     |  26 +++++
 drivers/gpu/drm/v3d/v3d_sched.c                    |  29 ++++-
 9 files changed, 271 insertions(+), 93 deletions(-)
---
base-commit: 2c7aafc05c8330be4c5f0092b79843507a5e1023
change-id: 20250224-v3d-gpu-reset-fixes-2d21fc70711d

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

[Maíra Canal]

V3D 7.1 exposes a new register block, called V3D_SMS. As BCM2712 has a
V3D 7.1 core, add a new register item to the list. Similar to the GCA
and bridge register, SMS is optional and should only be added for V3D
7.1 variants.

In order to enforce per-SoC register rules, add per-compatible
restrictions. The restrictions will assure that GCA will only be added
in BCM7268 (V3D 3.3) and SMS will only be added in BCM2712 (V3D 7.1).

Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Nicolas Saenz Julienne <nsaenz@kernel.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Maíra Canal <mcanal@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)
     minItems: 2
 
   reg-names:
-    items:
-      - const: hub
-      - const: core0
-      - enum: [ bridge, gca ]
-      - enum: [ bridge, gca ]
     minItems: 2
+    maxItems: 4
 
   interrupts:
     items:
@@ -60,6 +57,59 @@ required:
 
 additionalProperties: false
 
+allOf:
+  - 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
+  - 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
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: brcm,7268-v3d
+    then:
+      properties:
+        reg:
+          minItems: 3
+          maxItems: 4
+        reg-names:
+          items:
+            - const: hub
+            - const: core0
+            - enum: [ bridge, gca ]
+            - enum: [ bridge, gca ]
+          minItems: 3
+
 examples:
   - |
     gpu@f1200000 {

-- 
Git-154)

[PATCH v2 6/6] dt-bindings: gpu: Add V3D driver maintainer as DT maintainer

[Maíra Canal]

As established in commit 89d04995f76c ("MAINTAINERS: Drop Emma Anholt
from all M lines."), Emma is no longer active in the Linux kernel and
dropped the V3D maintainership. Therefore, remove Emma as one of the DT
maintainers and add the current V3D driver maintainer.

Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Nicolas Saenz Julienne <nsaenz@kernel.org>
Cc: devicetree@vger.kernel.org
Acked-by: Emma Anholt <emma@anholt.net>
Acked-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 Documentation/devicetree/bindings/gpu/brcm,bcm-v3d.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/gpu/brcm,bcm-v3d.yaml b/Documentation/devicetree/bindings/gpu/brcm,bcm-v3d.yaml
index c0caee055e8c18dbcac0e51aa192951996545695..ae890a46712477034f4efbc4d02635953bb68a40 100644
--- a/Documentation/devicetree/bindings/gpu/brcm,bcm-v3d.yaml
+++ b/Documentation/devicetree/bindings/gpu/brcm,bcm-v3d.yaml
@@ -7,7 +7,7 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
 title: Broadcom V3D GPU
 
 maintainers:
-  - Eric Anholt <eric@anholt.net>
+  - Maíra Canal <mcanal@igalia.com>
   - Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
 
 properties:

-- 
Git-154)

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

[Krzysztof Kozlowski]

On Sat, Mar 08, 2025 at 11:33:43AM -0300, Maíra Canal wrote:
> V3D 7.1 exposes a new register block, called V3D_SMS. As BCM2712 has a

Where is the comaptible for this new block? Or was it already documented
but with missing register?

> V3D 7.1 core, add a new register item to the list. Similar to the GCA
> and bridge register, SMS is optional and should only be added for V3D
> 7.1 variants.
> 
> In order to enforce per-SoC register rules, add per-compatible
> restrictions. The restrictions will assure that GCA will only be added
> in BCM7268 (V3D 3.3) and SMS will only be added in BCM2712 (V3D 7.1).
> 
> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: Nicolas Saenz Julienne <nsaenz@kernel.org>
> Cc: devicetree@vger.kernel.org

Please drop the autogenerated scripts/get_maintainer.pl CC-entries from
commit msg. There is no single need to store automated output of
get_maintainers.pl in the git log. It can be easily re-created at any
given time, thus its presence in the git history is redundant and
obfuscates the log.

If you need it for your own patch management purposes, keep it under the
--- separator.

> Signed-off-by: Maíra Canal <mcanal@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?

>      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.

>  
>    interrupts:
>      items:
> @@ -60,6 +57,59 @@ required:
>  
>  additionalProperties: false
>  
> +allOf:

This goes above additionalProperties.

> +  - 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.

> +  - 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?

Best regards,
Krzysztof

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

[Maíra Canal]

Hi Krzysztof,

On 3/10/25 06:49, Krzysztof Kozlowski wrote:
> On Sat, Mar 08, 2025 at 11:33:43AM -0300, Maíra Canal wrote:
>> V3D 7.1 exposes a new register block, called V3D_SMS. As BCM2712 has a
> 
> Where is the comaptible for this new block? Or was it already documented
> but with missing register?

The compatible is brcm,2712-v3d, which was already documented, but with
a missing register.

> 
>> V3D 7.1 core, add a new register item to the list. Similar to the GCA
>> and bridge register, SMS is optional and should only be added for V3D
>> 7.1 variants.
>>
>> In order to enforce per-SoC register rules, add per-compatible
>> restrictions. The restrictions will assure that GCA will only be added
>> in BCM7268 (V3D 3.3) and SMS will only be added in BCM2712 (V3D 7.1).
>>
>> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
>> Cc: Conor Dooley <conor+dt@kernel.org>
>> Cc: Nicolas Saenz Julienne <nsaenz@kernel.org>
>> Cc: devicetree@vger.kernel.org
> 
> Please drop the autogenerated scripts/get_maintainer.pl CC-entries from
> commit msg. There is no single need to store automated output of
> get_maintainers.pl in the git log. It can be easily re-created at any
> given time, thus its presence in the git history is redundant and
> obfuscates the log.
> 
> If you need it for your own patch management purposes, keep it under the
> --- separator.

Sorry, I'll change it for v3.

> 
>> Signed-off-by: Maíra Canal <mcanal@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.

> 
>>       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.

> 
>>   
>>     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?

> 
>> +  - 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.

Best Regards,
- Maíra

> 
> Best regards,
> Krzysztof
> 

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

[Krzysztof Kozlowski]

On 10/03/2025 12:57, Maíra Canal wrote:
>>
>>> Signed-off-by: Maíra Canal <mcanal@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.


> 
>>
>>>   
>>>     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.

> 
>>
>>> +  - 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.


Best regards,
Krzysztof

Re: [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@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

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

[Krzysztof Kozlowski]

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@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.

> 
>>
>>
>>>
>>>>
>>>>>    
>>>>>      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

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

[Maíra Canal]

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@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

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

[Krzysztof Kozlowski]

On 10/03/2025 21:07, Maíra Canal wrote:
> 
>  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.

Sure, that's fine.

> 
> 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:

Yes, constraints are expected and it does not matter that original
binding forgot them.

> 
>     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 ]

This does not look right, because there is no device from your
description which has 5 items.


Best regards,
Krzysztof