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@xxxxxxxxxxxxxxxx>
> > > > >
> > > > > 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@xxxxxxxxxxxxxxxx>
> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > > > > ---
> > > > > 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
