[PATCH v1 1/2] mtd: spi-nor: add Octal DTR support for Macronix flash
liao jaime
jaimeliao.tw at gmail.com
Tue Jul 25 02:05:42 PDT 2023
Hi Michael
>
> Hi,
>
> > From: JaimeLiao <jaimeliao.tw at 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 at gmail.com>
> > Co-authored-by: Tudor Ambarus <tudor.ambarus at 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
More information about the linux-mtd
mailing list