Received-SPF: pass (google.com: domain of linux-kernel+bounces-24925-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Content-Type: text/plain;
	charset=us-ascii
Precedence: bulk
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3774.300.61.1.2\))
Subject: Re: [PATCH RFC v2 04/11] ARM: dts: omap4: Add device tree entry for
 SGX GPU
From: H. Nikolaus Schaller <hns@goldelico.com>
In-Reply-To: <20240108183302.255055-5-afd@ti.com>
Date: Fri, 12 Jan 2024 18:33:58 +0100
Cc: Frank Binns <frank.binns@imgtec.com>,
 Donald Robson <donald.robson@imgtec.com>,
 Matt Coster <matt.coster@imgtec.com>,
 Adam Ford <aford173@gmail.com>,
 Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>,
 Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
 Maxime Ripard <mripard@kernel.org>,
 Thomas Zimmermann <tzimmermann@suse.de>,
 Rob Herring <robh+dt@kernel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
 Conor Dooley <conor+dt@kernel.org>,
 Chen-Yu Tsai <wens@csie.org>,
 Jernej Skrabec <jernej.skrabec@gmail.com>,
 Samuel Holland <samuel@sholland.org>,
 =?utf-8?Q?Beno=C3=AEt_Cousson?= <bcousson@baylibre.com>,
 Tony Lindgren <tony@atomide.com>,
 Nishanth Menon <nm@ti.com>,
 Vignesh Raghavendra <vigneshr@ti.com>,
 Tero Kristo <kristo@kernel.org>,
 Paul Cercueil <paul@crapouillou.net>,
 dri-devel@lists.freedesktop.org,
 devicetree@vger.kernel.org,
 linux-kernel@vger.kernel.org,
 linux-arm-kernel@lists.infradead.org,
 linux-sunxi@lists.linux.dev,
 linux-omap@vger.kernel.org,
 linux-mips@vger.kernel.org
Content-Transfer-Encoding: quoted-printable
Message-Id: <122DC5ED-2AA7-46A0-845F-083922458385@goldelico.com>
References: <20240108183302.255055-1-afd@ti.com>
 <20240108183302.255055-5-afd@ti.com>
To: Andrew Davis <afd@ti.com>

Hi,
I just comment on this example, but it applies almost the same for all =
other .dtsi changes.

> Am 08.01.2024 um 19:32 schrieb Andrew Davis <afd@ti.com>:
>=20
> Add SGX GPU device entry to base OMAP4 dtsi file.
>=20
> Signed-off-by: Andrew Davis <afd@ti.com>
> ---
> arch/arm/boot/dts/ti/omap/omap4.dtsi | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>=20
> diff --git a/arch/arm/boot/dts/ti/omap/omap4.dtsi =
b/arch/arm/boot/dts/ti/omap/omap4.dtsi
> index 2bbff9032be3e..559b2bfe4ca7c 100644
> --- a/arch/arm/boot/dts/ti/omap/omap4.dtsi
> +++ b/arch/arm/boot/dts/ti/omap/omap4.dtsi
> @@ -501,10 +501,11 @@ sgx_module: target-module@56000000 {
> #size-cells =3D <1>;
> ranges =3D <0 0x56000000 0x2000000>;
>=20
> - /*
> - * Closed source PowerVR driver, no child device
> - * binding or driver in mainline
> - */
> + gpu@0 {

I wonder why we don't add a "gpu:" label here.

Almost all other subsystem nodes have one (e.g. emif:, aes:, dss:, dsi:, =
hdmi:, etc.),
obviously for convenience when using a .dtsi file.

It would allow a board-specific DTS to easily add status =3D "disabled" =
to avoid driver
probing or disabling the GPU (e.g. if there is no display).

> + compatible =3D "ti,omap4430-gpu", "img,powervr-sgx540";

It still appears to me that the "img,powervr-sgx540" (or similar) entry =
is redundant
information.

I have experimentally updated our openpvrsgx driver and we do not have =
any use for
this information (at least in the kernel driver):

=
https://github.com/goldelico/letux-kernel/commit/f2f7cb3b858ef255f52f2b82a=
8bb34c047337afe

It shows how easy it is to derive the sgx version and revision number if =
we ever
need it inside the driver.

So if you want to keep a reference to powervr, it would suffice to have

> + compatible =3D "ti,omap4430-gpu", "img,powervr-sgx";

Otherwise your device tree entries compile fine and seem to work (at =
least in
a cursory test on PandaBoard ES).

> + reg =3D <0x0 0x2000000>; /* 32MB */
> + interrupts =3D <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
> + };
> };

BR and thanks,
Nikolaus=