clk: divider: Use round off divisor instead of round up

Commit Message

Tejas Patel May 3, 2018, 9:25 a.m. UTC

divider_get_val() is used to find clock divisor which is
giving round up value. So even though actual divisor is closer to
round down integer value it will return round up integer value.
This can result into large variation between expected clock rate and
actual clock rate.

See for example:
Parent rate is 1400MHz and desired child rate is 693MHz.
So if we calculate simple divisor for child clock it would be
1400000000/693000000 = 2.02. But, because of roundup logic
(DIV_ROUND_UP_ULL(1400000000, 693000000)) divisor returned from
divider_get_val() will be 3.

So actual child rate would be (1400000000/3) = 466666667 instead of
693000000. Here, difference between expected child rate and actual child
rate is approx 226MHz. So there is almost 32% error between desired and
actual clock rate.

Fix this by using round off value (DIV_ROUND_CLOSEST_ULL) instead of
round up/down.

Signed-off-by: Tejas Patel <[e-mail]>
---
 drivers/clk/clk-divider.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--
2.7.4

This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Stephen Boyd May 15, 2018, 6:21 p.m. UTC | #1

Quoting Tejas Patel (2018-05-03 02:25:35)
> divider_get_val() is used to find clock divisor which is
> giving round up value. So even though actual divisor is closer to
> round down integer value it will return round up integer value.
> This can result into large variation between expected clock rate and
> actual clock rate.
> 
> See for example:
> Parent rate is 1400MHz and desired child rate is 693MHz.
> So if we calculate simple divisor for child clock it would be
> 1400000000/693000000 = 2.02. But, because of roundup logic
> (DIV_ROUND_UP_ULL(1400000000, 693000000)) divisor returned from
> divider_get_val() will be 3.
> 
> So actual child rate would be (1400000000/3) = 466666667 instead of
> 693000000. Here, difference between expected child rate and actual child
> rate is approx 226MHz. So there is almost 32% error between desired and
> actual clock rate.
> 
> Fix this by using round off value (DIV_ROUND_CLOSEST_ULL) instead of
> round up/down.

Is your divider using the CLK_DIVIDER_ROUND_CLOSEST flag? When
divider_get_val() is called, the incoming rate should already be rounded
to something that is what the actual frequency would be so the rounding
here hopefully doesn't matter.

Maybe something is lost in translation though, because we do
round_rate() to figure out some divider based on a requested rate, and
then we return that rate we calculated that we can achieve to the
framework. In turn, that rate comes back into this divider_get_val()
function to get the divider out of the rate again. It's sort of annoying
how circular this is. Perhaps your example can show this call sequence
and the associated math gymnastics that go on and then I'll be convinced
that we need to update this part of the code.

--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index b6234a5..6ae6d43 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -391,7 +391,7 @@  int divider_get_val(unsigned long rate, unsigned long parent_rate,
 {
        unsigned int div, value;

-       div = DIV_ROUND_UP_ULL((u64)parent_rate, rate);
+       div = DIV_ROUND_CLOSEST_ULL((u64)parent_rate, rate);

        if (!_is_valid_div(table, div, flags))
                return -EINVAL;