[PATCH v4 0/7] clk: bcm: prerequisite and bus clock support

[PATCH v4 0/7] 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* operating on 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 prepared
and enabled at the beginning  of the dependent clock's ->prepare
method.  The prerequisite clock is similarly disabled and unprepared
at the end of the dependent clock's ->unprepare method.

These patches are based on Mike Turquette's current "clk-next"
branch.
    9ec2749 clk: qcom: Return error pointers for unimplemented clocks

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

					-Alex

Version history:
v4  - Remove boot-time initialization of clocks, opting instead to
      do this work when needed, via clock ->prepare method.
    - Prerequisite clocks are now prepared and enabled as
      in a dependent clock's ->prepare method.
    - Use __clk_lookup() to get the prerequisite clock pointer.
    - ccu_write_enable() calls can now be nested
v3: - Deleted clk_lookup field from struct kona_clk.
    - Now use term "initialized" rather than "enabled".
v2: - Added field "p" to the previously unnamed prereq union.

Alex Elder (7):
  clk: kona: allow nested ccu_write_enable() requests
  clk: kona: move some code
  clk: kona: don't init clocks at startup time
  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     | 123 ++++++++++++-
 drivers/clk/bcm/clk-kona.c           | 325 ++++++++++++++++++++++++-----------
 drivers/clk/bcm/clk-kona.h           |  31 +++-
 include/dt-bindings/clock/bcm281xx.h |   3 +-
 6 files changed, 387 insertions(+), 111 deletions(-)

-- 
1.9.1

[PATCH v4 1/7] clk: kona: allow nested ccu_write_enable() requests

[Alex Elder]

Use a counter rather than a Boolean to track whether write access to
a CCU has been enabled or not.  This will allow more than one of
these requests to be nested.

Note that __ccu_write_enable() and __ccu_write_disable() calls all
come in pairs, and they are always surrounded immediately by calls
to ccu_lock() and ccu_unlock().

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

diff --git a/drivers/clk/bcm/clk-kona.c b/drivers/clk/bcm/clk-kona.c
index 95af2e6..ee8e988 100644
--- a/drivers/clk/bcm/clk-kona.c
+++ b/drivers/clk/bcm/clk-kona.c
@@ -170,13 +170,8 @@ static inline void ccu_unlock(struct ccu_data *ccu, unsigned long flags)
  */
 static inline void __ccu_write_enable(struct ccu_data *ccu)
 {
-	if (ccu->write_enabled) {
-		pr_err("%s: access already enabled for %s\n", __func__,
-			ccu->name);
-		return;
-	}
-	ccu->write_enabled = true;
-	__ccu_write(ccu, 0, CCU_ACCESS_PASSWORD | 1);
+	if (!ccu->write_enabled++)
+		__ccu_write(ccu, 0, CCU_ACCESS_PASSWORD | 1);
 }
 
 static inline void __ccu_write_disable(struct ccu_data *ccu)
@@ -186,9 +181,8 @@ static inline void __ccu_write_disable(struct ccu_data *ccu)
 			ccu->name);
 		return;
 	}
-
-	__ccu_write(ccu, 0, CCU_ACCESS_PASSWORD);
-	ccu->write_enabled = false;
+	if (!--ccu->write_enabled)
+		__ccu_write(ccu, 0, CCU_ACCESS_PASSWORD);
 }
 
 /*
diff --git a/drivers/clk/bcm/clk-kona.h b/drivers/clk/bcm/clk-kona.h
index 2537b30..e9a8466 100644
--- a/drivers/clk/bcm/clk-kona.h
+++ b/drivers/clk/bcm/clk-kona.h
@@ -478,7 +478,7 @@ struct ccu_policy {
 struct ccu_data {
 	void __iomem *base;	/* base of mapped address space */
 	spinlock_t lock;	/* serialization lock */
-	bool write_enabled;	/* write access is currently enabled */
+	u32 write_enabled;	/* write access enable count */
 	struct ccu_policy policy;
 	struct list_head links;	/* for ccu_list */
 	struct device_node *node;
-- 
1.9.1

[PATCH v4 2/7] clk: kona: move some code

[Alex Elder]

Move the definition of __peri_clk_init() up in "clk-kona.c", in
preparation for the next patch (so it's defined before it's needed).
Move kona_ccu_init() and __kona_clk_init() as well.

Done as a separate patch so doing so doesn't obscure other changes.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/clk/bcm/clk-kona.c | 174 ++++++++++++++++++++++-----------------------
 1 file changed, 87 insertions(+), 87 deletions(-)

diff --git a/drivers/clk/bcm/clk-kona.c b/drivers/clk/bcm/clk-kona.c
index ee8e988..0c64504 100644
--- a/drivers/clk/bcm/clk-kona.c
+++ b/drivers/clk/bcm/clk-kona.c
@@ -975,6 +975,93 @@ static int selector_write(struct ccu_data *ccu, struct bcm_clk_gate *gate,
 	return ret;
 }
 
+/* Put a peripheral clock into its initial state */
+static bool __peri_clk_init(struct kona_clk *bcm_clk)
+{
+	struct ccu_data *ccu = bcm_clk->ccu;
+	struct peri_clk_data *peri = bcm_clk->u.peri;
+	const char *name = bcm_clk->init_data.name;
+	struct bcm_clk_trig *trig;
+
+	BUG_ON(bcm_clk->type != bcm_clk_peri);
+
+	if (!policy_init(ccu, &peri->policy)) {
+		pr_err("%s: error initializing policy for %s\n",
+			__func__, name);
+		return false;
+	}
+	if (!gate_init(ccu, &peri->gate)) {
+		pr_err("%s: error initializing gate for %s\n", __func__, name);
+		return false;
+	}
+	if (!hyst_init(ccu, &peri->hyst)) {
+		pr_err("%s: error initializing hyst for %s\n", __func__, name);
+		return false;
+	}
+	if (!div_init(ccu, &peri->gate, &peri->div, &peri->trig)) {
+		pr_err("%s: error initializing divider for %s\n", __func__,
+			name);
+		return false;
+	}
+
+	/*
+	 * For the pre-divider and selector, the pre-trigger is used
+	 * if it's present, otherwise we just use the regular trigger.
+	 */
+	trig = trigger_exists(&peri->pre_trig) ? &peri->pre_trig
+					       : &peri->trig;
+
+	if (!div_init(ccu, &peri->gate, &peri->pre_div, trig)) {
+		pr_err("%s: error initializing pre-divider for %s\n", __func__,
+			name);
+		return false;
+	}
+
+	if (!sel_init(ccu, &peri->gate, &peri->sel, trig)) {
+		pr_err("%s: error initializing selector for %s\n", __func__,
+			name);
+		return false;
+	}
+
+	return true;
+}
+
+static bool __kona_clk_init(struct kona_clk *bcm_clk)
+{
+	switch (bcm_clk->type) {
+	case bcm_clk_peri:
+		return __peri_clk_init(bcm_clk);
+	default:
+		BUG();
+	}
+	return -EINVAL;
+}
+
+/* Set a CCU and all its clocks into their desired initial state */
+bool __init kona_ccu_init(struct ccu_data *ccu)
+{
+	unsigned long flags;
+	unsigned int which;
+	struct clk **clks = ccu->clk_data.clks;
+	bool success = true;
+
+	flags = ccu_lock(ccu);
+	__ccu_write_enable(ccu);
+
+	for (which = 0; which < ccu->clk_data.clk_num; which++) {
+		struct kona_clk *bcm_clk;
+
+		if (!clks[which])
+			continue;
+		bcm_clk = to_kona_clk(__clk_get_hw(clks[which]));
+		success &= __kona_clk_init(bcm_clk);
+	}
+
+	__ccu_write_disable(ccu);
+	ccu_unlock(ccu, flags);
+	return success;
+}
+
 /* Clock operations */
 
 static int kona_peri_clk_enable(struct clk_hw *hw)
@@ -1186,90 +1273,3 @@ struct clk_ops kona_peri_clk_ops = {
 	.get_parent = kona_peri_clk_get_parent,
 	.set_rate = kona_peri_clk_set_rate,
 };
-
-/* Put a peripheral clock into its initial state */
-static bool __peri_clk_init(struct kona_clk *bcm_clk)
-{
-	struct ccu_data *ccu = bcm_clk->ccu;
-	struct peri_clk_data *peri = bcm_clk->u.peri;
-	const char *name = bcm_clk->init_data.name;
-	struct bcm_clk_trig *trig;
-
-	BUG_ON(bcm_clk->type != bcm_clk_peri);
-
-	if (!policy_init(ccu, &peri->policy)) {
-		pr_err("%s: error initializing policy for %s\n",
-			__func__, name);
-		return false;
-	}
-	if (!gate_init(ccu, &peri->gate)) {
-		pr_err("%s: error initializing gate for %s\n", __func__, name);
-		return false;
-	}
-	if (!hyst_init(ccu, &peri->hyst)) {
-		pr_err("%s: error initializing hyst for %s\n", __func__, name);
-		return false;
-	}
-	if (!div_init(ccu, &peri->gate, &peri->div, &peri->trig)) {
-		pr_err("%s: error initializing divider for %s\n", __func__,
-			name);
-		return false;
-	}
-
-	/*
-	 * For the pre-divider and selector, the pre-trigger is used
-	 * if it's present, otherwise we just use the regular trigger.
-	 */
-	trig = trigger_exists(&peri->pre_trig) ? &peri->pre_trig
-					       : &peri->trig;
-
-	if (!div_init(ccu, &peri->gate, &peri->pre_div, trig)) {
-		pr_err("%s: error initializing pre-divider for %s\n", __func__,
-			name);
-		return false;
-	}
-
-	if (!sel_init(ccu, &peri->gate, &peri->sel, trig)) {
-		pr_err("%s: error initializing selector for %s\n", __func__,
-			name);
-		return false;
-	}
-
-	return true;
-}
-
-static bool __kona_clk_init(struct kona_clk *bcm_clk)
-{
-	switch (bcm_clk->type) {
-	case bcm_clk_peri:
-		return __peri_clk_init(bcm_clk);
-	default:
-		BUG();
-	}
-	return -EINVAL;
-}
-
-/* Set a CCU and all its clocks into their desired initial state */
-bool __init kona_ccu_init(struct ccu_data *ccu)
-{
-	unsigned long flags;
-	unsigned int which;
-	struct clk **clks = ccu->clk_data.clks;
-	bool success = true;
-
-	flags = ccu_lock(ccu);
-	__ccu_write_enable(ccu);
-
-	for (which = 0; which < ccu->clk_data.clk_num; which++) {
-		struct kona_clk *bcm_clk;
-
-		if (!clks[which])
-			continue;
-		bcm_clk = to_kona_clk(__clk_get_hw(clks[which]));
-		success &= __kona_clk_init(bcm_clk);
-	}
-
-	__ccu_write_disable(ccu);
-	ccu_unlock(ccu, flags);
-	return success;
-}
-- 
1.9.1

[PATCH v4 3/7] clk: kona: don't init clocks at startup time

[Alex Elder]

The last thing done for CCU setup is a call to kona_ccu_init().
This locks and enables write access on the CCU, then calls
__kona_clk_init() for all of the clocks it provides.  The purpose of
this was to get or set the initial state of all the registers
related to each clock.

There's no reason to do this though.  The common clock framework
assumes nothing about the state of the hardware, and if a driver
requires its clock to be in a particular state it can set that
state up itself.

Instead of initializing clocks at startup time, define a prepare
method that performs that step when a clock is first needed.

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

