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

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

[Oleksij Rempel]

2018.07.26:
v8 is a rebase against kernel v4.18-rc6. No other changes are made.
Added: linux-imx@nxp.com and yibin.gong@nxp.com to the CC.

2018.05.17:
update patches to version v7

This patch series is providing power off support for Freescale/NXP iMX6 based
boards with external power management integrated circuit (PMIC).
As a first step the PMIC is configured to turn off the system if the
standby pin is asserted. On second step we assert the standby pin.
For this reason we need to use pm_power_off_prepare.

Usage of stnadby pin for power off is described in official iMX6
documentation.

2018.03.05:
As this patch set touches multiple subsystems I think it would make
sense for Shawn Guo to take the all patch set.
The only part which didn't receive an ACK is regulator stuff. So I would
hope that Mark Brown can ACK it.

Kind regards,
Oleksij Rempel

2017.12.06:
Adding Linus. Probably there is no maintainer for this patch set.
No changes are made, tested on v4.15-rc1.

2017.10.27:
Last version of this patch set was send at 20 Jun 2017, this is a rebase against
kernel v4.14-rc6. Probably this set got lost. If I forgot to address some comments,
please point me.

changes:
v7:
 - use EXPORT_SYMBOL_GPL(pm_power_off_prepare) instead of EXPORT_SYMBOL
 - call imx6q_suspend_finish() directly without cpu_suspend()

v6:
 - rename imx6_pm_poweroff to imx6_pm_stby_poweroff
 - fix "MPIC_STBY_REQ" typo in the comment.

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 ++
 .../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.18.0

[PATCH v8 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 a45ca67a9d5f..e1308346e00d 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.18.0

[PATCH v8 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 017539dd712b..2f5c643f62fb 100644
--- a/arch/arm/mach-imx/pm-imx6.c
+++ b/arch/arm/mach-imx/pm-imx6.c
@@ -601,6 +601,28 @@ static void __init imx6_pm_common_init(const struct imx6_pm_socdata
 				   IMX6Q_GPR1_GINT);
 }
 
+static void imx6_pm_stby_poweroff(void)
+{
+	imx6_set_lpm(STOP_POWER_OFF);
+	imx6q_suspend_finish(0);
+
+	mdelay(1000);
+
+	pr_emerg("Unable to poweroff system\n");
+}
+
+static int imx6_pm_stby_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_stby_poweroff;
+	return 0;
+}
+
 void __init imx6_pm_ccm_init(const char *ccm_compat)
 {
 	struct device_node *np;
@@ -617,6 +639,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_stby_poweroff_probe();
 }
 
 void __init imx6q_pm_init(void)
-- 
2.18.0

