[PATCH] regulator: twl: mark vdd1/2 as continuous on twl4030

[PATCH] regulator: twl: mark vdd1/2 as continuous on twl4030

[Andreas Kemnade]

_opp_supported_by_regulators() wrongly ignored errors from
regulator_is_supported_voltage(), so it considered errors as
success. Since
commit 498209445124 ("regulator: core: simplify return value on suported_voltage")
regulator_is_supported_voltage() returns a real boolean, so
errors make _opp_supported_by_regulators() return false.

The VDD1/VDD2 regulators on twl4030 are neither defined with
voltage lists nor with the continuous flag set, so
regulator_is_supported_voltage() returns false and an error
before above mentioned commit (which was considered success)
The result is that after the above mentioned commit cpufreq
does not work properly e.g. dm3730.

[    2.490997] core: _opp_supported_by_regulators: OPP minuV: 1012500 maxuV: 1012500, not supported by regulator
[    2.501617] cpu cpu0: _opp_add: OPP not supported by regulators (300000000)
[    2.509246] core: _opp_supported_by_regulators: OPP minuV: 1200000 maxuV: 1200000, not supported by regulator
[    2.519775] cpu cpu0: _opp_add: OPP not supported by regulators (600000000)
[    2.527313] core: _opp_supported_by_regulators: OPP minuV: 1325000 maxuV: 1325000, not supported by regulator
[    2.537750] cpu cpu0: _opp_add: OPP not supported by regulators (800000000)

The patch fixes declaration of VDD1/2 regulators.

Fixes: 498209445124 ("regulator: core: simplify return value on suported_voltage")
Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 drivers/regulator/twl-regulator.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
index 6fa15b2d6fb3..f7bfdf53701d 100644
--- a/drivers/regulator/twl-regulator.c
+++ b/drivers/regulator/twl-regulator.c
@@ -478,6 +478,7 @@ static const struct twlreg_info TWL4030_INFO_##label = { \
 		.type = REGULATOR_VOLTAGE, \
 		.owner = THIS_MODULE, \
 		.enable_time = turnon_delay, \
+		.continuous_voltage_range = true, \
 		.of_map_mode = twl4030reg_map_mode, \
 		}, \
 	}
-- 
2.20.1

Re: [PATCH] regulator: twl: mark vdd1/2 as continuous on twl4030

[Mark Brown]

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

On Sat, Jun 15, 2019 at 06:33:14PM +0200, Andreas Kemnade wrote:

> The VDD1/VDD2 regulators on twl4030 are neither defined with
> voltage lists nor with the continuous flag set, so
> regulator_is_supported_voltage() returns false and an error
> before above mentioned commit (which was considered success)
> The result is that after the above mentioned commit cpufreq
> does not work properly e.g. dm3730.

Why is this a good fix and not defining the supported voltages?  These
look like fairly standard linear range regulators.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regulator: twl: mark vdd1/2 as continuous on twl4030

[Andreas Kemnade]

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

Hi,

On Mon, 17 Jun 2019 11:31:11 +0100
Mark Brown <broonie@kernel.org> wrote:

> On Sat, Jun 15, 2019 at 06:33:14PM +0200, Andreas Kemnade wrote:
> 
> > The VDD1/VDD2 regulators on twl4030 are neither defined with
> > voltage lists nor with the continuous flag set, so
> > regulator_is_supported_voltage() returns false and an error
> > before above mentioned commit (which was considered success)
> > The result is that after the above mentioned commit cpufreq
> > does not work properly e.g. dm3730.  
> 
> Why is this a good fix and not defining the supported voltages?  These
> look like fairly standard linear range regulators.

I am fixing the definition of the two regulators in the patch.
I am defining them as continuous. 
Voltage ranges are defined in
arch/arm/boot/dts/twl4030.dtsi
Only the continuous flag is missing.

Is there anything else do you want to be defined?

Regards,
Andreas

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] regulator: twl: mark vdd1/2 as continuous on twl4030

[Mark Brown]

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

On Mon, Jun 17, 2019 at 01:03:57PM +0200, Andreas Kemnade wrote:
> Mark Brown <broonie@kernel.org> wrote:
> > On Sat, Jun 15, 2019 at 06:33:14PM +0200, Andreas Kemnade wrote:

> > Why is this a good fix and not defining the supported voltages?  These
> > look like fairly standard linear range regulators.

> I am fixing the definition of the two regulators in the patch.
> I am defining them as continuous. 
> Voltage ranges are defined in
> arch/arm/boot/dts/twl4030.dtsi
> Only the continuous flag is missing.

> Is there anything else do you want to be defined?

These regulators are not continuous regulators as far as I can see, they
are normal linear range regulators and so should have their voltages
enumerable like any other linear range regulator.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regulator: twl: mark vdd1/2 as continuous on twl4030

[Andreas Kemnade]

On Mon, 17 Jun 2019 12:40:48 +0100
Mark Brown <broonie@kernel.org> wrote:

> On Mon, Jun 17, 2019 at 01:03:57PM +0200, Andreas Kemnade wrote:
> > Mark Brown <broonie@kernel.org> wrote:  
> > > On Sat, Jun 15, 2019 at 06:33:14PM +0200, Andreas Kemnade wrote:  
> 
> > > Why is this a good fix and not defining the supported voltages?  These
> > > look like fairly standard linear range regulators.  
> 
> > I am fixing the definition of the two regulators in the patch.
> > I am defining them as continuous. 
> > Voltage ranges are defined in
> > arch/arm/boot/dts/twl4030.dtsi
> > Only the continuous flag is missing.  
> 
> > Is there anything else do you want to be defined?  
> 
> These regulators are not continuous regulators as far as I can see, they
> are normal linear range regulators and so should have their voltages
> enumerable like any other linear range regulator.

