Re: [RFC PATCH 1/2] dt-bindings: pwm: imx: Allow switching PWM output between PWM and GPIO

[VokÃÄ Michal]

    On 31.8.2018 15:30, Lothar WaÃmann wrote:
> Michal VokÃÄ <michal.vokac@xxxxxxxxx> wrote:
> 
>> On 31.8.2018 14:45, Lothar WaÃmann wrote:
>>> Rob Herring <robh@xxxxxxxxxx> wrote:
>>>    
>>>> On Tue, Aug 21, 2018 at 04:38:52PM +0200, Michal VokÃÄ wrote:
>>>>> Output of the PWM block of i.MX SoCs is always zero volts when the block
>>>>> is disabled. This can caue issues when inverted PWM polarity is needed.
>>>>> With inverted polarity a duty cycle = 0% corresponds to solid high level
>>>>> on the output. If the PWM is dissabled its output instantly goes to solid
>>>>> zero which corresponds to duty cycle = 100%.
>>>>>
>>>>> To have a trully inverted PWM output configure the PWM pad as a GPIO
>>>>> with pull-up. Then switch the pad to PWM output whenever non-zero
>>>>> duty cycle is needed.

Ahoj,
Sorry for the long delay. I spent most of the time learning more about
the pinctr, pwm and clock subsystem internals. I have also done a lot
of experiments and measurements of various PWM setups and use-cases.

The number of possible use-cases is really huge and I doubt it is possible
to address all of them. So I would like to address at least some of them.

Cases I have on my mind fall into these two groups:

1) A single purpose HW that needs inverted PWM signal. With PWM client
in the kernel. Eg. a board with PWM controlled LCD backlight. The client
should be able to enable/disable the PWM with expected results.
Enabled = bright, disabled = dark.

2) A more generic HW that still needs inverted PWM signal. No PWM client
in the kernel. Eg. a board that controls some technology using only a PWM
signal. That technology needs to stay disabled unless some process from
user space enables the PWM.

Users that need normal, non-inverted PWM can happily use current binding.
The same for cases with totally generic PWM output with no specific usage.

More comments bellow.

>>>>> Signed-off-by: Michal VokÃÄ <michal.vokac@xxxxxxxxx>
>>>>> ---
>>>>>    Documentation/devicetree/bindings/pwm/imx-pwm.txt | 44 +++++++++++++++++++++++
>>>>>    1 file changed, 44 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.txt b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
>>>>> index c61bdf8..3b1bc4c 100644
>>>>> --- a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
>>>>> +++ b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
>>>>> @@ -14,6 +14,12 @@ See the clock consumer binding,
>>>>>    	Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>>>    - interrupts: The interrupt for the pwm controller
>>>>>    
>>>>> +Optional properties:
>>>>> +- pinctrl: For i.MX27 and newer SoCs. Add extra pinctrl to configure the PWM
>>>>> +  pin to gpio function.  It allows control over the pin output level when the
>>>>> +  PWM block is disabled. This is meant to be used if inverted polarity of the
>>>>> +  PWM signal is required. See "Inverted PWM output" section bellow.
>>>>> +
>>>>>    Example:
>>>>>    
>>>>>    pwm1: pwm@53fb4000 {
>>>>> @@ -25,3 +31,41 @@ pwm1: pwm@53fb4000 {
>>>>>    	clock-names = "ipg", "per";
>>>>>    	interrupts = <61>;
>>>>>    };
>>>>> +
>>>>> +Inverted PWM output
>>>>> +-------------------
>>>>> +
>>>>> +The i.MX SoC has such limitation that whenever a pad is configured as a PWM
>>>>> +output, the output level is always zero volts when the PWM block is disabled.
>>>>> +The zero output level is actively driven by the output stage of the PWM block
>>>>> +and can not be overridden by pull-up. It also does not matter what PWM polarity
>>>>> +a PWM client (e.g. backlight) requested.
>>>>> +
>>>>> +To gain control of the PWM output level in disabled state two pinctrl states
>>>>> +can be used. The "default" state and the "pwm" state. In the default state the
>>>>> +PWM output is configured as a GPIO with pull-up. In the "pwm" state the output
>>>>> +is configured as a PWM output. This setup assures that the PWM output is at
>>>>> +the required level that corresponds to duty cycle = 0 when PWM is disabled.
>>>>> +E.g. at boot.
>>>>> +
>>>>> +Example:
>>>>> +
>>>>> +&pwm1 {
>>>>> +	pinctrl-names = "default", "pwm";
>>>>> +	pinctrl-0 = <&pinctrl_backlight_gpio>;
>>>>> +	pinctrl-1 = <&pinctrl_backlight_pwm>;
>>>>> +}
>>>>> +
>>>>> +pinctrl_backlight_gpio: pwm1grp-gpio {
>>>>> +	fsl,pins = <
>>>>> +		/* GPIO with 22kOhm pull-up */
>>>>> +		MX6QDL_PAD_GPIO_9__GPIO1_IO09	0xF008
>>>>
>>>> There's a slight problem here if I remember the i.MX pin muxing. In GPIO
>>>> mode, doesn't the GPIO block control the direction and level if an
>>>> output. I guess as long as unused GPIOs are all initialized to inputs it
>>>> will be okay.
>>
>> I am not sure if I understand you correctly. Did you mean: "..doesn't the
>> GPIO block control the PULL-UP/DOWN and level if an output."? Yes, that is
>> true. And as you said, all GPIOs are configured as inputs after reset.
>>
>>> One could set the pad_ctl DSE value to 0, so that the pin cannot be
>>> driven even if configured as output:
>>> 		MX6QDL_PAD_GPIO_9__GPIO1_IO09	0xF000
>>
>> Yes, it will make no harm to set the pin to high-Z if configured as
>> output. Though I am not sure that this makes sense.
>>
> If you want to rely on the function of the Pull resistors this is
> exactly what you need.

I think I got your point. Just setting the pull resistors from DT does
not assure the output level. As you noted below - the bootloader may have
configured the pin as output. Then the pull resistor will have no effect.

>> In case we choose the pull-up to keep the level high the pin needs to stay
>> configured as input. And as the GPIO is reserved for us there is actually
>> no one else who could re-configure it.
>>
> U-Boot may have configured the PWM pin as output to enable the
> backlight without brightness control.

I see I used wrong assumption. The pin may not have its default (input,
100k pull-up) configuration when Linux is booting.

So once we request the GPIO from driver code (either as input or output)
we have full control over the output level. In input mode with the pull
resistors and in output mode we can set the output value.

>> In case we choose to actively drive the pin instead of relying on the
>> internal pull-up we need to use gpiod lib and configure the pin as output.
>> In that case DSE must be set non-zero.
>>
> That is my personal preference too.

OK, lets do it that way. It seems like the most reliable solution.

Also I came up with another idea to make the binding less confusing.
Lets not misuse the "default" pinctrl state for GPIO function. Instead we
can define two separate "pwm" and "gpio" states and switch between those.

Why I am not happy with just the "default" state for PWM and "gpio" state
for GPIO output is that the "default" state is automagically selected by
the pinctrl driver at pwm-imx driver probe. And that will cause some level
changes on the output.

I will send v2 with all the changes soon.

As a side effect I also find out that the current driver does not support
HW state readout. If you enable the PWM in bootloader, Linux does not know
about it. Hence the clock subsystem (imx-gate2) disables the PWM source
clock as it seems unused. As a result the PWM block is left enabled with
disabled source clock and the PWM output is stopped at random level. That
is not really good.

So in the meantime I submitted a series [1] that implement the get_state()
function.

[1] http://patchwork.ozlabs.org/project/linux-pwm/list/?series=68445

Best regards,
Michal