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

[Uwe Kleine-König]

On Tue, Feb 25, 2020 at 04:56:02PM +0530, Lokesh Vutla wrote:
> Hi Uwe,
> 
> On 25/02/20 2:08 PM, Uwe Kleine-König wrote:
> > Hello Lokesh,
> > 
> > On Tue, Feb 25, 2020 at 01:29:57PM +0530, Lokesh Vutla wrote:
> >> On 25/02/20 12:18 PM, Uwe Kleine-König wrote:
> >>> On Tue, Feb 25, 2020 at 10:32:42AM +0530, Lokesh Vutla wrote:
> >>>> On 24/02/20 2:25 PM, Uwe Kleine-König wrote:
> >>>>> On Mon, Feb 24, 2020 at 10:51:34AM +0530, Lokesh Vutla wrote:
> >>>>>>  	omap->pdata->set_load(omap->dm_timer, true, load_value);
> >>>>>>  	omap->pdata->set_match(omap->dm_timer, true, match_value);
> >>>>>
> >>>>> (Without having looked into the depths of the driver I assume
> >>>>> .set_load() sets the period of the PWM and .set_match() the duty cycle.)
> >>>>
> >>>> Right.
> >>>>
> >>>>>
> >>>>> What happens on a running PWM if you change the period? Consider you
> >>>>> change from duty_cycle = 1000, period = 5000 to duty_cycle = 4000,
> >>>>> period = 10000. As you set the period first, can it happen the hardware
> >>>>> produces a cycle with duty_cycle = 1000, period = 10000?
> >>>>
> >>>> No. So, the current cycle is un affected with duty_cycle = 1000 and period =
> >>>> 5000. Starting from next cycle new settings gets reflected with duty_cycle =
> >>>> 4000 and period = 10000.
> >>>
> >>> Is the reference manual for this hardware publically available?
> >>
> >> AM335x TRM [0] Section 20.1.3.5 Pulse-Width Modulation (Page 4445).
> >>
> >> [0] http://www.ti.com/lit/ug/spruh73q/spruh73q.pdf
> > 
> > Great. This is BTW an opportunity to increase your patch count: Create a
> > patch that adds a reference to this document at the top of the driver.
> > 
> >>> So the .set_load callback just writes a shadow register and .set_match
> >>> latches it into hardware atomically with its own register changes? A
> >>> comment in the source code about this would be good. Also if .set_load
> >>> doesn't work without .set_match I wonder if it is sane to put their
> >>> logic in two different functions.
> >>
> >> Just to give a little bit of background:
> > 
> > Thanks, very appreciated.
> > 
> >> - The omap timer is an upward counter that can be started and stopped at any time.
> >> - Once the timer counter overflows, it gets loaded with a predefined load
> >> value.(Or can be configured to not re load at all).
> >> - Timer has a configurable output pin which can be toggled in the following two
> >> cases:
> >> 	- When the counter overflows
> >> 	- When the counter matches with a predefined register(match register).
> >>
> >> Using this o/p pin the driver tries to generate a PWM with period = (OVERFLOW -
> >> LOAD_VALUE) and duty_cycle = (MATCH_VALUE - LOAD_VALUE).
> >>
> >> .set_load will configure the load value .set_match will configure the match
> >> value. The configured values gets effected only in the next cycle of PWM.
> > 
> > Ah, so back to my original question: If you change from
> > duty_cycle/period = 1000/5000 to duty_cycle/period = 4000/10000 and
> > after you set the period but before you set the duty_cycle a period
> > happens to end, you get indeed a cycle with mixed settings, right?
> 
> hmm..you are right but the mixed period happens in a bit different case. Let me
> explain in bit more detail.
> 
> For omap dm timer, the load_value that gets set in the current period, will be
> reflected only in next cycle, as timer counter has to overflow to load this
> value. But in case of match register(which determines the duty cycle), the timer
> counter is continuously matched to it. So below are the cases where a mixed
> period can happen:
> 1) When signal is high and new match value is > current timer counter. Then the
> duty cycle gets reflected in the current cycle.(Duty_cycle for current period=
> new match value -  previous load  value).
> 2) When signal is high and new match value is < current timer counter. Then the
> period and duty cycle for the current cycle gets effected as well. Because the
> signal is pulled down only when counter matches with match register, and this
> happens only in the next cycle(after timer counter overflows). Then:
> 	- new Period for current cycle = (current period + new period)
> 	- new duty cycle for current cycle =  (current period + new duty_cycle).
> 
> I am able to observe the above mentioned 2 behaviors on the scope using beagle
> bone black. So the problem is with updating duty cycle when the signal is high.
> but when signal is low, nothing gets effected to the current cycle.
> 
> How do you want to go about this? Should we describe this as limitation in the
> driver as you asked?

OK, to sumarize: You have a counter that goes up. When it overflows it
gets reloaded with the load value and the output goes up. When
$counter = $match, the output goes down.

Having this is a comment at the top of the driver would be very welcome.

(I just noticed I duplicated this question about a racy update in
another end of this thread. This pops in my head too automatically :-)

Best regards
Uwe

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