LinuxTV Patchwork [v2,1/3] media: dt-bindings: media: document allegro-dvt bindings

login
register
mail settings
Submitter Michael Tretter
Date Jan. 18, 2019, 1:37 p.m.
Message ID <20190118133716.29288-2-m.tretter@pengutronix.de>
Download mbox | patch
Permalink /patch/54017/
State Superseded
Delegated to: Hans Verkuil
Headers show

Comments

Michael Tretter - Jan. 18, 2019, 1:37 p.m.
Add device-tree bindings for the Allegro DVT video IP core found on the
Xilinx ZynqMP EV family.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
Changes since v1:
none

---
 .../devicetree/bindings/media/allegro.txt     | 35 +++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/allegro.txt
Philipp Zabel - Jan. 21, 2019, 10:59 a.m.
On Fri, 2019-01-18 at 14:37 +0100, Michael Tretter wrote:
> Add device-tree bindings for the Allegro DVT video IP core found on the
> Xilinx ZynqMP EV family.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
> Changes since v1:
> none
> 
> ---
>  .../devicetree/bindings/media/allegro.txt     | 35 +++++++++++++++++++
>  1 file changed, 35 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/allegro.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/allegro.txt b/Documentation/devicetree/bindings/media/allegro.txt
> new file mode 100644
> index 000000000000..765f4b0c1a57
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/allegro.txt
> @@ -0,0 +1,35 @@
> +Device-tree bindings for the Allegro DVT video IP codecs present in the Xilinx
> +ZynqMP SoC. The IP core may either be a H.264/H.265 encoder or H.264/H.265
> +decoder ip core.
> +
> +Each actual codec engines is controlled by a microcontroller (MCU). Host
> +software uses a provided mailbox interface to communicate with the MCU. The
> +MCU share an interrupt.
> +
> +Required properties:
> +  - compatible: value should be one of the following
> +    "allegro,al5e-1.1", "allegro,al5e": encoder IP core
> +    "allegro,al5d-1.1", "allegro,al5d": decoder IP core
> +  - reg: base and length of the memory mapped register region and base and
> +    length of the memory mapped sram
> +  - reg-names: must include "regs" and "sram"
> +  - interrupts: shared interrupt from the MCUs to the processing system
> +  - interrupt-names: "vcu_host_interrupt"
> +
> +Example:
> +	al5e: al5e@a0009000 {

Should the node names be "vpu" or "video-codec"?

> +		compatible = "allegro,al5e";
> +		reg = <0 0xa0009000 0 0x1000>,
> +		      <0 0xa0000000 0 0x8000>;
> +		reg-names = "regs", "sram";
> +		interrupt-names = "vcu_host_interrupt";
> +		interrupts = <0 96 4>;
> +	};
> +	al5d: al5d@a0029000 {
> +		compatible = "allegro,al5d";
> +		reg = <0 0xa0029000 0 0x1000>,
> +		      <0 0xa0020000 0 0x8000>;
> +		reg-names = "regs", "sram";
> +		interrupt-names = "vcu_host_interrupt";
> +		interrupts = <0 96 4>;
> +	};

