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

[Tudor Ambarus <tudor.ambarus@linaro.org>]

On 26.09.2024 17:19, AlvinZhou wrote:
> From: AlvinZhou <alvinzhou@mxic.com.tw>
> 
> 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@linaro.org>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
> Signed-off-by: AlvinZhou <alvinzhou@mxic.com.tw>
> ---
>  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.

> +#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.

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

Looks good.