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

[Chin Liang See]

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.

> 
> 
> 
> > > +}
> > > +
> > > +int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst)
> > > +{
> > > +	int block, page, column, readlen;
> > > +	int ret;
> > > +	int force_bad_block_check = 1;
> > > +
> > > +	page = offs / page_size;
> > > +	column = offs % page_size;
> > > +
> > > +	block = page / pages_per_block;
> > > +	page = page % pages_per_block;
> > > +
> > > +	while (size) {
> > 
> > I believe we need to error out when reading beyond last block.
> 
> 
> How do you know the number of blocks of non-ONFI devices?
> Scan nand_ids.c for SPL?
> 

I believe we can have a macro for this. But rethinking back, we can skip
this check to make the SPL as simple as possible.

> > 
> 
> > > +		if (force_bad_block_check || page == 0) {
> > > +			ret = nand_block_isbad(block);
> > > +			if (ret < 0)
> > > +				return ret;
> > > +
> > > +			if (ret) {
> > > +				block++;
> > > +				continue;
> > > +			}
> > > +		}
> > > +
> > > +		force_bad_block_check = 0;
> > 
> > I believe we still need to check the subsequent block whether is bad or
> > not too. This can be enable when cross the block boundary.
> 
> 
> I am afraid you are misunderstanding my code.
> 
> This function does bad block checking for every block.
> 
> Here,
> 
> 		if (force_bad_block_check || page == 0) {
> 			ret = nand_block_isbad(block);
> 
> 
> "page == 0" means the beginning of each block.
> 
> 

Yup, you are right. I was confused with the variable.

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

Thanks
Chin Liang

> 
> Best Regards
> Masahiro Yamada
>