Patchwork mtd: gpmi: add NAND write verify support

login
register
mail settings
Submitter Huang Shijie
Date Aug. 10, 2012, 3:18 a.m.
Message ID <1344568737-902-1-git-send-email-b32955@freescale.com>
Download mbox | patch
Permalink /patch/176353/
State New
Headers show

Comments

Huang Shijie - Aug. 10, 2012, 3:18 a.m.
Add NAND write verify support in gpmi-nand driver.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)
Fabio Estevam - Aug. 10, 2012, 4:12 a.m.
On Fri, Aug 10, 2012 at 12:18 AM, Huang Shijie <b32955@freescale.com> wrote:
> Add NAND write verify support in gpmi-nand driver.
>
> Signed-off-by: Huang Shijie <b32955@freescale.com>

Tested by: Fabio Estevam <fabio.estevam@freescale.com>
Marek Vasut - Aug. 10, 2012, 11:25 a.m.
Dear Huang Shijie,

> Add NAND write verify support in gpmi-nand driver.
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c |   17 +++++++++++++++++
>  1 files changed, 17 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c index 8c0d2f0..6394483 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> @@ -1488,6 +1488,22 @@ static int gpmi_set_geometry(struct gpmi_nand_data
> *this) return gpmi_alloc_dma_buffer(this);
>  }
> 
> +#define MAX_PAGESIZE 8192

Use NAND_MAX_PAGESIZE, see include/linux/mtd/nand.h .

> +static uint8_t verify_buf[MAX_PAGESIZE];

What will happen when you have two NANDs attached to GPMI controller and they 
hit this place both at once? Race condition, causing this function to fail for 
both?

Possibly devm_kmalloc() such buffer per-controller?

> +static int gpmi_verify_buf(struct mtd_info *mtd, const uint8_t *buf, int
> len) +{
> +	struct nand_chip *nand = mtd->priv;
> +	int ret;
> +
> +	ret = gpmi_ecc_read_page(mtd, nand, verify_buf, 0, 0);

mtd->chip.ecc.read_page() ?

> +	if (ret)
> +		return -EFAULT;
> +	if (memcmp(buf, verify_buf, len))
> +		return -EFAULT;
> +	return 0;
> +}
> +
>  static int gpmi_pre_bbt_scan(struct gpmi_nand_data  *this)
>  {
>  	int ret;
> @@ -1556,6 +1572,7 @@ static int __devinit gpmi_nfc_init(struct
> gpmi_nand_data *this) chip->read_byte		= gpmi_read_byte;
>  	chip->read_buf		= gpmi_read_buf;
>  	chip->write_buf		= gpmi_write_buf;
> +	chip->verify_buf	= gpmi_verify_buf;
>  	chip->ecc.read_page	= gpmi_ecc_read_page;
>  	chip->ecc.write_page	= gpmi_ecc_write_page;
>  	chip->ecc.read_oob	= gpmi_ecc_read_oob;

Best regards,
Marek Vasut
Huang Shijie - Aug. 10, 2012, 2:15 p.m.
On Fri, Aug 10, 2012 at 7:25 AM, Marek Vasut <marex@denx.de> wrote:
> Dear Huang Shijie,
>
>> Add NAND write verify support in gpmi-nand driver.
>>
>> Signed-off-by: Huang Shijie <b32955@freescale.com>
>> ---
>>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c |   17 +++++++++++++++++
>>  1 files changed, 17 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
>> b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c index 8c0d2f0..6394483 100644
>> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
>> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
>> @@ -1488,6 +1488,22 @@ static int gpmi_set_geometry(struct gpmi_nand_data
>> *this) return gpmi_alloc_dma_buffer(this);
>>  }
>>
>> +#define MAX_PAGESIZE 8192
>
> Use NAND_MAX_PAGESIZE, see include/linux/mtd/nand.h .
>
>> +static uint8_t verify_buf[MAX_PAGESIZE];

ok, thanks.

>
> What will happen when you have two NANDs attached to GPMI controller and they
> hit this place both at once? Race condition, causing this function to fail for
> both?
>
Even there are two nand chips, there is only one gpmi controller.
And the current gpmi-nand does not support two nands now.
does the race occur with only one nand chip?


