diff mbox series

[RFC,v1] regulator: pwm-regulator: Fix continuous get_voltage for disabled PWM

Message ID 20231221211222.1380658-1-martin.blumenstingl@googlemail.com
State Handled Elsewhere
Headers show
Series [RFC,v1] regulator: pwm-regulator: Fix continuous get_voltage for disabled PWM | expand

Commit Message

Martin Blumenstingl Dec. 21, 2023, 9:12 p.m. UTC
Odroid-C1 uses a Monolithic Power Systems MP2161 controlled via PWM for
the VDDEE voltage supply of the Meson8b SoC. Commit 6b9352f3f8a1 ("pwm:
meson: modify and simplify calculation in meson_pwm_get_state") results
in my Odroid-C1 crashing with memory corruption in many different places
(seemingly at random). It turns out that this is due to a currently not
supported corner case.

The VDDEE regulator can generate between 860mV (duty cycle of ~91%) and
1140mV (duty cycle of 0%). We consider it to be enabled by the bootloader
(which is why it has the regulator-boot-on flag in .dts) as well as
being always-on (which is why it has the regulator-always-on flag in
.dts) because the VDDEE voltage is required for the Meson8b SoC to work.
The public S805 datasheet [0] states on page 17 (where "A5" refers to the
Cortex-A5 CPU cores):
  [...] So if EE domains is shut off, A5 memory is also shut off. That
  does not matter. Before EE power domain is shut off, A5 should be shut
  off at first.

It turns out that at least some bootloader versions are keeping the PWM
output disabled. This is not a problem due to the specific design of the
regulator: when the PWM output is disabled the output pin is pulled LOW,
effectively achieving a 0% duty cycle (which in return means that VDDEE
voltage is at 1140mV).

The problem comes when the pwm-regulator driver tries to initialize the
PWM output. To do so it reads the current state from the hardware, which
is:
  period: 3666ns
  duty cycle: 3333ns (= ~91%)
  enabled: false
Then those values are translated using the continuous voltage range to
860mV.
Later, when the regulator is being enabled (either by the regulator core
due to the always-on flag or first consumer - in this case the lima
driver for the Mali-450 GPU) the pwm-regulator driver tries to keep the
voltage (at 860mV) and just enable the PWM output. This is when things
start to go wrong as the typical voltage used for VDDEE is 1100mV.

Commit 6b9352f3f8a1 ("pwm: meson: modify and simplify calculation in
meson_pwm_get_state") triggers above condition as before that change
period and duty cycle were both at 0. Since the change to the pwm-meson
driver is considered correct the solution is to be found in the
pwm-regulator driver which now considers the voltage to be at the
minimum or maximum (depending on whether the polarity is inverted) if
the PWM output is disabled. This makes the VDDEE regulator on Odroid-C1
read 1140mV while the PWM output is disabled, so all following steps try
to keep the 1140mV until any regulator consumer (such as the lima
driver's devfreq implementation) tries to set a different voltage
(1100mV is the target voltage).

[0] https://dn.odroid.com/S805/Datasheet/S805_Datasheet%20V0.8%2020150126.pdf

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
Sending this as RFC as I'm not 100% sure if this is the correct way to
solve my problem. Reverting commit 6b9352f3f8a1 (which I found via git
bisect) also works, but it seems hacky.

Once we agreed on the "correct" solution I will add Fixes tags as needed


 drivers/regulator/pwm-regulator.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Mark Brown Dec. 21, 2023, 9:45 p.m. UTC | #1
On Thu, Dec 21, 2023 at 10:12:22PM +0100, Martin Blumenstingl wrote:

> It turns out that at least some bootloader versions are keeping the PWM
> output disabled. This is not a problem due to the specific design of the
> regulator: when the PWM output is disabled the output pin is pulled LOW,
> effectively achieving a 0% duty cycle (which in return means that VDDEE
> voltage is at 1140mV).

Hrm.  Perhaps the regulator should figure out that it's on with a
minimum voltage of 1.14V in this case - AIUI that broadly corresponds to
your change except for the fact that it doesn't recognise that there's
actually an output in this case since it assumes that disabling the PWM
disables the output which isn't the case with this hardware.  We'd need
to know more about the PWM in that case though I think.

> The problem comes when the pwm-regulator driver tries to initialize the
> PWM output. To do so it reads the current state from the hardware, which
> is:
>   period: 3666ns
>   duty cycle: 3333ns (= ~91%)
>   enabled: false
> Then those values are translated using the continuous voltage range to
> 860mV.

> Later, when the regulator is being enabled (either by the regulator core
> due to the always-on flag or first consumer - in this case the lima
> driver for the Mali-450 GPU) the pwm-regulator driver tries to keep the
> voltage (at 860mV) and just enable the PWM output. This is when things
> start to go wrong as the typical voltage used for VDDEE is 1100mV.

So, the constraints say that the 860mV voltage is within range.  Where
does the requirement for 1.1V come from in this situation?  Is it just
that lima hasn't started yet and requires the 1.1V for hardware init
(and presumably power on) even if it can use a lower voltage at runtime?

> @@ -157,7 +157,12 @@ static int pwm_regulator_get_voltage(struct regulator_dev *rdev)

> -       voltage = pwm_get_relative_duty_cycle(&pstate, duty_unit);
> +       if (pstate.enabled)
> +               voltage = pwm_get_relative_duty_cycle(&pstate, duty_unit);
> +       else if (max_uV_duty < min_uV_duty)
> +               voltage = max_uV_duty;
> +       else
> +               voltage = min_uV_duty;

AFAICT this means that enabling the PWM changes the voltage read back
which isn't what we expect (other than a change from 0 to target) and is
likely to cause issues.  get_voltage() should not change after an
enable(), and indeed I'm unclear how this change works?  I'd expect a
change in the init_state() function, possibly one that programs the PWM
to reflect the actual hardware state but I'm not 100% confident on that
without digging into the PWM API more.
Uwe Kleine-König Dec. 21, 2023, 10:03 p.m. UTC | #2
On Thu, Dec 21, 2023 at 10:12:22PM +0100, Martin Blumenstingl wrote:
> Odroid-C1 uses a Monolithic Power Systems MP2161 controlled via PWM for
> the VDDEE voltage supply of the Meson8b SoC. Commit 6b9352f3f8a1 ("pwm:
> meson: modify and simplify calculation in meson_pwm_get_state") results
> in my Odroid-C1 crashing with memory corruption in many different places
> (seemingly at random). It turns out that this is due to a currently not
> supported corner case.
> 
> The VDDEE regulator can generate between 860mV (duty cycle of ~91%) and
> 1140mV (duty cycle of 0%). We consider it to be enabled by the bootloader
> (which is why it has the regulator-boot-on flag in .dts) as well as
> being always-on (which is why it has the regulator-always-on flag in
> .dts) because the VDDEE voltage is required for the Meson8b SoC to work.
> The public S805 datasheet [0] states on page 17 (where "A5" refers to the
> Cortex-A5 CPU cores):
>   [...] So if EE domains is shut off, A5 memory is also shut off. That
>   does not matter. Before EE power domain is shut off, A5 should be shut
>   off at first.
> 
> It turns out that at least some bootloader versions are keeping the PWM
> output disabled. This is not a problem due to the specific design of the
> regulator: when the PWM output is disabled the output pin is pulled LOW,
> effectively achieving a 0% duty cycle (which in return means that VDDEE
> voltage is at 1140mV).
> 
> The problem comes when the pwm-regulator driver tries to initialize the
> PWM output. To do so it reads the current state from the hardware, which
> is:
>   period: 3666ns
>   duty cycle: 3333ns (= ~91%)
>   enabled: false
> Then those values are translated using the continuous voltage range to
> 860mV.
> Later, when the regulator is being enabled (either by the regulator core
> due to the always-on flag or first consumer - in this case the lima
> driver for the Mali-450 GPU) the pwm-regulator driver tries to keep the
> voltage (at 860mV) and just enable the PWM output. This is when things
> start to go wrong as the typical voltage used for VDDEE is 1100mV.
> 
> Commit 6b9352f3f8a1 ("pwm: meson: modify and simplify calculation in
> meson_pwm_get_state") triggers above condition as before that change
> period and duty cycle were both at 0. Since the change to the pwm-meson
> driver is considered correct the solution is to be found in the
> pwm-regulator driver which now considers the voltage to be at the
> minimum or maximum (depending on whether the polarity is inverted) if
> the PWM output is disabled. This makes the VDDEE regulator on Odroid-C1
> read 1140mV while the PWM output is disabled, so all following steps try
> to keep the 1140mV until any regulator consumer (such as the lima
> driver's devfreq implementation) tries to set a different voltage
> (1100mV is the target voltage).
> 
> [0] https://dn.odroid.com/S805/Datasheet/S805_Datasheet%20V0.8%2020150126.pdf
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
> Sending this as RFC as I'm not 100% sure if this is the correct way to
> solve my problem. Reverting commit 6b9352f3f8a1 (which I found via git
> bisect) also works, but it seems hacky.
> 
> Once we agreed on the "correct" solution I will add Fixes tags as needed
> 
> 
>  drivers/regulator/pwm-regulator.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
> index 2aff6db748e2..30402ee18392 100644
> --- a/drivers/regulator/pwm-regulator.c
> +++ b/drivers/regulator/pwm-regulator.c
> @@ -157,7 +157,12 @@ static int pwm_regulator_get_voltage(struct regulator_dev *rdev)
>  
>  	pwm_get_state(drvdata->pwm, &pstate);
>  
> -	voltage = pwm_get_relative_duty_cycle(&pstate, duty_unit);
> +	if (pstate.enabled)
> +		voltage = pwm_get_relative_duty_cycle(&pstate, duty_unit);

That part looks fine. pwm_get_relative_duty_cycle() is only sensible for
an enabled PWM.

> +	else if (max_uV_duty < min_uV_duty)
> +		voltage = max_uV_duty;
> +	else
> +		voltage = min_uV_duty;

This could be abbreviated to:

	else
		voltage = min(max_uV_duty, min_uV_duty);

which you might find easier or harder to read.

Note this isn't save in general. You're implicitly assuming that a
disabled PWM runs with the minimal supported duty_cycle. Most disabled
PWMs yield the inactive level (which corresponds to a 0% relative duty
cycle). But there are exceptions. Also if your regulator has

	pwm-dutycycle-range = <20 80>;
	pwm-dutycycle-unit = <100>;

a 0% relative duty cycle yields an undefined voltage.

Without claiming to understand all implications, I'd say
pwm_regulator_get_voltage should signal to the caller when the
duty_cycle isn't contained in [min(max_uV_duty, min_uV_duty),
max(max_uV_duty, min_uV_duty)].

With that implemented, I'd just do:

	if (pstate.enabled)
		voltage = pwm_get_relative_duty_cycle(&pstate, duty_unit);
	else
		/*
		 * We're assuming here that a disabled PWM yields a 0%
		 * relative duty cycle. This isn't true in general
		 * however. Maybe issue a warning here?!
		 */
		voltage = 0;

Best regards
Uwe
Uwe Kleine-König Dec. 21, 2023, 10:07 p.m. UTC | #3
On Thu, Dec 21, 2023 at 09:45:49PM +0000, Mark Brown wrote:
> On Thu, Dec 21, 2023 at 10:12:22PM +0100, Martin Blumenstingl wrote:
> 
> > It turns out that at least some bootloader versions are keeping the PWM
> > output disabled. This is not a problem due to the specific design of the
> > regulator: when the PWM output is disabled the output pin is pulled LOW,
> > effectively achieving a 0% duty cycle (which in return means that VDDEE
> > voltage is at 1140mV).
> 
> Hrm.  Perhaps the regulator should figure out that it's on with a
> minimum voltage of 1.14V in this case - AIUI that broadly corresponds to
> your change except for the fact that it doesn't recognise that there's
> actually an output in this case since it assumes that disabling the PWM
> disables the output which isn't the case with this hardware.  We'd need
> to know more about the PWM in that case though I think.
> 
> > The problem comes when the pwm-regulator driver tries to initialize the
> > PWM output. To do so it reads the current state from the hardware, which
> > is:
> >   period: 3666ns
> >   duty cycle: 3333ns (= ~91%)
> >   enabled: false
> > Then those values are translated using the continuous voltage range to
> > 860mV.
> 
> > Later, when the regulator is being enabled (either by the regulator core
> > due to the always-on flag or first consumer - in this case the lima
> > driver for the Mali-450 GPU) the pwm-regulator driver tries to keep the
> > voltage (at 860mV) and just enable the PWM output. This is when things
> > start to go wrong as the typical voltage used for VDDEE is 1100mV.
> 
> So, the constraints say that the 860mV voltage is within range.  Where
> does the requirement for 1.1V come from in this situation?  Is it just
> that lima hasn't started yet and requires the 1.1V for hardware init
> (and presumably power on) even if it can use a lower voltage at runtime?
> 
> > @@ -157,7 +157,12 @@ static int pwm_regulator_get_voltage(struct regulator_dev *rdev)
> 
> > -       voltage = pwm_get_relative_duty_cycle(&pstate, duty_unit);
> > +       if (pstate.enabled)
> > +               voltage = pwm_get_relative_duty_cycle(&pstate, duty_unit);
> > +       else if (max_uV_duty < min_uV_duty)
> > +               voltage = max_uV_duty;
> > +       else
> > +               voltage = min_uV_duty;
> 
> AFAICT this means that enabling the PWM changes the voltage read back
> which isn't what we expect (other than a change from 0 to target) and is
> likely to cause issues.  get_voltage() should not change after an
> enable(), and indeed I'm unclear how this change works?  I'd expect a
> change in the init_state() function, possibly one that programs the PWM
> to reflect the actual hardware state but I'm not 100% confident on that
> without digging into the PWM API more.

What is your question here? Looking at pwm_regulator_set_voltage() I
think this lacks a

	pstate.enabled = true;

which might also fix Martin's problem?

Best regards
Uwe
Martin Blumenstingl Dec. 21, 2023, 10:42 p.m. UTC | #4
Hi Mark,

On Thu, Dec 21, 2023 at 10:45 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Dec 21, 2023 at 10:12:22PM +0100, Martin Blumenstingl wrote:
>
> > It turns out that at least some bootloader versions are keeping the PWM
> > output disabled. This is not a problem due to the specific design of the
> > regulator: when the PWM output is disabled the output pin is pulled LOW,
> > effectively achieving a 0% duty cycle (which in return means that VDDEE
> > voltage is at 1140mV).
>
> Hrm.  Perhaps the regulator should figure out that it's on with a
> minimum voltage of 1.14V in this case - AIUI that broadly corresponds to
> your change except for the fact that it doesn't recognise that there's
> actually an output in this case since it assumes that disabling the PWM
> disables the output which isn't the case with this hardware.  We'd need
> to know more about the PWM in that case though I think.
If you have any specific questions then feel free to ask.
Generally it's a very simple PWM controller:
- when disabled the output is LOW
- when enabled the output matches the requested period and duty cycle
as best as possible (depending on the available input clocks)

> > The problem comes when the pwm-regulator driver tries to initialize the
> > PWM output. To do so it reads the current state from the hardware, which
> > is:
> >   period: 3666ns
> >   duty cycle: 3333ns (= ~91%)
> >   enabled: false
> > Then those values are translated using the continuous voltage range to
> > 860mV.
>
> > Later, when the regulator is being enabled (either by the regulator core
> > due to the always-on flag or first consumer - in this case the lima
> > driver for the Mali-450 GPU) the pwm-regulator driver tries to keep the
> > voltage (at 860mV) and just enable the PWM output. This is when things
> > start to go wrong as the typical voltage used for VDDEE is 1100mV.
>
> So, the constraints say that the 860mV voltage is within range.  Where
> does the requirement for 1.1V come from in this situation?  Is it just
> that lima hasn't started yet and requires the 1.1V for hardware init
> (and presumably power on) even if it can use a lower voltage at runtime?
The vendor BSP includes a custom u-boot with lots of relevant
information for which there's seemingly no documentation.
It seems that 1.1V is what should be used during normal operation.
0.86V is what can be used during system suspend (when power to the
Cortex-A5 cores is turned off and an integrated ARC core is taking
over for wakeup purposes).
Hence the supported voltage range of 0.86..1.1V


Best regards,
Martin
Martin Blumenstingl Dec. 22, 2023, 12:17 a.m. UTC | #5
Hi Uwe,

On Thu, Dec 21, 2023 at 11:03 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
[...]
> Note this isn't save in general. You're implicitly assuming that a
> disabled PWM runs with the minimal supported duty_cycle. Most disabled
> PWMs yield the inactive level (which corresponds to a 0% relative duty
> cycle). But there are exceptions.
Good catch - thank you!

[...]
> Without claiming to understand all implications, I'd say
> pwm_regulator_get_voltage should signal to the caller when the
> duty_cycle isn't contained in [min(max_uV_duty, min_uV_duty),
> max(max_uV_duty, min_uV_duty)].
It seems like there's -ENOTRECOVERABLE that we can signal to the caller.
This makes the following message appear in my kernel log:
  VDDEE: Setting 1100000-1140000uV
Which means that pwm_regulator_set_voltage() is called which will then
pick the minimum voltage.

To make this work I will need to update meson8b-odroidc1.dts (which
isn't a problem, I just want to point it out as it's mandatory for
that solution. also I will send that in a separate patch).

See my attached patch (which replaces the initial patch I sent, it's
not meant to be applied on top).
One question that I still have is whether we are allowed to just
enable the PWM output within pwm_regulator_set_voltage().
Doing so is actually mandatory, otherwise we end up in an infinite
loop of not being able to read the voltage, then sets the minimum
voltage (but leaves the PWM output disabled) which then means that it
can't read back the voltage which means it tries to set the minimum
voltage ....


Best regards,
Martin
Uwe Kleine-König Dec. 22, 2023, 7:10 a.m. UTC | #6
Good morning Martin,

On Fri, Dec 22, 2023 at 01:17:44AM +0100, Martin Blumenstingl wrote:
> On Thu, Dec 21, 2023 at 11:03 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> [...]
> > Note this isn't save in general. You're implicitly assuming that a
> > disabled PWM runs with the minimal supported duty_cycle. Most disabled
> > PWMs yield the inactive level (which corresponds to a 0% relative duty
> > cycle). But there are exceptions.
> Good catch - thank you!
> 
> [...]
> > Without claiming to understand all implications, I'd say
> > pwm_regulator_get_voltage should signal to the caller when the
> > duty_cycle isn't contained in [min(max_uV_duty, min_uV_duty),
> > max(max_uV_duty, min_uV_duty)].
> It seems like there's -ENOTRECOVERABLE that we can signal to the caller.
> This makes the following message appear in my kernel log:
>   VDDEE: Setting 1100000-1140000uV
> Which means that pwm_regulator_set_voltage() is called which will then
> pick the minimum voltage.
> 
> To make this work I will need to update meson8b-odroidc1.dts (which
> isn't a problem, I just want to point it out as it's mandatory for
> that solution. also I will send that in a separate patch).
> 
> See my attached patch (which replaces the initial patch I sent, it's
> not meant to be applied on top).
> One question that I still have is whether we are allowed to just
> enable the PWM output within pwm_regulator_set_voltage().
> Doing so is actually mandatory, otherwise we end up in an infinite
> loop of not being able to read the voltage, then sets the minimum
> voltage (but leaves the PWM output disabled) which then means that it
> can't read back the voltage which means it tries to set the minimum
> voltage ....

Without enabling the PWM pwm_regulator_set_voltage() doesn't work if the
PWM isn't enabled already when the function is entered. So I'd say yes,
you must enable it.

> diff --git a/arch/arm/boot/dts/amlogic/meson8b-odroidc1.dts b/arch/arm/boot/dts/amlogic/meson8b-odroidc1.dts
> index b03273d90ad8..df348e119643 100644
> --- a/arch/arm/boot/dts/amlogic/meson8b-odroidc1.dts
> +++ b/arch/arm/boot/dts/amlogic/meson8b-odroidc1.dts
> @@ -217,13 +217,13 @@ vddee: regulator-vddee {
>  		compatible = "pwm-regulator";
>  
>  		regulator-name = "VDDEE";
> -		regulator-min-microvolt = <860000>;
> +		regulator-min-microvolt = <1100000>;
>  		regulator-max-microvolt = <1140000>;
>  
>  		pwm-supply = <&p5v0>;
>  
>  		pwms = <&pwm_cd 1 12218 0>;
> -		pwm-dutycycle-range = <91 0>;
> +		pwm-dutycycle-range = <14 0>;

This is ugly. You have to take away the description that a relative
duty-cycle of 91% yields 860 mV just because the linux driver behaves in
a certain way.

Also the calculation is wrong: If a relative duty-cyle in the interval
[91%; 0%] maps lineary to [860 mV; 1140 mV] you get 1100 mV at

	     1100 mV - 860 mV
	91 + ---------------- * (0 - 91) = 13
	     1140 mV - 860 mV

(If the calculations in the driver used signed multiplication and
division, all the checks for max_uV_duty < min_uV_duty could just go
away.)

So you want

+		pwm-dutycycle-range = <13 0>;

(if this restriction is really necessary).

>  		regulator-boot-on;
>  		regulator-always-on;

> diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
> index 30402ee18392..cb4e5fad5702 100644
> --- a/drivers/regulator/pwm-regulator.c
> +++ b/drivers/regulator/pwm-regulator.c
> @@ -156,13 +156,10 @@ static int pwm_regulator_get_voltage(struct regulator_dev *rdev)
>  	unsigned int voltage;
>  
>  	pwm_get_state(drvdata->pwm, &pstate);
> +	if (!pstate.enabled)
> +		return -ENOTRECOVERABLE;

This is good.

>  
> -	if (pstate.enabled)
> -		voltage = pwm_get_relative_duty_cycle(&pstate, duty_unit);
> -	else if (max_uV_duty < min_uV_duty)
> -		voltage = max_uV_duty;
> -	else
> -		voltage = min_uV_duty;
> +	voltage = pwm_get_relative_duty_cycle(&pstate, duty_unit);

I'd add here:

	if (voltage < min(max_uV_duty, min_uV_duty) ||
	    voltage > max(max_uV_duty, min_uV_duty))
		return -ENOTRECOVERABLE;

>  	/*
>  	 * The dutycycle for min_uV might be greater than the one for max_uV.
> @@ -221,6 +218,7 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
>  
>  	pwm_set_relative_duty_cycle(&pstate, dutycycle, duty_unit);
>  
> +	pstate.enabled = true;
>  	ret = pwm_apply_state(drvdata->pwm, &pstate);
>  	if (ret) {
>  		dev_err(&rdev->dev, "Failed to configure PWM: %d\n", ret);

Otherwise the change to the driver looks fine to me.

Best regards
Uwe
Martin Blumenstingl Dec. 22, 2023, 10:12 a.m. UTC | #7
Hello Uwe,

On Fri, Dec 22, 2023 at 8:10 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
[...]
> Also the calculation is wrong: If a relative duty-cyle in the interval
> [91%; 0%] maps lineary to [860 mV; 1140 mV] you get 1100 mV at
>
>              1100 mV - 860 mV
>         91 + ---------------- * (0 - 91) = 13
>              1140 mV - 860 mV
>
> (If the calculations in the driver used signed multiplication and
> division, all the checks for max_uV_duty < min_uV_duty could just go
> away.)
>
> So you want
>
> +               pwm-dutycycle-range = <13 0>;
Thank you!

> (if this restriction is really necessary).
I could not find a way around this.
Without this change pwm_regulator_set_voltage() is called with req_min
860mV and req_max 1140mV.
pwm_regulator_set_voltage() will then pick the lowest possible
voltage, which then results in 860mV (exactly what I get without any
patches).

To be able to keep the original minimum voltage in .dts would be to
work on what Mark suggested where he said:
"I'd expect a change in the init_state() function, possibly one that
programs the PWM to reflect the actual hardware state"

[...]
> > -     if (pstate.enabled)
> > -             voltage = pwm_get_relative_duty_cycle(&pstate, duty_unit);
> > -     else if (max_uV_duty < min_uV_duty)
> > -             voltage = max_uV_duty;
> > -     else
> > -             voltage = min_uV_duty;
> > +     voltage = pwm_get_relative_duty_cycle(&pstate, duty_unit);
>
> I'd add here:
>
>         if (voltage < min(max_uV_duty, min_uV_duty) ||
>             voltage > max(max_uV_duty, min_uV_duty))
>                 return -ENOTRECOVERABLE;
I can do that - although I think it should be a separate change.


Best regards,
Martin
Uwe Kleine-König Dec. 22, 2023, 10:27 a.m. UTC | #8
On Fri, Dec 22, 2023 at 11:12:30AM +0100, Martin Blumenstingl wrote:
> Hello Uwe,
> 
> On Fri, Dec 22, 2023 at 8:10 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> [...]
> > Also the calculation is wrong: If a relative duty-cyle in the interval
> > [91%; 0%] maps lineary to [860 mV; 1140 mV] you get 1100 mV at
> >
> >              1100 mV - 860 mV
> >         91 + ---------------- * (0 - 91) = 13
> >              1140 mV - 860 mV
> >
> > (If the calculations in the driver used signed multiplication and
> > division, all the checks for max_uV_duty < min_uV_duty could just go
> > away.)
> >
> > So you want
> >
> > +               pwm-dutycycle-range = <13 0>;
> Thank you!
> 
> > (if this restriction is really necessary).
> I could not find a way around this.
> Without this change pwm_regulator_set_voltage() is called with req_min
> 860mV and req_max 1140mV.
> pwm_regulator_set_voltage() will then pick the lowest possible
> voltage, which then results in 860mV (exactly what I get without any
> patches).
> 
> To be able to keep the original minimum voltage in .dts would be to
> work on what Mark suggested where he said:
> "I'd expect a change in the init_state() function, possibly one that
> programs the PWM to reflect the actual hardware state"
> 
> [...]
> > > -     if (pstate.enabled)
> > > -             voltage = pwm_get_relative_duty_cycle(&pstate, duty_unit);
> > > -     else if (max_uV_duty < min_uV_duty)
> > > -             voltage = max_uV_duty;
> > > -     else
> > > -             voltage = min_uV_duty;
> > > +     voltage = pwm_get_relative_duty_cycle(&pstate, duty_unit);
> >
> > I'd add here:
> >
> >         if (voltage < min(max_uV_duty, min_uV_duty) ||
> >             voltage > max(max_uV_duty, min_uV_duty))
> >                 return -ENOTRECOVERABLE;
> I can do that - although I think it should be a separate change.

That can go in together with the

	if (pstate.enabled)
		return -ENOTRECOVERABLE;

Otherwise +1 to not mix that with the machine specfic stuff.

Best regards
Uwe
Mark Brown Dec. 22, 2023, 11:53 a.m. UTC | #9
On Thu, Dec 21, 2023 at 11:07:41PM +0100, Uwe Kleine-König wrote:
> On Thu, Dec 21, 2023 at 09:45:49PM +0000, Mark Brown wrote:

> > > -       voltage = pwm_get_relative_duty_cycle(&pstate, duty_unit);
> > > +       if (pstate.enabled)
> > > +               voltage = pwm_get_relative_duty_cycle(&pstate, duty_unit);
> > > +       else if (max_uV_duty < min_uV_duty)
> > > +               voltage = max_uV_duty;
> > > +       else
> > > +               voltage = min_uV_duty;

> > AFAICT this means that enabling the PWM changes the voltage read back
> > which isn't what we expect (other than a change from 0 to target) and is
> > likely to cause issues.  get_voltage() should not change after an
> > enable(), and indeed I'm unclear how this change works?  I'd expect a
> > change in the init_state() function, possibly one that programs the PWM
> > to reflect the actual hardware state but I'm not 100% confident on that
> > without digging into the PWM API more.

> What is your question here? Looking at pwm_regulator_set_voltage() I
> think this lacks a

> 	pstate.enabled = true;

> which might also fix Martin's problem?

That's not really a question, it's a statement - I don't see how the
change works at all and as it stands it introduces a problem with the
behaviour when the regulator is enabled.
Mark Brown Dec. 22, 2023, 11:54 a.m. UTC | #10
On Thu, Dec 21, 2023 at 11:42:29PM +0100, Martin Blumenstingl wrote:
> On Thu, Dec 21, 2023 at 10:45 PM Mark Brown <broonie@kernel.org> wrote:

> > Hrm.  Perhaps the regulator should figure out that it's on with a
> > minimum voltage of 1.14V in this case - AIUI that broadly corresponds to
> > your change except for the fact that it doesn't recognise that there's
> > actually an output in this case since it assumes that disabling the PWM
> > disables the output which isn't the case with this hardware.  We'd need
> > to know more about the PWM in that case though I think.

> If you have any specific questions then feel free to ask.
> Generally it's a very simple PWM controller:
> - when disabled the output is LOW
> - when enabled the output matches the requested period and duty cycle
> as best as possible (depending on the available input clocks)

We would need to know more at runtime to do the correct thing I think.
Mark Brown Dec. 22, 2023, 12:24 p.m. UTC | #11
On Thu, Dec 21, 2023 at 11:42:29PM +0100, Martin Blumenstingl wrote:

> The vendor BSP includes a custom u-boot with lots of relevant
> information for which there's seemingly no documentation.
> It seems that 1.1V is what should be used during normal operation.
> 0.86V is what can be used during system suspend (when power to the
> Cortex-A5 cores is turned off and an integrated ARC core is taking
> over for wakeup purposes).
> Hence the supported voltage range of 0.86..1.1V

That sounds like the constraints are wrong actually, if 0.86V can only
be used during system suspend then it shouldn't be in the valid voltage
range - the suspend voltage doesn't need to be in the range used during
normal operation.  If we might use it during runtime suspend then it
does need to be there though.
diff mbox series

Patch

diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
index 2aff6db748e2..30402ee18392 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -157,7 +157,12 @@  static int pwm_regulator_get_voltage(struct regulator_dev *rdev)
 
 	pwm_get_state(drvdata->pwm, &pstate);
 
-	voltage = pwm_get_relative_duty_cycle(&pstate, duty_unit);
+	if (pstate.enabled)
+		voltage = pwm_get_relative_duty_cycle(&pstate, duty_unit);
+	else if (max_uV_duty < min_uV_duty)
+		voltage = max_uV_duty;
+	else
+		voltage = min_uV_duty;
 
 	/*
 	 * The dutycycle for min_uV might be greater than the one for max_uV.