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

Commit Message

Oleksij Rempel July 26, 2018, 9:22 a.m. UTC

Signed-off-by: Oleksij Rempel <[e-mail]>
Acked-by: Rob Herring <[e-mail]>
---
 Documentation/devicetree/bindings/clock/imx6q-clock.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Robin Gong July 26, 2018, 9:51 a.m. UTC | #1

> -----Original Message-----
> From: Oleksij Rempel [mailto:[e-mail]]
> Sent: 2018年7月26日 17:22
> To: Shawn Guo <[e-mail]>; Mark Brown <[e-mail]>;
> Rafael J. Wysocki <[e-mail]>
> Cc: Oleksij Rempel <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Andrew Morton
> <[e-mail]>; Liam Girdwood <[e-mail]>;
> Leonard Crestez <[e-mail]>; Rob Herring
> <[e-mail]>; Mark Rutland <[e-mail]>; Michael
> Turquette <[e-mail]>; Stephen Boyd
> <[e-mail]>; Fabio Estevam <[e-mail]>; Russell
> King <[e-mail]>; dl-linux-imx <[e-mail]>; Robin Gong
> <[e-mail]>; A.s. Dong <[e-mail]>
> Subject: [PATCH v8 1/6] ARM: imx6q: provide documentation for new
> fsl,pmic-stby-poweroff property
> 
> Signed-off-by: Oleksij Rempel <[e-mail]>
> Acked-by: Rob Herring <[e-mail]>
> ---
>  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

Oleksij Rempel July 26, 2018, 11:37 a.m. UTC | #2

Hi,

On 26.07.2018 11:51, Robin Gong wrote:
> 
> 
>> -----Original Message-----
>> From: Oleksij Rempel [mailto:[e-mail]]
>> Sent: 2018年7月26日 17:22
>> To: Shawn Guo <[e-mail]>; Mark Brown <[e-mail]>;
>> Rafael J. Wysocki <[e-mail]>
>> Cc: Oleksij Rempel <[e-mail]>; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; Andrew Morton
>> <[e-mail]>; Liam Girdwood <[e-mail]>;
>> Leonard Crestez <[e-mail]>; Rob Herring
>> <[e-mail]>; Mark Rutland <[e-mail]>; Michael
>> Turquette <[e-mail]>; Stephen Boyd
>> <[e-mail]>; Fabio Estevam <[e-mail]>; Russell
>> King <[e-mail]>; dl-linux-imx <[e-mail]>; Robin Gong
>> <[e-mail]>; A.s. Dong <[e-mail]>
>> Subject: [PATCH v8 1/6] ARM: imx6q: provide documentation for new
>> fsl,pmic-stby-poweroff property
>>
>> Signed-off-by: Oleksij Rempel <[e-mail]>
>> Acked-by: Rob Herring <[e-mail]>
>> ---
>>  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.

Robin Gong July 27, 2018, 1:51 a.m. UTC | #3

> -----Original Message-----
> From: Oleksij Rempel [mailto:[e-mail]]
> Sent: 2018年7月26日 19:38
> To: Robin Gong <[e-mail]>; Shawn Guo <[e-mail]>;
> Mark Brown <[e-mail]>; Rafael J. Wysocki
> <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; Andrew Morton <[e-mail]>;
> Liam Girdwood <[e-mail]>; Leonard Crestez
> <[e-mail]>; Rob Herring <[e-mail]>; Mark
> Rutland <[e-mail]>; Michael Turquette
> <[e-mail]>; Stephen Boyd <[e-mail]>; Fabio
> Estevam <[e-mail]>; Russell King <[e-mail]>;
> dl-linux-imx <[e-mail]>; A.s. Dong <[e-mail]>
> 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:[e-mail]]
> >> Sent: 2018年7月26日 17:22
> >> To: Shawn Guo <[e-mail]>; Mark Brown
> <[e-mail]>;
> >> Rafael J. Wysocki <[e-mail]>
> >> Cc: Oleksij Rempel <[e-mail]>; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; Andrew
> >> Morton <[e-mail]>; Liam Girdwood
> >> <[e-mail]>; Leonard Crestez <[e-mail]>;
> Rob
> >> Herring <[e-mail]>; Mark Rutland <[e-mail]>;
> >> Michael Turquette <[e-mail]>; Stephen Boyd
> >> <[e-mail]>; Fabio Estevam <[e-mail]>;
> >> Russell King <[e-mail]>; dl-linux-imx
> >> <[e-mail]>; Robin Gong <[e-mail]>; A.s. Dong
> >> <[e-mail]>
> >> Subject: [PATCH v8 1/6] ARM: imx6q: provide documentation for new
> >> fsl,pmic-stby-poweroff property
> >>
> >> Signed-off-by: Oleksij Rempel <[e-mail]>
> >> Acked-by: Rob Herring <[e-mail]>
> >> ---
> >>  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?

Lucas Stach July 27, 2018, 8:30 a.m. UTC | #4

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

Oleksij Rempel July 27, 2018, 8:41 a.m. UTC | #5

On Fri, Jul 27, 2018 at 01:51:35AM +0000, Robin Gong wrote:
> 
> 
> > -----Original Message-----
> > From: Oleksij Rempel [mailto:[e-mail]]
> > Sent: 2018年7月26日 19:38
> > To: Robin Gong <[e-mail]>; Shawn Guo <[e-mail]>;
> > Mark Brown <[e-mail]>; Rafael J. Wysocki
> > <[e-mail]>
> > Cc: [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; Andrew Morton <[e-mail]>;
> > Liam Girdwood <[e-mail]>; Leonard Crestez
> > <[e-mail]>; Rob Herring <[e-mail]>; Mark
> > Rutland <[e-mail]>; Michael Turquette
> > <[e-mail]>; Stephen Boyd <[e-mail]>; Fabio
> > Estevam <[e-mail]>; Russell King <[e-mail]>;
> > dl-linux-imx <[e-mail]>; A.s. Dong <[e-mail]>
> > 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:[e-mail]]
> > >> Sent: 2018年7月26日 17:22
> > >> To: Shawn Guo <[e-mail]>; Mark Brown
> > <[e-mail]>;
> > >> Rafael J. Wysocki <[e-mail]>
> > >> Cc: Oleksij Rempel <[e-mail]>; [e-mail];
> > >> [e-mail]; [e-mail];
> > >> [e-mail]; [e-mail]; Andrew
> > >> Morton <[e-mail]>; Liam Girdwood
> > >> <[e-mail]>; Leonard Crestez <[e-mail]>;
> > Rob
> > >> Herring <[e-mail]>; Mark Rutland <[e-mail]>;
> > >> Michael Turquette <[e-mail]>; Stephen Boyd
> > >> <[e-mail]>; Fabio Estevam <[e-mail]>;
> > >> Russell King <[e-mail]>; dl-linux-imx
> > >> <[e-mail]>; Robin Gong <[e-mail]>; A.s. Dong
> > >> <[e-mail]>
> > >> Subject: [PATCH v8 1/6] ARM: imx6q: provide documentation for new
> > >> fsl,pmic-stby-poweroff property
> > >>
> > >> Signed-off-by: Oleksij Rempel <[e-mail]>
> > >> Acked-by: Rob Herring <[e-mail]>
> > >> ---
> > >>  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.

Robin Gong July 27, 2018, 8:58 a.m. UTC | #6

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

Oleksij Rempel July 27, 2018, 9:06 a.m. UTC | #7

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

Oleksij Rempel July 30, 2018, 8:03 a.m. UTC | #8

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

Robin Gong Aug. 6, 2018, 2:34 a.m. UTC | #9

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

Lucas Stach Aug. 6, 2018, 4:49 p.m. UTC | #10

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

Patch

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.