Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1512378 > unrolled thread

[PATCH v2 1/3] dt-bindings: mediatek: Add a binding for Mediatek JPEG Decoder

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 1/3] dt-bindings: mediatek: Add a binding for Mediatek JPEG Decoder Rick Chang <rick.chang@mediatek.com> - 2016-10-31 08:20 +0100
    Re: [PATCH v2 1/3] dt-bindings: mediatek: Add a binding for Mediatek JPEG Decoder Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2016-11-03 19:40 +0100
      Re: [PATCH v2 1/3] dt-bindings: mediatek: Add a binding for Mediatek JPEG Decoder Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2016-11-03 19:40 +0100
        Re: [PATCH v2 1/3] dt-bindings: mediatek: Add a binding for  Mediatek JPEG Decoder Rick Chang <rick.chang@mediatek.com> - 2016-11-04 06:00 +0100
          Re: [PATCH v2 1/3] dt-bindings: mediatek: Add a binding for Mediatek JPEG Decoder Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2016-11-04 17:00 +0100
      Re: [PATCH v2 1/3] dt-bindings: mediatek: Add a binding for  Mediatek JPEG Decoder Rick Chang <rick.chang@mediatek.com> - 2016-11-04 05:30 +0100

#1512378 — [PATCH v2 1/3] dt-bindings: mediatek: Add a binding for Mediatek JPEG Decoder

Add a DT binding documentation for Mediatek JPEG Decoder of
MT2701 SoC.

Signed-off-by: Rick Chang <rick.chang@mediatek.com>
Signed-off-by: Minghsiu Tsai <minghsiu.tsai@mediatek.com>
---
 .../bindings/media/mediatek-jpeg-codec.txt         | 35 ++++++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/mediatek-jpeg-codec.txt

diff --git a/Documentation/devicetree/bindings/media/mediatek-jpeg-codec.txt b/Documentation/devicetree/bindings/media/mediatek-jpeg-codec.txt
new file mode 100644
index 0000000..514e656
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/mediatek-jpeg-codec.txt
@@ -0,0 +1,35 @@
+* Mediatek JPEG Codec
+
+Mediatek JPEG Codec device driver is a v4l2 driver which can decode
+JPEG-encoded video frames.
+
+Required properties:
+  - compatible : "mediatek,mt2701-jpgdec"
+  - reg : Physical base address of the jpeg codec registers and length of
+        memory mapped region.
+  - interrupts : interrupt number to the cpu.
+  - clocks : clock name from clock manager
+  - clock-names: the clocks of the jpeg codec H/W
+  - power-domains : a phandle to the power domain.
+  - larb : must contain the larbes of current platform
+  - iommus : Mediatek IOMMU H/W has designed the fixed associations with
+        the multimedia H/W. and there is only one multimedia iommu domain.
+        "iommus = <&iommu portid>" the "portid" is from
+        dt-bindings\iommu\mt2701-iommu-port.h, it means that this portid will
+        enable iommu. The portid default is disable iommu if "<&iommu portid>"
+        don't be added.
+
+Example:
+	jpegdec: jpegdec@15004000 {
+		compatible = "mediatek,mt2701-jpgdec";
+		reg = <0 0x15004000 0 0x1000>;
+		interrupts = <GIC_SPI 143 IRQ_TYPE_LEVEL_LOW>;
+		clocks =  <&imgsys CLK_IMG_JPGDEC_SMI>,
+			  <&imgsys CLK_IMG_JPGDEC>;
+		clock-names = "jpgdec-smi",
+			      "jpgdec";
+		power-domains = <&scpsys MT2701_POWER_DOMAIN_ISP>;
+		mediatek,larb = <&larb2>;
+		iommus = <&iommu MT2701_M4U_PORT_JPGDEC_WDMA>,
+			 <&iommu MT2701_M4U_PORT_JPGDEC_BSDMA>;
+	};
-- 
1.9.1

[toc] | [next] | [standalone]

#1514777

Hi Rick,

Thank you for the patch.

On Monday 31 Oct 2016 15:16:55 Rick Chang wrote:
> Add a DT binding documentation for Mediatek JPEG Decoder of
> MT2701 SoC.
> 
> Signed-off-by: Rick Chang <rick.chang@mediatek.com>
> Signed-off-by: Minghsiu Tsai <minghsiu.tsai@mediatek.com>
> ---
>  .../bindings/media/mediatek-jpeg-codec.txt         | 35 +++++++++++++++++++
>  1 file changed, 35 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/media/mediatek-jpeg-codec.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/mediatek-jpeg-codec.txt
> b/Documentation/devicetree/bindings/media/mediatek-jpeg-codec.txt new file
> mode 100644
> index 0000000..514e656
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/mediatek-jpeg-codec.txt
> @@ -0,0 +1,35 @@
> +* Mediatek JPEG Codec

