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

[Kyungmin Park]

Hi,

On Fri, Sep 4, 2009 at 7:44 PM, Wolfgang Denk<wd at denx.de> wrote:
> 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.

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