Hi Laurent,
Thanks for the feedback.
> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Sent: Tuesday, February 6, 2024 10:59 PM
> Subject: Re: [PATCH RFC 1/3] clk: Add clk_disable_unprepare_sync()
>
> Hi Biju,
>
> Thank you for the patch.
>
> On Wed, Jan 31, 2024 at 04:09:45PM +0000, Biju Das wrote:
> > Currently clk_disable() is not synchronous. Consumers just gate the
> > clock, but it doesn't check actually the clock is gated. Introduce
> > clk_disable_unprepare_sync() to synchronize the clock gate operation.
>
> I think this needs to be designed a bit more.
OK.
>
> First of all, you state that clk_disable() is not synchronous, but that's
> not documented anywhere. This should be addressed as a first patch, with
> clarifications to the documentation of the clock API functions
> (clk_disable(), clk_unprepare() and clk_disable_unprepare()) and the
> corresponding clock provider operations (clk_ops). You should also explain
> in that patch why that's the desired behaviour, compared to making those
> operations synchronous.
Agreed.
>
> Then, instead of adding a new synchronous clock disable operation, I
> wonder if it wouldn't be better to add a public function that synchronizes
> a previous disable call by using the exising infrastructure, and in
> particular the .is_enabled() operation. This would provide the feature
> you're looking for without needing to modify all clock provider drivers.
I agree. It is simpler.
Will test and send a new version incorporating above changes.
Cheers,
Biju
>
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > ---
> > drivers/clk/clk.c | 36 +++++++++++++++++++++++++++---------
> > include/linux/clk-provider.h | 4 ++++
> > include/linux/clk.h | 25 +++++++++++++++++++++++++
> > 3 files changed, 56 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index
> > 2253c154a824..958d6677b3a6 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -1080,7 +1080,7 @@ int clk_prepare(struct clk *clk) }
> > EXPORT_SYMBOL_GPL(clk_prepare);
> >
> > -static void clk_core_disable(struct clk_core *core)
> > +static void clk_core_disable(struct clk_core *core, bool sync)
> > {
> > lockdep_assert_held(&enable_lock);
> >
> > @@ -1102,17 +1102,20 @@ static void clk_core_disable(struct clk_core
> *core)
> > if (core->ops->disable)
> > core->ops->disable(core->hw);
> >
> > + if (sync && core->ops->disable_sync)
> > + core->ops->disable_sync(core->hw);
> > +
> > trace_clk_disable_complete(core);
> >
> > - clk_core_disable(core->parent);
> > + clk_core_disable(core->parent, false);
> > }
> >
> > -static void clk_core_disable_lock(struct clk_core *core)
> > +static void clk_core_disable_lock(struct clk_core *core, bool sync)
> > {
> > unsigned long flags;
> >
> > flags = clk_enable_lock();
> > - clk_core_disable(core);
> > + clk_core_disable(core, sync);
> > clk_enable_unlock(flags);
> > }
> >
> > @@ -1133,10 +1136,25 @@ void clk_disable(struct clk *clk)
> > if (IS_ERR_OR_NULL(clk))
> > return;
> >
> > - clk_core_disable_lock(clk->core);
> > + clk_core_disable_lock(clk->core, false);
> > }
> > EXPORT_SYMBOL_GPL(clk_disable);
> >
> > +/**
> > + * clk_disable_sync - gate a clock synchronously
> > + * @clk: the clk being gated
> > + *
> > + * It is similar to clk_disable, but guarantees that clk is gated
> synchronously.
> > + */
> > +void clk_disable_sync(struct clk *clk) {
> > + if (IS_ERR_OR_NULL(clk))
> > + return;
> > +
> > + clk_core_disable_lock(clk->core, true); }
> > +EXPORT_SYMBOL_GPL(clk_disable_sync);
> > +
> > static int clk_core_enable(struct clk_core *core) {
> > int ret = 0;
> > @@ -1164,7 +1182,7 @@ static int clk_core_enable(struct clk_core *core)
> > trace_clk_enable_complete(core);
> >
> > if (ret) {
> > - clk_core_disable(core->parent);
> > + clk_core_disable(core->parent, false);
> > return ret;
> > }
> > }
> > @@ -1340,7 +1358,7 @@ static int clk_core_prepare_enable(struct
> > clk_core *core)
> >
> > static void clk_core_disable_unprepare(struct clk_core *core) {
> > - clk_core_disable_lock(core);
> > + clk_core_disable_lock(core, false);
> > clk_core_unprepare_lock(core);
> > }
> >
> > @@ -2058,7 +2076,7 @@ static void __clk_set_parent_after(struct clk_core
> *core,
> > * for preventing a race with clk_enable().
> > */
> > if (core->prepare_count) {
> > - clk_core_disable_lock(core);
> > + clk_core_disable_lock(core, false);
> > clk_core_disable_unprepare(old_parent);
> > }
> >
> > @@ -2352,7 +2370,7 @@ static void clk_change_rate(struct clk_core *core)
> > core->rate = clk_recalc(core, best_parent_rate);
> >
> > if (core->flags & CLK_SET_RATE_UNGATE) {
> > - clk_core_disable_lock(core);
> > + clk_core_disable_lock(core, false);
> > clk_core_unprepare(core);
> > }
> >
> > diff --git a/include/linux/clk-provider.h
> > b/include/linux/clk-provider.h index 1293c38ddb7f..604cc9338465 100644
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -115,6 +115,9 @@ struct clk_duty {
> > * @disable: Disable the clock atomically. Called with enable_lock
> held.
> > * This function must not sleep.
> > *
> > + * @disable_sync: Disable and sync the clockatomically. Called with
> enable_lock
> > + * held.This function must not sleep.
> > + *
> > * @is_enabled: Queries the hardware to determine if the clock is
> enabled.
> > * This function must not sleep. Optional, if this op is not
> > * set then the enable count will be used.
> > @@ -238,6 +241,7 @@ struct clk_ops {
> > void (*unprepare_unused)(struct clk_hw *hw);
> > int (*enable)(struct clk_hw *hw);
> > void (*disable)(struct clk_hw *hw);
> > + void (*disable_sync)(struct clk_hw *hw);
> > int (*is_enabled)(struct clk_hw *hw);
> > void (*disable_unused)(struct clk_hw *hw);
> > int (*save_context)(struct clk_hw *hw);
> > diff --git a/include/linux/clk.h b/include/linux/clk.h index
> > 06f1b292f8a0..f472756310c7 100644
> > --- a/include/linux/clk.h
> > +++ b/include/linux/clk.h
> > @@ -664,6 +664,23 @@ int __must_check clk_bulk_enable(int num_clks,
> > */
> > void clk_disable(struct clk *clk);
> >
> > +/**
> > + * clk_disable_sync - inform the system when the clock source is no
> longer
> > + * required.
> > + * @clk: clock source
> > + *
> > + * Inform the system that a clock source is no longer required by
> > + * a driver and is in shut down.
> > + *
> > + * May be called from atomic contexts.
> > + *
> > + * Implementation detail: if the clock source is shared between
> > + * multiple drivers, clk_enable() calls must be balanced by the
> > + * same number of clk_disable_sync() calls for the clock source to be
> > + * disabled.
> > + */
> > +void clk_disable_sync(struct clk *clk);
> > +
> > /**
> > * clk_bulk_disable - inform the system when the set of clks is no
> > * longer required.
> > @@ -995,6 +1012,8 @@ static inline int __must_check
> > clk_bulk_enable(int num_clks,
> >
> > static inline void clk_disable(struct clk *clk) {}
> >
> > +static inline void clk_disable_sync(struct clk *clk) {}
> > +
> >
> > static inline void clk_bulk_disable(int num_clks,
> > const struct clk_bulk_data *clks) {} @@ -
> 1086,6 +1105,12 @@
> > static inline void clk_disable_unprepare(struct clk *clk)
> > clk_unprepare(clk);
> > }
> >
> > +static inline void clk_disable_unprepare_sync(struct clk *clk) {
> > + clk_disable_sync(clk);
> > + clk_unprepare(clk);
> > +}
> > +
> > static inline int __must_check
> > clk_bulk_prepare_enable(int num_clks, const struct clk_bulk_data
> > *clks) {
>
> --
> Regards,
>
> Laurent Pinchart