[PATCH v8 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 e4ced883d8de..83810d726f3e 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -49,6 +49,7 @@ int reboot_force;
  */
 
 void (*pm_power_off_prepare)(void);
+EXPORT_SYMBOL_GPL(pm_power_off_prepare);
 
 /**
  *	emergency_restart - reboot the system
-- 
2.18.0

[PATCH v8 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 f0ada3b14d70..f3b922f5a11f 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.18.0

[PATCH v8 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 8d9dbcc775ea..e386e9acb3f7 100644
--- a/drivers/regulator/pfuze100-regulator.c
+++ b/drivers/regulator/pfuze100-regulator.c
@@ -15,6 +15,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
@@ -29,11 +30,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
@@ -44,6 +51,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 {
@@ -492,6 +506,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;
@@ -661,6 +738,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;
 }
 
@@ -671,6 +762,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.18.0

[PATCH v8 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
"PMIC_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 2e98c92adff7..a0e9753ee767 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>;
@@ -170,6 +174,7 @@
 		reg = <0x08>;
 		interrupt-parent = <&gpio5>;
 		interrupts = <16 8>;
+		fsl,pmic-stby-poweroff;
 
 		regulators {
 			reg_vddcore: sw1ab {				/* VDDARM_IN */
-- 
2.18.0

Re: [PATCH v8 0/6] provide power off support for iMX6 with external PMIC

[Stefan Wahren]

Hi Oleksij,

Am 26.07.2018 um 11:22 schrieb Oleksij Rempel:
> 2018.07.26:
> v8 is a rebase against kernel v4.18-rc6. No other changes are made.
> Added: linux-imx@nxp.com and yibin.gong@nxp.com to the CC.

your patches won't apply to regulator-next or linux-next because 
"regulator: pfuze100: add pfuze3001 support" has been merged before.

Since the pfuze3001 doesn't support standby, it would be nice to make 
the driver take care of this.

Best regards
Stefan

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

[Robin Gong]

> -----Original Message-----
> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> Sent: 2018年7月26日 17:22
> To: Shawn Guo <shawnguo@kernel.org>; Mark Brown <broonie@kernel.org>;
> Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; kernel@pengutronix.de;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; Andrew Morton
> <akpm@linux-foundation.org>; Liam Girdwood <lgirdwood@gmail.com>;
> Leonard Crestez <leonard.crestez@nxp.com>; Rob Herring
> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Michael
> Turquette <mturquette@baylibre.com>; Stephen Boyd
> <sboyd@codeaurora.org>; Fabio Estevam <fabio.estevam@nxp.com>; Russell
> King <linux@armlinux.org.uk>; dl-linux-imx <linux-imx@nxp.com>; Robin Gong
> <yibin.gong@nxp.com>; A.s. Dong <aisheng.dong@nxp.com>
> Subject: [PATCH v8 1/6] ARM: imx6q: provide documentation for new
> fsl,pmic-stby-poweroff property
> 
> 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 a45ca67a9d5f..e1308346e00d 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.
PMIC_ON_REQ didn't connect to any pin of PMIC in your case? Don't understand
why not follow normal board design guide to power off pmic by PMIC_ON_REQ.
How to power on board again then?
> +  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.18.0

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

[Oleksij Rempel]

[-- Attachment #1.1: Type: text/plain, Size: 2723 bytes --]

Hi,

On 26.07.2018 11:51, Robin Gong wrote:
> 
> 
>> -----Original Message-----
>> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
>> Sent: 2018年7月26日 17:22
>> To: Shawn Guo <shawnguo@kernel.org>; Mark Brown <broonie@kernel.org>;
>> Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; kernel@pengutronix.de;
>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>> linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; Andrew Morton
>> <akpm@linux-foundation.org>; Liam Girdwood <lgirdwood@gmail.com>;
>> Leonard Crestez <leonard.crestez@nxp.com>; Rob Herring
>> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Michael
>> Turquette <mturquette@baylibre.com>; Stephen Boyd
>> <sboyd@codeaurora.org>; Fabio Estevam <fabio.estevam@nxp.com>; Russell
>> King <linux@armlinux.org.uk>; dl-linux-imx <linux-imx@nxp.com>; Robin Gong
>> <yibin.gong@nxp.com>; A.s. Dong <aisheng.dong@nxp.com>
>> Subject: [PATCH v8 1/6] ARM: imx6q: provide documentation for new
>> fsl,pmic-stby-poweroff property
>>
>> 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 a45ca67a9d5f..e1308346e00d 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.
> PMIC_ON_REQ didn't connect to any pin of PMIC in your case?

No. First, it was only one customer specific issue. After some research
I found even publicly available boards (for example RioTboard) which has
same/similar design. After seeing this in imx6 documentation as valid
power off way, I have no doubts - there should be even more devices doin
this in the wild.

> Don't understand
> why not follow normal board design guide to power off pmic by PMIC_ON_REQ.
> How to power on board again then?

Power cycle. Without this patch, power of is not real power off. So,
power cycle, is expected behavior for user interaction. On usual PC,
reset button will not enable PC as well.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Robin Gong]

> -----Original Message-----
> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> Sent: 2018年7月26日 19:38
> To: Robin Gong <yibin.gong@nxp.com>; Shawn Guo <shawnguo@kernel.org>;
> Mark Brown <broonie@kernel.org>; Rafael J. Wysocki
> <rafael.j.wysocki@intel.com>
> Cc: kernel@pengutronix.de; devicetree@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-clk@vger.kernel.org;
> linux-kernel@vger.kernel.org; Andrew Morton <akpm@linux-foundation.org>;
> Liam Girdwood <lgirdwood@gmail.com>; Leonard Crestez
> <leonard.crestez@nxp.com>; Rob Herring <robh+dt@kernel.org>; Mark
> Rutland <mark.rutland@arm.com>; Michael Turquette
> <mturquette@baylibre.com>; Stephen Boyd <sboyd@codeaurora.org>; Fabio
> Estevam <fabio.estevam@nxp.com>; Russell King <linux@armlinux.org.uk>;
> dl-linux-imx <linux-imx@nxp.com>; A.s. Dong <aisheng.dong@nxp.com>
> Subject: Re: [PATCH v8 1/6] ARM: imx6q: provide documentation for new
> fsl,pmic-stby-poweroff property
> 
> Hi,
> 
> On 26.07.2018 11:51, Robin Gong wrote:
> >
> >
> >> -----Original Message-----
> >> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> >> Sent: 2018年7月26日 17:22
> >> To: Shawn Guo <shawnguo@kernel.org>; Mark Brown
> <broonie@kernel.org>;
> >> Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; kernel@pengutronix.de;
> >> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> >> linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; Andrew
> >> Morton <akpm@linux-foundation.org>; Liam Girdwood
> >> <lgirdwood@gmail.com>; Leonard Crestez <leonard.crestez@nxp.com>;
> Rob
> >> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>;
> >> Michael Turquette <mturquette@baylibre.com>; Stephen Boyd
> >> <sboyd@codeaurora.org>; Fabio Estevam <fabio.estevam@nxp.com>;
> >> Russell King <linux@armlinux.org.uk>; dl-linux-imx
> >> <linux-imx@nxp.com>; Robin Gong <yibin.gong@nxp.com>; A.s. Dong
> >> <aisheng.dong@nxp.com>
> >> Subject: [PATCH v8 1/6] ARM: imx6q: provide documentation for new
> >> fsl,pmic-stby-poweroff property
> >>
> >> 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 a45ca67a9d5f..e1308346e00d 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.
> > PMIC_ON_REQ didn't connect to any pin of PMIC in your case?
> 
> No. First, it was only one customer specific issue. After some research I found
> even publicly available boards (for example RioTboard) which has same/similar
> design. After seeing this in imx6 documentation as valid power off way, I have
> no doubts - there should be even more devices doin this in the wild.
Not sure why the customer didn't follow reference design, since PMIC_ON_REQ can
provide power off/on feature by pressing ONOFF key which connected ONOFF
pin of i.mx6(ONOFF will toggle PMIC_ON_REQ to power off/on PMIC). The official
power off/on way (PMIC_ON_REQ) can also power off all power rails of PFUZE except
snvs as your patch did on PMIC_STBY_REQ.  PMIC_STBY_REQ is used to notify pmic
switch power mode (PFM/APS) or decrease voltage to save power in kernel suspend,
not power off....I am not sure if we need this patchset to 'workaround' the board issue.
> 
> > Don't understand
> > why not follow normal board design guide to power off pmic by
> PMIC_ON_REQ.
> > How to power on board again then?
> 
> Power cycle. Without this patch, power of is not real power off. So, power cycle,
> is expected behavior for user interaction. On usual PC, reset button will not
> enable PC as well.
Your board can't support wakeup by RTC alarm if not use PMIC_ON_REQ to power off.
Again, why your board not follow the design guide?

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

[Lucas Stach]

Hi Robin,

Am Freitag, den 27.07.2018, 01:51 +0000 schrieb Robin Gong:
[...]
> > > > ---
> > > >  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 a45ca67a9d5f..e1308346e00d 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.
> > > 
> > > PMIC_ON_REQ didn't connect to any pin of PMIC in your case?
> > 
> > No. First, it was only one customer specific issue. After some research I found
> > even publicly available boards (for example RioTboard) which has same/similar
> > design. After seeing this in imx6 documentation as valid power off way, I have
> > no doubts - there should be even more devices doin this in the wild.
> 
> Not sure why the customer didn't follow reference design, since PMIC_ON_REQ can
> provide power off/on feature by pressing ONOFF key which connected ONOFF
> pin of i.mx6(ONOFF will toggle PMIC_ON_REQ to power off/on PMIC). The official
> power off/on way (PMIC_ON_REQ) can also power off all power rails of PFUZE except
> snvs as your patch did on PMIC_STBY_REQ.  PMIC_STBY_REQ is used to notify pmic
> switch power mode (PFM/APS) or decrease voltage to save power in kernel suspend,
> not power off....I am not sure if we need this patchset to 'workaround' the board issue.
> 
Not all boards follow the reference design, that's a fact of life.

Please look at the i.MX6Q reference manual. The sequence implemented in
this patchset can be found as a valid way to power off the system in
"60.4.3 Power mode transitions" "Normal ON to OFF with external PMIC",
so there is hardly any way to argue that this is a board specific
quirk. This is one of the Freescale/NXP recommended sequences to turn
off the system.

Regards,
Lucas

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

[Oleksij Rempel]

[-- Attachment #1: Type: text/plain, Size: 5675 bytes --]

On Fri, Jul 27, 2018 at 01:51:35AM +0000, Robin Gong wrote:
> 
> 
> > -----Original Message-----
> > From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> > Sent: 2018年7月26日 19:38
> > To: Robin Gong <yibin.gong@nxp.com>; Shawn Guo <shawnguo@kernel.org>;
> > Mark Brown <broonie@kernel.org>; Rafael J. Wysocki
> > <rafael.j.wysocki@intel.com>
> > Cc: kernel@pengutronix.de; devicetree@vger.kernel.org;
> > linux-arm-kernel@lists.infradead.org; linux-clk@vger.kernel.org;
> > linux-kernel@vger.kernel.org; Andrew Morton <akpm@linux-foundation.org>;
> > Liam Girdwood <lgirdwood@gmail.com>; Leonard Crestez
> > <leonard.crestez@nxp.com>; Rob Herring <robh+dt@kernel.org>; Mark
> > Rutland <mark.rutland@arm.com>; Michael Turquette
> > <mturquette@baylibre.com>; Stephen Boyd <sboyd@codeaurora.org>; Fabio
> > Estevam <fabio.estevam@nxp.com>; Russell King <linux@armlinux.org.uk>;
> > dl-linux-imx <linux-imx@nxp.com>; A.s. Dong <aisheng.dong@nxp.com>
> > Subject: Re: [PATCH v8 1/6] ARM: imx6q: provide documentation for new
> > fsl,pmic-stby-poweroff property
> > 
> > Hi,
> > 
> > On 26.07.2018 11:51, Robin Gong wrote:
> > >
> > >
> > >> -----Original Message-----
> > >> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> > >> Sent: 2018年7月26日 17:22
> > >> To: Shawn Guo <shawnguo@kernel.org>; Mark Brown
> > <broonie@kernel.org>;
> > >> Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; kernel@pengutronix.de;
> > >> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > >> linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; Andrew
> > >> Morton <akpm@linux-foundation.org>; Liam Girdwood
> > >> <lgirdwood@gmail.com>; Leonard Crestez <leonard.crestez@nxp.com>;
> > Rob
> > >> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>;
> > >> Michael Turquette <mturquette@baylibre.com>; Stephen Boyd
> > >> <sboyd@codeaurora.org>; Fabio Estevam <fabio.estevam@nxp.com>;
> > >> Russell King <linux@armlinux.org.uk>; dl-linux-imx
> > >> <linux-imx@nxp.com>; Robin Gong <yibin.gong@nxp.com>; A.s. Dong
> > >> <aisheng.dong@nxp.com>
> > >> Subject: [PATCH v8 1/6] ARM: imx6q: provide documentation for new
> > >> fsl,pmic-stby-poweroff property
> > >>
> > >> 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 a45ca67a9d5f..e1308346e00d 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.
> > > PMIC_ON_REQ didn't connect to any pin of PMIC in your case?
> > 
> > No. First, it was only one customer specific issue. After some research I found
> > even publicly available boards (for example RioTboard) which has same/similar
> > design. After seeing this in imx6 documentation as valid power off way, I have
> > no doubts - there should be even more devices doin this in the wild.
> Not sure why the customer didn't follow reference design, since PMIC_ON_REQ can
> provide power off/on feature by pressing ONOFF key which connected ONOFF
> pin of i.mx6(ONOFF will toggle PMIC_ON_REQ to power off/on PMIC). The official
> power off/on way (PMIC_ON_REQ) can also power off all power rails of PFUZE except
> snvs as your patch did on PMIC_STBY_REQ.  PMIC_STBY_REQ is used to notify pmic
> switch power mode (PFM/APS) or decrease voltage to save power in kernel suspend,
> not power off....I am not sure if we need this patchset to 'workaround' the board issue.
> > 
> > > Don't understand
> > > why not follow normal board design guide to power off pmic by
> > PMIC_ON_REQ.
> > > How to power on board again then?
> > 
> > Power cycle. Without this patch, power of is not real power off. So, power cycle,
> > is expected behavior for user interaction. On usual PC, reset button will not
> > enable PC as well.
> Your board can't support wakeup by RTC alarm if not use PMIC_ON_REQ to power off.
> Again, why your board not follow the design guide?

Just wild assumption: you haven't read comment provided in patch:
Subject: [PATCH v8 2/6] ARM: imx6: register pm_power_off handler if
 "fsl,pmic-stby-poweroff" is set

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.

-- 
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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Robin Gong]

> -----Original Message-----
> From: Lucas Stach [mailto:l.stach@pengutronix.de]
> Sent: 2018年7月27日 16:30
> To: Robin Gong <yibin.gong@nxp.com>; Oleksij Rempel
> <o.rempel@pengutronix.de>; Shawn Guo <shawnguo@kernel.org>; Mark
> Brown <broonie@kernel.org>; Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Mark Rutland <mark.rutland@arm.com>; devicetree@vger.kernel.org;
> Michael Turquette <mturquette@baylibre.com>; Stephen Boyd
> <sboyd@codeaurora.org>; linux-kernel@vger.kernel.org; Liam Girdwood
> <lgirdwood@gmail.com>; Rob Herring <robh+dt@kernel.org>; dl-linux-imx
> <linux-imx@nxp.com>; kernel@pengutronix.de; A.s. Dong
> <aisheng.dong@nxp.com>; Fabio Estevam <fabio.estevam@nxp.com>; Russell
> King <linux@armlinux.org.uk>; Andrew Morton <akpm@linux-foundation.org>;
> Leonard Crestez <leonard.crestez@nxp.com>; linux-clk@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH v8 1/6] ARM: imx6q: provide documentation for new
> fsl,pmic-stby-poweroff property
> 
> Hi Robin,
> 
> Am Freitag, den 27.07.2018, 01:51 +0000 schrieb Robin Gong:
> [...]
> > > > > ---
> > > > >  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 a45ca67a9d5f..e1308346e00d 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.
> > > >
> > > > PMIC_ON_REQ didn't connect to any pin of PMIC in your case?
> > >
> > > No. First, it was only one customer specific issue. After some
> > > research I found even publicly available boards (for example
> > > RioTboard) which has same/similar design. After seeing this in imx6
> > > documentation as valid power off way, I have no doubts - there should be
> even more devices doin this in the wild.
> >
> > Not sure why the customer didn't follow reference design, since
> > PMIC_ON_REQ can provide power off/on feature by pressing ONOFF key
> > which connected ONOFF pin of i.mx6(ONOFF will toggle PMIC_ON_REQ to
> > power off/on PMIC). The official power off/on way (PMIC_ON_REQ) can
> > also power off all power rails of PFUZE except snvs as your patch did
> > on PMIC_STBY_REQ.  PMIC_STBY_REQ is used to notify pmic switch power
> > mode (PFM/APS) or decrease voltage to save power in kernel suspend, not
> power off....I am not sure if we need this patchset to 'workaround' the board
> issue.
> >
> Not all boards follow the reference design, that's a fact of life.
> 
> Please look at the i.MX6Q reference manual. The sequence implemented in this
> patchset can be found as a valid way to power off the system in
> "60.4.3 Power mode transitions" "Normal ON to OFF with external PMIC", so
> there is hardly any way to argue that this is a board specific quirk. This is one of
> the Freescale/NXP recommended sequences to turn off the system.
Okay, but could you add one more comment for this solution? RTC alarm and ONOFF
Button wakeup feature can't be support in this case.
> 
> Regards,
> Lucas

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

