[PATCH v2 0/5] clk: bcm: prerequisite and bus clock support

[PATCH v2 0/5] clk: bcm: prerequisite and bus clock support

[Alex Elder]

Currently only peripheral clocks are supported for Broadcom platforms
that use Kona style CCUs for clocking.  This series adds support for
bus clocks as well.

One motivation for doing this is that there exist peripheral clocks
that cannot be configured without having first enabled a related
bus clock.  Adding bus clock support allows such peripheral clocks
to be usable.

This also imposes a new requirement, however--that the bus clock
be enabled *before* the clock that depends on it.  For this, we
define the notion of a "prerequisite" clock.  If a clock has a
prerequisite specified, that prequisite clock will be initialized
first.  For now this only affects startup-time initialization.

These patches are based on Mike Turquette's current "clk-next"
branch.
    6ed8eb5 Merge tag 'clk-hisi-for-v3.16' of https://git.kern...

They are available here:
    http://git.linaro.org/landing-teams/working/broadcom/kernel.git
    Branch review/bcm-bus-clk

					-Alex

Version history:
v2: Added field "p" to the previously unnamed prereq union.

Alex Elder (5):
  clk: bcm281xx: add an initialized flag
  clk: bcm281xx: implement prerequisite clocks
  clk: bcm281xx: add bus clock support
  clk: bcm281xx: define a bus clock
  ARM: dts: add bus clock bsc3_apb for bcm281xx

 arch/arm/boot/dts/bcm11351.dtsi      |   3 +-
 drivers/clk/bcm/clk-bcm281xx.c       |  13 +++-
 drivers/clk/bcm/clk-kona-setup.c     | 112 ++++++++++++++++++++++++++++++--
 drivers/clk/bcm/clk-kona.c           | 120 ++++++++++++++++++++++++++++++++++-
 drivers/clk/bcm/clk-kona.h           |  35 +++++++++-
 include/dt-bindings/clock/bcm281xx.h |   3 +-
 6 files changed, 271 insertions(+), 15 deletions(-)

-- 
1.9.1

[PATCH v2 1/5] clk: bcm281xx: add an initialized flag

[Alex Elder]

Add a flag that tracks whether a clock has already been initialized.
This will be used by the next patch to avoid initializing a clock
more than once when it's listed as a prerequisite.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/clk/bcm/clk-kona.c | 17 +++++++++++++++--
 drivers/clk/bcm/clk-kona.h |  7 +++++++
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/bcm/clk-kona.c b/drivers/clk/bcm/clk-kona.c
index d603c4e..d8a7f38 100644
--- a/drivers/clk/bcm/clk-kona.c
+++ b/drivers/clk/bcm/clk-kona.c
@@ -27,6 +27,9 @@
 #define CCU_ACCESS_PASSWORD      0xA5A500
 #define CLK_GATE_DELAY_LOOP      2000
 
+#define clk_is_initialized(_clk)	FLAG_TEST((_clk), KONA, INITIALIZED)
+#define clk_set_initialized(_clk)	FLAG_SET((_clk), KONA, INITIALIZED)
+
 /* Bitfield operations */
 
 /* Produces a mask of set bits covering a range of a 32-bit value */
@@ -1194,13 +1197,23 @@ static bool __peri_clk_init(struct kona_clk *bcm_clk)
 
 static bool __kona_clk_init(struct kona_clk *bcm_clk)
 {
+	bool ret;
+
+	if (clk_is_initialized(bcm_clk))
+		return true;
+
 	switch (bcm_clk->type) {
 	case bcm_clk_peri:
-		return __peri_clk_init(bcm_clk);
+		ret = __peri_clk_init(bcm_clk);
+		break;
 	default:
+		ret = false;
 		BUG();
 	}
-	return -EINVAL;
+	if (ret)
+		clk_set_initialized(bcm_clk);
+
+	return ret;
 }
 
 /* Set a CCU and all its clocks into their desired initial state */
