Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1466901 > unrolled thread

[PATCH 3/9] regulator: core: Try full range when adjusting regulators to constraints

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3/9] regulator: core: Try full range when adjusting regulators to constraints Chen-Yu Tsai <wens@csie.org> - 2016-08-21 05:10 +0200
    Re: [PATCH 3/9] regulator: core: Try full range when adjusting  regulators to constraints Mark Brown <broonie@kernel.org> - 2016-08-22 18:40 +0200
    Re: [PATCH 3/9] regulator: core: Try full range when adjusting  regulators to constraints Mark Brown <broonie@kernel.org> - 2016-08-22 18:40 +0200
      Re: [PATCH 3/9] regulator: core: Try full range when adjusting  regulators to constraints Rask Ingemann Lambertsen <ccc94453@vip.cybercity.dk> - 2016-08-22 20:00 +0200
        Re: [PATCH 3/9] regulator: core: Try full range when adjusting  regulators to constraints Mark Brown <broonie@kernel.org> - 2016-08-22 20:20 +0200
          Re: [PATCH 3/9] regulator: core: Try full range when adjusting  regulators to constraints Chen-Yu Tsai <wens@csie.org> - 2016-08-23 06:20 +0200
            Re: [PATCH 3/9] regulator: core: Try full range when adjusting  regulators to constraints Mark Brown <broonie@kernel.org> - 2016-08-23 13:30 +0200

#1466901 — [PATCH 3/9] regulator: core: Try full range when adjusting regulators to constraints

Currently when we try to bring regulator in bounds of its constraints,
we pick either the minimum or maximum voltage as the target. This fails
if the regulator range is not continuous, and the target voltage is not
an exact value the regulator can achieve, i.e. the target is not aligned
to the step of the regulator.

Instead pass the full range of the constraints, and have the regulator
core work out a suitable voltage within.

