| 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>
Andrew Davis <afd@ti.com> writes: > Add SGX GPU device entry to base AM437x dtsi file. > > Signed-off-by: Andrew Davis <afd@ti.com> > --- Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Hi Andrew, On Mon, 2024-01-08 at 12:32 -0600, Andrew Davis wrote: > Signed-off-by: Andrew Davis <afd@ti.com> > --- > .../bindings/gpu/{img,powervr.yaml => img,powervr-rogue.yaml} | 4 ++-- > MAINTAINERS | 2 +- > 2 files changed, 3 insertions(+), 3 deletions(-) > rename Documentation/devicetree/bindings/gpu/{img,powervr.yaml => img,powervr-rogue.yaml} (91%) > > diff --git a/Documentation/devicetree/bindings/gpu/img,powervr.yaml b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml > similarity index 91% > rename from Documentation/devicetree/bindings/gpu/img,powervr.yaml > rename to Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml > index a13298f1a1827..03a8308b41ae7 100644 > --- a/Documentation/devicetree/bindings/gpu/img,powervr.yaml > +++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml > @@ -2,10 +2,10 @@ > # Copyright (c) 2023 Imagination Technologies Ltd. > %YAML 1.2 > --- > -$id: http://devicetree.org/schemas/gpu/img,powervr.yaml# > +$id: http://devicetree.org/schemas/gpu/img,powervr-rogue.yaml# > $schema: http://devicetree.org/meta-schemas/core.yaml# > > -title: Imagination Technologies PowerVR and IMG GPU > +title: Imagination Technologies PowerVR Rogue and IMG GPUs All the GPUs that will appear in this file will be Rogues, so for me it would be more natural for 'Rogue' to come after 'IMG'. Can you change the title to: Imagination Technologies PowerVR and IMG Rogue GPUs With that changed and Javier's suggestions addressed: Reviewed-by: Frank Binns <frank.binns@imgtec.com> > > maintainers: > - Frank Binns <frank.binns@imgtec.com> > diff --git a/MAINTAINERS b/MAINTAINERS > index fa67e2624723f..5b205795da04e 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -10461,7 +10461,7 @@ M: Donald Robson <donald.robson@imgtec.com> > M: Matt Coster <matt.coster@imgtec.com> > S: Supported > T: git git://anongit.freedesktop.org/drm/drm-misc > -F: Documentation/devicetree/bindings/gpu/img,powervr.yaml > +F: Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml > F: Documentation/gpu/imagination/ > F: drivers/gpu/drm/imagination/ > F: include/uapi/drm/pvr_drm.h
On 08/01/2024 19:32, Andrew Davis wrote: > Signed-off-by: Andrew Davis <afd@ti.com> > --- > .../bindings/gpu/{img,powervr.yaml => img,powervr-rogue.yaml} | 4 ++-- > MAINTAINERS | 2 +- > 2 files changed, 3 insertions(+), 3 deletions(-) If you are renaming it, why not renaming to match compatible as we usually expect? Best regards, Krzysztof
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 1/9/24 5:28 AM, Krzysztof Kozlowski wrote: > On 08/01/2024 19:32, Andrew Davis wrote: >> Signed-off-by: Andrew Davis <afd@ti.com> >> --- >> .../bindings/gpu/{img,powervr.yaml => img,powervr-rogue.yaml} | 4 ++-- >> MAINTAINERS | 2 +- >> 2 files changed, 3 insertions(+), 3 deletions(-) > > If you are renaming it, why not renaming to match compatible as we > usually expect? > There are (or will be) multiple compatible strings described in this file, naming the file after just one would not fully convey the content of the file. This generic style naming seems common already for bindings with multiple compatibles. Andrew > Best regards, > Krzysztof >
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 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. >> + >> + 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. > ACK >> + >> +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. > It is so that both compatibles falls through to having clock being required. 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 } Now if I had an "else if" that didn't force the indention to keep growing I would have used that. (does one exist?) I also cannot simply add the clock properties only for the two compats need them for the reasons above and so must add them unconditionally before then explicitly disable them in a catch-all else path. Andrew >> + 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:14, Andrew Davis wrote: > On 1/9/24 5:28 AM, Krzysztof Kozlowski wrote: >> On 08/01/2024 19:32, Andrew Davis wrote: >>> Signed-off-by: Andrew Davis <afd@ti.com> >>> --- >>> .../bindings/gpu/{img,powervr.yaml => img,powervr-rogue.yaml} | 4 ++-- >>> MAINTAINERS | 2 +- >>> 2 files changed, 3 insertions(+), 3 deletions(-) >> >> If you are renaming it, why not renaming to match compatible as we >> usually expect? >> > > There are (or will be) multiple compatible strings described in this > file, naming the file after just one would not fully convey the content > of the file. This generic style naming seems common already for bindings > with multiple compatibles. I saw only one compatible used as fallback. Where are more? 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
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