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
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 <[email protected]>
Signed-off-by: Vincent Chen <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Nylon Chen <[email protected]>
---
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
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 <[email protected]>
What did Vincent contribute to this patch? Are you missing a
co-developed-by tag, perhaps?
> Signed-off-by: Nylon Chen <[email protected]>
Reviewed-by: Conor Dooley <[email protected]>
Acked-by: Conor Dooley <[email protected]>
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
>
Hi Conor,
Conor Dooley <[email protected]> 於 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 <[email protected]>
>
> 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 <[email protected]>
>
> Reviewed-by: Conor Dooley <[email protected]>
> Acked-by: Conor Dooley <[email protected]>
>
> 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
> >
On Wed, Oct 25, 2023 at 05:32:21PM +0800, Nylon Chen wrote:
> Hi Conor,
>
> Conor Dooley <[email protected]> 於 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 <[email protected]>
> >
> > 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.
Conor Dooley <[email protected]> 於 2023年10月25日 週三 下午10:14寫道：
>
> On Wed, Oct 25, 2023 at 05:32:21PM +0800, Nylon Chen wrote:
> > Hi Conor,
> >
> > Conor Dooley <[email protected]> 於 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 <[email protected]>
> > >
> > > 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.
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 <[email protected]> 於 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
>
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/ |
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 <[email protected]>
> Signed-off-by: Vincent Chen <[email protected]>
> ---
> 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/ |
Uwe Kleine-König <[email protected]> 於 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 <[email protected]>
> > Signed-off-by: Vincent Chen <[email protected]>
> > ---
> > 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/ |
Hello Nylon,
On Mon, Jan 08, 2024 at 04:27:40PM +0800, Nylon Chen wrote:
> Uwe Kleine-König <[email protected]> 於 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/ |