Re: [PATCH 1/2] mtd: spi-nand: Add fixups for read retry

[Miquel Raynal]

Hi Cheng Ming,

linchengming884@xxxxxxxxx wrote on Thu,  5 Sep 2024 13:53:32 +0800:

> From: Cheng Ming Lin <chengminglin@xxxxxxxxxxx>
> 
> Add fixups for support read retry:
> - Initialize the NAND device maximum retry mode.
> - Set feature on Special Read for Data Recovery register.
> 
> The Special Read for Data Recovery operation is enabled by Set Feature
> function.
> 
> There are 5 modes for the user to recover the lost data.
> 
> Signed-off-by: Cheng Ming Lin <chengminglin@xxxxxxxxxxx>
> ---
>  drivers/mtd/nand/spi/macronix.c | 79 ++++++++++++++++++++++++++-------
>  include/linux/mtd/spinand.h     | 17 +++++++
>  2 files changed, 81 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/mtd/nand/spi/macronix.c b/drivers/mtd/nand/spi/macronix.c
> index 3f9e9c572854..baca67ff1cd6 100644
> --- a/drivers/mtd/nand/spi/macronix.c
> +++ b/drivers/mtd/nand/spi/macronix.c
> @@ -9,6 +9,8 @@
>  #include <linux/kernel.h>
>  #include <linux/mtd/spinand.h>
>  
> +#define MACRONIX_NUM_READ_RETRY_MODES 6

You said 5 in the cover letter?

> +#define MACRONIX_FEATURE_ADDR_READ_RETRY 0x70

Both definitions should probably come...

>  #define SPINAND_MFR_MACRONIX		0xC2
>  #define MACRONIX_ECCSR_MASK		0x0F

...here

>  
> @@ -100,6 +102,38 @@ static int mx35lf1ge4ab_ecc_get_status(struct spinand_device *spinand,
>  	return -EINVAL;
>  }
>  
> +/**
> + * macronix_spinand_init_read_retry - Initialize read_retries
> + * @spinand: SPI NAND device
> + *
> + * Return: the number of read retry modes
> + */
> +static int macronix_spinand_init_read_retry(struct spinand_device *spinand)
> +{
> +	return MACRONIX_NUM_READ_RETRY_MODES;

Does not sound very useful as a function?

> +}
> +
> +/**
> + * macronix_spinand_setup_read_retry - Set the retry mode
> + * @spinand: SPI NAND device
> + * @retry_mode: Specify which retry mode to set
> + *
> + * Return: 0 on success, -error otherwise

			, a negative error code otherwise.

> + */
> +static int macronix_spinand_setup_read_retry(struct spinand_device *spinand, u8 retry_mode)
> +{
> +	struct spi_mem_op op = SPINAND_SET_FEATURE_OP(MACRONIX_FEATURE_ADDR_READ_RETRY,
> +						      spinand->scratchbuf);
> +
> +	*spinand->scratchbuf = retry_mode;
> +	return spi_mem_exec_op(spinand->spimem, &op);
> +}
> +
> +static const struct spi_nand_fixups read_retry_fixups = {
> +	.init_read_retry = macronix_spinand_init_read_retry,
> +	.setup_read_retry = macronix_spinand_setup_read_retry,
> +};
> +

...

> @@ -325,7 +373,8 @@ static const struct spinand_info macronix_spinand_table[] = {
>  					      &update_cache_variants),
>  		     SPINAND_HAS_QE_BIT,
>  		     SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout,
> -				     mx35lf1ge4ab_ecc_get_status)),
> +				     mx35lf1ge4ab_ecc_get_status),
> +		     SPINAND_FIXUPS(&read_retry_fixups)),
>  	SPINAND_INFO("MX35UF1GE4AC",
>  		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x92, 0x01),
>  		     NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 1, 1),

I expect a patch targeting the core first, and then the changes in the
Macronix driver.

> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
> index 5c19ead60499..e567d00a2805 100644
> --- a/include/linux/mtd/spinand.h
> +++ b/include/linux/mtd/spinand.h
> @@ -354,6 +354,7 @@ struct spinand_info {
>  	} op_variants;
>  	int (*select_target)(struct spinand_device *spinand,
>  			     unsigned int target);
> +	const struct spi_nand_fixups *fixups;
>  };
>  
>  #define SPINAND_ID(__method, ...)					\
> @@ -379,6 +380,9 @@ struct spinand_info {
>  #define SPINAND_SELECT_TARGET(__func)					\
>  	.select_target = __func,
>  
> +#define SPINAND_FIXUPS(__func)					\
> +	.fixups = __func,

I don't like the fixups naming. It feels like something is inherently
wrong.

> +
>  #define SPINAND_INFO(__model, __id, __memorg, __eccreq, __op_variants,	\
>  		     __flags, ...)					\
>  	{								\
> @@ -398,6 +402,16 @@ struct spinand_dirmap {
>  	struct spi_mem_dirmap_desc *rdesc_ecc;
>  };
>  
> +/**
> + * struct spi_nand_fixups - SPI NAND fixup hooks
> + * @init_read_retry: initialize spinand->read_retries
> + * @setup_read_retry: set the retry mode
> + */
> +struct spi_nand_fixups {
> +	int (*init_read_retry)(struct spinand_device *spinand);
> +	int (*setup_read_retry)(struct spinand_device *spinand, u8 retry_mode);
> +};
> +
>  /**
>   * struct spinand_device - SPI NAND device instance
>   * @base: NAND device instance
> @@ -423,6 +437,7 @@ struct spinand_dirmap {
>   *		the stack
>   * @manufacturer: SPI NAND manufacturer information
>   * @priv: manufacturer private data
> + * @read_retries: the number of read retry modes supported
>   */
>  struct spinand_device {
>  	struct nand_device base;
> @@ -449,8 +464,10 @@ struct spinand_device {
>  	u8 *databuf;
>  	u8 *oobbuf;
>  	u8 *scratchbuf;
> +	const struct spinand_info *info;

This looks like a leftover.

>  	const struct spinand_manufacturer *manufacturer;
>  	void *priv;
> +	int read_retries;

Any reason to keep this variable signed?

>  };
>  
>  /**


Thanks,
Miquèl