Messages in this thread | | | Date | Fri, 16 Jan 2015 20:12:28 +0100 | From | Hans de Goede <> | Subject | Re: [PATCH v4 4/4] ARM: mvebu: Armada 385 GP: Add regulators to the SATA port |
| |
Hi,
On 16-01-15 13:37, Mark Brown wrote: > On Fri, Jan 16, 2015 at 11:10:18AM +0100, Hans de Goede wrote: >> On 16-01-15 10:27, Gregory CLEMENT wrote: > >>>>> + reg_sata0: pwr-sata0 { >>>>> + compatible = "regulator-fixed"; >>>>> + regulator-name = "pwr_en_sata0"; >>>>> + enable-active-high; >>>>> + regulator-always-on; > >>>> done, but we're not using a power on delay anyways. > >>> But if regulator-always-on prevent to switch it off in >>> suspend then yes using regulator-boot-on is better. > >> AFAIK regulator-always-on means exactly that and thus likely >> is not what you want. As for using regulator-off-in-suspend >> that is not necessary as the suspend method for the acpi >> driver will already turn it off. > > regulator-always-on is a bit fuzzy for suspend, if the regulator has > suspend control it'll kick in - it's really about the Linux refcounting > while it's running. What's more concerning here is that the quick > sample of the regulators flagged as always on like the above that I > looked at in the patch don't seem to have any enable control in the DT > so this will have absolutely no effect. > >> It is probably a good idea to use regulator-boot-on and >> then test things this way, and if that works use >> regulator-boot-on. > > No, it's unlikely that boot-on makes sense here - it's there for cases > where we can't read back the hardware state at power on.
Which we cannot here since we've a gpio driven regulator, with the pin in output mode, so we cannot read it back. Or at least the current kernel implementation relies on regulator-boot-on rather then reading the state back from the hardware.
If we do not specify regulator-boot-on then drivers/regulator/fixed.c : of_get_fixed_voltage_config() will set the cfg.ena_gpio_flags GPIOF_OUT_INIT_HIGH / GPIOF_OUT_INIT_LOW flags such that the regulator will get turned off when registered (*) and then it will get turned back on when the ahci driver loads causing the disk to powercycle while spinning seriously deteriorating its live time, so regulator nodes for supplies which power spinning disks definitely need regulator-boot-on.
An alternative would be using regulator-always-on, but using regulator-always-on here is wrong because it removes all of control from the driver, making e.g. runtime pm for unused disks or empty drive-bays impossible.
*) It will turn it off as soon as it requests the gpio.
> Generally > drivers should work regardless of the initial state of the regulator > (and modular drivers will actually break if they try to rely on boot-on > since we clean up unused regulators at boot).
This is not about drivers, the driver will work fine, the problem is that power-cycling disks which have already been spun-up by the bootloader is very bad for those disks.
While we're discussing this I would also like to advocate to change the behavior for regulator-boot-on to be the same as always-on when it comes to regulator_init_complete(), iow regulator-boot-on regulators should not be touched by regulator_init_complete(). It makes no sense to have different behavior for build in versus module drivers here.
For build-in the regulator is left on (or turned on even) as soon as the regulator registers, and then stays that way until explicitly turned off by a driver,
I believe we should still leave these on once userspace starts running, there is a reason they are marked as regulator-boot-on and the fact that we're ready to start running userspace does not take away that reason, to me regulator-boot-on means "do not turn off until explicitly told so by a driver which (hopefully) knows wtf it is doing".
Regards,
Hans
| |