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

[PATCH V3, 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 V3, 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 | 60 ++++++++++++++++++++++++++++++++++--
 1 file changed, 57 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
index c7c4efe..ad8733f 100644
--- a/drivers/mtd/nand/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/brcmnand/brcmnand.c
@@ -24,6 +24,7 @@
 #include <linux/spinlock.h>
 #include <linux/dma-mapping.h>
 #include <linux/ioport.h>
+#include <linux/iopoll.h>
 #include <linux/bug.h>
 #include <linux/kernel.h>
 #include <linux/bitops.h>
@@ -101,6 +102,8 @@ struct brcm_nand_dma_desc {
 #define BRCMNAND_MIN_BLOCKSIZE	(8 * 1024)
 #define BRCMNAND_MIN_DEVSIZE	(4ULL * 1024 * 1024)
 
+#define NAND_CTRL_RDY	(INTFC_CTLR_READY | INTFC_FLASH_READY)
+
 /* Controller feature flags */
 enum {
 	BRCMNAND_HAS_1K_SECTORS			= BIT(0),
@@ -765,12 +768,63 @@ 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,
+				   u32 *reg_val)
+{
+	unsigned long timeout = jiffies + msecs_to_jiffies(200);
+	u32 val;
+
+	while (1) {
+		val = brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS);
+		if ((val & mask) == mask)
+			break;
+
+		if (time_after(jiffies, timeout)) {
+			dev_warn(ctrl->dev, "timeout on ctrl_ready %x\n", val);
+			break;
+		}
+		cpu_relax();
+	}
+
+	*reg_val = val;
+}
+
+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 val;
+	bool is_wp;
+
+	/*
+	 * make sure ctrl/flash ready before and after
+	 * changing state of WP PIN
+	 */
+	bcmnand_ctrl_busy_poll(ctrl, NAND_CTRL_RDY | NAND_STATUS_READY, &val);
+	brcmnand_set_wp_reg(ctrl, en);
+	chip->cmdfunc(mtd, NAND_CMD_STATUS, -1, -1);
+	bcmnand_ctrl_busy_poll(ctrl, NAND_CTRL_RDY | NAND_STATUS_READY, &val);
+	/* NAND_STATUS_WP 0x80 = not protected, 0x00 = protected */
+	is_wp = !(val & NAND_STATUS_WP);
+
+	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 %d\n",
+				    is_wp ? "On" : "Off", val & 0xff,
+				    en ? "On" : "Off", nand_wp ? 1 : 0);
+		return -EINVAL;
+	}
 
 	return 0;
 }
@@ -1167,7 +1221,7 @@ static void brcmnand_send_cmd(struct brcmnand_host *host, int cmd)
 	BUG_ON(ctrl->cmd_pending != 0);
 	ctrl->cmd_pending = cmd;
 
-	intfc = brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS);
+	bcmnand_ctrl_busy_poll(ctrl, NAND_CTRL_RDY, &intfc);
 	WARN_ON(!(intfc & INTFC_CTLR_READY));
 
 	mb(); /* flush previous writes */
-- 
1.9.1

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

[Boris Brezillon]

Hi Kamal,

This time I'm not even in the recipients list :-(. Can you please make
sure you Cc all the relevant maintainers when sending a patch (this is
not the first time it happens).

Thanks,

Boris

On Mon, 20 Feb 2017 22:30:31 -0500
Kamal Dasu <kdasu.kdev@gmail.com> wrote:

> 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;
> +			}
> +
>  		}
>  	}
>  

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

[Kamal Dasu]

Ok will make sure the mtd email list is up-to-date.

Thanks
Kamal

On Mon, Feb 27, 2017 at 11:09 AM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> Hi Kamal,
>
> This time I'm not even in the recipients list :-(. Can you please make
> sure you Cc all the relevant maintainers when sending a patch (this is
> not the first time it happens).
>
> Thanks,
>
> Boris
>
> On Mon, 20 Feb 2017 22:30:31 -0500
> Kamal Dasu <kdasu.kdev@gmail.com> wrote:
>
>> 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;
>> +                     }
>> +
>>               }
>>       }
>>
>

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

[Boris Brezillon]

Hi Kamal,

