Date:   Wed, 15 Apr 2020 18:42:18 +0200
From:   "H. Nikolaus Schaller" <hns@...delico.com>
To:     Maxime Ripard <maxime@...no.tech>
Cc:     Neil Armstrong <narmstrong@...libre.com>,
        Mark Rutland <mark.rutland@....com>,
        David Airlie <airlied@...ux.ie>,
        James Hogan <jhogan@...nel.org>,
        dri-devel@...ts.freedesktop.org, linux-mips@...r.kernel.org,
        Paul Cercueil <paul@...pouillou.net>,
        linux-samsung-soc@...r.kernel.org, letux-kernel@...nphoenux.org,
        Paul Burton <paulburton@...nel.org>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Tony Lindgren <tony@...mide.com>, Chen-Yu Tsai <wens@...e.org>,
        Kukjin Kim <kgene@...nel.org>, devicetree@...r.kernel.org,
        Benoît Cousson <bcousson@...libre.com>,
        Rob Herring <robh+dt@...nel.org>, linux-omap@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        Philipp Rossak <embed3d@...il.com>,
        openpvrsgx-devgroup@...ux.org, linux-kernel@...r.kernel.org,
        Ralf Baechle <ralf@...ux-mips.org>,
        Daniel Vetter <daniel@...ll.ch>, kernel@...a-handheld.com
Subject: Re: [PATCH v6 01/12] dt-bindings: add img, pvrsgx.yaml for Imagination GPUs

Hi Maxime,

> Am 15.04.2020 um 18:21 schrieb Maxime Ripard <maxime@...no.tech>:
> 
> On Wed, Apr 15, 2020 at 05:09:45PM +0200, H. Nikolaus Schaller wrote:
>> Hi Maxime,
>> 
>> 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.

Sure, we can deduce that from general hardware design knowledge.
But not every detail must be described in DT. Only the important
ones.

> 
>>> 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@...00000 node is already merged and therefore
>> we are only adding the gpu child node. Without defining clocks.
>> 
>> For example:
>> 
>> 		sgx_module: target-module@...00000 {
>> 			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@...40000 {
>> 		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.

Why?

TI SoC seem to be the broadest number of available users
of sgx5xx in the past and nowadays. Others are more the exception.

> 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.

So you mean that we should require a dummy clock for the omap gpu node
or did I misunderstand that?

Well, yes there is of course a clock connection between the
omap target-module and the sgx but it is IMHO pointless to
describe it because it can't and does not need to be controlled
separately.

As said the target-module is already accepted and upstream and my
proposal is to get the gpu node described there. There is simply
no need for a clocks node for the omap.

What I also assume is that developers of DTS know what they do.
So the risk that there is different semantics is IMHO very low.

If you agree I can add the clocks/clock-names property as an
optional property. This should solve omap and all others.

> 
> 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.

Now you have unhooked me... Nobody seems to know if and how DVFS can be
applied to SGX. IMHO we should bake small bread first and get initial
support into mainline.

BR,
Nikolaus

Powered by blists - more mailing lists