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

[Cheng Ming Lin]

Hi Miquel,

Miquel Raynal <miquel.raynal@xxxxxxxxxxx> 於 2024年10月1日 週二 下午5:40寫道：
>
> 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?

Since the original mode is labeled as default in our datasheet,
there are a total of six modes, including five additional modes
numbered from Mode 1 to Mode 5.

>
> > +#define MACRONIX_FEATURE_ADDR_READ_RETRY 0x70
>
> Both definitions should probably come...
>
> >  #define SPINAND_MFR_MACRONIX         0xC2
> >  #define MACRONIX_ECCSR_MASK          0x0F
>
> ...here

Sure, thanks for your advice.

>
> >
> > @@ -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.

Got it, so do you prefer that we switch to using flags instead?

>
> > 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?

No, we can simply change from int to u8.

>
> >  };
> >
> >  /**
>
>
> Thanks,
> Miquèl

Thanks,
Cheng Ming Lin