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

[Boris Brezillon <boris.brezillon@free-electrons.com>]

Hi Piotr,

On Wed, 22 Jul 2015 13:27:37 +0200
Piotr Zierhoffer <pzierhoffer@antmicro.com> wrote:

> 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@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 understand, but I was not talking about testing your code on all
existing boards, just making your code generic enough so that other
users can test on their platforms without having to modify the SPL code.
Of course if they detect that something is buggy or missing, they will
provide follow-up patches to fix those problems.

> 
> 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?

Yep, that pretty much what I was expecting. Just put those definition
into a board config header, or add a Kconfig entry.

> 
> >> +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.

Okay, then I'll let u-boot maintainers decide what should be done.

> 
> >
> > 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.

This is true for all generic definitions, not for driver specific ones.

> 
> > 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.

How about copying the definition from the Linux driver and fixing the
mismatch in your implementation ?

> It can always be done later as a part of a separate patchset with a
> Linux driver.

Hm, the merge will be even more complicated if you do that.
Could you at least move your code to sunxi_nand_spl.c so that we can
directly apply the linux commit adding support for the sunxi NAND
controller ?

Thanks.

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com