| Message ID | 1458760956-29892-1-git-send-email-ivo.g.dimitrov.75@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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". -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 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. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 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. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)
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 <ivo.g.dimitrov.75@gmail.com> --- drivers/regulator/twl-regulator.c | 85 ++++++++++++++++++++++++++++++++++----- 1 file changed, 75 insertions(+), 10 deletions(-)