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

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

[H. Nikolaus Schaller]

* changed commit message for the dt-bindings to better describe latest situation
* added properties for up to 4 clocks, reset, power-domains, sgx-supply - proposed by Maxime Ripard <maxime@cerno.tech>
* fixed jz4780 and s5pv210 to use "core" clock name
* simplified example
* update for arm: dts: s5pv210 - by Jonathan Bakker <xc-racer2@live.ca>
* removed a stray " from binding which had creeped in through copy&paste 
* fixed commit / tested-by messages and some not well formed commit messages - suggested by Krzysztof Kozlowski <krzk@kernel.org>
* added a $nodename: pattern: to enforce "gpu" nodenames - inspired by Neil Armstrong <narmstrong@baylibre.com>
* fixed node name for s5pv210 - suggested by Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

PATCH V6 2020-04-15 10:35:31:
* rebased to v5.7-rc1
* added DTS for for a31, a31s, a83t - by Philipp Rossak <embed3d@gmail.com>
* added DTS for "samsung,s5pv210-sgx540-120" - by Jonathan Bakker <xc-racer2@live.ca>
* bindings.yaml fixes:
  - added a31, a31
  - fixes for omap4470
  - jz4780 contains an sgx540-130 and not -120
  - a83t contains an sgx544-115 and not -116
  - removed "additionalProperties: false" because some SoC may need additional properties

PATCH V5 2020-03-29 19:38:32:
* reworked YAML bindings to pass dt_binding_check and be better grouped
* rename all nodes to "gpu: gpu@<address>"
* removed "img,sgx5" from example - suggested by Rob Herring <robh+dt@kernel.org>

PATCH V4 2019-12-17 19:02:11:
* 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, and optionally clocks, power, resets
depending on how the SoC integration is done.

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 in the future. So this patch series
is the basis.

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. The driver works on jz4780
but user-space could not yet be tested.

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

Jonathan Bakker (1):
  arm: dts: s5pv210: Add node for SGX 540

Philipp Rossak (3):
  ARM: dts: sun6i: a31: add sgx gpu child node
  ARM: dts: sun6i: a31s: add sgx gpu child node
  ARM: dts: sun8i: a83t: add sgx gpu child node

 .../devicetree/bindings/gpu/img,pvrsgx.yaml   | 150 ++++++++++++++++++
 arch/arm/boot/dts/am33xx.dtsi                 |  11 +-
 arch/arm/boot/dts/am3517.dtsi                 |   9 +-
 arch/arm/boot/dts/omap34xx.dtsi               |  11 +-
 arch/arm/boot/dts/omap36xx.dtsi               |   9 +-
 arch/arm/boot/dts/omap4.dtsi                  |  11 +-
 arch/arm/boot/dts/omap4470.dts                |  15 ++
 arch/arm/boot/dts/omap5.dtsi                  |  11 +-
 arch/arm/boot/dts/s5pv210.dtsi                |  13 ++
 arch/arm/boot/dts/sun6i-a31.dtsi              |  11 ++
 arch/arm/boot/dts/sun6i-a31s.dtsi             |  10 ++
 arch/arm/boot/dts/sun8i-a83t.dtsi             |  11 ++
 arch/mips/boot/dts/ingenic/jz4780.dtsi        |  11 ++
 13 files changed, 255 insertions(+), 28 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml
 create mode 100644 arch/arm/boot/dts/omap4470.dts

-- 
2.25.1

[PATCH v7 01/12] 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,
Allwinner A83 and others.

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

The interface also consists of clocks, reset, power but
information from data sheets is vague and some SoC integrators
(TI) deciced to use a PRCM wrapper (ti,sysc) which does
all clock, reset and power-management through registers
outside of the sgx register block.

Therefore all these properties are optional.

Tested by make dt_binding_check

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 .../devicetree/bindings/gpu/img,pvrsgx.yaml   | 150 ++++++++++++++++++
 1 file changed, 150 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..33a9c4c6e784
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml
@@ -0,0 +1,150 @@
+# 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 extensive list see: https://en.wikipedia.org/wiki/PowerVR#Implementations
+
+  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. If not, an optional clock can be specified here.
+
+properties:
+  $nodename:
+    pattern: '^gpu@[a-f0-9]+$'
+  compatible:
+    oneOf:
+      - description: SGX530-121 based SoC
+        items:
+          - enum:
+            - ti,omap3-sgx530-121 # BeagleBoard A/B/C, OpenPandora 600MHz and similar
+          - const: img,sgx530-121
+          - const: img,sgx530
+
+      - description: SGX530-125 based SoC
+        items:
+          - enum:
+            - ti,am3352-sgx530-125 # BeagleBone Black
+            - ti,am3517-sgx530-125
+            - ti,am4-sgx530-125
+            - ti,omap3-sgx530-125 # BeagleBoard XM, GTA04, OpenPandora 1GHz and similar
+            - ti,ti81xx-sgx530-125
+          - const: ti,omap3-sgx530-125
+          - const: img,sgx530-125
+          - const: img,sgx530
+
+      - description: SGX535-116 based SoC
+        items:
+          - const: intel,poulsbo-gma500-sgx535 # Atom Z5xx
+          - const: img,sgx535-116
+          - const: img,sgx535
+
+      - description: SGX540-116 based SoC
+        items:
+          - const: intel,medfield-gma-sgx540 # Atom Z24xx
+          - const: img,sgx540-116
+          - const: img,sgx540
+
+      - description: SGX540-120 based SoC
+        items:
+          - enum:
+            - samsung,s5pv210-sgx540-120
+            - ti,omap4-sgx540-120 # Pandaboard, Pandaboard ES and similar
+          - const: img,sgx540-120
+          - const: img,sgx540
+
+      - description: SGX540-130 based SoC
+        items:
+          - enum:
+            - ingenic,jz4780-sgx540-130 # CI20
+          - const: img,sgx540-130
+          - const: img,sgx540
+
+      - description: SGX544-112 based SoC
+        items:
+          - const: ti,omap4470-sgx544-112
+          - const: img,sgx544-112
+          - const: img,sgx544
+
+      - description: SGX544-115 based SoC
+        items:
+          - enum:
+            - allwinner,sun8i-a31-sgx544-115
+            - allwinner,sun8i-a31s-sgx544-115
+            - allwinner,sun8i-a83t-sgx544-115 # Banana-Pi-M3 (Allwinner A83T) and similar
+          - const: img,sgx544-115
+          - const: img,sgx544
+
+      - description: SGX544-116 based SoC
+        items:
+          - enum:
+            - ti,dra7-sgx544-116 # DRA7
+            - ti,omap5-sgx544-116 # OMAP5 UEVM, Pyra Handheld and similar
+          - const: img,sgx544-116
+          - const: img,sgx544
+
+      - description: SGX545 based SoC
+        items:
+          - const: intel,cedarview-gma3600-sgx545 # Atom N2600, D2500
+          - const: img,sgx545-116
+          - const: img,sgx545
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  interrupt-names:
+    maxItems: 1
+    items:
+      - const: sgx
+
+  clocks:
+    maxItems: 4
+
+  clock-names:
+    maxItems: 4
+    items:
+      - const: core
+      - const: sys
+      - const: mem
+      - const: hyd
+
+  sgx-supply: true
+
+  power-domains:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |+
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    gpu: gpu@fe00 {
+      compatible = "ti,omap5-sgx544-116", "img,sgx544-116", "img,sgx544";
+      reg = <0xfe00 0x200>;
+      interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
+    };
+
+...
-- 
2.25.1

[PATCH v7 02/12] ARM: DTS: am33xx: add sgx gpu child node

[H. Nikolaus Schaller]

Add SGX GPU node with interrupt. Tested on BeagleBone Black.

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

diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index a35f5052d76f..155424d87156 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -564,7 +564,7 @@ aes: aes@0 {
 			};
 		};
 
-		target-module@56000000 {
+		sgx_module: target-module@56000000 {
 			compatible = "ti,sysc-omap4", "ti,sysc";
 			reg = <0x5600fe00 0x4>,
 			      <0x5600fe10 0x4>;
@@ -583,10 +583,11 @@ target-module@56000000 {
 			#size-cells = <1>;
 			ranges = <0 0x56000000 0x1000000>;
 
-			/*
-			 * Closed source PowerVR driver, no child device
-			 * binding or driver in mainline
-			 */
+			gpu: gpu@0 {
+				compatible = "ti,am3352-sgx530-125", "img,sgx530-125", "img,sgx530";
+				reg = <0x00 0x1000000>;	/* 16 MB */
+				interrupts = <37>;
+			};
 		};
 	};
 };
-- 
2.25.1

[PATCH v7 03/12] ARM: DTS: am3517: add sgx gpu child node

[H. Nikolaus Schaller]

Add SGX GPU node with interrupt.

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

diff --git a/arch/arm/boot/dts/am3517.dtsi b/arch/arm/boot/dts/am3517.dtsi
index e0b5a00e2078..3fce56a646d1 100644
--- a/arch/arm/boot/dts/am3517.dtsi
+++ b/arch/arm/boot/dts/am3517.dtsi
@@ -138,10 +138,11 @@ sgx_module: target-module@50000000 {
 			#size-cells = <1>;
 			ranges = <0 0x50000000 0x4000>;
 
-			/*
-			 * Closed source PowerVR driver, no child device
-			 * binding or driver in mainline
-			 */
+			gpu: gpu@0 {
+				compatible = "ti,am3517-sgx530-125", "img,sgx530-125", "img,sgx530";
+				reg = <0x0 0x4000>;
+				interrupts = <21>;
+			};
 		};
 	};
 };
-- 
2.25.1

[PATCH v7 04/12] ARM: DTS: omap34xx: add sgx gpu child node

[H. Nikolaus Schaller]

Add SGX GPU node with interrupt. Tested on OpenPandora 600 MHz.

According to omap3530 TRM the SGX register block is 64kB.
See: 13.4  SGX Register Mapping, Table 13-2

Reported-by: Andrew F. Davis <afd@ti.com>	# register size
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..51c60ee2b68d 100644
--- a/arch/arm/boot/dts/omap34xx.dtsi
+++ b/arch/arm/boot/dts/omap34xx.dtsi
@@ -167,12 +167,13 @@ sgx_module: target-module@50000000 {
 			clock-names = "fck", "ick";
 			#address-cells = <1>;
 			#size-cells = <1>;
-			ranges = <0 0x50000000 0x4000>;
+			ranges = <0 0x50000000 0x10000>;
 
-			/*
-			 * Closed source PowerVR driver, no child device
-			 * binding or driver in mainline
-			 */
+			gpu: gpu@0 {
+				compatible = "ti,omap3-sgx530-121", "img,sgx530-121", "img,sgx530";
+				reg = <0x0 0x10000>;	/* 64kB */
+				interrupts = <21>;
+			};
 		};
 	};
 
-- 
2.25.1

[PATCH v7 05/12] ARM: DTS: omap36xx: add sgx gpu child node

[H. Nikolaus Schaller]

Add SGX GPU node with interrupt. Tested on GTA04, BeagleBoard XM.

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

diff --git a/arch/arm/boot/dts/omap36xx.dtsi b/arch/arm/boot/dts/omap36xx.dtsi
index 71f3c8f1f924..b308dbb3b1bb 100644
--- a/arch/arm/boot/dts/omap36xx.dtsi
+++ b/arch/arm/boot/dts/omap36xx.dtsi
@@ -211,10 +211,11 @@ sgx_module: target-module@50000000 {
 			#size-cells = <1>;
 			ranges = <0 0x50000000 0x2000000>;
 
-			/*
-			 * Closed source PowerVR driver, no child device
-			 * binding or driver in mainline
-			 */
+			gpu: gpu@0 {
+				compatible = "ti,omap3-sgx530-125", "img,sgx530-125", "img,sgx530";
+				reg = <0x0 0x10000>;	/* 64kB */
+				interrupts = <21>;
+			};
 		};
 	};
 
-- 
2.25.1

[PATCH v7 06/12] ARM: DTS: omap4: add sgx gpu child node

[H. Nikolaus Schaller]

Add SGX GPU node with interrupt. Tested on PandaBoard ES.

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.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 arch/arm/boot/dts/omap4.dtsi   | 11 ++++++-----
 arch/arm/boot/dts/omap4470.dts | 15 +++++++++++++++
 2 files changed, 21 insertions(+), 5 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 763bdea8c829..15ff3d7146af 100644
--- a/arch/arm/boot/dts/omap4.dtsi
+++ b/arch/arm/boot/dts/omap4.dtsi
@@ -389,7 +389,7 @@ abb_iva: regulator-abb-iva {
 			status = "disabled";
 		};
 
