Re: [PATCH RFC 3/5] clk: bcm281xx: implement prerequisite clocks

[Alex Elder <elder@riscstar.com>]

On 2/16/25 10:12 AM, Artur Weber wrote:
> From: Alex Elder <elder@kernel.org>
> 
> Allow a clock to specify a "prerequisite" clock, identified by its
> name.  The prerequisite clock must be prepared and enabled before a
> clock that depends on it is used.  In order to simplify locking, we
> require a clock and its prerequisite to be associated with the same
> CCU.  (We'll just trust--but not verify--that nobody defines a cycle
> of prerequisite clocks.)
> 
> Rework the KONA_CLK() macro, and define a new KONA_CLK_PREREQ()
> variant that allows a prerequisite clock to be specified.
> 
> Signed-off-by: Alex Elder <elder@linaro.org>
> --- Artur: rebase on v6.13, move prereq prepare/unprepare to main
>      prepare/unprepare functions, use locking versions of clk_prepare
>      and clk_enable since the non-locking versions are no longer
>      public ---
> Signed-off-by: Artur Weber <aweber.kernel@gmail.com>

I'm surprised there is no prepare function for the peripheral
clocks.

The prequisite clock should separate the prepare and the
enable functionality.  Right now you have kona_clk_prepare()
doing both.  Instead, a clock's prepare function should
prepare its prerequisite (if any).  Then its enable function
should enable its parent.

Should all the users of peripheral clocks just also be required
to specify the bus clocks as well?  I suppose that doesn't
encode the prerequisite property (bus comes before peripheral);
is that truly a requirement?

					-Alex

