Message-ID: <20200401203626.dknkg53uxzatwvmg@pengutronix.de>
Date:   Wed, 1 Apr 2020 22:36:26 +0200
From:   Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
To:     Thierry Reding <thierry.reding@...il.com>
Cc:     Lokesh Vutla <lokeshvutla@...com>,
        Tony Lindgren <tony@...mide.com>,
        Linux OMAP Mailing List <linux-omap@...r.kernel.org>,
        linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org,
        Sekhar Nori <nsekhar@...com>, Vignesh R <vigneshr@...com>,
        kernel@...gutronix.de
Subject: Re: [PATCH v3 4/5] pwm: omap-dmtimer: Do not disable pwm before
 changing period/duty_cycle

On Wed, Apr 01, 2020 at 08:39:19PM +0200, Thierry Reding wrote:
> On Wed, Apr 01, 2020 at 01:47:32PM +0200, Uwe Kleine-König wrote:
> > On Wed, Apr 01, 2020 at 03:52:21PM +0530, Lokesh Vutla wrote:
> > > Hi Uwe,
> > > 
> > > On 01/04/20 1:52 PM, 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?
> > > > 
> > > > 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.
> > > > 
> > > >>>> 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?
> > > > 
> > > >> 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.
> > > > 
> > > >> 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?
> > > > 
> > > >> 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.
> > > > 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.
> > > > 
> > > > 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"?
> > > 
> > > Does it help if a new DT property is introduced across PWM subsystem,
> > > representing dynamic period/duty-cycle updates. Based on this property driver
> > > can handle the updates. If the property is not present existing behaviour can be
> > > restored. This way based on the use-case things can be changed and need not
> > > patch the driver :). Does this sound good or you have other thoughts?
> > 
> > That's something that I'd rather see in the pwm API. (Either by a rule
> > that drivers should prefer one or the other, or by making it
> > configurable.) IMHO this property doesn't belong into the hardware
> > description as it is a software property.
> > 
> > That's not constructive though as I don't have an idea how to map this
> > into the API.
> 
> We can already enforce disable/config/enable with the existing API. The
> only think that we can't enforce is that a configuration will always be
> applied atomically or without disabling and reenabling the PWM.
> 
> One possible solution would be to extend struct pwm_state with a set of
> flags that can be set. For that PTP kind of applications, consumers
> could set some pwm_state.strict (or whatever) flag and then a driver
> could fail ->apply() if it doesn't support changing the period/duty-
> cycle atomically and without disabling the PWM first. Or it could be
> more fine-grained, like:
> 
> 	state.on_the_fly = true;
> 	state.consistent = true;
> 
> To specify that the PWM needs to be changed on the fly (i.e. without
> disabling and reenabling) and duty-cycle and period must be consistent
> (i.e. be applied to the signal at the same time).

I'm happy for now with explicitly documenting the quirks as we become
aware of them. This is already much better than the status quo and we're
not complicating stuff in a way that might be far from reality.

I still think stating a preference for one or the other might be
beneficial, but as we don't seem to agree on that, let's go with
documentation for now. Then maybe later with a better overview about the
capabilities of the available drivers we can make a good choice.

Best regards
Uwe

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

Powered by blists - more mailing lists