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

Commit Message

Alvin Zhou Sept. 26, 2024, 2:19 p.m. UTC

From: AlvinZhou <[e-mail]>

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 <[e-mail]>
Signed-off-by: Tudor Ambarus <[e-mail]>
Signed-off-by: JaimeLiao <[e-mail]>
Signed-off-by: AlvinZhou <[e-mail]>
---
 drivers/mtd/spi-nor/macronix.c | 91 ++++++++++++++++++++++++++++++++++
 1 file changed, 91 insertions(+)

Comments

Tudor Ambarus Oct. 2, 2024, 7:16 a.m. UTC | #1

On 26.09.2024 17:19, AlvinZhou wrote:
> From: AlvinZhou <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Tudor Ambarus <[e-mail]>
> Signed-off-by: JaimeLiao <[e-mail]>
> Signed-off-by: AlvinZhou <[e-mail]>
> ---
>  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.

Alvin Zhou Oct. 4, 2024, 9:05 a.m. UTC | #2

Hi Tudor,

Tudor Ambarus <[e-mail]> 於 2024年10月2日 週三 下午3:16寫道：
>
>
>
> On 26.09.2024 17:19, AlvinZhou wrote:
> > From: AlvinZhou <[e-mail]>
> >
> > 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 <[e-mail]>
> > Signed-off-by: Tudor Ambarus <[e-mail]>
> > Signed-off-by: JaimeLiao <[e-mail]>
> > Signed-off-by: AlvinZhou <[e-mail]>
> > ---
> >  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

Tudor Ambarus Oct. 4, 2024, 9:58 a.m. UTC | #3

On 10/4/24 10:05 AM, Alvin Zhou wrote:
>>> +     /* 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


nor->addr_nbytes is set in spi_nor_setup().
spi_nor_set_octal_dtr() is called after spi_nor_setup(), thus you can
use nor->addr_nbytes.

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

but we don't want magic numbers or states that are not tracked, so use
the parameters set

> Moreover, nor->addr_nbytes is set within the spi_nor_setup() function.

yep, use it then.

Alvin Zhou Oct. 4, 2024, 4:22 p.m. UTC | #4

Hi Tudor,

Tudor Ambarus <[e-mail]> 於 2024年10月4日 週五 下午5:58寫道：
>
>
>
> On 10/4/24 10:05 AM, Alvin Zhou wrote:
> >>> +     /* 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
>
>
> nor->addr_nbytes is set in spi_nor_setup().
> spi_nor_set_octal_dtr() is called after spi_nor_setup(), thus you can
> use nor->addr_nbytes.

I apologize for the misunderstanding. Thanks for your clarification. So
using nor->addr_nbytes as the second argument is not a problem. I
have verified, thanks!
>
> > 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.
>
> but we don't want magic numbers or states that are not tracked, so use
> the parameters set
>
> > Moreover, nor->addr_nbytes is set within the spi_nor_setup() function.
>
> yep, use it then.

Thanks,
Alvin

Patch

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)
+
+/* Macronix write CR2 operations */
+#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);
+	if (ret) {
+		dev_dbg(nor->dev, "error %d reading JEDEC ID after enabling 8D-8D-8D mode\n", ret);
+		return ret;
+	}
+
+	/* Macronix SPI-NOR flash 8D-8D-8D read ID would get 6 bytes data A-A-B-B-C-C */
+	for (i = 0; i < nor->info->id->len; i++)
+		if (buf[i * 2] != buf[(i * 2) + 1] || buf[i * 2] != nor->info->id->bytes[i])
+			return -EINVAL;
+
+	return 0;
+}
+
+static int macronix_nor_octal_dtr_dis(struct spi_nor *nor)
+{
+	struct spi_mem_op op;
+	u8 *buf = nor->bouncebuf;
+	int ret;
+
+	/*
+	 * The register is 1-byte wide, but 1-byte transactions are not
+	 * allowed in 8D-8D-8D mode. Since there is no register at the
+	 * next location, just initialize the value to 0 and let the
+	 * transaction go on.
+	 */
+	buf[0] = MXIC_NOR_REG_SPI_EN;
+	buf[1] = 0x0;
+	op = (struct spi_mem_op)MXIC_NOR_WR_CR2(MXIC_NOR_ADDR_CR2_MODE, 2, buf);
+	ret = spi_nor_write_any_volatile_reg(nor, &op, SNOR_PROTO_8_8_8_DTR);
+	if (ret)
+		return ret;
+
+	/* Read flash ID to make sure the switch was successful. */
+	ret = spi_nor_read_id(nor, 0, 0, buf, SNOR_PROTO_1_1_1);
+	if (ret) {
+		dev_dbg(nor->dev, "error %d reading JEDEC ID after disabling 8D-8D-8D mode\n", ret);
+		return ret;
+	}
+
+	if (memcmp(buf, nor->info->id->bytes, nor->info->id->len))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int macronix_nor_set_octal_dtr(struct spi_nor *nor, bool enable)
+{
+	return enable ? macronix_nor_octal_dtr_en(nor) : macronix_nor_octal_dtr_dis(nor);
+}
+
 static void macronix_nor_default_init(struct spi_nor *nor)
 {
 	nor->params->quad_enable = spi_nor_sr1_bit6_quad_enable;
@@ -194,6 +284,7 @@  static int macronix_nor_late_init(struct spi_nor *nor)
 {
 	if (!nor->params->set_4byte_addr_mode)
 		nor->params->set_4byte_addr_mode = spi_nor_set_4byte_addr_mode_en4b_ex4b;
+	nor->params->set_octal_dtr = macronix_nor_set_octal_dtr;
 
 	return 0;
 }