Hi, On Fri, Sep 4, 2009 at 7:44 PM, Wolfgang Denk<w...@denx.de> wrote: > Dear Minkyu Kang, > > In message <4aa0ce3f.60...@samsung.com> you wrote: >> This patch includes the onenand driver for s5pc1xx >> >> Signed-off-by: Minkyu Kang <mk7.k...@samsung.com> >> Signed-off-by: Kyungmin Park <kyungmin.p...@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.
I have different opinion. How much register map is required? I mean in case of read/write cmd, it has about 256MiB range. then how do you cast is as C structures. Only some small range for example. 1K or less. C structures possible. but more than this. it's too huge to case it. > > 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. It's mandatory. no need to shift. It's fixed. with this reason. we covers alomst 256MiB memory range. > > ... >> +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 &&" Agreed. > > ... >> +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? Now we got two version of samsung.c one for s3c6410, another is this one. No problem to add it for s3c6410 > >> 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. The end address is 0x400. Are you really need it to cast? I don't think so. Thank you, Kyungmin Park _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot