Re: [RCF PATCH,v2,2/2] pwm: imx: Configure output to GPIO in disabled state

[Uwe Kleine-König]

Hello,

On Tue, Nov 20, 2018 at 09:35:47AM +0100, Linus Walleij wrote:
> On Mon, Nov 19, 2018 at 9:32 AM Uwe Kleine-König
> <u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
> 
> > To sumarize: When the pwm driver probes it is not yet clear if the idle
> > state of the output pin is high or low. Even when the pinctrl device has
> > an "init" and a "default" pinctrl, it is not yet fixed when its
> > "default" is configured.
> >
> > The way Thierry wants to fix that is by disabling the output driver
> > until the pwm is in use und rely on a pull-up or pull-down in hardware.
> >
> > The way I want to fix this is to only configure the pwm pin as part of
> > the consumer. This is late enough that the consumer already requested
> > and configured the pwm such that the idle level is known.
> > Thierry's and Lothar's claim was that putting the pin setup of the pwm
> > pin into the pwm consumer's pinctrl was forbidden. That's why I asked
> > you as pinctrl maintainer if there is a requirement that I don't know
> > of.
> 
> I think we need to be pragmatic and listen most to whoever has
> the hardware and need to get it to work. The system needs
> to come up in some reasonable way, preferably so that vendors
> doing products with it do not have to apply a fat patch stack
> to get that backlight working in an acceptable way. Else the
> end result is out-of-tree code to paper over the issue and that
> IMO is worse than some minor ugliness in the kernel.
> 
> However the whole ordeal points to a problem that is with the
> pin control system and Thierry's and Lothar's intuition about this
> is right in a way: if the pin control system could read out the
> state of the hardware at boot (as we nowadays do with GPIO,
> which also has a consumer flag cleverly named GPIOD_ASIS)
> the whole thing would be no problem.

Well, on the problematic machine the setup currently is:

The bootloader configures the brightness pin as GPIO high output to
disable the backlight, the PWM isn't touched and so is off and
uninverted.

The next relevant event is that the PWM driver is probed.
The PWM registers are not touched, so keeps being off and outputs
a 0. If in this state the pin is muxed as PWM the output becomes 0 which
should be prevented, so this must not be part of a "default" or "init"
pinctrl for the PWM node.

The next event is that the backlight driver is probed. The probe
function grabs the PWM, tells it to be inverted and off. So the output
of the PWM is a 1 and only after that the PWM pin can be safely muxed as
PWM.

So the options are:

 a) In the bootloader setup the PWM as inverted and configure the pin as
    PWM.

 b) Make the PWM aware that it is off and let it mux an "idle" setting
    that depending on the SoC might involve muxing the pin as GPIO and
    setting the GPIO as input.

 c) setup the PWM pin as PWM only when the backlight is probed.

a) has the downside that configuring a PWM is a tad more complex then
setting up a GPIO. Also it's a bit ugly to let Linux impose a limitation
on the bootloader about how to disable the backlight there.

b) has the downside that the dts author has to specify a GPIO and the
idle level and two instead of only one pinctrl setting. Conceptually
this is not so nice, because normally the PWM doesn't know if it is used
inverted or not. With the GPIO specification this information is
implicitly added. (Also there might be additional hurdles, for example
on i.MX31 (IIRC) there are pins that cannot be muxed as GPIO but are in
use as CS signal for spi. I didn't check if there is a similar
limitation for PWM pins.)

With c) the pwm is setup only when the backlight driver requests it. If
the backlight driver fails to probe or is disabled nothing happens to
the PWM pin. If the backlight driver was loaded successfully the PWM pin
is muxed just after it was set up with the right polarity. Similar as
with b) there is no limitation about how the bootloader disabled the
backlight. The beauty I see in c) is this is a solution that doesn't
have to care about GPIOs and muxing pins depending on the PWM state. And
given that there is such a way, I don't see why we should impose on dts
authors to specify these things that are not technically necessary.

Both b) and c) is about delaying muxing the pin as PWM late enough.
Suggestion b) is about doing this explicitly somewhere below driver/pwm,
c) only uses pinctrl as available automatically for all devices.

Probably its subjective if you consider b) or c) as the better/prettier
solution.

Note that the solution b) is independent of the imx uglyness[1] and
applies to all PWMs that don't disable its output driver on disable.
(And for PWMs that disable the output driver, the solution might be
needed anyhow because there is "broken" hardware that doesn't have the
right pullups assembled to ensure the right idle level when the pin
doesn't drive.)

So the sensible options are in my eyes:

 - Implement b) in the PWM core (and not the pwm-imx driver); or
 - just use c) accepting that the PWM is muxed not by itself but by its
   consumer.

In my eyes doing b) consequently would result in letting the PWM know
the intended idle level even without a consumer. Not only implicitly by
a GPIO specification, but explicitly using something like:

	&pwm3 {
		pwm-default-inverted;
	};

in the device tree. (And with that the GPIO solution would loose
its importance because every PWM is able to drive a constant 1 and a
constant 0. It might just be useful as an optimisation that draws less
power.)

> The same kind of goes for PWM itself in this case I guess.

So as pinmuxing is involved it doesn't solve the problem as the driver
would diagnose the PWM to be uninverted and off and so would enable the
backlight. There is still a solution needed to delay the muxing.

> The whole issue with splash screens and different hardware
> turned over to Linux in running state is a bit imperfect I would
> say, I think Viresh was working on boot constraints to get
> handover of different systems components into some kind
> of shape but maybe that stopped short because of the
> complexities involved. Isn't that the real problem here?
> https://lkml.org/lkml/2017/8/1/191

The Boot Constraints core solves things like "Don't let the UART change
the clk frequency of a common parent of the UART itself and the PWM that
is needed to show the splash screen." This is orthogonal to the problem
here I'd say.

Best regards
Uwe

[1] If you disable the imx PWM while being inverted it outputs a 0
    also a 1 would be more consistent.
-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |