| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAPhrvRQirexA9QJzBSqjfmPnbnF62-hzg-neQ-cZX2hnkP_Zwg@mail.gmail.com>
Date: Fri, 4 Oct 2024 17:05:21 +0800
From: Alvin Zhou <alvinzhou.tw@...il.com>
To: Tudor Ambarus <tudor.ambarus@...aro.org>
Cc: linux-mtd@...ts.infradead.org, linux-spi@...r.kernel.org,
linux-kernel@...r.kernel.org, pratyush@...nel.org, mwalle@...nel.org,
miquel.raynal@...tlin.com, richard@....at, vigneshr@...com,
broonie@...nel.org, chengminglin@...c.com.tw, leoyu@...c.com.tw,
AlvinZhou <alvinzhou@...c.com.tw>, JaimeLiao <jaimeliao@...c.com.tw>
Subject: Re: [PATCH v10 1/6] mtd: spi-nor: add Octal DTR support for Macronix flash
Hi Tudor,
Tudor Ambarus <tudor.ambarus@...aro.org> 於 2024年10月2日 週三 下午3:16寫道：
>
>
>
> On 26.09.2024 17:19, AlvinZhou wrote:
> > From: AlvinZhou <alvinzhou@...c.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@...aro.org>
> > Signed-off-by: Tudor Ambarus <tudor.ambarus@...aro.org>
> > Signed-off-by: JaimeLiao <jaimeliao@...c.com.tw>
> > Signed-off-by: AlvinZhou <alvinzhou@...c.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.
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
Powered by blists - more mailing lists