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

[Vignesh Raghavendra <vigneshr@xxxxxx>]

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

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/