[PATCH v4 0/8] ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more)

[PATCH v4 0/8] ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more)

[H. Nikolaus Schaller]

* MIPS: DTS: jz4780: removed "img,sgx5" from bindings
* YAML bindings: updated according to suggestions by Rob Herring
* MIPS: DTS: jz4780: insert-sorted gpu node by register address - suggested by Paul Cercueil

PATCH V3 2019-11-24 12:40:33:
* reworked YAML format with help by Rob Herring
* removed .txt binding document
* change compatible "ti,am335x-sgx" to "ti,am3352-sgx" - suggested by Tony Lindgren

PATCH V2 2019-11-07 12:06:17:
* tried to convert bindings to YAML format - suggested by Rob Herring
* added JZ4780 DTS node (proven to load the driver)
* removed timer and img,cores properties until we know we really need them - suggested by Rob Herring

PATCH V1 2019-10-18 20:46:35:

This patch series defines child nodes for the SGX5xx interface inside
different SoC so that a driver can be found and probed by the
compatible strings and can retrieve information about the SGX revision
that is included in a specific SoC. It also defines the interrupt number
to be used by the SGX driver.

There is currently no mainline driver for these GPUs, but a project [1]
is ongoing with the goal to get the open-source part as provided by TI/IMG
and others into drivers/gpu/drm/pvrsgx.

The kernel modules built from this project have successfully demonstrated
to work with the DTS definitions from this patch set on AM335x BeagleBone
Black, DM3730 and OMAP5 Pyra and Droid 4. They partially work on OMAP3530 and
PandaBoard ES but that is likely a problem in the kernel driver or the
(non-free) user-space libraries and binaries.

Wotk for JZ4780 (CI20 board) is in progress and there is potential to extend
this work to e.g. BananaPi-M3 (A83) and  some Intel Poulsbo and CedarView
devices.

[1]: https://github.com/openpvrsgx-devgroup


H. Nikolaus Schaller (8):
  dt-bindings: add img,pvrsgx.yaml for Imagination GPUs
  ARM: DTS: am33xx: add sgx gpu child node
  ARM: DTS: am3517: add sgx gpu child node
  ARM: DTS: omap3: add sgx gpu child node
  ARM: DTS: omap36xx: add sgx gpu child node
  ARM: DTS: omap4: add sgx gpu child node
  ARM: DTS: omap5: add sgx gpu child node
  MIPS: DTS: jz4780: add sgx gpu node

 .../devicetree/bindings/gpu/img,pvrsgx.yaml   | 80 +++++++++++++++++++
 arch/arm/boot/dts/am33xx.dtsi                 |  9 ++-
 arch/arm/boot/dts/am3517.dtsi                 | 11 +--
 arch/arm/boot/dts/omap34xx.dtsi               | 11 +--
 arch/arm/boot/dts/omap36xx.dtsi               | 11 +--
 arch/arm/boot/dts/omap4.dtsi                  |  9 ++-
 arch/arm/boot/dts/omap4470.dts                | 15 ++++
 arch/arm/boot/dts/omap5.dtsi                  |  9 ++-
 arch/mips/boot/dts/ingenic/jz4780.dtsi        | 11 +++
 9 files changed, 139 insertions(+), 27 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml
 create mode 100644 arch/arm/boot/dts/omap4470.dts

-- 
2.23.0

[PATCH v4 1/8] dt-bindings: add img,pvrsgx.yaml for Imagination GPUs

[H. Nikolaus Schaller]

The Imagination PVR/SGX GPU is part of several SoC from
multiple vendors, e.g. TI OMAP, Ingenic JZ4780, Intel Poulsbo
and others.

With this binding, we describe how the SGX processor is
interfaced to the SoC (registers, interrupt etc.).

In most cases, Clock, Reset and power management is handled
by a parent node or elsewhere.

Tested by make dt_binding_check dtbs_check

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 .../devicetree/bindings/gpu/img,pvrsgx.yaml   | 80 +++++++++++++++++++
 1 file changed, 80 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml

