On Thu, Oct 17, 2013 at 09:00:27PM +0000, Pekon Gupta wrote: > > From: Brian Norris [mailto:computersforpeace@xxxxxxxxx] > > > On Thu, Oct 17, 2013 at 04:42:23AM +0000, Pekon Gupta wrote: > > > > From: Brian Norris [mailto:computersforpeace@xxxxxxxxx] > > > > > On Tue, Oct 15, 2013 at 11:19:52AM +0530, Pekon Gupta wrote: > [snip] > > > So this approach is other way round, where controller is configured > > > based on DT binding "nand-bus-width" and then it checks whether > > > device actual buswidth matches DT binding value or not. > > > > If this is the case, then you really don't want to use > > NAND_BUSWIDTH_AUTO. The default nand_base.c just *validates* the > > pre-selected buswidth and exits with an error, in much the same way that > > you're doing here (you're just duplicating the functionality). > > NAND_BUSWIDTH_AUTO is useful if you want to turn control of the > > buswidth > > configuration entirely over to nand_base.c. > > > > > - GPMC controller is pre-configured to support "x8" or "x16" device based > > > on DT binding "nand-bus-width". > > > Reference: $LINUX/arch/arm/mach-omap2/gpmc.c > > > @@gpmc_probe_nand_child() > > > val = of_get_nand_bus_width(child); > > > > > > - But, bus-width of NAND device is only known during NAND driver > > > Probe in nand_scan_ident(). > > > > > > Going forward when all NAND devices are compliant to ONFI, > > > then we can deprecate "nand-bus-width". > > > > Buswidth auto-detection is not actually restricted to ONFI. nand_base.c > > has (corretly, AFAIK) been detecting buswidths for a long time, using > > some form of ID decode detection. But it was just that: detection. It > > did not actually configure the buswidth, since it left that > > responsibility to the low-level driver to get right. > > > > Another thing to consider: I think this current patch is a regression > > from previous behavior. Previously, you would run nand_scan_ident() > > twice if the first one failed; once with the platform-configured > > buswidth and once with the opposite. But now, you only run > > nand_scan_ident() once, and then you quit if it detects differently than > > you expected. > > > Actually having nand_scan_ident() called again if first time it fails, is > _not_ the right way, it was a quick-fix work-around. > It should not have been approved.. OK, fair enough. I agree. > > My opinion: the platform- and device-tree-provided buswidth is > > unnecessary; it was previouisly only a suggestion which your driver > > would readily discard, and it isn't really needed now. You can probably > > get by with just NAND_BUSWIDTH_AUTO and (at most) a warning if the > > auto-configured buswidth is different than the platform specified. > > > > But the real point: you need to clearly communicate what you are > > choosing in this patch. Either you want to > > > > (1) strictly follow the buswidth provided by the platform or > > > > (2) use the nand_base.c BUSWIDTH_AUTO automatic configuration. > > > Ideally, I would like to go with (2), but that would need other changes > in-order to re-configure GPMC with newly parsed ONFI data, which > can be a separate patch-set. What exactly needs changed to support this? It looks like this omap2 NAND driver doesn't make assumptions about 8-bit vs. 16-bit until after nand_scan_ident(). But maybe there is something I missed. > Thus, I would drop this patch for now. And go with (1), OK, but to drop this patch entirely would still not be (1); it would still leave the possibility of calling nand_scan_ident() twice, and it puts you in a middle ground between (1) and (2). That's fine if it works for you, but I just want to acknowledge that now: this driver requires changes to become either (1) or (2). Does your series need a rebase if we're removing this patch? Or you're rewriting/simplifying it to the following two points? (Perhaps it's best to separate this portion to its own patch (set) and discussion, since it is independent of your ECC rewrite.) > with following > updates in omap_nand_probe(). > > (a) perform nand_scan_ident() in "x8" mode, so that ONFI params are > read and device-info (chip->writesize, chip->oobsize) are populated. OK. > (b) Then switch to "x8" or "x16" mode based on "nand-bus-width" > as passed from GPMC driver to NAND driver via platform-data. > And re-populate mtd->byte, mtd-read_buf, mtd->write_buf. I'm not sure if switching buswidth after nand_scan_ident() is really supported, but I'll hold off until I see the code you're proposing. Brian -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
