| Message ID | 20220615191153.3017939-1-oocheret@cisco.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] mtd: spi-nor: handle unsupported FSR opcodes properly | expand |
On Wed, Jun 15, 2022 at 12:11:53PM -0700, Oleksandr Ocheretnyi wrote: > Originally commit 094d3b9 ("mtd: spi-nor: Add USE_FSR flag for n25q* > entries") and following one 8f93826 ("mtd: spi-nor: micron-st: convert > USE_FSR to a manufacturer flag") enabled SPINOR_OP_RDFSR opcode handling > ability, however some controller drivers still cannot handle it properly > in the micron_st_nor_ready() call what breaks some mtd callbacks with > next error logs: Acked-by: Mark Brown <broonie@kernel.org>
On Wed, Jun 15, 2022 at 12:11:53PM -0700, Oleksandr Ocheretnyi wrote: > Originally commit 094d3b9 ("mtd: spi-nor: Add USE_FSR flag for n25q* > entries") and following one 8f93826 ("mtd: spi-nor: micron-st: convert > USE_FSR to a manufacturer flag") enabled SPINOR_OP_RDFSR opcode handling > ability, however some controller drivers still cannot handle it properly > in the micron_st_nor_ready() call what breaks some mtd callbacks with > next error logs: > > mtdblock: erase of region [address1, size1] on "BIOS" failed > mtdblock: erase of region [address2, size2] on "BIOS" failed > > The Intel SPI controller does not support low level operations, like > reading the flag status register (FSR). It only exposes a set of high > level operations for software to use. For this reason check the return > value of micron_st_nor_read_fsr() and if the operation was not > supported, use the status register value only. This allows the chip to > work even when attached to Intel SPI controller (there are such systems > out there). > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> I don't think I signed this off. > Signed-off-by: Oleksandr Ocheretnyi <oocheret@cisco.com> > Link: https://lore.kernel.org/lkml/YmZUCIE%2FND82BlNh@lahna/ > --- What changed between v1 and v2? And did you take into consideration the comments I gave?
Hi, On Thu, Jun 16, 2022 at 07:40:18AM +0000, Oleksandr Ocheretnyi -X (oocheret - GLOBALLOGIC INC at Cisco) wrote: > Hi Mika, > > > Originally commit 094d3b9 ("mtd: spi-nor: Add USE_FSR flag for > n25q* > > entries") and following one 8f93826 ("mtd: spi-nor: micron-st: > convert > > USE_FSR to a manufacturer flag") enabled SPINOR_OP_RDFSR opcode > handling > > ability, however some controller drivers still cannot handle it > properly > > in the micron_st_nor_ready() call what breaks some mtd callbacks > with > > next error logs: > > > > mtdblock: erase of region [address1, size1] on "BIOS" failed > > mtdblock: erase of region [address2, size2] on "BIOS" failed > > > > The Intel SPI controller does not support low level operations, > like > > reading the flag status register (FSR). It only exposes a set of > high > > level operations for software to use. For this reason check the > return > > value of micron_st_nor_read_fsr() and if the operation was not > > supported, use the status register value only. This allows the > chip to > > work even when attached to Intel SPI controller (there are such > systems > > out there). > > > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > I don't think I signed this off. > > I thought if I take your case (-EOPNOTSUPP) and update it with > (-ENOTSUPP) I need to keep > > your Sighed-off-by: note as well. That's not how it typically works. People will give their tag explicitly and then you can add those. > > Signed-off-by: Oleksandr Ocheretnyi <oocheret@cisco.com> > > Link: [1]https://lore.kernel.org/lkml/YmZUCIE%2FND82BlNh@lahna/ > > --- > > What changed between v1 and v2? > > I updated v1 patch taking into account your changes > [2]https://lore.kernel.org/linux-mtd/20220506105158.43613-1-mika.wester > berg@linux.intel.com to check -EOPNOTSUPP case as well. After I > combined both patches I've got v2. Please put that information after the '---' in the patch. > And did you take into consideration the comments I gave? > > If you say about keeping -ENOTSUPP as intel driver errorcode - I took > it however doubted to use it here because of note about nfs above. > There is no problem to restore previous variant with -ENOTSUPP in intel > driver errorcode. Well we would need to get some feedback from SPI-NOR maintainers. I would personally keep using ENOTSUPP to be consistent with the rest of the code in SPI-NOR code (or convert it to use EOPNOTSUPP everywhere) but it is not up to me ;-) For Intel driver it is fine to use either (whetever the decision of SPI-NOR maintainers' is).
On 6/16/22 13:35, Mika Westerberg wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Hi, > > On Thu, Jun 16, 2022 at 07:40:18AM +0000, Oleksandr Ocheretnyi -X (oocheret - GLOBALLOGIC INC at Cisco) wrote: >> Hi Mika, >> >> > Originally commit 094d3b9 ("mtd: spi-nor: Add USE_FSR flag for >> n25q* >> > entries") and following one 8f93826 ("mtd: spi-nor: micron-st: >> convert >> > USE_FSR to a manufacturer flag") enabled SPINOR_OP_RDFSR opcode >> handling >> > ability, however some controller drivers still cannot handle it >> properly >> > in the micron_st_nor_ready() call what breaks some mtd callbacks >> with >> > next error logs: >> > >> > mtdblock: erase of region [address1, size1] on "BIOS" failed >> > mtdblock: erase of region [address2, size2] on "BIOS" failed >> > >> > The Intel SPI controller does not support low level operations, >> like >> > reading the flag status register (FSR). It only exposes a set of >> high >> > level operations for software to use. For this reason check the >> return >> > value of micron_st_nor_read_fsr() and if the operation was not >> > supported, use the status register value only. This allows the >> chip to >> > work even when attached to Intel SPI controller (there are such >> systems >> > out there). >> > >> >> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> >> >> I don't think I signed this off. >> >> I thought if I take your case (-EOPNOTSUPP) and update it with >> (-ENOTSUPP) I need to keep >> >> your Sighed-off-by: note as well. > > That's not how it typically works. People will give their tag explicitly > and then you can add those. > >> > Signed-off-by: Oleksandr Ocheretnyi <oocheret@cisco.com> >> > Link: [1]https://lore.kernel.org/lkml/YmZUCIE%2FND82BlNh@lahna/ >> > --- >> >> What changed between v1 and v2? >> >> I updated v1 patch taking into account your changes >> [2]https://lore.kernel.org/linux-mtd/20220506105158.43613-1-mika.wester >> berg@linux.intel.com to check -EOPNOTSUPP case as well. After I >> combined both patches I've got v2. > > Please put that information after the '---' in the patch. > >> And did you take into consideration the comments I gave? >> >> If you say about keeping -ENOTSUPP as intel driver errorcode - I took >> it however doubted to use it here because of note about nfs above. >> There is no problem to restore previous variant with -ENOTSUPP in intel >> driver errorcode. > > Well we would need to get some feedback from SPI-NOR maintainers. I > would personally keep using ENOTSUPP to be consistent with the rest of > the code in SPI-NOR code (or convert it to use EOPNOTSUPP everywhere) SPI NOR does not return -ENOTSUPP, but SPI MEM does. Let's use EOPNOTSUPP in SPI NOR and verify if we can do a patch to s/ENOTSUPP/EOPNOTSUPP in SPI MEM. > but it is not up to me ;-) > > For Intel driver it is fine to use either (whetever the decision of > SPI-NOR maintainers' is).
diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c index a96f74e0f568..fd52e8feea44 100644 --- a/drivers/mtd/spi-nor/micron-st.c +++ b/drivers/mtd/spi-nor/micron-st.c @@ -399,8 +399,16 @@ static int micron_st_nor_ready(struct spi_nor *nor) return sr_ready; ret = micron_st_nor_read_fsr(nor, nor->bouncebuf); - if (ret) - return ret; + if (ret < 0) { + /* + * Some controllers, such as Intel SPI, do not support low + * level operations such as reading the flag status + * register. They only expose small amount of high level + * operations to the software. If this is the case we use + * only the status register value. + */ + return (ret == -ENOTSUPP || ret == -EOPNOTSUPP) ? sr_ready : ret; + } if (nor->bouncebuf[0] & (FSR_E_ERR | FSR_P_ERR)) { if (nor->bouncebuf[0] & FSR_E_ERR) diff --git a/drivers/spi/spi-intel.c b/drivers/spi/spi-intel.c index 50f42983b950..2659c8337937 100644 --- a/drivers/spi/spi-intel.c +++ b/drivers/spi/spi-intel.c @@ -352,7 +352,8 @@ static int intel_spi_hw_cycle(struct intel_spi *ispi, u8 opcode, size_t len) val |= HSFSTS_CTL_FCYCLE_RDSR; break; default: - return -EINVAL; + dev_dbg(ispi->dev, "%#x not supported\n", opcode); + return -EOPNOTSUPP; } if (len > INTEL_SPI_FIFO_SZ)
Originally commit 094d3b9 ("mtd: spi-nor: Add USE_FSR flag for n25q* entries") and following one 8f93826 ("mtd: spi-nor: micron-st: convert USE_FSR to a manufacturer flag") enabled SPINOR_OP_RDFSR opcode handling ability, however some controller drivers still cannot handle it properly in the micron_st_nor_ready() call what breaks some mtd callbacks with next error logs: mtdblock: erase of region [address1, size1] on "BIOS" failed mtdblock: erase of region [address2, size2] on "BIOS" failed The Intel SPI controller does not support low level operations, like reading the flag status register (FSR). It only exposes a set of high level operations for software to use. For this reason check the return value of micron_st_nor_read_fsr() and if the operation was not supported, use the status register value only. This allows the chip to work even when attached to Intel SPI controller (there are such systems out there). Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> Signed-off-by: Oleksandr Ocheretnyi <oocheret@cisco.com> Link: https://lore.kernel.org/lkml/YmZUCIE%2FND82BlNh@lahna/ --- drivers/mtd/spi-nor/micron-st.c | 12 ++++++++++-- drivers/spi/spi-intel.c | 3 ++- 2 files changed, 12 insertions(+), 3 deletions(-)