Received-SPF: pass (google.com: domain of linux-kernel+bounces-207981-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Mon, 10 Jun 2024 12:01:41 +0200
From: Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= <u.kleine-koenig@baylibre.com>
To: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc: lgirdwood@gmail.com, broonie@kernel.org, linux-pwm@vger.kernel.org, 
	linux-amlogic@lists.infradead.org, linux-kernel@vger.kernel.org, 
	Heiner Kallweit <hkallweit1@gmail.com>, Dmitry Rokosov <ddrokosov@sberdevices.ru>
Subject: Re: [RFC PATCH v2 3/3] regulator: pwm-regulator: Manage boot-on with
 disabled PWM channels
Message-ID: <hf24mrplr76xtziztkqiscowkh2f3vmceuarecqcwnr6udggs6@uiof2rvvqq5v>
References: <20240113224628.377993-1-martin.blumenstingl@googlemail.com>
 <20240113224628.377993-4-martin.blumenstingl@googlemail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="4hr62idwwdqmqmca"
Content-Disposition: inline
In-Reply-To: <20240113224628.377993-4-martin.blumenstingl@googlemail.com>


--4hr62idwwdqmqmca
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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.
>=20
> 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.
>=20
> 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).
>=20
> 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 (=3D ~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.
>=20
> 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.
>=20
> [0] https://dn.odroid.com/S805/Datasheet/S805_Datasheet%20V0.8%2020150126=
=2Epdf
>=20
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.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.
>=20
>=20
>  drivers/regulator/pwm-regulator.c | 33 +++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>=20
> diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-re=
gulator.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 plat=
form_device *pdev,
>  	return 0;
>  }
> =20
> +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 =3D=3D PWM_POLARITY_INVERSED)
> +		pstate.duty_cycle =3D pstate.period;
> +	else
> +		pstate.duty_cycle =3D 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 =3D 0 unconditionally.

Looking at arch/arm/boot/dts/amlogic/meson8b-odroidc1.dts's &vddee I
guess you only tested the pstate.polarity =3D=3D 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 =3D 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

--4hr62idwwdqmqmca
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEP4GsaTp6HlmJrf7Tj4D7WH0S/k4FAmZmzwAACgkQj4D7WH0S
/k6jqwf/YKPLdNCMffFnFQXY40SmaMzyxtujekyudUsSX8Pjr7R8h5oN5RZsPyEp
MUWEal/azCoDBcYFx2lDstfKaJNuEVQVPxkTuVcxvys/JsS0g6H523DSfLLsR/AT
5Uipi9rdYvpYvL56SL2CGZv0UpANVOSf8bjo1kiTkfpM1Ss/3Xm2ftHxFzlrg78j
uozemerDpVpuI5RPBTbToPxjTSlp5Vm1KBWTdIffJuHDItMPdr53jWa2qr/sTfNw
t1sZdVgD2285zBmh0wOdkp9HbR3uDpNnqNtd1PH+v3i4ErOT2soyM760/TQS7Ymd
lMcfOQXx3rC+3vleHaTwyVcQGKYpSQ==
=jaEU
-----END PGP SIGNATURE-----

--4hr62idwwdqmqmca--