| Message ID | 1382172254-12448-5-git-send-email-pekon@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Pekon, On Sat, Oct 19, 2013 at 02:14:08PM +0530, Pekon Gupta wrote: > As per comments below, NAND_CMD_RESET, NAND_CMD_READID, and NAND_CMD_PARAM would > work only in x8 mode. > commit 64b37b2a63eb2f80b65c7185f0013f8ffc637ae3 > Author: Matthieu CASTET <matthieu.castet@parrot.com> > AuthorDate: 2012-11-06 > Note that nand_scan_ident send command (NAND_CMD_RESET, NAND_CMD_READID, NAND_CMD_PARAM), address and read data > The ONFI specificication is not very clear for x16 device if high byte of address should be driven to 0, > but according to [1] it should be ok to not drive it during autodetection. > > [1] > 3.3.2. Target Initialization > > [...] > The Read ID and Read Parameter Page commands only use the lower 8-bits of the data bus. > The host shall not issue commands that use a word data width on x16 devices until the host > determines the device supports a 16-bit data bus width in the parameter page. > > Thus this patch run nand_scan_ident() with driver configured as x8 device. So are you saying that the driver currently doesn't work if you started in x16 buswidth? Are you having problems with a particular setup? What sort of devices are you testing? Running nand_scan_ident() with x8 when the device is actualy x16 will just cause nand_scan_ident() to abort with an error. It doesn't help you with the fact that RESET/READID/PARAM need special 8-bit handling on x16 devices, so you're not solving the problem alluded to by Matthieu. > Once the NAND device is detected, and its ONFI params are read, the driver > is re-configured based on device-width as passed by DT bindinig 'nand-bus-width' > > In-case there is a mis-match between the DT binding 'nand-bus-width' and actual > device-width detected during nand_get_flash_type() then probe returns failure. > > All other low-level callback updates happen after the device detection. > > Signed-off-by: Pekon Gupta <pekon@ti.com> What is your patch trying to solve? It seems like it's just a distortion of the same code that existed previously. It tries running nand_scan_ident() in one buswidth configuration, and then it tries the other if it failed. You still aren't doing either of the options we talked about previously. I'll repeat them: (1) You specify the buswidth given by device-tree/platform-data; if this is incorrect, you fail (2) You auto-configure using NAND_BUSWIDTH_AUTO, and you use that to validate the platform data; you just warn if the buswidth provided by device-tree/platform-data was incorrect Am I sensing that you are having some implementation problem with one of these? You really shouldn't need to run nand_scan_ident() twice. Or perhaps the "solution" in this patch is just that you're moving around the reassignment of the callback functions? If so, this is not obvious... see below. > --- > drivers/mtd/nand/omap2.c | 45 +++++++++++++++++++++++++++++++++------------ > 1 file changed, 33 insertions(+), 12 deletions(-) > > diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c > index 5596368..d29edda 100644 > --- a/drivers/mtd/nand/omap2.c > +++ b/drivers/mtd/nand/omap2.c > @@ -1856,7 +1856,6 @@ static int omap_nand_probe(struct platform_device *pdev) > mtd->name = dev_name(&pdev->dev); > mtd->owner = THIS_MODULE; > nand_chip = &info->nand; > - nand_chip->options = pdata->devsize; > nand_chip->options |= NAND_SKIP_BBTSCAN; > #ifdef CONFIG_MTD_NAND_OMAP_BCH > info->of_node = pdata->of_node; > @@ -1904,6 +1903,39 @@ static int omap_nand_probe(struct platform_device *pdev) > nand_chip->chip_delay = 50; > } > > + /* scan NAND device connected to chip controller */ Why is this comment separated from the comment below it? Either give a newline between them (if they're really separate) or make it a single comment block. > + /* configure driver in x8 mode to read ONFI parameter page, as > + * NAND_CMD_READID & NAND_CMD_PARAM may not work in x16 mode */ > + nand_chip->options &= ~NAND_BUSWIDTH_16; > + if (nand_scan_ident(mtd, 1, NULL)) { > + /* nand_scan_ident failed */ > + if (pdata->devsize) { > + /* may be because of mis-match of device-width, > + * platform data (DT binding) also says its x16 device > + * So re-scan with proper device-width */ > + nand_chip->options |= pdata->devsize; > + if (nand_scan_ident(mtd, 1, NULL)) { > + err = -ENXIO; > + goto out_release_mem_region; > + } > + } else { > + /* some genuine failure, because even platform-data > + * (DT binding) says that bus-width is x8 */ > + err = -ENXIO; > + goto out_release_mem_region; > + } > + } else { > + /* nand_scan_ident passed with x8 mode */ > + if (pdata->devsize) { > + /* but platform-data (DT binding) say its x16 device */ > + pr_err("%s: incorrect bus-width config\n", DRIVER_NAME); You only print this message in one case (detect x8, but DT said x16) and not the other (detect x16, but DT said x8). This is a result of your unnecessarily complicated logic here. > + err = -EINVAL; > + err = -ENXIO; > + goto out_release_mem_region; > + } > + } > + > + /* re-populate low-level callbacks based on xfer modes */ So am I to understand that the main purpose of this patch is not about the detection of the buswidth type, but about the (re)assignment of the callback functions? If so, you aren't making it very clear. (I wasn't paying too much attention to the fact that this code block is being moved across all the callback assignments.) The above code is just a more verbose way of doing the same thing as the code you're replacing, with an extra check to compare with the device-tree/platform-data. > switch (pdata->xfer_type) { > case NAND_OMAP_PREFETCH_POLLED: > nand_chip->read_buf = omap_read_buf_pref; > @@ -2011,17 +2043,6 @@ static int omap_nand_probe(struct platform_device *pdev) > } > } > > - /* DIP switches on some boards change between 8 and 16 bit > - * bus widths for flash. Try the other width if the first try fails. > - */ > - if (nand_scan_ident(mtd, 1, NULL)) { > - nand_chip->options ^= NAND_BUSWIDTH_16; > - if (nand_scan_ident(mtd, 1, NULL)) { > - err = -ENXIO; > - goto out_release_mem_region; > - } > - } > - > /* rom code layout */ > if (pdata->ecc_opt == OMAP_ECC_HAM1_CODE_HW) { > Anyway, I'm just going to have to flat out NAK this patch for now. Please rework the series without this patch and we can continue discussion of this patch independently (for one, this thread does not need to CC all of the device-tree folks). Brian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Brian, > From: Brian Norris [mailto:computersforpeace@gmail.com] > On Sat, Oct 19, 2013 at 02:14:08PM +0530, Pekon Gupta wrote: [...] > > > > Thus this patch run nand_scan_ident() with driver configured as x8 device. > > So are you saying that the driver currently doesn't work if you started > in x16 buswidth? Are you having problems with a particular setup? What > sort of devices are you testing? > No, I'm saying that you cannot read ONFI params in x16 mode. And, that is what was pointed out in following commit log also .. (Reference to 3.3.2. Target Initialization: given above) So, if I run nand_scan_ident() in x16 mode, my ONFI detection would fail for sure .. > Running nand_scan_ident() with x8 when the device is actualy x16 will > just cause nand_scan_ident() to abort with an error. It doesn't help you > with the fact that RESET/READID/PARAM need special 8-bit handling on x16 > devices, so you're not solving the problem alluded to by Matthieu. > Yes, absolutely agree.. The original code was calling nand_scan_ident() twice, without taking into consideration whether the first nand_scan_ident() failed because of bus-width mismatch or something else. I'm also calling nand_scan_ident() twice, but only when the failure may be due to bus-width mis-match. I'm just avoiding an extra call to nand_scan_ident() if the failure was genuine. If the device actually was x8 then nand_scan_ident() should not fail for the first-time (in my patch), but if it still fails, then it's a genuine failure _not_ because of bus-width mismatch. I'm avoiding the call to nand_scan_ident() again .. (same is in my comments below..) [...] > What is your patch trying to solve? It seems like it's just a distortion > of the same code that existed previously. It tries running > nand_scan_ident() in one buswidth configuration, and then it tries the > other if it failed. You still aren't doing either of the options we > talked about previously. I'll repeat them: > Absolutely.. probably you missed my replies in [PATCH v9 4/9]... > (1) You specify the buswidth given by device-tree/platform-data; if this > is incorrect, you fail > Absolutely this is what I'm doing. But tell me how would you know the actual device-width if nand_scan_ident() fails (a) to probe ONFI params and - your device is not in nand_ids[] So get the actual device width, I call the first nand_scan_ident() in x8 mode. so that ONFI params are read. Now, if it nand_scan_ident() fails then it means actual device *may* be x16 So, I re-invoke nand_scan_ident() with chip->option |= pdata->devsize; > (2) You auto-configure using NAND_BUSWIDTH_AUTO, and you use that to > validate the platform data; you just warn if the buswidth provided > by device-tree/platform-data was incorrect > I have removed NAND_BUSWIDTH_AUTO implementation, as per feedbacks This is <new> patch. May be you are confusing it with earlier version... *please re-review* > Am I sensing that you are having some implementation problem with one of > these? You really shouldn't need to run nand_scan_ident() twice. > > Or perhaps the "solution" in this patch is just that you're moving > around the reassignment of the callback functions? If so, this is not > obvious... see below. > I'm repost my replies from [PATCH v9 4/9] in-case you missed... ------------------------------------ The GPMC driver will be touched by NAND_BUSWIDTH_AUTO change. There are two set of configurations in GPMC controller.. (a) device based configurations: These are static configurations derived on DT bindings for each chip-select (like NAND signal timings, etc). These done on GPMC probe. (b) ecc-scheme based configurations: These are dynamic configurations done in NAND driver in chip->ecc.hwctl() and are refreshed at each NAND accesss. However, 'nand-bus-width' configuration is part of both (1) and (2). Thus, both 'OMAP NAND driver' and 'GPMC driver' need to be consistent in programming 'nand-bus-width' otherwise ecc-engine would not work. Thus, I'm proceeding with (1) DT based parsing of 'nand-bus-width'. I have re-posted [PATCH v10 4/10] with this updates. However, please take into account that some limitation of dual programming require such probe. New patch also call nand_scan_ident() twice, but only on certain pre-determined conditions, not in all failure. Once I convert to NAND_BUSWIDTH_AUTO these should get clean, as I would update both GPMC and NAND driver for this. (Till then this was most optimal solution I could think of).. ------------------------------------ > > --- > > drivers/mtd/nand/omap2.c | 45 > +++++++++++++++++++++++++++++++++------------ > > 1 file changed, 33 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c > > index 5596368..d29edda 100644 > > --- a/drivers/mtd/nand/omap2.c > > +++ b/drivers/mtd/nand/omap2.c > > @@ -1856,7 +1856,6 @@ static int omap_nand_probe(struct > platform_device *pdev) > > mtd->name = dev_name(&pdev->dev); > > mtd->owner = THIS_MODULE; > > nand_chip = &info->nand; > > - nand_chip->options = pdata->devsize; > > nand_chip->options |= NAND_SKIP_BBTSCAN; See there is no NAND_BUSWIDTH_AUTO here .... > > + /* re-populate low-level callbacks based on xfer modes */ > > switch (pdata->xfer_type) { > > case NAND_OMAP_PREFETCH_POLLED: > > nand_chip->read_buf = omap_read_buf_pref; > > So am I to understand that the main purpose of this patch is not about > the detection of the buswidth type, but about the (re)assignment of the > callback functions? If so, you aren't making it very clear. (I wasn't > paying too much attention to the fact that this code block is being > moved across all the callback assignments.) The above code is just a > more verbose way of doing the same thing as the code you're replacing, > with an extra check to compare with the device-tree/platform-data. > Nope, this is just the comment clean-up.. Please drop it if it confuses you. One main request please review this patch by locally including it. May be then you can understand that it has *nothing* to do with (re)assignment of callbacks.. rather there is no re-assignment at all in this patch.. See there is no change in the lines below this comment change Probably this comment clean-up confused you.. > > @@ -2011,17 +2043,6 @@ static int omap_nand_probe(struct > platform_device *pdev) > > } > > } > > > > - /* DIP switches on some boards change between 8 and 16 bit > > - * bus widths for flash. Try the other width if the first try fails. > > - */ > > - if (nand_scan_ident(mtd, 1, NULL)) { > > - nand_chip->options ^= NAND_BUSWIDTH_16; > > - if (nand_scan_ident(mtd, 1, NULL)) { > > - err = -ENXIO; > > - goto out_release_mem_region; > > - } > > - } > > - > > /* rom code layout */ > > if (pdata->ecc_opt == OMAP_ECC_HAM1_CODE_HW) { > > > > Anyway, I'm just going to have to flat out NAK this patch for now. > Please rework the series without this patch and we can continue > discussion of this patch independently (for one, this thread does not > need to CC all of the device-tree folks). > I think there was some confusion here.. So, I hv explained my patch above. Request you to please re-review this. I'll take NAND_BUSWIDTH_AUTO implementation as a separate patch, because it would require changes in GPMC driver as stated above. And so it would be all together independent patch-set. I would wait for your reply.. with regards, pekon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/22/2013 10:07 PM, Gupta, Pekon wrote: >> From: Brian Norris [mailto:computersforpeace@gmail.com] >> On Sat, Oct 19, 2013 at 02:14:08PM +0530, Pekon Gupta wrote: > [...] > >>> >>> Thus this patch run nand_scan_ident() with driver configured as x8 device. >> >> So are you saying that the driver currently doesn't work if you started >> in x16 buswidth? Are you having problems with a particular setup? What >> sort of devices are you testing? >> > No, I'm saying that you cannot read ONFI params in x16 mode. > And, that is what was pointed out in following commit log also .. > (Reference to 3.3.2. Target Initialization: given above) > So, if I run nand_scan_ident() in x16 mode, my ONFI detection would > fail for sure .. But you cannot just run nand_scan_ident() with !(chip->options & NAND_BUSWIDTH_16) when your devices is x16. You need to solve the ONFI detection problem while correctly specifying NAND_BUSWIDTH_16. Since you didn't answer the other 2 questions there: are you testing any x16 devices? >> Running nand_scan_ident() with x8 when the device is actualy x16 will >> just cause nand_scan_ident() to abort with an error. It doesn't help you >> with the fact that RESET/READID/PARAM need special 8-bit handling on x16 >> devices, so you're not solving the problem alluded to by Matthieu. >> > Yes, absolutely agree.. > The original code was calling nand_scan_ident() twice, without taking > into consideration whether the first nand_scan_ident() failed because > of bus-width mismatch or something else. > > I'm also calling nand_scan_ident() twice, but only when the failure may > be due to bus-width mis-match. I'm just avoiding an extra call to > nand_scan_ident() if the failure was genuine. You NEVER need to call nand_scan_ident() twice for the same chip. Period. I will reject any patch that retains this pattern. It is wrong, and I seriously doubt the code does what you think it does when you do this. I think nand_scan_ident() may have a weak point where it won't support ONFI properly for x16 devices. I believe NAND_BUSWIDTH_AUTO was added to help with this fact. (I don't have any x16 devices to test it.) But if this is a problem for you, fix it. Don't work around it. >> What is your patch trying to solve? It seems like it's just a distortion >> of the same code that existed previously. It tries running >> nand_scan_ident() in one buswidth configuration, and then it tries the >> other if it failed. You still aren't doing either of the options we >> talked about previously. I'll repeat them: >> > Absolutely.. probably you missed my replies in [PATCH v9 4/9]... No, I did not. I just don't see how you think that your code matches the options (1) or (2) that I described. Perhaps it's a failure in communication. I will try to be absolutely clear. >> (1) You specify the buswidth given by device-tree/platform-data; if this >> is incorrect, you fail >> > Absolutely this is what I'm doing. No it isn't. You are ignoring the provided buswidth information and UNCONDITIONALLY trying x8. If/when that fails, you then error out or retry in x16 (depending on the DT/platform-data). This is plain wrong. nand_base is designed (and it's documented in the comments) that the driver must set the buswidth correctly BEFORE calling nand_scan_ident(). You may not use nand_scan_ident() as a trial-and-error identification function. So, to properly do (1), you should only have something like this, just like all the other NAND drivers: nand_chip->options = pdata->devsize & NAND_BUSWIDTH_16; ret = nand_scan_ident(...); if (ret) { // exit with error code... } If there is a problem with this, then you have to fix your driver or nand_scan_ident(). Perhaps you have to adjust your readbuf() or cmdfunc() callbacks to do this. But do not add complicated workaround logic in your driver. > But tell me how would you know the actual device-width if > nand_scan_ident() fails If you are not using NAND_BUSWIDTH_AUTO, then you MUST know the correct buswidth before calling nand_scan_ident(). If your device-tree/platform-data is wrong, then fix that. > (a) to probe ONFI params and > - your device is not in nand_ids[] > So get the actual device width, I call the first nand_scan_ident() in x8 mode. > so that ONFI params are read. You don't call nand_scan_ident() with !(chip->options & NAND_BUSWIDTH_16) when you have an x16 device. Now, if this causes NAND_CMD_PARAM to fail, then you have a *different* problem to solve. But you are not solving this problem. [snipping the rest] I read your patch, and I gave you my review. I will not accept this patch, nor any patch that works around nand_scan_ident() by calling it twice. Fix the framework if the framework is giving you problems. I believe that this patch is not integral to the rest of the series, so I will repeat: separate this patch out so I can take the rest of this series without it. Brian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Brian, > From: Brian Norris [mailto:computersforpeace@gmail.com] > > On 10/22/2013 10:07 PM, Gupta, Pekon wrote: > >> From: Brian Norris [mailto:computersforpeace@gmail.com] > >> On Sat, Oct 19, 2013 at 02:14:08PM +0530, Pekon Gupta wrote: [...] >> So are you saying that the driver currently doesn't work if you started >> in x16 buswidth? Are you having problems with a particular setup? What >> sort of devices are you testing? >> ... > Since you didn't answer the other 2 questions there: are you testing any > x16 devices? > Ans-1: Yes, I'm testing on following boards: (a) AM335x-EVM which has x8 Micron device. http://www.ti.com/tool/tmdxevm3358 (b) beaglebone with 'NAND cape', which has x16 Micron device. http://beagleboardtoys.info/index.php?title=BeagleBone_4Gb_16-Bit_NAND_Module (c) AM437x board (which has 4K page NAND from Macronix). (d) Also would be testing on DRA7xx again having Micron Device. Ans-2: Its not the setup but rather constrain in nand_base.c which prevents reading of ONFI params in x16 mode. Please read below.. Ans-3: Mostly are either x8 or x16 SLC NAND device. [...] > You NEVER need to call nand_scan_ident() twice for the same chip. > Period. I will reject any patch that retains this pattern. It is wrong, > and I seriously doubt the code does what you think it does when you do this. > > I think nand_scan_ident() may have a weak point where it won't support > ONFI properly for x16 devices. I believe NAND_BUSWIDTH_AUTO was added > to > help with this fact. (I don't have any x16 devices to test it.) But if > this is a problem for you, fix it. Don't work around it. > So, here comes the conflict.. If I'm _not_ using NAND_BUSWIDTH_AUTO, how should I read the ONFI params on x16 device ? I don't think there is any way because of following code in generic nand_base.c @@ nand_flash_onfi_detect() /* * ONFI must be probed in 8-bit mode or with NAND_BUSWIDTH_AUTO, not * with NAND_BUSWIDTH_16 */ if (chip->options & NAND_BUSWIDTH_16) { pr_err("ONFI cannot be probed in 16-bit mode; aborting\n"); return 0; } Introduced in commit.. commit 0ce82b7f7b7373b16ecf7b5725e21e2975204500 Author: Matthieu CASTET <matthieu.castet@parrot.com> AuthorDate: 2013-01-16 And, I think this commit has implicitly made NAND_BUSWIDTH_AUTO *mandatory* to be supported by every driver for interfacing with x16 NAND devices. Isn't it ? (unless you add every x16 device present in market to nand_flash_id[]) [...] > nand_base is designed (and it's documented in the comments) that the > driver must set the buswidth correctly BEFORE calling nand_scan_ident(). > You may not use nand_scan_ident() as a trial-and-error identification > function. > > So, to properly do (1), you should only have something like this, just > like all the other NAND drivers: > > nand_chip->options = pdata->devsize & NAND_BUSWIDTH_16; > > ret = nand_scan_ident(...); > if (ret) { > // exit with error code... > } > For a x16 device.. This would *always* fail for x16 device, unless device is listed in nand_flash_id[]. isn't it ? because you cannot read ONFI params in x16 mode, and your device is not listed in nand_flash_ids[], so your device would not get identified. And I sincerely don't know how other NAND drivers are probing x16 NAND devices _without_ enabling NAND_BUSWIDTH_AUTO. (may be by adding every supported NAND device to nand_flash_id[] look-up table, which is not recommended). > If there is a problem with this, then you have to fix your driver or > nand_scan_ident(). Perhaps you have to adjust your readbuf() or > cmdfunc() callbacks to do this. But do not add complicated workaround > logic in your driver. > chip->cmdfunc() and chip->readbufs() are all fine. Rather I let the generic driver set them for nand_scan_ident(). So, all this calling nand_scan_ident() twice was done because of previously mentioned reasons.. > [snipping the rest] > > I read your patch, and I gave you my review. I will not accept this > patch, nor any patch that works around nand_scan_ident() by calling it > twice. Fix the framework if the framework is giving you problems. > It's a chicken and egg problem, I have no solution but all I can say is the above commit, which converted WARNING into ERROR, until all drivers adapt to NAND_BUSWIDTH_AUTO. Anyway I have to do nand_scan_ident() somewhere before populating the chip->ecc.layout and other fields.. so can't drop the patch.. But I'll put your suggestion, instead of my mine. > I believe that this patch is not integral to the rest of the series, so > I will repeat: separate this patch out so I can take the rest of this > series without it. > I'll replace the patch with your suggestions, So, that you have both versions. Please pick whichever you like :-) with regards, pekon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Brian, On Tue, Oct 22, 2013 at 11:13:32PM -0700, Brian Norris wrote: > On 10/22/2013 10:07 PM, Gupta, Pekon wrote: > >> From: Brian Norris [mailto:computersforpeace@gmail.com] > >> On Sat, Oct 19, 2013 at 02:14:08PM +0530, Pekon Gupta wrote: > > [...] > > > >>> > >>> Thus this patch run nand_scan_ident() with driver configured as x8 device. > >> > >> So are you saying that the driver currently doesn't work if you started > >> in x16 buswidth? Are you having problems with a particular setup? What > >> sort of devices are you testing? > >> > > No, I'm saying that you cannot read ONFI params in x16 mode. > > And, that is what was pointed out in following commit log also .. > > (Reference to 3.3.2. Target Initialization: given above) > > So, if I run nand_scan_ident() in x16 mode, my ONFI detection would > > fail for sure .. > > But you cannot just run nand_scan_ident() with !(chip->options & > NAND_BUSWIDTH_16) when your devices is x16. You need to solve the ONFI > detection problem while correctly specifying NAND_BUSWIDTH_16. > > Since you didn't answer the other 2 questions there: are you testing any > x16 devices? > I'm jumping on this thread without having read all the discussion, sorry about that. FWIW, I have a Beaglebone with a 16-bit bus NAND attached to it. Coincidentally, yesterday I was doing some tests as I'm ramping up the NAND and I found that weird double nand_scan_ident() call. The whole thing looks buggy to me, so I'm happy to help, review, test and patches to take care of this. I'm using some TI SDK with some ancient v3.2.x (with no git history!), but from this discussion it seems the issue is still present in mainline.
Hi, > From: Ezequiel Garcia [mailto:ezequiel.garcia@free-electrons.com] [...] > FWIW, I have a Beaglebone with a 16-bit bus NAND attached to it. > > Coincidentally, yesterday I was doing some tests as I'm ramping up the > NAND and I found that weird double nand_scan_ident() call. > The whole thing looks buggy to me, so I'm happy to help, review, test > and patches to take care of this. > Yes, thanks .. that would be of great help.. And may be your experience of Atmel drivers would help me here.. *Correct, should not be double calls to nand_scan_ident()..* But there is a constrain in nand_base.c, that it does not allow ONFI page reading in x16 mode.. So how to overcome that.. I see the similar implementation in your ATMEL driver, it does not use NAND_BUSWIDTH_AUTO so how do you perform ONFI read for x16 devices ? drivers/mtd/nand/atmel_nand.c @@atmel_nand_probe() /* here you move to x16 mode based on your DT or platform data */ if (host->board.bus_width_16) /* 16-bit bus width */ nand_chip->options |= NAND_BUSWIDTH_16; /* And then you call nand_scan_ident */ /* first scan to find the device and get the page size */ if (nand_scan_ident(mtd, 1, NULL)) { res = -ENXIO; goto err_scan_ident; } Wouldn't this fail, _unless_ your device is listed in nand_flash_id[] ? because it would not be able to read ONFI params.. Refer below commit.. commit 0ce82b7f7b7373b16ecf7b5725e21e2975204500 Author: Matthieu CASTET <matthieu.castet@parrot.com> AuthorDate: 2013-01-16 Thanks for pitching in, this would help me to understand this better. > I'm using some TI SDK with some ancient v3.2.x (with no git history!), > but from this discussion it seems the issue is still present in > mainline. > Aah sorry, then you might have some problem here in rebasing the patches. But still if you can, thanks much .. with regards, pekon
Hi Gupta, On Wed, Oct 23, 2013 at 01:15:20PM +0000, Gupta, Pekon wrote: > Hi, > > > From: Ezequiel Garcia [mailto:ezequiel.garcia@free-electrons.com] > [...] > > FWIW, I have a Beaglebone with a 16-bit bus NAND attached to it. > > > > Coincidentally, yesterday I was doing some tests as I'm ramping up the > > NAND and I found that weird double nand_scan_ident() call. > > The whole thing looks buggy to me, so I'm happy to help, review, test > > and patches to take care of this. > > > Yes, thanks .. that would be of great help.. > And may be your experience of Atmel drivers would help me here.. > It's not Atmel, but Marvell :-) > *Correct, should not be double calls to nand_scan_ident()..* > But there is a constrain in nand_base.c, that it does not allow ONFI > page reading in x16 mode.. So how to overcome that.. > > I see the similar implementation in your ATMEL driver, it does not use > NAND_BUSWIDTH_AUTO so how do you perform ONFI read > for x16 devices ? > drivers/mtd/nand/atmel_nand.c @@atmel_nand_probe() > /* here you move to x16 mode based on your DT or platform data */ > if (host->board.bus_width_16) /* 16-bit bus width */ > nand_chip->options |= NAND_BUSWIDTH_16; > /* And then you call nand_scan_ident */ > /* first scan to find the device and get the page size */ > if (nand_scan_ident(mtd, 1, NULL)) { > res = -ENXIO; > goto err_scan_ident; > } > > Wouldn't this fail, _unless_ your device is listed in nand_flash_id[] ? > because it would not be able to read ONFI params.. > Refer below commit.. > commit 0ce82b7f7b7373b16ecf7b5725e21e2975204500 > Author: Matthieu CASTET <matthieu.castet@parrot.com> > AuthorDate: 2013-01-16 > > Not my driver, but I'm taking a look at it now. Not sure if I'll get into something here. > > > > I'm using some TI SDK with some ancient v3.2.x (with no git history!), > > but from this discussion it seems the issue is still present in > > mainline. > > > Aah sorry, then you might have some problem here in rebasing the > patches. But still if you can, thanks much .. > I'm currently trying mainline (just for this issue not for my product). I just need some time to prepare the bootargs and write a DT node for the NAND cape. Again, not sure if I'll make some progress, but I'll give it a shot :-)
Pekon, On Wed, Oct 23, 2013 at 10:24:57AM -0300, Ezequiel Garcia wrote: [..] > > I'm currently trying mainline (just for this issue not for my product). > I just need some time to prepare the bootargs and write a DT node for > the NAND cape. > > Again, not sure if I'll make some progress, but I'll give it a shot :-) I won't be able to make too much progress without some help or without squeezing my brains out :P Care to push some git branch on some random repo with DT support for the NAND cape in the Beaglebone? Please base it in either some recent l2-mtd or some tag from Linus. Thanks!
Hi Ezequiel, > From: Ezequiel Garcia [mailto:ezequiel.garcia@free-electrons.com] > I won't be able to make too much progress without some help or without > squeezing my brains out :P > > Care to push some git branch on some random repo with DT support for > the NAND cape in the Beaglebone? > Apologies for delay, I was testing and preparing a newer version of patch-set.. Please find the beagle-bone DTS attached. However, consider this as RFC only, as I'm waiting for current series which has some DT-binding updates to get accepted first. Commit log in the patch would also guide you for some board tweaks for enabling NAND cape on beagle-bone (LT/white).. (I have recently sent v11 of the patch series incorporating last few comments from Brian about nand_scan_ident().) Thanks.. with regards, pekon
Pekon, On Thu, Oct 24, 2013 at 12:59:26PM +0000, Gupta, Pekon wrote: > Hi Ezequiel, > > > From: Ezequiel Garcia [mailto:ezequiel.garcia@free-electrons.com] > > I won't be able to make too much progress without some help or without > > squeezing my brains out :P > > > > Care to push some git branch on some random repo with DT support for > > the NAND cape in the Beaglebone? > > > Apologies for delay, I was testing and preparing a newer version of patch-set.. > > Please find the beagle-bone DTS attached. However, consider this as RFC > only, as I'm waiting for current series which has some DT-binding updates > to get accepted first. > > Commit log in the patch would also guide you for some board tweaks > for enabling NAND cape on beagle-bone (LT/white).. > (I have recently sent v11 of the patch series incorporating last few > comments from Brian about nand_scan_ident().) > Thanks! I'll give this a try. Anyway, for future reference, it's really easier for testers and reviewers to just push the code in some public git repo (github? gitorious?). This way I don't have to apply the patches from my mailbox, but just checkout a branch...
diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c index 5596368..d29edda 100644 --- a/drivers/mtd/nand/omap2.c +++ b/drivers/mtd/nand/omap2.c @@ -1856,7 +1856,6 @@ static int omap_nand_probe(struct platform_device *pdev) mtd->name = dev_name(&pdev->dev); mtd->owner = THIS_MODULE; nand_chip = &info->nand; - nand_chip->options = pdata->devsize; nand_chip->options |= NAND_SKIP_BBTSCAN; #ifdef CONFIG_MTD_NAND_OMAP_BCH info->of_node = pdata->of_node; @@ -1904,6 +1903,39 @@ static int omap_nand_probe(struct platform_device *pdev) nand_chip->chip_delay = 50; } + /* scan NAND device connected to chip controller */ + /* configure driver in x8 mode to read ONFI parameter page, as + * NAND_CMD_READID & NAND_CMD_PARAM may not work in x16 mode */ + nand_chip->options &= ~NAND_BUSWIDTH_16; + if (nand_scan_ident(mtd, 1, NULL)) { + /* nand_scan_ident failed */ + if (pdata->devsize) { + /* may be because of mis-match of device-width, + * platform data (DT binding) also says its x16 device + * So re-scan with proper device-width */ + nand_chip->options |= pdata->devsize; + if (nand_scan_ident(mtd, 1, NULL)) { + err = -ENXIO; + goto out_release_mem_region; + } + } else { + /* some genuine failure, because even platform-data + * (DT binding) says that bus-width is x8 */ + err = -ENXIO; + goto out_release_mem_region; + } + } else { + /* nand_scan_ident passed with x8 mode */ + if (pdata->devsize) { + /* but platform-data (DT binding) say its x16 device */ + pr_err("%s: incorrect bus-width config\n", DRIVER_NAME); + err = -EINVAL; + err = -ENXIO; + goto out_release_mem_region; + } + } + + /* re-populate low-level callbacks based on xfer modes */ switch (pdata->xfer_type) { case NAND_OMAP_PREFETCH_POLLED: nand_chip->read_buf = omap_read_buf_pref; @@ -2011,17 +2043,6 @@ static int omap_nand_probe(struct platform_device *pdev) } } - /* DIP switches on some boards change between 8 and 16 bit - * bus widths for flash. Try the other width if the first try fails. - */ - if (nand_scan_ident(mtd, 1, NULL)) { - nand_chip->options ^= NAND_BUSWIDTH_16; - if (nand_scan_ident(mtd, 1, NULL)) { - err = -ENXIO; - goto out_release_mem_region; - } - } - /* rom code layout */ if (pdata->ecc_opt == OMAP_ECC_HAM1_CODE_HW) {
As per comments below, NAND_CMD_RESET, NAND_CMD_READID, and NAND_CMD_PARAM would work only in x8 mode. commit 64b37b2a63eb2f80b65c7185f0013f8ffc637ae3 Author: Matthieu CASTET <matthieu.castet@parrot.com> AuthorDate: 2012-11-06 Note that nand_scan_ident send command (NAND_CMD_RESET, NAND_CMD_READID, NAND_CMD_PARAM), address and read data The ONFI specificication is not very clear for x16 device if high byte of address should be driven to 0, but according to [1] it should be ok to not drive it during autodetection. [1] 3.3.2. Target Initialization [...] The Read ID and Read Parameter Page commands only use the lower 8-bits of the data bus. The host shall not issue commands that use a word data width on x16 devices until the host determines the device supports a 16-bit data bus width in the parameter page. Thus this patch run nand_scan_ident() with driver configured as x8 device. Once the NAND device is detected, and its ONFI params are read, the driver is re-configured based on device-width as passed by DT bindinig 'nand-bus-width' In-case there is a mis-match between the DT binding 'nand-bus-width' and actual device-width detected during nand_get_flash_type() then probe returns failure. All other low-level callback updates happen after the device detection. Signed-off-by: Pekon Gupta <pekon@ti.com> --- drivers/mtd/nand/omap2.c | 45 +++++++++++++++++++++++++++++++++------------ 1 file changed, 33 insertions(+), 12 deletions(-)