> Possibly devm_kmalloc() such buffer per-controller?
>
>> +static int gpmi_verify_buf(struct mtd_info *mtd, const uint8_t *buf, int
>> len) +{
>> +     struct nand_chip *nand = mtd->priv;
>> +     int ret;
>> +
>> +     ret = gpmi_ecc_read_page(mtd, nand, verify_buf, 0, 0);
>
> mtd->chip.ecc.read_page() ?
In actually, they are the same.


Best Regards
Huang Shijie
Marek Vasut - Aug. 10, 2012, 3:04 p.m.
Dear Huang Shijie,

> On Fri, Aug 10, 2012 at 7:25 AM, Marek Vasut <marex@denx.de> wrote:
> > Dear Huang Shijie,
> > 
> >> Add NAND write verify support in gpmi-nand driver.
> >> 
> >> Signed-off-by: Huang Shijie <b32955@freescale.com>
> >> ---
> >> 
> >>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c |   17 +++++++++++++++++
> >>  1 files changed, 17 insertions(+), 0 deletions(-)
> >> 
> >> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> >> b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c index 8c0d2f0..6394483 100644
> >> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> >> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> >> @@ -1488,6 +1488,22 @@ static int gpmi_set_geometry(struct
> >> gpmi_nand_data *this) return gpmi_alloc_dma_buffer(this);
> >> 
> >>  }
> >> 
> >> +#define MAX_PAGESIZE 8192
> > 
> > Use NAND_MAX_PAGESIZE, see include/linux/mtd/nand.h .
> > 
> >> +static uint8_t verify_buf[MAX_PAGESIZE];
> 
> ok, thanks.
> 
> > What will happen when you have two NANDs attached to GPMI controller and
> > they hit this place both at once? Race condition, causing this function
> > to fail for both?
> 
> Even there are two nand chips, there is only one gpmi controller.

For now ... yes.

> And the current gpmi-nand does not support two nands now.
> does the race occur with only one nand chip?

With one chip and one GPMI controller, no ... but that's no reason to introduce 
crappy and error prone code!

> > Possibly devm_kmalloc() such buffer per-controller?
> > 
> >> +static int gpmi_verify_buf(struct mtd_info *mtd, const uint8_t *buf,
> >> int len) +{
> >> +     struct nand_chip *nand = mtd->priv;
> >> +     int ret;
> >> +
> >> +     ret = gpmi_ecc_read_page(mtd, nand, verify_buf, 0, 0);
> > 
> > mtd->chip.ecc.read_page() ?
> 
> In actually, they are the same.

They are, but if you rename the function, this is one less place to care for.

> 
> Best Regards
> Huang Shijie

Best regards,
Marek Vasut

Patch

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index 8c0d2f0..6394483 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -1488,6 +1488,22 @@  static int gpmi_set_geometry(struct gpmi_nand_data *this)
 	return gpmi_alloc_dma_buffer(this);
 }
 
+#define MAX_PAGESIZE 8192
+static uint8_t verify_buf[MAX_PAGESIZE];
+
+static int gpmi_verify_buf(struct mtd_info *mtd, const uint8_t *buf, int len)
+{
+	struct nand_chip *nand = mtd->priv;
+	int ret;
+
+	ret = gpmi_ecc_read_page(mtd, nand, verify_buf, 0, 0);
+	if (ret)
+		return -EFAULT;
+	if (memcmp(buf, verify_buf, len))
+		return -EFAULT;
+	return 0;
+}
+
 static int gpmi_pre_bbt_scan(struct gpmi_nand_data  *this)
 {
 	int ret;
@@ -1556,6 +1572,7 @@  static int __devinit gpmi_nfc_init(struct gpmi_nand_data *this)
 	chip->read_byte		= gpmi_read_byte;
 	chip->read_buf		= gpmi_read_buf;
 	chip->write_buf		= gpmi_write_buf;
+	chip->verify_buf	= gpmi_verify_buf;
 	chip->ecc.read_page	= gpmi_ecc_read_page;
 	chip->ecc.write_page	= gpmi_ecc_write_page;
 	chip->ecc.read_oob	= gpmi_ecc_read_oob;