by Lokesh Vutla

[permalink] [raw]

Subject: Re: [PATCH v3 0/5] pwm: omap-dmtimer: Allow for dynamic pwm period updates

Hi All,

On 12/03/20 9:52 AM, Lokesh Vutla wrote:
> This series fixes minor issues in config callback and allows for on the
> fly updates for pwm period and duty cycle. This is mainly intended to
> allow pwm omap dmtimer to be used for generating a 1PPS signal that can be
> syncronized to PTP clock in CPTS module available in AM335x and AM57xx SoCs.
>
> Series depends on the following series:
> - https://patchwork.kernel.org/patch/11379875/
> - https://patchwork.kernel.org/project/linux-omap/list/?series=251691

Gentle ping on this series :) The above dependencies are merged into timer tree
and below is the immutable branch:

https://git.linaro.org/people/dlezcano/linux.git/log/?h=timers/drivers/timer-ti-dm

Thanks and regards,
Lokesh

2020-03-30 14:09:07

by Thierry Reding

[permalink] [raw]

Subject: Re: [PATCH v3 0/5] pwm: omap-dmtimer: Allow for dynamic pwm period updates

On Mon, Mar 23, 2020 at 05:00:13PM +0530, Lokesh Vutla wrote:
> Hi All,
>
> On 12/03/20 9:52 AM, Lokesh Vutla wrote:
> > This series fixes minor issues in config callback and allows for on the
> > fly updates for pwm period and duty cycle. This is mainly intended to
> > allow pwm omap dmtimer to be used for generating a 1PPS signal that can be
> > syncronized to PTP clock in CPTS module available in AM335x and AM57xx SoCs.
> >
> > Series depends on the following series:
> > - https://patchwork.kernel.org/patch/11379875/
> > - https://patchwork.kernel.org/project/linux-omap/list/?series=251691
>
> Gentle ping on this series :) The above dependencies are merged into timer tree
> and below is the immutable branch:
>
> https://git.linaro.org/people/dlezcano/linux.git/log/?h=timers/drivers/timer-ti-dm

I've pulled in the above branch into the PWM tree and applied your
series on top.

Thanks,
Thierry

Attachments:

(No filename) (935.00 B)
signature.asc (849.00 B)
Download all attachments

2020-03-30 14:28:38

On Tue, Mar 31, 2020 at 08:59:47PM +0530, Lokesh Vutla wrote:
> Hi Thierry,
>
> On 30/03/20 7:44 PM, Thierry Reding wrote:
> > On Thu, Mar 12, 2020 at 07:40:42AM +0100, Uwe Kleine-König wrote:
> >> On Thu, Mar 12, 2020 at 09:52:09AM +0530, Lokesh Vutla wrote:
> >>> Only the Timer control register(TCLR) cannot be updated when the timer
> >>> is running. Registers like Counter register(TCRR), loader register(TLDR),
> >>> match register(TMAR) can be updated when the counter is running. Since
> >>> TCLR is not updated in pwm_omap_dmtimer_config(), do not stop the
> >>> timer for period/duty_cycle update.
> >>
> >> I'm not sure what is sensible here. Stopping the PWM for a short period
> >> is bad, but maybe emitting a wrong period isn't better. You can however
> >> optimise it if only one of period or duty_cycle changes.
> >>
> >> @Thierry, what is your position here? I tend to say a short stop is
> >> preferable.
> >
> > It's not clear to me from the above description how exactly the device
> > behaves, but I suspect that it may latch the values in those registers
> > and only update the actual signal output once a period has finished. I
> > know of a couple of other devices that do that, so it wouldn't be
> > surprising.
> >
> > Even if that was not the case, I think this is just the kind of thing
> > that we have to live with. Sometimes it just isn't possible to have all
> > supported devices adhere strictly to an API. So I think the best we can
> > do is have an API that loosely defines what's supposed to happen and
> > make a best effort to implement those semantics. If a device deviates
> > slightly from those expectations, we can always cross fingers and hope
> > that things still work. And it looks like they are.
> >
> > So I think if Lokesh and Tony agree that this is the right thing to do
> > and have verified that things still work after this, that's about as
> > good as it's going to get.
>
> Yes this is needed especially in the use-case[0] that I am trying to enable
> using PWM. In this case PWM cannot be stopped in between and needs to be updated
> dynamically. Also hardware doesn't provide any restrictions on updating the
> period. So IMHO, this might be the right thing to do.
>
> Tony did provide tested-by and I measured PWM signals on scope with these
> changes. Let me know if any thing else is required?
>
> [0] https://sourceforge.net/p/linuxptp/mailman/message/36943248/

From you measurements, can you tell whether or not the signal actually
gets updated in the middle of a period, or does it only get updated at
the end of a full period?

Does the reference manual document this?

Thierry

Attachments:

(No filename) (2.65 kB)
signature.asc (849.00 B)
Download all attachments

2020-03-31 20:46:38

