Skip to contentSkip to conversation

clk: bcm: prerequisite and bus clock support

PATCH Superseded Alex Elder 3 participants 15 messages May 20, 2014 – May 30, 2014 1 message not in the archive

Subsystems (5)
Device tree bindings Common clk Broadcom BCM2711/BCM2835 Arm architecture Broadcom BCM281XX/BCM11XXX/BCM216XX Arm architecture +1 more subsystems

v2 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Alex Elder

author
#0 lore raw

v2 cover clk: bcm: prerequisite and bus clock support

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(-)
Signature · 1 line
1.9.1
author
#1 lore raw

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

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 <[email protected]>
---
 drivers/clk/bcm/clk-kona.c | 17 +++++++++++++++--
 drivers/clk/bcm/clk-kona.h |  7 +++++++
 2 files changed, 22 insertions(+), 2 deletions(-)
drivers/clk/bcm/clk-kona.c +15 -2
index d603c4e..d8a7f38 100644
@@ -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 */
drivers/clk/bcm/clk-kona.h +7 -0
index 2537b30..10e238d 100644
@@ -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)				\
 	{								\
Signature · 1 line
1.9.1
#2 lore raw

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

Quoting Alex Elder (2014-05-20 05:52:38)

Quoted 27 lines

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 <[email protected]>
---
 drivers/clk/bcm/clk-kona.c | 17 +++++++++++++++--
 drivers/clk/bcm/clk-kona.h |  7 +++++++
 2 files changed, 22 insertions(+), 2 deletions(-)
drivers/clk/bcm/clk-kona.c +3 -0
index d603c4e..d8a7f38 100644
@@ -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)

drivers/clk/bcm/clk-kona.c static bool __peri_clk_init(struct kona_clk *bcm_clk) · line 1207

Quoted 10 lines
drivers/clk/bcm/clk-kona.c +6 -1
 {
+       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

Quoted 41 lines
drivers/clk/bcm/clk-kona.c +6 -1
+               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 */
drivers/clk/bcm/clk-kona.h +7 -0
index 2537b30..10e238d 100644
@@ -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)                          \
        {                                                               \
Signature · 1 line
1.9.1
author
#3 lore raw

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

Quoted 41 lines · Mike Turquette

Quoting Alex Elder (2014-05-20 05:52:38)

Quoted 27 lines

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 <[email protected]>
---
 drivers/clk/bcm/clk-kona.c | 17 +++++++++++++++--
 drivers/clk/bcm/clk-kona.h |  7 +++++++
 2 files changed, 22 insertions(+), 2 deletions(-)
drivers/clk/bcm/clk-kona.c +3 -0
index d603c4e..d8a7f38 100644
@@ -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)

drivers/clk/bcm/clk-kona.c static bool __peri_clk_init(struct kona_clk *bcm_clk) · line 1207

Quoted 10 lines
drivers/clk/bcm/clk-kona.c +6 -1
 {
+       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,

Quoted 1 line

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.)

Quoted 2 lines

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

Quoted 44 lines

Thanks,
Mike

Quoted 41 lines
drivers/clk/bcm/clk-kona.c +6 -1
+               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 */
drivers/clk/bcm/clk-kona.h +7 -0
index 2537b30..10e238d 100644
@@ -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)                          \
        {                                                               \
Signature · 1 line
1.9.1
author
#4 lore raw

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

In reply to 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 <[email protected]>
---
 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(-)
drivers/clk/bcm/clk-kona-setup.c +12 -4
index e5aeded..fcce22c 100644
@@ -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);
drivers/clk/bcm/clk-kona.c +45 -0

57 lines not on this page · raw message

drivers/clk/bcm/clk-kona.h +17 -3
index 10e238d..a5b61e0 100644
@@ -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 }
Signature · 1 line
1.9.1
#5 lore raw

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

Quoting Alex Elder (2014-05-20 05:52:39)

Review struct clk *kona_clk_setup(struct kona_clk *bcm_clk) · line 754

Quoted 11 lines
(quoted diff) +2 -1
@@ -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/ ?

Review struct clk *kona_clk_setup(struct kona_clk *bcm_clk) · line 757

Quoted 3 lines
(quoted diff) +3 -0
+       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.

Quoted 15 lines
(quoted diff) +1 -0
+
        return clk;
 out_teardown:
        bcm_clk_teardown(bcm_clk);
