On 02/25/2013 11:43:48 PM, Harvey Chapman wrote:
Adjust the sizes calculated for whole partition/chip operations by
removing the size of bad blocks so we don't try to erase/read/write
past a partition/chip boundary.
Signed-off-by: Harvey Chapman <hchap...@3gfp.com>
---
common/cmd_nand.c | 51
+++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 51 insertions(+)
Looks OK except for style issues:
diff --git a/common/cmd_nand.c b/common/cmd_nand.c
index 495610c..657ea23 100644
--- a/common/cmd_nand.c
+++ b/common/cmd_nand.c
@@ -428,6 +428,32 @@ static int raw_access(nand_info_t *nand, ulong
addr, loff_t off, ulong count,
return ret;
}
+static int adjust_size_for_badblocks(loff_t *size, loff_t offset,
int dev) {
Braces go on their own line for function definitions.
+ /* We grab the nand info object here fresh because this is
usually
+ * called after arg_off_size() which can change the value of
dev.
+ */
/*
* U-Boot multiline
* brace style is like this
*/
...in general, U-Boot follows Linux code style.
+ nand_info_t *nand = &nand_info[dev];
+ loff_t original_size = *size;
+ loff_t maxoffset = offset + *size;
+ int badblocks = 0;
+
+ /* count badblocks in NAND from offset to offset + size */
+ for (; offset < maxoffset; offset += nand->erasesize)
+ if (nand_block_isbad(nand, offset)) {
+ badblocks++;
+ }
Need braces around multi-line statements.
+ /* adjust size if any bad blocks found */
+ if (badblocks) {
+ *size -= badblocks * nand->erasesize;
+ printf("size adjusted to 0x%llx (%d bad blocks)\n",
+ (unsigned long long)*size, badblocks);
+ }
+ /* return size adjusted as a positive value so callers
+ * can use the return code to determine if anything happened
+ */
+ return (original_size - *size);
Unnecessary parens.
Do we have any callers that care about the return code? If not, don't
bother with it. This is an internal static function, not a public
API. It's easy to change later if we need to.
+ /* The size for erase.part and erase.chip has been
calculated
+ * for us as the remainder of the chip/partition from
offset.
+ * Adjust down for bad blocks, if necessary, so we don't
+ * erase past the end of the chip/partition by accident.
+ */
+ if (adjust_size && !scrub) {
+ adjust_size_for_badblocks(&size, off, dev);
+ }
+
No braces around single-line if-bodies.
nand = &nand_info[dev];
memset(&opts, 0, sizeof(opts));
@@ -644,6 +682,19 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag,
int argc, char * const argv[])
&off, &size) != 0)
return 1;
+ /* If no size was given, it has been calculated
for us as
+ * the remainder of the chip/partition from
offset. Adjust
+ * down for bad blocks, if necessary, so we
don't
+ * read/write past the end of the partition by
accident.
+ *
+ * nand read addr part size "size" is arg 5
+ */
+ if (argc < 5) {
+ /* Don't try to use rwsize here, it's
not the
+ * right type
+ */
+ adjust_size_for_badblocks(&size, off,
dev);
+ }
No need to be quite so verbose in the comments. If someone tries to
change "size" to "rwsize" the compiler will complain about the type
mismatch.
As for the other comment, the function name "adjust_size_for_badblocks"
explains what's going on well enough IMHO. At most a single comment of
/* size is unspecified */ to describe the if block. At least put the
explanation on the adjust_size_for_badblocks() function rather than
repeat it for read/write and erase.
-Scott
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
