[v3,02/17] dt-bindings: gpu: Add Imagination Technologies PowerVR GPU

Commit Message

Sarah Walker June 13, 2023, 2:47 p.m. UTC

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

Signed-off-by: Sarah Walker <[e-mail]>
---
 .../devicetree/bindings/gpu/img,powervr.yaml  | 71 +++++++++++++++++++
 MAINTAINERS                                   |  7 ++
 2 files changed, 78 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpu/img,powervr.yaml

Comments

Andrew Davis June 13, 2023, 5:38 p.m. UTC | #1

On 6/13/23 9:47 AM, Sarah Walker wrote:
> Add the device tree binding documentation for the Series AXE GPU used in
> TI AM62 SoCs.
> 
> Signed-off-by: Sarah Walker <[e-mail]>
> ---
>   .../devicetree/bindings/gpu/img,powervr.yaml  | 71 +++++++++++++++++++
>   MAINTAINERS                                   |  7 ++
>   2 files changed, 78 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..652343876d1c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpu/img,powervr.yaml
> @@ -0,0 +1,71 @@
> +# 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 <[e-mail]>
> +
> +properties:
> +  compatible:
> +    oneOf:

oneOf shouldn't be needed, you can just do the enum followed by const.

> +      - 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
> +
> +  power-supply: true

Why do you need power-supply?

Andrew

> +
> +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 b344e1318ac3..a41517843a10 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10084,6 +10084,13 @@ IMGTEC IR DECODER DRIVER
>   S:	Orphan
>   F:	drivers/media/rc/img-ir/
>   
> +IMGTEC POWERVR DRM DRIVER
> +M:	Frank Binns <[e-mail]>
> +M:	Sarah Walker <[e-mail]>
> +M:	Donald Robson <[e-mail]>
> +S:	Supported
> +F:	Documentation/devicetree/bindings/gpu/img,powervr.yaml
> +
>   IMON SOUNDGRAPH USB IR RECEIVER
>   M:	Sean Young <[e-mail]>
>   L:	[e-mail]

Krzysztof Kozlowski June 13, 2023, 6:24 p.m. UTC | #2

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

