[PATCH] regulator: palmas: Fix SMPS enable/disable/is_enabled

[PATCH] regulator: palmas: Fix SMPS enable/disable/is_enabled

[Nishanth Menon]

We use regmap regulator ops to enable/disable and check if regulator
is enabled for various SMPS. However, these depend on valid
enable_reg, enable_mask and enable_value in regulator descriptor.

Currently we do not populate these for SMPS other than SMPS10, this
results in spurious results as regmap assumes that the values are
valid and ends up reading register 0x0 RTC:SECONDS_REG on Palmas
variants that do have RTC! To fix this, we update proper parameters
for the descriptor fields.

Further, we want to ensure the behavior consistent with logic
prior to commit dbabd624d4eec50b6, where, once you do a set_mode,
enable/disable ensure the logic remains consistent and configures
Palmas to the configuration that we set with set_mode (since the
configuration register is common). To do this, we can rely on the
regulator core's regulator_register behavior where the regulator
descriptor pointer provided by the regulator driver is stored. (no
reallocation and copy is done). This lets us update the enable_value
post registration, to remain consistent with the mode we configure as
part of set_mode.

Fixes: dbabd624d4eec50b6 ("regulator: palmas: Reemove open coded functions with helper functions")
Reported-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Nishanth Menon <nm-l0cyMroinI0@public.gmane.org>
---

NOTE: there is a minor checkpatch check warning- but trying to fix
it makes the code inconsistent with remaining code flow, so, I have
chosen to ignore it.

CHECK: Alignment should match open parenthesis
#56: FILE: drivers/regulator/palmas-regulator.c:974:
+					PALMAS_BASE_TO_REG(PALMAS_LDO_BASE,
+						palmas_regs_info[id].ctrl_addr);

total: 0 errors, 0 warnings, 1 checks, 24 lines checked

Original Report: http://marc.info/?t=140316427500004&r=1&w=2 
Debug: http://marc.info/?t=140327063500007&r=1&w=2

 drivers/regulator/palmas-regulator.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/regulator/palmas-regulator.c b/drivers/regulator/palmas-regulator.c
index b982f0f..3c861d5 100644
--- a/drivers/regulator/palmas-regulator.c
+++ b/drivers/regulator/palmas-regulator.c
@@ -325,6 +325,10 @@ 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 */
+	pmic->desc[id].enable_val = pmic->current_reg_mode[id];
+
 	return 0;
 }
 
@@ -964,6 +968,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;
-- 
1.7.9.5

Re: [PATCH] regulator: palmas: Fix SMPS enable/disable/is_enabled

[Alexandre Courbot]

On Sat, Jun 21, 2014 at 2:26 AM, Nishanth Menon <nm@ti.com> wrote:
> We use regmap regulator ops to enable/disable and check if regulator
> is enabled for various SMPS. However, these depend on valid
> enable_reg, enable_mask and enable_value in regulator descriptor.
>
> Currently we do not populate these for SMPS other than SMPS10, this
> results in spurious results as regmap assumes that the values are
> valid and ends up reading register 0x0 RTC:SECONDS_REG on Palmas
> variants that do have RTC! To fix this, we update proper parameters
> for the descriptor fields.
>
> Further, we want to ensure the behavior consistent with logic
> prior to commit dbabd624d4eec50b6, where, once you do a set_mode,
> enable/disable ensure the logic remains consistent and configures
> Palmas to the configuration that we set with set_mode (since the
> configuration register is common). To do this, we can rely on the
> regulator core's regulator_register behavior where the regulator
> descriptor pointer provided by the regulator driver is stored. (no
> reallocation and copy is done). This lets us update the enable_value
> post registration, to remain consistent with the mode we configure as
> part of set_mode.

Tested-by: Alexandre Courbot <acourbot@nvidia.com>

Thanks!

Re: [PATCH] regulator: palmas: Fix SMPS enable/disable/is_enabled

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 283 bytes --]

On Fri, Jun 20, 2014 at 12:26:23PM -0500, Nishanth Menon wrote:
> We use regmap regulator ops to enable/disable and check if regulator
> is enabled for various SMPS. However, these depend on valid
> enable_reg, enable_mask and enable_value in regulator descriptor.

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] regulator: palmas: Fix SMPS enable/disable/is_enabled

