| Message ID | 1334026606-18327-1-git-send-email-b32955@freescale.com |
|---|---|
| State | New, archived |
| Headers | show |
Hi Huang, On Mon, Apr 9, 2012 at 7:56 PM, Huang Shijie <b32955@freescale.com> wrote: > drivers/mtd/nand/nand_base.c | 43 ++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 43 insertions(+), 0 deletions(-) > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > index 6315b94..8997023 100644 > --- a/drivers/mtd/nand/nand_base.c > +++ b/drivers/mtd/nand/nand_base.c > @@ -3028,6 +3028,49 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd, > mtd->erasesize = (128 * 1024) << > (((extid >> 1) & 0x04) | (extid & 0x03)); > busw = 0; > + } else if (id_data[0] == NAND_MFR_HYNIX && id_data[1] == 0xd7) { > + /* Calc pagesize */ I'm not sure this is the beset heuristic. Hynix could easily make a larger chip that has the same decoding table, so we probably shouldn't compare dev_id == 0xD7. It actually seems like the decoding table might be intended for all Hynix MLC. What do you think about the following condition? + } else if ((chip->cellinfo & NAND_CI_CELLTYPE_MSK) && id_data[0] == NAND_MFR_HYNIX) { > + /* Calc oobsize */ > + switch (extid & 0x03) { The table actually includes a third column (bit 6) that corresponds with the OOB field that, in future revs could have an expanded OOB size. Personally, I wrote this condition as: + switch (((extid >> 2) & 0x04) | (extid & 0x03)) { That way, we can catch the "default" case as an unknown OOB size. > + default: > + pr_info("Cannot parse out the oobsize.\n"); > + break; > + } I'm not sure if we should just print a message for the default case; the warning message could easily be missed, and the uninitialized oobsize would cause problems. Either we should throw an error somehow or just default to the highest known OOB for this chip type (448 bytes). Personally, I just chose the latter... BTW, I actually have some patches queued up that I need to complete properly, where I'm trying to clean up and update NAND detection a little. So I basically have this same patch, but rebased on top of some other changes I have ready... I don't mind taking a revised version of yours, or I can try to clean up my series and send it out soon. Either way, please remember to copy me on these type of NAND detection patches! I've been studying this for a while and have a lot of stuff queued up to work on sometime, so it's nice to know when others are touching these areas. Thanks, Brian
Hi Brian: > Hi Huang, > > On Mon, Apr 9, 2012 at 7:56 PM, Huang Shijie<b32955@freescale.com> wrote: >> drivers/mtd/nand/nand_base.c | 43 ++++++++++++++++++++++++++++++++++++++++++ >> 1 files changed, 43 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c >> index 6315b94..8997023 100644 >> --- a/drivers/mtd/nand/nand_base.c >> +++ b/drivers/mtd/nand/nand_base.c >> @@ -3028,6 +3028,49 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd, >> mtd->erasesize = (128 * 1024)<< >> (((extid>> 1)& 0x04) | (extid& 0x03)); >> busw = 0; >> + } else if (id_data[0] == NAND_MFR_HYNIX&& id_data[1] == 0xd7) { >> + /* Calc pagesize */ > I'm not sure this is the beset heuristic. Hynix could easily make a > larger chip that has the same decoding table, so we probably shouldn't > compare dev_id == 0xD7. It actually seems like the decoding table > might be intended for all Hynix MLC. What do you think about the > following condition? > > + } else if ((chip->cellinfo& NAND_CI_CELLTYPE_MSK)&& > id_data[0] == NAND_MFR_HYNIX) { I am afraid this can not solve the problem. The HY27UT088G2M whose id_data[2] is 0x14 DOES not follow your code. Please check the datasheet of HY27UT088G2M. ( I ever emailed to you.) The same issue exits in H627UW08CGFM too. >> + /* Calc oobsize */ >> + switch (extid& 0x03) { > The table actually includes a third column (bit 6) that corresponds > with the OOB field that, in future revs could have an expanded OOB > size. Personally, I wrote this condition as: > > + switch (((extid>> 2)& 0x04) | (extid& 0x03)) { yes, we can add this. > That way, we can catch the "default" case as an unknown OOB size. > >> + default: >> + pr_info("Cannot parse out the oobsize.\n"); >> + break; >> + } > I'm not sure if we should just print a message for the default case; > the warning message could easily be missed, and the uninitialized > oobsize would cause problems. Either we should throw an error somehow > or just default to the highest known OOB for this chip type (448 > bytes). Personally, I just chose the latter... ok. I can use the 448 as the default. > BTW, I actually have some patches queued up that I need to complete > properly, where I'm trying to clean up and update NAND detection a > little. So I basically have this same patch, but rebased on top of > some other changes I have ready... I don't mind taking a revised > version of yours, or I can try to clean up my series and send it out > soon. > I hope you send it out as soon as possible. :) > Either way, please remember to copy me on these type of NAND detection > patches! I've been studying this for a while and have a lot of stuff > queued up to work on sometime, so it's nice to know when others are > touching these areas. ok, no problem. thanks Huang Shijie
On Wed, Apr 11, 2012 at 7:43 PM, Huang Shijie <b32955@freescale.com> wrote: >> On Mon, Apr 9, 2012 at 7:56 PM, Huang Shijie<b32955@freescale.com> wrote: >>> >>> drivers/mtd/nand/nand_base.c | 43 >>> ++++++++++++++++++++++++++++++++++++++++++ >>> 1 files changed, 43 insertions(+), 0 deletions(-) >>> >>> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c >>> index 6315b94..8997023 100644 >>> --- a/drivers/mtd/nand/nand_base.c >>> +++ b/drivers/mtd/nand/nand_base.c >>> @@ -3028,6 +3028,49 @@ static struct nand_flash_dev >>> *nand_get_flash_type(struct mtd_info *mtd, >>> mtd->erasesize = (128 * 1024)<< >>> (((extid>> 1)& 0x04) | (extid& 0x03)); >>> busw = 0; >>> + } else if (id_data[0] == NAND_MFR_HYNIX&& id_data[1] == >>> 0xd7) { >>> >>> + /* Calc pagesize */ >> >> I'm not sure this is the beset heuristic. Hynix could easily make a >> larger chip that has the same decoding table, so we probably shouldn't >> compare dev_id == 0xD7. It actually seems like the decoding table >> might be intended for all Hynix MLC. What do you think about the >> following condition? >> >> + } else if ((chip->cellinfo& NAND_CI_CELLTYPE_MSK)&& >> id_data[0] == NAND_MFR_HYNIX) { > > I am afraid this can not solve the problem. > The HY27UT088G2M whose id_data[2] is 0x14 DOES not follow your code. > Please check the datasheet of HY27UT088G2M. ( I ever emailed to you.) > > The same issue exits in H627UW08CGFM too. Ah yes, I overlooked this. My proposal was a bit off-the-cuff. Thanks for the data sheets, BTW. I need to integrate them into my master table (and submit them to mtd-www) so that I check them whenever I'm looking at new Hynix support. Now that I've given them all a second read, it really looks like this new Hynix chip actually follows no discernible pattern, which seems to suggest that we have to know all the chip parameters in order to know which ID decode table to use...but if we know all the parameters, then why do we need the table?? (Just being devil's advocate) If I had samples of all the chips, perhaps I could test some to determine other patterns, like ID wraparound behavior, but I doubt it... So really, I'd be interested in asking Hynix (and Toshiba and probably Samsung, for that matter): what do you expect software developers to do when you provide datasheets with useless ID decode tables? Are we supposed to read your mind to determine which chips are supposed to follow a given decode table? But seriously: let me know if you have better insight into how to associate this table with a set of chip IDs. Otherwise, I might as well finish adding an 8-byte ID table and adding this one chip manually. >> BTW, I actually have some patches queued up that I need to complete >> properly, where I'm trying to clean up and update NAND detection a >> little. So I basically have this same patch, but rebased on top of >> some other changes I have ready... I don't mind taking a revised >> version of yours, or I can try to clean up my series and send it out >> soon. >> > I hope you send it out as soon as possible. :) I'm trying, but I get very busy with other things :) Anyway, I'm not very comfortable with the heuristics for this Hynix decode method, so I don't want to post another patch for it yet. If it helps, I can submit the other patches soon; they amount to mostly cleanup (IMO). But at least that way, whoever works on NAND detection in the near future has the same code base. Brian
On Wed, Apr 11, 2012 at 7:43 PM, Huang Shijie <b32955@freescale.com> wrote: >> On Mon, Apr 9, 2012 at 7:56 PM, Huang Shijie<b32955@freescale.com> wrote: >>> >>> drivers/mtd/nand/nand_base.c | 43 >>> ++++++++++++++++++++++++++++++++++++++++++ >>> 1 files changed, 43 insertions(+), 0 deletions(-) >>> >>> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c >>> index 6315b94..8997023 100644 >>> --- a/drivers/mtd/nand/nand_base.c >>> +++ b/drivers/mtd/nand/nand_base.c >>> @@ -3028,6 +3028,49 @@ static struct nand_flash_dev >>> *nand_get_flash_type(struct mtd_info *mtd, >>> mtd->erasesize = (128 * 1024)<< >>> (((extid>> 1)& 0x04) | (extid& 0x03)); >>> busw = 0; >>> + } else if (id_data[0] == NAND_MFR_HYNIX&& id_data[1] == >>> 0xd7) { >>> >>> + /* Calc pagesize */ >> >> I'm not sure this is the beset heuristic. Hynix could easily make a >> larger chip that has the same decoding table, so we probably shouldn't >> compare dev_id == 0xD7. It actually seems like the decoding table >> might be intended for all Hynix MLC. What do you think about the >> following condition? >> >> + } else if ((chip->cellinfo& NAND_CI_CELLTYPE_MSK)&& >> id_data[0] == NAND_MFR_HYNIX) { > > I am afraid this can not solve the problem. > The HY27UT088G2M whose id_data[2] is 0x14 DOES not follow your code. > Please check the datasheet of HY27UT088G2M. ( I ever emailed to you.) > > The same issue exits in H627UW08CGFM too. Ah yes, I overlooked this. My proposal was a bit off-the-cuff. Thanks for the data sheets, BTW. I need to integrate them into my master table (and submit them to mtd-www) so that I check them whenever I'm looking at new Hynix support. Now that I've given them all a second read, it really looks like this new Hynix chip actually follows no discernible pattern, which seems to suggest that we have to know all the chip parameters in order to know which ID decode table to use...but if we know all the parameters, then why do we need the table?? (Just being devil's advocate) If I had samples of all the chips, perhaps I could test some to determine other patterns, like ID wraparound behavior, but I doubt it... So really, I'd be interested in asking Hynix (and Toshiba and probably Samsung, for that matter): what do you expect software developers to do when you provide datasheets with useless ID decode tables? Are we supposed to read your mind to determine which chips are supposed to follow a given decode table? But seriously: let me know if you have better insight into how to associate this table with a set of chip IDs. Otherwise, I might as well finish adding an 8-byte ID table and adding this one chip manually. >> BTW, I actually have some patches queued up that I need to complete >> properly, where I'm trying to clean up and update NAND detection a >> little. So I basically have this same patch, but rebased on top of >> some other changes I have ready... I don't mind taking a revised >> version of yours, or I can try to clean up my series and send it out >> soon. >> > I hope you send it out as soon as possible. :) I'm trying, but I get very busy with other things :) Anyway, I'm not very comfortable with the heuristics for this Hynix decode method, so I don't want to post another patch for it yet. If it helps, I can submit the other patches soon; they amount to mostly cleanup (IMO). But at least that way, whoever works on NAND detection in the near future has the same code base. Brian
于 2012年04月18日 13:00, Brian Norris 写道: > Now that I've given them all a second read, it really looks like this > new Hynix chip actually follows no discernible pattern, which seems to > suggest that we have to know all the chip parameters in order to know > which ID decode table to use...but if we know all the parameters, then > why do we need the table?? (Just being devil's advocate) If I had This makes me confused too. It looks like hardcode when i use the 0xd7 to disdinguish this nand chip. > samples of all the chips, perhaps I could test some to determine other > patterns, like ID wraparound behavior, but I doubt it... > > So really, I'd be interested in asking Hynix (and Toshiba and probably > Samsung, for that matter): what do you expect software developers to > do when you provide datasheets with useless ID decode tables? Are we > supposed to read your mind to determine which chips are supposed to > follow a given decode table? > > But seriously: let me know if you have better insight into how to > associate this table with a set of chip IDs. Otherwise, I might as I only have the H27UBG8T2A nand chip in my hand, and test it in the imx28 board. I do not have other nand chips which may also follow the same parsing code as the H27UBG8T2A. Best Regards Huang Shijie > well finish adding an 8-byte ID table and adding this one chip > manually.
Hi Brian: > BTW, I actually have some patches queued up that I need to complete > properly, where I'm trying to clean up and update NAND detection a > little. So I basically have this same patch, but rebased on top of Did you finish your nand detection patches? I really hope this Hynix's nand chip could be supported by the mtd code. If your plan is blocked, could i send out a new version of this patch? Best Regards Huang Shijie
On Fri, Aug 17, 2012 at 1:43 AM, Huang Shijie <b32955@freescale.com> wrote: >> BTW, I actually have some patches queued up that I need to complete >> properly, where I'm trying to clean up and update NAND detection a >> little. So I basically have this same patch, but rebased on top of > > Did you finish your nand detection patches? > I really hope this Hynix's nand chip could be supported by the mtd code. > If your plan is blocked, could i send out a new version of this patch? Well, I have some working patches, but they aren't entirely ready for submission. Other things have taken priority. Sorry about the delay, since you were probably waiting... Yes, please go ahead if you have a good submission; but I thought we agreed there was no good way to detect these chips without practically hard-coding the ID + parameter configuration. If that's the case, then maybe I should speed up my patches (sitting around here as well...) regarding a full 8-byte ID detection table. I think this kind of table will be necessary to support some chips anyway. Brian
于 2012年08月18日 10:51, Brian Norris 写道: > agreed there was no good way to detect these chips without practically > hard-coding the ID + parameter configuration. If that's the case, then Yes, we have to hard-coding the ID for Hynix. But there is already a hard-coding for SAMSUNG. so I think the hard-coding is accecpted, aren't we? thanks Huang Shijie
On Sun, Aug 19, 2012 at 11:01 PM, Huang Shijie <b32955@freescale.com> wrote: > 于 2012年08月18日 10:51, Brian Norris 写道: > >> agreed there was no good way to detect these chips without practically >> hard-coding the ID + parameter configuration. If that's the case, then > > Yes, we have to hard-coding the ID for Hynix. But there is already a > hard-coding for SAMSUNG. It's not exactly hard-coding, as there is a recognizable pattern that covers a class of different chips: all Samsung MLC with 6-byte ID strings. > so I think the hard-coding is accecpted, aren't we? We try to do as little hard-coding as possible, as it can blow up as a larger maintainability problem. But as we see, we may need real hard-coding. Brian
Hi Huang, Sorry (again) that this is sitting for a while. I did some more work this evening on it, and I think I could have a solution. But I still don't have any real samples, so I'd like you help. On Wed, Apr 18, 2012 at 12:52 AM, Huang Shijie <b32955@freescale.com> wrote: > 于 2012年04月18日 13:00, Brian Norris 写道: > I only have the H27UBG8T2A nand chip in my hand, and test it in the imx28 > board. > I do not have other nand chips which may also follow the same parsing code > as the H27UBG8T2A. Can you capture a full ID string for any Hynix parts you have? Especially of interest are this H27UBG8T2A and other MLC. I want to check the ID length and wraparound behavior, since I think that we can write a rule such that we use this data table for all Hynix MLC with 6-byte ID strings. I have info for 3 type of Hynix MLC that follow the 6-byte pattern, and a few 5-byte Hynix MLC that follow existing patterns. I appreciate the help and the patience. Thanks, Brian
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index 6315b94..8997023 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -3028,6 +3028,49 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd, mtd->erasesize = (128 * 1024) << (((extid >> 1) & 0x04) | (extid & 0x03)); busw = 0; + } else if (id_data[0] == NAND_MFR_HYNIX && id_data[1] == 0xd7) { + /* Calc pagesize */ + mtd->writesize = 2048 << (extid & 0x03); + extid >>= 2; + + /* Calc oobsize */ + switch (extid & 0x03) { + case 0: + mtd->oobsize = 128; + break; + case 1: + mtd->oobsize = 224; + break; + case 2: + mtd->oobsize = 448; + break; + default: + pr_info("Cannot parse out the oobsize.\n"); + break; + } + extid >>= 2; + + /* Calc blocksize */ + extid = (((extid >> 1) & 0x04) | (extid & 0x03)); + + switch (extid) { + case 0: + case 1: + case 2: + mtd->erasesize = (128 * 1024) << extid; + break; + case 3: + mtd->erasesize = (128 * 1024) * 6; + break; + case 4: + case 5: + mtd->erasesize = (128 * 1024) << (extid - 1); + break; + default: + pr_info("Cannot parse out the blocksize.\n"); + break; + } + busw = 0; } else { /* Calc pagesize */ mtd->writesize = 1024 << (extid & 0x03);