| Message ID | 1514441553-27543-21-git-send-email-jagan@amarulasolutions.com |
|---|---|
| State | Rejected |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | dm: Generic MTD Subsystem, with SPI-NOR interface | expand |
Hi Jagan, Le 28/12/2017 à 07:12, Jagan Teki a écrit : > Add 4-byte address supports, so-that SPI-NOR chips > has > 16MiB should accessible. > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > --- > drivers/mtd/spi-nor/m25p80.c | 1 + > drivers/mtd/spi-nor/spi-nor.c | 38 ++++++++++++++++++++++++++++++++++++++ > include/linux/mtd/spi-nor.h | 6 +++++- > 3 files changed, 44 insertions(+), 1 deletion(-) > > diff --git a/drivers/mtd/spi-nor/m25p80.c b/drivers/mtd/spi-nor/m25p80.c > index 5465921..7af6f59 100644 > --- a/drivers/mtd/spi-nor/m25p80.c > +++ b/drivers/mtd/spi-nor/m25p80.c > @@ -35,6 +35,7 @@ static void m25p_addr2cmd(struct spi_nor *nor, unsigned int addr, u8 *cmd) > cmd[1] = addr >> (nor->addr_width * 8 - 8); > cmd[2] = addr >> (nor->addr_width * 8 - 16); > cmd[3] = addr >> (nor->addr_width * 8 - 24); > + cmd[4] = addr >> (nor->addr_width * 8 - 32); > } > > static int m25p_cmdsz(struct spi_nor *nor) > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index 8bf9e67..e0085cf 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -632,6 +632,38 @@ static int stm_unlock(struct udevice *dev, loff_t ofs, uint64_t len) > } > #endif > > +/* Enable/disable 4-byte addressing mode. */ > +static int set_4byte(struct udevice *dev, const struct spi_nor_info *info, > + int enable) > +{ > + struct spi_nor *nor = spi_nor_get_spi_nor_dev(dev); > + const struct spi_nor_ops *ops = spi_nor_get_ops(dev); > + int status; > + bool need_wren = false; > + u8 cmd; > + > + switch (JEDEC_MFR(info)) { > + case SNOR_MFR_MICRON: > + /* Some Micron need WREN command; all will accept it */ > + need_wren = true; > + case SNOR_MFR_MACRONIX: > + case SNOR_MFR_WINBOND: > + if (need_wren) > + write_enable(dev); > + > + cmd = enable ? SNOR_OP_EN4B : SNOR_OP_EX4B; > + status = ops->write_reg(dev, cmd, NULL, 0); > + if (need_wren) > + write_disable(dev); > + > + return status; > + default: > + /* Spansion style */ > + nor->cmd_buf[0] = enable << 7; > + return ops->write_reg(dev, SNOR_OP_BRWR, nor->cmd_buf, 1); > + } > +} > + > #ifdef CONFIG_SPI_NOR_MACRONIX > static int macronix_quad_enable(struct udevice *dev) > { > @@ -873,6 +905,12 @@ int spi_nor_scan(struct spi_nor *nor) > } > > nor->addr_width = 3; > + if (mtd->size > SNOR_16MB_BOUN) { > + nor->addr_width = 4; > + ret = set_4byte(nor->dev, info, true); > + if (ret) > + goto err; > + } > This is a bad idea: you make the SPI NOR memory enter its 4-byte address mode, which is statefull. Then, once in Linux for instance, the memory would still be in its 4-byte address mode but expected to be in its reset state, hence in 3-byte address mode. At this point, this might not be an issue yet because spi-nor under linux is likely to use the 4-byte address instruction set if available, which is stateless by the way, so whatever the memory is in its 3-byte or in 4-byte address mode, the op codes of the 4-byte address instruction set are always expected to be followed by a 4-byte address. So Linux won't notice that the SPI NOR memory is in its 4-byte mode but not in its 3-byte mode as expected. However, if a spurious reboot occurs at the CPU side after the SPI NOR memory has entered its 4-byte address mode, either still in u-boot or already in linux or whatever running code, the SPI NOR memory, likely not reset on its side, would still be in its 4-byte address mode, whereas many earlier boot-loaders would expect the memory to be in it's reset state, ie in its 3-byte address mode. Hence those boot-loaders, running before u-boot, would not be able to read data (load u-boot) from the SPI NOR memory: the boot would fail! Examples of those early boot-loaders are the ROM Codes of many Atmel SAMA5Dx SoCs but I'm pretty sure this issue also applies to other manufacturers based on patches I've seen on the linux-mtd mailing list to add the SPI_NOR_4B_OPCODES flag to some memory parts: https://patchwork.ozlabs.org/patch/750305/ Think about a watchdog resetting the CPU for instance. TL;DR You should avoid making the SPI NOR memory enter its statefull 4-byte address mode but consider using the stateless 4-byte address instruction set instead, when possible. Almost all recent SPI NOR memories with sizes > 16MiB support the 4-byte address mode. Best regards, Cyrille > /* Dummy cycles for read */ > switch (nor->read_opcode) { > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h > index e1688e2..fc4a649 100644 > --- a/include/linux/mtd/spi-nor.h > +++ b/include/linux/mtd/spi-nor.h > @@ -63,6 +63,10 @@ > #define SNOR_OP_BP 0x02 /* Byte program */ > #define SNOR_OP_AAI_WP 0xad /* Auto addr increment word program */ > > +/* Used for Macronix and Winbond flashes. */ > +#define SNOR_OP_EN4B 0xb7 /* Enter 4-byte mode */ > +#define SNOR_OP_EX4B 0xe9 /* Exit 4-byte mode */ > + > /* Status Register bits. */ > #define SR_WIP BIT(0) /* Write in progress */ > #define SR_WEL BIT(1) /* Write enable latch */ > @@ -84,7 +88,7 @@ > /* Flash timeout values */ > #define SNOR_READY_WAIT_PROG (2 * CONFIG_SYS_HZ) > #define SNOR_READY_WAIT_ERASE (5 * CONFIG_SYS_HZ) > -#define SNOR_MAX_CMD_SIZE 4 > +#define SNOR_MAX_CMD_SIZE 6 > #define SNOR_16MB_BOUN 0x1000000 > > /** >
Le 28/12/2017 à 09:06, Cyrille Pitchen a écrit : > Hi Jagan, > > Le 28/12/2017 à 07:12, Jagan Teki a écrit : >> Add 4-byte address supports, so-that SPI-NOR chips >> has > 16MiB should accessible. >> >> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> >> --- >> drivers/mtd/spi-nor/m25p80.c | 1 + >> drivers/mtd/spi-nor/spi-nor.c | 38 ++++++++++++++++++++++++++++++++++++++ >> include/linux/mtd/spi-nor.h | 6 +++++- >> 3 files changed, 44 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mtd/spi-nor/m25p80.c b/drivers/mtd/spi-nor/m25p80.c >> index 5465921..7af6f59 100644 >> --- a/drivers/mtd/spi-nor/m25p80.c >> +++ b/drivers/mtd/spi-nor/m25p80.c >> @@ -35,6 +35,7 @@ static void m25p_addr2cmd(struct spi_nor *nor, unsigned int addr, u8 *cmd) >> cmd[1] = addr >> (nor->addr_width * 8 - 8); >> cmd[2] = addr >> (nor->addr_width * 8 - 16); >> cmd[3] = addr >> (nor->addr_width * 8 - 24); >> + cmd[4] = addr >> (nor->addr_width * 8 - 32); >> } >> >> static int m25p_cmdsz(struct spi_nor *nor) >> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c >> index 8bf9e67..e0085cf 100644 >> --- a/drivers/mtd/spi-nor/spi-nor.c >> +++ b/drivers/mtd/spi-nor/spi-nor.c >> @@ -632,6 +632,38 @@ static int stm_unlock(struct udevice *dev, loff_t ofs, uint64_t len) >> } >> #endif >> >> +/* Enable/disable 4-byte addressing mode. */ >> +static int set_4byte(struct udevice *dev, const struct spi_nor_info *info, >> + int enable) >> +{ >> + struct spi_nor *nor = spi_nor_get_spi_nor_dev(dev); >> + const struct spi_nor_ops *ops = spi_nor_get_ops(dev); >> + int status; >> + bool need_wren = false; >> + u8 cmd; >> + >> + switch (JEDEC_MFR(info)) { >> + case SNOR_MFR_MICRON: >> + /* Some Micron need WREN command; all will accept it */ >> + need_wren = true; >> + case SNOR_MFR_MACRONIX: >> + case SNOR_MFR_WINBOND: >> + if (need_wren) >> + write_enable(dev); >> + >> + cmd = enable ? SNOR_OP_EN4B : SNOR_OP_EX4B; >> + status = ops->write_reg(dev, cmd, NULL, 0); >> + if (need_wren) >> + write_disable(dev); >> + >> + return status; >> + default: >> + /* Spansion style */ >> + nor->cmd_buf[0] = enable << 7; >> + return ops->write_reg(dev, SNOR_OP_BRWR, nor->cmd_buf, 1); >> + } >> +} >> + >> #ifdef CONFIG_SPI_NOR_MACRONIX >> static int macronix_quad_enable(struct udevice *dev) >> { >> @@ -873,6 +905,12 @@ int spi_nor_scan(struct spi_nor *nor) >> } >> >> nor->addr_width = 3; >> + if (mtd->size > SNOR_16MB_BOUN) { >> + nor->addr_width = 4; >> + ret = set_4byte(nor->dev, info, true); >> + if (ret) >> + goto err; >> + } >> > > This is a bad idea: you make the SPI NOR memory enter its 4-byte address > mode, which is statefull. Then, once in Linux for instance, the memory > would still be in its 4-byte address mode but expected to be in its reset > state, hence in 3-byte address mode. > > At this point, this might not be an issue yet because spi-nor under linux > is likely to use the 4-byte address instruction set if available, which is > stateless by the way, so whatever the memory is in its 3-byte or in 4-byte > address mode, the op codes of the 4-byte address instruction set are always > expected to be followed by a 4-byte address. So Linux won't notice that the > SPI NOR memory is in its 4-byte mode but not in its 3-byte mode as expected. > > However, if a spurious reboot occurs at the CPU side after the SPI NOR > memory has entered its 4-byte address mode, either still in u-boot or > already in linux or whatever running code, the SPI NOR memory, likely not > reset on its side, would still be in its 4-byte address mode, whereas many > earlier boot-loaders would expect the memory to be in it's reset state, ie > in its 3-byte address mode. Hence those boot-loaders, running before u-boot, > would not be able to read data (load u-boot) from the SPI NOR memory: > the boot would fail! > > Examples of those early boot-loaders are the ROM Codes of many Atmel > SAMA5Dx SoCs but I'm pretty sure this issue also applies to other > manufacturers based on patches I've seen on the linux-mtd mailing list to > add the SPI_NOR_4B_OPCODES flag to some memory parts: > https://patchwork.ozlabs.org/patch/750305/ > > Think about a watchdog resetting the CPU for instance. > > > TL;DR > You should avoid making the SPI NOR memory enter its statefull 4-byte > address mode but consider using the stateless 4-byte address instruction > set instead, when possible. > > Almost all recent SPI NOR memories with sizes > 16MiB support the 4-byte > address mode. I meant the 4-byte address instruction set ;) > > Best regards, > > Cyrille > >> /* Dummy cycles for read */ >> switch (nor->read_opcode) { >> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h >> index e1688e2..fc4a649 100644 >> --- a/include/linux/mtd/spi-nor.h >> +++ b/include/linux/mtd/spi-nor.h >> @@ -63,6 +63,10 @@ >> #define SNOR_OP_BP 0x02 /* Byte program */ >> #define SNOR_OP_AAI_WP 0xad /* Auto addr increment word program */ >> >> +/* Used for Macronix and Winbond flashes. */ >> +#define SNOR_OP_EN4B 0xb7 /* Enter 4-byte mode */ >> +#define SNOR_OP_EX4B 0xe9 /* Exit 4-byte mode */ >> + >> /* Status Register bits. */ >> #define SR_WIP BIT(0) /* Write in progress */ >> #define SR_WEL BIT(1) /* Write enable latch */ >> @@ -84,7 +88,7 @@ >> /* Flash timeout values */ >> #define SNOR_READY_WAIT_PROG (2 * CONFIG_SYS_HZ) >> #define SNOR_READY_WAIT_ERASE (5 * CONFIG_SYS_HZ) >> -#define SNOR_MAX_CMD_SIZE 4 >> +#define SNOR_MAX_CMD_SIZE 6 >> #define SNOR_16MB_BOUN 0x1000000 >> >> /** >> >
diff --git a/drivers/mtd/spi-nor/m25p80.c b/drivers/mtd/spi-nor/m25p80.c index 5465921..7af6f59 100644 --- a/drivers/mtd/spi-nor/m25p80.c +++ b/drivers/mtd/spi-nor/m25p80.c @@ -35,6 +35,7 @@ static void m25p_addr2cmd(struct spi_nor *nor, unsigned int addr, u8 *cmd) cmd[1] = addr >> (nor->addr_width * 8 - 8); cmd[2] = addr >> (nor->addr_width * 8 - 16); cmd[3] = addr >> (nor->addr_width * 8 - 24); + cmd[4] = addr >> (nor->addr_width * 8 - 32); } static int m25p_cmdsz(struct spi_nor *nor) diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index 8bf9e67..e0085cf 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -632,6 +632,38 @@ static int stm_unlock(struct udevice *dev, loff_t ofs, uint64_t len) } #endif +/* Enable/disable 4-byte addressing mode. */ +static int set_4byte(struct udevice *dev, const struct spi_nor_info *info, + int enable) +{ + struct spi_nor *nor = spi_nor_get_spi_nor_dev(dev); + const struct spi_nor_ops *ops = spi_nor_get_ops(dev); + int status; + bool need_wren = false; + u8 cmd; + + switch (JEDEC_MFR(info)) { + case SNOR_MFR_MICRON: + /* Some Micron need WREN command; all will accept it */ + need_wren = true; + case SNOR_MFR_MACRONIX: + case SNOR_MFR_WINBOND: + if (need_wren) + write_enable(dev); + + cmd = enable ? SNOR_OP_EN4B : SNOR_OP_EX4B; + status = ops->write_reg(dev, cmd, NULL, 0); + if (need_wren) + write_disable(dev); + + return status; + default: + /* Spansion style */ + nor->cmd_buf[0] = enable << 7; + return ops->write_reg(dev, SNOR_OP_BRWR, nor->cmd_buf, 1); + } +} + #ifdef CONFIG_SPI_NOR_MACRONIX static int macronix_quad_enable(struct udevice *dev) { @@ -873,6 +905,12 @@ int spi_nor_scan(struct spi_nor *nor) } nor->addr_width = 3; + if (mtd->size > SNOR_16MB_BOUN) { + nor->addr_width = 4; + ret = set_4byte(nor->dev, info, true); + if (ret) + goto err; + } /* Dummy cycles for read */ switch (nor->read_opcode) { diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index e1688e2..fc4a649 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -63,6 +63,10 @@ #define SNOR_OP_BP 0x02 /* Byte program */ #define SNOR_OP_AAI_WP 0xad /* Auto addr increment word program */ +/* Used for Macronix and Winbond flashes. */ +#define SNOR_OP_EN4B 0xb7 /* Enter 4-byte mode */ +#define SNOR_OP_EX4B 0xe9 /* Exit 4-byte mode */ + /* Status Register bits. */ #define SR_WIP BIT(0) /* Write in progress */ #define SR_WEL BIT(1) /* Write enable latch */ @@ -84,7 +88,7 @@ /* Flash timeout values */ #define SNOR_READY_WAIT_PROG (2 * CONFIG_SYS_HZ) #define SNOR_READY_WAIT_ERASE (5 * CONFIG_SYS_HZ) -#define SNOR_MAX_CMD_SIZE 4 +#define SNOR_MAX_CMD_SIZE 6 #define SNOR_16MB_BOUN 0x1000000 /**
Add 4-byte address supports, so-that SPI-NOR chips has > 16MiB should accessible. Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> --- drivers/mtd/spi-nor/m25p80.c | 1 + drivers/mtd/spi-nor/spi-nor.c | 38 ++++++++++++++++++++++++++++++++++++++ include/linux/mtd/spi-nor.h | 6 +++++- 3 files changed, 44 insertions(+), 1 deletion(-)