Hi,
On Sat, Apr 23, 2022 at 10:32:28PM +0000, Oleksandr Ocheretnyi -X (oocheret - GLOBALLOGIC INC at Cisco) wrote:
> Hello guys,
>
> recently I've faced the fatal issue "mtdblock: erase of region ... on
> ... failed"
>
> working with MTD device (Micron n25q128a13) on recent kernels with
> version
>
> 5.15.x and newer.
>
> Commit 094d3b9 ("mtd: spi-nor: Add USE_FSR flag for n25q* entries")
> adds
>
> SPINOR_OP_RDFSR opcode handling ability, however controller driver's
> side
>
> (i.e. intel-spi) cannot handle it properly in the intel_spi_hw_cycle()
> what causes
>
> a failures in the the spi_nor_fsr_ready() call what breaks some mtd
> callbacks.
>
> Are there any plans to implement this SPINOR_OP_RDFSR opcode handling
>
> on the intel controller side? Is there acceptable way to provide
> ENOTSUPP
>
> case response from controller driver side to spi-nor driver?
I think the Intel controller does support this internally but it does
not expose this to the software. I wonder if in this case we can, like
you suggest, return -ENOTSUPP and make micron_st_nor_ready() to handle
that case?
SPI-NOR folks, what you do think?
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") enables 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
Just skip subsequent processing of the SPINOR_OP_RDFSR opcode's results
because of -ENOTSUPP return value of the micron_st_nor_read_fsr()
if there is no proper handling of that opcode as it's been before
commit 094d3b9 ("mtd: spi-nor: Add USE_FSR flag for n25q* entries")
Signed-off-by: Oleksandr Ocheretnyi <[email protected]>
---
drivers/mtd/spi-nor/micron-st.c | 6 +++++-
drivers/spi/spi-intel.c | 3 ++-
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
index a96f74e0f568..507e675d81e0 100644
--- a/drivers/mtd/spi-nor/micron-st.c
+++ b/drivers/mtd/spi-nor/micron-st.c
@@ -399,8 +399,12 @@ static int micron_st_nor_ready(struct spi_nor *nor)
return sr_ready;
ret = micron_st_nor_read_fsr(nor, nor->bouncebuf);
- if (ret)
+ if (ret < 0) {
+ /* Check if read FSR is supported. If not, skip it. */
+ if (ret == -ENOTSUPP)
+ return sr_ready;
return 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)
--
2.28.0
Hi,
On Fri, Jun 10, 2022 at 12:15:48PM -0700, Oleksandr Ocheretnyi wrote:
> 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") enables 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
>
> Just skip subsequent processing of the SPINOR_OP_RDFSR opcode's results
> because of -ENOTSUPP return value of the micron_st_nor_read_fsr()
> if there is no proper handling of that opcode as it's been before
> commit 094d3b9 ("mtd: spi-nor: Add USE_FSR flag for n25q* entries")
>
> Signed-off-by: Oleksandr Ocheretnyi <[email protected]>
I sent similar patch some time ago here:
https://lore.kernel.org/linux-mtd/[email protected]/#t
but so far it has not been picked up by the maintainers. I'm fine if we
go with your patch instead, just one minor comment:
> ---
> drivers/mtd/spi-nor/micron-st.c | 6 +++++-
> drivers/spi/spi-intel.c | 3 ++-
> 2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
> index a96f74e0f568..507e675d81e0 100644
> --- a/drivers/mtd/spi-nor/micron-st.c
> +++ b/drivers/mtd/spi-nor/micron-st.c
> @@ -399,8 +399,12 @@ static int micron_st_nor_ready(struct spi_nor *nor)
> return sr_ready;
>
> ret = micron_st_nor_read_fsr(nor, nor->bouncebuf);
> - if (ret)
> + if (ret < 0) {
> + /* Check if read FSR is supported. If not, skip it. */
> + if (ret == -ENOTSUPP)
> + return sr_ready;
> return 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;
I don't think this is necessary because we already return -EOPNOTSUPP in
intel_spi_exec_mem_op() so we can just check that one in
micron_st_nor_ready().
With that changed feel free to add my,
Reviewed-by: Mika Westerberg <[email protected]>
Hi,
On Tue, Jun 14, 2022 at 05:56:54PM +0000, Oleksandr Ocheretnyi -X (oocheret - GLOBALLOGIC INC at Cisco) wrote:
> Hello Mika,
>
> in my case (I work with memory chip n25q128a13 for recent kernels) I'm
> getting return value -ENOTSUPP from spi_mem_exec_op() call in the
> micron_st_nor_read_fsr() method
> [[1]https://elixir.bootlin.com/linux/v5.19-rc2/source/drivers/spi/spi-m
> em.c#L326]. So I decided to provide the same errorcode to
> intel_spi_hw_cycle() method because older kernel versions throw the
> error there. It is fine to use -EOPNOTSUPP return value instead.
>
> I suspect we need to cover both cases to check -ENOTSUPP as well as
> -EOPNOTSUPP to let the driver work properly.
>
> if (ret == -ENOTSUPP || ret == -EOPNOTSUPP)
I think we should follow the same in the Intel driver and return
-ENOTSUPP too.
Am 15. Juni 2022 11:49:22 OEZ schrieb Mika Westerberg <[email protected]>:
>Hi,
>
>On Tue, Jun 14, 2022 at 05:56:54PM +0000, Oleksandr Ocheretnyi -X (oocheret - GLOBALLOGIC INC at Cisco) wrote:
>> Hello Mika,
>>
>> in my case (I work with memory chip n25q128a13 for recent kernels) I'm
>> getting return value -ENOTSUPP from spi_mem_exec_op() call in the
>> micron_st_nor_read_fsr() method
>> [[1]https://elixir.bootlin.com/linux/v5.19-rc2/source/drivers/spi/spi-m
>> em.c#L326]. So I decided to provide the same errorcode to
>> intel_spi_hw_cycle() method because older kernel versions throw the
>> error there. It is fine to use -EOPNOTSUPP return value instead.
>>
>> I suspect we need to cover both cases to check -ENOTSUPP as well as
>> -EOPNOTSUPP to let the driver work properly.
>>
>> if (ret == -ENOTSUPP || ret == -EOPNOTSUPP)
>
>I think we should follow the same in the Intel driver and return
>-ENOTSUPP too.
AFAIK ENOTSUPP is for nfs and shouldn't be used.
-michael
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 <[email protected]>
Signed-off-by: Oleksandr Ocheretnyi <[email protected]>
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(-)
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)
--
2.26.2.Cisco
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 <[email protected]>
On Wed, Jun 15, 2022 at 08:10:13PM +0200, Michael Walle wrote:
> Am 15. Juni 2022 11:49:22 OEZ schrieb Mika Westerberg <[email protected]>:
> >Hi,
> >
> >On Tue, Jun 14, 2022 at 05:56:54PM +0000, Oleksandr Ocheretnyi -X (oocheret - GLOBALLOGIC INC at Cisco) wrote:
> >> Hello Mika,
> >>
> >> in my case (I work with memory chip n25q128a13 for recent kernels) I'm
> >> getting return value -ENOTSUPP from spi_mem_exec_op() call in the
> >> micron_st_nor_read_fsr() method
> >> [[1]https://elixir.bootlin.com/linux/v5.19-rc2/source/drivers/spi/spi-m
> >> em.c#L326]. So I decided to provide the same errorcode to
> >> intel_spi_hw_cycle() method because older kernel versions throw the
> >> error there. It is fine to use -EOPNOTSUPP return value instead.
> >>
> >> I suspect we need to cover both cases to check -ENOTSUPP as well as
> >> -EOPNOTSUPP to let the driver work properly.
> >>
> >> if (ret == -ENOTSUPP || ret == -EOPNOTSUPP)
> >
> >I think we should follow the same in the Intel driver and return
> >-ENOTSUPP too.
>
> AFAIK ENOTSUPP is for nfs and shouldn't be used.
Yes, but that's what the SPI-NOR core is using so I think we want to be
consistent with it.
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 <[email protected]>
I don't think I signed this off.
> Signed-off-by: Oleksandr Ocheretnyi <[email protected]>
> 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 <[email protected]>
>
> 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 <[email protected]>
> > 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
> [email protected] 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 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 <[email protected]>
> 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/[email protected]
> 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
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 <[email protected]>
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/[email protected]
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;
+ }
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 <[email protected]>
> > 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
> [email protected]
> > 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
> [email protected]/ 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
> > [email protected]/ 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.
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 <[email protected]>
>>
>> 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 <[email protected]>
>> > 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
>> [email protected] 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).