On 12/5/23 2:17 AM, H. Nikolaus Schaller wrote:
Hi Andrew,
Am 04.12.2023 um 19:22 schrieb Andrew Davis <afd@xxxxxx>:
The Imagination PowerVR Series5 "SGX" GPU is part of several SoCs from
multiple vendors.
Great and thanks for the new attempt to get at least the Device Tree side
upstream. Really appreciated!
Thanks for helping us maintain these GPUs with the OpenPVRSGX project :)
Describe how the SGX GPU is integrated in these SoC,
including register space and interrupts.
Clocks, reset, and power domain
information is SoC specific.
Indeed. This makes it understandable why you did not directly
take our scheme from the openpvrsgx project.
Signed-off-by: Andrew Davis <afd@xxxxxx>
---
.../devicetree/bindings/gpu/img,powervr.yaml | 69 +++++++++++++++++--
1 file changed, 63 insertions(+), 6 deletions(-)
diff --git a/Documentation/devicetree/bindings/gpu/img,powervr.yaml b/Documentation/devicetree/bindings/gpu/img,powervr.yaml
index a13298f1a1827..9f036891dad0b 100644
--- a/Documentation/devicetree/bindings/gpu/img,powervr.yaml
+++ b/Documentation/devicetree/bindings/gpu/img,powervr.yaml
@@ -11,11 +11,33 @@ maintainers:
- Frank Binns <frank.binns@xxxxxxxxxx>
properties:
+ $nodename:
+ pattern: '^gpu@[a-f0-9]+$'
+
compatible:
- items:
- - enum:
- - ti,am62-gpu
- - const: img,img-axe # IMG AXE GPU model/revision is fully discoverable
+ oneOf:
+ - items:
+ - enum:
+ - ti,am62-gpu
+ - const: img,img-axe # IMG AXE GPU model/revision is fully discoverable
+ - items:
+ - enum:
+ - ti,omap3430-gpu # Rev 121
+ - ti,omap3630-gpu # Rev 125
Is the "Rev 121" and "Rev 125" a property of the SoC integration (clock/reset/power
hookup etc.) or of the integrated SGX core?
The Rev is a property of the SGX core, not the SoC integration. But it seems that
compatible string is being used to define both (as we see being debated in the other
thread on this series).
In my understanding the Revs are different variants of the SGX core (errata
fixes, instruction set, pipeline size etc.). And therefore the current driver code
has to be configured by some macros to handle such cases.
So the Rev should IMHO be part of the next line:
+ - const: img,powervr-sgx530
+ - enum:
+ - img,powervr-sgx530-121
+ - img,powervr-sgx530-125
We have a similar definition in the openpvrsgx code.
Example: compatible = "ti,omap3-sgx530-121", "img,sgx530-121", "img,sgx530";
(I don't mind about the powervr- prefix).
This would allow a generic and universal sgx driver (loaded through just matching
"img,sgx530") to handle the errata and revision specifics at runtime based on the
compatible entry ("img,sgx530-121") and know about SoC integration ("ti,omap3-sgx530-121").
And user-space can be made to load the right firmware variant based on "img,sgx530-121"
I don't know if there is some register which allows to discover the revision long
before the SGX subsystem is initialized and the firmware is up and running.
What I know is that it is possible to read out the revision after starting the firmware
but it may just echo the version number of the firmware binary provided from user-space.
We should be able to read out the revision (register EUR_CR_CORE_REVISION), the problem is
today the driver is built for a given revision at compile time. That is a software issue,
not something that we need to encode in DT. While the core ID (SGX5xx) can be also detected
(EUR_CR_CORE_ID), the location of that register changes, and so it does need encoded in
DT compatible.
The string "ti,omap3430-gpu" tells us the revision if we cannot detect it (as in the current
driver), and the SoC integration is generic anyway (just a reg and interrupt).
Andrew
+ - 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
@@ -40,8 +62,6 @@ properties:
required:
- compatible
- reg
- - clocks
- - clock-names
- interrupts
additionalProperties: false
@@ -56,6 +76,43 @@ allOf:
properties:
clocks:
maxItems: 1
+ required:
+ - clocks
+ - clock-names
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: ti,am654-sgx544
+ then:
+ properties:
+ power-domains:
+ minItems: 1
+ required:
+ - power-domains
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: allwinner,sun6i-a31-gpu
+ then:
+ properties:
+ clocks:
+ minItems: 2
+ clock-names:
+ minItems: 2
+ required:
+ - clocks
+ - clock-names
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: ingenic,jz4780-gpu
+ then:
+ required:
+ - clocks
+ - clock-names
examples:
- |
--
2.39.2
BR and thanks,
Nikolaus
