Re: [PATCH v3 1/3] mtd: spi-nor: macronix: add support for Macronix octal dtr operation

[zhengxunli@mxic.com.tw]

Hi Pratyush,

Thanks for your comment on this patch.

"Pratyush Yadav" <p.yadav@ti.com> wrote on 2021/04/27 上午 10:36:06:

> "Pratyush Yadav" <p.yadav@ti.com> 
> 2021/04/27 上午 10:36
> 
> To
> 
> "Zhengxun Li" <zhengxunli@mxic.com.tw>, 
> 
> cc
> 
> <linux-mtd@lists.infradead.org>, <linux-spi@vger.kernel.org>, 
> <tudor.ambarus@microchip.com>, <miquel.raynal@bootlin.com>, 
> <broonie@kernel.org>, <jaimeliao@mxic.com.tw>
> 
> Subject
> 
> Re: [PATCH v3 1/3] mtd: spi-nor: macronix: add support for Macronix 
> octal dtr operation
> 
> Hi,
> 
> On 20/04/21 02:29PM, Zhengxun Li wrote:
> > The ocatflash is an xSPI compliant octal DTR flash. Add support
> 
> Typo. s/ocatflash/octaflash/

Okay, it will be fixed in the next version.

> 
> > for using it in octal DTR mode.
> > 
> > Enable Octal DTR mode with 20 dummy cycles to allow running at the
> > maximum supported frequency of 200Mhz.
> 
> Which octaflash is that? The flash datasheets you have linked in patch 2 

> either have a max supported frequency of 133 MHz or 250 MHz.

Okay, it will be fixed in the next version.

> > 
> > Try to verify the flash ID to check whether the flash memory in octal
> > DTR mode is correct. When reading ID in OCTAL DTR mode, ID will appear
> > in a repeated manner. ex: ID[0] = 0xc2, ID[1] = 0xc2, ID[2] = 0x94,
> > ID[3] = 0x94... Rearrange the order so that the ID can pass.
> 
> Ok. I don't see this mentioned in the datasheet but the timing diagram 
> seems to imply this.
> 
> > 
> > Signed-off-by: Zhengxun Li <zhengxunli@mxic.com.tw>
> > ---
> >  drivers/mtd/spi-nor/macronix.c | 117 ++++++++++++++++++++++++++++
> +++++++++++++
> >  1 file changed, 117 insertions(+)
> > 
> > diff --git a/drivers/mtd/spi-nor/macronix.c 
b/drivers/mtd/spi-nor/macronix.c
> > index 42c2cf3..881eaf8 100644
> > --- a/drivers/mtd/spi-nor/macronix.c
> > +++ b/drivers/mtd/spi-nor/macronix.c
> > @@ -8,6 +8,16 @@
> > 
> >  #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_OP_MXIC_DTR_RD      0xee      /* Fast Read opcode 
> in DTR mode */
> > +#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_OPI_DTR_DIS   0x1      /* Disable Octal DTR 
*/
> 
> This would switch the flash to 8S-8S-8S mode, which isn't all that much 
> better than 8D-8D-8D (it is a stateful mode, you need to know beforehand 

> that the flash is in this mode before you can use it properly). I think 
> "disabling" octal DTR should mean switching back to the default 
> (1S-1S-1S) mode.
> 

Yes, should be switching back to default(1s-1s-1s) mode.

