* [PATCH] mtd: fix the wrong check condition @ 2012-02-15 10:33 Huang Shijie 2012-02-16 2:59 ` Brian Norris 0 siblings, 1 reply; 8+ messages in thread From: Huang Shijie @ 2012-02-15 10:33 UTC (permalink / raw) To: linux-arm-kernel If we use `||` check condition, many NAND chips which are not ONFI nands have to do the ONFI detection. Use `&&` here to detect the ONFI NAND when we can not find any type in the nand_flash_ids. Signed-off-by: Huang Shijie <b32955@freescale.com> --- drivers/mtd/nand/nand_base.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index 8a393f9..90020a5 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -2966,7 +2966,7 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd, break; chip->onfi_version = 0; - if (!type->name || !type->pagesize) { + if (!type->name && !type->pagesize) { /* Check is chip is ONFI compliant */ ret = nand_flash_detect_onfi(mtd, chip, &busw); if (ret) -- 1.7.3.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH] mtd: fix the wrong check condition 2012-02-15 10:33 [PATCH] mtd: fix the wrong check condition Huang Shijie @ 2012-02-16 2:59 ` Brian Norris 2012-02-16 3:15 ` Huang Shijie [not found] ` <4F3C7433.5030306@freescale.com> 0 siblings, 2 replies; 8+ messages in thread From: Brian Norris @ 2012-02-16 2:59 UTC (permalink / raw) To: linux-arm-kernel (Add Florian and Matthieu) On 2/15/2012 2:33 AM, Huang Shijie wrote: > If we use `||` check condition, many NAND chips which are not > ONFI nands have to do the ONFI detection. Running the ONFI detection on non-ONFI NAND should not, ideally, be a problem. They should fail one or both tests included in the routine: the 'O N F I' string check or the CRC calculation. > Use `&&` here to detect the ONFI NAND when we can not find any type > in the nand_flash_ids. There are many chips whose ID might be in the NAND table but for which it is preferable (or even required) to check by ONFI for one reason or another. For instance, some ONFI chips might use odd-sized OOB that isn't in the ID decoding algorithm. The current `||` check is, I think, designed to weed out old small-page NAND only, which define both 'name' and 'pagesize' in the table. So in short: the current code works as intended. Brian ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] mtd: fix the wrong check condition 2012-02-16 2:59 ` Brian Norris @ 2012-02-16 3:15 ` Huang Shijie 2012-02-16 8:32 ` Brian Norris [not found] ` <4F3C7433.5030306@freescale.com> 1 sibling, 1 reply; 8+ messages in thread From: Huang Shijie @ 2012-02-16 3:15 UTC (permalink / raw) To: linux-arm-kernel Hi, > (Add Florian and Matthieu) > > On 2/15/2012 2:33 AM, Huang Shijie wrote: >> If we use `||` check condition, many NAND chips which are not >> ONFI nands have to do the ONFI detection. > > Running the ONFI detection on non-ONFI NAND should not, ideally, be a > problem. They should fail one or both tests included in the routine: > the 'O N F I' string check or the CRC calculation. NO. I have Hynix nand in my hand: H27UBG8T2A (page size :8192, oob:448). It is not an ONFI nand. But it accidentally can pass the ONFI detection, and get the result : page size 4192, oob:96. This is a wrong result. > >> Use `&&` here to detect the ONFI NAND when we can not find any type >> in the nand_flash_ids. > > There are many chips whose ID might be in the NAND table but for which > it is preferable (or even required) to check by ONFI for one reason or > another. For instance, some ONFI chips might use odd-sized OOB that > isn't in the ID decoding algorithm. > This nand is 32Gb, but we can not parse it out from the id. I ever want to add a new database which use the all the 8/6 bytes id as key. It seems it's time to change it now. Huang Shijie > The current `||` check is, I think, designed to weed out old > small-page NAND only, which define both 'name' and 'pagesize' in the > table. > > So in short: the current code works as intended. > > Brian > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] mtd: fix the wrong check condition 2012-02-16 3:15 ` Huang Shijie @ 2012-02-16 8:32 ` Brian Norris 0 siblings, 0 replies; 8+ messages in thread From: Brian Norris @ 2012-02-16 8:32 UTC (permalink / raw) To: linux-arm-kernel (Add Angus) Hi, On Wed, Feb 15, 2012 at 7:15 PM, Huang Shijie <b32955@freescale.com> wrote: >> On 2/15/2012 2:33 AM, Huang Shijie wrote: >>> >>> If we use `||` check condition, many NAND chips which are not >>> ONFI nands have to do the ONFI detection. >> >> >> Running the ONFI detection on non-ONFI NAND should not, ideally, be a >> problem. They should fail one or both tests included in the routine: >> the 'O N F I' string check or the CRC calculation. > > NO. Whoa, no need to shout... > I have Hynix nand in my hand: H27UBG8T2A (page size :8192, oob:448). > It is not an ONFI nand. > > But it accidentally can pass the ONFI detection, and get the result : page > size 4192, oob:96. This is a wrong result. Yes, this is a wrong result, and that's pretty freaky. Why have a standard if some chips are going to totally demolish it? Uggh... Anyway, you totally left this information out of your patch. If you had, then I would have told you that this patch does not solve *this* problem either; where do you expect the "OOB:448" to come from? (BTW, your attachment didn't come through properly. I'll look for the datasheet elsewhere, I guess...) >>> Use `&&` here to detect the ONFI NAND when we can not find any type >>> in the nand_flash_ids. >> >> >> There are many chips whose ID might be in the NAND table but for which >> it is preferable (or even required) to check by ONFI for one reason or >> another. For instance, some ONFI chips might use odd-sized OOB that >> isn't in the ID decoding algorithm. >> > This nand is 32Gb, but we can not parse it out from the id. > I ever want to add a new database which use the all the 8/6 bytes id as key. > It seems it's time to change it now. Yes, we need some kind of new database. Angus Clark had mentioned wanting to update the table/detection mechanisms. I can help out too. But it needs to be intelligent, since we *can't* log the 8-byte ID for *every* chip. I have some ideas, though, and will share a few. Feel free to do the same. For starters, I'm thinking expand the table(s) to 8-byte ID, with some sort of mask (or length?) parameter to record how many of the 8 bytes are relevant. However, we would need a table for pre-ONFI checks (to catch exceptions like this) and one post-ONFI (for most typical non-ONFI chips): (1) ID table for ONFI exceptions (2) ONFI routines (3) ID table for standard chips We can extend (3) to use some of the current ID decoding mechanisms (for chips who only have 2-byte IDs in their table entry). Angus: do you have work-in-progress that you want to share for fixing up NAND detection? Or shall we work on something new? Brian ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <4F3C7433.5030306@freescale.com>]
* [PATCH] mtd: fix the wrong check condition [not found] ` <4F3C7433.5030306@freescale.com> @ 2012-02-16 11:04 ` Florian Fainelli 2012-02-17 3:07 ` Huang Shijie 0 siblings, 1 reply; 8+ messages in thread From: Florian Fainelli @ 2012-02-16 11:04 UTC (permalink / raw) To: linux-arm-kernel Hi, On 02/16/12 04:12, Huang Shijie wrote: > Hi, >> (Add Florian and Matthieu) >> >> On 2/15/2012 2:33 AM, Huang Shijie wrote: >>> If we use `||` check condition, many NAND chips which are not >>> ONFI nands have to do the ONFI detection. >> >> Running the ONFI detection on non-ONFI NAND should not, ideally, be a >> problem. They should fail one or both tests included in the routine: >> the 'O N F I' string check or the CRC calculation. > NO. Can you please post the dump of the ONFI page as read by your controller? Is the CRC check passing? The ONFI crc function is made so that a page full of zeroes or 0xff won't generate a respectively 0 or ff checksum, so we should catch this during the CRC check. > > I have Hynix nand in my hand: H27UBG8T2A (page size :8192, oob:448). > It is not an ONFI nand. See the datasheet in the attachment. > But it accidentally can pass the ONFI detection, and get the result : > page size 4192, oob:96. This is a wrong result. I have already seen a Hynix chip answering to the READID command too, and this was highly confusing our bootloader, however, I suppose that we should be able to circumvent this issue anyway. > > > > >> >>> Use `&&` here to detect the ONFI NAND when we can not find any type >>> in the nand_flash_ids. >> >> There are many chips whose ID might be in the NAND table but for which >> it is preferable (or even required) to check by ONFI for one reason or >> another. For instance, some ONFI chips might use odd-sized OOB that >> isn't in the ID decoding algorithm. >> > This nand is 32Gb, but we can not parse it out from the id. > I ever want to add a new database which use the all the 8/6 bytes id as > key. > It seems it's time to change it now. You have said that already, but we have yet to see patches for this, I guess if you can post your database patch that will be easier to comment on. -- Florian ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] mtd: fix the wrong check condition 2012-02-16 11:04 ` Florian Fainelli @ 2012-02-17 3:07 ` Huang Shijie 2012-02-17 8:50 ` Florian Fainelli 0 siblings, 1 reply; 8+ messages in thread From: Huang Shijie @ 2012-02-17 3:07 UTC (permalink / raw) To: linux-arm-kernel hi, On Thu, Feb 16, 2012 at 7:04 PM, Florian Fainelli <ffainelli@freebox.fr> wrote: > Hi, > > On 02/16/12 04:12, Huang Shijie wrote: >> >> Hi, >>> >>> (Add Florian and Matthieu) >>> >>> On 2/15/2012 2:33 AM, Huang Shijie wrote: >>>> >>>> If we use `||` check condition, many NAND chips which are not >>>> ONFI nands have to do the ONFI detection. >>> >>> >>> Running the ONFI detection on non-ONFI NAND should not, ideally, be a >>> problem. They should fail one or both tests included in the routine: >>> the 'O N F I' string check or the CRC calculation. >> >> NO. > > > Can you please post the dump of the ONFI page as read by your controller? Is > the CRC check passing? The ONFI crc function is made so that a page full of > zeroes or 0xff won't generate a respectively 0 or ff checksum, so we should > catch this during the CRC check. > Sorry. I did not check the log carefully. the Hynix nand does _not_ pass the CRC check, but it passed the "ONFI" string check. The log "ONFI flash detected" made me misunderstood. BR Huang Shijie >> >> I have Hynix nand in my hand: H27UBG8T2A (page size :8192, oob:448). >> It is not an ONFI nand. See the datasheet in the attachment. >> But it accidentally can pass the ONFI detection, and get the result : >> page size 4192, oob:96. This is a wrong result. > > > I have already seen a Hynix chip answering to the READID command too, and > this was highly confusing our bootloader, however, I suppose that we should > be able to circumvent this issue anyway. > >> >> >> >> >>> >>>> Use `&&` here to detect the ONFI NAND when we can not find any type >>>> in the nand_flash_ids. >>> >>> >>> There are many chips whose ID might be in the NAND table but for which >>> it is preferable (or even required) to check by ONFI for one reason or >>> another. For instance, some ONFI chips might use odd-sized OOB that >>> isn't in the ID decoding algorithm. >>> >> This nand is 32Gb, but we can not parse it out from the id. >> I ever want to add a new database which use the all the 8/6 bytes id as >> key. >> It seems it's time to change it now. > > > You have said that already, but we have yet to see patches for this, I guess > if you can post your database patch that will be easier to comment on. > -- > Florian > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] mtd: fix the wrong check condition 2012-02-17 3:07 ` Huang Shijie @ 2012-02-17 8:50 ` Florian Fainelli 2012-02-18 5:43 ` Brian Norris 0 siblings, 1 reply; 8+ messages in thread From: Florian Fainelli @ 2012-02-17 8:50 UTC (permalink / raw) To: linux-arm-kernel Le 02/17/12 04:07, Huang Shijie a ?crit : > hi, > > On Thu, Feb 16, 2012 at 7:04 PM, Florian Fainelli<ffainelli@freebox.fr> wrote: >> Hi, >> >> On 02/16/12 04:12, Huang Shijie wrote: >>> Hi, >>>> (Add Florian and Matthieu) >>>> >>>> On 2/15/2012 2:33 AM, Huang Shijie wrote: >>>>> If we use `||` check condition, many NAND chips which are not >>>>> ONFI nands have to do the ONFI detection. >>>> >>>> Running the ONFI detection on non-ONFI NAND should not, ideally, be a >>>> problem. They should fail one or both tests included in the routine: >>>> the 'O N F I' string check or the CRC calculation. >>> NO. >> >> Can you please post the dump of the ONFI page as read by your controller? Is >> the CRC check passing? The ONFI crc function is made so that a page full of >> zeroes or 0xff won't generate a respectively 0 or ff checksum, so we should >> catch this during the CRC check. >> > Sorry. I did not check the log carefully. > > the Hynix nand does _not_ pass the CRC check, but it passed the "ONFI" > string check. This is exactly what I saw too with a different Hynix chip, it replied ONFI, but the CRC check failed. > > The log "ONFI flash detected" made me misunderstood. Yes I agree, the log is there too soon, thanks for your patch. Thanks. > > BR > Huang Shijie > > >>> I have Hynix nand in my hand: H27UBG8T2A (page size :8192, oob:448). >>> It is not an ONFI nand. See the datasheet in the attachment. >>> But it accidentally can pass the ONFI detection, and get the result : >>> page size 4192, oob:96. This is a wrong result. >> >> I have already seen a Hynix chip answering to the READID command too, and >> this was highly confusing our bootloader, however, I suppose that we should >> be able to circumvent this issue anyway. >> >>> >>> >>> >>>>> Use `&&` here to detect the ONFI NAND when we can not find any type >>>>> in the nand_flash_ids. >>>> >>>> There are many chips whose ID might be in the NAND table but for which >>>> it is preferable (or even required) to check by ONFI for one reason or >>>> another. For instance, some ONFI chips might use odd-sized OOB that >>>> isn't in the ID decoding algorithm. >>>> >>> This nand is 32Gb, but we can not parse it out from the id. >>> I ever want to add a new database which use the all the 8/6 bytes id as >>> key. >>> It seems it's time to change it now. >> >> You have said that already, but we have yet to see patches for this, I guess >> if you can post your database patch that will be easier to comment on. >> -- >> Florian >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel at lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] mtd: fix the wrong check condition 2012-02-17 8:50 ` Florian Fainelli @ 2012-02-18 5:43 ` Brian Norris 0 siblings, 0 replies; 8+ messages in thread From: Brian Norris @ 2012-02-18 5:43 UTC (permalink / raw) To: linux-arm-kernel On Fri, Feb 17, 2012 at 12:50 AM, Florian Fainelli <ffainelli@freebox.fr> wrote: > Le 02/17/12 04:07, Huang Shijie a ?crit : >> Sorry. I did not check the log carefully. >> >> the Hynix nand does _not_ pass the CRC check, but it passed the "ONFI" >> string check. > > This is exactly what I saw too with a different Hynix chip, it replied ONFI, > but the CRC check failed. So your root detection issue is actually that this Hynix chip is neither ONFI-compliant nor does it have a good ID decoding scheme, hence the need for the ID table patch (separate; I'll review it soon). >> >> The log "ONFI flash detected" made me misunderstood. > > Yes I agree, the log is there too soon, thanks for your patch. I agree as well. I'll ACK the other patch. Thanks, Brian ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2012-02-18 5:43 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-02-15 10:33 [PATCH] mtd: fix the wrong check condition Huang Shijie
2012-02-16 2:59 ` Brian Norris
2012-02-16 3:15 ` Huang Shijie
2012-02-16 8:32 ` Brian Norris
[not found] ` <4F3C7433.5030306@freescale.com>
2012-02-16 11:04 ` Florian Fainelli
2012-02-17 3:07 ` Huang Shijie
2012-02-17 8:50 ` Florian Fainelli
2012-02-18 5:43 ` Brian Norris
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).