* [PATCH v2] arm64: dts: imx8mp: Add DT nodes for the two ISPs @ 2024-03-25 15:13 Laurent Pinchart 2024-03-25 15:16 ` Adam Ford 2024-03-25 15:52 ` Alexander Stein 0 siblings, 2 replies; 14+ messages in thread From: Laurent Pinchart @ 2024-03-25 15:13 UTC (permalink / raw) To: imx, linux-arm-kernel Cc: Paul Elder, Adam Ford, Alexander Stein, Conor Dooley, Fabio Estevam, Krzysztof Kozlowski, Marek Vasut, Pengutronix Kernel Team, Rob Herring, Sascha Hauer, Shawn Guo, devicetree, linux-media From: Paul Elder <paul.elder@ideasonboard.com> The ISP supports both CSI and parallel interfaces, where port 0 corresponds to the former and port 1 corresponds to the latter. Since the i.MX8MP's ISPs are connected by the parallel interface to the CSI receiver, set them both to port 1. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- Changes since v1: - Fix clock ordering - Add #address-cells and #size-cells to ports nodes --- arch/arm64/boot/dts/freescale/imx8mp.dtsi | 50 +++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi index bfc5c81a5bd4..1d2670b91b53 100644 --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi @@ -1616,6 +1616,56 @@ isi_in_1: endpoint { }; }; + isp_0: isp@32e10000 { + compatible = "fsl,imx8mp-isp"; + reg = <0x32e10000 0x10000>; + interrupts = <GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&clk IMX8MP_CLK_MEDIA_ISP_ROOT>, + <&clk IMX8MP_CLK_MEDIA_AXI_ROOT>, + <&clk IMX8MP_CLK_MEDIA_APB_ROOT>; + clock-names = "isp", "aclk", "hclk"; + assigned-clocks = <&clk IMX8MP_CLK_MEDIA_ISP>; + assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; + assigned-clock-rates = <500000000>; + power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_ISP>; + fsl,blk-ctrl = <&media_blk_ctrl 0>; + status = "disabled"; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@1 { + reg = <1>; + }; + }; + }; + + isp_1: isp@32e20000 { + compatible = "fsl,imx8mp-isp"; + reg = <0x32e20000 0x10000>; + interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&clk IMX8MP_CLK_MEDIA_ISP_ROOT>, + <&clk IMX8MP_CLK_MEDIA_AXI_ROOT>, + <&clk IMX8MP_CLK_MEDIA_APB_ROOT>; + clock-names = "isp", "aclk", "hclk"; + assigned-clocks = <&clk IMX8MP_CLK_MEDIA_ISP>; + assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; + assigned-clock-rates = <500000000>; + power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_ISP>; + fsl,blk-ctrl = <&media_blk_ctrl 1>; + status = "disabled"; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@1 { + reg = <1>; + }; + }; + }; + dewarp: dwe@32e30000 { compatible = "nxp,imx8mp-dw100"; reg = <0x32e30000 0x10000>; base-commit: 4cece764965020c22cff7665b18a012006359095 -- Regards, Laurent Pinchart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2] arm64: dts: imx8mp: Add DT nodes for the two ISPs 2024-03-25 15:13 [PATCH v2] arm64: dts: imx8mp: Add DT nodes for the two ISPs Laurent Pinchart @ 2024-03-25 15:16 ` Adam Ford 2024-03-25 15:52 ` Alexander Stein 1 sibling, 0 replies; 14+ messages in thread From: Adam Ford @ 2024-03-25 15:16 UTC (permalink / raw) To: Laurent Pinchart Cc: imx, linux-arm-kernel, Paul Elder, Alexander Stein, Conor Dooley, Fabio Estevam, Krzysztof Kozlowski, Marek Vasut, Pengutronix Kernel Team, Rob Herring, Sascha Hauer, Shawn Guo, devicetree, linux-media On Mon, Mar 25, 2024 at 10:13 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > From: Paul Elder <paul.elder@ideasonboard.com> > > The ISP supports both CSI and parallel interfaces, where port 0 > corresponds to the former and port 1 corresponds to the latter. Since > the i.MX8MP's ISPs are connected by the parallel interface to the CSI > receiver, set them both to port 1. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Tested-by: Adam Ford <aford173@gmail.com> # imx8mp-beacon > --- > Changes since v1: > > - Fix clock ordering > - Add #address-cells and #size-cells to ports nodes > --- > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 50 +++++++++++++++++++++++ > 1 file changed, 50 insertions(+) > > diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > index bfc5c81a5bd4..1d2670b91b53 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > @@ -1616,6 +1616,56 @@ isi_in_1: endpoint { > }; > }; > > + isp_0: isp@32e10000 { > + compatible = "fsl,imx8mp-isp"; > + reg = <0x32e10000 0x10000>; > + interrupts = <GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&clk IMX8MP_CLK_MEDIA_ISP_ROOT>, > + <&clk IMX8MP_CLK_MEDIA_AXI_ROOT>, > + <&clk IMX8MP_CLK_MEDIA_APB_ROOT>; > + clock-names = "isp", "aclk", "hclk"; > + assigned-clocks = <&clk IMX8MP_CLK_MEDIA_ISP>; > + assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; > + assigned-clock-rates = <500000000>; > + power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_ISP>; > + fsl,blk-ctrl = <&media_blk_ctrl 0>; > + status = "disabled"; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@1 { > + reg = <1>; > + }; > + }; > + }; > + > + isp_1: isp@32e20000 { > + compatible = "fsl,imx8mp-isp"; > + reg = <0x32e20000 0x10000>; > + interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&clk IMX8MP_CLK_MEDIA_ISP_ROOT>, > + <&clk IMX8MP_CLK_MEDIA_AXI_ROOT>, > + <&clk IMX8MP_CLK_MEDIA_APB_ROOT>; > + clock-names = "isp", "aclk", "hclk"; > + assigned-clocks = <&clk IMX8MP_CLK_MEDIA_ISP>; > + assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; > + assigned-clock-rates = <500000000>; > + power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_ISP>; > + fsl,blk-ctrl = <&media_blk_ctrl 1>; > + status = "disabled"; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@1 { > + reg = <1>; > + }; > + }; > + }; > + > dewarp: dwe@32e30000 { > compatible = "nxp,imx8mp-dw100"; > reg = <0x32e30000 0x10000>; > > base-commit: 4cece764965020c22cff7665b18a012006359095 > -- > Regards, > > Laurent Pinchart > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] arm64: dts: imx8mp: Add DT nodes for the two ISPs 2024-03-25 15:13 [PATCH v2] arm64: dts: imx8mp: Add DT nodes for the two ISPs Laurent Pinchart 2024-03-25 15:16 ` Adam Ford @ 2024-03-25 15:52 ` Alexander Stein 2024-03-25 20:49 ` Laurent Pinchart 1 sibling, 1 reply; 14+ messages in thread From: Alexander Stein @ 2024-03-25 15:52 UTC (permalink / raw) To: imx, linux-arm-kernel, Laurent Pinchart Cc: Paul Elder, Adam Ford, Conor Dooley, Fabio Estevam, Krzysztof Kozlowski, Marek Vasut, Pengutronix Kernel Team, Rob Herring, Sascha Hauer, Shawn Guo, devicetree, linux-media Hi Laurent, Am Montag, 25. März 2024, 16:13:39 CET schrieb Laurent Pinchart: > From: Paul Elder <paul.elder@ideasonboard.com> > > The ISP supports both CSI and parallel interfaces, where port 0 > corresponds to the former and port 1 corresponds to the latter. Since > the i.MX8MP's ISPs are connected by the parallel interface to the CSI > receiver, set them both to port 1. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Changes since v1: > > - Fix clock ordering > - Add #address-cells and #size-cells to ports nodes > --- > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 50 +++++++++++++++++++++++ > 1 file changed, 50 insertions(+) > > diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > index bfc5c81a5bd4..1d2670b91b53 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > @@ -1616,6 +1616,56 @@ isi_in_1: endpoint { > }; > }; > > + isp_0: isp@32e10000 { > + compatible = "fsl,imx8mp-isp"; > + reg = <0x32e10000 0x10000>; > + interrupts = <GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&clk IMX8MP_CLK_MEDIA_ISP_ROOT>, > + <&clk IMX8MP_CLK_MEDIA_AXI_ROOT>, > + <&clk IMX8MP_CLK_MEDIA_APB_ROOT>; > + clock-names = "isp", "aclk", "hclk"; > + assigned-clocks = <&clk IMX8MP_CLK_MEDIA_ISP>; > + assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; > + assigned-clock-rates = <500000000>; > + power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_ISP>; > + fsl,blk-ctrl = <&media_blk_ctrl 0>; > + status = "disabled"; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@1 { > + reg = <1>; > + }; > + }; > + }; > + > + isp_1: isp@32e20000 { > + compatible = "fsl,imx8mp-isp"; > + reg = <0x32e20000 0x10000>; > + interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&clk IMX8MP_CLK_MEDIA_ISP_ROOT>, > + <&clk IMX8MP_CLK_MEDIA_AXI_ROOT>, > + <&clk IMX8MP_CLK_MEDIA_APB_ROOT>; > + clock-names = "isp", "aclk", "hclk"; > + assigned-clocks = <&clk IMX8MP_CLK_MEDIA_ISP>; > + assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; > + assigned-clock-rates = <500000000>; > + power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_ISP>; > + fsl,blk-ctrl = <&media_blk_ctrl 1>; > + status = "disabled"; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@1 { > + reg = <1>; > + }; > + }; > + }; > + The patch itself is okay. But you might not be able to configure the parent of IMX8MP_CLK_MEDIA_ISP if dewarp is enabled before. This is due to IMX8MP_CLK_MEDIA_ISP_ROOT being enabled in 'pgc_ispdwp' power domain. Reparenting is not possible anymore in this case. Something like ---8<--- --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi @@ -1837,11 +1837,13 @@ media_blk_ctrl: blk-ctrl@32ec0000 { <&clk IMX8MP_CLK_MEDIA_APB>, <&clk IMX8MP_CLK_MEDIA_DISP1_PIX>, <&clk IMX8MP_CLK_MEDIA_DISP2_PIX>, + <&clk IMX8MP_CLK_MEDIA_ISP>, <&clk IMX8MP_VIDEO_PLL1>; assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>, <&clk IMX8MP_SYS_PLL1_800M>, <&clk IMX8MP_VIDEO_PLL1_OUT>, - <&clk IMX8MP_VIDEO_PLL1_OUT>; + <&clk IMX8MP_VIDEO_PLL1_OUT>, + <&clk IMX8MP_SYS_PLL2_500M>; assigned-clock-rates = <500000000>, <200000000>, <0>, <0>, <1039500000>; #power-domain-cells = <1>; ---8<--- is needed. Best regards, Alexander > dewarp: dwe@32e30000 { > compatible = "nxp,imx8mp-dw100"; > reg = <0x32e30000 0x10000>; > > base-commit: 4cece764965020c22cff7665b18a012006359095 > -- 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] arm64: dts: imx8mp: Add DT nodes for the two ISPs 2024-03-25 15:52 ` Alexander Stein @ 2024-03-25 20:49 ` Laurent Pinchart 2024-03-26 7:14 ` Alexander Stein 0 siblings, 1 reply; 14+ messages in thread From: Laurent Pinchart @ 2024-03-25 20:49 UTC (permalink / raw) To: Alexander Stein Cc: imx, linux-arm-kernel, Paul Elder, Adam Ford, Conor Dooley, Fabio Estevam, Krzysztof Kozlowski, Marek Vasut, Pengutronix Kernel Team, Rob Herring, Sascha Hauer, Shawn Guo, devicetree, linux-media Hi Alexander, On Mon, Mar 25, 2024 at 04:52:21PM +0100, Alexander Stein wrote: > Am Montag, 25. März 2024, 16:13:39 CET schrieb Laurent Pinchart: > > From: Paul Elder <paul.elder@ideasonboard.com> > > > > The ISP supports both CSI and parallel interfaces, where port 0 > > corresponds to the former and port 1 corresponds to the latter. Since > > the i.MX8MP's ISPs are connected by the parallel interface to the CSI > > receiver, set them both to port 1. > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > Changes since v1: > > > > - Fix clock ordering > > - Add #address-cells and #size-cells to ports nodes > > --- > > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 50 +++++++++++++++++++++++ > > 1 file changed, 50 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > index bfc5c81a5bd4..1d2670b91b53 100644 > > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > @@ -1616,6 +1616,56 @@ isi_in_1: endpoint { > > }; > > }; > > > > + isp_0: isp@32e10000 { > > + compatible = "fsl,imx8mp-isp"; > > + reg = <0x32e10000 0x10000>; > > + interrupts = <GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>; > > + clocks = <&clk IMX8MP_CLK_MEDIA_ISP_ROOT>, > > + <&clk IMX8MP_CLK_MEDIA_AXI_ROOT>, > > + <&clk IMX8MP_CLK_MEDIA_APB_ROOT>; > > + clock-names = "isp", "aclk", "hclk"; > > + assigned-clocks = <&clk IMX8MP_CLK_MEDIA_ISP>; > > + assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; > > + assigned-clock-rates = <500000000>; > > + power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_ISP>; > > + fsl,blk-ctrl = <&media_blk_ctrl 0>; > > + status = "disabled"; > > + > > + ports { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + port@1 { > > + reg = <1>; > > + }; > > + }; > > + }; > > + > > + isp_1: isp@32e20000 { > > + compatible = "fsl,imx8mp-isp"; > > + reg = <0x32e20000 0x10000>; > > + interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>; > > + clocks = <&clk IMX8MP_CLK_MEDIA_ISP_ROOT>, > > + <&clk IMX8MP_CLK_MEDIA_AXI_ROOT>, > > + <&clk IMX8MP_CLK_MEDIA_APB_ROOT>; > > + clock-names = "isp", "aclk", "hclk"; > > + assigned-clocks = <&clk IMX8MP_CLK_MEDIA_ISP>; > > + assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; > > + assigned-clock-rates = <500000000>; > > + power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_ISP>; > > + fsl,blk-ctrl = <&media_blk_ctrl 1>; > > + status = "disabled"; > > + > > + ports { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + port@1 { > > + reg = <1>; > > + }; > > + }; > > + }; > > + > > The patch itself is okay. But you might not be able to > configure the parent of IMX8MP_CLK_MEDIA_ISP if dewarp is enabled before. > This is due to IMX8MP_CLK_MEDIA_ISP_ROOT being enabled in 'pgc_ispdwp' > power domain. Reparenting is not possible anymore in this case. Good point. > Something like > ---8<--- > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > @@ -1837,11 +1837,13 @@ media_blk_ctrl: blk-ctrl@32ec0000 { > <&clk IMX8MP_CLK_MEDIA_APB>, > <&clk IMX8MP_CLK_MEDIA_DISP1_PIX>, > <&clk IMX8MP_CLK_MEDIA_DISP2_PIX>, > + <&clk IMX8MP_CLK_MEDIA_ISP>, > <&clk IMX8MP_VIDEO_PLL1>; > assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>, > <&clk IMX8MP_SYS_PLL1_800M>, > <&clk IMX8MP_VIDEO_PLL1_OUT>, > - <&clk IMX8MP_VIDEO_PLL1_OUT>; > + <&clk IMX8MP_VIDEO_PLL1_OUT>, > + <&clk IMX8MP_SYS_PLL2_500M>; > assigned-clock-rates = <500000000>, <200000000>, > <0>, <0>, <1039500000>; With an assigned clock rate here too then ? > #power-domain-cells = <1>; > ---8<--- > is needed. Sascha, are you OK with this approach ? > > dewarp: dwe@32e30000 { > > compatible = "nxp,imx8mp-dw100"; > > reg = <0x32e30000 0x10000>; > > > > base-commit: 4cece764965020c22cff7665b18a012006359095 -- Regards, Laurent Pinchart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] arm64: dts: imx8mp: Add DT nodes for the two ISPs 2024-03-25 20:49 ` Laurent Pinchart @ 2024-03-26 7:14 ` Alexander Stein 2024-06-09 20:21 ` Adam Ford 0 siblings, 1 reply; 14+ messages in thread From: Alexander Stein @ 2024-03-26 7:14 UTC (permalink / raw) To: Laurent Pinchart Cc: imx, linux-arm-kernel, Paul Elder, Adam Ford, Conor Dooley, Fabio Estevam, Krzysztof Kozlowski, Marek Vasut, Pengutronix Kernel Team, Rob Herring, Sascha Hauer, Shawn Guo, devicetree, linux-media Hi Laurent, Am Montag, 25. März 2024, 21:49:24 CET schrieb Laurent Pinchart: > Hi Alexander, > > On Mon, Mar 25, 2024 at 04:52:21PM +0100, Alexander Stein wrote: > > Am Montag, 25. März 2024, 16:13:39 CET schrieb Laurent Pinchart: > > > From: Paul Elder <paul.elder@ideasonboard.com> > > > > > > The ISP supports both CSI and parallel interfaces, where port 0 > > > corresponds to the former and port 1 corresponds to the latter. Since > > > the i.MX8MP's ISPs are connected by the parallel interface to the CSI > > > receiver, set them both to port 1. > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > Changes since v1: > > > > > > - Fix clock ordering > > > - Add #address-cells and #size-cells to ports nodes > > > --- > > > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 50 +++++++++++++++++++++++ > > > 1 file changed, 50 insertions(+) > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > index bfc5c81a5bd4..1d2670b91b53 100644 > > > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > @@ -1616,6 +1616,56 @@ isi_in_1: endpoint { > > > }; > > > }; > > > > > > + isp_0: isp@32e10000 { > > > + compatible = "fsl,imx8mp-isp"; > > > + reg = <0x32e10000 0x10000>; > > > + interrupts = <GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>; > > > + clocks = <&clk IMX8MP_CLK_MEDIA_ISP_ROOT>, > > > + <&clk IMX8MP_CLK_MEDIA_AXI_ROOT>, > > > + <&clk IMX8MP_CLK_MEDIA_APB_ROOT>; > > > + clock-names = "isp", "aclk", "hclk"; > > > + assigned-clocks = <&clk IMX8MP_CLK_MEDIA_ISP>; > > > + assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; > > > + assigned-clock-rates = <500000000>; > > > + power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_ISP>; > > > + fsl,blk-ctrl = <&media_blk_ctrl 0>; > > > + status = "disabled"; > > > + > > > + ports { > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + > > > + port@1 { > > > + reg = <1>; > > > + }; > > > + }; > > > + }; > > > + > > > + isp_1: isp@32e20000 { > > > + compatible = "fsl,imx8mp-isp"; > > > + reg = <0x32e20000 0x10000>; > > > + interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>; > > > + clocks = <&clk IMX8MP_CLK_MEDIA_ISP_ROOT>, > > > + <&clk IMX8MP_CLK_MEDIA_AXI_ROOT>, > > > + <&clk IMX8MP_CLK_MEDIA_APB_ROOT>; > > > + clock-names = "isp", "aclk", "hclk"; > > > + assigned-clocks = <&clk IMX8MP_CLK_MEDIA_ISP>; > > > + assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; > > > + assigned-clock-rates = <500000000>; > > > + power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_ISP>; > > > + fsl,blk-ctrl = <&media_blk_ctrl 1>; > > > + status = "disabled"; > > > + > > > + ports { > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + > > > + port@1 { > > > + reg = <1>; > > > + }; > > > + }; > > > + }; > > > + > > > > The patch itself is okay. But you might not be able to > > configure the parent of IMX8MP_CLK_MEDIA_ISP if dewarp is enabled before. > > This is due to IMX8MP_CLK_MEDIA_ISP_ROOT being enabled in 'pgc_ispdwp' > > power domain. Reparenting is not possible anymore in this case. > > Good point. > > > Something like > > ---8<--- > > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > @@ -1837,11 +1837,13 @@ media_blk_ctrl: blk-ctrl@32ec0000 { > > <&clk IMX8MP_CLK_MEDIA_APB>, > > <&clk IMX8MP_CLK_MEDIA_DISP1_PIX>, > > <&clk IMX8MP_CLK_MEDIA_DISP2_PIX>, > > + <&clk IMX8MP_CLK_MEDIA_ISP>, > > <&clk IMX8MP_VIDEO_PLL1>; > > assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>, > > <&clk IMX8MP_SYS_PLL1_800M>, > > <&clk IMX8MP_VIDEO_PLL1_OUT>, > > - <&clk IMX8MP_VIDEO_PLL1_OUT>; > > + <&clk IMX8MP_VIDEO_PLL1_OUT>, > > + <&clk IMX8MP_SYS_PLL2_500M>; > > assigned-clock-rates = <500000000>, <200000000>, > > <0>, <0>, <1039500000>; > > With an assigned clock rate here too then ? You are right. This posted diff is what I was using for a while now though. Apparently the clock frequency was still correct. Best regards, Alexander > > #power-domain-cells = <1>; > > ---8<--- > > is needed. > > Sascha, are you OK with this approach ? > > > > dewarp: dwe@32e30000 { > > > compatible = "nxp,imx8mp-dw100"; > > > reg = <0x32e30000 0x10000>; > > > > > > base-commit: 4cece764965020c22cff7665b18a012006359095 > > -- 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] arm64: dts: imx8mp: Add DT nodes for the two ISPs 2024-03-26 7:14 ` Alexander Stein @ 2024-06-09 20:21 ` Adam Ford 2024-06-11 1:04 ` Peng Fan 2024-08-13 22:22 ` Laurent Pinchart 0 siblings, 2 replies; 14+ messages in thread From: Adam Ford @ 2024-06-09 20:21 UTC (permalink / raw) To: Alexander Stein Cc: Laurent Pinchart, imx, linux-arm-kernel, Paul Elder, Conor Dooley, Fabio Estevam, Krzysztof Kozlowski, Marek Vasut, Pengutronix Kernel Team, Rob Herring, Sascha Hauer, Shawn Guo, devicetree, linux-media On Tue, Mar 26, 2024 at 2:14 AM Alexander Stein <alexander.stein@ew.tq-group.com> wrote: > > Hi Laurent, > > Am Montag, 25. März 2024, 21:49:24 CET schrieb Laurent Pinchart: > > Hi Alexander, > > > > On Mon, Mar 25, 2024 at 04:52:21PM +0100, Alexander Stein wrote: > > > Am Montag, 25. März 2024, 16:13:39 CET schrieb Laurent Pinchart: > > > > From: Paul Elder <paul.elder@ideasonboard.com> > > > > > > > > The ISP supports both CSI and parallel interfaces, where port 0 > > > > corresponds to the former and port 1 corresponds to the latter. Since > > > > the i.MX8MP's ISPs are connected by the parallel interface to the CSI > > > > receiver, set them both to port 1. > > > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > --- > > > > Changes since v1: > > > > > > > > - Fix clock ordering > > > > - Add #address-cells and #size-cells to ports nodes > > > > --- > > > > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 50 +++++++++++++++++++++++ > > > > 1 file changed, 50 insertions(+) > > > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > > index bfc5c81a5bd4..1d2670b91b53 100644 > > > > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > > @@ -1616,6 +1616,56 @@ isi_in_1: endpoint { > > > > }; > > > > }; > > > > > > > > + isp_0: isp@32e10000 { > > > > + compatible = "fsl,imx8mp-isp"; > > > > + reg = <0x32e10000 0x10000>; > > > > + interrupts = <GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>; > > > > + clocks = <&clk IMX8MP_CLK_MEDIA_ISP_ROOT>, > > > > + <&clk IMX8MP_CLK_MEDIA_AXI_ROOT>, > > > > + <&clk IMX8MP_CLK_MEDIA_APB_ROOT>; > > > > + clock-names = "isp", "aclk", "hclk"; > > > > + assigned-clocks = <&clk IMX8MP_CLK_MEDIA_ISP>; > > > > + assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; > > > > + assigned-clock-rates = <500000000>; > > > > + power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_ISP>; > > > > + fsl,blk-ctrl = <&media_blk_ctrl 0>; > > > > + status = "disabled"; > > > > + > > > > + ports { > > > > + #address-cells = <1>; > > > > + #size-cells = <0>; > > > > + > > > > + port@1 { > > > > + reg = <1>; > > > > + }; > > > > + }; > > > > + }; > > > > + > > > > + isp_1: isp@32e20000 { > > > > + compatible = "fsl,imx8mp-isp"; > > > > + reg = <0x32e20000 0x10000>; > > > > + interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>; > > > > + clocks = <&clk IMX8MP_CLK_MEDIA_ISP_ROOT>, > > > > + <&clk IMX8MP_CLK_MEDIA_AXI_ROOT>, > > > > + <&clk IMX8MP_CLK_MEDIA_APB_ROOT>; > > > > + clock-names = "isp", "aclk", "hclk"; > > > > + assigned-clocks = <&clk IMX8MP_CLK_MEDIA_ISP>; > > > > + assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; > > > > + assigned-clock-rates = <500000000>; > > > > + power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_ISP>; > > > > + fsl,blk-ctrl = <&media_blk_ctrl 1>; > > > > + status = "disabled"; > > > > + > > > > + ports { > > > > + #address-cells = <1>; > > > > + #size-cells = <0>; > > > > + > > > > + port@1 { > > > > + reg = <1>; > > > > + }; > > > > + }; > > > > + }; > > > > + > > > > > > The patch itself is okay. But you might not be able to > > > configure the parent of IMX8MP_CLK_MEDIA_ISP if dewarp is enabled before. > > > This is due to IMX8MP_CLK_MEDIA_ISP_ROOT being enabled in 'pgc_ispdwp' > > > power domain. Reparenting is not possible anymore in this case. > > > > Good point. > > > > > Something like > > > ---8<--- > > > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > @@ -1837,11 +1837,13 @@ media_blk_ctrl: blk-ctrl@32ec0000 { > > > <&clk IMX8MP_CLK_MEDIA_APB>, > > > <&clk IMX8MP_CLK_MEDIA_DISP1_PIX>, > > > <&clk IMX8MP_CLK_MEDIA_DISP2_PIX>, > > > + <&clk IMX8MP_CLK_MEDIA_ISP>, > > > <&clk IMX8MP_VIDEO_PLL1>; > > > assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>, > > > <&clk IMX8MP_SYS_PLL1_800M>, > > > <&clk IMX8MP_VIDEO_PLL1_OUT>, > > > - <&clk IMX8MP_VIDEO_PLL1_OUT>; > > > + <&clk IMX8MP_VIDEO_PLL1_OUT>, > > > + <&clk IMX8MP_SYS_PLL2_500M>; > > > assigned-clock-rates = <500000000>, <200000000>, > > > <0>, <0>, <1039500000>; > > According to the i.MX8MP Data sheet, the nominal speed for MEDIA_ISP_CLOCL_ROOT is 400MHZ with 500MHz being allowed in overdrive mode. I think this clock rate should drop to the nominal value of 400MHz and those boards who support overdrive can increase it to 500MHz to avoid stiability issues and/or running out of spec. I created an imx8mm and imx8mn-overdrive.dtsi file. If there is interest, I can do the same for the 8MP as well. I haven't gone through all the clocks to determine if/what clocks are being overdriven. > > With an assigned clock rate here too then ? > > You are right. This posted diff is what I was using for a while now though. > Apparently the clock frequency was still correct. > > Best regards, > Alexander > > > > #power-domain-cells = <1>; > > > ---8<--- > > > is needed. > > > > Sascha, are you OK with this approach ? This patch appears to have gone stale. Is there any way we can push this forward? Thanks, adam > > > > > > dewarp: dwe@32e30000 { > > > > compatible = "nxp,imx8mp-dw100"; > > > > reg = <0x32e30000 0x10000>; > > > > > > > > base-commit: 4cece764965020c22cff7665b18a012006359095 > > > > > > > -- > 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH v2] arm64: dts: imx8mp: Add DT nodes for the two ISPs 2024-06-09 20:21 ` Adam Ford @ 2024-06-11 1:04 ` Peng Fan 2024-06-11 1:34 ` Marek Vasut 2024-08-13 22:22 ` Laurent Pinchart 1 sibling, 1 reply; 14+ messages in thread From: Peng Fan @ 2024-06-11 1:04 UTC (permalink / raw) To: Adam Ford, Alexander Stein Cc: Laurent Pinchart, imx@lists.linux.dev, linux-arm-kernel@lists.infradead.org, Paul Elder, Conor Dooley, Fabio Estevam, Krzysztof Kozlowski, Marek Vasut, Pengutronix Kernel Team, Rob Herring, Sascha Hauer, Shawn Guo, devicetree@vger.kernel.org, linux-media@vger.kernel.org > Subject: Re: [PATCH v2] arm64: dts: imx8mp: Add DT nodes for the two ISPs > > > > > > > Something like > > > > ---8<--- > > > > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > > @@ -1837,11 +1837,13 @@ media_blk_ctrl: blk-ctrl@32ec0000 { > > > > <&clk IMX8MP_CLK_MEDIA_APB>, > > > > <&clk IMX8MP_CLK_MEDIA_DISP1_PIX>, > > > > <&clk > > > > IMX8MP_CLK_MEDIA_DISP2_PIX>, > > > > + <&clk > > > > + IMX8MP_CLK_MEDIA_ISP>, > > > > <&clk IMX8MP_VIDEO_PLL1>; > > > > assigned-clock-parents = <&clk > IMX8MP_SYS_PLL2_1000M>, > > > > <&clk IMX8MP_SYS_PLL1_800M>, > > > > <&clk IMX8MP_VIDEO_PLL1_OUT>, > > > > - <&clk IMX8MP_VIDEO_PLL1_OUT>; > > > > + <&clk IMX8MP_VIDEO_PLL1_OUT>, > > > > + <&clk > > > > + IMX8MP_SYS_PLL2_500M>; > > > > assigned-clock-rates = <500000000>, <200000000>, > > > > <0>, <0>, > > > > <1039500000>; > > > > > According to the i.MX8MP Data sheet, the nominal speed for > MEDIA_ISP_CLOCL_ROOT is 400MHZ with 500MHz being allowed in > overdrive mode. > > I think this clock rate should drop to the nominal value of 400MHz and those > boards who support overdrive can increase it to 500MHz to avoid stiability > issues and/or running out of spec. I created an imx8mm and imx8mn- > overdrive.dtsi file. If there is interest, I can do the same for the 8MP as well. > > I haven't gone through all the clocks to determine if/what clocks are being > overdriven. Shouldn't the bootloader take the work to runtime update the freq? Why need introduce an extra overdrive.dtsi? Thanks, Peng. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] arm64: dts: imx8mp: Add DT nodes for the two ISPs 2024-06-11 1:04 ` Peng Fan @ 2024-06-11 1:34 ` Marek Vasut 2024-06-11 3:01 ` Peng Fan 0 siblings, 1 reply; 14+ messages in thread From: Marek Vasut @ 2024-06-11 1:34 UTC (permalink / raw) To: Peng Fan, Adam Ford, Alexander Stein Cc: Laurent Pinchart, imx@lists.linux.dev, linux-arm-kernel@lists.infradead.org, Paul Elder, Conor Dooley, Fabio Estevam, Krzysztof Kozlowski, Pengutronix Kernel Team, Rob Herring, Sascha Hauer, Shawn Guo, devicetree@vger.kernel.org, linux-media@vger.kernel.org On 6/11/24 3:04 AM, Peng Fan wrote: >> Subject: Re: [PATCH v2] arm64: dts: imx8mp: Add DT nodes for the two ISPs >>>> >>>>> Something like >>>>> ---8<--- >>>>> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi >>>>> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi >>>>> @@ -1837,11 +1837,13 @@ media_blk_ctrl: blk-ctrl@32ec0000 { >>>>> <&clk IMX8MP_CLK_MEDIA_APB>, >>>>> <&clk IMX8MP_CLK_MEDIA_DISP1_PIX>, >>>>> <&clk >>>>> IMX8MP_CLK_MEDIA_DISP2_PIX>, >>>>> + <&clk >>>>> + IMX8MP_CLK_MEDIA_ISP>, >>>>> <&clk IMX8MP_VIDEO_PLL1>; >>>>> assigned-clock-parents = <&clk >> IMX8MP_SYS_PLL2_1000M>, >>>>> <&clk IMX8MP_SYS_PLL1_800M>, >>>>> <&clk IMX8MP_VIDEO_PLL1_OUT>, >>>>> - <&clk IMX8MP_VIDEO_PLL1_OUT>; >>>>> + <&clk IMX8MP_VIDEO_PLL1_OUT>, >>>>> + <&clk >>>>> + IMX8MP_SYS_PLL2_500M>; >>>>> assigned-clock-rates = <500000000>, <200000000>, >>>>> <0>, <0>, >>>>> <1039500000>; >>>> >> >> According to the i.MX8MP Data sheet, the nominal speed for >> MEDIA_ISP_CLOCL_ROOT is 400MHZ with 500MHz being allowed in >> overdrive mode. >> >> I think this clock rate should drop to the nominal value of 400MHz and those >> boards who support overdrive can increase it to 500MHz to avoid stiability >> issues and/or running out of spec. I created an imx8mm and imx8mn- >> overdrive.dtsi file. If there is interest, I can do the same for the 8MP as well. >> >> I haven't gone through all the clocks to determine if/what clocks are being >> overdriven. > > Shouldn't the bootloader take the work to runtime update the freq? > Why need introduce an extra overdrive.dtsi? Shouldn't the overdrive/non-overdrive decision be done in board DT instead ? _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH v2] arm64: dts: imx8mp: Add DT nodes for the two ISPs 2024-06-11 1:34 ` Marek Vasut @ 2024-06-11 3:01 ` Peng Fan 2024-06-13 1:10 ` Marek Vasut 0 siblings, 1 reply; 14+ messages in thread From: Peng Fan @ 2024-06-11 3:01 UTC (permalink / raw) To: Marek Vasut, Adam Ford, Alexander Stein Cc: Laurent Pinchart, imx@lists.linux.dev, linux-arm-kernel@lists.infradead.org, Paul Elder, Conor Dooley, Fabio Estevam, Krzysztof Kozlowski, Pengutronix Kernel Team, Rob Herring, Sascha Hauer, Shawn Guo, devicetree@vger.kernel.org, linux-media@vger.kernel.org > Subject: Re: [PATCH v2] arm64: dts: imx8mp: Add DT nodes for the two ISPs > > On 6/11/24 3:04 AM, Peng Fan wrote: > >> Subject: Re: [PATCH v2] arm64: dts: imx8mp: Add DT nodes for the two > >> ISPs > >>>> > >>>>> Something like > >>>>> ---8<--- > >>>>> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > >>>>> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > >>>>> @@ -1837,11 +1837,13 @@ media_blk_ctrl: blk-ctrl@32ec0000 { > >>>>> <&clk IMX8MP_CLK_MEDIA_APB>, > >>>>> <&clk IMX8MP_CLK_MEDIA_DISP1_PIX>, > >>>>> <&clk > >>>>> IMX8MP_CLK_MEDIA_DISP2_PIX>, > >>>>> + <&clk > >>>>> + IMX8MP_CLK_MEDIA_ISP>, > >>>>> <&clk IMX8MP_VIDEO_PLL1>; > >>>>> assigned-clock-parents = <&clk > >> IMX8MP_SYS_PLL2_1000M>, > >>>>> <&clk IMX8MP_SYS_PLL1_800M>, > >>>>> <&clk IMX8MP_VIDEO_PLL1_OUT>, > >>>>> - <&clk IMX8MP_VIDEO_PLL1_OUT>; > >>>>> + <&clk IMX8MP_VIDEO_PLL1_OUT>, > >>>>> + <&clk > >>>>> + IMX8MP_SYS_PLL2_500M>; > >>>>> assigned-clock-rates = <500000000>, <200000000>, > >>>>> <0>, <0>, > >>>>> <1039500000>; > >>>> > >> > >> According to the i.MX8MP Data sheet, the nominal speed for > >> MEDIA_ISP_CLOCL_ROOT is 400MHZ with 500MHz being allowed in > overdrive > >> mode. > >> > >> I think this clock rate should drop to the nominal value of 400MHz > >> and those boards who support overdrive can increase it to 500MHz to > >> avoid stiability issues and/or running out of spec. I created an > >> imx8mm and imx8mn- overdrive.dtsi file. If there is interest, I can do the > same for the 8MP as well. > >> > >> I haven't gone through all the clocks to determine if/what clocks are > >> being overdriven. > > > > Shouldn't the bootloader take the work to runtime update the freq? > > Why need introduce an extra overdrive.dtsi? > > Shouldn't the overdrive/non-overdrive decision be done in board DT instead ? It is bootloader configure voltage to nominal, then bootloader should use nominal device tree or runtime update dtb. If bootloader configure voltage to over-drive, bootloader could use nominal or over-drive dtb If introduce x.dtsi and x-overdrive.dtsi, how to let board choose which dtsi to include? Regards, Peng. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] arm64: dts: imx8mp: Add DT nodes for the two ISPs 2024-06-11 3:01 ` Peng Fan @ 2024-06-13 1:10 ` Marek Vasut 2024-06-13 8:24 ` Marco Felsch 0 siblings, 1 reply; 14+ messages in thread From: Marek Vasut @ 2024-06-13 1:10 UTC (permalink / raw) To: Peng Fan, Adam Ford, Alexander Stein Cc: Laurent Pinchart, imx@lists.linux.dev, linux-arm-kernel@lists.infradead.org, Paul Elder, Conor Dooley, Fabio Estevam, Krzysztof Kozlowski, Pengutronix Kernel Team, Rob Herring, Sascha Hauer, Shawn Guo, devicetree@vger.kernel.org, linux-media@vger.kernel.org On 6/11/24 5:01 AM, Peng Fan wrote: [...] >>>> According to the i.MX8MP Data sheet, the nominal speed for >>>> MEDIA_ISP_CLOCL_ROOT is 400MHZ with 500MHz being allowed in >> overdrive >>>> mode. >>>> >>>> I think this clock rate should drop to the nominal value of 400MHz >>>> and those boards who support overdrive can increase it to 500MHz to >>>> avoid stiability issues and/or running out of spec. I created an >>>> imx8mm and imx8mn- overdrive.dtsi file. If there is interest, I can do the >> same for the 8MP as well. >>>> >>>> I haven't gone through all the clocks to determine if/what clocks are >>>> being overdriven. >>> >>> Shouldn't the bootloader take the work to runtime update the freq? >>> Why need introduce an extra overdrive.dtsi? >> >> Shouldn't the overdrive/non-overdrive decision be done in board DT instead ? > > It is bootloader configure voltage to nominal, then bootloader should use > nominal device tree or runtime update dtb. > If bootloader configure voltage to over-drive, bootloader could use > nominal or over-drive dtb I think the bootloader should always configure the minimal common configuration, i.e. nominal voltage, nominal clock, so that it would achieve maximum compatibility with any SoC in that SoC line up. If the user does need overdrive configuration, they should specify that in their board DT. Keep in mind, the kernel is easy to update (including kernel DT), the bootloader is not easy to update (esp. in field, bootloader update may render a system unbootable if it fails). I would say, it is better to keep the complicated things out of the bootloader if at all possible. > If introduce x.dtsi and x-overdrive.dtsi, how to let board choose which dtsi > to include? #include "x.dtsi" or #include "x-overdrive.dtsi" But I think your question is -- how to do that at runtime ? U-Boot can apply DT overlays onto DT that is passed to Linux, so if the user has board variants where they need both nonoverdrive/overdrive options, they can apply DT overlay which enables the overdrive mode on boards which need it. This can be done from U-Boot boot.scr or similar boot script, which can again be easily updated, without the need to update the bootloader itself (if something goes wrong or needs to be changed in the future). ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] arm64: dts: imx8mp: Add DT nodes for the two ISPs 2024-06-13 1:10 ` Marek Vasut @ 2024-06-13 8:24 ` Marco Felsch 0 siblings, 0 replies; 14+ messages in thread From: Marco Felsch @ 2024-06-13 8:24 UTC (permalink / raw) To: Marek Vasut Cc: Peng Fan, Adam Ford, Alexander Stein, imx@lists.linux.dev, Conor Dooley, Pengutronix Kernel Team, Rob Herring, Shawn Guo, Sascha Hauer, Paul Elder, devicetree@vger.kernel.org, Laurent Pinchart, Krzysztof Kozlowski, Fabio Estevam, linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org On 24-06-13, Marek Vasut wrote: > On 6/11/24 5:01 AM, Peng Fan wrote: > > [...] > > > > > > According to the i.MX8MP Data sheet, the nominal speed for > > > > > MEDIA_ISP_CLOCL_ROOT is 400MHZ with 500MHz being allowed in > > > overdrive > > > > > mode. > > > > > > > > > > I think this clock rate should drop to the nominal value of 400MHz > > > > > and those boards who support overdrive can increase it to 500MHz to > > > > > avoid stiability issues and/or running out of spec. I created an > > > > > imx8mm and imx8mn- overdrive.dtsi file. If there is interest, I can do the > > > same for the 8MP as well. > > > > > > > > > > I haven't gone through all the clocks to determine if/what clocks are > > > > > being overdriven. > > > > > > > > Shouldn't the bootloader take the work to runtime update the freq? > > > > Why need introduce an extra overdrive.dtsi? > > > > > > Shouldn't the overdrive/non-overdrive decision be done in board DT instead ? > > > > It is bootloader configure voltage to nominal, then bootloader should use > > nominal device tree or runtime update dtb. > > If bootloader configure voltage to over-drive, bootloader could use > > nominal or over-drive dtb > > I think the bootloader should always configure the minimal common > configuration, i.e. nominal voltage, nominal clock, so that it would achieve > maximum compatibility with any SoC in that SoC line up. > > If the user does need overdrive configuration, they should specify that in > their board DT. +1 > Keep in mind, the kernel is easy to update (including kernel DT), the > bootloader is not easy to update (esp. in field, bootloader update may > render a system unbootable if it fails). I would say, it is better to keep > the complicated things out of the bootloader if at all possible. > > > If introduce x.dtsi and x-overdrive.dtsi, how to let board choose which dtsi > > to include? > > #include "x.dtsi" > or > #include "x-overdrive.dtsi" > > But I think your question is -- how to do that at runtime ? > > U-Boot can apply DT overlays onto DT that is passed to Linux, so if the user > has board variants where they need both nonoverdrive/overdrive options, they > can apply DT overlay which enables the overdrive mode on boards which need > it. This can be done from U-Boot boot.scr or similar boot script, which can > again be easily updated, without the need to update the bootloader itself > (if something goes wrong or needs to be changed in the future). +1 for the runtime configuration via overlay as well. The overlay can be generated very easily by including the x-overdrive.dtsi into a .dtso and for those who want to run in overdrive mode always they can mae use of the x-overdrive.dtsi directly within the .dts file. Regards, Marco ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] arm64: dts: imx8mp: Add DT nodes for the two ISPs 2024-06-09 20:21 ` Adam Ford 2024-06-11 1:04 ` Peng Fan @ 2024-08-13 22:22 ` Laurent Pinchart 2024-08-14 6:29 ` Adam Ford 1 sibling, 1 reply; 14+ messages in thread From: Laurent Pinchart @ 2024-08-13 22:22 UTC (permalink / raw) To: Adam Ford Cc: Alexander Stein, imx, linux-arm-kernel, Paul Elder, Conor Dooley, Fabio Estevam, Krzysztof Kozlowski, Marek Vasut, Pengutronix Kernel Team, Rob Herring, Sascha Hauer, Shawn Guo, devicetree, linux-media On Sun, Jun 09, 2024 at 03:21:19PM -0500, Adam Ford wrote: > On Tue, Mar 26, 2024 at 2:14 AM Alexander Stein wrote: > > Am Montag, 25. März 2024, 21:49:24 CET schrieb Laurent Pinchart: > > > On Mon, Mar 25, 2024 at 04:52:21PM +0100, Alexander Stein wrote: > > > > Am Montag, 25. März 2024, 16:13:39 CET schrieb Laurent Pinchart: > > > > > From: Paul Elder <paul.elder@ideasonboard.com> > > > > > > > > > > The ISP supports both CSI and parallel interfaces, where port 0 > > > > > corresponds to the former and port 1 corresponds to the latter. Since > > > > > the i.MX8MP's ISPs are connected by the parallel interface to the CSI > > > > > receiver, set them both to port 1. > > > > > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > --- > > > > > Changes since v1: > > > > > > > > > > - Fix clock ordering > > > > > - Add #address-cells and #size-cells to ports nodes > > > > > --- > > > > > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 50 +++++++++++++++++++++++ > > > > > 1 file changed, 50 insertions(+) > > > > > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > > > index bfc5c81a5bd4..1d2670b91b53 100644 > > > > > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > > > @@ -1616,6 +1616,56 @@ isi_in_1: endpoint { > > > > > }; > > > > > }; > > > > > > > > > > + isp_0: isp@32e10000 { > > > > > + compatible = "fsl,imx8mp-isp"; > > > > > + reg = <0x32e10000 0x10000>; > > > > > + interrupts = <GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>; > > > > > + clocks = <&clk IMX8MP_CLK_MEDIA_ISP_ROOT>, > > > > > + <&clk IMX8MP_CLK_MEDIA_AXI_ROOT>, > > > > > + <&clk IMX8MP_CLK_MEDIA_APB_ROOT>; > > > > > + clock-names = "isp", "aclk", "hclk"; > > > > > + assigned-clocks = <&clk IMX8MP_CLK_MEDIA_ISP>; > > > > > + assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; > > > > > + assigned-clock-rates = <500000000>; > > > > > + power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_ISP>; > > > > > + fsl,blk-ctrl = <&media_blk_ctrl 0>; > > > > > + status = "disabled"; > > > > > + > > > > > + ports { > > > > > + #address-cells = <1>; > > > > > + #size-cells = <0>; > > > > > + > > > > > + port@1 { > > > > > + reg = <1>; > > > > > + }; > > > > > + }; > > > > > + }; > > > > > + > > > > > + isp_1: isp@32e20000 { > > > > > + compatible = "fsl,imx8mp-isp"; > > > > > + reg = <0x32e20000 0x10000>; > > > > > + interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>; > > > > > + clocks = <&clk IMX8MP_CLK_MEDIA_ISP_ROOT>, > > > > > + <&clk IMX8MP_CLK_MEDIA_AXI_ROOT>, > > > > > + <&clk IMX8MP_CLK_MEDIA_APB_ROOT>; > > > > > + clock-names = "isp", "aclk", "hclk"; > > > > > + assigned-clocks = <&clk IMX8MP_CLK_MEDIA_ISP>; > > > > > + assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; > > > > > + assigned-clock-rates = <500000000>; > > > > > + power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_ISP>; > > > > > + fsl,blk-ctrl = <&media_blk_ctrl 1>; > > > > > + status = "disabled"; > > > > > + > > > > > + ports { > > > > > + #address-cells = <1>; > > > > > + #size-cells = <0>; > > > > > + > > > > > + port@1 { > > > > > + reg = <1>; > > > > > + }; > > > > > + }; > > > > > + }; > > > > > + > > > > > > > > The patch itself is okay. But you might not be able to > > > > configure the parent of IMX8MP_CLK_MEDIA_ISP if dewarp is enabled before. > > > > This is due to IMX8MP_CLK_MEDIA_ISP_ROOT being enabled in 'pgc_ispdwp' > > > > power domain. Reparenting is not possible anymore in this case. > > > > > > Good point. > > > > > > > Something like > > > > ---8<--- > > > > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > > @@ -1837,11 +1837,13 @@ media_blk_ctrl: blk-ctrl@32ec0000 { > > > > <&clk IMX8MP_CLK_MEDIA_APB>, > > > > <&clk IMX8MP_CLK_MEDIA_DISP1_PIX>, > > > > <&clk IMX8MP_CLK_MEDIA_DISP2_PIX>, > > > > + <&clk IMX8MP_CLK_MEDIA_ISP>, > > > > <&clk IMX8MP_VIDEO_PLL1>; > > > > assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>, > > > > <&clk IMX8MP_SYS_PLL1_800M>, > > > > <&clk IMX8MP_VIDEO_PLL1_OUT>, > > > > - <&clk IMX8MP_VIDEO_PLL1_OUT>; > > > > + <&clk IMX8MP_VIDEO_PLL1_OUT>, > > > > + <&clk IMX8MP_SYS_PLL2_500M>; > > > > assigned-clock-rates = <500000000>, <200000000>, > > > > <0>, <0>, <1039500000>; > > > > > According to the i.MX8MP Data sheet, the nominal speed for > MEDIA_ISP_CLOCL_ROOT is 400MHZ with 500MHz being allowed in overdrive > mode. > > I think this clock rate should drop to the nominal value of 400MHz > and those boards who support overdrive can increase it to 500MHz to > avoid stiability issues and/or running out of spec. I created an > imx8mm and imx8mn-overdrive.dtsi file. If there is interest, I can do > the same for the 8MP as well. I've experimented with this, and lowered the ISP clock to 400MHz by selecting IMX8MP_SYS_PLL1_400M as the parent. I couldn't capture images anymore :-S The camera sensor I'm using outputs a 74.25 Mpix/s pixel rate, which is way below the maximum in either normal or overdrive mode. The reference manual states, regarding the CSI-2 receivers, - When one ISP is used, MIPI CSI interface 1 supports: - Pixel clock up to 400MHz at nominal voltage and 500MHz at overdrive voltage - When two ISPs are used, both MIPI CSI interfaces supports: - Pixel clock up to 266MHz at nominal and overdrive voltage The datasheets for both the CEC and IEC variants give slightly different information: - For single Camera, MIPI CSI 1 can support up to 400/500 MHz pixel clock in the Nominal/Overdrive mode. - For single Camera, MIPI CSI 2 can support up to 277 MHz pixel clock. - For dual Camera, both MIPI CSI can support up to 266 MHz pixel clock. The also list the maximum clock frequencies as - MEDIA_ISP_CLK_ROOT: nominal 400 MHz, overdrive 500 MHz - MEDIA_CAM1_PIX_CLK_ROOT: nominal 400 MHz, overdrive 500 MHz - MEDIA_CAM2_PIX_CLK_ROOT: nominal 277 MHz, overdrive 277 MHz (On a side note, the revision history indicates the revision 2.1 of the datasheet changed MEDIA_CAM2_PIX_CLK_ROOT from 266/266 MHz to 277/277 MHz, but the 277 MHz frequency has been listed since revision 0.) The MIPI CSI-2 RX (MIPI_CSI) receives data for the camera sensor at the CSI-2 link frequency rate. It reclocks the data stream using an internal FIFO, with an output pixel rate selected by the IMX8MP_CLK_MEDIA_CAM1_PIX_ROOT (for the first CSI-2 receiver) or IMX8MP_CLK_MEDIA_CAM2_PIX_ROOT (for the second CSI-2 receiver). Those clocks support the following parents, with the following frequencies on my system: - 24M_REF_CLK 24 MHz - SYSTEM_PLL1_CLK 800 MHz - SYSTEM_PLL1_DIV3 266 MHz - SYSTEM_PLL2_CLK 1000 MHz - SYSTEM_PLL2_DIV4 250 MHz - SYSTEM_PLL3_CLK 600 MHz - AUDIO_PLL2_CLK 361.2672 MHz - VIDEO_PLL_CLK 400 MHz This makes it easy to configure the CAM PIX ROOT clock to 266 MHz or 400 MHz easily. Achieving 277 MHz or 500 MHz would I assume require modifying some of the PLL configurations. I'm not sure how the clock tree was designed to be configured for overdrive mode. imx8mp.dtsi configures the CAM1_PIX and CAM2_PIX clocks with assigned-clocks = <&clk IMX8MP_CLK_MEDIA_CAM1_PIX>; assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>; assigned-clock-rates = <266000000>; and assigned-clocks = <&clk IMX8MP_CLK_MEDIA_CAM1_PIX>; assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>; assigned-clock-rates = <266000000>; This is misleading as it selects the SYSTEM_PLL2_DIV4 parent, with a clock frequency of 250 MHz. I'll send a patch to set the assigned clock rate to 250 MHz to avoid misunderstandings, we can in parallel decide if the parent should be set to SYSTEM_PLL1_DIV3 instead. As the CSI-2 transmits, as far as I can tell, one pixel per clock cycle to the ISP for raw sensors, this is well within the hardware limits. The clock domains of the ISP are briefly described in the reference manual as follows: - Core Clock (clk) - Core clock is the main clock domain used by the ISP modules and is the biggest clock domain in the ISP. - Sensor Clock (sclk) - The sensor clock domain is selected from the sensor interface and is the data input clock. - AHB Clock (hclk) - The AHB Clock is the AHB Interface clock. - AXI Clock (m_hclk) - The AXI Clock is the AXI Interface clock. Additionally, the reference manual states that The communication between the different domains in ISP occurs mostly by way of asynchronous FIFOs. All clocks are asynchronous to each other. There is no communication between AHB and AXI clock domains. The sensor clock (sclk) is the clock received by the ISP, which is the MIPI_CSI output clock (MEDIA_CAM[12]_PIX_CLK_ROOT). This clock isn't listed in the ISP DT node, as it isn't controlled directly by the ISP. It isn't clear where the transition to the core clock (clk) happens, I would assume quite early in the ISP pipeline. That clock is the MEDIA_ISP_CLOCK_ROOT, which that patch configures to 500 MHz. My assumption is that the core clock frequency needs to be at least as high as the sensor clock, but as configuring the ISP clock to 400 MHz breaks image capture, there's something I'm missing. As the 500 MHz ISP clock frequency is exactly twice the sensor clock frequency in my setup, I wondered if there could be a x2 relationship between those two clocks. I have tried to increase the CAM1_PIX clock to 266 MHz, and the ISP then operates correctly with a 500 MHz ISP clock. There must thus be something else. I'm not sure how to move forward. There's no further information I could find in the datasheet or reference manual that would hint how to operate with a 400 MHz ISP clock frequency. We may require help from someone at NXP. In the meantime, could we merge this patch with a 500 MHz ISP clock frequency (setup in the blk-ctrl@32ec0000 node instead of the isp nodes) ? > I haven't gone through all the clocks to determine if/what clocks are > being overdriven. > > > > With an assigned clock rate here too then ? > > > > You are right. This posted diff is what I was using for a while now though. > > Apparently the clock frequency was still correct. > > > > > > #power-domain-cells = <1>; > > > > ---8<--- > > > > is needed. > > > > > > Sascha, are you OK with this approach ? > > This patch appears to have gone stale. Is there any way we can push > this forward? > > > > > > dewarp: dwe@32e30000 { > > > > > compatible = "nxp,imx8mp-dw100"; > > > > > reg = <0x32e30000 0x10000>; > > > > > > > > > > base-commit: 4cece764965020c22cff7665b18a012006359095 -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] arm64: dts: imx8mp: Add DT nodes for the two ISPs 2024-08-13 22:22 ` Laurent Pinchart @ 2024-08-14 6:29 ` Adam Ford 2024-08-14 14:47 ` Laurent Pinchart 0 siblings, 1 reply; 14+ messages in thread From: Adam Ford @ 2024-08-14 6:29 UTC (permalink / raw) To: Laurent Pinchart Cc: Alexander Stein, imx, linux-arm-kernel, Paul Elder, Conor Dooley, Fabio Estevam, Krzysztof Kozlowski, Marek Vasut, Pengutronix Kernel Team, Rob Herring, Sascha Hauer, Shawn Guo, devicetree, linux-media On Tue, Aug 13, 2024 at 5:22 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Sun, Jun 09, 2024 at 03:21:19PM -0500, Adam Ford wrote: > > On Tue, Mar 26, 2024 at 2:14 AM Alexander Stein wrote: > > > Am Montag, 25. März 2024, 21:49:24 CET schrieb Laurent Pinchart: > > > > On Mon, Mar 25, 2024 at 04:52:21PM +0100, Alexander Stein wrote: > > > > > Am Montag, 25. März 2024, 16:13:39 CET schrieb Laurent Pinchart: > > > > > > From: Paul Elder <paul.elder@ideasonboard.com> > > > > > > > > > > > > The ISP supports both CSI and parallel interfaces, where port 0 > > > > > > corresponds to the former and port 1 corresponds to the latter. Since > > > > > > the i.MX8MP's ISPs are connected by the parallel interface to the CSI > > > > > > receiver, set them both to port 1. > > > > > > > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > --- > > > > > > Changes since v1: > > > > > > > > > > > > - Fix clock ordering > > > > > > - Add #address-cells and #size-cells to ports nodes > > > > > > --- > > > > > > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 50 +++++++++++++++++++++++ > > > > > > 1 file changed, 50 insertions(+) > > > > > > > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > > > > index bfc5c81a5bd4..1d2670b91b53 100644 > > > > > > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > > > > @@ -1616,6 +1616,56 @@ isi_in_1: endpoint { > > > > > > }; > > > > > > }; > > > > > > > > > > > > + isp_0: isp@32e10000 { > > > > > > + compatible = "fsl,imx8mp-isp"; > > > > > > + reg = <0x32e10000 0x10000>; > > > > > > + interrupts = <GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>; > > > > > > + clocks = <&clk IMX8MP_CLK_MEDIA_ISP_ROOT>, > > > > > > + <&clk IMX8MP_CLK_MEDIA_AXI_ROOT>, > > > > > > + <&clk IMX8MP_CLK_MEDIA_APB_ROOT>; > > > > > > + clock-names = "isp", "aclk", "hclk"; > > > > > > + assigned-clocks = <&clk IMX8MP_CLK_MEDIA_ISP>; > > > > > > + assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; > > > > > > + assigned-clock-rates = <500000000>; > > > > > > + power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_ISP>; > > > > > > + fsl,blk-ctrl = <&media_blk_ctrl 0>; > > > > > > + status = "disabled"; > > > > > > + > > > > > > + ports { > > > > > > + #address-cells = <1>; > > > > > > + #size-cells = <0>; > > > > > > + > > > > > > + port@1 { > > > > > > + reg = <1>; > > > > > > + }; > > > > > > + }; > > > > > > + }; > > > > > > + > > > > > > + isp_1: isp@32e20000 { > > > > > > + compatible = "fsl,imx8mp-isp"; > > > > > > + reg = <0x32e20000 0x10000>; > > > > > > + interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>; > > > > > > + clocks = <&clk IMX8MP_CLK_MEDIA_ISP_ROOT>, > > > > > > + <&clk IMX8MP_CLK_MEDIA_AXI_ROOT>, > > > > > > + <&clk IMX8MP_CLK_MEDIA_APB_ROOT>; > > > > > > + clock-names = "isp", "aclk", "hclk"; > > > > > > + assigned-clocks = <&clk IMX8MP_CLK_MEDIA_ISP>; > > > > > > + assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; > > > > > > + assigned-clock-rates = <500000000>; > > > > > > + power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_ISP>; > > > > > > + fsl,blk-ctrl = <&media_blk_ctrl 1>; > > > > > > + status = "disabled"; > > > > > > + > > > > > > + ports { > > > > > > + #address-cells = <1>; > > > > > > + #size-cells = <0>; > > > > > > + > > > > > > + port@1 { > > > > > > + reg = <1>; > > > > > > + }; > > > > > > + }; > > > > > > + }; > > > > > > + > > > > > > > > > > The patch itself is okay. But you might not be able to > > > > > configure the parent of IMX8MP_CLK_MEDIA_ISP if dewarp is enabled before. > > > > > This is due to IMX8MP_CLK_MEDIA_ISP_ROOT being enabled in 'pgc_ispdwp' > > > > > power domain. Reparenting is not possible anymore in this case. > > > > > > > > Good point. > > > > > > > > > Something like > > > > > ---8<--- > > > > > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > > > @@ -1837,11 +1837,13 @@ media_blk_ctrl: blk-ctrl@32ec0000 { > > > > > <&clk IMX8MP_CLK_MEDIA_APB>, > > > > > <&clk IMX8MP_CLK_MEDIA_DISP1_PIX>, > > > > > <&clk IMX8MP_CLK_MEDIA_DISP2_PIX>, > > > > > + <&clk IMX8MP_CLK_MEDIA_ISP>, > > > > > <&clk IMX8MP_VIDEO_PLL1>; > > > > > assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>, > > > > > <&clk IMX8MP_SYS_PLL1_800M>, > > > > > <&clk IMX8MP_VIDEO_PLL1_OUT>, > > > > > - <&clk IMX8MP_VIDEO_PLL1_OUT>; > > > > > + <&clk IMX8MP_VIDEO_PLL1_OUT>, > > > > > + <&clk IMX8MP_SYS_PLL2_500M>; > > > > > assigned-clock-rates = <500000000>, <200000000>, > > > > > <0>, <0>, <1039500000>; > > > > > > > > According to the i.MX8MP Data sheet, the nominal speed for > > MEDIA_ISP_CLOCL_ROOT is 400MHZ with 500MHz being allowed in overdrive > > mode. > > > > I think this clock rate should drop to the nominal value of 400MHz > > and those boards who support overdrive can increase it to 500MHz to > > avoid stiability issues and/or running out of spec. I created an > > imx8mm and imx8mn-overdrive.dtsi file. If there is interest, I can do > > the same for the 8MP as well. > > I've experimented with this, and lowered the ISP clock to 400MHz by > selecting IMX8MP_SYS_PLL1_400M as the parent. I couldn't capture images > anymore :-S > > The camera sensor I'm using outputs a 74.25 Mpix/s pixel rate, which is > way below the maximum in either normal or overdrive mode. The reference > manual states, regarding the CSI-2 receivers, > > - When one ISP is used, MIPI CSI interface 1 supports: > - Pixel clock up to 400MHz at nominal voltage and 500MHz at overdrive > voltage > - When two ISPs are used, both MIPI CSI interfaces supports: > - Pixel clock up to 266MHz at nominal and overdrive voltage > > The datasheets for both the CEC and IEC variants give slightly > different information: > > - For single Camera, MIPI CSI 1 can support up to 400/500 MHz pixel > clock in the Nominal/Overdrive mode. > - For single Camera, MIPI CSI 2 can support up to 277 MHz pixel clock. > - For dual Camera, both MIPI CSI can support up to 266 MHz pixel clock. > > The also list the maximum clock frequencies as > > - MEDIA_ISP_CLK_ROOT: nominal 400 MHz, overdrive 500 MHz > - MEDIA_CAM1_PIX_CLK_ROOT: nominal 400 MHz, overdrive 500 MHz > - MEDIA_CAM2_PIX_CLK_ROOT: nominal 277 MHz, overdrive 277 MHz > > (On a side note, the revision history indicates the revision 2.1 of the > datasheet changed MEDIA_CAM2_PIX_CLK_ROOT from 266/266 MHz to 277/277 > MHz, but the 277 MHz frequency has been listed since revision 0.) > > The MIPI CSI-2 RX (MIPI_CSI) receives data for the camera sensor at the > CSI-2 link frequency rate. It reclocks the data stream using an internal > FIFO, with an output pixel rate selected by the > IMX8MP_CLK_MEDIA_CAM1_PIX_ROOT (for the first CSI-2 receiver) or > IMX8MP_CLK_MEDIA_CAM2_PIX_ROOT (for the second CSI-2 receiver). Those > clocks support the following parents, with the following frequencies on > my system: > > - 24M_REF_CLK 24 MHz > - SYSTEM_PLL1_CLK 800 MHz > - SYSTEM_PLL1_DIV3 266 MHz > - SYSTEM_PLL2_CLK 1000 MHz > - SYSTEM_PLL2_DIV4 250 MHz > - SYSTEM_PLL3_CLK 600 MHz > - AUDIO_PLL2_CLK 361.2672 MHz > - VIDEO_PLL_CLK 400 MHz > > This makes it easy to configure the CAM PIX ROOT clock to 266 MHz or 400 > MHz easily. Achieving 277 MHz or 500 MHz would I assume require > modifying some of the PLL configurations. I'm not sure how the clock > tree was designed to be configured for overdrive mode. > > imx8mp.dtsi configures the CAM1_PIX and CAM2_PIX clocks with > > assigned-clocks = <&clk IMX8MP_CLK_MEDIA_CAM1_PIX>; > assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>; > assigned-clock-rates = <266000000>; > > and > > assigned-clocks = <&clk IMX8MP_CLK_MEDIA_CAM1_PIX>; > assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>; > assigned-clock-rates = <266000000>; > > This is misleading as it selects the SYSTEM_PLL2_DIV4 parent, with a > clock frequency of 250 MHz. I'll send a patch to set the assigned clock > rate to 250 MHz to avoid misunderstandings, we can in parallel decide if > the parent should be set to SYSTEM_PLL1_DIV3 instead. > > As the CSI-2 transmits, as far as I can tell, one pixel per clock cycle > to the ISP for raw sensors, this is well within the hardware limits. > > The clock domains of the ISP are briefly described in the reference > manual as follows: > > - Core Clock (clk) - Core clock is the main clock domain used by the ISP > modules and is the biggest clock domain in the ISP. > - Sensor Clock (sclk) - The sensor clock domain is selected from the > sensor interface and is the data input clock. > - AHB Clock (hclk) - The AHB Clock is the AHB Interface clock. > - AXI Clock (m_hclk) - The AXI Clock is the AXI Interface clock. > > Additionally, the reference manual states that > > The communication between the different domains in ISP occurs mostly > by way of asynchronous FIFOs. All clocks are asynchronous to each > other. There is no communication between AHB and AXI clock domains. > > The sensor clock (sclk) is the clock received by the ISP, which is the > MIPI_CSI output clock (MEDIA_CAM[12]_PIX_CLK_ROOT). This clock isn't > listed in the ISP DT node, as it isn't controlled directly by the ISP. > It isn't clear where the transition to the core clock (clk) happens, I > would assume quite early in the ISP pipeline. That clock is the > MEDIA_ISP_CLOCK_ROOT, which that patch configures to 500 MHz. My > assumption is that the core clock frequency needs to be at least as high > as the sensor clock, but as configuring the ISP clock to 400 MHz breaks > image capture, there's something I'm missing. > > As the 500 MHz ISP clock frequency is exactly twice the sensor clock > frequency in my setup, I wondered if there could be a x2 relationship > between those two clocks. I have tried to increase the CAM1_PIX clock to > 266 MHz, and the ISP then operates correctly with a 500 MHz ISP clock. > There must thus be something else. > > I'm not sure how to move forward. There's no further information I could > find in the datasheet or reference manual that would hint how to operate > with a 400 MHz ISP clock frequency. We may require help from someone at > NXP. In the meantime, could we merge this patch with a 500 MHz ISP > clock frequency (setup in the blk-ctrl@32ec0000 node instead of the isp > nodes) ? Thank you for your detailed analysis. If the ISP doesn't work at 400MHz, it doesn't make sense to me to clock it at 400MHz. Maybe NXP can clarify this. If 500MHz is the only operating point, I have no objection, but I think having a comment in the device tree node where the clock is set to 500MHz as what it is this way might be in order so someone in the future doesn't go in a arbitrarily set it to 400MHz based on a document without doing some testing. adam > > > I haven't gone through all the clocks to determine if/what clocks are > > being overdriven. > > > > > > With an assigned clock rate here too then ? > > > > > > You are right. This posted diff is what I was using for a while now though. > > > Apparently the clock frequency was still correct. > > > > > > > > #power-domain-cells = <1>; > > > > > ---8<--- > > > > > is needed. > > > > > > > > Sascha, are you OK with this approach ? > > > > This patch appears to have gone stale. Is there any way we can push > > this forward? > > > > > > > > dewarp: dwe@32e30000 { > > > > > > compatible = "nxp,imx8mp-dw100"; > > > > > > reg = <0x32e30000 0x10000>; > > > > > > > > > > > > base-commit: 4cece764965020c22cff7665b18a012006359095 > > -- > Regards, > > Laurent Pinchart ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] arm64: dts: imx8mp: Add DT nodes for the two ISPs 2024-08-14 6:29 ` Adam Ford @ 2024-08-14 14:47 ` Laurent Pinchart 0 siblings, 0 replies; 14+ messages in thread From: Laurent Pinchart @ 2024-08-14 14:47 UTC (permalink / raw) To: Adam Ford Cc: Alexander Stein, imx, linux-arm-kernel, Paul Elder, Conor Dooley, Fabio Estevam, Krzysztof Kozlowski, Marek Vasut, Pengutronix Kernel Team, Rob Herring, Sascha Hauer, Shawn Guo, devicetree, linux-media, Peng Fan Hi Adam, (CC'ing Peng Fan) On Wed, Aug 14, 2024 at 01:29:16AM -0500, Adam Ford wrote: > On Tue, Aug 13, 2024 at 5:22 PM Laurent Pinchart wrote: > > On Sun, Jun 09, 2024 at 03:21:19PM -0500, Adam Ford wrote: > > > On Tue, Mar 26, 2024 at 2:14 AM Alexander Stein wrote: > > > > Am Montag, 25. März 2024, 21:49:24 CET schrieb Laurent Pinchart: > > > > > On Mon, Mar 25, 2024 at 04:52:21PM +0100, Alexander Stein wrote: > > > > > > Am Montag, 25. März 2024, 16:13:39 CET schrieb Laurent Pinchart: > > > > > > > From: Paul Elder <paul.elder@ideasonboard.com> > > > > > > > > > > > > > > The ISP supports both CSI and parallel interfaces, where port 0 > > > > > > > corresponds to the former and port 1 corresponds to the latter. Since > > > > > > > the i.MX8MP's ISPs are connected by the parallel interface to the CSI > > > > > > > receiver, set them both to port 1. > > > > > > > > > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > --- > > > > > > > Changes since v1: > > > > > > > > > > > > > > - Fix clock ordering > > > > > > > - Add #address-cells and #size-cells to ports nodes > > > > > > > --- > > > > > > > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 50 +++++++++++++++++++++++ > > > > > > > 1 file changed, 50 insertions(+) > > > > > > > > > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > > > > > index bfc5c81a5bd4..1d2670b91b53 100644 > > > > > > > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > > > > > @@ -1616,6 +1616,56 @@ isi_in_1: endpoint { > > > > > > > }; > > > > > > > }; > > > > > > > > > > > > > > + isp_0: isp@32e10000 { > > > > > > > + compatible = "fsl,imx8mp-isp"; > > > > > > > + reg = <0x32e10000 0x10000>; > > > > > > > + interrupts = <GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>; > > > > > > > + clocks = <&clk IMX8MP_CLK_MEDIA_ISP_ROOT>, > > > > > > > + <&clk IMX8MP_CLK_MEDIA_AXI_ROOT>, > > > > > > > + <&clk IMX8MP_CLK_MEDIA_APB_ROOT>; > > > > > > > + clock-names = "isp", "aclk", "hclk"; > > > > > > > + assigned-clocks = <&clk IMX8MP_CLK_MEDIA_ISP>; > > > > > > > + assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; > > > > > > > + assigned-clock-rates = <500000000>; > > > > > > > + power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_ISP>; > > > > > > > + fsl,blk-ctrl = <&media_blk_ctrl 0>; > > > > > > > + status = "disabled"; > > > > > > > + > > > > > > > + ports { > > > > > > > + #address-cells = <1>; > > > > > > > + #size-cells = <0>; > > > > > > > + > > > > > > > + port@1 { > > > > > > > + reg = <1>; > > > > > > > + }; > > > > > > > + }; > > > > > > > + }; > > > > > > > + > > > > > > > + isp_1: isp@32e20000 { > > > > > > > + compatible = "fsl,imx8mp-isp"; > > > > > > > + reg = <0x32e20000 0x10000>; > > > > > > > + interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>; > > > > > > > + clocks = <&clk IMX8MP_CLK_MEDIA_ISP_ROOT>, > > > > > > > + <&clk IMX8MP_CLK_MEDIA_AXI_ROOT>, > > > > > > > + <&clk IMX8MP_CLK_MEDIA_APB_ROOT>; > > > > > > > + clock-names = "isp", "aclk", "hclk"; > > > > > > > + assigned-clocks = <&clk IMX8MP_CLK_MEDIA_ISP>; > > > > > > > + assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; > > > > > > > + assigned-clock-rates = <500000000>; > > > > > > > + power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_ISP>; > > > > > > > + fsl,blk-ctrl = <&media_blk_ctrl 1>; > > > > > > > + status = "disabled"; > > > > > > > + > > > > > > > + ports { > > > > > > > + #address-cells = <1>; > > > > > > > + #size-cells = <0>; > > > > > > > + > > > > > > > + port@1 { > > > > > > > + reg = <1>; > > > > > > > + }; > > > > > > > + }; > > > > > > > + }; > > > > > > > + > > > > > > > > > > > > The patch itself is okay. But you might not be able to > > > > > > configure the parent of IMX8MP_CLK_MEDIA_ISP if dewarp is enabled before. > > > > > > This is due to IMX8MP_CLK_MEDIA_ISP_ROOT being enabled in 'pgc_ispdwp' > > > > > > power domain. Reparenting is not possible anymore in this case. > > > > > > > > > > Good point. > > > > > > > > > > > Something like > > > > > > ---8<--- > > > > > > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > > > > @@ -1837,11 +1837,13 @@ media_blk_ctrl: blk-ctrl@32ec0000 { > > > > > > <&clk IMX8MP_CLK_MEDIA_APB>, > > > > > > <&clk IMX8MP_CLK_MEDIA_DISP1_PIX>, > > > > > > <&clk IMX8MP_CLK_MEDIA_DISP2_PIX>, > > > > > > + <&clk IMX8MP_CLK_MEDIA_ISP>, > > > > > > <&clk IMX8MP_VIDEO_PLL1>; > > > > > > assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>, > > > > > > <&clk IMX8MP_SYS_PLL1_800M>, > > > > > > <&clk IMX8MP_VIDEO_PLL1_OUT>, > > > > > > - <&clk IMX8MP_VIDEO_PLL1_OUT>; > > > > > > + <&clk IMX8MP_VIDEO_PLL1_OUT>, > > > > > > + <&clk IMX8MP_SYS_PLL2_500M>; > > > > > > assigned-clock-rates = <500000000>, <200000000>, > > > > > > <0>, <0>, <1039500000>; > > > > > > > > > > > According to the i.MX8MP Data sheet, the nominal speed for > > > MEDIA_ISP_CLOCL_ROOT is 400MHZ with 500MHz being allowed in overdrive > > > mode. > > > > > > I think this clock rate should drop to the nominal value of 400MHz > > > and those boards who support overdrive can increase it to 500MHz to > > > avoid stiability issues and/or running out of spec. I created an > > > imx8mm and imx8mn-overdrive.dtsi file. If there is interest, I can do > > > the same for the 8MP as well. > > > > I've experimented with this, and lowered the ISP clock to 400MHz by > > selecting IMX8MP_SYS_PLL1_400M as the parent. I couldn't capture images > > anymore :-S > > > > The camera sensor I'm using outputs a 74.25 Mpix/s pixel rate, which is > > way below the maximum in either normal or overdrive mode. The reference > > manual states, regarding the CSI-2 receivers, > > > > - When one ISP is used, MIPI CSI interface 1 supports: > > - Pixel clock up to 400MHz at nominal voltage and 500MHz at overdrive > > voltage > > - When two ISPs are used, both MIPI CSI interfaces supports: > > - Pixel clock up to 266MHz at nominal and overdrive voltage > > > > The datasheets for both the CEC and IEC variants give slightly > > different information: > > > > - For single Camera, MIPI CSI 1 can support up to 400/500 MHz pixel > > clock in the Nominal/Overdrive mode. > > - For single Camera, MIPI CSI 2 can support up to 277 MHz pixel clock. > > - For dual Camera, both MIPI CSI can support up to 266 MHz pixel clock. > > > > The also list the maximum clock frequencies as > > > > - MEDIA_ISP_CLK_ROOT: nominal 400 MHz, overdrive 500 MHz > > - MEDIA_CAM1_PIX_CLK_ROOT: nominal 400 MHz, overdrive 500 MHz > > - MEDIA_CAM2_PIX_CLK_ROOT: nominal 277 MHz, overdrive 277 MHz > > > > (On a side note, the revision history indicates the revision 2.1 of the > > datasheet changed MEDIA_CAM2_PIX_CLK_ROOT from 266/266 MHz to 277/277 > > MHz, but the 277 MHz frequency has been listed since revision 0.) > > > > The MIPI CSI-2 RX (MIPI_CSI) receives data for the camera sensor at the > > CSI-2 link frequency rate. It reclocks the data stream using an internal > > FIFO, with an output pixel rate selected by the > > IMX8MP_CLK_MEDIA_CAM1_PIX_ROOT (for the first CSI-2 receiver) or > > IMX8MP_CLK_MEDIA_CAM2_PIX_ROOT (for the second CSI-2 receiver). Those > > clocks support the following parents, with the following frequencies on > > my system: > > > > - 24M_REF_CLK 24 MHz > > - SYSTEM_PLL1_CLK 800 MHz > > - SYSTEM_PLL1_DIV3 266 MHz > > - SYSTEM_PLL2_CLK 1000 MHz > > - SYSTEM_PLL2_DIV4 250 MHz > > - SYSTEM_PLL3_CLK 600 MHz > > - AUDIO_PLL2_CLK 361.2672 MHz > > - VIDEO_PLL_CLK 400 MHz > > > > This makes it easy to configure the CAM PIX ROOT clock to 266 MHz or 400 > > MHz easily. Achieving 277 MHz or 500 MHz would I assume require > > modifying some of the PLL configurations. I'm not sure how the clock > > tree was designed to be configured for overdrive mode. > > > > imx8mp.dtsi configures the CAM1_PIX and CAM2_PIX clocks with > > > > assigned-clocks = <&clk IMX8MP_CLK_MEDIA_CAM1_PIX>; > > assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>; > > assigned-clock-rates = <266000000>; > > > > and > > > > assigned-clocks = <&clk IMX8MP_CLK_MEDIA_CAM1_PIX>; > > assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>; > > assigned-clock-rates = <266000000>; > > > > This is misleading as it selects the SYSTEM_PLL2_DIV4 parent, with a > > clock frequency of 250 MHz. I'll send a patch to set the assigned clock > > rate to 250 MHz to avoid misunderstandings, we can in parallel decide if > > the parent should be set to SYSTEM_PLL1_DIV3 instead. > > > > As the CSI-2 transmits, as far as I can tell, one pixel per clock cycle > > to the ISP for raw sensors, this is well within the hardware limits. > > > > The clock domains of the ISP are briefly described in the reference > > manual as follows: > > > > - Core Clock (clk) - Core clock is the main clock domain used by the ISP > > modules and is the biggest clock domain in the ISP. > > - Sensor Clock (sclk) - The sensor clock domain is selected from the > > sensor interface and is the data input clock. > > - AHB Clock (hclk) - The AHB Clock is the AHB Interface clock. > > - AXI Clock (m_hclk) - The AXI Clock is the AXI Interface clock. > > > > Additionally, the reference manual states that > > > > The communication between the different domains in ISP occurs mostly > > by way of asynchronous FIFOs. All clocks are asynchronous to each > > other. There is no communication between AHB and AXI clock domains. > > > > The sensor clock (sclk) is the clock received by the ISP, which is the > > MIPI_CSI output clock (MEDIA_CAM[12]_PIX_CLK_ROOT). This clock isn't > > listed in the ISP DT node, as it isn't controlled directly by the ISP. > > It isn't clear where the transition to the core clock (clk) happens, I > > would assume quite early in the ISP pipeline. That clock is the > > MEDIA_ISP_CLOCK_ROOT, which that patch configures to 500 MHz. My > > assumption is that the core clock frequency needs to be at least as high > > as the sensor clock, but as configuring the ISP clock to 400 MHz breaks > > image capture, there's something I'm missing. > > > > As the 500 MHz ISP clock frequency is exactly twice the sensor clock > > frequency in my setup, I wondered if there could be a x2 relationship > > between those two clocks. I have tried to increase the CAM1_PIX clock to > > 266 MHz, and the ISP then operates correctly with a 500 MHz ISP clock. > > There must thus be something else. > > > > I'm not sure how to move forward. There's no further information I could > > find in the datasheet or reference manual that would hint how to operate > > with a 400 MHz ISP clock frequency. We may require help from someone at > > NXP. In the meantime, could we merge this patch with a 500 MHz ISP > > clock frequency (setup in the blk-ctrl@32ec0000 node instead of the isp > > nodes) ? > > Thank you for your detailed analysis. If the ISP doesn't work at > 400MHz, it doesn't make sense to me to clock it at 400MHz. Maybe NXP > can clarify this. If 500MHz is the only operating point, I have no > objection, but I think having a comment in the device tree node where > the clock is set to 500MHz as what it is this way might be in order so > someone in the future doesn't go in a arbitrarily set it to 400MHz > based on a document without doing some testing. I wouldn't say that the ISP can't work at 400 MHz at all, but I think we'll need help from NXP to better understand the clocking schemes and constraints. Peng, do you think this is something you or someone else could have a look at ? I'll send a new version with a comment in the device tree. > > > I haven't gone through all the clocks to determine if/what clocks are > > > being overdriven. > > > > > > > > With an assigned clock rate here too then ? > > > > > > > > You are right. This posted diff is what I was using for a while now though. > > > > Apparently the clock frequency was still correct. > > > > > > > > > > #power-domain-cells = <1>; > > > > > > ---8<--- > > > > > > is needed. > > > > > > > > > > Sascha, are you OK with this approach ? > > > > > > This patch appears to have gone stale. Is there any way we can push > > > this forward? > > > > > > > > > > dewarp: dwe@32e30000 { > > > > > > > compatible = "nxp,imx8mp-dw100"; > > > > > > > reg = <0x32e30000 0x10000>; > > > > > > > > > > > > > > base-commit: 4cece764965020c22cff7665b18a012006359095 -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2024-08-14 16:00 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-03-25 15:13 [PATCH v2] arm64: dts: imx8mp: Add DT nodes for the two ISPs Laurent Pinchart 2024-03-25 15:16 ` Adam Ford 2024-03-25 15:52 ` Alexander Stein 2024-03-25 20:49 ` Laurent Pinchart 2024-03-26 7:14 ` Alexander Stein 2024-06-09 20:21 ` Adam Ford 2024-06-11 1:04 ` Peng Fan 2024-06-11 1:34 ` Marek Vasut 2024-06-11 3:01 ` Peng Fan 2024-06-13 1:10 ` Marek Vasut 2024-06-13 8:24 ` Marco Felsch 2024-08-13 22:22 ` Laurent Pinchart 2024-08-14 6:29 ` Adam Ford 2024-08-14 14:47 ` Laurent Pinchart
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).