Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 15 Apr 2020 18:21:51 +0200
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,
        =?utf-8?Q?Beno=C3=AEt?= 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
Message-ID: <20200415162151.rwym4ioqz27migfn@gilmour.lan>
References: <cover.1586939718.git.hns@goldelico.com>
 <06fb6569259bb9183d0a0d0fe70ec4f3033b8aab.1586939718.git.hns@goldelico.com>
 <20200415101251.o3wi5t6xvf56xmhq@gilmour.lan>
 <72919514-0657-4B71-902F-3E775E528F64@goldelico.com>
 <f4fdca8a-d18c-a8d2-7f51-d1ebbbab3647@baylibre.com>
 <535CAEBE-F43E-4BFC-B989-612C81F0D7EF@goldelico.com>
 <20200415142124.yzfh6mtqq7cdq22e@gilmour.lan>
 <DC0A2DE2-3D77-46F8-8DE1-55050FDACC9B@goldelico.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="tfobgrwcfdirkzal"
Content-Disposition: inline
In-Reply-To: <DC0A2DE2-3D77-46F8-8DE1-55050FDACC9B@goldelico.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


--tfobgrwcfdirkzal
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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

--tfobgrwcfdirkzal
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCXpc0nwAKCRDj7w1vZxhR
xUDXAQCc7y/+vAoavZRmglq4lR2jquJFLRkH7PYZvWPuzBNdAAD/cb0ol1cdNUpA
oCJwB2WILZ+2E78cCeIXmjyL7nqW6AA=
=GQ1H
-----END PGP SIGNATURE-----

--tfobgrwcfdirkzal--