| Message ID | 20240220104336.260194-3-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Add clk_poll_disable_unprepare() | expand |
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@bp.renesas.com> > --- > 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) > {
Hi Sakari Ailus, Thanks for the feedback. > -----Original Message----- > From: Sakari Ailus <sakari.ailus@linux.intel.com> > Sent: Tuesday, February 27, 2024 9:36 AM > Subject: Re: [PATCH v2 2/3] clk: Add clk_poll_disable_unprepare() > > 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@bp.renesas.com> > > --- > > 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). Agreed. > > I'd perhaps go as far as do WARN_ON(enable count non-zero) and return an error code (-EBUSY). OK, the below code should cover the above case and below one right? + if (!clk->core->ops->is_enabled) + return -EOPNOTSUPP; + + if (WARN(__clk_get_enable_count(clk), "clk is in use\n")) + return -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. Even in RPM case, at the end, it comes down to enable count. So, the check you mentioned for enabled_could should be fine?? > > So perhaps return an error if there's no is_enabled() callback? OK, This will be taken care inside clk_poll_disabled(). Cheers, Biju > > > + > > + return ret; > > +} > > + > > static inline int __must_check > > clk_bulk_prepare_enable(int num_clks, const struct clk_bulk_data > > *clks) { >
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); +} +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); + + return ret; +} + static inline int __must_check clk_bulk_prepare_enable(int num_clks, const struct clk_bulk_data *clks) {
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> --- 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(+)