[PATCH V2, 1/2] mtd: nand: brcmnand: Change brcmnand_set_wp() prototype

[PATCH V2, 1/2] mtd: nand: brcmnand: Change brcmnand_set_wp() prototype

[Kamal Dasu]

Changing brcmnand_set_wp() prototype in prepration for refactoring the nand
write protect logic to add flash status byte check for the protection bit.

Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
---
 drivers/mtd/nand/brcmnand/brcmnand.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
index 42ebd73..c7c4efe 100644
--- a/drivers/mtd/nand/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/brcmnand/brcmnand.c
@@ -765,11 +765,14 @@ enum {
 	CS_SELECT_AUTO_DEVICE_ID_CFG		= BIT(30),
 };
 
-static inline void brcmnand_set_wp(struct brcmnand_controller *ctrl, bool en)
+static int brcmnand_set_wp(struct brcmnand_host *host, int en)
 {
+	struct brcmnand_controller *ctrl = host->ctrl;
 	u32 val = en ? CS_SELECT_NAND_WP : 0;
 
 	brcmnand_rmw_reg(ctrl, BRCMNAND_CS_SELECT, CS_SELECT_NAND_WP, 0, val);
+
+	return 0;
 }
 
 /***********************************************************************
@@ -1029,7 +1032,7 @@ static void brcmnand_wp(struct mtd_info *mtd, int wp)
 			dev_dbg(ctrl->dev, "WP %s\n", wp ? "on" : "off");
 			old_wp = wp;
 		}
-		brcmnand_set_wp(ctrl, wp);
+		brcmnand_set_wp(host, wp);
 	}
 }
 
@@ -2462,14 +2465,6 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
 	/* Disable XOR addressing */
 	brcmnand_rmw_reg(ctrl, BRCMNAND_CS_XOR, 0xff, 0, 0);
 
-	if (ctrl->features & BRCMNAND_HAS_WP) {
-		/* Permanently disable write protection */
-		if (wp_on == 2)
-			brcmnand_set_wp(ctrl, false);
-	} else {
-		wp_on = 0;
-	}
-
 	/* IRQ */
 	ctrl->irq = platform_get_irq(pdev, 0);
 	if ((int)ctrl->irq < 0) {
@@ -2522,6 +2517,18 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
 			}
 
 			list_add_tail(&host->node, &ctrl->host_list);
+
+			if (ctrl->features & BRCMNAND_HAS_WP) {
+				/* Permanently disable write protection */
+				if (wp_on == 2) {
+					ret = brcmnand_set_wp(host, false);
+					if (ret)
+						goto err;
+				}
+			} else {
+				wp_on = 0;
+			}
+
 		}
 	}
 
-- 
1.9.1

[PATCH V2, 2/2] mtd: nand: brcmnand: Check flash #WP pin status before nand erase/program

[Kamal Dasu]

brcmnand controller v6.x and v7.x lets driver to enable disable #WP pin
via NAND_WP bit in CS_SELECT register. Driver implementation assumes that
setting/resetting the bit would assert/de-assert  #WP pin instantaneously
from the flash part's perspective, and was proceeding to erase/program
without verifying flash status byte for protection bit. In rigorous
testing this was causing rare data corruptions with erase and/or
subsequent programming. To fix this added verification logic to
brcmandn_wp_set() by reading  flash status and verifying protection bit
indicating #WP pin status.  The new logic makes sure controller as well
as the flash is ready to accept commands.

Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
---
 drivers/mtd/nand/brcmnand/brcmnand.c | 53 ++++++++++++++++++++++++++++++++++--
 1 file changed, 51 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
index c7c4efe..2f082a3 100644
--- a/drivers/mtd/nand/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/brcmnand/brcmnand.c
@@ -101,6 +101,9 @@ struct brcm_nand_dma_desc {
 #define BRCMNAND_MIN_BLOCKSIZE	(8 * 1024)
 #define BRCMNAND_MIN_DEVSIZE	(4ULL * 1024 * 1024)
 
+#define FLASH_RDY	(NAND_STATUS_READY)
+#define NAND_CTRL_RDY	(INTFC_CTLR_READY | INTFC_FLASH_READY)
+
 /* Controller feature flags */
 enum {
 	BRCMNAND_HAS_1K_SECTORS			= BIT(0),
@@ -765,12 +768,57 @@ enum {
 	CS_SELECT_AUTO_DEVICE_ID_CFG		= BIT(30),
 };
 
-static int brcmnand_set_wp(struct brcmnand_host *host, int en)
+static void bcmnand_ctrl_busy_poll(struct brcmnand_controller *ctrl, u32 mask)
+{
+	unsigned long timeout = jiffies + msecs_to_jiffies(200);
+
+	while ((brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS) & mask) !=
+		mask) {
+		if (time_after(jiffies, timeout)) {
+			dev_warn(ctrl->dev, "timeout on ctrl_ready\n");
+			break;
+		}
+		cpu_relax();
+	}
+}
+
+static inline void brcmnand_set_wp_reg(struct brcmnand_controller *ctrl, int en)
 {
-	struct brcmnand_controller *ctrl = host->ctrl;
 	u32 val = en ? CS_SELECT_NAND_WP : 0;
 
 	brcmnand_rmw_reg(ctrl, BRCMNAND_CS_SELECT, CS_SELECT_NAND_WP, 0, val);
+}
+
+static int brcmnand_set_wp(struct brcmnand_host *host, int en)
+{
+	struct brcmnand_controller *ctrl = host->ctrl;
+	struct mtd_info *mtd = nand_to_mtd(&host->chip);
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	u32 sts_reg;
+	bool is_wp;
+
+	/*
+	 * make sure ctrl/flash ready before and after
+	 * changing state of WP PIN
+	 */
+	bcmnand_ctrl_busy_poll(ctrl, NAND_CTRL_RDY | FLASH_RDY);
+	brcmnand_set_wp_reg(ctrl, en);
+	chip->cmdfunc(mtd, NAND_CMD_STATUS, -1, -1);
+	bcmnand_ctrl_busy_poll(ctrl, NAND_CTRL_RDY | FLASH_RDY);
+	sts_reg = brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS);
+	/* NAND_STATUS_WP 0x80 = not protected, 0x00 = protected */
+	is_wp = (sts_reg & NAND_STATUS_WP) ? false : true;
+
+	if (is_wp != en) {
+		u32 nand_wp = brcmnand_read_reg(ctrl, BRCMNAND_CS_SELECT);
+
+		nand_wp &= CS_SELECT_NAND_WP;
+		dev_err_ratelimited(&host->pdev->dev,
+				    "#WP %s sts:0x%x expected %s NAND_WP 0x%x\n",
+				    is_wp ? "On" : "Off",  sts_reg & 0xff,
+				    en ? "On" : "Off", nand_wp ? 1 : 0);
+		return -EINVAL;
+	}
 
 	return 0;
 }
@@ -1167,6 +1215,7 @@ static void brcmnand_send_cmd(struct brcmnand_host *host, int cmd)
 	BUG_ON(ctrl->cmd_pending != 0);
 	ctrl->cmd_pending = cmd;
 
+	bcmnand_ctrl_busy_poll(ctrl, NAND_CTRL_RDY);
 	intfc = brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS);
 	WARN_ON(!(intfc & INTFC_CTLR_READY));
 
-- 
1.9.1

Re: [PATCH V2, 2/2] mtd: nand: brcmnand: Check flash #WP pin status before nand erase/program

[Marek Vasut]

