Re: [PATCH v13 00/15] mtd: spi-nor: add xSPI Octal DTR support
On 30/09/20 09:57AM, tudor.amba...@microchip.com wrote: > On 9/16/20 3:44 PM, Pratyush Yadav wrote: > > Hi, > > Hello, > > > > > This series adds support for Octal DTR flashes in the SPI NOR framework, > > and then adds hooks for the Cypress Semper and Micron Xcella flashes to > > allow running them in Octal DTR mode. This series assumes that the flash > > is handed to the kernel in Legacy SPI mode. > > > > I like this series. There are some comments that can be addressed, no big > deal though. > > I think that we shouldn't let the door open for users with flashes that > enter in a X-X-X mode in a non-volatile way. Think of two flashes that have > the same X-X-X mode enable sequence, but in which only the EN bit differs: > for one the EN bit is volatile and for the other it is non-volatile. Users > of the later flash that try to enable the X-X-X mode (using our code) will > end up with the flash in a mode from which they can't recover. Thus my advice > is to consider by default all the flashes, as X-X-X mode non-volatile flashes, > and to not let them use the X-X-X mode enable methods. Flashes that can enter > X-X-X modes in a volatile way, should discover this capability by parsing the > optional SFDP SCCR Map. Those that don't define this table, should pass this > capability as a flash_info flag when declaring the flash. With these, users > should be conscious about the V or NV modes, and the risk to end up with > flashes for which there is no software to recover is diminished. This is what > I tried in RFC 1/3 and RFC 3/3. I think those 2 patches should be part of > this series. 14/15 and 15/15 should be updated accordingly. RFC 2/3 has room > for discussions because it provides access system-wise, while ideally would be > to do it at flash granularity. I'll wait for your feedback on those. FWIW, I took a quick look at the RFC patches and I agree that patches 1/3 and 3/3 are good and I'll add them to my series. I don't like 2/3 mainly because of the same concerns that Vignesh has: wearing down the NV bits. Also IMO we should avoid touching NV bits as much as we can because I had used some on the Cypress flash during development, and I ended up "bricking" the flash a few times (for example when I used the wrong dummy cycles by mistake or the controller driver did something wrong). Since I knew exactly which registers I was touching I could recover it but the process is quite painful. > > > Tested on TI J721E EVM with 1-bit ECC on the Cypress flash. > > As a tip, when introducing some big changes to the core, would be nice to be > assured that things that worked previously are still working now. > An erase-write-read-back test in Quad SPI would suffice. Probably you have > already did that, but I haven't seen it mentioned. Ok, I'll include them next time. I tested this series on the CQSPI and TI QSPI controllers on MT25Q, S25FL for regressions, and on MT35 and S28 for Octal DTR. -- Regards, Pratyush Yadav Texas Instruments India
Re: [PATCH v13 00/15] mtd: spi-nor: add xSPI Octal DTR support
On 9/16/20 3:44 PM, Pratyush Yadav wrote: > Hi, Hello, > > This series adds support for Octal DTR flashes in the SPI NOR framework, > and then adds hooks for the Cypress Semper and Micron Xcella flashes to > allow running them in Octal DTR mode. This series assumes that the flash > is handed to the kernel in Legacy SPI mode. > I like this series. There are some comments that can be addressed, no big deal though. I think that we shouldn't let the door open for users with flashes that enter in a X-X-X mode in a non-volatile way. Think of two flashes that have the same X-X-X mode enable sequence, but in which only the EN bit differs: for one the EN bit is volatile and for the other it is non-volatile. Users of the later flash that try to enable the X-X-X mode (using our code) will end up with the flash in a mode from which they can't recover. Thus my advice is to consider by default all the flashes, as X-X-X mode non-volatile flashes, and to not let them use the X-X-X mode enable methods. Flashes that can enter X-X-X modes in a volatile way, should discover this capability by parsing the optional SFDP SCCR Map. Those that don't define this table, should pass this capability as a flash_info flag when declaring the flash. With these, users should be conscious about the V or NV modes, and the risk to end up with flashes for which there is no software to recover is diminished. This is what I tried in RFC 1/3 and RFC 3/3. I think those 2 patches should be part of this series. 14/15 and 15/15 should be updated accordingly. RFC 2/3 has room for discussions because it provides access system-wise, while ideally would be to do it at flash granularity. I'll wait for your feedback on those. > Tested on TI J721E EVM with 1-bit ECC on the Cypress flash. As a tip, when introducing some big changes to the core, would be nice to be assured that things that worked previously are still working now. An erase-write-read-back test in Quad SPI would suffice. Probably you have already did that, but I haven't seen it mentioned. Cheers, ta
[PATCH v13 00/15] mtd: spi-nor: add xSPI Octal DTR support
Hi,
This series adds support for Octal DTR flashes in the SPI NOR framework,
and then adds hooks for the Cypress Semper and Micron Xcella flashes to
allow running them in Octal DTR mode. This series assumes that the flash
is handed to the kernel in Legacy SPI mode.
Tested on TI J721E EVM with 1-bit ECC on the Cypress flash.
Changes in v13:
- Do not use multiple assignments in spi_nor_spimem_setup_op().
- Use EOPNOTSUPP instead of ENOTSUPP.
- Fix unbalanced braces around else statements.
- Fix whitespace alignment for spi_nor_set_read_settings() prototype.
Changes in v12:
- Rebase on latest master.
- Set dummy and data nbytes to 1 in spi_nor_spimem_check_readop() before
calling spi_nor_spimem_check_op() to make sure the buswidth for them
is properly set up. Similarly, set data nbytes to 1 for
spi_nor_spimem_check_pp(). This makes sure we don't send the wrong
dummy and data buswidth to the controller's supports_op().
- Enable DQS for Micron MT35XU512ABA. No reason not to.
- Update doc comment for spi_nor_parse_profile1() and
spi_nor_cypress_octal_dtr_enable() to add missing fields.
Changes in v11:
- Add helpers spi_nor_{read,write}_reg() to make it easier to reject DTR
ops for them.
- Add helper for spi_nor_controller_ops_erase() to make it easier to
reject DTR ops.
- s/spi-mem/SPIMEM/ and s/spi-nor/SPI NOR/
- Avoid enabling 4-byte addressing mode for all DTR ops instead of just
Octal DTR ops. This is based on the assumption that DTR ops can only
use 4-byte addressing.
- Use round_up() instead of defining ROUND_UP_TO().
- Rename 'table' to 'dwords' in xSPI Profile 1.0 parsing.
- Re-order Profile 1.0 related defines by DWORD order.
- Move rdsr parameter parsing to where opcode is parsed because it is
from the same DWORD.
- Drop local variables addr_width and dummy in spi_nor_read_sr()
spi_nor_read_fsr().
- Allow soft reset for all protocols, not just 8D-8D-8D.
- Do not set address value to 0 is spi_nor_read_sr() and
spi_nor_read_fsr(). It is 0 by default when we create the op.
- Do not make having command opcode extension as a reserved field fatal.
- Introduce the flag SPI_NOR_OCTAL_DTR_PP to indicate 8D page program
support since it can't be detected from SFDP.
- Move the call to spi_nor_spimem_setup_op() in spi_nor_read_sr() to the
commit which introduces it.
- Convert a comment in Profile 1.0 parsing from multi-line to one line.
Changes in v10:
- Rebase on latest linux-next/master. Drop a couple patches that made it
in the previous release.
- Move the code that sets 20 dummy cycles for MT35XU512ABA to its octal
enable function. This way, if the controller doesn't support 8D mode
20 dummy cycles won't be used.
Changes in v9:
- Do not use '& 0xff' to get the opcode LSB in spi-mxic and
spi-zynq-qspi. The cast to u8 will do that anyway.
- Do not use if (opcode) as a check for whether the command phase exists
in spi-zynq-qspi because the opcode 0 can be valid. Use the new
cmd.nbytes instead.
Changes in v8:
- Move controller changes in spi-mxic to the commit which introduces
2-byte opcodes to avoid problems when bisecting.
- Replace usage of sizeof(op->cmd.opcode) with op->cmd.nbytes.
- Extract opcode in spi-zynq-qspi instead of using >cmd.opcode.
Changes in v7:
- Reject ops with more than 1 command byte in
spi_mem_default_supports_op().
- Reject ops with more than 1 command byte in atmel and mtk controllers.
- Reject ops with 0 command bytes in spi_mem_check_op().
- Set cmd.nbytes to 1 when using SPI_MEM_OP_CMD().
- Avoid endianness problems in spi-mxic.
Changes in v6:
- Instead of hard-coding 8D-8D-8D Fast Read dummy cycles to 20, find
them out from the Profile 1.0 table.
Changes in v5:
- Do not enable stateful X-X-X modes if the reset line is broken.
- Instead of setting SNOR_READ_HWCAPS_8_8_8_DTR from Profile 1.0 table
parsing, do it in spi_nor_info_init_params() instead based on the
SPI_NOR_OCTAL_DTR_READ flag instead.
- Set SNOR_HWCAPS_PP_8_8_8_DTR in s28hs post_sfdp hook since this
capability is no longer set in Profile 1.0 parsing.
- Instead of just checking for spi_nor_get_protocol_width() in
spi_nor_octal_dtr_enable(), make sure the protocol is
SNOR_PROTO_8_8_8_DTR since get_protocol_width() only cares about data
width.
- Drop flag SPI_NOR_SOFT_RESET. Instead, discover soft reset capability
via BFPT.
- Do not make an invalid Quad Enable BFPT field a fatal error. Silently
ignore it by assuming no quad enable bit is present.
- Set dummy cycles for Cypress Semper flash to 24 instead of 20. This
allows for 200MHz operation in 8D mode compared to the 166MHz with 20.
- Rename spi_nor_cypress_octal_enable() to
spi_nor_cypress_octal_dtr_enable().
- Update spi-mtk-nor.c to reject DTR ops since it doesn't call
spi_mem_default_supports_op().
Changes in v4:
- Refactor the series to use the new spi-nor framework with the
manufacturer-specific bits separated from the core.
- Add support for Micron
