[PATCH v5 0/6] provide power off support for iMX6 with external PMIC

[PATCH v5 0/6] provide power off support for iMX6 with external PMIC

[Oleksij Rempel]

This patch series is providing power off support for Freescale/NXP iMX6 based
boards with external power management integrated circuit (PMIC).

changes:
v5:
 - remove useless includes from pm-imx6.c patch
 - add Acked-by to "regulator: pfuze100: add fsl,pmic-stby-poweroff property"
   patch
v4:
 - update comment in "regulator: pfuze100: add fsl,pmic-stby-poweroff ..."
   patch
 - add Acked-by to "ARM: imx6q: provide documentation for new ..."
   patch

v3:
 - set pm_power_off_prepare = NULL on .remove.
 - documentation and spelling fixes.
 - use %pf instead of lookup_symbol_name.

Oleksij Rempel (6):
  ARM: imx6q: provide documentation for new fsl,pmic-stby-poweroff
    property
  ARM: imx6: register pm_power_off handler if "fsl,pmic-stby-poweroff"
    is set
  kernel/reboot.c: export pm_power_off_prepare
  regulator: pfuze100: add fsl,pmic-stby-poweroff property
  regulator: pfuze100-regulator: provide pm_power_off_prepare handler
  ARM: dts: imx6: RIoTboard provide standby on power off option

 .../devicetree/bindings/clock/imx6q-clock.txt      |  8 ++
 .../devicetree/bindings/regulator/pfuze100.txt     |  7 ++
 arch/arm/boot/dts/imx6dl-riotboard.dts             |  5 ++
 arch/arm/mach-imx/pm-imx6.c                        | 25 ++++++
 drivers/regulator/pfuze100-regulator.c             | 92 ++++++++++++++++++++++
 kernel/reboot.c                                    |  1 +
 6 files changed, 138 insertions(+)

-- 
2.11.0

[PATCH v5 1/6] ARM: imx6q: provide documentation for new fsl,pmic-stby-poweroff property

[Oleksij Rempel]

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/clock/imx6q-clock.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/imx6q-clock.txt b/Documentation/devicetree/bindings/clock/imx6q-clock.txt
index 9252912a5b0e..8f93e1503e59 100644
--- a/Documentation/devicetree/bindings/clock/imx6q-clock.txt
+++ b/Documentation/devicetree/bindings/clock/imx6q-clock.txt
@@ -6,6 +6,14 @@ Required properties:
 - interrupts: Should contain CCM interrupt
 - #clock-cells: Should be <1>
 
+Optional properties:
+- fsl,pmic-stby-poweroff: Configure CCM to assert PMIC_STBY_REQ signal
+  on power off.
+  Use this property if the SoC should be powered off by external power
+  management IC (PMIC) triggered via PMIC_STBY_REQ signal.
+  Boards that are designed to initiate poweroff on PMIC_ON_REQ signal should
+  be using "syscon-poweroff" driver instead.
+
 The clock consumer should specify the desired clock by having the clock
 ID in its "clocks" phandle cell.  See include/dt-bindings/clock/imx6qdl-clock.h
 for the full list of i.MX6 Quad and DualLite clock IDs.
-- 
2.11.0

[PATCH v5 2/6] ARM: imx6: register pm_power_off handler if "fsl,pmic-stby-poweroff" is set

[Oleksij Rempel]

One of the Freescale recommended sequences for power off with external
PMIC is the following:
...
3.  SoC is programming PMIC for power off when standby is asserted.
4.  In CCM STOP mode, Standby is asserted, PMIC gates SoC supplies.

See:
http://www.nxp.com/assets/documents/data/en/reference-manuals/IMX6DQRM.pdf
page 5083

This patch implements step 4. of this sequence.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 arch/arm/mach-imx/pm-imx6.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/arch/arm/mach-imx/pm-imx6.c b/arch/arm/mach-imx/pm-imx6.c
index e61b1d1027e1..cb76832935f0 100644
--- a/arch/arm/mach-imx/pm-imx6.c
+++ b/arch/arm/mach-imx/pm-imx6.c
@@ -602,6 +602,28 @@ static void __init imx6_pm_common_init(const struct imx6_pm_socdata
 				   IMX6Q_GPR1_GINT);
 }
 
