From: Adam Ford <aford173@gmail.com> To: Alexander Stein <alexander.stein@ew.tq-group.com> Cc: Marco Felsch <m.felsch@pengutronix.de>, linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org, kernel@pengutronix.de, Xavier Roumegue <xavier.roumegue@oss.nxp.com>, Rob Herring <robh+dt@kernel.org>, linux-imx@nxp.com, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Jacopo Mondi <jacopo.mondi@ideasonboard.com>, Shawn Guo <shawnguo@kernel.org>, linux-media@vger.kernel.org, Laurent Pinchart <laurent.pinchart@ideasonboard.com> Subject: Re: [PATCH v1 1/2] arm64: dts: imx8mp: Add CSIS DT nodes Date: Mon, 17 Apr 2023 07:45:15 -0500 [thread overview] Message-ID: <CAHCN7xJ26TMD4U_5wTtGcGFHZBTTewVRd+mnKa5Ff5cRxBdHPA@mail.gmail.com> (raw) In-Reply-To: <3232774.44csPzL39Z@steina-w> On Mon, Apr 17, 2023 at 5:11 AM Alexander Stein <alexander.stein@ew.tq-group.com> wrote: > > Am Montag, 17. April 2023, 10:15:10 CEST schrieb Laurent Pinchart: > > Hi Marco, > > > > On Mon, Apr 17, 2023 at 10:01:17AM +0200, Marco Felsch wrote: > > > On 23-04-17, Laurent Pinchart wrote: > > > > On Mon, Apr 17, 2023 at 08:50:59AM +0200, Marco Felsch wrote: > > > > > Hi Laurent, > > > > > > > > > > your patch LGTM just one nit/idea, please see below. > > > > > > > > > > On 23-04-17, 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> > > > > > > --- > > > > > > > > > > > > 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>; > > > > > > + power-domains = > <&media_blk_ctrl > > > > > > IMX8MP_MEDIABLK_PD_MIPI_CSI2_1>; > > > > > > + status = "disabled"; > > > > > > + > > > > > > + ports { > > > > > > + #address-cells = > <1>; > > > > > > + #size-cells = <0>; > > > > > > + > > > > > > + port@0 { > > > > > > + reg = > <0>; > > > > > > > > > > If we would add: > > > > > mipi_csi_0_in: > endpoint {}; > > > > > > > > > > here we could refernce it from overlays/board dts files more easily. > > > > > > > > Isn't there an unwritten rule (or consensus) that an endpoint should > > > > always have a remote-endpoint property ? > > > > > > I don't know if there is one. > > > > > > > While ports describe hardware properties of a device and should always > > > > be there regardless of connections, endpoints describe connections and > > > > I don't think they should be instantiated with a valid > > > > remote-endpoint. > > > > > > I know, therefore I mentioned it as idea to make it 'easier' to add > > > camera nodes. > > > > As a middleground, would it be useful to have a label for the port ? > > Something like > > > > mipi_csi_0: csi@32e40000 { > > ports { > > mipi_csi_0_port_0: port@0 { > > }; > > }; > > }; > > > > An overlay could then reference that and create the endpoint. I'm not > > entirely sure how useful that would be though, as the overlay would need > > to enable the CSI node anyway. Compare > > > > -------- > > &mipi_csi_0 { > > status = "okay"; > > }; > > > > &mipi_csi_0_port_0 { > > mipi_csi_0_in: endpoint { > > remote-endpoint = <&imx327_out>; > > }; > > }; > > -------- > > > > with > > > > -------- > > &mipi_csi_0 { > > status = "okay"; > > > > ports { > > port@0 { > > mipi_csi_0_in: endpoint { > > remote-endpoint = <&imx327_out>; > > }; > > }; > > }; > > }; > > -------- > > > > I have a slight preference for the latter as it groups all the CSI0 data > > in a single overlay target, but if the former is generally preferred, > > I'm fine with that too. > > The former is more compact, but also raises the following dtc warnings while > creating the .dtbo: > Warning (graph_endpoint): /fragment@4/__overlay__: graph endpoint node name > should be 'endpoint' > Warning (graph_endpoint): /fragment@4/__overlay__: graph connection to node '/ > fragment@1/__overlay__/ports/port@1/endpoint' is not bidirectional > > for the following snippet: > > &mipi_csi_0_out { > remote-endpoint = <&isp1_in>; > }; > > I'm not sure if there is a chance to fix at all. Once there is consensus on how this should be generically plumbed, please keep me in the loop, so I can add the corresponding imx8m Nano trees as well. I've tested Laurent's work for a while on the Nano that I have. I was going to push DT updates for Nano, then I saw this conversation, so I decided to hold off for now. adam > > Best regards, > Alexander > > > > > > > > > + }; > > > > > > + > > > > > > + 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>; > > > > > > + 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>; > > > -- > TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany > Amtsgericht München, HRB 105018 > Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider > http://www.tq-group.com/ > >
WARNING: multiple messages have this Message-ID (diff)
From: Adam Ford <aford173@gmail.com> To: Alexander Stein <alexander.stein@ew.tq-group.com> Cc: Marco Felsch <m.felsch@pengutronix.de>, linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org, kernel@pengutronix.de, Xavier Roumegue <xavier.roumegue@oss.nxp.com>, Rob Herring <robh+dt@kernel.org>, linux-imx@nxp.com, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Jacopo Mondi <jacopo.mondi@ideasonboard.com>, Shawn Guo <shawnguo@kernel.org>, linux-media@vger.kernel.org, Laurent Pinchart <laurent.pinchart@ideasonboard.com> Subject: Re: [PATCH v1 1/2] arm64: dts: imx8mp: Add CSIS DT nodes Date: Mon, 17 Apr 2023 07:45:15 -0500 [thread overview] Message-ID: <CAHCN7xJ26TMD4U_5wTtGcGFHZBTTewVRd+mnKa5Ff5cRxBdHPA@mail.gmail.com> (raw) In-Reply-To: <3232774.44csPzL39Z@steina-w> On Mon, Apr 17, 2023 at 5:11 AM Alexander Stein <alexander.stein@ew.tq-group.com> wrote: > > Am Montag, 17. April 2023, 10:15:10 CEST schrieb Laurent Pinchart: > > Hi Marco, > > > > On Mon, Apr 17, 2023 at 10:01:17AM +0200, Marco Felsch wrote: > > > On 23-04-17, Laurent Pinchart wrote: > > > > On Mon, Apr 17, 2023 at 08:50:59AM +0200, Marco Felsch wrote: > > > > > Hi Laurent, > > > > > > > > > > your patch LGTM just one nit/idea, please see below. > > > > > > > > > > On 23-04-17, 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> > > > > > > --- > > > > > > > > > > > > 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>; > > > > > > + power-domains = > <&media_blk_ctrl > > > > > > IMX8MP_MEDIABLK_PD_MIPI_CSI2_1>; > > > > > > + status = "disabled"; > > > > > > + > > > > > > + ports { > > > > > > + #address-cells = > <1>; > > > > > > + #size-cells = <0>; > > > > > > + > > > > > > + port@0 { > > > > > > + reg = > <0>; > > > > > > > > > > If we would add: > > > > > mipi_csi_0_in: > endpoint {}; > > > > > > > > > > here we could refernce it from overlays/board dts files more easily. > > > > > > > > Isn't there an unwritten rule (or consensus) that an endpoint should > > > > always have a remote-endpoint property ? > > > > > > I don't know if there is one. > > > > > > > While ports describe hardware properties of a device and should always > > > > be there regardless of connections, endpoints describe connections and > > > > I don't think they should be instantiated with a valid > > > > remote-endpoint. > > > > > > I know, therefore I mentioned it as idea to make it 'easier' to add > > > camera nodes. > > > > As a middleground, would it be useful to have a label for the port ? > > Something like > > > > mipi_csi_0: csi@32e40000 { > > ports { > > mipi_csi_0_port_0: port@0 { > > }; > > }; > > }; > > > > An overlay could then reference that and create the endpoint. I'm not > > entirely sure how useful that would be though, as the overlay would need > > to enable the CSI node anyway. Compare > > > > -------- > > &mipi_csi_0 { > > status = "okay"; > > }; > > > > &mipi_csi_0_port_0 { > > mipi_csi_0_in: endpoint { > > remote-endpoint = <&imx327_out>; > > }; > > }; > > -------- > > > > with > > > > -------- > > &mipi_csi_0 { > > status = "okay"; > > > > ports { > > port@0 { > > mipi_csi_0_in: endpoint { > > remote-endpoint = <&imx327_out>; > > }; > > }; > > }; > > }; > > -------- > > > > I have a slight preference for the latter as it groups all the CSI0 data > > in a single overlay target, but if the former is generally preferred, > > I'm fine with that too. > > The former is more compact, but also raises the following dtc warnings while > creating the .dtbo: > Warning (graph_endpoint): /fragment@4/__overlay__: graph endpoint node name > should be 'endpoint' > Warning (graph_endpoint): /fragment@4/__overlay__: graph connection to node '/ > fragment@1/__overlay__/ports/port@1/endpoint' is not bidirectional > > for the following snippet: > > &mipi_csi_0_out { > remote-endpoint = <&isp1_in>; > }; > > I'm not sure if there is a chance to fix at all. Once there is consensus on how this should be generically plumbed, please keep me in the loop, so I can add the corresponding imx8m Nano trees as well. I've tested Laurent's work for a while on the Nano that I have. I was going to push DT updates for Nano, then I saw this conversation, so I decided to hold off for now. adam > > Best regards, > Alexander > > > > > > > > > + }; > > > > > > + > > > > > > + 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>; > > > > > > + 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>; > > > -- > TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany > Amtsgericht München, HRB 105018 > Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider > http://www.tq-group.com/ > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2023-04-17 12:45 UTC|newest] Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-04-17 5:56 [PATCH v1 0/2] arm64: dts: imx8mp: Enable CSIS and ISI in DT Laurent Pinchart 2023-04-17 5:56 ` Laurent Pinchart 2023-04-17 5:56 ` [PATCH v1 1/2] arm64: dts: imx8mp: Add CSIS DT nodes Laurent Pinchart 2023-04-17 5:56 ` Laurent Pinchart 2023-04-17 6:50 ` Marco Felsch 2023-04-17 6:50 ` Marco Felsch 2023-04-17 7:41 ` Laurent Pinchart 2023-04-17 7:41 ` Laurent Pinchart 2023-04-17 8:01 ` Marco Felsch 2023-04-17 8:01 ` Marco Felsch 2023-04-17 8:15 ` Laurent Pinchart 2023-04-17 8:15 ` Laurent Pinchart 2023-04-17 8:23 ` Marco Felsch 2023-04-17 8:23 ` Marco Felsch 2023-04-17 9:59 ` Alexander Stein 2023-04-17 9:59 ` Alexander Stein 2023-04-17 12:45 ` Adam Ford [this message] 2023-04-17 12:45 ` Adam Ford 2023-04-17 13:15 ` Marco Felsch 2023-04-17 13:15 ` Marco Felsch 2023-07-07 23:55 ` Tim Harvey 2023-07-07 23:55 ` Tim Harvey 2023-11-29 3:17 ` Adam Ford 2023-11-29 3:17 ` Adam Ford 2023-11-29 10:55 ` Laurent Pinchart 2023-11-29 10:55 ` Laurent Pinchart 2023-11-29 20:14 ` Kieran Bingham 2023-11-29 20:14 ` Kieran Bingham 2023-11-29 20:55 ` Adam Ford 2023-11-29 20:55 ` Adam Ford 2023-04-17 5:56 ` [PATCH v1 2/2] arm64: dts: imx8mp: Add ISI DT node Laurent Pinchart 2023-04-17 5:56 ` Laurent Pinchart 2023-04-17 6:13 ` Alexander Stein 2023-04-17 6:13 ` Alexander Stein 2023-04-18 9:02 ` [PATCH v1 0/2] arm64: dts: imx8mp: Enable CSIS and ISI in DT Paul Elder 2023-04-18 9:02 ` Paul Elder
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAHCN7xJ26TMD4U_5wTtGcGFHZBTTewVRd+mnKa5Ff5cRxBdHPA@mail.gmail.com \ --to=aford173@gmail.com \ --cc=alexander.stein@ew.tq-group.com \ --cc=devicetree@vger.kernel.org \ --cc=jacopo.mondi@ideasonboard.com \ --cc=kernel@pengutronix.de \ --cc=krzysztof.kozlowski+dt@linaro.org \ --cc=laurent.pinchart@ideasonboard.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-imx@nxp.com \ --cc=linux-media@vger.kernel.org \ --cc=m.felsch@pengutronix.de \ --cc=robh+dt@kernel.org \ --cc=shawnguo@kernel.org \ --cc=xavier.roumegue@oss.nxp.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.