Messages in this thread | ![/](/images/icornerl.gif) | | Subject | Re: [PATCH RFC v2 04/11] ARM: dts: omap4: Add device tree entry for SGX GPU | From | H. Nikolaus Schaller <> | Date | Fri, 12 Jan 2024 18:33:58 +0100 |
| |
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>: > > Add SGX GPU device entry to base OMAP4 dtsi file. > > Signed-off-by: Andrew Davis <afd@ti.com> > --- > 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
| ![\](/images/icornerr.gif) |