regards
Philipp
Nicolas Dufresne - Jan. 21, 2019, 4:17 p.m.
Le lundi 21 janvier 2019 à 11:59 +0100, Philipp Zabel a écrit :
> On Fri, 2019-01-18 at 14:37 +0100, Michael Tretter wrote:
> > Add device-tree bindings for the Allegro DVT video IP core found on the
> > Xilinx ZynqMP EV family.
> > 
> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > ---
> > Changes since v1:
> > none
> > 
> > ---
> >  .../devicetree/bindings/media/allegro.txt     | 35 +++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/allegro.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/media/allegro.txt b/Documentation/devicetree/bindings/media/allegro.txt
> > new file mode 100644
> > index 000000000000..765f4b0c1a57
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/allegro.txt
> > @@ -0,0 +1,35 @@
> > +Device-tree bindings for the Allegro DVT video IP codecs present in the Xilinx
> > +ZynqMP SoC. The IP core may either be a H.264/H.265 encoder or H.264/H.265
> > +decoder ip core.
> > +
> > +Each actual codec engines is controlled by a microcontroller (MCU). Host
> > +software uses a provided mailbox interface to communicate with the MCU. The
> > +MCU share an interrupt.
> > +
> > +Required properties:
> > +  - compatible: value should be one of the following
> > +    "allegro,al5e-1.1", "allegro,al5e": encoder IP core
> > +    "allegro,al5d-1.1", "allegro,al5d": decoder IP core
> > +  - reg: base and length of the memory mapped register region and base and
> > +    length of the memory mapped sram
> > +  - reg-names: must include "regs" and "sram"
> > +  - interrupts: shared interrupt from the MCUs to the processing system
> > +  - interrupt-names: "vcu_host_interrupt"
> > +
> > +Example:
> > +	al5e: al5e@a0009000 {
> 
> Should the node names be "vpu" or "video-codec"?

Xilinx calls this IP the "vcu", so "vpu" would be even more confusing.
Was this just a typo ? That being said, is this referring to the actual
HW or the firmware that runs on a microblaze (the firmware being
Allegro specific) ?

> 
> > +		compatible = "allegro,al5e";
> > +		reg = <0 0xa0009000 0 0x1000>,
> > +		      <0 0xa0000000 0 0x8000>;
> > +		reg-names = "regs", "sram";
> > +		interrupt-names = "vcu_host_interrupt";
> > +		interrupts = <0 96 4>;
> > +	};
> > +	al5d: al5d@a0029000 {
> > +		compatible = "allegro,al5d";
> > +		reg = <0 0xa0029000 0 0x1000>,
> > +		      <0 0xa0020000 0 0x8000>;
> > +		reg-names = "regs", "sram";
> > +		interrupt-names = "vcu_host_interrupt";
> > +		interrupts = <0 96 4>;
> > +	};
> 
> regards
> Philipp
Philipp Zabel - Jan. 21, 2019, 4:30 p.m.
On Mon, 2019-01-21 at 11:17 -0500, Nicolas Dufresne wrote:
[...]
> > > +Example:
> > > +	al5e: al5e@a0009000 {
> > 
> > Should the node names be "vpu" or "video-codec"?
> 
> Xilinx calls this IP the "vcu", so "vpu" would be even more confusing.
> Was this just a typo ?

I was just going by what is already commonly used in other device trees.
Given that "vpu" doesn't seem to be as universally accepted as for
example "gpu", maybe "video-codec" is the better choice?

In any case, it should not matter what the the IP vendor or the SoC
vendor call this, the node name should be common and descriptive of its
function, same as ethernet controller nodes should be called "ethernet".

> That being said, is this referring to the actual
> HW or the firmware that runs on a microblaze (the firmware being
> Allegro specific) ?

As I understand it this binding is referring to the whole [microblaze +
attached encoder/decoder hardware] setup that is supposed to work with
the Allegro specific firmware.

regards
Philipp
Rob Herring - Jan. 21, 2019, 5:13 p.m.
On Fri, Jan 18, 2019 at 02:37:14PM +0100, Michael Tretter wrote:
> Add device-tree bindings for the Allegro DVT video IP core found on the
> Xilinx ZynqMP EV family.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
> Changes since v1:
> none
> 
> ---
>  .../devicetree/bindings/media/allegro.txt     | 35 +++++++++++++++++++
>  1 file changed, 35 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/allegro.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/allegro.txt b/Documentation/devicetree/bindings/media/allegro.txt
> new file mode 100644
> index 000000000000..765f4b0c1a57
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/allegro.txt
> @@ -0,0 +1,35 @@
> +Device-tree bindings for the Allegro DVT video IP codecs present in the Xilinx
> +ZynqMP SoC. The IP core may either be a H.264/H.265 encoder or H.264/H.265
> +decoder ip core.
> +
> +Each actual codec engines is controlled by a microcontroller (MCU). Host
> +software uses a provided mailbox interface to communicate with the MCU. The
> +MCU share an interrupt.
> +
> +Required properties:
> +  - compatible: value should be one of the following
> +    "allegro,al5e-1.1", "allegro,al5e": encoder IP core
> +    "allegro,al5d-1.1", "allegro,al5d": decoder IP core
> +  - reg: base and length of the memory mapped register region and base and
> +    length of the memory mapped sram
> +  - reg-names: must include "regs" and "sram"
> +  - interrupts: shared interrupt from the MCUs to the processing system
> +  - interrupt-names: "vcu_host_interrupt"

No point in having *-names when there is only one entry.

> +
> +Example:
> +	al5e: al5e@a0009000 {

video-codec as suggested.

> +		compatible = "allegro,al5e";

Doesn't match the documentation above.

> +		reg = <0 0xa0009000 0 0x1000>,
> +		      <0 0xa0000000 0 0x8000>;
> +		reg-names = "regs", "sram";
> +		interrupt-names = "vcu_host_interrupt";
> +		interrupts = <0 96 4>;
> +	};
> +	al5d: al5d@a0029000 {
> +		compatible = "allegro,al5d";
> +		reg = <0 0xa0029000 0 0x1000>,
> +		      <0 0xa0020000 0 0x8000>;
> +		reg-names = "regs", "sram";
> +		interrupt-names = "vcu_host_interrupt";
> +		interrupts = <0 96 4>;
> +	};
> -- 
> 2.20.1
>
Michael Tretter - Jan. 21, 2019, 5:42 p.m.
On Mon, 21 Jan 2019 11:17:43 -0500, Nicolas Dufresne wrote:
> Le lundi 21 janvier 2019 à 11:59 +0100, Philipp Zabel a écrit :
> > On Fri, 2019-01-18 at 14:37 +0100, Michael Tretter wrote:  
> > > Add device-tree bindings for the Allegro DVT video IP core found on the
> > > Xilinx ZynqMP EV family.
> > > 
> > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > > ---
> > > Changes since v1:
> > > none
> > > 
> > > ---
> > >  .../devicetree/bindings/media/allegro.txt     | 35 +++++++++++++++++++
> > >  1 file changed, 35 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/media/allegro.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/media/allegro.txt b/Documentation/devicetree/bindings/media/allegro.txt
> > > new file mode 100644
> > > index 000000000000..765f4b0c1a57
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/allegro.txt
> > > @@ -0,0 +1,35 @@
> > > +Device-tree bindings for the Allegro DVT video IP codecs present in the Xilinx
> > > +ZynqMP SoC. The IP core may either be a H.264/H.265 encoder or H.264/H.265
> > > +decoder ip core.
> > > +
> > > +Each actual codec engines is controlled by a microcontroller (MCU). Host
> > > +software uses a provided mailbox interface to communicate with the MCU. The
> > > +MCU share an interrupt.
> > > +
> > > +Required properties:
> > > +  - compatible: value should be one of the following
> > > +    "allegro,al5e-1.1", "allegro,al5e": encoder IP core
> > > +    "allegro,al5d-1.1", "allegro,al5d": decoder IP core
> > > +  - reg: base and length of the memory mapped register region and base and
> > > +    length of the memory mapped sram
> > > +  - reg-names: must include "regs" and "sram"
> > > +  - interrupts: shared interrupt from the MCUs to the processing system
> > > +  - interrupt-names: "vcu_host_interrupt"
> > > +
> > > +Example:
> > > +	al5e: al5e@a0009000 {  
> > 
> > Should the node names be "vpu" or "video-codec"?  
> 
> Xilinx calls this IP the "vcu", so "vpu" would be even more confusing.

The term vcu, as used by Xilinx, includes the encoder, decoder, both
microblaze processors, and the "VCU Settings" module. The already
existing "xlnx,vcu" binding refers to this "VCU Settings" module (or
"VCU System-Level Control" or "LogicoreIP").

> Was this just a typo ? That being said, is this referring to the actual
> HW or the firmware that runs on a microblaze (the firmware being
> Allegro specific) ?

The binding refers to actual hardware, i.e., encoder + microblaze or
decoder + microblaze. My understanding is that the microblaze is a
generic microblaze, but is integrated with the Allegro specific
encoder/decoder IP into a single codec IP block.

Michael

> 
> >   
> > > +		compatible = "allegro,al5e";
> > > +		reg = <0 0xa0009000 0 0x1000>,
> > > +		      <0 0xa0000000 0 0x8000>;
> > > +		reg-names = "regs", "sram";
> > > +		interrupt-names = "vcu_host_interrupt";
> > > +		interrupts = <0 96 4>;
> > > +	};
> > > +	al5d: al5d@a0029000 {
> > > +		compatible = "allegro,al5d";
> > > +		reg = <0 0xa0029000 0 0x1000>,
> > > +		      <0 0xa0020000 0 0x8000>;
> > > +		reg-names = "regs", "sram";
> > > +		interrupt-names = "vcu_host_interrupt";
> > > +		interrupts = <0 96 4>;
> > > +	};  
> > 
> > regards
> > Philipp
Michael Tretter - Jan. 22, 2019, 1:38 p.m.
On Mon, 21 Jan 2019 11:13:48 -0600, Rob Herring wrote:
> On Fri, Jan 18, 2019 at 02:37:14PM +0100, Michael Tretter wrote:
> > Add device-tree bindings for the Allegro DVT video IP core found on the
> > Xilinx ZynqMP EV family.
> > 
> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > ---
> > Changes since v1:
> > none
> > 
> > ---
> >  .../devicetree/bindings/media/allegro.txt     | 35 +++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/allegro.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/media/allegro.txt b/Documentation/devicetree/bindings/media/allegro.txt
> > new file mode 100644
> > index 000000000000..765f4b0c1a57
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/allegro.txt
> > @@ -0,0 +1,35 @@
> > +Device-tree bindings for the Allegro DVT video IP codecs present in the Xilinx
> > +ZynqMP SoC. The IP core may either be a H.264/H.265 encoder or H.264/H.265
> > +decoder ip core.
> > +
> > +Each actual codec engines is controlled by a microcontroller (MCU). Host
> > +software uses a provided mailbox interface to communicate with the MCU. The
> > +MCU share an interrupt.
> > +
> > +Required properties:
> > +  - compatible: value should be one of the following
> > +    "allegro,al5e-1.1", "allegro,al5e": encoder IP core
> > +    "allegro,al5d-1.1", "allegro,al5d": decoder IP core
> > +  - reg: base and length of the memory mapped register region and base and
> > +    length of the memory mapped sram
> > +  - reg-names: must include "regs" and "sram"
> > +  - interrupts: shared interrupt from the MCUs to the processing system
> > +  - interrupt-names: "vcu_host_interrupt"  
> 
> No point in having *-names when there is only one entry.
> 
> > +
> > +Example:
> > +	al5e: al5e@a0009000 {  
> 
> video-codec as suggested.
> 
> > +		compatible = "allegro,al5e";  
> 
> Doesn't match the documentation above.

Where is the mismatch? "allegro,al5e" is one of the allowed compatible
strings that are specified above.

Michael

> 
> > +		reg = <0 0xa0009000 0 0x1000>,
> > +		      <0 0xa0000000 0 0x8000>;
> > +		reg-names = "regs", "sram";
> > +		interrupt-names = "vcu_host_interrupt";
> > +		interrupts = <0 96 4>;
> > +	};
> > +	al5d: al5d@a0029000 {
> > +		compatible = "allegro,al5d";
> > +		reg = <0 0xa0029000 0 0x1000>,
> > +		      <0 0xa0020000 0 0x8000>;
> > +		reg-names = "regs", "sram";
> > +		interrupt-names = "vcu_host_interrupt";
> > +		interrupts = <0 96 4>;
> > +	};
> > -- 
> > 2.20.1
> >   
>

Patch

diff --git a/Documentation/devicetree/bindings/media/allegro.txt b/Documentation/devicetree/bindings/media/allegro.txt
new file mode 100644
index 000000000000..765f4b0c1a57
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/allegro.txt
@@ -0,0 +1,35 @@ 
+Device-tree bindings for the Allegro DVT video IP codecs present in the Xilinx
+ZynqMP SoC. The IP core may either be a H.264/H.265 encoder or H.264/H.265
+decoder ip core.
+
+Each actual codec engines is controlled by a microcontroller (MCU). Host
+software uses a provided mailbox interface to communicate with the MCU. The
+MCU share an interrupt.
+
+Required properties:
+  - compatible: value should be one of the following
+    "allegro,al5e-1.1", "allegro,al5e": encoder IP core
+    "allegro,al5d-1.1", "allegro,al5d": decoder IP core
+  - reg: base and length of the memory mapped register region and base and
+    length of the memory mapped sram
+  - reg-names: must include "regs" and "sram"
+  - interrupts: shared interrupt from the MCUs to the processing system
+  - interrupt-names: "vcu_host_interrupt"
+
+Example:
+	al5e: al5e@a0009000 {
+		compatible = "allegro,al5e";
+		reg = <0 0xa0009000 0 0x1000>,
+		      <0 0xa0000000 0 0x8000>;
+		reg-names = "regs", "sram";
+		interrupt-names = "vcu_host_interrupt";
+		interrupts = <0 96 4>;
+	};
+	al5d: al5d@a0029000 {
+		compatible = "allegro,al5d";
+		reg = <0 0xa0029000 0 0x1000>,
+		      <0 0xa0020000 0 0x8000>;
+		reg-names = "regs", "sram";
+		interrupt-names = "vcu_host_interrupt";
+		interrupts = <0 96 4>;
+	};

Privacy Policy