Re: (EXT) Re: (EXT) Re: (EXT) Re: [PATCH v2 1/1] hwmon: pwm-fan: dynamically switch regulator

[Alexander Stein <alexander.stein@ew.tq-group.com>]

Hello Uwe,

Am Montag, 9. Mai 2022, 12:59:21 CEST schrieb Uwe Kleine-König:
> * PGP Signed by an unknown key
> 
> Hello Alexander, hello Guenter,
> 
> On Mon, May 09, 2022 at 09:39:15AM +0200, Alexander Stein wrote:
> > Am Freitag, 6. Mai 2022, 20:31:24 CEST schrieb Guenter Roeck:
> > > On Fri, May 06, 2022 at 04:29:13PM +0200, Uwe Kleine-König wrote:
> > > > [Dropped Bartlomiej Zolnierkiewicz from Cc:; my mailer daemon claims
> > > > the
> > > > email address doens't exist.]
> > > > 
> > > > Hello Guenter,
> > > > 
> > > > On Fri, May 06, 2022 at 07:12:44AM -0700, Guenter Roeck wrote:
> > > > > On Fri, May 06, 2022 at 02:23:11PM +0200, Alexander Stein wrote:
> > > > > > Am Freitag, 6. Mai 2022, 12:23:01 CEST schrieb Uwe Kleine-König:
> > > > > > > See
> > > > > > > https://lore.kernel.org/linux-pwm/20180806155129.cjcc7okmwtaujf4
> > > > > > > 3@pe
> > > > > > > ngutronix.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.
> > > > > 
> > > > > There are fans which never stop if pwm==0, such as some CPU fans. I
> > > > > don't
> > > > 
> > > > I assume with pwm==0 you actually mean duty_cycle == 0?
> > > 
> > > Correct. The "pwm" attribute sets the duty cycle.
> > > 
> > > > > think it is a good idea to force those off by turning off their
> > > > > power.
> > > > > The
> > > > > problem in the driver is that it treats pwm==0 as "disable pwm", not
> > > > > as
> > > > > "set pwm output to 0", Part of the probem may be that the ABI
> > > > > doesn't
> > > > > have
> > > > > a good representation for "disable pwm output", which is what is
> > > > > really
> > > > > wanted/needed here.
> > > > 
> > > > Disable pwm output == set pwm output to High-Z? Not all PWMs are able
> > > > to
> > > > provide that.
> > > 
> > > It is up to us to define whate it means exactly. If you are ok that "set
> > > duty cycle to 0" reflects "set duty cycle to 0, disable pwm, and turn
> > > off
> > > regulator", I would hope that you are ok with using the _enable
> > > attribute
> > > to do the same and leaving pwm==0 to do what it is supposed to do, ie to
> > > keep pwm control enabled and set the duty cycle to 0.
> > 
> > Just to make sure to be on the same side and summarize a bit. What you
> > mean is to add a new sysfs attribute to pwm-fan driver which controls
> > what pwm_duty==0 implies. I would suggest to name is 'keep_pwm_enabled'
> > (but I am open for other suggestions) with the following meaning:
> > 1 - pwm_duty==0 just means that. Set PWM duty to 0 and keep PWM (and fan
> > regulator) enabled.
> > 
> > 0 - pwm_duty==0 means that the PWM duty is set to 0, PWM is disabled and
> > any PWM fan regulator is disabled as well.
> 
> I'm not convinced. A property that is called "keep_pwm_enabled"
> shouldn't have any effect on the regulator. Maybe we need two
> properties, one for the PWM and one for the regulator?

Good point, that name might be misleading. 'keep_fan_enabled' or 
'keep_device_enabled' comes to might mind right now. The intention is to 
switch both PWM and regulator simultaneously, or not.
But having two orthogonal properties don't make sense to me.

regulator |   pwm   | comment
----------+---------+--------
 disable  | disable | keep_device_enabled=0
 disable  |  enable | does not make sense for me.
 enable   | disable | Current state, which is not correct in some cases
 enable   |  enable | keep_device_enabled=1

That's why I would keep a single property, but I might be missing something.
If there is no regulator at all, then keep_device_enabled=0 is the same as 
what currently happens.

> > With this it is up to the administrator to provide the correct setting for
> > this attribute as it highly depends on the actual hardware and/or usage.
> 
> I wonder if that is a devicetree (or firmware) property instead of
> a sysfs knob.
> 
> A related problem is that there is no official definition for the PWM
> framework what a consumer can expect from a disabled PWM, and some have
> adopted the expectation "constant inactive output" as this is what
> several lowlevel implementations provide.

I guess there are examples for both a) and b). pwm_bl disables both PWM and 
regulator in pwm_backlight_power_off(). AFAICS imx6dl-yapp4-common.dtsi 
assumes pwm duty 0 means to disable both regulator and PWM, due to inverted 
polarity and pwm-imx27 specific behavior.

> The two obvious candidates for such an expectation are:
> 
>  a) emit the inactive level
>  b) no guarantees about the output
> 
> I think there should be an explicit definition and which of them is
> picked has an influence on the decision how to properly model a PWM fan.
> (If you say now the design of a device tree model shouldn't depend on
> what the Linux PWM framework considers as right behaviour for a disabled
> PWM, you're right. But you have to have some concept of "disabled PWM"
> and the thoughts to pick one definition or the other are the same, so
> it's sensible to come to the same conclusion for both formally different
> questions.)
> 
> I'm convinced that b) is the sane way to pick. The reasons are:
> 
>  - Some hardware cannot be disabled while emitting a constant 0 or 1.
>  - There is already a way for consumers to express: I want a constant
>    inactive output. (i.e. .duty_cycle = 0, .enabled = 1, .period = $big)

Assuming b) for this driver has the benefit that a) can also be implemented 
using the above mentioned knob. This configuration option is apparently needed 
anyway to specify if the PWM and regulator can be turned off or need to be 
kept enabled. IMHO if the PWM and regulator can be turned off, it should be 
possible to do so.

> When adopting b) there is some expressiveness missing though and that
> has to do with transitions to new configurations. If a PWM runs at 100%
> relative duty cycle (i.e. .duty_cycle == .period) and the consumer then
> calls
> 
> 	pwm_apply(mypwm, {.duty_cycle = 0, .period = 5000, .enabled = true 
})
> 
> and some time later
> 
> 	pwm_apply(mypwm, {.duty_cycle = 5000, .period = 5000, .enabled = 
true })
> 
> they might expect that the PWM is low for a duration that is a multiple
> of 5ms. However I think that doesn't have a practical relevance and
> quite a few PWM hardware implementations cannot complete a period on a
> configuration change anyhow. So I believe it's safe to disable a PWM
> after
> 
> 	pwm_apply(mypwm, {.duty_cycle = 0, .period = 5000, .enabled = true 
})
> 
> *iff* it provides a constant inactive output. To fix that, we'd have to
> distinguish between "sync" and "async" configuration updates. However I
> see no need to do that now, as it doesn't solve a real life problem.

When adapting b) it should not matter whether a constant inactive output is 
provided. But maybe I didn't get your point here :/

> When adopting a) however a PWM that doesn't maintain an inactive output
> when disabled, must not be disabled in such a case (so we'd actually
> need to do
> 
> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> --- a/drivers/pwm/pwm-imx27.c
> +++ b/drivers/pwm/pwm-imx27.c
> @@ -280,7 +280,13 @@ static int pwm_imx27_apply(struct pwm_chip *chip,
> struct pwm_device *pwm, cr |= FIELD_PREP(MX3_PWMCR_POUTC,
>  				MX3_PWMCR_POUTC_INVERTED);
> 
> -	if (state->enabled)
> +	/*
> +	 * When the EN bit is not set, the hardware emits a constant low 
output.
> +	 * There is no formal definition about the right behaviour, but 
some
> +	 * consumers expect an inactive output from a disabled PWM. So only
> +	 * clear EN if normal polarity is configured.
> +	 */
> +	if (state->enabled || state->polarity == PWM_POLARITY_INVERSED)
>  		cr |= MX3_PWMCR_EN;
> 
>  	writel(cr, imx->mmio_base + MX3_PWMCR);
> 
> for the imx27 case[1]). So the downside of adopting a) is that there is
> no way for the lowlevel driver to know that it can safely disable the
> hardware and so safe some power.

From my point of view when a state->enable=false is applied, the PWM should be 
disabled regardless of any polarity configuration. As apparently not every PWM 
controller can provide an inactive output level in disabled state this needs 
to be handled in an upper layer, in this case pwm-fan.
The important thing then to know is whether disabling PWM can be done safely.

Best regards,
Alexander