Re: [PATCH RFC v2 04/11] ARM: dts: omap4: Add device tree entry for SGX GPU

[H . Nikolaus Schaller]

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@xxxxxx>:
> 
> Add SGX GPU device entry to base OMAP4 dtsi file.
> 
> Signed-off-by: Andrew Davis <afd@xxxxxx>
> ---
> arch/arm/boot/dts/ti/omap/omap4.dtsi | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
> 
> 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 = <1>;
> ranges = <0 0x56000000 0x2000000>;
> 
> - /*
> - * 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 = "disabled" to avoid driver
probing or disabling the GPU (e.g. if there is no display).

> + compatible = "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/f2f7cb3b858ef255f52f2b82a8bb34c047337afe

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 = "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 = <0x0 0x2000000>; /* 32MB */
> + interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
> + };
> };

BR and thanks,
Nikolaus