  `On Wed, 2018-08-08 at 11:19 +0800, Baolin Wang wrote:> On 8 August 2018 at 01:10, Trent Piepho <tpiepho@impinj.com> wrote:> > On Tue, 2018-08-07 at 18:43 +0800, Baolin Wang wrote:> > > > > > +static u32 sprd_spi_transfer_max_timeout(struct sprd_spi *ss,> > > +                                      struct spi_transfer *t)> > > +{> > > +     /*> > > +      * The time spent on transmission of the full FIFO data is the maximum> > > +      * SPI transmission time.> > > +      */> > > +     u32 size = t->bits_per_word * SPRD_SPI_FIFO_SIZE;> > > +     u32 bit_time_us = SPRD_SPI_HZ / t->speed_hz + 1;There's another flaw here in that the transfer speed, t->speed_hz,might not be exactly what is used, due to limitations of the clockdivider.  It would be better to find the actual SPI clock used, thenuse that in the calculations.> > > +     u32 total_time_us = size * bit_time_us;> > > +     /*> > > +      * There is an interval between data and the data in our SPI hardware,> > > +      * so the total transmission time need add the interval time.> > > +      *> > > +      * The inteval calculation formula:> > > +      * interval time (source clock cycles) = interval * 4 + 10.> > > +      */> > > +     u32 interval_cycle = SPRD_SPI_FIFO_SIZE * ((ss->interval << 2) + 10);> > > +     u32 interval_time_us = interval_cycle * SPRD_SPI_HZ / ss->src_clk + 1;> > > > If interval is greater than 31, this will overflow.> > Usually we will not set such a big value for interval,  but this is a> risk like you said. So we will check and limit the interval value to> make sure the formula will not overflow.> Better would be to limit the inter word delay to whatever maximum valueyour hardware supports, and then write code that can calculate thatwithout error.> > > +static void sprd_spi_set_speed(struct sprd_spi *ss, u32 speed_hz)> > > +{> > > +     /*> > > +      * From SPI datasheet, the prescale calculation formula:> > > +      * prescale = SPI source clock / (2 * SPI_freq) - 1;> > > +      */> > > +     u32 clk_div = ss->src_clk / (speed_hz << 1) - 1;> > > > You should probably round up here.  The convention is to use the> > closest speed less than equal to the requested speed, but since this is> > a divider, rounding it down will select the next highest speed.> > Per the datasheet, we do not need round up/down the speed. Since our> hardware can handle the clock calculated by the above formula if the> requested speed is in the normal region (less than ->max_speed_hz).That is not what I mean.  Let me explain differently.An integer divider like this can not produce any frequency exactly. Consider if src_clk is 10 MHz.  A clk_div value of 0 produces a 5 MHzSPI clock.  A clk_div value of 1 produces a 2.5 MHz SPI clock.What if the transfer requests a SPI clock of 3 MHz?Your math above will produce a SPI clock of 5 MHz, faster thanrequested.  This is not the convention in Linux SPI masters.  Youshould instead of have chosen a clk_div value of 1 to get a SPI clockof 2.5 MHz, the closest clock possible that is not greater than therequested value.To do this, round up.clk_div = DIV_ROUND_UP(ss->src_clk, speed_hz * 2) - 1;> `   