[U-Boot] [PATCH 2/2] nand: extend nand torture

[Benoît Thébaudeau]

Hi Max,

On Tue, Jun 7, 2016 at 12:57 PM, Max Krummenacher <max.oss.09 at gmail.com> wrote:
> Hi Benoît,
>
> Thank you for your review.

You're welcome.

> I wanted to wait for Scott's patchseries to make it into master to
> allow for potential needed
> changes.

No problem.

> 2016-05-31 22:21 GMT+02:00 Benoît Thébaudeau <benoit.thebaudeau.dev at gmail.com>:
> ...
>>> Extend this by allowing for a second parameter specifying the byte offset
>>> to the last block to be tested.
>>>
>>
>> End offsets are always ambiguous because users can hesitate between
>> the offset of the first byte of the last block, the offset of the last
>> byte of the last block, and the offset of the first byte of the block
>> following the last one (if any). A byte size would probably be better
>> here, and it would also be more consistent with the other nand
>> commands.
>
> Ok. I will change the interface to use offset/size.

Good.

> ...
>>> NAND torture: device 0 offset 0x1000000 size 0x20000
>>> passed 2, failed 0
>>>
>>
>> With more than one block to test, the printed size becomes ambiguous
>> here. It would be better to indicate that it is the erase size of the
>> block. The total test size could also be printed, either instead of
>> the erase size, or besides it.
>>
> Ok. I will change this to print test size and block size, e.g. like:
> NAND torture: device 0 offset 0x1000000 size 0x40000 (nand block size 0x20000)

Good.

> ...
>>> -               return ret == 0 ? 0 : 1;
>>> +                       ret = nand_torture(nand, off);
>>> +                       if (ret) {
> ...
>>
>> A size parameter could probably be added to nand_torture() instead of
>> handling the range in the command, so that the direct usages of
>> nand_torture() (in or out of tree) can also benefit from this
>> enhancement.
>
> I disagree here.
> Likely one uses the extended functionality only during HW bringup and
> only interactively. If one would want to test multiple blocks from code one
> would also want to know the testresult of each individual block rather
> than only having a return parameter indicating a 'all good' or
> 'at least one block failed'.
> Even here in the interactive 'nand torture' cmd the printf of a failed
> block in the loop
> is a usecase of this.

Makes sense. Agreed.

Best regards,
Benoît