| Message ID | 20240108183302.255055-3-afd@ti.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Device tree support for Imagination Series5 GPU | expand |
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: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: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
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. +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/gpu/img,powervr-sgx.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Imagination Technologies PowerVR SGX GPUs + +maintainers: + - Frank Binns <frank.binns@imgtec.com> + +properties: + compatible: + oneOf: + - items: + - enum: + - ti,omap3430-gpu # Rev 121 + - ti,omap3630-gpu # Rev 125 + - const: img,powervr-sgx530 + - items: + - enum: + - ingenic,jz4780-gpu # Rev 130 + - ti,omap4430-gpu # Rev 120 + - const: img,powervr-sgx540 + - items: + - enum: + - allwinner,sun6i-a31-gpu # MP2 Rev 115 + - ti,omap4470-gpu # MP1 Rev 112 + - ti,omap5432-gpu # MP2 Rev 105 + - ti,am5728-gpu # MP2 Rev 116 + - ti,am6548-gpu # MP1 Rev 117 + - const: img,powervr-sgx544 + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + clocks: true + + clock-names: + minItems: 1 + items: + - const: core + - const: mem + - const: sys + + power-domains: + maxItems: 1 + +required: + - compatible + - reg + - interrupts + +additionalProperties: false + +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: + properties: + compatible: + contains: + const: allwinner,sun6i-a31-gpu + then: + properties: + clocks: + minItems: 2 + maxItems: 2 + clock-names: + minItems: 2 + maxItems: 2 + - if: + properties: + compatible: + contains: + const: ingenic,jz4780-gpu + then: + properties: + clocks: + maxItems: 1 + clock-names: + maxItems: 1 + required: + - clocks + - clock-names + else: + properties: + clocks: false + clock-names: false + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/soc/ti,sci_pm_domain.h> + + gpu@7000000 { + compatible = "ti,am6548-gpu", "img,powervr-sgx544"; + reg = <0x7000000 0x10000>; + interrupts = <GIC_SPI 162 IRQ_TYPE_LEVEL_HIGH>; + power-domains = <&k3_pds 65 TI_SCI_PD_EXCLUSIVE>; + }; diff --git a/MAINTAINERS b/MAINTAINERS index 5b205795da04e..00ba13e019fa6 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -10462,6 +10462,7 @@ 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-rogue.yaml +F: Documentation/devicetree/bindings/gpu/img,powervr-sgx.yaml F: Documentation/gpu/imagination/ F: drivers/gpu/drm/imagination/ F: include/uapi/drm/pvr_drm.h
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