On 02/16/2017 07:46 PM, Kamal Dasu wrote:
> brcmnand controller v6.x and v7.x lets driver to enable disable #WP pin
> via NAND_WP bit in CS_SELECT register. Driver implementation assumes that
> setting/resetting the bit would assert/de-assert  #WP pin instantaneously
> from the flash part's perspective, and was proceeding to erase/program
> without verifying flash status byte for protection bit. In rigorous
> testing this was causing rare data corruptions with erase and/or
> subsequent programming. To fix this added verification logic to
> brcmandn_wp_set() by reading  flash status and verifying protection bit
> indicating #WP pin status.  The new logic makes sure controller as well
> as the flash is ready to accept commands.
> 
> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
> ---
>  drivers/mtd/nand/brcmnand/brcmnand.c | 53 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 51 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
> index c7c4efe..2f082a3 100644
> --- a/drivers/mtd/nand/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/brcmnand/brcmnand.c
> @@ -101,6 +101,9 @@ struct brcm_nand_dma_desc {
>  #define BRCMNAND_MIN_BLOCKSIZE	(8 * 1024)
>  #define BRCMNAND_MIN_DEVSIZE	(4ULL * 1024 * 1024)
>  
> +#define FLASH_RDY	(NAND_STATUS_READY)

Drop extra parenthesis

> +#define NAND_CTRL_RDY	(INTFC_CTLR_READY | INTFC_FLASH_READY)
> +
>  /* Controller feature flags */
>  enum {
>  	BRCMNAND_HAS_1K_SECTORS			= BIT(0),
> @@ -765,12 +768,57 @@ enum {
>  	CS_SELECT_AUTO_DEVICE_ID_CFG		= BIT(30),
>  };
>  
> -static int brcmnand_set_wp(struct brcmnand_host *host, int en)
> +static void bcmnand_ctrl_busy_poll(struct brcmnand_controller *ctrl, u32 mask)
> +{
> +	unsigned long timeout = jiffies + msecs_to_jiffies(200);
> +
> +	while ((brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS) & mask) !=
> +		mask) {

Ewww, make this into something like ...

while (true) { / for (;;) {
 reg = read.....
 if ((reg & mask) == mask)
   ...
 if (time ....)
   dev_warn...
 cpu_relax();
}

But then again, there's the readx_poll_timeout() , which seems like
exactly the thing you implemented here .

> +		if (time_after(jiffies, timeout)) {
> +			dev_warn(ctrl->dev, "timeout on ctrl_ready\n");
> +			break;
> +		}
> +		cpu_relax();
> +	}
> +}
> +
> +static inline void brcmnand_set_wp_reg(struct brcmnand_controller *ctrl, int en)
>  {
> -	struct brcmnand_controller *ctrl = host->ctrl;
>  	u32 val = en ? CS_SELECT_NAND_WP : 0;
>  
>  	brcmnand_rmw_reg(ctrl, BRCMNAND_CS_SELECT, CS_SELECT_NAND_WP, 0, val);
> +}
> +
> +static int brcmnand_set_wp(struct brcmnand_host *host, int en)
> +{
> +	struct brcmnand_controller *ctrl = host->ctrl;
> +	struct mtd_info *mtd = nand_to_mtd(&host->chip);
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +	u32 sts_reg;
> +	bool is_wp;
> +
> +	/*
> +	 * make sure ctrl/flash ready before and after
> +	 * changing state of WP PIN
> +	 */
> +	bcmnand_ctrl_busy_poll(ctrl, NAND_CTRL_RDY | FLASH_RDY);
> +	brcmnand_set_wp_reg(ctrl, en);
> +	chip->cmdfunc(mtd, NAND_CMD_STATUS, -1, -1);
> +	bcmnand_ctrl_busy_poll(ctrl, NAND_CTRL_RDY | FLASH_RDY);
> +	sts_reg = brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS);
> +	/* NAND_STATUS_WP 0x80 = not protected, 0x00 = protected */
> +	is_wp = (sts_reg & NAND_STATUS_WP) ? false : true;

I dont think you need the ternary op here.

> +	if (is_wp != en) {
> +		u32 nand_wp = brcmnand_read_reg(ctrl, BRCMNAND_CS_SELECT);
> +
> +		nand_wp &= CS_SELECT_NAND_WP;
> +		dev_err_ratelimited(&host->pdev->dev,
> +				    "#WP %s sts:0x%x expected %s NAND_WP 0x%x\n",
> +				    is_wp ? "On" : "Off",  sts_reg & 0xff,
> +				    en ? "On" : "Off", nand_wp ? 1 : 0);
> +		return -EINVAL;
> +	}
>  
>  	return 0;
>  }
> @@ -1167,6 +1215,7 @@ static void brcmnand_send_cmd(struct brcmnand_host *host, int cmd)
>  	BUG_ON(ctrl->cmd_pending != 0);
>  	ctrl->cmd_pending = cmd;
>  
> +	bcmnand_ctrl_busy_poll(ctrl, NAND_CTRL_RDY);
>  	intfc = brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS);
>  	WARN_ON(!(intfc & INTFC_CTLR_READY));
>  
> 


