Skip to contentSkip to conversation

pwm-sun4i: convert "next_period" to local variable

PATCH In review Max Kellermann 4 participants 12 messages Jan 25, 2022 – Apr 22, 2022

Subsystems (3)
Stable branch Arm/Allwinner sunXi SoC support PWM
lore
Share & tools
lore

Max Kellermann

author
#0 lore raw

PATCH 1/3 pwm-sun4i: convert "next_period" to local variable

Its value is calculated in sun4i_pwm_apply() and is used only there.

Cc [email protected]
Signed-off-by Max Kellermann <[email protected]>
---
 drivers/pwm/pwm-sun4i.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
drivers/pwm/pwm-sun4i.c +4 -5
index 91ca67651abd..c7c564a6bf36 100644
@@ -89,7 +89,6 @@ struct sun4i_pwm_chip {
 	void __iomem *base;
 	spinlock_t ctrl_lock;
 	const struct sun4i_pwm_data *data;
-	unsigned long next_period[2];
 };
 
 static inline struct sun4i_pwm_chip *to_sun4i_pwm_chip(struct pwm_chip *chip)

@@ -237,6 +236,7 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	int ret;
 	unsigned int delay_us, prescaler = 0;
 	unsigned long now;
+	unsigned long next_period;
 	bool bypass;
 
 	pwm_get_state(pwm, &cstate);

@@ -284,7 +284,7 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	val = (duty & PWM_DTY_MASK) | PWM_PRD(period);
 	sun4i_pwm_writel(sun4i_pwm, val, PWM_CH_PRD(pwm->hwpwm));
-	sun4i_pwm->next_period[pwm->hwpwm] = jiffies +
+	next_period = jiffies +
 		nsecs_to_jiffies(cstate.period + 1000);
 
 	if (state->polarity != PWM_POLARITY_NORMAL)

@@ -306,9 +306,8 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	/* We need a full period to elapse before disabling the channel. */
 	now = jiffies;