+static void imx6_pm_poweroff(void)
+{
+	imx6_set_lpm(STOP_POWER_OFF);
+	cpu_suspend(0, imx6q_suspend_finish);
+
+	mdelay(1000);
+
+	pr_emerg("Unable to poweroff system\n");
+}
+
+static int imx6_pm_poweroff_probe(void)
+{
+	if (pm_power_off) {
+		pr_warn("%s: pm_power_off already claimed  %p %pf!\n",
+			__func__, pm_power_off, pm_power_off);
+		return -EBUSY;
+	}
+
+	pm_power_off = imx6_pm_poweroff;
+	return 0;
+}
+
 void __init imx6_pm_ccm_init(const char *ccm_compat)
 {
 	struct device_node *np;
@@ -618,6 +640,9 @@ void __init imx6_pm_ccm_init(const char *ccm_compat)
 	val = readl_relaxed(ccm_base + CLPCR);
 	val &= ~BM_CLPCR_LPM;
 	writel_relaxed(val, ccm_base + CLPCR);
+
+	if (of_property_read_bool(np, "fsl,pmic-stby-poweroff"))
+		imx6_pm_poweroff_probe();
 }
 
 void __init imx6q_pm_init(void)
-- 
2.11.0

[PATCH v5 3/6] kernel/reboot.c: export pm_power_off_prepare

[Oleksij Rempel]

Export pm_power_off_prepare. It is needed to implement power off on
Freescale/NXP iMX6 based boards with external power management
integrated circuit (PMIC).

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 kernel/reboot.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/reboot.c b/kernel/reboot.c
index bd30a973fe94..a6903bf772c7 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -49,6 +49,7 @@ int reboot_force;
  */
 
 void (*pm_power_off_prepare)(void);
