| Message ID | 20240108183302.255055-1-afd@ti.com |
|---|---|
| Headers | show |
| Series | Device tree support for Imagination Series5 GPU | expand |
Andrew Davis <afd@ti.com> writes: Hello Andrew, > Signed-off-by: Andrew Davis <afd@ti.com> > --- I think this deserves a commit message with the rationale for the rename. Because kept an eye to the previous version, I know the reason and agree with the change. Also, if remember correctly this was suggested by Maxime? After a adding a commit message and Suggested-by tag: Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Andrew Davis <afd@ti.com> writes: > Add SGX GPU device entry to base OMAP4 dtsi file. > > Signed-off-by: Andrew Davis <afd@ti.com> > --- Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
On 08/01/2024 19:32, Andrew Davis wrote: > The Imagination PowerVR Series5 "SGX" GPU is part of several SoCs from > multiple vendors. Describe how the SGX GPU is integrated in these SoC, > including register space and interrupts. Clocks, reset, and power domain > information is SoC specific. > > Signed-off-by: Andrew Davis <afd@ti.com> > --- > .../bindings/gpu/img,powervr-sgx.yaml | 124 ++++++++++++++++++ > MAINTAINERS | 1 + > 2 files changed, 125 insertions(+) > create mode 100644 Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml > > diff --git a/Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml b/Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml > new file mode 100644 > index 0000000000000..bb821e1184de9 > --- /dev/null > +++ b/Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml > @@ -0,0 +1,124 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +# Copyright (c) 2023 Imagination Technologies Ltd. Your email has @TI domain, are you sure you attribute your copyrights to Imagination? ... > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + > + clocks: true Missing min/maxItems > + > + clock-names: > + minItems: 1 > + items: > + - const: core > + - const: mem > + - const: sys > + > + power-domains: > + maxItems: 1 > + > +required: > + - compatible > + - reg > + - interrupts > + > +additionalProperties: false This goes after allOf: block. > + > +allOf: > + - if: > + properties: > + compatible: > + contains: > + const: ti,am6548-gpu > + then: > + required: > + - power-domains > + else: > + properties: > + power-domains: false > + - if: > + properties: > + compatible: > + contains: > + enum: > + - allwinner,sun6i-a31-gpu > + - ingenic,jz4780-gpu > + then: > + allOf: > + - if: I don't understand why do you need to embed allOf inside another allOf. The upper (outer) if:then: looks entirely useless. > + properties: > + compatible: > + contains: > + const: allwinner,sun6i-a31-gpu > + then: > + properties: > + clocks: > + minItems: 2 > + maxItems: 2 > + clock-names: > + minItems: 2 > + maxItems: 2 Best regards, Krzysztof
On 09/01/2024 17:53, Andrew Davis wrote: > On 1/9/24 5:32 AM, Krzysztof Kozlowski wrote: >> On 08/01/2024 19:32, Andrew Davis wrote: >>> The Imagination PowerVR Series5 "SGX" GPU is part of several SoCs from >>> multiple vendors. Describe how the SGX GPU is integrated in these SoC, >>> including register space and interrupts. Clocks, reset, and power domain >>> information is SoC specific. >>> >>> Signed-off-by: Andrew Davis <afd@ti.com> >>> --- >>> .../bindings/gpu/img,powervr-sgx.yaml | 124 ++++++++++++++++++ >>> MAINTAINERS | 1 + >>> 2 files changed, 125 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml >>> >>> diff --git a/Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml b/Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml >>> new file mode 100644 >>> index 0000000000000..bb821e1184de9 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml >>> @@ -0,0 +1,124 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>> +# Copyright (c) 2023 Imagination Technologies Ltd. >> >> Your email has @TI domain, are you sure you attribute your copyrights to >> Imagination? >> > > The file started as a copy/paste from a IMG copyrighted file, even > though it is now almost completely re-written I've left their (c) > for good measure. I'll add an additional TI (c). > >> ... >> >>> + >>> + reg: >>> + maxItems: 1 >>> + >>> + interrupts: >>> + maxItems: 1 >>> + >>> + clocks: true >> >> Missing min/maxItems >> > > These are set in the allOf/if/then blocks below, seems I know, but we expect them here. https://elixir.bootlin.com/linux/v5.19-rc6/source/Documentation/devicetree/bindings/clock/samsung,exynos7-clock.yaml#L57 > if I don't set them to at least something here then I get > a warning: > > 'clock-names', 'clocks' do not match any of the regexes: 'pinctrl-[0-9]+' > > even if I define them in the allOf block below. I don't > know what the min/max should be until I check the compatible > in the allOf block. As always: the widest constraints. ... > Logic in YAML always seems messy to me, here it is in pseudo C: > > if (compatible == allwinner,sun6i-a31-gpu || > compatible == ingenic,jz4780-gpu) { > if (compatible == allwinner,sun6i-a31-gpu) > clocks: ... > if (compatible == ingenic,jz4780-gpu) > clocks: ... > required: > - clocks > - clock-names > } else { /* disallow for all others */ > properties: > clocks: false > clock-names: false > } OK, I see, that's the limitation of YAML. The point is that this code is not readable, so just list all fallbacks or variants. Best regards, Krzysztof
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
Hi, On Fri, Jan 12, 2024 at 06:33:58PM +0100, H. Nikolaus Schaller wrote: > > 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). There's no reason to disable it in the DT: the hardware block would still be there and it's rendering to memory so it still could be useful. If there's no display on the board and you really don't want the GPU driver, then you can disable the driver or block the module loading, but it should be a distro / package / user decision, not a DT / kernel one still. Maxime
Hi, > Am 15.01.2024 um 09:25 schrieb Maxime Ripard <mripard@kernel.org>: > > Hi, > > On Fri, Jan 12, 2024 at 06:33:58PM +0100, H. Nikolaus Schaller wrote: >>> 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). > > There's no reason to disable it in the DT: the hardware block would > still be there and it's rendering to memory so it still could be useful. Well, if you know that the board does not have a dm3730 but a dm3725 without GPU it is better to disable the GPU completely instead of loading the driver and make it detect by some internal bits that it has no GPU on the SoC. > If there's no display on the board and you really don't want the GPU > driver, then you can disable the driver or block the module loading, but > it should be a distro / package / user decision, not a DT / kernel one > still. The same holds for aes: dss: dsi: hdmi: etc. If they are not used by some board file, they don't change a single bit of the DTB [1] which IMHO would be of reasonable concern to question additional labels. BR and thanks, Nikolaus [1] https://devicetree-specification.readthedocs.io/en/stable/source-language.html "Labels are only used in the devicetree source format and are not encoded into the DTB binary."
Hi, On Mon, 15 Jan 2024 09:55:00 +0100 "H. Nikolaus Schaller" <hns@goldelico.com> wrote: > > There's no reason to disable it in the DT: the hardware block would > > still be there and it's rendering to memory so it still could be useful. > > Well, if you know that the board does not have a dm3730 but a dm3725 without > GPU it is better to disable the GPU completely instead of loading the driver > and make it detect by some internal bits that it has no GPU on the SoC. > That is at least some valid reason. > > If there's no display on the board and you really don't want the GPU > > driver, then you can disable the driver or block the module loading, but > > it should be a distro / package / user decision, not a DT / kernel one > > still. > > The same holds for aes: dss: dsi: hdmi: etc. If they are not used by some > board file, they don't change a single bit of the DTB [1] which IMHO would > be of reasonable concern to question additional labels. There is some difference here, some hardware can just not be used without wired external pins, the gpu can be used even if no display is connected either to accelerate some remote access or you could use the gpu for something completely else... Maybe mining bitcoins if temperate gets too low to warm you pocket ;-) But as these labels do not harm, I have no strong opinion against it. Regards, Andreas
On Mon, Jan 15, 2024 at 09:55:00AM +0100, H. Nikolaus Schaller wrote: > Hi, > > > Am 15.01.2024 um 09:25 schrieb Maxime Ripard <mripard@kernel.org>: > > > > Hi, > > > > On Fri, Jan 12, 2024 at 06:33:58PM +0100, H. Nikolaus Schaller wrote: > >>> 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). > > > > There's no reason to disable it in the DT: the hardware block would > > still be there and it's rendering to memory so it still could be useful. > > Well, if you know that the board does not have a dm3730 but a dm3725 without > GPU it is better to disable the GPU completely instead of loading the driver > and make it detect by some internal bits that it has no GPU on the SoC. It shouldn't even be in the DTSI if it's not in the SoC. > > If there's no display on the board and you really don't want the GPU > > driver, then you can disable the driver or block the module loading, but > > it should be a distro / package / user decision, not a DT / kernel one > > still. > > The same holds for aes: dss: dsi: hdmi: etc. If they are not used by some > board file, they don't change a single bit of the DTB [1] which IMHO would > be of reasonable concern to question additional labels. Not really, no. If there's no HDMI connector, the HDMI controller is useless. A GPU without a display can still be useful, depending on the workload. Maxime
Hello all, I know this has been tried before[0], but given the recent upstreaming of the Series6+ GPU bindings I figured it might be time to give the Series5 bindings another try. While there is currently no mainline driver for these binding, there is an open source out-of-tree kernel-side driver available[1]. Having a stable and upstream binding for these devices allows us to describe this hardware in device tree. This is my vision for how these bindings should look, along with some example uses in several SoC DT files. The compatible names have been updated to match what was decided on for Series6+, but otherwise most is the same as we have been using in our vendor tree for many years. Thanks, Andrew Based on next-20240108. [0]: https://lkml.org/lkml/2020/4/24/1222 [1]: https://github.com/openpvrsgx-devgroup Changes for RFC v2: - Added patch to rename Rogue+ binding to img,powervr-rogue.yaml - Locked all property item counts - Removed nodename pattern check Andrew Davis (11): dt-bindings: gpu: Rename img,powervr to img,powervr-rogue dt-bindings: gpu: Add PowerVR Series5 SGX GPUs ARM: dts: omap3: Add device tree entry for SGX GPU ARM: dts: omap4: Add device tree entry for SGX GPU ARM: dts: omap5: Add device tree entry for SGX GPU ARM: dts: AM33xx: Add device tree entry for SGX GPU ARM: dts: AM437x: Add device tree entry for SGX GPU ARM: dts: DRA7xx: Add device tree entry for SGX GPU arm64: dts: ti: k3-am654-main: Add device tree entry for SGX GPU ARM: dts: sun6i: Add device tree entry for SGX GPU MIPS: DTS: jz4780: Add device tree entry for SGX GPU ...mg,powervr.yaml => img,powervr-rogue.yaml} | 4 +- .../bindings/gpu/img,powervr-sgx.yaml | 124 ++++++++++++++++++ MAINTAINERS | 3 +- arch/arm/boot/dts/allwinner/sun6i-a31.dtsi | 9 ++ arch/arm/boot/dts/ti/omap/am33xx.dtsi | 9 +- arch/arm/boot/dts/ti/omap/am3517.dtsi | 11 +- arch/arm/boot/dts/ti/omap/am4372.dtsi | 6 + arch/arm/boot/dts/ti/omap/dra7.dtsi | 9 +- arch/arm/boot/dts/ti/omap/omap34xx.dtsi | 11 +- arch/arm/boot/dts/ti/omap/omap36xx.dtsi | 9 +- arch/arm/boot/dts/ti/omap/omap4.dtsi | 9 +- arch/arm/boot/dts/ti/omap/omap5.dtsi | 9 +- arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 7 + arch/mips/boot/dts/ingenic/jz4780.dtsi | 11 ++ 14 files changed, 201 insertions(+), 30 deletions(-) rename Documentation/devicetree/bindings/gpu/{img,powervr.yaml => img,powervr-rogue.yaml} (91%) create mode 100644 Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml