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

From: Andrew Davis <afd@ti.com>
To: Conor Dooley <conor@kernel.org>,
	"H. Nikolaus Schaller" <hns@goldelico.com>
Cc: "Frank Binns" <frank.binns@imgtec.com>,
	"Donald Robson" <donald.robson@imgtec.com>,
	"Matt Coster" <matt.coster@imgtec.com>,
	"Adam Ford" <aford173@gmail.com>,
	"Ivaylo Dimitrov" <ivo.g.dimitrov.75@gmail.com>,
	"Maarten Lankhorst" <maarten.lankhorst@linux.intel.com>,
	"Maxime Ripard" <mripard@kernel.org>,
	"Thomas Zimmermann" <tzimmermann@suse.de>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
	"Conor Dooley" <conor+dt@kernel.org>,
	"Chen-Yu Tsai" <wens@csie.org>,
	"Jernej Skrabec" <jernej.skrabec@gmail.com>,
	"Samuel Holland" <samuel@sholland.org>,
	"Benoît Cousson" <bcousson@baylibre.com>,
	"Tony Lindgren" <tony@atomide.com>, "Nishanth Menon" <nm@ti.com>,
	"Vignesh Raghavendra" <vigneshr@ti.com>,
	"Tero Kristo" <kristo@kernel.org>,
	"Paul Cercueil" <paul@crapouillou.net>,
	dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-sunxi@lists.linux.dev, linux-omap@vger.kernel.org,
	linux-mips@vger.kernel.org
Subject: Re: [PATCH RFC 01/10] dt-bindings: gpu: Add PowerVR Series5 SGX GPUs
Date: Wed, 6 Dec 2023 10:15:26 -0600	[thread overview]
Message-ID: <8328bec9-a963-4f8a-ae03-a531749a30db@ti.com> (raw)
In-Reply-To: <20231206-wolverine-paprika-0674ca01e1f2@spud>

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