Message-ID: <AM4PR0701MB2130E92644C8C4A359715F36FE900@AM4PR0701MB2130.eurprd07.prod.outlook.com>
Date:   Tue, 20 Dec 2016 19:51:21 +0000
From:   "Krzeminski, Marcin (Nokia - PL/Wroclaw)" 
        <marcin.krzeminski@...ia.com>
To:     Cyrille Pitchen <cyrille.pitchen@...el.com>,
        "computersforpeace@...il.com" <computersforpeace@...il.com>,
        "marek.vasut@...il.com" <marek.vasut@...il.com>,
        "boris.brezillon@...e-electrons.com" 
        <boris.brezillon@...e-electrons.com>,
        "richard@....at" <richard@....at>,
        "linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>
CC:     "nicolas.ferre@...el.com" <nicolas.ferre@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "mar.krzeminski@...il.com" <mar.krzeminski@...il.com>
Subject: Odp.: [PATCH v4 0/8] mtd: spi-nor: parse SFDP tables to setup (Q)SPI
 memories


> Hi Marcin,
>
> Le 13/12/2016 à 10:46, Krzeminski, Marcin (Nokia - PL/Wroclaw) a écrit :
> > 
> > 
> >> -----Original Message-----
> >> From: linux-mtd [mailto:linux-mtd-bounces@...ts.infradead.org] On Behalf
> >> Of Cyrille Pitchen
> >> Sent: Monday, November 21, 2016 3:16 PM
> >> To: computersforpeace@...il.com; marek.vasut@...il.com;
> >> boris.brezillon@...e-electrons.com; richard@....at; linux-
> >> mtd@...ts.infradead.org
> >> Cc: Cyrille Pitchen <cyrille.pitchen@...el.com>; nicolas.ferre@...el.com;
> >> linux-kernel@...r.kernel.org
> >> 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@...nedev.com>" on patch 1.
> >> - add "Tested-by: Vignesh R <vigneshr@...com>" 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/
> > 
>

Powered by blists - more mailing lists