Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230423212656.1381002-1-aford173@gmail.com> <20230424004706.GL21943@pendragon.ideasonboard.com>
 <20230424004902.GN21943@pendragon.ideasonboard.com>
In-Reply-To: <20230424004902.GN21943@pendragon.ideasonboard.com>
From:   Adam Ford <aford173@gmail.com>
Date:   Sun, 23 Apr 2023 21:29:04 -0500
Message-ID: <CAHCN7xLuJm6_WXFS3eXCf9wF1oyFJFPpEiBMV6UMCAqMRzAqbg@mail.gmail.com>
Subject: Re: [PATCH 1/2] arm64: dts: imx8mn: Enable CSI and ISI Nodes
To:     Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc:     linux-arm-kernel@lists.infradead.org,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Shawn Guo <shawnguo@kernel.org>,
        Sascha Hauer <s.hauer@pengutronix.de>,
        Pengutronix Kernel Team <kernel@pengutronix.de>,
        Fabio Estevam <festevam@gmail.com>,
        NXP Linux Team <linux-imx@nxp.com>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sun, Apr 23, 2023 at 7:48=E2=80=AFPM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Mon, Apr 24, 2023 at 03:47:13AM +0300, Laurent Pinchart wrote:
> > Hi Adam,
>
> Another comment, do you plan to submit a patch with a camera DT overlay
> for an i.MX8MN board ?

My test repo has the ISI driver working with my OV5640 camera, but for
some reason the mainline doesn't work.  I'm trying to figure out
what's different between my test repo and the mainline.  I am not
planning on an overlay, as the Beacon baseboard was only designed with
one camera module, a TD Next 5640, based on the OV5640.  Newer boards
have been designed with a more generic camera interface, so I plan to
use overlays in the future, but for the imx8mn-beacon-kit, I was
planning to follow a similar design to what was done for the
imx8mm-beacon-kit.  They share the same baseboard with a different
processor on the system-on-module.

Do you want me to add a patch to the imx8mn-beacon-kit so there is at
least one user of this driver, once I get my error resolved?

adam
>
> > Thank you for the patch.
> >
> > On Sun, Apr 23, 2023 at 04:26:55PM -0500, Adam Ford wrote:
> > > The CSI in the imx8mn is the same as what is used in the imx8mm,
> > > but it's routed to the ISI on the Nano. Add both the ISI and CSI
> > > nodes, and pointing them to each other. Since the CSI capture is
> > > dependent on an attached camera, mark both ISI and CSI as
> > > disabled by default.
> >
> > I'd then write the subject line as "Add CSI and ISI nodes".
> >
> > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > >
> > > diff --git a/arch/arm64/boot/dts/freescale/imx8mn.dtsi b/arch/arm64/b=
oot/dts/freescale/imx8mn.dtsi
> > > index 8be8f090e8b8..102550b41f22 100644
> > > --- a/arch/arm64/boot/dts/freescale/imx8mn.dtsi
> > > +++ b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
> > > @@ -1104,6 +1104,24 @@ dsim_from_lcdif: endpoint {
> > >                             };
> > >                     };
> > >
> > > +                   isi: isi@32e20000 {
> > > +                           compatible =3D "fsl,imx8mn-isi";
> > > +                           reg =3D <0x32e20000 0x100>;
> >
> > The i.MX8MN reference manual documents the ISI registers block size to
> > be 64kB. Should we use the same here, even if all the registers we need
> > are within the first 256 bytes ?
> >
> > > +                           interrupts =3D <GIC_SPI 16 IRQ_TYPE_LEVEL=
_HIGH>;
> > > +                           clocks =3D <&clk IMX8MN_CLK_DISP_AXI_ROOT=
>,
> > > +                                    <&clk IMX8MN_CLK_DISP_APB_ROOT>;
> > > +                           clock-names =3D "axi", "apb";
> > > +                           fsl,blk-ctrl =3D <&disp_blk_ctrl>;
> > > +                           power-domains =3D <&disp_blk_ctrl IMX8MN_=
DISPBLK_PD_ISI>;
> > > +                           status =3D "disabled";
> > > +
> > > +                           port {
> > > +                                   isi_in: endpoint {
> > > +                                           remote-endpoint =3D <&mip=
i_csi_out>;
> > > +                                   };
> > > +                           };
> >
> > This will fail to validate against the ISI DT binding, as they require =
a
> > "ports" node. When a single port is present using a "port" node directl=
y
> > is fine from an OF graph point of view, but to avoid too much complexit=
y
> > in the ISI binding the consensus was to always require a "ports" node
> > for the ISI.
> >
> > > +                   };
> > > +
> > >                     disp_blk_ctrl: blk-ctrl@32e28000 {
> > >                             compatible =3D "fsl,imx8mn-disp-blk-ctrl"=
, "syscon";
> > >                             reg =3D <0x32e28000 0x100>;
> > > @@ -1147,6 +1165,42 @@ disp_blk_ctrl: blk-ctrl@32e28000 {
> > >                             #power-domain-cells =3D <1>;
> > >                     };
> > >
> > > +                   mipi_csi: mipi-csi@32e30000 {
> > > +                           compatible =3D "fsl,imx8mm-mipi-csi2";
> > > +                           reg =3D <0x32e30000 0x1000>;
> > > +                           interrupts =3D <GIC_SPI 17 IRQ_TYPE_LEVEL=
_HIGH>;
> > > +                           assigned-clocks =3D <&clk IMX8MN_CLK_CAME=
RA_PIXEL>,
> > > +                                             <&clk IMX8MN_CLK_CSI1_P=
HY_REF>;
> > > +                           assigned-clock-parents =3D <&clk IMX8MN_S=
YS_PLL2_1000M>,
> > > +                                                     <&clk IMX8MN_SY=
S_PLL2_1000M>;
> > > +                           assigned-clock-rates =3D <333000000>;
> > > +                           clock-frequency =3D <333000000>;
> > > +                           clocks =3D <&clk IMX8MN_CLK_DISP_APB_ROOT=
>,
> > > +                                    <&clk IMX8MN_CLK_CAMERA_PIXEL>,
> > > +                                    <&clk IMX8MN_CLK_CSI1_PHY_REF>,
> > > +                                    <&clk IMX8MN_CLK_DISP_AXI_ROOT>;
> > > +                           clock-names =3D "pclk", "wrap", "phy", "a=
xi";
> > > +                           power-domains =3D <&disp_blk_ctrl IMX8MN_=
DISPBLK_PD_MIPI_CSI>;
> > > +                           status =3D "disabled";
> > > +
> > > +                           ports {
> > > +                                   #address-cells =3D <1>;
> > > +                                   #size-cells =3D <0>;
> > > +
> > > +                                   port@0 {
> > > +                                           reg =3D <0>;
> > > +                                   };
> > > +
> > > +                                   port@1 {
> > > +                                           reg =3D <1>;
> > > +
> > > +                                           mipi_csi_out: endpoint {
> > > +                                                   remote-endpoint =
=3D <&isi_in>;
> > > +                                           };
> > > +                                   };
> > > +                           };
> > > +                   };
> > > +
> > >                     usbotg1: usb@32e40000 {
> > >                             compatible =3D "fsl,imx8mn-usb", "fsl,imx=
7d-usb", "fsl,imx27-usb";
> > >                             reg =3D <0x32e40000 0x200>;
>
> --
> Regards,
>
> Laurent Pinchart