Message-ID: <jx2vspy4cfuidf3mwmje675igspxdpgrenw5jz35ud5bm65klc@s4ovjad4fusg>
Date: Thu, 21 Dec 2023 23:07:41 +0100
From: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To: Mark Brown <broonie@...nel.org>
Cc: Martin Blumenstingl <martin.blumenstingl@...glemail.com>, 
	lgirdwood@...il.com, linux-pwm@...r.kernel.org, linux-amlogic@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, Thierry Reding <thierry.reding@...il.com>, 
	Heiner Kallweit <hkallweit1@...il.com>, Dmitry Rokosov <ddrokosov@...rdevices.ru>
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/ |

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

Powered by blists - more mailing lists