diff --git a/drivers/clk/bcm/clk-kona-setup.c b/drivers/clk/bcm/clk-kona-setup.c
index e5aeded..317f7dd 100644
--- a/drivers/clk/bcm/clk-kona-setup.c
+++ b/drivers/clk/bcm/clk-kona-setup.c
@@ -867,9 +867,6 @@ void __init kona_dt_ccu_setup(struct ccu_data *ccu,
 		goto out_err;
 	}
 
-	if (!kona_ccu_init(ccu))
-		pr_err("Broadcom %s initialization had errors\n", node->name);
-
 	return;
 out_err:
 	kona_ccu_teardown(ccu);
diff --git a/drivers/clk/bcm/clk-kona.c b/drivers/clk/bcm/clk-kona.c
index 0c64504..9a69a01 100644
--- a/drivers/clk/bcm/clk-kona.c
+++ b/drivers/clk/bcm/clk-kona.c
@@ -1026,43 +1026,37 @@ static bool __peri_clk_init(struct kona_clk *bcm_clk)
 	return true;
 }
 
-static bool __kona_clk_init(struct kona_clk *bcm_clk)
+/* Clock operations */
+
+static int kona_clk_prepare(struct clk_hw *hw)
 {
+	struct kona_clk *bcm_clk = to_kona_clk(hw);
+	struct ccu_data *ccu = bcm_clk->ccu;
+	unsigned long flags;
+	int ret = 0;
+
+	flags = ccu_lock(ccu);
+	__ccu_write_enable(ccu);
+
 	switch (bcm_clk->type) {
 	case bcm_clk_peri:
-		return __peri_clk_init(bcm_clk);
+		if (!__peri_clk_init(bcm_clk))
+			ret = -EINVAL;
+		break;
 	default:
 		BUG();
 	}
-	return -EINVAL;
-}
-
-/* Set a CCU and all its clocks into their desired initial state */
-bool __init kona_ccu_init(struct ccu_data *ccu)
-{
-	unsigned long flags;
-	unsigned int which;
-	struct clk **clks = ccu->clk_data.clks;
-	bool success = true;
-
-	flags = ccu_lock(ccu);
-	__ccu_write_enable(ccu);
-
-	for (which = 0; which < ccu->clk_data.clk_num; which++) {
-		struct kona_clk *bcm_clk;
-
-		if (!clks[which])
-			continue;
-		bcm_clk = to_kona_clk(__clk_get_hw(clks[which]));
-		success &= __kona_clk_init(bcm_clk);
-	}
 
 	__ccu_write_disable(ccu);
 	ccu_unlock(ccu, flags);
-	return success;
+
+	return ret;
 }
 