+EXPORT_SYMBOL(pm_power_off_prepare);
 
 /**
  *	emergency_restart - reboot the system
-- 
2.11.0

[PATCH v5 4/6] regulator: pfuze100: add fsl,pmic-stby-poweroff property

[Oleksij Rempel]

Document the new optional "fsl,pmic-stby-poweroff" property.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/regulator/pfuze100.txt | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/pfuze100.txt b/Documentation/devicetree/bindings/regulator/pfuze100.txt
index 444c47831a40..197f1a52e960 100644
--- a/Documentation/devicetree/bindings/regulator/pfuze100.txt
+++ b/Documentation/devicetree/bindings/regulator/pfuze100.txt
@@ -4,6 +4,13 @@ Required properties:
 - compatible: "fsl,pfuze100", "fsl,pfuze200", "fsl,pfuze3000"
 - reg: I2C slave address
 
+Optional properties:
+- fsl,pmic-stby-poweroff: if present, configure the PMIC to shutdown all
+  power rails when PMIC_STBY_REQ line is asserted during the power off sequence.
+  Use this option if the SoC should be powered off by external power
+  management IC (PMIC) on PMIC_STBY_REQ signal.
+  As opposite to PMIC_STBY_REQ boards can implement PMIC_ON_REQ signal.
+
 Required child node:
 - regulators: This is the list of child nodes that specify the regulator
   initialization data for defined regulators. Please refer to below doc
-- 
2.11.0

[PATCH v5 5/6] regulator: pfuze100-regulator: provide pm_power_off_prepare handler

[Oleksij Rempel]

On some boards the SoC can use one pin "PMIC_STBY_REQ" to notify th PMIC
about state changes. In this case internal state of PMIC must be
preconfigured for upcomming state change.
It works fine with the current regulator framework, except with the
power-off case.

This patch is providing an optional pm_power_off_prepare handler
which will configure standby state of the PMIC to disable all power lines.

In my power consumption test on RIoTBoard, I got the following results:
power off without this patch:	320 mA
power off with this patch:	2   mA
suspend to ram:			40  mA

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/regulator/pfuze100-regulator.c | 92 ++++++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)

diff --git a/drivers/regulator/pfuze100-regulator.c b/drivers/regulator/pfuze100-regulator.c
index 63922a2167e5..f6c276ed91d8 100644
--- a/drivers/regulator/pfuze100-regulator.c
+++ b/drivers/regulator/pfuze100-regulator.c
@@ -28,6 +28,7 @@
 #include <linux/regulator/pfuze100.h>
 #include <linux/i2c.h>
 #include <linux/slab.h>
+#include <linux/kallsyms.h>
 #include <linux/regmap.h>
 
 #define PFUZE_NUMREGS		128
@@ -42,11 +43,17 @@
 
 #define PFUZE100_COINVOL	0x1a
 #define PFUZE100_SW1ABVOL	0x20
+#define PFUZE100_SW1ABMODE	0x23
 #define PFUZE100_SW1CVOL	0x2e
+#define PFUZE100_SW1CMODE	0x31
 #define PFUZE100_SW2VOL		0x35
+#define PFUZE100_SW2MODE	0x38
 #define PFUZE100_SW3AVOL	0x3c
+#define PFUZE100_SW3AMODE	0x3f
 #define PFUZE100_SW3BVOL	0x43
+#define PFUZE100_SW3BMODE	0x46
 #define PFUZE100_SW4VOL		0x4a
+#define PFUZE100_SW4MODE	0x4d
 #define PFUZE100_SWBSTCON1	0x66
 #define PFUZE100_VREFDDRCON	0x6a
 #define PFUZE100_VSNVSVOL	0x6b
@@ -57,6 +64,13 @@
 #define PFUZE100_VGEN5VOL	0x70
 #define PFUZE100_VGEN6VOL	0x71
 
+#define PFUZE100_SWxMODE_MASK	0xf
+#define PFUZE100_SWxMODE_APS_APS	0x8
+#define PFUZE100_SWxMODE_APS_OFF	0x4
+
+#define PFUZE100_VGENxLPWR	BIT(6)
+#define PFUZE100_VGENxSTBY	BIT(5)
+
 enum chips { PFUZE100, PFUZE200, PFUZE3000 = 3 };
 
 struct pfuze_regulator {
@@ -489,6 +503,69 @@ static inline struct device_node *match_of_node(int index)
 }
 #endif
 
+static struct pfuze_chip *syspm_pfuze_chip;
+
+static void pfuze_power_off_prepare(void)
+{
+	dev_info(syspm_pfuze_chip->dev, "Configure standy mode for power off");
+
+	/* Switch from default mode: APS/APS to APS/Off */
+	regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_SW1ABMODE,
+			   PFUZE100_SWxMODE_MASK, PFUZE100_SWxMODE_APS_OFF);
+	regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_SW1CMODE,
+			   PFUZE100_SWxMODE_MASK, PFUZE100_SWxMODE_APS_OFF);
+	regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_SW2MODE,
+			   PFUZE100_SWxMODE_MASK, PFUZE100_SWxMODE_APS_OFF);
+	regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_SW3AMODE,
+			   PFUZE100_SWxMODE_MASK, PFUZE100_SWxMODE_APS_OFF);
+	regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_SW3BMODE,
+			   PFUZE100_SWxMODE_MASK, PFUZE100_SWxMODE_APS_OFF);
+	regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_SW4MODE,
+			   PFUZE100_SWxMODE_MASK, PFUZE100_SWxMODE_APS_OFF);
+
+	regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_VGEN1VOL,
+			   PFUZE100_VGENxLPWR | PFUZE100_VGENxSTBY,
+			   PFUZE100_VGENxSTBY);
+	regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_VGEN2VOL,
+			   PFUZE100_VGENxLPWR | PFUZE100_VGENxSTBY,
+			   PFUZE100_VGENxSTBY);
+	regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_VGEN3VOL,
+			   PFUZE100_VGENxLPWR | PFUZE100_VGENxSTBY,
+			   PFUZE100_VGENxSTBY);
+	regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_VGEN4VOL,
+			   PFUZE100_VGENxLPWR | PFUZE100_VGENxSTBY,
+			   PFUZE100_VGENxSTBY);
+	regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_VGEN5VOL,
+			   PFUZE100_VGENxLPWR | PFUZE100_VGENxSTBY,
+			   PFUZE100_VGENxSTBY);
+	regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_VGEN6VOL,
+			   PFUZE100_VGENxLPWR | PFUZE100_VGENxSTBY,
+			   PFUZE100_VGENxSTBY);
+}
+
+static int pfuze_power_off_prepare_init(struct pfuze_chip *pfuze_chip)
+{
+	if (pfuze_chip->chip_id != PFUZE100) {
+		dev_warn(pfuze_chip->dev, "Requested pm_power_off_prepare handler for not supported chip\n");
+		return -ENODEV;
+	}
+
+	if (pm_power_off_prepare) {
+		dev_warn(pfuze_chip->dev, "pm_power_off_prepare is already registered.\n");
+		return -EBUSY;
+	}
+
+	if (syspm_pfuze_chip) {
+		dev_warn(pfuze_chip->dev, "syspm_pfuze_chip is already set.\n");
+		return -EBUSY;
+	}
+
+	syspm_pfuze_chip = pfuze_chip;
+	pm_power_off_prepare = pfuze_power_off_prepare;
+
+	return 0;
+}
+
 static int pfuze_identify(struct pfuze_chip *pfuze_chip)
 {
 	unsigned int value;
@@ -659,6 +736,20 @@ static int pfuze100_regulator_probe(struct i2c_client *client,
 		}
 	}
 
