[PATCH v2 17/35] mtd: spi-nor: Introduce spi_nor_nonsfdp_flags_init()

[Pratyush Yadav]

On 27/07/21 07:52AM, Tudor Ambarus wrote:
> Used to initialize the NOR flags for settings that are not defined
> in the JESD216 SFDP standard, thus can not be retrieved when parsing
> SFDP. No functional change.

I am worried if the order in which these flags are set can cause some 
subtle bugs.

I can see one instance of it with SNOR_F_HAS_LOCK. 
spi_nor_late_init_params() checks for SNOR_F_HAS_LOCK and if there are 
no locking ops specified, it sets the default locking ops. This works 
fine before this patch because the flag is set before the function is 
called. But now, the flag will be set _after_ the function is called, 
and so you will never be able to set the default flags.

This is one bug I can spot but I fear some others might be hiding 
somewhere as well. SPI NOR has accumulated a lot of spaghetti code over 
the years and I certainly felt it when working on my Octal DTR series. 
It caused an address width selection bug that was not obvious at all, 
and was not even caught during the rc cycles.

I think this series does clean up that spaghetti a lot. But you need to 
be careful of such bugs. I think you should definitely let this series 
cook in next for some time so it gets some exposure and hopefully some 
testing.

> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus at microchip.com>
> ---
>  drivers/mtd/spi-nor/core.c | 88 ++++++++++++++++++++++----------------
>  1 file changed, 52 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 1f38fa8ab2fa..6a8617346764 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2687,6 +2687,56 @@ static void spi_nor_late_init_params(struct spi_nor *nor)
>  		spi_nor_init_default_locking_ops(nor);
>  }
>  
> +/**
> + * spi_nor_nonsfdp_flags_init() - Initialize NOR flags for settings that are not
> + * defined in the JESD216 SFDP standard, thus can not be retrieved when parsing
> + * SFDP.
> + * @nor:	pointer to a 'struct spi_nor'
> + */
> +static void spi_nor_nonsfdp_flags_init(struct spi_nor *nor)
> +{
> +	const struct flash_info *info = nor->info;
> +	struct device_node *np = spi_nor_get_flash_node(nor);
> +
> +	if (of_property_read_bool(np, "broken-flash-reset"))
> +		nor->flags |= SNOR_F_BROKEN_RESET;
> +
> +	if (info->flags & SPI_NOR_SWP_IS_VOLATILE)
> +		nor->flags |= SNOR_F_SWP_IS_VOLATILE;
> +
> +	if (info->flags & SPI_NOR_HAS_LOCK)
> +		nor->flags |= SNOR_F_HAS_LOCK;

As mentioned above, this would cause a bug.

> +
> +	if (info->flags & SPI_NOR_HAS_TB) {
> +		nor->flags |= SNOR_F_HAS_SR_TB;
> +		if (info->flags & SPI_NOR_TB_SR_BIT6)
> +			nor->flags |= SNOR_F_HAS_SR_TB_BIT6;
> +	}
> +
> +	if (info->flags & SPI_NOR_4BIT_BP) {
> +		nor->flags |= SNOR_F_HAS_4BIT_BP;
> +		if (info->flags & SPI_NOR_BP3_SR_BIT6)
> +			nor->flags |= SNOR_F_HAS_SR_BP3_BIT6;
> +	}
> +
> +	if (info->flags & NO_CHIP_ERASE)
> +		nor->flags |= SNOR_F_NO_OP_CHIP_ERASE;
> +
> +	if (info->flags & USE_FSR)
> +		nor->flags |= SNOR_F_USE_FSR;
> +
> +	if (info->flags & USE_CLSR)
> +		nor->flags |= SNOR_F_USE_CLSR;
> +
> +	/*
> +	 * Make sure the XSR_RDY flag is set before calling
> +	 * spi_nor_wait_till_ready(). Xilinx S3AN share MFR
> +	 * with Atmel SPI NOR.
> +	 */
> +	if (info->flags & SPI_NOR_XSR_RDY)
> +		nor->flags |=  SNOR_F_READY_XSR_RDY;
> +}
> +
>  /**
>   * spi_nor_init_params() - Initialize the flash's parameters and settings.
>   * @nor:	pointer to a 'struct spi_nor'.
> @@ -2736,6 +2786,8 @@ static int spi_nor_init_params(struct spi_nor *nor)
>  
>  	spi_nor_late_init_params(nor);
>  
> +	spi_nor_nonsfdp_flags_init(nor);
> +
>  	return 0;
>  }
>  
> @@ -3078,7 +3130,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  	const struct flash_info *info;
>  	struct device *dev = nor->dev;
>  	struct mtd_info *mtd = &nor->mtd;
> -	struct device_node *np = spi_nor_get_flash_node(nor);
>  	int ret;
>  	int i;
>  
> @@ -3115,17 +3166,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  
>  	mutex_init(&nor->lock);
>  
> -	/*
> -	 * Make sure the XSR_RDY flag is set before calling
> -	 * spi_nor_wait_till_ready(). Xilinx S3AN share MFR
> -	 * with Atmel SPI NOR.
> -	 */
> -	if (info->flags & SPI_NOR_XSR_RDY)
> -		nor->flags |=  SNOR_F_READY_XSR_RDY;
> -
> -	if (info->flags & SPI_NOR_HAS_LOCK)
> -		nor->flags |= SNOR_F_HAS_LOCK;
> -
>  	mtd->_write = spi_nor_write;
>  
>  	/* Init flash parameters based on flash_info struct and SFDP */
> @@ -3147,27 +3187,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  	mtd->_get_device = spi_nor_get_device;
>  	mtd->_put_device = spi_nor_put_device;
>  
> -	if (info->flags & USE_FSR)
> -		nor->flags |= SNOR_F_USE_FSR;
> -	if (info->flags & SPI_NOR_HAS_TB) {
> -		nor->flags |= SNOR_F_HAS_SR_TB;
> -		if (info->flags & SPI_NOR_TB_SR_BIT6)
> -			nor->flags |= SNOR_F_HAS_SR_TB_BIT6;
> -	}
> -
> -	if (info->flags & NO_CHIP_ERASE)
> -		nor->flags |= SNOR_F_NO_OP_CHIP_ERASE;
> -	if (info->flags & USE_CLSR)
> -		nor->flags |= SNOR_F_USE_CLSR;
> -	if (info->flags & SPI_NOR_SWP_IS_VOLATILE)
> -		nor->flags |= SNOR_F_SWP_IS_VOLATILE;
> -
> -	if (info->flags & SPI_NOR_4BIT_BP) {
> -		nor->flags |= SNOR_F_HAS_4BIT_BP;
> -		if (info->flags & SPI_NOR_BP3_SR_BIT6)
> -			nor->flags |= SNOR_F_HAS_SR_BP3_BIT6;
> -	}
> -
>  	if (info->flags & SPI_NOR_NO_ERASE)
>  		mtd->flags |= MTD_NO_ERASE;
>  
> @@ -3175,9 +3194,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  	nor->page_size = nor->params->page_size;
>  	mtd->writebufsize = nor->page_size;
>  
> -	if (of_property_read_bool(np, "broken-flash-reset"))
> -		nor->flags |= SNOR_F_BROKEN_RESET;
> -

As I pointed out above, I think this patch is certainly going in the 
right direction. We just need to be careful of the bugs that slip 
through.

>  	/*
>  	 * Configure the SPI memory:
>  	 * - select op codes for (Fast) Read, Page Program and Sector Erase.
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.