| Message ID | 1854666513.5578.1415473916534.JavaMail.zimbra@xes-inc.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On 11/08/2014 04:11 PM, Aaron Sierra wrote: > From: Jordan Friendshuh <jfriendshuh@xes-inc.com> > > Support the generic nand-ecc-mode and nand-ecc-strength device-tree > properties with the Freescale UPM NAND driver. > > This patch preserves the default software ECC mode while adding the > ability to use BCH ECC for larger NAND devices. > > Signed-off-by: Jordan Friendshuh <jfriendshuh@xes-inc.com> > Signed-off-by: Aaron Sierra <asierra@xes-inc.com> > --- > v2: > * Now using ECC mode and strength helpers from of_mtd.h > * ECC mode and strength checking is more robust > > .../devicetree/bindings/mtd/fsl-upm-nand.txt | 2 + > drivers/mtd/nand/Kconfig | 1 + > drivers/mtd/nand/fsl_upm.c | 51 +++++++++++++++++++--- > 3 files changed, 49 insertions(+), 5 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt b/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt > index fce4894..a9906f6 100644 > --- a/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt > +++ b/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt > @@ -18,6 +18,8 @@ Optional properties: > - chip-delay : chip dependent delay for transferring data from array to > read registers (tR). Required if property "gpios" is not used > (R/B# pins not connected). > +- nand-ecc-mode : as defined by nand.txt ("soft" and "soft_bch", only). > +- nand-ecc-strength : as defined by nand.txt. > The nand.txt recommends that each binding documents the way these properties are interpreted. Moreover, there's no nand-ecc-step usage? Are you assuming a 512-byte size?
----- Original Message ----- > From: "Ezequiel Garcia" <ezequiel@vanguardiasur.com.ar> > Sent: Saturday, November 22, 2014 6:55:42 PM > > On 11/08/2014 04:11 PM, Aaron Sierra wrote: > > From: Jordan Friendshuh <jfriendshuh@xes-inc.com> > > > > Support the generic nand-ecc-mode and nand-ecc-strength device-tree > > properties with the Freescale UPM NAND driver. > > > > This patch preserves the default software ECC mode while adding the > > ability to use BCH ECC for larger NAND devices. > > > > Signed-off-by: Jordan Friendshuh <jfriendshuh@xes-inc.com> > > Signed-off-by: Aaron Sierra <asierra@xes-inc.com> > > --- > > v2: > > * Now using ECC mode and strength helpers from of_mtd.h > > * ECC mode and strength checking is more robust > > > > .../devicetree/bindings/mtd/fsl-upm-nand.txt | 2 + > > drivers/mtd/nand/Kconfig | 1 + > > drivers/mtd/nand/fsl_upm.c | 51 > > +++++++++++++++++++--- > > 3 files changed, 49 insertions(+), 5 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt > > b/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt > > index fce4894..a9906f6 100644 > > --- a/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt > > +++ b/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt > > @@ -18,6 +18,8 @@ Optional properties: > > - chip-delay : chip dependent delay for transferring data from array to > > read registers (tR). Required if property "gpios" is not used > > (R/B# pins not connected). > > +- nand-ecc-mode : as defined by nand.txt ("soft" and "soft_bch", only). > > +- nand-ecc-strength : as defined by nand.txt. > > > > The nand.txt recommends that each binding documents the way these > properties are interpreted. Ezequiel, I assume you are referring to this statement, "implementations are encouraged to further specify the value(s) they support." That is the only statement I found requesting documentation. I believe that request is satisfied, since I listed all of the modes that the driver would support with my patch applied. I used mxc-nand.txt as a reference. > Moreover, there's no nand-ecc-step usage? Are you assuming a 512-byte size? The ecc.bytes value is currently left to be calculated by the NAND_ECC_SOFT_BCH case in nand_base.c. That calculation assumes a 512-byte step size. -Aaron
On 11/08/2014 04:11 PM, Aaron Sierra wrote: [..] > + > + /* We know mode is either NAND_ECC_SOFT or NAND_ECC_SOFT_BCH */ > + if (strength < 0 && mode == NAND_ECC_SOFT_BCH) { > + dev_err(fun->dev, > + "ECC BCH mode requires nand-ecc-strength property"); > + ret = -EINVAL; > + goto err; > + } else if (strength == 0) { > + dev_err(fun->dev, "ECC strength of 0 bits is unsupported"); > + ret = -EINVAL; > + goto err; > + } else if (strength == 1 && mode == NAND_ECC_SOFT_BCH) { > + dev_err(fun->dev, "ECC BCH mode requires > 1-bit strength"); > + ret = -EINVAL; > + goto err; > + } else if (strength > 1 && mode == NAND_ECC_SOFT) { > + dev_warn(fun->dev, > + "Forcing ECC BCH due to %d-bit strength\n", strength); > + mode = NAND_ECC_SOFT_BCH; > + } > + fun->chip.ecc.mode = mode; > + fun->chip.ecc.strength = strength; > + Aside from my comment about the lack of ECC specification in the binding, I think the above is wrong. You don't have hardware ECC, but software ECC (either hamming or BCH). So, you don't need to specify any nand_ecc_ctrl.strength (i.e. ecc.strength above). It'll be set by the NAND core and override any value you set See nand_scan_tail. So, I'd say you just need to specify the nand-ecc-mode in the devicetree binding. The nand-ecc-strength and nand-ecc-step-size are meant for controllers with hardware ECC support.
> From: "Ezequiel Garcia" <ezequiel@vanguardiasur.com.ar> > Sent: Tuesday, November 25, 2014 2:08:59 PM > > On 11/08/2014 04:11 PM, Aaron Sierra wrote: > [..] > > + > > + /* We know mode is either NAND_ECC_SOFT or NAND_ECC_SOFT_BCH */ > > + if (strength < 0 && mode == NAND_ECC_SOFT_BCH) { > > + dev_err(fun->dev, > > + "ECC BCH mode requires nand-ecc-strength property"); > > + ret = -EINVAL; > > + goto err; > > + } else if (strength == 0) { > > + dev_err(fun->dev, "ECC strength of 0 bits is unsupported"); > > + ret = -EINVAL; > > + goto err; > > + } else if (strength == 1 && mode == NAND_ECC_SOFT_BCH) { > > + dev_err(fun->dev, "ECC BCH mode requires > 1-bit strength"); > > + ret = -EINVAL; > > + goto err; > > + } else if (strength > 1 && mode == NAND_ECC_SOFT) { > > + dev_warn(fun->dev, > > + "Forcing ECC BCH due to %d-bit strength\n", strength); > > + mode = NAND_ECC_SOFT_BCH; > > + } > > + fun->chip.ecc.mode = mode; > > + fun->chip.ecc.strength = strength; > > + > > Aside from my comment about the lack of ECC specification in the > binding, I think the above is wrong. > > You don't have hardware ECC, but software ECC (either hamming or BCH). > So, you don't need to specify any nand_ecc_ctrl.strength (i.e. > ecc.strength above). > > It'll be set by the NAND core and override any value you set > See nand_scan_tail. Ezequiel, This patch was originally the second of two patches. The first was applied to l2-mtd.git on 11/5/2014: mtd: nand: Base BCH ECC bytes on required strength It affects the code in nand_scan_tail that you're referring to so that it looks like this: /* * Board driver should supply ecc.size and ecc.bytes values to * select how many bits are correctable; see nand_bch_init() * for details. Otherwise, default to 4 bits for large page * devices. */ if (!ecc->size && (mtd->oobsize >= 64)) { ecc->size = 512; ecc->bytes = DIV_ROUND_UP(13 * ecc->strength, 8); } ecc->priv = nand_bch_init(mtd, ecc->size, ecc->bytes, &ecc->layout); if (!ecc->priv) { pr_warn("BCH ECC initialization failed!\n"); BUG(); } ecc->strength = ecc->bytes * 8 / fls(8 * ecc->size); In this case ecc->strength is unnecessarily recalculated at the end and ecc->strength isn't checked for zero in the initial calculation. Perhaps this block in nand_scan_tail should be patched again to preserve the original behaviour if ecc->strength or ecc->size are zero. The only other in-kernel user of SOFT_BCH is sunxi_nand.c and it calculates ecc->bytes from ecc->size and ecc->strength itself, so it has defined all three values by this point. > So, I'd say you just need to specify the nand-ecc-mode in the devicetree > binding. > > The nand-ecc-strength and nand-ecc-step-size are meant for controllers > with hardware ECC support. Software BCH allows controllers without hardware support for multiple bit correction to be used with NAND devices that require multiple bit correction, so like a hardware controller it needs to know how many bits to correct. > -- > Ezequiel Garcia, VanguardiaSur > www.vanguardiasur.com.ar >
On 11/25/2014 07:19 PM, Aaron Sierra wrote: >> From: "Ezequiel Garcia" <ezequiel@vanguardiasur.com.ar> >> Sent: Tuesday, November 25, 2014 2:08:59 PM >> >> On 11/08/2014 04:11 PM, Aaron Sierra wrote: >> [..] >>> + >>> + /* We know mode is either NAND_ECC_SOFT or NAND_ECC_SOFT_BCH */ >>> + if (strength < 0 && mode == NAND_ECC_SOFT_BCH) { >>> + dev_err(fun->dev, >>> + "ECC BCH mode requires nand-ecc-strength property"); >>> + ret = -EINVAL; >>> + goto err; >>> + } else if (strength == 0) { >>> + dev_err(fun->dev, "ECC strength of 0 bits is unsupported"); >>> + ret = -EINVAL; >>> + goto err; >>> + } else if (strength == 1 && mode == NAND_ECC_SOFT_BCH) { >>> + dev_err(fun->dev, "ECC BCH mode requires > 1-bit strength"); >>> + ret = -EINVAL; >>> + goto err; >>> + } else if (strength > 1 && mode == NAND_ECC_SOFT) { >>> + dev_warn(fun->dev, >>> + "Forcing ECC BCH due to %d-bit strength\n", strength); >>> + mode = NAND_ECC_SOFT_BCH; >>> + } >>> + fun->chip.ecc.mode = mode; >>> + fun->chip.ecc.strength = strength; >>> + >> >> Aside from my comment about the lack of ECC specification in the >> binding, I think the above is wrong. >> >> You don't have hardware ECC, but software ECC (either hamming or BCH). >> So, you don't need to specify any nand_ecc_ctrl.strength (i.e. >> ecc.strength above). >> >> It'll be set by the NAND core and override any value you set >> See nand_scan_tail. > > Ezequiel, > This patch was originally the second of two patches. The first was applied > to l2-mtd.git on 11/5/2014: > > mtd: nand: Base BCH ECC bytes on required strength > > It affects the code in nand_scan_tail that you're referring to so that it > looks like this: > > /* > * Board driver should supply ecc.size and ecc.bytes values to > * select how many bits are correctable; see nand_bch_init() > * for details. Otherwise, default to 4 bits for large page > * devices. > */ > if (!ecc->size && (mtd->oobsize >= 64)) { > ecc->size = 512; > ecc->bytes = DIV_ROUND_UP(13 * ecc->strength, 8); > } > ecc->priv = nand_bch_init(mtd, ecc->size, ecc->bytes, > &ecc->layout); > if (!ecc->priv) { > pr_warn("BCH ECC initialization failed!\n"); > BUG(); > } > ecc->strength = ecc->bytes * 8 / fls(8 * ecc->size); > > In this case ecc->strength is unnecessarily recalculated at the end and > ecc->strength isn't checked for zero in the initial calculation. Perhaps > this block in nand_scan_tail should be patched again to preserve the > original behaviour if ecc->strength or ecc->size are zero. > Exactly, I was talking about that snippet. I missed the fact that ecc.strength is used to get bytes, and then re-calculated. Some comments are definitely needed there. > The only other in-kernel user of SOFT_BCH is sunxi_nand.c and it calculates > ecc->bytes from ecc->size and ecc->strength itself, so it has defined all > three values by this point. > >> So, I'd say you just need to specify the nand-ecc-mode in the devicetree >> binding. >> >> The nand-ecc-strength and nand-ecc-step-size are meant for controllers >> with hardware ECC support. > > Software BCH allows controllers without hardware support for multiple > bit correction to be used with NAND devices that require multiple bit > correction, so like a hardware controller it needs to know how many > bits to correct. > Right, right. I'd say you should require an ECC step size in your binding, instead of assuming the 512-byte that gets set if the ecc.size is 0. BTW, how does this patch deal with old devicetree files?
----- Original Message ----- > From: "Ezequiel Garcia" <ezequiel@vanguardiasur.com.ar> > Sent: Tuesday, November 25, 2014 4:42:47 PM > > On 11/25/2014 07:19 PM, Aaron Sierra wrote: > >> From: "Ezequiel Garcia" <ezequiel@vanguardiasur.com.ar> > >> Sent: Tuesday, November 25, 2014 2:08:59 PM > >> > >> On 11/08/2014 04:11 PM, Aaron Sierra wrote: > >> [..] > >>> + > >>> + /* We know mode is either NAND_ECC_SOFT or NAND_ECC_SOFT_BCH */ > >>> + if (strength < 0 && mode == NAND_ECC_SOFT_BCH) { > >>> + dev_err(fun->dev, > >>> + "ECC BCH mode requires nand-ecc-strength property"); > >>> + ret = -EINVAL; > >>> + goto err; > >>> + } else if (strength == 0) { > >>> + dev_err(fun->dev, "ECC strength of 0 bits is unsupported"); > >>> + ret = -EINVAL; > >>> + goto err; > >>> + } else if (strength == 1 && mode == NAND_ECC_SOFT_BCH) { > >>> + dev_err(fun->dev, "ECC BCH mode requires > 1-bit strength"); > >>> + ret = -EINVAL; > >>> + goto err; > >>> + } else if (strength > 1 && mode == NAND_ECC_SOFT) { > >>> + dev_warn(fun->dev, > >>> + "Forcing ECC BCH due to %d-bit strength\n", strength); > >>> + mode = NAND_ECC_SOFT_BCH; > >>> + } > >>> + fun->chip.ecc.mode = mode; > >>> + fun->chip.ecc.strength = strength; > >>> + > >> > [ snip ] > I'd say you should require an ECC step size in your binding, instead of > assuming the 512-byte that gets set if the ecc.size is 0. Yes, I agree that the fsl_upm driver should require that nand-ecc-step-size and nand-ecc-strength both be defined with "soft_bch". > BTW, how does this patch deal with old devicetree files? The fsl_upm driver previously only supported 1-bit "soft" ECC, so _this_ patch expects that existing device trees don't define nand-ecc-strength or nand-ecc-mode. In that case "soft" ECC will be detected and used. However, if for some reason an existing device tree defines nand-ecc-strength, then the snippet that you provided above shows that the ECC mode would be overridden (forcing "soft_bch" for ECC strength values greater than 1). -Aaron
Hi Aaron, On Sat, Nov 08, 2014 at 01:11:56PM -0600, Aaron Sierra wrote: > From: Jordan Friendshuh <jfriendshuh@xes-inc.com> > > Support the generic nand-ecc-mode and nand-ecc-strength device-tree > properties with the Freescale UPM NAND driver. > > This patch preserves the default software ECC mode while adding the > ability to use BCH ECC for larger NAND devices. > > Signed-off-by: Jordan Friendshuh <jfriendshuh@xes-inc.com> > Signed-off-by: Aaron Sierra <asierra@xes-inc.com> > --- > v2: > * Now using ECC mode and strength helpers from of_mtd.h > * ECC mode and strength checking is more robust Should I be expecting an update? Ezequiel made a few comments, and I'm not sure if you were planning any action based on them. Brian
----- Original Message ----- > From: "Brian Norris" <computersforpeace@gmail.com> > Sent: Tuesday, December 16, 2014 6:35:06 PM > > Hi Aaron, > > On Sat, Nov 08, 2014 at 01:11:56PM -0600, Aaron Sierra wrote: > > From: Jordan Friendshuh <jfriendshuh@xes-inc.com> > > > > Support the generic nand-ecc-mode and nand-ecc-strength device-tree > > properties with the Freescale UPM NAND driver. > > > > This patch preserves the default software ECC mode while adding the > > ability to use BCH ECC for larger NAND devices. > > > > Signed-off-by: Jordan Friendshuh <jfriendshuh@xes-inc.com> > > Signed-off-by: Aaron Sierra <asierra@xes-inc.com> > > --- > > v2: > > * Now using ECC mode and strength helpers from of_mtd.h > > * ECC mode and strength checking is more robust > > Should I be expecting an update? Ezequiel made a few comments, and I'm > not sure if you were planning any action based on them. Brian, Yes, I intend to provide an updated patch based on Ezequiel's comments. Sorry for not being clear about this earlier. I thought I would have done the work by now. -Aaron
diff --git a/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt b/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt index fce4894..a9906f6 100644 --- a/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt +++ b/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt @@ -18,6 +18,8 @@ Optional properties: - chip-delay : chip dependent delay for transferring data from array to read registers (tR). Required if property "gpios" is not used (R/B# pins not connected). +- nand-ecc-mode : as defined by nand.txt ("soft" and "soft_bch", only). +- nand-ecc-strength : as defined by nand.txt. Each flash chip described may optionally contain additional sub-nodes describing partitions of the address space. See partition.txt for more diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig index f1cf503..85c0243 100644 --- a/drivers/mtd/nand/Kconfig +++ b/drivers/mtd/nand/Kconfig @@ -439,6 +439,7 @@ config MTD_NAND_FSL_UPM tristate "Support for NAND on Freescale UPM" depends on PPC_83xx || PPC_85xx select FSL_LBC + select MTD_NAND_ECC_BCH help Enables support for NAND Flash chips wired onto Freescale PowerPC processor localbus with User-Programmable Machine support. diff --git a/drivers/mtd/nand/fsl_upm.c b/drivers/mtd/nand/fsl_upm.c index 4d203e8..8f38447 100644 --- a/drivers/mtd/nand/fsl_upm.c +++ b/drivers/mtd/nand/fsl_upm.c @@ -18,6 +18,7 @@ #include <linux/mtd/nand_ecc.h> #include <linux/mtd/partitions.h> #include <linux/mtd/mtd.h> +#include <linux/of_mtd.h> #include <linux/of_address.h> #include <linux/of_platform.h> #include <linux/of_gpio.h> @@ -160,6 +161,11 @@ static int fun_chip_init(struct fsl_upm_nand *fun, int ret; struct device_node *flash_np; struct mtd_part_parser_data ppdata; + int mode, strength; + + flash_np = of_get_next_child(upm_np, NULL); + if (!flash_np) + return -ENODEV; fun->chip.IO_ADDR_R = fun->io_base; fun->chip.IO_ADDR_W = fun->io_base; @@ -168,7 +174,46 @@ static int fun_chip_init(struct fsl_upm_nand *fun, fun->chip.read_byte = fun_read_byte; fun->chip.read_buf = fun_read_buf; fun->chip.write_buf = fun_write_buf; - fun->chip.ecc.mode = NAND_ECC_SOFT; + + /* + * If nand-ecc-strength < 0, require ECC_SOFT, error otherwise. + * If nand-ecc-strength == 0, error. + * If nand-ecc-strength == 1, require ECC_SOFT, error otherwise. + * If nand-ecc-strength > 1, force ECC_SOFT_BCH (warn on mode mismatch). + */ + mode = of_get_nand_ecc_mode(flash_np); + strength = of_get_nand_ecc_strength(flash_np); + if (!of_property_read_bool(flash_np, "nand-ecc-mode")) { + dev_info(fun->dev, "ECC mode defaulting to 'soft'"); + mode = NAND_ECC_SOFT; + } else if (mode != NAND_ECC_SOFT && mode != NAND_ECC_SOFT_BCH) { + dev_err(fun->dev, "ECC mode in device tree is unsupported"); + ret = -EINVAL; + goto err; + } + + /* We know mode is either NAND_ECC_SOFT or NAND_ECC_SOFT_BCH */ + if (strength < 0 && mode == NAND_ECC_SOFT_BCH) { + dev_err(fun->dev, + "ECC BCH mode requires nand-ecc-strength property"); + ret = -EINVAL; + goto err; + } else if (strength == 0) { + dev_err(fun->dev, "ECC strength of 0 bits is unsupported"); + ret = -EINVAL; + goto err; + } else if (strength == 1 && mode == NAND_ECC_SOFT_BCH) { + dev_err(fun->dev, "ECC BCH mode requires > 1-bit strength"); + ret = -EINVAL; + goto err; + } else if (strength > 1 && mode == NAND_ECC_SOFT) { + dev_warn(fun->dev, + "Forcing ECC BCH due to %d-bit strength\n", strength); + mode = NAND_ECC_SOFT_BCH; + } + fun->chip.ecc.mode = mode; + fun->chip.ecc.strength = strength; + if (fun->mchip_count > 1) fun->chip.select_chip = fun_select_chip; @@ -178,10 +223,6 @@ static int fun_chip_init(struct fsl_upm_nand *fun, fun->mtd.priv = &fun->chip; fun->mtd.owner = THIS_MODULE; - flash_np = of_get_next_child(upm_np, NULL); - if (!flash_np) - return -ENODEV; - fun->mtd.name = kasprintf(GFP_KERNEL, "0x%llx.%s", (u64)io_res->start, flash_np->name); if (!fun->mtd.name) {