Date:   Tue, 14 Jul 2020 23:09:28 +0200
From:   Michael Walle <michael@...le.cc>
To:     Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc:     Thierry Reding <thierry.reding@...il.com>,
        linux-gpio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org,
        linux-pwm@...r.kernel.org, linux-watchdog@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Rob Herring <robh+dt@...nel.org>,
        Jean Delvare <jdelvare@...e.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Lee Jones <lee.jones@...aro.org>,
        Wim Van Sebroeck <wim@...ux-watchdog.org>,
        Shawn Guo <shawnguo@...nel.org>, Li Yang <leoyang.li@....com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Jason Cooper <jason@...edaemon.net>,
        Marc Zyngier <maz@...nel.org>, Mark Brown <broonie@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH v5 07/13] pwm: add support for sl28cpld PWM controller

Hi Uwe,

Am 2020-07-14 18:08, schrieb Uwe Kleine-König:
> On Tue, Jul 14, 2020 at 01:31:13PM +0200, Michael Walle wrote:
>> Am 2020-07-13 10:47, schrieb Uwe Kleine-König:
>> > I already thought about proposing pwm_get_rate_hw(), but for now there
>> > is (AFAICT) no user who would need it. And it's hard to know which
>> > variant is actually preferred by consumers. My expectation is that most
>> > don't even care.
>> >
>> > I also have a pwm_round_rate() function in mind that will give you the
>> > actual rate without applying it. This can then be used by consumers who
>> > care. But also there is no user who would need it today.
>> 
>> Ok. I take it that all such improvements are still in the making ;)
> 
> If you have a real use case, present it, then I give it a boost on my
> todo list.

Not really.

>> > > But the PWM subsystem returns the cached state,
>> > > right? get_state() is called only on device request (and during
>> > > debug it seems). Actually, enabling PWM_DEBUG might choke on this
>> > > workaround (".apply didn't pick the best available period"). Is
>> > > this ok?
>> >
>> > hmm, I didn't consider this when writing the checks for PWM_DEBUG.
>> > According to the currently checked rules the expected configuration is
>> > to pick the 250Hz mode and use cycle = 0x7f.
>> 
>> Not to use 0x80, which is the max_duty_cycle? Like its 0x40 for the 
>> 500Hz
>> mode.
> 
> No, when I thought about a sane set of rules (and so checks for
> PWM_DEBUG) I didn't consider a PWM that can implement 100% but not for
> all otherwise available period lengths. I'm still amazed sometimes how
> different the capabilities of different implementations for something 
> so
> seemingly easy like a PWM are.
> 
>> > Hmm, I have to think about
>> > this. Maybe we should weaken the check to the cases with
>> > 0 < duty_cycle < period. Thierry, what do you think?
>> >
>> > Special casing 0% and 100% is annoying, but insisting 250Hz + 0x7f seems
>> > to be far from reality. (Is it?)
>> 
>> If you mean by insisting to clip at 0x7f, yeah thats bad IMHO, because
>> the user wants an all-high line, but in the end it would be a toggling
>> line. It wouldn't be that bad for anything in between 0% and 100% but
>> IMHO its bad for exactly 0% and 100%.
>> 
>> You could also ask the driver about known quirks, like special 0% and
>> 100% handling and exclude it from the tests accordingly.
> 
> Do you care enough to propose a patch? You're in the situation to test
> it.

Ok. I'll come up with something outside of this patch series.

>> > > > > +	ret = regmap_write(priv->regmap, priv->offset + PWM_CTRL, ctrl);
>> > > > > +	if (ret)
>> > > > > +		return ret;
>> > > > > +
>> > > > > +	return regmap_write(priv->regmap, priv->offset + PWM_CYCLE,
>> > > > > (u8)cycle);
>> > > >
>> > > > I assume this can result in broken output? Consider the hardware runs
>> > > > with mode = 1 & cycle = 0x23 and you want to go to mode = 0 & cycle =
>> > > > 0x42: Can this result in a period that has mode = 0 & cycle = 0x23?
>> > >
>> > > Isn't that always the case if a write may fail and there are more than
>> > > one register to configure?
>> >
>> > Depending on hardware capabilities you might not be able to prevent
>> > this yes. Unfortunately this is quite common.
>> >
>> > But there are hardware implementations that are not prone to such
>> > failures. (E.g. the registers written can be only shadow values that are
>> > latched into hardware only when the last value is written.)
>> 
>> Maybe this could be improved in the future.
> 
> We should somewhere describe, what an ideal PWM can do.
> My wishlist (just as it comes to my mind, so no guarantee of
> completeness):
> 
>  - can do 0% duty cycle for all supported period lengths
>  - can do 100% duty cycle for all supported period lengths
>  - supports both polarities
>  - supports immediate change of configuration and after completion of
>    the currently running period
>  - atomic update (i.e. if you go from configuration A to configuration 
> B
>    the hardware guarantees to only emit periods of type A and then type
>    B. (Depending on the item above, the last A period might be cut 
> off.)

We actually discussed this, because the implementation would be easier. 
But
if the change takes place immediately you might end up with a longer 
duty
cycle. Assume the PWM runs at 80% duty cycle and starts with the 
on-period.
If you now change that to 50% you might end up with one successive duty
cycle of "130%". Eg. the 80% of the old and right after that you switch 
to
the new 50% and then you'd have a high output which corresponds to a 
130%
cycle. I don't know if that is acceptable for all applications.

>  - emits an irq when configuration changes

Why would you need the interrupt?

> 
>> > If you change only cycle but not mode, does the hardware complete the
>> > currently running period?
>> 
>> No it does not.
> 
> Please document this as a Limitation.

I've discussed this internally, for now its a limitation. In the worst
case you'd do one 100% duty cycle. Maybe we can fix the hardware. I
acknowledge that this is a severe limitation, esp. if you use the PWM
for controlling stuff (for now its only LCD backlight.. so thats ok).

>> > What about disable()?
>> 
>> Mhh well, it would do one 100% cycle.. mhh ;) Lets see if there we can
>> fix that (in hardware), not much we can do in the driver here. We are
>> _very_ constraint in size, therefore all that little edge cases fall 
>> off
>> the table.
> 
> You're saying that on disable the hardware emits a constant high level
> for one cycle? I hope not ...

Mh, I was mistaken, disabling the PWM will turn it off immediately, but
one 100% duty cycle may happen if you change from a higher to a lower
duty cycle setting. See above.

> I never programmed a CPLD to emulate a hardware PWM, but I wonder if
> these are really edge cases that increase the size of the binary?!

At the moment there is only one 8bit register which stores the value
which is used for matching. If you want to change that setting after
a whole cycle, you'd use another 8bit register to cache the new value.
So this would at least needs 8 additional flip-flops. This doesn't
sound much, but we are already near 100% usage of the CPLD. So its
hard to convince people why this is really necessary.

-michael

Powered by blists - more mailing lists