[PATCH 0/3] An alternative to SPI NAND

[PATCH 0/3] An alternative to SPI NAND

[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¥

[PATCH 0/3] An alternative to SPI NAND

[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

Re: [PATCH 0/3] An alternative to SPI NAND

[Brian Norris]

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

Re: [PATCH 0/3] An alternative to SPI NAND

[Brian Norris]

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

RE: [PATCH 0/3] An alternative to SPI NAND

[Qi Wang 王起 (qiwang)]

[-- 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¥

RE: [PATCH 0/3] An alternative to SPI NAND

[Qi Wang 王起 (qiwang)]

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

Re: [PATCH 0/3] An alternative to SPI NAND

[Ezequiel Garcia]

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

Re: [PATCH 0/3] An alternative to SPI NAND

[Ezequiel Garcia]

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

[PATCH 0/3] An alternative to SPI NAND

[Qi Wang 王起 (qiwang)]

[-- 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¥

[PATCH 0/3] An alternative to SPI NAND

[Qi Wang 王起 (qiwang)]

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

RE: [PATCH 0/3] An alternative to SPI NAND

[Peter Pan 潘栋 (peterpandong)]

[-- 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¥

RE: [PATCH 0/3] An alternative to SPI NAND

[Peter Pan 潘栋 (peterpandong)]

> 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

Re: [PATCH 0/3] An alternative to SPI NAND

[Ezequiel Garcia]

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

Re: [PATCH 0/3] An alternative to SPI NAND

[Ezequiel Garcia]

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

RE: [PATCH 0/3] An alternative to SPI NAND

[Qi Wang 王起 (qiwang)]

[-- 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¥

RE: [PATCH 0/3] An alternative to SPI NAND

[Qi Wang 王起 (qiwang)]

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

Re: [PATCH 0/3] An alternative to SPI NAND

[Ezequiel Garcia]

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

Re: [PATCH 0/3] An alternative to SPI NAND

[Ezequiel Garcia]

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

RE: [PATCH 0/3] An alternative to SPI NAND

[Peter Pan 潘栋 (peterpandong)]

[-- 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¥

RE: [PATCH 0/3] An alternative to SPI NAND

[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

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.

Re: [PATCH 0/3] An alternative to SPI NAND

[Ezequiel Garcia]

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

Re: [PATCH 0/3] An alternative to SPI NAND

[Ezequiel Garcia]

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

Re: [PATCH 0/3] An alternative to SPI NAND

[Brian Norris]

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

Re: [PATCH 0/3] An alternative to SPI NAND

[Brian Norris]

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

RE: [PATCH 0/3] An alternative to SPI NAND

[Peter Pan 潘栋 (peterpandong)]

[-- 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¥

RE: [PATCH 0/3] An alternative to SPI NAND

[Peter Pan 潘栋 (peterpandong)]

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

Re: [PATCH 0/3] An alternative to SPI NAND

[Ezequiel Garcia]

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

Re: [PATCH 0/3] An alternative to SPI NAND

[Ezequiel Garcia]

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

Re: [PATCH 0/3] An alternative to SPI NAND

[Brian Norris]

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

Re: [PATCH 0/3] An alternative to SPI NAND

[Brian Norris]

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

Re: [PATCH 0/3] An alternative to SPI NAND

[Ezequiel Garcia]

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

Re: [PATCH 0/3] An alternative to SPI NAND

[Ezequiel Garcia]

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