Hi Boris,
Firstly, thanks a lot for taking time to review my patches.
On Fri, Mar 25, 2016 at 4:35 PM, Boris Brezillon
<boris.brezil...@free-electrons.com> wrote:
> Hi Peter,
>
> On Mon, 14 Mar 2016 02:47:55 +0000
> Peter Pan <peterpans...@gmail.com> wrote:
>
>> From: Brian Norris <computersforpe...@gmail.com>
>>
>> Currently nand_bbt.c is tied with struct nand_chip, and it makes other
>> NAND family chips hard to use nand_bbt.c. Maybe it's the reason why
>> onenand has own bbt(onenand_bbt.c).
>>
>> Separate struct nand_chip from BBT code can make current BBT shareable.
>> We create struct nand_bbt to take place of nand_chip in nand_bbt.c
>>
>> Below is mtd folder structure we want:
>> drivers/mtd/nand/<all-nand-core-code>
>> drivers/mtd/nand/raw/<raw-nand-controller-drivers>
>> drivers/mtd/nand/spi/<spi-nand-code>
>> drivers/mtd/nand/onenand/<onenand-code>
>> drivers/mtd/nand/chips/<manufacturer-spcific-code>
>>
>> Of course, nand_bbt.c should be part of <all-nand-core-code>.
>>
>> We put every chip layout related information BBT needed into struct
>> nand_chip_layout_info.
>> @numchips: number of physical chips, required for NAND_BBT_PERCHIP
>> @chipsize: the size of one chip for multichip arrays
>> @chip_shift: number of address bits in one chip
>> @bbt_erase_shift: number of address bits in a bbt entry
>> @page_shift: number of address bits in a page
>>
>> We defined a struct nand_bbt_ops for BBT ops. Struct
>> @is_bad_bbm: check if a block is factory bad block
>> @erase: erase block bypassing resvered checks
>>
>> Struct nand_bbt includes all BBT information:
>> @mtd: pointer to MTD device structure
>> @bbt_options: bad block specific options. All options used
>> here must come from nand_bbt.h.
>> @bbt_ops: struct nand_bbt_ops pointer.
>> @info: struct nand_chip_layout_info pointer.
>> @bbt_td: bad block table descriptor for flash lookup.
>> @bbt_md: bad block table mirror descriptor
>> @bbt: bad block table pointer
>>
>> Signed-off-by: Brian Norris <computersforpe...@gmail.com>
>> [Peter: 1. correct comment style
>> 2. introduce struct nand_bbt_ops and nand_chip_layout_info]
>> Signed-off-by: Peter Pan <peterpand...@micron.com>
>> ---
>> include/linux/mtd/nand_bbt.h | 67
>> ++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 67 insertions(+)
>>
>> diff --git a/include/linux/mtd/nand_bbt.h b/include/linux/mtd/nand_bbt.h
>> index 5a65230..cfb22c8 100644
>> --- a/include/linux/mtd/nand_bbt.h
>> +++ b/include/linux/mtd/nand_bbt.h
>> @@ -18,6 +18,8 @@
>> #ifndef __LINUX_MTD_NAND_BBT_H
>> #define __LINUX_MTD_NAND_BBT_H
>>
>> +struct mtd_info;
>> +
>> /* The maximum number of NAND chips in an array */
>> #define NAND_MAX_CHIPS 8
>>
>> @@ -115,4 +117,69 @@ struct nand_bbt_descr {
>> /* The maximum number of blocks to scan for a bbt */
>> #define NAND_BBT_SCAN_MAXBLOCKS 4
>>
>> +struct nand_bbt;
>> +
>> +/**
>> + * struct nand_bbt_ops - bad block table operations
>> + * @is_bad_bbm: check if a block is factory bad block
>> + * @erase: erase block bypassing resvered checks
>> + */
>> +struct nand_bbt_ops {
>> + /*
>> + * 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);
>> +
>> + /* Erase a block, bypassing reserved checks */
>> + int (*erase)(struct mtd_info *mtd, loff_t ofs);
>> +};
>> +
>> +/**
>> + * struct nand_chip_layout_info - strucure contains all chip layout
>> + * information that BBT needed.
>> + * @numchips: number of physical chips, required for NAND_BBT_PERCHIP
>> + * @chipsize: the size of one chip for multichip arrays
>> + * @chip_shift: number of address bits in one chip
>> + * @bbt_erase_shift: number of address bits in a bbt entry
>> + * @page_shift: number of address bits in a page
>> + */
>> +struct nand_chip_layout_info {
>
> I know I'm the one who suggested this name, but NAND datasheet seems to
> call it "memory organization", so maybe we should rename this struct
> nand_memory_organization.
Fix this in v4
>
>> + int numchips;
>
> I would rename it numdies, or ndies. numchips implies you're having
> several chips, which is not the case.
Fix this in v4
>
>> + u64 chipsize;
>
> Ditto, s/chipsize/diesize/
Fix this in v4
>
>> + int chip_shift;
>
> Ditto.
Fix this in v4
>
>> + int bbt_erase_shift;
>
> Hm, this is not related to the memory organization. I'd prefer moving
> this one directly in
Yes, I also realize bbt_erase_shift is not proper. How about just rename it
to erase_block_shift or block_shift ?
>
>> + int page_shift;
>> +};
>
> The structure should probably contain other info like (oob size, pages
> per block, blocks per die, ...)
> I know some of those information are redundant with mtd_info content,
> but it would be clearer to have everything in a common place.
>
> Also, I'd recommend using helpers to access memory organization info.
> For example nand_get_die_size(mtd), nand_get_page_size(mtd), ...
>
> On a more general note, as already said, I'd like to see more
> generalization across NAND based devices, no matter the interface
> they're using.
> Doing that implies forcing all NAND based devices to inherit from a
> common class. Something like
>
> struct nand_device {
> struct mtd_info mtd;
> struct nand_memory_organization memorg;
> /* ... */
> };
>
> /* rawnand_device <-> nand_chip */
> struct rawnand_device {
> struct nand_device base;
> /* raw NAND specific fields */
> }
>
> struct spinand_device {
> struct nand_device base;
> /* SPI NAND specific fields */
> };
>
> struct onenand_device {
> struct nand_device base;
> /* OneNAND specific fields */
> };
>
> With this design, nand_bbt and nand_bbt_ops could use the generic
> nand_device instead of directly using the mtd instance.
>
> Anyway, that's just a long term goal, and I wanted to share my
> ideas. I guess your plan is to add support for SPI nand devices, so
> keep this in mind ;-).
Acctually your idea is quite good. Actually, struct nand_chip_layout_info
shouldn't be in nand_bbt.h. It should be in nand.h or nand_base.h and embedded
in struct nand_chip (or struct nand_deivce as your said).
The reason I did't do this is I feel it will be too involved. I need
to change almost
all files under mtd/nand/, which generates a larger patch set.
So I put struct nand_chip_layout_info here temporarily. Just as you
said, it's a long
term goal. Sorry for no comments to explain this in patches.
Thanks,
Peter Pan
