[PATCH RFC v2 00/11] Device tree support for Imagination Series5 GPU

[PATCH RFC v2 00/11] Device tree support for Imagination Series5 GPU

[Andrew Davis]

Hello all,

I know this has been tried before[0], but given the recent upstreaming of
the Series6+ GPU bindings I figured it might be time to give the Series5
bindings another try.

While there is currently no mainline driver for these binding, there is an
open source out-of-tree kernel-side driver available[1]. Having a stable
and upstream binding for these devices allows us to describe this hardware
in device tree.

This is my vision for how these bindings should look, along with some
example uses in several SoC DT files. The compatible names have been
updated to match what was decided on for Series6+, but otherwise most
is the same as we have been using in our vendor tree for many years.

Thanks,
Andrew

Based on next-20240108.

[0]: https://lkml.org/lkml/2020/4/24/1222
[1]: https://github.com/openpvrsgx-devgroup

Changes for RFC v2:
 - Added patch to rename Rogue+ binding to img,powervr-rogue.yaml
 - Locked all property item counts
 - Removed nodename pattern check

Andrew Davis (11):
  dt-bindings: gpu: Rename img,powervr to img,powervr-rogue
  dt-bindings: gpu: Add PowerVR Series5 SGX GPUs
  ARM: dts: omap3: Add device tree entry for SGX GPU
  ARM: dts: omap4: Add device tree entry for SGX GPU
  ARM: dts: omap5: Add device tree entry for SGX GPU
  ARM: dts: AM33xx: Add device tree entry for SGX GPU
  ARM: dts: AM437x: Add device tree entry for SGX GPU
  ARM: dts: DRA7xx: Add device tree entry for SGX GPU
  arm64: dts: ti: k3-am654-main: Add device tree entry for SGX GPU
  ARM: dts: sun6i: Add device tree entry for SGX GPU
  MIPS: DTS: jz4780: Add device tree entry for SGX GPU

 ...mg,powervr.yaml => img,powervr-rogue.yaml} |   4 +-
 .../bindings/gpu/img,powervr-sgx.yaml         | 124 ++++++++++++++++++
 MAINTAINERS                                   |   3 +-
 arch/arm/boot/dts/allwinner/sun6i-a31.dtsi    |   9 ++
 arch/arm/boot/dts/ti/omap/am33xx.dtsi         |   9 +-
 arch/arm/boot/dts/ti/omap/am3517.dtsi         |  11 +-
 arch/arm/boot/dts/ti/omap/am4372.dtsi         |   6 +
 arch/arm/boot/dts/ti/omap/dra7.dtsi           |   9 +-
 arch/arm/boot/dts/ti/omap/omap34xx.dtsi       |  11 +-
 arch/arm/boot/dts/ti/omap/omap36xx.dtsi       |   9 +-
 arch/arm/boot/dts/ti/omap/omap4.dtsi          |   9 +-
 arch/arm/boot/dts/ti/omap/omap5.dtsi          |   9 +-
 arch/arm64/boot/dts/ti/k3-am65-main.dtsi      |   7 +
 arch/mips/boot/dts/ingenic/jz4780.dtsi        |  11 ++
 14 files changed, 201 insertions(+), 30 deletions(-)
 rename Documentation/devicetree/bindings/gpu/{img,powervr.yaml => img,powervr-rogue.yaml} (91%)
 create mode 100644 Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml

-- 
2.39.2

[PATCH RFC v2 01/11] dt-bindings: gpu: Rename img,powervr to img,powervr-rogue

[Andrew Davis]

Signed-off-by: Andrew Davis <afd@ti.com>
---
 .../bindings/gpu/{img,powervr.yaml => img,powervr-rogue.yaml} | 4 ++--
 MAINTAINERS                                                   | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)
 rename Documentation/devicetree/bindings/gpu/{img,powervr.yaml => img,powervr-rogue.yaml} (91%)

diff --git a/Documentation/devicetree/bindings/gpu/img,powervr.yaml b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
similarity index 91%
rename from Documentation/devicetree/bindings/gpu/img,powervr.yaml
rename to Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
index a13298f1a1827..03a8308b41ae7 100644
--- a/Documentation/devicetree/bindings/gpu/img,powervr.yaml
+++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
@@ -2,10 +2,10 @@
 # Copyright (c) 2023 Imagination Technologies Ltd.
 %YAML 1.2
 ---
-$id: http://devicetree.org/schemas/gpu/img,powervr.yaml#
+$id: http://devicetree.org/schemas/gpu/img,powervr-rogue.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: Imagination Technologies PowerVR and IMG GPU
+title: Imagination Technologies PowerVR Rogue and IMG GPUs
 
 maintainers:
   - Frank Binns <frank.binns@imgtec.com>
diff --git a/MAINTAINERS b/MAINTAINERS
index fa67e2624723f..5b205795da04e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10461,7 +10461,7 @@ M:	Donald Robson <donald.robson@imgtec.com>
 M:	Matt Coster <matt.coster@imgtec.com>
 S:	Supported
 T:	git git://anongit.freedesktop.org/drm/drm-misc
-F:	Documentation/devicetree/bindings/gpu/img,powervr.yaml
+F:	Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
 F:	Documentation/gpu/imagination/
 F:	drivers/gpu/drm/imagination/
 F:	include/uapi/drm/pvr_drm.h
-- 
2.39.2

[PATCH RFC v2 02/11] dt-bindings: gpu: Add PowerVR Series5 SGX GPUs

[Andrew Davis]

The Imagination PowerVR Series5 "SGX" GPU is part of several SoCs from
multiple vendors. Describe how the SGX GPU is integrated in these SoC,
including register space and interrupts. Clocks, reset, and power domain
information is SoC specific.

Signed-off-by: Andrew Davis <afd@ti.com>
---
 .../bindings/gpu/img,powervr-sgx.yaml         | 124 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 2 files changed, 125 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml

