Messages in this thread | | | From | <> | Subject | Re: [PATCH v2 2/2] mtd: spi-nor: macronix: Add support for mx66lm1g45g | Date | Fri, 17 Dec 2021 12:38:52 +0000 |
| |
On 12/17/21 1:38 PM, Pratyush Yadav wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 09/12/21 09:04PM, Tudor Ambarus wrote: >> mx66lm1g45g supports just 1-1-1, 8-8-8 and 8d-8d-8d modes. There are >> versions of mx66lm1g45g which do not support SFDP, thus use >> SPI_NOR_SKIP_SFDP. The RDID command issued through the octal peripheral >> interface outputs data always in STR mode for whatever reason. Since > > Huh! I hope this is a mistake from the chip designers, because if it > isn't they need a stern talking-to ;-) > >> 8d-8d-8s is not common, avoid reading the ID when enabling the octal dtr >> mode. Instead, read back the CR2 to check if the switch was successful. >> Tested in 1-1-1 and 8d-8d-8d modes using sama7g5 QSPI IP. > > Datasheet?
MX66LM1G45G datasheet: https://www.macronix.com/Lists/Datasheet/Attachments/7929/MX66LM1G45G,%203V,%201Gb,%20v1.1.pdf
If you meant the controller's datasheet, it's not publicly available yet. > >> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> >> --- >> v2: SPI_NOR_SOFT_RESET as a FIXUP_FLAG >> >> # cat /sys/devices/platform/soc/e080c000.spi/spi_master/spi1/spi1.0/spi-nor/jedec_id >> c2853b >> # cat /sys/devices/platform/soc/e080c000.spi/spi_master/spi1/spi1.0/spi-nor/manufacturer >> macronix >> # cat /sys/devices/platform/soc/e080c000.spi/spi_master/spi1/spi1.0/spi-nor/partname >> mx66lm1g45g >> # cat /sys/devices/platform/soc/e080c000.spi/spi_master/spi1/spi1.0/spi-nor/sfdp >> cat: can't open '/sys/devices/platform/soc/e080c000.spi/spi_master/spi1/spi1.0/spi-nor/sfdp': No such file or directory >> >> drivers/mtd/spi-nor/macronix.c | 113 +++++++++++++++++++++++++++++++++ >> 1 file changed, 113 insertions(+) >> >> diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c >> index 67aaa83038b6..9d71149233e3 100644 >> --- a/drivers/mtd/spi-nor/macronix.c >> +++ b/drivers/mtd/spi-nor/macronix.c >> @@ -32,6 +32,112 @@ static struct spi_nor_fixups mx25l25635_fixups = { >> .post_bfpt = mx25l25635_post_bfpt_fixups, >> }; >> >> +#define SPINOR_OP_READ_CR2 0x71 >> +#define SPINOR_OP_WRITE_CR2 0x72 >> +#define SPINOR_OP_MX_DTR_RD 0xee >> + >> +#define SPINOR_REG_CR2_MODE_ADDR 0 >> +#define SPINOR_REG_CR2_DTR_OPI_ENABLE BIT(1) >> +#define SPINOR_REG_CR2_SPI 0 >> + >> +#define SPINOR_REG_CR2_DUMMY_ADDR 0x300 >> +#define SPINOR_REG_CR2_DUMMY_20 0 >> + >> +static int spi_nor_macronix_octal_dtr_enable(struct spi_nor *nor, bool enable) >> +{ >> + struct spi_mem_op op; >> + int ret; >> + >> + /* Use 20 dummy cycles for memory array reads. */ > > I just want to point out that if the default dummy cycle value can work > at the maximum frequency the flash supports, you don't need to do this. > I did it for S28 and MT35 because this wasn't the case but I remember > some flashes having sane defaults and not needing this.
This is a volatile bit field with the default value of 20 dummy cycles indeed, so it should be fine if the bootloaders do not touch it. Anyway, it's probably a good idea to reset the flash at kernel level before starting configuring it, so we can consider the default values as sane. I'll drop this config, sure.
> >> + if (enable) { >> + nor->bouncebuf[0] = SPINOR_REG_CR2_DUMMY_20; >> + op = (struct spi_mem_op) >> + SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WRITE_CR2, 1), >> + SPI_MEM_OP_ADDR(4, SPINOR_REG_CR2_DUMMY_ADDR, >> + 1), >> + SPI_MEM_OP_NO_DUMMY, >> + SPI_MEM_OP_DATA_OUT(1, nor->bouncebuf, 1)); >> + >> + ret = spi_nor_write_enable(nor); >> + if (ret) >> + return ret; >> + >> + ret = spi_mem_exec_op(nor->spimem, &op); >> + if (ret) >> + return ret; >> + >> + ret = spi_nor_wait_till_ready(nor); >> + if (ret) >> + return ret; >> + } >> + >> + /* Set/unset the octal and DTR enable bits. */ >> + if (enable) >> + nor->bouncebuf[0] = SPINOR_REG_CR2_DTR_OPI_ENABLE; >> + else >> + nor->bouncebuf[0] = SPINOR_REG_CR2_SPI; >> + >> + op = (struct spi_mem_op) >> + SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WRITE_CR2, 1), >> + SPI_MEM_OP_ADDR(4, SPINOR_REG_CR2_MODE_ADDR, 1), >> + SPI_MEM_OP_NO_DUMMY, >> + SPI_MEM_OP_DATA_OUT(1, nor->bouncebuf, 1)); > > This is not quite right. You can't have a 1-byte data phase in 8D mode > since then your data phase is only half a cycle. What happens to the > other half cycle would be undefined behavior for most controllers. I > sent some patches addressing this some time back [0]. Unfortunately they > fell off my radar.
right, will update. Please resend the series when you find time.
> >> + if (!enable) >> + spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR); >> + >> + ret = spi_nor_write_enable(nor); >> + if (ret) >> + return ret; >> + >> + ret = spi_mem_exec_op(nor->spimem, &op); >> + if (ret) >> + return ret; >> + >> + /* Read back CR2 to make sure the switch was successful. */ >> + op = (struct spi_mem_op) >> + SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_READ_CR2, 1), >> + SPI_MEM_OP_ADDR(4, SPINOR_REG_CR2_MODE_ADDR, 1), >> + SPI_MEM_OP_DUMMY(enable ? 4 : 0, 1), >> + SPI_MEM_OP_DATA_IN(1, nor->bouncebuf, 1)); > > Same as above.
ok
> >> + 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) { >> + if (nor->bouncebuf[0] != SPINOR_REG_CR2_DTR_OPI_ENABLE) { >> + dev_dbg(nor->dev, "Failed to enable 8d-8d-8d mode.\n"); > > Nitpick: s/8d-8d-8d/8D-8D-8D/
sure
> >> + return -EINVAL; >> + } >> + } else if (nor->bouncebuf[0] != SPINOR_REG_CR2_SPI) { >> + dev_dbg(nor->dev, "Failed to disable 8d-8d-8d mode.\n"); > > Nitpick: s/8d-8d-8d/8D-8D-8D/
ok. Thanks! ta
| |