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: 2
New patches: (none)
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 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 | 4 ----
arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts | 4 ----
drivers/pwm/pwm-sifive.c | 9 ++++++---
3 files changed, 6 insertions(+), 11 deletions(-)
--
2.40.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]
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.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]
Acked-by: Conor Dooley <[email protected]>
Reviewed-by: Conor Dooley <[email protected]>
Signed-off-by: Nylon Chen <[email protected]>
Signed-off-by: Vincent Chen <[email protected]>
---
drivers/pwm/pwm-sifive.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
index 393a4b97fc19..d5d5f36da297 100644
--- a/drivers/pwm/pwm-sifive.c
+++ b/drivers/pwm/pwm-sifive.c
@@ -132,13 +132,13 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
{
struct pwm_sifive_ddata *ddata = pwm_sifive_chip_to_ddata(chip);
struct pwm_state cur_state;
- unsigned int duty_cycle;
+ unsigned int duty_cycle, period;
unsigned long long num;
bool enabled;
int ret = 0;
u32 frac;
- if (state->polarity != PWM_POLARITY_INVERSED)
+ if (state->polarity != PWM_POLARITY_NORMAL && state->polarity != PWM_POLARITY_INVERSED)
return -EINVAL;
cur_state = pwm->state;
@@ -154,10 +154,13 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
* calculating the register values first and then writing them
* consecutively
*/
+ period = max(state->period, ddata->approx_period);
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 = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
+ /* The hardware cannot generate a 100% duty cycle */
+ frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac;
+
mutex_lock(&ddata->lock);
if (state->period != ddata->approx_period) {
--
2.40.0
On Thu, 20 Apr 2023 at 11:35, Nylon Chen <[email protected]> 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]
>
> Acked-by: Conor Dooley <[email protected]>
> Reviewed-by: Conor Dooley <[email protected]>
> Signed-off-by: Nylon Chen <[email protected]>
> Signed-off-by: Vincent Chen <[email protected]>
> ---
> drivers/pwm/pwm-sifive.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> index 393a4b97fc19..d5d5f36da297 100644
> --- a/drivers/pwm/pwm-sifive.c
> +++ b/drivers/pwm/pwm-sifive.c
> @@ -132,13 +132,13 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> {
> struct pwm_sifive_ddata *ddata = pwm_sifive_chip_to_ddata(chip);
> struct pwm_state cur_state;
> - unsigned int duty_cycle;
> + unsigned int duty_cycle, period;
> unsigned long long num;
> bool enabled;
> int ret = 0;
> u32 frac;
>
> - if (state->polarity != PWM_POLARITY_INVERSED)
> + if (state->polarity != PWM_POLARITY_NORMAL && state->polarity != PWM_POLARITY_INVERSED)
> return -EINVAL;
>
> cur_state = pwm->state;
> @@ -154,10 +154,13 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> * calculating the register values first and then writing them
> * consecutively
> */
> + period = max(state->period, ddata->approx_period);
Hi Nylon,
I don't understand this patch. You introduce this new variable,
period, and set it here but you never seem to use it. If you planned
to use it instead of state->period below, why should it be the max of
the old period and what is requested? What happens if the consumer
wants to lower the period?
Also above you now allow both PWM_POLARITY_NORMAL and
PWM_POLARITY_INVERSED but you treat both cases the same.
/Emil
> 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 = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
> + /* The hardware cannot generate a 100% duty cycle */
> + frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac;
> +
>
> mutex_lock(&ddata->lock);
> if (state->period != ddata->approx_period) {
> --
> 2.40.0
>
On Thu, 20 Apr 2023 at 12:41, Nylon Chen <[email protected]> wrote:
>
> Hi, Emil
>
> Emil Renner Berthing <[email protected]> 於 2023年4月20日 週四 下午6:04寫道：
> >
> > On Thu, 20 Apr 2023 at 11:35, Nylon Chen <[email protected]> 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]
> > >
> > > Acked-by: Conor Dooley <[email protected]>
> > > Reviewed-by: Conor Dooley <[email protected]>
> > > Signed-off-by: Nylon Chen <[email protected]>
> > > Signed-off-by: Vincent Chen <[email protected]>
> > > ---
> > > drivers/pwm/pwm-sifive.c | 9 ++++++---
> > > 1 file changed, 6 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> > > index 393a4b97fc19..d5d5f36da297 100644
> > > --- a/drivers/pwm/pwm-sifive.c
> > > +++ b/drivers/pwm/pwm-sifive.c
> > > @@ -132,13 +132,13 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > {
> > > struct pwm_sifive_ddata *ddata = pwm_sifive_chip_to_ddata(chip);
> > > struct pwm_state cur_state;
> > > - unsigned int duty_cycle;
> > > + unsigned int duty_cycle, period;
> > > unsigned long long num;
> > > bool enabled;
> > > int ret = 0;
> > > u32 frac;
> > >
> > > - if (state->polarity != PWM_POLARITY_INVERSED)
> > > + if (state->polarity != PWM_POLARITY_NORMAL && state->polarity != PWM_POLARITY_INVERSED)
> > > return -EINVAL;
> > >
> > > cur_state = pwm->state;
> > > @@ -154,10 +154,13 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > * calculating the register values first and then writing them
> > > * consecutively
> > > */
> > > + period = max(state->period, ddata->approx_period);
> >
> > Hi Nylon,
> >
> > I don't understand this patch. You introduce this new variable,
> > period, and set it here but you never seem to use it. If you planned
> > to use it instead of state->period below, why should it be the max of
> > the old period and what is requested? What happens if the consumer
> > wants to lower the period?
> Sorry this was an oversight on my part, there was a line correction that didn't change to
> - frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
> + frac = DIV64_U64_ROUND_CLOSEST(num, period);
I see, so then my second question was why period needs to be the
larger of the previous period and the requested period.
What happens if the requested period, state->period, is lower than the
old period, ddata->approx_period? Then the period will be changed to
state->period below, but the calculations will be made using period =
ddata->approx_period, right?
> >
> > Also above you now allow both PWM_POLARITY_NORMAL and
> > PWM_POLARITY_INVERSED but you treat both cases the same.
> I may have misunderstood what Uwe means here, I will confirm again here
> >
> > /Emil
> >
> > > 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 = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
> > > + /* The hardware cannot generate a 100% duty cycle */
> > > + frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac;
> > > +
> > >
> > > mutex_lock(&ddata->lock);
> > > if (state->period != ddata->approx_period) {
> > > --
> > > 2.40.0
> > >
Hi Nylon,
kernel test robot noticed the following build warnings:
[auto build test WARNING on robh/for-next]
[also build test WARNING on thierry-reding-pwm/for-next rockchip/for-next linus/master v6.3-rc7 next-20230419]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Nylon-Chen/riscv-dts-sifive-unleashed-unmatched-Remove-PWM-controlled-LED-s-active-low-properties/20230420-173619
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20230420093457.18936-3-nylon.chen%40sifive.com
patch subject: [PATCH v3 2/2] pwm: sifive: change the PWM controlled LED algorithm
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230420/[email protected]/config)
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/f2d706bf61190a45a8f90f1f455bc943d4ac7b6e
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Nylon-Chen/riscv-dts-sifive-unleashed-unmatched-Remove-PWM-controlled-LED-s-active-low-properties/20230420-173619
git checkout f2d706bf61190a45a8f90f1f455bc943d4ac7b6e
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
In file included from include/linux/kernel.h:26,
from include/linux/clk.h:13,
from drivers/pwm/pwm-sifive.c:14:
drivers/pwm/pwm-sifive.c: In function 'pwm_sifive_apply':
include/linux/minmax.h:20:35: warning: comparison of distinct pointer types lacks a cast
20 | (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
| ^~
include/linux/minmax.h:26:18: note: in expansion of macro '__typecheck'
26 | (__typecheck(x, y) && __no_side_effects(x, y))
| ^~~~~~~~~~~
include/linux/minmax.h:36:31: note: in expansion of macro '__safe_cmp'
36 | __builtin_choose_expr(__safe_cmp(x, y), \
| ^~~~~~~~~~
include/linux/minmax.h:74:25: note: in expansion of macro '__careful_cmp'
74 | #define max(x, y) __careful_cmp(x, y, >)
| ^~~~~~~~~~~~~
drivers/pwm/pwm-sifive.c:157:18: note: in expansion of macro 'max'
157 | period = max(state->period, ddata->approx_period);
| ^~~
>> drivers/pwm/pwm-sifive.c:135:34: warning: variable 'period' set but not used [-Wunused-but-set-variable]
135 | unsigned int duty_cycle, period;
| ^~~~~~
vim +/period +135 drivers/pwm/pwm-sifive.c
129
130 static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
131 const struct pwm_state *state)
132 {
133 struct pwm_sifive_ddata *ddata = pwm_sifive_chip_to_ddata(chip);
134 struct pwm_state cur_state;
> 135 unsigned int duty_cycle, period;
136 unsigned long long num;
137 bool enabled;
138 int ret = 0;
139 u32 frac;
140
141 if (state->polarity != PWM_POLARITY_NORMAL && state->polarity != PWM_POLARITY_INVERSED)
142 return -EINVAL;
143
144 cur_state = pwm->state;
145 enabled = cur_state.enabled;
146
147 duty_cycle = state->duty_cycle;
148 if (!state->enabled)
149 duty_cycle = 0;
150
151 /*
152 * The problem of output producing mixed setting as mentioned at top,
153 * occurs here. To minimize the window for this problem, we are
154 * calculating the register values first and then writing them
155 * consecutively
156 */
157 period = max(state->period, ddata->approx_period);
158 num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH);
159 frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
160 frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
161 /* The hardware cannot generate a 100% duty cycle */
162 frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac;
163
164
165 mutex_lock(&ddata->lock);
166 if (state->period != ddata->approx_period) {
167 /*
168 * Don't let a 2nd user change the period underneath the 1st user.
169 * However if ddate->approx_period == 0 this is the first time we set
170 * any period, so let whoever gets here first set the period so other
171 * users who agree on the period won't fail.
172 */
173 if (ddata->user_count != 1 && ddata->approx_period) {
174 mutex_unlock(&ddata->lock);
175 return -EBUSY;
176 }
177 ddata->approx_period = state->period;
178 pwm_sifive_update_clock(ddata, clk_get_rate(ddata->clk));
179 }
180 mutex_unlock(&ddata->lock);
181
182 /*
183 * If the PWM is enabled the clk is already on. So only enable it
184 * conditionally to have it on exactly once afterwards independent of
185 * the PWM state.
186 */
187 if (!enabled) {
188 ret = clk_enable(ddata->clk);
189 if (ret) {
190 dev_err(ddata->chip.dev, "Enable clk failed\n");
191 return ret;
192 }
193 }
194
195 writel(frac, ddata->regs + PWM_SIFIVE_PWMCMP(pwm->hwpwm));
196
197 if (!state->enabled)
198 clk_disable(ddata->clk);
199
200 return 0;
201 }
202
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Hi Emil,
Emil Renner Berthing <[email protected]> 於 2023年4月20日
週四 下午6:46寫道：
>
> On Thu, 20 Apr 2023 at 12:41, Nylon Chen <[email protected]> wrote:
> >
> > Hi, Emil
> >
> > Emil Renner Berthing <[email protected]> 於 2023年4月20日 週四 下午6:04寫道：
> > >
> > > On Thu, 20 Apr 2023 at 11:35, Nylon Chen <[email protected]> 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]
> > > >
> > > > Acked-by: Conor Dooley <[email protected]>
> > > > Reviewed-by: Conor Dooley <[email protected]>
> > > > Signed-off-by: Nylon Chen <[email protected]>
> > > > Signed-off-by: Vincent Chen <[email protected]>
> > > > ---
> > > > drivers/pwm/pwm-sifive.c | 9 ++++++---
> > > > 1 file changed, 6 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> > > > index 393a4b97fc19..d5d5f36da297 100644
> > > > --- a/drivers/pwm/pwm-sifive.c
> > > > +++ b/drivers/pwm/pwm-sifive.c
> > > > @@ -132,13 +132,13 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > {
> > > > struct pwm_sifive_ddata *ddata = pwm_sifive_chip_to_ddata(chip);
> > > > struct pwm_state cur_state;
> > > > - unsigned int duty_cycle;
> > > > + unsigned int duty_cycle, period;
> > > > unsigned long long num;
> > > > bool enabled;
> > > > int ret = 0;
> > > > u32 frac;
> > > >
> > > > - if (state->polarity != PWM_POLARITY_INVERSED)
> > > > + if (state->polarity != PWM_POLARITY_NORMAL && state->polarity != PWM_POLARITY_INVERSED)
> > > > return -EINVAL;
> > > >
> > > > cur_state = pwm->state;
> > > > @@ -154,10 +154,13 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > * calculating the register values first and then writing them
> > > > * consecutively
> > > > */
> > > > + period = max(state->period, ddata->approx_period);
> > >
> > > Hi Nylon,
> > >
> > > I don't understand this patch. You introduce this new variable,
> > > period, and set it here but you never seem to use it. If you planned
> > > to use it instead of state->period below, why should it be the max of
> > > the old period and what is requested? What happens if the consumer
> > > wants to lower the period?
> > Sorry this was an oversight on my part, there was a line correction that didn't change to
> > - frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
> > + frac = DIV64_U64_ROUND_CLOSEST(num, period);
>
> I see, so then my second question was why period needs to be the
> larger of the previous period and the requested period.
>
> What happens if the requested period, state->period, is lower than the
> old period, ddata->approx_period? Then the period will be changed to
> state->period below, but the calculations will be made using period =
> ddata->approx_period, right?
Your understanding is correct. According to the new algorithm proposed
by Uwe, the goal is to:
Pick the biggest period length possible that is not bigger than the
requested period.
>
> > >
> > > Also above you now allow both PWM_POLARITY_NORMAL and
> > > PWM_POLARITY_INVERSED but you treat both cases the same.
> > I may have misunderstood what Uwe means here, I will confirm again here
> > >
> > > /Emil
> > >
> > > > 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 = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
> > > > + /* The hardware cannot generate a 100% duty cycle */
> > > > + frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac;
> > > > +
> > > >
> > > > mutex_lock(&ddata->lock);
> > > > if (state->period != ddata->approx_period) {
> > > > --
> > > > 2.40.0
> > > >
On Fri, 21 Apr 2023 at 08:16, Nylon Chen <[email protected]> wrote:
>
> Hi Emil,
>
> Emil Renner Berthing <[email protected]> 於 2023年4月20日
> 週四 下午6:46寫道：
> >
> > On Thu, 20 Apr 2023 at 12:41, Nylon Chen <[email protected]> wrote:
> > >
> > > Hi, Emil
> > >
> > > Emil Renner Berthing <[email protected]> 於 2023年4月20日 週四 下午6:04寫道：
> > > >
> > > > On Thu, 20 Apr 2023 at 11:35, Nylon Chen <[email protected]> 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]
> > > > >
> > > > > Acked-by: Conor Dooley <[email protected]>
> > > > > Reviewed-by: Conor Dooley <[email protected]>
> > > > > Signed-off-by: Nylon Chen <[email protected]>
> > > > > Signed-off-by: Vincent Chen <[email protected]>
> > > > > ---
> > > > > drivers/pwm/pwm-sifive.c | 9 ++++++---
> > > > > 1 file changed, 6 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> > > > > index 393a4b97fc19..d5d5f36da297 100644
> > > > > --- a/drivers/pwm/pwm-sifive.c
> > > > > +++ b/drivers/pwm/pwm-sifive.c
> > > > > @@ -132,13 +132,13 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > > {
> > > > > struct pwm_sifive_ddata *ddata = pwm_sifive_chip_to_ddata(chip);
> > > > > struct pwm_state cur_state;
> > > > > - unsigned int duty_cycle;
> > > > > + unsigned int duty_cycle, period;
> > > > > unsigned long long num;
> > > > > bool enabled;
> > > > > int ret = 0;
> > > > > u32 frac;
> > > > >
> > > > > - if (state->polarity != PWM_POLARITY_INVERSED)
> > > > > + if (state->polarity != PWM_POLARITY_NORMAL && state->polarity != PWM_POLARITY_INVERSED)
> > > > > return -EINVAL;
> > > > >
> > > > > cur_state = pwm->state;
> > > > > @@ -154,10 +154,13 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > > * calculating the register values first and then writing them
> > > > > * consecutively
> > > > > */
> > > > > + period = max(state->period, ddata->approx_period);
> > > >
> > > > Hi Nylon,
> > > >
> > > > I don't understand this patch. You introduce this new variable,
> > > > period, and set it here but you never seem to use it. If you planned
> > > > to use it instead of state->period below, why should it be the max of
> > > > the old period and what is requested? What happens if the consumer
> > > > wants to lower the period?
> > > Sorry this was an oversight on my part, there was a line correction that didn't change to
> > > - frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
> > > + frac = DIV64_U64_ROUND_CLOSEST(num, period);
> >
> > I see, so then my second question was why period needs to be the
> > larger of the previous period and the requested period.
> >
> > What happens if the requested period, state->period, is lower than the
> > old period, ddata->approx_period? Then the period will be changed to
> > state->period below, but the calculations will be made using period =
> > ddata->approx_period, right?
>
> Your understanding is correct. According to the new algorithm proposed
> by Uwe, the goal is to:
> Pick the biggest period length possible that is not bigger than the
> requested period.
Right, and to be clear: this patch doesn't do that.
If the previous period in ddata->approx_period is bigger than the
requested period in state->period, it will do the frac calculations
with the old period, but still set the period to the shorter requested
period.
> > > >
> > > > Also above you now allow both PWM_POLARITY_NORMAL and
> > > > PWM_POLARITY_INVERSED but you treat both cases the same.
> > > I may have misunderstood what Uwe means here, I will confirm again here
> > > >
> > > > /Emil
> > > >
> > > > > 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 = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
> > > > > + /* The hardware cannot generate a 100% duty cycle */
> > > > > + frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac;
> > > > > +
> > > > >
> > > > > mutex_lock(&ddata->lock);
> > > > > if (state->period != ddata->approx_period) {
> > > > > --
> > > > > 2.40.0
> > > > >
Hi Nylon,
kernel test robot noticed the following build warnings:
[auto build test WARNING on robh/for-next]
[also build test WARNING on thierry-reding-pwm/for-next rockchip/for-next linus/master v6.3-rc7 next-20230421]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Nylon-Chen/riscv-dts-sifive-unleashed-unmatched-Remove-PWM-controlled-LED-s-active-low-properties/20230420-173619
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20230420093457.18936-3-nylon.chen%40sifive.com
patch subject: [PATCH v3 2/2] pwm: sifive: change the PWM controlled LED algorithm
config: sparc64-randconfig-s031-20230421 (https://download.01.org/0day-ci/archive/20230422/[email protected]/config)
compiler: sparc64-linux-gcc (GCC) 12.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-39-gce1a6720-dirty
# https://github.com/intel-lab-lkp/linux/commit/f2d706bf61190a45a8f90f1f455bc943d4ac7b6e
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Nylon-Chen/riscv-dts-sifive-unleashed-unmatched-Remove-PWM-controlled-LED-s-active-low-properties/20230420-173619
git checkout f2d706bf61190a45a8f90f1f455bc943d4ac7b6e
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=sparc64 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=sparc64 SHELL=/bin/bash drivers/pwm/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
sparse warnings: (new ones prefixed by >>)
>> drivers/pwm/pwm-sifive.c:157:18: sparse: sparse: incompatible types in comparison expression (different type sizes):
>> drivers/pwm/pwm-sifive.c:157:18: sparse: unsigned long long const *
>> drivers/pwm/pwm-sifive.c:157:18: sparse: unsigned int *
vim +157 drivers/pwm/pwm-sifive.c
129
130 static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
131 const struct pwm_state *state)
132 {
133 struct pwm_sifive_ddata *ddata = pwm_sifive_chip_to_ddata(chip);
134 struct pwm_state cur_state;
135 unsigned int duty_cycle, period;
136 unsigned long long num;
137 bool enabled;
138 int ret = 0;
139 u32 frac;
140
141 if (state->polarity != PWM_POLARITY_NORMAL && state->polarity != PWM_POLARITY_INVERSED)
142 return -EINVAL;
143
144 cur_state = pwm->state;
145 enabled = cur_state.enabled;
146
147 duty_cycle = state->duty_cycle;
148 if (!state->enabled)
149 duty_cycle = 0;
150
151 /*
152 * The problem of output producing mixed setting as mentioned at top,
153 * occurs here. To minimize the window for this problem, we are
154 * calculating the register values first and then writing them
155 * consecutively
156 */
> 157 period = max(state->period, ddata->approx_period);
158 num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH);
159 frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
160 frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
161 /* The hardware cannot generate a 100% duty cycle */
162 frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac;
163
164
165 mutex_lock(&ddata->lock);
166 if (state->period != ddata->approx_period) {
167 /*
168 * Don't let a 2nd user change the period underneath the 1st user.
169 * However if ddate->approx_period == 0 this is the first time we set
170 * any period, so let whoever gets here first set the period so other
171 * users who agree on the period won't fail.
172 */
173 if (ddata->user_count != 1 && ddata->approx_period) {
174 mutex_unlock(&ddata->lock);
175 return -EBUSY;
176 }
177 ddata->approx_period = state->period;
178 pwm_sifive_update_clock(ddata, clk_get_rate(ddata->clk));
179 }
180 mutex_unlock(&ddata->lock);
181
182 /*
183 * If the PWM is enabled the clk is already on. So only enable it
184 * conditionally to have it on exactly once afterwards independent of
185 * the PWM state.
186 */
187 if (!enabled) {
188 ret = clk_enable(ddata->clk);
189 if (ret) {
190 dev_err(ddata->chip.dev, "Enable clk failed\n");
191 return ret;
192 }
193 }
194
195 writel(frac, ddata->regs + PWM_SIFIVE_PWMCMP(pwm->hwpwm));
196
197 if (!state->enabled)
198 clk_disable(ddata->clk);
199
200 return 0;
201 }
202
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Hi Nylon,
kernel test robot noticed the following build warnings:
[auto build test WARNING on robh/for-next]
[also build test WARNING on thierry-reding-pwm/for-next rockchip/for-next linus/master v6.4-rc1 next-20230508]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Nylon-Chen/riscv-dts-sifive-unleashed-unmatched-Remove-PWM-controlled-LED-s-active-low-properties/20230420-173619
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20230420093457.18936-3-nylon.chen%40sifive.com
patch subject: [PATCH v3 2/2] pwm: sifive: change the PWM controlled LED algorithm
config: powerpc-randconfig-s041-20230507 (https://download.01.org/0day-ci/archive/20230508/[email protected]/config)
compiler: powerpc-linux-gcc (GCC) 12.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-39-gce1a6720-dirty
# https://github.com/intel-lab-lkp/linux/commit/f2d706bf61190a45a8f90f1f455bc943d4ac7b6e
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Nylon-Chen/riscv-dts-sifive-unleashed-unmatched-Remove-PWM-controlled-LED-s-active-low-properties/20230420-173619
git checkout f2d706bf61190a45a8f90f1f455bc943d4ac7b6e
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=powerpc olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=powerpc SHELL=/bin/bash drivers/pwm/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
sparse warnings: (new ones prefixed by >>)
>> drivers/pwm/pwm-sifive.c:157:18: sparse: sparse: incompatible types in comparison expression (different type sizes):
>> drivers/pwm/pwm-sifive.c:157:18: sparse: unsigned long long const *
>> drivers/pwm/pwm-sifive.c:157:18: sparse: unsigned int *
vim +157 drivers/pwm/pwm-sifive.c
129
130 static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
131 const struct pwm_state *state)
132 {
133 struct pwm_sifive_ddata *ddata = pwm_sifive_chip_to_ddata(chip);
134 struct pwm_state cur_state;
135 unsigned int duty_cycle, period;
136 unsigned long long num;
137 bool enabled;
138 int ret = 0;
139 u32 frac;
140
141 if (state->polarity != PWM_POLARITY_NORMAL && state->polarity != PWM_POLARITY_INVERSED)
142 return -EINVAL;
143
144 cur_state = pwm->state;
145 enabled = cur_state.enabled;
146
147 duty_cycle = state->duty_cycle;
148 if (!state->enabled)
149 duty_cycle = 0;
150
151 /*
152 * The problem of output producing mixed setting as mentioned at top,
153 * occurs here. To minimize the window for this problem, we are
154 * calculating the register values first and then writing them
155 * consecutively
156 */
> 157 period = max(state->period, ddata->approx_period);
158 num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH);
159 frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
160 frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
161 /* The hardware cannot generate a 100% duty cycle */
162 frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac;
163
164
165 mutex_lock(&ddata->lock);
166 if (state->period != ddata->approx_period) {
167 /*
168 * Don't let a 2nd user change the period underneath the 1st user.
169 * However if ddate->approx_period == 0 this is the first time we set
170 * any period, so let whoever gets here first set the period so other
171 * users who agree on the period won't fail.
172 */
173 if (ddata->user_count != 1 && ddata->approx_period) {
174 mutex_unlock(&ddata->lock);
175 return -EBUSY;
176 }
177 ddata->approx_period = state->period;
178 pwm_sifive_update_clock(ddata, clk_get_rate(ddata->clk));
179 }
180 mutex_unlock(&ddata->lock);
181
182 /*
183 * If the PWM is enabled the clk is already on. So only enable it
184 * conditionally to have it on exactly once afterwards independent of
185 * the PWM state.
186 */
187 if (!enabled) {
188 ret = clk_enable(ddata->clk);
189 if (ret) {
190 dev_err(ddata->chip.dev, "Enable clk failed\n");
191 return ret;
192 }
193 }
194
195 writel(frac, ddata->regs + PWM_SIFIVE_PWMCMP(pwm->hwpwm));
196
197 if (!state->enabled)
198 clk_disable(ddata->clk);
199
200 return 0;
201 }
202
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests