Message-ID: <nzqeqwof44e3nxjz6lsxmxcfh235unbu343br45esxh6vinskp@xvjydpxhvsuk>
Date: Thu, 21 Dec 2023 09:58:53 +0100
From: Maxime Ripard <mripard@...nel.org>
To: "H. Nikolaus Schaller" <hns@...delico.com>
Cc: Andrew Davis <afd@...com>, Frank Binns <frank.binns@...tec.com>, 
	Donald Robson <donald.robson@...tec.com>, Matt Coster <matt.coster@...tec.com>, 
	Adam Ford <aford173@...il.com>, Ivaylo Dimitrov <ivo.g.dimitrov.75@...il.com>, 
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Thomas Zimmermann <tzimmermann@...e.de>, 
	Rob Herring <robh+dt@...nel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, 
	Conor Dooley <conor+dt@...nel.org>, Chen-Yu Tsai <wens@...e.org>, 
	Jernej Skrabec <jernej.skrabec@...il.com>, Samuel Holland <samuel@...lland.org>, 
	Benoît Cousson <bcousson@...libre.com>, Tony Lindgren <tony@...mide.com>, Nishanth Menon <nm@...com>, 
	Vignesh Raghavendra <vigneshr@...com>, Tero Kristo <kristo@...nel.org>, 
	Paul Cercueil <paul@...pouillou.net>, dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	linux-sunxi@...ts.linux.dev, linux-omap@...r.kernel.org, linux-mips@...r.kernel.org
Subject: Re: [PATCH RFC 01/10] dt-bindings: gpu: Add PowerVR Series5 SGX GPUs

On Mon, Dec 18, 2023 at 11:54:47AM +0100, H. Nikolaus Schaller wrote:
> 
> 
> > Am 18.12.2023 um 11:14 schrieb Maxime Ripard <mripard@...nel.org>:
> > 
> > On Mon, Dec 18, 2023 at 10:28:09AM +0100, H. Nikolaus Schaller wrote:
> >> Hi Maxime,
> >> 
> >>> Am 15.12.2023 um 14:33 schrieb Maxime Ripard <mripard@...nel.org>:
> >>> 
> >>>>> 
> >>>>> It's for a separate architecture, with a separate driver, maintained out
> >>>>> of tree by a separate community, with a separate set of requirements as
> >>>>> evidenced by the other thread. And that's all fine in itself, but
> >>>>> there's very little reason to put these two bindings in the same file.
> >>>>> 
> >>>>> We could also turn this around, why is it important that it's in the
> >>>>> same file?
> >>>> 
> >>>> Same vendor. And enough similarity in architectures, even a logical sequence
> >>>> of development of versions (SGX = Version 5, Rogue = Version 6+) behind.
> >>>> (SGX and Rogue seem to be just trade names for their architecture development).
> >>> 
> >>> Again, none of that matters for *where* the binding is stored.
> >> 
> >> So what then speaks against extending the existing bindings file as proposed
> >> here?
> > 
> > I mean, apart from everything you quoted, then sure, nothing speaks
> > against it.
> > 
> >>>> AFAIK bindings should describe hardware and not communities or drivers
> >>>> or who is currently maintaining it. The latter can change, the first not.
> >>> 
> >>> Bindings are supposed to describe hardware indeed. Nothing was ever said
> >>> about where those bindings are supposed to be located.
> >>> 
> >>> There's hundreds of other YAML bindings describing devices of the same
> >>> vendors and different devices from the same generation.
> >> 
> >> Usually SoC seem to be split over multiple files by subsystem. Not by versions
> >> or generations. If the subsystems are similar enough they share the same bindings
> >> doc instead of having one for each generation duplicating a lot of code.
> >> 
> >> Here is a comparable example that combines multiple vendors and generations:
> >> 
> >> Documentation/devicetree/bindings/usb/generic-ehci.yaml
> > 
> > EHCI is a single interface for USB2.0 controllers. It's a standard API,
> > and is made of a single driver that requires minor modifications to deal
> > with multiple devices.
> > 
> > We're very far from the same situation here.
> 
> How far are we really?

There's one binding for one driver. You suggest one binding for two drivers.

> And, it is the purpose of the driver to handle different cases.
> 
> That there are currently two drivers is just a matter of history and
> not a necessity.

Cool, so what you're saying is that your plan is to support those GPUs
upstream in the imagination driver?

I guess we should delay this patch until we see that series then.

> >>> If anything it'll make it easier for you. I'm really not sure why it is
> >>> controversial and you're fighting this so hard.
> >> 
> >> Well, you made it controversial by proposing to split what IMHO belongs together.
> > 
> > No, reviews aren't controversial.
> > The controversy started when you chose
> > to oppose it while you could have just rolled with it.
> 
> Well, you asked
> 
> "I think it would be best to have a separate file for this, img,sgx.yaml
> maybe?"
> 
> and
> 
> "Because it's more convenient?"
> 
> I understood that as an invitation for discussing the pros and cons
> and working out the most convenient solution. And that involves
> playing the devil's advocate which of course is controversial by
> principle.
> 
> Now, IMHO all the pros and cons are on the table and the question is
> who makes a decision how to go.

You haven't listed any pro so far, you're claiming that the one I raise
are irrelevant.

> >> I feel that the original patch is good enough for its purpose and follows
> >> some design pattern that can be deduced from other binding docs.
> > 
> > [citation needed]
> 
> Joke: Documentation/devicetree/bindings/* - I am not aware of a formal analysis of course.
> 
> But see my example for ehci. It follows the pattern I mean. If clocks, regs, interrupts,
> resets, and more properties are (almost) the same, then group them and just differentiate
> by different compatible strings.

Again, EHCI is not something you can compare to. It's a binding to
support a standard interface. You don't have the same interface and your
driver will need to be different.

And more importantly: bindings are meant to describe the hardware
itself. How it's supported in Linux is irrelevant to the discussion.

So, we could have: 10 drivers for the same binding, or 1 driver for 10
bindings. The two notions are orthogonal.

> If necessary use some - if: clauses.
> 
> It is the task of drivers to handle the details.
>
> As my other (maybe more important) comment to this patch did indicate we IMHO can easily
> live with something like
> 
> +      - items:
> +          - enum:
> +              - ti,am62-gpu # IMG AXE GPU model/revision is fully discoverable
> +              - ti,omap3430-gpu # sgx530 Rev 121
> +              - ti,omap3630-gpu # sgx530 Rev 125
> +              - ingenic,jz4780-gpu # sgx540 Rev 130
> +              - ti,omap4430-gpu # sgx540 Rev 120
> +              - allwinner,sun6i-a31-gpu # sgx544 MP2 Rev 115
> +              - ti,omap4470-gpu # sgx544 MP1 Rev 112
> +              - ti,omap5432-gpu # sgx544 MP2 Rev 105
> +              - ti,am5728-gpu # sgx544 MP2 Rev 116
> +              - ti,am6548-gpu # sgx544 MP1 Rev 117
> 
> And leave it to drivers using a table to deduce the generation and
> revision or read it out from the chip. And there can even be different
> drivers handling only a subset of the potential compatibles.
> 
> Then the currently-out-of-tree driver for the sgx5 can be reworked in
> less than half an hour without loosing functionality.

Again, you're making it harder than it needs to be for no particular
reason other than the potential file name clash that can be addressed.

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists