[PATCH v2 0/8] clk: helpers and fixes

[PATCH v2 0/8] clk: helpers and fixes

[Jerome Brunet]

This changset is consist of various patches I have recently sent
for the clock framework. They are gathered here for your convinience.

The first two changes exports helpers of the generic clocks (divider and
mux). The goal is to avoid code duplication when writing clock driver
derived from these generic clock drivers.

The rest fixes various issues found in the clock framework while working on
a rework of meson's clock controllers [0]

Changes since the v1s:
 * dropped lpc32xx divider read-only patches as requested by
   Vladimir Zapolskiy [2]
 * Squashed generic divider read-only patches
 * Squashed mux documentations patches

[0]: https://lkml.kernel.org/r/20180212145846.19380-1-jbrunet@baylibre.com
[2]: https://lkml.kernel.org/r/54782a8f-9735-a574-d8e1-7ff3e9173229@mleia.com

Jerome Brunet (8):
  clk: divider: export clk_div_mask() helper
  clk: mux: add helper function for index/value translation
  clk: fix determine rate error with pass-through clock
  clk: migrate the count of orphaned clocks at init
  clk: call the clock init() callback before any other ops callback
  clk: fix mux clock documentation
  clk: divider: read-only divider can propagate rate change
  clk: qcom: use divider_ro_round_rate helper

 drivers/clk/clk-divider.c             | 58 ++++++++++++++++++---------
 drivers/clk/clk-mux.c                 | 75 ++++++++++++++++++++---------------
 drivers/clk/clk.c                     | 63 ++++++++++++++++-------------
 drivers/clk/qcom/clk-regmap-divider.c | 20 +++-------
 include/linux/clk-provider.h          | 23 ++++++++++-
 5 files changed, 146 insertions(+), 93 deletions(-)

-- 
2.14.3

[PATCH v2 1/8] clk: divider: export clk_div_mask() helper

[Jerome Brunet]

Export clk_div_mask() in clk-provider header so every clock providers
derived from the generic clock divider may share the definition instead
of redefining it.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---

I have 's/div_mask/clk_div_mask' to avoid the conflict with
tegra's divider, which also defines the macro div_mask() but does not
actually derives from the generic divider.

 drivers/clk/clk-divider.c    | 24 +++++++++++-------------
 include/linux/clk-provider.h |  1 +
 2 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index b49942b9fe50..3c98d2650fa3 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -28,12 +28,10 @@
  * parent - fixed parent.  No clk_set_parent support
  */
 