diff --git a/Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml b/Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml
new file mode 100644
index 0000000000000..bb821e1184de9
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml
@@ -0,0 +1,124 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (c) 2023 Imagination Technologies Ltd.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpu/img,powervr-sgx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Imagination Technologies PowerVR SGX GPUs
+
+maintainers:
+  - Frank Binns <frank.binns@imgtec.com>
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+          - enum:
+              - ti,omap3430-gpu # Rev 121
+              - ti,omap3630-gpu # Rev 125
+          - const: img,powervr-sgx530
+      - items:
+          - enum:
+              - ingenic,jz4780-gpu # Rev 130
+              - ti,omap4430-gpu # Rev 120
+          - const: img,powervr-sgx540
+      - items:
+          - enum:
+              - allwinner,sun6i-a31-gpu # MP2 Rev 115
+              - ti,omap4470-gpu # MP1 Rev 112
+              - ti,omap5432-gpu # MP2 Rev 105
+              - ti,am5728-gpu # MP2 Rev 116
+              - ti,am6548-gpu # MP1 Rev 117
+          - const: img,powervr-sgx544
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks: true
+
+  clock-names:
+    minItems: 1
+    items:
+      - const: core
+      - const: mem
+      - const: sys
+
+  power-domains:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+additionalProperties: false
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: ti,am6548-gpu
+    then:
+      required:
+        - power-domains
+    else:
+      properties:
+        power-domains: false
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - allwinner,sun6i-a31-gpu
+              - ingenic,jz4780-gpu
+    then:
+      allOf:
+        - if:
+            properties:
+              compatible:
+                contains:
+                  const: allwinner,sun6i-a31-gpu
+          then:
+            properties:
+              clocks:
+                minItems: 2
+                maxItems: 2
+              clock-names:
+                minItems: 2
+                maxItems: 2
+        - if:
+            properties:
+              compatible:
+                contains:
+                  const: ingenic,jz4780-gpu
+          then:
+            properties:
+              clocks:
+                maxItems: 1
+              clock-names:
+                maxItems: 1
+      required:
+        - clocks
+        - clock-names
+    else:
+      properties:
+        clocks: false
+        clock-names: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/soc/ti,sci_pm_domain.h>
+
+    gpu@7000000 {
+        compatible = "ti,am6548-gpu", "img,powervr-sgx544";
+        reg = <0x7000000 0x10000>;
+        interrupts = <GIC_SPI 162 IRQ_TYPE_LEVEL_HIGH>;
+        power-domains = <&k3_pds 65 TI_SCI_PD_EXCLUSIVE>;
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 5b205795da04e..00ba13e019fa6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10462,6 +10462,7 @@ M:	Matt Coster <matt.coster@imgtec.com>
 S:	Supported
 T:	git git://anongit.freedesktop.org/drm/drm-misc
 F:	Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
+F:	Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml
 F:	Documentation/gpu/imagination/
 F:	drivers/gpu/drm/imagination/
 F:	include/uapi/drm/pvr_drm.h
-- 
2.39.2

[PATCH RFC v2 03/11] ARM: dts: omap3: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entries to base OMAP3 dtsi files.

Signed-off-by: Andrew Davis <afd@ti.com>
---
 arch/arm/boot/dts/ti/omap/am3517.dtsi   | 11 ++++++-----
 arch/arm/boot/dts/ti/omap/omap34xx.dtsi | 11 ++++++-----
 arch/arm/boot/dts/ti/omap/omap36xx.dtsi |  9 +++++----
 3 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/arch/arm/boot/dts/ti/omap/am3517.dtsi b/arch/arm/boot/dts/ti/omap/am3517.dtsi
index 77e58e686fb17..19aad715dff70 100644
--- a/arch/arm/boot/dts/ti/omap/am3517.dtsi
+++ b/arch/arm/boot/dts/ti/omap/am3517.dtsi
@@ -162,12 +162,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@0 {
+				compatible = "ti,omap3430-gpu", "img,powervr-sgx530";
+				reg = <0x0 0x10000>; /* 64kB */
+				interrupts = <21>;
+			};
 		};
 	};
 };
diff --git a/arch/arm/boot/dts/ti/omap/omap34xx.dtsi b/arch/arm/boot/dts/ti/omap/omap34xx.dtsi
index fc7233ac183a8..acdd0ee34421d 100644
--- a/arch/arm/boot/dts/ti/omap/omap34xx.dtsi
+++ b/arch/arm/boot/dts/ti/omap/omap34xx.dtsi
@@ -164,12 +164,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@0 {
+				compatible = "ti,omap3430-gpu", "img,powervr-sgx530";
+				reg = <0x0 0x10000>; /* 64kB */
+				interrupts = <21>;
+			};
 		};
 	};
 
diff --git a/arch/arm/boot/dts/ti/omap/omap36xx.dtsi b/arch/arm/boot/dts/ti/omap/omap36xx.dtsi
index e6d8070c1bf88..c3d79ecd56e39 100644
--- a/arch/arm/boot/dts/ti/omap/omap36xx.dtsi
+++ b/arch/arm/boot/dts/ti/omap/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@0 {
+				compatible = "ti,omap3630-gpu", "img,powervr-sgx530";
+				reg = <0x0 0x2000000>; /* 32MB */
+				interrupts = <21>;
+			};
 		};
 	};
 
-- 
2.39.2

[PATCH RFC v2 04/11] ARM: dts: omap4: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entry to base OMAP4 dtsi file.

Signed-off-by: Andrew Davis <afd@ti.com>
---
 arch/arm/boot/dts/ti/omap/omap4.dtsi | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/ti/omap/omap4.dtsi b/arch/arm/boot/dts/ti/omap/omap4.dtsi