+	if (of_property_read_bool(client->dev.of_node,
+				  "fsl,pmic-stby-poweroff"))
+		return pfuze_power_off_prepare_init(pfuze_chip);
+
+	return 0;
+}
+
+static int pfuze100_regulator_remove(struct i2c_client *client)
+{
+	if (syspm_pfuze_chip) {
+		syspm_pfuze_chip = NULL;
+		pm_power_off_prepare = NULL;
+	}
+
 	return 0;
 }
 
@@ -669,6 +760,7 @@ static struct i2c_driver pfuze_driver = {
 		.of_match_table = pfuze_dt_ids,
 	},
 	.probe = pfuze100_regulator_probe,
+	.remove = pfuze100_regulator_remove,
 };
 module_i2c_driver(pfuze_driver);
 
-- 
2.11.0

[PATCH v5 6/6] ARM: dts: imx6: RIoTboard provide standby on power off option

[Oleksij Rempel]

This board, as well as some other boards with i.MX6 and a PMIC, uses a
"MPIC_STBY_REQ" line to notify the PMIC about a state change.
The PMIC is programmed for a specific state change before triggering the
line.
In this case, PMIC_STBY_REQ can be used for stand by, sleep
and power off modes.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 arch/arm/boot/dts/imx6dl-riotboard.dts | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/imx6dl-riotboard.dts b/arch/arm/boot/dts/imx6dl-riotboard.dts
index 2cb72824e800..da9d0c102a31 100644
--- a/arch/arm/boot/dts/imx6dl-riotboard.dts
+++ b/arch/arm/boot/dts/imx6dl-riotboard.dts
@@ -90,6 +90,10 @@
 	status = "okay";
 };
 