On Wed, Apr 01, 2020 at 10:22:27AM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
>
> On Tue, Mar 31, 2020 at 10:45:59PM +0200, Thierry Reding wrote:
> > On Mon, Mar 30, 2020 at 09:16:54PM +0200, Uwe Kleine-König wrote:
> > > On Mon, Mar 30, 2020 at 04:14:36PM +0200, Thierry Reding wrote:
> > > > On Thu, Mar 12, 2020 at 07:40:42AM +0100, Uwe Kleine-König wrote:
> > > > > On Thu, Mar 12, 2020 at 09:52:09AM +0530, Lokesh Vutla wrote:
> > > > > > Only the Timer control register(TCLR) cannot be updated when the timer
> > > > > > is running. Registers like Counter register(TCRR), loader register(TLDR),
> > > > > > match register(TMAR) can be updated when the counter is running. Since
> > > > > > TCLR is not updated in pwm_omap_dmtimer_config(), do not stop the
> > > > > > timer for period/duty_cycle update.
> > > > >
> > > > > I'm not sure what is sensible here. Stopping the PWM for a short period
> > > > > is bad, but maybe emitting a wrong period isn't better. You can however
> > > > > optimise it if only one of period or duty_cycle changes.
> > > > >
> > > > > @Thierry, what is your position here? I tend to say a short stop is
> > > > > preferable.
> > > >
> > > > It's not clear to me from the above description how exactly the device
> > > > behaves, but I suspect that it may latch the values in those registers
> > > > and only update the actual signal output once a period has finished. I
> > > > know of a couple of other devices that do that, so it wouldn't be
> > > > surprising.
> > > >
> > > > Even if that was not the case, I think this is just the kind of thing
> > > > that we have to live with. Sometimes it just isn't possible to have all
> > > > supported devices adhere strictly to an API. So I think the best we can
> > > > do is have an API that loosely defines what's supposed to happen and
> > > > make a best effort to implement those semantics. If a device deviates
> > > > slightly from those expectations, we can always cross fingers and hope
> > > > that things still work. And it looks like they are.
> > > >
> > > > So I think if Lokesh and Tony agree that this is the right thing to do
> > > > and have verified that things still work after this, that's about as
> > > > good as it's going to get.
> > >
> > > I'd say this isn't for the platform people to decide. My position here
> > > is that the PWM drivers should behave as uniform as possible to minimize
> > > surprises for consumers. And so it's a "PWM decision" that is to be made
> > > here, not an "omap decision".
> >
> > I think there's a fine line to be walked here. I agree that we should
> > aim to have as much consistency between drivers as possible. At the same
> > time I think we need to be pragmatic. As Lokesh said, the particular use
> > case here requires this type of on-the-fly adjustment of the PWM period
> > without stopping and restarting the PWM. It doesn't work otherwise. So
> > th alternative that you're proposing is to say that we don't support
> > that use-case, even though it works just fine given this particular
> > hardware. That's not really an option.
>
> I understand your opinion here. The situation now is that in current
> mainline the driver stops the hardware for reconfiguration and it
> doesn't fit Lokesh's use case so he changed to on-the-fly update
> (accepting that maybe a wrong period is emitted). What if someone relies
> on the old behaviour? What if in a year someone comes and claims the
> wrong period is bad for their usecase and changes back to
> stop-to-update?

Relying on that old behaviour is wrong. If you really need to rely on
the PWM being stopped before reconfiguration, then you should explicitly
request that by first disabling the PWM, then reconfiguring it and then
reenabling it.

> When I write a consumer driver, do I have a chance to know how the PWM,
> that I happen to use, behaves? To be able to get my consumer driver
> reliable I might need to know that however.

No, there's currently no way of knowing. As such, I think the most
sensible thing to do at this point is to work with the API in order to
get the behaviour that you want. If you need to be able to reconfigure
the PWM without stopping it, then just call pwm_config(), or preferably
pwm_apply_state() with only the duty-cycle and/or period (or perhaps
even polarity) changed but keeping the PWM enabled.

Now if you're unlucky the driver won't support that and you'll notice
eventually. But there's also nothing you can do about that if the
hardware doesn't work that way. Even if the PWM framework had a way of
querying these types of peculiarities it wouldn't really proved much of
an advantage since the driver would just refuse to probe rather than
attempting to do this and maybe succeeding because you got lucky.

I suppose one variant that we could implement is to have drivers return
an error if they are being asked to reconfigure while the PWM remains
on. But then again, it's hard to know whether the driver writer really
depends on the PWM remaining on the whole time and seamlessly
transitioning to the new period/duty-cycle, or whether they just do it
out of convenience.

Take for example a backlight driver. Ideally you'd be able to seamlessly
change the brightness by just modifying the duty-cycle while the PWM is
enabled. If you have to turn the PWM off, reconfigure it and then turn
it back on, it could always happen that the backlight may flicker
because it actually goes completely dark for a little bit. In practice I
would expect the delay to be negligible because this is ultimately
converted to an analog power and hence the small dip to 0 isn't going to
matter very much. So if we change the API to refuse to reconfigure at
runtime for cases where the PWM has to be disabled and enabled during
reconfiguration, that may all of a sudden cause failures where none were
observed before.

