| Message ID | 1394197344-9468-2-git-send-email-pekon@ti.com |
|---|---|
| State | Superseded |
| Headers | show |
Hello Pekon, On Mar 07, Pekon Gupta wrote: > ELM hardware engine is used by BCH ecc-schemes for detecting and locating > ECC errors. This patch adds following checks as per ELM design constrains: > s/constrains/constraints > - ELM internal buffers are of 1K, > So it cannot process data with ecc-step-size > 1K. > > - ELM engine can execute upto maximum of 8 threads in parallel, > So in *page-mode* (when complete page is processed in single iteration), > ELM cannot support ecc-steps > 8. > Altough it's just a nitpick, I think you can work a bit on your explanations. For instance, you start with a capital letter after a comma (1K, So it..). And you have some incomplete sentences (This patch adds *the* following). I'm not a native english speaker, so I know this can be hard! It's not a hard requirement for the patch, but rather just a suggestion to improve your upstream work. I have a few comments below. > Signed-off-by: Pekon Gupta <pekon@ti.com> > --- > drivers/mtd/devices/elm.c | 19 ++++++++++++++++++- > drivers/mtd/nand/omap2.c | 9 ++++++--- > include/linux/platform_data/elm.h | 3 ++- > 3 files changed, 26 insertions(+), 5 deletions(-) > > diff --git a/drivers/mtd/devices/elm.c b/drivers/mtd/devices/elm.c > index f160d2c..7fda50f 100644 > --- a/drivers/mtd/devices/elm.c > +++ b/drivers/mtd/devices/elm.c > @@ -84,6 +84,9 @@ struct elm_info { > struct list_head list; > enum bch_ecc bch_type; > struct elm_registers elm_regs; > + int ecc_steps; > + int ecc_step_size; > + int ecc_step_bytes; > }; > > static LIST_HEAD(elm_devices); > @@ -103,7 +106,8 @@ static u32 elm_read_reg(struct elm_info *info, int offset) > * @dev: ELM device > * @bch_type: Type of BCH ecc > */ > -int elm_config(struct device *dev, enum bch_ecc bch_type) > +int elm_config(struct device *dev, enum bch_ecc bch_type, > + int ecc_steps, int ecc_step_size, int ecc_step_bytes) > { > u32 reg_val; > struct elm_info *info = dev_get_drvdata(dev); > @@ -112,10 +116,23 @@ int elm_config(struct device *dev, enum bch_ecc bch_type) > dev_err(dev, "Unable to configure elm - device not probed?\n"); You are using dev_err on this function... > return -ENODEV; > } > + /* ELM cannot detect ECC errors for chunks > 1KB */ > + if (ecc_step_size > (ELM_ECC_SIZE/2 + 1)) { > + pr_err("unsupported config ecc-size=%d", ecc_step_size); ... but then you use pr_err? I think it's better to simply use dev_err() whenever possible as it carries more information. > + return -EINVAL; > + } > + /* ELM support 8 error syndrome process */ > + if (ecc_steps > ERROR_VECTOR_MAX) { > + pr_err("unsupported config ecc-step=%d", ecc_steps); Ditto. > + return -EINVAL; > + } > > reg_val = (bch_type & ECC_BCH_LEVEL_MASK) | (ELM_ECC_SIZE << 16); > elm_write_reg(info, ELM_LOCATION_CONFIG, reg_val); > info->bch_type = bch_type; > + info->ecc_steps = ecc_steps; > + info->ecc_step_size = ecc_step_size; > + info->ecc_step_bytes = ecc_step_bytes; > You're not using this values anywhere but in elm_config, at least in this patch. I'd suggest that you remove the new field introduction here and instead introduce it in the same patch you're using it. Otherwise, you just confuse reviewers :) > return 0; > } > diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c > index 5ce2097..369aee7 100644 > --- a/drivers/mtd/nand/omap2.c > +++ b/drivers/mtd/nand/omap2.c > @@ -1546,6 +1546,8 @@ static int is_elm_present(struct omap_nand_info *info, > struct device_node *elm_node, enum bch_ecc bch_type) > { > struct platform_device *pdev; > + struct nand_ecc_ctrl *ecc = &info->nand.ecc; > + int err = 0; Are you sure you need to initialize err here? Thanks and sorry for reviewing this only now, after six rounds.
Hi Ezequiel, >From: Ezequiel Garcia [mailto:ezequiel.garcia@free-electrons.com] [...] >Altough it's just a nitpick, I think you can work a bit on your explanations. >For instance, you start with a capital letter after a comma (1K, So it..). > >And you have some incomplete sentences (This patch adds *the* following). > >I'm not a native english speaker, so I know this can be hard! It's not a hard >requirement for the patch, but rather just a suggestion to improve your upstream >work. > Thanks much for review on this and other patches. I'll try to be more careful with my English next time. I'll incorporate your feedbacks in next version, But before sending next version, I'll wait for: - if Brian has any other feedbacks on this and previous series. - And acceptance/conclusion on following discussions.. "mtd: nand: omap: optimized chip->ecc.correct() for H/W ECC schemes" "mtd: nand: add erased-page bitflip correction" from Brain Norris with regards, pekon
diff --git a/drivers/mtd/devices/elm.c b/drivers/mtd/devices/elm.c index f160d2c..7fda50f 100644 --- a/drivers/mtd/devices/elm.c +++ b/drivers/mtd/devices/elm.c @@ -84,6 +84,9 @@ struct elm_info { struct list_head list; enum bch_ecc bch_type; struct elm_registers elm_regs; + int ecc_steps; + int ecc_step_size; + int ecc_step_bytes; }; static LIST_HEAD(elm_devices); @@ -103,7 +106,8 @@ static u32 elm_read_reg(struct elm_info *info, int offset) * @dev: ELM device * @bch_type: Type of BCH ecc */ -int elm_config(struct device *dev, enum bch_ecc bch_type) +int elm_config(struct device *dev, enum bch_ecc bch_type, + int ecc_steps, int ecc_step_size, int ecc_step_bytes) { u32 reg_val; struct elm_info *info = dev_get_drvdata(dev); @@ -112,10 +116,23 @@ int elm_config(struct device *dev, enum bch_ecc bch_type) dev_err(dev, "Unable to configure elm - device not probed?\n"); return -ENODEV; } + /* ELM cannot detect ECC errors for chunks > 1KB */ + if (ecc_step_size > (ELM_ECC_SIZE/2 + 1)) { + pr_err("unsupported config ecc-size=%d", ecc_step_size); + return -EINVAL; + } + /* ELM support 8 error syndrome process */ + if (ecc_steps > ERROR_VECTOR_MAX) { + pr_err("unsupported config ecc-step=%d", ecc_steps); + return -EINVAL; + } reg_val = (bch_type & ECC_BCH_LEVEL_MASK) | (ELM_ECC_SIZE << 16); elm_write_reg(info, ELM_LOCATION_CONFIG, reg_val); info->bch_type = bch_type; + info->ecc_steps = ecc_steps; + info->ecc_step_size = ecc_step_size; + info->ecc_step_bytes = ecc_step_bytes; return 0; } diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c index 5ce2097..369aee7 100644 --- a/drivers/mtd/nand/omap2.c +++ b/drivers/mtd/nand/omap2.c @@ -1546,6 +1546,8 @@ static int is_elm_present(struct omap_nand_info *info, struct device_node *elm_node, enum bch_ecc bch_type) { struct platform_device *pdev; + struct nand_ecc_ctrl *ecc = &info->nand.ecc; + int err = 0; /* check whether elm-id is passed via DT */ if (!elm_node) { pr_err("nand: error: ELM DT node not found\n"); @@ -1559,9 +1561,10 @@ static int is_elm_present(struct omap_nand_info *info, } /* ELM module available, now configure it */ info->elm_dev = &pdev->dev; - if (elm_config(info->elm_dev, bch_type)) - return -ENODEV; - return 0; + err = elm_config(info->elm_dev, bch_type, + (info->mtd.writesize / ecc->size), ecc->size, ecc->bytes); + + return err; } #endif /* CONFIG_MTD_NAND_ECC_BCH */ diff --git a/include/linux/platform_data/elm.h b/include/linux/platform_data/elm.h index bf0a83b..b824ff3 100644 --- a/include/linux/platform_data/elm.h +++ b/include/linux/platform_data/elm.h @@ -50,5 +50,6 @@ struct elm_errorvec { void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc, struct elm_errorvec *err_vec); -int elm_config(struct device *dev, enum bch_ecc bch_type); +int elm_config(struct device *dev, enum bch_ecc bch_type, + int ecc_steps, int ecc_step_size, int ecc_step_bytes); #endif /* __ELM_H */
ELM hardware engine is used by BCH ecc-schemes for detecting and locating ECC errors. This patch adds following checks as per ELM design constrains: - ELM internal buffers are of 1K, So it cannot process data with ecc-step-size > 1K. - ELM engine can execute upto maximum of 8 threads in parallel, So in *page-mode* (when complete page is processed in single iteration), ELM cannot support ecc-steps > 8. Signed-off-by: Pekon Gupta <pekon@ti.com> --- drivers/mtd/devices/elm.c | 19 ++++++++++++++++++- drivers/mtd/nand/omap2.c | 9 ++++++--- include/linux/platform_data/elm.h | 3 ++- 3 files changed, 26 insertions(+), 5 deletions(-)