Hi,
On 17-01-15 13:09, Chen-Yu Tsai wrote:
Hi,
On Sat, Jan 17, 2015 at 7:15 PM, Hans de Goede <[email protected]> wrote:
Hi,
On 17-01-15 12:12, Hans de Goede wrote:
Hi,
On 17-01-15 11:37, Chen-Yu Tsai wrote:
Hi,
On Sat, Jan 17, 2015 at 5:34 PM, Hans de Goede <[email protected]>
wrote:
Hi ChenYu,
Looking at drivers/clk/sunxi/clk-sun9i-core.c:
sun9i_a80_get_pll4_factors(), and comparing it with
the A80 user manual, things seem way off, this seems
more accurate (although also possibly not quite)
for pll1 / pll2 then for pll4, and the comment at
the top does mention PLL1 once.
PLL1 was mentioned as I forgot to change that one in
the comment. I copied the comment section from others.
Other than that, I'm not sure what part is off. The
code was done without the user manual, only with the
SDK bits. I think in the SDK the minimum value of 12
for N factor was not mentioned. But other than that,
it should work fine.
Ok, so looking at the code again it is not that much
of, just hard to read, and it does contain some errors:
/* divs above 256 cannot be odd */
if (div > 256)
div = round_up(div, 2);
Should be:
/* divs above 255 must be a multiple of 2 */
if (div > 255)
div = round_up(div, 2);
Note the 255, and replacing must be odd with
a multiple of 2, as this had nothing to do with odd /
even (more on that later).
Likewise this:
/* divs above 512 must be a multiple of 4 */
if (div > 512)
div = round_up(div, 4);
Should have s/512/511/ done on it.
And this:
/* m will be 1 if div is odd */
if (div & 1)
*m = 1;
else
*m = 0;
Is nonsense, the div may have been odd all along,
(never been rounded) and we should still use m = 1, e.g.
to make 78 MHz.
Correction this should read:
Is nonsense, the div may have been *even* all along,
(never been rounded) and we should still use m = 1, e.g.
to make *72* MHz.
The m and p constraints were based on comments in the
original SDK code, which weren't very clear as to if
they were hardware constraints or just preferred
behavior in the code path. Looking back, I agree the
work done by me here could have been better.
Regards,
Hans
So this should be:
if (div < 256)
*m = 1;
else
*m = 0;
Preferably I would like to see the entire thing rewritten
into something IMHO much more readable / less error prone,
like this:
{
u8 n;
u8 m = 1;
u8 p = 1;
/* Normalize value to a 6M multiple */
n = DIV_ROUND_UP(*freq, 6000000);
if (n > 255) {
m = 0;
n = (n + 1) / 2;
}
if (n > 255) {
p = 0;
n = (n + 1) / 2;
}
if (n > 255)
n = 255;
else if (n < 12)
n = 12;
*freq = ((24000000 * n) >> p) / (m + 1);
if (n_ret == NULL)
return;
*n_ret = n;
*m_ret = m;
*p_ret = p;
}
With the n / m / p function parameters renamed to foo_ret.
Since you've pretty much wrote the whole function, would
you send a patch out when you get the chance?
Sure, the reason I was proposing this by mail first was to
see if you agree that something like the above will be better,
if you agree I'll turn this into a patch.
Regards,
Hans
--
You received this message because you are subscribed to the Google Groups
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