-#define div_mask(width)	((1 << (width)) - 1)
-
 static unsigned int _get_table_maxdiv(const struct clk_div_table *table,
 				      u8 width)
 {
-	unsigned int maxdiv = 0, mask = div_mask(width);
+	unsigned int maxdiv = 0, mask = clk_div_mask(width);
 	const struct clk_div_table *clkt;
 
 	for (clkt = table; clkt->div; clkt++)
@@ -57,12 +55,12 @@ static unsigned int _get_maxdiv(const struct clk_div_table *table, u8 width,
 				unsigned long flags)
 {
 	if (flags & CLK_DIVIDER_ONE_BASED)
-		return div_mask(width);
+		return clk_div_mask(width);
 	if (flags & CLK_DIVIDER_POWER_OF_TWO)
-		return 1 << div_mask(width);
+		return 1 << clk_div_mask(width);
 	if (table)
 		return _get_table_maxdiv(table, width);
-	return div_mask(width) + 1;
+	return clk_div_mask(width) + 1;
 }
 
 static unsigned int _get_table_div(const struct clk_div_table *table,
@@ -84,7 +82,7 @@ static unsigned int _get_div(const struct clk_div_table *table,
 	if (flags & CLK_DIVIDER_POWER_OF_TWO)
 		return 1 << val;
 	if (flags & CLK_DIVIDER_MAX_AT_ZERO)
-		return val ? val : div_mask(width) + 1;
+		return val ? val : clk_div_mask(width) + 1;
 	if (table)
 		return _get_table_div(table, val);
 	return val + 1;
@@ -109,7 +107,7 @@ static unsigned int _get_val(const struct clk_div_table *table,
 	if (flags & CLK_DIVIDER_POWER_OF_TWO)
 		return __ffs(div);
 	if (flags & CLK_DIVIDER_MAX_AT_ZERO)
-		return (div == div_mask(width) + 1) ? 0 : div;
+		return (div == clk_div_mask(width) + 1) ? 0 : div;
 	if (table)
 		return  _get_table_val(table, div);
 	return div - 1;
@@ -141,7 +139,7 @@ static unsigned long clk_divider_recalc_rate(struct clk_hw *hw,
 	unsigned int val;
 
 	val = clk_readl(divider->reg) >> divider->shift;
-	val &= div_mask(divider->width);
+	val &= clk_div_mask(divider->width);
 
 	return divider_recalc_rate(hw, parent_rate, val, divider->table,
 				   divider->flags, divider->width);
@@ -353,7 +351,7 @@ static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,
 	/* if read only, just return current value */
 	if (divider->flags & CLK_DIVIDER_READ_ONLY) {
 		bestdiv = clk_readl(divider->reg) >> divider->shift;
-		bestdiv &= div_mask(divider->width);
+		bestdiv &= clk_div_mask(divider->width);
 		bestdiv = _get_div(divider->table, bestdiv, divider->flags,
 			divider->width);
 		return DIV_ROUND_UP_ULL((u64)*prate, bestdiv);
@@ -376,7 +374,7 @@ int divider_get_val(unsigned long rate, unsigned long parent_rate,
 
 	value = _get_val(table, div, flags, width);
 
-	return min_t(unsigned int, value, div_mask(width));
+	return min_t(unsigned int, value, clk_div_mask(width));
 }
 EXPORT_SYMBOL_GPL(divider_get_val);
 
@@ -399,10 +397,10 @@ static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
 		__acquire(divider->lock);
 
 	if (divider->flags & CLK_DIVIDER_HIWORD_MASK) {
-		val = div_mask(divider->width) << (divider->shift + 16);
+		val = clk_div_mask(divider->width) << (divider->shift + 16);
 	} else {
 		val = clk_readl(divider->reg);
-		val &= ~(div_mask(divider->width) << divider->shift);
+		val &= ~(clk_div_mask(divider->width) << divider->shift);
 	}
 	val |= (u32)value << divider->shift;
 	clk_writel(val, divider->reg);
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index f711be6e8c44..d8ba26d03332 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -399,6 +399,7 @@ struct clk_divider {
 	spinlock_t	*lock;
 };
 
+#define clk_div_mask(width)	((1 << (width)) - 1)
 #define to_clk_divider(_hw) container_of(_hw, struct clk_divider, hw)
 
 #define CLK_DIVIDER_ONE_BASED		BIT(0)
-- 
2.14.3

[PATCH v2 2/8] clk: mux: add helper function for index/value translation

[Jerome Brunet]

Add helper functions for the translation between parent index and
register value in the generic multiplexer function. The purpose of
this change is avoid duplicating the code in other clock providers,
using the same generic logic.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/clk-mux.c        | 75 +++++++++++++++++++++++++-------------------
 include/linux/clk-provider.h |  4 +++
 2 files changed, 47 insertions(+), 32 deletions(-)

diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c
index 39cabe157163..ac4a042f8658 100644
--- a/drivers/clk/clk-mux.c
+++ b/drivers/clk/clk-mux.c
@@ -26,35 +26,24 @@
  * parent - parent is adjustable through clk_set_parent
  */
 
-static u8 clk_mux_get_parent(struct clk_hw *hw)
+int clk_mux_val_to_index(struct clk_hw *hw, u32 *table, unsigned int flags,
+			 unsigned int val)
 {
-	struct clk_mux *mux = to_clk_mux(hw);
 	int num_parents = clk_hw_get_num_parents(hw);
-	u32 val;
 
-	/*
-	 * FIXME need a mux-specific flag to determine if val is bitwise or numeric
-	 * e.g. sys_clkin_ck's clksel field is 3 bits wide, but ranges from 0x1
-	 * to 0x7 (index starts at one)
-	 * OTOH, pmd_trace_clk_mux_ck uses a separate bit for each clock, so
-	 * val = 0x4 really means "bit 2, index starts at bit 0"
-	 */
-	val = clk_readl(mux->reg) >> mux->shift;
-	val &= mux->mask;
-
-	if (mux->table) {
+	if (table) {
 		int i;
 
 		for (i = 0; i < num_parents; i++)
-			if (mux->table[i] == val)
+			if (table[i] == val)
 				return i;
 		return -EINVAL;
 	}
 
-	if (val && (mux->flags & CLK_MUX_INDEX_BIT))
+	if (val && (flags & CLK_MUX_INDEX_BIT))
 		val = ffs(val) - 1;
 
-	if (val && (mux->flags & CLK_MUX_INDEX_ONE))
+	if (val && (flags & CLK_MUX_INDEX_ONE))
 		val--;
 
 	if (val >= num_parents)
@@ -62,36 +51,58 @@ static u8 clk_mux_get_parent(struct clk_hw *hw)
 
 	return val;
 }
+EXPORT_SYMBOL_GPL(clk_mux_val_to_index);
 
-static int clk_mux_set_parent(struct clk_hw *hw, u8 index)
+unsigned int clk_mux_index_to_val(u32 *table, unsigned int flags, u8 index)
 {
-	struct clk_mux *mux = to_clk_mux(hw);
-	u32 val;
-	unsigned long flags = 0;
+	unsigned int val = index;
 
-	if (mux->table) {
-		index = mux->table[index];
+	if (table) {
+		val = table[index];
 	} else {
-		if (mux->flags & CLK_MUX_INDEX_BIT)
-			index = 1 << index;
+		if (flags & CLK_MUX_INDEX_BIT)
+			val = 1 << index;
 
-		if (mux->flags & CLK_MUX_INDEX_ONE)
-			index++;
+		if (flags & CLK_MUX_INDEX_ONE)
+			val++;
 	}
 
+	return val;
+}
+EXPORT_SYMBOL_GPL(clk_mux_index_to_val);
+
+static u8 clk_mux_get_parent(struct clk_hw *hw)
+{
+	struct clk_mux *mux = to_clk_mux(hw);
+	u32 val;
+
+	val = clk_readl(mux->reg) >> mux->shift;
+	val &= mux->mask;
+
+	return clk_mux_val_to_index(hw, mux->table, mux->flags, val);
+}
+
+static int clk_mux_set_parent(struct clk_hw *hw, u8 index)
+{
+	struct clk_mux *mux = to_clk_mux(hw);
+	u32 val = clk_mux_index_to_val(mux->table, mux->flags, index);
+	unsigned long flags = 0;
+	u32 reg;
+
 	if (mux->lock)
 		spin_lock_irqsave(mux->lock, flags);
 	else
 		__acquire(mux->lock);
 
 	if (mux->flags & CLK_MUX_HIWORD_MASK) {
-		val = mux->mask << (mux->shift + 16);
+		reg = mux->mask << (mux->shift + 16);
 	} else {
-		val = clk_readl(mux->reg);
-		val &= ~(mux->mask << mux->shift);
+		reg = clk_readl(mux->reg);
+		reg &= ~(mux->mask << mux->shift);
 	}
-	val |= index << mux->shift;
-	clk_writel(val, mux->reg);
+	val = val << mux->shift;
+	reg |= val;
+	clk_writel(reg, mux->reg);
 
 	if (mux->lock)
 		spin_unlock_irqrestore(mux->lock, flags);
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index d8ba26d03332..fe720d679c31 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -511,6 +511,10 @@ struct clk_hw *clk_hw_register_mux_table(struct device *dev, const char *name,
 		void __iomem *reg, u8 shift, u32 mask,
 		u8 clk_mux_flags, u32 *table, spinlock_t *lock);
 
+int clk_mux_val_to_index(struct clk_hw *hw, u32 *table, unsigned int flags,
+			 unsigned int val);
+unsigned int clk_mux_index_to_val(u32 *table, unsigned int flags, u8 index);
+
 void clk_unregister_mux(struct clk *clk);
 void clk_hw_unregister_mux(struct clk_hw *hw);
 
-- 
2.14.3

[PATCH v2 3/8] clk: fix determine rate error with pass-through clock

[Jerome Brunet]

If we try to determine the rate of a pass-through clock (a clock which
does not implement .round_rate() nor .determine_rate()),
clk_core_round_rate_nolock() will directly forward the call to the
parent clock. In the particular case where the pass-through actually
does not have a parent, clk_core_round_rate_nolock() will directly
return 0 with the requested rate still set to the initial request
structure. This is interpreted as if the rate could be exactly achieved
while it actually cannot be adjusted.

This become a real problem when this particular pass-through clock is
the parent of a mux with the flag CLK_SET_RATE_PARENT set. The
pass-through clock will always report an exact match, get picked and
finally error when the rate is actually getting set.

This is fixed by setting the rate inside the req to 0 when core is NULL
in clk_core_round_rate_nolock() (same as in __clk_determine_rate() when
hw is NULL)

Fixes: 0f6cc2b8e94d ("clk: rework calls to round and determine rate callbacks")
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/clk.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 0f686a9dac3e..a4b4e4d6df5e 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1125,8 +1125,10 @@ static int clk_core_round_rate_nolock(struct clk_core *core,
 {
 	lockdep_assert_held(&prepare_lock);
 
-	if (!core)
+	if (!core) {
+		req->rate = 0;
 		return 0;
+	}
 
 	clk_core_init_rate_req(core, req);
 
-- 
2.14.3

[PATCH v2 4/8] clk: migrate the count of orphaned clocks at init

[Jerome Brunet]

The orphan clocks reparents should migrate any existing count from the
orphan clock to its new acestor clocks, otherwise we may have
inconsistent counts in the tree and end-up with gated critical clocks

Assuming we have two clocks, A and B.
* Clock A has CLK_IS_CRITICAL flag set.
* Clock B is an ancestor of A which can gate. Clock B gate is left
  enabled by the bootloader.

Step 1: Clock A is registered. Since it is a critical clock, it is
enabled. The clock being still an orphan, no parent are enabled.

Step 2: Clock B is registered and reparented to clock A (potentially
through several other clocks). We are now in situation where the enable
count of clock A is 1 while the enable count of its ancestors is 0, which
is not good.

Step 3: in lateinit, clk_disable_unused() is called, the enable_count of
clock B being 0, clock B is gated and and critical clock A actually gets
disabled.

This situation was found while adding fdiv_clk gates to the meson8b
platform.  These clocks parent clk81 critical clock, which is the mother
of all peripheral clocks in this system. Because of the issue described
here, the system is crashing when clk_disable_unused() is called.

The situation is solved by reverting
commit f8f8f1d04494 ("clk: Don't touch hardware when reparenting during registration").
To avoid breaking again the situation described in this commit
description, enabling critical clock should be done before walking the
orphan list. This way, a parent critical clock may not be accidentally
disabled due to the CLK_OPS_PARENT_ENABLE mechanism.

Fixes: f8f8f1d04494 ("clk: Don't touch hardware when reparenting during registration")
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Dong Aisheng <aisheng.dong@nxp.com>
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/clk.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index a4b4e4d6df5e..cca05ea2c058 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2969,23 +2969,38 @@ static int __clk_core_init(struct clk_core *core)
 		rate = 0;
 	core->rate = core->req_rate = rate;
 
+	/*
+	 * Enable CLK_IS_CRITICAL clocks so newly added critical clocks
+	 * don't get accidentally disabled when walking the orphan tree and
+	 * reparenting clocks
+	 */
+	if (core->flags & CLK_IS_CRITICAL) {
+		unsigned long flags;
+
+		clk_core_prepare(core);
+
+		flags = clk_enable_lock();
+		clk_core_enable(core);
+		clk_enable_unlock(flags);
+	}
+
 	/*
 	 * walk the list of orphan clocks and reparent any that newly finds a
 	 * parent.
 	 */
 	hlist_for_each_entry_safe(orphan, tmp2, &clk_orphan_list, child_node) {
 		struct clk_core *parent = __clk_init_parent(orphan);
-		unsigned long flags;
 
 		/*
-		 * we could call __clk_set_parent, but that would result in a
-		 * redundant call to the .set_rate op, if it exists
+		 * We need to use __clk_set_parent_before() and _after() to
+		 * to properly migrate any prepare/enable count of the orphan
+		 * clock. This is important for CLK_IS_CRITICAL clocks, which
+		 * are enabled during init but might not have a parent yet.
 		 */
 		if (parent) {
 			/* update the clk tree topology */
-			flags = clk_enable_lock();
-			clk_reparent(orphan, parent);
-			clk_enable_unlock(flags);
+			__clk_set_parent_before(orphan, parent);
+			__clk_set_parent_after(orphan, parent, NULL);
 			__clk_recalc_accuracies(orphan);
 			__clk_recalc_rates(orphan, 0);
 		}
@@ -3002,16 +3017,6 @@ static int __clk_core_init(struct clk_core *core)
 	if (core->ops->init)
 		core->ops->init(core->hw);
 
-	if (core->flags & CLK_IS_CRITICAL) {
-		unsigned long flags;
-
-		clk_core_prepare(core);
-
-		flags = clk_enable_lock();
-		clk_core_enable(core);
-		clk_enable_unlock(flags);
-	}
-
 	kref_init(&core->ref);
 out:
 	clk_pm_runtime_put(core);
-- 
2.14.3

Re: [PATCH v2 4/8] clk: migrate the count of orphaned clocks at init

[Marek Szyprowski]

Hi Jerome,

On 2018-02-14 14:43, Jerome Brunet wrote:
> The orphan clocks reparents should migrate any existing count from the
> orphan clock to its new acestor clocks, otherwise we may have
> inconsistent counts in the tree and end-up with gated critical clocks
>
> Assuming we have two clocks, A and B.
> * Clock A has CLK_IS_CRITICAL flag set.
> * Clock B is an ancestor of A which can gate. Clock B gate is left
>    enabled by the bootloader.
>
> Step 1: Clock A is registered. Since it is a critical clock, it is
> enabled. The clock being still an orphan, no parent are enabled.
>
> Step 2: Clock B is registered and reparented to clock A (potentially
> through several other clocks). We are now in situation where the enable
> count of clock A is 1 while the enable count of its ancestors is 0, which
> is not good.
>
> Step 3: in lateinit, clk_disable_unused() is called, the enable_count of
> clock B being 0, clock B is gated and and critical clock A actually gets
> disabled.
>
> This situation was found while adding fdiv_clk gates to the meson8b
> platform.  These clocks parent clk81 critical clock, which is the mother
> of all peripheral clocks in this system. Because of the issue described
> here, the system is crashing when clk_disable_unused() is called.
>
> The situation is solved by reverting
> commit f8f8f1d04494 ("clk: Don't touch hardware when reparenting during registration").
> To avoid breaking again the situation described in this commit
> description, enabling critical clock should be done before walking the
> orphan list. This way, a parent critical clock may not be accidentally
> disabled due to the CLK_OPS_PARENT_ENABLE mechanism.
>
> Fixes: f8f8f1d04494 ("clk: Don't touch hardware when reparenting during registration")
> Cc: Stephen Boyd <sboyd@codeaurora.org>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Dong Aisheng <aisheng.dong@nxp.com>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

This patch fixes kernel oops on Exynos5422-based boards (Odroid XU3/XU4)
mentioned in the following threads:
https://patchwork.kernel.org/patch/10185357/
https://www.spinics.net/lists/linux-samsung-soc/msg61766.html

> ---
>   drivers/clk/clk.c | 37 +++++++++++++++++++++----------------
>   1 file changed, 21 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index a4b4e4d6df5e..cca05ea2c058 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2969,23 +2969,38 @@ static int __clk_core_init(struct clk_core *core)
>   		rate = 0;
>   	core->rate = core->req_rate = rate;
>   
> +	/*
> +	 * Enable CLK_IS_CRITICAL clocks so newly added critical clocks
> +	 * don't get accidentally disabled when walking the orphan tree and
> +	 * reparenting clocks
> +	 */
> +	if (core->flags & CLK_IS_CRITICAL) {
> +		unsigned long flags;
> +
> +		clk_core_prepare(core);
> +
> +		flags = clk_enable_lock();
> +		clk_core_enable(core);
> +		clk_enable_unlock(flags);
> +	}
> +
>   	/*
>   	 * walk the list of orphan clocks and reparent any that newly finds a
>   	 * parent.
>   	 */
>   	hlist_for_each_entry_safe(orphan, tmp2, &clk_orphan_list, child_node) {
>   		struct clk_core *parent = __clk_init_parent(orphan);
> -		unsigned long flags;
>   
>   		/*
> -		 * we could call __clk_set_parent, but that would result in a
> -		 * redundant call to the .set_rate op, if it exists
> +		 * We need to use __clk_set_parent_before() and _after() to
> +		 * to properly migrate any prepare/enable count of the orphan
> +		 * clock. This is important for CLK_IS_CRITICAL clocks, which
> +		 * are enabled during init but might not have a parent yet.
>   		 */
>   		if (parent) {
>   			/* update the clk tree topology */
> -			flags = clk_enable_lock();
> -			clk_reparent(orphan, parent);
> -			clk_enable_unlock(flags);
> +			__clk_set_parent_before(orphan, parent);
> +			__clk_set_parent_after(orphan, parent, NULL);
>   			__clk_recalc_accuracies(orphan);
>   			__clk_recalc_rates(orphan, 0);
>   		}
> @@ -3002,16 +3017,6 @@ static int __clk_core_init(struct clk_core *core)
>   	if (core->ops->init)
>   		core->ops->init(core->hw);
>   
> -	if (core->flags & CLK_IS_CRITICAL) {
> -		unsigned long flags;
> -
> -		clk_core_prepare(core);
> -
> -		flags = clk_enable_lock();
> -		clk_core_enable(core);
> -		clk_enable_unlock(flags);
> -	}
> -
>   	kref_init(&core->ref);
>   out:
>   	clk_pm_runtime_put(core);

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

Re: [PATCH v2 4/8] clk: migrate the count of orphaned clocks at init

[Marek Szyprowski]

Hi All,

On 2018-02-15 13:55, Marek Szyprowski wrote:
> Hi Jerome,
>
> On 2018-02-14 14:43, Jerome Brunet wrote:
>> The orphan clocks reparents should migrate any existing count from the
>> orphan clock to its new acestor clocks, otherwise we may have
>> inconsistent counts in the tree and end-up with gated critical clocks
>>
>> Assuming we have two clocks, A and B.
>> * Clock A has CLK_IS_CRITICAL flag set.
>> * Clock B is an ancestor of A which can gate. Clock B gate is left
>>    enabled by the bootloader.
>>
>> Step 1: Clock A is registered. Since it is a critical clock, it is
>> enabled. The clock being still an orphan, no parent are enabled.
>>
>> Step 2: Clock B is registered and reparented to clock A (potentially
>> through several other clocks). We are now in situation where the enable
>> count of clock A is 1 while the enable count of its ancestors is 0, 
>> which
>> is not good.
>>
>> Step 3: in lateinit, clk_disable_unused() is called, the enable_count of
>> clock B being 0, clock B is gated and and critical clock A actually gets
>> disabled.
>>
>> This situation was found while adding fdiv_clk gates to the meson8b
>> platform.  These clocks parent clk81 critical clock, which is the mother
>> of all peripheral clocks in this system. Because of the issue described
>> here, the system is crashing when clk_disable_unused() is called.
>>
>> The situation is solved by reverting
>> commit f8f8f1d04494 ("clk: Don't touch hardware when reparenting 
>> during registration").
>> To avoid breaking again the situation described in this commit
>> description, enabling critical clock should be done before walking the
>> orphan list. This way, a parent critical clock may not be accidentally
>> disabled due to the CLK_OPS_PARENT_ENABLE mechanism.
>>
>> Fixes: f8f8f1d04494 ("clk: Don't touch hardware when reparenting 
>> during registration")
>> Cc: Stephen Boyd <sboyd@codeaurora.org>
>> Cc: Shawn Guo <shawnguo@kernel.org>
>> Cc: Dong Aisheng <aisheng.dong@nxp.com>
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>
> This patch fixes kernel oops on Exynos5422-based boards (Odroid XU3/XU4)
> mentioned in the following threads:
> https://patchwork.kernel.org/patch/10185357/
> https://www.spinics.net/lists/linux-samsung-soc/msg61766.html

Stephen, Michael: v4.16-rc4 is out today and this regression (which got 
merged
on 22 Dec 2017) is still not fixed yet...

Is there a chance to get it into final v4.16 release?

>> ---
>>   drivers/clk/clk.c | 37 +++++++++++++++++++++----------------
>>   1 file changed, 21 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index a4b4e4d6df5e..cca05ea2c058 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -2969,23 +2969,38 @@ static int __clk_core_init(struct clk_core 
>> *core)
>>           rate = 0;
>>       core->rate = core->req_rate = rate;
>>   +    /*
>> +     * Enable CLK_IS_CRITICAL clocks so newly added critical clocks
>> +     * don't get accidentally disabled when walking the orphan tree and
>> +     * reparenting clocks
>> +     */
>> +    if (core->flags & CLK_IS_CRITICAL) {
>> +        unsigned long flags;
>> +
>> +        clk_core_prepare(core);
>> +
>> +        flags = clk_enable_lock();
>> +        clk_core_enable(core);
>> +        clk_enable_unlock(flags);
>> +    }
>> +
>>       /*
>>        * walk the list of orphan clocks and reparent any that newly 
>> finds a
>>        * parent.
>>        */
>>       hlist_for_each_entry_safe(orphan, tmp2, &clk_orphan_list, 
>> child_node) {
>>           struct clk_core *parent = __clk_init_parent(orphan);
>> -        unsigned long flags;
>>             /*
>> -         * we could call __clk_set_parent, but that would result in a
>> -         * redundant call to the .set_rate op, if it exists
>> +         * We need to use __clk_set_parent_before() and _after() to
>> +         * to properly migrate any prepare/enable count of the orphan
>> +         * clock. This is important for CLK_IS_CRITICAL clocks, which
>> +         * are enabled during init but might not have a parent yet.
>>            */
>>           if (parent) {
>>               /* update the clk tree topology */
>> -            flags = clk_enable_lock();
>> -            clk_reparent(orphan, parent);
>> -            clk_enable_unlock(flags);
>> +            __clk_set_parent_before(orphan, parent);
>> +            __clk_set_parent_after(orphan, parent, NULL);
>>               __clk_recalc_accuracies(orphan);
>>               __clk_recalc_rates(orphan, 0);
>>           }
>> @@ -3002,16 +3017,6 @@ static int __clk_core_init(struct clk_core *core)
>>       if (core->ops->init)
>>           core->ops->init(core->hw);
>>   -    if (core->flags & CLK_IS_CRITICAL) {
>> -        unsigned long flags;
>> -
>> -        clk_core_prepare(core);
>> -
>> -        flags = clk_enable_lock();
>> -        clk_core_enable(core);
>> -        clk_enable_unlock(flags);
>> -    }
>> -
>>       kref_init(&core->ref);
>>   out:
>>       clk_pm_runtime_put(core);
>

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

Re: [PATCH v2 4/8] clk: migrate the count of orphaned clocks at init

[Heiko Stübner]

Am Mittwoch, 14. Februar 2018, 14:43:36 CET schrieb Jerome Brunet:
> The orphan clocks reparents should migrate any existing count from the
> orphan clock to its new acestor clocks, otherwise we may have
> inconsistent counts in the tree and end-up with gated critical clocks
> 
> Assuming we have two clocks, A and B.
> * Clock A has CLK_IS_CRITICAL flag set.
> * Clock B is an ancestor of A which can gate. Clock B gate is left
>   enabled by the bootloader.
> 
> Step 1: Clock A is registered. Since it is a critical clock, it is
> enabled. The clock being still an orphan, no parent are enabled.
> 
> Step 2: Clock B is registered and reparented to clock A (potentially
> through several other clocks). We are now in situation where the enable
> count of clock A is 1 while the enable count of its ancestors is 0, which
> is not good.
> 
> Step 3: in lateinit, clk_disable_unused() is called, the enable_count of
> clock B being 0, clock B is gated and and critical clock A actually gets
> disabled.
> 
> This situation was found while adding fdiv_clk gates to the meson8b
> platform.  These clocks parent clk81 critical clock, which is the mother
> of all peripheral clocks in this system. Because of the issue described
> here, the system is crashing when clk_disable_unused() is called.
> 
> The situation is solved by reverting
> commit f8f8f1d04494 ("clk: Don't touch hardware when reparenting during
> registration"). To avoid breaking again the situation described in this
> commit
> description, enabling critical clock should be done before walking the
> orphan list. This way, a parent critical clock may not be accidentally
> disabled due to the CLK_OPS_PARENT_ENABLE mechanism.
> 
> Fixes: f8f8f1d04494 ("clk: Don't touch hardware when reparenting during
> registration") Cc: Stephen Boyd <sboyd@codeaurora.org>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Dong Aisheng <aisheng.dong@nxp.com>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>

On a rk3288-veyron Chromebook
Tested-by: Heiko Stuebner <heiko@sntech.de>

This made the hdmi on the machine work again with 4.16-rc1 .
(hdmi-cec clock is sourced from the i2c connected pmic which provides
the 32kHz clock needed)

So it would be really cool if this could make it into 4.16-rc :-)


Heiko

Re: [PATCH v2 4/8] clk: migrate the count of orphaned clocks at init

[Michael Turquette]

Excerpts from Heiko St=C3=BCbner's message of February 15, 2018 1:01 pm:
> Am Mittwoch, 14. Februar 2018, 14:43:36 CET schrieb Jerome Brunet:
>> The orphan clocks reparents should migrate any existing count from the
>> orphan clock to its new acestor clocks, otherwise we may have
>> inconsistent counts in the tree and end-up with gated critical clocks
>>=20
>> Assuming we have two clocks, A and B.
>> * Clock A has CLK_IS_CRITICAL flag set.
>> * Clock B is an ancestor of A which can gate. Clock B gate is left
>>   enabled by the bootloader.
>>=20
>> Step 1: Clock A is registered. Since it is a critical clock, it is
>> enabled. The clock being still an orphan, no parent are enabled.
>>=20
>> Step 2: Clock B is registered and reparented to clock A (potentially
>> through several other clocks). We are now in situation where the enable
>> count of clock A is 1 while the enable count of its ancestors is 0, whic=
h
>> is not good.
>>=20
>> Step 3: in lateinit, clk_disable_unused() is called, the enable_count of
>> clock B being 0, clock B is gated and and critical clock A actually gets
>> disabled.
>>=20
>> This situation was found while adding fdiv_clk gates to the meson8b
>> platform.  These clocks parent clk81 critical clock, which is the mother
>> of all peripheral clocks in this system. Because of the issue described
>> here, the system is crashing when clk_disable_unused() is called.
>>=20
>> The situation is solved by reverting
>> commit f8f8f1d04494 ("clk: Don't touch hardware when reparenting during
>> registration"). To avoid breaking again the situation described in this
>> commit
>> description, enabling critical clock should be done before walking the
>> orphan list. This way, a parent critical clock may not be accidentally
>> disabled due to the CLK_OPS_PARENT_ENABLE mechanism.
>>=20
>> Fixes: f8f8f1d04494 ("clk: Don't touch hardware when reparenting during
>> registration") Cc: Stephen Boyd <sboyd@codeaurora.org>
>> Cc: Shawn Guo <shawnguo@kernel.org>
>> Cc: Dong Aisheng <aisheng.dong@nxp.com>
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>=20
> On a rk3288-veyron Chromebook
> Tested-by: Heiko Stuebner <heiko@sntech.de>
>=20
> This made the hdmi on the machine work again with 4.16-rc1 .
> (hdmi-cec clock is sourced from the i2c connected pmic which provides
> the 32kHz clock needed)
>=20
> So it would be really cool if this could make it into 4.16-rc :-)

I separated this patch out into clk-fixes to be set for -rc5 or -rc6.

Regards,
Mike

>=20
>=20
> Heiko
>=20
=

[PATCH v2 5/8] clk: call the clock init() callback before any other ops callback

[Jerome Brunet]

Some clocks may need to initialize things, whatever it is, before
being able to properly operate. Move the .init() call before any
other callback, such recalc_rate() or get_phase(), so the clock
is properly setup before being used.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---

Stephen, Mike,

This change is addressing a problem I have with clock driver I'm working
on. This driver needs the .init() callback to run first is order to answer
correctly .get_phase(). It does not address an existing issue, not one
that I'm aware of at least. This is why I did not put a Fixes tag.

In any case, it makes sense to run init before running anything else.

 drivers/clk/clk.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index cca05ea2c058..9d56be6ead39 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2929,6 +2929,17 @@ static int __clk_core_init(struct clk_core *core)
 		core->orphan = true;
 	}
 
+	/*
+	 * optional platform-specific magic
+	 *
+	 * The .init callback is not used by any of the basic clock types, but
+	 * exists for weird hardware that must perform initialization magic.
+	 * Please consider other ways of solving initialization problems before
+	 * using this callback, as its use is discouraged.
+	 */
+	if (core->ops->init)
+		core->ops->init(core->hw);
+
 	/*
 	 * Set clk's accuracy.  The preferred method is to use
 	 * .recalc_accuracy. For simple clocks and lazy developers the default
@@ -3006,17 +3017,6 @@ static int __clk_core_init(struct clk_core *core)
 		}
 	}
 
-	/*
-	 * optional platform-specific magic
-	 *
-	 * The .init callback is not used by any of the basic clock types, but
-	 * exists for weird hardware that must perform initialization magic.
-	 * Please consider other ways of solving initialization problems before
-	 * using this callback, as its use is discouraged.
-	 */
-	if (core->ops->init)
-		core->ops->init(core->hw);
-
 	kref_init(&core->ref);
 out:
 	clk_pm_runtime_put(core);
-- 
2.14.3

[PATCH v2 6/8] clk: fix mux clock documentation

[Jerome Brunet]

The mux documentation mentions the non-existing parameter width instead
of mask, so just sed this.

The table field is missing in the documentation of clk_mux.
Add a small blurb explaining what it is

Fixes: 9d9f78ed9af0 ("clk: basic clock hardware types")
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 include/linux/clk-provider.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index fe720d679c31..cb18526d69cb 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -450,8 +450,9 @@ void clk_hw_unregister_divider(struct clk_hw *hw);
  *
  * @hw:		handle between common and hardware-specific interfaces
  * @reg:	register controlling multiplexer
+ * @table:	array of register values corresponding to the parent index
  * @shift:	shift to multiplexer bit field
- * @width:	width of mutliplexer bit field
+ * @mask:	mask of mutliplexer bit field
  * @flags:	hardware-specific flags
  * @lock:	register lock
  *
-- 
2.14.3

[PATCH v2 7/8] clk: divider: read-only divider can propagate rate change

[Jerome Brunet]

When a divider clock has CLK_DIVIDER_READ_ONLY set, it means that the
register shall be left un-touched, but it does not mean the clock
should stop rate propagation if CLK_SET_RATE_PARENT is set

This is properly handled in qcom clk-regmap-divider but it was not in
the generic divider

To fix this situation, introduce a new helper function
divider_ro_round_rate, on the same model as divider_round_rate.

Fixes: e6d5e7d90be9 ("clk-divider: Fix READ_ONLY when divider > 1")
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/clk-divider.c    | 36 ++++++++++++++++++++++++++++++------
 include/linux/clk-provider.h | 15 +++++++++++++++
 2 files changed, 45 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index 3c98d2650fa3..b6234a5da12d 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -342,19 +342,43 @@ long divider_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,
 }
 EXPORT_SYMBOL_GPL(divider_round_rate_parent);
 
+long divider_ro_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,
+				  unsigned long rate, unsigned long *prate,
+				  const struct clk_div_table *table, u8 width,
+				  unsigned long flags, unsigned int val)
+{
+	int div;
+
+	div = _get_div(table, val, flags, width);
+
+	/* Even a read-only clock can propagate a rate change */
+	if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {
+		if (!parent)
+			return -EINVAL;
+
+		*prate = clk_hw_round_rate(parent, rate * div);
+	}
+
+	return DIV_ROUND_UP_ULL((u64)*prate, div);
+}
+EXPORT_SYMBOL_GPL(divider_ro_round_rate_parent);
+
+
 static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,
 				unsigned long *prate)
 {
 	struct clk_divider *divider = to_clk_divider(hw);
-	int bestdiv;
 
 	/* if read only, just return current value */
 	if (divider->flags & CLK_DIVIDER_READ_ONLY) {
-		bestdiv = clk_readl(divider->reg) >> divider->shift;
-		bestdiv &= clk_div_mask(divider->width);
-		bestdiv = _get_div(divider->table, bestdiv, divider->flags,
-			divider->width);
-		return DIV_ROUND_UP_ULL((u64)*prate, bestdiv);
+		u32 val;
+
+		val = clk_readl(divider->reg) >> divider->shift;
+		val &= clk_div_mask(divider->width);
+
+		return divider_ro_round_rate(hw, rate, prate, divider->table,
+					     divider->width, divider->flags,
+					     val);
 	}
 
 	return divider_round_rate(hw, rate, prate, divider->table,
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index cb18526d69cb..210a890008f9 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -420,6 +420,10 @@ long divider_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,
 			       unsigned long rate, unsigned long *prate,
 			       const struct clk_div_table *table,
 			       u8 width, unsigned long flags);
+long divider_ro_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,
+				  unsigned long rate, unsigned long *prate,
+				  const struct clk_div_table *table, u8 width,
+				  unsigned long flags, unsigned int val);
 int divider_get_val(unsigned long rate, unsigned long parent_rate,
 		const struct clk_div_table *table, u8 width,
 		unsigned long flags);
@@ -780,6 +784,17 @@ static inline long divider_round_rate(struct clk_hw *hw, unsigned long rate,
 					 rate, prate, table, width, flags);
 }
 
+static inline long divider_ro_round_rate(struct clk_hw *hw, unsigned long rate,
+					 unsigned long *prate,
+					 const struct clk_div_table *table,
+					 u8 width, unsigned long flags,
+					 unsigned int val)
+{
+	return divider_ro_round_rate_parent(hw, clk_hw_get_parent(hw),
+					    rate, prate, table, width, flags,
+					    val);
+}
+
 /*
  * FIXME clock api without lock protection
  */
-- 
2.14.3

Re: [v2,7/8] clk: divider: read-only divider can propagate rate change

[David Lechner]

On 02/14/2018 07:43 AM, Jerome Brunet wrote:
> When a divider clock has CLK_DIVIDER_READ_ONLY set, it means that the
> register shall be left un-touched, but it does not mean the clock
> should stop rate propagation if CLK_SET_RATE_PARENT is set
> 
> This is properly handled in qcom clk-regmap-divider but it was not in
> the generic divider
> 
> To fix this situation, introduce a new helper function
> divider_ro_round_rate, on the same model as divider_round_rate.
> 
> Fixes: e6d5e7d90be9 ("clk-divider: Fix READ_ONLY when divider > 1")
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---

Working for me with the davinci clk drivers I am developing.

Tested-By: David Lechner <david@lechnology.com>

[PATCH v2 8/8] clk: qcom: use divider_ro_round_rate helper

[Jerome Brunet]

There is now an helper function to round the rate when the
divider is read-only. Let's use it

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/qcom/clk-regmap-divider.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/clk/qcom/clk-regmap-divider.c b/drivers/clk/qcom/clk-regmap-divider.c
index 4e9b8c2c8980..1ee75a5e93f4 100644
--- a/drivers/clk/qcom/clk-regmap-divider.c
+++ b/drivers/clk/qcom/clk-regmap-divider.c
@@ -28,22 +28,14 @@ static long div_round_ro_rate(struct clk_hw *hw, unsigned long rate,
 {
 	struct clk_regmap_div *divider = to_clk_regmap_div(hw);
 	struct clk_regmap *clkr = &divider->clkr;
-	u32 div;
-	struct clk_hw *hw_parent = clk_hw_get_parent(hw);
-
-	regmap_read(clkr->regmap, divider->reg, &div);
-	div >>= divider->shift;
-	div &= BIT(divider->width) - 1;
-	div += 1;
-
-	if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {
-		if (!hw_parent)
-			return -EINVAL;
+	u32 val;
 
-		*prate = clk_hw_round_rate(hw_parent, rate * div);
-	}
+	regmap_read(clkr->regmap, divider->reg, &val);
+	val >>= divider->shift;
+	val &= BIT(divider->width) - 1;
 
-	return DIV_ROUND_UP_ULL((u64)*prate, div);
+	return divider_ro_round_rate(hw, rate, prate, NULL, divider->width,
+				     CLK_DIVIDER_ROUND_CLOSEST, val);
 }
 
 static long div_round_rate(struct clk_hw *hw, unsigned long rate,
-- 
2.14.3

Re: [PATCH v2 0/8] clk: helpers and fixes

[Michael Turquette]

Excerpts from Jerome Brunet's message of February 14, 2018 5:43 am:
> This changset is consist of various patches I have recently sent
> for the clock framework. They are gathered here for your convinience.
>=20
> The first two changes exports helpers of the generic clocks (divider and
> mux). The goal is to avoid code duplication when writing clock driver
> derived from these generic clock drivers.
>=20
> The rest fixes various issues found in the clock framework while working =
on
> a rework of meson's clock controllers [0]

Applied to clk-helpers towards v4.17.

Regards,
Mike

>=20
> Changes since the v1s:
>  * dropped lpc32xx divider read-only patches as requested by
>    Vladimir Zapolskiy [2]
>  * Squashed generic divider read-only patches
>  * Squashed mux documentations patches
>=20
> [0]: https://lkml.kernel.org/r/20180212145846.19380-1-jbrunet@baylibre.co=
m
> [2]: https://lkml.kernel.org/r/54782a8f-9735-a574-d8e1-7ff3e9173229@mleia=
.com
>=20
> Jerome Brunet (8):
>   clk: divider: export clk_div_mask() helper
>   clk: mux: add helper function for index/value translation
>   clk: fix determine rate error with pass-through clock
>   clk: migrate the count of orphaned clocks at init
>   clk: call the clock init() callback before any other ops callback
>   clk: fix mux clock documentation
>   clk: divider: read-only divider can propagate rate change
>   clk: qcom: use divider_ro_round_rate helper
>=20
>  drivers/clk/clk-divider.c             | 58 ++++++++++++++++++---------
>  drivers/clk/clk-mux.c                 | 75 ++++++++++++++++++++---------=
------
>  drivers/clk/clk.c                     | 63 ++++++++++++++++-------------
>  drivers/clk/qcom/clk-regmap-divider.c | 20 +++-------
>  include/linux/clk-provider.h          | 23 ++++++++++-
>  5 files changed, 146 insertions(+), 93 deletions(-)
>=20
> --=20
> 2.14.3
>=20
>=20
=