Hi Uwe, On 12/03/20 4:14 PM, Lokesh Vutla wrote: > 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]. Not sure what is the conclusion here. If there are no objections on this series, can it be merged? Thanks and regards, Lokesh > > >> >> 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 >
