Patchwork [01/10] mtd: set the cell information for ONFI nand

login
register
mail settings
Submitter Huang Shijie
Date Aug. 12, 2013, 5:42 a.m.
Message ID <1376286173-12581-2-git-send-email-b32955@freescale.com>
Download mbox | patch
Permalink /patch/266424/
State New
Headers show

Comments

Huang Shijie - Aug. 12, 2013, 5:42 a.m.
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(-)
pekon gupta - Aug. 12, 2013, 7:22 a.m.
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
Huang Shijie - Aug. 12, 2013, 7:27 a.m.
于 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
Brian Norris - Aug. 13, 2013, 12:49 a.m.
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
Huang Shijie - Aug. 13, 2013, 2:32 a.m.
于 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
>
Brian Norris - Aug. 13, 2013, 2:59 a.m.
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
Brian Norris - Aug. 13, 2013, 3:17 a.m.
(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
Huang Shijie - Aug. 13, 2013, 3:21 a.m.
于 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
pekon gupta - Aug. 13, 2013, 4:10 a.m.
> 

> 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
Huang Shijie - Aug. 13, 2013, 6:19 a.m.
于 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

Patch

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;