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
