[PATCH 0/2] Fix clock rate in the rockchip_fractional_approximation()

[PATCH 0/2] Fix clock rate in the rockchip_fractional_approximation()

[Alexander Kochetkov]

Hello!

Here are two patches fixing issue in the rockchip_fractional_approximation().
rockchip_fractional_approximation() can select clock rate whose
value will violate clock limit settings (i.e. one configured with 
clk_set_max_rate() and clk_set_min_rate()).

rockchip_fractional_approximation() was introduced by commit 5d890c2df900
("clk: rockchip: add special approximation to fix up fractional clk's jitter") 
whose description states that setting denominator 20 times larger than
numerator will generate precise clock frequency. It's strange, because
on my custom rk3188-based board and on radxa rock I've observed strange
hardware issues. I2S, for example, sometimes doesn't setup correct
rate on external SCLK_I2S0. UART0, for example, started to receive '\0' 
characters instead of valid symbols, signals on UART_RX was good.

So, I use clk_set_max_rate() to limit max value of i2s0_pre, uart0_pre,
uart1_pre, uart2_pre, uart3_pre to value of aclk_cpu_pre. That fixes
strange I2S and UART issues for me. If that make sense, than I can
send the patch. But it will logically conflict with commit 5d890c2df900
("clk: rockchip: add special approximation to fix up fractional clk's jitter").

Alexander Kochetkov (2):
  clk: rename clk_core_get_boundaries() to clk_hw_get_boundaries() and
    expose
  clk: rockchip: limit clock rate in the
    rockchip_fractional_approximation()

 drivers/clk/clk.c            |   14 ++++++++------
 drivers/clk/rockchip/clk.c   |    7 +++++++
 include/linux/clk-provider.h |    2 ++
 3 files changed, 17 insertions(+), 6 deletions(-)

-- 
1.7.9.5

[PATCH 1/2] clk: rename clk_core_get_boundaries() to clk_hw_get_boundaries() and expose

[Alexander Kochetkov]

In order to provide a way to know clock limits to clock providers.

The patch is needed for fixing commit 5d890c2df900 ("clk: rockchip:
add special approximation to fix up fractional clk's jitter").
Custom approximation function introduced by the patch, can
select frequency rate larger than one configured using
clk_set_max_rate().

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
---
 drivers/clk/clk.c            |   14 ++++++++------
 include/linux/clk-provider.h |    2 ++
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index c8d83ac..8943aac 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -421,10 +421,11 @@ struct clk *__clk_lookup(const char *name)
 	return !core ? NULL : core->hw->clk;
 }
 
