Date:   Tue, 5 Dec 2023 11:33:06 -0600
From:   Andrew Davis <afd@...com>
To:     "H. Nikolaus Schaller" <hns@...delico.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

On 12/5/23 2:17 AM, H. Nikolaus Schaller wrote:
> 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!
> 

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@...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?
> 

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

Powered by blists - more mailing lists