> > > > I know this is perhaps cheating a little, or turning a blind eye, but I
> > > > don't know what the alternative would be. Do we want to tell people that
> > > > a given PWM controller can't be used if it doesn't work according to our
> > > > expectations? That's hard to argue if that controller works just fine
> > > > for all known use-cases.
> > >
> > > I'd like have some official policy here which of the alternatives is the
> > > preferred cheat.
> > >
> > > The situation here is that period and duty_cycle cannot be updated
> > > atomically. So the two options are:
> > >
> > > - stop shortly
> > > - update with hardware running and maybe emit a broken period
> >
> > I think we can already support both of those with the existing API. If
> > a consumer wants to stop the PWM while reconfiguring, they should be
> > able to do pwm_enable(), pwm_config(), pwm_enable() (or the atomic
> > equivalent) and for the second case they can just do pwm_config() (or
> > the atomic equivalent).
>
> Yes, the consumer can force the stop and update. But assume I'm "only" a
> consumer driver author and I want: atomic update and if this is not
> possible I prefer "stop-to-update" over "on-the-fly-and-maybe-faulty".
> So I cannot benefit from a good driver/hardware that can do atomic
> updates? Or I have to patch each driver that I actually use to use
> stop-to-update?

You could do what everybody does and just assume that atomic update
works. If it works with the particular PWM device/driver that you have
that may be all you care about. Obviously that may not be true for a
different chip.

On the other hand, if you absolutely must ensure that there must never
be any glitches whatsoever but don't care whether the PWM goes through a
disable/enable sequence, then doing so explicitly is going to be your
best bet. From a consumer driver point of view it isn't going to matter
because even if we had a way of distinguishing between these
capabilities you'd still have to have code to deal with both.

So the only relevant use-case here would be if you had a requirement to
perform on-the-fly-and-strictly-correct updates. The API doesn't give
those kinds of guarantees. So we would need an extension that consumers
can query to determine whether what they want to do will work.

But like I said above, the chances that you will run into this are slim
because use-cases are typically known ahead of time and devices are
designed to be able to support them. So if you build a device that needs
to support these strict requirements, then you need to make sure the
hardware supports it, too. And when the hardware supports it, then the
driver should implement ->config() or ->apply() in a way that allows
this.

> > Some hardware may actually require the PWM to be disabled before
> > reconfiguring, so they won't be able to strictly adhere to the second
> > use-case.
> >
> > But as discussed above, I don't want to strive for a lowest common
> > denominator that would preclude some more specific use-cases from
> > working if the hardware supports it.
> >
> > So I think we should aim for drivers to implement the semantics as
> > closely as possible. If the hardware doesn't support some of these
> > requirements strictly while a particular use-case depends on that, then
> > that just means that the hardware isn't compatible with that use-case.
> > Chances are that the system just isn't going to be designed to support
> > that use-case in the first place if the hardware can't do it.
> >
> > The sysfs interface is a bit of a special case here because it isn't
> > possible to know what use-cases people are going to come up with.
>
> In my eyes the sysfs interface isn't special here. You also don't know
> what the OMAP PWM hardware is used for.

But platform designers will know what their device will be used for. If
they know that it will be used for a case that the OMAP PWM doesn't
support, then they had better choose a different chip. Or add an extra
IC that provides a PWM that can do what they need.

> > It's most likely that they'll try something and if it doesn't work
> > they can see if a driver patch can improve things.
>
> So either the group who prefers "stop-to-update" or the group who
> prefers "on-the-fly-and-maybe-faulty" has to carry a system specific
> driver patch?

No, the group that prefers "stop-to-update" should make that explicit
and write a consumer driver that first disables, then reconfigured and
then reenables the PWM.

If they don't *need* to update the PWM on the fly, then performing two
additional steps that would be happening anyway won't matter, right?

> > One possible extension that I can imagine would be to introduce some
> > sort of capability structure that drivers can fill in to describe the
> > behaviour of the hardware. Drivers like pwm-omap-dmtimer, for example,
> > could describe that they are able to change the period and/or duty cycle
> > while the PWM is on. There could be another capability bit that says
> > that the current period will finish before new settings are applied. Yet
> > another capability could describe that duty-cycle and period can be
> > applied atomically. Consumers could then check those capabilities to see
> > if they match their requirements.
> >
> > But then again, I think that would just make things overly complicated.
> > None of the existing consumers need that, so it doesn't seem like there
> > is much demand for that feature. In practice I suspect most consumers
> > work fine despite potentially small deviations in how the PWM behaves.
>
> I think the status quo is what I asked about above: People use sysfs and
> if the PWM behaves different than needed, the driver is patched and most
> of the time not mainlined. If your focus is to support a certain
> industrial system with a defined use case, this is fine. If however you
> target for an universal framework that works for any combination of
> consumer + lowlevel driver without patching (that at least is able to
> diagnose: This PWM cannot provide what my consumer needs), this is bad.

Again, my response to this is: how is this going to be beneficial? In
practice the way that this would work is that the consumer driver would
fail if presented with a PWM that doesn't meet the strict requirements.
Now if the requirements really are that strict, then that sounds like a
good idea.

But one issue I foresee with this is that we'll end up giving consumers
too much of a toolkit to restrict things. What if the consumer driver
author assumes wrongly that a given set of requirements exists? What if
for some combination of hardware that doesn't strictly conform to those
requirements it might still work? Sometimes you may not notice the
difference, at other times there may be some impact like a visual glitch
or so, but that may be something that users are willing to accept rather
than not have support at all.

> Also this means that whenever a system designer changes something on
> their machine (kernel update, different hardware, an new usecase for a
> PWM) they might have to reverify if the given PWM driver behaves as
> needed.