index 2bbff9032be3e..559b2bfe4ca7c 100644
--- a/arch/arm/boot/dts/ti/omap/omap4.dtsi
+++ b/arch/arm/boot/dts/ti/omap/omap4.dtsi
@@ -501,10 +501,11 @@ sgx_module: target-module@56000000 {
 			#size-cells = <1>;
 			ranges = <0 0x56000000 0x2000000>;
 
-			/*
-			 * Closed source PowerVR driver, no child device
-			 * binding or driver in mainline
-			 */
+			gpu@0 {
+				compatible = "ti,omap4430-gpu", "img,powervr-sgx540";
+				reg = <0x0 0x2000000>; /* 32MB */
+				interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
+			};
 		};
 
 		/*
-- 
2.39.2

[PATCH RFC v2 05/11] ARM: dts: omap5: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entry to base OMAP5 dtsi file.

Signed-off-by: Andrew Davis <afd@ti.com>
---
 arch/arm/boot/dts/ti/omap/omap5.dtsi | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/ti/omap/omap5.dtsi b/arch/arm/boot/dts/ti/omap/omap5.dtsi
index bac6fa8387936..6a66214ad0e2f 100644
--- a/arch/arm/boot/dts/ti/omap/omap5.dtsi
+++ b/arch/arm/boot/dts/ti/omap/omap5.dtsi
@@ -453,10 +453,11 @@ target-module@56000000 {
 			#size-cells = <1>;
 			ranges = <0 0x56000000 0x2000000>;
 
-			/*
-			 * Closed source PowerVR driver, no child device
-			 * binding or driver in mainline
-			 */
+			gpu@0 {
+				compatible = "ti,omap5432-gpu", "img,powervr-sgx544";
+				reg = <0x0 0x2000000>; /* 32MB */
+				interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
+			};
 		};
 
 		target-module@58000000 {
-- 
2.39.2

[PATCH RFC v2 06/11] ARM: dts: AM33xx: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entry to base AM33xx dtsi file.

Signed-off-by: Andrew Davis <afd@ti.com>
---
 arch/arm/boot/dts/ti/omap/am33xx.dtsi | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/ti/omap/am33xx.dtsi b/arch/arm/boot/dts/ti/omap/am33xx.dtsi
index 5b9e01a8aa5d5..989d5a6edeed9 100644
--- a/arch/arm/boot/dts/ti/omap/am33xx.dtsi
+++ b/arch/arm/boot/dts/ti/omap/am33xx.dtsi
@@ -640,10 +640,11 @@ target-module@56000000 {
 			#size-cells = <1>;
 			ranges = <0 0x56000000 0x1000000>;
 
-			/*
-			 * Closed source PowerVR driver, no child device
-			 * binding or driver in mainline
-			 */
+			gpu@0 {
+				compatible = "ti,omap3630-gpu", "img,powervr-sgx530";
+				reg = <0x0 0x10000>; /* 64kB */
+				interrupts = <37>;
+			};
 		};
 	};
 };
-- 
2.39.2

[PATCH RFC v2 07/11] ARM: dts: AM437x: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entry to base AM437x dtsi file.

Signed-off-by: Andrew Davis <afd@ti.com>
---
 arch/arm/boot/dts/ti/omap/am4372.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/ti/omap/am4372.dtsi b/arch/arm/boot/dts/ti/omap/am4372.dtsi
index 9d2c064534f7d..5fd1b380ece62 100644
--- a/arch/arm/boot/dts/ti/omap/am4372.dtsi
+++ b/arch/arm/boot/dts/ti/omap/am4372.dtsi
@@ -719,6 +719,12 @@ target-module@56000000 {
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges = <0 0x56000000 0x1000000>;
+
+			gpu@0 {
+				compatible = "ti,omap3630-gpu", "img,powervr-sgx530";
+				reg = <0x0 0x10000>; /* 64kB */
+				interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
+			};
 		};
 	};
 };
-- 
2.39.2

[PATCH RFC v2 08/11] ARM: dts: DRA7xx: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entry to base DRA7x dtsi file.

Signed-off-by: Andrew Davis <afd@ti.com>
---
 arch/arm/boot/dts/ti/omap/dra7.dtsi | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/ti/omap/dra7.dtsi b/arch/arm/boot/dts/ti/omap/dra7.dtsi
index 6509c742fb58c..8527643cb69a8 100644
--- a/arch/arm/boot/dts/ti/omap/dra7.dtsi
+++ b/arch/arm/boot/dts/ti/omap/dra7.dtsi
@@ -850,12 +850,19 @@ target-module@56000000 {
 					<SYSC_IDLE_SMART>;
 			ti,sysc-sidle = <SYSC_IDLE_FORCE>,
 					<SYSC_IDLE_NO>,
-					<SYSC_IDLE_SMART>;
+					<SYSC_IDLE_SMART>,
+					<SYSC_IDLE_SMART_WKUP>;
 			clocks = <&gpu_clkctrl DRA7_GPU_CLKCTRL 0>;
 			clock-names = "fck";
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges = <0 0x56000000 0x2000000>;
+
+			gpu@0 {
+				compatible = "ti,am5728-gpu", "img,powervr-sgx544";
+				reg = <0x0 0x10000>; /* 64kB */
+				interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
+			};
 		};
 
 		crossbar_mpu: crossbar@4a002a48 {
-- 
2.39.2

[PATCH RFC v2 09/11] arm64: dts: ti: k3-am654-main: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entry to base AM654 dtsi file.

Signed-off-by: Andrew Davis <afd@ti.com>
---
 arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
index fcea544656360..64b52c8dafc6c 100644
--- a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
@@ -1050,6 +1050,13 @@ dss_ports: ports {
 		};
 	};
 
+	gpu: gpu@7000000 {
+		compatible = "ti,am6548-gpu", "img,powervr-sgx544";
+		reg = <0x0 0x7000000 0x0 0x10000>;
+		interrupts = <GIC_SPI 162 IRQ_TYPE_LEVEL_HIGH>;
+		power-domains = <&k3_pds 65 TI_SCI_PD_EXCLUSIVE>;
+	};
+
 	ehrpwm0: pwm@3000000 {
 		compatible = "ti,am654-ehrpwm", "ti,am3352-ehrpwm";
 		#pwm-cells = <3>;
-- 
2.39.2

[PATCH RFC v2 10/11] ARM: dts: sun6i: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entry to base sun6i-a31 dtsi file.

Signed-off-by: Andrew Davis <afd@ti.com>
---
 arch/arm/boot/dts/allwinner/sun6i-a31.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/boot/dts/allwinner/sun6i-a31.dtsi b/arch/arm/boot/dts/allwinner/sun6i-a31.dtsi
index 5cce4918f84c9..e6998783b89aa 100644
--- a/arch/arm/boot/dts/allwinner/sun6i-a31.dtsi
+++ b/arch/arm/boot/dts/allwinner/sun6i-a31.dtsi
@@ -962,6 +962,15 @@ mdio: mdio {
 			};
 		};
 
+		gpu: gpu@1c40000 {
+			compatible = "allwinner,sun6i-a31-gpu", "img,powervr-sgx544";
+			reg = <0x01c40000 0x10000>;
+			interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_GPU_CORE>, <&ccu CLK_GPU_MEMORY>;
+			clock-names = "core", "mem";
+			status = "disabled";
+		};
+
 		crypto: crypto-engine@1c15000 {
 			compatible = "allwinner,sun6i-a31-crypto",
 				     "allwinner,sun4i-a10-crypto";
-- 
2.39.2

[PATCH RFC v2 11/11] MIPS: DTS: jz4780: Add device tree entry for SGX GPU

[Andrew Davis]

Add SGX GPU device entry to base jz4780 dtsi file.

Signed-off-by: Andrew Davis <afd@ti.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 18affff85ce38..5ea6833f5e872 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -460,6 +460,17 @@ hdmi: hdmi@10180000 {
 		status = "disabled";
 	};
 
+	gpu: gpu@13040000 {
+		compatible = "ingenic,jz4780-gpu", "img,powervr-sgx540";
+		reg = <0x13040000 0x4000>;
+
+		clocks = <&cgu JZ4780_CLK_GPU>;
+		clock-names = "core";
+
+		interrupt-parent = <&intc>;
+		interrupts = <63>;
+	};
+
 	lcdc0: lcdc0@13050000 {
 		compatible = "ingenic,jz4780-lcd";
 		reg = <0x13050000 0x1800>;
-- 
2.39.2

Re: [PATCH RFC v2 01/11] dt-bindings: gpu: Rename img, powervr to img, powervr-rogue

[Javier Martinez Canillas]

Andrew Davis <afd@ti.com> writes:

Hello Andrew,

> Signed-off-by: Andrew Davis <afd@ti.com>
> ---

I think this deserves a commit message with the rationale for the rename.

Because kept an eye to the previous version, I know the reason and agree
with the change. Also, if remember correctly this was suggested by Maxime?

After a adding a commit message and Suggested-by tag:

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH RFC v2 03/11] ARM: dts: omap3: Add device tree entry for SGX GPU

[Javier Martinez Canillas]

Andrew Davis <afd@ti.com> writes:

> Add SGX GPU device entries to base OMAP3 dtsi files.
>
> Signed-off-by: Andrew Davis <afd@ti.com>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH RFC v2 04/11] ARM: dts: omap4: Add device tree entry for SGX GPU

[Javier Martinez Canillas]

Andrew Davis <afd@ti.com> writes:

> Add SGX GPU device entry to base OMAP4 dtsi file.
>
> Signed-off-by: Andrew Davis <afd@ti.com>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH RFC v2 05/11] ARM: dts: omap5: Add device tree entry for SGX GPU

[Javier Martinez Canillas]

Andrew Davis <afd@ti.com> writes:

> Add SGX GPU device entry to base OMAP5 dtsi file.
>
> Signed-off-by: Andrew Davis <afd@ti.com>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH RFC v2 06/11] ARM: dts: AM33xx: Add device tree entry for SGX GPU

[Javier Martinez Canillas]

Andrew Davis <afd@ti.com> writes:

> Add SGX GPU device entry to base AM33xx dtsi file.
>
> Signed-off-by: Andrew Davis <afd@ti.com>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH RFC v2 07/11] ARM: dts: AM437x: Add device tree entry for SGX GPU

[Javier Martinez Canillas]

Andrew Davis <afd@ti.com> writes:

> Add SGX GPU device entry to base AM437x dtsi file.
>
> Signed-off-by: Andrew Davis <afd@ti.com>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH RFC v2 08/11] ARM: dts: DRA7xx: Add device tree entry for SGX GPU

[Javier Martinez Canillas]

Andrew Davis <afd@ti.com> writes:

> Add SGX GPU device entry to base DRA7x dtsi file.
>
> Signed-off-by: Andrew Davis <afd@ti.com>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH RFC v2 01/11] dt-bindings: gpu: Rename img,powervr to img,powervr-rogue

[Frank Binns]

Hi Andrew,

On Mon, 2024-01-08 at 12:32 -0600, Andrew Davis wrote:
> Signed-off-by: Andrew Davis <afd@ti.com>
> ---
>  .../bindings/gpu/{img,powervr.yaml => img,powervr-rogue.yaml} | 4 ++--
>  MAINTAINERS                                                   | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
>  rename Documentation/devicetree/bindings/gpu/{img,powervr.yaml => img,powervr-rogue.yaml} (91%)
> 
> diff --git a/Documentation/devicetree/bindings/gpu/img,powervr.yaml b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
> similarity index 91%
> rename from Documentation/devicetree/bindings/gpu/img,powervr.yaml
> rename to Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
> index a13298f1a1827..03a8308b41ae7 100644
> --- a/Documentation/devicetree/bindings/gpu/img,powervr.yaml
> +++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
> @@ -2,10 +2,10 @@
>  # Copyright (c) 2023 Imagination Technologies Ltd.
>  %YAML 1.2
>  ---
> -$id: http://devicetree.org/schemas/gpu/img,powervr.yaml#
> +$id: http://devicetree.org/schemas/gpu/img,powervr-rogue.yaml#
>  $schema: http://devicetree.org/meta-schemas/core.yaml#
>  
> -title: Imagination Technologies PowerVR and IMG GPU
> +title: Imagination Technologies PowerVR Rogue and IMG GPUs

