| Message ID | 20220616121446.293408-1-oocheret@cisco.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | [v2] mtd: spi-nor: handle unsupported FSR opcodes properly | expand |
On Thu, Jun 16, 2022 at 05:14:45AM -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: Oleksandr Ocheretnyi <oocheret@cisco.com> > Link: https://lore.kernel.org/lkml/YmZUCIE%2FND82BlNh@lahna/ > --- > PATCH v2 updates PATCH v1 taking into account changes from > https://lore.kernel.org/linux-mtd/20220506105158.43613-1-mika.westerberg@linux.intel.com > to check -EOPNOTSUPP value from micron_st_nor_read_fsr() as well. > > drivers/mtd/spi-nor/micron-st.c | 12 ++++++++++-- > drivers/spi/spi-intel.c | 3 ++- > 2 files changed, 12 insertions(+), 3 deletions(-) > > 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; The -EOPNOTSUPP here is not needed as you change the Intel SPI driver in the below. > + } > > 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..f0313a718d1b 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 -ENOTSUPP; > } > > if (len > INTEL_SPI_FIFO_SZ) > -- > 2.27.0
On Thu, Jun 16, 2022 at 12:54:42PM +0000, Oleksandr Ocheretnyi -X (oocheret - GLOBALLOGIC INC at Cisco) wrote: > Hi, > > > 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: Oleksandr Ocheretnyi <oocheret@cisco.com> > > Link: [1]https://lore.kernel.org/lkml/YmZUCIE%2FND82BlNh@lahna/ > > --- > > PATCH v2 updates PATCH v1 taking into account changes from > > > [2]https://lore.kernel.org/linux-mtd/20220506105158.43613-1-mika.wes > terberg@linux.intel.com > > to check -EOPNOTSUPP value from micron_st_nor_read_fsr() as well. > > > > drivers/mtd/spi-nor/micron-st.c | 12 ++++++++++-- > > drivers/spi/spi-intel.c | 3 ++- > > 2 files changed, 12 insertions(+), 3 deletions(-) > > > > 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; > The -EOPNOTSUPP here is not needed as you change the Intel SPI > driver in > > the below. > > However I remember you caught situation where micron_st_nor_read_fsr() > returns -EOPNOTSUPP > (intel_spi_exec_mem_op callback returns -EOPNOTSUPP), according to your > patch > [3]https://lore.kernel.org/linux-mtd/20220506105158.43613-1-mika.wester > berg@linux.intel.com/ I've noted in description body. So I think I have > to cover both errorcodes, haven't I? I was thinking that you change the both functions in Intel SPI to return -ENOTSUPP, not just one. > Or your patch as well as my one are going submitted independently and > can be merged sequentially? No, my patch can be ignored.
Hi, On Thu, Jun 16, 2022 at 08:26:33PM +0000, Oleksandr Ocheretnyi -X (oocheret - GLOBALLOGIC INC at Cisco) wrote: > Hi Mika, > > > However I remember you caught situation where > micron_st_nor_read_fsr() > > returns -EOPNOTSUPP > > (intel_spi_exec_mem_op callback returns -EOPNOTSUPP), according to > your > > patch > > > [3]https://lore.kernel.org/linux-mtd/20220506105158.43613-1-mika.wester > > berg@linux.intel.com/ I've noted in description body. So I think I > have > > to cover both errorcodes, haven't I? > I was thinking that you change the both functions in Intel SPI to > return > > -ENOTSUPP, not just one. > > you know 'drivers/mtd/spi-nor' sources use -EOPNOTSUPP errorcode only, > however > > 'drivers/spi' modules (where intel driver is located as well as > spi-mem.c) use both errorcodes many times > > (-EOPNOTSUPP and -ENOTSUPP). Oh, indeed. I remembered that SPI-NOR core was using ENOTSUP but it was SPI-MEM instead. > So maybe it is better to use -EOPNOTSUPP for intel driver file (what > uses -EOPNOTSUPP everywhere) and > > update the spi-mem.c with -EOPNOTSUPP as return value, how do you > think? Yes, I think this is the correct approach. You need to be careful though to make sure the callers of SPI-MEM functions do not get unexpected values.
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..f0313a718d1b 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 -ENOTSUPP; } 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: Oleksandr Ocheretnyi <oocheret@cisco.com> Link: https://lore.kernel.org/lkml/YmZUCIE%2FND82BlNh@lahna/ --- PATCH v2 updates PATCH v1 taking into account changes from https://lore.kernel.org/linux-mtd/20220506105158.43613-1-mika.westerberg@linux.intel.com to check -EOPNOTSUPP value from micron_st_nor_read_fsr() as well. drivers/mtd/spi-nor/micron-st.c | 12 ++++++++++-- drivers/spi/spi-intel.c | 3 ++- 2 files changed, 12 insertions(+), 3 deletions(-)