Re: [PATCH v4 1/8] dt-bindings: add img,pvrsgx.yaml for Imagination GPUs

[H. Nikolaus Schaller]

Hi Rob,

> Am 18.12.2019 um 22:16 schrieb Rob Herring <robh@xxxxxxxxxx>:
> 
> On Tue, Dec 17, 2019 at 07:01:59PM +0100, H. Nikolaus Schaller wrote:
>> The Imagination PVR/SGX GPU is part of several SoC from
>> multiple vendors, e.g. TI OMAP, Ingenic JZ4780, Intel Poulsbo
>> and others.
>> 
>> With this binding, we describe how the SGX processor is
>> interfaced to the SoC (registers, interrupt etc.).
>> 
>> In most cases, Clock, Reset and power management is handled
>> by a parent node or elsewhere.
>> 
>> Tested by make dt_binding_check dtbs_check
> 
> I'm surprised that worked... (Not for long if it did).

AFAIR, it did not fail but emit thousands of warnings for other areas
so I may have missed some warning but there was no error that did stop
compilation.

> 
>> 
>> Signed-off-by: H. Nikolaus Schaller <hns@xxxxxxxxxxxxx>
>> ---
>> .../devicetree/bindings/gpu/img,pvrsgx.yaml   | 80 +++++++++++++++++++
>> 1 file changed, 80 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml
>> 
>> diff --git a/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml b/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml
>> new file mode 100644
>> index 000000000000..44799774e34d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml
>> @@ -0,0 +1,80 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/gpu/img,pvrsgx.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Imagination PVR/SGX GPU
>> +
>> +maintainers:
>> +  - H. Nikolaus Schaller <hns@xxxxxxxxxxxxx>
>> +
>> +description: |+
>> +  This binding describes the Imagination SGX5 series of 3D accelerators which
>> +  are found in several different SoC like TI OMAP, Sitara, Ingenic JZ4780,
>> +  Allwinner A83, and Intel Poulsbo and CedarView and more.
>> +
>> +  For an almost complete list see: https://en.wikipedia.org/wiki/PowerVR#Implementations
>> +  
>> +  Only the Imagination SGX530, SGX540 and SGX544 GPUs are currently covered by
>> +  this binding but the extension of the pattern is straightforward.
>> +  
>> +  The SGX node is usually a child node of some DT node belonging to the SoC
>> +  which handles clocks, reset and general address space mapping of the SGX
>> +  register area.
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +    # Example: BeagleBoard A/B/C, OpenPandora 600MHz
>> +      - ti,omap3-sgx530-121, img,sgx530-121, img,sgx530
> 
> Didn't I comment before this is not valid.

Not that I am aware of. Your comment was:

<<<
> +properties:
> +  compatible:
> +    enum:
> +    # BeagleBoard ABC, OpenPandora 600MHz

I'd expect compatibles to be per SoC, not per board.

> +      - ti,omap3-sgx530-121, img,sgx530-121, img,sgx530, img,sgx5

4 compatibles is probably a bit much. Are there not any version or 
feature registers that some of this could be detected? If there are, I'd 
assume the middle 2 strings could be dropped. If not, drop the last one 
and just match on the 3rd string. It's not a long list.
>>>

So I interpreted your comment is about the # comment focussing too much
on boards instead of SoC and about dropping img,sgx5

> You are defining the 
> compatible string is: 'ti,omap3-sgx530-121, img,sgx530-121, img,sgx530'

Yes.

> 
> You need:
> 
> compatible:
>  oneOf:
>    - description: BeagleBoard A/B/C, OpenPandora 600MHz
>      items:
>        - const: ti,omap3-sgx530-121
>        - const: img,sgx530-121
>        - const: img,sgx530
> 
> And so on for each of the rest.

Hm. As far as I understand YAML, it has multiple ways to define the same
structure but this manifold nature has its drawbacks.

If it is wrong I have to admit that I still do not understand how to write
correct schemes... Why has it been made so difficult for us developers? 

> 
>> +    # Example: BeagleBoard XM, GTA04, OpenPandora 1GHz
>> +      - ti,omap3-sgx530-125, img,sgx530-125, img,sgx530
>> +    # Example: BeagleBone Black
>> +      - ti,am3352-sgx530-125, img,sgx530-125, img,sgx530
> 
> These 2 could be combined using 'enum' for the first item. Basically, 
> you can group ones where the last 2 strings are the same.

Is this better readable?

IMHO, this would be a nice coding trick that would make this
even more unreadable and does not add any new information.

Generally, in my opinion, a bindings document should not only
satisfy the dtbs_check but should be human readable (and writeable!)
so that a DTS developer can still understand and ideally copy&paste
fragments. With such enum tricks and -items const: constructions
this IMHO becomes more difficult.

Therefore I think the linear list is better readable and
can be directly copied. If the test rig has problems with
that statement, I would suggest that the parser should be
modified to understand what we can easily write and read.

The same is for # comment vs. description: . The # comment
is for a human reader of this document to get a hint what
board the next line is intended for. It is not necessary
(and IMHO impossible) to automatically test it. So I do not
see any need to add a formal -description entry here.

> 
>> +    # Example: Pandaboard, Pandaboard ES
>> +      - ti,omap4-sgx540-120, img,sgx540-120, img,sgx540
>> +      - ti,omap4-sgx544-112, img,sgx544-112, img,sgx544
>> +    # Example: OMAP5 UEVM, Pyra Handheld
>> +      - ti,omap5-sgx544-116, img,sgx544-116, img,sgx544
>> +      - ti,dra7-sgx544-116, img,sgx544-116, img,sgx544
>> +    # Example: CI20
>> +      - ingenic,jz4780-sgx540-120, img,sgx540-120, img,sgx540
>> +    # the following entries are not validated with real hardware
> 
> What am I supposed to do with that? You're just defining some strings. 
> If you're not sure they are okay, then don't define them.

I have a broader audience in mind than 'make dtbs_check'. IMHO, this
document should help future developers to get an idea what to write for
those SoC. Yes, it might not be 100% correct, but it is easier to correct
a 99% correct definition than inventing it from scratch after dropping
partially correct information I already have.

For example, we know from data sheets and public information that the
allwinner a83 contains a sgx544 but it is not sure if it is the -116
variant. Or, the poulsbo has a GPU called gma500 which is an sgx535. It
is only that nobody has verified this so far by real hardware.

If we leave it out and someone starts to adapt this to the poulsbo
in let's say 2 years from now, that someone has to be lucky to find
our discussion here and take that as a hint.

I am not sure how you are developing contributions, but my first
place to start with is the existing code and not the archives of
LKML.

That is my motivation adding these records right here even if they
are not verified.

> 
>> +    # more TI SoC
>> +      - ti,am3517-sgx530-125, img,sgx530-125, img,sgx530
>> +      - ti,am4-sgx530-125, img,sgx530-125, img,sgx530
>> +      - ti,ti81xx-sgx530-125, img,sgx530-125, img,sgx530
>> +    # Example: Banana-Pi-M3 (Allwinner A83T)
>> +      - allwinner,sun8i-a83t-sgx544-116, img,sgx544-116, img,sgx544
>> +    # Example: Atom Z5xx
>> +      - intel,poulsbo-gma500-sgx535, img,sgx535-116, img,sgx535
>> +    # Example: Atom Z24xx
>> +      - intel,medfield-gma-sgx540, img,sgx540-116, img,sgx540
>> +    # Example: Atom N2600, D2500
>> +      - intel,cedarview-gma3600-sgx545, img,sgx545-116, img,sgx545
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |+
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +
>> +    gpu@fe00 {
>> +      compatible = "ti,omap-omap5-sgx544-116", "img,sgx544-116", "img,sgx544", "img,sgx5";
> 
> Doesn't match the schema.

Ah, thanks. I missed that when reviewing the DTS patch set.
Queued for a v5.

> 
>> +      reg = <0xfe00 0x200>;
>> +      interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
>> +    };
>> +
>> +...
>> -- 
>> 2.23.0
>> 

BR and thanks,
Nikolaus