[PATCH v3 0/3] Add clk_poll_disable_unprepare()

[PATCH v3 0/3] Add clk_poll_disable_unprepare()

[Biju Das]

Currently documentation related to disabling of clock do not state
it is synchronous or asynchronous clock gating. So, make it clear it is
driver dependent.

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.

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:
 * Updated cover letter description and header.
 * Created patch#1 for updating existing documentation.
 * Renamed clk_disable_unprepare_sync()-->clk_poll_disable_unprepare()
 * Redesigned to make use of __clk_is_enabled() to poll the clock gating.

Biju Das (3):
  clk: Update API documentation related to clock disable
  clk: Add clk_poll_disable_unprepare()
  media: platform: rzg2l-cru: rzg2l-video: Use
    clk_poll_disable_unprepare()

 drivers/clk/clk.c                             | 32 +++++++++++-
 .../platform/renesas/rzg2l-cru/rzg2l-csi2.c   |  5 +-
 include/linux/clk-provider.h                  |  3 +-
 include/linux/clk.h                           | 49 ++++++++++++++++++-
 4 files changed, 85 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH v3 1/3] clk: Update API documentation related to clock disable

[Biju Das]

The API's related to clk disable operation does not explicitly
states the synchoronous or asynchrous behaviour as it is driver
dependent. So make this part clear in API documentation.

Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2->v3:
 * No change.
v2:
 * New patch.
---
 drivers/clk/clk.c            | 3 ++-
 include/linux/clk-provider.h | 3 ++-
 include/linux/clk.h          | 3 ++-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 25371c91a58f..f5fa91a339d7 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1010,7 +1010,8 @@ static void clk_core_unprepare_lock(struct clk_core *core)
  * if the operation may sleep.  One example is a clk which is accessed over
  * I2c.  In the complex case a clk gate operation may require a fast and a slow
  * part.  It is this reason that clk_unprepare and clk_disable are not mutually