I don't expect this type of change to happen very often. There's always
going to be some type of fine-tuning before a driver's behaviour is
completely stabilized. And then there could still always be other
factors impacting behaviour that aren't even related to the PWM
framework.

I suspect that most people will have an array of tests to validate that
everything still works after a kernel update. Obviously we don't want a
new kernel to behave completely differently, but we're not talking about
that here. This is merely dropping a needless disable/enable from a
configuration. If somebody was relying on this happening they were wrong
to rely on it to begin with because the API does not guarantee it.

> My suggestion for now is to start documenting how the drivers behave
> expanding how limitations are documented in some drivers. So maybe
> change from "Limitations" to "Implementation and Hardware Details"?

Yes, collecting such information is always a good idea.

Thierry

Attachments:

(No filename) (13.92 kB)
signature.asc (849.00 B)
Download all attachments

2020-04-01 18:41:20

On Wed, Apr 01, 2020 at 10:31:56PM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
>
> On Wed, Apr 01, 2020 at 08:28:33PM +0200, Thierry Reding wrote:
> > On Wed, Apr 01, 2020 at 10:22:27AM +0200, Uwe Kleine-König wrote:
> > > On Tue, Mar 31, 2020 at 10:45:59PM +0200, Thierry Reding wrote:
> > > > I think there's a fine line to be walked here. I agree that we should
> > > > aim to have as much consistency between drivers as possible. At the same
> > > > time I think we need to be pragmatic. As Lokesh said, the particular use
> > > > case here requires this type of on-the-fly adjustment of the PWM period
> > > > without stopping and restarting the PWM. It doesn't work otherwise. So
> > > > th alternative that you're proposing is to say that we don't support
> > > > that use-case, even though it works just fine given this particular
> > > > hardware. That's not really an option.
> > >
> > > I understand your opinion here. The situation now is that in current
> > > mainline the driver stops the hardware for reconfiguration and it
> > > doesn't fit Lokesh's use case so he changed to on-the-fly update
> > > (accepting that maybe a wrong period is emitted). What if someone relies
> > > on the old behaviour? What if in a year someone comes and claims the
> > > wrong period is bad for their usecase and changes back to
> > > stop-to-update?
> >
> > Relying on that old behaviour is wrong. If you really need to rely on
> > the PWM being stopped before reconfiguration, then you should
> > explicitly request that by first disabling the PWM, then reconfiguring
> > it and then reenabling it.
>
> You got me wrong here. See below.
>
> > > When I write a consumer driver, do I have a chance to know how the PWM,
> > > that I happen to use, behaves? To be able to get my consumer driver
> > > reliable I might need to know that however.
> >
> > No, there's currently no way of knowing. As such, I think the most
> > sensible thing to do at this point is to work with the API in order to
> > get the behaviour that you want.
>
> I want to switch from (duty_cycle, period) = (d1, p1) to
> (duty_cycle, period) = (d2, p2) without seeing (d1, p2) or (d2, p1) on
> the wire. How do I do this with Lokesh's patch applied?
>
> In my eyes this is something sensible to rely on. I agree that relying
> on a stop implemented by the driver is broken, but not wanting to seeing
> a (d1, p2) is sane.

I suppose it could be a sane requirement under some circumstances. In
other circumstances it may not matter. Making it a hard requirement
unconditionally would preclude the driver's usage in cases where it
doesn't matter.

Now, for many cases that als can never happen because the period is
always fixed. For example, the PWM consumer bindings specify that a
consumer should provide a fixed period in device tree. So once the
PWM has been configured that will never change, so you can only ever
be in a situation where you want to go from (d1, p) to (d2, p), so
there won't actually be a glitch.

That's certainly not true for all cases, and specifically with sysfs
you can always change the period as well as the duty-cycle.

> > Now if you're unlucky the driver won't support that and you'll notice
> > eventually. But there's also nothing you can do about that if the
> > hardware doesn't work that way. Even if the PWM framework had a way of
> > querying these types of peculiarities it wouldn't really proved much of
> > an advantage since the driver would just refuse to probe rather than
> > attempting to do this and maybe succeeding because you got lucky.
>
> It depends on the use case. If it's about a display that then has no
> backlight the damage is limited. If however I drive a step motor that
> rotates a laser some unexpected behaviour that only happens once in
> 100000 cases and so isn't catched during development is quite bad.

You probably wouldn't be using sysfs to control the PWM in such a case.
My understanding is that stepper motors also typically use either a
fixed duty-cycle or a fixed period. I would also think that it wouldn't
matter if you stopped a PWM, reconfigured it and then started it again
for that particular application.

So I understand that there can be situations where this kind of glitch
isn't optimal, but I think we also need to be pragmatic and see what the
real use-cases are. I doubt that anyone trying to control a potentially
lethal laser would be using some standard PWM IP and run Linux to
control it. There are just too many things that could go wrong
(scheduling interfering, kernel crashing, ...) for that to be a good
idea.

