Re: [RFC PATCH] regulator: pwm-regulator: Make assumptions about disabled PWM consistent

[Uwe Kleine-König]

Hello Martin,

On Sun, Jun 16, 2024 at 01:10:32AM +0200, Martin Blumenstingl wrote:
> On Mon, Jun 10, 2024 at 12:46 PM Uwe Kleine-König
> <u.kleine-koenig@xxxxxxxxxxxx> wrote:
> [...]
> > this is my suggestion to fix the concerns I expressed in
> > https://lore.kernel.org/all/hf24mrplr76xtziztkqiscowkh2f3vmceuarecqcwnr6udggs6@uiof2rvvqq5v/
> > .
> >
> > It's only compile tested as I don't have a machine with a pwm-regulator.
> > So getting test feedback before applying it would be great.
> Unfortunately this approach breaks my Odroid-C1 board again with the
> same symptoms as before the mentioned commits: random memory
> corruption, preventing the board from booting to userspace.
> 
> The cause also seems to be the same as before my commits:
> - period (3666ns) and duty cycle (3333ns) in the hardware registers
> the PWM controller when Linux boots, but the PWM output is disabled
> - with a duty cycle of 0 or PWM output being disabled the output of
> the voltage regulator is 1140mV, which is the only allowed voltage for
> that rail (even though the regulator can achieve other voltages)
> - pwm_regulator_enable() calls pwm_enable() which simply takes the
> period and and duty cycle that was read back from the hardware and
> enables the output, resulting in undervolting of a main voltage rail
> of the SoC

Ah, indeed. pwm_enable() looks so innocent, but in fact the details are
difficult. One more reason to drop that caching of parameters in the pwm
core.

> I hope that this (especially the last item) also clarifies the
> question you had in the linked mail on whether updating
> pwm_regulator_init_state() would help/work.
> 
> Regarding your alternative and preferred approach from the other mail:
> > 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.
> I tried this to see if it would work also for my Odroid-C1 board and
> I'm happy to report it does - see the attached diff.
> In case you are happy with this approach I can submit it as a proper patch.

Yes, I like it. From a quick glance, I only wonder about the dropped
error message in pwm_regulator_set_voltage(). Probably it's right that
this function is silent, but then this is orthogonal to the patch
we're discussing and should go in a separate patch.

Thanks for your valuable cooperation.

Best regards
Uwe

Attachment: signature.asc
Description: PGP signature