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

Message ID 8EC4D15B-4A89-43FA-953E-95AF81417067@gmail.com
State New
Headers show

Commit Message

Alexander Kochetkov Dec. 25, 2017, 9:38 a.m. UTC
> 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.

Comments

Stephen Boyd Dec. 27, 2017, 1:06 a.m. UTC | #1
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.
Alexander Kochetkov Dec. 28, 2017, 12:41 p.m. UTC | #2
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.
Stephen Boyd Dec. 29, 2017, 12:14 a.m. UTC | #3
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.
Alexander Kochetkov Dec. 29, 2017, 8:52 a.m. UTC | #4
> 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
diff mbox

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: