[1/2] mtd: spi-nor: core: move Spansion SR ready codes out of core

From: Yaliang Wang <Yaliang.Wang@windriver.com>

To move manufacture specific checking SR ready codes out of core, set up
an extra method "sr_ready" in struct spi_nor_fixups, by doing this, the
manufactures can fill in and initialize the method it in their own
files, and leaves the core a relatively clean place.

Spansion's checking flash chip ready codes are moved out by applying
forementioned "sr_ready" method. There will be several following-up
patches for moving "xsr" and "fsr" related codes out of core.

Signed-off-by: Yaliang Wang <Yaliang.Wang@windriver.com>
---
 drivers/mtd/spi-nor/core.c     | 84 ++++++++++------------------------
 drivers/mtd/spi-nor/core.h     |  8 ++++
 drivers/mtd/spi-nor/spansion.c | 70 ++++++++++++++++++++++++++++
 include/linux/mtd/spi-nor.h    |  1 -
 4 files changed, 101 insertions(+), 62 deletions(-)

On 01/03/21 10:28PM, yaliang.wang@windriver.com wrote:
> From: Yaliang Wang <Yaliang.Wang@windriver.com>
> 
> To move manufacture specific checking SR ready codes out of core, set up
> an extra method "sr_ready" in struct spi_nor_fixups, by doing this, the
> manufactures can fill in and initialize the method it in their own
> files, and leaves the core a relatively clean place.
> 
> Spansion's checking flash chip ready codes are moved out by applying
> forementioned "sr_ready" method. There will be several following-up
> patches for moving "xsr" and "fsr" related codes out of core.
> 
> Signed-off-by: Yaliang Wang <Yaliang.Wang@windriver.com>
> ---
>  drivers/mtd/spi-nor/core.c     | 84 ++++++++++------------------------
>  drivers/mtd/spi-nor/core.h     |  8 ++++
>  drivers/mtd/spi-nor/spansion.c | 70 ++++++++++++++++++++++++++++
>  include/linux/mtd/spi-nor.h    |  1 -
>  4 files changed, 101 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 20df44b753da..74729eb89947 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -157,7 +157,7 @@ static int spi_nor_spimem_exec_op(struct spi_nor *nor, struct spi_mem_op *op)
>  	return spi_mem_exec_op(nor->spimem, op);
>  }
>  
> -static int spi_nor_controller_ops_read_reg(struct spi_nor *nor, u8 opcode,
> +int spi_nor_controller_ops_read_reg(struct spi_nor *nor, u8 opcode,
>  					   u8 *buf, size_t len)
>  {
>  	if (spi_nor_protocol_is_dtr(nor->reg_proto))
> @@ -166,7 +166,7 @@ static int spi_nor_controller_ops_read_reg(struct spi_nor *nor, u8 opcode,
>  	return nor->controller_ops->read_reg(nor, opcode, buf, len);
>  }
>  
> -static int spi_nor_controller_ops_write_reg(struct spi_nor *nor, u8 opcode,
> +int spi_nor_controller_ops_write_reg(struct spi_nor *nor, u8 opcode,
>  					    const u8 *buf, size_t len)

The second line is not aligned with the open parenthesis in the above 3 
functions. You can pass "--strict" to checkpatch.pl to check for this.

>  {
>  	if (spi_nor_protocol_is_dtr(nor->reg_proto))
> @@ -175,7 +175,7 @@ static int spi_nor_controller_ops_write_reg(struct spi_nor *nor, u8 opcode,
>  	return nor->controller_ops->write_reg(nor, opcode, buf, len);
>  }
>  
> -static int spi_nor_controller_ops_erase(struct spi_nor *nor, loff_t offs)
> +int spi_nor_controller_ops_erase(struct spi_nor *nor, loff_t offs)
>  {
>  	if (spi_nor_protocol_is_dtr(nor->write_proto))
>  		return -EOPNOTSUPP;
> @@ -649,32 +649,7 @@ static int spi_nor_xsr_ready(struct spi_nor *nor)
>  	return !!(nor->bouncebuf[0] & XSR_RDY);
>  }
>  
> -/**
> - * spi_nor_clear_sr() - Clear the Status Register.
> - * @nor:	pointer to 'struct spi_nor'.
> - */
> -static void spi_nor_clear_sr(struct spi_nor *nor)
> -{
> -	int ret;
> -
> -	if (nor->spimem) {
> -		struct spi_mem_op op =
> -			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_CLSR, 0),
> -				   SPI_MEM_OP_NO_ADDR,
> -				   SPI_MEM_OP_NO_DUMMY,
> -				   SPI_MEM_OP_NO_DATA);
> -
> -		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> -
> -		ret = spi_mem_exec_op(nor->spimem, &op);
> -	} else {
> -		ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_CLSR,
> -						       NULL, 0);
> -	}
> -
> -	if (ret)
> -		dev_dbg(nor->dev, "error %d clearing SR\n", ret);
> -}
> +static const struct flash_info *spi_nor_read_id(struct spi_nor *nor);

This forward declaration is not needed. More on this below.

>  
>  /**
>   * spi_nor_sr_ready() - Query the Status Register to see if the flash is ready
> @@ -690,28 +665,6 @@ static int spi_nor_sr_ready(struct spi_nor *nor)
>  	if (ret)
>  		return ret;
>  
> -	if (nor->flags & SNOR_F_USE_CLSR &&
> -	    nor->bouncebuf[0] & (SR_E_ERR | SR_P_ERR)) {
> -		if (nor->bouncebuf[0] & SR_E_ERR)
> -			dev_err(nor->dev, "Erase Error occurred\n");
> -		else
> -			dev_err(nor->dev, "Programming Error occurred\n");
> -
> -		spi_nor_clear_sr(nor);
> -
> -		/*
> -		 * WEL bit remains set to one when an erase or page program
> -		 * error occurs. Issue a Write Disable command to protect
> -		 * against inadvertent writes that can possibly corrupt the
> -		 * contents of the memory.
> -		 */
> -		ret = spi_nor_write_disable(nor);
> -		if (ret)
> -			return ret;
> -
> -		return -EIO;
> -	}
> -
>  	return !(nor->bouncebuf[0] & SR_WIP);
>  }
>  
> @@ -792,18 +745,27 @@ static int spi_nor_fsr_ready(struct spi_nor *nor)
>   */
>  static int spi_nor_ready(struct spi_nor *nor)
>  {
> -	int sr, fsr;
> +	const struct flash_info *info = nor->info ? nor->info : spi_nor_read_id(nor);
> +
> +	if (IS_ERR_OR_NULL(info))
> +		return -ENOENT;

nor->info can't be NULL or invalid. The probe would fail in that case 
and this function would never get executed. No need to check anything 
before using nor->info.


> +
> +	/*Chip specific method for querying whether the flash is ready*/

Nitpick: Please add a space after the '*' when starting a comment and 
before it when ending a comment. Same for all other comments.

> +	if (info->fixups && info->fixups->sr_ready)
> +		return info->fixups->sr_ready(nor);
> +
> +	/*Manufacture specific method for querying whether the flash is ready*/

s/Manufacture/Manufacturer/

> +	if (nor->manufacturer && nor->manufacturer->fixups &&
> +	    nor->manufacturer->fixups->sr_ready)
> +		return nor->manufacturer->fixups->sr_ready(nor);

I don't think nor->info->fixups is the correct place for this hook. 
Those should be about fixing up incorrect or missing information about 
the flash. It should instead be placed in nor->params.

This eliminates the need for the call to 
nor->manufacturer->fixups->sr_ready. Now you can simply call 
nor->params->sr_ready(). The fixup hooks will take care of populating it 
with the correct function based on the flash or manufacturer.

>  
>  	if (nor->flags & SNOR_F_READY_XSR_RDY)
> -		sr = spi_nor_xsr_ready(nor);
> -	else
> -		sr = spi_nor_sr_ready(nor);
> -	if (sr < 0)
> -		return sr;
> -	fsr = nor->flags & SNOR_F_USE_FSR ? spi_nor_fsr_ready(nor) : 1;
> -	if (fsr < 0)
> -		return fsr;
> -	return sr && fsr;
> +		return spi_nor_xsr_ready(nor);
> +	if (nor->flags & SNOR_F_USE_FSR)
> +		return spi_nor_fsr_ready(nor);
> +
> +	/*Common method for querying whether the flash is ready*/
> +	return spi_nor_sr_ready(nor);

You are no longer taking into account the FSR status correctly. If a 
flash specifies the USE_FSR flag, then spi_nor_fsr_ready() directly 
returns and spi_nor_sr_ready() is not consulted at all. But if the flash 
then populates the sr_ready() hook then it directly the result of 
sr_ready() and spi_nor_fsr_ready() is not consulted at all.

>  }
>  
>  /**
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index d631ee299de3..36356f27ee92 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -253,6 +253,7 @@ struct spi_nor_flash_parameter {
>   *             parameters that could not be extracted by other means (i.e.
>   *             when information provided by the SFDP/flash_info tables are
>   *             incomplete or wrong).
> + * @sr_ready: special method for querying whether a flash chip is ready.

There is nothing "special" about this. Simply saying "queries whether a 
flash chip is ready or not" should be fine.

>   *
>   * Those hooks can be used to tweak the SPI NOR configuration when the SFDP
>   * table is broken or not available.
> @@ -264,6 +265,7 @@ struct spi_nor_fixups {
>  			 const struct sfdp_bfpt *bfpt,
>  			 struct spi_nor_flash_parameter *params);
>  	void (*post_sfdp)(struct spi_nor *nor);
> +	int (*sr_ready)(struct spi_nor *nor);
>  };
>  
>  struct flash_info {
> @@ -471,6 +473,12 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
>  			     const struct sfdp_bfpt *bfpt,
>  			     struct spi_nor_flash_parameter *params);
>  
> +int spi_nor_controller_ops_read_reg(struct spi_nor *nor, u8 opcode,
> +					   u8 *buf, size_t len);
> +int spi_nor_controller_ops_write_reg(struct spi_nor *nor, u8 opcode,
> +					    const u8 *buf, size_t len);
> +int spi_nor_controller_ops_erase(struct spi_nor *nor, loff_t offs);
> +
>  static struct spi_nor __maybe_unused *mtd_to_spi_nor(struct mtd_info *mtd)
>  {
>  	return mtd->priv;
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index b0c5521c1e27..67619b64c148 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -18,6 +18,7 @@
>  #define SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_EN	0x3
>  #define SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS	0
>  #define SPINOR_OP_CYPRESS_RD_FAST		0xee
> +#define SPINOR_OP_SPANSION_CLSR			0x30
>  
>  /**
>   * spi_nor_cypress_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
> @@ -175,6 +176,74 @@ static struct spi_nor_fixups s28hs512t_fixups = {
>  	.post_bfpt = s28hs512t_post_bfpt_fixup,
>  };
>  
> +/**
> + * spansion_clear_sr() - Clear the Status Register.
> + * @nor:	pointer to 'struct spi_nor'.
> + */
> +static void spansion_clear_sr(struct spi_nor *nor)
> +{
> +	int ret;
> +
> +	if (nor->spimem) {
> +		struct spi_mem_op op =
> +			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_SPANSION_CLSR, 0),
> +				   SPI_MEM_OP_NO_ADDR,
> +				   SPI_MEM_OP_NO_DUMMY,
> +				   SPI_MEM_OP_NO_DATA);
> +
> +		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> +
> +		ret = spi_mem_exec_op(nor->spimem, &op);
> +	} else {
> +		ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_SPANSION_CLSR,
> +						       NULL, 0);
> +	}
> +
> +	if (ret)
> +		dev_dbg(nor->dev, "error %d clearing SR\n", ret);
> +}