All the GPUs that will appear in this file will be Rogues, so for me it would be
more natural for 'Rogue' to come after 'IMG'. Can you change the title to:

Imagination Technologies PowerVR and IMG Rogue GPUs
With that changed and Javier's suggestions addressed:
Reviewed-by: Frank Binns <frank.binns@imgtec.com>

>  
>  maintainers:
>    - Frank Binns <frank.binns@imgtec.com>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index fa67e2624723f..5b205795da04e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10461,7 +10461,7 @@ M:	Donald Robson <donald.robson@imgtec.com>
>  M:	Matt Coster <matt.coster@imgtec.com>
>  S:	Supported
>  T:	git git://anongit.freedesktop.org/drm/drm-misc
> -F:	Documentation/devicetree/bindings/gpu/img,powervr.yaml
> +F:	Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
>  F:	Documentation/gpu/imagination/
>  F:	drivers/gpu/drm/imagination/
>  F:	include/uapi/drm/pvr_drm.h

Re: [PATCH RFC v2 01/11] dt-bindings: gpu: Rename img,powervr to img,powervr-rogue

[Krzysztof Kozlowski]

On 08/01/2024 19:32, Andrew Davis wrote:
> Signed-off-by: Andrew Davis <afd@ti.com>
> ---
>  .../bindings/gpu/{img,powervr.yaml => img,powervr-rogue.yaml} | 4 ++--
>  MAINTAINERS                                                   | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)

If you are renaming it, why not renaming to match compatible as we
usually expect?

Best regards,
Krzysztof

Re: [PATCH RFC v2 02/11] dt-bindings: gpu: Add PowerVR Series5 SGX GPUs

[Krzysztof Kozlowski]

On 08/01/2024 19:32, Andrew Davis wrote:
> The Imagination PowerVR Series5 "SGX" GPU is part of several SoCs from
> multiple vendors. Describe how the SGX GPU is integrated in these SoC,
> including register space and interrupts. Clocks, reset, and power domain
> information is SoC specific.
> 
> Signed-off-by: Andrew Davis <afd@ti.com>
> ---
>  .../bindings/gpu/img,powervr-sgx.yaml         | 124 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  2 files changed, 125 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml
> 
> diff --git a/Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml b/Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml
> new file mode 100644
> index 0000000000000..bb821e1184de9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml
> @@ -0,0 +1,124 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (c) 2023 Imagination Technologies Ltd.

Your email has @TI domain, are you sure you attribute your copyrights to
Imagination?

...

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks: true

Missing min/maxItems

> +
> +  clock-names:
> +    minItems: 1
> +    items:
> +      - const: core
> +      - const: mem
> +      - const: sys
> +
> +  power-domains:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +additionalProperties: false

This goes after allOf: block.

> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: ti,am6548-gpu
> +    then:
> +      required:
> +        - power-domains
> +    else:
> +      properties:
> +        power-domains: false
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - allwinner,sun6i-a31-gpu
> +              - ingenic,jz4780-gpu
> +    then:
> +      allOf:
> +        - if:

I don't understand why do you need to embed allOf inside another allOf.
The upper (outer) if:then: looks entirely useless.

> +            properties:
> +              compatible:
> +                contains:
> +                  const: allwinner,sun6i-a31-gpu
> +          then:
> +            properties:
> +              clocks:
> +                minItems: 2
> +                maxItems: 2
> +              clock-names:
> +                minItems: 2
> +                maxItems: 2


Best regards,
Krzysztof

Re: [PATCH RFC v2 01/11] dt-bindings: gpu: Rename img,powervr to img,powervr-rogue

[Andrew Davis]

On 1/9/24 5:28 AM, Krzysztof Kozlowski wrote:
> On 08/01/2024 19:32, Andrew Davis wrote:
>> Signed-off-by: Andrew Davis <afd@ti.com>
>> ---
>>   .../bindings/gpu/{img,powervr.yaml => img,powervr-rogue.yaml} | 4 ++--
>>   MAINTAINERS                                                   | 2 +-
>>   2 files changed, 3 insertions(+), 3 deletions(-)
> 
> If you are renaming it, why not renaming to match compatible as we
> usually expect?
> 

There are (or will be) multiple compatible strings described in this
file, naming the file after just one would not fully convey the content
of the file. This generic style naming seems common already for bindings
with multiple compatibles.

Andrew

> Best regards,
> Krzysztof
> 

Re: [PATCH RFC v2 02/11] dt-bindings: gpu: Add PowerVR Series5 SGX GPUs

[Andrew Davis]

On 1/9/24 5:32 AM, Krzysztof Kozlowski wrote:
> On 08/01/2024 19:32, Andrew Davis wrote:
>> The Imagination PowerVR Series5 "SGX" GPU is part of several SoCs from
>> multiple vendors. Describe how the SGX GPU is integrated in these SoC,
>> including register space and interrupts. Clocks, reset, and power domain
>> information is SoC specific.
>>
>> Signed-off-by: Andrew Davis <afd@ti.com>
>> ---
>>   .../bindings/gpu/img,powervr-sgx.yaml         | 124 ++++++++++++++++++
>>   MAINTAINERS                                   |   1 +
>>   2 files changed, 125 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml b/Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml
>> new file mode 100644
>> index 0000000000000..bb821e1184de9
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml
>> @@ -0,0 +1,124 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +# Copyright (c) 2023 Imagination Technologies Ltd.
> 
> Your email has @TI domain, are you sure you attribute your copyrights to
> Imagination?
> 

The file started as a copy/paste from a IMG copyrighted file, even
though it is now almost completely re-written I've left their (c)
for good measure. I'll add an additional TI (c).

