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

Miquel Raynal
2024-10-01T09:40:48Z

Hi Cheng Ming,

linchengming884@gmail.com wrote on Thu, 5 Sep 2024 13:53:32 +0800:

> From: Cheng Ming Lin <chengminglin@mxic.com.tw>
>
> 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@mxic.com.tw>
> ---
> 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