diff --git a/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml b/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml
new file mode 100644
index 000000000000..44799774e34d
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml
@@ -0,0 +1,80 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpu/img,pvrsgx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Imagination PVR/SGX GPU
+
+maintainers:
+  - H. Nikolaus Schaller <hns@goldelico.com>
+
+description: |+
+  This binding describes the Imagination SGX5 series of 3D accelerators which
+  are found in several different SoC like TI OMAP, Sitara, Ingenic JZ4780,
+  Allwinner A83, and Intel Poulsbo and CedarView and more.
+
+  For an almost complete list see: https://en.wikipedia.org/wiki/PowerVR#Implementations
+  
+  Only the Imagination SGX530, SGX540 and SGX544 GPUs are currently covered by
+  this binding but the extension of the pattern is straightforward.
+  
+  The SGX node is usually a child node of some DT node belonging to the SoC
+  which handles clocks, reset and general address space mapping of the SGX
+  register area.
+
+properties:
+  compatible:
+    enum:
+    # Example: BeagleBoard A/B/C, OpenPandora 600MHz
+      - ti,omap3-sgx530-121, img,sgx530-121, img,sgx530
+    # Example: BeagleBoard XM, GTA04, OpenPandora 1GHz
+      - ti,omap3-sgx530-125, img,sgx530-125, img,sgx530
+    # Example: BeagleBone Black
+      - ti,am3352-sgx530-125, img,sgx530-125, img,sgx530
+    # Example: Pandaboard, Pandaboard ES
+      - ti,omap4-sgx540-120, img,sgx540-120, img,sgx540
+      - ti,omap4-sgx544-112, img,sgx544-112, img,sgx544
+    # Example: OMAP5 UEVM, Pyra Handheld
+      - ti,omap5-sgx544-116, img,sgx544-116, img,sgx544
+      - ti,dra7-sgx544-116, img,sgx544-116, img,sgx544
+    # Example: CI20
+      - ingenic,jz4780-sgx540-120, img,sgx540-120, img,sgx540
+    # the following entries are not validated with real hardware
+    # more TI SoC
+      - ti,am3517-sgx530-125, img,sgx530-125, img,sgx530
+      - ti,am4-sgx530-125, img,sgx530-125, img,sgx530
+      - ti,ti81xx-sgx530-125, img,sgx530-125, img,sgx530
+    # Example: Banana-Pi-M3 (Allwinner A83T)
+      - allwinner,sun8i-a83t-sgx544-116, img,sgx544-116, img,sgx544
+    # Example: Atom Z5xx
+      - intel,poulsbo-gma500-sgx535, img,sgx535-116, img,sgx535
+    # Example: Atom Z24xx
+      - intel,medfield-gma-sgx540, img,sgx540-116, img,sgx540
+    # Example: Atom N2600, D2500
+      - intel,cedarview-gma3600-sgx545, img,sgx545-116, img,sgx545
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |+
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    gpu@fe00 {
+      compatible = "ti,omap-omap5-sgx544-116", "img,sgx544-116", "img,sgx544", "img,sgx5";
+      reg = <0xfe00 0x200>;
+      interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
+    };
+
+...
-- 
2.23.0

[PATCH v4 2/8] ARM: DTS: am33xx: add sgx gpu child node

[H. Nikolaus Schaller]

and add timer and interrupt

Tested-by: H. Nikolaus Schaller <hns@goldelico.com> # BeagleBone Black
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 arch/arm/boot/dts/am33xx.dtsi | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index c427ce8c2539..11c7b0dbb5dc 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -477,10 +477,11 @@
 			#size-cells = <1>;
 			ranges = <0 0x56000000 0x1000000>;
 
-			/*
-			 * Closed source PowerVR driver, no child device
-			 * binding or driver in mainline
-			 */
+			sgx: gpu@0 {
+				compatible = "ti,am3352-sgx530-125", "img,sgx530-125", "img,sgx530";
+				reg = <0x00 0x1000000>;	/* 16 MB */
+				interrupts = <37>;
+			};
 		};
 	};
 };