> ...
> 
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  clocks: true
> 
> Missing min/maxItems
> 

These are set in the allOf/if/then blocks below, seems
if I don't set them to at least something here then I get
a warning:

    'clock-names', 'clocks' do not match any of the regexes: 'pinctrl-[0-9]+'

even if I define them in the allOf block below. I don't
know what the min/max should be until I check the compatible
in the allOf block.

>> +
>> +  clock-names:
>> +    minItems: 1
>> +    items:
>> +      - const: core
>> +      - const: mem
>> +      - const: sys
>> +
>> +  power-domains:
>> +    maxItems: 1
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +
>> +additionalProperties: false
> 
> This goes after allOf: block.
> 

ACK

>> +
>> +allOf:
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const: ti,am6548-gpu
>> +    then:
>> +      required:
>> +        - power-domains
>> +    else:
>> +      properties:
>> +        power-domains: false
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - allwinner,sun6i-a31-gpu
>> +              - ingenic,jz4780-gpu
>> +    then:
>> +      allOf:
>> +        - if:
> 
> I don't understand why do you need to embed allOf inside another allOf.
> The upper (outer) if:then: looks entirely useless.
> 

It is so that both compatibles falls through to having
clock being required.

Logic in YAML always seems messy to me, here it is in pseudo C:

if (compatible == allwinner,sun6i-a31-gpu ||
     compatible == ingenic,jz4780-gpu) {
	if (compatible == allwinner,sun6i-a31-gpu)
		clocks: ...
	if (compatible == ingenic,jz4780-gpu)
		clocks: ...
	required:
		- clocks
		- clock-names
} else { /* disallow for all others */
	properties:
		clocks: false
		clock-names: false
}

Now if I had an "else if" that didn't force the indention to keep
growing I would have used that. (does one exist?) I also cannot
simply add the clock properties only for the two compats need
them for the reasons above and so must add them unconditionally
before then explicitly disable them in a catch-all else path.

Andrew

>> +            properties:
>> +              compatible:
>> +                contains:
>> +                  const: allwinner,sun6i-a31-gpu
>> +          then:
>> +            properties:
>> +              clocks:
>> +                minItems: 2
>> +                maxItems: 2
>> +              clock-names:
>> +                minItems: 2
>> +                maxItems: 2
> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH RFC v2 01/11] dt-bindings: gpu: Rename img,powervr to img,powervr-rogue

[Krzysztof Kozlowski]

On 09/01/2024 17:14, Andrew Davis wrote:
> On 1/9/24 5:28 AM, Krzysztof Kozlowski wrote:
>> On 08/01/2024 19:32, Andrew Davis wrote:
>>> Signed-off-by: Andrew Davis <afd@ti.com>
>>> ---
>>>   .../bindings/gpu/{img,powervr.yaml => img,powervr-rogue.yaml} | 4 ++--
>>>   MAINTAINERS                                                   | 2 +-
>>>   2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> If you are renaming it, why not renaming to match compatible as we
>> usually expect?
>>
> 
> There are (or will be) multiple compatible strings described in this
> file, naming the file after just one would not fully convey the content
> of the file. This generic style naming seems common already for bindings
> with multiple compatibles.

I saw only one compatible used as fallback. Where are more?

Best regards,
Krzysztof

Re: [PATCH RFC v2 02/11] dt-bindings: gpu: Add PowerVR Series5 SGX GPUs

[Krzysztof Kozlowski]

On 09/01/2024 17:53, Andrew Davis wrote:
> On 1/9/24 5:32 AM, Krzysztof Kozlowski wrote:
>> On 08/01/2024 19:32, Andrew Davis wrote:
>>> The Imagination PowerVR Series5 "SGX" GPU is part of several SoCs from
>>> multiple vendors. Describe how the SGX GPU is integrated in these SoC,
>>> including register space and interrupts. Clocks, reset, and power domain
>>> information is SoC specific.
>>>
>>> Signed-off-by: Andrew Davis <afd@ti.com>
>>> ---
>>>   .../bindings/gpu/img,powervr-sgx.yaml         | 124 ++++++++++++++++++
>>>   MAINTAINERS                                   |   1 +
>>>   2 files changed, 125 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml b/Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml
>>> new file mode 100644
>>> index 0000000000000..bb821e1184de9
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml
>>> @@ -0,0 +1,124 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +# Copyright (c) 2023 Imagination Technologies Ltd.
>>
>> Your email has @TI domain, are you sure you attribute your copyrights to
>> Imagination?
>>
> 
> The file started as a copy/paste from a IMG copyrighted file, even
> though it is now almost completely re-written I've left their (c)
> for good measure. I'll add an additional TI (c).
> 
>> ...
>>
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  interrupts:
>>> +    maxItems: 1
>>> +
>>> +  clocks: true
>>
>> Missing min/maxItems
>>
> 
> These are set in the allOf/if/then blocks below, seems

I know, but we expect them here.

https://elixir.bootlin.com/linux/v5.19-rc6/source/Documentation/devicetree/bindings/clock/samsung,exynos7-clock.yaml#L57

> if I don't set them to at least something here then I get
> a warning:
> 
>     'clock-names', 'clocks' do not match any of the regexes: 'pinctrl-[0-9]+'
> 
> even if I define them in the allOf block below. I don't
> know what the min/max should be until I check the compatible
> in the allOf block.

As always: the widest constraints.


...

> Logic in YAML always seems messy to me, here it is in pseudo C:
> 
> if (compatible == allwinner,sun6i-a31-gpu ||
>      compatible == ingenic,jz4780-gpu) {
> 	if (compatible == allwinner,sun6i-a31-gpu)
> 		clocks: ...
> 	if (compatible == ingenic,jz4780-gpu)
> 		clocks: ...
> 	required:
> 		- clocks
> 		- clock-names
> } else { /* disallow for all others */
> 	properties:
> 		clocks: false
> 		clock-names: false
> }