> > > > > > I know this is perhaps cheating a little, or turning a blind eye, but I
> > > > > > don't know what the alternative would be. Do we want to tell people that
> > > > > > a given PWM controller can't be used if it doesn't work according to our
> > > > > > expectations? That's hard to argue if that controller works just fine
> > > > > > for all known use-cases.
> > > > >
> > > > > I'd like have some official policy here which of the alternatives is the
> > > > > preferred cheat.
> > > > >
> > > > > The situation here is that period and duty_cycle cannot be updated
> > > > > atomically. So the two options are:
> > > > >
> > > > > - stop shortly
> > > > > - update with hardware running and maybe emit a broken period
> > > >
> > > > I think we can already support both of those with the existing API. If
> > > > a consumer wants to stop the PWM while reconfiguring, they should be
> > > > able to do pwm_enable(), pwm_config(), pwm_enable() (or the atomic
> > > > equivalent) and for the second case they can just do pwm_config() (or
> > > > the atomic equivalent).
> > >
> > > Yes, the consumer can force the stop and update. But assume I'm "only" a
> > > consumer driver author and I want: atomic update and if this is not
> > > possible I prefer "stop-to-update" over "on-the-fly-and-maybe-faulty".
> > > So I cannot benefit from a good driver/hardware that can do atomic
> > > updates? Or I have to patch each driver that I actually use to use
> > > stop-to-update?
> >
> > You could do what everybody does and just assume that atomic update
> > works.
>
> There are people out there that are more demanding. If you have 1000000
> machines in the field and only then find out that they all fail to
> operate correctly with a certain small but positive probability and you
> have to send someone to each machine to fix that, that's bad.

Agreed. But that's not really what we're talking about here, right? This
isn't some undefined behaviour that would spontaneously trigger. We're
still talking about deterministic behaviour that's going to be the same
whether you do it once in a test lab or a million times in the field. A
PWM-controlled backlight that you adjust the brightness of in the lab is
going to work the same way as in the field. Any potential glitch would
be happening in the lab as well. It would in fact, constantly happen.

> > If it works with the particular PWM device/driver that you
> > have that may be all you care about. Obviously that may not be true
> > for a different chip.
> >
> > On the other hand, if you absolutely must ensure that there must never
> > be any glitches whatsoever but don't care whether the PWM goes through a
> > disable/enable sequence, then doing so explicitly is going to be your
> > best bet. From a consumer driver point of view it isn't going to matter
> > because even if we had a way of distinguishing between these
> > capabilities you'd still have to have code to deal with both.
>
> There is a nice upside however: You have a good chance to notice this
> problem before the 1000000 devices are shipped to production and then
> escalate the problem and let the responsible people make an informed
> choice.

I suspect that what will really happen is that somebody will just hack
the driver and remove the restriction, check that it works and ship it.

> > So the only relevant use-case here would be if you had a requirement to
> > perform on-the-fly-and-strictly-correct updates. The API doesn't give
> > those kinds of guarantees. So we would need an extension that consumers
> > can query to determine whether what they want to do will work.
> >
> > But like I said above, the chances that you will run into this are slim
> > because use-cases are typically known ahead of time and devices are
> > designed to be able to support them. So if you build a device that needs
> > to support these strict requirements, then you need to make sure the
> > hardware supports it, too. And when the hardware supports it, then the
> > driver should implement ->config() or ->apply() in a way that allows
> > this.
>
> In my bubble that sometimes fails. The hardware is done and only then
> someone notices that a certain case wasn't considered.

Of course this sometimes happens. And then it's actually great if the
API isn't all too pedantic and still allows you to adjust that backlight
brightness rather than flatout refusing to do anything, leaving you with
a bunch of devices that you have to scrap and then do a redesign.

So ultimately you end up "faking it", but if it works, it works.

For the rare cases where you can't fake it, it doesn't really matter
whether the framework refuses to do something because it knows that the
hardware isn't capable of doing it, or whether the driver will simply
try its best to do what it was asked to do and fails to deliver. You'll
notice one way or another and then you need to go back to the drawing
board and redo the design.

> > > > Some hardware may actually require the PWM to be disabled before
> > > > reconfiguring, so they won't be able to strictly adhere to the second
> > > > use-case.
> > > >
> > > > But as discussed above, I don't want to strive for a lowest common
> > > > denominator that would preclude some more specific use-cases from
> > > > working if the hardware supports it.
> > > >
> > > > So I think we should aim for drivers to implement the semantics as
> > > > closely as possible. If the hardware doesn't support some of these
> > > > requirements strictly while a particular use-case depends on that, then
> > > > that just means that the hardware isn't compatible with that use-case.
> > > > Chances are that the system just isn't going to be designed to support
> > > > that use-case in the first place if the hardware can't do it.
> > > >
> > > > The sysfs interface is a bit of a special case here because it isn't
> > > > possible to know what use-cases people are going to come up with.
> > >
> > > In my eyes the sysfs interface isn't special here. You also don't know
> > > what the OMAP PWM hardware is used for.
> >
> > But platform designers will know what their device will be used for.
>
> Right. But if you rely on the people chosing the components for the
> platform to notice that the SoC's PWM has a glitch that is bad for the
> use case I think that's naive.

See above. If the glitch is bad enough to make it unusable you're going
to notice and you have to redesign, whether the PWM framework rejects a
configuration or not.

My point is that if the glitch is minor enough not to be noticed, the
PWM framework not getting in the way may make you (and whoever's got
their money in this) very happy.

