| Message ID | 20220125171129.472775-1-aford173@gmail.com |
|---|---|
| Headers | show |
| Series | media: hantro: imx8mq/imx8mm: Let VPU decoders get controlled by vpu-blk-ctrl | expand |
Am Dienstag, dem 25.01.2022 um 11:11 -0600 schrieb Adam Ford: > The vpumix power domain has a reset assigned to it, however > when used, it causes a system hang. Testing has shown that > it does not appear to be needed anywhere. > > Fixes: d39d4bb15310 ("arm64: dts: imx8mm: add GPC node") > Signed-off-by: Adam Ford <aford173@gmail.com> Reviewed-by: Lucas Stach <l.stach@pengutronix.de> > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > index f77f90ed416f..0c7a72c51a31 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > @@ -707,7 +707,6 @@ pgc_vpumix: power-domain@6 { > clocks = <&clk IMX8MM_CLK_VPU_DEC_ROOT>; > assigned-clocks = <&clk IMX8MM_CLK_VPU_BUS>; > assigned-clock-parents = <&clk IMX8MM_SYS_PLL1_800M>; > - resets = <&src IMX8MQ_RESET_VPU_RESET>; > }; > > pgc_vpu_g1: power-domain@7 {
Am Dienstag, dem 25.01.2022 um 11:11 -0600 schrieb Adam Ford: > With the Hantro G1 and G2 now setup to run independently, update > the device tree to allow both to operate. This requires the > vpu-blk-ctrl node to be configured. Since vpu-blk-ctrl needs > certain clock enabled to handle the gating of the G1 and G2 > fuses, the clock-parents and clock-rates for the various VPU's > to be moved into the pgc_vpu because they cannot get re-parented > once enabled, and the pgc_vpu is the highest in the chain. > > Signed-off-by: Adam Ford <aford173@gmail.com> > Reported-by: kernel test robot <lkp@intel.com> > Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> Reviewed-by: Lucas Stach <l.stach@pengutronix.de> > > diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > index 2df2510d0118..549b2440f55d 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > @@ -737,7 +737,21 @@ pgc_gpu: power-domain@5 { > pgc_vpu: power-domain@6 { > #power-domain-cells = <0>; > reg = <IMX8M_POWER_DOMAIN_VPU>; > - clocks = <&clk IMX8MQ_CLK_VPU_DEC_ROOT>; > + clocks = <&clk IMX8MQ_CLK_VPU_DEC_ROOT>, > + <&clk IMX8MQ_CLK_VPU_G1_ROOT>, > + <&clk IMX8MQ_CLK_VPU_G2_ROOT>; > + assigned-clocks = <&clk IMX8MQ_CLK_VPU_G1>, > + <&clk IMX8MQ_CLK_VPU_G2>, > + <&clk IMX8MQ_CLK_VPU_BUS>, > + <&clk IMX8MQ_VPU_PLL_BYPASS>; > + assigned-clock-parents = <&clk IMX8MQ_VPU_PLL_OUT>, > + <&clk IMX8MQ_VPU_PLL_OUT>, > + <&clk IMX8MQ_SYS1_PLL_800M>, > + <&clk IMX8MQ_VPU_PLL>; > + assigned-clock-rates = <600000000>, > + <600000000>, > + <800000000>, > + <0>; > }; > > pgc_disp: power-domain@7 { > @@ -1457,30 +1471,31 @@ usb3_phy1: usb-phy@382f0040 { > status = "disabled"; > }; > > - vpu: video-codec@38300000 { > - compatible = "nxp,imx8mq-vpu"; > - reg = <0x38300000 0x10000>, > - <0x38310000 0x10000>, > - <0x38320000 0x10000>; > - reg-names = "g1", "g2", "ctrl"; > - interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>, > - <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>; > - interrupt-names = "g1", "g2"; > + vpu_g1: video-codec@38300000 { > + compatible = "nxp,imx8mq-vpu-g1"; > + reg = <0x38300000 0x10000>; > + interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>; > + power-domains = <&vpu_blk_ctrl IMX8MQ_VPUBLK_PD_G1>; > + }; > + > + vpu_g2: video-codec@38310000 { > + compatible = "nxp,imx8mq-vpu-g2"; > + reg = <0x38310000 0x10000>; > + interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&clk IMX8MQ_CLK_VPU_G2_ROOT>; > + power-domains = <&vpu_blk_ctrl IMX8MQ_VPUBLK_PD_G2>; > + }; > + > + vpu_blk_ctrl: blk-ctrl@38320000 { > + compatible = "fsl,imx8mq-vpu-blk-ctrl"; > + reg = <0x38320000 0x100>; > + power-domains = <&pgc_vpu>, <&pgc_vpu>, <&pgc_vpu>; > + power-domain-names = "bus", "g1", "g2"; > clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>, > - <&clk IMX8MQ_CLK_VPU_G2_ROOT>, > - <&clk IMX8MQ_CLK_VPU_DEC_ROOT>; > - clock-names = "g1", "g2", "bus"; > - assigned-clocks = <&clk IMX8MQ_CLK_VPU_G1>, > - <&clk IMX8MQ_CLK_VPU_G2>, > - <&clk IMX8MQ_CLK_VPU_BUS>, > - <&clk IMX8MQ_VPU_PLL_BYPASS>; > - assigned-clock-parents = <&clk IMX8MQ_VPU_PLL_OUT>, > - <&clk IMX8MQ_VPU_PLL_OUT>, > - <&clk IMX8MQ_SYS1_PLL_800M>, > - <&clk IMX8MQ_VPU_PLL>; > - assigned-clock-rates = <600000000>, <600000000>, > - <800000000>, <0>; > - power-domains = <&pgc_vpu>; > + <&clk IMX8MQ_CLK_VPU_G2_ROOT>; > + clock-names = "g1", "g2"; > + #power-domain-cells = <1>; > }; > > pcie0: pcie@33800000 {
Am Dienstag, dem 25.01.2022 um 11:11 -0600 schrieb Adam Ford: > There are two decoders on the i.MX8M Mini controlled by the > vpu-blk-ctrl. The G1 supports H264 and VP8 while the > G2 support HEVC and VP9. > > Signed-off-by: Adam Ford <aford173@gmail.com> > Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> Reviewed-by: Lucas Stach <l.stach@pengutronix.de> > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > index 0c7a72c51a31..98aec4421713 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > @@ -1272,6 +1272,22 @@ gpu_2d: gpu@38008000 { > power-domains = <&pgc_gpu>; > }; > > + vpu_g1: video-codec@38300000 { > + compatible = "nxp,imx8mm-vpu-g1"; > + reg = <0x38300000 0x10000>; > + interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&clk IMX8MM_CLK_VPU_G1_ROOT>; > + power-domains = <&vpu_blk_ctrl IMX8MM_VPUBLK_PD_G1>; > + }; > + > + vpu_g2: video-codec@38310000 { > + compatible = "nxp,imx8mq-vpu-g2"; > + reg = <0x38310000 0x10000>; > + interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&clk IMX8MM_CLK_VPU_G2_ROOT>; > + power-domains = <&vpu_blk_ctrl IMX8MM_VPUBLK_PD_G2>; > + }; > + > vpu_blk_ctrl: blk-ctrl@38330000 { > compatible = "fsl,imx8mm-vpu-blk-ctrl", "syscon"; > reg = <0x38330000 0x100>; > @@ -1282,6 +1298,12 @@ vpu_blk_ctrl: blk-ctrl@38330000 { > <&clk IMX8MM_CLK_VPU_G2_ROOT>, > <&clk IMX8MM_CLK_VPU_H1_ROOT>; > clock-names = "g1", "g2", "h1"; > + assigned-clocks = <&clk IMX8MM_CLK_VPU_G1>, > + <&clk IMX8MM_CLK_VPU_G2>; > + assigned-clock-parents = <&clk IMX8MM_VPU_PLL_OUT>, > + <&clk IMX8MM_VPU_PLL_OUT>; > + assigned-clock-rates = <600000000>, > + <600000000>; > #power-domain-cells = <1>; > }; >
Am Dienstag, dem 25.01.2022 um 11:11 -0600 schrieb Adam Ford: > The vpu is enabled by default, so there is no need to manually > enable it. > > Signed-off-by: Adam Ford <aford173@gmail.com> Reviewed-by: Lucas Stach <l.stach@pengutronix.de> > > diff --git a/arch/arm64/boot/dts/freescale/imx8mq-tqma8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq-tqma8mq.dtsi > index 8aedcddfeab8..38ffcd145b33 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mq-tqma8mq.dtsi > +++ b/arch/arm64/boot/dts/freescale/imx8mq-tqma8mq.dtsi > @@ -272,10 +272,6 @@ &usdhc1 { > status = "okay"; > }; > > -&vpu { > - status = "okay"; > -}; > - > /* Attention: wdog reset forcing POR needs baseboard support */ > &wdog1 { > status = "okay";
On Tue, Jan 25, 2022 at 11:11:18AM -0600, Adam Ford wrote: > The vpu is enabled by default, so there is no need to manually > enable it. > > Signed-off-by: Adam Ford <aford173@gmail.com> > Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > diff --git a/arch/arm64/boot/dts/freescale/imx8mq-tqma8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq-tqma8mq.dtsi > index 8aedcddfeab8..38ffcd145b33 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mq-tqma8mq.dtsi > +++ b/arch/arm64/boot/dts/freescale/imx8mq-tqma8mq.dtsi > @@ -272,10 +272,6 @@ &usdhc1 { > status = "okay"; > }; > > -&vpu { > - status = "okay"; > -}; > - > /* Attention: wdog reset forcing POR needs baseboard support */ > &wdog1 { > status = "okay"; > -- > 2.32.0 >
On Tue, Jan 25, 2022 at 11:11:24AM -0600, Adam Ford wrote: > With the Hantro G1 and G2 now setup to run independently, update > the device tree to allow both to operate. This requires the > vpu-blk-ctrl node to be configured. Since vpu-blk-ctrl needs > certain clock enabled to handle the gating of the G1 and G2 > fuses, the clock-parents and clock-rates for the various VPU's > to be moved into the pgc_vpu because they cannot get re-parented > once enabled, and the pgc_vpu is the highest in the chain. > > Signed-off-by: Adam Ford <aford173@gmail.com> > Reported-by: kernel test robot <lkp@intel.com> It doesn't seem correct to have the Reported-by on this commit. Thanks, Ezequiel > Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > > diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > index 2df2510d0118..549b2440f55d 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > @@ -737,7 +737,21 @@ pgc_gpu: power-domain@5 { > pgc_vpu: power-domain@6 { > #power-domain-cells = <0>; > reg = <IMX8M_POWER_DOMAIN_VPU>; > - clocks = <&clk IMX8MQ_CLK_VPU_DEC_ROOT>; > + clocks = <&clk IMX8MQ_CLK_VPU_DEC_ROOT>, > + <&clk IMX8MQ_CLK_VPU_G1_ROOT>, > + <&clk IMX8MQ_CLK_VPU_G2_ROOT>; > + assigned-clocks = <&clk IMX8MQ_CLK_VPU_G1>, > + <&clk IMX8MQ_CLK_VPU_G2>, > + <&clk IMX8MQ_CLK_VPU_BUS>, > + <&clk IMX8MQ_VPU_PLL_BYPASS>; > + assigned-clock-parents = <&clk IMX8MQ_VPU_PLL_OUT>, > + <&clk IMX8MQ_VPU_PLL_OUT>, > + <&clk IMX8MQ_SYS1_PLL_800M>, > + <&clk IMX8MQ_VPU_PLL>; > + assigned-clock-rates = <600000000>, > + <600000000>, > + <800000000>, > + <0>; > }; > > pgc_disp: power-domain@7 { > @@ -1457,30 +1471,31 @@ usb3_phy1: usb-phy@382f0040 { > status = "disabled"; > }; > > - vpu: video-codec@38300000 { > - compatible = "nxp,imx8mq-vpu"; > - reg = <0x38300000 0x10000>, > - <0x38310000 0x10000>, > - <0x38320000 0x10000>; > - reg-names = "g1", "g2", "ctrl"; > - interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>, > - <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>; > - interrupt-names = "g1", "g2"; > + vpu_g1: video-codec@38300000 { > + compatible = "nxp,imx8mq-vpu-g1"; > + reg = <0x38300000 0x10000>; > + interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>; > + power-domains = <&vpu_blk_ctrl IMX8MQ_VPUBLK_PD_G1>; > + }; > + > + vpu_g2: video-codec@38310000 { > + compatible = "nxp,imx8mq-vpu-g2"; > + reg = <0x38310000 0x10000>; > + interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&clk IMX8MQ_CLK_VPU_G2_ROOT>; > + power-domains = <&vpu_blk_ctrl IMX8MQ_VPUBLK_PD_G2>; > + }; > + > + vpu_blk_ctrl: blk-ctrl@38320000 { > + compatible = "fsl,imx8mq-vpu-blk-ctrl"; > + reg = <0x38320000 0x100>; > + power-domains = <&pgc_vpu>, <&pgc_vpu>, <&pgc_vpu>; > + power-domain-names = "bus", "g1", "g2"; > clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>, > - <&clk IMX8MQ_CLK_VPU_G2_ROOT>, > - <&clk IMX8MQ_CLK_VPU_DEC_ROOT>; > - clock-names = "g1", "g2", "bus"; > - assigned-clocks = <&clk IMX8MQ_CLK_VPU_G1>, > - <&clk IMX8MQ_CLK_VPU_G2>, > - <&clk IMX8MQ_CLK_VPU_BUS>, > - <&clk IMX8MQ_VPU_PLL_BYPASS>; > - assigned-clock-parents = <&clk IMX8MQ_VPU_PLL_OUT>, > - <&clk IMX8MQ_VPU_PLL_OUT>, > - <&clk IMX8MQ_SYS1_PLL_800M>, > - <&clk IMX8MQ_VPU_PLL>; > - assigned-clock-rates = <600000000>, <600000000>, > - <800000000>, <0>; > - power-domains = <&pgc_vpu>; > + <&clk IMX8MQ_CLK_VPU_G2_ROOT>; > + clock-names = "g1", "g2"; > + #power-domain-cells = <1>; > }; > > pcie0: pcie@33800000 { > -- > 2.32.0 >
On Tue, Jan 25, 2022 at 1:04 PM Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote: > > On Tue, Jan 25, 2022 at 11:11:24AM -0600, Adam Ford wrote: > > With the Hantro G1 and G2 now setup to run independently, update > > the device tree to allow both to operate. This requires the > > vpu-blk-ctrl node to be configured. Since vpu-blk-ctrl needs > > certain clock enabled to handle the gating of the G1 and G2 > > fuses, the clock-parents and clock-rates for the various VPU's > > to be moved into the pgc_vpu because they cannot get re-parented > > once enabled, and the pgc_vpu is the highest in the chain. > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > Reported-by: kernel test robot <lkp@intel.com> > > It doesn't seem correct to have the Reported-by on this commit. I didn't put it here, because I fixed it in a whole different patch (Patch 1/11). This patch remains unchanged. I probably should have put in the other patch, but I didn't think it was essential. Sorry about that. Do I need to resend to just add the r-b tag? adam > > Thanks, > Ezequiel > > > Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > index 2df2510d0118..549b2440f55d 100644 > > --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > @@ -737,7 +737,21 @@ pgc_gpu: power-domain@5 { > > pgc_vpu: power-domain@6 { > > #power-domain-cells = <0>; > > reg = <IMX8M_POWER_DOMAIN_VPU>; > > - clocks = <&clk IMX8MQ_CLK_VPU_DEC_ROOT>; > > + clocks = <&clk IMX8MQ_CLK_VPU_DEC_ROOT>, > > + <&clk IMX8MQ_CLK_VPU_G1_ROOT>, > > + <&clk IMX8MQ_CLK_VPU_G2_ROOT>; > > + assigned-clocks = <&clk IMX8MQ_CLK_VPU_G1>, > > + <&clk IMX8MQ_CLK_VPU_G2>, > > + <&clk IMX8MQ_CLK_VPU_BUS>, > > + <&clk IMX8MQ_VPU_PLL_BYPASS>; > > + assigned-clock-parents = <&clk IMX8MQ_VPU_PLL_OUT>, > > + <&clk IMX8MQ_VPU_PLL_OUT>, > > + <&clk IMX8MQ_SYS1_PLL_800M>, > > + <&clk IMX8MQ_VPU_PLL>; > > + assigned-clock-rates = <600000000>, > > + <600000000>, > > + <800000000>, > > + <0>; > > }; > > > > pgc_disp: power-domain@7 { > > @@ -1457,30 +1471,31 @@ usb3_phy1: usb-phy@382f0040 { > > status = "disabled"; > > }; > > > > - vpu: video-codec@38300000 { > > - compatible = "nxp,imx8mq-vpu"; > > - reg = <0x38300000 0x10000>, > > - <0x38310000 0x10000>, > > - <0x38320000 0x10000>; > > - reg-names = "g1", "g2", "ctrl"; > > - interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>, > > - <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>; > > - interrupt-names = "g1", "g2"; > > + vpu_g1: video-codec@38300000 { > > + compatible = "nxp,imx8mq-vpu-g1"; > > + reg = <0x38300000 0x10000>; > > + interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>; > > + clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>; > > + power-domains = <&vpu_blk_ctrl IMX8MQ_VPUBLK_PD_G1>; > > + }; > > + > > + vpu_g2: video-codec@38310000 { > > + compatible = "nxp,imx8mq-vpu-g2"; > > + reg = <0x38310000 0x10000>; > > + interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>; > > + clocks = <&clk IMX8MQ_CLK_VPU_G2_ROOT>; > > + power-domains = <&vpu_blk_ctrl IMX8MQ_VPUBLK_PD_G2>; > > + }; > > + > > + vpu_blk_ctrl: blk-ctrl@38320000 { > > + compatible = "fsl,imx8mq-vpu-blk-ctrl"; > > + reg = <0x38320000 0x100>; > > + power-domains = <&pgc_vpu>, <&pgc_vpu>, <&pgc_vpu>; > > + power-domain-names = "bus", "g1", "g2"; > > clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>, > > - <&clk IMX8MQ_CLK_VPU_G2_ROOT>, > > - <&clk IMX8MQ_CLK_VPU_DEC_ROOT>; > > - clock-names = "g1", "g2", "bus"; > > - assigned-clocks = <&clk IMX8MQ_CLK_VPU_G1>, > > - <&clk IMX8MQ_CLK_VPU_G2>, > > - <&clk IMX8MQ_CLK_VPU_BUS>, > > - <&clk IMX8MQ_VPU_PLL_BYPASS>; > > - assigned-clock-parents = <&clk IMX8MQ_VPU_PLL_OUT>, > > - <&clk IMX8MQ_VPU_PLL_OUT>, > > - <&clk IMX8MQ_SYS1_PLL_800M>, > > - <&clk IMX8MQ_VPU_PLL>; > > - assigned-clock-rates = <600000000>, <600000000>, > > - <800000000>, <0>; > > - power-domains = <&pgc_vpu>; > > + <&clk IMX8MQ_CLK_VPU_G2_ROOT>; > > + clock-names = "g1", "g2"; > > + #power-domain-cells = <1>; > > }; > > > > pcie0: pcie@33800000 { > > -- > > 2.32.0 > >
Am Dienstag, 25. Januar 2022, 18:11:18 CET schrieb Adam Ford: > The vpu is enabled by default, so there is no need to manually > enable it. > > Signed-off-by: Adam Ford <aford173@gmail.com> Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com> > diff --git a/arch/arm64/boot/dts/freescale/imx8mq-tqma8mq.dtsi > b/arch/arm64/boot/dts/freescale/imx8mq-tqma8mq.dtsi index > 8aedcddfeab8..38ffcd145b33 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mq-tqma8mq.dtsi > +++ b/arch/arm64/boot/dts/freescale/imx8mq-tqma8mq.dtsi > @@ -272,10 +272,6 @@ &usdhc1 { > status = "okay"; > }; > > -&vpu { > - status = "okay"; > -}; > - > /* Attention: wdog reset forcing POR needs baseboard support */ > &wdog1 { > status = "okay";
On Tue, Jan 25, 2022 at 11:11:18AM -0600, Adam Ford wrote: > The vpu is enabled by default, so there is no need to manually > enable it. > > Signed-off-by: Adam Ford <aford173@gmail.com> Applied, thanks!
On Tue, Jan 25, 2022 at 11:11:21AM -0600, Adam Ford wrote: > From: Lucas Stach <l.stach@pengutronix.de> > > This adds the necessary bits to drive the VPU blk-ctrl on the i.MX8MQ, to > avoid putting more of this functionality into the decoder driver. > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > Signed-off-by: Adam Ford <aford173@gmail.com> Applied, thanks!
On Tue, Jan 25, 2022 at 04:04:38PM -0300, Ezequiel Garcia wrote: > On Tue, Jan 25, 2022 at 11:11:24AM -0600, Adam Ford wrote: > > With the Hantro G1 and G2 now setup to run independently, update > > the device tree to allow both to operate. This requires the > > vpu-blk-ctrl node to be configured. Since vpu-blk-ctrl needs > > certain clock enabled to handle the gating of the G1 and G2 > > fuses, the clock-parents and clock-rates for the various VPU's > > to be moved into the pgc_vpu because they cannot get re-parented > > once enabled, and the pgc_vpu is the highest in the chain. > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > Reported-by: kernel test robot <lkp@intel.com> > > It doesn't seem correct to have the Reported-by on this commit. Applied with Reported-by tag dropped, thanks!
On Tue, Jan 25, 2022 at 11:11:25AM -0600, Adam Ford wrote: > The vpumix power domain has a reset assigned to it, however > when used, it causes a system hang. Testing has shown that > it does not appear to be needed anywhere. > > Fixes: d39d4bb15310 ("arm64: dts: imx8mm: add GPC node") > Signed-off-by: Adam Ford <aford173@gmail.com> Applied, thanks!
On Tue, Jan 25, 2022 at 11:11:28AM -0600, Adam Ford wrote: > There are two decoders on the i.MX8M Mini controlled by the > vpu-blk-ctrl. The G1 supports H264 and VP8 while the > G2 support HEVC and VP9. > > Signed-off-by: Adam Ford <aford173@gmail.com> > Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> Applied, thanks!
Am Dienstag, dem 25.01.2022 um 11:11 -0600 schrieb Adam Ford: > With the Hantro G1 and G2 now setup to run independently, update > the device tree to allow both to operate. This requires the > vpu-blk-ctrl node to be configured. Since vpu-blk-ctrl needs > certain clock enabled to handle the gating of the G1 and G2 > fuses, the clock-parents and clock-rates for the various VPU's > to be moved into the pgc_vpu because they cannot get re-parented > once enabled, and the pgc_vpu is the highest in the chain. > > Signed-off-by: Adam Ford <aford173@gmail.com> > Reported-by: kernel test robot <lkp@intel.com> > Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > > diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > index 2df2510d0118..549b2440f55d 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > @@ -737,7 +737,21 @@ pgc_gpu: power-domain@5 { > pgc_vpu: power-domain@6 { > #power-domain-cells = > <0>; > reg = > <IMX8M_POWER_DOMAIN_VPU>; > - clocks = <&clk > IMX8MQ_CLK_VPU_DEC_ROOT>; > + clocks = <&clk > IMX8MQ_CLK_VPU_DEC_ROOT>, > + <&clk > IMX8MQ_CLK_VPU_G1_ROOT>, > + <&clk > IMX8MQ_CLK_VPU_G2_ROOT>; > + assigned-clocks = > <&clk IMX8MQ_CLK_VPU_G1>, > + > <&clk IMX8MQ_CLK_VPU_G2>, > + > <&clk IMX8MQ_CLK_VPU_BUS>, > + > <&clk IMX8MQ_VPU_PLL_BYPASS>; > + assigned-clock- > parents = <&clk IMX8MQ_VPU_PLL_OUT>, > + > <&clk IMX8MQ_VPU_PLL_OUT>, > + > <&clk IMX8MQ_SYS1_PLL_800M>, > + > <&clk IMX8MQ_VPU_PLL>; > + assigned-clock-rates > = <600000000>, > + > <600000000>, > + > <800000000>, > + > <0>; > }; > > pgc_disp: power-domain@7 { > @@ -1457,30 +1471,31 @@ usb3_phy1: usb-phy@382f0040 { > status = "disabled"; > }; > > - vpu: video-codec@38300000 { > - compatible = "nxp,imx8mq-vpu"; > - reg = <0x38300000 0x10000>, > - <0x38310000 0x10000>, > - <0x38320000 0x10000>; > - reg-names = "g1", "g2", "ctrl"; > - interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>, > - <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>; > - interrupt-names = "g1", "g2"; > + vpu_g1: video-codec@38300000 { > + compatible = "nxp,imx8mq-vpu-g1"; > + reg = <0x38300000 0x10000>; > + interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>; > + power-domains = <&vpu_blk_ctrl > IMX8MQ_VPUBLK_PD_G1>; > + }; > + > + vpu_g2: video-codec@38310000 { > + compatible = "nxp,imx8mq-vpu-g2"; > + reg = <0x38310000 0x10000>; > + interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&clk IMX8MQ_CLK_VPU_G2_ROOT>; > + power-domains = <&vpu_blk_ctrl > IMX8MQ_VPUBLK_PD_G2>; > + }; > + > + vpu_blk_ctrl: blk-ctrl@38320000 { > + compatible = "fsl,imx8mq-vpu-blk-ctrl"; > + reg = <0x38320000 0x100>; > + power-domains = <&pgc_vpu>, <&pgc_vpu>, > <&pgc_vpu>; > + power-domain-names = "bus", "g1", "g2"; > clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>, > - <&clk IMX8MQ_CLK_VPU_G2_ROOT>, > - <&clk IMX8MQ_CLK_VPU_DEC_ROOT>; > - clock-names = "g1", "g2", "bus"; > - assigned-clocks = <&clk IMX8MQ_CLK_VPU_G1>, > - <&clk IMX8MQ_CLK_VPU_G2>, > - <&clk IMX8MQ_CLK_VPU_BUS>, > - <&clk > IMX8MQ_VPU_PLL_BYPASS>; > - assigned-clock-parents = <&clk > IMX8MQ_VPU_PLL_OUT>, > - <&clk > IMX8MQ_VPU_PLL_OUT>, > - <&clk > IMX8MQ_SYS1_PLL_800M>, > - <&clk > IMX8MQ_VPU_PLL>; > - assigned-clock-rates = <600000000>, > <600000000>, > - <800000000>, <0>; > - power-domains = <&pgc_vpu>; > + <&clk IMX8MQ_CLK_VPU_G2_ROOT>; > + clock-names = "g1", "g2"; > + #power-domain-cells = <1>; > }; > > pcie0: pcie@33800000 { With this update, when testing suspend to ram on imx8mq, I get: buck4: failed to disable: -ETIMEDOUT where buck4 is power-supply of pgc_vpu. And thus the transition to suspend (and resuming) fails. Have you tested system suspend after the imx8m-blk-ctrl update on imx8mq? thank you, martin
Hi Martin, Am Montag, dem 25.04.2022 um 17:22 +0200 schrieb Martin Kepplinger: > Am Dienstag, dem 25.01.2022 um 11:11 -0600 schrieb Adam Ford: > > With the Hantro G1 and G2 now setup to run independently, update > > the device tree to allow both to operate. This requires the > > vpu-blk-ctrl node to be configured. Since vpu-blk-ctrl needs > > certain clock enabled to handle the gating of the G1 and G2 > > fuses, the clock-parents and clock-rates for the various VPU's > > to be moved into the pgc_vpu because they cannot get re-parented > > once enabled, and the pgc_vpu is the highest in the chain. > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > Reported-by: kernel test robot <lkp@intel.com> > > Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > index 2df2510d0118..549b2440f55d 100644 > > --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > @@ -737,7 +737,21 @@ pgc_gpu: power-domain@5 { > > pgc_vpu: power-domain@6 { > > #power-domain-cells = > > <0>; > > reg = > > <IMX8M_POWER_DOMAIN_VPU>; > > - clocks = <&clk > > IMX8MQ_CLK_VPU_DEC_ROOT>; > > + clocks = <&clk > > IMX8MQ_CLK_VPU_DEC_ROOT>, > > + <&clk > > IMX8MQ_CLK_VPU_G1_ROOT>, > > + <&clk > > IMX8MQ_CLK_VPU_G2_ROOT>; > > + assigned-clocks = > > <&clk IMX8MQ_CLK_VPU_G1>, > > + > > <&clk IMX8MQ_CLK_VPU_G2>, > > + > > <&clk IMX8MQ_CLK_VPU_BUS>, > > + > > <&clk IMX8MQ_VPU_PLL_BYPASS>; > > + assigned-clock- > > parents = <&clk IMX8MQ_VPU_PLL_OUT>, > > + > > <&clk IMX8MQ_VPU_PLL_OUT>, > > + > > <&clk IMX8MQ_SYS1_PLL_800M>, > > + > > <&clk IMX8MQ_VPU_PLL>; > > + assigned-clock-rates > > = <600000000>, > > + > > <600000000>, > > + > > <800000000>, > > + > > <0>; > > }; > > > > pgc_disp: power-domain@7 { > > @@ -1457,30 +1471,31 @@ usb3_phy1: usb-phy@382f0040 { > > status = "disabled"; > > }; > > > > - vpu: video-codec@38300000 { > > - compatible = "nxp,imx8mq-vpu"; > > - reg = <0x38300000 0x10000>, > > - <0x38310000 0x10000>, > > - <0x38320000 0x10000>; > > - reg-names = "g1", "g2", "ctrl"; > > - interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>, > > - <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>; > > - interrupt-names = "g1", "g2"; > > + vpu_g1: video-codec@38300000 { > > + compatible = "nxp,imx8mq-vpu-g1"; > > + reg = <0x38300000 0x10000>; > > + interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>; > > + clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>; > > + power-domains = <&vpu_blk_ctrl > > IMX8MQ_VPUBLK_PD_G1>; > > + }; > > + > > + vpu_g2: video-codec@38310000 { > > + compatible = "nxp,imx8mq-vpu-g2"; > > + reg = <0x38310000 0x10000>; > > + interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>; > > + clocks = <&clk IMX8MQ_CLK_VPU_G2_ROOT>; > > + power-domains = <&vpu_blk_ctrl > > IMX8MQ_VPUBLK_PD_G2>; > > + }; > > + > > + vpu_blk_ctrl: blk-ctrl@38320000 { > > + compatible = "fsl,imx8mq-vpu-blk-ctrl"; > > + reg = <0x38320000 0x100>; > > + power-domains = <&pgc_vpu>, <&pgc_vpu>, > > <&pgc_vpu>; > > + power-domain-names = "bus", "g1", "g2"; > > clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>, > > - <&clk IMX8MQ_CLK_VPU_G2_ROOT>, > > - <&clk IMX8MQ_CLK_VPU_DEC_ROOT>; > > - clock-names = "g1", "g2", "bus"; > > - assigned-clocks = <&clk IMX8MQ_CLK_VPU_G1>, > > - <&clk IMX8MQ_CLK_VPU_G2>, > > - <&clk IMX8MQ_CLK_VPU_BUS>, > > - <&clk > > IMX8MQ_VPU_PLL_BYPASS>; > > - assigned-clock-parents = <&clk > > IMX8MQ_VPU_PLL_OUT>, > > - <&clk > > IMX8MQ_VPU_PLL_OUT>, > > - <&clk > > IMX8MQ_SYS1_PLL_800M>, > > - <&clk > > IMX8MQ_VPU_PLL>; > > - assigned-clock-rates = <600000000>, > > <600000000>, > > - <800000000>, <0>; > > - power-domains = <&pgc_vpu>; > > + <&clk IMX8MQ_CLK_VPU_G2_ROOT>; > > + clock-names = "g1", "g2"; > > + #power-domain-cells = <1>; > > }; > > > > pcie0: pcie@33800000 { > > With this update, when testing suspend to ram on imx8mq, I get: > > buck4: failed to disable: -ETIMEDOUT > > where buck4 is power-supply of pgc_vpu. And thus the transition to > suspend (and resuming) fails. > > Have you tested system suspend after the imx8m-blk-ctrl update on > imx8mq? I haven't tested system suspend, don't know if anyone else did. However I guess that this is just uncovering a preexisting issue in the system suspend sequencing, which you would also hit if the video decoders were active at system suspend time. My guess is that the regulator disable fails, due to the power domains being disabled quite late in the suspend sequence, where i2c communication with the PMIC is no longer possible due to i2c being suspended already or something like that. Maybe you can dig in a bit on the actual sequence on your system and we can see how we can rework things to suspend the power domains at a time where communication with the PMIC is still possible? Regards, Lucas
On Mon, Apr 25, 2022 at 10:34 AM Lucas Stach <l.stach@pengutronix.de> wrote: > > Hi Martin, > > Am Montag, dem 25.04.2022 um 17:22 +0200 schrieb Martin Kepplinger: > > Am Dienstag, dem 25.01.2022 um 11:11 -0600 schrieb Adam Ford: > > > With the Hantro G1 and G2 now setup to run independently, update > > > the device tree to allow both to operate. This requires the > > > vpu-blk-ctrl node to be configured. Since vpu-blk-ctrl needs > > > certain clock enabled to handle the gating of the G1 and G2 > > > fuses, the clock-parents and clock-rates for the various VPU's > > > to be moved into the pgc_vpu because they cannot get re-parented > > > once enabled, and the pgc_vpu is the highest in the chain. > > > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > > Reported-by: kernel test robot <lkp@intel.com> > > > Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > index 2df2510d0118..549b2440f55d 100644 > > > --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > @@ -737,7 +737,21 @@ pgc_gpu: power-domain@5 { > > > pgc_vpu: power-domain@6 { > > > #power-domain-cells = > > > <0>; > > > reg = > > > <IMX8M_POWER_DOMAIN_VPU>; > > > - clocks = <&clk > > > IMX8MQ_CLK_VPU_DEC_ROOT>; > > > + clocks = <&clk > > > IMX8MQ_CLK_VPU_DEC_ROOT>, > > > + <&clk > > > IMX8MQ_CLK_VPU_G1_ROOT>, > > > + <&clk > > > IMX8MQ_CLK_VPU_G2_ROOT>; > > > + assigned-clocks = > > > <&clk IMX8MQ_CLK_VPU_G1>, > > > + > > > <&clk IMX8MQ_CLK_VPU_G2>, > > > + > > > <&clk IMX8MQ_CLK_VPU_BUS>, > > > + > > > <&clk IMX8MQ_VPU_PLL_BYPASS>; > > > + assigned-clock- > > > parents = <&clk IMX8MQ_VPU_PLL_OUT>, > > > + > > > <&clk IMX8MQ_VPU_PLL_OUT>, > > > + > > > <&clk IMX8MQ_SYS1_PLL_800M>, > > > + > > > <&clk IMX8MQ_VPU_PLL>; > > > + assigned-clock-rates > > > = <600000000>, > > > + > > > <600000000>, > > > + > > > <800000000>, > > > + > > > <0>; > > > }; > > > > > > pgc_disp: power-domain@7 { > > > @@ -1457,30 +1471,31 @@ usb3_phy1: usb-phy@382f0040 { > > > status = "disabled"; > > > }; > > > > > > - vpu: video-codec@38300000 { > > > - compatible = "nxp,imx8mq-vpu"; > > > - reg = <0x38300000 0x10000>, > > > - <0x38310000 0x10000>, > > > - <0x38320000 0x10000>; > > > - reg-names = "g1", "g2", "ctrl"; > > > - interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>, > > > - <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>; > > > - interrupt-names = "g1", "g2"; > > > + vpu_g1: video-codec@38300000 { > > > + compatible = "nxp,imx8mq-vpu-g1"; > > > + reg = <0x38300000 0x10000>; > > > + interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>; > > > + clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>; > > > + power-domains = <&vpu_blk_ctrl > > > IMX8MQ_VPUBLK_PD_G1>; > > > + }; > > > + > > > + vpu_g2: video-codec@38310000 { > > > + compatible = "nxp,imx8mq-vpu-g2"; > > > + reg = <0x38310000 0x10000>; > > > + interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>; > > > + clocks = <&clk IMX8MQ_CLK_VPU_G2_ROOT>; > > > + power-domains = <&vpu_blk_ctrl > > > IMX8MQ_VPUBLK_PD_G2>; > > > + }; > > > + > > > + vpu_blk_ctrl: blk-ctrl@38320000 { > > > + compatible = "fsl,imx8mq-vpu-blk-ctrl"; > > > + reg = <0x38320000 0x100>; > > > + power-domains = <&pgc_vpu>, <&pgc_vpu>, > > > <&pgc_vpu>; > > > + power-domain-names = "bus", "g1", "g2"; > > > clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>, > > > - <&clk IMX8MQ_CLK_VPU_G2_ROOT>, > > > - <&clk IMX8MQ_CLK_VPU_DEC_ROOT>; > > > - clock-names = "g1", "g2", "bus"; > > > - assigned-clocks = <&clk IMX8MQ_CLK_VPU_G1>, > > > - <&clk IMX8MQ_CLK_VPU_G2>, > > > - <&clk IMX8MQ_CLK_VPU_BUS>, > > > - <&clk > > > IMX8MQ_VPU_PLL_BYPASS>; > > > - assigned-clock-parents = <&clk > > > IMX8MQ_VPU_PLL_OUT>, > > > - <&clk > > > IMX8MQ_VPU_PLL_OUT>, > > > - <&clk > > > IMX8MQ_SYS1_PLL_800M>, > > > - <&clk > > > IMX8MQ_VPU_PLL>; > > > - assigned-clock-rates = <600000000>, > > > <600000000>, > > > - <800000000>, <0>; > > > - power-domains = <&pgc_vpu>; > > > + <&clk IMX8MQ_CLK_VPU_G2_ROOT>; > > > + clock-names = "g1", "g2"; > > > + #power-domain-cells = <1>; > > > }; > > > > > > pcie0: pcie@33800000 { > > > > With this update, when testing suspend to ram on imx8mq, I get: > > > > buck4: failed to disable: -ETIMEDOUT > > > > where buck4 is power-supply of pgc_vpu. And thus the transition to > > suspend (and resuming) fails. > > > > Have you tested system suspend after the imx8m-blk-ctrl update on > > imx8mq? > > I haven't tested system suspend, don't know if anyone else did. However > I guess that this is just uncovering a preexisting issue in the system > suspend sequencing, which you would also hit if the video decoders were > active at system suspend time. I have not tested it either. > > My guess is that the regulator disable fails, due to the power domains > being disabled quite late in the suspend sequence, where i2c > communication with the PMIC is no longer possible due to i2c being > suspended already or something like that. Maybe you can dig in a bit on > the actual sequence on your system and we can see how we can rework > things to suspend the power domains at a time where communication with > the PMIC is still possible? In the meantime, should we mark the regulator with regulator-always-on so it doesn't attempt to power it down? It might not be ideal,but it might be enough to let it suspend. adam > > Regards, > Lucas >
Am Montag, dem 25.04.2022 um 17:34 +0200 schrieb Lucas Stach: > Hi Martin, > > Am Montag, dem 25.04.2022 um 17:22 +0200 schrieb Martin Kepplinger: > > Am Dienstag, dem 25.01.2022 um 11:11 -0600 schrieb Adam Ford: > > > With the Hantro G1 and G2 now setup to run independently, update > > > the device tree to allow both to operate. This requires the > > > vpu-blk-ctrl node to be configured. Since vpu-blk-ctrl needs > > > certain clock enabled to handle the gating of the G1 and G2 > > > fuses, the clock-parents and clock-rates for the various VPU's > > > to be moved into the pgc_vpu because they cannot get re-parented > > > once enabled, and the pgc_vpu is the highest in the chain. > > > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > > Reported-by: kernel test robot <lkp@intel.com> > > > Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > index 2df2510d0118..549b2440f55d 100644 > > > --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > @@ -737,7 +737,21 @@ pgc_gpu: power-domain@5 { > > > pgc_vpu: power-domain@6 { > > > #power-domain- > > > cells = > > > <0>; > > > reg = > > > <IMX8M_POWER_DOMAIN_VPU>; > > > - clocks = <&clk > > > IMX8MQ_CLK_VPU_DEC_ROOT>; > > > + clocks = <&clk > > > IMX8MQ_CLK_VPU_DEC_ROOT>, > > > + <&clk > > > IMX8MQ_CLK_VPU_G1_ROOT>, > > > + <&clk > > > IMX8MQ_CLK_VPU_G2_ROOT>; > > > + assigned-clocks = > > > <&clk IMX8MQ_CLK_VPU_G1>, > > > + > > > <&clk IMX8MQ_CLK_VPU_G2>, > > > + > > > <&clk IMX8MQ_CLK_VPU_BUS>, > > > + > > > <&clk IMX8MQ_VPU_PLL_BYPASS>; > > > + assigned-clock- > > > parents = <&clk IMX8MQ_VPU_PLL_OUT>, > > > + > > > > > > <&clk IMX8MQ_VPU_PLL_OUT>, > > > + > > > > > > <&clk IMX8MQ_SYS1_PLL_800M>, > > > + > > > > > > <&clk IMX8MQ_VPU_PLL>; > > > + assigned-clock- > > > rates > > > = <600000000>, > > > + > > > > > > <600000000>, > > > + > > > > > > <800000000>, > > > + > > > > > > <0>; > > > }; > > > > > > pgc_disp: power-domain@7 > > > { > > > @@ -1457,30 +1471,31 @@ usb3_phy1: usb-phy@382f0040 { > > > status = "disabled"; > > > }; > > > > > > - vpu: video-codec@38300000 { > > > - compatible = "nxp,imx8mq-vpu"; > > > - reg = <0x38300000 0x10000>, > > > - <0x38310000 0x10000>, > > > - <0x38320000 0x10000>; > > > - reg-names = "g1", "g2", "ctrl"; > > > - interrupts = <GIC_SPI 7 > > > IRQ_TYPE_LEVEL_HIGH>, > > > - <GIC_SPI 8 > > > IRQ_TYPE_LEVEL_HIGH>; > > > - interrupt-names = "g1", "g2"; > > > + vpu_g1: video-codec@38300000 { > > > + compatible = "nxp,imx8mq-vpu-g1"; > > > + reg = <0x38300000 0x10000>; > > > + interrupts = <GIC_SPI 7 > > > IRQ_TYPE_LEVEL_HIGH>; > > > + clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>; > > > + power-domains = <&vpu_blk_ctrl > > > IMX8MQ_VPUBLK_PD_G1>; > > > + }; > > > + > > > + vpu_g2: video-codec@38310000 { > > > + compatible = "nxp,imx8mq-vpu-g2"; > > > + reg = <0x38310000 0x10000>; > > > + interrupts = <GIC_SPI 8 > > > IRQ_TYPE_LEVEL_HIGH>; > > > + clocks = <&clk IMX8MQ_CLK_VPU_G2_ROOT>; > > > + power-domains = <&vpu_blk_ctrl > > > IMX8MQ_VPUBLK_PD_G2>; > > > + }; > > > + > > > + vpu_blk_ctrl: blk-ctrl@38320000 { > > > + compatible = "fsl,imx8mq-vpu-blk-ctrl"; > > > + reg = <0x38320000 0x100>; > > > + power-domains = <&pgc_vpu>, <&pgc_vpu>, > > > <&pgc_vpu>; > > > + power-domain-names = "bus", "g1", "g2"; > > > clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>, > > > - <&clk IMX8MQ_CLK_VPU_G2_ROOT>, > > > - <&clk IMX8MQ_CLK_VPU_DEC_ROOT>; > > > - clock-names = "g1", "g2", "bus"; > > > - assigned-clocks = <&clk > > > IMX8MQ_CLK_VPU_G1>, > > > - <&clk > > > IMX8MQ_CLK_VPU_G2>, > > > - <&clk > > > IMX8MQ_CLK_VPU_BUS>, > > > - <&clk > > > IMX8MQ_VPU_PLL_BYPASS>; > > > - assigned-clock-parents = <&clk > > > IMX8MQ_VPU_PLL_OUT>, > > > - <&clk > > > IMX8MQ_VPU_PLL_OUT>, > > > - <&clk > > > IMX8MQ_SYS1_PLL_800M>, > > > - <&clk > > > IMX8MQ_VPU_PLL>; > > > - assigned-clock-rates = <600000000>, > > > <600000000>, > > > - <800000000>, <0>; > > > - power-domains = <&pgc_vpu>; > > > + <&clk IMX8MQ_CLK_VPU_G2_ROOT>; > > > + clock-names = "g1", "g2"; > > > + #power-domain-cells = <1>; > > > }; > > > > > > pcie0: pcie@33800000 { > > > > With this update, when testing suspend to ram on imx8mq, I get: > > > > buck4: failed to disable: -ETIMEDOUT > > > > where buck4 is power-supply of pgc_vpu. And thus the transition to > > suspend (and resuming) fails. > > > > Have you tested system suspend after the imx8m-blk-ctrl update on > > imx8mq? > > I haven't tested system suspend, don't know if anyone else did. > However > I guess that this is just uncovering a preexisting issue in the > system > suspend sequencing, which you would also hit if the video decoders > were > active at system suspend time. > > My guess is that the regulator disable fails, due to the power > domains > being disabled quite late in the suspend sequence, where i2c > communication with the PMIC is no longer possible due to i2c being > suspended already or something like that. Maybe you can dig in a bit > on > the actual sequence on your system and we can see how we can rework > things to suspend the power domains at a time where communication > with > the PMIC is still possible? What exactly would you like to see? Here's all gpcv2 regulators disabling on suspend. (gpu (domain 5) is disabled by runtime pm often): [ 47.138700] imx-pgc imx-pgc-domain.5: disabling regulator [ 47.298071] Freezing user space processes ... (elapsed 0.008 seconds) done. [ 47.313432] OOM killer disabled. [ 47.316670] Freezing remaining freezable tasks ... (elapsed 2.221 seconds) done. [ 49.672052] imx8m-blk-ctrl 38320000.blk-ctrl: imx8m_blk_ctrl_suspend start [ 49.704417] imx-pgc imx-pgc-domain.0: disabling regulator [ 49.711114] imx-pgc imx-pgc-domain.6: disabling regulator [ 49.819064] buck4: failed to disable: -ETIMEDOUT The stack looks pretty much the same for all of them, from pm_suspend() over genpd_suspend_noiry(). And as a reminder, power-domain nr 0=mipi, 5=gpu, 6=vpu. > > Regards, > Lucas >
Am Montag, dem 25.04.2022 um 10:47 -0500 schrieb Adam Ford: > On Mon, Apr 25, 2022 at 10:34 AM Lucas Stach <l.stach@pengutronix.de> > wrote: > > > > Hi Martin, > > > > Am Montag, dem 25.04.2022 um 17:22 +0200 schrieb Martin Kepplinger: > > > Am Dienstag, dem 25.01.2022 um 11:11 -0600 schrieb Adam Ford: > > > > With the Hantro G1 and G2 now setup to run independently, > > > > update > > > > the device tree to allow both to operate. This requires the > > > > vpu-blk-ctrl node to be configured. Since vpu-blk-ctrl needs > > > > certain clock enabled to handle the gating of the G1 and G2 > > > > fuses, the clock-parents and clock-rates for the various VPU's > > > > to be moved into the pgc_vpu because they cannot get re- > > > > parented > > > > once enabled, and the pgc_vpu is the highest in the chain. > > > > > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > > > Reported-by: kernel test robot <lkp@intel.com> > > > > Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > > > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > index 2df2510d0118..549b2440f55d 100644 > > > > --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > @@ -737,7 +737,21 @@ pgc_gpu: power-domain@5 { > > > > pgc_vpu: power-domain@6 > > > > { > > > > #power-domain- > > > > cells = > > > > <0>; > > > > reg = > > > > <IMX8M_POWER_DOMAIN_VPU>; > > > > - clocks = <&clk > > > > IMX8MQ_CLK_VPU_DEC_ROOT>; > > > > + clocks = <&clk > > > > IMX8MQ_CLK_VPU_DEC_ROOT>, > > > > + <&clk > > > > IMX8MQ_CLK_VPU_G1_ROOT>, > > > > + <&clk > > > > IMX8MQ_CLK_VPU_G2_ROOT>; > > > > + assigned-clocks > > > > = > > > > <&clk IMX8MQ_CLK_VPU_G1>, > > > > + > > > > <&clk IMX8MQ_CLK_VPU_G2>, > > > > + > > > > <&clk IMX8MQ_CLK_VPU_BUS>, > > > > + > > > > <&clk IMX8MQ_VPU_PLL_BYPASS>; > > > > + assigned-clock- > > > > parents = <&clk IMX8MQ_VPU_PLL_OUT>, > > > > + > > > > <&clk IMX8MQ_VPU_PLL_OUT>, > > > > + > > > > <&clk IMX8MQ_SYS1_PLL_800M>, > > > > + > > > > <&clk IMX8MQ_VPU_PLL>; > > > > + assigned-clock- > > > > rates > > > > = <600000000>, > > > > + > > > > <600000000>, > > > > + > > > > <800000000>, > > > > + > > > > <0>; > > > > }; > > > > > > > > pgc_disp: power- > > > > domain@7 { > > > > @@ -1457,30 +1471,31 @@ usb3_phy1: usb-phy@382f0040 { > > > > status = "disabled"; > > > > }; > > > > > > > > - vpu: video-codec@38300000 { > > > > - compatible = "nxp,imx8mq-vpu"; > > > > - reg = <0x38300000 0x10000>, > > > > - <0x38310000 0x10000>, > > > > - <0x38320000 0x10000>; > > > > - reg-names = "g1", "g2", "ctrl"; > > > > - interrupts = <GIC_SPI 7 > > > > IRQ_TYPE_LEVEL_HIGH>, > > > > - <GIC_SPI 8 > > > > IRQ_TYPE_LEVEL_HIGH>; > > > > - interrupt-names = "g1", "g2"; > > > > + vpu_g1: video-codec@38300000 { > > > > + compatible = "nxp,imx8mq-vpu-g1"; > > > > + reg = <0x38300000 0x10000>; > > > > + interrupts = <GIC_SPI 7 > > > > IRQ_TYPE_LEVEL_HIGH>; > > > > + clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>; > > > > + power-domains = <&vpu_blk_ctrl > > > > IMX8MQ_VPUBLK_PD_G1>; > > > > + }; > > > > + > > > > + vpu_g2: video-codec@38310000 { > > > > + compatible = "nxp,imx8mq-vpu-g2"; > > > > + reg = <0x38310000 0x10000>; > > > > + interrupts = <GIC_SPI 8 > > > > IRQ_TYPE_LEVEL_HIGH>; > > > > + clocks = <&clk IMX8MQ_CLK_VPU_G2_ROOT>; > > > > + power-domains = <&vpu_blk_ctrl > > > > IMX8MQ_VPUBLK_PD_G2>; > > > > + }; > > > > + > > > > + vpu_blk_ctrl: blk-ctrl@38320000 { > > > > + compatible = "fsl,imx8mq-vpu-blk-ctrl"; > > > > + reg = <0x38320000 0x100>; > > > > + power-domains = <&pgc_vpu>, <&pgc_vpu>, > > > > <&pgc_vpu>; > > > > + power-domain-names = "bus", "g1", "g2"; > > > > clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>, > > > > - <&clk IMX8MQ_CLK_VPU_G2_ROOT>, > > > > - <&clk > > > > IMX8MQ_CLK_VPU_DEC_ROOT>; > > > > - clock-names = "g1", "g2", "bus"; > > > > - assigned-clocks = <&clk > > > > IMX8MQ_CLK_VPU_G1>, > > > > - <&clk > > > > IMX8MQ_CLK_VPU_G2>, > > > > - <&clk > > > > IMX8MQ_CLK_VPU_BUS>, > > > > - <&clk > > > > IMX8MQ_VPU_PLL_BYPASS>; > > > > - assigned-clock-parents = <&clk > > > > IMX8MQ_VPU_PLL_OUT>, > > > > - <&clk > > > > IMX8MQ_VPU_PLL_OUT>, > > > > - <&clk > > > > IMX8MQ_SYS1_PLL_800M>, > > > > - <&clk > > > > IMX8MQ_VPU_PLL>; > > > > - assigned-clock-rates = <600000000>, > > > > <600000000>, > > > > - <800000000>, > > > > <0>; > > > > - power-domains = <&pgc_vpu>; > > > > + <&clk IMX8MQ_CLK_VPU_G2_ROOT>; > > > > + clock-names = "g1", "g2"; > > > > + #power-domain-cells = <1>; > > > > }; > > > > > > > > pcie0: pcie@33800000 { > > > > > > With this update, when testing suspend to ram on imx8mq, I get: > > > > > > buck4: failed to disable: -ETIMEDOUT > > > > > > where buck4 is power-supply of pgc_vpu. And thus the transition > > > to > > > suspend (and resuming) fails. > > > > > > Have you tested system suspend after the imx8m-blk-ctrl update on > > > imx8mq? > > > > I haven't tested system suspend, don't know if anyone else did. > > However > > I guess that this is just uncovering a preexisting issue in the > > system > > suspend sequencing, which you would also hit if the video decoders > > were > > active at system suspend time. > > I have not tested it either. > > > > > My guess is that the regulator disable fails, due to the power > > domains > > being disabled quite late in the suspend sequence, where i2c > > communication with the PMIC is no longer possible due to i2c being > > suspended already or something like that. Maybe you can dig in a > > bit on > > the actual sequence on your system and we can see how we can rework > > things to suspend the power domains at a time where communication > > with > > the PMIC is still possible? > > In the meantime, should we mark the regulator with regulator-always- > on > so it doesn't attempt to power it down? It might not be ideal,but it > might be enough to let it suspend. > it would be a temporary workaround, but I want to remind you that it wouldn't help much: even if suspending "works" again, system resume is broken on imx8mq since https://lore.kernel.org/all/a20ecd639f8e8b7fa4a9bed7a8e9590225262784.camel@puri.sm/ Of course I did the current tests on v5.18-rc4 without any gpcv2 changes to mainline. But for resume to work I need the one revert from the above link (plus a minor additional hack) already. If we'd have that working in mainline I could make sure it stays that way :) martin
Am Dienstag, dem 26.04.2022 um 12:28 +0200 schrieb Martin Kepplinger: > Am Montag, dem 25.04.2022 um 10:47 -0500 schrieb Adam Ford: > > On Mon, Apr 25, 2022 at 10:34 AM Lucas Stach <l.stach@pengutronix.de> > > wrote: > > > > > > Hi Martin, > > > > > > Am Montag, dem 25.04.2022 um 17:22 +0200 schrieb Martin Kepplinger: > > > > Am Dienstag, dem 25.01.2022 um 11:11 -0600 schrieb Adam Ford: > > > > > With the Hantro G1 and G2 now setup to run independently, > > > > > update > > > > > the device tree to allow both to operate. This requires the > > > > > vpu-blk-ctrl node to be configured. Since vpu-blk-ctrl needs > > > > > certain clock enabled to handle the gating of the G1 and G2 > > > > > fuses, the clock-parents and clock-rates for the various VPU's > > > > > to be moved into the pgc_vpu because they cannot get re- > > > > > parented > > > > > once enabled, and the pgc_vpu is the highest in the chain. > > > > > > > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > > > > Reported-by: kernel test robot <lkp@intel.com> > > > > > Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > > > > > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > > b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > > index 2df2510d0118..549b2440f55d 100644 > > > > > --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > > @@ -737,7 +737,21 @@ pgc_gpu: power-domain@5 { > > > > > pgc_vpu: power-domain@6 > > > > > { > > > > > #power-domain- > > > > > cells = > > > > > <0>; > > > > > reg = > > > > > <IMX8M_POWER_DOMAIN_VPU>; > > > > > - clocks = <&clk > > > > > IMX8MQ_CLK_VPU_DEC_ROOT>; > > > > > + clocks = <&clk > > > > > IMX8MQ_CLK_VPU_DEC_ROOT>, > > > > > + <&clk > > > > > IMX8MQ_CLK_VPU_G1_ROOT>, > > > > > + <&clk > > > > > IMX8MQ_CLK_VPU_G2_ROOT>; > > > > > + assigned-clocks > > > > > = > > > > > <&clk IMX8MQ_CLK_VPU_G1>, > > > > > + > > > > > <&clk IMX8MQ_CLK_VPU_G2>, > > > > > + > > > > > <&clk IMX8MQ_CLK_VPU_BUS>, > > > > > + > > > > > <&clk IMX8MQ_VPU_PLL_BYPASS>; > > > > > + assigned-clock- > > > > > parents = <&clk IMX8MQ_VPU_PLL_OUT>, > > > > > + > > > > > <&clk IMX8MQ_VPU_PLL_OUT>, > > > > > + > > > > > <&clk IMX8MQ_SYS1_PLL_800M>, > > > > > + > > > > > <&clk IMX8MQ_VPU_PLL>; > > > > > + assigned-clock- > > > > > rates > > > > > = <600000000>, > > > > > + > > > > > <600000000>, > > > > > + > > > > > <800000000>, > > > > > + > > > > > <0>; > > > > > }; > > > > > > > > > > pgc_disp: power- > > > > > domain@7 { > > > > > @@ -1457,30 +1471,31 @@ usb3_phy1: usb-phy@382f0040 { > > > > > status = "disabled"; > > > > > }; > > > > > > > > > > - vpu: video-codec@38300000 { > > > > > - compatible = "nxp,imx8mq-vpu"; > > > > > - reg = <0x38300000 0x10000>, > > > > > - <0x38310000 0x10000>, > > > > > - <0x38320000 0x10000>; > > > > > - reg-names = "g1", "g2", "ctrl"; > > > > > - interrupts = <GIC_SPI 7 > > > > > IRQ_TYPE_LEVEL_HIGH>, > > > > > - <GIC_SPI 8 > > > > > IRQ_TYPE_LEVEL_HIGH>; > > > > > - interrupt-names = "g1", "g2"; > > > > > + vpu_g1: video-codec@38300000 { > > > > > + compatible = "nxp,imx8mq-vpu-g1"; > > > > > + reg = <0x38300000 0x10000>; > > > > > + interrupts = <GIC_SPI 7 > > > > > IRQ_TYPE_LEVEL_HIGH>; > > > > > + clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>; > > > > > + power-domains = <&vpu_blk_ctrl > > > > > IMX8MQ_VPUBLK_PD_G1>; > > > > > + }; > > > > > + > > > > > + vpu_g2: video-codec@38310000 { > > > > > + compatible = "nxp,imx8mq-vpu-g2"; > > > > > + reg = <0x38310000 0x10000>; > > > > > + interrupts = <GIC_SPI 8 > > > > > IRQ_TYPE_LEVEL_HIGH>; > > > > > + clocks = <&clk IMX8MQ_CLK_VPU_G2_ROOT>; > > > > > + power-domains = <&vpu_blk_ctrl > > > > > IMX8MQ_VPUBLK_PD_G2>; > > > > > + }; > > > > > + > > > > > + vpu_blk_ctrl: blk-ctrl@38320000 { > > > > > + compatible = "fsl,imx8mq-vpu-blk-ctrl"; > > > > > + reg = <0x38320000 0x100>; > > > > > + power-domains = <&pgc_vpu>, <&pgc_vpu>, > > > > > <&pgc_vpu>; > > > > > + power-domain-names = "bus", "g1", "g2"; > > > > > clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>, > > > > > - <&clk IMX8MQ_CLK_VPU_G2_ROOT>, > > > > > - <&clk > > > > > IMX8MQ_CLK_VPU_DEC_ROOT>; > > > > > - clock-names = "g1", "g2", "bus"; > > > > > - assigned-clocks = <&clk > > > > > IMX8MQ_CLK_VPU_G1>, > > > > > - <&clk > > > > > IMX8MQ_CLK_VPU_G2>, > > > > > - <&clk > > > > > IMX8MQ_CLK_VPU_BUS>, > > > > > - <&clk > > > > > IMX8MQ_VPU_PLL_BYPASS>; > > > > > - assigned-clock-parents = <&clk > > > > > IMX8MQ_VPU_PLL_OUT>, > > > > > - <&clk > > > > > IMX8MQ_VPU_PLL_OUT>, > > > > > - <&clk > > > > > IMX8MQ_SYS1_PLL_800M>, > > > > > - <&clk > > > > > IMX8MQ_VPU_PLL>; > > > > > - assigned-clock-rates = <600000000>, > > > > > <600000000>, > > > > > - <800000000>, > > > > > <0>; > > > > > - power-domains = <&pgc_vpu>; > > > > > + <&clk IMX8MQ_CLK_VPU_G2_ROOT>; > > > > > + clock-names = "g1", "g2"; > > > > > + #power-domain-cells = <1>; > > > > > }; > > > > > > > > > > pcie0: pcie@33800000 { > > > > > > > > With this update, when testing suspend to ram on imx8mq, I get: > > > > > > > > buck4: failed to disable: -ETIMEDOUT > > > > > > > > where buck4 is power-supply of pgc_vpu. And thus the transition > > > > to > > > > suspend (and resuming) fails. > > > > > > > > Have you tested system suspend after the imx8m-blk-ctrl update on > > > > imx8mq? > > > > > > I haven't tested system suspend, don't know if anyone else did. > > > However > > > I guess that this is just uncovering a preexisting issue in the > > > system > > > suspend sequencing, which you would also hit if the video decoders > > > were > > > active at system suspend time. > > > > I have not tested it either. > > > > > > > > My guess is that the regulator disable fails, due to the power > > > domains > > > being disabled quite late in the suspend sequence, where i2c > > > communication with the PMIC is no longer possible due to i2c being > > > suspended already or something like that. Maybe you can dig in a > > > bit on > > > the actual sequence on your system and we can see how we can rework > > > things to suspend the power domains at a time where communication > > > with > > > the PMIC is still possible? > > > > In the meantime, should we mark the regulator with regulator-always- > > on > > so it doesn't attempt to power it down? It might not be ideal,but it > > might be enough to let it suspend. > > > > it would be a temporary workaround, but I want to remind you that it > wouldn't help much: even if suspending "works" again, system resume is > broken on imx8mq since > https://lore.kernel.org/all/a20ecd639f8e8b7fa4a9bed7a8e9590225262784.camel@puri.sm/ > > Of course I did the current tests on v5.18-rc4 without any gpcv2 > changes to mainline. But for resume to work I need the one revert from > the above link (plus a minor additional hack) already. > > If we'd have that working in mainline I could make sure it stays that > way :) > This looks like the same underlying issue to me. We need to suspend the power domains before i2c (or whatever is causing the regulator disable fail) and only resume them after this subsystem is resumed. Regards, Lucas
Am Dienstag, dem 26.04.2022 um 09:38 +0200 schrieb Martin Kepplinger: > Am Montag, dem 25.04.2022 um 17:34 +0200 schrieb Lucas Stach: > > Hi Martin, > > > > Am Montag, dem 25.04.2022 um 17:22 +0200 schrieb Martin Kepplinger: > > > Am Dienstag, dem 25.01.2022 um 11:11 -0600 schrieb Adam Ford: > > > > With the Hantro G1 and G2 now setup to run independently, update > > > > the device tree to allow both to operate. This requires the > > > > vpu-blk-ctrl node to be configured. Since vpu-blk-ctrl needs > > > > certain clock enabled to handle the gating of the G1 and G2 > > > > fuses, the clock-parents and clock-rates for the various VPU's > > > > to be moved into the pgc_vpu because they cannot get re-parented > > > > once enabled, and the pgc_vpu is the highest in the chain. > > > > > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > > > Reported-by: kernel test robot <lkp@intel.com> > > > > Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > > > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > index 2df2510d0118..549b2440f55d 100644 > > > > --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > @@ -737,7 +737,21 @@ pgc_gpu: power-domain@5 { > > > > pgc_vpu: power-domain@6 { > > > > #power-domain- > > > > cells = > > > > <0>; > > > > reg = > > > > <IMX8M_POWER_DOMAIN_VPU>; > > > > - clocks = <&clk > > > > IMX8MQ_CLK_VPU_DEC_ROOT>; > > > > + clocks = <&clk > > > > IMX8MQ_CLK_VPU_DEC_ROOT>, > > > > + <&clk > > > > IMX8MQ_CLK_VPU_G1_ROOT>, > > > > + <&clk > > > > IMX8MQ_CLK_VPU_G2_ROOT>; > > > > + assigned-clocks = > > > > <&clk IMX8MQ_CLK_VPU_G1>, > > > > + > > > > <&clk IMX8MQ_CLK_VPU_G2>, > > > > + > > > > <&clk IMX8MQ_CLK_VPU_BUS>, > > > > + > > > > <&clk IMX8MQ_VPU_PLL_BYPASS>; > > > > + assigned-clock- > > > > parents = <&clk IMX8MQ_VPU_PLL_OUT>, > > > > + > > > > > > > > <&clk IMX8MQ_VPU_PLL_OUT>, > > > > + > > > > > > > > <&clk IMX8MQ_SYS1_PLL_800M>, > > > > + > > > > > > > > <&clk IMX8MQ_VPU_PLL>; > > > > + assigned-clock- > > > > rates > > > > = <600000000>, > > > > + > > > > > > > > <600000000>, > > > > + > > > > > > > > <800000000>, > > > > + > > > > > > > > <0>; > > > > }; > > > > > > > > pgc_disp: power-domain@7 > > > > { > > > > @@ -1457,30 +1471,31 @@ usb3_phy1: usb-phy@382f0040 { > > > > status = "disabled"; > > > > }; > > > > > > > > - vpu: video-codec@38300000 { > > > > - compatible = "nxp,imx8mq-vpu"; > > > > - reg = <0x38300000 0x10000>, > > > > - <0x38310000 0x10000>, > > > > - <0x38320000 0x10000>; > > > > - reg-names = "g1", "g2", "ctrl"; > > > > - interrupts = <GIC_SPI 7 > > > > IRQ_TYPE_LEVEL_HIGH>, > > > > - <GIC_SPI 8 > > > > IRQ_TYPE_LEVEL_HIGH>; > > > > - interrupt-names = "g1", "g2"; > > > > + vpu_g1: video-codec@38300000 { > > > > + compatible = "nxp,imx8mq-vpu-g1"; > > > > + reg = <0x38300000 0x10000>; > > > > + interrupts = <GIC_SPI 7 > > > > IRQ_TYPE_LEVEL_HIGH>; > > > > + clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>; > > > > + power-domains = <&vpu_blk_ctrl > > > > IMX8MQ_VPUBLK_PD_G1>; > > > > + }; > > > > + > > > > + vpu_g2: video-codec@38310000 { > > > > + compatible = "nxp,imx8mq-vpu-g2"; > > > > + reg = <0x38310000 0x10000>; > > > > + interrupts = <GIC_SPI 8 > > > > IRQ_TYPE_LEVEL_HIGH>; > > > > + clocks = <&clk IMX8MQ_CLK_VPU_G2_ROOT>; > > > > + power-domains = <&vpu_blk_ctrl > > > > IMX8MQ_VPUBLK_PD_G2>; > > > > + }; > > > > + > > > > + vpu_blk_ctrl: blk-ctrl@38320000 { > > > > + compatible = "fsl,imx8mq-vpu-blk-ctrl"; > > > > + reg = <0x38320000 0x100>; > > > > + power-domains = <&pgc_vpu>, <&pgc_vpu>, > > > > <&pgc_vpu>; > > > > + power-domain-names = "bus", "g1", "g2"; > > > > clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>, > > > > - <&clk IMX8MQ_CLK_VPU_G2_ROOT>, > > > > - <&clk IMX8MQ_CLK_VPU_DEC_ROOT>; > > > > - clock-names = "g1", "g2", "bus"; > > > > - assigned-clocks = <&clk > > > > IMX8MQ_CLK_VPU_G1>, > > > > - <&clk > > > > IMX8MQ_CLK_VPU_G2>, > > > > - <&clk > > > > IMX8MQ_CLK_VPU_BUS>, > > > > - <&clk > > > > IMX8MQ_VPU_PLL_BYPASS>; > > > > - assigned-clock-parents = <&clk > > > > IMX8MQ_VPU_PLL_OUT>, > > > > - <&clk > > > > IMX8MQ_VPU_PLL_OUT>, > > > > - <&clk > > > > IMX8MQ_SYS1_PLL_800M>, > > > > - <&clk > > > > IMX8MQ_VPU_PLL>; > > > > - assigned-clock-rates = <600000000>, > > > > <600000000>, > > > > - <800000000>, <0>; > > > > - power-domains = <&pgc_vpu>; > > > > + <&clk IMX8MQ_CLK_VPU_G2_ROOT>; > > > > + clock-names = "g1", "g2"; > > > > + #power-domain-cells = <1>; > > > > }; > > > > > > > > pcie0: pcie@33800000 { > > > > > > With this update, when testing suspend to ram on imx8mq, I get: > > > > > > buck4: failed to disable: -ETIMEDOUT > > > > > > where buck4 is power-supply of pgc_vpu. And thus the transition to > > > suspend (and resuming) fails. > > > > > > Have you tested system suspend after the imx8m-blk-ctrl update on > > > imx8mq? > > > > I haven't tested system suspend, don't know if anyone else did. > > However > > I guess that this is just uncovering a preexisting issue in the > > system > > suspend sequencing, which you would also hit if the video decoders > > were > > active at system suspend time. > > > > My guess is that the regulator disable fails, due to the power > > domains > > being disabled quite late in the suspend sequence, where i2c > > communication with the PMIC is no longer possible due to i2c being > > suspended already or something like that. Maybe you can dig in a bit > > on > > the actual sequence on your system and we can see how we can rework > > things to suspend the power domains at a time where communication > > with > > the PMIC is still possible? > > What exactly would you like to see? Here's all gpcv2 regulators > disabling on suspend. (gpu (domain 5) is disabled by runtime pm often): > > [ 47.138700] imx-pgc imx-pgc-domain.5: disabling regulator > [ 47.298071] Freezing user space processes ... (elapsed 0.008 > seconds) done. > [ 47.313432] OOM killer disabled. > [ 47.316670] Freezing remaining freezable tasks ... (elapsed 2.221 > seconds) done. > [ 49.672052] imx8m-blk-ctrl 38320000.blk-ctrl: imx8m_blk_ctrl_suspend > start > [ 49.704417] imx-pgc imx-pgc-domain.0: disabling regulator > [ 49.711114] imx-pgc imx-pgc-domain.6: disabling regulator > [ 49.819064] buck4: failed to disable: -ETIMEDOUT > > The stack looks pretty much the same for all of them, from pm_suspend() > over genpd_suspend_noiry(). So the GPU domain is already suspended before the system suspend, probably due to short runtime PM timeouts. Can you please check at which point the i2c subsystem is suspended? I think we are already past that point when running the PM domain suspend from a _noirq callback. I'll take a look on how we can properly change this ordering. Regards, Lucas
Am Dienstag, dem 26.04.2022 um 12:43 +0200 schrieb Lucas Stach: > Am Dienstag, dem 26.04.2022 um 09:38 +0200 schrieb Martin Kepplinger: > > Am Montag, dem 25.04.2022 um 17:34 +0200 schrieb Lucas Stach: > > > Hi Martin, > > > > > > Am Montag, dem 25.04.2022 um 17:22 +0200 schrieb Martin > > > Kepplinger: > > > > Am Dienstag, dem 25.01.2022 um 11:11 -0600 schrieb Adam Ford: > > > > > With the Hantro G1 and G2 now setup to run independently, > > > > > update > > > > > the device tree to allow both to operate. This requires the > > > > > vpu-blk-ctrl node to be configured. Since vpu-blk-ctrl needs > > > > > certain clock enabled to handle the gating of the G1 and G2 > > > > > fuses, the clock-parents and clock-rates for the various > > > > > VPU's > > > > > to be moved into the pgc_vpu because they cannot get re- > > > > > parented > > > > > once enabled, and the pgc_vpu is the highest in the chain. > > > > > > > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > > > > Reported-by: kernel test robot <lkp@intel.com> > > > > > Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > > > > > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > > b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > > index 2df2510d0118..549b2440f55d 100644 > > > > > --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > > @@ -737,7 +737,21 @@ pgc_gpu: power-domain@5 { > > > > > pgc_vpu: power- > > > > > domain@6 { > > > > > #power- > > > > > domain- > > > > > cells = > > > > > <0>; > > > > > reg = > > > > > <IMX8M_POWER_DOMAIN_VPU>; > > > > > - clocks = > > > > > <&clk > > > > > IMX8MQ_CLK_VPU_DEC_ROOT>; > > > > > + clocks = > > > > > <&clk > > > > > IMX8MQ_CLK_VPU_DEC_ROOT>, > > > > > + > > > > > <&clk > > > > > IMX8MQ_CLK_VPU_G1_ROOT>, > > > > > + > > > > > <&clk > > > > > IMX8MQ_CLK_VPU_G2_ROOT>; > > > > > + assigned- > > > > > clocks = > > > > > <&clk IMX8MQ_CLK_VPU_G1>, > > > > > + > > > > > > > > > > <&clk IMX8MQ_CLK_VPU_G2>, > > > > > + > > > > > > > > > > <&clk IMX8MQ_CLK_VPU_BUS>, > > > > > + > > > > > > > > > > <&clk IMX8MQ_VPU_PLL_BYPASS>; > > > > > + assigned- > > > > > clock- > > > > > parents = <&clk IMX8MQ_VPU_PLL_OUT>, > > > > > + > > > > > > > > > > > > > > > <&clk IMX8MQ_VPU_PLL_OUT>, > > > > > + > > > > > > > > > > > > > > > <&clk IMX8MQ_SYS1_PLL_800M>, > > > > > + > > > > > > > > > > > > > > > <&clk IMX8MQ_VPU_PLL>; > > > > > + assigned- > > > > > clock- > > > > > rates > > > > > = <600000000>, > > > > > + > > > > > > > > > > > > > > > <600000000>, > > > > > + > > > > > > > > > > > > > > > <800000000>, > > > > > + > > > > > > > > > > > > > > > <0>; > > > > > }; > > > > > > > > > > pgc_disp: > > > > > power-domain@7 > > > > > { > > > > > @@ -1457,30 +1471,31 @@ usb3_phy1: usb-phy@382f0040 { > > > > > status = "disabled"; > > > > > }; > > > > > > > > > > - vpu: video-codec@38300000 { > > > > > - compatible = "nxp,imx8mq-vpu"; > > > > > - reg = <0x38300000 0x10000>, > > > > > - <0x38310000 0x10000>, > > > > > - <0x38320000 0x10000>; > > > > > - reg-names = "g1", "g2", "ctrl"; > > > > > - interrupts = <GIC_SPI 7 > > > > > IRQ_TYPE_LEVEL_HIGH>, > > > > > - <GIC_SPI 8 > > > > > IRQ_TYPE_LEVEL_HIGH>; > > > > > - interrupt-names = "g1", "g2"; > > > > > + vpu_g1: video-codec@38300000 { > > > > > + compatible = "nxp,imx8mq-vpu-g1"; > > > > > + reg = <0x38300000 0x10000>; > > > > > + interrupts = <GIC_SPI 7 > > > > > IRQ_TYPE_LEVEL_HIGH>; > > > > > + clocks = <&clk > > > > > IMX8MQ_CLK_VPU_G1_ROOT>; > > > > > + power-domains = <&vpu_blk_ctrl > > > > > IMX8MQ_VPUBLK_PD_G1>; > > > > > + }; > > > > > + > > > > > + vpu_g2: video-codec@38310000 { > > > > > + compatible = "nxp,imx8mq-vpu-g2"; > > > > > + reg = <0x38310000 0x10000>; > > > > > + interrupts = <GIC_SPI 8 > > > > > IRQ_TYPE_LEVEL_HIGH>; > > > > > + clocks = <&clk > > > > > IMX8MQ_CLK_VPU_G2_ROOT>; > > > > > + power-domains = <&vpu_blk_ctrl > > > > > IMX8MQ_VPUBLK_PD_G2>; > > > > > + }; > > > > > + > > > > > + vpu_blk_ctrl: blk-ctrl@38320000 { > > > > > + compatible = "fsl,imx8mq-vpu-blk- > > > > > ctrl"; > > > > > + reg = <0x38320000 0x100>; > > > > > + power-domains = <&pgc_vpu>, > > > > > <&pgc_vpu>, > > > > > <&pgc_vpu>; > > > > > + power-domain-names = "bus", "g1", > > > > > "g2"; > > > > > clocks = <&clk > > > > > IMX8MQ_CLK_VPU_G1_ROOT>, > > > > > - <&clk > > > > > IMX8MQ_CLK_VPU_G2_ROOT>, > > > > > - <&clk > > > > > IMX8MQ_CLK_VPU_DEC_ROOT>; > > > > > - clock-names = "g1", "g2", "bus"; > > > > > - assigned-clocks = <&clk > > > > > IMX8MQ_CLK_VPU_G1>, > > > > > - <&clk > > > > > IMX8MQ_CLK_VPU_G2>, > > > > > - <&clk > > > > > IMX8MQ_CLK_VPU_BUS>, > > > > > - <&clk > > > > > IMX8MQ_VPU_PLL_BYPASS>; > > > > > - assigned-clock-parents = <&clk > > > > > IMX8MQ_VPU_PLL_OUT>, > > > > > - <&clk > > > > > IMX8MQ_VPU_PLL_OUT>, > > > > > - <&clk > > > > > IMX8MQ_SYS1_PLL_800M>, > > > > > - <&clk > > > > > IMX8MQ_VPU_PLL>; > > > > > - assigned-clock-rates = <600000000>, > > > > > <600000000>, > > > > > - <800000000>, > > > > > <0>; > > > > > - power-domains = <&pgc_vpu>; > > > > > + <&clk > > > > > IMX8MQ_CLK_VPU_G2_ROOT>; > > > > > + clock-names = "g1", "g2"; > > > > > + #power-domain-cells = <1>; > > > > > }; > > > > > > > > > > pcie0: pcie@33800000 { > > > > > > > > With this update, when testing suspend to ram on imx8mq, I get: > > > > > > > > buck4: failed to disable: -ETIMEDOUT > > > > > > > > where buck4 is power-supply of pgc_vpu. And thus the transition > > > > to > > > > suspend (and resuming) fails. > > > > > > > > Have you tested system suspend after the imx8m-blk-ctrl update > > > > on > > > > imx8mq? > > > > > > I haven't tested system suspend, don't know if anyone else did. > > > However > > > I guess that this is just uncovering a preexisting issue in the > > > system > > > suspend sequencing, which you would also hit if the video > > > decoders > > > were > > > active at system suspend time. > > > > > > My guess is that the regulator disable fails, due to the power > > > domains > > > being disabled quite late in the suspend sequence, where i2c > > > communication with the PMIC is no longer possible due to i2c > > > being > > > suspended already or something like that. Maybe you can dig in a > > > bit > > > on > > > the actual sequence on your system and we can see how we can > > > rework > > > things to suspend the power domains at a time where communication > > > with > > > the PMIC is still possible? > > > > What exactly would you like to see? Here's all gpcv2 regulators > > disabling on suspend. (gpu (domain 5) is disabled by runtime pm > > often): > > > > [ 47.138700] imx-pgc imx-pgc-domain.5: disabling regulator > > [ 47.298071] Freezing user space processes ... (elapsed 0.008 > > seconds) done. > > [ 47.313432] OOM killer disabled. > > [ 47.316670] Freezing remaining freezable tasks ... (elapsed > > 2.221 > > seconds) done. > > [ 49.672052] imx8m-blk-ctrl 38320000.blk-ctrl: > > imx8m_blk_ctrl_suspend > > start > > [ 49.704417] imx-pgc imx-pgc-domain.0: disabling regulator > > [ 49.711114] imx-pgc imx-pgc-domain.6: disabling regulator > > [ 49.819064] buck4: failed to disable: -ETIMEDOUT > > > > The stack looks pretty much the same for all of them, from > > pm_suspend() > > over genpd_suspend_noiry(). > > So the GPU domain is already suspended before the system suspend, > probably due to short runtime PM timeouts. > > Can you please check at which point the i2c subsystem is suspended? I > think we are already past that point when running the PM domain > suspend > from a _noirq callback. I'll take a look on how we can properly > change > this ordering. > > Regards, > Lucas > not sure whether I correctly check that, but the last i2c_imx_runtime_suspend() (here for i2c1 and i2c3) are executed before the power-domain disable, so that makes sense(?): [ 40.774853] imx-pgc imx-pgc-domain.5: disable regulator now [ 40.786211] imx-i2c 30a20000.i2c: i2c_imx_runtime_suspend [ 40.910189] imx-i2c 30a40000.i2c: i2c_imx_runtime_suspend [ 40.940895] Freezing user space processes ... [ 40.968914] rfkill: input handler enabled [ 40.979955] (elapsed 0.007 seconds) done. [ 40.984033] OOM killer disabled. [ 40.987297] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. [ 41.264683] imx-pgc imx-pgc-domain.0: disable regulator now [ 41.271679] imx-pgc imx-pgc-domain.6: disable regulator now [ 41.378759] buck4: failed to disable: -ETIMEDOUT [ 41.383405] imx-pgc imx-pgc-domain.6: failed to disable regulator: - 110 And you can see that I have the domain.0 regulator set to always-on. Otherwise it would time out as well in this case: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/imx8mq-librem5.dtsi#n900 thank a lot for taking the time to look at this! martin
Am Dienstag, dem 26.04.2022 um 12:43 +0200 schrieb Lucas Stach: > Am Dienstag, dem 26.04.2022 um 09:38 +0200 schrieb Martin Kepplinger: > > Am Montag, dem 25.04.2022 um 17:34 +0200 schrieb Lucas Stach: > > > Hi Martin, > > > > > > Am Montag, dem 25.04.2022 um 17:22 +0200 schrieb Martin > > > Kepplinger: > > > > Am Dienstag, dem 25.01.2022 um 11:11 -0600 schrieb Adam Ford: > > > > > With the Hantro G1 and G2 now setup to run independently, > > > > > update > > > > > the device tree to allow both to operate. This requires the > > > > > vpu-blk-ctrl node to be configured. Since vpu-blk-ctrl needs > > > > > certain clock enabled to handle the gating of the G1 and G2 > > > > > fuses, the clock-parents and clock-rates for the various > > > > > VPU's > > > > > to be moved into the pgc_vpu because they cannot get re- > > > > > parented > > > > > once enabled, and the pgc_vpu is the highest in the chain. > > > > > > > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > > > > Reported-by: kernel test robot <lkp@intel.com> > > > > > Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > > > > > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > > b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > > index 2df2510d0118..549b2440f55d 100644 > > > > > --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > > @@ -737,7 +737,21 @@ pgc_gpu: power-domain@5 { > > > > > pgc_vpu: power- > > > > > domain@6 { > > > > > #power- > > > > > domain- > > > > > cells = > > > > > <0>; > > > > > reg = > > > > > <IMX8M_POWER_DOMAIN_VPU>; > > > > > - clocks = > > > > > <&clk > > > > > IMX8MQ_CLK_VPU_DEC_ROOT>; > > > > > + clocks = > > > > > <&clk > > > > > IMX8MQ_CLK_VPU_DEC_ROOT>, > > > > > + > > > > > <&clk > > > > > IMX8MQ_CLK_VPU_G1_ROOT>, > > > > > + > > > > > <&clk > > > > > IMX8MQ_CLK_VPU_G2_ROOT>; > > > > > + assigned- > > > > > clocks = > > > > > <&clk IMX8MQ_CLK_VPU_G1>, > > > > > + > > > > > > > > > > <&clk IMX8MQ_CLK_VPU_G2>, > > > > > + > > > > > > > > > > <&clk IMX8MQ_CLK_VPU_BUS>, > > > > > + > > > > > > > > > > <&clk IMX8MQ_VPU_PLL_BYPASS>; > > > > > + assigned- > > > > > clock- > > > > > parents = <&clk IMX8MQ_VPU_PLL_OUT>, > > > > > + > > > > > > > > > > > > > > > <&clk IMX8MQ_VPU_PLL_OUT>, > > > > > + > > > > > > > > > > > > > > > <&clk IMX8MQ_SYS1_PLL_800M>, > > > > > + > > > > > > > > > > > > > > > <&clk IMX8MQ_VPU_PLL>; > > > > > + assigned- > > > > > clock- > > > > > rates > > > > > = <600000000>, > > > > > + > > > > > > > > > > > > > > > <600000000>, > > > > > + > > > > > > > > > > > > > > > <800000000>, > > > > > + > > > > > > > > > > > > > > > <0>; > > > > > }; > > > > > > > > > > pgc_disp: > > > > > power-domain@7 > > > > > { > > > > > @@ -1457,30 +1471,31 @@ usb3_phy1: usb-phy@382f0040 { > > > > > status = "disabled"; > > > > > }; > > > > > > > > > > - vpu: video-codec@38300000 { > > > > > - compatible = "nxp,imx8mq-vpu"; > > > > > - reg = <0x38300000 0x10000>, > > > > > - <0x38310000 0x10000>, > > > > > - <0x38320000 0x10000>; > > > > > - reg-names = "g1", "g2", "ctrl"; > > > > > - interrupts = <GIC_SPI 7 > > > > > IRQ_TYPE_LEVEL_HIGH>, > > > > > - <GIC_SPI 8 > > > > > IRQ_TYPE_LEVEL_HIGH>; > > > > > - interrupt-names = "g1", "g2"; > > > > > + vpu_g1: video-codec@38300000 { > > > > > + compatible = "nxp,imx8mq-vpu-g1"; > > > > > + reg = <0x38300000 0x10000>; > > > > > + interrupts = <GIC_SPI 7 > > > > > IRQ_TYPE_LEVEL_HIGH>; > > > > > + clocks = <&clk > > > > > IMX8MQ_CLK_VPU_G1_ROOT>; > > > > > + power-domains = <&vpu_blk_ctrl > > > > > IMX8MQ_VPUBLK_PD_G1>; > > > > > + }; > > > > > + > > > > > + vpu_g2: video-codec@38310000 { > > > > > + compatible = "nxp,imx8mq-vpu-g2"; > > > > > + reg = <0x38310000 0x10000>; > > > > > + interrupts = <GIC_SPI 8 > > > > > IRQ_TYPE_LEVEL_HIGH>; > > > > > + clocks = <&clk > > > > > IMX8MQ_CLK_VPU_G2_ROOT>; > > > > > + power-domains = <&vpu_blk_ctrl > > > > > IMX8MQ_VPUBLK_PD_G2>; > > > > > + }; > > > > > + > > > > > + vpu_blk_ctrl: blk-ctrl@38320000 { > > > > > + compatible = "fsl,imx8mq-vpu-blk- > > > > > ctrl"; > > > > > + reg = <0x38320000 0x100>; > > > > > + power-domains = <&pgc_vpu>, > > > > > <&pgc_vpu>, > > > > > <&pgc_vpu>; > > > > > + power-domain-names = "bus", "g1", > > > > > "g2"; > > > > > clocks = <&clk > > > > > IMX8MQ_CLK_VPU_G1_ROOT>, > > > > > - <&clk > > > > > IMX8MQ_CLK_VPU_G2_ROOT>, > > > > > - <&clk > > > > > IMX8MQ_CLK_VPU_DEC_ROOT>; > > > > > - clock-names = "g1", "g2", "bus"; > > > > > - assigned-clocks = <&clk > > > > > IMX8MQ_CLK_VPU_G1>, > > > > > - <&clk > > > > > IMX8MQ_CLK_VPU_G2>, > > > > > - <&clk > > > > > IMX8MQ_CLK_VPU_BUS>, > > > > > - <&clk > > > > > IMX8MQ_VPU_PLL_BYPASS>; > > > > > - assigned-clock-parents = <&clk > > > > > IMX8MQ_VPU_PLL_OUT>, > > > > > - <&clk > > > > > IMX8MQ_VPU_PLL_OUT>, > > > > > - <&clk > > > > > IMX8MQ_SYS1_PLL_800M>, > > > > > - <&clk > > > > > IMX8MQ_VPU_PLL>; > > > > > - assigned-clock-rates = <600000000>, > > > > > <600000000>, > > > > > - <800000000>, > > > > > <0>; > > > > > - power-domains = <&pgc_vpu>; > > > > > + <&clk > > > > > IMX8MQ_CLK_VPU_G2_ROOT>; > > > > > + clock-names = "g1", "g2"; > > > > > + #power-domain-cells = <1>; > > > > > }; > > > > > > > > > > pcie0: pcie@33800000 { > > > > > > > > With this update, when testing suspend to ram on imx8mq, I get: > > > > > > > > buck4: failed to disable: -ETIMEDOUT > > > > > > > > where buck4 is power-supply of pgc_vpu. And thus the transition > > > > to > > > > suspend (and resuming) fails. > > > > > > > > Have you tested system suspend after the imx8m-blk-ctrl update > > > > on > > > > imx8mq? > > > > > > I haven't tested system suspend, don't know if anyone else did. > > > However > > > I guess that this is just uncovering a preexisting issue in the > > > system > > > suspend sequencing, which you would also hit if the video > > > decoders > > > were > > > active at system suspend time. > > > > > > My guess is that the regulator disable fails, due to the power > > > domains > > > being disabled quite late in the suspend sequence, where i2c > > > communication with the PMIC is no longer possible due to i2c > > > being > > > suspended already or something like that. Maybe you can dig in a > > > bit > > > on > > > the actual sequence on your system and we can see how we can > > > rework > > > things to suspend the power domains at a time where communication > > > with > > > the PMIC is still possible? > > > > What exactly would you like to see? Here's all gpcv2 regulators > > disabling on suspend. (gpu (domain 5) is disabled by runtime pm > > often): > > > > [ 47.138700] imx-pgc imx-pgc-domain.5: disabling regulator > > [ 47.298071] Freezing user space processes ... (elapsed 0.008 > > seconds) done. > > [ 47.313432] OOM killer disabled. > > [ 47.316670] Freezing remaining freezable tasks ... (elapsed > > 2.221 > > seconds) done. > > [ 49.672052] imx8m-blk-ctrl 38320000.blk-ctrl: > > imx8m_blk_ctrl_suspend > > start > > [ 49.704417] imx-pgc imx-pgc-domain.0: disabling regulator > > [ 49.711114] imx-pgc imx-pgc-domain.6: disabling regulator > > [ 49.819064] buck4: failed to disable: -ETIMEDOUT > > > > The stack looks pretty much the same for all of them, from > > pm_suspend() > > over genpd_suspend_noiry(). > > So the GPU domain is already suspended before the system suspend, > probably due to short runtime PM timeouts. > > Can you please check at which point the i2c subsystem is suspended? I > think we are already past that point when running the PM domain > suspend > from a _noirq callback. I'll take a look on how we can properly > change > this ordering. hi Lucas, when I just add the same system suspend hook you added to gpcv2 (pm_runtime_get()), I get the same timeout: [ 366.100154] Freezing remaining freezable tasks ... (elapsed 2.863 seconds) done. [ 369.228387] imx-i2c 30a50000.i2c: pm_runtime get. [ 369.234204] imx-i2c 30a40000.i2c: pm_runtime get. [ 369.239834] imx-i2c 30a30000.i2c: pm_runtime get. [ 369.247417] imx-i2c 30a20000.i2c: pm_runtime get. [ 369.277838] imx-pgc imx-pgc-domain.0: disable regulator now [ 369.284758] imx-pgc imx-pgc-domain.6: disable regulator now [ 369.394954] buck4: failed to disable: -ETIMEDOUT do you have an idea? martin
Am Dienstag, dem 26.04.2022 um 12:43 +0200 schrieb Lucas Stach: > Am Dienstag, dem 26.04.2022 um 09:38 +0200 schrieb Martin Kepplinger: > > Am Montag, dem 25.04.2022 um 17:34 +0200 schrieb Lucas Stach: > > > Hi Martin, > > > > > > Am Montag, dem 25.04.2022 um 17:22 +0200 schrieb Martin > > > Kepplinger: > > > > Am Dienstag, dem 25.01.2022 um 11:11 -0600 schrieb Adam Ford: > > > > > With the Hantro G1 and G2 now setup to run independently, > > > > > update > > > > > the device tree to allow both to operate. This requires the > > > > > vpu-blk-ctrl node to be configured. Since vpu-blk-ctrl needs > > > > > certain clock enabled to handle the gating of the G1 and G2 > > > > > fuses, the clock-parents and clock-rates for the various > > > > > VPU's > > > > > to be moved into the pgc_vpu because they cannot get re- > > > > > parented > > > > > once enabled, and the pgc_vpu is the highest in the chain. > > > > > > > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > > > > Reported-by: kernel test robot <lkp@intel.com> > > > > > Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > > > > > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > > b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > > index 2df2510d0118..549b2440f55d 100644 > > > > > --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > > @@ -737,7 +737,21 @@ pgc_gpu: power-domain@5 { > > > > > pgc_vpu: power- > > > > > domain@6 { > > > > > #power- > > > > > domain- > > > > > cells = > > > > > <0>; > > > > > reg = > > > > > <IMX8M_POWER_DOMAIN_VPU>; > > > > > - clocks = > > > > > <&clk > > > > > IMX8MQ_CLK_VPU_DEC_ROOT>; > > > > > + clocks = > > > > > <&clk > > > > > IMX8MQ_CLK_VPU_DEC_ROOT>, > > > > > + > > > > > <&clk > > > > > IMX8MQ_CLK_VPU_G1_ROOT>, > > > > > + > > > > > <&clk > > > > > IMX8MQ_CLK_VPU_G2_ROOT>; > > > > > + assigned- > > > > > clocks = > > > > > <&clk IMX8MQ_CLK_VPU_G1>, > > > > > + > > > > > > > > > > <&clk IMX8MQ_CLK_VPU_G2>, > > > > > + > > > > > > > > > > <&clk IMX8MQ_CLK_VPU_BUS>, > > > > > + > > > > > > > > > > <&clk IMX8MQ_VPU_PLL_BYPASS>; > > > > > + assigned- > > > > > clock- > > > > > parents = <&clk IMX8MQ_VPU_PLL_OUT>, > > > > > + > > > > > > > > > > > > > > > <&clk IMX8MQ_VPU_PLL_OUT>, > > > > > + > > > > > > > > > > > > > > > <&clk IMX8MQ_SYS1_PLL_800M>, > > > > > + > > > > > > > > > > > > > > > <&clk IMX8MQ_VPU_PLL>; > > > > > + assigned- > > > > > clock- > > > > > rates > > > > > = <600000000>, > > > > > + > > > > > > > > > > > > > > > <600000000>, > > > > > + > > > > > > > > > > > > > > > <800000000>, > > > > > + > > > > > > > > > > > > > > > <0>; > > > > > }; > > > > > > > > > > pgc_disp: > > > > > power-domain@7 > > > > > { > > > > > @@ -1457,30 +1471,31 @@ usb3_phy1: usb-phy@382f0040 { > > > > > status = "disabled"; > > > > > }; > > > > > > > > > > - vpu: video-codec@38300000 { > > > > > - compatible = "nxp,imx8mq-vpu"; > > > > > - reg = <0x38300000 0x10000>, > > > > > - <0x38310000 0x10000>, > > > > > - <0x38320000 0x10000>; > > > > > - reg-names = "g1", "g2", "ctrl"; > > > > > - interrupts = <GIC_SPI 7 > > > > > IRQ_TYPE_LEVEL_HIGH>, > > > > > - <GIC_SPI 8 > > > > > IRQ_TYPE_LEVEL_HIGH>; > > > > > - interrupt-names = "g1", "g2"; > > > > > + vpu_g1: video-codec@38300000 { > > > > > + compatible = "nxp,imx8mq-vpu-g1"; > > > > > + reg = <0x38300000 0x10000>; > > > > > + interrupts = <GIC_SPI 7 > > > > > IRQ_TYPE_LEVEL_HIGH>; > > > > > + clocks = <&clk > > > > > IMX8MQ_CLK_VPU_G1_ROOT>; > > > > > + power-domains = <&vpu_blk_ctrl > > > > > IMX8MQ_VPUBLK_PD_G1>; > > > > > + }; > > > > > + > > > > > + vpu_g2: video-codec@38310000 { > > > > > + compatible = "nxp,imx8mq-vpu-g2"; > > > > > + reg = <0x38310000 0x10000>; > > > > > + interrupts = <GIC_SPI 8 > > > > > IRQ_TYPE_LEVEL_HIGH>; > > > > > + clocks = <&clk > > > > > IMX8MQ_CLK_VPU_G2_ROOT>; > > > > > + power-domains = <&vpu_blk_ctrl > > > > > IMX8MQ_VPUBLK_PD_G2>; > > > > > + }; > > > > > + > > > > > + vpu_blk_ctrl: blk-ctrl@38320000 { > > > > > + compatible = "fsl,imx8mq-vpu-blk- > > > > > ctrl"; > > > > > + reg = <0x38320000 0x100>; > > > > > + power-domains = <&pgc_vpu>, > > > > > <&pgc_vpu>, > > > > > <&pgc_vpu>; > > > > > + power-domain-names = "bus", "g1", > > > > > "g2"; > > > > > clocks = <&clk > > > > > IMX8MQ_CLK_VPU_G1_ROOT>, > > > > > - <&clk > > > > > IMX8MQ_CLK_VPU_G2_ROOT>, > > > > > - <&clk > > > > > IMX8MQ_CLK_VPU_DEC_ROOT>; > > > > > - clock-names = "g1", "g2", "bus"; > > > > > - assigned-clocks = <&clk > > > > > IMX8MQ_CLK_VPU_G1>, > > > > > - <&clk > > > > > IMX8MQ_CLK_VPU_G2>, > > > > > - <&clk > > > > > IMX8MQ_CLK_VPU_BUS>, > > > > > - <&clk > > > > > IMX8MQ_VPU_PLL_BYPASS>; > > > > > - assigned-clock-parents = <&clk > > > > > IMX8MQ_VPU_PLL_OUT>, > > > > > - <&clk > > > > > IMX8MQ_VPU_PLL_OUT>, > > > > > - <&clk > > > > > IMX8MQ_SYS1_PLL_800M>, > > > > > - <&clk > > > > > IMX8MQ_VPU_PLL>; > > > > > - assigned-clock-rates = <600000000>, > > > > > <600000000>, > > > > > - <800000000>, > > > > > <0>; > > > > > - power-domains = <&pgc_vpu>; > > > > > + <&clk > > > > > IMX8MQ_CLK_VPU_G2_ROOT>; > > > > > + clock-names = "g1", "g2"; > > > > > + #power-domain-cells = <1>; > > > > > }; > > > > > > > > > > pcie0: pcie@33800000 { > > > > > > > > With this update, when testing suspend to ram on imx8mq, I get: > > > > > > > > buck4: failed to disable: -ETIMEDOUT > > > > > > > > where buck4 is power-supply of pgc_vpu. And thus the transition > > > > to > > > > suspend (and resuming) fails. > > > > > > > > Have you tested system suspend after the imx8m-blk-ctrl update > > > > on > > > > imx8mq? > > > > > > I haven't tested system suspend, don't know if anyone else did. > > > However > > > I guess that this is just uncovering a preexisting issue in the > > > system > > > suspend sequencing, which you would also hit if the video > > > decoders > > > were > > > active at system suspend time. > > > > > > My guess is that the regulator disable fails, due to the power > > > domains > > > being disabled quite late in the suspend sequence, where i2c > > > communication with the PMIC is no longer possible due to i2c > > > being > > > suspended already or something like that. Maybe you can dig in a > > > bit > > > on > > > the actual sequence on your system and we can see how we can > > > rework > > > things to suspend the power domains at a time where communication > > > with > > > the PMIC is still possible? > > > > What exactly would you like to see? Here's all gpcv2 regulators > > disabling on suspend. (gpu (domain 5) is disabled by runtime pm > > often): > > > > [ 47.138700] imx-pgc imx-pgc-domain.5: disabling regulator > > [ 47.298071] Freezing user space processes ... (elapsed 0.008 > > seconds) done. > > [ 47.313432] OOM killer disabled. > > [ 47.316670] Freezing remaining freezable tasks ... (elapsed > > 2.221 > > seconds) done. > > [ 49.672052] imx8m-blk-ctrl 38320000.blk-ctrl: > > imx8m_blk_ctrl_suspend > > start > > [ 49.704417] imx-pgc imx-pgc-domain.0: disabling regulator > > [ 49.711114] imx-pgc imx-pgc-domain.6: disabling regulator > > [ 49.819064] buck4: failed to disable: -ETIMEDOUT > > > > The stack looks pretty much the same for all of them, from > > pm_suspend() > > over genpd_suspend_noiry(). > > So the GPU domain is already suspended before the system suspend, > probably due to short runtime PM timeouts. > > Can you please check at which point the i2c subsystem is suspended? I > think we are already past that point when running the PM domain > suspend > from a _noirq callback. I'll take a look on how we can properly > change > this ordering. > > Regards, > Lucas > hi Lucas, just to update a bit: (I rmmod hantro_vpu because it adds another bug but let's ignore for now) when I use a mainline kernel and for testing just ignore ETIMEDOUT in gpcv2 on regulator_disable(): - return ret; + if (ret == -ETIMEDOUT) + ret = 0; + else + return ret; I see the error message the first time I suspend. Not the consecutive times (and the systems seems to be working after resuming). Now what's weird is when I suspend and resume often, after some time, resume will definitely fail. Not sure yet where, but when I add debug output for the kernel/ directory, that's usually about where it hangs: [ 969.620825] OOM killer enabled. [ 969.623971] Restarting tasks ... [ 969.624230] systemd-udevd left refrigerator [ 969.624535] khugepaged left refrigerator [ 969.624568] gdbus left refrigerator [ 969.624795] alsa-sink-308b0 left refrigerator [ 969.626864] pipewire left refrigerator [ 969.626880] alsa-source-308 left refrigerator [ 969.626913] alsa-source-300 left refrigerator [ 969.626920] gmain left refrigerator [ 969.626949] gvfs-afc-volume left refrigerator [ 969.626949] systemd left refrigerator [ 969.626977] gmain left refrigerator [ 969.627009] chatty left refrigerator [ 969.627036] gmain left refrigerator [ 969.627060] gmain left refrigerator [ 969.627087] gmain left refrigerator And the above hang ("sometimes") happens for "platform" in pm_test already, so I guess it's purely a linux bug. just so you know, in case that tells you anything more... thanks, martin
Am Dienstag, dem 26.04.2022 um 12:43 +0200 schrieb Lucas Stach: > Am Dienstag, dem 26.04.2022 um 09:38 +0200 schrieb Martin Kepplinger: > > Am Montag, dem 25.04.2022 um 17:34 +0200 schrieb Lucas Stach: > > > Hi Martin, > > > > > > Am Montag, dem 25.04.2022 um 17:22 +0200 schrieb Martin > > > Kepplinger: > > > > Am Dienstag, dem 25.01.2022 um 11:11 -0600 schrieb Adam Ford: > > > > > With the Hantro G1 and G2 now setup to run independently, > > > > > update > > > > > the device tree to allow both to operate. This requires the > > > > > vpu-blk-ctrl node to be configured. Since vpu-blk-ctrl needs > > > > > certain clock enabled to handle the gating of the G1 and G2 > > > > > fuses, the clock-parents and clock-rates for the various > > > > > VPU's > > > > > to be moved into the pgc_vpu because they cannot get re- > > > > > parented > > > > > once enabled, and the pgc_vpu is the highest in the chain. > > > > > > > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > > > > Reported-by: kernel test robot <lkp@intel.com> > > > > > Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > > > > > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > > b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > > index 2df2510d0118..549b2440f55d 100644 > > > > > --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > > @@ -737,7 +737,21 @@ pgc_gpu: power-domain@5 { > > > > > pgc_vpu: power- > > > > > domain@6 { > > > > > #power- > > > > > domain- > > > > > cells = > > > > > <0>; > > > > > reg = > > > > > <IMX8M_POWER_DOMAIN_VPU>; > > > > > - clocks = > > > > > <&clk > > > > > IMX8MQ_CLK_VPU_DEC_ROOT>; > > > > > + clocks = > > > > > <&clk > > > > > IMX8MQ_CLK_VPU_DEC_ROOT>, > > > > > + > > > > > <&clk > > > > > IMX8MQ_CLK_VPU_G1_ROOT>, > > > > > + > > > > > <&clk > > > > > IMX8MQ_CLK_VPU_G2_ROOT>; > > > > > + assigned- > > > > > clocks = > > > > > <&clk IMX8MQ_CLK_VPU_G1>, > > > > > + > > > > > > > > > > <&clk IMX8MQ_CLK_VPU_G2>, > > > > > + > > > > > > > > > > <&clk IMX8MQ_CLK_VPU_BUS>, > > > > > + > > > > > > > > > > <&clk IMX8MQ_VPU_PLL_BYPASS>; > > > > > + assigned- > > > > > clock- > > > > > parents = <&clk IMX8MQ_VPU_PLL_OUT>, > > > > > + > > > > > > > > > > > > > > > <&clk IMX8MQ_VPU_PLL_OUT>, > > > > > + > > > > > > > > > > > > > > > <&clk IMX8MQ_SYS1_PLL_800M>, > > > > > + > > > > > > > > > > > > > > > <&clk IMX8MQ_VPU_PLL>; > > > > > + assigned- > > > > > clock- > > > > > rates > > > > > = <600000000>, > > > > > + > > > > > > > > > > > > > > > <600000000>, > > > > > + > > > > > > > > > > > > > > > <800000000>, > > > > > + > > > > > > > > > > > > > > > <0>; > > > > > }; > > > > > > > > > > pgc_disp: > > > > > power-domain@7 > > > > > { > > > > > @@ -1457,30 +1471,31 @@ usb3_phy1: usb-phy@382f0040 { > > > > > status = "disabled"; > > > > > }; > > > > > > > > > > - vpu: video-codec@38300000 { > > > > > - compatible = "nxp,imx8mq-vpu"; > > > > > - reg = <0x38300000 0x10000>, > > > > > - <0x38310000 0x10000>, > > > > > - <0x38320000 0x10000>; > > > > > - reg-names = "g1", "g2", "ctrl"; > > > > > - interrupts = <GIC_SPI 7 > > > > > IRQ_TYPE_LEVEL_HIGH>, > > > > > - <GIC_SPI 8 > > > > > IRQ_TYPE_LEVEL_HIGH>; > > > > > - interrupt-names = "g1", "g2"; > > > > > + vpu_g1: video-codec@38300000 { > > > > > + compatible = "nxp,imx8mq-vpu-g1"; > > > > > + reg = <0x38300000 0x10000>; > > > > > + interrupts = <GIC_SPI 7 > > > > > IRQ_TYPE_LEVEL_HIGH>; > > > > > + clocks = <&clk > > > > > IMX8MQ_CLK_VPU_G1_ROOT>; > > > > > + power-domains = <&vpu_blk_ctrl > > > > > IMX8MQ_VPUBLK_PD_G1>; > > > > > + }; > > > > > + > > > > > + vpu_g2: video-codec@38310000 { > > > > > + compatible = "nxp,imx8mq-vpu-g2"; > > > > > + reg = <0x38310000 0x10000>; > > > > > + interrupts = <GIC_SPI 8 > > > > > IRQ_TYPE_LEVEL_HIGH>; > > > > > + clocks = <&clk > > > > > IMX8MQ_CLK_VPU_G2_ROOT>; > > > > > + power-domains = <&vpu_blk_ctrl > > > > > IMX8MQ_VPUBLK_PD_G2>; > > > > > + }; > > > > > + > > > > > + vpu_blk_ctrl: blk-ctrl@38320000 { > > > > > + compatible = "fsl,imx8mq-vpu-blk- > > > > > ctrl"; > > > > > + reg = <0x38320000 0x100>; > > > > > + power-domains = <&pgc_vpu>, > > > > > <&pgc_vpu>, > > > > > <&pgc_vpu>; > > > > > + power-domain-names = "bus", "g1", > > > > > "g2"; > > > > > clocks = <&clk > > > > > IMX8MQ_CLK_VPU_G1_ROOT>, > > > > > - <&clk > > > > > IMX8MQ_CLK_VPU_G2_ROOT>, > > > > > - <&clk > > > > > IMX8MQ_CLK_VPU_DEC_ROOT>; > > > > > - clock-names = "g1", "g2", "bus"; > > > > > - assigned-clocks = <&clk > > > > > IMX8MQ_CLK_VPU_G1>, > > > > > - <&clk > > > > > IMX8MQ_CLK_VPU_G2>, > > > > > - <&clk > > > > > IMX8MQ_CLK_VPU_BUS>, > > > > > - <&clk > > > > > IMX8MQ_VPU_PLL_BYPASS>; > > > > > - assigned-clock-parents = <&clk > > > > > IMX8MQ_VPU_PLL_OUT>, > > > > > - <&clk > > > > > IMX8MQ_VPU_PLL_OUT>, > > > > > - <&clk > > > > > IMX8MQ_SYS1_PLL_800M>, > > > > > - <&clk > > > > > IMX8MQ_VPU_PLL>; > > > > > - assigned-clock-rates = <600000000>, > > > > > <600000000>, > > > > > - <800000000>, > > > > > <0>; > > > > > - power-domains = <&pgc_vpu>; > > > > > + <&clk > > > > > IMX8MQ_CLK_VPU_G2_ROOT>; > > > > > + clock-names = "g1", "g2"; > > > > > + #power-domain-cells = <1>; > > > > > }; > > > > > > > > > > pcie0: pcie@33800000 { > > > > > > > > With this update, when testing suspend to ram on imx8mq, I get: > > > > > > > > buck4: failed to disable: -ETIMEDOUT > > > > > > > > where buck4 is power-supply of pgc_vpu. And thus the transition > > > > to > > > > suspend (and resuming) fails. > > > > > > > > Have you tested system suspend after the imx8m-blk-ctrl update > > > > on > > > > imx8mq? > > > > > > I haven't tested system suspend, don't know if anyone else did. > > > However > > > I guess that this is just uncovering a preexisting issue in the > > > system > > > suspend sequencing, which you would also hit if the video > > > decoders > > > were > > > active at system suspend time. > > > > > > My guess is that the regulator disable fails, due to the power > > > domains > > > being disabled quite late in the suspend sequence, where i2c > > > communication with the PMIC is no longer possible due to i2c > > > being > > > suspended already or something like that. Maybe you can dig in a > > > bit > > > on > > > the actual sequence on your system and we can see how we can > > > rework > > > things to suspend the power domains at a time where communication > > > with > > > the PMIC is still possible? > > > > What exactly would you like to see? Here's all gpcv2 regulators > > disabling on suspend. (gpu (domain 5) is disabled by runtime pm > > often): > > > > [ 47.138700] imx-pgc imx-pgc-domain.5: disabling regulator > > [ 47.298071] Freezing user space processes ... (elapsed 0.008 > > seconds) done. > > [ 47.313432] OOM killer disabled. > > [ 47.316670] Freezing remaining freezable tasks ... (elapsed > > 2.221 > > seconds) done. > > [ 49.672052] imx8m-blk-ctrl 38320000.blk-ctrl: > > imx8m_blk_ctrl_suspend > > start > > [ 49.704417] imx-pgc imx-pgc-domain.0: disabling regulator > > [ 49.711114] imx-pgc imx-pgc-domain.6: disabling regulator > > [ 49.819064] buck4: failed to disable: -ETIMEDOUT > > > > The stack looks pretty much the same for all of them, from > > pm_suspend() > > over genpd_suspend_noiry(). > > So the GPU domain is already suspended before the system suspend, > probably due to short runtime PM timeouts. > > Can you please check at which point the i2c subsystem is suspended? I > think we are already past that point when running the PM domain > suspend > from a _noirq callback. I'll take a look on how we can properly > change > this ordering. > > Regards, > Lucas > hi Lucas, sorry for not following up on this for so long. This fixes suspend/resume for me: https://lore.kernel.org/linux-arm-kernel/20220711094549.3445566-1-martin.kepplinger@puri.sm/T/#t thank you for you help so far, martin
Hi Martin, On Mon, Jul 11, 2022 at 6:53 AM Martin Kepplinger <martin.kepplinger@puri.sm> wrote: > > Am Dienstag, dem 26.04.2022 um 12:43 +0200 schrieb Lucas Stach: > > Am Dienstag, dem 26.04.2022 um 09:38 +0200 schrieb Martin Kepplinger: > > > Am Montag, dem 25.04.2022 um 17:34 +0200 schrieb Lucas Stach: > > > > Hi Martin, > > > > > > > > Am Montag, dem 25.04.2022 um 17:22 +0200 schrieb Martin > > > > Kepplinger: > > > > > Am Dienstag, dem 25.01.2022 um 11:11 -0600 schrieb Adam Ford: > > > > > > With the Hantro G1 and G2 now setup to run independently, > > > > > > update > > > > > > the device tree to allow both to operate. This requires the > > > > > > vpu-blk-ctrl node to be configured. Since vpu-blk-ctrl needs > > > > > > certain clock enabled to handle the gating of the G1 and G2 > > > > > > fuses, the clock-parents and clock-rates for the various > > > > > > VPU's > > > > > > to be moved into the pgc_vpu because they cannot get re- > > > > > > parented > > > > > > once enabled, and the pgc_vpu is the highest in the chain. > > > > > > > > > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > > > > > Reported-by: kernel test robot <lkp@intel.com> > > > > > > Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > > > > > > > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > > > b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > > > index 2df2510d0118..549b2440f55d 100644 > > > > > > --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > > > @@ -737,7 +737,21 @@ pgc_gpu: power-domain@5 { > > > > > > pgc_vpu: power- > > > > > > domain@6 { > > > > > > #power- > > > > > > domain- > > > > > > cells = > > > > > > <0>; > > > > > > reg = > > > > > > <IMX8M_POWER_DOMAIN_VPU>; > > > > > > - clocks = > > > > > > <&clk > > > > > > IMX8MQ_CLK_VPU_DEC_ROOT>; > > > > > > + clocks = > > > > > > <&clk > > > > > > IMX8MQ_CLK_VPU_DEC_ROOT>, > > > > > > + > > > > > > <&clk > > > > > > IMX8MQ_CLK_VPU_G1_ROOT>, > > > > > > + > > > > > > <&clk > > > > > > IMX8MQ_CLK_VPU_G2_ROOT>; > > > > > > + assigned- > > > > > > clocks = > > > > > > <&clk IMX8MQ_CLK_VPU_G1>, > > > > > > + > > > > > > > > > > > > <&clk IMX8MQ_CLK_VPU_G2>, > > > > > > + > > > > > > > > > > > > <&clk IMX8MQ_CLK_VPU_BUS>, > > > > > > + > > > > > > > > > > > > <&clk IMX8MQ_VPU_PLL_BYPASS>; > > > > > > + assigned- > > > > > > clock- > > > > > > parents = <&clk IMX8MQ_VPU_PLL_OUT>, > > > > > > + > > > > > > > > > > > > > > > > > > <&clk IMX8MQ_VPU_PLL_OUT>, > > > > > > + > > > > > > > > > > > > > > > > > > <&clk IMX8MQ_SYS1_PLL_800M>, > > > > > > + > > > > > > > > > > > > > > > > > > <&clk IMX8MQ_VPU_PLL>; > > > > > > + assigned- > > > > > > clock- > > > > > > rates > > > > > > = <600000000>, > > > > > > + > > > > > > > > > > > > > > > > > > <600000000>, > > > > > > + > > > > > > > > > > > > > > > > > > <800000000>, > > > > > > + > > > > > > > > > > > > > > > > > > <0>; > > > > > > }; > > > > > > > > > > > > pgc_disp: > > > > > > power-domain@7 > > > > > > { > > > > > > @@ -1457,30 +1471,31 @@ usb3_phy1: usb-phy@382f0040 { > > > > > > status = "disabled"; > > > > > > }; > > > > > > > > > > > > - vpu: video-codec@38300000 { > > > > > > - compatible = "nxp,imx8mq-vpu"; > > > > > > - reg = <0x38300000 0x10000>, > > > > > > - <0x38310000 0x10000>, > > > > > > - <0x38320000 0x10000>; > > > > > > - reg-names = "g1", "g2", "ctrl"; > > > > > > - interrupts = <GIC_SPI 7 > > > > > > IRQ_TYPE_LEVEL_HIGH>, > > > > > > - <GIC_SPI 8 > > > > > > IRQ_TYPE_LEVEL_HIGH>; > > > > > > - interrupt-names = "g1", "g2"; > > > > > > + vpu_g1: video-codec@38300000 { > > > > > > + compatible = "nxp,imx8mq-vpu-g1"; > > > > > > + reg = <0x38300000 0x10000>; > > > > > > + interrupts = <GIC_SPI 7 > > > > > > IRQ_TYPE_LEVEL_HIGH>; > > > > > > + clocks = <&clk > > > > > > IMX8MQ_CLK_VPU_G1_ROOT>; > > > > > > + power-domains = <&vpu_blk_ctrl > > > > > > IMX8MQ_VPUBLK_PD_G1>; > > > > > > + }; > > > > > > + > > > > > > + vpu_g2: video-codec@38310000 { > > > > > > + compatible = "nxp,imx8mq-vpu-g2"; > > > > > > + reg = <0x38310000 0x10000>; > > > > > > + interrupts = <GIC_SPI 8 > > > > > > IRQ_TYPE_LEVEL_HIGH>; > > > > > > + clocks = <&clk > > > > > > IMX8MQ_CLK_VPU_G2_ROOT>; > > > > > > + power-domains = <&vpu_blk_ctrl > > > > > > IMX8MQ_VPUBLK_PD_G2>; > > > > > > + }; > > > > > > + > > > > > > + vpu_blk_ctrl: blk-ctrl@38320000 { > > > > > > + compatible = "fsl,imx8mq-vpu-blk- > > > > > > ctrl"; > > > > > > + reg = <0x38320000 0x100>; > > > > > > + power-domains = <&pgc_vpu>, > > > > > > <&pgc_vpu>, > > > > > > <&pgc_vpu>; > > > > > > + power-domain-names = "bus", "g1", > > > > > > "g2"; > > > > > > clocks = <&clk > > > > > > IMX8MQ_CLK_VPU_G1_ROOT>, > > > > > > - <&clk > > > > > > IMX8MQ_CLK_VPU_G2_ROOT>, > > > > > > - <&clk > > > > > > IMX8MQ_CLK_VPU_DEC_ROOT>; > > > > > > - clock-names = "g1", "g2", "bus"; > > > > > > - assigned-clocks = <&clk > > > > > > IMX8MQ_CLK_VPU_G1>, > > > > > > - <&clk > > > > > > IMX8MQ_CLK_VPU_G2>, > > > > > > - <&clk > > > > > > IMX8MQ_CLK_VPU_BUS>, > > > > > > - <&clk > > > > > > IMX8MQ_VPU_PLL_BYPASS>; > > > > > > - assigned-clock-parents = <&clk > > > > > > IMX8MQ_VPU_PLL_OUT>, > > > > > > - <&clk > > > > > > IMX8MQ_VPU_PLL_OUT>, > > > > > > - <&clk > > > > > > IMX8MQ_SYS1_PLL_800M>, > > > > > > - <&clk > > > > > > IMX8MQ_VPU_PLL>; > > > > > > - assigned-clock-rates = <600000000>, > > > > > > <600000000>, > > > > > > - <800000000>, > > > > > > <0>; > > > > > > - power-domains = <&pgc_vpu>; > > > > > > + <&clk > > > > > > IMX8MQ_CLK_VPU_G2_ROOT>; > > > > > > + clock-names = "g1", "g2"; > > > > > > + #power-domain-cells = <1>; > > > > > > }; > > > > > > > > > > > > pcie0: pcie@33800000 { > > > > > > > > > > With this update, when testing suspend to ram on imx8mq, I get: > > > > > > > > > > buck4: failed to disable: -ETIMEDOUT > > > > > > > > > > where buck4 is power-supply of pgc_vpu. And thus the transition > > > > > to > > > > > suspend (and resuming) fails. > > > > > > > > > > Have you tested system suspend after the imx8m-blk-ctrl update > > > > > on > > > > > imx8mq? > > > > > > > > I haven't tested system suspend, don't know if anyone else did. > > > > However > > > > I guess that this is just uncovering a preexisting issue in the > > > > system > > > > suspend sequencing, which you would also hit if the video > > > > decoders > > > > were > > > > active at system suspend time. > > > > > > > > My guess is that the regulator disable fails, due to the power > > > > domains > > > > being disabled quite late in the suspend sequence, where i2c > > > > communication with the PMIC is no longer possible due to i2c > > > > being > > > > suspended already or something like that. Maybe you can dig in a > > > > bit > > > > on > > > > the actual sequence on your system and we can see how we can > > > > rework > > > > things to suspend the power domains at a time where communication > > > > with > > > > the PMIC is still possible? > > > > > > What exactly would you like to see? Here's all gpcv2 regulators > > > disabling on suspend. (gpu (domain 5) is disabled by runtime pm > > > often): > > > > > > [ 47.138700] imx-pgc imx-pgc-domain.5: disabling regulator > > > [ 47.298071] Freezing user space processes ... (elapsed 0.008 > > > seconds) done. > > > [ 47.313432] OOM killer disabled. > > > [ 47.316670] Freezing remaining freezable tasks ... (elapsed > > > 2.221 > > > seconds) done. > > > [ 49.672052] imx8m-blk-ctrl 38320000.blk-ctrl: > > > imx8m_blk_ctrl_suspend > > > start > > > [ 49.704417] imx-pgc imx-pgc-domain.0: disabling regulator > > > [ 49.711114] imx-pgc imx-pgc-domain.6: disabling regulator > > > [ 49.819064] buck4: failed to disable: -ETIMEDOUT > > > > > > The stack looks pretty much the same for all of them, from > > > pm_suspend() > > > over genpd_suspend_noiry(). > > > > So the GPU domain is already suspended before the system suspend, > > probably due to short runtime PM timeouts. > > > > Can you please check at which point the i2c subsystem is suspended? I > > think we are already past that point when running the PM domain > > suspend > > from a _noirq callback. I'll take a look on how we can properly > > change > > this ordering. > > > > Regards, > > Lucas > > > > hi Lucas, sorry for not following up on this for so long. This fixes > suspend/resume for me: > > https://lore.kernel.org/linux-arm-kernel/20220711094549.3445566-1-martin.kepplinger@puri.sm/T/#t > > thank you for you help so far, > Thanks a lot for keeping us posted. The fix for suspend/resume looks great! Ezequiel
Both the i.MX8MQ and i.MX8MM have G1 and G2 decoders. The two decoders are similar, but the imx8mm lacks the post-processor, so they will have distinct compatible flags. From what I can tell, the G2 decoder wasn't working, so splitting the i.MX8MQ VPU into G1 and G2 makes it easier to control them independently since the TRM of both the i.MX8MQ and i.MX8MM list them as distinct IP blocks. This also allowed G2 to become available. With them being split, the power-domain can shift to the vpu-blk-ctrl which is available on both i.MX8MQ and i.MX8MM, but some of bits are different, so they'll have separate bindings. Lastly, with the G1 and G2 operational, enable the i.MX8MM. On the i.MX8MM, the clock speed of 600MHz was chosen to match the default of the kernel repo from NXP and can be overwritten by board files for anyone who under/over volts the power rail. There seems to be some disagreement between the TRM and the Datasheet for the imx8mq as to whether the speed should be 300MHz (TRM) or 600MHz (datasheet), so feedback from NXP would be very much appreciated. The repo used as the starting point was: 5.17-rc1 Fluster was run on both i.MX8MM and i.MX8MQ At 600 MHz, the i.MX8MM had the following: ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0 Ran 55/61 tests successfully in 8.299 secs ./fluster.py run -dGStreamer-H.264-V4L2SL-Gst1.0 Ran 90/135 tests successfully in 71.200 secs ./fluster.py run -d GStreamer-VP9-V4L2SL-Gst1.0 Ran 139/303 tests successfully in 218.079 secs The i.MX8MQ had the following: ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0 Ran 55/61 tests successfully in 7.732 secs ./fluster.py run -dGStreamer-H.264-V4L2SL-Gst1.0 Ran 90/135 tests successfully in 58.558 secs ./fluster.py run -d GStreamer-VP9-V4L2SL-Gst1.0 Ran 144/303 tests successfully in 271.373 secs V4: Add a comment and a notice based on feedback from Ezequiel Garcia if the older compatible flag is used. Add new patch which removes a reference to vpu from imx8mq-tqma8mq because the VPU is enabled by default and it's not needed. Without this change, its device tree would fail to build. V3: Rebase on 5.17-RC1. Remove imx8mm-vpu-g2 since it's identical to imx8mq-vpu-g2 Remove unnecessary examples in YAML files. Remove some unused variables. V2: Remove references to legacy dt-binding from YAML, but keep it in the driver so older device trees can still be used. Fix typos in YAML Remove reg-names, interrupt-names, and clock-names from YAML, since each node will only have one of each, they're not necessary Add Fluster scores to cover letter for i.MX8MQ Adam Ford (8): arm64: dts: imx8mq-tqma8mq: Remove redundant vpu reference dt-bindings: media: nxp, imx8mq-vpu: Split G1 and G2 nodes media: hantro: Allow i.MX8MQ G1 and G2 to run independently arm64: dts: imx8mq: Enable both G1 and G2 VPU's with vpu-blk-ctrl arm64: dts: imx8mm: Fix VPU Hanging dt-bindings: media: nxp, imx8mq-vpu: Add support for G1 on imx8mm media: hantro: Add support for i.MX8MM Hantro-G1 arm64: dts: imx8mm: Enable Hantro G1 and G2 video decoders Lucas Stach (3): dt-bindings: power: imx8mq: add defines for VPU blk-ctrl domains dt-bindings: soc: add binding for i.MX8MQ VPU blk-ctrl soc: imx: imx8m-blk-ctrl: add i.MX8MQ VPU blk-ctrl .../bindings/media/nxp,imx8mq-vpu.yaml | 68 ++++++++---------- .../soc/imx/fsl,imx8mq-vpu-blk-ctrl.yaml | 71 +++++++++++++++++++ arch/arm64/boot/dts/freescale/imx8mm.dtsi | 23 +++++- .../boot/dts/freescale/imx8mq-tqma8mq.dtsi | 4 -- arch/arm64/boot/dts/freescale/imx8mq.dtsi | 63 +++++++++------- drivers/soc/imx/imx8m-blk-ctrl.c | 66 +++++++++++++++++ drivers/staging/media/hantro/hantro_drv.c | 11 +++ drivers/staging/media/hantro/hantro_hw.h | 2 + drivers/staging/media/hantro/imx8m_vpu_hw.c | 62 ++++++++++++---- include/dt-bindings/power/imx8mq-power.h | 3 + 10 files changed, 292 insertions(+), 81 deletions(-) create mode 100644 Documentation/devicetree/bindings/soc/imx/fsl,imx8mq-vpu-blk-ctrl.yaml base-commit: e783362eb54cd99b2cac8b3a9aeac942e6f6ac07