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.

So this patchset removes active-low in DTS and adds reverse logic to the driver.

Updated patches: 1
New patches: 0
Unchanged patches: 2

Links:
- [0]: https://sifive.cdn.prismic.io/sifive/c52a8e32-05ce-4aaf-95c8-7bf8453f8698_hifive-unleashed-a00-schematics-1.pdf
- [1]: https://sifive.cdn.prismic.io/sifive/6a06d6c0-6e66-49b5-8e9e-e68ce76f4192_hifive-unmatched-schematics-v3.pdf
- [2]: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf

Changed in v10:
 - Add 'inactive' variable in apply() to match pwm_sifive_get_state() style
 - Update comment about hardware limitation - it cannot generate 0% duty
   cycle rather than 100% duty cycle

Changed in v9:
 - Fix commit message to adhere to 75 columns rule.
 - Update commit message's subject.
 - Add a variable for inactive logic.

Changed in v8:
 - Fix Signed-off-by and Co-developed-by typo.

Changed in v7:
 - Remove active-low strings from hifive-unleashed-a00.dts file.

Changed in v6:
 - Separate the idempotent test bug fixes into a new patch.
 - Move the reversing the duty before the line checking
   state->enabled.
 - Fix the algorithm and change it to take the minimum value first and
   then reverse it.

Changed in v5:
 - Add the updates to the PWM algorithm based on version 2 back in.
 - Replace div64_ul with DIV_ROUND_UP_ULL to correct the error in the
   period value of the idempotent test in pwm_apply_state_debug.

Changed in v4:
 - Remove previous updates to the PWM algorithm.

Changed in v3:
 - Convert the reference link to standard link.
 - Move the inverted function before taking the minimum value.
 - Change polarity check condition(high and low).
 - Pick the biggest period length possible that is not bigger than the
   requested period.

Changed in v2:
 - Convert the reference link to standard link.
 - Fix typo: s/sifive unmatched:/sifive: unmatched:/.
 - Remove active-low from hifive-unleashed-a00.dts.
 - Include this reference link in the dts and pwm commit messages.

Nylon Chen (2):
  pwm: sifive: change the PWM algorithm
  pwm: sifive: Fix the error in the idempotent test within the
    pwm_apply_state_debug function

NylonChen (1):
  riscv: dts: sifive: unleashed/unmatched: Remove PWM controlled LED's
    active-low properties

 .../boot/dts/sifive/hifive-unleashed-a00.dts   | 12 ++++--------
 .../boot/dts/sifive/hifive-unmatched-a00.dts   | 12 ++++--------
 drivers/pwm/pwm-sifive.c                       | 18 ++++++++++--------
 3 files changed, 18 insertions(+), 24 deletions(-)

-- 
2.34.1

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

Uwe Kleine-König <u.kleine-koenig@baylibre.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, I've contacted the spec's author, and he is willing to correct
the spec-related error.

Based on your suggestions, I think we have two approaches
1. First add comments explaining where the spec and implementation
don't match, then after the spec is corrected, submit another patch to
remove the comments
2. No need to add this error explanation part, because the spec will
be corrected later.

I don't have a preference, so I wanted to check with you - do you lean
more toward option 1 or option 2
> 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

