Hello Uwe,
Am Freitag, 6. Mai 2022, 12:23:01 CEST schrieb Uwe Kleine-König:
> * PGP Signed by an unknown key
>
> On Fri, May 06, 2022 at 10:35:21AM +0200, Alexander Stein wrote:
> > Am Freitag, 6. Mai 2022, 10:20:01 CEST schrieb Uwe Kleine-König:
> > > > Old Signed by an unknown key
> > >
> > > Hello,
> > >
> > > On Fri, May 06, 2022 at 09:15:55AM +0200, Alexander Stein wrote:
> > > > Am Freitag, 6. Mai 2022, 00:00:37 CEST schrieb Guenter Roeck:
> > > > > On Wed, May 04, 2022 at 02:45:51PM +0200, Alexander Stein wrote:
> > > > > > From: Markus Niebel <Markus.Niebel@xxxxxxxxxxxxxxx>
> > > > > >
> > > > > > A pwm value equal to zero is meant to switch off the pwm
> > > > > > hence also switching off the fan. Currently the optional
> > > > > > regulator is always on. When using this driver on boards
> > > > > > with an inverted pwm signal polarity this can cause running
> > > > > > the fan at maximum speed when setting pwm to zero.
> > > > >
> > > > > The appropriate solution in this case would be to tell the
> > > > > software that the pwm is inverted. Turning off the regulator
> > > > > in that situation is a bad idea since setting the pwm value to
> > > > > 1 would set it to almost full speed. That does not really make
> > > > > sense.
> > > >
> > > > The pwm-fan driver is already configured for inverted PWM (ommited
> > > > some
> > > > properties for shortness):
> > > > fan0: pwm-fan {
> > > >
> > > > compatible = "pwm-fan";
> > > > fan-supply = <®_pwm_fan>;
> > > > pwms = <&pwm3 0 40000 PWM_POLARITY_INVERTED>;
> > > > cooling-levels = <0 32 64 128 196 240>;
> > > >
> > > > [...]
> > > > };
> > > >
> > > > The problem here is that the pwm-fan driver currently enables the
> > > > regulator
> > > > unconditionally, but the PWM only when the fan is enabled, refer to
> > > > __set_pwm(). This results in a fan at full speed when pwm-fan is idle,
> > > > as
> > > > pwm state is not enabled.
> > >
> > > Which PWM driver are you using?
> >
> > It's pwm-imx27 on a imx8mp based board.
>
> This is one of the known offenders.
>
> > > There is an implicit assumption in some PWM consumers that a disabled
> > > PWM emits the inactive level. However not all PWMs do this. Is this such
> > > a case?
> >
> > Oh, I was not aware of that assumption. As far I can tell, this assumption
> > might actually be violated in pwm-imx27.
>
> The pwm-imx27 driver is a known offender.
>
> IMHO the problem is that there is no properly documented definition what
> "disabled" means for a PWM. I had some discussions about that in the
> past with Thierry, but with no agreement. Either we have do define that
> the output of a PWM is undefined when it's disabled (then the pwm-fan
> needs fixing) or the output must be the inactive level (then the
> pwm-imx27 must be fixed to not unset the EN bit when configured for an
> inverted polarity). In my eyes the first is the sensible thing to do.
>
> See
> https://lore.kernel.org/linux-pwm/20180806155129.cjcc7okmwtaujf43@pengutroni
> x.de/ for one of the previous discussions.
Thanks for the link. I took a look into it. I'm on your side here, IMHO
pwm_disable() implies that the PWM perphery is disabled, including any clocks
or powerdomain. This is what pwm-imx27 actually does. This might lead to a,
probably platform dependent, (undefined?) state of the PWM output pin.
This implies it is not possible to disable the PWM periphery for inverted
signals, if the disabled state is not the inactive level. You know all about
it already.
Then again from pwm-fan side I want be able to disable the FAN, turning of
regulator and PWM, so powersaving is possible. That's what this patch is
about. This is similar also what pwm_bl is doing.
Independent of the exact semantics, it makes sense to disable the regulator in
pwm-fan as well when the fan shall be disabled.
> > If state->enabled==false then the EN Bit in PWMCR is not set which most
> > probably renders the output polarity in POUTC as inactive.
>
> It drives the output to 0 which in the inverted polarity case is a 100%
> relative duty.
Thanks for confirmation, this matches my observations.
Best regards,
Alexander
