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

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 16:21:24 +0200	[thread overview]
Message-ID: <20200415142124.yzfh6mtqq7cdq22e@gilmour.lan> (raw)
In-Reply-To: <535CAEBE-F43E-4BFC-B989-612C81F0D7EF@goldelico.com>

[-- Attachment #1: Type: text/plain, Size: 2705 bytes --]

On Wed, Apr 15, 2020 at 03:17:25PM +0200, H. Nikolaus Schaller wrote:
> Hi Neil,
>
> > Am 15.04.2020 um 14:54 schrieb Neil Armstrong <narmstrong@baylibre.com>:
> >
> > Hi,
> >
> > On 15/04/2020 14:43, H. Nikolaus Schaller wrote:
> >>
> >>> Am 15.04.2020 um 12:12 schrieb Maxime Ripard <maxime@cerno.tech>:
> >>>
> >>> Hi,
> >>>
> >>> On Wed, Apr 15, 2020 at 10:35:08AM +0200, H. Nikolaus Schaller wrote:
> >>>> The Imagination PVR/SGX GPU is part of several SoC from
> >>>> multiple vendors, e.g. TI OMAP, Ingenic JZ4780, Intel Poulsbo,
> >>>> Allwinner A83 and others.
> >>>>
> >>>> With this binding, we describe how the SGX processor is
> >>>> interfaced to the SoC (registers, interrupt etc.).
> >>>>
> >>>> In most cases, Clock, Reset and power management is handled
> >>>> by a parent node or elsewhere (e.g. code in the driver).
> >>>
> >>> Wouldn't the "code in the driver" still require the clock / reset /
> >>> power domain to be set in the DT?
> >>
> >> Well, some SoC seem to use existing clocks and have no reset.
> >> Or, although not recommended, they may have the io-address range
> >> hard-coded.
> >
> > The possible clocks and resets should be added, even if optional.
> >
> > Please look at the arm utgard, midgard and bifrost bindings.
>
> Interesting to compare to. Maybe we should also add the
> $nodename: pattern: '^gpu@[a-f0-9]+$'
>
> But the sgx binding is difficult to grasp here. Some SoC like the
> omap series have their own ti,sysc based target modules and the
> gpu nodes is a child of it lacking any clock and reset references
> for purpose.
>
> The jz4780 and some other need a clocks definition, but no reset.
> Having a reset seems to be an option for the SoC designer and
> not mandated by img. So is it part of the pvrsgx bindings or the
> SoC?
>
> 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. 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.

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.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]