[U-Boot] [PATCH 3/4 V2] OneNAND: Add simple OneNAND SPL

[Scott Wood]

On 11/01/2011 05:54 PM, Marek Vasut wrote:
> +static void spl_onenand_get_geometry(struct spl_onenand_data *data)
> +{
> +	uint32_t tmp;
> +	uint32_t dev_id, density;
> +
> +	/* Default geometry -- 2048b page, 128k erase block. */
> +	data->pagesize = 2048;
> +	data->erasesize = 0x20000;
> +
> +	tmp = onenand_readw(ONENAND_REG_TECHNOLOGY);
> +	if (tmp)
> +		goto dev_4k;
> +
> +	dev_id = onenand_readw(ONENAND_REG_DEVICE_ID);
> +	density = dev_id >> ONENAND_DEVICE_DENSITY_SHIFT;
> +	density &= ONENAND_DEVICE_DENSITY_MASK;
> +
> +	if (density < ONENAND_DEVICE_DENSITY_4Gb)
> +		return;
> +
> +	if (dev_id & ONENAND_DEVICE_IS_DDP)
> +		return;
> +
> +	/* 4k device geometry -- 4096b page, 256k erase block. */
> +dev_4k:
> +	data->pagesize = 4096;
> +	data->erasesize = 0x40000;
> +}

Drop the goto and "tmp" variable, just do:

	if (!onenand_readw(ONENAND_REG_TECHNOLOGY)) {
		dev_id = onenand_readw(ONENAND_REG_DEVICE_ID);
		density = dev_id >> ONENAND_DEVICE_DENSITY_SHIFT;
		density &= ONENAND_DEVICE_DENSITY_MASK;

		if (density < ONENAND_DEVICE_DENSITY_4Gb)
			return;

		if (dev_id & ONENAND_DEVICE_IS_DDP)
			return;
	}

> +int spl_onenand_load_image(uint32_t dst, uint32_t offset, uint32_t len)

Please use the same name and arguments as nand_spl_load_image() in
nand_spl_simple.c.

> +{
> +	uint32_t *addr = (uint32_t *)dst;

Why not pass it in as a pointer in the first place?  I know U-Boot is
unlkely to support being built as 64-bit any time soon, but why
introduce gratuitous 64-bit-uncleanliness?

> +	struct spl_onenand_data data;
> +	uint32_t total_pages;
> +	uint32_t block;
> +	uint32_t page, rpage;
> +	int ret, err = 0;
> +
> +	spl_onenand_get_geometry(&data);
> +
> +	/* The page can be either 2k or 4k, avoid using DIV_ROUND_UP. */
> +	if (data.pagesize == 2048) {
> +		total_pages = len / 2048;
> +		page = offset / 2048;
> +		total_pages += !!(len & 2047);
> +	} else if (data.pagesize == 4096) {
> +		total_pages = len / 4096;
> +		page = offset / 4096;
> +		total_pages += !!(len & 4095);
> +	}

What's wrong with DIV_ROUND_UP?  It should produce smaller code than
what you've done here...

> +	for (; page <= total_pages; page++) {
> +		block = page / ONENAND_PAGES_PER_BLOCK;
> +		rpage = page & (ONENAND_PAGES_PER_BLOCK - 1);
> +		ret = spl_onenand_read_page(block, rpage, addr, data.pagesize);
> +		if (ret) {
> +			total_pages++;
> +			err |= 1;
> +		} else
> +			addr += data.pagesize / 4;
> +	}

As discussed, please retain the existing block-skipping semantics.  And
if you do skip a block, that's not an error to be propagated upward (as
opposed to something like an uncorrectable ECC error).

If one side of an if statement requires braces, both sides should have them.

> diff --git a/include/onenand_uboot.h b/include/onenand_uboot.h
> index 92279d5..fcb50ff 100644
> --- a/include/onenand_uboot.h
> +++ b/include/onenand_uboot.h
> @@ -16,6 +16,8 @@
>  
>  #include <linux/types.h>
>  
> +#ifndef	CONFIG_SPL_BUILD
> +

Please use a space rather than a tab after #define, #ifndef, etc.

>  /* Forward declarations */
>  struct mtd_info;
>  struct mtd_oob_ops;
> @@ -52,4 +54,10 @@ extern int flexonenand_set_boundary(struct mtd_info *mtd, int die,
>  extern void s3c64xx_onenand_init(struct mtd_info *);
>  extern void s3c64xx_set_width_regs(struct onenand_chip *);
>  
> +#else
> +
> +int spl_onenand_load_image(uint32_t dst, uint32_t offset, uint32_t len);
> +
> +#endif

Why does this need to be ifdeffed at all?  We normally don't ifdef
header declarations.

-Scott