Messages in this thread | | | Date | Thu, 21 Dec 2023 23:03:00 +0100 | From | Uwe Kleine-König <> | Subject | Re: [RFC PATCH v1] regulator: pwm-regulator: Fix continuous get_voltage for disabled PWM |
| |
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
-- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [unhandled content-type:application/pgp-signature] | |