Message-ID: <20231221211222.1380658-1-martin.blumenstingl@googlemail.com>
Date: Thu, 21 Dec 2023 22:12:22 +0100
From: Martin Blumenstingl <martin.blumenstingl@...glemail.com>
To: lgirdwood@...il.com,
	broonie@...nel.org,
	Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
	linux-pwm@...r.kernel.org,
	linux-amlogic@...ts.infradead.org
Cc: linux-kernel@...r.kernel.org,
	Thierry Reding <thierry.reding@...il.com>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Dmitry Rokosov <ddrokosov@...rdevices.ru>,
	Martin Blumenstingl <martin.blumenstingl@...glemail.com>
Subject: [RFC PATCH v1] regulator: pwm-regulator: Fix continuous get_voltage for disabled PWM

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);
+	else if (max_uV_duty < min_uV_duty)
+		voltage = max_uV_duty;
+	else
+		voltage = min_uV_duty;
 
 	/*
 	 * The dutycycle for min_uV might be greater than the one for max_uV.
-- 
2.43.0

Powered by blists - more mailing lists