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@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
