by Robin Gong

[permalink] [raw]

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

2018-07-26 11:39:01

by Oleksij Rempel

Attachments:

(No filename) (5.54 kB)
signature.asc (499.00 B)
Download all attachments

2018-07-27 09:00:12

by Robin Gong

[permalink] [raw]

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

> -----Original Message-----
> From: Lucas Stach [mailto:[email protected]]
> Sent: 2018年7月27日 16:30
> To: Robin Gong <[email protected]>; Oleksij Rempel
> <[email protected]>; Shawn Guo <[email protected]>; Mark
> Brown <[email protected]>; Rafael J. Wysocki <[email protected]>
> Cc: Mark Rutland <[email protected]>; [email protected];
> Michael Turquette <[email protected]>; Stephen Boyd
> <[email protected]>; [email protected]; Liam Girdwood
> <[email protected]>; Rob Herring <[email protected]>; dl-linux-imx
> <[email protected]>; [email protected]; A.s. Dong
> <[email protected]>; Fabio Estevam <[email protected]>; Russell
> King <[email protected]>; Andrew Morton <[email protected]>;
> Leonard Crestez <[email protected]>; [email protected];
> [email protected]
> 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

2018-07-27 09:08:42

by Oleksij Rempel

[permalink] [raw]

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

On Fri, Jul 27, 2018 at 08:58:22AM +0000, Robin Gong wrote:
>
>
> > -----Original Message-----
> > From: Lucas Stach [mailto:[email protected]]
> > Sent: 2018年7月27日 16:30
> > To: Robin Gong <[email protected]>; Oleksij Rempel
> > <[email protected]>; Shawn Guo <[email protected]>; Mark
> > Brown <[email protected]>; Rafael J. Wysocki <[email protected]>
> > Cc: Mark Rutland <[email protected]>; [email protected];
> > Michael Turquette <[email protected]>; Stephen Boyd
> > <[email protected]>; [email protected]; Liam Girdwood
> > <[email protected]>; Rob Herring <[email protected]>; dl-linux-imx
> > <[email protected]>; [email protected]; A.s. Dong
> > <[email protected]>; Fabio Estevam <[email protected]>; Russell
> > King <[email protected]>; Andrew Morton <[email protected]>;
> > Leonard Crestez <[email protected]>; [email protected];
> > [email protected]
> > 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 |

Attachments:

(No filename) (4.31 kB)
signature.asc (499.00 B)
Download all attachments

2018-07-27 09:20:41

by Robin Gong

[permalink] [raw]

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

2018-07-27 09:33:57

by Robin Gong

[permalink] [raw]

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

> -----Original Message-----
> From: Oleksij Rempel [mailto:[email protected]]
> Sent: 2018??7??26?? 17:22
> To: Shawn Guo <[email protected]>; Mark Brown <[email protected]>;
> Rafael J. Wysocki <[email protected]>
> Cc: Oleksij Rempel <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; Andrew Morton
> <[email protected]>; Liam Girdwood <[email protected]>;
> Leonard Crestez <[email protected]>; Rob Herring
> <[email protected]>; Mark Rutland <[email protected]>; Michael
> Turquette <[email protected]>; Stephen Boyd
> <[email protected]>; Fabio Estevam <[email protected]>; Russell
> King <[email protected]>; dl-linux-imx <[email protected]>; Robin Gong
> <[email protected]>; A.s. Dong <[email protected]>
> 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 <[email protected]>
> ---
> 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

2018-07-27 09:34:36

by Robin Gong

[permalink] [raw]

signature.asc (499.00 B)
OpenPGP digital signature

2018-08-06 02:35:50

by Robin Gong

[permalink] [raw]

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

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

2018-08-06 02:52:51

by Robin Gong

[permalink] [raw]

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

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

2018-08-06 16:51:27

by Lucas Stach

[permalink] [raw]

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

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