diff mbox series

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

Message ID 20200224052135.17278-4-lokeshvutla@ti.com (mailing list archive)
State New, archived
Headers show
Series pwm: omap-dmtimer: Allow for dynamic pwm period updates | expand

Commit Message

Lokesh Vutla Feb. 24, 2020, 5:21 a.m. UTC
Only the Timer control register(TCLR) can be updated only when the timer
is stopped. 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.

Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
 drivers/pwm/pwm-omap-dmtimer.c | 14 --------------
 1 file changed, 14 deletions(-)

Comments

Uwe Kleine-König Feb. 24, 2020, 8:55 a.m. UTC | #1
Hello,

On Mon, Feb 24, 2020 at 10:51:34AM +0530, Lokesh Vutla wrote:
> Only the Timer control register(TCLR) can be updated only when the timer
> is stopped. 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.
> 
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> ---
>  drivers/pwm/pwm-omap-dmtimer.c | 14 --------------
>  1 file changed, 14 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
> index f13be7216847..58c61559e72f 100644
> --- a/drivers/pwm/pwm-omap-dmtimer.c
> +++ b/drivers/pwm/pwm-omap-dmtimer.c
> @@ -102,7 +102,6 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
>  	u32 load_value, match_value;
>  	struct clk *fclk;
>  	unsigned long clk_rate;
> -	bool timer_active;
>  
>  	dev_dbg(chip->dev, "requested duty cycle: %d ns, period: %d ns\n",
>  		duty_ns, period_ns);
> @@ -178,25 +177,12 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
>  	load_value = (DM_TIMER_MAX - period_cycles) + 1;
>  	match_value = load_value + duty_cycles - 1;
>  
> -	/*
> -	 * We MUST stop the associated dual-mode timer before attempting to
> -	 * write its registers, but calls to omap_dm_timer_start/stop must
> -	 * be balanced so check if timer is active before calling timer_stop.
> -	 */
> -	timer_active = pm_runtime_active(&omap->dm_timer_pdev->dev);
> -	if (timer_active)
> -		omap->pdata->stop(omap->dm_timer);
> -
>  	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.)

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?

Best regards
Uwe
Lokesh Vutla Feb. 25, 2020, 5:02 a.m. UTC | #2
Hi Uwe,

On 24/02/20 2:25 PM, Uwe Kleine-König wrote:
> Hello,
> 
> On Mon, Feb 24, 2020 at 10:51:34AM +0530, Lokesh Vutla wrote:
>> Only the Timer control register(TCLR) can be updated only when the timer
>> is stopped. 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.
>>
>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>> ---
>>  drivers/pwm/pwm-omap-dmtimer.c | 14 --------------
>>  1 file changed, 14 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
>> index f13be7216847..58c61559e72f 100644
>> --- a/drivers/pwm/pwm-omap-dmtimer.c
>> +++ b/drivers/pwm/pwm-omap-dmtimer.c
>> @@ -102,7 +102,6 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
>>  	u32 load_value, match_value;
>>  	struct clk *fclk;
>>  	unsigned long clk_rate;
>> -	bool timer_active;
>>  
>>  	dev_dbg(chip->dev, "requested duty cycle: %d ns, period: %d ns\n",
>>  		duty_ns, period_ns);
>> @@ -178,25 +177,12 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
>>  	load_value = (DM_TIMER_MAX - period_cycles) + 1;
>>  	match_value = load_value + duty_cycles - 1;
>>  
>> -	/*
>> -	 * We MUST stop the associated dual-mode timer before attempting to
>> -	 * write its registers, but calls to omap_dm_timer_start/stop must
>> -	 * be balanced so check if timer is active before calling timer_stop.
>> -	 */
>> -	timer_active = pm_runtime_active(&omap->dm_timer_pdev->dev);
>> -	if (timer_active)
>> -		omap->pdata->stop(omap->dm_timer);
>> -
>>  	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.

Thanks and regards,
Lokesh
Uwe Kleine-König Feb. 25, 2020, 6:48 a.m. UTC | #3
Hello,

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:
> > Hello,
> > 
> > On Mon, Feb 24, 2020 at 10:51:34AM +0530, Lokesh Vutla wrote:
> >> Only the Timer control register(TCLR) can be updated only when the timer
> >> is stopped. 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.
> >>
> >> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> >> ---
> >>  drivers/pwm/pwm-omap-dmtimer.c | 14 --------------
> >>  1 file changed, 14 deletions(-)
> >>
> >> diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
> >> index f13be7216847..58c61559e72f 100644
> >> --- a/drivers/pwm/pwm-omap-dmtimer.c
> >> +++ b/drivers/pwm/pwm-omap-dmtimer.c
> >> @@ -102,7 +102,6 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
> >>  	u32 load_value, match_value;
> >>  	struct clk *fclk;
> >>  	unsigned long clk_rate;
> >> -	bool timer_active;
> >>  
> >>  	dev_dbg(chip->dev, "requested duty cycle: %d ns, period: %d ns\n",
> >>  		duty_ns, period_ns);
> >> @@ -178,25 +177,12 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
> >>  	load_value = (DM_TIMER_MAX - period_cycles) + 1;
> >>  	match_value = load_value + duty_cycles - 1;
> >>  
> >> -	/*
> >> -	 * We MUST stop the associated dual-mode timer before attempting to
> >> -	 * write its registers, but calls to omap_dm_timer_start/stop must
> >> -	 * be balanced so check if timer is active before calling timer_stop.
> >> -	 */
> >> -	timer_active = pm_runtime_active(&omap->dm_timer_pdev->dev);
> >> -	if (timer_active)
> >> -		omap->pdata->stop(omap->dm_timer);
> >> -
> >>  	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?

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.

