sun9i pll4 upstream kernel code seems wrong

[Chen-Yu Tsai]

On Sat, Jan 17, 2015 at 8:30 PM, Hans de Goede <hdegoede at redhat.com> wrote:
> Hi,
>
>
> On 17-01-15 13:09, Chen-Yu Tsai wrote:
>>
>> Hi,
>>
>> On Sat, Jan 17, 2015 at 7:15 PM, Hans de Goede <hdegoede at redhat.com>
>> 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 <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.
>>>
>>>
>>>
>>> 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.

I say go for it. I'm a bit busy with travel preparations and
other stuff this week, so I haven't tested the sample above.


ChenYu