Linux-OMAP Archive on lore.kernel.org
 help / color / Atom feed
From: "H. Nikolaus Schaller" <hns@goldelico.com>
To: Paul Cercueil <paul@crapouillou.net>
Cc: "David Airlie" <airlied@linux.ie>,
	"Daniel Vetter" <daniel@ffwll.ch>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Mark Rutland" <mark.rutland@arm.com>,
	"Benoît Cousson" <bcousson@baylibre.com>,
	"Tony Lindgren" <tony@atomide.com>,
	"Ralf Baechle" <ralf@linux-mips.org>,
	"Paul Burton" <paulburton@kernel.org>,
	"James Hogan" <jhogan@kernel.org>,
	"Kukjin Kim" <kgene@kernel.org>,
	"Krzysztof Kozlowski" <krzk@kernel.org>,
	"Maxime Ripard" <mripard@kernel.org>,
	"Chen-Yu Tsai" <wens@csie.org>,
	"Thomas Bogendoerfer" <tsbogend@alpha.franken.de>,
	"Jonathan Bakker" <xc-racer2@live.ca>,
	"Philipp Rossak" <embed3d@gmail.com>,
	dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org,
	openpvrsgx-devgroup@letux.org, letux-kernel@openphoenux.org,
	kernel@pyra-handheld.com, linux-mips@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-samsung-soc@vger.kernel.org
Subject: Re: [PATCH v7 01/12] dt-bindings: add img,pvrsgx.yaml for Imagination GPUs
Date: Sat, 2 May 2020 22:26:29 +0200
Message-ID: <28138EC0-0FA5-4F97-B528-3442BF087C7A@goldelico.com> (raw)
In-Reply-To: <NMCE9Q.LWG45P20NBVJ@crapouillou.net>

Hi Paul,

> Am 26.04.2020 um 15:11 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> Hi Nikolaus,
> 
> Le ven. 24 avril 2020 à 22:34, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>> The Imagination PVR/SGX GPU is part of several SoC from
>> multiple vendors, e.g. TI OMAP, Ingenic JZ4780, Intel Poulsbo,
>> Allwinner A83 and others.
>> With this binding, we describe how the SGX processor is
>> interfaced to the SoC (registers and interrupt).
>> The interface also consists of clocks, reset, power but
>> information from data sheets is vague and some SoC integrators
>> (TI) deciced to use a PRCM wrapper (ti,sysc) which does
>> all clock, reset and power-management through registers
>> outside of the sgx register block.
>> Therefore all these properties are optional.
>> Tested by make dt_binding_check
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> .../devicetree/bindings/gpu/img,pvrsgx.yaml   | 150 ++++++++++++++++++
>> 1 file changed, 150 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..33a9c4c6e784
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml
>> @@ -0,0 +1,150 @@
>> +# 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@goldelico.com>
>> +
>> +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 extensive list see: https://en.wikipedia.org/wiki/PowerVR#Implementations
>> +
>> +  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. If not, an optional clock can be specified here.
>> +
>> +properties:
>> +  $nodename:
>> +    pattern: '^gpu@[a-f0-9]+$'
>> +  compatible:
>> +    oneOf:
>> +      - description: SGX530-121 based SoC
>> +        items:
>> +          - enum:
>> +            - ti,omap3-sgx530-121 # BeagleBoard A/B/C, OpenPandora 600MHz and similar
>> +          - const: img,sgx530-121
>> +          - const: img,sgx530
>> +
>> +      - description: SGX530-125 based SoC
>> +        items:
>> +          - enum:
>> +            - ti,am3352-sgx530-125 # BeagleBone Black
>> +            - ti,am3517-sgx530-125
>> +            - ti,am4-sgx530-125
>> +            - ti,omap3-sgx530-125 # BeagleBoard XM, GTA04, OpenPandora 1GHz and similar
>> +            - ti,ti81xx-sgx530-125
>> +          - const: ti,omap3-sgx530-125
>> +          - const: img,sgx530-125
>> +          - const: img,sgx530
>> +
>> +      - description: SGX535-116 based SoC
>> +        items:
>> +          - const: intel,poulsbo-gma500-sgx535 # Atom Z5xx
>> +          - const: img,sgx535-116
>> +          - const: img,sgx535
>> +
>> +      - description: SGX540-116 based SoC
>> +        items:
>> +          - const: intel,medfield-gma-sgx540 # Atom Z24xx
>> +          - const: img,sgx540-116
>> +          - const: img,sgx540
>> +
>> +      - description: SGX540-120 based SoC
>> +        items:
>> +          - enum:
>> +            - samsung,s5pv210-sgx540-120
>> +            - ti,omap4-sgx540-120 # Pandaboard, Pandaboard ES and similar
>> +          - const: img,sgx540-120
>> +          - const: img,sgx540
>> +
>> +      - description: SGX540-130 based SoC
>> +        items:
>> +          - enum:
>> +            - ingenic,jz4780-sgx540-130 # CI20
>> +          - const: img,sgx540-130
>> +          - const: img,sgx540
>> +
>> +      - description: SGX544-112 based SoC
>> +        items:
>> +          - const: ti,omap4470-sgx544-112
>> +          - const: img,sgx544-112
>> +          - const: img,sgx544
>> +
>> +      - description: SGX544-115 based SoC
>> +        items:
>> +          - enum:
>> +            - allwinner,sun8i-a31-sgx544-115
>> +            - allwinner,sun8i-a31s-sgx544-115
>> +            - allwinner,sun8i-a83t-sgx544-115 # Banana-Pi-M3 (Allwinner A83T) and similar
>> +          - const: img,sgx544-115
>> +          - const: img,sgx544
>> +
>> +      - description: SGX544-116 based SoC
>> +        items:
>> +          - enum:
>> +            - ti,dra7-sgx544-116 # DRA7
>> +            - ti,omap5-sgx544-116 # OMAP5 UEVM, Pyra Handheld and similar
>> +          - const: img,sgx544-116
>> +          - const: img,sgx544
>> +
>> +      - description: SGX545 based SoC
>> +        items:
>> +          - const: intel,cedarview-gma3600-sgx545 # Atom N2600, D2500
>> +          - const: img,sgx545-116
>> +          - const: img,sgx545
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  interrupt-names:
>> +    maxItems: 1
>> +    items:
>> +      - const: sgx
>> +
>> +  clocks:
>> +    maxItems: 4
>> +
>> +  clock-names:
>> +    maxItems: 4
>> +    items:
>> +      - const: core
>> +      - const: sys
>> +      - const: mem
>> +      - const: hyd
>> +
>> +  sgx-supply: true
>> +
>> +  power-domains:
>> +    maxItems: 1
>> +
>> +  resets:
>> +    maxItems: 1
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
> 
> By not making 'clocks' required you make it possible to create broken bindings; according to your schema, a GPU node without a 'clocks' for the JZ4780 would be perfectly valid.