Ok.

> +
> +/**
> + * spansion_sr_ready() - Spansion specific method for querying the flash to
> + * see if it is ready for new commands.
> + * @nor:	pointer to 'struct spi_nor'.
> + *
> + * Return: 1 if ready, 0 if not ready, -errno on errors.
> + */
> +static int spansion_sr_ready(struct spi_nor *nor)
> +{
> +	u8 *sr = nor->bouncebuf;
> +	int ret;
> +
> +	ret = spi_nor_read_sr(nor, sr);
> +	if (ret)
> +		return ret;
> +
> +	if (nor->flags & SNOR_F_USE_CLSR &&
> +	    nor->bouncebuf[0] & (SR_E_ERR | SR_P_ERR)) {
> +		if (nor->bouncebuf[0] & SR_E_ERR)
> +			dev_err(nor->dev, "Erase Error occurred\n");
> +		else
> +			dev_err(nor->dev, "Programming Error occurred\n");
> +
> +		spansion_clear_sr(nor);
> +
> +		/*
> +		 * WEL bit remains set to one when an erase or page program
> +		 * error occurs. Issue a Write Disable command to protect
> +		 * against inadvertent writes that can possibly corrupt the
> +		 * contents of the memory.
> +		 */
> +		ret = spi_nor_write_disable(nor);
> +		if (ret)
> +			return ret;
> +
> +		return -EIO;
> +	}
> +
> +	return !(sr[0] & SR_WIP);
> +}
> +