[Stephen Warren]

On 06/20/2014 11:26 AM, Nishanth Menon wrote:
> We use regmap regulator ops to enable/disable and check if regulator
> is enabled for various SMPS. However, these depend on valid
> enable_reg, enable_mask and enable_value in regulator descriptor.
> 
> Currently we do not populate these for SMPS other than SMPS10, this
> results in spurious results as regmap assumes that the values are
> valid and ends up reading register 0x0 RTC:SECONDS_REG on Palmas
> variants that do have RTC! To fix this, we update proper parameters
> for the descriptor fields.
> 
> Further, we want to ensure the behavior consistent with logic
> prior to commit dbabd624d4eec50b6, where, once you do a set_mode,
> enable/disable ensure the logic remains consistent and configures
> Palmas to the configuration that we set with set_mode (since the
> configuration register is common). To do this, we can rely on the
> regulator core's regulator_register behavior where the regulator
> descriptor pointer provided by the regulator driver is stored. (no
> reallocation and copy is done). This lets us update the enable_value
> post registration, to remain consistent with the mode we configure as
> part of set_mode.
> 
> Fixes: dbabd624d4eec50b6 ("regulator: palmas: Reemove open coded functions with helper functions")
> Reported-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Nishanth Menon <nm-l0cyMroinI0@public.gmane.org>

Unfortunately, there is still some lingering problem in the original
commit. In next-20130623 (and indeed since at least next-20140611),
neither the LCD panel or HDMI work on the NVIDIA Dalmore board.
Reverting this commit (just for conflicts) and the original problematic
commit "regulator: palmas: Reemove open coded functions with helper
functions" solves this. I see the following on boot:

> [    3.558776] tegra-dsi 54300000.dsi: cannot get VDD supply
> [    3.564272] platform 54300000.dsi: Driver tegra-dsi requests probe deferral
> [    3.571990] tegra-hdmi 54280000.hdmi: failed to get PLL regulator
> [    3.578377] platform 54280000.hdmi: Driver tegra-hdmi requests probe deferral

... but probe deferral never completes, yet with your "remove open coded
..." patch reverted, it all works fine.

Can you please take another look at the original patch?

Re: [PATCH] regulator: palmas: Fix SMPS enable/disable/is_enabled

[Nishanth Menon]

On Mon, Jun 23, 2014 at 2:50 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 06/20/2014 11:26 AM, Nishanth Menon wrote:
>> We use regmap regulator ops to enable/disable and check if regulator
>> is enabled for various SMPS. However, these depend on valid
>> enable_reg, enable_mask and enable_value in regulator descriptor.
>>
>> Currently we do not populate these for SMPS other than SMPS10, this
>> results in spurious results as regmap assumes that the values are
>> valid and ends up reading register 0x0 RTC:SECONDS_REG on Palmas
>> variants that do have RTC! To fix this, we update proper parameters
>> for the descriptor fields.
>>
>> Further, we want to ensure the behavior consistent with logic
>> prior to commit dbabd624d4eec50b6, where, once you do a set_mode,
>> enable/disable ensure the logic remains consistent and configures
>> Palmas to the configuration that we set with set_mode (since the
>> configuration register is common). To do this, we can rely on the
>> regulator core's regulator_register behavior where the regulator
>> descriptor pointer provided by the regulator driver is stored. (no
>> reallocation and copy is done). This lets us update the enable_value
>> post registration, to remain consistent with the mode we configure as
>> part of set_mode.
>>
>> Fixes: dbabd624d4eec50b6 ("regulator: palmas: Reemove open coded functions with helper functions")
>> Reported-by: Alexandre Courbot <acourbot@nvidia.com>
>> Signed-off-by: Nishanth Menon <nm@ti.com>
>
> Unfortunately, there is still some lingering problem in the original
> commit. In next-20130623 (and indeed since at least next-20140611),
> neither the LCD panel or HDMI work on the NVIDIA Dalmore board.
> Reverting this commit (just for conflicts) and the original problematic
> commit "regulator: palmas: Reemove open coded functions with helper
> functions" solves this. I see the following on boot:
>
>> [    3.558776] tegra-dsi 54300000.dsi: cannot get VDD supply
>> [    3.564272] platform 54300000.dsi: Driver tegra-dsi requests probe deferral
>> [    3.571990] tegra-hdmi 54280000.hdmi: failed to get PLL regulator
>> [    3.578377] platform 54280000.hdmi: Driver tegra-hdmi requests probe deferral
>
> ... but probe deferral never completes, yet with your "remove open coded
> ..." patch reverted, it all works fine.
>
> Can you please take another look at the original patch?

