| Message ID | 20231024101902.6689-3-nylon.chen@sifive.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Change PWM-controlled LED pin active mode and algorithm | expand |
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 >
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
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 >
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
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/ |
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
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);