On Mon, 20 Feb 2017 22:30:32 -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.

v6.x and v7.x of the brcmnand controller let the driver
assert/de-assert the #WP pin via the 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

			  ^ To fix this, check the NAND status in
addition to the NAND controller #WP pin status. This way, we make sure
both sides are ready to accept new commands.

> 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 | 60 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 57 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
> index c7c4efe..ad8733f 100644
> --- a/drivers/mtd/nand/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/brcmnand/brcmnand.c
> @@ -24,6 +24,7 @@
>  #include <linux/spinlock.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/ioport.h>
> +#include <linux/iopoll.h>
>  #include <linux/bug.h>
>  #include <linux/kernel.h>
>  #include <linux/bitops.h>
> @@ -101,6 +102,8 @@ struct brcm_nand_dma_desc {
>  #define BRCMNAND_MIN_BLOCKSIZE	(8 * 1024)
>  #define BRCMNAND_MIN_DEVSIZE	(4ULL * 1024 * 1024)
>  
> +#define NAND_CTRL_RDY	(INTFC_CTLR_READY | INTFC_FLASH_READY)
> +
>  /* Controller feature flags */
>  enum {
>  	BRCMNAND_HAS_1K_SECTORS			= BIT(0),
> @@ -765,12 +768,63 @@ 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,
> +				   u32 *reg_val)

I'd rename the function into bcmnand_poll_status(), since you're not
necessarily polling for the R/B bit.

> +{
> +	unsigned long timeout = jiffies + msecs_to_jiffies(200);

Where do you get this 200 msec value from. It's probably better if you
pass this timeout val through a parameter.

> +	u32 val;
> +
> +	while (1) {
> +		val = brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS);
> +		if ((val & mask) == mask)
> +			break;
> +
> +		if (time_after(jiffies, timeout)) {
> +			dev_warn(ctrl->dev, "timeout on ctrl_ready %x\n", val);
> +			break;
> +		}
> +		cpu_relax();
> +	}
> +
> +	*reg_val = val;
> +}

I still think this function should return 0 in case of success and
-ETIMEDOUT in case of error.

static int bcmnand_poll_status(struct brcmnand_controller *ctrl,
			       u32 mask, u32 expected_val,
			       unsigned long timeout_ms)
{
	unsigned long limit = jiffies + msecs_to_jiffies(timeout_ms);
	u32 val;

	if (!timeout_ms)
		timeout_ms = POLL_STATUS_DEFAULT_TIMEOUT_MS;

	limit = jiffies + msecs_to_jiffies(timeout_ms);
	do {
		val = brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS);
		if ((val & mask) == expected_val)
			return 0;

		cpu_relax();
	} while (time_after(timeout, jiffies));

	/* Maybe we should read the status one last time here? */
	dev_warn(ctrl->dev,
		 "timeout on status poll (expected %x got %x)\n",
		 expected_val, val);

	return -ETIMEDOUT;
}

> +
> +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 val;
> +	bool is_wp;
> +
> +	/*
> +	 * make sure ctrl/flash ready before and after
> +	 * changing state of WP PIN
> +	 */

You could replace:

> +	bcmnand_ctrl_busy_poll(ctrl, NAND_CTRL_RDY | NAND_STATUS_READY, &val);
> +	brcmnand_set_wp_reg(ctrl, en);
> +	chip->cmdfunc(mtd, NAND_CMD_STATUS, -1, -1);
> +	bcmnand_ctrl_busy_poll(ctrl, NAND_CTRL_RDY | NAND_STATUS_READY, &val);
> +	/* NAND_STATUS_WP 0x80 = not protected, 0x00 = protected */
> +	is_wp = !(val & NAND_STATUS_WP);
> +
> +	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 %d\n",
> +				    is_wp ? "On" : "Off", val & 0xff,
> +				    en ? "On" : "Off", nand_wp ? 1 : 0);
> +		return -EINVAL;
> +	}

