<html><head><meta http-equiv="content-type" content="text/html; charset=us-ascii"></head><body style="overflow-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;"><br id="lineBreakAtBeginningOfMessage"><div><br><blockquote type="cite"><div>Am 06.12.2023 um 17:15 schrieb Andrew Davis <afd@ti.com>:</div><br class="Apple-interchange-newline"><div><div>On 12/6/23 10:02 AM, Conor Dooley wrote:<br><blockquote type="cite">On Tue, Dec 05, 2023 at 07:04:05PM +0100, H. Nikolaus Schaller wrote:<br><blockquote type="cite"><blockquote type="cite">Am 05.12.2023 um 18:33 schrieb Andrew Davis <afd@ti.com>:<br><br>On 12/5/23 2:17 AM, H. Nikolaus Schaller wrote:<br><blockquote type="cite"><blockquote type="cite">+ - enum:<br>+ - ti,omap3430-gpu # Rev 121<br>+ - ti,omap3630-gpu # Rev 125<br></blockquote>Is the "Rev 121" and "Rev 125" a property of the SoC integration (clock/reset/power<br>hookup etc.) or of the integrated SGX core?<br></blockquote><br>The Rev is a property of the SGX core, not the SoC integration.<br></blockquote><br>Then, it should belong there and not be a comment of the ti,omap*-gpu record.<br>In this way it does not seem to be a proper hardware description.<br><br>BTW: there are examples where the revision is part of the compatible string, even<br>if the (Linux) driver makes no use of it:<br><br>drivers/net/ethernet/xilinx/xilinx_emaclite.c<br></blockquote>AFAICT these Xilinx devices that put the revisions in the compatible are<br>a different case - they're "soft" IP intended for use in the fabric of<br>an FPGA, and assigning a device specific compatible there does not make<br>sense.<br>In this case it appears that the revision is completely known once you<br>see "ti,omap3630-gpu", so encoding the extra "121" into the compatible<br>string is not required.<br><blockquote type="cite"><br><blockquote type="cite">But it seems that<br>compatible string is being used to define both (as we see being debated in the other<br>thread on this series).<br><br><blockquote type="cite">In my understanding the Revs are different variants of the SGX core (errata<br>fixes, instruction set, pipeline size etc.). And therefore the current driver code<br>has to be configured by some macros to handle such cases.<br>So the Rev should IMHO be part of the next line:<br><blockquote type="cite">+ - const: img,powervr-sgx530<br></blockquote>+ - enum:<br>+ - img,powervr-sgx530-121<br>+ - img,powervr-sgx530-125<br>We have a similar definition in the openpvrsgx code.<br>Example: compatible = "ti,omap3-sgx530-121", "img,sgx530-121", "img,sgx530";<br>(I don't mind about the powervr- prefix).<br>This would allow a generic and universal sgx driver (loaded through just matching<br>"img,sgx530") to handle the errata and revision specifics at runtime based on the<br>compatible entry ("img,sgx530-121") and know about SoC integration ("ti,omap3-sgx530-121").<br></blockquote></blockquote></blockquote>The "raw" sgx530 compatible does not seem helpful if the sgx530-121 or<br>sgx530-125 compatibles are also required to be present for the driver to<br>actually function. The revision specific compatibles I would not object<br>to, but everything in here has different revisions anyway - does the<br>same revision actually appear in multiple devices? If it doesn't then I<br>don't see any value in the suffixed compatibles either.<br></blockquote><br>Everything here has different revisions because any device that uses<br>the same revision can also use the same base compatible string. For<br>instance AM335x SoCs use the SGX530 revision 125, same as OMAP3630,<br>so I simply reuse that compatible in their DT as you can see in<br>patch [5/10]. I didn't see the need for a new compatible string<br>for identical (i.e. "compatible") IP and integration.<br></div></div></blockquote><div><br></div>Ok, this is a point. As long as there is no SoC which can come with different</div><div>SGX revisions the SoC compatible is enough for everything.</div><div><br></div><div>I never looked it that way.</div><div><br><blockquote type="cite"><div><div><br>The first device to use that IP/revision combo gets the named<br>compatible, all others re-use the same compatible where possible.<br></div></div></blockquote><div><br></div>Hm. If we take this rule, we can even completely leave out all</div><div><span style="font-family: Menlo-Regular; font-size: 11px;"><br></span></div><div><span style="font-family: Menlo-Regular; font-size: 11px;">+ - const: img,powervr-sgx530</span><br style="font-family: Menlo-Regular; font-size: 11px;"><span style="font-family: Menlo-Regular; font-size: 11px;">+ - const: img,powervr-sgx540</span></div><div><span style="font-family: Menlo-Regular; font-size: 11px;">+ - const: img,powervr-sgx544</span><br style="font-family: Menlo-Regular; font-size: 11px;"><br></div><div>and just have a list of allsgx compatible SoC:</div><div><span style="font-family: Menlo-Regular; font-size: 11px;"><br></span></div><div><span style="font-family: Menlo-Regular; font-size: 11px;">+ - items:</span><br style="font-family: Menlo-Regular; font-size: 11px;"><span style="font-family: Menlo-Regular; font-size: 11px;">+ - enum:</span><br style="font-family: Menlo-Regular; font-size: 11px;"><span style="font-family: Menlo-Regular; font-size: 11px;">+ - ti,am62-gpu<span class="Apple-tab-span" style="white-space:pre"> </span># </span><span style="font-family: Menlo-Regular; font-size: 11px;">IMG AXE GPU model/revision is fully discoverable</span><br style="font-family: Menlo-Regular; font-size: 11px;"><span style="font-family: Menlo-Regular; font-size: 11px;">+ - ti,omap3430-gpu # sgx530 Rev 121</span><br style="font-family: Menlo-Regular; font-size: 11px;"><span style="font-family: Menlo-Regular; font-size: 11px;">+ - ti,omap3630-gpu # sgx530 Rev 125</span><br style="font-family: Menlo-Regular; font-size: 11px;"><span style="font-family: Menlo-Regular; font-size: 11px;">+ - ingenic,jz4780-gpu # sgx540 Rev 130</span><br style="font-family: Menlo-Regular; font-size: 11px;"><span style="font-family: Menlo-Regular; font-size: 11px;">+ - ti,omap4430-gpu # sgx540 Rev 120</span><br style="font-family: Menlo-Regular; font-size: 11px;"><span style="font-family: Menlo-Regular; font-size: 11px;">+ - allwinner,sun6i-a31-gpu # sgx544 MP2 Rev 115</span><br style="font-family: Menlo-Regular; font-size: 11px;"><span style="font-family: Menlo-Regular; font-size: 11px;">+ - ti,omap4470-gpu # </span><span style="font-family: Menlo-Regular; font-size: 11px;">sgx544</span><span style="font-family: Menlo-Regular; font-size: 11px;"> </span><span style="font-family: Menlo-Regular; font-size: 11px;">MP1 Rev 112</span></div><div><span style="font-family: Menlo-Regular; font-size: 11px;">+ - ti,omap5432-gpu # </span><span style="font-family: Menlo-Regular; font-size: 11px;">sgx544</span><span style="font-family: Menlo-Regular; font-size: 11px;"> </span><span style="font-family: Menlo-Regular; font-size: 11px;">MP2 Rev 105</span></div><div><span style="font-family: Menlo-Regular; font-size: 11px;">+ - ti,am5728-gpu # </span><span style="font-family: Menlo-Regular; font-size: 11px;">sgx544</span><span style="font-family: Menlo-Regular; font-size: 11px;"> </span><span style="font-family: Menlo-Regular; font-size: 11px;">MP2 Rev 116</span></div><div><span style="font-family: Menlo-Regular; font-size: 11px;">+ - ti,am6548-gpu # </span><span style="font-family: Menlo-Regular; font-size: 11px;">sgx544</span><span style="font-family: Menlo-Regular; font-size: 11px;"> </span><span style="font-family: Menlo-Regular; font-size: 11px;">MP1 Rev 117</span></div><div><font face="Menlo-Regular"><span style="font-size: 11px;">+ - more to come</span></font></div><div><br></div><div><blockquote type="cite"><div><div><br>Andrew<br><br><blockquote type="cite"><blockquote type="cite"><blockquote type="cite"><blockquote type="cite">And user-space can be made to load the right firmware variant based on "img,sgx530-121"<br>I don't know if there is some register which allows to discover the revision long<br>before the SGX subsystem is initialized and the firmware is up and running.<br>What I know is that it is possible to read out the revision after starting the firmware<br>but it may just echo the version number of the firmware binary provided from user-space.<br></blockquote><br>We should be able to read out the revision (register EUR_CR_CORE_REVISION), the problem is<br>today the driver is built for a given revision at compile time.<br></blockquote><br>Yes, that is something we had planned to get rid of for a long time by using different compatible<br>strings and some variant specific struct like many others drivers are doing it.<br>But it was a to big task so nobody did start with it.<br><br><blockquote type="cite">That is a software issue,<br>not something that we need to encode in DT. While the core ID (SGX5xx) can be also detected<br>(EUR_CR_CORE_ID), the location of that register changes, and so it does need encoded in<br>DT compatible.<br></blockquote><br>Ok, I didn't know about such registers as there is not much public information available.<br>Fair enough, there are some error reports about in different forums.<br><br>On the other hand we then must read out this register in more or less early initialization<br>stages. Even if we know this information to be static and it could be as simple as a list<br>of compatible strings in the driver.<br><br><blockquote type="cite">The string "ti,omap3430-gpu" tells us the revision if we cannot detect it (as in the current<br>driver), and the SoC integration is generic anyway (just a reg and interrupt).<br></blockquote><br>It of course tells, but may need a translation table that needs to be maintained in a<br>different format. Basically the same what the comments show in a non-machine readable<br>format.<br><br>I just wonder why the specific version can or should not become simply part of the DTS<br>and needs this indirection.<br><br>Basically it is a matter of openness for future (driver) development and why it needs<br>careful decisions.<br><br>So in other words: I would prefer to see the comments about versions encoded in the device<br>tree binary to make it machine readable.<br></blockquote>It's already machine readable if it is invariant on an SoC given the<br>patch had SoC-specific compatibles.<br></blockquote><br></div></div></blockquote></div><br></body></html>