[U-Boot] [PATCH 2/4] s5pc1xx: support onenand driver

[Wolfgang Denk]

Dear Minkyu Kang,

In message <4AA0CE3F.60304 at samsung.com> you wrote:
> This patch includes the onenand driver for s5pc1xx
> 
> Signed-off-by: Minkyu Kang <mk7.kang at samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park at samsung.com>
...
> +static int s3c_read_reg(int offset)
> +{
> +	return readl(onenand->base + offset);
> +}
> +
> +static void s3c_write_reg(int value, int offset)
> +{
> +	writel(value, onenand->base + offset);
> +}
> +
> +static int s3c_read_cmd(unsigned int cmd)
> +{
> +	return readl(onenand->ahb_addr + cmd);
> +}
> +
> +static void s3c_write_cmd(int value, unsigned int cmd)
> +{
> +	writel(value, onenand->ahb_addr + cmd);
> +}

PLease see comments to previous patch about usage of register plus
offset versus using proper C structures.

Please change this code to use structs, too.

> +static unsigned int s5pc100_mem_addr(int fba, int fpa, int fsa)
> +{
> +	return (fba << 13) | (fpa << 7) | (fsa << 5);
> +}

This function needs explanation. Please add a comment what it does,
and how.

...
> +static int s3c_onenand_bbt_wait(struct mtd_info *mtd, int state)
> +{
> +	unsigned int flags = INT_ACT | LOAD_CMP;
> +	unsigned int stat;
> +	unsigned long timeout = 0x10000;
> +
> +	while (1 && timeout--) {

Please do not add bogus code. Remove the "1 &&"

...
> +void s3c_onenand_init(struct mtd_info *mtd)
> +{
> +	struct onenand_chip *this = mtd->priv;
> +
> +	onenand = malloc(sizeof(struct s3c_onenand));
> +	if (!onenand)
> +		return;
> +
> +	onenand->page_buf = malloc(SZ_4K * sizeof(char));
> +	if (!onenand->page_buf)
> +		return;
> +	memset(onenand->page_buf, 0xFF, SZ_4K);
> +
> +	onenand->oob_buf = malloc(128 * sizeof(char));
> +	if (!onenand->oob_buf)
> +		return;
> +	memset(onenand->oob_buf, 0xFF, 128);
> +
> +	onenand->mtd = mtd;
> +
> +#ifdef CONFIG_S5PC1XX
> +	/* S5PC100 specific values */
> +	onenand->base = (void *) 0xE7100000;
> +	onenand->ahb_addr = (void *) 0xB0000000;
> +	onenand->mem_addr = s5pc100_mem_addr;
> +#else
> +#error Please fix it at s3c6410
> +#endif

What does this mean?

> diff --git a/include/samsung_onenand.h b/include/samsung_onenand.h
> new file mode 100644
> index 0000000..91b40e1
> --- /dev/null
> +++ b/include/samsung_onenand.h
...
> +/*
> + * OneNAND Controller
> + */
> +#define MEM_CFG_OFFSET		0x0000
> +#define BURST_LEN_OFFSET	0x0010
> +#define MEM_RESET_OFFSET	0x0020
> +#define INT_ERR_STAT_OFFSET	0x0030
> +#define INT_ERR_MASK_OFFSET	0x0040
> +#define INT_ERR_ACK_OFFSET	0x0050

Please use a C struct instead.


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
Lack of skill dictates economy of style.                - Joey Ramone