Message-ID: <CAHh=Yk-TosOmwEughfK9mMx-=DgzWK5H_bf6H641SGh1ue8BrA@mail.gmail.com>
Date: Sun, 19 Jan 2025 15:03:16 +0800
From: Nylon Chen <nylon.chen@...ive.com>
To: Uwe Kleine-König <u.kleine-koenig@...libre.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

Nylon Chen <nylon.chen@...ive.com> 於 2025年1月6日 週一 下午5:09寫道：
>
> Uwe Kleine-König <u.kleine-koenig@...libre.com> 於 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.

>From what I understand, your main concern is to ensure we never end up
with a duty cycle that’s smaller than what the user requested, which a
round-up approach would help guarantee. If you still recommend making
that change to achieve the desired behavior, I’m happy to update the
code accordingly(CLOSEST->UP).

Thanks again for your feedback.

> I will conduct relevant experiments to clarify this issue.
>
> Thanks again.
> >
> > Best regards
> > Uwe

Powered by blists - more mailing lists