Ok.

>  static int
>  s25fs_s_post_bfpt_fixups(struct spi_nor *nor,
>  			 const struct sfdp_parameter_header *bfpt_header,
> @@ -291,6 +360,7 @@ static void spansion_post_sfdp_fixups(struct spi_nor *nor)
>  
>  static const struct spi_nor_fixups spansion_fixups = {
>  	.post_sfdp = spansion_post_sfdp_fixups,
> +	.sr_ready = spansion_sr_ready,
>  };
>  
>  const struct spi_nor_manufacturer spi_nor_spansion = {
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index d13958de6d8a..43bd66204fdf 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -100,7 +100,6 @@
>  
>  /* Used for Spansion flashes only. */
>  #define SPINOR_OP_BRWR		0x17	/* Bank register write */
> -#define SPINOR_OP_CLSR		0x30	/* Clear status register 1 */
>  
>  /* Used for Micron flashes only. */
>  #define SPINOR_OP_RD_EVCR      0x65    /* Read EVCR register */

Hi,

On 3/1/21 7:58 PM, yaliang.wang@windriver.com wrote:
> +/**
> + * spansion_clear_sr() - Clear the Status Register.
> + * @nor:	pointer to 'struct spi_nor'.
> + */
> +static void spansion_clear_sr(struct spi_nor *nor)
> +{
> +	int ret;
> +
> +	if (nor->spimem) {
> +		struct spi_mem_op op =
> +			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_SPANSION_CLSR, 0),
> +				   SPI_MEM_OP_NO_ADDR,
> +				   SPI_MEM_OP_NO_DUMMY,
> +				   SPI_MEM_OP_NO_DATA);
> +
> +		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> +
> +		ret = spi_mem_exec_op(nor->spimem, &op);
> +	} else {
> +		ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_SPANSION_CLSR,
> +						       NULL, 0);
> +	}
> +
> +	if (ret)
> +		dev_dbg(nor->dev, "error %d clearing SR\n", ret);
> +}
> +

