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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4d830367-403a-5cf5-abf0-7daccbece1ae@ti.com>
Date:   Tue, 25 Feb 2020 16:56:02 +0530
From:   Lokesh Vutla <lokeshvutla@...com>
To:     Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
CC:     Thierry Reding <thierry.reding@...il.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>, <kernel@...gutronix.de>
Subject: Re: [PATCH 3/4] pwm: omap-dmtimer: Do not disable pwm before changing
 period/duty_cycle

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