-- 
Best regards,
Marek Vasut

Re: [PATCH V2, 2/2] mtd: nand: brcmnand: Check flash #WP pin status before nand erase/programAAs

[Boris Brezillon]

On Thu, 16 Feb 2017 13:46:52 -0500
Kamal Dasu <kdasu.kdev@gmail.com> wrote:

> brcmnand controller v6.x and v7.x lets driver to enable disable #WP pin
> via NAND_WP bit in CS_SELECT register. Driver implementation assumes that
> setting/resetting the bit would assert/de-assert  #WP pin instantaneously
> from the flash part's perspective, and was proceeding to erase/program
> without verifying flash status byte for protection bit. In rigorous
> testing this was causing rare data corruptions with erase and/or
> subsequent programming. To fix this added verification logic to
> brcmandn_wp_set() by reading  flash status and verifying protection bit
> indicating #WP pin status.  The new logic makes sure controller as well
> as the flash is ready to accept commands.
> 
> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
> ---
>  drivers/mtd/nand/brcmnand/brcmnand.c | 53 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 51 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
> index c7c4efe..2f082a3 100644
> --- a/drivers/mtd/nand/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/brcmnand/brcmnand.c
> @@ -101,6 +101,9 @@ struct brcm_nand_dma_desc {
>  #define BRCMNAND_MIN_BLOCKSIZE	(8 * 1024)
>  #define BRCMNAND_MIN_DEVSIZE	(4ULL * 1024 * 1024)
>  
> +#define FLASH_RDY	(NAND_STATUS_READY)

Drop FLASH_RDY and use NAND_STATUS_READY directly. This re-definition
just adds more confusion.

> +#define NAND_CTRL_RDY	(INTFC_CTLR_READY | INTFC_FLASH_READY)
> +
>  /* Controller feature flags */
>  enum {
>  	BRCMNAND_HAS_1K_SECTORS			= BIT(0),
> @@ -765,12 +768,57 @@ enum {
>  	CS_SELECT_AUTO_DEVICE_ID_CFG		= BIT(30),
>  };
>  
> -static int brcmnand_set_wp(struct brcmnand_host *host, int en)
> +static void bcmnand_ctrl_busy_poll(struct brcmnand_controller *ctrl, u32 mask)
> +{
> +	unsigned long timeout = jiffies + msecs_to_jiffies(200);
> +
> +	while ((brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS) & mask) !=
> +		mask) {
> +		if (time_after(jiffies, timeout)) {
> +			dev_warn(ctrl->dev, "timeout on ctrl_ready\n");
> +			break;
> +		}
> +		cpu_relax();
> +	}
> +}
> +
> +static inline void brcmnand_set_wp_reg(struct brcmnand_controller *ctrl, int en)
>  {
> -	struct brcmnand_controller *ctrl = host->ctrl;
>  	u32 val = en ? CS_SELECT_NAND_WP : 0;
>  
>  	brcmnand_rmw_reg(ctrl, BRCMNAND_CS_SELECT, CS_SELECT_NAND_WP, 0, val);
> +}
> +
> +static int brcmnand_set_wp(struct brcmnand_host *host, int en)
> +{
> +	struct brcmnand_controller *ctrl = host->ctrl;
> +	struct mtd_info *mtd = nand_to_mtd(&host->chip);
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +	u32 sts_reg;
> +	bool is_wp;
> +
> +	/*
> +	 * make sure ctrl/flash ready before and after
> +	 * changing state of WP PIN
> +	 */
> +	bcmnand_ctrl_busy_poll(ctrl, NAND_CTRL_RDY | FLASH_RDY);
> +	brcmnand_set_wp_reg(ctrl, en);
> +	chip->cmdfunc(mtd, NAND_CMD_STATUS, -1, -1);
> +	bcmnand_ctrl_busy_poll(ctrl, NAND_CTRL_RDY | FLASH_RDY);
> +	sts_reg = brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS);
> +	/* NAND_STATUS_WP 0x80 = not protected, 0x00 = protected */
> +	is_wp = (sts_reg & NAND_STATUS_WP) ? false : true;
> +
> +	if (is_wp != en) {
> +		u32 nand_wp = brcmnand_read_reg(ctrl, BRCMNAND_CS_SELECT);
> +
> +		nand_wp &= CS_SELECT_NAND_WP;
> +		dev_err_ratelimited(&host->pdev->dev,
> +				    "#WP %s sts:0x%x expected %s NAND_WP 0x%x\n",
> +				    is_wp ? "On" : "Off",  sts_reg & 0xff,
> +				    en ? "On" : "Off", nand_wp ? 1 : 0);
> +		return -EINVAL;
> +	}
>  
>  	return 0;
>  }
> @@ -1167,6 +1215,7 @@ static void brcmnand_send_cmd(struct brcmnand_host *host, int cmd)
>  	BUG_ON(ctrl->cmd_pending != 0);
>  	ctrl->cmd_pending = cmd;
>  
> +	bcmnand_ctrl_busy_poll(ctrl, NAND_CTRL_RDY);
>  	intfc = brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS);
>  	WARN_ON(!(intfc & INTFC_CTLR_READY));
>  