+&clks {
+	fsl,pmic-stby-poweroff;
+};
+
 &fec {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_enet>;
@@ -125,6 +129,7 @@
 		reg = <0x08>;
 		interrupt-parent = <&gpio5>;
 		interrupts = <16 8>;
+		fsl,pmic-stby-poweroff;
 
 		regulators {
 			reg_vddcore: sw1ab {				/* VDDARM_IN */
-- 
2.11.0

Re: [PATCH v5 3/6] kernel/reboot.c: export pm_power_off_prepare

[Leonard Crestez]

On Mon, 2017-06-19 at 07:02 +0200, Oleksij Rempel wrote:
> Export pm_power_off_prepare. It is needed to implement power off on
> Freescale/NXP iMX6 based boards with external power management
> integrated circuit (PMIC).
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  kernel/reboot.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/reboot.c b/kernel/reboot.c
> index bd30a973fe94..a6903bf772c7 100644
> --- a/kernel/reboot.c
> +++ b/kernel/reboot.c
> @@ -49,6 +49,7 @@ int reboot_force;
>   */
>  
>  void (*pm_power_off_prepare)(void);
> +EXPORT_SYMBOL(pm_power_off_prepare);
>  
Could you register a reboot notifier in the pfuze driver instead? Right
now the only user of pm_power_off_prepare is ACPI so this alternative
seems less intrusive.

--
Regards,
Leonard

Re: [PATCH v5 2/6] ARM: imx6: register pm_power_off handler if "fsl,pmic-stby-poweroff" is set

[Leonard Crestez]

On Mon, 2017-06-19 at 07:02 +0200, Oleksij Rempel wrote:
> One of the Freescale recommended sequences for power off with
> external
> PMIC is the following:
> ...
> 3.  SoC is programming PMIC for power off when standby is asserted.
> 4.  In CCM STOP mode, Standby is asserted, PMIC gates SoC supplies.
> 
> See:
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww
> .nxp.com%2Fassets%2Fdocuments%2Fdata%2Fen%2Freference-
> manuals%2FIMX6DQRM.pdf&data=01%7C01%7Cleonard.crestez%40nxp.com%7C32c
> df19d0ab44ed8e24d08d4b6d097f4%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0&
> sdata=CWDpnEGikPGmSTM3dQH8IMxQCVzcIvHL69nu962d8M0%3D&reserved=0
> page 5083
> 
> This patch implements step 4. of this sequence.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  arch/arm/mach-imx/pm-imx6.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/arch/arm/mach-imx/pm-imx6.c b/arch/arm/mach-imx/pm-
> imx6.c
> index e61b1d1027e1..cb76832935f0 100644
> --- a/arch/arm/mach-imx/pm-imx6.c
> +++ b/arch/arm/mach-imx/pm-imx6.c
> @@ -602,6 +602,28 @@ static void __init imx6_pm_common_init(const
> struct imx6_pm_socdata
>  				   IMX6Q_GPR1_GINT);
>  }
>  
> +static void imx6_pm_poweroff(void)
> +{
> +	imx6_set_lpm(STOP_POWER_OFF);
> +	cpu_suspend(0, imx6q_suspend_finish);

It's a bit strange that you're using parts of the suspend code for
this. Perhaps you could just write your bits to CLPCR and then execute
a WFI?

It would also be nice to rename this to something like
imx6_pm_stby_poweroff to make it clear that it's only for a certain
power off path.

Re: [PATCH v5 6/6] ARM: dts: imx6: RIoTboard provide standby on power off option

[Stephen Boyd]

On 06/19, Oleksij Rempel wrote:
> This board, as well as some other boards with i.MX6 and a PMIC, uses a
> "MPIC_STBY_REQ" line to notify the PMIC about a state change.

PMIC_STBY_REQ?

> The PMIC is programmed for a specific state change before triggering the
> line.
> In this case, PMIC_STBY_REQ can be used for stand by, sleep

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v5 6/6] ARM: dts: imx6: RIoTboard provide standby on power off option

[Oleksij Rempel]

On 20.06.2017 02:19, Stephen Boyd wrote:
> On 06/19, Oleksij Rempel wrote:
>> This board, as well as some other boards with i.MX6 and a PMIC, uses a
>> "MPIC_STBY_REQ" line to notify the PMIC about a state change.
>
> PMIC_STBY_REQ?

Ok, thanks.

Re: [PATCH v5 2/6] ARM: imx6: register pm_power_off handler if "fsl,pmic-stby-poweroff" is set

[Oleksij Rempel]

Hi Leonard,

On 19.06.2017 13:35, Leonard Crestez wrote:
> On Mon, 2017-06-19 at 07:02 +0200, Oleksij Rempel wrote:
>> One of the Freescale recommended sequences for power off with
>> external
>> PMIC is the following:
>> ...
>> 3.  SoC is programming PMIC for power off when standby is asserted.
>> 4.  In CCM STOP mode, Standby is asserted, PMIC gates SoC supplies.
>>
>> See:
>> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww
>> .nxp.com%2Fassets%2Fdocuments%2Fdata%2Fen%2Freference-
>> manuals%2FIMX6DQRM.pdf&data=01%7C01%7Cleonard.crestez%40nxp.com%7C32c
>> df19d0ab44ed8e24d08d4b6d097f4%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0&
>> sdata=CWDpnEGikPGmSTM3dQH8IMxQCVzcIvHL69nu962d8M0%3D&reserved=0
>> page 5083
>>
>> This patch implements step 4. of this sequence.
>>
>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>> ---
>>  arch/arm/mach-imx/pm-imx6.c | 25 +++++++++++++++++++++++++
>>  1 file changed, 25 insertions(+)
>>
>> diff --git a/arch/arm/mach-imx/pm-imx6.c b/arch/arm/mach-imx/pm-
>> imx6.c
>> index e61b1d1027e1..cb76832935f0 100644
>> --- a/arch/arm/mach-imx/pm-imx6.c
>> +++ b/arch/arm/mach-imx/pm-imx6.c
>> @@ -602,6 +602,28 @@ static void __init imx6_pm_common_init(const
>> struct imx6_pm_socdata
>>  				   IMX6Q_GPR1_GINT);
>>  }
>>
>> +static void imx6_pm_poweroff(void)
>> +{
>> +	imx6_set_lpm(STOP_POWER_OFF);
>> +	cpu_suspend(0, imx6q_suspend_finish);
>
> It's a bit strange that you're using parts of the suspend code for
> this. Perhaps you could just write your bits to CLPCR and then execute
> a WFI?

Well, every thing in this patch set, yell about using standby/suspend 
path in HW for power off. Even iMX code use STOP_POWER_OFF for 
configuring this regs. How can it be more strange? :) Should i really 
duplicate code to hide this?

> It would also be nice to rename this to something like
> imx6_pm_stby_poweroff to make it clear that it's only for a certain
> power off path.

ok.

Re: [PATCH v5 3/6] kernel/reboot.c: export pm_power_off_prepare

[Oleksij Rempel]

On 19.06.2017 13:35, Leonard Crestez wrote:
> On Mon, 2017-06-19 at 07:02 +0200, Oleksij Rempel wrote:
>> Export pm_power_off_prepare. It is needed to implement power off on
>> Freescale/NXP iMX6 based boards with external power management
>> integrated circuit (PMIC).
>>
>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>> ---
>>  kernel/reboot.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/kernel/reboot.c b/kernel/reboot.c
>> index bd30a973fe94..a6903bf772c7 100644
>> --- a/kernel/reboot.c
>> +++ b/kernel/reboot.c
>> @@ -49,6 +49,7 @@ int reboot_force;
>>   */
>>
>>  void (*pm_power_off_prepare)(void);
>> +EXPORT_SYMBOL(pm_power_off_prepare);
>>
> Could you register a reboot notifier in the pfuze driver instead? Right
> now the only user of pm_power_off_prepare is ACPI so this alternative
> seems less intrusive.

hm... in this case i will need to make sure that reboot handler is not 
executed on reboot. This will make code looks strange. Which is opposite 
of your comment in other email about strange standby code for power off :)
Should i really do this?

