Hi Biju,
Thanks for the patchset.
On Tue, Feb 20, 2024 at 10:43:35AM +0000, Biju Das wrote:
> The clk_disable_unprepare() doesn't guarantee that a clock is gated after
> the execution as it is driver dependent. The Renesas and most of the other
> platforms don't wait until clock is stopped because of performance reason.
> But these platforms wait while turning on the clock.
>
> The normal case for shutting down the clock is unbind/close/suspend or
> error paths in the driver. Not waiting for the shutting down the clock
> will improve the suspend time.
>
> But on RZ/G2L Camera Data Receiving Unit (CRU) IP, initially the vclk is
> on. Before enabling link reception, we need to wait for vclk to be off
> and after enabling reception, we need to turn the vlck on. Special cases
> like this requires a sync API for clock gating.
>
> Add clk_poll_disable_unprepare() to poll the clock gate operation that
> guarantees gating of clk after the execution.
>
> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> ---
> RFC->v2:
> * Renamed clk_disable_unprepare_sync()-->clk_poll_disable_unprepare()
> * Redesigned to make use of __clk_is_enabled() to poll the clock gating.
> ---
> drivers/clk/clk.c | 23 +++++++++++++++++++++++
> include/linux/clk.h | 46 +++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 69 insertions(+)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 9a09f51f4af1..0e66b7180388 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -13,6 +13,7 @@
> #include <linux/mutex.h>
> #include <linux/spinlock.h>
> #include <linux/err.h>
> +#include <linux/iopoll.h>
> #include <linux/list.h>
> #include <linux/slab.h>
> #include <linux/of.h>
> @@ -1138,6 +1139,28 @@ void clk_disable(struct clk *clk)
> }
> EXPORT_SYMBOL_GPL(clk_disable);
>
> +/**
> + * clk_poll_disabled - poll for clock gating.
> + * @clk: the clk that is going to stop
> + * @sleep_us: Maximum time to sleep between reads in us (0
> + * tight-loops). Should be less than ~20ms since usleep_range
> + * is used (see Documentation/timers/timers-howto.rst).
> + * @timeout_us: Timeout in us, 0 means never timeout
> + *
> + * It polls for a clk to be stopped.
> + */
> +int clk_poll_disabled(struct clk *clk, unsigned long sleep_us, u64 timeout_us)
> +{
> + bool status;
> +
> + if (IS_ERR_OR_NULL(clk))
> + return 0;
> +
> + return read_poll_timeout(__clk_is_enabled, status, !status, sleep_us,
> + timeout_us, false, clk);
This API is a bit problematic as anything else in the system could enable
or disable the clock while polling happens. I think you should add a
warning that this may only be used if the user is the sole user of the
clock in the system (which is of course hard to guarantee in a general
case) and has not increased the enable count (or has decremented it again
to zero).
I'd perhaps go as far as do WARN_ON(enable count non-zero) and return
an error code (-EBUSY).
> +}
> +EXPORT_SYMBOL_GPL(clk_poll_disabled);
> +
> static int clk_core_enable(struct clk_core *core)
> {
> int ret = 0;
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index e6acec5d8dbe..2d63a12214e5 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -665,6 +665,20 @@ int __must_check clk_bulk_enable(int num_clks,
> */
> void clk_disable(struct clk *clk);
>
> +/**
> + * clk_poll_disabled - inform the system whether the clock source is stopped.
> + * @clk: clock source
> + * @sleep_us: Maximum time to sleep between reads in us (0
> + * tight-loops). Should be less than ~20ms since usleep_range
> + * is used (see Documentation/timers/timers-howto.rst).
> + * @timeout_us: Timeout in us, 0 means never timeout
> + *
> + * Poll for clock gating and Inform the system about it's status.
> + *
> + * Context: May sleep.
> + */
> +int clk_poll_disabled(struct clk *clk, unsigned long sleep_us, u64 timeout_us);
> +
> /**
> * clk_bulk_disable - inform the system when the set of clks is no
> * longer required.
> @@ -996,6 +1010,11 @@ static inline int __must_check clk_bulk_enable(int num_clks,
>
> static inline void clk_disable(struct clk *clk) {}
>
> +static inline int clk_poll_disabled(struct clk *clk, unsigned long sleep_us,
> + u64 timeout_us)
> +{
> + return 0;
> +}
>
> static inline void clk_bulk_disable(int num_clks,
> const struct clk_bulk_data *clks) {}
> @@ -1087,6 +1106,33 @@ static inline void clk_disable_unprepare(struct clk *clk)
> clk_unprepare(clk);
> }
>
> +/**
> + * clk_poll_disable_unprepare - Poll clk_disable_unprepare
> + * @clk: clock source
> + * @sleep_us: Maximum time to sleep between reads in us (0
> + * tight-loops). Should be less than ~20ms since usleep_range
> + * is used (see Documentation/timers/timers-howto.rst).
> + * @timeout_us: Timeout in us, 0 means never timeout
> + *
> + * Context: May sleep.
> + *
> + * This function polls until the clock has stopped.
> + *
> + * Returns success (0) or negative errno.
> + */
> +static inline int clk_poll_disable_unprepare(struct clk *clk,
> + unsigned long sleep_us,
> + u64 timeout_us)
> +{
> + int ret;
> +
> + clk_disable(clk);
> + ret = clk_poll_disabled(clk, sleep_us, timeout_us);
> + clk_unprepare(clk);
How about clocks that are generated by devices to which access always
sleeps, such as I²C devices? I presume they're actually stopped in
clk_unprepare() as clk_disable() may not sleep. They also can't implement
is_enabled as it cannot sleep either.
It seems to depend on the implementation on what they do. The runtime PM
function used is pm_runtime_put_sync(), so you may have a guarantee the
device is powered off but ONLY if it had no other users and had runtime PM
enabled.
So perhaps return an error if there's no is_enabled() callback?
> +
> + return ret;
> +}
> +
> static inline int __must_check
> clk_bulk_prepare_enable(int num_clks, const struct clk_bulk_data *clks)
> {
--
Regards,
Sakari Ailus