-/* Clock operations */
+static void kona_clk_unprepare(struct clk_hw *hw)
+{
+	/* Nothing to do. */
+}
 
 static int kona_peri_clk_enable(struct clk_hw *hw)
 {
@@ -1264,6 +1258,8 @@ static int kona_peri_clk_set_rate(struct clk_hw *hw, unsigned long rate,
 }
 
 struct clk_ops kona_peri_clk_ops = {
+	.prepare = kona_clk_prepare,
+	.unprepare = kona_clk_unprepare,
 	.enable = kona_peri_clk_enable,
 	.disable = kona_peri_clk_disable,
 	.is_enabled = kona_peri_clk_is_enabled,
diff --git a/drivers/clk/bcm/clk-kona.h b/drivers/clk/bcm/clk-kona.h
index e9a8466..3409111 100644
--- a/drivers/clk/bcm/clk-kona.h
+++ b/drivers/clk/bcm/clk-kona.h
@@ -511,6 +511,5 @@ extern u64 scaled_div_build(struct bcm_clk_div *div, u32 div_value,
 extern struct clk *kona_clk_setup(struct kona_clk *bcm_clk);
 extern void __init kona_dt_ccu_setup(struct ccu_data *ccu,
 				struct device_node *node);
-extern bool __init kona_ccu_init(struct ccu_data *ccu);
 
 #endif /* _CLK_KONA_H */
-- 
1.9.1

[PATCH v4 4/7] clk: bcm281xx: implement prerequisite clocks

[Alex Elder]

Allow a clock to specify a "prerequisite" clock, identified by its
name.  The prerequisite clock must be prepared and enabled before a
clock that depends on it is used.  In order to simplify locking, we
require a clock and its prerequisite to be associated with the same
CCU.  (We'll just trust--but not verify--that nobody defines a cycle
of prerequisite clocks.)

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

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

diff --git a/drivers/clk/bcm/clk-kona-setup.c b/drivers/clk/bcm/clk-kona-setup.c
index 317f7dd..36a99b9 100644
--- a/drivers/clk/bcm/clk-kona-setup.c
+++ b/drivers/clk/bcm/clk-kona-setup.c
@@ -796,6 +796,28 @@ static bool ccu_data_valid(struct ccu_data *ccu)
 }
 
 /*
+ * If a clock specifies a prerequisite clock, they must both be on
+ * the same CCU.  Check if the named prerequisite clock matches one
+ * of the ones provided by CCU.  A null name means no prerequisite.
+ */
+static bool kona_prereq_valid(struct ccu_data *ccu, const char *prereq_name)
+{
+	unsigned int i;
+
+	if (!prereq_name)
+		return true;
+
+	for (i = 0; i < ccu->clk_data.clk_num; i++)
+		if (!strcmp(prereq_name, ccu->kona_clks[i].init_data.name))
+			return true;
+
+	pr_err("%s: prereq clock %s not defined for ccu %s\n", __func__,
+		prereq_name, ccu->name);
+
+	return false;
+}
+
+/*
  * Set up a CCU.  Call the provided ccu_clks_setup callback to
  * initialize the array of clocks provided by the CCU.
  */
@@ -857,6 +879,8 @@ void __init kona_dt_ccu_setup(struct ccu_data *ccu,
 	for (i = 0; i < ccu->clk_data.clk_num; i++) {
 		if (!ccu->kona_clks[i].ccu)
 			continue;
+		if (!kona_prereq_valid(ccu, ccu->kona_clks[i].prereq.name))
+			continue;
 		ccu->clk_data.clks[i] = kona_clk_setup(&ccu->kona_clks[i]);
 	}
 
diff --git a/drivers/clk/bcm/clk-kona.c b/drivers/clk/bcm/clk-kona.c
index 9a69a01..a6a45cd 100644
--- a/drivers/clk/bcm/clk-kona.c
+++ b/drivers/clk/bcm/clk-kona.c
@@ -1028,6 +1028,44 @@ static bool __peri_clk_init(struct kona_clk *bcm_clk)
 
 /* Clock operations */
 
+static int __kona_prereq_prepare_enable(struct kona_clk *bcm_clk)
+{
+	const char *clk_name = bcm_clk->init_data.name;
+	const char *prereq_name = bcm_clk->prereq.name;
+	struct clk *prereq_clk = bcm_clk->prereq.clk;
+	int ret;
+
+	BUG_ON(!clk_name);
+
+	/* Look up the prerequisite clock if we haven't already */
+	if (!prereq_clk) {
+		prereq_clk = __clk_lookup(prereq_name);
+		if (!prereq_clk) {
+			pr_err("%s: prereq clock %s for %s not found\n",
+				__func__, prereq_name, clk_name);
+			return -ENOENT;
+		}
+		bcm_clk->prereq.clk = prereq_clk;
+	}
+
+	ret = __clk_prepare(prereq_clk);
+	if (ret) {
+		pr_err("%s: unable to prepare prereq clock %s for %s\n",
+			__func__, prereq_name, clk_name);
+		return ret;
+	}
+
+	ret = clk_enable(prereq_clk);
+	if (ret) {
+		__clk_unprepare(prereq_clk);
+		pr_err("%s: unable to enable prereq clock %s for %s\n",
+			__func__, prereq_name, clk_name);
+		return ret;
+	}
+
+	return 0;
+}
+
 static int kona_clk_prepare(struct clk_hw *hw)
 {
 	struct kona_clk *bcm_clk = to_kona_clk(hw);
@@ -1038,6 +1076,13 @@ static int kona_clk_prepare(struct clk_hw *hw)
 	flags = ccu_lock(ccu);
 	__ccu_write_enable(ccu);
 
+	/* Prepare the prerequisite clock first */
+	if (bcm_clk->prereq.name) {
+		ret = __kona_prereq_prepare_enable(bcm_clk);
+		if (ret)
+			goto out;
+	}
+
 	switch (bcm_clk->type) {
 	case bcm_clk_peri:
 		if (!__peri_clk_init(bcm_clk))
@@ -1046,16 +1091,51 @@ static int kona_clk_prepare(struct clk_hw *hw)
 	default:
 		BUG();
 	}
-
+out:
 	__ccu_write_disable(ccu);
 	ccu_unlock(ccu, flags);
 
 	return ret;
 }
 
+/*
+ * Disable and unprepare a prerequisite clock, and drop our
+ * reference to it.
+ */
+static void __kona_prereq_disable_unprepare(struct kona_clk *bcm_clk)
+{
+	struct clk *prereq_clk = bcm_clk->prereq.clk;
+
+	BUG_ON(!bcm_clk->prereq.name);
+	WARN_ON_ONCE(!prereq_clk);
+
+	bcm_clk->prereq.clk = NULL;
+
+	clk_disable(prereq_clk);
+	__clk_unprepare(prereq_clk);
+}
+
 static void kona_clk_unprepare(struct clk_hw *hw)
 {
-	/* Nothing to do. */
+	struct kona_clk *bcm_clk = to_kona_clk(hw);
+	struct ccu_data *ccu;
+	unsigned long flags;
+
+	/*
+	 * We don't do anything to unprepare Kona clocks themselves,
+	 * but if there's a prerequisite we'll need to unprepare it.
+	 */
+	if (!bcm_clk->prereq.name)
+		return;
+
+	ccu = bcm_clk->ccu;
+	flags = ccu_lock(ccu);
+	__ccu_write_enable(ccu);
+
+	__kona_prereq_disable_unprepare(bcm_clk);
+
+	__ccu_write_disable(ccu);
+	ccu_unlock(ccu, flags);
 }
 
 static int kona_peri_clk_enable(struct clk_hw *hw)
diff --git a/drivers/clk/bcm/clk-kona.h b/drivers/clk/bcm/clk-kona.h
index 3409111..0a845a0 100644
--- a/drivers/clk/bcm/clk-kona.h
+++ b/drivers/clk/bcm/clk-kona.h
@@ -406,6 +406,10 @@ struct kona_clk {
 	struct clk_init_data init_data;	/* includes name of this clock */
 	struct ccu_data *ccu;	/* ccu this clock is associated with */
 	enum bcm_clk_type type;
+	struct {
+		const char *name;
+		struct clk *clk;
+	} prereq;
 	union {
 		void *data;
 		struct peri_clk_data *peri;
@@ -415,16 +419,26 @@ struct kona_clk {
 	container_of(_hw, struct kona_clk, hw)
 
 /* Initialization macro for an entry in a CCU's kona_clks[] array. */
-#define KONA_CLK(_ccu_name, _clk_name, _type)				\
-	{								\
+#define ___KONA_CLK_COMMON(_ccu_name, _clk_name, _type)			\
 		.init_data	= {					\
 			.name = #_clk_name,				\
 			.ops = &kona_ ## _type ## _clk_ops,		\
 		},							\
 		.ccu		= &_ccu_name ## _ccu_data,		\
 		.type		= bcm_clk_ ## _type,			\
-		.u.data		= &_clk_name ## _data,			\
+		.u.data		= &_clk_name ## _data
+
+#define KONA_CLK_PREREQ(_ccu_name, _clk_name, _type, _prereq)		\
+	{								\
+		.prereq.name	= #_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 v4 5/7] 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       | 61 +++++++++++++++++++++++++
 drivers/clk/bcm/clk-kona.h       |  8 ++++
 3 files changed, 162 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/bcm/clk-kona-setup.c b/drivers/clk/bcm/clk-kona-setup.c
index 36a99b9..285d093 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)
 {
@@ -687,6 +770,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)
 {
 	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;
@@ -722,6 +808,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 a6a45cd..735c5f5 100644
--- a/drivers/clk/bcm/clk-kona.c
+++ b/drivers/clk/bcm/clk-kona.c
@@ -975,6 +975,31 @@ static int selector_write(struct ccu_data *ccu, struct bcm_clk_gate *gate,
 	return ret;
 }
 
+/* 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)
 {
@@ -1084,6 +1109,10 @@ static int kona_clk_prepare(struct clk_hw *hw)
 	}
 
 	switch (bcm_clk->type) {
+	case bcm_clk_bus:
+		if (!__bus_clk_init(bcm_clk))
+			ret = -EINVAL;
+		break;
 	case bcm_clk_peri:
 		if (!__peri_clk_init(bcm_clk))
 			ret = -EINVAL;
@@ -1138,6 +1167,38 @@ static void kona_clk_unprepare(struct clk_hw *hw)
 	ccu_unlock(ccu, flags);
 }
 
+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 = {
+	.prepare = kona_clk_prepare,
+	.unprepare = kona_clk_unprepare,
+	.enable = kona_bus_clk_enable,
+	.disable = kona_bus_clk_disable,
+	.is_enabled = kona_bus_clk_is_enabled,
+};
+
 static int kona_peri_clk_enable(struct clk_hw *hw)
 {
 	struct kona_clk *bcm_clk = to_kona_clk(hw);
diff --git a/drivers/clk/bcm/clk-kona.h b/drivers/clk/bcm/clk-kona.h
index 0a845a0..7ded642 100644
--- a/drivers/clk/bcm/clk-kona.h
+++ b/drivers/clk/bcm/clk-kona.h
@@ -387,6 +387,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;
@@ -412,6 +418,7 @@ struct kona_clk {
 	} prereq;
 	union {
 		void *data;
+		struct bus_clk_data *bus;
 		struct peri_clk_data *peri;
 	} u;
 };
@@ -513,6 +520,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 v4 6/7] 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

[PATCH v4 7/7] ARM: dts: add bus clock bsc3_apb for bcm281xx

[Alex Elder]

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

Signed-off-by: Alex Elder <elder@linaro.org>
---
 arch/arm/boot/dts/bcm11351.dtsi | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/bcm11351.dtsi b/arch/arm/boot/dts/bcm11351.dtsi
index 64d069b..faff8af 100644
--- a/arch/arm/boot/dts/bcm11351.dtsi
+++ b/arch/arm/boot/dts/bcm11351.dtsi
@@ -247,7 +247,8 @@
 					     "bsc1",
 					     "bsc2",
 					     "bsc3",
-					     "pwm";
+					     "pwm",
+					     "bsc3_apb";
 		};
 
 		ref_1m_clk: ref_1m {
-- 
1.9.1

Re: [PATCH v4 1/7] clk: kona: allow nested ccu_write_enable() requests

[Mike Turquette]

Quoting Alex Elder (2014-05-30 13:53:02)
> Use a counter rather than a Boolean to track whether write access to
> a CCU has been enabled or not.  This will allow more than one of
> these requests to be nested.
> 
> Note that __ccu_write_enable() and __ccu_write_disable() calls all
> come in pairs, and they are always surrounded immediately by calls
> to ccu_lock() and ccu_unlock().
> 
> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
>  drivers/clk/bcm/clk-kona.c | 14 ++++----------
>  drivers/clk/bcm/clk-kona.h |  2 +-
>  2 files changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/clk/bcm/clk-kona.c b/drivers/clk/bcm/clk-kona.c
> index 95af2e6..ee8e988 100644
> --- a/drivers/clk/bcm/clk-kona.c
> +++ b/drivers/clk/bcm/clk-kona.c
> @@ -170,13 +170,8 @@ static inline void ccu_unlock(struct ccu_data *ccu, unsigned long flags)
>   */
>  static inline void __ccu_write_enable(struct ccu_data *ccu)

Per Documentation/CodingStyle, chapter 15, "the inline disease", it
might be best to not inline these functions.

>  {
> -       if (ccu->write_enabled) {
> -               pr_err("%s: access already enabled for %s\n", __func__,
> -                       ccu->name);
> -               return;
> -       }
> -       ccu->write_enabled = true;
> -       __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD | 1);
> +       if (!ccu->write_enabled++)
> +               __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD | 1);
>  }
>  
>  static inline void __ccu_write_disable(struct ccu_data *ccu)
> @@ -186,9 +181,8 @@ static inline void __ccu_write_disable(struct ccu_data *ccu)
>                         ccu->name);
>                 return;
>         }
> -
> -       __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD);
> -       ccu->write_enabled = false;
> +       if (!--ccu->write_enabled)
> +               __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD);

What happens if calls to __ccu_write_enable and __ccu_write_disable are
unbalanced? It would be better to catch that case and throw a WARN:

	if (WARN_ON(ccu->write_enabled == 0))
		return;

	if (--ccu->write_enabled > 0)
		return;

	__ccu_write(ccu, 0, CCU_ACCESS_PASSWORD);

>  }
>  
>  /*
> diff --git a/drivers/clk/bcm/clk-kona.h b/drivers/clk/bcm/clk-kona.h
> index 2537b30..e9a8466 100644
> --- a/drivers/clk/bcm/clk-kona.h
> +++ b/drivers/clk/bcm/clk-kona.h
> @@ -478,7 +478,7 @@ struct ccu_policy {
>  struct ccu_data {
>         void __iomem *base;     /* base of mapped address space */
>         spinlock_t lock;        /* serialization lock */
> -       bool write_enabled;     /* write access is currently enabled */
> +       u32 write_enabled;      /* write access enable count */

Why u32? An unsigned int will do just nicely here.

Regards,
Mike

>         struct ccu_policy policy;
>         struct list_head links; /* for ccu_list */
>         struct device_node *node;
> -- 
> 1.9.1
> 

Re: [PATCH v4 3/7] clk: kona: don't init clocks at startup time

[Mike Turquette]

Quoting Alex Elder (2014-05-30 13:53:04)
> +static int kona_clk_prepare(struct clk_hw *hw)
>  {
> +       struct kona_clk *bcm_clk = to_kona_clk(hw);
> +       struct ccu_data *ccu = bcm_clk->ccu;
> +       unsigned long flags;
> +       int ret = 0;
> +
> +       flags = ccu_lock(ccu);
> +       __ccu_write_enable(ccu);
> +
>         switch (bcm_clk->type) {
>         case bcm_clk_peri:
> -               return __peri_clk_init(bcm_clk);
> +               if (!__peri_clk_init(bcm_clk))
> +                       ret = -EINVAL;
> +               break;
>         default:
>                 BUG();
>         }

The switch-case only has one match, plus a default. Will there be others
in the future?  Otherwise it can be replaced with an if-statement.

> -       return -EINVAL;
> -}
> -
> -/* Set a CCU and all its clocks into their desired initial state */
> -bool __init kona_ccu_init(struct ccu_data *ccu)
> -{
> -       unsigned long flags;
> -       unsigned int which;
> -       struct clk **clks = ccu->clk_data.clks;
> -       bool success = true;
> -
> -       flags = ccu_lock(ccu);
> -       __ccu_write_enable(ccu);
> -
> -       for (which = 0; which < ccu->clk_data.clk_num; which++) {
> -               struct kona_clk *bcm_clk;
> -
> -               if (!clks[which])
> -                       continue;
> -               bcm_clk = to_kona_clk(__clk_get_hw(clks[which]));
> -               success &= __kona_clk_init(bcm_clk);
> -       }
>  
>         __ccu_write_disable(ccu);
>         ccu_unlock(ccu, flags);
> -       return success;
> +
> +       return ret;
>  }

Does this prepare callback "enable" a clock? E.g does a line NOT toggle
at a rate prior to this call, and then after this call completes that
same line is now toggling at a rate?

>  
> -/* Clock operations */
> +static void kona_clk_unprepare(struct clk_hw *hw)
> +{
> +       /* Nothing to do. */
> +}

Is doing nothing the right thing to do? Could power be saved somehow if
the .unprepare callback really gets called? Remember that if .unprepare
actually runs (because struct clk->prepare_count == 0) then the next
call to clk_prepare will actually call your .prepare callback and set up
the prereq clocks again. So the prereq clock initialization is no longer
a one-time thing, which might afford you some optimizations.

Regards,
Mike

>  
>  static int kona_peri_clk_enable(struct clk_hw *hw)
>  {
> @@ -1264,6 +1258,8 @@ static int kona_peri_clk_set_rate(struct clk_hw *hw, unsigned long rate,
>  }
>  
>  struct clk_ops kona_peri_clk_ops = {
> +       .prepare = kona_clk_prepare,
> +       .unprepare = kona_clk_unprepare,
>         .enable = kona_peri_clk_enable,
>         .disable = kona_peri_clk_disable,
>         .is_enabled = kona_peri_clk_is_enabled,
> diff --git a/drivers/clk/bcm/clk-kona.h b/drivers/clk/bcm/clk-kona.h
> index e9a8466..3409111 100644
> --- a/drivers/clk/bcm/clk-kona.h
> +++ b/drivers/clk/bcm/clk-kona.h
> @@ -511,6 +511,5 @@ extern u64 scaled_div_build(struct bcm_clk_div *div, u32 div_value,
>  extern struct clk *kona_clk_setup(struct kona_clk *bcm_clk);
>  extern void __init kona_dt_ccu_setup(struct ccu_data *ccu,
>                                 struct device_node *node);
> -extern bool __init kona_ccu_init(struct ccu_data *ccu);
>  
>  #endif /* _CLK_KONA_H */
> -- 
> 1.9.1
> 

Re: [PATCH v4 1/7] clk: kona: allow nested ccu_write_enable() requests

[Alex Elder]

On 05/30/2014 06:28 PM, Mike Turquette wrote:
> Quoting Alex Elder (2014-05-30 13:53:02)
>> Use a counter rather than a Boolean to track whether write access to
>> a CCU has been enabled or not.  This will allow more than one of
>> these requests to be nested.
>>
>> Note that __ccu_write_enable() and __ccu_write_disable() calls all
>> come in pairs, and they are always surrounded immediately by calls
>> to ccu_lock() and ccu_unlock().
>>
>> Signed-off-by: Alex Elder <elder@linaro.org>
>> ---
>>  drivers/clk/bcm/clk-kona.c | 14 ++++----------
>>  drivers/clk/bcm/clk-kona.h |  2 +-
>>  2 files changed, 5 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/clk/bcm/clk-kona.c b/drivers/clk/bcm/clk-kona.c
>> index 95af2e6..ee8e988 100644
>> --- a/drivers/clk/bcm/clk-kona.c
>> +++ b/drivers/clk/bcm/clk-kona.c
>> @@ -170,13 +170,8 @@ static inline void ccu_unlock(struct ccu_data *ccu, unsigned long flags)
>>   */
>>  static inline void __ccu_write_enable(struct ccu_data *ccu)
> 
> Per Documentation/CodingStyle, chapter 15, "the inline disease", it
> might be best to not inline these functions.

This was not intentional.  I normally only inline things
defined in header files, and maybe this is an artifact of
having been in a header at one time.  I don't know, I'll get
rid of the inline.

> 
>>  {
>> -       if (ccu->write_enabled) {
>> -               pr_err("%s: access already enabled for %s\n", __func__,
>> -                       ccu->name);
>> -               return;
>> -       }
>> -       ccu->write_enabled = true;
>> -       __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD | 1);
>> +       if (!ccu->write_enabled++)
>> +               __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD | 1);
>>  }
>>  
>>  static inline void __ccu_write_disable(struct ccu_data *ccu)
>> @@ -186,9 +181,8 @@ static inline void __ccu_write_disable(struct ccu_data *ccu)
>>                         ccu->name);
>>                 return;
>>         }
>> -
>> -       __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD);
>> -       ccu->write_enabled = false;
>> +       if (!--ccu->write_enabled)
>> +               __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD);
> 
> What happens if calls to __ccu_write_enable and __ccu_write_disable are
> unbalanced? It would be better to catch that case and throw a WARN:

You can't see it in the diff, but that's what happens
(well, it's a pr_err(), not a WARN()).   I think a WARN()
is probably right in this case though.

> 	if (WARN_ON(ccu->write_enabled == 0))
> 		return;
> 
> 	if (--ccu->write_enabled > 0)
> 		return;
> 
> 	__ccu_write(ccu, 0, CCU_ACCESS_PASSWORD);
> 
>>  }
>>  
>>  /*
>> diff --git a/drivers/clk/bcm/clk-kona.h b/drivers/clk/bcm/clk-kona.h
>> index 2537b30..e9a8466 100644
>> --- a/drivers/clk/bcm/clk-kona.h
>> +++ b/drivers/clk/bcm/clk-kona.h
>> @@ -478,7 +478,7 @@ struct ccu_policy {
>>  struct ccu_data {
>>         void __iomem *base;     /* base of mapped address space */
>>         spinlock_t lock;        /* serialization lock */
>> -       bool write_enabled;     /* write access is currently enabled */
>> +       u32 write_enabled;      /* write access enable count */
> 
> Why u32? An unsigned int will do just nicely here.

That's a preference of mine.  I almost always favor
using u32, etc. because they are compact, and explicit
about the size and signedness.  I "know" an int is 32
bits, but I still prefer being explicit.

