[U-Boot] [PATCH] spi: mxc_spi: Fix pre and post divider calculation

[Troy Kisky]

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