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 <[email protected]>
> ---
> 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