linux-kernel - Re: [PATCH v10 0/3] Change PWM-controlled LED pin active mode and algorithm

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] [day] [month] [year] [list]

Message-ID: <zqkx7cx5nalslfmxeoxdnsjbvrvzajrjybsmsyeyc65a64sntr@gpc5qp6aoyp7>
Date: Fri, 27 Dec 2024 09:20:21 +0100
From: Uwe Kleine-König <u.kleine-koenig@...libre.com>
To: Nylon Chen <nylon.chen@...ive.com>
Cc: linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org, 
	devicetree@...r.kernel.org, Paul Walmsley <paul.walmsley@...ive.com>, 
	Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>, 
	Samuel Holland <samuel.holland@...ive.com>, linux-riscv@...ts.infradead.org
Subject: Re: [PATCH v10 0/3] Change PWM-controlled LED pin active mode and
 algorithm

Hello Nylon,

On Tue, Dec 24, 2024 at 05:38:58PM +0800, Nylon Chen wrote:
> According to the circuit diagram of User LEDs - RGB described in the
> manual hifive-unleashed-a00.pdf[0] and hifive-unmatched-schematics-v3.pdf[1].
> 
> The behavior of PWM is acitve-high.
> 
> According to the descriptionof PWM for pwmcmp in SiFive FU740-C000 Manual[2].
> 
> The pwm algorithm is (PW) pulse active time  = (D) duty * (T) period.
> The `frac` variable is pulse "inactive" time so we need to invert it.

I'm trying to understand that. You're saying that the PWMCMP register
holds the inactive time. Looking at the logic diagram (Figure 29) of
"SiFive FU740-C000 Manual v1p6" that is because pwms is feed into the
comparator after going through that XNOR where the lower input is always
0 (as pwmcmpXcenter is always 0) and so effectively counts backwards,
right?
In that case the sentence "The output of each comparator is high
whenever the value of pwms is greater than or equal to the corresponding
pwmcmpX." from the description of the Compare Registers is wrong.

With that assumption there are a few issues with the second patch:

 - The Limitations paragraph still says "The hardware cannot generate a
   100% duty cycle."
 - If pwm_sifive_apply() is called with state->duty_cycle = 0 the PWMCMP
   register becomes (1U << PWM_SIFIVE_CMPWIDTH) - 1 which results in a
   wave form that is active for 1 clock tick each period. That's bogus.
   If duty_cycle = 0 is requested, either make sure the output is
   inactive the whole time, or return an error.
 - With the above error in the official documentation, I'd like to have
   a code comment that explains the mismatch such that a future reader
   of the code has a chance to understand the situation without in
   detail review of the manual and the driver.

Orthogonal to your patches, I wonder about

	frac = DIV64_U64_ROUND_CLOSEST(num, state->period);

. Round-closest is usually wrong in an .apply() callback. I didn't do
the detailed math, but I think you need to round up here.

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)