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

[Flavio Suligoi <f.suligoi@xxxxxxx>]

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:
> >
> > 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.
> >
> > 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;
> >  }
> >
> > +/**
> > + * check_nojedec_nor_presence() - check the real presence of a non-
> JEDEC nor
> > + * @nor: pointer to a 'struct spi_nor'
> > + *
> > + * The presence of a JEDEC compliant device is implicity verified
> during the
> 
> s/implicity/implicitly
> 
> > + * JEDEC auto-detect procedure.
> > + * For a non-JEDEC device, the presence have to explicity checked,
> reading
> 
> s/explicity/explicitly
> 
> > + * one or more known registers, according to the chip features.
> > + *
> > + * Return: 0 on success, -errno otherwise.
> > + */
> > +int check_nojedec_nor_presence(struct spi_nor *nor)
> 
> all functions should start with spi_nor_*. How about naming it
> spi_nor_check_nojedec_presence()?
> 
> > +{
> > +	struct device *dev = nor->dev;
> 
> you use dev once, no need to declare a local variable for it.
> 
> > +	const struct flash_info *info = nor->info;
> 
> this will probably disappear, see below
> 
> > +	int ret = 0;
> 
> initialization not needed
> 
> > +	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?

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?

> 
> > +		/* 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;
> > +	}
> > +
> > +	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
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/