Odp.: [PATCH v4 0/8] mtd: spi-nor: parse SFDP tables to setup (Q)SPI memories

From: Krzeminski, Marcin (Nokia - PL/Wroclaw)
Date: Tue Dec 20 2016 - 22:23:36 EST

Next message: Joe Perches: "Re: [PATCH] indention and space fixes to align with linux style"
Previous message: David Miller: "Re: [PATCH] staging: octeon: Call SET_NETDEV_DEV()"
In reply to: Cyrille Pitchen: "Re: [PATCH v4 0/8] mtd: spi-nor: parse SFDP tables to setup (Q)SPI memories"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> Hi Marcin,
>
> Le 13/12/2016 à 10:46, Krzeminski, Marcin (Nokia - PL/Wroclaw) a écrit :
> >
> >
> >> -----Original Message-----
> >> From: linux-mtd [mailto:linux-mtd-bounces@xxxxxxxxxxxxxxxxxxx] On Behalf
> >> Of Cyrille Pitchen
> >> Sent: Monday, November 21, 2016 3:16 PM
> >> To: computersforpeace@xxxxxxxxx; marek.vasut@xxxxxxxxx;
> >> boris.brezillon@xxxxxxxxxxxxxxxxxx; richard@xxxxxx; linux-
> >> mtd@xxxxxxxxxxxxxxxxxxx
> >> Cc: Cyrille Pitchen <cyrille.pitchen@xxxxxxxxx>; nicolas.ferre@xxxxxxxxx;
> >> linux-kernel@xxxxxxxxxxxxxxx
> >> Subject: [PATCH v4 0/8] mtd: spi-nor: parse SFDP tables to setup (Q)SPI
> >> memories
> >>
> >> Hi all,
> >>
> >> This series extends support of SPI protocols to new protocols such as SPI x-2-
> >> 2 and SPI x-4-4. Also spi_nor_scan() tries now to select the right op codes,
> >> timing parameters (number of mode and dummy cycles) and erase sector
> >> size by parsing the Serial Flash Discoverable Parameter (SFDP) tables, when
> >> available, as defined in the JEDEC JESD216 specifications.
> >>
> >> When SFDP tables are not available, legacy settings are still used for
> >> backward compatibility (SPI and earlier QSPI memories).
> >>
> >> Support of SPI memories >128Mbits is also improved by using the 4byte
> >> address instruction set, when available. Using those dedicated op codes is
> >> stateless as opposed to enter the 4byte address mode, hence a better
> >> compatibility with some boot loaders which expect to use 3byte address op
> >> codes.
> >>
> >>
> >> This series was tested on a Atmel sama5d2 xplained board with the atmel-
> >> qspi.c driver. Except for SST memories, the SPI bus clock was set to 83MHz.
> >> The config MTD_SPI_NOR_USE_4K_SECTORS was selected.
> >>
> >> For my tests, I used some shell scripts using mtd_debug and sha1sum to
> >> check the data integrity.
> >>
> >> e.g:
> >> #!/bin/sh
> >>
> >> mtd_debug erase /dev/mtd5 0 0x780000
> >> mtd_debug write /dev/mtd5 0 7674703 sama5d4_doc11238.pdf mtd_debug
> >> read /dev/mtd5 0 7674703 sama5d4_tmp.pdf
> >>
> >> sha1sum sama5d4_doc11238.pdf sama5d4_tmp.pdf
> >>
> >>
> >> Depending on the actual memory size, I may have used other partitions
> >> (/dev/mtd4) and input file size (2880044 and 320044 bytes).
> >>
> >>
> >> The series was tested with the following QSPI memories:
> >>
> >> Spansion/Cypress:
> >> - s25fl127s OK
> >> - s25fl512s OK
> >> - s25fl164k OK
> >>
> >> Micron:
> >> - n25q128a OK
> >> - n25q512 OK
> >> - n25q01g OK
> >>
> >> Macronix:
> >> - mx25v1635f OK
> >> - mx25l3235f OK
> >> - mx25l3273f OK
> >> - mx25l6433f OK
> >> - mx25l6473f OK
> >> - mx25l12835f OK
> >> - mx25l12845g OK
> >> - mx25l12873g OK
> >> - mx25l25645g OK
> >> - mx25l25673g OK
> >> - mx25l51245g OK
> >> - mx66l1g45g OK
> >>
> >> SST:
> >> - sst26vf016b OK
> >> - sst26vf032b OK
> >> - sst26vf064b OK
> >>
> >>
> >> Best regards,
> >>
> >> Cyrille
> >>
> > Hello,
> >
> > I have tested this series on:
> > mx66u51235f OK
> > mt25qu01g OK(1)
> > s25fs512s FAILED(2)
> >
> > (1) - There is a problem with Die Erase command. There is no 4byte version. I workarounded it by enabling 4byte mode before send, and disabled just after.
> > Resigning from Die Erase is bad idea because of erase speed. Having stateless mode is also bad, so some better idea that my workaround could be nice.
> > (2) - Totally failed because this chip does not support 64KiB sectors, but still reports that command for erasing that kind of sector is available,
> > moreover it is the same as command for erasing 256KiB sector. Solution for this, and for other flash could be that preferred sector size
> > is a parameter for spi-nor framework.
> >
>
> OK, I need to develop another patch adding support with the Sector Map
> Parameter Table, which is optional (1) when all erase sectors share a
> common size but becomes mandatory (2) when those erase sectors have
> different sizes.
>
> Looking at the datasheet of the Cypress S25FS512S, this memory falls into
> case (2), meaning that even without the SFDP patch, the current spi-nor
> framework is not able to implement erase operations properly. Indeed
> spi_nor_erase() assumes an uniform erase sector size.

