* Re: [PATCH v3 7/7] pwm: rockchip: Enable clock before calling clk_get_rate()
@ 2020-12-25 7:15 Kever Yang
2021-01-13 9:16 ` Uwe Kleine-König
0 siblings, 1 reply; 4+ messages in thread
From: Kever Yang @ 2020-12-25 7:15 UTC (permalink / raw)
To: Simon South
Cc: tpiepho, thierry.reding, u.kleine-koenig, Robin Murphy,
lee.jones, Heiko Stuebner, bbrezillon, linux-pwm,
linux-arm-kernel, linux-rockchip, David Wu, steven.liu
[-- Attachment #1: Type: text/plain, Size: 2483 bytes --]
+ David and Steven,
Hi Steven,
please help to review this patch set.
Thanks
- Kever
Simon South <simon@simonsouth.net> 于2020年12月24日周四 上午12:01写道:
> The documentation for clk_get_rate() in include/linux/clk.h states the
> function's result is valid only for a clock source that has been
> enabled. However, the Rockchip PWM driver uses this function in two places
> to query the rate of a clock without first ensuring it is enabled.
>
> Fix this by modifying rockchip_pwm_get_state() and rockchip_pwm_apply() so
> they enable a device's PWM clock before querying its rate (in the latter
> case, the querying is actually done in rockchip_pwm_config()) and disable
> the clock again before returning.
>
> Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Simon South <simon@simonsouth.net>
> ---
> drivers/pwm/pwm-rockchip.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
> index 02da7370db70..44425eeb4e81 100644
> --- a/drivers/pwm/pwm-rockchip.c
> +++ b/drivers/pwm/pwm-rockchip.c
> @@ -72,6 +72,10 @@ static void rockchip_pwm_get_state(struct pwm_chip
> *chip,
> if (ret)
> return;
>
> + ret = clk_enable(pc->clk);
> + if (ret)
> + return;
> +
> clk_rate = clk_get_rate(pc->clk);
>
> tmp = readl_relaxed(pc->base + pc->data->regs.period);
> @@ -90,6 +94,7 @@ static void rockchip_pwm_get_state(struct pwm_chip *chip,
> else
> state->polarity = PWM_POLARITY_NORMAL;
>
> + clk_disable(pc->clk);
> clk_disable(pc->pclk);
> }
>
> @@ -189,6 +194,10 @@ static int rockchip_pwm_apply(struct pwm_chip *chip,
> struct pwm_device *pwm,
> if (ret)
> return ret;
>
> + ret = clk_enable(pc->clk);
> + if (ret)
> + return ret;
> +
> pwm_get_state(pwm, &curstate);
> enabled = curstate.enabled;
>
> @@ -208,6 +217,7 @@ static int rockchip_pwm_apply(struct pwm_chip *chip,
> struct pwm_device *pwm,
> }
>
> out:
> + clk_disable(pc->clk);
> clk_disable(pc->pclk);
>
> return ret;
> --
> 2.29.2
>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>
[-- Attachment #2: Type: text/html, Size: 3476 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3 7/7] pwm: rockchip: Enable clock before calling clk_get_rate()
2020-12-25 7:15 [PATCH v3 7/7] pwm: rockchip: Enable clock before calling clk_get_rate() Kever Yang
@ 2021-01-13 9:16 ` Uwe Kleine-König
0 siblings, 0 replies; 4+ messages in thread
From: Uwe Kleine-König @ 2021-01-13 9:16 UTC (permalink / raw)
To: Kever Yang; +Cc: Simon South, linux-pwm, David Wu, steven.liu
[-- Attachment #1: Type: text/plain, Size: 489 bytes --]
Hi Kever,
[stripped recipients a bit]
On Fri, Dec 25, 2020 at 03:15:12PM +0800, Kever Yang wrote:
> + David and Steven,
>
> Hi Steven,
> please help to review this patch set.
I wonder if a private message would have been enough instead of seven
identical mails to a massive set of people.
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] 4+ messages in thread
* [PATCH v3 0/7] pwm: rockchip: Eliminate potential race condition when probing
@ 2020-12-23 16:01 Simon South
2020-12-23 16:01 ` [PATCH v3 7/7] pwm: rockchip: Enable clock before calling clk_get_rate() Simon South
0 siblings, 1 reply; 4+ messages in thread
From: Simon South @ 2020-12-23 16:01 UTC (permalink / raw)
To: tpiepho, thierry.reding, u.kleine-koenig, robin.murphy,
lee.jones, heiko, bbrezillon, linux-pwm, linux-arm-kernel,
linux-rockchip
Cc: simon
This patch series aims to eliminate the race condition Trent Piepho
identified[0] in the Rockchip PWM driver's rockchip_pwm_probe()
function, by moving code that checks whether a device is enabled ahead
of the code that registers it via pwmchip_add().
It has grown to include a number of other small fixes and improvements
to the driver. It now also
- Fixes a potential kernel hang introduced by my earlier commit
457f74abbed0 ("pwm: rockchip: Keep enabled PWMs running while
probing") by ensuring a device's APB clock is enabled before its
registers are accessed;
- Removes a superfluous call to clk_unprepare() that could result in
warnings from the kernel;
- Clarifies the driver's error messages by replacing "bus clk" with
"PWM clk";
- Removes the now-unneeded goto targets from rockchip_pwm_probe();
- Tries to improve rockchip_pwm_probe() by having it enable the signal
clock of only PWM devices that are already running; and
- Ensures the driver enables a clock before querying its rate with
clk_get_rate(), as stated as a requirement in that function's
documentation.
The first patch ("Enable APB clock...") is unchanged from version 2.
New in version 3 are
- Finer patch granularity, with patches 2 and 5 added to clarify
changes included with others in v2;
- A rewritten patch 6 ("Enable PWM clock...") with a smaller change
and the use of if...else in place of a ternary operator;
- Patches 3 and 7 with fixes suggested by Robin Murphy and Uwe
Kleine-König; and
- Rewritten and (hopefully) more accurate commit messages.
I've tested these changes on my (RK3399-based) Pinebook Pro with its
screen backlight enabled by U-Boot and each one appears to work fine.
I'd (still) be grateful for help with testing on other devices,
particularly those with SoCs like the RK3328 that use separate bus and
signal clocks for their PWM devices. (My ROCK64 uses its PWM-output
pins for other purposes and wasn't of help here.)
[0] https://www.spinics.net/lists/linux-pwm/msg14611.html
--
Simon South
simon@simonsouth.net
Simon South (7):
pwm: rockchip: Enable APB clock during register access while probing
pwm: rockchip: rockchip_pwm_probe(): Remove superfluous
clk_unprepare()
pwm: rockchip: Replace "bus clk" with "PWM clk"
pwm: rockchip: Eliminate potential race condition when probing
pwm: rockchip: rockchip_pwm_probe(): Remove unneeded goto target
pwm: rockchip: Enable PWM clock of probed device only if running
pwm: rockchip: Enable clock before calling clk_get_rate()
drivers/pwm/pwm-rockchip.c | 64 ++++++++++++++++++++++++--------------
1 file changed, 40 insertions(+), 24 deletions(-)
--
2.29.2
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v3 7/7] pwm: rockchip: Enable clock before calling clk_get_rate()
2020-12-23 16:01 [PATCH v3 0/7] pwm: rockchip: Eliminate potential race condition when probing Simon South
@ 2020-12-23 16:01 ` Simon South
2021-01-13 7:54 ` Uwe Kleine-König
0 siblings, 1 reply; 4+ messages in thread
From: Simon South @ 2020-12-23 16:01 UTC (permalink / raw)
To: tpiepho, thierry.reding, u.kleine-koenig, robin.murphy,
lee.jones, heiko, bbrezillon, linux-pwm, linux-arm-kernel,
linux-rockchip
Cc: simon
The documentation for clk_get_rate() in include/linux/clk.h states the
function's result is valid only for a clock source that has been
enabled. However, the Rockchip PWM driver uses this function in two places
to query the rate of a clock without first ensuring it is enabled.
Fix this by modifying rockchip_pwm_get_state() and rockchip_pwm_apply() so
they enable a device's PWM clock before querying its rate (in the latter
case, the querying is actually done in rockchip_pwm_config()) and disable
the clock again before returning.
Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Simon South <simon@simonsouth.net>
---
drivers/pwm/pwm-rockchip.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index 02da7370db70..44425eeb4e81 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -72,6 +72,10 @@ static void rockchip_pwm_get_state(struct pwm_chip *chip,
if (ret)
return;
+ ret = clk_enable(pc->clk);
+ if (ret)
+ return;
+
clk_rate = clk_get_rate(pc->clk);
tmp = readl_relaxed(pc->base + pc->data->regs.period);
@@ -90,6 +94,7 @@ static void rockchip_pwm_get_state(struct pwm_chip *chip,
else
state->polarity = PWM_POLARITY_NORMAL;
+ clk_disable(pc->clk);
clk_disable(pc->pclk);
}
@@ -189,6 +194,10 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
if (ret)
return ret;
+ ret = clk_enable(pc->clk);
+ if (ret)
+ return ret;
+
pwm_get_state(pwm, &curstate);
enabled = curstate.enabled;
@@ -208,6 +217,7 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
}
out:
+ clk_disable(pc->clk);
clk_disable(pc->pclk);
return ret;
--
2.29.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v3 7/7] pwm: rockchip: Enable clock before calling clk_get_rate()
2020-12-23 16:01 ` [PATCH v3 7/7] pwm: rockchip: Enable clock before calling clk_get_rate() Simon South
@ 2021-01-13 7:54 ` Uwe Kleine-König
0 siblings, 0 replies; 4+ messages in thread
From: Uwe Kleine-König @ 2021-01-13 7:54 UTC (permalink / raw)
To: Simon South
Cc: tpiepho, thierry.reding, robin.murphy, lee.jones, heiko,
bbrezillon, linux-pwm, linux-arm-kernel, linux-rockchip
[-- Attachment #1: Type: text/plain, Size: 993 bytes --]
On Wed, Dec 23, 2020 at 11:01:09AM -0500, Simon South wrote:
> The documentation for clk_get_rate() in include/linux/clk.h states the
> function's result is valid only for a clock source that has been
> enabled. However, the Rockchip PWM driver uses this function in two places
> to query the rate of a clock without first ensuring it is enabled.
>
> Fix this by modifying rockchip_pwm_get_state() and rockchip_pwm_apply() so
> they enable a device's PWM clock before querying its rate (in the latter
> case, the querying is actually done in rockchip_pwm_config()) and disable
> the clock again before returning.
>
> Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Simon South <simon@simonsouth.net>
Reviewed-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] 4+ messages in thread
end of thread, other threads:[~2021-01-13 9:17 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-25 7:15 [PATCH v3 7/7] pwm: rockchip: Enable clock before calling clk_get_rate() Kever Yang
2021-01-13 9:16 ` Uwe Kleine-König
-- strict thread matches above, loose matches on Subject: below --
2020-12-23 16:01 [PATCH v3 0/7] pwm: rockchip: Eliminate potential race condition when probing Simon South
2020-12-23 16:01 ` [PATCH v3 7/7] pwm: rockchip: Enable clock before calling clk_get_rate() Simon South
2021-01-13 7:54 ` Uwe Kleine-König
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).