Hi Sakari Ailus, Thanks for the feedback. > -----Original Message----- > From: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> > 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@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). 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) { >