On 5/4/2013 3:06 AM, Dirk Behme wrote:
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;

?

Looks good to me. (but {} for else too)


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

Sounds like a plan.

Troy

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

Reply via email to