Fixes: ("regulator: core: Ensure we are at least in bounds for our
	 constraints")
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/regulator/core.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index db320e8fa865..86f69c92ce7a 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -897,13 +897,9 @@ static int machine_constraints_voltage(struct regulator_dev *rdev,
 		target_min = current_uV;
 		target_max = current_uV;
 
-		if (current_uV < rdev->constraints->min_uV) {
+		if (current_uV < rdev->constraints->min_uV ||
+		    current_uV > rdev->constraints->max_uV) {
 			target_min = rdev->constraints->min_uV;
-			target_max = rdev->constraints->min_uV;
-		}
-
-		if (current_uV > rdev->constraints->max_uV) {
-			target_min = rdev->constraints->max_uV;
 			target_max = rdev->constraints->max_uV;
 		}
 
-- 
2.9.3

[toc] | [next] | [standalone]

#1467780 — Re: [PATCH 3/9] regulator: core: Try full range when adjusting regulators to constraints

[Multipart message — attachments visible in raw view] — view raw

On Sun, Aug 21, 2016 at 10:11:19AM +0800, Chen-Yu Tsai wrote:
> Currently when we try to bring regulator in bounds of its constraints,
> we pick either the minimum or maximum voltage as the target. This fails
> if the regulator range is not continuous, and the target voltage is not
> an exact value the regulator can achieve, i.e. the target is not aligned
> to the step of the regulator.

Oh, and this is nothing to do with implementing support for this MFD so
should be a separate patch for ease of review.

[toc] | [prev] | [next] | [standalone]

#1467786 — Re: [PATCH 3/9] regulator: core: Try full range when adjusting regulators to constraints

[Multipart message — attachments visible in raw view] — view raw

On Sun, Aug 21, 2016 at 10:11:19AM +0800, Chen-Yu Tsai wrote:

> Currently when we try to bring regulator in bounds of its constraints,
> we pick either the minimum or maximum voltage as the target. This fails
> if the regulator range is not continuous, and the target voltage is not
> an exact value the regulator can achieve, i.e. the target is not aligned
> to the step of the regulator.

This seems like you have buggy constraints, constraints which allow
voltages that can't physically be satisfied don't make obvious sense.

> -		if (current_uV < rdev->constraints->min_uV) {
> +		if (current_uV < rdev->constraints->min_uV ||
> +		    current_uV > rdev->constraints->max_uV) {
>  			target_min = rdev->constraints->min_uV;
> -			target_max = rdev->constraints->min_uV;
> -		}
> -
> -		if (current_uV > rdev->constraints->max_uV) {
> -			target_min = rdev->constraints->max_uV;
>  			target_max = rdev->constraints->max_uV;
>  		}

This is most likely going to cause disruption to systems where the
voltage is over the constraint voltage - it will result in the voltage
being lowered to the minimum allowed which will have a much higher
chance of upsetting things.  This is why we don't just do a constraints
run.

[toc] | [prev] | [next] | [standalone]

#1467866 — Re: [PATCH 3/9] regulator: core: Try full range when adjusting regulators to constraints

On Mon, Aug 22, 2016 at 05:29:07PM +0100, Mark Brown wrote:
> On Sun, Aug 21, 2016 at 10:11:19AM +0800, Chen-Yu Tsai wrote:
> 
> > Currently when we try to bring regulator in bounds of its constraints,
> > we pick either the minimum or maximum voltage as the target. This fails
> > if the regulator range is not continuous, and the target voltage is not
> > an exact value the regulator can achieve, i.e. the target is not aligned
> > to the step of the regulator.
> 
> This seems like you have buggy constraints, constraints which allow
> voltages that can't physically be satisfied don't make obvious sense.

No, it's for cases like this (see the cubietruck4 or a80-optimus dts
patches):

			reg_bldo4: bldo4 {
				regulator-min-microvolt = <1080000>;
				regulator-max-microvolt = <1320000>;
				regulator-name = "vcc12-hsic";
			};

The regulator can do 1100000 uV, 1200000 uV and 1300000 uV, all of which
are within the constraints, so obviously the constraints can be satisfied,
yet the regulator core fails do so with a message like this:

vcc12-hsic: Bringing 700000uV into 1080000-1080000uV
vcc12-hsic: failed to apply 1080000-1080000uV constraint(-22).

I've run into the same bug trying to support the CX-A99 board, which uses an
AXP808 PMIC [1], which seems to be very similar to the AXP 806. The patch
fixes the bug by rounding the constraints to voltages which are supported by
the regulator. Output from dmesg on my CX-A99 board looks like this:

[    2.577202] vcc12-hsic: Bringing 700000uV into 1080000-1320000uV
[    2.583335] vcc12-hsic: override min_uV, 1080000 -> 1100000
[    2.589003] vcc12-hsic: override max_uV, 1320000 -> 1300000
[    2.594673] vcc12-hsic: 1100 <--> 1300 mV at 1100 mV 

[1] Details on which will be happily accepted, btw.

-- 
Rask Ingemann Lambertsen

[toc] | [prev] | [next] | [standalone]

#1467877 — Re: [PATCH 3/9] regulator: core: Try full range when adjusting regulators to constraints

[Multipart message — attachments visible in raw view] — view raw

On Mon, Aug 22, 2016 at 07:52:05PM +0200, Rask Ingemann Lambertsen wrote:
> On Mon, Aug 22, 2016 at 05:29:07PM +0100, Mark Brown wrote:

> > This seems like you have buggy constraints, constraints which allow
> > voltages that can't physically be satisfied don't make obvious sense.

> No, it's for cases like this (see the cubietruck4 or a80-optimus dts
> patches):

> 			reg_bldo4: bldo4 {
> 				regulator-min-microvolt = <1080000>;
> 				regulator-max-microvolt = <1320000>;
> 				regulator-name = "vcc12-hsic";
> 			};

> The regulator can do 1100000 uV, 1200000 uV and 1300000 uV, all of which
> are within the constraints, so obviously the constraints can be satisfied,
> yet the regulator core fails do so with a message like this:

Sure, but the constraints also say that you can do 1.32V which the
system is not physically capable of delivering.  That's not a good sign
for the constraints, it suggests that at least the capabilities of the
regulator have not been taken into consideration when setting up the
constraints.

[toc] | [prev] | [next] | [standalone]

#1468264 — Re: [PATCH 3/9] regulator: core: Try full range when adjusting regulators to constraints

On Tue, Aug 23, 2016 at 2:09 AM, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Aug 22, 2016 at 07:52:05PM +0200, Rask Ingemann Lambertsen wrote:
>> On Mon, Aug 22, 2016 at 05:29:07PM +0100, Mark Brown wrote:
>
>> > This seems like you have buggy constraints, constraints which allow
>> > voltages that can't physically be satisfied don't make obvious sense.
>
>> No, it's for cases like this (see the cubietruck4 or a80-optimus dts
>> patches):
>
>>                       reg_bldo4: bldo4 {
>>                               regulator-min-microvolt = <1080000>;
>>                               regulator-max-microvolt = <1320000>;
>>                               regulator-name = "vcc12-hsic";
>>                       };
>
>> The regulator can do 1100000 uV, 1200000 uV and 1300000 uV, all of which
>> are within the constraints, so obviously the constraints can be satisfied,
>> yet the regulator core fails do so with a message like this:
>
> Sure, but the constraints also say that you can do 1.32V which the
> system is not physically capable of delivering.  That's not a good sign
> for the constraints, it suggests that at least the capabilities of the
> regulator have not been taken into consideration when setting up the
> constraints.

So to be clear, the constraints should be the intersection of the
recommended operating parameters of the consumer and the regulator's
output, with the voltage/current steps taken in to consideration.

In that case I'll drop this patch and fix up the constraints.
And maybe send a patch to clarify the regulator bindings.

Thanks
ChenYu

[toc] | [prev] | [next] | [standalone]

#1468486 — Re: [PATCH 3/9] regulator: core: Try full range when adjusting regulators to constraints

[Multipart message — attachments visible in raw view] — view raw

On Tue, Aug 23, 2016 at 12:17:23PM +0800, Chen-Yu Tsai wrote:
> On Tue, Aug 23, 2016 at 2:09 AM, Mark Brown <broonie@kernel.org> wrote:

> > Sure, but the constraints also say that you can do 1.32V which the
> > system is not physically capable of delivering.  That's not a good sign
> > for the constraints, it suggests that at least the capabilities of the
> > regulator have not been taken into consideration when setting up the
> > constraints.

> So to be clear, the constraints should be the intersection of the
> recommended operating parameters of the consumer and the regulator's
> output, with the voltage/current steps taken in to consideration.

The constraints are there to say what the *system* can deliver.  That
includes the limitations of the consumers, the regulators and the
physical design of the board.  Just as one shouldn't just throw in the
maximum voltage range that the regulator can deliver one also shouldn't
just use the maximum voltage range a consumer can support for similar
reasons.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web