Am 03.05.2013 22:47, schrieb Troy Kisky:
On 5/2/2013 10:58 PM, Dirk Behme wrote:
Do you want to say you propose

post_div = pre_div / 16;
pre_div = 16;

?
yes, that's what I said

If so:

First, I agree that we have to use the same dividers in both lines.

But, second, this would mean that you use /16 as max pre_div. For
the i.MX6 case where clk_src is 60MHz this would result in a
pre-divided clock of 3.75Mhz (instead of 4MHz with /15).

That does sound better for i.MX6, what about other processors using
this file?


So using /15 or /16 is just a decision of which end clocks most
probably are needed.

If you want to be able to configure 4MHz, 2MHz, 1MHz, 500kHz etc
then /15 is the better choice.

If you want to be able to configure 3.75Mhz, 1.875MHz, 937.5kHz,
468.75kHz etc then /16 is the better choice.

I vote for /15 as done by my patch.

Thanks for explaining. The downside of using /15 is that you can't get
the slowest clock possible.

Yes. I was looking for the _highest_ clock possible, though ;) And this isn't correctly done by the recent code. This is why I was looking into it ...

How about restructuring the code to improve both. Calculate post_div
first.

pre_div = DIV_ROUND_UP(clk_src, max_hz);
/* fls(1) = 1, fls(0x80000000) = 32, fls(16) = 5 */
post_div = fls(pre_div - 1);
if (post_div > 4)
     post_div -= 4;
else
     post_div = 0;

if (post_div >= 16) {
            printf("Error: no divider for the freq: %d\n",
                                         max_hz);
            return -1;
}
pre_div = (pre_div + (1 << post_div) - 1) >> post_div;

Using my test code gives the correct values using this algorithm. So yes, sounds good.

Just a small note: Wouldn't it be better to put the printf and the last line with the pre_div calculation into the if(post_div > 4) part? I.e.

pre_div = DIV_ROUND_UP(clk_src, max_hz);
/* fls(1) = 1, fls(0x80000000) = 32, fls(16) = 5 */
post_div = fls(pre_div - 1);
if (post_div > 4) {
    post_div -= 4;

    if (post_div >= 16) {
           printf("Error: no divider for the freq: %d\n",
                                        max_hz);
           return -1;
    }
    pre_div = (pre_div + (1 << post_div) - 1) >> post_div;

} else
    post_div = 0;

?

In case we agree on this, I'm thinking about doing 2 patches to make clear what we are doing:

1. Re-doing my initial patch with

post_div = pre_div / 16;
pre_div = 16;

This would be the "fix the issues in the existing (non-optimal) algorithm but keep that" patch.

2. Replace the existing algorithm with your above version. This would be the "improve the algorithm" patch.

What do you think?

Best regards

Dirk





_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to