RE: [PATCH 1/3] mtd: cfi_cmdset_0002: Change do_write_oneword() to use chip_good()

[IKEGAMI Tokunori <ikegami@xxxxxxxxxxxxxxxxxxxx>]

Let me comment below in the mail.

> -----Original Message-----
> From: Boris Brezillon [mailto:boris.brezillon@xxxxxxxxxxx]
> Sent: Friday, October 19, 2018 5:30 PM
> To: IKEGAMI Tokunori
> Cc: boris.brezillon@xxxxxxxxxxxxxxxxxx; Fabio Bettoni; PACKHAM Chris;
> Joakim Tjernlund; linux-mtd@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH 1/3] mtd: cfi_cmdset_0002: Change do_write_oneword()
> to use chip_good()
> 
> On Fri, 19 Oct 2018 17:13:18 +0900
> Tokunori Ikegami <ikegami@xxxxxxxxxxxxxxxxxxxx> wrote:
> 
> > This is required for OpenWrt Project to result the flash write issue as
> > below patche.
> >
> >
> <https://git.openwrt.org/?p=openwrt/openwrt.git;a=commitdiff;h=ddc11c3
> 932c7b7b7df7d5fbd48f207e77619eaa7>
> 
> This part is not really explaining why this fix is needed. Can you
> please give more details about why chip_good() should be used instead of
> chip_ready().

[Ikegami] The reason is that just actually fix the issue.
And also in the past I fixed the erase function also as same way by the patch below.
  <https://patchwork.ozlabs.org/patch/922656/>
   Note: The reason for the patch for erase is same.
In my understanding the chip_ready() is just checked the value twice from flash.
So I think that sometimes incorrect value is read twice and it is depended on the flash device behavior but not sure..

Hauke-san and Koen-san,

  If you have any advice please let me know.


> 
> >
> > So change to use chip_good() instead of chip_ready().
> >
> > Signed-off-by: Tokunori Ikegami <ikegami@xxxxxxxxxxxxxxxxxxxx>
> > Cc: Fabio Bettoni <fbettoni@xxxxxxxxx>
> > Cc: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx>
> > Cc: Joakim Tjernlund <Joakim.Tjernlund@xxxxxxxxxxxx>
> > Cc: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>
> > Cc: linux-mtd@xxxxxxxxxxxxxxxxxxx
> 
> Would be great to have Fixes and Cc-stable tags here.

[Ikegami] I see I will do that.

> 
> > ---
> >  drivers/mtd/chips/cfi_cmdset_0002.c | 18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c
> b/drivers/mtd/chips/cfi_cmdset_0002.c
> > index 72428b6bfc47..251c9e1675bd 100644
> > --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> > @@ -1627,31 +1627,37 @@ static int __xipram do_write_oneword(struct
> map_info *map, struct flchip *chip,
> >  			continue;
> >  		}
> >
> > -		if (time_after(jiffies, timeo) && !chip_ready(map, adr)){
> > +		if (chip_good(map, adr, datum))
> > +			break;
> > +
> > +		if (time_after(jiffies, timeo)){
> >  			xip_enable(map, chip, adr);
> >  			printk(KERN_WARNING "MTD %s(): software
> timeout\n", __func__);
> >  			xip_disable(map, chip, adr);
> > +			ret = -EIO;
> >  			break;
> >  		}
> >
> > -		if (chip_ready(map, adr))
> > -			break;
> > -
> >  		/* Latency issues. Drop the lock, wait a while and retry
> */
> >  		UDELAY(map, chip, adr, 1);
> >  	}
> > +
> >  	/* Did we succeed? */
> > -	if (!chip_good(map, adr, datum)) {
> > +	if (ret) {
> >  		/* reset on all failures. */
> >  		map_write(map, CMD(0xF0), chip->start);
> >  		/* FIXME - should have reset delay before continuing */
> >
> > -		if (++retry_cnt <= MAX_RETRIES)
> > +		if (++retry_cnt <= MAX_RETRIES) {
> > +			ret = 0;
> >  			goto retry;
> > +		}
> >
> >  		ret = -EIO;
> >  	}
> > +
> >  	xip_enable(map, chip, adr);
> > +
> >   op_done:
> >  	if (mode == FL_OTP_WRITE)
> >  		otp_exit(map, chip, adr, map_bankwidth(map));


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