> > > > It's most likely that they'll try something and if it doesn't work
> > > > they can see if a driver patch can improve things.
> > >
> > > So either the group who prefers "stop-to-update" or the group who
> > > prefers "on-the-fly-and-maybe-faulty" has to carry a system specific
> > > driver patch?
> >
> > No, the group that prefers "stop-to-update" should make that explicit
> > and write a consumer driver that first disables, then reconfigured and
> > then reenables the PWM.
>
> Both groups prefer atomic updates, and just prefer different cluches if
> atomic isn't possible. So doing an explicit stop is bad. Also I'd expect
> the timing of a driver doing the stop is better than when the consumer
> explicitly stops.

Yes, perhaps. It obviously depends a lot on what exactly the time is
that we're talking about here. If we're talking about a single pulse
every second, whether the driver stops and restarts or whether the
consumer does is probably negligible.

But again, I think if we really run into cases where the consumer knows
exactly which behaviour it wants, then I think we need to extend the API
to enable it to transport such notions.

We currently can't, and assumptions baked into the API are bound to be
the wrong ones every now and then.

> > If they don't *need* to update the PWM on the fly, then performing two
> > additional steps that would be happening anyway won't matter, right?
>
> I don't agree here. Moreover the consumer might be preempted between
> stop and reconfiguration. (And you don't want to disable preemption over
> a pwm-API calls as they might take quite some time assuming they block
> until periods are completed.)

I hate to break it to you, but if timing is really that critical, you're
probably better off not using Linux.

> > > > One possible extension that I can imagine would be to introduce some
> > > > sort of capability structure that drivers can fill in to describe the
> > > > behaviour of the hardware. Drivers like pwm-omap-dmtimer, for example,
> > > > could describe that they are able to change the period and/or duty cycle
> > > > while the PWM is on. There could be another capability bit that says
> > > > that the current period will finish before new settings are applied. Yet
> > > > another capability could describe that duty-cycle and period can be
> > > > applied atomically. Consumers could then check those capabilities to see
> > > > if they match their requirements.
> > > >
> > > > But then again, I think that would just make things overly complicated.
> > > > None of the existing consumers need that, so it doesn't seem like there
> > > > is much demand for that feature. In practice I suspect most consumers
> > > > work fine despite potentially small deviations in how the PWM behaves.
> > >
> > > I think the status quo is what I asked about above: People use sysfs and
> > > if the PWM behaves different than needed, the driver is patched and most
> > > of the time not mainlined. If your focus is to support a certain
> > > industrial system with a defined use case, this is fine. If however you
> > > target for an universal framework that works for any combination of
> > > consumer + lowlevel driver without patching (that at least is able to
> > > diagnose: This PWM cannot provide what my consumer needs), this is bad.
> >
> > Again, my response to this is: how is this going to be beneficial? In
> > practice the way that this would work is that the consumer driver would
> > fail if presented with a PWM that doesn't meet the strict requirements.
> > Now if the requirements really are that strict, then that sounds like a
> > good idea.
> >
> > But one issue I foresee with this is that we'll end up giving consumers
> > too much of a toolkit to restrict things. What if the consumer driver
> > author assumes wrongly that a given set of requirements exists?
>
> If you want to catch usage errors you're doomed anyhow.
>
> > What if for some combination of hardware that doesn't strictly conform
> > to those requirements it might still work?
>
> Then this is a bug that can be fixed. And it's a bug that is not in the
> PWM framework's area, so we even don't really have to care.

It's still the responsibility of the PWM framework to provide the knobs
that make sense in terms of what the hardware capabilities are.

> > Sometimes you may not notice the difference, at other times there may
> > be some impact like a visual glitch or so, but that may be something
> > that users are willing to accept rather than not have support at all.
>
> If laxer requirements are ok, they shouldn't request strict ones.

Well yeah. That's exactly why I'm pushing back on this. So far I haven't
yet seen a case where there actually were any such strict requirements.

> > > Also this means that whenever a system designer changes something on
> > > their machine (kernel update, different hardware, an new usecase for a
> > > PWM) they might have to reverify if the given PWM driver behaves as
> > > needed.
> >
> > I don't expect this type of change to happen very often.
>
> Yeah, I also expect that most things don't break on such a change. But
> Lokesh's patch is an example that justifies to be vigilant because the
> implemented change in behaviour is a good one for Lokesh but might break
> things for someone else. And I don't expect that the common test
> coverage to be that good to catch the glitch during test.

That's certainly true, but so far nobody's complained about this patch
breaking their use-case, so I'm going to assume for now that it's
harmless. If that changes at some point we need to revisit this of
course.

However, it's no use sitting on patches waiting for somebody to report a
regression, otherwise we could never make changes to a driver. Does the
absence of a regression report mean that there is no regression, or that
the regression just hasn't been noticed yet?

> > There's always going to be some type of fine-tuning before a driver's
> > behaviour is completely stabilized. And then there could still always
> > be other factors impacting behaviour that aren't even related to the
> > PWM framework.
>
> I think I don't get what you intend to say here. I understand "As
> updating the kernel might make a PLL unstable, changing behaviour for
> the PWM is fine."

What I'm trying to say is that it's normal for a driver to go through
these kinds of changes. It used to work for some use-cases, now it's
being extended to work for one more. The behaviour is now slightly
different, but hopefully it won't impact existing use-cases.

That's in contrast to saying that we can never change existing drivers
because it may cause existing use-cases to fail.

Furthermore there are so many moving parts in Linux that upgrading to a
new kernel always requires you to revalidate functionality.

