[PATCH v5 02/17] dt-bindings: gpu: Add Imagination Technologies PowerVR GPU

[PATCH v5 02/17] dt-bindings: gpu: Add Imagination Technologies PowerVR GPU

[Sarah Walker]

Add the device tree binding documentation for the Series AXE GPU used in
TI AM62 SoCs.

Co-developed-by: Frank Binns <frank.binns@imgtec.com>
Signed-off-by: Frank Binns <frank.binns@imgtec.com>
Signed-off-by: Sarah Walker <sarah.walker@imgtec.com>
---
Changes since v4:
- Add clocks constraint for ti,am62-gpu
- Remove excess address and size cells in example
- Remove interrupt name and add maxItems
- Make property order consistent between dts and bindings doc
- Update example to match dts

Changes since v3:
- Remove oneOf in compatible property
- Remove power-supply (not used on AM62)

Changes since v2:
- Add commit message description
- Remove mt8173-gpu support (not currently supported)
- Drop quotes from $id and $schema
- Remove reg: minItems
- Drop _clk suffixes from clock-names
- Remove operating-points-v2 property and cooling-cells (not currently
  used)
- Add additionalProperties: false
- Remove stray blank line at the end of file

 .../devicetree/bindings/gpu/img,powervr.yaml  | 75 +++++++++++++++++++
 MAINTAINERS                                   |  7 ++
 2 files changed, 82 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpu/img,powervr.yaml

diff --git a/Documentation/devicetree/bindings/gpu/img,powervr.yaml b/Documentation/devicetree/bindings/gpu/img,powervr.yaml
new file mode 100644
index 000000000000..40ade5ceef6e
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpu/img,powervr.yaml
@@ -0,0 +1,75 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (c) 2023 Imagination Technologies Ltd.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpu/img,powervr.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Imagination Technologies PowerVR GPU
+
+maintainers:
+  - Sarah Walker <sarah.walker@imgtec.com>
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - ti,am62-gpu
+      - const: img,powervr-seriesaxe
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    minItems: 1
+    maxItems: 3
+
+  clock-names:
+    items:
+      - const: core
+      - const: mem
+      - const: sys
+    minItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  power-domains:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - interrupts
+
+additionalProperties: false
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: ti,am62-gpu
+    then:
+      properties:
+        clocks:
+          maxItems: 1
+        clock-names:
+          const: core
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/soc/ti,sci_pm_domain.h>
+
+    gpu: gpu@fd00000 {
+        compatible = "ti,am62-gpu", "img,powervr-seriesaxe";
+        reg = <0x0fd00000 0x20000>;
+        clocks = <&k3_clks 187 0>;
+        clock-names = "core";
+        interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>;
+        power-domains = <&k3_pds 187 TI_SCI_PD_EXCLUSIVE>;
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index cd882b87a3c6..f84390bb6cfe 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10138,6 +10138,13 @@ IMGTEC IR DECODER DRIVER
 S:	Orphan
 F:	drivers/media/rc/img-ir/
 
+IMGTEC POWERVR DRM DRIVER
+M:	Frank Binns <frank.binns@imgtec.com>
+M:	Sarah Walker <sarah.walker@imgtec.com>
+M:	Donald Robson <donald.robson@imgtec.com>
+S:	Supported
+F:	Documentation/devicetree/bindings/gpu/img,powervr.yaml
+
 IMON SOUNDGRAPH USB IR RECEIVER
 M:	Sean Young <sean@mess.org>
 L:	linux-media@vger.kernel.org
-- 
2.41.0

Re: [PATCH v5 02/17] dt-bindings: gpu: Add Imagination Technologies PowerVR GPU

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 4144 bytes --]

On Wed, Aug 16, 2023 at 09:27:25AM +0100, Sarah Walker wrote:
> Add the device tree binding documentation for the Series AXE GPU used in
> TI AM62 SoCs.
> 
> Co-developed-by: Frank Binns <frank.binns@imgtec.com>
> Signed-off-by: Frank Binns <frank.binns@imgtec.com>
> Signed-off-by: Sarah Walker <sarah.walker@imgtec.com>

This seems okay to me,
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