OK, I see, that's the limitation of YAML. The point is that this code is
not readable, so just list all fallbacks or variants.



Best regards,
Krzysztof

Re: [PATCH RFC v2 04/11] ARM: dts: omap4: Add device tree entry for SGX GPU

[H. Nikolaus Schaller]

Hi,
I just comment on this example, but it applies almost the same for all other .dtsi changes.

> Am 08.01.2024 um 19:32 schrieb Andrew Davis <afd@ti.com>:
> 
> Add SGX GPU device entry to base OMAP4 dtsi file.
> 
> Signed-off-by: Andrew Davis <afd@ti.com>
> ---
> arch/arm/boot/dts/ti/omap/omap4.dtsi | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/ti/omap/omap4.dtsi b/arch/arm/boot/dts/ti/omap/omap4.dtsi
> index 2bbff9032be3e..559b2bfe4ca7c 100644
> --- a/arch/arm/boot/dts/ti/omap/omap4.dtsi
> +++ b/arch/arm/boot/dts/ti/omap/omap4.dtsi
> @@ -501,10 +501,11 @@ sgx_module: target-module@56000000 {
> #size-cells = <1>;
> ranges = <0 0x56000000 0x2000000>;
> 
> - /*
> - * Closed source PowerVR driver, no child device
> - * binding or driver in mainline
> - */
> + gpu@0 {

I wonder why we don't add a "gpu:" label here.

Almost all other subsystem nodes have one (e.g. emif:, aes:, dss:, dsi:, hdmi:, etc.),
obviously for convenience when using a .dtsi file.

It would allow a board-specific DTS to easily add status = "disabled" to avoid driver
probing or disabling the GPU (e.g. if there is no display).

> + compatible = "ti,omap4430-gpu", "img,powervr-sgx540";

It still appears to me that the "img,powervr-sgx540" (or similar) entry is redundant
information.

I have experimentally updated our openpvrsgx driver and we do not have any use for
this information (at least in the kernel driver):

https://github.com/goldelico/letux-kernel/commit/f2f7cb3b858ef255f52f2b82a8bb34c047337afe

It shows how easy it is to derive the sgx version and revision number if we ever
need it inside the driver.

So if you want to keep a reference to powervr, it would suffice to have

> + compatible = "ti,omap4430-gpu", "img,powervr-sgx";

Otherwise your device tree entries compile fine and seem to work (at least in
a cursory test on PandaBoard ES).

> + reg = <0x0 0x2000000>; /* 32MB */
> + interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
> + };
> };

BR and thanks,
Nikolaus

Re: Re: [PATCH RFC v2 04/11] ARM: dts: omap4: Add device tree entry for SGX GPU

[Maxime Ripard]

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

Hi,

