| Message ID | 20210311154055.3496076-7-emil.l.velikov@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Microship SAMA5D4 VPU support et al | expand |
On Thu, Mar 11, 2021 at 03:40:51PM +0000, Emil Velikov wrote: > From: Emil Velikov <emil.velikov@collabora.com> > > Add devicetree binding documentation for the Hantro G1/G2 VDEC on > the Microchip SAMAS5D4 SoC. > > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Frank Rowand <frowand.list@gmail.com> > Cc: devicetree@vger.kernel.org> > Signed-off-by: Emil Velikov <emil.velikov@collabora.com> > --- > v2 > - Newly introduced > - s/Atmel/Microchip/ (Nicolas) > - Drop leading 0 in node name/address > --- > .../media/microchip,sama5d4-vdec.yaml | 59 +++++++++++++++++++ > 1 file changed, 59 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml > > diff --git a/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml b/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml > new file mode 100644 > index 000000000000..9cb2c0295d54 > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml > @@ -0,0 +1,59 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > + > +%YAML 1.2 > +--- > +$id: "http://devicetree.org/schemas/media/microchip,sama5d4-vdec.yaml#" > +$schema: "http://devicetree.org/meta-schemas/core.yaml#" > + > +title: Hantro G1 VPU codec implemented on Microchip SAMA5D4 SoCs > + > +maintainers: > + - Emil Velikov <emil.velikov@collabora.com> > + > +description: > + Hantro G1 video decode accelerator present on Microchip SAMA5D4 SoCs. > + > +properties: > + compatible: > + const: microchip,sama5d4-vdec > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + > + interrupt-names: > + items: > + - const: vdec Why do you need a name? *-names are used to distinguish multiple entries and don't add anything if only a single entry. > + > + clocks: > + maxItems: 1 > + > + clock-names: > + items: > + - const: vdec_clk And here too? These are typically named for either the name of input signal (hclk, aclk, etc.) or function ('core', 'bus', etc.). > + > +required: > + - compatible > + - reg > + - interrupts > + - interrupt-names > + - clocks > + - clock-names > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/clock/at91.h> > + #include <dt-bindings/interrupt-controller/irq.h> > + > + vdec0: vdec@300000 { > + compatible = "microchip,sama5d4-vdec"; > + reg = <0x00300000 0x100000>; > + interrupts = <19 IRQ_TYPE_LEVEL_HIGH 4>; > + interrupt-names = "vdec"; > + clocks = <&pmc PMC_TYPE_PERIPHERAL 19>; > + clock-names = "vdec_clk"; > + }; > -- > 2.30.1 >
On Wed, 24 Mar 2021 at 15:17, Rob Herring <robh@kernel.org> wrote: > > On Thu, Mar 11, 2021 at 03:40:51PM +0000, Emil Velikov wrote: > > From: Emil Velikov <emil.velikov@collabora.com> > > > > Add devicetree binding documentation for the Hantro G1/G2 VDEC on > > the Microchip SAMAS5D4 SoC. > > > > Cc: Rob Herring <robh+dt@kernel.org> > > Cc: Frank Rowand <frowand.list@gmail.com> > > Cc: devicetree@vger.kernel.org> > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com> > > --- > > v2 > > - Newly introduced > > - s/Atmel/Microchip/ (Nicolas) > > - Drop leading 0 in node name/address > > --- > > .../media/microchip,sama5d4-vdec.yaml | 59 +++++++++++++++++++ > > 1 file changed, 59 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml > > > > diff --git a/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml b/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml > > new file mode 100644 > > index 000000000000..9cb2c0295d54 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml > > @@ -0,0 +1,59 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > + > > +%YAML 1.2 > > +--- > > +$id: "http://devicetree.org/schemas/media/microchip,sama5d4-vdec.yaml#" > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#" > > + > > +title: Hantro G1 VPU codec implemented on Microchip SAMA5D4 SoCs > > + > > +maintainers: > > + - Emil Velikov <emil.velikov@collabora.com> > > + > > +description: > > + Hantro G1 video decode accelerator present on Microchip SAMA5D4 SoCs. > > + > > +properties: > > + compatible: > > + const: microchip,sama5d4-vdec > > + > > + reg: > > + maxItems: 1 > > + > > + interrupts: > > + maxItems: 1 > > + > > + interrupt-names: > > + items: > > + - const: vdec > > Why do you need a name? *-names are used to distinguish multiple entries > and don't add anything if only a single entry. > On one hand the names are used to describe the hardware - the SAMA board uses designated clock and interrupt lines - both called "vdec". Additionally the names are ultimately required by the underlying API - platform_get_irq_byname and devm_clk_bulk_get respectively. How can we get the respective entries without the name? Skimming through the existing dts file - arch/arm/boot/dts/sama5d4.dtsi and other dts files - there are lots of examples where the device tree binding contains the name for a single clock/interrupt. Thanks Emil
Hi Emil, On Thu, 2021-03-25 at 14:14 +0000, Emil Velikov wrote: > On Wed, 24 Mar 2021 at 15:17, Rob Herring <robh@kernel.org> wrote: > > > > On Thu, Mar 11, 2021 at 03:40:51PM +0000, Emil Velikov wrote: > > > From: Emil Velikov <emil.velikov@collabora.com> > > > > > > Add devicetree binding documentation for the Hantro G1/G2 VDEC on > > > the Microchip SAMAS5D4 SoC. > > > > > > Cc: Rob Herring <robh+dt@kernel.org> > > > Cc: Frank Rowand <frowand.list@gmail.com> > > > Cc: devicetree@vger.kernel.org> > > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com> > > > --- > > > v2 > > > - Newly introduced > > > - s/Atmel/Microchip/ (Nicolas) > > > - Drop leading 0 in node name/address > > > --- > > > .../media/microchip,sama5d4-vdec.yaml | 59 +++++++++++++++++++ > > > 1 file changed, 59 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml b/Documentation/devicetree/bindings/media/microchip,sama5d4- > > > vdec.yaml > > > new file mode 100644 > > > index 000000000000..9cb2c0295d54 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml > > > @@ -0,0 +1,59 @@ > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > > + > > > +%YAML 1.2 > > > +--- > > > +$id: "http://devicetree.org/schemas/media/microchip,sama5d4-vdec.yaml#" > > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#" > > > + > > > +title: Hantro G1 VPU codec implemented on Microchip SAMA5D4 SoCs > > > + > > > +maintainers: > > > + - Emil Velikov <emil.velikov@collabora.com> > > > + > > > +description: > > > + Hantro G1 video decode accelerator present on Microchip SAMA5D4 SoCs. > > > + > > > +properties: > > > + compatible: > > > + const: microchip,sama5d4-vdec > > > + > > > + reg: > > > + maxItems: 1 > > > + > > > + interrupts: > > > + maxItems: 1 > > > + > > > + interrupt-names: > > > + items: > > > + - const: vdec > > > > Why do you need a name? *-names are used to distinguish multiple entries > > and don't add anything if only a single entry. > > > On one hand the names are used to describe the hardware - the SAMA > board uses designated clock and interrupt lines - both called "vdec". > Additionally the names are ultimately required by the underlying API - > platform_get_irq_byname and devm_clk_bulk_get respectively. > How can we get the respective entries without the name? > There are APIs to get the resource by index as opposed to by name. E.g. platform_get_irq. However, you are confusing here internal kernel implementation with devicetree bindings. The latter is just a representation of the hardware. > Skimming through the existing dts file - > arch/arm/boot/dts/sama5d4.dtsi and other dts files - there are lots of > examples where the device tree binding contains the name for a single > clock/interrupt. > Note that dtsi and dts are the device-tree files, not the bindings. And even if you find examples, the fact that there exist examples doesn't mean it's the right thing to do. Thanks, Ezequiel
Hi Rob, Thanks for reviewing. On Wed, 2021-03-24 at 09:17 -0600, Rob Herring wrote: > On Thu, Mar 11, 2021 at 03:40:51PM +0000, Emil Velikov wrote: > > From: Emil Velikov <emil.velikov@collabora.com> > > > > Add devicetree binding documentation for the Hantro G1/G2 VDEC on > > the Microchip SAMAS5D4 SoC. > > > > Cc: Rob Herring <robh+dt@kernel.org> > > Cc: Frank Rowand <frowand.list@gmail.com> > > Cc: devicetree@vger.kernel.org> > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com> > > --- > > v2 > > - Newly introduced > > - s/Atmel/Microchip/ (Nicolas) > > - Drop leading 0 in node name/address > > --- > > .../media/microchip,sama5d4-vdec.yaml | 59 +++++++++++++++++++ > > 1 file changed, 59 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml > > > > diff --git a/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml b/Documentation/devicetree/bindings/media/microchip,sama5d4- > > vdec.yaml > > new file mode 100644 > > index 000000000000..9cb2c0295d54 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml > > @@ -0,0 +1,59 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > + > > +%YAML 1.2 > > +--- > > +$id: "http://devicetree.org/schemas/media/microchip,sama5d4-vdec.yaml#" > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#" > > + > > +title: Hantro G1 VPU codec implemented on Microchip SAMA5D4 SoCs > > + > > +maintainers: > > + - Emil Velikov <emil.velikov@collabora.com> > > + > > +description: > > + Hantro G1 video decode accelerator present on Microchip SAMA5D4 SoCs. > > + > > +properties: > > + compatible: > > + const: microchip,sama5d4-vdec > > + > > + reg: > > + maxItems: 1 > > + > > + interrupts: > > + maxItems: 1 > > + > > + interrupt-names: > > + items: > > + - const: vdec > > Why do you need a name? *-names are used to distinguish multiple entries > and don't add anything if only a single entry. > I guess you are right. I was about to argue that it makes backwards compatibility easier, but I suspect that's not the case either. > > + > > + clocks: > > + maxItems: 1 > > + > > + clock-names: > > + items: > > + - const: vdec_clk > > And here too? These are typically named for either the name of input > signal (hclk, aclk, etc.) or function ('core', 'bus', etc.). > Indeed, "core" might be a better choice. But it seems we don't need names here. Thanks, Ezequiel
On Fri, 26 Mar 2021 at 14:29, Ezequiel Garcia <ezequiel@collabora.com> wrote: > > Hi Emil, > > On Thu, 2021-03-25 at 14:14 +0000, Emil Velikov wrote: > > On Wed, 24 Mar 2021 at 15:17, Rob Herring <robh@kernel.org> wrote: > > > > > > On Thu, Mar 11, 2021 at 03:40:51PM +0000, Emil Velikov wrote: > > > > From: Emil Velikov <emil.velikov@collabora.com> > > > > > > > > Add devicetree binding documentation for the Hantro G1/G2 VDEC on > > > > the Microchip SAMAS5D4 SoC. > > > > > > > > Cc: Rob Herring <robh+dt@kernel.org> > > > > Cc: Frank Rowand <frowand.list@gmail.com> > > > > Cc: devicetree@vger.kernel.org> > > > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com> > > > > --- > > > > v2 > > > > - Newly introduced > > > > - s/Atmel/Microchip/ (Nicolas) > > > > - Drop leading 0 in node name/address > > > > --- > > > > .../media/microchip,sama5d4-vdec.yaml | 59 +++++++++++++++++++ > > > > 1 file changed, 59 insertions(+) > > > > create mode 100644 Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml > > > > > > > > diff --git a/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml b/Documentation/devicetree/bindings/media/microchip,sama5d4- > > > > vdec.yaml > > > > new file mode 100644 > > > > index 000000000000..9cb2c0295d54 > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml > > > > @@ -0,0 +1,59 @@ > > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > > > + > > > > +%YAML 1.2 > > > > +--- > > > > +$id: "http://devicetree.org/schemas/media/microchip,sama5d4-vdec.yaml#" > > > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#" > > > > + > > > > +title: Hantro G1 VPU codec implemented on Microchip SAMA5D4 SoCs > > > > + > > > > +maintainers: > > > > + - Emil Velikov <emil.velikov@collabora.com> > > > > + > > > > +description: > > > > + Hantro G1 video decode accelerator present on Microchip SAMA5D4 SoCs. > > > > + > > > > +properties: > > > > + compatible: > > > > + const: microchip,sama5d4-vdec > > > > + > > > > + reg: > > > > + maxItems: 1 > > > > + > > > > + interrupts: > > > > + maxItems: 1 > > > > + > > > > + interrupt-names: > > > > + items: > > > > + - const: vdec > > > > > > Why do you need a name? *-names are used to distinguish multiple entries > > > and don't add anything if only a single entry. > > > > > On one hand the names are used to describe the hardware - the SAMA > > board uses designated clock and interrupt lines - both called "vdec". > > Additionally the names are ultimately required by the underlying API - > > platform_get_irq_byname and devm_clk_bulk_get respectively. > > How can we get the respective entries without the name? > > > > There are APIs to get the resource by index as opposed to by name. > E.g. platform_get_irq. > > However, you are confusing here internal kernel implementation > with devicetree bindings. The latter is just a representation > of the hardware. > My train of thought was that if we're missing API to fetch the entries via non-name means, then there's a reasonable precedent to expose the name. Clearly I missed the API, so my argument is just off. > > Skimming through the existing dts file - > > arch/arm/boot/dts/sama5d4.dtsi and other dts files - there are lots of > > examples where the device tree binding contains the name for a single > > clock/interrupt. > > > > > Note that dtsi and dts are the device-tree files, not the bindings. > And even if you find examples, the fact that there exist examples > doesn't mean it's the right thing to do. > Fully agreed. Thanks for the correction and hints. The v3, just submitted, lacks the names all together. -Emil
diff --git a/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml b/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml new file mode 100644 index 000000000000..9cb2c0295d54 --- /dev/null +++ b/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml @@ -0,0 +1,59 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) + +%YAML 1.2 +--- +$id: "http://devicetree.org/schemas/media/microchip,sama5d4-vdec.yaml#" +$schema: "http://devicetree.org/meta-schemas/core.yaml#" + +title: Hantro G1 VPU codec implemented on Microchip SAMA5D4 SoCs + +maintainers: + - Emil Velikov <emil.velikov@collabora.com> + +description: + Hantro G1 video decode accelerator present on Microchip SAMA5D4 SoCs. + +properties: + compatible: + const: microchip,sama5d4-vdec + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + interrupt-names: + items: + - const: vdec + + clocks: + maxItems: 1 + + clock-names: + items: + - const: vdec_clk + +required: + - compatible + - reg + - interrupts + - interrupt-names + - clocks + - clock-names + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/at91.h> + #include <dt-bindings/interrupt-controller/irq.h> + + vdec0: vdec@300000 { + compatible = "microchip,sama5d4-vdec"; + reg = <0x00300000 0x100000>; + interrupts = <19 IRQ_TYPE_LEVEL_HIGH 4>; + interrupt-names = "vdec"; + clocks = <&pmc PMC_TYPE_PERIPHERAL 19>; + clock-names = "vdec_clk"; + };