[U-Boot] [PATCH 3/4] [v3] nand_util: drop trailing all-0xff pages if requested

[Ben Gardiner]

Hi Scott,

Thanks for the review.

On Mon, Jun 6, 2011 at 4:58 PM, Scott Wood <scottwood at freescale.com> wrote:
> On Tue, May 24, 2011 at 10:18:36AM -0400, Ben Gardiner wrote:
>> +#ifdef CONFIG_CMD_NAND_TRIMFFS
>> +static size_t drop_ffs(const nand_info_t *nand, const u_char *buf,
>> +                     const size_t *len)
>> +{
>> +     size_t i, l = *len;
>> +
>> +     for (i = l - 1; i >= 0; i--)
>> +             if (((const uint8_t *)buf)[i] != 0xFF)
>> +                     break;
>
> This cast looks unnecessary.

You're absolutely right. It will be gone in v4.

>> +     /* The resulting length must be aligned to the minimum flash I/O size */
>> +     l = i + 1;
>> +     l = (l + nand->writesize - 1) / nand->writesize;
>> +     l *=  nand->writesize;
>> +     return l;
>
> We allow unaligned lengths (the rest of the page gets padded with 0xff,
> see nand_do_page_write-ops).  The input length might be unaligned --
> this adjustment could cause you to read beyond the end of the supplied
> buffer.

Right. Sorry I missed that. In v4 I will drop also any trailling 0xff
which do not make-up a full page since they would be padded out to
trailing 0xff.

>> +}
>> +#endif
>> +
>>  /**
>>   * nand_write_skip_bad:
>>   *
>> @@ -499,7 +520,7 @@ int nand_write_skip_bad(nand_info_t *nand, loff_t offset, size_t *length,
>>               return -EINVAL;
>>       }
>>
>> -     if (!need_skip) {
>> +     if (!need_skip && !(flags & WITH_DROP_FFS)) {
>>               rval = nand_write (nand, offset, length, buffer);
>>               if (rval == 0)
>>                       return 0;
>
> Why not call drop_ffs before this point?

To achieve the desired effect, drop_ffs must be called on each
eraseblock sized chunk being written; so it seemed the simplest way
was to force a block-by-block pass with the !WITH_DROP_FFS to enter

  while (left_to_write > 0) {

I'll leave this as-is in v4.

>> @@ -512,7 +533,7 @@ int nand_write_skip_bad(nand_info_t *nand, loff_t offset, size_t *length,
>>
>>       while (left_to_write > 0) {
>>               size_t block_offset = offset & (nand->erasesize - 1);
>> -             size_t write_size;
>> +             size_t write_size, truncated_write_size;
>>
>>               WATCHDOG_RESET ();
>>
>> @@ -558,7 +579,15 @@ int nand_write_skip_bad(nand_info_t *nand, loff_t offset, size_t *length,
>>               else
>>  #endif
>>               {
>> -                     rval = nand_write (nand, offset, &write_size, p_buffer);
>> +                     truncated_write_size = write_size;
>> +#ifdef CONFIG_CMD_NAND_TRIMFFS
>> +                     if (flags & WITH_DROP_FFS)
>> +                             truncated_write_size = drop_ffs(nand, p_buffer,
>> +                                             &write_size);
>> +#endif
>
> What if both WITH_DROP_FFS and WITH_YAFFS_OOB are specified?

I didn't plan for that or intend for it to be supported.

Previous to my introduction of WITH_DROP_FFS; using the YAFFS oob mode
was mutually exclusive with the 'usual' way of writing. The
introduction of WITH_DROP_FFs respects this precedent.

If both flags were set 1) cmd_nand.c would need to be changed ( :) )
and 2) the WITH_YAFFS_OOB behaviour would override.

In v4 I will add a -EINVAL if WITH_YAFFS_OOB flag is used with any other flag.

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca