On 12/12/2011 01:08, Ilya Yanok wrote:
Hi Stefano,
thanks for posting this. There is a couple of comments below.
On 11.12.2011 21:22, Stefano Babic wrote:
diff --git a/drivers/mtd/nand/nand_spl_simple.c b/drivers/mtd/nand/nand_spl_simple.c index ed821f2..a3d1af0 100644 --- a/drivers/mtd/nand/nand_spl_simple.c +++ b/drivers/mtd/nand/nand_spl_simple.c @@ -145,9 +145,9 @@ static int nand_is_bad_block(int block) static int nand_read_page(int block, int page, uchar *dst) { struct nand_chip *this = mtd.priv;
- u_char *ecc_calc;
- u_char *ecc_code;
- u_char *oob_data;
- u_char ecc_calc[CONFIG_SYS_NAND_ECCSTEPS * CONFIG_SYS_NAND_ECCBYTES];
- u_char ecc_code[CONFIG_SYS_NAND_ECCTOTAL];
Doesn't ECCTOTAL always equal to ECCSTEPS * ECCBYTES?
I see. Then we can also remove CONFIG_SYS_NAND_ECCTOTAL from the configuration files, because it is not useful. I will do in V2.
By grepping the code, the same issue is present in nand_spl/nand_boot.c, too. This is part of the old spl code, I do not know if we have also to fix this one. IMHO we can only fix nand_spl_simple.c, because the other one will become obsolete and there is not probelms with current boards.
- u_char oob_data[CONFIG_SYS_NAND_OOBSIZE];
I think we can save a few bytes by placing ecc_code and oob_data into a union. We only need oob_data to get ecc_code...
Checking the real values I see that ECCTOTAL is small - 8 bytes on the TI we are currently testing, and it has not a big value on other architectures. So I think we can avoid the union, also because this can generate overlapping when the oob data is copied: I mean in case nand_ecc_pos[i] is smaller as CONFIG_SYS_NAND_ECCTOTAL at:
for (i = 0; i < CONFIG_SYS_NAND_ECCTOTAL; i++) ecc_code[i] = oob_data[nand_ecc_pos[i]];
Regards, Stefano