Yes, I have rewritten version that try to support erase regions.
>
> However this SFDP patch, when combined with the previous patch of the
> series adding support to the SPI 1-4-4 protocol, already fixes another
> issue of the S25FS512S part: this memory doesn't support the Fast Read
> 1-1-4 (6Bh) command at all but only the Fast Read 1-4-4 (EBh).

Indeed, I could removed my patch for that and it still worked :).
>
> I think the support of the Sector Map Parameter Table should be sent in a
> dedicated patch otherwise there would be to many modifications in a single
> patch making it even harder to review.

Yes, without basic JESD216 parsing it is pointless to even start a work on erase region support.
BTW, S25FS512S is tricky, in case when 4KiB sectors are at the beginning you need
to have at least three erase regions:
- 0 - 32KiB (8x4KiB) - erase with 4KiB_ERASE_CMD
- 32KiB - 256KiB - to mark rest of the firs sector, it need to be erased wit Sector Erase CMD.
- rest, erased with Sector Erase CMD.

Thanks,
Marcin

>
> Thanks for your review, it is very helpful and appreciated :)
>
> Best regards,
>
> Cyrille
>
>
> > My tests covers my use case of mtd/spi-nor framework (ubi, ubifs and jffs2 + some regression).
> >
> > Some small comments in patches.
> >
> > Thanks,
> > Marcin
> >
> >> ChangeLog:
> >>
> >> v3 -> v4
> >> - replace dev_info() by dev_dbg() in patch 1.
> >> - split former patch 2 into 2 patches:
> >> + new patch 2 deals with the rename of SPINOR_OP_READ4_* macros
> >> + new patch 3 deals with the alternative methode to support memory
> >>> 16MiB
> >> - add comment in patch 3 to describe the dichotomic search performed by
> >> spi_nor_convert_opcode().
> >> - change return type from int to void for m25p80_proto2nbits() in patch 6.
> >> - remove former patches 8 & 9 from the v2 series: the support of the
> >> Macronix mx66l1g45g memory will be sent in a separated patch.
> >>
> >> v2 -> v3
> >> - tested with new samples: Micron n25q512, n25q01g and Macronix
> >> mx25v1635f, mx25l3235f, mx25l3273f.
> >> - add "Reviewed-by: Jagan Teki <jagan@xxxxxxxxxxxx>" on patch 1.
> >> - add "Tested-by: Vignesh R <vigneshr@xxxxxx>" on patch 2.
> >> - fix some checkpatch warnings.
> >> - add call of spi_nor_wait_till_ready() in spansion_new_quad_enable()
> >> and sr2_bit7_quad_enable(), as suggested by Joel Esponde on patch 6.
> >> - test JESD216 rev A (minor 5) instead of rev B (minor 6) with the return
> >> code of spi_nor_parse_sfdp() from spi_nor_init_params() on patch 6.
> >> The seven additional DWORDs of the Basic Flash Parameter Table were
> >> introduced in rev A, not rev B, so the 15th DWORD was already available
> >> in rev A. The 15th DWORD provides us with the Quad Enable Requirements
> >> (QER) bits.
> >> Basic Flash Parameter Table size:
> >> + JESD216 : 9 DWORDS
> >> + JESD216A: 16 DWORDS
> >> + JESD216B: 16 DWORDS
> >>
> >> v1 -> v2
> >> - fix patch 3 to resolve compiler errors on hisi-sfc.c and cadence-quadspi.c
> >> drivers
> >>
> >>
> >> Cyrille Pitchen (8):
> >> mtd: spi-nor: improve macronix_quad_enable()
> >> mtd: spi-nor: rename SPINOR_OP_* macros of the 4-byte address op codes
> >> mtd: spi-nor: add an alternative method to support memory >16MiB
> >> mtd: spi-nor: add support of SPI protocols like SPI 1-2-2 and SPI
> >> 1-4-4
> >> mtd: spi-nor: remove unused set_quad_mode() function
> >> mtd: m25p80: add support of dual and quad spi protocols to all
> >> commands
> >> mtd: spi-nor: parse Serial Flash Discoverable Parameters (SFDP) tables
> >> mtd: spi-nor: parse SFDP 4-byte Address Instruction Table
> >>
> >> drivers/mtd/devices/m25p80.c | 191 ++++--
> >> drivers/mtd/devices/serial_flash_cmds.h | 7 -
> >> drivers/mtd/devices/st_spi_fsm.c | 28 +-
> >> drivers/mtd/spi-nor/atmel-quadspi.c | 83 ++-
> >> drivers/mtd/spi-nor/cadence-quadspi.c | 18 +-
> >> drivers/mtd/spi-nor/fsl-quadspi.c | 8 +-
> >> drivers/mtd/spi-nor/hisi-sfc.c | 32 +-
> >> drivers/mtd/spi-nor/mtk-quadspi.c | 16 +-
> >> drivers/mtd/spi-nor/nxp-spifi.c | 21 +-
> >> drivers/mtd/spi-nor/spi-nor.c | 1017
> >> ++++++++++++++++++++++++++++---
> >> drivers/spi/spi-bcm-qspi.c | 6 +-
> >> include/linux/mtd/spi-nor.h | 164 ++++-
> >> 12 files changed, 1351 insertions(+), 240 deletions(-)
> >>
> >> --
> >> 2.7.4
> >>
> >>
> >> ______________________________________________________
> >> Linux MTD discussion mailing list
> >> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> >
>

Next message: Joe Perches: "Re: [PATCH] indention and space fixes to align with linux style"
Previous message: David Miller: "Re: [PATCH] staging: octeon: Call SET_NETDEV_DEV()"
In reply to: Cyrille Pitchen: "Re: [PATCH v4 0/8] mtd: spi-nor: parse SFDP tables to setup (Q)SPI memories"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]