[v2,4/9] mtd: gpmi: rewrite the gpmi_ecc_write_oob() to support the jffs2

When we use the ECC info which is get from the nand chip's datasheet,
we may have some freed oob area now.

This patch rewrites the gpmi_ecc_write_oob() to implement the ecc.write_oob().
We also update the comment for gpmi_hw_ecclayout.

Yes! We can support the JFFS2 for the SLC nand now.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c |   31 ++++++++++++++++++++++---------
 1 files changed, 22 insertions(+), 9 deletions(-)

On Mon, Aug 19, 2013 at 10:31:13AM +0800, Huang Shijie wrote:
> When we use the ECC info which is get from the nand chip's datasheet,
> we may have some freed oob area now.
> 
> This patch rewrites the gpmi_ecc_write_oob() to implement the ecc.write_oob().
> We also update the comment for gpmi_hw_ecclayout.
> 
> Yes! We can support the JFFS2 for the SLC nand now.
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c |   31 ++++++++++++++++++++++---------
>  1 files changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> index 9c89e80..cc0306b 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> @@ -45,7 +45,10 @@ static struct nand_bbt_descr gpmi_bbt_descr = {
>  	.pattern	= scan_ff_pattern
>  };
>  
> -/*  We will use all the (page + OOB). */
> +/*
> + * We may change the layout if we can get the ECC info from the datasheet,
> + * else we will use all the (page + OOB).
> + */
>  static struct nand_ecclayout gpmi_hw_ecclayout = {
>  	.eccbytes = 0,
>  	.eccpos = { 0, },
> @@ -1263,14 +1266,24 @@ static int gpmi_ecc_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
>  static int
>  gpmi_ecc_write_oob(struct mtd_info *mtd, struct nand_chip *chip, int page)
>  {
> -	/*
> -	 * The BCH will use all the (page + oob).
> -	 * Our gpmi_hw_ecclayout can only prohibit the JFFS2 to write the oob.
> -	 * But it can not stop some ioctls such MEMWRITEOOB which uses
> -	 * MTD_OPS_PLACE_OOB. So We have to implement this function to prohibit
> -	 * these ioctls too.
> -	 */
> -	return -EPERM;
> +	struct nand_oobfree *of = gpmi_hw_ecclayout.oobfree;

Why do you directly access your static layout? I think you should use
the proper indirection, through chip->ecc.layout. (There's a similar
issue with set_geometry_by_ecc_info() currently.)

[BTW, I was about to recommend nand_chip.ecclayout, but it looks like
that is just a dead field. It's not initialized anywhere, AFAICT, and if
it's used anywhere, it's more likely a bug than anything else...]

> +	int status = 0;
> +
> +	/* Do we have available oob area? */
> +	if (!of->length)
> +		return -EPERM;
> +
> +	if (!nand_is_slc(chip))
> +		return -EPERM;
> +
> +	pr_debug("page number is %d\n", page);
> +
> +	chip->cmdfunc(mtd, NAND_CMD_SEQIN, mtd->writesize + of->offset, page);
> +	chip->write_buf(mtd, chip->oob_poi + of->offset, of->length);
> +	chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
> +
> +	status = chip->waitfunc(mtd, chip);
> +	return status & NAND_STATUS_FAIL ? -EIO : 0;
>  }
>  
>  static int gpmi_block_markbad(struct mtd_info *mtd, loff_t ofs)

Brian

On Sat, Aug 24, 2013 at 03:18:13PM -0400, Huang Shijie wrote:
> On Fri, Aug 23, 2013 at 11:19:16PM -0700, Brian Norris wrote:
> > On Mon, Aug 19, 2013 at 10:31:13AM +0800, Huang Shijie wrote:
> > > -	 */
> > > -	return -EPERM;
> > > +	struct nand_oobfree *of = gpmi_hw_ecclayout.oobfree;
> > 
> > Why do you directly access your static layout? I think you should use
> > the proper indirection, through chip->ecc.layout. (There's a similar
> 		
> yes, it is better to use the chip->ecc.layout.
> 
> > issue with set_geometry_by_ecc_info() currently.)
> 
> Not a same issue.  The set_geometry_by_ecc_info() is called before we
> call the nand_scan_tail(). so it should not changed.

OK.

> > 
> > [BTW, I was about to recommend nand_chip.ecclayout, but it looks like
> > that is just a dead field. It's not initialized anywhere, AFAICT, and if
> The jffs2 write may also uses the ecclayout. 

It uses mtd_info.ecclayout. I'm pretty sure nand_chip.ecclayout is never
used (except for a debug print).

Brian

On Fri, Aug 23, 2013 at 11:19:16PM -0700, Brian Norris wrote:
> On Mon, Aug 19, 2013 at 10:31:13AM +0800, Huang Shijie wrote:
> > -	 */
> > -	return -EPERM;
> > +	struct nand_oobfree *of = gpmi_hw_ecclayout.oobfree;
> 
> Why do you directly access your static layout? I think you should use
> the proper indirection, through chip->ecc.layout. (There's a similar
		
yes, it is better to use the chip->ecc.layout.

> issue with set_geometry_by_ecc_info() currently.)

Not a same issue.  The set_geometry_by_ecc_info() is called before we
call the nand_scan_tail(). so it should not changed.

> 
> [BTW, I was about to recommend nand_chip.ecclayout, but it looks like
> that is just a dead field. It's not initialized anywhere, AFAICT, and if
The jffs2 write may also uses the ecclayout. 

thanks
Huang Shijie