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

[Adam Ford <aford173@gmail.com>]

On Wed, Nov 29, 2023 at 2:14 PM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> 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@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 ?
>
> 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.

Any objections if I slow them both to 266Mhz and add a note that
indicates what they're each capable of doing based on the datasheet?
This way, regardless of whether or not people have one or two cameras
it won't violate a spec.  Adding the note at least tells people they
have options to increase the clock speeds if they choose.

adam

>
> --
> Kieran
>
>
> >
> > > > +                               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