sun9i pll4 upstream kernel code seems wrong

[Hans de Goede]

Hi,

On 17-01-15 11:37, Chen-Yu Tsai wrote:
> Hi,
>
> On Sat, Jan 17, 2015 at 5:34 PM, Hans de Goede <hdegoede at redhat.com> 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.

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.

Regards,

Hans


>
> Note the "div" variable uses a base frequency of 6,
> not 24. I used a spreadsheet to do some calculations
> to see if the code does output the right numbers.
> Unfortunately I don't have either the notes or the
> spreadsheet I used back then. It would take me a bit
> of time to check.
>
>> Note according to the datasheet pll4 should be treated
>> as an inmutable pll fixed at 960 MHz, so maybe we should
>> just drop the get_factors function for it ?
>
> Is that acceptable? Or is there a readonly flag we should
> set for the clock?
>
> Regards,
> ChenYu
>
>> Luckily the struct clk_factors_config sun9i_a80_pll4_config
>> is correct, so as long as we do not try to change the
>> rate the current upstream code for pll4 does work.
>>
>> Regards,
>>
>> Hans