[PATCH] mtd: spi-nor: fixed spansion quad enable

[Esponde, Joel]

Hi Cyrille,

I apologize but I did not have time to work on this.
I will try to send you the patch this evening (European time).

Regards,

Joël Esponde
Honeywell | Safety and Productivity Solutions

> -----Message d'origine-----
> De : Cyrille Pitchen [mailto:cyrille.pitchen at atmel.com]
> Envoyé : mercredi 16 novembre 2016 13:58
> À : Esponde, Joel <Joel.Esponde at Honeywell.com>; linux-
> mtd at lists.infradead.org; Marek Vasut <marex at denx.de>
> Objet : Re: [PATCH] mtd: spi-nor: fixed spansion quad enable
> 
> Hi Joel,
> 
> +Marek
> 
> Le 24/10/2016 à 10:33, Esponde, Joel a écrit :
> > Hi Cyrille,
> >
> >> -----Message d'origine-----
> >> De : Cyrille Pitchen [mailto:cyrille.pitchen at atmel.com]
> >> Envoyé : vendredi 21 octobre 2016 14:50 À : Esponde, Joel
> >> <Joel.Esponde at Honeywell.com>; linux- mtd at lists.infradead.org Objet :
> >> Re: [PATCH] mtd: spi-nor: fixed spansion quad enable
> >>
> >>>>> +	/* set SR & CR, enable quad I/O */
> >>>>>  	write_enable(nor);
> >>>>>
> >>>>>  	ret = write_sr_cr(nor, quad_en);
> >>>>>  	if (ret < 0) {
> >>>>> -		dev_err(nor->dev,
> >>>>> -			"error while writing configuration
> register\n");
> >>>>> +		dev_err(nor->dev, "error while writing SR and CR
> >>>> registers\n");
> >>>>>  		return -EINVAL;
> >>>>>  	}
> >>>>>
> >>>>> -	/* read back and check it */
> >>>>> +	ret = spi_nor_wait_till_ready(nor);
> >>>>> +	if (ret)
> >>>>> +		return ret;
> >>>>> +
> >>>
> >>> I think that this wait is required with every memories that store
> >>> Quad
> >> Enable bit in a non-volatile way.
> >>> I am using today a stripped down kernel with a rootfs based on UBIFS.
> >>> When this bit is set, UBIFS will start reading the memory 100ms after.
> >>> As this bit writing step takes up to 200ms, spansion memory is not
> >>> ready
> >> and UBIFS will fail when trying to read the content of the partition.
> >>> As UBIFS has a recovery mechanism, it will even broke the file system.
> >>>
> >>
> >> I agree with you about adding the spi_nor_wait_till_ready() call. I
> >> plan to add it in my SFDP patch for the spansion_new_quad_enable(), if
> not done yet.
> >>
> >
> > Good news !!
> > Because I think that this part of the patch is the most critical.
> > Without this, you may broke your filesystem so this is a big issue.
> >
> > Best regards,
> >
> > Joël
> >
> 
> I've recently tested with two Winbond QSPI memories, W25Q256 and
> W25M512 and now I can confirm that the issue you reported also applies to
> those memories:
> if spi_nor_wait_till_ready() is not called after write_sr_cr(), the next SPI
> command may be sent too quickly and bad data may be read.
> Indeed, this issue occured in some bare metal (boot loader) code, when I
> performed a Fast Read 1-4-4 (EBh) command right after the Write Status
> Register
> (01h) command, without polling the BUSY bit with Read Status Register (05h)
> commands.
> 
> Also the W25Q256 datasheet I have claims that the memory supports the
> Read Status Register2 (35h) however the data read from the SFDP table of
> my memory sample tells that the Quad Enable bit should be set only using
> the Write Status Register (01h) command with 2 data bytes (SR1 then SR2)
> and without using the Read Status Register 2 (35h) command:
> 
> Indeed, bits[22:20] of the 15th dword in the Basic Flash Parameter Table
> (BFPT) are 100b and not 101b!
> 
> 100b: QE is bit 1 of status register 2. It is set wia Write Status with two
>       data bytes where bit 1 of the second byte is one.
> 
> 101b: QE is bit 1 of status register 2. Status register 1 is read using Read
>       Status instruction 05h. Status register 2 is read using instruction 35h.
>       QE is set via Write Status instruction 01h with two data bytes where bit
>       1 of the second byte is one.
> 
> I didn't test whether the datasheet is right and the 35h command is actually
> supported by those Winbond memories so their bits[22:20] of the 15th
> dword in the BFPT should have been set to 101b instead of 100b.
> However I'd rather be compliant with the JESD216B specification.
> 
> So could you please send a new version of your patch adding only the
> spi_nor_wait_till_ready() call but without adding other read to the Status
> Register 2/Control Register 1 (35h)?
> 
> The 2nd procedure, which uses the 35h instruction to check whether the
> Quad Enable bit has already been set, will be implemented by
> spansion_new_quad_enable() from the SFDP patch. I've already fixed this
> patch to add the call of spi_nor_wait_till_ready().
> 
> With both your patch and mine, we will fix the issue and still be compliant
> with the SFDP specification.
> 
> Best regards,
> 
> Cyrille