Message-ID: <tek6c6symqgm6x6ujh4m67q32en24pzrkjbchffir7qljo4gor@7qpu4zmgyzpq>
Date: Thu, 21 Dec 2023 23:03:00 +0100
From: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To: Martin Blumenstingl <martin.blumenstingl@...glemail.com>
Cc: lgirdwood@...il.com, broonie@...nel.org, linux-pwm@...r.kernel.org, 
	linux-amlogic@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	Thierry Reding <thierry.reding@...il.com>, Heiner Kallweit <hkallweit1@...il.com>, 
	Dmitry Rokosov <ddrokosov@...rdevices.ru>
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@...glemail.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/ |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists