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