Re: [PATCH V2, 2/2] mtd: nand: brcmnand: Check flash #WP pin status before nand erase/program

[Boris Brezillon]

On Fri, 17 Feb 2017 22:38:04 +0100
Marek Vasut <marek.vasut@gmail.com> wrote:

> On 02/16/2017 07:46 PM, Kamal Dasu wrote:
> > brcmnand controller v6.x and v7.x lets driver to enable disable #WP pin
> > via NAND_WP bit in CS_SELECT register. Driver implementation assumes that
> > setting/resetting the bit would assert/de-assert  #WP pin instantaneously
> > from the flash part's perspective, and was proceeding to erase/program
> > without verifying flash status byte for protection bit. In rigorous
> > testing this was causing rare data corruptions with erase and/or
> > subsequent programming. To fix this added verification logic to
> > brcmandn_wp_set() by reading  flash status and verifying protection bit
> > indicating #WP pin status.  The new logic makes sure controller as well
> > as the flash is ready to accept commands.
> > 
> > Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
> > ---
> >  drivers/mtd/nand/brcmnand/brcmnand.c | 53 ++++++++++++++++++++++++++++++++++--
> >  1 file changed, 51 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
> > index c7c4efe..2f082a3 100644
> > --- a/drivers/mtd/nand/brcmnand/brcmnand.c
> > +++ b/drivers/mtd/nand/brcmnand/brcmnand.c
> > @@ -101,6 +101,9 @@ struct brcm_nand_dma_desc {
> >  #define BRCMNAND_MIN_BLOCKSIZE	(8 * 1024)
> >  #define BRCMNAND_MIN_DEVSIZE	(4ULL * 1024 * 1024)
> >  
> > +#define FLASH_RDY	(NAND_STATUS_READY)  
> 
> Drop extra parenthesis
> 
> > +#define NAND_CTRL_RDY	(INTFC_CTLR_READY | INTFC_FLASH_READY)
> > +
> >  /* Controller feature flags */
> >  enum {
> >  	BRCMNAND_HAS_1K_SECTORS			= BIT(0),
> > @@ -765,12 +768,57 @@ enum {
> >  	CS_SELECT_AUTO_DEVICE_ID_CFG		= BIT(30),
> >  };
> >  
> > -static int brcmnand_set_wp(struct brcmnand_host *host, int en)
> > +static void bcmnand_ctrl_busy_poll(struct brcmnand_controller *ctrl, u32 mask)
> > +{
> > +	unsigned long timeout = jiffies + msecs_to_jiffies(200);
> > +
> > +	while ((brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS) & mask) !=
> > +		mask) {  
> 
> Ewww, make this into something like ...
> 
> while (true) { / for (;;) {
>  reg = read.....
>  if ((reg & mask) == mask)
>    ...
>  if (time ....)
>    dev_warn...
>  cpu_relax();
> }
> 
> But then again, there's the readx_poll_timeout() , which seems like
> exactly the thing you implemented here .

I agree that using readl_poll_timeout() would be better, but it's not
so simple (see brcmnand_read_reg() and brcmnand_readl()
implementations).

> 
> > +		if (time_after(jiffies, timeout)) {
> > +			dev_warn(ctrl->dev, "timeout on ctrl_ready\n");
> > +			break;
> > +		}
> > +		cpu_relax();
> > +	}
> > +}
> > +
> > +static inline void brcmnand_set_wp_reg(struct brcmnand_controller *ctrl, int en)
> >  {
> > -	struct brcmnand_controller *ctrl = host->ctrl;
> >  	u32 val = en ? CS_SELECT_NAND_WP : 0;
> >  
> >  	brcmnand_rmw_reg(ctrl, BRCMNAND_CS_SELECT, CS_SELECT_NAND_WP, 0, val);
> > +}
> > +
> > +static int brcmnand_set_wp(struct brcmnand_host *host, int en)
> > +{
> > +	struct brcmnand_controller *ctrl = host->ctrl;
> > +	struct mtd_info *mtd = nand_to_mtd(&host->chip);
> > +	struct nand_chip *chip = mtd_to_nand(mtd);
> > +	u32 sts_reg;
> > +	bool is_wp;
> > +
> > +	/*
> > +	 * make sure ctrl/flash ready before and after
> > +	 * changing state of WP PIN
> > +	 */
> > +	bcmnand_ctrl_busy_poll(ctrl, NAND_CTRL_RDY | FLASH_RDY);
> > +	brcmnand_set_wp_reg(ctrl, en);
> > +	chip->cmdfunc(mtd, NAND_CMD_STATUS, -1, -1);
> > +	bcmnand_ctrl_busy_poll(ctrl, NAND_CTRL_RDY | FLASH_RDY);
> > +	sts_reg = brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS);
> > +	/* NAND_STATUS_WP 0x80 = not protected, 0x00 = protected */
> > +	is_wp = (sts_reg & NAND_STATUS_WP) ? false : true;  
> 
> I dont think you need the ternary op here.

Indeed.

> 
> > +	if (is_wp != en) {
> > +		u32 nand_wp = brcmnand_read_reg(ctrl, BRCMNAND_CS_SELECT);
> > +
> > +		nand_wp &= CS_SELECT_NAND_WP;
> > +		dev_err_ratelimited(&host->pdev->dev,
> > +				    "#WP %s sts:0x%x expected %s NAND_WP 0x%x\n",
> > +				    is_wp ? "On" : "Off",  sts_reg & 0xff,
> > +				    en ? "On" : "Off", nand_wp ? 1 : 0);
> > +		return -EINVAL;
> > +	}
> >  
> >  	return 0;
> >  }
> > @@ -1167,6 +1215,7 @@ static void brcmnand_send_cmd(struct brcmnand_host *host, int cmd)
> >  	BUG_ON(ctrl->cmd_pending != 0);
> >  	ctrl->cmd_pending = cmd;
> >  
> > +	bcmnand_ctrl_busy_poll(ctrl, NAND_CTRL_RDY);
> >  	intfc = brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS);
> >  	WARN_ON(!(intfc & INTFC_CTLR_READY));
> >  
> >   
> 
> 