Will let keerthy (original patch author) comment on it.

arch/arm/boot/dts/tegra114-dalmore.dts tps65090, avdd_lcd_reg I
suppose is the path in question?

Seems to use drivers/mfd/tps65090.c and
drivers/regulator/tps65090-regulator.c and not palmas?

Am I looking at the right dts?

---
Regards,
Nishanth Menon

Re: [PATCH] regulator: palmas: Fix SMPS enable/disable/is_enabled

[Stephen Warren]

On 06/23/2014 02:11 PM, Nishanth Menon wrote:
> On Mon, Jun 23, 2014 at 2:50 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 06/20/2014 11:26 AM, Nishanth Menon wrote:
>>> We use regmap regulator ops to enable/disable and check if regulator
>>> is enabled for various SMPS. However, these depend on valid
>>> enable_reg, enable_mask and enable_value in regulator descriptor.
>>>
>>> Currently we do not populate these for SMPS other than SMPS10, this
>>> results in spurious results as regmap assumes that the values are
>>> valid and ends up reading register 0x0 RTC:SECONDS_REG on Palmas
>>> variants that do have RTC! To fix this, we update proper parameters
>>> for the descriptor fields.
>>>
>>> Further, we want to ensure the behavior consistent with logic
>>> prior to commit dbabd624d4eec50b6, where, once you do a set_mode,
>>> enable/disable ensure the logic remains consistent and configures
>>> Palmas to the configuration that we set with set_mode (since the
>>> configuration register is common). To do this, we can rely on the
>>> regulator core's regulator_register behavior where the regulator
>>> descriptor pointer provided by the regulator driver is stored. (no
>>> reallocation and copy is done). This lets us update the enable_value
>>> post registration, to remain consistent with the mode we configure as
>>> part of set_mode.
>>>
>>> Fixes: dbabd624d4eec50b6 ("regulator: palmas: Reemove open coded functions with helper functions")
>>> Reported-by: Alexandre Courbot <acourbot@nvidia.com>
>>> Signed-off-by: Nishanth Menon <nm@ti.com>
>>
>> Unfortunately, there is still some lingering problem in the original
>> commit. In next-20130623 (and indeed since at least next-20140611),
>> neither the LCD panel or HDMI work on the NVIDIA Dalmore board.
>> Reverting this commit (just for conflicts) and the original problematic
>> commit "regulator: palmas: Reemove open coded functions with helper
>> functions" solves this. I see the following on boot:
>>
>>> [    3.558776] tegra-dsi 54300000.dsi: cannot get VDD supply
>>> [    3.564272] platform 54300000.dsi: Driver tegra-dsi requests probe deferral
>>> [    3.571990] tegra-hdmi 54280000.hdmi: failed to get PLL regulator
>>> [    3.578377] platform 54280000.hdmi: Driver tegra-hdmi requests probe deferral
>>
>> ... but probe deferral never completes, yet with your "remove open coded
>> ..." patch reverted, it all works fine.
>>
>> Can you please take another look at the original patch?
> 
> Will let keerthy (original patch author) comment on it.
> 
> arch/arm/boot/dts/tegra114-dalmore.dts tps65090, avdd_lcd_reg I
> suppose is the path in question?
> 
> Seems to use drivers/mfd/tps65090.c and
> drivers/regulator/tps65090-regulator.c and not palmas?
> 
> Am I looking at the right dts?

Yes, that's the right DTS.

There's both a 65090 and a Palmas on the board. It's probably simpler to
look at the HDMI PLL regulator, since the path to the Palmas is more
obvious:

/ {
	host1x@50000000 {
		hdmi@54280000 {
			pll-supply = <&palmas_smps3_reg>;
...
	i2c@7000d000 {
		palmas: tps65913@58 {
			compatible = "ti,palmas";
			pmic {
				compatible = "ti,tps65913-pmic",
						"ti,palmas-pmic";
				regulators {
					palmas_smps3_reg: smps3 {

Re: [PATCH] regulator: palmas: Fix SMPS enable/disable/is_enabled

[Nishanth Menon]

On Mon, Jun 23, 2014 at 3:20 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 06/23/2014 02:11 PM, Nishanth Menon wrote:
>> On Mon, Jun 23, 2014 at 2:50 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>>> On 06/20/2014 11:26 AM, Nishanth Menon wrote:
>>>> We use regmap regulator ops to enable/disable and check if regulator
>>>> is enabled for various SMPS. However, these depend on valid
>>>> enable_reg, enable_mask and enable_value in regulator descriptor.
>>>>
>>>> Currently we do not populate these for SMPS other than SMPS10, this
>>>> results in spurious results as regmap assumes that the values are
>>>> valid and ends up reading register 0x0 RTC:SECONDS_REG on Palmas
>>>> variants that do have RTC! To fix this, we update proper parameters
>>>> for the descriptor fields.
>>>>
>>>> Further, we want to ensure the behavior consistent with logic
>>>> prior to commit dbabd624d4eec50b6, where, once you do a set_mode,
>>>> enable/disable ensure the logic remains consistent and configures
>>>> Palmas to the configuration that we set with set_mode (since the
>>>> configuration register is common). To do this, we can rely on the
>>>> regulator core's regulator_register behavior where the regulator
>>>> descriptor pointer provided by the regulator driver is stored. (no
>>>> reallocation and copy is done). This lets us update the enable_value
>>>> post registration, to remain consistent with the mode we configure as
>>>> part of set_mode.
>>>>
>>>> Fixes: dbabd624d4eec50b6 ("regulator: palmas: Reemove open coded functions with helper functions")
>>>> Reported-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>> Signed-off-by: Nishanth Menon <nm-l0cyMroinI0@public.gmane.org>
>>>
>>> Unfortunately, there is still some lingering problem in the original
>>> commit. In next-20130623 (and indeed since at least next-20140611),
>>> neither the LCD panel or HDMI work on the NVIDIA Dalmore board.
>>> Reverting this commit (just for conflicts) and the original problematic
>>> commit "regulator: palmas: Reemove open coded functions with helper
>>> functions" solves this. I see the following on boot:
>>>
>>>> [    3.558776] tegra-dsi 54300000.dsi: cannot get VDD supply
>>>> [    3.564272] platform 54300000.dsi: Driver tegra-dsi requests probe deferral
>>>> [    3.571990] tegra-hdmi 54280000.hdmi: failed to get PLL regulator
>>>> [    3.578377] platform 54280000.hdmi: Driver tegra-hdmi requests probe deferral
>>>
>>> ... but probe deferral never completes, yet with your "remove open coded
>>> ..." patch reverted, it all works fine.
>>>
>>> Can you please take another look at the original patch?
>>
>> Will let keerthy (original patch author) comment on it.
>>
>> arch/arm/boot/dts/tegra114-dalmore.dts tps65090, avdd_lcd_reg I
>> suppose is the path in question?
>>
>> Seems to use drivers/mfd/tps65090.c and
>> drivers/regulator/tps65090-regulator.c and not palmas?
>>
>> Am I looking at the right dts?
>
> Yes, that's the right DTS.
>
> There's both a 65090 and a Palmas on the board. It's probably simpler to
> look at the HDMI PLL regulator, since the path to the Palmas is more
> obvious:
>
> / {
>         host1x@50000000 {
>                 hdmi@54280000 {
>                         pll-supply = <&palmas_smps3_reg>;
> ...
>         i2c@7000d000 {
>                 palmas: tps65913@58 {
>                         compatible = "ti,palmas";
>                         pmic {
>                                 compatible = "ti,tps65913-pmic",
>                                                 "ti,palmas-pmic";
>                                 regulators {
>                                         palmas_smps3_reg: smps3 {
>

based on "tegra-dsi 54300000.dsi: cannot get VDD supply" (from
drivers/gpu/drm/tegra/dsi.c), first fail of the log,  I suspect LDO
and not smps?

 avdd_1v2_reg: ldo3 {
         regulator-name = "avdd-dsi-csi";
         regulator-min-microvolt = <1200000>;
         regulator-max-microvolt = <1200000>;
 };

Do you have a complete boot log? there could be cascade fail that
results in LDO not being available. a post on hastebin/slexy.org would
be nice.

The trouble is that when 1 registration fails, the entire cascade of
regulators will not get registered :(.

---
Regards,
Nishanth Menon

Re: [PATCH] regulator: palmas: Fix SMPS enable/disable/is_enabled

[Stephen Warren]

On 06/23/2014 02:20 PM, Stephen Warren wrote:
> On 06/23/2014 02:11 PM, Nishanth Menon wrote:
>> On Mon, Jun 23, 2014 at 2:50 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> On 06/20/2014 11:26 AM, Nishanth Menon wrote:
>>>> We use regmap regulator ops to enable/disable and check if regulator
>>>> is enabled for various SMPS. However, these depend on valid
>>>> enable_reg, enable_mask and enable_value in regulator descriptor.
>>>>
>>>> Currently we do not populate these for SMPS other than SMPS10, this
>>>> results in spurious results as regmap assumes that the values are
>>>> valid and ends up reading register 0x0 RTC:SECONDS_REG on Palmas
>>>> variants that do have RTC! To fix this, we update proper parameters
>>>> for the descriptor fields.
>>>>
>>>> Further, we want to ensure the behavior consistent with logic
>>>> prior to commit dbabd624d4eec50b6, where, once you do a set_mode,
>>>> enable/disable ensure the logic remains consistent and configures
>>>> Palmas to the configuration that we set with set_mode (since the
>>>> configuration register is common). To do this, we can rely on the
>>>> regulator core's regulator_register behavior where the regulator
>>>> descriptor pointer provided by the regulator driver is stored. (no
>>>> reallocation and copy is done). This lets us update the enable_value
>>>> post registration, to remain consistent with the mode we configure as
>>>> part of set_mode.
>>>>
>>>> Fixes: dbabd624d4eec50b6 ("regulator: palmas: Reemove open coded functions with helper functions")
>>>> Reported-by: Alexandre Courbot <acourbot@nvidia.com>
>>>> Signed-off-by: Nishanth Menon <nm@ti.com>
>>>
>>> Unfortunately, there is still some lingering problem in the original
>>> commit. In next-20130623 (and indeed since at least next-20140611),
>>> neither the LCD panel or HDMI work on the NVIDIA Dalmore board.
>>> Reverting this commit (just for conflicts) and the original problematic
>>> commit "regulator: palmas: Reemove open coded functions with helper
>>> functions" solves this. I see the following on boot:
>>>
>>>> [    3.558776] tegra-dsi 54300000.dsi: cannot get VDD supply
>>>> [    3.564272] platform 54300000.dsi: Driver tegra-dsi requests probe deferral
>>>> [    3.571990] tegra-hdmi 54280000.hdmi: failed to get PLL regulator
>>>> [    3.578377] platform 54280000.hdmi: Driver tegra-hdmi requests probe deferral
>>>
>>> ... but probe deferral never completes, yet with your "remove open coded
>>> ..." patch reverted, it all works fine.
>>>
>>> Can you please take another look at the original patch?
>>
>> Will let keerthy (original patch author) comment on it.
>>
>> arch/arm/boot/dts/tegra114-dalmore.dts tps65090, avdd_lcd_reg I
>> suppose is the path in question?
>>
>> Seems to use drivers/mfd/tps65090.c and
>> drivers/regulator/tps65090-regulator.c and not palmas?
>>
>> Am I looking at the right dts?
> 
> Yes, that's the right DTS.
> 
> There's both a 65090 and a Palmas on the board. It's probably simpler to
> look at the HDMI PLL regulator, since the path to the Palmas is more
> obvious:
...

I think I found the problem; I sent "[PATCH] regulator: palmas: fix typo
in enable_reg calculation" with what's probably the fix.