RE: [PATCH v9 4/9] mtd: nand: omap: enable auto-detection of bus-width for omap-nand drivers

2013-10-19 Thread Gupta, Pekon 
Hi Brian,

 From: Brian Norris [mailto:computersforpe...@gmail.com]
 On Thu, Oct 17, 2013 at 09:00:27PM +, Pekon Gupta wrote:
   From: Brian Norris [mailto:computersforpe...@gmail.com]
On Thu, Oct 17, 2013 at 04:42:23AM +, 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
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 4/9] mtd: nand: omap: enable auto-detection of bus-width for omap-nand drivers

2013-10-17 Thread Brian Norris 
On Thu, Oct 17, 2013 at 04:42:23AM +, Pekon Gupta wrote:
  From: Brian Norris [mailto:computersforpe...@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
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v9 4/9] mtd: nand: omap: enable auto-detection of bus-width for omap-nand drivers

2013-10-17 Thread Gupta, Pekon 
Hi Brain,

 From: Brian Norris [mailto:computersforpe...@gmail.com]
  On Thu, Oct 17, 2013 at 04:42:23AM +, Pekon Gupta wrote:
   From: Brian Norris [mailto:computersforpe...@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
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v9 4/9] mtd: nand: omap: enable auto-detection of bus-width for omap-nand drivers

2013-10-17 Thread Gupta, Pekon 
 
 Hi Brain,
 
Oop sorry .. 
s/Brain/Brian

synonymous though :-)


with regards, pekon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 4/9] mtd: nand: omap: enable auto-detection of bus-width for omap-nand drivers

2013-10-17 Thread Brian Norris 
On Thu, Oct 17, 2013 at 09:00:27PM +, Pekon Gupta wrote:
  From: Brian Norris [mailto:computersforpe...@gmail.com]
   On Thu, Oct 17, 2013 at 04:42:23AM +, Pekon Gupta wrote:
From: Brian Norris [mailto:computersforpe...@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
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 4/9] mtd: nand: omap: enable auto-detection of bus-width for omap-nand drivers

2013-10-16 Thread Brian Norris 
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.cas...@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 pe...@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
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v9 4/9] mtd: nand: omap: enable auto-detection of bus-width for omap-nand drivers

2013-10-16 Thread Gupta, Pekon 
Hi Brian,

 From: Brian Norris [mailto:computersforpe...@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
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html