| Message ID | 1381816197-20477-5-git-send-email-pekon@ti.com |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Oct 15, 2013 at 11:19:52AM +0530, Pekon Gupta wrote: > Autodetection of NAND device bus-width was added in generic NAND driver as > part of following commit > commit 64b37b2a63eb2f80b65c7185f0013f8ffc637ae3 > Author: Matthieu CASTET <matthieu.castet@parrot.com> > AuthorDate: 2012-11-06 > mtd: nand: add NAND_BUSWIDTH_AUTO to autodetect bus width > This patch enables this feature for OMAP2 NAND driver > > Signed-off-by: Pekon Gupta <pekon@ti.com> > --- > drivers/mtd/nand/omap2.c | 29 ++++++++++++++++------------- > 1 file changed, 16 insertions(+), 13 deletions(-) > > diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c > index 5596368..57a3f51 100644 > --- a/drivers/mtd/nand/omap2.c > +++ b/drivers/mtd/nand/omap2.c [...] > @@ -1904,6 +1903,21 @@ static int omap_nand_probe(struct platform_device *pdev) > nand_chip->chip_delay = 50; > } > > + /* scan for NAND device connected to chip controller */ > + if (nand_scan_ident(mtd, 1, NULL)) { > + err = -ENXIO; > + goto out_release_mem_region; > + } > + if ((nand_chip->options & NAND_BUSWIDTH_16) != > + (pdata->devsize & NAND_BUSWIDTH_16)) { > + pr_err("%s: detected %s device but driver configured for %s\n", > + DRIVER_NAME, > + (nand_chip->options & NAND_BUSWIDTH_16) ? "x16" : "x8", > + (pdata->devsize & NAND_BUSWIDTH_16) ? "x16" : "x8"); I'm not sure on the policy choice here; if you're using BUSWIDTH_AUTO, do you even want to try to configure the buswidth by platform data? You're not really getting the full use out of NAND_BUSWIDTH_AUTO because the platform data has to guess the same buswidth that nand_base.c determines. This is worse than the previous behavior, in which you would try both buswidths if the specified type failed. IOW, what are you trying to achieve with auto-buswidth? Automatic determination of the buswidth or just automatic validation? What do you achieve with platform data's selection of buswidth? Specification of the buswidth or just a hint? Do the goals conflict? Perhaps you can just warn, and not error out if the two selections don't match? Or maybe you really wanted to replace the platform data selection mechanism entirely with auto-buswidth? > + err = -EINVAL; > + goto out_release_mem_region; > + } > + > switch (pdata->xfer_type) { > case NAND_OMAP_PREFETCH_POLLED: > nand_chip->read_buf = omap_read_buf_pref; [...] Brian
Hi Brian, > From: Brian Norris [mailto:computersforpeace@gmail.com] > > On Tue, Oct 15, 2013 at 11:19:52AM +0530, Pekon Gupta wrote: > > Autodetection of NAND device bus-width was added in generic NAND > driver as [...] > > @@ -1904,6 +1903,21 @@ static int omap_nand_probe(struct > platform_device *pdev) > > nand_chip->chip_delay = 50; > > } > > > > + /* scan for NAND device connected to chip controller */ > > + if (nand_scan_ident(mtd, 1, NULL)) { > > + err = -ENXIO; > > + goto out_release_mem_region; > > + } > > + if ((nand_chip->options & NAND_BUSWIDTH_16) != > > + (pdata->devsize & NAND_BUSWIDTH_16)) { > > + pr_err("%s: detected %s device but driver configured for > %s\n", > > + DRIVER_NAME, > > + (nand_chip->options & NAND_BUSWIDTH_16) ? > "x16" : "x8", > > + (pdata->devsize & NAND_BUSWIDTH_16) ? "x16" : > "x8"); > > I'm not sure on the policy choice here; if you're using BUSWIDTH_AUTO, > do you even want to try to configure the buswidth by platform data? > You're not really getting the full use out of NAND_BUSWIDTH_AUTO because > the platform data has to guess the same buswidth that nand_base.c > determines. This is worse than the previous behavior, in which you would > try both buswidths if the specified type failed. > > IOW, what are you trying to achieve with auto-buswidth? Automatic > determination of the buswidth or just automatic validation? What do you > achieve with platform data's selection of buswidth? Specification of the > buswidth or just a hint? Do the goals conflict? Perhaps you can just > warn, and not error out if the two selections don't match? Or maybe you > really wanted to replace the platform data selection mechanism entirely > with auto-buswidth? > This is 'automatic validation' of value set in DT binding "nand-bus-width" 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. - 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". with regards, pekon
On Thu, Oct 17, 2013 at 04:42:23AM +0000, Pekon Gupta wrote: > > From: Brian Norris [mailto:computersforpeace@gmail.com] > > > On Tue, Oct 15, 2013 at 11:19:52AM +0530, Pekon Gupta wrote: > > > Autodetection of NAND device bus-width was added in generic NAND > > driver as > [...] > > > @@ -1904,6 +1903,21 @@ static int omap_nand_probe(struct > > platform_device *pdev) > > > nand_chip->chip_delay = 50; > > > } > > > > > > + /* scan for NAND device connected to chip controller */ > > > + if (nand_scan_ident(mtd, 1, NULL)) { > > > + err = -ENXIO; > > > + goto out_release_mem_region; > > > + } > > > + if ((nand_chip->options & NAND_BUSWIDTH_16) != > > > + (pdata->devsize & NAND_BUSWIDTH_16)) { > > > + pr_err("%s: detected %s device but driver configured for > > %s\n", > > > + DRIVER_NAME, > > > + (nand_chip->options & NAND_BUSWIDTH_16) ? > > "x16" : "x8", > > > + (pdata->devsize & NAND_BUSWIDTH_16) ? "x16" : > > "x8"); > > > > I'm not sure on the policy choice here; if you're using BUSWIDTH_AUTO, > > do you even want to try to configure the buswidth by platform data? > > You're not really getting the full use out of NAND_BUSWIDTH_AUTO because > > the platform data has to guess the same buswidth that nand_base.c > > determines. This is worse than the previous behavior, in which you would > > try both buswidths if the specified type failed. > > > > IOW, what are you trying to achieve with auto-buswidth? Automatic > > determination of the buswidth or just automatic validation? What do you > > achieve with platform data's selection of buswidth? Specification of the > > buswidth or just a hint? Do the goals conflict? Perhaps you can just > > warn, and not error out if the two selections don't match? Or maybe you > > really wanted to replace the platform data selection mechanism entirely > > with auto-buswidth? > > > This is 'automatic validation' of value set in DT binding "nand-bus-width" You mean you intend for the patched driver to simply validate, not configure the buswidth? > 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. 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. Trying to mix both (as your patch currently does) just makes everything worse. Brian
Hi Brain, > From: Brian Norris [mailto:computersforpeace@gmail.com] > > On Thu, Oct 17, 2013 at 04:42:23AM +0000, Pekon Gupta wrote: > > > From: Brian Norris [mailto:computersforpeace@gmail.com] > > > > 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.. > 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. Thus, I would drop this patch for now. And go with (1), 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. (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. with regards, pekon
> > Hi Brain, > Oop sorry .. s/Brain/Brian synonymous though :-) with regards, pekon
On Thu, Oct 17, 2013 at 09:00:27PM +0000, Pekon Gupta wrote: > > From: Brian Norris [mailto:computersforpeace@gmail.com] > > > On Thu, Oct 17, 2013 at 04:42:23AM +0000, Pekon Gupta wrote: > > > > From: Brian Norris [mailto:computersforpeace@gmail.com] > > > > > 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
Hi Brian, > From: Brian Norris [mailto:computersforpeace@gmail.com] > On Thu, Oct 17, 2013 at 09:00:27PM +0000, Pekon Gupta wrote: > > > From: Brian Norris [mailto:computersforpeace@gmail.com] > > > > On Thu, Oct 17, 2013 at 04:42:23AM +0000, Pekon Gupta wrote: [...] > > > 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. > 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'. > > 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). > 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).. > 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. > I have re-posted [PATCH v10 4/10] with this updates. Please accept this ... with regards, pekon
diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c index 5596368..57a3f51 100644 --- a/drivers/mtd/nand/omap2.c +++ b/drivers/mtd/nand/omap2.c @@ -1856,8 +1856,7 @@ 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; + nand_chip->options |= NAND_SKIP_BBTSCAN | NAND_BUSWIDTH_AUTO; #ifdef CONFIG_MTD_NAND_OMAP_BCH info->of_node = pdata->of_node; #endif @@ -1904,6 +1903,21 @@ static int omap_nand_probe(struct platform_device *pdev) nand_chip->chip_delay = 50; } + /* scan for NAND device connected to chip controller */ + if (nand_scan_ident(mtd, 1, NULL)) { + err = -ENXIO; + goto out_release_mem_region; + } + if ((nand_chip->options & NAND_BUSWIDTH_16) != + (pdata->devsize & NAND_BUSWIDTH_16)) { + pr_err("%s: detected %s device but driver configured for %s\n", + DRIVER_NAME, + (nand_chip->options & NAND_BUSWIDTH_16) ? "x16" : "x8", + (pdata->devsize & NAND_BUSWIDTH_16) ? "x16" : "x8"); + err = -EINVAL; + goto out_release_mem_region; + } + switch (pdata->xfer_type) { case NAND_OMAP_PREFETCH_POLLED: nand_chip->read_buf = omap_read_buf_pref; @@ -2011,17 +2025,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) {
Autodetection of NAND device bus-width was added in generic NAND driver as part of following commit commit 64b37b2a63eb2f80b65c7185f0013f8ffc637ae3 Author: Matthieu CASTET <matthieu.castet@parrot.com> AuthorDate: 2012-11-06 mtd: nand: add NAND_BUSWIDTH_AUTO to autodetect bus width This patch enables this feature for OMAP2 NAND driver Signed-off-by: Pekon Gupta <pekon@ti.com> --- drivers/mtd/nand/omap2.c | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-)