Hi Sakari Ailus,
Thanks for the feedback.
> -----Original Message-----
> From: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> Sent: Monday, March 18, 2024 6:24 PM
> Subject: Re: [PATCH v3 2/3] clk: Add clk_poll_disable_unprepare()
>
> 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@xxxxxxxxxxxxxx>
> > ---
> > 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.
OK, will drop from here, If it is ok for every one.
>
> > +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.
OK.
>
> > + *
> > + * 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.
This API is only for exclusive use. I am ok for clk_disable_sync_unprepare()
as it synchronize clk disable operation first and then call unprepared.
Cheers,
Biju
>
> > + * @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