Yes. But it will never pass a test with real hardware. So it can't be omitted anyways.

On a more general thought, this argument holds for any optional property. So it is not specific to clocks. Since the reg address values are also never specified you can still create broken bindings. Or by connecting the wrong clock. So the ways to create broken bindings are numerous.

I also assume that SGX integrators are not beginners and do you think they need to find out through a make dt_binding_check dtbs_check that they should define a clock? based on *assumptions* we do without having access to all systems?

IMHO the bindings documentation is a documentation. So it needs to be helpful but not perfect. Formalizing all corner cases in a bindings document (just because we can since .yaml was introduced) is IMHO overkill.

In times before the introduction of more formal .yaml I think we would not even have considered this for a comment in the bindings.txt.

> It's possible to forbid the presence of the 'clocks' property on some implementations, and require it on others.

To be precise we have to specify the exact number of clocks (between 0 and 4) for every architecture.

This also contradicts my dream to get rid of the architecture specific components in the long run. My dream (because I can't tell how it can be done) is that we can one day develop something which just needs compatible = img,530 or imp,540 or img,544. Then we can't make the number clocks depend on the implementation any more.

> See how it's done for instance on Documentation/devicetree/bindings/serial/samsung_uart.yaml.

Yes I know the design pattern, but I wonder if such a move makes the whole thing even less maintainable.

Assume we have finished DTS for some SoC. Then these DTS have been tested on real hardware and are working. Clocks are there where needed and missing where not. We may now forbid or not forbid them for some implementations in the bindings.yaml but the result of dtbs_check won't change! Because they are tested and working and the bindings.yaml has been adapted to the result. So we have just duplicated something for no practical benefit.

Next, assume there is coming support for more and more new SoC. Then, developers not only have to figure out which clocks they need in the DTS but they also have to add a patch to the implementation specific part of the bindings.yaml to clearly define exactly the same what they already have written into their .dts (the clocks are either there for the of_node or they are not). So again the rules are for no benefit, since a new SoC is introduced exactly once. And tested if it works. And if it is there, it will stay as it is. It is just work for maintainers to review that patch as well.

It boils down to the question if we need to formalize the rule how a working DTS was derived. Or just have a working DTS and not formalize everything.

So IMHO carrying along such a detail (forbid clocks on some architectures) is nice to have (and fun to learn the .yaml thing) but not of benefit for anyone. Not for the DTS developer nor for the maintainers nor for the users of a Linux kernel. "Keep it simple" is always a good rule for maintainability.

In summary I don't see a good reason to follow this in v8. But you could add it by a separate patch later if the DTS have been reviewed and agreed.

BR and thanks,
Nikolaus


  reply index

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-24 20:34 [PATCH v7 00/12] ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more) H. Nikolaus Schaller
2020-04-24 20:34 ` [PATCH v7 01/12] dt-bindings: add img,pvrsgx.yaml for Imagination GPUs H. Nikolaus Schaller
2020-04-24 20:43   ` H. Nikolaus Schaller
2020-04-26 13:11   ` Paul Cercueil
2020-05-02 20:26     ` H. Nikolaus Schaller [this message]
2020-05-03 12:52       ` Paul Cercueil
2020-05-03 13:31         ` H. Nikolaus Schaller
2020-05-03 14:18           ` Paul Cercueil
2020-05-03 15:01             ` Tony Lindgren
2020-05-15  7:58               ` H. Nikolaus Schaller
2020-05-03 16:41             ` H. Nikolaus Schaller
2020-05-15  7:18               ` H. Nikolaus Schaller
2020-04-26 19:36   ` Philipp Rossak
2020-04-26 20:59     ` H. Nikolaus Schaller
2020-05-05 15:53   ` Rob Herring
2020-05-15  7:59     ` H. Nikolaus Schaller
2020-04-24 20:34 ` [PATCH v7 02/12] ARM: DTS: am33xx: add sgx gpu child node H. Nikolaus Schaller
2020-04-24 20:34 ` [PATCH v7 03/12] ARM: DTS: am3517: " H. Nikolaus Schaller
2020-04-24 20:34 ` [PATCH v7 04/12] ARM: DTS: omap34xx: " H. Nikolaus Schaller
2020-04-24 20:34 ` [PATCH v7 05/12] ARM: DTS: omap36xx: " H. Nikolaus Schaller
2020-04-24 20:34 ` [PATCH v7 06/12] ARM: DTS: omap4: " H. Nikolaus Schaller
2020-04-26 12:50   ` Paul Cercueil
2020-04-28  7:59     ` H. Nikolaus Schaller
2020-04-24 20:34 ` [PATCH v7 07/12] ARM: DTS: omap5: " H. Nikolaus Schaller
2020-04-24 20:34 ` [PATCH v7 08/12] arm: dts: s5pv210: Add node for SGX 540 H. Nikolaus Schaller
2020-04-26 12:56   ` Paul Cercueil
2020-04-26 14:57     ` Jonathan Bakker
2020-04-27 15:46       ` Krzysztof Kozlowski
2020-04-28 21:39         ` Jonathan Bakker
2020-04-28 22:58           ` Jonathan Bakker
2020-04-29  8:47             ` Maxime Ripard
2020-04-29 12:26             ` Paul Cercueil
2020-04-24 20:34 ` [PATCH v7 09/12] ARM: dts: sun6i: a31: add sgx gpu child node H. Nikolaus Schaller
2020-04-26 12:53   ` Paul Cercueil
2020-04-26 19:31     ` Philipp Rossak
2020-04-24 20:34 ` [PATCH v7 10/12] ARM: dts: sun6i: a31s: " H. Nikolaus Schaller
2020-04-24 20:34 ` [PATCH v7 11/12] ARM: dts: sun8i: a83t: " H. Nikolaus Schaller
2020-04-24 20:34 ` [PATCH v7 12/12] MIPS: DTS: jz4780: add sgx gpu node H. Nikolaus Schaller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=28138EC0-0FA5-4F97-B528-3442BF087C7A@goldelico.com \
    --to=hns@goldelico.com \
    --cc=airlied@linux.ie \
    --cc=bcousson@baylibre.com \
    --cc=daniel@ffwll.ch \
    --cc=devicetree@vger.kernel.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=embed3d@gmail.com \
    --cc=jhogan@kernel.org \
    --cc=kernel@pyra-handheld.com \
    --cc=kgene@kernel.org \
    --cc=krzk@kernel.org \
    --cc=letux-kernel@openphoenux.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mips@vger.kernel.org \
    --cc=linux-omap@vger.kernel.org \
    --cc=linux-samsung-soc@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mripard@kernel.org \
    --cc=openpvrsgx-devgroup@letux.org \
    --cc=paul@crapouillou.net \
    --cc=paulburton@kernel.org \
    --cc=ralf@linux-mips.org \
    --cc=robh+dt@kernel.org \
    --cc=tony@atomide.com \
    --cc=tsbogend@alpha.franken.de \
    --cc=wens@csie.org \
    --cc=xc-racer2@live.ca \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-OMAP Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-omap/0 linux-omap/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-omap linux-omap/ https://lore.kernel.org/linux-omap \
		linux-omap@vger.kernel.org
	public-inbox-index linux-omap

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-omap


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git