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

[Florian Fainelli <florian.fainelli@broadcom.com>]

On 03/15/2017 07:01 AM, Boris Brezillon wrote:
> On Fri, 10 Mar 2017 14:22:39 +0100
> Marek Vasut <marek.vasut@gmail.com> wrote:
> 
>> On 03/03/2017 10:16 PM, Kamal Dasu wrote:
>>> On brcmnand controller v6.x and v7.x, the #WP pin is controlled through
>>> the NAND_WP bit in CS_SELECT register.
>>>
>>> The driver currently assumes that toggling the #WP pin is
>>> instantaneously enabling/disabling write-protection, but it actually
>>> takes some time to propagate the new state to the internal NAND chip
>>> logic. This behavior is sometime causing data corruptions when an
>>> erase/program operation is executed before write-protection has really
>>> been disabled.
>>>
>>> Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB NAND controller")
>>> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
>>> ---
>>>  drivers/mtd/nand/brcmnand/brcmnand.c | 61 ++++++++++++++++++++++++++++++++++--
>>>  1 file changed, 58 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
>>> index 42ebd73..7419c5c 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 NAND_CTRL_RDY			(INTFC_CTLR_READY | INTFC_FLASH_READY)
>>> +#define NAND_POLL_STATUS_TIMEOUT_MS	100
>>> +
>>>  /* Controller feature flags */
>>>  enum {
>>>  	BRCMNAND_HAS_1K_SECTORS			= BIT(0),
>>> @@ -765,6 +768,31 @@ enum {
>>>  	CS_SELECT_AUTO_DEVICE_ID_CFG		= BIT(30),
>>>  };
>>>  
>>> +static int bcmnand_ctrl_poll_status(struct brcmnand_controller *ctrl,
>>> +				    u32 mask, u32 expected_val,
>>> +				    unsigned long timeout_ms)
>>> +{
>>> +	unsigned long limit;
>>> +	u32 val;
>>> +
>>> +	if (!timeout_ms)
>>> +		timeout_ms = NAND_POLL_STATUS_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(limit, jiffies));
>>> +
>>> +	dev_warn(ctrl->dev, "timeout on status poll (expected %x got %x)\n",
>>> +		 expected_val, val & mask);
>>> +
>>> +	return -ETIMEDOUT;
>>> +}
>>> +
>>>  static inline void brcmnand_set_wp(struct brcmnand_controller *ctrl, bool en)
>>>  {
>>>  	u32 val = en ? CS_SELECT_NAND_WP : 0;
>>> @@ -1024,12 +1052,39 @@ static void brcmnand_wp(struct mtd_info *mtd, int wp)
>>>  
>>>  	if ((ctrl->features & BRCMNAND_HAS_WP) && wp_on == 1) {
>>>  		static int old_wp = -1;  
>>
>> Unrelated to this patch, but this static variable should be moved to
>> driver's private data instead.
>>
>>> +		int ret;
>>>  
>>>  		if (old_wp != wp) {
>>>  			dev_dbg(ctrl->dev, "WP %s\n", wp ? "on" : "off");
>>>  			old_wp = wp;
>>>  		}
>>> +
>>> +		/*
>>> +		 * make sure ctrl/flash ready before and after
>>> +		 * changing state of #WP pin
>>> +		 */  
>>
>> Shouldn't the brcmnand_set_wp() do this ?
> 
> Hm, AFAIU, brcmnand_set_wp() is only controlling the WP pin from the
> controller side, so maybe we should rename the function
> brcmnand_ctrl_set_wp() to clarify that.

While I don't object that this change should be made, we are already at
version 6 here, and this is a bugfix, so what else needs to be done to
get this included?
-- 
Florian