On Thu, Dec 21, 2017 at 05:24:01PM -0800, Stephen Boyd wrote:

On 12/20, Dong Aisheng wrote:

quoted

On Thu, Nov 02, 2017 at 12:50:39AM -0700, Stephen Boyd wrote:

quoted

On 07/13, Dong Aisheng wrote:

quoted

diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index 9bb472c..55f8c41 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c

@@ -123,6 +123,9 @@ unsigned long divider_recalc_rate(struct clk_hw *hw, unsigned long parent_rate,
 	struct clk_divider *divider = to_clk_divider(hw);
 	unsigned int div;
 
+	if (flags & CLK_DIVIDER_ZERO_GATE && !val)
+		return 0;
+
 	div = _get_div(table, val, flags, divider->width);
 	if (!div) {
 		WARN(!(flags & CLK_DIVIDER_ALLOW_ZERO),

@@ -141,8 +144,13 @@ static unsigned long clk_divider_recalc_rate(struct clk_hw *hw,
 	struct clk_divider *divider = to_clk_divider(hw);
 	unsigned int val;
 
-	val = clk_readl(divider->reg) >> divider->shift;
-	val &= div_mask(divider->width);
+	if ((divider->flags & CLK_DIVIDER_ZERO_GATE) &&
+	    !clk_hw_is_enabled(hw)) {

This seems racy. Are we holding the register lock here?

Would you please clarify what type of racy you mean?

I mean a race between clk_enable() and clk_set_rate(). A
clk_enable() can happen while a rate change is going on, and then
the clk_hw_is_enabled() check here would be racing, unless this
driver specifically tries to prevent that from happening by
holding a spinlock somewhere.

Will this race cause real problems as clk_divider_is_enabled is only
a register read?

And seems either clk_hw_is_enable or __clk_is_enabled is allowed
to be called by anywhere currently. So it may be none of calling
in set_rate or recalc_rate issue.

If they may be race with clk_enable/disable function, seems they may
be better protected by the core.

And i did see some clarify in Documentation/clk.txt:
"The enable lock is a spinlock and is held across calls to the .enable,
.disable and .is_enabled operations. Those operations are thus not allowed to
sleep, and calls to the clk_enable(), clk_disable() and clk_is_enabled() API
functions are allowed in atomic context."

Then how about do like below:

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 8a1860a..ce5fa96 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c

@@ -191,6 +191,7 @@ static bool clk_core_is_prepared(struct clk_core *core)
 
 static bool clk_core_is_enabled(struct clk_core *core)
 {
+       unsigned long flags;
        bool ret = false;
 
        /*

@@ -218,7 +219,10 @@ static bool clk_core_is_enabled(struct clk_core *core)
                }
        }
 
+       flags = clk_enable_lock();
        ret = core->ops->is_enabled(core->hw);
+       clk_enable_unlock(flags);
+
 done:
        clk_pm_runtime_put(core);

quoted

Currently it only protects register write between set_rate and enable/disable,
and other register read are not protected.
e.g. in recalc_rate and is_enabled.

If you're holding some lock that is used to protect the register
writes and also the clk from getting enabled/disabled during a
rate change then it's fine.

Yes, all possible register write are protected.

Regards
Dong Aisheng

quoted

And i did see similar users, e.g.
drivers/clk/sunxi-ng/ccu_mult.c

Sure. Those could also be broken. I'm not sure.

quoted

Should we still need protect them here?

quoted

quoted

+		val = divider->cached_val;
+	} else {
+		val = clk_readl(divider->reg) >> divider->shift;
+		val &= div_mask(divider->width);
+	}
 
 	return divider_recalc_rate(hw, parent_rate, val, divider->table,
 				   divider->flags);

@@ -392,6 +400,12 @@ static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
 	value = divider_get_val(rate, parent_rate, divider->table,
 				divider->width, divider->flags);
 
+	if ((divider->flags & CLK_DIVIDER_ZERO_GATE) &&
+	    !clk_hw_is_enabled(hw)) {

Same racy comment here.

quoted

+		divider->cached_val = value;
+		return 0;
+	}
+
 	if (divider->lock)
 		spin_lock_irqsave(divider->lock, flags);
 	else

@@ -414,10 +428,85 @@ static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
 	return 0;
 }
 
+static int clk_divider_enable(struct clk_hw *hw)
+{
+	struct clk_divider *divider = to_clk_divider(hw);
+	unsigned long flags = 0;
+	u32 val;
+
+	if (!(divider->flags & CLK_DIVIDER_ZERO_GATE))
+		return 0;

This is not good. We will always jump to these functions on
enable/disable for a divider although 99.9% of all dividers that
exist won't need to run this code at all.

I absolutely understand this concern.

quoted

Can you please move this logic into your own divider
implementation? The flag can be added to the generic layer if
necessary but I'd prefer to see this logic kept in the driver
that uses it. If we get more than one driver doing the cached
divider thing then we can think about moving it to the more
generic place like here, but for now we should be able to keep
this contained away from the basic types and handled by the
quirky driver that needs it.

If only for above issue, how about invent a clk_divider_gate_ops
to separate the users of normal divider and zero gate divider:

diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index 4ed516c..b51f3f9 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c

@@ -125,6 +125,9 @@ unsigned long divider_recalc_rate(struct clk_hw *hw, unsigned long parent_rate,
 
 	div = _get_div(table, val, flags, divider->width);
 	if (!div) {
+		if (flags & CLK_DIVIDER_ZERO_GATE)
+			return 0;
+
 		WARN(!(flags & CLK_DIVIDER_ALLOW_ZERO),
 			"%s: Zero divisor and CLK_DIVIDER_ALLOW_ZERO not set\n",
 			clk_hw_get_name(hw));

@@ -148,6 +151,23 @@ static unsigned long clk_divider_recalc_rate(struct clk_hw *hw,
 				   divider->flags);
 }
 
+static unsigned long clk_divider_gate_recalc_rate(struct clk_hw *hw,
+						  unsigned long parent_rate)
+{
+	struct clk_divider *divider = to_clk_divider(hw);
+	unsigned int val;
+
+	if (!clk_hw_is_enabled(hw)) {
+		val = divider->cached_val;
+	} else {
+		val = clk_readl(divider->reg) >> divider->shift;
+		val &= div_mask(divider->width);
+	}
+
+	return divider_recalc_rate(hw, parent_rate, val, divider->table,
+				   divider->flags);
+}
+
 static bool _is_valid_table_div(const struct clk_div_table *table,
 							 unsigned int div)
 {

@@ -416,6 +436,89 @@ static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
 	return 0;
 }
 
+static int clk_divider_gate_set_rate(struct clk_hw *hw, unsigned long rate,
+				unsigned long parent_rate)
+{
+	struct clk_divider *divider = to_clk_divider(hw);
+	int value;
+
+	if (!clk_hw_is_enabled(hw)) {
+		value = divider_get_val(rate, parent_rate, divider->table,
+					divider->width, divider->flags);
+		if (value < 0)
+			return value;
+
+		divider->cached_val = value;
+
+		return 0;
+	}
+
+	return clk_divider_set_rate(hw, rate, parent_rate);
+}
+
+static int clk_divider_enable(struct clk_hw *hw)
+{
+	struct clk_divider *divider = to_clk_divider(hw);
+	unsigned long uninitialized_var(flags);
+	u32 val;
+
+	if (!divider->cached_val) {
+		pr_err("%s: no valid preset rate\n", clk_hw_get_name(hw));
+		return -EINVAL;
+	}
+
+	if (divider->lock)
+		spin_lock_irqsave(divider->lock, flags);
+	else
+		__acquire(divider->lock);
+
+	/* restore div val */
+	val = clk_readl(divider->reg);
+	val |= divider->cached_val << divider->shift;
+	clk_writel(val, divider->reg);
+
+	if (divider->lock)
+		spin_unlock_irqrestore(divider->lock, flags);
+	else
+		__release(divider->lock);
+
+	return 0;
+}
+
+static void clk_divider_disable(struct clk_hw *hw)
+{
+	struct clk_divider *divider = to_clk_divider(hw);
+	unsigned long uninitialized_var(flags);
+	u32 val;
+
+	if (divider->lock)
+		spin_lock_irqsave(divider->lock, flags);
+	else
+		__acquire(divider->lock);
+
+	/* store the current div val */
+	val = clk_readl(divider->reg) >> divider->shift;
+	val &= div_mask(divider->width);
+	divider->cached_val = val;
+	clk_writel(0, divider->reg);
+
+	if (divider->lock)
+		spin_unlock_irqrestore(divider->lock, flags);
+	else
+		__release(divider->lock);
+}
+
+static int clk_divider_is_enabled(struct clk_hw *hw)
+{
+	struct clk_divider *divider = to_clk_divider(hw);
+	u32 val;
+
+	val = clk_readl(divider->reg) >> divider->shift;
+	val &= div_mask(divider->width);
+
+	return val ? 1 : 0;
+}
+
 const struct clk_ops clk_divider_ops = {
 	.recalc_rate = clk_divider_recalc_rate,
 	.round_rate = clk_divider_round_rate,

@@ -423,6 +526,16 @@ const struct clk_ops clk_divider_ops = {
 };
 EXPORT_SYMBOL_GPL(clk_divider_ops);
 
+const struct clk_ops clk_divider_gate_ops = {
+	.recalc_rate = clk_divider_gate_recalc_rate,
+	.round_rate = clk_divider_round_rate,
+	.set_rate = clk_divider_gate_set_rate,
+	.enable = clk_divider_enable,
+	.disable = clk_divider_disable,
+	.is_enabled = clk_divider_is_enabled,
+};
+EXPORT_SYMBOL_GPL(clk_divider_gate_ops);
+
 const struct clk_ops clk_divider_ro_ops = {
 	.recalc_rate = clk_divider_recalc_rate,
 	.round_rate = clk_divider_round_rate,

@@ -438,6 +551,7 @@ static struct clk_hw *_register_divider(struct device *dev, const char *name,
 	struct clk_divider *div;
 	struct clk_hw *hw;
 	struct clk_init_data init;
+	u32 val;
 	int ret;
 
 	if (clk_divider_flags & CLK_DIVIDER_HIWORD_MASK) {

@@ -455,6 +569,8 @@ static struct clk_hw *_register_divider(struct device *dev, const char *name,
 	init.name = name;
 	if (clk_divider_flags & CLK_DIVIDER_READ_ONLY)
 		init.ops = &clk_divider_ro_ops;
+	else if (clk_divider_flags & CLK_DIVIDER_ZERO_GATE)
+		init.ops = &clk_divider_gate_ops;
 	else
 		init.ops = &clk_divider_ops;
 	init.flags = flags | CLK_IS_BASIC;

@@ -470,6 +586,12 @@ static struct clk_hw *_register_divider(struct device *dev, const char *name,
 	div->hw.init = &init;
 	div->table = table;
 
+	if (div->flags & CLK_DIVIDER_ZERO_GATE) {
+		val = clk_readl(reg) >> shift;
+		val &= div_mask(width);
+		div->cached_val = val;
+	}
+
 	/* register the clock */
 	hw = &div->hw;
 	ret = clk_hw_register(dev, hw);

diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 7c925e6..5f33b73 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h

@@ -358,6 +358,7 @@ struct clk_div_table {
  * @shift:	shift to the divider bit field
  * @width:	width of the divider bit field
  * @table:	array of value/divider pairs, last entry should have div = 0
+ * @cached_val: cached div hw value used for CLK_DIVIDER_ZERO_GATE
  * @lock:	register lock
  *
  * Clock with an adjustable divider affecting its output frequency.  Implements

@@ -386,6 +387,12 @@ struct clk_div_table {
  * CLK_DIVIDER_MAX_AT_ZERO - For dividers which are like CLK_DIVIDER_ONE_BASED
  *	except when the value read from the register is zero, the divisor is
  *	2^width of the field.
+ * CLK_DIVIDER_ZERO_GATE - For dividers which are like CLK_DIVIDER_ONE_BASED
+ *	when the value read from the register is zero, it means the divisor
+ *	is gated. For this case, the cached_val will be used to store the
+ *	intermediate div for the normal rate operation, like set_rate/get_rate/
+ *	recalc_rate. When the divider is ungated, the driver will actually
+ *	program the hardware to have the requested divider value.
  */
 struct clk_divider {
 	struct clk_hw	hw;

@@ -394,6 +401,7 @@ struct clk_divider {
 	u8		width;
 	u8		flags;
 	const struct clk_div_table	*table;
+	u32		cached_val;
 	spinlock_t	*lock;
 };
 

@@ -406,6 +414,7 @@ struct clk_divider {
 #define CLK_DIVIDER_ROUND_CLOSEST	BIT(4)
 #define CLK_DIVIDER_READ_ONLY		BIT(5)
 #define CLK_DIVIDER_MAX_AT_ZERO		BIT(6)
+#define CLK_DIVIDER_ZERO_GATE		BIT(7)
 
 extern const struct clk_ops clk_divider_ops;
 extern const struct clk_ops clk_divider_ro_ops;

Anyway, if you still think it's not proper, i can put it in platform
driver as you wish, just in the cost of a few duplicated codes.

Ok. Keeping it in the basic types but split into different ops
path looks good.

quoted

quoted

quoted

+
+	if (!divider->cached_val) {
+		pr_err("%s: no valid preset rate\n", clk_hw_get_name(hw));
+		return -EINVAL;
+	}
+
+	if (divider->lock)
+		spin_lock_irqsave(divider->lock, flags);
+	else
+		__acquire(divider->lock);
+
+	/* restore div val */
+	val = clk_readl(divider->reg);
+	val |= divider->cached_val << divider->shift;
+	clk_writel(val, divider->reg);
+
+	if (divider->lock)
+		spin_unlock_irqrestore(divider->lock, flags);
+	else
+		__release(divider->lock);
+
+	return 0;
+}
+
+static void clk_divider_disable(struct clk_hw *hw)
+{
+	struct clk_divider *divider = to_clk_divider(hw);
+	unsigned long flags = 0;
+	u32 val;
+
+	if (!(divider->flags & CLK_DIVIDER_ZERO_GATE))
+		return;
+
+	if (divider->lock)
+		spin_lock_irqsave(divider->lock, flags);
+	else
+		__acquire(divider->lock);
+
+	/* store the current div val */
+	val = clk_readl(divider->reg) >> divider->shift;
+	val &= div_mask(divider->width);
+	divider->cached_val = val;
+	clk_writel(0, divider->reg);
+
+	if (divider->lock)
+		spin_unlock_irqrestore(divider->lock, flags);
+	else
+		__release(divider->lock);
+}
+
+static int clk_divider_is_enabled(struct clk_hw *hw)
+{
+	struct clk_divider *divider = to_clk_divider(hw);
+	u32 val;
+
+	if (!(divider->flags & CLK_DIVIDER_ZERO_GATE))
+		return __clk_get_enable_count(hw->clk);

The plan was to delete this API once OMAP stopped using it.
clk_hw_is_enabled() doesn't work?

No, it did not work before because clk_hw_is_enabled will result
in the dead loop by calling .is_enabled() callback again.

That's why __clk_get_enable_count is used instead.

However, with above new patch method, this issue was gone.

Great!

quoted

quoted

quoted

+
+	val = clk_readl(divider->reg) >> divider->shift;
+	val &= div_mask(divider->width);
+
+	return val ? 1 : 0;
+}
+
 const struct clk_ops clk_divider_ops = {
 	.recalc_rate = clk_divider_recalc_rate,
 	.round_rate = clk_divider_round_rate,
 	.set_rate = clk_divider_set_rate,
+	.enable = clk_divider_enable,
+	.disable = clk_divider_disable,
+	.is_enabled = clk_divider_is_enabled,
 };
 EXPORT_SYMBOL_GPL(clk_divider_ops);
 

@@ -436,6 +525,7 @@ static struct clk_hw *_register_divider(struct device *dev, const char *name,
 	struct clk_divider *div;
 	struct clk_hw *hw;
 	struct clk_init_data init;
+	u32 val;
 	int ret;
 
 	if (clk_divider_flags & CLK_DIVIDER_HIWORD_MASK) {

@@ -468,6 +558,12 @@ static struct clk_hw *_register_divider(struct device *dev, const char *name,
 	div->hw.init = &init;
 	div->table = table;
 
+	if (div->flags & CLK_DIVIDER_ZERO_GATE) {
+		val = clk_readl(reg) >> shift;
+		val &= div_mask(width);
+		div->cached_val = val;
+	}

What if it isn't on? Setting cached_val to 0 is ok?

If it isn't on, then the cache_val should be 0.
And recalc_rate will catch this case and return 0 as there's
no proper pre-set rate.

Ok.

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