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

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

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
  

* 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