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
Add SGX GPU device entries to base OMAP3 dtsi files.
Signed-off-by: Andrew Davis <[email protected]>
---
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
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 <[email protected]>
---
.../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 <[email protected]>
+
+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 <[email protected]>
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
Add SGX GPU device entry to base jz4780 dtsi file.
Signed-off-by: Andrew Davis <[email protected]>
---
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
Add SGX GPU device entry to base DRA7x dtsi file.
Signed-off-by: Andrew Davis <[email protected]>
---
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
Andrew Davis <[email protected]> writes:
> Add SGX GPU device entries to base OMAP3 dtsi files.
>
> Signed-off-by: Andrew Davis <[email protected]>
> ---
Reviewed-by: Javier Martinez Canillas <[email protected]>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Andrew Davis <[email protected]> writes:
> Add SGX GPU device entry to base DRA7x dtsi file.
>
> Signed-off-by: Andrew Davis <[email protected]>
> ---
Reviewed-by: Javier Martinez Canillas <[email protected]>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
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 <[email protected]>
> ---
> .../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
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 <[email protected]>
>> ---
>> .../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
>
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 <[email protected]>
>>> ---
>>> .../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