Re: [PATCH v16 00/15] mtd: spi-nor: add xSPI Octal DTR support
On Mon, 5 Oct 2020 21:01:23 +0530, Pratyush Yadav wrote:
> 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 Micron MT35X and S28HS flashes for Octal DTR. Tested on Micron
> MT25Q, and Cypress S25FL for regressions. All flashes were tested by
> running a read/write stress test on top of UBIFS. On the Cypress S28HS
> flash 1-bit ECC had to be used to allow UBIFS to work since partial page
> writes don't work with 2-bit ECC.
>
> [...]
With comments on 14/15 addressed:
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git
spi-nor/next, thanks!
[01/15] mtd: spi-nor: core: use EOPNOTSUPP instead of ENOTSUPP
https://git.kernel.org/mtd/c/39bdfb789b
[02/15] mtd: spi-nor: add spi_nor_controller_ops_{read_reg, write_reg, erase}()
https://git.kernel.org/mtd/c/6e1bf55d72
[03/15] mtd: spi-nor: add support for DTR protocol
https://git.kernel.org/mtd/c/0e30f47232
[04/15] mtd: spi-nor: sfdp: get command opcode extension type from BFPT
https://git.kernel.org/mtd/c/0e1b2fc4e5
[05/15] mtd: spi-nor: sfdp: parse xSPI Profile 1.0 table
https://git.kernel.org/mtd/c/fb27f19897
[06/15] mtd: spi-nor: core: use dummy cycle and address width info from SFDP
https://git.kernel.org/mtd/c/6c6a2b2b8e
[07/15] mtd: spi-nor: core: do 2 byte reads for SR and FSR in DTR mode
https://git.kernel.org/mtd/c/354b412967
[08/15] mtd: spi-nor: Introduce SNOR_F_IO_MODE_EN_VOLATILE
https://git.kernel.org/mtd/c/c6908077b1
[09/15] mtd: spi-nor: Parse SFDP SCCR Map
https://git.kernel.org/mtd/c/981a8d60e0
[10/15] mtd: spi-nor: core: enable octal DTR mode when possible
https://git.kernel.org/mtd/c/a33c89db4c
[11/15] mtd: spi-nor: sfdp: detect Soft Reset sequence support from BFPT
https://git.kernel.org/mtd/c/1131324aa5
[12/15] mtd: spi-nor: core: perform a Soft Reset on shutdown
https://git.kernel.org/mtd/c/d73ee7534c
[13/15] mtd: spi-nor: core: disable Octal DTR mode on suspend.
https://git.kernel.org/mtd/c/1b65c43f70
[14/15] mtd: spi-nor: spansion: add support for Cypress Semper flash
https://git.kernel.org/mtd/c/c3266af101
[15/15] mtd: spi-nor: micron-st: allow using MT35XU512ABA in Octal DTR mode
https://git.kernel.org/mtd/c/ad624dfd7b
--
Regards
Vignesh
Re: [PATCH v16 00/15] mtd: spi-nor: add xSPI Octal DTR support
Hi Tudor, On 28/10/20 07:53AM, tudor.amba...@microchip.com wrote: > Hi, Pratyush, > > On 10/5/20 6:31 PM, Pratyush Yadav wrote: > > Tested on Micron MT35X and S28HS flashes for Octal DTR. > > Do these flashes define the "Command Sequences to Change to > Octal DDR (8D-8D-8D) mode" table? Can't we use that table > instead of defining our own octal dtr enable functions? The Micron flash does not have this table. The Cypress flash does. The problem is that one of the samples of the Cypress flash I tested on had incorrect data in that table which meant the sequence would fail. The newer samples of the flash have the correct data. I don't know how many of those faulty flashes are out there in the wild. IMO, to be on the safe side spi_nor_cypress_octal_dtr_enable() needs to be implemented. So from the point of view of this series there is no need to parse the Octal DDR enable table. > I see that Mason used this table for a macronix flash: > https://patchwork.ozlabs.org/project/linux-mtd/patch/1590737775-4798-4-git-send-email-masonccy...@mxic.com.tw/ > https://patchwork.ozlabs.org/project/linux-mtd/patch/1590737775-4798-8-git-send-email-masonccy...@mxic.com.tw/ > > Cheers, > ta -- Regards, Pratyush Yadav Texas Instruments India
Re: [PATCH v16 00/15] mtd: spi-nor: add xSPI Octal DTR support
On 10/28/20 8:51 PM, tudor.amba...@microchip.com wrote: > On 10/28/20 2:49 PM, Pratyush Yadav wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the >> content is safe >> >> Hi Tudor, >> >> On 28/10/20 07:53AM, tudor.amba...@microchip.com wrote: >>> Hi, Pratyush, >>> >>> On 10/5/20 6:31 PM, Pratyush Yadav wrote: Tested on Micron MT35X and S28HS flashes for Octal DTR. >>> >>> Do these flashes define the "Command Sequences to Change to >>> Octal DDR (8D-8D-8D) mode" table? Can't we use that table >>> instead of defining our own octal dtr enable functions? >> >> The Micron flash does not have this table. The Cypress flash does. The >> problem is that one of the samples of the Cypress flash I tested on had >> incorrect data in that table which meant the sequence would fail. The >> newer samples of the flash have the correct data. > > Can we differentiate the Cypress flashes? Do you remember what > was the incorrect data? > >> >> I don't know how many of those faulty flashes are out there in the wild. >> IMO, to be on the safe side spi_nor_cypress_octal_dtr_enable() needs to >> be implemented. So from the point of view of this series there is no >> need to parse the Octal DDR enable table. > > Meh, we cover manufacturer's mistakes. On the long run, our aim should be > to follow the SFDP standard and if a flash implements it wrong, to either > fix it via a fixup hook (if the fix is minimal), or to skip the faulty > table. > > Regarding "Command Sequences to Change to Octal DDR (8D-8D-8D) mode" > table. Have you looked over > https://patchwork.ozlabs.org/project/linux-mtd/patch/1590737775-4798-4-git-send-email-masonccy...@mxic.com.tw/ > ? > Is there a standard way to determine the offsets of opcode, addr and > data in the cmd seq? > There is no standard way of doing this and I recommend against it. Each cmd seq has 0 to 7 Bytes. So the sequence maybe: 1 cmd byte-3 addr bytes- 3 data bytes or 1 cmd byte-0 address bytes-6 data bytes We could just assume first byte to be command and rest to be data bytes always, but one problem maybe that controller may not support data length to be so long when address phase is absent. Eg.: Cadence OSPI controller supports only upto 8 bytes length transfers in absence of address phase but other controllers may limit the length further? One more drawback of using "Command Sequences to Change to Octal DDR (8D-8D-8D) mode" table is that it not only switches flash to 8D mode but also configures flash to be in: - 50 ohm I/O driver strength (Driver Type 0, mandatory for xSPI devices) - 20 dummy cycles for Read Fast commands - Operation at 100MHz (or higher, if supported) Note that 20 dummy cycles may not be enough for flashes to operate at 166/200MHz or higher and thus requiring flash specific fixups. So, I am beginning to doubt if parsing "Command Sequences to Change to Octal DDR (8D-8D-8D) mode" table is worth it. Not to mention we still need flash specific code to "verify" that mode switch is indeed successful. Regards Vignesh
Re: [PATCH v16 00/15] mtd: spi-nor: add xSPI Octal DTR support
Hi, Pratyush, On 10/5/20 6:31 PM, Pratyush Yadav wrote: > Tested on Micron MT35X and S28HS flashes for Octal DTR. Do these flashes define the "Command Sequences to Change to Octal DDR (8D-8D-8D) mode" table? Can't we use that table instead of defining our own octal dtr enable functions? I see that Mason used this table for a macronix flash: https://patchwork.ozlabs.org/project/linux-mtd/patch/1590737775-4798-4-git-send-email-masonccy...@mxic.com.tw/ https://patchwork.ozlabs.org/project/linux-mtd/patch/1590737775-4798-8-git-send-email-masonccy...@mxic.com.tw/ Cheers, ta
Re: [PATCH v16 00/15] mtd: spi-nor: add xSPI Octal DTR support
On 28/10/20 03:21PM, tudor.amba...@microchip.com wrote: > On 10/28/20 2:49 PM, Pratyush Yadav wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > > content is safe > > > > Hi Tudor, > > > > On 28/10/20 07:53AM, tudor.amba...@microchip.com wrote: > >> Hi, Pratyush, > >> > >> On 10/5/20 6:31 PM, Pratyush Yadav wrote: > >>> Tested on Micron MT35X and S28HS flashes for Octal DTR. > >> > >> Do these flashes define the "Command Sequences to Change to > >> Octal DDR (8D-8D-8D) mode" table? Can't we use that table > >> instead of defining our own octal dtr enable functions? > > > > The Micron flash does not have this table. The Cypress flash does. The > > problem is that one of the samples of the Cypress flash I tested on had > > incorrect data in that table which meant the sequence would fail. The > > newer samples of the flash have the correct data. > > Can we differentiate the Cypress flashes? No way I know of. > Do you remember what was the incorrect data? The address width for the write register command was 4 bytes when the flash uses 3 bytes by default. > > > > I don't know how many of those faulty flashes are out there in the wild. > > IMO, to be on the safe side spi_nor_cypress_octal_dtr_enable() needs to > > be implemented. So from the point of view of this series there is no > > need to parse the Octal DDR enable table. > > Meh, we cover manufacturer's mistakes. On the long run, our aim should be > to follow the SFDP standard and if a flash implements it wrong, to either > fix it via a fixup hook (if the fix is minimal), or to skip the faulty > table. > > Regarding "Command Sequences to Change to Octal DDR (8D-8D-8D) mode" > table. Have you looked over > https://patchwork.ozlabs.org/project/linux-mtd/patch/1590737775-4798-4-git-send-email-masonccy...@mxic.com.tw/ > ? > Is there a standard way to determine the offsets of opcode, addr and > data in the cmd seq? To be honest the spec does not say much about how the data should be interpreted so I am not sure either. My understanding is that those are effectively data bytes to send out on the bus. One way to implement such a function, would be to put the first byte as the command opcode and the rest as data [0], with no address and dummy cycles. So no matter the address length, the controller should send out the bytes in sequence and then the flash can interpret them according to the address width it expects. The downside is that someone debugging this on the controller's end might get confused seeing an opcode that expects an address phase but SPI NOR not sending one. The other way would be to use the first byte as opcode, the next nor->addr_width bytes as address and the remaining as data, with no dummy cycles. This would fail if the 8D enable command does not use nor->addr_width address bytes [1]. I don't know which of the two is better but I think both are better than the switch-case hackery in Mason's patch which has to assume either the address width or the data length and leaves no way to play around with them in fixup hooks. If you have any better ideas I'm all ears. [0] AFAIK many controllers can't have 0 command bytes. [1] I'm not sure how common that would be though. > Cheers, > ta > > > >> I see that Mason used this table for a macronix flash: > >> https://patchwork.ozlabs.org/project/linux-mtd/patch/1590737775-4798-4-git-send-email-masonccy...@mxic.com.tw/ > >> https://patchwork.ozlabs.org/project/linux-mtd/patch/1590737775-4798-8-git-send-email-masonccy...@mxic.com.tw/ > >> > >> Cheers, > >> ta > > > > -- > > Regards, > > Pratyush Yadav > > Texas Instruments India > > > -- Regards, Pratyush Yadav Texas Instruments India
Re: [PATCH v16 00/15] mtd: spi-nor: add xSPI Octal DTR support
On 10/28/20 2:49 PM, Pratyush Yadav wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > Hi Tudor, > > On 28/10/20 07:53AM, tudor.amba...@microchip.com wrote: >> Hi, Pratyush, >> >> On 10/5/20 6:31 PM, Pratyush Yadav wrote: >>> Tested on Micron MT35X and S28HS flashes for Octal DTR. >> >> Do these flashes define the "Command Sequences to Change to >> Octal DDR (8D-8D-8D) mode" table? Can't we use that table >> instead of defining our own octal dtr enable functions? > > The Micron flash does not have this table. The Cypress flash does. The > problem is that one of the samples of the Cypress flash I tested on had > incorrect data in that table which meant the sequence would fail. The > newer samples of the flash have the correct data. Can we differentiate the Cypress flashes? Do you remember what was the incorrect data? > > I don't know how many of those faulty flashes are out there in the wild. > IMO, to be on the safe side spi_nor_cypress_octal_dtr_enable() needs to > be implemented. So from the point of view of this series there is no > need to parse the Octal DDR enable table. Meh, we cover manufacturer's mistakes. On the long run, our aim should be to follow the SFDP standard and if a flash implements it wrong, to either fix it via a fixup hook (if the fix is minimal), or to skip the faulty table. Regarding "Command Sequences to Change to Octal DDR (8D-8D-8D) mode" table. Have you looked over https://patchwork.ozlabs.org/project/linux-mtd/patch/1590737775-4798-4-git-send-email-masonccy...@mxic.com.tw/ ? Is there a standard way to determine the offsets of opcode, addr and data in the cmd seq? Cheers, ta > >> I see that Mason used this table for a macronix flash: >> https://patchwork.ozlabs.org/project/linux-mtd/patch/1590737775-4798-4-git-send-email-masonccy...@mxic.com.tw/ >> https://patchwork.ozlabs.org/project/linux-mtd/patch/1590737775-4798-8-git-send-email-masonccy...@mxic.com.tw/ >> >> Cheers, >> ta > > -- > Regards, > Pratyush Yadav > Texas Instruments India >
[PATCH v16 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 Micron MT35X and S28HS flashes for Octal DTR. Tested on Micron
MT25Q, and Cypress S25FL for regressions. All flashes were tested by
running a read/write stress test on top of UBIFS. On the Cypress S28HS
flash 1-bit ECC had to be used to allow UBIFS to work since partial page
writes don't work with 2-bit ECC.
Changes in v16:
- Do not wait for Micron flash to switch to 8D mode. The switch is
instant according to the datasheet.
- Read flash ID in both Micron and Cypress Octal enable hooks as a
confirmation that the switch to 8D mode was successful. This way we
can catch any errors early rather than later commands failing.
- Add Tudor's Reviewed-by on patch 10/15.
Changes in v15:
- Give precedence to addr_width found via SFDP over forcing it to 4 for
8D. The standard knows better.
- Sleep for a range of 1000 to 1500 us instead of 400 to 600. The 400 to
600 range was too close to the actual time it took to change to 8D
mode (discovered by polling SR right after and observing that it
freezed the controller sometimes). Bump it to 1000 - 1500 to be safe.
- Do not initialize dummy to 0 in Profile 1.0 parsing.
- Drop the variable io_mode_en_volatile in spi_nor_parse_sccr(). Use the
FIELD_GET expression in the if statement directly.
- Drop the debug message when setting dummy cycle configuration failed
for S28HS flash.
- Move the patches that introduce SNOR_F_IO_MODE_EN_VOLATILE to before
the one that introduces spi_nor_octal_dtr_enable(). This way we can
reject flashes with non-volatile configuration from day 0.
Changes in v14:
- Rename spi_nor_{read,write}_reg() to
spi_nor_controller_ops_{read,write}_reg().
- When spi_nor_spimem_setup_op() will be called after a spi_mem_op is
initialized, set buswidth of all phases to 0. This will make the
reader question where the buswidth is actually set and avoid any
confusions.
- Only use address and dummy bytes from Profile 1.0 table for 8D-8D-8D
Read SR/FSR instead of all DTR ones.
- Do not make spi_nor_default_setup_op() public. It is not used anymore
in latest iterations so this is not needed.
- Only enable Octal DTR mode when the configuration to enable it is
volatile.
- Do not prevent modes other than 8D-8D-8D from enabling 4-byte
addressing. All other modes don't automatically imply 4-byte
addressing.
- Add some blank lines in spi_nor_soft_reset().
- Drop the local variable 'dev' in spi_nor_suspend().
- Do not force 4-byte addressing on all DTR modes. Only force it for
Octal DTR because only in that case 3-byte addresses are not allowed.
- Drop variable addr_width from spi_nor_micron_octal_dtr_enable() and
spi_nor_cypress_octal_dtr_enable().
- Remove print from spi_nor_micron_octal_dtr_enable() and
spi_nor_cypress_octal_dtr_enable() when enabling/disabling Octal DTR
fails.
- Wait some time after enabling Octal DTR mode in
spi_nor_micron_octal_dtr_enable() and
spi_nor_cypress_octal_dtr_enable(). This makes sure the mode is
enabled and flash is usuable.
- Fix alignment of .fixups in micron_parts and spansion_parts.
- s/BFPT_DWORD16_SOFT_RST/BFPT_DWORD16_SWRST_EN_RST
- Fix copy-paste leftover in spi_nor_parse_profile1() documentation.
- Do not assume a default dummy cycle value if none is found in Profile
1.0 parsing. Leave it to 0 and let flashes fix it up via fixup hooks.
- Parse the SCCR table to discover if the Octal DTR enable bit is
volatile or not.
- Drop variable addr_width in s28hs512t_post_bfpt_fixup() and move op
initialization to the declaration to avoid a cast and use
nor->bouncebuf directly instead of declaring a local variable.
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
