[U-Boot-Users] [PATCH 3/5] OneNAND support

[Wolfgang Denk]

In message <20070907005923.GA19912 at party> you wrote:
> [PATCH] OneNAND support
> 
> Signed-off-by: Kyungmin Park <kyungmin.park at samsung.com>
> ---
> diff --git a/drivers/onenand/onenand_base.c b/drivers/onenand/onenand_base.c
> new file mode 100644
> index 0000000..997bd00
> --- /dev/null
> +++ b/drivers/onenand/onenand_base.c
...
> +#ifdef __ARM__
> +extern void memcpy32(void *, void *, size_t);
> +
> +static void *memcpy(void *dest, const void *src, size_t count)
> +{
> +	if (count < 32) {
> +		unsigned short *_s = (unsigned short *)(src);
> +		unsigned short *_d = (unsigned short *)(dest);
> +		count >>= 1;
> +		while (count--)
> +			*_d++ = *_s++;
> +		return _d;
> +	}
> +
> +	memcpy32(dest, (void *)src, count);
> +
> +	return (void *)count;
> +}
> +#endif

Why exactly do we need this? And why do we need an additonal  special
function memcpy32?

> +static const unsigned char ffchars[] = {
> +	0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +	0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,	/* 16 */
> +	0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +	0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,	/* 32 */
> +	0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +	0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,	/* 48 */
> +	0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> +	0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,	/* 64 */
> +};

Using a dynamic buffer and a memset() call in the init code may helpt
to reduce the memory footprint. What do you think?

> +static unsigned short onenand_readw(void __iomem * addr)
> +{
> +	return readw(addr);
> +}
...
> +static void onenand_writew(unsigned short value, void __iomem * addr)
> +{
> +	writew(value, addr);
> +}

If this is all you do, you should probably use an inline or a macro.

> + * Get the device and lock it for exclusive access
> + */
> +static void onenand_get_device(struct mtd_info *mtd, int new_state)
> +{
> +	/* Do nothing */
> +}
...
> + * Deselect, release chip lock and wake up anyone waiting on the device
> + */
> +static void onenand_release_device(struct mtd_info *mtd)
> +{
> +	/* Do nothing */
> +}

Inline? Macro?

> +int onenand_read(struct mtd_info *mtd, loff_t from, size_t len,
> +		 size_t * retlen, u_char * buf)
> +{
> +	return onenand_read_ecc(mtd, from, len, retlen, buf, NULL, NULL);
> +}

Inline? Macro?

> +int onenand_write(struct mtd_info *mtd, loff_t to, size_t len,
> +		  size_t * retlen, const u_char * buf)
> +{
> +	return onenand_write_ecc(mtd, to, len, retlen, buf, NULL, NULL);
> +}

Inline? Macro?

> +int onenand_block_isbad(struct mtd_info *mtd, loff_t ofs)
> +{
> +	/*
> +	 * TODO
> +	 * 1. Bad block table (BBT)
> +	 *   -> using NAND BBT to support JFFS2
> +	 * 2. Bad block management (BBM)
> +	 *   -> bad block replace scheme
> +	 *
> +	 * Currently we do nothing
> +	 */
> +	return 0;
> +}
> +
> +/**
> + * onenand_block_markbad - [MTD Interface] Mark the block at the given offset as bad
> + * @param mtd		MTD device structure
> + * @param ofs		offset relative to mtd start
> + */
> +int onenand_block_markbad(struct mtd_info *mtd, loff_t ofs)
> +{
> +	/* see above */
> +	return 0;
> +}

Do we need these, then?

> +/**
> + * onenand_release - [OneNAND Interface] Free resources held by the OneNAND device
> + * @param mtd		MTD device structure
> + */
> +void onenand_release(struct mtd_info *mtd)
> +{
> +}

Do we need this, then?


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I think that all right-thinking people in this country are  sick  and
tired  of  being  told that ordinary decent people are fed up in this
country with being sick and tired. I'm certainly not.  But  I'm  sick
and tired of being told that I am.                     - Monty Python