Re: [PATCH v5 2/6] ARM: imx6: register pm_power_off handler if "fsl,pmic-stby-poweroff" is set

[Leonard Crestez]

On Tue, 2017-06-20 at 06:55 +0200, Oleksij Rempel wrote:
> On 19.06.2017 13:35, Leonard Crestez wrote:
> > On Mon, 2017-06-19 at 07:02 +0200, Oleksij Rempel wrote:
> > > 
> > > One of the Freescale recommended sequences for power off with
> > > external
> > > PMIC is the following:
> > > ...
> > > 3.  SoC is programming PMIC for power off when standby is asserted.
> > > 4.  In CCM STOP mode, Standby is asserted, PMIC gates SoC supplies.
> > > 
> > > This patch implements step 4. of this sequence.
> > > 
> > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > > ---
> > >  arch/arm/mach-imx/pm-imx6.c | 25 +++++++++++++++++++++++++
> > >  1 file changed, 25 insertions(+)
> > > 
> > > diff --git a/arch/arm/mach-imx/pm-imx6.c b/arch/arm/mach-imx/pm-
> > > imx6.c
> > > index e61b1d1027e1..cb76832935f0 100644
> > > --- a/arch/arm/mach-imx/pm-imx6.c
> > > +++ b/arch/arm/mach-imx/pm-imx6.c
> > > @@ -602,6 +602,28 @@ static void __init imx6_pm_common_init(const
> > > struct imx6_pm_socdata
> > >  				   IMX6Q_GPR1_GINT);
> > >  }
> > > 
> > > +static void imx6_pm_poweroff(void)
> > > +{
> > > +	imx6_set_lpm(STOP_POWER_OFF);
> > > +	cpu_suspend(0, imx6q_suspend_finish);
> > It's a bit strange that you're using parts of the suspend code for
> > this. Perhaps you could just write your bits to CLPCR and then execute
> > a WFI?
> Well, every thing in this patch set, yell about using standby/suspend 
> path in HW for power off. Even iMX code use STOP_POWER_OFF for 
> configuring this regs. How can it be more strange? :) Should i really 
> duplicate code to hide this?
> 
This code is used to set the power state the soc enters on WFI. These
function try to ensure that it is possible to come back up when woken
by an interrupt later. That part you don't want, right?

However I can't think of any obvious way this could misbehave so I
guess it's fine.

Re: [PATCH v5 3/6] kernel/reboot.c: export pm_power_off_prepare

[Leonard Crestez]

On Tue, 2017-06-20 at 07:01 +0200, Oleksij Rempel wrote:
> 
> On 19.06.2017 13:35, Leonard Crestez wrote:
> > 
> > On Mon, 2017-06-19 at 07:02 +0200, Oleksij Rempel wrote:
> > > 
> > > Export pm_power_off_prepare. It is needed to implement power off on
> > > Freescale/NXP iMX6 based boards with external power management
> > > integrated circuit (PMIC).
> > > 
> > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > > ---
> > >  kernel/reboot.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/kernel/reboot.c b/kernel/reboot.c
> > > index bd30a973fe94..a6903bf772c7 100644
> > > --- a/kernel/reboot.c
> > > +++ b/kernel/reboot.c
> > > @@ -49,6 +49,7 @@ int reboot_force;
> > >   */
> > > 
> > >  void (*pm_power_off_prepare)(void);
> > > +EXPORT_SYMBOL(pm_power_off_prepare);
> > > 
> > Could you register a reboot notifier in the pfuze driver instead? Right
> > now the only user of pm_power_off_prepare is ACPI so this alternative
> > seems less intrusive.
> hm... in this case i will need to make sure that reboot handler is not 
> executed on reboot. This will make code looks strange. Which is opposite 
> of your comment in other email about strange standby code for power off :)
> Should i really do this?

A reboot handler receives a parameter to differentiate between reboot
and shutdown so it would be easy. Isn't it preferable to use an
existing mechanism instead of exporting a new symbol?

But maybe somebody else will Ack this, I don't particularly insist on
changing this.

--
Regards,
Leonard

Re: [PATCH v5 2/6] ARM: imx6: register pm_power_off handler if "fsl,pmic-stby-poweroff" is set

[Oleksij Rempel]