> ---
> Changes since v4:
> - Add clocks constraint for ti,am62-gpu
> - Remove excess address and size cells in example
> - Remove interrupt name and add maxItems
> - Make property order consistent between dts and bindings doc
> - Update example to match dts
> 
> Changes since v3:
> - Remove oneOf in compatible property
> - Remove power-supply (not used on AM62)
> 
> Changes since v2:
> - Add commit message description
> - Remove mt8173-gpu support (not currently supported)
> - Drop quotes from $id and $schema
> - Remove reg: minItems
> - Drop _clk suffixes from clock-names
> - Remove operating-points-v2 property and cooling-cells (not currently
>   used)
> - Add additionalProperties: false
> - Remove stray blank line at the end of file
> 
>  .../devicetree/bindings/gpu/img,powervr.yaml  | 75 +++++++++++++++++++
>  MAINTAINERS                                   |  7 ++
>  2 files changed, 82 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpu/img,powervr.yaml
> 
> diff --git a/Documentation/devicetree/bindings/gpu/img,powervr.yaml b/Documentation/devicetree/bindings/gpu/img,powervr.yaml
> new file mode 100644
> index 000000000000..40ade5ceef6e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpu/img,powervr.yaml
> @@ -0,0 +1,75 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (c) 2023 Imagination Technologies Ltd.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpu/img,powervr.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Imagination Technologies PowerVR GPU
> +
> +maintainers:
> +  - Sarah Walker <sarah.walker@imgtec.com>
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - ti,am62-gpu
> +      - const: img,powervr-seriesaxe
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    minItems: 1
> +    maxItems: 3
> +
> +  clock-names:
> +    items:
> +      - const: core
> +      - const: mem
> +      - const: sys
> +    minItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  power-domains:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - interrupts
> +
> +additionalProperties: false
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: ti,am62-gpu
> +    then:
> +      properties:
> +        clocks:
> +          maxItems: 1
> +        clock-names:
> +          const: core
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/soc/ti,sci_pm_domain.h>
> +
> +    gpu: gpu@fd00000 {
> +        compatible = "ti,am62-gpu", "img,powervr-seriesaxe";
> +        reg = <0x0fd00000 0x20000>;
> +        clocks = <&k3_clks 187 0>;
> +        clock-names = "core";
> +        interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>;
> +        power-domains = <&k3_pds 187 TI_SCI_PD_EXCLUSIVE>;
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index cd882b87a3c6..f84390bb6cfe 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10138,6 +10138,13 @@ IMGTEC IR DECODER DRIVER
>  S:	Orphan
>  F:	drivers/media/rc/img-ir/
>  
> +IMGTEC POWERVR DRM DRIVER
> +M:	Frank Binns <frank.binns@imgtec.com>
> +M:	Sarah Walker <sarah.walker@imgtec.com>
> +M:	Donald Robson <donald.robson@imgtec.com>
> +S:	Supported
> +F:	Documentation/devicetree/bindings/gpu/img,powervr.yaml
> +
>  IMON SOUNDGRAPH USB IR RECEIVER
>  M:	Sean Young <sean@mess.org>
>  L:	linux-media@vger.kernel.org
> -- 
> 2.41.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v5 02/17] dt-bindings: gpu: Add Imagination Technologies PowerVR GPU

[Rob Herring]

On Wed, Aug 16, 2023 at 09:27:25AM +0100, Sarah Walker wrote:
> Add the device tree binding documentation for the Series AXE GPU used in
> TI AM62 SoCs.
> 
> Co-developed-by: Frank Binns <frank.binns@imgtec.com>
> Signed-off-by: Frank Binns <frank.binns@imgtec.com>
> Signed-off-by: Sarah Walker <sarah.walker@imgtec.com>
> ---
> Changes since v4:
> - Add clocks constraint for ti,am62-gpu
> - Remove excess address and size cells in example
> - Remove interrupt name and add maxItems
> - Make property order consistent between dts and bindings doc
> - Update example to match dts
> 
> Changes since v3:
> - Remove oneOf in compatible property
> - Remove power-supply (not used on AM62)
> 
> Changes since v2:
> - Add commit message description
> - Remove mt8173-gpu support (not currently supported)
> - Drop quotes from $id and $schema
> - Remove reg: minItems
> - Drop _clk suffixes from clock-names
> - Remove operating-points-v2 property and cooling-cells (not currently
>   used)
> - Add additionalProperties: false
> - Remove stray blank line at the end of file
> 
>  .../devicetree/bindings/gpu/img,powervr.yaml  | 75 +++++++++++++++++++
>  MAINTAINERS                                   |  7 ++
>  2 files changed, 82 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpu/img,powervr.yaml
> 
> diff --git a/Documentation/devicetree/bindings/gpu/img,powervr.yaml b/Documentation/devicetree/bindings/gpu/img,powervr.yaml
> new file mode 100644
> index 000000000000..40ade5ceef6e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpu/img,powervr.yaml
> @@ -0,0 +1,75 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (c) 2023 Imagination Technologies Ltd.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpu/img,powervr.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Imagination Technologies PowerVR GPU
> +
> +maintainers:
> +  - Sarah Walker <sarah.walker@imgtec.com>
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - ti,am62-gpu
> +      - const: img,powervr-seriesaxe
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    minItems: 1
> +    maxItems: 3
> +
> +  clock-names:
> +    items:
> +      - const: core
> +      - const: mem
> +      - const: sys
> +    minItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  power-domains:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - interrupts
> +
> +additionalProperties: false
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: ti,am62-gpu
> +    then:
> +      properties:
> +        clocks:
> +          maxItems: 1
> +        clock-names:
> +          const: core

The main section already defined the name, so just 'maxItems: 1' here.

