| Message ID | 20230725022302.210275-2-jaimeliao.tw@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | Add octal DTR support for Macronix flash | expand |
Hi, > From: JaimeLiao <jaimeliao.tw@gmail.com> You write "We" in your next patch. "We" as in macronix? Then please use your macronix email address for the patches. Please note, you can still send them through your gmail account. > Enable Octal DTR mode with 20 dummy cycles to allow running > at the maximum supported frequency for adding Macronix flash > in Octal DTR mode. Please explain a bit more what you are doing here. The patch itself looks dodgy. You are writing CR2 but maybe thats used for register accesses?! I also can't really tell from the macro names. > > Signed-off-by: JaimeLiao <jaimeliao.tw@gmail.com> > Co-authored-by: Tudor Ambarus <tudor.ambarus@linaro.org> There seems to be no written process documentation wether this has to be followed by a SoB (unlike Co-developed-by). Dunno. > --- > drivers/mtd/spi-nor/macronix.c | 77 ++++++++++++++++++++++++++++++++++ > 1 file changed, 77 insertions(+) > > diff --git a/drivers/mtd/spi-nor/macronix.c > b/drivers/mtd/spi-nor/macronix.c > index 04888258e891..9010b81e098f 100644 > --- a/drivers/mtd/spi-nor/macronix.c > +++ b/drivers/mtd/spi-nor/macronix.c > @@ -8,6 +8,12 @@ > > #include "core.h" > > +#define SPINOR_OP_RD_CR2 0x71 /* Read configuration register 2 */ > +#define SPINOR_OP_WR_CR2 0x72 /* Write configuration register 2 */ Copied from spansion.c? Why isn't there an _MXIC_ in the name? > +#define SPINOR_REG_MXIC_CR2_MODE 0x00000000 /* For setting octal DTR > mode */ > +#define SPINOR_REG_MXIC_OPI_DTR_EN 0x2 /* Enable Octal DTR */ > +#define SPINOR_REG_MXIC_SPI_EN 0x0 /* Enable SPI */ The names don't make much sense to me. Are you accessing individual registers? If so please use MXIC_REG_<regname> and MXIC_REG_<regname>_<bit/mode/mask/..> > + > static int > mx25l25635_post_bfpt_fixups(struct spi_nor *nor, > const struct sfdp_parameter_header *bfpt_header, > @@ -32,6 +38,76 @@ static const struct spi_nor_fixups mx25l25635_fixups > = { > .post_bfpt = mx25l25635_post_bfpt_fixups, > }; > > +/** > + * spi_nor_macronix_octal_dtr_enable() - Enable octal DTR on Macronix > flashes. > + * @nor: pointer to a 'struct spi_nor' > + * @enable: whether to enable Octal DTR or switch back to SPI > + * > + * Return: 0 on success, -errno otherwise. > + */ > +static int spi_nor_macronix_octal_dtr_enable(struct spi_nor *nor, bool > enable) > +{ > + struct spi_mem_op op; > + u8 *buf = nor->bouncebuf, i; > + int ret; > + > + /* Set/unset the octal and DTR enable bits. */ > + ret = spi_nor_write_enable(nor); > + if (ret) > + return ret; > + > + if (enable) { > + buf[0] = SPINOR_REG_MXIC_OPI_DTR_EN; > + } else { > + /* > + * 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] = SPINOR_REG_MXIC_SPI_EN; > + buf[1] = 0x0; > + } > + > + op = (struct spi_mem_op) > + SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WR_CR2, 1), > + SPI_MEM_OP_ADDR(4, SPINOR_REG_MXIC_CR2_MODE, 1), > + SPI_MEM_OP_NO_DUMMY, > + SPI_MEM_OP_DATA_OUT(enable ? 1 : 2, buf, 1)); > + > + if (!enable) > + spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR); > + > + ret = spi_mem_exec_op(nor->spimem, &op); > + if (ret) > + return ret; > + > + /* Read flash ID to make sure the switch was successful. */ While cleaning up the flash_info db I come around this and it is copied all over the place. Please work on factoring this (also the other code in micron-st.c and spansion.c) out into a helper. > + op = (struct spi_mem_op) > + SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 1), > + SPI_MEM_OP_ADDR(enable ? 4 : 0, 0, 1), > + SPI_MEM_OP_DUMMY(enable ? 4 : 0, 1), > + SPI_MEM_OP_DATA_IN(SPI_NOR_MAX_ID_LEN, buf, 1)); > + > + if (enable) > + spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR); > + > + ret = spi_mem_exec_op(nor->spimem, &op); > + if (ret) > + return ret; > + > + if (enable) { > + for (i = 0; i < nor->info->id_len; i++) > + if (buf[i * 2] != nor->info->id[i]) > + return -EINVAL; Why is the ID now swapped? Doesn't look right. -michael > + } else { > + if (memcmp(buf, nor->info->id, nor->info->id_len)) > + return -EINVAL; > + } > + > + return 0; > +} > + > static const struct flash_info macronix_nor_parts[] = { > /* Macronix */ > { "mx25l512e", INFO(0xc22010, 0, 64 * 1024, 1) > @@ -108,6 +184,7 @@ static const struct flash_info macronix_nor_parts[] > = { > static void macronix_nor_default_init(struct spi_nor *nor) > { > nor->params->quad_enable = spi_nor_sr1_bit6_quad_enable; > + nor->params->octal_dtr_enable = spi_nor_macronix_octal_dtr_enable; > } > > static void macronix_nor_late_init(struct spi_nor *nor)
Hi Michael > > Hi, > > > From: JaimeLiao <jaimeliao.tw@gmail.com> > > You write "We" in your next patch. "We" as in macronix? Then please > use your macronix email address for the patches. Please note, you > can still send them through your gmail account. Yes I am Macronix engineer and sorry for the company mail issue so that I can't send and reply patch on Macronix mail. > > > Enable Octal DTR mode with 20 dummy cycles to allow running > > at the maximum supported frequency for adding Macronix flash > > in Octal DTR mode. > > Please explain a bit more what you are doing here. The patch itself > looks dodgy. You are writing CR2 but maybe thats used for register > accesses?! I also can't really tell from the macro names. Ok I will correct it to _MXIC_ next version of patch. > > > > > Signed-off-by: JaimeLiao <jaimeliao.tw@gmail.com> > > Co-authored-by: Tudor Ambarus <tudor.ambarus@linaro.org> > > There seems to be no written process documentation wether this has > to be followed by a SoB (unlike Co-developed-by). Dunno. Maybe it is not "Co-authored-by", it will be correct next version. > > > --- > > drivers/mtd/spi-nor/macronix.c | 77 ++++++++++++++++++++++++++++++++++ > > 1 file changed, 77 insertions(+) > > > > diff --git a/drivers/mtd/spi-nor/macronix.c > > b/drivers/mtd/spi-nor/macronix.c > > index 04888258e891..9010b81e098f 100644 > > --- a/drivers/mtd/spi-nor/macronix.c > > +++ b/drivers/mtd/spi-nor/macronix.c > > @@ -8,6 +8,12 @@ > > > > #include "core.h" > > > > +#define SPINOR_OP_RD_CR2 0x71 /* Read configuration register 2 */ > > +#define SPINOR_OP_WR_CR2 0x72 /* Write configuration register 2 */ > > Copied from spansion.c? Why isn't there an _MXIC_ in the name? It will be correct next version. > > > +#define SPINOR_REG_MXIC_CR2_MODE 0x00000000 /* For setting octal DTR > > mode */ > > +#define SPINOR_REG_MXIC_OPI_DTR_EN 0x2 /* Enable Octal DTR */ > > +#define SPINOR_REG_MXIC_SPI_EN 0x0 /* Enable SPI */ > > The names don't make much sense to me. Are you accessing individual > registers? If so please use MXIC_REG_<regname> and > MXIC_REG_<regname>_<bit/mode/mask/..> > > > + > > static int > > mx25l25635_post_bfpt_fixups(struct spi_nor *nor, > > const struct sfdp_parameter_header *bfpt_header, > > @@ -32,6 +38,76 @@ static const struct spi_nor_fixups mx25l25635_fixups > > = { > > .post_bfpt = mx25l25635_post_bfpt_fixups, > > }; > > > > +/** > > + * spi_nor_macronix_octal_dtr_enable() - Enable octal DTR on Macronix > > flashes. > > + * @nor: pointer to a 'struct spi_nor' > > + * @enable: whether to enable Octal DTR or switch back to SPI > > + * > > + * Return: 0 on success, -errno otherwise. > > + */ > > +static int spi_nor_macronix_octal_dtr_enable(struct spi_nor *nor, bool > > enable) > > +{ > > + struct spi_mem_op op; > > + u8 *buf = nor->bouncebuf, i; > > + int ret; > > + > > + /* Set/unset the octal and DTR enable bits. */ > > + ret = spi_nor_write_enable(nor); > > + if (ret) > > + return ret; > > + > > + if (enable) { > > + buf[0] = SPINOR_REG_MXIC_OPI_DTR_EN; > > + } else { > > + /* > > + * 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] = SPINOR_REG_MXIC_SPI_EN; > > + buf[1] = 0x0; > > + } > > + > > + op = (struct spi_mem_op) > > + SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WR_CR2, 1), > > + SPI_MEM_OP_ADDR(4, SPINOR_REG_MXIC_CR2_MODE, 1), > > + SPI_MEM_OP_NO_DUMMY, > > + SPI_MEM_OP_DATA_OUT(enable ? 1 : 2, buf, 1)); > > + > > + if (!enable) > > + spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR); > > + > > + ret = spi_mem_exec_op(nor->spimem, &op); > > + if (ret) > > + return ret; > > + > > + /* Read flash ID to make sure the switch was successful. */ > > While cleaning up the flash_info db I come around this and it is > copied all over the place. Please work on factoring this (also the > other code in micron-st.c and spansion.c) out into a helper. Let me clearify the data order for read ID on Macronix flashes. Read ID SPI mode : c2-84-3c-c2-84-3c OPI DTR mode : c2-c2-84-84-3c-3c So that I create a specify judement for checking ID via 8D-8D-8D on Macronix flash. > > > + op = (struct spi_mem_op) > > + SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 1), > > + SPI_MEM_OP_ADDR(enable ? 4 : 0, 0, 1), > > + SPI_MEM_OP_DUMMY(enable ? 4 : 0, 1), > > + SPI_MEM_OP_DATA_IN(SPI_NOR_MAX_ID_LEN, buf, 1)); > > + > > + if (enable) > > + spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR); > > + > > + ret = spi_mem_exec_op(nor->spimem, &op); > > + if (ret) > > + return ret; > > + > > + if (enable) { > > + for (i = 0; i < nor->info->id_len; i++) > > + if (buf[i * 2] != nor->info->id[i]) > > + return -EINVAL; > > Why is the ID now swapped? Doesn't look right. Actullay 6 bytes data are c2-c2-84-84-3c-3c which are got by 8D-8D-8D read id on Macronix flash. > > -michael > > > + } else { > > + if (memcmp(buf, nor->info->id, nor->info->id_len)) > > + return -EINVAL; > > + } > > + > > + return 0; > > +} > > + > > static const struct flash_info macronix_nor_parts[] = { > > /* Macronix */ > > { "mx25l512e", INFO(0xc22010, 0, 64 * 1024, 1) > > @@ -108,6 +184,7 @@ static const struct flash_info macronix_nor_parts[] > > = { > > static void macronix_nor_default_init(struct spi_nor *nor) > > { > > nor->params->quad_enable = spi_nor_sr1_bit6_quad_enable; > > + nor->params->octal_dtr_enable = spi_nor_macronix_octal_dtr_enable; > > } > > > > static void macronix_nor_late_init(struct spi_nor *nor) Thanks your reply Jaime
Hi, >> You write "We" in your next patch. "We" as in macronix? Then please >> use your macronix email address for the patches. Please note, you >> can still send them through your gmail account. > Yes I am Macronix engineer and sorry for the company mail > issue so that I can't send and reply patch on Macronix mail. Thus I was saying that you should use the macronix mail for the patch author and the SoB and send your mails with your gmail account (git will take care of the difference). >> While cleaning up the flash_info db I come around this and it is >> copied all over the place. Please work on factoring this (also the >> other code in micron-st.c and spansion.c) out into a helper. > Let me clearify the data order for read ID on Macronix flashes. > Read ID > SPI mode : c2-84-3c-c2-84-3c > OPI DTR mode : c2-c2-84-84-3c-3c So you are basically duplicating the id bytes in DTR mode. The same byte is transferred both on the falling and rising edge of a clock cycle. I think this needs further changes to the core. I means that if the core will do a rdid in octal mode, it returns "garbage" for now. How is the data transferred in octal DTR mode? Reading the same data in SPI and octal DTR mode should return the exact same bytes. So one test would be to use the flash in SPI mode, write some data, enable octal DTR mode and read it back and compare it. They must match. > So that I create a specify judement for checking ID via 8D-8D-8D > on Macronix flash. No, please make it a common helper for all flashes. >> > + op = (struct spi_mem_op) >> > + SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 1), >> > + SPI_MEM_OP_ADDR(enable ? 4 : 0, 0, 1), >> > + SPI_MEM_OP_DUMMY(enable ? 4 : 0, 1), >> > + SPI_MEM_OP_DATA_IN(SPI_NOR_MAX_ID_LEN, buf, 1)); >> > + >> > + if (enable) >> > + spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR); >> > + >> > + ret = spi_mem_exec_op(nor->spimem, &op); >> > + if (ret) >> > + return ret; >> > + >> > + if (enable) { >> > + for (i = 0; i < nor->info->id_len; i++) >> > + if (buf[i * 2] != nor->info->id[i]) >> > + return -EINVAL; >> >> Why is the ID now swapped? Doesn't look right. > Actullay 6 bytes data are c2-c2-84-84-3c-3c which are got by 8D-8D-8D > read id > on Macronix flash. See above. -michael
Hi Michael > > Hi, > > >> You write "We" in your next patch. "We" as in macronix? Then please > >> use your macronix email address for the patches. Please note, you > >> can still send them through your gmail account. > > Yes I am Macronix engineer and sorry for the company mail > > issue so that I can't send and reply patch on Macronix mail. > > Thus I was saying that you should use the macronix mail for the > patch author and the SoB and send your mails with your gmail > account (git will take care of the difference). Got it, thanks. > > >> While cleaning up the flash_info db I come around this and it is > >> copied all over the place. Please work on factoring this (also the > >> other code in micron-st.c and spansion.c) out into a helper. > > Let me clearify the data order for read ID on Macronix flashes. > > Read ID > > SPI mode : c2-84-3c-c2-84-3c > > OPI DTR mode : c2-c2-84-84-3c-3c > > So you are basically duplicating the id bytes in DTR mode. The same > byte is transferred both on the falling and rising edge of a clock > cycle. I think this needs further changes to the core. I means that > if the core will do a rdid in octal mode, it returns "garbage" for > now. Do you mean a common function for ID comapre in core.c? > > How is the data transferred in octal DTR mode? Reading the same data > in SPI and octal DTR mode should return the exact same bytes. For all flashed in this patch, data reading are all same between SPI mode and octal DTR mode. > > So one test would be to use the flash in SPI mode, write some data, > enable octal DTR mode and read it back and compare it. They must > match. > > > So that I create a specify judement for checking ID via 8D-8D-8D > > on Macronix flash. > > No, please make it a common helper for all flashes. > > >> > + op = (struct spi_mem_op) > >> > + SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 1), > >> > + SPI_MEM_OP_ADDR(enable ? 4 : 0, 0, 1), > >> > + SPI_MEM_OP_DUMMY(enable ? 4 : 0, 1), > >> > + SPI_MEM_OP_DATA_IN(SPI_NOR_MAX_ID_LEN, buf, 1)); > >> > + > >> > + if (enable) > >> > + spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR); > >> > + > >> > + ret = spi_mem_exec_op(nor->spimem, &op); > >> > + if (ret) > >> > + return ret; > >> > + > >> > + if (enable) { > >> > + for (i = 0; i < nor->info->id_len; i++) > >> > + if (buf[i * 2] != nor->info->id[i]) > >> > + return -EINVAL; > >> > >> Why is the ID now swapped? Doesn't look right. > > Actullay 6 bytes data are c2-c2-84-84-3c-3c which are got by 8D-8D-8D > > read id > > on Macronix flash. > > See above. > > -michael Thank you Jaime
diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c index 04888258e891..9010b81e098f 100644 --- a/drivers/mtd/spi-nor/macronix.c +++ b/drivers/mtd/spi-nor/macronix.c @@ -8,6 +8,12 @@ #include "core.h" +#define SPINOR_OP_RD_CR2 0x71 /* Read configuration register 2 */ +#define SPINOR_OP_WR_CR2 0x72 /* Write configuration register 2 */ +#define SPINOR_REG_MXIC_CR2_MODE 0x00000000 /* For setting octal DTR mode */ +#define SPINOR_REG_MXIC_OPI_DTR_EN 0x2 /* Enable Octal DTR */ +#define SPINOR_REG_MXIC_SPI_EN 0x0 /* Enable SPI */ + static int mx25l25635_post_bfpt_fixups(struct spi_nor *nor, const struct sfdp_parameter_header *bfpt_header, @@ -32,6 +38,76 @@ static const struct spi_nor_fixups mx25l25635_fixups = { .post_bfpt = mx25l25635_post_bfpt_fixups, }; +/** + * spi_nor_macronix_octal_dtr_enable() - Enable octal DTR on Macronix flashes. + * @nor: pointer to a 'struct spi_nor' + * @enable: whether to enable Octal DTR or switch back to SPI + * + * Return: 0 on success, -errno otherwise. + */ +static int spi_nor_macronix_octal_dtr_enable(struct spi_nor *nor, bool enable) +{ + struct spi_mem_op op; + u8 *buf = nor->bouncebuf, i; + int ret; + + /* Set/unset the octal and DTR enable bits. */ + ret = spi_nor_write_enable(nor); + if (ret) + return ret; + + if (enable) { + buf[0] = SPINOR_REG_MXIC_OPI_DTR_EN; + } else { + /* + * 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] = SPINOR_REG_MXIC_SPI_EN; + buf[1] = 0x0; + } + + op = (struct spi_mem_op) + SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WR_CR2, 1), + SPI_MEM_OP_ADDR(4, SPINOR_REG_MXIC_CR2_MODE, 1), + SPI_MEM_OP_NO_DUMMY, + SPI_MEM_OP_DATA_OUT(enable ? 1 : 2, buf, 1)); + + if (!enable) + spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR); + + ret = spi_mem_exec_op(nor->spimem, &op); + if (ret) + return ret; + + /* Read flash ID to make sure the switch was successful. */ + op = (struct spi_mem_op) + SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 1), + SPI_MEM_OP_ADDR(enable ? 4 : 0, 0, 1), + SPI_MEM_OP_DUMMY(enable ? 4 : 0, 1), + SPI_MEM_OP_DATA_IN(SPI_NOR_MAX_ID_LEN, buf, 1)); + + if (enable) + spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR); + + ret = spi_mem_exec_op(nor->spimem, &op); + if (ret) + return ret; + + if (enable) { + for (i = 0; i < nor->info->id_len; i++) + if (buf[i * 2] != nor->info->id[i]) + return -EINVAL; + } else { + if (memcmp(buf, nor->info->id, nor->info->id_len)) + return -EINVAL; + } + + return 0; +} + static const struct flash_info macronix_nor_parts[] = { /* Macronix */ { "mx25l512e", INFO(0xc22010, 0, 64 * 1024, 1) @@ -108,6 +184,7 @@ static const struct flash_info macronix_nor_parts[] = { static void macronix_nor_default_init(struct spi_nor *nor) { nor->params->quad_enable = spi_nor_sr1_bit6_quad_enable; + nor->params->octal_dtr_enable = spi_nor_macronix_octal_dtr_enable; } static void macronix_nor_late_init(struct spi_nor *nor)