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

[Kieran Bingham]

Quoting Laurent Pinchart (2023-11-29 10:55:36)
> 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 at 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 at 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 at 0 {
> > > +                                               reg = <0>;
> > > +                                       };
> > > +
> > > +                                       port at 1 {
> > > +                                               reg = <1>;
> > > +                                       };
> > > +                               };
> > > +                       };
> > > +
> > > +                       mipi_csi_1: csi at 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 ?

Well I've been seeing issues like dropped packets and various issues on
the CSI2 receiver that could be because of badly configured clocks ...
so there's definitely some things to check and validate here still.

I don't know how the clocking would be adapted at runtime yet either ...
but it might be something interesting to look at. It's likely more
difficult as it's two separate nodes that would have to know about each
other at some level.

--
Kieran


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