| Message ID | 1403164154-1362-1-git-send-email-acourbot@nvidia.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 06/19/2014 01:49 AM, Alexandre Courbot wrote: > Remove the regulator-always-on property from some regulators that do not > need it. On recent kernels fixed regulators which supply is always on > fail registration. That sounds like a bug in the regulator core, which should be fixed there. After all, we can fix *new* DTs to work around that behaviour change, but old DTs are supposed to work with new kernels. Fixing this in the DT doesn't help that. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 19, 2014 at 09:59:04AM -0600, Stephen Warren wrote: > On 06/19/2014 01:49 AM, Alexandre Courbot wrote: > > Remove the regulator-always-on property from some regulators that do not > > need it. On recent kernels fixed regulators which supply is always on > > fail registration. > That sounds like a bug in the regulator core, which should be fixed there. Please actually describe the problem you believe you are seeing - I've seen no reports and I can't tell anything from what you've described, nor can I see any obvious way that a regulator being fixed would have any effect on its supply.
On 06/20/2014 02:56 AM, Mark Brown wrote: > * PGP Signed by an unknown key > > On Thu, Jun 19, 2014 at 09:59:04AM -0600, Stephen Warren wrote: >> On 06/19/2014 01:49 AM, Alexandre Courbot wrote: > >>> Remove the regulator-always-on property from some regulators that do not >>> need it. On recent kernels fixed regulators which supply is always on >>> fail registration. > >> That sounds like a bug in the regulator core, which should be fixed there. > > Please actually describe the problem you believe you are seeing - I've > seen no reports and I can't tell anything from what you've described, > nor can I see any obvious way that a regulator being fixed would have > any effect on its supply. Here is some more information about what happens. We have a fixed regulator defined as follows: vdd_lcd: regulator@2 { compatible = "regulator-fixed"; reg = <2>; regulator-name = "VD_LCD_1V8"; regulator-min-microvolt = <1800000>; regulator-max-microvolt = <1800000>; gpio = <&palmas_gpio 4 GPIO_ACTIVE_HIGH>; enable-active-high; vin-supply = <&vdd_1v8>; regulator-boot-on; }; Its vin-supply is part of the palmas device: vdd_1v8: smps8 { regulator-name = "vs-pmu-1v8"; regulator-min-microvolt = <1800000>; regulator-max-microvolt = <1800000>; regulator-always-on; regulator-boot-on; }; When vdd_lcd is registered, set_supply() is called, which creates a new regulator for vdd_1v8. In create_regulator(), _regulator_can_change_status() returns false (as it should since the regulator is always_on) and _regulator_is_enabled() *also* returns false, so as a result regulator->always_on remains false for vdd_1v8. Later in regulator_register(), we try to enable the supply. Since regulator->always_on is false, _regulator_enable() is called on vdd_1v8, and the pair _regulator_is_enabled() / _regulator_can_change_status() is called again with the same result, which causes _regulator_enable() to return -EPERM. This prevents vdd_lcd from being registered. So I can see three questions here: 1) Why does _regulator_enable() on vdd_1v8 return 0 while everything suggests that it is enabled (this regulator powers lot of devices, like eMMC, which are working fine). This may be an issue with the palmas driver. 2) When an always-on regulator that is not yet enabled is registered, shouldn't it be switched on by the regulator framework? 3) When a boot-on regulator is registered and _regulator_is_enabled() returns contradictory information, what should be done? Note that whether the regulator-boot-on property is present or not does not change anything. I tried to find a recent patch that could have introduced a change of behavior, but could not find anything so far. Bisecting is made harder by the fact this happens on a newly-introduced board which requires a bunch of patches of its own, but it we need more information I can try to do it anyway. Thanks, Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/20/2014 02:26 PM, Alexandre Courbot wrote: > On 06/20/2014 02:56 AM, Mark Brown wrote: >> * PGP Signed by an unknown key >> >> On Thu, Jun 19, 2014 at 09:59:04AM -0600, Stephen Warren wrote: >>> On 06/19/2014 01:49 AM, Alexandre Courbot wrote: >> >>>> Remove the regulator-always-on property from some regulators that do >>>> not >>>> need it. On recent kernels fixed regulators which supply is always on >>>> fail registration. >> >>> That sounds like a bug in the regulator core, which should be fixed >>> there. >> >> Please actually describe the problem you believe you are seeing - I've >> seen no reports and I can't tell anything from what you've described, >> nor can I see any obvious way that a regulator being fixed would have >> any effect on its supply. > > Here is some more information about what happens. > > We have a fixed regulator defined as follows: > > vdd_lcd: regulator@2 { > compatible = "regulator-fixed"; > reg = <2>; > regulator-name = "VD_LCD_1V8"; > regulator-min-microvolt = <1800000>; > regulator-max-microvolt = <1800000>; > gpio = <&palmas_gpio 4 GPIO_ACTIVE_HIGH>; > enable-active-high; > vin-supply = <&vdd_1v8>; > regulator-boot-on; > }; > > Its vin-supply is part of the palmas device: > > vdd_1v8: smps8 { > regulator-name = "vs-pmu-1v8"; > regulator-min-microvolt = <1800000>; > regulator-max-microvolt = <1800000>; > regulator-always-on; > regulator-boot-on; > }; > > When vdd_lcd is registered, set_supply() is called, which creates a new > regulator for vdd_1v8. In create_regulator(), > _regulator_can_change_status() returns false (as it should since the > regulator is always_on) and _regulator_is_enabled() *also* returns > false, so as a result regulator->always_on remains false for vdd_1v8. > > Later in regulator_register(), we try to enable the supply. Since > regulator->always_on is false, _regulator_enable() is called on vdd_1v8, > and the pair _regulator_is_enabled() / _regulator_can_change_status() is > called again with the same result, which causes _regulator_enable() to > return -EPERM. This prevents vdd_lcd from being registered. > > So I can see three questions here: > > 1) Why does _regulator_enable() on vdd_1v8 return 0 while everything > suggests that it is enabled (this regulator powers lot of devices, like > eMMC, which are working fine). This may be an issue with the palmas driver. Ran a bisect eventually, found that reverting this commit led to SMPS8's enabled status to be properly reported at boot time (and consequently the register probe to succeed): dbabd624d regulator: palmas: Reemove open coded functions with helper functions Keerthy, Nishanth, could it be that there is still something wrong with the REGULATOR_LINEAR_RANGE() definitions? This seems to be the cause for our trouble, but the other questions might still stand, in case there is interest in discussing them. > > 2) When an always-on regulator that is not yet enabled is registered, > shouldn't it be switched on by the regulator framework? > > 3) When a boot-on regulator is registered and _regulator_is_enabled() > returns contradictory information, what should be done? > > Note that whether the regulator-boot-on property is present or not does > not change anything. > > I tried to find a recent patch that could have introduced a change of > behavior, but could not find anything so far. Bisecting is made harder > by the fact this happens on a newly-introduced board which requires a > bunch of patches of its own, but it we need more information I can try > to do it anyway. > > Thanks, > Alex. > -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 20, 2014 at 03:44:46PM +0900, Alexandre Courbot wrote: > dbabd624d > regulator: palmas: Reemove open coded functions with helper functions > Keerthy, Nishanth, could it be that there is still something wrong with the > REGULATOR_LINEAR_RANGE() definitions? > This seems to be the cause for our trouble, but the other questions might > still stand, in case there is interest in discussing them. There was a bug fix to the Palmas driver which just went to Linus the other day, are you sure this isn't fixed in mainline (or -next, it's been in -next for a week or something)?
On 06/20/2014 06:41 PM, Mark Brown wrote: > * PGP Signed by an unknown key > > On Fri, Jun 20, 2014 at 03:44:46PM +0900, Alexandre Courbot wrote: > >> dbabd624d >> regulator: palmas: Reemove open coded functions with helper functions > >> Keerthy, Nishanth, could it be that there is still something wrong with the >> REGULATOR_LINEAR_RANGE() definitions? > >> This seems to be the cause for our trouble, but the other questions might >> still stand, in case there is interest in discussing them. > > There was a bug fix to the Palmas driver which just went to Linus the > other day, are you sure this isn't fixed in mainline (or -next, it's > been in -next for a week or something)? If you are talking about 6b7f2d82d5 regulator: palmas: Fix SMPS list for 0V then it is in my tree. There is actually no difference on palmas-regulator.c between my tree and the current -next (or Linus' tree for that instance). So it seems to be something else we are dealing with here. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 20, 2014 at 02:26:43PM +0900, Alexandre Courbot wrote: > So I can see three questions here: > 1) Why does _regulator_enable() on vdd_1v8 return 0 while everything > suggests that it is enabled (this regulator powers lot of devices, like > eMMC, which are working fine). This may be an issue with the palmas driver. Returning 0 is reporting success and you say it is enabled so I'm not seeing any contradiction here... or do you mean regulator_is_enabled() here? An always on regulator should report that it is enabled. > 2) When an always-on regulator that is not yet enabled is registered, > shouldn't it be switched on by the regulator framework? This happens during set_machine_constraints(). > 3) When a boot-on regulator is registered and _regulator_is_enabled() > returns contradictory information, what should be done? Same thing here (same check even).
On 06/20/2014 07:16 PM, Mark Brown wrote: > * PGP Signed by an unknown key > > On Fri, Jun 20, 2014 at 02:26:43PM +0900, Alexandre Courbot wrote: > >> So I can see three questions here: > >> 1) Why does _regulator_enable() on vdd_1v8 return 0 while everything >> suggests that it is enabled (this regulator powers lot of devices, like >> eMMC, which are working fine). This may be an issue with the palmas driver. > > Returning 0 is reporting success and you say it is enabled so I'm not > seeing any contradiction here... or do you mean regulator_is_enabled() > here? An always on regulator should report that it is enabled. I meant to write regulator_is_enabled() here, yes. Apologies for the confusion. > >> 2) When an always-on regulator that is not yet enabled is registered, >> shouldn't it be switched on by the regulator framework? > > This happens during set_machine_constraints(). > >> 3) When a boot-on regulator is registered and _regulator_is_enabled() >> returns contradictory information, what should be done? > > Same thing here (same check even). Indeed, everything is coherent here - I was confused by the fact the regulator was always reported as disabled, which led me to wrongly assume that the framework did not try to enable it. But of course we cannot ask the framework to account for drivers that return incorrect regulator state, as seems to be the case with Palmas here. So it all seems to come down to a bug in the Palmas driver introduced by dbabd624d. Reverting that change brings me back to normal. Thanks, Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" 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/arch/arm/boot/dts/tegra114-tn7.dts b/arch/arm/boot/dts/tegra114-tn7.dts index 963662145635..abcfe77c3a99 100644 --- a/arch/arm/boot/dts/tegra114-tn7.dts +++ b/arch/arm/boot/dts/tegra114-tn7.dts @@ -114,7 +114,6 @@ regulator-name = "vs-pmu-1v8"; regulator-min-microvolt = <1800000>; regulator-max-microvolt = <1800000>; - regulator-always-on; regulator-boot-on; }; @@ -130,7 +129,6 @@ regulator-name = "vd-smps10-out1"; regulator-min-microvolt = <5000000>; regulator-max-microvolt = <5000000>; - regulator-always-on; regulator-boot-on; }; @@ -138,7 +136,6 @@ regulator-name = "vd-smps10-out2"; regulator-min-microvolt = <5000000>; regulator-max-microvolt = <5000000>; - regulator-always-on; regulator-boot-on; };
Remove the regulator-always-on property from some regulators that do not need it. On recent kernels fixed regulators which supply is always on fail registration. Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> --- Stephen, do you think you could queue this for 3.16-rc2 or 3? Without this the TN7 panel does not show up which makes the device virtually unusable. arch/arm/boot/dts/tegra114-tn7.dts | 3 --- 1 file changed, 3 deletions(-)