| Message ID | 1439923455-109818-2-git-send-email-briannorris@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Brian, thanks for catching these oversights, but see below Am Dienstag, 18. August 2015, 11:44:15 schrieb Brian Norris: > This DTS file was submitted with non-upstream bindings. I happened > across this while reviewing the jaq DTS. > > Signed-off-by: Brian Norris <briannorris@chromium.org> > Cc: Alexandru M Stan <amstan@chromium.org> > Cc: Douglas Anderson <dianders@chromium.org> > --- > Tested on jaq, not minnie > > arch/arm/boot/dts/rk3288-veyron-minnie.dts | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/boot/dts/rk3288-veyron-minnie.dts > b/arch/arm/boot/dts/rk3288-veyron-minnie.dts index > 0e30bd6bf92b..6f619c154dc6 100644 > --- a/arch/arm/boot/dts/rk3288-veyron-minnie.dts > +++ b/arch/arm/boot/dts/rk3288-veyron-minnie.dts > @@ -128,12 +128,16 @@ > regulator-min-microvolt = <3300000>; > regulator-max-microvolt = <3300000>; > regulator-name = "vcc33_touch"; > - regulator-suspend-mem-disabled; > + regulator-state-mem { > + regulator-on-in-suspend; > + }; > }; > > vcc5v_touch: SWITCH_REG2 { > regulator-name = "vcc5v_touch"; > - regulator-suspend-mem-disabled; > + regulator-state-mem { > + regulator-on-in-suspend; > + }; wouldn't regulator-suspend-mem-disabled translate to regulator-off-in-suspend? At least looks like it according to https://lkml.org/lkml/2013/7/25/592 Heiko > }; > }; > };
Hello Heiko, On Tue, Aug 18, 2015 at 9:17 PM, Heiko Stuebner <heiko@sntech.de> wrote: > Hi Brian, > > > thanks for catching these oversights, but see below > > Am Dienstag, 18. August 2015, 11:44:15 schrieb Brian Norris: >> This DTS file was submitted with non-upstream bindings. I happened >> across this while reviewing the jaq DTS. >> >> Signed-off-by: Brian Norris <briannorris@chromium.org> >> Cc: Alexandru M Stan <amstan@chromium.org> >> Cc: Douglas Anderson <dianders@chromium.org> >> --- >> Tested on jaq, not minnie >> >> arch/arm/boot/dts/rk3288-veyron-minnie.dts | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm/boot/dts/rk3288-veyron-minnie.dts >> b/arch/arm/boot/dts/rk3288-veyron-minnie.dts index >> 0e30bd6bf92b..6f619c154dc6 100644 >> --- a/arch/arm/boot/dts/rk3288-veyron-minnie.dts >> +++ b/arch/arm/boot/dts/rk3288-veyron-minnie.dts >> @@ -128,12 +128,16 @@ >> regulator-min-microvolt = <3300000>; >> regulator-max-microvolt = <3300000>; >> regulator-name = "vcc33_touch"; >> - regulator-suspend-mem-disabled; >> + regulator-state-mem { >> + regulator-on-in-suspend; >> + }; >> }; >> >> vcc5v_touch: SWITCH_REG2 { >> regulator-name = "vcc5v_touch"; >> - regulator-suspend-mem-disabled; >> + regulator-state-mem { >> + regulator-on-in-suspend; >> + }; > > wouldn't regulator-suspend-mem-disabled translate to regulator-off-in-suspend? Correct, the downstream regulator-suspend-mem-disabled is equivalent to regulator-off-in-suspend in mainline. > At least looks like it according to https://lkml.org/lkml/2013/7/25/592 > I guess you meant https://lkml.org/lkml/2014/10/10/162 since it was Chanwoo's and not Vincent's version that finally landed. > > Heiko > Best regards, Javier
On Tue, Aug 18, 2015 at 10:30 PM, Javier Martinez Canillas <javier@dowhile0.org> wrote: > On Tue, Aug 18, 2015 at 9:17 PM, Heiko Stuebner <heiko@sntech.de> wrote: >>> >>> vcc5v_touch: SWITCH_REG2 { >>> regulator-name = "vcc5v_touch"; >>> - regulator-suspend-mem-disabled; >>> + regulator-state-mem { >>> + regulator-on-in-suspend; >>> + }; >> >> wouldn't regulator-suspend-mem-disabled translate to regulator-off-in-suspend? > > Correct, the downstream regulator-suspend-mem-disabled is equivalent > to regulator-off-in-suspend in mainline. > >> At least looks like it according to https://lkml.org/lkml/2013/7/25/592 >> > > I guess you meant https://lkml.org/lkml/2014/10/10/162 since it was > Chanwoo's and not Vincent's version that finally landed. > >> oh, now I got that you referenced Vincent's patch just to show the regulator-suspend-mem-disabled property description. Sorry for the noise then but yes you are right about the translation :-) Best regards, Javier
On Tue, Aug 18, 2015 at 09:17:06PM +0200, Heiko Stuebner wrote: > Am Dienstag, 18. August 2015, 11:44:15 schrieb Brian Norris: > > This DTS file was submitted with non-upstream bindings. I happened > > across this while reviewing the jaq DTS. > > > > Signed-off-by: Brian Norris <briannorris@chromium.org> > > Cc: Alexandru M Stan <amstan@chromium.org> > > Cc: Douglas Anderson <dianders@chromium.org> > > --- > > Tested on jaq, not minnie > > > > arch/arm/boot/dts/rk3288-veyron-minnie.dts | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/arch/arm/boot/dts/rk3288-veyron-minnie.dts > > b/arch/arm/boot/dts/rk3288-veyron-minnie.dts index > > 0e30bd6bf92b..6f619c154dc6 100644 > > --- a/arch/arm/boot/dts/rk3288-veyron-minnie.dts > > +++ b/arch/arm/boot/dts/rk3288-veyron-minnie.dts > > @@ -128,12 +128,16 @@ > > regulator-min-microvolt = <3300000>; > > regulator-max-microvolt = <3300000>; > > regulator-name = "vcc33_touch"; > > - regulator-suspend-mem-disabled; > > + regulator-state-mem { > > + regulator-on-in-suspend; > > + }; > > }; > > > > vcc5v_touch: SWITCH_REG2 { > > regulator-name = "vcc5v_touch"; > > - regulator-suspend-mem-disabled; > > + regulator-state-mem { > > + regulator-on-in-suspend; > > + }; > > wouldn't regulator-suspend-mem-disabled translate to regulator-off-in-suspend? > At least looks like it according to https://lkml.org/lkml/2013/7/25/592 You are right. My bad. Will fix. (Need to fix this in jaq too.) Thanks, Brian
On Tue, Aug 18, 2015 at 03:48:31PM -0700, Brian Norris wrote: > On Tue, Aug 18, 2015 at 09:17:06PM +0200, Heiko Stuebner wrote: > > Am Dienstag, 18. August 2015, 11:44:15 schrieb Brian Norris: > > > This DTS file was submitted with non-upstream bindings. I happened > > > across this while reviewing the jaq DTS. > > > > > > Signed-off-by: Brian Norris <briannorris@chromium.org> > > > Cc: Alexandru M Stan <amstan@chromium.org> > > > Cc: Douglas Anderson <dianders@chromium.org> > > > --- > > > Tested on jaq, not minnie > > > > > > arch/arm/boot/dts/rk3288-veyron-minnie.dts | 8 ++++++-- > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > diff --git a/arch/arm/boot/dts/rk3288-veyron-minnie.dts > > > b/arch/arm/boot/dts/rk3288-veyron-minnie.dts index > > > 0e30bd6bf92b..6f619c154dc6 100644 > > > --- a/arch/arm/boot/dts/rk3288-veyron-minnie.dts > > > +++ b/arch/arm/boot/dts/rk3288-veyron-minnie.dts > > > @@ -128,12 +128,16 @@ > > > regulator-min-microvolt = <3300000>; > > > regulator-max-microvolt = <3300000>; > > > regulator-name = "vcc33_touch"; > > > - regulator-suspend-mem-disabled; > > > + regulator-state-mem { > > > + regulator-on-in-suspend; > > > + }; > > > }; > > > > > > vcc5v_touch: SWITCH_REG2 { > > > regulator-name = "vcc5v_touch"; > > > - regulator-suspend-mem-disabled; > > > + regulator-state-mem { > > > + regulator-on-in-suspend; > > > + }; > > > > wouldn't regulator-suspend-mem-disabled translate to regulator-off-in-suspend? > > At least looks like it according to https://lkml.org/lkml/2013/7/25/592 > > You are right. My bad. Will fix. (Need to fix this in jaq too.) Now that I'm looking a little closer, it seems like other existing DTS's are broken too, then. Jerry looks like it was converted to the regulator-state-mem node binding, but the conversion doesn't seem to make sense when I compare the chromium DTS sources with the for-next source I see in your tree. In -next: regulators { mic_vcc: LDO_REG2 { regulator-name = "mic_vcc"; regulator-always-on; regulator-boot-on; regulator-min-microvolt = <1800000>; regulator-max-microvolt = <1800000>; regulator-state-mem { regulator-on-in-suspend; }; }; }; But chromium had: regulators { mic_vcc: LDO_REG2 { regulator-always-on; regulator-boot-on; regulator-min-microvolt = <1800000>; regulator-max-microvolt = <1800000>; regulator-name = "mic_vcc"; regulator-suspend-mem-disabled; }; }; So I guess I'll make the proper conversion for all the veyron variants I see. Brian
Am Dienstag, 18. August 2015, 16:28:06 schrieb Brian Norris: > Now that I'm looking a little closer, it seems like other existing DTS's > are broken too, then. Jerry looks like it was converted to the > regulator-state-mem node binding, but the conversion doesn't seem to > make sense when I compare the chromium DTS sources with the for-next > source I see in your tree. > > In -next: > > regulators { > mic_vcc: LDO_REG2 { > regulator-name = "mic_vcc"; > regulator-always-on; > regulator-boot-on; > regulator-min-microvolt = <1800000>; > regulator-max-microvolt = <1800000>; > regulator-state-mem { > regulator-on-in-suspend; > }; > }; > }; > > But chromium had: > > regulators { > mic_vcc: LDO_REG2 { > regulator-always-on; > regulator-boot-on; > regulator-min-microvolt = <1800000>; > regulator-max-microvolt = <1800000>; > regulator-name = "mic_vcc"; > regulator-suspend-mem-disabled; > }; > }; > > So I guess I'll make the proper conversion for all the veyron variants I > see. great, just take into account the deep vs. shallow suspend modes :-) The original regulator-state changes did happen when we had this suspend instability (counter and gpioint stuff) and I did go with the values similar to the rk3288-evb, as this was the only one resuming at all at the time. So I guess I never looked to closesly what it did, as long as the system came out of suspend sucessfully again :-) Heiko
Hi,
On Tue, Aug 18, 2015 at 11:19 PM, Heiko Stuebner <heiko@sntech.de> wrote:
> great, just take into account the deep vs. shallow suspend modes :-)
One note: do you think it would make sense to re-implement shallow
suspend as "standby"? I had a proof of concept doing that in
<https://chromium-review.googlesource.com/#/c/275123/>. One nice
advantage is that you "magically" get a second set of regulator states
for standby vs "mem".
If I understand correctly, the distinction between "standby" and "mem"
is not too clearly defined, so if we wanted to use it for this it
wouldn't be terrible?
-Doug
Am Donnerstag, 27. August 2015, 12:30:51 schrieb Doug Anderson: > Hi, > > On Tue, Aug 18, 2015 at 11:19 PM, Heiko Stuebner <heiko@sntech.de> wrote: > > great, just take into account the deep vs. shallow suspend modes :-) > > One note: do you think it would make sense to re-implement shallow > suspend as "standby"? I had a proof of concept doing that in > <https://chromium-review.googlesource.com/#/c/275123/>. One nice > advantage is that you "magically" get a second set of regulator states > for standby vs "mem". Somewhere I've read something about keeping wifi associated to an ap during suspend which might be a candidate for such a distinction? > If I understand correctly, the distinction between "standby" and "mem" > is not too clearly defined, so if we wanted to use it for this it > wouldn't be terrible? From reading Documentation/power/states.txt it looks like the boot-cpu is supposed to retain power in the suspend state. Although we also do not lose "operating state" in our suspend I guess? So using the shallow suspend as standby sounds interesting, for the time when the deep suspend works too. If there is only one suspend state it automatically becomes the "mem"-state it seems. Heiko
On Thu, Aug 27, 2015 at 10:51:22PM +0200, Heiko Stuebner wrote: > Am Donnerstag, 27. August 2015, 12:30:51 schrieb Doug Anderson: > > If I understand correctly, the distinction between "standby" and "mem" > > is not too clearly defined, so if we wanted to use it for this it > > wouldn't be terrible? I never understood many clear definitions here either, personally. > From reading Documentation/power/states.txt it looks like the boot-cpu is > supposed to retain power in the suspend state. Although we also do not lose > "operating state" in our suspend I guess? > > So using the shallow suspend as standby sounds interesting, for the time when > the deep suspend works too. If there is only one suspend state it > automatically becomes the "mem"-state it seems. It's not really "automatic", it's a product of this line: static const struct platform_suspend_ops rk3288_suspend_ops = { .enter = rk3288_suspend_enter, .valid = suspend_valid_only_mem, <--- here .prepare = rk3288_suspend_prepare, .finish = rk3288_suspend_finish, }; and the fact that we don't check the 'state' argument in .enter/.prepare/.finish. But still, I'm not sure it's productive to rename shallow until we support deep. Regards, Brian
diff --git a/arch/arm/boot/dts/rk3288-veyron-minnie.dts b/arch/arm/boot/dts/rk3288-veyron-minnie.dts index 0e30bd6bf92b..6f619c154dc6 100644 --- a/arch/arm/boot/dts/rk3288-veyron-minnie.dts +++ b/arch/arm/boot/dts/rk3288-veyron-minnie.dts @@ -128,12 +128,16 @@ regulator-min-microvolt = <3300000>; regulator-max-microvolt = <3300000>; regulator-name = "vcc33_touch"; - regulator-suspend-mem-disabled; + regulator-state-mem { + regulator-on-in-suspend; + }; }; vcc5v_touch: SWITCH_REG2 { regulator-name = "vcc5v_touch"; - regulator-suspend-mem-disabled; + regulator-state-mem { + regulator-on-in-suspend; + }; }; }; };
This DTS file was submitted with non-upstream bindings. I happened across this while reviewing the jaq DTS. Signed-off-by: Brian Norris <briannorris@chromium.org> Cc: Alexandru M Stan <amstan@chromium.org> Cc: Douglas Anderson <dianders@chromium.org> --- Tested on jaq, not minnie arch/arm/boot/dts/rk3288-veyron-minnie.dts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)