Re: [PATCH 1/2] spi: Add MXIC controller driver

[Trent Piepho <tpiepho@xxxxxxxxxx>]

On Fri, 2018-08-03 at 13:06 +0200, Geert Uytterhoeven wrote:
> Hi Boris,
> 
> On Fri, Aug 3, 2018 at 9:48 AM Boris Brezillon
> <boris.brezillon@xxxxxxxxxxx> wrote:
> > On Thu, 2 Aug 2018 19:27:45 +0000
> > Trent Piepho <tpiepho@xxxxxxxxxx> wrote:
> > > On Thu, 2018-08-02 at 10:07 +0800, masonccyang@xxxxxxxxxxx wrote:
> > > > +   ret = clk_set_phase(mxic->send_dly_clk, 360 / (1000000000 / freq));
> > > 
> > > Can also be written as freq*9/25000000.
> > 
> > I'd like to keep 360 here. The phase is in degree, hence the 360. The
> > compiler should be smart enough to optimize that at compilation
> > time ;-).
> 
> It may have a hard time optimizing that, while preserving the semantics
> of the original expression (rounding behavior!)...

Yes, that is the issue.  Two integer divisions must be done.

> 
> Speaking about that, there's a serious issue due to loss of accuracy when
> doing two divisions like that. "360 * freq / 1000000000" is better, assumed
> the multiplication will be done in 64-bit arithmetic to avoid overflow:

That's why I suggested 9 * freq / 25000000.  Take the factor of 40 out
of both sides to raise the freq needed to overflow to above the max
speed of the controller.  It doesn't change the result to do that.

gcc ends up being smart enough to optimize this into:

((freq + freq << 3) * 1441151881) >> 55

to avoid multiplication by 9 ("add r0,r0,r0,lsl #3") and turn the
division into a multiply and shift.  Division is slow.
��.n��������+%������w��{.n�����{����)��jg��������ݢj����G�������j:+v���w�m������w�������h�����٥