[U-Boot] [PATCH v3 09/11] S3C24XX: Add NAND Flash driver

["José Miguel Gonçalves" <jose.goncalves@inov.pt>]

On 18-09-2012 19:30, Scott Wood wrote:
> On 09/18/2012 01:22:58 PM, Jos? Miguel Gon?alves wrote:
>> On 18-09-2012 19:02, Scott Wood wrote:
>>> On 09/18/2012 12:40:36 PM, Jos? Miguel Gon?alves wrote:
>>>> +#define TACLS_VAL    7    /* CLE & ALE duration setting (0~7) */
>>>> +#define    TWRPH0_VAL    7    /* TWRPH0 duration setting (0~7) */
>>>> +#define TWRPH1_VAL    7    /* TWRPH1 duration setting (0~7) */
>>>
>>> Please use space, not tab, as a word separator (after the second #define).
>>
>> OK.
>>
>>>
>>>> +
>>>> +#define MAX_CHIPS    2
>>>> +static int nand_cs[MAX_CHIPS] = { 0, 1 };
>>>
>>> This needs explanation (and const).  Better would be to use a priv struct, as 
>>> discussed before.
>>>
>>>> +static void s3c_nand_select_chip(struct mtd_info *mtd, int chip)
>>>> +{
>>>> +    struct s3c24xx_nand *const nand = s3c24xx_get_base_nand();
>>>> +    u_long nfcont;
>>>
>>> s/u_long/u32/
>>
>> Didn't catch this...
>
> Replace u_long with u32.

OK (didn't expected a regex expression in that context...)

>
>>>> +    nfcont = readl(&nand->nfcont);
>>>> +
>>>> +    switch (chip) {
>>>> +    case -1:
>>>> +        nfcont |= NFCONT_NCE1 | NFCONT_NCE0;
>>>> +        break;
>>>> +    case 0:
>>>> +        nfcont &= ~NFCONT_NCE0;
>>>> +        break;
>>>> +    case 1:
>>>> +        nfcont &= ~NFCONT_NCE1;
>>>> +        break;
>>>> +    default:
>>>> +        return;
>>>
>>> error message on default?
>>
>> OK.
>>
>>>
>>>> +    }
>>>> +
>>>> +    writel(nfcont, &nand->nfcont);
>>>> +}
>>>> +
>>>> +/*
>>>> + * Hardware specific access to control-lines function
>>>> + */
>>>> +static void s3c_nand_hwcontrol(struct mtd_info *mtd, int cmd, unsigned int 
>>>> ctrl)
>>>> +{
>>>> +    struct s3c24xx_nand *const nand = s3c24xx_get_base_nand();
>>>> +    struct nand_chip *this = mtd->priv;
>>>> +
>>>> +    if (ctrl & NAND_CTRL_CHANGE) {
>>>> +        if (ctrl & NAND_CLE)
>>>> +            this->IO_ADDR_W = &nand->nfcmmd;
>>>> +        else if (ctrl & NAND_ALE)
>>>> +            this->IO_ADDR_W = &nand->nfaddr;
>>>> +        else
>>>> +            this->IO_ADDR_W = &nand->nfdata;
>>>> +        if (ctrl & NAND_NCE)
>>>> +            s3c_nand_select_chip(mtd, *(int *)this->priv);
>>>> +        else
>>>> +            s3c_nand_select_chip(mtd, -1);
>>>> +    }
>>>> +
>>>> +    if (cmd != NAND_CMD_NONE)
>>>> +        writeb(cmd, this->IO_ADDR_W);
>>>> +}
>>>
>>> As discussed earlier, do you really need to mess with IO_ADDR_W or can you do 
>>> it the way ndfc.c does?
>>
>> I will take a look at ndfc.c.
>
>
>
> Most of this driver was copy-paste from s3c64xx.c driver and an older patched 
> U-Boot sources from Samsung, so I did not make any real code examination after 
> it started to work...
>>
>>>
>>> I.e.:
>>>
>>> static void s3c_nand_hwcontrol(struct mtd_info *mtd, int cmd, unsigned int ctrl)
>>> {
>>>     struct s3c24xx_nand *const nand = s3c24xx_get_base_nand();
>>>     struct nand_chip *this = mtd->priv;
>>>
>>>     if (cmd == NAND_CMD_NONE)
>>>         return;
>>>
>>>     if (ctrl & NAND_CLE)
>>>         writeb(cmd, &nand->nfcmmd);
>>>     else
>>>         writeb(cmd, &nand->nfaddr);
>>> }
>
> Oh sorry -- the above is missing the NCE part.  Before the NAND_CMD_NONE check 
> insert:
>
>     if (ctrl & NAND_CTRL_CHANGE) {
>         if (ctrl & NAND_NCE)
>             s3c_nand_select_chip(mtd, *(const int *)this->priv);
>         else
>             s3c_nand_select_chip(mtd, -1);
>     }
>
>
>
>>>
>>>> +/*
>>>> + * Board-specific NAND initialization.
>>>> + */
>>>> +int board_nand_init(struct nand_chip *nand)
>>>> +{
>>>> +    static int chip_n;
>>>> +    struct s3c24xx_nand *const nand_reg = s3c24xx_get_base_nand();
>>>> +
>>>> +    if (chip_n == 0) {
>>>> +        /* Extend NAND timings to the maximum */
>>>> +        clrsetbits_le32(&nand_reg->nfconf,
>>>> +                NFCONF_TACLS_MASK | NFCONF_TWRPH0_MASK |
>>>> +                NFCONF_TWRPH1_MASK,
>>>> +                NFCONF_TACLS(TACLS_VAL) |
>>>> +                NFCONF_TWRPH0(TWRPH0_VAL) |
>>>> +                NFCONF_TWRPH1(TWRPH1_VAL));
>>>> +
>>>> +        /* Disable chip selects and soft lock, enable controller */
>>>> +        clrsetbits_le32(&nand_reg->nfcont, NFCONT_WP,
>>>> +                NFCONT_NCE1 | NFCONT_NCE0 | NFCONT_ENABLE);
>>>> +    } else if (chip_n >= MAX_CHIPS) {
>>>> +        return -ENODEV;
>>>> +    }
>>>> +
>>>> +    nand->IO_ADDR_R = &nand_reg->nfdata;
>>>> +    nand->IO_ADDR_W = &nand_reg->nfdata;
>>>> +    nand->cmd_ctrl = s3c_nand_hwcontrol;
>>>> +    nand->dev_ready = s3c_nand_device_ready;
>>>> +    nand->select_chip = s3c_nand_select_chip;
>>>> +    nand->options = 0;
>>>> +#ifdef CONFIG_SPL_BUILD
>>>> +    nand->read_buf = nand_read_buf;
>>>> +#endif
>>>> +
>>>> +#ifdef CONFIG_S3C24XX_NAND_HWECC
>>>> +    nand->ecc.hwctl = s3c_nand_enable_hwecc;
>>>> +    nand->ecc.calculate = s3c_nand_calculate_ecc;
>>>> +    nand->ecc.correct = s3c_nand_correct_data;
>>>> +    nand->ecc.mode = NAND_ECC_HW_OOB_FIRST;
>>>> +    nand->ecc.size = CONFIG_SYS_NAND_ECCSIZE;
>>>> +    nand->ecc.bytes = CONFIG_SYS_NAND_ECCBYTES;
>>>> +#else
>>>> +    nand->ecc.mode = NAND_ECC_SOFT;
>>>> +#endif /* ! CONFIG_S3C24XX_NAND_HWECC */
>>>> +
>>>> +    nand->priv = nand_cs + chip_n++;
>>>> +
>>>> +    return 0;
>>>> +}
>>>
>>> Please consider using the new SELF_INIT mechanism.
>>>
>>
>> Can you explain and/or point_to_resources for what this means (I'm a U-Boot 
>> newbie)...
>
> See CONFIG_SYS_NAND_SELF_INIT in doc/README.nand
>
> I'd like the old way to be removed at some point.

 From what I've seen there is an incompatibility between the SPL simple nand 
driver and the SELF_INIT mechanism.
The nand_init() function in drivers/mtd/nand/nand_spl_simple.c calls 
board_nand_init() with a pointer to a struct nand_chip as an argument (the old 
way). This needs to be changed to be able to use that SPL driver. Can you give me 
an hint on the proper way to handle this?

Best regards,
Jos? Gon?alves