| Message ID | 20240131160947.96171-2-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Add clk_disable_unprepare_sync() | expand |
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. 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. 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. > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > 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) > {
Hi Laurent, Thanks for the feedback. > -----Original Message----- > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > 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@bp.renesas.com> > > --- > > 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
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) {
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. Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- drivers/clk/clk.c | 36 +++++++++++++++++++++++++++--------- include/linux/clk-provider.h | 4 ++++ include/linux/clk.h | 25 +++++++++++++++++++++++++ 3 files changed, 56 insertions(+), 9 deletions(-)