Is it a codec or a decoder only ?

> +Mediatek JPEG Codec device driver is a v4l2 driver which can decode
> +JPEG-encoded video frames.

DT bindings should not reference drivers, they are OS-agnostic.

> +Required properties:
> +  - compatible : "mediatek,mt2701-jpgdec"
> +  - reg : Physical base address of the jpeg codec registers and length of
> +        memory mapped region.
> +  - interrupts : interrupt number to the cpu.

That's actually not correct, the interrupt number is local to the interrupt 
controller, not to the CPU.

> +  - clocks : clock name from clock manager

The clocks property doesn't contain a name.

Until we provide standardized descriptions for those properties, I recommend 
copying the compatible, reg, interrupts, clocks, clock-names, power-domains 
and iommus properties descriptions from good DT bindings. Which DT bindings 
are good source of inspiration here is left as an exercise for the reader I'm 
afraid :-(

> +  - clock-names: the clocks of the jpeg codec H/W
> +  - power-domains : a phandle to the power domain.
> +  - larb : must contain the larbes of current platform

Shouldn't this be mediatek,larb ? And what is a larb ?

> +  - iommus : Mediatek IOMMU H/W has designed the fixed associations with
> +        the multimedia H/W. and there is only one multimedia iommu domain.
> +        "iommus = <&iommu portid>" the "portid" is from
> +        dt-bindings\iommu\mt2701-iommu-port.h, it means that this portid
> will
> +        enable iommu. The portid default is disable iommu if "<&iommu> 
portid>"
> +        don't be added.

There are two iommus instances in your example below, this should be 
documented. This description is not very clear I'm afraid.

> +
> +Example:
> +	jpegdec: jpegdec@15004000 {
> +		compatible = "mediatek,mt2701-jpgdec";
> +		reg = <0 0x15004000 0 0x1000>;
> +		interrupts = <GIC_SPI 143 IRQ_TYPE_LEVEL_LOW>;
> +		clocks =  <&imgsys CLK_IMG_JPGDEC_SMI>,
> +			  <&imgsys CLK_IMG_JPGDEC>;
> +		clock-names = "jpgdec-smi",
> +			      "jpgdec";
> +		power-domains = <&scpsys MT2701_POWER_DOMAIN_ISP>;
> +		mediatek,larb = <&larb2>;
> +		iommus = <&iommu MT2701_M4U_PORT_JPGDEC_WDMA>,
> +			 <&iommu MT2701_M4U_PORT_JPGDEC_BSDMA>;
> +	};

-- 
Regards,

Laurent Pinchart

[toc] | [prev] | [next] | [standalone]

#1514781

Hi Rick,

A few more comments.

On Thursday 03 Nov 2016 20:33:12 Laurent Pinchart wrote:
> On Monday 31 Oct 2016 15:16:55 Rick Chang wrote:
> > Add a DT binding documentation for Mediatek JPEG Decoder of
> > MT2701 SoC.
> > 
> > Signed-off-by: Rick Chang <rick.chang@mediatek.com>
> > Signed-off-by: Minghsiu Tsai <minghsiu.tsai@mediatek.com>
> > ---
> > 
> >  .../bindings/media/mediatek-jpeg-codec.txt         | 35 ++++++++++++++++
> >  1 file changed, 35 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/media/mediatek-jpeg-codec.txt
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/media/mediatek-jpeg-codec.txt
> > b/Documentation/devicetree/bindings/media/mediatek-jpeg-codec.txt new
> > file mode 100644
> > index 0000000..514e656
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/mediatek-jpeg-codec.txt
> > @@ -0,0 +1,35 @@
> > +* Mediatek JPEG Codec
> 
> Is it a codec or a decoder only ?
> 
> > +Mediatek JPEG Codec device driver is a v4l2 driver which can decode
> > +JPEG-encoded video frames.
> 
> DT bindings should not reference drivers, they are OS-agnostic.
> 
> > +Required properties:
> > +  - compatible : "mediatek,mt2701-jpgdec"

Is the JPEG decoder found in MT2701 only, or in other Mediatek SoCs as well ?

> > +  - reg : Physical base address of the jpeg codec registers and length of
> > +        memory mapped region.
> > +  - interrupts : interrupt number to the cpu.
> 
> That's actually not correct, the interrupt number is local to the interrupt
> controller, not to the CPU.
> 
> > +  - clocks : clock name from clock manager
> 
> The clocks property doesn't contain a name.

Furthermore you should document which clocks need to be specified here. There 
are two of them in the example below, the documentation should explain this 
clearly.

> Until we provide standardized descriptions for those properties, I recommend
> copying the compatible, reg, interrupts, clocks, clock-names, power-domains
> and iommus properties descriptions from good DT bindings. Which DT bindings
> are good source of inspiration here is left as an exercise for the reader
> I'm afraid :-(
> 
> > +  - clock-names: the clocks of the jpeg codec H/W
> > +  - power-domains : a phandle to the power domain.
> > +  - larb : must contain the larbes of current platform
> 
> Shouldn't this be mediatek,larb ? And what is a larb ?
> 
> > +  - iommus : Mediatek IOMMU H/W has designed the fixed associations with
> > +        the multimedia H/W. and there is only one multimedia iommu
> > domain.
> > +        "iommus = <&iommu portid>" the "portid" is from
> > +        dt-bindings\iommu\mt2701-iommu-port.h, it means that this portid
> > will
> > +        enable iommu. The portid default is disable iommu if "<&iommu>
> 
> portid>"
> 
> > +        don't be added.
> 
> There are two iommus instances in your example below, this should be
> documented. This description is not very clear I'm afraid.
> 
> > +
> > +Example:
> > +	jpegdec: jpegdec@15004000 {
> > +		compatible = "mediatek,mt2701-jpgdec";
> > +		reg = <0 0x15004000 0 0x1000>;
> > +		interrupts = <GIC_SPI 143 IRQ_TYPE_LEVEL_LOW>;
> > +		clocks =  <&imgsys CLK_IMG_JPGDEC_SMI>,
> > +			  <&imgsys CLK_IMG_JPGDEC>;
> > +		clock-names = "jpgdec-smi",
> > +			      "jpgdec";
> > +		power-domains = <&scpsys MT2701_POWER_DOMAIN_ISP>;
> > +		mediatek,larb = <&larb2>;
> > +		iommus = <&iommu MT2701_M4U_PORT_JPGDEC_WDMA>,
> > +			 <&iommu MT2701_M4U_PORT_JPGDEC_BSDMA>;
> > +	};

-- 
Regards,

Laurent Pinchart

[toc] | [prev] | [next] | [standalone]

#1514995 — Re: [PATCH v2 1/3] dt-bindings: mediatek: Add a binding for Mediatek JPEG Decoder

Hi Laurent,

Thank you for the comments.I will fix them in the next patch (v3).

On Thu, 2016-11-03 at 20:34 +0200, Laurent Pinchart wrote:
> Hi Rick,
> 
> A few more comments.
> 
> On Thursday 03 Nov 2016 20:33:12 Laurent Pinchart wrote:
> > On Monday 31 Oct 2016 15:16:55 Rick Chang wrote:
> > > Add a DT binding documentation for Mediatek JPEG Decoder of
> > > MT2701 SoC.
> > > 
> > > Signed-off-by: Rick Chang <rick.chang@mediatek.com>
> > > Signed-off-by: Minghsiu Tsai <minghsiu.tsai@mediatek.com>
> > > ---
> > > 
> > >  .../bindings/media/mediatek-jpeg-codec.txt         | 35 ++++++++++++++++
> > >  1 file changed, 35 insertions(+)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/media/mediatek-jpeg-codec.txt
> > > 
> > > diff --git
> > > a/Documentation/devicetree/bindings/media/mediatek-jpeg-codec.txt
> > > b/Documentation/devicetree/bindings/media/mediatek-jpeg-codec.txt new
> > > file mode 100644
> > > index 0000000..514e656
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/mediatek-jpeg-codec.txt
> > > @@ -0,0 +1,35 @@
> > > +* Mediatek JPEG Codec
> > 
> > Is it a codec or a decoder only ?
> > 
> > > +Mediatek JPEG Codec device driver is a v4l2 driver which can decode
> > > +JPEG-encoded video frames.
> > 
> > DT bindings should not reference drivers, they are OS-agnostic.
> > 
> > > +Required properties:
> > > +  - compatible : "mediatek,mt2701-jpgdec"
> 
> Is the JPEG decoder found in MT2701 only, or in other Mediatek SoCs as well ?

Yes, the JPEG decoder is found in other Mediatek SoCs. However, the JPEG
decoder HW in different SoCs have different register base, interrupt,
power-domain and iommu setting. This patch series is only applicable in
MT2701.

> > > +  - reg : Physical base address of the jpeg codec registers and length of
> > > +        memory mapped region.
> > > +  - interrupts : interrupt number to the cpu.
> > 
> > That's actually not correct, the interrupt number is local to the interrupt
> > controller, not to the CPU.
> > 
> > > +  - clocks : clock name from clock manager
> > 
> > The clocks property doesn't contain a name.
> 
> Furthermore you should document which clocks need to be specified here. There 
> are two of them in the example below, the documentation should explain this 
> clearly.

OK.

> > Until we provide standardized descriptions for those properties, I recommend
> > copying the compatible, reg, interrupts, clocks, clock-names, power-domains
> > and iommus properties descriptions from good DT bindings. Which DT bindings
> > are good source of inspiration here is left as an exercise for the reader
> > I'm afraid :-(
> > 
> > > +  - clock-names: the clocks of the jpeg codec H/W
> > > +  - power-domains : a phandle to the power domain.
> > > +  - larb : must contain the larbes of current platform
> > 
> > Shouldn't this be mediatek,larb ? And what is a larb ?
> > 
> > > +  - iommus : Mediatek IOMMU H/W has designed the fixed associations with
> > > +        the multimedia H/W. and there is only one multimedia iommu
> > > domain.
> > > +        "iommus = <&iommu portid>" the "portid" is from
> > > +        dt-bindings\iommu\mt2701-iommu-port.h, it means that this portid
> > > will
> > > +        enable iommu. The portid default is disable iommu if "<&iommu>
> > 
> > portid>"
> > 
> > > +        don't be added.
> > 
> > There are two iommus instances in your example below, this should be
> > documented. This description is not very clear I'm afraid.
> > 
> > > +
> > > +Example:
> > > +	jpegdec: jpegdec@15004000 {
> > > +		compatible = "mediatek,mt2701-jpgdec";
> > > +		reg = <0 0x15004000 0 0x1000>;
> > > +		interrupts = <GIC_SPI 143 IRQ_TYPE_LEVEL_LOW>;
> > > +		clocks =  <&imgsys CLK_IMG_JPGDEC_SMI>,
> > > +			  <&imgsys CLK_IMG_JPGDEC>;
> > > +		clock-names = "jpgdec-smi",
> > > +			      "jpgdec";
> > > +		power-domains = <&scpsys MT2701_POWER_DOMAIN_ISP>;
> > > +		mediatek,larb = <&larb2>;
> > > +		iommus = <&iommu MT2701_M4U_PORT_JPGDEC_WDMA>,
> > > +			 <&iommu MT2701_M4U_PORT_JPGDEC_BSDMA>;
> > > +	};
> 

[toc] | [prev] | [next] | [standalone]

#1515275

Hi Rick,

On Friday 04 Nov 2016 12:51:00 Rick Chang wrote:
> On Thu, 2016-11-03 at 20:34 +0200, Laurent Pinchart wrote:
> > On Thursday 03 Nov 2016 20:33:12 Laurent Pinchart wrote:
> >> On Monday 31 Oct 2016 15:16:55 Rick Chang wrote:
> >>> Add a DT binding documentation for Mediatek JPEG Decoder of
> >>> MT2701 SoC.
> >>> 
> >>> Signed-off-by: Rick Chang <rick.chang@mediatek.com>
> >>> Signed-off-by: Minghsiu Tsai <minghsiu.tsai@mediatek.com>
> >>> ---
> >>> 
> >>>  .../bindings/media/mediatek-jpeg-codec.txt         | 35 ++++++++++++++
> >>>  1 file changed, 35 insertions(+)
> >>>  create mode 100644
> >>> Documentation/devicetree/bindings/media/mediatek-jpeg-codec.txt
> >>> 
> >>> diff --git
> >>> a/Documentation/devicetree/bindings/media/mediatek-jpeg-codec.txt
> >>> b/Documentation/devicetree/bindings/media/mediatek-jpeg-codec.txt new
> >>> file mode 100644
> >>> index 0000000..514e656
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/media/mediatek-jpeg-codec.txt
> >>> @@ -0,0 +1,35 @@
> >>> +* Mediatek JPEG Codec
> >> 
> >> Is it a codec or a decoder only ?
> >> 
> >>> +Mediatek JPEG Codec device driver is a v4l2 driver which can decode
> >>> +JPEG-encoded video frames.
> >> 
> >> DT bindings should not reference drivers, they are OS-agnostic.
> >> 
> >>> +Required properties:
> >>> +  - compatible : "mediatek,mt2701-jpgdec"
> > 
> > Is the JPEG decoder found in MT2701 only, or in other Mediatek SoCs as
> > well ?
>
> Yes, the JPEG decoder is found in other Mediatek SoCs. However, the JPEG
> decoder HW in different SoCs have different register base, interrupt,
> power-domain and iommu setting.

That's fine, and that's exactly what the device tree is used for. When an 
identical IP core is integrated differently in different SoCs, the driver 
retrieves the resources (base address, clocks, IOMMU, interrupt, power domain 
and more) from the device tree without any need for SoC-specific code.

> This patch series is only applicable in MT2701.

That was precisely my question, apart from integration properties, is there 
anything specific to the MT2701 in patches 1/3 and 2/3 ?

-- 
Regards,

Laurent Pinchart

[toc] | [prev] | [next] | [standalone]

#1514992 — Re: [PATCH v2 1/3] dt-bindings: mediatek: Add a binding for Mediatek JPEG Decoder

Hi Laurent,

Thanks for your patient review.I will fix them in the next patch (v3).

On Thu, 2016-11-03 at 20:33 +0200, Laurent Pinchart wrote:
> Hi Rick,
> 
> Thank you for the patch.
> 
> On Monday 31 Oct 2016 15:16:55 Rick Chang wrote:
> > Add a DT binding documentation for Mediatek JPEG Decoder of
> > MT2701 SoC.
> > 
> > Signed-off-by: Rick Chang <rick.chang@mediatek.com>
> > Signed-off-by: Minghsiu Tsai <minghsiu.tsai@mediatek.com>
> > ---
> >  .../bindings/media/mediatek-jpeg-codec.txt         | 35 +++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/media/mediatek-jpeg-codec.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/media/mediatek-jpeg-codec.txt
> > b/Documentation/devicetree/bindings/media/mediatek-jpeg-codec.txt new file
> > mode 100644
> > index 0000000..514e656
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/mediatek-jpeg-codec.txt
> > @@ -0,0 +1,35 @@
> > +* Mediatek JPEG Codec
> 
> Is it a codec or a decoder only ?

It's a decoder precisely.

> > +Mediatek JPEG Codec device driver is a v4l2 driver which can decode
> > +JPEG-encoded video frames.
> 
> DT bindings should not reference drivers, they are OS-agnostic.

OK.

> > +Required properties:
> > +  - compatible : "mediatek,mt2701-jpgdec"
> > +  - reg : Physical base address of the jpeg codec registers and length of
> > +        memory mapped region.
> > +  - interrupts : interrupt number to the cpu.
> 
> That's actually not correct, the interrupt number is local to the interrupt 
> controller, not to the CPU.

OK.

> > +  - clocks : clock name from clock manager
> 
> The clocks property doesn't contain a name.

OK.

> Until we provide standardized descriptions for those properties, I recommend 
> copying the compatible, reg, interrupts, clocks, clock-names, power-domains 
> and iommus properties descriptions from good DT bindings. Which DT bindings 
> are good source of inspiration here is left as an exercise for the reader I'm 
> afraid :-(

Thank you for the advice. I will revise the descriptions with the
statements in existed upstream documents.

> > +  - clock-names: the clocks of the jpeg codec H/W
> > +  - power-domains : a phandle to the power domain.
> > +  - larb : must contain the larbes of current platform
> 
> Shouldn't this be mediatek,larb ? And what is a larb ?

It should be mediatek,larb.

> > +  - iommus : Mediatek IOMMU H/W has designed the fixed associations with
> > +        the multimedia H/W. and there is only one multimedia iommu domain.
> > +        "iommus = <&iommu portid>" the "portid" is from
> > +        dt-bindings\iommu\mt2701-iommu-port.h, it means that this portid
> > will
> > +        enable iommu. The portid default is disable iommu if "<&iommu> 
> portid>"
> > +        don't be added.
> 
> There are two iommus instances in your example below, this should be 
> documented. This description is not very clear I'm afraid.

OK.

> > +
> > +Example:
> > +	jpegdec: jpegdec@15004000 {
> > +		compatible = "mediatek,mt2701-jpgdec";
> > +		reg = <0 0x15004000 0 0x1000>;
> > +		interrupts = <GIC_SPI 143 IRQ_TYPE_LEVEL_LOW>;
> > +		clocks =  <&imgsys CLK_IMG_JPGDEC_SMI>,
> > +			  <&imgsys CLK_IMG_JPGDEC>;
> > +		clock-names = "jpgdec-smi",
> > +			      "jpgdec";
> > +		power-domains = <&scpsys MT2701_POWER_DOMAIN_ISP>;
> > +		mediatek,larb = <&larb2>;
> > +		iommus = <&iommu MT2701_M4U_PORT_JPGDEC_WDMA>,
> > +			 <&iommu MT2701_M4U_PORT_JPGDEC_BSDMA>;
> > +	};
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web