No, instead introduce a helper in core.c which can handle both
spi_mem_exec_op() and spi_nor_controller_ops_write_reg().

Regards
Vignesh

On 3/1/21 9:08 PM, Pratyush Yadav wrote:
>> +     if (nor->manufacturer && nor->manufacturer->fixups &&
>> +         nor->manufacturer->fixups->sr_ready)
>> +             return nor->manufacturer->fixups->sr_ready(nor);
> I don't think nor->info->fixups is the correct place for this hook.
> Those should be about fixing up incorrect or missing information about
> the flash. It should instead be placed in nor->params.
> 

Pratyush is right.

> This eliminates the need for the call to
> nor->manufacturer->fixups->sr_ready. Now you can simply call
> nor->params->sr_ready(). The fixup hooks will take care of populating it
> with the correct function based on the flash or manufacturer.
> 

For operations that require a different sequence of opcodes issued,
or different checks needed, I would use something like from below.

struct spi_nor_ops {
	int (*ready)(struct spi_nor *nor);
};

struct spi_nor_flash_parameter {
	...
	struct spi_nor_erase_map        erase_map;
	struct spi_nor_ops		ops;
	...
};

You'll have to get rid of the SNOR_F_USE_FSR, SNOR_F_READY_XSR_RDY and
SNOR_F_USE_CLSR. You'll init the default spi_nor_ready() in
spi_nor_info_init_params(), and you will overwrite it for the 3 cases
from above in the manufacturer or flash default_init() hook.
You'll then use across the core nor->params->ops.ready().

Not related to your patch, but related with the overall architecture:
there is another case, where the just the opcode differs, where manufacturers
use different hex opcodes for the same command (ex. some manufacturers use 0x35
for reading CR, others use 0x15). That will require a struct spi_nor_opcodes.

Cheers,
ta