| Message ID | 20231004091552.3531659-4-hugues.fruchet@foss.st.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for video hardware codec of STMicroelectronics STM32 SoC series | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On Wed, Oct 4, 2023 at 4:16 AM Hugues Fruchet <hugues.fruchet@foss.st.com> wrote: > > Add STM32MP25 VENC video encoder bindings. > > Signed-off-by: Hugues Fruchet <hugues.fruchet@foss.st.com> > --- > .../bindings/media/st,stm32mp25-venc.yaml | 56 +++++++++++++++++++ > 1 file changed, 56 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/st,stm32mp25-venc.yaml > > diff --git a/Documentation/devicetree/bindings/media/st,stm32mp25-venc.yaml b/Documentation/devicetree/bindings/media/st,stm32mp25-venc.yaml > new file mode 100644 > index 000000000000..c69e0a34f675 > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/st,stm32mp25-venc.yaml > @@ -0,0 +1,56 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > + > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/media/st,stm32mp25-venc.yaml# Can this dt-binding be made more generic, like something like hantro-h1 or VC8000NanoE? I think there will be more boards that may incorporate the Hantro-H1 or a VC8000 in the future, because I don't think this IP is unique to the STM32MP25. adam > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: STMicroelectronics STM32MP25 VENC video encoder > + > +maintainers: > + - Hugues Fruchet <hugues.fruchet@foss.st.com> > + > +description: > + The STMicroelectronics STM32MP25 SOCs embeds a VENC video hardware encoder > + peripheral based on Verisilicon VC8000NanoE IP (former Hantro H1). > + > +properties: > + compatible: > + const: st,stm32mp25-venc > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + > + interrupt-names: > + maxItems: 1 > + > + clocks: > + maxItems: 1 > + > + clock-names: > + maxItems: 1 > + > +required: > + - compatible > + - reg > + - interrupts > + - interrupt-names > + - clocks > + - clock-names > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + venc: venc@580e0000 { > + compatible = "st,stm32mp25-venc"; > + reg = <0x580e0000 0x800>; > + interrupts = <GIC_SPI 167 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-names = "venc"; Is the interrupt-names needed if there is only one? > + clocks = <&ck_icn_p_venc>; > + clock-names = "venc-clk"; Same thing for the clock. if there is only one clock, doe they need names? adam > + }; > -- > 2.25.1 >
Hi Adam, Thanks for review, On 10/5/23 01:41, Adam Ford wrote: > On Wed, Oct 4, 2023 at 4:16 AM Hugues Fruchet > <hugues.fruchet@foss.st.com> wrote: >> >> Add STM32MP25 VENC video encoder bindings. >> >> Signed-off-by: Hugues Fruchet <hugues.fruchet@foss.st.com> >> --- >> .../bindings/media/st,stm32mp25-venc.yaml | 56 +++++++++++++++++++ >> 1 file changed, 56 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/media/st,stm32mp25-venc.yaml >> >> diff --git a/Documentation/devicetree/bindings/media/st,stm32mp25-venc.yaml b/Documentation/devicetree/bindings/media/st,stm32mp25-venc.yaml >> new file mode 100644 >> index 000000000000..c69e0a34f675 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/media/st,stm32mp25-venc.yaml >> @@ -0,0 +1,56 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> + >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/media/st,stm32mp25-venc.yaml# > > Can this dt-binding be made more generic, like something like > hantro-h1 or VC8000NanoE? > > I think there will be more boards that may incorporate the Hantro-H1 > or a VC8000 in the future, because I don't think this IP is unique to > the STM32MP25. This is already the case, check variants in hantro_drv.c. Several SoCs are sharing this IP but each IP slightly differs because of supported resolution, codec, preprocessing features, ... There are also some differences on how clock, interrupt, reset are hardware mapped: shared or not by decoder and encoder for ex. > > adam > >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: STMicroelectronics STM32MP25 VENC video encoder >> + >> +maintainers: >> + - Hugues Fruchet <hugues.fruchet@foss.st.com> >> + >> +description: >> + The STMicroelectronics STM32MP25 SOCs embeds a VENC video hardware encoder >> + peripheral based on Verisilicon VC8000NanoE IP (former Hantro H1). >> + >> +properties: >> + compatible: >> + const: st,stm32mp25-venc >> + >> + reg: >> + maxItems: 1 >> + >> + interrupts: >> + maxItems: 1 >> + >> + interrupt-names: >> + maxItems: 1 >> + >> + clocks: >> + maxItems: 1 >> + >> + clock-names: >> + maxItems: 1 >> + >> +required: >> + - compatible >> + - reg >> + - interrupts >> + - interrupt-names >> + - clocks >> + - clock-names >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/interrupt-controller/arm-gic.h> >> + venc: venc@580e0000 { >> + compatible = "st,stm32mp25-venc"; >> + reg = <0x580e0000 0x800>; >> + interrupts = <GIC_SPI 167 IRQ_TYPE_LEVEL_HIGH>; >> + interrupt-names = "venc"; > > > Is the interrupt-names needed if there is only one? > Not really, could be dropped. >> + clocks = <&ck_icn_p_venc>; >> + clock-names = "venc-clk"; > > Same thing for the clock. if there is only one clock, doe they need names? > Not really, could be dropped. > adam >> + }; >> -- >> 2.25.1 >> BR, Hugues.
On 04/10/2023 11:15, Hugues Fruchet wrote: > Add STM32MP25 VENC video encoder bindings. > I don't understand why this binding is separate from video decoder. Merge them. Best regards, Krzysztof
On Wed, Oct 04, 2023 at 06:41:09PM -0500, Adam Ford wrote: > On Wed, Oct 4, 2023 at 4:16 AM Hugues Fruchet > <hugues.fruchet@foss.st.com> wrote: > > > > Add STM32MP25 VENC video encoder bindings. > > > > Signed-off-by: Hugues Fruchet <hugues.fruchet@foss.st.com> > > --- > > .../bindings/media/st,stm32mp25-venc.yaml | 56 +++++++++++++++++++ > > 1 file changed, 56 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/media/st,stm32mp25-venc.yaml > > > > diff --git a/Documentation/devicetree/bindings/media/st,stm32mp25-venc.yaml b/Documentation/devicetree/bindings/media/st,stm32mp25-venc.yaml > > new file mode 100644 > > index 000000000000..c69e0a34f675 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/st,stm32mp25-venc.yaml > > @@ -0,0 +1,56 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > + > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/media/st,stm32mp25-venc.yaml# > > Can this dt-binding be made more generic, like something like > hantro-h1 or VC8000NanoE? > > I think there will be more boards that may incorporate the Hantro-H1 > or a VC8000 in the future, because I don't think this IP is unique to > the STM32MP25. Unless the underlying IP is well documented (i.e. public), then it's kind of pointless. Everyone will just invent their own numbers and names of clocks, resets, etc. unless someone can enforce not doing that. Rob
Hi Rob, On 10/6/23 18:27, Rob Herring wrote: > On Wed, Oct 04, 2023 at 06:41:09PM -0500, Adam Ford wrote: >> On Wed, Oct 4, 2023 at 4:16 AM Hugues Fruchet >> <hugues.fruchet@foss.st.com> wrote: >>> >>> Add STM32MP25 VENC video encoder bindings. >>> >>> Signed-off-by: Hugues Fruchet <hugues.fruchet@foss.st.com> >>> --- >>> .../bindings/media/st,stm32mp25-venc.yaml | 56 +++++++++++++++++++ >>> 1 file changed, 56 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/media/st,stm32mp25-venc.yaml >>> >>> diff --git a/Documentation/devicetree/bindings/media/st,stm32mp25-venc.yaml b/Documentation/devicetree/bindings/media/st,stm32mp25-venc.yaml >>> new file mode 100644 >>> index 000000000000..c69e0a34f675 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/media/st,stm32mp25-venc.yaml >>> @@ -0,0 +1,56 @@ >>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >>> + >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/media/st,stm32mp25-venc.yaml# >> >> Can this dt-binding be made more generic, like something like >> hantro-h1 or VC8000NanoE? >> >> I think there will be more boards that may incorporate the Hantro-H1 >> or a VC8000 in the future, because I don't think this IP is unique to >> the STM32MP25. > > Unless the underlying IP is well documented (i.e. public), then it's > kind of pointless. Everyone will just invent their own numbers and names > of clocks, resets, etc. unless someone can enforce not doing that. Unfortunately the IP documentation is not public, there are no documents provided publicly by Verisilicon for the time being. > > Rob BR, Hugues
Hi Krzysztof, On 10/5/23 21:45, Krzysztof Kozlowski wrote: > On 04/10/2023 11:15, Hugues Fruchet wrote: >> Add STM32MP25 VENC video encoder bindings. >> > > I don't understand why this binding is separate from video decoder. > Merge them. VDEC and VENC are two independent IPs with their own clock, reset, interrupt & register set, they have their own access to APB/AXI bus. Moreover future chipsets may embed only VENC or VDEC. Hoping that this clarifies the reason of two different bindings. > > Best regards, > Krzysztof > Br, Hugues.
On 09/10/2023 15:49, Hugues FRUCHET wrote: > Hi Krzysztof, > > On 10/5/23 21:45, Krzysztof Kozlowski wrote: >> On 04/10/2023 11:15, Hugues Fruchet wrote: >>> Add STM32MP25 VENC video encoder bindings. >>> >> >> I don't understand why this binding is separate from video decoder. >> Merge them. > VDEC and VENC are two independent IPs with their own clock, reset, > interrupt & register set, they have their own access to APB/AXI bus. > Moreover future chipsets may embed only VENC or VDEC. > > Hoping that this clarifies the reason of two different bindings. No, it does not. These are no reasons to have independent bindings, except when having actual impact on the bindings. The bindings look identical. What are the differences? Best regards, Krzysztof
Hi Krzysztof, On 10/9/23 15:56, Krzysztof Kozlowski wrote: > On 09/10/2023 15:49, Hugues FRUCHET wrote: >> Hi Krzysztof, >> >> On 10/5/23 21:45, Krzysztof Kozlowski wrote: >>> On 04/10/2023 11:15, Hugues Fruchet wrote: >>>> Add STM32MP25 VENC video encoder bindings. >>>> >>> >>> I don't understand why this binding is separate from video decoder. >>> Merge them. >> VDEC and VENC are two independent IPs with their own clock, reset, >> interrupt & register set, they have their own access to APB/AXI bus. >> Moreover future chipsets may embed only VENC or VDEC. >> >> Hoping that this clarifies the reason of two different bindings. > > No, it does not. These are no reasons to have independent bindings, > except when having actual impact on the bindings. The bindings look > identical. What are the differences? I'm sorry but I really don't understand your point, these are two different IPs with very different registers in it, so why should I share that in a single binding ? > > Best regards, > Krzysztof > BR, Hugues.
On 09/10/2023 16:24, Hugues FRUCHET wrote: > Hi Krzysztof, > > On 10/9/23 15:56, Krzysztof Kozlowski wrote: >> On 09/10/2023 15:49, Hugues FRUCHET wrote: >>> Hi Krzysztof, >>> >>> On 10/5/23 21:45, Krzysztof Kozlowski wrote: >>>> On 04/10/2023 11:15, Hugues Fruchet wrote: >>>>> Add STM32MP25 VENC video encoder bindings. >>>>> >>>> >>>> I don't understand why this binding is separate from video decoder. >>>> Merge them. >>> VDEC and VENC are two independent IPs with their own clock, reset, >>> interrupt & register set, they have their own access to APB/AXI bus. >>> Moreover future chipsets may embed only VENC or VDEC. >>> >>> Hoping that this clarifies the reason of two different bindings. >> >> No, it does not. These are no reasons to have independent bindings, >> except when having actual impact on the bindings. The bindings look >> identical. What are the differences? > I'm sorry but I really don't understand your point, these are two > different IPs with very different registers in it, so why should > I share that in a single binding ? Because the binding is identical. If not, maybe I missed something, so please point me to differences in the binding. Best regards, Krzysztof
Hi Krzysztof, On 10/9/23 16:28, Krzysztof Kozlowski wrote: > On 09/10/2023 16:24, Hugues FRUCHET wrote: >> Hi Krzysztof, >> >> On 10/9/23 15:56, Krzysztof Kozlowski wrote: >>> On 09/10/2023 15:49, Hugues FRUCHET wrote: >>>> Hi Krzysztof, >>>> >>>> On 10/5/23 21:45, Krzysztof Kozlowski wrote: >>>>> On 04/10/2023 11:15, Hugues Fruchet wrote: >>>>>> Add STM32MP25 VENC video encoder bindings. >>>>>> >>>>> >>>>> I don't understand why this binding is separate from video decoder. >>>>> Merge them. >>>> VDEC and VENC are two independent IPs with their own clock, reset, >>>> interrupt & register set, they have their own access to APB/AXI bus. >>>> Moreover future chipsets may embed only VENC or VDEC. >>>> >>>> Hoping that this clarifies the reason of two different bindings. >>> >>> No, it does not. These are no reasons to have independent bindings, >>> except when having actual impact on the bindings. The bindings look >>> identical. What are the differences? >> I'm sorry but I really don't understand your point, these are two >> different IPs with very different registers in it, so why should >> I share that in a single binding ? > > Because the binding is identical. If not, maybe I missed something, so > please point me to differences in the binding. OK, currently they are identical so I will merge into a single one even if I disagree on that. I hope that in future this will not change otherwise I'll need to revisit that and make separate bindings as initially proposed... I'll so push a v2 with merged version proposal. > > Best regards, > Krzysztof > BR, Hugues.
diff --git a/Documentation/devicetree/bindings/media/st,stm32mp25-venc.yaml b/Documentation/devicetree/bindings/media/st,stm32mp25-venc.yaml new file mode 100644 index 000000000000..c69e0a34f675 --- /dev/null +++ b/Documentation/devicetree/bindings/media/st,stm32mp25-venc.yaml @@ -0,0 +1,56 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) + +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/st,stm32mp25-venc.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: STMicroelectronics STM32MP25 VENC video encoder + +maintainers: + - Hugues Fruchet <hugues.fruchet@foss.st.com> + +description: + The STMicroelectronics STM32MP25 SOCs embeds a VENC video hardware encoder + peripheral based on Verisilicon VC8000NanoE IP (former Hantro H1). + +properties: + compatible: + const: st,stm32mp25-venc + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + interrupt-names: + maxItems: 1 + + clocks: + maxItems: 1 + + clock-names: + maxItems: 1 + +required: + - compatible + - reg + - interrupts + - interrupt-names + - clocks + - clock-names + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + venc: venc@580e0000 { + compatible = "st,stm32mp25-venc"; + reg = <0x580e0000 0x800>; + interrupts = <GIC_SPI 167 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "venc"; + clocks = <&ck_icn_p_venc>; + clock-names = "venc-clk"; + };
Add STM32MP25 VENC video encoder bindings. Signed-off-by: Hugues Fruchet <hugues.fruchet@foss.st.com> --- .../bindings/media/st,stm32mp25-venc.yaml | 56 +++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/st,stm32mp25-venc.yaml