From mboxrd@z Thu Jan  1 00:00:00 1970
From: Josh Karabin <gkarabin@vocollect.com>
Date: Mon, 11 May 2009 16:47:33 -0400
Subject: [U-Boot] [PATCH] Add +size syntax to nand commands to pad
 lengths to page	sizes.
In-Reply-To: <20090511193712.GC17546@b07421-ec1.am.freescale.net>
References: <20090501202419.GA11129@gkarabin-f9.vocollect.int>
	<20090511193712.GC17546@b07421-ec1.am.freescale.net>
Message-ID: <4A088EE5.7030401@vocollect.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de


Thanks for the review!  I have some questions below that'll help me get
rev 2 correct.

Scott Wood wrote:
> On Fri, May 01, 2009 at 04:24:20PM -0400, Josh Karabin wrote:
>> @@ -119,8 +121,12 @@ arg_off_size(int argc, char *argv[], nand_info_t *nand, ulong *off, size_t *size
>>  			}
>>  			*off = part->offset;
>>  			if (argc >= 2) {
>> -				if (!(str2long(argv[1], (ulong *)size))) {
>> -					printf("'%s' is not a number\n", argv[1]);
>> +				if (plussed && *ps == '+') {
>> +					*plussed = 1;
>> +					ps++;
>> +				}
>> +				if (!(str2long(ps, (ulong *)size))) {
>> +					printf("'%s' is not a number\n", ps);
>>  					return -1;
>>  				}
>>  				if (*size > part->size)
>> @@ -145,8 +151,12 @@ arg_off_size(int argc, char *argv[], nand_info_t *nand, ulong *off, size_t *size
>>  	}
>>  
>>  	if (argc >= 2) {
>> -		if (!(str2long(argv[1], (ulong *)size))) {
>> -			printf("'%s' is not a number\n", argv[1]);
>> +		if (plussed && *ps == '+') {
>> +			*plussed = 1;
>> +			ps++;
>> +		}
>> +		if (!(str2long(ps, (ulong *)size))) {
>> +			printf("'%s' is not a number\n", ps);
>>  			return -1;
> 
> Hmm... would be nice to untangle the duplicated code path rather than add
> more stuff to both branches.

No problem.  The two branches already duplicated the "str2long" check,
which was why I left it that way, but I can certainly clean it up.


>> @@ -317,7 +327,7 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
>>  
>>  		printf("\nNAND %s: ", scrub ? "scrub" : "erase");
>>  		/* skip first two or three arguments, look for offset and size */
>> -		if (arg_off_size(argc - o, argv + o, nand, &off, &size) != 0)
>> +		if (arg_off_size(argc - o, argv + o, nand, &off, &size, NULL) != 0)
>>  			return 1;
>>  
>>  		memset(&opts, 0, sizeof(opts));
>> @@ -378,8 +388,18 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
>>  
>>  		read = strncmp(cmd, "read", 4) == 0; /* 1 = read, 0 = write */
>>  		printf("\nNAND %s: ", read ? "read" : "write");
>> -		if (arg_off_size(argc - 3, argv + 3, nand, &off, &size) != 0)
>> +		if (read && arg_off_size(argc - 3, argv + 3, nand, &off, &size, NULL) != 0)
>>  			return 1;
>> +		else if (!read) {
>> +			int plussed = 0;
>> +			if (arg_off_size(argc - 3, argv + 3, nand, &off, &size, &plussed) != 0)
>> +				return 1;
> 
> Why not support plussed for read as well?

"read" isn't strictly necessary, since the existing code permits lengths
that result in page-unaligned reads.

Other operations are a little obvious.  "erase" implicitly extends the
page size already if it needs to, while "unlock" requires page aligned
lengths, and "lock" is whole-chip only, so the concept doesn't apply there.

"write" is the only operation that would ever need to depend on
+{filesize} for which I could think of a non contrived example, so I
stopped there.  That said, would you prefer me to support plussed "read"
or any of the other operations (erase, unlock)?


> 
>> +			if (plussed) {
>> +				int tailsize = size & (nand->writesize - 1);
>> +				memset ((u_char *)addr + size, 0xff, nand->writesize - tailsize);
>> +				size += nand->writesize - tailsize;
> 
> NACK, you cannot write to arbitrary memory beyond the end of the range
> specified.  Allocate a buffer to hold the partial page.

OK.  I expected this, but I wanted to ask a question about it.

Are there actual callers for which this would cause a problem, or is
this intended to make the code future proof?  I couldn't find any, so I
assume that the objection is the latter?  Commands like "tftpboot" and
"loadb" already stomp on memory without allocating it, so I wasn't sure
why the nand commands should be handled differently.

That said, it's not a hard change, so I'll make it.


> Plus, this will append an entire page of padding if the size does happen
> to be page-aligned.

Thanks for the catch.


> 
>> +			}
>> +		}
> 
> Please keep lines under 80 characters.
> 

Will do.  I noticed a few other lines in the file that hadn't, so
figured it wasn't an enforced standard.  It will be corrected.


> -Scott