- * exclusive.  In fact clk_disable must be called before clk_unprepare.
+ * exclusive.  In fact clk_disable must be called before clk_unprepare.  The
+ * synchronous or asynchronous clock gating operation is driver dependent.
  */
 void clk_unprepare(struct clk *clk)
 {
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 4a537260f655..5b493024e1ec 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -113,7 +113,8 @@ struct clk_duty {
  *		sleep.
  *
  * @disable:	Disable the clock atomically. Called with enable_lock held.
- *		This function must not sleep.
+ *		This function must not sleep. The synchronous or asynchronous
+ *		disabling of the clock is driver dependent.
  *
  * @is_enabled:	Queries the hardware to determine if the clock is enabled.
  *		This function must not sleep. Optional, if this op is not
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 00623f4de5e1..84b02518791f 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -681,7 +681,8 @@ int __must_check clk_bulk_enable(int num_clks,
  * @clk: clock source
  *
  * Inform the system that a clock source is no longer required by
- * a driver and may be shut down.
+ * a driver and may be shut down. It is not guaranteed to ever actually
+ * be stopped, that will be driver dependent.
  *
  * May be called from atomic contexts.
  *
-- 
2.25.1

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

[Biju Das]

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.
+ */
+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.
+ *
+ * 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
+ * @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)
 {
-- 
2.25.1

[PATCH v3 3/3] media: platform: rzg2l-cru: rzg2l-video: Use clk_poll_disable_unprepare()

[Biju Das]

Use the clk_poll_disable_unprepare() for synchronizing clk gating in
rzg2l_csi2_mipi_link_enable().

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2->v3:
 * No change.
v1->v2:
 * Replaced clk_disable_unprepare_sync()-->clk_poll_disable_unprepare()
   and the error propagated to the caller.
---
 drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
index e68fcdaea207..986435bd85c1 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
@@ -366,6 +366,7 @@ static int rzg2l_csi2_mipi_link_enable(struct rzg2l_csi2 *csi2)
 {
 	unsigned long vclk_rate = csi2->vclk_rate / HZ_PER_MHZ;
 	u32 frrskw, frrclk, frrskw_coeff, frrclk_coeff;
+	int ret;
 
 	/* Select data lanes */
 	rzg2l_csi2_write(csi2, CSI2nMCT0, CSI2nMCT0_VDLN(csi2->lanes));
@@ -387,7 +388,9 @@ static int rzg2l_csi2_mipi_link_enable(struct rzg2l_csi2 *csi2)
 	rzg2l_csi2_write(csi2, CSI2nDTEL, 0xf778ff0f);
 	rzg2l_csi2_write(csi2, CSI2nDTEH, 0x00ffff1f);
 
-	clk_disable_unprepare(csi2->vclk);
+	ret = clk_poll_disable_unprepare(csi2->vclk, 10, 10000);
+	if (ret)
+		return ret;
 
 	/* Enable LINK reception */
 	rzg2l_csi2_write(csi2, CSI2nMCT3, CSI2nMCT3_RXEN);
-- 
2.25.1

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

[Sakari Ailus]

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

Re: [PATCH v3 1/3] clk: Update API documentation related to clock disable

[Sakari Ailus]

Hi Biju,

On Mon, Mar 18, 2024 at 11:08:40AM +0000, Biju Das wrote:
> 
> The API's related to clk disable operation does not explicitly
> states the synchoronous or asynchrous behaviour as it is driver
> dependent. So make this part clear in API documentation.
> 
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v2->v3:
>  * No change.
> v2:
>  * New patch.
> ---
>  drivers/clk/clk.c            | 3 ++-
>  include/linux/clk-provider.h | 3 ++-
>  include/linux/clk.h          | 3 ++-
>  3 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 25371c91a58f..f5fa91a339d7 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1010,7 +1010,8 @@ static void clk_core_unprepare_lock(struct clk_core *core)
>   * if the operation may sleep.  One example is a clk which is accessed over
>   * I2c.  In the complex case a clk gate operation may require a fast and a slow
>   * part.  It is this reason that clk_unprepare and clk_disable are not mutually
> - * exclusive.  In fact clk_disable must be called before clk_unprepare.
> + * exclusive.  In fact clk_disable must be called before clk_unprepare.  The
> + * synchronous or asynchronous clock gating operation is driver dependent.
>   */
>  void clk_unprepare(struct clk *clk)
>  {
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 4a537260f655..5b493024e1ec 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -113,7 +113,8 @@ struct clk_duty {
>   *		sleep.
>   *
>   * @disable:	Disable the clock atomically. Called with enable_lock held.
> - *		This function must not sleep.
> + *		This function must not sleep. The synchronous or asynchronous
> + *		disabling of the clock is driver dependent.

s/driver\K/ and hardware/

Same in the first chunk actually.

>   *
>   * @is_enabled:	Queries the hardware to determine if the clock is enabled.
>   *		This function must not sleep. Optional, if this op is not
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 00623f4de5e1..84b02518791f 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -681,7 +681,8 @@ int __must_check clk_bulk_enable(int num_clks,
>   * @clk: clock source
>   *
>   * Inform the system that a clock source is no longer required by
> - * a driver and may be shut down.
> + * a driver and may be shut down. It is not guaranteed to ever actually
> + * be stopped, that will be driver dependent.

I'd rephrase this, taking other users into account:

	There's no guarantee that the clock stops within a particular time
	window or at all, depending on other users of the clock as well as
	the driver and hardware implementation.

>   *
>   * May be called from atomic contexts.
>   *

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v3 1/3] clk: Update API documentation related to clock disable

[Laurent Pinchart]

Hi Biju,

Thank you for the patch.

On Mon, Mar 18, 2024 at 11:08:40AM +0000, Biju Das wrote:
> 
> The API's related to clk disable operation does not explicitly
> states the synchoronous or asynchrous behaviour as it is driver

s/synchoronous/synchronous/

> dependent. So make this part clear in API documentation.

You need to explain the rationale here, why asynchronous behaviour is
preferred.

> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v2->v3:
>  * No change.
> v2:
>  * New patch.
> ---
>  drivers/clk/clk.c            | 3 ++-
>  include/linux/clk-provider.h | 3 ++-
>  include/linux/clk.h          | 3 ++-
>  3 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 25371c91a58f..f5fa91a339d7 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1010,7 +1010,8 @@ static void clk_core_unprepare_lock(struct clk_core *core)
>   * if the operation may sleep.  One example is a clk which is accessed over
>   * I2c.  In the complex case a clk gate operation may require a fast and a slow
>   * part.  It is this reason that clk_unprepare and clk_disable are not mutually
> - * exclusive.  In fact clk_disable must be called before clk_unprepare.
> + * exclusive.  In fact clk_disable must be called before clk_unprepare.  The
> + * synchronous or asynchronous clock gating operation is driver dependent.

If synchronous operation is not guaranteed, then it's asynchonous.
Asynchronous doesn't mean slow, even an asynchronous provider can
complete the disable operation before the function returns to the
caller. All it means is that there's no guarantee of synchronous
operation. I would document it as such:

 * This function is asynchronous, if may return before the clock provider 
 * completes the unprepare operation.

However, below you're addressing the disable operation. Did you mean to
patch the documentation for clk_prepare() instead ? Making
clk_unprepare() asynchronous seems a bit weird, given that the function
may sleep and is expected to take more time.

>   */
>  void clk_unprepare(struct clk *clk)
>  {
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 4a537260f655..5b493024e1ec 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -113,7 +113,8 @@ struct clk_duty {
>   *		sleep.
>   *
>   * @disable:	Disable the clock atomically. Called with enable_lock held.
> - *		This function must not sleep.
> + *		This function must not sleep. The synchronous or asynchronous
> + *		disabling of the clock is driver dependent.

As this is the documentation that targets clock providers, I would
expand it and explain why a provider may want to make the disable
operation not synchronous.

>   *
>   * @is_enabled:	Queries the hardware to determine if the clock is enabled.
>   *		This function must not sleep. Optional, if this op is not

.is_enabled() should become mandatory if .disable() is not synchronous.
The relationship between the two operations should be better explained.

> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 00623f4de5e1..84b02518791f 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -681,7 +681,8 @@ int __must_check clk_bulk_enable(int num_clks,
>   * @clk: clock source
>   *
>   * Inform the system that a clock source is no longer required by
> - * a driver and may be shut down.
> + * a driver and may be shut down. It is not guaranteed to ever actually
> + * be stopped, that will be driver dependent.

This is the documentation of clk_bulk_disable(), you should address
clk_disable() too. I've just noticed that both functions are documented
in two places, in include/linux/clk.h, and in drivers/clk/. I wonder why
that is. It sounds like it should be fixed, or you'll have to patch both
documentation blocks.

There's another issue that I'll raise in the review of 2/3.

>   *
>   * May be called from atomic contexts.
>   *

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Biju,

Thank you for the patch.

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.

Is that really the case for most platforms ? I've never seen that being
an issue in practice.

> 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.
> + */
> +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;

A WARN() is fairly bad, given how easy it could be to this this
condition. To make it safe in the genral case for drivers to use this
API, we may need a way to acquire a clock exclusively, which would
complexify the API quite a bit.

> +
> +	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.
> + *
> + * 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
> + * @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);

What happens in the clk_disable_unprepare() case, if the clk_unprepare()
function is called on a clock that hasn't been synchronously disabled ?
This is ill-defined, a clock provider driver that implements .disable()
asynchronously would see its .unprepare() operation called with
different clock states. That behaviour is error-prone, especially given
that it could be difficult to test clock provider drivers to ensure that
handle both cases correctly.

One option could be to turn the .unprepare() operation into a
synchronization point, requiring drivers that implement .disable()
asynchronously to implement synchronization in .unprepare(). That way
you wouldn't need a new API function for clock consumers. The downside
is that consumers that call clk_disable_unprepare() will never benefit
from the .disable() operation being asynchronous, which may defeat the
whole point of this exercise.

I'm starting to wonder if the simplest option in your case wouldn't be
to make your clock provider synchronous for the vclk...

> +
> +	return ret;
> +}
> +
>  static inline int __must_check
>  clk_bulk_prepare_enable(int num_clks, const struct clk_bulk_data *clks)
>  {

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 1/3] clk: Update API documentation related to clock disable

[Stephen Boyd]

Quoting Biju Das (2024-03-18 04:08:40)
> 
> The API's related to clk disable operation does not explicitly
> states the synchoronous or asynchrous behaviour as it is driver
> dependent. So make this part clear in API documentation.
> 
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---

I'd like Russell's ack on this.

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

[Stephen Boyd]

Quoting Laurent Pinchart (2024-03-18 15:55:46)
> On Mon, Mar 18, 2024 at 11:08:41AM +0000, Biju Das wrote:
> > +     int ret;
> > +
> > +     clk_disable(clk);
> > +     ret = clk_poll_disabled(clk, sleep_us, timeout_us);
> > +     clk_unprepare(clk);
> 
> What happens in the clk_disable_unprepare() case, if the clk_unprepare()
> function is called on a clock that hasn't been synchronously disabled ?
> This is ill-defined, a clock provider driver that implements .disable()
> asynchronously would see its .unprepare() operation called with
> different clock states. That behaviour is error-prone, especially given
> that it could be difficult to test clock provider drivers to ensure that
> handle both cases correctly.
> 
> One option could be to turn the .unprepare() operation into a
> synchronization point, requiring drivers that implement .disable()
> asynchronously to implement synchronization in .unprepare(). That way
> you wouldn't need a new API function for clock consumers. The downside
> is that consumers that call clk_disable_unprepare() will never benefit
> from the .disable() operation being asynchronous, which may defeat the
> whole point of this exercise.
> 
> I'm starting to wonder if the simplest option in your case wouldn't be
> to make your clock provider synchronous for the vclk...

Yes. This all looks unnecessary if the device using the clk always
requires the clk to actually be shut down when it is disabled. Just do
that for this vclk and move on. It _is_ tightly coupling this specific
clk to the specific consumer, but that's simplest and most expedient to
implement, i.e. it's practical.

We would only ever need this API if we had a clk consumer which
absolutely required the clk to stop clocking upon returning from
clk_unprepare(), and that clk could be any random clk. It sounds like in
this case that isn't true. We know which clk must be off when
clk_unprepare() returns, so just implement the wait in the
clk_ops::unprepare() callback?

Re: [PATCH v3 1/3] clk: Update API documentation related to clock disable

[Russell King (Oracle)]

On Mon, Mar 18, 2024 at 11:08:40AM +0000, Biju Das wrote:
> 
> The API's related to clk disable operation does not explicitly
> states the synchoronous or asynchrous behaviour as it is driver
> dependent. So make this part clear in API documentation.
> 
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v2->v3:
>  * No change.
> v2:
>  * New patch.
> ---
>  drivers/clk/clk.c            | 3 ++-
>  include/linux/clk-provider.h | 3 ++-
>  include/linux/clk.h          | 3 ++-
>  3 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 25371c91a58f..f5fa91a339d7 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1010,7 +1010,8 @@ static void clk_core_unprepare_lock(struct clk_core *core)
>   * if the operation may sleep.  One example is a clk which is accessed over
>   * I2c.  In the complex case a clk gate operation may require a fast and a slow
>   * part.  It is this reason that clk_unprepare and clk_disable are not mutually
> - * exclusive.  In fact clk_disable must be called before clk_unprepare.
> + * exclusive.  In fact clk_disable must be called before clk_unprepare.  The
> + * synchronous or asynchronous clock gating operation is driver dependent.

What is important is that:

	clk_unprepare(clk);
	clk_prepare(clk);

must result in the prepare operation being completed before
clk_prepare() returns. The same applies to clk_disable(clk) followed
by clk_enable(clk).

Since these comment updates are aimed at the unprepare/disable
functionality, I think they're fine.

Acked-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Thanks!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Russell King (Oracle)]

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.

I'm not sure it's "because of performance reason". It's probably more
that it's not important for functionality.

> 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

"vclk" not "vlck".

> like this requires a sync API for clock gating.

I suppose this is fine for clocks that only have a single user, but
this is highly undefined for clocks that could be shared between
several different users, since it becomes racy whether another user
of the clock has enabled or disabled this clock.

I think this new API needs to spell it that it is not for clocks
that are shared.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!