* [PATCH 0/3] An alternative to SPI NAND @ 2015-01-08 0:47 ` Peter Pan 潘栋 (peterpandong) 0 siblings, 0 replies; 32+ messages in thread From: Peter Pan 潘栋 (peterpandong) @ 2015-01-08 0:47 UTC (permalink / raw) To: dwmw2, Brian Norris, Ezequiel Garcia Cc: linux-kernel, linux-mtd, Qi Wang 王起 (qiwang), Frank Liu 刘群 (frankliu), Melanie Zhang 张燕 (melaniezhang), Peter Pan 潘栋 (peterpandong) [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="gb2312", Size: 3055 bytes --] This patchset is an alternative to Ezequiel's series[1]. This patchset separate SPI NAND code and Parallel NAND code, make SPI NAND have its own spi_nand_scan, read, write, BBM mechanism, so that it would be better for code maintenance in the future. TODO - 1. This patchset is validated only on Micron SPI NAND device MT29F2G01AAAED by run mtdtest program, and mount UBIFS on SPI NAND, further testing on other Manufactory SPI NAND is needed. 2. Although this patchset's framework separate SPI NAND and Parall NAND code, some code do is common that can share by SPI NAND and Parallel NAND at same time. For view the code structure might be more make sense as below diagram, so that SPI NAND and Parallel NAND can have their own specific code and meanwhile can share Common code. But may bring a lot change for current code, I am glad to discuss this structure if any guys are interested. |------------------------------------------------------------------| | MTD/NAND folder | | |-------------| |---------------| |-------------------------| | | | Common code | | SPI NAND code | | Parallel NAND code | | | |-------------| | --------------| |-------------------------| | | | Nand_bch.c | |spi_nand_base.c| | parallel_nand_base.c | | | | Nand_ecc.c | | ......... | |specific controllers code| | | | Nand_bbt.c | | | | | | |------------------------------------------------------------------| This series is based on v3.19-rc1. [1] http://lists.infradead.org/pipermail/linux-mtd/2014-December/056763.html Peter Pan (3): mtd: spi-nand framework mtd: spi-nand: support spi-nand devices mtd: spi-nand: add devicetree binding Documentation/devicetree/bindings/mtd/spi-nand.txt | 22 + drivers/mtd/Kconfig | 2 + drivers/mtd/Makefile | 1 + drivers/mtd/spi-nand/Kconfig | 7 + drivers/mtd/spi-nand/Makefile | 3 + drivers/mtd/spi-nand/spi-nand-base.c | 2034 ++++++++++++++++++++ drivers/mtd/spi-nand/spi-nand-bbt.c | 1279 ++++++++++++ drivers/mtd/spi-nand/spi-nand-device.c | 281 +++ include/linux/mtd/spi-nand.h | 317 +++ 9 files changed, 3946 insertions(+) create mode 100644 Documentation/devicetree/bindings/mtd/spi-nand.txt create mode 100644 drivers/mtd/spi-nand/Kconfig create mode 100644 drivers/mtd/spi-nand/Makefile create mode 100644 drivers/mtd/spi-nand/spi-nand-base.c create mode 100644 drivers/mtd/spi-nand/spi-nand-bbt.c create mode 100644 drivers/mtd/spi-nand/spi-nand-device.c create mode 100644 include/linux/mtd/spi-nand.h -- 1.9.1 ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥ ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 0/3] An alternative to SPI NAND @ 2015-01-08 0:47 ` Peter Pan 潘栋 (peterpandong) 0 siblings, 0 replies; 32+ messages in thread From: Peter Pan 潘栋 (peterpandong) @ 2015-01-08 0:47 UTC (permalink / raw) To: dwmw2, Brian Norris, Ezequiel Garcia Cc: Qi Wang 王起 (qiwang), linux-kernel, linux-mtd, Frank Liu 刘群 (frankliu), Melanie Zhang 张燕 (melaniezhang), Peter Pan 潘栋 (peterpandong) This patchset is an alternative to Ezequiel's series[1]. This patchset separate SPI NAND code and Parallel NAND code, make SPI NAND have its own spi_nand_scan, read, write, BBM mechanism, so that it would be better for code maintenance in the future. TODO - 1. This patchset is validated only on Micron SPI NAND device MT29F2G01AAAED by run mtdtest program, and mount UBIFS on SPI NAND, further testing on other Manufactory SPI NAND is needed. 2. Although this patchset's framework separate SPI NAND and Parall NAND code, some code do is common that can share by SPI NAND and Parallel NAND at same time. For view the code structure might be more make sense as below diagram, so that SPI NAND and Parallel NAND can have their own specific code and meanwhile can share Common code. But may bring a lot change for current code, I am glad to discuss this structure if any guys are interested. |------------------------------------------------------------------| | MTD/NAND folder | | |-------------| |---------------| |-------------------------| | | | Common code | | SPI NAND code | | Parallel NAND code | | | |-------------| | --------------| |-------------------------| | | | Nand_bch.c | |spi_nand_base.c| | parallel_nand_base.c | | | | Nand_ecc.c | | ......... | |specific controllers code| | | | Nand_bbt.c | | | | | | |------------------------------------------------------------------| This series is based on v3.19-rc1. [1] http://lists.infradead.org/pipermail/linux-mtd/2014-December/056763.html Peter Pan (3): mtd: spi-nand framework mtd: spi-nand: support spi-nand devices mtd: spi-nand: add devicetree binding Documentation/devicetree/bindings/mtd/spi-nand.txt | 22 + drivers/mtd/Kconfig | 2 + drivers/mtd/Makefile | 1 + drivers/mtd/spi-nand/Kconfig | 7 + drivers/mtd/spi-nand/Makefile | 3 + drivers/mtd/spi-nand/spi-nand-base.c | 2034 ++++++++++++++++++++ drivers/mtd/spi-nand/spi-nand-bbt.c | 1279 ++++++++++++ drivers/mtd/spi-nand/spi-nand-device.c | 281 +++ include/linux/mtd/spi-nand.h | 317 +++ 9 files changed, 3946 insertions(+) create mode 100644 Documentation/devicetree/bindings/mtd/spi-nand.txt create mode 100644 drivers/mtd/spi-nand/Kconfig create mode 100644 drivers/mtd/spi-nand/Makefile create mode 100644 drivers/mtd/spi-nand/spi-nand-base.c create mode 100644 drivers/mtd/spi-nand/spi-nand-bbt.c create mode 100644 drivers/mtd/spi-nand/spi-nand-device.c create mode 100644 include/linux/mtd/spi-nand.h -- 1.9.1 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 0/3] An alternative to SPI NAND 2015-01-08 0:47 ` Peter Pan 潘栋 (peterpandong) @ 2015-01-08 1:03 ` Brian Norris -1 siblings, 0 replies; 32+ messages in thread From: Brian Norris @ 2015-01-08 1:03 UTC (permalink / raw) To: Peter Pan 潘栋 (peterpandong) Cc: dwmw2, Ezequiel Garcia, linux-kernel, linux-mtd, Qi Wang 王起 (qiwang), Frank Liu 刘群 (frankliu), Melanie Zhang 张燕 (melaniezhang) On Thu, Jan 08, 2015 at 12:47:24AM +0000, Peter Pan 潘栋 (peterpandong) wrote: > Documentation/devicetree/bindings/mtd/spi-nand.txt | 22 + > drivers/mtd/Kconfig | 2 + > drivers/mtd/Makefile | 1 + > drivers/mtd/spi-nand/Kconfig | 7 + > drivers/mtd/spi-nand/Makefile | 3 + > drivers/mtd/spi-nand/spi-nand-base.c | 2034 ++++++++++++++++++++ > drivers/mtd/spi-nand/spi-nand-bbt.c | 1279 ++++++++++++ I can already tell by the diffstat that I don't like this. We probably don't need 3000 new lines of code for this, but we especially don't want to duplicate nand_bbt.c. It won't take a lot of work to augment nand_bbt.c to make it shareable. (I can whip that patch up if needed.) I'll still take a look at the rest of the code eventually, but just wanted to give my 2 cents up front. > drivers/mtd/spi-nand/spi-nand-device.c | 281 +++ > include/linux/mtd/spi-nand.h | 317 +++ > 9 files changed, 3946 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mtd/spi-nand.txt > create mode 100644 drivers/mtd/spi-nand/Kconfig > create mode 100644 drivers/mtd/spi-nand/Makefile > create mode 100644 drivers/mtd/spi-nand/spi-nand-base.c > create mode 100644 drivers/mtd/spi-nand/spi-nand-bbt.c > create mode 100644 drivers/mtd/spi-nand/spi-nand-device.c > create mode 100644 include/linux/mtd/spi-nand.h Brian ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 0/3] An alternative to SPI NAND @ 2015-01-08 1:03 ` Brian Norris 0 siblings, 0 replies; 32+ messages in thread From: Brian Norris @ 2015-01-08 1:03 UTC (permalink / raw) To: Peter Pan 潘栋 (peterpandong) Cc: Qi Wang 王起 (qiwang), linux-kernel, linux-mtd, Frank Liu 刘群 (frankliu), Ezequiel Garcia, Melanie Zhang 张燕 (melaniezhang), dwmw2 On Thu, Jan 08, 2015 at 12:47:24AM +0000, Peter Pan 潘栋 (peterpandong) wrote: > Documentation/devicetree/bindings/mtd/spi-nand.txt | 22 + > drivers/mtd/Kconfig | 2 + > drivers/mtd/Makefile | 1 + > drivers/mtd/spi-nand/Kconfig | 7 + > drivers/mtd/spi-nand/Makefile | 3 + > drivers/mtd/spi-nand/spi-nand-base.c | 2034 ++++++++++++++++++++ > drivers/mtd/spi-nand/spi-nand-bbt.c | 1279 ++++++++++++ I can already tell by the diffstat that I don't like this. We probably don't need 3000 new lines of code for this, but we especially don't want to duplicate nand_bbt.c. It won't take a lot of work to augment nand_bbt.c to make it shareable. (I can whip that patch up if needed.) I'll still take a look at the rest of the code eventually, but just wanted to give my 2 cents up front. > drivers/mtd/spi-nand/spi-nand-device.c | 281 +++ > include/linux/mtd/spi-nand.h | 317 +++ > 9 files changed, 3946 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mtd/spi-nand.txt > create mode 100644 drivers/mtd/spi-nand/Kconfig > create mode 100644 drivers/mtd/spi-nand/Makefile > create mode 100644 drivers/mtd/spi-nand/spi-nand-base.c > create mode 100644 drivers/mtd/spi-nand/spi-nand-bbt.c > create mode 100644 drivers/mtd/spi-nand/spi-nand-device.c > create mode 100644 include/linux/mtd/spi-nand.h Brian ^ permalink raw reply [flat|nested] 32+ messages in thread
* RE: [PATCH 0/3] An alternative to SPI NAND 2015-01-08 1:03 ` Brian Norris @ 2015-01-08 2:45 ` Qi Wang 王起 (qiwang) -1 siblings, 0 replies; 32+ messages in thread From: Qi Wang 王起 (qiwang) @ 2015-01-08 2:45 UTC (permalink / raw) To: Brian Norris Cc: dwmw2, Ezequiel Garcia, linux-kernel, linux-mtd, Frank Liu 刘群 (frankliu), Melanie Zhang 张燕 (melaniezhang), Peter Pan 潘栋 (peterpandong) [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2560 bytes --] Hi Brian, On Thu, Jan 08, 2015 at 9:03:24AM +0000, Brian Norris wrote: > >On Thu, Jan 08, 2015 at 12:47:24AM +0000, Peter Pan æ½æ (peterpandong) >wrote: >> Documentation/devicetree/bindings/mtd/spi-nand.txt | 22 + >> drivers/mtd/Kconfig | 2 + >> drivers/mtd/Makefile | 1 + >> drivers/mtd/spi-nand/Kconfig | 7 + >> drivers/mtd/spi-nand/Makefile | 3 + >> drivers/mtd/spi-nand/spi-nand-base.c | 2034 >++++++++++++++++++++ >> drivers/mtd/spi-nand/spi-nand-bbt.c | 1279 ++++++++++++ > >I can already tell by the diffstat that I don't like this. We probably >don't need 3000 new lines of code for this, but we especially don't want >to duplicate nand_bbt.c. It won't take a lot of work to augment >nand_bbt.c to make it shareable. (I can whip that patch up if needed.) Yes, I agree with you, Nand_bbt.c do can be shared by Parallel NAND and SPI NAND. Actually, we are working at this now. Will send patches to you Once we finished it. But for the 2000 new lines code in spi-nand-base.c, frankly speaking, I still no idea how to decrease the code lines. I separate code of the SPI NAND and Parallel NAND that different with Ezequiel Garcia patches, because I think this framework could be easier to maintain it, in the future, SPI NAND may have more specific features that will be difficult to reuse the code of Nand_base.c but just remap command to SPI NAND from Parallel NAND. So we write a new spi-nand-base.c, to implement read/write/erase function, that will cause many code lines as you see. If you have other suggestion, please let me know. > >I'll still take a look at the rest of the code eventually, but just >wanted to give my 2 cents up front. > >> drivers/mtd/spi-nand/spi-nand-device.c | 281 +++ >> include/linux/mtd/spi-nand.h | 317 +++ >> 9 files changed, 3946 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/mtd/spi-nand.txt >> create mode 100644 drivers/mtd/spi-nand/Kconfig >> create mode 100644 drivers/mtd/spi-nand/Makefile >> create mode 100644 drivers/mtd/spi-nand/spi-nand-base.c >> create mode 100644 drivers/mtd/spi-nand/spi-nand-bbt.c >> create mode 100644 drivers/mtd/spi-nand/spi-nand-device.c >> create mode 100644 include/linux/mtd/spi-nand.h > Qi Wang ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥ ^ permalink raw reply [flat|nested] 32+ messages in thread
* RE: [PATCH 0/3] An alternative to SPI NAND @ 2015-01-08 2:45 ` Qi Wang 王起 (qiwang) 0 siblings, 0 replies; 32+ messages in thread From: Qi Wang 王起 (qiwang) @ 2015-01-08 2:45 UTC (permalink / raw) To: Brian Norris Cc: linux-kernel, linux-mtd, Frank Liu 刘群 (frankliu), Ezequiel Garcia, Melanie Zhang 张燕 (melaniezhang), dwmw2, Peter Pan 潘栋 (peterpandong) Hi Brian, On Thu, Jan 08, 2015 at 9:03:24AM +0000, Brian Norris wrote: > >On Thu, Jan 08, 2015 at 12:47:24AM +0000, Peter Pan 潘栋 (peterpandong) >wrote: >> Documentation/devicetree/bindings/mtd/spi-nand.txt | 22 + >> drivers/mtd/Kconfig | 2 + >> drivers/mtd/Makefile | 1 + >> drivers/mtd/spi-nand/Kconfig | 7 + >> drivers/mtd/spi-nand/Makefile | 3 + >> drivers/mtd/spi-nand/spi-nand-base.c | 2034 >++++++++++++++++++++ >> drivers/mtd/spi-nand/spi-nand-bbt.c | 1279 ++++++++++++ > >I can already tell by the diffstat that I don't like this. We probably >don't need 3000 new lines of code for this, but we especially don't want >to duplicate nand_bbt.c. It won't take a lot of work to augment >nand_bbt.c to make it shareable. (I can whip that patch up if needed.) Yes, I agree with you, Nand_bbt.c do can be shared by Parallel NAND and SPI NAND. Actually, we are working at this now. Will send patches to you Once we finished it. But for the 2000 new lines code in spi-nand-base.c, frankly speaking, I still no idea how to decrease the code lines. I separate code of the SPI NAND and Parallel NAND that different with Ezequiel Garcia patches, because I think this framework could be easier to maintain it, in the future, SPI NAND may have more specific features that will be difficult to reuse the code of Nand_base.c but just remap command to SPI NAND from Parallel NAND. So we write a new spi-nand-base.c, to implement read/write/erase function, that will cause many code lines as you see. If you have other suggestion, please let me know. > >I'll still take a look at the rest of the code eventually, but just >wanted to give my 2 cents up front. > >> drivers/mtd/spi-nand/spi-nand-device.c | 281 +++ >> include/linux/mtd/spi-nand.h | 317 +++ >> 9 files changed, 3946 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/mtd/spi-nand.txt >> create mode 100644 drivers/mtd/spi-nand/Kconfig >> create mode 100644 drivers/mtd/spi-nand/Makefile >> create mode 100644 drivers/mtd/spi-nand/spi-nand-base.c >> create mode 100644 drivers/mtd/spi-nand/spi-nand-bbt.c >> create mode 100644 drivers/mtd/spi-nand/spi-nand-device.c >> create mode 100644 include/linux/mtd/spi-nand.h > Qi Wang ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 0/3] An alternative to SPI NAND 2015-01-08 2:45 ` Qi Wang 王起 (qiwang) @ 2015-01-08 3:27 ` Ezequiel Garcia -1 siblings, 0 replies; 32+ messages in thread From: Ezequiel Garcia @ 2015-01-08 3:27 UTC (permalink / raw) To: "Qi Wang 王起 (qiwang)", Brian Norris Cc: dwmw2, linux-kernel, linux-mtd, "Frank Liu 刘群 (frankliu)", "Melanie Zhang 张燕 (melaniezhang)", "Peter Pan 潘栋 (peterpandong)" Hi Qi Wang, On 01/07/2015 11:45 PM, Qi Wang 王起 (qiwang) wrote: > Hi Brian, > > On Thu, Jan 08, 2015 at 9:03:24AM +0000, Brian Norris wrote: >> >> On Thu, Jan 08, 2015 at 12:47:24AM +0000, Peter Pan 潘栋 (peterpandong) >> wrote: >>> Documentation/devicetree/bindings/mtd/spi-nand.txt | 22 + >>> drivers/mtd/Kconfig | 2 + >>> drivers/mtd/Makefile | 1 + >>> drivers/mtd/spi-nand/Kconfig | 7 + >>> drivers/mtd/spi-nand/Makefile | 3 + >>> drivers/mtd/spi-nand/spi-nand-base.c | 2034 >> ++++++++++++++++++++ >>> drivers/mtd/spi-nand/spi-nand-bbt.c | 1279 ++++++++++++ >> >> I can already tell by the diffstat that I don't like this. We probably >> don't need 3000 new lines of code for this, but we especially don't want >> to duplicate nand_bbt.c. It won't take a lot of work to augment >> nand_bbt.c to make it shareable. (I can whip that patch up if needed.) > > Yes, I agree with you, Nand_bbt.c do can be shared by Parallel NAND and > SPI NAND. Actually, we are working at this now. Will send patches to you > Once we finished it. > Thanks for the quick submission! However, Brian is right, this code duplication is a no go. Perhaps a more valid approach would be to first identify the code that needs to be shared in nand_bbt.c and nand_base.c, and export those symbols (or maybe do the required refactor). Then, separate the SPI NAND upper and lower logic (in a similar to my proposal, which I still consider turned out to be clean). These two things would lead to a simpler and smaller patchset. I also suggest to cut off everything that we don't utterly need on a first submission, so it's easier to review. -- Ezequiel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 0/3] An alternative to SPI NAND @ 2015-01-08 3:27 ` Ezequiel Garcia 0 siblings, 0 replies; 32+ messages in thread From: Ezequiel Garcia @ 2015-01-08 3:27 UTC (permalink / raw) To: "Qi Wang 王起 (qiwang)", Brian Norris Cc: linux-kernel, linux-mtd, "Frank Liu 刘群 (frankliu)", "Melanie Zhang 张燕 (melaniezhang)", dwmw2, "Peter Pan 潘栋 (peterpandong)" Hi Qi Wang, On 01/07/2015 11:45 PM, Qi Wang 王起 (qiwang) wrote: > Hi Brian, > > On Thu, Jan 08, 2015 at 9:03:24AM +0000, Brian Norris wrote: >> >> On Thu, Jan 08, 2015 at 12:47:24AM +0000, Peter Pan 潘栋 (peterpandong) >> wrote: >>> Documentation/devicetree/bindings/mtd/spi-nand.txt | 22 + >>> drivers/mtd/Kconfig | 2 + >>> drivers/mtd/Makefile | 1 + >>> drivers/mtd/spi-nand/Kconfig | 7 + >>> drivers/mtd/spi-nand/Makefile | 3 + >>> drivers/mtd/spi-nand/spi-nand-base.c | 2034 >> ++++++++++++++++++++ >>> drivers/mtd/spi-nand/spi-nand-bbt.c | 1279 ++++++++++++ >> >> I can already tell by the diffstat that I don't like this. We probably >> don't need 3000 new lines of code for this, but we especially don't want >> to duplicate nand_bbt.c. It won't take a lot of work to augment >> nand_bbt.c to make it shareable. (I can whip that patch up if needed.) > > Yes, I agree with you, Nand_bbt.c do can be shared by Parallel NAND and > SPI NAND. Actually, we are working at this now. Will send patches to you > Once we finished it. > Thanks for the quick submission! However, Brian is right, this code duplication is a no go. Perhaps a more valid approach would be to first identify the code that needs to be shared in nand_bbt.c and nand_base.c, and export those symbols (or maybe do the required refactor). Then, separate the SPI NAND upper and lower logic (in a similar to my proposal, which I still consider turned out to be clean). These two things would lead to a simpler and smaller patchset. I also suggest to cut off everything that we don't utterly need on a first submission, so it's easier to review. -- Ezequiel ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 0/3] An alternative to SPI NAND 2015-01-08 3:27 ` Ezequiel Garcia @ 2015-01-12 15:10 ` Qi Wang 王起 (qiwang) -1 siblings, 0 replies; 32+ messages in thread From: Qi Wang 王起 (qiwang) @ 2015-01-12 15:10 UTC (permalink / raw) To: Ezequiel Garcia, Brian Norris Cc: dwmw2, linux-kernel, linux-mtd, Frank Liu 刘群 (frankliu), Melanie Zhang 张燕 (melaniezhang), Peter Pan 潘栋 (peterpandong) [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3674 bytes --] Hi Ezequiel, On 01/08/2015 11:27 AM, Ezequiel Garcia wrote: > >Hi Qi Wang, > >On 01/07/2015 11:45 PM, Qi Wang çèµ· (qiwang) wrote: >> Hi Brian, >> >> On Thu, Jan 08, 2015 at 9:03:24AM +0000, Brian Norris wrote: >>> >>> On Thu, Jan 08, 2015 at 12:47:24AM +0000, Peter Pan æ½æ (peterpandong) >>> wrote: >>>> Documentation/devicetree/bindings/mtd/spi-nand.txt | 22 + >>>> drivers/mtd/Kconfig | 2 + >>>> drivers/mtd/Makefile | 1 + >>>> drivers/mtd/spi-nand/Kconfig | 7 + >>>> drivers/mtd/spi-nand/Makefile | 3 + >>>> drivers/mtd/spi-nand/spi-nand-base.c | 2034 >>> ++++++++++++++++++++ >>>> drivers/mtd/spi-nand/spi-nand-bbt.c | 1279 ++++++++++++ >>> >>> I can already tell by the diffstat that I don't like this. We probably >>> don't need 3000 new lines of code for this, but we especially don't want >>> to duplicate nand_bbt.c. It won't take a lot of work to augment >>> nand_bbt.c to make it shareable. (I can whip that patch up if needed.) >> >> Yes, I agree with you, Nand_bbt.c do can be shared by Parallel NAND and >> SPI NAND. Actually, we are working at this now. Will send patches to you >> Once we finished it. >> > >Thanks for the quick submission! > >However, Brian is right, this code duplication is a no go. > >Perhaps a more valid approach would be to first identify the code that >needs to be shared in nand_bbt.c and nand_base.c, and export those >symbols (or maybe do the required refactor). Yes, I agree Brian's suggestion in another mail. " The BBT code is something we definitely want to share, but it's actually not very closely tied to nand_base.c, and it looks pretty easy to adapt to any MTD that implements mtd_read_oob()/mtd_write_oob(). We'd just need to parameterize a few relevant device details into a new nand_bbt struct, rather than using struct nand_chip directly." To abstract a new nand_bbt struct instead of nand_chip to make SPI NAND and parallel NAND can share nand_bbt.c file, I already begin to work on this. For code shared in nand_base.c, I agree it would be better if we can find a good method to share nand_base.c code between spi nand and parallel nand. But frankly speaking, I'm not satisfied for the remap command method. This method make code difficult to maintain when SPI NAND and Parallel NAND evolve much differently in the future. Take some example, If one new command (cache operation, multiple plane operation) implemented in parallel NAND code, and is used in nand_read or nand_write, that will cause maintainer to modify SPI NAND code to remap this new command, though this modification probably could be slight. That means modification on Parallel NAND flash need to consider SPI NAND as well. How do you think about this? For Peter Pan's patchset, if we do some modification to make nand_bbt.c to make it shareable for Parallel and SPI NAND. The code line should be 2000. I believe I can review this spi-nand-base.c to remove some redundant code that may hundreds line. Is 1700 or 1800 code line is more acceptable? Let me know your opinions. > >Then, separate the SPI NAND upper and lower logic (in a similar to my >proposal, which I still consider turned out to be clean). > >These two things would lead to a simpler and smaller patchset. I also >suggest to cut off everything that we don't utterly need on a first >submission, so it's easier to review. >-- >Ezequiel -- Qi Wang ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥ ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 0/3] An alternative to SPI NAND @ 2015-01-12 15:10 ` Qi Wang 王起 (qiwang) 0 siblings, 0 replies; 32+ messages in thread From: Qi Wang 王起 (qiwang) @ 2015-01-12 15:10 UTC (permalink / raw) To: Ezequiel Garcia, Brian Norris Cc: linux-kernel, linux-mtd, Frank Liu 刘群 (frankliu), Melanie Zhang 张燕 (melaniezhang), dwmw2, Peter Pan 潘栋 (peterpandong) Hi Ezequiel, On 01/08/2015 11:27 AM, Ezequiel Garcia wrote: > >Hi Qi Wang, > >On 01/07/2015 11:45 PM, Qi Wang 王起 (qiwang) wrote: >> Hi Brian, >> >> On Thu, Jan 08, 2015 at 9:03:24AM +0000, Brian Norris wrote: >>> >>> On Thu, Jan 08, 2015 at 12:47:24AM +0000, Peter Pan 潘栋 (peterpandong) >>> wrote: >>>> Documentation/devicetree/bindings/mtd/spi-nand.txt | 22 + >>>> drivers/mtd/Kconfig | 2 + >>>> drivers/mtd/Makefile | 1 + >>>> drivers/mtd/spi-nand/Kconfig | 7 + >>>> drivers/mtd/spi-nand/Makefile | 3 + >>>> drivers/mtd/spi-nand/spi-nand-base.c | 2034 >>> ++++++++++++++++++++ >>>> drivers/mtd/spi-nand/spi-nand-bbt.c | 1279 ++++++++++++ >>> >>> I can already tell by the diffstat that I don't like this. We probably >>> don't need 3000 new lines of code for this, but we especially don't want >>> to duplicate nand_bbt.c. It won't take a lot of work to augment >>> nand_bbt.c to make it shareable. (I can whip that patch up if needed.) >> >> Yes, I agree with you, Nand_bbt.c do can be shared by Parallel NAND and >> SPI NAND. Actually, we are working at this now. Will send patches to you >> Once we finished it. >> > >Thanks for the quick submission! > >However, Brian is right, this code duplication is a no go. > >Perhaps a more valid approach would be to first identify the code that >needs to be shared in nand_bbt.c and nand_base.c, and export those >symbols (or maybe do the required refactor). Yes, I agree Brian's suggestion in another mail. " The BBT code is something we definitely want to share, but it's actually not very closely tied to nand_base.c, and it looks pretty easy to adapt to any MTD that implements mtd_read_oob()/mtd_write_oob(). We'd just need to parameterize a few relevant device details into a new nand_bbt struct, rather than using struct nand_chip directly." To abstract a new nand_bbt struct instead of nand_chip to make SPI NAND and parallel NAND can share nand_bbt.c file, I already begin to work on this. For code shared in nand_base.c, I agree it would be better if we can find a good method to share nand_base.c code between spi nand and parallel nand. But frankly speaking, I'm not satisfied for the remap command method. This method make code difficult to maintain when SPI NAND and Parallel NAND evolve much differently in the future. Take some example, If one new command (cache operation, multiple plane operation) implemented in parallel NAND code, and is used in nand_read or nand_write, that will cause maintainer to modify SPI NAND code to remap this new command, though this modification probably could be slight. That means modification on Parallel NAND flash need to consider SPI NAND as well. How do you think about this? For Peter Pan's patchset, if we do some modification to make nand_bbt.c to make it shareable for Parallel and SPI NAND. The code line should be 2000. I believe I can review this spi-nand-base.c to remove some redundant code that may hundreds line. Is 1700 or 1800 code line is more acceptable? Let me know your opinions. > >Then, separate the SPI NAND upper and lower logic (in a similar to my >proposal, which I still consider turned out to be clean). > >These two things would lead to a simpler and smaller patchset. I also >suggest to cut off everything that we don't utterly need on a first >submission, so it's easier to review. >-- >Ezequiel -- Qi Wang ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 0/3] An alternative to SPI NAND 2015-01-12 15:10 ` Qi Wang 王起 (qiwang) @ 2015-01-20 10:35 ` Ezequiel Garcia -1 siblings, 0 replies; 32+ messages in thread From: Ezequiel Garcia @ 2015-01-20 10:35 UTC (permalink / raw) To: "Qi Wang 王起 (qiwang)", Brian Norris Cc: dwmw2, linux-kernel, linux-mtd, "Frank Liu 刘群 (frankliu)", "Melanie Zhang 张燕 (melaniezhang)", "Peter Pan 潘栋 (peterpandong)" On 01/12/2015 12:10 PM, Qi Wang 王起 (qiwang) wrote: > Hi Ezequiel, > > On 01/08/2015 11:27 AM, Ezequiel Garcia wrote: >> >> Hi Qi Wang, >> >> On 01/07/2015 11:45 PM, Qi Wang 王起 (qiwang) wrote: >>> Hi Brian, >>> >>> On Thu, Jan 08, 2015 at 9:03:24AM +0000, Brian Norris wrote: >>>> >>>> On Thu, Jan 08, 2015 at 12:47:24AM +0000, Peter Pan 潘栋 (peterpandong) >>>> wrote: >>>>> Documentation/devicetree/bindings/mtd/spi-nand.txt | 22 + >>>>> drivers/mtd/Kconfig | 2 + >>>>> drivers/mtd/Makefile | 1 + >>>>> drivers/mtd/spi-nand/Kconfig | 7 + >>>>> drivers/mtd/spi-nand/Makefile | 3 + >>>>> drivers/mtd/spi-nand/spi-nand-base.c | 2034 >>>> ++++++++++++++++++++ >>>>> drivers/mtd/spi-nand/spi-nand-bbt.c | 1279 ++++++++++++ >>>> >>>> I can already tell by the diffstat that I don't like this. We probably >>>> don't need 3000 new lines of code for this, but we especially don't want >>>> to duplicate nand_bbt.c. It won't take a lot of work to augment >>>> nand_bbt.c to make it shareable. (I can whip that patch up if needed.) >>> >>> Yes, I agree with you, Nand_bbt.c do can be shared by Parallel NAND and >>> SPI NAND. Actually, we are working at this now. Will send patches to you >>> Once we finished it. >>> >> >> Thanks for the quick submission! >> >> However, Brian is right, this code duplication is a no go. >> >> Perhaps a more valid approach would be to first identify the code that >> needs to be shared in nand_bbt.c and nand_base.c, and export those >> symbols (or maybe do the required refactor). > > Yes, I agree Brian's suggestion in another mail. > > " The BBT code is something we definitely want to share, but it's actually > not very closely tied to nand_base.c, and it looks pretty easy to adapt > to any MTD that implements mtd_read_oob()/mtd_write_oob(). We'd just > need to parameterize a few relevant device details into a new nand_bbt > struct, rather than using struct nand_chip directly." > > To abstract a new nand_bbt struct instead of nand_chip to make SPI NAND > and parallel NAND can share nand_bbt.c file, I already begin to work on > this. > > For code shared in nand_base.c, I agree it would be better if we can find > a good method to share nand_base.c code between spi nand and parallel nand. > But frankly speaking, I'm not satisfied for the remap command method. This > method make code difficult to maintain when SPI NAND and Parallel NAND > evolve much differently in the future. > > Take some example, > If one new command (cache operation, multiple plane operation) implemented > in parallel NAND code, and is used in nand_read or nand_write, that will > cause maintainer to modify SPI NAND code to remap this new command, though > this modification probably could be slight. That means modification on > Parallel NAND flash need to consider SPI NAND as well. > > How do you think about this? > > For Peter Pan's patchset, if we do some modification to make nand_bbt.c to > make it shareable for Parallel and SPI NAND. The code line should be 2000. > I believe I can review this spi-nand-base.c to remove some redundant code > that may hundreds line. Is 1700 or 1800 code line is more acceptable? > > Let me know your opinions. > Sounds good. Do you still plan to maintain the spi-nand-base.c and spi-nand-device.c separation? -- Ezequiel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 0/3] An alternative to SPI NAND @ 2015-01-20 10:35 ` Ezequiel Garcia 0 siblings, 0 replies; 32+ messages in thread From: Ezequiel Garcia @ 2015-01-20 10:35 UTC (permalink / raw) To: "Qi Wang 王起 (qiwang)", Brian Norris Cc: linux-kernel, linux-mtd, "Frank Liu 刘群 (frankliu)", "Melanie Zhang 张燕 (melaniezhang)", dwmw2, "Peter Pan 潘栋 (peterpandong)" On 01/12/2015 12:10 PM, Qi Wang 王起 (qiwang) wrote: > Hi Ezequiel, > > On 01/08/2015 11:27 AM, Ezequiel Garcia wrote: >> >> Hi Qi Wang, >> >> On 01/07/2015 11:45 PM, Qi Wang 王起 (qiwang) wrote: >>> Hi Brian, >>> >>> On Thu, Jan 08, 2015 at 9:03:24AM +0000, Brian Norris wrote: >>>> >>>> On Thu, Jan 08, 2015 at 12:47:24AM +0000, Peter Pan 潘栋 (peterpandong) >>>> wrote: >>>>> Documentation/devicetree/bindings/mtd/spi-nand.txt | 22 + >>>>> drivers/mtd/Kconfig | 2 + >>>>> drivers/mtd/Makefile | 1 + >>>>> drivers/mtd/spi-nand/Kconfig | 7 + >>>>> drivers/mtd/spi-nand/Makefile | 3 + >>>>> drivers/mtd/spi-nand/spi-nand-base.c | 2034 >>>> ++++++++++++++++++++ >>>>> drivers/mtd/spi-nand/spi-nand-bbt.c | 1279 ++++++++++++ >>>> >>>> I can already tell by the diffstat that I don't like this. We probably >>>> don't need 3000 new lines of code for this, but we especially don't want >>>> to duplicate nand_bbt.c. It won't take a lot of work to augment >>>> nand_bbt.c to make it shareable. (I can whip that patch up if needed.) >>> >>> Yes, I agree with you, Nand_bbt.c do can be shared by Parallel NAND and >>> SPI NAND. Actually, we are working at this now. Will send patches to you >>> Once we finished it. >>> >> >> Thanks for the quick submission! >> >> However, Brian is right, this code duplication is a no go. >> >> Perhaps a more valid approach would be to first identify the code that >> needs to be shared in nand_bbt.c and nand_base.c, and export those >> symbols (or maybe do the required refactor). > > Yes, I agree Brian's suggestion in another mail. > > " The BBT code is something we definitely want to share, but it's actually > not very closely tied to nand_base.c, and it looks pretty easy to adapt > to any MTD that implements mtd_read_oob()/mtd_write_oob(). We'd just > need to parameterize a few relevant device details into a new nand_bbt > struct, rather than using struct nand_chip directly." > > To abstract a new nand_bbt struct instead of nand_chip to make SPI NAND > and parallel NAND can share nand_bbt.c file, I already begin to work on > this. > > For code shared in nand_base.c, I agree it would be better if we can find > a good method to share nand_base.c code between spi nand and parallel nand. > But frankly speaking, I'm not satisfied for the remap command method. This > method make code difficult to maintain when SPI NAND and Parallel NAND > evolve much differently in the future. > > Take some example, > If one new command (cache operation, multiple plane operation) implemented > in parallel NAND code, and is used in nand_read or nand_write, that will > cause maintainer to modify SPI NAND code to remap this new command, though > this modification probably could be slight. That means modification on > Parallel NAND flash need to consider SPI NAND as well. > > How do you think about this? > > For Peter Pan's patchset, if we do some modification to make nand_bbt.c to > make it shareable for Parallel and SPI NAND. The code line should be 2000. > I believe I can review this spi-nand-base.c to remove some redundant code > that may hundreds line. Is 1700 or 1800 code line is more acceptable? > > Let me know your opinions. > Sounds good. Do you still plan to maintain the spi-nand-base.c and spi-nand-device.c separation? -- Ezequiel ^ permalink raw reply [flat|nested] 32+ messages in thread
* RE: [PATCH 0/3] An alternative to SPI NAND 2015-01-20 10:35 ` Ezequiel Garcia @ 2015-01-21 2:11 ` Qi Wang 王起 (qiwang) -1 siblings, 0 replies; 32+ messages in thread From: Qi Wang 王起 (qiwang) @ 2015-01-21 2:11 UTC (permalink / raw) To: Ezequiel Garcia, Brian Norris Cc: dwmw2, linux-kernel, linux-mtd, Frank Liu 刘群 (frankliu), Melanie Zhang 张燕 (melaniezhang), Peter Pan 潘栋 (peterpandong) [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3992 bytes --] On 01/20/2015 6:36 PM, Ezequiel Garcia wrote: > >On 01/12/2015 12:10 PM, Qi Wang çèµ· (qiwang) wrote: >> Hi Ezequiel, >> >> On 01/08/2015 11:27 AM, Ezequiel Garcia wrote: >>> >>> Hi Qi Wang, >>> >>> On 01/07/2015 11:45 PM, Qi Wang çèµ· (qiwang) wrote: >>>> Hi Brian, >>>> >>>> On Thu, Jan 08, 2015 at 9:03:24AM +0000, Brian Norris wrote: >>>>> >>>>> On Thu, Jan 08, 2015 at 12:47:24AM +0000, Peter Pan æ½æ (peterpandong) >>>>> wrote: >>>>>> Documentation/devicetree/bindings/mtd/spi-nand.txt | 22 + >>>>>> drivers/mtd/Kconfig | 2 + >>>>>> drivers/mtd/Makefile | 1 + >>>>>> drivers/mtd/spi-nand/Kconfig | 7 + >>>>>> drivers/mtd/spi-nand/Makefile | 3 + >>>>>> drivers/mtd/spi-nand/spi-nand-base.c | 2034 >>>>> ++++++++++++++++++++ >>>>>> drivers/mtd/spi-nand/spi-nand-bbt.c | 1279 >++++++++++++ >>>>> >>>>> I can already tell by the diffstat that I don't like this. We probably >>>>> don't need 3000 new lines of code for this, but we especially don't >want >>>>> to duplicate nand_bbt.c. It won't take a lot of work to augment >>>>> nand_bbt.c to make it shareable. (I can whip that patch up if needed.) >>>> >>>> Yes, I agree with you, Nand_bbt.c do can be shared by Parallel NAND and >>>> SPI NAND. Actually, we are working at this now. Will send patches to >you >>>> Once we finished it. >>>> >>> >>> Thanks for the quick submission! >>> >>> However, Brian is right, this code duplication is a no go. >>> >>> Perhaps a more valid approach would be to first identify the code that >>> needs to be shared in nand_bbt.c and nand_base.c, and export those >>> symbols (or maybe do the required refactor). >> >> Yes, I agree Brian's suggestion in another mail. >> >> " The BBT code is something we definitely want to share, but it's >actually >> not very closely tied to nand_base.c, and it looks pretty easy to adapt >> to any MTD that implements mtd_read_oob()/mtd_write_oob(). We'd just >> need to parameterize a few relevant device details into a new nand_bbt >> struct, rather than using struct nand_chip directly." >> >> To abstract a new nand_bbt struct instead of nand_chip to make SPI NAND >> and parallel NAND can share nand_bbt.c file, I already begin to work on >> this. >> >> For code shared in nand_base.c, I agree it would be better if we can find >> a good method to share nand_base.c code between spi nand and parallel >nand. >> But frankly speaking, I'm not satisfied for the remap command method. >This >> method make code difficult to maintain when SPI NAND and Parallel NAND >> evolve much differently in the future. >> >> Take some example, >> If one new command (cache operation, multiple plane operation) >implemented >> in parallel NAND code, and is used in nand_read or nand_write, that will >> cause maintainer to modify SPI NAND code to remap this new command, >though >> this modification probably could be slight. That means modification on >> Parallel NAND flash need to consider SPI NAND as well. >> >> How do you think about this? >> >> For Peter Pan's patchset, if we do some modification to make nand_bbt.c >to >> make it shareable for Parallel and SPI NAND. The code line should be 2000. >> I believe I can review this spi-nand-base.c to remove some redundant code >> that may hundreds line. Is 1700 or 1800 code line is more acceptable? >> >> Let me know your opinions. >> > >Sounds good. > >Do you still plan to maintain the spi-nand-base.c and spi-nand-device.c >separation? Yes, still plan to maintain the spi-nand-base.c and spi-nand-device.c separation. Abstract common code to spi-nand-base.c, and spi-nand-device.c is used for realize the different function for different SPI NAND, such as ecc layout, read ID etc. Thanks -- Qi Wang ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥ ^ permalink raw reply [flat|nested] 32+ messages in thread
* RE: [PATCH 0/3] An alternative to SPI NAND @ 2015-01-21 2:11 ` Qi Wang 王起 (qiwang) 0 siblings, 0 replies; 32+ messages in thread From: Qi Wang 王起 (qiwang) @ 2015-01-21 2:11 UTC (permalink / raw) To: Ezequiel Garcia, Brian Norris Cc: linux-kernel, linux-mtd, Frank Liu 刘群 (frankliu), Melanie Zhang 张燕 (melaniezhang), dwmw2, Peter Pan 潘栋 (peterpandong) On 01/20/2015 6:36 PM, Ezequiel Garcia wrote: > >On 01/12/2015 12:10 PM, Qi Wang 王起 (qiwang) wrote: >> Hi Ezequiel, >> >> On 01/08/2015 11:27 AM, Ezequiel Garcia wrote: >>> >>> Hi Qi Wang, >>> >>> On 01/07/2015 11:45 PM, Qi Wang 王起 (qiwang) wrote: >>>> Hi Brian, >>>> >>>> On Thu, Jan 08, 2015 at 9:03:24AM +0000, Brian Norris wrote: >>>>> >>>>> On Thu, Jan 08, 2015 at 12:47:24AM +0000, Peter Pan 潘栋 (peterpandong) >>>>> wrote: >>>>>> Documentation/devicetree/bindings/mtd/spi-nand.txt | 22 + >>>>>> drivers/mtd/Kconfig | 2 + >>>>>> drivers/mtd/Makefile | 1 + >>>>>> drivers/mtd/spi-nand/Kconfig | 7 + >>>>>> drivers/mtd/spi-nand/Makefile | 3 + >>>>>> drivers/mtd/spi-nand/spi-nand-base.c | 2034 >>>>> ++++++++++++++++++++ >>>>>> drivers/mtd/spi-nand/spi-nand-bbt.c | 1279 >++++++++++++ >>>>> >>>>> I can already tell by the diffstat that I don't like this. We probably >>>>> don't need 3000 new lines of code for this, but we especially don't >want >>>>> to duplicate nand_bbt.c. It won't take a lot of work to augment >>>>> nand_bbt.c to make it shareable. (I can whip that patch up if needed.) >>>> >>>> Yes, I agree with you, Nand_bbt.c do can be shared by Parallel NAND and >>>> SPI NAND. Actually, we are working at this now. Will send patches to >you >>>> Once we finished it. >>>> >>> >>> Thanks for the quick submission! >>> >>> However, Brian is right, this code duplication is a no go. >>> >>> Perhaps a more valid approach would be to first identify the code that >>> needs to be shared in nand_bbt.c and nand_base.c, and export those >>> symbols (or maybe do the required refactor). >> >> Yes, I agree Brian's suggestion in another mail. >> >> " The BBT code is something we definitely want to share, but it's >actually >> not very closely tied to nand_base.c, and it looks pretty easy to adapt >> to any MTD that implements mtd_read_oob()/mtd_write_oob(). We'd just >> need to parameterize a few relevant device details into a new nand_bbt >> struct, rather than using struct nand_chip directly." >> >> To abstract a new nand_bbt struct instead of nand_chip to make SPI NAND >> and parallel NAND can share nand_bbt.c file, I already begin to work on >> this. >> >> For code shared in nand_base.c, I agree it would be better if we can find >> a good method to share nand_base.c code between spi nand and parallel >nand. >> But frankly speaking, I'm not satisfied for the remap command method. >This >> method make code difficult to maintain when SPI NAND and Parallel NAND >> evolve much differently in the future. >> >> Take some example, >> If one new command (cache operation, multiple plane operation) >implemented >> in parallel NAND code, and is used in nand_read or nand_write, that will >> cause maintainer to modify SPI NAND code to remap this new command, >though >> this modification probably could be slight. That means modification on >> Parallel NAND flash need to consider SPI NAND as well. >> >> How do you think about this? >> >> For Peter Pan's patchset, if we do some modification to make nand_bbt.c >to >> make it shareable for Parallel and SPI NAND. The code line should be 2000. >> I believe I can review this spi-nand-base.c to remove some redundant code >> that may hundreds line. Is 1700 or 1800 code line is more acceptable? >> >> Let me know your opinions. >> > >Sounds good. > >Do you still plan to maintain the spi-nand-base.c and spi-nand-device.c >separation? Yes, still plan to maintain the spi-nand-base.c and spi-nand-device.c separation. Abstract common code to spi-nand-base.c, and spi-nand-device.c is used for realize the different function for different SPI NAND, such as ecc layout, read ID etc. Thanks -- Qi Wang ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 0/3] An alternative to SPI NAND 2015-01-21 2:11 ` Qi Wang 王起 (qiwang) @ 2015-01-29 18:03 ` Ezequiel Garcia -1 siblings, 0 replies; 32+ messages in thread From: Ezequiel Garcia @ 2015-01-29 18:03 UTC (permalink / raw) To: "Qi Wang 王起 (qiwang)", Brian Norris Cc: dwmw2, linux-kernel, linux-mtd, "Frank Liu 刘群 (frankliu)", "Melanie Zhang 张燕 (melaniezhang)", "Peter Pan 潘栋 (peterpandong)" On 01/20/2015 11:11 PM, Qi Wang 王起 (qiwang) wrote: > On 01/20/2015 6:36 PM, Ezequiel Garcia wrote: >> >> On 01/12/2015 12:10 PM, Qi Wang 王起 (qiwang) wrote: >>> Hi Ezequiel, >>> >>> On 01/08/2015 11:27 AM, Ezequiel Garcia wrote: >>>> >>>> Hi Qi Wang, >>>> >>>> On 01/07/2015 11:45 PM, Qi Wang 王起 (qiwang) wrote: >>>>> Hi Brian, >>>>> >>>>> On Thu, Jan 08, 2015 at 9:03:24AM +0000, Brian Norris wrote: >>>>>> >>>>>> On Thu, Jan 08, 2015 at 12:47:24AM +0000, Peter Pan 潘栋 (peterpandong) >>>>>> wrote: >>>>>>> Documentation/devicetree/bindings/mtd/spi-nand.txt | 22 + >>>>>>> drivers/mtd/Kconfig | 2 + >>>>>>> drivers/mtd/Makefile | 1 + >>>>>>> drivers/mtd/spi-nand/Kconfig | 7 + >>>>>>> drivers/mtd/spi-nand/Makefile | 3 + >>>>>>> drivers/mtd/spi-nand/spi-nand-base.c | 2034 >>>>>> ++++++++++++++++++++ >>>>>>> drivers/mtd/spi-nand/spi-nand-bbt.c | 1279 >> ++++++++++++ >>>>>> >>>>>> I can already tell by the diffstat that I don't like this. We probably >>>>>> don't need 3000 new lines of code for this, but we especially don't >> want >>>>>> to duplicate nand_bbt.c. It won't take a lot of work to augment >>>>>> nand_bbt.c to make it shareable. (I can whip that patch up if needed.) >>>>> >>>>> Yes, I agree with you, Nand_bbt.c do can be shared by Parallel NAND and >>>>> SPI NAND. Actually, we are working at this now. Will send patches to >> you >>>>> Once we finished it. >>>>> >>>> >>>> Thanks for the quick submission! >>>> >>>> However, Brian is right, this code duplication is a no go. >>>> >>>> Perhaps a more valid approach would be to first identify the code that >>>> needs to be shared in nand_bbt.c and nand_base.c, and export those >>>> symbols (or maybe do the required refactor). >>> >>> Yes, I agree Brian's suggestion in another mail. >>> >>> " The BBT code is something we definitely want to share, but it's >> actually >>> not very closely tied to nand_base.c, and it looks pretty easy to adapt >>> to any MTD that implements mtd_read_oob()/mtd_write_oob(). We'd just >>> need to parameterize a few relevant device details into a new nand_bbt >>> struct, rather than using struct nand_chip directly." >>> >>> To abstract a new nand_bbt struct instead of nand_chip to make SPI NAND >>> and parallel NAND can share nand_bbt.c file, I already begin to work on >>> this. >>> >>> For code shared in nand_base.c, I agree it would be better if we can find >>> a good method to share nand_base.c code between spi nand and parallel >> nand. >>> But frankly speaking, I'm not satisfied for the remap command method. >> This >>> method make code difficult to maintain when SPI NAND and Parallel NAND >>> evolve much differently in the future. >>> >>> Take some example, >>> If one new command (cache operation, multiple plane operation) >> implemented >>> in parallel NAND code, and is used in nand_read or nand_write, that will >>> cause maintainer to modify SPI NAND code to remap this new command, >> though >>> this modification probably could be slight. That means modification on >>> Parallel NAND flash need to consider SPI NAND as well. >>> >>> How do you think about this? >>> >>> For Peter Pan's patchset, if we do some modification to make nand_bbt.c >> to >>> make it shareable for Parallel and SPI NAND. The code line should be 2000. >>> I believe I can review this spi-nand-base.c to remove some redundant code >>> that may hundreds line. Is 1700 or 1800 code line is more acceptable? >>> >>> Let me know your opinions. >>> >> >> Sounds good. >> >> Do you still plan to maintain the spi-nand-base.c and spi-nand-device.c >> separation? > > Yes, still plan to maintain the spi-nand-base.c and spi-nand-device.c > separation. Abstract common code to spi-nand-base.c, and spi-nand-device.c is > used for realize the different function for different SPI NAND, such as ecc > layout, read ID etc. > Any news about this? Is there anything I can do to help (reviewing, testing, coding...)? Thanks! -- Ezequiel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 0/3] An alternative to SPI NAND @ 2015-01-29 18:03 ` Ezequiel Garcia 0 siblings, 0 replies; 32+ messages in thread From: Ezequiel Garcia @ 2015-01-29 18:03 UTC (permalink / raw) To: "Qi Wang 王起 (qiwang)", Brian Norris Cc: linux-kernel, linux-mtd, "Frank Liu 刘群 (frankliu)", "Melanie Zhang 张燕 (melaniezhang)", dwmw2, "Peter Pan 潘栋 (peterpandong)" On 01/20/2015 11:11 PM, Qi Wang 王起 (qiwang) wrote: > On 01/20/2015 6:36 PM, Ezequiel Garcia wrote: >> >> On 01/12/2015 12:10 PM, Qi Wang 王起 (qiwang) wrote: >>> Hi Ezequiel, >>> >>> On 01/08/2015 11:27 AM, Ezequiel Garcia wrote: >>>> >>>> Hi Qi Wang, >>>> >>>> On 01/07/2015 11:45 PM, Qi Wang 王起 (qiwang) wrote: >>>>> Hi Brian, >>>>> >>>>> On Thu, Jan 08, 2015 at 9:03:24AM +0000, Brian Norris wrote: >>>>>> >>>>>> On Thu, Jan 08, 2015 at 12:47:24AM +0000, Peter Pan 潘栋 (peterpandong) >>>>>> wrote: >>>>>>> Documentation/devicetree/bindings/mtd/spi-nand.txt | 22 + >>>>>>> drivers/mtd/Kconfig | 2 + >>>>>>> drivers/mtd/Makefile | 1 + >>>>>>> drivers/mtd/spi-nand/Kconfig | 7 + >>>>>>> drivers/mtd/spi-nand/Makefile | 3 + >>>>>>> drivers/mtd/spi-nand/spi-nand-base.c | 2034 >>>>>> ++++++++++++++++++++ >>>>>>> drivers/mtd/spi-nand/spi-nand-bbt.c | 1279 >> ++++++++++++ >>>>>> >>>>>> I can already tell by the diffstat that I don't like this. We probably >>>>>> don't need 3000 new lines of code for this, but we especially don't >> want >>>>>> to duplicate nand_bbt.c. It won't take a lot of work to augment >>>>>> nand_bbt.c to make it shareable. (I can whip that patch up if needed.) >>>>> >>>>> Yes, I agree with you, Nand_bbt.c do can be shared by Parallel NAND and >>>>> SPI NAND. Actually, we are working at this now. Will send patches to >> you >>>>> Once we finished it. >>>>> >>>> >>>> Thanks for the quick submission! >>>> >>>> However, Brian is right, this code duplication is a no go. >>>> >>>> Perhaps a more valid approach would be to first identify the code that >>>> needs to be shared in nand_bbt.c and nand_base.c, and export those >>>> symbols (or maybe do the required refactor). >>> >>> Yes, I agree Brian's suggestion in another mail. >>> >>> " The BBT code is something we definitely want to share, but it's >> actually >>> not very closely tied to nand_base.c, and it looks pretty easy to adapt >>> to any MTD that implements mtd_read_oob()/mtd_write_oob(). We'd just >>> need to parameterize a few relevant device details into a new nand_bbt >>> struct, rather than using struct nand_chip directly." >>> >>> To abstract a new nand_bbt struct instead of nand_chip to make SPI NAND >>> and parallel NAND can share nand_bbt.c file, I already begin to work on >>> this. >>> >>> For code shared in nand_base.c, I agree it would be better if we can find >>> a good method to share nand_base.c code between spi nand and parallel >> nand. >>> But frankly speaking, I'm not satisfied for the remap command method. >> This >>> method make code difficult to maintain when SPI NAND and Parallel NAND >>> evolve much differently in the future. >>> >>> Take some example, >>> If one new command (cache operation, multiple plane operation) >> implemented >>> in parallel NAND code, and is used in nand_read or nand_write, that will >>> cause maintainer to modify SPI NAND code to remap this new command, >> though >>> this modification probably could be slight. That means modification on >>> Parallel NAND flash need to consider SPI NAND as well. >>> >>> How do you think about this? >>> >>> For Peter Pan's patchset, if we do some modification to make nand_bbt.c >> to >>> make it shareable for Parallel and SPI NAND. The code line should be 2000. >>> I believe I can review this spi-nand-base.c to remove some redundant code >>> that may hundreds line. Is 1700 or 1800 code line is more acceptable? >>> >>> Let me know your opinions. >>> >> >> Sounds good. >> >> Do you still plan to maintain the spi-nand-base.c and spi-nand-device.c >> separation? > > Yes, still plan to maintain the spi-nand-base.c and spi-nand-device.c > separation. Abstract common code to spi-nand-base.c, and spi-nand-device.c is > used for realize the different function for different SPI NAND, such as ecc > layout, read ID etc. > Any news about this? Is there anything I can do to help (reviewing, testing, coding...)? Thanks! -- Ezequiel ^ permalink raw reply [flat|nested] 32+ messages in thread
* RE: [PATCH 0/3] An alternative to SPI NAND 2015-01-29 18:03 ` Ezequiel Garcia @ 2015-01-30 0:57 ` Peter Pan 潘栋 (peterpandong) -1 siblings, 0 replies; 32+ messages in thread From: Peter Pan 潘栋 (peterpandong) @ 2015-01-30 0:57 UTC (permalink / raw) To: Ezequiel Garcia, Qi Wang 王起 (qiwang), Brian Norris Cc: dwmw2, linux-kernel, linux-mtd, Frank Liu 刘群 (frankliu), Melanie Zhang 张燕 (melaniezhang) [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="utf-8", Size: 6619 bytes --] > On 01/20/2015 11:11 PM, Qi Wang çèµ· (qiwang) wrote: > > On 01/20/2015 6:36 PM, Ezequiel Garcia wrote: > >> > >> On 01/12/2015 12:10 PM, Qi Wang çèµ· (qiwang) wrote: > >>> Hi Ezequiel, > >>> > >>> On 01/08/2015 11:27 AM, Ezequiel Garcia wrote: > >>>> > >>>> Hi Qi Wang, > >>>> > >>>> On 01/07/2015 11:45 PM, Qi Wang çèµ· (qiwang) wrote: > >>>>> Hi Brian, > >>>>> > >>>>> On Thu, Jan 08, 2015 at 9:03:24AM +0000, Brian Norris wrote: > >>>>>> > >>>>>> On Thu, Jan 08, 2015 at 12:47:24AM +0000, Peter Pan æ½æ > (peterpandong) > >>>>>> wrote: > >>>>>>> Documentation/devicetree/bindings/mtd/spi-nand.txt | 22 + > >>>>>>> drivers/mtd/Kconfig | 2 + > >>>>>>> drivers/mtd/Makefile | 1 + > >>>>>>> drivers/mtd/spi-nand/Kconfig | 7 + > >>>>>>> drivers/mtd/spi-nand/Makefile | 3 + > >>>>>>> drivers/mtd/spi-nand/spi-nand-base.c | 2034 > >>>>>> ++++++++++++++++++++ > >>>>>>> drivers/mtd/spi-nand/spi-nand-bbt.c | 1279 > >> ++++++++++++ > >>>>>> > >>>>>> I can already tell by the diffstat that I don't like this. We > probably > >>>>>> don't need 3000 new lines of code for this, but we especially > don't > >> want > >>>>>> to duplicate nand_bbt.c. It won't take a lot of work to augment > >>>>>> nand_bbt.c to make it shareable. (I can whip that patch up if > needed.) > >>>>> > >>>>> Yes, I agree with you, Nand_bbt.c do can be shared by Parallel > NAND and > >>>>> SPI NAND. Actually, we are working at this now. Will send patches > to > >> you > >>>>> Once we finished it. > >>>>> > >>>> > >>>> Thanks for the quick submission! > >>>> > >>>> However, Brian is right, this code duplication is a no go. > >>>> > >>>> Perhaps a more valid approach would be to first identify the code > that > >>>> needs to be shared in nand_bbt.c and nand_base.c, and export those > >>>> symbols (or maybe do the required refactor). > >>> > >>> Yes, I agree Brian's suggestion in another mail. > >>> > >>> " The BBT code is something we definitely want to share, but it's > >> actually > >>> not very closely tied to nand_base.c, and it looks pretty easy to > adapt > >>> to any MTD that implements mtd_read_oob()/mtd_write_oob(). We'd > just > >>> need to parameterize a few relevant device details into a new > nand_bbt > >>> struct, rather than using struct nand_chip directly." > >>> > >>> To abstract a new nand_bbt struct instead of nand_chip to make SPI > NAND > >>> and parallel NAND can share nand_bbt.c file, I already begin to > work on > >>> this. > >>> > >>> For code shared in nand_base.c, I agree it would be better if we > can find > >>> a good method to share nand_base.c code between spi nand and > parallel > >> nand. > >>> But frankly speaking, I'm not satisfied for the remap command > method. > >> This > >>> method make code difficult to maintain when SPI NAND and Parallel > NAND > >>> evolve much differently in the future. > >>> > >>> Take some example, > >>> If one new command (cache operation, multiple plane operation) > >> implemented > >>> in parallel NAND code, and is used in nand_read or nand_write, that > will > >>> cause maintainer to modify SPI NAND code to remap this new command, > >> though > >>> this modification probably could be slight. That means modification > on > >>> Parallel NAND flash need to consider SPI NAND as well. > >>> > >>> How do you think about this? > >>> > >>> For Peter Pan's patchset, if we do some modification to make > nand_bbt.c > >> to > >>> make it shareable for Parallel and SPI NAND. The code line should > be 2000. > >>> I believe I can review this spi-nand-base.c to remove some > redundant code > >>> that may hundreds line. Is 1700 or 1800 code line is more > acceptable? > >>> > >>> Let me know your opinions. > >>> > >> > >> Sounds good. > >> > >> Do you still plan to maintain the spi-nand-base.c and spi-nand- > device.c > >> separation? > > > > Yes, still plan to maintain the spi-nand-base.c and spi-nand-device.c > > separation. Abstract common code to spi-nand-base.c, and spi-nand- > device.c is > > used for realize the different function for different SPI NAND, such > as ecc > > layout, read ID etc. > > > > Any news about this? Is there anything I can do to help (reviewing, > testing, coding...)? > > Thanks! > -- > Ezequiel Currently, we are working on sharing the bbt code. I think your and Brain's suggestion will be very helpful. There are two options. We can put struct nand_bbt pointer in either nand_chip or mtd_info structure. If put nand_bbt in nand_chip, we need to change the parameter of nand_chip->scan_bbt function from mtd_info to nand_bbt. But nand_chip->scan_bbt is used in many place. drivers/mtd/nand/diskonchip.c:1372: this->scan_bbt = nftl_scan_bbt; drivers/mtd/nand/diskonchip.c:1399: this->scan_bbt = inftl_scan_bbt; drivers/mtd/nand/diskonchip.c:1405: this->scan_bbt = nftl_scan_bbt; drivers/mtd/nand/diskonchip.c:1418: this->scan_bbt = inftl_scan_bbt; drivers/mtd/nand/nand_base.c:2986: if (!chip->scan_bbt) drivers/mtd/nand/nand_base.c:2987: chip->scan_bbt = nand_default_bbt; drivers/mtd/nand/nand_base.c:4159: * Initialize bitflip_threshold to its default prior scan_bbt() call. drivers/mtd/nand/nand_base.c:4160: * scan_bbt() might invoke mtd_read(), thus bitflip_threshold must be drivers/mtd/nand/nand_base.c:4171: return chip->scan_bbt(mtd); drivers/mtd/nand/gpmi-nand/gpmi-nand.c:1953: chip->scan_bbt(mtd); drivers/mtd/nand/docg4.c:1083: * first page of the block. The default scan_bbt() in the nand drivers/mtd/nand/nandsim.c:2381: if ((retval = chip->scan_bbt(nsmtd)) != 0) drivers/mtd/onenand/onenand_base.c:3976: if (!this->scan_bbt) drivers/mtd/onenand/onenand_base.c:3977: this->scan_bbt = onenand_default_bbt; drivers/mtd/onenand/onenand_base.c:4101: ret = this->scan_bbt(mtd); If put nand_bbt in mtd_info, we needn't to change the parameter of nand_chip->scan_bbt. But only nand flash need bbt, I don't know whether it is proper to put nand_bbt structure in mtd_info or not. Besides, using nand_bbt struct will cause some elements(such as chip_size, page_size and so on) in both nand_chip and nand_bbt struct. Maybe there is another way but I don't know. So please feel free to talk about it. Brain, could you give some suggestion ? We really need your help. ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥ ^ permalink raw reply [flat|nested] 32+ messages in thread
* RE: [PATCH 0/3] An alternative to SPI NAND @ 2015-01-30 0:57 ` Peter Pan 潘栋 (peterpandong) 0 siblings, 0 replies; 32+ messages in thread From: Peter Pan 潘栋 (peterpandong) @ 2015-01-30 0:57 UTC (permalink / raw) To: Ezequiel Garcia, Qi Wang 王起 (qiwang), Brian Norris Cc: linux-mtd, Melanie Zhang 张燕 (melaniezhang), dwmw2, linux-kernel, Frank Liu 刘群 (frankliu) > On 01/20/2015 11:11 PM, Qi Wang 王起 (qiwang) wrote: > > On 01/20/2015 6:36 PM, Ezequiel Garcia wrote: > >> > >> On 01/12/2015 12:10 PM, Qi Wang 王起 (qiwang) wrote: > >>> Hi Ezequiel, > >>> > >>> On 01/08/2015 11:27 AM, Ezequiel Garcia wrote: > >>>> > >>>> Hi Qi Wang, > >>>> > >>>> On 01/07/2015 11:45 PM, Qi Wang 王起 (qiwang) wrote: > >>>>> Hi Brian, > >>>>> > >>>>> On Thu, Jan 08, 2015 at 9:03:24AM +0000, Brian Norris wrote: > >>>>>> > >>>>>> On Thu, Jan 08, 2015 at 12:47:24AM +0000, Peter Pan 潘栋 > (peterpandong) > >>>>>> wrote: > >>>>>>> Documentation/devicetree/bindings/mtd/spi-nand.txt | 22 + > >>>>>>> drivers/mtd/Kconfig | 2 + > >>>>>>> drivers/mtd/Makefile | 1 + > >>>>>>> drivers/mtd/spi-nand/Kconfig | 7 + > >>>>>>> drivers/mtd/spi-nand/Makefile | 3 + > >>>>>>> drivers/mtd/spi-nand/spi-nand-base.c | 2034 > >>>>>> ++++++++++++++++++++ > >>>>>>> drivers/mtd/spi-nand/spi-nand-bbt.c | 1279 > >> ++++++++++++ > >>>>>> > >>>>>> I can already tell by the diffstat that I don't like this. We > probably > >>>>>> don't need 3000 new lines of code for this, but we especially > don't > >> want > >>>>>> to duplicate nand_bbt.c. It won't take a lot of work to augment > >>>>>> nand_bbt.c to make it shareable. (I can whip that patch up if > needed.) > >>>>> > >>>>> Yes, I agree with you, Nand_bbt.c do can be shared by Parallel > NAND and > >>>>> SPI NAND. Actually, we are working at this now. Will send patches > to > >> you > >>>>> Once we finished it. > >>>>> > >>>> > >>>> Thanks for the quick submission! > >>>> > >>>> However, Brian is right, this code duplication is a no go. > >>>> > >>>> Perhaps a more valid approach would be to first identify the code > that > >>>> needs to be shared in nand_bbt.c and nand_base.c, and export those > >>>> symbols (or maybe do the required refactor). > >>> > >>> Yes, I agree Brian's suggestion in another mail. > >>> > >>> " The BBT code is something we definitely want to share, but it's > >> actually > >>> not very closely tied to nand_base.c, and it looks pretty easy to > adapt > >>> to any MTD that implements mtd_read_oob()/mtd_write_oob(). We'd > just > >>> need to parameterize a few relevant device details into a new > nand_bbt > >>> struct, rather than using struct nand_chip directly." > >>> > >>> To abstract a new nand_bbt struct instead of nand_chip to make SPI > NAND > >>> and parallel NAND can share nand_bbt.c file, I already begin to > work on > >>> this. > >>> > >>> For code shared in nand_base.c, I agree it would be better if we > can find > >>> a good method to share nand_base.c code between spi nand and > parallel > >> nand. > >>> But frankly speaking, I'm not satisfied for the remap command > method. > >> This > >>> method make code difficult to maintain when SPI NAND and Parallel > NAND > >>> evolve much differently in the future. > >>> > >>> Take some example, > >>> If one new command (cache operation, multiple plane operation) > >> implemented > >>> in parallel NAND code, and is used in nand_read or nand_write, that > will > >>> cause maintainer to modify SPI NAND code to remap this new command, > >> though > >>> this modification probably could be slight. That means modification > on > >>> Parallel NAND flash need to consider SPI NAND as well. > >>> > >>> How do you think about this? > >>> > >>> For Peter Pan's patchset, if we do some modification to make > nand_bbt.c > >> to > >>> make it shareable for Parallel and SPI NAND. The code line should > be 2000. > >>> I believe I can review this spi-nand-base.c to remove some > redundant code > >>> that may hundreds line. Is 1700 or 1800 code line is more > acceptable? > >>> > >>> Let me know your opinions. > >>> > >> > >> Sounds good. > >> > >> Do you still plan to maintain the spi-nand-base.c and spi-nand- > device.c > >> separation? > > > > Yes, still plan to maintain the spi-nand-base.c and spi-nand-device.c > > separation. Abstract common code to spi-nand-base.c, and spi-nand- > device.c is > > used for realize the different function for different SPI NAND, such > as ecc > > layout, read ID etc. > > > > Any news about this? Is there anything I can do to help (reviewing, > testing, coding...)? > > Thanks! > -- > Ezequiel Currently, we are working on sharing the bbt code. I think your and Brain's suggestion will be very helpful. There are two options. We can put struct nand_bbt pointer in either nand_chip or mtd_info structure. If put nand_bbt in nand_chip, we need to change the parameter of nand_chip->scan_bbt function from mtd_info to nand_bbt. But nand_chip->scan_bbt is used in many place. drivers/mtd/nand/diskonchip.c:1372: this->scan_bbt = nftl_scan_bbt; drivers/mtd/nand/diskonchip.c:1399: this->scan_bbt = inftl_scan_bbt; drivers/mtd/nand/diskonchip.c:1405: this->scan_bbt = nftl_scan_bbt; drivers/mtd/nand/diskonchip.c:1418: this->scan_bbt = inftl_scan_bbt; drivers/mtd/nand/nand_base.c:2986: if (!chip->scan_bbt) drivers/mtd/nand/nand_base.c:2987: chip->scan_bbt = nand_default_bbt; drivers/mtd/nand/nand_base.c:4159: * Initialize bitflip_threshold to its default prior scan_bbt() call. drivers/mtd/nand/nand_base.c:4160: * scan_bbt() might invoke mtd_read(), thus bitflip_threshold must be drivers/mtd/nand/nand_base.c:4171: return chip->scan_bbt(mtd); drivers/mtd/nand/gpmi-nand/gpmi-nand.c:1953: chip->scan_bbt(mtd); drivers/mtd/nand/docg4.c:1083: * first page of the block. The default scan_bbt() in the nand drivers/mtd/nand/nandsim.c:2381: if ((retval = chip->scan_bbt(nsmtd)) != 0) drivers/mtd/onenand/onenand_base.c:3976: if (!this->scan_bbt) drivers/mtd/onenand/onenand_base.c:3977: this->scan_bbt = onenand_default_bbt; drivers/mtd/onenand/onenand_base.c:4101: ret = this->scan_bbt(mtd); If put nand_bbt in mtd_info, we needn't to change the parameter of nand_chip->scan_bbt. But only nand flash need bbt, I don't know whether it is proper to put nand_bbt structure in mtd_info or not. Besides, using nand_bbt struct will cause some elements(such as chip_size, page_size and so on) in both nand_chip and nand_bbt struct. Maybe there is another way but I don't know. So please feel free to talk about it. Brain, could you give some suggestion ? We really need your help. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 0/3] An alternative to SPI NAND 2015-01-30 0:57 ` Peter Pan 潘栋 (peterpandong) @ 2015-01-30 11:47 ` Ezequiel Garcia -1 siblings, 0 replies; 32+ messages in thread From: Ezequiel Garcia @ 2015-01-30 11:47 UTC (permalink / raw) To: "Peter Pan 潘栋 (peterpandong)", "Qi Wang 王起 (qiwang)", Brian Norris Cc: dwmw2, linux-kernel, linux-mtd, "Frank Liu 刘群 (frankliu)", "Melanie Zhang 张燕 (melaniezhang)", James Hartley, Andrew Bresticker On 01/29/2015 09:57 PM, Peter Pan 潘栋 (peterpandong) wrote: [..] > > Currently, we are working on sharing the bbt code. I think your and Brain's suggestion > will be very helpful. > > There are two options. We can put struct nand_bbt pointer in either nand_chip > or mtd_info structure. I'm sorry, you lost me here. Do you mean struct nand_bbt_descr ? > If put nand_bbt in nand_chip, we need to change the I thought the plan was NOT to base spi-nand on nand, so you can't put this in nand_chip, can you? Also: After looking at the nand_bbt.c file, I'm wondering how promising is to separate it from NAND. It seems the BBT code is quite attached to NAND! Are you planning to do this in just one patch? Maybe it's better to start simple and prepare small patches that gradually make the nand_base.c and nand_bbt.c files less dependent. For instance, you can get rid of the memory release in a first patch: /* Free bad block table memory */ kfree(chip->bbt); if (!(chip->options & NAND_OWN_BUFFERS)) kfree(chip->buffers); /* Free bad block descriptor memory */ if (chip->badblock_pattern && chip->badblock_pattern->options & NAND_BBT_DYNAMICSTRUCT) kfree(chip->badblock_pattern); by moving it to some nand_bbt_release() function. I might be pushing some patches to do this, as I think it can be useful in general to clean this code. -- Ezequiel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 0/3] An alternative to SPI NAND @ 2015-01-30 11:47 ` Ezequiel Garcia 0 siblings, 0 replies; 32+ messages in thread From: Ezequiel Garcia @ 2015-01-30 11:47 UTC (permalink / raw) To: "Peter Pan 潘栋 (peterpandong)", "Qi Wang 王起 (qiwang)", Brian Norris Cc: Andrew Bresticker, linux-kernel, linux-mtd, "Frank Liu 刘群 (frankliu)", "Melanie Zhang 张燕 (melaniezhang)", dwmw2, James Hartley On 01/29/2015 09:57 PM, Peter Pan 潘栋 (peterpandong) wrote: [..] > > Currently, we are working on sharing the bbt code. I think your and Brain's suggestion > will be very helpful. > > There are two options. We can put struct nand_bbt pointer in either nand_chip > or mtd_info structure. I'm sorry, you lost me here. Do you mean struct nand_bbt_descr ? > If put nand_bbt in nand_chip, we need to change the I thought the plan was NOT to base spi-nand on nand, so you can't put this in nand_chip, can you? Also: After looking at the nand_bbt.c file, I'm wondering how promising is to separate it from NAND. It seems the BBT code is quite attached to NAND! Are you planning to do this in just one patch? Maybe it's better to start simple and prepare small patches that gradually make the nand_base.c and nand_bbt.c files less dependent. For instance, you can get rid of the memory release in a first patch: /* Free bad block table memory */ kfree(chip->bbt); if (!(chip->options & NAND_OWN_BUFFERS)) kfree(chip->buffers); /* Free bad block descriptor memory */ if (chip->badblock_pattern && chip->badblock_pattern->options & NAND_BBT_DYNAMICSTRUCT) kfree(chip->badblock_pattern); by moving it to some nand_bbt_release() function. I might be pushing some patches to do this, as I think it can be useful in general to clean this code. -- Ezequiel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 0/3] An alternative to SPI NAND 2015-01-30 11:47 ` Ezequiel Garcia @ 2015-01-31 7:02 ` Brian Norris -1 siblings, 0 replies; 32+ messages in thread From: Brian Norris @ 2015-01-31 7:02 UTC (permalink / raw) To: Ezequiel Garcia Cc: "Peter Pan 潘栋 (peterpandong)", "Qi Wang 王起 (qiwang)", dwmw2, linux-kernel, linux-mtd, "Frank Liu 刘群 (frankliu)", "Melanie Zhang 张燕 (melaniezhang)", James Hartley, Andrew Bresticker On Fri, Jan 30, 2015 at 08:47:29AM -0300, Ezequiel Garcia wrote: > On 01/29/2015 09:57 PM, Peter Pan 潘栋 (peterpandong) wrote: > [..] > > > > Currently, we are working on sharing the bbt code. I think your and Brain's suggestion > > will be very helpful. > > > > There are two options. We can put struct nand_bbt pointer in either nand_chip > > or mtd_info structure. > > I'm sorry, you lost me here. Do you mean struct nand_bbt_descr ? I'm assuming he's suggesting a new struct that he's calling nand_bbt. > > If put nand_bbt in nand_chip, we need to change the > > I thought the plan was NOT to base spi-nand on nand, so you can't put > this in nand_chip, can you? You could. It's just a matter of who does the pointer chasing. (It's just important that nand_bbt.c doesn't *assume* that it has nand_chip as a parent.) If struct nand_bbt is embedded directly in struct mtd_info, then nand_bbt.c could implement functions such that its users can just do this: mtd->_block_isbad = nand_bbt_blockisbad; But if you don't (and instead embed it in a custom location like nand_chip), we'd have to write wrappers that call the nand_bbt_* functions (like nand_base does today). e.g.: mtd->_block_isbad = spi_nand_blockisbad; int spi_nand_block_isbad(struct mtd_info *mtd, loff_t offs) { struct spi_nand_chip *chip = mtd->priv; struct nand_bbt *bbt = chip->bbt; return nand_bbt_blockisbad(bbt, offs); } Anyway, I think it's worth laying out exactly where the pieces are going to fit here. I reckon we need the following: 1. a short list of parameters that nand_bbt needs from the NAND implementation (either nand_base or spi-nand-*) 2. a list of parameters that need to be kept around for nand_bbt bookkeeping / handling 3. an init function (nand_bbt_init()?) that takes #1 and computes #2 3(a) a corresponding release function 4. a set of functions that, given only a few obvious parameters (e.g., block offsets) and a struct that holds #2, handle all BBT needs (e.g., bad block lookup, bad block scanning, marking new bad blocks) Since I see #1 and #2 overlapping quite a bit, we might combine them, where several parameters are expected to be set by the nand_bbt user (either nand_base.c or nand_bbt.c), and a few are considered private to nand_bbt. struct nand_bbt { struct mtd_info *mtd; /* * This is important to abstract out of nand_bbt.c and provide * separately in nand_base.c and spi-nand-base.c -- it's sort of * duplicated in nand_block_bad() (nand_base) and * scan_block_fast() (nand_bbt) right now * * Note that this also means nand_chip.badblock_pattern should * be removed from nand_bbt.c */ int (*is_bad_bbm)(struct mtd_info *mtd, loff_t ofs); /* Only required if the driver wants to attempt to program new * bad block markers that imitate the factory-marked BBMs */ int (*mark_bad_bbm)(struct mtd_info *mtd, loff_t ofs); unsigned int bbt_options; /* Only required for NAND_BBT_PERCHIP */ int numchips; /* Discourage new customer usages here; suggest usage of the * relevant NAND_BBT_* options instead */ struct nand_bbt_descr *bbt_td; struct nand_bbt_descr *bbt_md; /* The rest are filled in by nand_bbt_init() */ int bbt_erase_shift; int page_shift; u8 *bbt; }; #3 might simply look like: int nand_bbt_init(struct nand_bbt *bbt); void nand_bbt_release(struct nand_bbt *bbt); #4 might look like the following APIs for nand_bbt.c (not too different than we have now): int nand_bbt_init(struct nand_bbt *bbt); /* init + scan? */ int nand_bbt_markbad(struct nand_bbt *bbt, loff_t offs); int nand_bbt_isreserved(struct nand_bbt *bbt, loff_t offs); int nand_bbt_isbad(struct nand_bbt *bbt, loff_t offs); (The above allows for nand_bbt to be embedded anywhere; we could modify this API to be drop-in replacements for the mtd_info callbacks if we embed struct nand_bbt in mtd_info.) > Also: After looking at the nand_bbt.c file, I'm wondering how promising > is to separate it from NAND. It seems the BBT code is quite attached to > NAND! There will be quite a bit of churn, but I don't think that it is too strongly attached. > Are you planning to do this in just one patch? Maybe it's better to > start simple and prepare small patches that gradually make the > nand_base.c and nand_bbt.c files less dependent. Yeah, a series of patches would be best. And maybe start smaller. If the nand_bbt stuff really slows someone down, we might want to just agree on an API similar to the above and then work on the rest of the SPI NAND details, assuming someone (e.g., me) writes the implementation (and transitions other drivers to do this). Unless someone else thinks they have an excellent handle on the above, I'll take a stab at doing this. I actually have a few patches that have hung around a while (with little incentive to finish them) that do parts of what I'm suggesting above. > For instance, you can get rid of the memory release in a first patch: > > /* Free bad block table memory */ > kfree(chip->bbt); > if (!(chip->options & NAND_OWN_BUFFERS)) > kfree(chip->buffers); > > /* Free bad block descriptor memory */ > if (chip->badblock_pattern && chip->badblock_pattern->options > & NAND_BBT_DYNAMICSTRUCT) > kfree(chip->badblock_pattern); > > by moving it to some nand_bbt_release() function. > > I might be pushing some patches to do this, as I think it can be useful > in general to clean this code. Sounds like a good idea. That probably can be handled quickly. Feel free to send patch(es). Brian ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 0/3] An alternative to SPI NAND @ 2015-01-31 7:02 ` Brian Norris 0 siblings, 0 replies; 32+ messages in thread From: Brian Norris @ 2015-01-31 7:02 UTC (permalink / raw) To: Ezequiel Garcia Cc: "Qi Wang 王起 (qiwang)", Andrew Bresticker, linux-kernel, linux-mtd, "Frank Liu 刘群 (frankliu)", "Melanie Zhang 张燕 (melaniezhang)", dwmw2, James Hartley, "Peter Pan 潘栋 (peterpandong)" On Fri, Jan 30, 2015 at 08:47:29AM -0300, Ezequiel Garcia wrote: > On 01/29/2015 09:57 PM, Peter Pan 潘栋 (peterpandong) wrote: > [..] > > > > Currently, we are working on sharing the bbt code. I think your and Brain's suggestion > > will be very helpful. > > > > There are two options. We can put struct nand_bbt pointer in either nand_chip > > or mtd_info structure. > > I'm sorry, you lost me here. Do you mean struct nand_bbt_descr ? I'm assuming he's suggesting a new struct that he's calling nand_bbt. > > If put nand_bbt in nand_chip, we need to change the > > I thought the plan was NOT to base spi-nand on nand, so you can't put > this in nand_chip, can you? You could. It's just a matter of who does the pointer chasing. (It's just important that nand_bbt.c doesn't *assume* that it has nand_chip as a parent.) If struct nand_bbt is embedded directly in struct mtd_info, then nand_bbt.c could implement functions such that its users can just do this: mtd->_block_isbad = nand_bbt_blockisbad; But if you don't (and instead embed it in a custom location like nand_chip), we'd have to write wrappers that call the nand_bbt_* functions (like nand_base does today). e.g.: mtd->_block_isbad = spi_nand_blockisbad; int spi_nand_block_isbad(struct mtd_info *mtd, loff_t offs) { struct spi_nand_chip *chip = mtd->priv; struct nand_bbt *bbt = chip->bbt; return nand_bbt_blockisbad(bbt, offs); } Anyway, I think it's worth laying out exactly where the pieces are going to fit here. I reckon we need the following: 1. a short list of parameters that nand_bbt needs from the NAND implementation (either nand_base or spi-nand-*) 2. a list of parameters that need to be kept around for nand_bbt bookkeeping / handling 3. an init function (nand_bbt_init()?) that takes #1 and computes #2 3(a) a corresponding release function 4. a set of functions that, given only a few obvious parameters (e.g., block offsets) and a struct that holds #2, handle all BBT needs (e.g., bad block lookup, bad block scanning, marking new bad blocks) Since I see #1 and #2 overlapping quite a bit, we might combine them, where several parameters are expected to be set by the nand_bbt user (either nand_base.c or nand_bbt.c), and a few are considered private to nand_bbt. struct nand_bbt { struct mtd_info *mtd; /* * This is important to abstract out of nand_bbt.c and provide * separately in nand_base.c and spi-nand-base.c -- it's sort of * duplicated in nand_block_bad() (nand_base) and * scan_block_fast() (nand_bbt) right now * * Note that this also means nand_chip.badblock_pattern should * be removed from nand_bbt.c */ int (*is_bad_bbm)(struct mtd_info *mtd, loff_t ofs); /* Only required if the driver wants to attempt to program new * bad block markers that imitate the factory-marked BBMs */ int (*mark_bad_bbm)(struct mtd_info *mtd, loff_t ofs); unsigned int bbt_options; /* Only required for NAND_BBT_PERCHIP */ int numchips; /* Discourage new customer usages here; suggest usage of the * relevant NAND_BBT_* options instead */ struct nand_bbt_descr *bbt_td; struct nand_bbt_descr *bbt_md; /* The rest are filled in by nand_bbt_init() */ int bbt_erase_shift; int page_shift; u8 *bbt; }; #3 might simply look like: int nand_bbt_init(struct nand_bbt *bbt); void nand_bbt_release(struct nand_bbt *bbt); #4 might look like the following APIs for nand_bbt.c (not too different than we have now): int nand_bbt_init(struct nand_bbt *bbt); /* init + scan? */ int nand_bbt_markbad(struct nand_bbt *bbt, loff_t offs); int nand_bbt_isreserved(struct nand_bbt *bbt, loff_t offs); int nand_bbt_isbad(struct nand_bbt *bbt, loff_t offs); (The above allows for nand_bbt to be embedded anywhere; we could modify this API to be drop-in replacements for the mtd_info callbacks if we embed struct nand_bbt in mtd_info.) > Also: After looking at the nand_bbt.c file, I'm wondering how promising > is to separate it from NAND. It seems the BBT code is quite attached to > NAND! There will be quite a bit of churn, but I don't think that it is too strongly attached. > Are you planning to do this in just one patch? Maybe it's better to > start simple and prepare small patches that gradually make the > nand_base.c and nand_bbt.c files less dependent. Yeah, a series of patches would be best. And maybe start smaller. If the nand_bbt stuff really slows someone down, we might want to just agree on an API similar to the above and then work on the rest of the SPI NAND details, assuming someone (e.g., me) writes the implementation (and transitions other drivers to do this). Unless someone else thinks they have an excellent handle on the above, I'll take a stab at doing this. I actually have a few patches that have hung around a while (with little incentive to finish them) that do parts of what I'm suggesting above. > For instance, you can get rid of the memory release in a first patch: > > /* Free bad block table memory */ > kfree(chip->bbt); > if (!(chip->options & NAND_OWN_BUFFERS)) > kfree(chip->buffers); > > /* Free bad block descriptor memory */ > if (chip->badblock_pattern && chip->badblock_pattern->options > & NAND_BBT_DYNAMICSTRUCT) > kfree(chip->badblock_pattern); > > by moving it to some nand_bbt_release() function. > > I might be pushing some patches to do this, as I think it can be useful > in general to clean this code. Sounds like a good idea. That probably can be handled quickly. Feel free to send patch(es). Brian ^ permalink raw reply [flat|nested] 32+ messages in thread
* RE: [PATCH 0/3] An alternative to SPI NAND 2015-01-31 7:02 ` Brian Norris @ 2015-02-02 1:53 ` Peter Pan 潘栋 (peterpandong) -1 siblings, 0 replies; 32+ messages in thread From: Peter Pan 潘栋 (peterpandong) @ 2015-02-02 1:53 UTC (permalink / raw) To: Brian Norris, Ezequiel Garcia Cc: Qi Wang 王起 (qiwang), dwmw2, linux-kernel, linux-mtd, Frank Liu 刘群 (frankliu), Melanie Zhang 张燕 (melaniezhang), James Hartley, Andrew Bresticker [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="utf-8", Size: 5550 bytes --] On Sat, Jan 31, 2015 at 15:02:29AM -0300, Brian Norris wrote: > > On Fri, Jan 30, 2015 at 08:47:29AM -0300, Ezequiel Garcia wrote: > > On 01/29/2015 09:57 PM, Peter Pan æ½æ (peterpandong) wrote: > > [..] > > > > > > Currently, we are working on sharing the bbt code. I think your and > Brain's suggestion > > > will be very helpful. > > > > > > There are two options. We can put struct nand_bbt pointer in either > nand_chip > > > or mtd_info structure. > > > > I'm sorry, you lost me here. Do you mean struct nand_bbt_descr ? > > I'm assuming he's suggesting a new struct that he's calling nand_bbt. > > > > If put nand_bbt in nand_chip, we need to change the > > > > I thought the plan was NOT to base spi-nand on nand, so you can't put > > this in nand_chip, can you? > > You could. It's just a matter of who does the pointer chasing. (It's > just important that nand_bbt.c doesn't *assume* that it has nand_chip > as > a parent.) > > If struct nand_bbt is embedded directly in struct mtd_info, then > nand_bbt.c could implement functions such that its users can just do > this: > > mtd->_block_isbad = nand_bbt_blockisbad; > > But if you don't (and instead embed it in a custom location like > nand_chip), we'd have to write wrappers that call the nand_bbt_* > functions (like nand_base does today). e.g.: > > mtd->_block_isbad = spi_nand_blockisbad; > > int spi_nand_block_isbad(struct mtd_info *mtd, loff_t offs) > { > struct spi_nand_chip *chip = mtd->priv; > struct nand_bbt *bbt = chip->bbt; > > return nand_bbt_blockisbad(bbt, offs); > } > > Anyway, I think it's worth laying out exactly where the pieces are > going > to fit here. I reckon we need the following: > > 1. a short list of parameters that nand_bbt needs from the NAND > implementation (either nand_base or spi-nand-*) > > 2. a list of parameters that need to be kept around for nand_bbt > bookkeeping / handling > > 3. an init function (nand_bbt_init()?) that takes #1 and computes #2 > > 3(a) a corresponding release function > > 4. a set of functions that, given only a few obvious parameters (e.g., > block offsets) and a struct that holds #2, handle all BBT needs (e.g., > bad block lookup, bad block scanning, marking new bad blocks) > > Since I see #1 and #2 overlapping quite a bit, we might combine them, > where several parameters are expected to be set by the nand_bbt user > (either nand_base.c or nand_bbt.c), and a few are considered private to > nand_bbt. > > struct nand_bbt { > struct mtd_info *mtd; > > /* > * This is important to abstract out of nand_bbt.c and provide > * separately in nand_base.c and spi-nand-base.c -- it's sort of > * duplicated in nand_block_bad() (nand_base) and > * scan_block_fast() (nand_bbt) right now > * > * Note that this also means nand_chip.badblock_pattern should > * be removed from nand_bbt.c > */ > int (*is_bad_bbm)(struct mtd_info *mtd, loff_t ofs); > > /* Only required if the driver wants to attempt to program new > * bad block markers that imitate the factory-marked BBMs */ > int (*mark_bad_bbm)(struct mtd_info *mtd, loff_t ofs); > > unsigned int bbt_options; > > /* Only required for NAND_BBT_PERCHIP */ > int numchips; > > /* Discourage new customer usages here; suggest usage of the > * relevant NAND_BBT_* options instead */ > struct nand_bbt_descr *bbt_td; > struct nand_bbt_descr *bbt_md; > > /* The rest are filled in by nand_bbt_init() */ > > int bbt_erase_shift; > int page_shift; > > u8 *bbt; > }; > > #3 might simply look like: > > int nand_bbt_init(struct nand_bbt *bbt); > void nand_bbt_release(struct nand_bbt *bbt); > > #4 might look like the following APIs for nand_bbt.c (not too different > than we have now): > > int nand_bbt_init(struct nand_bbt *bbt); /* init + scan? */ > int nand_bbt_markbad(struct nand_bbt *bbt, loff_t offs); > int nand_bbt_isreserved(struct nand_bbt *bbt, loff_t offs); > int nand_bbt_isbad(struct nand_bbt *bbt, loff_t offs); > > (The above allows for nand_bbt to be embedded anywhere; we could modify > this API to be drop-in replacements for the mtd_info callbacks if we > embed struct nand_bbt in mtd_info.) > > > Also: After looking at the nand_bbt.c file, I'm wondering how > promising > > is to separate it from NAND. It seems the BBT code is quite attached > to > > NAND! > > There will be quite a bit of churn, but I don't think that it is too > strongly attached. > > > Are you planning to do this in just one patch? Maybe it's better to > > start simple and prepare small patches that gradually make the > > nand_base.c and nand_bbt.c files less dependent. > > Yeah, a series of patches would be best. And maybe start smaller. > > If the nand_bbt stuff really slows someone down, we might want to just > agree on an API similar to the above and then work on the rest of the > SPI NAND details, assuming someone (e.g., me) writes the implementation > (and transitions other drivers to do this). > > Unless someone else thinks they have an excellent handle on the above, > I'll take a stab at doing this. I actually have a few patches that have > hung around a while (with little incentive to finish them) that do > parts > of what I'm suggesting above. > I'm glad to hear that there are already some patches about your suggestion above. I know you are quite busy. Maybe I can do some help if you need. Peter ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥ ^ permalink raw reply [flat|nested] 32+ messages in thread
* RE: [PATCH 0/3] An alternative to SPI NAND @ 2015-02-02 1:53 ` Peter Pan 潘栋 (peterpandong) 0 siblings, 0 replies; 32+ messages in thread From: Peter Pan 潘栋 (peterpandong) @ 2015-02-02 1:53 UTC (permalink / raw) To: Brian Norris, Ezequiel Garcia Cc: Qi Wang 王起 (qiwang), Andrew Bresticker, linux-kernel, linux-mtd, Frank Liu 刘群 (frankliu), Melanie Zhang 张燕 (melaniezhang), dwmw2, James Hartley On Sat, Jan 31, 2015 at 15:02:29AM -0300, Brian Norris wrote: > > On Fri, Jan 30, 2015 at 08:47:29AM -0300, Ezequiel Garcia wrote: > > On 01/29/2015 09:57 PM, Peter Pan 潘栋 (peterpandong) wrote: > > [..] > > > > > > Currently, we are working on sharing the bbt code. I think your and > Brain's suggestion > > > will be very helpful. > > > > > > There are two options. We can put struct nand_bbt pointer in either > nand_chip > > > or mtd_info structure. > > > > I'm sorry, you lost me here. Do you mean struct nand_bbt_descr ? > > I'm assuming he's suggesting a new struct that he's calling nand_bbt. > > > > If put nand_bbt in nand_chip, we need to change the > > > > I thought the plan was NOT to base spi-nand on nand, so you can't put > > this in nand_chip, can you? > > You could. It's just a matter of who does the pointer chasing. (It's > just important that nand_bbt.c doesn't *assume* that it has nand_chip > as > a parent.) > > If struct nand_bbt is embedded directly in struct mtd_info, then > nand_bbt.c could implement functions such that its users can just do > this: > > mtd->_block_isbad = nand_bbt_blockisbad; > > But if you don't (and instead embed it in a custom location like > nand_chip), we'd have to write wrappers that call the nand_bbt_* > functions (like nand_base does today). e.g.: > > mtd->_block_isbad = spi_nand_blockisbad; > > int spi_nand_block_isbad(struct mtd_info *mtd, loff_t offs) > { > struct spi_nand_chip *chip = mtd->priv; > struct nand_bbt *bbt = chip->bbt; > > return nand_bbt_blockisbad(bbt, offs); > } > > Anyway, I think it's worth laying out exactly where the pieces are > going > to fit here. I reckon we need the following: > > 1. a short list of parameters that nand_bbt needs from the NAND > implementation (either nand_base or spi-nand-*) > > 2. a list of parameters that need to be kept around for nand_bbt > bookkeeping / handling > > 3. an init function (nand_bbt_init()?) that takes #1 and computes #2 > > 3(a) a corresponding release function > > 4. a set of functions that, given only a few obvious parameters (e.g., > block offsets) and a struct that holds #2, handle all BBT needs (e.g., > bad block lookup, bad block scanning, marking new bad blocks) > > Since I see #1 and #2 overlapping quite a bit, we might combine them, > where several parameters are expected to be set by the nand_bbt user > (either nand_base.c or nand_bbt.c), and a few are considered private to > nand_bbt. > > struct nand_bbt { > struct mtd_info *mtd; > > /* > * This is important to abstract out of nand_bbt.c and provide > * separately in nand_base.c and spi-nand-base.c -- it's sort of > * duplicated in nand_block_bad() (nand_base) and > * scan_block_fast() (nand_bbt) right now > * > * Note that this also means nand_chip.badblock_pattern should > * be removed from nand_bbt.c > */ > int (*is_bad_bbm)(struct mtd_info *mtd, loff_t ofs); > > /* Only required if the driver wants to attempt to program new > * bad block markers that imitate the factory-marked BBMs */ > int (*mark_bad_bbm)(struct mtd_info *mtd, loff_t ofs); > > unsigned int bbt_options; > > /* Only required for NAND_BBT_PERCHIP */ > int numchips; > > /* Discourage new customer usages here; suggest usage of the > * relevant NAND_BBT_* options instead */ > struct nand_bbt_descr *bbt_td; > struct nand_bbt_descr *bbt_md; > > /* The rest are filled in by nand_bbt_init() */ > > int bbt_erase_shift; > int page_shift; > > u8 *bbt; > }; > > #3 might simply look like: > > int nand_bbt_init(struct nand_bbt *bbt); > void nand_bbt_release(struct nand_bbt *bbt); > > #4 might look like the following APIs for nand_bbt.c (not too different > than we have now): > > int nand_bbt_init(struct nand_bbt *bbt); /* init + scan? */ > int nand_bbt_markbad(struct nand_bbt *bbt, loff_t offs); > int nand_bbt_isreserved(struct nand_bbt *bbt, loff_t offs); > int nand_bbt_isbad(struct nand_bbt *bbt, loff_t offs); > > (The above allows for nand_bbt to be embedded anywhere; we could modify > this API to be drop-in replacements for the mtd_info callbacks if we > embed struct nand_bbt in mtd_info.) > > > Also: After looking at the nand_bbt.c file, I'm wondering how > promising > > is to separate it from NAND. It seems the BBT code is quite attached > to > > NAND! > > There will be quite a bit of churn, but I don't think that it is too > strongly attached. > > > Are you planning to do this in just one patch? Maybe it's better to > > start simple and prepare small patches that gradually make the > > nand_base.c and nand_bbt.c files less dependent. > > Yeah, a series of patches would be best. And maybe start smaller. > > If the nand_bbt stuff really slows someone down, we might want to just > agree on an API similar to the above and then work on the rest of the > SPI NAND details, assuming someone (e.g., me) writes the implementation > (and transitions other drivers to do this). > > Unless someone else thinks they have an excellent handle on the above, > I'll take a stab at doing this. I actually have a few patches that have > hung around a while (with little incentive to finish them) that do > parts > of what I'm suggesting above. > I'm glad to hear that there are already some patches about your suggestion above. I know you are quite busy. Maybe I can do some help if you need. Peter ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 0/3] An alternative to SPI NAND 2015-01-31 7:02 ` Brian Norris @ 2015-02-23 15:32 ` Ezequiel Garcia -1 siblings, 0 replies; 32+ messages in thread From: Ezequiel Garcia @ 2015-02-23 15:32 UTC (permalink / raw) To: Brian Norris Cc: "Peter Pan 潘栋 (peterpandong)", "Qi Wang 王起 (qiwang)", dwmw2, linux-kernel, linux-mtd, "Frank Liu 刘群 (frankliu)", "Melanie Zhang 张燕 (melaniezhang)", James Hartley, Andrew Bresticker Brian, On 01/31/2015 04:02 AM, Brian Norris wrote: > Unless someone else thinks they have an excellent handle on the above, > I'll take a stab at doing this. I actually have a few patches that have > hung around a while (with little incentive to finish them) that do parts > of what I'm suggesting above. Any progress with this? You outlined the plan quite clearly, so perhaps I can help you by putting together some patches? If you have some work-in-progress stuff, I can even start there. -- Ezequiel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 0/3] An alternative to SPI NAND @ 2015-02-23 15:32 ` Ezequiel Garcia 0 siblings, 0 replies; 32+ messages in thread From: Ezequiel Garcia @ 2015-02-23 15:32 UTC (permalink / raw) To: Brian Norris Cc: "Qi Wang 王起 (qiwang)", Andrew Bresticker, linux-kernel, linux-mtd, "Frank Liu 刘群 (frankliu)", "Melanie Zhang 张燕 (melaniezhang)", dwmw2, James Hartley, "Peter Pan 潘栋 (peterpandong)" Brian, On 01/31/2015 04:02 AM, Brian Norris wrote: > Unless someone else thinks they have an excellent handle on the above, > I'll take a stab at doing this. I actually have a few patches that have > hung around a while (with little incentive to finish them) that do parts > of what I'm suggesting above. Any progress with this? You outlined the plan quite clearly, so perhaps I can help you by putting together some patches? If you have some work-in-progress stuff, I can even start there. -- Ezequiel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 0/3] An alternative to SPI NAND 2015-02-23 15:32 ` Ezequiel Garcia @ 2015-02-24 3:54 ` Brian Norris -1 siblings, 0 replies; 32+ messages in thread From: Brian Norris @ 2015-02-24 3:54 UTC (permalink / raw) To: Ezequiel Garcia Cc: "Peter Pan 潘栋 (peterpandong)", "Qi Wang 王起 (qiwang)", dwmw2, linux-kernel, linux-mtd, "Frank Liu 刘群 (frankliu)", "Melanie Zhang 张燕 (melaniezhang)", James Hartley, Andrew Bresticker On Mon, Feb 23, 2015 at 12:32:42PM -0300, Ezequiel Garcia wrote: > On 01/31/2015 04:02 AM, Brian Norris wrote: > > Unless someone else thinks they have an excellent handle on the above, > > I'll take a stab at doing this. I actually have a few patches that have > > hung around a while (with little incentive to finish them) that do parts > > of what I'm suggesting above. > > Any progress with this? > > You outlined the plan quite clearly, so perhaps I can help you by > putting together some patches? If you have some work-in-progress stuff, > I can even start there. I did quite a decent chunk of work a few weeks back, but I didn't have time to finish it in my free time. The day job has distracted me from my less urgent tasks. Anyway, I had queued up stuff here: http://git.infradead.org/users/norris/linux-mtd.git/shortlog/refs/heads/nand-bbt Take a look if you'd like. I could post the first couple, I guess, but the last is definitely not complete. I can flesh out the TODO list a bit better, if you really want to pick it up. Brian ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 0/3] An alternative to SPI NAND @ 2015-02-24 3:54 ` Brian Norris 0 siblings, 0 replies; 32+ messages in thread From: Brian Norris @ 2015-02-24 3:54 UTC (permalink / raw) To: Ezequiel Garcia Cc: "Qi Wang 王起 (qiwang)", Andrew Bresticker, linux-kernel, linux-mtd, "Frank Liu 刘群 (frankliu)", "Melanie Zhang 张燕 (melaniezhang)", dwmw2, James Hartley, "Peter Pan 潘栋 (peterpandong)" On Mon, Feb 23, 2015 at 12:32:42PM -0300, Ezequiel Garcia wrote: > On 01/31/2015 04:02 AM, Brian Norris wrote: > > Unless someone else thinks they have an excellent handle on the above, > > I'll take a stab at doing this. I actually have a few patches that have > > hung around a while (with little incentive to finish them) that do parts > > of what I'm suggesting above. > > Any progress with this? > > You outlined the plan quite clearly, so perhaps I can help you by > putting together some patches? If you have some work-in-progress stuff, > I can even start there. I did quite a decent chunk of work a few weeks back, but I didn't have time to finish it in my free time. The day job has distracted me from my less urgent tasks. Anyway, I had queued up stuff here: http://git.infradead.org/users/norris/linux-mtd.git/shortlog/refs/heads/nand-bbt Take a look if you'd like. I could post the first couple, I guess, but the last is definitely not complete. I can flesh out the TODO list a bit better, if you really want to pick it up. Brian ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 0/3] An alternative to SPI NAND 2015-02-24 3:54 ` Brian Norris @ 2015-02-26 18:39 ` Ezequiel Garcia -1 siblings, 0 replies; 32+ messages in thread From: Ezequiel Garcia @ 2015-02-26 18:39 UTC (permalink / raw) To: Brian Norris Cc: "Peter Pan 潘栋 (peterpandong)", "Qi Wang 王起 (qiwang)", dwmw2, linux-kernel, linux-mtd, "Frank Liu 刘群 (frankliu)", "Melanie Zhang 张燕 (melaniezhang)", James Hartley, Andrew Bresticker On 02/24/2015 12:54 AM, Brian Norris wrote: > On Mon, Feb 23, 2015 at 12:32:42PM -0300, Ezequiel Garcia wrote: >> On 01/31/2015 04:02 AM, Brian Norris wrote: >>> Unless someone else thinks they have an excellent handle on the above, >>> I'll take a stab at doing this. I actually have a few patches that have >>> hung around a while (with little incentive to finish them) that do parts >>> of what I'm suggesting above. >> >> Any progress with this? >> >> You outlined the plan quite clearly, so perhaps I can help you by >> putting together some patches? If you have some work-in-progress stuff, >> I can even start there. > > I did quite a decent chunk of work a few weeks back, but I didn't have > time to finish it in my free time. The day job has distracted me from my > less urgent tasks. > > Anyway, I had queued up stuff here: > > http://git.infradead.org/users/norris/linux-mtd.git/shortlog/refs/heads/nand-bbt > > Take a look if you'd like. I could post the first couple, I guess, but > the last is definitely not complete. I can flesh out the TODO list a bit > better, if you really want to pick it up. > I think the first four patches are nice cleanups, so if you can submit them it would be great. -- Ezequiel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 0/3] An alternative to SPI NAND @ 2015-02-26 18:39 ` Ezequiel Garcia 0 siblings, 0 replies; 32+ messages in thread From: Ezequiel Garcia @ 2015-02-26 18:39 UTC (permalink / raw) To: Brian Norris Cc: "Qi Wang 王起 (qiwang)", Andrew Bresticker, linux-kernel, linux-mtd, "Frank Liu 刘群 (frankliu)", "Melanie Zhang 张燕 (melaniezhang)", dwmw2, James Hartley, "Peter Pan 潘栋 (peterpandong)" On 02/24/2015 12:54 AM, Brian Norris wrote: > On Mon, Feb 23, 2015 at 12:32:42PM -0300, Ezequiel Garcia wrote: >> On 01/31/2015 04:02 AM, Brian Norris wrote: >>> Unless someone else thinks they have an excellent handle on the above, >>> I'll take a stab at doing this. I actually have a few patches that have >>> hung around a while (with little incentive to finish them) that do parts >>> of what I'm suggesting above. >> >> Any progress with this? >> >> You outlined the plan quite clearly, so perhaps I can help you by >> putting together some patches? If you have some work-in-progress stuff, >> I can even start there. > > I did quite a decent chunk of work a few weeks back, but I didn't have > time to finish it in my free time. The day job has distracted me from my > less urgent tasks. > > Anyway, I had queued up stuff here: > > http://git.infradead.org/users/norris/linux-mtd.git/shortlog/refs/heads/nand-bbt > > Take a look if you'd like. I could post the first couple, I guess, but > the last is definitely not complete. I can flesh out the TODO list a bit > better, if you really want to pick it up. > I think the first four patches are nice cleanups, so if you can submit them it would be great. -- Ezequiel ^ permalink raw reply [flat|nested] 32+ messages in thread
* RE: [PATCH 0/3] An alternative to SPI NAND 2015-01-08 1:03 ` Brian Norris @ 2015-01-20 6:15 ` Peter Pan 潘栋 (peterpandong) -1 siblings, 0 replies; 32+ messages in thread From: Peter Pan 潘栋 (peterpandong) @ 2015-01-20 6:15 UTC (permalink / raw) To: Brian Norris Cc: dwmw2, Ezequiel Garcia, linux-kernel, linux-mtd, Qi Wang 王起 (qiwang), Frank Liu 刘群 (frankliu), Melanie Zhang 张燕 (melaniezhang) [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2368 bytes --] > On Thu, Jan 08, 2015 at 12:47:24AM +0000, Peter Pan æ½æ (peterpandong) > wrote: > > Documentation/devicetree/bindings/mtd/spi-nand.txt | 22 + > > drivers/mtd/Kconfig | 2 + > > drivers/mtd/Makefile | 1 + > > drivers/mtd/spi-nand/Kconfig | 7 + > > drivers/mtd/spi-nand/Makefile | 3 + > > drivers/mtd/spi-nand/spi-nand-base.c | 2034 > ++++++++++++++++++++ > > drivers/mtd/spi-nand/spi-nand-bbt.c | 1279 > ++++++++++++ > > I can already tell by the diffstat that I don't like this. We probably > don't need 3000 new lines of code for this, but we especially don't > want > to duplicate nand_bbt.c. It won't take a lot of work to augment > nand_bbt.c to make it shareable. (I can whip that patch up if needed.) Qi Wang and I have started to work on make nand_bbt.c shareable. nand_bbt.c need some member of nand_chip. Like what you said, if we want to remove nand_chip from nand_bbt.c, "We'd just need to parameterize a few relevant device details into a new nand_bbt struct, rather than using struct nand_chip directly." We can put struct nand_bbt pointer in either nand_chip or mtd_info structure. If put nand_bbt in nand_chip, we need to change the parameter of nand_chip->scan_bbt function from mtd_info to nand_bbt. Using nand_bbt struct will cause some member in both nand_chip and nand_bbt struct. Brain, do you have any suggest about this? > > I'll still take a look at the rest of the code eventually, but just > wanted to give my 2 cents up front. > > > drivers/mtd/spi-nand/spi-nand-device.c | 281 +++ > > include/linux/mtd/spi-nand.h | 317 +++ > > 9 files changed, 3946 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/mtd/spi-nand.txt > > create mode 100644 drivers/mtd/spi-nand/Kconfig > > create mode 100644 drivers/mtd/spi-nand/Makefile > > create mode 100644 drivers/mtd/spi-nand/spi-nand-base.c > > create mode 100644 drivers/mtd/spi-nand/spi-nand-bbt.c > > create mode 100644 drivers/mtd/spi-nand/spi-nand-device.c > > create mode 100644 include/linux/mtd/spi-nand.h > > Brian ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥ ^ permalink raw reply [flat|nested] 32+ messages in thread
* RE: [PATCH 0/3] An alternative to SPI NAND @ 2015-01-20 6:15 ` Peter Pan 潘栋 (peterpandong) 0 siblings, 0 replies; 32+ messages in thread From: Peter Pan 潘栋 (peterpandong) @ 2015-01-20 6:15 UTC (permalink / raw) To: Brian Norris Cc: Qi Wang 王起 (qiwang), linux-kernel, linux-mtd, Frank Liu 刘群 (frankliu), Ezequiel Garcia, Melanie Zhang 张燕 (melaniezhang), dwmw2 > On Thu, Jan 08, 2015 at 12:47:24AM +0000, Peter Pan 潘栋 (peterpandong) > wrote: > > Documentation/devicetree/bindings/mtd/spi-nand.txt | 22 + > > drivers/mtd/Kconfig | 2 + > > drivers/mtd/Makefile | 1 + > > drivers/mtd/spi-nand/Kconfig | 7 + > > drivers/mtd/spi-nand/Makefile | 3 + > > drivers/mtd/spi-nand/spi-nand-base.c | 2034 > ++++++++++++++++++++ > > drivers/mtd/spi-nand/spi-nand-bbt.c | 1279 > ++++++++++++ > > I can already tell by the diffstat that I don't like this. We probably > don't need 3000 new lines of code for this, but we especially don't > want > to duplicate nand_bbt.c. It won't take a lot of work to augment > nand_bbt.c to make it shareable. (I can whip that patch up if needed.) Qi Wang and I have started to work on make nand_bbt.c shareable. nand_bbt.c need some member of nand_chip. Like what you said, if we want to remove nand_chip from nand_bbt.c, "We'd just need to parameterize a few relevant device details into a new nand_bbt struct, rather than using struct nand_chip directly." We can put struct nand_bbt pointer in either nand_chip or mtd_info structure. If put nand_bbt in nand_chip, we need to change the parameter of nand_chip->scan_bbt function from mtd_info to nand_bbt. Using nand_bbt struct will cause some member in both nand_chip and nand_bbt struct. Brain, do you have any suggest about this? > > I'll still take a look at the rest of the code eventually, but just > wanted to give my 2 cents up front. > > > drivers/mtd/spi-nand/spi-nand-device.c | 281 +++ > > include/linux/mtd/spi-nand.h | 317 +++ > > 9 files changed, 3946 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/mtd/spi-nand.txt > > create mode 100644 drivers/mtd/spi-nand/Kconfig > > create mode 100644 drivers/mtd/spi-nand/Makefile > > create mode 100644 drivers/mtd/spi-nand/spi-nand-base.c > > create mode 100644 drivers/mtd/spi-nand/spi-nand-bbt.c > > create mode 100644 drivers/mtd/spi-nand/spi-nand-device.c > > create mode 100644 include/linux/mtd/spi-nand.h > > Brian ^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2015-02-26 18:42 UTC | newest] Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-01-08 0:47 [PATCH 0/3] An alternative to SPI NAND Peter Pan 潘栋 (peterpandong) 2015-01-08 0:47 ` Peter Pan 潘栋 (peterpandong) 2015-01-08 1:03 ` Brian Norris 2015-01-08 1:03 ` Brian Norris 2015-01-08 2:45 ` Qi Wang 王起 (qiwang) 2015-01-08 2:45 ` Qi Wang 王起 (qiwang) 2015-01-08 3:27 ` Ezequiel Garcia 2015-01-08 3:27 ` Ezequiel Garcia 2015-01-12 15:10 ` Qi Wang 王起 (qiwang) 2015-01-12 15:10 ` Qi Wang 王起 (qiwang) 2015-01-20 10:35 ` Ezequiel Garcia 2015-01-20 10:35 ` Ezequiel Garcia 2015-01-21 2:11 ` Qi Wang 王起 (qiwang) 2015-01-21 2:11 ` Qi Wang 王起 (qiwang) 2015-01-29 18:03 ` Ezequiel Garcia 2015-01-29 18:03 ` Ezequiel Garcia 2015-01-30 0:57 ` Peter Pan 潘栋 (peterpandong) 2015-01-30 0:57 ` Peter Pan 潘栋 (peterpandong) 2015-01-30 11:47 ` Ezequiel Garcia 2015-01-30 11:47 ` Ezequiel Garcia 2015-01-31 7:02 ` Brian Norris 2015-01-31 7:02 ` Brian Norris 2015-02-02 1:53 ` Peter Pan 潘栋 (peterpandong) 2015-02-02 1:53 ` Peter Pan 潘栋 (peterpandong) 2015-02-23 15:32 ` Ezequiel Garcia 2015-02-23 15:32 ` Ezequiel Garcia 2015-02-24 3:54 ` Brian Norris 2015-02-24 3:54 ` Brian Norris 2015-02-26 18:39 ` Ezequiel Garcia 2015-02-26 18:39 ` Ezequiel Garcia 2015-01-20 6:15 ` Peter Pan 潘栋 (peterpandong) 2015-01-20 6:15 ` Peter Pan 潘栋 (peterpandong)
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.