> > I suspect that most people will have an array of tests to validate that
> > everything still works after a kernel update. Obviously we don't want a
> > new kernel to behave completely differently, but we're not talking about
> > that here. This is merely dropping a needless disable/enable from a
> > configuration. If somebody was relying on this happening they were wrong
> > to rely on it to begin with because the API does not guarantee it.
> >
> > > My suggestion for now is to start documenting how the drivers behave
> > > expanding how limitations are documented in some drivers. So maybe
> > > change from "Limitations" to "Implementation and Hardware Details"?
> >
> > Yes, collecting such information is always a good idea.
>
> So Lokesh's patch should at least be amended to add something like
>
> * Limitations:
> * - When changing both duty cycle and period, we cannot prevent in
> * software that the output might produce a period with mixed
> * settings (new period length and old duty cycle) without stopping
> * the hardware.
>
> at the top of the driver.

Something to that effect already exists in the driver.

Thierry

Attachments:

(No filename) (19.19 kB)
signature.asc (849.00 B)
Download all attachments

2020-04-02 14:06:23

by Uwe Kleine-König

[permalink] [raw]

Subject: Re: [PATCH v3 4/5] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

Hello Thierry,

On Wed, Apr 01, 2020 at 11:37:38PM +0200, Thierry Reding wrote:
> On Wed, Apr 01, 2020 at 10:31:56PM +0200, Uwe Kleine-K?nig wrote:
> > There are people out there that are more demanding. If you have 1000000
> > machines in the field and only then find out that they all fail to
> > operate correctly with a certain small but positive probability and you
> > have to send someone to each machine to fix that, that's bad.
>
> Agreed. But that's not really what we're talking about here, right? This
> isn't some undefined behaviour that would spontaneously trigger.
> We're still talking about deterministic behaviour that's going to be
> the same whether you do it once in a test lab or a million times in
> the field. A PWM-controlled backlight that you adjust the brightness
> of in the lab is going to work the same way as in the field. Any
> potential glitch would be happening in the lab as well. It would in
> fact, constantly happen.

The glitch we're talking about really doesn't happen that often that we
should rely on this problem popping up during testing. To change both
period and duty_cycle two register writes are necessary. The hickup only
happens if after the first register write a period ends before the
second register write hits the hardware. (For the omap driver it might
happen a bit more often, don't remember the details, but I think the
above is what could be reached theoretically.)

> For the rare cases where you can't fake it, it doesn't really matter
> whether the framework refuses to do something because it knows that the
> hardware isn't capable of doing it, or whether the driver will simply
> try its best to do what it was asked to do and fails to deliver. You'll
> notice one way or another and then you need to go back to the drawing
> board and redo the design.

I think you didn't understand up to now that the glitch depends on
timing between register access and the PWM period and so is really hard
to actually reproduce. In the hope you understood that now, I assume you
see that your argumentation is incomplete.

And I think to assume that somebody would complain about a race
condition in a patch that just hit next is quite optimistic.

Having said that I don't know how critical this really is. Given that
the PWM under discussion doesn't complete periods on stop, it probably
isn't.

I spend some time thinking about when the glitch actually happens.
Currently the load value is written first and then the match value.
If no period ends between the two writes there is only a problem when in
the currently running period the match event didn't happen yet. Then we
see a cycle with

.period = oldperiod + newperiod
.dutycycle = oldperiod + newdutycycle

