According to the circuit diagram of User LEDs - RGB described in themanual hifive-unleashed-a00.pdf[0] and hifive-unmatched-schematics-v3.pdf[1].
The behavior of PWM is acitve-high.
Removed patches: 1
New patches: (none)
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 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 (1):
riscv: dts: sifive: unleashed/unmatched: Remove PWM controlled LED's
active-low properties
arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts | 4 ----
arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts | 4 ----
2 files changed, 8 deletions(-)
--
2.40.1
Hey Nylon,
(I yoinked the reply to 1/1 to here, as it makes more sense in this
context)
> On Thu, Aug 03, 2023 at 10:15:10AM +0100, Conor Dooley wrote:
> > On Thu, Aug 03, 2023 at 04:57:33PM +0800, Nylon Chen wrote:
> > > According to the circuit diagram of User LEDs - RGB described in themanual hifive-unleashed-a00.pdf[0] and hifive-unmatched-schematics-v3.pdf[1].
> > >
> > > The behavior of PWM is acitve-high.
> > >
> > > Removed patches: 1
> > > New patches: (none)
> > >
> > > 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 v4:
> > > - Remove previous updates to the PWM algorithm.
> >
> > Why? I don't recall the conclusion on the previous version being that
> > that patch was not needed.
>
> I apologize for forgetting about this update earlier. Just now,
> I tried to pull rebase master and noticed that other developers seem
> to have made some fixes to the algorithm. Upon closer inspection, I
> found that they addressed the part we previously discussed with Emil
> and Uwe, such as "first pwm_apply_state."
>
> Therefore, my instinct tells me that they have already taken care of
> the issues we discussed before.
I didn't see anything in linux-next that would solve this problem of
inversion. The last meaningful change is:
commit 334c7b13d38321e47d1a51dba0bef9f4c403ec75
Author: Emil Renner Berthing <[email protected]>
AuthorDate: Wed Nov 9 12:37:24 2022 +0100
Commit: Thierry Reding <[email protected]>
CommitDate: Mon Jan 30 16:42:45 2023 +0100
pwm: sifive: Always let the first pwm_apply_state succeed
which predates your v3 by quite a bit.
> I will review the conflicting parts in the pwm-sifive.c code in my v4
> version once again to ensure there are no omissions. If I find any, I
> will submit v5 accordingly.
And if this patch is okay in isolation, please reply here explaining
which commit fixed the algorithm, so that I can pick it up.
Thanks,
Conor.
On Thu, Aug 03, 2023 at 04:57:33PM +0800, Nylon Chen wrote:
> According to the circuit diagram of User LEDs - RGB described in themanual hifive-unleashed-a00.pdf[0] and hifive-unmatched-schematics-v3.pdf[1].
>
> The behavior of PWM is acitve-high.
>
> Removed patches: 1
> New patches: (none)
>
> 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 v4:
> - Remove previous updates to the PWM algorithm.
Why? I don't recall the conclusion on the previous version being that
that patch was not needed.
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]
Acked-by: Conor Dooley <[email protected]>
Reviewed-by: Conor Dooley <[email protected]>
Signed-off-by: Vincent Chen <[email protected]>
Signed-off-by: Nylon Chen <[email protected]>
---
arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts | 4 ----
arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts | 4 ----
2 files changed, 8 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..7a9f336a391c 100644
--- a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
+++ b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
@@ -50,7 +50,6 @@ led-controller {
led-d1 {
pwms = <&pwm0 0 7812500 PWM_POLARITY_INVERTED>;
- active-low;
color = <LED_COLOR_ID_GREEN>;
max-brightness = <255>;
label = "d1";
@@ -58,7 +57,6 @@ led-d1 {
led-d2 {
pwms = <&pwm0 1 7812500 PWM_POLARITY_INVERTED>;
- active-low;
color = <LED_COLOR_ID_GREEN>;
max-brightness = <255>;
label = "d2";
@@ -66,7 +64,6 @@ led-d2 {
led-d3 {
pwms = <&pwm0 2 7812500 PWM_POLARITY_INVERTED>;
- active-low;
color = <LED_COLOR_ID_GREEN>;
max-brightness = <255>;
label = "d3";
@@ -74,7 +71,6 @@ led-d3 {
led-d4 {
pwms = <&pwm0 3 7812500 PWM_POLARITY_INVERTED>;
- active-low;
color = <LED_COLOR_ID_GREEN>;
max-brightness = <255>;
label = "d4";
diff --git a/arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts b/arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts
index 07387f9c135c..11f08a545ee6 100644
--- a/arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts
+++ b/arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts
@@ -52,7 +52,6 @@ led-controller-1 {
led-d12 {
pwms = <&pwm0 0 7812500 PWM_POLARITY_INVERTED>;
- active-low;
color = <LED_COLOR_ID_GREEN>;
max-brightness = <255>;
label = "d12";
@@ -69,19 +68,16 @@ multi-led {
led-red {
pwms = <&pwm0 2 7812500 PWM_POLARITY_INVERTED>;
- active-low;
color = <LED_COLOR_ID_RED>;
};
led-green {
pwms = <&pwm0 1 7812500 PWM_POLARITY_INVERTED>;
- active-low;
color = <LED_COLOR_ID_GREEN>;
};
led-blue {
pwms = <&pwm0 3 7812500 PWM_POLARITY_INVERTED>;
- active-low;
color = <LED_COLOR_ID_BLUE>;
};
};
--
2.40.1
Hi Conor,
Conor Dooley <[email protected]> 於 2023年8月3日 週四 下午5:44寫道：
>
> Hey Nylon,
>
> (I yoinked the reply to 1/1 to here, as it makes more sense in this
> context)
>
> > On Thu, Aug 03, 2023 at 10:15:10AM +0100, Conor Dooley wrote:
> > > On Thu, Aug 03, 2023 at 04:57:33PM +0800, Nylon Chen wrote:
> > > > According to the circuit diagram of User LEDs - RGB described in themanual hifive-unleashed-a00.pdf[0] and hifive-unmatched-schematics-v3.pdf[1].
> > > >
> > > > The behavior of PWM is acitve-high.
> > > >
> > > > Removed patches: 1
> > > > New patches: (none)
> > > >
> > > > 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 v4:
> > > > - Remove previous updates to the PWM algorithm.
> > >
> > > Why? I don't recall the conclusion on the previous version being that
> > > that patch was not needed.
> >
> > I apologize for forgetting about this update earlier. Just now,
> > I tried to pull rebase master and noticed that other developers seem
> > to have made some fixes to the algorithm. Upon closer inspection, I
> > found that they addressed the part we previously discussed with Emil
> > and Uwe, such as "first pwm_apply_state."
> >
> > Therefore, my instinct tells me that they have already taken care of
> > the issues we discussed before.
>
> I didn't see anything in linux-next that would solve this problem of
> inversion. The last meaningful change is:
> commit 334c7b13d38321e47d1a51dba0bef9f4c403ec75
> Author: Emil Renner Berthing <[email protected]>
> AuthorDate: Wed Nov 9 12:37:24 2022 +0100
> Commit: Thierry Reding <[email protected]>
> CommitDate: Mon Jan 30 16:42:45 2023 +0100
>
> pwm: sifive: Always let the first pwm_apply_state succeed
>
> which predates your v3 by quite a bit.
>
> > I will review the conflicting parts in the pwm-sifive.c code in my v4
> > version once again to ensure there are no omissions. If I find any, I
> > will submit v5 accordingly.
>
> And if this patch is okay in isolation, please reply here explaining
> which commit fixed the algorithm, so that I can pick it up.
This patch needs to be accompanied by modifications to the
pwm_sifive_apply() function to make sense.
I will double-check and review the previous discussions to ensure
that. Thank you for your response.
>
> Thanks,
> Conor.
Hi Conor,
Thank you for patiently giving me advice. I appreciate your help.
Not long ago, I said, "This patch needs to be accompanied by
modifications to the pwm_sifive_apply() function to make sense."
I recently reviewed the v3 version, and after discussing it with Emil,
there are several areas that require modification. I will provide the
necessary changes for each of them:
1. polarity check. (Suggestion from Uwe)
- if (state->polarity != PWM_POLARITY_INVERSED)
+ if (state->polarity != PWM_POLARITY_NORMAL)
2. avoid using old periodperiod, not state->period
- period = max(state->period, ddata->approx_period);
- frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
+ frac = DIV64_U64_ROUND_CLOSEST(num, period);
3. add a conditional check can be added in the code to set
ddata->approx_period to state->period when state->period is smaller
than ddata->approx_period
if (state->period != ddata->approx_period) {
...
+ if (state->period < ddata->approx_period) {
+ ddata->approx_period = state->period;
+ }
- ddata->approx_period = state->period;
+ period = ddata->approx_period;
I will use 'unmatched' on my end to verify again. If there are any
other errors, feel free to point them out. Thank you.
Nylon Chen <[email protected]> 於 2023年8月4日 週五 上午9:42寫道：
>
> Hi Conor,
>
> Conor Dooley <[email protected]> 於 2023年8月3日 週四 下午5:44寫道：
> >
> > Hey Nylon,
> >
> > (I yoinked the reply to 1/1 to here, as it makes more sense in this
> > context)
> >
> > > On Thu, Aug 03, 2023 at 10:15:10AM +0100, Conor Dooley wrote:
> > > > On Thu, Aug 03, 2023 at 04:57:33PM +0800, Nylon Chen wrote:
> > > > > According to the circuit diagram of User LEDs - RGB described in themanual hifive-unleashed-a00.pdf[0] and hifive-unmatched-schematics-v3.pdf[1].
> > > > >
> > > > > The behavior of PWM is acitve-high.
> > > > >
> > > > > Removed patches: 1
> > > > > New patches: (none)
> > > > >
> > > > > 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 v4:
> > > > > - Remove previous updates to the PWM algorithm.
> > > >
> > > > Why? I don't recall the conclusion on the previous version being that
> > > > that patch was not needed.
> > >
> > > I apologize for forgetting about this update earlier. Just now,
> > > I tried to pull rebase master and noticed that other developers seem
> > > to have made some fixes to the algorithm. Upon closer inspection, I
> > > found that they addressed the part we previously discussed with Emil
> > > and Uwe, such as "first pwm_apply_state."
> > >
> > > Therefore, my instinct tells me that they have already taken care of
> > > the issues we discussed before.
> >
> > I didn't see anything in linux-next that would solve this problem of
> > inversion. The last meaningful change is:
> > commit 334c7b13d38321e47d1a51dba0bef9f4c403ec75
> > Author: Emil Renner Berthing <[email protected]>
> > AuthorDate: Wed Nov 9 12:37:24 2022 +0100
> > Commit: Thierry Reding <[email protected]>
> > CommitDate: Mon Jan 30 16:42:45 2023 +0100
> >
> > pwm: sifive: Always let the first pwm_apply_state succeed
> >
> > which predates your v3 by quite a bit.
> >
> > > I will review the conflicting parts in the pwm-sifive.c code in my v4
> > > version once again to ensure there are no omissions. If I find any, I
> > > will submit v5 accordingly.
> >
> > And if this patch is okay in isolation, please reply here explaining
> > which commit fixed the algorithm, so that I can pick it up.
> This patch needs to be accompanied by modifications to the
> pwm_sifive_apply() function to make sense.
>
> I will double-check and review the previous discussions to ensure
> that. Thank you for your response.
> >
> > Thanks,
> > Conor.
On Fri, Aug 04, 2023 at 02:54:33PM +0800, Nylon Chen wrote:
> Hi Conor,
>
> Thank you for patiently giving me advice. I appreciate your help.
>
> Not long ago, I said, "This patch needs to be accompanied by
> modifications to the pwm_sifive_apply() function to make sense."
>
> I recently reviewed the v3 version, and after discussing it with Emil,
> there are several areas that require modification. I will provide the
> necessary changes for each of them:
>
> 1. polarity check. (Suggestion from Uwe)
> - if (state->polarity != PWM_POLARITY_INVERSED)
> + if (state->polarity != PWM_POLARITY_NORMAL)
> 2. avoid using old periodperiod, not state->period
> - period = max(state->period, ddata->approx_period);
> - frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
> + frac = DIV64_U64_ROUND_CLOSEST(num, period);
> 3. add a conditional check can be added in the code to set
> ddata->approx_period to state->period when state->period is smaller
> than ddata->approx_period
> if (state->period != ddata->approx_period) {
> ...
> + if (state->period < ddata->approx_period) {
> + ddata->approx_period = state->period;
> + }
> - ddata->approx_period = state->period;
> + period = ddata->approx_period;
>
> I will use 'unmatched' on my end to verify again. If there are any
> other errors, feel free to point them out. Thank you.
I'm not sure of the driver details without going and looking into the
code itself, but this sounds like it makes a lot more sense than just
flipping the polarity in the dts. Thanks for taking another look!