Best regards
Uwe
Lokesh Vutla Feb. 25, 2020, 7:59 a.m. UTC | #4
Hi Uwe,

On 25/02/20 12:18 PM, Uwe Kleine-König wrote:
> Hello,
> 
> 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:
>>> Hello,
>>>
>>> On Mon, Feb 24, 2020 at 10:51:34AM +0530, Lokesh Vutla wrote:
>>>> Only the Timer control register(TCLR) can be updated only when the timer
>>>> is stopped. 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.
>>>>
>>>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>>>> ---
>>>>  drivers/pwm/pwm-omap-dmtimer.c | 14 --------------
>>>>  1 file changed, 14 deletions(-)
>>>>
>>>> diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
>>>> index f13be7216847..58c61559e72f 100644
>>>> --- a/drivers/pwm/pwm-omap-dmtimer.c
>>>> +++ b/drivers/pwm/pwm-omap-dmtimer.c
>>>> @@ -102,7 +102,6 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
>>>>  	u32 load_value, match_value;
>>>>  	struct clk *fclk;
>>>>  	unsigned long clk_rate;
>>>> -	bool timer_active;
>>>>  
>>>>  	dev_dbg(chip->dev, "requested duty cycle: %d ns, period: %d ns\n",
>>>>  		duty_ns, period_ns);
>>>> @@ -178,25 +177,12 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
>>>>  	load_value = (DM_TIMER_MAX - period_cycles) + 1;
>>>>  	match_value = load_value + duty_cycles - 1;
>>>>  
>>>> -	/*
>>>> -	 * We MUST stop the associated dual-mode timer before attempting to
>>>> -	 * write its registers, but calls to omap_dm_timer_start/stop must
>>>> -	 * be balanced so check if timer is active before calling timer_stop.
>>>> -	 */
>>>> -	timer_active = pm_runtime_active(&omap->dm_timer_pdev->dev);
>>>> -	if (timer_active)
>>>> -		omap->pdata->stop(omap->dm_timer);
>>>> -
>>>>  	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

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

I hope this is clear. Let me know if you need more details.

Thanks and regards,
Lokesh
Uwe Kleine-König Feb. 25, 2020, 8:38 a.m. UTC | #5
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?

Best regards
Uwe
Lokesh Vutla Feb. 25, 2020, 11:26 a.m. UTC | #6
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?

Thanks and regards,
Lokesh
Uwe Kleine-König Feb. 26, 2020, 4:35 p.m. UTC | #7
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
diff mbox series

Patch

diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
index f13be7216847..58c61559e72f 100644
--- a/drivers/pwm/pwm-omap-dmtimer.c
+++ b/drivers/pwm/pwm-omap-dmtimer.c
@@ -102,7 +102,6 @@  static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
 	u32 load_value, match_value;
 	struct clk *fclk;
 	unsigned long clk_rate;
-	bool timer_active;
 
 	dev_dbg(chip->dev, "requested duty cycle: %d ns, period: %d ns\n",
 		duty_ns, period_ns);
@@ -178,25 +177,12 @@  static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
 	load_value = (DM_TIMER_MAX - period_cycles) + 1;
 	match_value = load_value + duty_cycles - 1;
 
-	/*
-	 * We MUST stop the associated dual-mode timer before attempting to
-	 * write its registers, but calls to omap_dm_timer_start/stop must
-	 * be balanced so check if timer is active before calling timer_stop.
-	 */
-	timer_active = pm_runtime_active(&omap->dm_timer_pdev->dev);
-	if (timer_active)
-		omap->pdata->stop(omap->dm_timer);
-
 	omap->pdata->set_load(omap->dm_timer, true, load_value);
 	omap->pdata->set_match(omap->dm_timer, true, match_value);
 
 	dev_dbg(chip->dev, "load value: %#08x (%d), match value: %#08x (%d)\n",
 		load_value, load_value,	match_value, match_value);
 
-	/* If config was called while timer was running it must be reenabled. */
-	if (timer_active)
-		pwm_omap_dmtimer_start(omap);
-
 	mutex_unlock(&omap->mutex);
 
 	return 0;