* [PATCH 2/3] pwm-sun4i: calculate "delay_jiffies" directly, eliminate absolute time
2022-01-25 12:34 [PATCH 1/3] pwm-sun4i: convert "next_period" to local variable Max Kellermann
@ 2022-01-25 12:34 ` Max Kellermann
2022-02-24 16:50 ` Uwe Kleine-König
2022-01-25 12:34 ` [PATCH 3/3] pwm-sun4i: calculate the delay without rounding down to jiffies Max Kellermann
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Max Kellermann @ 2022-01-25 12:34 UTC (permalink / raw)
To: linux-pwm, thierry.reding, u.kleine-koenig, lee.jones
Cc: linux-kernel, andrey, Max Kellermann, stable
Basically this code did "jiffies + period - jiffies", and we can
simply eliminate the "jiffies" time stamp here.
Cc: stable@vger.kernel.org
Signed-off-by: Max Kellermann <max.kellermann@gmail.com>
---
drivers/pwm/pwm-sun4i.c | 19 +++++++------------
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index c7c564a6bf36..b44deececb8b 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -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);
--
2.34.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] pwm-sun4i: calculate "delay_jiffies" directly, eliminate absolute time
2022-01-25 12:34 ` [PATCH 2/3] pwm-sun4i: calculate "delay_jiffies" directly, eliminate absolute time Max Kellermann
@ 2022-02-24 16:50 ` Uwe Kleine-König
0 siblings, 0 replies; 12+ messages in thread
From: Uwe Kleine-König @ 2022-02-24 16:50 UTC (permalink / raw)
To: Max Kellermann
Cc: linux-pwm, thierry.reding, lee.jones, linux-kernel, andrey, stable
[-- Attachment #1: Type: text/plain, Size: 530 bytes --]
On Tue, Jan 25, 2022 at 01:34:28PM +0100, Max Kellermann wrote:
> Basically this code did "jiffies + period - jiffies", and we can
> simply eliminate the "jiffies" time stamp here.
>
> Cc: stable@vger.kernel.org
I don't see how this is relevant for stable, but the change looks fine:
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Thanks
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] pwm-sun4i: calculate the delay without rounding down to jiffies
2022-01-25 12:34 [PATCH 1/3] pwm-sun4i: convert "next_period" to local variable Max Kellermann
2022-01-25 12:34 ` [PATCH 2/3] pwm-sun4i: calculate "delay_jiffies" directly, eliminate absolute time Max Kellermann
@ 2022-01-25 12:34 ` Max Kellermann
2022-02-24 16:51 ` Uwe Kleine-König
2022-01-25 14:31 ` [PATCH 1/3] pwm-sun4i: convert "next_period" to local variable Uwe Kleine-König
2022-04-22 15:47 ` Thierry Reding
3 siblings, 1 reply; 12+ messages in thread
From: Max Kellermann @ 2022-01-25 12:34 UTC (permalink / raw)
To: linux-pwm, thierry.reding, u.kleine-koenig, lee.jones
Cc: linux-kernel, andrey, Max Kellermann, stable
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: stable@vger.kernel.org
Signed-off-by: Max Kellermann <max.kellermann@gmail.com>
---
drivers/pwm/pwm-sun4i.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index b44deececb8b..8527a655034c 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -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
--
2.34.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] pwm-sun4i: calculate the delay without rounding down to jiffies
2022-01-25 12:34 ` [PATCH 3/3] pwm-sun4i: calculate the delay without rounding down to jiffies Max Kellermann
@ 2022-02-24 16:51 ` Uwe Kleine-König
2022-03-03 21:56 ` Uwe Kleine-König
0 siblings, 1 reply; 12+ messages in thread
From: Uwe Kleine-König @ 2022-02-24 16:51 UTC (permalink / raw)
To: Max Kellermann
Cc: linux-pwm, thierry.reding, lee.jones, linux-kernel, andrey, stable
[-- Attachment #1: Type: text/plain, Size: 1674 bytes --]
On Tue, Jan 25, 2022 at 01:34:29PM +0100, Max Kellermann wrote:
> 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: stable@vger.kernel.org
> Signed-off-by: Max Kellermann <max.kellermann@gmail.com>
Sounds reasonable
Acked-by; Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] pwm-sun4i: calculate the delay without rounding down to jiffies
2022-02-24 16:51 ` Uwe Kleine-König
@ 2022-03-03 21:56 ` Uwe Kleine-König
0 siblings, 0 replies; 12+ messages in thread
From: Uwe Kleine-König @ 2022-03-03 21:56 UTC (permalink / raw)
To: Max Kellermann
Cc: linux-pwm, thierry.reding, lee.jones, linux-kernel, andrey, stable
[-- Attachment #1: Type: text/plain, Size: 2017 bytes --]
On Thu, Feb 24, 2022 at 05:51:35PM +0100, Uwe Kleine-König wrote:
> On Tue, Jan 25, 2022 at 01:34:29PM +0100, Max Kellermann wrote:
> > 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: stable@vger.kernel.org
> > Signed-off-by: Max Kellermann <max.kellermann@gmail.com>
>
> Sounds reasonable
>
> Acked-by; Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
/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 <u.kleine-koenig@pengutronix.de>
Thanks
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] pwm-sun4i: convert "next_period" to local variable
2022-01-25 12:34 [PATCH 1/3] pwm-sun4i: convert "next_period" to local variable Max Kellermann
2022-01-25 12:34 ` [PATCH 2/3] pwm-sun4i: calculate "delay_jiffies" directly, eliminate absolute time Max Kellermann
2022-01-25 12:34 ` [PATCH 3/3] pwm-sun4i: calculate the delay without rounding down to jiffies Max Kellermann
@ 2022-01-25 14:31 ` Uwe Kleine-König
2022-01-25 14:39 ` Max Kellermann
2022-02-24 13:04 ` Thierry Reding
2022-04-22 15:47 ` Thierry Reding
3 siblings, 2 replies; 12+ messages in thread
From: Uwe Kleine-König @ 2022-01-25 14:31 UTC (permalink / raw)
To: Max Kellermann
Cc: linux-pwm, thierry.reding, lee.jones, linux-kernel, andrey, stable
[-- Attachment #1: Type: text/plain, Size: 764 bytes --]
Hello,
On Tue, Jan 25, 2022 at 01:34:27PM +0100, Max Kellermann wrote:
> Its value is calculated in sun4i_pwm_apply() and is used only there.
>
> Cc: stable@vger.kernel.org
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).
> Signed-off-by: Max Kellermann <max.kellermann@gmail.com>
Other than that, LGTM:
Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Thanks
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] pwm-sun4i: convert "next_period" to local variable
2022-01-25 14:31 ` [PATCH 1/3] pwm-sun4i: convert "next_period" to local variable Uwe Kleine-König
@ 2022-01-25 14:39 ` Max Kellermann
2022-01-25 14:43 ` Uwe Kleine-König
2022-02-24 13:04 ` Thierry Reding
1 sibling, 1 reply; 12+ messages in thread
From: Max Kellermann @ 2022-01-25 14:39 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: linux-pwm, thierry.reding, lee.jones, linux-kernel, andrey, stable
On 2022/01/25 15:31, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] pwm-sun4i: convert "next_period" to local variable
2022-01-25 14:39 ` Max Kellermann
@ 2022-01-25 14:43 ` Uwe Kleine-König
0 siblings, 0 replies; 12+ messages in thread
From: Uwe Kleine-König @ 2022-01-25 14:43 UTC (permalink / raw)
To: linux-pwm, thierry.reding, lee.jones, linux-kernel, andrey, stable
[-- Attachment #1: Type: text/plain, Size: 1099 bytes --]
On Tue, Jan 25, 2022 at 03:39:16PM +0100, Max Kellermann wrote:
> On 2022/01/25 15:31, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> > 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).
That sounds fine. Note my statement "I'd drop this" only refers to the
Cc: stable line.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] pwm-sun4i: convert "next_period" to local variable
2022-01-25 14:31 ` [PATCH 1/3] pwm-sun4i: convert "next_period" to local variable Uwe Kleine-König
2022-01-25 14:39 ` Max Kellermann
@ 2022-02-24 13:04 ` Thierry Reding
2022-02-24 16:49 ` Uwe Kleine-König
1 sibling, 1 reply; 12+ messages in thread
From: Thierry Reding @ 2022-02-24 13:04 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Max Kellermann, linux-pwm, lee.jones, linux-kernel, andrey, stable
[-- Attachment #1: Type: text/plain, Size: 749 bytes --]
On Tue, Jan 25, 2022 at 03:31:58PM +0100, Uwe Kleine-König wrote:
> Hello,
>
> On Tue, Jan 25, 2022 at 01:34:27PM +0100, Max Kellermann wrote:
> > Its value is calculated in sun4i_pwm_apply() and is used only there.
> >
> > Cc: stable@vger.kernel.org
>
> 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).
>
> > Signed-off-by: Max Kellermann <max.kellermann@gmail.com>
>
> Other than that, LGTM:
>
> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Does that apply to patches 2 & 3 as well?
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] pwm-sun4i: convert "next_period" to local variable
2022-02-24 13:04 ` Thierry Reding
@ 2022-02-24 16:49 ` Uwe Kleine-König
0 siblings, 0 replies; 12+ messages in thread
From: Uwe Kleine-König @ 2022-02-24 16:49 UTC (permalink / raw)
To: Thierry Reding
Cc: Max Kellermann, linux-pwm, lee.jones, linux-kernel, andrey, stable
[-- Attachment #1: Type: text/plain, Size: 1132 bytes --]
On Thu, Feb 24, 2022 at 02:04:23PM +0100, Thierry Reding wrote:
> On Tue, Jan 25, 2022 at 03:31:58PM +0100, Uwe Kleine-König wrote:
> > Hello,
> >
> > On Tue, Jan 25, 2022 at 01:34:27PM +0100, Max Kellermann wrote:
> > > Its value is calculated in sun4i_pwm_apply() and is used only there.
> > >
> > > Cc: stable@vger.kernel.org
> >
> > 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).
> >
> > > Signed-off-by: Max Kellermann <max.kellermann@gmail.com>
> >
> > Other than that, LGTM:
> >
> > Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>
> 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
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] pwm-sun4i: convert "next_period" to local variable
2022-01-25 12:34 [PATCH 1/3] pwm-sun4i: convert "next_period" to local variable Max Kellermann
` (2 preceding siblings ...)
2022-01-25 14:31 ` [PATCH 1/3] pwm-sun4i: convert "next_period" to local variable Uwe Kleine-König
@ 2022-04-22 15:47 ` Thierry Reding
3 siblings, 0 replies; 12+ messages in thread
From: Thierry Reding @ 2022-04-22 15:47 UTC (permalink / raw)
To: Max Kellermann
Cc: linux-pwm, u.kleine-koenig, lee.jones, linux-kernel, andrey, stable
[-- Attachment #1: Type: text/plain, Size: 424 bytes --]
On Tue, Jan 25, 2022 at 01:34:27PM +0100, Max Kellermann wrote:
> Its value is calculated in sun4i_pwm_apply() and is used only there.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Max Kellermann <max.kellermann@gmail.com>
> ---
> drivers/pwm/pwm-sun4i.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
Patches applied, though I dropped the Cc: stable on patches 1 and 2.
Thanks,
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread