From: "Chia-Lin Kao (AceLan)" <[email protected]>
Not all SPI drivers support soft reset enable and soft reset commands.
This failure is expected and not critical. Thus, we avoid reporting it
to regular users to prevent potential confusion regarding power-off issues.
Signed-off-by: Chia-Lin Kao (AceLan) <[email protected]>
---
drivers/mtd/spi-nor/core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 1b0c6770c14e..7bca8ffcd756 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -3252,7 +3252,7 @@ static void spi_nor_soft_reset(struct spi_nor *nor)
ret = spi_mem_exec_op(nor->spimem, &op);
if (ret) {
- dev_warn(nor->dev, "Software reset failed: %d\n", ret);
+ dev_info(nor->dev, "Software reset failed: %d\n", ret);
return;
}
@@ -3262,7 +3262,7 @@ static void spi_nor_soft_reset(struct spi_nor *nor)
ret = spi_mem_exec_op(nor->spimem, &op);
if (ret) {
- dev_warn(nor->dev, "Software reset failed: %d\n", ret);
+ dev_info(nor->dev, "Software reset failed: %d\n", ret);
return;
}
--
2.34.1
Hi,
> Not all SPI drivers support soft reset enable and soft reset commands.
> This failure is expected and not critical.
This is not really expected. What driver is this? Let me guess, the
intel
SPI driver.
Please mention this in the commit message.
> Thus, we avoid reporting it
> to regular users to prevent potential confusion regarding power-off
> issues.
>
> Signed-off-by: Chia-Lin Kao (AceLan) <[email protected]>
> ---
> drivers/mtd/spi-nor/core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 1b0c6770c14e..7bca8ffcd756 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3252,7 +3252,7 @@ static void spi_nor_soft_reset(struct spi_nor
> *nor)
>
> ret = spi_mem_exec_op(nor->spimem, &op);
> if (ret) {
> - dev_warn(nor->dev, "Software reset failed: %d\n", ret);
> + dev_info(nor->dev, "Software reset failed: %d\n", ret);
What is the value of ret here? Ideally it should be -EOPNOTSUPP and then
don't print this message at all. Otherwise leave it at dev_warn(). Also,
please add a comment here.
-michael
On Thu, Oct 19 2023, AceLan Kao wrote:
> From: "Chia-Lin Kao (AceLan)" <[email protected]>
>
> Not all SPI drivers support soft reset enable and soft reset commands.
> This failure is expected and not critical. Thus, we avoid reporting it
> to regular users to prevent potential confusion regarding power-off issues.
No, failure to soft reset can very much be critical in certain cases.
For example, if you are operating the flash in 8D-8D-8D mode and do not
have the hard reset line connected, the bootloader (or the kernel) would
be unable to detect or operate the flash after a warm reboot.
Perhaps it makes sense to just call it when SNOR_F_BROKEN_RESET is set?
This way you do not unnecessarily call it when you do not need to, and
won't see the error message.
>
> Signed-off-by: Chia-Lin Kao (AceLan) <[email protected]>
> ---
> drivers/mtd/spi-nor/core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 1b0c6770c14e..7bca8ffcd756 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3252,7 +3252,7 @@ static void spi_nor_soft_reset(struct spi_nor *nor)
>
> ret = spi_mem_exec_op(nor->spimem, &op);
> if (ret) {
> - dev_warn(nor->dev, "Software reset failed: %d\n", ret);
> + dev_info(nor->dev, "Software reset failed: %d\n", ret);
> return;
> }
>
> @@ -3262,7 +3262,7 @@ static void spi_nor_soft_reset(struct spi_nor *nor)
>
> ret = spi_mem_exec_op(nor->spimem, &op);
> if (ret) {
> - dev_warn(nor->dev, "Software reset failed: %d\n", ret);
> + dev_info(nor->dev, "Software reset failed: %d\n", ret);
> return;
> }
--
Regards,
Pratyush Yadav
Pratyush Yadav <[email protected]> 於 2023年10月19日 週四 下午8:52寫道：
>
> On Thu, Oct 19 2023, AceLan Kao wrote:
>
> > From: "Chia-Lin Kao (AceLan)" <[email protected]>
> >
> > Not all SPI drivers support soft reset enable and soft reset commands.
> > This failure is expected and not critical. Thus, we avoid reporting it
> > to regular users to prevent potential confusion regarding power-off issues.
Hi Pratyush,
>
> No, failure to soft reset can very much be critical in certain cases.
> For example, if you are operating the flash in 8D-8D-8D mode and do not
> have the hard reset line connected, the bootloader (or the kernel) would
> be unable to detect or operate the flash after a warm reboot.
>
> Perhaps it makes sense to just call it when SNOR_F_BROKEN_RESET is set?
> This way you do not unnecessarily call it when you do not need to, and
> won't see the error message.
The issue I found was on a x86 desktop, and I can find many other same
bug reports talked about the spi-nor reset failure.
The issue is from spi-intel driver that doesn't accept the reset
command and return false when calls its supports function
spi_nor_soft_reset() -> spi_mem_exec_op() ->
spi_mem_internal_supports_op() -> ctlr->mem_ops->supports_op() ->
intel_spi_supports_mem_op() return false
And from spi_mem_exec_op(), it returns -ENOTSUPP.
So, do you think it's better that we distinguish -ENOTSUPP and other
failures in spi_nor_soft_reset() likes
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 1b0c6770c14e..76920dbc568b 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -3252,7 +3252,10 @@ static void spi_nor_soft_reset(struct spi_nor *nor)
ret = spi_mem_exec_op(nor->spimem, &op);
if (ret) {
- dev_warn(nor->dev, "Software reset failed: %d\n", ret);
+ if (ret == -ENOTSUPP)
+ dev_info(nor->dev, "Software reset enable
command doesn't support: %d\n", ret);
+ else
+ dev_warn(nor->dev, "Software reset failed: %d\n", ret);
return;
}
@@ -3262,7 +3265,10 @@ static void spi_nor_soft_reset(struct spi_nor *nor)
ret = spi_mem_exec_op(nor->spimem, &op);
if (ret) {
- dev_warn(nor->dev, "Software reset failed: %d\n", ret);
+ if (ret == -ENOTSUPP)
+ dev_info(nor->dev, "Software reset command
doesn't support: %d\n", ret);
+ else
+ dev_warn(nor->dev, "Software reset failed: %d\n", ret);
return;
}
>
> >
> > Signed-off-by: Chia-Lin Kao (AceLan) <[email protected]>
> > ---
> > drivers/mtd/spi-nor/core.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > index 1b0c6770c14e..7bca8ffcd756 100644
> > --- a/drivers/mtd/spi-nor/core.c
> > +++ b/drivers/mtd/spi-nor/core.c
> > @@ -3252,7 +3252,7 @@ static void spi_nor_soft_reset(struct spi_nor *nor)
> >
> > ret = spi_mem_exec_op(nor->spimem, &op);
> > if (ret) {
> > - dev_warn(nor->dev, "Software reset failed: %d\n", ret);
> > + dev_info(nor->dev, "Software reset failed: %d\n", ret);
> > return;
> > }
> >
> > @@ -3262,7 +3262,7 @@ static void spi_nor_soft_reset(struct spi_nor *nor)
> >
> > ret = spi_mem_exec_op(nor->spimem, &op);
> > if (ret) {
> > - dev_warn(nor->dev, "Software reset failed: %d\n", ret);
> > + dev_info(nor->dev, "Software reset failed: %d\n", ret);
> > return;
> > }
>
> --
> Regards,
> Pratyush Yadav