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.cjcc7okmwtaujf43@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? > 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. 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) 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 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. Best regards Uwe [1] or something more complicated. I remember a patch that switched the pinmuxing to GPIO that emitted a constant low output which I consider not being worth the power savings (if there are any). -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