With that, 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v5 02/17] dt-bindings: gpu: Add Imagination Technologies PowerVR GPU

[Linus Walleij]

Hi Sarah,

thanks for your patch!

Patches adding device tree bindings need to be CC:ed to
devicetree@vger.kernel.org
and the DT binding maintainers, I have added it for now.

On Wed, Aug 16, 2023 at 10:26 AM Sarah Walker <sarah.walker@imgtec.com> wrote:

> Add the device tree binding documentation for the Series AXE GPU used in
> TI AM62 SoCs.
>
> Co-developed-by: Frank Binns <frank.binns@imgtec.com>
> Signed-off-by: Frank Binns <frank.binns@imgtec.com>
> Signed-off-by: Sarah Walker <sarah.walker@imgtec.com>
(...)
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - ti,am62-gpu
> +      - const: img,powervr-seriesaxe

Should there not at least be a dash there?

img,powervr-series-axe?

It is spelled in two words in the commit message,
Series AXE not SeriesAXE?

Moreover, if this pertains to the AXE-1-16 and AXE-2-16 it is kind of a wildcard
and we usually don't do that, I would use the exact version instead,
such as:
const: img,powervr-axe-1-16
any reason not to do this?

I asked about the relationship between these strings and the product
designations earlier I think :/

Yours,
Linus Walleij

Re: [PATCH v5 02/17] dt-bindings: gpu: Add Imagination Technologies PowerVR GPU

[Krzysztof Kozlowski]

On 18/08/2023 11:36, Linus Walleij wrote:
> Hi Sarah,
> 
> thanks for your patch!
> 
> Patches adding device tree bindings need to be CC:ed to
> devicetree@vger.kernel.org
> and the DT binding maintainers, I have added it for now.
> 

This won't help, I think. Patch will not be tested.

I was already asking for using get_maintainers in the past... sigh...

Best regards,
Krzysztof

Re: [PATCH v5 02/17] dt-bindings: gpu: Add Imagination Technologies PowerVR GPU

[Krzysztof Kozlowski]

On 16/08/2023 10:27, Sarah Walker wrote:
> Add the device tree binding documentation for the Series AXE GPU used in
> TI AM62 SoCs.
> 
> Co-developed-by: Frank Binns <frank.binns@imgtec.com>
> Signed-off-by: Frank Binns <frank.binns@imgtec.com>
> Signed-off-by: Sarah Walker <sarah.walker@imgtec.com>

This is a duplicated patch sent only to few people. The original patch
was sent to other people.

Such process won't work. You cannot send two of the same patches to
different set of people and expect they magically merge somewhere in the
cloud.

Best regards,
Krzysztof

Re: [PATCH v5 02/17] dt-bindings: gpu: Add Imagination Technologies PowerVR GPU

[Frank Binns]

Hi Linus,

Thank you for your feedback (comments below).

On Fri, 2023-08-18 at 11:36 +0200, Linus Walleij wrote:
> Hi Sarah,
> 
> thanks for your patch!
> 
> Patches adding device tree bindings need to be CC:ed to
> devicetree@vger.kernel.org
> and the DT binding maintainers, I have added it for now.

Thank you - it looks like something went wrong when the patch was sent.

> 
> On Wed, Aug 16, 2023 at 10:26 AM Sarah Walker <sarah.walker@imgtec.com> wrote:
> 
> > Add the device tree binding documentation for the Series AXE GPU used in
> > TI AM62 SoCs.
> > 
> > Co-developed-by: Frank Binns <frank.binns@imgtec.com>
> > Signed-off-by: Frank Binns <frank.binns@imgtec.com>
> > Signed-off-by: Sarah Walker <sarah.walker@imgtec.com>
> (...)
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - ti,am62-gpu
> > +      - const: img,powervr-seriesaxe
> 
> Should there not at least be a dash there?
> 
> img,powervr-series-axe?
> 
> It is spelled in two words in the commit message,
> Series AXE not SeriesAXE?

We've now changed the string to address your earlier feedback (see below).

> 
> Moreover, if this pertains to the AXE-1-16 and AXE-2-16 it is kind of a wildcard
> and we usually don't do that, I would use the exact version instead,
> such as:
> const: img,powervr-axe-1-16
> any reason not to do this?

The exact GPU model/revision is fully discoverable via a register. We saw the
same is also true for Mali Bifrost, where they have a single string covering
multiple models [1], so we took the same approach. We'll add a comment in v6
along the lines of the one in the Mali Bifrost bindings.

> 
> I asked about the relationship between these strings and the product
> designations earlier I think :/

Sorry about that, I honestly thought we'd addressed that bit of feedback by
changing the compatibility string, but clearly we hadn't :( Thank you for
catching this.

We've now changed the string to "img,img-axe" to align with the marketing name,
along with updating the commit message and various other places to refer to
PowerVR and IMG GPUs (the DRM driver supporting both).

Thanks
Frank

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml?h=v6.5#n29