Hi Brian, > From: Brian Norris [mailto:computersforpeace@xxxxxxxxx] > > On 10/22/2013 10:07 PM, Gupta, Pekon wrote: > >> From: Brian Norris [mailto:computersforpeace@xxxxxxxxx] > >> 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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