-- 
2.23.0

[PATCH v4 3/8] ARM: DTS: am3517: add sgx gpu child node

[H. Nikolaus Schaller]

and add interrupt.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 arch/arm/boot/dts/am3517.dtsi | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/arm/boot/dts/am3517.dtsi b/arch/arm/boot/dts/am3517.dtsi
index 125379ecab2f..6d2ad5e5f1a6 100644
--- a/arch/arm/boot/dts/am3517.dtsi
+++ b/arch/arm/boot/dts/am3517.dtsi
@@ -128,7 +128,7 @@
 		 * revision register instead of the unreadable OCP revision
 		 * register.
 		 */
-		sgx_module: target-module@50000000 {
+		target-module@50000000 {
 			compatible = "ti,sysc-omap2", "ti,sysc";
 			reg = <0x50000014 0x4>;
 			reg-names = "rev";
@@ -138,10 +138,11 @@
 			#size-cells = <1>;
 			ranges = <0 0x50000000 0x4000>;
 
-			/*
-			 * Closed source PowerVR driver, no child device
-			 * binding or driver in mainline
-			 */
+			sgx: gpu@0 {
+				compatible = "ti,am3517-sgx530-125", "img,sgx530-125", "img,sgx530";
+				reg = <0x0 0x4000>;
+				interrupts = <21>;
+			};
 		};
 	};
 };
-- 
2.23.0

[PATCH v4 4/8] ARM: DTS: omap3: add sgx gpu child node

[H. Nikolaus Schaller]

and add interrupt

Tested-by: H. Nikolaus Schaller <hns@goldelico.com> # OpenPandora 600 MHz.
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 arch/arm/boot/dts/omap34xx.dtsi | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/arm/boot/dts/omap34xx.dtsi b/arch/arm/boot/dts/omap34xx.dtsi
index c4dd9801840d..a858a2e8002d 100644
--- a/arch/arm/boot/dts/omap34xx.dtsi
+++ b/arch/arm/boot/dts/omap34xx.dtsi
@@ -159,7 +159,7 @@
 		 * are also different clocks, but we do not have any dts users
 		 * for it.
 		 */
-		sgx_module: target-module@50000000 {
+		target-module@50000000 {
 			compatible = "ti,sysc-omap2", "ti,sysc";
 			reg = <0x50000014 0x4>;
 			reg-names = "rev";
@@ -169,10 +169,11 @@
 			#size-cells = <1>;
 			ranges = <0 0x50000000 0x4000>;
 
-			/*
-			 * Closed source PowerVR driver, no child device
-			 * binding or driver in mainline
-			 */
+			sgx: gpu@0 {
+				compatible = "ti,omap3-sgx530-121", "img,sgx530-121", "img,sgx530";
+				reg = <0x0 0x4000>;	/* 64kB */
+				interrupts = <21>;
+			};
 		};
 	};
 
-- 
2.23.0

[PATCH v4 5/8] ARM: DTS: omap36xx: add sgx gpu child node

[H. Nikolaus Schaller]

and add interrupt.

Tested-by: H. Nikolaus Schaller <hns@goldelico.com> # GTA04, BeagleBoard XM
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 arch/arm/boot/dts/omap36xx.dtsi | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/arm/boot/dts/omap36xx.dtsi b/arch/arm/boot/dts/omap36xx.dtsi
index c618cb257d00..b9d488a193e8 100644
--- a/arch/arm/boot/dts/omap36xx.dtsi
+++ b/arch/arm/boot/dts/omap36xx.dtsi
@@ -194,7 +194,7 @@
 		 * "ti,sysc-omap4" type register with just sidle and midle bits
 		 * available while omap34xx has "ti,sysc-omap2" type sysconfig.
 		 */