> > +#define SPINOR_REG_MXIC_CR2_DC      0x00000300   /* For setting 
> dummy cycles */
> > +#define SPINOR_REG_MXIC_DC_20      0x0      /* Setting dummy 
> cycles to 20 */
> > +#define MXIC_MAX_DC         20      /* Maximum value of dummy cycles 
*/
> > +
> >  static int
> >  mx25l25635_post_bfpt_fixups(struct spi_nor *nor,
> >               const struct sfdp_parameter_header *bfpt_header,
> > @@ -32,6 +42,113 @@
> >     .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 or disable Octal DTR
> > + *
> > + * This also sets the memory access dummy cycles to 20 to allow 
> the flash to
> > + * run at up to 200MHz.
> 
> For some flashes it is 250 MHz and for some it is 133 MHz. More on this 
> below...
> 

Okay, it will be fixed in the next version.

> > + *
> > + * 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;
> > +
> > +   if (enable) {
> > +      /* Use 20 dummy cycles for memory array reads. */
> > +      ret = spi_nor_write_enable(nor);
> > +      if (ret)
> > +         return ret;
> > +
> > +      *buf = SPINOR_REG_MXIC_DC_20;
> 
> I've looked at both the 133 and 250 MHz parts. In both, the default 
> dummy cycles are already 20. This can be skipped. This way you also 
> don't have to say in comments whether this enables 133 MHz or 250 MHz 
> operation (again, I don't get where the 200 MHz comes from...).
> 
> The chip manufacturer chose sane defaults for the dummy cycle value. 
> Let's reap the benefits and reduce the code we have to maintain.

Okay, it will be fixed in the next version.

> > +      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_DC, 1),
> > +               SPI_MEM_OP_NO_DUMMY,
> > +               SPI_MEM_OP_DATA_OUT(1, buf, 1));
> > +
> > +      ret = spi_mem_exec_op(nor->spimem, &op);
> > +      if (ret)
> > +         return ret;
> > +
> > +      ret = spi_nor_wait_till_ready(nor);
> > +      if (ret)
> > +         return ret;
> > +
> > +      nor->read_dummy = MXIC_MAX_DC;
> 
> I don't see SFDP values listed in the datasheet. Since the flash is 
> supposed to be xSPI compliant, it should have a Profile 1.0 table. Does 
> the Profile 1.0 parser correctly select 20 dummy cycles for this flash? 
> If yes, there is no need for this.

yes, should be removed in the next version.

> > +   }
> > +
> > +   /* Set/unset the octal and DTR enable bits. */
> > +   ret = spi_nor_write_enable(nor);
> > +   if (ret)
> > +      return ret;
> > +
> > +   if (enable)
> > +      *buf = SPINOR_REG_MXIC_OPI_DTR_EN;
> > +   else
> > +      *buf = SPINOR_REG_MXIC_OPI_DTR_DIS;
> > +
> > +   op = (struct spi_mem_op)
> > +      SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WR_CR2, 1),
> 
> I see that the flash uses inverted 2nd byte for opcode in 8D mode. I 
> assume this is discovered correctly via SFDP.
> 
> > +            SPI_MEM_OP_ADDR(4, SPINOR_REG_MXIC_CR2_MODE, 1),
> 
> 4 byte addressing is used for this command even in 1S-1S-1S mode. Ok.
> 
> > +            SPI_MEM_OP_NO_DUMMY,
> > +            SPI_MEM_OP_DATA_OUT(1, buf, 1));
> > +
> > +   if (!enable)
> > +      spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
> 
> When disabling, the op would be in 8D-8D-8D mode so having a data length 

> of 1 would be invalid. This is currently the case even in the patches 
> that I sent for Micron and Cypress.
> 
> I am not sure what the correct fix for this is though. One option is to 
> send the same byte twice, but I remember that on the Cypress flash the 
> second byte over-writes the register at the next address. I'm not sure 
> how Macronix flashes handle the second byte. Can you check what the 
> behavior for your flash is when you write 2 bytes to the register?

I checked the behavior of Macronix and the second byte will overwrites the 
register.
Do we need to send the same bytes to resolve this error?

> > +
> > +   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;
> > +
> > +   for (i = 0; i < nor->info->id_len; i++)
> > +      if (buf[i * 2] != nor->info->id[i])
> > +         return -EINVAL;
> 
> Ok.
> 
> > +
> > +   return 0;
> > +}
> > +
> > +static void octaflash_default_init(struct spi_nor *nor)
> > +{
> > +   nor->params->octal_dtr_enable = spi_nor_macronix_octal_dtr_enable;
> > +}
> > +
> > +static void octaflash_post_sfdp_fixup(struct spi_nor *nor)
> > +{
> > +   /* Set the Fast Read settings. */
> > +   nor->params->hwcaps.mask |= SNOR_HWCAPS_READ_8_8_8_DTR;
> > + 
spi_nor_set_read_settings(&nor->params->reads[SNOR_CMD_READ_8_8_8_DTR],
> > +              0, MXIC_MAX_DC, SPINOR_OP_MXIC_DTR_RD,
> > +              SNOR_PROTO_8_8_8_DTR);
> > +
> > +   nor->cmd_ext_type = SPI_NOR_EXT_INVERT;
> 
> Shouldn't this be discovered via BFPT DWORD 18?

Okay, it will be removed in the next version.

> 
> > +   nor->params->rdsr_dummy = 4;
> > +   nor->params->rdsr_addr_nbytes = 4;
> 
> Shouldn't these two be discovered via the Profile 1.0 table?
> 
> In general, avoid hard-coding values that can be discovered through 
> SFDP. The device usually knows more about itself than we do.
> 

Okay, it will be removed in the next version.

> > +}
> > +
> > +static struct spi_nor_fixups octaflash_fixups = {
> 
> Hm, I don't like this unreferenced variable here. In fact, you should
> merge patch 1 and 2 into a single patch. This would combine things in a 
> single neat commit, including the links to datasheets. This will make it 

> easier for future archaeologists digging around with git blame to find 
> some more info about the flash.

It will be merged in the next version.

> > +   .default_init = octaflash_default_init,
> > +   .post_sfdp = octaflash_post_sfdp_fixup,
> > +};
> > +
> >  static const struct flash_info macronix_parts[] = {
> >     /* Macronix */
> >     { "mx25l512e",   INFO(0xc22010, 0, 64 * 1024,   1, SECT_4K) },
> > -- 
> > 1.9.1
> 
> That's an ancient version of Git you're using there ;-)
> 
> Overall, I like the direction this patch is taking, but lots of minor 
> things to polish out.

Thanks,
Zhengxun


CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================