[U-Boot] [PATCH v2 1/3] sunxi: nand: Add basic sunxi NAND driver for SPL with DMA support

[Piotr Zierhoffer]

Hi Boris,

thanks for your review. I have applied most of your comments, but I
have few remarks and questions.

2015-07-20 18:13 GMT+02:00 Boris Brezillon <boris.brezillon at free-electrons.com>:
>> +     page = real_addr / CONFIG_SYS_NAND_BLOCK_SIZE;
>> +     column = real_addr % CONFIG_SYS_NAND_BLOCK_SIZE;
>> +
>> +     if (syndrome) {
>> +             /* shift every 1kB in syndrome */
>
> Well, this scheme is not really related to the ECC syndrome scheme,
> it comes from the BROM implementation which can only really 1K of data
> per page.
> Actually, this is not exactly true, depending on the BROM version it
> will try different things, see this description [2].
> So once more, you're making assumptions that could be wrong on some
> boards.

Actually, I have only one board and I wouldn't like to submit code
that is supposed to be general and was never really tested.

I'd suggest removing the comment and making the following options
configurable, with the default values as provided, in Kconfig:
CONFIG_NAND_PAGE_SIZE 0x2000
CONFIG_NAND_ECC_PAGE_SIZE 0x400
CONFIG_NAND_SUNXI_ECC_STRENGTH 40
CONFIG_NAND_SYNDROME_PARTITIONS_END 0x400000

Do you think that would be sensible?

>> +int nand_spl_load_image(uint32_t offs, unsigned int size, void *dest)
>> +{
>> +     void *current_dest;
>> +     uint32_t count;
>> +     uint32_t current_count;
>> +
>> +     memset(dest, 0x0, size); /* clean destination memory */
>> +     ecc_errors = 0;
>> +     for (current_dest = dest;
>> +                     current_dest < (dest + size);
>> +                     current_dest += CONFIG_SYS_NAND_PAGE_SIZE) {
>> +             nand_read_page(offs, offs < SYNDROME_PARTITIONS_END);
>> +             count = current_dest - dest;
>> +
>> +             if (size - count > CONFIG_SYS_NAND_PAGE_SIZE)
>> +                     current_count = CONFIG_SYS_NAND_PAGE_SIZE;
>> +             else
>> +                     current_count = size - count;
>> +
>> +             memcpy(current_dest,
>> +                    temp_buf,
>> +                    current_count);
>> +             offs += CONFIG_SYS_NAND_PAGE_SIZE;
>> +     }
>> +     return ecc_errors;
>
> I'm not sure what's the exact convention for nand_spl_load_image return
> code, but I'd say that returning a negative error code if ecc_errors !=
> 0 is better than returning the number of ECC errors.
>

>From my observations it seems that there is no established convention, but
returning -1 as an error indicator can be found here and there, so I
may do that.

It's not really interpreted anywhere, though.

>
> Are all these NFC_ macros used outside of the driver itself.
> If that's not the case then I would recommend moving them direcly in
> the .c file.
>

In general you may find that constants regarding NANDs are kept in .h files
around U-Boot, so I'd like stick with that convention.

> Also, I haven't checked if the MACRO names are matching the one defined
> in the linux driver, but if that's not the case then I would recommend
> using the same definition since the final goal is to port the Linux
> driver to u-boot (I know you're just implementing the SPL part, but
> since you moved your code into drivers/mtd/nand/sunxi_nand* I'll have
> to merge the Linux implementation into this file).
>

I have made the macros consistent, but unfortunately I won't be able to verify
them all with the Linux drivers, due to time constraints.
It can always be done later as a part of a separate patchset with a
Linux driver.

I will submit another version of this patchset later today.

I will follow your suggestion to create a new thread for that.

> Boris
>

Best regards


*Piotr Zierhoffer*
Antmicro Ltd | www.antmicro.com