RE: [PATCH 1/1] mtd: spi-nor: add a presence check for non-JEDEC spi nor

[Flavio Suligoi <f.suligoi@xxxxxxx>]

Hi Vignesh and Tudor,

> -----Original Message-----
> From: Vignesh Raghavendra <vigneshr@xxxxxx>
> Sent: lunedì 24 giugno 2019 15:12
> To: Tudor.Ambarus@xxxxxxxxxxxxx; Flavio Suligoi <f.suligoi@xxxxxxx>
> Cc: linux-mtd@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxx;
> richard@xxxxxx; bbrezillon@xxxxxxxxxx; computersforpeace@xxxxxxxxx;
> dwmw2@xxxxxxxxxxxxx; marek.vasut@xxxxxxxxx
> Subject: Re: [PATCH 1/1] mtd: spi-nor: add a presence check for non-JEDEC
> spi nor
> 
> 
> 
> On 24/06/19 1:50 PM, Tudor.Ambarus@xxxxxxxxxxxxx wrote:
> > Hi, Flavio, Marek, Vignesh,
> >
> > On 06/24/2019 10:07 AM, Flavio Suligoi wrote:
> >> External E-Mail
> >>
> >>
> >> Hi Tudor,
> >>
> >>> -----Original Message-----
> >>> From: Tudor.Ambarus@xxxxxxxxxxxxx <Tudor.Ambarus@xxxxxxxxxxxxx>
> >>> Sent: domenica 23 giugno 2019 06:00
> >>> To: Flavio Suligoi <f.suligoi@xxxxxxx>; marek.vasut@xxxxxxxxx;
> >>> dwmw2@xxxxxxxxxxxxx; computersforpeace@xxxxxxxxx;
> bbrezillon@xxxxxxxxxx;
> >>> richard@xxxxxx
> >>> Cc: linux-mtd@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxx
> >>> Subject: Re: [PATCH 1/1] mtd: spi-nor: add a presence check for non-
> JEDEC
> >>> spi nor
> >>>
> >>> Hi, Flavio,
> >>>
> >>> On 03/21/2019 02:55 PM, Flavio Suligoi wrote:
> >>>> External E-Mail
> >>>>
> >>>>
> >>>> This patch fixes the following kernel error message:
> >>>>
> >>>> "flash operation timed out"
> >>>>
> >>>> that occurs when a non-JEDEC spi-nor, declared in a Device Tree, but
> not
> >>>> physically present on the board, is involved in a write operation,
> as:
> >>>>
> 
> DT node should have status = "disabled" if flash is not physically
> present on the board.
> 
> >>>> cat datafile > /dev/mtd0
> >>>>
> >>>> In some cases, with enough quantity of data, the writing hangs for
> >>> minutes.
> >>>>
> >>>> This situation can happen, for example, in some embedded systems,
> when
> >>>> a non-JEDEC spi-nor is mounted using a removable add-on board. So is
> >>>> important to find a method to check a non-JEDEC device presence
> before
> >>>> using it.
> >>>>
> 
> Can't bootloader detect presence on removable board using some means
> like GPIO and then fixup DT to enable/disable flash node?
> 
> >>>> The presence of a JEDEC compliant device is implicitly verified
> during
> >>> the
> >>>> JEDEC auto-detect procedure.
> >>>> But for a non-JEDEC device, the presence must be explicitly checked,
> >>>> reading one or more known registers, according to the chip features.
> >>>>
> >>>> Without any check, if the spi-nor is declared in a Device Tree but
> not
> >>>> physically present on the board, the driver (i.e. m25p80) is loaded
> >>> anyway
> >>>> and the correspondent mtd device is also created.
> >>>> In this way any read operation on this device returns 0xff (or 0x00,
> >>>> depending on the driver and the hardware used) and any write
> operation
> >>>> hangs until the flash operation timeout occurs, with the "flash
> >>> operation
> >>>> timed out" error message.
> >>>>
> >>>> This patch adds the non-JEDEC spi-nor presence check before
> initializing
> >>>> the device.
> >>>>
> >>>> Note: currently this presence check supports only the Everspin
> mr25h40,
> >>>>       reading its status register.
> >>>>
> >>>>       The support for other non-JEDEC devices has to be added.
> >>>>
> >>>> Signed-off-by: Flavio Suligoi <f.suligoi@xxxxxxx>
> >>>> ---
> >>>>  drivers/mtd/spi-nor/spi-nor.c | 43
> >>> +++++++++++++++++++++++++++++++++++++++++++
> >>>>  1 file changed, 43 insertions(+)
> >>>>
> >>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-
> >>> nor.c
> >>>> index fae1474..d2cb710 100644
> >>>> --- a/drivers/mtd/spi-nor/spi-nor.c
> >>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
> >>>> @@ -3981,6 +3981,42 @@ static const struct flash_info
> >>> *spi_nor_match_id(const char *name)
> >>>>  	return NULL;
> >>>>  }
> >>>>
> [...]
> >>>
> >>>> +	u8 val;
> >>>> +
> >>>> +	/* Check presence for Everspin mr25h40 MRAM */
> >>>> +	if (!strcmp(info->name, "mr25h40")) {
> >>>
> >>> Couldn't we make this check for all non-jedec flashes? Aren't we safe
> to
> >>> assume
> >>> that all the flashes have a Status Register that contains a WEL bit
> which
> >>> come
> >>> with value zero by default?
> >
> > Marek, Vignesh, do you know if this assumption is correct?
> >
> 
> Seems to be fair assumption IMO, otherwise write operation to such
> non-jedec flashes is broken with current SPI NOR/m25p80 driver. But, I
> have a different concern even if above assumption is true. See below...
> 
> >>
> >> I don't know if all the non-JEDEC flashes have a Status Register with
> >> the same configuration and with the same default values. So for this
> >> reason I thought to add a specific test for each single flash.
> >> In this way, every person who work with a specific flash can add
> >> a proper flash presence test.
> >> What do you think about this?
> >
> > I'm not very happy with having a check for each specific flash, the code
> will go
> > nuts in size. Let's do a generic method, like the one I proposed above.
> If Marek
> > or Vignesh can not confirm if the assumption is correct, then we will
> have to
> > check the datasheets for each non-jedec flash declared in spi-nor to
> validate
> > the assumption :(.
> >
> > ta
> >
> >>
> >>>
> >>>> +		/* Read the status register */
> >>>> +		ret = nor->read_reg(nor, SPINOR_OP_RDSR, &val, 1);
> >>>> +		if (ret)
> >>>> +			return ret;
> >>>> +
> >>>> +		dev_dbg(dev, "%s - status register = 0x%2.2x\n",
> >>>
> >>> %hhx?
> >>>
> >>>> +			info->name, val);
> >>>> +
> >>>> +		/* The factory preset of the status register is 0x00 */
> >>>
> >>> if we generalize this, the comment will become irrelevant. How about
> >>> something
> >>> like: "Check if flash is connected."
> >>>
> >>>> +		if (val == 0xff)
> >>>> +			return -ENODEV;
> 
> Hmm, This depends on how board is wired right? If MISO line is pulled up
> to logic 1 when there is no flash connected then read would result in
> 0xff. But if the pin is floating read would return undefined value
> (maybe 0x0) and code would wrongly assume "flash is connected"?

Yes, you are right Vignesh!
I assumed that the MISO is always internally pulled-up in the master
SPI controller (as in my case), but it may not always be true!
The MISO is normally driven by the slave, so in some SPI controllers 
this signal could be leave floating.
Ok, I have to think another way to check the SPI NOR presence,
I agree that this method is not foolproof.

> 
> Also, what if there is a different SPI slave connected to that CS on
> removable board that responds to SPINOR_OP_RDSR command?
> 
> So, I am not fully convinced that this presence check logic is foolproof
> 
> Regards
> Vignesh
> 
> >>>> +	}
> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>>  int spi_nor_scan(struct spi_nor *nor, const char *name,
> >>>>  		 const struct spi_nor_hwcaps *hwcaps)
> >>>>  {
> >>>> @@ -4158,6 +4194,13 @@ int spi_nor_scan(struct spi_nor *nor, const
> char
> >>> *name,
> >>>>  			return ret;
> >>>>  	}
> >>>>
> >>>> +	/* Check non-JEDEC nor presence */
> >>>> +	if (!info->id_len) {
> >>>
> >>> if (name && !info->id_len)?
> >>>
> >>> How about moving the entire if block to
> >>> https://elixir.bootlin.com/linux/v5.2-rc6/source/drivers/mtd/spi-
> nor/spi-
> >>> nor.c#L4037?
> >>>
> >>> Cheers,
> >>> ta
> >>>
> >>>> +		ret = check_nojedec_nor_presence(nor);
> >>>> +		if (ret)
> >>>> +			return ret;
> >>>> +	}
> >>>> +
> >>>>  	/* Send all the required SPI flash commands to initialize
> device */
> >>>>  	ret = spi_nor_init(nor);
> >>>>  	if (ret)
> >>>>
> >>
> >> Thanks,
> >> Flavio
> >>
> 
> --
> Regards
> Vignesh

Thanks to all of you,
Flavio
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/