diff mbox series

[v4,2/4] mtd: spi-nor-core: Adding different type of command extension in Soft Reset

Message ID 20211018062428.4201-3-jaimeliao.tw@gmail.com
State Superseded
Delegated to: Jagannadha Sutradharudu Teki
Headers show
Series Add octal DTR support for Macronix flash | expand

Commit Message

liao jaime Oct. 18, 2021, 6:24 a.m. UTC
Power-on-Reset is a method to restore flash back to 1S-1S-1S mode from 8D-8D-8D
in the begging of probe.

Command extension type is not standardized across flash vendors in DTR mode.

For suiting different vendor flash devices, adding a flag to seperate types for
soft reset on boot.

Signed-off-by: JaimeLiao <jaimeliao.tw@gmail.com>
---
 drivers/mtd/spi/Kconfig        | 7 +++++++
 drivers/mtd/spi/spi-nor-core.c | 7 ++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

Comments

Jagan Teki Oct. 25, 2021, 7 a.m. UTC | #1
On Mon, Oct 18, 2021 at 11:54 AM JaimeLiao <jaimeliao.tw@gmail.com> wrote:
>
> Power-on-Reset is a method to restore flash back to 1S-1S-1S mode from 8D-8D-8D
> in the begging of probe.
>
> Command extension type is not standardized across flash vendors in DTR mode.
>
> For suiting different vendor flash devices, adding a flag to seperate types for
> soft reset on boot.
>
> Signed-off-by: JaimeLiao <jaimeliao.tw@gmail.com>
> ---
>  drivers/mtd/spi/Kconfig        | 7 +++++++
>  drivers/mtd/spi/spi-nor-core.c | 7 ++++++-
>  2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/spi/Kconfig b/drivers/mtd/spi/Kconfig
> index 67599b32c9..8304d6c973 100644
> --- a/drivers/mtd/spi/Kconfig
> +++ b/drivers/mtd/spi/Kconfig
> @@ -97,6 +97,13 @@ config SPI_FLASH_SMART_HWCAPS
>          can support a type of operation in a much more refined way compared
>          to using flags like SPI_RX_DUAL, SPI_TX_QUAD, etc.
>
> +config SPI_NOR_BOOT_SOFT_RESET_EXT_INVERT
> +       bool "Command extension type is INVERT for Software Reset on boot"
> +       default n
> +       help
> +        Because of SFDP information can not be get before boot.
> +        So define command extension type is INVERT when Software Reset on boot only.
> +
>  config SPI_FLASH_SOFT_RESET
>         bool "Software Reset support for SPI NOR flashes"
>         default n
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index fdaca0a0d3..87a7de7d60 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -3661,7 +3661,12 @@ static int spi_nor_soft_reset(struct spi_nor *nor)
>         enum spi_nor_cmd_ext ext;
>
>         ext = nor->cmd_ext_type;
> -       nor->cmd_ext_type = SPI_NOR_EXT_REPEAT;
> +       if (nor->cmd_ext_type == SPI_NOR_EXT_NONE) {
> +               nor->cmd_ext_type = SPI_NOR_EXT_REPEAT;
> +#if CONFIG_IS_ENABLED(SPI_NOR_BOOT_SOFT_RESET_EXT_INVERT)
> +               nor->cmd_ext_type = SPI_NOR_EXT_INVERT;
> +#endif /* SPI_NOR_BOOT_SOFT_RESET_EXT_INVERT */

I think we can get the SPI_NOR_EXT_INVERT by parsing bfpt, can you try
it that way instead of new macro?

Jagan.
Pratyush Yadav Oct. 25, 2021, 7:42 p.m. UTC | #2
On 25/10/21 12:30PM, Jagan Teki wrote:
> On Mon, Oct 18, 2021 at 11:54 AM JaimeLiao <jaimeliao.tw@gmail.com> wrote:
> >
> > Power-on-Reset is a method to restore flash back to 1S-1S-1S mode from 8D-8D-8D
> > in the begging of probe.
> >
> > Command extension type is not standardized across flash vendors in DTR mode.
> >
> > For suiting different vendor flash devices, adding a flag to seperate types for
> > soft reset on boot.
> >
> > Signed-off-by: JaimeLiao <jaimeliao.tw@gmail.com>
> > ---
> >  drivers/mtd/spi/Kconfig        | 7 +++++++
> >  drivers/mtd/spi/spi-nor-core.c | 7 ++++++-
> >  2 files changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mtd/spi/Kconfig b/drivers/mtd/spi/Kconfig
> > index 67599b32c9..8304d6c973 100644
> > --- a/drivers/mtd/spi/Kconfig
> > +++ b/drivers/mtd/spi/Kconfig
> > @@ -97,6 +97,13 @@ config SPI_FLASH_SMART_HWCAPS
> >          can support a type of operation in a much more refined way compared
> >          to using flags like SPI_RX_DUAL, SPI_TX_QUAD, etc.
> >
> > +config SPI_NOR_BOOT_SOFT_RESET_EXT_INVERT
> > +       bool "Command extension type is INVERT for Software Reset on boot"
> > +       default n
> > +       help
> > +        Because of SFDP information can not be get before boot.
> > +        So define command extension type is INVERT when Software Reset on boot only.
> > +
> >  config SPI_FLASH_SOFT_RESET
> >         bool "Software Reset support for SPI NOR flashes"
> >         default n
> > diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> > index fdaca0a0d3..87a7de7d60 100644
> > --- a/drivers/mtd/spi/spi-nor-core.c
> > +++ b/drivers/mtd/spi/spi-nor-core.c
> > @@ -3661,7 +3661,12 @@ static int spi_nor_soft_reset(struct spi_nor *nor)
> >         enum spi_nor_cmd_ext ext;
> >
> >         ext = nor->cmd_ext_type;
> > -       nor->cmd_ext_type = SPI_NOR_EXT_REPEAT;
> > +       if (nor->cmd_ext_type == SPI_NOR_EXT_NONE) {
> > +               nor->cmd_ext_type = SPI_NOR_EXT_REPEAT;
> > +#if CONFIG_IS_ENABLED(SPI_NOR_BOOT_SOFT_RESET_EXT_INVERT)
> > +               nor->cmd_ext_type = SPI_NOR_EXT_INVERT;
> > +#endif /* SPI_NOR_BOOT_SOFT_RESET_EXT_INVERT */
> 
> I think we can get the SPI_NOR_EXT_INVERT by parsing bfpt, can you try
> it that way instead of new macro?

The problem is that for OSPI boot mode the ROM can often leave the flash 
in 8D-8D-8D mode. So when U-Boot first starts executing the flash is 
already in 8D-8D-8D mode and there is no easy and reliable way to detect 
this mode. So we must blindly perform a soft reset before probing the 
flash and reading SFDP so that it is reset back to a usable state.

This is why we can't detect the extension via BFPT since the flash is in 
an unknown state. This is not the case when resetting before booting the 
OS since at that point we have fully probed the flash. So I think this 
config must only be used for the reset at boot time. For later resets we 
should indeed use the extension provided by BFPT.

This is a kinda hacky but I can't come up with a better alternative.

Jamie,

Below diff is what I have been suggesting you in my earlier replies. 
Note that this is just a quick and dirty implementation, I have not 
tested this at all. That is up to you to do. Please also test soft reset 
_after_ the probe is finished so we know that it works correctly when 
using data from BFPT as well.

-- 8< --
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index 4388a08a90..b0f22e0ce5 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -3616,10 +3616,6 @@ static int spi_nor_soft_reset(struct spi_nor *nor)
 {
 	struct spi_mem_op op;
 	int ret;
-	enum spi_nor_cmd_ext ext;
-
-	ext = nor->cmd_ext_type;
-	nor->cmd_ext_type = SPI_NOR_EXT_REPEAT;
 
 	op = (struct spi_mem_op)SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_SRSTEN, 0),
 			SPI_MEM_OP_NO_DUMMY,
@@ -3629,7 +3625,7 @@ static int spi_nor_soft_reset(struct spi_nor *nor)
 	ret = spi_mem_exec_op(nor->spi, &op);
 	if (ret) {
 		dev_warn(nor->dev, "Software reset enable failed: %d\n", ret);
-		goto out;
+		return ret;
 	}
 
 	op = (struct spi_mem_op)SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_SRST, 0),
@@ -3640,7 +3636,7 @@ static int spi_nor_soft_reset(struct spi_nor *nor)
 	ret = spi_mem_exec_op(nor->spi, &op);
 	if (ret) {
 		dev_warn(nor->dev, "Software reset failed: %d\n", ret);
-		goto out;
+		return ret;
 	}
 
 	/*
@@ -3650,9 +3646,7 @@ static int spi_nor_soft_reset(struct spi_nor *nor)
 	 */
 	udelay(SPI_NOR_SRST_SLEEP_LEN);
 
-out:
-	nor->cmd_ext_type = ext;
-	return ret;
+	return 0;
 }
 #endif /* CONFIG_SPI_FLASH_SOFT_RESET */
 
@@ -3698,6 +3692,44 @@ void spi_nor_set_fixups(struct spi_nor *nor)
 #endif
 }
 
+/*
+ * When the flash is handed to us in a stateful mode like 8D-8D-8D, it is
+ * difficult to detect the mode the flash is in. One option is to read SFDP in
+ * all modes and see which one gives the correct "SFDP" signature, but not all
+ * flashes support SFDP in 8D-8D-8D mode.
+ *
+ * Further, even if you detect the mode of the flash via SFDP, you still have
+ * the problem of actually reading the ID. The Read ID command is not
+ * standardized across flash vendors. Flashes can have different dummy cycles
+ * needed for reading the ID. Some flashes even expect a 4-byte dummy address
+ * with the Read ID command. All this information cannot be obtained from the
+ * SFDP table.
+ *
+ * So, perform a Software Reset sequence before reading the ID and initializing
+ * the flash. A Soft Reset will bring back the flash in its default protocol
+ * mode assuming no non-volatile configuration was set. This will let us detect
+ * the flash even if ROM hands it to us in Octal DTR mode.
+ *
+ * To accommodate cases where there is more than one flash on a board, and only
+ * one of them needs a soft reset, failure to reset is not made fatal, and we
+ * still try to read ID if possible.
+ */
+static void spi_nor_soft_reset_on_boot(struct spi_nor *nor)
+{
+	enum spi_nor_cmd_ext ext;
+
+	ext = nor->cmd_ext_type;
+
+	if (CONFIG_IS_ENABLED(SPI_NOR_BOOT_SOFT_RESET_EXT_INVERT))
+		nor->cmd_ext_type = SPI_NOR_EXT_INVERT;
+	else
+		nor->cmd_ext_type = SPI_NOR_EXT_REPEAT;
+
+	spi_nor_soft_reset(nor);
+
+	nor->cmd_ext_type = ext;
+}
+
 int spi_nor_scan(struct spi_nor *nor)
 {
 	struct spi_nor_flash_parameter params;
@@ -3722,32 +3754,8 @@ int spi_nor_scan(struct spi_nor *nor)
 
 	nor->setup = spi_nor_default_setup;
 
-#ifdef CONFIG_SPI_FLASH_SOFT_RESET_ON_BOOT
-	/*
-	 * When the flash is handed to us in a stateful mode like 8D-8D-8D, it
-	 * is difficult to detect the mode the flash is in. One option is to
-	 * read SFDP in all modes and see which one gives the correct "SFDP"
-	 * signature, but not all flashes support SFDP in 8D-8D-8D mode.
-	 *
-	 * Further, even if you detect the mode of the flash via SFDP, you
-	 * still have the problem of actually reading the ID. The Read ID
-	 * command is not standardized across flash vendors. Flashes can have
-	 * different dummy cycles needed for reading the ID. Some flashes even
-	 * expect a 4-byte dummy address with the Read ID command. All this
-	 * information cannot be obtained from the SFDP table.
-	 *
-	 * So, perform a Software Reset sequence before reading the ID and
-	 * initializing the flash. A Soft Reset will bring back the flash in
-	 * its default protocol mode assuming no non-volatile configuration was
-	 * set. This will let us detect the flash even if ROM hands it to us in
-	 * Octal DTR mode.
-	 *
-	 * To accommodate cases where there is more than one flash on a board,
-	 * and only one of them needs a soft reset, failure to reset is not
-	 * made fatal, and we still try to read ID if possible.
-	 */
-	spi_nor_soft_reset(nor);
-#endif /* CONFIG_SPI_FLASH_SOFT_RESET_ON_BOOT */
+	if (CONFIG_IS_ENABLED(SPI_FLASH_SOFT_RESET_ON_BOOT))
+		spi_nor_soft_reset_on_boot(nor);
 
 	info = spi_nor_read_id(nor);
 	if (IS_ERR_OR_NULL(info))
liao jaime Oct. 26, 2021, 6:42 a.m. UTC | #3
For validating soft_reset after probe, I have test it on my side.
It is working with command extension type which got from BFPT.

Pratyush Yadav <p.yadav@ti.com> 於 2021年10月26日 週二 上午3:42寫道:

> On 25/10/21 12:30PM, Jagan Teki wrote:
> > On Mon, Oct 18, 2021 at 11:54 AM JaimeLiao <jaimeliao.tw@gmail.com>
> wrote:
> > >
> > > Power-on-Reset is a method to restore flash back to 1S-1S-1S mode from
> 8D-8D-8D
> > > in the begging of probe.
> > >
> > > Command extension type is not standardized across flash vendors in DTR
> mode.
> > >
> > > For suiting different vendor flash devices, adding a flag to seperate
> types for
> > > soft reset on boot.
> > >
> > > Signed-off-by: JaimeLiao <jaimeliao.tw@gmail.com>
> > > ---
> > >  drivers/mtd/spi/Kconfig        | 7 +++++++
> > >  drivers/mtd/spi/spi-nor-core.c | 7 ++++++-
> > >  2 files changed, 13 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/mtd/spi/Kconfig b/drivers/mtd/spi/Kconfig
> > > index 67599b32c9..8304d6c973 100644
> > > --- a/drivers/mtd/spi/Kconfig
> > > +++ b/drivers/mtd/spi/Kconfig
> > > @@ -97,6 +97,13 @@ config SPI_FLASH_SMART_HWCAPS
> > >          can support a type of operation in a much more refined way
> compared
> > >          to using flags like SPI_RX_DUAL, SPI_TX_QUAD, etc.
> > >
> > > +config SPI_NOR_BOOT_SOFT_RESET_EXT_INVERT
> > > +       bool "Command extension type is INVERT for Software Reset on
> boot"
> > > +       default n
> > > +       help
> > > +        Because of SFDP information can not be get before boot.
> > > +        So define command extension type is INVERT when Software
> Reset on boot only.
> > > +
> > >  config SPI_FLASH_SOFT_RESET
> > >         bool "Software Reset support for SPI NOR flashes"
> > >         default n
> > > diff --git a/drivers/mtd/spi/spi-nor-core.c
> b/drivers/mtd/spi/spi-nor-core.c
> > > index fdaca0a0d3..87a7de7d60 100644
> > > --- a/drivers/mtd/spi/spi-nor-core.c
> > > +++ b/drivers/mtd/spi/spi-nor-core.c
> > > @@ -3661,7 +3661,12 @@ static int spi_nor_soft_reset(struct spi_nor
> *nor)
> > >         enum spi_nor_cmd_ext ext;
> > >
> > >         ext = nor->cmd_ext_type;
> > > -       nor->cmd_ext_type = SPI_NOR_EXT_REPEAT;
> > > +       if (nor->cmd_ext_type == SPI_NOR_EXT_NONE) {
> > > +               nor->cmd_ext_type = SPI_NOR_EXT_REPEAT;
> > > +#if CONFIG_IS_ENABLED(SPI_NOR_BOOT_SOFT_RESET_EXT_INVERT)
> > > +               nor->cmd_ext_type = SPI_NOR_EXT_INVERT;
> > > +#endif /* SPI_NOR_BOOT_SOFT_RESET_EXT_INVERT */
> >
> > I think we can get the SPI_NOR_EXT_INVERT by parsing bfpt, can you try
> > it that way instead of new macro?
>
> The problem is that for OSPI boot mode the ROM can often leave the flash
> in 8D-8D-8D mode. So when U-Boot first starts executing the flash is
> already in 8D-8D-8D mode and there is no easy and reliable way to detect
> this mode. So we must blindly perform a soft reset before probing the
> flash and reading SFDP so that it is reset back to a usable state.
>
> This is why we can't detect the extension via BFPT since the flash is in
> an unknown state. This is not the case when resetting before booting the
> OS since at that point we have fully probed the flash. So I think this
> config must only be used for the reset at boot time. For later resets we
> should indeed use the extension provided by BFPT.
>
> This is a kinda hacky but I can't come up with a better alternative.
>
> Jamie,
>
> Below diff is what I have been suggesting you in my earlier replies.
> Note that this is just a quick and dirty implementation, I have not
> tested this at all. That is up to you to do. Please also test soft reset
> _after_ the probe is finished so we know that it works correctly when
> using data from BFPT as well.
>
> -- 8< --
> diff --git a/drivers/mtd/spi/spi-nor-core.c
> b/drivers/mtd/spi/spi-nor-core.c
> index 4388a08a90..b0f22e0ce5 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -3616,10 +3616,6 @@ static int spi_nor_soft_reset(struct spi_nor *nor)
>  {
>         struct spi_mem_op op;
>         int ret;
> -       enum spi_nor_cmd_ext ext;
> -
> -       ext = nor->cmd_ext_type;
> -       nor->cmd_ext_type = SPI_NOR_EXT_REPEAT;
>
>         op = (struct
> spi_mem_op)SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_SRSTEN, 0),
>                         SPI_MEM_OP_NO_DUMMY,
> @@ -3629,7 +3625,7 @@ static int spi_nor_soft_reset(struct spi_nor *nor)
>         ret = spi_mem_exec_op(nor->spi, &op);
>         if (ret) {
>                 dev_warn(nor->dev, "Software reset enable failed: %d\n",
> ret);
> -               goto out;
> +               return ret;
>         }
>
>         op = (struct spi_mem_op)SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_SRST,
> 0),
> @@ -3640,7 +3636,7 @@ static int spi_nor_soft_reset(struct spi_nor *nor)
>         ret = spi_mem_exec_op(nor->spi, &op);
>         if (ret) {
>                 dev_warn(nor->dev, "Software reset failed: %d\n", ret);
> -               goto out;
> +               return ret;
>         }
>
>         /*
> @@ -3650,9 +3646,7 @@ static int spi_nor_soft_reset(struct spi_nor *nor)
>          */
>         udelay(SPI_NOR_SRST_SLEEP_LEN);
>
> -out:
> -       nor->cmd_ext_type = ext;
> -       return ret;
> +       return 0;
>  }
>  #endif /* CONFIG_SPI_FLASH_SOFT_RESET */
>
> @@ -3698,6 +3692,44 @@ void spi_nor_set_fixups(struct spi_nor *nor)
>  #endif
>  }
>
> +/*
> + * When the flash is handed to us in a stateful mode like 8D-8D-8D, it is
> + * difficult to detect the mode the flash is in. One option is to read
> SFDP in
> + * all modes and see which one gives the correct "SFDP" signature, but
> not all
> + * flashes support SFDP in 8D-8D-8D mode.
> + *
> + * Further, even if you detect the mode of the flash via SFDP, you still
> have
> + * the problem of actually reading the ID. The Read ID command is not
> + * standardized across flash vendors. Flashes can have different dummy
> cycles
> + * needed for reading the ID. Some flashes even expect a 4-byte dummy
> address
> + * with the Read ID command. All this information cannot be obtained from
> the
> + * SFDP table.
> + *
> + * So, perform a Software Reset sequence before reading the ID and
> initializing
> + * the flash. A Soft Reset will bring back the flash in its default
> protocol
> + * mode assuming no non-volatile configuration was set. This will let us
> detect
> + * the flash even if ROM hands it to us in Octal DTR mode.
> + *
> + * To accommodate cases where there is more than one flash on a board,
> and only
> + * one of them needs a soft reset, failure to reset is not made fatal,
> and we
> + * still try to read ID if possible.
> + */
> +static void spi_nor_soft_reset_on_boot(struct spi_nor *nor)
> +{
> +       enum spi_nor_cmd_ext ext;
> +
> +       ext = nor->cmd_ext_type;
> +
> +       if (CONFIG_IS_ENABLED(SPI_NOR_BOOT_SOFT_RESET_EXT_INVERT))
> +               nor->cmd_ext_type = SPI_NOR_EXT_INVERT;
> +       else
> +               nor->cmd_ext_type = SPI_NOR_EXT_REPEAT;
> +
> +       spi_nor_soft_reset(nor);
> +
> +       nor->cmd_ext_type = ext;
> +}
> +
>  int spi_nor_scan(struct spi_nor *nor)
>  {
>         struct spi_nor_flash_parameter params;
> @@ -3722,32 +3754,8 @@ int spi_nor_scan(struct spi_nor *nor)
>
>         nor->setup = spi_nor_default_setup;
>
> -#ifdef CONFIG_SPI_FLASH_SOFT_RESET_ON_BOOT
> -       /*
> -        * When the flash is handed to us in a stateful mode like
> 8D-8D-8D, it
> -        * is difficult to detect the mode the flash is in. One option is
> to
> -        * read SFDP in all modes and see which one gives the correct
> "SFDP"
> -        * signature, but not all flashes support SFDP in 8D-8D-8D mode.
> -        *
> -        * Further, even if you detect the mode of the flash via SFDP, you
> -        * still have the problem of actually reading the ID. The Read ID
> -        * command is not standardized across flash vendors. Flashes can
> have
> -        * different dummy cycles needed for reading the ID. Some flashes
> even
> -        * expect a 4-byte dummy address with the Read ID command. All this
> -        * information cannot be obtained from the SFDP table.
> -        *
> -        * So, perform a Software Reset sequence before reading the ID and
> -        * initializing the flash. A Soft Reset will bring back the flash
> in
> -        * its default protocol mode assuming no non-volatile
> configuration was
> -        * set. This will let us detect the flash even if ROM hands it to
> us in
> -        * Octal DTR mode.
> -        *
> -        * To accommodate cases where there is more than one flash on a
> board,
> -        * and only one of them needs a soft reset, failure to reset is not
> -        * made fatal, and we still try to read ID if possible.
> -        */
> -       spi_nor_soft_reset(nor);
> -#endif /* CONFIG_SPI_FLASH_SOFT_RESET_ON_BOOT */
> +       if (CONFIG_IS_ENABLED(SPI_FLASH_SOFT_RESET_ON_BOOT))
> +               spi_nor_soft_reset_on_boot(nor);
>
>         info = spi_nor_read_id(nor);
>         if (IS_ERR_OR_NULL(info))
>
> --
> Regards,
> Pratyush Yadav
> Texas Instruments Inc.
>
liao jaime Oct. 26, 2021, 7:03 a.m. UTC | #4
>
> On 25/10/21 12:30PM, Jagan Teki wrote:
> > On Mon, Oct 18, 2021 at 11:54 AM JaimeLiao <jaimeliao.tw@gmail.com> wrote:
> > >
> > > Power-on-Reset is a method to restore flash back to 1S-1S-1S mode from 8D-8D-8D
> > > in the begging of probe.
> > >
> > > Command extension type is not standardized across flash vendors in DTR mode.
> > >
> > > For suiting different vendor flash devices, adding a flag to seperate types for
> > > soft reset on boot.
> > >
> > > Signed-off-by: JaimeLiao <jaimeliao.tw@gmail.com>
> > > ---
> > >  drivers/mtd/spi/Kconfig        | 7 +++++++
> > >  drivers/mtd/spi/spi-nor-core.c | 7 ++++++-
> > >  2 files changed, 13 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/mtd/spi/Kconfig b/drivers/mtd/spi/Kconfig
> > > index 67599b32c9..8304d6c973 100644
> > > --- a/drivers/mtd/spi/Kconfig
> > > +++ b/drivers/mtd/spi/Kconfig
> > > @@ -97,6 +97,13 @@ config SPI_FLASH_SMART_HWCAPS
> > >          can support a type of operation in a much more refined way compared
> > >          to using flags like SPI_RX_DUAL, SPI_TX_QUAD, etc.
> > >
> > > +config SPI_NOR_BOOT_SOFT_RESET_EXT_INVERT
> > > +       bool "Command extension type is INVERT for Software Reset on boot"
> > > +       default n
> > > +       help
> > > +        Because of SFDP information can not be get before boot.
> > > +        So define command extension type is INVERT when Software Reset on boot only.
> > > +
> > >  config SPI_FLASH_SOFT_RESET
> > >         bool "Software Reset support for SPI NOR flashes"
> > >         default n
> > > diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> > > index fdaca0a0d3..87a7de7d60 100644
> > > --- a/drivers/mtd/spi/spi-nor-core.c
> > > +++ b/drivers/mtd/spi/spi-nor-core.c
> > > @@ -3661,7 +3661,12 @@ static int spi_nor_soft_reset(struct spi_nor *nor)
> > >         enum spi_nor_cmd_ext ext;
> > >
> > >         ext = nor->cmd_ext_type;
> > > -       nor->cmd_ext_type = SPI_NOR_EXT_REPEAT;
> > > +       if (nor->cmd_ext_type == SPI_NOR_EXT_NONE) {
> > > +               nor->cmd_ext_type = SPI_NOR_EXT_REPEAT;
> > > +#if CONFIG_IS_ENABLED(SPI_NOR_BOOT_SOFT_RESET_EXT_INVERT)
> > > +               nor->cmd_ext_type = SPI_NOR_EXT_INVERT;
> > > +#endif /* SPI_NOR_BOOT_SOFT_RESET_EXT_INVERT */
> >
> > I think we can get the SPI_NOR_EXT_INVERT by parsing bfpt, can you try
> > it that way instead of new macro?
>
> The problem is that for OSPI boot mode the ROM can often leave the flash
> in 8D-8D-8D mode. So when U-Boot first starts executing the flash is
> already in 8D-8D-8D mode and there is no easy and reliable way to detect
> this mode. So we must blindly perform a soft reset before probing the
> flash and reading SFDP so that it is reset back to a usable state.
>
> This is why we can't detect the extension via BFPT since the flash is in
> an unknown state. This is not the case when resetting before booting the
> OS since at that point we have fully probed the flash. So I think this
> config must only be used for the reset at boot time. For later resets we
> should indeed use the extension provided by BFPT.
>
> This is a kinda hacky but I can't come up with a better alternative.
>
> Jamie,
>
> Below diff is what I have been suggesting you in my earlier replies.
> Note that this is just a quick and dirty implementation, I have not
> tested this at all. That is up to you to do. Please also test soft reset
> _after_ the probe is finished so we know that it works correctly when
> using data from BFPT as well.
>
> -- 8< --
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index 4388a08a90..b0f22e0ce5 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -3616,10 +3616,6 @@ static int spi_nor_soft_reset(struct spi_nor *nor)
>  {
>         struct spi_mem_op op;
>         int ret;
> -       enum spi_nor_cmd_ext ext;
> -
> -       ext = nor->cmd_ext_type;
> -       nor->cmd_ext_type = SPI_NOR_EXT_REPEAT;
>
>         op = (struct spi_mem_op)SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_SRSTEN, 0),
>                         SPI_MEM_OP_NO_DUMMY,
> @@ -3629,7 +3625,7 @@ static int spi_nor_soft_reset(struct spi_nor *nor)
>         ret = spi_mem_exec_op(nor->spi, &op);
>         if (ret) {
>                 dev_warn(nor->dev, "Software reset enable failed: %d\n", ret);
> -               goto out;
> +               return ret;
>         }
>
>         op = (struct spi_mem_op)SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_SRST, 0),
> @@ -3640,7 +3636,7 @@ static int spi_nor_soft_reset(struct spi_nor *nor)
>         ret = spi_mem_exec_op(nor->spi, &op);
>         if (ret) {
>                 dev_warn(nor->dev, "Software reset failed: %d\n", ret);
> -               goto out;
> +               return ret;
>         }
>
>         /*
> @@ -3650,9 +3646,7 @@ static int spi_nor_soft_reset(struct spi_nor *nor)
>          */
>         udelay(SPI_NOR_SRST_SLEEP_LEN);
>
> -out:
> -       nor->cmd_ext_type = ext;
> -       return ret;
> +       return 0;
>  }
>  #endif /* CONFIG_SPI_FLASH_SOFT_RESET */
>
> @@ -3698,6 +3692,44 @@ void spi_nor_set_fixups(struct spi_nor *nor)
>  #endif
>  }
>
> +/*
> + * When the flash is handed to us in a stateful mode like 8D-8D-8D, it is
> + * difficult to detect the mode the flash is in. One option is to read SFDP in
> + * all modes and see which one gives the correct "SFDP" signature, but not all
> + * flashes support SFDP in 8D-8D-8D mode.
> + *
> + * Further, even if you detect the mode of the flash via SFDP, you still have
> + * the problem of actually reading the ID. The Read ID command is not
> + * standardized across flash vendors. Flashes can have different dummy cycles
> + * needed for reading the ID. Some flashes even expect a 4-byte dummy address
> + * with the Read ID command. All this information cannot be obtained from the
> + * SFDP table.
> + *
> + * So, perform a Software Reset sequence before reading the ID and initializing
> + * the flash. A Soft Reset will bring back the flash in its default protocol
> + * mode assuming no non-volatile configuration was set. This will let us detect
> + * the flash even if ROM hands it to us in Octal DTR mode.
> + *
> + * To accommodate cases where there is more than one flash on a board, and only
> + * one of them needs a soft reset, failure to reset is not made fatal, and we
> + * still try to read ID if possible.
> + */
> +static void spi_nor_soft_reset_on_boot(struct spi_nor *nor)
> +{
> +       enum spi_nor_cmd_ext ext;
> +
> +       ext = nor->cmd_ext_type;
> +
> +       if (CONFIG_IS_ENABLED(SPI_NOR_BOOT_SOFT_RESET_EXT_INVERT))
> +               nor->cmd_ext_type = SPI_NOR_EXT_INVERT;
> +       else
> +               nor->cmd_ext_type = SPI_NOR_EXT_REPEAT;
> +
> +       spi_nor_soft_reset(nor);
> +
> +       nor->cmd_ext_type = ext;
> +}
> +
>  int spi_nor_scan(struct spi_nor *nor)
>  {
>         struct spi_nor_flash_parameter params;
> @@ -3722,32 +3754,8 @@ int spi_nor_scan(struct spi_nor *nor)
>
>         nor->setup = spi_nor_default_setup;
>
> -#ifdef CONFIG_SPI_FLASH_SOFT_RESET_ON_BOOT
> -       /*
> -        * When the flash is handed to us in a stateful mode like 8D-8D-8D, it
> -        * is difficult to detect the mode the flash is in. One option is to
> -        * read SFDP in all modes and see which one gives the correct "SFDP"
> -        * signature, but not all flashes support SFDP in 8D-8D-8D mode.
> -        *
> -        * Further, even if you detect the mode of the flash via SFDP, you
> -        * still have the problem of actually reading the ID. The Read ID
> -        * command is not standardized across flash vendors. Flashes can have
> -        * different dummy cycles needed for reading the ID. Some flashes even
> -        * expect a 4-byte dummy address with the Read ID command. All this
> -        * information cannot be obtained from the SFDP table.
> -        *
> -        * So, perform a Software Reset sequence before reading the ID and
> -        * initializing the flash. A Soft Reset will bring back the flash in
> -        * its default protocol mode assuming no non-volatile configuration was
> -        * set. This will let us detect the flash even if ROM hands it to us in
> -        * Octal DTR mode.
> -        *
> -        * To accommodate cases where there is more than one flash on a board,
> -        * and only one of them needs a soft reset, failure to reset is not
> -        * made fatal, and we still try to read ID if possible.
> -        */
> -       spi_nor_soft_reset(nor);
> -#endif /* CONFIG_SPI_FLASH_SOFT_RESET_ON_BOOT */
> +       if (CONFIG_IS_ENABLED(SPI_FLASH_SOFT_RESET_ON_BOOT))
> +               spi_nor_soft_reset_on_boot(nor);
>
>         info = spi_nor_read_id(nor);
>         if (IS_ERR_OR_NULL(info))
>
> --

For validating soft_reset after probe, I have test it on my side.
It is working with command extension type which got from BFPT.

> Regards,
> Pratyush Yadav
> Texas Instruments Inc.
diff mbox series

Patch

diff --git a/drivers/mtd/spi/Kconfig b/drivers/mtd/spi/Kconfig
index 67599b32c9..8304d6c973 100644
--- a/drivers/mtd/spi/Kconfig
+++ b/drivers/mtd/spi/Kconfig
@@ -97,6 +97,13 @@  config SPI_FLASH_SMART_HWCAPS
 	 can support a type of operation in a much more refined way compared
 	 to using flags like SPI_RX_DUAL, SPI_TX_QUAD, etc.
 
+config SPI_NOR_BOOT_SOFT_RESET_EXT_INVERT
+	bool "Command extension type is INVERT for Software Reset on boot"
+	default n
+	help
+	 Because of SFDP information can not be get before boot.
+	 So define command extension type is INVERT when Software Reset on boot only.
+
 config SPI_FLASH_SOFT_RESET
 	bool "Software Reset support for SPI NOR flashes"
 	default n
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index fdaca0a0d3..87a7de7d60 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -3661,7 +3661,12 @@  static int spi_nor_soft_reset(struct spi_nor *nor)
 	enum spi_nor_cmd_ext ext;
 
 	ext = nor->cmd_ext_type;
-	nor->cmd_ext_type = SPI_NOR_EXT_REPEAT;
+	if (nor->cmd_ext_type == SPI_NOR_EXT_NONE) {
+		nor->cmd_ext_type = SPI_NOR_EXT_REPEAT;
+#if CONFIG_IS_ENABLED(SPI_NOR_BOOT_SOFT_RESET_EXT_INVERT)
+		nor->cmd_ext_type = SPI_NOR_EXT_INVERT;
+#endif /* SPI_NOR_BOOT_SOFT_RESET_EXT_INVERT */
+	}
 
 	op = (struct spi_mem_op)SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_SRSTEN, 0),
 			SPI_MEM_OP_NO_DUMMY,