I'll interpret this as a preference on your part for
unsigned int, and I have no problem making that change.

					-Alex

> Regards,
> Mike
> 
>>         struct ccu_policy policy;
>>         struct list_head links; /* for ccu_list */
>>         struct device_node *node;
>> -- 
>> 1.9.1
>>

Re: [PATCH v4 3/7] clk: kona: don't init clocks at startup time

[Alex Elder]

On 05/30/2014 06:37 PM, Mike Turquette wrote:
> Quoting Alex Elder (2014-05-30 13:53:04)
>> +static int kona_clk_prepare(struct clk_hw *hw)
>>  {
>> +       struct kona_clk *bcm_clk = to_kona_clk(hw);
>> +       struct ccu_data *ccu = bcm_clk->ccu;
>> +       unsigned long flags;
>> +       int ret = 0;
>> +
>> +       flags = ccu_lock(ccu);
>> +       __ccu_write_enable(ccu);
>> +
>>         switch (bcm_clk->type) {
>>         case bcm_clk_peri:
>> -               return __peri_clk_init(bcm_clk);
>> +               if (!__peri_clk_init(bcm_clk))
>> +                       ret = -EINVAL;
>> +               break;
>>         default:
>>                 BUG();
>>         }
> 
> The switch-case only has one match, plus a default. Will there be others
> in the future?  Otherwise it can be replaced with an if-statement.

Yes, bus clocks in patch 5/7.  Maybe another type
someday but at least these two.  All the code is
structured this way in anticipation of other clock
types.


>> -       return -EINVAL;
>> -}
>> -
>> -/* Set a CCU and all its clocks into their desired initial state */
>> -bool __init kona_ccu_init(struct ccu_data *ccu)
>> -{
>> -       unsigned long flags;
>> -       unsigned int which;
>> -       struct clk **clks = ccu->clk_data.clks;
>> -       bool success = true;
>> -
>> -       flags = ccu_lock(ccu);
>> -       __ccu_write_enable(ccu);
>> -
>> -       for (which = 0; which < ccu->clk_data.clk_num; which++) {
>> -               struct kona_clk *bcm_clk;
>> -
>> -               if (!clks[which])
>> -                       continue;
>> -               bcm_clk = to_kona_clk(__clk_get_hw(clks[which]));
>> -               success &= __kona_clk_init(bcm_clk);
>> -       }
>>  
>>         __ccu_write_disable(ccu);
>>         ccu_unlock(ccu, flags);
>> -       return success;
>> +
>> +       return ret;
>>  }
> 
> Does this prepare callback "enable" a clock? E.g does a line NOT toggle
> at a rate prior to this call, and then after this call completes that
> same line is now toggling at a rate?

