Re: [linux-sunxi] [PATCH 1/5] spi: sunxi: fix transfer timeout

From: Michal Suchanek
Date: Fri May 27 2016 - 01:06:07 EST


On 27 May 2016 at 04:05, Julian Calaby <julian.calaby@xxxxxxxxx> wrote:
> Hi Michal,
>
> On Fri, May 27, 2016 at 5:25 AM, Michal Suchanek <hramrach@xxxxxxxxx> wrote:
>> The trasfer timeout is fixed at 1000 ms. Reading a 4Mbyte flash over
>> 1MHz SPI bus takes way longer than that. Calculate the timeout from the
>> actual time the transfer is supposed to take and multiply by 2 for good
>> measure.
>>
>> Signed-off-by: Michal Suchanek <hramrach@xxxxxxxxx>
>> ---
>>
>> v2:
>> - fix build error
>> - use unsigned instead of int in max_t
>> - use tfr->speed_hz instead of sspi->max_speed_hz
>> ---
>> drivers/spi/spi-sun4i.c | 11 ++++++++++-
>> drivers/spi/spi-sun6i.c | 11 ++++++++++-
>> 2 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
>> index 1ddd9e2..fe63bbd 100644
>> --- a/drivers/spi/spi-sun4i.c
>> +++ b/drivers/spi/spi-sun4i.c
>> @@ -173,6 +173,7 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
>> {
>> struct sun4i_spi *sspi = spi_master_get_devdata(master);
>> unsigned int mclk_rate, div, timeout;
>> + unsigned int start, end, tx_time;
>> unsigned int tx_len = 0;
>> int ret = 0;
>> u32 reg;
>> @@ -279,9 +280,17 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
>> reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
>> sun4i_spi_write(sspi, SUN4I_CTL_REG, reg | SUN4I_CTL_XCH);
>>
>> + tx_time = max_t(unsigned, tfr->len * 8 * 2 / (tfr->speed_hz / 1000),
>
> You should probably use "unsigned int" instead of just "unsigned" here.
>
>> + 100);

Or just 100U constant and avoid max_t altogether.

>> + start = jiffies;
>> timeout = wait_for_completion_timeout(&sspi->done,
>> - msecs_to_jiffies(1000));
>> + msecs_to_jiffies(tx_time));
>> + end = jiffies;
>> if (!timeout) {
>> + dev_warn(&master->dev,
>> + "%s: timeout transferring %u bytes@%iHz for %i(%i)ms",
>> + dev_name(&spi->dev), tfr->len, tfr->speed_hz,
>> + jiffies_to_msecs(end - start), tx_time);
>
> Should the debug changes be in a separate patch?

Is this so big of a change that it needs to be split?

>
>> ret = -ETIMEDOUT;
>> goto out;
>> }
>> diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
>> index 42e2c4b..8be5c5c 100644
>> --- a/drivers/spi/spi-sun6i.c
>> +++ b/drivers/spi/spi-sun6i.c
>> @@ -162,6 +162,7 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
>> unsigned int mclk_rate, div, timeout;
>> unsigned int tx_len = 0;
>> int ret = 0;
>> + unsigned int start, end, tx_time;
>> u32 reg;
>>
>> /* We don't support transfer larger than the FIFO */
>> @@ -269,9 +270,17 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
>> reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
>> sun6i_spi_write(sspi, SUN6I_TFR_CTL_REG, reg | SUN6I_TFR_CTL_XCH);
>>
>> + tx_time = max_t(unsigned, tfr->len * 8 * 2 / (tfr->speed_hz / 1000),
>
> Ditto, "unsigned int" instead of "unsigned"?
>
>> + 100);
>> + start = jiffies;
>> timeout = wait_for_completion_timeout(&sspi->done,
>> - msecs_to_jiffies(1000));
>> + msecs_to_jiffies(tx_time));
>> + end = jiffies;
>> if (!timeout) {
>> + dev_warn(&master->dev,
>> + "%s: timeout transferring %u bytes@%iHz for %i(%i)ms",
>> + dev_name(&spi->dev), tfr->len, tfr->speed_hz,
>> + jiffies_to_msecs(end - start), tx_time);
>
> Ditto, separate patch?
>
> Also, should the changes for the drivers be in two separate patches also?

That's basically the same driver with different constants so I guess not.

Thanks

Michal