I don't see improvements. That's a NAK :(

This is a friendly reminder during the review process.

It seems my previous comments were not fully addressed. Maybe my
feedback got lost between the quotes, maybe you just forgot to apply it.
Please go back to the previous discussion and either implement all
requested changes or keep discussing them.

Thank you.

Best regards,
Krzysztof

Frank Binns June 14, 2023, 2:34 p.m. UTC | #3

Hi Krzysztof,

On Tue, 2023-06-13 at 20:24 +0200, Krzysztof Kozlowski wrote:
> On 13/06/2023 16:47, Sarah Walker wrote:
> > Add the device tree binding documentation for the Series AXE GPU used in
> > TI AM62 SoCs.
> > 
> 
> I don't see improvements. That's a NAK :(
> 
> This is a friendly reminder during the review process.
> 
> It seems my previous comments were not fully addressed. Maybe my
> feedback got lost between the quotes, maybe you just forgot to apply it.
> Please go back to the previous discussion and either implement all
> requested changes or keep discussing them.
> 

Apologies for not including a change log for this patch and for not highlighting
that we'd made changes in this area in the covering letter as well. This was an
oversight on our part.

The change log for this patch is as follows:
* Added commit message description
* Dropped quotes from $id and $schema
* Dropped reg minItems
* Dropped _clk suffixes from clock-names
* Dropped operating-points-v2 property
* Added missing additionalProperties:false
* Removed stray blank line at end of file

We'll be sure to include this information going forwards.

We've also run 'make dt_binding_check' and there are no reported issues that we
can see.

As far as I'm aware, this should cover all your feedback on the previous version
of the patch. Of course, we may have missed something or unintentionally
introduced more issues. We'd really appreciate if you can highlight anything
else that needs fixing and we'll make sure we address it in the next iteration
and/or respond to individual points where necessary.

Thank you for your feedback so far.

Best regards,
Frank

> Thank you.
> 
> Best regards,
> Krzysztof
>

Rob Herring (Arm) June 15, 2023, 8:50 p.m. UTC | #4

On Tue, Jun 13, 2023 at 9:20 AM Sarah Walker <[e-mail]> wrote:
>
> Add the device tree binding documentation for the Series AXE GPU used in
> TI AM62 SoCs.
>
> Signed-off-by: Sarah Walker <[e-mail]>
> ---
>  .../devicetree/bindings/gpu/img,powervr.yaml  | 71 +++++++++++++++++++
>  MAINTAINERS                                   |  7 ++
>  2 files changed, 78 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpu/img,powervr.yaml

Please use get_maintainers.pl and send your patches to the correct
people and lists or they won't get reviewed.

Rob

Frank Binns June 16, 2023, 11:23 a.m. UTC | #5

Hi Andrew,

Thank you for your feedback (comments below).

On Tue, 2023-06-13 at 12:38 -0500, Andrew Davis wrote:
> On 6/13/23 9:47 AM, Sarah Walker wrote:
> > Add the device tree binding documentation for the Series AXE GPU used in
> > TI AM62 SoCs.
> > 
> > Signed-off-by: Sarah Walker <[e-mail]>
> > ---
> >   .../devicetree/bindings/gpu/img,powervr.yaml  | 71 +++++++++++++++++++
> >   MAINTAINERS                                   |  7 ++
> >   2 files changed, 78 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..652343876d1c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpu/img,powervr.yaml
> > @@ -0,0 +1,71 @@
> > +# 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 <[e-mail]>
> > +
> > +properties:
> > +  compatible:
> > +    oneOf:
> 
> oneOf shouldn't be needed, you can just do the enum followed by const.

Sure, we'll make this change.

> 
> > +      - 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
> > +
> > +  power-supply: true
> 
> Why do you need power-supply?

We don't need this, so will remove it in the next iteration.

Thanks
Frank

> 
> Andrew
> 
> > +
> > +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 b344e1318ac3..a41517843a10 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -10084,6 +10084,13 @@ IMGTEC IR DECODER DRIVER
> >   S:	Orphan
> >   F:	drivers/media/rc/img-ir/
> >   
> > +IMGTEC POWERVR DRM DRIVER
> > +M:	Frank Binns <[e-mail]>
> > +M:	Sarah Walker <[e-mail]>
> > +M:	Donald Robson <[e-mail]>
> > +S:	Supported
> > +F:	Documentation/devicetree/bindings/gpu/img,powervr.yaml
> > +
> >   IMON SOUNDGRAPH USB IR RECEIVER
> >   M:	Sean Young <[e-mail]>
> >   L:	[e-mail]

Frank Binns June 16, 2023, 11:27 a.m. UTC | #6

Hi Rob,

On Thu, 2023-06-15 at 14:50 -0600, Rob Herring wrote:
> On Tue, Jun 13, 2023 at 9:20 AM Sarah Walker <[e-mail]> wrote:
> > Add the device tree binding documentation for the Series AXE GPU used in
> > TI AM62 SoCs.
> > 
> > Signed-off-by: Sarah Walker <[e-mail]>
> > ---
> >  .../devicetree/bindings/gpu/img,powervr.yaml  | 71 +++++++++++++++++++
> >  MAINTAINERS                                   |  7 ++
> >  2 files changed, 78 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/gpu/img,powervr.yaml
> 
> Please use get_maintainers.pl and send your patches to the correct
> people and lists or they won't get reviewed.

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

Thanks
Frank

> 
> Rob

Linus Walleij June 16, 2023, 12:48 p.m. UTC | #7

Hi Sarah,

thanks for your patch!

On Tue, Jun 13, 2023 at 5:20 PM Sarah Walker <[e-mail]> wrote:

> Add the device tree binding documentation for the Series AXE GPU used in
> TI AM62 SoCs.
>
> Signed-off-by: Sarah Walker <[e-mail]>

> +properties:
> +  compatible:
> +    oneOf:
> +      - items:
> +          - enum:
> +              - ti,am62-gpu
> +          - const: img,powervr-seriesaxe

I don't see why you need to restrict the bindings to just the stuff
you happen to
be writing Linux drivers for right now.

Add all of them!

There is this out-of-tree binding:
https://github.com/openpvrsgx-devgroup/linux_openpvrsgx/commit/0ddd308a78926782b8a72f75c74b91417ceb9779

With these two on top:
https://github.com/openpvrsgx-devgroup/linux_openpvrsgx/commit/053346e1933932815066f16ebf6e6bda45d67548
https://github.com/openpvrsgx-devgroup/linux_openpvrsgx/commit/1cb62c4cdcad096d438ee7d1d51f6001998acee3

They are indeed out-of-tree, but H. Nikolaus is a well-known and respected
contributor to the kernel, and I'm pretty sure he would be contributing
these upstream if he had the time and incentive.

To me it seems much more like you should talk to Nikolaus about submitting
these patches initially, and then add Rogue support with patches on top of it.
It could be a nice series with just DT bindings.

I see that your binding uses a power domain rather than a regulator
(sgx-supply) for power and that is probably a better approach but other
than that the openpvrsgx binding looks more complete and to the point?

It will also help them to get these bindings settled so they can merge all
of the DTS patches adding the SGX block to misc platforms in the
kernel upstream so they can focus their work on the actual driver.

When I look at the PowerVR wikipedia page:
https://en.wikipedia.org/wiki/PowerVR
there is no correspondence between the product names there and
"img,powervr-seriesaxe" as used here.

I think you need to explain if these are internal product names or what
is going on, and what the relationship is to the marketing names, it could
be part of the description simply, so that people know what string to use
somewhat intuitively. Maybe all the strings in the out-of-tree documentation
are just wrong because internal code names need to be used?

Yours,
Linus Walleij

Rob Herring (Arm) June 16, 2023, 2:23 p.m. UTC | #8

On Fri, Jun 16, 2023 at 6:49 AM Linus Walleij <[e-mail]> wrote:
>
> Hi Sarah,
>
> thanks for your patch!
>
> On Tue, Jun 13, 2023 at 5:20 PM Sarah Walker <[e-mail]> wrote:
>
> > Add the device tree binding documentation for the Series AXE GPU used in
> > TI AM62 SoCs.
> >
> > Signed-off-by: Sarah Walker <[e-mail]>
>
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - items:
> > +          - enum:
> > +              - ti,am62-gpu
> > +          - const: img,powervr-seriesaxe
>
> I don't see why you need to restrict the bindings to just the stuff
> you happen to
> be writing Linux drivers for right now.
>
> Add all of them!
>
> There is this out-of-tree binding:
> https://github.com/openpvrsgx-devgroup/linux_openpvrsgx/commit/0ddd308a78926782b8a72f75c74b91417ceb9779
>
> With these two on top:
> https://github.com/openpvrsgx-devgroup/linux_openpvrsgx/commit/053346e1933932815066f16ebf6e6bda45d67548
> https://github.com/openpvrsgx-devgroup/linux_openpvrsgx/commit/1cb62c4cdcad096d438ee7d1d51f6001998acee3
>
> They are indeed out-of-tree, but H. Nikolaus is a well-known and respected
> contributor to the kernel, and I'm pretty sure he would be contributing
> these upstream if he had the time and incentive.

He did try[1].

Rob

[1] https://lore.kernel.org/all/[e-mail]/

Frank Binns July 4, 2023, 3:13 p.m. UTC | #9

Hi Linus,

On Fri, 2023-06-16 at 14:48 +0200, Linus Walleij wrote:
> Hi Sarah,
> 
> thanks for your patch!
> 
> On Tue, Jun 13, 2023 at 5:20 PM Sarah Walker <[e-mail]> wrote:
> 
> > Add the device tree binding documentation for the Series AXE GPU used in
> > TI AM62 SoCs.
> > 
> > Signed-off-by: Sarah Walker <[e-mail]>
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - items:
> > +          - enum:
> > +              - ti,am62-gpu
> > +          - const: img,powervr-seriesaxe
> 
> I don't see why you need to restrict the bindings to just the stuff
> you happen to
> be writing Linux drivers for right now.
> 
> Add all of them!

The main thinking here was to start off with a single simple case (TI AM62) to
support the initial driver upstreaming rather than trying to cover too many
things at once. This can then be built upon for other GPUs. So, for example, we
can extend the bindings to add a second power domain for those that need it,
such as the GPU in the Mediatek MT8173. The benefit of this approach is that the
bindings, dts and code changes can all be reviewed together so that all the
context is present.

> 
> There is this out-of-tree binding:
> https://github.com/openpvrsgx-devgroup/linux_openpvrsgx/commit/0ddd308a78926782b8a72f75c74b91417ceb9779
> 
> With these two on top:
> https://github.com/openpvrsgx-devgroup/linux_openpvrsgx/commit/053346e1933932815066f16ebf6e6bda45d67548
> https://github.com/openpvrsgx-devgroup/linux_openpvrsgx/commit/1cb62c4cdcad096d438ee7d1d51f6001998acee3
> 
> They are indeed out-of-tree, but H. Nikolaus is a well-known and respected
> contributor to the kernel, and I'm pretty sure he would be contributing
> these upstream if he had the time and incentive.
> 
> To me it seems much more like you should talk to Nikolaus about submitting
> these patches initially, and then add Rogue support with patches on top of it.
> It could be a nice series with just DT bindings.
> 
> I see that your binding uses a power domain rather than a regulator
> (sgx-supply) for power and that is probably a better approach but other
> than that the openpvrsgx binding looks more complete and to the point?
> 
> It will also help them to get these bindings settled so they can merge all
> of the DTS patches adding the SGX block to misc platforms in the
> kernel upstream so they can focus their work on the actual driver.
> 
> When I look at the PowerVR wikipedia page:
> https://en.wikipedia.org/wiki/PowerVR
> there is no correspondence between the product names there and
> "img,powervr-seriesaxe" as used here.
> 
> I think you need to explain if these are internal product names or what
> is going on, and what the relationship is to the marketing names, it could
> be part of the description simply, so that people know what string to use
> somewhat intuitively. Maybe all the strings in the out-of-tree documentation
> are just wrong because internal code names need to be used?

It's been quite some time since we originally wrote these bindings, so we'll
need to go back and check why we settled on "powervr-seriesaxe", but I suspect
it was just a case of us normalising the naming across different series.

Thanks
Frank

> 
> Yours,
> Linus Walleij

Linus Walleij July 5, 2023, 7:08 a.m. UTC | #10

On Tue, Jul 4, 2023 at 5:13 PM Frank Binns <[e-mail]> wrote:

> > > Signed-off-by: Sarah Walker <[e-mail]>
> > > +properties:
> > > +  compatible:
> > > +    oneOf:
> > > +      - items:
> > > +          - enum:
> > > +              - ti,am62-gpu
> > > +          - const: img,powervr-seriesaxe
> >
> > I don't see why you need to restrict the bindings to just the stuff
> > you happen to
> > be writing Linux drivers for right now.
> >
> > Add all of them!
>
> The main thinking here was to start off with a single simple case (TI AM62) to
> support the initial driver upstreaming rather than trying to cover too many
> things at once. This can then be built upon for other GPUs. So, for example, we
> can extend the bindings to add a second power domain for those that need it,
> such as the GPU in the Mediatek MT8173. The benefit of this approach is that the
> bindings, dts and code changes can all be reviewed together so that all the
> context is present.

I understand that you want to scale down the problem so it becomes easier.

However the reverse can also be said: by removing the other platforms you
may end up with one way street bindings that are really hard to augment to
accomodate for the other family members later on.

But as long as H. Nikolaus is involved in the review I suppose he will be
able to point out obvious cases, and I expect that you read through the
old thread:
https://lore.kernel.org/all/[e-mail]/

Yours,
Linus Walleij

Patch

diff --git a/Documentation/devicetree/bindings/gpu/img,powervr.yaml b/Documentation/devicetree/bindings/gpu/img,powervr.yaml
new file mode 100644
index 000000000000..652343876d1c
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpu/img,powervr.yaml
@@ -0,0 +1,71 @@ 
+# 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 <[e-mail]>
+
+properties:
+  compatible:
+    oneOf:
+      - 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
+
+  power-supply: true
+
+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 b344e1318ac3..a41517843a10 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10084,6 +10084,13 @@  IMGTEC IR DECODER DRIVER
 S:	Orphan
 F:	drivers/media/rc/img-ir/
 
+IMGTEC POWERVR DRM DRIVER
+M:	Frank Binns <[e-mail]>
+M:	Sarah Walker <[e-mail]>
+M:	Donald Robson <[e-mail]>
+S:	Supported
+F:	Documentation/devicetree/bindings/gpu/img,powervr.yaml
+
 IMON SOUNDGRAPH USB IR RECEIVER
 M:	Sean Young <[e-mail]>
 L:	[e-mail]