On Fri, Jan 12, 2024 at 06:33:58PM +0100, H. Nikolaus Schaller wrote:
> > Am 08.01.2024 um 19:32 schrieb Andrew Davis <afd@ti.com>:
> > 
> > Add SGX GPU device entry to base OMAP4 dtsi file.
> > 
> > Signed-off-by: Andrew Davis <afd@ti.com>
> > ---
> > arch/arm/boot/dts/ti/omap/omap4.dtsi | 9 +++++----
> > 1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/ti/omap/omap4.dtsi b/arch/arm/boot/dts/ti/omap/omap4.dtsi
> > index 2bbff9032be3e..559b2bfe4ca7c 100644
> > --- a/arch/arm/boot/dts/ti/omap/omap4.dtsi
> > +++ b/arch/arm/boot/dts/ti/omap/omap4.dtsi
> > @@ -501,10 +501,11 @@ sgx_module: target-module@56000000 {
> > #size-cells = <1>;
> > ranges = <0 0x56000000 0x2000000>;
> > 
> > - /*
> > - * Closed source PowerVR driver, no child device
> > - * binding or driver in mainline
> > - */
> > + gpu@0 {
> 
> I wonder why we don't add a "gpu:" label here.
> 
> Almost all other subsystem nodes have one (e.g. emif:, aes:, dss:, dsi:, hdmi:, etc.),
> obviously for convenience when using a .dtsi file.
> 
> It would allow a board-specific DTS to easily add status = "disabled" to avoid driver
> probing or disabling the GPU (e.g. if there is no display).

There's no reason to disable it in the DT: the hardware block would
still be there and it's rendering to memory so it still could be useful.

If there's no display on the board and you really don't want the GPU
driver, then you can disable the driver or block the module loading, but
it should be a distro / package / user decision, not a DT / kernel one
still.

Maxime

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

Re: [PATCH RFC v2 04/11] ARM: dts: omap4: Add device tree entry for SGX GPU

[H. Nikolaus Schaller]

Hi,

> Am 15.01.2024 um 09:25 schrieb Maxime Ripard <mripard@kernel.org>:
> 
> Hi,
> 
> On Fri, Jan 12, 2024 at 06:33:58PM +0100, H. Nikolaus Schaller wrote:
>>> Am 08.01.2024 um 19:32 schrieb Andrew Davis <afd@ti.com>:
>>> 
>>> Add SGX GPU device entry to base OMAP4 dtsi file.
>>> 
>>> Signed-off-by: Andrew Davis <afd@ti.com>
>>> ---
>>> arch/arm/boot/dts/ti/omap/omap4.dtsi | 9 +++++----
>>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/arch/arm/boot/dts/ti/omap/omap4.dtsi b/arch/arm/boot/dts/ti/omap/omap4.dtsi
>>> index 2bbff9032be3e..559b2bfe4ca7c 100644
>>> --- a/arch/arm/boot/dts/ti/omap/omap4.dtsi
>>> +++ b/arch/arm/boot/dts/ti/omap/omap4.dtsi
>>> @@ -501,10 +501,11 @@ sgx_module: target-module@56000000 {
>>> #size-cells = <1>;
>>> ranges = <0 0x56000000 0x2000000>;
>>> 
>>> - /*
>>> - * Closed source PowerVR driver, no child device
>>> - * binding or driver in mainline
>>> - */
>>> + gpu@0 {
>> 
>> I wonder why we don't add a "gpu:" label here.
>> 
>> Almost all other subsystem nodes have one (e.g. emif:, aes:, dss:, dsi:, hdmi:, etc.),
>> obviously for convenience when using a .dtsi file.
>> 
>> It would allow a board-specific DTS to easily add status = "disabled" to avoid driver
>> probing or disabling the GPU (e.g. if there is no display).
> 
> There's no reason to disable it in the DT: the hardware block would
> still be there and it's rendering to memory so it still could be useful.

Well, if you know that the board does not have a dm3730 but a dm3725 without
GPU it is better to disable the GPU completely instead of loading the driver
and make it detect by some internal bits that it has no GPU on the SoC.

> If there's no display on the board and you really don't want the GPU
> driver, then you can disable the driver or block the module loading, but
> it should be a distro / package / user decision, not a DT / kernel one
> still.

The same holds for aes: dss: dsi: hdmi: etc. If they are not used by some
board file, they don't change a single bit of the DTB [1] which IMHO would
be of reasonable concern to question additional labels.

BR and thanks,
Nikolaus

[1] https://devicetree-specification.readthedocs.io/en/stable/source-language.html
"Labels are only used in the devicetree source format and are not encoded into the
DTB binary."

Re: [PATCH RFC v2 04/11] ARM: dts: omap4: Add device tree entry for SGX GPU

[Andreas Kemnade]

Hi, 

On Mon, 15 Jan 2024 09:55:00 +0100
"H. Nikolaus Schaller" <hns@goldelico.com> wrote:

> > There's no reason to disable it in the DT: the hardware block would
> > still be there and it's rendering to memory so it still could be useful.  
> 
> Well, if you know that the board does not have a dm3730 but a dm3725 without
> GPU it is better to disable the GPU completely instead of loading the driver
> and make it detect by some internal bits that it has no GPU on the SoC.
> 
That is at least some valid reason.

> > If there's no display on the board and you really don't want the GPU
> > driver, then you can disable the driver or block the module loading, but
> > it should be a distro / package / user decision, not a DT / kernel one
> > still.  
> 
> The same holds for aes: dss: dsi: hdmi: etc. If they are not used by some
> board file, they don't change a single bit of the DTB [1] which IMHO would
> be of reasonable concern to question additional labels.

There is some difference here, some hardware can just not be used without
wired external pins, the gpu can be used even if no display is connected
either to accelerate some remote access or you could use the gpu for something
completely else...
Maybe mining bitcoins if temperate gets too low to warm you pocket ;-)

But as these labels do not harm, I have no strong opinion against it.

Regards,
Andreas

Re: Re: [PATCH RFC v2 04/11] ARM: dts: omap4: Add device tree entry for SGX GPU

[Maxime Ripard]

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

On Mon, Jan 15, 2024 at 09:55:00AM +0100, H. Nikolaus Schaller wrote:
> Hi,
> 
> > Am 15.01.2024 um 09:25 schrieb Maxime Ripard <mripard@kernel.org>:
> > 
> > Hi,
> > 
> > On Fri, Jan 12, 2024 at 06:33:58PM +0100, H. Nikolaus Schaller wrote:
> >>> Am 08.01.2024 um 19:32 schrieb Andrew Davis <afd@ti.com>:
> >>> 
> >>> Add SGX GPU device entry to base OMAP4 dtsi file.
> >>> 
> >>> Signed-off-by: Andrew Davis <afd@ti.com>
> >>> ---
> >>> arch/arm/boot/dts/ti/omap/omap4.dtsi | 9 +++++----
> >>> 1 file changed, 5 insertions(+), 4 deletions(-)
> >>> 
> >>> diff --git a/arch/arm/boot/dts/ti/omap/omap4.dtsi b/arch/arm/boot/dts/ti/omap/omap4.dtsi
> >>> index 2bbff9032be3e..559b2bfe4ca7c 100644
> >>> --- a/arch/arm/boot/dts/ti/omap/omap4.dtsi
> >>> +++ b/arch/arm/boot/dts/ti/omap/omap4.dtsi
> >>> @@ -501,10 +501,11 @@ sgx_module: target-module@56000000 {
> >>> #size-cells = <1>;
> >>> ranges = <0 0x56000000 0x2000000>;
> >>> 
> >>> - /*
> >>> - * Closed source PowerVR driver, no child device
> >>> - * binding or driver in mainline
> >>> - */
> >>> + gpu@0 {
> >> 
> >> I wonder why we don't add a "gpu:" label here.
> >> 
> >> Almost all other subsystem nodes have one (e.g. emif:, aes:, dss:, dsi:, hdmi:, etc.),
> >> obviously for convenience when using a .dtsi file.
> >> 
> >> It would allow a board-specific DTS to easily add status = "disabled" to avoid driver
> >> probing or disabling the GPU (e.g. if there is no display).
> > 
> > There's no reason to disable it in the DT: the hardware block would
> > still be there and it's rendering to memory so it still could be useful.
> 
> Well, if you know that the board does not have a dm3730 but a dm3725 without
> GPU it is better to disable the GPU completely instead of loading the driver
> and make it detect by some internal bits that it has no GPU on the SoC.

It shouldn't even be in the DTSI if it's not in the SoC.

> > If there's no display on the board and you really don't want the GPU
> > driver, then you can disable the driver or block the module loading, but
> > it should be a distro / package / user decision, not a DT / kernel one
> > still.
> 
> The same holds for aes: dss: dsi: hdmi: etc. If they are not used by some
> board file, they don't change a single bit of the DTB [1] which IMHO would
> be of reasonable concern to question additional labels.

Not really, no. If there's no HDMI connector, the HDMI controller is
useless. A GPU without a display can still be useful, depending on the
workload.

Maxime

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