| Message ID | 20220207192547.1997549-7-l.stach@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/9] soc: imx: gpcv2: add PGC control register indirection | expand |
Hi Lucas, Thank you for the patch. On Mon, Feb 07, 2022 at 08:25:45PM +0100, Lucas Stach wrote: > This adds the GPC and HSIO blk-ctrl nodes providing power control for > the high-speed (USB and PCIe) IOs. > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > --- > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 63 ++++++++++++++++++++--- > 1 file changed, 57 insertions(+), 6 deletions(-) > > diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > index 6b840c05dd77..dc488a147d0c 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > @@ -4,6 +4,7 @@ > */ > > #include <dt-bindings/clock/imx8mp-clock.h> > +#include <dt-bindings/power/imx8mp-power.h> > #include <dt-bindings/gpio/gpio.h> > #include <dt-bindings/input/input.h> > #include <dt-bindings/interrupt-controller/arm-gic.h> > @@ -475,6 +476,44 @@ src: reset-controller@30390000 { > interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>; > #reset-cells = <1>; > }; > + > + gpc: gpc@303a0000 { > + compatible = "fsl,imx8mp-gpc"; > + reg = <0x303a0000 0x10000>; According to the reference manual, the GPC occupies 4kB, not 64kB. > + interrupt-parent = <&gic>; > + interrupt-controller; > + #interrupt-cells = <3>; > + > + pgc { > + #address-cells = <1>; > + #size-cells = <0>; > + We're working on support for the MEDIAMIX power domains, which we'll rebase on top of this. In case the HSIO part still needs more work, could you split this patch in two, with one patch that adds the PGC, with an empty pgc nodde, and a second patch that adds the HSIO-related power domains ? The first one could then be merged faster (it would be great if it could get in v5.18). > + pgc_pcie_phy: power-domain@1 { > + #power-domain-cells = <0>; > + reg = <IMX8MP_POWER_DOMAIN_PCIE_PHY>; > + }; > + > + pgc_usb1_phy: power-domain@2 { > + #power-domain-cells = <0>; > + reg = <IMX8MP_POWER_DOMAIN_USB1_PHY>; > + }; > + > + pgc_usb2_phy: power-domain@3 { > + #power-domain-cells = <0>; > + reg = <IMX8MP_POWER_DOMAIN_USB2_PHY>; > + }; > + > + pgc_hsiomix: power-domains@17 { > + #power-domain-cells = <0>; > + reg = <IMX8MP_POWER_DOMAIN_HSIOMIX>; > + clocks = <&clk IMX8MP_CLK_HSIO_AXI>, > + <&clk IMX8MP_CLK_HSIO_ROOT>; > + assigned-clocks = <&clk IMX8MP_CLK_HSIO_AXI>; > + assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; > + assigned-clock-rates = <500000000>; > + }; > + }; > + }; > }; > > aips2: bus@30400000 { > @@ -908,6 +947,20 @@ ddr-pmu@3d800000 { > interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>; > }; > > + hsio_blk_ctrl: blk-ctrl@32f10000 { > + compatible = "fsl,imx8mp-hsio-blk-ctrl", "syscon"; > + reg = <0x32f10000 0x24>; > + clocks = <&clk IMX8MP_CLK_USB_ROOT>, > + <&clk IMX8MP_CLK_PCIE_ROOT>; > + clock-names = "usb", "pcie"; > + power-domains = <&pgc_hsiomix>, <&pgc_hsiomix>, > + <&pgc_usb1_phy>, <&pgc_usb2_phy>, > + <&pgc_hsiomix>, <&pgc_pcie_phy>; Would it be useful to rework the driver to avoid specifying the same parent power domain multiple times in DT ? > + power-domain-names = "bus", "usb", "usb-phy1", > + "usb-phy2", "pcie", "pcie-phy"; > + #power-domain-cells = <1>; > + }; > + > usb3_phy0: usb-phy@381f0040 { > compatible = "fsl,imx8mp-usb-phy"; > reg = <0x381f0040 0x40>; > @@ -915,6 +968,7 @@ usb3_phy0: usb-phy@381f0040 { > clock-names = "phy"; > assigned-clocks = <&clk IMX8MP_CLK_USB_PHY_REF>; > assigned-clock-parents = <&clk IMX8MP_CLK_24M>; > + power-domains = <&hsio_blk_ctrl IMX8MP_HSIOBLK_PD_USB_PHY1>; > #phy-cells = <0>; > status = "disabled"; > }; > @@ -926,6 +980,7 @@ usb3_0: usb@32f10100 { > <&clk IMX8MP_CLK_USB_ROOT>; > clock-names = "hsio", "suspend"; > interrupts = <GIC_SPI 148 IRQ_TYPE_LEVEL_HIGH>; > + power-domains = <&hsio_blk_ctrl IMX8MP_HSIOBLK_PD_USB>; > #address-cells = <1>; > #size-cells = <1>; > dma-ranges = <0x40000000 0x40000000 0xc0000000>; > @@ -939,9 +994,6 @@ usb_dwc3_0: usb@38100000 { > <&clk IMX8MP_CLK_USB_CORE_REF>, > <&clk IMX8MP_CLK_USB_ROOT>; > clock-names = "bus_early", "ref", "suspend"; > - assigned-clocks = <&clk IMX8MP_CLK_HSIO_AXI>; > - assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; > - assigned-clock-rates = <500000000>; > interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>; > phys = <&usb3_phy0>, <&usb3_phy0>; > phy-names = "usb2-phy", "usb3-phy"; > @@ -957,6 +1009,7 @@ usb3_phy1: usb-phy@382f0040 { > clock-names = "phy"; > assigned-clocks = <&clk IMX8MP_CLK_USB_PHY_REF>; > assigned-clock-parents = <&clk IMX8MP_CLK_24M>; > + power-domains = <&hsio_blk_ctrl IMX8MP_HSIOBLK_PD_USB_PHY2>; > #phy-cells = <0>; > }; > > @@ -967,6 +1020,7 @@ usb3_1: usb@32f10108 { > <&clk IMX8MP_CLK_USB_ROOT>; > clock-names = "hsio", "suspend"; > interrupts = <GIC_SPI 149 IRQ_TYPE_LEVEL_HIGH>; > + power-domains = <&hsio_blk_ctrl IMX8MP_HSIOBLK_PD_USB>; > #address-cells = <1>; > #size-cells = <1>; > dma-ranges = <0x40000000 0x40000000 0xc0000000>; > @@ -980,9 +1034,6 @@ usb_dwc3_1: usb@38200000 { > <&clk IMX8MP_CLK_USB_CORE_REF>, > <&clk IMX8MP_CLK_USB_ROOT>; > clock-names = "bus_early", "ref", "suspend"; > - assigned-clocks = <&clk IMX8MP_CLK_HSIO_AXI>; > - assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; > - assigned-clock-rates = <500000000>; > interrupts = <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>; > phys = <&usb3_phy1>, <&usb3_phy1>; > phy-names = "usb2-phy", "usb3-phy"; >
Hi Lucas, One more comment. On Sat, Feb 19, 2022 at 09:40:50AM +0200, Laurent Pinchart wrote: > On Mon, Feb 07, 2022 at 08:25:45PM +0100, Lucas Stach wrote: > > This adds the GPC and HSIO blk-ctrl nodes providing power control for > > the high-speed (USB and PCIe) IOs. > > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > > --- > > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 63 ++++++++++++++++++++--- > > 1 file changed, 57 insertions(+), 6 deletions(-) > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > index 6b840c05dd77..dc488a147d0c 100644 > > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > @@ -4,6 +4,7 @@ > > */ > > > > #include <dt-bindings/clock/imx8mp-clock.h> > > +#include <dt-bindings/power/imx8mp-power.h> > > #include <dt-bindings/gpio/gpio.h> > > #include <dt-bindings/input/input.h> > > #include <dt-bindings/interrupt-controller/arm-gic.h> > > @@ -475,6 +476,44 @@ src: reset-controller@30390000 { > > interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>; > > #reset-cells = <1>; > > }; > > + > > + gpc: gpc@303a0000 { > > + compatible = "fsl,imx8mp-gpc"; > > + reg = <0x303a0000 0x10000>; > > According to the reference manual, the GPC occupies 4kB, not 64kB. > > > + interrupt-parent = <&gic>; > > + interrupt-controller; > > + #interrupt-cells = <3>; > > + > > + pgc { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > We're working on support for the MEDIAMIX power domains, which we'll > rebase on top of this. In case the HSIO part still needs more work, > could you split this patch in two, with one patch that adds the PGC, > with an empty pgc nodde, and a second patch that adds the HSIO-related > power domains ? The first one could then be merged faster (it would be > great if it could get in v5.18). > > > + pgc_pcie_phy: power-domain@1 { > > + #power-domain-cells = <0>; > > + reg = <IMX8MP_POWER_DOMAIN_PCIE_PHY>; > > + }; > > + > > + pgc_usb1_phy: power-domain@2 { > > + #power-domain-cells = <0>; > > + reg = <IMX8MP_POWER_DOMAIN_USB1_PHY>; > > + }; > > + > > + pgc_usb2_phy: power-domain@3 { > > + #power-domain-cells = <0>; > > + reg = <IMX8MP_POWER_DOMAIN_USB2_PHY>; > > + }; > > + > > + pgc_hsiomix: power-domains@17 { > > + #power-domain-cells = <0>; > > + reg = <IMX8MP_POWER_DOMAIN_HSIOMIX>; > > + clocks = <&clk IMX8MP_CLK_HSIO_AXI>, > > + <&clk IMX8MP_CLK_HSIO_ROOT>; > > + assigned-clocks = <&clk IMX8MP_CLK_HSIO_AXI>; > > + assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; > > + assigned-clock-rates = <500000000>; > > + }; > > + }; > > + }; > > }; > > > > aips2: bus@30400000 { > > @@ -908,6 +947,20 @@ ddr-pmu@3d800000 { > > interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>; > > }; > > > > + hsio_blk_ctrl: blk-ctrl@32f10000 { > > + compatible = "fsl,imx8mp-hsio-blk-ctrl", "syscon"; > > + reg = <0x32f10000 0x24>; > > + clocks = <&clk IMX8MP_CLK_USB_ROOT>, > > + <&clk IMX8MP_CLK_PCIE_ROOT>; > > + clock-names = "usb", "pcie"; > > + power-domains = <&pgc_hsiomix>, <&pgc_hsiomix>, > > + <&pgc_usb1_phy>, <&pgc_usb2_phy>, > > + <&pgc_hsiomix>, <&pgc_pcie_phy>; > > Would it be useful to rework the driver to avoid specifying the same > parent power domain multiple times in DT ? > > > + power-domain-names = "bus", "usb", "usb-phy1", > > + "usb-phy2", "pcie", "pcie-phy"; > > + #power-domain-cells = <1>; > > + }; Shouldn't this be in an aips4 node ? > > + > > usb3_phy0: usb-phy@381f0040 { > > compatible = "fsl,imx8mp-usb-phy"; > > reg = <0x381f0040 0x40>; > > @@ -915,6 +968,7 @@ usb3_phy0: usb-phy@381f0040 { > > clock-names = "phy"; > > assigned-clocks = <&clk IMX8MP_CLK_USB_PHY_REF>; > > assigned-clock-parents = <&clk IMX8MP_CLK_24M>; > > + power-domains = <&hsio_blk_ctrl IMX8MP_HSIOBLK_PD_USB_PHY1>; > > #phy-cells = <0>; > > status = "disabled"; > > }; > > @@ -926,6 +980,7 @@ usb3_0: usb@32f10100 { > > <&clk IMX8MP_CLK_USB_ROOT>; > > clock-names = "hsio", "suspend"; > > interrupts = <GIC_SPI 148 IRQ_TYPE_LEVEL_HIGH>; > > + power-domains = <&hsio_blk_ctrl IMX8MP_HSIOBLK_PD_USB>; > > #address-cells = <1>; > > #size-cells = <1>; > > dma-ranges = <0x40000000 0x40000000 0xc0000000>; > > @@ -939,9 +994,6 @@ usb_dwc3_0: usb@38100000 { > > <&clk IMX8MP_CLK_USB_CORE_REF>, > > <&clk IMX8MP_CLK_USB_ROOT>; > > clock-names = "bus_early", "ref", "suspend"; > > - assigned-clocks = <&clk IMX8MP_CLK_HSIO_AXI>; > > - assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; > > - assigned-clock-rates = <500000000>; > > interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>; > > phys = <&usb3_phy0>, <&usb3_phy0>; > > phy-names = "usb2-phy", "usb3-phy"; > > @@ -957,6 +1009,7 @@ usb3_phy1: usb-phy@382f0040 { > > clock-names = "phy"; > > assigned-clocks = <&clk IMX8MP_CLK_USB_PHY_REF>; > > assigned-clock-parents = <&clk IMX8MP_CLK_24M>; > > + power-domains = <&hsio_blk_ctrl IMX8MP_HSIOBLK_PD_USB_PHY2>; > > #phy-cells = <0>; > > }; > > > > @@ -967,6 +1020,7 @@ usb3_1: usb@32f10108 { > > <&clk IMX8MP_CLK_USB_ROOT>; > > clock-names = "hsio", "suspend"; > > interrupts = <GIC_SPI 149 IRQ_TYPE_LEVEL_HIGH>; > > + power-domains = <&hsio_blk_ctrl IMX8MP_HSIOBLK_PD_USB>; > > #address-cells = <1>; > > #size-cells = <1>; > > dma-ranges = <0x40000000 0x40000000 0xc0000000>; > > @@ -980,9 +1034,6 @@ usb_dwc3_1: usb@38200000 { > > <&clk IMX8MP_CLK_USB_CORE_REF>, > > <&clk IMX8MP_CLK_USB_ROOT>; > > clock-names = "bus_early", "ref", "suspend"; > > - assigned-clocks = <&clk IMX8MP_CLK_HSIO_AXI>; > > - assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; > > - assigned-clock-rates = <500000000>; > > interrupts = <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>; > > phys = <&usb3_phy1>, <&usb3_phy1>; > > phy-names = "usb2-phy", "usb3-phy"; > >
Hi Laurent, Am Samstag, dem 19.02.2022 um 09:40 +0200 schrieb Laurent Pinchart: > Hi Lucas, > > Thank you for the patch. > > On Mon, Feb 07, 2022 at 08:25:45PM +0100, Lucas Stach wrote: > > This adds the GPC and HSIO blk-ctrl nodes providing power control for > > the high-speed (USB and PCIe) IOs. > > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > > --- > > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 63 ++++++++++++++++++++--- > > 1 file changed, 57 insertions(+), 6 deletions(-) > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > index 6b840c05dd77..dc488a147d0c 100644 > > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > @@ -4,6 +4,7 @@ > > */ > > > > #include <dt-bindings/clock/imx8mp-clock.h> > > +#include <dt-bindings/power/imx8mp-power.h> > > #include <dt-bindings/gpio/gpio.h> > > #include <dt-bindings/input/input.h> > > #include <dt-bindings/interrupt-controller/arm-gic.h> > > @@ -475,6 +476,44 @@ src: reset-controller@30390000 { > > interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>; > > #reset-cells = <1>; > > }; > > + > > + gpc: gpc@303a0000 { > > + compatible = "fsl,imx8mp-gpc"; > > + reg = <0x303a0000 0x10000>; > > According to the reference manual, the GPC occupies 4kB, not 64kB. Right, will fix. > > > + interrupt-parent = <&gic>; > > + interrupt-controller; > > + #interrupt-cells = <3>; > > + > > + pgc { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > We're working on support for the MEDIAMIX power domains, which we'll > rebase on top of this. In case the HSIO part still needs more work, > could you split this patch in two, with one patch that adds the PGC, > with an empty pgc nodde, and a second patch that adds the HSIO-related > power domains ? The first one could then be merged faster (it would be > great if it could get in v5.18). I don't think the HSIO part is controversial. It seems to work well in my testing and DT binding is already reviewed. Not sure if someone is going to review it properly, but I hope that we can land it together with the reset of this series. > > > + pgc_pcie_phy: power-domain@1 { > > + #power-domain-cells = <0>; > > + reg = <IMX8MP_POWER_DOMAIN_PCIE_PHY>; > > + }; > > + > > + pgc_usb1_phy: power-domain@2 { > > + #power-domain-cells = <0>; > > + reg = <IMX8MP_POWER_DOMAIN_USB1_PHY>; > > + }; > > + > > + pgc_usb2_phy: power-domain@3 { > > + #power-domain-cells = <0>; > > + reg = <IMX8MP_POWER_DOMAIN_USB2_PHY>; > > + }; > > + > > + pgc_hsiomix: power-domains@17 { > > + #power-domain-cells = <0>; > > + reg = <IMX8MP_POWER_DOMAIN_HSIOMIX>; > > + clocks = <&clk IMX8MP_CLK_HSIO_AXI>, > > + <&clk IMX8MP_CLK_HSIO_ROOT>; > > + assigned-clocks = <&clk IMX8MP_CLK_HSIO_AXI>; > > + assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; > > + assigned-clock-rates = <500000000>; > > + }; > > + }; > > + }; > > }; > > > > aips2: bus@30400000 { > > @@ -908,6 +947,20 @@ ddr-pmu@3d800000 { > > interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>; > > }; > > > > + hsio_blk_ctrl: blk-ctrl@32f10000 { > > + compatible = "fsl,imx8mp-hsio-blk-ctrl", "syscon"; > > + reg = <0x32f10000 0x24>; > > + clocks = <&clk IMX8MP_CLK_USB_ROOT>, > > + <&clk IMX8MP_CLK_PCIE_ROOT>; > > + clock-names = "usb", "pcie"; > > + power-domains = <&pgc_hsiomix>, <&pgc_hsiomix>, > > + <&pgc_usb1_phy>, <&pgc_usb2_phy>, > > + <&pgc_hsiomix>, <&pgc_pcie_phy>; > > Would it be useful to rework the driver to avoid specifying the same > parent power domain multiple times in DT ? I don't think so. That's the pattern we used for all the other blk- ctrls, where each virtual power-domain in the blk-ctrl has its own handle to a upstream GPC power domain. I don't see why we would want to change this now. Regards, Lucas > > > + power-domain-names = "bus", "usb", "usb-phy1", > > + "usb-phy2", "pcie", "pcie-phy"; > > + #power-domain-cells = <1>; > > + }; > > + > > usb3_phy0: usb-phy@381f0040 { > > compatible = "fsl,imx8mp-usb-phy"; > > reg = <0x381f0040 0x40>; > > @@ -915,6 +968,7 @@ usb3_phy0: usb-phy@381f0040 { > > clock-names = "phy"; > > assigned-clocks = <&clk IMX8MP_CLK_USB_PHY_REF>; > > assigned-clock-parents = <&clk IMX8MP_CLK_24M>; > > + power-domains = <&hsio_blk_ctrl IMX8MP_HSIOBLK_PD_USB_PHY1>; > > #phy-cells = <0>; > > status = "disabled"; > > }; > > @@ -926,6 +980,7 @@ usb3_0: usb@32f10100 { > > <&clk IMX8MP_CLK_USB_ROOT>; > > clock-names = "hsio", "suspend"; > > interrupts = <GIC_SPI 148 IRQ_TYPE_LEVEL_HIGH>; > > + power-domains = <&hsio_blk_ctrl IMX8MP_HSIOBLK_PD_USB>; > > #address-cells = <1>; > > #size-cells = <1>; > > dma-ranges = <0x40000000 0x40000000 0xc0000000>; > > @@ -939,9 +994,6 @@ usb_dwc3_0: usb@38100000 { > > <&clk IMX8MP_CLK_USB_CORE_REF>, > > <&clk IMX8MP_CLK_USB_ROOT>; > > clock-names = "bus_early", "ref", "suspend"; > > - assigned-clocks = <&clk IMX8MP_CLK_HSIO_AXI>; > > - assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; > > - assigned-clock-rates = <500000000>; > > interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>; > > phys = <&usb3_phy0>, <&usb3_phy0>; > > phy-names = "usb2-phy", "usb3-phy"; > > @@ -957,6 +1009,7 @@ usb3_phy1: usb-phy@382f0040 { > > clock-names = "phy"; > > assigned-clocks = <&clk IMX8MP_CLK_USB_PHY_REF>; > > assigned-clock-parents = <&clk IMX8MP_CLK_24M>; > > + power-domains = <&hsio_blk_ctrl IMX8MP_HSIOBLK_PD_USB_PHY2>; > > #phy-cells = <0>; > > }; > > > > @@ -967,6 +1020,7 @@ usb3_1: usb@32f10108 { > > <&clk IMX8MP_CLK_USB_ROOT>; > > clock-names = "hsio", "suspend"; > > interrupts = <GIC_SPI 149 IRQ_TYPE_LEVEL_HIGH>; > > + power-domains = <&hsio_blk_ctrl IMX8MP_HSIOBLK_PD_USB>; > > #address-cells = <1>; > > #size-cells = <1>; > > dma-ranges = <0x40000000 0x40000000 0xc0000000>; > > @@ -980,9 +1034,6 @@ usb_dwc3_1: usb@38200000 { > > <&clk IMX8MP_CLK_USB_CORE_REF>, > > <&clk IMX8MP_CLK_USB_ROOT>; > > clock-names = "bus_early", "ref", "suspend"; > > - assigned-clocks = <&clk IMX8MP_CLK_HSIO_AXI>; > > - assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; > > - assigned-clock-rates = <500000000>; > > interrupts = <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>; > > phys = <&usb3_phy1>, <&usb3_phy1>; > > phy-names = "usb2-phy", "usb3-phy"; > > >
Hi Lucas, On Mon, Feb 21, 2022 at 11:05:00AM +0100, Lucas Stach wrote: > Am Samstag, dem 19.02.2022 um 09:40 +0200 schrieb Laurent Pinchart: > > On Mon, Feb 07, 2022 at 08:25:45PM +0100, Lucas Stach wrote: > > > This adds the GPC and HSIO blk-ctrl nodes providing power control for > > > the high-speed (USB and PCIe) IOs. > > > > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > > > --- > > > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 63 ++++++++++++++++++++--- > > > 1 file changed, 57 insertions(+), 6 deletions(-) > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > index 6b840c05dd77..dc488a147d0c 100644 > > > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > @@ -4,6 +4,7 @@ > > > */ > > > > > > #include <dt-bindings/clock/imx8mp-clock.h> > > > +#include <dt-bindings/power/imx8mp-power.h> > > > #include <dt-bindings/gpio/gpio.h> > > > #include <dt-bindings/input/input.h> > > > #include <dt-bindings/interrupt-controller/arm-gic.h> > > > @@ -475,6 +476,44 @@ src: reset-controller@30390000 { > > > interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>; > > > #reset-cells = <1>; > > > }; > > > + > > > + gpc: gpc@303a0000 { > > > + compatible = "fsl,imx8mp-gpc"; > > > + reg = <0x303a0000 0x10000>; > > > > According to the reference manual, the GPC occupies 4kB, not 64kB. > > Right, will fix. > > > > + interrupt-parent = <&gic>; > > > + interrupt-controller; > > > + #interrupt-cells = <3>; > > > + > > > + pgc { > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + > > > > We're working on support for the MEDIAMIX power domains, which we'll > > rebase on top of this. In case the HSIO part still needs more work, > > could you split this patch in two, with one patch that adds the PGC, > > with an empty pgc nodde, and a second patch that adds the HSIO-related > > power domains ? The first one could then be merged faster (it would be > > great if it could get in v5.18). > > I don't think the HSIO part is controversial. It seems to work well in > my testing and DT binding is already reviewed. Not sure if someone is > going to review it properly, but I hope that we can land it together > with the reset of this series. I'll give it a review. > > > + pgc_pcie_phy: power-domain@1 { > > > + #power-domain-cells = <0>; > > > + reg = <IMX8MP_POWER_DOMAIN_PCIE_PHY>; > > > + }; > > > + > > > + pgc_usb1_phy: power-domain@2 { > > > + #power-domain-cells = <0>; > > > + reg = <IMX8MP_POWER_DOMAIN_USB1_PHY>; > > > + }; > > > + > > > + pgc_usb2_phy: power-domain@3 { > > > + #power-domain-cells = <0>; > > > + reg = <IMX8MP_POWER_DOMAIN_USB2_PHY>; > > > + }; > > > + > > > + pgc_hsiomix: power-domains@17 { > > > + #power-domain-cells = <0>; > > > + reg = <IMX8MP_POWER_DOMAIN_HSIOMIX>; > > > + clocks = <&clk IMX8MP_CLK_HSIO_AXI>, > > > + <&clk IMX8MP_CLK_HSIO_ROOT>; > > > + assigned-clocks = <&clk IMX8MP_CLK_HSIO_AXI>; > > > + assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; > > > + assigned-clock-rates = <500000000>; > > > + }; > > > + }; > > > + }; > > > }; > > > > > > aips2: bus@30400000 { > > > @@ -908,6 +947,20 @@ ddr-pmu@3d800000 { > > > interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>; > > > }; > > > > > > + hsio_blk_ctrl: blk-ctrl@32f10000 { > > > + compatible = "fsl,imx8mp-hsio-blk-ctrl", "syscon"; > > > + reg = <0x32f10000 0x24>; > > > + clocks = <&clk IMX8MP_CLK_USB_ROOT>, > > > + <&clk IMX8MP_CLK_PCIE_ROOT>; > > > + clock-names = "usb", "pcie"; > > > + power-domains = <&pgc_hsiomix>, <&pgc_hsiomix>, > > > + <&pgc_usb1_phy>, <&pgc_usb2_phy>, > > > + <&pgc_hsiomix>, <&pgc_pcie_phy>; > > > > Would it be useful to rework the driver to avoid specifying the same > > parent power domain multiple times in DT ? > > I don't think so. That's the pattern we used for all the other blk- > ctrls, where each virtual power-domain in the blk-ctrl has its own > handle to a upstream GPC power domain. I don't see why we would want to > change this now. It's a small optimization, but probably not really important. > > > + power-domain-names = "bus", "usb", "usb-phy1", > > > + "usb-phy2", "pcie", "pcie-phy"; > > > + #power-domain-cells = <1>; > > > + }; > > > + > > > usb3_phy0: usb-phy@381f0040 { > > > compatible = "fsl,imx8mp-usb-phy"; > > > reg = <0x381f0040 0x40>; > > > @@ -915,6 +968,7 @@ usb3_phy0: usb-phy@381f0040 { > > > clock-names = "phy"; > > > assigned-clocks = <&clk IMX8MP_CLK_USB_PHY_REF>; > > > assigned-clock-parents = <&clk IMX8MP_CLK_24M>; > > > + power-domains = <&hsio_blk_ctrl IMX8MP_HSIOBLK_PD_USB_PHY1>; > > > #phy-cells = <0>; > > > status = "disabled"; > > > }; > > > @@ -926,6 +980,7 @@ usb3_0: usb@32f10100 { > > > <&clk IMX8MP_CLK_USB_ROOT>; > > > clock-names = "hsio", "suspend"; > > > interrupts = <GIC_SPI 148 IRQ_TYPE_LEVEL_HIGH>; > > > + power-domains = <&hsio_blk_ctrl IMX8MP_HSIOBLK_PD_USB>; > > > #address-cells = <1>; > > > #size-cells = <1>; > > > dma-ranges = <0x40000000 0x40000000 0xc0000000>; > > > @@ -939,9 +994,6 @@ usb_dwc3_0: usb@38100000 { > > > <&clk IMX8MP_CLK_USB_CORE_REF>, > > > <&clk IMX8MP_CLK_USB_ROOT>; > > > clock-names = "bus_early", "ref", "suspend"; > > > - assigned-clocks = <&clk IMX8MP_CLK_HSIO_AXI>; > > > - assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; > > > - assigned-clock-rates = <500000000>; > > > interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>; > > > phys = <&usb3_phy0>, <&usb3_phy0>; > > > phy-names = "usb2-phy", "usb3-phy"; > > > @@ -957,6 +1009,7 @@ usb3_phy1: usb-phy@382f0040 { > > > clock-names = "phy"; > > > assigned-clocks = <&clk IMX8MP_CLK_USB_PHY_REF>; > > > assigned-clock-parents = <&clk IMX8MP_CLK_24M>; > > > + power-domains = <&hsio_blk_ctrl IMX8MP_HSIOBLK_PD_USB_PHY2>; > > > #phy-cells = <0>; > > > }; > > > > > > @@ -967,6 +1020,7 @@ usb3_1: usb@32f10108 { > > > <&clk IMX8MP_CLK_USB_ROOT>; > > > clock-names = "hsio", "suspend"; > > > interrupts = <GIC_SPI 149 IRQ_TYPE_LEVEL_HIGH>; > > > + power-domains = <&hsio_blk_ctrl IMX8MP_HSIOBLK_PD_USB>; > > > #address-cells = <1>; > > > #size-cells = <1>; > > > dma-ranges = <0x40000000 0x40000000 0xc0000000>; > > > @@ -980,9 +1034,6 @@ usb_dwc3_1: usb@38200000 { > > > <&clk IMX8MP_CLK_USB_CORE_REF>, > > > <&clk IMX8MP_CLK_USB_ROOT>; > > > clock-names = "bus_early", "ref", "suspend"; > > > - assigned-clocks = <&clk IMX8MP_CLK_HSIO_AXI>; > > > - assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; > > > - assigned-clock-rates = <500000000>; > > > interrupts = <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>; > > > phys = <&usb3_phy1>, <&usb3_phy1>; > > > phy-names = "usb2-phy", "usb3-phy"; > > >
Hi Lucas, On Sun, Feb 20, 2022 at 07:56:11AM +0200, Laurent Pinchart wrote: > On Sat, Feb 19, 2022 at 09:40:50AM +0200, Laurent Pinchart wrote: > > On Mon, Feb 07, 2022 at 08:25:45PM +0100, Lucas Stach wrote: > > > This adds the GPC and HSIO blk-ctrl nodes providing power control for > > > the high-speed (USB and PCIe) IOs. > > > > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > > > --- > > > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 63 ++++++++++++++++++++--- > > > 1 file changed, 57 insertions(+), 6 deletions(-) > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > index 6b840c05dd77..dc488a147d0c 100644 > > > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > @@ -4,6 +4,7 @@ > > > */ > > > > > > #include <dt-bindings/clock/imx8mp-clock.h> > > > +#include <dt-bindings/power/imx8mp-power.h> > > > #include <dt-bindings/gpio/gpio.h> > > > #include <dt-bindings/input/input.h> > > > #include <dt-bindings/interrupt-controller/arm-gic.h> > > > @@ -475,6 +476,44 @@ src: reset-controller@30390000 { > > > interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>; > > > #reset-cells = <1>; > > > }; > > > + > > > + gpc: gpc@303a0000 { > > > + compatible = "fsl,imx8mp-gpc"; > > > + reg = <0x303a0000 0x10000>; > > > > According to the reference manual, the GPC occupies 4kB, not 64kB. > > > > > + interrupt-parent = <&gic>; > > > + interrupt-controller; > > > + #interrupt-cells = <3>; > > > + > > > + pgc { > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + > > > > We're working on support for the MEDIAMIX power domains, which we'll > > rebase on top of this. In case the HSIO part still needs more work, > > could you split this patch in two, with one patch that adds the PGC, > > with an empty pgc nodde, and a second patch that adds the HSIO-related > > power domains ? The first one could then be merged faster (it would be > > great if it could get in v5.18). > > > > > + pgc_pcie_phy: power-domain@1 { > > > + #power-domain-cells = <0>; > > > + reg = <IMX8MP_POWER_DOMAIN_PCIE_PHY>; > > > + }; > > > + > > > + pgc_usb1_phy: power-domain@2 { > > > + #power-domain-cells = <0>; > > > + reg = <IMX8MP_POWER_DOMAIN_USB1_PHY>; > > > + }; > > > + > > > + pgc_usb2_phy: power-domain@3 { > > > + #power-domain-cells = <0>; > > > + reg = <IMX8MP_POWER_DOMAIN_USB2_PHY>; > > > + }; > > > + > > > + pgc_hsiomix: power-domains@17 { > > > + #power-domain-cells = <0>; > > > + reg = <IMX8MP_POWER_DOMAIN_HSIOMIX>; > > > + clocks = <&clk IMX8MP_CLK_HSIO_AXI>, > > > + <&clk IMX8MP_CLK_HSIO_ROOT>; > > > + assigned-clocks = <&clk IMX8MP_CLK_HSIO_AXI>; > > > + assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; > > > + assigned-clock-rates = <500000000>; > > > + }; > > > + }; > > > + }; > > > }; > > > > > > aips2: bus@30400000 { > > > @@ -908,6 +947,20 @@ ddr-pmu@3d800000 { > > > interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>; > > > }; > > > > > > + hsio_blk_ctrl: blk-ctrl@32f10000 { > > > + compatible = "fsl,imx8mp-hsio-blk-ctrl", "syscon"; > > > + reg = <0x32f10000 0x24>; > > > + clocks = <&clk IMX8MP_CLK_USB_ROOT>, > > > + <&clk IMX8MP_CLK_PCIE_ROOT>; > > > + clock-names = "usb", "pcie"; > > > + power-domains = <&pgc_hsiomix>, <&pgc_hsiomix>, > > > + <&pgc_usb1_phy>, <&pgc_usb2_phy>, > > > + <&pgc_hsiomix>, <&pgc_pcie_phy>; > > > > Would it be useful to rework the driver to avoid specifying the same > > parent power domain multiple times in DT ? > > > > > + power-domain-names = "bus", "usb", "usb-phy1", > > > + "usb-phy2", "pcie", "pcie-phy"; > > > + #power-domain-cells = <1>; > > > + }; > > Shouldn't this be in an aips4 node ? With the reg size fixed, and this device moved to an aips4 node, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > + > > > usb3_phy0: usb-phy@381f0040 { > > > compatible = "fsl,imx8mp-usb-phy"; > > > reg = <0x381f0040 0x40>; > > > @@ -915,6 +968,7 @@ usb3_phy0: usb-phy@381f0040 { > > > clock-names = "phy"; > > > assigned-clocks = <&clk IMX8MP_CLK_USB_PHY_REF>; > > > assigned-clock-parents = <&clk IMX8MP_CLK_24M>; > > > + power-domains = <&hsio_blk_ctrl IMX8MP_HSIOBLK_PD_USB_PHY1>; > > > #phy-cells = <0>; > > > status = "disabled"; > > > }; > > > @@ -926,6 +980,7 @@ usb3_0: usb@32f10100 { > > > <&clk IMX8MP_CLK_USB_ROOT>; > > > clock-names = "hsio", "suspend"; > > > interrupts = <GIC_SPI 148 IRQ_TYPE_LEVEL_HIGH>; > > > + power-domains = <&hsio_blk_ctrl IMX8MP_HSIOBLK_PD_USB>; > > > #address-cells = <1>; > > > #size-cells = <1>; > > > dma-ranges = <0x40000000 0x40000000 0xc0000000>; > > > @@ -939,9 +994,6 @@ usb_dwc3_0: usb@38100000 { > > > <&clk IMX8MP_CLK_USB_CORE_REF>, > > > <&clk IMX8MP_CLK_USB_ROOT>; > > > clock-names = "bus_early", "ref", "suspend"; > > > - assigned-clocks = <&clk IMX8MP_CLK_HSIO_AXI>; > > > - assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; > > > - assigned-clock-rates = <500000000>; > > > interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>; > > > phys = <&usb3_phy0>, <&usb3_phy0>; > > > phy-names = "usb2-phy", "usb3-phy"; > > > @@ -957,6 +1009,7 @@ usb3_phy1: usb-phy@382f0040 { > > > clock-names = "phy"; > > > assigned-clocks = <&clk IMX8MP_CLK_USB_PHY_REF>; > > > assigned-clock-parents = <&clk IMX8MP_CLK_24M>; > > > + power-domains = <&hsio_blk_ctrl IMX8MP_HSIOBLK_PD_USB_PHY2>; > > > #phy-cells = <0>; > > > }; > > > > > > @@ -967,6 +1020,7 @@ usb3_1: usb@32f10108 { > > > <&clk IMX8MP_CLK_USB_ROOT>; > > > clock-names = "hsio", "suspend"; > > > interrupts = <GIC_SPI 149 IRQ_TYPE_LEVEL_HIGH>; > > > + power-domains = <&hsio_blk_ctrl IMX8MP_HSIOBLK_PD_USB>; > > > #address-cells = <1>; > > > #size-cells = <1>; > > > dma-ranges = <0x40000000 0x40000000 0xc0000000>; > > > @@ -980,9 +1034,6 @@ usb_dwc3_1: usb@38200000 { > > > <&clk IMX8MP_CLK_USB_CORE_REF>, > > > <&clk IMX8MP_CLK_USB_ROOT>; > > > clock-names = "bus_early", "ref", "suspend"; > > > - assigned-clocks = <&clk IMX8MP_CLK_HSIO_AXI>; > > > - assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; > > > - assigned-clock-rates = <500000000>; > > > interrupts = <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>; > > > phys = <&usb3_phy1>, <&usb3_phy1>; > > > phy-names = "usb2-phy", "usb3-phy"; > > >
diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi index 6b840c05dd77..dc488a147d0c 100644 --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi @@ -4,6 +4,7 @@ */ #include <dt-bindings/clock/imx8mp-clock.h> +#include <dt-bindings/power/imx8mp-power.h> #include <dt-bindings/gpio/gpio.h> #include <dt-bindings/input/input.h> #include <dt-bindings/interrupt-controller/arm-gic.h> @@ -475,6 +476,44 @@ src: reset-controller@30390000 { interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>; #reset-cells = <1>; }; + + gpc: gpc@303a0000 { + compatible = "fsl,imx8mp-gpc"; + reg = <0x303a0000 0x10000>; + interrupt-parent = <&gic>; + interrupt-controller; + #interrupt-cells = <3>; + + pgc { + #address-cells = <1>; + #size-cells = <0>; + + pgc_pcie_phy: power-domain@1 { + #power-domain-cells = <0>; + reg = <IMX8MP_POWER_DOMAIN_PCIE_PHY>; + }; + + pgc_usb1_phy: power-domain@2 { + #power-domain-cells = <0>; + reg = <IMX8MP_POWER_DOMAIN_USB1_PHY>; + }; + + pgc_usb2_phy: power-domain@3 { + #power-domain-cells = <0>; + reg = <IMX8MP_POWER_DOMAIN_USB2_PHY>; + }; + + pgc_hsiomix: power-domains@17 { + #power-domain-cells = <0>; + reg = <IMX8MP_POWER_DOMAIN_HSIOMIX>; + clocks = <&clk IMX8MP_CLK_HSIO_AXI>, + <&clk IMX8MP_CLK_HSIO_ROOT>; + assigned-clocks = <&clk IMX8MP_CLK_HSIO_AXI>; + assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; + assigned-clock-rates = <500000000>; + }; + }; + }; }; aips2: bus@30400000 { @@ -908,6 +947,20 @@ ddr-pmu@3d800000 { interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>; }; + hsio_blk_ctrl: blk-ctrl@32f10000 { + compatible = "fsl,imx8mp-hsio-blk-ctrl", "syscon"; + reg = <0x32f10000 0x24>; + clocks = <&clk IMX8MP_CLK_USB_ROOT>, + <&clk IMX8MP_CLK_PCIE_ROOT>; + clock-names = "usb", "pcie"; + power-domains = <&pgc_hsiomix>, <&pgc_hsiomix>, + <&pgc_usb1_phy>, <&pgc_usb2_phy>, + <&pgc_hsiomix>, <&pgc_pcie_phy>; + power-domain-names = "bus", "usb", "usb-phy1", + "usb-phy2", "pcie", "pcie-phy"; + #power-domain-cells = <1>; + }; + usb3_phy0: usb-phy@381f0040 { compatible = "fsl,imx8mp-usb-phy"; reg = <0x381f0040 0x40>; @@ -915,6 +968,7 @@ usb3_phy0: usb-phy@381f0040 { clock-names = "phy"; assigned-clocks = <&clk IMX8MP_CLK_USB_PHY_REF>; assigned-clock-parents = <&clk IMX8MP_CLK_24M>; + power-domains = <&hsio_blk_ctrl IMX8MP_HSIOBLK_PD_USB_PHY1>; #phy-cells = <0>; status = "disabled"; }; @@ -926,6 +980,7 @@ usb3_0: usb@32f10100 { <&clk IMX8MP_CLK_USB_ROOT>; clock-names = "hsio", "suspend"; interrupts = <GIC_SPI 148 IRQ_TYPE_LEVEL_HIGH>; + power-domains = <&hsio_blk_ctrl IMX8MP_HSIOBLK_PD_USB>; #address-cells = <1>; #size-cells = <1>; dma-ranges = <0x40000000 0x40000000 0xc0000000>; @@ -939,9 +994,6 @@ usb_dwc3_0: usb@38100000 { <&clk IMX8MP_CLK_USB_CORE_REF>, <&clk IMX8MP_CLK_USB_ROOT>; clock-names = "bus_early", "ref", "suspend"; - assigned-clocks = <&clk IMX8MP_CLK_HSIO_AXI>; - assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; - assigned-clock-rates = <500000000>; interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>; phys = <&usb3_phy0>, <&usb3_phy0>; phy-names = "usb2-phy", "usb3-phy"; @@ -957,6 +1009,7 @@ usb3_phy1: usb-phy@382f0040 { clock-names = "phy"; assigned-clocks = <&clk IMX8MP_CLK_USB_PHY_REF>; assigned-clock-parents = <&clk IMX8MP_CLK_24M>; + power-domains = <&hsio_blk_ctrl IMX8MP_HSIOBLK_PD_USB_PHY2>; #phy-cells = <0>; }; @@ -967,6 +1020,7 @@ usb3_1: usb@32f10108 { <&clk IMX8MP_CLK_USB_ROOT>; clock-names = "hsio", "suspend"; interrupts = <GIC_SPI 149 IRQ_TYPE_LEVEL_HIGH>; + power-domains = <&hsio_blk_ctrl IMX8MP_HSIOBLK_PD_USB>; #address-cells = <1>; #size-cells = <1>; dma-ranges = <0x40000000 0x40000000 0xc0000000>; @@ -980,9 +1034,6 @@ usb_dwc3_1: usb@38200000 { <&clk IMX8MP_CLK_USB_CORE_REF>, <&clk IMX8MP_CLK_USB_ROOT>; clock-names = "bus_early", "ref", "suspend"; - assigned-clocks = <&clk IMX8MP_CLK_HSIO_AXI>; - assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>; - assigned-clock-rates = <500000000>; interrupts = <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>; phys = <&usb3_phy1>, <&usb3_phy1>; phy-names = "usb2-phy", "usb3-phy";
This adds the GPC and HSIO blk-ctrl nodes providing power control for the high-speed (USB and PCIe) IOs. Signed-off-by: Lucas Stach <l.stach@pengutronix.de> --- arch/arm64/boot/dts/freescale/imx8mp.dtsi | 63 ++++++++++++++++++++--- 1 file changed, 57 insertions(+), 6 deletions(-)