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

[Maxime Ripard]

On Fri, Apr 17, 2020 at 02:15:44PM +0200, H. Nikolaus Schaller wrote:
> > Am 17.04.2020 um 12:25 schrieb Maxime Ripard <maxime@xxxxxxxxxx>:
> > On Wed, Apr 15, 2020 at 06:42:18PM +0200, H. Nikolaus Schaller wrote:
> >>> Am 15.04.2020 um 18:21 schrieb Maxime Ripard <maxime@xxxxxxxxxx>:
> >>> 
> >>> 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@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.
> >> 
> >> Why?
> >> 
> >> TI SoC seem to be the broadest number of available users
> >> of sgx5xx in the past and nowadays. Others are more the exception.
> > 
> > And maybe TI has some complicated stuff around the GPU that others don't have?
> 
> Looks so.
> 
> > If I look quickly at the Allwinner stuff, I see nothing looking alike in the
> > SoC, so making the binding like that for everyone just because TI did something
> > doesn't really make much sense.
> 
> That is why I propose to make the clocks optional. This solves both
> cases in a simple and neat way.
> 
> > 
> >>> 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.
> > 
> > There is no need for a clocks property *currently* *on the OMAP*.
> 
> Yes. But why "currently"? Do you think the OMAPs we already have
> defined and tested will change?

Like I said, DVFS is likely to be one in the future.

> >> 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.
> > 
> > Well, they know what they do if you document the binding. Let's say I have two
> > clocks now on my SoC, and you just document that you want a clocks property,
> > with a generic name in clock-names like "gpu".
> 
> Yes, that is what I want to propose for v7:
> 
>   clocks:
>     maxItems: 1
> 
>   clock-names:
>     maxItems: 1
>     items:
>       - const: gpu

If you document what the "gpu" clock is supposed to be.

Is it the clock for the bus (clocking the register part of the GPU), the clock
for the GPU cores? Something else?

> >> If you agree I can add the clocks/clock-names property as an
> >> optional property. This should solve omap and all others.
> > 
> > With the above example, what clock should I put in there? In which order? This
> > isn't some random example pulled out of nowhere. The Allwinner A31 has (at
> > least) 4 clocks for the GPU, 1 reset line and 1 regulator, so I can only assume
> > that the GPU actually needs at least that amount to be properly integrated into
> > an SoC.
> 
> Ah, now I understand your motivation: you have access and experience with
> the A31 and you know that our proposal doesn't fit to it.

Not only the A31. If you don't document what your expectations are for a generic
component like that, every SoC will assume that your GPU clock is something
different and you won't be able to make any sense of it in your driver.

> From what I know from your description is that the A31 is quite special with
> 4 GPU clocks... Are they all really for the GPU or 3 of them for the interface
> logic (like on OMAP which separates between "functional clocks" and "interface
> clocks")? Or are there 4 groups of GPU cores with a separate clock for each one?

1 is the equivalent of the interface clock, the others seem to be for the
functional clocks.

> So what would be your proposal for the A31 DT?
> 
> Then I get a chance to compare DT snippets and try to make a mixture for
> the bindings.

You'd have to know the GPU to do that, and I don't.

> >>> 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.
> > 
> > On the software side, yes, of course. But the discussion here doesn't have much
> > to do with software support, this is about the hardware. No matter if you enable
> > DVFS or not, you'll have those resources connected to the GPU.
> > 
> > And if you want to enable the strict minimum in DT for now and expand it later
> > as the software gains support for more stuff, then you'll have to deal with the
> > minimal stuff in software later-on to keep the backward compatibility.
> 
> That is IMHO common practise everywhere. Sometimes you even have to adapt
> years old DT to new limitations of the drivers (this happened recently for
> combination of SPI and GPIO).

To some extent, yes. But those old bindings that turn out to be wrong at least
contain most infos about the hardware, even though it's incomplete or flawed.
Your proposal doesn't.

> And you can still write two different drivers for a single bindings document
> or use the .data field of the compatibility table. And I think clocks and regulators
> can also be referenced by name if they are not defined in DT. This is not a
> "single variety" style, but a potential solution.
> 
> What I want to say: there are many roads to Rome.

What I want to say is: all the roads you listed above are going to be painful.
Take your time, have a generic driver running from your generic binding you want
to introduce on all the SoCs you want to support, and *then* start this
discussion again.

Maxime

Attachment: signature.asc
Description: PGP signature