by:

	ret = bcmnand_poll_status(ctrl,
				  NAND_CTRL_RDY | NAND_STATUS_READY,
				  NAND_CTRL_RDY | NAND_STATUS_READY, 0);
	if (ret)
		return ret;

	brcmnand_set_wp_reg(ctrl, en);
	chip->cmdfunc(mtd, NAND_CMD_STATUS, -1, -1);

	ret = bcmnand_poll_status(ctrl,
				  NAND_CTRL_RDY | NAND_STATUS_READY |
				  NAND_STATUS_WP,
				  NAND_CTRL_RDY | NAND_STATUS_READY |
				  (en ? NAND_STATUS_WP : 0),
				  0);
	if (ret)
		return ret;


You can add the dev_err_ratelimited() trace if you feel is necessary,
but there's already the same kind of warning in bcmnand_poll_status().

>  
>  	return 0;
>  }
> @@ -1167,7 +1221,7 @@ static void brcmnand_send_cmd(struct brcmnand_host *host, int cmd)
>  	BUG_ON(ctrl->cmd_pending != 0);
>  	ctrl->cmd_pending = cmd;
>  
> -	intfc = brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS);
> +	bcmnand_ctrl_busy_poll(ctrl, NAND_CTRL_RDY, &intfc);
>  	WARN_ON(!(intfc & INTFC_CTLR_READY));

	ret = bcmnand_poll_status(ctrl, NAND_CTRL_RDY,
				  NAND_CTRL_RDY, 0);
	WARN_ON(ret);

>  
>  	mb(); /* flush previous writes */

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

[Kamal Dasu]

Boris,

On Mon, Feb 27, 2017 at 12:15 PM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> Hi Kamal,
>
> On Mon, 20 Feb 2017 22:30:32 -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.
>
> v6.x and v7.x of the brcmnand controller let the driver
> assert/de-assert the #WP pin via the 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
>
>                           ^ To fix this, check the NAND status in
> addition to the NAND controller #WP pin status. This way, we make sure
> both sides are ready to accept new commands.
>

Will rephrase it.

>> 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 | 60 ++++++++++++++++++++++++++++++++++--
>>  1 file changed, 57 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
>> index c7c4efe..ad8733f 100644
>> --- a/drivers/mtd/nand/brcmnand/brcmnand.c
>> +++ b/drivers/mtd/nand/brcmnand/brcmnand.c
>> @@ -24,6 +24,7 @@
>>  #include <linux/spinlock.h>
>>  #include <linux/dma-mapping.h>
>>  #include <linux/ioport.h>
>> +#include <linux/iopoll.h>
>>  #include <linux/bug.h>
>>  #include <linux/kernel.h>
>>  #include <linux/bitops.h>
>> @@ -101,6 +102,8 @@ struct brcm_nand_dma_desc {
>>  #define BRCMNAND_MIN_BLOCKSIZE       (8 * 1024)
>>  #define BRCMNAND_MIN_DEVSIZE (4ULL * 1024 * 1024)
>>
>> +#define NAND_CTRL_RDY        (INTFC_CTLR_READY | INTFC_FLASH_READY)
>> +
>>  /* Controller feature flags */
>>  enum {
>>       BRCMNAND_HAS_1K_SECTORS                 = BIT(0),
>> @@ -765,12 +768,63 @@ 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,
>> +                                u32 *reg_val)
>
> I'd rename the function into bcmnand_poll_status(), since you're not
> necessarily polling for the R/B bit.
>
>> +{
>> +     unsigned long timeout = jiffies + msecs_to_jiffies(200);
>
> Where do you get this 200 msec value from. It's probably better if you
> pass this timeout val through a parameter.
>

Its worst case value recommended for the controller hardware based on
testing under various conditions.

>> +     u32 val;
>> +
>> +     while (1) {
>> +             val = brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS);
>> +             if ((val & mask) == mask)
>> +                     break;
>> +
>> +             if (time_after(jiffies, timeout)) {
>> +                     dev_warn(ctrl->dev, "timeout on ctrl_ready %x\n", val);
>> +                     break;
>> +             }
>> +             cpu_relax();
>> +     }
>> +
>> +     *reg_val = val;
>> +}
>
> I still think this function should return 0 in case of success and
> -ETIMEDOUT in case of error.
>

Will change it as per your recommendation.

> static int bcmnand_poll_status(struct brcmnand_controller *ctrl,
>                                u32 mask, u32 expected_val,
>                                unsigned long timeout_ms)
> {
>         unsigned long limit = jiffies + msecs_to_jiffies(timeout_ms);
>         u32 val;
>
>         if (!timeout_ms)
>                 timeout_ms = POLL_STATUS_DEFAULT_TIMEOUT_MS;
>
>         limit = jiffies + msecs_to_jiffies(timeout_ms);
>         do {
>                 val = brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS);
>                 if ((val & mask) == expected_val)
>                         return 0;
>
>                 cpu_relax();
>         } while (time_after(timeout, jiffies));
>
>         /* Maybe we should read the status one last time here? */
>         dev_warn(ctrl->dev,
>                  "timeout on status poll (expected %x got %x)\n",
>                  expected_val, val);
>
>         return -ETIMEDOUT;
> }
>

Ok will use this logic. I think you mean
>         } while (time_after(limit, jiffies));

>> +
>> +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 val;
>> +     bool is_wp;
>> +
>> +     /*
>> +      * make sure ctrl/flash ready before and after
>> +      * changing state of WP PIN
>> +      */
>
> You could replace:
>
>> +     bcmnand_ctrl_busy_poll(ctrl, NAND_CTRL_RDY | NAND_STATUS_READY, &val);
>> +     brcmnand_set_wp_reg(ctrl, en);
>> +     chip->cmdfunc(mtd, NAND_CMD_STATUS, -1, -1);
>> +     bcmnand_ctrl_busy_poll(ctrl, NAND_CTRL_RDY | NAND_STATUS_READY, &val);
>> +     /* NAND_STATUS_WP 0x80 = not protected, 0x00 = protected */
>> +     is_wp = !(val & NAND_STATUS_WP);
>> +
>> +     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 %d\n",
>> +                                 is_wp ? "On" : "Off", val & 0xff,
>> +                                 en ? "On" : "Off", nand_wp ? 1 : 0);
>> +             return -EINVAL;
>> +     }
>
> by:
>
>         ret = bcmnand_poll_status(ctrl,
>                                   NAND_CTRL_RDY | NAND_STATUS_READY,
>                                   NAND_CTRL_RDY | NAND_STATUS_READY, 0);
>         if (ret)
>                 return ret;
>
>         brcmnand_set_wp_reg(ctrl, en);
>         chip->cmdfunc(mtd, NAND_CMD_STATUS, -1, -1);
>
>         ret = bcmnand_poll_status(ctrl,
>                                   NAND_CTRL_RDY | NAND_STATUS_READY |
>                                   NAND_STATUS_WP,
>                                   NAND_CTRL_RDY | NAND_STATUS_READY |
>                                   (en ? NAND_STATUS_WP : 0),
>                                   0);
>         if (ret)
>                 return ret;
>
>
> You can add the dev_err_ratelimited() trace if you feel is necessary,
> but there's already the same kind of warning in bcmnand_poll_status().
>

Will make the above changes as well.
Also since  /* NAND_STATUS_WP 0x00 = protected, 0x80 = not protected */,
so the expected_val mask would be as below.

>         ret = bcmnand_poll_status(ctrl,
>                                   NAND_CTRL_RDY | NAND_STATUS_READY |
>                                   NAND_STATUS_WP,
>                                   NAND_CTRL_RDY | NAND_STATUS_READY |
>                                   (en ? 0 : NAND_STATUS_WP),
>                                   0);

>>
>>       return 0;
>>  }
>> @@ -1167,7 +1221,7 @@ static void brcmnand_send_cmd(struct brcmnand_host *host, int cmd)
>>       BUG_ON(ctrl->cmd_pending != 0);
>>       ctrl->cmd_pending = cmd;
>>
>> -     intfc = brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS);
>> +     bcmnand_ctrl_busy_poll(ctrl, NAND_CTRL_RDY, &intfc);
>>       WARN_ON(!(intfc & INTFC_CTLR_READY));
>
>         ret = bcmnand_poll_status(ctrl, NAND_CTRL_RDY,
>                                   NAND_CTRL_RDY, 0);
>         WARN_ON(ret);
>
>>
>>       mb(); /* flush previous writes */

Thanks
Kamal