| Message ID | 1553172946-2251-1-git-send-email-f.suligoi@asem.it |
|---|---|
| State | Changes Requested |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | [1/1] mtd: spi-nor: add a presence check for non-JEDEC spi nor | expand |
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@asem.it> > --- > 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) >
Hi Tudor, > -----Original Message----- > From: Tudor.Ambarus@microchip.com <Tudor.Ambarus@microchip.com> > Sent: domenica 23 giugno 2019 06:00 > To: Flavio Suligoi <f.suligoi@asem.it>; marek.vasut@gmail.com; > dwmw2@infradead.org; computersforpeace@gmail.com; bbrezillon@kernel.org; > richard@nod.at > Cc: linux-mtd@lists.infradead.org; linux-kernel@vger.kernel.or > 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@asem.it> > > --- > > 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
Hi, Flavio, Marek, Vignesh, On 06/24/2019 10:07 AM, Flavio Suligoi wrote: > External E-Mail > > > Hi Tudor, > >> -----Original Message----- >> From: Tudor.Ambarus@microchip.com <Tudor.Ambarus@microchip.com> >> Sent: domenica 23 giugno 2019 06:00 >> To: Flavio Suligoi <f.suligoi@asem.it>; marek.vasut@gmail.com; >> dwmw2@infradead.org; computersforpeace@gmail.com; bbrezillon@kernel.org; >> richard@nod.at >> Cc: linux-mtd@lists.infradead.org; linux-kernel@vger.kernel.or >> 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@asem.it> >>> --- >>> 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 >
On 24/06/19 1:50 PM, Tudor.Ambarus@microchip.com 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@microchip.com <Tudor.Ambarus@microchip.com> >>> Sent: domenica 23 giugno 2019 06:00 >>> To: Flavio Suligoi <f.suligoi@asem.it>; marek.vasut@gmail.com; >>> dwmw2@infradead.org; computersforpeace@gmail.com; bbrezillon@kernel.org; >>> richard@nod.at >>> Cc: linux-mtd@lists.infradead.org; linux-kernel@vger.kernel.or >>> 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@asem.it> >>>> --- >>>> 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 >>
Hi Vignesh and Tudor, > -----Original Message----- > From: Vignesh Raghavendra <vigneshr@ti.com> > Sent: lunedì 24 giugno 2019 15:12 > To: Tudor.Ambarus@microchip.com; Flavio Suligoi <f.suligoi@asem.it> > Cc: linux-mtd@lists.infradead.org; linux-kernel@vger.kernel.or; > richard@nod.at; bbrezillon@kernel.org; computersforpeace@gmail.com; > dwmw2@infradead.org; marek.vasut@gmail.com > 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@microchip.com 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@microchip.com <Tudor.Ambarus@microchip.com> > >>> Sent: domenica 23 giugno 2019 06:00 > >>> To: Flavio Suligoi <f.suligoi@asem.it>; marek.vasut@gmail.com; > >>> dwmw2@infradead.org; computersforpeace@gmail.com; > bbrezillon@kernel.org; > >>> richard@nod.at > >>> Cc: linux-mtd@lists.infradead.org; linux-kernel@vger.kernel.or > >>> 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@asem.it> > >>>> --- > >>>> 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
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 + * JEDEC auto-detect procedure. + * For a non-JEDEC device, the presence have to explicity checked, reading + * 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) +{ + struct device *dev = nor->dev; + const struct flash_info *info = nor->info; + int ret = 0; + u8 val; + + /* Check presence for Everspin mr25h40 MRAM */ + if (!strcmp(info->name, "mr25h40")) { + /* 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", + info->name, val); + + /* The factory preset of the status register is 0x00 */ + 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) { + 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)
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@asem.it> --- drivers/mtd/spi-nor/spi-nor.c | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+)