Re: [EXT] Re: [PATCH v11] arm64: dts: imx8qxp: Add jpeg encoder/decoder nodes

[Mirela Rabulea]

Hi Shawn,

On Thu, 2021-05-13 at 15:38 +0800, Shawn Guo wrote:
> 
> On Fri, Apr 23, 2021 at 01:14:14PM +0300, Mirela Rabulea (OSS) wrote:
> > From: Mirela Rabulea <mirela.rabulea@xxxxxxx>
> > 
> > Add dts for imaging subsytem, include jpeg nodes here.
> > Tested on imx8qxp only, should work on imx8qm, but it was not
> > tested.
> > 
> > Signed-off-by: Mirela Rabulea <mirela.rabulea@xxxxxxx>
> 
> So the bindings and driver parts have been accepted already?

Yes, they are already in linux-next.

> 
> > ---
> > Changes in v11:
> >   Adress feedback from Aisheng Dong:
> >   - Rename img_jpeg_dec_clk/img_jpeg_enc_clk to
> > jpeg_dec_lpcg/jpeg_enc_lpcg to make it visible it's lpcg not other
> > type of clk
> >   - Drop the cameradev node, not needed for jpeg
> >   - Match assigned-clocks & assigned-clock-rates
> > 
> >  .../arm64/boot/dts/freescale/imx8-ss-img.dtsi | 82
> > +++++++++++++++++++
> >  arch/arm64/boot/dts/freescale/imx8qxp.dtsi    |  1 +
> >  2 files changed, 83 insertions(+)
> >  create mode 100644 arch/arm64/boot/dts/freescale/imx8-ss-img.dtsi
> > 
> > diff --git a/arch/arm64/boot/dts/freescale/imx8-ss-img.dtsi
> > b/arch/arm64/boot/dts/freescale/imx8-ss-img.dtsi
> > new file mode 100644
> > index 000000000000..c508e5d0c92b
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/freescale/imx8-ss-img.dtsi
> > @@ -0,0 +1,82 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2019-2021 NXP
> > + * Zhou Guoniu <guoniu.zhou@xxxxxxx>
> > + */
> > +img_subsys: bus@58000000 {
> > +     compatible = "simple-bus";
> > +     #address-cells = <1>;
> > +     #size-cells = <1>;
> > +     ranges = <0x58000000 0x0 0x58000000 0x1000000>;
> > +
> > +     img_ipg_clk: clock-img-ipg {
> > +             compatible = "fixed-clock";
> > +             #clock-cells = <0>;
> > +             clock-frequency = <200000000>;
> > +             clock-output-names = "img_ipg_clk";
> > +     };
> 
> Hmm, not sure a fixed-clock should be in the subsystem.

I checked the reference manuals for 8QXP and 8QM (where this sybsystem
could be included) and this clock is the same, 200 MHz.
Also, as Aisheng mentioned, we will be able to overwrite in QM subsytem
if something is needed.
The same approach is done in the following subsystems, which also
contain fixed-clocks:
    arch/arm64/boot/dts/freescale/imx8-ss-audio.dtsi
    arch/arm64/boot/dts/freescale/imx8-ss-dma.dtsi
    arch/arm64/boot/dts/freescale/imx8-ss-lsio.dtsi 

> 
> > +
> > +     img_jpeg_dec_lpcg: clock-controller@585d0000 {
> > +             compatible = "fsl,imx8qxp-lpcg";
> > +             reg = <0x585d0000 0x10000>;
> > +             #clock-cells = <1>;
> > +             clocks = <&img_ipg_clk>, <&img_ipg_clk>;
> > +             clock-indices = <IMX_LPCG_CLK_0>,
> > +                             <IMX_LPCG_CLK_4>;
> > +             clock-output-names = "img_jpeg_dec_lpcg_clk",
> > +                                  "img_jpeg_dec_lpcg_ipg_clk";
> > +             power-domains = <&pd IMX_SC_R_MJPEG_DEC_MP>;
> > +     };
> > +
> > +     img_jpeg_enc_lpcg: clock-controller@585f0000 {
> > +             compatible = "fsl,imx8qxp-lpcg";
> > +             reg = <0x585f0000 0x10000>;
> > +             #clock-cells = <1>;
> > +             clocks = <&img_ipg_clk>, <&img_ipg_clk>;
> > +             clock-indices = <IMX_LPCG_CLK_0>,
> > +                             <IMX_LPCG_CLK_4>;
> > +             clock-output-names = "img_jpeg_enc_lpcg_clk",
> > +                                  "img_jpeg_enc_lpcg_ipg_clk";
> > +             power-domains = <&pd IMX_SC_R_MJPEG_ENC_MP>;
> > +     };
> > +
> > +     jpegdec: jpegdec@58400000 {
> 
> Keep nodes sorted in unit address.

I fixed this in v12. Thanks for the feedback.

> 
> Shawn
> 
> > +             compatible = "nxp,imx8qxp-jpgdec";
> > +             reg = <0x58400000 0x00050000 >;
> > +             interrupts = <GIC_SPI 309 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI 310 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI 311 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI 312 IRQ_TYPE_LEVEL_HIGH>;
> > +             clocks = <&img_jpeg_dec_lpcg 0>,
> > +                      <&img_jpeg_dec_lpcg 1>;
> > +             clock-names = "per", "ipg";
> > +             assigned-clocks = <&img_jpeg_dec_lpcg 0>,
> > +                               <&img_jpeg_dec_lpcg 1>;
> > +             assigned-clock-rates = <200000000>, <200000000>;
> > +             power-domains = <&pd IMX_SC_R_MJPEG_DEC_MP>,
> > +                             <&pd IMX_SC_R_MJPEG_DEC_S0>,
> > +                             <&pd IMX_SC_R_MJPEG_DEC_S1>,
> > +                             <&pd IMX_SC_R_MJPEG_DEC_S2>,
> > +                             <&pd IMX_SC_R_MJPEG_DEC_S3>;
> > +     };
> > +
> > +     jpegenc: jpegenc@58450000 {
> > +             compatible = "nxp,imx8qxp-jpgenc";
> > +             reg = <0x58450000 0x00050000 >;
> > +             interrupts = <GIC_SPI 305 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI 306 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI 307 IRQ_TYPE_LEVEL_HIGH>,
> > +                          <GIC_SPI 308 IRQ_TYPE_LEVEL_HIGH>;
> > +             clocks = <&img_jpeg_enc_lpcg 0>,
> > +                      <&img_jpeg_enc_lpcg 1>;
> > +             clock-names = "per", "ipg";
> > +             assigned-clocks = <&img_jpeg_enc_lpcg 0>,
> > +                               <&img_jpeg_enc_lpcg 1>;
> > +             assigned-clock-rates = <200000000>, <200000000>;
> > +             power-domains = <&pd IMX_SC_R_MJPEG_ENC_MP>,
> > +                             <&pd IMX_SC_R_MJPEG_ENC_S0>,
> > +                             <&pd IMX_SC_R_MJPEG_ENC_S1>,
> > +                             <&pd IMX_SC_R_MJPEG_ENC_S2>,
> > +                             <&pd IMX_SC_R_MJPEG_ENC_S3>;
> > +     };
> > +};
> > diff --git a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> > b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> > index 1e6b4995091e..2d9589309bd0 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> > @@ -258,6 +258,7 @@
> >       };
> > 
> >       /* sorted in register address */
> > +     #include "imx8-ss-img.dtsi"
> >       #include "imx8-ss-adma.dtsi"
> >       #include "imx8-ss-conn.dtsi"
> >       #include "imx8-ss-ddr.dtsi"
> > --
> > 2.17.1
> >