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

[<Tudor.Ambarus@xxxxxxxxxxxxx>]

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?

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