linux-kernel - Re: [PATCH v6 1/1] pwm: imx27: workaround of the pwm output bug when decrease the duty cycle

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: <ab75066d-31ae-4725-b524-9cf6720bc866@denx.de>
Date: Wed, 2 Oct 2024 21:48:12 +0200
From: Marek Vasut <marex@...x.de>
To: Frank Li <Frank.li@....com>
Cc: conor+dt@...nel.org, devicetree@...r.kernel.org, festevam@...il.com,
 francesco@...cini.it, imx@...ts.linux.dev, jun.li@....com,
 kernel@...gutronix.de, krzk+dt@...nel.org,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 linux-pwm@...r.kernel.org, p.zabel@...gutronix.de, pratikmanvar09@...il.com,
 robh@...nel.org, s.hauer@...gutronix.de, shawnguo@...nel.org,
 ukleinek@...nel.org, xiaoning.wang@....com
Subject: Re: [PATCH v6 1/1] pwm: imx27: workaround of the pwm output bug when
 decrease the duty cycle

On 10/2/24 9:45 PM, Frank Li wrote:
> On Sun, Sep 22, 2024 at 10:28:02PM +0200, Marek Vasut wrote:
>> Hi,
>>
>> On 9/17/24 9:25 PM, Frank Li wrote:
>>
>> [...]
>>
>>> @@ -223,6 +224,8 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>>>    	struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
>>>    	unsigned long long c;
>>>    	unsigned long long clkrate;
>>> +	unsigned long flags;
>>> +	int val;
>>>    	int ret;
>>>    	u32 cr;
>>> @@ -263,7 +266,69 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>>
>> [...]
>>
>>> +	c = clkrate * 1500;
>>> +	do_div(c, NSEC_PER_SEC);
>>> +
>>> +	local_irq_save(flags);
>>> +	val = FIELD_GET(MX3_PWMSR_FIFOAV, readl_relaxed(imx->mmio_base + MX3_PWMSR));
>>
>> I think the multi-write I mentioned in v5 for > 500 kHz case could further
>> improve the patch, let's see what others think:
>>
>> if (state->period < 2000) { /* 2000ns = 500 kHz */
>>     /* Best effort attempt to fix up >500 kHz case */
>>     udelay(6); /* 2us per FIFO entry, 3 FIFO entries written => 6 us */
>>     writel_relaxed(duty_cycles, imx->mmio_base + MX3_PWMSAR);
>>     writel_relaxed(duty_cycles, imx->mmio_base + MX3_PWMSAR);
>>     /* Last write is outside, after this conditional */
>> } else if (duty_cycles ...

Can you have a look at this part ?

>>> +	if (duty_cycles < imx->duty_cycle && val < MX3_PWMSR_FIFOAV_2WORDS) {
>>> +		val = readl_relaxed(imx->mmio_base + MX3_PWMCNR);
>>> +		/*
>>> +		 * If counter is close to period, controller may roll over when
>>> +		 * next IO write.
>>> +		 */
>>
>> c is only used in this if (duty_cycles ...) { } conditional, the do_div()
>> above can be moved here:
> 
> It is in local_irq_save(flags) scope, it'd better as less as possible. So
> I prefer do_div() is outside local_irq_save()
Good point, either way is fine by me.