| Message ID | 288216793.189179.1438710774869.JavaMail.zimbra@xes-inc.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Tue, Aug 04, 2015 at 12:52:54PM -0500, Aaron Sierra wrote: > This patch preserves the default software ECC mode while adding the > ability to use BCH ECC (as required for larger NAND devices). > > The BCH-required strength and step size values are pulled from the > device-tree by nand_scan_ident(), so we replace nand_scan() with > explicit calls to nand_scan_ident() and nand_scan_tail() in order > to sanity check ECC properties from the device-tree. > > Tested-by: Ryan Schaefer <rschaefer@xes-inc.com> > Signed-off-by: Jordan Friendshuh <jfriendshuh@xes-inc.com> > Signed-off-by: Aaron Sierra <asierra@xes-inc.com> > --- > .../devicetree/bindings/mtd/fsl-upm-nand.txt | 32 ++++++++++++++++++++ > drivers/mtd/nand/fsl_upm.c | 35 +++++++++++++++++++++- > 2 files changed, 66 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt b/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt > index fce4894..3643ee1 100644 > --- a/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt > +++ b/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt > @@ -18,6 +18,9 @@ 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. > +- nand-ecc-step-size : as defined by nand.txt. These three properties go under the flash node (which is not yet documented, though it's mentioned in example and implementation), not the controller node. Can you add a separate paragraph/section to describe the flash node, and put these properties under that? > > Each flash chip described may optionally contain additional sub-nodes > describing partitions of the address space. See partition.txt for more > @@ -65,3 +68,32 @@ upm@3,0 { > }; > }; > }; > + > +/* > + * Micron MT29F32G08AFABA (M62B) > + * 32 Gb (4 GiB), 2 chipselect > + */ > +upm@2,0 { > + #address-cells = <0>; > + #size-cells = <0>; > + compatible = "fsl,upm-nand"; > + reg = <2 0x0 0x80000>; > + fsl,upm-addr-line-cs-offsets = <0x0 0x10000>; > + fsl,upm-addr-offset = <0x10>; > + fsl,upm-cmd-offset = <0x08>; > + fsl,upm-wait-flags = <0x1>; > + chip-delay = <50>; > + > + nand@0 { > + #address-cells = <1>; > + #size-cells = <2>; > + nand-ecc-mode = "soft_bch"; > + nand-ecc-strength = <4>; > + nand-ecc-step-size = <512>; > + > + partition@0 { > + label = "NAND filesystem"; > + reg = <0x0 0x1 0x00000000>; > + }; > + }; > +}; > diff --git a/drivers/mtd/nand/fsl_upm.c b/drivers/mtd/nand/fsl_upm.c > index 72755d7..0982d7a 100644 > --- a/drivers/mtd/nand/fsl_upm.c > +++ b/drivers/mtd/nand/fsl_upm.c > @@ -182,6 +182,7 @@ static int fun_chip_init(struct fsl_upm_nand *fun, > if (!flash_np) > return -ENODEV; > > + fun->chip.dn = flash_np; > fun->mtd.name = kasprintf(GFP_KERNEL, "0x%llx.%s", (u64)io_res->start, > flash_np->name); > if (!fun->mtd.name) { > @@ -189,7 +190,39 @@ static int fun_chip_init(struct fsl_upm_nand *fun, > goto err; > } > > - ret = nand_scan(&fun->mtd, fun->mchip_count); > + ret = nand_scan_ident(&fun->mtd, fun->mchip_count, NULL); > + if (ret) > + goto err; > + > + switch (fun->chip.ecc.mode) { > + case NAND_ECC_NONE: Maybe a comment here, to say that we default to soft for legacy reasons? "NONE" is actually a potentially valid option (but not likely or useful). > + fun->chip.ecc.mode = NAND_ECC_SOFT; > + break; > + case NAND_ECC_SOFT: > + if (fun->chip.ecc.strength && fun->chip.ecc.strength != 1) > + dev_warn(fun->dev, "Ignoring %d-bit software ECC\n", > + fun->chip.ecc.strength); Do you really want to ignore this? I think it's fair to make this an error case in nand_scan_tail(). Like: case NAND_ECC_SOFT: ... if (chip->ecc.strength && chip->ecc.strength != 1) { // error out // we really need to kill the heavy usage of // BUG() in nand_scan_tail()... } > + if (fun->chip.ecc.size && > + (fun->chip.ecc.size != 256) && > + (fun->chip.ecc.size != 512)) { > + dev_err(fun->dev, "Invalid software ECC step: %d\n", > + fun->chip.ecc.size); Is that a generic soft ECC limitation? (Looks like it only supports 256 and 512 to me.) If so, then let's put this in nand_base.c. Either in nand_dt_init(), or possibly in nand_scan_tail(), under the case for NAND_ECC_SOFT. > + goto err; > + } > + break; > + case NAND_ECC_SOFT_BCH: > + if (fun->chip.ecc.strength < 2) { > + dev_err(fun->dev, "Invalid BCH ECC strength: %d\n", > + fun->chip.ecc.strength); Same here. Maybe in nand_scan_tail()? > + goto err; > + } > + break; > + default: > + dev_err(fun->dev, "ECC mode unsupported"); > + goto err; > + } > + > + ret = nand_scan_tail(&fun->mtd); > if (ret) > goto err; > Brian
----- Original Message ----- > From: "Brian Norris" <computersforpeace@gmail.com> > Sent: Tuesday, August 25, 2015 4:34:08 PM [snip] > > --- a/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt > > +++ b/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt > > @@ -18,6 +18,9 @@ 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. > > +- nand-ecc-step-size : as defined by nand.txt. > > These three properties go under the flash node (which is not yet > documented, though it's mentioned in example and implementation), not > the controller node. Can you add a separate paragraph/section to > describe the flash node, and put these properties under that? Sure thing. [snip] > > --- a/drivers/mtd/nand/fsl_upm.c > > +++ b/drivers/mtd/nand/fsl_upm.c > > @@ -182,6 +182,7 @@ static int fun_chip_init(struct fsl_upm_nand *fun, > > if (!flash_np) > > return -ENODEV; > > > > + fun->chip.dn = flash_np; > > fun->mtd.name = kasprintf(GFP_KERNEL, "0x%llx.%s", (u64)io_res->start, > > flash_np->name); > > if (!fun->mtd.name) { > > @@ -189,7 +190,39 @@ static int fun_chip_init(struct fsl_upm_nand *fun, > > goto err; > > } > > > > - ret = nand_scan(&fun->mtd, fun->mchip_count); > > + ret = nand_scan_ident(&fun->mtd, fun->mchip_count, NULL); > > + if (ret) > > + goto err; > > + > > + switch (fun->chip.ecc.mode) { > > + case NAND_ECC_NONE: > > Maybe a comment here, to say that we default to soft for legacy reasons? > "NONE" is actually a potentially valid option (but not likely or > useful). OK > > + fun->chip.ecc.mode = NAND_ECC_SOFT; > > + break; > > + case NAND_ECC_SOFT: > > + if (fun->chip.ecc.strength && fun->chip.ecc.strength != 1) > > + dev_warn(fun->dev, "Ignoring %d-bit software ECC\n", > > + fun->chip.ecc.strength); > > Do you really want to ignore this? I think it's fair to make this an > error case in nand_scan_tail(). Like: > > case NAND_ECC_SOFT: > ... > if (chip->ecc.strength && chip->ecc.strength != 1) { > // error out > // we really need to kill the heavy usage of > // BUG() in nand_scan_tail()... > } My rationale was that for "soft" ECC mode, ecc.strength is really a don't-care property; the driver doesn't use it since the algorithm only supports 1-bit. Therefore it could be OK to output a warning and carry on. I can see the other side, too, where we might want all of the user-supplied values to agree with the capabilities of the driver. If I rework this, I would intend to accept zeros for ecc.strength and ecc.size as valid for this mode (size defaults to 256 within nand_ecc.c). Does that sit well with you? > > > + if (fun->chip.ecc.size && > > + (fun->chip.ecc.size != 256) && > > + (fun->chip.ecc.size != 512)) { > > + dev_err(fun->dev, "Invalid software ECC step: %d\n", > > + fun->chip.ecc.size); > > Is that a generic soft ECC limitation? (Looks like it only supports 256 > and 512 to me.) If so, then let's put this in nand_base.c. Either in > nand_dt_init(), or possibly in nand_scan_tail(), under the case for > NAND_ECC_SOFT. Yes, it is. OK. > > + goto err; > > + } > > + break; > > + case NAND_ECC_SOFT_BCH: > > + if (fun->chip.ecc.strength < 2) { > > + dev_err(fun->dev, "Invalid BCH ECC strength: %d\n", > > + fun->chip.ecc.strength); > > Same here. Maybe in nand_scan_tail()? OK -Aaron
On Wed, Aug 26, 2015 at 11:22:18AM -0500, Aaron Sierra wrote: > ----- Original Message ----- > > From: "Brian Norris" <computersforpeace@gmail.com> > > Sent: Tuesday, August 25, 2015 4:34:08 PM > > > + case NAND_ECC_SOFT: > > > + if (fun->chip.ecc.strength && fun->chip.ecc.strength != 1) > > > + dev_warn(fun->dev, "Ignoring %d-bit software ECC\n", > > > + fun->chip.ecc.strength); > > > > Do you really want to ignore this? I think it's fair to make this an > > error case in nand_scan_tail(). Like: > > > > case NAND_ECC_SOFT: > > ... > > if (chip->ecc.strength && chip->ecc.strength != 1) { > > // error out > > // we really need to kill the heavy usage of > > // BUG() in nand_scan_tail()... > > } > > My rationale was that for "soft" ECC mode, ecc.strength is really a > don't-care property; the driver doesn't use it since the algorithm only > supports 1-bit. Therefore it could be OK to output a warning and carry on. Understood. > I can see the other side, too, where we might want all of the > user-supplied values to agree with the capabilities of the driver. Right. I especially would want to keep sanity on the device tree, as this is something closer to an ABI (depending on who you ask...) than internal driver-provided values. > If I rework this, I would intend to accept zeros for ecc.strength and > ecc.size as valid for this mode (size defaults to 256 within nand_ecc.c). > Does that sit well with you? Sure, that's fair. We've been allowing this in nand_scan_tail() already (we tend to fill in reasonable defaults if the driver doesn't specify), so I don't want to remove that fallback. I just don't want people specifying 4-bit hamming ECC in their DT and not noticing your little warning log message saying that we're ignoring them and using 1-bit. But the DT binding doc should not suggest that they can be left at 0. If you're up for it, perhaps you can add some details in Documentation/devicetree/bindings/mtd/nand.txt about the "soft" and "soft_bch" ECC modes while you're at it. Thanks, Brian
diff --git a/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt b/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt index fce4894..3643ee1 100644 --- a/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt +++ b/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt @@ -18,6 +18,9 @@ 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. +- nand-ecc-step-size : 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 @@ -65,3 +68,32 @@ upm@3,0 { }; }; }; + +/* + * Micron MT29F32G08AFABA (M62B) + * 32 Gb (4 GiB), 2 chipselect + */ +upm@2,0 { + #address-cells = <0>; + #size-cells = <0>; + compatible = "fsl,upm-nand"; + reg = <2 0x0 0x80000>; + fsl,upm-addr-line-cs-offsets = <0x0 0x10000>; + fsl,upm-addr-offset = <0x10>; + fsl,upm-cmd-offset = <0x08>; + fsl,upm-wait-flags = <0x1>; + chip-delay = <50>; + + nand@0 { + #address-cells = <1>; + #size-cells = <2>; + nand-ecc-mode = "soft_bch"; + nand-ecc-strength = <4>; + nand-ecc-step-size = <512>; + + partition@0 { + label = "NAND filesystem"; + reg = <0x0 0x1 0x00000000>; + }; + }; +}; diff --git a/drivers/mtd/nand/fsl_upm.c b/drivers/mtd/nand/fsl_upm.c index 72755d7..0982d7a 100644 --- a/drivers/mtd/nand/fsl_upm.c +++ b/drivers/mtd/nand/fsl_upm.c @@ -182,6 +182,7 @@ static int fun_chip_init(struct fsl_upm_nand *fun, if (!flash_np) return -ENODEV; + fun->chip.dn = flash_np; fun->mtd.name = kasprintf(GFP_KERNEL, "0x%llx.%s", (u64)io_res->start, flash_np->name); if (!fun->mtd.name) { @@ -189,7 +190,39 @@ static int fun_chip_init(struct fsl_upm_nand *fun, goto err; } - ret = nand_scan(&fun->mtd, fun->mchip_count); + ret = nand_scan_ident(&fun->mtd, fun->mchip_count, NULL); + if (ret) + goto err; + + switch (fun->chip.ecc.mode) { + case NAND_ECC_NONE: + fun->chip.ecc.mode = NAND_ECC_SOFT; + break; + case NAND_ECC_SOFT: + if (fun->chip.ecc.strength && fun->chip.ecc.strength != 1) + dev_warn(fun->dev, "Ignoring %d-bit software ECC\n", + fun->chip.ecc.strength); + if (fun->chip.ecc.size && + (fun->chip.ecc.size != 256) && + (fun->chip.ecc.size != 512)) { + dev_err(fun->dev, "Invalid software ECC step: %d\n", + fun->chip.ecc.size); + goto err; + } + break; + case NAND_ECC_SOFT_BCH: + if (fun->chip.ecc.strength < 2) { + dev_err(fun->dev, "Invalid BCH ECC strength: %d\n", + fun->chip.ecc.strength); + goto err; + } + break; + default: + dev_err(fun->dev, "ECC mode unsupported"); + goto err; + } + + ret = nand_scan_tail(&fun->mtd); if (ret) goto err;