[Oleksij Rempel]

[-- Attachment #1: Type: text/plain, Size: 4410 bytes --]

On Fri, Jul 27, 2018 at 08:58:22AM +0000, Robin Gong wrote:
> 
> 
> > -----Original Message-----
> > From: Lucas Stach [mailto:l.stach@pengutronix.de]
> > Sent: 2018年7月27日 16:30
> > To: Robin Gong <yibin.gong@nxp.com>; Oleksij Rempel
> > <o.rempel@pengutronix.de>; Shawn Guo <shawnguo@kernel.org>; Mark
> > Brown <broonie@kernel.org>; Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>; devicetree@vger.kernel.org;
> > Michael Turquette <mturquette@baylibre.com>; Stephen Boyd
> > <sboyd@codeaurora.org>; linux-kernel@vger.kernel.org; Liam Girdwood
> > <lgirdwood@gmail.com>; Rob Herring <robh+dt@kernel.org>; dl-linux-imx
> > <linux-imx@nxp.com>; kernel@pengutronix.de; A.s. Dong
> > <aisheng.dong@nxp.com>; Fabio Estevam <fabio.estevam@nxp.com>; Russell
> > King <linux@armlinux.org.uk>; Andrew Morton <akpm@linux-foundation.org>;
> > Leonard Crestez <leonard.crestez@nxp.com>; linux-clk@vger.kernel.org;
> > linux-arm-kernel@lists.infradead.org
> > Subject: Re: [PATCH v8 1/6] ARM: imx6q: provide documentation for new
> > fsl,pmic-stby-poweroff property
> > 
> > Hi Robin,
> > 
> > Am Freitag, den 27.07.2018, 01:51 +0000 schrieb Robin Gong:
> > [...]
> > > > > > ---
> > > > > >  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 a45ca67a9d5f..e1308346e00d 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.
> > > > >
> > > > > PMIC_ON_REQ didn't connect to any pin of PMIC in your case?
> > > >
> > > > No. First, it was only one customer specific issue. After some
> > > > research I found even publicly available boards (for example
> > > > RioTboard) which has same/similar design. After seeing this in imx6
> > > > documentation as valid power off way, I have no doubts - there should be
> > even more devices doin this in the wild.
> > >
> > > Not sure why the customer didn't follow reference design, since
> > > PMIC_ON_REQ can provide power off/on feature by pressing ONOFF key
> > > which connected ONOFF pin of i.mx6(ONOFF will toggle PMIC_ON_REQ to
> > > power off/on PMIC). The official power off/on way (PMIC_ON_REQ) can
> > > also power off all power rails of PFUZE except snvs as your patch did
> > > on PMIC_STBY_REQ.  PMIC_STBY_REQ is used to notify pmic switch power
> > > mode (PFM/APS) or decrease voltage to save power in kernel suspend, not
> > power off....I am not sure if we need this patchset to 'workaround' the board
> > issue.
> > >
> > Not all boards follow the reference design, that's a fact of life.
> > 
> > Please look at the i.MX6Q reference manual. The sequence implemented in this
> > patchset can be found as a valid way to power off the system in
> > "60.4.3 Power mode transitions" "Normal ON to OFF with external PMIC", so
> > there is hardly any way to argue that this is a board specific quirk. This is one of
> > the Freescale/NXP recommended sequences to turn off the system.
> Okay, but could you add one more comment for this solution? RTC alarm and ONOFF
> Button wakeup feature can't be support in this case.

Sure, at least it will power off. It make no sense to have power off
functionality which use same amount of power as power on system.

-- 
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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Robin Gong]

