Re: [PATCH v6 01/12] dt-bindings: add img, pvrsgx.yaml for Imagination GPUs

[Maxime Ripard]

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@xxxxxxxxxx>:
> >
> >>
> >> 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: signature.asc
Description: PGP signature