On Fri, Mar 28, 2025 at 05:41:37PM +0800, Nylon Chen wrote:
>  * (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, I've contacted the spec's author, and he is willing to correct
> the spec-related error.
> 
> Based on your suggestions, I think we have two approaches
> 1. First add comments explaining where the spec and implementation
> don't match, then after the spec is corrected, submit another patch to
> remove the comments
> 2. No need to add this error explanation part, because the spec will
> be corrected later.
> 
> I don't have a preference, so I wanted to check with you - do you lean
> more toward option 1 or option 2

I would go for 1, mentioning the version of the broken documenatation
and the expectation that this will be fixed in later revisions. So there
is no confusion when the documenatation is fixed but the comments not
removed yet.

Best regards
Uwe

Uwe Kleine-König <u.kleine-koenig@baylibre.com> 於 2025年3月28日 週五 下午6:27寫道：
>
> On Fri, Mar 28, 2025 at 05:41:37PM +0800, Nylon Chen wrote:
> >  * (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, I've contacted the spec's author, and he is willing to correct
> > the spec-related error.
> >
> > Based on your suggestions, I think we have two approaches
> > 1. First add comments explaining where the spec and implementation
> > don't match, then after the spec is corrected, submit another patch to
> > remove the comments
> > 2. No need to add this error explanation part, because the spec will
> > be corrected later.
> >
> > I don't have a preference, so I wanted to check with you - do you lean
> > more toward option 1 or option 2
>
> I would go for 1, mentioning the version of the broken documenatation
> and the expectation that this will be fixed in later revisions. So there
> is no confusion when the documenatation is fixed but the comments not
> removed yet.
Hi Uwe, thanks for the feedback

I understand. I'll proceed with option one to handle the patch for the
next version.
>
> Best regards
> Uwe

Uwe Kleine-König <u.kleine-koenig@baylibre.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.
I will conduct relevant experiments to clarify this issue.

Thanks again.
>
> Best regards
> Uwe

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

Hello,

On Sun, Jan 19, 2025 at 03:03:16PM +0800, Nylon Chen wrote:
> I ran some basic tests by changing the period and duty cycle in both
> decreasing and increasing sequences (see the script below).

What is clk_get_rate(ddata->clk) for you?

> # 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.

Just to be sure: You have PWM_DEBUG enabled?

> 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).

No, .apply should round down and so to ensure that

	pwm_get_state(mypwm, &state);
	pwm_apply(mypwm, &state);

doesn't modify the hardware setting, .get_state has to round up.

Best regards
Uwe

Uwe Kleine-König <u.kleine-koenig@baylibre.com> 於 2025年1月21日 週二 下午3:47寫道：
>
> Hello,
>
> On Sun, Jan 19, 2025 at 03:03:16PM +0800, Nylon Chen wrote:
> > I ran some basic tests by changing the period and duty cycle in both
> > decreasing and increasing sequences (see the script below).
>
> What is clk_get_rate(ddata->clk) for you?
130 MHz
>
> > # 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.
>
> Just to be sure: You have PWM_DEBUG enabled?
Yes, to verify my patch, I always enable it by default.

>
> > 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).
>
> No, .apply should round down and so to ensure that
>
>         pwm_get_state(mypwm, &state);
>         pwm_apply(mypwm, &state);
>
> doesn't modify the hardware setting, .get_state has to round up.
I have some questions that I'd like to further confirm, to ensure I
understand correctly and can adjust the patch accordingly

your earlier suggestions were as follows:
"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."

The more recent suggestions are as follows:
"No, .apply should round down and so to ensure that....."

I speculate that the latter suggestion is to ensure idempotency
between .apply and .get_state, avoiding unnecessary hardware setting
modifications during repeated reading and applying processes. However,
the earlier suggestions seem to conflict with this.

If needed, I can provide more test results or make further adjustments.
Thank you again for your patient guidance.
>
> Best regards
> Uwe

On Tue, Jan 21, 2025 at 04:47:46PM +0800, Nylon Chen wrote:
> Uwe Kleine-König <u.kleine-koenig@baylibre.com> 於 2025年1月21日 週二 下午3:47寫道：
> >
> > Hello,
> >
> > On Sun, Jan 19, 2025 at 03:03:16PM +0800, Nylon Chen wrote:
> > > I ran some basic tests by changing the period and duty cycle in both
> > > decreasing and increasing sequences (see the script below).
> >
> > What is clk_get_rate(ddata->clk) for you?
> 130 MHz

OK, so the possible period lengths are

	(1 << (16 + scale)) / (130 MHz)

for scale in [0, .. 15], right? That's

	2^scale * 504123.07692307694 ns

So testing period in the range between 5000 ns and 15000 ns isn't
sensible? Did I get something wrong? If the above is right, switching
between period=1008246 ns and 1008247 ns is likely to trigger a
warning.

