| Message ID | 20230714142526.111569-1-sarah.walker@imgtec.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Imagination Technologies PowerVR DRM driver | expand |
Hey Sarah, Your series does not appear to be threaded. `git send-email` can be passed, for example, a directory containing a whole series & will set the correct in-reply-to headers so that the series is in a single thread. On Fri, Jul 14, 2023 at 03:25:26PM +0100, Sarah Walker wrote: > Add the device tree binding documentation for the Series AXE GPU used in > TI AM62 SoCs. > Changes since v3: > - Remove oneOf in compatible property > - Remove power-supply (not used on AM62) > > Changes since v2: > - Add commit message description > - Remove mt8173-gpu support (not currently supported) > - Drop quotes from $id and $schema > - Remove reg: minItems > - Drop _clk suffixes from clock-names > - Remove operating-points-v2 property and cooling-cells (not currently > used) > - Add additionalProperties: false > - Remove stray blank line at the end of file The changelog should go below the --- line. > Signed-off-by: Sarah Walker <sarah.walker@imgtec.com> > --- > .../devicetree/bindings/gpu/img,powervr.yaml | 68 +++++++++++++++++++ > MAINTAINERS | 7 ++ > 2 files changed, 75 insertions(+) > create mode 100644 Documentation/devicetree/bindings/gpu/img,powervr.yaml > diff --git a/Documentation/devicetree/bindings/gpu/img,powervr.yaml b/Documentation/devicetree/bindings/gpu/img,powervr.yaml > new file mode 100644 > index 000000000000..3292a0440465 > --- /dev/null > +++ b/Documentation/devicetree/bindings/gpu/img,powervr.yaml > @@ -0,0 +1,68 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +# Copyright (c) 2022 Imagination Technologies Ltd. > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/gpu/img,powervr.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Imagination Technologies PowerVR GPU > + > +maintainers: > + - Sarah Walker <sarah.walker@imgtec.com> > + interrupts: > + items: > + - description: GPU interrupt The description here doesn't add any value, since there is only one interrupt, so you can drop it and do maxItems: 1 as you have done elsewhere. > + interrupt-names: > + items: > + - const: gpu And this items: - const: gpu can just be const: gpu Although, if there is only one interrupt this is probably not particularly helpful. Are there other implementations of this IP that have more interrupts? Otherwise, this looks good to me. Thanks, Conor.
On 14/07/2023 16:25, Sarah Walker wrote: > Add the device tree binding documentation for the Series AXE GPU used in > TI AM62 SoCs. > ... > + > + clocks: > + minItems: 1 > + maxItems: 3 > + > + clock-names: > + items: > + - const: core > + - const: mem > + - const: sys > + minItems: 1 Why clocks for this device vary? That's really unusual to have a SoC IP block which can have a clock physically disconnected, depending on the board (not SoC!). Best regards, Krzysztof
On 14/07/2023 16:25, Sarah Walker wrote: > Add the device tree binding documentation for the Series AXE GPU used in > TI AM62 SoCs. > ... > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/irq.h> > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + > + gpu: gpu@fd00000 { > + compatible = "ti,am62-gpu", "img,powervr-seriesaxe"; > + reg = <0x0fd00000 0x20000>; > + power-domains = <&some_pds 187>; > + clocks = <&k3_clks 187 0>; > + clock-names = "core"; > + interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-names = "gpu"; Why does it differ from your DTS? Best regards, Krzysztof
Hi Conor, Thank you for your feedback (comments below). On Sat, 2023-07-15 at 11:40 +0100, Conor Dooley wrote: > Hey Sarah, > > Your series does not appear to be threaded. `git send-email` can be > passed, for example, a directory containing a whole series & will set > the correct in-reply-to headers so that the series is in a single > thread. Thank you pointing this out, we'll make sure we do this for the next iteration. > > On Fri, Jul 14, 2023 at 03:25:26PM +0100, Sarah Walker wrote: > > Add the device tree binding documentation for the Series AXE GPU used in > > TI AM62 SoCs. > > Changes since v3: > > - Remove oneOf in compatible property > > - Remove power-supply (not used on AM62) > > > > Changes since v2: > > - Add commit message description > > - Remove mt8173-gpu support (not currently supported) > > - Drop quotes from $id and $schema > > - Remove reg: minItems > > - Drop _clk suffixes from clock-names > > - Remove operating-points-v2 property and cooling-cells (not currently > > used) > > - Add additionalProperties: false > > - Remove stray blank line at the end of file > > The changelog should go below the --- line. Ack > > > Signed-off-by: Sarah Walker <sarah.walker@imgtec.com> > > --- > > .../devicetree/bindings/gpu/img,powervr.yaml | 68 +++++++++++++++++++ > > MAINTAINERS | 7 ++ > > 2 files changed, 75 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/gpu/img,powervr.yaml > > diff --git a/Documentation/devicetree/bindings/gpu/img,powervr.yaml b/Documentation/devicetree/bindings/gpu/img,powervr.yaml > > new file mode 100644 > > index 000000000000..3292a0440465 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/gpu/img,powervr.yaml > > @@ -0,0 +1,68 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +# Copyright (c) 2022 Imagination Technologies Ltd. > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/gpu/img,powervr.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Imagination Technologies PowerVR GPU > > + > > +maintainers: > > + - Sarah Walker <sarah.walker@imgtec.com> > > + interrupts: > > + items: > > + - description: GPU interrupt > > The description here doesn't add any value, since there is only one > interrupt, so you can drop it and do maxItems: 1 as you have done > elsewhere. Sure, will make this change. > > > + interrupt-names: > > + items: > > + - const: gpu > > And this > items: > - const: gpu > can just be > const: gpu > > Although, if there is only one interrupt this is probably not > particularly helpful. Are there other implementations of this IP that > have more interrupts? No, all our current GPUs just have a single interrupt. I assume it's more future proof to keep the name in case that ever changes? As in, by having the name now we can make it a required property, which I guess we won't be able to do at some later point. Thanks Frank > > Otherwise, this looks good to me. > > Thanks, > Conor.
On 18/07/2023 13:08, Frank Binns wrote: >> And this >> items: >> - const: gpu >> can just be >> const: gpu >> >> Although, if there is only one interrupt this is probably not >> particularly helpful. Are there other implementations of this IP that >> have more interrupts? > > No, all our current GPUs just have a single interrupt. I assume it's more future > proof to keep the name in case that ever changes? Why do you need name in the first place? If there is single entry, the name is pointless, especially if it repeats the name of the IP block. > As in, by having the name now > we can make it a required property, which I guess we won't be able to do at some > later point. Why even making it required? Best regards, Krzysztof
Hi, To expand on what Krzysztof said On Tue, Jul 18, 2023 at 01:10:14PM +0200, Krzysztof Kozlowski wrote: > On 18/07/2023 13:08, Frank Binns wrote: > >> And this > >> items: > >> - const: gpu > >> can just be > >> const: gpu > >> > >> Although, if there is only one interrupt this is probably not > >> particularly helpful. Are there other implementations of this IP that > >> have more interrupts? > > > > No, all our current GPUs just have a single interrupt. I assume it's more future > > proof to keep the name in case that ever changes? > > Why do you need name in the first place? If there is single entry, the > name is pointless, especially if it repeats the name of the IP block. Generally speaking, if there's only one resource (interrupt, clock, etc) we don't put any discriminant. If you need to extend it later on for some reason, then you'll add an interrupt-names property and you can either require it for that new GPU or whatever, or fallback on the nameless on if no name was found. > > As in, by having the name now > > we can make it a required property, which I guess we won't be able to do at some > > later point. > > Why even making it required? There's no issue with introducing a new property later on if a GPU needs it. Then, you can either make it required only for that particular generation, or you can have some fallback case. Both are fine and easy to do. Maxime
Hi Krzysztof, On Mon, 2023-07-17 at 09:29 +0200, Krzysztof Kozlowski wrote: > On 14/07/2023 16:25, Sarah Walker wrote: > > Add the device tree binding documentation for the Series AXE GPU used in > > TI AM62 SoCs. > > > > ... > > > + > > + clocks: > > + minItems: 1 > > + maxItems: 3 > > + > > + clock-names: > > + items: > > + - const: core > > + - const: mem > > + - const: sys > > + minItems: 1 > > Why clocks for this device vary? That's really unusual to have a SoC IP > block which can have a clock physically disconnected, depending on the > board (not SoC!). By default, this GPU IP (Series AXE) operates on a single clock (the core clock), but the SoC vendor can choose at IP integration time to run the memory and SoC interfaces on separate clocks (mem and sys clocks respectively). We also have IP, such as the Series 6XT, that requires all 3 clocks. So the situation here is that Series AXE may have 1 or 3 clocks, but the TI implementation being added only has 1. I guess we need to add something like: allOf: - if: properties: compatible: contains: const: ti,am62-gpu then: properties: clocks: maxItems: 1 Or should we be doing something else? Thanks Frank > > > Best regards, > Krzysztof >
Hi Krzysztof, On Tue, 2023-07-18 at 13:10 +0200, Krzysztof Kozlowski wrote: > On 18/07/2023 13:08, Frank Binns wrote: > > > And this > > > items: > > > - const: gpu > > > can just be > > > const: gpu > > > > > > Although, if there is only one interrupt this is probably not > > > particularly helpful. Are there other implementations of this IP that > > > have more interrupts? > > > > No, all our current GPUs just have a single interrupt. I assume it's more future > > proof to keep the name in case that ever changes? > > Why do you need name in the first place? If there is single entry, the > name is pointless, especially if it repeats the name of the IP block. > > > As in, by having the name now > > we can make it a required property, which I guess we won't be able to do at some > > later point. > > Why even making it required? It seems nicer to look up a resource in the driver based on a name rather than an index. Happy to drop it though. Thanks Frank > > Best regards, > Krzysztof >
Hi Krzysztof, On Tue, 2023-07-18 at 08:20 +0200, Krzysztof Kozlowski wrote: > On 14/07/2023 16:25, Sarah Walker wrote: > > Add the device tree binding documentation for the Series AXE GPU used in > > TI AM62 SoCs. > > > > ... > > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/interrupt-controller/irq.h> > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > + > > + gpu: gpu@fd00000 { > > + compatible = "ti,am62-gpu", "img,powervr-seriesaxe"; > > + reg = <0x0fd00000 0x20000>; > > + power-domains = <&some_pds 187>; > > + clocks = <&k3_clks 187 0>; > > + clock-names = "core"; > > + interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>; > > + interrupt-names = "gpu"; > > Why does it differ from your DTS? This is just an oversight on our part. We'll make sure they both match in the next version. Thanks Frank > > Best regards, > Krzysztof >
On 18/07/2023 13:36, Frank Binns wrote: > Hi Krzysztof, > > On Tue, 2023-07-18 at 13:10 +0200, Krzysztof Kozlowski wrote: >> On 18/07/2023 13:08, Frank Binns wrote: >>>> And this >>>> items: >>>> - const: gpu >>>> can just be >>>> const: gpu >>>> >>>> Although, if there is only one interrupt this is probably not >>>> particularly helpful. Are there other implementations of this IP that >>>> have more interrupts? >>> >>> No, all our current GPUs just have a single interrupt. I assume it's more future >>> proof to keep the name in case that ever changes? >> >> Why do you need name in the first place? If there is single entry, the >> name is pointless, especially if it repeats the name of the IP block. >> >>> As in, by having the name now >>> we can make it a required property, which I guess we won't be able to do at some >>> later point. >> >> Why even making it required? > > It seems nicer to look up a resource in the driver based on a name rather than > an index. Not really... Slower and confuses people, because then they think order is flexible. Best regards, Krzysztof
On 18/07/2023 13:47, Frank Binns wrote: > Hi Krzysztof, > > On Tue, 2023-07-18 at 08:20 +0200, Krzysztof Kozlowski wrote: >> On 14/07/2023 16:25, Sarah Walker wrote: >>> Add the device tree binding documentation for the Series AXE GPU used in >>> TI AM62 SoCs. >>> >> >> ... >> >>> + >>> +additionalProperties: false >>> + >>> +examples: >>> + - | >>> + #include <dt-bindings/interrupt-controller/irq.h> >>> + #include <dt-bindings/interrupt-controller/arm-gic.h> >>> + >>> + gpu: gpu@fd00000 { >>> + compatible = "ti,am62-gpu", "img,powervr-seriesaxe"; >>> + reg = <0x0fd00000 0x20000>; >>> + power-domains = <&some_pds 187>; >>> + clocks = <&k3_clks 187 0>; >>> + clock-names = "core"; >>> + interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>; >>> + interrupt-names = "gpu"; >> >> Why does it differ from your DTS? > > This is just an oversight on our part. We'll make sure they both match in the > next version. > Just test your DTS before sending. You would see errors and there is no need to involve manual reviewing. It is always better to use tools than reviewers time. Otherwise, please kindly donate your time by helping to review other patches. Best regards, Krzysztof
On 18/07/2023 13:32, Frank Binns wrote: > Hi Krzysztof, > > On Mon, 2023-07-17 at 09:29 +0200, Krzysztof Kozlowski wrote: >> On 14/07/2023 16:25, Sarah Walker wrote: >>> Add the device tree binding documentation for the Series AXE GPU used in >>> TI AM62 SoCs. >>> >> >> ... >> >>> + >>> + clocks: >>> + minItems: 1 >>> + maxItems: 3 >>> + >>> + clock-names: >>> + items: >>> + - const: core >>> + - const: mem >>> + - const: sys >>> + minItems: 1 >> >> Why clocks for this device vary? That's really unusual to have a SoC IP >> block which can have a clock physically disconnected, depending on the >> board (not SoC!). > > By default, this GPU IP (Series AXE) operates on a single clock (the core > clock), but the SoC vendor can choose at IP integration time to run the memory > and SoC interfaces on separate clocks (mem and sys clocks respectively). We also > have IP, such as the Series 6XT, that requires all 3 clocks. Currently you have only one SoC vendor with only one SoC, so the clocks do not vary. Describing the clocks for all possible variants is a good idea, but then this should be clear that this implementation uses subset. > > So the situation here is that Series AXE may have 1 or 3 clocks, but the TI > implementation being added only has 1. > > I guess we need to add something like: > > allOf: > - if: > properties: > compatible: > contains: > const: ti,am62-gpu > then: > properties: > clocks: > maxItems: 1 > > Or should we be doing something else? Yes. clock-names as well.. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/gpu/img,powervr.yaml b/Documentation/devicetree/bindings/gpu/img,powervr.yaml new file mode 100644 index 000000000000..3292a0440465 --- /dev/null +++ b/Documentation/devicetree/bindings/gpu/img,powervr.yaml @@ -0,0 +1,68 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +# Copyright (c) 2022 Imagination Technologies Ltd. +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/gpu/img,powervr.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Imagination Technologies PowerVR GPU + +maintainers: + - Sarah Walker <sarah.walker@imgtec.com> + +properties: + compatible: + items: + - enum: + - ti,am62-gpu + - const: img,powervr-seriesaxe + + reg: + maxItems: 1 + + clocks: + minItems: 1 + maxItems: 3 + + clock-names: + items: + - const: core + - const: mem + - const: sys + minItems: 1 + + interrupts: + items: + - description: GPU interrupt + + interrupt-names: + items: + - const: gpu + + power-domains: + maxItems: 1 + +required: + - compatible + - reg + - clocks + - clock-names + - interrupts + - interrupt-names + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + + gpu: gpu@fd00000 { + compatible = "ti,am62-gpu", "img,powervr-seriesaxe"; + reg = <0x0fd00000 0x20000>; + power-domains = <&some_pds 187>; + clocks = <&k3_clks 187 0>; + clock-names = "core"; + interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "gpu"; + }; diff --git a/MAINTAINERS b/MAINTAINERS index 9852d6bfdb95..0763388b31ef 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -10099,6 +10099,13 @@ IMGTEC IR DECODER DRIVER S: Orphan F: drivers/media/rc/img-ir/ +IMGTEC POWERVR DRM DRIVER +M: Frank Binns <frank.binns@imgtec.com> +M: Sarah Walker <sarah.walker@imgtec.com> +M: Donald Robson <donald.robson@imgtec.com> +S: Supported +F: Documentation/devicetree/bindings/gpu/img,powervr.yaml + IMON SOUNDGRAPH USB IR RECEIVER M: Sean Young <sean@mess.org> L: linux-media@vger.kernel.org
Add the device tree binding documentation for the Series AXE GPU used in TI AM62 SoCs. Changes since v3: - Remove oneOf in compatible property - Remove power-supply (not used on AM62) Changes since v2: - Add commit message description - Remove mt8173-gpu support (not currently supported) - Drop quotes from $id and $schema - Remove reg: minItems - Drop _clk suffixes from clock-names - Remove operating-points-v2 property and cooling-cells (not currently used) - Add additionalProperties: false - Remove stray blank line at the end of file Signed-off-by: Sarah Walker <sarah.walker@imgtec.com> --- .../devicetree/bindings/gpu/img,powervr.yaml | 68 +++++++++++++++++++ MAINTAINERS | 7 ++ 2 files changed, 75 insertions(+) create mode 100644 Documentation/devicetree/bindings/gpu/img,powervr.yaml