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