[U-Boot] [PATCH v4 1/6] nand: denali: add Denali NAND driver for SPL

[Chin Liang See]

Hi Masahiro,

On Wed, 2014-09-17 at 18:09 +0900, Masahiro Yamada wrote:
> Hi Chin,
> 
> 
> On Mon, 15 Sep 2014 01:39:07 -0500
> Chin Liang See <clsee at altera.com> wrote:
> 
> > Hi Masahiro,
> > 
> > On Fri, 2014-09-12 at 17:06 +0900, Masahiro Yamada wrote:
> > 
> > > > > +/* nand_init() - initialize data to make nand usable by SPL */
> > > > > +void nand_init(void)
> > > > > +{
> > > > > +	/* access to main area */
> > > > > +	writel(0, denali_flash_reg + TRANSFER_SPARE_REG);
> > > > > +
> > > > > +	page_size = readl(denali_flash_reg + DEVICE_MAIN_AREA_SIZE);
> > > > > +	oob_size = readl(denali_flash_reg + DEVICE_SPARE_AREA_SIZE);
> > > > > +	pages_per_block = readl(denali_flash_reg + PAGES_PER_BLOCK);
> > > > 
> > > > 
> > > > I believe this will work for ONFI NAND devices only.
> > > > For non-ONFI, the value might not correct.
> > > 
> > > 
> > > I don't think so.
> > > It depends on the hardware; in my understanding
> > > Denali IP is capable of detecting MAIN_AREA_SIZE etc.
> > > for non-ONFI devices.  At least this is working with non-ONFI devices
> > > on some Panasonic boards.
> > > 
> > > If it does not work for Altera SoCs (and if you are planning to use
> > > this driver), these three registers should be set in advance
> > > in an earlier board init.
> > > 
> > 
> > I recall one of my colleague was telling me that it doesn't work for one
> > of non ONFI part where it read incorrect page size. Nevertheless, we can
> > put comments so user which use this driver need to take note.
> 
> I have also heard of that.  I think it happens on some Hynix devices.
> 
> According to my colleague, for the device with
> maf_id = 0xAD, device_id = 0xDA,
> the Denali IP seems to set wrong parameters.
> 
> Either comments or fixup code like get_hynix_nand_para() in denali.c
> will do.
> 
> BTW I've noticed only a few Hynix devices are supported by denali.c.
> (only the device id = 0xD5 or 0xD7)
> 
> This is, anyway, a upstream limitation and we can send a follow up
> patch if needed.
> 
> 

Yup, that sound a good plan to enable this patch to move forward. We
just need to add some write-up to the commit message.


> 
> 
> > > 
> > > 
> > > > 
> > > > 
> > > > Currently U-Boot has drivers/mtd/nand/nand_spl_simple.c which handling
> > > > the SPL NAND image load. Wonder this driver will be integrated into
> > > > nand_spl_simple.c once drivers/mtd/nand/denali.c is applied?
> > > 
> > > I am not planning to do so because:
> > > 
> > > [1] nand_spl_simple.c requires CONFIG_SYS_NAND_BLOCK_SIZE, CONFIG_SYS_NAND_PAGE_SIZE,
> > > CONFIG_SYS_NAND_PAGE_COUNT; we need to specify the device attributes at compilation,
> > > which the Denali IP is able to detect at run time.
> > > It is not acceptable for us because we need (want) the run time configuration.
> > > 
> > > [2] nand_spl_simple.c is so generic that it cannot use the hardware acceleration of
> > > the Denali IP, that is, slower booting.
> > > 
> > 
> > Yup, you identified the nand_spl_simple.c constrain. This is why I
> > patched this file at
> > http://rocketboards.org/gitweb/?p=u-boot-socfpga.git;a=commit;h=461a61b8f03d3b690de1f4ff007cd23fb80018a5. But I didn't send this patch out as I am waiting the NAND driver patch accepted.
> 
> 
> After applying your patch, both [1] and [2] are still the constrain of nand_spl_simple.c
> 

For [1], yup. But the intention is to ensure it works for various
devices and NAND controllers. While for [2], the patch will take
advantage of the hardware based ECC calculation. But it will not take
advantage of the DMA as I believe DMA might not exist for certain NAND
controller. 

With that, I don't have much concern to remain this file as this would
enable better performance as its specific for Denali. Probably Altera
should use this too if its accepted into mainline :)

Thanks
Chin Liang

> 
> Best Regards
> Masahiro Yamada
>