Re: [PATCH v3 2/3] clk: Add clk_poll_disable_unprepare()

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Biju,

Thanks for the update.

On Mon, Mar 18, 2024 at 11:08:41AM +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@bp.renesas.com>
> ---
> v2->v3:
>  * Added WARN_ON(enable count non-zero) and return an error code (-EBUSY),
>    if the user is not the sole user of the clock and the enable count is
>    non-zero.
>  * Returned an error if there's no is_enabled() callback.
> 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   | 29 ++++++++++++++++++++++++++++
>  include/linux/clk.h | 46 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 75 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index f5fa91a339d7..e10bb14c904d 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>
> @@ -1160,6 +1161,34 @@ 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.
> + */

We should have documentation either in the header or here, not both. I'd
drop this.

> +int clk_poll_disabled(struct clk *clk, unsigned long sleep_us, u64 timeout_us)
> +{
> +	bool status;
> +
> +	if (IS_ERR_OR_NULL(clk))
> +		return 0;
> +
> +	if (!clk->core->ops->is_enabled)
> +		return -EOPNOTSUPP;
> +
> +	if (WARN(__clk_get_enable_count(clk), "clk is in use\n"))
> +		return -EBUSY;
> +
> +	return read_poll_timeout(__clk_is_enabled, status, !status, sleep_us,
> +				 timeout_us, false, clk);
> +}
> +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 84b02518791f..7f714ecce0eb 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -693,6 +693,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.

How about, instead:

	Poll for clock gating and return when either there's a timeout or
	the clock has been gated.

	Returns: 0 if the clock is successfully gated, error otherwise.

Please run scripts/kerneldoc -Wall on this.

> + *
> + * 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.
> @@ -1030,6 +1044,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) {}
> @@ -1121,6 +1140,33 @@ static inline void clk_disable_unprepare(struct clk *clk)
>  	clk_unprepare(clk);
>  }
>  
> +/**
> + * clk_poll_disable_unprepare - Poll clk_disable_unprepare

How about calling this clk_disable_sync_unprepare?

I'm not sure if a special function is needed for something needed so rarely
as you can already call clk_poll_disabled(). Maybe others have an opinion
on this, too.

> + * @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);
> +
> +	return ret;
> +}
> +
>  static inline int __must_check
>  clk_bulk_prepare_enable(int num_clks, const struct clk_bulk_data *clks)
>  {

-- 
Kind regards,

Sakari Ailus