Re: [PATCH v5 07/13] pwm: add support for sl28cpld PWM controller

From: Michael Walle
Date: Wed Jul 15 2020 - 13:45:22 EST


Hi Uwe,

Am 2020-07-15 18:36, schrieb Uwe Kleine-KÃnig:
On Tue, Jul 14, 2020 at 11:09:28PM +0200, Michael Walle wrote:
> 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.

I thought this is a "change takes place immediately" implementation?! So
these problems are actually real here. (And this not happening is exactly
my wish here. Is there a mis-understanding?)

I wasn't talking about the sl28cpld btw. What is the difference between
your proposed "change take place immediately" and "after the cycle".
I understand how the after the cycle should work. But how would the
immediate change work in your ideal PWM?

> > > 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).

That happens if you reduce the duty cycle from A to B and the counter is
already bigger than B but smaller than A, right?
That is correct.

The fix would be to
compare for counter >= match instead of counter = match. (Which then
would result in a period with a duty cycle bigger than B but smaller
than A. Also not ideal, but probably better.)

This would actually work. I could imagine that comparing "less than" will
take up more space again. But it would be worth a try; see also below.

> > > 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

And does turn off mean, the output gets inactive?
If so you might also disable the hardware if a 0% duty cycle is
configured assuming this saves some energy without modifying the
resulting wave form.

Disabling it has some side effects like switching to another function
for this multi function pin. So I'd rather keep it on ;)

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.

OK. (Maybe there is enough space to allow implementing 100% for mode 0?)

Little bit here a little bit there ;) TBH there are some more critical
bugs which would need to be fixed first. So this would need to be a
limitation for now.

-michael