Re: [PATCH] [MTD] MXC NAND driver fixes

Re: [PATCH] [MTD] MXC NAND driver fixes

[Sascha Hauer]

Hi,

On Fri, Apr 03, 2009 at 01:54:20AM +0400, Vladimir Barinov wrote:
> The following patch fixes:
>  - re-initialization of host->col_addr which is used as byte index
>    between the successive READID flash commands.
>  - compile error when CONFIG_PM is enabled
>  - pass on the error code from clk_get()
>  - return -ENOMEM in case of failed ioremap()
>  - pass on the return value of platform_driver_probe() instead of inventing -ENODEV
>  - let command line partition table parsing with mxc_nand name.
>    The cmd_line parsing is done via <mtd-id> name that differs
>    from mxc_nand by default and looks like "NAND 256MiB 1,8V 8-bit"
> 
> Signed-off-by: Vladimir Barinov <vbarinov@embeddedalley.com>
> Signed-off-by: Lothar Wassmann <LW@KARO-electronics.de>
> ---
>  drivers/mtd/nand/mxc_nand.c |   47 +++++++++++++++++++++++++++---------------
>  1 files changed, 30 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> index bad048a..e94c956 100644
> --- a/drivers/mtd/nand/mxc_nand.c
> +++ b/drivers/mtd/nand/mxc_nand.c
> @@ -831,6 +831,7 @@ static void mxc_nand_command(struct mtd_info *mtd, unsigned command,
>  		break;
>  
>  	case NAND_CMD_READID:
> +		host->col_addr = 0;
>  		send_read_id(host);
>  		break;
>  
> @@ -881,8 +882,10 @@ static int __init mxcnd_probe(struct platform_device *pdev)
>  	this->verify_buf = mxc_nand_verify_buf;
>  
>  	host->clk = clk_get(&pdev->dev, "nfc");
> -	if (IS_ERR(host->clk))
> +	if (IS_ERR(host->clk)) {
> +		err = PTR_ERR(host->clk);
>  		goto eclk;
> +	}
>  
>  	clk_enable(host->clk);
>  	host->clk_act = 1;
> @@ -895,7 +898,7 @@ static int __init mxcnd_probe(struct platform_device *pdev)
>  
>  	host->regs = ioremap(res->start, res->end - res->start + 1);
>  	if (!host->regs) {
> -		err = -EIO;
> +		err = -ENOMEM;
>  		goto eres;
>  	}
>  
> @@ -964,6 +967,9 @@ static int __init mxcnd_probe(struct platform_device *pdev)
>  
>  	/* Register the partitions */
>  #ifdef CONFIG_MTD_PARTITIONS
> +#ifdef CONFIG_MTD_CMDLINE_PARTS
> +	mtd->name = "mxc_nand";
> +#endif

Do we need an ifdef here?

>  	nr_parts =
>  	    parse_mtd_partitions(mtd, part_probes, &host->parts, 0);
>  	if (nr_parts > 0)
> @@ -1010,30 +1016,35 @@ static int __devexit mxcnd_remove(struct platform_device *pdev)
>  #ifdef CONFIG_PM
>  static int mxcnd_suspend(struct platform_device *pdev, pm_message_t state)
>  {
> -	struct mtd_info *info = platform_get_drvdata(pdev);
> +	struct mtd_info *mtd = platform_get_drvdata(pdev);
> +	struct nand_chip *nand_chip = mtd->priv;
> +	struct mxc_nand_host *host = nand_chip->priv;
>  	int ret = 0;
>  
>  	DEBUG(MTD_DEBUG_LEVEL0, "MXC_ND : NAND suspend\n");
> -	if (info)
> -		ret = info->suspend(info);
> -
> -	/* Disable the NFC clock */
> -	clk_disable(nfc_clk);	/* FIXME */
> +	if (mtd) {
> +		ret = mtd->suspend(mtd);
> +		/* Disable the NFC clock */
> +		clk_disable(host->clk);
> +	}
>  
>  	return ret;
>  }
>  
>  static int mxcnd_resume(struct platform_device *pdev)
>  {
> -	struct mtd_info *info = platform_get_drvdata(pdev);
> +	struct mtd_info *mtd = platform_get_drvdata(pdev);
> +	struct nand_chip *nand_chip = mtd->priv;
> +	struct mxc_nand_host *host = nand_chip->priv;
>  	int ret = 0;
>  
>  	DEBUG(MTD_DEBUG_LEVEL0, "MXC_ND : NAND resume\n");
> -	/* Enable the NFC clock */
> -	clk_enable(nfc_clk);	/* FIXME */
>  
> -	if (info)
> -		info->resume(info);
> +	if (mtd) {
> +		/* Enable the NFC clock */
> +		clk_enable(host->clk);
> +		mtd->resume(mtd);
> +	}
>  
>  	return ret;
>  }
> @@ -1054,13 +1065,15 @@ static struct platform_driver mxcnd_driver = {
>  
>  static int __init mxc_nd_init(void)
>  {
> +	int ret;
> +
>  	/* Register the device driver structure. */
>  	pr_info("MXC MTD nand Driver\n");
> -	if (platform_driver_probe(&mxcnd_driver, mxcnd_probe) != 0) {
> +	ret = platform_driver_probe(&mxcnd_driver, mxcnd_probe);
> +	if (ret)
>  		printk(KERN_ERR "Driver register failed for mxcnd_driver\n");

Can you please also remove this printk? platform_driver_probe()
fails when no platform device is registered for this driver which
only means that a particular board does not have NAND support. Getting
an error here is quite confusing in this situation.

Sascha

> -		return -ENODEV;
> -	}
> -	return 0;
> +
> +	return ret;
>  }
>  
>  static void __exit mxc_nd_cleanup(void)
> -- 
> 1.5.6
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] [MTD] MXC NAND driver fixes

[Vladimir Barinov]

Sascha Hauer wrote:
> Hi,
>
> On Fri, Apr 03, 2009 at 01:54:20AM +0400, Vladimir Barinov wrote:
>   
>> The following patch fixes:
>>  - re-initialization of host->col_addr which is used as byte index
>>    between the successive READID flash commands.
>>  - compile error when CONFIG_PM is enabled
>>  - pass on the error code from clk_get()
>>  - return -ENOMEM in case of failed ioremap()
>>  - pass on the return value of platform_driver_probe() instead of inventing -ENODEV
>>  - let command line partition table parsing with mxc_nand name.
>>    The cmd_line parsing is done via <mtd-id> name that differs
>>    from mxc_nand by default and looks like "NAND 256MiB 1,8V 8-bit"
>>
>> Signed-off-by: Vladimir Barinov <vbarinov@embeddedalley.com>
>> Signed-off-by: Lothar Wassmann <LW@KARO-electronics.de>
>> ---
>>  drivers/mtd/nand/mxc_nand.c |   47 +++++++++++++++++++++++++++---------------
>>  1 files changed, 30 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
>> index bad048a..e94c956 100644
>> --- a/drivers/mtd/nand/mxc_nand.c
>> +++ b/drivers/mtd/nand/mxc_nand.c
>> @@ -831,6 +831,7 @@ static void mxc_nand_command(struct mtd_info *mtd, unsigned command,
>>  		break;
>>  
>>  	case NAND_CMD_READID:
>> +		host->col_addr = 0;
>>  		send_read_id(host);
>>  		break;
>>  
>> @@ -881,8 +882,10 @@ static int __init mxcnd_probe(struct platform_device *pdev)
>>  	this->verify_buf = mxc_nand_verify_buf;
>>  
>>  	host->clk = clk_get(&pdev->dev, "nfc");
>> -	if (IS_ERR(host->clk))
>> +	if (IS_ERR(host->clk)) {
>> +		err = PTR_ERR(host->clk);
>>  		goto eclk;
>> +	}
>>  
>>  	clk_enable(host->clk);
>>  	host->clk_act = 1;
>> @@ -895,7 +898,7 @@ static int __init mxcnd_probe(struct platform_device *pdev)
>>  
>>  	host->regs = ioremap(res->start, res->end - res->start + 1);
>>  	if (!host->regs) {
>> -		err = -EIO;
>> +		err = -ENOMEM;
>>  		goto eres;
>>  	}
>>  
>> @@ -964,6 +967,9 @@ static int __init mxcnd_probe(struct platform_device *pdev)
>>  
>>  	/* Register the partitions */
>>  #ifdef CONFIG_MTD_PARTITIONS
>> +#ifdef CONFIG_MTD_CMDLINE_PARTS
>> +	mtd->name = "mxc_nand";
>> +#endif
>>     
>
> Do we need an ifdef here?
>   
That is due to the useless of such naming for all other cases (i.e. 
redboot fis partition table parsing)
>   
>>  	nr_parts =
>>  	    parse_mtd_partitions(mtd, part_probes, &host->parts, 0);
>>  	if (nr_parts > 0)
>> @@ -1010,30 +1016,35 @@ static int __devexit mxcnd_remove(struct platform_device *pdev)
>>  #ifdef CONFIG_PM
>>  static int mxcnd_suspend(struct platform_device *pdev, pm_message_t state)
>>  {
>> -	struct mtd_info *info = platform_get_drvdata(pdev);
>> +	struct mtd_info *mtd = platform_get_drvdata(pdev);
>> +	struct nand_chip *nand_chip = mtd->priv;
>> +	struct mxc_nand_host *host = nand_chip->priv;
>>  	int ret = 0;
>>  
>>  	DEBUG(MTD_DEBUG_LEVEL0, "MXC_ND : NAND suspend\n");
>> -	if (info)
>> -		ret = info->suspend(info);
>> -
>> -	/* Disable the NFC clock */
>> -	clk_disable(nfc_clk);	/* FIXME */
>> +	if (mtd) {
>> +		ret = mtd->suspend(mtd);
>> +		/* Disable the NFC clock */
>> +		clk_disable(host->clk);
>> +	}
>>  
>>  	return ret;
>>  }
>>  
>>  static int mxcnd_resume(struct platform_device *pdev)
>>  {
>> -	struct mtd_info *info = platform_get_drvdata(pdev);
>> +	struct mtd_info *mtd = platform_get_drvdata(pdev);
>> +	struct nand_chip *nand_chip = mtd->priv;
>> +	struct mxc_nand_host *host = nand_chip->priv;
>>  	int ret = 0;
>>  
>>  	DEBUG(MTD_DEBUG_LEVEL0, "MXC_ND : NAND resume\n");
>> -	/* Enable the NFC clock */
>> -	clk_enable(nfc_clk);	/* FIXME */
>>  
>> -	if (info)
>> -		info->resume(info);
>> +	if (mtd) {
>> +		/* Enable the NFC clock */
>> +		clk_enable(host->clk);
>> +		mtd->resume(mtd);
>> +	}
>>  
>>  	return ret;
>>  }
>> @@ -1054,13 +1065,15 @@ static struct platform_driver mxcnd_driver = {
>>  
>>  static int __init mxc_nd_init(void)
>>  {
>> +	int ret;
>> +
>>  	/* Register the device driver structure. */
>>  	pr_info("MXC MTD nand Driver\n");
>> -	if (platform_driver_probe(&mxcnd_driver, mxcnd_probe) != 0) {
>> +	ret = platform_driver_probe(&mxcnd_driver, mxcnd_probe);
>> +	if (ret)
>>  		printk(KERN_ERR "Driver register failed for mxcnd_driver\n");
>>     
>
> Can you please also remove this printk? platform_driver_probe()
> fails when no platform device is registered for this driver which
> only means that a particular board does not have NAND support. Getting
> an error here is quite confusing in this situation.
>   
Ok, wiil do in the next try.
> Sascha
>
>   
>> -		return -ENODEV;
>> -	}
>> -	return 0;
>> +
>> +	return ret;
>>  }
>>  
>>  static void __exit mxc_nd_cleanup(void)
>> -- 
>> 1.5.6
>>
>>
>>     
>
>   

Re: [PATCH] [MTD] MXC NAND driver fixes (v2)

[Sascha Hauer]

On Fri, Apr 03, 2009 at 12:43:15PM +0400, Vladimir Barinov wrote:
> The following patch fixes:
>  - re-initialization of host->col_addr which is used as byte index
>    between the successive READID flash commands.
>  - compile error when CONFIG_PM is enabled
>  - pass on the error code from clk_get()
>  - return -ENOMEM in case of failed ioremap()
>  - pass on the return value of platform_driver_probe() directly
>  - remove excessive printk
>  - let command line partition table parsing with mxc_nand name.
>    The cmd_line parsing is done via <mtd-id> name that differs
>    from mxc_nand by default and looks like "NAND 256MiB 1,8V 8-bit"
> 
> Signed-off-by: Vladimir Barinov <vbarinov@embeddedalley.com>
> Signed-off-by: Lothar Wassmann <LW@KARO-electronics.de>
> ---
>  drivers/mtd/nand/mxc_nand.c |   45 +++++++++++++++++++++++-------------------
>  1 files changed, 25 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> index bad048a..7ecfde0 100644
> --- a/drivers/mtd/nand/mxc_nand.c
> +++ b/drivers/mtd/nand/mxc_nand.c
> @@ -831,6 +831,7 @@ static void mxc_nand_command(struct mtd_info *mtd, unsigned command,
>  		break;
>  
>  	case NAND_CMD_READID:
> +		host->col_addr = 0;
>  		send_read_id(host);
>  		break;
>  
> @@ -881,8 +882,10 @@ static int __init mxcnd_probe(struct platform_device *pdev)
>  	this->verify_buf = mxc_nand_verify_buf;
>  
>  	host->clk = clk_get(&pdev->dev, "nfc");
> -	if (IS_ERR(host->clk))
> +	if (IS_ERR(host->clk)) {
> +		err = PTR_ERR(host->clk);
>  		goto eclk;
> +	}
>  
>  	clk_enable(host->clk);
>  	host->clk_act = 1;
> @@ -895,7 +898,7 @@ static int __init mxcnd_probe(struct platform_device *pdev)
>  
>  	host->regs = ioremap(res->start, res->end - res->start + 1);
>  	if (!host->regs) {
> -		err = -EIO;
> +		err = -ENOMEM;
>  		goto eres;
>  	}
>  
> @@ -964,6 +967,9 @@ static int __init mxcnd_probe(struct platform_device *pdev)
>  
>  	/* Register the partitions */
>  #ifdef CONFIG_MTD_PARTITIONS
> +#ifdef CONFIG_MTD_CMDLINE_PARTS
> +	mtd->name = "mxc_nand";
> +#endif

Thinking about it I'm totally against this ifdef. The name should not
depend on command line partition support being compiled into the kernel
or not. Just because it's compiled in does not mean that the user
actually wants to use it and it's quite confusing that the name changes
when a totally unrelated option is changed. I think it should not even
be inside CONFIG_MTD_PARTITIONS.

The rest of the patch seems ok to me.

Sascha

>  	nr_parts =
>  	    parse_mtd_partitions(mtd, part_probes, &host->parts, 0);
>  	if (nr_parts > 0)
> @@ -1010,30 +1016,35 @@ static int __devexit mxcnd_remove(struct platform_device *pdev)
>  #ifdef CONFIG_PM
>  static int mxcnd_suspend(struct platform_device *pdev, pm_message_t state)
>  {
> -	struct mtd_info *info = platform_get_drvdata(pdev);
> +	struct mtd_info *mtd = platform_get_drvdata(pdev);
> +	struct nand_chip *nand_chip = mtd->priv;
> +	struct mxc_nand_host *host = nand_chip->priv;
>  	int ret = 0;
>  
>  	DEBUG(MTD_DEBUG_LEVEL0, "MXC_ND : NAND suspend\n");
> -	if (info)
> -		ret = info->suspend(info);
> -
> -	/* Disable the NFC clock */
> -	clk_disable(nfc_clk);	/* FIXME */
> +	if (mtd) {
> +		ret = mtd->suspend(mtd);
> +		/* Disable the NFC clock */
> +		clk_disable(host->clk);
> +	}
>  
>  	return ret;
>  }
>  
>  static int mxcnd_resume(struct platform_device *pdev)
>  {
> -	struct mtd_info *info = platform_get_drvdata(pdev);
> +	struct mtd_info *mtd = platform_get_drvdata(pdev);
> +	struct nand_chip *nand_chip = mtd->priv;
> +	struct mxc_nand_host *host = nand_chip->priv;
>  	int ret = 0;
>  
>  	DEBUG(MTD_DEBUG_LEVEL0, "MXC_ND : NAND resume\n");
> -	/* Enable the NFC clock */
> -	clk_enable(nfc_clk);	/* FIXME */
>  
> -	if (info)
> -		info->resume(info);
> +	if (mtd) {
> +		/* Enable the NFC clock */
> +		clk_enable(host->clk);
> +		mtd->resume(mtd);
> +	}
>  
>  	return ret;
>  }
> @@ -1054,13 +1065,7 @@ static struct platform_driver mxcnd_driver = {
>  
>  static int __init mxc_nd_init(void)
>  {
> -	/* Register the device driver structure. */
> -	pr_info("MXC MTD nand Driver\n");
> -	if (platform_driver_probe(&mxcnd_driver, mxcnd_probe) != 0) {
> -		printk(KERN_ERR "Driver register failed for mxcnd_driver\n");
> -		return -ENODEV;
> -	}
> -	return 0;
> +	return platform_driver_probe(&mxcnd_driver, mxcnd_probe);
>  }
>  
>  static void __exit mxc_nd_cleanup(void)
> -- 
> 1.5.6
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] [MTD] MXC NAND driver fixes (v2)

[Vladimir Barinov]

Sascha Hauer wrote:
> On Fri, Apr 03, 2009 at 12:43:15PM +0400, Vladimir Barinov wrote:
>   
>> The following patch fixes:
>>  - re-initialization of host->col_addr which is used as byte index
>>    between the successive READID flash commands.
>>  - compile error when CONFIG_PM is enabled
>>  - pass on the error code from clk_get()
>>  - return -ENOMEM in case of failed ioremap()
>>  - pass on the return value of platform_driver_probe() directly
>>  - remove excessive printk
>>  - let command line partition table parsing with mxc_nand name.
>>    The cmd_line parsing is done via <mtd-id> name that differs
>>    from mxc_nand by default and looks like "NAND 256MiB 1,8V 8-bit"
>>
>> Signed-off-by: Vladimir Barinov <vbarinov@embeddedalley.com>
>> Signed-off-by: Lothar Wassmann <LW@KARO-electronics.de>
>> ---
>>  drivers/mtd/nand/mxc_nand.c |   45 +++++++++++++++++++++++-------------------
>>  1 files changed, 25 insertions(+), 20 deletions(-)
>>
>>  
>>  	/* Register the partitions */
>>  #ifdef CONFIG_MTD_PARTITIONS
>> +#ifdef CONFIG_MTD_CMDLINE_PARTS
>> +	mtd->name = "mxc_nand";
>> +#endif
>>     
>
> Thinking about it I'm totally against this ifdef. The name should not
> depend on command line partition support being compiled into the kernel
> or not. Just because it's compiled in does not mean that the user
> actually wants to use it and it's quite confusing that the name changes
> when a totally unrelated option is changed. I think it should not even
> be inside CONFIG_MTD_PARTITIONS.
>   
Actually I agree to make the name permanent and put out of any ifdef.
I've added cmdline dependency here just because of too long command line 
name
needed to parse partition table and all other cases were useless.
> The rest of the patch seems ok to me.
>   
Then I'll put the driver naming out of ifdefs and resend the v3.
> Sascha
>   

Re: [PATCH] [MTD] MXC NAND driver fixes (v3)

[Sascha Hauer]

On Wed, Apr 08, 2009 at 09:08:22PM +0400, Vladimir Barinov wrote:
> The following patch fixes:
>  - re-initialization of host->col_addr which is used as byte index
>    between the successive READID flash commands.
>  - compile error when CONFIG_PM is enabled
>  - pass on the error code from clk_get()
>  - return -ENOMEM in case of failed ioremap()
>  - pass on the return value of platform_driver_probe() directly
>  - remove excessive printk
>  - let command line partition table parsing with mxc_nand name.
>    The cmd_line parsing is done via <mtd-id> name that differs
>    from mxc_nand by default and looks like "NAND 256MiB 1,8V 8-bit"
> 
> Signed-off-by: Vladimir Barinov <vbarinov@embeddedalley.com>
> Signed-off-by: Lothar Wassmann <LW@KARO-electronics.de>

Acked-by: Sascha Hauer <s.hauer@pengutronix.de>

> ---
>  drivers/mtd/nand/mxc_nand.c |   43 +++++++++++++++++++++++--------------------
>  1 files changed, 23 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> index bad048a..d4dbcbe 100644
> --- a/drivers/mtd/nand/mxc_nand.c
> +++ b/drivers/mtd/nand/mxc_nand.c
> @@ -831,6 +831,7 @@ static void mxc_nand_command(struct mtd_info *mtd, unsigned command,
>  		break;
>  
>  	case NAND_CMD_READID:
> +		host->col_addr = 0;
>  		send_read_id(host);
>  		break;
>  
> @@ -866,6 +867,7 @@ static int __init mxcnd_probe(struct platform_device *pdev)
>  	mtd = &host->mtd;
>  	mtd->priv = this;
>  	mtd->owner = THIS_MODULE;
> +	mtd->name = "mxc_nand";
>  
>  	/* 50 us command delay time */
>  	this->chip_delay = 5;
> @@ -881,8 +883,10 @@ static int __init mxcnd_probe(struct platform_device *pdev)
>  	this->verify_buf = mxc_nand_verify_buf;
>  
>  	host->clk = clk_get(&pdev->dev, "nfc");
> -	if (IS_ERR(host->clk))
> +	if (IS_ERR(host->clk)) {
> +		err = PTR_ERR(host->clk);
>  		goto eclk;
> +	}
>  
>  	clk_enable(host->clk);
>  	host->clk_act = 1;
> @@ -895,7 +899,7 @@ static int __init mxcnd_probe(struct platform_device *pdev)
>  
>  	host->regs = ioremap(res->start, res->end - res->start + 1);
>  	if (!host->regs) {
> -		err = -EIO;
> +		err = -ENOMEM;
>  		goto eres;
>  	}
>  
> @@ -1010,30 +1014,35 @@ static int __devexit mxcnd_remove(struct platform_device *pdev)
>  #ifdef CONFIG_PM
>  static int mxcnd_suspend(struct platform_device *pdev, pm_message_t state)
>  {
> -	struct mtd_info *info = platform_get_drvdata(pdev);
> +	struct mtd_info *mtd = platform_get_drvdata(pdev);
> +	struct nand_chip *nand_chip = mtd->priv;
> +	struct mxc_nand_host *host = nand_chip->priv;
>  	int ret = 0;
>  
>  	DEBUG(MTD_DEBUG_LEVEL0, "MXC_ND : NAND suspend\n");
> -	if (info)
> -		ret = info->suspend(info);
> -
> -	/* Disable the NFC clock */
> -	clk_disable(nfc_clk);	/* FIXME */
> +	if (mtd) {
> +		ret = mtd->suspend(mtd);
> +		/* Disable the NFC clock */
> +		clk_disable(host->clk);
> +	}
>  
>  	return ret;
>  }
>  
>  static int mxcnd_resume(struct platform_device *pdev)
>  {
> -	struct mtd_info *info = platform_get_drvdata(pdev);
> +	struct mtd_info *mtd = platform_get_drvdata(pdev);
> +	struct nand_chip *nand_chip = mtd->priv;
> +	struct mxc_nand_host *host = nand_chip->priv;
>  	int ret = 0;
>  
>  	DEBUG(MTD_DEBUG_LEVEL0, "MXC_ND : NAND resume\n");
> -	/* Enable the NFC clock */
> -	clk_enable(nfc_clk);	/* FIXME */
>  
> -	if (info)
> -		info->resume(info);
> +	if (mtd) {
> +		/* Enable the NFC clock */
> +		clk_enable(host->clk);
> +		mtd->resume(mtd);
> +	}
>  
>  	return ret;
>  }
> @@ -1054,13 +1063,7 @@ static struct platform_driver mxcnd_driver = {
>  
>  static int __init mxc_nd_init(void)
>  {
> -	/* Register the device driver structure. */
> -	pr_info("MXC MTD nand Driver\n");
> -	if (platform_driver_probe(&mxcnd_driver, mxcnd_probe) != 0) {
> -		printk(KERN_ERR "Driver register failed for mxcnd_driver\n");
> -		return -ENODEV;
> -	}
> -	return 0;
> +	return platform_driver_probe(&mxcnd_driver, mxcnd_probe);
>  }
>  
>  static void __exit mxc_nd_cleanup(void)
> -- 
> 1.5.6
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] [MTD] MXC NAND driver fixes (v3)

[Holger Schurig]

Hmm, in u-boot v2, drivers/nand/nand_imx.c, I read:


/*
 * MX21 Hardware contains a bug which causes HW ECC to fail for two
 * consecutive read pages containing 1bit Errors (See MX21 Chip Erata,
 * Erratum 16). Use software ECC for this chip.
 */

BTW, in my version of MC9328MX21CE.pdf it's number 18.


Do you know if all I need on i.MX21 is then

static struct mxc_nand_platform_data nand_board_info = {
       .width = 1,
       .hw_ecc = 0,  /* Broken hardware */
};

Re: [PATCH] [MTD] MXC NAND driver fixes (v3)

[Vladimir Barinov]

Hello Holger,

Could you explain how this message is related to this thread?
This patch as well as linux-mtd mail list doesn't care about platform 
specific code that you can do in your custom mx21 based board.

Holger Schurig wrote:
> Hmm, in u-boot v2, drivers/nand/nand_imx.c, I read:
>
>
> /*
>  * MX21 Hardware contains a bug which causes HW ECC to fail for two
>  * consecutive read pages containing 1bit Errors (See MX21 Chip Erata,
>  * Erratum 16). Use software ECC for this chip.
>  */
>
> BTW, in my version of MC9328MX21CE.pdf it's number 18.
>
>
> Do you know if all I need on i.MX21 is then
>
> static struct mxc_nand_platform_data nand_board_info = {
>        .width = 1,
>        .hw_ecc = 0,  /* Broken hardware */
> };
>   
Great, so the current mxc_nand driver can support your imx21 cpu through 
platform data.

Regarding to me then I use mx27 cpu and it's errata taken from FSL site 
says nothing about HW ECC:
http://www.freescale.com/files/32bit/doc/errata/MCIMX27CE.pdf?fsrch=1
nor in addendum:
http://www.freescale.com/files/dsp/doc/ref_manual/MCIMX27RMAD.pdf?fsrch=1

I can successfully flash jffs2 image to NAND flash, mount and read the 
context.
Ditto I can do with ubifs. All this with hw_ecc = 1.

Regards,
Vladimir

Re: [PATCH] [MTD] MXC NAND driver fixes (v3)

[Holger Schurig]

> Hello Holger,
>
> Could you explain how this message is related to this thread?
> This patch as well as linux-mtd mail list doesn't care about
> platform specific code that you can do in your custom mx21
> based board.

The thread is about a NAND flash driver for Linux MTD. And this 
driver will work with i.MX27 and i.MX21 SoCs. However, the 
i.MX21 has a specific error in it's silicon, where the hardware 
ECC cannot one-bit-errors IF they are in two consecutive NAND 
blocks.

If the NAND driver, that is supposed to be used for i.MX21, 
cannot handle this (e.g. using software ECC), then the NAND 
driver is buggy.


This has nothing to do with board-specific things, my question 
was just if the board setup code can keep ".ecc = 0". Or if that 
won't work, we have to fix the MXC NAND driver with some 
cpu_is_mx21() code paths.

Re: [PATCH] [MTD] MXC NAND driver fixes (v3)

[Vladimir Barinov]

Holger Schurig wrote:
>> Hello Holger,
>>
>> Could you explain how this message is related to this thread?
>> This patch as well as linux-mtd mail list doesn't care about
>> platform specific code that you can do in your custom mx21
>> based board.
>>     
>
> The thread is about a NAND flash driver for Linux MTD. And this 
> driver will work with i.MX27 and i.MX21 SoCs. However, the 
> i.MX21 has a specific error in it's silicon, where the hardware 
> ECC cannot one-bit-errors IF they are in two consecutive NAND 
> blocks.
>   
This thread is about minor fixes for MXC NAND driver :)
> If the NAND driver, that is supposed to be used for i.MX21, 
> cannot handle this (e.g. using software ECC), then the NAND 
> driver is buggy.
>
>   
You have pointed to the way how to resolve the mx21 h/w bug via platform 
data.
> This has nothing to do with board-specific things, my question 
> was just if the board setup code can keep ".ecc = 0". Or if that 
> won't work, we have to fix the MXC NAND driver with some 
> cpu_is_mx21() code paths.
>   
It's up to you to see if it work using .hw_ecc = 0. I have no mx21 
hardware at all.

Regards,
Vladimir

Re: [PATCH] [MTD] MXC NAND driver fixes (v3)

[Holger Schurig]

> It's up to you to see if it work using .hw_ecc = 0. I have no
> mx21 hardware at all.

That's a mailing list, I wasn't specically talking to you.

Re: [PATCH] [MTD] MXC NAND driver fixes (v3)

[Holger Schurig]

Hmm, in u-boot v2, drivers/nand/nand_imx.c, I read:


/*
 * MX21 Hardware contains a bug which causes HW ECC to fail for two
 * consecutive read pages containing 1bit Errors (See MX21 Chip Erata,
 * Erratum 16). Use software ECC for this chip.
 */

BTW, in my version of MC9328MX21CE.pdf it's number 18.


Do you know if all I need on i.MX21 is then


static struct mxc_nand_platform_data nand_board_info = {
       .width = 1,
       .hw_ecc = 0,  /* Broken hardware */
};