Patchwork [v4,15/31] mtd: nand: pxa3xx: Use a completion to signal device ready

login
register
mail settings
Submitter Ezequiel Garcia
Date Nov. 7, 2013, 3:17 p.m.
Message ID <1383837455-30721-16-git-send-email-ezequiel.garcia@free-electrons.com>
Download mbox | patch
Permalink /patch/289406/
State Superseded
Headers show

Comments

Ezequiel Garcia - Nov. 7, 2013, 3:17 p.m.
The expected behavior of the waitfunc() NAND chip call is to wait
for the device to be READY (this is a standard chip line).
However, the current implementation does almost nothing, which opens
the possibility of issuing a command to a non-ready device.

Fix this by adding a new completion to wait for the ready event to arrive.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)
Brian Norris - Nov. 14, 2013, 6:39 p.m.
On Thu, Nov 07, 2013 at 12:17:19PM -0300, Ezequiel Garcia wrote:
> --- a/drivers/mtd/nand/pxa3xx_nand.c
> +++ b/drivers/mtd/nand/pxa3xx_nand.c
> @@ -863,21 +867,28 @@ static int pxa3xx_nand_waitfunc(struct mtd_info *mtd, struct nand_chip *this)
>  {
>  	struct pxa3xx_nand_host *host = mtd->priv;
>  	struct pxa3xx_nand_info *info = host->info_data;
> +	int ret;
> +
> +	/* Need to wait? */
> +	if (!info->is_ready) {
> +		ret = wait_for_completion_timeout(&info->dev_ready,
> +				CHIP_DELAY_TIMEOUT);
> +		if (!ret) {
> +			dev_err(&info->pdev->dev, "Ready time out!!!\n");
> +			return NAND_STATUS_FAIL;
> +		}
> +		info->is_ready = 1;

Shouldn't the is_ready=1 line to be above the if (!ret) condition? I
think you want to set is_ready=1 in either case (success or timeout).
With this code, any timeout will cause subsequent waitfunc()'s to block,
even if they are never going to catch an interrupt.

I think this kind of mistake is easier to make now, since the 'is_ready'
field isn't properly descriptive any more. It doesn't represent "is the
device ready"; it represents "is there a pending command on which I need
to wait". (I don't care if you change the name; I'm just pointing this
out.)

> +	}
>  

I think all the other patches up to this one are good. I may push them
to l2-mtd.git now, unless you object.

Brian
Ezequiel Garcia - Nov. 14, 2013, 6:53 p.m.
Brian,

On Thu, Nov 14, 2013 at 10:39:23AM -0800, Brian Norris wrote:
> On Thu, Nov 07, 2013 at 12:17:19PM -0300, Ezequiel Garcia wrote:
> > --- a/drivers/mtd/nand/pxa3xx_nand.c
> > +++ b/drivers/mtd/nand/pxa3xx_nand.c
> > @@ -863,21 +867,28 @@ static int pxa3xx_nand_waitfunc(struct mtd_info *mtd, struct nand_chip *this)
> >  {
> >  	struct pxa3xx_nand_host *host = mtd->priv;
> >  	struct pxa3xx_nand_info *info = host->info_data;
> > +	int ret;
> > +
> > +	/* Need to wait? */
> > +	if (!info->is_ready) {
> > +		ret = wait_for_completion_timeout(&info->dev_ready,
> > +				CHIP_DELAY_TIMEOUT);
> > +		if (!ret) {
> > +			dev_err(&info->pdev->dev, "Ready time out!!!\n");
> > +			return NAND_STATUS_FAIL;
> > +		}
> > +		info->is_ready = 1;
> 
> Shouldn't the is_ready=1 line to be above the if (!ret) condition? I
> think you want to set is_ready=1 in either case (success or timeout).
> With this code, any timeout will cause subsequent waitfunc()'s to block,
> even if they are never going to catch an interrupt.
> 

Yes, good catch!

> I think this kind of mistake is easier to make now, since the 'is_ready'
> field isn't properly descriptive any more. It doesn't represent "is the
> device ready"; it represents "is there a pending command on which I need
> to wait". (I don't care if you change the name; I'm just pointing this
> out.)
> 

Yes, I agree. Maybe, "need_wait" or something like that would fit
better. Let me submit a new patch for this one.

> > +	}
> >  
> 
> I think all the other patches up to this one are good. I may push them
> to l2-mtd.git now, unless you object.
> 