-static void clk_core_get_boundaries(struct clk_core *core,
-				    unsigned long *min_rate,
-				    unsigned long *max_rate)
+void clk_hw_get_boundaries(struct clk_hw *hw,
+			   unsigned long *min_rate,
+			   unsigned long *max_rate)
 {
+	struct clk_core *core = hw->core;
 	struct clk *clk_user;
 
 	*min_rate = core->min_rate;
@@ -436,6 +437,7 @@ static void clk_core_get_boundaries(struct clk_core *core,
 	hlist_for_each_entry(clk_user, &core->clks, clks_node)
 		*max_rate = min(*max_rate, clk_user->max_rate);
 }
+EXPORT_SYMBOL_GPL(clk_hw_get_boundaries);
 
 void clk_hw_set_rate_range(struct clk_hw *hw, unsigned long min_rate,
 			   unsigned long max_rate)
@@ -894,7 +896,7 @@ unsigned long clk_hw_round_rate(struct clk_hw *hw, unsigned long rate)
 	int ret;
 	struct clk_rate_request req;
 
-	clk_core_get_boundaries(hw->core, &req.min_rate, &req.max_rate);
+	clk_hw_get_boundaries(hw, &req.min_rate, &req.max_rate);
 	req.rate = rate;
 
 	ret = clk_core_round_rate_nolock(hw->core, &req);
@@ -924,7 +926,7 @@ long clk_round_rate(struct clk *clk, unsigned long rate)
 
 	clk_prepare_lock();
 
-	clk_core_get_boundaries(clk->core, &req.min_rate, &req.max_rate);
+	clk_hw_get_boundaries(clk->core->hw, &req.min_rate, &req.max_rate);
 	req.rate = rate;
 
 	ret = clk_core_round_rate_nolock(clk->core, &req);
@@ -1353,7 +1355,7 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
 	if (parent)
 		best_parent_rate = parent->rate;
 
-	clk_core_get_boundaries(core, &min_rate, &max_rate);
+	clk_hw_get_boundaries(core->hw, &min_rate, &max_rate);
 
 	/* find the closest rate and parent clk/rate */
 	if (core->ops->determine_rate) {
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 5100ec1..2f10999 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -755,6 +755,8 @@ int __clk_mux_determine_rate_closest(struct clk_hw *hw,
 void clk_hw_reparent(struct clk_hw *hw, struct clk_hw *new_parent);
 void clk_hw_set_rate_range(struct clk_hw *hw, unsigned long min_rate,
 			   unsigned long max_rate);
+void clk_hw_get_boundaries(struct clk_hw *hw, unsigned long *min_rate,
+			   unsigned long *max_rate);
 
 static inline void __clk_hw_set_clk(struct clk_hw *dst, struct clk_hw *src)
 {
-- 
1.7.9.5

[PATCH 1/2] clk: rename clk_core_get_boundaries() to clk_hw_get_boundaries() and expose

[Stephen Boyd]

On 12/21, Alexander Kochetkov wrote:
> In order to provide a way to know clock limits to clock providers.
> 
> The patch is needed for fixing commit 5d890c2df900 ("clk: rockchip:
> add special approximation to fix up fractional clk's jitter").
> Custom approximation function introduced by the patch, can
> select frequency rate larger than one configured using
> clk_set_max_rate().
> 
> Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
> ---

Can you convert to the determine_rate op instead of round_rate?
That function should tell you the min/max limits so that you
don't need to query that information from the core.

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

[PATCH 1/2] clk: rename clk_core_get_boundaries() to clk_hw_get_boundaries() and expose

[Alexander Kochetkov]

> 21 ???. 2017 ?., ? 23:07, Stephen Boyd <sboyd@codeaurora.org> ???????(?):
> 
> Can you convert to the determine_rate op instead of round_rate?
> That function should tell you the min/max limits so that you
> don't need to query that information from the core.

I converted rockchip_fractional_approximation() to rockchip_determine_rate() (see the patch attached).
If it increase parent?s clock for out of limits value, than clock request will fail with -EINVAL, like
with round_rate() approach.

The problem is that min/max limits provided to determine_rate() is for clock for which the determine_rate()
was called. While rockchip_determine_rate() (rockchip_fractional_approximation()) requires information
about parent clock limits.

How can I know parents clock limits for current clock? Implement determine_rate() for each parent clocks
the same way I did for this one clock?

Regards,
Alexander.

diff --git a/drivers/clk/rockchip/clk.c b/drivers/clk/rockchip/clk.c
index 3c1fb0d..1e0c701 100644
--- a/drivers/clk/rockchip/clk.c
+++ b/drivers/clk/rockchip/clk.c
@@ -174,23 +174,9 @@ static void rockchip_fractional_approximation(struct clk_hw *hw,
 		unsigned long *m, unsigned long *n)
 {
 	struct clk_fractional_divider *fd = to_clk_fd(hw);
-	unsigned long p_rate, p_parent_rate;
-	unsigned long min_rate = 0, max_rate = 0;
-	struct clk_hw *p_parent;
 	unsigned long scale;
-
-	p_rate = clk_hw_get_rate(clk_hw_get_parent(hw));
-	if ((rate * 20 > p_rate) && (p_rate % rate != 0)) {
-		p_parent = clk_hw_get_parent(clk_hw_get_parent(hw));
-		p_parent_rate = clk_hw_get_rate(p_parent);
-		clk_hw_get_boundaries(clk_hw_get_parent(hw),
-			&min_rate, &max_rate);
-		if (p_parent_rate < min_rate)
-			p_parent_rate = min_rate;
-		if (p_parent_rate > max_rate)
-			p_parent_rate = max_rate;
-		*parent_rate = p_parent_rate;
-	}
+	unsigned long rate_orig = rate;
+	unsigned long parent_rate_orig = *parent_rate;
 
 	/*
 	 * Get rate closer to *parent_rate to guarantee there is no overflow
@@ -204,8 +190,36 @@ static void rockchip_fractional_approximation(struct clk_hw *hw,
 	rational_best_approximation(rate, *parent_rate,
 			GENMASK(fd->mwidth - 1, 0), GENMASK(fd->nwidth - 1, 0),
 			m, n);
+
+	pr_info("%s: %s: rate:%lu -> %lu parent_rate:%lu -> %lu m:%lu n:%lu\n",
+		__func__, clk_hw_get_name(hw), rate_orig,  rate,
+		parent_rate_orig, *parent_rate,
+		*m, *n);
 }
 
+static int rockchip_determine_rate(struct clk_hw *hw,
+				   struct clk_rate_request *req)
+{
+	unsigned long p_rate, p_parent_rate;
+	struct clk_hw *p_parent;
+	unsigned long best_parent_rate = req->best_parent_rate;
+
+	p_rate = clk_hw_get_rate(clk_hw_get_parent(hw));
+	if ((req->rate * 20 > p_rate) && (p_rate % req->rate != 0)) {
+		p_parent = clk_hw_get_parent(clk_hw_get_parent(hw));
+		p_parent_rate = clk_hw_get_rate(p_parent);
+		req->best_parent_rate = p_parent_rate;
+	}
+
+	pr_info("%s: %s: rate:%lu min_rate:%lu max_rate:%lu best_parent_rate:%lu -> %lu best_parent_hw:%s\n",
+		__func__, clk_hw_get_name(hw), req->rate, req->min_rate, req->max_rate, best_parent_rate, req->best_parent_rate,
+		req->best_parent_hw ? clk_hw_get_name(req->best_parent_hw) : "<null>");
+
+	return 0;
+}
+
+static struct clk_ops rockchip_clk_fractional_divider_ops;
+
 static struct clk *rockchip_clk_register_frac_branch(
 		struct rockchip_clk_provider *ctx, const char *name,
 		const char *const *parent_names, u8 num_parents,
@@ -253,7 +267,8 @@ static struct clk *rockchip_clk_register_frac_branch(
 	div->nmask = GENMASK(div->nwidth - 1, 0) << div->nshift;
 	div->lock = lock;
 	div->approximation = rockchip_fractional_approximation;
-	div_ops = &clk_fractional_divider_ops;
+	div_ops = &rockchip_clk_fractional_divider_ops;
+
 
 	clk = clk_register_composite(NULL, name, parent_names, num_parents,
 				     NULL, NULL,
@@ -392,6 +407,9 @@ struct rockchip_clk_provider * __init rockchip_clk_init(struct device_node *np,
 	ctx->grf = syscon_regmap_lookup_by_phandle(ctx->cru_node,
 						   "rockchip,grf");
 
+	rockchip_clk_fractional_divider_ops = clk_fractional_divider_ops;
+	rockchip_clk_fractional_divider_ops.determine_rate = rockchip_determine_rate;
+
 	return ctx;
 
 err_free:

[PATCH 1/2] clk: rename clk_core_get_boundaries() to clk_hw_get_boundaries() and expose

[Stephen Boyd]

On 12/25, Alexander Kochetkov wrote:
> 
> > 21 ???. 2017 ?., ? 23:07, Stephen Boyd <sboyd@codeaurora.org> ???????(?):
> > 
> > Can you convert to the determine_rate op instead of round_rate?
> > That function should tell you the min/max limits so that you
> > don't need to query that information from the core.
> 
> I converted rockchip_fractional_approximation() to rockchip_determine_rate() (see the patch attached).
> If it increase parent?s clock for out of limits value, than clock request will fail with -EINVAL, like
> with round_rate() approach.
> 
> The problem is that min/max limits provided to determine_rate() is for clock for which the determine_rate()
> was called. While rockchip_determine_rate() (rockchip_fractional_approximation()) requires information
> about parent clock limits.

Are these limits the min/max limits that the parent clk can
output at? Or the min/max limits that software has constrained on
the clk?

> 
> How can I know parents clock limits for current clock? Implement determine_rate() for each parent clocks
> the same way I did for this one clock?

If the parent can change rate, then the idea is that the child
will calculate the limits that it can handle based on what it can
do with the incoming min/max constraints, and then call
__clk_determine_rate() on its parent with a request structure
that has limits for whatever the child clk is able to handle. The
parent can then determine a rate it can output that's within that
range and tell the child clk if it will satisfy the constraints
or not along with the resulting rate it will output when the
__clk_determine_rate() function returns. I would expect the
constraints to get closer together the higher in the tree we go.

I haven't looked in detail at this
rockchip_fractional_approximation() code, but it shouldn't be
doing the work of both the child rate determination and the
parent rate determination in one place. It should work with the
parent to figure out the rate the parent can provide and then
figure out how to achieve the desired rate from there.

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

[PATCH 1/2] clk: rename clk_core_get_boundaries() to clk_hw_get_boundaries() and expose

[Alexander Kochetkov]

Initial thread here:
https://www.spinics.net/lists/linux-clk/msg21682.html


> 27 ???. 2017 ?., ? 4:06, Stephen Boyd <sboyd@codeaurora.org> ???????(?):
> 
> Are these limits the min/max limits that the parent clk can
> output at? Or the min/max limits that software has constrained on
> the clk?
> 

Don?t know how to answer. For example, parent can output 768MHz,
but some IP work unstable with that parent rate. This issues was observed by
me and I didn?t get official confirmation from rockchip. So, I limit
such clock to 192MHz using clk_set_max_rate(). May be I have to limit clk rate
using another approach.

Anybody from rockchip can confirm that? Or may be contact me clarifying details?

> I haven't looked in detail at this
> rockchip_fractional_approximation() code, but it shouldn't be
> doing the work of both the child rate determination and the
> parent rate determination in one place. It should work with the
> parent to figure out the rate the parent can provide and then
> figure out how to achieve the desired rate from there.

Here is clock tree:

        clock                       rate
        -----                       ----
        xin24m                      24000000
          pll_gpll                    768000000
             gpll                       768000000
                i2s_src              768000000
                   i2s0_pre         192000000
                      i2s0_frac     16384000
                         sclk_i2s0  16384000

I limit i2s0_pre rate to 192MHz in order to I2S IP work properly.
rockchip_fractional_approximation() get called for i2s0_frac.
if i2s0_pre rate is 20x times less than i2s0_frac, than rockchip_fractional_approximation()
tries to set i2s0_pre rate to i2s_src rate. It tries to increase it?s parent rate in order
to maximise relation between nominator and denominator.

If I convert rockchip_fractional_approximation() to rockchip_determine_rate(), than I get
min=0, max=192MHz limits inside rockchip_determine_rate(). May be there should be
new logic inside clk framework based on some new clk flags, that will provide max=768MHz
for rockchip_determine_rate().

Also found, that rockchip_fractional_approximation() increase parents rate unconditionally
without taking into account CLK_SET_RATE_PARENT flag.

Stephen, thanks a lot for deep description of determine_rate() background. I?ll taking some
time thinking about possible solutions.

Regards,
Alexander.

[PATCH 1/2] clk: rename clk_core_get_boundaries() to clk_hw_get_boundaries() and expose

[Stephen Boyd]

On 12/28, Alexander Kochetkov wrote:
> Initial thread here:
> https://www.spinics.net/lists/linux-clk/msg21682.html
> 
> 
> > 27 ???. 2017 ?., ? 4:06, Stephen Boyd <sboyd@codeaurora.org> ???????(?):
> > 
> > Are these limits the min/max limits that the parent clk can
> > output at? Or the min/max limits that software has constrained on
> > the clk?
> > 
> 
> Don?t know how to answer. For example, parent can output 768MHz,
> but some IP work unstable with that parent rate. This issues was observed by
> me and I didn?t get official confirmation from rockchip. So, I limit
> such clock to 192MHz using clk_set_max_rate(). May be I have to limit clk rate
> using another approach.

I'm asking if the rate is capped on the consumer side with
clk_set_max_rate() or if it's capped on the clk provider side to
express a hardware constraint.

> 
> Anybody from rockchip can confirm that? Or may be contact me clarifying details?
> 
> > I haven't looked in detail at this
> > rockchip_fractional_approximation() code, but it shouldn't be
> > doing the work of both the child rate determination and the
> > parent rate determination in one place. It should work with the
> > parent to figure out the rate the parent can provide and then
> > figure out how to achieve the desired rate from there.
> 
> Here is clock tree:
> 
>         clock                       rate
>         -----                       ----
>         xin24m                      24000000
>           pll_gpll                    768000000
>              gpll                       768000000
>                 i2s_src              768000000
>                    i2s0_pre         192000000
>                       i2s0_frac     16384000
>                          sclk_i2s0  16384000
> 
> I limit i2s0_pre rate to 192MHz in order to I2S IP work properly.
> rockchip_fractional_approximation() get called for i2s0_frac.
> if i2s0_pre rate is 20x times less than i2s0_frac, than rockchip_fractional_approximation()
> tries to set i2s0_pre rate to i2s_src rate. It tries to increase it?s parent rate in order
> to maximise relation between nominator and denominator.
> 
> If I convert rockchip_fractional_approximation() to rockchip_determine_rate(), than I get
> min=0, max=192MHz limits inside rockchip_determine_rate(). May be there should be
> new logic inside clk framework based on some new clk flags, that will provide max=768MHz
> for rockchip_determine_rate().
> 
> Also found, that rockchip_fractional_approximation() increase parents rate unconditionally
> without taking into account CLK_SET_RATE_PARENT flag.
> 
> Stephen, thanks a lot for deep description of determine_rate() background. I?ll taking some
> time thinking about possible solutions.
> 

Sounds like there are some things to be figured out here still. I
can take a closer look next week. Maybe Heiko will respond before
then.

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

[PATCH 1/2] clk: rename clk_core_get_boundaries() to clk_hw_get_boundaries() and expose

[Alexander Kochetkov]

> 29 ???. 2017 ?., ? 3:14, Stephen Boyd <sboyd@codeaurora.org> ???????(?):
> 
> I'm asking if the rate is capped on the consumer side with
> clk_set_max_rate() or if it's capped on the clk provider side to
> express a hardware constraint.
I do that using clk_set_max_rate() at provider size inside clk-rk3188.c.

> 
> Sounds like there are some things to be figured out here still. I
> can take a closer look next week. Maybe Heiko will respond before
> then.

I will be very grateful for the ideas. I can continue to work on this next
week too.

Happy New Year and Merry Christmas!

Regards,
Alexander.

[PATCH 2/2] clk: rockchip: limit clock rate in the rockchip_fractional_approximation()

[Alexander Kochetkov]

rockchip_fractional_approximation() can choose clock rate that can
be larger than one configured using clk_set_max_rate(). Request
to setup correct clock rate whose parent rate will be adjusted
to out of range value will fail with -EINVAL.

Fixes: commit 5d890c2df900 ("clk: rockchip: add special approximation
to fix up fractional clk's jitter").

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
---
 drivers/clk/rockchip/clk.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/clk/rockchip/clk.c b/drivers/clk/rockchip/clk.c
index 35dbd63..3c1fb0d 100644
--- a/drivers/clk/rockchip/clk.c
+++ b/drivers/clk/rockchip/clk.c
@@ -175,6 +175,7 @@ static void rockchip_fractional_approximation(struct clk_hw *hw,
 {
 	struct clk_fractional_divider *fd = to_clk_fd(hw);
 	unsigned long p_rate, p_parent_rate;
+	unsigned long min_rate = 0, max_rate = 0;
 	struct clk_hw *p_parent;
 	unsigned long scale;
 
@@ -182,6 +183,12 @@ static void rockchip_fractional_approximation(struct clk_hw *hw,
 	if ((rate * 20 > p_rate) && (p_rate % rate != 0)) {
 		p_parent = clk_hw_get_parent(clk_hw_get_parent(hw));
 		p_parent_rate = clk_hw_get_rate(p_parent);
+		clk_hw_get_boundaries(clk_hw_get_parent(hw),
+			&min_rate, &max_rate);
+		if (p_parent_rate < min_rate)
+			p_parent_rate = min_rate;
+		if (p_parent_rate > max_rate)
+			p_parent_rate = max_rate;
 		*parent_rate = p_parent_rate;
 	}
 
-- 
1.7.9.5