diff mbox series

[v2] arm64: dts: imx8mp: Add DT nodes for the two ISPs

Message ID 20240325151339.19041-1-laurent.pinchart@ideasonboard.com
State New
Headers show
Series [v2] arm64: dts: imx8mp: Add DT nodes for the two ISPs | expand

Commit Message

Laurent Pinchart March 25, 2024, 3:13 p.m. UTC
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(+)


base-commit: 4cece764965020c22cff7665b18a012006359095

Comments

Peng Fan June 11, 2024, 1:04 a.m. UTC | #1
> 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.
Peng Fan June 11, 2024, 3:01 a.m. UTC | #2
> 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.
Marco Felsch June 13, 2024, 8:24 a.m. UTC | #3
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
Laurent Pinchart Aug. 13, 2024, 10:22 p.m. UTC | #4
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
Adam Ford Aug. 14, 2024, 6:29 a.m. UTC | #5
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
Laurent Pinchart Aug. 14, 2024, 2:47 p.m. UTC | #6
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
diff mbox series

Patch

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>;