Message-ID: <hf24mrplr76xtziztkqiscowkh2f3vmceuarecqcwnr6udggs6@uiof2rvvqq5v>
Date: Mon, 10 Jun 2024 12:01:41 +0200
From: Uwe Kleine-König <u.kleine-koenig@...libre.com>
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, 
	Heiner Kallweit <hkallweit1@...il.com>, Dmitry Rokosov <ddrokosov@...rdevices.ru>
Subject: Re: [RFC PATCH v2 3/3] regulator: pwm-regulator: Manage boot-on with
 disabled PWM channels

Hello Martin,

while doing some pwm related cleanup, I stumbled over the part of the
pwm-regulator driver that was added here. This is either wrong or I
don't understand it.

On Sat, Jan 13, 2024 at 11:46:28PM +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 generally 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.

Maybe pwm_regulator_init_state() needs a similar logic as
pwm_regulator_get_voltage() and then this patch's changes can (and
should) go away?

> 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. Update the duty cycle during driver probe if the
> regulator is flagged as boot-on so that a call to pwm_regulator_enable()
> (by the regulator core during initialization of a regulator flagged with
> boot-on) without any preceding call to pwm_regulator_set_voltage() does
> not change the output voltage.
> 
> [0] https://dn.odroid.com/S805/Datasheet/S805_Datasheet%20V0.8%2020150126.pdf
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@...glemail.com>
> ---
> Changes from v1 -> v2:
> - This patch is new and the idea is to keep the voltage regulator
>   description in device-tree as-is (which means: support for 860mV to
>   1140mV). Instead we allow the pwm-regulator driver to preserve the
>   output voltage at boot when enabling the regulator.
> - note: Mark Brown said in v1 "I'd expect a change in the init_state()
>   function". That function is not updated as it's only relevant for
>   regulators with a voltage table - on Odroid-C1 we have a continuous
>   regulator though.
> 
> 
>  drivers/regulator/pwm-regulator.c | 33 +++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
> index d27b9a7a30c9..60cfcd741c2a 100644
> --- a/drivers/regulator/pwm-regulator.c
> +++ b/drivers/regulator/pwm-regulator.c
> @@ -323,6 +323,32 @@ static int pwm_regulator_init_continuous(struct platform_device *pdev,
>  	return 0;
>  }
>  
> +static int pwm_regulator_init_boot_on(struct platform_device *pdev,
> +				      struct pwm_regulator_data *drvdata,
> +				      const struct regulator_init_data *init_data)
> +{
> +	struct pwm_state pstate;
> +
> +	if (!init_data->constraints.boot_on || drvdata->enb_gpio)
> +		return 0;
> +
> +	pwm_get_state(drvdata->pwm, &pstate);
> +	if (pstate.enabled)
> +		return 0;
> +
> +	/*
> +	 * Update the duty cycle so the output does not change
> +	 * when the regulator core enables the regulator (and
> +	 * thus the PWM channel).
> +	 */
> +	if (pstate.polarity == PWM_POLARITY_INVERSED)
> +		pstate.duty_cycle = pstate.period;
> +	else
> +		pstate.duty_cycle = 0;

This looks wrong. If you assume that a disabled PWM emits the inactive
level (which is wrong in general, but the best we can assume, and this
is already done in pwm_regulator_get_voltage() since patch #2 in this
series), and you want to call pwm_apply_state() without changing that,
you need to do pstate.duty_cycle = 0 unconditionally.

Looking at arch/arm/boot/dts/amlogic/meson8b-odroidc1.dts's &vddee I
guess you only tested the pstate.polarity == PWM_POLARITY_NORMAL case,
so didn't suffer from the problem above?!

> +	return pwm_apply_might_sleep(drvdata->pwm, &pstate);

This doesn't necessarily has an effect on the PWM. The HW was disabled
before and here we have pstate.enabled = false, too. So the only
reliable effect is that pwm_get_state() returns duty_cycle as set above.
That might be worth to be noted in a comment.

Alternatively (and IMHO nicer) just keep the pwm_state around and don't
use the (mis) feature of the PWM core that pwm_get_state only returns
the last set state.

> +}

Best regards
Uwe

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

Powered by blists - more mailing lists