[v5 0/2] Change PWM-controlled LED pin active mode and algorithm

[v5 0/2] Change PWM-controlled LED pin active mode and algorithm

[Nylon Chen]

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: 1
Unchanged patches: 1

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 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):
  riscv: dts: sifive: unleashed/unmatched: Remove PWM controlled LED's
    active-low properties
  pwm: sifive: change the PWM controlled LED algorithm

 arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts |  8 ++++----
 arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts | 12 ++++--------
 drivers/pwm/pwm-sifive.c                            | 10 ++++++----
 3 files changed, 14 insertions(+), 16 deletions(-)

-- 
2.42.0

[v5 1/2] riscv: dts: sifive: unleashed/unmatched: Remove PWM controlled LED's active-low properties

[Nylon Chen]

This removes the active-low properties of the PWM-controlled LEDs in
the HiFive Unmatched device tree.

The reference is hifive-unleashed-a00.pdf[0] and hifive-unmatched-schematics-v3.pdf[1].

Link: https://sifive.cdn.prismic.io/sifive/c52a8e32-05ce-4aaf-95c8-7bf8453f8698_hifive-unleashed-a00-schematics-1.pdf [0]
Link: https://sifive.cdn.prismic.io/sifive/6a06d6c0-6e66-49b5-8e9e-e68ce76f4192_hifive-unmatched-schematics-v3.pdf [1]

Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
Signed-off-by: Nylon Chen <nylon.chen@sifive.com>
---
 arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts |  8 ++++----
 arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts | 12 ++++--------
 2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