Maybe also something like

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 7e863c2cd44b..6c82aca84472 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -2247,9 +2247,10 @@ static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
 
 	for (i = 0; i < chip->npwm; i++) {
 		struct pwm_device *pwm = &chip->pwms[i];
-		struct pwm_state state;
+		struct pwm_state state, hwstate;
 
 		pwm_get_state(pwm, &state);
+		pwm_get_state_hw(pwm, &hwstate);
 
 		seq_printf(s, " pwm-%-3d (%-20.20s):", i, pwm->label);
 
@@ -2259,8 +2260,8 @@ static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
 		if (state.enabled)
 			seq_puts(s, " enabled");
 
-		seq_printf(s, " period: %llu ns", state.period);
-		seq_printf(s, " duty: %llu ns", state.duty_cycle);
+		seq_printf(s, " period: %llu (%llu) ns", state.period, hwstate.period);
+		seq_printf(s, " duty: %llu (%llu) ns", state.duty_cycle, hwstate.duty_cycle);
 		seq_printf(s, " polarity: %s",
 			   state.polarity ? "inverse" : "normal");
 

is useful for debugging to see what is actually implemented for a given
request.

Having said that, I don't like struct pwm_sifive_ddata::real_period and
pwm_sifive_ddata::approx_period. These complicate the calculation and
.get_state should better calculate the period instead of just sticking
to ddata->real_period.

Best regards
Uwe

Hello Nylon,

I took another look in the driver and found another problem:

On Tue, Jan 21, 2025 at 07:12:10PM +0100, Uwe Kleine-König wrote:
> On Tue, Jan 21, 2025 at 04:47:46PM +0800, Nylon Chen wrote:
> > Uwe Kleine-König <u.kleine-koenig@baylibre.com> 於 2025年1月21日 週二 下午3:47寫道：
> > >
> > > Hello,
> > >
> > > On Sun, Jan 19, 2025 at 03:03:16PM +0800, Nylon Chen wrote:
> > > > I ran some basic tests by changing the period and duty cycle in both
> > > > decreasing and increasing sequences (see the script below).
> > >
> > > What is clk_get_rate(ddata->clk) for you?
> > 130 MHz
> 
> OK, so the possible period lengths are
> 
> 	(1 << (16 + scale)) / (130 MHz)
> 
> for scale in [0, .. 15], right? That's
> 
> 	2^scale * 504123.07692307694 ns
> 
> So testing period in the range between 5000 ns and 15000 ns isn't
> sensible? Did I get something wrong? If the above is right, switching
> between period=1008246 ns and 1008247 ns is likely to trigger a
> warning.

The possible periods are of the form

	2^scale * A

where A is constant and only depends on the input clock rate. scale
ranges over [0, ... 15]. (If I got it right in my last mail, we have A =
504123.07692307694 ns.)

If you request say:

	.period = 3.9 * A
	.duty_cycle = 1.9 * A

the period actually emitted by the PWM will be 2 * A. But to calculate
frac the originally requested period (i.e. 3.9 * A) is used. So frac
becomes 31927 resulting in .duty_cycle being ~0.974 A. A better value
would be frac = 62259 which results in .duty_cycle ≅ 1.9 * A.
(Depending on A the values for frac might be off by one due to rounding
differences.)

So I would expect that PWM_DEBUG is angry with you if you go from

	.period = 2 * A
	.duty_cycle = 1.9 * A

to

	.period = 3.9 * A
	.duty_cycle = 1.9 * A

.

Best regards
Uwe

Uwe Kleine-König <u.kleine-koenig@baylibre.com> 於 2025年1月22日 週三 下午7:44寫道：
>
> Hello Nylon,
>
> I took another look in the driver and found another problem:
Hi Uwe, Thank you for the information.

I'll need some time to verify and understand these details, as well as
conduct the relevant tests
>
> On Tue, Jan 21, 2025 at 07:12:10PM +0100, Uwe Kleine-König wrote:
> > On Tue, Jan 21, 2025 at 04:47:46PM +0800, Nylon Chen wrote:
> > > Uwe Kleine-König <u.kleine-koenig@baylibre.com> 於 2025年1月21日 週二 下午3:47寫道：
> > > >
> > > > Hello,
> > > >
> > > > On Sun, Jan 19, 2025 at 03:03:16PM +0800, Nylon Chen wrote:
> > > > > I ran some basic tests by changing the period and duty cycle in both
> > > > > decreasing and increasing sequences (see the script below).
> > > >
> > > > What is clk_get_rate(ddata->clk) for you?
> > > 130 MHz
> >
> > OK, so the possible period lengths are
> >
> >       (1 << (16 + scale)) / (130 MHz)
> >
> > for scale in [0, .. 15], right? That's
> >
> >       2^scale * 504123.07692307694 ns
> >
> > So testing period in the range between 5000 ns and 15000 ns isn't
> > sensible? Did I get something wrong? If the above is right, switching
> > between period=1008246 ns and 1008247 ns is likely to trigger a
> > warning.
>
> The possible periods are of the form
>
>         2^scale * A
>
> where A is constant and only depends on the input clock rate. scale
> ranges over [0, ... 15]. (If I got it right in my last mail, we have A =
> 504123.07692307694 ns.)
>
> If you request say:
>
>         .period = 3.9 * A
>         .duty_cycle = 1.9 * A
>
> the period actually emitted by the PWM will be 2 * A. But to calculate
> frac the originally requested period (i.e. 3.9 * A) is used. So frac
> becomes 31927 resulting in .duty_cycle being ~0.974 A. A better value
> would be frac = 62259 which results in .duty_cycle ≅ 1.9 * A.
> (Depending on A the values for frac might be off by one due to rounding
> differences.)
>
> So I would expect that PWM_DEBUG is angry with you if you go from
>
>         .period = 2 * A
>         .duty_cycle = 1.9 * A
>
> to
>
>         .period = 3.9 * A
>         .duty_cycle = 1.9 * A
>
> .
>
> Best regards
> Uwe

Nylon Chen <nylon.chen@sifive.com> 於 2025年1月23日 週四 上午8:20寫道：
>
> Uwe Kleine-König <u.kleine-koenig@baylibre.com> 於 2025年1月22日 週三 下午7:44寫道：
> >
> > Hello Nylon,
> >
> > I took another look in the driver and found another problem:
> Hi Uwe, Thank you for the information.
>
> I'll need some time to verify and understand these details, as well as
> conduct the relevant tests
> >
> > On Tue, Jan 21, 2025 at 07:12:10PM +0100, Uwe Kleine-König wrote:
> > > On Tue, Jan 21, 2025 at 04:47:46PM +0800, Nylon Chen wrote:
> > > > Uwe Kleine-König <u.kleine-koenig@baylibre.com> 於 2025年1月21日 週二 下午3:47寫道：
> > > > >
> > > > > Hello,
> > > > >
> > > > > On Sun, Jan 19, 2025 at 03:03:16PM +0800, Nylon Chen wrote:
> > > > > > I ran some basic tests by changing the period and duty cycle in both
> > > > > > decreasing and increasing sequences (see the script below).
> > > > >
> > > > > What is clk_get_rate(ddata->clk) for you?
> > > > 130 MHz
> > >
> > > OK, so the possible period lengths are
> > >
> > >       (1 << (16 + scale)) / (130 MHz)
> > >
> > > for scale in [0, .. 15], right? That's
> > >
> > >       2^scale * 504123.07692307694 ns
> > >
> > > So testing period in the range between 5000 ns and 15000 ns isn't
> > > sensible? Did I get something wrong? If the above is right, switching
> > > between period=1008246 ns and 1008247 ns is likely to trigger a
> > > warning.
> >
> > The possible periods are of the form
> >
> >         2^scale * A
> >
> > where A is constant and only depends on the input clock rate. scale
> > ranges over [0, ... 15]. (If I got it right in my last mail, we have A =
> > 504123.07692307694 ns.)
> >
> > If you request say:
> >
> >         .period = 3.9 * A
> >         .duty_cycle = 1.9 * A
> >
> > the period actually emitted by the PWM will be 2 * A. But to calculate
> > frac the originally requested period (i.e. 3.9 * A) is used. So frac
> > becomes 31927 resulting in .duty_cycle being ~0.974 A. A better value
> > would be frac = 62259 which results in .duty_cycle ≅ 1.9 * A.
> > (Depending on A the values for frac might be off by one due to rounding
> > differences.)
> >
> > So I would expect that PWM_DEBUG is angry with you if you go from
> >
> >         .period = 2 * A
> >         .duty_cycle = 1.9 * A
> >
> > to
> >
> >         .period = 3.9 * A
> >         .duty_cycle = 1.9 * A
> >
> > .
> >
> > Best regards
> > Uwe

Hi Uwe, Based on your suggestions, I conducted relevant tests and
corrected algorithm errors.

According to this test program, I can make the system generate related
error messages on v10's patchset.

e.g.
[ 75.043652] pwm-sifive 10021000.pwm: .apply is supposed to round down
duty_cycle (requested: 360/504000, applied: 361/504124)
[ 75.055042] pwm-sifive 10021000.pwm: .apply is supposed to round down
period (requested: 504000, applied: 504124)

PWMCHIP=1
PWMCHANNEL=0
PERIOD=504000
STEP=1
MAX_DUTY=504000

echo 0 > /sys/class/pwm/pwmchip${PWMCHIP}/export

echo $PERIOD > /sys/class/pwm/pwmchip${PWMCHIP}/pwm${PWMCHANNEL}/period
echo "Set period to $PERIOD ns (scale=0 region)"

COUNT=$((MAX_DUTY / STEP))
echo "Testing duty-cycle from 0 to $MAX_DUTY in step of $STEP..."
echo "Total steps (forward): $((COUNT+1))"


CURRENT=0
while [ $CURRENT -le $MAX_DUTY ]; do
    echo $CURRENT > /sys/class/pwm/pwmchip${PWMCHIP}/pwm${PWMCHANNEL}/duty_cycle
    CURRENT=$((CURRENT + STEP))
done

echo "Now do a backward test from $MAX_DUTY down to 0 in step of $STEP..."
echo "Total steps (backward): $((COUNT+1))"


CURRENT=$MAX_DUTY
while [ $CURRENT -ge 0 ]; do
    echo $CURRENT > /sys/class/pwm/pwmchip${PWMCHIP}/pwm${PWMCHANNEL}/duty_cycle
    CURRENT=$((CURRENT - STEP))
done


echo 0 > /sys/class/pwm/pwmchip${PWMCHIP}/pwm${PWMCHANNEL}/enable
echo ${PWMCHANNEL} > /sys/class/pwm/pwmchip${PWMCHIP}/unexport

echo "Done!"

Based on your previous suggestions, I have made the following related
modifications, and now I'm able to fix the relevant errors.

But I want to make sure my understanding aligns with your suggestions,
so I'd like to discuss with you first before sending the patch.

- In .apply, use "round down" for calculations to ensure the value
doesn't exceed what the user requested. (Never set a duty cycle higher
than what the user requested.)
pwm_sifive_apply() {
    - frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
    +frac = num / state->period;
}
- When converting hardware values back to duty_cycle in .get_state,
use "round up" to compensate for the errors introduced by .apply.()
pwm_sifive_get_state() {
    - state->duty_cycle = (u64)duty * ddata->real_period >> PWM_SIFIVE_CMPWIDTH;
    + state->duty_cycle = DIV_ROUND_UP_ULL((u64)duty *
ddata->real_period, (1U << PWM_SIFIVE_CMPWIDTH));
}

