* Re: [PATCH v6 00/15] A SPI NAND framework under generic NAND framework [not found] <74cb9a07bd3247fd86002ef97509828f@SIWEX4H.sing.micron.com> @ 2017-05-31 6:20 ` Boris Brezillon 2017-05-31 6:34 ` Peter Pan 潘栋 (peterpandong) 0 siblings, 1 reply; 32+ messages in thread From: Boris Brezillon @ 2017-05-31 6:20 UTC (permalink / raw) To: Peter Pan 潘栋 (peterpandong) Cc: arnaud.mouiche@gmail.com, richard@nod.at, computersforpeace@gmail.com, thomas.petazzoni@free-electrons.com, marex@denx.de, cyrille.pitchen@wedev4u.fr, linux-mtd@lists.infradead.org, peterpansjtu@gmail.com, linshunquan1@hisilicon.com Le Wed, 31 May 2017 01:12:16 +0000, Peter Pan 潘栋 (peterpandong) <peterpandong@micron.com> a écrit : > Hi Boris, > > On Thu, May 30, 2017 at 5:00 AM, Boris Brezillon > <boris.brezillon@free-electrons.com> wrote: > > Hi Peter, > > > > On Wed, 24 May 2017 15:06:56 +0800 > > Peter Pan <peterpandong@micron.com> wrote: > > > > > First of all, thank Boris and Marek for your priceless comments > > > on v5 and thank everyone reviewed and tested on my previous series. > > > I can never be here without your help. This series comes to v6 > > > and it becomes much better with your help. > > > > > > SPI NAND is a new NAND family device with SPI protocol as > > > its interface. And its command set is totally different > > > with parallel NAND. > > > > > > Our first attempt to SPI NAND was more than 2 years ago[1]. > > > At that time, I didn't make BBT shareable and there were > > > too many duplicate code with parallel NAND, so that serie > > > stoped. But the discussion never stops. Now Boris has a plan > > > to make a generic NAND framework which can be shared with > > > both parallel and SPI NAND. Now the first step of the > > > new generic NAND framework is finished. And it is waiting > > > for a user. After discussion with Boris. We both think it's > > > time to rebuild SPI NAND framework based on the new NAND > > > framework and send out for reviewing. > > > > > > This series includes two part. The first part (patch 1 to 9) > > > is a new generic NAND framework from Boris Brezillon, which > > > is from Biris's nand/generic branch[2]. The second part > > > (patch 10 to 15) introductes a SPI NAND framework based on > > > the new generic NAND framework. > > > > > > This series only supports basic SPI NAND features and uses > > > generic spi controller for data transfer. ECC support is removed > > > since it's not in a good structure and more important, it should > > > be shared between different NAND devices, which means it should > > > be in new NAND core. Support different types of ECC and advanced > > > SPI NAND features is the next step. > > > > > > This series is based on nand/next branch and is tested on > > > Xilinx Zedboard with Micron MT29F2G01ABAGDSF SPI NAND chip. > > > > As you can see, I started to review this v6, but I don't expect you to > > send a new version (at least not immediately). > > > > Here is the plan: I'll finish reviewing the series, and while I'm > > reviewing I'll try to address my own comments. Once the review/changes > > are done, I'll push a nand/spi branch to the github repo [1] and ask > > you and Arnaud to review/test the whole thing. If you're happy with > > my changes (it should only be minor changes to your > > initial implementation) I'll ask you to send a v7. I'll then wait for > > 4.13-rc1 to be out and apply everything to my nand/next branch (I > > expect a few conflicting changes in the nand/mtd area caused by the > > migration to the new doc format, that's why I'd like to wait one more > > cycle). > > > > If everything goes well, we should be good for 4.14, and the patches > > will have spent enough time in linux-next to discover obvious bugs. > > > > Let me know if you have a problem with this approach. > > You plan is great, I'm OK with it. Looking forward to merge SPI NAND > to main line :) I'm still expecting replies to my review(s) to know if the changes I plan to do are correct ;-). ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 00/15] A SPI NAND framework under generic NAND framework 2017-05-31 6:20 ` [PATCH v6 00/15] A SPI NAND framework under generic NAND framework Boris Brezillon @ 2017-05-31 6:34 ` Peter Pan 潘栋 (peterpandong) 0 siblings, 0 replies; 32+ messages in thread From: Peter Pan 潘栋 (peterpandong) @ 2017-05-31 6:34 UTC (permalink / raw) To: Boris Brezillon Cc: arnaud.mouiche@gmail.com, richard@nod.at, computersforpeace@gmail.com, thomas.petazzoni@free-electrons.com, marex@denx.de, cyrille.pitchen@wedev4u.fr, linux-mtd@lists.infradead.org, peterpansjtu@gmail.com, linshunquan1@hisilicon.com Hi Boris, > On 31 May 2017, at 14:20, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > > Le Wed, 31 May 2017 01:12:16 +0000, > Peter Pan 潘栋 (peterpandong) <peterpandong@micron.com> a écrit : > >> Hi Boris, >> >> On Thu, May 30, 2017 at 5:00 AM, Boris Brezillon >> <boris.brezillon@free-electrons.com> wrote: >>> Hi Peter, >>> >>> On Wed, 24 May 2017 15:06:56 +0800 >>> Peter Pan <peterpandong@micron.com> wrote: >>> >>>> First of all, thank Boris and Marek for your priceless comments >>>> on v5 and thank everyone reviewed and tested on my previous series. >>>> I can never be here without your help. This series comes to v6 >>>> and it becomes much better with your help. >>>> >>>> SPI NAND is a new NAND family device with SPI protocol as >>>> its interface. And its command set is totally different >>>> with parallel NAND. >>>> >>>> Our first attempt to SPI NAND was more than 2 years ago[1]. >>>> At that time, I didn't make BBT shareable and there were >>>> too many duplicate code with parallel NAND, so that serie >>>> stoped. But the discussion never stops. Now Boris has a plan >>>> to make a generic NAND framework which can be shared with >>>> both parallel and SPI NAND. Now the first step of the >>>> new generic NAND framework is finished. And it is waiting >>>> for a user. After discussion with Boris. We both think it's >>>> time to rebuild SPI NAND framework based on the new NAND >>>> framework and send out for reviewing. >>>> >>>> This series includes two part. The first part (patch 1 to 9) >>>> is a new generic NAND framework from Boris Brezillon, which >>>> is from Biris's nand/generic branch[2]. The second part >>>> (patch 10 to 15) introductes a SPI NAND framework based on >>>> the new generic NAND framework. >>>> >>>> This series only supports basic SPI NAND features and uses >>>> generic spi controller for data transfer. ECC support is removed >>>> since it's not in a good structure and more important, it should >>>> be shared between different NAND devices, which means it should >>>> be in new NAND core. Support different types of ECC and advanced >>>> SPI NAND features is the next step. >>>> >>>> This series is based on nand/next branch and is tested on >>>> Xilinx Zedboard with Micron MT29F2G01ABAGDSF SPI NAND chip. >>> >>> As you can see, I started to review this v6, but I don't expect you to >>> send a new version (at least not immediately). >>> >>> Here is the plan: I'll finish reviewing the series, and while I'm >>> reviewing I'll try to address my own comments. Once the review/changes >>> are done, I'll push a nand/spi branch to the github repo [1] and ask >>> you and Arnaud to review/test the whole thing. If you're happy with >>> my changes (it should only be minor changes to your >>> initial implementation) I'll ask you to send a v7. I'll then wait for >>> 4.13-rc1 to be out and apply everything to my nand/next branch (I >>> expect a few conflicting changes in the nand/mtd area caused by the >>> migration to the new doc format, that's why I'd like to wait one more >>> cycle). >>> >>> If everything goes well, we should be good for 4.14, and the patches >>> will have spent enough time in linux-next to discover obvious bugs. >>> >>> Let me know if you have a problem with this approach. >> >> You plan is great, I'm OK with it. Looking forward to merge SPI NAND >> to main line :) > > I'm still expecting replies to my review(s) to know if the changes I > plan to do are correct ;-). Actually I'm looking at your comments right now. Something was wrong with my mailbox. Hope to give you feedback today or tomorrow. Thanks Peter Pan ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v6 00/15] A SPI NAND framework under generic NAND framework
@ 2017-05-24 7:06 Peter Pan
2017-05-29 20:59 ` Boris Brezillon
0 siblings, 1 reply; 32+ messages in thread
From: Peter Pan @ 2017-05-24 7:06 UTC (permalink / raw)
To: boris.brezillon, richard, computersforpeace, arnaud.mouiche,
thomas.petazzoni, marex, cyrille.pitchen, linux-mtd
Cc: peterpandong, peterpansjtu, linshunquan1
First of all, thank Boris and Marek for your priceless comments
on v5 and thank everyone reviewed and tested on my previous series.
I can never be here without your help. This series comes to v6
and it becomes much better with your help.
SPI NAND is a new NAND family device with SPI protocol as
its interface. And its command set is totally different
with parallel NAND.
Our first attempt to SPI NAND was more than 2 years ago[1].
At that time, I didn't make BBT shareable and there were
too many duplicate code with parallel NAND, so that serie
stoped. But the discussion never stops. Now Boris has a plan
to make a generic NAND framework which can be shared with
both parallel and SPI NAND. Now the first step of the
new generic NAND framework is finished. And it is waiting
for a user. After discussion with Boris. We both think it's
time to rebuild SPI NAND framework based on the new NAND
framework and send out for reviewing.
This series includes two part. The first part (patch 1 to 9)
is a new generic NAND framework from Boris Brezillon, which
is from Biris's nand/generic branch[2]. The second part
(patch 10 to 15) introductes a SPI NAND framework based on
the new generic NAND framework.
This series only supports basic SPI NAND features and uses
generic spi controller for data transfer. ECC support is removed
since it's not in a good structure and more important, it should
be shared between different NAND devices, which means it should
be in new NAND core. Support different types of ECC and advanced
SPI NAND features is the next step.
This series is based on nand/next branch and is tested on
Xilinx Zedboard with Micron MT29F2G01ABAGDSF SPI NAND chip.
v6 changes:
- includes generic NAND framework patches in series
- rebase on nand/next (commit 6076fd1e9d879521f7082a5e22185b71e480b777)
- remove on-die ECC support
- remove devm_free() since everything allocated by devm_kmalloc() will be
automatically freed when device is released
- add comment header for structs in spinand.h
- remove spinand_register()/unregister(), call spinand_detect() in
spinand_init() and only expose spinand_init()/cleanup()
- add nand_release_bbt() in bbt.c and use it in nand_cleanup() and
spinand_cleanup()
- use BIT(n) instead (1 << n) in macro of spinand.h
- rename spinand_alloc() to devm_spinand_alloc()
- name lables in better way
- fix some typos
- add empty lines between code blocks
v5 changes:
- rebase patch on nand/next with Boris's generic NAND framework patches[3]
- replace pr_xxx() with dev_xxx()
- replace kzalloc()i/kfree() with devm_kzalloc()/devm_kfree()
- rename spinand_op_init() to spinand_init_op() for consistency
- remove command opcode in function comments
- use BIT(n) instead (1 << n) in macro
- remove manufactures.c and put spinand_manufacturers table in core.c
- change spinand_write_reg() u8 *buf argument to u8 value,
since the length is always 1
- remove spinand_manufacture->detect() check, since it is always != NULL
- alloc spinand_ecc_engine struct in vendor.c when using on-die ECC
(for hardware ECC, it should be in controllers/*.c)
- add comment header for struct spinand_op
- fix timeout bug in spinand_wait(), thanks for Arnaud's debug
- make spinand_manufacturers const
- add ecc_engine_ops pointer in struct micron_spinand_info
- make controller->cap assignment right with SPI_TX/RX_QUAD/DUAL flag
v4 changes:
- initialize struct mtd_oob_ops to 0 in bbt.c
- rename new added helper in nand.h to nand_check_xxxx()
- add struct mtd_oob_ops consistency check in nand_check_oob_ops()
- add dataleft in struct nand_page_iter instead of offs
- remove spinand_manufacturers->ops->detect() check since it is mandatory
- remove spinand_set_manufacturer_ops() and do the job in
spinand_manufacturer_detect()
- move .priv out of struct spinand_controller
- add spinand_alloc/free/register/unregister() and make
spinand_detect/init() static
- make BBT be configured by device tree
- chip->id.data stores raw ID directly
- refine device info print message after detect success
- add struct mtd_layout_ops pointer in struct micron_spinand_info
- remove micron_spinand_init() and do its job in micron_spinand_detect()
- fix BBT block cannot be erased bug
v3 changes:
- rebase patch on 4.11-rc1[2]
- change read ID method. read 4 bytes ID out then let ->detect() of each
manufacutre driver to decode ID and detect the device.
- make SPI NAND id table private to each manufacutre driver
- fix coding style to make checkpatch.pl happy
- update the MAINTAINERS file for spi nand code
- add nand_size() helper in nand.h
- use nand_for_each_page() helper in spinand_do_read/write_ops()
- create helper to check boundaries in generic NAND code and use it
in SPI NAND core
- rename spinand_base.c to core.c
- manufactures' drivers expose spinand_manufacturer struct instead of
spinand_manufacturer_ops struct to keep Manufacture ID macro in
manufactures' drivers and rename spinand_ids.c to manufacture.c
- rename spinand_micron.c to micron.c
- rename chips/ directory to controllers/
- rename generic_spi.c to generic-spi.c
- replace ->build_column_addr() and ->get_dummy() hooks with ->prepare_op() in
spinand_manufacturer_ops struct
- rename __spinand_erase() to spinand_erase()
- rename spinand_erase() to spinand_erase_skip_bbt()
- rename spinand_scan_ident() to spinand_detect()
- rename spinand_scan_tail() to spinand_init()
- move non detect related code from spinand_detect() to spinand_init()
- remove spinand_fill_nandd, assign nand->ops in spinand_detect()
- merge v2 patch 3(bad block support) and patch 4(BBT support)
- drop getchip parameter, remove spinand_get/remove_device(), take the lock
by caller directly
- fix function comment headers
- use nand_bbt_is_initialized() helper
- replace spinand_ecc_engine and spinand_controller object in spinand_device
struct with pointer
- replace struct spinand_manufacturer_ops pointer in spinand_device struct
with spinand_manufacturer struct
v2 changes:
- replace "spi_nand" with "spinand".
- rename spi nand related structs for better understanding.
- introduce spi nand controller, manufacturer and ecc_engine struct.
- add spi nand manufacturer initialization function refer to Boris's
manuf-init branch.
- remove NAND_SKIP_BBTSCAN from series. Add it later when enabling HW ECC.
- reorganize series according to Boris's suggestion.
[1]http://lists.infradead.org/pipermail/linux-mtd/2015-January/057223.html
[2]https://github.com/bbrezillon/linux-0day/tree/nand/generic
[3]https://github.com/peterpansjtu/linux/tree/nand/spinand
Boris Brezillon (8):
mtd: nand: Rename nand.h into rawnand.h
mtd: nand: move raw NAND related code to the raw/ subdir
mtd: nand: add a nand.h file to expose basic NAND stuff
mtd: nand: raw: prefix conflicting names with nandcchip instead of
nand
mtd: nand: raw: create struct rawnand_device
mtd: nand: raw: make BBT code more generic
mtd: nand: move BBT code to drivers/mtd/nand/
mtd: nand: Add the page iterator concept
Peter Pan (7):
mtd: nand: make sure mtd_oob_ops consistent in bbt
nand: spi: add basic blocks for infrastructure
nand: spi: add basic operations support
nand: spi: Add bad block support
nand: spi: add Micron spi nand support
nand: spi: Add generic SPI controller support
MAINTAINERS: Add SPI NAND entry
Documentation/DocBook/mtdnand.tmpl | 12 +-
MAINTAINERS | 29 +-
arch/arm/mach-davinci/board-da850-evm.c | 2 +-
arch/arm/mach-davinci/board-dm355-evm.c | 2 +-
arch/arm/mach-davinci/board-dm355-leopard.c | 2 +-
arch/arm/mach-davinci/board-dm365-evm.c | 2 +-
arch/arm/mach-davinci/board-dm644x-evm.c | 2 +-
arch/arm/mach-davinci/board-dm646x-evm.c | 2 +-
arch/arm/mach-davinci/board-sffsdr.c | 2 +-
arch/arm/mach-dove/dove-db-setup.c | 2 +-
arch/arm/mach-ep93xx/snappercl15.c | 6 +-
arch/arm/mach-ep93xx/ts72xx.c | 6 +-
arch/arm/mach-imx/mach-qong.c | 4 +-
arch/arm/mach-ixp4xx/ixdp425-setup.c | 4 +-
arch/arm/mach-mmp/aspenite.c | 2 +-
arch/arm/mach-omap1/board-fsample.c | 2 +-
arch/arm/mach-omap1/board-h2.c | 2 +-
arch/arm/mach-omap1/board-h3.c | 2 +-
arch/arm/mach-omap1/board-nand.c | 4 +-
arch/arm/mach-omap1/board-perseus2.c | 2 +-
arch/arm/mach-orion5x/db88f5281-setup.c | 2 +-
arch/arm/mach-orion5x/kurobox_pro-setup.c | 2 +-
arch/arm/mach-orion5x/ts209-setup.c | 2 +-
arch/arm/mach-orion5x/ts78xx-setup.c | 8 +-
arch/arm/mach-pxa/balloon3.c | 4 +-
arch/arm/mach-pxa/em-x270.c | 4 +-
arch/arm/mach-pxa/eseries.c | 2 +-
arch/arm/mach-pxa/palmtx.c | 4 +-
arch/arm/mach-pxa/tosa.c | 2 +-
arch/arm/mach-s3c24xx/common-smdk.c | 2 +-
arch/arm/mach-s3c24xx/mach-anubis.c | 2 +-
arch/arm/mach-s3c24xx/mach-at2440evb.c | 2 +-
arch/arm/mach-s3c24xx/mach-bast.c | 2 +-
arch/arm/mach-s3c24xx/mach-gta02.c | 2 +-
arch/arm/mach-s3c24xx/mach-jive.c | 2 +-
arch/arm/mach-s3c24xx/mach-mini2440.c | 2 +-
arch/arm/mach-s3c24xx/mach-osiris.c | 2 +-
arch/arm/mach-s3c24xx/mach-qt2410.c | 2 +-
arch/arm/mach-s3c24xx/mach-rx3715.c | 2 +-
arch/arm/mach-s3c24xx/mach-vstms.c | 2 +-
arch/blackfin/mach-bf537/boards/dnp5370.c | 2 +-
arch/blackfin/mach-bf537/boards/stamp.c | 4 +-
arch/blackfin/mach-bf561/boards/acvilon.c | 4 +-
arch/cris/arch-v32/drivers/mach-a3/nandflash.c | 6 +-
arch/cris/arch-v32/drivers/mach-fs/nandflash.c | 6 +-
arch/mips/alchemy/devboards/db1200.c | 4 +-
arch/mips/alchemy/devboards/db1300.c | 4 +-
arch/mips/alchemy/devboards/db1550.c | 4 +-
arch/mips/include/asm/mach-jz4740/jz4740_nand.h | 2 +-
arch/mips/jz4740/board-qi_lb60.c | 2 +-
arch/mips/netlogic/xlr/platform-flash.c | 2 +-
arch/mips/pnx833x/common/platform.c | 4 +-
arch/mips/rb532/devices.c | 4 +-
arch/sh/boards/mach-migor/setup.c | 4 +-
drivers/mtd/inftlcore.c | 2 +-
drivers/mtd/nand/Kconfig | 566 +------
drivers/mtd/nand/Makefile | 71 +-
drivers/mtd/nand/{nand_bbt.c => bbt.c} | 697 ++++----
drivers/mtd/nand/raw/Kconfig | 566 +++++++
drivers/mtd/nand/raw/Makefile | 69 +
drivers/mtd/nand/{ => raw}/ams-delta.c | 10 +-
drivers/mtd/nand/{ => raw}/atmel/Makefile | 0
drivers/mtd/nand/{ => raw}/atmel/nand-controller.c | 52 +-
drivers/mtd/nand/{ => raw}/atmel/pmecc.c | 2 +-
drivers/mtd/nand/{ => raw}/atmel/pmecc.h | 0
drivers/mtd/nand/{ => raw}/au1550nd.c | 28 +-
drivers/mtd/nand/{ => raw}/bcm47xxnflash/Makefile | 0
.../nand/{ => raw}/bcm47xxnflash/bcm47xxnflash.h | 2 +-
drivers/mtd/nand/{ => raw}/bcm47xxnflash/main.c | 4 +-
.../mtd/nand/{ => raw}/bcm47xxnflash/ops_bcm4706.c | 18 +-
drivers/mtd/nand/{ => raw}/bf5xx_nand.c | 34 +-
drivers/mtd/nand/{ => raw}/brcmnand/Makefile | 0
.../mtd/nand/{ => raw}/brcmnand/bcm63138_nand.c | 0
drivers/mtd/nand/{ => raw}/brcmnand/bcm6368_nand.c | 0
drivers/mtd/nand/{ => raw}/brcmnand/brcmnand.c | 34 +-
drivers/mtd/nand/{ => raw}/brcmnand/brcmnand.h | 0
drivers/mtd/nand/{ => raw}/brcmnand/brcmstb_nand.c | 0
drivers/mtd/nand/{ => raw}/brcmnand/iproc_nand.c | 0
drivers/mtd/nand/{ => raw}/cafe_nand.c | 26 +-
drivers/mtd/nand/{ => raw}/cmx270_nand.c | 14 +-
drivers/mtd/nand/{ => raw}/cs553x_nand.c | 22 +-
drivers/mtd/nand/{ => raw}/davinci_nand.c | 16 +-
drivers/mtd/nand/{ => raw}/denali.c | 14 +-
drivers/mtd/nand/{ => raw}/denali.h | 2 +-
drivers/mtd/nand/{ => raw}/denali_dt.c | 0
drivers/mtd/nand/{ => raw}/denali_pci.c | 0
drivers/mtd/nand/{ => raw}/diskonchip.c | 70 +-
drivers/mtd/nand/{ => raw}/docg4.c | 45 +-
drivers/mtd/nand/{ => raw}/fsl_elbc_nand.c | 28 +-
drivers/mtd/nand/{ => raw}/fsl_ifc_nand.c | 30 +-
drivers/mtd/nand/{ => raw}/fsl_upm.c | 14 +-
drivers/mtd/nand/{ => raw}/fsmc_nand.c | 26 +-
drivers/mtd/nand/{ => raw}/gpio.c | 8 +-
drivers/mtd/nand/{ => raw}/gpmi-nand/Makefile | 0
drivers/mtd/nand/{ => raw}/gpmi-nand/bch-regs.h | 0
drivers/mtd/nand/{ => raw}/gpmi-nand/gpmi-lib.c | 2 +-
drivers/mtd/nand/{ => raw}/gpmi-nand/gpmi-nand.c | 38 +-
drivers/mtd/nand/{ => raw}/gpmi-nand/gpmi-nand.h | 2 +-
drivers/mtd/nand/{ => raw}/gpmi-nand/gpmi-regs.h | 0
drivers/mtd/nand/{ => raw}/hisi504_nand.c | 26 +-
drivers/mtd/nand/{ => raw}/jz4740_nand.c | 14 +-
drivers/mtd/nand/{ => raw}/jz4780_bch.c | 0
drivers/mtd/nand/{ => raw}/jz4780_bch.h | 0
drivers/mtd/nand/{ => raw}/jz4780_nand.c | 10 +-
drivers/mtd/nand/{ => raw}/lpc32xx_mlc.c | 18 +-
drivers/mtd/nand/{ => raw}/lpc32xx_slc.c | 22 +-
drivers/mtd/nand/{ => raw}/mpc5121_nfc.c | 28 +-
drivers/mtd/nand/{ => raw}/mtk_ecc.c | 0
drivers/mtd/nand/{ => raw}/mtk_ecc.h | 0
drivers/mtd/nand/{ => raw}/mtk_nand.c | 40 +-
drivers/mtd/nand/{ => raw}/mxc_nand.c | 54 +-
drivers/mtd/nand/{ => raw}/nand_amd.c | 4 +-
drivers/mtd/nand/{ => raw}/nand_base.c | 272 +++-
drivers/mtd/nand/{ => raw}/nand_bch.c | 8 +-
drivers/mtd/nand/{ => raw}/nand_ecc.c | 6 +-
drivers/mtd/nand/{ => raw}/nand_hynix.c | 12 +-
drivers/mtd/nand/{ => raw}/nand_ids.c | 2 +-
drivers/mtd/nand/{ => raw}/nand_macronix.c | 2 +-
drivers/mtd/nand/{ => raw}/nand_micron.c | 12 +-
drivers/mtd/nand/{ => raw}/nand_samsung.c | 6 +-
drivers/mtd/nand/{ => raw}/nand_timings.c | 2 +-
drivers/mtd/nand/{ => raw}/nand_toshiba.c | 4 +-
drivers/mtd/nand/{ => raw}/nandsim.c | 24 +-
drivers/mtd/nand/{ => raw}/ndfc.c | 20 +-
drivers/mtd/nand/{ => raw}/nuc900_nand.c | 10 +-
drivers/mtd/nand/{ => raw}/omap2.c | 22 +-
drivers/mtd/nand/{ => raw}/omap_elm.c | 0
drivers/mtd/nand/{ => raw}/orion_nand.c | 10 +-
drivers/mtd/nand/{ => raw}/oxnas_nand.c | 14 +-
drivers/mtd/nand/{ => raw}/pasemi_nand.c | 12 +-
drivers/mtd/nand/{ => raw}/plat_nand.c | 6 +-
drivers/mtd/nand/{ => raw}/pxa3xx_nand.c | 38 +-
drivers/mtd/nand/{ => raw}/qcom_nandc.c | 30 +-
drivers/mtd/nand/{ => raw}/r852.c | 8 +-
drivers/mtd/nand/{ => raw}/r852.h | 2 +-
drivers/mtd/nand/{ => raw}/s3c2410.c | 18 +-
drivers/mtd/nand/{ => raw}/sh_flctl.c | 12 +-
drivers/mtd/nand/{ => raw}/sharpsl.c | 10 +-
drivers/mtd/nand/{ => raw}/sm_common.c | 4 +-
drivers/mtd/nand/{ => raw}/sm_common.h | 0
drivers/mtd/nand/{ => raw}/socrates_nand.c | 14 +-
drivers/mtd/nand/{ => raw}/sunxi_nand.c | 60 +-
drivers/mtd/nand/{ => raw}/tango_nand.c | 32 +-
drivers/mtd/nand/{ => raw}/tmio_nand.c | 10 +-
drivers/mtd/nand/{ => raw}/txx9ndfmc.c | 16 +-
drivers/mtd/nand/{ => raw}/vf610_nfc.c | 6 +-
drivers/mtd/nand/{ => raw}/xway_nand.c | 12 +-
drivers/mtd/nand/spi/Kconfig | 7 +
drivers/mtd/nand/spi/Makefile | 3 +
drivers/mtd/nand/spi/controllers/Kconfig | 5 +
drivers/mtd/nand/spi/controllers/Makefile | 1 +
drivers/mtd/nand/spi/controllers/generic-spi.c | 162 ++
drivers/mtd/nand/spi/core.c | 1381 ++++++++++++++++
drivers/mtd/nand/spi/micron.c | 157 ++
drivers/mtd/nftlcore.c | 2 +-
drivers/mtd/nftlmount.c | 2 +-
drivers/mtd/sm_ftl.c | 2 +-
drivers/mtd/ssfdc.c | 2 +-
drivers/mtd/tests/nandbiterrs.c | 2 +-
drivers/staging/mt29f_spinand/mt29f_spinand.c | 8 +-
fs/jffs2/wbuf.c | 2 +-
include/linux/mtd/nand-gpio.h | 2 +-
include/linux/mtd/nand.h | 1667 +++++++-------------
include/linux/mtd/rawnand.h | 1286 +++++++++++++++
include/linux/mtd/sh_flctl.h | 4 +-
include/linux/mtd/sharpsl.h | 2 +-
include/linux/mtd/spinand.h | 272 ++++
include/linux/platform_data/atmel.h | 2 +-
include/linux/platform_data/mtd-davinci.h | 2 +-
include/linux/platform_data/mtd-nand-s3c2410.h | 2 +-
170 files changed, 5708 insertions(+), 2906 deletions(-)
rename drivers/mtd/nand/{nand_bbt.c => bbt.c} (69%)
create mode 100644 drivers/mtd/nand/raw/Kconfig
create mode 100644 drivers/mtd/nand/raw/Makefile
rename drivers/mtd/nand/{ => raw}/ams-delta.c (97%)
rename drivers/mtd/nand/{ => raw}/atmel/Makefile (100%)
rename drivers/mtd/nand/{ => raw}/atmel/nand-controller.c (97%)
rename drivers/mtd/nand/{ => raw}/atmel/pmecc.c (99%)
rename drivers/mtd/nand/{ => raw}/atmel/pmecc.h (100%)
rename drivers/mtd/nand/{ => raw}/au1550nd.c (95%)
rename drivers/mtd/nand/{ => raw}/bcm47xxnflash/Makefile (100%)
rename drivers/mtd/nand/{ => raw}/bcm47xxnflash/bcm47xxnflash.h (92%)
rename drivers/mtd/nand/{ => raw}/bcm47xxnflash/main.c (95%)
rename drivers/mtd/nand/{ => raw}/bcm47xxnflash/ops_bcm4706.c (96%)
rename drivers/mtd/nand/{ => raw}/bf5xx_nand.c (95%)
rename drivers/mtd/nand/{ => raw}/brcmnand/Makefile (100%)
rename drivers/mtd/nand/{ => raw}/brcmnand/bcm63138_nand.c (100%)
rename drivers/mtd/nand/{ => raw}/brcmnand/bcm6368_nand.c (100%)
rename drivers/mtd/nand/{ => raw}/brcmnand/brcmnand.c (98%)
rename drivers/mtd/nand/{ => raw}/brcmnand/brcmnand.h (100%)
rename drivers/mtd/nand/{ => raw}/brcmnand/brcmstb_nand.c (100%)
rename drivers/mtd/nand/{ => raw}/brcmnand/iproc_nand.c (100%)
rename drivers/mtd/nand/{ => raw}/cafe_nand.c (97%)
rename drivers/mtd/nand/{ => raw}/cmx270_nand.c (94%)
rename drivers/mtd/nand/{ => raw}/cs553x_nand.c (95%)
rename drivers/mtd/nand/{ => raw}/davinci_nand.c (98%)
rename drivers/mtd/nand/{ => raw}/denali.c (99%)
rename drivers/mtd/nand/{ => raw}/denali.h (99%)
rename drivers/mtd/nand/{ => raw}/denali_dt.c (100%)
rename drivers/mtd/nand/{ => raw}/denali_pci.c (100%)
rename drivers/mtd/nand/{ => raw}/diskonchip.c (96%)
rename drivers/mtd/nand/{ => raw}/docg4.c (97%)
rename drivers/mtd/nand/{ => raw}/fsl_elbc_nand.c (97%)
rename drivers/mtd/nand/{ => raw}/fsl_ifc_nand.c (97%)
rename drivers/mtd/nand/{ => raw}/fsl_upm.c (95%)
rename drivers/mtd/nand/{ => raw}/fsmc_nand.c (97%)
rename drivers/mtd/nand/{ => raw}/gpio.c (97%)
rename drivers/mtd/nand/{ => raw}/gpmi-nand/Makefile (100%)
rename drivers/mtd/nand/{ => raw}/gpmi-nand/bch-regs.h (100%)
rename drivers/mtd/nand/{ => raw}/gpmi-nand/gpmi-lib.c (99%)
rename drivers/mtd/nand/{ => raw}/gpmi-nand/gpmi-nand.c (98%)
rename drivers/mtd/nand/{ => raw}/gpmi-nand/gpmi-nand.h (99%)
rename drivers/mtd/nand/{ => raw}/gpmi-nand/gpmi-regs.h (100%)
rename drivers/mtd/nand/{ => raw}/hisi504_nand.c (97%)
rename drivers/mtd/nand/{ => raw}/jz4740_nand.c (97%)
rename drivers/mtd/nand/{ => raw}/jz4780_bch.c (100%)
rename drivers/mtd/nand/{ => raw}/jz4780_bch.h (100%)
rename drivers/mtd/nand/{ => raw}/jz4780_nand.c (97%)
rename drivers/mtd/nand/{ => raw}/lpc32xx_mlc.c (98%)
rename drivers/mtd/nand/{ => raw}/lpc32xx_slc.c (98%)
rename drivers/mtd/nand/{ => raw}/mpc5121_nfc.c (96%)
rename drivers/mtd/nand/{ => raw}/mtk_ecc.c (100%)
rename drivers/mtd/nand/{ => raw}/mtk_ecc.h (100%)
rename drivers/mtd/nand/{ => raw}/mtk_nand.c (97%)
rename drivers/mtd/nand/{ => raw}/mxc_nand.c (97%)
rename drivers/mtd/nand/{ => raw}/nand_amd.c (94%)
rename drivers/mtd/nand/{ => raw}/nand_base.c (94%)
rename drivers/mtd/nand/{ => raw}/nand_bch.c (97%)
rename drivers/mtd/nand/{ => raw}/nand_ecc.c (99%)
rename drivers/mtd/nand/{ => raw}/nand_hynix.c (98%)
rename drivers/mtd/nand/{ => raw}/nand_ids.c (99%)
rename drivers/mtd/nand/{ => raw}/nand_macronix.c (96%)
rename drivers/mtd/nand/{ => raw}/nand_micron.c (96%)
rename drivers/mtd/nand/{ => raw}/nand_samsung.c (95%)
rename drivers/mtd/nand/{ => raw}/nand_timings.c (99%)
rename drivers/mtd/nand/{ => raw}/nand_toshiba.c (95%)
rename drivers/mtd/nand/{ => raw}/nandsim.c (99%)
rename drivers/mtd/nand/{ => raw}/ndfc.c (93%)
rename drivers/mtd/nand/{ => raw}/nuc900_nand.c (96%)
rename drivers/mtd/nand/{ => raw}/omap2.c (99%)
rename drivers/mtd/nand/{ => raw}/omap_elm.c (100%)
rename drivers/mtd/nand/{ => raw}/orion_nand.c (96%)
rename drivers/mtd/nand/{ => raw}/oxnas_nand.c (93%)
rename drivers/mtd/nand/{ => raw}/pasemi_nand.c (95%)
rename drivers/mtd/nand/{ => raw}/plat_nand.c (96%)
rename drivers/mtd/nand/{ => raw}/pxa3xx_nand.c (98%)
rename drivers/mtd/nand/{ => raw}/qcom_nandc.c (98%)
rename drivers/mtd/nand/{ => raw}/r852.c (99%)
rename drivers/mtd/nand/{ => raw}/r852.h (99%)
rename drivers/mtd/nand/{ => raw}/s3c2410.c (98%)
rename drivers/mtd/nand/{ => raw}/sh_flctl.c (99%)
rename drivers/mtd/nand/{ => raw}/sharpsl.c (96%)
rename drivers/mtd/nand/{ => raw}/sm_common.c (98%)
rename drivers/mtd/nand/{ => raw}/sm_common.h (100%)
rename drivers/mtd/nand/{ => raw}/socrates_nand.c (94%)
rename drivers/mtd/nand/{ => raw}/sunxi_nand.c (97%)
rename drivers/mtd/nand/{ => raw}/tango_nand.c (95%)
rename drivers/mtd/nand/{ => raw}/tmio_nand.c (98%)
rename drivers/mtd/nand/{ => raw}/txx9ndfmc.c (97%)
rename drivers/mtd/nand/{ => raw}/vf610_nfc.c (99%)
rename drivers/mtd/nand/{ => raw}/xway_nand.c (96%)
create mode 100644 drivers/mtd/nand/spi/Kconfig
create mode 100644 drivers/mtd/nand/spi/Makefile
create mode 100644 drivers/mtd/nand/spi/controllers/Kconfig
create mode 100644 drivers/mtd/nand/spi/controllers/Makefile
create mode 100644 drivers/mtd/nand/spi/controllers/generic-spi.c
create mode 100644 drivers/mtd/nand/spi/core.c
create mode 100644 drivers/mtd/nand/spi/micron.c
create mode 100644 include/linux/mtd/rawnand.h
create mode 100644 include/linux/mtd/spinand.h
--
1.9.1
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v6 00/15] A SPI NAND framework under generic NAND framework 2017-05-24 7:06 Peter Pan @ 2017-05-29 20:59 ` Boris Brezillon 2017-12-04 13:32 ` Frieder Schrempf 0 siblings, 1 reply; 32+ messages in thread From: Boris Brezillon @ 2017-05-29 20:59 UTC (permalink / raw) To: Peter Pan, arnaud.mouiche Cc: richard, computersforpeace, thomas.petazzoni, marex, cyrille.pitchen, linux-mtd, peterpansjtu, linshunquan1 Hi Peter, On Wed, 24 May 2017 15:06:56 +0800 Peter Pan <peterpandong@micron.com> wrote: > First of all, thank Boris and Marek for your priceless comments > on v5 and thank everyone reviewed and tested on my previous series. > I can never be here without your help. This series comes to v6 > and it becomes much better with your help. > > SPI NAND is a new NAND family device with SPI protocol as > its interface. And its command set is totally different > with parallel NAND. > > Our first attempt to SPI NAND was more than 2 years ago[1]. > At that time, I didn't make BBT shareable and there were > too many duplicate code with parallel NAND, so that serie > stoped. But the discussion never stops. Now Boris has a plan > to make a generic NAND framework which can be shared with > both parallel and SPI NAND. Now the first step of the > new generic NAND framework is finished. And it is waiting > for a user. After discussion with Boris. We both think it's > time to rebuild SPI NAND framework based on the new NAND > framework and send out for reviewing. > > This series includes two part. The first part (patch 1 to 9) > is a new generic NAND framework from Boris Brezillon, which > is from Biris's nand/generic branch[2]. The second part > (patch 10 to 15) introductes a SPI NAND framework based on > the new generic NAND framework. > > This series only supports basic SPI NAND features and uses > generic spi controller for data transfer. ECC support is removed > since it's not in a good structure and more important, it should > be shared between different NAND devices, which means it should > be in new NAND core. Support different types of ECC and advanced > SPI NAND features is the next step. > > This series is based on nand/next branch and is tested on > Xilinx Zedboard with Micron MT29F2G01ABAGDSF SPI NAND chip. As you can see, I started to review this v6, but I don't expect you to send a new version (at least not immediately). Here is the plan: I'll finish reviewing the series, and while I'm reviewing I'll try to address my own comments. Once the review/changes are done, I'll push a nand/spi branch to the github repo [1] and ask you and Arnaud to review/test the whole thing. If you're happy with my changes (it should only be minor changes to your initial implementation) I'll ask you to send a v7. I'll then wait for 4.13-rc1 to be out and apply everything to my nand/next branch (I expect a few conflicting changes in the nand/mtd area caused by the migration to the new doc format, that's why I'd like to wait one more cycle). If everything goes well, we should be good for 4.14, and the patches will have spent enough time in linux-next to discover obvious bugs. Let me know if you have a problem with this approach. Thanks, Boris > > > v6 changes: > - includes generic NAND framework patches in series > - rebase on nand/next (commit 6076fd1e9d879521f7082a5e22185b71e480b777) > - remove on-die ECC support > - remove devm_free() since everything allocated by devm_kmalloc() will be > automatically freed when device is released > - add comment header for structs in spinand.h > - remove spinand_register()/unregister(), call spinand_detect() in > spinand_init() and only expose spinand_init()/cleanup() > - add nand_release_bbt() in bbt.c and use it in nand_cleanup() and > spinand_cleanup() > - use BIT(n) instead (1 << n) in macro of spinand.h > - rename spinand_alloc() to devm_spinand_alloc() > - name lables in better way > - fix some typos > - add empty lines between code blocks > > v5 changes: > - rebase patch on nand/next with Boris's generic NAND framework patches[3] > - replace pr_xxx() with dev_xxx() > - replace kzalloc()i/kfree() with devm_kzalloc()/devm_kfree() > - rename spinand_op_init() to spinand_init_op() for consistency > - remove command opcode in function comments > - use BIT(n) instead (1 << n) in macro > - remove manufactures.c and put spinand_manufacturers table in core.c > - change spinand_write_reg() u8 *buf argument to u8 value, > since the length is always 1 > - remove spinand_manufacture->detect() check, since it is always != NULL > - alloc spinand_ecc_engine struct in vendor.c when using on-die ECC > (for hardware ECC, it should be in controllers/*.c) > - add comment header for struct spinand_op > - fix timeout bug in spinand_wait(), thanks for Arnaud's debug > - make spinand_manufacturers const > - add ecc_engine_ops pointer in struct micron_spinand_info > - make controller->cap assignment right with SPI_TX/RX_QUAD/DUAL flag > > v4 changes: > - initialize struct mtd_oob_ops to 0 in bbt.c > - rename new added helper in nand.h to nand_check_xxxx() > - add struct mtd_oob_ops consistency check in nand_check_oob_ops() > - add dataleft in struct nand_page_iter instead of offs > - remove spinand_manufacturers->ops->detect() check since it is mandatory > - remove spinand_set_manufacturer_ops() and do the job in > spinand_manufacturer_detect() > - move .priv out of struct spinand_controller > - add spinand_alloc/free/register/unregister() and make > spinand_detect/init() static > - make BBT be configured by device tree > - chip->id.data stores raw ID directly > - refine device info print message after detect success > - add struct mtd_layout_ops pointer in struct micron_spinand_info > - remove micron_spinand_init() and do its job in micron_spinand_detect() > - fix BBT block cannot be erased bug > > v3 changes: > - rebase patch on 4.11-rc1[2] > - change read ID method. read 4 bytes ID out then let ->detect() of each > manufacutre driver to decode ID and detect the device. > - make SPI NAND id table private to each manufacutre driver > - fix coding style to make checkpatch.pl happy > - update the MAINTAINERS file for spi nand code > - add nand_size() helper in nand.h > - use nand_for_each_page() helper in spinand_do_read/write_ops() > - create helper to check boundaries in generic NAND code and use it > in SPI NAND core > - rename spinand_base.c to core.c > - manufactures' drivers expose spinand_manufacturer struct instead of > spinand_manufacturer_ops struct to keep Manufacture ID macro in > manufactures' drivers and rename spinand_ids.c to manufacture.c > - rename spinand_micron.c to micron.c > - rename chips/ directory to controllers/ > - rename generic_spi.c to generic-spi.c > - replace ->build_column_addr() and ->get_dummy() hooks with ->prepare_op() in > spinand_manufacturer_ops struct > - rename __spinand_erase() to spinand_erase() > - rename spinand_erase() to spinand_erase_skip_bbt() > - rename spinand_scan_ident() to spinand_detect() > - rename spinand_scan_tail() to spinand_init() > - move non detect related code from spinand_detect() to spinand_init() > - remove spinand_fill_nandd, assign nand->ops in spinand_detect() > - merge v2 patch 3(bad block support) and patch 4(BBT support) > - drop getchip parameter, remove spinand_get/remove_device(), take the lock > by caller directly > - fix function comment headers > - use nand_bbt_is_initialized() helper > - replace spinand_ecc_engine and spinand_controller object in spinand_device > struct with pointer > - replace struct spinand_manufacturer_ops pointer in spinand_device struct > with spinand_manufacturer struct > > v2 changes: > - replace "spi_nand" with "spinand". > - rename spi nand related structs for better understanding. > - introduce spi nand controller, manufacturer and ecc_engine struct. > - add spi nand manufacturer initialization function refer to Boris's > manuf-init branch. > - remove NAND_SKIP_BBTSCAN from series. Add it later when enabling HW ECC. > - reorganize series according to Boris's suggestion. > > > [1]http://lists.infradead.org/pipermail/linux-mtd/2015-January/057223.html > [2]https://github.com/bbrezillon/linux-0day/tree/nand/generic > [3]https://github.com/peterpansjtu/linux/tree/nand/spinand > > > Boris Brezillon (8): > mtd: nand: Rename nand.h into rawnand.h > mtd: nand: move raw NAND related code to the raw/ subdir > mtd: nand: add a nand.h file to expose basic NAND stuff > mtd: nand: raw: prefix conflicting names with nandcchip instead of > nand > mtd: nand: raw: create struct rawnand_device > mtd: nand: raw: make BBT code more generic > mtd: nand: move BBT code to drivers/mtd/nand/ > mtd: nand: Add the page iterator concept > > Peter Pan (7): > mtd: nand: make sure mtd_oob_ops consistent in bbt > nand: spi: add basic blocks for infrastructure > nand: spi: add basic operations support > nand: spi: Add bad block support > nand: spi: add Micron spi nand support > nand: spi: Add generic SPI controller support > MAINTAINERS: Add SPI NAND entry > > Documentation/DocBook/mtdnand.tmpl | 12 +- > MAINTAINERS | 29 +- > arch/arm/mach-davinci/board-da850-evm.c | 2 +- > arch/arm/mach-davinci/board-dm355-evm.c | 2 +- > arch/arm/mach-davinci/board-dm355-leopard.c | 2 +- > arch/arm/mach-davinci/board-dm365-evm.c | 2 +- > arch/arm/mach-davinci/board-dm644x-evm.c | 2 +- > arch/arm/mach-davinci/board-dm646x-evm.c | 2 +- > arch/arm/mach-davinci/board-sffsdr.c | 2 +- > arch/arm/mach-dove/dove-db-setup.c | 2 +- > arch/arm/mach-ep93xx/snappercl15.c | 6 +- > arch/arm/mach-ep93xx/ts72xx.c | 6 +- > arch/arm/mach-imx/mach-qong.c | 4 +- > arch/arm/mach-ixp4xx/ixdp425-setup.c | 4 +- > arch/arm/mach-mmp/aspenite.c | 2 +- > arch/arm/mach-omap1/board-fsample.c | 2 +- > arch/arm/mach-omap1/board-h2.c | 2 +- > arch/arm/mach-omap1/board-h3.c | 2 +- > arch/arm/mach-omap1/board-nand.c | 4 +- > arch/arm/mach-omap1/board-perseus2.c | 2 +- > arch/arm/mach-orion5x/db88f5281-setup.c | 2 +- > arch/arm/mach-orion5x/kurobox_pro-setup.c | 2 +- > arch/arm/mach-orion5x/ts209-setup.c | 2 +- > arch/arm/mach-orion5x/ts78xx-setup.c | 8 +- > arch/arm/mach-pxa/balloon3.c | 4 +- > arch/arm/mach-pxa/em-x270.c | 4 +- > arch/arm/mach-pxa/eseries.c | 2 +- > arch/arm/mach-pxa/palmtx.c | 4 +- > arch/arm/mach-pxa/tosa.c | 2 +- > arch/arm/mach-s3c24xx/common-smdk.c | 2 +- > arch/arm/mach-s3c24xx/mach-anubis.c | 2 +- > arch/arm/mach-s3c24xx/mach-at2440evb.c | 2 +- > arch/arm/mach-s3c24xx/mach-bast.c | 2 +- > arch/arm/mach-s3c24xx/mach-gta02.c | 2 +- > arch/arm/mach-s3c24xx/mach-jive.c | 2 +- > arch/arm/mach-s3c24xx/mach-mini2440.c | 2 +- > arch/arm/mach-s3c24xx/mach-osiris.c | 2 +- > arch/arm/mach-s3c24xx/mach-qt2410.c | 2 +- > arch/arm/mach-s3c24xx/mach-rx3715.c | 2 +- > arch/arm/mach-s3c24xx/mach-vstms.c | 2 +- > arch/blackfin/mach-bf537/boards/dnp5370.c | 2 +- > arch/blackfin/mach-bf537/boards/stamp.c | 4 +- > arch/blackfin/mach-bf561/boards/acvilon.c | 4 +- > arch/cris/arch-v32/drivers/mach-a3/nandflash.c | 6 +- > arch/cris/arch-v32/drivers/mach-fs/nandflash.c | 6 +- > arch/mips/alchemy/devboards/db1200.c | 4 +- > arch/mips/alchemy/devboards/db1300.c | 4 +- > arch/mips/alchemy/devboards/db1550.c | 4 +- > arch/mips/include/asm/mach-jz4740/jz4740_nand.h | 2 +- > arch/mips/jz4740/board-qi_lb60.c | 2 +- > arch/mips/netlogic/xlr/platform-flash.c | 2 +- > arch/mips/pnx833x/common/platform.c | 4 +- > arch/mips/rb532/devices.c | 4 +- > arch/sh/boards/mach-migor/setup.c | 4 +- > drivers/mtd/inftlcore.c | 2 +- > drivers/mtd/nand/Kconfig | 566 +------ > drivers/mtd/nand/Makefile | 71 +- > drivers/mtd/nand/{nand_bbt.c => bbt.c} | 697 ++++---- > drivers/mtd/nand/raw/Kconfig | 566 +++++++ > drivers/mtd/nand/raw/Makefile | 69 + > drivers/mtd/nand/{ => raw}/ams-delta.c | 10 +- > drivers/mtd/nand/{ => raw}/atmel/Makefile | 0 > drivers/mtd/nand/{ => raw}/atmel/nand-controller.c | 52 +- > drivers/mtd/nand/{ => raw}/atmel/pmecc.c | 2 +- > drivers/mtd/nand/{ => raw}/atmel/pmecc.h | 0 > drivers/mtd/nand/{ => raw}/au1550nd.c | 28 +- > drivers/mtd/nand/{ => raw}/bcm47xxnflash/Makefile | 0 > .../nand/{ => raw}/bcm47xxnflash/bcm47xxnflash.h | 2 +- > drivers/mtd/nand/{ => raw}/bcm47xxnflash/main.c | 4 +- > .../mtd/nand/{ => raw}/bcm47xxnflash/ops_bcm4706.c | 18 +- > drivers/mtd/nand/{ => raw}/bf5xx_nand.c | 34 +- > drivers/mtd/nand/{ => raw}/brcmnand/Makefile | 0 > .../mtd/nand/{ => raw}/brcmnand/bcm63138_nand.c | 0 > drivers/mtd/nand/{ => raw}/brcmnand/bcm6368_nand.c | 0 > drivers/mtd/nand/{ => raw}/brcmnand/brcmnand.c | 34 +- > drivers/mtd/nand/{ => raw}/brcmnand/brcmnand.h | 0 > drivers/mtd/nand/{ => raw}/brcmnand/brcmstb_nand.c | 0 > drivers/mtd/nand/{ => raw}/brcmnand/iproc_nand.c | 0 > drivers/mtd/nand/{ => raw}/cafe_nand.c | 26 +- > drivers/mtd/nand/{ => raw}/cmx270_nand.c | 14 +- > drivers/mtd/nand/{ => raw}/cs553x_nand.c | 22 +- > drivers/mtd/nand/{ => raw}/davinci_nand.c | 16 +- > drivers/mtd/nand/{ => raw}/denali.c | 14 +- > drivers/mtd/nand/{ => raw}/denali.h | 2 +- > drivers/mtd/nand/{ => raw}/denali_dt.c | 0 > drivers/mtd/nand/{ => raw}/denali_pci.c | 0 > drivers/mtd/nand/{ => raw}/diskonchip.c | 70 +- > drivers/mtd/nand/{ => raw}/docg4.c | 45 +- > drivers/mtd/nand/{ => raw}/fsl_elbc_nand.c | 28 +- > drivers/mtd/nand/{ => raw}/fsl_ifc_nand.c | 30 +- > drivers/mtd/nand/{ => raw}/fsl_upm.c | 14 +- > drivers/mtd/nand/{ => raw}/fsmc_nand.c | 26 +- > drivers/mtd/nand/{ => raw}/gpio.c | 8 +- > drivers/mtd/nand/{ => raw}/gpmi-nand/Makefile | 0 > drivers/mtd/nand/{ => raw}/gpmi-nand/bch-regs.h | 0 > drivers/mtd/nand/{ => raw}/gpmi-nand/gpmi-lib.c | 2 +- > drivers/mtd/nand/{ => raw}/gpmi-nand/gpmi-nand.c | 38 +- > drivers/mtd/nand/{ => raw}/gpmi-nand/gpmi-nand.h | 2 +- > drivers/mtd/nand/{ => raw}/gpmi-nand/gpmi-regs.h | 0 > drivers/mtd/nand/{ => raw}/hisi504_nand.c | 26 +- > drivers/mtd/nand/{ => raw}/jz4740_nand.c | 14 +- > drivers/mtd/nand/{ => raw}/jz4780_bch.c | 0 > drivers/mtd/nand/{ => raw}/jz4780_bch.h | 0 > drivers/mtd/nand/{ => raw}/jz4780_nand.c | 10 +- > drivers/mtd/nand/{ => raw}/lpc32xx_mlc.c | 18 +- > drivers/mtd/nand/{ => raw}/lpc32xx_slc.c | 22 +- > drivers/mtd/nand/{ => raw}/mpc5121_nfc.c | 28 +- > drivers/mtd/nand/{ => raw}/mtk_ecc.c | 0 > drivers/mtd/nand/{ => raw}/mtk_ecc.h | 0 > drivers/mtd/nand/{ => raw}/mtk_nand.c | 40 +- > drivers/mtd/nand/{ => raw}/mxc_nand.c | 54 +- > drivers/mtd/nand/{ => raw}/nand_amd.c | 4 +- > drivers/mtd/nand/{ => raw}/nand_base.c | 272 +++- > drivers/mtd/nand/{ => raw}/nand_bch.c | 8 +- > drivers/mtd/nand/{ => raw}/nand_ecc.c | 6 +- > drivers/mtd/nand/{ => raw}/nand_hynix.c | 12 +- > drivers/mtd/nand/{ => raw}/nand_ids.c | 2 +- > drivers/mtd/nand/{ => raw}/nand_macronix.c | 2 +- > drivers/mtd/nand/{ => raw}/nand_micron.c | 12 +- > drivers/mtd/nand/{ => raw}/nand_samsung.c | 6 +- > drivers/mtd/nand/{ => raw}/nand_timings.c | 2 +- > drivers/mtd/nand/{ => raw}/nand_toshiba.c | 4 +- > drivers/mtd/nand/{ => raw}/nandsim.c | 24 +- > drivers/mtd/nand/{ => raw}/ndfc.c | 20 +- > drivers/mtd/nand/{ => raw}/nuc900_nand.c | 10 +- > drivers/mtd/nand/{ => raw}/omap2.c | 22 +- > drivers/mtd/nand/{ => raw}/omap_elm.c | 0 > drivers/mtd/nand/{ => raw}/orion_nand.c | 10 +- > drivers/mtd/nand/{ => raw}/oxnas_nand.c | 14 +- > drivers/mtd/nand/{ => raw}/pasemi_nand.c | 12 +- > drivers/mtd/nand/{ => raw}/plat_nand.c | 6 +- > drivers/mtd/nand/{ => raw}/pxa3xx_nand.c | 38 +- > drivers/mtd/nand/{ => raw}/qcom_nandc.c | 30 +- > drivers/mtd/nand/{ => raw}/r852.c | 8 +- > drivers/mtd/nand/{ => raw}/r852.h | 2 +- > drivers/mtd/nand/{ => raw}/s3c2410.c | 18 +- > drivers/mtd/nand/{ => raw}/sh_flctl.c | 12 +- > drivers/mtd/nand/{ => raw}/sharpsl.c | 10 +- > drivers/mtd/nand/{ => raw}/sm_common.c | 4 +- > drivers/mtd/nand/{ => raw}/sm_common.h | 0 > drivers/mtd/nand/{ => raw}/socrates_nand.c | 14 +- > drivers/mtd/nand/{ => raw}/sunxi_nand.c | 60 +- > drivers/mtd/nand/{ => raw}/tango_nand.c | 32 +- > drivers/mtd/nand/{ => raw}/tmio_nand.c | 10 +- > drivers/mtd/nand/{ => raw}/txx9ndfmc.c | 16 +- > drivers/mtd/nand/{ => raw}/vf610_nfc.c | 6 +- > drivers/mtd/nand/{ => raw}/xway_nand.c | 12 +- > drivers/mtd/nand/spi/Kconfig | 7 + > drivers/mtd/nand/spi/Makefile | 3 + > drivers/mtd/nand/spi/controllers/Kconfig | 5 + > drivers/mtd/nand/spi/controllers/Makefile | 1 + > drivers/mtd/nand/spi/controllers/generic-spi.c | 162 ++ > drivers/mtd/nand/spi/core.c | 1381 ++++++++++++++++ > drivers/mtd/nand/spi/micron.c | 157 ++ > drivers/mtd/nftlcore.c | 2 +- > drivers/mtd/nftlmount.c | 2 +- > drivers/mtd/sm_ftl.c | 2 +- > drivers/mtd/ssfdc.c | 2 +- > drivers/mtd/tests/nandbiterrs.c | 2 +- > drivers/staging/mt29f_spinand/mt29f_spinand.c | 8 +- > fs/jffs2/wbuf.c | 2 +- > include/linux/mtd/nand-gpio.h | 2 +- > include/linux/mtd/nand.h | 1667 +++++++------------- > include/linux/mtd/rawnand.h | 1286 +++++++++++++++ > include/linux/mtd/sh_flctl.h | 4 +- > include/linux/mtd/sharpsl.h | 2 +- > include/linux/mtd/spinand.h | 272 ++++ > include/linux/platform_data/atmel.h | 2 +- > include/linux/platform_data/mtd-davinci.h | 2 +- > include/linux/platform_data/mtd-nand-s3c2410.h | 2 +- > 170 files changed, 5708 insertions(+), 2906 deletions(-) > rename drivers/mtd/nand/{nand_bbt.c => bbt.c} (69%) > create mode 100644 drivers/mtd/nand/raw/Kconfig > create mode 100644 drivers/mtd/nand/raw/Makefile > rename drivers/mtd/nand/{ => raw}/ams-delta.c (97%) > rename drivers/mtd/nand/{ => raw}/atmel/Makefile (100%) > rename drivers/mtd/nand/{ => raw}/atmel/nand-controller.c (97%) > rename drivers/mtd/nand/{ => raw}/atmel/pmecc.c (99%) > rename drivers/mtd/nand/{ => raw}/atmel/pmecc.h (100%) > rename drivers/mtd/nand/{ => raw}/au1550nd.c (95%) > rename drivers/mtd/nand/{ => raw}/bcm47xxnflash/Makefile (100%) > rename drivers/mtd/nand/{ => raw}/bcm47xxnflash/bcm47xxnflash.h (92%) > rename drivers/mtd/nand/{ => raw}/bcm47xxnflash/main.c (95%) > rename drivers/mtd/nand/{ => raw}/bcm47xxnflash/ops_bcm4706.c (96%) > rename drivers/mtd/nand/{ => raw}/bf5xx_nand.c (95%) > rename drivers/mtd/nand/{ => raw}/brcmnand/Makefile (100%) > rename drivers/mtd/nand/{ => raw}/brcmnand/bcm63138_nand.c (100%) > rename drivers/mtd/nand/{ => raw}/brcmnand/bcm6368_nand.c (100%) > rename drivers/mtd/nand/{ => raw}/brcmnand/brcmnand.c (98%) > rename drivers/mtd/nand/{ => raw}/brcmnand/brcmnand.h (100%) > rename drivers/mtd/nand/{ => raw}/brcmnand/brcmstb_nand.c (100%) > rename drivers/mtd/nand/{ => raw}/brcmnand/iproc_nand.c (100%) > rename drivers/mtd/nand/{ => raw}/cafe_nand.c (97%) > rename drivers/mtd/nand/{ => raw}/cmx270_nand.c (94%) > rename drivers/mtd/nand/{ => raw}/cs553x_nand.c (95%) > rename drivers/mtd/nand/{ => raw}/davinci_nand.c (98%) > rename drivers/mtd/nand/{ => raw}/denali.c (99%) > rename drivers/mtd/nand/{ => raw}/denali.h (99%) > rename drivers/mtd/nand/{ => raw}/denali_dt.c (100%) > rename drivers/mtd/nand/{ => raw}/denali_pci.c (100%) > rename drivers/mtd/nand/{ => raw}/diskonchip.c (96%) > rename drivers/mtd/nand/{ => raw}/docg4.c (97%) > rename drivers/mtd/nand/{ => raw}/fsl_elbc_nand.c (97%) > rename drivers/mtd/nand/{ => raw}/fsl_ifc_nand.c (97%) > rename drivers/mtd/nand/{ => raw}/fsl_upm.c (95%) > rename drivers/mtd/nand/{ => raw}/fsmc_nand.c (97%) > rename drivers/mtd/nand/{ => raw}/gpio.c (97%) > rename drivers/mtd/nand/{ => raw}/gpmi-nand/Makefile (100%) > rename drivers/mtd/nand/{ => raw}/gpmi-nand/bch-regs.h (100%) > rename drivers/mtd/nand/{ => raw}/gpmi-nand/gpmi-lib.c (99%) > rename drivers/mtd/nand/{ => raw}/gpmi-nand/gpmi-nand.c (98%) > rename drivers/mtd/nand/{ => raw}/gpmi-nand/gpmi-nand.h (99%) > rename drivers/mtd/nand/{ => raw}/gpmi-nand/gpmi-regs.h (100%) > rename drivers/mtd/nand/{ => raw}/hisi504_nand.c (97%) > rename drivers/mtd/nand/{ => raw}/jz4740_nand.c (97%) > rename drivers/mtd/nand/{ => raw}/jz4780_bch.c (100%) > rename drivers/mtd/nand/{ => raw}/jz4780_bch.h (100%) > rename drivers/mtd/nand/{ => raw}/jz4780_nand.c (97%) > rename drivers/mtd/nand/{ => raw}/lpc32xx_mlc.c (98%) > rename drivers/mtd/nand/{ => raw}/lpc32xx_slc.c (98%) > rename drivers/mtd/nand/{ => raw}/mpc5121_nfc.c (96%) > rename drivers/mtd/nand/{ => raw}/mtk_ecc.c (100%) > rename drivers/mtd/nand/{ => raw}/mtk_ecc.h (100%) > rename drivers/mtd/nand/{ => raw}/mtk_nand.c (97%) > rename drivers/mtd/nand/{ => raw}/mxc_nand.c (97%) > rename drivers/mtd/nand/{ => raw}/nand_amd.c (94%) > rename drivers/mtd/nand/{ => raw}/nand_base.c (94%) > rename drivers/mtd/nand/{ => raw}/nand_bch.c (97%) > rename drivers/mtd/nand/{ => raw}/nand_ecc.c (99%) > rename drivers/mtd/nand/{ => raw}/nand_hynix.c (98%) > rename drivers/mtd/nand/{ => raw}/nand_ids.c (99%) > rename drivers/mtd/nand/{ => raw}/nand_macronix.c (96%) > rename drivers/mtd/nand/{ => raw}/nand_micron.c (96%) > rename drivers/mtd/nand/{ => raw}/nand_samsung.c (95%) > rename drivers/mtd/nand/{ => raw}/nand_timings.c (99%) > rename drivers/mtd/nand/{ => raw}/nand_toshiba.c (95%) > rename drivers/mtd/nand/{ => raw}/nandsim.c (99%) > rename drivers/mtd/nand/{ => raw}/ndfc.c (93%) > rename drivers/mtd/nand/{ => raw}/nuc900_nand.c (96%) > rename drivers/mtd/nand/{ => raw}/omap2.c (99%) > rename drivers/mtd/nand/{ => raw}/omap_elm.c (100%) > rename drivers/mtd/nand/{ => raw}/orion_nand.c (96%) > rename drivers/mtd/nand/{ => raw}/oxnas_nand.c (93%) > rename drivers/mtd/nand/{ => raw}/pasemi_nand.c (95%) > rename drivers/mtd/nand/{ => raw}/plat_nand.c (96%) > rename drivers/mtd/nand/{ => raw}/pxa3xx_nand.c (98%) > rename drivers/mtd/nand/{ => raw}/qcom_nandc.c (98%) > rename drivers/mtd/nand/{ => raw}/r852.c (99%) > rename drivers/mtd/nand/{ => raw}/r852.h (99%) > rename drivers/mtd/nand/{ => raw}/s3c2410.c (98%) > rename drivers/mtd/nand/{ => raw}/sh_flctl.c (99%) > rename drivers/mtd/nand/{ => raw}/sharpsl.c (96%) > rename drivers/mtd/nand/{ => raw}/sm_common.c (98%) > rename drivers/mtd/nand/{ => raw}/sm_common.h (100%) > rename drivers/mtd/nand/{ => raw}/socrates_nand.c (94%) > rename drivers/mtd/nand/{ => raw}/sunxi_nand.c (97%) > rename drivers/mtd/nand/{ => raw}/tango_nand.c (95%) > rename drivers/mtd/nand/{ => raw}/tmio_nand.c (98%) > rename drivers/mtd/nand/{ => raw}/txx9ndfmc.c (97%) > rename drivers/mtd/nand/{ => raw}/vf610_nfc.c (99%) > rename drivers/mtd/nand/{ => raw}/xway_nand.c (96%) > create mode 100644 drivers/mtd/nand/spi/Kconfig > create mode 100644 drivers/mtd/nand/spi/Makefile > create mode 100644 drivers/mtd/nand/spi/controllers/Kconfig > create mode 100644 drivers/mtd/nand/spi/controllers/Makefile > create mode 100644 drivers/mtd/nand/spi/controllers/generic-spi.c > create mode 100644 drivers/mtd/nand/spi/core.c > create mode 100644 drivers/mtd/nand/spi/micron.c > create mode 100644 include/linux/mtd/rawnand.h > create mode 100644 include/linux/mtd/spinand.h > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 00/15] A SPI NAND framework under generic NAND framework 2017-05-29 20:59 ` Boris Brezillon @ 2017-12-04 13:32 ` Frieder Schrempf 2017-12-04 14:05 ` Boris Brezillon 0 siblings, 1 reply; 32+ messages in thread From: Frieder Schrempf @ 2017-12-04 13:32 UTC (permalink / raw) To: linux-mtd, boris.brezillon; +Cc: peterpandong Hi Boris, > If everything goes well, we should be good for 4.14, and the patches > will have spent enough time in linux-next to discover obvious bugs. What is the latest status of the SPI-NAND framework patches? The reason I am asking is, that we have hardware based on the NXP i.MX6UL SOC with a serial NAND connected via QSPI interface. We currently have a working implementation based on a 3.14 vendor kernel using a modified version of the "mt29f_spinand" staging driver, but for the future we plan to use a recent mainline kernel. We also think about porting our implementation to the new framework to enable support for more SPI NAND chips (Winbond, Toshiba) and for the NXP QSPI-controller. Therefore we would like to know about the current schedule for bringing the framework to mainline. Thanks, Frieder Schrempf ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 00/15] A SPI NAND framework under generic NAND framework 2017-12-04 13:32 ` Frieder Schrempf @ 2017-12-04 14:05 ` Boris Brezillon 2017-12-05 1:35 ` Peter Pan 潘栋 (peterpandong) 0 siblings, 1 reply; 32+ messages in thread From: Boris Brezillon @ 2017-12-04 14:05 UTC (permalink / raw) To: Frieder Schrempf; +Cc: linux-mtd, peterpandong Hi Frieder, On Mon, 4 Dec 2017 14:32:14 +0100 Frieder Schrempf <frieder.schrempf@exceet.de> wrote: > Hi Boris, > > > If everything goes well, we should be good for 4.14, and the patches > > will have spent enough time in linux-next to discover obvious bugs. > > What is the latest status of the SPI-NAND framework patches? > > The reason I am asking is, that we have hardware based on the NXP > i.MX6UL SOC with a serial NAND connected via QSPI interface. > > We currently have a working implementation based on a 3.14 vendor kernel > using a modified version of the "mt29f_spinand" staging driver, but for > the future we plan to use a recent mainline kernel. > > We also think about porting our implementation to the new framework to > enable support for more SPI NAND chips (Winbond, Toshiba) and for the > NXP QSPI-controller. > > Therefore we would like to know about the current schedule for bringing > the framework to mainline. Sorry for the silence and the lack of progress on this front. I don't have much time to work on this SPI-NAND framework (I do it on my spare time), and last time I had a look and tried to address my own comments on Peter's version, I realized I was not really happy with the implementation, mainly because it copies some of the mistakes done in the raw/parallel NAND framework. So I ended up rewriting a lot of code, and didn't have time to test the new implementation [1]. Note that it's mainly a rewrite of the generic NAND layer the SPI-NAND framework is based on. I also re-considered the option of moving existing BBT handling code in the generic layer, because again, I think we should try to lighten the existing implementation instead of quickly adapting it to the generic NAND layer. So, what's in [1] is basic SPI-NAND support without BBT and ECC. Of course, this will be extended later on, but I think we should start small, and take the time to think about how we want to extend the generic layer so that some of the code can be re-used by the parallel/raw NAND and OneNAND frameworks. I'm really sorry to have blocked this initiative by not responding and/or not spending the necessary time to rework the code, but I really think we should have a strong base if we don't want to end up with what he have in the parallel/raw NAND/OneNAND frameworks (a code base that is hardly maintainable, with a lot of code duplication). Anyway, any help is appreciated, so if you do have time to review/test/enhance this code, feel free to do it. Regards, Boris [1]https://github.com/bbrezillon/linux-0day/commits/nand/spi-nand ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 00/15] A SPI NAND framework under generic NAND framework 2017-12-04 14:05 ` Boris Brezillon @ 2017-12-05 1:35 ` Peter Pan 潘栋 (peterpandong) 2017-12-05 12:58 ` Boris Brezillon 0 siblings, 1 reply; 32+ messages in thread From: Peter Pan 潘栋 (peterpandong) @ 2017-12-05 1:35 UTC (permalink / raw) To: Boris Brezillon Cc: Frieder Schrempf, linux-mtd@lists.infradead.org, Peter Pan Hi Boris, 在 2017年12月4日，22:05，Boris Brezillon <boris.brezillon@free-electrons.com<mailto:boris.brezillon@free-electrons.com>> 写道： Hi Frieder, On Mon, 4 Dec 2017 14:32:14 +0100 Frieder Schrempf <frieder.schrempf@exceet.de<mailto:frieder.schrempf@exceet.de>> wrote: Hi Boris, If everything goes well, we should be good for 4.14, and the patches will have spent enough time in linux-next to discover obvious bugs. What is the latest status of the SPI-NAND framework patches? The reason I am asking is, that we have hardware based on the NXP i.MX6UL SOC with a serial NAND connected via QSPI interface. We currently have a working implementation based on a 3.14 vendor kernel using a modified version of the "mt29f_spinand" staging driver, but for the future we plan to use a recent mainline kernel. We also think about porting our implementation to the new framework to enable support for more SPI NAND chips (Winbond, Toshiba) and for the NXP QSPI-controller. Therefore we would like to know about the current schedule for bringing the framework to mainline. Sorry for the silence and the lack of progress on this front. I don't have much time to work on this SPI-NAND framework (I do it on my spare time), and last time I had a look and tried to address my own comments on Peter's version, I realized I was not really happy with the implementation, mainly because it copies some of the mistakes done in the raw/parallel NAND framework. So I ended up rewriting a lot of code, and didn't have time to test the new implementation [1]. I’m still waiting for your git branch for spi NAND. Is this the branch I’m waiting for? I just took a quick look at it and I found you put more common code into new Nand core. This is cool. I thought we will do this job after spi NAND being merged. Anyway, do you want both spi and raw NAND code to be rebased on new NAND core or we just put spi NAND in? Anything I can help to speed the spi NAND merge up? Thanks Peter Pan Note that it's mainly a rewrite of the generic NAND layer the SPI-NAND framework is based on. I also re-considered the option of moving existing BBT handling code in the generic layer, because again, I think we should try to lighten the existing implementation instead of quickly adapting it to the generic NAND layer. So, what's in [1] is basic SPI-NAND support without BBT and ECC. Of course, this will be extended later on, but I think we should start small, and take the time to think about how we want to extend the generic layer so that some of the code can be re-used by the parallel/raw NAND and OneNAND frameworks. I'm really sorry to have blocked this initiative by not responding and/or not spending the necessary time to rework the code, but I really think we should have a strong base if we don't want to end up with what he have in the parallel/raw NAND/OneNAND frameworks (a code base that is hardly maintainable, with a lot of code duplication). Anyway, any help is appreciated, so if you do have time to review/test/enhance this code, feel free to do it. Regards, Boris [1]https://github.com/bbrezillon/linux-0day/commits/nand/spi-nand ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 00/15] A SPI NAND framework under generic NAND framework 2017-12-05 1:35 ` Peter Pan 潘栋 (peterpandong) @ 2017-12-05 12:58 ` Boris Brezillon 2017-12-05 13:03 ` Boris Brezillon 0 siblings, 1 reply; 32+ messages in thread From: Boris Brezillon @ 2017-12-05 12:58 UTC (permalink / raw) To: Peter Pan 潘栋 (peterpandong) Cc: Peter Pan, Frieder Schrempf, linux-mtd@lists.infradead.org Hi Peter, Can you please try to fix your mailer so that we can distinguish what is quoted from what you add? On Tue, 5 Dec 2017 01:35:05 +0000 Peter Pan 潘栋 (peterpandong) <peterpandong@micron.com> wrote: > > I’m still waiting for your git branch for spi NAND. Is this the branch > I’m waiting for? Yes, it's the branch I promised to share with you, but I didn't communicate on it since it's not yet in a clean state (still have to add kerneldoc headers, test everything, provide proper commit message, fix authorship, ...). > I just took a quick look at > it and I found you put more common code into new Nand core. This is > cool. I thought we will do this job after spi NAND being merged. Well, I don't think there's more code than before, it's just that I reworked the logic so that it could be more useful to other NAND sub-layers. > Anyway, do you want both spi and raw NAND code to be rebased on new > NAND core or we just put spi NAND in? That's another decision I took in this rework: I want to keep existing raw/parallel NAND framework unchanged, because it's a real pain to validate that everything works as expected when you do such invasive changes as the bbt rework we had done in earlier versions of this series. > Anything I can help to speed the > spi NAND merge up? Testing and reviewing, as usual. I'd really like to get the preparation patches (all patches touching mtd core code) in 4.16. For the rest, it really depends how much time you and other contributors (including me) can spend testing/reviewing/fixing/documenting the code. I am totally aware that I'm the one blocking the progress on this framework because of my constant hesitations on what the generic NAND layer should look like, but I'm a bit more confident now that we isolated the raw NAND code from the generic NAND changes (less risks of breaking existing NAND users). Regards, Boris ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 00/15] A SPI NAND framework under generic NAND framework 2017-12-05 12:58 ` Boris Brezillon @ 2017-12-05 13:03 ` Boris Brezillon 2017-12-12 9:58 ` Frieder Schrempf 0 siblings, 1 reply; 32+ messages in thread From: Boris Brezillon @ 2017-12-05 13:03 UTC (permalink / raw) To: Peter Pan 潘栋 (peterpandong) Cc: Peter Pan, Frieder Schrempf, linux-mtd@lists.infradead.org On Tue, 5 Dec 2017 13:58:04 +0100 Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > Hi Peter, > > Can you please try to fix your mailer so that we can distinguish what > is quoted from what you add? > > On Tue, 5 Dec 2017 01:35:05 +0000 > Peter Pan 潘栋 (peterpandong) <peterpandong@micron.com> wrote: > > > > > I’m still waiting for your git branch for spi NAND. Is this the branch > > I’m waiting for? > > Yes, it's the branch I promised to share with you, but I didn't > communicate on it since it's not yet in a clean state (still have to > add kerneldoc headers, test everything, provide proper commit > message, fix authorship, ...). > > > I just took a quick look at > > it and I found you put more common code into new Nand core. This is > > cool. I thought we will do this job after spi NAND being merged. > > Well, I don't think there's more code than before, it's just that I > reworked the logic so that it could be more useful to other NAND > sub-layers. > > > Anyway, do you want both spi and raw NAND code to be rebased on new > > NAND core or we just put spi NAND in? > > That's another decision I took in this rework: I want to keep existing > raw/parallel NAND framework unchanged, because it's a real pain to > validate that everything works as expected when you do such invasive > changes as the bbt rework we had done in earlier versions of this > series. > > > Anything I can help to speed the > > spi NAND merge up? > > Testing and reviewing, as usual. I'd really like to get the preparation > patches (all patches touching mtd core code) in 4.16. For the rest, it > really depends how much time you and other contributors (including me) > can spend testing/reviewing/fixing/documenting the code. > > I am totally aware that I'm the one blocking the progress on this > framework because of my constant hesitations on what the generic NAND > layer should look like, but I'm a bit more confident now that we > isolated the raw NAND code from the generic NAND changes (less risks of > breaking existing NAND users). Lastest version, with fixup patches squashed in original patches is available here[1]. [1]https://github.com/bbrezillon/linux-0day/commits/nand/spi-nand-squashed ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 00/15] A SPI NAND framework under generic NAND framework 2017-12-05 13:03 ` Boris Brezillon @ 2017-12-12 9:58 ` Frieder Schrempf 2017-12-13 21:27 ` Boris Brezillon 0 siblings, 1 reply; 32+ messages in thread From: Frieder Schrempf @ 2017-12-12 9:58 UTC (permalink / raw) To: Boris Brezillon Cc: Peter Pan 潘栋 (peterpandong), Peter Pan, linux-mtd@lists.infradead.org Hi Boris, I spent some time looking at and working with your latest SPI NAND code. In my previous mail I said, that we have some working code for our 3.14 kernel based on the "mt29f_spinand" staging driver, but that was wrong as I got things mixed up in my head. Actually our codebase was an early version of Peter's code, so the effort to port it to the latest framework code was not that big. I forked your repo and you can find my working tree at [1]. What I did so far: * Rebase your patches on latest Linux 4.14.5 * Add a driver for the Winbond W25M02GV SPI NAND chip, that we have on some of our boards. * Add a driver for the Freescale QSPI controller, derived from the existing QSPI-NOR driver at drivers/mtd/spi-nor/fsl-quadspi.c. * Add a setup and setup_late op to the controller layer (see [3]). I don't know if that makes sense, but at least the setup_late is needed for the FSL QSPI driver, as it needs to know the size of the flash to setup the memory mapping for the QSPI read operations. * A bit of testing and fixing two small bugs, which look like copy and paste mistakes. See [2]. * Running nandtest successfully on our hardware (i.MX6UL -> FSL-QSPI -> W25M02GV) I hope this is of use while moving on. I guess you would like to have the basic framework with Micron support and generic SPI tested and stabilized first, before adding more code, but to be able to test with our hardware I need Micron and FSL QSPI. Some questions/flaws that occurred to me: * The W25M02GV chip has two dies of 128M each. My current driver only makes use of the first die. The chip expects a die-select command to switch between the two dies. What would be the place to implement this? Can I just add the command and issue it in winbond_spinand_adjust_cache_op if luns_per_target > 1? * The FSL QSPI controller has a lookup table that needs to be filled with command sequences at the time of setup. Therefore the number of dummy bytes for each command is fixed and in my current implementation op->dummy_bytes is ignored. That's not a problem if all chips need the same number of dummy bytes for each command, but I guess that's not the case, as there is a _spinand_get_dummy function. * What is your plan for ECC and BBT? At least enabling the onchip ECC will be necessary to be able to use the flash properly (e.g. with UBI), or am I wrong? * Do you have any special test cases? What do you usually do to test the code? Thanks for your patience and best regards, Frieder [1] https://github.com/fschrempf/linux-0day/commits/spi-nand-exceet [2] https://github.com/fschrempf/linux-0day/commit/213caf23a0467eba4002e3dd5832c48adbeee3f4 [3] https://github.com/fschrempf/linux-0day/commit/3f39780e7c7ca12aeab77963e6c7dc1c38f524b5 On 05.12.2017 14:03, Boris Brezillon wrote: > On Tue, 5 Dec 2017 13:58:04 +0100 > Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > >> Testing and reviewing, as usual. I'd really like to get the preparation >> patches (all patches touching mtd core code) in 4.16. For the rest, it >> really depends how much time you and other contributors (including me) >> can spend testing/reviewing/fixing/documenting the code. >> >> I am totally aware that I'm the one blocking the progress on this >> framework because of my constant hesitations on what the generic NAND >> layer should look like, but I'm a bit more confident now that we >> isolated the raw NAND code from the generic NAND changes (less risks of >> breaking existing NAND users). > > Lastest version, with fixup patches squashed in original patches is > available here[1]. > > [1]https://github.com/bbrezillon/linux-0day/commits/nand/spi-nand-squashed > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 00/15] A SPI NAND framework under generic NAND framework 2017-12-12 9:58 ` Frieder Schrempf @ 2017-12-13 21:27 ` Boris Brezillon 2017-12-14 6:15 ` Peter Pan 0 siblings, 1 reply; 32+ messages in thread From: Boris Brezillon @ 2017-12-13 21:27 UTC (permalink / raw) To: Frieder Schrempf Cc: Peter Pan 潘栋 (peterpandong), Peter Pan, linux-mtd@lists.infradead.org Hi Frieder, On Tue, 12 Dec 2017 10:58:10 +0100 Frieder Schrempf <frieder.schrempf@exceet.de> wrote: > Hi Boris, > > I spent some time looking at and working with your latest SPI NAND code. > In my previous mail I said, that we have some working code for our 3.14 > kernel based on the "mt29f_spinand" staging driver, but that was wrong > as I got things mixed up in my head. > Actually our codebase was an early version of Peter's code, so the > effort to port it to the latest framework code was not that big. > > I forked your repo and you can find my working tree at [1]. Great, I'll try to have a look. > > What I did so far: > > * Rebase your patches on latest Linux 4.14.5 Cool, rebasing on 4.15-rc1 would be even better, but I can do that if you don't have the time. > > * Add a driver for the Winbond W25M02GV SPI NAND chip, that we have on > some of our boards. Okay, that's actually a good thing to have tested with another chip, This way we can make sure the framework is generic enough. > > * Add a driver for the Freescale QSPI controller, derived from the > existing QSPI-NOR driver at drivers/mtd/spi-nor/fsl-quadspi.c. That's an interesting case as well, the generic NAND controller is probably the easiest implementation, and that's a good thing to have another controller. > > * Add a setup and setup_late op to the controller layer (see [3]). I > don't know if that makes sense, but at least the setup_late is needed > for the FSL QSPI driver, as it needs to know the size of the flash to > setup the memory mapping for the QSPI read operations. Hm, not sure what ->setup_late() is for, but I'll have a look. > > * A bit of testing and fixing two small bugs, which look like copy and > paste mistakes. See [2]. Thanks, I'll squash the fixes in the original commit. > > * Running nandtest successfully on our hardware (i.MX6UL -> FSL-QSPI -> > W25M02GV) > > I hope this is of use while moving on. Definitely. I'd also like to have a review on the framework code if possible, but that can be done when posted on the ML. > I guess you would like to have the basic framework with Micron support > and generic SPI tested and stabilized first, before adding more code, > but to be able to test with our hardware I need Micron and FSL QSPI. I guess you mean Winbond not Micron. That's okay if those patches are posted after the initial series. All I need is reviews and tests from different parties, so that I'm less confident in merging the code. > > Some questions/flaws that occurred to me: > > * The W25M02GV chip has two dies of 128M each. My current driver only > makes use of the first die. The chip expects a die-select command to > switch between the two dies. > What would be the place to implement this? > Can I just add the command and issue it in > winbond_spinand_adjust_cache_op if luns_per_target > 1? It really depends when the die selection happens. I guess it happens in 2 places: when preparing a program/read operation and when doing the transfer to the in-chip cache. Anyway, the die information is already passed in the nand_pos object, so all you'll have to do is create a new hook to customize the SPI command when needed. > > * The FSL QSPI controller has a lookup table that needs to be filled > with command sequences at the time of setup. Therefore the number of > dummy bytes for each command is fixed and in my current implementation > op->dummy_bytes is ignored. > That's not a problem if all chips need the same number of dummy bytes > for each command, but I guess that's not the case, as there is a > _spinand_get_dummy function. We should definitely expose that in a generic way, and on a per-operation basis, so that, after the detection step, the NAND controller can query this information. > > * What is your plan for ECC and BBT? At least enabling the onchip ECC > will be necessary to be able to use the flash properly (e.g. with UBI), > or am I wrong? Well, if all SPI NAND chips are supporting ECC the same way and on-die ECC support is mandatory for SPI NANDs, then supporting that directly in the core is probably the best option. If, on the other hand, you have various implementations, and some SPI controllers have their own ECC engine that you can use with SPI NANDs, then it's probably a better idea to abstract ECC engine in the generic NAND layer. For BBTs, I'd like to have a clean version of the nand_bbt logic that uses all of the helpers exposed by the generic NAND layer. I'd also like to simplify the code if possible. > > * Do you have any special test cases? What do you usually do to test the > code? Well, make sure all the mtd tests are passing (drivers/mtd/tests), and then, the next step is to test it with UBI+UBIFS. But honestly, I'm not so worried, this is new code, and we've isolated from the raw NAND layer, so if it's buggy or instable at the beginning it's not a big deal, it will be noticed and fixed for the next few releases. > > Thanks for your patience and best regards, No problem. Thanks for your work, and I'll try to be more active on this topic (I promised that several times, and failed it :-/). Regards, Boris ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 00/15] A SPI NAND framework under generic NAND framework 2017-12-13 21:27 ` Boris Brezillon @ 2017-12-14 6:15 ` Peter Pan 2017-12-14 7:50 ` Boris Brezillon 0 siblings, 1 reply; 32+ messages in thread From: Peter Pan @ 2017-12-14 6:15 UTC (permalink / raw) To: Boris Brezillon Cc: Frieder Schrempf, Peter Pan 潘栋 (peterpandong), linux-mtd@lists.infradead.org Hi Boris and Frieder, On Thu, Dec 14, 2017 at 5:27 AM, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > Hi Frieder, > > On Tue, 12 Dec 2017 10:58:10 +0100 > Frieder Schrempf <frieder.schrempf@exceet.de> wrote: > >> Hi Boris, >> >> I spent some time looking at and working with your latest SPI NAND code. >> In my previous mail I said, that we have some working code for our 3.14 >> kernel based on the "mt29f_spinand" staging driver, but that was wrong >> as I got things mixed up in my head. >> Actually our codebase was an early version of Peter's code, so the >> effort to port it to the latest framework code was not that big. >> >> I forked your repo and you can find my working tree at [1]. > > Great, I'll try to have a look. > >> >> What I did so far: >> >> * Rebase your patches on latest Linux 4.14.5 > > Cool, rebasing on 4.15-rc1 would be even better, but I can do that if > you don't have the time. > >> >> * Add a driver for the Winbond W25M02GV SPI NAND chip, that we have on >> some of our boards. > > Okay, that's actually a good thing to have tested with another chip, > This way we can make sure the framework is generic enough. > >> >> * Add a driver for the Freescale QSPI controller, derived from the >> existing QSPI-NOR driver at drivers/mtd/spi-nor/fsl-quadspi.c. > > That's an interesting case as well, the generic NAND controller is > probably the easiest implementation, and that's a good thing to have > another controller. > >> >> * Add a setup and setup_late op to the controller layer (see [3]). I >> don't know if that makes sense, but at least the setup_late is needed >> for the FSL QSPI driver, as it needs to know the size of the flash to >> setup the memory mapping for the QSPI read operations. > > Hm, not sure what ->setup_late() is for, but I'll have a look. > >> >> * A bit of testing and fixing two small bugs, which look like copy and >> paste mistakes. See [2]. > > Thanks, I'll squash the fixes in the original commit. > >> >> * Running nandtest successfully on our hardware (i.MX6UL -> FSL-QSPI -> >> W25M02GV) >> >> I hope this is of use while moving on. > > Definitely. I'd also like to have a review on the framework code if > possible, but that can be done when posted on the ML. > >> I guess you would like to have the basic framework with Micron support >> and generic SPI tested and stabilized first, before adding more code, >> but to be able to test with our hardware I need Micron and FSL QSPI. > > I guess you mean Winbond not Micron. That's okay if those patches are > posted after the initial series. All I need is reviews and tests from > different parties, so that I'm less confident in merging the code. > >> >> Some questions/flaws that occurred to me: >> >> * The W25M02GV chip has two dies of 128M each. My current driver only >> makes use of the first die. The chip expects a die-select command to >> switch between the two dies. >> What would be the place to implement this? >> Can I just add the command and issue it in >> winbond_spinand_adjust_cache_op if luns_per_target > 1? > > It really depends when the die selection happens. I guess it happens in > 2 places: when preparing a program/read operation and when doing the > transfer to the in-chip cache. Anyway, the die information is already > passed in the nand_pos object, so all you'll have to do is create a new > hook to customize the SPI command when needed. I don't know whether we can do the the die switch in the generic NAND core (drivers/mtd/nand/core.c). I'm not sure if chip->select_chip() in nand_base.c does the same thing or not. If so, we can do the die switch before read/write/erase operation. And let spi nand core to implement a hook to support it. > >> >> * The FSL QSPI controller has a lookup table that needs to be filled >> with command sequences at the time of setup. Therefore the number of >> dummy bytes for each command is fixed and in my current implementation >> op->dummy_bytes is ignored. >> That's not a problem if all chips need the same number of dummy bytes >> for each command, but I guess that's not the case, as there is a >> _spinand_get_dummy function. > > We should definitely expose that in a generic way, and on a > per-operation basis, so that, after the detection step, the NAND > controller can query this information. > >> >> * What is your plan for ECC and BBT? At least enabling the onchip ECC >> will be necessary to be able to use the flash properly (e.g. with UBI), >> or am I wrong? > > Well, if all SPI NAND chips are supporting ECC the same way and > on-die ECC support is mandatory for SPI NANDs, then supporting that > directly in the core is probably the best option. If, on the other > hand, you have various implementations, and some SPI controllers have > their own ECC engine that you can use with SPI NANDs, then it's > probably a better idea to abstract ECC engine in the generic NAND layer. As far as I know, all the SPI NAND supports on-die ECC (at least all the SPI NAND I heard). But different chips may have different ECC layout in OOB area. But I think this cannot be a problem, we can handle this in vendor's file. > > For BBTs, I'd like to have a clean version of the nand_bbt logic that > uses all of the helpers exposed by the generic NAND layer. I'd also > like to simplify the code if possible. > >> >> * Do you have any special test cases? What do you usually do to test the >> code? > > Well, make sure all the mtd tests are passing (drivers/mtd/tests), and > then, the next step is to test it with UBI+UBIFS. But honestly, I'm not > so worried, this is new code, and we've isolated from the raw NAND > layer, so if it's buggy or instable at the beginning it's not a big > deal, it will be noticed and fixed for the next few releases. > >> >> Thanks for your patience and best regards, > > No problem. Thanks for your work, and I'll try to be more active on > this topic (I promised that several times, and failed it :-/). Boris, I think we need to move forward, MediaTek's engineer has already asked for the status of the spi nand framework upstreaming, since they want to upstream their SPI NAND controller driver. And Hisilicon also has a controller driver. Since more and more platforms start to support SPI NAND, we'd better merge a simple but well designed framework first. I will run your modified version on Zedboard with generic SPI controller this or next week. Thanks, Peter Pan > > Regards, > > Boris ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 00/15] A SPI NAND framework under generic NAND framework 2017-12-14 6:15 ` Peter Pan @ 2017-12-14 7:50 ` Boris Brezillon 2017-12-14 8:06 ` Peter Pan 2017-12-15 2:35 ` Peter Pan 0 siblings, 2 replies; 32+ messages in thread From: Boris Brezillon @ 2017-12-14 7:50 UTC (permalink / raw) To: Peter Pan Cc: Frieder Schrempf, Peter Pan 潘栋 (peterpandong), linux-mtd@lists.infradead.org Hi Peter, On Thu, 14 Dec 2017 14:15:57 +0800 Peter Pan <peterpansjtu@gmail.com> wrote: > Hi Boris and Frieder, > > On Thu, Dec 14, 2017 at 5:27 AM, Boris Brezillon > <boris.brezillon@free-electrons.com> wrote: > > Hi Frieder, > > > > On Tue, 12 Dec 2017 10:58:10 +0100 > > Frieder Schrempf <frieder.schrempf@exceet.de> wrote: > > > >> Hi Boris, > >> > >> I spent some time looking at and working with your latest SPI NAND code. > >> In my previous mail I said, that we have some working code for our 3.14 > >> kernel based on the "mt29f_spinand" staging driver, but that was wrong > >> as I got things mixed up in my head. > >> Actually our codebase was an early version of Peter's code, so the > >> effort to port it to the latest framework code was not that big. > >> > >> I forked your repo and you can find my working tree at [1]. > > > > Great, I'll try to have a look. > > > >> > >> What I did so far: > >> > >> * Rebase your patches on latest Linux 4.14.5 > > > > Cool, rebasing on 4.15-rc1 would be even better, but I can do that if > > you don't have the time. > > > >> > >> * Add a driver for the Winbond W25M02GV SPI NAND chip, that we have on > >> some of our boards. > > > > Okay, that's actually a good thing to have tested with another chip, > > This way we can make sure the framework is generic enough. > > > >> > >> * Add a driver for the Freescale QSPI controller, derived from the > >> existing QSPI-NOR driver at drivers/mtd/spi-nor/fsl-quadspi.c. > > > > That's an interesting case as well, the generic NAND controller is > > probably the easiest implementation, and that's a good thing to have > > another controller. > > > >> > >> * Add a setup and setup_late op to the controller layer (see [3]). I > >> don't know if that makes sense, but at least the setup_late is needed > >> for the FSL QSPI driver, as it needs to know the size of the flash to > >> setup the memory mapping for the QSPI read operations. > > > > Hm, not sure what ->setup_late() is for, but I'll have a look. > > > >> > >> * A bit of testing and fixing two small bugs, which look like copy and > >> paste mistakes. See [2]. > > > > Thanks, I'll squash the fixes in the original commit. > > > >> > >> * Running nandtest successfully on our hardware (i.MX6UL -> FSL-QSPI -> > >> W25M02GV) > >> > >> I hope this is of use while moving on. > > > > Definitely. I'd also like to have a review on the framework code if > > possible, but that can be done when posted on the ML. > > > >> I guess you would like to have the basic framework with Micron support > >> and generic SPI tested and stabilized first, before adding more code, > >> but to be able to test with our hardware I need Micron and FSL QSPI. > > > > I guess you mean Winbond not Micron. That's okay if those patches are > > posted after the initial series. All I need is reviews and tests from > > different parties, so that I'm less confident in merging the code. > > > >> > >> Some questions/flaws that occurred to me: > >> > >> * The W25M02GV chip has two dies of 128M each. My current driver only > >> makes use of the first die. The chip expects a die-select command to > >> switch between the two dies. > >> What would be the place to implement this? > >> Can I just add the command and issue it in > >> winbond_spinand_adjust_cache_op if luns_per_target > 1? > > > > It really depends when the die selection happens. I guess it happens in > > 2 places: when preparing a program/read operation and when doing the > > transfer to the in-chip cache. Anyway, the die information is already > > passed in the nand_pos object, so all you'll have to do is create a new > > hook to customize the SPI command when needed. > > I don't know whether we can do the the die switch in the generic NAND core > (drivers/mtd/nand/core.c). I'm not sure if chip->select_chip() in nand_base.c > does the same thing or not. If so, we can do the die switch before > read/write/erase > operation. And let spi nand core to implement a hook to support it. Actually, I'm trying to move away from this ->select_chip() approach in the raw NAND framework, simply because the controller might be able to parallelize operations (like 2 erase on 2 different dies, or one erase on one die, and a program on the other), and having this ->select_chip() done early in the chain prevents this kind of optimization. Anyway, the controller should now have all the necessary information to know which die an operation should be executed on, and if a specific command has to be sent to the device to select a specific die, it can be done in the NAND vendor specific code. > > > > >> > >> * The FSL QSPI controller has a lookup table that needs to be filled > >> with command sequences at the time of setup. Therefore the number of > >> dummy bytes for each command is fixed and in my current implementation > >> op->dummy_bytes is ignored. > >> That's not a problem if all chips need the same number of dummy bytes > >> for each command, but I guess that's not the case, as there is a > >> _spinand_get_dummy function. > > > > We should definitely expose that in a generic way, and on a > > per-operation basis, so that, after the detection step, the NAND > > controller can query this information. > > > >> > >> * What is your plan for ECC and BBT? At least enabling the onchip ECC > >> will be necessary to be able to use the flash properly (e.g. with UBI), > >> or am I wrong? > > > > Well, if all SPI NAND chips are supporting ECC the same way and > > on-die ECC support is mandatory for SPI NANDs, then supporting that > > directly in the core is probably the best option. If, on the other > > hand, you have various implementations, and some SPI controllers have > > their own ECC engine that you can use with SPI NANDs, then it's > > probably a better idea to abstract ECC engine in the generic NAND layer. > > As far as I know, all the SPI NAND supports on-die ECC (at least all > the SPI NAND > I heard). But different chips may have different ECC layout in OOB > area. But I think > this cannot be a problem, we can handle this in vendor's file. Yep. > > > > > For BBTs, I'd like to have a clean version of the nand_bbt logic that > > uses all of the helpers exposed by the generic NAND layer. I'd also > > like to simplify the code if possible. > > > >> > >> * Do you have any special test cases? What do you usually do to test the > >> code? > > > > Well, make sure all the mtd tests are passing (drivers/mtd/tests), and > > then, the next step is to test it with UBI+UBIFS. But honestly, I'm not > > so worried, this is new code, and we've isolated from the raw NAND > > layer, so if it's buggy or instable at the beginning it's not a big > > deal, it will be noticed and fixed for the next few releases. > > > >> > >> Thanks for your patience and best regards, > > > > No problem. Thanks for your work, and I'll try to be more active on > > this topic (I promised that several times, and failed it :-/). > > Boris, > > I think we need to move forward, MediaTek's engineer has already asked for > the status of the spi nand framework upstreaming, since they want to upstream > their SPI NAND controller driver. And Hisilicon also has a controller driver. > Since more and more platforms start to support SPI NAND, we'd better merge a > simple but well designed framework first. I will run your modified > version on Zedboard > with generic SPI controller this or next week. Then why are they not spending time reviewing the SPI NAND framework? You know I'm not paid to work on SPI NAND, unlike those engineers working for SoC/NAND vendors. While I agree being a maintainer implies doing part of the work on our spare time, what I don't like is when people that did not invest time in a specific feature ask for this feature to be implemented/released. This is not your case, you invested a lot of time in it, and that's the reason I feel bad when I can't spend the necessary time to make SPI NAND code ready for inclusion. So here are the next set of things to do if you want to move forward: 1/ Re-submit the preparation patches (those touching MTD core) and review them (or find someone to review them) 2/ Add the missing doc to the code (I was planning on doing that, but if you're in hurry you can take care of it), and add real commit messages. 3/ Fix the authorship on patches (some were mainly written by you, but during my rework authorship has been lost) 4/ Ask others to test and review the patches Regards, Boris ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 00/15] A SPI NAND framework under generic NAND framework 2017-12-14 7:50 ` Boris Brezillon @ 2017-12-14 8:06 ` Peter Pan 2017-12-14 14:39 ` Frieder Schrempf 2017-12-15 2:35 ` Peter Pan 1 sibling, 1 reply; 32+ messages in thread From: Peter Pan @ 2017-12-14 8:06 UTC (permalink / raw) To: Boris Brezillon Cc: Frieder Schrempf, Peter Pan 潘栋 (peterpandong), linux-mtd@lists.infradead.org Hi Boris, On Thu, Dec 14, 2017 at 3:50 PM, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > Hi Peter, > > On Thu, 14 Dec 2017 14:15:57 +0800 > Peter Pan <peterpansjtu@gmail.com> wrote: > >> Hi Boris and Frieder, >> >> On Thu, Dec 14, 2017 at 5:27 AM, Boris Brezillon >> <boris.brezillon@free-electrons.com> wrote: >> > Hi Frieder, >> > >> > On Tue, 12 Dec 2017 10:58:10 +0100 >> > Frieder Schrempf <frieder.schrempf@exceet.de> wrote: >> > >> >> Hi Boris, >> >> >> >> I spent some time looking at and working with your latest SPI NAND code. >> >> In my previous mail I said, that we have some working code for our 3.14 >> >> kernel based on the "mt29f_spinand" staging driver, but that was wrong >> >> as I got things mixed up in my head. >> >> Actually our codebase was an early version of Peter's code, so the >> >> effort to port it to the latest framework code was not that big. >> >> >> >> I forked your repo and you can find my working tree at [1]. >> > >> > Great, I'll try to have a look. >> > >> >> >> >> What I did so far: >> >> >> >> * Rebase your patches on latest Linux 4.14.5 >> > >> > Cool, rebasing on 4.15-rc1 would be even better, but I can do that if >> > you don't have the time. >> > >> >> >> >> * Add a driver for the Winbond W25M02GV SPI NAND chip, that we have on >> >> some of our boards. >> > >> > Okay, that's actually a good thing to have tested with another chip, >> > This way we can make sure the framework is generic enough. >> > >> >> >> >> * Add a driver for the Freescale QSPI controller, derived from the >> >> existing QSPI-NOR driver at drivers/mtd/spi-nor/fsl-quadspi.c. >> > >> > That's an interesting case as well, the generic NAND controller is >> > probably the easiest implementation, and that's a good thing to have >> > another controller. >> > >> >> >> >> * Add a setup and setup_late op to the controller layer (see [3]). I >> >> don't know if that makes sense, but at least the setup_late is needed >> >> for the FSL QSPI driver, as it needs to know the size of the flash to >> >> setup the memory mapping for the QSPI read operations. >> > >> > Hm, not sure what ->setup_late() is for, but I'll have a look. >> > >> >> >> >> * A bit of testing and fixing two small bugs, which look like copy and >> >> paste mistakes. See [2]. >> > >> > Thanks, I'll squash the fixes in the original commit. >> > >> >> >> >> * Running nandtest successfully on our hardware (i.MX6UL -> FSL-QSPI -> >> >> W25M02GV) >> >> >> >> I hope this is of use while moving on. >> > >> > Definitely. I'd also like to have a review on the framework code if >> > possible, but that can be done when posted on the ML. >> > >> >> I guess you would like to have the basic framework with Micron support >> >> and generic SPI tested and stabilized first, before adding more code, >> >> but to be able to test with our hardware I need Micron and FSL QSPI. >> > >> > I guess you mean Winbond not Micron. That's okay if those patches are >> > posted after the initial series. All I need is reviews and tests from >> > different parties, so that I'm less confident in merging the code. >> > >> >> >> >> Some questions/flaws that occurred to me: >> >> >> >> * The W25M02GV chip has two dies of 128M each. My current driver only >> >> makes use of the first die. The chip expects a die-select command to >> >> switch between the two dies. >> >> What would be the place to implement this? >> >> Can I just add the command and issue it in >> >> winbond_spinand_adjust_cache_op if luns_per_target > 1? >> > >> > It really depends when the die selection happens. I guess it happens in >> > 2 places: when preparing a program/read operation and when doing the >> > transfer to the in-chip cache. Anyway, the die information is already >> > passed in the nand_pos object, so all you'll have to do is create a new >> > hook to customize the SPI command when needed. >> >> I don't know whether we can do the the die switch in the generic NAND core >> (drivers/mtd/nand/core.c). I'm not sure if chip->select_chip() in nand_base.c >> does the same thing or not. If so, we can do the die switch before >> read/write/erase >> operation. And let spi nand core to implement a hook to support it. > > Actually, I'm trying to move away from this ->select_chip() approach in > the raw NAND framework, simply because the controller might be able to > parallelize operations (like 2 erase on 2 different dies, or one erase > on one die, and a program on the other), and having this ->select_chip() > done early in the chain prevents this kind of optimization. > > Anyway, the controller should now have all the necessary information to > know which die an operation should be executed on, and if a specific > command has to be sent to the device to select a specific die, it can > be done in the NAND vendor specific code. Got your point. It makes sense. > >> >> > >> >> >> >> * The FSL QSPI controller has a lookup table that needs to be filled >> >> with command sequences at the time of setup. Therefore the number of >> >> dummy bytes for each command is fixed and in my current implementation >> >> op->dummy_bytes is ignored. >> >> That's not a problem if all chips need the same number of dummy bytes >> >> for each command, but I guess that's not the case, as there is a >> >> _spinand_get_dummy function. >> > >> > We should definitely expose that in a generic way, and on a >> > per-operation basis, so that, after the detection step, the NAND >> > controller can query this information. >> > >> >> >> >> * What is your plan for ECC and BBT? At least enabling the onchip ECC >> >> will be necessary to be able to use the flash properly (e.g. with UBI), >> >> or am I wrong? >> > >> > Well, if all SPI NAND chips are supporting ECC the same way and >> > on-die ECC support is mandatory for SPI NANDs, then supporting that >> > directly in the core is probably the best option. If, on the other >> > hand, you have various implementations, and some SPI controllers have >> > their own ECC engine that you can use with SPI NANDs, then it's >> > probably a better idea to abstract ECC engine in the generic NAND layer. >> >> As far as I know, all the SPI NAND supports on-die ECC (at least all >> the SPI NAND >> I heard). But different chips may have different ECC layout in OOB >> area. But I think >> this cannot be a problem, we can handle this in vendor's file. > > Yep. > >> >> > >> > For BBTs, I'd like to have a clean version of the nand_bbt logic that >> > uses all of the helpers exposed by the generic NAND layer. I'd also >> > like to simplify the code if possible. >> > >> >> >> >> * Do you have any special test cases? What do you usually do to test the >> >> code? >> > >> > Well, make sure all the mtd tests are passing (drivers/mtd/tests), and >> > then, the next step is to test it with UBI+UBIFS. But honestly, I'm not >> > so worried, this is new code, and we've isolated from the raw NAND >> > layer, so if it's buggy or instable at the beginning it's not a big >> > deal, it will be noticed and fixed for the next few releases. >> > >> >> >> >> Thanks for your patience and best regards, >> > >> > No problem. Thanks for your work, and I'll try to be more active on >> > this topic (I promised that several times, and failed it :-/). >> >> Boris, >> >> I think we need to move forward, MediaTek's engineer has already asked for >> the status of the spi nand framework upstreaming, since they want to upstream >> their SPI NAND controller driver. And Hisilicon also has a controller driver. >> Since more and more platforms start to support SPI NAND, we'd better merge a >> simple but well designed framework first. I will run your modified >> version on Zedboard >> with generic SPI controller this or next week. > > Then why are they not spending time reviewing the SPI NAND framework? > You know I'm not paid to work on SPI NAND, unlike those engineers > working for SoC/NAND vendors. While I agree being a maintainer implies > doing part of the work on our spare time, what I don't like is when > people that did not invest time in a specific feature ask for this > feature to be implemented/released. This is not your case, you invested > a lot of time in it, and that's the reason I feel bad when I can't spend > the necessary time to make SPI NAND code ready for inclusion. > > So here are the next set of things to do if you want to move forward: > 1/ Re-submit the preparation patches (those touching MTD core) and > review them (or find someone to review them) > 2/ Add the missing doc to the code (I was planning on doing that, but > if you're in hurry you can take care of it), and add real commit > messages. > 3/ Fix the authorship on patches (some were mainly written by you, but > during my rework authorship has been lost) > 4/ Ask others to test and review the patches To be honest, I also feel bad to keep pushing you on this... I will try to communicate with them to see if they can help us to do some review or valudation task. You already make the path clear, I will take the rest. If anything unclear, I will come back to discuss with you. Thanks Peter Pan > > Regards, > > Boris ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 00/15] A SPI NAND framework under generic NAND framework 2017-12-14 8:06 ` Peter Pan @ 2017-12-14 14:39 ` Frieder Schrempf 2017-12-14 14:43 ` Frieder Schrempf 2017-12-14 15:38 ` Boris Brezillon 0 siblings, 2 replies; 32+ messages in thread From: Frieder Schrempf @ 2017-12-14 14:39 UTC (permalink / raw) To: Peter Pan, Boris Brezillon Cc: Peter Pan 潘栋 (peterpandong), linux-mtd@lists.infradead.org Hello Peter, hello Boris >>>>> What I did so far: >>>>> >>>>> * Rebase your patches on latest Linux 4.14.5 >>>> >>>> Cool, rebasing on 4.15-rc1 would be even better, but I can do that if >>>> you don't have the time. I will try to rebase on 4.15-rc1. Can you give me a quick explanation on which patches are needed for SPI NAND + preparation, if I only want to add the generic NAND and the SPI NAND layer? Previously I just rebased the whole branch, but I think you have some other pending patches in there? Would this be the correct changeset: [1] Or are there other preparation patches needed? >>>>> I guess you would like to have the basic framework with Micron support >>>>> and generic SPI tested and stabilized first, before adding more code, >>>>> but to be able to test with our hardware I need Micron and FSL QSPI. >>>> >>>> I guess you mean Winbond not Micron. That's okay if those patches are >>>> posted after the initial series. All I need is reviews and tests from >>>> different parties, so that I'm less confident in merging the code. Yeah I meant Winbond of course. And I guess you meant "more confident". Or that was just irony? Nevermind ;) >>>>> Some questions/flaws that occurred to me: >>>>> >>>>> * The W25M02GV chip has two dies of 128M each. My current driver only >>>>> makes use of the first die. The chip expects a die-select command to >>>>> switch between the two dies. >>>>> What would be the place to implement this? >>>>> Can I just add the command and issue it in >>>>> winbond_spinand_adjust_cache_op if luns_per_target > 1? >>>> >>>> It really depends when the die selection happens. I guess it happens in >>>> 2 places: when preparing a program/read operation and when doing the >>>> transfer to the in-chip cache. Anyway, the die information is already >>>> passed in the nand_pos object, so all you'll have to do is create a new >>>> hook to customize the SPI command when needed. The die selection is a separate SPI command and not integrated into the program/read/erase sequence. So I can't customize an existing op, but have to issue a new "die select" op. >>> >>> I don't know whether we can do the the die switch in the generic NAND core >>> (drivers/mtd/nand/core.c). I'm not sure if chip->select_chip() in nand_base.c >>> does the same thing or not. If so, we can do the die switch before >>> read/write/erase >>> operation. And let spi nand core to implement a hook to support it. >> >> Actually, I'm trying to move away from this ->select_chip() approach in >> the raw NAND framework, simply because the controller might be able to >> parallelize operations (like 2 erase on 2 different dies, or one erase >> on one die, and a program on the other), and having this ->select_chip() >> done early in the chain prevents this kind of optimization. >> >> Anyway, the controller should now have all the necessary information to >> know which die an operation should be executed on, and if a specific >> command has to be sent to the device to select a specific die, it can >> be done in the NAND vendor specific code. But needing a die select op is quite common for any multi-die chips, isn't it? So shouldn't there be an spinand_die_select_op in the SPI NAND core that is executed when necessary and skipped if there's only one die. > Got your point. It makes sense. >> >>>>> * The FSL QSPI controller has a lookup table that needs to be filled >>>>> with command sequences at the time of setup. Therefore the number of >>>>> dummy bytes for each command is fixed and in my current implementation >>>>> op->dummy_bytes is ignored. >>>>> That's not a problem if all chips need the same number of dummy bytes >>>>> for each command, but I guess that's not the case, as there is a >>>>> _spinand_get_dummy function. >>>> >>>> We should definitely expose that in a generic way, and on a >>>> per-operation basis, so that, after the detection step, the NAND >>>> controller can query this information. Ok, I might try this and see where it leads me. >>>>> * What is your plan for ECC and BBT? At least enabling the onchip ECC >>>>> will be necessary to be able to use the flash properly (e.g. with UBI), >>>>> or am I wrong? >>>> >>>> Well, if all SPI NAND chips are supporting ECC the same way and >>>> on-die ECC support is mandatory for SPI NANDs, then supporting that >>>> directly in the core is probably the best option. If, on the other >>>> hand, you have various implementations, and some SPI controllers have >>>> their own ECC engine that you can use with SPI NANDs, then it's >>>> probably a better idea to abstract ECC engine in the generic NAND layer. >>> >>> As far as I know, all the SPI NAND supports on-die ECC (at least all >>> the SPI NAND >>> I heard). But different chips may have different ECC layout in OOB >>> area. But I think >>> this cannot be a problem, we can handle this in vendor's file. >> >> Yep. Ok. If I have time I will think about how the ECC and OOB layout might be implemented. But I have not much experience here, so if anyone of you can propose something to get started, this would be great. >>>> For BBTs, I'd like to have a clean version of the nand_bbt logic that >>>> uses all of the helpers exposed by the generic NAND layer. I'd also >>>> like to simplify the code if possible. >>>> >>>>> >>>>> * Do you have any special test cases? What do you usually do to test the >>>>> code? >>>> >>>> Well, make sure all the mtd tests are passing (drivers/mtd/tests), and >>>> then, the next step is to test it with UBI+UBIFS. But honestly, I'm not >>>> so worried, this is new code, and we've isolated from the raw NAND >>>> layer, so if it's buggy or instable at the beginning it's not a big >>>> deal, it will be noticed and fixed for the next few releases. >>>> >>>>> >>>>> Thanks for your patience and best regards, >>>> >>>> No problem. Thanks for your work, and I'll try to be more active on >>>> this topic (I promised that several times, and failed it :-/). I hope, that as more people get interested in the topic, more people will join. Thanks in advance for your further work on this. >> So here are the next set of things to do if you want to move forward: >> 1/ Re-submit the preparation patches (those touching MTD core) and >> review them (or find someone to review them) >> 2/ Add the missing doc to the code (I was planning on doing that, but >> if you're in hurry you can take care of it), and add real commit >> messages. >> 3/ Fix the authorship on patches (some were mainly written by you, but >> during my rework authorship has been lost) >> 4/ Ask others to test and review the patches > > To be honest, I also feel bad to keep pushing you on this... > I will try to communicate with them to see if they can help us to do some review > or valudation task. > You already make the path clear, I will take the rest. If anything > unclear, I will come > back to discuss with you. I don't have any experience in reviewing kernel code and only little in submitting patches, but if I can be of any help let me know. As I have a certain use case and the hardware, I will also be happy to help testing. Thanks and regards Frieder ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 00/15] A SPI NAND framework under generic NAND framework 2017-12-14 14:39 ` Frieder Schrempf @ 2017-12-14 14:43 ` Frieder Schrempf 2017-12-14 15:38 ` Boris Brezillon 1 sibling, 0 replies; 32+ messages in thread From: Frieder Schrempf @ 2017-12-14 14:43 UTC (permalink / raw) To: Peter Pan, Boris Brezillon Cc: Peter Pan 潘栋 (peterpandong), linux-mtd@lists.infradead.org Forgot the link: [1]: https://github.com/fschrempf/linux-0day/compare/374c97f3a6b8e83dd1b005a3aec64d7008259185...fschrempf:spi-nand-exceet On 14.12.2017 15:39, Frieder Schrempf wrote: > Hello Peter, hello Boris > >>>>>> What I did so far: >>>>>> >>>>>> * Rebase your patches on latest Linux 4.14.5 >>>>> >>>>> Cool, rebasing on 4.15-rc1 would be even better, but I can do that if >>>>> you don't have the time. > > I will try to rebase on 4.15-rc1. Can you give me a quick explanation on > which patches are needed for SPI NAND + preparation, if I only want to > add the generic NAND and the SPI NAND layer? > Previously I just rebased the whole branch, but I think you have some > other pending patches in there? > Would this be the correct changeset: [1] > Or are there other preparation patches needed? > >>>>>> I guess you would like to have the basic framework with Micron >>>>>> support >>>>>> and generic SPI tested and stabilized first, before adding more code, >>>>>> but to be able to test with our hardware I need Micron and FSL QSPI. >>>>> >>>>> I guess you mean Winbond not Micron. That's okay if those patches are >>>>> posted after the initial series. All I need is reviews and tests from >>>>> different parties, so that I'm less confident in merging the code. > > Yeah I meant Winbond of course. And I guess you meant "more confident". > Or that was just irony? Nevermind ;) > >>>>>> Some questions/flaws that occurred to me: >>>>>> >>>>>> * The W25M02GV chip has two dies of 128M each. My current driver only >>>>>> makes use of the first die. The chip expects a die-select command to >>>>>> switch between the two dies. >>>>>> What would be the place to implement this? >>>>>> Can I just add the command and issue it in >>>>>> winbond_spinand_adjust_cache_op if luns_per_target > 1? >>>>> >>>>> It really depends when the die selection happens. I guess it >>>>> happens in >>>>> 2 places: when preparing a program/read operation and when doing the >>>>> transfer to the in-chip cache. Anyway, the die information is already >>>>> passed in the nand_pos object, so all you'll have to do is create a >>>>> new >>>>> hook to customize the SPI command when needed. > > The die selection is a separate SPI command and not integrated into the > program/read/erase sequence. > So I can't customize an existing op, but have to issue a new "die > select" op. > >>>> >>>> I don't know whether we can do the the die switch in the generic >>>> NAND core >>>> (drivers/mtd/nand/core.c). I'm not sure if chip->select_chip() in >>>> nand_base.c >>>> does the same thing or not. If so, we can do the die switch before >>>> read/write/erase >>>> operation. And let spi nand core to implement a hook to support it. >>> >>> Actually, I'm trying to move away from this ->select_chip() approach in >>> the raw NAND framework, simply because the controller might be able to >>> parallelize operations (like 2 erase on 2 different dies, or one erase >>> on one die, and a program on the other), and having this ->select_chip() >>> done early in the chain prevents this kind of optimization. >>> >>> Anyway, the controller should now have all the necessary information to >>> know which die an operation should be executed on, and if a specific >>> command has to be sent to the device to select a specific die, it can >>> be done in the NAND vendor specific code. > > But needing a die select op is quite common for any multi-die chips, > isn't it? > So shouldn't there be an spinand_die_select_op in the SPI NAND core that > is executed when necessary and skipped if there's only one die. > >> Got your point. It makes sense. >>> >>>>>> * The FSL QSPI controller has a lookup table that needs to be filled >>>>>> with command sequences at the time of setup. Therefore the number of >>>>>> dummy bytes for each command is fixed and in my current >>>>>> implementation >>>>>> op->dummy_bytes is ignored. >>>>>> That's not a problem if all chips need the same number of dummy bytes >>>>>> for each command, but I guess that's not the case, as there is a >>>>>> _spinand_get_dummy function. >>>>> >>>>> We should definitely expose that in a generic way, and on a >>>>> per-operation basis, so that, after the detection step, the NAND >>>>> controller can query this information. > > Ok, I might try this and see where it leads me. > >>>>>> * What is your plan for ECC and BBT? At least enabling the onchip ECC >>>>>> will be necessary to be able to use the flash properly (e.g. with >>>>>> UBI), >>>>>> or am I wrong? >>>>> >>>>> Well, if all SPI NAND chips are supporting ECC the same way and >>>>> on-die ECC support is mandatory for SPI NANDs, then supporting that >>>>> directly in the core is probably the best option. If, on the other >>>>> hand, you have various implementations, and some SPI controllers have >>>>> their own ECC engine that you can use with SPI NANDs, then it's >>>>> probably a better idea to abstract ECC engine in the generic NAND >>>>> layer. >>>> >>>> As far as I know, all the SPI NAND supports on-die ECC (at least all >>>> the SPI NAND >>>> I heard). But different chips may have different ECC layout in OOB >>>> area. But I think >>>> this cannot be a problem, we can handle this in vendor's file. >>> >>> Yep. > > Ok. If I have time I will think about how the ECC and OOB layout might > be implemented. But I have not much experience here, so if anyone of you > can propose something to get started, this would be great. > >>>>> For BBTs, I'd like to have a clean version of the nand_bbt logic that >>>>> uses all of the helpers exposed by the generic NAND layer. I'd also >>>>> like to simplify the code if possible. >>>>> >>>>>> >>>>>> * Do you have any special test cases? What do you usually do to >>>>>> test the >>>>>> code? >>>>> >>>>> Well, make sure all the mtd tests are passing (drivers/mtd/tests), and >>>>> then, the next step is to test it with UBI+UBIFS. But honestly, I'm >>>>> not >>>>> so worried, this is new code, and we've isolated from the raw NAND >>>>> layer, so if it's buggy or instable at the beginning it's not a big >>>>> deal, it will be noticed and fixed for the next few releases. >>>>> >>>>>> >>>>>> Thanks for your patience and best regards, >>>>> >>>>> No problem. Thanks for your work, and I'll try to be more active on >>>>> this topic (I promised that several times, and failed it :-/). > > I hope, that as more people get interested in the topic, more people > will join. Thanks in advance for your further work on this. > >>> So here are the next set of things to do if you want to move forward: >>> 1/ Re-submit the preparation patches (those touching MTD core) and >>> review them (or find someone to review them) >>> 2/ Add the missing doc to the code (I was planning on doing that, but >>> if you're in hurry you can take care of it), and add real commit >>> messages. >>> 3/ Fix the authorship on patches (some were mainly written by you, but >>> during my rework authorship has been lost) >>> 4/ Ask others to test and review the patches >> >> To be honest, I also feel bad to keep pushing you on this... >> I will try to communicate with them to see if they can help us to do >> some review >> or valudation task. >> You already make the path clear, I will take the rest. If anything >> unclear, I will come >> back to discuss with you. > > I don't have any experience in reviewing kernel code and only little in > submitting patches, but if I can be of any help let me know. > > As I have a certain use case and the hardware, I will also be happy to > help testing. > > Thanks and regards > > Frieder ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 00/15] A SPI NAND framework under generic NAND framework 2017-12-14 14:39 ` Frieder Schrempf 2017-12-14 14:43 ` Frieder Schrempf @ 2017-12-14 15:38 ` Boris Brezillon 2017-12-15 1:08 ` Peter Pan 1 sibling, 1 reply; 32+ messages in thread From: Boris Brezillon @ 2017-12-14 15:38 UTC (permalink / raw) To: Frieder Schrempf Cc: Peter Pan, Peter Pan 潘栋 (peterpandong), linux-mtd@lists.infradead.org On Thu, 14 Dec 2017 15:39:13 +0100 Frieder Schrempf <frieder.schrempf@exceet.de> wrote: > Hello Peter, hello Boris > > >>>>> What I did so far: > >>>>> > >>>>> * Rebase your patches on latest Linux 4.14.5 > >>>> > >>>> Cool, rebasing on 4.15-rc1 would be even better, but I can do that if > >>>> you don't have the time. > > I will try to rebase on 4.15-rc1. Can you give me a quick explanation on > which patches are needed for SPI NAND + preparation, if I only want to > add the generic NAND and the SPI NAND layer? > Previously I just rebased the whole branch, but I think you have some > other pending patches in there? > Would this be the correct changeset: [1] > Or are there other preparation patches needed? Looks good. By preparation patches I meant from: "mtd: Do not allow MTD devices with inconsistent erase properties" to "mtd: nand: move raw NAND related code to the raw/ subdir" so basically everything that touches existing code. > > >>>>> I guess you would like to have the basic framework with Micron support > >>>>> and generic SPI tested and stabilized first, before adding more code, > >>>>> but to be able to test with our hardware I need Micron and FSL QSPI. > >>>> > >>>> I guess you mean Winbond not Micron. That's okay if those patches are > >>>> posted after the initial series. All I need is reviews and tests from > >>>> different parties, so that I'm less confident in merging the code. > > Yeah I meant Winbond of course. And I guess you meant "more confident". Yep. > Or that was just irony? Nevermind ;) > > >>>>> Some questions/flaws that occurred to me: > >>>>> > >>>>> * The W25M02GV chip has two dies of 128M each. My current driver only > >>>>> makes use of the first die. The chip expects a die-select command to > >>>>> switch between the two dies. > >>>>> What would be the place to implement this? > >>>>> Can I just add the command and issue it in > >>>>> winbond_spinand_adjust_cache_op if luns_per_target > 1? > >>>> > >>>> It really depends when the die selection happens. I guess it happens in > >>>> 2 places: when preparing a program/read operation and when doing the > >>>> transfer to the in-chip cache. Anyway, the die information is already > >>>> passed in the nand_pos object, so all you'll have to do is create a new > >>>> hook to customize the SPI command when needed. > > The die selection is a separate SPI command and not integrated into the > program/read/erase sequence. > So I can't customize an existing op, but have to issue a new "die > select" op. Okay. > > >>> > >>> I don't know whether we can do the the die switch in the generic NAND core > >>> (drivers/mtd/nand/core.c). I'm not sure if chip->select_chip() in nand_base.c > >>> does the same thing or not. If so, we can do the die switch before > >>> read/write/erase > >>> operation. And let spi nand core to implement a hook to support it. > >> > >> Actually, I'm trying to move away from this ->select_chip() approach in > >> the raw NAND framework, simply because the controller might be able to > >> parallelize operations (like 2 erase on 2 different dies, or one erase > >> on one die, and a program on the other), and having this ->select_chip() > >> done early in the chain prevents this kind of optimization. > >> > >> Anyway, the controller should now have all the necessary information to > >> know which die an operation should be executed on, and if a specific > >> command has to be sent to the device to select a specific die, it can > >> be done in the NAND vendor specific code. > > But needing a die select op is quite common for any multi-die chips, > isn't it? It is, it's just that some controllers are able to interleave operations on multiple dies, so having ->{select,unselect}_chip() methods at the generic NAND level is not such a good idea, because that means you'll have to serialize accesses, even if they could be done in parallel. > So shouldn't there be an spinand_die_select_op in the SPI NAND core that > is executed when necessary and skipped if there's only one die. Sure, I was arguing against a ->select_chip() at the generic NAND level. That's something you can add in the SPI NAND framework. > > > Got your point. It makes sense. > >> > >>>>> * The FSL QSPI controller has a lookup table that needs to be filled > >>>>> with command sequences at the time of setup. Therefore the number of > >>>>> dummy bytes for each command is fixed and in my current implementation > >>>>> op->dummy_bytes is ignored. > >>>>> That's not a problem if all chips need the same number of dummy bytes > >>>>> for each command, but I guess that's not the case, as there is a > >>>>> _spinand_get_dummy function. > >>>> > >>>> We should definitely expose that in a generic way, and on a > >>>> per-operation basis, so that, after the detection step, the NAND > >>>> controller can query this information. > > Ok, I might try this and see where it leads me. > > >>>>> * What is your plan for ECC and BBT? At least enabling the onchip ECC > >>>>> will be necessary to be able to use the flash properly (e.g. with UBI), > >>>>> or am I wrong? > >>>> > >>>> Well, if all SPI NAND chips are supporting ECC the same way and > >>>> on-die ECC support is mandatory for SPI NANDs, then supporting that > >>>> directly in the core is probably the best option. If, on the other > >>>> hand, you have various implementations, and some SPI controllers have > >>>> their own ECC engine that you can use with SPI NANDs, then it's > >>>> probably a better idea to abstract ECC engine in the generic NAND layer. > >>> > >>> As far as I know, all the SPI NAND supports on-die ECC (at least all > >>> the SPI NAND > >>> I heard). But different chips may have different ECC layout in OOB > >>> area. But I think > >>> this cannot be a problem, we can handle this in vendor's file. > >> > >> Yep. > > Ok. If I have time I will think about how the ECC and OOB layout might > be implemented. But I have not much experience here, so if anyone of you > can propose something to get started, this would be great. I can definitely help there, and Peter should be able to give some insights as well. > > >>>> For BBTs, I'd like to have a clean version of the nand_bbt logic that > >>>> uses all of the helpers exposed by the generic NAND layer. I'd also > >>>> like to simplify the code if possible. > >>>> > >>>>> > >>>>> * Do you have any special test cases? What do you usually do to test the > >>>>> code? > >>>> > >>>> Well, make sure all the mtd tests are passing (drivers/mtd/tests), and > >>>> then, the next step is to test it with UBI+UBIFS. But honestly, I'm not > >>>> so worried, this is new code, and we've isolated from the raw NAND > >>>> layer, so if it's buggy or instable at the beginning it's not a big > >>>> deal, it will be noticed and fixed for the next few releases. > >>>> > >>>>> > >>>>> Thanks for your patience and best regards, > >>>> > >>>> No problem. Thanks for your work, and I'll try to be more active on > >>>> this topic (I promised that several times, and failed it :-/). > > I hope, that as more people get interested in the topic, more people > will join. Thanks in advance for your further work on this. > > >> So here are the next set of things to do if you want to move forward: > >> 1/ Re-submit the preparation patches (those touching MTD core) and > >> review them (or find someone to review them) > >> 2/ Add the missing doc to the code (I was planning on doing that, but > >> if you're in hurry you can take care of it), and add real commit > >> messages. > >> 3/ Fix the authorship on patches (some were mainly written by you, but > >> during my rework authorship has been lost) > >> 4/ Ask others to test and review the patches > > > > To be honest, I also feel bad to keep pushing you on this... > > I will try to communicate with them to see if they can help us to do some review > > or valudation task. > > You already make the path clear, I will take the rest. If anything > > unclear, I will come > > back to discuss with you. > > I don't have any experience in reviewing kernel code and only little in > submitting patches, but if I can be of any help let me know. > > As I have a certain use case and the hardware, I will also be happy to > help testing. Testing and reporting issues is already helpful. Thanks, Boris ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 00/15] A SPI NAND framework under generic NAND framework 2017-12-14 15:38 ` Boris Brezillon @ 2017-12-15 1:08 ` Peter Pan 2017-12-15 1:21 ` Peter Pan 0 siblings, 1 reply; 32+ messages in thread From: Peter Pan @ 2017-12-15 1:08 UTC (permalink / raw) To: Boris Brezillon Cc: Frieder Schrempf, Peter Pan 潘栋 (peterpandong), linux-mtd@lists.infradead.org Hi Boris and Frieder On Thu, Dec 14, 2017 at 11:38 PM, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > On Thu, 14 Dec 2017 15:39:13 +0100 > Frieder Schrempf <frieder.schrempf@exceet.de> wrote: > >> Hello Peter, hello Boris >> >> >>>>> What I did so far: >> >>>>> >> >>>>> * Rebase your patches on latest Linux 4.14.5 >> >>>> >> >>>> Cool, rebasing on 4.15-rc1 would be even better, but I can do that if >> >>>> you don't have the time. >> >> I will try to rebase on 4.15-rc1. Can you give me a quick explanation on >> which patches are needed for SPI NAND + preparation, if I only want to >> add the generic NAND and the SPI NAND layer? >> Previously I just rebased the whole branch, but I think you have some >> other pending patches in there? >> Would this be the correct changeset: [1] >> Or are there other preparation patches needed? > > Looks good. By preparation patches I meant > > from: > "mtd: Do not allow MTD devices with inconsistent erase properties" > to > "mtd: nand: move raw NAND related code to the raw/ subdir" > > so basically everything that touches existing code. I think "mtd: Add sanity checks in mtd_write/read_oob()" is also included since I cannot find it in nand/next branch of l2-mtd > >> >> >>>>> I guess you would like to have the basic framework with Micron support >> >>>>> and generic SPI tested and stabilized first, before adding more code, >> >>>>> but to be able to test with our hardware I need Micron and FSL QSPI. >> >>>> >> >>>> I guess you mean Winbond not Micron. That's okay if those patches are >> >>>> posted after the initial series. All I need is reviews and tests from >> >>>> different parties, so that I'm less confident in merging the code. >> >> Yeah I meant Winbond of course. And I guess you meant "more confident". > > Yep. > >> Or that was just irony? Nevermind ;) >> >> >>>>> Some questions/flaws that occurred to me: >> >>>>> >> >>>>> * The W25M02GV chip has two dies of 128M each. My current driver only >> >>>>> makes use of the first die. The chip expects a die-select command to >> >>>>> switch between the two dies. >> >>>>> What would be the place to implement this? >> >>>>> Can I just add the command and issue it in >> >>>>> winbond_spinand_adjust_cache_op if luns_per_target > 1? >> >>>> >> >>>> It really depends when the die selection happens. I guess it happens in >> >>>> 2 places: when preparing a program/read operation and when doing the >> >>>> transfer to the in-chip cache. Anyway, the die information is already >> >>>> passed in the nand_pos object, so all you'll have to do is create a new >> >>>> hook to customize the SPI command when needed. >> >> The die selection is a separate SPI command and not integrated into the >> program/read/erase sequence. >> So I can't customize an existing op, but have to issue a new "die >> select" op. > > Okay. > >> >> >>> >> >>> I don't know whether we can do the the die switch in the generic NAND core >> >>> (drivers/mtd/nand/core.c). I'm not sure if chip->select_chip() in nand_base.c >> >>> does the same thing or not. If so, we can do the die switch before >> >>> read/write/erase >> >>> operation. And let spi nand core to implement a hook to support it. >> >> >> >> Actually, I'm trying to move away from this ->select_chip() approach in >> >> the raw NAND framework, simply because the controller might be able to >> >> parallelize operations (like 2 erase on 2 different dies, or one erase >> >> on one die, and a program on the other), and having this ->select_chip() >> >> done early in the chain prevents this kind of optimization. >> >> >> >> Anyway, the controller should now have all the necessary information to >> >> know which die an operation should be executed on, and if a specific >> >> command has to be sent to the device to select a specific die, it can >> >> be done in the NAND vendor specific code. >> >> But needing a die select op is quite common for any multi-die chips, >> isn't it? > > It is, it's just that some controllers are able to interleave > operations on multiple dies, so having ->{select,unselect}_chip() > methods at the generic NAND level is not such a good idea, because that > means you'll have to serialize accesses, even if they could be done in > parallel. > >> So shouldn't there be an spinand_die_select_op in the SPI NAND core that >> is executed when necessary and skipped if there's only one die. > > Sure, I was arguing against a ->select_chip() at the generic NAND > level. That's something you can add in the SPI NAND framework. > >> >> > Got your point. It makes sense. >> >> >> >>>>> * The FSL QSPI controller has a lookup table that needs to be filled >> >>>>> with command sequences at the time of setup. Therefore the number of >> >>>>> dummy bytes for each command is fixed and in my current implementation >> >>>>> op->dummy_bytes is ignored. >> >>>>> That's not a problem if all chips need the same number of dummy bytes >> >>>>> for each command, but I guess that's not the case, as there is a >> >>>>> _spinand_get_dummy function. >> >>>> >> >>>> We should definitely expose that in a generic way, and on a >> >>>> per-operation basis, so that, after the detection step, the NAND >> >>>> controller can query this information. >> >> Ok, I might try this and see where it leads me. >> >> >>>>> * What is your plan for ECC and BBT? At least enabling the onchip ECC >> >>>>> will be necessary to be able to use the flash properly (e.g. with UBI), >> >>>>> or am I wrong? >> >>>> >> >>>> Well, if all SPI NAND chips are supporting ECC the same way and >> >>>> on-die ECC support is mandatory for SPI NANDs, then supporting that >> >>>> directly in the core is probably the best option. If, on the other >> >>>> hand, you have various implementations, and some SPI controllers have >> >>>> their own ECC engine that you can use with SPI NANDs, then it's >> >>>> probably a better idea to abstract ECC engine in the generic NAND layer. >> >>> >> >>> As far as I know, all the SPI NAND supports on-die ECC (at least all >> >>> the SPI NAND >> >>> I heard). But different chips may have different ECC layout in OOB >> >>> area. But I think >> >>> this cannot be a problem, we can handle this in vendor's file. >> >> >> >> Yep. >> >> Ok. If I have time I will think about how the ECC and OOB layout might >> be implemented. But I have not much experience here, so if anyone of you >> can propose something to get started, this would be great. > > I can definitely help there, and Peter should be able to give some > insights as well. Frieder. Actually the v5 patchset of SPI NAND framework from me[1] includes on-die ECC. You can have a reference (maybe patch 2 and patch 4 is enough). Of course v5 is quite different with the latest code, but I think you can get some idea from it. > >> >> >>>> For BBTs, I'd like to have a clean version of the nand_bbt logic that >> >>>> uses all of the helpers exposed by the generic NAND layer. I'd also >> >>>> like to simplify the code if possible. >> >>>> >> >>>>> >> >>>>> * Do you have any special test cases? What do you usually do to test the >> >>>>> code? >> >>>> >> >>>> Well, make sure all the mtd tests are passing (drivers/mtd/tests), and >> >>>> then, the next step is to test it with UBI+UBIFS. But honestly, I'm not >> >>>> so worried, this is new code, and we've isolated from the raw NAND >> >>>> layer, so if it's buggy or instable at the beginning it's not a big >> >>>> deal, it will be noticed and fixed for the next few releases. >> >>>> >> >>>>> >> >>>>> Thanks for your patience and best regards, >> >>>> >> >>>> No problem. Thanks for your work, and I'll try to be more active on >> >>>> this topic (I promised that several times, and failed it :-/). >> >> I hope, that as more people get interested in the topic, more people >> will join. Thanks in advance for your further work on this. >> >> >> So here are the next set of things to do if you want to move forward: >> >> 1/ Re-submit the preparation patches (those touching MTD core) and >> >> review them (or find someone to review them) >> >> 2/ Add the missing doc to the code (I was planning on doing that, but >> >> if you're in hurry you can take care of it), and add real commit >> >> messages. >> >> 3/ Fix the authorship on patches (some were mainly written by you, but >> >> during my rework authorship has been lost) >> >> 4/ Ask others to test and review the patches >> > >> > To be honest, I also feel bad to keep pushing you on this... >> > I will try to communicate with them to see if they can help us to do some review >> > or valudation task. >> > You already make the path clear, I will take the rest. If anything >> > unclear, I will come >> > back to discuss with you. >> >> I don't have any experience in reviewing kernel code and only little in >> submitting patches, but if I can be of any help let me know. >> >> As I have a certain use case and the hardware, I will also be happy to >> help testing. > > Testing and reporting issues is already helpful. I would like to thank you for your help in advance, Frieder. Thanks, Peter Pan [1] http://patchwork.ozlabs.org/project/linux-mtd/list/?series=&submitter=65489&state=9&q=v5&archive=&delegate= > > Thanks, > > Boris ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 00/15] A SPI NAND framework under generic NAND framework 2017-12-15 1:08 ` Peter Pan @ 2017-12-15 1:21 ` Peter Pan 2017-12-21 11:48 ` Frieder Schrempf 0 siblings, 1 reply; 32+ messages in thread From: Peter Pan @ 2017-12-15 1:21 UTC (permalink / raw) To: Boris Brezillon Cc: Frieder Schrempf, Peter Pan 潘栋 (peterpandong), linux-mtd@lists.infradead.org Hi Boris, On Fri, Dec 15, 2017 at 9:08 AM, Peter Pan <peterpansjtu@gmail.com> wrote: > Hi Boris and Frieder > > On Thu, Dec 14, 2017 at 11:38 PM, Boris Brezillon > <boris.brezillon@free-electrons.com> wrote: >> On Thu, 14 Dec 2017 15:39:13 +0100 >> Frieder Schrempf <frieder.schrempf@exceet.de> wrote: >> >>> Hello Peter, hello Boris >>> >>> >>>>> What I did so far: >>> >>>>> >>> >>>>> * Rebase your patches on latest Linux 4.14.5 >>> >>>> >>> >>>> Cool, rebasing on 4.15-rc1 would be even better, but I can do that if >>> >>>> you don't have the time. >>> >>> I will try to rebase on 4.15-rc1. Can you give me a quick explanation on >>> which patches are needed for SPI NAND + preparation, if I only want to >>> add the generic NAND and the SPI NAND layer? >>> Previously I just rebased the whole branch, but I think you have some >>> other pending patches in there? >>> Would this be the correct changeset: [1] >>> Or are there other preparation patches needed? >> >> Looks good. By preparation patches I meant >> >> from: >> "mtd: Do not allow MTD devices with inconsistent erase properties" >> to >> "mtd: nand: move raw NAND related code to the raw/ subdir" >> >> so basically everything that touches existing code. > > I think "mtd: Add sanity checks in mtd_write/read_oob()" is also > included since I cannot > find it in nand/next branch of l2-mtd Sorry, my mistake. it is in nand/next. > >> >>> >>> >>>>> I guess you would like to have the basic framework with Micron support >>> >>>>> and generic SPI tested and stabilized first, before adding more code, >>> >>>>> but to be able to test with our hardware I need Micron and FSL QSPI. >>> >>>> >>> >>>> I guess you mean Winbond not Micron. That's okay if those patches are >>> >>>> posted after the initial series. All I need is reviews and tests from >>> >>>> different parties, so that I'm less confident in merging the code. >>> >>> Yeah I meant Winbond of course. And I guess you meant "more confident". >> >> Yep. >> >>> Or that was just irony? Nevermind ;) >>> >>> >>>>> Some questions/flaws that occurred to me: >>> >>>>> >>> >>>>> * The W25M02GV chip has two dies of 128M each. My current driver only >>> >>>>> makes use of the first die. The chip expects a die-select command to >>> >>>>> switch between the two dies. >>> >>>>> What would be the place to implement this? >>> >>>>> Can I just add the command and issue it in >>> >>>>> winbond_spinand_adjust_cache_op if luns_per_target > 1? >>> >>>> >>> >>>> It really depends when the die selection happens. I guess it happens in >>> >>>> 2 places: when preparing a program/read operation and when doing the >>> >>>> transfer to the in-chip cache. Anyway, the die information is already >>> >>>> passed in the nand_pos object, so all you'll have to do is create a new >>> >>>> hook to customize the SPI command when needed. >>> >>> The die selection is a separate SPI command and not integrated into the >>> program/read/erase sequence. >>> So I can't customize an existing op, but have to issue a new "die >>> select" op. >> >> Okay. >> >>> >>> >>> >>> >>> I don't know whether we can do the the die switch in the generic NAND core >>> >>> (drivers/mtd/nand/core.c). I'm not sure if chip->select_chip() in nand_base.c >>> >>> does the same thing or not. If so, we can do the die switch before >>> >>> read/write/erase >>> >>> operation. And let spi nand core to implement a hook to support it. >>> >> >>> >> Actually, I'm trying to move away from this ->select_chip() approach in >>> >> the raw NAND framework, simply because the controller might be able to >>> >> parallelize operations (like 2 erase on 2 different dies, or one erase >>> >> on one die, and a program on the other), and having this ->select_chip() >>> >> done early in the chain prevents this kind of optimization. >>> >> >>> >> Anyway, the controller should now have all the necessary information to >>> >> know which die an operation should be executed on, and if a specific >>> >> command has to be sent to the device to select a specific die, it can >>> >> be done in the NAND vendor specific code. >>> >>> But needing a die select op is quite common for any multi-die chips, >>> isn't it? >> >> It is, it's just that some controllers are able to interleave >> operations on multiple dies, so having ->{select,unselect}_chip() >> methods at the generic NAND level is not such a good idea, because that >> means you'll have to serialize accesses, even if they could be done in >> parallel. >> >>> So shouldn't there be an spinand_die_select_op in the SPI NAND core that >>> is executed when necessary and skipped if there's only one die. >> >> Sure, I was arguing against a ->select_chip() at the generic NAND >> level. That's something you can add in the SPI NAND framework. >> >>> >>> > Got your point. It makes sense. >>> >> >>> >>>>> * The FSL QSPI controller has a lookup table that needs to be filled >>> >>>>> with command sequences at the time of setup. Therefore the number of >>> >>>>> dummy bytes for each command is fixed and in my current implementation >>> >>>>> op->dummy_bytes is ignored. >>> >>>>> That's not a problem if all chips need the same number of dummy bytes >>> >>>>> for each command, but I guess that's not the case, as there is a >>> >>>>> _spinand_get_dummy function. >>> >>>> >>> >>>> We should definitely expose that in a generic way, and on a >>> >>>> per-operation basis, so that, after the detection step, the NAND >>> >>>> controller can query this information. >>> >>> Ok, I might try this and see where it leads me. >>> >>> >>>>> * What is your plan for ECC and BBT? At least enabling the onchip ECC >>> >>>>> will be necessary to be able to use the flash properly (e.g. with UBI), >>> >>>>> or am I wrong? >>> >>>> >>> >>>> Well, if all SPI NAND chips are supporting ECC the same way and >>> >>>> on-die ECC support is mandatory for SPI NANDs, then supporting that >>> >>>> directly in the core is probably the best option. If, on the other >>> >>>> hand, you have various implementations, and some SPI controllers have >>> >>>> their own ECC engine that you can use with SPI NANDs, then it's >>> >>>> probably a better idea to abstract ECC engine in the generic NAND layer. >>> >>> >>> >>> As far as I know, all the SPI NAND supports on-die ECC (at least all >>> >>> the SPI NAND >>> >>> I heard). But different chips may have different ECC layout in OOB >>> >>> area. But I think >>> >>> this cannot be a problem, we can handle this in vendor's file. >>> >> >>> >> Yep. >>> >>> Ok. If I have time I will think about how the ECC and OOB layout might >>> be implemented. But I have not much experience here, so if anyone of you >>> can propose something to get started, this would be great. >> >> I can definitely help there, and Peter should be able to give some >> insights as well. > > Frieder. > > Actually the v5 patchset of SPI NAND framework from me[1] includes on-die ECC. > You can have a reference (maybe patch 2 and patch 4 is enough). > Of course v5 is quite different with the latest code, but I think you > can get some idea from it. > >> >>> >>> >>>> For BBTs, I'd like to have a clean version of the nand_bbt logic that >>> >>>> uses all of the helpers exposed by the generic NAND layer. I'd also >>> >>>> like to simplify the code if possible. >>> >>>> >>> >>>>> >>> >>>>> * Do you have any special test cases? What do you usually do to test the >>> >>>>> code? >>> >>>> >>> >>>> Well, make sure all the mtd tests are passing (drivers/mtd/tests), and >>> >>>> then, the next step is to test it with UBI+UBIFS. But honestly, I'm not >>> >>>> so worried, this is new code, and we've isolated from the raw NAND >>> >>>> layer, so if it's buggy or instable at the beginning it's not a big >>> >>>> deal, it will be noticed and fixed for the next few releases. >>> >>>> >>> >>>>> >>> >>>>> Thanks for your patience and best regards, >>> >>>> >>> >>>> No problem. Thanks for your work, and I'll try to be more active on >>> >>>> this topic (I promised that several times, and failed it :-/). >>> >>> I hope, that as more people get interested in the topic, more people >>> will join. Thanks in advance for your further work on this. >>> >>> >> So here are the next set of things to do if you want to move forward: >>> >> 1/ Re-submit the preparation patches (those touching MTD core) and >>> >> review them (or find someone to review them) >>> >> 2/ Add the missing doc to the code (I was planning on doing that, but >>> >> if you're in hurry you can take care of it), and add real commit >>> >> messages. >>> >> 3/ Fix the authorship on patches (some were mainly written by you, but >>> >> during my rework authorship has been lost) >>> >> 4/ Ask others to test and review the patches >>> > >>> > To be honest, I also feel bad to keep pushing you on this... >>> > I will try to communicate with them to see if they can help us to do some review >>> > or valudation task. >>> > You already make the path clear, I will take the rest. If anything >>> > unclear, I will come >>> > back to discuss with you. >>> >>> I don't have any experience in reviewing kernel code and only little in >>> submitting patches, but if I can be of any help let me know. >>> >>> As I have a certain use case and the hardware, I will also be happy to >>> help testing. >> >> Testing and reporting issues is already helpful. > > I would like to thank you for your help in advance, Frieder. > > > Thanks, > Peter Pan > > [1] http://patchwork.ozlabs.org/project/linux-mtd/list/?series=&submitter=65489&state=9&q=v5&archive=&delegate= > >> >> Thanks, >> >> Boris ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 00/15] A SPI NAND framework under generic NAND framework 2017-12-15 1:21 ` Peter Pan @ 2017-12-21 11:48 ` Frieder Schrempf 2017-12-21 13:01 ` Boris Brezillon 2017-12-22 0:49 ` Peter Pan 0 siblings, 2 replies; 32+ messages in thread From: Frieder Schrempf @ 2017-12-21 11:48 UTC (permalink / raw) To: Boris Brezillon Cc: Peter Pan, Peter Pan 潘栋 (peterpandong), linux-mtd@lists.infradead.org Hello Boris, >>>> So shouldn't there be an spinand_die_select_op in the SPI NAND core that >>>> is executed when necessary and skipped if there's only one die. >>> >>> Sure, I was arguing against a ->select_chip() at the generic NAND >>> level. That's something you can add in the SPI NAND framework. I added an op to send the die selection command and call it, where I think it is needed: [1] Accessing both dies on the Winbond SPI NAND works fine like this. While running tests I came across some problems: * On accessing the BBT in RAM via nanddev_bbt_[set/get]_block_status, the calculation of position and offset seems to be wrong. My fix is here: [2] * On calculating the row address for read/program/erase via nanddev_pos_to_row, it seems like the eraseblock_addr_shift is wrong. * Also, I'm not sure if the LUN should be taken into account when calculating the row address. At least when you select the LUN by issuing a separate command, the row address sent to the chip should only contain the page address. But I'm not sure if that's the case in general, or if some code is needed to differentiate. See my changes of nanddev_pos_to_row here: [3] * I run into a mutex deadlock, when spinand_write_page fails (e.g. because of a bad block) as the lock is not released in such cases. See my fix here: [4] With these fixes applied and as far as I can tell everything seems to work fine. I'll do some tests with UBI next and look into the ECC topic. Regards, Frieder [1]: https://github.com/fschrempf/linux-0day/compare/4f8472ea05f27d55ebb2e42eadc9ed59d7036f4c...fschrempf:2c5471c1f1e04fa91ff80c95276b4828b99c4f94 [2]: https://github.com/fschrempf/linux-0day/commit/937a16248570368fe05e15b1f70e753e202b6ae6 [3]: https://github.com/fschrempf/linux-0day/commit/8013696e0394960bfc36625d277f0d2425262939 [4]: https://github.com/fschrempf/linux-0day/commit/28ffc7211745dd124702358c24ddccff8e6f2d95 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 00/15] A SPI NAND framework under generic NAND framework 2017-12-21 11:48 ` Frieder Schrempf @ 2017-12-21 13:01 ` Boris Brezillon 2017-12-21 13:54 ` Frieder Schrempf 2017-12-22 0:49 ` Peter Pan 1 sibling, 1 reply; 32+ messages in thread From: Boris Brezillon @ 2017-12-21 13:01 UTC (permalink / raw) To: Frieder Schrempf Cc: Peter Pan, Peter Pan 潘栋 (peterpandong), linux-mtd@lists.infradead.org Hi Frieder, On Thu, 21 Dec 2017 12:48:16 +0100 Frieder Schrempf <frieder.schrempf@exceet.de> wrote: > Hello Boris, > > >>>> So shouldn't there be an spinand_die_select_op in the SPI NAND core that > >>>> is executed when necessary and skipped if there's only one die. > >>> > >>> Sure, I was arguing against a ->select_chip() at the generic NAND > >>> level. That's something you can add in the SPI NAND framework. > > I added an op to send the die selection command and call it, where I > think it is needed: [1] > > Accessing both dies on the Winbond SPI NAND works fine like this. Okay, I'll have a look. > > While running tests I came across some problems: > > * On accessing the BBT in RAM via nanddev_bbt_[set/get]_block_status, > the calculation of position and offset seems to be wrong. > My fix is here: [2] Indeed. > > * On calculating the row address for read/program/erase via > nanddev_pos_to_row, it seems like the eraseblock_addr_shift is wrong. My version was incorrect, but yours is not good either :-), should be: nand->rowconv.eraseblock_addr_shift = fls(memorg->pages_per_eraseblock - 1); otherwise it doesn't work when the number of pages per block is not a power of 2, and that can happen :-/. > > * Also, I'm not sure if the LUN should be taken into account when > calculating the row address. At least when you select the LUN by issuing > a separate command, the row address sent to the chip should only contain > the page address. The LUN id is part of the row address on parallel ONFI NANDs. Are you sure what you're trying to access is actually a LUN? The ONFI spec says: " LUN (logical unit number) The minimum unit that can independently execute commands and report status. There are one or more LUNs per NAND Target. " I suspect what you're trying to expose is a chip with 2 targets (target is a synonym for die). If this is the case, then you should have: luns_per_target = 1; ntargets = 2; /* ntargets <=> number of dies */ > But I'm not sure if that's the case in general, or if some code is > needed to differentiate. > > See my changes of nanddev_pos_to_row here: [3] > > * I run into a mutex deadlock, when spinand_write_page fails (e.g. > because of a bad block) as the lock is not released in such cases. See > my fix here: [4] This is a valid fix. > > With these fixes applied and as far as I can tell everything seems to > work fine. I'll do some tests with UBI next and look into the ECC topic. Okay cool. I'll squash your fixes in the original commits and push an updated version on my repo. Thanks, Boris ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 00/15] A SPI NAND framework under generic NAND framework 2017-12-21 13:01 ` Boris Brezillon @ 2017-12-21 13:54 ` Frieder Schrempf 0 siblings, 0 replies; 32+ messages in thread From: Frieder Schrempf @ 2017-12-21 13:54 UTC (permalink / raw) To: Boris Brezillon Cc: Peter Pan, Peter Pan 潘栋 (peterpandong), linux-mtd@lists.infradead.org Hi Boris, On 21.12.2017 14:01, Boris Brezillon wrote: > Hi Frieder, > > On Thu, 21 Dec 2017 12:48:16 +0100 > Frieder Schrempf <frieder.schrempf@exceet.de> wrote: >> >> * On calculating the row address for read/program/erase via >> nanddev_pos_to_row, it seems like the eraseblock_addr_shift is wrong. > > My version was incorrect, but yours is not good either :-), should > be: > > nand->rowconv.eraseblock_addr_shift = > fls(memorg->pages_per_eraseblock - 1); > > otherwise it doesn't work when the number of pages per block is not a > power of 2, and that can happen :-/. Ok, makes sense. >> * Also, I'm not sure if the LUN should be taken into account when >> calculating the row address. At least when you select the LUN by issuing >> a separate command, the row address sent to the chip should only contain >> the page address. > > The LUN id is part of the row address on parallel ONFI NANDs. Are you > sure what you're trying to access is actually a LUN? > > The ONFI spec says: > " > LUN (logical unit number) > The minimum unit that can independently execute commands and report > status. There are one or more LUNs per NAND Target. > " > > I suspect what you're trying to expose is a chip with 2 targets > (target is a synonym for die). If this is the case, then you should > have: > > luns_per_target = 1; > ntargets = 2; /* ntargets <=> number of dies */ Indeed. It seems like I misunderstood the meaning of LUN. Thank you for explaining. I fixed my chip setup and refactored my die/target selection code accordingly. > >> But I'm not sure if that's the case in general, or if some code is >> needed to differentiate. >> >> See my changes of nanddev_pos_to_row here: [3] >> >> * I run into a mutex deadlock, when spinand_write_page fails (e.g. >> because of a bad block) as the lock is not released in such cases. See >> my fix here: [4] > > This is a valid fix. > >> >> With these fixes applied and as far as I can tell everything seems to >> work fine. I'll do some tests with UBI next and look into the ECC topic. > > Okay cool. > > I'll squash your fixes in the original commits and push an updated > version on my repo. Ok, great! Thanks a lot, Frieder ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 00/15] A SPI NAND framework under generic NAND framework 2017-12-21 11:48 ` Frieder Schrempf 2017-12-21 13:01 ` Boris Brezillon @ 2017-12-22 0:49 ` Peter Pan 2017-12-22 6:37 ` Peter Pan 1 sibling, 1 reply; 32+ messages in thread From: Peter Pan @ 2017-12-22 0:49 UTC (permalink / raw) To: Frieder Schrempf Cc: Boris Brezillon, Peter Pan 潘栋 (peterpandong), linux-mtd@lists.infradead.org Hi Frieder, On Thu, Dec 21, 2017 at 7:48 PM, Frieder Schrempf <frieder.schrempf@exceet.de> wrote: > Hello Boris, > >>>>> So shouldn't there be an spinand_die_select_op in the SPI NAND core >>>>> that >>>>> is executed when necessary and skipped if there's only one die. >>>> >>>> >>>> Sure, I was arguing against a ->select_chip() at the generic NAND >>>> level. That's something you can add in the SPI NAND framework. > > > I added an op to send the die selection command and call it, where I think > it is needed: [1] I think we should add die_select_op in vendor's file and call a hook in core.c since the die select command implementation is different by vendors. Thanks Peter Pan > > Accessing both dies on the Winbond SPI NAND works fine like this. > > While running tests I came across some problems: > > * On accessing the BBT in RAM via nanddev_bbt_[set/get]_block_status, the > calculation of position and offset seems to be wrong. > My fix is here: [2] > > * On calculating the row address for read/program/erase via > nanddev_pos_to_row, it seems like the eraseblock_addr_shift is wrong. > > * Also, I'm not sure if the LUN should be taken into account when > calculating the row address. At least when you select the LUN by issuing a > separate command, the row address sent to the chip should only contain the > page address. > But I'm not sure if that's the case in general, or if some code is needed to > differentiate. > > See my changes of nanddev_pos_to_row here: [3] > > * I run into a mutex deadlock, when spinand_write_page fails (e.g. because > of a bad block) as the lock is not released in such cases. See my fix here: > [4] > > With these fixes applied and as far as I can tell everything seems to work > fine. I'll do some tests with UBI next and look into the ECC topic. > > Regards, > > Frieder > > [1]: > https://github.com/fschrempf/linux-0day/compare/4f8472ea05f27d55ebb2e42eadc9ed59d7036f4c...fschrempf:2c5471c1f1e04fa91ff80c95276b4828b99c4f94 > > [2]: > https://github.com/fschrempf/linux-0day/commit/937a16248570368fe05e15b1f70e753e202b6ae6 > > [3]: > https://github.com/fschrempf/linux-0day/commit/8013696e0394960bfc36625d277f0d2425262939 > > [4]: > https://github.com/fschrempf/linux-0day/commit/28ffc7211745dd124702358c24ddccff8e6f2d95 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 00/15] A SPI NAND framework under generic NAND framework 2017-12-22 0:49 ` Peter Pan @ 2017-12-22 6:37 ` Peter Pan 2017-12-22 8:28 ` Boris Brezillon 2017-12-22 13:51 ` Boris Brezillon 0 siblings, 2 replies; 32+ messages in thread From: Peter Pan @ 2017-12-22 6:37 UTC (permalink / raw) To: Frieder Schrempf Cc: Boris Brezillon, Peter Pan 潘栋 (peterpandong), linux-mtd@lists.infradead.org Hi Boris and Frieder On Fri, Dec 22, 2017 at 8:49 AM, Peter Pan <peterpansjtu@gmail.com> wrote: > Hi Frieder, > > On Thu, Dec 21, 2017 at 7:48 PM, Frieder Schrempf > <frieder.schrempf@exceet.de> wrote: >> Hello Boris, >> >>>>>> So shouldn't there be an spinand_die_select_op in the SPI NAND core >>>>>> that >>>>>> is executed when necessary and skipped if there's only one die. >>>>> >>>>> >>>>> Sure, I was arguing against a ->select_chip() at the generic NAND >>>>> level. That's something you can add in the SPI NAND framework. >> >> >> I added an op to send the die selection command and call it, where I think >> it is needed: [1] > > I think we should add die_select_op in vendor's file and call a hook in core.c > since the die select command implementation is different by vendors. > > Thanks > Peter Pan > > >> >> Accessing both dies on the Winbond SPI NAND works fine like this. >> >> While running tests I came across some problems: >> >> * On accessing the BBT in RAM via nanddev_bbt_[set/get]_block_status, the >> calculation of position and offset seems to be wrong. >> My fix is here: [2] >> >> * On calculating the row address for read/program/erase via >> nanddev_pos_to_row, it seems like the eraseblock_addr_shift is wrong. >> >> * Also, I'm not sure if the LUN should be taken into account when >> calculating the row address. At least when you select the LUN by issuing a >> separate command, the row address sent to the chip should only contain the >> page address. >> But I'm not sure if that's the case in general, or if some code is needed to >> differentiate. >> >> See my changes of nanddev_pos_to_row here: [3] >> >> * I run into a mutex deadlock, when spinand_write_page fails (e.g. because >> of a bad block) as the lock is not released in such cases. See my fix here: >> [4] >> >> With these fixes applied and as far as I can tell everything seems to work >> fine. I'll do some tests with UBI next and look into the ECC topic. UBI attach failed since missing mtd->writebufsize assignment in nanddev_init() After fixing it and with Frieder's fixing, UBIFS can be mounted successfully on Zedboard with generic spi controller and Micron SPI NAND 2Gb device. Also, the code now can pass mtd read and page test. 1 error found in oob test since we don't have "past end of partition" check in part_write_oob() which I mentioned in earlier mail. Since we don't support ECC right now, I didn't try nandbiterror test. @@ -195,6 +195,7 @@ int nanddev_init(struct nand_device *nand, const struct nand_ops *ops, mtd->flags = MTD_CAP_NANDFLASH; mtd->erasesize = memorg->pagesize * memorg->pages_per_eraseblock; mtd->writesize = memorg->pagesize; + mtd->writebufsize = mtd->writesize; mtd->oobsize = memorg->oobsize; mtd->size = nanddev_size(nand); mtd->owner = owner; Thanks Peter Pan >> >> Regards, >> >> Frieder >> >> [1]: >> https://github.com/fschrempf/linux-0day/compare/4f8472ea05f27d55ebb2e42eadc9ed59d7036f4c...fschrempf:2c5471c1f1e04fa91ff80c95276b4828b99c4f94 >> >> [2]: >> https://github.com/fschrempf/linux-0day/commit/937a16248570368fe05e15b1f70e753e202b6ae6 >> >> [3]: >> https://github.com/fschrempf/linux-0day/commit/8013696e0394960bfc36625d277f0d2425262939 >> >> [4]: >> https://github.com/fschrempf/linux-0day/commit/28ffc7211745dd124702358c24ddccff8e6f2d95 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 00/15] A SPI NAND framework under generic NAND framework 2017-12-22 6:37 ` Peter Pan @ 2017-12-22 8:28 ` Boris Brezillon 2017-12-22 13:51 ` Boris Brezillon 1 sibling, 0 replies; 32+ messages in thread From: Boris Brezillon @ 2017-12-22 8:28 UTC (permalink / raw) To: Peter Pan Cc: Frieder Schrempf, Peter Pan 潘栋 (peterpandong), linux-mtd@lists.infradead.org On Fri, 22 Dec 2017 14:37:06 +0800 Peter Pan <peterpansjtu@gmail.com> wrote: > Hi Boris and Frieder > > On Fri, Dec 22, 2017 at 8:49 AM, Peter Pan <peterpansjtu@gmail.com> wrote: > > Hi Frieder, > > > > On Thu, Dec 21, 2017 at 7:48 PM, Frieder Schrempf > > <frieder.schrempf@exceet.de> wrote: > >> Hello Boris, > >> > >>>>>> So shouldn't there be an spinand_die_select_op in the SPI NAND core > >>>>>> that > >>>>>> is executed when necessary and skipped if there's only one die. > >>>>> > >>>>> > >>>>> Sure, I was arguing against a ->select_chip() at the generic NAND > >>>>> level. That's something you can add in the SPI NAND framework. > >> > >> > >> I added an op to send the die selection command and call it, where I think > >> it is needed: [1] > > > > I think we should add die_select_op in vendor's file and call a hook in core.c > > since the die select command implementation is different by vendors. > > > > Thanks > > Peter Pan > > > > > >> > >> Accessing both dies on the Winbond SPI NAND works fine like this. > >> > >> While running tests I came across some problems: > >> > >> * On accessing the BBT in RAM via nanddev_bbt_[set/get]_block_status, the > >> calculation of position and offset seems to be wrong. > >> My fix is here: [2] > >> > >> * On calculating the row address for read/program/erase via > >> nanddev_pos_to_row, it seems like the eraseblock_addr_shift is wrong. > >> > >> * Also, I'm not sure if the LUN should be taken into account when > >> calculating the row address. At least when you select the LUN by issuing a > >> separate command, the row address sent to the chip should only contain the > >> page address. > >> But I'm not sure if that's the case in general, or if some code is needed to > >> differentiate. > >> > >> See my changes of nanddev_pos_to_row here: [3] > >> > >> * I run into a mutex deadlock, when spinand_write_page fails (e.g. because > >> of a bad block) as the lock is not released in such cases. See my fix here: > >> [4] > >> > >> With these fixes applied and as far as I can tell everything seems to work > >> fine. I'll do some tests with UBI next and look into the ECC topic. > > UBI attach failed since missing mtd->writebufsize assignment in nanddev_init() > After fixing it and with Frieder's fixing, UBIFS can be mounted successfully on > Zedboard with generic spi controller and Micron SPI NAND 2Gb device. I'll squash your fix, thanks. > > Also, the code now can pass mtd read and page test. 1 error found in oob test > since we don't have "past end of partition" check in part_write_oob() which I > mentioned in earlier mail. Still not sure about that one, but I'll reply in the other thread. > Since we don't support ECC right now, I didn't try > nandbiterror test. > > > @@ -195,6 +195,7 @@ int nanddev_init(struct nand_device *nand, const > struct nand_ops *ops, > mtd->flags = MTD_CAP_NANDFLASH; > mtd->erasesize = memorg->pagesize * memorg->pages_per_eraseblock; > mtd->writesize = memorg->pagesize; > + mtd->writebufsize = mtd->writesize; > mtd->oobsize = memorg->oobsize; > mtd->size = nanddev_size(nand); > mtd->owner = owner; > > > Thanks > Peter Pan > > >> > >> Regards, > >> > >> Frieder > >> > >> [1]: > >> https://github.com/fschrempf/linux-0day/compare/4f8472ea05f27d55ebb2e42eadc9ed59d7036f4c...fschrempf:2c5471c1f1e04fa91ff80c95276b4828b99c4f94 > >> > >> [2]: > >> https://github.com/fschrempf/linux-0day/commit/937a16248570368fe05e15b1f70e753e202b6ae6 > >> > >> [3]: > >> https://github.com/fschrempf/linux-0day/commit/8013696e0394960bfc36625d277f0d2425262939 > >> > >> [4]: > >> https://github.com/fschrempf/linux-0day/commit/28ffc7211745dd124702358c24ddccff8e6f2d95 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 00/15] A SPI NAND framework under generic NAND framework 2017-12-22 6:37 ` Peter Pan 2017-12-22 8:28 ` Boris Brezillon @ 2017-12-22 13:51 ` Boris Brezillon 2018-01-02 2:51 ` Peter Pan 1 sibling, 1 reply; 32+ messages in thread From: Boris Brezillon @ 2017-12-22 13:51 UTC (permalink / raw) To: Peter Pan Cc: Frieder Schrempf, Peter Pan 潘栋 (peterpandong), linux-mtd@lists.infradead.org On Fri, 22 Dec 2017 14:37:06 +0800 Peter Pan <peterpansjtu@gmail.com> wrote: > Hi Boris and Frieder > > On Fri, Dec 22, 2017 at 8:49 AM, Peter Pan <peterpansjtu@gmail.com> wrote: > > Hi Frieder, > > > > On Thu, Dec 21, 2017 at 7:48 PM, Frieder Schrempf > > <frieder.schrempf@exceet.de> wrote: > >> Hello Boris, > >> > >>>>>> So shouldn't there be an spinand_die_select_op in the SPI NAND core > >>>>>> that > >>>>>> is executed when necessary and skipped if there's only one die. > >>>>> > >>>>> > >>>>> Sure, I was arguing against a ->select_chip() at the generic NAND > >>>>> level. That's something you can add in the SPI NAND framework. > >> > >> > >> I added an op to send the die selection command and call it, where I think > >> it is needed: [1] > > > > I think we should add die_select_op in vendor's file and call a hook in core.c > > since the die select command implementation is different by vendors. > > > > Thanks > > Peter Pan > > > > > >> > >> Accessing both dies on the Winbond SPI NAND works fine like this. > >> > >> While running tests I came across some problems: > >> > >> * On accessing the BBT in RAM via nanddev_bbt_[set/get]_block_status, the > >> calculation of position and offset seems to be wrong. > >> My fix is here: [2] > >> > >> * On calculating the row address for read/program/erase via > >> nanddev_pos_to_row, it seems like the eraseblock_addr_shift is wrong. > >> > >> * Also, I'm not sure if the LUN should be taken into account when > >> calculating the row address. At least when you select the LUN by issuing a > >> separate command, the row address sent to the chip should only contain the > >> page address. > >> But I'm not sure if that's the case in general, or if some code is needed to > >> differentiate. > >> > >> See my changes of nanddev_pos_to_row here: [3] > >> > >> * I run into a mutex deadlock, when spinand_write_page fails (e.g. because > >> of a bad block) as the lock is not released in such cases. See my fix here: > >> [4] > >> > >> With these fixes applied and as far as I can tell everything seems to work > >> fine. I'll do some tests with UBI next and look into the ECC topic. > > UBI attach failed since missing mtd->writebufsize assignment in nanddev_init() > After fixing it and with Frieder's fixing, UBIFS can be mounted successfully on > Zedboard with generic spi controller and Micron SPI NAND 2Gb device. > > Also, the code now can pass mtd read and page test. 1 error found in oob test > since we don't have "past end of partition" check in part_write_oob() which I > mentioned in earlier mail. Since we don't support ECC right now, I didn't try > nandbiterror test. > Peter, Frider, I just pushed a new version to my nand/spi-nand branch [1] fixing the authorship, adding/fixing/removing comments where needed. Can you please have a look at those changes (everything I changed should appear as !fixup commits) and let me know if I did something wrong. We still need to make commit messages a bit more verbose. This afternoon I'll have a look at the ->select_die() feature. Regards, Boris [1]https://github.com/bbrezillon/linux-0day/commits/nand/spi-nand ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 00/15] A SPI NAND framework under generic NAND framework 2017-12-22 13:51 ` Boris Brezillon @ 2018-01-02 2:51 ` Peter Pan 2018-01-03 16:46 ` Boris Brezillon 0 siblings, 1 reply; 32+ messages in thread From: Peter Pan @ 2018-01-02 2:51 UTC (permalink / raw) To: Boris Brezillon Cc: Frieder Schrempf, Peter Pan 潘栋 (peterpandong), linux-mtd@lists.infradead.org Hi Boris, On Fri, Dec 22, 2017 at 9:51 PM, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > On Fri, 22 Dec 2017 14:37:06 +0800 > Peter Pan <peterpansjtu@gmail.com> wrote: > >> Hi Boris and Frieder >> >> On Fri, Dec 22, 2017 at 8:49 AM, Peter Pan <peterpansjtu@gmail.com> wrote: >> > Hi Frieder, >> > >> > On Thu, Dec 21, 2017 at 7:48 PM, Frieder Schrempf >> > <frieder.schrempf@exceet.de> wrote: >> >> Hello Boris, >> >> >> >>>>>> So shouldn't there be an spinand_die_select_op in the SPI NAND core >> >>>>>> that >> >>>>>> is executed when necessary and skipped if there's only one die. >> >>>>> >> >>>>> >> >>>>> Sure, I was arguing against a ->select_chip() at the generic NAND >> >>>>> level. That's something you can add in the SPI NAND framework. >> >> >> >> >> >> I added an op to send the die selection command and call it, where I think >> >> it is needed: [1] >> > >> > I think we should add die_select_op in vendor's file and call a hook in core.c >> > since the die select command implementation is different by vendors. >> > >> > Thanks >> > Peter Pan >> > >> > >> >> >> >> Accessing both dies on the Winbond SPI NAND works fine like this. >> >> >> >> While running tests I came across some problems: >> >> >> >> * On accessing the BBT in RAM via nanddev_bbt_[set/get]_block_status, the >> >> calculation of position and offset seems to be wrong. >> >> My fix is here: [2] >> >> >> >> * On calculating the row address for read/program/erase via >> >> nanddev_pos_to_row, it seems like the eraseblock_addr_shift is wrong. >> >> >> >> * Also, I'm not sure if the LUN should be taken into account when >> >> calculating the row address. At least when you select the LUN by issuing a >> >> separate command, the row address sent to the chip should only contain the >> >> page address. >> >> But I'm not sure if that's the case in general, or if some code is needed to >> >> differentiate. >> >> >> >> See my changes of nanddev_pos_to_row here: [3] >> >> >> >> * I run into a mutex deadlock, when spinand_write_page fails (e.g. because >> >> of a bad block) as the lock is not released in such cases. See my fix here: >> >> [4] >> >> >> >> With these fixes applied and as far as I can tell everything seems to work >> >> fine. I'll do some tests with UBI next and look into the ECC topic. >> >> UBI attach failed since missing mtd->writebufsize assignment in nanddev_init() >> After fixing it and with Frieder's fixing, UBIFS can be mounted successfully on >> Zedboard with generic spi controller and Micron SPI NAND 2Gb device. >> >> Also, the code now can pass mtd read and page test. 1 error found in oob test >> since we don't have "past end of partition" check in part_write_oob() which I >> mentioned in earlier mail. Since we don't support ECC right now, I didn't try >> nandbiterror test. >> > > Peter, Frider, I just pushed a new version to my nand/spi-nand branch > [1] fixing the authorship, adding/fixing/removing comments where needed. > > Can you please have a look at those changes (everything I changed > should appear as !fixup commits) and let me know if I did something > wrong. You forgot to initialize mtd->writebufsize and mtd->oobavail. For mtd->writebufsize I think we can do it in nanddev_init() like below: --- a/drivers/mtd/nand/core.c +++ b/drivers/mtd/nand/core.c @@ -196,7 +196,9 @@ int nanddev_init(struct nand_device *nand, const struct nand_ops *ops, mtd->flags = MTD_CAP_NANDFLASH; mtd->erasesize = memorg->pagesize * memorg->pages_per_eraseblock; mtd->writesize = memorg->pagesize; + mtd->writebufsize = mtd->writesize; mtd->oobsize = memorg->oobsize; mtd->size = nanddev_size(nand); mtd->owner = owner; For mtd->oobavail, I think we need to call some hooks in spinand_init(), and implement on-die ECC layout in vendor's file and HW ECC layout in controller's file. What's your opinion? With the two thing fixed, we can pass UBIFS mount and mtd test. Thanks Peter Pan > > We still need to make commit messages a bit more verbose. > > This afternoon I'll have a look at the ->select_die() feature. > > Regards, > > Boris > > [1]https://github.com/bbrezillon/linux-0day/commits/nand/spi-nand ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 00/15] A SPI NAND framework under generic NAND framework 2018-01-02 2:51 ` Peter Pan @ 2018-01-03 16:46 ` Boris Brezillon 2018-01-04 2:01 ` Peter Pan 0 siblings, 1 reply; 32+ messages in thread From: Boris Brezillon @ 2018-01-03 16:46 UTC (permalink / raw) To: Peter Pan Cc: Frieder Schrempf, Peter Pan 潘栋 (peterpandong), linux-mtd@lists.infradead.org On Tue, 2 Jan 2018 10:51:25 +0800 Peter Pan <peterpansjtu@gmail.com> wrote: > Hi Boris, > > > On Fri, Dec 22, 2017 at 9:51 PM, Boris Brezillon > <boris.brezillon@free-electrons.com> wrote: > > On Fri, 22 Dec 2017 14:37:06 +0800 > > Peter Pan <peterpansjtu@gmail.com> wrote: > > > >> Hi Boris and Frieder > >> > >> On Fri, Dec 22, 2017 at 8:49 AM, Peter Pan <peterpansjtu@gmail.com> wrote: > >> > Hi Frieder, > >> > > >> > On Thu, Dec 21, 2017 at 7:48 PM, Frieder Schrempf > >> > <frieder.schrempf@exceet.de> wrote: > >> >> Hello Boris, > >> >> > >> >>>>>> So shouldn't there be an spinand_die_select_op in the SPI NAND core > >> >>>>>> that > >> >>>>>> is executed when necessary and skipped if there's only one die. > >> >>>>> > >> >>>>> > >> >>>>> Sure, I was arguing against a ->select_chip() at the generic NAND > >> >>>>> level. That's something you can add in the SPI NAND framework. > >> >> > >> >> > >> >> I added an op to send the die selection command and call it, where I think > >> >> it is needed: [1] > >> > > >> > I think we should add die_select_op in vendor's file and call a hook in core.c > >> > since the die select command implementation is different by vendors. > >> > > >> > Thanks > >> > Peter Pan > >> > > >> > > >> >> > >> >> Accessing both dies on the Winbond SPI NAND works fine like this. > >> >> > >> >> While running tests I came across some problems: > >> >> > >> >> * On accessing the BBT in RAM via nanddev_bbt_[set/get]_block_status, the > >> >> calculation of position and offset seems to be wrong. > >> >> My fix is here: [2] > >> >> > >> >> * On calculating the row address for read/program/erase via > >> >> nanddev_pos_to_row, it seems like the eraseblock_addr_shift is wrong. > >> >> > >> >> * Also, I'm not sure if the LUN should be taken into account when > >> >> calculating the row address. At least when you select the LUN by issuing a > >> >> separate command, the row address sent to the chip should only contain the > >> >> page address. > >> >> But I'm not sure if that's the case in general, or if some code is needed to > >> >> differentiate. > >> >> > >> >> See my changes of nanddev_pos_to_row here: [3] > >> >> > >> >> * I run into a mutex deadlock, when spinand_write_page fails (e.g. because > >> >> of a bad block) as the lock is not released in such cases. See my fix here: > >> >> [4] > >> >> > >> >> With these fixes applied and as far as I can tell everything seems to work > >> >> fine. I'll do some tests with UBI next and look into the ECC topic. > >> > >> UBI attach failed since missing mtd->writebufsize assignment in nanddev_init() > >> After fixing it and with Frieder's fixing, UBIFS can be mounted successfully on > >> Zedboard with generic spi controller and Micron SPI NAND 2Gb device. > >> > >> Also, the code now can pass mtd read and page test. 1 error found in oob test > >> since we don't have "past end of partition" check in part_write_oob() which I > >> mentioned in earlier mail. Since we don't support ECC right now, I didn't try > >> nandbiterror test. > >> > > > > Peter, Frider, I just pushed a new version to my nand/spi-nand branch > > [1] fixing the authorship, adding/fixing/removing comments where needed. > > > > Can you please have a look at those changes (everything I changed > > should appear as !fixup commits) and let me know if I did something > > wrong. > > You forgot to initialize mtd->writebufsize and mtd->oobavail. > For mtd->writebufsize I think we can do it in nanddev_init() like below: > > --- a/drivers/mtd/nand/core.c > +++ b/drivers/mtd/nand/core.c > @@ -196,7 +196,9 @@ int nanddev_init(struct nand_device *nand, const > struct nand_ops *ops, > mtd->flags = MTD_CAP_NANDFLASH; > mtd->erasesize = memorg->pagesize * memorg->pages_per_eraseblock; > mtd->writesize = memorg->pagesize; > + mtd->writebufsize = mtd->writesize; > mtd->oobsize = memorg->oobsize; > mtd->size = nanddev_size(nand); > mtd->owner = owner; I fixed it already, see [1]. Note that I haven't squashed fixup commits yet. > > For mtd->oobavail, I think we need to call some hooks in spinand_init(), > and implement on-die ECC layout in vendor's file and HW ECC layout in > controller's file. > > What's your opinion? Hm, I'm not so sure. spinand_init() should be called just before registering the MTD device, so by that time everything should have been setup correctly by the upper layer. This being said, I'd like to keep a clear separation between the MTD and NAND layers, and this means preventing any NAND sub-layer from directly manipulating the MTD device and its fields. Really, I don't know yet how I want to abstract ECC stuff, so let's keep it simple for now and expose the whole OOB area (I'll fix the core to do that if you're okay). > > With the two thing fixed, we can pass UBIFS mount and mtd test. Do you still have the problem you reported earlier (OOB test)? Thanks, Boris [1]https://github.com/bbrezillon/linux-0day/commit/32eeea1c35bfdc2ac0fcea599f6a926c94b835bb ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 00/15] A SPI NAND framework under generic NAND framework 2018-01-03 16:46 ` Boris Brezillon @ 2018-01-04 2:01 ` Peter Pan 2018-01-08 22:07 ` Boris Brezillon 0 siblings, 1 reply; 32+ messages in thread From: Peter Pan @ 2018-01-04 2:01 UTC (permalink / raw) To: Boris Brezillon Cc: Frieder Schrempf, Peter Pan 潘栋 (peterpandong), linux-mtd@lists.infradead.org Boris, On Thu, Jan 4, 2018 at 12:46 AM, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > On Tue, 2 Jan 2018 10:51:25 +0800 > Peter Pan <peterpansjtu@gmail.com> wrote: > >> Hi Boris, >> >> >> On Fri, Dec 22, 2017 at 9:51 PM, Boris Brezillon >> <boris.brezillon@free-electrons.com> wrote: >> > On Fri, 22 Dec 2017 14:37:06 +0800 >> > Peter Pan <peterpansjtu@gmail.com> wrote: >> > >> >> Hi Boris and Frieder >> >> >> >> On Fri, Dec 22, 2017 at 8:49 AM, Peter Pan <peterpansjtu@gmail.com> wrote: >> >> > Hi Frieder, >> >> > >> >> > On Thu, Dec 21, 2017 at 7:48 PM, Frieder Schrempf >> >> > <frieder.schrempf@exceet.de> wrote: >> >> >> Hello Boris, >> >> >> >> >> >>>>>> So shouldn't there be an spinand_die_select_op in the SPI NAND core >> >> >>>>>> that >> >> >>>>>> is executed when necessary and skipped if there's only one die. >> >> >>>>> >> >> >>>>> >> >> >>>>> Sure, I was arguing against a ->select_chip() at the generic NAND >> >> >>>>> level. That's something you can add in the SPI NAND framework. >> >> >> >> >> >> >> >> >> I added an op to send the die selection command and call it, where I think >> >> >> it is needed: [1] >> >> > >> >> > I think we should add die_select_op in vendor's file and call a hook in core.c >> >> > since the die select command implementation is different by vendors. >> >> > >> >> > Thanks >> >> > Peter Pan >> >> > >> >> > >> >> >> >> >> >> Accessing both dies on the Winbond SPI NAND works fine like this. >> >> >> >> >> >> While running tests I came across some problems: >> >> >> >> >> >> * On accessing the BBT in RAM via nanddev_bbt_[set/get]_block_status, the >> >> >> calculation of position and offset seems to be wrong. >> >> >> My fix is here: [2] >> >> >> >> >> >> * On calculating the row address for read/program/erase via >> >> >> nanddev_pos_to_row, it seems like the eraseblock_addr_shift is wrong. >> >> >> >> >> >> * Also, I'm not sure if the LUN should be taken into account when >> >> >> calculating the row address. At least when you select the LUN by issuing a >> >> >> separate command, the row address sent to the chip should only contain the >> >> >> page address. >> >> >> But I'm not sure if that's the case in general, or if some code is needed to >> >> >> differentiate. >> >> >> >> >> >> See my changes of nanddev_pos_to_row here: [3] >> >> >> >> >> >> * I run into a mutex deadlock, when spinand_write_page fails (e.g. because >> >> >> of a bad block) as the lock is not released in such cases. See my fix here: >> >> >> [4] >> >> >> >> >> >> With these fixes applied and as far as I can tell everything seems to work >> >> >> fine. I'll do some tests with UBI next and look into the ECC topic. >> >> >> >> UBI attach failed since missing mtd->writebufsize assignment in nanddev_init() >> >> After fixing it and with Frieder's fixing, UBIFS can be mounted successfully on >> >> Zedboard with generic spi controller and Micron SPI NAND 2Gb device. >> >> >> >> Also, the code now can pass mtd read and page test. 1 error found in oob test >> >> since we don't have "past end of partition" check in part_write_oob() which I >> >> mentioned in earlier mail. Since we don't support ECC right now, I didn't try >> >> nandbiterror test. >> >> >> > >> > Peter, Frider, I just pushed a new version to my nand/spi-nand branch >> > [1] fixing the authorship, adding/fixing/removing comments where needed. >> > >> > Can you please have a look at those changes (everything I changed >> > should appear as !fixup commits) and let me know if I did something >> > wrong. >> >> You forgot to initialize mtd->writebufsize and mtd->oobavail. >> For mtd->writebufsize I think we can do it in nanddev_init() like below: >> >> --- a/drivers/mtd/nand/core.c >> +++ b/drivers/mtd/nand/core.c >> @@ -196,7 +196,9 @@ int nanddev_init(struct nand_device *nand, const >> struct nand_ops *ops, >> mtd->flags = MTD_CAP_NANDFLASH; >> mtd->erasesize = memorg->pagesize * memorg->pages_per_eraseblock; >> mtd->writesize = memorg->pagesize; >> + mtd->writebufsize = mtd->writesize; >> mtd->oobsize = memorg->oobsize; >> mtd->size = nanddev_size(nand); >> mtd->owner = owner; > > I fixed it already, see [1]. Note that I haven't squashed fixup commits > yet. Noted. > >> >> For mtd->oobavail, I think we need to call some hooks in spinand_init(), >> and implement on-die ECC layout in vendor's file and HW ECC layout in >> controller's file. >> >> What's your opinion? > > Hm, I'm not so sure. spinand_init() should be called just before > registering the MTD device, so by that time everything should have been > setup correctly by the upper layer. This being said, I'd like to keep a > clear separation between the MTD and NAND layers, and this means > preventing any NAND sub-layer from directly manipulating the MTD device > and its fields. > > Really, I don't know yet how I want to abstract ECC stuff, so let's > keep it simple for now and expose the whole OOB area (I'll fix the core > to do that if you're okay). OK. > >> >> With the two thing fixed, we can pass UBIFS mount and mtd test. > > Do you still have the problem you reported earlier (OOB test)? I forgot to reply that mail :(. Actually there is no problem because mtd_write_oob() already has mtd_check_oob_ops() to check it. That failure in OOB test occurs because mtd->oobavail is zero. Thanks Peter Pan > > Thanks, > > Boris > > [1]https://github.com/bbrezillon/linux-0day/commit/32eeea1c35bfdc2ac0fcea599f6a926c94b835bb > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 00/15] A SPI NAND framework under generic NAND framework 2018-01-04 2:01 ` Peter Pan @ 2018-01-08 22:07 ` Boris Brezillon 0 siblings, 0 replies; 32+ messages in thread From: Boris Brezillon @ 2018-01-08 22:07 UTC (permalink / raw) To: Peter Pan Cc: Frieder Schrempf, Peter Pan 潘栋 (peterpandong), linux-mtd@lists.infradead.org On Thu, 4 Jan 2018 10:01:57 +0800 Peter Pan <peterpansjtu@gmail.com> wrote: > Boris, > > On Thu, Jan 4, 2018 at 12:46 AM, Boris Brezillon > <boris.brezillon@free-electrons.com> wrote: > > On Tue, 2 Jan 2018 10:51:25 +0800 > > Peter Pan <peterpansjtu@gmail.com> wrote: > > > >> Hi Boris, > >> > >> > >> On Fri, Dec 22, 2017 at 9:51 PM, Boris Brezillon > >> <boris.brezillon@free-electrons.com> wrote: > >> > On Fri, 22 Dec 2017 14:37:06 +0800 > >> > Peter Pan <peterpansjtu@gmail.com> wrote: > >> > > >> >> Hi Boris and Frieder > >> >> > >> >> On Fri, Dec 22, 2017 at 8:49 AM, Peter Pan <peterpansjtu@gmail.com> wrote: > >> >> > Hi Frieder, > >> >> > > >> >> > On Thu, Dec 21, 2017 at 7:48 PM, Frieder Schrempf > >> >> > <frieder.schrempf@exceet.de> wrote: > >> >> >> Hello Boris, > >> >> >> > >> >> >>>>>> So shouldn't there be an spinand_die_select_op in the SPI NAND core > >> >> >>>>>> that > >> >> >>>>>> is executed when necessary and skipped if there's only one die. > >> >> >>>>> > >> >> >>>>> > >> >> >>>>> Sure, I was arguing against a ->select_chip() at the generic NAND > >> >> >>>>> level. That's something you can add in the SPI NAND framework. > >> >> >> > >> >> >> > >> >> >> I added an op to send the die selection command and call it, where I think > >> >> >> it is needed: [1] > >> >> > > >> >> > I think we should add die_select_op in vendor's file and call a hook in core.c > >> >> > since the die select command implementation is different by vendors. > >> >> > > >> >> > Thanks > >> >> > Peter Pan > >> >> > > >> >> > > >> >> >> > >> >> >> Accessing both dies on the Winbond SPI NAND works fine like this. > >> >> >> > >> >> >> While running tests I came across some problems: > >> >> >> > >> >> >> * On accessing the BBT in RAM via nanddev_bbt_[set/get]_block_status, the > >> >> >> calculation of position and offset seems to be wrong. > >> >> >> My fix is here: [2] > >> >> >> > >> >> >> * On calculating the row address for read/program/erase via > >> >> >> nanddev_pos_to_row, it seems like the eraseblock_addr_shift is wrong. > >> >> >> > >> >> >> * Also, I'm not sure if the LUN should be taken into account when > >> >> >> calculating the row address. At least when you select the LUN by issuing a > >> >> >> separate command, the row address sent to the chip should only contain the > >> >> >> page address. > >> >> >> But I'm not sure if that's the case in general, or if some code is needed to > >> >> >> differentiate. > >> >> >> > >> >> >> See my changes of nanddev_pos_to_row here: [3] > >> >> >> > >> >> >> * I run into a mutex deadlock, when spinand_write_page fails (e.g. because > >> >> >> of a bad block) as the lock is not released in such cases. See my fix here: > >> >> >> [4] > >> >> >> > >> >> >> With these fixes applied and as far as I can tell everything seems to work > >> >> >> fine. I'll do some tests with UBI next and look into the ECC topic. > >> >> > >> >> UBI attach failed since missing mtd->writebufsize assignment in nanddev_init() > >> >> After fixing it and with Frieder's fixing, UBIFS can be mounted successfully on > >> >> Zedboard with generic spi controller and Micron SPI NAND 2Gb device. > >> >> > >> >> Also, the code now can pass mtd read and page test. 1 error found in oob test > >> >> since we don't have "past end of partition" check in part_write_oob() which I > >> >> mentioned in earlier mail. Since we don't support ECC right now, I didn't try > >> >> nandbiterror test. > >> >> > >> > > >> > Peter, Frider, I just pushed a new version to my nand/spi-nand branch > >> > [1] fixing the authorship, adding/fixing/removing comments where needed. > >> > > >> > Can you please have a look at those changes (everything I changed > >> > should appear as !fixup commits) and let me know if I did something > >> > wrong. > >> > >> You forgot to initialize mtd->writebufsize and mtd->oobavail. > >> For mtd->writebufsize I think we can do it in nanddev_init() like below: > >> > >> --- a/drivers/mtd/nand/core.c > >> +++ b/drivers/mtd/nand/core.c > >> @@ -196,7 +196,9 @@ int nanddev_init(struct nand_device *nand, const > >> struct nand_ops *ops, > >> mtd->flags = MTD_CAP_NANDFLASH; > >> mtd->erasesize = memorg->pagesize * memorg->pages_per_eraseblock; > >> mtd->writesize = memorg->pagesize; > >> + mtd->writebufsize = mtd->writesize; > >> mtd->oobsize = memorg->oobsize; > >> mtd->size = nanddev_size(nand); > >> mtd->owner = owner; > > > > I fixed it already, see [1]. Note that I haven't squashed fixup commits > > yet. > > Noted. > > > > >> > >> For mtd->oobavail, I think we need to call some hooks in spinand_init(), > >> and implement on-die ECC layout in vendor's file and HW ECC layout in > >> controller's file. > >> > >> What's your opinion? > > > > Hm, I'm not so sure. spinand_init() should be called just before > > registering the MTD device, so by that time everything should have been > > setup correctly by the upper layer. This being said, I'd like to keep a > > clear separation between the MTD and NAND layers, and this means > > preventing any NAND sub-layer from directly manipulating the MTD device > > and its fields. > > > > Really, I don't know yet how I want to abstract ECC stuff, so let's > > keep it simple for now and expose the whole OOB area (I'll fix the core > > to do that if you're okay). > > OK. > > > > >> > >> With the two thing fixed, we can pass UBIFS mount and mtd test. > > > > Do you still have the problem you reported earlier (OOB test)? > > I forgot to reply that mail :(. Actually there is no problem because > mtd_write_oob() already has mtd_check_oob_ops() to check it. > That failure in OOB test occurs because mtd->oobavail is zero. I reconsidered what I said earlier, and I think it's safer to set ->oobavail to 0 are for now, otherwise we might end up with setups that start using some of it and we'll break things as soon as we'll add support for ECC. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 00/15] A SPI NAND framework under generic NAND framework 2017-12-14 7:50 ` Boris Brezillon 2017-12-14 8:06 ` Peter Pan @ 2017-12-15 2:35 ` Peter Pan 2017-12-15 12:41 ` Boris Brezillon 1 sibling, 1 reply; 32+ messages in thread From: Peter Pan @ 2017-12-15 2:35 UTC (permalink / raw) To: Boris Brezillon Cc: Frieder Schrempf, Peter Pan 潘栋 (peterpandong), linux-mtd@lists.infradead.org Hi Boris On Thu, Dec 14, 2017 at 3:50 PM, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > Hi Peter, > > On Thu, 14 Dec 2017 14:15:57 +0800 > Peter Pan <peterpansjtu@gmail.com> wrote: > >> Hi Boris and Frieder, >> >> On Thu, Dec 14, 2017 at 5:27 AM, Boris Brezillon >> <boris.brezillon@free-electrons.com> wrote: >> > Hi Frieder, >> > >> > On Tue, 12 Dec 2017 10:58:10 +0100 >> > Frieder Schrempf <frieder.schrempf@exceet.de> wrote: >> > >> >> Hi Boris, >> >> >> >> I spent some time looking at and working with your latest SPI NAND code. >> >> In my previous mail I said, that we have some working code for our 3.14 >> >> kernel based on the "mt29f_spinand" staging driver, but that was wrong >> >> as I got things mixed up in my head. >> >> Actually our codebase was an early version of Peter's code, so the >> >> effort to port it to the latest framework code was not that big. >> >> >> >> I forked your repo and you can find my working tree at [1]. >> > >> > Great, I'll try to have a look. >> > >> >> >> >> What I did so far: >> >> >> >> * Rebase your patches on latest Linux 4.14.5 >> > >> > Cool, rebasing on 4.15-rc1 would be even better, but I can do that if >> > you don't have the time. >> > >> >> >> >> * Add a driver for the Winbond W25M02GV SPI NAND chip, that we have on >> >> some of our boards. >> > >> > Okay, that's actually a good thing to have tested with another chip, >> > This way we can make sure the framework is generic enough. >> > >> >> >> >> * Add a driver for the Freescale QSPI controller, derived from the >> >> existing QSPI-NOR driver at drivers/mtd/spi-nor/fsl-quadspi.c. >> > >> > That's an interesting case as well, the generic NAND controller is >> > probably the easiest implementation, and that's a good thing to have >> > another controller. >> > >> >> >> >> * Add a setup and setup_late op to the controller layer (see [3]). I >> >> don't know if that makes sense, but at least the setup_late is needed >> >> for the FSL QSPI driver, as it needs to know the size of the flash to >> >> setup the memory mapping for the QSPI read operations. >> > >> > Hm, not sure what ->setup_late() is for, but I'll have a look. >> > >> >> >> >> * A bit of testing and fixing two small bugs, which look like copy and >> >> paste mistakes. See [2]. >> > >> > Thanks, I'll squash the fixes in the original commit. >> > >> >> >> >> * Running nandtest successfully on our hardware (i.MX6UL -> FSL-QSPI -> >> >> W25M02GV) >> >> >> >> I hope this is of use while moving on. >> > >> > Definitely. I'd also like to have a review on the framework code if >> > possible, but that can be done when posted on the ML. >> > >> >> I guess you would like to have the basic framework with Micron support >> >> and generic SPI tested and stabilized first, before adding more code, >> >> but to be able to test with our hardware I need Micron and FSL QSPI. >> > >> > I guess you mean Winbond not Micron. That's okay if those patches are >> > posted after the initial series. All I need is reviews and tests from >> > different parties, so that I'm less confident in merging the code. >> > >> >> >> >> Some questions/flaws that occurred to me: >> >> >> >> * The W25M02GV chip has two dies of 128M each. My current driver only >> >> makes use of the first die. The chip expects a die-select command to >> >> switch between the two dies. >> >> What would be the place to implement this? >> >> Can I just add the command and issue it in >> >> winbond_spinand_adjust_cache_op if luns_per_target > 1? >> > >> > It really depends when the die selection happens. I guess it happens in >> > 2 places: when preparing a program/read operation and when doing the >> > transfer to the in-chip cache. Anyway, the die information is already >> > passed in the nand_pos object, so all you'll have to do is create a new >> > hook to customize the SPI command when needed. >> >> I don't know whether we can do the the die switch in the generic NAND core >> (drivers/mtd/nand/core.c). I'm not sure if chip->select_chip() in nand_base.c >> does the same thing or not. If so, we can do the die switch before >> read/write/erase >> operation. And let spi nand core to implement a hook to support it. > > Actually, I'm trying to move away from this ->select_chip() approach in > the raw NAND framework, simply because the controller might be able to > parallelize operations (like 2 erase on 2 different dies, or one erase > on one die, and a program on the other), and having this ->select_chip() > done early in the chain prevents this kind of optimization. > > Anyway, the controller should now have all the necessary information to > know which die an operation should be executed on, and if a specific > command has to be sent to the device to select a specific die, it can > be done in the NAND vendor specific code. > >> >> > >> >> >> >> * The FSL QSPI controller has a lookup table that needs to be filled >> >> with command sequences at the time of setup. Therefore the number of >> >> dummy bytes for each command is fixed and in my current implementation >> >> op->dummy_bytes is ignored. >> >> That's not a problem if all chips need the same number of dummy bytes >> >> for each command, but I guess that's not the case, as there is a >> >> _spinand_get_dummy function. >> > >> > We should definitely expose that in a generic way, and on a >> > per-operation basis, so that, after the detection step, the NAND >> > controller can query this information. >> > >> >> >> >> * What is your plan for ECC and BBT? At least enabling the onchip ECC >> >> will be necessary to be able to use the flash properly (e.g. with UBI), >> >> or am I wrong? >> > >> > Well, if all SPI NAND chips are supporting ECC the same way and >> > on-die ECC support is mandatory for SPI NANDs, then supporting that >> > directly in the core is probably the best option. If, on the other >> > hand, you have various implementations, and some SPI controllers have >> > their own ECC engine that you can use with SPI NANDs, then it's >> > probably a better idea to abstract ECC engine in the generic NAND layer. >> >> As far as I know, all the SPI NAND supports on-die ECC (at least all >> the SPI NAND >> I heard). But different chips may have different ECC layout in OOB >> area. But I think >> this cannot be a problem, we can handle this in vendor's file. > > Yep. > >> >> > >> > For BBTs, I'd like to have a clean version of the nand_bbt logic that >> > uses all of the helpers exposed by the generic NAND layer. I'd also >> > like to simplify the code if possible. >> > >> >> >> >> * Do you have any special test cases? What do you usually do to test the >> >> code? >> > >> > Well, make sure all the mtd tests are passing (drivers/mtd/tests), and >> > then, the next step is to test it with UBI+UBIFS. But honestly, I'm not >> > so worried, this is new code, and we've isolated from the raw NAND >> > layer, so if it's buggy or instable at the beginning it's not a big >> > deal, it will be noticed and fixed for the next few releases. >> > >> >> >> >> Thanks for your patience and best regards, >> > >> > No problem. Thanks for your work, and I'll try to be more active on >> > this topic (I promised that several times, and failed it :-/). >> >> Boris, >> >> I think we need to move forward, MediaTek's engineer has already asked for >> the status of the spi nand framework upstreaming, since they want to upstream >> their SPI NAND controller driver. And Hisilicon also has a controller driver. >> Since more and more platforms start to support SPI NAND, we'd better merge a >> simple but well designed framework first. I will run your modified >> version on Zedboard >> with generic SPI controller this or next week. > > Then why are they not spending time reviewing the SPI NAND framework? > You know I'm not paid to work on SPI NAND, unlike those engineers > working for SoC/NAND vendors. While I agree being a maintainer implies > doing part of the work on our spare time, what I don't like is when > people that did not invest time in a specific feature ask for this > feature to be implemented/released. This is not your case, you invested > a lot of time in it, and that's the reason I feel bad when I can't spend > the necessary time to make SPI NAND code ready for inclusion. > > So here are the next set of things to do if you want to move forward: > 1/ Re-submit the preparation patches (those touching MTD core) and > review them (or find someone to review them) I already finished the rebase of your preparation patches on l2-mtd nand/next brach (v4.15-rc1). Should I send them as v3 or reset to v1? I think we don't need to wait for SPI NAND code. These preparation patches should be review ASAP. I'd like to review it after the sending, since they are all your effort. What's your opinion? Thanks Peter Pan > 2/ Add the missing doc to the code (I was planning on doing that, but > if you're in hurry you can take care of it), and add real commit > messages. > 3/ Fix the authorship on patches (some were mainly written by you, but > during my rework authorship has been lost) > 4/ Ask others to test and review the patches > > Regards, > > Boris ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 00/15] A SPI NAND framework under generic NAND framework 2017-12-15 2:35 ` Peter Pan @ 2017-12-15 12:41 ` Boris Brezillon 0 siblings, 0 replies; 32+ messages in thread From: Boris Brezillon @ 2017-12-15 12:41 UTC (permalink / raw) To: Peter Pan, Frieder Schrempf Cc: Peter Pan 潘栋 (peterpandong), linux-mtd@lists.infradead.org On Fri, 15 Dec 2017 10:35:16 +0800 Peter Pan <peterpansjtu@gmail.com> wrote: > Hi Boris > > On Thu, Dec 14, 2017 at 3:50 PM, Boris Brezillon > <boris.brezillon@free-electrons.com> wrote: > > Hi Peter, > > > > On Thu, 14 Dec 2017 14:15:57 +0800 > > Peter Pan <peterpansjtu@gmail.com> wrote: > > > >> Hi Boris and Frieder, > >> > >> On Thu, Dec 14, 2017 at 5:27 AM, Boris Brezillon > >> <boris.brezillon@free-electrons.com> wrote: > >> > Hi Frieder, > >> > > >> > On Tue, 12 Dec 2017 10:58:10 +0100 > >> > Frieder Schrempf <frieder.schrempf@exceet.de> wrote: > >> > > >> >> Hi Boris, > >> >> > >> >> I spent some time looking at and working with your latest SPI NAND code. > >> >> In my previous mail I said, that we have some working code for our 3.14 > >> >> kernel based on the "mt29f_spinand" staging driver, but that was wrong > >> >> as I got things mixed up in my head. > >> >> Actually our codebase was an early version of Peter's code, so the > >> >> effort to port it to the latest framework code was not that big. > >> >> > >> >> I forked your repo and you can find my working tree at [1]. > >> > > >> > Great, I'll try to have a look. > >> > > >> >> > >> >> What I did so far: > >> >> > >> >> * Rebase your patches on latest Linux 4.14.5 > >> > > >> > Cool, rebasing on 4.15-rc1 would be even better, but I can do that if > >> > you don't have the time. > >> > > >> >> > >> >> * Add a driver for the Winbond W25M02GV SPI NAND chip, that we have on > >> >> some of our boards. > >> > > >> > Okay, that's actually a good thing to have tested with another chip, > >> > This way we can make sure the framework is generic enough. > >> > > >> >> > >> >> * Add a driver for the Freescale QSPI controller, derived from the > >> >> existing QSPI-NOR driver at drivers/mtd/spi-nor/fsl-quadspi.c. > >> > > >> > That's an interesting case as well, the generic NAND controller is > >> > probably the easiest implementation, and that's a good thing to have > >> > another controller. > >> > > >> >> > >> >> * Add a setup and setup_late op to the controller layer (see [3]). I > >> >> don't know if that makes sense, but at least the setup_late is needed > >> >> for the FSL QSPI driver, as it needs to know the size of the flash to > >> >> setup the memory mapping for the QSPI read operations. > >> > > >> > Hm, not sure what ->setup_late() is for, but I'll have a look. > >> > > >> >> > >> >> * A bit of testing and fixing two small bugs, which look like copy and > >> >> paste mistakes. See [2]. > >> > > >> > Thanks, I'll squash the fixes in the original commit. > >> > > >> >> > >> >> * Running nandtest successfully on our hardware (i.MX6UL -> FSL-QSPI -> > >> >> W25M02GV) > >> >> > >> >> I hope this is of use while moving on. > >> > > >> > Definitely. I'd also like to have a review on the framework code if > >> > possible, but that can be done when posted on the ML. > >> > > >> >> I guess you would like to have the basic framework with Micron support > >> >> and generic SPI tested and stabilized first, before adding more code, > >> >> but to be able to test with our hardware I need Micron and FSL QSPI. > >> > > >> > I guess you mean Winbond not Micron. That's okay if those patches are > >> > posted after the initial series. All I need is reviews and tests from > >> > different parties, so that I'm less confident in merging the code. > >> > > >> >> > >> >> Some questions/flaws that occurred to me: > >> >> > >> >> * The W25M02GV chip has two dies of 128M each. My current driver only > >> >> makes use of the first die. The chip expects a die-select command to > >> >> switch between the two dies. > >> >> What would be the place to implement this? > >> >> Can I just add the command and issue it in > >> >> winbond_spinand_adjust_cache_op if luns_per_target > 1? > >> > > >> > It really depends when the die selection happens. I guess it happens in > >> > 2 places: when preparing a program/read operation and when doing the > >> > transfer to the in-chip cache. Anyway, the die information is already > >> > passed in the nand_pos object, so all you'll have to do is create a new > >> > hook to customize the SPI command when needed. > >> > >> I don't know whether we can do the the die switch in the generic NAND core > >> (drivers/mtd/nand/core.c). I'm not sure if chip->select_chip() in nand_base.c > >> does the same thing or not. If so, we can do the die switch before > >> read/write/erase > >> operation. And let spi nand core to implement a hook to support it. > > > > Actually, I'm trying to move away from this ->select_chip() approach in > > the raw NAND framework, simply because the controller might be able to > > parallelize operations (like 2 erase on 2 different dies, or one erase > > on one die, and a program on the other), and having this ->select_chip() > > done early in the chain prevents this kind of optimization. > > > > Anyway, the controller should now have all the necessary information to > > know which die an operation should be executed on, and if a specific > > command has to be sent to the device to select a specific die, it can > > be done in the NAND vendor specific code. > > > >> > >> > > >> >> > >> >> * The FSL QSPI controller has a lookup table that needs to be filled > >> >> with command sequences at the time of setup. Therefore the number of > >> >> dummy bytes for each command is fixed and in my current implementation > >> >> op->dummy_bytes is ignored. > >> >> That's not a problem if all chips need the same number of dummy bytes > >> >> for each command, but I guess that's not the case, as there is a > >> >> _spinand_get_dummy function. > >> > > >> > We should definitely expose that in a generic way, and on a > >> > per-operation basis, so that, after the detection step, the NAND > >> > controller can query this information. > >> > > >> >> > >> >> * What is your plan for ECC and BBT? At least enabling the onchip ECC > >> >> will be necessary to be able to use the flash properly (e.g. with UBI), > >> >> or am I wrong? > >> > > >> > Well, if all SPI NAND chips are supporting ECC the same way and > >> > on-die ECC support is mandatory for SPI NANDs, then supporting that > >> > directly in the core is probably the best option. If, on the other > >> > hand, you have various implementations, and some SPI controllers have > >> > their own ECC engine that you can use with SPI NANDs, then it's > >> > probably a better idea to abstract ECC engine in the generic NAND layer. > >> > >> As far as I know, all the SPI NAND supports on-die ECC (at least all > >> the SPI NAND > >> I heard). But different chips may have different ECC layout in OOB > >> area. But I think > >> this cannot be a problem, we can handle this in vendor's file. > > > > Yep. > > > >> > >> > > >> > For BBTs, I'd like to have a clean version of the nand_bbt logic that > >> > uses all of the helpers exposed by the generic NAND layer. I'd also > >> > like to simplify the code if possible. > >> > > >> >> > >> >> * Do you have any special test cases? What do you usually do to test the > >> >> code? > >> > > >> > Well, make sure all the mtd tests are passing (drivers/mtd/tests), and > >> > then, the next step is to test it with UBI+UBIFS. But honestly, I'm not > >> > so worried, this is new code, and we've isolated from the raw NAND > >> > layer, so if it's buggy or instable at the beginning it's not a big > >> > deal, it will be noticed and fixed for the next few releases. > >> > > >> >> > >> >> Thanks for your patience and best regards, > >> > > >> > No problem. Thanks for your work, and I'll try to be more active on > >> > this topic (I promised that several times, and failed it :-/). > >> > >> Boris, > >> > >> I think we need to move forward, MediaTek's engineer has already asked for > >> the status of the spi nand framework upstreaming, since they want to upstream > >> their SPI NAND controller driver. And Hisilicon also has a controller driver. > >> Since more and more platforms start to support SPI NAND, we'd better merge a > >> simple but well designed framework first. I will run your modified > >> version on Zedboard > >> with generic SPI controller this or next week. > > > > Then why are they not spending time reviewing the SPI NAND framework? > > You know I'm not paid to work on SPI NAND, unlike those engineers > > working for SoC/NAND vendors. While I agree being a maintainer implies > > doing part of the work on our spare time, what I don't like is when > > people that did not invest time in a specific feature ask for this > > feature to be implemented/released. This is not your case, you invested > > a lot of time in it, and that's the reason I feel bad when I can't spend > > the necessary time to make SPI NAND code ready for inclusion. > > > > So here are the next set of things to do if you want to move forward: > > 1/ Re-submit the preparation patches (those touching MTD core) and > > review them (or find someone to review them) > > I already finished the rebase of your preparation patches on l2-mtd > nand/next brach (v4.15-rc1). Should I send them as v3 or reset to v1? > I think we don't need to wait for SPI NAND code. These preparation > patches should be review ASAP. I'd like to review it after the sending, > since they are all your effort. What's your opinion? I just sent them on the ML. Feel free to review. ^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2018-01-08 22:07 UTC | newest]
Thread overview: 32+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <74cb9a07bd3247fd86002ef97509828f@SIWEX4H.sing.micron.com>
2017-05-31 6:20 ` [PATCH v6 00/15] A SPI NAND framework under generic NAND framework Boris Brezillon
2017-05-31 6:34 ` Peter Pan 潘栋 (peterpandong)
2017-05-24 7:06 Peter Pan
2017-05-29 20:59 ` Boris Brezillon
2017-12-04 13:32 ` Frieder Schrempf
2017-12-04 14:05 ` Boris Brezillon
2017-12-05 1:35 ` Peter Pan 潘栋 (peterpandong)
2017-12-05 12:58 ` Boris Brezillon
2017-12-05 13:03 ` Boris Brezillon
2017-12-12 9:58 ` Frieder Schrempf
2017-12-13 21:27 ` Boris Brezillon
2017-12-14 6:15 ` Peter Pan
2017-12-14 7:50 ` Boris Brezillon
2017-12-14 8:06 ` Peter Pan
2017-12-14 14:39 ` Frieder Schrempf
2017-12-14 14:43 ` Frieder Schrempf
2017-12-14 15:38 ` Boris Brezillon
2017-12-15 1:08 ` Peter Pan
2017-12-15 1:21 ` Peter Pan
2017-12-21 11:48 ` Frieder Schrempf
2017-12-21 13:01 ` Boris Brezillon
2017-12-21 13:54 ` Frieder Schrempf
2017-12-22 0:49 ` Peter Pan
2017-12-22 6:37 ` Peter Pan
2017-12-22 8:28 ` Boris Brezillon
2017-12-22 13:51 ` Boris Brezillon
2018-01-02 2:51 ` Peter Pan
2018-01-03 16:46 ` Boris Brezillon
2018-01-04 2:01 ` Peter Pan
2018-01-08 22:07 ` Boris Brezillon
2017-12-15 2:35 ` Peter Pan
2017-12-15 12:41 ` Boris Brezillon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).