Re: [PATCH V2, 2/2] mtd: nand: brcmnand: Check flash #WP pin status before nand erase/program

[Marek Vasut]

On 02/18/2017 09:03 AM, Boris Brezillon wrote:
> On Fri, 17 Feb 2017 22:38:04 +0100
> Marek Vasut <marek.vasut@gmail.com> wrote:
> 
>> On 02/16/2017 07:46 PM, Kamal Dasu wrote:
>>> brcmnand controller v6.x and v7.x lets driver to enable disable #WP pin
>>> via NAND_WP bit in CS_SELECT register. Driver implementation assumes that
>>> setting/resetting the bit would assert/de-assert  #WP pin instantaneously
>>> from the flash part's perspective, and was proceeding to erase/program
>>> without verifying flash status byte for protection bit. In rigorous
>>> testing this was causing rare data corruptions with erase and/or
>>> subsequent programming. To fix this added verification logic to
>>> brcmandn_wp_set() by reading  flash status and verifying protection bit
>>> indicating #WP pin status.  The new logic makes sure controller as well
>>> as the flash is ready to accept commands.
>>>
>>> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
>>> ---
>>>  drivers/mtd/nand/brcmnand/brcmnand.c | 53 ++++++++++++++++++++++++++++++++++--
>>>  1 file changed, 51 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
>>> index c7c4efe..2f082a3 100644
>>> --- a/drivers/mtd/nand/brcmnand/brcmnand.c
>>> +++ b/drivers/mtd/nand/brcmnand/brcmnand.c
>>> @@ -101,6 +101,9 @@ struct brcm_nand_dma_desc {
>>>  #define BRCMNAND_MIN_BLOCKSIZE	(8 * 1024)
>>>  #define BRCMNAND_MIN_DEVSIZE	(4ULL * 1024 * 1024)
>>>  
>>> +#define FLASH_RDY	(NAND_STATUS_READY)  
>>
>> Drop extra parenthesis
>>
>>> +#define NAND_CTRL_RDY	(INTFC_CTLR_READY | INTFC_FLASH_READY)
>>> +
>>>  /* Controller feature flags */
>>>  enum {
>>>  	BRCMNAND_HAS_1K_SECTORS			= BIT(0),
>>> @@ -765,12 +768,57 @@ enum {
>>>  	CS_SELECT_AUTO_DEVICE_ID_CFG		= BIT(30),
>>>  };
>>>  
>>> -static int brcmnand_set_wp(struct brcmnand_host *host, int en)
>>> +static void bcmnand_ctrl_busy_poll(struct brcmnand_controller *ctrl, u32 mask)
>>> +{
>>> +	unsigned long timeout = jiffies + msecs_to_jiffies(200);
>>> +
>>> +	while ((brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS) & mask) !=
>>> +		mask) {  
>>
>> Ewww, make this into something like ...
>>
>> while (true) { / for (;;) {
>>  reg = read.....
>>  if ((reg & mask) == mask)
>>    ...
>>  if (time ....)
>>    dev_warn...
>>  cpu_relax();
>> }
>>
>> But then again, there's the readx_poll_timeout() , which seems like
>> exactly the thing you implemented here .
> 
> I agree that using readl_poll_timeout() would be better, but it's not
> so simple (see brcmnand_read_reg() and brcmnand_readl()
> implementations).

I admit I didn't look at how those are implemented.

-- 
Best regards,
Marek Vasut