Hello Nylon,

On Tue, Mar 04, 2025 at 05:02:13PM +0800, Nylon Chen wrote:
> Nylon Chen <nylon.chen@sifive.com> 於 2025年1月23日 週四 上午8:20寫道：
> >
> > Uwe Kleine-König <u.kleine-koenig@baylibre.com> 於 2025年1月22日 週三 下午7:44寫道：
> > >
> > > Hello Nylon,
> > >
> > > I took another look in the driver and found another problem:
> > Hi Uwe, Thank you for the information.
> >
> > I'll need some time to verify and understand these details, as well as
> > conduct the relevant tests
> > >
> > > On Tue, Jan 21, 2025 at 07:12:10PM +0100, Uwe Kleine-König wrote:
> > > > On Tue, Jan 21, 2025 at 04:47:46PM +0800, Nylon Chen wrote:
> > > > > Uwe Kleine-König <u.kleine-koenig@baylibre.com> 於 2025年1月21日 週二 下午3:47寫道：
> > > > > >
> > > > > > Hello,
> > > > > >
> > > > > > On Sun, Jan 19, 2025 at 03:03:16PM +0800, Nylon Chen wrote:
> > > > > > > I ran some basic tests by changing the period and duty cycle in both
> > > > > > > decreasing and increasing sequences (see the script below).
> > > > > >
> > > > > > What is clk_get_rate(ddata->clk) for you?
> > > > > 130 MHz
> > > >
> > > > OK, so the possible period lengths are
> > > >
> > > >       (1 << (16 + scale)) / (130 MHz)
> > > >
> > > > for scale in [0, .. 15], right? That's
> > > >
> > > >       2^scale * 504123.07692307694 ns
> > > >
> > > > So testing period in the range between 5000 ns and 15000 ns isn't
> > > > sensible? Did I get something wrong? If the above is right, switching
> > > > between period=1008246 ns and 1008247 ns is likely to trigger a
> > > > warning.
> > >
> > > The possible periods are of the form
> > >
> > >         2^scale * A
> > >
> > > where A is constant and only depends on the input clock rate. scale
> > > ranges over [0, ... 15]. (If I got it right in my last mail, we have A =
> > > 504123.07692307694 ns.)
> > >
> > > If you request say:
> > >
> > >         .period = 3.9 * A
> > >         .duty_cycle = 1.9 * A
> > >
> > > the period actually emitted by the PWM will be 2 * A. But to calculate
> > > frac the originally requested period (i.e. 3.9 * A) is used. So frac
> > > becomes 31927 resulting in .duty_cycle being ~0.974 A. A better value
> > > would be frac = 62259 which results in .duty_cycle ≅ 1.9 * A.
> > > (Depending on A the values for frac might be off by one due to rounding
> > > differences.)
> > >
> > > So I would expect that PWM_DEBUG is angry with you if you go from
> > >
> > >         .period = 2 * A
> > >         .duty_cycle = 1.9 * A
> > >
> > > to
> > >
> > >         .period = 3.9 * A
> > >         .duty_cycle = 1.9 * A
> > >
> > > .
> > >
> > > Best regards
> > > Uwe
> 
> Hi Uwe, Based on your suggestions, I conducted relevant tests and
> corrected algorithm errors.
> 
> According to this test program, I can make the system generate related
> error messages on v10's patchset.
> 
> e.g.
> [ 75.043652] pwm-sifive 10021000.pwm: .apply is supposed to round down
> duty_cycle (requested: 360/504000, applied: 361/504124)
> [ 75.055042] pwm-sifive 10021000.pwm: .apply is supposed to round down
> period (requested: 504000, applied: 504124)
> 
> PWMCHIP=1
> PWMCHANNEL=0
> PERIOD=504000
> STEP=1
> MAX_DUTY=504000
> 
> echo 0 > /sys/class/pwm/pwmchip${PWMCHIP}/export
> 
> echo $PERIOD > /sys/class/pwm/pwmchip${PWMCHIP}/pwm${PWMCHANNEL}/period
> echo "Set period to $PERIOD ns (scale=0 region)"
> 
> COUNT=$((MAX_DUTY / STEP))
> echo "Testing duty-cycle from 0 to $MAX_DUTY in step of $STEP..."
> echo "Total steps (forward): $((COUNT+1))"
> 
> 
> CURRENT=0
> while [ $CURRENT -le $MAX_DUTY ]; do
>     echo $CURRENT > /sys/class/pwm/pwmchip${PWMCHIP}/pwm${PWMCHANNEL}/duty_cycle
>     CURRENT=$((CURRENT + STEP))
> done
> 
> echo "Now do a backward test from $MAX_DUTY down to 0 in step of $STEP..."
> echo "Total steps (backward): $((COUNT+1))"
> 
> 
> CURRENT=$MAX_DUTY
> while [ $CURRENT -ge 0 ]; do
>     echo $CURRENT > /sys/class/pwm/pwmchip${PWMCHIP}/pwm${PWMCHANNEL}/duty_cycle
>     CURRENT=$((CURRENT - STEP))
> done
> 
> 
> echo 0 > /sys/class/pwm/pwmchip${PWMCHIP}/pwm${PWMCHANNEL}/enable
> echo ${PWMCHANNEL} > /sys/class/pwm/pwmchip${PWMCHIP}/unexport
> 
> echo "Done!"
> 
> Based on your previous suggestions, I have made the following related
> modifications, and now I'm able to fix the relevant errors.
> 
> But I want to make sure my understanding aligns with your suggestions,
> so I'd like to discuss with you first before sending the patch.
> 
> - In .apply, use "round down" for calculations to ensure the value
> doesn't exceed what the user requested. (Never set a duty cycle higher
> than what the user requested.)
> pwm_sifive_apply() {
>     - frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
>     +frac = num / state->period;
> }
> - When converting hardware values back to duty_cycle in .get_state,
> use "round up" to compensate for the errors introduced by .apply.()
> pwm_sifive_get_state() {
>     - state->duty_cycle = (u64)duty * ddata->real_period >> PWM_SIFIVE_CMPWIDTH;
>     + state->duty_cycle = DIV_ROUND_UP_ULL((u64)duty *
> ddata->real_period, (1U << PWM_SIFIVE_CMPWIDTH));
> }

That sounds right.

Best regards
Uwe