-		target-module@56000000 {
+		sgx_module: target-module@56000000 {
 			compatible = "ti,sysc-omap4", "ti,sysc";
 			reg = <0x5600fe00 0x4>,
 			      <0x5600fe10 0x4>;
@@ -408,10 +408,11 @@ target-module@56000000 {
 			#size-cells = <1>;
 			ranges = <0 0x56000000 0x2000000>;
 
-			/*
-			 * Closed source PowerVR driver, no child device
-			 * binding or driver in mainline
-			 */
+			gpu: gpu@0 {
+				compatible = "ti,omap4-sgx540-120", "img,sgx540-120", "img,sgx540";
+				reg = <0x0 0x2000000>;	/* 32MB */
+				interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
+			};
 		};
 
 		/*
diff --git a/arch/arm/boot/dts/omap4470.dts b/arch/arm/boot/dts/omap4470.dts
new file mode 100644
index 000000000000..f29c581300bf
--- /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 = "ti,omap4470-sgx544-112", "img,sgx544-112", "img,sgx544";
+};
-- 
2.25.1

[PATCH v7 07/12] ARM: DTS: omap5: add sgx gpu child node

[H. Nikolaus Schaller]

Add SGX GPU node with interrupt. Tested on Pyra-Handheld.

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

diff --git a/arch/arm/boot/dts/omap5.dtsi b/arch/arm/boot/dts/omap5.dtsi
index 2ac7f021c284..1cf41664fd00 100644
--- a/arch/arm/boot/dts/omap5.dtsi
+++ b/arch/arm/boot/dts/omap5.dtsi
@@ -270,7 +270,7 @@ sata: sata@4a141100 {
 			ports-implemented = <0x1>;
 		};
 
-		target-module@56000000 {
+		sgx_module: target-module@56000000 {
 			compatible = "ti,sysc-omap4", "ti,sysc";
 			reg = <0x5600fe00 0x4>,
 			      <0x5600fe10 0x4>;
@@ -287,10 +287,11 @@ target-module@56000000 {
 			#size-cells = <1>;
 			ranges = <0 0x56000000 0x2000000>;
 
-			/*
-			 * Closed source PowerVR driver, no child device
-			 * binding or driver in mainline
-			 */
+			gpu: gpu@0 {
+				compatible = "ti,omap5-sgx544-116", "img,sgx544-116", "img,sgx544";
+				reg = <0x0 0x10000>;
+				interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
+			};
 		};
 
 		target-module@58000000 {
-- 
2.25.1

[PATCH v7 08/12] arm: dts: s5pv210: Add node for SGX 540

[H. Nikolaus Schaller]

From: Jonathan Bakker <xc-racer2@live.ca>

All s5pv210 devices have a PowerVR SGX 540 (revision 120) attached.

There is no external regulator for it so it can be enabled by default.

Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 arch/arm/boot/dts/s5pv210.dtsi | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/arm/boot/dts/s5pv210.dtsi b/arch/arm/boot/dts/s5pv210.dtsi
index 2ad642f51fd9..abbdda205c1b 100644
--- a/arch/arm/boot/dts/s5pv210.dtsi
+++ b/arch/arm/boot/dts/s5pv210.dtsi
@@ -512,6 +512,19 @@ vic3: interrupt-controller@f2300000 {
 			#interrupt-cells = <1>;
 		};
 
+		gpu: gpu@f3000000 {
+			compatible = "samsung,s5pv210-sgx540-120";
+			reg = <0xf3000000 0x10000>;
+			interrupt-parent = <&vic2>;
+			interrupts = <10>;
+			clock-names = "core";
+			clocks = <&clocks CLK_G3D>;
+
+			assigned-clocks = <&clocks MOUT_G3D>, <&clocks DOUT_G3D>;
+			assigned-clock-rates = <0>, <66700000>;
+			assigned-clock-parents = <&clocks MOUT_MPLL>;
+		};
+
 		fimd: fimd@f8000000 {
 			compatible = "samsung,s5pv210-fimd";
 			interrupt-parent = <&vic2>;
-- 
2.25.1

[PATCH v7 09/12] ARM: dts: sun6i: a31: add sgx gpu child node

[H. Nikolaus Schaller]

From: Philipp Rossak <embed3d@gmail.com>

We are adding the devicetree binding for the PVR-SGX-544-115 gpu.

This driver is currently under development in the openpvrsgx-devgroup.
Right now the full binding is not figured out, so we provide here a
placeholder. It will be completed as soon as there is a demo available.

The currently used binding that is used during development is more
complete and was already verifyed by loading the kernelmodule successful.

Signed-off-by: Philipp Rossak <embed3d@gmail.com>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 arch/arm/boot/dts/sun6i-a31.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
index f3425a66fc0a..933a825bf460 100644
--- a/arch/arm/boot/dts/sun6i-a31.dtsi
+++ b/arch/arm/boot/dts/sun6i-a31.dtsi
@@ -1417,5 +1417,16 @@ p2wi: i2c@1f03400 {
 			#address-cells = <1>;
 			#size-cells = <0>;
 		};
+
+		gpu: gpu@1c400000 {
+			compatible = "allwinner,sun8i-a31-sgx544-115",
+				     "img,sgx544-115", "img,sgx544";
+			reg = <0x01c40000 0x10000>;
+			/*
+			 * This node is currently a placeholder for the gpu.
+			 * This will be completed when a full demonstration
+			 * of the openpvrsgx driver is available for this board.
+			 */
+		};
 	};
 };
-- 
2.25.1

[PATCH v7 10/12] ARM: dts: sun6i: a31s: add sgx gpu child node

[H. Nikolaus Schaller]

From: Philipp Rossak <embed3d@gmail.com>

We are adding the devicetree binding for the PVR-SGX-544-115 gpu.

This driver is currently under development in the openpvrsgx-devgroup.
Right now the full binding is not figured out, so we provide here a
placeholder. It will be completed as soon as there is a demo available.

The currently used binding that is used during development is more
complete and was already verifyed by loading the kernelmodule successful.

Signed-off-by: Philipp Rossak <embed3d@gmail.com>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 arch/arm/boot/dts/sun6i-a31s.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/sun6i-a31s.dtsi b/arch/arm/boot/dts/sun6i-a31s.dtsi
index 97e2c51d0aea..669770d2934a 100644
--- a/arch/arm/boot/dts/sun6i-a31s.dtsi
+++ b/arch/arm/boot/dts/sun6i-a31s.dtsi
@@ -59,3 +59,13 @@ &pio {
 &tcon0 {
 	compatible = "allwinner,sun6i-a31s-tcon";
 };
+
+&gpu {
+	compatible = "allwinner,sun8i-a31s-sgx544-115",
+		     "img,sgx544-115", "img,sgx544";
+	/*
+	 * This node is currently a placeholder for the gpu.
+	 * This will be completed when a full demonstration
+	 * of the openpvrsgx driver is available for this board.
+	 */
+};
-- 
2.25.1

[PATCH v7 11/12] ARM: dts: sun8i: a83t: add sgx gpu child node

[H. Nikolaus Schaller]

From: Philipp Rossak <embed3d@gmail.com>

We are adding the devicetree binding for the PVR-SGX-544-115 gpu.

This driver is currently under development in the openpvrsgx-devgroup.
Right now the full binding is not figured out, so we provide here a
placeholder. It will be completed as soon as there is a demo available.

The currently used binding that is used during development is more
complete and was already verifyed by loading the kernelmodule successful.

Signed-off-by: Philipp Rossak <embed3d@gmail.com>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 arch/arm/boot/dts/sun8i-a83t.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-a83t.dtsi b/arch/arm/boot/dts/sun8i-a83t.dtsi
index 655404d6d3a3..bfb900622bf6 100644
--- a/arch/arm/boot/dts/sun8i-a83t.dtsi
+++ b/arch/arm/boot/dts/sun8i-a83t.dtsi
@@ -1192,6 +1192,17 @@ ths: thermal-sensor@1f04000 {
 			nvmem-cell-names = "calibration";
 			#thermal-sensor-cells = <1>;
 		};
+
+		gpu: gpu@1c400000 {
+			compatible = "allwinner,sun8i-a83t-sgx544-115",
+				     "img,sgx544-115", "img,sgx544";
+			reg = <0x01c40000 0x10000>;
+			/*
+			 * This node is currently a placeholder for the gpu.
+			 * This will be completed when a full demonstration
+			 * of the openpvrsgx driver is available for this board.
+			 */
+		};
 	};
 
 	thermal-zones {
-- 
2.25.1

[PATCH v7 12/12] MIPS: DTS: jz4780: add sgx gpu node

[H. Nikolaus Schaller]

and add interrupt and clocks.

Tested to build for CI20 board and load a driver.
Setup can not yet be fully tested since there is no working
HDMI driver for jz4780.

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 bb89653d16a3..92b91aec7993 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -357,6 +357,17 @@ i2c4: i2c@10054000 {
 		status = "disabled";
 	};
 
+	gpu: gpu@13040000 {
+		compatible = "ingenic,jz4780-sgx540-130", "img,sgx540-130", "img,sgx540";
+		reg = <0x13040000 0x4000>;
+
+		clocks = <&cgu JZ4780_CLK_GPU>;
+		clock-names = "core";
+
+		interrupt-parent = <&intc>;
+		interrupts = <63>;
+	};
+
 	nemc: nemc@13410000 {
 		compatible = "ingenic,jz4780-nemc";
 		reg = <0x13410000 0x10000>;
-- 
2.25.1

Re: [PATCH v7 01/12] dt-bindings: add img,pvrsgx.yaml for Imagination GPUs

[H. Nikolaus Schaller]

> Am 24.04.2020 um 22:34 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> 
> The Imagination PVR/SGX GPU is part of several SoC from
> multiple vendors, e.g. TI OMAP, Ingenic JZ4780, Intel Poulsbo,
> Allwinner A83 and others.
> 
> With this binding, we describe how the SGX processor is
> interfaced to the SoC (registers and interrupt).
> 
> The interface also consists of clocks, reset, power but
> information from data sheets is vague and some SoC integrators
> (TI) deciced to use a PRCM wrapper (ti,sysc) which does

s/deciced/decided/

> all clock, reset and power-management through registers
> outside of the sgx register block.
> 
> Therefore all these properties are optional.
> 
> Tested by make dt_binding_check
> 
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
> .../devicetree/bindings/gpu/img,pvrsgx.yaml   | 150 ++++++++++++++++++
> 1 file changed, 150 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..33a9c4c6e784
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml
> @@ -0,0 +1,150 @@
> +# 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 extensive list see: https://en.wikipedia.org/wiki/PowerVR#Implementations
> +
> +  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. If not, an optional clock can be specified here.

     ^^^ this is no longer that way. now clocks, reset etc. are part of this
         node but can be omitted if done by the parent node.

     => either remove this sentence or rewrite.

> +
> +properties:
> +  $nodename:
> +    pattern: '^gpu@[a-f0-9]+$'
> +  compatible:
> +    oneOf:
> +      - description: SGX530-121 based SoC
> +        items:
> +          - enum:
> +            - ti,omap3-sgx530-121 # BeagleBoard A/B/C, OpenPandora 600MHz and similar
> +          - const: img,sgx530-121
> +          - const: img,sgx530
> +
> +      - description: SGX530-125 based SoC
> +        items:
> +          - enum:
> +            - ti,am3352-sgx530-125 # BeagleBone Black
> +            - ti,am3517-sgx530-125
> +            - ti,am4-sgx530-125
> +            - ti,omap3-sgx530-125 # BeagleBoard XM, GTA04, OpenPandora 1GHz and similar
> +            - ti,ti81xx-sgx530-125
> +          - const: ti,omap3-sgx530-125
> +          - const: img,sgx530-125
> +          - const: img,sgx530
> +
> +      - description: SGX535-116 based SoC
> +        items:
> +          - const: intel,poulsbo-gma500-sgx535 # Atom Z5xx
> +          - const: img,sgx535-116
> +          - const: img,sgx535
> +
> +      - description: SGX540-116 based SoC
> +        items:
> +          - const: intel,medfield-gma-sgx540 # Atom Z24xx
> +          - const: img,sgx540-116
> +          - const: img,sgx540
> +
> +      - description: SGX540-120 based SoC
> +        items:
> +          - enum:
> +            - samsung,s5pv210-sgx540-120
> +            - ti,omap4-sgx540-120 # Pandaboard, Pandaboard ES and similar
> +          - const: img,sgx540-120
> +          - const: img,sgx540
> +
> +      - description: SGX540-130 based SoC
> +        items:
> +          - enum:
> +            - ingenic,jz4780-sgx540-130 # CI20
> +          - const: img,sgx540-130
> +          - const: img,sgx540
> +
> +      - description: SGX544-112 based SoC
> +        items:
> +          - const: ti,omap4470-sgx544-112
> +          - const: img,sgx544-112
> +          - const: img,sgx544
> +
> +      - description: SGX544-115 based SoC
> +        items:
> +          - enum:
> +            - allwinner,sun8i-a31-sgx544-115
> +            - allwinner,sun8i-a31s-sgx544-115
> +            - allwinner,sun8i-a83t-sgx544-115 # Banana-Pi-M3 (Allwinner A83T) and similar
> +          - const: img,sgx544-115
> +          - const: img,sgx544
> +
> +      - description: SGX544-116 based SoC
> +        items:
> +          - enum:
> +            - ti,dra7-sgx544-116 # DRA7
> +            - ti,omap5-sgx544-116 # OMAP5 UEVM, Pyra Handheld and similar
> +          - const: img,sgx544-116
> +          - const: img,sgx544
> +
> +      - description: SGX545 based SoC
> +        items:
> +          - const: intel,cedarview-gma3600-sgx545 # Atom N2600, D2500
> +          - const: img,sgx545-116
> +          - const: img,sgx545
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  interrupt-names:
> +    maxItems: 1
> +    items:
> +      - const: sgx
> +
> +  clocks:
> +    maxItems: 4
> +
> +  clock-names:
> +    maxItems: 4
> +    items:
> +      - const: core
> +      - const: sys
> +      - const: mem
> +      - const: hyd
> +
> +  sgx-supply: true
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |+
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    gpu: gpu@fe00 {
> +      compatible = "ti,omap5-sgx544-116", "img,sgx544-116", "img,sgx544";
> +      reg = <0xfe00 0x200>;
> +      interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
> +    };
> +
> +...
> -- 
> 2.25.1
> 

Re: [PATCH v7 06/12] ARM: DTS: omap4: add sgx gpu child node

[Paul Cercueil]

Hi Nikolaus,

Le ven. 24 avril 2020 à 22:34, H. Nikolaus Schaller 
<hns@goldelico.com> a écrit :
> Add SGX GPU node with interrupt. Tested on PandaBoard ES.
> 
> 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.
> 
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  arch/arm/boot/dts/omap4.dtsi   | 11 ++++++-----
>  arch/arm/boot/dts/omap4470.dts | 15 +++++++++++++++
>  2 files changed, 21 insertions(+), 5 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 763bdea8c829..15ff3d7146af 100644
> --- a/arch/arm/boot/dts/omap4.dtsi
> +++ b/arch/arm/boot/dts/omap4.dtsi
> @@ -389,7 +389,7 @@ abb_iva: regulator-abb-iva {
>  			status = "disabled";
>  		};
> 
> -		target-module@56000000 {
> +		sgx_module: target-module@56000000 {
>  			compatible = "ti,sysc-omap4", "ti,sysc";
>  			reg = <0x5600fe00 0x4>,
>  			      <0x5600fe10 0x4>;
> @@ -408,10 +408,11 @@ target-module@56000000 {
>  			#size-cells = <1>;
>  			ranges = <0 0x56000000 0x2000000>;
> 
> -			/*
> -			 * Closed source PowerVR driver, no child device
> -			 * binding or driver in mainline
> -			 */
> +			gpu: gpu@0 {
> +				compatible = "ti,omap4-sgx540-120", "img,sgx540-120", 
> "img,sgx540";
> +				reg = <0x0 0x2000000>;	/* 32MB */
> +				interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
> +			};
>  		};
> 
>  		/*
> diff --git a/arch/arm/boot/dts/omap4470.dts 
> b/arch/arm/boot/dts/omap4470.dts
> new file mode 100644
> index 000000000000..f29c581300bf
> --- /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 {

Does this even compile?

The node's handle is named sgx_module, not sgx.

-Paul

> +	compatible = "ti,omap4470-sgx544-112", "img,sgx544-112", 
> "img,sgx544";
> +};
> --
> 2.25.1
> 

Re: [PATCH v7 09/12] ARM: dts: sun6i: a31: add sgx gpu child node

[Paul Cercueil]

Le ven. 24 avril 2020 à 22:34, H. Nikolaus Schaller 
<hns@goldelico.com> a écrit :
> From: Philipp Rossak <embed3d@gmail.com>
> 
> We are adding the devicetree binding for the PVR-SGX-544-115 gpu.
> 
> This driver is currently under development in the openpvrsgx-devgroup.
> Right now the full binding is not figured out, so we provide here a
> placeholder. It will be completed as soon as there is a demo 
> available.
> 
> The currently used binding that is used during development is more
> complete and was already verifyed by loading the kernelmodule 
> successful.
> 
> Signed-off-by: Philipp Rossak <embed3d@gmail.com>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  arch/arm/boot/dts/sun6i-a31.dtsi | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi 
> b/arch/arm/boot/dts/sun6i-a31.dtsi
> index f3425a66fc0a..933a825bf460 100644
> --- a/arch/arm/boot/dts/sun6i-a31.dtsi
> +++ b/arch/arm/boot/dts/sun6i-a31.dtsi
> @@ -1417,5 +1417,16 @@ p2wi: i2c@1f03400 {
>  			#address-cells = <1>;
>  			#size-cells = <0>;
>  		};
> +
> +		gpu: gpu@1c400000 {
> +			compatible = "allwinner,sun8i-a31-sgx544-115",
> +				     "img,sgx544-115", "img,sgx544";
> +			reg = <0x01c40000 0x10000>;
> +			/*
> +			 * This node is currently a placeholder for the gpu.
> +			 * This will be completed when a full demonstration
> +			 * of the openpvrsgx driver is available for this board.
> +			 */

This node doesn't have clocks, so I don't see how it'd work.

Better delay the introduction of the GPU node for this board until you 
know it works.

-Paul

> +		};
>  	};
>  };
> --
> 2.25.1
> 

Re: [PATCH v7 08/12] arm: dts: s5pv210: Add node for SGX 540

[Paul Cercueil]

Le ven. 24 avril 2020 à 22:34, H. Nikolaus Schaller 
<hns@goldelico.com> a écrit :
> From: Jonathan Bakker <xc-racer2@live.ca>
> 
> All s5pv210 devices have a PowerVR SGX 540 (revision 120) attached.
> 
> There is no external regulator for it so it can be enabled by default.
> 
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  arch/arm/boot/dts/s5pv210.dtsi | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/s5pv210.dtsi 
> b/arch/arm/boot/dts/s5pv210.dtsi
> index 2ad642f51fd9..abbdda205c1b 100644
> --- a/arch/arm/boot/dts/s5pv210.dtsi
> +++ b/arch/arm/boot/dts/s5pv210.dtsi
> @@ -512,6 +512,19 @@ vic3: interrupt-controller@f2300000 {
>  			#interrupt-cells = <1>;
>  		};
> 
> +		gpu: gpu@f3000000 {
> +			compatible = "samsung,s5pv210-sgx540-120";
> +			reg = <0xf3000000 0x10000>;
> +			interrupt-parent = <&vic2>;
> +			interrupts = <10>;
> +			clock-names = "core";
> +			clocks = <&clocks CLK_G3D>;
> +
> +			assigned-clocks = <&clocks MOUT_G3D>, <&clocks DOUT_G3D>;
> +			assigned-clock-rates = <0>, <66700000>;
> +			assigned-clock-parents = <&clocks MOUT_MPLL>;

What are these clocks for, and why are they reparented / reclocked?

Shouldn't they be passed to 'clocks' as well?

-Paul

> +		};
> +
>  		fimd: fimd@f8000000 {
>  			compatible = "samsung,s5pv210-fimd";
>  			interrupt-parent = <&vic2>;
> --
> 2.25.1
> 

Re: [PATCH v7 01/12] dt-bindings: add img,pvrsgx.yaml for Imagination GPUs

[Paul Cercueil]

Hi Nikolaus,

Le ven. 24 avril 2020 à 22:34, H. Nikolaus Schaller 
<hns@goldelico.com> a écrit :
> The Imagination PVR/SGX GPU is part of several SoC from
> multiple vendors, e.g. TI OMAP, Ingenic JZ4780, Intel Poulsbo,
> Allwinner A83 and others.
> 
> With this binding, we describe how the SGX processor is
> interfaced to the SoC (registers and interrupt).
> 
> The interface also consists of clocks, reset, power but
> information from data sheets is vague and some SoC integrators
> (TI) deciced to use a PRCM wrapper (ti,sysc) which does
> all clock, reset and power-management through registers
> outside of the sgx register block.
> 
> Therefore all these properties are optional.
> 
> Tested by make dt_binding_check
> 
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  .../devicetree/bindings/gpu/img,pvrsgx.yaml   | 150 
> ++++++++++++++++++
>  1 file changed, 150 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..33a9c4c6e784
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml
> @@ -0,0 +1,150 @@
> +# 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 extensive list see: 
> https://en.wikipedia.org/wiki/PowerVR#Implementations
> +
> +  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. If not, an optional clock can be specified here.
> +
> +properties:
> +  $nodename:
> +    pattern: '^gpu@[a-f0-9]+$'
> +  compatible:
> +    oneOf:
> +      - description: SGX530-121 based SoC
> +        items:
> +          - enum:
> +            - ti,omap3-sgx530-121 # BeagleBoard A/B/C, OpenPandora 
> 600MHz and similar
> +          - const: img,sgx530-121
> +          - const: img,sgx530
> +
> +      - description: SGX530-125 based SoC
> +        items:
> +          - enum:
> +            - ti,am3352-sgx530-125 # BeagleBone Black
> +            - ti,am3517-sgx530-125
> +            - ti,am4-sgx530-125
> +            - ti,omap3-sgx530-125 # BeagleBoard XM, GTA04, 
> OpenPandora 1GHz and similar
> +            - ti,ti81xx-sgx530-125
> +          - const: ti,omap3-sgx530-125
> +          - const: img,sgx530-125
> +          - const: img,sgx530
> +
> +      - description: SGX535-116 based SoC
> +        items:
> +          - const: intel,poulsbo-gma500-sgx535 # Atom Z5xx
> +          - const: img,sgx535-116
> +          - const: img,sgx535
> +
> +      - description: SGX540-116 based SoC
> +        items:
> +          - const: intel,medfield-gma-sgx540 # Atom Z24xx
> +          - const: img,sgx540-116
> +          - const: img,sgx540
> +
> +      - description: SGX540-120 based SoC
> +        items:
> +          - enum:
> +            - samsung,s5pv210-sgx540-120
> +            - ti,omap4-sgx540-120 # Pandaboard, Pandaboard ES and 
> similar
> +          - const: img,sgx540-120
> +          - const: img,sgx540
> +
> +      - description: SGX540-130 based SoC
> +        items:
> +          - enum:
> +            - ingenic,jz4780-sgx540-130 # CI20
> +          - const: img,sgx540-130
> +          - const: img,sgx540
> +
> +      - description: SGX544-112 based SoC
> +        items:
> +          - const: ti,omap4470-sgx544-112
> +          - const: img,sgx544-112
> +          - const: img,sgx544
> +
> +      - description: SGX544-115 based SoC
> +        items:
> +          - enum:
> +            - allwinner,sun8i-a31-sgx544-115
> +            - allwinner,sun8i-a31s-sgx544-115
> +            - allwinner,sun8i-a83t-sgx544-115 # Banana-Pi-M3 
> (Allwinner A83T) and similar
> +          - const: img,sgx544-115
> +          - const: img,sgx544
> +
> +      - description: SGX544-116 based SoC
> +        items:
> +          - enum:
> +            - ti,dra7-sgx544-116 # DRA7
> +            - ti,omap5-sgx544-116 # OMAP5 UEVM, Pyra Handheld and 
> similar
> +          - const: img,sgx544-116
> +          - const: img,sgx544
> +
> +      - description: SGX545 based SoC
> +        items:
> +          - const: intel,cedarview-gma3600-sgx545 # Atom N2600, D2500
> +          - const: img,sgx545-116
> +          - const: img,sgx545
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  interrupt-names:
> +    maxItems: 1
> +    items:
> +      - const: sgx
> +
> +  clocks:
> +    maxItems: 4
> +
> +  clock-names:
> +    maxItems: 4
> +    items:
> +      - const: core
> +      - const: sys
> +      - const: mem
> +      - const: hyd
> +
> +  sgx-supply: true
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts

By not making 'clocks' required you make it possible to create broken 
bindings; according to your schema, a GPU node without a 'clocks' for 
the JZ4780 would be perfectly valid.

It's possible to forbid the presence of the 'clocks' property on some 
implementations, and require it on others.
See how it's done for instance on 
Documentation/devicetree/bindings/serial/samsung_uart.yaml.

-Paul

> +
> +additionalProperties: false
> +
> +examples:
> +  - |+
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    gpu: gpu@fe00 {
> +      compatible = "ti,omap5-sgx544-116", "img,sgx544-116", 
> "img,sgx544";
> +      reg = <0xfe00 0x200>;
> +      interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
> +    };
> +
> +...
> --
> 2.25.1
> 

Re: [PATCH v7 08/12] arm: dts: s5pv210: Add node for SGX 540

[Jonathan Bakker]

Hi Paul,

On 2020-04-26 5:56 a.m., Paul Cercueil wrote:
> 
> 
> Le ven. 24 avril 2020 à 22:34, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>> From: Jonathan Bakker <xc-racer2@live.ca>
>>
>> All s5pv210 devices have a PowerVR SGX 540 (revision 120) attached.
>>
>> There is no external regulator for it so it can be enabled by default.
>>
>> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>>  arch/arm/boot/dts/s5pv210.dtsi | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/s5pv210.dtsi b/arch/arm/boot/dts/s5pv210.dtsi
>> index 2ad642f51fd9..abbdda205c1b 100644
>> --- a/arch/arm/boot/dts/s5pv210.dtsi
>> +++ b/arch/arm/boot/dts/s5pv210.dtsi
>> @@ -512,6 +512,19 @@ vic3: interrupt-controller@f2300000 {
>>              #interrupt-cells = <1>;
>>          };
>>
>> +        gpu: gpu@f3000000 {
>> +            compatible = "samsung,s5pv210-sgx540-120";
>> +            reg = <0xf3000000 0x10000>;
>> +            interrupt-parent = <&vic2>;
>> +            interrupts = <10>;
>> +            clock-names = "core";
>> +            clocks = <&clocks CLK_G3D>;
>> +
>> +            assigned-clocks = <&clocks MOUT_G3D>, <&clocks DOUT_G3D>;
>> +            assigned-clock-rates = <0>, <66700000>;
>> +            assigned-clock-parents = <&clocks MOUT_MPLL>;
> 
> What are these clocks for, and why are they reparented / reclocked?
> 
> Shouldn't they be passed to 'clocks' as well?
> 
> -Paul
> 

The G3D clock system can have multiple parents, and for stable operation
it's recommended to use the MPLL clock as the parent (which in turn
is actually a mux as well).  MOUT_G3D is simply the mux for CLK_G3D
(SGX core clock), DOUT_G3D is the divider.  DOUT_G3D could equally be CLK_G3D
(and probably should be, for readability) as CLK_G3D is simply the gate and
DOUT_G3D is the divider for it.

The SGX clock layout on S5PV210 looks something like this:

        MOUT_MPLL -----------> MOUT_G3D ---> DOUT_G3D  ---> CLK_G3D
(selectable parent clock)      (mux)    ---> (divider) ---> (gate)

This is fairly common for older Samsung SoCs, eg having a look at
arch/arm/boot/dts/exynos4210-universal_c210.dts you can see that
the FIMC clocks are parented to MPLL and have a rate set.

>> +        };
>> +
>>          fimd: fimd@f8000000 {
>>              compatible = "samsung,s5pv210-fimd";
>>              interrupt-parent = <&vic2>;
>> -- 
>> 2.25.1
>>
> 
> 

Thanks,
Jonathan

Re: [PATCH v7 09/12] ARM: dts: sun6i: a31: add sgx gpu child node

[Philipp Rossak]

Hi Paul,

On 26.04.20 14:53, Paul Cercueil wrote:
> 
> 
> Le ven. 24 avril 2020 à 22:34, H. Nikolaus Schaller <hns@goldelico.com> 
> a écrit :
>> From: Philipp Rossak <embed3d@gmail.com>
>>
>> We are adding the devicetree binding for the PVR-SGX-544-115 gpu.
>>
>> This driver is currently under development in the openpvrsgx-devgroup.
>> Right now the full binding is not figured out, so we provide here a
>> placeholder. It will be completed as soon as there is a demo available.
>>
>> The currently used binding that is used during development is more
>> complete and was already verifyed by loading the kernelmodule successful.
>>
>> Signed-off-by: Philipp Rossak <embed3d@gmail.com>
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>>  arch/arm/boot/dts/sun6i-a31.dtsi | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi 
>> b/arch/arm/boot/dts/sun6i-a31.dtsi
>> index f3425a66fc0a..933a825bf460 100644
>> --- a/arch/arm/boot/dts/sun6i-a31.dtsi
>> +++ b/arch/arm/boot/dts/sun6i-a31.dtsi
>> @@ -1417,5 +1417,16 @@ p2wi: i2c@1f03400 {
>>              #address-cells = <1>;
>>              #size-cells = <0>;
>>          };
>> +
>> +        gpu: gpu@1c400000 {
>> +            compatible = "allwinner,sun8i-a31-sgx544-115",

looks like a copy paste error from my side this should be 
allwinner,sun6i-a31-sgx544-115

>> +                     "img,sgx544-115", "img,sgx544";
>> +            reg = <0x01c40000 0x10000>;
>> +            /*
>> +             * This node is currently a placeholder for the gpu.
>> +             * This will be completed when a full demonstration
>> +             * of the openpvrsgx driver is available for this board.
>> +             */
> 
> This node doesn't have clocks, so I don't see how it'd work.
> 
> Better delay the introduction of the GPU node for this board until you 
> know it works.
> 
> -Paul
This was already discuss in an earlier version that series that this 
should be delayed.

I will send a follow up patch series, as soon as I mainlined an other 
driver that I'm working on, which is required to properly describe the gpu.

Cheers
Philipp

Re: [PATCH v7 01/12] dt-bindings: add img,pvrsgx.yaml for Imagination GPUs

[Philipp Rossak]

Hi Nikolaus,

On 24.04.20 22:34, 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,
> Allwinner A83 and others.
> 
> With this binding, we describe how the SGX processor is
> interfaced to the SoC (registers and interrupt).
> 
> The interface also consists of clocks, reset, power but
> information from data sheets is vague and some SoC integrators
> (TI) deciced to use a PRCM wrapper (ti,sysc) which does
> all clock, reset and power-management through registers
> outside of the sgx register block.
> 
> Therefore all these properties are optional.
> 
> Tested by make dt_binding_check
> 
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>   .../devicetree/bindings/gpu/img,pvrsgx.yaml   | 150 ++++++++++++++++++
>   1 file changed, 150 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..33a9c4c6e784
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml
> @@ -0,0 +1,150 @@
> +# 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 extensive list see: https://en.wikipedia.org/wiki/PowerVR#Implementations
> +
> +  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. If not, an optional clock can be specified here.
> +
> +properties:
> +  $nodename:
> +    pattern: '^gpu@[a-f0-9]+$'
> +  compatible:
> +    oneOf:
> +      - description: SGX530-121 based SoC
> +        items:
> +          - enum:
> +            - ti,omap3-sgx530-121 # BeagleBoard A/B/C, OpenPandora 600MHz and similar
> +          - const: img,sgx530-121
> +          - const: img,sgx530
> +
> +      - description: SGX530-125 based SoC
> +        items:
> +          - enum:
> +            - ti,am3352-sgx530-125 # BeagleBone Black
> +            - ti,am3517-sgx530-125
> +            - ti,am4-sgx530-125
> +            - ti,omap3-sgx530-125 # BeagleBoard XM, GTA04, OpenPandora 1GHz and similar
> +            - ti,ti81xx-sgx530-125
> +          - const: ti,omap3-sgx530-125
> +          - const: img,sgx530-125
> +          - const: img,sgx530
> +
> +      - description: SGX535-116 based SoC
> +        items:
> +          - const: intel,poulsbo-gma500-sgx535 # Atom Z5xx
> +          - const: img,sgx535-116
> +          - const: img,sgx535
> +
> +      - description: SGX540-116 based SoC
> +        items:
> +          - const: intel,medfield-gma-sgx540 # Atom Z24xx
> +          - const: img,sgx540-116
> +          - const: img,sgx540
> +
> +      - description: SGX540-120 based SoC
> +        items:
> +          - enum:
> +            - samsung,s5pv210-sgx540-120
> +            - ti,omap4-sgx540-120 # Pandaboard, Pandaboard ES and similar
> +          - const: img,sgx540-120
> +          - const: img,sgx540
> +
> +      - description: SGX540-130 based SoC
> +        items:
> +          - enum:
> +            - ingenic,jz4780-sgx540-130 # CI20
> +          - const: img,sgx540-130
> +          - const: img,sgx540
> +
> +      - description: SGX544-112 based SoC
> +        items:
> +          - const: ti,omap4470-sgx544-112
> +          - const: img,sgx544-112
> +          - const: img,sgx544
> +
> +      - description: SGX544-115 based SoC
> +        items:
> +          - enum:
> +            - allwinner,sun8i-a31-sgx544-115
> +            - allwinner,sun8i-a31s-sgx544-115
those two bindings are wrong.
It should be allwinner,sun6i-a31-sgx544-115 and 
allwinner,sun6i-a31s-sgx544-115. I did a copy paste error in the patches 
that I provided for this series.


Cheers,
Philipp

Re: [PATCH v7 01/12] dt-bindings: add img,pvrsgx.yaml for Imagination GPUs

[H. Nikolaus Schaller]

Hi Philipp,

> Am 26.04.2020 um 21:36 schrieb Philipp Rossak <embed3d@gmail.com>:
> 
> Hi Nikolaus,
> 
> On 24.04.20 22:34, 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,
>> Allwinner A83 and others.
>> With this binding, we describe how the SGX processor is
>> interfaced to the SoC (registers and interrupt).
>> The interface also consists of clocks, reset, power but
>> information from data sheets is vague and some SoC integrators
>> (TI) deciced to use a PRCM wrapper (ti,sysc) which does
>> all clock, reset and power-management through registers
>> outside of the sgx register block.
>> Therefore all these properties are optional.
>> Tested by make dt_binding_check
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>>  .../devicetree/bindings/gpu/img,pvrsgx.yaml   | 150 ++++++++++++++++++
>>  1 file changed, 150 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..33a9c4c6e784
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml
>> @@ -0,0 +1,150 @@
>> +# 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
>> +

...

>> +      - description: SGX544-112 based SoC
>> +        items:
>> +          - const: ti,omap4470-sgx544-112
>> +          - const: img,sgx544-112
>> +          - const: img,sgx544
>> +
>> +      - description: SGX544-115 based SoC
>> +        items:
>> +          - enum:
>> +            - allwinner,sun8i-a31-sgx544-115
>> +            - allwinner,sun8i-a31s-sgx544-115
> those two bindings are wrong.
> It should be allwinner,sun6i-a31-sgx544-115 and allwinner,sun6i-a31s-sgx544-115. I did a copy paste error in the patches that I provided for this series.

Ok, noted for v8.

BR and thanks,
Nikolaus

Re: [PATCH v7 08/12] arm: dts: s5pv210: Add node for SGX 540

[Krzysztof Kozlowski]

On Sun, Apr 26, 2020 at 07:57:12AM -0700, Jonathan Bakker wrote:
> Hi Paul,
> 
> On 2020-04-26 5:56 a.m., Paul Cercueil wrote:
> > 
> > 
> > Le ven. 24 avril 2020 à 22:34, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
> >> From: Jonathan Bakker <xc-racer2@live.ca>
> >>
> >> All s5pv210 devices have a PowerVR SGX 540 (revision 120) attached.
> >>
> >> There is no external regulator for it so it can be enabled by default.
> >>
> >> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
> >> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> >> ---
> >>  arch/arm/boot/dts/s5pv210.dtsi | 13 +++++++++++++
> >>  1 file changed, 13 insertions(+)
> >>
> >> diff --git a/arch/arm/boot/dts/s5pv210.dtsi b/arch/arm/boot/dts/s5pv210.dtsi
> >> index 2ad642f51fd9..abbdda205c1b 100644
> >> --- a/arch/arm/boot/dts/s5pv210.dtsi
> >> +++ b/arch/arm/boot/dts/s5pv210.dtsi
> >> @@ -512,6 +512,19 @@ vic3: interrupt-controller@f2300000 {
> >>              #interrupt-cells = <1>;
> >>          };
> >>
> >> +        gpu: gpu@f3000000 {
> >> +            compatible = "samsung,s5pv210-sgx540-120";

This should not pass the bindings check because you missed last
compatibles.

> >> +            reg = <0xf3000000 0x10000>;
> >> +            interrupt-parent = <&vic2>;
> >> +            interrupts = <10>;
> >> +            clock-names = "core";
> >> +            clocks = <&clocks CLK_G3D>;
> >> +
> >> +            assigned-clocks = <&clocks MOUT_G3D>, <&clocks DOUT_G3D>;
> >> +            assigned-clock-rates = <0>, <66700000>;
> >> +            assigned-clock-parents = <&clocks MOUT_MPLL>;
> > 
> > What are these clocks for, and why are they reparented / reclocked?
> > 
> > Shouldn't they be passed to 'clocks' as well?
> > 
> > -Paul
> > 
> 
> The G3D clock system can have multiple parents, and for stable operation
> it's recommended to use the MPLL clock as the parent (which in turn
> is actually a mux as well).  MOUT_G3D is simply the mux for CLK_G3D
> (SGX core clock), DOUT_G3D is the divider.  DOUT_G3D could equally be CLK_G3D
> (and probably should be, for readability) as CLK_G3D is simply the gate and
> DOUT_G3D is the divider for it.

Good point, it should be CLK_G3D instead of DOUT.  Can you fix this as
well?

Best regards,
Krzysztof

Re: [PATCH v7 06/12] ARM: DTS: omap4: add sgx gpu child node

[H. Nikolaus Schaller]

Hi Paul,

> Am 26.04.2020 um 14:50 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> Hi Nikolaus,
> 
> Le ven. 24 avril 2020 à 22:34, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>> Add SGX GPU node with interrupt. Tested on PandaBoard ES.
>> 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.
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> arch/arm/boot/dts/omap4.dtsi   | 11 ++++++-----
>> arch/arm/boot/dts/omap4470.dts | 15 +++++++++++++++
>> 2 files changed, 21 insertions(+), 5 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 763bdea8c829..15ff3d7146af 100644
>> --- a/arch/arm/boot/dts/omap4.dtsi
>> +++ b/arch/arm/boot/dts/omap4.dtsi
>> @@ -389,7 +389,7 @@ abb_iva: regulator-abb-iva {
>> 			status = "disabled";
>> 		};
>> -		target-module@56000000 {
>> +		sgx_module: target-module@56000000 {
>> 			compatible = "ti,sysc-omap4", "ti,sysc";
>> 			reg = <0x5600fe00 0x4>,
>> 			      <0x5600fe10 0x4>;
>> @@ -408,10 +408,11 @@ target-module@56000000 {
>> 			#size-cells = <1>;
>> 			ranges = <0 0x56000000 0x2000000>;
>> -			/*
>> -			 * Closed source PowerVR driver, no child device
>> -			 * binding or driver in mainline
>> -			 */
>> +			gpu: gpu@0 {
>> +				compatible = "ti,omap4-sgx540-120", "img,sgx540-120", "img,sgx540";
>> +				reg = <0x0 0x2000000>;	/* 32MB */
>> +				interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
>> +			};
>> 		};
>> 		/*
>> diff --git a/arch/arm/boot/dts/omap4470.dts b/arch/arm/boot/dts/omap4470.dts
>> new file mode 100644
>> index 000000000000..f29c581300bf
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/omap4470.dts

^^^ there is also a missing "i" in the file name

>> @@ -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 {
> 
> Does this even compile?

Good question.

So far there is no well known eval board in mainline that #includes this .dtsi (because it is new) and therefore it passes any compile tests.

  DTC     arch/arm/boot/dts/omap4470-test.dtb - due to target missing
Error: arch/arm/boot/dts/omap4470.dtsi:13.1-5 Label or path sgx not found

I have now added a dummy board (not to be mainlined) for my own compile test...

> 
> The node's handle is named sgx_module, not sgx.

Indeed. A fix is queued for v8.

BR and thanks,
Nikolaus

Re: [PATCH v7 08/12] arm: dts: s5pv210: Add node for SGX 540

[Jonathan Bakker]

Hi Krzysztof,

On 2020-04-27 8:46 a.m., Krzysztof Kozlowski wrote:
> On Sun, Apr 26, 2020 at 07:57:12AM -0700, Jonathan Bakker wrote:
>> Hi Paul,
>>
>> On 2020-04-26 5:56 a.m., Paul Cercueil wrote:
>>>
>>>
>>> Le ven. 24 avril 2020 à 22:34, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>>>> From: Jonathan Bakker <xc-racer2@live.ca>
>>>>
>>>> All s5pv210 devices have a PowerVR SGX 540 (revision 120) attached.
>>>>
>>>> There is no external regulator for it so it can be enabled by default.
>>>>
>>>> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>>> ---
>>>>  arch/arm/boot/dts/s5pv210.dtsi | 13 +++++++++++++
>>>>  1 file changed, 13 insertions(+)
>>>>
>>>> diff --git a/arch/arm/boot/dts/s5pv210.dtsi b/arch/arm/boot/dts/s5pv210.dtsi
>>>> index 2ad642f51fd9..abbdda205c1b 100644
>>>> --- a/arch/arm/boot/dts/s5pv210.dtsi
>>>> +++ b/arch/arm/boot/dts/s5pv210.dtsi
>>>> @@ -512,6 +512,19 @@ vic3: interrupt-controller@f2300000 {
>>>>              #interrupt-cells = <1>;
>>>>          };
>>>>
>>>> +        gpu: gpu@f3000000 {
>>>> +            compatible = "samsung,s5pv210-sgx540-120";
> 
> This should not pass the bindings check because you missed last
> compatibles.
> 

Thanks for pointing that out, I'll add it and make sure it passes the bindings check.

>>>> +            reg = <0xf3000000 0x10000>;
>>>> +            interrupt-parent = <&vic2>;
>>>> +            interrupts = <10>;
>>>> +            clock-names = "core";
>>>> +            clocks = <&clocks CLK_G3D>;
>>>> +
>>>> +            assigned-clocks = <&clocks MOUT_G3D>, <&clocks DOUT_G3D>;
>>>> +            assigned-clock-rates = <0>, <66700000>;
>>>> +            assigned-clock-parents = <&clocks MOUT_MPLL>;
>>>
>>> What are these clocks for, and why are they reparented / reclocked?
>>>
>>> Shouldn't they be passed to 'clocks' as well?
>>>
>>> -Paul
>>>
>>
>> The G3D clock system can have multiple parents, and for stable operation
>> it's recommended to use the MPLL clock as the parent (which in turn
>> is actually a mux as well).  MOUT_G3D is simply the mux for CLK_G3D
>> (SGX core clock), DOUT_G3D is the divider.  DOUT_G3D could equally be CLK_G3D
>> (and probably should be, for readability) as CLK_G3D is simply the gate and
>> DOUT_G3D is the divider for it.
> 
> Good point, it should be CLK_G3D instead of DOUT.  Can you fix this as
> well?

Yep, will do.  Nikolaus, I'll send you an updated patch to include.

> 
> Best regards,
> Krzysztof
> 

Thanks,
Jonathan

Re: [PATCH v7 08/12] arm: dts: s5pv210: Add node for SGX 540

[Jonathan Bakker]

Hi all,

On 2020-04-28 2:39 p.m., Jonathan Bakker wrote:
> Hi Krzysztof,
> 
> On 2020-04-27 8:46 a.m., Krzysztof Kozlowski wrote:
>> On Sun, Apr 26, 2020 at 07:57:12AM -0700, Jonathan Bakker wrote:
>>> Hi Paul,
>>>
>>> On 2020-04-26 5:56 a.m., Paul Cercueil wrote:
>>>>
>>>>
>>>> Le ven. 24 avril 2020 à 22:34, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>>>>> From: Jonathan Bakker <xc-racer2@live.ca>
>>>>>
>>>>> All s5pv210 devices have a PowerVR SGX 540 (revision 120) attached.
>>>>>
>>>>> There is no external regulator for it so it can be enabled by default.
>>>>>
>>>>> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
>>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>>>> ---
>>>>>  arch/arm/boot/dts/s5pv210.dtsi | 13 +++++++++++++
>>>>>  1 file changed, 13 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/boot/dts/s5pv210.dtsi b/arch/arm/boot/dts/s5pv210.dtsi
>>>>> index 2ad642f51fd9..abbdda205c1b 100644
>>>>> --- a/arch/arm/boot/dts/s5pv210.dtsi
>>>>> +++ b/arch/arm/boot/dts/s5pv210.dtsi
>>>>> @@ -512,6 +512,19 @@ vic3: interrupt-controller@f2300000 {
>>>>>              #interrupt-cells = <1>;
>>>>>          };
>>>>>
>>>>> +        gpu: gpu@f3000000 {
>>>>> +            compatible = "samsung,s5pv210-sgx540-120";
>>
>> This should not pass the bindings check because you missed last
>> compatibles.
>>
> 
> Thanks for pointing that out, I'll add it and make sure it passes the bindings check.
> 
>>>>> +            reg = <0xf3000000 0x10000>;
>>>>> +            interrupt-parent = <&vic2>;
>>>>> +            interrupts = <10>;
>>>>> +            clock-names = "core";
>>>>> +            clocks = <&clocks CLK_G3D>;
>>>>> +
>>>>> +            assigned-clocks = <&clocks MOUT_G3D>, <&clocks DOUT_G3D>;
>>>>> +            assigned-clock-rates = <0>, <66700000>;
>>>>> +            assigned-clock-parents = <&clocks MOUT_MPLL>;
>>>>
>>>> What are these clocks for, and why are they reparented / reclocked?
>>>>
>>>> Shouldn't they be passed to 'clocks' as well?
>>>>
>>>> -Paul
>>>>
>>>
>>> The G3D clock system can have multiple parents, and for stable operation
>>> it's recommended to use the MPLL clock as the parent (which in turn
>>> is actually a mux as well).  MOUT_G3D is simply the mux for CLK_G3D
>>> (SGX core clock), DOUT_G3D is the divider.  DOUT_G3D could equally be CLK_G3D
>>> (and probably should be, for readability) as CLK_G3D is simply the gate and
>>> DOUT_G3D is the divider for it.
>>
>> Good point, it should be CLK_G3D instead of DOUT.  Can you fix this as
>> well?
> 
> Yep, will do.  Nikolaus, I'll send you an updated patch to include.
> 

How are assigned-clocks handled in the yaml DT schema?  When running make dtbs_check,
I end up with messages such as

arch/arm/boot/dts/s5pv210-aquila.dt.yaml: gpu@f3000000: 'assigned-clock-parents', 'assigned-clock-rates', 'assigned-clocks' do not match any of the regexes: 'pinctrl-[0-9]+'

Do they need to explicitly be listed as valid entries?

Thanks,
Jonathan

Re: [PATCH v7 08/12] arm: dts: s5pv210: Add node for SGX 540

[Maxime Ripard]

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

Hi,

On Tue, Apr 28, 2020 at 03:58:03PM -0700, Jonathan Bakker wrote:
> On 2020-04-28 2:39 p.m., Jonathan Bakker wrote:
> > On 2020-04-27 8:46 a.m., Krzysztof Kozlowski wrote:
> >> On Sun, Apr 26, 2020 at 07:57:12AM -0700, Jonathan Bakker wrote:
> >>> Hi Paul,
> >>>
> >>> On 2020-04-26 5:56 a.m., Paul Cercueil wrote:
> >>>>
> >>>>
> >>>> Le ven. 24 avril 2020 à 22:34, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
> >>>>> From: Jonathan Bakker <xc-racer2@live.ca>
> >>>>>
> >>>>> All s5pv210 devices have a PowerVR SGX 540 (revision 120) attached.
> >>>>>
> >>>>> There is no external regulator for it so it can be enabled by default.
> >>>>>
> >>>>> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
> >>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> >>>>> ---
> >>>>>  arch/arm/boot/dts/s5pv210.dtsi | 13 +++++++++++++
> >>>>>  1 file changed, 13 insertions(+)
> >>>>>
> >>>>> diff --git a/arch/arm/boot/dts/s5pv210.dtsi b/arch/arm/boot/dts/s5pv210.dtsi
> >>>>> index 2ad642f51fd9..abbdda205c1b 100644
> >>>>> --- a/arch/arm/boot/dts/s5pv210.dtsi
> >>>>> +++ b/arch/arm/boot/dts/s5pv210.dtsi
> >>>>> @@ -512,6 +512,19 @@ vic3: interrupt-controller@f2300000 {
> >>>>>              #interrupt-cells = <1>;
> >>>>>          };
> >>>>>
> >>>>> +        gpu: gpu@f3000000 {
> >>>>> +            compatible = "samsung,s5pv210-sgx540-120";
> >>
> >> This should not pass the bindings check because you missed last
> >> compatibles.
> >>
> > 
> > Thanks for pointing that out, I'll add it and make sure it passes the bindings check.
> > 
> >>>>> +            reg = <0xf3000000 0x10000>;
> >>>>> +            interrupt-parent = <&vic2>;
> >>>>> +            interrupts = <10>;
> >>>>> +            clock-names = "core";
> >>>>> +            clocks = <&clocks CLK_G3D>;
> >>>>> +
> >>>>> +            assigned-clocks = <&clocks MOUT_G3D>, <&clocks DOUT_G3D>;
> >>>>> +            assigned-clock-rates = <0>, <66700000>;
> >>>>> +            assigned-clock-parents = <&clocks MOUT_MPLL>;
> >>>>
> >>>> What are these clocks for, and why are they reparented / reclocked?
> >>>>
> >>>> Shouldn't they be passed to 'clocks' as well?
> >>>>
> >>>> -Paul
> >>>>
> >>>
> >>> The G3D clock system can have multiple parents, and for stable operation
> >>> it's recommended to use the MPLL clock as the parent (which in turn
> >>> is actually a mux as well).  MOUT_G3D is simply the mux for CLK_G3D
> >>> (SGX core clock), DOUT_G3D is the divider.  DOUT_G3D could equally be CLK_G3D
> >>> (and probably should be, for readability) as CLK_G3D is simply the gate and
> >>> DOUT_G3D is the divider for it.
> >>
> >> Good point, it should be CLK_G3D instead of DOUT.  Can you fix this as
> >> well?
> > 
> > Yep, will do.  Nikolaus, I'll send you an updated patch to include.
> > 
> 
> How are assigned-clocks handled in the yaml DT schema?  When running make dtbs_check,
> I end up with messages such as
> 
> arch/arm/boot/dts/s5pv210-aquila.dt.yaml: gpu@f3000000: 'assigned-clock-parents', 'assigned-clock-rates', 'assigned-clocks' do not match any of the regexes: 'pinctrl-[0-9]+'
> 
> Do they need to explicitly be listed as valid entries?

If you have additionalProperties set to false, yes. But you should really
consider not using them in the first place, since they provide no guarantee on
whether the setup you did in the DT will remain during the life of the system.

I'm not sure why it's needed on that SoC in particular, but this should be fixed
in the driver itself (either the clock or the GPU driver).

Maxime

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

Re: [PATCH v7 08/12] arm: dts: s5pv210: Add node for SGX 540

[Paul Cercueil]

Hi Jonathan,

Le mar. 28 avril 2020 à 15:58, Jonathan Bakker <xc-racer2@live.ca> a 
écrit :
> Hi all,
> 
> On 2020-04-28 2:39 p.m., Jonathan Bakker wrote:
>>  Hi Krzysztof,
>> 
>>  On 2020-04-27 8:46 a.m., Krzysztof Kozlowski wrote:
>>>  On Sun, Apr 26, 2020 at 07:57:12AM -0700, Jonathan Bakker wrote:
>>>>  Hi Paul,
>>>> 
>>>>  On 2020-04-26 5:56 a.m., Paul Cercueil wrote:
>>>>> 
>>>>> 
>>>>>  Le ven. 24 avril 2020 à 22:34, H. Nikolaus Schaller 
>>>>> <hns@goldelico.com> a écrit :
>>>>>>  From: Jonathan Bakker <xc-racer2@live.ca>
>>>>>> 
>>>>>>  All s5pv210 devices have a PowerVR SGX 540 (revision 120) 
>>>>>> attached.
>>>>>> 
>>>>>>  There is no external regulator for it so it can be enabled by 
>>>>>> default.
>>>>>> 
>>>>>>  Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
>>>>>>  Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>>>>>  ---
>>>>>>   arch/arm/boot/dts/s5pv210.dtsi | 13 +++++++++++++
>>>>>>   1 file changed, 13 insertions(+)
>>>>>> 
>>>>>>  diff --git a/arch/arm/boot/dts/s5pv210.dtsi 
>>>>>> b/arch/arm/boot/dts/s5pv210.dtsi
>>>>>>  index 2ad642f51fd9..abbdda205c1b 100644
>>>>>>  --- a/arch/arm/boot/dts/s5pv210.dtsi
>>>>>>  +++ b/arch/arm/boot/dts/s5pv210.dtsi
>>>>>>  @@ -512,6 +512,19 @@ vic3: interrupt-controller@f2300000 {
>>>>>>               #interrupt-cells = <1>;
>>>>>>           };
>>>>>> 
>>>>>>  +        gpu: gpu@f3000000 {
>>>>>>  +            compatible = "samsung,s5pv210-sgx540-120";
>>> 
>>>  This should not pass the bindings check because you missed last
>>>  compatibles.
>>> 
>> 
>>  Thanks for pointing that out, I'll add it and make sure it passes 
>> the bindings check.
>> 
>>>>>>  +            reg = <0xf3000000 0x10000>;
>>>>>>  +            interrupt-parent = <&vic2>;
>>>>>>  +            interrupts = <10>;
>>>>>>  +            clock-names = "core";
>>>>>>  +            clocks = <&clocks CLK_G3D>;
>>>>>>  +
>>>>>>  +            assigned-clocks = <&clocks MOUT_G3D>, <&clocks 
>>>>>> DOUT_G3D>;
>>>>>>  +            assigned-clock-rates = <0>, <66700000>;
>>>>>>  +            assigned-clock-parents = <&clocks MOUT_MPLL>;
>>>>> 
>>>>>  What are these clocks for, and why are they reparented / 
>>>>> reclocked?
>>>>> 
>>>>>  Shouldn't they be passed to 'clocks' as well?
>>>>> 
>>>>>  -Paul
>>>>> 
>>>> 
>>>>  The G3D clock system can have multiple parents, and for stable 
>>>> operation
>>>>  it's recommended to use the MPLL clock as the parent (which in 
>>>> turn
>>>>  is actually a mux as well).  MOUT_G3D is simply the mux for 
>>>> CLK_G3D
>>>>  (SGX core clock), DOUT_G3D is the divider.  DOUT_G3D could 
>>>> equally be CLK_G3D
>>>>  (and probably should be, for readability) as CLK_G3D is simply 
>>>> the gate and
>>>>  DOUT_G3D is the divider for it.
>>> 
>>>  Good point, it should be CLK_G3D instead of DOUT.  Can you fix 
>>> this as
>>>  well?
>> 
>>  Yep, will do.  Nikolaus, I'll send you an updated patch to include.
>> 
> 
> How are assigned-clocks handled in the yaml DT schema?  When running 
> make dtbs_check,
> I end up with messages such as
> 
> arch/arm/boot/dts/s5pv210-aquila.dt.yaml: gpu@f3000000: 
> 'assigned-clock-parents', 'assigned-clock-rates', 'assigned-clocks' 
> do not match any of the regexes: 'pinctrl-[0-9]+'
> 
> Do they need to explicitly be listed as valid entries?

The assigned-* can also be moved inside the node of the clocks 
provider. I would say it makes more sense to have them there.

-Paul

Re: [PATCH v7 01/12] dt-bindings: add img,pvrsgx.yaml for Imagination GPUs

[H. Nikolaus Schaller]

Hi Paul,

> Am 26.04.2020 um 15:11 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> Hi Nikolaus,
> 
> Le ven. 24 avril 2020 à 22:34, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>> The Imagination PVR/SGX GPU is part of several SoC from
>> multiple vendors, e.g. TI OMAP, Ingenic JZ4780, Intel Poulsbo,
>> Allwinner A83 and others.
>> With this binding, we describe how the SGX processor is
>> interfaced to the SoC (registers and interrupt).
>> The interface also consists of clocks, reset, power but
>> information from data sheets is vague and some SoC integrators
>> (TI) deciced to use a PRCM wrapper (ti,sysc) which does
>> all clock, reset and power-management through registers
>> outside of the sgx register block.
>> Therefore all these properties are optional.
>> Tested by make dt_binding_check
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> .../devicetree/bindings/gpu/img,pvrsgx.yaml   | 150 ++++++++++++++++++
>> 1 file changed, 150 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..33a9c4c6e784
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml
>> @@ -0,0 +1,150 @@
>> +# 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 extensive list see: https://en.wikipedia.org/wiki/PowerVR#Implementations
>> +
>> +  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. If not, an optional clock can be specified here.
>> +
>> +properties:
>> +  $nodename:
>> +    pattern: '^gpu@[a-f0-9]+$'
>> +  compatible:
>> +    oneOf:
>> +      - description: SGX530-121 based SoC
>> +        items:
>> +          - enum:
>> +            - ti,omap3-sgx530-121 # BeagleBoard A/B/C, OpenPandora 600MHz and similar
>> +          - const: img,sgx530-121
>> +          - const: img,sgx530
>> +
>> +      - description: SGX530-125 based SoC
>> +        items:
>> +          - enum:
>> +            - ti,am3352-sgx530-125 # BeagleBone Black
>> +            - ti,am3517-sgx530-125
>> +            - ti,am4-sgx530-125
>> +            - ti,omap3-sgx530-125 # BeagleBoard XM, GTA04, OpenPandora 1GHz and similar
>> +            - ti,ti81xx-sgx530-125
>> +          - const: ti,omap3-sgx530-125
>> +          - const: img,sgx530-125
>> +          - const: img,sgx530
>> +
>> +      - description: SGX535-116 based SoC
>> +        items:
>> +          - const: intel,poulsbo-gma500-sgx535 # Atom Z5xx
>> +          - const: img,sgx535-116
>> +          - const: img,sgx535
>> +
>> +      - description: SGX540-116 based SoC
>> +        items:
>> +          - const: intel,medfield-gma-sgx540 # Atom Z24xx
>> +          - const: img,sgx540-116
>> +          - const: img,sgx540
>> +
>> +      - description: SGX540-120 based SoC
>> +        items:
>> +          - enum:
>> +            - samsung,s5pv210-sgx540-120
>> +            - ti,omap4-sgx540-120 # Pandaboard, Pandaboard ES and similar
>> +          - const: img,sgx540-120
>> +          - const: img,sgx540
>> +
>> +      - description: SGX540-130 based SoC
>> +        items:
>> +          - enum:
>> +            - ingenic,jz4780-sgx540-130 # CI20
>> +          - const: img,sgx540-130
>> +          - const: img,sgx540
>> +
>> +      - description: SGX544-112 based SoC
>> +        items:
>> +          - const: ti,omap4470-sgx544-112
>> +          - const: img,sgx544-112
>> +          - const: img,sgx544
>> +
>> +      - description: SGX544-115 based SoC
>> +        items:
>> +          - enum:
>> +            - allwinner,sun8i-a31-sgx544-115
>> +            - allwinner,sun8i-a31s-sgx544-115
>> +            - allwinner,sun8i-a83t-sgx544-115 # Banana-Pi-M3 (Allwinner A83T) and similar
>> +          - const: img,sgx544-115
>> +          - const: img,sgx544
>> +
>> +      - description: SGX544-116 based SoC
>> +        items:
>> +          - enum:
>> +            - ti,dra7-sgx544-116 # DRA7
>> +            - ti,omap5-sgx544-116 # OMAP5 UEVM, Pyra Handheld and similar
>> +          - const: img,sgx544-116
>> +          - const: img,sgx544
>> +
>> +      - description: SGX545 based SoC
>> +        items:
>> +          - const: intel,cedarview-gma3600-sgx545 # Atom N2600, D2500
>> +          - const: img,sgx545-116
>> +          - const: img,sgx545
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  interrupt-names:
>> +    maxItems: 1
>> +    items:
>> +      - const: sgx
>> +
>> +  clocks:
>> +    maxItems: 4
>> +
>> +  clock-names:
>> +    maxItems: 4
>> +    items:
>> +      - const: core
>> +      - const: sys
>> +      - const: mem
>> +      - const: hyd
>> +
>> +  sgx-supply: true
>> +
>> +  power-domains:
>> +    maxItems: 1
>> +
>> +  resets:
>> +    maxItems: 1
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
> 
> By not making 'clocks' required you make it possible to create broken bindings; according to your schema, a GPU node without a 'clocks' for the JZ4780 would be perfectly valid.

Yes. But it will never pass a test with real hardware. So it can't be omitted anyways.

On a more general thought, this argument holds for any optional property. So it is not specific to clocks. Since the reg address values are also never specified you can still create broken bindings. Or by connecting the wrong clock. So the ways to create broken bindings are numerous.

I also assume that SGX integrators are not beginners and do you think they need to find out through a make dt_binding_check dtbs_check that they should define a clock? based on *assumptions* we do without having access to all systems?

IMHO the bindings documentation is a documentation. So it needs to be helpful but not perfect. Formalizing all corner cases in a bindings document (just because we can since .yaml was introduced) is IMHO overkill.

In times before the introduction of more formal .yaml I think we would not even have considered this for a comment in the bindings.txt.

> It's possible to forbid the presence of the 'clocks' property on some implementations, and require it on others.

To be precise we have to specify the exact number of clocks (between 0 and 4) for every architecture.

This also contradicts my dream to get rid of the architecture specific components in the long run. My dream (because I can't tell how it can be done) is that we can one day develop something which just needs compatible = img,530 or imp,540 or img,544. Then we can't make the number clocks depend on the implementation any more.

> See how it's done for instance on Documentation/devicetree/bindings/serial/samsung_uart.yaml.

Yes I know the design pattern, but I wonder if such a move makes the whole thing even less maintainable.

Assume we have finished DTS for some SoC. Then these DTS have been tested on real hardware and are working. Clocks are there where needed and missing where not. We may now forbid or not forbid them for some implementations in the bindings.yaml but the result of dtbs_check won't change! Because they are tested and working and the bindings.yaml has been adapted to the result. So we have just duplicated something for no practical benefit.

Next, assume there is coming support for more and more new SoC. Then, developers not only have to figure out which clocks they need in the DTS but they also have to add a patch to the implementation specific part of the bindings.yaml to clearly define exactly the same what they already have written into their .dts (the clocks are either there for the of_node or they are not). So again the rules are for no benefit, since a new SoC is introduced exactly once. And tested if it works. And if it is there, it will stay as it is. It is just work for maintainers to review that patch as well.

It boils down to the question if we need to formalize the rule how a working DTS was derived. Or just have a working DTS and not formalize everything.

So IMHO carrying along such a detail (forbid clocks on some architectures) is nice to have (and fun to learn the .yaml thing) but not of benefit for anyone. Not for the DTS developer nor for the maintainers nor for the users of a Linux kernel. "Keep it simple" is always a good rule for maintainability.

In summary I don't see a good reason to follow this in v8. But you could add it by a separate patch later if the DTS have been reviewed and agreed.

BR and thanks,
Nikolaus

Re: [PATCH v7 01/12] dt-bindings: add img,pvrsgx.yaml for Imagination GPUs

[Paul Cercueil]

Hi Nikolaus,

Le sam. 2 mai 2020 à 22:26, H. Nikolaus Schaller <hns@goldelico.com> a 
écrit :
> Hi Paul,
> 
>>  Am 26.04.2020 um 15:11 schrieb Paul Cercueil <paul@crapouillou.net>:
>> 
>>  Hi Nikolaus,
>> 
>>  Le ven. 24 avril 2020 à 22:34, H. Nikolaus Schaller 
>> <hns@goldelico.com> a écrit :
>>>  The Imagination PVR/SGX GPU is part of several SoC from
>>>  multiple vendors, e.g. TI OMAP, Ingenic JZ4780, Intel Poulsbo,
>>>  Allwinner A83 and others.
>>>  With this binding, we describe how the SGX processor is
>>>  interfaced to the SoC (registers and interrupt).
>>>  The interface also consists of clocks, reset, power but
>>>  information from data sheets is vague and some SoC integrators
>>>  (TI) deciced to use a PRCM wrapper (ti,sysc) which does
>>>  all clock, reset and power-management through registers
>>>  outside of the sgx register block.
>>>  Therefore all these properties are optional.
>>>  Tested by make dt_binding_check
>>>  Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>>  ---
>>>  .../devicetree/bindings/gpu/img,pvrsgx.yaml   | 150 
>>> ++++++++++++++++++
>>>  1 file changed, 150 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..33a9c4c6e784
>>>  --- /dev/null
>>>  +++ b/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml
>>>  @@ -0,0 +1,150 @@
>>>  +# 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 extensive list see: 
>>> https://en.wikipedia.org/wiki/PowerVR#Implementations
>>>  +
>>>  +  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. If not, an optional clock can be specified here.
>>>  +
>>>  +properties:
>>>  +  $nodename:
>>>  +    pattern: '^gpu@[a-f0-9]+$'
>>>  +  compatible:
>>>  +    oneOf:
>>>  +      - description: SGX530-121 based SoC
>>>  +        items:
>>>  +          - enum:
>>>  +            - ti,omap3-sgx530-121 # BeagleBoard A/B/C, 
>>> OpenPandora 600MHz and similar
>>>  +          - const: img,sgx530-121
>>>  +          - const: img,sgx530
>>>  +
>>>  +      - description: SGX530-125 based SoC
>>>  +        items:
>>>  +          - enum:
>>>  +            - ti,am3352-sgx530-125 # BeagleBone Black
>>>  +            - ti,am3517-sgx530-125
>>>  +            - ti,am4-sgx530-125
>>>  +            - ti,omap3-sgx530-125 # BeagleBoard XM, GTA04, 
>>> OpenPandora 1GHz and similar
>>>  +            - ti,ti81xx-sgx530-125
>>>  +          - const: ti,omap3-sgx530-125
>>>  +          - const: img,sgx530-125
>>>  +          - const: img,sgx530
>>>  +
>>>  +      - description: SGX535-116 based SoC
>>>  +        items:
>>>  +          - const: intel,poulsbo-gma500-sgx535 # Atom Z5xx
>>>  +          - const: img,sgx535-116
>>>  +          - const: img,sgx535
>>>  +
>>>  +      - description: SGX540-116 based SoC
>>>  +        items:
>>>  +          - const: intel,medfield-gma-sgx540 # Atom Z24xx
>>>  +          - const: img,sgx540-116
>>>  +          - const: img,sgx540
>>>  +
>>>  +      - description: SGX540-120 based SoC
>>>  +        items:
>>>  +          - enum:
>>>  +            - samsung,s5pv210-sgx540-120
>>>  +            - ti,omap4-sgx540-120 # Pandaboard, Pandaboard ES and 
>>> similar
>>>  +          - const: img,sgx540-120
>>>  +          - const: img,sgx540
>>>  +
>>>  +      - description: SGX540-130 based SoC
>>>  +        items:
>>>  +          - enum:
>>>  +            - ingenic,jz4780-sgx540-130 # CI20
>>>  +          - const: img,sgx540-130
>>>  +          - const: img,sgx540
>>>  +
>>>  +      - description: SGX544-112 based SoC
>>>  +        items:
>>>  +          - const: ti,omap4470-sgx544-112
>>>  +          - const: img,sgx544-112
>>>  +          - const: img,sgx544
>>>  +
>>>  +      - description: SGX544-115 based SoC
>>>  +        items:
>>>  +          - enum:
>>>  +            - allwinner,sun8i-a31-sgx544-115
>>>  +            - allwinner,sun8i-a31s-sgx544-115
>>>  +            - allwinner,sun8i-a83t-sgx544-115 # Banana-Pi-M3 
>>> (Allwinner A83T) and similar
>>>  +          - const: img,sgx544-115
>>>  +          - const: img,sgx544
>>>  +
>>>  +      - description: SGX544-116 based SoC
>>>  +        items:
>>>  +          - enum:
>>>  +            - ti,dra7-sgx544-116 # DRA7
>>>  +            - ti,omap5-sgx544-116 # OMAP5 UEVM, Pyra Handheld and 
>>> similar
>>>  +          - const: img,sgx544-116
>>>  +          - const: img,sgx544
>>>  +
>>>  +      - description: SGX545 based SoC
>>>  +        items:
>>>  +          - const: intel,cedarview-gma3600-sgx545 # Atom N2600, 
>>> D2500
>>>  +          - const: img,sgx545-116
>>>  +          - const: img,sgx545
>>>  +
>>>  +  reg:
>>>  +    maxItems: 1
>>>  +
>>>  +  interrupts:
>>>  +    maxItems: 1
>>>  +
>>>  +  interrupt-names:
>>>  +    maxItems: 1
>>>  +    items:
>>>  +      - const: sgx
>>>  +
>>>  +  clocks:
>>>  +    maxItems: 4
>>>  +
>>>  +  clock-names:
>>>  +    maxItems: 4
>>>  +    items:
>>>  +      - const: core
>>>  +      - const: sys
>>>  +      - const: mem
>>>  +      - const: hyd
>>>  +
>>>  +  sgx-supply: true
>>>  +
>>>  +  power-domains:
>>>  +    maxItems: 1
>>>  +
>>>  +  resets:
>>>  +    maxItems: 1
>>>  +
>>>  +required:
>>>  +  - compatible
>>>  +  - reg
>>>  +  - interrupts
>> 
>>  By not making 'clocks' required you make it possible to create 
>> broken bindings; according to your schema, a GPU node without a 
>> 'clocks' for the JZ4780 would be perfectly valid.
> 
> Yes. But it will never pass a test with real hardware. So it can't be 
> omitted anyways.
> 
> On a more general thought, this argument holds for any optional 
> property. So it is not specific to clocks. Since the reg address 
> values are also never specified you can still create broken bindings. 
> Or by connecting the wrong clock. So the ways to create broken 
> bindings are numerous.
> 
> I also assume that SGX integrators are not beginners and do you think 
> they need to find out through a make dt_binding_check dtbs_check that 
> they should define a clock? based on *assumptions* we do without 
> having access to all systems?
> 
> IMHO the bindings documentation is a documentation. So it needs to be 
> helpful but not perfect. Formalizing all corner cases in a bindings 
> document (just because we can since .yaml was introduced) is IMHO 
> overkill.
> 
> In times before the introduction of more formal .yaml I think we 
> would not even have considered this for a comment in the bindings.txt.
> 
>>  It's possible to forbid the presence of the 'clocks' property on 
>> some implementations, and require it on others.
> 
> To be precise we have to specify the exact number of clocks (between 
> 0 and 4) for every architecture.
> 
> This also contradicts my dream to get rid of the architecture 
> specific components in the long run. My dream (because I can't tell 
> how it can be done) is that we can one day develop something which 
> just needs compatible = img,530 or imp,540 or img,544. Then we can't 
> make the number clocks depend on the implementation any more.

As we said before, the number of clocks is a property of the GPU and 
*not* its integration into the SoC.

So you would *not* have a number of clocks between 0 and 4. You get 
either 0, or 4, depending on whether or not you have a wrapper.


>>  See how it's done for instance on 
>> Documentation/devicetree/bindings/serial/samsung_uart.yaml.
> 
> Yes I know the design pattern, but I wonder if such a move makes the 
> whole thing even less maintainable.
> 
> Assume we have finished DTS for some SoC. Then these DTS have been 
> tested on real hardware and are working. Clocks are there where 
> needed and missing where not. We may now forbid or not forbid them 
> for some implementations in the bindings.yaml but the result of 
> dtbs_check won't change! Because they are tested and working and the 
> bindings.yaml has been adapted to the result. So we have just 
> duplicated something for no practical benefit.
> 
> Next, assume there is coming support for more and more new SoC. Then, 
> developers not only have to figure out which clocks they need in the 
> DTS but they also have to add a patch to the implementation specific 
> part of the bindings.yaml to clearly define exactly the same what 
> they already have written into their .dts (the clocks are either 
> there for the of_node or they are not). So again the rules are for no 
> benefit, since a new SoC is introduced exactly once. And tested if it 
> works. And if it is there, it will stay as it is. It is just work for 
> maintainers to review that patch as well.

If you add support for a new SoC, you'd still need to modify the 
binding to add the compatible string. So the argument of "more work" is 
moot.

-Paul

> It boils down to the question if we need to formalize the rule how a 
> working DTS was derived. Or just have a working DTS and not formalize 
> everything.
> 
> So IMHO carrying along such a detail (forbid clocks on some 
> architectures) is nice to have (and fun to learn the .yaml thing) but 
> not of benefit for anyone. Not for the DTS developer nor for the 
> maintainers nor for the users of a Linux kernel. "Keep it simple" is 
> always a good rule for maintainability.
> 
> In summary I don't see a good reason to follow this in v8. But you 
> could add it by a separate patch later if the DTS have been reviewed 
> and agreed.
> 
> BR and thanks,
> Nikolaus
> 

Re: [PATCH v7 01/12] dt-bindings: add img,pvrsgx.yaml for Imagination GPUs

[H. Nikolaus Schaller]

Hi Paul,

> Am 03.05.2020 um 14:52 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
>>> It's possible to forbid the presence of the 'clocks' property on some implementations, and require it on others.
>> To be precise we have to specify the exact number of clocks (between 0 and 4) for every architecture.
>> This also contradicts my dream to get rid of the architecture specific components in the long run. My dream (because I can't tell how it can be done) is that we can one day develop something which just needs compatible = img,530 or imp,540 or img,544. Then we can't make the number clocks depend on the implementation any more.
> 
> As we said before, the number of clocks is a property of the GPU and *not* its integration into the SoC.

Well, it is a not very well documented property of the GPU. We have no data sheet of the standalone GPU. Only several SoC data sheets which give some indications.

It appears as if some sgx5xx versions have 3 clocks and some have 4. So you are right, the number of clocks depends on the sgx5xx version and that could be made dependent in the bindings (if necessary).

> 
> So you would *not* have a number of clocks between 0 and 4. You get either 0, or 4, depending on whether or not you have a wrapper.

I think this is contradicting your previous sentence. If the number of clocks is a property of the GPU and not the integration it must also not depend on whether there is a wrapper. I.e. it must be a constant for any type of integration.

The really correct variant would be to always make the SoC integration (wrapper) a separate subsystem (because it is never part of the SGX core but some interface bus) and clock provider and connect it explicitly to the clock inputs.

To be clear: I am not at all against describing the clocks. I just doubt that the time we invest into discussing on this level of detail and adding conditional clock requirements is worth the result. IMHO the bindings and .dts do not become better by describing them in more detail than just "optional". It just takes our time from contributing to other subsystems.

> 
> 
>>> See how it's done for instance on Documentation/devicetree/bindings/serial/samsung_uart.yaml.
>> Yes I know the design pattern, but I wonder if such a move makes the whole thing even less maintainable.
>> Assume we have finished DTS for some SoC. Then these DTS have been tested on real hardware and are working. Clocks are there where needed and missing where not. We may now forbid or not forbid them for some implementations in the bindings.yaml but the result of dtbs_check won't change! Because they are tested and working and the bindings.yaml has been adapted to the result. So we have just duplicated something for no practical benefit.
>> Next, assume there is coming support for more and more new SoC. Then, developers not only have to figure out which clocks they need in the DTS but they also have to add a patch to the implementation specific part of the bindings.yaml to clearly define exactly the same what they already have written into their .dts (the clocks are either there for the of_node or they are not). So again the rules are for no benefit, since a new SoC is introduced exactly once. And tested if it works. And if it is there, it will stay as it is. It is just work for maintainers to review that patch as well.
> 
> If you add support for a new SoC, you'd still need to modify the binding to add the compatible string. So the argument of "more work" is moot.

Agreed, I forgot this aspect. Nevertheless, it is easier to review a new compatible string than a new clock number rule (question: how do you practically review this? By looking if it does match the DTS?).

We have to add the compatible string as long as we need to have the SoC name in the compatible string (which as said is my dream to get rid of in far future). If we could get rid of it, there won't be a change any more. By just taking "img,sgx544" into a new SoC. The change would be moved into SoC specific wrappers. In such an ideal world, we would explicitly describe the wrappers as separate DT nodes. Even if they have no explicit driver (e.g. by some simple-pm-bus).

                   PRCM,bus,
Processor <<---->> Wrapper <<----->> SGX
ti,...             ti,sysc           img,sgx530
img,...            simple-bus        img,sgx540
samsung,...        ...               img,sgx544
other,             other,gpu-wrapper img,...

But this IMHO correct proposal was already rejected.

So at the moment we are circling around several proposals because none can fulfill all requirements.

Therefore my attempt to solve the gordian knot is to make clocks generally optional. This keeps the bindings simple but not generally wrong. And since the DTS are not only tested against bindings.yaml but on real hardware, the omission to enforce a specific number of clocks doesn't harm anyone. As said it is impossible to get the SGX running without defining the correct clocks (whether they are enforced by bindings.yaml or not).

BR and thanks,
Nikolaus

Re: [PATCH v7 01/12] dt-bindings: add img,pvrsgx.yaml for Imagination GPUs

[Paul Cercueil]

Le dim. 3 mai 2020 à 15:31, H. Nikolaus Schaller <hns@goldelico.com> a 
écrit :
> Hi Paul,
> 
>>  Am 03.05.2020 um 14:52 schrieb Paul Cercueil <paul@crapouillou.net>:
>> 
>>>>  It's possible to forbid the presence of the 'clocks' property on 
>>>> some implementations, and require it on others.
>>>  To be precise we have to specify the exact number of clocks 
>>> (between 0 and 4) for every architecture.
>>>  This also contradicts my dream to get rid of the architecture 
>>> specific components in the long run. My dream (because I can't tell 
>>> how it can be done) is that we can one day develop something which 
>>> just needs compatible = img,530 or imp,540 or img,544. Then we 
>>> can't make the number clocks depend on the implementation any more.
>> 
>>  As we said before, the number of clocks is a property of the GPU 
>> and *not* its integration into the SoC.
> 
> Well, it is a not very well documented property of the GPU. We have 
> no data sheet of the standalone GPU. Only several SoC data sheets 
> which give some indications.

Maybe we can nicely ask them?

I expect Paul Burton to have some contacts at ImgTec. Asking for a doc 
would be too much, but maybe they can help a bit with the DT bindings.

> It appears as if some sgx5xx versions have 3 clocks and some have 4. 
> So you are right, the number of clocks depends on the sgx5xx version 
> and that could be made dependent in the bindings (if necessary).
> 
>> 
>>  So you would *not* have a number of clocks between 0 and 4. You get 
>> either 0, or 4, depending on whether or not you have a wrapper.
> 
> I think this is contradicting your previous sentence. If the number 
> of clocks is a property of the GPU and not the integration it must 
> also not depend on whether there is a wrapper. I.e. it must be a 
> constant for any type of integration.

Well, I expected all SGX versions to have 4 clocks.

If some SGX versions have 3 clocks, and others have 4 clocks, it's 
still OK as long as the number of clocks is enforced, so that all 
implementations of a given SGX core will have to use the same number of 
clocks.

> The really correct variant would be to always make the SoC 
> integration (wrapper) a separate subsystem (because it is never part 
> of the SGX core but some interface bus) and clock provider and 
> connect it explicitly to the clock inputs.

About the wrapper... I don't really know how it's done there. But you 
could very well pass all clocks unconditionally to the SGX node, even 
if it's inside a wrapper.
The wrapper itself probably needs only one clock, the one that allows 
it to access its registers.

> To be clear: I am not at all against describing the clocks. I just 
> doubt that the time we invest into discussing on this level of detail 
> and adding conditional clock requirements is worth the result. IMHO 
> the bindings and .dts do not become better by describing them in more 
> detail than just "optional". It just takes our time from contributing 
> to other subsystems.
> 

You have a new SoC with a SGX, and you only need to enable one clock to 
get it to work. So you create a devicetree node which receives only one 
clock.

Turns out, that the bootloader was enabling the other 3 clocks, and 
since the last release, it doesn't anymore. You're left with having to 
support a broken devicetree.

That's the kind of problem that can be easily avoided by enforcing the 
number of clocks that have to be provided.
>> 
>> 
>>>>  See how it's done for instance on 
>>>> Documentation/devicetree/bindings/serial/samsung_uart.yaml.
>>>  Yes I know the design pattern, but I wonder if such a move makes 
>>> the whole thing even less maintainable.
>>>  Assume we have finished DTS for some SoC. Then these DTS have been 
>>> tested on real hardware and are working. Clocks are there where 
>>> needed and missing where not. We may now forbid or not forbid them 
>>> for some implementations in the bindings.yaml but the result of 
>>> dtbs_check won't change! Because they are tested and working and 
>>> the bindings.yaml has been adapted to the result. So we have just 
>>> duplicated something for no practical benefit.
>>>  Next, assume there is coming support for more and more new SoC. 
>>> Then, developers not only have to figure out which clocks they need 
>>> in the DTS but they also have to add a patch to the implementation 
>>> specific part of the bindings.yaml to clearly define exactly the 
>>> same what they already have written into their .dts (the clocks are 
>>> either there for the of_node or they are not). So again the rules 
>>> are for no benefit, since a new SoC is introduced exactly once. And 
>>> tested if it works. And if it is there, it will stay as it is. It 
>>> is just work for maintainers to review that patch as well.
>> 
>>  If you add support for a new SoC, you'd still need to modify the 
>> binding to add the compatible string. So the argument of "more work" 
>> is moot.
> 
> Agreed, I forgot this aspect. Nevertheless, it is easier to review a 
> new compatible string than a new clock number rule (question: how do 
> you practically review this? By looking if it does match the DTS?).
> 
> We have to add the compatible string as long as we need to have the 
> SoC name in the compatible string (which as said is my dream to get 
> rid of in far future). If we could get rid of it, there won't be a 
> change any more. By just taking "img,sgx544" into a new SoC. The 
> change would be moved into SoC specific wrappers. In such an ideal 
> world, we would explicitly describe the wrappers as separate DT 
> nodes. Even if they have no explicit driver (e.g. by some 
> simple-pm-bus).

What's wrong with having the SoC name in the compatible string?

You cannot use just a "img,sgx544" compatible string, as then you would 
assume that the same SGX version in (e.g.) an Ingenic or a Omap SoC is 
the exact same. This may actually be true. But the moment you discover 
even a tiny thing that needs to be handled differently, you wouldn't 
have the possibility to do so.

>                    PRCM,bus,
> Processor <<---->> Wrapper <<----->> SGX
> ti,...             ti,sysc           img,sgx530
> img,...            simple-bus        img,sgx540
> samsung,...        ...               img,sgx544
> other,             other,gpu-wrapper img,...
> 
> But this IMHO correct proposal was already rejected.
> 
> So at the moment we are circling around several proposals because 
> none can fulfill all requirements.
> 
> Therefore my attempt to solve the gordian knot is to make clocks 
> generally optional. This keeps the bindings simple but not generally 
> wrong. And since the DTS are not only tested against bindings.yaml 
> but on real hardware, the omission to enforce a specific number of 
> clocks doesn't harm anyone. As said it is impossible to get the SGX 
> running without defining the correct clocks (whether they are 
> enforced by bindings.yaml or not).

That's what I tried to explain above. You'd be able to get the SGX to 
work without a single clock in devicetree. That doesn't mean it should 
be allowed.

Cheers,
-Paul

Re: [PATCH v7 01/12] dt-bindings: add img,pvrsgx.yaml for Imagination GPUs

[Tony Lindgren]

* Paul Cercueil <paul@crapouillou.net> [200503 14:19]:
> You have a new SoC with a SGX, and you only need to enable one clock to get
> it to work. So you create a devicetree node which receives only one clock.
> 
> Turns out, that the bootloader was enabling the other 3 clocks, and since
> the last release, it doesn't anymore. You're left with having to support a
> broken devicetree.
> 
> That's the kind of problem that can be easily avoided by enforcing the
> number of clocks that have to be provided.

The number of clocks depends on how it's wired for the SoC.

On omaps, there's are no controls for additinoal SGX clocks. Sure some
of the clocks may be routed to multple places internally by the wrapper
module. But we have no control over that.

If we wanted to specify just the "fck" clock on omaps, then we can
do it with something like this:

allOf:
  - if:
    properites:
      compatible:
        enum:
	  - "ti,omap4-sgx544-112"
	  - "ti,omap5-sgx544-116"
	  - "ti,dra7-sgx544-116"
    then:
      properties:
        clocks:
	  minItems: 1
	  maxItems: 1

        clock-names:
	  const: fck

    required:
      - clocks
      - clock-names

There's no need for the SGX driver to toggle the "fck" here, it's
all done by PM runtime alreaedy so we would be just tweaking
the usage count for it. But hey, showing the clock rate might
be nice. Or maybe we want to at some point scale it, so no problem
specifying it.

For omap3, we should then specify "fck" and "ick". On omap4 and
later, there's no separate control over the "ick".

Then for the other SoCs, you can specify whatever clocks you need
there.

Regards,

Tony

Re: [PATCH v7 01/12] dt-bindings: add img,pvrsgx.yaml for Imagination GPUs

[H. Nikolaus Schaller]

Hi Paul and Paul,

> Am 03.05.2020 um 16:18 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> 
> 
> Le dim. 3 mai 2020 à 15:31, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>> Hi Paul,
>>> Am 03.05.2020 um 14:52 schrieb Paul Cercueil <paul@crapouillou.net>:
>>>>> It's possible to forbid the presence of the 'clocks' property on some implementations, and require it on others.
>>>> To be precise we have to specify the exact number of clocks (between 0 and 4) for every architecture.
>>>> This also contradicts my dream to get rid of the architecture specific components in the long run. My dream (because I can't tell how it can be done) is that we can one day develop something which just needs compatible = img,530 or imp,540 or img,544. Then we can't make the number clocks depend on the implementation any more.
>>> As we said before, the number of clocks is a property of the GPU and *not* its integration into the SoC.
>> Well, it is a not very well documented property of the GPU. We have no data sheet of the standalone GPU. Only several SoC data sheets which give some indications.
> 
> Maybe we can nicely ask them?

There is some (old) answer here:

https://github.com/MIPS/CI20_linux/blob/ci20-v3.18/arch/mips/boot/dts/jz4780.dtsi#L63

> I expect Paul Burton to have some contacts at ImgTec. Asking for a doc would be too much, but maybe they can help a bit with the DT bindings.

Good idea! It is definitively worth to try. Therefore I have moved him from CC: to To:

> 
>> It appears as if some sgx5xx versions have 3 clocks and some have 4. So you are right, the number of clocks depends on the sgx5xx version and that could be made dependent in the bindings (if necessary).
>>> So you would *not* have a number of clocks between 0 and 4. You get either 0, or 4, depending on whether or not you have a wrapper.
>> I think this is contradicting your previous sentence. If the number of clocks is a property of the GPU and not the integration it must also not depend on whether there is a wrapper. I.e. it must be a constant for any type of integration.
> 
> Well, I expected all SGX versions to have 4 clocks.
> 
> If some SGX versions have 3 clocks, and others have 4 clocks, it's still OK as long as the number of clocks is enforced, so that all implementations of a given SGX core will have to use the same number of clocks.

> 
>> The really correct variant would be to always make the SoC integration (wrapper) a separate subsystem (because it is never part of the SGX core but some interface bus) and clock provider and connect it explicitly to the clock inputs.
> 
> About the wrapper... I don't really know how it's done there. But you could very well pass all clocks unconditionally to the SGX node, even if it's inside a wrapper.
> The wrapper itself probably needs only one clock, the one that allows it to access its registers.
> 
>> To be clear: I am not at all against describing the clocks. I just doubt that the time we invest into discussing on this level of detail and adding conditional clock requirements is worth the result. IMHO the bindings and .dts do not become better by describing them in more detail than just "optional". It just takes our time from contributing to other subsystems.
> 
> You have a new SoC with a SGX, and you only need to enable one clock to get it to work. So you create a devicetree node which receives only one clock.
> 
> Turns out, that the bootloader was enabling the other 3 clocks,

Does it? I haven't seen such boot loaders. Usually they bring up only the core and e.g. mmc to be able to boot.

> and since the last release, it doesn't anymore. You're left with having to support a broken devicetree.

> 
> That's the kind of problem that can be easily avoided by enforcing the number of clocks that have to be provided.
>>>>> See how it's done for instance on Documentation/devicetree/bindings/serial/samsung_uart.yaml.
>>>> Yes I know the design pattern, but I wonder if such a move makes the whole thing even less maintainable.
>>>> Assume we have finished DTS for some SoC. Then these DTS have been tested on real hardware and are working. Clocks are there where needed and missing where not. We may now forbid or not forbid them for some implementations in the bindings.yaml but the result of dtbs_check won't change! Because they are tested and working and the bindings.yaml has been adapted to the result. So we have just duplicated something for no practical benefit.
>>>> Next, assume there is coming support for more and more new SoC. Then, developers not only have to figure out which clocks they need in the DTS but they also have to add a patch to the implementation specific part of the bindings.yaml to clearly define exactly the same what they already have written into their .dts (the clocks are either there for the of_node or they are not). So again the rules are for no benefit, since a new SoC is introduced exactly once. And tested if it works. And if it is there, it will stay as it is. It is just work for maintainers to review that patch as well.
>>> If you add support for a new SoC, you'd still need to modify the binding to add the compatible string. So the argument of "more work" is moot.
>> Agreed, I forgot this aspect. Nevertheless, it is easier to review a new compatible string than a new clock number rule (question: how do you practically review this? By looking if it does match the DTS?).
>> We have to add the compatible string as long as we need to have the SoC name in the compatible string (which as said is my dream to get rid of in far future). If we could get rid of it, there won't be a change any more. By just taking "img,sgx544" into a new SoC. The change would be moved into SoC specific wrappers. In such an ideal world, we would explicitly describe the wrappers as separate DT nodes. Even if they have no explicit driver (e.g. by some simple-pm-bus).
> 
> What's wrong with having the SoC name in the compatible string?

I'd say it should be avoided if possible. But you give a good hint and a little research shows some examples having the SoC name in the compatible string: musb, dwc2, dwc3.

> 
> You cannot use just a "img,sgx544" compatible string, as then you would assume that the same SGX version in (e.g.) an Ingenic or a Omap SoC is the exact same. This may actually be true.

Yes. That is the assumption and I have not seen any hints for the opposite in the pvrsrvkm sources. They only differentiate the SoC integration (clocks, reset) but not in the SGX operation (memory mapping, communication with firmware) itself. So the differences could easily be factored out into a wrapper driver.

> But the moment you discover even a tiny thing that needs to be handled differently, you wouldn't have the possibility to do so.

You would still have the possibility. An SGX driver can instead of differentiating by its own compatible string table look for the wrapper or SoC compatible string to find out where the sgx is integrated to. It is just simpler to do if we have the combined soc+sgx versions. And at the moment we even compile separate kernel modules from the same source.

> 
>>                   PRCM,bus,
>> Processor <<---->> Wrapper <<----->> SGX
>> ti,...             ti,sysc           img,sgx530
>> img,...            simple-bus        img,sgx540
>> samsung,...        ...               img,sgx544
>> other,             other,gpu-wrapper img,...
>> But this IMHO correct proposal was already rejected.
>> So at the moment we are circling around several proposals because none can fulfill all requirements.
>> Therefore my attempt to solve the gordian knot is to make clocks generally optional. This keeps the bindings simple but not generally wrong. And since the DTS are not only tested against bindings.yaml but on real hardware, the omission to enforce a specific number of clocks doesn't harm anyone. As said it is impossible to get the SGX running without defining the correct clocks (whether they are enforced by bindings.yaml or not).
> 
> That's what I tried to explain above. You'd be able to get the SGX to work without a single clock in devicetree. That doesn't mean it should be allowed.
> 
> Cheers,
> -Paul

BR and thanks,
Nikolaus

Re: [PATCH v7 01/12] dt-bindings: add img,pvrsgx.yaml for Imagination GPUs

[Rob Herring]

On Fri, Apr 24, 2020 at 10:34:04PM +0200, 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,
> Allwinner A83 and others.
> 
> With this binding, we describe how the SGX processor is
> interfaced to the SoC (registers and interrupt).
> 
> The interface also consists of clocks, reset, power but
> information from data sheets is vague and some SoC integrators
> (TI) deciced to use a PRCM wrapper (ti,sysc) which does
> all clock, reset and power-management through registers
> outside of the sgx register block.
> 
> Therefore all these properties are optional.
> 
> Tested by make dt_binding_check
> 
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  .../devicetree/bindings/gpu/img,pvrsgx.yaml   | 150 ++++++++++++++++++
>  1 file changed, 150 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..33a9c4c6e784
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml
> @@ -0,0 +1,150 @@
> +# 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 extensive list see: https://en.wikipedia.org/wiki/PowerVR#Implementations
> +
> +  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. If not, an optional clock can be specified here.
> +
> +properties:
> +  $nodename:
> +    pattern: '^gpu@[a-f0-9]+$'
> +  compatible:
> +    oneOf:
> +      - description: SGX530-121 based SoC
> +        items:
> +          - enum:
> +            - ti,omap3-sgx530-121 # BeagleBoard A/B/C, OpenPandora 600MHz and similar

Should be indented 2 more here and elsewhere where you have a list 
under a list.

> +          - const: img,sgx530-121
> +          - const: img,sgx530
> +
> +      - description: SGX530-125 based SoC
> +        items:
> +          - enum:
> +            - ti,am3352-sgx530-125 # BeagleBone Black
> +            - ti,am3517-sgx530-125
> +            - ti,am4-sgx530-125
> +            - ti,omap3-sgx530-125 # BeagleBoard XM, GTA04, OpenPandora 1GHz and similar
> +            - ti,ti81xx-sgx530-125
> +          - const: ti,omap3-sgx530-125
> +          - const: img,sgx530-125
> +          - const: img,sgx530
> +
> +      - description: SGX535-116 based SoC
> +        items:
> +          - const: intel,poulsbo-gma500-sgx535 # Atom Z5xx
> +          - const: img,sgx535-116
> +          - const: img,sgx535
> +
> +      - description: SGX540-116 based SoC
> +        items:
> +          - const: intel,medfield-gma-sgx540 # Atom Z24xx
> +          - const: img,sgx540-116
> +          - const: img,sgx540
> +
> +      - description: SGX540-120 based SoC
> +        items:
> +          - enum:
> +            - samsung,s5pv210-sgx540-120
> +            - ti,omap4-sgx540-120 # Pandaboard, Pandaboard ES and similar
> +          - const: img,sgx540-120
> +          - const: img,sgx540
> +
> +      - description: SGX540-130 based SoC
> +        items:
> +          - enum:
> +            - ingenic,jz4780-sgx540-130 # CI20
> +          - const: img,sgx540-130
> +          - const: img,sgx540
> +
> +      - description: SGX544-112 based SoC
> +        items:
> +          - const: ti,omap4470-sgx544-112
> +          - const: img,sgx544-112
> +          - const: img,sgx544
> +
> +      - description: SGX544-115 based SoC
> +        items:
> +          - enum:
> +            - allwinner,sun8i-a31-sgx544-115
> +            - allwinner,sun8i-a31s-sgx544-115
> +            - allwinner,sun8i-a83t-sgx544-115 # Banana-Pi-M3 (Allwinner A83T) and similar
> +          - const: img,sgx544-115
> +          - const: img,sgx544
> +
> +      - description: SGX544-116 based SoC
> +        items:
> +          - enum:
> +            - ti,dra7-sgx544-116 # DRA7
> +            - ti,omap5-sgx544-116 # OMAP5 UEVM, Pyra Handheld and similar
> +          - const: img,sgx544-116
> +          - const: img,sgx544
> +
> +      - description: SGX545 based SoC
> +        items:
> +          - const: intel,cedarview-gma3600-sgx545 # Atom N2600, D2500
> +          - const: img,sgx545-116
> +          - const: img,sgx545
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  interrupt-names:
> +    maxItems: 1
> +    items:
> +      - const: sgx
> +
> +  clocks:
> +    maxItems: 4
> +
> +  clock-names:
> +    maxItems: 4
> +    items:
> +      - const: core
> +      - const: sys
> +      - const: mem
> +      - const: hyd
> +
> +  sgx-supply: true
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |+
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    gpu: gpu@fe00 {
> +      compatible = "ti,omap5-sgx544-116", "img,sgx544-116", "img,sgx544";
> +      reg = <0xfe00 0x200>;
> +      interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
> +    };
> +
> +...
> -- 
> 2.25.1
> 

Re: [PATCH v7 01/12] dt-bindings: add img,pvrsgx.yaml for Imagination GPUs

[H. Nikolaus Schaller]

Hi Paul & Paul,

> Am 03.05.2020 um 18:41 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> 
> Hi Paul and Paul,
> 
>> Am 03.05.2020 um 16:18 schrieb Paul Cercueil <paul@crapouillou.net>:
>> 
>> 
>> 
>> Le dim. 3 mai 2020 à 15:31, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>>> Hi Paul,
>>>> Am 03.05.2020 um 14:52 schrieb Paul Cercueil <paul@crapouillou.net>:
>>>>>> It's possible to forbid the presence of the 'clocks' property on some implementations, and require it on others.
>>>>> To be precise we have to specify the exact number of clocks (between 0 and 4) for every architecture.
>>>>> This also contradicts my dream to get rid of the architecture specific components in the long run. My dream (because I can't tell how it can be done) is that we can one day develop something which just needs compatible = img,530 or imp,540 or img,544. Then we can't make the number clocks depend on the implementation any more.
>>>> As we said before, the number of clocks is a property of the GPU and *not* its integration into the SoC.
>>> Well, it is a not very well documented property of the GPU. We have no data sheet of the standalone GPU. Only several SoC data sheets which give some indications.
>> 
>> Maybe we can nicely ask them?
> 
> There is some (old) answer here:
> 
> https://github.com/MIPS/CI20_linux/blob/ci20-v3.18/arch/mips/boot/dts/jz4780.dtsi#L63
> 
>> I expect Paul Burton to have some contacts at ImgTec. Asking for a doc would be too much, but maybe they can help a bit with the DT bindings.
> 
> Good idea! It is definitively worth to try. Therefore I have moved him from CC: to To:

Do we already have an idea if we can get into contact and get help from ImgTec for this topic or if we have to live with what we have?

BR and thanks,
Nikolaus

Re: [PATCH v7 01/12] dt-bindings: add img,pvrsgx.yaml for Imagination GPUs

[H. Nikolaus Schaller]

Hi Tony,

> Am 03.05.2020 um 17:01 schrieb Tony Lindgren <tony@atomide.com>:
> 
> * Paul Cercueil <paul@crapouillou.net> [200503 14:19]:
>> You have a new SoC with a SGX, and you only need to enable one clock to get
>> it to work. So you create a devicetree node which receives only one clock.
>> 
>> Turns out, that the bootloader was enabling the other 3 clocks, and since
>> the last release, it doesn't anymore. You're left with having to support a
>> broken devicetree.
>> 
>> That's the kind of problem that can be easily avoided by enforcing the
>> number of clocks that have to be provided.
> 
> The number of clocks depends on how it's wired for the SoC.
> 
> On omaps, there's are no controls for additinoal SGX clocks. Sure some
> of the clocks may be routed to multple places internally by the wrapper
> module. But we have no control over that.
> 
> If we wanted to specify just the "fck" clock on omaps, then we can
> do it with something like this:
> 
> allOf:
>  - if:
>    properites:
>      compatible:
>        enum:
> 	  - "ti,omap4-sgx544-112"
> 	  - "ti,omap5-sgx544-116"
> 	  - "ti,dra7-sgx544-116"
>    then:
>      properties:
>        clocks:
> 	  minItems: 1
> 	  maxItems: 1
> 
>        clock-names:
> 	  const: fck
> 
>    required:
>      - clocks
>      - clock-names

will add to v8 of this series as a separate patch on top of the
general one. This should make it easier to have a focussed discussion
and revert/bisect if something goes wrong.

BR and thanks,
Nikolaus

Re: [PATCH v7 01/12] dt-bindings: add img,pvrsgx.yaml for Imagination GPUs

[H. Nikolaus Schaller]

> Am 05.05.2020 um 17:53 schrieb Rob Herring <robh@kernel.org>:
> 
> On Fri, Apr 24, 2020 at 10:34:04PM +0200, 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,
>> Allwinner A83 and others.
>> 
>> With this binding, we describe how the SGX processor is
>> interfaced to the SoC (registers and interrupt).
>> 
>> The interface also consists of clocks, reset, power but
>> information from data sheets is vague and some SoC integrators
>> (TI) deciced to use a PRCM wrapper (ti,sysc) which does
>> all clock, reset and power-management through registers
>> outside of the sgx register block.
>> 
>> Therefore all these properties are optional.
>> 
>> Tested by make dt_binding_check
>> 
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> .../devicetree/bindings/gpu/img,pvrsgx.yaml   | 150 ++++++++++++++++++
>> 1 file changed, 150 insertions(+)
>> +    oneOf:
>> +      - description: SGX530-121 based SoC
>> +        items:
>> +          - enum:
>> +            - ti,omap3-sgx530-121 # BeagleBoard A/B/C, OpenPandora 600MHz and similar
> 
> Should be indented 2 more here and elsewhere where you have a list 
> under a list.

added for patch v8 series.

BR and thanks,
Nikolaus