> ---
>   drivers/clk/bcm/clk-kona.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++
>   drivers/clk/bcm/clk-kona.h | 20 ++++++++++++---
>   2 files changed, 79 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/bcm/clk-kona.c b/drivers/clk/bcm/clk-kona.c
> index e92d57f3bbb147e72221802175a80502897d7504..21f925683d0da05ebc97f92236dfb207b1f9c741 100644
> --- a/drivers/clk/bcm/clk-kona.c
> +++ b/drivers/clk/bcm/clk-kona.c
> @@ -9,6 +9,7 @@
>   #include <linux/delay.h>
>   #include <linux/io.h>
>   #include <linux/kernel.h>
> +#include <linux/clk.h>
>   #include <linux/clk-provider.h>
>   
>   /*
> @@ -961,6 +962,63 @@ static int selector_write(struct ccu_data *ccu, struct bcm_clk_gate *gate,
>   	return ret;
>   }
>   
> +/*
> + * Common clock prepare/unprepare functions. These implement a "prerequisite"
> + * mechanism; the prerequisite clock is prepared and enabled before the main
> + * clock is prepared.
> + */
> +
> +static int kona_clk_prepare(struct clk_hw *hw)
> +{
> +	struct kona_clk *bcm_clk = to_kona_clk(hw);
> +	const char *clk_name = bcm_clk->init_data.name;
> +	const char *prereq_name = bcm_clk->prereq.name;
> +	struct clk *prereq_clk = bcm_clk->prereq.clk;
> +	int ret;
> +
> +	/* If there's no prerequisite clock, there's nothing to do */
> +	if (!prereq_name)
> +		return 0;
> +
> +	/* Look up the prerequisite clock if we haven't already */
> +	if (!prereq_clk) {
> +		prereq_clk = __clk_lookup(prereq_name);
> +		if (WARN_ON_ONCE(!prereq_clk))
> +			return -ENOENT;
> +		bcm_clk->prereq.clk = prereq_clk;
> +	}
> +
> +	ret = clk_prepare(prereq_clk);
> +	if (ret) {
> +		pr_err("%s: unable to prepare prereq clock %s for %s\n",
> +			__func__, prereq_name, clk_name);
> +		return ret;
> +	}
> +
> +	ret = clk_enable(prereq_clk);
> +	if (ret) {
> +		clk_unprepare(prereq_clk);
> +		pr_err("%s: unable to enable prereq clock %s for %s\n",
> +			__func__, prereq_name, clk_name);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void kona_clk_unprepare(struct clk_hw *hw)
> +{
> +	struct kona_clk *bcm_clk = to_kona_clk(hw);
> +	struct clk *prereq_clk = bcm_clk->prereq.clk;
> +
> +	/* If there's no prerequisite clock, there's nothing to do */
> +	if (!bcm_clk->prereq.name)
> +		return;
> +
> +	clk_disable(prereq_clk);
> +	clk_unprepare(prereq_clk);
> +}
> +
>   /* Peripheral clock operations */
>   
>   static int kona_peri_clk_enable(struct clk_hw *hw)
> @@ -1172,6 +1230,8 @@ static int kona_peri_clk_set_rate(struct clk_hw *hw, unsigned long rate,
>   }
>   
>   struct clk_ops kona_peri_clk_ops = {
> +	.prepare = kona_clk_prepare,
> +	.unprepare = kona_clk_unprepare,
>   	.enable = kona_peri_clk_enable,
>   	.disable = kona_peri_clk_disable,
>   	.is_enabled = kona_peri_clk_is_enabled,
> @@ -1260,6 +1320,8 @@ static int kona_bus_clk_is_enabled(struct clk_hw *hw)
>   }
>   
>   struct clk_ops kona_bus_clk_ops = {
> +	.prepare = kona_clk_prepare,
> +	.unprepare = kona_clk_unprepare,
>   	.enable = kona_bus_clk_enable,
>   	.disable = kona_bus_clk_disable,
>   	.is_enabled = kona_bus_clk_is_enabled,
> diff --git a/drivers/clk/bcm/clk-kona.h b/drivers/clk/bcm/clk-kona.h
> index a5b3d8bdb54eaee9fad80c28796170207b817dfd..c32c621282ec6dd40fff3f7598ee8aa007fed524 100644
> --- a/drivers/clk/bcm/clk-kona.h
> +++ b/drivers/clk/bcm/clk-kona.h
> @@ -406,6 +406,10 @@ struct kona_clk {
>   	struct clk_init_data init_data;	/* includes name of this clock */
>   	struct ccu_data *ccu;	/* ccu this clock is associated with */
>   	enum bcm_clk_type type;
> +	struct {
> +		const char *name;
> +		struct clk *clk;
> +	} prereq;
>   	union {
>   		void *data;
>   		struct peri_clk_data *peri;
> @@ -416,16 +420,26 @@ struct kona_clk {
>   	container_of(_hw, struct kona_clk, hw)
>   
>   /* Initialization macro for an entry in a CCU's kona_clks[] array. */
> -#define KONA_CLK(_ccu_name, _clk_name, _type)				\
> -	{								\
> +#define __KONA_CLK_COMMON(_ccu_name, _clk_name, _type)			\
>   		.init_data	= {					\
>   			.name = #_clk_name,				\
>   			.ops = &kona_ ## _type ## _clk_ops,		\
>   		},							\
>   		.ccu		= &_ccu_name ## _ccu_data,		\
>   		.type		= bcm_clk_ ## _type,			\
> -		.u.data		= &_clk_name ## _data,			\
> +		.u.data		= &_clk_name ## _data
> +
> +#define KONA_CLK(_ccu_name, _clk_name, _type)				\
> +	{								\
> +		__KONA_CLK_COMMON(_ccu_name, _clk_name, _type),	\
>   	}
> +
> +#define KONA_CLK_PREREQ(_ccu_name, _clk_name, _type, _prereq)		\
> +	{								\
> +		.prereq.name	= #_prereq,				\
> +		__KONA_CLK_COMMON(_ccu_name, _clk_name, _type),	\
> +	}
> +
>   #define LAST_KONA_CLK	{ .type = bcm_clk_none }
>   
>   /*
>