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

[Pratyush Yadav]

On 13/09/21 01:42PM, JaimeLiao 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.

Typo: begging -> beginning

> 
> Command extension type is not standardized across flash vendors in DTR mode.
> 
> For suiting different vendor flash devices, adding a flag to seperate types if
> nor->cmd_ext_type didn't configure from SFDP.

No, the code does not do what the commit message says. The code only 
sets the extension for soft reset, which is then reset back to the 
default at the end of the function. Then we read from SFDP and do the 
usual initialization.

> 
> Signed-off-by: JaimeLiao <jaimeliao.tw at gmail.com>
> ---
>  drivers/mtd/spi/Kconfig        | 6 ++++++
>  drivers/mtd/spi/spi-nor-core.c | 7 ++++++-
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi/Kconfig b/drivers/mtd/spi/Kconfig
> index 67599b32c9..d850480401 100644
> --- a/drivers/mtd/spi/Kconfig
> +++ b/drivers/mtd/spi/Kconfig
> @@ -97,6 +97,12 @@ 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_CMD_EXT_INVERT

Let's only have this extension for the soft reset on boot, and then let 
the usual initialization process discover it via SFDP. This will make 
the code a little less complicated IMO.

So I suggest calling it SPI_NOR_BOOT_SOFT_RESET_EXT.

> +	bool "Command extension type is INVERT for SPI NOR flashed"
> +	default n
> +	help
> +	 Define command extension type is INVERT.

And explain here that this only sets the extension for the soft reset on 
boot since we can't discover it at that point. Then we drop the 
information and rediscover it as normal via SFDP or flash fixup hooks.

> +
>  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 b8dda02aa7..4bcd58d839 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) {

Use (nor->cmd_ext_type == SPI_NOR_EXT_NONE). Also add a comment 
explaining here why you do this check.

> +		nor->cmd_ext_type = SPI_NOR_EXT_REPEAT;
> +#ifdef CONFIG_SPI_NOR_CMD_EXT_INVERT
> +		nor->cmd_ext_type = SPI_NOR_EXT_INVERT;
> +#endif

Avoid using #ifdef. You can replace it with 

  if (CONFIG_IS_ENABLED(SPI_NOR_CMD_EXT_INVERT))
      nor->cmd_ext_type = SPI_NOR_EXT_INVERT;
  else
      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,
> -- 
> 2.17.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.