(if the new match value isn't hit in the current cycle) or one with

.period = oldperiod
.duty_cycle = newdutycycle + (oldperiod - newperiod)

(if the new match value is hit in the current cycle). The probability
that one of the two happen is: olddutycycle / oldperiod which is quite
probable. (With olddutycycle = oldperiod there is no problem though.)

If after writing the new load value and before writing the new match
value a period ends it might happen that we see a cycle with

.period = newperiod
.dutycycle = olddutycycle + (newperiod - oldperiod)

(if the previous match value is used) or one with

.period = 2 * newperiod
.dutycycle = newperiod + newdutycycle

(if new match value is written too late for the first cycle with the new
period).

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |

2020-04-03 08:52:59

by Lokesh Vutla

[permalink] [raw]

Subject: Re: [PATCH v3 4/5] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

On 02/04/20 7:32 PM, Uwe Kleine-König wrote:
> Hello Thierry,
>
> On Wed, Apr 01, 2020 at 11:37:38PM +0200, Thierry Reding wrote:
>> On Wed, Apr 01, 2020 at 10:31:56PM +0200, Uwe Kleine-König wrote:
>>> There are people out there that are more demanding. If you have 1000000
>>> machines in the field and only then find out that they all fail to
>>> operate correctly with a certain small but positive probability and you
>>> have to send someone to each machine to fix that, that's bad.
>>
>> Agreed. But that's not really what we're talking about here, right? This
>> isn't some undefined behaviour that would spontaneously trigger.
>> We're still talking about deterministic behaviour that's going to be
>> the same whether you do it once in a test lab or a million times in
>> the field. A PWM-controlled backlight that you adjust the brightness
>> of in the lab is going to work the same way as in the field. Any
>> potential glitch would be happening in the lab as well. It would in
>> fact, constantly happen.
>
> The glitch we're talking about really doesn't happen that often that we
> should rely on this problem popping up during testing. To change both
> period and duty_cycle two register writes are necessary. The hickup only
> happens if after the first register write a period ends before the
> second register write hits the hardware. (For the omap driver it might
> happen a bit more often, don't remember the details, but I think the
> above is what could be reached theoretically.)
>
>> For the rare cases where you can't fake it, it doesn't really matter
>> whether the framework refuses to do something because it knows that the
>> hardware isn't capable of doing it, or whether the driver will simply
>> try its best to do what it was asked to do and fails to deliver. You'll
>> notice one way or another and then you need to go back to the drawing
>> board and redo the design.
>
> I think you didn't understand up to now that the glitch depends on
> timing between register access and the PWM period and so is really hard
> to actually reproduce. In the hope you understood that now, I assume you
> see that your argumentation is incomplete.
>
> And I think to assume that somebody would complain about a race
> condition in a patch that just hit next is quite optimistic.
>
> Having said that I don't know how critical this really is. Given that
> the PWM under discussion doesn't complete periods on stop, it probably
> isn't.

It is a limitation with the existing driver as well. Nothing is being changed
regarding stopping of PWM. The same is marked under the limitations in the driver.

>
> I spend some time thinking about when the glitch actually happens.
> Currently the load value is written first and then the match value.
> If no period ends between the two writes there is only a problem when in
> the currently running period the match event didn't happen yet. Then we
> see a cycle with
>
> .period = oldperiod + newperiod
> .dutycycle = oldperiod + newdutycycle
>
> (if the new match value isn't hit in the current cycle) or one with
>
> .period = oldperiod
> .duty_cycle = newdutycycle + (oldperiod - newperiod)
>
> (if the new match value is hit in the current cycle). The probability
> that one of the two happen is: olddutycycle / oldperiod which is quite
> probable. (With olddutycycle = oldperiod there is no problem though.)
>
> If after writing the new load value and before writing the new match
> value a period ends it might happen that we see a cycle with
>
> .period = newperiod
> .dutycycle = olddutycycle + (newperiod - oldperiod)
>
> (if the previous match value is used) or one with
>
> .period = 2 * newperiod
> .dutycycle = newperiod + newdutycycle
>
> (if new match value is written too late for the first cycle with the new
> period).

That's exactly why we have marked in the Limitations sections that the current
period might produce a cycle with mixed settings. Frankly, I'm a bit torn here.
There are other PWMs inside Linux with similar limitations and documented
similarly. If there is an overall objection for such hardware, the entire policy
should be changed or the framework should be updated to allow user to choose for
dynamic updates. IMHO, this series should not be blocked for this decision.
Please consider it for the coming merge window.

Thanks and regards,
Lokesh

2020-04-03 14:05:40

by Uwe Kleine-König

[permalink] [raw]

Subject: Re: [PATCH v3 4/5] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

Hello,

On Fri, Apr 03, 2020 at 02:21:38PM +0530, Lokesh Vutla wrote:
> On 02/04/20 7:32 PM, Uwe Kleine-K?nig wrote:
> > Having said that I don't know how critical this really is. Given that
> > the PWM under discussion doesn't complete periods on stop, it probably
> > isn't.
>
> It is a limitation with the existing driver as well. Nothing is being changed
> regarding stopping of PWM. The same is marked under the limitations in the driver.

What I wrote was ambiguous and I think you understood the meaning I
didn't intend. What I wanted to say is: Given that the PWM stops
abruptly there is only little (if any at all) advantage of
"stop-to-update" over "racy-atomic-update" as we see broken cycles no
matter which alternative we pick.

> > I spend some time thinking about when the glitch actually happens.
> > Currently the load value is written first and then the match value.
> > If no period ends between the two writes there is only a problem when in
> > the currently running period the match event didn't happen yet. Then we
> > see a cycle with
> >
> > .period = oldperiod + newperiod
> > .dutycycle = oldperiod + newdutycycle
> >
> > (if the new match value isn't hit in the current cycle) or one with
> >
> > .period = oldperiod
> > .duty_cycle = newdutycycle + (oldperiod - newperiod)
> >
> > (if the new match value is hit in the current cycle). The probability
> > that one of the two happen is: olddutycycle / oldperiod which is quite
> > probable. (With olddutycycle = oldperiod there is no problem though.)
> >
> > If after writing the new load value and before writing the new match
> > value a period ends it might happen that we see a cycle with
> >
> > .period = newperiod
> > .dutycycle = olddutycycle + (newperiod - oldperiod)
> >
> > (if the previous match value is used) or one with
> >
> > .period = 2 * newperiod
> > .dutycycle = newperiod + newdutycycle
> >
> > (if new match value is written too late for the first cycle with the new
> > period).
>
> That's exactly why we have marked in the Limitations sections that the current
> period might produce a cycle with mixed settings. Frankly, I'm a bit torn here.
> There are other PWMs inside Linux with similar limitations and documented
> similarly. If there is an overall objection for such hardware, the entire policy
> should be changed or the framework should be updated to allow user to choose for
> dynamic updates. IMHO, this series should not be blocked for this decision.

Yes, there are other drivers that have similar "problems" and the status
quo is that depending on the driver author one or the other workaround
is chosen. I think the PWM framework would benefit if there were a
common guideline which path to choose in such a situation.

> Please consider it for the coming merge window.

It's already in next, so I assume it will be merged.

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |