| Message ID | 20231204182245.33683-2-afd@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Device tree support for Imagination Series5 GPU | expand |
On Mon, Dec 04, 2023 at 12:22:36PM -0600, Andrew Davis wrote: > The Imagination PowerVR Series5 "SGX" GPU is part of several SoCs from > multiple vendors. Describe how the SGX GPU is integrated in these SoC, > including register space and interrupts. Clocks, reset, and power domain > information is SoC specific. > > Signed-off-by: Andrew Davis <afd@ti.com> > --- > .../devicetree/bindings/gpu/img,powervr.yaml | 69 +++++++++++++++++-- > 1 file changed, 63 insertions(+), 6 deletions(-) I think it would be best to have a separate file for this, img,sgx.yaml maybe? Maxime
On 04/12/2023 19:22, Andrew Davis wrote: > The Imagination PowerVR Series5 "SGX" GPU is part of several SoCs from > multiple vendors. Describe how the SGX GPU is integrated in these SoC, > including register space and interrupts. Clocks, reset, and power domain > information is SoC specific. > > Signed-off-by: Andrew Davis <afd@ti.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@imgtec.com> > > properties: > + $nodename: > + pattern: '^gpu@[a-f0-9]+$' Why? We do not enforce it in other bindings. > + > 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 > + - const: img,powervr-sgx530 > + - 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 I don't think so... How can you run without clocks? > - interrupts > > additionalProperties: false > @@ -56,6 +76,43 @@ allOf: > properties: > clocks: > maxItems: 1 > + required: > + - clocks > + - clock-names You need to define the clocks for your variants or disallow them. The original code should be fixed as well and make the clocks fixed for all img-axe cases. Best regards, Krzysztof
* Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> [231205 07:10]: > On 04/12/2023 19:22, Andrew Davis wrote: > > @@ -56,6 +76,43 @@ allOf: > > properties: > > clocks: > > maxItems: 1 > > + required: > > + - clocks > > + - clock-names > > You need to define the clocks for your variants or disallow them. The > original code should be fixed as well and make the clocks fixed for all > img-axe cases. To clarify, the clocks may be optional as they can be hardwired and coming from the interconnect target wrapper module and enabled with runtime PM. Regards, Tony
On 05/12/2023 08:56, Tony Lindgren wrote: > * Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> [231205 07:10]: >> On 04/12/2023 19:22, Andrew Davis wrote: >>> @@ -56,6 +76,43 @@ allOf: >>> properties: >>> clocks: >>> maxItems: 1 >>> + required: >>> + - clocks >>> + - clock-names >> >> You need to define the clocks for your variants or disallow them. The >> original code should be fixed as well and make the clocks fixed for all >> img-axe cases. > > To clarify, the clocks may be optional as they can be hardwired and coming > from the interconnect target wrapper module and enabled with runtime PM. What does runtime PM have to do with it? If runtime PM enables clocks, these are real signals and not optional. The bindings is quite unspecific in that matter which is not what we want usually. If you have implementation which does not have these clocks exposed, then disallow them. Just don't make it floating like it is in existing binding and how Andrew continues for new devices. Best regards, Krzysztof
* Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> [231205 08:03]: > What does runtime PM have to do with it? If runtime PM enables clocks, > these are real signals and not optional. Runtime PM propagates to the parent device. Regards, Tony
On 05/12/2023 09:10, Tony Lindgren wrote: > * Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> [231205 08:03]: >> What does runtime PM have to do with it? If runtime PM enables clocks, >> these are real signals and not optional. > > Runtime PM propagates to the parent device. Then it is not really relevant to the hardware talk here, unless you put this device clocks in parent node, but then it's just wrong hardware description. Best regards, Krzysztof
Hi Andrew, > Am 04.12.2023 um 19:22 schrieb Andrew Davis <afd@ti.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! > 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@ti.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@imgtec.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? 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. > + - 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
> Am 05.12.2023 um 07:57 schrieb Maxime Ripard <mripard@kernel.org>: > > On Mon, Dec 04, 2023 at 12:22:36PM -0600, Andrew Davis wrote: >> The Imagination PowerVR Series5 "SGX" GPU is part of several SoCs from >> multiple vendors. Describe how the SGX GPU is integrated in these SoC, >> including register space and interrupts. Clocks, reset, and power domain >> information is SoC specific. >> >> Signed-off-by: Andrew Davis <afd@ti.com> >> --- >> .../devicetree/bindings/gpu/img,powervr.yaml | 69 +++++++++++++++++-- >> 1 file changed, 63 insertions(+), 6 deletions(-) > > I think it would be best to have a separate file for this, img,sgx.yaml > maybe? Why? The whole family of IMG GPUs is PowerVR and SGX and Rogue are generations 5 and 6++: https://en.wikipedia.org/wiki/PowerVR So I would suggest to keep either img,powervr.yaml for all of them or img,powervr-sgx.yaml img,powervr-rogue.yaml etc. But as far as I can see the hardware integration into SoC (and hence description) is quite similar so a single file should suffice. BR, Nikolaus
* Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> [231205 08:16]: > On 05/12/2023 09:10, Tony Lindgren wrote: > > * Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> [231205 08:03]: > >> What does runtime PM have to do with it? If runtime PM enables clocks, > >> these are real signals and not optional. > > > > Runtime PM propagates to the parent device. > > Then it is not really relevant to the hardware talk here, unless you put > this device clocks in parent node, but then it's just wrong hardware > description. No it's not. The interconnect target module may have one or more separate devices with the same shared clocks. See for example the am3 usb module that has usb controllers, phys and dma at target-module@47400000 in am33xx.dtsi. Sure the clock nodes can be there for the child IP, but they won't do anything. And still need to be managed separately by the device driver if added. Regards, Tony
On 05/12/2023 09:30, Tony Lindgren wrote: > * Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> [231205 08:16]: >> On 05/12/2023 09:10, Tony Lindgren wrote: >>> * Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> [231205 08:03]: >>>> What does runtime PM have to do with it? If runtime PM enables clocks, >>>> these are real signals and not optional. >>> >>> Runtime PM propagates to the parent device. >> >> Then it is not really relevant to the hardware talk here, unless you put >> this device clocks in parent node, but then it's just wrong hardware >> description. > > No it's not. The interconnect target module may have one or more separate Interconnects are not parents of devices, so I still don't get why do you bring it here. > devices with the same shared clocks. See for example the am3 usb module that > has usb controllers, phys and dma at target-module@47400000 in am33xx.dtsi. There is no interconnect-cells there, so why do we talk about interconnect here? > > Sure the clock nodes can be there for the child IP, but they won't do > anything. And still need to be managed separately by the device driver if > added. So if OS does not have runtime PM, the bindings are wrong? Bindings should not depend on some particular feature of some particular OS. Best regards, Krzysztof
On Tue, 5 Dec 2023 09:45:44 +0100 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > Sure the clock nodes can be there for the child IP, but they won't do > > anything. And still need to be managed separately by the device driver if > > added. > > So if OS does not have runtime PM, the bindings are wrong? Bindings > should not depend on some particular feature of some particular OS. Any user of the devicetree sees that there is a parent and the parent needs to be enabled by some mechanism. E.g. I2c devices do not specify the clocks of the parent (the i2c master) Maybe it is just more fine-grained on omap. look e.g. at ti/omap/omap4-l4.dtsi there are target-module@xxxx with the devices as a child and a clock in the parent. Regards, Andreas
On 05/12/2023 10:02, Andreas Kemnade wrote: > On Tue, 5 Dec 2023 09:45:44 +0100 > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > >>> Sure the clock nodes can be there for the child IP, but they won't do >>> anything. And still need to be managed separately by the device driver if >>> added. >> >> So if OS does not have runtime PM, the bindings are wrong? Bindings >> should not depend on some particular feature of some particular OS. > > Any user of the devicetree sees that there is a parent and the parent needs > to be enabled by some mechanism. > E.g. I2c devices do not specify the clocks of the parent (the i2c master) If you use this analogy, then compare it with an I2C device which has these clock inputs. Such device must have clocks in the bindings. > > Maybe it is just more fine-grained on omap. > > look e.g. at ti/omap/omap4-l4.dtsi > there are target-module@xxxx > with the devices as a child and a clock in the parent. Not related to runtime PM... Best regards, Krzysztof
On Tue, 5 Dec 2023 10:27:56 +0100 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 05/12/2023 10:02, Andreas Kemnade wrote: > > On Tue, 5 Dec 2023 09:45:44 +0100 > > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > > >>> Sure the clock nodes can be there for the child IP, but they won't do > >>> anything. And still need to be managed separately by the device driver if > >>> added. > >> > >> So if OS does not have runtime PM, the bindings are wrong? Bindings > >> should not depend on some particular feature of some particular OS. > > > > Any user of the devicetree sees that there is a parent and the parent needs > > to be enabled by some mechanism. > > E.g. I2c devices do not specify the clocks of the parent (the i2c master) > > If you use this analogy, then compare it with an I2C device which has > these clock inputs. Such device must have clocks in the bindings. > I would see target-module = i2c master. Well, if there is a variant of the i2c device which does not require external clocks and a variant which requires it, then clock can be optional. > > > > Maybe it is just more fine-grained on omap. > > > > look e.g. at ti/omap/omap4-l4.dtsi > > there are target-module@xxxx > > with the devices as a child and a clock in the parent. > > Not related to runtime PM... > Well, runtime PM is just the linux-specific mechanism to enable the resources needed by the parent, so yes, it is not related... As said, another OS can have another mechanism. But anyways, the target module specifies resources which are required. Regards, Andreas
Hi, On Tue, Dec 05, 2023 at 09:18:58AM +0100, H. Nikolaus Schaller wrote: > > Am 05.12.2023 um 07:57 schrieb Maxime Ripard <mripard@kernel.org>: > > > > On Mon, Dec 04, 2023 at 12:22:36PM -0600, Andrew Davis wrote: > >> The Imagination PowerVR Series5 "SGX" GPU is part of several SoCs from > >> multiple vendors. Describe how the SGX GPU is integrated in these SoC, > >> including register space and interrupts. Clocks, reset, and power domain > >> information is SoC specific. > >> > >> Signed-off-by: Andrew Davis <afd@ti.com> > >> --- > >> .../devicetree/bindings/gpu/img,powervr.yaml | 69 +++++++++++++++++-- > >> 1 file changed, 63 insertions(+), 6 deletions(-) > > > > I think it would be best to have a separate file for this, img,sgx.yaml > > maybe? > > Why? Because it's more convenient? > The whole family of IMG GPUs is PowerVR and SGX and Rogue are generations 5 and 6++: > > https://en.wikipedia.org/wiki/PowerVR That's not really relevant as far as bindings go. We have multiple binding files for devices of the same generation, or single bindings covering multiple generations. The important part is that every compatible is documented. It doesn't really matter how or where. Maxime
Hi, > Am 05.12.2023 um 14:29 schrieb Maxime Ripard <mripard@kernel.org>: > > Hi, > > On Tue, Dec 05, 2023 at 09:18:58AM +0100, H. Nikolaus Schaller wrote: >>> Am 05.12.2023 um 07:57 schrieb Maxime Ripard <mripard@kernel.org>: >>> >>> On Mon, Dec 04, 2023 at 12:22:36PM -0600, Andrew Davis wrote: >>>> The Imagination PowerVR Series5 "SGX" GPU is part of several SoCs from >>>> multiple vendors. Describe how the SGX GPU is integrated in these SoC, >>>> including register space and interrupts. Clocks, reset, and power domain >>>> information is SoC specific. >>>> >>>> Signed-off-by: Andrew Davis <afd@ti.com> >>>> --- >>>> .../devicetree/bindings/gpu/img,powervr.yaml | 69 +++++++++++++++++-- >>>> 1 file changed, 63 insertions(+), 6 deletions(-) >>> >>> I think it would be best to have a separate file for this, img,sgx.yaml >>> maybe? >> >> Why? > > Because it's more convenient? Is it? >> The whole family of IMG GPUs is PowerVR and SGX and Rogue are generations 5 and 6++: >> >> https://en.wikipedia.org/wiki/PowerVR > > That's not really relevant as far as bindings go. But maybe for choosing binding file names. Well they are machine readable but sometimes humans work with them. > We have multiple > binding files for devices of the same generation, or single bindings > covering multiple generations. > > The important part is that every compatible is documented. It doesn't > really matter how or where. Yes, and that is why I would find it more convenient to have a single "img,powervr.yaml" for all variations unless it becomes filled with unrelated stuff (which isn't as far as I see). BR, Nikolaus
On 12/5/23 2:17 AM, H. Nikolaus Schaller wrote: > Hi Andrew, > >> Am 04.12.2023 um 19:22 schrieb Andrew Davis <afd@ti.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@ti.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@imgtec.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
> 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 > 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
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. > >> 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.
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. >
> Am 06.12.2023 um 17:02 schrieb Conor Dooley <conor@kernel.org>: > > 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. Well, I would not put my hand in the fire for this assumption. And I am a friend of explicitly stating what is instead ot encoding indirectly. > >> >>> 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. Indeed. This seems to be redundant (but may need some pattern processing). > 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. Well, we don't know. So far only a subset of SoC with the SGX GPU core variants has been considered (mainly because lack of user space code and device owners). Maybe someone with insider knowledge can give a hint if the SGX version numbers were assigned uniquely for each SoC integration project. > >>>> 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. But needs a translation table to get to the revision number. I have not yet brought into discussion that there are different firmwares for sgx530-121, sgx530-125, sgx544-116 etc. And user-space code may also depend on to be able to chose the right one if multiple firmware packages are installed. Currently this is not the case but would be a major benfit for OS packages. To automate this we need a mechanism to scan the device tree for a compatible string that tells which firmware variant to load. But why force this to depend on the SoC compatible if it only depends indirectly? By the way, there is a tested and working driver using the scheme with the sub-versions: https://github.com/openpvrsgx-devgroup/linux_openpvrsgx/blob/11cc7876ba39b6172d19ee0bf0a872c1d3d745e1/drivers/gpu/drm/pvrsgx/pvr-drv.c#L306 On the other hand As far as I see this will can of course be adapted to the newly proposed scheme. But it still seems a bit twisted to me. Maybe because we for example define LCD panel compatibles not by the compatible of the device they are installed in. BR, Nikolaus
> Am 06.12.2023 um 17:15 schrieb Andrew Davis <afd@ti.com>: > > 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. Ok, this is a point. As long as there is no SoC which can come with different SGX revisions the SoC compatible is enough for everything. I never looked it that way. > > The first device to use that IP/revision combo gets the named > compatible, all others re-use the same compatible where possible. Hm. If we take this rule, we can even completely leave out all + - const: img,powervr-sgx530 + - const: img,powervr-sgx540 + - const: img,powervr-sgx544 and just have a list of allsgx compatible SoC: + - items: + - enum: + - ti,am62-gpu # IMG AXE GPU model/revision is fully discoverable + - ti,omap3430-gpu # sgx530 Rev 121 + - ti,omap3630-gpu # sgx530 Rev 125 + - ingenic,jz4780-gpu # sgx540 Rev 130 + - ti,omap4430-gpu # sgx540 Rev 120 + - allwinner,sun6i-a31-gpu # sgx544 MP2 Rev 115 + - ti,omap4470-gpu # sgx544 MP1 Rev 112 + - ti,omap5432-gpu # sgx544 MP2 Rev 105 + - ti,am5728-gpu # sgx544 MP2 Rev 116 + - ti,am6548-gpu # sgx544 MP1 Rev 117 + - more to come > > 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. >
(non-html) > Am 06.12.2023 um 17:15 schrieb Andrew Davis <afd@ti.com>: > > 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. Ok, this is a point. As long as there is no SoC which can come with different SGX revisions the SoC compatible is enough for everything. I never looked it that way. > > The first device to use that IP/revision combo gets the named > compatible, all others re-use the same compatible where possible. Hm. If we take this rule, we can even completely leave out all + - const: img,powervr-sgx530 + - const: img,powervr-sgx540 + - const: img,powervr-sgx544 and just have a list of allsgx compatible SoC: + - items: + - enum: + - ti,am62-gpu # IMG AXE GPU model/revision is fully discoverable + - ti,omap3430-gpu # sgx530 Rev 121 + - ti,omap3630-gpu # sgx530 Rev 125 + - ingenic,jz4780-gpu # sgx540 Rev 130 + - ti,omap4430-gpu # sgx540 Rev 120 + - allwinner,sun6i-a31-gpu # sgx544 MP2 Rev 115 + - ti,omap4470-gpu # sgx544 MP1 Rev 112 + - ti,omap5432-gpu # sgx544 MP2 Rev 105 + - ti,am5728-gpu # sgx544 MP2 Rev 116 + - ti,am6548-gpu # sgx544 MP1 Rev 117 + - more to come > > 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. >
* Andreas Kemnade <andreas@kemnade.info> [231205 09:43]: > On Tue, 5 Dec 2023 10:27:56 +0100 > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > > On 05/12/2023 10:02, Andreas Kemnade wrote: > > > On Tue, 5 Dec 2023 09:45:44 +0100 > > > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > > > > >>> Sure the clock nodes can be there for the child IP, but they won't do > > >>> anything. And still need to be managed separately by the device driver if > > >>> added. > > >> > > >> So if OS does not have runtime PM, the bindings are wrong? Bindings > > >> should not depend on some particular feature of some particular OS. > > > > > > Any user of the devicetree sees that there is a parent and the parent needs > > > to be enabled by some mechanism. > > > E.g. I2c devices do not specify the clocks of the parent (the i2c master) Yeah the interconnect target module needs to be enabled before the child IP can be probed for any OS. That is unless the target module is left on from the bootloader. But like I said, I have no objection to also having the clocks for the child SGX device here. I think two out of the tree SGX clocks are merged, so one of the three clocks would repeat twice in the binding. We do provide some of the clock aliases, like fck and ick, for the child ip automatically by the ti-sysc interconnect target module. But likely we don't want to clock name specific handling in the driver so best to standardize on SGX specific clock names. That is if the clock properties are not set optional. > > If you use this analogy, then compare it with an I2C device which has > > these clock inputs. Such device must have clocks in the bindings. > > > I would see target-module = i2c master. > > Well, if there is a variant of the i2c device which does not require > external clocks and a variant which requires it, then clock can be > optional. Yes that sounds about right for an analogy :) Regards, Tony
On Tue, Dec 05, 2023 at 02:50:08PM +0100, H. Nikolaus Schaller wrote: > Hi, > > > Am 05.12.2023 um 14:29 schrieb Maxime Ripard <mripard@kernel.org>: > > > > Hi, > > > > On Tue, Dec 05, 2023 at 09:18:58AM +0100, H. Nikolaus Schaller wrote: > >>> Am 05.12.2023 um 07:57 schrieb Maxime Ripard <mripard@kernel.org>: > >>> > >>> On Mon, Dec 04, 2023 at 12:22:36PM -0600, Andrew Davis wrote: > >>>> The Imagination PowerVR Series5 "SGX" GPU is part of several SoCs from > >>>> multiple vendors. Describe how the SGX GPU is integrated in these SoC, > >>>> including register space and interrupts. Clocks, reset, and power domain > >>>> information is SoC specific. > >>>> > >>>> Signed-off-by: Andrew Davis <afd@ti.com> > >>>> --- > >>>> .../devicetree/bindings/gpu/img,powervr.yaml | 69 +++++++++++++++++-- > >>>> 1 file changed, 63 insertions(+), 6 deletions(-) > >>> > >>> I think it would be best to have a separate file for this, img,sgx.yaml > >>> maybe? > >> > >> Why? > > > > Because it's more convenient? > > Is it? It's for a separate architecture, with a separate driver, maintained out of tree by a separate community, with a separate set of requirements as evidenced by the other thread. And that's all fine in itself, but there's very little reason to put these two bindings in the same file. We could also turn this around, why is it important that it's in the same file? > >> The whole family of IMG GPUs is PowerVR and SGX and Rogue are generations 5 and 6++: > >> > >> https://en.wikipedia.org/wiki/PowerVR > > > > That's not really relevant as far as bindings go. > > But maybe for choosing binding file names. Well they are machine readable > but sometimes humans work with them. Heh. It's something that can also be easily grepped, and the name is never going to reflect all the compatibles in a binding so it's what you'll end up doing anyway. But feel free to suggest another name to avoid the confusion. > > We have multiple > > binding files for devices of the same generation, or single bindings > > covering multiple generations. > > > > The important part is that every compatible is documented. It doesn't > > really matter how or where. > > Yes, and that is why I would find it more convenient to have a single > "img,powervr.yaml" for all variations unless it becomes filled with > unrelated stuff (which isn't as far as I see). Again, hard disagree there. Maxime
Hi Maxime, > Am 07.12.2023 um 10:20 schrieb Maxime Ripard <mripard@kernel.org>: > > On Tue, Dec 05, 2023 at 02:50:08PM +0100, H. Nikolaus Schaller wrote: >> Hi, >> >>> Am 05.12.2023 um 14:29 schrieb Maxime Ripard <mripard@kernel.org>: >>> >>> Hi, >>> >>> On Tue, Dec 05, 2023 at 09:18:58AM +0100, H. Nikolaus Schaller wrote: >>>>> Am 05.12.2023 um 07:57 schrieb Maxime Ripard <mripard@kernel.org>: >>>>> >>>>> On Mon, Dec 04, 2023 at 12:22:36PM -0600, Andrew Davis wrote: >>>>>> The Imagination PowerVR Series5 "SGX" GPU is part of several SoCs from >>>>>> multiple vendors. Describe how the SGX GPU is integrated in these SoC, >>>>>> including register space and interrupts. Clocks, reset, and power domain >>>>>> information is SoC specific. >>>>>> >>>>>> Signed-off-by: Andrew Davis <afd@ti.com> >>>>>> --- >>>>>> .../devicetree/bindings/gpu/img,powervr.yaml | 69 +++++++++++++++++-- >>>>>> 1 file changed, 63 insertions(+), 6 deletions(-) >>>>> >>>>> I think it would be best to have a separate file for this, img,sgx.yaml >>>>> maybe? >>>> >>>> Why? >>> >>> Because it's more convenient? >> >> Is it? > > It's for a separate architecture, with a separate driver, maintained out > of tree by a separate community, with a separate set of requirements as > evidenced by the other thread. And that's all fine in itself, but > there's very little reason to put these two bindings in the same file. > > We could also turn this around, why is it important that it's in the > same file? Same vendor. And enough similarity in architectures, even a logical sequence of development of versions (SGX = Version 5, Rogue = Version 6+) behind. (SGX and Rogue seem to be just trade names for their architecture development). AFAIK bindings should describe hardware and not communities or drivers or who is currently maintaining it. The latter can change, the first not. > >>>> The whole family of IMG GPUs is PowerVR and SGX and Rogue are generations 5 and 6++: >>>> >>>> https://en.wikipedia.org/wiki/PowerVR >>> >>> That's not really relevant as far as bindings go. >> >> But maybe for choosing binding file names. Well they are machine readable >> but sometimes humans work with them. > > Heh. It's something that can also be easily grepped, Yes, arbitrarily introduced confusion can always be resolved by search engines and makes them necessary and more and more advanced :) > and the name is > never going to reflect all the compatibles in a binding so it's what > you'll end up doing anyway. But feel free to suggest another name to > avoid the confusion. Well, 1. rename img,powervr.yaml => img,powervr-rogue.yaml 2. new file img,powervr-sgx.yaml to have at least a systematic approach here. >>> We have multiple >>> binding files for devices of the same generation, or single bindings >>> covering multiple generations. >>> >>> The important part is that every compatible is documented. It doesn't >>> really matter how or where. >> >> Yes, and that is why I would find it more convenient to have a single >> "img,powervr.yaml" for all variations unless it becomes filled with >> unrelated stuff (which isn't as far as I see). > > Again, hard disagree there. I am fine with that. I just advocate to follow the KISS principle. Same vendor, similar purpose, similar architecture => single bindings file as Andrew proposed. BR, Nikolaus
On Thu, Dec 07, 2023 at 11:33:53AM +0100, H. Nikolaus Schaller wrote: > Hi Maxime, > > > Am 07.12.2023 um 10:20 schrieb Maxime Ripard <mripard@kernel.org>: > > > > On Tue, Dec 05, 2023 at 02:50:08PM +0100, H. Nikolaus Schaller wrote: > >> Hi, > >> > >>> Am 05.12.2023 um 14:29 schrieb Maxime Ripard <mripard@kernel.org>: > >>> > >>> Hi, > >>> > >>> On Tue, Dec 05, 2023 at 09:18:58AM +0100, H. Nikolaus Schaller wrote: > >>>>> Am 05.12.2023 um 07:57 schrieb Maxime Ripard <mripard@kernel.org>: > >>>>> > >>>>> On Mon, Dec 04, 2023 at 12:22:36PM -0600, Andrew Davis wrote: > >>>>>> The Imagination PowerVR Series5 "SGX" GPU is part of several SoCs from > >>>>>> multiple vendors. Describe how the SGX GPU is integrated in these SoC, > >>>>>> including register space and interrupts. Clocks, reset, and power domain > >>>>>> information is SoC specific. > >>>>>> > >>>>>> Signed-off-by: Andrew Davis <afd@ti.com> > >>>>>> --- > >>>>>> .../devicetree/bindings/gpu/img,powervr.yaml | 69 +++++++++++++++++-- > >>>>>> 1 file changed, 63 insertions(+), 6 deletions(-) > >>>>> > >>>>> I think it would be best to have a separate file for this, img,sgx.yaml > >>>>> maybe? > >>>> > >>>> Why? > >>> > >>> Because it's more convenient? > >> > >> Is it? > > > > It's for a separate architecture, with a separate driver, maintained out > > of tree by a separate community, with a separate set of requirements as > > evidenced by the other thread. And that's all fine in itself, but > > there's very little reason to put these two bindings in the same file. > > > > We could also turn this around, why is it important that it's in the > > same file? > > Same vendor. And enough similarity in architectures, even a logical sequence > of development of versions (SGX = Version 5, Rogue = Version 6+) behind. > (SGX and Rogue seem to be just trade names for their architecture development). Again, none of that matters for *where* the binding is stored. > AFAIK bindings should describe hardware and not communities or drivers > or who is currently maintaining it. The latter can change, the first not. Bindings are supposed to describe hardware indeed. Nothing was ever said about where those bindings are supposed to be located. There's hundreds of other YAML bindings describing devices of the same vendors and different devices from the same generation. If anything it'll make it easier for you. I'm really not sure why it is controversial and you're fighting this so hard. Maxime
Hi Maxime, > Am 15.12.2023 um 14:33 schrieb Maxime Ripard <mripard@kernel.org>: > >>> >>> It's for a separate architecture, with a separate driver, maintained out >>> of tree by a separate community, with a separate set of requirements as >>> evidenced by the other thread. And that's all fine in itself, but >>> there's very little reason to put these two bindings in the same file. >>> >>> We could also turn this around, why is it important that it's in the >>> same file? >> >> Same vendor. And enough similarity in architectures, even a logical sequence >> of development of versions (SGX = Version 5, Rogue = Version 6+) behind. >> (SGX and Rogue seem to be just trade names for their architecture development). > > Again, none of that matters for *where* the binding is stored. So what then speaks against extending the existing bindings file as proposed here? > >> AFAIK bindings should describe hardware and not communities or drivers >> or who is currently maintaining it. The latter can change, the first not. > > Bindings are supposed to describe hardware indeed. Nothing was ever said > about where those bindings are supposed to be located. > > There's hundreds of other YAML bindings describing devices of the same > vendors and different devices from the same generation. Usually SoC seem to be split over multiple files by subsystem. Not by versions or generations. If the subsystems are similar enough they share the same bindings doc instead of having one for each generation duplicating a lot of code. Here is a comparable example that combines multiple vendors and generations: Documentation/devicetree/bindings/usb/generic-ehci.yaml > If anything it'll make it easier for you. I'm really not sure why it is > controversial and you're fighting this so hard. Well, you made it controversial by proposing to split what IMHO belongs together. I feel that the original patch is good enough for its purpose and follows some design pattern that can be deduced from other binding docs. BR, Nikolaus
On Mon, Dec 18, 2023 at 10:28:09AM +0100, H. Nikolaus Schaller wrote: > Hi Maxime, > > > Am 15.12.2023 um 14:33 schrieb Maxime Ripard <mripard@kernel.org>: > > > >>> > >>> It's for a separate architecture, with a separate driver, maintained out > >>> of tree by a separate community, with a separate set of requirements as > >>> evidenced by the other thread. And that's all fine in itself, but > >>> there's very little reason to put these two bindings in the same file. > >>> > >>> We could also turn this around, why is it important that it's in the > >>> same file? > >> > >> Same vendor. And enough similarity in architectures, even a logical sequence > >> of development of versions (SGX = Version 5, Rogue = Version 6+) behind. > >> (SGX and Rogue seem to be just trade names for their architecture development). > > > > Again, none of that matters for *where* the binding is stored. > > So what then speaks against extending the existing bindings file as proposed > here? I mean, apart from everything you quoted, then sure, nothing speaks against it. > >> AFAIK bindings should describe hardware and not communities or drivers > >> or who is currently maintaining it. The latter can change, the first not. > > > > Bindings are supposed to describe hardware indeed. Nothing was ever said > > about where those bindings are supposed to be located. > > > > There's hundreds of other YAML bindings describing devices of the same > > vendors and different devices from the same generation. > > Usually SoC seem to be split over multiple files by subsystem. Not by versions > or generations. If the subsystems are similar enough they share the same bindings > doc instead of having one for each generation duplicating a lot of code. > > Here is a comparable example that combines multiple vendors and generations: > > Documentation/devicetree/bindings/usb/generic-ehci.yaml EHCI is a single interface for USB2.0 controllers. It's a standard API, and is made of a single driver that requires minor modifications to deal with multiple devices. We're very far from the same situation here. > > If anything it'll make it easier for you. I'm really not sure why it is > > controversial and you're fighting this so hard. > > Well, you made it controversial by proposing to split what IMHO belongs together. No, reviews aren't controversial. The controversy started when you chose to oppose it while you could have just rolled with it. > I feel that the original patch is good enough for its purpose and follows > some design pattern that can be deduced from other binding docs. [citation needed] Maxime
> Am 18.12.2023 um 11:14 schrieb Maxime Ripard <mripard@kernel.org>: > > On Mon, Dec 18, 2023 at 10:28:09AM +0100, H. Nikolaus Schaller wrote: >> Hi Maxime, >> >>> Am 15.12.2023 um 14:33 schrieb Maxime Ripard <mripard@kernel.org>: >>> >>>>> >>>>> It's for a separate architecture, with a separate driver, maintained out >>>>> of tree by a separate community, with a separate set of requirements as >>>>> evidenced by the other thread. And that's all fine in itself, but >>>>> there's very little reason to put these two bindings in the same file. >>>>> >>>>> We could also turn this around, why is it important that it's in the >>>>> same file? >>>> >>>> Same vendor. And enough similarity in architectures, even a logical sequence >>>> of development of versions (SGX = Version 5, Rogue = Version 6+) behind. >>>> (SGX and Rogue seem to be just trade names for their architecture development). >>> >>> Again, none of that matters for *where* the binding is stored. >> >> So what then speaks against extending the existing bindings file as proposed >> here? > > I mean, apart from everything you quoted, then sure, nothing speaks > against it. > >>>> AFAIK bindings should describe hardware and not communities or drivers >>>> or who is currently maintaining it. The latter can change, the first not. >>> >>> Bindings are supposed to describe hardware indeed. Nothing was ever said >>> about where those bindings are supposed to be located. >>> >>> There's hundreds of other YAML bindings describing devices of the same >>> vendors and different devices from the same generation. >> >> Usually SoC seem to be split over multiple files by subsystem. Not by versions >> or generations. If the subsystems are similar enough they share the same bindings >> doc instead of having one for each generation duplicating a lot of code. >> >> Here is a comparable example that combines multiple vendors and generations: >> >> Documentation/devicetree/bindings/usb/generic-ehci.yaml > > EHCI is a single interface for USB2.0 controllers. It's a standard API, > and is made of a single driver that requires minor modifications to deal > with multiple devices. > > We're very far from the same situation here. How far are we really? And, it is the purpose of the driver to handle different cases. That there are currently two drivers is just a matter of history and not a necessity. > >>> If anything it'll make it easier for you. I'm really not sure why it is >>> controversial and you're fighting this so hard. >> >> Well, you made it controversial by proposing to split what IMHO belongs together. > > No, reviews aren't controversial. > The controversy started when you chose > to oppose it while you could have just rolled with it. Well, you asked "I think it would be best to have a separate file for this, img,sgx.yaml maybe?" and "Because it's more convenient?" I understood that as an invitation for discussing the pros and cons and working out the most convenient solution. And that involves playing the devil's advocate which of course is controversial by principle. Now, IMHO all the pros and cons are on the table and the question is who makes a decision how to go. > >> I feel that the original patch is good enough for its purpose and follows >> some design pattern that can be deduced from other binding docs. > > [citation needed] Joke: Documentation/devicetree/bindings/* - I am not aware of a formal analysis of course. But see my example for ehci. It follows the pattern I mean. If clocks, regs, interrupts, resets, and more properties are (almost) the same, then group them and just differentiate by different compatible strings. If necessary use some - if: clauses. It is the task of drivers to handle the details. As my other (maybe more important) comment to this patch did indicate we IMHO can easily live with something like + - items: + - enum: + - ti,am62-gpu # IMG AXE GPU model/revision is fully discoverable + - ti,omap3430-gpu # sgx530 Rev 121 + - ti,omap3630-gpu # sgx530 Rev 125 + - ingenic,jz4780-gpu # sgx540 Rev 130 + - ti,omap4430-gpu # sgx540 Rev 120 + - allwinner,sun6i-a31-gpu # sgx544 MP2 Rev 115 + - ti,omap4470-gpu # sgx544 MP1 Rev 112 + - ti,omap5432-gpu # sgx544 MP2 Rev 105 + - ti,am5728-gpu # sgx544 MP2 Rev 116 + - ti,am6548-gpu # sgx544 MP1 Rev 117 And leave it to drivers using a table to deduce the generation and revision or read it out from the chip. And there can even be different drivers handling only a subset of the potential compatibles. Then the currently-out-of-tree driver for the sgx5 can be reworked in less than half an hour without loosing functionality. BR, Nikolaus
On 12/18/23 4:54 AM, H. Nikolaus Schaller wrote: > > >> Am 18.12.2023 um 11:14 schrieb Maxime Ripard <mripard@kernel.org>: >> >> On Mon, Dec 18, 2023 at 10:28:09AM +0100, H. Nikolaus Schaller wrote: >>> Hi Maxime, >>> >>>> Am 15.12.2023 um 14:33 schrieb Maxime Ripard <mripard@kernel.org>: >>>> >>>>>> >>>>>> It's for a separate architecture, with a separate driver, maintained out >>>>>> of tree by a separate community, with a separate set of requirements as >>>>>> evidenced by the other thread. And that's all fine in itself, but >>>>>> there's very little reason to put these two bindings in the same file. >>>>>> >>>>>> We could also turn this around, why is it important that it's in the >>>>>> same file? >>>>> >>>>> Same vendor. And enough similarity in architectures, even a logical sequence >>>>> of development of versions (SGX = Version 5, Rogue = Version 6+) behind. >>>>> (SGX and Rogue seem to be just trade names for their architecture development). >>>> >>>> Again, none of that matters for *where* the binding is stored. >>> >>> So what then speaks against extending the existing bindings file as proposed >>> here? >> >> I mean, apart from everything you quoted, then sure, nothing speaks >> against it. >> >>>>> AFAIK bindings should describe hardware and not communities or drivers >>>>> or who is currently maintaining it. The latter can change, the first not. >>>> >>>> Bindings are supposed to describe hardware indeed. Nothing was ever said >>>> about where those bindings are supposed to be located. >>>> >>>> There's hundreds of other YAML bindings describing devices of the same >>>> vendors and different devices from the same generation. >>> >>> Usually SoC seem to be split over multiple files by subsystem. Not by versions >>> or generations. If the subsystems are similar enough they share the same bindings >>> doc instead of having one for each generation duplicating a lot of code. >>> >>> Here is a comparable example that combines multiple vendors and generations: >>> >>> Documentation/devicetree/bindings/usb/generic-ehci.yaml >> >> EHCI is a single interface for USB2.0 controllers. It's a standard API, >> and is made of a single driver that requires minor modifications to deal >> with multiple devices. >> >> We're very far from the same situation here. > > How far are we really? And, it is the purpose of the driver to handle different cases. > > That there are currently two drivers is just a matter of history and not a necessity. > >> >>>> If anything it'll make it easier for you. I'm really not sure why it is >>>> controversial and you're fighting this so hard. >>> >>> Well, you made it controversial by proposing to split what IMHO belongs together. >> >> No, reviews aren't controversial. >> The controversy started when you chose >> to oppose it while you could have just rolled with it. > > Well, you asked > > "I think it would be best to have a separate file for this, img,sgx.yaml > maybe?" > > and > > "Because it's more convenient?" > > I understood that as an invitation for discussing the pros and cons and working out the > most convenient solution. And that involves playing the devil's advocate which of course > is controversial by principle. > > Now, IMHO all the pros and cons are on the table and the question is who makes a decision > how to go. > As much as I would land on the side of same file for both, the answer to this question is simple: the maintainer makes the decision :) So I'll respin with separate binding files. The hidden unaddressed issue here is that by making these bindings separate it implies they are not on equal footing (i.e. pre-series6 GPUs are not true "powervr" and so do not belong in img,powervr.yaml). So if no one objects I'd also like to do the rename of that file as suggested before and have: img,powervr-sgx.yaml img,powervr-rogue.yaml >> >>> I feel that the original patch is good enough for its purpose and follows >>> some design pattern that can be deduced from other binding docs. >> >> [citation needed] > > Joke: Documentation/devicetree/bindings/* - I am not aware of a formal analysis of course. > > But see my example for ehci. It follows the pattern I mean. If clocks, regs, interrupts, > resets, and more properties are (almost) the same, then group them and just differentiate > by different compatible strings. If necessary use some - if: clauses. > > It is the task of drivers to handle the details. > > As my other (maybe more important) comment to this patch did indicate we IMHO can easily > live with something like > > + - items: > + - enum: > + - ti,am62-gpu # IMG AXE GPU model/revision is fully discoverable > + - ti,omap3430-gpu # sgx530 Rev 121 > + - ti,omap3630-gpu # sgx530 Rev 125 > + - ingenic,jz4780-gpu # sgx540 Rev 130 > + - ti,omap4430-gpu # sgx540 Rev 120 > + - allwinner,sun6i-a31-gpu # sgx544 MP2 Rev 115 > + - ti,omap4470-gpu # sgx544 MP1 Rev 112 > + - ti,omap5432-gpu # sgx544 MP2 Rev 105 > + - ti,am5728-gpu # sgx544 MP2 Rev 116 > + - ti,am6548-gpu # sgx544 MP1 Rev 117 > While we could live with this, the "compatible" groupings makes life just a bit easier. This is true really for any DT compatible string and is not based on any technical reasoning. Andrew > And leave it to drivers using a table to deduce the generation and > revision or read it out from the chip. And there can even be different > drivers handling only a subset of the potential compatibles. > > Then the currently-out-of-tree driver for the sgx5 can be reworked in > less than half an hour without loosing functionality. > > BR, > Nikolaus >
On Mon, Dec 18, 2023 at 11:54:47AM +0100, H. Nikolaus Schaller wrote: > > > > Am 18.12.2023 um 11:14 schrieb Maxime Ripard <mripard@kernel.org>: > > > > On Mon, Dec 18, 2023 at 10:28:09AM +0100, H. Nikolaus Schaller wrote: > >> Hi Maxime, > >> > >>> Am 15.12.2023 um 14:33 schrieb Maxime Ripard <mripard@kernel.org>: > >>> > >>>>> > >>>>> It's for a separate architecture, with a separate driver, maintained out > >>>>> of tree by a separate community, with a separate set of requirements as > >>>>> evidenced by the other thread. And that's all fine in itself, but > >>>>> there's very little reason to put these two bindings in the same file. > >>>>> > >>>>> We could also turn this around, why is it important that it's in the > >>>>> same file? > >>>> > >>>> Same vendor. And enough similarity in architectures, even a logical sequence > >>>> of development of versions (SGX = Version 5, Rogue = Version 6+) behind. > >>>> (SGX and Rogue seem to be just trade names for their architecture development). > >>> > >>> Again, none of that matters for *where* the binding is stored. > >> > >> So what then speaks against extending the existing bindings file as proposed > >> here? > > > > I mean, apart from everything you quoted, then sure, nothing speaks > > against it. > > > >>>> AFAIK bindings should describe hardware and not communities or drivers > >>>> or who is currently maintaining it. The latter can change, the first not. > >>> > >>> Bindings are supposed to describe hardware indeed. Nothing was ever said > >>> about where those bindings are supposed to be located. > >>> > >>> There's hundreds of other YAML bindings describing devices of the same > >>> vendors and different devices from the same generation. > >> > >> Usually SoC seem to be split over multiple files by subsystem. Not by versions > >> or generations. If the subsystems are similar enough they share the same bindings > >> doc instead of having one for each generation duplicating a lot of code. > >> > >> Here is a comparable example that combines multiple vendors and generations: > >> > >> Documentation/devicetree/bindings/usb/generic-ehci.yaml > > > > EHCI is a single interface for USB2.0 controllers. It's a standard API, > > and is made of a single driver that requires minor modifications to deal > > with multiple devices. > > > > We're very far from the same situation here. > > How far are we really? There's one binding for one driver. You suggest one binding for two drivers. > And, it is the purpose of the driver to handle different cases. > > That there are currently two drivers is just a matter of history and > not a necessity. Cool, so what you're saying is that your plan is to support those GPUs upstream in the imagination driver? I guess we should delay this patch until we see that series then. > >>> If anything it'll make it easier for you. I'm really not sure why it is > >>> controversial and you're fighting this so hard. > >> > >> Well, you made it controversial by proposing to split what IMHO belongs together. > > > > No, reviews aren't controversial. > > The controversy started when you chose > > to oppose it while you could have just rolled with it. > > Well, you asked > > "I think it would be best to have a separate file for this, img,sgx.yaml > maybe?" > > and > > "Because it's more convenient?" > > I understood that as an invitation for discussing the pros and cons > and working out the most convenient solution. And that involves > playing the devil's advocate which of course is controversial by > principle. > > Now, IMHO all the pros and cons are on the table and the question is > who makes a decision how to go. You haven't listed any pro so far, you're claiming that the one I raise are irrelevant. > >> I feel that the original patch is good enough for its purpose and follows > >> some design pattern that can be deduced from other binding docs. > > > > [citation needed] > > Joke: Documentation/devicetree/bindings/* - I am not aware of a formal analysis of course. > > But see my example for ehci. It follows the pattern I mean. If clocks, regs, interrupts, > resets, and more properties are (almost) the same, then group them and just differentiate > by different compatible strings. Again, EHCI is not something you can compare to. It's a binding to support a standard interface. You don't have the same interface and your driver will need to be different. And more importantly: bindings are meant to describe the hardware itself. How it's supported in Linux is irrelevant to the discussion. So, we could have: 10 drivers for the same binding, or 1 driver for 10 bindings. The two notions are orthogonal. > If necessary use some - if: clauses. > > It is the task of drivers to handle the details. > > As my other (maybe more important) comment to this patch did indicate we IMHO can easily > live with something like > > + - items: > + - enum: > + - ti,am62-gpu # IMG AXE GPU model/revision is fully discoverable > + - ti,omap3430-gpu # sgx530 Rev 121 > + - ti,omap3630-gpu # sgx530 Rev 125 > + - ingenic,jz4780-gpu # sgx540 Rev 130 > + - ti,omap4430-gpu # sgx540 Rev 120 > + - allwinner,sun6i-a31-gpu # sgx544 MP2 Rev 115 > + - ti,omap4470-gpu # sgx544 MP1 Rev 112 > + - ti,omap5432-gpu # sgx544 MP2 Rev 105 > + - ti,am5728-gpu # sgx544 MP2 Rev 116 > + - ti,am6548-gpu # sgx544 MP1 Rev 117 > > And leave it to drivers using a table to deduce the generation and > revision or read it out from the chip. And there can even be different > drivers handling only a subset of the potential compatibles. > > Then the currently-out-of-tree driver for the sgx5 can be reworked in > less than half an hour without loosing functionality. Again, you're making it harder than it needs to be for no particular reason other than the potential file name clash that can be addressed. Maxime
On Tue, Dec 19, 2023 at 11:19:49AM -0600, Andrew Davis wrote: > On 12/18/23 4:54 AM, H. Nikolaus Schaller wrote: > > > > > > > Am 18.12.2023 um 11:14 schrieb Maxime Ripard <mripard@kernel.org>: > > > > > > On Mon, Dec 18, 2023 at 10:28:09AM +0100, H. Nikolaus Schaller wrote: > > > > Hi Maxime, > > > > > > > > > Am 15.12.2023 um 14:33 schrieb Maxime Ripard <mripard@kernel.org>: > > > > > > > > > > > > > > > > > > > It's for a separate architecture, with a separate driver, maintained out > > > > > > > of tree by a separate community, with a separate set of requirements as > > > > > > > evidenced by the other thread. And that's all fine in itself, but > > > > > > > there's very little reason to put these two bindings in the same file. > > > > > > > > > > > > > > We could also turn this around, why is it important that it's in the > > > > > > > same file? > > > > > > > > > > > > Same vendor. And enough similarity in architectures, even a logical sequence > > > > > > of development of versions (SGX = Version 5, Rogue = Version 6+) behind. > > > > > > (SGX and Rogue seem to be just trade names for their architecture development). > > > > > > > > > > Again, none of that matters for *where* the binding is stored. > > > > > > > > So what then speaks against extending the existing bindings file as proposed > > > > here? > > > > > > I mean, apart from everything you quoted, then sure, nothing speaks > > > against it. > > > > > > > > > AFAIK bindings should describe hardware and not communities or drivers > > > > > > or who is currently maintaining it. The latter can change, the first not. > > > > > > > > > > Bindings are supposed to describe hardware indeed. Nothing was ever said > > > > > about where those bindings are supposed to be located. > > > > > > > > > > There's hundreds of other YAML bindings describing devices of the same > > > > > vendors and different devices from the same generation. > > > > > > > > Usually SoC seem to be split over multiple files by subsystem. Not by versions > > > > or generations. If the subsystems are similar enough they share the same bindings > > > > doc instead of having one for each generation duplicating a lot of code. > > > > > > > > Here is a comparable example that combines multiple vendors and generations: > > > > > > > > Documentation/devicetree/bindings/usb/generic-ehci.yaml > > > > > > EHCI is a single interface for USB2.0 controllers. It's a standard API, > > > and is made of a single driver that requires minor modifications to deal > > > with multiple devices. > > > > > > We're very far from the same situation here. > > > > How far are we really? And, it is the purpose of the driver to handle different cases. > > > > That there are currently two drivers is just a matter of history and not a necessity. > > > > > > > > > > If anything it'll make it easier for you. I'm really not sure why it is > > > > > controversial and you're fighting this so hard. > > > > > > > > Well, you made it controversial by proposing to split what IMHO belongs together. > > > > > > No, reviews aren't controversial. > > > The controversy started when you chose > > > to oppose it while you could have just rolled with it. > > > > Well, you asked > > > > "I think it would be best to have a separate file for this, img,sgx.yaml > > maybe?" > > > > and > > > > "Because it's more convenient?" > > > > I understood that as an invitation for discussing the pros and cons and working out the > > most convenient solution. And that involves playing the devil's advocate which of course > > is controversial by principle. > > > > Now, IMHO all the pros and cons are on the table and the question is who makes a decision > > how to go. > > > > As much as I would land on the side of same file for both, the answer to this question > is simple: the maintainer makes the decision :) So I'll respin with separate binding files. > > The hidden unaddressed issue here is that by making these bindings separate it implies > they are not on equal footing (i.e. pre-series6 GPUs are not true "powervr" and so do not > belong in img,powervr.yaml). No, not really. As far as I'm concerned, the only unequal footing here is that one driver is in-tree and the other isn't, but this situation was handled nicely for Mali GPUs and lima that used to be in the same situation for example. The situation is simple, really: bindings are supposed to be backward compatible, period. If we ever make a change to that binding that isn't, you will be well within your right to complain because your driver is now broken. > So if no one objects I'd also like to do the rename of that > file as suggested before and have: > > img,powervr-sgx.yaml > img,powervr-rogue.yaml Sounds good to me. Maxime
> Am 21.12.2023 um 09:58 schrieb Maxime Ripard <mripard@kernel.org>: > > Cool, so what you're saying is that your plan is to support those GPUs > upstream in the imagination driver? Yes, I would like to see PowerVR Series 5 SGX supported upstream since there are still so many devices in the wild which could use it. The most advanced being the Pyra handheld gaming computer but there are omap4 based phones or other omap3/amm335x based devices. And the only reason the OpenPVRSGX group was founded (BTW not by me, I am just maintaining the code and running a mailing list because it was rejected to host it on vger.kernel.org), was to make that happen. From the GitHub description: This is about shaping existing GPL Linux kernel drivers for the PVR/SGX5 architecture so that they can become accepted into drivers/staging But nobody can currently tell if it can be integrated with the recently upstreamed Rogue driver (I wouldn't call that *the* imagination driver) or if it better stays a separate driver because the first would need touching closed user-space code and GPU firmware. And nobody knows who is capable and willing to work on it. It depends on access to (confidential) documentation and available time to make such a big task a rewarding project. And discussions like this one are not at all encouraging to even try. >> Now, IMHO all the pros and cons are on the table and the question is >> who makes a decision how to go. > > You haven't listed any pro so far, you're claiming that the one I raise > are irrelevant. I have listed some "pros" for "single file" but you apparently don't see them as such. I can't change that. The main argument is that a single file is simpler than two files duplicating parts, which are apparently the same (integration of PVR architectures into SoC doesn't differ very much: shared register block, DMA memory, clocks, resets etc.). Yours is that two files duplicating such common things is "more convenient". I just wonder for whom. But it seems as if the IMHO second best solution has already been chosen. So let it be. >> Then the currently-out-of-tree driver for the sgx5 can be reworked in >> less than half an hour without loosing functionality. > > Again, you're making it harder than it needs to be for no particular > reason other than the potential file name clash that can be addressed. What I want to avoid is a situation that upstream activities do not take the existing and working out-of-tree SGX driver into account and make porting (not even speaking of upstreaming) that driver more difficult than necessary and force device tree files to contain redundant information nobody will need and use. You can of course ignore experience and suggestions of people who have worked on an SGX driver for a while. But that is the reason why I participate in this discussion and raise my voice. Now, I am looking forward to a v2 of this patch. BR, Nikolaus
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@imgtec.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 + - const: img,powervr-sgx530 + - 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: - |
The Imagination PowerVR Series5 "SGX" GPU is part of several SoCs from multiple vendors. Describe how the SGX GPU is integrated in these SoC, including register space and interrupts. Clocks, reset, and power domain information is SoC specific. Signed-off-by: Andrew Davis <afd@ti.com> --- .../devicetree/bindings/gpu/img,powervr.yaml | 69 +++++++++++++++++-- 1 file changed, 63 insertions(+), 6 deletions(-)