Citing tps65950 trm page 55:

The device contains three switch-mode power supplies (SMPS):
• VDD1: 1.2-A, buck DC/DC converter (VOUT = 0.6 V to 1.45 V, in steps of 12.5 mV)
• VDD2: 600-mA buck DC/DC converter (VOUT = 0.6 V to 1.45 V, in steps of 12.5 mV, and 1.5 V as a
   single programmable value)

you are right, they are not really continuous. So should I add these
68 steps they have as a voltage list?

I think they are nearly continuous, so we should IMHO rather take that
not that strict. I guess there are no really continuous regulators, all
have steps as voltage is specified in a limited resolution. So what is
the exact meaning of that flag here?

I think it is common sense to specify these regulators as continuous.
Max and min values are already in arch/arm/boot/dts/twl4030.dtsi.

Regards,
Andreas

Re: [PATCH] regulator: twl: mark vdd1/2 as continuous on twl4030

[Mark Brown]

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

On Mon, Jun 17, 2019 at 06:27:43PM +0200, Andreas Kemnade wrote:

> Citing tps65950 trm page 55:

> The device contains three switch-mode power supplies (SMPS):
> • VDD1: 1.2-A, buck DC/DC converter (VOUT = 0.6 V to 1.45 V, in steps of 12.5 mV)
> • VDD2: 600-mA buck DC/DC converter (VOUT = 0.6 V to 1.45 V, in steps of 12.5 mV, and 1.5 V as a
>    single programmable value)

> you are right, they are not really continuous. So should I add these
> 68 steps they have as a voltage list?

There's helpers for linear mappings, you should be able to use those
(see helpers.c).

> I think they are nearly continuous, so we should IMHO rather take that
> not that strict. I guess there are no really continuous regulators, all
> have steps as voltage is specified in a limited resolution. So what is
> the exact meaning of that flag here?

This was added for devices with extremely high resolution interfaces
like some microcontroller interfaces that take voltage values directly
(mirroring the regulator API) or PWM regulators - it's for cases where
enumerating all the voltages is unreasonable.  The TWL4030 regulators
look fairly standard in comparison.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regulator: twl: mark vdd1/2 as continuous on twl4030

[Andreas Kemnade]

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

On Mon, 17 Jun 2019 18:02:55 +0100
Mark Brown <broonie@kernel.org> wrote:

> On Mon, Jun 17, 2019 at 06:27:43PM +0200, Andreas Kemnade wrote:
> 
> > Citing tps65950 trm page 55:  
> 
> > The device contains three switch-mode power supplies (SMPS):
> > • VDD1: 1.2-A, buck DC/DC converter (VOUT = 0.6 V to 1.45 V, in steps of 12.5 mV)
> > • VDD2: 600-mA buck DC/DC converter (VOUT = 0.6 V to 1.45 V, in steps of 12.5 mV, and 1.5 V as a
> >    single programmable value)  
> 
> > you are right, they are not really continuous. So should I add these
> > 68 steps they have as a voltage list?  
> 
> There's helpers for linear mappings, you should be able to use those
> (see helpers.c).
> 
ok, I will send a 2 with such a list.

Thanks for the hint.

> > I think they are nearly continuous, so we should IMHO rather take that
> > not that strict. I guess there are no really continuous regulators, all
> > have steps as voltage is specified in a limited resolution. So what is
> > the exact meaning of that flag here?  
> 
> This was added for devices with extremely high resolution interfaces
> like some microcontroller interfaces that take voltage values directly
> (mirroring the regulator API) or PWM regulators - it's for cases where
> enumerating all the voltages is unreasonable.  The TWL4030 regulators
> look fairly standard in comparison.

well, VDD1 is a lot more continuous than e.g. VAUX3, but your examples
seem to be even more continuous. But maybe a comment in the api documentation
might be helpful so that people do not misinterpret the meaning.

Regards,
Andreas

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] regulator: twl: mark vdd1/2 as continuous on twl4030

[Andreas Kemnade]

On Mon, 17 Jun 2019 12:40:48 +0100
Mark Brown <broonie@kernel.org> wrote:

> On Mon, Jun 17, 2019 at 01:03:57PM +0200, Andreas Kemnade wrote:
> > Mark Brown <broonie@kernel.org> wrote:  
> > > On Sat, Jun 15, 2019 at 06:33:14PM +0200, Andreas Kemnade wrote:  
> 
> > > Why is this a good fix and not defining the supported voltages?  These
> > > look like fairly standard linear range regulators.  
> 
> > I am fixing the definition of the two regulators in the patch.
> > I am defining them as continuous. 
> > Voltage ranges are defined in
> > arch/arm/boot/dts/twl4030.dtsi
> > Only the continuous flag is missing.  
> 
> > Is there anything else do you want to be defined?  
> 
> These regulators are not continuous regulators as far as I can see, they
> are normal linear range regulators and so should have their voltages
> enumerable like any other linear range regulator.

another thing which might be misleading: The patch belongs to the
section after
#define TWL4030_ADJUSTABLE_SMPS(label, offset, num, turnon_delay, remap_conf)

that might be easily misread (because of too less context in the diff),
or if line numbers change.
It is *not* for
#define TWL4030_ADJUSTABLE_LDO(label, offset, num, turnon_delay, remap_conf)

Regards,
Andreas