-		sgx_module: target-module@50000000 {
+		target-module@50000000 {
 			compatible = "ti,sysc-omap4", "ti,sysc";
 			reg = <0x5000fe00 0x4>,
 			      <0x5000fe10 0x4>;
@@ -211,10 +211,11 @@
 			#size-cells = <1>;
 			ranges = <0 0x50000000 0x2000000>;
 
-			/*
-			 * Closed source PowerVR driver, no child device
-			 * binding or driver in mainline
-			 */
+			sgx: gpu@0 {
+				compatible = "ti,omap3-sgx530-125", "img,sgx530-125", "img,sgx530";
+				reg = <0x0 0x10000>;	/* 64kB */
+				interrupts = <21>;
+			};
 		};
 	};
 
-- 
2.23.0

[PATCH v4 6/8] ARM: DTS: omap4: add sgx gpu child node

[H. Nikolaus Schaller]

and add interrupt.

Since omap4420/30/60 and omap4470 come with different SGX variants
we need to introduce a new omap4470.dtsi. If an omap4470 board
does not want to use SGX it is no problem to still include
omap4460.dtsi.

Tested-by: H. Nikolaus Schaller <hns@goldelico.com> # PandaBoard ES
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 arch/arm/boot/dts/omap4.dtsi   |  9 +++++----
 arch/arm/boot/dts/omap4470.dts | 15 +++++++++++++++
 2 files changed, 20 insertions(+), 4 deletions(-)
 create mode 100644 arch/arm/boot/dts/omap4470.dts

diff --git a/arch/arm/boot/dts/omap4.dtsi b/arch/arm/boot/dts/omap4.dtsi
index 2de8a6b53de9..bfca05309a99 100644
--- a/arch/arm/boot/dts/omap4.dtsi
+++ b/arch/arm/boot/dts/omap4.dtsi
@@ -347,10 +347,11 @@
 			#size-cells = <1>;
 			ranges = <0 0x56000000 0x2000000>;
 
-			/*
-			 * Closed source PowerVR driver, no child device
-			 * binding or driver in mainline
-			 */
+			sgx: img@0 {
+				compatible = "ti,omap4-sgx540-120", "img,sgx540-120", "img,sgx540";
+				reg = <0x0 0x2000000>;	/* 32MB */
+				interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
+			};
 		};
 
 		dss: dss@58000000 {
diff --git a/arch/arm/boot/dts/omap4470.dts b/arch/arm/boot/dts/omap4470.dts
new file mode 100644
index 000000000000..19b554612401
--- /dev/null
+++ b/arch/arm/boot/dts/omap4470.dts
@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Device Tree Source for OMAP4470 SoC
+ *
+ * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * This file is licensed under the terms of the GNU General Public License
+ * version 2.  This program is licensed "as is" without any warranty of any
+ * kind, whether express or implied.
+ */
+#include "omap4460.dtsi"
+
+&sgx {
+	compatible = "img,sgx544-112", "img,sgx544", "ti,omap-omap4-sgx544-112";
+};
-- 
2.23.0

[PATCH v4 7/8] ARM: DTS: omap5: add sgx gpu child node

[H. Nikolaus Schaller]

and add interrupt.

Tested-by: H. Nikolaus Schaller <hns@goldelico.com> # Pyra-Handheld.
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 arch/arm/boot/dts/omap5.dtsi | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/omap5.dtsi b/arch/arm/boot/dts/omap5.dtsi
index 1f6ad1debc90..3bb7a4949c12 100644
--- a/arch/arm/boot/dts/omap5.dtsi
+++ b/arch/arm/boot/dts/omap5.dtsi
@@ -274,10 +274,11 @@
 			#size-cells = <1>;
 			ranges = <0 0x56000000 0x2000000>;
 
-			/*
-			 * Closed source PowerVR driver, no child device
-			 * binding or driver in mainline
-			 */
+			sgx: gpu@0 {
+				compatible = "ti,omap5-sgx544-116", "img,sgx544-116", "img,sgx544";
+				reg = <0x0 0x10000>;
+				interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
+			};
 		};
 
 		dss: dss@58000000 {
-- 
2.23.0

[PATCH v4 8/8] MIPS: DTS: jz4780: add sgx gpu node

[H. Nikolaus Schaller]

and add interrupt and clocks.

Tested to build for CI20 board and load a (non-working - there is no video driver for jz4780) driver.

Suggested-by: Paul Boddie <paul@boddie.org.uk>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 arch/mips/boot/dts/ingenic/jz4780.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index f928329b034b..c111f955dece 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -356,6 +356,17 @@
 		clock-names = "rtc";
 	};
 
+	gpu: gpu@13040000 {
+		compatible = "ingenic,jz4780-sgx540-120", "img,sgx540-120", "img,sgx540";
+		reg = <0x13040000 0x4000>;
+
+		clocks = <&cgu JZ4780_CLK_GPU>;
+		clock-names = "gpu";
+
+		interrupt-parent = <&intc>;
+		interrupts = <63>;
+	};
+
 	nemc: nemc@13410000 {
 		compatible = "ingenic,jz4780-nemc";
 		reg = <0x13410000 0x10000>;
-- 
2.23.0

Re: [PATCH v4 1/8] dt-bindings: add img,pvrsgx.yaml for Imagination GPUs

[Rob Herring]

On Tue, Dec 17, 2019 at 07:01:59PM +0100, H. Nikolaus Schaller wrote:
> The Imagination PVR/SGX GPU is part of several SoC from
> multiple vendors, e.g. TI OMAP, Ingenic JZ4780, Intel Poulsbo
> and others.
> 
> With this binding, we describe how the SGX processor is
> interfaced to the SoC (registers, interrupt etc.).
> 
> In most cases, Clock, Reset and power management is handled
> by a parent node or elsewhere.
> 
> Tested by make dt_binding_check dtbs_check

I'm surprised that worked... (Not for long if it did).

> 
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  .../devicetree/bindings/gpu/img,pvrsgx.yaml   | 80 +++++++++++++++++++
>  1 file changed, 80 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml
> 
> diff --git a/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml b/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml
> new file mode 100644
> index 000000000000..44799774e34d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml
> @@ -0,0 +1,80 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpu/img,pvrsgx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Imagination PVR/SGX GPU
> +
> +maintainers:
> +  - H. Nikolaus Schaller <hns@goldelico.com>
> +
> +description: |+
> +  This binding describes the Imagination SGX5 series of 3D accelerators which
> +  are found in several different SoC like TI OMAP, Sitara, Ingenic JZ4780,
> +  Allwinner A83, and Intel Poulsbo and CedarView and more.
> +
> +  For an almost complete list see: https://en.wikipedia.org/wiki/PowerVR#Implementations
> +  
> +  Only the Imagination SGX530, SGX540 and SGX544 GPUs are currently covered by
> +  this binding but the extension of the pattern is straightforward.
> +  
> +  The SGX node is usually a child node of some DT node belonging to the SoC
> +  which handles clocks, reset and general address space mapping of the SGX
> +  register area.
> +
> +properties:
> +  compatible:
> +    enum:
> +    # Example: BeagleBoard A/B/C, OpenPandora 600MHz
> +      - ti,omap3-sgx530-121, img,sgx530-121, img,sgx530

Didn't I comment before this is not valid. You are defining the 
compatible string is: 'ti,omap3-sgx530-121, img,sgx530-121, img,sgx530'

You need:

compatible:
  oneOf:
    - description: BeagleBoard A/B/C, OpenPandora 600MHz
      items:
        - const: ti,omap3-sgx530-121
        - const: img,sgx530-121
        - const: img,sgx530

And so on for each of the rest.

> +    # Example: BeagleBoard XM, GTA04, OpenPandora 1GHz
> +      - ti,omap3-sgx530-125, img,sgx530-125, img,sgx530
> +    # Example: BeagleBone Black
> +      - ti,am3352-sgx530-125, img,sgx530-125, img,sgx530

These 2 could be combined using 'enum' for the first item. Basically, 
you can group ones where the last 2 strings are the same.

> +    # Example: Pandaboard, Pandaboard ES
> +      - ti,omap4-sgx540-120, img,sgx540-120, img,sgx540
> +      - ti,omap4-sgx544-112, img,sgx544-112, img,sgx544
> +    # Example: OMAP5 UEVM, Pyra Handheld
> +      - ti,omap5-sgx544-116, img,sgx544-116, img,sgx544
> +      - ti,dra7-sgx544-116, img,sgx544-116, img,sgx544
> +    # Example: CI20
> +      - ingenic,jz4780-sgx540-120, img,sgx540-120, img,sgx540
> +    # the following entries are not validated with real hardware

What am I supposed to do with that? You're just defining some strings. 
If you're not sure they are okay, then don't define them.

> +    # more TI SoC
> +      - ti,am3517-sgx530-125, img,sgx530-125, img,sgx530
> +      - ti,am4-sgx530-125, img,sgx530-125, img,sgx530
> +      - ti,ti81xx-sgx530-125, img,sgx530-125, img,sgx530
> +    # Example: Banana-Pi-M3 (Allwinner A83T)
> +      - allwinner,sun8i-a83t-sgx544-116, img,sgx544-116, img,sgx544
> +    # Example: Atom Z5xx
> +      - intel,poulsbo-gma500-sgx535, img,sgx535-116, img,sgx535
> +    # Example: Atom Z24xx
> +      - intel,medfield-gma-sgx540, img,sgx540-116, img,sgx540
> +    # Example: Atom N2600, D2500
> +      - intel,cedarview-gma3600-sgx545, img,sgx545-116, img,sgx545
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |+
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    gpu@fe00 {
> +      compatible = "ti,omap-omap5-sgx544-116", "img,sgx544-116", "img,sgx544", "img,sgx5";

Doesn't match the schema.

> +      reg = <0xfe00 0x200>;
> +      interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
> +    };
> +
> +...
> -- 
> 2.23.0
> 

Re: [PATCH v4 1/8] dt-bindings: add img,pvrsgx.yaml for Imagination GPUs

[H. Nikolaus Schaller]

Hi Rob,

> Am 18.12.2019 um 22:16 schrieb Rob Herring <robh@kernel.org>:
> 
> On Tue, Dec 17, 2019 at 07:01:59PM +0100, H. Nikolaus Schaller wrote:
>> The Imagination PVR/SGX GPU is part of several SoC from
>> multiple vendors, e.g. TI OMAP, Ingenic JZ4780, Intel Poulsbo
>> and others.
>> 
>> With this binding, we describe how the SGX processor is
>> interfaced to the SoC (registers, interrupt etc.).
>> 
>> In most cases, Clock, Reset and power management is handled
>> by a parent node or elsewhere.
>> 
>> Tested by make dt_binding_check dtbs_check
> 
> I'm surprised that worked... (Not for long if it did).

AFAIR, it did not fail but emit thousands of warnings for other areas
so I may have missed some warning but there was no error that did stop
compilation.

> 
>> 
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> .../devicetree/bindings/gpu/img,pvrsgx.yaml   | 80 +++++++++++++++++++
>> 1 file changed, 80 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml
>> 
>> diff --git a/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml b/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml
>> new file mode 100644
>> index 000000000000..44799774e34d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml
>> @@ -0,0 +1,80 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/gpu/img,pvrsgx.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Imagination PVR/SGX GPU
>> +
>> +maintainers:
>> +  - H. Nikolaus Schaller <hns@goldelico.com>
>> +
>> +description: |+
>> +  This binding describes the Imagination SGX5 series of 3D accelerators which
>> +  are found in several different SoC like TI OMAP, Sitara, Ingenic JZ4780,
>> +  Allwinner A83, and Intel Poulsbo and CedarView and more.
>> +
>> +  For an almost complete list see: https://en.wikipedia.org/wiki/PowerVR#Implementations
>> +  
>> +  Only the Imagination SGX530, SGX540 and SGX544 GPUs are currently covered by
>> +  this binding but the extension of the pattern is straightforward.
>> +  
>> +  The SGX node is usually a child node of some DT node belonging to the SoC
>> +  which handles clocks, reset and general address space mapping of the SGX
>> +  register area.
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +    # Example: BeagleBoard A/B/C, OpenPandora 600MHz
>> +      - ti,omap3-sgx530-121, img,sgx530-121, img,sgx530
> 
> Didn't I comment before this is not valid.

Not that I am aware of. Your comment was:

<<<
> +properties:
> +  compatible:
> +    enum:
> +    # BeagleBoard ABC, OpenPandora 600MHz

I'd expect compatibles to be per SoC, not per board.

> +      - ti,omap3-sgx530-121, img,sgx530-121, img,sgx530, img,sgx5

4 compatibles is probably a bit much. Are there not any version or 
feature registers that some of this could be detected? If there are, I'd 
assume the middle 2 strings could be dropped. If not, drop the last one 
and just match on the 3rd string. It's not a long list.
>>>

So I interpreted your comment is about the # comment focussing too much
on boards instead of SoC and about dropping img,sgx5

> You are defining the 
> compatible string is: 'ti,omap3-sgx530-121, img,sgx530-121, img,sgx530'

Yes.

> 
> You need:
> 
> compatible:
>  oneOf:
>    - description: BeagleBoard A/B/C, OpenPandora 600MHz
>      items:
>        - const: ti,omap3-sgx530-121
>        - const: img,sgx530-121
>        - const: img,sgx530
> 
> And so on for each of the rest.

Hm. As far as I understand YAML, it has multiple ways to define the same
structure but this manifold nature has its drawbacks.

If it is wrong I have to admit that I still do not understand how to write
correct schemes... Why has it been made so difficult for us developers? 

> 
>> +    # Example: BeagleBoard XM, GTA04, OpenPandora 1GHz
>> +      - ti,omap3-sgx530-125, img,sgx530-125, img,sgx530
>> +    # Example: BeagleBone Black
>> +      - ti,am3352-sgx530-125, img,sgx530-125, img,sgx530
> 
> These 2 could be combined using 'enum' for the first item. Basically, 
> you can group ones where the last 2 strings are the same.

Is this better readable?

IMHO, this would be a nice coding trick that would make this
even more unreadable and does not add any new information.

Generally, in my opinion, a bindings document should not only
satisfy the dtbs_check but should be human readable (and writeable!)
so that a DTS developer can still understand and ideally copy&paste
fragments. With such enum tricks and -items const: constructions
this IMHO becomes more difficult.

Therefore I think the linear list is better readable and
can be directly copied. If the test rig has problems with
that statement, I would suggest that the parser should be
modified to understand what we can easily write and read.

The same is for # comment vs. description: . The # comment
is for a human reader of this document to get a hint what
board the next line is intended for. It is not necessary
(and IMHO impossible) to automatically test it. So I do not
see any need to add a formal -description entry here.

> 
>> +    # Example: Pandaboard, Pandaboard ES
>> +      - ti,omap4-sgx540-120, img,sgx540-120, img,sgx540
>> +      - ti,omap4-sgx544-112, img,sgx544-112, img,sgx544
>> +    # Example: OMAP5 UEVM, Pyra Handheld
>> +      - ti,omap5-sgx544-116, img,sgx544-116, img,sgx544
>> +      - ti,dra7-sgx544-116, img,sgx544-116, img,sgx544
>> +    # Example: CI20
>> +      - ingenic,jz4780-sgx540-120, img,sgx540-120, img,sgx540
>> +    # the following entries are not validated with real hardware
> 
> What am I supposed to do with that? You're just defining some strings. 
> If you're not sure they are okay, then don't define them.

I have a broader audience in mind than 'make dtbs_check'. IMHO, this
document should help future developers to get an idea what to write for
those SoC. Yes, it might not be 100% correct, but it is easier to correct
a 99% correct definition than inventing it from scratch after dropping
partially correct information I already have.

For example, we know from data sheets and public information that the
allwinner a83 contains a sgx544 but it is not sure if it is the -116
variant. Or, the poulsbo has a GPU called gma500 which is an sgx535. It
is only that nobody has verified this so far by real hardware.

If we leave it out and someone starts to adapt this to the poulsbo
in let's say 2 years from now, that someone has to be lucky to find
our discussion here and take that as a hint.

I am not sure how you are developing contributions, but my first
place to start with is the existing code and not the archives of
LKML.

That is my motivation adding these records right here even if they
are not verified.

> 
>> +    # more TI SoC
>> +      - ti,am3517-sgx530-125, img,sgx530-125, img,sgx530
>> +      - ti,am4-sgx530-125, img,sgx530-125, img,sgx530
>> +      - ti,ti81xx-sgx530-125, img,sgx530-125, img,sgx530
>> +    # Example: Banana-Pi-M3 (Allwinner A83T)
>> +      - allwinner,sun8i-a83t-sgx544-116, img,sgx544-116, img,sgx544
>> +    # Example: Atom Z5xx
>> +      - intel,poulsbo-gma500-sgx535, img,sgx535-116, img,sgx535
>> +    # Example: Atom Z24xx
>> +      - intel,medfield-gma-sgx540, img,sgx540-116, img,sgx540
>> +    # Example: Atom N2600, D2500
>> +      - intel,cedarview-gma3600-sgx545, img,sgx545-116, img,sgx545
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |+
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +
>> +    gpu@fe00 {
>> +      compatible = "ti,omap-omap5-sgx544-116", "img,sgx544-116", "img,sgx544", "img,sgx5";
> 
> Doesn't match the schema.

Ah, thanks. I missed that when reviewing the DTS patch set.
Queued for a v5.

> 
>> +      reg = <0xfe00 0x200>;
>> +      interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
>> +    };
>> +
>> +...
>> -- 
>> 2.23.0
>> 

BR and thanks,
Nikolaus

Re: [PATCH v4 4/8] ARM: DTS: omap3: add sgx gpu child node

[Andrew F. Davis]

On 12/17/19 1:02 PM, H. Nikolaus Schaller wrote:
> and add interrupt
> 
> Tested-by: H. Nikolaus Schaller <hns@goldelico.com> # OpenPandora 600 MHz.
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  arch/arm/boot/dts/omap34xx.dtsi | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/omap34xx.dtsi b/arch/arm/boot/dts/omap34xx.dtsi
> index c4dd9801840d..a858a2e8002d 100644
> --- a/arch/arm/boot/dts/omap34xx.dtsi
> +++ b/arch/arm/boot/dts/omap34xx.dtsi
> @@ -159,7 +159,7 @@
>  		 * are also different clocks, but we do not have any dts users
>  		 * for it.
>  		 */
> -		sgx_module: target-module@50000000 {
> +		target-module@50000000 {
>  			compatible = "ti,sysc-omap2", "ti,sysc";
>  			reg = <0x50000014 0x4>;
>  			reg-names = "rev";
> @@ -169,10 +169,11 @@
>  			#size-cells = <1>;
>  			ranges = <0 0x50000000 0x4000>;
>  
> -			/*
> -			 * Closed source PowerVR driver, no child device
> -			 * binding or driver in mainline
> -			 */
> +			sgx: gpu@0 {
> +				compatible = "ti,omap3-sgx530-121", "img,sgx530-121", "img,sgx530";
> +				reg = <0x0 0x4000>;	/* 64kB */


Just for future reference, this range should be 0x10000.

Andrew


> +				interrupts = <21>;
> +			};
>  		};
>  	};
>  
>