[U-Boot] [PATCHv2] fsl_ddr: Don't use full 64-bit divides on32-bit PowerPC

[York Sun]

On Fri, 2011-07-29 at 11:35 -0700, York Sun wrote:
> Kyle,
> 
> On Fri, 2011-07-29 at 12:20 -0500, Moffett, Kyle D wrote:
> > On Jul 28, 2011, at 17:41, York Sun wrote:
> > > On Mon, 2011-03-14 at 16:35 -0500, Moffett, Kyle D wrote:
> > >> On Mar 14, 2011, at 16:22, York Sun wrote:
> > >>> On Wed, 2011-02-23 at 11:35 -0500, Kyle Moffett wrote:
> > >>>> +	 * Now divide by 5^12 and track the 32-bit remainder, then divide
> > >>>> +	 * by 2*(2^12) using shifts (and updating the remainder).
> > >>>> +	 */
> > >>>> +	clks_rem = do_div(clks, UL_5pow12);
> > >>>> +	clks_rem <<= 13;
> > >>> 
> > >>> Shouldn't this be clks_rem >>= 13 ?
> > >>>> 
> > >>>> +	clks_rem |= clks & (UL_2pow13-1);
> > >>>> +	clks >>= 13;
> > >>>> +
> > >>>> +	/* If we had a remainder, then round up */
> > >>>> +	if (clks_rem)
> > >>>> 		clks++;
> > >>>> -	}
> > > 
> > > I found a problem with the round up. Please try
> > > 
> > > picos_to_mclk(mclk_to_picos(3))
> > > 
> > > You will notice the result is round up. I am sure it is unexpected.
> > 
> > Hmm... what does fsl_ddr_get_mem_data_rate() return on your system?
> > I cannot reproduce this here with my memory freq of 800MHz.
> > 
> > The function has obviously always done rounding, even before I made
> > any changes.  Have you retested what the math does with the patch
> > reverted to see if it makes any difference?
> > 
> > Hmm, looking at it, the obvious problem case would be mclk_to_picos()
> > using a rounded result in a subsequent multiply.  Can you retest by
> > replacing mclk_to_picos() with the following code; this should keep
> > the error down by doing the rounding-to-10ps *after* the multiply.
> > 
> > unsigned int mclk_to_picos(unsigned int mclk)
> > {
> >         unsigned int data_rate = get_ddr_freq(0);
> >         unsigned int result;
> > 
> >         /* Round to nearest 10ps, being careful about 64-bit multiply/divide */
> >         unsigned long long mclk_ps = ULL_2e12 * mclk;
> > 
> >         /* Add 5*data_rate, for rounding */
> >         mclk_ps += 5*(unsigned long long)data_rate;
> >  
> >         /* Now perform the big divide, the result fits in 32-bits */
> >         do_div(mclk_ps, data_rate);
> >         result = mclk_ps;
> > 
> >         /* We still need to round to 10ps */
> >         return 10 * (result/10);
> > }
> > 
> > Obviously you could also get rid of the rounding alltogether, but I
> > don't know why it was put in the code in the first place...
> 
> I think the problem comes from the round up. But your calculation of
> remainder is not right. You explained to me with example of 2*10^^12-1.
> It seems to be right until I tried another number 264*10^^7. I think the
> following calculation of remainder is
> 
> 	clks_rem = do_div(clks, UL_5POW12);
> 	clks >>= 13;
> 	clks_rem |= (clks & (UL_2POW13-1)) * UL_5POW12;
> 
> Then we can compare the remainder with the "error" introduced by
> mclk_to_picos, which is 10ps.
> 
> 	if (clks_rem > 10 * data_rate)
> 		clk++;
> 
> What do you think?
> 

Maybe not a good idea. It may not round up when necessary.

York