drivers/clk/bcm/clk-kona.c +3 -0
index d8a7f38..fd070d6 100644
@@ -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)
+{

drivers/clk/bcm/clk-kona.c static bool __peri_clk_init(struct kona_clk *bcm_clk) · line 1210

Quoted 10 lines
drivers/clk/bcm/clk-kona.c +10 -0
+       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.

Quoted 15 lines
drivers/clk/bcm/clk-kona.c +15 -0
+       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);

drivers/clk/bcm/clk-kona.c static bool __peri_clk_init(struct kona_clk *bcm_clk) · line 1235

Quoted 10 lines
drivers/clk/bcm/clk-kona.c +10 -0
+               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:

drivers/clk/bcm/clk-kona.c +9 -0
index d603c4e..51f35b4 100644
@@ -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

author
#6 lore raw

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

Quoted 11 lines · Mike Turquette

Quoting Alex Elder (2014-05-20 05:52:39)

Quoted 10 lines
(quoted diff) +1 -1
@@ -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);
Quoted 3 lines

Review struct clk *kona_clk_setup(struct kona_clk *bcm_clk) · line 754

Quoted 1 line
(quoted diff) +1 -0
+       /*  Make it so we can look the clock up using clk_find() */

s/clk_find/clk_get/ ?

Yes, this is a mistake.

Quoted 6 lines

Review struct clk *kona_clk_setup(struct kona_clk *bcm_clk) · line 757

Quoted 4 lines
(quoted diff) +3 -0
+       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.

Quoted 22 lines
Quoted 22 lines
(quoted diff) +1 -0
+
        return clk;
 out_teardown:
        bcm_clk_teardown(bcm_clk);
drivers/clk/bcm/clk-kona.c +10 -0
index d8a7f38..fd070d6 100644
@@ -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)
Quoted 8 lines

drivers/clk/bcm/clk-kona.c static bool __peri_clk_init(struct kona_clk *bcm_clk) · line 1210

Quoted 3 lines
drivers/clk/bcm/clk-kona.c +3 -0
+               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.

Quoted 3 lines

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().

Quoted 26 lines
Quoted 15 lines
drivers/clk/bcm/clk-kona.c +15 -0
+       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);

drivers/clk/bcm/clk-kona.c static bool __peri_clk_init(struct kona_clk *bcm_clk) · line 1235

Quoted 10 lines
drivers/clk/bcm/clk-kona.c +10 -0
+               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;
Quoted 3 lines

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.

Quoted 30 lines
drivers/clk/bcm/clk-kona.c +9 -0
index d603c4e..51f35b4 100644
@@ -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)
Quoted 2 lines

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

#7 lore raw

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

Quoting Alex Elder (2014-05-29 06:26:15)

Quoted 51 lines · Mike Turquette
Quoted 3 lines

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.

Quoted 30 lines
drivers/clk/bcm/clk-kona.c +9 -0
index d603c4e..51f35b4 100644
@@ -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)
Quoted 2 lines

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.

Quoted 2 lines

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

Quoted 3 lines

-Alex

author
#8 lore raw

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

Quoted 13 lines · Mike Turquette

Quoting Alex Elder (2014-05-29 06:26:15)

Quoted 12 lines · Mike Turquette
Quoted 3 lines

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.

Quoted 38 lines
Quoted 38 lines
Quoted 30 lines
drivers/clk/bcm/clk-kona.c +9 -0
index d603c4e..51f35b4 100644
@@ -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)
Quoted 2 lines

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.

Quoted 8 lines
Quoted 2 lines

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

Quoted 10 lines

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

Quoted 3 lines

-Alex

#9 lore raw

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

Quoting Alex Elder (2014-05-29 09:53:50)

Quoted 62 lines · Mike Turquette
Quoted 13 lines

Quoting Alex Elder (2014-05-29 06:26:15)

Quoted 12 lines · Mike Turquette
Quoted 3 lines

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.

Quoted 43 lines
Quoted 38 lines
Quoted 30 lines
drivers/clk/bcm/clk-kona.c +9 -0
index d603c4e..51f35b4 100644
@@ -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)
Quoted 2 lines

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.

Quoted 2 lines

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

Hi Alex,

Quoted 8 lines
Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 5 lines

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().

Quoted 4 lines

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