diff --git a/drivers/clk/bcm/clk-kona.h b/drivers/clk/bcm/clk-kona.h
index 2537b30..10e238d 100644
--- a/drivers/clk/bcm/clk-kona.h
+++ b/drivers/clk/bcm/clk-kona.h
@@ -406,6 +406,7 @@ 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;
+	u32 flags;		/* BCM_CLK_KONA_FLAGS_* below */
 	union {
 		void *data;
 		struct peri_clk_data *peri;
@@ -414,6 +415,12 @@ struct kona_clk {
 #define to_kona_clk(_hw) \
 	container_of(_hw, struct kona_clk, hw)
 
+/*
+ * Kona clock flags:
+ *   INITIALIZED	clock has been initialized already
+ */
+#define BCM_CLK_KONA_FLAGS_INITIALIZED	((u32)1 << 0)	/* Clock initialized */
+
 /* Initialization macro for an entry in a CCU's kona_clks[] array. */
 #define KONA_CLK(_ccu_name, _clk_name, _type)				\
 	{								\
-- 
1.9.1

[PATCH v2 1/5] clk: bcm281xx: add an initialized flag

[Mike Turquette]

Quoting Alex Elder (2014-05-20 05:52:38)
> Add a flag that tracks whether a clock has already been initialized.
> This will be used by the next patch to avoid initializing a clock
> more than once when it's listed as a prerequisite.
> 
> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
>  drivers/clk/bcm/clk-kona.c | 17 +++++++++++++++--
>  drivers/clk/bcm/clk-kona.h |  7 +++++++
>  2 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/bcm/clk-kona.c b/drivers/clk/bcm/clk-kona.c
> index d603c4e..d8a7f38 100644
> --- a/drivers/clk/bcm/clk-kona.c
> +++ b/drivers/clk/bcm/clk-kona.c
> @@ -27,6 +27,9 @@
>  #define CCU_ACCESS_PASSWORD      0xA5A500
>  #define CLK_GATE_DELAY_LOOP      2000
>  
> +#define clk_is_initialized(_clk)       FLAG_TEST((_clk), KONA, INITIALIZED)
> +#define clk_set_initialized(_clk)      FLAG_SET((_clk), KONA, INITIALIZED)
> +
>  /* Bitfield operations */
>  
>  /* Produces a mask of set bits covering a range of a 32-bit value */
> @@ -1194,13 +1197,23 @@ static bool __peri_clk_init(struct kona_clk *bcm_clk)
>  
>  static bool __kona_clk_init(struct kona_clk *bcm_clk)
>  {
> +       bool ret;
> +
> +       if (clk_is_initialized(bcm_clk))
> +               return true;
> +
>         switch (bcm_clk->type) {
>         case bcm_clk_peri:
> -               return __peri_clk_init(bcm_clk);
> +               ret = __peri_clk_init(bcm_clk);

Hi Alex,

Going through this code, it's a bit hard to keep up ;-)

Does the call to __peri_clk_init enable the prereq clocks? If so, is
their clk->prepare_count and clk->enable_count properly incremented?

Thanks,
Mike

> +               break;
>         default:
> +               ret = false;
>                 BUG();
>         }
> -       return -EINVAL;
> +       if (ret)
> +               clk_set_initialized(bcm_clk);
> +
> +       return ret;
>  }
>  
>  /* Set a CCU and all its clocks into their desired initial state */
> diff --git a/drivers/clk/bcm/clk-kona.h b/drivers/clk/bcm/clk-kona.h
> index 2537b30..10e238d 100644
> --- a/drivers/clk/bcm/clk-kona.h
> +++ b/drivers/clk/bcm/clk-kona.h
> @@ -406,6 +406,7 @@ 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;
> +       u32 flags;              /* BCM_CLK_KONA_FLAGS_* below */
>         union {
>                 void *data;
>                 struct peri_clk_data *peri;
> @@ -414,6 +415,12 @@ struct kona_clk {
>  #define to_kona_clk(_hw) \
>         container_of(_hw, struct kona_clk, hw)
>  
> +/*
> + * Kona clock flags:
> + *   INITIALIZED       clock has been initialized already
> + */
> +#define BCM_CLK_KONA_FLAGS_INITIALIZED ((u32)1 << 0)   /* Clock initialized */
> +
>  /* Initialization macro for an entry in a CCU's kona_clks[] array. */
>  #define KONA_CLK(_ccu_name, _clk_name, _type)                          \
>         {                                                               \
> -- 
> 1.9.1
> 

[PATCH v2 1/5] clk: bcm281xx: add an initialized flag

[Alex Elder]

On 05/23/2014 07:33 PM, Mike Turquette wrote:
> Quoting Alex Elder (2014-05-20 05:52:38)
>> Add a flag that tracks whether a clock has already been initialized.
>> This will be used by the next patch to avoid initializing a clock
>> more than once when it's listed as a prerequisite.
>>
>> Signed-off-by: Alex Elder <elder@linaro.org>
>> ---
>>  drivers/clk/bcm/clk-kona.c | 17 +++++++++++++++--
>>  drivers/clk/bcm/clk-kona.h |  7 +++++++
>>  2 files changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clk/bcm/clk-kona.c b/drivers/clk/bcm/clk-kona.c
>> index d603c4e..d8a7f38 100644
>> --- a/drivers/clk/bcm/clk-kona.c
>> +++ b/drivers/clk/bcm/clk-kona.c
>> @@ -27,6 +27,9 @@
>>  #define CCU_ACCESS_PASSWORD      0xA5A500
>>  #define CLK_GATE_DELAY_LOOP      2000
>>  
>> +#define clk_is_initialized(_clk)       FLAG_TEST((_clk), KONA, INITIALIZED)
>> +#define clk_set_initialized(_clk)      FLAG_SET((_clk), KONA, INITIALIZED)
>> +
>>  /* Bitfield operations */
>>  
>>  /* Produces a mask of set bits covering a range of a 32-bit value */
>> @@ -1194,13 +1197,23 @@ static bool __peri_clk_init(struct kona_clk *bcm_clk)
>>  
>>  static bool __kona_clk_init(struct kona_clk *bcm_clk)
>>  {
>> +       bool ret;
>> +
>> +       if (clk_is_initialized(bcm_clk))
>> +               return true;
>> +
>>         switch (bcm_clk->type) {
>>         case bcm_clk_peri:
>> -               return __peri_clk_init(bcm_clk);
>> +               ret = __peri_clk_init(bcm_clk);
> 
> Hi Alex,
> 
> Going through this code, it's a bit hard to keep up ;-)

Here's how the structures are organized:
- A Kona clock is either a peripheral or a bus clock,
  but a lot of things are handled generically at the
  Kona level of abstraction.
- A peripheral clock has a selector (mux), up to two
  dividers, and a gate.  All of these are optional.
- A bus clock has a gate, which is optional.  (There
  are a few other things but I'm just ignoring them
  for now.)
- Each of these sort of sub-components (gate, divider,
  etc.) are handled on their own.  In other words, there
  are gate routines that operate on a gate regardless
  of whether it's on bus or peripheral clock.
- These sub-components are grouped like this because
  there are some weird gating requirements.  (I've
  said before I wanted to try to split things off where
  possible to use the common framework code more, but
  that opportunity hasn't arisen yet.)

> Does the call to __peri_clk_init enable the prereq clocks? If so, is
> their clk->prepare_count and clk->enable_count properly incremented?

My use of the term "enable" is imprecise.

The purpose of these *_init() routines is to set the hardware
to a known initial state.  Right now, *defining* what that
state should be has not been sent out for review, but that's
the reason it's set up this way.  The model is basically, when
you want to make a change, you record what the new value should
be and the *_commit() it.  Special values, used at initialization
time, indicate we don't have a desired value but we don't know
what the hardware is currently is set to either.  When those
special values (like BAD_SCALED_DIV_VALUE) are seen, the current
value is read from the hardware rather than written.

Anyway, to (hopefully) answer your question...

All of the prerequisite activity occurs in __kona_clk_init(),
which is called for every Kona clock.  The first thing that
does is call __kona_prereq_init(), in order to set up and
initialize (using __kona_clk_init()) the prerequisite clock
if there is one.

*After* the prerequisite clock is set up, the rest of the
original clock initialization occurs, by calling (e.g.)
__peri_clk_init().

Sorry to be so verbose but I think it helps to understand
the design underlying this stuff.

I'm going to be submitting a new version of this series
today.  It will pull out the clk_lookup field and
some comments that you spotted that are just dead code.

					-Alex

> Thanks,
> Mike
> 
>> +               break;
>>         default:
>> +               ret = false;
>>                 BUG();
>>         }
>> -       return -EINVAL;
>> +       if (ret)
>> +               clk_set_initialized(bcm_clk);
>> +
>> +       return ret;
>>  }
>>  
>>  /* Set a CCU and all its clocks into their desired initial state */
>> diff --git a/drivers/clk/bcm/clk-kona.h b/drivers/clk/bcm/clk-kona.h
>> index 2537b30..10e238d 100644
>> --- a/drivers/clk/bcm/clk-kona.h
>> +++ b/drivers/clk/bcm/clk-kona.h
>> @@ -406,6 +406,7 @@ 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;
>> +       u32 flags;              /* BCM_CLK_KONA_FLAGS_* below */
>>         union {
>>                 void *data;
>>                 struct peri_clk_data *peri;
>> @@ -414,6 +415,12 @@ struct kona_clk {
>>  #define to_kona_clk(_hw) \
>>         container_of(_hw, struct kona_clk, hw)
>>  
>> +/*
>> + * Kona clock flags:
>> + *   INITIALIZED       clock has been initialized already
>> + */
>> +#define BCM_CLK_KONA_FLAGS_INITIALIZED ((u32)1 << 0)   /* Clock initialized */
>> +
>>  /* Initialization macro for an entry in a CCU's kona_clks[] array. */
>>  #define KONA_CLK(_ccu_name, _clk_name, _type)                          \
>>         {                                                               \
>> -- 
>> 1.9.1
>>

[PATCH v2 2/5] clk: bcm281xx: implement prerequisite clocks

[Alex Elder]

Allow a clock to specify a "prerequisite" clock.  The prerequisite
clock must be initialized before the clock that depends on it.  A
prerequisite clock is defined initially by its name; as that clock
gets initialized the name gets replaced with a pointer to its clock
structure pointer.  In order to allow getting a reference to a clock
by its name we call clkdev_add() for each clock as it gets set up.
A new clk_lookup structure is added to the kona_clk type for this
purpose.

Rework the KONA_CLK() macro, and define a new KONA_CLK_PREREQ()
variant that allows a prerequisite clock to be specified.

There exist clocks that could specify more than one prequisite, but
almost all clocks only ever use one.  We can add support for more
than one if we find we need it at some point.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/clk/bcm/clk-kona-setup.c | 16 ++++++++++----
 drivers/clk/bcm/clk-kona.c       | 45 ++++++++++++++++++++++++++++++++++++++++
 drivers/clk/bcm/clk-kona.h       | 20 +++++++++++++++---
 3 files changed, 74 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/bcm/clk-kona-setup.c b/drivers/clk/bcm/clk-kona-setup.c
index e5aeded..fcce22c 100644
--- a/drivers/clk/bcm/clk-kona-setup.c
+++ b/drivers/clk/bcm/clk-kona-setup.c
@@ -686,6 +686,9 @@ peri_clk_setup(struct peri_clk_data *data, struct clk_init_data *init_data)
 
 static void bcm_clk_teardown(struct kona_clk *bcm_clk)
 {
+	/* There is no function defined for this (yet) */
+	/* clkdev_remove(&bcm_clk->cl); */
+
 	switch (bcm_clk->type) {
 	case bcm_clk_peri:
 		peri_clk_teardown(bcm_clk->u.data, &bcm_clk->init_data);
@@ -719,6 +722,7 @@ static void kona_clk_teardown(struct clk *clk)
 struct clk *kona_clk_setup(struct kona_clk *bcm_clk)
 {
 	struct clk_init_data *init_data = &bcm_clk->init_data;
+	const char *name = init_data->name;
 	struct clk *clk = NULL;
 
 	switch (bcm_clk->type) {
@@ -728,14 +732,13 @@ struct clk *kona_clk_setup(struct kona_clk *bcm_clk)
 		break;
 	default:
 		pr_err("%s: clock type %d invalid for %s\n", __func__,
-			(int)bcm_clk->type, init_data->name);
+			(int)bcm_clk->type, name);
 		return NULL;
 	}
 
 	/* Make sure everything makes sense before we set it up */
 	if (!kona_clk_valid(bcm_clk)) {
-		pr_err("%s: clock data invalid for %s\n", __func__,
-			init_data->name);
+		pr_err("%s: clock data invalid for %s\n", __func__, name);
 		goto out_teardown;
 	}
 
@@ -743,11 +746,16 @@ struct clk *kona_clk_setup(struct kona_clk *bcm_clk)
 	clk = clk_register(NULL, &bcm_clk->hw);
 	if (IS_ERR(clk)) {
 		pr_err("%s: error registering clock %s (%ld)\n", __func__,
-			init_data->name, PTR_ERR(clk));
+			name, PTR_ERR(clk));
 		goto out_teardown;
 	}
 	BUG_ON(!clk);
 
+	/*  Make it so we can look the clock up using clk_find() */
+	bcm_clk->cl.con_id = name;
+	bcm_clk->cl.clk = clk;
+	clkdev_add(&bcm_clk->cl);
+
 	return clk;
 out_teardown:
 	bcm_clk_teardown(bcm_clk);
diff --git a/drivers/clk/bcm/clk-kona.c b/drivers/clk/bcm/clk-kona.c
index d8a7f38..fd070d6 100644
--- a/drivers/clk/bcm/clk-kona.c
+++ b/drivers/clk/bcm/clk-kona.c
@@ -1195,6 +1195,48 @@ static bool __peri_clk_init(struct kona_clk *bcm_clk)
 	return true;
 }
 
+static bool __kona_clk_init(struct kona_clk *bcm_clk);
+static bool __kona_prereq_init(struct kona_clk *bcm_clk)
+{
+	struct clk *clk;
+	struct clk_hw *hw;
+	struct kona_clk *prereq;
+
+	BUG_ON(clk_is_initialized(bcm_clk));
+
+	if (!bcm_clk->p.prereq)
+		return true;
+
+	clk = clk_get(NULL, bcm_clk->p.prereq);
+	if (IS_ERR(clk)) {
+		pr_err("%s: unable to get prereq clock %s for %s\n",
+			__func__, bcm_clk->p.prereq, bcm_clk->init_data.name);
+		return false;
+	}
+	hw = __clk_get_hw(clk);
+	if (!hw) {
+		pr_err("%s: null hw pointer for clock %s\n", __func__,
+			bcm_clk->init_data.name);
+		return false;
+	}
+	prereq = to_kona_clk(hw);
+	if (prereq->ccu != bcm_clk->ccu) {
+		pr_err("%s: prereq clock %s CCU different for clock %s\n",
+			__func__, bcm_clk->p.prereq, bcm_clk->init_data.name);
+		return false;
+	}
+
+	/* Initialize the prerequisite clock first */
+	if (!__kona_clk_init(prereq)) {
+		pr_err("%s: failed to init prereq %s for clock %s\n",
+			__func__, bcm_clk->p.prereq, bcm_clk->init_data.name);
+		return false;
+	}
+	bcm_clk->p.prereq_clk = clk;
+
+	return true;
+}
+
 static bool __kona_clk_init(struct kona_clk *bcm_clk)
 {
 	bool ret;
@@ -1202,6 +1244,9 @@ static bool __kona_clk_init(struct kona_clk *bcm_clk)
 	if (clk_is_initialized(bcm_clk))
 		return true;
 
+	if (!__kona_prereq_init(bcm_clk))
+		return false;
+
 	switch (bcm_clk->type) {
 	case bcm_clk_peri:
 		ret = __peri_clk_init(bcm_clk);
diff --git a/drivers/clk/bcm/clk-kona.h b/drivers/clk/bcm/clk-kona.h
index 10e238d..a5b61e0 100644
--- a/drivers/clk/bcm/clk-kona.h
+++ b/drivers/clk/bcm/clk-kona.h
@@ -22,6 +22,8 @@
 #include <linux/device.h>
 #include <linux/of.h>
 #include <linux/clk-provider.h>
+#include <linux/clkdev.h>
+#include <linux/debugfs.h>
 
 #define	BILLION		1000000000
 
@@ -407,6 +409,11 @@ struct kona_clk {
 	struct ccu_data *ccu;	/* ccu this clock is associated with */
 	enum bcm_clk_type type;
 	u32 flags;		/* BCM_CLK_KONA_FLAGS_* below */
+	struct clk_lookup cl;
+	union {
+		const char *prereq;
+		struct clk *prereq_clk;
+	} p;
 	union {
 		void *data;
 		struct peri_clk_data *peri;
@@ -422,15 +429,22 @@ struct kona_clk {
 #define BCM_CLK_KONA_FLAGS_INITIALIZED	((u32)1 << 0)	/* Clock initialized */
 
 /* 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_PREREQ(_ccu_name, _clk_name, _type, _prereq)		\
+	{								\
+		.p.prereq	= #_prereq,				\
+		___KONA_CLK_COMMON(_ccu_name, _clk_name, _type),	\
+	}
+#define KONA_CLK(_ccu_name, _clk_name, _type)				\
+	{								\
+		___KONA_CLK_COMMON(_ccu_name, _clk_name, _type),	\
 	}
 #define LAST_KONA_CLK	{ .type = bcm_clk_none }
 
-- 
1.9.1

[PATCH v2 2/5] clk: bcm281xx: implement prerequisite clocks

[Mike Turquette]

Quoting Alex Elder (2014-05-20 05:52:39)
> @@ -743,11 +746,16 @@ struct clk *kona_clk_setup(struct kona_clk *bcm_clk)
>         clk = clk_register(NULL, &bcm_clk->hw);
>         if (IS_ERR(clk)) {
>                 pr_err("%s: error registering clock %s (%ld)\n", __func__,
> -                       init_data->name, PTR_ERR(clk));
> +                       name, PTR_ERR(clk));
>                 goto out_teardown;
>         }
>         BUG_ON(!clk);
>  
> +       /*  Make it so we can look the clock up using clk_find() */

s/clk_find/clk_get/ ?

> +       bcm_clk->cl.con_id = name;
> +       bcm_clk->cl.clk = clk;
> +       clkdev_add(&bcm_clk->cl);

This is not so nice. I'll explain more below.

> +
>         return clk;
>  out_teardown:
>         bcm_clk_teardown(bcm_clk);
> diff --git a/drivers/clk/bcm/clk-kona.c b/drivers/clk/bcm/clk-kona.c
> index d8a7f38..fd070d6 100644
> --- a/drivers/clk/bcm/clk-kona.c
> +++ b/drivers/clk/bcm/clk-kona.c
> @@ -1195,6 +1195,48 @@ static bool __peri_clk_init(struct kona_clk *bcm_clk)
>         return true;
>  }
>  
> +static bool __kona_clk_init(struct kona_clk *bcm_clk);
> +static bool __kona_prereq_init(struct kona_clk *bcm_clk)
> +{
> +       struct clk *clk;
> +       struct clk_hw *hw;
> +       struct kona_clk *prereq;
> +
> +       BUG_ON(clk_is_initialized(bcm_clk));
> +
> +       if (!bcm_clk->p.prereq)
> +               return true;
> +
> +       clk = clk_get(NULL, bcm_clk->p.prereq);

The clkdev global namespace is getting polluted with all of these new
prereq clocks. If there was an associated struct device *dev with them
then it wouldn't be a problem, but you might get collisions with other
clock drivers that also use NULL for the device.

It would be a lot nicer for the clocks that require a prereq clock to
just use clk_get(dev, "well_known_name"); in the same way that drivers
use it, without considering it a special case.

> +       if (IS_ERR(clk)) {
> +               pr_err("%s: unable to get prereq clock %s for %s\n",
> +                       __func__, bcm_clk->p.prereq, bcm_clk->init_data.name);
> +               return false;
> +       }
> +       hw = __clk_get_hw(clk);
> +       if (!hw) {
> +               pr_err("%s: null hw pointer for clock %s\n", __func__,
> +                       bcm_clk->init_data.name);
> +               return false;
> +       }
> +       prereq = to_kona_clk(hw);
> +       if (prereq->ccu != bcm_clk->ccu) {
> +               pr_err("%s: prereq clock %s CCU different for clock %s\n",
> +                       __func__, bcm_clk->p.prereq, bcm_clk->init_data.name);
> +               return false;
> +       }
> +
> +       /* Initialize the prerequisite clock first */
> +       if (!__kona_clk_init(prereq)) {
> +               pr_err("%s: failed to init prereq %s for clock %s\n",
> +                       __func__, bcm_clk->p.prereq, bcm_clk->init_data.name);
> +               return false;
> +       }
> +       bcm_clk->p.prereq_clk = clk;

The above seems like a lot effort to go to. Why not skip all of this and
just implement the prerequisite logic in the .enable & .disable
callbacks? E.g. your kona clk .enable callback would look like:

diff --git a/drivers/clk/bcm/clk-kona.c b/drivers/clk/bcm/clk-kona.c
index d603c4e..51f35b4 100644
--- a/drivers/clk/bcm/clk-kona.c
+++ b/drivers/clk/bcm/clk-kona.c
@@ -987,6 +987,12 @@ static int kona_peri_clk_enable(struct clk_hw *hw)
 {
 	struct kona_clk *bcm_clk = to_kona_clk(hw);
 	struct bcm_clk_gate *gate = &bcm_clk->u.peri->gate;
+	int ret;
+
+	hw->prereq_bus_clk = clk_get(hw->ccu, hw->prereq);
+	ret = clk_enable(prereq_bus_clk);
+	if (ret)
+		return ret;
 
 	return clk_gate(bcm_clk->ccu, bcm_clk->init_data.name, gate, true);
 }
@@ -997,6 +1003,9 @@ static void kona_peri_clk_disable(struct clk_hw *hw)
 	struct bcm_clk_gate *gate = &bcm_clk->u.peri->gate;
 
 	(void)clk_gate(bcm_clk->ccu, bcm_clk->init_data.name, gate, false);
+
+	clk_disable(hw->prereq_bus_clk);
+	clk_put(hw->prereq_bus_clk);
 }
 
 static int kona_peri_clk_is_enabled(struct clk_hw *hw)


I guess it might take some trickery to get clk_get to work like that.
Let me know if I've completely lost the plot.

Regards,
Mike

[PATCH v2 2/5] clk: bcm281xx: implement prerequisite clocks

[Alex Elder]

On 05/23/2014 07:53 PM, Mike Turquette wrote:
> Quoting Alex Elder (2014-05-20 05:52:39)
>> @@ -743,11 +746,16 @@ struct clk *kona_clk_setup(struct kona_clk *bcm_clk)
>>         clk = clk_register(NULL, &bcm_clk->hw);
>>         if (IS_ERR(clk)) {
>>                 pr_err("%s: error registering clock %s (%ld)\n", __func__,
>> -                       init_data->name, PTR_ERR(clk));
>> +                       name, PTR_ERR(clk));
>>                 goto out_teardown;
>>         }
>>         BUG_ON(!clk);
>>  
>> +       /*  Make it so we can look the clock up using clk_find() */
> 
> s/clk_find/clk_get/ ?

Yes, this is a mistake.

> 
>> +       bcm_clk->cl.con_id = name;
>> +       bcm_clk->cl.clk = clk;
>> +       clkdev_add(&bcm_clk->cl);
> 
> This is not so nice. I'll explain more below.

Actually, this code is no longer needed at all.  It was
at one time, but I evolved away from that need, and never
noticed that this remnant remained.  I will delete it.

I'm really sorry I missed that, it was confusing for
it to still be there I'm sure.

>> +
>>         return clk;
>>  out_teardown:
>>         bcm_clk_teardown(bcm_clk);
>> diff --git a/drivers/clk/bcm/clk-kona.c b/drivers/clk/bcm/clk-kona.c
>> index d8a7f38..fd070d6 100644
>> --- a/drivers/clk/bcm/clk-kona.c
>> +++ b/drivers/clk/bcm/clk-kona.c
>> @@ -1195,6 +1195,48 @@ static bool __peri_clk_init(struct kona_clk *bcm_clk)
>>         return true;
>>  }
>>  
>> +static bool __kona_clk_init(struct kona_clk *bcm_clk);
>> +static bool __kona_prereq_init(struct kona_clk *bcm_clk)
>> +{
>> +       struct clk *clk;
>> +       struct clk_hw *hw;
>> +       struct kona_clk *prereq;
>> +
>> +       BUG_ON(clk_is_initialized(bcm_clk));
>> +
>> +       if (!bcm_clk->p.prereq)
>> +               return true;
>> +
>> +       clk = clk_get(NULL, bcm_clk->p.prereq);
> 
> The clkdev global namespace is getting polluted with all of these new
> prereq clocks. If there was an associated struct device *dev with them
> then it wouldn't be a problem, but you might get collisions with other
> clock drivers that also use NULL for the device.

Again, you caught a confusing mistake.  The clk_lookup
structure will go away.

> It would be a lot nicer for the clocks that require a prereq clock to
> just use clk_get(dev, "well_known_name"); in the same way that drivers
> use it, without considering it a special case.

That is in fact what happens, in __kona_prereq_init().

>> +       if (IS_ERR(clk)) {
>> +               pr_err("%s: unable to get prereq clock %s for %s\n",
>> +                       __func__, bcm_clk->p.prereq, bcm_clk->init_data.name);
>> +               return false;
>> +       }
>> +       hw = __clk_get_hw(clk);
>> +       if (!hw) {
>> +               pr_err("%s: null hw pointer for clock %s\n", __func__,
>> +                       bcm_clk->init_data.name);
>> +               return false;
>> +       }
>> +       prereq = to_kona_clk(hw);
>> +       if (prereq->ccu != bcm_clk->ccu) {
>> +               pr_err("%s: prereq clock %s CCU different for clock %s\n",
>> +                       __func__, bcm_clk->p.prereq, bcm_clk->init_data.name);
>> +               return false;
>> +       }
>> +
>> +       /* Initialize the prerequisite clock first */
>> +       if (!__kona_clk_init(prereq)) {
>> +               pr_err("%s: failed to init prereq %s for clock %s\n",
>> +                       __func__, bcm_clk->p.prereq, bcm_clk->init_data.name);
>> +               return false;
>> +       }
>> +       bcm_clk->p.prereq_clk = clk;
> 
> The above seems like a lot effort to go to. Why not skip all of this and
> just implement the prerequisite logic in the .enable & .disable
> callbacks? E.g. your kona clk .enable callback would look like:

I think the problem is that it means the clock consumers
would have to know that prerequisite relationship.  And
that is dependent on the clock tree.  The need for it in
this case was because the boot loader didn't initialize
all the clocks that were needed.  If we could count on
the boot loader setting things up initially we might not
need to do this.

> 
> diff --git a/drivers/clk/bcm/clk-kona.c b/drivers/clk/bcm/clk-kona.c
> index d603c4e..51f35b4 100644
> --- a/drivers/clk/bcm/clk-kona.c
> +++ b/drivers/clk/bcm/clk-kona.c
> @@ -987,6 +987,12 @@ static int kona_peri_clk_enable(struct clk_hw *hw)
>  {
>  	struct kona_clk *bcm_clk = to_kona_clk(hw);
>  	struct bcm_clk_gate *gate = &bcm_clk->u.peri->gate;
> +	int ret;
> +
> +	hw->prereq_bus_clk = clk_get(hw->ccu, hw->prereq);
> +	ret = clk_enable(prereq_bus_clk);
> +	if (ret)
> +		return ret;
>  
>  	return clk_gate(bcm_clk->ccu, bcm_clk->init_data.name, gate, true);
>  }
> @@ -997,6 +1003,9 @@ static void kona_peri_clk_disable(struct clk_hw *hw)
>  	struct bcm_clk_gate *gate = &bcm_clk->u.peri->gate;
>  
>  	(void)clk_gate(bcm_clk->ccu, bcm_clk->init_data.name, gate, false);
> +
> +	clk_disable(hw->prereq_bus_clk);
> +	clk_put(hw->prereq_bus_clk);
>  }
>  
>  static int kona_peri_clk_is_enabled(struct clk_hw *hw)
> 
> 
> I guess it might take some trickery to get clk_get to work like that.
> Let me know if I've completely lost the plot.

I don't think so, but I think there's a lot of stuff
here to try to understand, and you're trying to extract
it from the code without the benefit of some background
of how and why it's done this way.

Hopefully all this verbiage is moving you closer to
understanding...  I appreciate your patience.

					-Alex

[PATCH v2 2/5] clk: bcm281xx: implement prerequisite clocks

[Mike Turquette]

Quoting Alex Elder (2014-05-29 06:26:15)
> On 05/23/2014 07:53 PM, Mike Turquette wrote:
> > The above seems like a lot effort to go to. Why not skip all of this and
> > just implement the prerequisite logic in the .enable & .disable
> > callbacks? E.g. your kona clk .enable callback would look like:
> 
> I think the problem is that it means the clock consumers
> would have to know that prerequisite relationship.  And
> that is dependent on the clock tree.  The need for it in
> this case was because the boot loader didn't initialize
> all the clocks that were needed.  If we could count on
> the boot loader setting things up initially we might not
> need to do this.
> 
> > 
> > diff --git a/drivers/clk/bcm/clk-kona.c b/drivers/clk/bcm/clk-kona.c
> > index d603c4e..51f35b4 100644
> > --- a/drivers/clk/bcm/clk-kona.c
> > +++ b/drivers/clk/bcm/clk-kona.c
> > @@ -987,6 +987,12 @@ static int kona_peri_clk_enable(struct clk_hw *hw)
> >  {
> >       struct kona_clk *bcm_clk = to_kona_clk(hw);
> >       struct bcm_clk_gate *gate = &bcm_clk->u.peri->gate;
> > +     int ret;
> > +
> > +     hw->prereq_bus_clk = clk_get(hw->ccu, hw->prereq);
> > +     ret = clk_enable(prereq_bus_clk);
> > +     if (ret)
> > +             return ret;
> >  
> >       return clk_gate(bcm_clk->ccu, bcm_clk->init_data.name, gate, true);
> >  }
> > @@ -997,6 +1003,9 @@ static void kona_peri_clk_disable(struct clk_hw *hw)
> >       struct bcm_clk_gate *gate = &bcm_clk->u.peri->gate;
> >  
> >       (void)clk_gate(bcm_clk->ccu, bcm_clk->init_data.name, gate, false);
> > +
> > +     clk_disable(hw->prereq_bus_clk);
> > +     clk_put(hw->prereq_bus_clk);
> >  }
> >  
> >  static int kona_peri_clk_is_enabled(struct clk_hw *hw)
> > 
> > 
> > I guess it might take some trickery to get clk_get to work like that.
> > Let me know if I've completely lost the plot.
> 
> I don't think so, but I think there's a lot of stuff
> here to try to understand, and you're trying to extract
> it from the code without the benefit of some background
> of how and why it's done this way.
> 
> Hopefully all this verbiage is moving you closer to
> understanding...  I appreciate your patience.

Hi Alex,

Can you comment on my diff above? I basically tossed up some pseudo-code
to show how clk_enable calls can be nested inside of each other. I'd
like to know if that approach makes sense for your prereq clocks case.

Note that Linux device drivers that consume leaf clocks do NOT need to
know about the prereq clocks. All of that prereq clock knowledge is
stored in the .enable callback for the leaf clock (see above).

Regards,
Mike

> 
>                                         -Alex
> 

[PATCH v2 2/5] clk: bcm281xx: implement prerequisite clocks

[Alex Elder]

On 05/29/2014 11:35 AM, Mike Turquette wrote:
> Quoting Alex Elder (2014-05-29 06:26:15)
>> On 05/23/2014 07:53 PM, Mike Turquette wrote:
>>> The above seems like a lot effort to go to. Why not skip all of this and
>>> just implement the prerequisite logic in the .enable & .disable
>>> callbacks? E.g. your kona clk .enable callback would look like:
>>
>> I think the problem is that it means the clock consumers
>> would have to know that prerequisite relationship.  And
>> that is dependent on the clock tree.  The need for it in
>> this case was because the boot loader didn't initialize
>> all the clocks that were needed.  If we could count on
>> the boot loader setting things up initially we might not
>> need to do this.

I think you've convinced me that if the prerequisite is
set up at initialization time, the consumers don't need
to know about the the clock tree.

>>>
>>> diff --git a/drivers/clk/bcm/clk-kona.c b/drivers/clk/bcm/clk-kona.c
>>> index d603c4e..51f35b4 100644
>>> --- a/drivers/clk/bcm/clk-kona.c
>>> +++ b/drivers/clk/bcm/clk-kona.c
>>> @@ -987,6 +987,12 @@ static int kona_peri_clk_enable(struct clk_hw *hw)
>>>  {
>>>       struct kona_clk *bcm_clk = to_kona_clk(hw);
>>>       struct bcm_clk_gate *gate = &bcm_clk->u.peri->gate;
>>> +     int ret;
>>> +
>>> +     hw->prereq_bus_clk = clk_get(hw->ccu, hw->prereq);
>>> +     ret = clk_enable(prereq_bus_clk);
>>> +     if (ret)
>>> +             return ret;
>>>  
>>>       return clk_gate(bcm_clk->ccu, bcm_clk->init_data.name, gate, true);
>>>  }
>>> @@ -997,6 +1003,9 @@ static void kona_peri_clk_disable(struct clk_hw *hw)
>>>       struct bcm_clk_gate *gate = &bcm_clk->u.peri->gate;
>>>  
>>>       (void)clk_gate(bcm_clk->ccu, bcm_clk->init_data.name, gate, false);
>>> +
>>> +     clk_disable(hw->prereq_bus_clk);
>>> +     clk_put(hw->prereq_bus_clk);
>>>  }
>>>  
>>>  static int kona_peri_clk_is_enabled(struct clk_hw *hw)
>>>
>>>
>>> I guess it might take some trickery to get clk_get to work like that.
>>> Let me know if I've completely lost the plot.
>>
>> I don't think so, but I think there's a lot of stuff
>> here to try to understand, and you're trying to extract
>> it from the code without the benefit of some background
>> of how and why it's done this way.
>>
>> Hopefully all this verbiage is moving you closer to
>> understanding...  I appreciate your patience.
> 
> Hi Alex,
> 
> Can you comment on my diff above? I basically tossed up some pseudo-code
> to show how clk_enable calls can be nested inside of each other. I'd
> like to know if that approach makes sense for your prereq clocks case.

Yes, I should have looked more closely before.

Are you suggesting this prerequisite notion get elevated into the
common framework?  Or is "hw" here just representative of the
Kona-specific clock structure?

In any case, you're suggesting the prerequisite be handled in the
enable path (as opposed to the one-time initialization path),
which during the course of this discussion I've been thinking may
be the right way to do it.

Let me see if I can rework it that way and I'll let you know
what I discover as a result.  I hope to have something to
talk about later today.

Thanks a lot Mike.

					-Alex

> Note that Linux device drivers that consume leaf clocks do NOT need to
> know about the prereq clocks. All of that prereq clock knowledge is
> stored in the .enable callback for the leaf clock (see above).
> 
> Regards,
> Mike
> 
>>
>>                                         -Alex
>>

[PATCH v2 2/5] clk: bcm281xx: implement prerequisite clocks

[Mike Turquette]

Quoting Alex Elder (2014-05-29 09:53:50)
> On 05/29/2014 11:35 AM, Mike Turquette wrote:
> > Quoting Alex Elder (2014-05-29 06:26:15)
> >> On 05/23/2014 07:53 PM, Mike Turquette wrote:
> >>> The above seems like a lot effort to go to. Why not skip all of this and
> >>> just implement the prerequisite logic in the .enable & .disable
> >>> callbacks? E.g. your kona clk .enable callback would look like:
> >>
> >> I think the problem is that it means the clock consumers
> >> would have to know that prerequisite relationship.  And
> >> that is dependent on the clock tree.  The need for it in
> >> this case was because the boot loader didn't initialize
> >> all the clocks that were needed.  If we could count on
> >> the boot loader setting things up initially we might not
> >> need to do this.
> 
> I think you've convinced me that if the prerequisite is
> set up at initialization time, the consumers don't need
> to know about the the clock tree.
> 
> >>>
> >>> diff --git a/drivers/clk/bcm/clk-kona.c b/drivers/clk/bcm/clk-kona.c
> >>> index d603c4e..51f35b4 100644
> >>> --- a/drivers/clk/bcm/clk-kona.c
> >>> +++ b/drivers/clk/bcm/clk-kona.c
> >>> @@ -987,6 +987,12 @@ static int kona_peri_clk_enable(struct clk_hw *hw)
> >>>  {
> >>>       struct kona_clk *bcm_clk = to_kona_clk(hw);
> >>>       struct bcm_clk_gate *gate = &bcm_clk->u.peri->gate;
> >>> +     int ret;
> >>> +
> >>> +     hw->prereq_bus_clk = clk_get(hw->ccu, hw->prereq);
> >>> +     ret = clk_enable(prereq_bus_clk);
> >>> +     if (ret)
> >>> +             return ret;
> >>>  
> >>>       return clk_gate(bcm_clk->ccu, bcm_clk->init_data.name, gate, true);
> >>>  }
> >>> @@ -997,6 +1003,9 @@ static void kona_peri_clk_disable(struct clk_hw *hw)
> >>>       struct bcm_clk_gate *gate = &bcm_clk->u.peri->gate;
> >>>  
> >>>       (void)clk_gate(bcm_clk->ccu, bcm_clk->init_data.name, gate, false);
> >>> +
> >>> +     clk_disable(hw->prereq_bus_clk);
> >>> +     clk_put(hw->prereq_bus_clk);
> >>>  }
> >>>  
> >>>  static int kona_peri_clk_is_enabled(struct clk_hw *hw)
> >>>
> >>>
> >>> I guess it might take some trickery to get clk_get to work like that.
> >>> Let me know if I've completely lost the plot.
> >>
> >> I don't think so, but I think there's a lot of stuff
> >> here to try to understand, and you're trying to extract
> >> it from the code without the benefit of some background
> >> of how and why it's done this way.
> >>
> >> Hopefully all this verbiage is moving you closer to
> >> understanding...  I appreciate your patience.
> > 
> > Hi Alex,
> > 
> > Can you comment on my diff above? I basically tossed up some pseudo-code
> > to show how clk_enable calls can be nested inside of each other. I'd
> > like to know if that approach makes sense for your prereq clocks case.
> 
> Yes, I should have looked more closely before.
> 
> Are you suggesting this prerequisite notion get elevated into the
> common framework?

Nope.

> Or is "hw" here just representative of the
> Kona-specific clock structure?

Yup. It's just good old struct clk_hw. There is one instance of this
struct for every struct clk object.

> 
> In any case, you're suggesting the prerequisite be handled in the
> enable path (as opposed to the one-time initialization path),
> which during the course of this discussion I've been thinking may
> be the right way to do it.

Right, and don't forget that you have both the prepare path AND the
enable path. It is common for drivers to call clk_prepare once at probe
time and then aggressively call clk_enable/clk_disable for fine-grained
PM. Likewise some drivers always use clk_prepare_enable and
clk_disable_unprepare.

The point is that you have two callbacks that you might split some of
this stuff across. Your "initializiation" stuff might go into .prepare()
and simply enabling the clock might go into .enable().

> 
> Let me see if I can rework it that way and I'll let you know
> what I discover as a result.  I hope to have something to
> talk about later today.

Sounds good.

Regards,
Mike

> 
> Thanks a lot Mike.
> 
>                                         -Alex
> 
> > Note that Linux device drivers that consume leaf clocks do NOT need to
> > know about the prereq clocks. All of that prereq clock knowledge is
> > stored in the .enable callback for the leaf clock (see above).
> > 
> > Regards,
> > Mike
> > 
> >>
> >>                                         -Alex
> >>
> 

[PATCH v2 2/5] clk: bcm281xx: implement prerequisite clocks

[Alex Elder]

On 05/23/2014 07:53 PM, Mike Turquette wrote:
> Quoting Alex Elder (2014-05-20 05:52:39)
>> @@ -743,11 +746,16 @@ struct clk *kona_clk_setup(struct kona_clk *bcm_clk)
>>         clk = clk_register(NULL, &bcm_clk->hw);
>>         if (IS_ERR(clk)) {
>>                 pr_err("%s: error registering clock %s (%ld)\n", __func__,
>> -                       init_data->name, PTR_ERR(clk));
>> +                       name, PTR_ERR(clk));
>>                 goto out_teardown;
>>         }
>>         BUG_ON(!clk);
>>  
>> +       /*  Make it so we can look the clock up using clk_find() */
> 
> s/clk_find/clk_get/ ?
> 
>> +       bcm_clk->cl.con_id = name;
>> +       bcm_clk->cl.clk = clk;
>> +       clkdev_add(&bcm_clk->cl);
> 
> This is not so nice. I'll explain more below.

OK, despite what I said before, I do need this, or
something like it, so I can look up clocks by name.
(Continued below.)

> 
>> +
>>         return clk;
>>  out_teardown:
>>         bcm_clk_teardown(bcm_clk);
>> diff --git a/drivers/clk/bcm/clk-kona.c b/drivers/clk/bcm/clk-kona.c
>> index d8a7f38..fd070d6 100644
>> --- a/drivers/clk/bcm/clk-kona.c
>> +++ b/drivers/clk/bcm/clk-kona.c
>> @@ -1195,6 +1195,48 @@ static bool __peri_clk_init(struct kona_clk *bcm_clk)
>>         return true;
>>  }
>>  
>> +static bool __kona_clk_init(struct kona_clk *bcm_clk);
>> +static bool __kona_prereq_init(struct kona_clk *bcm_clk)
>> +{
>> +       struct clk *clk;
>> +       struct clk_hw *hw;
>> +       struct kona_clk *prereq;
>> +
>> +       BUG_ON(clk_is_initialized(bcm_clk));
>> +
>> +       if (!bcm_clk->p.prereq)
>> +               return true;
>> +
>> +       clk = clk_get(NULL, bcm_clk->p.prereq);
> 
> The clkdev global namespace is getting polluted with all of these new
> prereq clocks. If there was an associated struct device *dev with them
> then it wouldn't be a problem, but you might get collisions with other
> clock drivers that also use NULL for the device.

Yes I recognize this.  Ideally a CCU would have a device struct
associated with it that I could use, because the name of a clock
is unique within that context.  But I have no such device available.
(Please correct me if I'm wrong.  I don't want to make one up, and
I would like to use it if it exists.)

> It would be a lot nicer for the clocks that require a prereq clock to
> just use clk_get(dev, "well_known_name"); in the same way that drivers
> use it, without considering it a special case.

I can do something like that if I can get a meaningful device
structure.  Do you have any suggestions?

Other than this issue, I've implemented all of the previous
initialization routines using ->prepare() instead, and it
works fine.  I'm going to send an updated series out tomorrow.
I want to look it over again after a good night's sleep...

					-Alex

>> +       if (IS_ERR(clk)) {
>> +               pr_err("%s: unable to get prereq clock %s for %s\n",
>> +                       __func__, bcm_clk->p.prereq, bcm_clk->init_data.name);
>> +               return false;
>> +       }
>> +       hw = __clk_get_hw(clk);
>> +       if (!hw) {
>> +               pr_err("%s: null hw pointer for clock %s\n", __func__,
>> +                       bcm_clk->init_data.name);
>> +               return false;
>> +       }
>> +       prereq = to_kona_clk(hw);
>> +       if (prereq->ccu != bcm_clk->ccu) {
>> +               pr_err("%s: prereq clock %s CCU different for clock %s\n",
>> +                       __func__, bcm_clk->p.prereq, bcm_clk->init_data.name);
>> +               return false;
>> +       }
>> +
>> +       /* Initialize the prerequisite clock first */
>> +       if (!__kona_clk_init(prereq)) {
>> +               pr_err("%s: failed to init prereq %s for clock %s\n",
>> +                       __func__, bcm_clk->p.prereq, bcm_clk->init_data.name);
>> +               return false;
>> +       }
>> +       bcm_clk->p.prereq_clk = clk;
> 
> The above seems like a lot effort to go to. Why not skip all of this and
> just implement the prerequisite logic in the .enable & .disable
> callbacks? E.g. your kona clk .enable callback would look like:
> 
> diff --git a/drivers/clk/bcm/clk-kona.c b/drivers/clk/bcm/clk-kona.c
> index d603c4e..51f35b4 100644
> --- a/drivers/clk/bcm/clk-kona.c
> +++ b/drivers/clk/bcm/clk-kona.c
> @@ -987,6 +987,12 @@ static int kona_peri_clk_enable(struct clk_hw *hw)
>  {
>  	struct kona_clk *bcm_clk = to_kona_clk(hw);
>  	struct bcm_clk_gate *gate = &bcm_clk->u.peri->gate;
> +	int ret;
> +
> +	hw->prereq_bus_clk = clk_get(hw->ccu, hw->prereq);
> +	ret = clk_enable(prereq_bus_clk);
> +	if (ret)
> +		return ret;
>  
>  	return clk_gate(bcm_clk->ccu, bcm_clk->init_data.name, gate, true);
>  }
> @@ -997,6 +1003,9 @@ static void kona_peri_clk_disable(struct clk_hw *hw)
>  	struct bcm_clk_gate *gate = &bcm_clk->u.peri->gate;
>  
>  	(void)clk_gate(bcm_clk->ccu, bcm_clk->init_data.name, gate, false);
> +
> +	clk_disable(hw->prereq_bus_clk);
> +	clk_put(hw->prereq_bus_clk);
>  }
>  
>  static int kona_peri_clk_is_enabled(struct clk_hw *hw)
> 
> 
> I guess it might take some trickery to get clk_get to work like that.
> Let me know if I've completely lost the plot.
> 
> Regards,
> Mike
> 

[PATCH v2 2/5] clk: bcm281xx: implement prerequisite clocks

[Alex Elder]

On 05/29/2014 10:20 PM, Alex Elder wrote:
> On 05/23/2014 07:53 PM, Mike Turquette wrote:
>> Quoting Alex Elder (2014-05-20 05:52:39)
>>> @@ -743,11 +746,16 @@ struct clk *kona_clk_setup(struct kona_clk *bcm_clk)
>>>         clk = clk_register(NULL, &bcm_clk->hw);
>>>         if (IS_ERR(clk)) {
>>>                 pr_err("%s: error registering clock %s (%ld)\n", __func__,
>>> -                       init_data->name, PTR_ERR(clk));
>>> +                       name, PTR_ERR(clk));
>>>                 goto out_teardown;
>>>         }
>>>         BUG_ON(!clk);
>>>  
>>> +       /*  Make it so we can look the clock up using clk_find() */
>>
>> s/clk_find/clk_get/ ?
>>
>>> +       bcm_clk->cl.con_id = name;
>>> +       bcm_clk->cl.clk = clk;
>>> +       clkdev_add(&bcm_clk->cl);
>>
>> This is not so nice. I'll explain more below.
> 
> OK, despite what I said before, I do need this, or
> something like it, so I can look up clocks by name.
> (Continued below.)
...

I've been thinking this morning about ways to at least
improve this.  The problem is worse than just prerequisite
clocks polluting the global name space.  Right now *all*
clocks get their name registered this way, because any one
of them could be tagged as a prerequisite, and therefore
in need of lookup by name.

If I had a device structure to associate the clock names
with it would help, but I don't have one.  There is no
other way to define a separate name space, it's either
associated with a device, or it's global.

Given all that, I could prefix or suffix the clock names
with some special string, in order to sort of carve out a
reserved portion of the global name space.

I could specify the prerequisite clock by its index in
its CCU's clocks array.  I could then manufacture a
of_phandle_args structure and use of_clk_get_from_provider()
to look up what we need, but that seems kind of kludgy.
Maybe a new function could encapsulate the messy details
of that.

Do you have any suggestions?  I can create some new
common code if appropriate, but only if it represents
missing functionality that's generally useful.

					-Alex

[PATCH v2 3/5] clk: bcm281xx: add bus clock support

[Alex Elder]

Add bus clock support.  A bus clock has a subset of the components
present in a peripheral clock (again, all optional): a gate; CCU
policy management bits; and if needed, bits to control hysteresis.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/clk/bcm/clk-kona-setup.c | 96 ++++++++++++++++++++++++++++++++++++++--
 drivers/clk/bcm/clk-kona.c       | 58 ++++++++++++++++++++++++
 drivers/clk/bcm/clk-kona.h       |  8 ++++
 3 files changed, 159 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/bcm/clk-kona-setup.c b/drivers/clk/bcm/clk-kona-setup.c
index fcce22c..60d18f0 100644
--- a/drivers/clk/bcm/clk-kona-setup.c
+++ b/drivers/clk/bcm/clk-kona-setup.c
@@ -76,6 +76,56 @@ static bool clk_requires_trigger(struct kona_clk *bcm_clk)
 	return divider_exists(div) && !divider_is_fixed(div);
 }
 
+static bool bus_clk_data_offsets_valid(struct kona_clk *bcm_clk)
+{
+	struct bus_clk_data *bus;
+	struct bcm_clk_policy *policy;
+	struct bcm_clk_gate *gate;
+	struct bcm_clk_hyst *hyst;
+	const char *name;
+	u32 limit;
+
+	BUG_ON(bcm_clk->type != bcm_clk_bus);
+	bus = bcm_clk->u.bus;
+	name = bcm_clk->init_data.name;
+
+	limit = bcm_clk->ccu->range - sizeof(u32);
+	limit = round_down(limit, sizeof(u32));
+
+	policy = &bus->policy;
+	if (policy_exists(policy)) {
+		if (policy->offset > limit) {
+			pr_err("%s: bad policy offset for %s (%u > %u)\n",
+				__func__, name, policy->offset, limit);
+			return false;
+		}
+	}
+
+	gate = &bus->gate;
+	hyst = &bus->hyst;
+	if (gate_exists(gate)) {
+		if (gate->offset > limit) {
+			pr_err("%s: bad gate offset for %s (%u > %u)\n",
+				__func__, name, gate->offset, limit);
+			return false;
+		}
+		if (hyst_exists(hyst)) {
+			if (hyst->offset > limit) {
+				pr_err("%s: bad hysteresis offset for %s "
+					"(%u > %u)\n", __func__,
+					name, hyst->offset, limit);
+				return false;
+			}
+		}
+	} else if (hyst_exists(hyst)) {
+		pr_err("%s: hysteresis but no gate for %s\n", __func__, name);
+		return false;
+	}
+
+
+	return true;
+}
+
 static bool peri_clk_data_offsets_valid(struct kona_clk *bcm_clk)
 {
 	struct peri_clk_data *peri;
@@ -86,15 +136,13 @@ static bool peri_clk_data_offsets_valid(struct kona_clk *bcm_clk)
 	struct bcm_clk_sel *sel;
 	struct bcm_clk_trig *trig;
 	const char *name;
-	u32 range;
 	u32 limit;
 
 	BUG_ON(bcm_clk->type != bcm_clk_peri);
 	peri = bcm_clk->u.peri;
 	name = bcm_clk->init_data.name;
-	range = bcm_clk->ccu->range;
 
-	limit = range - sizeof(u32);
+	limit = bcm_clk->ccu->range - sizeof(u32);
 	limit = round_down(limit, sizeof(u32));
 
 	policy = &peri->policy;
@@ -397,6 +445,23 @@ static bool trig_valid(struct bcm_clk_trig *trig, const char *field_name,
 	return bit_posn_valid(trig->bit, field_name, clock_name);
 }
 
+/* Determine whether the set of bus clock registers are valid. */
+static bool
+bus_clk_data_valid(struct kona_clk *bcm_clk)
+{
+	struct bcm_clk_gate *gate;
+
+	BUG_ON(bcm_clk->type != bcm_clk_bus);
+	if (!bus_clk_data_offsets_valid(bcm_clk))
+		return false;
+
+	gate = &bcm_clk->u.bus->gate;
+	if (!gate_exists(gate))
+		return true;
+
+	return gate_valid(gate, "gate", bcm_clk->init_data.name);
+}
+
 /* Determine whether the set of peripheral clock registers are valid. */
 static bool
 peri_clk_data_valid(struct kona_clk *bcm_clk)
@@ -494,6 +559,10 @@ peri_clk_data_valid(struct kona_clk *bcm_clk)
 static bool kona_clk_valid(struct kona_clk *bcm_clk)
 {
 	switch (bcm_clk->type) {
+	case bcm_clk_bus:
+		if (!bus_clk_data_valid(bcm_clk))
+			return false;
+		break;
 	case bcm_clk_peri:
 		if (!peri_clk_data_valid(bcm_clk))
 			return false;
@@ -664,6 +733,20 @@ static void clk_sel_teardown(struct bcm_clk_sel *sel,
 	init_data->parent_names = NULL;
 }
 
+static void bus_clk_teardown(struct bus_clk_data *data,
+				struct clk_init_data *init_data)
+{
+	/* Nothing to do */
+}
+
+static int
+bus_clk_setup(struct bus_clk_data *data, struct clk_init_data *init_data)
+{
+	init_data->flags = CLK_IGNORE_UNUSED;
+
+	return 0;
+}
+
 static void peri_clk_teardown(struct peri_clk_data *data,
 				struct clk_init_data *init_data)
 {
@@ -690,6 +773,9 @@ static void bcm_clk_teardown(struct kona_clk *bcm_clk)
 	/* clkdev_remove(&bcm_clk->cl); */
 
 	switch (bcm_clk->type) {
+	case bcm_clk_bus:
+		bus_clk_teardown(bcm_clk->u.data, &bcm_clk->init_data);
+		break;
 	case bcm_clk_peri:
 		peri_clk_teardown(bcm_clk->u.data, &bcm_clk->init_data);
 		break;
@@ -726,6 +812,10 @@ struct clk *kona_clk_setup(struct kona_clk *bcm_clk)
 	struct clk *clk = NULL;
 
 	switch (bcm_clk->type) {
+	case bcm_clk_bus:
+		if (bus_clk_setup(bcm_clk->u.data, init_data))
+			return NULL;
+		break;
 	case bcm_clk_peri:
 		if (peri_clk_setup(bcm_clk->u.data, init_data))
 			return NULL;
diff --git a/drivers/clk/bcm/clk-kona.c b/drivers/clk/bcm/clk-kona.c
index fd070d6..b3d556c 100644
--- a/drivers/clk/bcm/clk-kona.c
+++ b/drivers/clk/bcm/clk-kona.c
@@ -986,6 +986,36 @@ static int selector_write(struct ccu_data *ccu, struct bcm_clk_gate *gate,
 
 /* Clock operations */
 
+static int kona_bus_clk_enable(struct clk_hw *hw)
+{
+	struct kona_clk *bcm_clk = to_kona_clk(hw);
+	struct bcm_clk_gate *gate = &bcm_clk->u.bus->gate;
+
+	return clk_gate(bcm_clk->ccu, bcm_clk->init_data.name, gate, true);
+}
+
+static void kona_bus_clk_disable(struct clk_hw *hw)
+{
+	struct kona_clk *bcm_clk = to_kona_clk(hw);
+	struct bcm_clk_gate *gate = &bcm_clk->u.bus->gate;
+
+	(void)clk_gate(bcm_clk->ccu, bcm_clk->init_data.name, gate, false);
+}
+
+static int kona_bus_clk_is_enabled(struct clk_hw *hw)
+{
+	struct kona_clk *bcm_clk = to_kona_clk(hw);
+	struct bcm_clk_gate *gate = &bcm_clk->u.bus->gate;
+
+	return is_clk_gate_enabled(bcm_clk->ccu, gate) ? 1 : 0;
+}
+
+struct clk_ops kona_bus_clk_ops = {
+	.enable = kona_bus_clk_enable,
+	.disable = kona_bus_clk_disable,
+	.is_enabled = kona_bus_clk_is_enabled,
+};
+
 static int kona_peri_clk_enable(struct clk_hw *hw)
 {
 	struct kona_clk *bcm_clk = to_kona_clk(hw);
@@ -1144,6 +1174,31 @@ struct clk_ops kona_peri_clk_ops = {
 	.set_rate = kona_peri_clk_set_rate,
 };
 
+/* Put a bus clock into its initial state */
+static bool __bus_clk_init(struct kona_clk *bcm_clk)
+{
+	struct ccu_data *ccu = bcm_clk->ccu;
+	struct bus_clk_data *bus = bcm_clk->u.bus;
+	const char *name = bcm_clk->init_data.name;
+
+	BUG_ON(bcm_clk->type != bcm_clk_bus);
+
+	if (!policy_init(ccu, &bus->policy)) {
+		pr_err("%s: error initializing policy for %s\n",
+			__func__, name);
+		return false;
+	}
+	if (!gate_init(ccu, &bus->gate)) {
+		pr_err("%s: error initializing gate for %s\n", __func__, name);
+		return false;
+	}
+	if (!hyst_init(ccu, &bus->hyst)) {
+		pr_err("%s: error initializing hyst for %s\n", __func__, name);
+		return false;
+	}
+	return true;
+}
+
 /* Put a peripheral clock into its initial state */
 static bool __peri_clk_init(struct kona_clk *bcm_clk)
 {
@@ -1248,6 +1303,9 @@ static bool __kona_clk_init(struct kona_clk *bcm_clk)
 		return false;
 
 	switch (bcm_clk->type) {
+	case bcm_clk_bus:
+		ret = __bus_clk_init(bcm_clk);
+		break;
 	case bcm_clk_peri:
 		ret = __peri_clk_init(bcm_clk);
 		break;
diff --git a/drivers/clk/bcm/clk-kona.h b/drivers/clk/bcm/clk-kona.h
index a5b61e0..345d15f 100644
--- a/drivers/clk/bcm/clk-kona.h
+++ b/drivers/clk/bcm/clk-kona.h
@@ -389,6 +389,12 @@ struct bcm_clk_trig {
 		.flags = FLAG(TRIG, EXISTS),				\
 	}
 
+struct bus_clk_data {
+	struct bcm_clk_policy policy;
+	struct bcm_clk_gate gate;
+	struct bcm_clk_hyst hyst;
+};
+
 struct peri_clk_data {
 	struct bcm_clk_policy policy;
 	struct bcm_clk_gate gate;
@@ -416,6 +422,7 @@ struct kona_clk {
 	} p;
 	union {
 		void *data;
+		struct bus_clk_data *bus;
 		struct peri_clk_data *peri;
 	} u;
 };
@@ -520,6 +527,7 @@ struct ccu_data {
 
 /* Exported globals */
 
+extern struct clk_ops kona_bus_clk_ops;
 extern struct clk_ops kona_peri_clk_ops;
 
 /* Externally visible functions */
-- 
1.9.1

[PATCH v2 4/5] clk: bcm281xx: define a bus clock

[Alex Elder]

Define the bus clock "bsc3_apb".  This bus clock has to be managed
using the CCU policy mechanism, so add the definitions required for
that to the clock and its CCU.

This one bus clock in particular is defined because it is needed
by peripheral clock "bsc3".  Our boot loader does not properly
activate "bsc3_apb", and as a result, "bsc3" isn't able to function
properly.  With "bsc3_apb" specified as a prerequisite clock for
"bsc3", the latter works correctly.

For now only this one bus clock is defined, because it allows
correct operation of "bsc3".  Others can be added later as needed
(and this patch serves to show how that's done).

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/clk/bcm/clk-bcm281xx.c       | 13 ++++++++++++-
 include/dt-bindings/clock/bcm281xx.h |  3 ++-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/bcm/clk-bcm281xx.c b/drivers/clk/bcm/clk-bcm281xx.c
index 502a487..b937fc9 100644
--- a/drivers/clk/bcm/clk-bcm281xx.c
+++ b/drivers/clk/bcm/clk-bcm281xx.c
@@ -309,8 +309,17 @@ static struct peri_clk_data pwm_data = {
 	.trig		= TRIGGER(0x0afc, 15),
 };
 
+static struct bus_clk_data bsc3_apb_data = {
+	.policy		= POLICY(0x0048, 4),
+	.gate		= HW_SW_GATE(0x0484, 16, 0, 1),
+};
+
 static struct ccu_data slave_ccu_data = {
 	BCM281XX_CCU_COMMON(slave, SLAVE),
+	.policy		= {
+		.enable		= CCU_LVM_EN(0x0034, 0),
+		.control	= CCU_POLICY_CTL(0x000c, 0, 1, 2),
+	},
 	.kona_clks	= {
 		[BCM281XX_SLAVE_CCU_UARTB] =
 			KONA_CLK(slave, uartb, peri),
@@ -329,9 +338,11 @@ static struct ccu_data slave_ccu_data = {
 		[BCM281XX_SLAVE_CCU_BSC2] =
 			KONA_CLK(slave, bsc2, peri),
 		[BCM281XX_SLAVE_CCU_BSC3] =
-			KONA_CLK(slave, bsc3, peri),
+			KONA_CLK_PREREQ(slave, bsc3, peri, bsc3_apb),
 		[BCM281XX_SLAVE_CCU_PWM] =
 			KONA_CLK(slave, pwm, peri),
+		[BCM281XX_SLAVE_CCU_BSC3_APB] =
+			KONA_CLK(slave, bsc3_apb, bus),
 		[BCM281XX_SLAVE_CCU_CLOCK_COUNT] = LAST_KONA_CLK,
 	},
 };
diff --git a/include/dt-bindings/clock/bcm281xx.h b/include/dt-bindings/clock/bcm281xx.h
index a763460..99f4aad 100644
--- a/include/dt-bindings/clock/bcm281xx.h
+++ b/include/dt-bindings/clock/bcm281xx.h
@@ -72,6 +72,7 @@
 #define BCM281XX_SLAVE_CCU_BSC2			7
 #define BCM281XX_SLAVE_CCU_BSC3			8
 #define BCM281XX_SLAVE_CCU_PWM			9
-#define BCM281XX_SLAVE_CCU_CLOCK_COUNT		10
+#define BCM281XX_SLAVE_CCU_BSC3_APB		10
+#define BCM281XX_SLAVE_CCU_CLOCK_COUNT		11
 
 #endif /* _CLOCK_BCM281XX_H */
-- 
1.9.1