Re: [PATCH v1 1/2] arm64: dts: imx8mp: Add CSIS DT nodes

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Adam,

(CC'ing Kieran)

On Tue, Nov 28, 2023 at 09:17:51PM -0600, Adam Ford wrote:
> On Mon, Apr 17, 2023 at 1:01 AM Laurent Pinchart wrote:
> >
> > Add DT nodes for the two CSI-2 receivers of the i.MX8MP.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> 
> Laurent,
> 
> Sorry to dig up an old thread, but I have a concern about the clock
> ratings and nominal mode vs overdrive mode.  I started investigating
> the different data sheets amongst the various imx8m[mnp] families to
> make the default device trees run at nominal mode while also creating
> a separate dtsi file with settings for overdrive so boards who use it
> can include them without having to duplicate the clock settings for
> everyone who supports overdrive.
> 
> >  arch/arm64/boot/dts/freescale/imx8mp.dtsi | 60 +++++++++++++++++++++++
> >  1 file changed, 60 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > index 2dd60e3252f3..2a374a4c14a2 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > @@ -1239,6 +1239,66 @@ ldb_lvds_ch1: endpoint {
> >                                 };
> >                         };
> >
> > +                       mipi_csi_0: csi@32e40000 {
> > +                               compatible = "fsl,imx8mp-mipi-csi2", "fsl,imx8mm-mipi-csi2";
> > +                               reg = <0x32e40000 0x10000>;
> > +                               interrupts = <GIC_SPI 17 IRQ_TYPE_LEVEL_HIGH>;
> > +                               clock-frequency = <500000000>;
> > +                               clocks = <&clk IMX8MP_CLK_MEDIA_APB_ROOT>,
> > +                                        <&clk IMX8MP_CLK_MEDIA_CAM1_PIX_ROOT>,
> > +                                        <&clk IMX8MP_CLK_MEDIA_MIPI_PHY1_REF_ROOT>,
> > +                                        <&clk IMX8MP_CLK_MEDIA_AXI_ROOT>;
> > +                               clock-names = "pclk", "wrap", "phy", "axi";
> > +                               assigned-clocks = <&clk IMX8MP_CLK_MEDIA_CAM1_PIX>;
> > +                               assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>;
> > +                               assigned-clock-rates = <500000000>;
> 
> According to Rev 2.1 of the Data sheet (IMX8MPCEC), dated July 2023,
> 500MHz is listed as single-camera, overdrive mode.  Single-camera,
> nominal mode is 400MHz, but there is more...
> If configured for dual cameras, both CSI can only support up to
> 266MHz, but we have partially configured both albeit without the
> actual camera sensors connected.
> 
> > +                               power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_MIPI_CSI2_1>;
> > +                               status = "disabled";
> > +
> > +                               ports {
> > +                                       #address-cells = <1>;
> > +                                       #size-cells = <0>;
> > +
> > +                                       port@0 {
> > +                                               reg = <0>;
> > +                                       };
> > +
> > +                                       port@1 {
> > +                                               reg = <1>;
> > +                                       };
> > +                               };
> > +                       };
> > +
> > +                       mipi_csi_1: csi@32e50000 {
> > +                               compatible = "fsl,imx8mp-mipi-csi2", "fsl,imx8mm-mipi-csi2";
> > +                               reg = <0x32e50000 0x10000>;
> > +                               interrupts = <GIC_SPI 80 IRQ_TYPE_LEVEL_HIGH>;
> > +                               clock-frequency = <266000000>;
> > +                               clocks = <&clk IMX8MP_CLK_MEDIA_APB_ROOT>,
> > +                                        <&clk IMX8MP_CLK_MEDIA_CAM2_PIX_ROOT>,
> > +                                        <&clk IMX8MP_CLK_MEDIA_MIPI_PHY1_REF_ROOT>,
> > +                                        <&clk IMX8MP_CLK_MEDIA_AXI_ROOT>;
> > +                               clock-names = "pclk", "wrap", "phy", "axi";
> > +                               assigned-clocks = <&clk IMX8MP_CLK_MEDIA_CAM2_PIX>;
> > +                               assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>;
> > +                               assigned-clock-rates = <266000000>;
> 
> 266MHz is correct for dual camera, but in single camera, the second
> CSI is capable of 277MHz.
> 
> At a minimum, I'd like to fix the overdrive frequency to nominal, but
> since we're plumbing in both cameras, I wonder if it would be better
> to run both at 266MHz with a note on CSI0 that states it could run at
> 400 or 500 if the second CSI is disabled and a note on the second CSI
> that it could run at 277 when the first one is disabled? What are your
> thoughts?

My thoughts is that this all should be selected at runtime, based on how
many cameras are used. That won't be trivial to do though :-S Kieran,
you've been working with two cameras, any opinion ?

> > +                               power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_MIPI_CSI2_2>;
> > +                               status = "disabled";
> > +
> > +                               ports {
> > +                                       #address-cells = <1>;
> > +                                       #size-cells = <0>;
> > +
> > +                                       port@0 {
> > +                                               reg = <0>;
> > +                                       };
> > +
> > +                                       port@1 {
> > +                                               reg = <1>;
> > +                                       };
> > +                               };
> > +                       };
> > +
> >                         pcie_phy: pcie-phy@32f00000 {
> >                                 compatible = "fsl,imx8mp-pcie-phy";
> >                                 reg = <0x32f00000 0x10000>;

-- 
Regards,

Laurent Pinchart