Date:   Tue, 5 Dec 2023 09:17:55 +0100
From:   "H. Nikolaus Schaller" <hns@...delico.com>
To:     Andrew Davis <afd@...com>
Cc:     Frank Binns <frank.binns@...tec.com>,
        Donald Robson <donald.robson@...tec.com>,
        Matt Coster <matt.coster@...tec.com>,
        Adam Ford <aford173@...il.com>,
        Ivaylo Dimitrov <ivo.g.dimitrov.75@...il.com>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Chen-Yu Tsai <wens@...e.org>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        Samuel Holland <samuel@...lland.org>,
        Benoît Cousson <bcousson@...libre.com>,
        Tony Lindgren <tony@...mide.com>, Nishanth Menon <nm@...com>,
        Vignesh Raghavendra <vigneshr@...com>,
        Tero Kristo <kristo@...nel.org>,
        Paul Cercueil <paul@...pouillou.net>,
        dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-sunxi@...ts.linux.dev, linux-omap@...r.kernel.org,
        linux-mips@...r.kernel.org
Subject: Re: [PATCH RFC 01/10] dt-bindings: gpu: Add PowerVR Series5 SGX GPUs

Hi Andrew,

> Am 04.12.2023 um 19:22 schrieb Andrew Davis <afd@...com>:
> 
> 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!

> 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@...com>
> ---
> .../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@...tec.com>
> 
> 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?

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.

> +      - 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

Powered by blists - more mailing lists