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

[PATCH v4 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.

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

Signed-off-by: Sarah Walker <sarah.walker@imgtec.com>
---
 .../devicetree/bindings/gpu/img,powervr.yaml  | 68 +++++++++++++++++++
 MAINTAINERS                                   |  7 ++
 2 files changed, 75 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..3292a0440465
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpu/img,powervr.yaml
@@ -0,0 +1,68 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (c) 2022 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:
+    items:
+      - description: GPU interrupt
+
+  interrupt-names:
+    items:
+      - const: gpu
+
+  power-domains:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - interrupts
+  - interrupt-names
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    gpu: gpu@fd00000 {
+        compatible = "ti,am62-gpu", "img,powervr-seriesaxe";
+        reg = <0x0fd00000 0x20000>;
+        power-domains = <&some_pds 187>;
+        clocks = <&k3_clks 187 0>;
+        clock-names = "core";
+        interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>;
+        interrupt-names = "gpu";
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 9852d6bfdb95..0763388b31ef 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10099,6 +10099,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 v4 02/17] dt-bindings: gpu: Add Imagination Technologies PowerVR GPU

[Conor Dooley]

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

Hey Sarah,

Your series does not appear to be threaded. `git send-email` can be
passed, for example, a directory containing a whole series & will set
the correct in-reply-to headers so that the series is in a single
thread.

On Fri, Jul 14, 2023 at 03:25:26PM +0100, Sarah Walker wrote:
> Add the device tree binding documentation for the Series AXE GPU used in
> TI AM62 SoCs.

> 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

The changelog should go below the --- line.

> Signed-off-by: Sarah Walker <sarah.walker@imgtec.com>
> ---
>  .../devicetree/bindings/gpu/img,powervr.yaml  | 68 +++++++++++++++++++
>  MAINTAINERS                                   |  7 ++
>  2 files changed, 75 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..3292a0440465
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpu/img,powervr.yaml
> @@ -0,0 +1,68 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (c) 2022 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>

> +  interrupts:
> +    items:
> +      - description: GPU interrupt

The description here doesn't add any value, since there is only one
interrupt, so you can drop it and do maxItems: 1 as you have done
elsewhere.

> +  interrupt-names:
> +    items:
> +      - const: gpu

And this
items:
  - const: gpu
can just be
const: gpu

Although, if there is only one interrupt this is probably not
particularly helpful. Are there other implementations of this IP that
have more interrupts?

Otherwise, this looks good to me.

Thanks,
Conor.

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

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

[Krzysztof Kozlowski]

On 14/07/2023 16:25, Sarah Walker wrote:
> Add the device tree binding documentation for the Series AXE GPU used in
> TI AM62 SoCs.
> 

...

> +
> +  clocks:
> +    minItems: 1
> +    maxItems: 3
> +
> +  clock-names:
> +    items:
> +      - const: core
> +      - const: mem
> +      - const: sys
> +    minItems: 1

Why clocks for this device vary? That's really unusual to have a SoC IP
block which can have a clock physically disconnected, depending on the
board (not SoC!).


Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 14/07/2023 16:25, Sarah Walker wrote:
> Add the device tree binding documentation for the Series AXE GPU used in
> TI AM62 SoCs.
> 

...

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    gpu: gpu@fd00000 {
> +        compatible = "ti,am62-gpu", "img,powervr-seriesaxe";
> +        reg = <0x0fd00000 0x20000>;
> +        power-domains = <&some_pds 187>;
> +        clocks = <&k3_clks 187 0>;
> +        clock-names = "core";
> +        interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>;
> +        interrupt-names = "gpu";

Why does it differ from your DTS?

Best regards,
Krzysztof

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

[Frank Binns]

Hi Conor,

Thank you for your feedback (comments below).

On Sat, 2023-07-15 at 11:40 +0100, Conor Dooley wrote:
> Hey Sarah,
> 
> Your series does not appear to be threaded. `git send-email` can be
> passed, for example, a directory containing a whole series & will set
> the correct in-reply-to headers so that the series is in a single
> thread.

Thank you pointing this out, we'll make sure we do this for the next iteration.

> 
> On Fri, Jul 14, 2023 at 03:25:26PM +0100, Sarah Walker wrote:
> > Add the device tree binding documentation for the Series AXE GPU used in
> > TI AM62 SoCs.
> > 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
> 
> The changelog should go below the --- line.

Ack

> 
> > Signed-off-by: Sarah Walker <sarah.walker@imgtec.com>
> > ---
> >  .../devicetree/bindings/gpu/img,powervr.yaml  | 68 +++++++++++++++++++
> >  MAINTAINERS                                   |  7 ++
> >  2 files changed, 75 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..3292a0440465
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpu/img,powervr.yaml
> > @@ -0,0 +1,68 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +# Copyright (c) 2022 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>
> > +  interrupts:
> > +    items:
> > +      - description: GPU interrupt
> 
> The description here doesn't add any value, since there is only one
> interrupt, so you can drop it and do maxItems: 1 as you have done
> elsewhere.

Sure, will make this change.

> 
> > +  interrupt-names:
> > +    items:
> > +      - const: gpu
> 
> And this
> items:
>   - const: gpu
> can just be
> const: gpu
> 
> Although, if there is only one interrupt this is probably not
> particularly helpful. Are there other implementations of this IP that
> have more interrupts?

No, all our current GPUs just have a single interrupt. I assume it's more future
proof to keep the name in case that ever changes? As in, by having the name now
we can make it a required property, which I guess we won't be able to do at some
later point.

Thanks
Frank

> 
> Otherwise, this looks good to me.
> 
> Thanks,
> Conor.

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

[Krzysztof Kozlowski]

On 18/07/2023 13:08, Frank Binns wrote:
>> And this
>> items:
>>   - const: gpu
>> can just be
>> const: gpu
>>
>> Although, if there is only one interrupt this is probably not
>> particularly helpful. Are there other implementations of this IP that
>> have more interrupts?
> 
> No, all our current GPUs just have a single interrupt. I assume it's more future
> proof to keep the name in case that ever changes? 

Why do you need name in the first place? If there is single entry, the
name is pointless, especially if it repeats the name of the IP block.

> As in, by having the name now
> we can make it a required property, which I guess we won't be able to do at some
> later point.

Why even making it required?

Best regards,
Krzysztof

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

[mripard]

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

Hi,

To expand on what Krzysztof said

On Tue, Jul 18, 2023 at 01:10:14PM +0200, Krzysztof Kozlowski wrote:
> On 18/07/2023 13:08, Frank Binns wrote:
> >> And this
> >> items:
> >>   - const: gpu
> >> can just be
> >> const: gpu
> >>
> >> Although, if there is only one interrupt this is probably not
> >> particularly helpful. Are there other implementations of this IP that
> >> have more interrupts?
> > 
> > No, all our current GPUs just have a single interrupt. I assume it's more future
> > proof to keep the name in case that ever changes? 
> 
> Why do you need name in the first place? If there is single entry, the
> name is pointless, especially if it repeats the name of the IP block.

Generally speaking, if there's only one resource (interrupt, clock, etc)
we don't put any discriminant.

If you need to extend it later on for some reason, then you'll add an
interrupt-names property and you can either require it for that new GPU
or whatever, or fallback on the nameless on if no name was found.

> > As in, by having the name now
> > we can make it a required property, which I guess we won't be able to do at some
> > later point.
> 
> Why even making it required?

There's no issue with introducing a new property later on if a GPU needs
it. Then, you can either make it required only for that particular
generation, or you can have some fallback case.

Both are fine and easy to do.

Maxime

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

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

[Frank Binns]

Hi Krzysztof,

On Mon, 2023-07-17 at 09:29 +0200, Krzysztof Kozlowski wrote:
> On 14/07/2023 16:25, Sarah Walker wrote:
> > Add the device tree binding documentation for the Series AXE GPU used in
> > TI AM62 SoCs.
> > 
> 
> ...
> 
> > +
> > +  clocks:
> > +    minItems: 1
> > +    maxItems: 3
> > +
> > +  clock-names:
> > +    items:
> > +      - const: core
> > +      - const: mem
> > +      - const: sys
> > +    minItems: 1
> 
> Why clocks for this device vary? That's really unusual to have a SoC IP
> block which can have a clock physically disconnected, depending on the
> board (not SoC!).

By default, this GPU IP (Series AXE) operates on a single clock (the core
clock), but the SoC vendor can choose at IP integration time to run the memory
and SoC interfaces on separate clocks (mem and sys clocks respectively). We also
have IP, such as the Series 6XT, that requires all 3 clocks.

So the situation here is that Series AXE may have 1 or 3 clocks, but the TI
implementation being added only has 1.

I guess we need to add something like:

  allOf:
    - if:
        properties:
          compatible:
            contains:
              const: ti,am62-gpu
      then:
        properties:
          clocks:
            maxItems: 1

Or should we be doing something else?

Thanks
Frank

> 
> 
> Best regards,
> Krzysztof
> 

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

[Frank Binns]

Hi Krzysztof,

On Tue, 2023-07-18 at 13:10 +0200, Krzysztof Kozlowski wrote:
> On 18/07/2023 13:08, Frank Binns wrote:
> > > And this
> > > items:
> > >   - const: gpu
> > > can just be
> > > const: gpu
> > > 
> > > Although, if there is only one interrupt this is probably not
> > > particularly helpful. Are there other implementations of this IP that
> > > have more interrupts?
> > 
> > No, all our current GPUs just have a single interrupt. I assume it's more future
> > proof to keep the name in case that ever changes? 
> 
> Why do you need name in the first place? If there is single entry, the
> name is pointless, especially if it repeats the name of the IP block.
> 
> > As in, by having the name now
> > we can make it a required property, which I guess we won't be able to do at some
> > later point.
> 
> Why even making it required?

It seems nicer to look up a resource in the driver based on a name rather than
an index. Happy to drop it though.

Thanks
Frank

> 
> Best regards,
> Krzysztof
> 

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

[Frank Binns]

Hi Krzysztof,

On Tue, 2023-07-18 at 08:20 +0200, Krzysztof Kozlowski wrote:
> On 14/07/2023 16:25, Sarah Walker wrote:
> > Add the device tree binding documentation for the Series AXE GPU used in
> > TI AM62 SoCs.
> > 
> 
> ...
> 
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    gpu: gpu@fd00000 {
> > +        compatible = "ti,am62-gpu", "img,powervr-seriesaxe";
> > +        reg = <0x0fd00000 0x20000>;
> > +        power-domains = <&some_pds 187>;
> > +        clocks = <&k3_clks 187 0>;
> > +        clock-names = "core";
> > +        interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>;
> > +        interrupt-names = "gpu";
> 
> Why does it differ from your DTS?

This is just an oversight on our part. We'll make sure they both match in the
next version.

Thanks
Frank

> 
> Best regards,
> Krzysztof
> 

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

[Krzysztof Kozlowski]

On 18/07/2023 13:36, Frank Binns wrote:
> Hi Krzysztof,
> 
> On Tue, 2023-07-18 at 13:10 +0200, Krzysztof Kozlowski wrote:
>> On 18/07/2023 13:08, Frank Binns wrote:
>>>> And this
>>>> items:
>>>>   - const: gpu
>>>> can just be
>>>> const: gpu
>>>>
>>>> Although, if there is only one interrupt this is probably not
>>>> particularly helpful. Are there other implementations of this IP that
>>>> have more interrupts?
>>>
>>> No, all our current GPUs just have a single interrupt. I assume it's more future
>>> proof to keep the name in case that ever changes? 
>>
>> Why do you need name in the first place? If there is single entry, the
>> name is pointless, especially if it repeats the name of the IP block.
>>
>>> As in, by having the name now
>>> we can make it a required property, which I guess we won't be able to do at some
>>> later point.
>>
>> Why even making it required?
> 
> It seems nicer to look up a resource in the driver based on a name rather than
> an index. 

Not really... Slower and confuses people, because then they think order
is flexible.


Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 18/07/2023 13:47, Frank Binns wrote:
> Hi Krzysztof,
> 
> On Tue, 2023-07-18 at 08:20 +0200, Krzysztof Kozlowski wrote:
>> On 14/07/2023 16:25, Sarah Walker wrote:
>>> Add the device tree binding documentation for the Series AXE GPU used in
>>> TI AM62 SoCs.
>>>
>>
>> ...
>>
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/interrupt-controller/irq.h>
>>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>>> +
>>> +    gpu: gpu@fd00000 {
>>> +        compatible = "ti,am62-gpu", "img,powervr-seriesaxe";
>>> +        reg = <0x0fd00000 0x20000>;
>>> +        power-domains = <&some_pds 187>;
>>> +        clocks = <&k3_clks 187 0>;
>>> +        clock-names = "core";
>>> +        interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>;
>>> +        interrupt-names = "gpu";
>>
>> Why does it differ from your DTS?
> 
> This is just an oversight on our part. We'll make sure they both match in the
> next version.
> 

Just test your DTS before sending. You would see errors and there is no
need to involve manual reviewing. It is always better to use tools than
reviewers time. Otherwise, please kindly donate your time by helping to
review other patches.

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 18/07/2023 13:32, Frank Binns wrote:
> Hi Krzysztof,
> 
> On Mon, 2023-07-17 at 09:29 +0200, Krzysztof Kozlowski wrote:
>> On 14/07/2023 16:25, Sarah Walker wrote:
>>> Add the device tree binding documentation for the Series AXE GPU used in
>>> TI AM62 SoCs.
>>>
>>
>> ...
>>
>>> +
>>> +  clocks:
>>> +    minItems: 1
>>> +    maxItems: 3
>>> +
>>> +  clock-names:
>>> +    items:
>>> +      - const: core
>>> +      - const: mem
>>> +      - const: sys
>>> +    minItems: 1
>>
>> Why clocks for this device vary? That's really unusual to have a SoC IP
>> block which can have a clock physically disconnected, depending on the
>> board (not SoC!).
> 
> By default, this GPU IP (Series AXE) operates on a single clock (the core
> clock), but the SoC vendor can choose at IP integration time to run the memory
> and SoC interfaces on separate clocks (mem and sys clocks respectively). We also
> have IP, such as the Series 6XT, that requires all 3 clocks.

Currently you have only one SoC vendor with only one SoC, so the clocks
do not vary. Describing the clocks for all possible variants is a good
idea, but then this should be clear that this implementation uses subset.

> 
> So the situation here is that Series AXE may have 1 or 3 clocks, but the TI
> implementation being added only has 1.
> 
> I guess we need to add something like:
> 
>   allOf:
>     - if:
>         properties:
>           compatible:
>             contains:
>               const: ti,am62-gpu
>       then:
>         properties:
>           clocks:
>             maxItems: 1
> 
> Or should we be doing something else?

Yes. clock-names as well..


Best regards,
Krzysztof