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

[<Tudor.Ambarus@xxxxxxxxxxxxx>]

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:
>>>
>>> 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?

Marek, Vignesh, do you know if this assumption is correct?

> 
> 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;
>>> +	}
>>> +
>>> +	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/