Messages in this thread Patch in this message | ![/](/images/icornerl.gif) | | Date | Thu, 21 Dec 2023 23:07:41 +0100 | From | Uwe Kleine-König <> | Subject | Re: [RFC PATCH v1] regulator: pwm-regulator: Fix continuous get_voltage for disabled PWM |
| |
On Thu, Dec 21, 2023 at 09:45:49PM +0000, Mark Brown wrote: > On Thu, Dec 21, 2023 at 10:12:22PM +0100, Martin Blumenstingl wrote: > > > 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). > > Hrm. Perhaps the regulator should figure out that it's on with a > minimum voltage of 1.14V in this case - AIUI that broadly corresponds to > your change except for the fact that it doesn't recognise that there's > actually an output in this case since it assumes that disabling the PWM > disables the output which isn't the case with this hardware. We'd need > to know more about the PWM in that case though I think. > > > 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. > > So, the constraints say that the 860mV voltage is within range. Where > does the requirement for 1.1V come from in this situation? Is it just > that lima hasn't started yet and requires the 1.1V for hardware init > (and presumably power on) even if it can use a lower voltage at runtime? > > > @@ -157,7 +157,12 @@ static int pwm_regulator_get_voltage(struct regulator_dev *rdev) > > > - 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; > > AFAICT this means that enabling the PWM changes the voltage read back > which isn't what we expect (other than a change from 0 to target) and is > likely to cause issues. get_voltage() should not change after an > enable(), and indeed I'm unclear how this change works? I'd expect a > change in the init_state() function, possibly one that programs the PWM > to reflect the actual hardware state but I'm not 100% confident on that > without digging into the PWM API more.
What is your question here? Looking at pwm_regulator_set_voltage() I think this lacks a
pstate.enabled = true;
which might also fix Martin's problem?
Best regards Uwe
-- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [unhandled content-type:application/pgp-signature] | ![\](/images/icornerr.gif) |