> -----Original Message-----
> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> Sent: 2018年7月26日 17:22
> To: Shawn Guo <shawnguo@kernel.org>; Mark Brown <broonie@kernel.org>;
> Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; kernel@pengutronix.de;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; Andrew Morton
> <akpm@linux-foundation.org>; Liam Girdwood <lgirdwood@gmail.com>;
> Leonard Crestez <leonard.crestez@nxp.com>; Rob Herring
> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Michael
> Turquette <mturquette@baylibre.com>; Stephen Boyd
> <sboyd@codeaurora.org>; Fabio Estevam <fabio.estevam@nxp.com>; Russell
> King <linux@armlinux.org.uk>; dl-linux-imx <linux-imx@nxp.com>; Robin Gong
> <yibin.gong@nxp.com>; A.s. Dong <aisheng.dong@nxp.com>
> Subject: [PATCH v8 2/6] ARM: imx6: register pm_power_off handler if
> "fsl,pmic-stby-poweroff" is set
> 
> 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%2FIM
> X6DQRM.pdf&amp;data=02%7C01%7Cyibin.gong%40nxp.com%7C193fd19e3a
> 40416ffa4a08d5f2d9583c%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1
> %7C636681937661914076&amp;sdata=lICAelYpUh4%2Ft%2Fs7N9mdk2cLQMi
> cHcOqQ07vTOUoyNY%3D&amp;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 017539dd712b..2f5c643f62fb 100644
> --- a/arch/arm/mach-imx/pm-imx6.c
> +++ b/arch/arm/mach-imx/pm-imx6.c
> @@ -601,6 +601,28 @@ static void __init imx6_pm_common_init(const struct
> imx6_pm_socdata
>  				   IMX6Q_GPR1_GINT);
>  }
> 
> +static void imx6_pm_stby_poweroff(void) {
> +	imx6_set_lpm(STOP_POWER_OFF);
> +	imx6q_suspend_finish(0);
> +
> +	mdelay(1000);
> +
> +	pr_emerg("Unable to poweroff system\n"); }
> +
> +static int imx6_pm_stby_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);
'syscon-poweroff' and 'pmic-stby-poweroff ' should be chosen as a single
Poweroff way for any i.mx6 board. Why not delete directly 'syscon-poweroff' in dts 
to avoid such two power off ways coexist?
> +		return -EBUSY;
> +	}
> +
> +	pm_power_off = imx6_pm_stby_poweroff;
> +	return 0;
> +}
> +
>  void __init imx6_pm_ccm_init(const char *ccm_compat)  {
>  	struct device_node *np;
> @@ -617,6 +639,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_stby_poweroff_probe();
>  }
> 
>  void __init imx6q_pm_init(void)
> --
> 2.18.0

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

