From: Maxime Ripard <maxime@cerno.tech>
To: "H. Nikolaus Schaller" <hns@goldelico.com>
Cc: "Neil Armstrong" <narmstrong@baylibre.com>,
"Mark Rutland" <mark.rutland@arm.com>,
"David Airlie" <airlied@linux.ie>,
"James Hogan" <jhogan@kernel.org>,
dri-devel@lists.freedesktop.org, linux-mips@vger.kernel.org,
"Paul Cercueil" <paul@crapouillou.net>,
linux-samsung-soc@vger.kernel.org, letux-kernel@openphoenux.org,
"Paul Burton" <paulburton@kernel.org>,
"Krzysztof Kozlowski" <krzk@kernel.org>,
"Tony Lindgren" <tony@atomide.com>,
"Chen-Yu Tsai" <wens@csie.org>, "Kukjin Kim" <kgene@kernel.org>,
devicetree@vger.kernel.org,
"Benoît Cousson" <bcousson@baylibre.com>,
"Rob Herring" <robh+dt@kernel.org>,
linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
"Thomas Bogendoerfer" <tsbogend@alpha.franken.de>,
"Philipp Rossak" <embed3d@gmail.com>,
openpvrsgx-devgroup@letux.org, linux-kernel@vger.kernel.org,
"Ralf Baechle" <ralf@linux-mips.org>,
"Daniel Vetter" <daniel@ffwll.ch>,
kernel@pyra-handheld.com
Subject: Re: [PATCH v6 01/12] dt-bindings: add img, pvrsgx.yaml for Imagination GPUs
Date: Wed, 15 Apr 2020 18:21:51 +0200 [thread overview]
Message-ID: <20200415162151.rwym4ioqz27migfn@gilmour.lan> (raw)
In-Reply-To: <DC0A2DE2-3D77-46F8-8DE1-55050FDACC9B@goldelico.com>
[-- Attachment #1: Type: text/plain, Size: 4807 bytes --]
On Wed, Apr 15, 2020 at 05:09:45PM +0200, H. Nikolaus Schaller wrote:
> Hi Maxime,
>
> > Am 15.04.2020 um 16:21 schrieb Maxime Ripard <maxime@cerno.tech>:
> >
> >>
> >> Well we could add clocks and resets as optional but that would
> >> allow to wrongly define omap.
> >>
> >> Or delegate them to a parent "simple-pm-bus" node.
> >>
> >> I have to study that material more to understand what you seem
> >> to expect.
> >
> > The thing is, once that binding is in, it has to be backward
> > compatible. So every thing that you leave out is something that you'll
> > need to support in the driver eventually.
>
> >
> > If you don't want it to be a complete nightmare, you'll want to figure
> > out as much as possible on how the GPU is integrated and make a
> > binding out of that.
>
> Hm. Yes. We know that there likely are clocks and maybe reset
> but for some SoC this seems to be undocumented and the reset
> line the VHDL of the sgx gpu provides may be permanently tied
> to "inactive".
>
> So if clocks are optional and not provided, a driver simply can assume
> they are enabled somewhere else and does not have to care about. If
> they are specified, the driver can enable/disable them.
Except that at the hardware level, the clock is always going to be
there. You can't control it, but it's there.
> > If OMAP is too much of a pain, you can also make
> > a separate binding for it, and a generic one for the rest of us.
>
> No, omap isn't any pain at all.
>
> The pain is that some other SoC are most easily defined by clocks in
> the gpu node which the omap doesn't need to explicitly specify.
>
> I would expect a much bigger nightmare if we split this into two
> bindings variants.
>
> > I'd say that it's pretty unlikely that the clocks, interrupts (and
> > even regulators) are optional. It might be fixed on some SoCs, but
> > that's up to the DT to express that using fixed clocks / regulators,
> > not the GPU binding itself.
>
> omap already has these defined them not to be part of the GPU binding.
> The reason seems to be that this needs special clock gating control
> especially for idle states which is beyond simple clock-enable.
>
> This sysc target-module@56000000 node is already merged and therefore
> we are only adding the gpu child node. Without defining clocks.
>
> For example:
>
> sgx_module: target-module@56000000 {
> compatible = "ti,sysc-omap4", "ti,sysc";
> reg = <0x5600fe00 0x4>,
> <0x5600fe10 0x4>;
> reg-names = "rev", "sysc";
> ti,sysc-midle = <SYSC_IDLE_FORCE>,
> <SYSC_IDLE_NO>,
> <SYSC_IDLE_SMART>;
> ti,sysc-sidle = <SYSC_IDLE_FORCE>,
> <SYSC_IDLE_NO>,
> <SYSC_IDLE_SMART>;
> clocks = <&gpu_clkctrl OMAP5_GPU_CLKCTRL 0>;
> clock-names = "fck";
> #address-cells = <1>;
> #size-cells = <1>;
> ranges = <0 0x56000000 0x2000000>;
>
> gpu: gpu@0 {
> compatible = "ti,omap5-sgx544-116", "img,sgx544-116", "img,sgx544";
> reg = <0x0 0x10000>;
> interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
> };
> };
>
> The jz4780 example will like this:
>
> gpu: gpu@13040000 {
> compatible = "ingenic,jz4780-sgx540-130", "img,sgx540-130", "img,sgx540";
> reg = <0x13040000 0x4000>;
>
> clocks = <&cgu JZ4780_CLK_GPU>;
> clock-names = "gpu";
>
> interrupt-parent = <&intc>;
> interrupts = <63>;
> };
>
> So the question is which one is "generic for the rest of us"?
I'd say the latter.
> And how can we make a single binding for the sgx. Not one for each
> special SoC variant that may exist.
>
> IMHO the best answer is to make clocks an optional property.
> Or if we do not want to define them explicitly, we use
> additionalProperties: true.
If your clock is optional, then you define it but don't mandate
it. Not documenting it will only result in a mess where everyone will
put some clock into it, possibly with different semantics each and
every time.
> An alternative could be to use a simple-pm-bus like:
>
> sgx_module: sgx_module@13040000 {
> compatible = "simple-pm-bus";
>
> clocks = <&cgu JZ4780_CLK_GPU>;
> clock-names = "gpu";
>
> #address-cells = <1>;
> #size-cells = <1>;
> ranges = <0 0x13040000 0x10000>;
>
> gpu: gpu@0 {
> compatible = "ingenic,jz4780-sgx540-130", "img,sgx540-130", "img,sgx540";
> reg = <0x0 0x4000>;
>
> interrupt-parent = <&intc>;
> interrupts = <63>;
> };
> };
>
> This gets rid of any clock, reset and pm definitions for the sgx bindings.
> But how this is done is outside this sgx bindings.
>
> With such a scheme, the binding I propose here would be complete and fully
> generic. We can even add additionalProperties: false.
This has nothing to do with the binding being complete. And if you use
a binding like this one, you'll be severely limited when you'll want
to implement things like DVFS.
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
next prev parent reply other threads:[~2020-04-15 16:22 UTC|newest]
Thread overview: 68+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-15 8:35 [PATCH v6 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-15 8:35 ` [PATCH v6 01/12] dt-bindings: add img,pvrsgx.yaml for Imagination GPUs H. Nikolaus Schaller
2020-04-15 10:12 ` Maxime Ripard
2020-04-15 12:43 ` H. Nikolaus Schaller
2020-04-15 12:54 ` [PATCH v6 01/12] dt-bindings: add img, pvrsgx.yaml " Neil Armstrong
2020-04-15 13:17 ` H. Nikolaus Schaller
2020-04-15 14:21 ` Maxime Ripard
2020-04-15 15:09 ` H. Nikolaus Schaller
2020-04-15 16:21 ` Maxime Ripard [this message]
2020-04-15 16:42 ` H. Nikolaus Schaller
2020-04-15 17:13 ` Tony Lindgren
2020-04-17 10:25 ` Maxime Ripard
2020-04-17 12:15 ` H. Nikolaus Schaller
2020-04-18 23:02 ` Philipp Rossak
2020-04-20 8:04 ` Maxime Ripard
2020-04-16 20:41 ` [PATCH v6 01/12] dt-bindings: add img,pvrsgx.yaml " Rob Herring
[not found] ` <C7C58E41-99CB-49F6-934E-68FA458CB8B1@goldelico.com>
2020-04-21 19:02 ` [PATCH v6 01/12] dt-bindings: add img, pvrsgx.yaml " Rob Herring
2020-04-15 8:35 ` [PATCH v6 02/12] ARM: DTS: am33xx: add sgx gpu child node H. Nikolaus Schaller
2020-04-15 8:35 ` [PATCH v6 03/12] ARM: DTS: am3517: " H. Nikolaus Schaller
2020-04-15 8:35 ` [PATCH v6 04/12] ARM: DTS: omap34xx: " H. Nikolaus Schaller
2020-04-15 8:35 ` [PATCH v6 05/12] ARM: DTS: omap36xx: " H. Nikolaus Schaller
2020-04-15 8:35 ` [PATCH v6 06/12] ARM: DTS: omap4: " H. Nikolaus Schaller
2020-04-15 8:35 ` [PATCH v6 07/12] ARM: DTS: omap5: " H. Nikolaus Schaller
2020-04-15 11:38 ` Krzysztof Kozlowski
2020-04-15 11:46 ` H. Nikolaus Schaller
2020-04-15 13:47 ` Krzysztof Kozlowski
2020-04-15 14:07 ` H. Nikolaus Schaller
2020-04-15 8:35 ` [PATCH v6 08/12] arm: dts: s5pv210: Add G3D node H. Nikolaus Schaller
2020-04-15 9:15 ` Sergei Shtylyov
2020-04-15 9:26 ` H. Nikolaus Schaller
2020-04-15 11:49 ` Krzysztof Kozlowski
2020-04-15 12:50 ` H. Nikolaus Schaller
2020-04-15 13:44 ` Krzysztof Kozlowski
2020-04-15 18:17 ` Jonathan Bakker
2020-04-16 8:50 ` Krzysztof Kozlowski
2020-04-17 12:15 ` H. Nikolaus Schaller
2020-04-22 5:56 ` kbuild test robot
2020-04-15 8:35 ` [PATCH v6 09/12] ARM: dts: sun6i: a31: add sgx gpu child node H. Nikolaus Schaller
2020-04-15 8:35 ` [PATCH v6 10/12] ARM: dts: sun6i: a31s: " H. Nikolaus Schaller
2020-04-15 8:35 ` [PATCH v6 11/12] ARM: dts: sun8i: a83t: " H. Nikolaus Schaller
2020-04-15 8:35 ` [PATCH v6 12/12] MIPS: DTS: jz4780: add sgx gpu node H. Nikolaus Schaller
2020-04-15 10:10 ` [PATCH v6 00/12] ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more) Maxime Ripard
2020-04-15 12:41 ` H. Nikolaus Schaller
2020-04-15 12:46 ` Daniel Vetter
2020-04-15 13:02 ` Maxime Ripard
2020-04-15 13:04 ` H. Nikolaus Schaller
2020-04-17 12:09 ` Philipp Rossak
2020-04-20 7:38 ` Maxime Ripard
2020-04-21 9:57 ` Philipp Rossak
2020-04-21 11:21 ` Maxime Ripard
2020-04-21 14:15 ` Tony Lindgren
2020-04-21 17:29 ` H. Nikolaus Schaller
2020-04-21 17:39 ` Tony Lindgren
2020-04-21 17:46 ` Tony Lindgren
2020-04-22 6:58 ` Maxime Ripard
2020-04-22 7:10 ` H. Nikolaus Schaller
2020-04-22 15:13 ` Maxime Ripard
2020-04-22 16:09 ` H. Nikolaus Schaller
[not found] ` <MC879Q.XY9S0U9R35681@crapouillou.net>
2020-04-22 17:23 ` H. Nikolaus Schaller
2020-04-22 19:00 ` Philipp Rossak
2020-04-22 19:33 ` Tony Lindgren
2020-04-22 21:12 ` H. Nikolaus Schaller
2020-04-23 15:00 ` Neil Armstrong
2020-04-23 15:45 ` H. Nikolaus Schaller
2020-04-23 20:36 ` Maxime Ripard
2020-04-24 9:51 ` H. Nikolaus Schaller
2020-04-21 16:42 ` Philipp Rossak
2020-04-23 20:37 ` Maxime Ripard
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=20200415162151.rwym4ioqz27migfn@gilmour.lan \
--to=maxime@cerno.tech \
--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=narmstrong@baylibre.com \
--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 \
/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).