On 8.11.2018 20:18, Uwe Kleine-König wrote:

On Thu, Nov 08, 2018 at 03:21:44PM +0000, Vokáč Michal wrote:

quoted

Hi Uwe,

On 7.11.2018 16:01, Uwe Kleine-König wrote:

quoted

quoted

Interesting idea. I just wonder why nobody else did not come up with such
a simple solution before.

I think I mentioned it already in this thread, but it went unnoticed :-)

I meant it like "How happened this was not invented years ago, when people
first noticed the issue with using inverted PWM for backlight on i.MX6."
In our project, this issue dates back to 2015 :(

quoted

Then the patch isn't correct yet. The idea is always keep the hardware
running and only disable it if it's uninverted.

OK, I got the point.

quoted

In imx_pwm_probe it's not yet known what the polarity is supposed to be,
right?

Not really. It can already be known but currently there is no way how to
pass the information to the probe function. I, as a creator of the device
(and author of a DTS file) know that the circuit needs inverted PWM signal.
And I know that the circuit needs to be disabled until I say it can be
enabled. How I say that can warry. It may be default brightness level > 0
in DTS file or from a userspace program using PWM sysfs interface.

quoted

  So the right thing to do there is to not touch the configuration
of the pwm. I think all states that are problematic then are also
problematic with the gpio/pinmux approach.

I think my use-case I already presented before is an example where
involving pinctrl solves the problem while the "leave PWM enabled
for inverted users" does not. That is all the time between
imx_pwm_probe() and imx_pwm_apply_v2().

You're doing in probe:

   if (pwm_is_running()):
     mux(pin, function=pwm)
   else:
     gpio_set_value(gpio, 0)
     mux(pin, function=gpio)

This gives you the right level assuming the gpio specification uses the
right flag (GPIO_ACTIVE_HIGH or GPIO_ACTIVE_LOW).

Agree.

Taking your example with the backlight device you specify an "init" and
a "default" pinctrl and only "default" contains the muxing for the PWM
pin everything should be as smooth as necessary: The pwm is only muxed
when the backlight device is successfully bound.

Have you tried that Uwe? The bad news is I tested that before and now
again and it does not work like that. We already discussed that earlier.
The "default" state is selected by the pinctrl core driver when imx-pwm's
driver probe is finished. And it does not matter if the imx-pwm driver
has some in-kernel users or not. See:

https://elixir.bootlin.com/linux/v4.20-rc1/source/drivers/pinctrl/core.c#L1497

It is possible that I made some mistake. So this is what I have in DT:

	pinctrl_pwm: pwm1grp {
		// PWM out
         	fsl,pins = <MX6QDL_PAD_GPIO_9__PWM1_OUT 0x8>;
	}:

	pinctrl_pwm_gpio: pwm1gpiogrp {
		// GPIO, 100k pull-up
         	fsl,pins = <MX6QDL_PAD_GPIO_9__GPIO1_IO09 0xb000>;
	};

	&pwm1 {
		#pwm-cells = <3>;
		pinctrl-names = "init", "default";
		pinctrl-0 = <&pinctrl_pwm_gpio>;
		pinctrl-1 = <&pinctrl_pwm>;
		status = "okay";
	};

The PWM output pin is not connected anywhere. It is flying in the breeze.
I do not touch the pin configuration from bootloader. So the default is
input, 100k pull-up. There is no user of the pwm1 anywhere in my DT.
If what you say is correct I should see a straight HIGH level line on
a scope from power-up to userspace. Unfortunately I do not. The output
goes low as soon as the imx-pwm probe is finished.

The probe function (of
the backlight) should have initialized the PWM with the right polarity.

Agree.

Until then nothing happens on the pin (either because it's not muxed as
PWM or because the PWM is already initialized by the bootloader).

I do not agree. My tests repeatedly show it does not work like that.

The only thing that is (maybe) needed on top of my change is that the
pwm isn't stopped in pwm-imx's .probe.

quoted

quoted

quoted

In probe you do not have any users yet. So you do not know the requested
output polarity. With "default" pinctrl the PWM output would be muxed to
the selected pin and since the PWM chip is most probably disabled
(unless you enabled it in bootloader) you would get low level on the pin.
That means your backlight is fully enabled until the first call to
imx_pwm_apply_v2(). On my system this is 2 seconds.

With the gpio/pinmux approach you don't know the intended polarity
either and maybe enable the display, too.

You know it because the pinctrl solution is optional. So if you use it,
you use it on purpose to override the default PWM output level in PWM
disabled state. It is very useful in cases where you need inverted and
disabled PWM signal from power-up to userspace. Or until some kernel
driver (backlight, led, fan..) enables it. For this it is the only
solution I think.

It allows you to boot with disabled PWM that has normal polarity set
by default. Later on from your userspace program you configure the PWM
to desired period/duty, set PWM output to inversed and enable it.
Until this point the circuit is disabled with my solution.

quoted

For both the solution is to let the bootloader enable the pwm with
the right output level. Am I missing something?

Bootloader is only a small part of the whole solution I think. And I
suppose you meant: "enable the *GPIO* with the right output level". 

I am sorry I am confused. Were you talking about the bootloader code or
about the kernel code? Because your previous sentence was clear to me:

  "For both the solution is to let the bootloader enable the pwm
   with the right output level".

My comment to that is: I think it is not necessery to configure and
enable PWM just to keep the right level on the pin. In bootloader.
I would certainly use GPIO or nothing at all depending on what logic
level I actually need.

No I meant the pwm. Well, it's as easy as that: Whenever with your
approach you configure the pin as GPIO with the output set to low,
instead configure the pwm with duty_cycle to zero (or disable it).
Whenever with your approach you configure the pin as GPIO with the
output set to high, configure the pwm with duty_cycle to 100%. (Keeping
out inverted PWMs for the ease of discussion, but the procedure can be
adapted accordingly.) The only difference then is that with your
approach you already "know" in pwm-imx's .probe the idle level and can
configure the GPIO accordingly. With my approach you just have to wait
until the first pwm_apply which (as described above) works just as well.

While here I am quite confident you are talking about kernel code, right?
If yes, then your approach is clear to me.

The problem is I am quite sure your approach does not solve the cases
the pinctrl solution does. And according to my tests so far it does not
work at all because the "init" and "default" states does not work as you
are saying.

quoted

   - Even if you use GPIO in bootloader to set the required level the
     time frame from imx_pwm_probe to imx_pwm_apply is not covered.

   - Currently there is no support in Linux pwm-imx driver to detect
     the PWM chip is already enabled at probe time. I actually send
     patches for this a month ago [1]. No response yet.

   - Inverted PWM does not work in U-Boot (on imx at least). And it
     does not seam like it can be fixed easily. I do not know what is
     the situation in other bootloaders.

So my current bootloader solution is one of:
   - Set the pin to the appropriate (HIGH) level using GPIO.
   - Do not touch the pin at all, it has 100k pull-up by default.

quoted

quoted

The other thing is I would prefer to make the change optional. With your
approach you are changing the behavior for all current users of inverted
PWM. I do not think all imx6 users are aware of the problem so they might
not be OK with the sudden change in the behavior.

Isn't my change an improvement for all users? What state do you have in
mind that make things worse than they are now?

Lets say that the user:
- Needs inverted PWM signal.
- Needs it to be disabled all the time unless he enable it.

I don't see why anybody should care if the PWM is "disabled". A user
should only care about the output level, how the pwm-imx driver
implements this is out of scope.

Agree. That is what I meant.
Here my "PWM signal disabled" == yours "right output level".

As backlight-pwm user/author you
shouldn't even care which PWM driver is in use. It might be even an
implementation that cannot be disabled (like the bcm-kona one).

quoted

What you propose (for all users of inverted PWM):
H|____________________                                     _____________
L|                    \___________________________________/
 +-------+------------+-----------------+-----------------+-------------+
 | reset | bootloader | default pinctrl | PWM enable 100% | PWM disable |

My solution (for those who want it):
H|______________________________________                   _____________
L|                                      \_________________/
 +-------+------------+-----------------+-----------------+-------------+
 | reset | bootloader | default pinctrl | PWM enable 100% | PWM disable |

I made a copy-paste mistake here, ^this^ should be "gpio pinctrl". And:
"PWM enable 100%" == "echo $(cat period) > duty_cycle"
"PWM disable" == "echo 0 > enabled"

With the above changes there is no relevant difference between your and
my approach. I save your and my time to repeat this once more with these
nice pictures. (If you don't believe me, I can do this later if you
wish.) The nice upside of my approach however is that the pwm-imx driver
stays simpler and the dts author doesn't need to care to add a gpio
specifier and two pinmux configurations instead of one.

Even if it would work I am aware your solution would not be as simple as
you think. Do you have a working code/example? What about this case:

- Boot the system into userspace.
- At this point user configures the PWM like this:
    $ echo 500000 > period
    $ echo 100000 > duty_cycle
    $ echo inversed > polarity
    $ echo 1 > enabled

- After some time the user decides to stop the PWM signal:
    $ echo 0 > enabled

With your approach you need to fully configure the PWM to produce 0% duty
cycle signal to keep HIGH level on the pin. Not just let it run as it is.
I only spent short time looking at the code trying to figure out how I
would do that. And I could not come up with a simple solution that does
not radically change all the current if-else logic. But my experience is
limited.

quoted

So your solution at least allows the user to really disable the circuit.
I can not really think of cases where this might not be good for current
users. Maybe that they simply expect that no matter what polarity is set,
the output in disabled state is always low. And they may have HW that
get already used to that and does not like the change :)

And it reminds me of something similar I have done for OLED display reset
recently [2]. I tried to fix active-low reset sequence that is hardcoded
in the driver. So you are supposed to use GPIO_ACTIVE_HIGH in DT to make
the active-low reset work. It was rejected. The reason was backward DTB
compatibility [3]. In other words: "Users of newer kernels expect that
the reset still work the same if they do not update DTBs on their boards".
I think this is kind of similar?

I don't see a compatibility issue here.

Then it is fine.

I am happy to accept/implement any "non-pinctrl/GPIO" solution as soon
as it solves all the cases I am trying to cover. That is:

- Allow pwm-backlight users to disable their backlight with inverted PWM.
- Allow unchanged level on the PWM pin from power-up to first pwm_apply.
- Allow to enable and configure PWM to any period/duty in bootloader and
   reuse that state in Linux imx-pwm driver.

Anyway, thank you for all the comments and your time you are spending
on this.

Michal