[Robin Gong]

> -----Original Message-----
> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> Sent: 2018年7月26日 17:22
> To: Shawn Guo <shawnguo@kernel.org>; Mark Brown <broonie@kernel.org>;
> Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; kernel@pengutronix.de;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; Andrew Morton
> <akpm@linux-foundation.org>; Liam Girdwood <lgirdwood@gmail.com>;
> Leonard Crestez <leonard.crestez@nxp.com>; Rob Herring
> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Michael
> Turquette <mturquette@baylibre.com>; Stephen Boyd
> <sboyd@codeaurora.org>; Fabio Estevam <fabio.estevam@nxp.com>; Russell
> King <linux@armlinux.org.uk>; dl-linux-imx <linux-imx@nxp.com>; Robin Gong
> <yibin.gong@nxp.com>; A.s. Dong <aisheng.dong@nxp.com>
> Subject: [PATCH v8 5/6] regulator: pfuze100-regulator: provide
> pm_power_off_prepare handler
> 
> 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 8d9dbcc775ea..e386e9acb3f7 100644
> --- a/drivers/regulator/pfuze100-regulator.c
> +++ b/drivers/regulator/pfuze100-regulator.c
> @@ -15,6 +15,7 @@
>  #include <linux/regulator/pfuze100.h>
>  #include <linux/i2c.h>
>  #include <linux/slab.h>
> +#include <linux/kallsyms.h>
Is it necessary?
>  #include <linux/regmap.h>
> 
>  #define PFUZE_NUMREGS		128
> @@ -29,11 +30,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
> @@ -44,6 +51,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 {
> @@ -492,6 +506,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");
Add 'if (syspm_pfuze_chip ->chip_id == PFUZE100))' here is easy for extend 
Support on pfuze200/3000.. in the feature.
> +
> +	/* 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;
> @@ -661,6 +738,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;
>  }
> 
> @@ -671,6 +762,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.18.0

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

[Robin Gong]

> -----Original Message-----
> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> Sent: 2018年7月26日 17:22
> To: Shawn Guo <shawnguo@kernel.org>; Mark Brown <broonie@kernel.org>;
> Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; kernel@pengutronix.de;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; Andrew Morton
> <akpm@linux-foundation.org>; Liam Girdwood <lgirdwood@gmail.com>;
> Leonard Crestez <leonard.crestez@nxp.com>; Rob Herring
> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Michael
> Turquette <mturquette@baylibre.com>; Stephen Boyd
> <sboyd@codeaurora.org>; Fabio Estevam <fabio.estevam@nxp.com>; Russell
> King <linux@armlinux.org.uk>; dl-linux-imx <linux-imx@nxp.com>; Robin Gong
> <yibin.gong@nxp.com>; A.s. Dong <aisheng.dong@nxp.com>
> Subject: [PATCH v8 6/6] ARM: dts: imx6: RIoTboard provide standby on power
> off option
> 
> This board, as well as some other boards with i.MX6 and a PMIC, uses a
> "PMIC_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 2e98c92adff7..a0e9753ee767 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;
> +};
It's better remove the default "syscon-poweroff" power off node.
> +
>  &fec {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&pinctrl_enet>;
> @@ -170,6 +174,7 @@
>  		reg = <0x08>;
>  		interrupt-parent = <&gpio5>;
>  		interrupts = <16 8>;
> +		fsl,pmic-stby-poweroff;
> 
>  		regulators {
>  			reg_vddcore: sw1ab {				/* VDDARM_IN */
> --
> 2.18.0

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

[Oleksij Rempel]

[-- Attachment #1.1: Type: text/plain, Size: 7233 bytes --]



On 27.07.2018 11:32, Robin Gong wrote:
> 
> 
>> -----Original Message-----
>> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
>> Sent: 2018年7月26日 17:22
>> To: Shawn Guo <shawnguo@kernel.org>; Mark Brown <broonie@kernel.org>;
>> Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; kernel@pengutronix.de;
>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>> linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; Andrew Morton
>> <akpm@linux-foundation.org>; Liam Girdwood <lgirdwood@gmail.com>;
>> Leonard Crestez <leonard.crestez@nxp.com>; Rob Herring
>> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Michael
>> Turquette <mturquette@baylibre.com>; Stephen Boyd
>> <sboyd@codeaurora.org>; Fabio Estevam <fabio.estevam@nxp.com>; Russell
>> King <linux@armlinux.org.uk>; dl-linux-imx <linux-imx@nxp.com>; Robin Gong
>> <yibin.gong@nxp.com>; A.s. Dong <aisheng.dong@nxp.com>
>> Subject: [PATCH v8 5/6] regulator: pfuze100-regulator: provide
>> pm_power_off_prepare handler
>>
>> 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 8d9dbcc775ea..e386e9acb3f7 100644
>> --- a/drivers/regulator/pfuze100-regulator.c
>> +++ b/drivers/regulator/pfuze100-regulator.c
>> @@ -15,6 +15,7 @@
>>  #include <linux/regulator/pfuze100.h>
>>  #include <linux/i2c.h>
>>  #include <linux/slab.h>
>> +#include <linux/kallsyms.h>
> Is it necessary?

yes, for pm_power_off_prepare

>>  #include <linux/regmap.h>
>>
>>  #define PFUZE_NUMREGS		128
>> @@ -29,11 +30,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
>> @@ -44,6 +51,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 {
>> @@ -492,6 +506,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");
> Add 'if (syspm_pfuze_chip ->chip_id == PFUZE100))' here is easy for extend 
> Support on pfuze200/3000.. in the feature.

ok.

>> +
>> +	/* 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;
>> @@ -661,6 +738,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;
>>  }
>>
>> @@ -671,6 +762,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.18.0
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Oleksij Rempel]

[-- Attachment #1.1: Type: text/plain, Size: 3722 bytes --]



On 27.07.2018 11:15, Robin Gong wrote:
> 
> 
>> -----Original Message-----
>> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
>> Sent: 2018年7月26日 17:22
>> To: Shawn Guo <shawnguo@kernel.org>; Mark Brown <broonie@kernel.org>;
>> Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; kernel@pengutronix.de;
>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>> linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; Andrew Morton
>> <akpm@linux-foundation.org>; Liam Girdwood <lgirdwood@gmail.com>;
>> Leonard Crestez <leonard.crestez@nxp.com>; Rob Herring
>> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Michael
>> Turquette <mturquette@baylibre.com>; Stephen Boyd
>> <sboyd@codeaurora.org>; Fabio Estevam <fabio.estevam@nxp.com>; Russell
>> King <linux@armlinux.org.uk>; dl-linux-imx <linux-imx@nxp.com>; Robin Gong
>> <yibin.gong@nxp.com>; A.s. Dong <aisheng.dong@nxp.com>
>> Subject: [PATCH v8 2/6] ARM: imx6: register pm_power_off handler if
>> "fsl,pmic-stby-poweroff" is set
>>
>> 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%2FIM
>> X6DQRM.pdf&amp;data=02%7C01%7Cyibin.gong%40nxp.com%7C193fd19e3a
>> 40416ffa4a08d5f2d9583c%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1
>> %7C636681937661914076&amp;sdata=lICAelYpUh4%2Ft%2Fs7N9mdk2cLQMi
>> cHcOqQ07vTOUoyNY%3D&amp;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 017539dd712b..2f5c643f62fb 100644
>> --- a/arch/arm/mach-imx/pm-imx6.c
>> +++ b/arch/arm/mach-imx/pm-imx6.c
>> @@ -601,6 +601,28 @@ static void __init imx6_pm_common_init(const struct
>> imx6_pm_socdata
>>  				   IMX6Q_GPR1_GINT);
>>  }
>>
>> +static void imx6_pm_stby_poweroff(void) {
>> +	imx6_set_lpm(STOP_POWER_OFF);
>> +	imx6q_suspend_finish(0);
>> +
>> +	mdelay(1000);
>> +
>> +	pr_emerg("Unable to poweroff system\n"); }
>> +
>> +static int imx6_pm_stby_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);

> 'syscon-poweroff' and 'pmic-stby-poweroff ' should be chosen as a single
> Poweroff way for any i.mx6 board. Why not delete directly 'syscon-poweroff' in dts 
> to avoid such two power off ways coexist?

pm_power_off can be registred by any part of the kernel. So, we need it
to avoid conflicts or at least to be able see them.

On other hand, you are right. syscon-poweroff should be disabled for
this board.

>> +		return -EBUSY;
>> +	}
>> +
>> +	pm_power_off = imx6_pm_stby_poweroff;
>> +	return 0;
>> +}
>> +
>>  void __init imx6_pm_ccm_init(const char *ccm_compat)  {
>>  	struct device_node *np;
>> @@ -617,6 +639,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_stby_poweroff_probe();
>>  }
>>
>>  void __init imx6q_pm_init(void)
>> --
>> 2.18.0
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Oleksij Rempel]

[-- Attachment #1.1: Type: text/plain, Size: 2303 bytes --]



On 27.07.2018 11:33, Robin Gong wrote:
> 
> 
>> -----Original Message-----
>> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
>> Sent: 2018年7月26日 17:22
>> To: Shawn Guo <shawnguo@kernel.org>; Mark Brown <broonie@kernel.org>;
>> Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; kernel@pengutronix.de;
>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>> linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; Andrew Morton
>> <akpm@linux-foundation.org>; Liam Girdwood <lgirdwood@gmail.com>;
>> Leonard Crestez <leonard.crestez@nxp.com>; Rob Herring
>> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Michael
>> Turquette <mturquette@baylibre.com>; Stephen Boyd
>> <sboyd@codeaurora.org>; Fabio Estevam <fabio.estevam@nxp.com>; Russell
>> King <linux@armlinux.org.uk>; dl-linux-imx <linux-imx@nxp.com>; Robin Gong
>> <yibin.gong@nxp.com>; A.s. Dong <aisheng.dong@nxp.com>
>> Subject: [PATCH v8 6/6] ARM: dts: imx6: RIoTboard provide standby on power
>> off option
>>
>> This board, as well as some other boards with i.MX6 and a PMIC, uses a
>> "PMIC_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 2e98c92adff7..a0e9753ee767 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;
>> +};
> It's better remove the default "syscon-poweroff" power off node.

ok

>>  &fec {
>>  	pinctrl-names = "default";
>>  	pinctrl-0 = <&pinctrl_enet>;
>> @@ -170,6 +174,7 @@
>>  		reg = <0x08>;
>>  		interrupt-parent = <&gpio5>;
>>  		interrupts = <16 8>;
>> +		fsl,pmic-stby-poweroff;
>>
>>  		regulators {
>>  			reg_vddcore: sw1ab {				/* VDDARM_IN */
>> --
>> 2.18.0
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Oleksij Rempel]

[-- Attachment #1.1: Type: text/plain, Size: 3846 bytes --]



On 27.07.2018 10:58, Robin Gong wrote:
> 
> 
>> -----Original Message-----
>> From: Lucas Stach [mailto:l.stach@pengutronix.de]
>> Sent: 2018年7月27日 16:30
>> To: Robin Gong <yibin.gong@nxp.com>; Oleksij Rempel
>> <o.rempel@pengutronix.de>; Shawn Guo <shawnguo@kernel.org>; Mark
>> Brown <broonie@kernel.org>; Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Cc: Mark Rutland <mark.rutland@arm.com>; devicetree@vger.kernel.org;
>> Michael Turquette <mturquette@baylibre.com>; Stephen Boyd
>> <sboyd@codeaurora.org>; linux-kernel@vger.kernel.org; Liam Girdwood
>> <lgirdwood@gmail.com>; Rob Herring <robh+dt@kernel.org>; dl-linux-imx
>> <linux-imx@nxp.com>; kernel@pengutronix.de; A.s. Dong
>> <aisheng.dong@nxp.com>; Fabio Estevam <fabio.estevam@nxp.com>; Russell
>> King <linux@armlinux.org.uk>; Andrew Morton <akpm@linux-foundation.org>;
>> Leonard Crestez <leonard.crestez@nxp.com>; linux-clk@vger.kernel.org;
>> linux-arm-kernel@lists.infradead.org
>> Subject: Re: [PATCH v8 1/6] ARM: imx6q: provide documentation for new
>> fsl,pmic-stby-poweroff property
>>
>> Hi Robin,
>>
>> Am Freitag, den 27.07.2018, 01:51 +0000 schrieb Robin Gong:
>> [...]
>>>>>> ---
>>>>>>  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 a45ca67a9d5f..e1308346e00d 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.
>>>>>
>>>>> PMIC_ON_REQ didn't connect to any pin of PMIC in your case?
>>>>
>>>> No. First, it was only one customer specific issue. After some
>>>> research I found even publicly available boards (for example
>>>> RioTboard) which has same/similar design. After seeing this in imx6
>>>> documentation as valid power off way, I have no doubts - there should be
>> even more devices doin this in the wild.
>>>
>>> Not sure why the customer didn't follow reference design, since
>>> PMIC_ON_REQ can provide power off/on feature by pressing ONOFF key
>>> which connected ONOFF pin of i.mx6(ONOFF will toggle PMIC_ON_REQ to
>>> power off/on PMIC). The official power off/on way (PMIC_ON_REQ) can
>>> also power off all power rails of PFUZE except snvs as your patch did
>>> on PMIC_STBY_REQ.  PMIC_STBY_REQ is used to notify pmic switch power
>>> mode (PFM/APS) or decrease voltage to save power in kernel suspend, not
>> power off....I am not sure if we need this patchset to 'workaround' the board
>> issue.
>>>
>> Not all boards follow the reference design, that's a fact of life.
>>
>> Please look at the i.MX6Q reference manual. The sequence implemented in this
>> patchset can be found as a valid way to power off the system in
>> "60.4.3 Power mode transitions" "Normal ON to OFF with external PMIC", so
>> there is hardly any way to argue that this is a board specific quirk. This is one of
>> the Freescale/NXP recommended sequences to turn off the system.

> Okay, but could you add one more comment for this solution? RTC alarm and ONOFF
> Button wakeup feature can't be support in this case.

Enough to add it in to changelog? or should it go to the binding
documentation?


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 365 bytes --]

On Mon, Jul 30, 2018 at 09:50:55AM +0200, Oleksij Rempel wrote:
> On 27.07.2018 11:32, Robin Gong wrote:

> >>  #include <linux/slab.h>
> >> +#include <linux/kallsyms.h>

> > Is it necessary?

> yes, for pm_power_off_prepare

That's a *weird* header to have to use for that symbol, are you sure
there isn't something more specific - if there isn't there should be.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Oleksij Rempel]

[-- Attachment #1.1: Type: text/plain, Size: 468 bytes --]



On 30.07.2018 12:24, Mark Brown wrote:
> On Mon, Jul 30, 2018 at 09:50:55AM +0200, Oleksij Rempel wrote:
>> On 27.07.2018 11:32, Robin Gong wrote:
> 
>>>>  #include <linux/slab.h>
>>>> +#include <linux/kallsyms.h>
> 
>>> Is it necessary?
> 
>> yes, for pm_power_off_prepare
> 
> That's a *weird* header to have to use for that symbol, are you sure
> there isn't something more specific - if there isn't there should be.

Hm... you right. Removed.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Oleksij Rempel]

[-- Attachment #1.1: Type: text/plain, Size: 7541 bytes --]



On 27.07.2018 11:32, Robin Gong wrote:
> 
> 
>> -----Original Message-----
>> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
>> Sent: 2018年7月26日 17:22
>> To: Shawn Guo <shawnguo@kernel.org>; Mark Brown <broonie@kernel.org>;
>> Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; kernel@pengutronix.de;
>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>> linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; Andrew Morton
>> <akpm@linux-foundation.org>; Liam Girdwood <lgirdwood@gmail.com>;
>> Leonard Crestez <leonard.crestez@nxp.com>; Rob Herring
>> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Michael
>> Turquette <mturquette@baylibre.com>; Stephen Boyd
>> <sboyd@codeaurora.org>; Fabio Estevam <fabio.estevam@nxp.com>; Russell
>> King <linux@armlinux.org.uk>; dl-linux-imx <linux-imx@nxp.com>; Robin Gong
>> <yibin.gong@nxp.com>; A.s. Dong <aisheng.dong@nxp.com>
>> Subject: [PATCH v8 5/6] regulator: pfuze100-regulator: provide
>> pm_power_off_prepare handler
>>
>> 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 8d9dbcc775ea..e386e9acb3f7 100644
>> --- a/drivers/regulator/pfuze100-regulator.c
>> +++ b/drivers/regulator/pfuze100-regulator.c
>> @@ -15,6 +15,7 @@
>>  #include <linux/regulator/pfuze100.h>
>>  #include <linux/i2c.h>
>>  #include <linux/slab.h>
>> +#include <linux/kallsyms.h>
> Is it necessary?
>>  #include <linux/regmap.h>
>>
>>  #define PFUZE_NUMREGS		128
>> @@ -29,11 +30,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
>> @@ -44,6 +51,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 {
>> @@ -492,6 +506,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");
> Add 'if (syspm_pfuze_chip ->chip_id == PFUZE100))' here is easy for extend 
> Support on pfuze200/3000.. in the feature.
There is already:
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;
        }


No need to add it in pfuze_power_off_prepare()

>> +
>> +	/* 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;
>> @@ -661,6 +738,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;
>>  }
>>
>> @@ -671,6 +762,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.18.0
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Oleksij Rempel]

[-- Attachment #1.1: Type: text/plain, Size: 2084 bytes --]



On 27.07.2018 11:33, Robin Gong wrote:
> 
> 
>> -----Original Message-----
>> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
>> Sent: 2018年7月26日 17:22
>> To: Shawn Guo <shawnguo@kernel.org>; Mark Brown <broonie@kernel.org>;
>> Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; kernel@pengutronix.de;
>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>> linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; Andrew Morton
>> <akpm@linux-foundation.org>; Liam Girdwood <lgirdwood@gmail.com>;
>> Leonard Crestez <leonard.crestez@nxp.com>; Rob Herring
>> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Michael
>> Turquette <mturquette@baylibre.com>; Stephen Boyd
>> <sboyd@codeaurora.org>; Fabio Estevam <fabio.estevam@nxp.com>; Russell
>> King <linux@armlinux.org.uk>; dl-linux-imx <linux-imx@nxp.com>; Robin Gong
>> <yibin.gong@nxp.com>; A.s. Dong <aisheng.dong@nxp.com>
>> Subject: [PATCH v8 6/6] ARM: dts: imx6: RIoTboard provide standby on power
>> off option
>>
>> This board, as well as some other boards with i.MX6 and a PMIC, uses a
>> "PMIC_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 2e98c92adff7..a0e9753ee767 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;
>> +};
> It's better remove the default "syscon-poweroff" power off node.

"syscon-poweroff" is by default disabled and not enabled in
arch/arm/boot/dts/imx6dl-riotboard.dts


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Robin Gong]

> >> Not all boards follow the reference design, that's a fact of life.
> >>
> >> Please look at the i.MX6Q reference manual. The sequence implemented
> >> in this patchset can be found as a valid way to power off the system
> >> in
> >> "60.4.3 Power mode transitions" "Normal ON to OFF with external
> >> PMIC", so there is hardly any way to argue that this is a board
> >> specific quirk. This is one of the Freescale/NXP recommended sequences to
> turn off the system.
> 
> > Okay, but could you add one more comment for this solution? RTC alarm
> > and ONOFF Button wakeup feature can't be support in this case.
> 
> Enough to add it in to changelog? or should it go to the binding documentation?
The binding doc is better.

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

[Robin Gong]

> >> +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");
> > Add 'if (syspm_pfuze_chip ->chip_id == PFUZE100))' here is easy for
> > extend Support on pfuze200/3000.. in the feature.
> There is already:
> 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;
>         }
> 
> 
> No need to add it in pfuze_power_off_prepare()
I saw you add chip check in pfuze_power_off_prepare_init(), but I'm saying
In the future case pfuze200/3000 may should still support this feature, but registers
are different between different chips, thus, move checking chip into pfuze_power_off_prepare()
could make the later patch for pfuze200/3000 more clear, less and easier.

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

[Lucas Stach]

Am Montag, den 06.08.2018, 02:34 +0000 schrieb Robin Gong:
> > > > Not all boards follow the reference design, that's a fact of
> > > > life.
> > > > 
> > > > Please look at the i.MX6Q reference manual. The sequence
> > > > implemented
> > > > in this patchset can be found as a valid way to power off the
> > > > system
> > > > in
> > > > "60.4.3 Power mode transitions" "Normal ON to OFF with external
> > > > PMIC", so there is hardly any way to argue that this is a board
> > > > specific quirk. This is one of the Freescale/NXP recommended
> > > > sequences to
> > 
> > turn off the system.
> > 
> > > Okay, but could you add one more comment for this solution? RTC
> > > alarm
> > > and ONOFF Button wakeup feature can't be support in this case.
> > 
> > Enough to add it in to changelog? or should it go to the binding
> > documentation?
> 
> The binding doc is better.

Sorry, I disagree.

A binding is a way to describe a specific hardware layout, it isn't the
right place to advice a hardware designer on the implications of a
specific hardware implementation. The NXP hardware design guide is a
more suitable place for this information.

We also don't mention in random bindings that the system won't be able
to brew a fresh cup of coffee.

Regards,
Lucas