Assigning a device group to a regulator does not change its state. To
change the state of a regulator a message over the powerbus is required.
Also, the check for the current state of a regulator should not count on
a device group being assigned, but on the current resource state.
Signed-off-by: Ivaylo Dimitrov <[email protected]>
---
drivers/regulator/twl-regulator.c | 85 ++++++++++++++++++++++++++++++++++-----
1 file changed, 75 insertions(+), 10 deletions(-)
diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
index 955a6fb..125d5b1 100644
--- a/drivers/regulator/twl-regulator.c
+++ b/drivers/regulator/twl-regulator.c
@@ -21,7 +21,7 @@
#include <linux/regulator/machine.h>
#include <linux/regulator/of_regulator.h>
#include <linux/i2c/twl.h>
-
+#include <linux/delay.h>
/*
* The TWL4030/TW5030/TPS659x0/TWL6030 family chips include power management, a
@@ -165,7 +165,7 @@ static int twl4030reg_is_enabled(struct regulator_dev *rdev)
if (state < 0)
return state;
- return state & P1_GRP_4030;
+ return (state & 0x0f) != 0;
}
static int twl6030reg_is_enabled(struct regulator_dev *rdev)
@@ -188,11 +188,78 @@ static int twl6030reg_is_enabled(struct regulator_dev *rdev)
return grp && (val == TWL6030_CFG_STATE_ON);
}
+#define PB_I2C_BUSY BIT(0)
+#define PB_I2C_BWEN BIT(1)
+
+static int twl4030_wait_pb_ready(void)
+{
+
+ int ret;
+ int timeout = 10;
+ u8 val;
+
+ do {
+ ret = twl_i2c_read_u8(TWL_MODULE_PM_MASTER, &val,
+ TWL4030_PM_MASTER_PB_CFG);
+ if (ret < 0)
+ return ret;
+
+ if (!(val & PB_I2C_BUSY))
+ return 0;
+
+ mdelay(1);
+ timeout--;
+ } while (timeout);
+
+ return -ETIMEDOUT;
+}
+
+static int twl4030_send_pb_msg(unsigned msg)
+{
+ u8 val;
+ int ret;
+
+ /* save powerbus configuration */
+ ret = twl_i2c_read_u8(TWL_MODULE_PM_MASTER, &val,
+ TWL4030_PM_MASTER_PB_CFG);
+ if (ret < 0)
+ return ret;
+
+ /* Enable I2C access to powerbus */
+ ret = twl_i2c_write_u8(TWL_MODULE_PM_MASTER, val | PB_I2C_BWEN,
+ TWL4030_PM_MASTER_PB_CFG);
+ if (ret < 0)
+ return ret;
+
+ ret = twl4030_wait_pb_ready();
+ if (ret < 0)
+ return ret;
+
+ ret = twl_i2c_write_u8(TWL_MODULE_PM_MASTER, msg >> 8,
+ TWL4030_PM_MASTER_PB_WORD_MSB);
+ if (ret < 0)
+ return ret;
+
+ ret = twl_i2c_write_u8(TWL_MODULE_PM_MASTER, msg & 0xff,
+ TWL4030_PM_MASTER_PB_WORD_LSB);
+ if (ret < 0)
+ return ret;
+
+ ret = twl4030_wait_pb_ready();
+ if (ret < 0)
+ return ret;
+
+ /* Restore powerbus configuration */
+ return twl_i2c_write_u8(TWL_MODULE_PM_MASTER, val,
+ TWL_MODULE_PM_MASTER);
+}
+
static int twl4030reg_enable(struct regulator_dev *rdev)
{
struct twlreg_info *info = rdev_get_drvdata(rdev);
int grp;
int ret;
+ unsigned message;
grp = twlreg_grp(rdev);
if (grp < 0)
@@ -201,8 +268,12 @@ static int twl4030reg_enable(struct regulator_dev *rdev)
grp |= P1_GRP_4030;
ret = twlreg_write(info, TWL_MODULE_PM_RECEIVER, VREG_GRP, grp);
+ if (ret < 0)
+ return ret;
- return ret;
+ message = MSG_SINGULAR(DEV_GRP_P1, info->id, RES_STATE_ACTIVE);
+
+ return twl4030_send_pb_msg(message);
}
static int twl6030reg_enable(struct regulator_dev *rdev)
@@ -324,13 +395,7 @@ static int twl4030reg_set_mode(struct regulator_dev *rdev, unsigned mode)
if (!(status & (P3_GRP_4030 | P2_GRP_4030 | P1_GRP_4030)))
return -EACCES;
- status = twl_i2c_write_u8(TWL_MODULE_PM_MASTER,
- message >> 8, TWL4030_PM_MASTER_PB_WORD_MSB);
- if (status < 0)
- return status;
-
- return twl_i2c_write_u8(TWL_MODULE_PM_MASTER,
- message & 0xff, TWL4030_PM_MASTER_PB_WORD_LSB);
+ return twl4030_send_pb_msg(message);
}
static int twl6030reg_set_mode(struct regulator_dev *rdev, unsigned mode)
--
1.9.1
On Wed, Mar 23, 2016 at 09:22:36PM +0200, Ivaylo Dimitrov wrote:
> Assigning a device group to a regulator does not change its state. To
> change the state of a regulator a message over the powerbus is required.
> Also, the check for the current state of a regulator should not count on
> a device group being assigned, but on the current resource state.
How did this driver ever work then? It sounds like there must be
something else going on here.
Hi Mark,
On Fri, Mar 25, 2016 at 11:17:57AM +0000, Mark Brown wrote:
> On Wed, Mar 23, 2016 at 09:22:36PM +0200, Ivaylo Dimitrov wrote:
>
> > Assigning a device group to a regulator does not change its state. To
> > change the state of a regulator a message over the powerbus is required.
> > Also, the check for the current state of a regulator should not count on
> > a device group being assigned, but on the current resource state.
>
> How did this driver ever work then? It sounds like there must be
> something else going on here.
From my understanding of the twl4030 TRM assigning a device group
means "<device group> wants this regulator enabled". It does not
change the regulator mode (sleep vs normal or in regulator-framework
terms: REGULATOR_STATUS_NORMAL vs REGULATOR_STATUS_STANDBY).
It usually works, since the default state is normal. If the system
is rebooted from a non-mainline kernel, which left the regulator in
sleep/standby, nothing in the kernel switches it to normal.
-- Sebastian
On Fri, Mar 25, 2016 at 04:02:59PM +0100, Sebastian Reichel wrote:
> On Fri, Mar 25, 2016 at 11:17:57AM +0000, Mark Brown wrote:
> > On Wed, Mar 23, 2016 at 09:22:36PM +0200, Ivaylo Dimitrov wrote:
> > > Assigning a device group to a regulator does not change its state. To
> > > change the state of a regulator a message over the powerbus is required.
> > > Also, the check for the current state of a regulator should not count on
> > > a device group being assigned, but on the current resource state.
> > How did this driver ever work then? It sounds like there must be
> > something else going on here.
> From my understanding of the twl4030 TRM assigning a device group
> means "<device group> wants this regulator enabled". It does not
> change the regulator mode (sleep vs normal or in regulator-framework
> terms: REGULATOR_STATUS_NORMAL vs REGULATOR_STATUS_STANDBY).
> It usually works, since the default state is normal. If the system
> is rebooted from a non-mainline kernel, which left the regulator in
> sleep/standby, nothing in the kernel switches it to normal.
I really can't tell how anyone could get from the changelog to what
you're saying about modes. The explanation needs to be *much* clearer.
Part of the confusion is that if you're trying to do something to do
with the mode support that really needs to use the mode APIs, enabling
or disabling the regulator should not silently change the mode.
On 25.03.2016 17:54, Mark Brown wrote:
> On Fri, Mar 25, 2016 at 04:02:59PM +0100, Sebastian Reichel wrote:
>> On Fri, Mar 25, 2016 at 11:17:57AM +0000, Mark Brown wrote:
>>> On Wed, Mar 23, 2016 at 09:22:36PM +0200, Ivaylo Dimitrov wrote:
>
>>>> Assigning a device group to a regulator does not change its state. To
>>>> change the state of a regulator a message over the powerbus is required.
>>>> Also, the check for the current state of a regulator should not count on
>>>> a device group being assigned, but on the current resource state.
>
>>> How did this driver ever work then? It sounds like there must be
>>> something else going on here.
In short - it does not work, the voltages and regulator states are left
on the mercy of the reset defaults and the bootloader.
>
>> From my understanding of the twl4030 TRM assigning a device group
>> means "<device group> wants this regulator enabled". It does not
>> change the regulator mode (sleep vs normal or in regulator-framework
>> terms: REGULATOR_STATUS_NORMAL vs REGULATOR_STATUS_STANDBY).
>
>> It usually works, since the default state is normal. If the system
>> is rebooted from a non-mainline kernel, which left the regulator in
>> sleep/standby, nothing in the kernel switches it to normal.
>
This is exactly what happens
> I really can't tell how anyone could get from the changelog to what
> you're saying about modes. The explanation needs to be *much* clearer.
>
> Part of the confusion is that if you're trying to do something to do
> with the mode support that really needs to use the mode APIs, enabling
> or disabling the regulator should not silently change the mode.
>
Ok, so you say that regulator framework should call
twl4030reg_set_mode(), but it doesn't. If that is the case, then the bug
is in the regulator framework, a similar one to what you've fixed in
"regulator: core: Always flag voltage constraints as appliable".
On Fri, Mar 25, 2016 at 06:09:27PM +0200, Ivaylo Dimitrov wrote:
> Ok, so you say that regulator framework should call twl4030reg_set_mode(),
> but it doesn't. If that is the case, then the bug is in the regulator
> framework, a similar one to what you've fixed in "regulator: core: Always
> flag voltage constraints as appliable".
What makes you claim that this is a bug in the framework? Does anything
in the machine configuration say that changing the modes is allowed?
Hi,
On Fri, Mar 25, 2016 at 03:54:19PM +0000, Mark Brown wrote:
> On Fri, Mar 25, 2016 at 04:02:59PM +0100, Sebastian Reichel wrote:
> > On Fri, Mar 25, 2016 at 11:17:57AM +0000, Mark Brown wrote:
> > > On Wed, Mar 23, 2016 at 09:22:36PM +0200, Ivaylo Dimitrov wrote:
>
> > > > Assigning a device group to a regulator does not change its state. To
> > > > change the state of a regulator a message over the powerbus is required.
> > > > Also, the check for the current state of a regulator should not count on
> > > > a device group being assigned, but on the current resource state.
>
> > > How did this driver ever work then? It sounds like there must be
> > > something else going on here.
>
> > From my understanding of the twl4030 TRM assigning a device group
> > means "<device group> wants this regulator enabled". It does not
> > change the regulator mode (sleep vs normal or in regulator-framework
> > terms: REGULATOR_STATUS_NORMAL vs REGULATOR_STATUS_STANDBY).
>
> > It usually works, since the default state is normal. If the system
> > is rebooted from a non-mainline kernel, which left the regulator in
> > sleep/standby, nothing in the kernel switches it to normal.
>
> I really can't tell how anyone could get from the changelog to what
> you're saying about modes. The explanation needs to be *much* clearer.
>
> Part of the confusion is that if you're trying to do something to do
> with the mode support that really needs to use the mode APIs, enabling
> or disabling the regulator should not silently change the mode.
I just tried to describe what's going on, so that you can see the
whole picture. I don't agree with the patch and think that the mode
should be switched to normal at probe time. I assumed, that you
would suggest the correct solution if I describe the problem.
-- Sebastian
On 25.03.2016 18:19, Mark Brown wrote:
> On Fri, Mar 25, 2016 at 06:09:27PM +0200, Ivaylo Dimitrov wrote:
>
>> Ok, so you say that regulator framework should call twl4030reg_set_mode(),
>> but it doesn't. If that is the case, then the bug is in the regulator
>> framework, a similar one to what you've fixed in "regulator: core: Always
>> flag voltage constraints as appliable".
>
> What makes you claim that this is a bug in the framework? Does anything
> in the machine configuration say that changing the modes is allowed?
>
My understanding is that regulator core have to make sure an enabled
regulator to be in REGULATOR_STATUS_NORMAL. Now it enables the
regulator, but does not make it in REGULATOR_STATUS_NORMAL. There are 3
places set_mode() is called in regulator/core.c - in drms_uA_update(),
in regulator_set_mode() and in set_machine_constraints().
set_machine_constraints() calls set_mode() only if there is initial mode
for that regulator. I can't find a call to regulator_set_mode() anywhere
in the tree.
From the documentation:
"regulator-initial-mode: initial operating mode...."
Does the above imply that every regulator present in the system must
have "initial-mode" defined even if it is "always-on" regulator?
Also, who puts a regulator out of REGULATOR_STATUS_NORMAL if there are
no more consumers?
It might be that I am not getting the logic behind.
Ivo
On Fri, Mar 25, 2016 at 06:50:18PM +0200, Ivaylo Dimitrov wrote:
> On 25.03.2016 18:19, Mark Brown wrote:
> >What makes you claim that this is a bug in the framework? Does anything
> >in the machine configuration say that changing the modes is allowed?
> My understanding is that regulator core have to make sure an enabled
> regulator to be in REGULATOR_STATUS_NORMAL. Now it enables the regulator,
No, absolutely not. Modes are completely orthogonal to enabling and
disabling the regulator - modes reflect an efficiency/accuracy tradeoff
in the regulation, they are nothing to do with the regulator being
enabled. Setting a mode should not affect the regulator enable state
and enabling the regulator should not affect the mode.
> It might be that I am not getting the logic behind.
Yes, that seems to be the case.
On Fri, Mar 25, 2016 at 05:47:50PM +0100, Sebastian Reichel wrote:
> I just tried to describe what's going on, so that you can see the
> whole picture. I don't agree with the patch and think that the mode
> should be switched to normal at probe time. I assumed, that you
> would suggest the correct solution if I describe the problem.
Well, I'm not 100% sure what's going on. If there is a constraint in
the system that requires setting the mode then something needs to
set that but it sounds like it might be more the case that the mode
support in this driver is just broken at the minute if it's not
orthogonal to the enable support. Perhaps the mode API is being used
for suspend states or something?
On 25.03.2016 19:05, Mark Brown wrote:
> On Fri, Mar 25, 2016 at 06:50:18PM +0200, Ivaylo Dimitrov wrote:
>> On 25.03.2016 18:19, Mark Brown wrote:
>
>>> What makes you claim that this is a bug in the framework? Does anything
>>> in the machine configuration say that changing the modes is allowed?
>
>> My understanding is that regulator core have to make sure an enabled
>> regulator to be in REGULATOR_STATUS_NORMAL. Now it enables the regulator,
>
> No, absolutely not. Modes are completely orthogonal to enabling and
> disabling the regulator - modes reflect an efficiency/accuracy tradeoff
> in the regulation, they are nothing to do with the regulator being
> enabled. Setting a mode should not affect the regulator enable state
> and enabling the regulator should not affect the mode.
>
>> It might be that I am not getting the logic behind.
>
> Yes, that seems to be the case.
>
Fair enough.
Now, what am I supposed to do to the fix the problem. Will try to
explain it in more details:
On Nokia N900 regulators are left in the mode last set by the bootloader
or by the stock kernel, depends on whether it is power-on or reboot from
stock kernel to mainline. That leads to problem with devices connected
to vmmc2 regulator - when the device is rebooted from stock kernel vmmc2
is left in "sleep" mode (REGULATOR_STATUS_STANDBY in terms of regulator
framework) and as noone in mainline kernel switches vmmc2 regulator to
normal (REGULATOR_STATUS_NORMAL) mode, devices supplied by it does not
get enough power to operate normally.
On Fri, Mar 25, 2016 at 07:22:45PM +0200, Ivaylo Dimitrov wrote:
> On Nokia N900 regulators are left in the mode last set by the bootloader or
> by the stock kernel, depends on whether it is power-on or reboot from stock
> kernel to mainline. That leads to problem with devices connected to vmmc2
> regulator - when the device is rebooted from stock kernel vmmc2 is left in
> "sleep" mode (REGULATOR_STATUS_STANDBY in terms of regulator framework) and
> as noone in mainline kernel switches vmmc2 regulator to normal
> (REGULATOR_STATUS_NORMAL) mode, devices supplied by it does not get enough
> power to operate normally.
Then there is a constraint that the regulators must be in normal mode
and this needs to be expressed in the machine constraints.
Hi Mark,
On Fri, Mar 25, 2016 at 06:20:17PM +0000, Mark Brown wrote:
> On Fri, Mar 25, 2016 at 07:22:45PM +0200, Ivaylo Dimitrov wrote:
>
> > On Nokia N900 regulators are left in the mode last set by the bootloader or
> > by the stock kernel, depends on whether it is power-on or reboot from stock
> > kernel to mainline. That leads to problem with devices connected to vmmc2
> > regulator - when the device is rebooted from stock kernel vmmc2 is left in
> > "sleep" mode (REGULATOR_STATUS_STANDBY in terms of regulator framework) and
> > as noone in mainline kernel switches vmmc2 regulator to normal
> > (REGULATOR_STATUS_NORMAL) mode, devices supplied by it does not get enough
> > power to operate normally.
>
> Then there is a constraint that the regulators must be in normal mode
> and this needs to be expressed in the machine constraints.
As in adding "regulator-initial-mode = <TWL4030_OPMODE_NORMAL>;" to
the regulator's DT node (and providing an of_map_mode method in the
twl-regulator driver)?
-- Sebastian
On Fri, Mar 25, 2016 at 09:19:12PM +0100, Sebastian Reichel wrote:
> On Fri, Mar 25, 2016 at 06:20:17PM +0000, Mark Brown wrote:
> > Then there is a constraint that the regulators must be in normal mode
> > and this needs to be expressed in the machine constraints.
> As in adding "regulator-initial-mode = <TWL4030_OPMODE_NORMAL>;" to
> the regulator's DT node (and providing an of_map_mode method in the
> twl-regulator driver)?
Yes, that sounds like a sensible way to do it.
On 26.03.2016 00:28, Mark Brown wrote:
> On Fri, Mar 25, 2016 at 09:19:12PM +0100, Sebastian Reichel wrote:
>> On Fri, Mar 25, 2016 at 06:20:17PM +0000, Mark Brown wrote:
>
>>> Then there is a constraint that the regulators must be in normal mode
>>> and this needs to be expressed in the machine constraints.
>
>> As in adding "regulator-initial-mode = <TWL4030_OPMODE_NORMAL>;" to
>> the regulator's DT node (and providing an of_map_mode method in the
>> twl-regulator driver)?
>
> Yes, that sounds like a sensible way to do it.
>
Ok, going to send the relevant patches.