| Message ID | 20210910202640.980366-15-l.stach@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i.MX8MM GPC improvements and BLK_CTRL driver | expand |
On Fri, Sep 10, 2021 at 1:26 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > Add the DT node for the GPC, including all the PGC power domains, > some of them are not fully functional yet, as they require interaction > with the blk-ctrls to properly power up/down the peripherals. > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > --- > arch/arm64/boot/dts/freescale/imx8mm.dtsi | 107 ++++++++++++++++++++++ > 1 file changed, 107 insertions(+) > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > index e7648c3b8390..3922f26f8fd4 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > @@ -7,6 +7,8 @@ > #include <dt-bindings/gpio/gpio.h> > #include <dt-bindings/input/input.h> > #include <dt-bindings/interrupt-controller/arm-gic.h> > +#include <dt-bindings/power/imx8mm-power.h> > +#include <dt-bindings/reset/imx8mq-reset.h> > #include <dt-bindings/thermal/thermal.h> > > #include "imx8mm-pinfunc.h" > @@ -609,6 +611,111 @@ src: reset-controller@30390000 { > interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>; > #reset-cells = <1>; > }; > + > + gpc: gpc@303a0000 { > + compatible = "fsl,imx8mm-gpc"; > + reg = <0x303a0000 0x10000>; > + interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-parent = <&gic>; > + interrupt-controller; > + #interrupt-cells = <3>; > + > + pgc { > + #address-cells = <1>; > + #size-cells = <0>; > + > + pgc_hsiomix: power-domain@0 { > + #power-domain-cells = <0>; > + reg = <IMX8MM_POWER_DOMAIN_HSIOMIX>; > + clocks = <&clk IMX8MM_CLK_USB_BUS>; > + assigned-clocks = <&clk IMX8MM_CLK_USB_BUS>; > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_500M>; > + }; > + > + pgc_pcie: power-domain@1 { > + #power-domain-cells = <0>; > + reg = <IMX8MM_POWER_DOMAIN_PCIE>; > + power-domains = <&pgc_hsiomix>; > + clocks = <&clk IMX8MM_CLK_PCIE1_ROOT>; > + }; > + > + pgc_otg1: power-domain@2 { > + #power-domain-cells = <0>; > + reg = <IMX8MM_POWER_DOMAIN_OTG1>; > + power-domains = <&pgc_hsiomix>; > + }; > + > + pgc_otg2: power-domain@3 { > + #power-domain-cells = <0>; > + reg = <IMX8MM_POWER_DOMAIN_OTG2>; > + power-domains = <&pgc_hsiomix>; > + }; > + > + pgc_gpumix: power-domain@4 { > + #power-domain-cells = <0>; > + reg = <IMX8MM_POWER_DOMAIN_GPUMIX>; > + clocks = <&clk IMX8MM_CLK_GPU_BUS_ROOT>, > + <&clk IMX8MM_CLK_GPU_AHB>; > + assigned-clocks = <&clk IMX8MM_CLK_GPU_AXI>, > + <&clk IMX8MM_CLK_GPU_AHB>; > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL1_800M>, > + <&clk IMX8MM_SYS_PLL1_800M>; > + assigned-clock-rates = <800000000>, <400000000>; > + }; > + > + pgc_gpu: power-domain@5 { > + #power-domain-cells = <0>; > + reg = <IMX8MM_POWER_DOMAIN_GPU>; > + clocks = <&clk IMX8MM_CLK_GPU_AHB>, > + <&clk IMX8MM_CLK_GPU_BUS_ROOT>, > + <&clk IMX8MM_CLK_GPU2D_ROOT>, > + <&clk IMX8MM_CLK_GPU3D_ROOT>; > + resets = <&src IMX8MQ_RESET_GPU_RESET>; > + power-domains = <&pgc_gpumix>; > + }; > + > + pgc_vpumix: power-domain@6 { > + #power-domain-cells = <0>; > + reg = <IMX8MM_POWER_DOMAIN_VPUMIX>; > + 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 { > + #power-domain-cells = <0>; > + reg = <IMX8MM_POWER_DOMAIN_VPUG1>; > + }; > + > + pgc_vpu_g2: power-domain@8 { > + #power-domain-cells = <0>; > + reg = <IMX8MM_POWER_DOMAIN_VPUG2>; > + }; > + > + pgc_vpu_h1: power-domain@9 { > + #power-domain-cells = <0>; > + reg = <IMX8MM_POWER_DOMAIN_VPUH1>; > + }; > + > + pgc_dispmix: power-domain@10 { > + #power-domain-cells = <0>; > + reg = <IMX8MM_POWER_DOMAIN_DISPMIX>; > + clocks = <&clk IMX8MM_CLK_DISP_APB_ROOT>, > + <&clk IMX8MM_CLK_DISP_AXI_ROOT>; > + assigned-clocks = <&clk IMX8MM_CLK_DISP_AXI>, > + <&clk IMX8MM_CLK_DISP_APB>; > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_1000M>, > + <&clk IMX8MM_SYS_PLL1_800M>; > + assigned-clock-rates = <500000000>, <200000000>; > + }; > + > + pgc_mipi: power-domain@11 { > + #power-domain-cells = <0>; > + reg = <IMX8MM_POWER_DOMAIN_MIPI>; > + }; > + }; > + }; > }; > > aips2: bus@30400000 { > -- > 2.30.2 > Lucas, I've been using your 'i.MX8MM GPC improvements and BLK_CTRL driver' series for imx8mm-venice* and imx8mn-venice* boards. Thank you for this work and I hope to see it merged soon! I have found that on the imx8mm-venice-gw7901 board which does not use MIPI and thus does not connect VDD_MIPI_1P8, VDD_MIPI_1P2, VDD_MIPI_0P9, MIPI_VREG_CAP pins on the IMX8MM hangs with this particular patch. If I comment out the pgc_mipi domain and subsequent disp_blk_ctrl node from a later patch it resolves the hang. Is this behavior expected and what would your recommendation be to work around it? Best regards, Tim
Hi Tim, Am Freitag, dem 01.10.2021 um 16:07 -0700 schrieb Tim Harvey: > On Fri, Sep 10, 2021 at 1:26 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > Add the DT node for the GPC, including all the PGC power domains, > > some of them are not fully functional yet, as they require interaction > > with the blk-ctrls to properly power up/down the peripherals. > > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > > --- > > arch/arm64/boot/dts/freescale/imx8mm.dtsi | 107 ++++++++++++++++++++++ > > 1 file changed, 107 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > index e7648c3b8390..3922f26f8fd4 100644 > > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > @@ -7,6 +7,8 @@ > > #include <dt-bindings/gpio/gpio.h> > > #include <dt-bindings/input/input.h> > > #include <dt-bindings/interrupt-controller/arm-gic.h> > > +#include <dt-bindings/power/imx8mm-power.h> > > +#include <dt-bindings/reset/imx8mq-reset.h> > > #include <dt-bindings/thermal/thermal.h> > > > > #include "imx8mm-pinfunc.h" > > @@ -609,6 +611,111 @@ src: reset-controller@30390000 { > > interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>; > > #reset-cells = <1>; > > }; > > + > > + gpc: gpc@303a0000 { > > + compatible = "fsl,imx8mm-gpc"; > > + reg = <0x303a0000 0x10000>; > > + interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>; > > + interrupt-parent = <&gic>; > > + interrupt-controller; > > + #interrupt-cells = <3>; > > + > > + pgc { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + pgc_hsiomix: power-domain@0 { > > + #power-domain-cells = <0>; > > + reg = <IMX8MM_POWER_DOMAIN_HSIOMIX>; > > + clocks = <&clk IMX8MM_CLK_USB_BUS>; > > + assigned-clocks = <&clk IMX8MM_CLK_USB_BUS>; > > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_500M>; > > + }; > > + > > + pgc_pcie: power-domain@1 { > > + #power-domain-cells = <0>; > > + reg = <IMX8MM_POWER_DOMAIN_PCIE>; > > + power-domains = <&pgc_hsiomix>; > > + clocks = <&clk IMX8MM_CLK_PCIE1_ROOT>; > > + }; > > + > > + pgc_otg1: power-domain@2 { > > + #power-domain-cells = <0>; > > + reg = <IMX8MM_POWER_DOMAIN_OTG1>; > > + power-domains = <&pgc_hsiomix>; > > + }; > > + > > + pgc_otg2: power-domain@3 { > > + #power-domain-cells = <0>; > > + reg = <IMX8MM_POWER_DOMAIN_OTG2>; > > + power-domains = <&pgc_hsiomix>; > > + }; > > + > > + pgc_gpumix: power-domain@4 { > > + #power-domain-cells = <0>; > > + reg = <IMX8MM_POWER_DOMAIN_GPUMIX>; > > + clocks = <&clk IMX8MM_CLK_GPU_BUS_ROOT>, > > + <&clk IMX8MM_CLK_GPU_AHB>; > > + assigned-clocks = <&clk IMX8MM_CLK_GPU_AXI>, > > + <&clk IMX8MM_CLK_GPU_AHB>; > > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL1_800M>, > > + <&clk IMX8MM_SYS_PLL1_800M>; > > + assigned-clock-rates = <800000000>, <400000000>; > > + }; > > + > > + pgc_gpu: power-domain@5 { > > + #power-domain-cells = <0>; > > + reg = <IMX8MM_POWER_DOMAIN_GPU>; > > + clocks = <&clk IMX8MM_CLK_GPU_AHB>, > > + <&clk IMX8MM_CLK_GPU_BUS_ROOT>, > > + <&clk IMX8MM_CLK_GPU2D_ROOT>, > > + <&clk IMX8MM_CLK_GPU3D_ROOT>; > > + resets = <&src IMX8MQ_RESET_GPU_RESET>; > > + power-domains = <&pgc_gpumix>; > > + }; > > + > > + pgc_vpumix: power-domain@6 { > > + #power-domain-cells = <0>; > > + reg = <IMX8MM_POWER_DOMAIN_VPUMIX>; > > + 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 { > > + #power-domain-cells = <0>; > > + reg = <IMX8MM_POWER_DOMAIN_VPUG1>; > > + }; > > + > > + pgc_vpu_g2: power-domain@8 { > > + #power-domain-cells = <0>; > > + reg = <IMX8MM_POWER_DOMAIN_VPUG2>; > > + }; > > + > > + pgc_vpu_h1: power-domain@9 { > > + #power-domain-cells = <0>; > > + reg = <IMX8MM_POWER_DOMAIN_VPUH1>; > > + }; > > + > > + pgc_dispmix: power-domain@10 { > > + #power-domain-cells = <0>; > > + reg = <IMX8MM_POWER_DOMAIN_DISPMIX>; > > + clocks = <&clk IMX8MM_CLK_DISP_APB_ROOT>, > > + <&clk IMX8MM_CLK_DISP_AXI_ROOT>; > > + assigned-clocks = <&clk IMX8MM_CLK_DISP_AXI>, > > + <&clk IMX8MM_CLK_DISP_APB>; > > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_1000M>, > > + <&clk IMX8MM_SYS_PLL1_800M>; > > + assigned-clock-rates = <500000000>, <200000000>; > > + }; > > + > > + pgc_mipi: power-domain@11 { > > + #power-domain-cells = <0>; > > + reg = <IMX8MM_POWER_DOMAIN_MIPI>; > > + }; > > + }; > > + }; > > }; > > > > aips2: bus@30400000 { > > -- > > 2.30.2 > > > > Lucas, > > I've been using your 'i.MX8MM GPC improvements and BLK_CTRL driver' > series for imx8mm-venice* and imx8mn-venice* boards. Thank you for > this work and I hope to see it merged soon! > > I have found that on the imx8mm-venice-gw7901 board which does not use > MIPI and thus does not connect VDD_MIPI_1P8, VDD_MIPI_1P2, > VDD_MIPI_0P9, MIPI_VREG_CAP pins on the IMX8MM hangs with this > particular patch. If I comment out the pgc_mipi domain and subsequent > disp_blk_ctrl node from a later patch it resolves the hang. Is this > behavior expected and what would your recommendation be to work around > it? No, this isn't expected. If there are no active devices in the MIPI domain, the power domain should not be touched, as we treat all of them as disabled initially. If we don't touch the domain I would expect that the power supply not being present shouldn't be an issue. Can you check if something in your system causes this power domain to be powered up? Easiest way is probably to sprinkle a printk("%s\n, genpd->name) in both imx8m_blk_ctrl_power_on() and imx_gpc_power_on(). Regards, Lucas
On Fri, Oct 1, 2021 at 4:20 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > Hi Tim, > > Am Freitag, dem 01.10.2021 um 16:07 -0700 schrieb Tim Harvey: > > On Fri, Sep 10, 2021 at 1:26 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > > > Add the DT node for the GPC, including all the PGC power domains, > > > some of them are not fully functional yet, as they require interaction > > > with the blk-ctrls to properly power up/down the peripherals. > > > > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > > > --- > > > arch/arm64/boot/dts/freescale/imx8mm.dtsi | 107 ++++++++++++++++++++++ > > > 1 file changed, 107 insertions(+) > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > > index e7648c3b8390..3922f26f8fd4 100644 > > > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > > @@ -7,6 +7,8 @@ > > > #include <dt-bindings/gpio/gpio.h> > > > #include <dt-bindings/input/input.h> > > > #include <dt-bindings/interrupt-controller/arm-gic.h> > > > +#include <dt-bindings/power/imx8mm-power.h> > > > +#include <dt-bindings/reset/imx8mq-reset.h> > > > #include <dt-bindings/thermal/thermal.h> > > > > > > #include "imx8mm-pinfunc.h" > > > @@ -609,6 +611,111 @@ src: reset-controller@30390000 { > > > interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>; > > > #reset-cells = <1>; > > > }; > > > + > > > + gpc: gpc@303a0000 { > > > + compatible = "fsl,imx8mm-gpc"; > > > + reg = <0x303a0000 0x10000>; > > > + interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>; > > > + interrupt-parent = <&gic>; > > > + interrupt-controller; > > > + #interrupt-cells = <3>; > > > + > > > + pgc { > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + > > > + pgc_hsiomix: power-domain@0 { > > > + #power-domain-cells = <0>; > > > + reg = <IMX8MM_POWER_DOMAIN_HSIOMIX>; > > > + clocks = <&clk IMX8MM_CLK_USB_BUS>; > > > + assigned-clocks = <&clk IMX8MM_CLK_USB_BUS>; > > > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_500M>; > > > + }; > > > + > > > + pgc_pcie: power-domain@1 { > > > + #power-domain-cells = <0>; > > > + reg = <IMX8MM_POWER_DOMAIN_PCIE>; > > > + power-domains = <&pgc_hsiomix>; > > > + clocks = <&clk IMX8MM_CLK_PCIE1_ROOT>; > > > + }; > > > + > > > + pgc_otg1: power-domain@2 { > > > + #power-domain-cells = <0>; > > > + reg = <IMX8MM_POWER_DOMAIN_OTG1>; > > > + power-domains = <&pgc_hsiomix>; > > > + }; > > > + > > > + pgc_otg2: power-domain@3 { > > > + #power-domain-cells = <0>; > > > + reg = <IMX8MM_POWER_DOMAIN_OTG2>; > > > + power-domains = <&pgc_hsiomix>; > > > + }; > > > + > > > + pgc_gpumix: power-domain@4 { > > > + #power-domain-cells = <0>; > > > + reg = <IMX8MM_POWER_DOMAIN_GPUMIX>; > > > + clocks = <&clk IMX8MM_CLK_GPU_BUS_ROOT>, > > > + <&clk IMX8MM_CLK_GPU_AHB>; > > > + assigned-clocks = <&clk IMX8MM_CLK_GPU_AXI>, > > > + <&clk IMX8MM_CLK_GPU_AHB>; > > > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL1_800M>, > > > + <&clk IMX8MM_SYS_PLL1_800M>; > > > + assigned-clock-rates = <800000000>, <400000000>; > > > + }; > > > + > > > + pgc_gpu: power-domain@5 { > > > + #power-domain-cells = <0>; > > > + reg = <IMX8MM_POWER_DOMAIN_GPU>; > > > + clocks = <&clk IMX8MM_CLK_GPU_AHB>, > > > + <&clk IMX8MM_CLK_GPU_BUS_ROOT>, > > > + <&clk IMX8MM_CLK_GPU2D_ROOT>, > > > + <&clk IMX8MM_CLK_GPU3D_ROOT>; > > > + resets = <&src IMX8MQ_RESET_GPU_RESET>; > > > + power-domains = <&pgc_gpumix>; > > > + }; > > > + > > > + pgc_vpumix: power-domain@6 { > > > + #power-domain-cells = <0>; > > > + reg = <IMX8MM_POWER_DOMAIN_VPUMIX>; > > > + 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 { > > > + #power-domain-cells = <0>; > > > + reg = <IMX8MM_POWER_DOMAIN_VPUG1>; > > > + }; > > > + > > > + pgc_vpu_g2: power-domain@8 { > > > + #power-domain-cells = <0>; > > > + reg = <IMX8MM_POWER_DOMAIN_VPUG2>; > > > + }; > > > + > > > + pgc_vpu_h1: power-domain@9 { > > > + #power-domain-cells = <0>; > > > + reg = <IMX8MM_POWER_DOMAIN_VPUH1>; > > > + }; > > > + > > > + pgc_dispmix: power-domain@10 { > > > + #power-domain-cells = <0>; > > > + reg = <IMX8MM_POWER_DOMAIN_DISPMIX>; > > > + clocks = <&clk IMX8MM_CLK_DISP_APB_ROOT>, > > > + <&clk IMX8MM_CLK_DISP_AXI_ROOT>; > > > + assigned-clocks = <&clk IMX8MM_CLK_DISP_AXI>, > > > + <&clk IMX8MM_CLK_DISP_APB>; > > > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_1000M>, > > > + <&clk IMX8MM_SYS_PLL1_800M>; > > > + assigned-clock-rates = <500000000>, <200000000>; > > > + }; > > > + > > > + pgc_mipi: power-domain@11 { > > > + #power-domain-cells = <0>; > > > + reg = <IMX8MM_POWER_DOMAIN_MIPI>; > > > + }; > > > + }; > > > + }; > > > }; > > > > > > aips2: bus@30400000 { > > > -- > > > 2.30.2 > > > > > > > Lucas, > > > > I've been using your 'i.MX8MM GPC improvements and BLK_CTRL driver' > > series for imx8mm-venice* and imx8mn-venice* boards. Thank you for > > this work and I hope to see it merged soon! > > > > I have found that on the imx8mm-venice-gw7901 board which does not use > > MIPI and thus does not connect VDD_MIPI_1P8, VDD_MIPI_1P2, > > VDD_MIPI_0P9, MIPI_VREG_CAP pins on the IMX8MM hangs with this > > particular patch. If I comment out the pgc_mipi domain and subsequent > > disp_blk_ctrl node from a later patch it resolves the hang. Is this > > behavior expected and what would your recommendation be to work around > > it? > > No, this isn't expected. If there are no active devices in the MIPI > domain, the power domain should not be touched, as we treat all of them > as disabled initially. If we don't touch the domain I would expect that > the power supply not being present shouldn't be an issue. > > Can you check if something in your system causes this power domain to > be powered up? Easiest way is probably to sprinkle a > printk("%s\n, genpd->name) in both imx8m_blk_ctrl_power_on() and > imx_gpc_power_on(). > Lucas, Here's what I see before I hang (debug print on both power on/off followed by a msleep(1000) to make sure I see it before I hang): [ 0.518319] imx_pgc_power_up hsiomix [ 0.624031] imx_pgc_power_down hsiomix [ 0.731879] imx_pgc_power_up hsiomix [ 0.839906] imx_pgc_power_down hsiomix [ 0.947875] imx_pgc_power_up hsiomix [ 1.055859] imx_pgc_power_down hsiomix [ 1.057296] imx_pgc_power_up gpumix [ 1.167884] imx_pgc_power_down gpumix [ 0.518513] imx_pgc_power_up hsiomix [ 0.519933] imx_pgc_power_up gpumix Tim
On Fri, Oct 1, 2021 at 5:06 PM Tim Harvey <tharvey@gateworks.com> wrote: > > On Fri, Oct 1, 2021 at 4:20 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > Hi Tim, > > > > Am Freitag, dem 01.10.2021 um 16:07 -0700 schrieb Tim Harvey: > > > On Fri, Sep 10, 2021 at 1:26 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > > > > > Add the DT node for the GPC, including all the PGC power domains, > > > > some of them are not fully functional yet, as they require interaction > > > > with the blk-ctrls to properly power up/down the peripherals. > > > > > > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > > > > --- > > > > arch/arm64/boot/dts/freescale/imx8mm.dtsi | 107 ++++++++++++++++++++++ > > > > 1 file changed, 107 insertions(+) > > > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > > > index e7648c3b8390..3922f26f8fd4 100644 > > > > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > > > @@ -7,6 +7,8 @@ > > > > #include <dt-bindings/gpio/gpio.h> > > > > #include <dt-bindings/input/input.h> > > > > #include <dt-bindings/interrupt-controller/arm-gic.h> > > > > +#include <dt-bindings/power/imx8mm-power.h> > > > > +#include <dt-bindings/reset/imx8mq-reset.h> > > > > #include <dt-bindings/thermal/thermal.h> > > > > > > > > #include "imx8mm-pinfunc.h" > > > > @@ -609,6 +611,111 @@ src: reset-controller@30390000 { > > > > interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>; > > > > #reset-cells = <1>; > > > > }; > > > > + > > > > + gpc: gpc@303a0000 { > > > > + compatible = "fsl,imx8mm-gpc"; > > > > + reg = <0x303a0000 0x10000>; > > > > + interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>; > > > > + interrupt-parent = <&gic>; > > > > + interrupt-controller; > > > > + #interrupt-cells = <3>; > > > > + > > > > + pgc { > > > > + #address-cells = <1>; > > > > + #size-cells = <0>; > > > > + > > > > + pgc_hsiomix: power-domain@0 { > > > > + #power-domain-cells = <0>; > > > > + reg = <IMX8MM_POWER_DOMAIN_HSIOMIX>; > > > > + clocks = <&clk IMX8MM_CLK_USB_BUS>; > > > > + assigned-clocks = <&clk IMX8MM_CLK_USB_BUS>; > > > > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_500M>; > > > > + }; > > > > + > > > > + pgc_pcie: power-domain@1 { > > > > + #power-domain-cells = <0>; > > > > + reg = <IMX8MM_POWER_DOMAIN_PCIE>; > > > > + power-domains = <&pgc_hsiomix>; > > > > + clocks = <&clk IMX8MM_CLK_PCIE1_ROOT>; > > > > + }; > > > > + > > > > + pgc_otg1: power-domain@2 { > > > > + #power-domain-cells = <0>; > > > > + reg = <IMX8MM_POWER_DOMAIN_OTG1>; > > > > + power-domains = <&pgc_hsiomix>; > > > > + }; > > > > + > > > > + pgc_otg2: power-domain@3 { > > > > + #power-domain-cells = <0>; > > > > + reg = <IMX8MM_POWER_DOMAIN_OTG2>; > > > > + power-domains = <&pgc_hsiomix>; > > > > + }; > > > > + > > > > + pgc_gpumix: power-domain@4 { > > > > + #power-domain-cells = <0>; > > > > + reg = <IMX8MM_POWER_DOMAIN_GPUMIX>; > > > > + clocks = <&clk IMX8MM_CLK_GPU_BUS_ROOT>, > > > > + <&clk IMX8MM_CLK_GPU_AHB>; > > > > + assigned-clocks = <&clk IMX8MM_CLK_GPU_AXI>, > > > > + <&clk IMX8MM_CLK_GPU_AHB>; > > > > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL1_800M>, > > > > + <&clk IMX8MM_SYS_PLL1_800M>; > > > > + assigned-clock-rates = <800000000>, <400000000>; > > > > + }; > > > > + > > > > + pgc_gpu: power-domain@5 { > > > > + #power-domain-cells = <0>; > > > > + reg = <IMX8MM_POWER_DOMAIN_GPU>; > > > > + clocks = <&clk IMX8MM_CLK_GPU_AHB>, > > > > + <&clk IMX8MM_CLK_GPU_BUS_ROOT>, > > > > + <&clk IMX8MM_CLK_GPU2D_ROOT>, > > > > + <&clk IMX8MM_CLK_GPU3D_ROOT>; > > > > + resets = <&src IMX8MQ_RESET_GPU_RESET>; > > > > + power-domains = <&pgc_gpumix>; > > > > + }; > > > > + > > > > + pgc_vpumix: power-domain@6 { > > > > + #power-domain-cells = <0>; > > > > + reg = <IMX8MM_POWER_DOMAIN_VPUMIX>; > > > > + 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 { > > > > + #power-domain-cells = <0>; > > > > + reg = <IMX8MM_POWER_DOMAIN_VPUG1>; > > > > + }; > > > > + > > > > + pgc_vpu_g2: power-domain@8 { > > > > + #power-domain-cells = <0>; > > > > + reg = <IMX8MM_POWER_DOMAIN_VPUG2>; > > > > + }; > > > > + > > > > + pgc_vpu_h1: power-domain@9 { > > > > + #power-domain-cells = <0>; > > > > + reg = <IMX8MM_POWER_DOMAIN_VPUH1>; > > > > + }; > > > > + > > > > + pgc_dispmix: power-domain@10 { > > > > + #power-domain-cells = <0>; > > > > + reg = <IMX8MM_POWER_DOMAIN_DISPMIX>; > > > > + clocks = <&clk IMX8MM_CLK_DISP_APB_ROOT>, > > > > + <&clk IMX8MM_CLK_DISP_AXI_ROOT>; > > > > + assigned-clocks = <&clk IMX8MM_CLK_DISP_AXI>, > > > > + <&clk IMX8MM_CLK_DISP_APB>; > > > > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_1000M>, > > > > + <&clk IMX8MM_SYS_PLL1_800M>; > > > > + assigned-clock-rates = <500000000>, <200000000>; > > > > + }; > > > > + > > > > + pgc_mipi: power-domain@11 { > > > > + #power-domain-cells = <0>; > > > > + reg = <IMX8MM_POWER_DOMAIN_MIPI>; > > > > + }; > > > > + }; > > > > + }; > > > > }; > > > > > > > > aips2: bus@30400000 { > > > > -- > > > > 2.30.2 > > > > > > > > > > Lucas, > > > > > > I've been using your 'i.MX8MM GPC improvements and BLK_CTRL driver' > > > series for imx8mm-venice* and imx8mn-venice* boards. Thank you for > > > this work and I hope to see it merged soon! > > > > > > I have found that on the imx8mm-venice-gw7901 board which does not use > > > MIPI and thus does not connect VDD_MIPI_1P8, VDD_MIPI_1P2, > > > VDD_MIPI_0P9, MIPI_VREG_CAP pins on the IMX8MM hangs with this > > > particular patch. If I comment out the pgc_mipi domain and subsequent > > > disp_blk_ctrl node from a later patch it resolves the hang. Is this > > > behavior expected and what would your recommendation be to work around > > > it? > > > > No, this isn't expected. If there are no active devices in the MIPI > > domain, the power domain should not be touched, as we treat all of them > > as disabled initially. If we don't touch the domain I would expect that > > the power supply not being present shouldn't be an issue. > > > > Can you check if something in your system causes this power domain to > > be powered up? Easiest way is probably to sprinkle a > > printk("%s\n, genpd->name) in both imx8m_blk_ctrl_power_on() and > > imx_gpc_power_on(). > > > > Lucas, > > Here's what I see before I hang (debug print on both power on/off > followed by a msleep(1000) to make sure I see it before I hang): > [ 0.518319] imx_pgc_power_up hsiomix > [ 0.624031] imx_pgc_power_down hsiomix > [ 0.731879] imx_pgc_power_up hsiomix > [ 0.839906] imx_pgc_power_down hsiomix > [ 0.947875] imx_pgc_power_up hsiomix > [ 1.055859] imx_pgc_power_down hsiomix > [ 1.057296] imx_pgc_power_up gpumix > [ 1.167884] imx_pgc_power_down gpumix > [ 0.518513] imx_pgc_power_up hsiomix > [ 0.519933] imx_pgc_power_up gpumix > The board also has IMX8MM VDD_GPU pins not connected so it makes sense that we hang here I suppose. Yet if I add the folloiwng to imx8mm-venice-gw7901.dts it still tries to enable them and hangs: &gpu_2d { status = "disabled"; }; &gpu_3d { status = "disabled"; }; &vpu_blk_ctrl { status = "disabled"; }; Tim
Am Freitag, dem 01.10.2021 um 17:15 -0700 schrieb Tim Harvey: > On Fri, Oct 1, 2021 at 5:06 PM Tim Harvey <tharvey@gateworks.com> wrote: > > > > On Fri, Oct 1, 2021 at 4:20 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > > > Hi Tim, > > > > > > Am Freitag, dem 01.10.2021 um 16:07 -0700 schrieb Tim Harvey: > > > > On Fri, Sep 10, 2021 at 1:26 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > > > > > > > Add the DT node for the GPC, including all the PGC power domains, > > > > > some of them are not fully functional yet, as they require interaction > > > > > with the blk-ctrls to properly power up/down the peripherals. > > > > > > > > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > > > > > --- > > > > > arch/arm64/boot/dts/freescale/imx8mm.dtsi | 107 ++++++++++++++++++++++ > > > > > 1 file changed, 107 insertions(+) > > > > > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > > > > index e7648c3b8390..3922f26f8fd4 100644 > > > > > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > > > > @@ -7,6 +7,8 @@ > > > > > #include <dt-bindings/gpio/gpio.h> > > > > > #include <dt-bindings/input/input.h> > > > > > #include <dt-bindings/interrupt-controller/arm-gic.h> > > > > > +#include <dt-bindings/power/imx8mm-power.h> > > > > > +#include <dt-bindings/reset/imx8mq-reset.h> > > > > > #include <dt-bindings/thermal/thermal.h> > > > > > > > > > > #include "imx8mm-pinfunc.h" > > > > > @@ -609,6 +611,111 @@ src: reset-controller@30390000 { > > > > > interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>; > > > > > #reset-cells = <1>; > > > > > }; > > > > > + > > > > > + gpc: gpc@303a0000 { > > > > > + compatible = "fsl,imx8mm-gpc"; > > > > > + reg = <0x303a0000 0x10000>; > > > > > + interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>; > > > > > + interrupt-parent = <&gic>; > > > > > + interrupt-controller; > > > > > + #interrupt-cells = <3>; > > > > > + > > > > > + pgc { > > > > > + #address-cells = <1>; > > > > > + #size-cells = <0>; > > > > > + > > > > > + pgc_hsiomix: power-domain@0 { > > > > > + #power-domain-cells = <0>; > > > > > + reg = <IMX8MM_POWER_DOMAIN_HSIOMIX>; > > > > > + clocks = <&clk IMX8MM_CLK_USB_BUS>; > > > > > + assigned-clocks = <&clk IMX8MM_CLK_USB_BUS>; > > > > > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_500M>; > > > > > + }; > > > > > + > > > > > + pgc_pcie: power-domain@1 { > > > > > + #power-domain-cells = <0>; > > > > > + reg = <IMX8MM_POWER_DOMAIN_PCIE>; > > > > > + power-domains = <&pgc_hsiomix>; > > > > > + clocks = <&clk IMX8MM_CLK_PCIE1_ROOT>; > > > > > + }; > > > > > + > > > > > + pgc_otg1: power-domain@2 { > > > > > + #power-domain-cells = <0>; > > > > > + reg = <IMX8MM_POWER_DOMAIN_OTG1>; > > > > > + power-domains = <&pgc_hsiomix>; > > > > > + }; > > > > > + > > > > > + pgc_otg2: power-domain@3 { > > > > > + #power-domain-cells = <0>; > > > > > + reg = <IMX8MM_POWER_DOMAIN_OTG2>; > > > > > + power-domains = <&pgc_hsiomix>; > > > > > + }; > > > > > + > > > > > + pgc_gpumix: power-domain@4 { > > > > > + #power-domain-cells = <0>; > > > > > + reg = <IMX8MM_POWER_DOMAIN_GPUMIX>; > > > > > + clocks = <&clk IMX8MM_CLK_GPU_BUS_ROOT>, > > > > > + <&clk IMX8MM_CLK_GPU_AHB>; > > > > > + assigned-clocks = <&clk IMX8MM_CLK_GPU_AXI>, > > > > > + <&clk IMX8MM_CLK_GPU_AHB>; > > > > > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL1_800M>, > > > > > + <&clk IMX8MM_SYS_PLL1_800M>; > > > > > + assigned-clock-rates = <800000000>, <400000000>; > > > > > + }; > > > > > + > > > > > + pgc_gpu: power-domain@5 { > > > > > + #power-domain-cells = <0>; > > > > > + reg = <IMX8MM_POWER_DOMAIN_GPU>; > > > > > + clocks = <&clk IMX8MM_CLK_GPU_AHB>, > > > > > + <&clk IMX8MM_CLK_GPU_BUS_ROOT>, > > > > > + <&clk IMX8MM_CLK_GPU2D_ROOT>, > > > > > + <&clk IMX8MM_CLK_GPU3D_ROOT>; > > > > > + resets = <&src IMX8MQ_RESET_GPU_RESET>; > > > > > + power-domains = <&pgc_gpumix>; > > > > > + }; > > > > > + > > > > > + pgc_vpumix: power-domain@6 { > > > > > + #power-domain-cells = <0>; > > > > > + reg = <IMX8MM_POWER_DOMAIN_VPUMIX>; > > > > > + 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 { > > > > > + #power-domain-cells = <0>; > > > > > + reg = <IMX8MM_POWER_DOMAIN_VPUG1>; > > > > > + }; > > > > > + > > > > > + pgc_vpu_g2: power-domain@8 { > > > > > + #power-domain-cells = <0>; > > > > > + reg = <IMX8MM_POWER_DOMAIN_VPUG2>; > > > > > + }; > > > > > + > > > > > + pgc_vpu_h1: power-domain@9 { > > > > > + #power-domain-cells = <0>; > > > > > + reg = <IMX8MM_POWER_DOMAIN_VPUH1>; > > > > > + }; > > > > > + > > > > > + pgc_dispmix: power-domain@10 { > > > > > + #power-domain-cells = <0>; > > > > > + reg = <IMX8MM_POWER_DOMAIN_DISPMIX>; > > > > > + clocks = <&clk IMX8MM_CLK_DISP_APB_ROOT>, > > > > > + <&clk IMX8MM_CLK_DISP_AXI_ROOT>; > > > > > + assigned-clocks = <&clk IMX8MM_CLK_DISP_AXI>, > > > > > + <&clk IMX8MM_CLK_DISP_APB>; > > > > > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_1000M>, > > > > > + <&clk IMX8MM_SYS_PLL1_800M>; > > > > > + assigned-clock-rates = <500000000>, <200000000>; > > > > > + }; > > > > > + > > > > > + pgc_mipi: power-domain@11 { > > > > > + #power-domain-cells = <0>; > > > > > + reg = <IMX8MM_POWER_DOMAIN_MIPI>; > > > > > + }; > > > > > + }; > > > > > + }; > > > > > }; > > > > > > > > > > aips2: bus@30400000 { > > > > > -- > > > > > 2.30.2 > > > > > > > > > > > > > Lucas, > > > > > > > > I've been using your 'i.MX8MM GPC improvements and BLK_CTRL driver' > > > > series for imx8mm-venice* and imx8mn-venice* boards. Thank you for > > > > this work and I hope to see it merged soon! > > > > > > > > I have found that on the imx8mm-venice-gw7901 board which does not use > > > > MIPI and thus does not connect VDD_MIPI_1P8, VDD_MIPI_1P2, > > > > VDD_MIPI_0P9, MIPI_VREG_CAP pins on the IMX8MM hangs with this > > > > particular patch. If I comment out the pgc_mipi domain and subsequent > > > > disp_blk_ctrl node from a later patch it resolves the hang. Is this > > > > behavior expected and what would your recommendation be to work around > > > > it? > > > > > > No, this isn't expected. If there are no active devices in the MIPI > > > domain, the power domain should not be touched, as we treat all of them > > > as disabled initially. If we don't touch the domain I would expect that > > > the power supply not being present shouldn't be an issue. > > > > > > Can you check if something in your system causes this power domain to > > > be powered up? Easiest way is probably to sprinkle a > > > printk("%s\n, genpd->name) in both imx8m_blk_ctrl_power_on() and > > > imx_gpc_power_on(). > > > > > > > Lucas, > > > > Here's what I see before I hang (debug print on both power on/off > > followed by a msleep(1000) to make sure I see it before I hang): > > [ 0.518319] imx_pgc_power_up hsiomix > > [ 0.624031] imx_pgc_power_down hsiomix > > [ 0.731879] imx_pgc_power_up hsiomix > > [ 0.839906] imx_pgc_power_down hsiomix > > [ 0.947875] imx_pgc_power_up hsiomix > > [ 1.055859] imx_pgc_power_down hsiomix > > [ 1.057296] imx_pgc_power_up gpumix > > [ 1.167884] imx_pgc_power_down gpumix > > [ 0.518513] imx_pgc_power_up hsiomix > > [ 0.519933] imx_pgc_power_up gpumix > > > > The board also has IMX8MM VDD_GPU pins not connected so it makes sense > that we hang here I suppose. Yet if I add the folloiwng to > imx8mm-venice-gw7901.dts it still tries to enable them and hangs: > &gpu_2d { > status = "disabled"; > }; > > &gpu_3d { > status = "disabled"; > }; > > &vpu_blk_ctrl { > status = "disabled"; > }; The pgc_gpu is a "active" consumer of the pgc_gpumix domain while the driver gets probed, so the driver core will power up the gpumix domain for a moment during kernel init. To avoid this you must at least set the status of the pgc_gpu node to disabled. Regards, Lucas
'On Fri, Oct 1, 2021 at 5:25 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > Am Freitag, dem 01.10.2021 um 17:15 -0700 schrieb Tim Harvey: > > On Fri, Oct 1, 2021 at 5:06 PM Tim Harvey <tharvey@gateworks.com> wrote: > > > > > > On Fri, Oct 1, 2021 at 4:20 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > > > > > Hi Tim, > > > > > > > > Am Freitag, dem 01.10.2021 um 16:07 -0700 schrieb Tim Harvey: > > > > > On Fri, Sep 10, 2021 at 1:26 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > > > > > > > > > Add the DT node for the GPC, including all the PGC power domains, > > > > > > some of them are not fully functional yet, as they require interaction > > > > > > with the blk-ctrls to properly power up/down the peripherals. > > > > > > > > > > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > > > > > > --- > > > > > > arch/arm64/boot/dts/freescale/imx8mm.dtsi | 107 ++++++++++++++++++++++ > > > > > > 1 file changed, 107 insertions(+) > > > > > > > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > > > > > index e7648c3b8390..3922f26f8fd4 100644 > > > > > > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > > > > > @@ -7,6 +7,8 @@ > > > > > > #include <dt-bindings/gpio/gpio.h> > > > > > > #include <dt-bindings/input/input.h> > > > > > > #include <dt-bindings/interrupt-controller/arm-gic.h> > > > > > > +#include <dt-bindings/power/imx8mm-power.h> > > > > > > +#include <dt-bindings/reset/imx8mq-reset.h> > > > > > > #include <dt-bindings/thermal/thermal.h> > > > > > > > > > > > > #include "imx8mm-pinfunc.h" > > > > > > @@ -609,6 +611,111 @@ src: reset-controller@30390000 { > > > > > > interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>; > > > > > > #reset-cells = <1>; > > > > > > }; > > > > > > + > > > > > > + gpc: gpc@303a0000 { > > > > > > + compatible = "fsl,imx8mm-gpc"; > > > > > > + reg = <0x303a0000 0x10000>; > > > > > > + interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>; > > > > > > + interrupt-parent = <&gic>; > > > > > > + interrupt-controller; > > > > > > + #interrupt-cells = <3>; > > > > > > + > > > > > > + pgc { > > > > > > + #address-cells = <1>; > > > > > > + #size-cells = <0>; > > > > > > + > > > > > > + pgc_hsiomix: power-domain@0 { > > > > > > + #power-domain-cells = <0>; > > > > > > + reg = <IMX8MM_POWER_DOMAIN_HSIOMIX>; > > > > > > + clocks = <&clk IMX8MM_CLK_USB_BUS>; > > > > > > + assigned-clocks = <&clk IMX8MM_CLK_USB_BUS>; > > > > > > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_500M>; > > > > > > + }; > > > > > > + > > > > > > + pgc_pcie: power-domain@1 { > > > > > > + #power-domain-cells = <0>; > > > > > > + reg = <IMX8MM_POWER_DOMAIN_PCIE>; > > > > > > + power-domains = <&pgc_hsiomix>; > > > > > > + clocks = <&clk IMX8MM_CLK_PCIE1_ROOT>; > > > > > > + }; > > > > > > + > > > > > > + pgc_otg1: power-domain@2 { > > > > > > + #power-domain-cells = <0>; > > > > > > + reg = <IMX8MM_POWER_DOMAIN_OTG1>; > > > > > > + power-domains = <&pgc_hsiomix>; > > > > > > + }; > > > > > > + > > > > > > + pgc_otg2: power-domain@3 { > > > > > > + #power-domain-cells = <0>; > > > > > > + reg = <IMX8MM_POWER_DOMAIN_OTG2>; > > > > > > + power-domains = <&pgc_hsiomix>; > > > > > > + }; > > > > > > + > > > > > > + pgc_gpumix: power-domain@4 { > > > > > > + #power-domain-cells = <0>; > > > > > > + reg = <IMX8MM_POWER_DOMAIN_GPUMIX>; > > > > > > + clocks = <&clk IMX8MM_CLK_GPU_BUS_ROOT>, > > > > > > + <&clk IMX8MM_CLK_GPU_AHB>; > > > > > > + assigned-clocks = <&clk IMX8MM_CLK_GPU_AXI>, > > > > > > + <&clk IMX8MM_CLK_GPU_AHB>; > > > > > > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL1_800M>, > > > > > > + <&clk IMX8MM_SYS_PLL1_800M>; > > > > > > + assigned-clock-rates = <800000000>, <400000000>; > > > > > > + }; > > > > > > + > > > > > > + pgc_gpu: power-domain@5 { > > > > > > + #power-domain-cells = <0>; > > > > > > + reg = <IMX8MM_POWER_DOMAIN_GPU>; > > > > > > + clocks = <&clk IMX8MM_CLK_GPU_AHB>, > > > > > > + <&clk IMX8MM_CLK_GPU_BUS_ROOT>, > > > > > > + <&clk IMX8MM_CLK_GPU2D_ROOT>, > > > > > > + <&clk IMX8MM_CLK_GPU3D_ROOT>; > > > > > > + resets = <&src IMX8MQ_RESET_GPU_RESET>; > > > > > > + power-domains = <&pgc_gpumix>; > > > > > > + }; > > > > > > + > > > > > > + pgc_vpumix: power-domain@6 { > > > > > > + #power-domain-cells = <0>; > > > > > > + reg = <IMX8MM_POWER_DOMAIN_VPUMIX>; > > > > > > + 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 { > > > > > > + #power-domain-cells = <0>; > > > > > > + reg = <IMX8MM_POWER_DOMAIN_VPUG1>; > > > > > > + }; > > > > > > + > > > > > > + pgc_vpu_g2: power-domain@8 { > > > > > > + #power-domain-cells = <0>; > > > > > > + reg = <IMX8MM_POWER_DOMAIN_VPUG2>; > > > > > > + }; > > > > > > + > > > > > > + pgc_vpu_h1: power-domain@9 { > > > > > > + #power-domain-cells = <0>; > > > > > > + reg = <IMX8MM_POWER_DOMAIN_VPUH1>; > > > > > > + }; > > > > > > + > > > > > > + pgc_dispmix: power-domain@10 { > > > > > > + #power-domain-cells = <0>; > > > > > > + reg = <IMX8MM_POWER_DOMAIN_DISPMIX>; > > > > > > + clocks = <&clk IMX8MM_CLK_DISP_APB_ROOT>, > > > > > > + <&clk IMX8MM_CLK_DISP_AXI_ROOT>; > > > > > > + assigned-clocks = <&clk IMX8MM_CLK_DISP_AXI>, > > > > > > + <&clk IMX8MM_CLK_DISP_APB>; > > > > > > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_1000M>, > > > > > > + <&clk IMX8MM_SYS_PLL1_800M>; > > > > > > + assigned-clock-rates = <500000000>, <200000000>; > > > > > > + }; > > > > > > + > > > > > > + pgc_mipi: power-domain@11 { > > > > > > + #power-domain-cells = <0>; > > > > > > + reg = <IMX8MM_POWER_DOMAIN_MIPI>; > > > > > > + }; > > > > > > + }; > > > > > > + }; > > > > > > }; > > > > > > > > > > > > aips2: bus@30400000 { > > > > > > -- > > > > > > 2.30.2 > > > > > > > > > > > > > > > > Lucas, > > > > > > > > > > I've been using your 'i.MX8MM GPC improvements and BLK_CTRL driver' > > > > > series for imx8mm-venice* and imx8mn-venice* boards. Thank you for > > > > > this work and I hope to see it merged soon! > > > > > > > > > > I have found that on the imx8mm-venice-gw7901 board which does not use > > > > > MIPI and thus does not connect VDD_MIPI_1P8, VDD_MIPI_1P2, > > > > > VDD_MIPI_0P9, MIPI_VREG_CAP pins on the IMX8MM hangs with this > > > > > particular patch. If I comment out the pgc_mipi domain and subsequent > > > > > disp_blk_ctrl node from a later patch it resolves the hang. Is this > > > > > behavior expected and what would your recommendation be to work around > > > > > it? > > > > > > > > No, this isn't expected. If there are no active devices in the MIPI > > > > domain, the power domain should not be touched, as we treat all of them > > > > as disabled initially. If we don't touch the domain I would expect that > > > > the power supply not being present shouldn't be an issue. > > > > > > > > Can you check if something in your system causes this power domain to > > > > be powered up? Easiest way is probably to sprinkle a > > > > printk("%s\n, genpd->name) in both imx8m_blk_ctrl_power_on() and > > > > imx_gpc_power_on(). > > > > > > > > > > Lucas, > > > > > > Here's what I see before I hang (debug print on both power on/off > > > followed by a msleep(1000) to make sure I see it before I hang): > > > [ 0.518319] imx_pgc_power_up hsiomix > > > [ 0.624031] imx_pgc_power_down hsiomix > > > [ 0.731879] imx_pgc_power_up hsiomix > > > [ 0.839906] imx_pgc_power_down hsiomix > > > [ 0.947875] imx_pgc_power_up hsiomix > > > [ 1.055859] imx_pgc_power_down hsiomix > > > [ 1.057296] imx_pgc_power_up gpumix > > > [ 1.167884] imx_pgc_power_down gpumix > > > [ 0.518513] imx_pgc_power_up hsiomix > > > [ 0.519933] imx_pgc_power_up gpumix > > > > > > > The board also has IMX8MM VDD_GPU pins not connected so it makes sense > > that we hang here I suppose. Yet if I add the folloiwng to > > imx8mm-venice-gw7901.dts it still tries to enable them and hangs: > > &gpu_2d { > > status = "disabled"; > > }; > > > > &gpu_3d { > > status = "disabled"; > > }; > > > > &vpu_blk_ctrl { > > status = "disabled"; > > }; > > The pgc_gpu is a "active" consumer of the pgc_gpumix domain while the > driver gets probed, so the driver core will power up the gpumix domain > for a moment during kernel init. To avoid this you must at least set > the status of the pgc_gpu node to disabled. > I've tried that and it doesn't work either. &gpu_2d { status = "disabled"; }; &gpu_3d { status = "disabled"; }; &vpu_blk_ctrl { status = "disabled"; }; &pgc_gpumix { status = "disabled"; }; &pgc_gpu { status = "disabled"; }; The interesting thing is that the first patch that causes this is 'arm64: dts: imx8mm: add GPC node' which just adds the gpc node and is before the addition of the other nodes. Therefore these are being enabled by default regardless of the above nodes being disabled (or not even added yet to imx8mm.dtsi). Tim
Hi Tim, Am Freitag, dem 01.10.2021 um 19:43 -0700 schrieb Tim Harvey: > > > > > [...] > > > > > > Lucas, > > > > > > > > > > > > I've been using your 'i.MX8MM GPC improvements and BLK_CTRL driver' > > > > > > series for imx8mm-venice* and imx8mn-venice* boards. Thank you for > > > > > > this work and I hope to see it merged soon! > > > > > > > > > > > > I have found that on the imx8mm-venice-gw7901 board which does not use > > > > > > MIPI and thus does not connect VDD_MIPI_1P8, VDD_MIPI_1P2, > > > > > > VDD_MIPI_0P9, MIPI_VREG_CAP pins on the IMX8MM hangs with this > > > > > > particular patch. If I comment out the pgc_mipi domain and subsequent > > > > > > disp_blk_ctrl node from a later patch it resolves the hang. Is this > > > > > > behavior expected and what would your recommendation be to work around > > > > > > it? > > > > > > > > > > No, this isn't expected. If there are no active devices in the MIPI > > > > > domain, the power domain should not be touched, as we treat all of them > > > > > as disabled initially. If we don't touch the domain I would expect that > > > > > the power supply not being present shouldn't be an issue. > > > > > > > > > > Can you check if something in your system causes this power domain to > > > > > be powered up? Easiest way is probably to sprinkle a > > > > > printk("%s\n, genpd->name) in both imx8m_blk_ctrl_power_on() and > > > > > imx_gpc_power_on(). > > > > > > > > > > > > > Lucas, > > > > > > > > Here's what I see before I hang (debug print on both power on/off > > > > followed by a msleep(1000) to make sure I see it before I hang): > > > > [ 0.518319] imx_pgc_power_up hsiomix > > > > [ 0.624031] imx_pgc_power_down hsiomix > > > > [ 0.731879] imx_pgc_power_up hsiomix > > > > [ 0.839906] imx_pgc_power_down hsiomix > > > > [ 0.947875] imx_pgc_power_up hsiomix > > > > [ 1.055859] imx_pgc_power_down hsiomix > > > > [ 1.057296] imx_pgc_power_up gpumix > > > > [ 1.167884] imx_pgc_power_down gpumix > > > > [ 0.518513] imx_pgc_power_up hsiomix > > > > [ 0.519933] imx_pgc_power_up gpumix > > > > > > > > > > The board also has IMX8MM VDD_GPU pins not connected so it makes sense > > > that we hang here I suppose. Yet if I add the folloiwng to > > > imx8mm-venice-gw7901.dts it still tries to enable them and hangs: > > > &gpu_2d { > > > status = "disabled"; > > > }; > > > > > > &gpu_3d { > > > status = "disabled"; > > > }; > > > > > > &vpu_blk_ctrl { > > > status = "disabled"; > > > }; > > > > The pgc_gpu is a "active" consumer of the pgc_gpumix domain while the > > driver gets probed, so the driver core will power up the gpumix domain > > for a moment during kernel init. To avoid this you must at least set > > the status of the pgc_gpu node to disabled. > > > > I've tried that and it doesn't work either. > > &gpu_2d { > status = "disabled"; > }; > > &gpu_3d { > status = "disabled"; > }; > > &vpu_blk_ctrl { > status = "disabled"; > }; > > &pgc_gpumix { > status = "disabled"; > }; > > &pgc_gpu { > status = "disabled"; > }; > > The interesting thing is that the first patch that causes this is > 'arm64: dts: imx8mm: add GPC node' which just adds the gpc node and is > before the addition of the other nodes. Therefore these are being > enabled by default regardless of the above nodes being disabled (or > not even added yet to imx8mm.dtsi). My bad, I didn't think about the fact that the power domain devices are not instantiated via the common OF populate code. For the status property to work as expected the GPCv2 code needs to check this property. I've just sent a patch to make this happen. Can you give it another try with your DT changes and this patch applied? Regards, Lucas
On Sat, Oct 2, 2021 at 5:51 AM Lucas Stach <l.stach@pengutronix.de> wrote: > > Hi Tim, > > Am Freitag, dem 01.10.2021 um 19:43 -0700 schrieb Tim Harvey: > > > > > > > [...] > > > > > > > Lucas, > > > > > > > > > > > > > > I've been using your 'i.MX8MM GPC improvements and BLK_CTRL driver' > > > > > > > series for imx8mm-venice* and imx8mn-venice* boards. Thank you for > > > > > > > this work and I hope to see it merged soon! > > > > > > > > > > > > > > I have found that on the imx8mm-venice-gw7901 board which does not use > > > > > > > MIPI and thus does not connect VDD_MIPI_1P8, VDD_MIPI_1P2, > > > > > > > VDD_MIPI_0P9, MIPI_VREG_CAP pins on the IMX8MM hangs with this > > > > > > > particular patch. If I comment out the pgc_mipi domain and subsequent > > > > > > > disp_blk_ctrl node from a later patch it resolves the hang. Is this > > > > > > > behavior expected and what would your recommendation be to work around > > > > > > > it? > > > > > > > > > > > > No, this isn't expected. If there are no active devices in the MIPI > > > > > > domain, the power domain should not be touched, as we treat all of them > > > > > > as disabled initially. If we don't touch the domain I would expect that > > > > > > the power supply not being present shouldn't be an issue. > > > > > > > > > > > > Can you check if something in your system causes this power domain to > > > > > > be powered up? Easiest way is probably to sprinkle a > > > > > > printk("%s\n, genpd->name) in both imx8m_blk_ctrl_power_on() and > > > > > > imx_gpc_power_on(). > > > > > > > > > > > > > > > > Lucas, > > > > > > > > > > Here's what I see before I hang (debug print on both power on/off > > > > > followed by a msleep(1000) to make sure I see it before I hang): > > > > > [ 0.518319] imx_pgc_power_up hsiomix > > > > > [ 0.624031] imx_pgc_power_down hsiomix > > > > > [ 0.731879] imx_pgc_power_up hsiomix > > > > > [ 0.839906] imx_pgc_power_down hsiomix > > > > > [ 0.947875] imx_pgc_power_up hsiomix > > > > > [ 1.055859] imx_pgc_power_down hsiomix > > > > > [ 1.057296] imx_pgc_power_up gpumix > > > > > [ 1.167884] imx_pgc_power_down gpumix > > > > > [ 0.518513] imx_pgc_power_up hsiomix > > > > > [ 0.519933] imx_pgc_power_up gpumix > > > > > > > > > > > > > The board also has IMX8MM VDD_GPU pins not connected so it makes sense > > > > that we hang here I suppose. Yet if I add the folloiwng to > > > > imx8mm-venice-gw7901.dts it still tries to enable them and hangs: > > > > &gpu_2d { > > > > status = "disabled"; > > > > }; > > > > > > > > &gpu_3d { > > > > status = "disabled"; > > > > }; > > > > > > > > &vpu_blk_ctrl { > > > > status = "disabled"; > > > > }; > > > > > > The pgc_gpu is a "active" consumer of the pgc_gpumix domain while the > > > driver gets probed, so the driver core will power up the gpumix domain > > > for a moment during kernel init. To avoid this you must at least set > > > the status of the pgc_gpu node to disabled. > > > > > > > I've tried that and it doesn't work either. > > > > &gpu_2d { > > status = "disabled"; > > }; > > > > &gpu_3d { > > status = "disabled"; > > }; > > > > &vpu_blk_ctrl { > > status = "disabled"; > > }; > > > > &pgc_gpumix { > > status = "disabled"; > > }; > > > > &pgc_gpu { > > status = "disabled"; > > }; > > > > The interesting thing is that the first patch that causes this is > > 'arm64: dts: imx8mm: add GPC node' which just adds the gpc node and is > > before the addition of the other nodes. Therefore these are being > > enabled by default regardless of the above nodes being disabled (or > > not even added yet to imx8mm.dtsi). > > My bad, I didn't think about the fact that the power domain devices are > not instantiated via the common OF populate code. For the status > property to work as expected the GPCv2 code needs to check this > property. I've just sent a patch to make this happen. Can you give it > another try with your DT changes and this patch applied? > Yes, this makes sense and resolves it with only disabling pgc_gpumix in my dts. I believe it is fine for me to not specifically disable gpu_2d and gpu_3d as they are not enabled by default in imx8mm.dtsi and I don't believe that will change and I don't 'currently' need to disable disp_blk_ctrl or pgc_mipi but wondering if I should as the MIPI rails are not hooked up either? (ie will something added in the future try to use them?) What needs to be done to get this series merged? I suppose it's too late to get it into 5.13? Best regards, Tim
Am Sonntag, dem 03.10.2021 um 10:17 -0700 schrieb Tim Harvey: > On Sat, Oct 2, 2021 at 5:51 AM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > Hi Tim, > > > > Am Freitag, dem 01.10.2021 um 19:43 -0700 schrieb Tim Harvey: > > > > > > > > > [...] > > > > > > > > Lucas, > > > > > > > > > > > > > > > > I've been using your 'i.MX8MM GPC improvements and BLK_CTRL driver' > > > > > > > > series for imx8mm-venice* and imx8mn-venice* boards. Thank you for > > > > > > > > this work and I hope to see it merged soon! > > > > > > > > > > > > > > > > I have found that on the imx8mm-venice-gw7901 board which does not use > > > > > > > > MIPI and thus does not connect VDD_MIPI_1P8, VDD_MIPI_1P2, > > > > > > > > VDD_MIPI_0P9, MIPI_VREG_CAP pins on the IMX8MM hangs with this > > > > > > > > particular patch. If I comment out the pgc_mipi domain and subsequent > > > > > > > > disp_blk_ctrl node from a later patch it resolves the hang. Is this > > > > > > > > behavior expected and what would your recommendation be to work around > > > > > > > > it? > > > > > > > > > > > > > > No, this isn't expected. If there are no active devices in the MIPI > > > > > > > domain, the power domain should not be touched, as we treat all of them > > > > > > > as disabled initially. If we don't touch the domain I would expect that > > > > > > > the power supply not being present shouldn't be an issue. > > > > > > > > > > > > > > Can you check if something in your system causes this power domain to > > > > > > > be powered up? Easiest way is probably to sprinkle a > > > > > > > printk("%s\n, genpd->name) in both imx8m_blk_ctrl_power_on() and > > > > > > > imx_gpc_power_on(). > > > > > > > > > > > > > > > > > > > Lucas, > > > > > > > > > > > > Here's what I see before I hang (debug print on both power on/off > > > > > > followed by a msleep(1000) to make sure I see it before I hang): > > > > > > [ 0.518319] imx_pgc_power_up hsiomix > > > > > > [ 0.624031] imx_pgc_power_down hsiomix > > > > > > [ 0.731879] imx_pgc_power_up hsiomix > > > > > > [ 0.839906] imx_pgc_power_down hsiomix > > > > > > [ 0.947875] imx_pgc_power_up hsiomix > > > > > > [ 1.055859] imx_pgc_power_down hsiomix > > > > > > [ 1.057296] imx_pgc_power_up gpumix > > > > > > [ 1.167884] imx_pgc_power_down gpumix > > > > > > [ 0.518513] imx_pgc_power_up hsiomix > > > > > > [ 0.519933] imx_pgc_power_up gpumix > > > > > > > > > > > > > > > > The board also has IMX8MM VDD_GPU pins not connected so it makes sense > > > > > that we hang here I suppose. Yet if I add the folloiwng to > > > > > imx8mm-venice-gw7901.dts it still tries to enable them and hangs: > > > > > &gpu_2d { > > > > > status = "disabled"; > > > > > }; > > > > > > > > > > &gpu_3d { > > > > > status = "disabled"; > > > > > }; > > > > > > > > > > &vpu_blk_ctrl { > > > > > status = "disabled"; > > > > > }; > > > > > > > > The pgc_gpu is a "active" consumer of the pgc_gpumix domain while the > > > > driver gets probed, so the driver core will power up the gpumix domain > > > > for a moment during kernel init. To avoid this you must at least set > > > > the status of the pgc_gpu node to disabled. > > > > > > > > > > I've tried that and it doesn't work either. > > > > > > &gpu_2d { > > > status = "disabled"; > > > }; > > > > > > &gpu_3d { > > > status = "disabled"; > > > }; > > > > > > &vpu_blk_ctrl { > > > status = "disabled"; > > > }; > > > > > > &pgc_gpumix { > > > status = "disabled"; > > > }; > > > > > > &pgc_gpu { > > > status = "disabled"; > > > }; > > > > > > The interesting thing is that the first patch that causes this is > > > 'arm64: dts: imx8mm: add GPC node' which just adds the gpc node and is > > > before the addition of the other nodes. Therefore these are being > > > enabled by default regardless of the above nodes being disabled (or > > > not even added yet to imx8mm.dtsi). > > > > My bad, I didn't think about the fact that the power domain devices are > > not instantiated via the common OF populate code. For the status > > property to work as expected the GPCv2 code needs to check this > > property. I've just sent a patch to make this happen. Can you give it > > another try with your DT changes and this patch applied? > > > > Yes, this makes sense and resolves it with only disabling pgc_gpumix in my dts. > > I believe it is fine for me to not specifically disable gpu_2d and > gpu_3d as they are not enabled by default in imx8mm.dtsi and I don't > believe that will change and I don't 'currently' need to disable > disp_blk_ctrl or pgc_mipi but wondering if I should as the MIPI rails > are not hooked up either? (ie will something added in the future try > to use them?) > I think it would be better to disable the GPU devices in your DT. They don't have a status property and thus are enabled by default. They won't probe without the power domain being available, but it will cause those devices to stay in probe deferred state forever, which may confuse some users. So I guess it would be good style to disable the GPU nodes. One consequence of disabling the MIPI power domain is that the disp- blk-ctrl driver won't probe anymore, as it needs all the referenced power domains to be available. Do you think this might be an issue? Is there a valid system configuration where you would use devices from from the display power domain, but not the MIPI domain? If that's a valid case we might need to make this configuration possible in the disp-blk-ctrl driver. > What needs to be done to get this series merged? I suppose it's too > late to get it into 5.13? 5.13? Way too late, even the 5.15 merge window is long closed. The series is almost completely reviewed and the DT bindings are also good to go. So if no one happens to run into any real showstopper, Shawn may be willing to pick up the series and stage it for the 5.16 merge window. Regards, Lucas
On Sun, Oct 3, 2021 at 12:44 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > Am Sonntag, dem 03.10.2021 um 10:17 -0700 schrieb Tim Harvey: > > On Sat, Oct 2, 2021 at 5:51 AM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > > > Hi Tim, > > > > > > Am Freitag, dem 01.10.2021 um 19:43 -0700 schrieb Tim Harvey: > > > > > > > > > > > [...] > > > > > > > > > Lucas, > > > > > > > > > > > > > > > > > > I've been using your 'i.MX8MM GPC improvements and BLK_CTRL driver' > > > > > > > > > series for imx8mm-venice* and imx8mn-venice* boards. Thank you for > > > > > > > > > this work and I hope to see it merged soon! > > > > > > > > > > > > > > > > > > I have found that on the imx8mm-venice-gw7901 board which does not use > > > > > > > > > MIPI and thus does not connect VDD_MIPI_1P8, VDD_MIPI_1P2, > > > > > > > > > VDD_MIPI_0P9, MIPI_VREG_CAP pins on the IMX8MM hangs with this > > > > > > > > > particular patch. If I comment out the pgc_mipi domain and subsequent > > > > > > > > > disp_blk_ctrl node from a later patch it resolves the hang. Is this > > > > > > > > > behavior expected and what would your recommendation be to work around > > > > > > > > > it? > > > > > > > > > > > > > > > > No, this isn't expected. If there are no active devices in the MIPI > > > > > > > > domain, the power domain should not be touched, as we treat all of them > > > > > > > > as disabled initially. If we don't touch the domain I would expect that > > > > > > > > the power supply not being present shouldn't be an issue. > > > > > > > > > > > > > > > > Can you check if something in your system causes this power domain to > > > > > > > > be powered up? Easiest way is probably to sprinkle a > > > > > > > > printk("%s\n, genpd->name) in both imx8m_blk_ctrl_power_on() and > > > > > > > > imx_gpc_power_on(). > > > > > > > > > > > > > > > > > > > > > > Lucas, > > > > > > > > > > > > > > Here's what I see before I hang (debug print on both power on/off > > > > > > > followed by a msleep(1000) to make sure I see it before I hang): > > > > > > > [ 0.518319] imx_pgc_power_up hsiomix > > > > > > > [ 0.624031] imx_pgc_power_down hsiomix > > > > > > > [ 0.731879] imx_pgc_power_up hsiomix > > > > > > > [ 0.839906] imx_pgc_power_down hsiomix > > > > > > > [ 0.947875] imx_pgc_power_up hsiomix > > > > > > > [ 1.055859] imx_pgc_power_down hsiomix > > > > > > > [ 1.057296] imx_pgc_power_up gpumix > > > > > > > [ 1.167884] imx_pgc_power_down gpumix > > > > > > > [ 0.518513] imx_pgc_power_up hsiomix > > > > > > > [ 0.519933] imx_pgc_power_up gpumix > > > > > > > > > > > > > > > > > > > The board also has IMX8MM VDD_GPU pins not connected so it makes sense > > > > > > that we hang here I suppose. Yet if I add the folloiwng to > > > > > > imx8mm-venice-gw7901.dts it still tries to enable them and hangs: > > > > > > &gpu_2d { > > > > > > status = "disabled"; > > > > > > }; > > > > > > > > > > > > &gpu_3d { > > > > > > status = "disabled"; > > > > > > }; > > > > > > > > > > > > &vpu_blk_ctrl { > > > > > > status = "disabled"; > > > > > > }; > > > > > > > > > > The pgc_gpu is a "active" consumer of the pgc_gpumix domain while the > > > > > driver gets probed, so the driver core will power up the gpumix domain > > > > > for a moment during kernel init. To avoid this you must at least set > > > > > the status of the pgc_gpu node to disabled. > > > > > > > > > > > > > I've tried that and it doesn't work either. > > > > > > > > &gpu_2d { > > > > status = "disabled"; > > > > }; > > > > > > > > &gpu_3d { > > > > status = "disabled"; > > > > }; > > > > > > > > &vpu_blk_ctrl { > > > > status = "disabled"; > > > > }; > > > > > > > > &pgc_gpumix { > > > > status = "disabled"; > > > > }; > > > > > > > > &pgc_gpu { > > > > status = "disabled"; > > > > }; > > > > > > > > The interesting thing is that the first patch that causes this is > > > > 'arm64: dts: imx8mm: add GPC node' which just adds the gpc node and is > > > > before the addition of the other nodes. Therefore these are being > > > > enabled by default regardless of the above nodes being disabled (or > > > > not even added yet to imx8mm.dtsi). > > > > > > My bad, I didn't think about the fact that the power domain devices are > > > not instantiated via the common OF populate code. For the status > > > property to work as expected the GPCv2 code needs to check this > > > property. I've just sent a patch to make this happen. Can you give it > > > another try with your DT changes and this patch applied? > > > > > > > Yes, this makes sense and resolves it with only disabling pgc_gpumix in my dts. > > > > I believe it is fine for me to not specifically disable gpu_2d and > > gpu_3d as they are not enabled by default in imx8mm.dtsi and I don't > > believe that will change and I don't 'currently' need to disable > > disp_blk_ctrl or pgc_mipi but wondering if I should as the MIPI rails > > are not hooked up either? (ie will something added in the future try > > to use them?) > > > I think it would be better to disable the GPU devices in your DT. They > don't have a status property and thus are enabled by default. They > won't probe without the power domain being available, but it will cause > those devices to stay in probe deferred state forever, which may > confuse some users. So I guess it would be good style to disable the > GPU nodes. > it still tries to power the pgc_gpumix domain if I 'only' disable gpu_2d and gpu_3d. I have to disable 'pgc_gpumix' specifically - is that what you expect here? > One consequence of disabling the MIPI power domain is that the disp- > blk-ctrl driver won't probe anymore, as it needs all the referenced > power domains to be available. Do you think this might be an issue? Is > there a valid system configuration where you would use devices from > from the display power domain, but not the MIPI domain? If that's a > valid case we might need to make this configuration possible in the > disp-blk-ctrl driver. In the case of imx8mm I don't see that you can have any display without MIPI however the imx8mp can have HDMI display without needing MIPI so in general that should be allowed. > > > What needs to be done to get this series merged? I suppose it's too > > late to get it into 5.13? > > 5.13? Way too late, even the 5.15 merge window is long closed. The > series is almost completely reviewed and the DT bindings are also good > to go. So if no one happens to run into any real showstopper, Shawn may > be willing to pick up the series and stage it for the 5.16 merge > window. oops... I meant 5.15 but yes, seems too late for that. We seem to have time to get this into 5.16 however. I'm not sure what Shawn's cut-off is there. Tim
Am Sonntag, dem 03.10.2021 um 14:21 -0700 schrieb Tim Harvey: > On Sun, Oct 3, 2021 at 12:44 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > Am Sonntag, dem 03.10.2021 um 10:17 -0700 schrieb Tim Harvey: > > > On Sat, Oct 2, 2021 at 5:51 AM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > > > > > Hi Tim, > > > > > > > > Am Freitag, dem 01.10.2021 um 19:43 -0700 schrieb Tim Harvey: > > > > > > > > > > > > > [...] > > > > > > > > > > Lucas, > > > > > > > > > > > > > > > > > > > > I've been using your 'i.MX8MM GPC improvements and BLK_CTRL driver' > > > > > > > > > > series for imx8mm-venice* and imx8mn-venice* boards. Thank you for > > > > > > > > > > this work and I hope to see it merged soon! > > > > > > > > > > > > > > > > > > > > I have found that on the imx8mm-venice-gw7901 board which does not use > > > > > > > > > > MIPI and thus does not connect VDD_MIPI_1P8, VDD_MIPI_1P2, > > > > > > > > > > VDD_MIPI_0P9, MIPI_VREG_CAP pins on the IMX8MM hangs with this > > > > > > > > > > particular patch. If I comment out the pgc_mipi domain and subsequent > > > > > > > > > > disp_blk_ctrl node from a later patch it resolves the hang. Is this > > > > > > > > > > behavior expected and what would your recommendation be to work around > > > > > > > > > > it? > > > > > > > > > > > > > > > > > > No, this isn't expected. If there are no active devices in the MIPI > > > > > > > > > domain, the power domain should not be touched, as we treat all of them > > > > > > > > > as disabled initially. If we don't touch the domain I would expect that > > > > > > > > > the power supply not being present shouldn't be an issue. > > > > > > > > > > > > > > > > > > Can you check if something in your system causes this power domain to > > > > > > > > > be powered up? Easiest way is probably to sprinkle a > > > > > > > > > printk("%s\n, genpd->name) in both imx8m_blk_ctrl_power_on() and > > > > > > > > > imx_gpc_power_on(). > > > > > > > > > > > > > > > > > > > > > > > > > Lucas, > > > > > > > > > > > > > > > > Here's what I see before I hang (debug print on both power on/off > > > > > > > > followed by a msleep(1000) to make sure I see it before I hang): > > > > > > > > [ 0.518319] imx_pgc_power_up hsiomix > > > > > > > > [ 0.624031] imx_pgc_power_down hsiomix > > > > > > > > [ 0.731879] imx_pgc_power_up hsiomix > > > > > > > > [ 0.839906] imx_pgc_power_down hsiomix > > > > > > > > [ 0.947875] imx_pgc_power_up hsiomix > > > > > > > > [ 1.055859] imx_pgc_power_down hsiomix > > > > > > > > [ 1.057296] imx_pgc_power_up gpumix > > > > > > > > [ 1.167884] imx_pgc_power_down gpumix > > > > > > > > [ 0.518513] imx_pgc_power_up hsiomix > > > > > > > > [ 0.519933] imx_pgc_power_up gpumix > > > > > > > > > > > > > > > > > > > > > > The board also has IMX8MM VDD_GPU pins not connected so it makes sense > > > > > > > that we hang here I suppose. Yet if I add the folloiwng to > > > > > > > imx8mm-venice-gw7901.dts it still tries to enable them and hangs: > > > > > > > &gpu_2d { > > > > > > > status = "disabled"; > > > > > > > }; > > > > > > > > > > > > > > &gpu_3d { > > > > > > > status = "disabled"; > > > > > > > }; > > > > > > > > > > > > > > &vpu_blk_ctrl { > > > > > > > status = "disabled"; > > > > > > > }; > > > > > > > > > > > > The pgc_gpu is a "active" consumer of the pgc_gpumix domain while the > > > > > > driver gets probed, so the driver core will power up the gpumix domain > > > > > > for a moment during kernel init. To avoid this you must at least set > > > > > > the status of the pgc_gpu node to disabled. > > > > > > > > > > > > > > > > I've tried that and it doesn't work either. > > > > > > > > > > &gpu_2d { > > > > > status = "disabled"; > > > > > }; > > > > > > > > > > &gpu_3d { > > > > > status = "disabled"; > > > > > }; > > > > > > > > > > &vpu_blk_ctrl { > > > > > status = "disabled"; > > > > > }; > > > > > > > > > > &pgc_gpumix { > > > > > status = "disabled"; > > > > > }; > > > > > > > > > > &pgc_gpu { > > > > > status = "disabled"; > > > > > }; > > > > > > > > > > The interesting thing is that the first patch that causes this is > > > > > 'arm64: dts: imx8mm: add GPC node' which just adds the gpc node and is > > > > > before the addition of the other nodes. Therefore these are being > > > > > enabled by default regardless of the above nodes being disabled (or > > > > > not even added yet to imx8mm.dtsi). > > > > > > > > My bad, I didn't think about the fact that the power domain devices are > > > > not instantiated via the common OF populate code. For the status > > > > property to work as expected the GPCv2 code needs to check this > > > > property. I've just sent a patch to make this happen. Can you give it > > > > another try with your DT changes and this patch applied? > > > > > > > > > > Yes, this makes sense and resolves it with only disabling pgc_gpumix in my dts. > > > > > > I believe it is fine for me to not specifically disable gpu_2d and > > > gpu_3d as they are not enabled by default in imx8mm.dtsi and I don't > > > believe that will change and I don't 'currently' need to disable > > > disp_blk_ctrl or pgc_mipi but wondering if I should as the MIPI rails > > > are not hooked up either? (ie will something added in the future try > > > to use them?) > > > > > I think it would be better to disable the GPU devices in your DT. They > > don't have a status property and thus are enabled by default. They > > won't probe without the power domain being available, but it will cause > > those devices to stay in probe deferred state forever, which may > > confuse some users. So I guess it would be good style to disable the > > GPU nodes. > > > > it still tries to power the pgc_gpumix domain if I 'only' disable > gpu_2d and gpu_3d. I have to disable 'pgc_gpumix' specifically - is > that what you expect here? > > > One consequence of disabling the MIPI power domain is that the disp- > > blk-ctrl driver won't probe anymore, as it needs all the referenced > > power domains to be available. Do you think this might be an issue? Is > > there a valid system configuration where you would use devices from > > from the display power domain, but not the MIPI domain? If that's a > > valid case we might need to make this configuration possible in the > > disp-blk-ctrl driver. > > In the case of imx8mm I don't see that you can have any display > without MIPI however the imx8mp can have HDMI display without needing > MIPI so in general that should be allowed. I was thinking about the camera input path, but this one also needs the MIPI domain. The camera interface is useless without the MIPI PHY to get data from. The i.MX8MP is a different topic, as the HDMI part is a separate blk-ctrl domain there. With that in mind, I think things are okay as they are implemented now, as I don't see any use-case where one would want to use devices from the DISP domain (CSI, LCDIF) without having the MIPI PHY parts available. Regards, Lucas > > > > > > What needs to be done to get this series merged? I suppose it's too > > > late to get it into 5.13? > > > > 5.13? Way too late, even the 5.15 merge window is long closed. The > > series is almost completely reviewed and the DT bindings are also good > > to go. So if no one happens to run into any real showstopper, Shawn may > > be willing to pick up the series and stage it for the 5.16 merge > > window. > > oops... I meant 5.15 but yes, seems too late for that. We seem to have > time to get this into 5.16 however. I'm not sure what Shawn's cut-off > is there. > > Tim
Am Sonntag, dem 03.10.2021 um 14:21 -0700 schrieb Tim Harvey: > On Sun, Oct 3, 2021 at 12:44 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > Am Sonntag, dem 03.10.2021 um 10:17 -0700 schrieb Tim Harvey: > > > On Sat, Oct 2, 2021 at 5:51 AM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > > > > > Hi Tim, > > > > > > > > Am Freitag, dem 01.10.2021 um 19:43 -0700 schrieb Tim Harvey: > > > > > > > > > > > > > [...] > > > > > > > > > > Lucas, > > > > > > > > > > > > > > > > > > > > I've been using your 'i.MX8MM GPC improvements and BLK_CTRL driver' > > > > > > > > > > series for imx8mm-venice* and imx8mn-venice* boards. Thank you for > > > > > > > > > > this work and I hope to see it merged soon! > > > > > > > > > > > > > > > > > > > > I have found that on the imx8mm-venice-gw7901 board which does not use > > > > > > > > > > MIPI and thus does not connect VDD_MIPI_1P8, VDD_MIPI_1P2, > > > > > > > > > > VDD_MIPI_0P9, MIPI_VREG_CAP pins on the IMX8MM hangs with this > > > > > > > > > > particular patch. If I comment out the pgc_mipi domain and subsequent > > > > > > > > > > disp_blk_ctrl node from a later patch it resolves the hang. Is this > > > > > > > > > > behavior expected and what would your recommendation be to work around > > > > > > > > > > it? > > > > > > > > > > > > > > > > > > No, this isn't expected. If there are no active devices in the MIPI > > > > > > > > > domain, the power domain should not be touched, as we treat all of them > > > > > > > > > as disabled initially. If we don't touch the domain I would expect that > > > > > > > > > the power supply not being present shouldn't be an issue. > > > > > > > > > > > > > > > > > > Can you check if something in your system causes this power domain to > > > > > > > > > be powered up? Easiest way is probably to sprinkle a > > > > > > > > > printk("%s\n, genpd->name) in both imx8m_blk_ctrl_power_on() and > > > > > > > > > imx_gpc_power_on(). > > > > > > > > > > > > > > > > > > > > > > > > > Lucas, > > > > > > > > > > > > > > > > Here's what I see before I hang (debug print on both power on/off > > > > > > > > followed by a msleep(1000) to make sure I see it before I hang): > > > > > > > > [ 0.518319] imx_pgc_power_up hsiomix > > > > > > > > [ 0.624031] imx_pgc_power_down hsiomix > > > > > > > > [ 0.731879] imx_pgc_power_up hsiomix > > > > > > > > [ 0.839906] imx_pgc_power_down hsiomix > > > > > > > > [ 0.947875] imx_pgc_power_up hsiomix > > > > > > > > [ 1.055859] imx_pgc_power_down hsiomix > > > > > > > > [ 1.057296] imx_pgc_power_up gpumix > > > > > > > > [ 1.167884] imx_pgc_power_down gpumix > > > > > > > > [ 0.518513] imx_pgc_power_up hsiomix > > > > > > > > [ 0.519933] imx_pgc_power_up gpumix > > > > > > > > > > > > > > > > > > > > > > The board also has IMX8MM VDD_GPU pins not connected so it makes sense > > > > > > > that we hang here I suppose. Yet if I add the folloiwng to > > > > > > > imx8mm-venice-gw7901.dts it still tries to enable them and hangs: > > > > > > > &gpu_2d { > > > > > > > status = "disabled"; > > > > > > > }; > > > > > > > > > > > > > > &gpu_3d { > > > > > > > status = "disabled"; > > > > > > > }; > > > > > > > > > > > > > > &vpu_blk_ctrl { > > > > > > > status = "disabled"; > > > > > > > }; > > > > > > > > > > > > The pgc_gpu is a "active" consumer of the pgc_gpumix domain while the > > > > > > driver gets probed, so the driver core will power up the gpumix domain > > > > > > for a moment during kernel init. To avoid this you must at least set > > > > > > the status of the pgc_gpu node to disabled. > > > > > > > > > > > > > > > > I've tried that and it doesn't work either. > > > > > > > > > > &gpu_2d { > > > > > status = "disabled"; > > > > > }; > > > > > > > > > > &gpu_3d { > > > > > status = "disabled"; > > > > > }; > > > > > > > > > > &vpu_blk_ctrl { > > > > > status = "disabled"; > > > > > }; > > > > > > > > > > &pgc_gpumix { > > > > > status = "disabled"; > > > > > }; > > > > > > > > > > &pgc_gpu { > > > > > status = "disabled"; > > > > > }; > > > > > > > > > > The interesting thing is that the first patch that causes this is > > > > > 'arm64: dts: imx8mm: add GPC node' which just adds the gpc node and is > > > > > before the addition of the other nodes. Therefore these are being > > > > > enabled by default regardless of the above nodes being disabled (or > > > > > not even added yet to imx8mm.dtsi). > > > > > > > > My bad, I didn't think about the fact that the power domain devices are > > > > not instantiated via the common OF populate code. For the status > > > > property to work as expected the GPCv2 code needs to check this > > > > property. I've just sent a patch to make this happen. Can you give it > > > > another try with your DT changes and this patch applied? > > > > > > > > > > Yes, this makes sense and resolves it with only disabling pgc_gpumix in my dts. > > > > > > I believe it is fine for me to not specifically disable gpu_2d and > > > gpu_3d as they are not enabled by default in imx8mm.dtsi and I don't > > > believe that will change and I don't 'currently' need to disable > > > disp_blk_ctrl or pgc_mipi but wondering if I should as the MIPI rails > > > are not hooked up either? (ie will something added in the future try > > > to use them?) > > > > > I think it would be better to disable the GPU devices in your DT. They > > don't have a status property and thus are enabled by default. They > > won't probe without the power domain being available, but it will cause > > those devices to stay in probe deferred state forever, which may > > confuse some users. So I guess it would be good style to disable the > > GPU nodes. > > > > it still tries to power the pgc_gpumix domain if I 'only' disable > gpu_2d and gpu_3d. I have to disable 'pgc_gpumix' specifically - is > that what you expect here? Yes, that's expected. As I said earlier, the gpu domain is considered a active child device of the gpumix domain if it isn't disabled in DT, so the driver core will try to power up the gpumix domain before the gpu domain is probed. You must disable the power domains in DT to avoid them ever being touched on your board. You should disable the GPU devices depending on this power domain to avoid the GPU devices staying in probe deferred limbo forever. Regards, Lucas
diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi index e7648c3b8390..3922f26f8fd4 100644 --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi @@ -7,6 +7,8 @@ #include <dt-bindings/gpio/gpio.h> #include <dt-bindings/input/input.h> #include <dt-bindings/interrupt-controller/arm-gic.h> +#include <dt-bindings/power/imx8mm-power.h> +#include <dt-bindings/reset/imx8mq-reset.h> #include <dt-bindings/thermal/thermal.h> #include "imx8mm-pinfunc.h" @@ -609,6 +611,111 @@ src: reset-controller@30390000 { interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>; #reset-cells = <1>; }; + + gpc: gpc@303a0000 { + compatible = "fsl,imx8mm-gpc"; + reg = <0x303a0000 0x10000>; + interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>; + interrupt-parent = <&gic>; + interrupt-controller; + #interrupt-cells = <3>; + + pgc { + #address-cells = <1>; + #size-cells = <0>; + + pgc_hsiomix: power-domain@0 { + #power-domain-cells = <0>; + reg = <IMX8MM_POWER_DOMAIN_HSIOMIX>; + clocks = <&clk IMX8MM_CLK_USB_BUS>; + assigned-clocks = <&clk IMX8MM_CLK_USB_BUS>; + assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_500M>; + }; + + pgc_pcie: power-domain@1 { + #power-domain-cells = <0>; + reg = <IMX8MM_POWER_DOMAIN_PCIE>; + power-domains = <&pgc_hsiomix>; + clocks = <&clk IMX8MM_CLK_PCIE1_ROOT>; + }; + + pgc_otg1: power-domain@2 { + #power-domain-cells = <0>; + reg = <IMX8MM_POWER_DOMAIN_OTG1>; + power-domains = <&pgc_hsiomix>; + }; + + pgc_otg2: power-domain@3 { + #power-domain-cells = <0>; + reg = <IMX8MM_POWER_DOMAIN_OTG2>; + power-domains = <&pgc_hsiomix>; + }; + + pgc_gpumix: power-domain@4 { + #power-domain-cells = <0>; + reg = <IMX8MM_POWER_DOMAIN_GPUMIX>; + clocks = <&clk IMX8MM_CLK_GPU_BUS_ROOT>, + <&clk IMX8MM_CLK_GPU_AHB>; + assigned-clocks = <&clk IMX8MM_CLK_GPU_AXI>, + <&clk IMX8MM_CLK_GPU_AHB>; + assigned-clock-parents = <&clk IMX8MM_SYS_PLL1_800M>, + <&clk IMX8MM_SYS_PLL1_800M>; + assigned-clock-rates = <800000000>, <400000000>; + }; + + pgc_gpu: power-domain@5 { + #power-domain-cells = <0>; + reg = <IMX8MM_POWER_DOMAIN_GPU>; + clocks = <&clk IMX8MM_CLK_GPU_AHB>, + <&clk IMX8MM_CLK_GPU_BUS_ROOT>, + <&clk IMX8MM_CLK_GPU2D_ROOT>, + <&clk IMX8MM_CLK_GPU3D_ROOT>; + resets = <&src IMX8MQ_RESET_GPU_RESET>; + power-domains = <&pgc_gpumix>; + }; + + pgc_vpumix: power-domain@6 { + #power-domain-cells = <0>; + reg = <IMX8MM_POWER_DOMAIN_VPUMIX>; + 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 { + #power-domain-cells = <0>; + reg = <IMX8MM_POWER_DOMAIN_VPUG1>; + }; + + pgc_vpu_g2: power-domain@8 { + #power-domain-cells = <0>; + reg = <IMX8MM_POWER_DOMAIN_VPUG2>; + }; + + pgc_vpu_h1: power-domain@9 { + #power-domain-cells = <0>; + reg = <IMX8MM_POWER_DOMAIN_VPUH1>; + }; + + pgc_dispmix: power-domain@10 { + #power-domain-cells = <0>; + reg = <IMX8MM_POWER_DOMAIN_DISPMIX>; + clocks = <&clk IMX8MM_CLK_DISP_APB_ROOT>, + <&clk IMX8MM_CLK_DISP_AXI_ROOT>; + assigned-clocks = <&clk IMX8MM_CLK_DISP_AXI>, + <&clk IMX8MM_CLK_DISP_APB>; + assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_1000M>, + <&clk IMX8MM_SYS_PLL1_800M>; + assigned-clock-rates = <500000000>, <200000000>; + }; + + pgc_mipi: power-domain@11 { + #power-domain-cells = <0>; + reg = <IMX8MM_POWER_DOMAIN_MIPI>; + }; + }; + }; }; aips2: bus@30400000 {
Add the DT node for the GPC, including all the PGC power domains, some of them are not fully functional yet, as they require interaction with the blk-ctrls to properly power up/down the peripherals. Signed-off-by: Lucas Stach <l.stach@pengutronix.de> --- arch/arm64/boot/dts/freescale/imx8mm.dtsi | 107 ++++++++++++++++++++++ 1 file changed, 107 insertions(+)