From: Paul Cercueil <paul@crapouillou.net>
To: "H. Nikolaus Schaller" <hns@goldelico.com>
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: Sun, 03 May 2020 14:52:05 +0200 [thread overview]
Message-ID: <TEAR9Q.6HI5DFRO5U0I3@crapouillou.net> (raw)
In-Reply-To: <28138EC0-0FA5-4F97-B528-3442BF087C7A@goldelico.com>
Hi Nikolaus,
Le sam. 2 mai 2020 à 22:26, H. Nikolaus Schaller <hns@goldelico.com> a
écrit :
> 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.
As we said before, the number of clocks is a property of the GPU and
*not* its integration into the SoC.
So you would *not* have a number of clocks between 0 and 4. You get
either 0, or 4, depending on whether or not you have a wrapper.
>> 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.
If you add support for a new SoC, you'd still need to modify the
binding to add the compatible string. So the argument of "more work" is
moot.
-Paul
> 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
>
next prev parent reply other threads:[~2020-05-03 12:52 UTC|newest]
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
2020-05-03 12:52 ` Paul Cercueil [this message]
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=TEAR9Q.6HI5DFRO5U0I3@crapouillou.net \
--to=paul@crapouillou.net \
--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=hns@goldelico.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=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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).