-	if (time_before(now, sun4i_pwm->next_period[pwm->hwpwm])) {
-		delay_us = jiffies_to_usecs(sun4i_pwm->next_period[pwm->hwpwm] -
-					   now);
+	if (time_before(now, next_period)) {
+		delay_us = jiffies_to_usecs(next_period - now);
 		if ((delay_us / 500) > MAX_UDELAY_MS)
 			msleep(delay_us / 1000 + 1);
 		else
Signature · 1 line
2.34.0
author
#1 lore raw

PATCH 2/3 pwm-sun4i: calculate "delay_jiffies" directly, eliminate absolute time

Basically this code did "jiffies + period - jiffies", and we can simply eliminate the "jiffies" time stamp here.

Cc [email protected]
Signed-off-by Max Kellermann <[email protected]>
---
 drivers/pwm/pwm-sun4i.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)
drivers/pwm/pwm-sun4i.c +7 -12
index c7c564a6bf36..b44deececb8b 100644
@@ -234,9 +234,8 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	struct pwm_state cstate;
 	u32 ctrl, duty = 0, period = 0, val;
 	int ret;
+	unsigned int delay_jiffies;
 	unsigned int delay_us, prescaler = 0;
-	unsigned long now;
-	unsigned long next_period;
 	bool bypass;
 
 	pwm_get_state(pwm, &cstate);

@@ -284,8 +283,6 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	val = (duty & PWM_DTY_MASK) | PWM_PRD(period);
 	sun4i_pwm_writel(sun4i_pwm, val, PWM_CH_PRD(pwm->hwpwm));
-	next_period = jiffies +
-		nsecs_to_jiffies(cstate.period + 1000);
 
 	if (state->polarity != PWM_POLARITY_NORMAL)
 		ctrl &= ~BIT_CH(PWM_ACT_STATE, pwm->hwpwm);

@@ -305,14 +302,12 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		return 0;
 
 	/* We need a full period to elapse before disabling the channel. */
-	now = jiffies;
-	if (time_before(now, next_period)) {
-		delay_us = jiffies_to_usecs(next_period - now);
-		if ((delay_us / 500) > MAX_UDELAY_MS)
-			msleep(delay_us / 1000 + 1);
-		else
-			usleep_range(delay_us, delay_us * 2);
-	}
+	delay_jiffies = nsecs_to_jiffies(cstate.period + 1000);
+	delay_us = jiffies_to_usecs(delay_jiffies);
+	if ((delay_us / 500) > MAX_UDELAY_MS)
+		msleep(delay_us / 1000 + 1);
+	else
+		usleep_range(delay_us, delay_us * 2);
 
 	spin_lock(&sun4i_pwm->ctrl_lock);
 	ctrl = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
Signature · 1 line
2.34.0
Acked-by on v1 2/3
#2 lore raw

Re: [PATCH 2/3] pwm-sun4i: calculate "delay_jiffies" directly, eliminate absolute time

Quoted 4 lines · Max Kellermann

Basically this code did "jiffies + period - jiffies", and we can simply eliminate the "jiffies" time stamp here.

Cc [email protected]

I don't see how this is relevant for stable, but the change looks fine:

Acked-by Uwe Kleine-König <[email protected]>

Thanks
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
author
#3 lore raw

PATCH 3/3 pwm-sun4i: calculate the delay without rounding down to jiffies

In reply to Max Kellermann

This fixes a problem that was supposed to be addressed by commit 6eefb79d6f5bc ("pwm: sun4i: Remove erroneous else branch") - backlight could not be switched off on some Allwinner A20. The commit was correct, but was not a reliable fix for the problem, which was timing related.

The real problem for the backlight switching problem was that sleeping for a full period did not work, because delay_us is always zero.

It is zero because the period (plus 1 microsecond) is rounded down to the next "jiffies", but the period is less than one jiffy.

On my Cubieboard 2, the period is 5ms, and 1 jiffy (at the default HZ=100) is 10ms, so nsecs_to_jiffies(10ms+1us)=0.

The roundtrip from nanoseconds to jiffies and back to microseconds is an unnecessary loss of precision; always rounding down (via nsecs_to_jiffies()) then causes the breakage.

This patch eliminates this roundtrip, and directly converts from nanoseconds to microseconds (for usleep_range()), using DIV_ROUND_UP_ULL() to force rounding up. This way, the sleep time is never zero, and after the sleep, we are guaranteed to be in a different period, and the device is ready for another control command for sure.

Cc [email protected]
Signed-off-by Max Kellermann <[email protected]>
---
 drivers/pwm/pwm-sun4i.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/pwm/pwm-sun4i.c +1 -3
index b44deececb8b..8527a655034c 100644
@@ -234,7 +234,6 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	struct pwm_state cstate;
 	u32 ctrl, duty = 0, period = 0, val;
 	int ret;
-	unsigned int delay_jiffies;
 	unsigned int delay_us, prescaler = 0;
 	bool bypass;
 

@@ -302,8 +301,7 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		return 0;
 
 	/* We need a full period to elapse before disabling the channel. */
-	delay_jiffies = nsecs_to_jiffies(cstate.period + 1000);
-	delay_us = jiffies_to_usecs(delay_jiffies);
+	delay_us = DIV_ROUND_UP_ULL(cstate.period, NSEC_PER_USEC);
 	if ((delay_us / 500) > MAX_UDELAY_MS)
 		msleep(delay_us / 1000 + 1);
 	else
Signature · 1 line
2.34.0
#4 lore raw

Re: [PATCH 3/3] pwm-sun4i: calculate the delay without rounding down to jiffies

Quoted 26 lines · Max Kellermann

This fixes a problem that was supposed to be addressed by commit 6eefb79d6f5bc ("pwm: sun4i: Remove erroneous else branch") - backlight could not be switched off on some Allwinner A20. The commit was correct, but was not a reliable fix for the problem, which was timing related.

The real problem for the backlight switching problem was that sleeping for a full period did not work, because delay_us is always zero.

It is zero because the period (plus 1 microsecond) is rounded down to the next "jiffies", but the period is less than one jiffy.

On my Cubieboard 2, the period is 5ms, and 1 jiffy (at the default HZ=100) is 10ms, so nsecs_to_jiffies(10ms+1us)=0.

The roundtrip from nanoseconds to jiffies and back to microseconds is an unnecessary loss of precision; always rounding down (via nsecs_to_jiffies()) then causes the breakage.

This patch eliminates this roundtrip, and directly converts from nanoseconds to microseconds (for usleep_range()), using DIV_ROUND_UP_ULL() to force rounding up. This way, the sleep time is never zero, and after the sleep, we are guaranteed to be in a different period, and the device is ready for another control command for sure.

Quoted 2 lines
Cc [email protected]
Signed-off-by Max Kellermann <[email protected]>

Sounds reasonable

Acked-by; Uwe Kleine-König <[email protected]>

Best regards
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
Acked-by
#5 lore raw

Re: [PATCH 3/3] pwm-sun4i: calculate the delay without rounding down to jiffies

Quoted 27 lines · Uwe Kleine-König
Quoted 26 lines · Max Kellermann

This fixes a problem that was supposed to be addressed by commit 6eefb79d6f5bc ("pwm: sun4i: Remove erroneous else branch") - backlight could not be switched off on some Allwinner A20. The commit was correct, but was not a reliable fix for the problem, which was timing related.

The real problem for the backlight switching problem was that sleeping for a full period did not work, because delay_us is always zero.

It is zero because the period (plus 1 microsecond) is rounded down to the next "jiffies", but the period is less than one jiffy.

On my Cubieboard 2, the period is 5ms, and 1 jiffy (at the default HZ=100) is 10ms, so nsecs_to_jiffies(10ms+1us)=0.

The roundtrip from nanoseconds to jiffies and back to microseconds is an unnecessary loss of precision; always rounding down (via nsecs_to_jiffies()) then causes the breakage.

This patch eliminates this roundtrip, and directly converts from nanoseconds to microseconds (for usleep_range()), using DIV_ROUND_UP_ULL() to force rounding up. This way, the sleep time is never zero, and after the sleep, we are guaranteed to be in a different period, and the device is ready for another control command for sure.

Quoted 6 lines
Quoted 2 lines
Cc [email protected]
Signed-off-by Max Kellermann <[email protected]>

Sounds reasonable

Acked-by; Uwe Kleine-König <[email protected]>

/me had problems with is keyboard, the Shift-key didn't work and so I typed a ; instead a :. To make patchwork pick up my tag, I'll repeat it here:

Acked-by Uwe Kleine-König <[email protected]>

Thanks
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
Reviewed-by on v1 1/3
#6 lore raw
In reply to Max Kellermann

Hello,

Quoted 3 lines · Max Kellermann

Its value is calculated in sun4i_pwm_apply() and is used only there.

Cc [email protected]

I think I'd drop this. This isn't a fix worth on it's own to be backported and if this is needed for one of the next patches, the stable maintainers will notice themselves (and it might be worth to shuffle this series to make the fixes come first).

Quoted 1 line
Signed-off-by Max Kellermann <[email protected]>

Other than that, LGTM:

Reviewed-by Uwe Kleine-König <[email protected]>

Thanks
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
#7 lore raw
Quoted 4 lines · Uwe Kleine-König

I think I'd drop this. This isn't a fix worth on it's own to be backported and if this is needed for one of the next patches, the stable maintainers will notice themselves (and it might be worth to shuffle this series to make the fixes come first).

The first two patches are preparation for the third patch, which fixes the actual bug.

Of course, I could have done everything in one patch, but I thought splitting the first two out makes review easier. This way, every step is almost trivial.

If you want me to fold the three patches into one, I can do that. But I can't reorder them (or backport only the bug fix to stable).

Max

#8 lore raw
Quoted 9 lines · Max Kellermann
Quoted 4 lines · Uwe Kleine-König

I think I'd drop this. This isn't a fix worth on it's own to be backported and if this is needed for one of the next patches, the stable maintainers will notice themselves (and it might be worth to shuffle this series to make the fixes come first).

The first two patches are preparation for the third patch, which fixes the actual bug.

Quoted 6 lines

Of course, I could have done everything in one patch, but I thought splitting the first two out makes review easier. This way, every step is almost trivial.

If you want me to fold the three patches into one, I can do that. But I can't reorder them (or backport only the bug fix to stable).

That sounds fine. Note my statement "I'd drop this" only refers to the
Cc: stable line.

Best regards
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
#9 lore raw
In reply to Uwe Kleine-König
Quoted 12 lines · Uwe Kleine-König

Hello,

Quoted 3 lines · Max Kellermann

Its value is calculated in sun4i_pwm_apply() and is used only there.

Cc [email protected]

I think I'd drop this. This isn't a fix worth on it's own to be backported and if this is needed for one of the next patches, the stable maintainers will notice themselves (and it might be worth to shuffle this series to make the fixes come first).

Quoted 5 lines
Quoted 1 line
Signed-off-by Max Kellermann <[email protected]>

Other than that, LGTM:

Reviewed-by Uwe Kleine-König <[email protected]>

Does that apply to patches 2 & 3 as well?

Thierry

#10 lore raw
Quoted 13 lines · Thierry Reding
Quoted 12 lines · Uwe Kleine-König

Hello,

Quoted 3 lines · Max Kellermann

Its value is calculated in sun4i_pwm_apply() and is used only there.

Cc [email protected]

I think I'd drop this. This isn't a fix worth on it's own to be backported and if this is needed for one of the next patches, the stable maintainers will notice themselves (and it might be worth to shuffle this series to make the fixes come first).

Quoted 7 lines
Quoted 5 lines
Quoted 1 line
Signed-off-by Max Kellermann <[email protected]>

Other than that, LGTM:

Reviewed-by Uwe Kleine-König <[email protected]>

Does that apply to patches 2 & 3 as well?

No, at that time I only looked at patch 1.

I just looked at 2 and 3 and will reply to them individually.

Best regards
Uwe

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