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

Arnaud Mouiche
2017-03-30T12:26:07Z

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@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