Re: [PATCH v13 2/2] arm64: dts: imx: Add jpeg encoder/decoder nodes

[Dong Aisheng]

[...]

> > > +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";
> > > +   };
> > > +
> > > +   jpegdec: jpegdec@58400000 {
> >
> > Node should be disabled by default.
> > And enable it in board dts including LPCG.
>
> At version v5 of this patch, the node was disabled by default, and I
> received this feedback from Ezequiel Garcia:
>
> "Pure memory-to-memory are typically not enabled per-board, but just
> per-platform.
> So you can drop the disabled status here."
>
> So, in v6 I made it enabled by default.
>
> Any strong reasons for enabled/disabled per platform?

AFAIK we usually only enable system basic features and let other
user selectable features disabled by default in dts.
Even for device LPCG clocks, if it's enabled by default and later
enter runtime suspend if no users, it still consumes power.

Regards
Aisheng

>
> Thanks,
> Mirela
>
> >
> > > +           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 IMX_LPCG_CLK_0>,
> > > +                    <&img_jpeg_dec_lpcg IMX_LPCG_CLK_4>;
> > > +           clock-names = "per", "ipg";
> > > +           assigned-clocks = <&img_jpeg_dec_lpcg IMX_LPCG_CLK_0>,
> > > +                             <&img_jpeg_dec_lpcg IMX_LPCG_CLK_4>;
> > > +           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 {
> >
> > Ditto
> >
> > > +           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 IMX_LPCG_CLK_0>,
> > > +                    <&img_jpeg_enc_lpcg IMX_LPCG_CLK_4>;
> > > +           clock-names = "per", "ipg";
> > > +           assigned-clocks = <&img_jpeg_enc_lpcg IMX_LPCG_CLK_0>,
> > > +                             <&img_jpeg_enc_lpcg IMX_LPCG_CLK_4>;
> > > +           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>;
> > > +   };
> > > +
> > > +   img_jpeg_dec_lpcg: clock-controller@585d0000 {
> >
> > Ditto
> >
> > > +           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";
> >
> > Ditto
> >
> > Otherwise, I'm fine with this patch.
> >
> > > +           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>;
> > > +   };
> > > +};
> > > diff --git a/arch/arm64/boot/dts/freescale/imx8qm-ss-img.dtsi
> > > b/arch/arm64/boot/dts/freescale/imx8qm-ss-img.dtsi
> > > new file mode 100644
> > > index 000000000000..7764b4146e0a
> > > --- /dev/null
> > > +++ b/arch/arm64/boot/dts/freescale/imx8qm-ss-img.dtsi
> > > @@ -0,0 +1,12 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Copyright 2021 NXP
> > > + */
> > > +
> > > +&jpegdec {
> > > +   compatible = "nxp,imx8qm-jpgdec", "nxp,imx8qxp-jpgdec"; };
> > > +
> > > +&jpegenc {
> > > +   compatible = "nxp,imx8qm-jpgdec", "nxp,imx8qxp-jpgenc"; };
> > > diff --git a/arch/arm64/boot/dts/freescale/imx8qm.dtsi
> > > b/arch/arm64/boot/dts/freescale/imx8qm.dtsi
> > > index 12cd059b339b..aebbe2b84aa1 100644
> > > --- a/arch/arm64/boot/dts/freescale/imx8qm.dtsi
> > > +++ b/arch/arm64/boot/dts/freescale/imx8qm.dtsi
> > > @@ -166,11 +166,13 @@
> > >     };
> > >
> > >     /* sorted in register address */
> > > +   #include "imx8-ss-img.dtsi"
> > >     #include "imx8-ss-dma.dtsi"
> > >     #include "imx8-ss-conn.dtsi"
> > >     #include "imx8-ss-lsio.dtsi"
> > >  };
> > >
> > > +#include "imx8qm-ss-img.dtsi"
> > >  #include "imx8qm-ss-dma.dtsi"
> > >  #include "imx8qm-ss-conn.dtsi"
> > >  #include "imx8qm-ss-lsio.dtsi"
> > > diff --git a/arch/arm64/boot/dts/freescale/imx8qxp-ss-img.dtsi
> > > b/arch/arm64/boot/dts/freescale/imx8qxp-ss-img.dtsi
> > > new file mode 100644
> > > index 000000000000..3a087317591d
> > > --- /dev/null
> > > +++ b/arch/arm64/boot/dts/freescale/imx8qxp-ss-img.dtsi
> > > @@ -0,0 +1,13 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Copyright 2021 NXP
> > > + * Dong Aisheng <aisheng.dong@xxxxxxx>
> > > + */
> > > +
> > > +&jpegdec {
> > > +   compatible = "nxp,imx8qxp-jpgdec";
> > > +};
> > > +
> > > +&jpegenc {
> > > +   compatible = "nxp,imx8qxp-jpgenc";
> > > +};
> > > diff --git a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> > > b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> > > index 1e6b4995091e..a625fb6bdc62 100644
> > > --- a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> > > +++ b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> > > @@ -258,12 +258,14 @@
> > >     };
> > >
> > >     /* sorted in register address */
> > > +   #include "imx8-ss-img.dtsi"
> > >     #include "imx8-ss-adma.dtsi"
> > >     #include "imx8-ss-conn.dtsi"
> > >     #include "imx8-ss-ddr.dtsi"
> > >     #include "imx8-ss-lsio.dtsi"
> > >  };
> > >
> > > +#include "imx8qxp-ss-img.dtsi"
> > >  #include "imx8qxp-ss-adma.dtsi"
> > >  #include "imx8qxp-ss-conn.dtsi"
> > >  #include "imx8qxp-ss-lsio.dtsi"
> > > --
> > > 2.17.1
> >
> >
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel