Subject: [PATCH 1/1] mtd:nand:fix nand_lock/unlock() function

[bpqw]

Hi Brain,
How about my patch? Do you have any other doubts?

Br
White Ding 
-----Original Message-----
From: bpqw 
Sent: Monday, July 28, 2014 3:47 PM
To: Brian Norris; bpqw
Cc: dwmw2 at infradead.org; b32955 at freescale.com; artem.bityutskiy at linux.intel.com; ron at debian.org; u.kleine-koenig at pengutronix.de; ezequiel.garcia at free-electrons.com; linux-mtd at lists.infradead.org; linux-kernel at vger.kernel.org
Subject: RE: Subject: [PATCH 1/1] mtd:nand:fix nand_lock/unlock() function

Hi Brain,

>> Do nand reset before write protect check If we want to check the WP# 
>> low or high through STATUS READ and check bit 7, we must reset the 
>> device, other operation (eg.erase/program a locked block) can also 
>> clear the bit 7 of status register.
>This description doesn't really tell me why we need this patch.
If we want to use the lock/unlock function, we must confirm the WP# is high, if the WP# is low, the write protect is provided by WP#, we don't need LOKC/UNLOCK function.
So before we use the LOCK/UNLOCK function we must confirm the WP# is high.
We can check the WP# is high or low through READ STATUS and check the bit 7, but this only correct when we READ STATUS directly after RESET or Power On.
If we don't add this patch, We can't check the WP# high or low just through READ STATUS and check bit7. 

>First of all, where is the 'lock' sequence specified? I see the commit that introduced nand_lock() (without any users) which says Micron parts support it, but I don't see it documented in the datasheet:
The LOCK/UNLOCK feature not apply all micron nand, only 1.8V device have this feature.

>  commit 7d70f334ad2bf1b3aaa1f0699c0f442e14bcc9e0
>  Author: Vimal Singh <vimal.newwork at gmail.com>
>  Date:   Mon Feb 8 15:50:49 2010 +0530

>      mtd: nand: add lock/unlock routines

>Now, supposing this is documented somewhere, are you seeing some kind 
>of out-of-spec behavior? Is this a controller quirk you're seeing? Why 
>should I need to reset the chip? I would presume that

>  chip->cmdfunc(mtd, NAND_CMD_STATUS, -1, -1);

>would refresh the status properly. Is that not the case?
chip->cmdfunc(mtd, NAND_CMD_STATUS, -1, -1) can refresh the status properly, but we must do some operation to trigger it.
For example if we do rease/program operation to a block that is locked, then READ STATUS, the bit 7 will be 0 that indicate the device is write protect.
Then if we do erase/program operation to another block that is unlocked, the bit 7 of READ STATUS will be 1 indicate that the device is not write protect.

Now if we don't do any operation just through chip->cmdfunc(mtd, NAND_CMD_STATUS, -1, -1); to check the WP# is high or low.
Suppose we check the bit 7 of READ STATUS is 0 then we judge the WP# is low (write protect), but in this case the WP# may be high if we do erase/program operation to a locked block.



Br
White Ding
____________________________
EBU APAC Application Engineering
Tel:86-21-38997078
Mobile: 86-13761729112
Address: No 601 Fasai Rd, Waigaoqiao Free Trade Zone Pudong, Shanghai, China

-----Original Message-----
From: Brian Norris [mailto:computersforpeace at gmail.com]
Sent: Monday, July 28, 2014 2:10 PM
To: bpqw
Cc: dwmw2 at infradead.org; b32955 at freescale.com; artem.bityutskiy at linux.intel.com; ron at debian.org; u.kleine-koenig at pengutronix.de; ezequiel.garcia at free-electrons.com; linux-mtd at lists.infradead.org; linux-kernel at vger.kernel.org
Subject: Re: Subject: [PATCH 1/1] mtd:nand:fix nand_lock/unlock() function

Hi White,

You've responded to one of my messages, but your mail server is also sending bounce replies. You might want to contact your I.T. about this.
(I'll let you know if I ever stop getting bounces.)

On Thu, Jul 24, 2014 at 01:00:01AM +0000, bpqw wrote:
> Do nand reset before write protect check If we want to check the WP# 
> low or high through STATUS READ and check bit 7, we must reset the 
> device, other operation (eg.erase/program a locked block) can also 
> clear the bit 7 of status register.

This description doesn't really tell me why we need this patch.

First of all, where is the 'lock' sequence specified? I see the commit that introduced nand_lock() (without any users) which says Micron parts support it, but I don't see it documented in the datasheet:

  commit 7d70f334ad2bf1b3aaa1f0699c0f442e14bcc9e0
  Author: Vimal Singh <vimal.newwork at gmail.com>
  Date:   Mon Feb 8 15:50:49 2010 +0530

      mtd: nand: add lock/unlock routines

Now, supposing this is documented somewhere, are you seeing some kind of out-of-spec behavior? Is this a controller quirk you're seeing? Why should I need to reset the chip? I would presume that 

  chip->cmdfunc(mtd, NAND_CMD_STATUS, -1, -1);

would refresh the status properly. Is that not the case?

> Signed-off-by: White Ding <bpqw at micron.com>
> ---
>  drivers/mtd/nand/nand_base.c |   18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/mtd/nand/nand_base.c 
> b/drivers/mtd/nand/nand_base.c index 41167e9..22dd3aa 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -965,6 +965,15 @@ int nand_unlock(struct mtd_info *mtd, loff_t ofs, 
> uint64_t len)
>  
>  	chip->select_chip(mtd, chipnr);
>  
> +	/*
> +	 * Reset the chip.
> +	 * If we want to check the WP through READ STATUS and check the bit 7
> +	 * we must reset the chip
> +	 * some operation can also clear the bit 7 of status register
> +	 * eg. erase/program a locked block
> +	 */
> +	chip->cmdfunc(mtd, NAND_CMD_RESET, -1, -1);
> +
>  	/* Check, if it is write protected */
>  	if (nand_check_wp(mtd)) {
>  		pr_debug("%s: device is write protected!\n", @@ -1015,6 +1024,15 @@ 
> int nand_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>  
>  	chip->select_chip(mtd, chipnr);
>  
> +	/*
> +	 * Reset the chip.
> +	 * If we want to check the WP through READ STATUS and check the bit 7
> +	 * we must reset the chip
> +	 * some operation can also clear the bit 7 of status register
> +	 * eg. erase/program a locked block
> +	 */
> +	chip->cmdfunc(mtd, NAND_CMD_RESET, -1, -1);
> +
>  	/* Check, if it is write protected */
>  	if (nand_check_wp(mtd)) {
>  		pr_debug("%s: device is write protected!\n",

Brian