Re: [PATCH 11/17] mtd: rawnand: cafe: Don't leave ECC enabled in the write path

[Miquel Raynal <miquel.raynal@xxxxxxxxxxx>]

Hi Boris,

Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> wrote on Mon, 27 Apr
2020 10:20:21 +0200:

> cafe_nand_write_page_lowlevel() sets the ECC auto-generation flag but

IIRC you renamed this function in patch 1 so now it is named
"_write_page()".

> never clears it, thus forcing the cafe_nand_cmdfunc() to clear it
> in certain circumstances. Let's just clear this flag in
> cafe_nand_write_page_lowlevel() instead.

Same here              ^

> 
> Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> ---
>  drivers/mtd/nand/raw/cafe_nand.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/cafe_nand.c b/drivers/mtd/nand/raw/cafe_nand.c
> index 2825489a71b8..31493a201a02 100644
> --- a/drivers/mtd/nand/raw/cafe_nand.c
> +++ b/drivers/mtd/nand/raw/cafe_nand.c
> @@ -261,7 +261,6 @@ static void cafe_nand_cmdfunc(struct nand_chip *chip, unsigned command,
>  			    CAFE_FIELD_PREP(NAND_CTRL2, CMD2, command),
>  			    NAND_CTRL2);
>  		ctl1 = cafe->ctl1;
> -		cafe->ctl2 &= ~CAFE_NAND_CTRL2_AUTO_WRITE_ECC;
>  		dev_dbg(&cafe->pdev->dev, "Continue command, ctl1 %08x, #data %d\n",
>  			cafe->ctl1, cafe->nr_data);
>  		goto do_command;
> @@ -643,6 +642,7 @@ static int cafe_nand_write_page(struct nand_chip *chip,
>  {
>  	struct mtd_info *mtd = nand_to_mtd(chip);
>  	struct cafe_priv *cafe = nand_get_controller_data(chip);
> +	int ret;
>  
>  	nand_prog_page_begin_op(chip, page, 0, buf, mtd->writesize);
>  	chip->legacy.write_buf(chip, chip->oob_poi, mtd->oobsize);
> @@ -650,7 +650,14 @@ static int cafe_nand_write_page(struct nand_chip *chip,
>  	/* Set up ECC autogeneration */
>  	cafe->ctl2 |= CAFE_NAND_CTRL2_AUTO_WRITE_ECC;
>  
> -	return nand_prog_page_end_op(chip);
> +	ret = nand_prog_page_end_op(chip);
> +
> +	/*
> +	 * And clear it before returning so that following write operations
> +	 * that do not involve ECC don't generate ECC bytes.
> +	 */
> +	cafe->ctl2 &= ~CAFE_NAND_CTRL2_AUTO_WRITE_ECC;

I like spaces before returning, but again it's really a nitpick, you
can ignore that :)

> +	return ret;
>  }
>  
>  /* F_2[X]/(X**6+X+1)  */


With the commit log updated

Reviewed-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/