* [PATCH v7 0/2] GPCv2 power gating driver @ 2017-03-21 14:50 Andrey Smirnov 2017-03-21 14:50 ` [PATCH v7 1/2] dt-bindings: Add " Andrey Smirnov 2017-03-21 14:50 ` [PATCH v7 2/2] soc/imx: " Andrey Smirnov 0 siblings, 2 replies; 16+ messages in thread From: Andrey Smirnov @ 2017-03-21 14:50 UTC (permalink / raw) To: Shawn Guo Cc: Andrey Smirnov, yurovsky, Lucas Stach, Fabio Estevam, Dong Aisheng, linux-arm-kernel, linux-kernel Shawn, Here's v7 of GPCv2 patches with feedback from Dong incorporated. Let me know if you want to do any of renaming that Dong suggested and if there's more to be improved in general. Thanks, Andrey Smirnov Changes since v6 (see [v6]): - Drop .readable_reg related code - Switch regmap to REGCACHE_NONE and drop .volatile_reg - Remove unnecessary #include directives Changes since v5 (see [v5]): - Apply reverse tree variable declartaion fromatting to imx_gpcv2_probe() and imx7_gpc_pu_pgc_sw_pxx_req() - Drop patch #3 in favour of defaulting to y in Kconfig Changes since v4 (see [v4]): - Single patch is split into multiple as per Shawn's reques - Incorporated various feedback from Shawn Changes since v3 (see [v3]): - Minor device tree bindings documentation fixes as per feedback from Rob Herring - Collect Acked-by from Rob Changes since v2 (see [v2]): - Fix a critical bug where incorrect state of a bit was expected in a busy wait loop (bit set instead of bit cleared) imx7_gpc_pu_pgc_sw_pxx_req() - Add missing step (setting of PCR in GPC_PGC_nCTRL) in power down procedure Changes since v1 (see [v1]): - Various small DT bindings description fixes as per feedback from Rob Herring [v1] https://lkml.org/lkml/2017/2/6/554 [v2] https://lkml.org/lkml/2017/2/13/489 [v3] https://lkml.org/lkml/2017/2/20/338 [v4] https://lkml.org/lkml/2017/2/28/738 [v5] https://lkml.org/lkml/2017/3/14/547 [v6] https://lkml.org/lkml/2017/3/16/357 Andrey Smirnov (2): dt-bindings: Add GPCv2 power gating driver soc/imx: Add GPCv2 power gating driver .../devicetree/bindings/power/fsl,imx-gpcv2.txt | 71 ++++ drivers/soc/Kconfig | 1 + drivers/soc/imx/Kconfig | 10 + drivers/soc/imx/Makefile | 1 + drivers/soc/imx/gpcv2.c | 365 +++++++++++++++++++++ include/dt-bindings/power/imx7-power.h | 18 + 6 files changed, 466 insertions(+) create mode 100644 Documentation/devicetree/bindings/power/fsl,imx-gpcv2.txt create mode 100644 drivers/soc/imx/Kconfig create mode 100644 drivers/soc/imx/gpcv2.c create mode 100644 include/dt-bindings/power/imx7-power.h -- 2.9.3 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v7 1/2] dt-bindings: Add GPCv2 power gating driver 2017-03-21 14:50 [PATCH v7 0/2] GPCv2 power gating driver Andrey Smirnov @ 2017-03-21 14:50 ` Andrey Smirnov 2017-03-24 6:32 ` Dong Aisheng 2017-03-21 14:50 ` [PATCH v7 2/2] soc/imx: " Andrey Smirnov 1 sibling, 1 reply; 16+ messages in thread From: Andrey Smirnov @ 2017-03-21 14:50 UTC (permalink / raw) To: Shawn Guo Cc: Andrey Smirnov, yurovsky, Lucas Stach, Rob Herring, Mark Rutland, Fabio Estevam, Dong Aisheng, devicetree, linux-arm-kernel, linux-kernel Add DT bindings for power domain driver for GPCv2 IP block found in i.MX7 SoCs. Cc: yurovsky@gmail.com Cc: Lucas Stach <l.stach@pengutronix.de> Cc: Rob Herring <robh+dt@kernel.org> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Fabio Estevam <fabio.estevam@nxp.com> Cc: Dong Aisheng <dongas86@gmail.com> Cc: devicetree@vger.kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-kernel@vger.kernel.org Acked-by: Rob Herring <robh@kernel.org> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> --- .../devicetree/bindings/power/fsl,imx-gpcv2.txt | 71 ++++++++++++++++++++++ include/dt-bindings/power/imx7-power.h | 18 ++++++ 2 files changed, 89 insertions(+) create mode 100644 Documentation/devicetree/bindings/power/fsl,imx-gpcv2.txt create mode 100644 include/dt-bindings/power/imx7-power.h diff --git a/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.txt b/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.txt new file mode 100644 index 0000000..02f45c6 --- /dev/null +++ b/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.txt @@ -0,0 +1,71 @@ +Freescale i.MX General Power Controller v2 +========================================== + +The i.MX7S/D General Power Control (GPC) block contains Power Gating +Control (PGC) for various power domains. + +Required properties: + +- compatible: Should be "fsl,imx7d-gpc" + +- reg: should be register base and length as documented in the + datasheet + +- interrupts: Should contain GPC interrupt request 1 + +Power domains contained within GPC node are generic power domain +providers, documented in +Documentation/devicetree/bindings/power/power_domain.txt, which are +described as subnodes of the power gating controller 'pgc' node, +which, in turn, is expected to contain the following: + +Required properties: + +- reg: Power domain index. Valid values are defined in + include/dt-bindings/power/imx7-power.h + +- #power-domain-cells: Should be 0 + +Optional properties: + +- power-supply: Power supply used to power the domain + +Example: + + gpc: gpc@303a0000 { + compatible = "fsl,imx7d-gpc"; + reg = <0x303a0000 0x1000>; + interrupt-controller; + interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>; + #interrupt-cells = <3>; + interrupt-parent = <&intc>; + + pgc { + #address-cells = <1>; + #size-cells = <0>; + + pgc_pcie_phy: power-domain@3 { + #power-domain-cells = <0>; + + reg = <IMX7_POWER_DOMAIN_PCIE_PHY>; + power-supply = <®_1p0d>; + }; + }; + }; + + +Specifying power domain for IP modules +====================================== + +IP cores belonging to a power domain should contain a 'power-domains' +property that is a phandle for PGC node representing the domain. + +Example of a device that is part of the PCIE_PHY power domain: + + pcie: pcie@33800000 { + reg = <0x33800000 0x4000>, + <0x4ff00000 0x80000>; + /* ... */ + power-domains = <&pgc_pcie_phy>; + /* ... */ + }; diff --git a/include/dt-bindings/power/imx7-power.h b/include/dt-bindings/power/imx7-power.h new file mode 100644 index 0000000..eb70023 --- /dev/null +++ b/include/dt-bindings/power/imx7-power.h @@ -0,0 +1,18 @@ +/* + * Copyright (C) 2017 Impinj + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#ifndef __DT_BINDINGS_IMX7_POWER_H__ +#define __DT_BINDINGS_IMX7_POWER_H__ + +#define IMX7_POWER_DOMAIN_USB_HSIC_PHY 0 +#define IMX7_POWER_DOMAIN_USB_OTG2_PHY 1 +#define IMX7_POWER_DOMAIN_USB_OTG1_PHY 2 +#define IMX7_POWER_DOMAIN_PCIE_PHY 3 +#define IMX7_POWER_DOMAIN_MIPI_PHY 4 + +#endif -- 2.9.3 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v7 1/2] dt-bindings: Add GPCv2 power gating driver 2017-03-21 14:50 ` [PATCH v7 1/2] dt-bindings: Add " Andrey Smirnov @ 2017-03-24 6:32 ` Dong Aisheng 2017-03-27 18:42 ` Andrey Smirnov 0 siblings, 1 reply; 16+ messages in thread From: Dong Aisheng @ 2017-03-24 6:32 UTC (permalink / raw) To: Andrey Smirnov Cc: Shawn Guo, yurovsky, Lucas Stach, Rob Herring, Mark Rutland, Fabio Estevam, devicetree, linux-arm-kernel, linux-kernel On Tue, Mar 21, 2017 at 07:50:03AM -0700, Andrey Smirnov wrote: > Add DT bindings for power domain driver for GPCv2 IP block found in > i.MX7 SoCs. > > Cc: yurovsky@gmail.com > Cc: Lucas Stach <l.stach@pengutronix.de> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Fabio Estevam <fabio.estevam@nxp.com> > Cc: Dong Aisheng <dongas86@gmail.com> > Cc: devicetree@vger.kernel.org > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > Acked-by: Rob Herring <robh@kernel.org> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > --- > .../devicetree/bindings/power/fsl,imx-gpcv2.txt | 71 ++++++++++++++++++++++ > include/dt-bindings/power/imx7-power.h | 18 ++++++ > 2 files changed, 89 insertions(+) > create mode 100644 Documentation/devicetree/bindings/power/fsl,imx-gpcv2.txt > create mode 100644 include/dt-bindings/power/imx7-power.h > > diff --git a/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.txt b/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.txt > new file mode 100644 > index 0000000..02f45c6 > --- /dev/null > +++ b/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.txt > @@ -0,0 +1,71 @@ > +Freescale i.MX General Power Controller v2 > +========================================== > + > +The i.MX7S/D General Power Control (GPC) block contains Power Gating > +Control (PGC) for various power domains. > + > +Required properties: > + > +- compatible: Should be "fsl,imx7d-gpc" > + > +- reg: should be register base and length as documented in the > + datasheet > + > +- interrupts: Should contain GPC interrupt request 1 > + > +Power domains contained within GPC node are generic power domain > +providers, documented in > +Documentation/devicetree/bindings/power/power_domain.txt, which are > +described as subnodes of the power gating controller 'pgc' node, > +which, in turn, is expected to contain the following: > + > +Required properties: > + > +- reg: Power domain index. Valid values are defined in > + include/dt-bindings/power/imx7-power.h > + > +- #power-domain-cells: Should be 0 > + > +Optional properties: > + > +- power-supply: Power supply used to power the domain > + > +Example: > + > + gpc: gpc@303a0000 { > + compatible = "fsl,imx7d-gpc"; > + reg = <0x303a0000 0x1000>; > + interrupt-controller; > + interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>; > + #interrupt-cells = <3>; > + interrupt-parent = <&intc>; > + > + pgc { > + #address-cells = <1>; > + #size-cells = <0>; > + > + pgc_pcie_phy: power-domain@3 { > + #power-domain-cells = <0>; > + > + reg = <IMX7_POWER_DOMAIN_PCIE_PHY>; > + power-supply = <®_1p0d>; > + }; > + }; > + }; > + > + > +Specifying power domain for IP modules > +====================================== > + > +IP cores belonging to a power domain should contain a 'power-domains' > +property that is a phandle for PGC node representing the domain. > + > +Example of a device that is part of the PCIE_PHY power domain: > + > + pcie: pcie@33800000 { > + reg = <0x33800000 0x4000>, > + <0x4ff00000 0x80000>; > + /* ... */ > + power-domains = <&pgc_pcie_phy>; > + /* ... */ > + }; > diff --git a/include/dt-bindings/power/imx7-power.h b/include/dt-bindings/power/imx7-power.h > new file mode 100644 > index 0000000..eb70023 > --- /dev/null > +++ b/include/dt-bindings/power/imx7-power.h > @@ -0,0 +1,18 @@ > +/* > + * Copyright (C) 2017 Impinj > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#ifndef __DT_BINDINGS_IMX7_POWER_H__ > +#define __DT_BINDINGS_IMX7_POWER_H__ > + > +#define IMX7_POWER_DOMAIN_USB_HSIC_PHY 0 > +#define IMX7_POWER_DOMAIN_USB_OTG2_PHY 1 > +#define IMX7_POWER_DOMAIN_USB_OTG1_PHY 2 > +#define IMX7_POWER_DOMAIN_PCIE_PHY 3 > +#define IMX7_POWER_DOMAIN_MIPI_PHY 4 Nitpick: Probably better to define according to the reference manual defined order. 0x800 ~ 0x83F : PGC for A7 core0 0x840 ~ 0x87F: PGC for A7 core1 0x880 ~ 0x8BF: PGC for A7 SCU 0xA00 ~ 0xA3F: PGC for fastmix/megamix 0xC00 ~ 0xC3F: PGC for MIPI PHY 0xC40 ~ 0xC7F: PGC for PCIE_PHY 0xC80 ~ 0xCBF: Reserved 0xCC0 ~ 0xCFF: Reserved 0xD00 ~ 0xD3F: PGC for USB HSIC PHY You can drop A7 core/scu/fastmix/megamix as well. > + > +#endif > -- > 2.9.3 > Regards Dong Aisheng ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v7 1/2] dt-bindings: Add GPCv2 power gating driver 2017-03-24 6:32 ` Dong Aisheng @ 2017-03-27 18:42 ` Andrey Smirnov 2017-03-30 7:15 ` Dong Aisheng 0 siblings, 1 reply; 16+ messages in thread From: Andrey Smirnov @ 2017-03-27 18:42 UTC (permalink / raw) To: Dong Aisheng Cc: Shawn Guo, Andrey Yurovsky, Lucas Stach, Rob Herring, Mark Rutland, Fabio Estevam, devicetree, linux-arm-kernel, linux-kernel On Thu, Mar 23, 2017 at 11:32 PM, Dong Aisheng <dongas86@gmail.com> wrote: > On Tue, Mar 21, 2017 at 07:50:03AM -0700, Andrey Smirnov wrote: >> Add DT bindings for power domain driver for GPCv2 IP block found in >> i.MX7 SoCs. >> >> Cc: yurovsky@gmail.com >> Cc: Lucas Stach <l.stach@pengutronix.de> >> Cc: Rob Herring <robh+dt@kernel.org> >> Cc: Mark Rutland <mark.rutland@arm.com> >> Cc: Fabio Estevam <fabio.estevam@nxp.com> >> Cc: Dong Aisheng <dongas86@gmail.com> >> Cc: devicetree@vger.kernel.org >> Cc: linux-arm-kernel@lists.infradead.org >> Cc: linux-kernel@vger.kernel.org >> Acked-by: Rob Herring <robh@kernel.org> >> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> >> --- >> .../devicetree/bindings/power/fsl,imx-gpcv2.txt | 71 ++++++++++++++++++++++ >> include/dt-bindings/power/imx7-power.h | 18 ++++++ >> 2 files changed, 89 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/power/fsl,imx-gpcv2.txt >> create mode 100644 include/dt-bindings/power/imx7-power.h >> >> diff --git a/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.txt b/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.txt >> new file mode 100644 >> index 0000000..02f45c6 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.txt >> @@ -0,0 +1,71 @@ >> +Freescale i.MX General Power Controller v2 >> +========================================== >> + >> +The i.MX7S/D General Power Control (GPC) block contains Power Gating >> +Control (PGC) for various power domains. >> + >> +Required properties: >> + >> +- compatible: Should be "fsl,imx7d-gpc" >> + >> +- reg: should be register base and length as documented in the >> + datasheet >> + >> +- interrupts: Should contain GPC interrupt request 1 >> + >> +Power domains contained within GPC node are generic power domain >> +providers, documented in >> +Documentation/devicetree/bindings/power/power_domain.txt, which are >> +described as subnodes of the power gating controller 'pgc' node, >> +which, in turn, is expected to contain the following: >> + >> +Required properties: >> + >> +- reg: Power domain index. Valid values are defined in >> + include/dt-bindings/power/imx7-power.h >> + >> +- #power-domain-cells: Should be 0 >> + >> +Optional properties: >> + >> +- power-supply: Power supply used to power the domain >> + >> +Example: >> + >> + gpc: gpc@303a0000 { >> + compatible = "fsl,imx7d-gpc"; >> + reg = <0x303a0000 0x1000>; >> + interrupt-controller; >> + interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>; >> + #interrupt-cells = <3>; >> + interrupt-parent = <&intc>; >> + >> + pgc { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + pgc_pcie_phy: power-domain@3 { >> + #power-domain-cells = <0>; >> + >> + reg = <IMX7_POWER_DOMAIN_PCIE_PHY>; >> + power-supply = <®_1p0d>; >> + }; >> + }; >> + }; >> + >> + >> +Specifying power domain for IP modules >> +====================================== >> + >> +IP cores belonging to a power domain should contain a 'power-domains' >> +property that is a phandle for PGC node representing the domain. >> + >> +Example of a device that is part of the PCIE_PHY power domain: >> + >> + pcie: pcie@33800000 { >> + reg = <0x33800000 0x4000>, >> + <0x4ff00000 0x80000>; >> + /* ... */ >> + power-domains = <&pgc_pcie_phy>; >> + /* ... */ >> + }; >> diff --git a/include/dt-bindings/power/imx7-power.h b/include/dt-bindings/power/imx7-power.h >> new file mode 100644 >> index 0000000..eb70023 >> --- /dev/null >> +++ b/include/dt-bindings/power/imx7-power.h >> @@ -0,0 +1,18 @@ >> +/* >> + * Copyright (C) 2017 Impinj >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + */ >> + >> +#ifndef __DT_BINDINGS_IMX7_POWER_H__ >> +#define __DT_BINDINGS_IMX7_POWER_H__ >> + >> +#define IMX7_POWER_DOMAIN_USB_HSIC_PHY 0 >> +#define IMX7_POWER_DOMAIN_USB_OTG2_PHY 1 >> +#define IMX7_POWER_DOMAIN_USB_OTG1_PHY 2 >> +#define IMX7_POWER_DOMAIN_PCIE_PHY 3 >> +#define IMX7_POWER_DOMAIN_MIPI_PHY 4 > > Nitpick: Probably better to define according to the reference manual > defined order. > > 0x800 ~ 0x83F : PGC for A7 core0 > 0x840 ~ 0x87F: PGC for A7 core1 > 0x880 ~ 0x8BF: PGC for A7 SCU > 0xA00 ~ 0xA3F: PGC for fastmix/megamix > 0xC00 ~ 0xC3F: PGC for MIPI PHY > 0xC40 ~ 0xC7F: PGC for PCIE_PHY > 0xC80 ~ 0xCBF: Reserved > 0xCC0 ~ 0xCFF: Reserved > 0xD00 ~ 0xD3F: PGC for USB HSIC PHY > > You can drop A7 core/scu/fastmix/megamix as well. > Sure, will do. Thanks, Andrey Smirnov ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v7 1/2] dt-bindings: Add GPCv2 power gating driver 2017-03-27 18:42 ` Andrey Smirnov @ 2017-03-30 7:15 ` Dong Aisheng 0 siblings, 0 replies; 16+ messages in thread From: Dong Aisheng @ 2017-03-30 7:15 UTC (permalink / raw) To: Andrey Smirnov Cc: Shawn Guo, Andrey Yurovsky, Lucas Stach, Rob Herring, Mark Rutland, Fabio Estevam, devicetree, linux-arm-kernel, linux-kernel On Mon, Mar 27, 2017 at 11:42:51AM -0700, Andrey Smirnov wrote: > On Thu, Mar 23, 2017 at 11:32 PM, Dong Aisheng <dongas86@gmail.com> wrote: > > On Tue, Mar 21, 2017 at 07:50:03AM -0700, Andrey Smirnov wrote: > >> Add DT bindings for power domain driver for GPCv2 IP block found in > >> i.MX7 SoCs. > >> > >> Cc: yurovsky@gmail.com > >> Cc: Lucas Stach <l.stach@pengutronix.de> > >> Cc: Rob Herring <robh+dt@kernel.org> > >> Cc: Mark Rutland <mark.rutland@arm.com> > >> Cc: Fabio Estevam <fabio.estevam@nxp.com> > >> Cc: Dong Aisheng <dongas86@gmail.com> > >> Cc: devicetree@vger.kernel.org > >> Cc: linux-arm-kernel@lists.infradead.org > >> Cc: linux-kernel@vger.kernel.org > >> Acked-by: Rob Herring <robh@kernel.org> > >> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > >> --- > >> .../devicetree/bindings/power/fsl,imx-gpcv2.txt | 71 ++++++++++++++++++++++ > >> include/dt-bindings/power/imx7-power.h | 18 ++++++ > >> 2 files changed, 89 insertions(+) > >> create mode 100644 Documentation/devicetree/bindings/power/fsl,imx-gpcv2.txt > >> create mode 100644 include/dt-bindings/power/imx7-power.h > >> > >> diff --git a/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.txt b/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.txt > >> new file mode 100644 > >> index 0000000..02f45c6 > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.txt > >> @@ -0,0 +1,71 @@ > >> +Freescale i.MX General Power Controller v2 > >> +========================================== > >> + > >> +The i.MX7S/D General Power Control (GPC) block contains Power Gating > >> +Control (PGC) for various power domains. > >> + > >> +Required properties: > >> + > >> +- compatible: Should be "fsl,imx7d-gpc" > >> + > >> +- reg: should be register base and length as documented in the > >> + datasheet > >> + > >> +- interrupts: Should contain GPC interrupt request 1 > >> + > >> +Power domains contained within GPC node are generic power domain > >> +providers, documented in > >> +Documentation/devicetree/bindings/power/power_domain.txt, which are > >> +described as subnodes of the power gating controller 'pgc' node, > >> +which, in turn, is expected to contain the following: > >> + > >> +Required properties: > >> + > >> +- reg: Power domain index. Valid values are defined in > >> + include/dt-bindings/power/imx7-power.h > >> + > >> +- #power-domain-cells: Should be 0 > >> + > >> +Optional properties: > >> + > >> +- power-supply: Power supply used to power the domain > >> + > >> +Example: > >> + > >> + gpc: gpc@303a0000 { > >> + compatible = "fsl,imx7d-gpc"; > >> + reg = <0x303a0000 0x1000>; > >> + interrupt-controller; > >> + interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>; > >> + #interrupt-cells = <3>; > >> + interrupt-parent = <&intc>; > >> + > >> + pgc { > >> + #address-cells = <1>; > >> + #size-cells = <0>; > >> + > >> + pgc_pcie_phy: power-domain@3 { > >> + #power-domain-cells = <0>; > >> + > >> + reg = <IMX7_POWER_DOMAIN_PCIE_PHY>; > >> + power-supply = <®_1p0d>; > >> + }; > >> + }; > >> + }; > >> + > >> + > >> +Specifying power domain for IP modules > >> +====================================== > >> + > >> +IP cores belonging to a power domain should contain a 'power-domains' > >> +property that is a phandle for PGC node representing the domain. > >> + > >> +Example of a device that is part of the PCIE_PHY power domain: > >> + > >> + pcie: pcie@33800000 { > >> + reg = <0x33800000 0x4000>, > >> + <0x4ff00000 0x80000>; > >> + /* ... */ > >> + power-domains = <&pgc_pcie_phy>; > >> + /* ... */ > >> + }; > >> diff --git a/include/dt-bindings/power/imx7-power.h b/include/dt-bindings/power/imx7-power.h > >> new file mode 100644 > >> index 0000000..eb70023 > >> --- /dev/null > >> +++ b/include/dt-bindings/power/imx7-power.h > >> @@ -0,0 +1,18 @@ > >> +/* > >> + * Copyright (C) 2017 Impinj > >> + * > >> + * This program is free software; you can redistribute it and/or modify > >> + * it under the terms of the GNU General Public License version 2 as > >> + * published by the Free Software Foundation. > >> + */ > >> + > >> +#ifndef __DT_BINDINGS_IMX7_POWER_H__ > >> +#define __DT_BINDINGS_IMX7_POWER_H__ > >> + > >> +#define IMX7_POWER_DOMAIN_USB_HSIC_PHY 0 > >> +#define IMX7_POWER_DOMAIN_USB_OTG2_PHY 1 > >> +#define IMX7_POWER_DOMAIN_USB_OTG1_PHY 2 > >> +#define IMX7_POWER_DOMAIN_PCIE_PHY 3 > >> +#define IMX7_POWER_DOMAIN_MIPI_PHY 4 > > > > Nitpick: Probably better to define according to the reference manual > > defined order. > > > > 0x800 ~ 0x83F : PGC for A7 core0 > > 0x840 ~ 0x87F: PGC for A7 core1 > > 0x880 ~ 0x8BF: PGC for A7 SCU > > 0xA00 ~ 0xA3F: PGC for fastmix/megamix > > 0xC00 ~ 0xC3F: PGC for MIPI PHY > > 0xC40 ~ 0xC7F: PGC for PCIE_PHY > > 0xC80 ~ 0xCBF: Reserved > > 0xCC0 ~ 0xCFF: Reserved > > 0xD00 ~ 0xD3F: PGC for USB HSIC PHY > > > > You can drop A7 core/scu/fastmix/megamix as well. > > > > Sure, will do. > BTW, i read some other SoC power domain implementations, they keep the CPU domains although not used in kernel. e.g. include/dt-bindings/power/rk3288-power.h include/soc/tegra/pmc.h That probably is a good reference in case we may need them in the future. And device tree actually is describe HW. I wonder we may be better to keep them as well. Regards Dong Aisheng > Thanks, > Andrey Smirnov ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v7 2/2] soc/imx: Add GPCv2 power gating driver 2017-03-21 14:50 [PATCH v7 0/2] GPCv2 power gating driver Andrey Smirnov 2017-03-21 14:50 ` [PATCH v7 1/2] dt-bindings: Add " Andrey Smirnov @ 2017-03-21 14:50 ` Andrey Smirnov 2017-03-24 6:24 ` Dong Aisheng 1 sibling, 1 reply; 16+ messages in thread From: Andrey Smirnov @ 2017-03-21 14:50 UTC (permalink / raw) To: Shawn Guo Cc: Andrey Smirnov, yurovsky, Lucas Stach, Fabio Estevam, Dong Aisheng, linux-arm-kernel, linux-kernel Add code allowing for control of various power domains managed by GPCv2 IP block found in i.MX7 series of SoCs. Power domains covered by this patch are: - PCIE PHY - MIPI PHY - USB HSIC PHY - USB OTG1/2 PHY Support for any other power domain controlled by GPC is not present, and can be added at some later point. Testing of this code was done against a PCIe driver. Cc: yurovsky@gmail.com Cc: Lucas Stach <l.stach@pengutronix.de> Cc: Fabio Estevam <fabio.estevam@nxp.com> Cc: Dong Aisheng <dongas86@gmail.com> Cc: linux-arm-kernel@lists.infradead.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> --- drivers/soc/Kconfig | 1 + drivers/soc/imx/Kconfig | 10 ++ drivers/soc/imx/Makefile | 1 + drivers/soc/imx/gpcv2.c | 365 +++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 377 insertions(+) create mode 100644 drivers/soc/imx/Kconfig create mode 100644 drivers/soc/imx/gpcv2.c diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig index f09023f..8943543 100644 --- a/drivers/soc/Kconfig +++ b/drivers/soc/Kconfig @@ -2,6 +2,7 @@ menu "SOC (System On Chip) specific Drivers" source "drivers/soc/bcm/Kconfig" source "drivers/soc/fsl/Kconfig" +source "drivers/soc/imx/Kconfig" source "drivers/soc/mediatek/Kconfig" source "drivers/soc/qcom/Kconfig" source "drivers/soc/rockchip/Kconfig" diff --git a/drivers/soc/imx/Kconfig b/drivers/soc/imx/Kconfig new file mode 100644 index 0000000..bc7f0ee0 --- /dev/null +++ b/drivers/soc/imx/Kconfig @@ -0,0 +1,10 @@ +menu "i.MX SoC drivers" + +config IMX7_PM_DOMAINS + bool "i.MX7 PM domains" + select PM_GENERIC_DOMAINS + depends on SOC_IMX7D || (COMPILE_TEST && OF) + default y if SOC_IMX7D + +endmenu + diff --git a/drivers/soc/imx/Makefile b/drivers/soc/imx/Makefile index 35861f5..5b6e396 100644 --- a/drivers/soc/imx/Makefile +++ b/drivers/soc/imx/Makefile @@ -1 +1,2 @@ obj-y += gpc.o +obj-$(CONFIG_IMX7_PM_DOMAINS) += gpcv2.o diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c new file mode 100644 index 0000000..a27c5f8 --- /dev/null +++ b/drivers/soc/imx/gpcv2.c @@ -0,0 +1,365 @@ +/* + * Copyright 2017 Impinj, Inc + * Author: Andrey Smirnov <andrew.smirnov@gmail.com> + * + * Based on the code of analogus driver: + * + * Copyright 2015-2017 Pengutronix, Lucas Stach <kernel@pengutronix.de> + * + * The code contained herein is licensed under the GNU General Public + * License. You may obtain a copy of the GNU General Public License + * Version 2 or later at the following locations: + * + * http://www.opensource.org/licenses/gpl-license.html + * http://www.gnu.org/copyleft/gpl.html + */ + +#include <linux/platform_device.h> +#include <linux/pm_domain.h> +#include <linux/regmap.h> +#include <linux/regulator/consumer.h> +#include <dt-bindings/power/imx7-power.h> + +#define GPC_PGC_CPU_MAPPING 0xec +#define USB_HSIC_PHY_A7_DOMAIN BIT(6) +#define USB_OTG2_PHY_A7_DOMAIN BIT(5) +#define USB_OTG1_PHY_A7_DOMAIN BIT(4) +#define PCIE_PHY_A7_DOMAIN BIT(3) +#define MIPI_PHY_A7_DOMAIN BIT(2) + +#define GPC_PU_PGC_SW_PUP_REQ 0xf8 +#define GPC_PU_PGC_SW_PDN_REQ 0x104 +#define USB_HSIC_PHY_SW_Pxx_REQ BIT(4) +#define USB_OTG2_PHY_SW_Pxx_REQ BIT(3) +#define USB_OTG1_PHY_SW_Pxx_REQ BIT(2) +#define PCIE_PHY_SW_Pxx_REQ BIT(1) +#define MIPI_PHY_SW_Pxx_REQ BIT(0) + +#define GPC_MAX_REGISTER 0x1000 + +#define GPC_PGC_nCTRL_PCR BIT(0) + +struct imx7_pgc_domain { + struct generic_pm_domain genpd; + struct regmap *regmap; + struct regulator *regulator; + + unsigned int pgc_nctrl; + + const struct { + u32 pxx; + u32 map; + } bits; + + const int voltage; + struct device *dev; +}; + +static int imx7_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain *genpd, + bool on) +{ + struct imx7_pgc_domain *domain = container_of(genpd, + struct imx7_pgc_domain, + genpd); + unsigned int offset = on ? + GPC_PU_PGC_SW_PUP_REQ : GPC_PU_PGC_SW_PDN_REQ; + const bool enable_power_control = domain->pgc_nctrl && !on; + const bool has_regulator = !IS_ERR(domain->regulator); + unsigned long deadline; + int ret = 0; + + regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING, + domain->bits.map, domain->bits.map); + + if (has_regulator && on) { + ret = regulator_enable(domain->regulator); + if (ret) { + dev_err(domain->dev, "failed to enable regulator\n"); + goto unmap; + } + } + + if (enable_power_control) + regmap_update_bits(domain->regmap, domain->pgc_nctrl, + GPC_PGC_nCTRL_PCR, GPC_PGC_nCTRL_PCR); + + regmap_update_bits(domain->regmap, offset, + domain->bits.pxx, domain->bits.pxx); + + /* + * As per "5.5.9.4 Example Code 4" in IMX7DRM.pdf wait + * for PUP_REQ/PDN_REQ bit to be cleared + */ + deadline = jiffies + msecs_to_jiffies(1); + while (true) { + u32 pxx_req; + + regmap_read(domain->regmap, offset, &pxx_req); + + if (!(pxx_req & domain->bits.pxx)) + break; + + if (time_after(jiffies, deadline)) { + dev_err(domain->dev, "falied to command PGC\n"); + ret = -ETIMEDOUT; + /* + * If we were in a process of enabling a + * domain and failed we might as well disable + * the regulator we just enabled. And if it + * was the opposite situation and we failed to + * power down -- keep the regulator on + */ + on = !on; + break; + } + + cpu_relax(); + } + + if (enable_power_control) + regmap_update_bits(domain->regmap, domain->pgc_nctrl, + GPC_PGC_nCTRL_PCR, 0); + + if (has_regulator && !on) { + int err; + + err = regulator_disable(domain->regulator); + if (err) + dev_err(domain->dev, + "failed to disable regulator: %d\n", ret); + /* Preserve earlier error code */ + ret = ret ?: err; + } +unmap: + regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING, + domain->bits.map, 0); + return ret; +} + +static int imx7_gpc_pu_pgc_sw_pup_req(struct generic_pm_domain *genpd) +{ + return imx7_gpc_pu_pgc_sw_pxx_req(genpd, true); +} + +static int imx7_gpc_pu_pgc_sw_pdn_req(struct generic_pm_domain *genpd) +{ + return imx7_gpc_pu_pgc_sw_pxx_req(genpd, false); +} + +static struct imx7_pgc_domain imx7_pgc_domains[] = { + [IMX7_POWER_DOMAIN_USB_HSIC_PHY] = { + .genpd = { + .name = "usb-hsic-phy", + }, + .bits = { + .pxx = USB_HSIC_PHY_SW_Pxx_REQ, + .map = USB_HSIC_PHY_A7_DOMAIN, + }, + .voltage = 1200000, + .pgc_nctrl = 0x0d00, + }, + + [IMX7_POWER_DOMAIN_USB_OTG2_PHY] = { + .genpd = { + .name = "usb-otg2-phy", + }, + .bits = { + .pxx = USB_OTG2_PHY_SW_Pxx_REQ, + .map = USB_OTG2_PHY_A7_DOMAIN, + }, + }, + + [IMX7_POWER_DOMAIN_USB_OTG1_PHY] = { + .genpd = { + .name = "usb-otg1-phy", + }, + .bits = { + .pxx = USB_OTG1_PHY_SW_Pxx_REQ, + .map = USB_OTG1_PHY_A7_DOMAIN, + }, + }, + + [IMX7_POWER_DOMAIN_PCIE_PHY] = { + .genpd = { + .name = "pcie-phy", + }, + .bits = { + .pxx = PCIE_PHY_SW_Pxx_REQ, + .map = PCIE_PHY_A7_DOMAIN, + }, + .voltage = 1000000, + .pgc_nctrl = 0x0c40, + }, + + [IMX7_POWER_DOMAIN_MIPI_PHY] = { + .genpd = { + .name = "mipi-phy", + }, + .bits = { + .pxx = MIPI_PHY_SW_Pxx_REQ, + .map = MIPI_PHY_A7_DOMAIN, + }, + .voltage = 1000000, + .pgc_nctrl = 0x0c00, + }, +}; + +static int imx7_pgc_domain_probe(struct platform_device *pdev) +{ + struct imx7_pgc_domain *domain = pdev->dev.platform_data; + int ret; + + domain->dev = &pdev->dev; + + ret = pm_genpd_init(&domain->genpd, NULL, true); + if (ret) { + dev_err(domain->dev, "Failed to init power domain\n"); + return ret; + } + + domain->regulator = devm_regulator_get_optional(domain->dev, "power"); + if (IS_ERR(domain->regulator) && + PTR_ERR(domain->regulator) != -ENODEV) { + dev_err(domain->dev, "Failed to get domain's regulator\n"); + return PTR_ERR(domain->regulator); + } + + if (!IS_ERR(domain->regulator)) { + if (!domain->voltage) { + WARN(1, "No voltage configured for domain's regulator"); + return -EINVAL; + } + + regulator_set_voltage(domain->regulator, + domain->voltage, domain->voltage); + } + + ret = of_genpd_add_provider_simple(domain->dev->of_node, + &domain->genpd); + if (ret) { + dev_err(domain->dev, "Failed to add genpd provider\n"); + pm_genpd_remove(&domain->genpd); + } + + return ret; +} + +static int imx7_pgc_domain_remove(struct platform_device *pdev) +{ + struct imx7_pgc_domain *domain = pdev->dev.platform_data; + + of_genpd_del_provider(domain->dev->of_node); + pm_genpd_remove(&domain->genpd); + + return 0; +} + +static const struct platform_device_id imx7_pgc_domain_id[] = { + { "imx7-pgc-domain", }, + { }, +}; + +static struct platform_driver imx7_pgc_domain_driver = { + .driver = { + .name = "imx7-pgc", + }, + .probe = imx7_pgc_domain_probe, + .remove = imx7_pgc_domain_remove, + .id_table = imx7_pgc_domain_id, +}; +builtin_platform_driver(imx7_pgc_domain_driver) + +static int imx_gpcv2_probe(struct platform_device *pdev) +{ + static const struct regmap_config regmap_config = { + .cache_type = REGCACHE_NONE, + .reg_bits = 32, + .val_bits = 32, + .reg_stride = 4, + .max_register = GPC_MAX_REGISTER, + }; + struct device *dev = &pdev->dev; + struct device_node *pgc_np, *np; + struct regmap *regmap; + struct resource *res; + void __iomem *base; + int ret; + + pgc_np = of_get_child_by_name(dev->of_node, "pgc"); + if (!pgc_np) { + dev_err(dev, "No power domains specified in DT\n"); + return -EINVAL; + } + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + base = devm_ioremap_resource(dev, res); + if (IS_ERR(base)) + return PTR_ERR(base); + + regmap = devm_regmap_init_mmio(dev, base, ®map_config); + if (IS_ERR(regmap)) { + ret = PTR_ERR(regmap); + dev_err(dev, "failed to init regmap (%d)\n", ret); + return ret; + } + + for_each_child_of_node(pgc_np, np) { + struct platform_device *pd_pdev; + struct imx7_pgc_domain *domain; + u32 domain_index; + + ret = of_property_read_u32(np, "reg", &domain_index); + if (ret) { + dev_err(dev, "Failed to read 'reg' property\n"); + of_node_put(np); + return ret; + } + + if (domain_index >= ARRAY_SIZE(imx7_pgc_domains)) { + dev_warn(dev, + "Domain index %d is out of bounds\n", + domain_index); + continue; + } + + domain = &imx7_pgc_domains[domain_index]; + domain->regmap = regmap; + domain->genpd.power_on = imx7_gpc_pu_pgc_sw_pup_req; + domain->genpd.power_off = imx7_gpc_pu_pgc_sw_pdn_req; + + pd_pdev = platform_device_alloc("imx7-pgc-domain", + domain_index); + if (!pd_pdev) { + dev_err(dev, "Failed to allocate platform device\n"); + of_node_put(np); + return -ENOMEM; + } + + pd_pdev->dev.platform_data = domain; + pd_pdev->dev.parent = dev; + pd_pdev->dev.of_node = np; + + ret = platform_device_add(pd_pdev); + if (ret) { + platform_device_put(pd_pdev); + of_node_put(np); + return ret; + } + } + + return 0; +} + +static const struct of_device_id imx_gpcv2_dt_ids[] = { + { .compatible = "fsl,imx7d-gpc" }, + { } +}; + +static struct platform_driver imx_gpc_driver = { + .driver = { + .name = "imx-gpcv2", + .of_match_table = imx_gpcv2_dt_ids, + }, + .probe = imx_gpcv2_probe, +}; +builtin_platform_driver(imx_gpc_driver) -- 2.9.3 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v7 2/2] soc/imx: Add GPCv2 power gating driver 2017-03-21 14:50 ` [PATCH v7 2/2] soc/imx: " Andrey Smirnov @ 2017-03-24 6:24 ` Dong Aisheng 2017-03-23 14:35 ` Lucas Stach 2017-03-27 18:42 ` Andrey Smirnov 0 siblings, 2 replies; 16+ messages in thread From: Dong Aisheng @ 2017-03-24 6:24 UTC (permalink / raw) To: Andrey Smirnov Cc: Shawn Guo, yurovsky, Lucas Stach, Fabio Estevam, linux-arm-kernel, linux-kernel On Tue, Mar 21, 2017 at 07:50:04AM -0700, Andrey Smirnov wrote: > Add code allowing for control of various power domains managed by GPCv2 > IP block found in i.MX7 series of SoCs. Power domains covered by this > patch are: > > - PCIE PHY > - MIPI PHY > - USB HSIC PHY > - USB OTG1/2 PHY > You probably may need drop USB OTG which is not claimed in current RM. See the PGC definition in 5.5.10 GPC Memory Map section. Each PGC (CPU type, MIX type, PU type) will occupy 64 Bytes address space, the specific base address of each PGC are listed as below. • 0x800 ~ 0x83F : PGC for A7 core0 • 0x840 ~ 0x87F: PGC for A7 core1 • 0x880 ~ 0x8BF: PGC for A7 SCU • 0xA00 ~ 0xA3F: PGC for fastmix/megamix • 0xC00 ~ 0xC3F: PGC for MIPI PHY • 0xC40 ~ 0xC7F: PGC for PCIE_PHY • 0xC80 ~ 0xCBF: Reserved • 0xCC0 ~ 0xCFF: Reserved • 0xD00 ~ 0xD3F: PGC for USB HSIC PHY And in 5.4 Power Management Unit (PMU) chapter, you will find the USB OTG phy power is directly supplied by VDD_USB_OTG1_3P3_IN/VDD_USB_OTG2_3P3_IN. http://www.nxp.com/assets/documents/data/en/reference-manuals/IMX7DRM.pdf I understand that there's also some USB OTG code exist in NXP internal tree, but that's legacy for early doc implementation and may be deprecated. so i assume it should be gone. Hopefully i will double confirm with our IC designer tomorrow. > Support for any other power domain controlled by GPC is not present, and > can be added at some later point. > > Testing of this code was done against a PCIe driver. > > Cc: yurovsky@gmail.com > Cc: Lucas Stach <l.stach@pengutronix.de> > Cc: Fabio Estevam <fabio.estevam@nxp.com> > Cc: Dong Aisheng <dongas86@gmail.com> > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > --- > drivers/soc/Kconfig | 1 + > drivers/soc/imx/Kconfig | 10 ++ > drivers/soc/imx/Makefile | 1 + > drivers/soc/imx/gpcv2.c | 365 +++++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 377 insertions(+) > create mode 100644 drivers/soc/imx/Kconfig > create mode 100644 drivers/soc/imx/gpcv2.c > > diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig > index f09023f..8943543 100644 > --- a/drivers/soc/Kconfig > +++ b/drivers/soc/Kconfig > @@ -2,6 +2,7 @@ menu "SOC (System On Chip) specific Drivers" > > source "drivers/soc/bcm/Kconfig" > source "drivers/soc/fsl/Kconfig" > +source "drivers/soc/imx/Kconfig" > source "drivers/soc/mediatek/Kconfig" > source "drivers/soc/qcom/Kconfig" > source "drivers/soc/rockchip/Kconfig" > diff --git a/drivers/soc/imx/Kconfig b/drivers/soc/imx/Kconfig > new file mode 100644 > index 0000000..bc7f0ee0 > --- /dev/null > +++ b/drivers/soc/imx/Kconfig > @@ -0,0 +1,10 @@ > +menu "i.MX SoC drivers" > + > +config IMX7_PM_DOMAINS > + bool "i.MX7 PM domains" > + select PM_GENERIC_DOMAINS > + depends on SOC_IMX7D || (COMPILE_TEST && OF) > + default y if SOC_IMX7D > + > +endmenu > + > diff --git a/drivers/soc/imx/Makefile b/drivers/soc/imx/Makefile > index 35861f5..5b6e396 100644 > --- a/drivers/soc/imx/Makefile > +++ b/drivers/soc/imx/Makefile > @@ -1 +1,2 @@ > obj-y += gpc.o > +obj-$(CONFIG_IMX7_PM_DOMAINS) += gpcv2.o > diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c > new file mode 100644 > index 0000000..a27c5f8 > --- /dev/null > +++ b/drivers/soc/imx/gpcv2.c > @@ -0,0 +1,365 @@ > +/* > + * Copyright 2017 Impinj, Inc > + * Author: Andrey Smirnov <andrew.smirnov@gmail.com> > + * > + * Based on the code of analogus driver: > + * > + * Copyright 2015-2017 Pengutronix, Lucas Stach <kernel@pengutronix.de> > + * > + * The code contained herein is licensed under the GNU General Public > + * License. You may obtain a copy of the GNU General Public License > + * Version 2 or later at the following locations: > + * > + * http://www.opensource.org/licenses/gpl-license.html > + * http://www.gnu.org/copyleft/gpl.html > + */ > + > +#include <linux/platform_device.h> > +#include <linux/pm_domain.h> > +#include <linux/regmap.h> > +#include <linux/regulator/consumer.h> > +#include <dt-bindings/power/imx7-power.h> > + > +#define GPC_PGC_CPU_MAPPING 0xec > +#define USB_HSIC_PHY_A7_DOMAIN BIT(6) > +#define USB_OTG2_PHY_A7_DOMAIN BIT(5) > +#define USB_OTG1_PHY_A7_DOMAIN BIT(4) > +#define PCIE_PHY_A7_DOMAIN BIT(3) > +#define MIPI_PHY_A7_DOMAIN BIT(2) > + > +#define GPC_PU_PGC_SW_PUP_REQ 0xf8 > +#define GPC_PU_PGC_SW_PDN_REQ 0x104 > +#define USB_HSIC_PHY_SW_Pxx_REQ BIT(4) > +#define USB_OTG2_PHY_SW_Pxx_REQ BIT(3) > +#define USB_OTG1_PHY_SW_Pxx_REQ BIT(2) > +#define PCIE_PHY_SW_Pxx_REQ BIT(1) > +#define MIPI_PHY_SW_Pxx_REQ BIT(0) > + After apply, what i see is: #define GPC_PU_PGC_SW_PUP_REQ 0xf8 #define GPC_PU_PGC_SW_PDN_REQ 0x104 #define USB_HSIC_PHY_SW_Pxx_REQ BIT(4) #define USB_OTG2_PHY_SW_Pxx_REQ BIT(3) #define USB_OTG1_PHY_SW_Pxx_REQ BIT(2) #define PCIE_PHY_SW_Pxx_REQ BIT(1) #define MIPI_PHY_SW_Pxx_REQ BIT(0) Looks quite tight, Can we have one more TAB for the definition? Like: #define GPC_PU_PGC_SW_PUP_REQ 0xf8 > +#define GPC_MAX_REGISTER 0x1000 > + > +#define GPC_PGC_nCTRL_PCR BIT(0) > + > +struct imx7_pgc_domain { > + struct generic_pm_domain genpd; > + struct regmap *regmap; > + struct regulator *regulator; > + > + unsigned int pgc_nctrl; > + > + const struct { > + u32 pxx; > + u32 map; > + } bits; > + > + const int voltage; > + struct device *dev; > +}; > + > +static int imx7_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain *genpd, > + bool on) > +{ > + struct imx7_pgc_domain *domain = container_of(genpd, > + struct imx7_pgc_domain, > + genpd); > + unsigned int offset = on ? > + GPC_PU_PGC_SW_PUP_REQ : GPC_PU_PGC_SW_PDN_REQ; > + const bool enable_power_control = domain->pgc_nctrl && !on; > + const bool has_regulator = !IS_ERR(domain->regulator); > + unsigned long deadline; > + int ret = 0; > + > + regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING, > + domain->bits.map, domain->bits.map); > + > + if (has_regulator && on) { > + ret = regulator_enable(domain->regulator); > + if (ret) { > + dev_err(domain->dev, "failed to enable regulator\n"); > + goto unmap; > + } > + } > + > + if (enable_power_control) > + regmap_update_bits(domain->regmap, domain->pgc_nctrl, > + GPC_PGC_nCTRL_PCR, GPC_PGC_nCTRL_PCR); > + > + regmap_update_bits(domain->regmap, offset, > + domain->bits.pxx, domain->bits.pxx); > + > + /* > + * As per "5.5.9.4 Example Code 4" in IMX7DRM.pdf wait > + * for PUP_REQ/PDN_REQ bit to be cleared > + */ > + deadline = jiffies + msecs_to_jiffies(1); > + while (true) { > + u32 pxx_req; > + > + regmap_read(domain->regmap, offset, &pxx_req); > + > + if (!(pxx_req & domain->bits.pxx)) > + break; > + > + if (time_after(jiffies, deadline)) { > + dev_err(domain->dev, "falied to command PGC\n"); > + ret = -ETIMEDOUT; > + /* > + * If we were in a process of enabling a > + * domain and failed we might as well disable > + * the regulator we just enabled. And if it > + * was the opposite situation and we failed to > + * power down -- keep the regulator on > + */ > + on = !on; > + break; > + } > + > + cpu_relax(); > + } > + > + if (enable_power_control) > + regmap_update_bits(domain->regmap, domain->pgc_nctrl, > + GPC_PGC_nCTRL_PCR, 0); > + > + if (has_regulator && !on) { > + int err; > + > + err = regulator_disable(domain->regulator); > + if (err) > + dev_err(domain->dev, > + "failed to disable regulator: %d\n", ret); > + /* Preserve earlier error code */ > + ret = ret ?: err; > + } > +unmap: > + regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING, > + domain->bits.map, 0); > + return ret; > +} > + > +static int imx7_gpc_pu_pgc_sw_pup_req(struct generic_pm_domain *genpd) > +{ > + return imx7_gpc_pu_pgc_sw_pxx_req(genpd, true); > +} > + > +static int imx7_gpc_pu_pgc_sw_pdn_req(struct generic_pm_domain *genpd) > +{ > + return imx7_gpc_pu_pgc_sw_pxx_req(genpd, false); > +} > + > +static struct imx7_pgc_domain imx7_pgc_domains[] = { > + [IMX7_POWER_DOMAIN_USB_HSIC_PHY] = { > + .genpd = { > + .name = "usb-hsic-phy", > + }, > + .bits = { > + .pxx = USB_HSIC_PHY_SW_Pxx_REQ, > + .map = USB_HSIC_PHY_A7_DOMAIN, > + }, > + .voltage = 1200000, > + .pgc_nctrl = 0x0d00, > + }, > + > + [IMX7_POWER_DOMAIN_USB_OTG2_PHY] = { > + .genpd = { > + .name = "usb-otg2-phy", > + }, > + .bits = { > + .pxx = USB_OTG2_PHY_SW_Pxx_REQ, > + .map = USB_OTG2_PHY_A7_DOMAIN, > + }, > + }, > + > + [IMX7_POWER_DOMAIN_USB_OTG1_PHY] = { > + .genpd = { > + .name = "usb-otg1-phy", > + }, > + .bits = { > + .pxx = USB_OTG1_PHY_SW_Pxx_REQ, > + .map = USB_OTG1_PHY_A7_DOMAIN, > + }, > + }, > + > + [IMX7_POWER_DOMAIN_PCIE_PHY] = { > + .genpd = { > + .name = "pcie-phy", > + }, > + .bits = { > + .pxx = PCIE_PHY_SW_Pxx_REQ, > + .map = PCIE_PHY_A7_DOMAIN, > + }, > + .voltage = 1000000, > + .pgc_nctrl = 0x0c40, > + }, > + > + [IMX7_POWER_DOMAIN_MIPI_PHY] = { > + .genpd = { > + .name = "mipi-phy", > + }, > + .bits = { > + .pxx = MIPI_PHY_SW_Pxx_REQ, > + .map = MIPI_PHY_A7_DOMAIN, > + }, > + .voltage = 1000000, > + .pgc_nctrl = 0x0c00, > + }, > +}; > + > +static int imx7_pgc_domain_probe(struct platform_device *pdev) > +{ > + struct imx7_pgc_domain *domain = pdev->dev.platform_data; > + int ret; > + > + domain->dev = &pdev->dev; > + > + ret = pm_genpd_init(&domain->genpd, NULL, true); > + if (ret) { > + dev_err(domain->dev, "Failed to init power domain\n"); > + return ret; > + } > + > + domain->regulator = devm_regulator_get_optional(domain->dev, "power"); > + if (IS_ERR(domain->regulator) && > + PTR_ERR(domain->regulator) != -ENODEV) { > + dev_err(domain->dev, "Failed to get domain's regulator\n"); > + return PTR_ERR(domain->regulator); > + } > + > + if (!IS_ERR(domain->regulator)) { > + if (!domain->voltage) { > + WARN(1, "No voltage configured for domain's regulator"); > + return -EINVAL; > + } > + > + regulator_set_voltage(domain->regulator, > + domain->voltage, domain->voltage); > + } > + > + ret = of_genpd_add_provider_simple(domain->dev->of_node, > + &domain->genpd); > + if (ret) { > + dev_err(domain->dev, "Failed to add genpd provider\n"); > + pm_genpd_remove(&domain->genpd); > + } > + > + return ret; > +} > + > +static int imx7_pgc_domain_remove(struct platform_device *pdev) > +{ > + struct imx7_pgc_domain *domain = pdev->dev.platform_data; > + > + of_genpd_del_provider(domain->dev->of_node); > + pm_genpd_remove(&domain->genpd); > + > + return 0; > +} > + > +static const struct platform_device_id imx7_pgc_domain_id[] = { > + { "imx7-pgc-domain", }, > + { }, > +}; > + > +static struct platform_driver imx7_pgc_domain_driver = { > + .driver = { > + .name = "imx7-pgc", > + }, > + .probe = imx7_pgc_domain_probe, > + .remove = imx7_pgc_domain_remove, > + .id_table = imx7_pgc_domain_id, > +}; > +builtin_platform_driver(imx7_pgc_domain_driver) Again, i have a fundamental question about this patch implementation that why we choose above way to register the power domain? I'm sorry that i did not know too much history. Would you guys please help share some information? Because AFAIK this way will register each domain as a power domain provider which is a bit violate the real HW and current power domain framework design. And it is a bit more complicated to use than before. IMHO i would rather prefer the old traditional and simpler way that one provider (GPC) supplies multiple domains (PCIE/MIPI/HSIC PHY domain) than this patch does. However, i might be wrong. Please help to clear. > + > +static int imx_gpcv2_probe(struct platform_device *pdev) > +{ > + static const struct regmap_config regmap_config = { > + .cache_type = REGCACHE_NONE, You could drop it as GPC does. > + .reg_bits = 32, > + .val_bits = 32, > + .reg_stride = 4, > + .max_register = GPC_MAX_REGISTER, Do you need add some readable/writeable reg? There seems to be some reserved regs in your range. I can easily got the below crash: root@imx6ul7d:/sys/kernel/debug/regmap/303a0000.gpc# cat registers [ 54.985395] Unhandled fault: external abort on non-linefetch (0x1008) at 0xf08511c0 [ 54.993166] pgd = ed02c000 [ 54.995894] [f08511c0] *pgd=af00c811, *pte=303a0653, *ppte=303a0453 [ 55.002215] Internal error: : 1008 [#1] SMP ARM [ 55.006756] Modules linked in: [ 55.009830] CPU: 0 PID: 750 Comm: cat Not tainted 4.11.0-rc1-00057-g2d60158-dirty #1169 [ 55.017842] Hardware name: Freescale i.MX7 Dual (Device Tree) [ 55.023598] task: ee20ee40 task.stack: ed0f8000 [ 55.028146] PC is at regmap_mmio_read32le+0x14/0x24 [ 55.033035] LR is at regmap_mmio_read+0x40/0x60 [ 55.037577] pc : [<c054f160>] lr : [<c054f348>] psr: 20070093 [ 55.037577] sp : ed0f9dc8 ip : ed0f9dd8 fp : ed0f9dd4 [ 55.049064] r10: ed160000 r9 : ef341000 r8 : ed0f9f80 [ 55.054299] r7 : ed0f9e5c r6 : ed0f9e5c r5 : 000001c0 r4 : ef3bb900 [ 55.060836] r3 : f0851000 r2 : ed0f9e5c r1 : f08511c0 r0 : ef3bb900 [ 55.067374] Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment none [ 55.074606] Control: 10c5387d Table: ad02c06a DAC: 00000051 [ 55.080361] Process cat (pid: 750, stack limit = 0xed0f8210) .... > + }; > + struct device *dev = &pdev->dev; > + struct device_node *pgc_np, *np; > + struct regmap *regmap; > + struct resource *res; > + void __iomem *base; > + int ret; > + > + pgc_np = of_get_child_by_name(dev->of_node, "pgc"); > + if (!pgc_np) { > + dev_err(dev, "No power domains specified in DT\n"); > + return -EINVAL; > + } > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + base = devm_ioremap_resource(dev, res); > + if (IS_ERR(base)) > + return PTR_ERR(base); > + > + regmap = devm_regmap_init_mmio(dev, base, ®map_config); > + if (IS_ERR(regmap)) { > + ret = PTR_ERR(regmap); > + dev_err(dev, "failed to init regmap (%d)\n", ret); > + return ret; > + } > + > + for_each_child_of_node(pgc_np, np) { > + struct platform_device *pd_pdev; > + struct imx7_pgc_domain *domain; > + u32 domain_index; > + > + ret = of_property_read_u32(np, "reg", &domain_index); > + if (ret) { > + dev_err(dev, "Failed to read 'reg' property\n"); > + of_node_put(np); > + return ret; > + } > + > + if (domain_index >= ARRAY_SIZE(imx7_pgc_domains)) { > + dev_warn(dev, > + "Domain index %d is out of bounds\n", > + domain_index); > + continue; > + } > + > + domain = &imx7_pgc_domains[domain_index]; > + domain->regmap = regmap; > + domain->genpd.power_on = imx7_gpc_pu_pgc_sw_pup_req; > + domain->genpd.power_off = imx7_gpc_pu_pgc_sw_pdn_req; > + > + pd_pdev = platform_device_alloc("imx7-pgc-domain", > + domain_index); > + if (!pd_pdev) { > + dev_err(dev, "Failed to allocate platform device\n"); > + of_node_put(np); > + return -ENOMEM; > + } > + > + pd_pdev->dev.platform_data = domain; > + pd_pdev->dev.parent = dev; > + pd_pdev->dev.of_node = np; > + > + ret = platform_device_add(pd_pdev); > + if (ret) { > + platform_device_put(pd_pdev); > + of_node_put(np); > + return ret; > + } > + } > + > + return 0; > +} > + > +static const struct of_device_id imx_gpcv2_dt_ids[] = { > + { .compatible = "fsl,imx7d-gpc" }, > + { } > +}; > + > +static struct platform_driver imx_gpc_driver = { > + .driver = { > + .name = "imx-gpcv2", > + .of_match_table = imx_gpcv2_dt_ids, > + }, > + .probe = imx_gpcv2_probe, > +}; > +builtin_platform_driver(imx_gpc_driver) > -- > 2.9.3 > Regards Dong Aisheng ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v7 2/2] soc/imx: Add GPCv2 power gating driver 2017-03-24 6:24 ` Dong Aisheng @ 2017-03-23 14:35 ` Lucas Stach 2017-03-30 7:51 ` Dong Aisheng 2017-03-27 18:42 ` Andrey Smirnov 1 sibling, 1 reply; 16+ messages in thread From: Lucas Stach @ 2017-03-23 14:35 UTC (permalink / raw) To: Dong Aisheng Cc: Andrey Smirnov, Shawn Guo, yurovsky, Fabio Estevam, linux-arm-kernel, linux-kernel Hi Dong, Am Freitag, den 24.03.2017, 14:24 +0800 schrieb Dong Aisheng: [...] > > +static struct platform_driver imx7_pgc_domain_driver = { > > + .driver = { > > + .name = "imx7-pgc", > > + }, > > + .probe = imx7_pgc_domain_probe, > > + .remove = imx7_pgc_domain_remove, > > + .id_table = imx7_pgc_domain_id, > > +}; > > +builtin_platform_driver(imx7_pgc_domain_driver) > > Again, i have a fundamental question about this patch implementation > that why we choose above way to register the power domain? > > I'm sorry that i did not know too much history. > Would you guys please help share some information? > > Because AFAIK this way will register each domain as a power domain > provider which is a bit violate the real HW and current power domain > framework design. And it is a bit more complicated to use than before. > > IMHO i would rather prefer the old traditional and simpler way that one > provider (GPC) supplies multiple domains (PCIE/MIPI/HSIC PHY domain) > than this patch does. > > However, i might be wrong. Please help to clear. This way we can properly describe each power domain with the regulator supplying the domain and the clocks of the devices inside the domain in the device tree. This is needed as for the upstream version we are controlling the regulator from the GPC driver, as opposed to the downstream version, where each device has to implement the regulator handling and power up/down sequencing. See the rationale in the commits adding the multidomain support to the i.MX6 GPC. Regards, Lucas ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v7 2/2] soc/imx: Add GPCv2 power gating driver 2017-03-23 14:35 ` Lucas Stach @ 2017-03-30 7:51 ` Dong Aisheng 2017-03-29 16:08 ` Lucas Stach 0 siblings, 1 reply; 16+ messages in thread From: Dong Aisheng @ 2017-03-30 7:51 UTC (permalink / raw) To: Lucas Stach Cc: Andrey Smirnov, Shawn Guo, yurovsky, Fabio Estevam, linux-arm-kernel, linux-kernel Hi Lucas, On Thu, Mar 23, 2017 at 03:35:49PM +0100, Lucas Stach wrote: > Hi Dong, > > Am Freitag, den 24.03.2017, 14:24 +0800 schrieb Dong Aisheng: > [...] > > > +static struct platform_driver imx7_pgc_domain_driver = { > > > + .driver = { > > > + .name = "imx7-pgc", > > > + }, > > > + .probe = imx7_pgc_domain_probe, > > > + .remove = imx7_pgc_domain_remove, > > > + .id_table = imx7_pgc_domain_id, > > > +}; > > > +builtin_platform_driver(imx7_pgc_domain_driver) > > > > Again, i have a fundamental question about this patch implementation > > that why we choose above way to register the power domain? > > > > I'm sorry that i did not know too much history. > > Would you guys please help share some information? > > > > Because AFAIK this way will register each domain as a power domain > > provider which is a bit violate the real HW and current power domain > > framework design. And it is a bit more complicated to use than before. > > > > IMHO i would rather prefer the old traditional and simpler way that one > > provider (GPC) supplies multiple domains (PCIE/MIPI/HSIC PHY domain) > > than this patch does. > > > > However, i might be wrong. Please help to clear. > > This way we can properly describe each power domain with the regulator > supplying the domain and the clocks of the devices inside the domain in > the device tree. > Thanks for the explaination. I understand that purpose. Now my concern is why we doing things like this: Builtin two platforms driver and use one to dynamically create device to trigger another driver bind to register the domain. static int imx7_pgc_domain_probe(struct platform_device *pdev) { of_genpd_add_provider_simple(domain->dev->of_node, &domain->genpd); } static struct platform_driver imx7_pgc_domain_driver = { .driver = { .name = "imx7-pgc", }, .probe = imx7_pgc_domain_probe, }; builtin_platform_driver(imx7_pgc_domain_driver) static int imx_gpcv2_probe(struct platform_device *pdev) { for_each_child_of_node(pgc_np, np) { pd_pdev = platform_device_alloc("imx7-pgc-domain", domain_index); ret = platform_device_add(pd_pdev); } } static struct platform_driver imx_gpc_driver = { .driver = { .name = "imx-gpcv2", .of_match_table = imx_gpcv2_dt_ids, }, .probe = imx_gpcv2_probe, }; builtin_platform_driver(imx_gpc_driver) Is there any special purpose or i missed something? Can we just use one or a simple core_initcall(imx_gpcv2_probe) cause this probably should be registered early for other consumers? Personally i'd be more like Rockchip's power domain implementation. See: arch/arm/boot/dts/rk3288.dtsi drivers/soc/rockchip/pm_domains.c Dcumentation/devicetree/bindings/soc/rockchip/power_domain.txt How about refer to the Rockchip's way? Then it could also address our issues and the binding would be still like: gpc: gpc@303a0000 { compatible = "fsl,imx7d-gpc"; reg = <0x303a0000 0x1000>; interrupt-controller; interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>; #interrupt-cells = <3>; interrupt-parent = <&intc>; pgc { #address-cells = <1>; #size-cells = <0>; pgc_pcie_phy: power-domain@IMX7_POWER_DOMAIN_PCIE_PHY { reg = <IMX7_POWER_DOMAIN_PCIE_PHY>; power-supply = <®_1p0d>; clocks = <xxx>; }; .... }; }; It also drops #power-domain-cells and register domain by one provider with multi domains which is more align with HW. How do you think of it? Regards Dong Aisheng > This is needed as for the upstream version we are controlling the > regulator from the GPC driver, as opposed to the downstream version, > where each device has to implement the regulator handling and power > up/down sequencing. > > See the rationale in the commits adding the multidomain support to the > i.MX6 GPC. > > Regards, > Lucas > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v7 2/2] soc/imx: Add GPCv2 power gating driver 2017-03-30 7:51 ` Dong Aisheng @ 2017-03-29 16:08 ` Lucas Stach 2017-04-01 4:10 ` Dong Aisheng 0 siblings, 1 reply; 16+ messages in thread From: Lucas Stach @ 2017-03-29 16:08 UTC (permalink / raw) To: Dong Aisheng Cc: Andrey Smirnov, Shawn Guo, yurovsky, Fabio Estevam, linux-arm-kernel, linux-kernel Hi Dong, Am Donnerstag, den 30.03.2017, 15:51 +0800 schrieb Dong Aisheng: > Hi Lucas, > > On Thu, Mar 23, 2017 at 03:35:49PM +0100, Lucas Stach wrote: > > Hi Dong, > > > > Am Freitag, den 24.03.2017, 14:24 +0800 schrieb Dong Aisheng: > > [...] > > > > +static struct platform_driver imx7_pgc_domain_driver = { > > > > + .driver = { > > > > + .name = "imx7-pgc", > > > > + }, > > > > + .probe = imx7_pgc_domain_probe, > > > > + .remove = imx7_pgc_domain_remove, > > > > + .id_table = imx7_pgc_domain_id, > > > > +}; > > > > +builtin_platform_driver(imx7_pgc_domain_driver) > > > > > > Again, i have a fundamental question about this patch implementation > > > that why we choose above way to register the power domain? > > > > > > I'm sorry that i did not know too much history. > > > Would you guys please help share some information? > > > > > > Because AFAIK this way will register each domain as a power domain > > > provider which is a bit violate the real HW and current power domain > > > framework design. And it is a bit more complicated to use than before. > > > > > > IMHO i would rather prefer the old traditional and simpler way that one > > > provider (GPC) supplies multiple domains (PCIE/MIPI/HSIC PHY domain) > > > than this patch does. > > > > > > However, i might be wrong. Please help to clear. > > > > This way we can properly describe each power domain with the regulator > > supplying the domain and the clocks of the devices inside the domain in > > the device tree. > > > > Thanks for the explaination. I understand that purpose. > > Now my concern is why we doing things like this: > Builtin two platforms driver and use one to dynamically create > device to trigger another driver bind to register the domain. > > static int imx7_pgc_domain_probe(struct platform_device *pdev) > { > of_genpd_add_provider_simple(domain->dev->of_node, > &domain->genpd); > } > > static struct platform_driver imx7_pgc_domain_driver = { > .driver = { > .name = "imx7-pgc", > }, > .probe = imx7_pgc_domain_probe, > }; > builtin_platform_driver(imx7_pgc_domain_driver) > > > static int imx_gpcv2_probe(struct platform_device *pdev) > { > > for_each_child_of_node(pgc_np, np) { > pd_pdev = platform_device_alloc("imx7-pgc-domain", > domain_index); > ret = platform_device_add(pd_pdev); > } > } > > static struct platform_driver imx_gpc_driver = { > .driver = { > .name = "imx-gpcv2", > .of_match_table = imx_gpcv2_dt_ids, > }, > .probe = imx_gpcv2_probe, > }; > builtin_platform_driver(imx_gpc_driver) > > Is there any special purpose or i missed something? Yes, clocks and regulators can be looked up by the devices attached to the DT nodes. This makes handling of those easy (or at all possible, the regulator API doesn't allow to get regulators without the proper devnode attached to a DT node). > Can we just use one or a simple core_initcall(imx_gpcv2_probe) cause > this probably should be registered early for other consumers? Initcall levels are not going to work. We are dealing with regulators, which can have supplies that are only probed when other modules are loaded. If we need the domains to be up before the consumers, the only way to deal with that is to select CONFIG_PM and CONFIG_PM_GENERIC_DOMAINS from the platform, so we have proper probe defer handling for consumer devices of the power domains. > Personally i'd be more like Rockchip's power domain implementation. Why? > See: > arch/arm/boot/dts/rk3288.dtsi > drivers/soc/rockchip/pm_domains.c > Dcumentation/devicetree/bindings/soc/rockchip/power_domain.txt > > How about refer to the Rockchip's way? Why? We just changed the way how it's done for GPCv1, after more than 1 year of those patches being on the list. Why should we do it differently for GPCv2? > > Then it could also address our issues and the binding would be > still like: > gpc: gpc@303a0000 { > compatible = "fsl,imx7d-gpc"; > reg = <0x303a0000 0x1000>; > interrupt-controller; > interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>; > #interrupt-cells = <3>; > interrupt-parent = <&intc>; > > pgc { > #address-cells = <1>; > #size-cells = <0>; > > pgc_pcie_phy: power-domain@IMX7_POWER_DOMAIN_PCIE_PHY { > reg = <IMX7_POWER_DOMAIN_PCIE_PHY>; > power-supply = <®_1p0d>; > clocks = <xxx>; > }; > > .... > }; > }; > > It also drops #power-domain-cells and register domain by > one provider with multi domains which is more align with HW. > > How do you think of it? How is this more aligned with the hardware? Both options are an arbitrary abstraction chosen in the DT binding. Regards, Lucas ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v7 2/2] soc/imx: Add GPCv2 power gating driver 2017-03-29 16:08 ` Lucas Stach @ 2017-04-01 4:10 ` Dong Aisheng 2017-03-31 12:28 ` Lucas Stach 0 siblings, 1 reply; 16+ messages in thread From: Dong Aisheng @ 2017-04-01 4:10 UTC (permalink / raw) To: Lucas Stach Cc: Andrey Smirnov, Shawn Guo, yurovsky, Fabio Estevam, linux-arm-kernel, linux-kernel, rjw Hi Lucas, Thanks for the explaination. On Wed, Mar 29, 2017 at 06:08:31PM +0200, Lucas Stach wrote: > Hi Dong, > > Am Donnerstag, den 30.03.2017, 15:51 +0800 schrieb Dong Aisheng: > > Hi Lucas, > > > > On Thu, Mar 23, 2017 at 03:35:49PM +0100, Lucas Stach wrote: > > > Hi Dong, > > > > > > Am Freitag, den 24.03.2017, 14:24 +0800 schrieb Dong Aisheng: > > > [...] > > > > > +static struct platform_driver imx7_pgc_domain_driver = { > > > > > + .driver = { > > > > > + .name = "imx7-pgc", > > > > > + }, > > > > > + .probe = imx7_pgc_domain_probe, > > > > > + .remove = imx7_pgc_domain_remove, > > > > > + .id_table = imx7_pgc_domain_id, > > > > > +}; > > > > > +builtin_platform_driver(imx7_pgc_domain_driver) > > > > > > > > Again, i have a fundamental question about this patch implementation > > > > that why we choose above way to register the power domain? > > > > > > > > I'm sorry that i did not know too much history. > > > > Would you guys please help share some information? > > > > > > > > Because AFAIK this way will register each domain as a power domain > > > > provider which is a bit violate the real HW and current power domain > > > > framework design. And it is a bit more complicated to use than before. > > > > > > > > IMHO i would rather prefer the old traditional and simpler way that one > > > > provider (GPC) supplies multiple domains (PCIE/MIPI/HSIC PHY domain) > > > > than this patch does. > > > > > > > > However, i might be wrong. Please help to clear. > > > > > > This way we can properly describe each power domain with the regulator > > > supplying the domain and the clocks of the devices inside the domain in > > > the device tree. > > > > > > > Thanks for the explaination. I understand that purpose. > > > > Now my concern is why we doing things like this: > > Builtin two platforms driver and use one to dynamically create > > device to trigger another driver bind to register the domain. > > > > static int imx7_pgc_domain_probe(struct platform_device *pdev) > > { > > of_genpd_add_provider_simple(domain->dev->of_node, > > &domain->genpd); > > } > > > > static struct platform_driver imx7_pgc_domain_driver = { > > .driver = { > > .name = "imx7-pgc", > > }, > > .probe = imx7_pgc_domain_probe, > > }; > > builtin_platform_driver(imx7_pgc_domain_driver) > > > > > > static int imx_gpcv2_probe(struct platform_device *pdev) > > { > > > > for_each_child_of_node(pgc_np, np) { > > pd_pdev = platform_device_alloc("imx7-pgc-domain", > > domain_index); > > ret = platform_device_add(pd_pdev); > > } > > } > > > > static struct platform_driver imx_gpc_driver = { > > .driver = { > > .name = "imx-gpcv2", > > .of_match_table = imx_gpcv2_dt_ids, > > }, > > .probe = imx_gpcv2_probe, > > }; > > builtin_platform_driver(imx_gpc_driver) > > > > Is there any special purpose or i missed something? > > Yes, clocks and regulators can be looked up by the devices attached to > the DT nodes. This makes handling of those easy (or at all possible, the > regulator API doesn't allow to get regulators without the proper devnode > attached to a DT node). > That probably is not true. Regulator API does allow to get regulators without dev or devnode parameter. e.g. reg_arm = regulator_get(NULL, "vddarm"); However, i did feel like that this using is not quite suitable for DT users while it introduces limitation and dependencies in device tree. Then, platform driver/device mode seems truely a better approach for this issue. > > Can we just use one or a simple core_initcall(imx_gpcv2_probe) cause > > this probably should be registered early for other consumers? > > Initcall levels are not going to work. We are dealing with regulators, > which can have supplies that are only probed when other modules are > loaded. I see. A clear explain. > If we need the domains to be up before the consumers, the only > way to deal with that is to select CONFIG_PM and > CONFIG_PM_GENERIC_DOMAINS from the platform, so we have proper probe > defer handling for consumer devices of the power domains. > A bit confuse about these words... > > Personally i'd be more like Rockchip's power domain implementation. > > Why? > > > See: > > arch/arm/boot/dts/rk3288.dtsi > > drivers/soc/rockchip/pm_domains.c > > Dcumentation/devicetree/bindings/soc/rockchip/power_domain.txt > > > > How about refer to the Rockchip's way? > > Why? We just changed the way how it's done for GPCv1, after more than 1 > year of those patches being on the list. Why should we do it differently > for GPCv2? > Hmm? I just thought your GPCv1 change was picked a few weeks ago (Feb 17 2017) and there's no current users in kernel of the new binding. That's why i come out of the idea if we could improve it before any users. Maybe i made mistake? See: commit 721cabf6c6600dbe689ee2782bc087270e97e652 Author: Lucas Stach <l.stach@pengutronix.de> Date: Fri Feb 17 20:02:44 2017 +0100 soc: imx: move PGC handling to a new GPC driver This is an almost complete re-write of the previous GPC power gating control code found in the IMX architecture code. It supports both the old and the new DT binding, allowing more domains to be added later and generally makes the driver easier to extend, while keeping compatibility with existing DTBs. As the result, all functionality regarding the power gating controller gets removed from the IMX architecture GPC driver. It keeps only the IRQ controller code in the architecture, as this is closely coupled to the CPU idle implementation. Signed-off-by: Lucas Stach <l.stach@pengutronix.de> Signed-off-by: Shawn Guo <shawnguo@kernel.org> > > > > Then it could also address our issues and the binding would be > > still like: > > gpc: gpc@303a0000 { > > compatible = "fsl,imx7d-gpc"; > > reg = <0x303a0000 0x1000>; > > interrupt-controller; > > interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>; > > #interrupt-cells = <3>; > > interrupt-parent = <&intc>; > > > > pgc { > > #address-cells = <1>; > > #size-cells = <0>; > > > > pgc_pcie_phy: power-domain@IMX7_POWER_DOMAIN_PCIE_PHY { > > reg = <IMX7_POWER_DOMAIN_PCIE_PHY>; > > power-supply = <®_1p0d>; > > clocks = <xxx>; > > }; > > > > .... > > }; > > }; > > > > It also drops #power-domain-cells and register domain by > > one provider with multi domains which is more align with HW. > > > > How do you think of it? > > How is this more aligned with the hardware? Both options are an > arbitrary abstraction chosen in the DT binding. > GPC is a Power Controller which controls multi power domains to subsystem by its different registers bits. e.g. PCIE/MIPI/USB HSIC PHY. • 0xC00 ~ 0xC3F: PGC for MIPI PHY • 0xC40 ~ 0xC7F: PGC for PCIE_PHY • 0xD00 ~ 0xD3F: PGC for USB HSIC PHY So i thought it looks more like GPC is a power domain provider with multi domains support to different subsystems from HW point of view. Isn't that true? And there's also other two concerns: First, current genpd sysfs output still not include provider. e.g. root@imx6qdlsolo:~# cat /sys/kernel/debug/pm_genpd/pm_genpd_summary domain status slaves /device runtime status ---------------------------------------------------------------------- PU off-0 /devices/soc0/soc/130000.gpu suspended /devices/soc0/soc/134000.gpu suspended /devices/soc0/soc/2204000.gpu suspended /devices/soc0/soc/2000000.aips-bus/2040000.vpu suspended ARM off-0 I wonder it might be a bit mess once the provider is added while each domain is registered as a virtual provider. Second, it sacrifices a bit performance when look-up PM domain in genpd_get_from_provider if every domain is a provider. Though it is arguable that currently only 3 domains support on MX7, but who knows the future when it becomes much more. However, i did see many exist users in kernel using one provider one domain way. Maybe i'm over worried and it's not big deal. Rafael, Would you provide some guidance on this issue? > Regards, > Lucas > Regards Dong Aisheng ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v7 2/2] soc/imx: Add GPCv2 power gating driver 2017-04-01 4:10 ` Dong Aisheng @ 2017-03-31 12:28 ` Lucas Stach 2017-04-11 3:22 ` Dong Aisheng 0 siblings, 1 reply; 16+ messages in thread From: Lucas Stach @ 2017-03-31 12:28 UTC (permalink / raw) To: Dong Aisheng Cc: Andrey Smirnov, Shawn Guo, yurovsky, Fabio Estevam, linux-arm-kernel, linux-kernel, rjw Hi Dong, Am Samstag, den 01.04.2017, 12:10 +0800 schrieb Dong Aisheng: [...] > > If we need the domains to be up before the consumers, the only > > way to deal with that is to select CONFIG_PM and > > CONFIG_PM_GENERIC_DOMAINS from the platform, so we have proper probe > > defer handling for consumer devices of the power domains. > > > > A bit confuse about these words... If those options are selected we get proper PROBE_DEFER handling for consumers of the power domain, so probe order doesn't matter. > > > Personally i'd be more like Rockchip's power domain implementation. > > > > Why? > > > > > See: > > > arch/arm/boot/dts/rk3288.dtsi > > > drivers/soc/rockchip/pm_domains.c > > > Dcumentation/devicetree/bindings/soc/rockchip/power_domain.txt > > > > > > How about refer to the Rockchip's way? > > > > Why? We just changed the way how it's done for GPCv1, after more than 1 > > year of those patches being on the list. Why should we do it differently > > for GPCv2? > > > > Hmm? > > I just thought your GPCv1 change was picked a few weeks ago (Feb 17 2017) > and there's no current users in kernel of the new binding. > That's why i come out of the idea if we could improve it before any users. > > Maybe i made mistake? The patches to change this have been out for over 1 year. I'm less than motivated to change the binding again, after it has gone through the DT review and has finally been picked up. > See: > commit 721cabf6c6600dbe689ee2782bc087270e97e652 > Author: Lucas Stach <l.stach@pengutronix.de> > Date: Fri Feb 17 20:02:44 2017 +0100 > > soc: imx: move PGC handling to a new GPC driver > > This is an almost complete re-write of the previous GPC power gating control > code found in the IMX architecture code. It supports both the old and the new > DT binding, allowing more domains to be added later and generally makes the > driver easier to extend, while keeping compatibility with existing DTBs. > > As the result, all functionality regarding the power gating controller > gets removed from the IMX architecture GPC driver. It keeps only the > IRQ controller code in the architecture, as this is closely coupled to > the CPU idle implementation. > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > Signed-off-by: Shawn Guo <shawnguo@kernel.org> > > > > > > > Then it could also address our issues and the binding would be > > > still like: > > > gpc: gpc@303a0000 { > > > compatible = "fsl,imx7d-gpc"; > > > reg = <0x303a0000 0x1000>; > > > interrupt-controller; > > > interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>; > > > #interrupt-cells = <3>; > > > interrupt-parent = <&intc>; > > > > > > pgc { > > > #address-cells = <1>; > > > #size-cells = <0>; > > > > > > pgc_pcie_phy: power-domain@IMX7_POWER_DOMAIN_PCIE_PHY { > > > reg = <IMX7_POWER_DOMAIN_PCIE_PHY>; > > > power-supply = <®_1p0d>; > > > clocks = <xxx>; > > > }; > > > > > > .... > > > }; > > > }; > > > > > > It also drops #power-domain-cells and register domain by > > > one provider with multi domains which is more align with HW. > > > > > > How do you think of it? > > > > How is this more aligned with the hardware? Both options are an > > arbitrary abstraction chosen in the DT binding. > > > > GPC is a Power Controller which controls multi power domains to subsystem > by its different registers bits. > e.g. PCIE/MIPI/USB HSIC PHY. > • 0xC00 ~ 0xC3F: PGC for MIPI PHY > • 0xC40 ~ 0xC7F: PGC for PCIE_PHY > • 0xD00 ~ 0xD3F: PGC for USB HSIC PHY > > So i thought it looks more like GPC is a power domain provider with multi > domains support to different subsystems from HW point of view. > > Isn't that true? Linux and hardware devices are not required to match 1:1. There is nothing that would make subdividing a single hardware device into multiple ones bad style. > > And there's also other two concerns: > First, current genpd sysfs output still not include provider. > e.g. > root@imx6qdlsolo:~# cat /sys/kernel/debug/pm_genpd/pm_genpd_summary > domain status slaves > /device runtime status > ---------------------------------------------------------------------- > PU off-0 > /devices/soc0/soc/130000.gpu suspended > /devices/soc0/soc/134000.gpu suspended > /devices/soc0/soc/2204000.gpu suspended > /devices/soc0/soc/2000000.aips-bus/2040000.vpu suspended > ARM off-0 > > I wonder it might be a bit mess once the provider is added while each > domain is registered as a virtual provider. The provider is a Linux device. Linux devices don't necessarily have to correspond to hardware devices. There is no such rule. > > Second, it sacrifices a bit performance when look-up PM domain in > genpd_get_from_provider if every domain is a provider. > The performance penalty of a list walk won't hurt us in the probe path, where we are (re-)probing entire devices. There is a lot more going on than a simple list walk. > Though it is arguable that currently only 3 domains support on MX7, > but who knows the future when it becomes much more. > > However, i did see many exist users in kernel using one provider one > domain way. Maybe i'm over worried and it's not big deal. I see that one provider with multiple domains is used more often, that doesn't means it's necessarily better. At least I haven't hear a convincing argument on why the chosen implementation in the GPC driver is worse than the alternative. Regards, Lucas ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v7 2/2] soc/imx: Add GPCv2 power gating driver 2017-03-31 12:28 ` Lucas Stach @ 2017-04-11 3:22 ` Dong Aisheng 0 siblings, 0 replies; 16+ messages in thread From: Dong Aisheng @ 2017-04-11 3:22 UTC (permalink / raw) To: Lucas Stach Cc: Andrey Smirnov, Shawn Guo, yurovsky, Fabio Estevam, linux-arm-kernel, linux-kernel, rjw On Fri, Mar 31, 2017 at 02:28:11PM +0200, Lucas Stach wrote: > Hi Dong, > > Am Samstag, den 01.04.2017, 12:10 +0800 schrieb Dong Aisheng: > [...] > > > If we need the domains to be up before the consumers, the only > > > way to deal with that is to select CONFIG_PM and > > > CONFIG_PM_GENERIC_DOMAINS from the platform, so we have proper probe > > > defer handling for consumer devices of the power domains. > > > > > > > A bit confuse about these words... > > If those options are selected we get proper PROBE_DEFER handling for > consumers of the power domain, so probe order doesn't matter. > > > > > Personally i'd be more like Rockchip's power domain implementation. > > > > > > Why? > > > > > > > See: > > > > arch/arm/boot/dts/rk3288.dtsi > > > > drivers/soc/rockchip/pm_domains.c > > > > Dcumentation/devicetree/bindings/soc/rockchip/power_domain.txt > > > > > > > > How about refer to the Rockchip's way? > > > > > > Why? We just changed the way how it's done for GPCv1, after more than 1 > > > year of those patches being on the list. Why should we do it differently > > > for GPCv2? > > > > > > > Hmm? > > > > I just thought your GPCv1 change was picked a few weeks ago (Feb 17 2017) > > and there's no current users in kernel of the new binding. > > That's why i come out of the idea if we could improve it before any users. > > > > Maybe i made mistake? > > The patches to change this have been out for over 1 year. I'm less than > motivated to change the binding again, after it has gone through the DT > review and has finally been picked up. > > > See: > > commit 721cabf6c6600dbe689ee2782bc087270e97e652 > > Author: Lucas Stach <l.stach@pengutronix.de> > > Date: Fri Feb 17 20:02:44 2017 +0100 > > > > soc: imx: move PGC handling to a new GPC driver > > > > This is an almost complete re-write of the previous GPC power gating control > > code found in the IMX architecture code. It supports both the old and the new > > DT binding, allowing more domains to be added later and generally makes the > > driver easier to extend, while keeping compatibility with existing DTBs. > > > > As the result, all functionality regarding the power gating controller > > gets removed from the IMX architecture GPC driver. It keeps only the > > IRQ controller code in the architecture, as this is closely coupled to > > the CPU idle implementation. > > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > > Signed-off-by: Shawn Guo <shawnguo@kernel.org> > > > > > > > > > > Then it could also address our issues and the binding would be > > > > still like: > > > > gpc: gpc@303a0000 { > > > > compatible = "fsl,imx7d-gpc"; > > > > reg = <0x303a0000 0x1000>; > > > > interrupt-controller; > > > > interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>; > > > > #interrupt-cells = <3>; > > > > interrupt-parent = <&intc>; > > > > > > > > pgc { > > > > #address-cells = <1>; > > > > #size-cells = <0>; > > > > > > > > pgc_pcie_phy: power-domain@IMX7_POWER_DOMAIN_PCIE_PHY { > > > > reg = <IMX7_POWER_DOMAIN_PCIE_PHY>; > > > > power-supply = <®_1p0d>; > > > > clocks = <xxx>; > > > > }; > > > > > > > > .... > > > > }; > > > > }; > > > > > > > > It also drops #power-domain-cells and register domain by > > > > one provider with multi domains which is more align with HW. > > > > > > > > How do you think of it? > > > > > > How is this more aligned with the hardware? Both options are an > > > arbitrary abstraction chosen in the DT binding. > > > > > > > GPC is a Power Controller which controls multi power domains to subsystem > > by its different registers bits. > > e.g. PCIE/MIPI/USB HSIC PHY. > > • 0xC00 ~ 0xC3F: PGC for MIPI PHY > > • 0xC40 ~ 0xC7F: PGC for PCIE_PHY > > • 0xD00 ~ 0xD3F: PGC for USB HSIC PHY > > > > So i thought it looks more like GPC is a power domain provider with multi > > domains support to different subsystems from HW point of view. > > > > Isn't that true? > > Linux and hardware devices are not required to match 1:1. There is > nothing that would make subdividing a single hardware device into > multiple ones bad style. > > > > > And there's also other two concerns: > > First, current genpd sysfs output still not include provider. > > e.g. > > root@imx6qdlsolo:~# cat /sys/kernel/debug/pm_genpd/pm_genpd_summary > > domain status slaves > > /device runtime status > > ---------------------------------------------------------------------- > > PU off-0 > > /devices/soc0/soc/130000.gpu suspended > > /devices/soc0/soc/134000.gpu suspended > > /devices/soc0/soc/2204000.gpu suspended > > /devices/soc0/soc/2000000.aips-bus/2040000.vpu suspended > > ARM off-0 > > > > I wonder it might be a bit mess once the provider is added while each > > domain is registered as a virtual provider. > > The provider is a Linux device. Linux devices don't necessarily have to > correspond to hardware devices. There is no such rule. > > > > > Second, it sacrifices a bit performance when look-up PM domain in > > genpd_get_from_provider if every domain is a provider. > > > The performance penalty of a list walk won't hurt us in the probe path, > where we are (re-)probing entire devices. There is a lot more going on > than a simple list walk. > It is mostly care in a simulation platform like Zebu while the code execution time is quite long even it's very small in real word. > > Though it is arguable that currently only 3 domains support on MX7, > > but who knows the future when it becomes much more. > > > > However, i did see many exist users in kernel using one provider one > > domain way. Maybe i'm over worried and it's not big deal. > > I see that one provider with multiple domains is used more often, that > doesn't means it's necessarily better. At least I haven't hear a > convincing argument on why the chosen implementation in the GPC driver > is worse than the alternative. > Well, this is not a strong objection. I could also accept it if no objection from maintainer. And seems Shawn already picked the patches. So never mind, let's keep going on. Regards Dong Aisheg > Regards, > Lucas > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v7 2/2] soc/imx: Add GPCv2 power gating driver 2017-03-24 6:24 ` Dong Aisheng 2017-03-23 14:35 ` Lucas Stach @ 2017-03-27 18:42 ` Andrey Smirnov 2017-03-30 6:58 ` Dong Aisheng 1 sibling, 1 reply; 16+ messages in thread From: Andrey Smirnov @ 2017-03-27 18:42 UTC (permalink / raw) To: Dong Aisheng Cc: Shawn Guo, Andrey Yurovsky, Lucas Stach, Fabio Estevam, linux-arm-kernel, linux-kernel On Thu, Mar 23, 2017 at 11:24 PM, Dong Aisheng <dongas86@gmail.com> wrote: > On Tue, Mar 21, 2017 at 07:50:04AM -0700, Andrey Smirnov wrote: >> Add code allowing for control of various power domains managed by GPCv2 >> IP block found in i.MX7 series of SoCs. Power domains covered by this >> patch are: >> >> - PCIE PHY >> - MIPI PHY >> - USB HSIC PHY >> - USB OTG1/2 PHY >> > > You probably may need drop USB OTG which is not claimed in current RM. > See the PGC definition in 5.5.10 GPC Memory Map section. > > Each PGC (CPU type, MIX type, PU type) will occupy 64 Bytes address space, > the specific base address of each PGC are listed as below. > • 0x800 ~ 0x83F : PGC for A7 core0 > • 0x840 ~ 0x87F: PGC for A7 core1 > • 0x880 ~ 0x8BF: PGC for A7 SCU > • 0xA00 ~ 0xA3F: PGC for fastmix/megamix > • 0xC00 ~ 0xC3F: PGC for MIPI PHY > • 0xC40 ~ 0xC7F: PGC for PCIE_PHY > • 0xC80 ~ 0xCBF: Reserved > • 0xCC0 ~ 0xCFF: Reserved > • 0xD00 ~ 0xD3F: PGC for USB HSIC PHY > > And in 5.4 Power Management Unit (PMU) chapter, > you will find the USB OTG phy power is directly supplied by > VDD_USB_OTG1_3P3_IN/VDD_USB_OTG2_3P3_IN. > > http://www.nxp.com/assets/documents/data/en/reference-manuals/IMX7DRM.pdf > > I understand that there's also some USB OTG code exist in NXP internal > tree, but that's legacy for early doc implementation and may be deprecated. > so i assume it should be gone. > > Hopefully i will double confirm with our IC designer tomorrow. > USB OTG domains are absent from that list, true, but they are mentioned all of the place further in that section in register map documentation, which makes it difficult to tell which part of the datasheet is not up to date. I'm going to drop those power domains for now, since I don't have a use-case for them and it would also allow me to get rid of the chunk of code you thought was messy. However it would be nice to get an updated version of RM where all of that is straightened out. >> Support for any other power domain controlled by GPC is not present, and >> can be added at some later point. >> >> Testing of this code was done against a PCIe driver. >> >> Cc: yurovsky@gmail.com >> Cc: Lucas Stach <l.stach@pengutronix.de> >> Cc: Fabio Estevam <fabio.estevam@nxp.com> >> Cc: Dong Aisheng <dongas86@gmail.com> >> Cc: linux-arm-kernel@lists.infradead.org >> Cc: linux-kernel@vger.kernel.org >> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> >> --- >> drivers/soc/Kconfig | 1 + >> drivers/soc/imx/Kconfig | 10 ++ >> drivers/soc/imx/Makefile | 1 + >> drivers/soc/imx/gpcv2.c | 365 +++++++++++++++++++++++++++++++++++++++++++++++ >> 4 files changed, 377 insertions(+) >> create mode 100644 drivers/soc/imx/Kconfig >> create mode 100644 drivers/soc/imx/gpcv2.c >> >> diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig >> index f09023f..8943543 100644 >> --- a/drivers/soc/Kconfig >> +++ b/drivers/soc/Kconfig >> @@ -2,6 +2,7 @@ menu "SOC (System On Chip) specific Drivers" >> >> source "drivers/soc/bcm/Kconfig" >> source "drivers/soc/fsl/Kconfig" >> +source "drivers/soc/imx/Kconfig" >> source "drivers/soc/mediatek/Kconfig" >> source "drivers/soc/qcom/Kconfig" >> source "drivers/soc/rockchip/Kconfig" >> diff --git a/drivers/soc/imx/Kconfig b/drivers/soc/imx/Kconfig >> new file mode 100644 >> index 0000000..bc7f0ee0 >> --- /dev/null >> +++ b/drivers/soc/imx/Kconfig >> @@ -0,0 +1,10 @@ >> +menu "i.MX SoC drivers" >> + >> +config IMX7_PM_DOMAINS >> + bool "i.MX7 PM domains" >> + select PM_GENERIC_DOMAINS >> + depends on SOC_IMX7D || (COMPILE_TEST && OF) >> + default y if SOC_IMX7D >> + >> +endmenu >> + >> diff --git a/drivers/soc/imx/Makefile b/drivers/soc/imx/Makefile >> index 35861f5..5b6e396 100644 >> --- a/drivers/soc/imx/Makefile >> +++ b/drivers/soc/imx/Makefile >> @@ -1 +1,2 @@ >> obj-y += gpc.o >> +obj-$(CONFIG_IMX7_PM_DOMAINS) += gpcv2.o >> diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c >> new file mode 100644 >> index 0000000..a27c5f8 >> --- /dev/null >> +++ b/drivers/soc/imx/gpcv2.c >> @@ -0,0 +1,365 @@ >> +/* >> + * Copyright 2017 Impinj, Inc >> + * Author: Andrey Smirnov <andrew.smirnov@gmail.com> >> + * >> + * Based on the code of analogus driver: >> + * >> + * Copyright 2015-2017 Pengutronix, Lucas Stach <kernel@pengutronix.de> >> + * >> + * The code contained herein is licensed under the GNU General Public >> + * License. You may obtain a copy of the GNU General Public License >> + * Version 2 or later at the following locations: >> + * >> + * http://www.opensource.org/licenses/gpl-license.html >> + * http://www.gnu.org/copyleft/gpl.html >> + */ >> + >> +#include <linux/platform_device.h> >> +#include <linux/pm_domain.h> >> +#include <linux/regmap.h> >> +#include <linux/regulator/consumer.h> >> +#include <dt-bindings/power/imx7-power.h> >> + >> +#define GPC_PGC_CPU_MAPPING 0xec >> +#define USB_HSIC_PHY_A7_DOMAIN BIT(6) >> +#define USB_OTG2_PHY_A7_DOMAIN BIT(5) >> +#define USB_OTG1_PHY_A7_DOMAIN BIT(4) >> +#define PCIE_PHY_A7_DOMAIN BIT(3) >> +#define MIPI_PHY_A7_DOMAIN BIT(2) >> + >> +#define GPC_PU_PGC_SW_PUP_REQ 0xf8 >> +#define GPC_PU_PGC_SW_PDN_REQ 0x104 >> +#define USB_HSIC_PHY_SW_Pxx_REQ BIT(4) >> +#define USB_OTG2_PHY_SW_Pxx_REQ BIT(3) >> +#define USB_OTG1_PHY_SW_Pxx_REQ BIT(2) >> +#define PCIE_PHY_SW_Pxx_REQ BIT(1) >> +#define MIPI_PHY_SW_Pxx_REQ BIT(0) >> + > > After apply, what i see is: > #define GPC_PU_PGC_SW_PUP_REQ 0xf8 > #define GPC_PU_PGC_SW_PDN_REQ 0x104 > #define USB_HSIC_PHY_SW_Pxx_REQ BIT(4) > #define USB_OTG2_PHY_SW_Pxx_REQ BIT(3) > #define USB_OTG1_PHY_SW_Pxx_REQ BIT(2) > #define PCIE_PHY_SW_Pxx_REQ BIT(1) > #define MIPI_PHY_SW_Pxx_REQ BIT(0) > > Looks quite tight, > Can we have one more TAB for the definition? > Like: > #define GPC_PU_PGC_SW_PUP_REQ 0xf8 > Sure, will do. >> +#define GPC_MAX_REGISTER 0x1000 >> + >> +#define GPC_PGC_nCTRL_PCR BIT(0) >> + >> +struct imx7_pgc_domain { >> + struct generic_pm_domain genpd; >> + struct regmap *regmap; >> + struct regulator *regulator; >> + >> + unsigned int pgc_nctrl; >> + >> + const struct { >> + u32 pxx; >> + u32 map; >> + } bits; >> + >> + const int voltage; >> + struct device *dev; >> +}; >> + >> +static int imx7_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain *genpd, >> + bool on) >> +{ >> + struct imx7_pgc_domain *domain = container_of(genpd, >> + struct imx7_pgc_domain, >> + genpd); >> + unsigned int offset = on ? >> + GPC_PU_PGC_SW_PUP_REQ : GPC_PU_PGC_SW_PDN_REQ; >> + const bool enable_power_control = domain->pgc_nctrl && !on; >> + const bool has_regulator = !IS_ERR(domain->regulator); >> + unsigned long deadline; >> + int ret = 0; >> + >> + regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING, >> + domain->bits.map, domain->bits.map); >> + >> + if (has_regulator && on) { >> + ret = regulator_enable(domain->regulator); >> + if (ret) { >> + dev_err(domain->dev, "failed to enable regulator\n"); >> + goto unmap; >> + } >> + } >> + >> + if (enable_power_control) >> + regmap_update_bits(domain->regmap, domain->pgc_nctrl, >> + GPC_PGC_nCTRL_PCR, GPC_PGC_nCTRL_PCR); >> + >> + regmap_update_bits(domain->regmap, offset, >> + domain->bits.pxx, domain->bits.pxx); >> + >> + /* >> + * As per "5.5.9.4 Example Code 4" in IMX7DRM.pdf wait >> + * for PUP_REQ/PDN_REQ bit to be cleared >> + */ >> + deadline = jiffies + msecs_to_jiffies(1); >> + while (true) { >> + u32 pxx_req; >> + >> + regmap_read(domain->regmap, offset, &pxx_req); >> + >> + if (!(pxx_req & domain->bits.pxx)) >> + break; >> + >> + if (time_after(jiffies, deadline)) { >> + dev_err(domain->dev, "falied to command PGC\n"); >> + ret = -ETIMEDOUT; >> + /* >> + * If we were in a process of enabling a >> + * domain and failed we might as well disable >> + * the regulator we just enabled. And if it >> + * was the opposite situation and we failed to >> + * power down -- keep the regulator on >> + */ >> + on = !on; >> + break; >> + } >> + >> + cpu_relax(); >> + } >> + >> + if (enable_power_control) >> + regmap_update_bits(domain->regmap, domain->pgc_nctrl, >> + GPC_PGC_nCTRL_PCR, 0); >> + >> + if (has_regulator && !on) { >> + int err; >> + >> + err = regulator_disable(domain->regulator); >> + if (err) >> + dev_err(domain->dev, >> + "failed to disable regulator: %d\n", ret); >> + /* Preserve earlier error code */ >> + ret = ret ?: err; >> + } >> +unmap: >> + regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING, >> + domain->bits.map, 0); >> + return ret; >> +} >> + >> +static int imx7_gpc_pu_pgc_sw_pup_req(struct generic_pm_domain *genpd) >> +{ >> + return imx7_gpc_pu_pgc_sw_pxx_req(genpd, true); >> +} >> + >> +static int imx7_gpc_pu_pgc_sw_pdn_req(struct generic_pm_domain *genpd) >> +{ >> + return imx7_gpc_pu_pgc_sw_pxx_req(genpd, false); >> +} >> + >> +static struct imx7_pgc_domain imx7_pgc_domains[] = { >> + [IMX7_POWER_DOMAIN_USB_HSIC_PHY] = { >> + .genpd = { >> + .name = "usb-hsic-phy", >> + }, >> + .bits = { >> + .pxx = USB_HSIC_PHY_SW_Pxx_REQ, >> + .map = USB_HSIC_PHY_A7_DOMAIN, >> + }, >> + .voltage = 1200000, >> + .pgc_nctrl = 0x0d00, >> + }, >> + >> + [IMX7_POWER_DOMAIN_USB_OTG2_PHY] = { >> + .genpd = { >> + .name = "usb-otg2-phy", >> + }, >> + .bits = { >> + .pxx = USB_OTG2_PHY_SW_Pxx_REQ, >> + .map = USB_OTG2_PHY_A7_DOMAIN, >> + }, >> + }, >> + >> + [IMX7_POWER_DOMAIN_USB_OTG1_PHY] = { >> + .genpd = { >> + .name = "usb-otg1-phy", >> + }, >> + .bits = { >> + .pxx = USB_OTG1_PHY_SW_Pxx_REQ, >> + .map = USB_OTG1_PHY_A7_DOMAIN, >> + }, >> + }, >> + >> + [IMX7_POWER_DOMAIN_PCIE_PHY] = { >> + .genpd = { >> + .name = "pcie-phy", >> + }, >> + .bits = { >> + .pxx = PCIE_PHY_SW_Pxx_REQ, >> + .map = PCIE_PHY_A7_DOMAIN, >> + }, >> + .voltage = 1000000, >> + .pgc_nctrl = 0x0c40, >> + }, >> + >> + [IMX7_POWER_DOMAIN_MIPI_PHY] = { >> + .genpd = { >> + .name = "mipi-phy", >> + }, >> + .bits = { >> + .pxx = MIPI_PHY_SW_Pxx_REQ, >> + .map = MIPI_PHY_A7_DOMAIN, >> + }, >> + .voltage = 1000000, >> + .pgc_nctrl = 0x0c00, >> + }, >> +}; >> + >> +static int imx7_pgc_domain_probe(struct platform_device *pdev) >> +{ >> + struct imx7_pgc_domain *domain = pdev->dev.platform_data; >> + int ret; >> + >> + domain->dev = &pdev->dev; >> + >> + ret = pm_genpd_init(&domain->genpd, NULL, true); >> + if (ret) { >> + dev_err(domain->dev, "Failed to init power domain\n"); >> + return ret; >> + } >> + >> + domain->regulator = devm_regulator_get_optional(domain->dev, "power"); >> + if (IS_ERR(domain->regulator) && >> + PTR_ERR(domain->regulator) != -ENODEV) { >> + dev_err(domain->dev, "Failed to get domain's regulator\n"); >> + return PTR_ERR(domain->regulator); >> + } >> + >> + if (!IS_ERR(domain->regulator)) { >> + if (!domain->voltage) { >> + WARN(1, "No voltage configured for domain's regulator"); >> + return -EINVAL; >> + } >> + >> + regulator_set_voltage(domain->regulator, >> + domain->voltage, domain->voltage); >> + } >> + >> + ret = of_genpd_add_provider_simple(domain->dev->of_node, >> + &domain->genpd); >> + if (ret) { >> + dev_err(domain->dev, "Failed to add genpd provider\n"); >> + pm_genpd_remove(&domain->genpd); >> + } >> + >> + return ret; >> +} >> + >> +static int imx7_pgc_domain_remove(struct platform_device *pdev) >> +{ >> + struct imx7_pgc_domain *domain = pdev->dev.platform_data; >> + >> + of_genpd_del_provider(domain->dev->of_node); >> + pm_genpd_remove(&domain->genpd); >> + >> + return 0; >> +} >> + >> +static const struct platform_device_id imx7_pgc_domain_id[] = { >> + { "imx7-pgc-domain", }, >> + { }, >> +}; >> + >> +static struct platform_driver imx7_pgc_domain_driver = { >> + .driver = { >> + .name = "imx7-pgc", >> + }, >> + .probe = imx7_pgc_domain_probe, >> + .remove = imx7_pgc_domain_remove, >> + .id_table = imx7_pgc_domain_id, >> +}; >> +builtin_platform_driver(imx7_pgc_domain_driver) > > Again, i have a fundamental question about this patch implementation > that why we choose above way to register the power domain? > > I'm sorry that i did not know too much history. > Would you guys please help share some information? > > Because AFAIK this way will register each domain as a power domain > provider which is a bit violate the real HW and current power domain > framework design. And it is a bit more complicated to use than before. > > IMHO i would rather prefer the old traditional and simpler way that one > provider (GPC) supplies multiple domains (PCIE/MIPI/HSIC PHY domain) > than this patch does. > > However, i might be wrong. Please help to clear. > >> + >> +static int imx_gpcv2_probe(struct platform_device *pdev) >> +{ >> + static const struct regmap_config regmap_config = { >> + .cache_type = REGCACHE_NONE, > > You could drop it as GPC does. > Yep, will remove. >> + .reg_bits = 32, >> + .val_bits = 32, >> + .reg_stride = 4, >> + .max_register = GPC_MAX_REGISTER, > > Do you need add some readable/writeable reg? > > There seems to be some reserved regs in your range. > I can easily got the below crash: > > root@imx6ul7d:/sys/kernel/debug/regmap/303a0000.gpc# cat registers > [ 54.985395] Unhandled fault: external abort on non-linefetch (0x1008) at 0xf08511c0 > [ 54.993166] pgd = ed02c000 > [ 54.995894] [f08511c0] *pgd=af00c811, *pte=303a0653, *ppte=303a0453 > [ 55.002215] Internal error: : 1008 [#1] SMP ARM > [ 55.006756] Modules linked in: > [ 55.009830] CPU: 0 PID: 750 Comm: cat Not tainted 4.11.0-rc1-00057-g2d60158-dirty #1169 > [ 55.017842] Hardware name: Freescale i.MX7 Dual (Device Tree) > [ 55.023598] task: ee20ee40 task.stack: ed0f8000 > [ 55.028146] PC is at regmap_mmio_read32le+0x14/0x24 > [ 55.033035] LR is at regmap_mmio_read+0x40/0x60 > [ 55.037577] pc : [<c054f160>] lr : [<c054f348>] psr: 20070093 > [ 55.037577] sp : ed0f9dc8 ip : ed0f9dd8 fp : ed0f9dd4 > [ 55.049064] r10: ed160000 r9 : ef341000 r8 : ed0f9f80 > [ 55.054299] r7 : ed0f9e5c r6 : ed0f9e5c r5 : 000001c0 r4 : ef3bb900 > [ 55.060836] r3 : f0851000 r2 : ed0f9e5c r1 : f08511c0 r0 : ef3bb900 > [ 55.067374] Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment none > [ 55.074606] Control: 10c5387d Table: ad02c06a DAC: 00000051 > [ 55.080361] Process cat (pid: 750, stack limit = 0xed0f8210) > .... Good catch, thank you! I'll update the code to make sure this doesn't happen. Thanks, Andrey Smirnov ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v7 2/2] soc/imx: Add GPCv2 power gating driver 2017-03-27 18:42 ` Andrey Smirnov @ 2017-03-30 6:58 ` Dong Aisheng 2017-03-30 7:04 ` Dong Aisheng 0 siblings, 1 reply; 16+ messages in thread From: Dong Aisheng @ 2017-03-30 6:58 UTC (permalink / raw) To: Andrey Smirnov Cc: Shawn Guo, Andrey Yurovsky, Lucas Stach, Fabio Estevam, linux-arm-kernel, linux-kernel On Mon, Mar 27, 2017 at 11:42:15AM -0700, Andrey Smirnov wrote: > On Thu, Mar 23, 2017 at 11:24 PM, Dong Aisheng <dongas86@gmail.com> wrote: > > On Tue, Mar 21, 2017 at 07:50:04AM -0700, Andrey Smirnov wrote: > >> Add code allowing for control of various power domains managed by GPCv2 > >> IP block found in i.MX7 series of SoCs. Power domains covered by this > >> patch are: > >> > >> - PCIE PHY > >> - MIPI PHY > >> - USB HSIC PHY > >> - USB OTG1/2 PHY > >> > > > > You probably may need drop USB OTG which is not claimed in current RM. > > See the PGC definition in 5.5.10 GPC Memory Map section. > > > > Each PGC (CPU type, MIX type, PU type) will occupy 64 Bytes address space, > > the specific base address of each PGC are listed as below. > > • 0x800 ~ 0x83F : PGC for A7 core0 > > • 0x840 ~ 0x87F: PGC for A7 core1 > > • 0x880 ~ 0x8BF: PGC for A7 SCU > > • 0xA00 ~ 0xA3F: PGC for fastmix/megamix > > • 0xC00 ~ 0xC3F: PGC for MIPI PHY > > • 0xC40 ~ 0xC7F: PGC for PCIE_PHY > > • 0xC80 ~ 0xCBF: Reserved > > • 0xCC0 ~ 0xCFF: Reserved > > • 0xD00 ~ 0xD3F: PGC for USB HSIC PHY > > > > And in 5.4 Power Management Unit (PMU) chapter, > > you will find the USB OTG phy power is directly supplied by > > VDD_USB_OTG1_3P3_IN/VDD_USB_OTG2_3P3_IN. > > > > http://www.nxp.com/assets/documents/data/en/reference-manuals/IMX7DRM.pdf > > > > I understand that there's also some USB OTG code exist in NXP internal > > tree, but that's legacy for early doc implementation and may be deprecated. > > so i assume it should be gone. > > > > Hopefully i will double confirm with our IC designer tomorrow. > > > > USB OTG domains are absent from that list, true, but they are > mentioned all of the place further in that section in register map > documentation, which makes it difficult to tell which part of the > datasheet is not up to date. > > I'm going to drop those power domains for now, since I don't have a > use-case for them and it would also allow me to get rid of the chunk > of code you thought was messy. However it would be nice to get an > updated version of RM where all of that is straightened out. > I checked with our IC designer and he confirmed the USB OTG is removed and not supported in GPC. SW should not control it, instead, its power domain is handled by hardware automatically. Currently there's true some incorrectness in GPC chapter, i already reported the issue to the designer, but still no timeline when i can get a updated version. But i think it's fine if you're going to only support PCIE/MIPI/USB HSIC PHY power domain. I suppose those bits are correct in RM. Regards Dong Aisheng > > >> Support for any other power domain controlled by GPC is not present, and > >> can be added at some later point. > >> > >> Testing of this code was done against a PCIe driver. > >> > >> Cc: yurovsky@gmail.com > >> Cc: Lucas Stach <l.stach@pengutronix.de> > >> Cc: Fabio Estevam <fabio.estevam@nxp.com> > >> Cc: Dong Aisheng <dongas86@gmail.com> > >> Cc: linux-arm-kernel@lists.infradead.org > >> Cc: linux-kernel@vger.kernel.org > >> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > >> --- > >> drivers/soc/Kconfig | 1 + > >> drivers/soc/imx/Kconfig | 10 ++ > >> drivers/soc/imx/Makefile | 1 + > >> drivers/soc/imx/gpcv2.c | 365 +++++++++++++++++++++++++++++++++++++++++++++++ > >> 4 files changed, 377 insertions(+) > >> create mode 100644 drivers/soc/imx/Kconfig > >> create mode 100644 drivers/soc/imx/gpcv2.c > >> > >> diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig > >> index f09023f..8943543 100644 > >> --- a/drivers/soc/Kconfig > >> +++ b/drivers/soc/Kconfig > >> @@ -2,6 +2,7 @@ menu "SOC (System On Chip) specific Drivers" > >> > >> source "drivers/soc/bcm/Kconfig" > >> source "drivers/soc/fsl/Kconfig" > >> +source "drivers/soc/imx/Kconfig" > >> source "drivers/soc/mediatek/Kconfig" > >> source "drivers/soc/qcom/Kconfig" > >> source "drivers/soc/rockchip/Kconfig" > >> diff --git a/drivers/soc/imx/Kconfig b/drivers/soc/imx/Kconfig > >> new file mode 100644 > >> index 0000000..bc7f0ee0 > >> --- /dev/null > >> +++ b/drivers/soc/imx/Kconfig > >> @@ -0,0 +1,10 @@ > >> +menu "i.MX SoC drivers" > >> + > >> +config IMX7_PM_DOMAINS > >> + bool "i.MX7 PM domains" > >> + select PM_GENERIC_DOMAINS > >> + depends on SOC_IMX7D || (COMPILE_TEST && OF) > >> + default y if SOC_IMX7D > >> + > >> +endmenu > >> + > >> diff --git a/drivers/soc/imx/Makefile b/drivers/soc/imx/Makefile > >> index 35861f5..5b6e396 100644 > >> --- a/drivers/soc/imx/Makefile > >> +++ b/drivers/soc/imx/Makefile > >> @@ -1 +1,2 @@ > >> obj-y += gpc.o > >> +obj-$(CONFIG_IMX7_PM_DOMAINS) += gpcv2.o > >> diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c > >> new file mode 100644 > >> index 0000000..a27c5f8 > >> --- /dev/null > >> +++ b/drivers/soc/imx/gpcv2.c > >> @@ -0,0 +1,365 @@ > >> +/* > >> + * Copyright 2017 Impinj, Inc > >> + * Author: Andrey Smirnov <andrew.smirnov@gmail.com> > >> + * > >> + * Based on the code of analogus driver: > >> + * > >> + * Copyright 2015-2017 Pengutronix, Lucas Stach <kernel@pengutronix.de> > >> + * > >> + * The code contained herein is licensed under the GNU General Public > >> + * License. You may obtain a copy of the GNU General Public License > >> + * Version 2 or later at the following locations: > >> + * > >> + * http://www.opensource.org/licenses/gpl-license.html > >> + * http://www.gnu.org/copyleft/gpl.html > >> + */ > >> + > >> +#include <linux/platform_device.h> > >> +#include <linux/pm_domain.h> > >> +#include <linux/regmap.h> > >> +#include <linux/regulator/consumer.h> > >> +#include <dt-bindings/power/imx7-power.h> > >> + > >> +#define GPC_PGC_CPU_MAPPING 0xec > >> +#define USB_HSIC_PHY_A7_DOMAIN BIT(6) > >> +#define USB_OTG2_PHY_A7_DOMAIN BIT(5) > >> +#define USB_OTG1_PHY_A7_DOMAIN BIT(4) > >> +#define PCIE_PHY_A7_DOMAIN BIT(3) > >> +#define MIPI_PHY_A7_DOMAIN BIT(2) > >> + > >> +#define GPC_PU_PGC_SW_PUP_REQ 0xf8 > >> +#define GPC_PU_PGC_SW_PDN_REQ 0x104 > >> +#define USB_HSIC_PHY_SW_Pxx_REQ BIT(4) > >> +#define USB_OTG2_PHY_SW_Pxx_REQ BIT(3) > >> +#define USB_OTG1_PHY_SW_Pxx_REQ BIT(2) > >> +#define PCIE_PHY_SW_Pxx_REQ BIT(1) > >> +#define MIPI_PHY_SW_Pxx_REQ BIT(0) > >> + > > > > After apply, what i see is: > > #define GPC_PU_PGC_SW_PUP_REQ 0xf8 > > #define GPC_PU_PGC_SW_PDN_REQ 0x104 > > #define USB_HSIC_PHY_SW_Pxx_REQ BIT(4) > > #define USB_OTG2_PHY_SW_Pxx_REQ BIT(3) > > #define USB_OTG1_PHY_SW_Pxx_REQ BIT(2) > > #define PCIE_PHY_SW_Pxx_REQ BIT(1) > > #define MIPI_PHY_SW_Pxx_REQ BIT(0) > > > > Looks quite tight, > > Can we have one more TAB for the definition? > > Like: > > #define GPC_PU_PGC_SW_PUP_REQ 0xf8 > > > > Sure, will do. > > >> +#define GPC_MAX_REGISTER 0x1000 > >> + > >> +#define GPC_PGC_nCTRL_PCR BIT(0) > >> + > >> +struct imx7_pgc_domain { > >> + struct generic_pm_domain genpd; > >> + struct regmap *regmap; > >> + struct regulator *regulator; > >> + > >> + unsigned int pgc_nctrl; > >> + > >> + const struct { > >> + u32 pxx; > >> + u32 map; > >> + } bits; > >> + > >> + const int voltage; > >> + struct device *dev; > >> +}; > >> + > >> +static int imx7_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain *genpd, > >> + bool on) > >> +{ > >> + struct imx7_pgc_domain *domain = container_of(genpd, > >> + struct imx7_pgc_domain, > >> + genpd); > >> + unsigned int offset = on ? > >> + GPC_PU_PGC_SW_PUP_REQ : GPC_PU_PGC_SW_PDN_REQ; > >> + const bool enable_power_control = domain->pgc_nctrl && !on; > >> + const bool has_regulator = !IS_ERR(domain->regulator); > >> + unsigned long deadline; > >> + int ret = 0; > >> + > >> + regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING, > >> + domain->bits.map, domain->bits.map); > >> + > >> + if (has_regulator && on) { > >> + ret = regulator_enable(domain->regulator); > >> + if (ret) { > >> + dev_err(domain->dev, "failed to enable regulator\n"); > >> + goto unmap; > >> + } > >> + } > >> + > >> + if (enable_power_control) > >> + regmap_update_bits(domain->regmap, domain->pgc_nctrl, > >> + GPC_PGC_nCTRL_PCR, GPC_PGC_nCTRL_PCR); > >> + > >> + regmap_update_bits(domain->regmap, offset, > >> + domain->bits.pxx, domain->bits.pxx); > >> + > >> + /* > >> + * As per "5.5.9.4 Example Code 4" in IMX7DRM.pdf wait > >> + * for PUP_REQ/PDN_REQ bit to be cleared > >> + */ > >> + deadline = jiffies + msecs_to_jiffies(1); > >> + while (true) { > >> + u32 pxx_req; > >> + > >> + regmap_read(domain->regmap, offset, &pxx_req); > >> + > >> + if (!(pxx_req & domain->bits.pxx)) > >> + break; > >> + > >> + if (time_after(jiffies, deadline)) { > >> + dev_err(domain->dev, "falied to command PGC\n"); > >> + ret = -ETIMEDOUT; > >> + /* > >> + * If we were in a process of enabling a > >> + * domain and failed we might as well disable > >> + * the regulator we just enabled. And if it > >> + * was the opposite situation and we failed to > >> + * power down -- keep the regulator on > >> + */ > >> + on = !on; > >> + break; > >> + } > >> + > >> + cpu_relax(); > >> + } > >> + > >> + if (enable_power_control) > >> + regmap_update_bits(domain->regmap, domain->pgc_nctrl, > >> + GPC_PGC_nCTRL_PCR, 0); > >> + > >> + if (has_regulator && !on) { > >> + int err; > >> + > >> + err = regulator_disable(domain->regulator); > >> + if (err) > >> + dev_err(domain->dev, > >> + "failed to disable regulator: %d\n", ret); > >> + /* Preserve earlier error code */ > >> + ret = ret ?: err; > >> + } > >> +unmap: > >> + regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING, > >> + domain->bits.map, 0); > >> + return ret; > >> +} > >> + > >> +static int imx7_gpc_pu_pgc_sw_pup_req(struct generic_pm_domain *genpd) > >> +{ > >> + return imx7_gpc_pu_pgc_sw_pxx_req(genpd, true); > >> +} > >> + > >> +static int imx7_gpc_pu_pgc_sw_pdn_req(struct generic_pm_domain *genpd) > >> +{ > >> + return imx7_gpc_pu_pgc_sw_pxx_req(genpd, false); > >> +} > >> + > >> +static struct imx7_pgc_domain imx7_pgc_domains[] = { > >> + [IMX7_POWER_DOMAIN_USB_HSIC_PHY] = { > >> + .genpd = { > >> + .name = "usb-hsic-phy", > >> + }, > >> + .bits = { > >> + .pxx = USB_HSIC_PHY_SW_Pxx_REQ, > >> + .map = USB_HSIC_PHY_A7_DOMAIN, > >> + }, > >> + .voltage = 1200000, > >> + .pgc_nctrl = 0x0d00, > >> + }, > >> + > >> + [IMX7_POWER_DOMAIN_USB_OTG2_PHY] = { > >> + .genpd = { > >> + .name = "usb-otg2-phy", > >> + }, > >> + .bits = { > >> + .pxx = USB_OTG2_PHY_SW_Pxx_REQ, > >> + .map = USB_OTG2_PHY_A7_DOMAIN, > >> + }, > >> + }, > >> + > >> + [IMX7_POWER_DOMAIN_USB_OTG1_PHY] = { > >> + .genpd = { > >> + .name = "usb-otg1-phy", > >> + }, > >> + .bits = { > >> + .pxx = USB_OTG1_PHY_SW_Pxx_REQ, > >> + .map = USB_OTG1_PHY_A7_DOMAIN, > >> + }, > >> + }, > >> + > >> + [IMX7_POWER_DOMAIN_PCIE_PHY] = { > >> + .genpd = { > >> + .name = "pcie-phy", > >> + }, > >> + .bits = { > >> + .pxx = PCIE_PHY_SW_Pxx_REQ, > >> + .map = PCIE_PHY_A7_DOMAIN, > >> + }, > >> + .voltage = 1000000, > >> + .pgc_nctrl = 0x0c40, > >> + }, > >> + > >> + [IMX7_POWER_DOMAIN_MIPI_PHY] = { > >> + .genpd = { > >> + .name = "mipi-phy", > >> + }, > >> + .bits = { > >> + .pxx = MIPI_PHY_SW_Pxx_REQ, > >> + .map = MIPI_PHY_A7_DOMAIN, > >> + }, > >> + .voltage = 1000000, > >> + .pgc_nctrl = 0x0c00, > >> + }, > >> +}; > >> + > >> +static int imx7_pgc_domain_probe(struct platform_device *pdev) > >> +{ > >> + struct imx7_pgc_domain *domain = pdev->dev.platform_data; > >> + int ret; > >> + > >> + domain->dev = &pdev->dev; > >> + > >> + ret = pm_genpd_init(&domain->genpd, NULL, true); > >> + if (ret) { > >> + dev_err(domain->dev, "Failed to init power domain\n"); > >> + return ret; > >> + } > >> + > >> + domain->regulator = devm_regulator_get_optional(domain->dev, "power"); > >> + if (IS_ERR(domain->regulator) && > >> + PTR_ERR(domain->regulator) != -ENODEV) { > >> + dev_err(domain->dev, "Failed to get domain's regulator\n"); > >> + return PTR_ERR(domain->regulator); > >> + } > >> + > >> + if (!IS_ERR(domain->regulator)) { > >> + if (!domain->voltage) { > >> + WARN(1, "No voltage configured for domain's regulator"); > >> + return -EINVAL; > >> + } > >> + > >> + regulator_set_voltage(domain->regulator, > >> + domain->voltage, domain->voltage); > >> + } > >> + > >> + ret = of_genpd_add_provider_simple(domain->dev->of_node, > >> + &domain->genpd); > >> + if (ret) { > >> + dev_err(domain->dev, "Failed to add genpd provider\n"); > >> + pm_genpd_remove(&domain->genpd); > >> + } > >> + > >> + return ret; > >> +} > >> + > >> +static int imx7_pgc_domain_remove(struct platform_device *pdev) > >> +{ > >> + struct imx7_pgc_domain *domain = pdev->dev.platform_data; > >> + > >> + of_genpd_del_provider(domain->dev->of_node); > >> + pm_genpd_remove(&domain->genpd); > >> + > >> + return 0; > >> +} > >> + > >> +static const struct platform_device_id imx7_pgc_domain_id[] = { > >> + { "imx7-pgc-domain", }, > >> + { }, > >> +}; > >> + > >> +static struct platform_driver imx7_pgc_domain_driver = { > >> + .driver = { > >> + .name = "imx7-pgc", > >> + }, > >> + .probe = imx7_pgc_domain_probe, > >> + .remove = imx7_pgc_domain_remove, > >> + .id_table = imx7_pgc_domain_id, > >> +}; > >> +builtin_platform_driver(imx7_pgc_domain_driver) > > > > Again, i have a fundamental question about this patch implementation > > that why we choose above way to register the power domain? > > > > I'm sorry that i did not know too much history. > > Would you guys please help share some information? > > > > Because AFAIK this way will register each domain as a power domain > > provider which is a bit violate the real HW and current power domain > > framework design. And it is a bit more complicated to use than before. > > > > IMHO i would rather prefer the old traditional and simpler way that one > > provider (GPC) supplies multiple domains (PCIE/MIPI/HSIC PHY domain) > > than this patch does. > > > > However, i might be wrong. Please help to clear. > > > >> + > >> +static int imx_gpcv2_probe(struct platform_device *pdev) > >> +{ > >> + static const struct regmap_config regmap_config = { > >> + .cache_type = REGCACHE_NONE, > > > > You could drop it as GPC does. > > > > Yep, will remove. > > >> + .reg_bits = 32, > >> + .val_bits = 32, > >> + .reg_stride = 4, > >> + .max_register = GPC_MAX_REGISTER, > > > > Do you need add some readable/writeable reg? > > > > There seems to be some reserved regs in your range. > > I can easily got the below crash: > > > > root@imx6ul7d:/sys/kernel/debug/regmap/303a0000.gpc# cat registers > > [ 54.985395] Unhandled fault: external abort on non-linefetch (0x1008) at 0xf08511c0 > > [ 54.993166] pgd = ed02c000 > > [ 54.995894] [f08511c0] *pgd=af00c811, *pte=303a0653, *ppte=303a0453 > > [ 55.002215] Internal error: : 1008 [#1] SMP ARM > > [ 55.006756] Modules linked in: > > [ 55.009830] CPU: 0 PID: 750 Comm: cat Not tainted 4.11.0-rc1-00057-g2d60158-dirty #1169 > > [ 55.017842] Hardware name: Freescale i.MX7 Dual (Device Tree) > > [ 55.023598] task: ee20ee40 task.stack: ed0f8000 > > [ 55.028146] PC is at regmap_mmio_read32le+0x14/0x24 > > [ 55.033035] LR is at regmap_mmio_read+0x40/0x60 > > [ 55.037577] pc : [<c054f160>] lr : [<c054f348>] psr: 20070093 > > [ 55.037577] sp : ed0f9dc8 ip : ed0f9dd8 fp : ed0f9dd4 > > [ 55.049064] r10: ed160000 r9 : ef341000 r8 : ed0f9f80 > > [ 55.054299] r7 : ed0f9e5c r6 : ed0f9e5c r5 : 000001c0 r4 : ef3bb900 > > [ 55.060836] r3 : f0851000 r2 : ed0f9e5c r1 : f08511c0 r0 : ef3bb900 > > [ 55.067374] Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment none > > [ 55.074606] Control: 10c5387d Table: ad02c06a DAC: 00000051 > > [ 55.080361] Process cat (pid: 750, stack limit = 0xed0f8210) > > .... > > Good catch, thank you! I'll update the code to make sure this doesn't happen. > > Thanks, > Andrey Smirnov ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v7 2/2] soc/imx: Add GPCv2 power gating driver 2017-03-30 6:58 ` Dong Aisheng @ 2017-03-30 7:04 ` Dong Aisheng 0 siblings, 0 replies; 16+ messages in thread From: Dong Aisheng @ 2017-03-30 7:04 UTC (permalink / raw) To: Andrey Smirnov Cc: Shawn Guo, Andrey Yurovsky, Lucas Stach, Fabio Estevam, linux-arm-kernel, linux-kernel On Thu, Mar 30, 2017 at 02:58:36PM +0800, Dong Aisheng wrote: > On Mon, Mar 27, 2017 at 11:42:15AM -0700, Andrey Smirnov wrote: > > On Thu, Mar 23, 2017 at 11:24 PM, Dong Aisheng <dongas86@gmail.com> wrote: > > > On Tue, Mar 21, 2017 at 07:50:04AM -0700, Andrey Smirnov wrote: > > >> Add code allowing for control of various power domains managed by GPCv2 > > >> IP block found in i.MX7 series of SoCs. Power domains covered by this > > >> patch are: > > >> > > >> - PCIE PHY > > >> - MIPI PHY > > >> - USB HSIC PHY > > >> - USB OTG1/2 PHY > > >> > > > > > > You probably may need drop USB OTG which is not claimed in current RM. > > > See the PGC definition in 5.5.10 GPC Memory Map section. > > > > > > Each PGC (CPU type, MIX type, PU type) will occupy 64 Bytes address space, > > > the specific base address of each PGC are listed as below. > > > • 0x800 ~ 0x83F : PGC for A7 core0 > > > • 0x840 ~ 0x87F: PGC for A7 core1 > > > • 0x880 ~ 0x8BF: PGC for A7 SCU > > > • 0xA00 ~ 0xA3F: PGC for fastmix/megamix > > > • 0xC00 ~ 0xC3F: PGC for MIPI PHY > > > • 0xC40 ~ 0xC7F: PGC for PCIE_PHY > > > • 0xC80 ~ 0xCBF: Reserved > > > • 0xCC0 ~ 0xCFF: Reserved > > > • 0xD00 ~ 0xD3F: PGC for USB HSIC PHY > > > > > > And in 5.4 Power Management Unit (PMU) chapter, > > > you will find the USB OTG phy power is directly supplied by > > > VDD_USB_OTG1_3P3_IN/VDD_USB_OTG2_3P3_IN. > > > > > > http://www.nxp.com/assets/documents/data/en/reference-manuals/IMX7DRM.pdf > > > > > > I understand that there's also some USB OTG code exist in NXP internal > > > tree, but that's legacy for early doc implementation and may be deprecated. > > > so i assume it should be gone. > > > > > > Hopefully i will double confirm with our IC designer tomorrow. > > > > > > > USB OTG domains are absent from that list, true, but they are > > mentioned all of the place further in that section in register map > > documentation, which makes it difficult to tell which part of the > > datasheet is not up to date. > > > > I'm going to drop those power domains for now, since I don't have a > > use-case for them and it would also allow me to get rid of the chunk > > of code you thought was messy. However it would be nice to get an > > updated version of RM where all of that is straightened out. > > > > I checked with our IC designer and he confirmed the USB OTG is removed > and not supported in GPC. SW should not control it, instead, its power > domain is handled by hardware automatically. > > Currently there's true some incorrectness in GPC chapter, i already > reported the issue to the designer, but still no timeline when i > can get a updated version. > > But i think it's fine if you're going to only support PCIE/MIPI/USB HSIC > PHY power domain. I suppose those bits are correct in RM. > BTW, would you please CC my company email next time? aisheng.dong@nxp.com Then i can see them in time to help the review. Regards Dong Aisheng ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2017-04-10 11:25 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-21 14:50 [PATCH v7 0/2] GPCv2 power gating driver Andrey Smirnov 2017-03-21 14:50 ` [PATCH v7 1/2] dt-bindings: Add " Andrey Smirnov 2017-03-24 6:32 ` Dong Aisheng 2017-03-27 18:42 ` Andrey Smirnov 2017-03-30 7:15 ` Dong Aisheng 2017-03-21 14:50 ` [PATCH v7 2/2] soc/imx: " Andrey Smirnov 2017-03-24 6:24 ` Dong Aisheng 2017-03-23 14:35 ` Lucas Stach 2017-03-30 7:51 ` Dong Aisheng 2017-03-29 16:08 ` Lucas Stach 2017-04-01 4:10 ` Dong Aisheng 2017-03-31 12:28 ` Lucas Stach 2017-04-11 3:22 ` Dong Aisheng 2017-03-27 18:42 ` Andrey Smirnov 2017-03-30 6:58 ` Dong Aisheng 2017-03-30 7:04 ` Dong Aisheng
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).