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

[Scott Wood <scottwood@freescale.com>]

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.

>>> +    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.

-Scott