Great, thanks a lot for reviewing and for all the feedback!

Patch

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index dd7df0e..588b23a 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -37,6 +37,7 @@ 
 
 #include <linux/platform_data/mtd-nand-pxa3xx.h>
 
+#define NAND_DEV_READY_TIMEOUT  50
 #define	CHIP_DELAY_TIMEOUT	(2 * HZ/10)
 #define NAND_STOP_DELAY		(2 * HZ/50)
 #define PAGE_CHUNK_SIZE		(2048)
@@ -168,7 +169,7 @@  struct pxa3xx_nand_info {
 	struct clk		*clk;
 	void __iomem		*mmio_base;
 	unsigned long		mmio_phys;
-	struct completion	cmd_complete;
+	struct completion	cmd_complete, dev_ready;
 
 	unsigned int 		buf_start;
 	unsigned int		buf_count;
@@ -480,7 +481,7 @@  static void start_data_dma(struct pxa3xx_nand_info *info)
 static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
 {
 	struct pxa3xx_nand_info *info = devid;
-	unsigned int status, is_completed = 0;
+	unsigned int status, is_completed = 0, is_ready = 0;
 	unsigned int ready, cmd_done;
 
 	if (info->cs == 0) {
@@ -516,8 +517,8 @@  static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
 		is_completed = 1;
 	}
 	if (status & ready) {
-		info->is_ready = 1;
 		info->state = STATE_READY;
+		is_ready = 1;
 	}
 
 	if (status & NDSR_WRCMDREQ) {
@@ -546,6 +547,8 @@  static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
 	nand_writel(info, NDSR, status);
 	if (is_completed)
 		complete(&info->cmd_complete);
+	if (is_ready)
+		complete(&info->dev_ready);
 NORMAL_IRQ_EXIT:
 	return IRQ_HANDLED;
 }
@@ -576,7 +579,6 @@  static int prepare_command_pool(struct pxa3xx_nand_info *info, int command,
 	info->oob_size		= 0;
 	info->use_ecc		= 0;
 	info->use_spare		= 1;
-	info->is_ready		= 0;
 	info->retcode		= ERR_NONE;
 	if (info->cs != 0)
 		info->ndcb0 = NDCB0_CSEL;
@@ -749,6 +751,8 @@  static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, unsigned command,
 	exec_cmd = prepare_command_pool(info, command, column, page_addr);
 	if (exec_cmd) {
 		init_completion(&info->cmd_complete);
+		init_completion(&info->dev_ready);
+		info->is_ready = 0;
 		pxa3xx_nand_start(info);
 
 		ret = wait_for_completion_timeout(&info->cmd_complete,
@@ -863,21 +867,28 @@  static int pxa3xx_nand_waitfunc(struct mtd_info *mtd, struct nand_chip *this)
 {
 	struct pxa3xx_nand_host *host = mtd->priv;
 	struct pxa3xx_nand_info *info = host->info_data;
+	int ret;
+
+	/* Need to wait? */
+	if (!info->is_ready) {
+		ret = wait_for_completion_timeout(&info->dev_ready,
+				CHIP_DELAY_TIMEOUT);
+		if (!ret) {
+			dev_err(&info->pdev->dev, "Ready time out!!!\n");
+			return NAND_STATUS_FAIL;
+		}
+		info->is_ready = 1;
+	}
 
 	/* pxa3xx_nand_send_command has waited for command complete */
 	if (this->state == FL_WRITING || this->state == FL_ERASING) {
 		if (info->retcode == ERR_NONE)
 			return 0;
-		else {
-			/*
-			 * any error make it return 0x01 which will tell
-			 * the caller the erase and write fail
-			 */
-			return 0x01;
-		}
+		else
+			return NAND_STATUS_FAIL;
 	}
 
-	return 0;
+	return NAND_STATUS_READY;
 }
 
 static int pxa3xx_nand_config_flash(struct pxa3xx_nand_info *info,