[U-Boot] [PATCH 3/6] mtd: nand: Add the sunxi NAND controller driver

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

On Mon, 06 Jun 2016 18:54:03 -0500
Scott Wood <oss@buserror.net> wrote:

> On Mon, 2016-06-06 at 20:31 +0200, Boris Brezillon wrote:
> > On Mon, 06 Jun 2016 12:56:48 -0500
> > Scott Wood <oss@buserror.net> wrote:
> >   
> > > On Mon, 2016-06-06 at 18:22 +0200, Boris Brezillon wrote:  
> > > > On Mon, 6 Jun 2016 17:36:10 +0200
> > > > Hans de Goede <hdegoede@redhat.com> wrote:
> > > >     
> > > > > > +#ifndef CONFIG_SPL_BUILD
> > > > > > +void sunxi_nand_init(void);
> > > > > > +#endif
> > > > > > +      
> > > > > 
> > > > > Can we have this in a header somewhere please, and without
> > > > > the #ifdef around it, that is not necessary for prototypes.    
> > > > 
> > > > Hehe, I was expecting this one :-). Do you know where I should put this
> > > > prototype definition? A board.h file in board/sunxi/?    
> > > 
> > > It's defined in drivers/mtd/nand and called from board/sunxi so the
> > > prototype
> > > needs to go somewhere under include/.  It can go in include/configs/sunxi
> > > -common.h with #ifndef __ASSEMBLY__ around it -- or as long as it's
> > > limited to
> > > one init func per driver, maybe we could just put it in include/nand.h.  
> > 
> > Hm, none of these solutions seem ideal.
> > 
> > Maybe we could define a generic void nand_controller_init(void)
> > prototype so that we don't need to add new xxx_nand_init() functions for
> > platforms needing this 2 steps initialization (platform specific pinmux
> > + clocks config before NAND controller initialization).
> > 
> > Otherwise, I think I'll go for the 2nd solution (defining
> > sunxi_nand_init() in include/nand.h).  
> 
> I'd prefer not having a generic nand_controller_init() because some platforms
> may want to pass arguments, plus I don't want to rule out the possibility of
> two different NAND controller types being supported at once.
> 
> include/nand.h is fine with me, as long as any driver than wants more than an
> initfunc moves its stuff into a dedicated header.

Sure.

> Of course the driver model
> is probably the long-term solution.

Definitely, and talking about things that need to be reworked, do you
know why u-boot is using its own MTD partition infrastructure instead
of relying on mtdpart.c?
That's really a pain when one wants to add a new feature (like
definitions of partitions in the DT, or SLC mode on MLC NANDs) because
he has to do it twice.

And that's not the only inconsistent part in the MTD/NAND layer IMO.
MTD is providing a generic abstraction for all flashes, but nand_util
is still directly accessing the NAND layer instead of going through the
MTD abstraction.
By using the MTD abstraction everywhere (I mean for all flash devices),
we could provide generic utils (flash erase, flash write), even if
specific tools might be needed in a few cases.

Anyway, good to hear that you plan to switch to the driver model.

Regards,

Boris


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