[PATCH RFC 01/10] dt-bindings: gpu: Add PowerVR Series5 SGX GPUs

[H. Nikolaus Schaller]

> Am 05.12.2023 um 18:33 schrieb Andrew Davis <afd at ti.com>:
> 
> On 12/5/23 2:17 AM, H. Nikolaus Schaller wrote:
>>> +          - 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.

Then, it should belong there and not be a comment of the ti,omap*-gpu record.
In this way it does not seem to be a proper hardware description.

BTW: there are examples where the revision is part of the compatible string, even
if the (Linux) driver makes no use of it:

drivers/net/ethernet/xilinx/xilinx_emaclite.c

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

Yes, that is something we had planned to get rid of for a long time by using different compatible
strings and some variant specific struct like many others drivers are doing it.
But it was a to big task so nobody did start with it.

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

Ok, I didn't know about such registers as there is not much public information available.
Fair enough, there are some error reports about in different forums.

On the other hand we then must read out this register in more or less early initialization
stages. Even if we know this information to be static and it could be as simple as a list
of compatible strings in the driver.

> 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).

It of course tells, but may need a translation table that needs to be maintained in a
different format. Basically the same what the comments show in a non-machine readable
format.

I just wonder why the specific version can or should not become simply part of the DTS
and needs this indirection.

Basically it is a matter of openness for future (driver) development and why it needs
careful decisions.

So in other words: I would prefer to see the comments about versions encoded in the device
tree binary to make it machine readable.

BR and thanks,
Nikolaus