2023-10-03 05:51:07

by Ivaylo Dimitrov

[permalink] [raw]
Subject: [PATCH] drivers/clocksource/timer-ti-dm: Don't call clk_get_rate() in stop function

clk_get_rate() might sleep, and that prevents dm-timer based PWM from being
used from atomic context.

Fix that by getting fclk rate in probe() and using a notifier in case rate
changes.

Fixes: af04aa856e93 ("ARM: OMAP: Move dmtimer driver out of plat-omap to drivers under clocksource")
Signed-off-by: Ivaylo Dimitrov <[email protected]>
---
drivers/clocksource/timer-ti-dm.c | 36 ++++++++++++++++++++++++++++--------
1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/drivers/clocksource/timer-ti-dm.c b/drivers/clocksource/timer-ti-dm.c
index 09ab29c..5f60f6b 100644
--- a/drivers/clocksource/timer-ti-dm.c
+++ b/drivers/clocksource/timer-ti-dm.c
@@ -140,6 +140,8 @@ struct dmtimer {
struct platform_device *pdev;
struct list_head node;
struct notifier_block nb;
+ struct notifier_block fclk_nb;
+ unsigned long fclk_rate;
};

static u32 omap_reserved_systimers;
@@ -253,8 +255,7 @@ static inline void __omap_dm_timer_enable_posted(struct dmtimer *timer)
timer->posted = OMAP_TIMER_POSTED;
}

-static inline void __omap_dm_timer_stop(struct dmtimer *timer,
- unsigned long rate)
+static inline void __omap_dm_timer_stop(struct dmtimer *timer)
{
u32 l;

@@ -269,7 +270,7 @@ static inline void __omap_dm_timer_stop(struct dmtimer *timer,
* Wait for functional clock period x 3.5 to make sure that
* timer is stopped
*/
- udelay(3500000 / rate + 1);
+ udelay(3500000 / timer->fclk_rate + 1);
#endif
}

@@ -348,6 +349,21 @@ static int omap_timer_context_notifier(struct notifier_block *nb,
return NOTIFY_OK;
}

+static int omap_timer_fclk_notifier(struct notifier_block *nb,
+ unsigned long event, void *data)
+{
+ struct clk_notifier_data *clk_data = data;
+ struct dmtimer *timer = container_of(nb, struct dmtimer, fclk_nb);
+
+ switch (event) {
+ case POST_RATE_CHANGE:
+ timer->fclk_rate = clk_data->new_rate;
+ return NOTIFY_OK;
+ default:
+ return NOTIFY_DONE;
+ }
+}
+
static int omap_dm_timer_reset(struct dmtimer *timer)
{
u32 l, timeout = 100000;
@@ -754,7 +770,6 @@ static int omap_dm_timer_stop(struct omap_dm_timer *cookie)
{
struct dmtimer *timer;
struct device *dev;
- unsigned long rate = 0;

timer = to_dmtimer(cookie);
if (unlikely(!timer))
@@ -762,10 +777,7 @@ static int omap_dm_timer_stop(struct omap_dm_timer *cookie)

dev = &timer->pdev->dev;

- if (!timer->omap1)
- rate = clk_get_rate(timer->fclk);
-
- __omap_dm_timer_stop(timer, rate);
+ __omap_dm_timer_stop(timer);

pm_runtime_put_sync(dev);

@@ -1124,6 +1136,14 @@ static int omap_dm_timer_probe(struct platform_device *pdev)
timer->fclk = devm_clk_get(dev, "fck");
if (IS_ERR(timer->fclk))
return PTR_ERR(timer->fclk);
+
+ timer->fclk_nb.notifier_call = omap_timer_fclk_notifier;
+ ret = devm_clk_notifier_register(dev, timer->fclk,
+ &timer->fclk_nb);
+ if (ret)
+ return ret;
+
+ timer->fclk_rate = clk_get_rate(timer->fclk);
} else {
timer->fclk = ERR_PTR(-ENODEV);
}
--
1.9.1


2023-10-03 07:57:20

by Tony Lindgren

[permalink] [raw]
Subject: Re: [PATCH] drivers/clocksource/timer-ti-dm: Don't call clk_get_rate() in stop function

* Ivaylo Dimitrov <[email protected]> [231003 05:50]:
> clk_get_rate() might sleep, and that prevents dm-timer based PWM from being
> used from atomic context.
>
> Fix that by getting fclk rate in probe() and using a notifier in case rate
> changes.

Makes sense to me:

Reviewed-by: Tony Lindgren <[email protected]>

Subject: [tip: timers/core] drivers/clocksource/timer-ti-dm: Don't call clk_get_rate() in stop function

The following commit has been merged into the timers/core branch of tip:

Commit-ID: 12590d4d0e331d3cb9e6b3494515cd61c8a6624e
Gitweb: https://git.kernel.org/tip/12590d4d0e331d3cb9e6b3494515cd61c8a6624e
Author: Ivaylo Dimitrov <[email protected]>
AuthorDate: Tue, 03 Oct 2023 08:50:20 +03:00
Committer: Daniel Lezcano <[email protected]>
CommitterDate: Wed, 11 Oct 2023 10:14:53 +02:00

drivers/clocksource/timer-ti-dm: Don't call clk_get_rate() in stop function

clk_get_rate() might sleep, and that prevents dm-timer based PWM from being
used from atomic context.

Fix that by getting fclk rate in probe() and using a notifier in case rate
changes.

Fixes: af04aa856e93 ("ARM: OMAP: Move dmtimer driver out of plat-omap to drivers under clocksource")
Signed-off-by: Ivaylo Dimitrov <[email protected]>
Reviewed-by: Tony Lindgren <[email protected]>
Signed-off-by: Daniel Lezcano <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
drivers/clocksource/timer-ti-dm.c | 36 +++++++++++++++++++++++-------
1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/drivers/clocksource/timer-ti-dm.c b/drivers/clocksource/timer-ti-dm.c
index 09ab29c..5f60f6b 100644
--- a/drivers/clocksource/timer-ti-dm.c
+++ b/drivers/clocksource/timer-ti-dm.c
@@ -140,6 +140,8 @@ struct dmtimer {
struct platform_device *pdev;
struct list_head node;
struct notifier_block nb;
+ struct notifier_block fclk_nb;
+ unsigned long fclk_rate;
};

static u32 omap_reserved_systimers;
@@ -253,8 +255,7 @@ static inline void __omap_dm_timer_enable_posted(struct dmtimer *timer)
timer->posted = OMAP_TIMER_POSTED;
}

-static inline void __omap_dm_timer_stop(struct dmtimer *timer,
- unsigned long rate)
+static inline void __omap_dm_timer_stop(struct dmtimer *timer)
{
u32 l;

@@ -269,7 +270,7 @@ static inline void __omap_dm_timer_stop(struct dmtimer *timer,
* Wait for functional clock period x 3.5 to make sure that
* timer is stopped
*/
- udelay(3500000 / rate + 1);
+ udelay(3500000 / timer->fclk_rate + 1);
#endif
}

@@ -348,6 +349,21 @@ static int omap_timer_context_notifier(struct notifier_block *nb,
return NOTIFY_OK;
}

+static int omap_timer_fclk_notifier(struct notifier_block *nb,
+ unsigned long event, void *data)
+{
+ struct clk_notifier_data *clk_data = data;
+ struct dmtimer *timer = container_of(nb, struct dmtimer, fclk_nb);
+
+ switch (event) {
+ case POST_RATE_CHANGE:
+ timer->fclk_rate = clk_data->new_rate;
+ return NOTIFY_OK;
+ default:
+ return NOTIFY_DONE;
+ }
+}
+
static int omap_dm_timer_reset(struct dmtimer *timer)
{
u32 l, timeout = 100000;
@@ -754,7 +770,6 @@ static int omap_dm_timer_stop(struct omap_dm_timer *cookie)
{
struct dmtimer *timer;
struct device *dev;
- unsigned long rate = 0;

timer = to_dmtimer(cookie);
if (unlikely(!timer))
@@ -762,10 +777,7 @@ static int omap_dm_timer_stop(struct omap_dm_timer *cookie)

dev = &timer->pdev->dev;

- if (!timer->omap1)
- rate = clk_get_rate(timer->fclk);
-
- __omap_dm_timer_stop(timer, rate);
+ __omap_dm_timer_stop(timer);

pm_runtime_put_sync(dev);

@@ -1124,6 +1136,14 @@ static int omap_dm_timer_probe(struct platform_device *pdev)
timer->fclk = devm_clk_get(dev, "fck");
if (IS_ERR(timer->fclk))
return PTR_ERR(timer->fclk);
+
+ timer->fclk_nb.notifier_call = omap_timer_fclk_notifier;
+ ret = devm_clk_notifier_register(dev, timer->fclk,
+ &timer->fclk_nb);
+ if (ret)
+ return ret;
+
+ timer->fclk_rate = clk_get_rate(timer->fclk);
} else {
timer->fclk = ERR_PTR(-ENODEV);
}

2024-01-13 16:26:58

by Sean Young

[permalink] [raw]
Subject: Re: [PATCH] drivers/clocksource/timer-ti-dm: Don't call clk_get_rate() in stop function

Hi Ivaylo,

On Tue, Oct 03, 2023 at 08:50:20AM +0300, Ivaylo Dimitrov wrote:
> clk_get_rate() might sleep, and that prevents dm-timer based PWM from being
> used from atomic context.

Now that this is merged, pwm-ir-tx can only use the pwm in atomic context
if pwm-omap-timer.c sets the atomic field like the rpi pwm does here:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pwm/pwm-bcm2835.c#n175

see pwm-ir-tx here:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/rc/pwm-ir-tx.c#n170

It would be nice to have this tested and working properly for the n900.

Thanks,

Sean


>
> Fix that by getting fclk rate in probe() and using a notifier in case rate
> changes.
>
> Fixes: af04aa856e93 ("ARM: OMAP: Move dmtimer driver out of plat-omap to drivers under clocksource")
> Signed-off-by: Ivaylo Dimitrov <[email protected]>
> ---
> drivers/clocksource/timer-ti-dm.c | 36 ++++++++++++++++++++++++++++--------
> 1 file changed, 28 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/clocksource/timer-ti-dm.c b/drivers/clocksource/timer-ti-dm.c
> index 09ab29c..5f60f6b 100644
> --- a/drivers/clocksource/timer-ti-dm.c
> +++ b/drivers/clocksource/timer-ti-dm.c
> @@ -140,6 +140,8 @@ struct dmtimer {
> struct platform_device *pdev;
> struct list_head node;
> struct notifier_block nb;
> + struct notifier_block fclk_nb;
> + unsigned long fclk_rate;
> };
>
> static u32 omap_reserved_systimers;
> @@ -253,8 +255,7 @@ static inline void __omap_dm_timer_enable_posted(struct dmtimer *timer)
> timer->posted = OMAP_TIMER_POSTED;
> }
>
> -static inline void __omap_dm_timer_stop(struct dmtimer *timer,
> - unsigned long rate)
> +static inline void __omap_dm_timer_stop(struct dmtimer *timer)
> {
> u32 l;
>
> @@ -269,7 +270,7 @@ static inline void __omap_dm_timer_stop(struct dmtimer *timer,
> * Wait for functional clock period x 3.5 to make sure that
> * timer is stopped
> */
> - udelay(3500000 / rate + 1);
> + udelay(3500000 / timer->fclk_rate + 1);
> #endif
> }
>
> @@ -348,6 +349,21 @@ static int omap_timer_context_notifier(struct notifier_block *nb,
> return NOTIFY_OK;
> }
>
> +static int omap_timer_fclk_notifier(struct notifier_block *nb,
> + unsigned long event, void *data)
> +{
> + struct clk_notifier_data *clk_data = data;
> + struct dmtimer *timer = container_of(nb, struct dmtimer, fclk_nb);
> +
> + switch (event) {
> + case POST_RATE_CHANGE:
> + timer->fclk_rate = clk_data->new_rate;
> + return NOTIFY_OK;
> + default:
> + return NOTIFY_DONE;
> + }
> +}
> +
> static int omap_dm_timer_reset(struct dmtimer *timer)
> {
> u32 l, timeout = 100000;
> @@ -754,7 +770,6 @@ static int omap_dm_timer_stop(struct omap_dm_timer *cookie)
> {
> struct dmtimer *timer;
> struct device *dev;
> - unsigned long rate = 0;
>
> timer = to_dmtimer(cookie);
> if (unlikely(!timer))
> @@ -762,10 +777,7 @@ static int omap_dm_timer_stop(struct omap_dm_timer *cookie)
>
> dev = &timer->pdev->dev;
>
> - if (!timer->omap1)
> - rate = clk_get_rate(timer->fclk);
> -
> - __omap_dm_timer_stop(timer, rate);
> + __omap_dm_timer_stop(timer);
>
> pm_runtime_put_sync(dev);
>
> @@ -1124,6 +1136,14 @@ static int omap_dm_timer_probe(struct platform_device *pdev)
> timer->fclk = devm_clk_get(dev, "fck");
> if (IS_ERR(timer->fclk))
> return PTR_ERR(timer->fclk);
> +
> + timer->fclk_nb.notifier_call = omap_timer_fclk_notifier;
> + ret = devm_clk_notifier_register(dev, timer->fclk,
> + &timer->fclk_nb);
> + if (ret)
> + return ret;
> +
> + timer->fclk_rate = clk_get_rate(timer->fclk);
> } else {
> timer->fclk = ERR_PTR(-ENODEV);
> }
> --
> 1.9.1