| Message ID | 1414583525-17395-8-git-send-email-zyw@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Chris, On Wed, Oct 29, 2014 at 4:52 AM, Chris Zhong <zyw@rock-chips.com> wrote: > global_pwroff would be pull to high when RK3288 entering suspend, > this pin is a sleep signal for RK808, so RK808 could goto sleep > mode, and some regulators would be disable. > > Signed-off-by: Chris Zhong <zyw@rock-chips.com> > > --- > > Changes in v6: None > Changes in v5: None > Changes in v4: None > Changes in v3: None > Changes in v2: None > > arch/arm/boot/dts/rk3288-evb-rk808.dts | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/boot/dts/rk3288-evb-rk808.dts b/arch/arm/boot/dts/rk3288-evb-rk808.dts > index ff522f8..319fcb6 100644 > --- a/arch/arm/boot/dts/rk3288-evb-rk808.dts > +++ b/arch/arm/boot/dts/rk3288-evb-rk808.dts > @@ -27,7 +27,7 @@ > interrupt-parent = <&gpio0>; > interrupts = <4 IRQ_TYPE_LEVEL_LOW>; > pinctrl-names = "default"; > - pinctrl-0 = <&pmic_int>; > + pinctrl-0 = <&pmic_int &global_pwroff>; > rockchip,system-power-controller; > wakeup-source; > #clock-cells = <1>; > @@ -46,6 +46,7 @@ > regulator-min-microvolt = <750000>; > regulator-max-microvolt = <1300000>; > regulator-name = "vdd_arm"; > + regulator-suspend-mem-disabled; NAK. "regulator-suspend-mem-disabled" is a local property added by local Chrome OS patches and doesn't belong in an upstream submission. You should using the new patches from Chanwoo. 40e20d6 regulator: of: Add support for parsing regulator_state for suspend state 291d761 regulator: Document binding for regulator suspend state for PM state In your case, it would look like: regulator-state-mem { regulator-off-in-suspend; }; > @@ -76,6 +80,7 @@ > regulator-min-microvolt = <3300000>; > regulator-max-microvolt = <3300000>; > regulator-name = "vccio_pmu"; > + regulator-suspend-mem-microvolt = <3300000>; Similarly this property isn't upstream. You can see Javier's work on this in <https://patchwork.kernel.org/patch/5106351/> and I think you'll need an rk808-specific patch just like he needs an max77802 patch. You probably want to wait for him to spin it first, though, since Mark had feedback on his last patch. -Doug
Hello Chris, On 10/29/2014 04:40 PM, Doug Anderson wrote: >> regulator-min-microvolt = <750000>; >> regulator-max-microvolt = <1300000>; >> regulator-name = "vdd_arm"; >> + regulator-suspend-mem-disabled; > > NAK. "regulator-suspend-mem-disabled" is a local property added by > local Chrome OS patches and doesn't belong in an upstream submission. > > You should using the new patches from Chanwoo. > > 40e20d6 regulator: of: Add support for parsing regulator_state for suspend state > 291d761 regulator: Document binding for regulator suspend state for PM state > > In your case, it would look like: > > regulator-state-mem { > regulator-off-in-suspend; > }; Agreed. > >> @@ -76,6 +80,7 @@ >> regulator-min-microvolt = <3300000>; >> regulator-max-microvolt = <3300000>; >> regulator-name = "vccio_pmu"; >> + regulator-suspend-mem-microvolt = <3300000>; > > Similarly this property isn't upstream. You can see Javier's work on > this in <https://patchwork.kernel.org/patch/5106351/> and I think > you'll need an rk808-specific patch just like he needs an max77802 > patch. You probably want to wait for him to spin it first, though, > since Mark had feedback on his last patch. > I'm working on adding support to configure the regulator mode on startup and when the system enters in a suspend state. As Doug said I've to re-spin since Mark wanted things to be more integrated with the core. So I'm doing some refactoring to pass the static regulator descriptor to the function extracting the regulator initial data so drivers should only define a function handler that does the modes translation. I believe this is the more sensible place to add the mapping function since the modes translation should be a non-varying property of the regulator. Having said that, I see a different use case here. You want to set a voltage on system suspend. But the value is the same that is set to your fixed regulator so I wonder if what you want here is to enable the regulator on suspend instead? In other words, do you want the core to call rk808_set_suspend_voltage() or rk808_set_suspend_enable()? If the later then you can use Chanwoo's bindings: regulator-state-mem { regulator-on-in-suspend; }; If you want the former then you should look at Chanwoo's previous series that had support to define the voltage for regulators during a suspend state [0] Mark had some questions about how that should play with the regulator voltage range [1] but was not against the idea AFAIU. Best regards, Javier [0]: https://lkml.org/lkml/2014/8/18/22 [1]: https://lkml.org/lkml/2014/9/4/651
Javier, On Wed, Oct 29, 2014 at 9:29 AM, Javier Martinez Canillas <javier.martinez@collabora.co.uk> wrote: >>> @@ -76,6 +80,7 @@ >>> regulator-min-microvolt = <3300000>; >>> regulator-max-microvolt = <3300000>; >>> regulator-name = "vccio_pmu"; >>> + regulator-suspend-mem-microvolt = <3300000>; >> >> Similarly this property isn't upstream. You can see Javier's work on >> this in <https://patchwork.kernel.org/patch/5106351/> and I think >> you'll need an rk808-specific patch just like he needs an max77802 >> patch. You probably want to wait for him to spin it first, though, >> since Mark had feedback on his last patch. >> > > I'm working on adding support to configure the regulator mode on startup > and when the system enters in a suspend state. > > As Doug said I've to re-spin since Mark wanted things to be more integrated > with the core. So I'm doing some refactoring to pass the static regulator > descriptor to the function extracting the regulator initial data so drivers > should only define a function handler that does the modes translation. > I believe this is the more sensible place to add the mapping function since > the modes translation should be a non-varying property of the regulator. > > Having said that, I see a different use case here. You want to set a voltage > on system suspend. But the value is the same that is set to your fixed > regulator so I wonder if what you want here is to enable the regulator on > suspend instead? > > In other words, do you want the core to call rk808_set_suspend_voltage() or > rk808_set_suspend_enable()? If the later then you can use Chanwoo's bindings: > > regulator-state-mem { > regulator-on-in-suspend; > }; The rk808 has a separate register for storing the sleep voltage, though. ...so just using "regulator-on-in-suspend" without any changes to the rk808 driver won't get us this "for free". I'd expect to need regulator-state-mem { regulator-on-in-suspend; regulator-suspend-microvolts = <3300000>; }; IMHO it wouldn't be insane to say that if someone has "regulator-on-in-suspend" but no suspend voltage defined that it should carry forward the runtime voltage. I actually argued for that earlier and Mark Brown said "no" at <http://marc.info/?l=linux-kernel&m=141277262106368&w=2>. More at <http://marc.info/?l=linux-kernel&m=141279245027919&w=2>. It's entirely possible I wasn't making myself clear, though. -Doug
Hello Doug, On 10/29/2014 05:51 PM, Doug Anderson wrote: >> >> In other words, do you want the core to call rk808_set_suspend_voltage() or >> rk808_set_suspend_enable()? If the later then you can use Chanwoo's bindings: >> >> regulator-state-mem { >> regulator-on-in-suspend; >> }; > > The rk808 has a separate register for storing the sleep voltage, > though. ...so just using "regulator-on-in-suspend" without any > changes to the rk808 driver won't get us this "for free". I'd expect > to need > > regulator-state-mem { > regulator-on-in-suspend; > regulator-suspend-microvolts = <3300000>; > }; > I see, then as you said the voltage has to be defined in the DT for the suspend state. As I mentioned in my previous email, Chanwoo's original suspend state series had a "regulator-volt" DT property that was used to fill the struct regulator_state .uV field so the core could call the .set_suspend_voltage function handler. AFAIU Mark wanted the property to be renamed to "rebulator-microvolt" [0] to be consistent with the rest of the binding and also he was not sure if the suspend voltage needed be in the runtime voltage range specified by the properties "regulator-{min,max}-microvolt" since Chanwoo's patch had a check for that [1]. Chanwoo decided to drop setting the suspend voltage in a following version, but given that there is a use case for this feature, I think that those two issues could be addressed and added that property to the generic regulator DT binding. > IMHO it wouldn't be insane to say that if someone has > "regulator-on-in-suspend" but no suspend voltage defined that it > should carry forward the runtime voltage. I actually argued for that > earlier and Mark Brown said "no" at > <http://marc.info/?l=linux-kernel&m=141277262106368&w=2>. More at > <http://marc.info/?l=linux-kernel&m=141279245027919&w=2>. It's > entirely possible I wasn't making myself clear, though. > What I learned from previous Mark's explanations is that there is always a reason why the regulator API split different operations. So if there is both a .set_voltage and .set_suspend_voltage, then that is an indication for me that if someone wants a particular voltage during suspend then it has to be explicitly set and should not rely on the runtime voltage previously set. The fact that the rk808 has different registers to configure the runtime and suspend voltage seems to confirm that the API split makes sense. Best regards, Javier [0]: https://lkml.org/lkml/2014/9/4/652 [1]: https://lkml.org/lkml/2014/9/4/651
diff --git a/arch/arm/boot/dts/rk3288-evb-rk808.dts b/arch/arm/boot/dts/rk3288-evb-rk808.dts index ff522f8..319fcb6 100644 --- a/arch/arm/boot/dts/rk3288-evb-rk808.dts +++ b/arch/arm/boot/dts/rk3288-evb-rk808.dts @@ -27,7 +27,7 @@ interrupt-parent = <&gpio0>; interrupts = <4 IRQ_TYPE_LEVEL_LOW>; pinctrl-names = "default"; - pinctrl-0 = <&pmic_int>; + pinctrl-0 = <&pmic_int &global_pwroff>; rockchip,system-power-controller; wakeup-source; #clock-cells = <1>; @@ -46,6 +46,7 @@ regulator-min-microvolt = <750000>; regulator-max-microvolt = <1300000>; regulator-name = "vdd_arm"; + regulator-suspend-mem-disabled; }; vdd_gpu: DCDC_REG2 { @@ -54,12 +55,14 @@ regulator-min-microvolt = <850000>; regulator-max-microvolt = <1250000>; regulator-name = "vdd_gpu"; + regulator-suspend-mem-disabled; }; vcc_ddr: DCDC_REG3 { regulator-always-on; regulator-boot-on; regulator-name = "vcc_ddr"; + regulator-suspend-mem-enabled; }; vcc_io: DCDC_REG4 { @@ -68,6 +71,7 @@ regulator-min-microvolt = <3300000>; regulator-max-microvolt = <3300000>; regulator-name = "vcc_io"; + regulator-suspend-mem-microvolt = <3300000>; }; vccio_pmu: LDO_REG1 { @@ -76,6 +80,7 @@ regulator-min-microvolt = <3300000>; regulator-max-microvolt = <3300000>; regulator-name = "vccio_pmu"; + regulator-suspend-mem-microvolt = <3300000>; }; vcc_tp: LDO_REG2 { @@ -84,6 +89,7 @@ regulator-min-microvolt = <3300000>; regulator-max-microvolt = <3300000>; regulator-name = "vcc_tp"; + regulator-suspend-mem-disabled; }; vdd_10: LDO_REG3 { @@ -92,6 +98,7 @@ regulator-min-microvolt = <1000000>; regulator-max-microvolt = <1000000>; regulator-name = "vdd_10"; + regulator-suspend-mem-microvolt = <1000000>; }; vcc18_lcd: LDO_REG4 { @@ -100,6 +107,7 @@ regulator-min-microvolt = <1800000>; regulator-max-microvolt = <1800000>; regulator-name = "vcc18_lcd"; + regulator-suspend-mem-disabled; }; vccio_sd: LDO_REG5 { @@ -108,6 +116,7 @@ regulator-min-microvolt = <1800000>; regulator-max-microvolt = <3300000>; regulator-name = "vccio_sd"; + regulator-suspend-mem-disabled; }; vdd10_lcd: LDO_REG6 { @@ -116,6 +125,7 @@ regulator-min-microvolt = <1000000>; regulator-max-microvolt = <1000000>; regulator-name = "vdd10_lcd"; + regulator-suspend-mem-disabled; }; vcc_18: LDO_REG7 { @@ -124,6 +134,7 @@ regulator-min-microvolt = <1800000>; regulator-max-microvolt = <1800000>; regulator-name = "vcc_18"; + regulator-suspend-mem-microvolt = <1800000>; }; vcca_codec: LDO_REG8 { @@ -132,18 +143,21 @@ regulator-min-microvolt = <3300000>; regulator-max-microvolt = <3300000>; regulator-name = "vcca_codec"; + regulator-suspend-mem-disabled; }; vcc_wl: SWITCH_REG1 { regulator-always-on; regulator-boot-on; regulator-name = "vcc_wl"; + regulator-suspend-mem-enabled; }; vcc_lcd: SWITCH_REG2 { regulator-always-on; regulator-boot-on; regulator-name = "vcc_lcd"; + regulator-suspend-mem-disabled; }; }; };
global_pwroff would be pull to high when RK3288 entering suspend, this pin is a sleep signal for RK808, so RK808 could goto sleep mode, and some regulators would be disable. Signed-off-by: Chris Zhong <zyw@rock-chips.com> --- Changes in v6: None Changes in v5: None Changes in v4: None Changes in v3: None Changes in v2: None arch/arm/boot/dts/rk3288-evb-rk808.dts | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)