| Message ID | 1383837455-30721-16-git-send-email-ezequiel.garcia@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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!
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,
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(-)