* [PATCH] ARM: tegra: TN7: relax some regulators @ 2014-06-19 7:49 ` Alexandre Courbot 0 siblings, 0 replies; 22+ messages in thread From: Alexandre Courbot @ 2014-06-19 7:49 UTC (permalink / raw) To: Stephen Warren, Thierry Reding Cc: linux-tegra, linux-kernel, Alexandre Courbot 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(-) 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; }; -- 2.0.0 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH] ARM: tegra: TN7: relax some regulators @ 2014-06-19 7:49 ` Alexandre Courbot 0 siblings, 0 replies; 22+ messages in thread From: Alexandre Courbot @ 2014-06-19 7:49 UTC (permalink / raw) To: Stephen Warren, Thierry Reding Cc: linux-tegra, linux-kernel, Alexandre Courbot 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(-) 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; }; -- 2.0.0 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] ARM: tegra: TN7: relax some regulators 2014-06-19 7:49 ` Alexandre Courbot (?) @ 2014-06-19 15:59 ` Stephen Warren 2014-06-19 17:56 ` Mark Brown -1 siblings, 1 reply; 22+ messages in thread From: Stephen Warren @ 2014-06-19 15:59 UTC (permalink / raw) To: Alexandre Courbot, Thierry Reding; +Cc: linux-tegra, linux-kernel, Mark Brown 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ARM: tegra: TN7: relax some regulators 2014-06-19 15:59 ` Stephen Warren @ 2014-06-19 17:56 ` Mark Brown [not found] ` <20140619175643.GR5099-GFdadSzt00ze9xe1eoZjHA@public.gmane.org> 0 siblings, 1 reply; 22+ messages in thread From: Mark Brown @ 2014-06-19 17:56 UTC (permalink / raw) To: Stephen Warren Cc: Alexandre Courbot, Thierry Reding, linux-tegra, linux-kernel [-- Attachment #1: Type: text/plain, Size: 606 bytes --] 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. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 22+ messages in thread
[parent not found: <20140619175643.GR5099-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>]
* Re: [PATCH] ARM: tegra: TN7: relax some regulators 2014-06-19 17:56 ` Mark Brown @ 2014-06-20 5:26 ` Alexandre Courbot 0 siblings, 0 replies; 22+ messages in thread From: Alexandre Courbot @ 2014-06-20 5:26 UTC (permalink / raw) To: Mark Brown, Stephen Warren Cc: Thierry Reding, linux-tegra-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ARM: tegra: TN7: relax some regulators @ 2014-06-20 5:26 ` Alexandre Courbot 0 siblings, 0 replies; 22+ messages in thread From: Alexandre Courbot @ 2014-06-20 5:26 UTC (permalink / raw) To: Mark Brown, Stephen Warren; +Cc: Thierry Reding, linux-tegra, linux-kernel 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
[parent not found: <53A3C613.8030207-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]
* Re: [PATCH] ARM: tegra: TN7: relax some regulators 2014-06-20 5:26 ` Alexandre Courbot @ 2014-06-20 6:44 ` Alexandre Courbot -1 siblings, 0 replies; 22+ messages in thread From: Alexandre Courbot @ 2014-06-20 6:44 UTC (permalink / raw) To: Mark Brown, Stephen Warren, Keerthy, Nishanth Menon Cc: Thierry Reding, linux-tegra-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA 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. > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ARM: tegra: TN7: relax some regulators @ 2014-06-20 6:44 ` Alexandre Courbot 0 siblings, 0 replies; 22+ messages in thread From: Alexandre Courbot @ 2014-06-20 6:44 UTC (permalink / raw) To: Mark Brown, Stephen Warren, Keerthy, Nishanth Menon Cc: Thierry Reding, linux-tegra, linux-kernel 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. > ^ permalink raw reply [flat|nested] 22+ messages in thread
[parent not found: <53A3D85E.7030704-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]
* Re: [PATCH] ARM: tegra: TN7: relax some regulators 2014-06-20 6:44 ` Alexandre Courbot @ 2014-06-20 9:41 ` Mark Brown -1 siblings, 0 replies; 22+ messages in thread From: Mark Brown @ 2014-06-20 9:41 UTC (permalink / raw) To: Alexandre Courbot Cc: Stephen Warren, Keerthy, Nishanth Menon, Thierry Reding, linux-tegra-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA [-- Attachment #1: Type: text/plain, Size: 589 bytes --] 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)? [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ARM: tegra: TN7: relax some regulators @ 2014-06-20 9:41 ` Mark Brown 0 siblings, 0 replies; 22+ messages in thread From: Mark Brown @ 2014-06-20 9:41 UTC (permalink / raw) To: Alexandre Courbot Cc: Stephen Warren, Keerthy, Nishanth Menon, Thierry Reding, linux-tegra, linux-kernel [-- Attachment #1: Type: text/plain, Size: 589 bytes --] 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)? [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 22+ messages in thread
[parent not found: <20140620094119.GT5099-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>]
* Re: [PATCH] ARM: tegra: TN7: relax some regulators 2014-06-20 9:41 ` Mark Brown @ 2014-06-20 9:49 ` Alexandre Courbot -1 siblings, 0 replies; 22+ messages in thread From: Alexandre Courbot @ 2014-06-20 9:49 UTC (permalink / raw) To: Mark Brown Cc: Stephen Warren, Keerthy, Nishanth Menon, Thierry Reding, linux-tegra-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ARM: tegra: TN7: relax some regulators @ 2014-06-20 9:49 ` Alexandre Courbot 0 siblings, 0 replies; 22+ messages in thread From: Alexandre Courbot @ 2014-06-20 9:49 UTC (permalink / raw) To: Mark Brown Cc: Stephen Warren, Keerthy, Nishanth Menon, Thierry Reding, linux-tegra, linux-kernel 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Palmas regulator broken (was Re: [PATCH] ARM: tegra: TN7: relax some regulators) 2014-06-20 9:49 ` Alexandre Courbot @ 2014-06-20 13:23 ` Nishanth Menon -1 siblings, 0 replies; 22+ messages in thread From: Nishanth Menon @ 2014-06-20 13:23 UTC (permalink / raw) To: Alexandre Courbot Cc: Mark Brown, Stephen Warren, Keerthy, Thierry Reding, linux-tegra, linux-kernel, linux-omap + l-o, http://marc.info/?t=140316427500004&r=1&w=2 full thread Minor change in subject to indicate palmas regulator fail On 18:49-20140620, Alexandre Courbot wrote: > 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. Your quote earlier in the thread " _regulator_is_enabled() *also* returns false " Got me curious. Looking at the patch: dbabd624d4eec50b623bab070d1e39a854b2d65c (regulator: palmas: Reemove open coded functions with helper functions) I noticed the following change palmas_is_enabled_smps -> regulator_is_enabled_regmap So I decided to search for enable_reg in palmas-regulator.c and I think it needs valid enable_reg, mask, value for regulator_is_enabled_regmap to work :). Maybe to be sure, we could print the following: PALMAS_SMPS8_VOLTAGE, PALMAS_SMPS8_CTRL, PALMAS_SMPS8_TSTEP, Anyways, I quickly boot tested the following on DRA7evm (which also uses Palmas): [ 1.933939] palmas-pmic 48070000.i2c:tps659038@58:tps659038_pmic: enable_reg = 0x00, mask =0x00 [ 1.944210] smps123: 850 <--> 1250 mV at 1060 mV [ 1.950717] palmas-pmic 48070000.i2c:tps659038@58:tps659038_pmic: enable_reg = 0x00, mask =0x00 [ 1.960754] smps45: 850 <--> 1150 mV at 1060 mV [ 1.967048] palmas-pmic 48070000.i2c:tps659038@58:tps659038_pmic: enable_reg = 0x00, mask =0x00 [ 1.977072] smps6: 850 <--> 1650 mV at 1060 mV [ 1.983077] palmas-pmic 48070000.i2c:tps659038@58:tps659038_pmic: enable_reg = 0x00, mask =0x00 [ 1.992994] smps7: 850 <--> 1030 mV at 1030 mV [ 1.999238] palmas-pmic 48070000.i2c:tps659038@58:tps659038_pmic: enable_reg = 0x00, mask =0x00 [ 2.009161] smps8: 850 <--> 1250 mV at 1060 mV [ 2.015304] palmas-pmic 48070000.i2c:tps659038@58:tps659038_pmic: enable_reg = 0x00, mask =0x00 It does seem to me that either set_mode also should use core functions OR you still need a palmas specific is_enable, enable/disable functions (contrary to the claim of the patch in question - which I think introduced regressions). Otherwise, completely untested diff below - can you give this a shot? diff --git a/drivers/regulator/palmas-regulator.c b/drivers/regulator/palmas-regulator.c index b982f0f..bbfe22f 100644 --- a/drivers/regulator/palmas-regulator.c +++ b/drivers/regulator/palmas-regulator.c @@ -964,6 +964,20 @@ static int palmas_regulators_probe(struct platform_device *pdev) return ret; pmic->current_reg_mode[id] = reg & PALMAS_SMPS12_CTRL_MODE_ACTIVE_MASK; + + dev_err(&pdev->dev, "enable_reg = 0x%02x, mask =0x%02x\n", + pmic->desc[id].enable_reg, + pmic->desc[id].enable_mask); + pmic->desc[id].enable_reg = + PALMAS_BASE_TO_REG(PALMAS_LDO_BASE, + palmas_regs_info[id].ctrl_addr); + pmic->desc[id].enable_mask = + PALMAS_SMPS12_CTRL_MODE_ACTIVE_MASK; + /* + * The following completely ignores + * pmic->current_reg_mode[id] (set_mode) + */ + pmic->desc[id].enable_val = SMPS_CTRL_MODE_ON; } pmic->desc[id].type = REGULATOR_VOLTAGE; -- Regards, Nishanth Menon ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Palmas regulator broken (was Re: [PATCH] ARM: tegra: TN7: relax some regulators) @ 2014-06-20 13:23 ` Nishanth Menon 0 siblings, 0 replies; 22+ messages in thread From: Nishanth Menon @ 2014-06-20 13:23 UTC (permalink / raw) To: Alexandre Courbot Cc: Mark Brown, Stephen Warren, Keerthy, Thierry Reding, linux-tegra, linux-kernel, linux-omap + l-o, http://marc.info/?t=140316427500004&r=1&w=2 full thread Minor change in subject to indicate palmas regulator fail On 18:49-20140620, Alexandre Courbot wrote: > 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. Your quote earlier in the thread " _regulator_is_enabled() *also* returns false " Got me curious. Looking at the patch: dbabd624d4eec50b623bab070d1e39a854b2d65c (regulator: palmas: Reemove open coded functions with helper functions) I noticed the following change palmas_is_enabled_smps -> regulator_is_enabled_regmap So I decided to search for enable_reg in palmas-regulator.c and I think it needs valid enable_reg, mask, value for regulator_is_enabled_regmap to work :). Maybe to be sure, we could print the following: PALMAS_SMPS8_VOLTAGE, PALMAS_SMPS8_CTRL, PALMAS_SMPS8_TSTEP, Anyways, I quickly boot tested the following on DRA7evm (which also uses Palmas): [ 1.933939] palmas-pmic 48070000.i2c:tps659038@58:tps659038_pmic: enable_reg = 0x00, mask =0x00 [ 1.944210] smps123: 850 <--> 1250 mV at 1060 mV [ 1.950717] palmas-pmic 48070000.i2c:tps659038@58:tps659038_pmic: enable_reg = 0x00, mask =0x00 [ 1.960754] smps45: 850 <--> 1150 mV at 1060 mV [ 1.967048] palmas-pmic 48070000.i2c:tps659038@58:tps659038_pmic: enable_reg = 0x00, mask =0x00 [ 1.977072] smps6: 850 <--> 1650 mV at 1060 mV [ 1.983077] palmas-pmic 48070000.i2c:tps659038@58:tps659038_pmic: enable_reg = 0x00, mask =0x00 [ 1.992994] smps7: 850 <--> 1030 mV at 1030 mV [ 1.999238] palmas-pmic 48070000.i2c:tps659038@58:tps659038_pmic: enable_reg = 0x00, mask =0x00 [ 2.009161] smps8: 850 <--> 1250 mV at 1060 mV [ 2.015304] palmas-pmic 48070000.i2c:tps659038@58:tps659038_pmic: enable_reg = 0x00, mask =0x00 It does seem to me that either set_mode also should use core functions OR you still need a palmas specific is_enable, enable/disable functions (contrary to the claim of the patch in question - which I think introduced regressions). Otherwise, completely untested diff below - can you give this a shot? diff --git a/drivers/regulator/palmas-regulator.c b/drivers/regulator/palmas-regulator.c index b982f0f..bbfe22f 100644 --- a/drivers/regulator/palmas-regulator.c +++ b/drivers/regulator/palmas-regulator.c @@ -964,6 +964,20 @@ static int palmas_regulators_probe(struct platform_device *pdev) return ret; pmic->current_reg_mode[id] = reg & PALMAS_SMPS12_CTRL_MODE_ACTIVE_MASK; + + dev_err(&pdev->dev, "enable_reg = 0x%02x, mask =0x%02x\n", + pmic->desc[id].enable_reg, + pmic->desc[id].enable_mask); + pmic->desc[id].enable_reg = + PALMAS_BASE_TO_REG(PALMAS_LDO_BASE, + palmas_regs_info[id].ctrl_addr); + pmic->desc[id].enable_mask = + PALMAS_SMPS12_CTRL_MODE_ACTIVE_MASK; + /* + * The following completely ignores + * pmic->current_reg_mode[id] (set_mode) + */ + pmic->desc[id].enable_val = SMPS_CTRL_MODE_ON; } pmic->desc[id].type = REGULATOR_VOLTAGE; -- Regards, Nishanth Menon ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: Palmas regulator broken (was Re: [PATCH] ARM: tegra: TN7: relax some regulators) 2014-06-20 13:23 ` Nishanth Menon @ 2014-06-20 13:54 ` Nishanth Menon -1 siblings, 0 replies; 22+ messages in thread From: Nishanth Menon @ 2014-06-20 13:54 UTC (permalink / raw) To: Alexandre Courbot Cc: Mark Brown, Stephen Warren, Keerthy, Thierry Reding, linux-tegra, linux-kernel, linux-omap On 08:23-20140620, Nishanth Menon wrote: > + l-o, > http://marc.info/?t=140316427500004&r=1&w=2 full thread > > Minor change in subject to indicate palmas regulator fail > > On 18:49-20140620, Alexandre Courbot wrote: > > 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. > > Your quote earlier in the thread > " > _regulator_is_enabled() *also* returns false > " > > Got me curious. Looking at the patch: > dbabd624d4eec50b623bab070d1e39a854b2d65c (regulator: palmas: Reemove > open coded functions with helper functions) > I noticed the following change > palmas_is_enabled_smps -> regulator_is_enabled_regmap > > So I decided to search for enable_reg in palmas-regulator.c and I think > it needs valid enable_reg, mask, value for regulator_is_enabled_regmap to work > :). > > Maybe to be sure, we could print the following: > PALMAS_SMPS8_VOLTAGE, PALMAS_SMPS8_CTRL, PALMAS_SMPS8_TSTEP, > > Anyways, I quickly boot tested the following on DRA7evm (which also uses Palmas): > [ 1.933939] palmas-pmic 48070000.i2c:tps659038@58:tps659038_pmic: enable_reg = 0x00, mask =0x00 > [ 1.944210] smps123: 850 <--> 1250 mV at 1060 mV > [ 1.950717] palmas-pmic 48070000.i2c:tps659038@58:tps659038_pmic: enable_reg = 0x00, mask =0x00 > [ 1.960754] smps45: 850 <--> 1150 mV at 1060 mV > [ 1.967048] palmas-pmic 48070000.i2c:tps659038@58:tps659038_pmic: enable_reg = 0x00, mask =0x00 > [ 1.977072] smps6: 850 <--> 1650 mV at 1060 mV > [ 1.983077] palmas-pmic 48070000.i2c:tps659038@58:tps659038_pmic: enable_reg = 0x00, mask =0x00 > [ 1.992994] smps7: 850 <--> 1030 mV at 1030 mV > [ 1.999238] palmas-pmic 48070000.i2c:tps659038@58:tps659038_pmic: enable_reg = 0x00, mask =0x00 > [ 2.009161] smps8: 850 <--> 1250 mV at 1060 mV > [ 2.015304] palmas-pmic 48070000.i2c:tps659038@58:tps659038_pmic: enable_reg = 0x00, mask =0x00 > > It does seem to me that either set_mode also should use core functions > OR you still need a palmas specific is_enable, enable/disable functions > (contrary to the claim of the patch in question - which I think > introduced regressions). > > Otherwise, completely untested diff below - can you give this a shot? > > diff --git a/drivers/regulator/palmas-regulator.c b/drivers/regulator/palmas-regulator.c > index b982f0f..bbfe22f 100644 > --- a/drivers/regulator/palmas-regulator.c > +++ b/drivers/regulator/palmas-regulator.c > @@ -964,6 +964,20 @@ static int palmas_regulators_probe(struct platform_device *pdev) > return ret; > pmic->current_reg_mode[id] = reg & > PALMAS_SMPS12_CTRL_MODE_ACTIVE_MASK; > + > + dev_err(&pdev->dev, "enable_reg = 0x%02x, mask =0x%02x\n", > + pmic->desc[id].enable_reg, > + pmic->desc[id].enable_mask); > + pmic->desc[id].enable_reg = > + PALMAS_BASE_TO_REG(PALMAS_LDO_BASE, > + palmas_regs_info[id].ctrl_addr); > + pmic->desc[id].enable_mask = > + PALMAS_SMPS12_CTRL_MODE_ACTIVE_MASK; > + /* > + * The following completely ignores > + * pmic->current_reg_mode[id] (set_mode) > + */ > + pmic->desc[id].enable_val = SMPS_CTRL_MODE_ON; > } > > pmic->desc[id].type = REGULATOR_VOLTAGE; rev 2 of the diff - this does depened on the fact that regulator_desc is not memdup-ed by regulator code - that lets us do a bit of a trickery ;) - and I dropped the prints.. Unrelated: This makes me wonder why palmas_is_enabled_ldo at all? Keerthy, Mark, what do you think of the following (esp the flip of desc value): diff --git a/drivers/regulator/palmas-regulator.c b/drivers/regulator/palmas-regulator.c index b982f0f..f01d9c5 100644 --- a/drivers/regulator/palmas-regulator.c +++ b/drivers/regulator/palmas-regulator.c @@ -299,7 +299,7 @@ static int palmas_set_mode_smps(struct regulator_dev *dev, unsigned int mode) struct palmas_pmic *pmic = rdev_get_drvdata(dev); int id = rdev_get_id(dev); unsigned int reg; - bool rail_enable = true; + bool rail_enable = true, enable_val = true; palmas_smps_read(pmic->palmas, palmas_regs_info[id].ctrl_addr, ®); reg &= ~PALMAS_SMPS12_CTRL_MODE_ACTIVE_MASK; @@ -318,6 +318,7 @@ static int palmas_set_mode_smps(struct regulator_dev *dev, unsigned int mode) reg |= SMPS_CTRL_MODE_PWM; break; default: + enable_val = false; return -EINVAL; } @@ -325,6 +326,11 @@ static int palmas_set_mode_smps(struct regulator_dev *dev, unsigned int mode) if (rail_enable) palmas_smps_write(pmic->palmas, palmas_regs_info[id].ctrl_addr, reg); + + /* Switch the enable value to ensure this is used for enable */ + if (enable_val) + pmic->desc[id].enable_val = pmic->current_reg_mode[id]; + return 0; } @@ -964,6 +970,14 @@ static int palmas_regulators_probe(struct platform_device *pdev) return ret; pmic->current_reg_mode[id] = reg & PALMAS_SMPS12_CTRL_MODE_ACTIVE_MASK; + + pmic->desc[id].enable_reg = + PALMAS_BASE_TO_REG(PALMAS_LDO_BASE, + palmas_regs_info[id].ctrl_addr); + pmic->desc[id].enable_mask = + PALMAS_SMPS12_CTRL_MODE_ACTIVE_MASK; + /* set_mode overrides this value */ + pmic->desc[id].enable_val = SMPS_CTRL_MODE_ON; } pmic->desc[id].type = REGULATOR_VOLTAGE; -- Regards, Nishanth Menon ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: Palmas regulator broken (was Re: [PATCH] ARM: tegra: TN7: relax some regulators) @ 2014-06-20 13:54 ` Nishanth Menon 0 siblings, 0 replies; 22+ messages in thread From: Nishanth Menon @ 2014-06-20 13:54 UTC (permalink / raw) To: Alexandre Courbot Cc: Mark Brown, Stephen Warren, Keerthy, Thierry Reding, linux-tegra, linux-kernel, linux-omap On 08:23-20140620, Nishanth Menon wrote: > + l-o, > http://marc.info/?t=140316427500004&r=1&w=2 full thread > > Minor change in subject to indicate palmas regulator fail > > On 18:49-20140620, Alexandre Courbot wrote: > > 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. > > Your quote earlier in the thread > " > _regulator_is_enabled() *also* returns false > " > > Got me curious. Looking at the patch: > dbabd624d4eec50b623bab070d1e39a854b2d65c (regulator: palmas: Reemove > open coded functions with helper functions) > I noticed the following change > palmas_is_enabled_smps -> regulator_is_enabled_regmap > > So I decided to search for enable_reg in palmas-regulator.c and I think > it needs valid enable_reg, mask, value for regulator_is_enabled_regmap to work > :). > > Maybe to be sure, we could print the following: > PALMAS_SMPS8_VOLTAGE, PALMAS_SMPS8_CTRL, PALMAS_SMPS8_TSTEP, > > Anyways, I quickly boot tested the following on DRA7evm (which also uses Palmas): > [ 1.933939] palmas-pmic 48070000.i2c:tps659038@58:tps659038_pmic: enable_reg = 0x00, mask =0x00 > [ 1.944210] smps123: 850 <--> 1250 mV at 1060 mV > [ 1.950717] palmas-pmic 48070000.i2c:tps659038@58:tps659038_pmic: enable_reg = 0x00, mask =0x00 > [ 1.960754] smps45: 850 <--> 1150 mV at 1060 mV > [ 1.967048] palmas-pmic 48070000.i2c:tps659038@58:tps659038_pmic: enable_reg = 0x00, mask =0x00 > [ 1.977072] smps6: 850 <--> 1650 mV at 1060 mV > [ 1.983077] palmas-pmic 48070000.i2c:tps659038@58:tps659038_pmic: enable_reg = 0x00, mask =0x00 > [ 1.992994] smps7: 850 <--> 1030 mV at 1030 mV > [ 1.999238] palmas-pmic 48070000.i2c:tps659038@58:tps659038_pmic: enable_reg = 0x00, mask =0x00 > [ 2.009161] smps8: 850 <--> 1250 mV at 1060 mV > [ 2.015304] palmas-pmic 48070000.i2c:tps659038@58:tps659038_pmic: enable_reg = 0x00, mask =0x00 > > It does seem to me that either set_mode also should use core functions > OR you still need a palmas specific is_enable, enable/disable functions > (contrary to the claim of the patch in question - which I think > introduced regressions). > > Otherwise, completely untested diff below - can you give this a shot? > > diff --git a/drivers/regulator/palmas-regulator.c b/drivers/regulator/palmas-regulator.c > index b982f0f..bbfe22f 100644 > --- a/drivers/regulator/palmas-regulator.c > +++ b/drivers/regulator/palmas-regulator.c > @@ -964,6 +964,20 @@ static int palmas_regulators_probe(struct platform_device *pdev) > return ret; > pmic->current_reg_mode[id] = reg & > PALMAS_SMPS12_CTRL_MODE_ACTIVE_MASK; > + > + dev_err(&pdev->dev, "enable_reg = 0x%02x, mask =0x%02x\n", > + pmic->desc[id].enable_reg, > + pmic->desc[id].enable_mask); > + pmic->desc[id].enable_reg = > + PALMAS_BASE_TO_REG(PALMAS_LDO_BASE, > + palmas_regs_info[id].ctrl_addr); > + pmic->desc[id].enable_mask = > + PALMAS_SMPS12_CTRL_MODE_ACTIVE_MASK; > + /* > + * The following completely ignores > + * pmic->current_reg_mode[id] (set_mode) > + */ > + pmic->desc[id].enable_val = SMPS_CTRL_MODE_ON; > } > > pmic->desc[id].type = REGULATOR_VOLTAGE; rev 2 of the diff - this does depened on the fact that regulator_desc is not memdup-ed by regulator code - that lets us do a bit of a trickery ;) - and I dropped the prints.. Unrelated: This makes me wonder why palmas_is_enabled_ldo at all? Keerthy, Mark, what do you think of the following (esp the flip of desc value): diff --git a/drivers/regulator/palmas-regulator.c b/drivers/regulator/palmas-regulator.c index b982f0f..f01d9c5 100644 --- a/drivers/regulator/palmas-regulator.c +++ b/drivers/regulator/palmas-regulator.c @@ -299,7 +299,7 @@ static int palmas_set_mode_smps(struct regulator_dev *dev, unsigned int mode) struct palmas_pmic *pmic = rdev_get_drvdata(dev); int id = rdev_get_id(dev); unsigned int reg; - bool rail_enable = true; + bool rail_enable = true, enable_val = true; palmas_smps_read(pmic->palmas, palmas_regs_info[id].ctrl_addr, ®); reg &= ~PALMAS_SMPS12_CTRL_MODE_ACTIVE_MASK; @@ -318,6 +318,7 @@ static int palmas_set_mode_smps(struct regulator_dev *dev, unsigned int mode) reg |= SMPS_CTRL_MODE_PWM; break; default: + enable_val = false; return -EINVAL; } @@ -325,6 +326,11 @@ static int palmas_set_mode_smps(struct regulator_dev *dev, unsigned int mode) if (rail_enable) palmas_smps_write(pmic->palmas, palmas_regs_info[id].ctrl_addr, reg); + + /* Switch the enable value to ensure this is used for enable */ + if (enable_val) + pmic->desc[id].enable_val = pmic->current_reg_mode[id]; + return 0; } @@ -964,6 +970,14 @@ static int palmas_regulators_probe(struct platform_device *pdev) return ret; pmic->current_reg_mode[id] = reg & PALMAS_SMPS12_CTRL_MODE_ACTIVE_MASK; + + pmic->desc[id].enable_reg = + PALMAS_BASE_TO_REG(PALMAS_LDO_BASE, + palmas_regs_info[id].ctrl_addr); + pmic->desc[id].enable_mask = + PALMAS_SMPS12_CTRL_MODE_ACTIVE_MASK; + /* set_mode overrides this value */ + pmic->desc[id].enable_val = SMPS_CTRL_MODE_ON; } pmic->desc[id].type = REGULATOR_VOLTAGE; -- Regards, Nishanth Menon ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: Palmas regulator broken (was Re: [PATCH] ARM: tegra: TN7: relax some regulators) 2014-06-20 13:54 ` Nishanth Menon (?) @ 2014-06-20 14:14 ` Alexandre Courbot [not found] ` <CAAVeFuJ=JTNkwDKBvuoyL=ARQs_UJb_MHzL9S8gOc8EU98znPg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> -1 siblings, 1 reply; 22+ messages in thread From: Alexandre Courbot @ 2014-06-20 14:14 UTC (permalink / raw) To: Nishanth Menon Cc: Alexandre Courbot, Mark Brown, Stephen Warren, Keerthy, Thierry Reding, linux-tegra, linux-kernel, linux-omap On Fri, Jun 20, 2014 at 10:54 PM, Nishanth Menon <nm@ti.com> wrote: > On 08:23-20140620, Nishanth Menon wrote: >> + l-o, >> http://marc.info/?t=140316427500004&r=1&w=2 full thread >> >> Minor change in subject to indicate palmas regulator fail >> >> On 18:49-20140620, Alexandre Courbot wrote: >> > 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. >> >> Your quote earlier in the thread >> " >> _regulator_is_enabled() *also* returns false >> " >> >> Got me curious. Looking at the patch: >> dbabd624d4eec50b623bab070d1e39a854b2d65c (regulator: palmas: Reemove >> open coded functions with helper functions) >> I noticed the following change >> palmas_is_enabled_smps -> regulator_is_enabled_regmap >> >> So I decided to search for enable_reg in palmas-regulator.c and I think >> it needs valid enable_reg, mask, value for regulator_is_enabled_regmap to work >> :). >> >> Maybe to be sure, we could print the following: >> PALMAS_SMPS8_VOLTAGE, PALMAS_SMPS8_CTRL, PALMAS_SMPS8_TSTEP, >> >> Anyways, I quickly boot tested the following on DRA7evm (which also uses Palmas): >> [ 1.933939] palmas-pmic 48070000.i2c:tps659038@58:tps659038_pmic: enable_reg = 0x00, mask =0x00 >> [ 1.944210] smps123: 850 <--> 1250 mV at 1060 mV >> [ 1.950717] palmas-pmic 48070000.i2c:tps659038@58:tps659038_pmic: enable_reg = 0x00, mask =0x00 >> [ 1.960754] smps45: 850 <--> 1150 mV at 1060 mV >> [ 1.967048] palmas-pmic 48070000.i2c:tps659038@58:tps659038_pmic: enable_reg = 0x00, mask =0x00 >> [ 1.977072] smps6: 850 <--> 1650 mV at 1060 mV >> [ 1.983077] palmas-pmic 48070000.i2c:tps659038@58:tps659038_pmic: enable_reg = 0x00, mask =0x00 >> [ 1.992994] smps7: 850 <--> 1030 mV at 1030 mV >> [ 1.999238] palmas-pmic 48070000.i2c:tps659038@58:tps659038_pmic: enable_reg = 0x00, mask =0x00 >> [ 2.009161] smps8: 850 <--> 1250 mV at 1060 mV >> [ 2.015304] palmas-pmic 48070000.i2c:tps659038@58:tps659038_pmic: enable_reg = 0x00, mask =0x00 >> >> It does seem to me that either set_mode also should use core functions >> OR you still need a palmas specific is_enable, enable/disable functions >> (contrary to the claim of the patch in question - which I think >> introduced regressions). >> >> Otherwise, completely untested diff below - can you give this a shot? >> >> diff --git a/drivers/regulator/palmas-regulator.c b/drivers/regulator/palmas-regulator.c >> index b982f0f..bbfe22f 100644 >> --- a/drivers/regulator/palmas-regulator.c >> +++ b/drivers/regulator/palmas-regulator.c >> @@ -964,6 +964,20 @@ static int palmas_regulators_probe(struct platform_device *pdev) >> return ret; >> pmic->current_reg_mode[id] = reg & >> PALMAS_SMPS12_CTRL_MODE_ACTIVE_MASK; >> + >> + dev_err(&pdev->dev, "enable_reg = 0x%02x, mask =0x%02x\n", >> + pmic->desc[id].enable_reg, >> + pmic->desc[id].enable_mask); >> + pmic->desc[id].enable_reg = >> + PALMAS_BASE_TO_REG(PALMAS_LDO_BASE, >> + palmas_regs_info[id].ctrl_addr); >> + pmic->desc[id].enable_mask = >> + PALMAS_SMPS12_CTRL_MODE_ACTIVE_MASK; >> + /* >> + * The following completely ignores >> + * pmic->current_reg_mode[id] (set_mode) >> + */ >> + pmic->desc[id].enable_val = SMPS_CTRL_MODE_ON; >> } >> >> pmic->desc[id].type = REGULATOR_VOLTAGE; > > rev 2 of the diff - this does depened on the fact that regulator_desc is > not memdup-ed by regulator code - that lets us do a bit of a trickery ;) > - and I dropped the prints.. Unrelated: This makes me wonder why > palmas_is_enabled_ldo at all? > > Keerthy, Mark, > what do you think of the following (esp the flip of desc value): > diff --git a/drivers/regulator/palmas-regulator.c b/drivers/regulator/palmas-regulator.c > index b982f0f..f01d9c5 100644 > --- a/drivers/regulator/palmas-regulator.c > +++ b/drivers/regulator/palmas-regulator.c > @@ -299,7 +299,7 @@ static int palmas_set_mode_smps(struct regulator_dev *dev, unsigned int mode) > struct palmas_pmic *pmic = rdev_get_drvdata(dev); > int id = rdev_get_id(dev); > unsigned int reg; > - bool rail_enable = true; > + bool rail_enable = true, enable_val = true; > > palmas_smps_read(pmic->palmas, palmas_regs_info[id].ctrl_addr, ®); > reg &= ~PALMAS_SMPS12_CTRL_MODE_ACTIVE_MASK; > @@ -318,6 +318,7 @@ static int palmas_set_mode_smps(struct regulator_dev *dev, unsigned int mode) > reg |= SMPS_CTRL_MODE_PWM; > break; > default: > + enable_val = false; > return -EINVAL; > } > > @@ -325,6 +326,11 @@ static int palmas_set_mode_smps(struct regulator_dev *dev, unsigned int mode) > if (rail_enable) > palmas_smps_write(pmic->palmas, > palmas_regs_info[id].ctrl_addr, reg); > + > + /* Switch the enable value to ensure this is used for enable */ > + if (enable_val) > + pmic->desc[id].enable_val = pmic->current_reg_mode[id]; > + > return 0; > } > > @@ -964,6 +970,14 @@ static int palmas_regulators_probe(struct platform_device *pdev) > return ret; > pmic->current_reg_mode[id] = reg & > PALMAS_SMPS12_CTRL_MODE_ACTIVE_MASK; > + > + pmic->desc[id].enable_reg = > + PALMAS_BASE_TO_REG(PALMAS_LDO_BASE, > + palmas_regs_info[id].ctrl_addr); > + pmic->desc[id].enable_mask = > + PALMAS_SMPS12_CTRL_MODE_ACTIVE_MASK; > + /* set_mode overrides this value */ > + pmic->desc[id].enable_val = SMPS_CTRL_MODE_ON; > } > > pmic->desc[id].type = REGULATOR_VOLTAGE; Tried this v2 and it seems to do the trick! My panel switches on as expected. Thanks, Alex. ^ permalink raw reply [flat|nested] 22+ messages in thread
[parent not found: <CAAVeFuJ=JTNkwDKBvuoyL=ARQs_UJb_MHzL9S8gOc8EU98znPg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: Palmas regulator broken (was Re: [PATCH] ARM: tegra: TN7: relax some regulators) 2014-06-20 14:14 ` Alexandre Courbot @ 2014-06-20 17:27 ` Nishanth Menon 0 siblings, 0 replies; 22+ messages in thread From: Nishanth Menon @ 2014-06-20 17:27 UTC (permalink / raw) To: Alexandre Courbot Cc: Alexandre Courbot, Mark Brown, Stephen Warren, Keerthy, Thierry Reding, linux-tegra-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-omap-u79uwXL29TY76Z2rM5mHXA On 06/20/2014 09:14 AM, Alexandre Courbot wrote: [...] > Tried this v2 and it seems to do the trick! My panel switches on as expected. Thanks. I posted a formal patch https://patchwork.kernel.org/patch/4391271/ -> might want to add tested/reviewed/ack on that thread. I hope I got your email ID correct as well there.. -- Regards, Nishanth Menon ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Palmas regulator broken (was Re: [PATCH] ARM: tegra: TN7: relax some regulators) @ 2014-06-20 17:27 ` Nishanth Menon 0 siblings, 0 replies; 22+ messages in thread From: Nishanth Menon @ 2014-06-20 17:27 UTC (permalink / raw) To: Alexandre Courbot Cc: Alexandre Courbot, Mark Brown, Stephen Warren, Keerthy, Thierry Reding, linux-tegra, linux-kernel, linux-omap On 06/20/2014 09:14 AM, Alexandre Courbot wrote: [...] > Tried this v2 and it seems to do the trick! My panel switches on as expected. Thanks. I posted a formal patch https://patchwork.kernel.org/patch/4391271/ -> might want to add tested/reviewed/ack on that thread. I hope I got your email ID correct as well there.. -- Regards, Nishanth Menon ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ARM: tegra: TN7: relax some regulators 2014-06-20 5:26 ` Alexandre Courbot (?) (?) @ 2014-06-20 10:16 ` Mark Brown [not found] ` <20140620101618.GU5099-GFdadSzt00ze9xe1eoZjHA@public.gmane.org> -1 siblings, 1 reply; 22+ messages in thread From: Mark Brown @ 2014-06-20 10:16 UTC (permalink / raw) To: Alexandre Courbot Cc: Stephen Warren, Thierry Reding, linux-tegra, linux-kernel [-- Attachment #1: Type: text/plain, Size: 882 bytes --] 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). [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 22+ messages in thread
[parent not found: <20140620101618.GU5099-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>]
* Re: [PATCH] ARM: tegra: TN7: relax some regulators 2014-06-20 10:16 ` [PATCH] ARM: tegra: TN7: relax some regulators Mark Brown @ 2014-06-20 10:33 ` Alexandre Courbot 0 siblings, 0 replies; 22+ messages in thread From: Alexandre Courbot @ 2014-06-20 10:33 UTC (permalink / raw) To: Mark Brown Cc: Stephen Warren, Thierry Reding, linux-tegra-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ARM: tegra: TN7: relax some regulators @ 2014-06-20 10:33 ` Alexandre Courbot 0 siblings, 0 replies; 22+ messages in thread From: Alexandre Courbot @ 2014-06-20 10:33 UTC (permalink / raw) To: Mark Brown; +Cc: Stephen Warren, Thierry Reding, linux-tegra, linux-kernel 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2014-06-20 17:27 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-06-19 7:49 [PATCH] ARM: tegra: TN7: relax some regulators Alexandre Courbot 2014-06-19 7:49 ` Alexandre Courbot 2014-06-19 15:59 ` Stephen Warren 2014-06-19 17:56 ` Mark Brown [not found] ` <20140619175643.GR5099-GFdadSzt00ze9xe1eoZjHA@public.gmane.org> 2014-06-20 5:26 ` Alexandre Courbot 2014-06-20 5:26 ` Alexandre Courbot [not found] ` <53A3C613.8030207-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2014-06-20 6:44 ` Alexandre Courbot 2014-06-20 6:44 ` Alexandre Courbot [not found] ` <53A3D85E.7030704-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2014-06-20 9:41 ` Mark Brown 2014-06-20 9:41 ` Mark Brown [not found] ` <20140620094119.GT5099-GFdadSzt00ze9xe1eoZjHA@public.gmane.org> 2014-06-20 9:49 ` Alexandre Courbot 2014-06-20 9:49 ` Alexandre Courbot 2014-06-20 13:23 ` Palmas regulator broken (was Re: [PATCH] ARM: tegra: TN7: relax some regulators) Nishanth Menon 2014-06-20 13:23 ` Nishanth Menon 2014-06-20 13:54 ` Nishanth Menon 2014-06-20 13:54 ` Nishanth Menon 2014-06-20 14:14 ` Alexandre Courbot [not found] ` <CAAVeFuJ=JTNkwDKBvuoyL=ARQs_UJb_MHzL9S8gOc8EU98znPg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2014-06-20 17:27 ` Nishanth Menon 2014-06-20 17:27 ` Nishanth Menon 2014-06-20 10:16 ` [PATCH] ARM: tegra: TN7: relax some regulators Mark Brown [not found] ` <20140620101618.GU5099-GFdadSzt00ze9xe1eoZjHA@public.gmane.org> 2014-06-20 10:33 ` Alexandre Courbot 2014-06-20 10:33 ` Alexandre Courbot
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.