On Tue, Jun 20, 2017 at 05:32:16PM +0300, Leonard Crestez wrote:
> On Tue, 2017-06-20 at 06:55 +0200, Oleksij Rempel wrote:
> > On 19.06.2017 13:35, Leonard Crestez wrote:
> > > On Mon, 2017-06-19 at 07:02 +0200, Oleksij Rempel wrote:
> > > > 
> > > > One of the Freescale recommended sequences for power off with
> > > > external
> > > > PMIC is the following:
> > > > ...
> > > > 3.  SoC is programming PMIC for power off when standby is asserted.
> > > > 4.  In CCM STOP mode, Standby is asserted, PMIC gates SoC supplies.
> > > > 
> > > > This patch implements step 4. of this sequence.
> > > > 
> > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > > > ---
> > > >  arch/arm/mach-imx/pm-imx6.c | 25 +++++++++++++++++++++++++
> > > >  1 file changed, 25 insertions(+)
> > > > 
> > > > diff --git a/arch/arm/mach-imx/pm-imx6.c b/arch/arm/mach-imx/pm-
> > > > imx6.c
> > > > index e61b1d1027e1..cb76832935f0 100644
> > > > --- a/arch/arm/mach-imx/pm-imx6.c
> > > > +++ b/arch/arm/mach-imx/pm-imx6.c
> > > > @@ -602,6 +602,28 @@ static void __init imx6_pm_common_init(const
> > > > struct imx6_pm_socdata
> > > >  				   IMX6Q_GPR1_GINT);
> > > >  }
> > > > 
> > > > +static void imx6_pm_poweroff(void)
> > > > +{
> > > > +	imx6_set_lpm(STOP_POWER_OFF);
> > > > +	cpu_suspend(0, imx6q_suspend_finish);
> > > It's a bit strange that you're using parts of the suspend code for
> > > this. Perhaps you could just write your bits to CLPCR and then execute
> > > a WFI?
> > Well, every thing in this patch set, yell about using standby/suspend 
> > path in HW for power off. Even iMX code use STOP_POWER_OFF for 
> > configuring this regs. How can it be more strange? :) Should i really 
> > duplicate code to hide this?
> > 
> This code is used to set the power state the soc enters on WFI. These
> function try to ensure that it is possible to come back up when woken
> by an interrupt later. That part you don't want, right?
> 
> However I can't think of any obvious way this could misbehave so I
> guess it's fine.

ok, thx.

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v5 3/6] kernel/reboot.c: export pm_power_off_prepare

[Oleksij Rempel]

On Tue, Jun 20, 2017 at 05:37:06PM +0300, Leonard Crestez wrote:
> On Tue, 2017-06-20 at 07:01 +0200, Oleksij Rempel wrote:
> > 
> > On 19.06.2017 13:35, Leonard Crestez wrote:
> > > 
> > > On Mon, 2017-06-19 at 07:02 +0200, Oleksij Rempel wrote:
> > > > 
> > > > Export pm_power_off_prepare. It is needed to implement power off on
> > > > Freescale/NXP iMX6 based boards with external power management
> > > > integrated circuit (PMIC).
> > > > 
> > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > > > ---
> > > >  kernel/reboot.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/kernel/reboot.c b/kernel/reboot.c
> > > > index bd30a973fe94..a6903bf772c7 100644
> > > > --- a/kernel/reboot.c
> > > > +++ b/kernel/reboot.c
> > > > @@ -49,6 +49,7 @@ int reboot_force;
> > > >   */
> > > > 
> > > >  void (*pm_power_off_prepare)(void);
> > > > +EXPORT_SYMBOL(pm_power_off_prepare);
> > > > 
> > > Could you register a reboot notifier in the pfuze driver instead? Right
> > > now the only user of pm_power_off_prepare is ACPI so this alternative
> > > seems less intrusive.
> > hm... in this case i will need to make sure that reboot handler is not 
> > executed on reboot. This will make code looks strange. Which is opposite 
> > of your comment in other email about strange standby code for power off :)
> > Should i really do this?
> 
> A reboot handler receives a parameter to differentiate between reboot
> and shutdown so it would be easy. Isn't it preferable to use an
> existing mechanism instead of exporting a new symbol?
> 
> But maybe somebody else will Ack this, I don't particularly insist on
> changing this.

I just need to make sure, nothing else will do some thing unexpected
with PMIC. Don't forget we use same PMIC_STBY_REQ signal for stanby,
suspend to RAM and for power off. Entering wrong state in wrong moment
will be fatal. This is why I use last possible step before pm_power_off.

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |