| Message ID | 20100819171558.GA8536@code-monkey.de |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Aug 19, 2010 at 10:16 AM, Tilman Sauerbeck <tilman@code-monkey.de> wrote: >> This patch uses the new 6-byte scheme if the following conditions are >> all true: >> [...] >> 3) 6th byte is zero > > ... but what the code does is use new scheme if id_data[5] is _not_ > zero. ID for Samsung K9GAG08U0D (6-byte sequence): ec d5 94 29 34 41 The code is right; the comment is wrong.
On 08/19/2010 12:46 PM, Kevin Cernekee wrote: > The code is right; the comment is wrong. In that case, should we fix the comment and add the check that Tilman mentioned previously? (below) This looks safe and will at least eliminate a few weird cases where Samsung SLC happen to have 6-byte ID strings (such as the K9F4G08U0B in question). I have another fix I will tack on in series to this; should I include you as a "Signed-off-by", Tilman? Brian > @@ -2852,6 +2852,7 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd, > */ > if (id_data[0] == id_data[6] && id_data[1] == id_data[7] && > id_data[0] == NAND_MFR_SAMSUNG && > + (chip->cellinfo & NAND_CI_CELLTYPE_MSK) && > id_data[5] != 0x00) { > /* Calc pagesize */ > mtd->writesize = 2048 << (extid & 0x03);
On Thu, Aug 19, 2010 at 3:28 PM, Brian Norris <norris@broadcom.com> wrote: > On 08/19/2010 12:46 PM, Kevin Cernekee wrote: >> The code is right; the comment is wrong. > > In that case, should we fix the comment and add the > check that Tilman mentioned previously? (below) To clarify - the comment in the code is correct, but the checkin comment was inaccurate. >> @@ -2852,6 +2852,7 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd, >> */ >> if (id_data[0] == id_data[6] && id_data[1] == id_data[7] && >> id_data[0] == NAND_MFR_SAMSUNG && >> + (chip->cellinfo & NAND_CI_CELLTYPE_MSK) && >> id_data[5] != 0x00) { >> /* Calc pagesize */ >> mtd->writesize = 2048 << (extid & 0x03); This looks OK (at least for K9GAG08U0D). I wonder if Samsung could provide some firm guidelines for when to use the old style vs. the new style.
> -----Original Message----- > From: linux-mtd-bounces@lists.infradead.org [mailto:linux-mtd- > bounces@lists.infradead.org] On Behalf Of Kevin Cernekee > Sent: Friday, August 20, 2010 11:30 AM > To: Brian Norris > Cc: linux-mtd@lists.infradead.org; Tilman Sauerbeck > Subject: Re: Bad assumption about ID field definition for Samsung NAND? > > On Thu, Aug 19, 2010 at 3:28 PM, Brian Norris <norris@broadcom.com> wrote: > > On 08/19/2010 12:46 PM, Kevin Cernekee wrote: > >> The code is right; the comment is wrong. > > > > In that case, should we fix the comment and add the check that Tilman > > mentioned previously? (below) > > To clarify - the comment in the code is correct, but the checkin comment > was inaccurate. > > >> @@ -2852,6 +2852,7 @@ static struct nand_flash_dev > >> *nand_get_flash_type(struct mtd_info *mtd, > >> */ > >> if (id_data[0] == id_data[6] && id_data[1] == > >> id_data[7] && > >> id_data[0] == NAND_MFR_SAMSUNG && > >> + (chip->cellinfo & > >> + NAND_CI_CELLTYPE_MSK) && > >> id_data[5] != 0x00) { > >> /* Calc pagesize */ > >> mtd->writesize = 2048 << (extid & 0x03); > > This looks OK (at least for K9GAG08U0D). The temp fix is not the good solution. We can make a fix for Samsung here, what about other vendors? If we put such kind of fix into nand_base.c file, which will make the code ugly. > > I wonder if Samsung could provide some firm guidelines for when to use > the old style vs. the new style. > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/
Kevin Cernekee [2010-08-19 20:29]: > On Thu, Aug 19, 2010 at 3:28 PM, Brian Norris <norris@broadcom.com> wrote: > > On 08/19/2010 12:46 PM, Kevin Cernekee wrote: > >> @@ -2852,6 +2852,7 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd, > >> */ > >> if (id_data[0] == id_data[6] && id_data[1] == id_data[7] && > >> id_data[0] == NAND_MFR_SAMSUNG && > >> + (chip->cellinfo & NAND_CI_CELLTYPE_MSK) && > >> id_data[5] != 0x00) { > >> /* Calc pagesize */ > >> mtd->writesize = 2048 << (extid & 0x03); > > This looks OK (at least for K9GAG08U0D). > > I wonder if Samsung could provide some firm guidelines for when to use > the old style vs. the new style. Okay, how do we proceed? Should I send a proper patch with the diff above? Or does anyone want to try and come up with a better fix...? Regards, Tilman
On 08/20/2010 06:43 AM, Tilman Sauerbeck wrote: > Okay, how do we proceed? Should I send a proper patch with the diff > above? Or does anyone want to try and come up with a better fix...? I vote for Tilman's patch. There's nothing unnecessarily ugly about it; it simply checks cell-type in order to decide whether we use Samsung's new "standard" for MLC or fall-back to the real standard. If anything, the existing code (checking ID length) is ugly. However, both checks seem necessary. Brian
On Fri, 2010-08-20 at 10:42 -0700, Brian Norris wrote: > On 08/20/2010 06:43 AM, Tilman Sauerbeck wrote: > > > Okay, how do we proceed? Should I send a proper patch with the diff > > above? Or does anyone want to try and come up with a better fix...? > > I vote for Tilman's patch. There's nothing unnecessarily ugly about it; > it simply checks cell-type in order to decide whether we use Samsung's > new "standard" for MLC or fall-back to the real standard. If anything, > the existing code (checking ID length) is ugly. However, both checks > seem necessary. That's for 2.6.36 and -stable (for 2.6.35), yes? @@ -2852,6 +2852,7 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd, */ if (id_data[0] == id_data[6] && id_data[1] == id_data[7] && id_data[0] == NAND_MFR_SAMSUNG && + (chip->cellinfo & NAND_CI_CELLTYPE_MSK) && id_data[5] != 0x00) { /* Calc pagesize */ mtd->writesize = 2048 << (extid & 0x03); Can I have a signed-off-by for it? Brian, I have a distinct impression that there's at least one more patch from you that I really ought to be sending to Linus for 2.6.36, but I can't find it right now. Other than this and what's already in mtd-2.6.git, is there anything else? Thanks, btw.
David Woodhouse [2010-08-20 20:53]: > On Fri, 2010-08-20 at 10:42 -0700, Brian Norris wrote: > > On 08/20/2010 06:43 AM, Tilman Sauerbeck wrote: > > > > > Okay, how do we proceed? Should I send a proper patch with the diff > > > above? Or does anyone want to try and come up with a better fix...? > > > > I vote for Tilman's patch. There's nothing unnecessarily ugly about it; > > it simply checks cell-type in order to decide whether we use Samsung's > > new "standard" for MLC or fall-back to the real standard. If anything, > > the existing code (checking ID length) is ugly. However, both checks > > seem necessary. > > That's for 2.6.36 and -stable (for 2.6.35), yes? Yes. > @@ -2852,6 +2852,7 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd, > */ > if (id_data[0] == id_data[6] && id_data[1] == id_data[7] && > id_data[0] == NAND_MFR_SAMSUNG && > + (chip->cellinfo & NAND_CI_CELLTYPE_MSK) && > id_data[5] != 0x00) { > /* Calc pagesize */ > mtd->writesize = 2048 << (extid & 0x03); > > Can I have a signed-off-by for it? Signed-off-by: Tilman Sauerbeck <tilman@code-monkey.de> Thanks, Tilman
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index 4a7b864..abc71cd 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -2847,12 +2847,12 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd, * Old style (4,5 byte ID): Samsung K9GAG08U0M (p.32) * New style (6 byte ID): Samsung K9GAG08U0D (p.40) * - * Check for wraparound + Samsung ID + nonzero 6th byte + * Check for wraparound + Samsung ID + zero 6th byte * to decide what to do. */ if (id_data[0] == id_data[6] && id_data[1] == id_data[7] && id_data[0] == NAND_MFR_SAMSUNG && - id_data[5] != 0x00) { + id_data[5] == 0x00) { /* Calc pagesize */ mtd->writesize = 2048 << (extid & 0x03); extid >>= 2;