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

[Lokesh Vutla <lokeshvutla@ti.com>]

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