| Message ID | 1421330978-9694-5-git-send-email-gregory.clement@free-electrons.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
Hi, On 15-01-15 15:09, Gregory CLEMENT wrote: > Add the regulators to each SATA port. > > Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> > --- > arch/arm/boot/dts/armada-388-gp.dts | 126 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 126 insertions(+) > > diff --git a/arch/arm/boot/dts/armada-388-gp.dts b/arch/arm/boot/dts/armada-388-gp.dts > index 4df22bf91683..590b383db323 100644 > --- a/arch/arm/boot/dts/armada-388-gp.dts > +++ b/arch/arm/boot/dts/armada-388-gp.dts > @@ -173,6 +173,16 @@ > status = "okay"; > #address-cells = <1>; > #size-cells = <0>; > + > + sata0: sata-port@0 { > + reg = <0>; > + target-supply = <®_5v_sata0>; > + }; > + > + sata1: sata-port@1 { > + reg = <1>; > + target-supply = <®_5v_sata1>; > + }; > }; > > sata@e0000 { > @@ -181,6 +191,16 @@ > status = "okay"; > #address-cells = <1>; > #size-cells = <0>; > + > + sata2: sata-port@0 { > + reg = <0>; > + target-supply = <®_5v_sata2>; > + }; > + > + sata3: sata-port@1 { > + reg = <1>; > + target-supply = <®_5v_sata3>; > + }; > }; > > sdhci@d8000 { > @@ -278,6 +298,112 @@ > regulator-always-on; > gpio = <&expander0 4 GPIO_ACTIVE_HIGH>; > }; > + > + reg_sata0: pwr-sata0 { > + compatible = "regulator-fixed"; > + regulator-name = "pwr_en_sata0"; > + enable-active-high; > + regulator-always-on; > + > + }; > + > + reg_5v_sata0: v5-sata0 { > + compatible = "regulator-fixed"; > + regulator-name = "v5.0-sata0"; > + regulator-min-microvolt = <5000000>; > + regulator-max-microvolt = <5000000>; > + regulator-always-on; > + vin-supply = <®_sata0>; > + }; > + > + reg_12v_sata0: v12-sata0 { > + compatible = "regulator-fixed"; > + regulator-name = "v12.0-sata0"; > + regulator-min-microvolt = <12000000>; > + regulator-max-microvolt = <12000000>; > + regulator-always-on; > + vin-supply = <®_sata0>; > + }; AFAIK the separate v5 / 12v regulators you're creating here are not used anywhere. So I guess there just here to accurately / completely describe the power topology ? > + > + reg_sata1: pwr-sata1 { > + regulator-name = "pwr_en_sata1"; > + compatible = "regulator-fixed"; > + regulator-min-microvolt = <12000000>; > + regulator-max-microvolt = <12000000>; > + enable-active-high; > + regulator-always-on; The always on here seems to a bit weird, wasn't the whole purpose of this patch set to teach ahci_platform to turn it on as needed ? You do probably want to put a regulator-boot-on here so that disks do not get an unwanted powercycle (bad for their lifetime) when the firmware has already turned on the disk. Downside of using regulator-boot-on is that if the power is not actually turned on no power-on-delay is done, but we're not using a power on delay anyways. The same goes for reg_sata2 & reg_sata3. > + gpio = <&expander0 3 GPIO_ACTIVE_HIGH>; > + }; > + > + reg_5v_sata1: v5-sata1 { > + compatible = "regulator-fixed"; > + regulator-name = "v5.0-sata1"; > + regulator-min-microvolt = <5000000>; > + regulator-max-microvolt = <5000000>; > + regulator-always-on; > + vin-supply = <®_sata1>; > + }; > + > + reg_12v_sata1: v12-sata1 { > + compatible = "regulator-fixed"; > + regulator-name = "v12.0-sata1"; > + regulator-min-microvolt = <12000000>; > + regulator-max-microvolt = <12000000>; > + regulator-always-on; > + vin-supply = <®_sata1>; > + }; > + > + reg_sata2: pwr-sata2 { > + compatible = "regulator-fixed"; > + regulator-name = "pwr_en_sata2"; > + enable-active-high; > + regulator-always-on; > + gpio = <&expander0 11 GPIO_ACTIVE_HIGH>; > + }; > + > + reg_5v_sata2: v5-sata2 { > + compatible = "regulator-fixed"; > + regulator-name = "v5.0-sata2"; > + regulator-min-microvolt = <5000000>; > + regulator-max-microvolt = <5000000>; > + regulator-always-on; > + vin-supply = <®_sata2>; > + }; > + > + reg_12v_sata2: v12-sata2 { > + compatible = "regulator-fixed"; > + regulator-name = "v12.0-sata2"; > + regulator-min-microvolt = <12000000>; > + regulator-max-microvolt = <12000000>; > + regulator-always-on; > + vin-supply = <®_sata2>; > + }; > + > + reg_sata3: pwr-sata3 { > + compatible = "regulator-fixed"; > + regulator-name = "pwr_en_sata3"; > + enable-active-high; > + regulator-always-on; > + gpio = <&expander0 12 GPIO_ACTIVE_HIGH>; > + }; > + > + reg_5v_sata3: v5-sata3 { > + compatible = "regulator-fixed"; > + regulator-name = "v5.0-sata3"; > + regulator-min-microvolt = <5000000>; > + regulator-max-microvolt = <5000000>; > + regulator-always-on; > + vin-supply = <®_sata3>; > + }; > + > + reg_12v_sata3: v12-sata3 { > + compatible = "regulator-fixed"; > + regulator-name = "v12.0-sata3"; > + regulator-min-microvolt = <12000000>; > + regulator-max-microvolt = <12000000>; > + regulator-always-on; > + vin-supply = <®_sata3>; > + }; > }; > > &pinctrl { > Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, On 16/01/2015 09:17, Hans de Goede wrote: > Hi, > > On 15-01-15 15:09, Gregory CLEMENT wrote: >> Add the regulators to each SATA port. >> >> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> >> --- >> arch/arm/boot/dts/armada-388-gp.dts | 126 ++++++++++++++++++++++++++++++++++++ >> 1 file changed, 126 insertions(+) >> >> diff --git a/arch/arm/boot/dts/armada-388-gp.dts b/arch/arm/boot/dts/armada-388-gp.dts >> index 4df22bf91683..590b383db323 100644 >> --- a/arch/arm/boot/dts/armada-388-gp.dts >> +++ b/arch/arm/boot/dts/armada-388-gp.dts >> @@ -173,6 +173,16 @@ >> status = "okay"; >> #address-cells = <1>; >> #size-cells = <0>; >> + >> + sata0: sata-port@0 { >> + reg = <0>; >> + target-supply = <®_5v_sata0>; >> + }; >> + >> + sata1: sata-port@1 { >> + reg = <1>; >> + target-supply = <®_5v_sata1>; >> + }; >> }; >> >> sata@e0000 { >> @@ -181,6 +191,16 @@ >> status = "okay"; >> #address-cells = <1>; >> #size-cells = <0>; >> + >> + sata2: sata-port@0 { >> + reg = <0>; >> + target-supply = <®_5v_sata2>; >> + }; >> + >> + sata3: sata-port@1 { >> + reg = <1>; >> + target-supply = <®_5v_sata3>; >> + }; >> }; >> >> sdhci@d8000 { >> @@ -278,6 +298,112 @@ >> regulator-always-on; >> gpio = <&expander0 4 GPIO_ACTIVE_HIGH>; >> }; >> + >> + reg_sata0: pwr-sata0 { >> + compatible = "regulator-fixed"; >> + regulator-name = "pwr_en_sata0"; >> + enable-active-high; >> + regulator-always-on; >> + >> + }; >> + >> + reg_5v_sata0: v5-sata0 { >> + compatible = "regulator-fixed"; >> + regulator-name = "v5.0-sata0"; >> + regulator-min-microvolt = <5000000>; >> + regulator-max-microvolt = <5000000>; >> + regulator-always-on; >> + vin-supply = <®_sata0>; >> + }; >> + >> + reg_12v_sata0: v12-sata0 { >> + compatible = "regulator-fixed"; >> + regulator-name = "v12.0-sata0"; >> + regulator-min-microvolt = <12000000>; >> + regulator-max-microvolt = <12000000>; >> + regulator-always-on; >> + vin-supply = <®_sata0>; >> + }; > > AFAIK the separate v5 / 12v regulators you're creating here > are not used anywhere. So I guess there just here to > accurately / completely describe the power topology ? Yes it was the point. > >> + >> + reg_sata1: pwr-sata1 { >> + regulator-name = "pwr_en_sata1"; >> + compatible = "regulator-fixed"; >> + regulator-min-microvolt = <12000000>; >> + regulator-max-microvolt = <12000000>; >> + enable-active-high; >> + regulator-always-on; > > > The always on here seems to a bit weird, wasn't the > whole purpose of this patch set to teach ahci_platform > to turn it on as needed ? Maybe I misunderstood the regulator binding, but I thought that (once the suspend will be available on this platform) I could use: regulator-state-mem { regulator-off-in-suspend; }; > > You do probably want to put a regulator-boot-on here so > that disks do not get an unwanted powercycle (bad for > their lifetime) when the firmware has already turned on > the disk. Downside of using regulator-boot-on is that if > the power is not actually turned on no power-on-delay is That's why I didn't use it > 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. Thanks, Gregory > > The same goes for reg_sata2 & reg_sata3. > >> + gpio = <&expander0 3 GPIO_ACTIVE_HIGH>; >> + }; >> + >> + reg_5v_sata1: v5-sata1 { >> + compatible = "regulator-fixed"; >> + regulator-name = "v5.0-sata1"; >> + regulator-min-microvolt = <5000000>; >> + regulator-max-microvolt = <5000000>; >> + regulator-always-on; >> + vin-supply = <®_sata1>; >> + }; >> + >> + reg_12v_sata1: v12-sata1 { >> + compatible = "regulator-fixed"; >> + regulator-name = "v12.0-sata1"; >> + regulator-min-microvolt = <12000000>; >> + regulator-max-microvolt = <12000000>; >> + regulator-always-on; >> + vin-supply = <®_sata1>; >> + }; >> + >> + reg_sata2: pwr-sata2 { >> + compatible = "regulator-fixed"; >> + regulator-name = "pwr_en_sata2"; >> + enable-active-high; >> + regulator-always-on; >> + gpio = <&expander0 11 GPIO_ACTIVE_HIGH>; >> + }; >> + >> + reg_5v_sata2: v5-sata2 { >> + compatible = "regulator-fixed"; >> + regulator-name = "v5.0-sata2"; >> + regulator-min-microvolt = <5000000>; >> + regulator-max-microvolt = <5000000>; >> + regulator-always-on; >> + vin-supply = <®_sata2>; >> + }; >> + >> + reg_12v_sata2: v12-sata2 { >> + compatible = "regulator-fixed"; >> + regulator-name = "v12.0-sata2"; >> + regulator-min-microvolt = <12000000>; >> + regulator-max-microvolt = <12000000>; >> + regulator-always-on; >> + vin-supply = <®_sata2>; >> + }; >> + >> + reg_sata3: pwr-sata3 { >> + compatible = "regulator-fixed"; >> + regulator-name = "pwr_en_sata3"; >> + enable-active-high; >> + regulator-always-on; >> + gpio = <&expander0 12 GPIO_ACTIVE_HIGH>; >> + }; >> + >> + reg_5v_sata3: v5-sata3 { >> + compatible = "regulator-fixed"; >> + regulator-name = "v5.0-sata3"; >> + regulator-min-microvolt = <5000000>; >> + regulator-max-microvolt = <5000000>; >> + regulator-always-on; >> + vin-supply = <®_sata3>; >> + }; >> + >> + reg_12v_sata3: v12-sata3 { >> + compatible = "regulator-fixed"; >> + regulator-name = "v12.0-sata3"; >> + regulator-min-microvolt = <12000000>; >> + regulator-max-microvolt = <12000000>; >> + regulator-always-on; >> + vin-supply = <®_sata3>; >> + }; >> }; >> >> &pinctrl { >> > > Regards, > > Hans >
Hi, On 16-01-15 10:27, Gregory CLEMENT wrote: > Hi Hans, > > On 16/01/2015 09:17, Hans de Goede wrote: >> Hi, >> >> On 15-01-15 15:09, Gregory CLEMENT wrote: >>> Add the regulators to each SATA port. >>> >>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> >>> --- >>> arch/arm/boot/dts/armada-388-gp.dts | 126 ++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 126 insertions(+) >>> >>> diff --git a/arch/arm/boot/dts/armada-388-gp.dts b/arch/arm/boot/dts/armada-388-gp.dts >>> index 4df22bf91683..590b383db323 100644 >>> --- a/arch/arm/boot/dts/armada-388-gp.dts >>> +++ b/arch/arm/boot/dts/armada-388-gp.dts >>> @@ -173,6 +173,16 @@ >>> status = "okay"; >>> #address-cells = <1>; >>> #size-cells = <0>; >>> + >>> + sata0: sata-port@0 { >>> + reg = <0>; >>> + target-supply = <®_5v_sata0>; >>> + }; >>> + >>> + sata1: sata-port@1 { >>> + reg = <1>; >>> + target-supply = <®_5v_sata1>; >>> + }; >>> }; >>> >>> sata@e0000 { >>> @@ -181,6 +191,16 @@ >>> status = "okay"; >>> #address-cells = <1>; >>> #size-cells = <0>; >>> + >>> + sata2: sata-port@0 { >>> + reg = <0>; >>> + target-supply = <®_5v_sata2>; >>> + }; >>> + >>> + sata3: sata-port@1 { >>> + reg = <1>; >>> + target-supply = <®_5v_sata3>; >>> + }; >>> }; >>> >>> sdhci@d8000 { >>> @@ -278,6 +298,112 @@ >>> regulator-always-on; >>> gpio = <&expander0 4 GPIO_ACTIVE_HIGH>; >>> }; >>> + >>> + reg_sata0: pwr-sata0 { >>> + compatible = "regulator-fixed"; >>> + regulator-name = "pwr_en_sata0"; >>> + enable-active-high; >>> + regulator-always-on; >>> + >>> + }; >>> + >>> + reg_5v_sata0: v5-sata0 { >>> + compatible = "regulator-fixed"; >>> + regulator-name = "v5.0-sata0"; >>> + regulator-min-microvolt = <5000000>; >>> + regulator-max-microvolt = <5000000>; >>> + regulator-always-on; >>> + vin-supply = <®_sata0>; >>> + }; >>> + >>> + reg_12v_sata0: v12-sata0 { >>> + compatible = "regulator-fixed"; >>> + regulator-name = "v12.0-sata0"; >>> + regulator-min-microvolt = <12000000>; >>> + regulator-max-microvolt = <12000000>; >>> + regulator-always-on; >>> + vin-supply = <®_sata0>; >>> + }; >> >> AFAIK the separate v5 / 12v regulators you're creating here >> are not used anywhere. So I guess there just here to >> accurately / completely describe the power topology ? > > Yes it was the point. Ok. >>> + >>> + reg_sata1: pwr-sata1 { >>> + regulator-name = "pwr_en_sata1"; >>> + compatible = "regulator-fixed"; >>> + regulator-min-microvolt = <12000000>; >>> + regulator-max-microvolt = <12000000>; >>> + enable-active-high; >>> + regulator-always-on; >> >> >> The always on here seems to a bit weird, wasn't the >> whole purpose of this patch set to teach ahci_platform >> to turn it on as needed ? > > Maybe I misunderstood the regulator binding, but I thought > that (once the suspend will be available on this platform) I > could use: > > regulator-state-mem { > regulator-off-in-suspend; > }; > >> >> You do probably want to put a regulator-boot-on here so >> that disks do not get an unwanted powercycle (bad for >> their lifetime) when the firmware has already turned on >> the disk. Downside of using regulator-boot-on is that if >> the power is not actually turned on no power-on-delay is > > That's why I didn't use it > >> 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. Note that you can test this today by doing (IIRC): echo devices > /sys/power/pm_test echo mem > /sys/power/state This is how I tested ahci_platform suspend handling on the freescale imx processor on the wandboard. 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. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. 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).
Hi Mark and Hans, On 16/01/2015 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. Actually the reg_sata[0-4] are controlled by gpio, so there is a mean to enable/disable them. For the reg_5v_sata[0-4] and reg_12v_sata[0-4] they depend on their respective reg_sata and I just propagated the regulator-always-on, this was maybe a mistake. > >> 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. 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). As pointed by Hans my concern here was be sure that during boot the disk are not power off. In this case which property would be accurate? Thanks, Gregory
On Fri, Jan 16, 2015 at 03:27:00PM +0100, Gregory CLEMENT wrote: > On 16/01/2015 13:37, Mark Brown wrote: > > 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. > Actually the reg_sata[0-4] are controlled by gpio, so there is a mean > to enable/disable them. For the reg_5v_sata[0-4] and reg_12v_sata[0-4] > they depend on their respective reg_sata and I just propagated the > regulator-always-on, this was maybe a mistake. It certainly makes everything confusing if you have control related stuff on regulators that are not directly controllable. > >> 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. 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). > As pointed by Hans my concern here was be sure that during boot the disk > are not power off. In this case which property would be accurate? None, the core won't do anything with the regulator until the end of init anyway.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 16-01-15 16:34, Mark Brown wrote: > On Fri, Jan 16, 2015 at 03:27:00PM +0100, Gregory CLEMENT wrote: >> On 16/01/2015 13:37, Mark Brown wrote: > >>> 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. > >> Actually the reg_sata[0-4] are controlled by gpio, so there is a mean >> to enable/disable them. For the reg_5v_sata[0-4] and reg_12v_sata[0-4] >> they depend on their respective reg_sata and I just propagated the >> regulator-always-on, this was maybe a mistake. > > It certainly makes everything confusing if you have control related > stuff on regulators that are not directly controllable. > >>>> 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. 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). > >> As pointed by Hans my concern here was be sure that during boot the disk >> are not power off. In this case which property would be accurate? > > None, the core won't do anything with the regulator until the end of > init anyway. That us simply not true, see my other mail gpio enabled regulators will be turned off *at register time* unless they have regulator-boot-on set. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 16, 2015 at 08:13:36PM +0100, Hans de Goede wrote: > On 16-01-15 16:34, Mark Brown wrote: > >>As pointed by Hans my concern here was be sure that during boot the disk > >>are not power off. In this case which property would be accurate? > >None, the core won't do anything with the regulator until the end of > >init anyway. > That us simply not true, see my other mail gpio enabled regulators will > be turned off *at register time* unless they have regulator-boot-on set. That's not the core, that's the specific driver (which I didn't know this board happened to be using).
On Fri, Jan 16, 2015 at 08:12:28PM +0100, Hans de Goede wrote: > On 16-01-15 13:37, Mark Brown wrote: > >>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. Right, this always seems like a problem with the GPIO API - not being able to read the state back is a total pain for boot handover, we should really have a way of reading back the current output state. This is one of the few cases where boot-on can be useful, though ideally it'd be set by the bootloader rather than as a static thing since otherwise you have the opposite problem in the case where the disk isn't being used at the current time. > 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. Like I say boot-on is *only* intended to support bootstrapping, if you want the regulator to be always on then mark it as always on. If you want some way of marking the end of userspace init so we can defer cleanup of unused resources then go do that - it's a change I've advocated before, it'd help with modular drivers in general. > 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". No, that's not it at all - it just means that either the hardware didn't support readback or we're working around our own limitations in not being capable of doing that. Remember, regulators are reference counted so with a few exceptions drivers aren't saying "turn this off now" they're saying "this device doesn't need this regulator right now, turn it off if you like" and there's no guarantee that a driver will ever be loaded in the first place.
Hi, On 16-01-15 21:25, Mark Brown wrote: > On Fri, Jan 16, 2015 at 08:12:28PM +0100, Hans de Goede wrote: >> On 16-01-15 13:37, Mark Brown wrote: > >>>> 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. > > Right, this always seems like a problem with the GPIO API - not being > able to read the state back is a total pain for boot handover, we should > really have a way of reading back the current output state. This is one > of the few cases where boot-on can be useful, though ideally it'd be set > by the bootloader rather than as a static thing since otherwise you have > the opposite problem in the case where the disk isn't being used at the > current time. > >> 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. > > Like I say boot-on is *only* intended to support bootstrapping, if you > want the regulator to be always on then mark it as always on. If you > want some way of marking the end of userspace init so we can defer > cleanup of unused resources then go do that - it's a change I've > advocated before, it'd help with modular drivers in general. The problem is there is no clear end of userspace init in modern event driven systems, I think it would be much clearer to simply be consistent and leave these regulators on, rather then turn them of $random time, because as I've advocated before (see the simplefb thread) there is no simple magic moment when it is right to turn things off, so doing it when userspace starts is more or less as good as any moment, and if it turns out that that is not a good moment, then maybe we need to re-think the turning off in general. With that said, since this is a re-occuring theme I've send a mail to the systemd-devel list with you (Mark) in the CC, as well as the ide, arm and devicetree lists. In this mail I'm asking the udev developers if they would be willing to implement some sort of call into the kernel to tell the kernel that udev is done with loading modules. This will only cover devices which are already enumerated at late_init_call time, if we've some slow scanning bus still enumerating stuff in a thread (e.g. usb, scsi) then all bets are of, which is why I'm not enthusiastic about this solution TBH. A simpler solution for the use-case at hand may be to simply disallow building of the ahci_platform driver as a module. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Jan 17, 2015 at 09:48:57AM +0100, Hans de Goede wrote: > and leave these regulators on, rather then turn them of $random time, > because as I've advocated before (see the simplefb thread) there is > no simple magic moment when it is right to turn things off, so doing > it when userspace starts is more or less as good as any moment, and if > it turns out that that is not a good moment, then maybe we need to > re-think the turning off in general. Following your argument to the logical conclusion means we can never turn any regualtor off - we always have the risk that there's another shared user which is going to get a power bounce if we power down. More directly we'll also get people complaining that we're burning power pointlessly on their systems for devices they've not even got drivers enabled for. This powering down is something there's been user demand for.
Hi, On 17-01-15 14:14, Mark Brown wrote: > On Sat, Jan 17, 2015 at 09:48:57AM +0100, Hans de Goede wrote: > >> and leave these regulators on, rather then turn them of $random time, >> because as I've advocated before (see the simplefb thread) there is >> no simple magic moment when it is right to turn things off, so doing >> it when userspace starts is more or less as good as any moment, and if >> it turns out that that is not a good moment, then maybe we need to >> re-think the turning off in general. > > Following your argument to the logical conclusion means we can never > turn any regualtor off - we always have the risk that there's another > shared user which is going to get a power bounce if we power down. More > directly we'll also get people complaining that we're burning power > pointlessly on their systems for devices they've not even got drivers > enabled for. This powering down is something there's been user demand > for. Right, note I'm only advocating to not turn off regulators marked as regulator-boot-on. I would expect any regulator to have such a marking to have at least one user with an actual driver. If people decide to not build that driver, and then complain we can simply tell them to build the driver ... Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Jan 17, 2015 at 03:28:39PM +0100, Hans de Goede wrote: > On 17-01-15 14:14, Mark Brown wrote: > >Following your argument to the logical conclusion means we can never > >turn any regualtor off - we always have the risk that there's another > >shared user which is going to get a power bounce if we power down. More > >directly we'll also get people complaining that we're burning power > >pointlessly on their systems for devices they've not even got drivers > >enabled for. This powering down is something there's been user demand > >for. > Right, note I'm only advocating to not turn off regulators marked as > regulator-boot-on. I would expect any regulator to have such a > marking to have at least one user with an actual driver. If people decide > to not build that driver, and then complain we can simply tell them to > build the driver ... Right, but that's not what regulator-boot-on actually means (and I'm not sure why you would think it would TBH) so this will disrupt existing users who are expecting the current behaviour. We could try adding a new property but it doesn't feel very idiomatic for DT which isn't very nice. Telling people not to build the driver doesn't in general work any better than telling them to build it in I fear, it seems like it's essentially just shuffling things around so people have to change their kernel config in a different way to avoid issues.
Hi, On 18-01-15 13:35, Mark Brown wrote: > On Sat, Jan 17, 2015 at 03:28:39PM +0100, Hans de Goede wrote: >> On 17-01-15 14:14, Mark Brown wrote: > >>> Following your argument to the logical conclusion means we can never >>> turn any regualtor off - we always have the risk that there's another >>> shared user which is going to get a power bounce if we power down. More >>> directly we'll also get people complaining that we're burning power >>> pointlessly on their systems for devices they've not even got drivers >>> enabled for. This powering down is something there's been user demand >>> for. > >> Right, note I'm only advocating to not turn off regulators marked as >> regulator-boot-on. I would expect any regulator to have such a >> marking to have at least one user with an actual driver. If people decide >> to not build that driver, and then complain we can simply tell them to >> build the driver ... > > Right, but that's not what regulator-boot-on actually means (and I'm not > sure why you would think it would TBH) Well, the meaning of regulator-boot-on is not clearly defined really, to begin we need with fixing that, currently all the bindings file says is: - regulator-boot-on: bootloader/firmware enabled regulator One could easily argue that the bootloader likely has a good reason to turn the regulator on, and that unless there is a specific driver which claims the regulator and thus knows what to do with it it is best left alone ... > so this will disrupt existing > users who are expecting the current behaviour. We could try adding a > new property but it doesn't feel very idiomatic for DT which isn't very > nice. > > Telling people not to build the driver doesn't in general work any > better than telling them to build it in I fear, it seems like it's > essentially just shuffling things around so people have to change their > kernel config in a different way to avoid issues. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jan 18, 2015 at 04:29:02PM +0100, Hans de Goede wrote: > On 18-01-15 13:35, Mark Brown wrote: > >Right, but that's not what regulator-boot-on actually means (and I'm not > >sure why you would think it would TBH) > Well, the meaning of regulator-boot-on is not clearly defined really, to > begin we need with fixing that, currently all the bindings file says is: > - regulator-boot-on: bootloader/firmware enabled regulator If that meant anything about what to do with the regulator at runtime it would say so - it means exactly what it says. > One could easily argue that the bootloader likely has a good reason to turn > the regulator on, and that unless there is a specific driver which claims > the regulator and thus knows what to do with it it is best left alone ... That's an excessively big stretch; it doesn't reflect the reality that the bootloader is often just leaving the settings it found on initial power up alone nor the general quality of implementation concerns one often sees with bootloaders. A big use case for this feature is that there is a fairly large class of systems where the bootloader can't be relied on.
diff --git a/arch/arm/boot/dts/armada-388-gp.dts b/arch/arm/boot/dts/armada-388-gp.dts index 4df22bf91683..590b383db323 100644 --- a/arch/arm/boot/dts/armada-388-gp.dts +++ b/arch/arm/boot/dts/armada-388-gp.dts @@ -173,6 +173,16 @@ status = "okay"; #address-cells = <1>; #size-cells = <0>; + + sata0: sata-port@0 { + reg = <0>; + target-supply = <®_5v_sata0>; + }; + + sata1: sata-port@1 { + reg = <1>; + target-supply = <®_5v_sata1>; + }; }; sata@e0000 { @@ -181,6 +191,16 @@ status = "okay"; #address-cells = <1>; #size-cells = <0>; + + sata2: sata-port@0 { + reg = <0>; + target-supply = <®_5v_sata2>; + }; + + sata3: sata-port@1 { + reg = <1>; + target-supply = <®_5v_sata3>; + }; }; sdhci@d8000 { @@ -278,6 +298,112 @@ regulator-always-on; gpio = <&expander0 4 GPIO_ACTIVE_HIGH>; }; + + reg_sata0: pwr-sata0 { + compatible = "regulator-fixed"; + regulator-name = "pwr_en_sata0"; + enable-active-high; + regulator-always-on; + + }; + + reg_5v_sata0: v5-sata0 { + compatible = "regulator-fixed"; + regulator-name = "v5.0-sata0"; + regulator-min-microvolt = <5000000>; + regulator-max-microvolt = <5000000>; + regulator-always-on; + vin-supply = <®_sata0>; + }; + + reg_12v_sata0: v12-sata0 { + compatible = "regulator-fixed"; + regulator-name = "v12.0-sata0"; + regulator-min-microvolt = <12000000>; + regulator-max-microvolt = <12000000>; + regulator-always-on; + vin-supply = <®_sata0>; + }; + + reg_sata1: pwr-sata1 { + regulator-name = "pwr_en_sata1"; + compatible = "regulator-fixed"; + regulator-min-microvolt = <12000000>; + regulator-max-microvolt = <12000000>; + enable-active-high; + regulator-always-on; + gpio = <&expander0 3 GPIO_ACTIVE_HIGH>; + }; + + reg_5v_sata1: v5-sata1 { + compatible = "regulator-fixed"; + regulator-name = "v5.0-sata1"; + regulator-min-microvolt = <5000000>; + regulator-max-microvolt = <5000000>; + regulator-always-on; + vin-supply = <®_sata1>; + }; + + reg_12v_sata1: v12-sata1 { + compatible = "regulator-fixed"; + regulator-name = "v12.0-sata1"; + regulator-min-microvolt = <12000000>; + regulator-max-microvolt = <12000000>; + regulator-always-on; + vin-supply = <®_sata1>; + }; + + reg_sata2: pwr-sata2 { + compatible = "regulator-fixed"; + regulator-name = "pwr_en_sata2"; + enable-active-high; + regulator-always-on; + gpio = <&expander0 11 GPIO_ACTIVE_HIGH>; + }; + + reg_5v_sata2: v5-sata2 { + compatible = "regulator-fixed"; + regulator-name = "v5.0-sata2"; + regulator-min-microvolt = <5000000>; + regulator-max-microvolt = <5000000>; + regulator-always-on; + vin-supply = <®_sata2>; + }; + + reg_12v_sata2: v12-sata2 { + compatible = "regulator-fixed"; + regulator-name = "v12.0-sata2"; + regulator-min-microvolt = <12000000>; + regulator-max-microvolt = <12000000>; + regulator-always-on; + vin-supply = <®_sata2>; + }; + + reg_sata3: pwr-sata3 { + compatible = "regulator-fixed"; + regulator-name = "pwr_en_sata3"; + enable-active-high; + regulator-always-on; + gpio = <&expander0 12 GPIO_ACTIVE_HIGH>; + }; + + reg_5v_sata3: v5-sata3 { + compatible = "regulator-fixed"; + regulator-name = "v5.0-sata3"; + regulator-min-microvolt = <5000000>; + regulator-max-microvolt = <5000000>; + regulator-always-on; + vin-supply = <®_sata3>; + }; + + reg_12v_sata3: v12-sata3 { + compatible = "regulator-fixed"; + regulator-name = "v12.0-sata3"; + regulator-min-microvolt = <12000000>; + regulator-max-microvolt = <12000000>; + regulator-always-on; + vin-supply = <®_sata3>; + }; }; &pinctrl {
Add the regulators to each SATA port. Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> --- arch/arm/boot/dts/armada-388-gp.dts | 126 ++++++++++++++++++++++++++++++++++++ 1 file changed, 126 insertions(+)