[PATCH] ARM: tegra: TN7: relax some regulators

[PATCH] ARM: tegra: TN7: relax some regulators

[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

Re: [PATCH] ARM: tegra: TN7: relax some regulators

[Stephen Warren]

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.

Re: [PATCH] ARM: tegra: TN7: relax some regulators

[Mark Brown]

[-- 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 --]

Re: [PATCH] ARM: tegra: TN7: relax some regulators

[Alexandre Courbot]

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.

Re: [PATCH] ARM: tegra: TN7: relax some regulators

[Alexandre Courbot]

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.
>

Re: [PATCH] ARM: tegra: TN7: relax some regulators

[Mark Brown]

[-- 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 --]

Re: [PATCH] ARM: tegra: TN7: relax some regulators

[Alexandre Courbot]

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.

Palmas regulator broken (was Re: [PATCH] ARM: tegra: TN7: relax some regulators)

[Nishanth Menon]

+ 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

Re: Palmas regulator broken (was Re: [PATCH] ARM: tegra: TN7: relax some regulators)

[Nishanth Menon]

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);
 	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

Re: Palmas regulator broken (was Re: [PATCH] ARM: tegra: TN7: relax some regulators)

[Alexandre Courbot]

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);
>         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.

Re: Palmas regulator broken (was Re: [PATCH] ARM: tegra: TN7: relax some regulators)

[Nishanth Menon]

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

Re: [PATCH] ARM: tegra: TN7: relax some regulators

[Mark Brown]

[-- 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 --]

Re: [PATCH] ARM: tegra: TN7: relax some regulators

[Alexandre Courbot]

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.