| Message ID | 20190615163314.28173-1-andreas@kemnade.info (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | regulator: twl: mark vdd1/2 as continuous on twl4030 | expand |
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.
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
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.
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
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
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.
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
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, \ }, \ }
_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(+)