[U-Boot] [PATCH] OneNAND partial read/write support

[Kyungmin Park]

On Wed, Oct 21, 2009 at 7:48 AM, Scott Wood <scottwood at freescale.com> wrote:
> On Mon, Oct 12, 2009 at 04:27:10PM +0900, Kyungmin Park wrote:
>> Now OneNAND handles block operation only.
>> With this patch OneNAND handles all read/write size.
>>
>> Signed-off-by: Kyungmin Park <kyungmin.park at samsung.com>
>> ---
>> diff --git a/common/cmd_onenand.c b/common/cmd_onenand.c
>> index 9090940..2b8f01b 100644
>> --- a/common/cmd_onenand.c
>> +++ b/common/cmd_onenand.c
>> @@ -36,7 +36,7 @@ static inline int str2long(char *p, ulong *num)
>>       return (*p != '\0' && *endptr == '\0') ? 1 : 0;
>>  }
>>
>> -static int arg_off_size(int argc, char *argv[], ulong *off, size_t *size)
>> +static int arg_off_size(int argc, char *argv[], ulong *off, ssize_t *size)
>>  {
>>       if (argc >= 1) {
>>               if (!(str2long(argv[0], off))) {
>
> Are you expecting negative sizes?

I don't know why these are implemented. it's existing one.

>
>> @@ -69,61 +69,65 @@ static int arg_off_size(int argc, char *argv[], ulong *off, size_t *size)
>>       return 0;
>>  }
>>
>> -static int onenand_block_read(loff_t from, size_t len,
>> -                           size_t *retlen, u_char *buf, int oob)
>> +static int onenand_block_read(loff_t from, ssize_t len,
>> +                           ssize_t *retlen, u_char *buf, int oob)
>>  {
>
> Is it still onenand_block_read if you don't have to read a whole block?
>
>>       struct onenand_chip *this = mtd->priv;
>> -     int blocks = (int) len >> this->erase_shift;
>>       int blocksize = (1 << this->erase_shift);
>>       loff_t ofs = from;
>>       struct mtd_oob_ops ops = {
>>               .retlen         = 0,
>>       };
>> +     ssize_t thislen;
>>       int ret;
>>
>> -     if (oob)
>> -             ops.ooblen = blocksize;
>> -     else
>> -             ops.len = blocksize;
>> +     while (len > 0) {
>> +             thislen = min_t(ssize_t, len, blocksize);
>> +             thislen = ALIGN(thislen, mtd->writesize);
>>
>> -     while (blocks) {
>>               ret = mtd->block_isbad(mtd, ofs);
>>               if (ret) {
>>                       printk("Bad blocks %d at 0x%x\n",
>>                              (u32)(ofs >> this->erase_shift), (u32)ofs);
>> -                     ofs += blocksize;
>> +                     ofs += thislen;
>>                       continue;
>>               }
>>
>> -             if (oob)
>> +             if (oob) {
>>                       ops.oobbuf = buf;
>> -             else
>> +                     ops.ooblen = thislen;
>> +             } else {
>>                       ops.datbuf = buf;
>> +                     ops.len = thislen;
>
> thislen can be greater than len, in which case you'll be overflowing buf.

No, len is unsigned int, but thislen is int. and can't overflow since.
it's size down with blocksize at min macro.

>
> If you want to allow partial page reads, you need to allocate a temporary
> buffer at some point.  If not (I don't see a huge need), the ALIGN()
> should be an error check instead.


>
> Does this code handle being given an offset that is not at a block (or
> page) boundary?  It doesn't look like it (it will try to read across
> block boundaries).

Basically it reads/writes data based on block. but last or first
partial read/writes support.

>
>> @@ -265,9 +276,10 @@ static int onenand_block_test(u32 start, u32 size)
>>                       goto next;
>>               }
>>
>> -             if (memcmp(buf, verify_buf, blocksize))
>> +             if (memcmp(buf, verify_buf, blocksize)) {
>>                       printk("\nRead/Write test failed at 0x%x\n", (u32)ofs);
>> -
>> +                     break;
>> +             }
>> @@ -322,6 +334,7 @@ static int onenand_dump(struct mtd_info *mtd, ulong off, int only_oob)
>>               p += 16;
>>       }
>>       puts("OOB:\n");
>> +     p = oobbuf;
>>       i = mtd->oobsize >> 3;
>>       while (i--) {
>>               printf("\t%02x %02x %02x %02x %02x %02x %02x %02x\n",
>> @@ -339,7 +352,7 @@ int do_onenand(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
>>       struct onenand_chip *this;
>>       int blocksize;
>>       ulong addr, ofs;
>> -     size_t len, retlen = 0;
>> +     ssize_t len, retlen = 0;
>>       int ret = 0;
>>       char *cmd, *s;
>>
>> @@ -385,7 +398,8 @@ int do_onenand(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
>>                       int erase;
>>
>>                       erase = strcmp(cmd, "erase") == 0; /* 1 = erase, 0 = test */
>> -                     printf("\nOneNAND %s: ", erase ? "erase" : "test");
>> +                     printf("\nOneNAND %s %s: ", erase ? "erase" : "test",
>> +                             force ? "force" : "");
>
> These seem to be unrelated changes.

Right, but it's for exact display for user.

Thank you,
Kyungmin Park