Re: [PATCH v10 1/6] mtd: spi-nor: add Octal DTR support for Macronix flash

[Alvin Zhou <alvinzhou.tw@xxxxxxxxx>]

Hi Tudor,

Tudor Ambarus <tudor.ambarus@xxxxxxxxxx> 於 2024年10月2日 週三 下午3:16寫道：
>
>
>
> On 26.09.2024 17:19, AlvinZhou wrote:
> > From: AlvinZhou <alvinzhou@xxxxxxxxxxx>
> >
> > Create Macronix specify method for enable Octal DTR mode and
> > set 20 dummy cycles to allow running at the maximum supported
> > frequency for Macronix Octal flash.
> >
> > Use number of dummy cycles which is parse by SFDP then convert
> > it to bit pattern and set in CR2 register.
> > Set CR2 register for enable octal DTR mode.
> >
> > Use Read ID to confirm that enabling/disabling octal DTR mode
> > was successful.
> >
> > Macronix ID format is A-A-B-B-C-C in octal DTR mode.
> > To ensure the successful enablement of octal DTR mode, confirm
> > that the 6-byte data is entirely correct.
> >
> > Co-developed-by: Tudor Ambarus <tudor.ambarus@xxxxxxxxxx>
> > Signed-off-by: Tudor Ambarus <tudor.ambarus@xxxxxxxxxx>
> > Signed-off-by: JaimeLiao <jaimeliao@xxxxxxxxxxx>
> > Signed-off-by: AlvinZhou <alvinzhou@xxxxxxxxxxx>
> > ---
> >  drivers/mtd/spi-nor/macronix.c | 91 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 91 insertions(+)
> >
> > diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
> > index ea6be95e75a5..f039819a5252 100644
> > --- a/drivers/mtd/spi-nor/macronix.c
> > +++ b/drivers/mtd/spi-nor/macronix.c
> > @@ -8,6 +8,24 @@
> >
> >  #include "core.h"
> >
> > +#define MXIC_NOR_OP_RD_CR2   0x71            /* Read configuration register 2 opcode */
> > +#define MXIC_NOR_OP_WR_CR2   0x72            /* Write configuration register 2 opcode */
> > +#define MXIC_NOR_ADDR_CR2_MODE       0x00000000      /* CR2 address for setting spi/sopi/dopi mode */
> > +#define MXIC_NOR_ADDR_CR2_DC 0x00000300      /* CR2 address for setting dummy cycles */
> > +#define MXIC_NOR_REG_DOPI_EN 0x2             /* Enable Octal DTR */
> > +#define MXIC_NOR_REG_SPI_EN  0x0             /* Enable SPI */
> > +
> > +/* Convert dummy cycles to bit pattern */
> > +#define MXIC_NOR_REG_DC(p) \
> > +     ((20 - (p)) >> 1)
>
> This is unfortunate as we convert dummy cycles to bytes in mtd and then
> we convert back from bytes to cycles in spi. I had an attempt fixing
> this in the past, but couldn't allocate more time for spinning another
> version for the patch set.
>
> I won't block the patch set for this, but if someone cares to fix it,
> would be great to take over.
>
> > +
> > +/* Macronix write CR2 operations */
>
> I'll drop this comment when applying, as I can already see what the
> macro is doing from its name.

Got it, I will pay attention to it, thanks!

>
> > +#define MXIC_NOR_WR_CR2(addr, ndata, buf)                    \
> > +     SPI_MEM_OP(SPI_MEM_OP_CMD(MXIC_NOR_OP_WR_CR2, 0),       \
> > +                SPI_MEM_OP_ADDR(4, addr, 0),                 \
> > +                SPI_MEM_OP_NO_DUMMY,                         \
> > +                SPI_MEM_OP_DATA_OUT(ndata, buf, 0))
> > +
> >  static int
> >  mx25l25635_post_bfpt_fixups(struct spi_nor *nor,
> >                           const struct sfdp_parameter_header *bfpt_header,
> > @@ -185,6 +203,78 @@ static const struct flash_info macronix_nor_parts[] = {
> >       }
> >  };
> >
> > +static int macronix_nor_octal_dtr_en(struct spi_nor *nor)
> > +{
> > +     struct spi_mem_op op;
> > +     u8 *buf = nor->bouncebuf, i;
> > +     int ret;
> > +
> > +     /* Use dummy cycles which is parse by SFDP and convert to bit pattern. */
> > +     buf[0] = MXIC_NOR_REG_DC(nor->params->reads[SNOR_CMD_READ_8_8_8_DTR].num_wait_states);
> > +     op = (struct spi_mem_op)MXIC_NOR_WR_CR2(MXIC_NOR_ADDR_CR2_DC, 1, buf);
> > +     ret = spi_nor_write_any_volatile_reg(nor, &op, nor->reg_proto);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Set the octal and DTR enable bits. */
> > +     buf[0] = MXIC_NOR_REG_DOPI_EN;
> > +     op = (struct spi_mem_op)MXIC_NOR_WR_CR2(MXIC_NOR_ADDR_CR2_MODE, 1, buf);
> > +     ret = spi_nor_write_any_volatile_reg(nor, &op, nor->reg_proto);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Read flash ID to make sure the switch was successful. */
> > +     ret = spi_nor_read_id(nor, 4, 4, buf, SNOR_PROTO_8_8_8_DTR);
>
> can we use nor->addr_nbytes for the second argument? Please test and
> confirm. No need to resend for this, just confirm and I can amend when
> applying.

The following is the process of spi_nor_scan()
int spi_nor_scan(...)
{
......
ret = spi_nor_init_params(nor);
......
ret = spi_nor_setup(nor, hwcaps);
......
}
First, within the spi_nor_parse_sfdp() function inside
spi_nor_init_params(): nor->params->addr_nbytes is set based on the
SFDP, while nor->addr_nbytes is not available. Therefore, the second
argument cannot use nor->addr_nbytes but can use
nor->params->addr_nbytes. Additionally, For Macronix Octal NOR Flash
in Octal DDR mode, both the address and dummy cycles are fixed at 4
in READID, so setting the second and third argument to 4 is also valid.
Moreover, nor->addr_nbytes is set within the spi_nor_setup() function.

>
> What about the third argument, the number of dummy nbytes. Can we get
> the cycles needed for READID from somewhere in SFDP?

Currently, the SFDP does not provide the number of dummy cycles for the
READID.

>
> Looks good.

Thanks,
Alvin