On 12/6/23 10:02 AM, Conor Dooley wrote:
On Tue, Dec 05, 2023 at 07:04:05PM +0100, H. Nikolaus Schaller wrote:
Am 05.12.2023 um 18:33 schrieb Andrew Davis <afd@xxxxxx>:
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
AFAICT these Xilinx devices that put the revisions in the compatible are
a different case - they're "soft" IP intended for use in the fabric of
an FPGA, and assigning a device specific compatible there does not make
sense.
In this case it appears that the revision is completely known once you
see "ti,omap3630-gpu", so encoding the extra "121" into the compatible
string is not required.
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").
The "raw" sgx530 compatible does not seem helpful if the sgx530-121 or
sgx530-125 compatibles are also required to be present for the driver to
actually function. The revision specific compatibles I would not object
to, but everything in here has different revisions anyway - does the
same revision actually appear in multiple devices? If it doesn't then I
don't see any value in the suffixed compatibles either.
Everything here has different revisions because any device that uses
the same revision can also use the same base compatible string. For
instance AM335x SoCs use the SGX530 revision 125, same as OMAP3630,
so I simply reuse that compatible in their DT as you can see in
patch [5/10]. I didn't see the need for a new compatible string
for identical (i.e. "compatible") IP and integration.
The first device to use that IP/revision combo gets the named
compatible, all others re-use the same compatible where possible.
Andrew
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.
It's already machine readable if it is invariant on an SoC given the
patch had SoC-specific compatibles.
