[RFC 00/17] clk: Add per-controller locks to fix deadlocks

28 messages, 7 authors, 2016-11-12 · open the first message on its own page

[RFC 00/17] clk: Add per-controller locks to fix deadlocks

From: Krzysztof Kozlowski <hidden>
Date: 2016-08-16 13:36:36

Hi,

RFC, please, do not apply, maybe except patch #1 which is harmless.


Introduction
============
The patchset brings new entity: clock controller representing a hardware
block.  The clock controller comes with its own prepare lock which
is used then in many places.  The idea is to fix the deadlock mentioned
in commit 10ff4c5239a1 ("i2c: exynos5: Fix possible ABBA deadlock by keeping
I2C clock prepared") and commit 34e81ad5f0b6 ("i2c: s3c2410: fix ABBA deadlock
by keeping clock prepared").


Disclaimer
==========
Request for comments, so:
1. Only exynos_defconfig builds,
2. A lot of FIXME/TODO note still,
3. Checkpatch not run, lines not aligned,
4. Other (non-exynos) drivers not converted,
5. Probably not yet bisectable,
6. Locking became quite complex.
   The previous one lock was simple.  Inefficient and dead-lock prone but
   simple.  Because of clock hierarchy spanning through controllers, the
   new locking became quite complicated.  I don't like it but...


Details
=======
In Exynos-based boards case the deadlock occurs between clock's
prepare_lock and regmap-i2c's lock:

CPU #0:                             CPU #1:
lock(regmap)
                                    s2mps11-clk: clk_prepare_lock()

i2c-exynos: clk_prepare_lock() - wait
                                    lock(regmap) - wait

The clk_prepare_lock() on both CPUs come from different clock drivers
and components:
1. I2C clock is part of SoC block and is required by i2c-s3c2410/i2c-exynos5
   driver,
2. S2MPS11 clock is separate device, however driver uses I2C regmap.

The deadlock was reported by lockdep (always) and was happening
in 20% of boots of Odroid XU3 with multi_v7 defconfig.  Workaround for
deadlock was implemented by removing prepare/unprepare calls from I2C
transfers.  However these are just workarounds... which after applying
this patch can be reverted.

Additionally Marek Szyprowski's work on domains/clocks/pinctrl exposed
the deadlock again in different configuration.


Comments as usual are welcomed.

Best regards,
Krzysztof

Krzysztof Kozlowski (17):
  clk: bcm2835: Rename clk_register to avoid name conflict
  clk: Add clock controller to fine-grain the prepare lock
  clk: s2mps11: Switch to new clock controller API
  clk: samsung: Allocate a clock controller in context
  clk: fixed-rate: Switch to new clock controller API
  clk: gate: Switch to new clock controller API
  clk: mux: Switch to new clock controller API
  clk: fixed-factor: Switch to new clock controller API
  clk: divider: Switch to new clock controller API
  clk: composite: Switch to new clock controller API
  clk: gpio: Switch to new clock controller API
  ASoC: samsung: Switch to new clock controller API
  clk: samsung: audss: samsung: Switch to new clock controller API
  clk: samsung: clkout: samsung: Switch to new clock controller API
  clk: Use per-controller locking
  Revert "i2c: exynos5: Fix possible ABBA deadlock by keeping I2C clock
    prepared"
  Revert "i2c: s3c2410: fix ABBA deadlock by keeping clock prepared"

 drivers/clk/bcm/clk-bcm2835.c           |   8 +-
 drivers/clk/clk-composite.c             |   8 +-
 drivers/clk/clk-divider.c               |  10 +-
 drivers/clk/clk-fixed-factor.c          |  11 +-
 drivers/clk/clk-fixed-rate.c            |  28 +-
 drivers/clk/clk-fractional-divider.c    |   5 +-
 drivers/clk/clk-gate.c                  |   8 +-
 drivers/clk/clk-gpio.c                  |  29 +-
 drivers/clk/clk-mux.c                   |  32 ++-
 drivers/clk/clk-s2mps11.c               |  10 +-
 drivers/clk/clk.c                       | 456 +++++++++++++++++++++++++++-----
 drivers/clk/samsung/clk-exynos-audss.c  |  30 ++-
 drivers/clk/samsung/clk-exynos-clkout.c |  11 +-
 drivers/clk/samsung/clk.c               |  25 +-
 drivers/clk/samsung/clk.h               |   1 +
 drivers/i2c/busses/i2c-exynos5.c        |  24 +-
 drivers/i2c/busses/i2c-s3c2410.c        |  23 +-
 include/linux/clk-provider.h            |  88 ++++--
 include/linux/clk.h                     |   1 +
 sound/soc/samsung/i2s.c                 |  13 +-
 20 files changed, 612 insertions(+), 209 deletions(-)

-- 
1.9.1

[RFC 04/17] clk: samsung: Allocate a clock controller in context

From: Krzysztof Kozlowski <hidden>
Date: 2016-08-16 13:36:42

Allocate a clock controller and store it in context so it will be passed
later for creating clocks.

Signed-off-by: Krzysztof Kozlowski <redacted>
---
 drivers/clk/samsung/clk.c | 4 ++++
 drivers/clk/samsung/clk.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
index b7d87d6db9dc..fdeb35a48d3a 100644
--- a/drivers/clk/samsung/clk.c
+++ b/drivers/clk/samsung/clk.c

@@ -67,6 +67,10 @@ struct samsung_clk_provider *__init samsung_clk_init(struct device_node *np,
 	if (!ctx)
 		panic("could not allocate clock provider context.\n");
 
+	ctx->clk_ctrl = clk_ctrl_register(NULL);
+	if (!ctx->clk_ctrl)
+		panic("could not allocate clock provider controller.\n");
+
 	clk_table = kcalloc(nr_clks, sizeof(struct clk *), GFP_KERNEL);
 	if (!clk_table)
 		panic("could not allocate clock lookup table\n");

diff --git a/drivers/clk/samsung/clk.h b/drivers/clk/samsung/clk.h
index da3bdebabf1e..cb0ef6266b6d 100644
--- a/drivers/clk/samsung/clk.h
+++ b/drivers/clk/samsung/clk.h

@@ -28,6 +28,7 @@ struct samsung_clk_provider {
 	void __iomem *reg_base;
 	struct clk_onecell_data clk_data;
 	spinlock_t lock;
+	struct clk_ctrl *clk_ctrl;
 };
 
 /**

-- 
1.9.1

[RFC 06/17] clk: gate: Switch to new clock controller API

From: Krzysztof Kozlowski <hidden>
Date: 2016-08-16 13:36:45

Allocate a clock controller and use new clk_register_with_ctrl() API.

Signed-off-by: Krzysztof Kozlowski <redacted>
---
 drivers/clk/clk-gate.c       | 8 +++++---
 drivers/clk/samsung/clk.c    | 5 +++--
 include/linux/clk-provider.h | 6 ++++--
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c
index 4e691e35483a..5eea0457778d 100644
--- a/drivers/clk/clk-gate.c
+++ b/drivers/clk/clk-gate.c

@@ -120,7 +120,8 @@ EXPORT_SYMBOL_GPL(clk_gate_ops);
  * @clk_gate_flags: gate-specific flags for this clock
  * @lock: shared register lock for this clock
  */
-struct clk_hw *clk_hw_register_gate(struct device *dev, const char *name,
+struct clk_hw *clk_hw_register_gate(struct device *dev, struct clk_ctrl *ctrl,
+		const char *name,
 		const char *parent_name, unsigned long flags,
 		void __iomem *reg, u8 bit_idx,
 		u8 clk_gate_flags, spinlock_t *lock)

@@ -166,14 +167,15 @@ struct clk_hw *clk_hw_register_gate(struct device *dev, const char *name,
 }
 EXPORT_SYMBOL_GPL(clk_hw_register_gate);
 
-struct clk *clk_register_gate(struct device *dev, const char *name,
+struct clk *clk_register_gate(struct device *dev, struct clk_ctrl *ctrl,
+		const char *name,
 		const char *parent_name, unsigned long flags,
 		void __iomem *reg, u8 bit_idx,
 		u8 clk_gate_flags, spinlock_t *lock)
 {
 	struct clk_hw *hw;
 
-	hw = clk_hw_register_gate(dev, name, parent_name, flags, reg,
+	hw = clk_hw_register_gate(dev, ctrl, name, parent_name, flags, reg,
 				  bit_idx, clk_gate_flags, lock);
 	if (IS_ERR(hw))
 		return ERR_CAST(hw);

diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
index cb1e4398d6e5..79332011f258 100644
--- a/drivers/clk/samsung/clk.c
+++ b/drivers/clk/samsung/clk.c

@@ -268,8 +268,9 @@ void __init samsung_clk_register_gate(struct samsung_clk_provider *ctx,
 	unsigned int idx, ret;
 
 	for (idx = 0; idx < nr_clk; idx++, list++) {
-		clk = clk_register_gate(NULL, list->name, list->parent_name,
-				list->flags, ctx->reg_base + list->offset,
+		clk = clk_register_gate(NULL, ctx->clk_ctrl, list->name,
+				list->parent_name, list->flags,
+				ctx->reg_base + list->offset,
 				list->bit_idx, list->gate_flags, &ctx->lock);
 		if (IS_ERR(clk)) {
 			pr_err("%s: failed to register clock %s\n", __func__,

diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 2ee4e337e784..289290655283 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h

@@ -340,11 +340,13 @@ struct clk_gate {
 #define CLK_GATE_HIWORD_MASK		BIT(1)
 
 extern const struct clk_ops clk_gate_ops;
-struct clk *clk_register_gate(struct device *dev, const char *name,
+struct clk *clk_register_gate(struct device *dev, struct clk_ctrl *ctrl,
+		const char *name,
 		const char *parent_name, unsigned long flags,
 		void __iomem *reg, u8 bit_idx,
 		u8 clk_gate_flags, spinlock_t *lock);
-struct clk_hw *clk_hw_register_gate(struct device *dev, const char *name,
+struct clk_hw *clk_hw_register_gate(struct device *dev, struct clk_ctrl *ctrl,
+		const char *name,
 		const char *parent_name, unsigned long flags,
 		void __iomem *reg, u8 bit_idx,
 		u8 clk_gate_flags, spinlock_t *lock);

-- 
1.9.1

[RFC 05/17] clk: fixed-rate: Switch to new clock controller API

From: Krzysztof Kozlowski <hidden>
Date: 2016-08-16 13:36:46

Allocate a clock controller and use new clk_register_with_ctrl() API.

Signed-off-by: Krzysztof Kozlowski <redacted>
---
 drivers/clk/clk-fixed-rate.c | 28 ++++++++++++++++++----------
 drivers/clk/samsung/clk.c    |  2 +-
 include/linux/clk-provider.h | 10 ++++++++--
 3 files changed, 27 insertions(+), 13 deletions(-)

diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c
index 2edb39342a02..0a5c4f2105da 100644
--- a/drivers/clk/clk-fixed-rate.c
+++ b/drivers/clk/clk-fixed-rate.c

@@ -55,6 +55,7 @@ EXPORT_SYMBOL_GPL(clk_fixed_rate_ops);
  * @fixed_accuracy: non-adjustable clock rate
  */
 struct clk_hw *clk_hw_register_fixed_rate_with_accuracy(struct device *dev,
+		struct clk_ctrl *ctrl, // FIXME: re-align
 		const char *name, const char *parent_name, unsigned long flags,
 		unsigned long fixed_rate, unsigned long fixed_accuracy)
 {

@@ -81,7 +82,7 @@ struct clk_hw *clk_hw_register_fixed_rate_with_accuracy(struct device *dev,
 
 	/* register the clock */
 	hw = &fixed->hw;
-	ret = clk_hw_register(dev, hw);
+	ret = clk_hw_register_with_ctrl(dev, ctrl, hw);
 	if (ret) {
 		kfree(fixed);
 		hw = ERR_PTR(ret);

@@ -92,13 +93,14 @@ struct clk_hw *clk_hw_register_fixed_rate_with_accuracy(struct device *dev,
 EXPORT_SYMBOL_GPL(clk_hw_register_fixed_rate_with_accuracy);
 
 struct clk *clk_register_fixed_rate_with_accuracy(struct device *dev,
+		struct clk_ctrl *ctrl, // FIXME: re-align
 		const char *name, const char *parent_name, unsigned long flags,
 		unsigned long fixed_rate, unsigned long fixed_accuracy)
 {
 	struct clk_hw *hw;
 
-	hw = clk_hw_register_fixed_rate_with_accuracy(dev, name, parent_name,
-			flags, fixed_rate, fixed_accuracy);
+	hw = clk_hw_register_fixed_rate_with_accuracy(dev, ctrl, name,
+			parent_name, flags, fixed_rate, fixed_accuracy);
 	if (IS_ERR(hw))
 		return ERR_CAST(hw);
 	return hw->clk;

@@ -114,21 +116,27 @@ EXPORT_SYMBOL_GPL(clk_register_fixed_rate_with_accuracy);
  * @flags: framework-specific flags
  * @fixed_rate: non-adjustable clock rate
  */
-struct clk_hw *clk_hw_register_fixed_rate(struct device *dev, const char *name,
+struct clk_hw *clk_hw_register_fixed_rate(struct device *dev,
+		struct clk_ctrl *ctrl, // FIXME: re-align
+		const char *name,
 		const char *parent_name, unsigned long flags,
 		unsigned long fixed_rate)
 {
-	return clk_hw_register_fixed_rate_with_accuracy(dev, name, parent_name,
-						     flags, fixed_rate, 0);
+	return clk_hw_register_fixed_rate_with_accuracy(dev, ctrl, name,
+							parent_name, flags,
+							fixed_rate, 0);
 }
 EXPORT_SYMBOL_GPL(clk_hw_register_fixed_rate);
 
-struct clk *clk_register_fixed_rate(struct device *dev, const char *name,
+struct clk *clk_register_fixed_rate(struct device *dev,
+		struct clk_ctrl *ctrl, // FIXME: re-align
+		const char *name,
 		const char *parent_name, unsigned long flags,
 		unsigned long fixed_rate)
 {
-	return clk_register_fixed_rate_with_accuracy(dev, name, parent_name,
-						     flags, fixed_rate, 0);
+	return clk_register_fixed_rate_with_accuracy(dev, ctrl, name,
+						     parent_name, flags,
+						     fixed_rate, 0);
 }
 EXPORT_SYMBOL_GPL(clk_register_fixed_rate);

@@ -174,7 +182,7 @@ void of_fixed_clk_setup(struct device_node *node)
 
 	of_property_read_string(node, "clock-output-names", &clk_name);
 
-	clk = clk_register_fixed_rate_with_accuracy(NULL, clk_name, NULL,
+	clk = clk_register_fixed_rate_with_accuracy(NULL, NULL, clk_name, NULL,
 						    0, rate, accuracy);
 	if (!IS_ERR(clk))
 		of_clk_add_provider(node, of_clk_src_simple_get, clk);

diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
index fdeb35a48d3a..cb1e4398d6e5 100644
--- a/drivers/clk/samsung/clk.c
+++ b/drivers/clk/samsung/clk.c

@@ -147,7 +147,7 @@ void __init samsung_clk_register_fixed_rate(struct samsung_clk_provider *ctx,
 	unsigned int idx, ret;
 
 	for (idx = 0; idx < nr_clk; idx++, list++) {
-		clk = clk_register_fixed_rate(NULL, list->name,
+		clk = clk_register_fixed_rate(NULL, ctx->clk_ctrl, list->name,
 			list->parent_name, list->flags, list->fixed_rate);
 		if (IS_ERR(clk)) {
 			pr_err("%s: failed to register clock %s\n", __func__,

diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 3589f164ff94..2ee4e337e784 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h

@@ -283,17 +283,23 @@ struct clk_fixed_rate {
 #define to_clk_fixed_rate(_hw) container_of(_hw, struct clk_fixed_rate, hw)
 
 extern const struct clk_ops clk_fixed_rate_ops;
-struct clk *clk_register_fixed_rate(struct device *dev, const char *name,
+struct clk *clk_register_fixed_rate(struct device *dev,
+		struct clk_ctrl *ctrl, // FIXME: re-align
+		const char *name,
 		const char *parent_name, unsigned long flags,
 		unsigned long fixed_rate);
-struct clk_hw *clk_hw_register_fixed_rate(struct device *dev, const char *name,
+struct clk_hw *clk_hw_register_fixed_rate(struct device *dev,
+		struct clk_ctrl *ctrl, // FIXME: re-align
+		const char *name,
 		const char *parent_name, unsigned long flags,
 		unsigned long fixed_rate);
 struct clk *clk_register_fixed_rate_with_accuracy(struct device *dev,
+		struct clk_ctrl *ctrl, // FIXME: re-align
 		const char *name, const char *parent_name, unsigned long flags,
 		unsigned long fixed_rate, unsigned long fixed_accuracy);
 void clk_unregister_fixed_rate(struct clk *clk);
 struct clk_hw *clk_hw_register_fixed_rate_with_accuracy(struct device *dev,
+		struct clk_ctrl *ctrl, // FIXME: re-align
 		const char *name, const char *parent_name, unsigned long flags,
 		unsigned long fixed_rate, unsigned long fixed_accuracy);
 void clk_hw_unregister_fixed_rate(struct clk_hw *hw);

-- 
1.9.1

[RFC 14/17] clk: samsung: clkout: samsung: Switch to new clock controller API

From: Krzysztof Kozlowski <hidden>
Date: 2016-08-16 13:36:55

Allocate a clock controller and use new clk_register_with_ctrl() API.

Signed-off-by: Krzysztof Kozlowski <redacted>
---
 drivers/clk/samsung/clk-exynos-clkout.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/samsung/clk-exynos-clkout.c b/drivers/clk/samsung/clk-exynos-clkout.c
index 96fab6cfb202..9c57a623e741 100644
--- a/drivers/clk/samsung/clk-exynos-clkout.c
+++ b/drivers/clk/samsung/clk-exynos-clkout.c

@@ -26,6 +26,7 @@
 #define EXYNOS5_CLKOUT_MUX_MASK		0x1f
 
 struct exynos_clkout {
+	struct clk_ctrl *clk_ctrl;
 	struct clk_gate gate;
 	struct clk_mux mux;
 	spinlock_t slock;

@@ -66,6 +67,10 @@ static void __init exynos_clkout_init(struct device_node *node, u32 mux_mask)
 	if (!clkout)
 		return;
 
+	clkout->clk_ctrl = clk_ctrl_register(NULL);
+	if (IS_ERR(clkout->clk_ctrl))
+		goto free_clkout;
+
 	spin_lock_init(&clkout->slock);
 
 	parent_count = 0;

@@ -84,7 +89,7 @@ static void __init exynos_clkout_init(struct device_node *node, u32 mux_mask)
 	}
 
 	if (!parent_count)
-		goto free_clkout;
+		goto free_ctrl;
 
 	clkout->reg = of_iomap(node, 0);
 	if (!clkout->reg)

@@ -100,7 +105,7 @@ static void __init exynos_clkout_init(struct device_node *node, u32 mux_mask)
 	clkout->mux.shift = EXYNOS_CLKOUT_MUX_SHIFT;
 	clkout->mux.lock = &clkout->slock;
 
-	clkout->clk_table[0] = clk_register_composite(NULL, "clkout",
+	clkout->clk_table[0] = clk_register_composite(NULL, clkout->clk_ctrl, "clkout",
 				parent_names, parent_count, &clkout->mux.hw,
 				&clk_mux_ops, NULL, NULL, &clkout->gate.hw,
 				&clk_gate_ops, CLK_SET_RATE_PARENT

@@ -126,6 +131,8 @@ clks_put:
 	for (i = 0; i < EXYNOS_CLKOUT_PARENTS; ++i)
 		if (!IS_ERR(parents[i]))
 			clk_put(parents[i]);
+free_ctrl:
+	clk_ctrl_unregister(clkout->clk_ctrl);
 free_clkout:
 	kfree(clkout);

-- 
1.9.1

[RFC 10/17] clk: composite: Switch to new clock controller API

From: Krzysztof Kozlowski <hidden>
Date: 2016-08-16 13:36:57

Allocate a clock controller and use new clk_register_with_ctrl() API.

Signed-off-by: Krzysztof Kozlowski <redacted>
---
 drivers/clk/clk-composite.c  | 8 +++++---
 include/linux/clk-provider.h | 6 ++++--
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
index 00269de2f390..8ad5c2c4a329 100644
--- a/drivers/clk/clk-composite.c
+++ b/drivers/clk/clk-composite.c

@@ -211,7 +211,8 @@ static void clk_composite_disable(struct clk_hw *hw)
 	gate_ops->disable(gate_hw);
 }
 
-struct clk_hw *clk_hw_register_composite(struct device *dev, const char *name,
+struct clk_hw *clk_hw_register_composite(struct device *dev, struct clk_ctrl *ctrl,
+			const char *name,
 			const char * const *parent_names, int num_parents,
 			struct clk_hw *mux_hw, const struct clk_ops *mux_ops,
 			struct clk_hw *rate_hw, const struct clk_ops *rate_ops,

@@ -324,7 +325,8 @@ err:
 	return hw;
 }
 
-struct clk *clk_register_composite(struct device *dev, const char *name,
+struct clk *clk_register_composite(struct device *dev, struct clk_ctrl *ctrl,
+			const char *name,
 			const char * const *parent_names, int num_parents,
 			struct clk_hw *mux_hw, const struct clk_ops *mux_ops,
 			struct clk_hw *rate_hw, const struct clk_ops *rate_ops,

@@ -333,7 +335,7 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
 {
 	struct clk_hw *hw;
 
-	hw = clk_hw_register_composite(dev, name, parent_names, num_parents,
+	hw = clk_hw_register_composite(dev, ctrl, name, parent_names, num_parents,
 			mux_hw, mux_ops, rate_hw, rate_ops, gate_hw, gate_ops,
 			flags);
 	if (IS_ERR(hw))

diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index cfb3aa3912c5..2bd0a8cb7a9c 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h

@@ -663,14 +663,16 @@ struct clk_composite {
 
 #define to_clk_composite(_hw) container_of(_hw, struct clk_composite, hw)
 
-struct clk *clk_register_composite(struct device *dev, const char *name,
+struct clk *clk_register_composite(struct device *dev, struct clk_ctrl *ctrl,
+		const char *name,
 		const char * const *parent_names, int num_parents,
 		struct clk_hw *mux_hw, const struct clk_ops *mux_ops,
 		struct clk_hw *rate_hw, const struct clk_ops *rate_ops,
 		struct clk_hw *gate_hw, const struct clk_ops *gate_ops,
 		unsigned long flags);
 void clk_unregister_composite(struct clk *clk);
-struct clk_hw *clk_hw_register_composite(struct device *dev, const char *name,
+struct clk_hw *clk_hw_register_composite(struct device *dev,
+		struct clk_ctrl *ctrl, const char *name,
 		const char * const *parent_names, int num_parents,
 		struct clk_hw *mux_hw, const struct clk_ops *mux_ops,
 		struct clk_hw *rate_hw, const struct clk_ops *rate_ops,

-- 
1.9.1

[RFC 17/17] Revert "i2c: s3c2410: fix ABBA deadlock by keeping clock prepared"

From: Krzysztof Kozlowski <hidden>
Date: 2016-08-16 13:37:14

This reverts commit 34e81ad5f0b60007c95995eb7803da7e00c6c611.

Signed-off-by: Krzysztof Kozlowski <redacted>

Conflicts:
	drivers/i2c/busses/i2c-s3c2410.c
---
 drivers/i2c/busses/i2c-s3c2410.c | 23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)

diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index 38dc1cacfd8b..571250415617 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c

@@ -771,16 +771,14 @@ static int s3c24xx_i2c_xfer(struct i2c_adapter *adap,
 	int retry;
 	int ret;
 
-	ret = clk_enable(i2c->clk);
-	if (ret)
-		return ret;
+	clk_prepare_enable(i2c->clk);
 
 	for (retry = 0; retry < adap->retries; retry++) {
 
 		ret = s3c24xx_i2c_doxfer(i2c, msgs, num);
 
 		if (ret != -EAGAIN) {
-			clk_disable(i2c->clk);
+			clk_disable_unprepare(i2c->clk);
 			return ret;
 		}

@@ -789,7 +787,7 @@ static int s3c24xx_i2c_xfer(struct i2c_adapter *adap,
 		udelay(100);
 	}
 
-	clk_disable(i2c->clk);
+	clk_disable_unprepare(i2c->clk);
 	return -EREMOTEIO;
 }

@@ -1165,7 +1163,7 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev)
 	}
 
 	ret = s3c24xx_i2c_init(i2c);
-	clk_disable(i2c->clk);
+	clk_disable_unprepare(i2c->clk);
 	if (ret != 0) {
 		dev_err(&pdev->dev, "I2C controller init failed\n");
 		clk_unprepare(i2c->clk);

@@ -1180,7 +1178,6 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev)
 		i2c->irq = ret = platform_get_irq(pdev, 0);
 		if (ret <= 0) {
 			dev_err(&pdev->dev, "cannot find IRQ\n");
-			clk_unprepare(i2c->clk);
 			return ret;
 		}

@@ -1188,7 +1185,6 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev)
 				       0, dev_name(&pdev->dev), i2c);
 		if (ret != 0) {
 			dev_err(&pdev->dev, "cannot claim IRQ %d\n", i2c->irq);
-			clk_unprepare(i2c->clk);
 			return ret;
 		}
 	}

@@ -1196,7 +1192,6 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev)
 	ret = s3c24xx_i2c_register_cpufreq(i2c);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "failed to register cpufreq notifier\n");
-		clk_unprepare(i2c->clk);
 		return ret;
 	}

@@ -1218,7 +1213,6 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "failed to add bus to i2c core\n");
 		pm_runtime_disable(&pdev->dev);
 		s3c24xx_i2c_deregister_cpufreq(i2c);
-		clk_unprepare(i2c->clk);
 		return ret;
 	}

@@ -1230,8 +1224,6 @@ static int s3c24xx_i2c_remove(struct platform_device *pdev)
 {
 	struct s3c24xx_i2c *i2c = platform_get_drvdata(pdev);
 
-	clk_unprepare(i2c->clk);
-
 	pm_runtime_disable(&pdev->dev);
 
 	s3c24xx_i2c_deregister_cpufreq(i2c);

@@ -1262,16 +1254,13 @@ static int s3c24xx_i2c_resume_noirq(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct s3c24xx_i2c *i2c = platform_get_drvdata(pdev);
-	int ret;
 
 	if (!IS_ERR(i2c->sysreg))
 		regmap_write(i2c->sysreg, EXYNOS5_SYS_I2C_CFG, i2c->sys_i2c_cfg);
 
-	ret = clk_enable(i2c->clk);
-	if (ret)
-		return ret;
+	clk_prepare_enable(i2c->clk);
 	s3c24xx_i2c_init(i2c);
-	clk_disable(i2c->clk);
+	clk_disable_unprepare(i2c->clk);
 	i2c->suspended = 0;
 
 	return 0;

-- 
1.9.1

[RFC 16/17] Revert "i2c: exynos5: Fix possible ABBA deadlock by keeping I2C clock prepared"

From: Krzysztof Kozlowski <hidden>
Date: 2016-08-16 13:37:50

This reverts commit 10ff4c5239a137abfc896ec73ef3d15a0f86a16a.
---
 drivers/i2c/busses/i2c-exynos5.c | 24 +++++-------------------
 1 file changed, 5 insertions(+), 19 deletions(-)

diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
index c0e3ada02876..8710052eeb6b 100644
--- a/drivers/i2c/busses/i2c-exynos5.c
+++ b/drivers/i2c/busses/i2c-exynos5.c

@@ -671,9 +671,7 @@ static int exynos5_i2c_xfer(struct i2c_adapter *adap,
 		return -EIO;
 	}
 
-	ret = clk_enable(i2c->clk);
-	if (ret)
-		return ret;
+	clk_prepare_enable(i2c->clk);
 
 	for (i = 0; i < num; i++, msgs++) {
 		stop = (i == num - 1);

@@ -697,7 +695,7 @@ static int exynos5_i2c_xfer(struct i2c_adapter *adap,
 	}
 
  out:
-	clk_disable(i2c->clk);
+	clk_disable_unprepare(i2c->clk);
 	return ret;
 }

@@ -749,9 +747,7 @@ static int exynos5_i2c_probe(struct platform_device *pdev)
 		return -ENOENT;
 	}
 
-	ret = clk_prepare_enable(i2c->clk);
-	if (ret)
-		return ret;
+	clk_prepare_enable(i2c->clk);
 
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	i2c->regs = devm_ioremap_resource(&pdev->dev, mem);

@@ -803,10 +799,6 @@ static int exynos5_i2c_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, i2c);
 
-	clk_disable(i2c->clk);
-
-	return 0;
-
  err_clk:
 	clk_disable_unprepare(i2c->clk);
 	return ret;

@@ -818,8 +810,6 @@ static int exynos5_i2c_remove(struct platform_device *pdev)
 
 	i2c_del_adapter(&i2c->adap);
 
-	clk_unprepare(i2c->clk);
-
 	return 0;
 }

@@ -831,8 +821,6 @@ static int exynos5_i2c_suspend_noirq(struct device *dev)
 
 	i2c->suspended = 1;
 
-	clk_unprepare(i2c->clk);
-
 	return 0;
 }

@@ -842,9 +830,7 @@ static int exynos5_i2c_resume_noirq(struct device *dev)
 	struct exynos5_i2c *i2c = platform_get_drvdata(pdev);
 	int ret = 0;
 
-	ret = clk_prepare_enable(i2c->clk);
-	if (ret)
-		return ret;
+	clk_prepare_enable(i2c->clk);
 
 	ret = exynos5_hsi2c_clock_setup(i2c);
 	if (ret) {

@@ -853,7 +839,7 @@ static int exynos5_i2c_resume_noirq(struct device *dev)
 	}
 
 	exynos5_i2c_init(i2c);
-	clk_disable(i2c->clk);
+	clk_disable_unprepare(i2c->clk);
 	i2c->suspended = 0;
 
 	return 0;

-- 
1.9.1

[RFC 15/17] clk: Use per-controller locking

From: Krzysztof Kozlowski <hidden>
Date: 2016-08-16 13:37:51

Replace global prepare lock with a more fine-grained solution - per
clock controller locks.  The global lock is unfortunately still present
and used on some paths but at least the prepare path could be
simplified.

This directly removes the deadlocks mentioned in:
1. commit 10ff4c5239a1 ("i2c: exynos5: Fix possible ABBA deadlock by
   keeping I2C clock prepared"),
2. commit 34e81ad5f0b6 ("i2c: s3c2410: fix ABBA deadlock by keeping
   clock prepared").

The locking got unfortunately much more complicated.  Following locking
design was chosen:
1. For prepare/unprepare paths: lock only clock controller and its
   parents.
2. For recalc rates paths: lock global lock, the controller and its
   children.
3. For reparent paths: lock entire tree up down (children and parents)
   and the global lock as well.

In each case of traversing the clock hierarchy, the locking of
controllers is always from children to parents.

Signed-off-by: Krzysztof Kozlowski <redacted>
---
 drivers/clk/clk.c | 156 ++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 100 insertions(+), 56 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index ee1cedfbaa29..37113f7cef64 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c

@@ -710,11 +710,11 @@ EXPORT_SYMBOL_GPL(__clk_mux_determine_rate_closest);
 
 static void clk_core_unprepare(struct clk_core *core)
 {
-	lockdep_assert_held(&prepare_lock);
-
 	if (!core)
 		return;
 
+	lockdep_assert_held(&core->ctrl->prepare_lock);
+
 	if (WARN_ON(core->prepare_count == 0))
 		return;

@@ -737,9 +737,13 @@ static void clk_core_unprepare(struct clk_core *core)
 
 static void clk_core_unprepare_lock(struct clk_core *core)
 {
-	clk_prepare_lock();
+	/*
+	 * TODO: optimize, no need to lock parent controllers
+	 * if prepare_count > 1
+	 */
+	clk_prepare_lock_parents_locked(core);
 	clk_core_unprepare(core);
-	clk_prepare_unlock();
+	clk_prepare_unlock_parents(core);
 }
 
 /**

@@ -766,11 +770,11 @@ static int clk_core_prepare(struct clk_core *core)
 {
 	int ret = 0;
 
-	lockdep_assert_held(&prepare_lock);
-
 	if (!core)
 		return 0;
 
+	lockdep_assert_held(&core->ctrl->prepare_lock);
+
 	if (core->prepare_count == 0) {
 		ret = clk_core_prepare(core->parent);
 		if (ret)

@@ -798,9 +802,13 @@ static int clk_core_prepare_lock(struct clk_core *core)
 {
 	int ret;
 
-	clk_prepare_lock();
+	/*
+	 * TODO: optimize, no need to lock parent controllers
+	 * if prepare_count >= 1 already
+	 */
+	clk_prepare_lock_parents_locked(core);
 	ret = clk_core_prepare(core);
-	clk_prepare_unlock();
+	clk_prepare_unlock_parents(core);
 
 	return ret;
 }

@@ -1055,7 +1063,7 @@ static int clk_disable_unused(void)
 		return 0;
 	}
 
-	clk_prepare_lock();
+	clk_prepare_lock_all();
 
 	hlist_for_each_entry(core, &clk_root_list, child_node)
 		clk_disable_unused_subtree(core);

@@ -1069,7 +1077,7 @@ static int clk_disable_unused(void)
 	hlist_for_each_entry(core, &clk_orphan_list, child_node)
 		clk_unprepare_unused_subtree(core);
 
-	clk_prepare_unlock();
+	clk_prepare_unlock_all();
 
 	return 0;
 }

@@ -1081,11 +1089,11 @@ static int clk_core_round_rate_nolock(struct clk_core *core,
 	struct clk_core *parent;
 	long rate;
 
-	lockdep_assert_held(&prepare_lock);
-
 	if (!core)
 		return 0;
 
+	lockdep_assert_held(&core->ctrl->prepare_lock);
+
 	parent = core->parent;
 	if (parent) {
 		req->best_parent_hw = parent->hw;

@@ -1164,13 +1172,13 @@ long clk_round_rate(struct clk *clk, unsigned long rate)
 	if (!clk)
 		return 0;
 
-	clk_prepare_lock();
+	clk_prepare_lock_parents(clk->core);
 
 	clk_core_get_boundaries(clk->core, &req.min_rate, &req.max_rate);
 	req.rate = rate;
 
 	ret = clk_core_round_rate_nolock(clk->core, &req);
-	clk_prepare_unlock();
+	clk_prepare_unlock_parents(clk->core);
 
 	if (ret)
 		return ret;

@@ -1228,7 +1236,7 @@ static void __clk_recalc_accuracies(struct clk_core *core)
 	unsigned long parent_accuracy = 0;
 	struct clk_core *child;
 
-	lockdep_assert_held(&prepare_lock);
+	lockdep_assert_held(&core->ctrl->prepare_lock);
 
 	if (core->parent)
 		parent_accuracy = core->parent->accuracy;

@@ -1247,12 +1255,12 @@ static long clk_core_get_accuracy(struct clk_core *core)
 {
 	unsigned long accuracy;
 
-	clk_prepare_lock();
+	clk_prepare_lock_children(core);
 	if (core && (core->flags & CLK_GET_ACCURACY_NOCACHE))
 		__clk_recalc_accuracies(core);
 
 	accuracy = __clk_get_accuracy(core);
-	clk_prepare_unlock();
+	clk_prepare_unlock_children(core);
 
 	return accuracy;
 }

@@ -1301,7 +1309,7 @@ static void __clk_recalc_rates(struct clk_core *core, unsigned long msg)
 	unsigned long parent_rate = 0;
 	struct clk_core *child;
 
-	lockdep_assert_held(&prepare_lock);
+	lockdep_assert_held(&core->ctrl->prepare_lock);
 
 	old_rate = core->rate;

@@ -1325,13 +1333,14 @@ static unsigned long clk_core_get_rate(struct clk_core *core)
 {
 	unsigned long rate;
 
-	clk_prepare_lock();
+	clk_prepare_lock_tree(core);
 
 	if (core && (core->flags & CLK_GET_RATE_NOCACHE))
 		__clk_recalc_rates(core, 0);
 
 	rate = clk_core_get_rate_nolock(core);
-	clk_prepare_unlock();
+
+	clk_prepare_unlock_tree(core);
 
 	return rate;
 }

@@ -1525,7 +1534,7 @@ static int __clk_speculate_rates(struct clk_core *core,
 	unsigned long new_rate;
 	int ret = NOTIFY_DONE;
 
-	lockdep_assert_held(&prepare_lock);
+	lockdep_assert_held(&core->ctrl->prepare_lock);
 
 	new_rate = clk_recalc(core, parent_rate);

@@ -1867,11 +1876,11 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
 		return 0;
 
 	/* prevent racing with updates to the clock topology */
-	clk_prepare_lock();
+	clk_prepare_lock_tree(clk->core);
 
 	ret = clk_core_set_rate_nolock(clk->core, rate);
 
-	clk_prepare_unlock();
+	clk_prepare_unlock_tree(clk->core);
 
 	return ret;
 }

@@ -1899,7 +1908,7 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
 		return -EINVAL;
 	}
 
-	clk_prepare_lock();
+	clk_prepare_lock_parents(clk->core);
 
 	if (min != clk->min_rate || max != clk->max_rate) {
 		clk->min_rate = min;

@@ -1907,7 +1916,7 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
 		ret = clk_core_set_rate_nolock(clk->core, clk->core->req_rate);
 	}
 
-	clk_prepare_unlock();
+	clk_prepare_unlock_parents(clk->core);
 
 	return ret;
 }

@@ -1958,10 +1967,13 @@ struct clk *clk_get_parent(struct clk *clk)
 	if (!clk)
 		return NULL;
 
-	clk_prepare_lock();
-	/* TODO: Create a per-user clk and change callers to call clk_put */
+	clk_prepare_lock_ctrl(clk->core);
+	/*
+	 * TODO: Create a per-user clk and change callers to call clk_put.
+	 * Switch to tree locking then.
+	 */
 	parent = !clk->core->parent ? NULL : clk->core->parent->hw->clk;
-	clk_prepare_unlock();
+	clk_prepare_unlock_ctrl(clk->core);
 
 	return parent;
 }

@@ -1981,8 +1993,11 @@ static void clk_core_reparent(struct clk_core *core,
 				  struct clk_core *new_parent)
 {
 	clk_reparent(core, new_parent);
+
+	clk_prepare_lock_children(core);
 	__clk_recalc_accuracies(core);
 	__clk_recalc_rates(core, POST_RATE_CHANGE);
+	clk_prepare_unlock_children(core);
 }
 
 void clk_hw_reparent(struct clk_hw *hw, struct clk_hw *new_parent)

@@ -2032,26 +2047,31 @@ static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
 	int ret = 0;
 	int p_index = 0;
 	unsigned long p_rate = 0;
+	struct clk_core *old_parent;
 
 	if (!core)
 		return 0;
 
 	/* prevent racing with updates to the clock topology */
-	clk_prepare_lock();
+	clk_prepare_lock_tree(core);
 
+	old_parent = core->parent;
 	if (core->parent == parent)
-		goto out;
+		goto unlock_tree;
+
+	if (parent && (parent->ctrl != core->ctrl))
+		clk_ctrl_prepare_lock(parent->ctrl);
 
 	/* verify ops for for multi-parent clks */
 	if ((core->num_parents > 1) && (!core->ops->set_parent)) {
 		ret = -ENOSYS;
-		goto out;
+		goto unlock_new_parent;
 	}
 
 	/* check that we are allowed to re-parent if the clock is in use */
 	if ((core->flags & CLK_SET_PARENT_GATE) && core->prepare_count) {
 		ret = -EBUSY;
-		goto out;
+		goto unlock_new_parent;
 	}
 
 	/* try finding the new parent index */

@@ -2061,7 +2081,7 @@ static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
 			pr_debug("%s: clk %s can not be parent of clk %s\n",
 					__func__, parent->name, core->name);
 			ret = p_index;
-			goto out;
+			goto unlock_new_parent;
 		}
 		p_rate = parent->rate;
 	}

@@ -2071,7 +2091,7 @@ static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
 
 	/* abort if a driver objects */
 	if (ret & NOTIFY_STOP_MASK)
-		goto out;
+		goto unlock_new_parent;
 
 	/* do the re-parent */
 	ret = __clk_set_parent(core, parent, p_index);

@@ -2084,8 +2104,13 @@ static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
 		__clk_recalc_accuracies(core);
 	}
 
-out:
-	clk_prepare_unlock();
+
+unlock_new_parent:
+	if (parent && (parent->ctrl != core->ctrl))
+		clk_ctrl_prepare_unlock(parent->ctrl);
+
+unlock_tree:
+	clk_prepare_unlock_oldtree(core, old_parent);
 
 	return ret;
 }

@@ -2148,7 +2173,7 @@ int clk_set_phase(struct clk *clk, int degrees)
 	if (degrees < 0)
 		degrees += 360;
 
-	clk_prepare_lock();
+	clk_prepare_lock_parents(clk->core);
 
 	/* bail early if nothing to do */
 	if (degrees == clk->core->phase)

@@ -2165,7 +2190,7 @@ int clk_set_phase(struct clk *clk, int degrees)
 		clk->core->phase = degrees;
 
 out:
-	clk_prepare_unlock();
+	clk_prepare_unlock_parents(clk->core);
 
 	return ret;
 }

@@ -2175,9 +2200,9 @@ static int clk_core_get_phase(struct clk_core *core)
 {
 	int ret;
 
-	clk_prepare_lock();
+	clk_prepare_lock_parents(core);
 	ret = core->phase;
-	clk_prepare_unlock();
+	clk_prepare_unlock_parents(core);
 
 	return ret;
 }

@@ -2280,13 +2305,13 @@ static int clk_summary_show(struct seq_file *s, void *data)
 	seq_puts(s, "   clock                         enable_cnt  prepare_cnt        rate   accuracy   phase\n");
 	seq_puts(s, "----------------------------------------------------------------------------------------\n");
 
-	clk_prepare_lock();
+	clk_prepare_lock_all();
 
 	for (; *lists; lists++)
 		hlist_for_each_entry(c, *lists, child_node)
 			clk_summary_show_subtree(s, c, 0);
 
-	clk_prepare_unlock();
+	clk_prepare_unlock_all();
 
 	return 0;
 }

@@ -2343,7 +2368,7 @@ static int clk_dump(struct seq_file *s, void *data)
 
 	seq_printf(s, "{");
 
-	clk_prepare_lock();
+	clk_prepare_lock_all();
 
 	for (; *lists; lists++) {
 		hlist_for_each_entry(c, *lists, child_node) {

@@ -2354,7 +2379,7 @@ static int clk_dump(struct seq_file *s, void *data)
 		}
 	}
 
-	clk_prepare_unlock();
+	clk_prepare_unlock_all();
 
 	seq_puts(s, "}\n");
 	return 0;

@@ -2572,7 +2597,7 @@ static int __clk_core_init(struct clk_core *core)
 	if (!core)
 		return -EINVAL;
 
-	clk_prepare_lock();
+	clk_prepare_lock_ctrl(core);
 
 	/* check to see if a clock with this name is already registered */
 	if (clk_core_lookup(core->name)) {

@@ -2696,10 +2721,12 @@ static int __clk_core_init(struct clk_core *core)
 		 * redundant call to the .set_rate op, if it exists
 		 */
 		if (parent) {
+			clk_prepare_lock_children(orphan);
 			__clk_set_parent_before(orphan, parent);
 			__clk_set_parent_after(orphan, parent, NULL);
 			__clk_recalc_accuracies(orphan);
 			__clk_recalc_rates(orphan, 0);
+			clk_prepare_unlock_children(orphan);
 		}
 	}

@@ -2726,7 +2753,7 @@ static int __clk_core_init(struct clk_core *core)
 
 	kref_init(&core->ref);
 out:
-	clk_prepare_unlock();
+	clk_prepare_unlock_ctrl(core);
 
 	if (!ret)
 		clk_debug_register(core);

@@ -2752,18 +2779,19 @@ struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
 	clk->con_id = con_id;
 	clk->max_rate = ULONG_MAX;
 
-	clk_prepare_lock();
+	/* FIXME: Check if core->ctrl is always initialized here (in all paths) */
+	clk_prepare_lock_ctrl(clk->core);
 	hlist_add_head(&clk->clks_node, &hw->core->clks);
-	clk_prepare_unlock();
+	clk_prepare_unlock_ctrl(clk->core);
 
 	return clk;
 }
 
 void __clk_free_clk(struct clk *clk)
 {
-	clk_prepare_lock();
+	clk_prepare_lock_ctrl(clk->core);
 	hlist_del(&clk->clks_node);
-	clk_prepare_unlock();
+	clk_prepare_unlock_ctrl(clk->core);
 
 	kfree(clk);
 }

@@ -2979,12 +3007,18 @@ void clk_unregister(struct clk *clk)
 
 	clk_debug_unregister(clk->core);
 
+	/*
+	 * Take prepare lock again because controller lock
+	 * will have to be released before final clk_release.
+	 */
 	clk_prepare_lock();
+	clk_prepare_lock_parents(clk->core);
 
 	if (clk->core->ops == &clk_nodrv_ops) {
 		pr_err("%s: unregistered clock: %s\n", __func__,
 		       clk->core->name);
-		goto unlock;
+		clk_prepare_unlock_parents(clk->core);
+		goto half_unlock;
 	}
 	/*
 	 * Assign empty clock ops for consumers that might still hold

@@ -3009,8 +3043,10 @@ void clk_unregister(struct clk *clk)
 	if (clk->core->prepare_count)
 		pr_warn("%s: unregistering prepared clock: %s\n",
 					__func__, clk->core->name);
+	clk_prepare_unlock_parents(clk->core);
 	kref_put(&clk->core->ref, __clk_release);
-unlock:
+
+half_unlock:
 	clk_prepare_unlock();
 }
 EXPORT_SYMBOL_GPL(clk_unregister);

@@ -3166,7 +3202,12 @@ void __clk_put(struct clk *clk)
 	if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
 		return;
 
+	/*
+	 * Take prepare lock again because controller lock
+	 * will have to be released before final clk_release.
+	 */
 	clk_prepare_lock();
+	clk_prepare_lock_parents(clk->core);
 
 	hlist_del(&clk->clks_node);
 	if (clk->min_rate > clk->core->req_rate ||

@@ -3174,6 +3215,9 @@ void __clk_put(struct clk *clk)
 		clk_core_set_rate_nolock(clk->core, clk->core->req_rate);
 
 	owner = clk->core->owner;
+
+	clk_prepare_unlock_parents(clk->core);
+
 	kref_put(&clk->core->ref, __clk_release);
 
 	clk_prepare_unlock();

@@ -3213,7 +3257,7 @@ int clk_notifier_register(struct clk *clk, struct notifier_block *nb)
 	if (!clk || !nb)
 		return -EINVAL;
 
-	clk_prepare_lock();
+	clk_prepare_lock_ctrl(clk->core);
 
 	/* search the list of notifiers for this clk */
 	list_for_each_entry(cn, &clk_notifier_list, node)

@@ -3237,7 +3281,7 @@ int clk_notifier_register(struct clk *clk, struct notifier_block *nb)
 	clk->core->notifier_count++;
 
 out:
-	clk_prepare_unlock();
+	clk_prepare_unlock_ctrl(clk->core);
 
 	return ret;
 }

@@ -3262,7 +3306,7 @@ int clk_notifier_unregister(struct clk *clk, struct notifier_block *nb)
 	if (!clk || !nb)
 		return -EINVAL;
 
-	clk_prepare_lock();
+	clk_prepare_lock_ctrl(clk->core);
 
 	list_for_each_entry(cn, &clk_notifier_list, node)
 		if (cn->clk == clk)

@@ -3284,7 +3328,7 @@ int clk_notifier_unregister(struct clk *clk, struct notifier_block *nb)
 		ret = -ENOENT;
 	}
 
-	clk_prepare_unlock();
+	clk_prepare_unlock_ctrl(clk->core);
 
 	return ret;
 }

-- 
1.9.1

[RFC 09/17] clk: divider: Switch to new clock controller API

From: Krzysztof Kozlowski <hidden>
Date: 2016-08-16 13:38:21

Allocate a clock controller and use new clk_register_with_ctrl() API.

Signed-off-by: Krzysztof Kozlowski <redacted>
---
 drivers/clk/clk-divider.c            | 10 +++++++---
 drivers/clk/clk-fractional-divider.c |  5 +++--
 drivers/clk/samsung/clk.c            |  8 ++++----
 include/linux/clk-provider.h         | 13 +++++++++----
 4 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index a0f55bc1ad3d..8e9e89f8c5a8 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c

@@ -490,7 +490,8 @@ static struct clk_hw *_register_divider(struct device *dev, const char *name,
  * @clk_divider_flags: divider-specific flags for this clock
  * @lock: shared register lock for this clock
  */
-struct clk *clk_register_divider(struct device *dev, const char *name,
+struct clk *clk_register_divider(struct device *dev, struct clk_ctrl *ctrl,
+		const char *name,
 		const char *parent_name, unsigned long flags,
 		void __iomem *reg, u8 shift, u8 width,
 		u8 clk_divider_flags, spinlock_t *lock)

@@ -517,7 +518,8 @@ EXPORT_SYMBOL_GPL(clk_register_divider);
  * @clk_divider_flags: divider-specific flags for this clock
  * @lock: shared register lock for this clock
  */
-struct clk_hw *clk_hw_register_divider(struct device *dev, const char *name,
+struct clk_hw *clk_hw_register_divider(struct device *dev,
+		struct clk_ctrl *ctrl, const char *name,
 		const char *parent_name, unsigned long flags,
 		void __iomem *reg, u8 shift, u8 width,
 		u8 clk_divider_flags, spinlock_t *lock)

@@ -541,7 +543,8 @@ EXPORT_SYMBOL_GPL(clk_hw_register_divider);
  * @table: array of divider/value pairs ending with a div set to 0
  * @lock: shared register lock for this clock
  */
-struct clk *clk_register_divider_table(struct device *dev, const char *name,
+struct clk *clk_register_divider_table(struct device *dev,
+		struct clk_ctrl *ctrl, const char *name,
 		const char *parent_name, unsigned long flags,
 		void __iomem *reg, u8 shift, u8 width,
 		u8 clk_divider_flags, const struct clk_div_table *table,

@@ -572,6 +575,7 @@ EXPORT_SYMBOL_GPL(clk_register_divider_table);
  * @lock: shared register lock for this clock
  */
 struct clk_hw *clk_hw_register_divider_table(struct device *dev,
+		struct clk_ctrl *ctrl,
 		const char *name, const char *parent_name, unsigned long flags,
 		void __iomem *reg, u8 shift, u8 width,
 		u8 clk_divider_flags, const struct clk_div_table *table,

diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c
index aab904618eb6..1b712a424fd4 100644
--- a/drivers/clk/clk-fractional-divider.c
+++ b/drivers/clk/clk-fractional-divider.c

@@ -117,6 +117,7 @@ const struct clk_ops clk_fractional_divider_ops = {
 EXPORT_SYMBOL_GPL(clk_fractional_divider_ops);
 
 struct clk_hw *clk_hw_register_fractional_divider(struct device *dev,
+		struct clk_ctrl *ctrl,
 		const char *name, const char *parent_name, unsigned long flags,
 		void __iomem *reg, u8 mshift, u8 mwidth, u8 nshift, u8 nwidth,
 		u8 clk_divider_flags, spinlock_t *lock)

@@ -158,14 +159,14 @@ struct clk_hw *clk_hw_register_fractional_divider(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(clk_hw_register_fractional_divider);
 
-struct clk *clk_register_fractional_divider(struct device *dev,
+struct clk *clk_register_fractional_divider(struct device *dev, struct clk_ctrl *ctrl,
 		const char *name, const char *parent_name, unsigned long flags,
 		void __iomem *reg, u8 mshift, u8 mwidth, u8 nshift, u8 nwidth,
 		u8 clk_divider_flags, spinlock_t *lock)
 {
 	struct clk_hw *hw;
 
-	hw = clk_hw_register_fractional_divider(dev, name, parent_name, flags,
+	hw = clk_hw_register_fractional_divider(dev, ctrl, name, parent_name, flags,
 			reg, mshift, mwidth, nshift, nwidth, clk_divider_flags,
 			lock);
 	if (IS_ERR(hw))

diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
index 7bfd895781c5..9a8068d4da77 100644
--- a/drivers/clk/samsung/clk.c
+++ b/drivers/clk/samsung/clk.c

@@ -230,14 +230,14 @@ void __init samsung_clk_register_div(struct samsung_clk_provider *ctx,
 
 	for (idx = 0; idx < nr_clk; idx++, list++) {
 		if (list->table)
-			clk = clk_register_divider_table(NULL, list->name,
-				list->parent_name, list->flags,
+			clk = clk_register_divider_table(NULL, ctx->clk_ctrl,
+				list->name, list->parent_name, list->flags,
 				ctx->reg_base + list->offset,
 				list->shift, list->width, list->div_flags,
 				list->table, &ctx->lock);
 		else
-			clk = clk_register_divider(NULL, list->name,
-				list->parent_name, list->flags,
+			clk = clk_register_divider(NULL, ctx->clk_ctrl,
+				list->name, list->parent_name, list->flags,
 				ctx->reg_base + list->offset, list->shift,
 				list->width, list->div_flags, &ctx->lock);
 		if (IS_ERR(clk)) {

diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 26171815948e..cfb3aa3912c5 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h

@@ -428,20 +428,23 @@ int divider_get_val(unsigned long rate, unsigned long parent_rate,
 		const struct clk_div_table *table, u8 width,
 		unsigned long flags);
 
-struct clk *clk_register_divider(struct device *dev, const char *name,
-		const char *parent_name, unsigned long flags,
+struct clk *clk_register_divider(struct device *dev, struct clk_ctrl *ctrl,
+		const char *name, const char *parent_name, unsigned long flags,
 		void __iomem *reg, u8 shift, u8 width,
 		u8 clk_divider_flags, spinlock_t *lock);
-struct clk_hw *clk_hw_register_divider(struct device *dev, const char *name,
+struct clk_hw *clk_hw_register_divider(struct device *dev, struct clk_ctrl *ctrl,
+		const char *name,
 		const char *parent_name, unsigned long flags,
 		void __iomem *reg, u8 shift, u8 width,
 		u8 clk_divider_flags, spinlock_t *lock);
-struct clk *clk_register_divider_table(struct device *dev, const char *name,
+struct clk *clk_register_divider_table(struct device *dev,
+		struct clk_ctrl *ctrl, const char *name,
 		const char *parent_name, unsigned long flags,
 		void __iomem *reg, u8 shift, u8 width,
 		u8 clk_divider_flags, const struct clk_div_table *table,
 		spinlock_t *lock);
 struct clk_hw *clk_hw_register_divider_table(struct device *dev,
+		struct clk_ctrl *ctrl,
 		const char *name, const char *parent_name, unsigned long flags,
 		void __iomem *reg, u8 shift, u8 width,
 		u8 clk_divider_flags, const struct clk_div_table *table,

@@ -586,10 +589,12 @@ struct clk_fractional_divider {
 
 extern const struct clk_ops clk_fractional_divider_ops;
 struct clk *clk_register_fractional_divider(struct device *dev,
+		struct clk_ctrl *ctrl,
 		const char *name, const char *parent_name, unsigned long flags,
 		void __iomem *reg, u8 mshift, u8 mwidth, u8 nshift, u8 nwidth,
 		u8 clk_divider_flags, spinlock_t *lock);
 struct clk_hw *clk_hw_register_fractional_divider(struct device *dev,
+		struct clk_ctrl *ctrl,
 		const char *name, const char *parent_name, unsigned long flags,
 		void __iomem *reg, u8 mshift, u8 mwidth, u8 nshift, u8 nwidth,
 		u8 clk_divider_flags, spinlock_t *lock);

-- 
1.9.1

[RFC 13/17] clk: samsung: audss: samsung: Switch to new clock controller API

From: Krzysztof Kozlowski <hidden>
Date: 2016-08-16 13:38:23

Allocate a clock controller and use new clk_register_with_ctrl() API.

Signed-off-by: Krzysztof Kozlowski <redacted>
---
 drivers/clk/samsung/clk-exynos-audss.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/clk/samsung/clk-exynos-audss.c b/drivers/clk/samsung/clk-exynos-audss.c
index bdf8b971f332..c13e6ab6f3b6 100644
--- a/drivers/clk/samsung/clk-exynos-audss.c
+++ b/drivers/clk/samsung/clk-exynos-audss.c

@@ -30,6 +30,7 @@ static DEFINE_SPINLOCK(lock);
 static struct clk **clk_table;
 static void __iomem *reg_base;
 static struct clk_onecell_data clk_data;
+static struct clk_ctrl *clk_ctrl;
 /*
  * On Exynos5420 this will be a clock which has to be enabled before any
  * access to audss registers. Typically a child of EPLL.

@@ -135,6 +136,10 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
 	if (!clk_table)
 		return -ENOMEM;
 
+	clk_ctrl = clk_ctrl_register(&pdev->dev); /* FIXME: Use devm-like, leaks now on error path */
+	if (IS_ERR(clk_ctrl))
+		return PTR_ERR(clk_ctrl);
+
 	clk_data.clks = clk_table;
 	if (variant == TYPE_EXYNOS5420)
 		clk_data.clk_num = EXYNOS_AUDSS_MAX_CLKS;

@@ -159,7 +164,7 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
 			}
 		}
 	}
-	clk_table[EXYNOS_MOUT_AUDSS] = clk_register_mux(NULL, "mout_audss",
+	clk_table[EXYNOS_MOUT_AUDSS] = clk_register_mux(NULL, clk_ctrl, "mout_audss",
 				mout_audss_p, ARRAY_SIZE(mout_audss_p),
 				CLK_SET_RATE_NO_REPARENT,
 				reg_base + ASS_CLK_SRC, 0, 1, 0, &lock);

@@ -170,48 +175,48 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
 		mout_i2s_p[1] = __clk_get_name(cdclk);
 	if (!IS_ERR(sclk_audio))
 		mout_i2s_p[2] = __clk_get_name(sclk_audio);
-	clk_table[EXYNOS_MOUT_I2S] = clk_register_mux(NULL, "mout_i2s",
+	clk_table[EXYNOS_MOUT_I2S] = clk_register_mux(NULL, clk_ctrl, "mout_i2s",
 				mout_i2s_p, ARRAY_SIZE(mout_i2s_p),
 				CLK_SET_RATE_NO_REPARENT,
 				reg_base + ASS_CLK_SRC, 2, 2, 0, &lock);
 
-	clk_table[EXYNOS_DOUT_SRP] = clk_register_divider(NULL, "dout_srp",
+	clk_table[EXYNOS_DOUT_SRP] = clk_register_divider(NULL, clk_ctrl, "dout_srp",
 				"mout_audss", 0, reg_base + ASS_CLK_DIV, 0, 4,
 				0, &lock);
 
-	clk_table[EXYNOS_DOUT_AUD_BUS] = clk_register_divider(NULL,
+	clk_table[EXYNOS_DOUT_AUD_BUS] = clk_register_divider(NULL, clk_ctrl,
 				"dout_aud_bus", "dout_srp", 0,
 				reg_base + ASS_CLK_DIV, 4, 4, 0, &lock);
 
-	clk_table[EXYNOS_DOUT_I2S] = clk_register_divider(NULL, "dout_i2s",
+	clk_table[EXYNOS_DOUT_I2S] = clk_register_divider(NULL, clk_ctrl, "dout_i2s",
 				"mout_i2s", 0, reg_base + ASS_CLK_DIV, 8, 4, 0,
 				&lock);
 
-	clk_table[EXYNOS_SRP_CLK] = clk_register_gate(NULL, "srp_clk",
+	clk_table[EXYNOS_SRP_CLK] = clk_register_gate(NULL, clk_ctrl, "srp_clk",
 				"dout_srp", CLK_SET_RATE_PARENT,
 				reg_base + ASS_CLK_GATE, 0, 0, &lock);
 
-	clk_table[EXYNOS_I2S_BUS] = clk_register_gate(NULL, "i2s_bus",
+	clk_table[EXYNOS_I2S_BUS] = clk_register_gate(NULL, clk_ctrl, "i2s_bus",
 				"dout_aud_bus", CLK_SET_RATE_PARENT,
 				reg_base + ASS_CLK_GATE, 2, 0, &lock);
 
-	clk_table[EXYNOS_SCLK_I2S] = clk_register_gate(NULL, "sclk_i2s",
+	clk_table[EXYNOS_SCLK_I2S] = clk_register_gate(NULL, clk_ctrl, "sclk_i2s",
 				"dout_i2s", CLK_SET_RATE_PARENT,
 				reg_base + ASS_CLK_GATE, 3, 0, &lock);
 
-	clk_table[EXYNOS_PCM_BUS] = clk_register_gate(NULL, "pcm_bus",
+	clk_table[EXYNOS_PCM_BUS] = clk_register_gate(NULL, clk_ctrl, "pcm_bus",
 				 "sclk_pcm", CLK_SET_RATE_PARENT,
 				reg_base + ASS_CLK_GATE, 4, 0, &lock);
 
 	sclk_pcm_in = devm_clk_get(&pdev->dev, "sclk_pcm_in");
 	if (!IS_ERR(sclk_pcm_in))
 		sclk_pcm_p = __clk_get_name(sclk_pcm_in);
-	clk_table[EXYNOS_SCLK_PCM] = clk_register_gate(NULL, "sclk_pcm",
+	clk_table[EXYNOS_SCLK_PCM] = clk_register_gate(NULL, clk_ctrl, "sclk_pcm",
 				sclk_pcm_p, CLK_SET_RATE_PARENT,
 				reg_base + ASS_CLK_GATE, 5, 0, &lock);
 
 	if (variant == TYPE_EXYNOS5420) {
-		clk_table[EXYNOS_ADMA] = clk_register_gate(NULL, "adma",
+		clk_table[EXYNOS_ADMA] = clk_register_gate(NULL, clk_ctrl, "adma",
 				"dout_srp", CLK_SET_RATE_PARENT,
 				reg_base + ASS_CLK_GATE, 9, 0, &lock);
 	}

@@ -261,6 +266,9 @@ static int exynos_audss_clk_remove(struct platform_device *pdev)
 	if (!IS_ERR(epll))
 		clk_disable_unprepare(epll);
 
+	clk_ctrl_unregister(clk_ctrl);
+	clk_ctrl = NULL;
+
 	return 0;
 }

-- 
1.9.1

[RFC 12/17] ASoC: samsung: Switch to new clock controller API

From: Krzysztof Kozlowski <hidden>
Date: 2016-08-16 13:38:52

Allocate a clock controller and use new clk_register_with_ctrl() API.

Signed-off-by: Krzysztof Kozlowski <redacted>
---
 sound/soc/samsung/i2s.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/sound/soc/samsung/i2s.c b/sound/soc/samsung/i2s.c
index fa3ff03d97d5..1ec90daa4df4 100644
--- a/sound/soc/samsung/i2s.c
+++ b/sound/soc/samsung/i2s.c

@@ -104,6 +104,7 @@ struct i2s_dai {
 	/* Below fields are only valid if this is the primary FIFO */
 	struct clk *clk_table[3];
 	struct clk_onecell_data clk_data;
+	struct clk_ctrl *clk_ctrl;
 };
 
 /* Lock for cross i/f checks */

@@ -1148,6 +1149,7 @@ static void i2s_unregister_clock_provider(struct platform_device *pdev)
 
 	of_clk_del_provider(pdev->dev.of_node);
 	i2s_unregister_clocks(i2s);
+	clk_ctrl_unregister(i2s->clk_ctrl);
 }
 
 static int i2s_register_clock_provider(struct platform_device *pdev)

@@ -1164,6 +1166,10 @@ static int i2s_register_clock_provider(struct platform_device *pdev)
 	if (!of_find_property(dev->of_node, "#clock-cells", NULL))
 		return 0;
 
+	i2s->clk_ctrl = clk_ctrl_register(dev);
+	if (IS_ERR(i2s->clk_ctrl))
+		return PTR_ERR(i2s->clk_ctrl);
+
 	/* Get the RCLKSRC mux clock parent clock names */
 	for (i = 0; i < ARRAY_SIZE(p_names); i++) {
 		rclksrc = clk_get(dev, clk_name[i]);

@@ -1179,13 +1185,14 @@ static int i2s_register_clock_provider(struct platform_device *pdev)
 		writel(val | PSR_PSREN, i2s->addr + I2SPSR);
 
 		i2s->clk_table[CLK_I2S_RCLK_SRC] = clk_register_mux(NULL,
+				i2s->clk_ctrl,
 				"i2s_rclksrc", p_names, ARRAY_SIZE(p_names),
 				CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT,
 				i2s->addr + I2SMOD, reg_info->rclksrc_off,
 				1, 0, i2s->lock);
 
 		i2s->clk_table[CLK_I2S_RCLK_PSR] = clk_register_divider(NULL,
-				"i2s_presc", "i2s_rclksrc",
+				i2s->clk_ctrl, "i2s_presc", "i2s_rclksrc",
 				CLK_SET_RATE_PARENT,
 				i2s->addr + I2SPSR, 8, 6, 0, i2s->lock);

@@ -1195,8 +1202,8 @@ static int i2s_register_clock_provider(struct platform_device *pdev)
 	of_property_read_string_index(dev->of_node,
 				"clock-output-names", 0, &clk_name[0]);
 
-	i2s->clk_table[CLK_I2S_CDCLK] = clk_register_gate(NULL, clk_name[0],
-				p_names[0], CLK_SET_RATE_PARENT,
+	i2s->clk_table[CLK_I2S_CDCLK] = clk_register_gate(NULL, i2s->clk_ctrl,
+				clk_name[0], p_names[0], CLK_SET_RATE_PARENT,
 				i2s->addr + I2SMOD, reg_info->cdclkcon_off,
 				CLK_GATE_SET_TO_DISABLE, i2s->lock);

-- 
1.9.1

Re: [RFC 12/17] ASoC: samsung: Switch to new clock controller API

From: Mark Brown <broonie@kernel.org>
Date: 2016-08-16 14:14:26

On Tue, Aug 16, 2016 at 03:35:09PM +0200, Krzysztof Kozlowski wrote:

Allocate a clock controller and use new clk_register_with_ctrl() API.

Acked-by: Mark Brown <broonie@kernel.org>

 	i2s_unregister_clocks(i2s);
+	clk_ctrl_unregister(i2s->clk_ctrl);

devm?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160816/31281df2/attachment.sig>

Re: [RFC 12/17] ASoC: samsung: Switch to new clock controller API

From: Krzysztof Kozlowski <hidden>
Date: 2016-08-16 16:41:49

On Tue, Aug 16, 2016 at 03:13:45PM +0100, Mark Brown wrote:

On Tue, Aug 16, 2016 at 03:35:09PM +0200, Krzysztof Kozlowski wrote:

quoted

Allocate a clock controller and use new clk_register_with_ctrl() API.

Acked-by: Mark Brown <broonie@kernel.org>

quoted

 	i2s_unregister_clocks(i2s);
+	clk_ctrl_unregister(i2s->clk_ctrl);

devm?

Right, that would make sense. Thanks for pointing this out.

Best regards,
Krzysztof

[RFC 11/17] clk: gpio: Switch to new clock controller API

From: Krzysztof Kozlowski <hidden>
Date: 2016-08-16 13:39:18

Allocate a clock controller and use new clk_register_with_ctrl() API.

Signed-off-by: Krzysztof Kozlowski <redacted>
---
 drivers/clk/clk-gpio.c       | 29 +++++++++++++++++------------
 include/linux/clk-provider.h | 12 ++++++++----
 2 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
index 86b245746a6b..8ebd520c1aa0 100644
--- a/drivers/clk/clk-gpio.c
+++ b/drivers/clk/clk-gpio.c

@@ -94,7 +94,8 @@ const struct clk_ops clk_gpio_mux_ops = {
 };
 EXPORT_SYMBOL_GPL(clk_gpio_mux_ops);
 
-static struct clk_hw *clk_register_gpio(struct device *dev, const char *name,
+static struct clk_hw *clk_register_gpio(struct device *dev, struct clk_ctrl *ctrl,
+		const char *name,
 		const char * const *parent_names, u8 num_parents, unsigned gpio,
 		bool active_low, unsigned long flags,
 		const struct clk_ops *clk_gpio_ops)

@@ -168,25 +169,27 @@ static struct clk_hw *clk_register_gpio(struct device *dev, const char *name,
  * @active_low: true if gpio should be set to 0 to enable clock
  * @flags: clock flags
  */
-struct clk_hw *clk_hw_register_gpio_gate(struct device *dev, const char *name,
+struct clk_hw *clk_hw_register_gpio_gate(struct device *dev, struct clk_ctrl *ctrl,
+		const char *name,
 		const char *parent_name, unsigned gpio, bool active_low,
 		unsigned long flags)
 {
-	return clk_register_gpio(dev, name,
+	return clk_register_gpio(dev, ctrl, name,
 			(parent_name ? &parent_name : NULL),
 			(parent_name ? 1 : 0), gpio, active_low, flags,
 			&clk_gpio_gate_ops);
 }
 EXPORT_SYMBOL_GPL(clk_hw_register_gpio_gate);
 
-struct clk *clk_register_gpio_gate(struct device *dev, const char *name,
+struct clk *clk_register_gpio_gate(struct device *dev, struct clk_ctrl *ctrl,
+		const char *name,
 		const char *parent_name, unsigned gpio, bool active_low,
 		unsigned long flags)
 {
 	struct clk_hw *hw;
 
-	hw = clk_hw_register_gpio_gate(dev, name, parent_name, gpio, active_low,
-				       flags);
+	hw = clk_hw_register_gpio_gate(dev, ctrl, name, parent_name, gpio,
+				       active_low, flags);
 	if (IS_ERR(hw))
 		return ERR_CAST(hw);
 	return hw->clk;

@@ -203,7 +206,8 @@ EXPORT_SYMBOL_GPL(clk_register_gpio_gate);
  * @active_low: true if gpio should be set to 0 to enable clock
  * @flags: clock flags
  */
-struct clk_hw *clk_hw_register_gpio_mux(struct device *dev, const char *name,
+struct clk_hw *clk_hw_register_gpio_mux(struct device *dev, struct clk_ctrl *ctrl,
+		const char *name,
 		const char * const *parent_names, u8 num_parents, unsigned gpio,
 		bool active_low, unsigned long flags)
 {

@@ -212,18 +216,19 @@ struct clk_hw *clk_hw_register_gpio_mux(struct device *dev, const char *name,
 		return ERR_PTR(-EINVAL);
 	}
 
-	return clk_register_gpio(dev, name, parent_names, num_parents,
+	return clk_register_gpio(dev, ctrl, name, parent_names, num_parents,
 			gpio, active_low, flags, &clk_gpio_mux_ops);
 }
 EXPORT_SYMBOL_GPL(clk_hw_register_gpio_mux);
 
-struct clk *clk_register_gpio_mux(struct device *dev, const char *name,
+struct clk *clk_register_gpio_mux(struct device *dev, struct clk_ctrl *ctrl,
+		const char *name,
 		const char * const *parent_names, u8 num_parents, unsigned gpio,
 		bool active_low, unsigned long flags)
 {
 	struct clk_hw *hw;
 
-	hw = clk_hw_register_gpio_mux(dev, name, parent_names, num_parents,
+	hw = clk_hw_register_gpio_mux(dev, ctrl, name, parent_names, num_parents,
 			gpio, active_low, flags);
 	if (IS_ERR(hw))
 		return ERR_CAST(hw);

@@ -271,10 +276,10 @@ static int gpio_clk_driver_probe(struct platform_device *pdev)
 	active_low = of_flags & OF_GPIO_ACTIVE_LOW;
 
 	if (is_mux)
-		clk = clk_register_gpio_mux(&pdev->dev, node->name,
+		clk = clk_register_gpio_mux(&pdev->dev, NULL, node->name,
 				parent_names, num_parents, gpio, active_low, 0);
 	else
-		clk = clk_register_gpio_gate(&pdev->dev, node->name,
+		clk = clk_register_gpio_gate(&pdev->dev, NULL, node->name,
 				parent_names ?  parent_names[0] : NULL, gpio,
 				active_low, 0);
 	if (IS_ERR(clk))

diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 2bd0a8cb7a9c..c39760cba9ac 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h

@@ -698,10 +698,12 @@ struct clk_gpio {
 #define to_clk_gpio(_hw) container_of(_hw, struct clk_gpio, hw)
 
 extern const struct clk_ops clk_gpio_gate_ops;
-struct clk *clk_register_gpio_gate(struct device *dev, const char *name,
+struct clk *clk_register_gpio_gate(struct device *dev, struct clk_ctrl *ctrl,
+		const char *name,
 		const char *parent_name, unsigned gpio, bool active_low,
 		unsigned long flags);
-struct clk_hw *clk_hw_register_gpio_gate(struct device *dev, const char *name,
+struct clk_hw *clk_hw_register_gpio_gate(struct device *dev, struct clk_ctrl *ctrl,
+		const char *name,
 		const char *parent_name, unsigned gpio, bool active_low,
 		unsigned long flags);
 void clk_hw_unregister_gpio_gate(struct clk_hw *hw);

@@ -717,10 +719,12 @@ void clk_hw_unregister_gpio_gate(struct clk_hw *hw);
  */
 
 extern const struct clk_ops clk_gpio_mux_ops;
-struct clk *clk_register_gpio_mux(struct device *dev, const char *name,
+struct clk *clk_register_gpio_mux(struct device *dev, struct clk_ctrl *ctrl,
+		const char *name,
 		const char * const *parent_names, u8 num_parents, unsigned gpio,
 		bool active_low, unsigned long flags);
-struct clk_hw *clk_hw_register_gpio_mux(struct device *dev, const char *name,
+struct clk_hw *clk_hw_register_gpio_mux(struct device *dev, struct clk_ctrl *ctrl,
+		const char *name,
 		const char * const *parent_names, u8 num_parents, unsigned gpio,
 		bool active_low, unsigned long flags);
 void clk_hw_unregister_gpio_mux(struct clk_hw *hw);

-- 
1.9.1

[RFC 07/17] clk: mux: Switch to new clock controller API

From: Krzysztof Kozlowski <hidden>
Date: 2016-08-16 13:40:05

Allocate a clock controller and use new clk_register_with_ctrl() API.

Signed-off-by: Krzysztof Kozlowski <redacted>
---
 drivers/clk/clk-mux.c        | 32 ++++++++++++++++++--------------
 drivers/clk/samsung/clk.c    |  4 ++--
 include/linux/clk-provider.h | 12 ++++++++----
 3 files changed, 28 insertions(+), 20 deletions(-)

diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c
index 16a3d5717f4e..a85bbef1992b 100644
--- a/drivers/clk/clk-mux.c
+++ b/drivers/clk/clk-mux.c

@@ -113,7 +113,8 @@ const struct clk_ops clk_mux_ro_ops = {
 };
 EXPORT_SYMBOL_GPL(clk_mux_ro_ops);
 
-struct clk_hw *clk_hw_register_mux_table(struct device *dev, const char *name,
+struct clk_hw *clk_hw_register_mux_table(struct device *dev,
+		struct clk_ctrl *ctrl, const char *name,
 		const char * const *parent_names, u8 num_parents,
 		unsigned long flags,
 		void __iomem *reg, u8 shift, u32 mask,

@@ -159,7 +160,7 @@ struct clk_hw *clk_hw_register_mux_table(struct device *dev, const char *name,
 	mux->hw.init = &init;
 
 	hw = &mux->hw;
-	ret = clk_hw_register(dev, hw);
+	ret = clk_hw_register_with_ctrl(dev, ctrl, hw);
 	if (ret) {
 		kfree(mux);
 		hw = ERR_PTR(ret);

@@ -169,7 +170,8 @@ struct clk_hw *clk_hw_register_mux_table(struct device *dev, const char *name,
 }
 EXPORT_SYMBOL_GPL(clk_hw_register_mux_table);
 
-struct clk *clk_register_mux_table(struct device *dev, const char *name,
+struct clk *clk_register_mux_table(struct device *dev, struct clk_ctrl *ctrl,
+		const char *name,
 		const char * const *parent_names, u8 num_parents,
 		unsigned long flags,
 		void __iomem *reg, u8 shift, u32 mask,

@@ -177,16 +179,17 @@ struct clk *clk_register_mux_table(struct device *dev, const char *name,
 {
 	struct clk_hw *hw;
 
-	hw = clk_hw_register_mux_table(dev, name, parent_names, num_parents,
-				       flags, reg, shift, mask, clk_mux_flags,
-				       table, lock);
+	hw = clk_hw_register_mux_table(dev, ctrl, name, parent_names,
+				       num_parents, flags, reg, shift, mask,
+				       clk_mux_flags, table, lock);
 	if (IS_ERR(hw))
 		return ERR_CAST(hw);
 	return hw->clk;
 }
 EXPORT_SYMBOL_GPL(clk_register_mux_table);
 
-struct clk *clk_register_mux(struct device *dev, const char *name,
+struct clk *clk_register_mux(struct device *dev, struct clk_ctrl *ctrl,
+		const char *name,
 		const char * const *parent_names, u8 num_parents,
 		unsigned long flags,
 		void __iomem *reg, u8 shift, u8 width,

@@ -194,13 +197,14 @@ struct clk *clk_register_mux(struct device *dev, const char *name,
 {
 	u32 mask = BIT(width) - 1;
 
-	return clk_register_mux_table(dev, name, parent_names, num_parents,
-				      flags, reg, shift, mask, clk_mux_flags,
-				      NULL, lock);
+	return clk_register_mux_table(dev, ctrl, name, parent_names,
+				      num_parents, flags, reg, shift, mask,
+				      clk_mux_flags, NULL, lock);
 }
 EXPORT_SYMBOL_GPL(clk_register_mux);
 
-struct clk_hw *clk_hw_register_mux(struct device *dev, const char *name,
+struct clk_hw *clk_hw_register_mux(struct device *dev, struct clk_ctrl *ctrl,
+		const char *name,
 		const char * const *parent_names, u8 num_parents,
 		unsigned long flags,
 		void __iomem *reg, u8 shift, u8 width,

@@ -208,9 +212,9 @@ struct clk_hw *clk_hw_register_mux(struct device *dev, const char *name,
 {
 	u32 mask = BIT(width) - 1;
 
-	return clk_hw_register_mux_table(dev, name, parent_names, num_parents,
-				      flags, reg, shift, mask, clk_mux_flags,
-				      NULL, lock);
+	return clk_hw_register_mux_table(dev, ctrl, name, parent_names,
+					 num_parents, flags, reg, shift, mask,
+					 clk_mux_flags, NULL, lock);
 }
 EXPORT_SYMBOL_GPL(clk_hw_register_mux);

diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
index 79332011f258..1a296bbabd47 100644
--- a/drivers/clk/samsung/clk.c
+++ b/drivers/clk/samsung/clk.c

@@ -197,8 +197,8 @@ void __init samsung_clk_register_mux(struct samsung_clk_provider *ctx,
 	unsigned int idx, ret;
 
 	for (idx = 0; idx < nr_clk; idx++, list++) {
-		clk = clk_register_mux(NULL, list->name, list->parent_names,
-			list->num_parents, list->flags,
+		clk = clk_register_mux(NULL, ctx->clk_ctrl, list->name,
+			list->parent_names, list->num_parents, list->flags,
 			ctx->reg_base + list->offset,
 			list->shift, list->width, list->mux_flags, &ctx->lock);
 		if (IS_ERR(clk)) {

diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 289290655283..f5f062d267a9 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h

@@ -493,23 +493,27 @@ struct clk_mux {
 extern const struct clk_ops clk_mux_ops;
 extern const struct clk_ops clk_mux_ro_ops;
 
-struct clk *clk_register_mux(struct device *dev, const char *name,
+struct clk *clk_register_mux(struct device *dev, struct clk_ctrl *ctrl,
+		const char *name,
 		const char * const *parent_names, u8 num_parents,
 		unsigned long flags,
 		void __iomem *reg, u8 shift, u8 width,
 		u8 clk_mux_flags, spinlock_t *lock);
-struct clk_hw *clk_hw_register_mux(struct device *dev, const char *name,
+struct clk_hw *clk_hw_register_mux(struct device *dev, struct clk_ctrl *ctrl,
+		const char *name,
 		const char * const *parent_names, u8 num_parents,
 		unsigned long flags,
 		void __iomem *reg, u8 shift, u8 width,
 		u8 clk_mux_flags, spinlock_t *lock);
 
-struct clk *clk_register_mux_table(struct device *dev, const char *name,
+struct clk *clk_register_mux_table(struct device *dev, struct clk_ctrl *ctrl,
+		const char *name,
 		const char * const *parent_names, u8 num_parents,
 		unsigned long flags,
 		void __iomem *reg, u8 shift, u32 mask,
 		u8 clk_mux_flags, u32 *table, spinlock_t *lock);
-struct clk_hw *clk_hw_register_mux_table(struct device *dev, const char *name,
+struct clk_hw *clk_hw_register_mux_table(struct device *dev,
+		struct clk_ctrl *ctrl, const char *name,
 		const char * const *parent_names, u8 num_parents,
 		unsigned long flags,
 		void __iomem *reg, u8 shift, u32 mask,

-- 
1.9.1

[RFC 08/17] clk: fixed-factor: Switch to new clock controller API

From: Krzysztof Kozlowski <hidden>
Date: 2016-08-16 13:40:07

Allocate a clock controller and use new clk_register_with_ctrl() API.

Signed-off-by: Krzysztof Kozlowski <redacted>
---
 drivers/clk/clk-fixed-factor.c | 11 +++++++----
 drivers/clk/samsung/clk.c      |  2 +-
 include/linux/clk-provider.h   |  4 +++-
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c
index 4db3be214077..921a99b6e421 100644
--- a/drivers/clk/clk-fixed-factor.c
+++ b/drivers/clk/clk-fixed-factor.c

@@ -69,6 +69,7 @@ const struct clk_ops clk_fixed_factor_ops = {
 EXPORT_SYMBOL_GPL(clk_fixed_factor_ops);
 
 struct clk_hw *clk_hw_register_fixed_factor(struct device *dev,
+		struct clk_ctrl *ctrl,
 		const char *name, const char *parent_name, unsigned long flags,
 		unsigned int mult, unsigned int div)
 {

@@ -103,13 +104,14 @@ struct clk_hw *clk_hw_register_fixed_factor(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(clk_hw_register_fixed_factor);
 
-struct clk *clk_register_fixed_factor(struct device *dev, const char *name,
+struct clk *clk_register_fixed_factor(struct device *dev,
+		struct clk_ctrl *ctrl, const char *name,
 		const char *parent_name, unsigned long flags,
 		unsigned int mult, unsigned int div)
 {
 	struct clk_hw *hw;
 
-	hw = clk_hw_register_fixed_factor(dev, name, parent_name, flags, mult,
+	hw = clk_hw_register_fixed_factor(dev, ctrl, name, parent_name, flags, mult,
 					  div);
 	if (IS_ERR(hw))
 		return ERR_CAST(hw);

@@ -176,8 +178,9 @@ void __init of_fixed_factor_clk_setup(struct device_node *node)
 	if (of_match_node(set_rate_parent_matches, node))
 		flags |= CLK_SET_RATE_PARENT;
 
-	clk = clk_register_fixed_factor(NULL, clk_name, parent_name, flags,
-					mult, div);
+	/* TODO: convert to clk_ctrl */
+	clk = clk_register_fixed_factor(NULL, NULL, clk_name, parent_name,
+					flags, mult, div);
 	if (!IS_ERR(clk))
 		of_clk_add_provider(node, of_clk_src_simple_get, clk);
 }

diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
index 1a296bbabd47..7bfd895781c5 100644
--- a/drivers/clk/samsung/clk.c
+++ b/drivers/clk/samsung/clk.c

@@ -176,7 +176,7 @@ void __init samsung_clk_register_fixed_factor(struct samsung_clk_provider *ctx,
 	unsigned int idx;
 
 	for (idx = 0; idx < nr_clk; idx++, list++) {
-		clk = clk_register_fixed_factor(NULL, list->name,
+		clk = clk_register_fixed_factor(NULL, ctx->clk_ctrl, list->name,
 			list->parent_name, list->flags, list->mult, list->div);
 		if (IS_ERR(clk)) {
 			pr_err("%s: failed to register clock %s\n", __func__,

diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index f5f062d267a9..26171815948e 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h

@@ -545,11 +545,13 @@ struct clk_fixed_factor {
 #define to_clk_fixed_factor(_hw) container_of(_hw, struct clk_fixed_factor, hw)
 
 extern const struct clk_ops clk_fixed_factor_ops;
-struct clk *clk_register_fixed_factor(struct device *dev, const char *name,
+struct clk *clk_register_fixed_factor(struct device *dev,
+		struct clk_ctrl *ctrl, const char *name,
 		const char *parent_name, unsigned long flags,
 		unsigned int mult, unsigned int div);
 void clk_unregister_fixed_factor(struct clk *clk);
 struct clk_hw *clk_hw_register_fixed_factor(struct device *dev,
+		struct clk_ctrl *ctrl,
 		const char *name, const char *parent_name, unsigned long flags,
 		unsigned int mult, unsigned int div);
 void clk_hw_unregister_fixed_factor(struct clk_hw *hw);

-- 
1.9.1

[RFC 03/17] clk: s2mps11: Switch to new clock controller API

From: Krzysztof Kozlowski <hidden>
Date: 2016-08-16 13:40:59

Allocate a clock controller and use new clk_register_with_ctrl() API.

Signed-off-by: Krzysztof Kozlowski <redacted>
---
 drivers/clk/clk-s2mps11.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk-s2mps11.c b/drivers/clk/clk-s2mps11.c
index fbaa84a33c46..881f1e226867 100644
--- a/drivers/clk/clk-s2mps11.c
+++ b/drivers/clk/clk-s2mps11.c

@@ -41,6 +41,7 @@ struct s2mps11_clk {
 	struct clk_hw hw;
 	struct clk *clk;
 	struct clk_lookup *lookup;
+	struct clk_ctrl *clk_ctrl;
 	u32 mask;
 	unsigned int reg;
 };

@@ -176,6 +177,10 @@ static int s2mps11_clk_probe(struct platform_device *pdev)
 	if (IS_ERR(s2mps11_clks->clk_np))
 		return PTR_ERR(s2mps11_clks->clk_np);
 
+	s2mps11_clks->clk_ctrl = clk_ctrl_register(&pdev->dev);
+	if (IS_ERR(s2mps11_clks->clk_ctrl))
+		return PTR_ERR(s2mps11_clks->clk_ctrl); // FIXME: use devm-like
+
 	for (i = 0; i < S2MPS11_CLKS_NUM; i++) {
 		if (i == S2MPS11_CLK_CP && hwid == S2MPS14X)
 			continue; /* Skip clocks not present in some devices */

@@ -184,7 +189,8 @@ static int s2mps11_clk_probe(struct platform_device *pdev)
 		s2mps11_clks[i].mask = 1 << i;
 		s2mps11_clks[i].reg = s2mps11_reg;
 
-		s2mps11_clks[i].clk = devm_clk_register(&pdev->dev,
+		s2mps11_clks[i].clk = devm_clk_register_with_ctrl(&pdev->dev,
+							s2mps11_clks->clk_ctrl,
 							&s2mps11_clks[i].hw);
 		if (IS_ERR(s2mps11_clks[i].clk)) {
 			dev_err(&pdev->dev, "Fail to register : %s\n",

@@ -233,6 +239,8 @@ static int s2mps11_clk_remove(struct platform_device *pdev)
 		clkdev_drop(s2mps11_clks[i].lookup);
 	}
 
+	clk_ctrl_unregister(s2mps11_clks->clk_ctrl);
+
 	return 0;
 }

-- 
1.9.1

[RFC 01/17] clk: bcm2835: Rename clk_register to avoid name conflict

From: Krzysztof Kozlowski <hidden>
Date: 2016-08-16 13:41:17

During clk_register_*() API refactoring, macros will be used for hiding
and narrowing the scope of changes thus leading to a name conflict with
clk_register found in bcm2835 clk driver.

Signed-off-by: Krzysztof Kozlowski <redacted>
---
 drivers/clk/bcm/clk-bcm2835.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
index 7a7970865c2d..b77a8efbfce7 100644
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c

@@ -1262,12 +1262,12 @@ static struct clk *bcm2835_register_gate(struct bcm2835_cprman *cprman,
 typedef struct clk *(*bcm2835_clk_register)(struct bcm2835_cprman *cprman,
 					    const void *data);
 struct bcm2835_clk_desc {
-	bcm2835_clk_register clk_register;
+	bcm2835_clk_register bcm2835_clk_register;
 	const void *data;
 };
 
 /* assignment helper macros for different clock types */
-#define _REGISTER(f, ...) { .clk_register = (bcm2835_clk_register)f, \
+#define _REGISTER(f, ...) { .bcm2835_clk_register = (bcm2835_clk_register)f, \
 			    .data = __VA_ARGS__ }
 #define REGISTER_PLL(...)	_REGISTER(&bcm2835_register_pll,	\
 					  &(struct bcm2835_pll_data)	\

@@ -1825,8 +1825,8 @@ static int bcm2835_clk_probe(struct platform_device *pdev)
 
 	for (i = 0; i < asize; i++) {
 		desc = &clk_desc_array[i];
-		if (desc->clk_register && desc->data)
-			clks[i] = desc->clk_register(cprman, desc->data);
+		if (desc->bcm2835_clk_register && desc->data)
+			clks[i] = desc->bcm2835_clk_register(cprman, desc->data);
 	}
 
 	return of_clk_add_provider(dev->of_node, of_clk_src_onecell_get,

-- 
1.9.1

[RFC 02/17] clk: Add clock controller to fine-grain the prepare lock

From: Krzysztof Kozlowski <hidden>
Date: 2016-08-16 13:41:33

Add a new entity - clock controller - so the global clock prepare lock
could be fine-grained per controller.  The controller is an abstract way
of representing a hardware block.  It overlaps a little with clock
provider so there is a potential of merging them.

The clock hierarchy might span between many controllers so add necessary
locking primitives for locking children, parents or everything.

Add a global controller for drivers not converted to new API.  This will
be removed once everything uses per-device/per-driver clock controller.

Signed-off-by: Krzysztof Kozlowski <redacted>
---
 drivers/clk/clk.c            | 300 +++++++++++++++++++++++++++++++++++++++++--
 include/linux/clk-provider.h |  25 +++-
 include/linux/clk.h          |   1 +
 3 files changed, 310 insertions(+), 16 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 238b989bf778..ee1cedfbaa29 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c

@@ -35,6 +35,7 @@ static struct task_struct *enable_owner;
 static int prepare_refcnt;
 static int enable_refcnt;
 
+static LIST_HEAD(clk_ctrl_list);
 static HLIST_HEAD(clk_root_list);
 static HLIST_HEAD(clk_orphan_list);
 static LIST_HEAD(clk_notifier_list);

@@ -46,6 +47,7 @@ struct clk_core {
 	const struct clk_ops	*ops;
 	struct clk_hw		*hw;
 	struct module		*owner;
+	struct clk_ctrl		*ctrl;
 	struct clk_core		*parent;
 	const char		**parent_names;
 	struct clk_core		**parents;

@@ -87,6 +89,24 @@ struct clk {
 	struct hlist_node clks_node;
 };
 
+struct clk_ctrl {
+	struct device *dev; /* Needed? */
+	struct mutex prepare_lock;
+	struct task_struct *prepare_owner;
+	int prepare_refcnt;
+	struct list_head node;
+};
+
+/*
+ * As a temporary solution, register all clocks which pass NULL as clock
+ * controller under this one.  This should be removed after converting
+ * all users to new clock controller aware API.
+ */
+static struct clk_ctrl global_ctrl = {
+	.prepare_lock = __MUTEX_INITIALIZER(global_ctrl.prepare_lock),
+	.node = LIST_HEAD_INIT(global_ctrl.node),
+};
+
 /***           locking             ***/
 static void clk_prepare_lock(void)
 {

@@ -148,6 +168,228 @@ static void clk_enable_unlock(unsigned long flags)
 	spin_unlock_irqrestore(&enable_lock, flags);
 }
 
+static void clk_ctrl_prepare_lock(struct clk_ctrl *ctrl)
+{
+	if (!ctrl)
+		return;
+
+	if (!mutex_trylock(&ctrl->prepare_lock)) {
+		if (ctrl->prepare_owner == current) {
+			ctrl->prepare_refcnt++;
+			return;
+		}
+		mutex_lock(&ctrl->prepare_lock);
+	}
+	WARN_ON_ONCE(ctrl->prepare_owner != NULL);
+	WARN_ON_ONCE(ctrl->prepare_refcnt != 0);
+	ctrl->prepare_owner = current;
+	ctrl->prepare_refcnt = 1;
+}
+
+static void clk_ctrl_prepare_unlock(struct clk_ctrl *ctrl)
+{
+	if (!ctrl)
+		return;
+
+	WARN_ON_ONCE(ctrl->prepare_owner != current);
+	WARN_ON_ONCE(ctrl->prepare_refcnt == 0);
+
+	if (--ctrl->prepare_refcnt)
+		return;
+	ctrl->prepare_owner = NULL;
+	mutex_unlock(&ctrl->prepare_lock);
+}
+
+static void clk_prepare_lock_ctrl(struct clk_core *core)
+{
+	if (!core)
+		return;
+
+	clk_ctrl_prepare_lock(core->ctrl);
+}
+
+static void clk_prepare_unlock_ctrl(struct clk_core *core)
+{
+	if (!core)
+		return;
+
+	clk_ctrl_prepare_unlock(core->ctrl);
+}
+
+static void clk_prepare_lock_parents_locked(struct clk_core *core)
+{
+	struct clk_ctrl *prev = NULL;
+
+	// lockdep_assert_held(&prepare_lock); // tmp comment?
+
+	if (!core)
+		return;
+
+	do {
+		if (core->ctrl != prev) {
+			clk_ctrl_prepare_lock(core->ctrl);
+			prev = core->ctrl;
+		}
+	} while ((core = core->parent));
+}
+
+static void clk_prepare_lock_parents(struct clk_core *core)
+{
+	if (!core)
+		return;
+
+	clk_prepare_lock();
+	clk_prepare_lock_parents_locked(core);
+	clk_prepare_unlock();
+}
+
+static void clk_prepare_unlock_parents_recur(struct clk_core *core,
+					     struct clk_ctrl *prev)
+{
+	if (!core)
+		return;
+
+	clk_prepare_unlock_parents_recur(core->parent, core->ctrl);
+	if (core->ctrl != prev)
+		clk_ctrl_prepare_unlock(core->ctrl);
+}
+
+static void clk_prepare_unlock_parents(struct clk_core *core)
+{
+	if (!core)
+		return;
+
+	clk_prepare_unlock_parents_recur(core, NULL);
+}
+
+// FIXME: important note - will skip first lock
+static void clk_prepare_lock_children_locked(struct clk_core *core)
+{
+	struct clk_core *child;
+
+	lockdep_assert_held(&prepare_lock);
+
+	if (!core)
+		return;
+
+	hlist_for_each_entry(child, &core->children, child_node) {
+		clk_prepare_lock_children_locked(child);
+
+		/* No need to double lock the same controller */
+		if (child->ctrl != core->ctrl)
+			clk_ctrl_prepare_lock(child->ctrl);
+	}
+}
+
+static void clk_prepare_lock_children(struct clk_core *core)
+{
+	if (!core)
+		return;
+
+	clk_prepare_lock();
+	clk_prepare_lock_children_locked(core);
+	/* Initial lock because children recurrency skiped first one */
+	clk_ctrl_prepare_lock(core->ctrl);
+}
+
+static void clk_prepare_unlock_children_locked(struct clk_core *core)
+{
+	struct clk_core *child;
+
+	if (!core)
+		return;
+
+	hlist_for_each_entry(child, &core->children, child_node) {
+		/* No need to double unlock the same controller */
+		if (child->ctrl != core->ctrl)
+			clk_ctrl_prepare_unlock(child->ctrl);
+
+		clk_prepare_unlock_children_locked(child);
+	}
+}
+
+static void clk_prepare_unlock_children(struct clk_core *core)
+{
+	if (!core)
+		return;
+
+	/* Unlock the initial controller, skipped in children recurrency */
+	clk_ctrl_prepare_unlock(core->ctrl);
+	clk_prepare_unlock_children_locked(core);
+	clk_prepare_unlock();
+}
+
+/* Locks prepare lock, children and parents */
+static void clk_prepare_lock_tree(struct clk_core *core)
+{
+	if (!core)
+		return;
+
+	clk_prepare_lock();
+	clk_prepare_lock_children_locked(core);
+	/* Children recurrency skiped locking first one */
+	clk_ctrl_prepare_lock(core->ctrl);
+	clk_prepare_lock_parents_locked(core);
+}
+
+static void clk_prepare_unlock_tree(struct clk_core *core)
+{
+	if (!core)
+		return;
+
+	clk_prepare_unlock_parents(core);
+	/* Unlock the initial controller, skipped in children recurrency */
+	clk_ctrl_prepare_unlock(core->ctrl);
+	clk_prepare_unlock_children_locked(core);
+	clk_prepare_unlock();
+}
+
+/*
+ * Unlocks the controller hierarchy (children and parents) but going from
+ * old parent.  Used in case of reparenting.
+ * If (core->parent == old_parent), this is equal to clk_prepare_unlock_tree().
+ */
+static void clk_prepare_unlock_oldtree(struct clk_core *core,
+				       struct clk_core *old_parent)
+{
+	if (!core)
+		return;
+
+	clk_prepare_unlock_parents(old_parent);
+	/*
+	 * Lock parents was called on 'core', but we unlock starting from
+	 * 'old_parent'. In the same time locking did not lock the same
+	 * controller twice but this check will be skipped for 'core'.
+	 */
+	if (old_parent->ctrl != core->ctrl)
+		clk_ctrl_prepare_unlock(core->ctrl);
+
+	/* Unlock the initial controller, skipped in children recurrency */
+	clk_ctrl_prepare_unlock(core->ctrl);
+	clk_prepare_unlock_children_locked(core);
+	clk_prepare_unlock();
+}
+
+/* Locks everything */
+/* FIXME: order of locking, it does not follow child-parent */
+static void clk_prepare_lock_all(void)
+{
+	struct clk_ctrl *ctrl;
+
+	clk_prepare_lock();
+	list_for_each_entry(ctrl, &clk_ctrl_list, node)
+		clk_ctrl_prepare_lock(ctrl);
+}
+
+static void clk_prepare_unlock_all(void)
+{
+	struct clk_ctrl *ctrl;
+
+	list_for_each_entry(ctrl, &clk_ctrl_list, node)
+		clk_ctrl_prepare_unlock(ctrl);
+	clk_prepare_unlock();
+}
+
 static bool clk_core_is_prepared(struct clk_core *core)
 {
 	/*

@@ -2526,6 +2768,34 @@ void __clk_free_clk(struct clk *clk)
 	kfree(clk);
 }
 
+struct clk_ctrl *clk_ctrl_register(struct device *dev)
+{
+	struct clk_ctrl *ctrl;
+
+	ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL);
+	if (!ctrl)
+		return ERR_PTR(-ENOMEM);
+
+	mutex_init(&ctrl->prepare_lock);
+
+	clk_prepare_lock();
+	list_add(&ctrl->node, &clk_ctrl_list);
+	clk_prepare_unlock();
+
+	return ctrl;
+}
+EXPORT_SYMBOL_GPL(clk_ctrl_register);
+
+void clk_ctrl_unregister(struct clk_ctrl *ctrl)
+{
+	clk_prepare_lock();
+	list_del(&ctrl->node);
+	clk_prepare_unlock();
+
+	kfree(ctrl);
+}
+EXPORT_SYMBOL_GPL(clk_ctrl_unregister);
+
 /**
  * clk_register - allocate a new clock, register it and return an opaque cookie
  * @dev: device that is registering this clock

@@ -2537,7 +2807,8 @@ void __clk_free_clk(struct clk *clk)
  * rest of the clock API.  In the event of an error clk_register will return an
  * error code; drivers must test for an error code after calling clk_register.
  */
-struct clk *clk_register(struct device *dev, struct clk_hw *hw)
+struct clk *clk_register_with_ctrl(struct device *dev, struct clk_ctrl *ctrl,
+				   struct clk_hw *hw)
 {
 	int i, ret;
 	struct clk_core *core;

@@ -2561,6 +2832,10 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 	core->num_parents = hw->init->num_parents;
 	core->min_rate = 0;
 	core->max_rate = ULONG_MAX;
+	if (ctrl)
+		core->ctrl = ctrl;
+	else
+		core->ctrl = &global_ctrl;
 	hw->core = core;
 
 	/* allocate local copy in case parent_names is __initdata */

@@ -2619,7 +2894,7 @@ fail_name:
 fail_out:
 	return ERR_PTR(ret);
 }
-EXPORT_SYMBOL_GPL(clk_register);
+EXPORT_SYMBOL_GPL(clk_register_with_ctrl);
 
 /**
  * clk_hw_register - register a clk_hw and return an error code

@@ -2631,11 +2906,12 @@ EXPORT_SYMBOL_GPL(clk_register);
  * less than zero indicating failure. Drivers must test for an error code after
  * calling clk_hw_register().
  */
-int clk_hw_register(struct device *dev, struct clk_hw *hw)
+int clk_hw_register_with_ctrl(struct device *dev, struct clk_ctrl *ctrl,
+			      struct clk_hw *hw)
 {
-	return PTR_ERR_OR_ZERO(clk_register(dev, hw));
+	return PTR_ERR_OR_ZERO(clk_register_with_ctrl(dev, ctrl, hw));
 }
-EXPORT_SYMBOL_GPL(clk_hw_register);
+EXPORT_SYMBOL_GPL(clk_hw_register_with_ctrl);
 
 /* Free memory allocated for a clock. */
 static void __clk_release(struct kref *ref)

@@ -2644,6 +2920,7 @@ static void __clk_release(struct kref *ref)
 	int i = core->num_parents;
 
 	lockdep_assert_held(&prepare_lock);
+	// lockdep_assert_not_held(&core->ctrl->prepare_lock); // TODO?
 
 	kfree(core->parents);
 	while (--i >= 0)

@@ -2767,7 +3044,7 @@ static void devm_clk_hw_release(struct device *dev, void *res)
  * automatically clk_unregister()ed on driver detach. See clk_register() for
  * more information.
  */
-struct clk *devm_clk_register(struct device *dev, struct clk_hw *hw)
+struct clk *devm_clk_register_with_ctrl(struct device *dev, struct clk_ctrl *ctrl, struct clk_hw *hw)
 {
 	struct clk *clk;
 	struct clk **clkp;

@@ -2776,7 +3053,7 @@ struct clk *devm_clk_register(struct device *dev, struct clk_hw *hw)
 	if (!clkp)
 		return ERR_PTR(-ENOMEM);
 
-	clk = clk_register(dev, hw);
+	clk = clk_register_with_ctrl(dev, ctrl, hw);
 	if (!IS_ERR(clk)) {
 		*clkp = clk;
 		devres_add(dev, clkp);

@@ -2786,7 +3063,7 @@ struct clk *devm_clk_register(struct device *dev, struct clk_hw *hw)
 
 	return clk;
 }
-EXPORT_SYMBOL_GPL(devm_clk_register);
+EXPORT_SYMBOL_GPL(devm_clk_register_with_ctrl);
 
 /**
  * devm_clk_hw_register - resource managed clk_hw_register()

@@ -2797,7 +3074,8 @@ EXPORT_SYMBOL_GPL(devm_clk_register);
  * automatically clk_hw_unregister()ed on driver detach. See clk_hw_register()
  * for more information.
  */
-int devm_clk_hw_register(struct device *dev, struct clk_hw *hw)
+int devm_clk_hw_register_with_ctrl(struct device *dev, struct clk_ctrl *ctrl,
+				   struct clk_hw *hw)
 {
 	struct clk_hw **hwp;
 	int ret;

@@ -2806,7 +3084,7 @@ int devm_clk_hw_register(struct device *dev, struct clk_hw *hw)
 	if (!hwp)
 		return -ENOMEM;
 
-	ret = clk_hw_register(dev, hw);
+	ret = clk_hw_register_with_ctrl(dev, ctrl, hw);
 	if (!ret) {
 		*hwp = hw;
 		devres_add(dev, hwp);

@@ -2816,7 +3094,7 @@ int devm_clk_hw_register(struct device *dev, struct clk_hw *hw)
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(devm_clk_hw_register);
+EXPORT_SYMBOL_GPL(devm_clk_hw_register_with_ctrl);
 
 static int devm_clk_match(struct device *dev, void *res, void *data)
 {

diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index a39c0c530778..3589f164ff94 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h

@@ -39,6 +39,7 @@
 struct clk;
 struct clk_hw;
 struct clk_core;
+struct clk_ctrl;
 struct dentry;
 
 /**

@@ -703,6 +704,8 @@ struct clk_hw *clk_hw_register_gpio_mux(struct device *dev, const char *name,
 		bool active_low, unsigned long flags);
 void clk_hw_unregister_gpio_mux(struct clk_hw *hw);
 
+struct clk_ctrl *clk_ctrl_register(struct device *dev);
+void clk_ctrl_unregister(struct clk_ctrl *ctrl);
 /**
  * clk_register - allocate a new clock, register it and return an opaque cookie
  * @dev: device that is registering this clock

@@ -714,11 +717,23 @@ void clk_hw_unregister_gpio_mux(struct clk_hw *hw);
  * rest of the clock API.  In the event of an error clk_register will return an
  * error code; drivers must test for an error code after calling clk_register.
  */
-struct clk *clk_register(struct device *dev, struct clk_hw *hw);
-struct clk *devm_clk_register(struct device *dev, struct clk_hw *hw);
-
-int __must_check clk_hw_register(struct device *dev, struct clk_hw *hw);
-int __must_check devm_clk_hw_register(struct device *dev, struct clk_hw *hw);
+struct clk *clk_register_with_ctrl(struct device *dev, struct clk_ctrl *ctrl,
+				   struct clk_hw *hw);
+struct clk *devm_clk_register_with_ctrl(struct device *dev, struct clk_ctrl *ctrl,
+					struct clk_hw *hw);
+
+#define clk_register(dev, hw)		clk_register_with_ctrl(dev, NULL, hw)
+#define devm_clk_register(dev, hw)	devm_clk_register_with_ctrl(dev, NULL, hw)
+
+int __must_check clk_hw_register_with_ctrl(struct device *dev,
+					   struct clk_ctrl *ctrl,
+					   struct clk_hw *hw);
+int __must_check devm_clk_hw_register_with_ctrl(struct device *dev,
+						struct clk_ctrl *ctrl,
+						struct clk_hw *hw);
+
+#define clk_hw_register(dev, hw) clk_hw_register_with_ctrl(dev, NULL, hw)
+#define devm_clk_hw_register(dev, hw) devm_clk_hw_register_with_ctrl(dev, NULL, hw)
 
 void clk_unregister(struct clk *clk);
 void devm_clk_unregister(struct device *dev, struct clk *clk);

diff --git a/include/linux/clk.h b/include/linux/clk.h
index 123c02788807..8f751d1eb1df 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h

@@ -19,6 +19,7 @@
 struct device;
 
 struct clk;
+struct clk_ctrl;
 
 /**
  * DOC: clk notifier callback types

-- 
1.9.1

Re: [RFC 00/17] clk: Add per-controller locks to fix deadlocks

From: Krzysztof Kozlowski <hidden>
Date: 2016-08-16 13:51:20

On 08/16/2016 03:34 PM, Krzysztof Kozlowski wrote:

Hi,

RFC, please, do not apply, maybe except patch #1 which is harmless.


Introduction
============
The patchset brings new entity: clock controller representing a hardware
block.  The clock controller comes with its own prepare lock which
is used then in many places.  The idea is to fix the deadlock mentioned
in commit 10ff4c5239a1 ("i2c: exynos5: Fix possible ABBA deadlock by keeping
I2C clock prepared") and commit 34e81ad5f0b6 ("i2c: s3c2410: fix ABBA deadlock
by keeping clock prepared").

Damn, I forgot to describe the overall idea. :) It is mentioned in patch
15 but probably not many will have enough of patience to reach it.

The locking idea
================
Clock controllers representing different hardware blocks, will contain
its own prepare lock which protects the clocks inside controller.  The
hierarchy itself is protected by global lock.

In prepare path, the global prepare lock is removed.  This is direct
solution for the deadlock.

Clock hierarchy imposes also hierarchy between controllers so when a
prepare happens, also parents have to be locked.

Following locking design was chosen:
1. For prepare/unprepare paths: lock only clock controller and its
   parents.
2. For recalc rates paths: lock global lock, the controller and its
   children.
3. For reparent paths: lock entire tree up down (children and parents)
   and the global lock as well.


In each case of traversing the clock hierarchy, the locking of
controllers is always from children to parents.


Best regards,
Krzysztof

Re: [RFC 00/17] clk: Add per-controller locks to fix deadlocks

From: Charles Keepax <hidden>
Date: 2016-08-19 14:47:26

On Tue, Aug 16, 2016 at 03:51:10PM +0200, Krzysztof Kozlowski wrote:

On 08/16/2016 03:34 PM, Krzysztof Kozlowski wrote:

quoted

Hi,

RFC, please, do not apply, maybe except patch #1 which is harmless.


Introduction
============
The patchset brings new entity: clock controller representing a hardware
block.  The clock controller comes with its own prepare lock which
is used then in many places.  The idea is to fix the deadlock mentioned
in commit 10ff4c5239a1 ("i2c: exynos5: Fix possible ABBA deadlock by keeping
I2C clock prepared") and commit 34e81ad5f0b6 ("i2c: s3c2410: fix ABBA deadlock
by keeping clock prepared").

Damn, I forgot to describe the overall idea. :) It is mentioned in patch
15 but probably not many will have enough of patience to reach it.

The locking idea
================
Clock controllers representing different hardware blocks, will contain
its own prepare lock which protects the clocks inside controller.  The
hierarchy itself is protected by global lock.

In prepare path, the global prepare lock is removed.  This is direct
solution for the deadlock.

Clock hierarchy imposes also hierarchy between controllers so when a
prepare happens, also parents have to be locked.

Following locking design was chosen:
1. For prepare/unprepare paths: lock only clock controller and its
   parents.
2. For recalc rates paths: lock global lock, the controller and its
   children.
3. For reparent paths: lock entire tree up down (children and parents)
   and the global lock as well.


In each case of traversing the clock hierarchy, the locking of
controllers is always from children to parents.


Best regards,
Krzysztof

I have been playing with these patches on my Arndale board and
they certainly do seem to resolve the interaction issues I have
been SPI and the clocking framework, which is awesome and lets me
sensibly add the clocking framework into our codec drivers. I will
keep investigating and for what its worth have a little more
detailed look through the code.

Thanks,
Charles

Re: [RFC 00/17] clk: Add per-controller locks to fix deadlocks

From: Krzysztof Kozlowski <krzk@kernel.org>
Date: 2016-08-19 16:58:56

On Fri, Aug 19, 2016 at 03:46:40PM +0100, Charles Keepax wrote:

On Tue, Aug 16, 2016 at 03:51:10PM +0200, Krzysztof Kozlowski wrote:

quoted

On 08/16/2016 03:34 PM, Krzysztof Kozlowski wrote:

quoted

Hi,

RFC, please, do not apply, maybe except patch #1 which is harmless.


Introduction
============
The patchset brings new entity: clock controller representing a hardware
block.  The clock controller comes with its own prepare lock which
is used then in many places.  The idea is to fix the deadlock mentioned
in commit 10ff4c5239a1 ("i2c: exynos5: Fix possible ABBA deadlock by keeping
I2C clock prepared") and commit 34e81ad5f0b6 ("i2c: s3c2410: fix ABBA deadlock
by keeping clock prepared").

Damn, I forgot to describe the overall idea. :) It is mentioned in patch
15 but probably not many will have enough of patience to reach it.

The locking idea
================
Clock controllers representing different hardware blocks, will contain
its own prepare lock which protects the clocks inside controller.  The
hierarchy itself is protected by global lock.

In prepare path, the global prepare lock is removed.  This is direct
solution for the deadlock.

Clock hierarchy imposes also hierarchy between controllers so when a
prepare happens, also parents have to be locked.

Following locking design was chosen:
1. For prepare/unprepare paths: lock only clock controller and its
   parents.
2. For recalc rates paths: lock global lock, the controller and its
   children.
3. For reparent paths: lock entire tree up down (children and parents)
   and the global lock as well.


In each case of traversing the clock hierarchy, the locking of
controllers is always from children to parents.


Best regards,
Krzysztof

I have been playing with these patches on my Arndale board and
they certainly do seem to resolve the interaction issues I have
been SPI and the clocking framework, which is awesome and lets me
sensibly add the clocking framework into our codec drivers. I will
keep investigating and for what its worth have a little more
detailed look through the code.

I am really happy to hear it!

Along with other Samsung guys from Poland we really spent a lot of time
figuring out all the locking cases, possible scenarios and new issues
which could come out of it.

I am glad that it solves also other people's cases, not only ours!

Certainly there is a lot of things to improve in the patchset. Probably
merging the new "clock controller" entity into clock provider makes
sense.

Best regards,
Krzysztof

Re: [RFC 00/17] clk: Add per-controller locks to fix deadlocks

From: Javier Martinez Canillas <hidden>
Date: 2016-08-19 19:32:00

Hello Krzysztof,

On 08/16/2016 09:34 AM, Krzysztof Kozlowski wrote:

Hi,

RFC, please, do not apply, maybe except patch #1 which is harmless.


Introduction
============
The patchset brings new entity: clock controller representing a hardware
block.  The clock controller comes with its own prepare lock which
is used then in many places.  The idea is to fix the deadlock mentioned
in commit 10ff4c5239a1 ("i2c: exynos5: Fix possible ABBA deadlock by keeping
I2C clock prepared") and commit 34e81ad5f0b6 ("i2c: s3c2410: fix ABBA deadlock
by keeping clock prepared").

I'm not familiar enough with the common clock framework to do a proper review
of this patch-set, but I've tested both on an Exynos5800 Peach Pi Chromebook
and an Exynos5422 Odroid XU4 board and I didn't find any clock regressions.

Also, I confirmed that the possible deadlock in the Odroid XU4 that was fixed
by reverted commit 10ff4c5239a1 doesn't happen anymore with your patches.

Tested-by: Javier Martinez Canillas <redacted>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [RFC 00/17] clk: Add per-controller locks to fix deadlocks

From: Krzysztof Kozlowski <krzk@kernel.org>
Date: 2016-08-20 16:03:26

On Fri, Aug 19, 2016 at 03:31:08PM -0400, Javier Martinez Canillas wrote:

Hello Krzysztof,

On 08/16/2016 09:34 AM, Krzysztof Kozlowski wrote:

quoted

Hi,

RFC, please, do not apply, maybe except patch #1 which is harmless.


Introduction
============
The patchset brings new entity: clock controller representing a hardware
block.  The clock controller comes with its own prepare lock which
is used then in many places.  The idea is to fix the deadlock mentioned
in commit 10ff4c5239a1 ("i2c: exynos5: Fix possible ABBA deadlock by keeping
I2C clock prepared") and commit 34e81ad5f0b6 ("i2c: s3c2410: fix ABBA deadlock
by keeping clock prepared").

I'm not familiar enough with the common clock framework to do a proper review
of this patch-set, but I've tested both on an Exynos5800 Peach Pi Chromebook
and an Exynos5422 Odroid XU4 board and I didn't find any clock regressions.

Also, I confirmed that the possible deadlock in the Odroid XU4 that was fixed
by reverted commit 10ff4c5239a1 doesn't happen anymore with your patches.

Tested-by: Javier Martinez Canillas <redacted>

Thanks for testing, I appreciate that!

Best regards,
Krzysztof

Re: [RFC 00/17] clk: Add per-controller locks to fix deadlocks

From: Stephen Boyd <hidden>
Date: 2016-09-09 00:24:39

On 08/16, Krzysztof Kozlowski wrote:

Hi,

RFC, please, do not apply, maybe except patch #1 which is harmless.


Introduction
============
The patchset brings new entity: clock controller representing a hardware
block.  The clock controller comes with its own prepare lock which
is used then in many places.  The idea is to fix the deadlock mentioned
in commit 10ff4c5239a1 ("i2c: exynos5: Fix possible ABBA deadlock by keeping
I2C clock prepared") and commit 34e81ad5f0b6 ("i2c: s3c2410: fix ABBA deadlock
by keeping clock prepared").


Disclaimer
==========
Request for comments, so:
1. Only exynos_defconfig builds,
2. A lot of FIXME/TODO note still,
3. Checkpatch not run, lines not aligned,
4. Other (non-exynos) drivers not converted,
5. Probably not yet bisectable,
6. Locking became quite complex.
   The previous one lock was simple.  Inefficient and dead-lock prone but
   simple.  Because of clock hierarchy spanning through controllers, the
   new locking became quite complicated.  I don't like it but...


Details
=======
In Exynos-based boards case the deadlock occurs between clock's
prepare_lock and regmap-i2c's lock:

CPU #0:                             CPU #1:
lock(regmap)
                                    s2mps11-clk: clk_prepare_lock()

i2c-exynos: clk_prepare_lock() - wait
                                    lock(regmap) - wait

The clk_prepare_lock() on both CPUs come from different clock drivers
and components:
1. I2C clock is part of SoC block and is required by i2c-s3c2410/i2c-exynos5
   driver,
2. S2MPS11 clock is separate device, however driver uses I2C regmap.

The deadlock was reported by lockdep (always) and was happening
in 20% of boots of Odroid XU3 with multi_v7 defconfig.  Workaround for
deadlock was implemented by removing prepare/unprepare calls from I2C
transfers.  However these are just workarounds... which after applying
this patch can be reverted.

Additionally Marek Szyprowski's work on domains/clocks/pinctrl exposed
the deadlock again in different configuration.

Per-controller locks seems to be a misnomer. These patches look
more like per-clk_register() locks. The clk registrant can decide
how fine grained to make the prepare locking scheme. They can
decide to have one lock per clk, one lock for the entire tree, or
one lock per arbitrary subtree. I'd prefer to drop the controller
concept unless it's really done that way, by attaching to the OF
clk provider or device pointer.

Given that, it seems that conceptually these patches allow clk
registrants to carve out a set of clks that they want to be
treated as an atomic unit with regards to the prepare lock.
TL;DR trees of per-process recursive prepare mutex locks where
the locks cover one or more clks.

The locks are recursive per-process to simplify the problem where
we need to acquire the same lock multiple times while traversing
a tree that shares the same lock, right? Otherwise we would need
to track which locks we've already grabbed and refcount and we
wouldn't be able to re-enter the framework from within the clk
ops of the same atomic unit.

As long as we can guarantee that we always take the locks in the
same order we'll be fine. The burden to ensure that would be
placed on the clk registrants though, and verifying that will be
left up to humans? That seems fragile and error prone. We can
always say that we take the global lock and then traverse the
children up to the roots, but we can't be sure that during the
tree traversal we don't take locks in different order because of
how the clk registrant has structured things.

It would be great if the different clk APIs were listed, with the
order of how locks are taken BTW. And really overall just more
information about how the locking scheme works. I had to read
this patch series quite a few times to come to (hopefully the
right) conclusions.

So I'm not very fond of this design because the locking scheme is
pretty much out of the hands of the framework and can be easily
broken. I'm biased of course, because I'd prefer we go with my
wwmutex design of per-clk locks[1]. Taking locks in any order
works fine there, and we resolve quite a few long standing
locking problems that we have while improving scalability. The
problem there is that we don't get the recursive mutex design
(maybe that's a benefit!). Once a clk_op reenters the framework
with consumer APIs and tries to grab the same lock we deadlock.
This is why I've been slowly splitting consumers from providers
so we can easily identify these cases. If we had something like
coordinated clk rate switching, we could get rid of clk_ops
reentering the framework and avoid this problem (and we really do
need to do that).

The one thing I do like about this patch series though is the
gradual movement of providers to the new locking scheme. Maybe if
we combined that idea with per-clk wwmutex locks we could have
the best of both worlds and fix reentrancy later when we need it?
There's wwmutex_trylock() that we could use. Perhaps always pass
a NULL context if some clk flag isn't set (CLK_USE_PER_CLK_LOCK).
Plus we could assign the same wwmutex all over the place when
that flag isn't set and allocate a unique wwmutex at clk_register
time otherwise. I haven't thought about it deeply, so there may
be some glaring problem that I'm missing like when we have half
the clks in a tree with the flag set or something. This is where
driving home from the office for 45 minutes helps.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-April/251039.html

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC 00/17] clk: Add per-controller locks to fix deadlocks

From: Marek Szyprowski <m.szyprowski@samsung.com>
Date: 2016-11-04 10:53:28

Hi Stephen,

Krzysztof has left Samsung, but we would like to continue this task, because
the ABBA dead-locks related to global prepare lock becomes more and more
problematic for us to workaround.

On 2016-09-09 02:24, Stephen Boyd wrote:

On 08/16, Krzysztof Kozlowski wrote:

quoted

RFC, please, do not apply, maybe except patch #1 which is harmless.


Introduction
============
The patchset brings new entity: clock controller representing a hardware
block.  The clock controller comes with its own prepare lock which
is used then in many places.  The idea is to fix the deadlock mentioned
in commit 10ff4c5239a1 ("i2c: exynos5: Fix possible ABBA deadlock by keeping
I2C clock prepared") and commit 34e81ad5f0b6 ("i2c: s3c2410: fix ABBA deadlock
by keeping clock prepared").


Disclaimer
==========
Request for comments, so:
1. Only exynos_defconfig builds,
2. A lot of FIXME/TODO note still,
3. Checkpatch not run, lines not aligned,
4. Other (non-exynos) drivers not converted,
5. Probably not yet bisectable,
6. Locking became quite complex.
    The previous one lock was simple.  Inefficient and dead-lock prone but
    simple.  Because of clock hierarchy spanning through controllers, the
    new locking became quite complicated.  I don't like it but...


Details
=======
In Exynos-based boards case the deadlock occurs between clock's
prepare_lock and regmap-i2c's lock:

CPU #0:                             CPU #1:
lock(regmap)
                                     s2mps11-clk: clk_prepare_lock()

i2c-exynos: clk_prepare_lock() - wait
                                     lock(regmap) - wait

The clk_prepare_lock() on both CPUs come from different clock drivers
and components:
1. I2C clock is part of SoC block and is required by i2c-s3c2410/i2c-exynos5
    driver,
2. S2MPS11 clock is separate device, however driver uses I2C regmap.

The deadlock was reported by lockdep (always) and was happening
in 20% of boots of Odroid XU3 with multi_v7 defconfig.  Workaround for
deadlock was implemented by removing prepare/unprepare calls from I2C
transfers.  However these are just workarounds... which after applying
this patch can be reverted.

Additionally Marek Szyprowski's work on domains/clocks/pinctrl exposed
the deadlock again in different configuration.

Per-controller locks seems to be a misnomer. These patches look
more like per-clk_register() locks. The clk registrant can decide
how fine grained to make the prepare locking scheme. They can
decide to have one lock per clk, one lock for the entire tree, or
one lock per arbitrary subtree. I'd prefer to drop the controller
concept unless it's really done that way, by attaching to the OF
clk provider or device pointer.

This was not phrased well but the idea was to let provider/controller
have ability to define which clocks will use common lock. Probably
removing the concept of clock controller and moving the lock to
clk_provider will be much better approach.

Given that, it seems that conceptually these patches allow clk
registrants to carve out a set of clks that they want to be
treated as an atomic unit with regards to the prepare lock.
TL;DR trees of per-process recursive prepare mutex locks where
the locks cover one or more clks.

The locks are recursive per-process to simplify the problem where
we need to acquire the same lock multiple times while traversing
a tree that shares the same lock, right? Otherwise we would need
to track which locks we've already grabbed and refcount and we
wouldn't be able to re-enter the framework from within the clk
ops of the same atomic unit.

As long as we can guarantee that we always take the locks in the
same order we'll be fine. The burden to ensure that would be
placed on the clk registrants though, and verifying that will be
left up to humans? That seems fragile and error prone. We can
always say that we take the global lock and then traverse the
children up to the roots, but we can't be sure that during the
tree traversal we don't take locks in different order because of
how the clk registrant has structured things.

It would be great if the different clk APIs were listed, with the
order of how locks are taken BTW. And really overall just more
information about how the locking scheme works. I had to read
this patch series quite a few times to come to (hopefully the
right) conclusions.

Right, such change requires much more documentation and especially
that it took significant amount of time to figure out all(?)
possible use cases and locking schemes.

So I'm not very fond of this design because the locking scheme is
pretty much out of the hands of the framework and can be easily
broken.

Well, switching from a single global lock to more granular locking
is always a good approach. Please note that the global lock sooner
or later became a serious bottleneck if one wants to make somehow
more aggressive power management and clock gating.

  I'm biased of course, because I'd prefer we go with my
wwmutex design of per-clk locks[1]. Taking locks in any order
works fine there, and we resolve quite a few long standing
locking problems that we have while improving scalability. The
problem there is that we don't get the recursive mutex design
(maybe that's a benefit!).

Do you have any plan to continue working on your approach? per-clk
wwmutex looks like an overkill on the first glance, but that's probably
the only working solution if you want to get rid of recursive locks.
I'm still not really convinced that we really need wwmutex here,
especially if it is possible to guarantee the same order of locking
operations inside the clock core. This requires a bit of cooperation
from clock providers (with proper documentation and a list of
DO/DON'T it shouldn't be that hard).

Once a clk_op reenters the framework
with consumer APIs and tries to grab the same lock we deadlock.
This is why I've been slowly splitting consumers from providers
so we can easily identify these cases. If we had something like
coordinated clk rate switching, we could get rid of clk_ops
reentering the framework and avoid this problem (and we really do
need to do that).

I'm not sure that this makes really sense split consumers and
providers. You will get recursive calls to clk core anyway with
consumers calls if you are implementing i2c clock, for which an i2c
bus driver does it's own clock gating (i2c controller uses
consumer clk api).

The one thing I do like about this patch series though is the
gradual movement of providers to the new locking scheme. Maybe if
we combined that idea with per-clk wwmutex locks we could have
the best of both worlds and fix reentrancy later when we need it?
There's wwmutex_trylock() that we could use. Perhaps always pass
a NULL context if some clk flag isn't set (CLK_USE_PER_CLK_LOCK).
Plus we could assign the same wwmutex all over the place when
that flag isn't set and allocate a unique wwmutex at clk_register
time otherwise. I haven't thought about it deeply, so there may
be some glaring problem that I'm missing like when we have half
the clks in a tree with the flag set or something. This is where
driving home from the office for 45 minutes helps.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-April/251039.html

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [RFC 00/17] clk: Add per-controller locks to fix deadlocks

From: Stephen Boyd <hidden>
Date: 2016-11-12 02:38:50

On 11/04, Marek Szyprowski wrote:

Hi Stephen,

Krzysztof has left Samsung, but we would like to continue this task, because
the ABBA dead-locks related to global prepare lock becomes more and more
problematic for us to workaround.

Hmm. Ok. Thanks for the info.

On 2016-09-09 02:24, Stephen Boyd wrote:

quoted

So I'm not very fond of this design because the locking scheme is
pretty much out of the hands of the framework and can be easily
broken.

Well, switching from a single global lock to more granular locking
is always a good approach. Please note that the global lock sooner
or later became a serious bottleneck if one wants to make somehow
more aggressive power management and clock gating.

I'm not so sure switching from a global lock to a more granular
lock is _always_ a great idea. Sometimes simpler code is better,
even if it doesn't scale to a million clk nodes. The largest
systems I've seen only have clocks in the hundreds, and a
majority of those aren't rate changing in parallel, so it's not
like we're suffering from VFS type scalability problems here with
tens of thousands of inodes.

That isn't to say I don't agree there's a scalability problem
here, but I'd like to actually see numbers to prove that there's
some sort of scalability problem before making drastic changes.

quoted

 I'm biased of course, because I'd prefer we go with my
wwmutex design of per-clk locks[1]. Taking locks in any order
works fine there, and we resolve quite a few long standing
locking problems that we have while improving scalability. The
problem there is that we don't get the recursive mutex design
(maybe that's a benefit!).

Do you have any plan to continue working on your approach? per-clk
wwmutex looks like an overkill on the first glance, but that's probably
the only working solution if you want to get rid of recursive locks.
I'm still not really convinced that we really need wwmutex here,
especially if it is possible to guarantee the same order of locking
operations inside the clock core. This requires a bit of cooperation
from clock providers (with proper documentation and a list of
DO/DON'T it shouldn't be that hard).

So far I haven't gotten around to resurrecting the wwmutex stuff.
If you have interest in doing it that's great. Having a locking
scheme with rules of DO/DON'T sounds brittle to me, unless it can
be automated to find problems. I know that I'm not going to spend
time policing that.

quoted

Once a clk_op reenters the framework
with consumer APIs and tries to grab the same lock we deadlock.
This is why I've been slowly splitting consumers from providers
so we can easily identify these cases. If we had something like
coordinated clk rate switching, we could get rid of clk_ops
reentering the framework and avoid this problem (and we really do
need to do that).

I'm not sure that this makes really sense split consumers and
providers. You will get recursive calls to clk core anyway with
consumers calls if you are implementing i2c clock, for which an i2c
bus driver does it's own clock gating (i2c controller uses
consumer clk api).

I suppose this is a different topic. Regardless of the recursive
call or not, we can easily see that a clk consumer is also a clk
provider and just knowing that is useful. Once we know that, we
can look to see if they're calling clk consumer APIs from their
provider callbacks which is not desired because it makes it
impossible to get rid of the recursive lock design. If the lock
is per-clock, then recursion doesn't happen when the provider is
also a consumer. If it does, that's bad and lockdep should tell
us.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help