index 900a50526d77..11e7ac1c54bb 100644
--- a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
+++ b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
@@ -49,7 +49,7 @@ led-controller {
 		compatible = "pwm-leds";
 
 		led-d1 {
-			pwms = <&pwm0 0 7812500 PWM_POLARITY_INVERTED>;
+			pwms = <&pwm0 0 7812500 0>;
 			active-low;
 			color = <LED_COLOR_ID_GREEN>;
 			max-brightness = <255>;
@@ -57,7 +57,7 @@ led-d1 {
 		};
 
 		led-d2 {
-			pwms = <&pwm0 1 7812500 PWM_POLARITY_INVERTED>;
+			pwms = <&pwm0 1 7812500 0>;
 			active-low;
 			color = <LED_COLOR_ID_GREEN>;
 			max-brightness = <255>;
@@ -65,7 +65,7 @@ led-d2 {
 		};
 
 		led-d3 {
-			pwms = <&pwm0 2 7812500 PWM_POLARITY_INVERTED>;
+			pwms = <&pwm0 2 7812500 0>;
 			active-low;
 			color = <LED_COLOR_ID_GREEN>;
 			max-brightness = <255>;
@@ -73,7 +73,7 @@ led-d3 {
 		};
 
 		led-d4 {
-			pwms = <&pwm0 3 7812500 PWM_POLARITY_INVERTED>;
+			pwms = <&pwm0 3 7812500 0>;
 			active-low;
 			color = <LED_COLOR_ID_GREEN>;
 			max-brightness = <255>;
diff --git a/arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts b/arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts
index 07387f9c135c..b328ee80693f 100644
--- a/arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts
+++ b/arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts
@@ -51,8 +51,7 @@ led-controller-1 {
 		compatible = "pwm-leds";
 
 		led-d12 {
-			pwms = <&pwm0 0 7812500 PWM_POLARITY_INVERTED>;
-			active-low;
+			pwms = <&pwm0 0 7812500 0>;
 			color = <LED_COLOR_ID_GREEN>;
 			max-brightness = <255>;
 			label = "d12";
@@ -68,20 +67,17 @@ multi-led {
 			label = "d2";
 
 			led-red {
-				pwms = <&pwm0 2 7812500 PWM_POLARITY_INVERTED>;
-				active-low;
+				pwms = <&pwm0 2 7812500 0>;
 				color = <LED_COLOR_ID_RED>;
 			};
 
 			led-green {
-				pwms = <&pwm0 1 7812500 PWM_POLARITY_INVERTED>;
-				active-low;
+				pwms = <&pwm0 1 7812500 0>;
 				color = <LED_COLOR_ID_GREEN>;
 			};
 
 			led-blue {
-				pwms = <&pwm0 3 7812500 PWM_POLARITY_INVERTED>;
-				active-low;
+				pwms = <&pwm0 3 7812500 0>;
 				color = <LED_COLOR_ID_BLUE>;
 			};
 		};
-- 
2.42.0

[v5 2/2] pwm: sifive: change the PWM controlled LED algorithm

[Nylon Chen]

The `frac` variable represents the pulse inactive time, and the result
of this algorithm is the pulse active time. Therefore, we must reverse the result.

The reference is SiFive FU740-C000 Manual[0]

Link: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf [0]

Signed-off-by: Nylon Chen <nylon.chen@sifive.com>
Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
---
 drivers/pwm/pwm-sifive.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
index eabddb7c7820..353c2342fbf1 100644
--- a/drivers/pwm/pwm-sifive.c
+++ b/drivers/pwm/pwm-sifive.c
@@ -101,7 +101,7 @@ static void pwm_sifive_update_clock(struct pwm_sifive_ddata *ddata,
 
 	/* As scale <= 15 the shift operation cannot overflow. */
 	num = (unsigned long long)NSEC_PER_SEC << (PWM_SIFIVE_CMPWIDTH + scale);
-	ddata->real_period = div64_ul(num, rate);
+	ddata->real_period = DIV_ROUND_UP_ULL(num, rate);
 	dev_dbg(ddata->chip.dev,
 		"New real_period = %u ns\n", ddata->real_period);
 }
@@ -121,13 +121,14 @@ static int pwm_sifive_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 		state->enabled = false;
 
 	state->period = ddata->real_period;
+
+	duty = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - duty;
 	state->duty_cycle =
 		(u64)duty * ddata->real_period >> PWM_SIFIVE_CMPWIDTH;
-	state->polarity = PWM_POLARITY_INVERSED;
+	state->polarity = PWM_POLARITY_NORMAL;
 
 	return 0;
 }
-
 static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			    const struct pwm_state *state)
 {
@@ -139,7 +140,7 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	int ret = 0;
 	u32 frac;
 
-	if (state->polarity != PWM_POLARITY_INVERSED)
+	if (state->polarity != PWM_POLARITY_NORMAL)
 		return -EINVAL;
 
 	cur_state = pwm->state;
@@ -158,6 +159,7 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH);
 	frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
 	/* The hardware cannot generate a 100% duty cycle */
+	frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac;
 	frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
 
 	mutex_lock(&ddata->lock);
-- 
2.42.0

Re: [v5 1/2] riscv: dts: sifive: unleashed/unmatched: Remove PWM controlled LED's active-low properties

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3756 bytes --]

Hey,

On Tue, Oct 24, 2023 at 06:19:01PM +0800, Nylon Chen wrote:
> This removes the active-low properties of the PWM-controlled LEDs in
> the HiFive Unmatched device tree.
> 
> The reference is hifive-unleashed-a00.pdf[0] and hifive-unmatched-schematics-v3.pdf[1].
> 
> Link: https://sifive.cdn.prismic.io/sifive/c52a8e32-05ce-4aaf-95c8-7bf8453f8698_hifive-unleashed-a00-schematics-1.pdf [0]
> Link: https://sifive.cdn.prismic.io/sifive/6a06d6c0-6e66-49b5-8e9e-e68ce76f4192_hifive-unmatched-schematics-v3.pdf [1]

> 

This blank line should be removed if there is a follow-up.

> Signed-off-by: Vincent Chen <vincent.chen@sifive.com>

What did Vincent contribute to this patch? Are you missing a
co-developed-by tag, perhaps?

> Signed-off-by: Nylon Chen <nylon.chen@sifive.com>

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Acked-by: Conor Dooley <conor.dooley@microchip.com>

I expect this to go via the pwm tree since this is going to "break" (in
the loosest possible sense) existing systems if merged separately.

Cheers,
Conor.

> ---
>  arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts |  8 ++++----
>  arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts | 12 ++++--------
>  2 files changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> index 900a50526d77..11e7ac1c54bb 100644
> --- a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> +++ b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> @@ -49,7 +49,7 @@ led-controller {
>  		compatible = "pwm-leds";
>  
>  		led-d1 {
> -			pwms = <&pwm0 0 7812500 PWM_POLARITY_INVERTED>;
> +			pwms = <&pwm0 0 7812500 0>;
>  			active-low;
>  			color = <LED_COLOR_ID_GREEN>;
>  			max-brightness = <255>;
> @@ -57,7 +57,7 @@ led-d1 {
>  		};
>  
>  		led-d2 {
> -			pwms = <&pwm0 1 7812500 PWM_POLARITY_INVERTED>;
> +			pwms = <&pwm0 1 7812500 0>;
>  			active-low;
>  			color = <LED_COLOR_ID_GREEN>;
>  			max-brightness = <255>;
> @@ -65,7 +65,7 @@ led-d2 {
>  		};
>  
>  		led-d3 {
> -			pwms = <&pwm0 2 7812500 PWM_POLARITY_INVERTED>;
> +			pwms = <&pwm0 2 7812500 0>;
>  			active-low;
>  			color = <LED_COLOR_ID_GREEN>;
>  			max-brightness = <255>;
> @@ -73,7 +73,7 @@ led-d3 {
>  		};
>  
>  		led-d4 {
> -			pwms = <&pwm0 3 7812500 PWM_POLARITY_INVERTED>;
> +			pwms = <&pwm0 3 7812500 0>;
>  			active-low;
>  			color = <LED_COLOR_ID_GREEN>;
>  			max-brightness = <255>;
> diff --git a/arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts b/arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts
> index 07387f9c135c..b328ee80693f 100644
> --- a/arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts
> +++ b/arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts
> @@ -51,8 +51,7 @@ led-controller-1 {
>  		compatible = "pwm-leds";
>  
>  		led-d12 {
> -			pwms = <&pwm0 0 7812500 PWM_POLARITY_INVERTED>;
> -			active-low;
> +			pwms = <&pwm0 0 7812500 0>;
>  			color = <LED_COLOR_ID_GREEN>;
>  			max-brightness = <255>;
>  			label = "d12";
> @@ -68,20 +67,17 @@ multi-led {
>  			label = "d2";
>  
>  			led-red {
> -				pwms = <&pwm0 2 7812500 PWM_POLARITY_INVERTED>;
> -				active-low;
> +				pwms = <&pwm0 2 7812500 0>;
>  				color = <LED_COLOR_ID_RED>;
>  			};
>  
>  			led-green {
> -				pwms = <&pwm0 1 7812500 PWM_POLARITY_INVERTED>;
> -				active-low;
> +				pwms = <&pwm0 1 7812500 0>;
>  				color = <LED_COLOR_ID_GREEN>;
>  			};
>  
>  			led-blue {
> -				pwms = <&pwm0 3 7812500 PWM_POLARITY_INVERTED>;
> -				active-low;
> +				pwms = <&pwm0 3 7812500 0>;
>  				color = <LED_COLOR_ID_BLUE>;
>  			};
>  		};
> -- 
> 2.42.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [v5 1/2] riscv: dts: sifive: unleashed/unmatched: Remove PWM controlled LED's active-low properties

[Nylon Chen]

Hi Conor,

Conor Dooley <conor@kernel.org> 於 2023年10月24日 週二 下午10:55寫道：
>
> Hey,
>
> On Tue, Oct 24, 2023 at 06:19:01PM +0800, Nylon Chen wrote:
> > This removes the active-low properties of the PWM-controlled LEDs in
> > the HiFive Unmatched device tree.
> >
> > The reference is hifive-unleashed-a00.pdf[0] and hifive-unmatched-schematics-v3.pdf[1].
> >
> > Link: https://sifive.cdn.prismic.io/sifive/c52a8e32-05ce-4aaf-95c8-7bf8453f8698_hifive-unleashed-a00-schematics-1.pdf [0]
> > Link: https://sifive.cdn.prismic.io/sifive/6a06d6c0-6e66-49b5-8e9e-e68ce76f4192_hifive-unmatched-schematics-v3.pdf [1]
>
> >
>
> This blank line should be removed if there is a follow-up.
thanks, I got it.
>
> > Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
>
> What did Vincent contribute to this patch? Are you missing a
> co-developed-by tag, perhaps?
Yes, Vincent was the first person to find the PWM driver problem, and
Zong Li helped me with the relevant review internally.

so in the next version, I will add the correct tags for these two developers.

Thank you again for your time.
>
> > Signed-off-by: Nylon Chen <nylon.chen@sifive.com>
>
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
>
> I expect this to go via the pwm tree since this is going to "break" (in
> the loosest possible sense) existing systems if merged separately.
>
> Cheers,
> Conor.
>
> > ---
> >  arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts |  8 ++++----
> >  arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts | 12 ++++--------
> >  2 files changed, 8 insertions(+), 12 deletions(-)
> >
> > diff --git a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> > index 900a50526d77..11e7ac1c54bb 100644
> > --- a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> > +++ b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> > @@ -49,7 +49,7 @@ led-controller {
> >               compatible = "pwm-leds";
> >
> >               led-d1 {
> > -                     pwms = <&pwm0 0 7812500 PWM_POLARITY_INVERTED>;
> > +                     pwms = <&pwm0 0 7812500 0>;
> >                       active-low;
> >                       color = <LED_COLOR_ID_GREEN>;
> >                       max-brightness = <255>;
> > @@ -57,7 +57,7 @@ led-d1 {
> >               };
> >
> >               led-d2 {
> > -                     pwms = <&pwm0 1 7812500 PWM_POLARITY_INVERTED>;
> > +                     pwms = <&pwm0 1 7812500 0>;
> >                       active-low;
> >                       color = <LED_COLOR_ID_GREEN>;
> >                       max-brightness = <255>;
> > @@ -65,7 +65,7 @@ led-d2 {
> >               };
> >
> >               led-d3 {
> > -                     pwms = <&pwm0 2 7812500 PWM_POLARITY_INVERTED>;
> > +                     pwms = <&pwm0 2 7812500 0>;
> >                       active-low;
> >                       color = <LED_COLOR_ID_GREEN>;
> >                       max-brightness = <255>;
> > @@ -73,7 +73,7 @@ led-d3 {
> >               };
> >
> >               led-d4 {
> > -                     pwms = <&pwm0 3 7812500 PWM_POLARITY_INVERTED>;
> > +                     pwms = <&pwm0 3 7812500 0>;
> >                       active-low;
> >                       color = <LED_COLOR_ID_GREEN>;
> >                       max-brightness = <255>;
> > diff --git a/arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts b/arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts
> > index 07387f9c135c..b328ee80693f 100644
> > --- a/arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts
> > +++ b/arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts
> > @@ -51,8 +51,7 @@ led-controller-1 {
> >               compatible = "pwm-leds";
> >
> >               led-d12 {
> > -                     pwms = <&pwm0 0 7812500 PWM_POLARITY_INVERTED>;
> > -                     active-low;
> > +                     pwms = <&pwm0 0 7812500 0>;
> >                       color = <LED_COLOR_ID_GREEN>;
> >                       max-brightness = <255>;
> >                       label = "d12";
> > @@ -68,20 +67,17 @@ multi-led {
> >                       label = "d2";
> >
> >                       led-red {
> > -                             pwms = <&pwm0 2 7812500 PWM_POLARITY_INVERTED>;
> > -                             active-low;
> > +                             pwms = <&pwm0 2 7812500 0>;
> >                               color = <LED_COLOR_ID_RED>;
> >                       };
> >
> >                       led-green {
> > -                             pwms = <&pwm0 1 7812500 PWM_POLARITY_INVERTED>;
> > -                             active-low;
> > +                             pwms = <&pwm0 1 7812500 0>;
> >                               color = <LED_COLOR_ID_GREEN>;
> >                       };
> >
> >                       led-blue {
> > -                             pwms = <&pwm0 3 7812500 PWM_POLARITY_INVERTED>;
> > -                             active-low;
> > +                             pwms = <&pwm0 3 7812500 0>;
> >                               color = <LED_COLOR_ID_BLUE>;
> >                       };
> >               };
> > --
> > 2.42.0
> >

Re: [v5 1/2] riscv: dts: sifive: unleashed/unmatched: Remove PWM controlled LED's active-low properties

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1150 bytes --]

On Wed, Oct 25, 2023 at 05:32:21PM +0800, Nylon Chen wrote:
> Hi Conor,
> 
> Conor Dooley <conor@kernel.org> 於 2023年10月24日 週二 下午10:55寫道：
> >
> > Hey,
> >
> > On Tue, Oct 24, 2023 at 06:19:01PM +0800, Nylon Chen wrote:
> > > This removes the active-low properties of the PWM-controlled LEDs in
> > > the HiFive Unmatched device tree.
> > >
> > > The reference is hifive-unleashed-a00.pdf[0] and hifive-unmatched-schematics-v3.pdf[1].
> > >
> > > Link: https://sifive.cdn.prismic.io/sifive/c52a8e32-05ce-4aaf-95c8-7bf8453f8698_hifive-unleashed-a00-schematics-1.pdf [0]
> > > Link: https://sifive.cdn.prismic.io/sifive/6a06d6c0-6e66-49b5-8e9e-e68ce76f4192_hifive-unmatched-schematics-v3.pdf [1]
> >
> > >
> >
> > This blank line should be removed if there is a follow-up.
> thanks, I got it.
> >
> > > Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
> >
> > What did Vincent contribute to this patch? Are you missing a
> > co-developed-by tag, perhaps?
> Yes, Vincent was the first person to find the PWM driver problem, and

That sounds like s/Signed-off-by/Reported-by/ then.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [v5 1/2] riscv: dts: sifive: unleashed/unmatched: Remove PWM controlled LED's active-low properties

[Nylon Chen]

Conor Dooley <conor@kernel.org> 於 2023年10月25日 週三 下午10:14寫道：
>
> On Wed, Oct 25, 2023 at 05:32:21PM +0800, Nylon Chen wrote:
> > Hi Conor,
> >
> > Conor Dooley <conor@kernel.org> 於 2023年10月24日 週二 下午10:55寫道：
> > >
> > > Hey,
> > >
> > > On Tue, Oct 24, 2023 at 06:19:01PM +0800, Nylon Chen wrote:
> > > > This removes the active-low properties of the PWM-controlled LEDs in
> > > > the HiFive Unmatched device tree.
> > > >
> > > > The reference is hifive-unleashed-a00.pdf[0] and hifive-unmatched-schematics-v3.pdf[1].
> > > >
> > > > Link: https://sifive.cdn.prismic.io/sifive/c52a8e32-05ce-4aaf-95c8-7bf8453f8698_hifive-unleashed-a00-schematics-1.pdf [0]
> > > > Link: https://sifive.cdn.prismic.io/sifive/6a06d6c0-6e66-49b5-8e9e-e68ce76f4192_hifive-unmatched-schematics-v3.pdf [1]
> > >
> > > >
> > >
> > > This blank line should be removed if there is a follow-up.
> > thanks, I got it.
> > >
> > > > Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
> > >
> > > What did Vincent contribute to this patch? Are you missing a
> > > co-developed-by tag, perhaps?
> > Yes, Vincent was the first person to find the PWM driver problem, and
>
> That sounds like s/Signed-off-by/Reported-by/ then.
Thanks, I got it.
>
> Cheers,
> Conor.

Re: [v5 2/2] pwm: sifive: change the PWM controlled LED algorithm

[Nylon Chen]

Hi, Ping on the series.

Uwe, is there anything more I can do to push the process forward?

Nylon Chen <nylon.chen@sifive.com> 於 2023年10月24日 週二 下午6:19寫道：

>
> The `frac` variable represents the pulse inactive time, and the result
> of this algorithm is the pulse active time. Therefore, we must reverse the result.
>
> The reference is SiFive FU740-C000 Manual[0]
>
> Link: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf [0]
>
> Signed-off-by: Nylon Chen <nylon.chen@sifive.com>
> Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
> ---
>  drivers/pwm/pwm-sifive.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> index eabddb7c7820..353c2342fbf1 100644
> --- a/drivers/pwm/pwm-sifive.c
> +++ b/drivers/pwm/pwm-sifive.c
> @@ -101,7 +101,7 @@ static void pwm_sifive_update_clock(struct pwm_sifive_ddata *ddata,
>
>         /* As scale <= 15 the shift operation cannot overflow. */
>         num = (unsigned long long)NSEC_PER_SEC << (PWM_SIFIVE_CMPWIDTH + scale);
> -       ddata->real_period = div64_ul(num, rate);
> +       ddata->real_period = DIV_ROUND_UP_ULL(num, rate);
>         dev_dbg(ddata->chip.dev,
>                 "New real_period = %u ns\n", ddata->real_period);
>  }
> @@ -121,13 +121,14 @@ static int pwm_sifive_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>                 state->enabled = false;
>
>         state->period = ddata->real_period;
> +
> +       duty = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - duty;
>         state->duty_cycle =
>                 (u64)duty * ddata->real_period >> PWM_SIFIVE_CMPWIDTH;
> -       state->polarity = PWM_POLARITY_INVERSED;
> +       state->polarity = PWM_POLARITY_NORMAL;
>
>         return 0;
>  }
> -
>  static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>                             const struct pwm_state *state)
>  {
> @@ -139,7 +140,7 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>         int ret = 0;
>         u32 frac;
>
> -       if (state->polarity != PWM_POLARITY_INVERSED)
> +       if (state->polarity != PWM_POLARITY_NORMAL)
>                 return -EINVAL;
>
>         cur_state = pwm->state;
> @@ -158,6 +159,7 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>         num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH);
>         frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
>         /* The hardware cannot generate a 100% duty cycle */
> +       frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac;
>         frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
>
>         mutex_lock(&ddata->lock);
> --
> 2.42.0
>

Re: [v5 2/2] pwm: sifive: change the PWM controlled LED algorithm

[Krzysztof Kozlowski]

On 09/11/2023 08:02, Nylon Chen wrote:
> Hi, Ping on the series.
> 
> Uwe, is there anything more I can do to push the process forward?

It's merge window. What do you exactly expect to happen?

Best regards,
Krzysztof

Re: [v5 2/2] pwm: sifive: change the PWM controlled LED algorithm

[Nylon Chen]

Hi Ping on the series.

The merge window should have ended.

Is there anything more I can do to push the process forward?

Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 於 2023年11月9日 週四 下午4:22寫道：
>
> On 09/11/2023 08:02, Nylon Chen wrote:
> > Hi, Ping on the series.
> >
> > Uwe, is there anything more I can do to push the process forward?
>
> It's merge window. What do you exactly expect to happen?
>
> Best regards,
> Krzysztof
>

Re: [v5 1/2] riscv: dts: sifive: unleashed/unmatched: Remove PWM controlled LED's active-low properties

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 844 bytes --]

On Tue, Oct 24, 2023 at 06:19:01PM +0800, Nylon Chen wrote:
> This removes the active-low properties of the PWM-controlled LEDs in
> the HiFive Unmatched device tree.
> 
> The reference is hifive-unleashed-a00.pdf[0] and hifive-unmatched-schematics-v3.pdf[1].
> 
> Link: https://sifive.cdn.prismic.io/sifive/c52a8e32-05ce-4aaf-95c8-7bf8453f8698_hifive-unleashed-a00-schematics-1.pdf [0]
> Link: https://sifive.cdn.prismic.io/sifive/6a06d6c0-6e66-49b5-8e9e-e68ce76f4192_hifive-unmatched-schematics-v3.pdf [1]

IMHO the commit log should mention that the driver got inversion wrong
and so both dts and driver need adaption.

Otherwise looks fine to me.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [v5 2/2] pwm: sifive: change the PWM controlled LED algorithm

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3012 bytes --]

Hello Nylon,

On Tue, Oct 24, 2023 at 06:19:02PM +0800, Nylon Chen wrote:
> The `frac` variable represents the pulse inactive time, and the result
> of this algorithm is the pulse active time. Therefore, we must reverse the result.
> 
> The reference is SiFive FU740-C000 Manual[0]
> 
> Link: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf [0]
> 
> Signed-off-by: Nylon Chen <nylon.chen@sifive.com>
> Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
> ---
>  drivers/pwm/pwm-sifive.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> index eabddb7c7820..353c2342fbf1 100644
> --- a/drivers/pwm/pwm-sifive.c
> +++ b/drivers/pwm/pwm-sifive.c
> @@ -101,7 +101,7 @@ static void pwm_sifive_update_clock(struct pwm_sifive_ddata *ddata,
>  
>  	/* As scale <= 15 the shift operation cannot overflow. */
>  	num = (unsigned long long)NSEC_PER_SEC << (PWM_SIFIVE_CMPWIDTH + scale);
> -	ddata->real_period = div64_ul(num, rate);
> +	ddata->real_period = DIV_ROUND_UP_ULL(num, rate);

It's unclear to me, why you changed that.

>  	dev_dbg(ddata->chip.dev,
>  		"New real_period = %u ns\n", ddata->real_period);
>  }
> @@ -121,13 +121,14 @@ static int pwm_sifive_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>  		state->enabled = false;
>  
>  	state->period = ddata->real_period;
> +
> +	duty = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - duty;

I would have placed that directly after

	duty = readl(...);

which then also influences

	state->enabled = duty > 0;

(as it should?).

>  	state->duty_cycle =
>  		(u64)duty * ddata->real_period >> PWM_SIFIVE_CMPWIDTH;
> -	state->polarity = PWM_POLARITY_INVERSED;
> +	state->polarity = PWM_POLARITY_NORMAL;
>  
>  	return 0;
>  }
> -
>  static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  			    const struct pwm_state *state)
>  {
> @@ -139,7 +140,7 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	int ret = 0;
>  	u32 frac;
>  
> -	if (state->polarity != PWM_POLARITY_INVERSED)
> +	if (state->polarity != PWM_POLARITY_NORMAL)
>  		return -EINVAL;
>  
>  	cur_state = pwm->state;
> @@ -158,6 +159,7 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH);
>  	frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
>  	/* The hardware cannot generate a 100% duty cycle */
> +	frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac;
>  	frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);

frac can only be > (1U << PWM_SIFIVE_CMPWIDTH) - 1 if an overflow
happend the line above. Is that what you want here?

>  	mutex_lock(&ddata->lock);

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [v5 2/2] pwm: sifive: change the PWM controlled LED algorithm

[Nylon Chen]

Uwe Kleine-König <u.kleine-koenig@pengutronix.de> 於 2023年12月12日 週二 上午4:50寫道：
>
> Hello Nylon,

Hi Uwe, thanks for your feedback.
>
>
>
> On Tue, Oct 24, 2023 at 06:19:02PM +0800, Nylon Chen wrote:
> > The `frac` variable represents the pulse inactive time, and the result
> > of this algorithm is the pulse active time. Therefore, we must reverse the result.
> >
> > The reference is SiFive FU740-C000 Manual[0]
> >
> > Link: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf [0]
> >
> > Signed-off-by: Nylon Chen <nylon.chen@sifive.com>
> > Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
> > ---
> >  drivers/pwm/pwm-sifive.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> > index eabddb7c7820..353c2342fbf1 100644
> > --- a/drivers/pwm/pwm-sifive.c
> > +++ b/drivers/pwm/pwm-sifive.c
> > @@ -101,7 +101,7 @@ static void pwm_sifive_update_clock(struct pwm_sifive_ddata *ddata,
> >
> >       /* As scale <= 15 the shift operation cannot overflow. */
> >       num = (unsigned long long)NSEC_PER_SEC << (PWM_SIFIVE_CMPWIDTH + scale);
> > -     ddata->real_period = div64_ul(num, rate);
> > +     ddata->real_period = DIV_ROUND_UP_ULL(num, rate);
>
> It's unclear to me, why you changed that.
Because there is a gap in idempotent tests.
e.g.
root@unmatched:~# echo 110 >
/sys/devices/platform/led-controller-1/leds/d12/brightness
[  706.987712] .apply is not idempotent (ena=1 pol=0 1739692/4032985)
-> (ena=1 pol=0 1739630/4032985)
root@unmatched:~# echo 120 >
/sys/devices/platform/led-controller-1/leds/d12/brightness
[  709.817554] .apply is not idempotent (ena=1 pol=0 1897846/4032985)
-> (ena=1 pol=0 1897784/4032985)

Round the result to the nearest whole number. This ensures that
real_period is always a reasonable integer that is not lower than the
actual value.

After modification, idempotent errors can be avoided.
>
>
> >       dev_dbg(ddata->chip.dev,
> >               "New real_period = %u ns\n", ddata->real_period);
> >  }
> > @@ -121,13 +121,14 @@ static int pwm_sifive_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> >               state->enabled = false;
> >
> >       state->period = ddata->real_period;
> > +
> > +     duty = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - duty;
>
> I would have placed that directly after
>
>         duty = readl(...);
>
> which then also influences
>
>         state->enabled = duty > 0;
>
> (as it should?).
>
Yes, you are right. I will make relevant corrections.
...
        duty = readl(ddata->regs + PWM_SIFIVE_PWMCMP(pwm->hwpwm));
+       duty = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - duty;
-       state->enabled = duty <= 65535;
+       state->enabled = duty > 0;
...
        state->period = ddata->real_period;
-       duty = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - duty;

>
> >       state->duty_cycle =
> >               (u64)duty * ddata->real_period >> PWM_SIFIVE_CMPWIDTH;
> > -     state->polarity = PWM_POLARITY_INVERSED;
> > +     state->polarity = PWM_POLARITY_NORMAL;
> >
> >       return 0;
> >  }
> > -
> >  static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >                           const struct pwm_state *state)
> >  {
> > @@ -139,7 +140,7 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >       int ret = 0;
> >       u32 frac;
> >
> > -     if (state->polarity != PWM_POLARITY_INVERSED)
> > +     if (state->polarity != PWM_POLARITY_NORMAL)
> >               return -EINVAL;
> >
> >       cur_state = pwm->state;
> > @@ -158,6 +159,7 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >       num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH);
> >       frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
> >       /* The hardware cannot generate a 100% duty cycle */
> > +     frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac;
> >       frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
>
> frac can only be > (1U << PWM_SIFIVE_CMPWIDTH) - 1 if an overflow
> happend the line above. Is that what you want here?
I made a mistake, I pushed the wrong changes.

I want to invert it after taking the minimum value, which makes sense to me.
        frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
        /* The hardware cannot generate a 100% duty cycle */
        frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
+       frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac;

>
> >       mutex_lock(&ddata->lock);
>
> Best regards
> Uwe
Best regards
Nylon
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [v5 2/2] pwm: sifive: change the PWM controlled LED algorithm

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1879 bytes --]

Hello Nylon,

On Mon, Jan 08, 2024 at 04:27:40PM +0800, Nylon Chen wrote:
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> 於 2023年12月12日 週二 上午4:50寫道：
> > On Tue, Oct 24, 2023 at 06:19:02PM +0800, Nylon Chen wrote:
> > > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> > > index eabddb7c7820..353c2342fbf1 100644
> > > --- a/drivers/pwm/pwm-sifive.c
> > > +++ b/drivers/pwm/pwm-sifive.c
> > > @@ -101,7 +101,7 @@ static void pwm_sifive_update_clock(struct pwm_sifive_ddata *ddata,
> > >
> > >       /* As scale <= 15 the shift operation cannot overflow. */
> > >       num = (unsigned long long)NSEC_PER_SEC << (PWM_SIFIVE_CMPWIDTH + scale);
> > > -     ddata->real_period = div64_ul(num, rate);
> > > +     ddata->real_period = DIV_ROUND_UP_ULL(num, rate);
> >
> > It's unclear to me, why you changed that.
> Because there is a gap in idempotent tests.
> e.g.
> root@unmatched:~# echo 110 >
> /sys/devices/platform/led-controller-1/leds/d12/brightness
> [  706.987712] .apply is not idempotent (ena=1 pol=0 1739692/4032985)
> -> (ena=1 pol=0 1739630/4032985)
> root@unmatched:~# echo 120 >
> /sys/devices/platform/led-controller-1/leds/d12/brightness
> [  709.817554] .apply is not idempotent (ena=1 pol=0 1897846/4032985)
> -> (ena=1 pol=0 1897784/4032985)
> 
> Round the result to the nearest whole number. This ensures that
> real_period is always a reasonable integer that is not lower than the
> actual value.
> 
> After modification, idempotent errors can be avoided.

That's very welcome, however I think this should be a separate change.

I'll think about the rest of your changes when you send a new patch.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]