Hmmm.  Good question.

The state of the gate (if present) will not change, so it'll continue
toggling if it was before, and will not start if it was not.  But once
this is done we'll know whether the hardware is enabled or not because
we'll have read its value.

>>  
>> -/* Clock operations */
>> +static void kona_clk_unprepare(struct clk_hw *hw)
>> +{
>> +       /* Nothing to do. */
>> +}
> 
> Is doing nothing the right thing to do? Could power be saved somehow if
> the .unprepare callback really gets called? Remember that if .unprepare

This is again a skeleton in place now because the prerequisite code will
actually fill this in with something to do.  But in any case I use the
enable/disable methods for gate control; the main purpose for these
prepare methods will be to prepare and enable the prerequisite clock.
I may need to think a bit more about what belongs in the prepare
function

> actually runs (because struct clk->prepare_count == 0) then the next
> call to clk_prepare will actually call your .prepare callback and set up
> the prereq clocks again. So the prereq clock initialization is no longer
> a one-time thing, which might afford you some optimizations.

I'll think about this.  Already the code caches the current state
of the hardware and doesn't update the hardware unless it changes
that state.  If I maintained an initialized flag (which I just got
rid of) I could probably avoid taking the spin lock, etc., for
some savings.

This switch over to using the prepare method to do the
initialization may not have been as well thought out as it
should have been.  It changes from "initialize all clocks, once,
all at once" to "initialize each clock only when it's needed
(but every time it's needed)."

					-Alex

> Regards,
> Mike
> 
>>  
>>  static int kona_peri_clk_enable(struct clk_hw *hw)
>>  {
>> @@ -1264,6 +1258,8 @@ static int kona_peri_clk_set_rate(struct clk_hw *hw, unsigned long rate,
>>  }
>>  
>>  struct clk_ops kona_peri_clk_ops = {
>> +       .prepare = kona_clk_prepare,
>> +       .unprepare = kona_clk_unprepare,
>>         .enable = kona_peri_clk_enable,
>>         .disable = kona_peri_clk_disable,
>>         .is_enabled = kona_peri_clk_is_enabled,
>> diff --git a/drivers/clk/bcm/clk-kona.h b/drivers/clk/bcm/clk-kona.h
>> index e9a8466..3409111 100644
>> --- a/drivers/clk/bcm/clk-kona.h
>> +++ b/drivers/clk/bcm/clk-kona.h
>> @@ -511,6 +511,5 @@ extern u64 scaled_div_build(struct bcm_clk_div *div, u32 div_value,
>>  extern struct clk *kona_clk_setup(struct kona_clk *bcm_clk);
>>  extern void __init kona_dt_ccu_setup(struct ccu_data *ccu,
>>                                 struct device_node *node);
>> -extern bool __init kona_ccu_init(struct ccu_data *ccu);
>>  
>>  #endif /* _CLK_KONA_H */
>> -- 
>> 1.9.1
>>

Re: [PATCH v4 1/7] clk: kona: allow nested ccu_write_enable() requests

[Mike Turquette]

Quoting Alex Elder (2014-05-30 20:46:46)
> On 05/30/2014 06:28 PM, Mike Turquette wrote:
> > Quoting Alex Elder (2014-05-30 13:53:02)
> >> Use a counter rather than a Boolean to track whether write access to
> >> a CCU has been enabled or not.  This will allow more than one of
> >> these requests to be nested.
> >>
> >> Note that __ccu_write_enable() and __ccu_write_disable() calls all
> >> come in pairs, and they are always surrounded immediately by calls
> >> to ccu_lock() and ccu_unlock().
> >>
> >> Signed-off-by: Alex Elder <elder@linaro.org>
> >> ---
> >>  drivers/clk/bcm/clk-kona.c | 14 ++++----------
> >>  drivers/clk/bcm/clk-kona.h |  2 +-
> >>  2 files changed, 5 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/clk/bcm/clk-kona.c b/drivers/clk/bcm/clk-kona.c
> >> index 95af2e6..ee8e988 100644
> >> --- a/drivers/clk/bcm/clk-kona.c
> >> +++ b/drivers/clk/bcm/clk-kona.c
> >> @@ -170,13 +170,8 @@ static inline void ccu_unlock(struct ccu_data *ccu, unsigned long flags)
> >>   */
> >>  static inline void __ccu_write_enable(struct ccu_data *ccu)
> > 
> > Per Documentation/CodingStyle, chapter 15, "the inline disease", it
> > might be best to not inline these functions.
> 
> This was not intentional.  I normally only inline things
> defined in header files, and maybe this is an artifact of
> having been in a header at one time.  I don't know, I'll get
> rid of the inline.
> 
> > 
> >>  {
> >> -       if (ccu->write_enabled) {
> >> -               pr_err("%s: access already enabled for %s\n", __func__,
> >> -                       ccu->name);
> >> -               return;
> >> -       }
> >> -       ccu->write_enabled = true;
> >> -       __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD | 1);
> >> +       if (!ccu->write_enabled++)
> >> +               __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD | 1);
> >>  }
> >>  
> >>  static inline void __ccu_write_disable(struct ccu_data *ccu)
> >> @@ -186,9 +181,8 @@ static inline void __ccu_write_disable(struct ccu_data *ccu)
> >>                         ccu->name);
> >>                 return;
> >>         }
> >> -
> >> -       __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD);
> >> -       ccu->write_enabled = false;
> >> +       if (!--ccu->write_enabled)
> >> +               __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD);
> > 
> > What happens if calls to __ccu_write_enable and __ccu_write_disable are
> > unbalanced? It would be better to catch that case and throw a WARN:
> 
> You can't see it in the diff, but that's what happens
> (well, it's a pr_err(), not a WARN()).   I think a WARN()
> is probably right in this case though.
> 
> >       if (WARN_ON(ccu->write_enabled == 0))
> >               return;
> > 
> >       if (--ccu->write_enabled > 0)
> >               return;
> > 
> >       __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD);
> > 
> >>  }
> >>  
> >>  /*
> >> diff --git a/drivers/clk/bcm/clk-kona.h b/drivers/clk/bcm/clk-kona.h
> >> index 2537b30..e9a8466 100644
> >> --- a/drivers/clk/bcm/clk-kona.h
> >> +++ b/drivers/clk/bcm/clk-kona.h
> >> @@ -478,7 +478,7 @@ struct ccu_policy {
> >>  struct ccu_data {
> >>         void __iomem *base;     /* base of mapped address space */
> >>         spinlock_t lock;        /* serialization lock */
> >> -       bool write_enabled;     /* write access is currently enabled */
> >> +       u32 write_enabled;      /* write access enable count */
> > 
> > Why u32? An unsigned int will do just nicely here.
> 
> That's a preference of mine.  I almost always favor
> using u32, etc. because they are compact, and explicit
> about the size and signedness.  I "know" an int is 32
> bits, but I still prefer being explicit.
> 
> I'll interpret this as a preference on your part for
> unsigned int, and I have no problem making that change.

It's not a big deal, I was just curious why. Feel free to use whatever
solution you prefer here.

Regards,
Mike

> 
>                                         -Alex
> 
> > Regards,
> > Mike
> > 
> >>         struct ccu_policy policy;
> >>         struct list_head links; /* for ccu_list */
> >>         struct device_node *node;
> >> -- 
> >> 1.9.1
> >>
>