Quoted 16 lines

Thanks a lot Mike.

-Alex

Quoted 10 lines

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

Quoted 3 lines

-Alex

author
#10 lore raw

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

In reply to Mike Turquette
Quoted 15 lines · Mike Turquette

Quoting Alex Elder (2014-05-20 05:52:39)

Review struct clk *kona_clk_setup(struct kona_clk *bcm_clk) · line 754

Quoted 11 lines
(quoted diff) +2 -1
@@ -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/ ?

Quoted 5 lines

Review struct clk *kona_clk_setup(struct kona_clk *bcm_clk) · line 757

Quoted 3 lines
(quoted diff) +3 -0
+       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.)

Quoted 23 lines
Quoted 23 lines
(quoted diff) +1 -0
+
        return clk;
 out_teardown:
        bcm_clk_teardown(bcm_clk);
drivers/clk/bcm/clk-kona.c +10 -0
index d8a7f38..fd070d6 100644
@@ -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)
Quoted 8 lines

drivers/clk/bcm/clk-kona.c static bool __peri_clk_init(struct kona_clk *bcm_clk) · line 1210

Quoted 3 lines
drivers/clk/bcm/clk-kona.c +3 -0
+               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.)

Quoted 3 lines

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

Quoted 65 lines
Quoted 15 lines
drivers/clk/bcm/clk-kona.c +15 -0
+       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);

drivers/clk/bcm/clk-kona.c static bool __peri_clk_init(struct kona_clk *bcm_clk) · line 1235

Quoted 10 lines
drivers/clk/bcm/clk-kona.c +10 -0
+               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:

drivers/clk/bcm/clk-kona.c +9 -0
index d603c4e..51f35b4 100644
@@ -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

author
#11 lore raw

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

Quoted 20 lines · Alex Elder
Quoted 19 lines · Mike Turquette

Quoting Alex Elder (2014-05-20 05:52:39)

Review struct clk *kona_clk_setup(struct kona_clk *bcm_clk) · line 754

Quoted 11 lines
(quoted diff) +2 -1
@@ -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/ ?

Quoted 3 lines
(quoted diff) +3 -0
+       bcm_clk->cl.con_id = name;
+       bcm_clk->cl.clk = clk;
+       clkdev_add(&bcm_clk->cl);
Quoted 5 lines
Quoted 1 line

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

author
#12 lore raw

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

In reply to 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 <[email protected]>
---
 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(-)
drivers/clk/bcm/clk-kona-setup.c +93 -3
index fcce22c..60d18f0 100644
@@ -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;
drivers/clk/bcm/clk-kona.c +58 -0

76 lines not on this page · raw message

drivers/clk/bcm/clk-kona.h +8 -0
index a5b61e0..345d15f 100644
@@ -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 */
Signature · 1 line
1.9.1
author
#13 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/clk/bcm/clk-bcm281xx.c       | 13 ++++++++++++-
 include/dt-bindings/clock/bcm281xx.h |  3 ++-
 2 files changed, 14 insertions(+), 2 deletions(-)
drivers/clk/bcm/clk-bcm281xx.c +12 -1
index 502a487..b937fc9 100644
@@ -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,
 	},
 };
include/dt-bindings/clock/bcm281xx.h +2 -1
index a763460..99f4aad 100644
@@ -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 */
Signature · 1 line
1.9.1

A message that is not in the archive (sent off-list or not yet received): 1400590362-11177-1-git-send-email-elder-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org

#15 lore raw

PATCH v2 5/5 ARM: dts: add bus clock bsc3_apb for bcm281xx

Add the bus clock named "bsc3_apb" to the list of those provided by the slave CCU.

Signed-off-by Alex Elder <[email protected]>

---
[Re-sent this one manually because it failed while sending. -Alex]

 arch/arm/boot/dts/bcm11351.dtsi | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
(quoted diff) +0 -0

b/arch/arm/boot/dts/bcm11351.dtsi
index 64d069b..faff8af 100644

arch/arm/boot/dts/bcm11351.dtsi +2 -1
@@ -247,7 +247,8 @@
 					     "bsc1",
 					     "bsc2",
 					     "bsc3",
-					     "pwm";
+					     "pwm",
+					     "bsc3_apb";
 		};
 
 		ref_1m_clk: ref_1m {
Signature · 7 lines
1.9.1


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html