| Message ID | 9D0D31AA57AAF5499AFDC63D6472631B09C76A@008-AM1MPN1-036.mgdnok.nokia.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
Hello, Jokiniemi Kalle (Nokia-SD/Tampere) wrote: > Hello regulator FW and OMAP3 ISP fellows, > > I'm currently optimizing power management for Nokia N900 MeeGo DE release, > and found an issue with how regulators are handled at boot. > > The N900 uses VAUX2 regulator in OMAP3430 to power the CSIb IO complex > that is used by the camera. While implementing regulator FW support to > handle this regulator in the camera driver I noticed a problem with the > regulator init sequence: > > If the device driver using the regulator does not enable and disable the > regulator after regulator_get, the regulator is left in the state that it was > after bootloader. In case of N900 this is a problem as the regulator is left > on to leak current. Of course there is the option to let regulator FW disable > all unused regulators, but this will break the N900 functionality, as the > regulator handling is not in place for many drivers. > > I found couple of solutions to this: > 1. reset all regulators that have users (regulator_get is called on them) with > a regulator_enable/disable cycle within the regulator FW. > 2. enable/disable the specific vdds_csib regulator in the omap3isp driver > to reset this one specific regulator to disabled state. > > So, please share comments on which approach is more appropriate to take? > Or maybe there is option 3? > > Here are example code for the two options (based on .37 kernel, will update > on top of appropriate tree once right solution is agreed): > > Option1: > > If a consumer device of a regulator gets a regulator, but > does not enable/disable it during probe, the regulator may > be left active from boot, even though it is not needed. If > it were needed it would be enabled by the consumer device. > > So reset the regulator on first regulator_get call to make > sure that any regulator that has users is not left active > needlessly. I'm not an expert in the regulator framework, but I'd think that one should be able to assume a regulator is in a sane state after boot. The fact that the regulator is enabled when it has no users should likely be fixed in the boot loader. Is that an option? Does the problem exist in other boards beyond N900? Another alternative to the first option you proposed could be to add a flags field to regulator_consumer_supply, and use a flag to recognise regulators which need to be disabled during initialisation. The flag could be set by using a new macro e.g. REGULATOR_SUPPLY_NASTY() when defining the regulator. Just my 0,05 euros. ;-) Cc Laurent Pinchart. Regards,
Hi, > -----Original Message----- > From: Sakari Ailus [mailto:sakari.ailus@nokia.com] > Sent: 28. huhtikuuta 2011 12:34 > To: Jokiniemi Kalle (Nokia-SD/Tampere) > Cc: broonie@opensource.wolfsonmicro.com; lrg@slimlogic.co.uk; > mchehab@infradead.org; svarbatov@mm-sol.com; saaguirre@ti.com; > grosikopulos@mm-sol.com; Zutshi Vimarsh (Nokia-SD/Helsinki); linux- > kernel@vger.kernel.org; linux-media@vger.kernel.org; Laurent Pinchart > Subject: Re: [RFC] Regulator state after regulator_get > > Hello, > > Jokiniemi Kalle (Nokia-SD/Tampere) wrote: > > Hello regulator FW and OMAP3 ISP fellows, > > > > I'm currently optimizing power management for Nokia N900 MeeGo DE > release, > > and found an issue with how regulators are handled at boot. > > > > The N900 uses VAUX2 regulator in OMAP3430 to power the CSIb IO complex > > that is used by the camera. While implementing regulator FW support to > > handle this regulator in the camera driver I noticed a problem with the > > regulator init sequence: > > > > If the device driver using the regulator does not enable and disable the > > regulator after regulator_get, the regulator is left in the state that it was > > after bootloader. In case of N900 this is a problem as the regulator is left > > on to leak current. Of course there is the option to let regulator FW disable > > all unused regulators, but this will break the N900 functionality, as the > > regulator handling is not in place for many drivers. > > > > I found couple of solutions to this: > > 1. reset all regulators that have users (regulator_get is called on them) with > > a regulator_enable/disable cycle within the regulator FW. > > 2. enable/disable the specific vdds_csib regulator in the omap3isp driver > > to reset this one specific regulator to disabled state. > > > > So, please share comments on which approach is more appropriate to take? > > Or maybe there is option 3? > > > > Here are example code for the two options (based on .37 kernel, will update > > on top of appropriate tree once right solution is agreed): > > > > Option1: > > > > If a consumer device of a regulator gets a regulator, but > > does not enable/disable it during probe, the regulator may > > be left active from boot, even though it is not needed. If > > it were needed it would be enabled by the consumer device. > > > > So reset the regulator on first regulator_get call to make > > sure that any regulator that has users is not left active > > needlessly. > > I'm not an expert in the regulator framework, but I'd think that one > should be able to assume a regulator is in a sane state after boot. The > fact that the regulator is enabled when it has no users should likely be > fixed in the boot loader. Is that an option? Not an option, the device has shipped and there will be no updates to the bootloader. > > Does the problem exist in other boards beyond N900? Don't know, I currently have only N900 to test with. > > Another alternative to the first option you proposed could be to add a > flags field to regulator_consumer_supply, and use a flag to recognise > regulators which need to be disabled during initialisation. The flag > could be set by using a new macro e.g. REGULATOR_SUPPLY_NASTY() when > defining the regulator. This sounds like a good option actually. Liam, Mark, any opinions? - Kalle > > Just my 0,05 euros. ;-) > > Cc Laurent Pinchart. > > Regards, > > -- > Sakari Ailus > sakari.ailus@maxwell.research.nokia.com -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 28, 2011 at 09:01:03AM +0000, kalle.jokiniemi@nokia.com wrote: > If the device driver using the regulator does not enable and disable the > regulator after regulator_get, the regulator is left in the state that it was > after bootloader. In case of N900 this is a problem as the regulator is left > on to leak current. Of course there is the option to let regulator FW disable > all unused regulators, but this will break the N900 functionality, as the > regulator handling is not in place for many drivers. You should use regulator_full_constraints() if your board has a fully described set of regulators. This will cause the framework to power down any regulators which aren't in use after init has completed. If you have some regulators with no consumers or missing consumers you need to mark them as always_on in their constraints. > So reset the regulator on first regulator_get call to make > sure that any regulator that has users is not left active > needlessly. This would cause lots of breakage, it would mean that all regulators that aren't always_on would get bounced off at least once during startup - that's not going to be great for things like the backlight. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 28, 2011 at 09:44:10AM +0000, kalle.jokiniemi@nokia.com wrote: > > Another alternative to the first option you proposed could be to add a > > flags field to regulator_consumer_supply, and use a flag to recognise > > regulators which need to be disabled during initialisation. The flag > > could be set by using a new macro e.g. REGULATOR_SUPPLY_NASTY() when > > defining the regulator. > This sounds like a good option actually. Liam, Mark, any opinions? I'm not sure what "supply_nasty" would mean? This also doesn't seem like something that we can set up per supply - it's going to affect the whole regulator state, it's not something that only affects a single supply. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown wrote: > On Thu, Apr 28, 2011 at 09:44:10AM +0000, kalle.jokiniemi@nokia.com wrote: > >> > Another alternative to the first option you proposed could be to add a >> > flags field to regulator_consumer_supply, and use a flag to recognise >> > regulators which need to be disabled during initialisation. The flag >> > could be set by using a new macro e.g. REGULATOR_SUPPLY_NASTY() when >> > defining the regulator. > >> This sounds like a good option actually. Liam, Mark, any opinions? > > I'm not sure what "supply_nasty" would mean? This also doesn't seem > like something that we can set up per supply - it's going to affect the > whole regulator state, it's not something that only affects a single > supply. supply_nasty() would be used to define a regulator which is enabled by the boot loader when it shouldn't be, which is the actual problem. We have a regulator which is enabled by the boot loader. However, this regulator shouldn't be on at boot since it's not needed by any devices --- the drivers for those devices will use proper regulator framework calls to use the regulator when it's needed. There's no chance to have the boot loader fixed, as stated by Kalle. How should this regulator be turned off in the boot by the kernel? Regards,
On Thu, Apr 28, 2011 at 01:27:46PM +0300, Sakari Ailus wrote: > Mark Brown wrote: > > I'm not sure what "supply_nasty" would mean? This also doesn't seem > > like something that we can set up per supply - it's going to affect the > > whole regulator state, it's not something that only affects a single > > supply. > supply_nasty() would be used to define a regulator which is enabled by > the boot loader when it shouldn't be, which is the actual problem. That's *really* not a clear name. > How should this regulator be turned off in the boot by the kernel? Have you read my previous mail where I described the existing support for doing this when we have a full set of information on the regualtors in the systems? -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown wrote: > On Thu, Apr 28, 2011 at 01:27:46PM +0300, Sakari Ailus wrote: >> Mark Brown wrote: > >>> I'm not sure what "supply_nasty" would mean? This also doesn't seem >>> like something that we can set up per supply - it's going to affect the >>> whole regulator state, it's not something that only affects a single >>> supply. > >> supply_nasty() would be used to define a regulator which is enabled by >> the boot loader when it shouldn't be, which is the actual problem. > > That's *really* not a clear name. I agree. It was just meant to imply that there's something wrong in the way it behaves. :-) >> How should this regulator be turned off in the boot by the kernel? > > Have you read my previous mail where I described the existing support > for doing this when we have a full set of information on the regualtors > in the systems? Yes, I did read it but I first understood that this use case wasn't supported right now. Having read it again, that's clearly the way to go with fixing this. Thanks. Regards,
Hi, > -----Original Message----- > From: ext Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] > Sent: 28. huhtikuuta 2011 13:06 > To: Jokiniemi Kalle (Nokia-SD/Tampere) > Cc: lrg@slimlogic.co.uk; mchehab@infradead.org; svarbatov@mm-sol.com; > saaguirre@ti.com; grosikopulos@mm-sol.com; Zutshi Vimarsh (Nokia- > SD/Helsinki); Ailus Sakari (Nokia-SD/Helsinki); linux-kernel@vger.kernel.org; > linux-media@vger.kernel.org > Subject: Re: [RFC] Regulator state after regulator_get > > On Thu, Apr 28, 2011 at 09:01:03AM +0000, kalle.jokiniemi@nokia.com wrote: > > > If the device driver using the regulator does not enable and disable the > > regulator after regulator_get, the regulator is left in the state that it was > > after bootloader. In case of N900 this is a problem as the regulator is left > > on to leak current. Of course there is the option to let regulator FW disable > > all unused regulators, but this will break the N900 functionality, as the > > regulator handling is not in place for many drivers. > > You should use regulator_full_constraints() if your board has a fully > described set of regulators. This will cause the framework to power > down any regulators which aren't in use after init has completed. If > you have some regulators with no consumers or missing consumers you need > to mark them as always_on in their constraints. I don't have a full set of regulators described, that's why things broke when I tried the regulator_full_constraints call earlier. But I don't think it would be too big issue to check the current after boot configuration and define all the regulators as you suggest. I will try this approach. > > > So reset the regulator on first regulator_get call to make > > sure that any regulator that has users is not left active > > needlessly. > > This would cause lots of breakage, it would mean that all regulators > that aren't always_on would get bounced off at least once during startup > - that's not going to be great for things like the backlight. OK, this is not a viable solution. Thanks for the comments, Kalle -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 28, 2011 at 11:08:08AM +0000, kalle.jokiniemi@nokia.com wrote: > I don't have a full set of regulators described, that's why things broke when > I tried the regulator_full_constraints call earlier. But I don't think it would be too > big issue to check the current after boot configuration and define all the > regulators as you suggest. I will try this approach. As I said in my previous mail if you've got a reagulator you don't know about which is on you can give it an always_on constraint until you figure out what (if anything) should be controlling it. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: ext Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] > Sent: 28. huhtikuuta 2011 14:15 > To: Jokiniemi Kalle (Nokia-SD/Tampere) > Cc: lrg@slimlogic.co.uk; mchehab@infradead.org; svarbatov@mm-sol.com; > saaguirre@ti.com; grosikopulos@mm-sol.com; Zutshi Vimarsh (Nokia- > SD/Helsinki); Ailus Sakari (Nokia-SD/Helsinki); linux-kernel@vger.kernel.org; > linux-media@vger.kernel.org > Subject: Re: [RFC] Regulator state after regulator_get > > On Thu, Apr 28, 2011 at 11:08:08AM +0000, kalle.jokiniemi@nokia.com wrote: > > > I don't have a full set of regulators described, that's why things broke when > > I tried the regulator_full_constraints call earlier. But I don't think it would be > too > > big issue to check the current after boot configuration and define all the > > regulators as you suggest. I will try this approach. > > As I said in my previous mail if you've got a reagulator you don't know > about which is on you can give it an always_on constraint until you > figure out what (if anything) should be controlling it. Yeps, I plan to check the PMIC for the regulators that are left on after boot In the current working setup and then put those "always_on" flags on those regulators in the final setup. Thanks for the help! - Kalle -- To unsubscribe from this list: send the line "unsubscribe linux-media" 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/core.c b/drivers/regulator/core.c index ba521f0..040d850 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -1152,6 +1152,12 @@ found: module_put(rdev->owner); } + /* Reset regulator to make sure it is disabled, if not needed */ + if (!rdev->open_count) { + regulator_enable(regulator); + regulator_disable(regulator); + } + rdev->open_count++; if (exclusive) { rdev->exclusive = 1; -- Option2: Do a enable/disable cycle of the vdds_csib regulator to make sure it is disabled after init in case there are no other users for it. Otherwise regulator framework may leave it active in case it was activated by bootloader. Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@nokia.com> --- drivers/media/video/isp/ispccp2.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-) diff --git a/drivers/media/video/isp/ispccp2.c b/drivers/media/video/isp/ispccp2.c index 2d6b014..e11957f 100644 --- a/drivers/media/video/isp/ispccp2.c +++ b/drivers/media/video/isp/ispccp2.c @@ -1201,6 +1201,11 @@ int isp_ccp2_init(struct isp_device *isp) } } + if (ccp2->vdds_csib) { + regulator_enable(ccp2->vdds); + regulator_disable(ccp2->vdds); + } + ret = isp_ccp2_init_entities(ccp2); if (ret < 0) goto out;
Hello regulator FW and OMAP3 ISP fellows, I'm currently optimizing power management for Nokia N900 MeeGo DE release, and found an issue with how regulators are handled at boot. The N900 uses VAUX2 regulator in OMAP3430 to power the CSIb IO complex that is used by the camera. While implementing regulator FW support to handle this regulator in the camera driver I noticed a problem with the regulator init sequence: If the device driver using the regulator does not enable and disable the regulator after regulator_get, the regulator is left in the state that it was after bootloader. In case of N900 this is a problem as the regulator is left on to leak current. Of course there is the option to let regulator FW disable all unused regulators, but this will break the N900 functionality, as the regulator handling is not in place for many drivers. I found couple of solutions to this: 1. reset all regulators that have users (regulator_get is called on them) with a regulator_enable/disable cycle within the regulator FW. 2. enable/disable the specific vdds_csib regulator in the omap3isp driver to reset this one specific regulator to disabled state. So, please share comments on which approach is more appropriate to take? Or maybe there is option 3? Here are example code for the two options (based on .37 kernel, will update on top of appropriate tree once right solution is agreed): Option1: If a consumer device of a regulator gets a regulator, but does not enable/disable it during probe, the regulator may be left active from boot, even though it is not needed. If it were needed it would be enabled by the consumer device. So reset the regulator on first regulator_get call to make sure that any regulator that has users is not left active needlessly. Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@nokia.com> --- drivers/regulator/core.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-) -- Thanks, Kalle -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html