| Message ID | 1376286173-12581-2-git-send-email-b32955@freescale.com |
|---|---|
| State | New, archived |
| Headers | show |
Hi, > > The current code does not set the SLC/MLC information for onfi nand. > (This makes that the kernel treats all the onfi nand as SLC nand.) > > This patch fills the chip->cellinfo when the onfi nand is a MLC(or TLC) nand > (p->bits_per_cell > 1). > > The macro NAND_CI_CELLTYPE_SHIFT is added to avoid the hardcode. > > Signed-off-by: Huang Shijie <b32955@freescale.com> > --- > drivers/mtd/nand/nand_base.c | 3 +++ > include/linux/mtd/nand.h | 1 + > 2 files changed, 4 insertions(+), 0 deletions(-) > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > index ff605c8..ee1aa52 100644 > --- a/drivers/mtd/nand/nand_base.c > +++ b/drivers/mtd/nand/nand_base.c > @@ -2988,6 +2988,9 @@ static int nand_flash_detect_onfi(struct mtd_info > *mtd, struct nand_chip *chip, > chip->chipsize = le32_to_cpu(p->blocks_per_lun); > chip->chipsize *= (uint64_t)mtd->erasesize * p->lun_count; > > + /* @bits_per_cell equals 1 means this is a SLC nand. */ > + chip->cellinfo = (p->bits_per_cell - 1) << NAND_CI_CELLTYPE_SHIFT; > + [Pekon]: For future scalability, good to update only MLC related bit-fields So ORing instead of assigning.. chip->cellinfo |= (p->bits_per_cell - 1) << NAND_CI_CELLTYPE_SHIFT; > if (onfi_feature(chip) & ONFI_FEATURE_16_BIT_BUS) > *busw = NAND_BUSWIDTH_16; > else > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h > index ac8e89d..bf743ba 100644 > --- a/include/linux/mtd/nand.h > +++ b/include/linux/mtd/nand.h > @@ -198,6 +198,7 @@ typedef enum { > /* Cell info constants */ > #define NAND_CI_CHIPNR_MSK 0x03 > #define NAND_CI_CELLTYPE_MSK 0x0C > +#define NAND_CI_CELLTYPE_SHIFT 2 > > /* Keep gcc happy */ > struct nand_chip; > -- > 1.7.1 > With regards, pekon
于 2013年08月12日 15:22, Gupta, Pekon 写道: > [Pekon]: For future scalability, good to update only MLC related bit-fields > So ORing instead of assigning.. > chip->cellinfo |= (p->bits_per_cell - 1)<< NAND_CI_CELLTYPE_SHIFT; > thanks for review. yes, use the ORing is better. I will fix it in the next version. thanks Huang Shijie
On Mon, Aug 12, 2013 at 07:22:38AM +0000, Gupta, Pekon wrote: > > The current code does not set the SLC/MLC information for onfi nand. > > (This makes that the kernel treats all the onfi nand as SLC nand.) > > > > This patch fills the chip->cellinfo when the onfi nand is a MLC(or TLC) nand > > (p->bits_per_cell > 1). > > > > The macro NAND_CI_CELLTYPE_SHIFT is added to avoid the hardcode. > > > > Signed-off-by: Huang Shijie <b32955@freescale.com> > > --- > > drivers/mtd/nand/nand_base.c | 3 +++ > > include/linux/mtd/nand.h | 1 + > > 2 files changed, 4 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > > index ff605c8..ee1aa52 100644 > > --- a/drivers/mtd/nand/nand_base.c > > +++ b/drivers/mtd/nand/nand_base.c > > @@ -2988,6 +2988,9 @@ static int nand_flash_detect_onfi(struct mtd_info > > *mtd, struct nand_chip *chip, > > chip->chipsize = le32_to_cpu(p->blocks_per_lun); > > chip->chipsize *= (uint64_t)mtd->erasesize * p->lun_count; > > > > + /* @bits_per_cell equals 1 means this is a SLC nand. */ > > + chip->cellinfo = (p->bits_per_cell - 1) << NAND_CI_CELLTYPE_SHIFT; > > + > [Pekon]: For future scalability, good to update only MLC related bit-fields > So ORing instead of assigning.. > chip->cellinfo |= (p->bits_per_cell - 1) << NAND_CI_CELLTYPE_SHIFT; I was thinking of an alternate approach: since nand_chip.cellinfo is only used for checking SLC vs. MLC (and it is admittedly bad at that, currently), we should modify it so that is a reliable source of *only* 1 piece of information -- the number of bits per cell. Currently, it contains unused (and potentially unmaintainable) information for some chips about number of simultaneously-programmed pages, write caching, internal chip numbering, etc. So personally, I would rename cellinfo to bits_per_cell and make sure it is set properly. That is, for the legacy chips, make sure we initialize it 1 (SLC); for extended-ID chips, make sure the third ID byte has the correct info (you can refer to [1]) and set it with something like this: chip->bits_per_cell = id_data[2] & NAND_CI_CELLTYPE_MSK chip->bits_per_cell >>= NAND_CI_CELLTYPE_SHIFT; chip->bits_per_cell += 1; for chips listed by full-ID, add an appropriate flag/field; and for ONFI chips, just use p->bits_per_cell. If you really need the other cellinfo fields in the future, we can add more fields to nand_chip. > > > if (onfi_feature(chip) & ONFI_FEATURE_16_BIT_BUS) > > *busw = NAND_BUSWIDTH_16; > > else Thanks, Brian [1] An incomplete NAND ID table: http://www.linux-mtd.infradead.org/nand-data/nanddata.html
于 2013年08月13日 08:49, Brian Norris 写道: > On Mon, Aug 12, 2013 at 07:22:38AM +0000, Gupta, Pekon wrote: >>> The current code does not set the SLC/MLC information for onfi nand. >>> (This makes that the kernel treats all the onfi nand as SLC nand.) >>> >>> This patch fills the chip->cellinfo when the onfi nand is a MLC(or TLC) nand >>> (p->bits_per_cell> 1). >>> >>> The macro NAND_CI_CELLTYPE_SHIFT is added to avoid the hardcode. >>> >>> Signed-off-by: Huang Shijie<b32955@freescale.com> >>> --- >>> drivers/mtd/nand/nand_base.c | 3 +++ >>> include/linux/mtd/nand.h | 1 + >>> 2 files changed, 4 insertions(+), 0 deletions(-) >>> >>> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c >>> index ff605c8..ee1aa52 100644 >>> --- a/drivers/mtd/nand/nand_base.c >>> +++ b/drivers/mtd/nand/nand_base.c >>> @@ -2988,6 +2988,9 @@ static int nand_flash_detect_onfi(struct mtd_info >>> *mtd, struct nand_chip *chip, >>> chip->chipsize = le32_to_cpu(p->blocks_per_lun); >>> chip->chipsize *= (uint64_t)mtd->erasesize * p->lun_count; >>> >>> + /* @bits_per_cell equals 1 means this is a SLC nand. */ >>> + chip->cellinfo = (p->bits_per_cell - 1)<< NAND_CI_CELLTYPE_SHIFT; >>> + >> [Pekon]: For future scalability, good to update only MLC related bit-fields >> So ORing instead of assigning.. >> chip->cellinfo |= (p->bits_per_cell - 1)<< NAND_CI_CELLTYPE_SHIFT; > I was thinking of an alternate approach: since nand_chip.cellinfo is > only used for checking SLC vs. MLC (and it is admittedly bad at that, > currently), we should modify it so that is a reliable source of *only* 1 > piece of information -- the number of bits per cell. Currently, it > contains unused (and potentially unmaintainable) information for some > chips about number of simultaneously-programmed pages, write caching, > internal chip numbering, etc. I do not object to rename the cellinfo to bits_per_cell. :) It's okay to me. Do Artem & David (or some other people) have any opinion about this? > byte has the correct info (you can refer to [1]) and set it with > something like this: > > chip->bits_per_cell = id_data[2]& NAND_CI_CELLTYPE_MSK > chip->bits_per_cell>>= NAND_CI_CELLTYPE_SHIFT; > chip->bits_per_cell += 1; > > for chips listed by full-ID, add an appropriate flag/field; and for ONFI > chips, just use p->bits_per_cell. we do not need to worry about the full-id case, we can get the correct cell info from the id[2] for all the 4 toshiba nand. > If you really need the other cellinfo fields in the future, we can add > more fields to nand_chip. > I only need the SLC/MLC info now. thanks Huang Shijie >
On Mon, Aug 12, 2013 at 7:32 PM, Huang Shijie <b32955@freescale.com> wrote: > 于 2013年08月13日 08:49, Brian Norris 写道: >> On Mon, Aug 12, 2013 at 07:22:38AM +0000, Gupta, Pekon wrote: >> for chips listed by full-ID, add an appropriate flag/field; and for ONFI >> chips, just use p->bits_per_cell. > > we do not need to worry about the full-id case, we can get the correct cell > info from the id[2] for all the 4 toshiba nand. I see. I forgot that you assign nand_chip.cellinfo in find_full_id_nand(). It still seems like a bad design decision to base the implementation of the full-ID table on the fact that it currently only holds your 4 Toshiba entries, but I suppose it can be improved if/when additions are needed that don't support the same CELLINFO layout. Brian
(You used HTML again, so this might not have gotten through the list) On Tue, Aug 13, 2013 at 11:03:14AM +0800, Huang Shijie wrote: > 于 2013年08月13日 08:49, Brian Norris 写道: > currently), we should modify it so that is a reliable source of > *only* 1 > piece of information -- the number of bits per cell. Currently, it > do you need to rename the cellinfo to bits_per_cell, or add a new field : > bits_per_cell? Well, my whole point was that 'cellinfo' is really not very useful for us. We just mask it off all the time, and it makes life more complicated. So I'd just rename cellinfo to bits_per_cell and change its assignment/usage appropriately. But we can also wait a few days, considering that you asked for others' thoughts, although I doubt many others will speak up (I'd be glad to be proven wrong). Then you can send v2 at your leisure. Brian
于 2013年08月13日 11:17, Brian Norris 写道: > But we can also wait a few days, considering that you asked for others' > thoughts, although I doubt many others will speak up (I'd be glad to be yes. i will wait for several days. I hope others give us some other opinions. thanks Huang Shijie
> > On Tue, Aug 13, 2013 at 11:03:14AM +0800, Huang Shijie wrote: > > 于 2013年08月13日 08:49, Brian Norris 写道: > > currently), we should modify it so that is a reliable source of > > *only* 1 > > piece of information -- the number of bits per cell. Currently, it > > do you need to rename the cellinfo to bits_per_cell, or add a new field : > > bits_per_cell? > > Well, my whole point was that 'cellinfo' is really not very useful for > us. We just mask it off all the time, and it makes life more > complicated. > > So I'd just rename cellinfo to bits_per_cell and change its > assignment/usage appropriately. > [Pekon]: How about moving 'bit_per_cell' info to chip->options ? Considering 'bit_per_cell' info is similar to NAND_BUSWIDTH_16.. - It can be determined by reading ONFI parameters. OR - It can be taken from 'nand_flash_id' table. OR - And it can be provided along with board-data (which now is obsolete). with regards, pekon
于 2013年08月13日 12:10, Gupta, Pekon 写道: >> On Tue, Aug 13, 2013 at 11:03:14AM +0800, Huang Shijie wrote: >>> 于 2013年08月13日 08:49, Brian Norris 写道: >>> currently), we should modify it so that is a reliable source of >>> *only* 1 >>> piece of information -- the number of bits per cell. Currently, it >>> do you need to rename the cellinfo to bits_per_cell, or add a new field : >>> bits_per_cell? >> Well, my whole point was that 'cellinfo' is really not very useful for >> us. We just mask it off all the time, and it makes life more >> complicated. >> >> So I'd just rename cellinfo to bits_per_cell and change its >> assignment/usage appropriately. >> > [Pekon]: How about moving 'bit_per_cell' info to chip->options ? sorry, i do not think this is a good idea. the drivers may changes the chip->options. I prefer to keep it as new field. thanks Huang Shijie
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index ff605c8..ee1aa52 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -2988,6 +2988,9 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip, chip->chipsize = le32_to_cpu(p->blocks_per_lun); chip->chipsize *= (uint64_t)mtd->erasesize * p->lun_count; + /* @bits_per_cell equals 1 means this is a SLC nand. */ + chip->cellinfo = (p->bits_per_cell - 1) << NAND_CI_CELLTYPE_SHIFT; + if (onfi_feature(chip) & ONFI_FEATURE_16_BIT_BUS) *busw = NAND_BUSWIDTH_16; else diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h index ac8e89d..bf743ba 100644 --- a/include/linux/mtd/nand.h +++ b/include/linux/mtd/nand.h @@ -198,6 +198,7 @@ typedef enum { /* Cell info constants */ #define NAND_CI_CHIPNR_MSK 0x03 #define NAND_CI_CELLTYPE_MSK 0x0C +#define NAND_CI_CELLTYPE_SHIFT 2 /* Keep gcc happy */ struct nand_chip;
The current code does not set the SLC/MLC information for onfi nand. (This makes that the kernel treats all the onfi nand as SLC nand.) This patch fills the chip->cellinfo when the onfi nand is a MLC(or TLC) nand (p->bits_per_cell > 1). The macro NAND_CI_CELLTYPE_SHIFT is added to avoid the hardcode. Signed-off-by: Huang Shijie <b32955@freescale.com> --- drivers/mtd/nand/nand_base.c | 3 +++ include/linux/mtd/nand.h | 1 + 2 files changed, 4 insertions(+), 0 deletions(-)