linux-kernel - Re: [PATCH v3 4/5] 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: <2a5a06cd-7aca-c450-b048-33329d058eca@ti.com>
Date:   Thu, 12 Mar 2020 16:14:34 +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>, 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

Hi Uwe,

On 12/03/20 2:17 PM, Uwe Kleine-König wrote:
> On Thu, Mar 12, 2020 at 01:35:32PM +0530, Lokesh Vutla wrote:
>> On 12/03/20 12:10 PM, 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.
>>
>> Short stop has side effects especially in the case where 1PPS is generated using
>> this PWM. In this case where PWM period is continuously synced with PTP clock,
>> cannot expect any breaks in PWM. This doesn't fall in the above limitations as
>> well. as duty_cycle is not a worry and only the rising edge is all that matters.
>>
>> Also any specific reason why you wanted to stop rather than having the mentioned
>> limitation? it is just a corner anyway and doesn't happen all the time.
> 
> I'm a bit torn here. Which of the two steps out of line is worse depends
> on what is driven by the PWM in question. And also I think ignoring
> "just corner cases" is a reliable way into trouble.

I do agree that corner cases should not be ignored. But in this particular
driver, just trying to explain the effect of this corner case. On dynamic pwm
period update, the current pwm cycle might generate a period with mixed
settings. IMHO, it is okay to live with it and mark it as a limitation as you
pointed out in case of sifive driver[0].


> 
> The usual PWM contributer (understandably) cares mostly about their own
> problem they have to solve. If however you take a step back and care
> about the PWM framework as a whole to be capable to solve problems in
> general, such that any consumer just has to know that there is a PWM and
> start requesting specific settings for their work to get done, it gets
> obvious that you want some kind of uniform behaviour of each hardware
> driver. And then a short inactive break between two periods is more
> common and better understandable than a mixed period.

But the problem here is that inactive breaks between two periods is not desired.
Because the pwm is used to generate a 1PPS signal and is continuously
synchronized with PTP clock.

I am up if this can be solved generically. But updating period is very specific
to hardware implementation. Not sure what generic solution can be brought out of
this. Please correct me if I am wrong.

[0]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pwm/pwm-sifive.c#n7

Thanks and regards,
Lokesh