Re: [RFC PATCH] mtd: add per NAND partition ECC config
From: Boris BREZILLON
Date: Mon Feb 10 2014 - 07:28:30 EST
On 10/02/2014 12:55, Gupta, Pekon wrote:
Hi Boris,
From: Boris BREZILLON
This patch aims to add per partition ECC config for NAND devices.
It defines a new field in the mtd struct to store the mtd ECC config and
thus each mtd partition device can store its config instead of using the
default NAND chip config.
This feature is needed to support the sunxi boot0 paritition case:
Allwinner boot code (BROM) requires a specific HW ECC for its boot code
that may not fit the HW NAND requirements for the entire NAND chip.
Signed-off-by: Boris BREZILLON <b.brezillon.dev@xxxxxxxxx>
---
Hello,
This patch is just a draft that implement per partition ECC config.
It's currently not properly splitted (it should be separated in several
patches) and not documented either.
There's at least one point that bother me in the current implementation:
I introduced DT notions in the nand core code by the mean of the get_ecc_ctrl
callback, and so far this was kept out of mtd/nand core code (I guess it was
on purpose).
Please let me know if you see other drawbacks.
If you think per partition ECC should not be implemented, could you help me
find a way to handle sunxi specific case decribed above ?
There was a discussion on having per partition based ECC earlier [1].
Though I was not in favor of supporting it earlier, but your proposal looks good.
However, I still feel there are more caveats here, to explore..
[...]
@@ -364,7 +367,13 @@ static struct mtd_part *allocate_partition(struct mtd_info *master,
slave->mtd.writesize = master->writesize;
slave->mtd.writebufsize = master->writebufsize;
slave->mtd.oobsize = master->oobsize;
- slave->mtd.oobavail = master->oobavail;
+ if (part->eccctrl) {
+ slave->mtd.eccctrl = part->eccctrl;
+ slave->mtd.oobavail = part->eccctrl->layout->oobavail;
+ } else {
+ slave->mtd.eccctrl = master->eccctrl;
+ slave->mtd.oobavail = master->oobavail;
+ }
slave->mtd.subpage_sft = master->subpage_sft;
slave->mtd.name = name;
@@ -515,9 +524,15 @@ static struct mtd_part *allocate_partition(struct mtd_info *master,
part->name);
}
- slave->mtd.ecclayout = master->ecclayout;
- slave->mtd.ecc_step_size = master->ecc_step_size;
- slave->mtd.ecc_strength = master->ecc_strength;
+ if (part->eccctrl) {
+ slave->mtd.ecclayout = part->eccctrl->layout;
+ slave->mtd.ecc_step_size = part->eccctrl->size;
+ slave->mtd.ecc_strength = part->eccctrl->strength;
You have to replicate 'struct nand_ecc_ctrl' from 'nand_chip' to 'mtd_info'
because each ECC algorithm may have its own callback for hwctl(), calculate(), and correct().
How do you plan to populate those ? In driver probe ?
That's already handled (see parse_ofnandpart function):
For each new partition if a valid ecc-mode is detected the function will
call the get_ecc_ctrl
callback which is then supposed to allocate and initialize a new
nand_ecc_ctrl struct.
On nand probe (more precisely in the nand_scan_tail function), I simply
assign the mtd eccctrl
field to the nand chip ecc field, so that even when there's no partition
the mtd device have a
reference to a valid nand_ecc_ctrl struct.
Is that what you were talking about ?
+ } else {
+ slave->mtd.ecclayout = master->ecclayout;
+ slave->mtd.ecc_step_size = master->ecc_step_size;
+ slave->mtd.ecc_strength = master->ecc_strength;
+ }
slave->mtd.bitflip_threshold = master->bitflip_threshold;
if (master->_block_isbad) {
[...]
+const struct nand_ecc_ctrl *nand_get_ecc_ctrl(struct mtd_info *mtd,
+ nand_ecc_modes_t mode,
+ struct device_node *np)
+{
+ struct nand_chip *chip = mtd->priv;
+ struct nand_ecc_ctrl *ecc;
+ u32 ecc_step, ecc_strength;
+ int ret;
+
+ if (mode != NAND_ECC_NONE && mode != NAND_ECC_SOFT &&
+ mode != NAND_ECC_SOFT_BCH)
+ return ERR_PTR(-EINVAL);
+
+ ecc = kzalloc(sizeof(*ecc), GFP_KERNEL);
+ if (!ecc)
+ return ERR_PTR(-ENOMEM);
+
+ ecc->size = chip->ecc_step_ds;
+ ecc->strength = chip->ecc_strength_ds;
+ if (!of_get_nand_ecc_level(np, &ecc_strength, &ecc_step)) {
+ ecc->size = ecc_step;
+ ecc->strength = ecc_strength;
+ }
+
Here we may need to ecc_strength for each partition.
Example: for OMAP3 ROM supports only HAM1 (1-bit hamming),
but kernel may support up-to BCH8 (8-bit BCH) ECC schemes.
But, anyways that's specific to OMAP platforms..
If you need HW specific handling, you just have to implement a new
get_ecc_ctrl
function and assign it to the get_ecc_ctrl field before calling
nand_scan_tail.
That's what I'm doing in the sunxi driver.
This nand_get_ecc_ctrl function is only used when no specific
get_ecc_ctrl is provided.
+ switch (mode) {
+ case NAND_ECC_NONE:
+ break;
+ case NAND_ECC_SOFT:
+ break;
+ case NAND_ECC_SOFT_BCH:
+ ecc->bytes = ((ecc->strength * fls(8 * ecc->size)) + 7) / 8;
+ break;
+ default:
+ ret = -EINVAL;
+ goto err;
+ }
+
+ ecc->mode = mode;
+ ret = nand_ecc_ctrl_init(mtd, ecc);
+ if (ret)
+ goto err;
+
+ ecc->release = nand_release_ecc_ctrl;
+
+ return ecc;
+
+err:
+ kfree(ecc);
+ return ERR_PTR(ret);
+}
+EXPORT_SYMBOL(nand_get_ecc_ctrl);
+
I'll be happy to extend and test this, if you plan a complete version.
This version is fully functionnal (I tested it with the sunxi driver),
but it depends on a this
patch: https://lkml.org/lkml/2014/1/29/210.
I'll be happy to get it tested on other platforms.
Best Regards,
Boris
[1] http://comments.gmane.org/gmane.linux.ports.arm.omap/108083
(you can skip initial discussion about OMAP3, and jump to Thomas Petazzoni | 2 Dec 17:19 2013)
with regards, pekon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/