Nylon Chen <nylon.chen@xxxxxxxxxx> 於 2025年1月6日 週一 下午5:09寫道： > > Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxx> 於 2024年12月27日 週五 下午4:20寫道： > > > > 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. > > > Hi Uwe, > > Please give us some time to clarify these questions, thank you. > > 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. Hi Uwe, about this part. I ran some basic tests by changing the period and duty cycle in both decreasing and increasing sequences (see the script below). # Backward testing for period (decreasing) echo "Testing period backward..." seq 15000 -1 5000 | while read p; do echo $p > /sys/class/pwm/pwmchip1/pwm0/period echo "Testing period: $p" done # Forward testing for period (increasing) echo "Testing period forward..." seq 5000 1 15000 | while read p; do echo $p > /sys/class/pwm/pwmchip1/pwm0/period echo "Testing period: $p" done # Backward testing for duty cycle (decreasing) echo "Testing duty cycle backward..." for duty in $(seq 10000 -1 0); do echo $duty > /sys/class/pwm/pwmchip1/pwm0/duty_cycle echo "Testing duty cycle: $duty" done # Forward testing for duty cycle (increasing) echo "Testing duty cycle forward..." for duty in $(seq 0 1 10000); do echo $duty > /sys/class/pwm/pwmchip1/pwm0/duty_cycle echo "Testing duty cycle: $duty" done In these particular tests, I didn’t see any functional difference or unexpected behavior whether I used DIV64_U64_ROUND_CLOSEST() or DIV64_U64_ROUND_UP. Of course, there’s a chance my tests haven’t covered every scenario, so there could still be edge cases I missed.
