[PATCH v4 4/9] nand: spi: add basic blocks for infrastructure

[Arnaud Mouiche]

On 23/03/2017 17:33, Marek Vasut wrote:
> On 03/23/2017 04:40 PM, Boris Brezillon wrote:
>> On Thu, 23 Mar 2017 12:29:58 +0100
>> Marek Vasut <marex at denx.de> wrote:
>>
>>
>>>> +
>>>> +/*
>>>> + * spinand_read_status - get status register value
>>>> + * @chip: SPI NAND device structure
>>>> + * @status: buffer to store value
>>>> + * Description:
>>>> + *   After read, write, or erase, the NAND device is expected to set the
>>>> + *   busy status.
>>>> + *   This function is to allow reading the status of the command: read,
>>>> + *   write, and erase.
>>>> + */
>>>> +static int spinand_read_status(struct spinand_device *chip, uint8_t *status)
>>>> +{
>>>> +	return spinand_read_reg(chip, REG_STATUS, status);
>>>> +}
>>>> +
>>>> +/*
>>>> + * spinand_wait - wait until the command is done
>>>> + * @chip: SPI NAND device structure
>>>> + * @s: buffer to store status register value (can be NULL)
>>>> + */
>>>> +static int spinand_wait(struct spinand_device *chip, u8 *s)
>>>> +{
>>>> +	unsigned long timeo = msecs_to_jiffies(400);
>>>> +	u8 status;
>>>> +
>>>> +	do {
>>>> +		spinand_read_status(chip, &status);
>>>> +		if ((status & STATUS_OIP_MASK) == STATUS_READY)
>>>> +			goto out;
>>>> +	} while (time_before(jiffies, timeo));
>>>> +
>>>> +	/*
>>>> +	 * Extra read, just in case the STATUS_READY bit has changed
>>>> +	 * since our last check
>>>> +	 */
>>> Is this likely to happen ? Probably not ... so in case you reach this
>>> place here, timeout happened.
>> If the timeout is big enough, no. But it does not hurt to do one last
>> check.
> 400 mSec is not big enough ? It's huge ... this seems like a duplication
> to me. btw the ad-hoc 400 mSec delay value should be replaced with a macro.
>
In fact, there is a bug:

> unsigned long timeo = msecs_to_jiffies(400);
> [...]
> do { } while (time_before(jiffies, timeo));

The condition is almost true except during the 5 minutes following the boot (before jiffies wrap around).
So, only one status read is done, which give a high chance of returning a timeout status.
I guess you should do : 	

	unsigned long timeo = jiffies + msecs_to_jiffies(400);

I also suspect that this is the reason why you have an additional status read.

Arnaud