* [PATCH 2/2] mtd: nand: realtek-ecc: Add Realtek external ECC engine support
@ 2025-08-18 9:27 ` Markus Stockhausen
0 siblings, 0 replies; 14+ messages in thread
From: Markus Stockhausen @ 2025-08-18 9:27 UTC (permalink / raw)
To: miquel.raynal, richard, vigneshr, robh, krzk+dt, conor+dt,
linux-mtd, devicetree
Cc: Markus Stockhausen
The Realtek RTl93xx switch SoC series has a built in ECC controller
that can provide BCH6 or BCH12 over 512 data and 6 tag bytes. It
generates 10 (BCH6) or 20 (BCH12) bytes of parity.
This engine will most likely work in conjunction with the Realtek
spi-mem based NAND controller but can work on its own. Therefore
the initial implementation will be of type external.
Remark! The engine can support any data blocks that are multiples
of 512 bytes. For now limit it to data+oob layouts that have been
analyzed from existing devices. This way it keeps compatibility
and pre-existing vendor data can be read.
Signed-off-by: Markus Stockhausen <markus.stockhausen@gmx.de>
---
drivers/mtd/nand/Kconfig | 8 +
drivers/mtd/nand/Makefile | 1 +
drivers/mtd/nand/ecc-realtek.c | 433 +++++++++++++++++++++++++++++++++
3 files changed, 442 insertions(+)
create mode 100644 drivers/mtd/nand/ecc-realtek.c
diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 5b0c2c95f10c..4a17271076bc 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -61,6 +61,14 @@ config MTD_NAND_ECC_MEDIATEK
help
This enables support for the hardware ECC engine from Mediatek.
+config MTD_NAND_ECC_REALTEK
+ tristate "Realtek RTL93xx hardware ECC engine"
+ depends on HAS_IOMEM
+ depends on MACH_REALTEK_RTL || COMPILE_TEST
+ select MTD_NAND_ECC
+ help
+ This enables support for the hardware ECC engine from Realtek.
+
endmenu
endmenu
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index 44913ff1bf12..2e0e56267718 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -3,6 +3,7 @@
nandcore-objs := core.o bbt.o
obj-$(CONFIG_MTD_NAND_CORE) += nandcore.o
obj-$(CONFIG_MTD_NAND_ECC_MEDIATEK) += ecc-mtk.o
+obj-$(CONFIG_MTD_NAND_ECC_REALTEK) += ecc-realtek.o
obj-$(CONFIG_SPI_QPIC_SNAND) += qpic_common.o
obj-$(CONFIG_MTD_NAND_QCOM) += qpic_common.o
obj-y += onenand/
diff --git a/drivers/mtd/nand/ecc-realtek.c b/drivers/mtd/nand/ecc-realtek.c
new file mode 100644
index 000000000000..a9c2a3a23b78
--- /dev/null
+++ b/drivers/mtd/nand/ecc-realtek.c
@@ -0,0 +1,433 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Support for Realtek hardware ECC engine in RTL93xx SoCs
+ */
+
+#include <linux/dma-mapping.h>
+#include <linux/mtd/nand.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+/*
+ * The Realtek ECC engine has two operation modes.
+ *
+ * - BCH6 : Generate 10 ECC bytes from 512 data bytes plus 6 tag bytes
+ * - BCH12: Generate 20 ECC bytes from 512 data bytes plus 6 tag bytes
+ *
+ * It can run for arbitrary NAND flash chips with different block and OOB sizes. Currently there
+ * are only two known devices in the wild that have NAND flash and make use of this ECC engine
+ * (Linksys LGS328C & LGS352C). To keep compatibility with vendor firmware, new modes can only
+ * be added when new data layouts have been analyzed. For now allow BCH6 on flash with 2048 byte
+ * blocks and 64 bytes oob.
+ */
+
+#define RTL_ECC_ALLOWED_PAGE_SIZE 2048
+#define RTL_ECC_ALLOWED_OOB_SIZE 64
+#define RTL_ECC_ALLOWED_STRENGTH 6
+
+#define RTL_ECC_BLOCK_SIZE 512
+#define RTL_ECC_TAG_SIZE 6
+#define RTL_ECC_PARITY_SIZE_BCH6 10
+#define RTL_ECC_PARITY_SIZE_BCH12 20
+
+/*
+ * The engine is fed with two DMA regions. One for data (always 512 bytes) and one for tag and
+ * parity (either 16 bytes for BCH6 or 26 bytes for BCH12). Start and length of each must be
+ * aligned to a multiple of 4.
+ */
+
+#define RTL_ECC_DMA_TAG_PARITY_SIZE ALIGN(RTL_ECC_TAG_SIZE + RTL_ECC_PARITY_SIZE_BCH12, 4)
+#define RTL_ECC_DMA_SIZE (RTL_ECC_BLOCK_SIZE + RTL_ECC_DMA_TAG_PARITY_SIZE)
+
+#define RTL_ECC_CFG 0x00
+#define RTL_ECC_BCH6 0
+#define RTL_ECC_BCH12 BIT(28)
+#define RTL_ECC_DMA_PRECISE BIT(12)
+#define RTL_ECC_BURST_128 GENMASK(1, 0)
+#define RTL_ECC_DMA_TRIGGER 0x08
+#define RTL_ECC_OP_DECODE 0
+#define RTL_ECC_OP_ENCODE BIT(0)
+#define RTL_ECC_DMA_START 0x0c
+#define RTL_ECC_DMA_TAG 0x10
+#define RTL_ECC_STATUS 0x14
+#define RTL_ECC_CORR_COUNT GENMASK(19, 12)
+#define RTL_ECC_RESULT BIT(8)
+#define RTL_ECC_ALL_ONE BIT(4)
+#define RTL_ECC_OP_STATUS BIT(0)
+
+struct rtl_ecc_engine {
+ struct device *dev;
+ struct nand_ecc_engine engine;
+ struct mutex lock;
+ char *buf;
+ dma_addr_t buf_dma;
+ struct regmap *regmap;
+};
+
+struct rtl_ecc_ctx {
+ struct rtl_ecc_engine * rtlc;
+ struct nand_ecc_req_tweak_ctx req_ctx;
+ int steps;
+ int bch_mode;
+ int parity_size;
+ int rc_bitflips;
+ int rc_status;
+};
+
+static const struct regmap_config rtl_ecc_regmap_config = {
+ .reg_bits = 32,
+ .val_bits = 32,
+ .reg_stride = 4,
+};
+
+static inline void *nand_to_ctx(struct nand_device *nand)
+{
+ return nand->ecc.ctx.priv;
+}
+
+static inline struct rtl_ecc_engine *nand_to_rtlc(struct nand_device *nand)
+{
+ struct nand_ecc_engine *eng = nand->ecc.engine;
+
+ return container_of(eng, struct rtl_ecc_engine, engine);
+}
+
+static int rtl_ecc_ooblayout_ecc(struct mtd_info *mtd, int section,
+ struct mtd_oob_region *oobregion)
+{
+ struct nand_device *nand = mtd_to_nanddev(mtd);
+ struct rtl_ecc_ctx *ctx = nand_to_ctx(nand);
+
+ if (section < 0 || section >= ctx->steps)
+ return -ERANGE;
+
+ oobregion->offset = ctx->steps * RTL_ECC_TAG_SIZE + section * ctx->parity_size;
+ oobregion->length = ctx->parity_size;
+
+ return 0;
+}
+
+static int rtl_ecc_ooblayout_free(struct mtd_info *mtd, int section,
+ struct mtd_oob_region *oobregion)
+{
+ struct nand_device *nand = mtd_to_nanddev(mtd);
+ struct rtl_ecc_ctx *ctx = nand_to_ctx(nand);
+
+ if (section < 0 || section >= ctx->steps)
+ return -ERANGE;
+
+ /* Tags can hold arbitrary values, so they are free for user data */
+ oobregion->offset = section * RTL_ECC_TAG_SIZE;
+ oobregion->length = RTL_ECC_TAG_SIZE;
+
+ return 0;
+}
+
+static const struct mtd_ooblayout_ops rtl_ecc_ooblayout_ops = {
+ .ecc = rtl_ecc_ooblayout_ecc,
+ .free = rtl_ecc_ooblayout_free,
+};
+
+static void rtl_ecc_kick_engine(struct rtl_ecc_ctx *ctx, int operation)
+{
+ struct rtl_ecc_engine *rtlc = ctx->rtlc;
+
+ regmap_write(rtlc->regmap, RTL_ECC_CFG,
+ ctx->bch_mode | RTL_ECC_BURST_128 | RTL_ECC_DMA_PRECISE);
+
+ regmap_write(rtlc->regmap, RTL_ECC_DMA_START, rtlc->buf_dma);
+ regmap_write(rtlc->regmap, RTL_ECC_DMA_TAG, rtlc->buf_dma + RTL_ECC_BLOCK_SIZE);
+ regmap_write(rtlc->regmap, RTL_ECC_DMA_TRIGGER, operation);
+}
+
+static void rtl_ecc_wait_for_engine(struct rtl_ecc_ctx *ctx)
+{
+ struct rtl_ecc_engine *rtlc = ctx->rtlc;
+ int ret, status;
+ bool all_one;
+
+ /*
+ * The ECC engine needs 6-8 us to encode/decode a BCH6 syndrome for 512 bytes of data
+ * and 6 tag bytes. In case the NAND area has been erased and all data and oob is
+ * set to 0xff, decoding takes 30us (reason unknown). Although the engine can trigger
+ * interrupts when finished, use active polling for now. 12 us maximum wait time has
+ * proven to be a good tradeoff between performance and overhead.
+ */
+
+ ret = regmap_read_poll_timeout(rtlc->regmap, RTL_ECC_STATUS, status,
+ !(status & RTL_ECC_OP_STATUS), 12, 600);
+ if (ret) {
+ ctx->rc_status = ret;
+ return;
+ }
+
+ all_one = FIELD_GET(RTL_ECC_ALL_ONE, status);
+ ctx->rc_status = FIELD_GET(RTL_ECC_RESULT, status);
+ ctx->rc_bitflips = FIELD_GET(RTL_ECC_CORR_COUNT, status);
+
+ /* For erased blocks (all bits one) error status can be ignored */
+ if (ctx->rc_status && all_one)
+ ctx->rc_status = 0;
+}
+
+static void rtl_ecc_run_engine(struct rtl_ecc_ctx *ctx, char *data, char *tag,
+ char *parity, int operation)
+{
+ struct rtl_ecc_engine *rtlc = ctx->rtlc;
+ char *buf_parity = rtlc->buf + RTL_ECC_BLOCK_SIZE + RTL_ECC_TAG_SIZE;
+ char *buf_tag = rtlc->buf + RTL_ECC_BLOCK_SIZE;
+ char *buf_data = rtlc->buf;
+
+ mutex_lock(&rtlc->lock);
+
+ memcpy(buf_data, data, RTL_ECC_BLOCK_SIZE);
+ memcpy(buf_tag, tag, RTL_ECC_TAG_SIZE);
+ memcpy(buf_parity, parity, ctx->parity_size);
+
+ dma_sync_single_for_device(rtlc->dev, rtlc->buf_dma, RTL_ECC_DMA_SIZE, DMA_TO_DEVICE);
+ rtl_ecc_kick_engine(ctx, operation);
+ rtl_ecc_wait_for_engine(ctx);
+ dma_sync_single_for_cpu(rtlc->dev, rtlc->buf_dma, RTL_ECC_DMA_SIZE, DMA_FROM_DEVICE);
+
+ if (!ctx->rc_status) {
+ memcpy(data, buf_data, RTL_ECC_BLOCK_SIZE);
+ memcpy(tag, buf_tag, RTL_ECC_TAG_SIZE);
+ memcpy(parity, buf_parity, ctx->parity_size);
+ }
+
+ mutex_unlock(&rtlc->lock);
+}
+
+static int rtl_ecc_prepare_io_req(struct nand_device *nand, struct nand_page_io_req *req)
+{
+ struct rtl_ecc_engine *rtlc = nand_to_rtlc(nand);
+ struct rtl_ecc_ctx *ctx = nand_to_ctx(nand);
+ char *data, *tag, *parity;
+ int failure = 0;
+
+ if (req->mode == MTD_OPS_RAW)
+ return 0;
+
+ nand_ecc_tweak_req(&ctx->req_ctx, req);
+
+ if (req->type == NAND_PAGE_READ)
+ return 0;
+
+ tag = req->oobbuf.in;
+ data = req->databuf.in;
+ parity = req->oobbuf.in + ctx->steps * RTL_ECC_TAG_SIZE;
+
+ for (int i = 0; i < ctx->steps; i++) {
+ rtl_ecc_run_engine(ctx, data, tag, parity, RTL_ECC_OP_ENCODE);
+ failure |= ctx->rc_status;
+
+ tag += RTL_ECC_TAG_SIZE;
+ data += RTL_ECC_BLOCK_SIZE;
+ parity += ctx->parity_size;
+ }
+
+ if (unlikely(failure))
+ dev_err(rtlc->dev, "ECC calculation failed\n");
+
+ return failure ? -EBADMSG : 0;
+}
+
+static int rtl_ecc_finish_io_req(struct nand_device *nand, struct nand_page_io_req *req)
+{
+ struct rtl_ecc_engine *rtlc = nand_to_rtlc(nand);
+ struct rtl_ecc_ctx *ctx = nand_to_ctx(nand);
+ struct mtd_info *mtd = nanddev_to_mtd(nand);
+ char *data, *tag, *parity;
+ unsigned int bitflips = 0;
+ int failure = 0;
+
+ if (req->mode == MTD_OPS_RAW)
+ return 0;
+
+ if (req->type == NAND_PAGE_WRITE) {
+ nand_ecc_restore_req(&ctx->req_ctx, req);
+ return 0;
+ }
+
+ tag = req->oobbuf.in;
+ data = req->databuf.in;
+ parity = req->oobbuf.in + ctx->steps * RTL_ECC_TAG_SIZE;
+
+ for (int i = 0 ; i < ctx->steps; i++) {
+ rtl_ecc_run_engine(ctx, data, tag, parity, RTL_ECC_OP_DECODE);
+ failure |= ctx->rc_status;
+ mtd->ecc_stats.failed += !!ctx->rc_status;
+ bitflips = max_t(unsigned int, ctx->rc_bitflips, bitflips);
+
+ tag += RTL_ECC_TAG_SIZE;
+ data += RTL_ECC_BLOCK_SIZE;
+ parity += ctx->parity_size;
+ }
+
+ nand_ecc_restore_req(&ctx->req_ctx, req);
+
+ if (unlikely(failure))
+ dev_err(rtlc->dev, "ECC correction failed\n");
+ else if (unlikely(bitflips > 2))
+ dev_warn(rtlc->dev, "%d bitflips detected\n", ctx->rc_bitflips);
+
+ return failure ? -EBADMSG : bitflips;
+}
+
+static int rtl_ecc_check_support(struct nand_device *nand)
+{
+ struct mtd_info *mtd = nanddev_to_mtd(nand);
+ struct device *dev = nand->ecc.engine->dev;
+
+ if (mtd->oobsize != RTL_ECC_ALLOWED_OOB_SIZE ||
+ mtd->writesize != RTL_ECC_ALLOWED_PAGE_SIZE) {
+ dev_err(dev, "only flash geometry data=%d, oob=%d supported\n",
+ RTL_ECC_ALLOWED_PAGE_SIZE, RTL_ECC_ALLOWED_OOB_SIZE);
+ return -EINVAL;
+ }
+
+ if (nand->ecc.user_conf.algo != NAND_ECC_ALGO_BCH ||
+ nand->ecc.user_conf.strength != RTL_ECC_ALLOWED_STRENGTH ||
+ nand->ecc.user_conf.placement != NAND_ECC_PLACEMENT_OOB ||
+ nand->ecc.user_conf.step_size != RTL_ECC_BLOCK_SIZE) {
+ dev_err(dev, "only algo=bch, strength=%d, placement=oob, step=%d supported\n",
+ RTL_ECC_ALLOWED_STRENGTH, RTL_ECC_BLOCK_SIZE);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int rtl_ecc_init_ctx(struct nand_device *nand)
+{
+ struct nand_ecc_props *conf = &nand->ecc.ctx.conf;
+ struct rtl_ecc_engine *rtlc = nand_to_rtlc(nand);
+ struct mtd_info *mtd = nanddev_to_mtd(nand);
+ int strength = nand->ecc.user_conf.strength;
+ struct device *dev = nand->ecc.engine->dev;
+ struct rtl_ecc_ctx *ctx;
+ int ret;
+
+ ret = rtl_ecc_check_support(nand);
+ if (ret)
+ return ret;
+
+ ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+ if (!ctx)
+ return -ENOMEM;
+
+ nand->ecc.ctx.priv = ctx;
+ mtd_set_ooblayout(mtd, &rtl_ecc_ooblayout_ops);
+
+ conf->algo = NAND_ECC_ALGO_BCH;
+ conf->strength = strength;
+ conf->step_size = RTL_ECC_BLOCK_SIZE;
+ conf->engine_type = NAND_ECC_ENGINE_TYPE_ON_HOST;
+
+ ctx->rtlc = rtlc;
+ ctx->steps = mtd->writesize / RTL_ECC_BLOCK_SIZE;
+ ctx->bch_mode = strength == 6 ? RTL_ECC_BCH6 : RTL_ECC_BCH12;
+ ctx->parity_size = strength == 6 ? RTL_ECC_PARITY_SIZE_BCH6 : RTL_ECC_PARITY_SIZE_BCH12;
+
+ ret = nand_ecc_init_req_tweaking(&ctx->req_ctx, nand);
+ if (ret)
+ return ret;
+
+ dev_info(dev, "using bch%d with geometry data=%dx%d, tag=%dx6, parity=%dx%d",
+ conf->strength, ctx->steps, conf->step_size,
+ ctx->steps, ctx->steps, ctx->parity_size);
+
+ return 0;
+}
+
+static void rtl_ecc_cleanup_ctx(struct nand_device *nand)
+{
+ struct rtl_ecc_ctx *ctx = nand_to_ctx(nand);
+
+ if (ctx)
+ nand_ecc_cleanup_req_tweaking(&ctx->req_ctx);
+}
+
+static struct nand_ecc_engine_ops rtl_ecc_engine_ops = {
+ .init_ctx = rtl_ecc_init_ctx,
+ .cleanup_ctx = rtl_ecc_cleanup_ctx,
+ .prepare_io_req = rtl_ecc_prepare_io_req,
+ .finish_io_req = rtl_ecc_finish_io_req,
+};
+
+static int rtl_ecc_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct rtl_ecc_engine *rtlc;
+ void __iomem *base;
+ int ret;
+
+ rtlc = devm_kzalloc(dev, sizeof(*rtlc), GFP_KERNEL);
+ if (!rtlc)
+ return -ENOMEM;
+
+ base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(base))
+ return PTR_ERR(base);
+
+ ret = devm_mutex_init(dev, &rtlc->lock);
+ if (ret)
+ return ret;
+
+ rtlc->regmap = devm_regmap_init_mmio(dev, base, &rtl_ecc_regmap_config);
+ if (IS_ERR(rtlc->regmap))
+ return PTR_ERR(rtlc->regmap);
+
+ /*
+ * Focus on simplicity and use a preallocated DMA buffer for data exchange with the
+ * engine. Make it a noncoherent memory model as invalidating/flushing caches is
+ * faster than reading/writing uncached memory.
+ */
+
+ rtlc->buf = dma_alloc_noncoherent(dev, RTL_ECC_DMA_SIZE, &rtlc->buf_dma,
+ DMA_BIDIRECTIONAL, GFP_KERNEL);
+ if (IS_ERR(rtlc->buf))
+ return PTR_ERR(rtlc->buf);
+
+ rtlc->dev = dev;
+ rtlc->engine.dev = dev;
+ rtlc->engine.ops = &rtl_ecc_engine_ops;
+ rtlc->engine.integration = NAND_ECC_ENGINE_INTEGRATION_EXTERNAL;
+
+ nand_ecc_register_on_host_hw_engine(&rtlc->engine);
+
+ platform_set_drvdata(pdev, rtlc);
+
+ return 0;
+}
+
+static void rtl_ecc_remove(struct platform_device *pdev)
+{
+ struct rtl_ecc_engine *rtlc = platform_get_drvdata(pdev);
+
+ nand_ecc_unregister_on_host_hw_engine(&rtlc->engine);
+ dma_free_noncoherent(rtlc->dev, RTL_ECC_DMA_SIZE, rtlc->buf, rtlc->buf_dma,
+ DMA_BIDIRECTIONAL);
+}
+
+static const struct of_device_id rtl_ecc_of_ids[] = {
+ {
+ .compatible = "realtek,rtl9301-ecc",
+ },
+ { /* sentinel */ },
+};
+
+static struct platform_driver rtl_ecc_driver = {
+ .driver = {
+ .name = "rtl-nand-ecc-engine",
+ .of_match_table = rtl_ecc_of_ids,
+ },
+ .probe = rtl_ecc_probe,
+ .remove = rtl_ecc_remove,
+};
+module_platform_driver(rtl_ecc_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Markus Stockhausen <markus.stockhausen@gmx.de>");
+MODULE_DESCRIPTION("Realtek NAND hardware ECC controller");
--
2.47.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 2/2] mtd: nand: realtek-ecc: Add Realtek external ECC engine support
2025-08-18 9:27 ` Markus Stockhausen
@ 2025-08-19 5:11 ` kernel test robot
-1 siblings, 0 replies; 14+ messages in thread
From: kernel test robot @ 2025-08-19 5:11 UTC (permalink / raw)
To: Markus Stockhausen, miquel.raynal, richard, vigneshr, robh,
krzk+dt, conor+dt, linux-mtd, devicetree
Cc: oe-kbuild-all, Markus Stockhausen
Hi Markus,
kernel test robot noticed the following build errors:
[auto build test ERROR on mtd/nand/next]
[also build test ERROR on linus/master v6.17-rc2 next-20250818]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Markus-Stockhausen/dt-bindings-mtd-Add-realtek-rtl9301-ecc/20250818-173253
base: https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next
patch link: https://lore.kernel.org/r/20250818092725.1977105-3-markus.stockhausen%40gmx.de
patch subject: [PATCH 2/2] mtd: nand: realtek-ecc: Add Realtek external ECC engine support
config: openrisc-allyesconfig (https://download.01.org/0day-ci/archive/20250819/202508191251.z4mFuzA6-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 15.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250819/202508191251.z4mFuzA6-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202508191251.z4mFuzA6-lkp@intel.com/
All errors (new ones prefixed by >>):
drivers/mtd/nand/ecc-realtek.c: In function 'rtl_ecc_wait_for_engine':
>> drivers/mtd/nand/ecc-realtek.c:165:19: error: implicit declaration of function 'FIELD_GET' [-Wimplicit-function-declaration]
165 | all_one = FIELD_GET(RTL_ECC_ALL_ONE, status);
| ^~~~~~~~~
vim +/FIELD_GET +165 drivers/mtd/nand/ecc-realtek.c
143
144 static void rtl_ecc_wait_for_engine(struct rtl_ecc_ctx *ctx)
145 {
146 struct rtl_ecc_engine *rtlc = ctx->rtlc;
147 int ret, status;
148 bool all_one;
149
150 /*
151 * The ECC engine needs 6-8 us to encode/decode a BCH6 syndrome for 512 bytes of data
152 * and 6 tag bytes. In case the NAND area has been erased and all data and oob is
153 * set to 0xff, decoding takes 30us (reason unknown). Although the engine can trigger
154 * interrupts when finished, use active polling for now. 12 us maximum wait time has
155 * proven to be a good tradeoff between performance and overhead.
156 */
157
158 ret = regmap_read_poll_timeout(rtlc->regmap, RTL_ECC_STATUS, status,
159 !(status & RTL_ECC_OP_STATUS), 12, 600);
160 if (ret) {
161 ctx->rc_status = ret;
162 return;
163 }
164
> 165 all_one = FIELD_GET(RTL_ECC_ALL_ONE, status);
166 ctx->rc_status = FIELD_GET(RTL_ECC_RESULT, status);
167 ctx->rc_bitflips = FIELD_GET(RTL_ECC_CORR_COUNT, status);
168
169 /* For erased blocks (all bits one) error status can be ignored */
170 if (ctx->rc_status && all_one)
171 ctx->rc_status = 0;
172 }
173
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 2/2] mtd: nand: realtek-ecc: Add Realtek external ECC engine support
@ 2025-08-19 5:11 ` kernel test robot
0 siblings, 0 replies; 14+ messages in thread
From: kernel test robot @ 2025-08-19 5:11 UTC (permalink / raw)
To: Markus Stockhausen, miquel.raynal, richard, vigneshr, robh,
krzk+dt, conor+dt, linux-mtd, devicetree
Cc: oe-kbuild-all, Markus Stockhausen
Hi Markus,
kernel test robot noticed the following build errors:
[auto build test ERROR on mtd/nand/next]
[also build test ERROR on linus/master v6.17-rc2 next-20250818]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Markus-Stockhausen/dt-bindings-mtd-Add-realtek-rtl9301-ecc/20250818-173253
base: https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next
patch link: https://lore.kernel.org/r/20250818092725.1977105-3-markus.stockhausen%40gmx.de
patch subject: [PATCH 2/2] mtd: nand: realtek-ecc: Add Realtek external ECC engine support
config: openrisc-allyesconfig (https://download.01.org/0day-ci/archive/20250819/202508191251.z4mFuzA6-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 15.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250819/202508191251.z4mFuzA6-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202508191251.z4mFuzA6-lkp@intel.com/
All errors (new ones prefixed by >>):
drivers/mtd/nand/ecc-realtek.c: In function 'rtl_ecc_wait_for_engine':
>> drivers/mtd/nand/ecc-realtek.c:165:19: error: implicit declaration of function 'FIELD_GET' [-Wimplicit-function-declaration]
165 | all_one = FIELD_GET(RTL_ECC_ALL_ONE, status);
| ^~~~~~~~~
vim +/FIELD_GET +165 drivers/mtd/nand/ecc-realtek.c
143
144 static void rtl_ecc_wait_for_engine(struct rtl_ecc_ctx *ctx)
145 {
146 struct rtl_ecc_engine *rtlc = ctx->rtlc;
147 int ret, status;
148 bool all_one;
149
150 /*
151 * The ECC engine needs 6-8 us to encode/decode a BCH6 syndrome for 512 bytes of data
152 * and 6 tag bytes. In case the NAND area has been erased and all data and oob is
153 * set to 0xff, decoding takes 30us (reason unknown). Although the engine can trigger
154 * interrupts when finished, use active polling for now. 12 us maximum wait time has
155 * proven to be a good tradeoff between performance and overhead.
156 */
157
158 ret = regmap_read_poll_timeout(rtlc->regmap, RTL_ECC_STATUS, status,
159 !(status & RTL_ECC_OP_STATUS), 12, 600);
160 if (ret) {
161 ctx->rc_status = ret;
162 return;
163 }
164
> 165 all_one = FIELD_GET(RTL_ECC_ALL_ONE, status);
166 ctx->rc_status = FIELD_GET(RTL_ECC_RESULT, status);
167 ctx->rc_bitflips = FIELD_GET(RTL_ECC_CORR_COUNT, status);
168
169 /* For erased blocks (all bits one) error status can be ignored */
170 if (ctx->rc_status && all_one)
171 ctx->rc_status = 0;
172 }
173
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] mtd: nand: realtek-ecc: Add Realtek external ECC engine support
2025-08-18 9:27 ` Markus Stockhausen
@ 2025-08-24 16:51 ` Miquel Raynal
-1 siblings, 0 replies; 14+ messages in thread
From: Miquel Raynal @ 2025-08-24 16:51 UTC (permalink / raw)
To: Markus Stockhausen
Cc: richard, vigneshr, robh, krzk+dt, conor+dt, linux-mtd, devicetree
Hi Markus,
On 18/08/2025 at 05:27:25 -04, Markus Stockhausen <markus.stockhausen@gmx.de> wrote:
> The Realtek RTl93xx switch SoC series has a built in ECC controller
> that can provide BCH6 or BCH12 over 512 data and 6 tag bytes. It
> generates 10 (BCH6) or 20 (BCH12) bytes of parity.
>
> This engine will most likely work in conjunction with the Realtek
> spi-mem based NAND controller but can work on its own. Therefore
> the initial implementation will be of type external.
>
> Remark! The engine can support any data blocks that are multiples
> of 512 bytes. For now limit it to data+oob layouts that have been
> analyzed from existing devices. This way it keeps compatibility
> and pre-existing vendor data can be read.
>
> Signed-off-by: Markus Stockhausen <markus.stockhausen@gmx.de>
> ---
Very happy to see a new external ECC driver!
> drivers/mtd/nand/Kconfig | 8 +
> drivers/mtd/nand/Makefile | 1 +
> drivers/mtd/nand/ecc-realtek.c | 433 +++++++++++++++++++++++++++++++++
> 3 files changed, 442 insertions(+)
> create mode 100644 drivers/mtd/nand/ecc-realtek.c
>
> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> index 5b0c2c95f10c..4a17271076bc 100644
> --- a/drivers/mtd/nand/Kconfig
> +++ b/drivers/mtd/nand/Kconfig
> @@ -61,6 +61,14 @@ config MTD_NAND_ECC_MEDIATEK
> help
> This enables support for the hardware ECC engine from Mediatek.
>
> +config MTD_NAND_ECC_REALTEK
> + tristate "Realtek RTL93xx hardware ECC engine"
> + depends on HAS_IOMEM
> + depends on MACH_REALTEK_RTL || COMPILE_TEST
> + select MTD_NAND_ECC
> + help
> + This enables support for the hardware ECC engine from Realtek.
> +
> endmenu
>
> endmenu
> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> index 44913ff1bf12..2e0e56267718 100644
> --- a/drivers/mtd/nand/Makefile
> +++ b/drivers/mtd/nand/Makefile
> @@ -3,6 +3,7 @@
> nandcore-objs := core.o bbt.o
> obj-$(CONFIG_MTD_NAND_CORE) += nandcore.o
> obj-$(CONFIG_MTD_NAND_ECC_MEDIATEK) += ecc-mtk.o
> +obj-$(CONFIG_MTD_NAND_ECC_REALTEK) += ecc-realtek.o
> obj-$(CONFIG_SPI_QPIC_SNAND) += qpic_common.o
> obj-$(CONFIG_MTD_NAND_QCOM) += qpic_common.o
> obj-y += onenand/
> diff --git a/drivers/mtd/nand/ecc-realtek.c b/drivers/mtd/nand/ecc-realtek.c
> new file mode 100644
> index 000000000000..a9c2a3a23b78
> --- /dev/null
> +++ b/drivers/mtd/nand/ecc-realtek.c
> @@ -0,0 +1,433 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Support for Realtek hardware ECC engine in RTL93xx SoCs
> + */
> +
> +#include <linux/dma-mapping.h>
> +#include <linux/mtd/nand.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +/*
> + * The Realtek ECC engine has two operation modes.
> + *
> + * - BCH6 : Generate 10 ECC bytes from 512 data bytes plus 6 tag bytes
> + * - BCH12: Generate 20 ECC bytes from 512 data bytes plus 6 tag
> bytes
What is a tag? I believe it is Realtek wording and we need to find a
more generic translation.
> + *
> + * It can run for arbitrary NAND flash chips with different block and OOB sizes. Currently there
> + * are only two known devices in the wild that have NAND flash and make use of this ECC engine
> + * (Linksys LGS328C & LGS352C). To keep compatibility with vendor firmware, new modes can only
> + * be added when new data layouts have been analyzed. For now allow BCH6 on flash with 2048 byte
> + * blocks and 64 bytes oob.
> + */
> +
> +#define RTL_ECC_ALLOWED_PAGE_SIZE 2048
> +#define RTL_ECC_ALLOWED_OOB_SIZE 64
> +#define RTL_ECC_ALLOWED_STRENGTH 6
> +
> +#define RTL_ECC_BLOCK_SIZE 512
> +#define RTL_ECC_TAG_SIZE 6
> +#define RTL_ECC_PARITY_SIZE_BCH6 10
> +#define RTL_ECC_PARITY_SIZE_BCH12 20
> +
> +/*
> + * The engine is fed with two DMA regions. One for data (always 512 bytes) and one for tag and
> + * parity (either 16 bytes for BCH6 or 26 bytes for BCH12). Start and length of each must be
> + * aligned to a multiple of 4.
> + */
> +
> +#define RTL_ECC_DMA_TAG_PARITY_SIZE ALIGN(RTL_ECC_TAG_SIZE + RTL_ECC_PARITY_SIZE_BCH12, 4)
> +#define RTL_ECC_DMA_SIZE (RTL_ECC_BLOCK_SIZE + RTL_ECC_DMA_TAG_PARITY_SIZE)
> +
> +#define RTL_ECC_CFG 0x00
> +#define RTL_ECC_BCH6 0
> +#define RTL_ECC_BCH12 BIT(28)
> +#define RTL_ECC_DMA_PRECISE BIT(12)
> +#define RTL_ECC_BURST_128 GENMASK(1, 0)
> +#define RTL_ECC_DMA_TRIGGER 0x08
> +#define RTL_ECC_OP_DECODE 0
> +#define RTL_ECC_OP_ENCODE BIT(0)
> +#define RTL_ECC_DMA_START 0x0c
> +#define RTL_ECC_DMA_TAG 0x10
> +#define RTL_ECC_STATUS 0x14
> +#define RTL_ECC_CORR_COUNT GENMASK(19, 12)
> +#define RTL_ECC_RESULT BIT(8)
> +#define RTL_ECC_ALL_ONE BIT(4)
> +#define RTL_ECC_OP_STATUS BIT(0)
> +
> +struct rtl_ecc_engine {
> + struct device *dev;
> + struct nand_ecc_engine engine;
> + struct mutex lock;
> + char *buf;
> + dma_addr_t buf_dma;
> + struct regmap *regmap;
> +};
> +
> +struct rtl_ecc_ctx {
> + struct rtl_ecc_engine * rtlc;
> + struct nand_ecc_req_tweak_ctx req_ctx;
> + int steps;
> + int bch_mode;
> + int parity_size;
> + int rc_bitflips;
> + int rc_status;
> +};
> +
> +static const struct regmap_config rtl_ecc_regmap_config = {
> + .reg_bits = 32,
> + .val_bits = 32,
> + .reg_stride = 4,
> +};
> +
> +static inline void *nand_to_ctx(struct nand_device *nand)
> +{
> + return nand->ecc.ctx.priv;
> +}
> +
> +static inline struct rtl_ecc_engine *nand_to_rtlc(struct nand_device *nand)
> +{
> + struct nand_ecc_engine *eng = nand->ecc.engine;
> +
> + return container_of(eng, struct rtl_ecc_engine, engine);
> +}
> +
> +static int rtl_ecc_ooblayout_ecc(struct mtd_info *mtd, int section,
> + struct mtd_oob_region *oobregion)
> +{
> + struct nand_device *nand = mtd_to_nanddev(mtd);
> + struct rtl_ecc_ctx *ctx = nand_to_ctx(nand);
> +
> + if (section < 0 || section >= ctx->steps)
> + return -ERANGE;
> +
> + oobregion->offset = ctx->steps * RTL_ECC_TAG_SIZE + section * ctx->parity_size;
> + oobregion->length = ctx->parity_size;
Please reserve 2 bytes for BBM.
For me it looks like you have a bit free section and a bit ECC
section. If that simplifies your life, you can just return a single
section for both.
> +
> + return 0;
> +}
> +
> +static int rtl_ecc_ooblayout_free(struct mtd_info *mtd, int section,
> + struct mtd_oob_region *oobregion)
> +{
> + struct nand_device *nand = mtd_to_nanddev(mtd);
> + struct rtl_ecc_ctx *ctx = nand_to_ctx(nand);
> +
> + if (section < 0 || section >= ctx->steps)
> + return -ERANGE;
> +
> + /* Tags can hold arbitrary values, so they are free for user data */
> + oobregion->offset = section * RTL_ECC_TAG_SIZE;
> + oobregion->length = RTL_ECC_TAG_SIZE;
> +
> + return 0;
> +}
> +
> +static const struct mtd_ooblayout_ops rtl_ecc_ooblayout_ops = {
> + .ecc = rtl_ecc_ooblayout_ecc,
> + .free = rtl_ecc_ooblayout_free,
> +};
> +
> +static void rtl_ecc_kick_engine(struct rtl_ecc_ctx *ctx, int operation)
> +{
> + struct rtl_ecc_engine *rtlc = ctx->rtlc;
> +
> + regmap_write(rtlc->regmap, RTL_ECC_CFG,
> + ctx->bch_mode | RTL_ECC_BURST_128 | RTL_ECC_DMA_PRECISE);
> +
> + regmap_write(rtlc->regmap, RTL_ECC_DMA_START, rtlc->buf_dma);
> + regmap_write(rtlc->regmap, RTL_ECC_DMA_TAG, rtlc->buf_dma + RTL_ECC_BLOCK_SIZE);
> + regmap_write(rtlc->regmap, RTL_ECC_DMA_TRIGGER, operation);
> +}
> +
> +static void rtl_ecc_wait_for_engine(struct rtl_ecc_ctx *ctx)
> +{
> + struct rtl_ecc_engine *rtlc = ctx->rtlc;
> + int ret, status;
> + bool all_one;
> +
> + /*
> + * The ECC engine needs 6-8 us to encode/decode a BCH6 syndrome for 512 bytes of data
> + * and 6 tag bytes. In case the NAND area has been erased and all data and oob is
> + * set to 0xff, decoding takes 30us (reason unknown). Although the engine can trigger
> + * interrupts when finished, use active polling for now. 12 us maximum wait time has
> + * proven to be a good tradeoff between performance and overhead.
> + */
> +
> + ret = regmap_read_poll_timeout(rtlc->regmap, RTL_ECC_STATUS, status,
> + !(status & RTL_ECC_OP_STATUS), 12, 600);
> + if (ret) {
> + ctx->rc_status = ret;
> + return;
Please make this function return an int and use it in the caller instead
of using an intermediate status variable for that.
> + }
> +
> + all_one = FIELD_GET(RTL_ECC_ALL_ONE, status);
> + ctx->rc_status = FIELD_GET(RTL_ECC_RESULT, status);
> + ctx->rc_bitflips = FIELD_GET(RTL_ECC_CORR_COUNT, status);
> +
> + /* For erased blocks (all bits one) error status can be ignored */
> + if (ctx->rc_status && all_one)
> + ctx->rc_status = 0;
> +}
> +
> +static void rtl_ecc_run_engine(struct rtl_ecc_ctx *ctx, char *data, char *tag,
> + char *parity, int operation)
> +{
> + struct rtl_ecc_engine *rtlc = ctx->rtlc;
> + char *buf_parity = rtlc->buf + RTL_ECC_BLOCK_SIZE + RTL_ECC_TAG_SIZE;
> + char *buf_tag = rtlc->buf + RTL_ECC_BLOCK_SIZE;
> + char *buf_data = rtlc->buf;
> +
> + mutex_lock(&rtlc->lock);
> +
> + memcpy(buf_data, data, RTL_ECC_BLOCK_SIZE);
> + memcpy(buf_tag, tag, RTL_ECC_TAG_SIZE);
> + memcpy(buf_parity, parity, ctx->parity_size);
> +
> + dma_sync_single_for_device(rtlc->dev, rtlc->buf_dma, RTL_ECC_DMA_SIZE, DMA_TO_DEVICE);
> + rtl_ecc_kick_engine(ctx, operation);
> + rtl_ecc_wait_for_engine(ctx);
> + dma_sync_single_for_cpu(rtlc->dev, rtlc->buf_dma, RTL_ECC_DMA_SIZE, DMA_FROM_DEVICE);
> +
> + if (!ctx->rc_status) {
> + memcpy(data, buf_data, RTL_ECC_BLOCK_SIZE);
> + memcpy(tag, buf_tag, RTL_ECC_TAG_SIZE);
> + memcpy(parity, buf_parity, ctx->parity_size);
> + }
> +
> + mutex_unlock(&rtlc->lock);
> +}
> +
> +static int rtl_ecc_prepare_io_req(struct nand_device *nand, struct nand_page_io_req *req)
> +{
> + struct rtl_ecc_engine *rtlc = nand_to_rtlc(nand);
> + struct rtl_ecc_ctx *ctx = nand_to_ctx(nand);
> + char *data, *tag, *parity;
> + int failure = 0;
> +
> + if (req->mode == MTD_OPS_RAW)
> + return 0;
> +
> + nand_ecc_tweak_req(&ctx->req_ctx, req);
> +
> + if (req->type == NAND_PAGE_READ)
> + return 0;
> +
> + tag = req->oobbuf.in;
> + data = req->databuf.in;
> + parity = req->oobbuf.in + ctx->steps * RTL_ECC_TAG_SIZE;
> +
> + for (int i = 0; i < ctx->steps; i++) {
> + rtl_ecc_run_engine(ctx, data, tag, parity, RTL_ECC_OP_ENCODE);
> + failure |= ctx->rc_status;
> +
> + tag += RTL_ECC_TAG_SIZE;
> + data += RTL_ECC_BLOCK_SIZE;
> + parity += ctx->parity_size;
> + }
> +
> + if (unlikely(failure))
> + dev_err(rtlc->dev, "ECC calculation failed\n");
This is not helpful as the other layers will complain if that's
relevant. For instance in case you use this driver with a NAND with read
retry support, it would be very noisy. Please turn this into a dev_dbg
call at most.
> +
> + return failure ? -EBADMSG : 0;
> +}
> +
> +static int rtl_ecc_finish_io_req(struct nand_device *nand, struct nand_page_io_req *req)
> +{
> + struct rtl_ecc_engine *rtlc = nand_to_rtlc(nand);
> + struct rtl_ecc_ctx *ctx = nand_to_ctx(nand);
> + struct mtd_info *mtd = nanddev_to_mtd(nand);
> + char *data, *tag, *parity;
> + unsigned int bitflips = 0;
> + int failure = 0;
> +
> + if (req->mode == MTD_OPS_RAW)
> + return 0;
> +
> + if (req->type == NAND_PAGE_WRITE) {
> + nand_ecc_restore_req(&ctx->req_ctx, req);
> + return 0;
> + }
> +
> + tag = req->oobbuf.in;
> + data = req->databuf.in;
> + parity = req->oobbuf.in + ctx->steps * RTL_ECC_TAG_SIZE;
> +
> + for (int i = 0 ; i < ctx->steps; i++) {
> + rtl_ecc_run_engine(ctx, data, tag, parity, RTL_ECC_OP_DECODE);
> + failure |= ctx->rc_status;
> + mtd->ecc_stats.failed += !!ctx->rc_status;
> + bitflips = max_t(unsigned int, ctx->rc_bitflips, bitflips);
> +
> + tag += RTL_ECC_TAG_SIZE;
> + data += RTL_ECC_BLOCK_SIZE;
> + parity += ctx->parity_size;
> + }
> +
> + nand_ecc_restore_req(&ctx->req_ctx, req);
> +
> + if (unlikely(failure))
> + dev_err(rtlc->dev, "ECC correction failed\n");
> + else if (unlikely(bitflips > 2))
> + dev_warn(rtlc->dev, "%d bitflips detected\n", ctx->rc_bitflips);
Same here. Way too noisy for something (a bitflip) that happens normally.
> +
> + return failure ? -EBADMSG : bitflips;
> +}
> +
> +static int rtl_ecc_check_support(struct nand_device *nand)
> +{
> + struct mtd_info *mtd = nanddev_to_mtd(nand);
> + struct device *dev = nand->ecc.engine->dev;
> +
> + if (mtd->oobsize != RTL_ECC_ALLOWED_OOB_SIZE ||
> + mtd->writesize != RTL_ECC_ALLOWED_PAGE_SIZE) {
> + dev_err(dev, "only flash geometry data=%d, oob=%d supported\n",
> + RTL_ECC_ALLOWED_PAGE_SIZE, RTL_ECC_ALLOWED_OOB_SIZE);
> + return -EINVAL;
> + }
> +
> + if (nand->ecc.user_conf.algo != NAND_ECC_ALGO_BCH ||
> + nand->ecc.user_conf.strength != RTL_ECC_ALLOWED_STRENGTH ||
> + nand->ecc.user_conf.placement != NAND_ECC_PLACEMENT_OOB ||
> + nand->ecc.user_conf.step_size != RTL_ECC_BLOCK_SIZE) {
> + dev_err(dev, "only algo=bch, strength=%d, placement=oob, step=%d supported\n",
> + RTL_ECC_ALLOWED_STRENGTH, RTL_ECC_BLOCK_SIZE);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int rtl_ecc_init_ctx(struct nand_device *nand)
> +{
> + struct nand_ecc_props *conf = &nand->ecc.ctx.conf;
> + struct rtl_ecc_engine *rtlc = nand_to_rtlc(nand);
> + struct mtd_info *mtd = nanddev_to_mtd(nand);
> + int strength = nand->ecc.user_conf.strength;
> + struct device *dev = nand->ecc.engine->dev;
> + struct rtl_ecc_ctx *ctx;
> + int ret;
> +
> + ret = rtl_ecc_check_support(nand);
> + if (ret)
> + return ret;
> +
> + ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> + if (!ctx)
> + return -ENOMEM;
> +
> + nand->ecc.ctx.priv = ctx;
> + mtd_set_ooblayout(mtd, &rtl_ecc_ooblayout_ops);
> +
> + conf->algo = NAND_ECC_ALGO_BCH;
> + conf->strength = strength;
> + conf->step_size = RTL_ECC_BLOCK_SIZE;
> + conf->engine_type = NAND_ECC_ENGINE_TYPE_ON_HOST;
> +
> + ctx->rtlc = rtlc;
> + ctx->steps = mtd->writesize / RTL_ECC_BLOCK_SIZE;
> + ctx->bch_mode = strength == 6 ? RTL_ECC_BCH6 : RTL_ECC_BCH12;
> + ctx->parity_size = strength == 6 ? RTL_ECC_PARITY_SIZE_BCH6 : RTL_ECC_PARITY_SIZE_BCH12;
> +
> + ret = nand_ecc_init_req_tweaking(&ctx->req_ctx, nand);
> + if (ret)
> + return ret;
> +
> + dev_info(dev, "using bch%d with geometry data=%dx%d, tag=%dx6, parity=%dx%d",
> + conf->strength, ctx->steps, conf->step_size,
> + ctx->steps, ctx->steps, ctx->parity_size);
Same, dev_dbg is fine, but otherwise no need.
> +
> + return 0;
> +}
> +
> +static void rtl_ecc_cleanup_ctx(struct nand_device *nand)
> +{
> + struct rtl_ecc_ctx *ctx = nand_to_ctx(nand);
> +
> + if (ctx)
> + nand_ecc_cleanup_req_tweaking(&ctx->req_ctx);
> +}
> +
> +static struct nand_ecc_engine_ops rtl_ecc_engine_ops = {
> + .init_ctx = rtl_ecc_init_ctx,
> + .cleanup_ctx = rtl_ecc_cleanup_ctx,
> + .prepare_io_req = rtl_ecc_prepare_io_req,
> + .finish_io_req = rtl_ecc_finish_io_req,
> +};
> +
> +static int rtl_ecc_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct rtl_ecc_engine *rtlc;
> + void __iomem *base;
> + int ret;
> +
> + rtlc = devm_kzalloc(dev, sizeof(*rtlc), GFP_KERNEL);
> + if (!rtlc)
> + return -ENOMEM;
> +
> + base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(base))
> + return PTR_ERR(base);
> +
> + ret = devm_mutex_init(dev, &rtlc->lock);
> + if (ret)
> + return ret;
> +
> + rtlc->regmap = devm_regmap_init_mmio(dev, base, &rtl_ecc_regmap_config);
> + if (IS_ERR(rtlc->regmap))
> + return PTR_ERR(rtlc->regmap);
> +
> + /*
> + * Focus on simplicity and use a preallocated DMA buffer for data exchange with the
> + * engine. Make it a noncoherent memory model as invalidating/flushing caches is
> + * faster than reading/writing uncached memory.
> + */
I guess it depends on the architecture, but that's fine.
> +
> + rtlc->buf = dma_alloc_noncoherent(dev, RTL_ECC_DMA_SIZE, &rtlc->buf_dma,
> + DMA_BIDIRECTIONAL, GFP_KERNEL);
> + if (IS_ERR(rtlc->buf))
> + return PTR_ERR(rtlc->buf);
> +
> + rtlc->dev = dev;
> + rtlc->engine.dev = dev;
> + rtlc->engine.ops = &rtl_ecc_engine_ops;
> + rtlc->engine.integration = NAND_ECC_ENGINE_INTEGRATION_EXTERNAL;
> +
> + nand_ecc_register_on_host_hw_engine(&rtlc->engine);
> +
> + platform_set_drvdata(pdev, rtlc);
> +
> + return 0;
> +}
> +
> +static void rtl_ecc_remove(struct platform_device *pdev)
> +{
> + struct rtl_ecc_engine *rtlc = platform_get_drvdata(pdev);
> +
> + nand_ecc_unregister_on_host_hw_engine(&rtlc->engine);
> + dma_free_noncoherent(rtlc->dev, RTL_ECC_DMA_SIZE, rtlc->buf, rtlc->buf_dma,
> + DMA_BIDIRECTIONAL);
> +}
> +
> +static const struct of_device_id rtl_ecc_of_ids[] = {
> + {
> + .compatible = "realtek,rtl9301-ecc",
> + },
> + { /* sentinel */ },
> +};
> +
> +static struct platform_driver rtl_ecc_driver = {
> + .driver = {
> + .name = "rtl-nand-ecc-engine",
> + .of_match_table = rtl_ecc_of_ids,
> + },
> + .probe = rtl_ecc_probe,
> + .remove = rtl_ecc_remove,
> +};
> +module_platform_driver(rtl_ecc_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Markus Stockhausen <markus.stockhausen@gmx.de>");
> +MODULE_DESCRIPTION("Realtek NAND hardware ECC controller");
Great work nevertheless! Looking forward to get it upstreamed.
Thanks,
Miquèl
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 2/2] mtd: nand: realtek-ecc: Add Realtek external ECC engine support
@ 2025-08-24 16:51 ` Miquel Raynal
0 siblings, 0 replies; 14+ messages in thread
From: Miquel Raynal @ 2025-08-24 16:51 UTC (permalink / raw)
To: Markus Stockhausen
Cc: richard, vigneshr, robh, krzk+dt, conor+dt, linux-mtd, devicetree
Hi Markus,
On 18/08/2025 at 05:27:25 -04, Markus Stockhausen <markus.stockhausen@gmx.de> wrote:
> The Realtek RTl93xx switch SoC series has a built in ECC controller
> that can provide BCH6 or BCH12 over 512 data and 6 tag bytes. It
> generates 10 (BCH6) or 20 (BCH12) bytes of parity.
>
> This engine will most likely work in conjunction with the Realtek
> spi-mem based NAND controller but can work on its own. Therefore
> the initial implementation will be of type external.
>
> Remark! The engine can support any data blocks that are multiples
> of 512 bytes. For now limit it to data+oob layouts that have been
> analyzed from existing devices. This way it keeps compatibility
> and pre-existing vendor data can be read.
>
> Signed-off-by: Markus Stockhausen <markus.stockhausen@gmx.de>
> ---
Very happy to see a new external ECC driver!
> drivers/mtd/nand/Kconfig | 8 +
> drivers/mtd/nand/Makefile | 1 +
> drivers/mtd/nand/ecc-realtek.c | 433 +++++++++++++++++++++++++++++++++
> 3 files changed, 442 insertions(+)
> create mode 100644 drivers/mtd/nand/ecc-realtek.c
>
> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> index 5b0c2c95f10c..4a17271076bc 100644
> --- a/drivers/mtd/nand/Kconfig
> +++ b/drivers/mtd/nand/Kconfig
> @@ -61,6 +61,14 @@ config MTD_NAND_ECC_MEDIATEK
> help
> This enables support for the hardware ECC engine from Mediatek.
>
> +config MTD_NAND_ECC_REALTEK
> + tristate "Realtek RTL93xx hardware ECC engine"
> + depends on HAS_IOMEM
> + depends on MACH_REALTEK_RTL || COMPILE_TEST
> + select MTD_NAND_ECC
> + help
> + This enables support for the hardware ECC engine from Realtek.
> +
> endmenu
>
> endmenu
> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> index 44913ff1bf12..2e0e56267718 100644
> --- a/drivers/mtd/nand/Makefile
> +++ b/drivers/mtd/nand/Makefile
> @@ -3,6 +3,7 @@
> nandcore-objs := core.o bbt.o
> obj-$(CONFIG_MTD_NAND_CORE) += nandcore.o
> obj-$(CONFIG_MTD_NAND_ECC_MEDIATEK) += ecc-mtk.o
> +obj-$(CONFIG_MTD_NAND_ECC_REALTEK) += ecc-realtek.o
> obj-$(CONFIG_SPI_QPIC_SNAND) += qpic_common.o
> obj-$(CONFIG_MTD_NAND_QCOM) += qpic_common.o
> obj-y += onenand/
> diff --git a/drivers/mtd/nand/ecc-realtek.c b/drivers/mtd/nand/ecc-realtek.c
> new file mode 100644
> index 000000000000..a9c2a3a23b78
> --- /dev/null
> +++ b/drivers/mtd/nand/ecc-realtek.c
> @@ -0,0 +1,433 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Support for Realtek hardware ECC engine in RTL93xx SoCs
> + */
> +
> +#include <linux/dma-mapping.h>
> +#include <linux/mtd/nand.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +/*
> + * The Realtek ECC engine has two operation modes.
> + *
> + * - BCH6 : Generate 10 ECC bytes from 512 data bytes plus 6 tag bytes
> + * - BCH12: Generate 20 ECC bytes from 512 data bytes plus 6 tag
> bytes
What is a tag? I believe it is Realtek wording and we need to find a
more generic translation.
> + *
> + * It can run for arbitrary NAND flash chips with different block and OOB sizes. Currently there
> + * are only two known devices in the wild that have NAND flash and make use of this ECC engine
> + * (Linksys LGS328C & LGS352C). To keep compatibility with vendor firmware, new modes can only
> + * be added when new data layouts have been analyzed. For now allow BCH6 on flash with 2048 byte
> + * blocks and 64 bytes oob.
> + */
> +
> +#define RTL_ECC_ALLOWED_PAGE_SIZE 2048
> +#define RTL_ECC_ALLOWED_OOB_SIZE 64
> +#define RTL_ECC_ALLOWED_STRENGTH 6
> +
> +#define RTL_ECC_BLOCK_SIZE 512
> +#define RTL_ECC_TAG_SIZE 6
> +#define RTL_ECC_PARITY_SIZE_BCH6 10
> +#define RTL_ECC_PARITY_SIZE_BCH12 20
> +
> +/*
> + * The engine is fed with two DMA regions. One for data (always 512 bytes) and one for tag and
> + * parity (either 16 bytes for BCH6 or 26 bytes for BCH12). Start and length of each must be
> + * aligned to a multiple of 4.
> + */
> +
> +#define RTL_ECC_DMA_TAG_PARITY_SIZE ALIGN(RTL_ECC_TAG_SIZE + RTL_ECC_PARITY_SIZE_BCH12, 4)
> +#define RTL_ECC_DMA_SIZE (RTL_ECC_BLOCK_SIZE + RTL_ECC_DMA_TAG_PARITY_SIZE)
> +
> +#define RTL_ECC_CFG 0x00
> +#define RTL_ECC_BCH6 0
> +#define RTL_ECC_BCH12 BIT(28)
> +#define RTL_ECC_DMA_PRECISE BIT(12)
> +#define RTL_ECC_BURST_128 GENMASK(1, 0)
> +#define RTL_ECC_DMA_TRIGGER 0x08
> +#define RTL_ECC_OP_DECODE 0
> +#define RTL_ECC_OP_ENCODE BIT(0)
> +#define RTL_ECC_DMA_START 0x0c
> +#define RTL_ECC_DMA_TAG 0x10
> +#define RTL_ECC_STATUS 0x14
> +#define RTL_ECC_CORR_COUNT GENMASK(19, 12)
> +#define RTL_ECC_RESULT BIT(8)
> +#define RTL_ECC_ALL_ONE BIT(4)
> +#define RTL_ECC_OP_STATUS BIT(0)
> +
> +struct rtl_ecc_engine {
> + struct device *dev;
> + struct nand_ecc_engine engine;
> + struct mutex lock;
> + char *buf;
> + dma_addr_t buf_dma;
> + struct regmap *regmap;
> +};
> +
> +struct rtl_ecc_ctx {
> + struct rtl_ecc_engine * rtlc;
> + struct nand_ecc_req_tweak_ctx req_ctx;
> + int steps;
> + int bch_mode;
> + int parity_size;
> + int rc_bitflips;
> + int rc_status;
> +};
> +
> +static const struct regmap_config rtl_ecc_regmap_config = {
> + .reg_bits = 32,
> + .val_bits = 32,
> + .reg_stride = 4,
> +};
> +
> +static inline void *nand_to_ctx(struct nand_device *nand)
> +{
> + return nand->ecc.ctx.priv;
> +}
> +
> +static inline struct rtl_ecc_engine *nand_to_rtlc(struct nand_device *nand)
> +{
> + struct nand_ecc_engine *eng = nand->ecc.engine;
> +
> + return container_of(eng, struct rtl_ecc_engine, engine);
> +}
> +
> +static int rtl_ecc_ooblayout_ecc(struct mtd_info *mtd, int section,
> + struct mtd_oob_region *oobregion)
> +{
> + struct nand_device *nand = mtd_to_nanddev(mtd);
> + struct rtl_ecc_ctx *ctx = nand_to_ctx(nand);
> +
> + if (section < 0 || section >= ctx->steps)
> + return -ERANGE;
> +
> + oobregion->offset = ctx->steps * RTL_ECC_TAG_SIZE + section * ctx->parity_size;
> + oobregion->length = ctx->parity_size;
Please reserve 2 bytes for BBM.
For me it looks like you have a bit free section and a bit ECC
section. If that simplifies your life, you can just return a single
section for both.
> +
> + return 0;
> +}
> +
> +static int rtl_ecc_ooblayout_free(struct mtd_info *mtd, int section,
> + struct mtd_oob_region *oobregion)
> +{
> + struct nand_device *nand = mtd_to_nanddev(mtd);
> + struct rtl_ecc_ctx *ctx = nand_to_ctx(nand);
> +
> + if (section < 0 || section >= ctx->steps)
> + return -ERANGE;
> +
> + /* Tags can hold arbitrary values, so they are free for user data */
> + oobregion->offset = section * RTL_ECC_TAG_SIZE;
> + oobregion->length = RTL_ECC_TAG_SIZE;
> +
> + return 0;
> +}
> +
> +static const struct mtd_ooblayout_ops rtl_ecc_ooblayout_ops = {
> + .ecc = rtl_ecc_ooblayout_ecc,
> + .free = rtl_ecc_ooblayout_free,
> +};
> +
> +static void rtl_ecc_kick_engine(struct rtl_ecc_ctx *ctx, int operation)
> +{
> + struct rtl_ecc_engine *rtlc = ctx->rtlc;
> +
> + regmap_write(rtlc->regmap, RTL_ECC_CFG,
> + ctx->bch_mode | RTL_ECC_BURST_128 | RTL_ECC_DMA_PRECISE);
> +
> + regmap_write(rtlc->regmap, RTL_ECC_DMA_START, rtlc->buf_dma);
> + regmap_write(rtlc->regmap, RTL_ECC_DMA_TAG, rtlc->buf_dma + RTL_ECC_BLOCK_SIZE);
> + regmap_write(rtlc->regmap, RTL_ECC_DMA_TRIGGER, operation);
> +}
> +
> +static void rtl_ecc_wait_for_engine(struct rtl_ecc_ctx *ctx)
> +{
> + struct rtl_ecc_engine *rtlc = ctx->rtlc;
> + int ret, status;
> + bool all_one;
> +
> + /*
> + * The ECC engine needs 6-8 us to encode/decode a BCH6 syndrome for 512 bytes of data
> + * and 6 tag bytes. In case the NAND area has been erased and all data and oob is
> + * set to 0xff, decoding takes 30us (reason unknown). Although the engine can trigger
> + * interrupts when finished, use active polling for now. 12 us maximum wait time has
> + * proven to be a good tradeoff between performance and overhead.
> + */
> +
> + ret = regmap_read_poll_timeout(rtlc->regmap, RTL_ECC_STATUS, status,
> + !(status & RTL_ECC_OP_STATUS), 12, 600);
> + if (ret) {
> + ctx->rc_status = ret;
> + return;
Please make this function return an int and use it in the caller instead
of using an intermediate status variable for that.
> + }
> +
> + all_one = FIELD_GET(RTL_ECC_ALL_ONE, status);
> + ctx->rc_status = FIELD_GET(RTL_ECC_RESULT, status);
> + ctx->rc_bitflips = FIELD_GET(RTL_ECC_CORR_COUNT, status);
> +
> + /* For erased blocks (all bits one) error status can be ignored */
> + if (ctx->rc_status && all_one)
> + ctx->rc_status = 0;
> +}
> +
> +static void rtl_ecc_run_engine(struct rtl_ecc_ctx *ctx, char *data, char *tag,
> + char *parity, int operation)
> +{
> + struct rtl_ecc_engine *rtlc = ctx->rtlc;
> + char *buf_parity = rtlc->buf + RTL_ECC_BLOCK_SIZE + RTL_ECC_TAG_SIZE;
> + char *buf_tag = rtlc->buf + RTL_ECC_BLOCK_SIZE;
> + char *buf_data = rtlc->buf;
> +
> + mutex_lock(&rtlc->lock);
> +
> + memcpy(buf_data, data, RTL_ECC_BLOCK_SIZE);
> + memcpy(buf_tag, tag, RTL_ECC_TAG_SIZE);
> + memcpy(buf_parity, parity, ctx->parity_size);
> +
> + dma_sync_single_for_device(rtlc->dev, rtlc->buf_dma, RTL_ECC_DMA_SIZE, DMA_TO_DEVICE);
> + rtl_ecc_kick_engine(ctx, operation);
> + rtl_ecc_wait_for_engine(ctx);
> + dma_sync_single_for_cpu(rtlc->dev, rtlc->buf_dma, RTL_ECC_DMA_SIZE, DMA_FROM_DEVICE);
> +
> + if (!ctx->rc_status) {
> + memcpy(data, buf_data, RTL_ECC_BLOCK_SIZE);
> + memcpy(tag, buf_tag, RTL_ECC_TAG_SIZE);
> + memcpy(parity, buf_parity, ctx->parity_size);
> + }
> +
> + mutex_unlock(&rtlc->lock);
> +}
> +
> +static int rtl_ecc_prepare_io_req(struct nand_device *nand, struct nand_page_io_req *req)
> +{
> + struct rtl_ecc_engine *rtlc = nand_to_rtlc(nand);
> + struct rtl_ecc_ctx *ctx = nand_to_ctx(nand);
> + char *data, *tag, *parity;
> + int failure = 0;
> +
> + if (req->mode == MTD_OPS_RAW)
> + return 0;
> +
> + nand_ecc_tweak_req(&ctx->req_ctx, req);
> +
> + if (req->type == NAND_PAGE_READ)
> + return 0;
> +
> + tag = req->oobbuf.in;
> + data = req->databuf.in;
> + parity = req->oobbuf.in + ctx->steps * RTL_ECC_TAG_SIZE;
> +
> + for (int i = 0; i < ctx->steps; i++) {
> + rtl_ecc_run_engine(ctx, data, tag, parity, RTL_ECC_OP_ENCODE);
> + failure |= ctx->rc_status;
> +
> + tag += RTL_ECC_TAG_SIZE;
> + data += RTL_ECC_BLOCK_SIZE;
> + parity += ctx->parity_size;
> + }
> +
> + if (unlikely(failure))
> + dev_err(rtlc->dev, "ECC calculation failed\n");
This is not helpful as the other layers will complain if that's
relevant. For instance in case you use this driver with a NAND with read
retry support, it would be very noisy. Please turn this into a dev_dbg
call at most.
> +
> + return failure ? -EBADMSG : 0;
> +}
> +
> +static int rtl_ecc_finish_io_req(struct nand_device *nand, struct nand_page_io_req *req)
> +{
> + struct rtl_ecc_engine *rtlc = nand_to_rtlc(nand);
> + struct rtl_ecc_ctx *ctx = nand_to_ctx(nand);
> + struct mtd_info *mtd = nanddev_to_mtd(nand);
> + char *data, *tag, *parity;
> + unsigned int bitflips = 0;
> + int failure = 0;
> +
> + if (req->mode == MTD_OPS_RAW)
> + return 0;
> +
> + if (req->type == NAND_PAGE_WRITE) {
> + nand_ecc_restore_req(&ctx->req_ctx, req);
> + return 0;
> + }
> +
> + tag = req->oobbuf.in;
> + data = req->databuf.in;
> + parity = req->oobbuf.in + ctx->steps * RTL_ECC_TAG_SIZE;
> +
> + for (int i = 0 ; i < ctx->steps; i++) {
> + rtl_ecc_run_engine(ctx, data, tag, parity, RTL_ECC_OP_DECODE);
> + failure |= ctx->rc_status;
> + mtd->ecc_stats.failed += !!ctx->rc_status;
> + bitflips = max_t(unsigned int, ctx->rc_bitflips, bitflips);
> +
> + tag += RTL_ECC_TAG_SIZE;
> + data += RTL_ECC_BLOCK_SIZE;
> + parity += ctx->parity_size;
> + }
> +
> + nand_ecc_restore_req(&ctx->req_ctx, req);
> +
> + if (unlikely(failure))
> + dev_err(rtlc->dev, "ECC correction failed\n");
> + else if (unlikely(bitflips > 2))
> + dev_warn(rtlc->dev, "%d bitflips detected\n", ctx->rc_bitflips);
Same here. Way too noisy for something (a bitflip) that happens normally.
> +
> + return failure ? -EBADMSG : bitflips;
> +}
> +
> +static int rtl_ecc_check_support(struct nand_device *nand)
> +{
> + struct mtd_info *mtd = nanddev_to_mtd(nand);
> + struct device *dev = nand->ecc.engine->dev;
> +
> + if (mtd->oobsize != RTL_ECC_ALLOWED_OOB_SIZE ||
> + mtd->writesize != RTL_ECC_ALLOWED_PAGE_SIZE) {
> + dev_err(dev, "only flash geometry data=%d, oob=%d supported\n",
> + RTL_ECC_ALLOWED_PAGE_SIZE, RTL_ECC_ALLOWED_OOB_SIZE);
> + return -EINVAL;
> + }
> +
> + if (nand->ecc.user_conf.algo != NAND_ECC_ALGO_BCH ||
> + nand->ecc.user_conf.strength != RTL_ECC_ALLOWED_STRENGTH ||
> + nand->ecc.user_conf.placement != NAND_ECC_PLACEMENT_OOB ||
> + nand->ecc.user_conf.step_size != RTL_ECC_BLOCK_SIZE) {
> + dev_err(dev, "only algo=bch, strength=%d, placement=oob, step=%d supported\n",
> + RTL_ECC_ALLOWED_STRENGTH, RTL_ECC_BLOCK_SIZE);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int rtl_ecc_init_ctx(struct nand_device *nand)
> +{
> + struct nand_ecc_props *conf = &nand->ecc.ctx.conf;
> + struct rtl_ecc_engine *rtlc = nand_to_rtlc(nand);
> + struct mtd_info *mtd = nanddev_to_mtd(nand);
> + int strength = nand->ecc.user_conf.strength;
> + struct device *dev = nand->ecc.engine->dev;
> + struct rtl_ecc_ctx *ctx;
> + int ret;
> +
> + ret = rtl_ecc_check_support(nand);
> + if (ret)
> + return ret;
> +
> + ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> + if (!ctx)
> + return -ENOMEM;
> +
> + nand->ecc.ctx.priv = ctx;
> + mtd_set_ooblayout(mtd, &rtl_ecc_ooblayout_ops);
> +
> + conf->algo = NAND_ECC_ALGO_BCH;
> + conf->strength = strength;
> + conf->step_size = RTL_ECC_BLOCK_SIZE;
> + conf->engine_type = NAND_ECC_ENGINE_TYPE_ON_HOST;
> +
> + ctx->rtlc = rtlc;
> + ctx->steps = mtd->writesize / RTL_ECC_BLOCK_SIZE;
> + ctx->bch_mode = strength == 6 ? RTL_ECC_BCH6 : RTL_ECC_BCH12;
> + ctx->parity_size = strength == 6 ? RTL_ECC_PARITY_SIZE_BCH6 : RTL_ECC_PARITY_SIZE_BCH12;
> +
> + ret = nand_ecc_init_req_tweaking(&ctx->req_ctx, nand);
> + if (ret)
> + return ret;
> +
> + dev_info(dev, "using bch%d with geometry data=%dx%d, tag=%dx6, parity=%dx%d",
> + conf->strength, ctx->steps, conf->step_size,
> + ctx->steps, ctx->steps, ctx->parity_size);
Same, dev_dbg is fine, but otherwise no need.
> +
> + return 0;
> +}
> +
> +static void rtl_ecc_cleanup_ctx(struct nand_device *nand)
> +{
> + struct rtl_ecc_ctx *ctx = nand_to_ctx(nand);
> +
> + if (ctx)
> + nand_ecc_cleanup_req_tweaking(&ctx->req_ctx);
> +}
> +
> +static struct nand_ecc_engine_ops rtl_ecc_engine_ops = {
> + .init_ctx = rtl_ecc_init_ctx,
> + .cleanup_ctx = rtl_ecc_cleanup_ctx,
> + .prepare_io_req = rtl_ecc_prepare_io_req,
> + .finish_io_req = rtl_ecc_finish_io_req,
> +};
> +
> +static int rtl_ecc_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct rtl_ecc_engine *rtlc;
> + void __iomem *base;
> + int ret;
> +
> + rtlc = devm_kzalloc(dev, sizeof(*rtlc), GFP_KERNEL);
> + if (!rtlc)
> + return -ENOMEM;
> +
> + base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(base))
> + return PTR_ERR(base);
> +
> + ret = devm_mutex_init(dev, &rtlc->lock);
> + if (ret)
> + return ret;
> +
> + rtlc->regmap = devm_regmap_init_mmio(dev, base, &rtl_ecc_regmap_config);
> + if (IS_ERR(rtlc->regmap))
> + return PTR_ERR(rtlc->regmap);
> +
> + /*
> + * Focus on simplicity and use a preallocated DMA buffer for data exchange with the
> + * engine. Make it a noncoherent memory model as invalidating/flushing caches is
> + * faster than reading/writing uncached memory.
> + */
I guess it depends on the architecture, but that's fine.
> +
> + rtlc->buf = dma_alloc_noncoherent(dev, RTL_ECC_DMA_SIZE, &rtlc->buf_dma,
> + DMA_BIDIRECTIONAL, GFP_KERNEL);
> + if (IS_ERR(rtlc->buf))
> + return PTR_ERR(rtlc->buf);
> +
> + rtlc->dev = dev;
> + rtlc->engine.dev = dev;
> + rtlc->engine.ops = &rtl_ecc_engine_ops;
> + rtlc->engine.integration = NAND_ECC_ENGINE_INTEGRATION_EXTERNAL;
> +
> + nand_ecc_register_on_host_hw_engine(&rtlc->engine);
> +
> + platform_set_drvdata(pdev, rtlc);
> +
> + return 0;
> +}
> +
> +static void rtl_ecc_remove(struct platform_device *pdev)
> +{
> + struct rtl_ecc_engine *rtlc = platform_get_drvdata(pdev);
> +
> + nand_ecc_unregister_on_host_hw_engine(&rtlc->engine);
> + dma_free_noncoherent(rtlc->dev, RTL_ECC_DMA_SIZE, rtlc->buf, rtlc->buf_dma,
> + DMA_BIDIRECTIONAL);
> +}
> +
> +static const struct of_device_id rtl_ecc_of_ids[] = {
> + {
> + .compatible = "realtek,rtl9301-ecc",
> + },
> + { /* sentinel */ },
> +};
> +
> +static struct platform_driver rtl_ecc_driver = {
> + .driver = {
> + .name = "rtl-nand-ecc-engine",
> + .of_match_table = rtl_ecc_of_ids,
> + },
> + .probe = rtl_ecc_probe,
> + .remove = rtl_ecc_remove,
> +};
> +module_platform_driver(rtl_ecc_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Markus Stockhausen <markus.stockhausen@gmx.de>");
> +MODULE_DESCRIPTION("Realtek NAND hardware ECC controller");
Great work nevertheless! Looking forward to get it upstreamed.
Thanks,
Miquèl
^ permalink raw reply [flat|nested] 14+ messages in thread* AW: [PATCH 2/2] mtd: nand: realtek-ecc: Add Realtek external ECC engine support
2025-08-24 16:51 ` Miquel Raynal
(?)
@ 2025-08-24 19:16 ` markus.stockhausen
2025-08-28 10:07 ` Miquel Raynal
-1 siblings, 1 reply; 14+ messages in thread
From: markus.stockhausen @ 2025-08-24 19:16 UTC (permalink / raw)
To: 'Miquel Raynal'; +Cc: richard, vigneshr, linux-mtd
> Von: Miquel Raynal <miquel.raynal@bootlin.com>
> Gesendet: Sonntag, 24. August 2025 18:51
> ...
> +/*
> + * The Realtek ECC engine has two operation modes.
> + *
> + * - BCH6 : Generate 10 ECC bytes from 512 data bytes plus 6 tag bytes
> + * - BCH12: Generate 20 ECC bytes from 512 data bytes plus 6 tag bytes
>
> What is a tag? I believe it is Realtek wording and we need to find a
> more generic translation.
I have no real idea. They simply call it "tag". To be more precise
- tag (lowercase): means 6 oob bytes for one 512 byte block
- syndrome: means 10 ECC bytes for one 512 byte block.
- TAG (uppercase): means 16 oob bytes (tag+syndrome)
Even the register is named ECDTAR (ECC Controller DMA TAG
address register). For me tag (lowercase) is simply additional
input data that can contain arbitrary content. It can be used
for extended/auxiliary/meta data or simply padding I assume.
Looking at the data on my devices I see that tags are always
filled with 0xff. So it seems as this gets populated after erasing
data and is never overwritten afterwards.
What naming do you advise?
> > + oobregion->offset = ctx->steps * RTL_ECC_TAG_SIZE + section * ctx->parity_size;
> > + oobregion->length = ctx->parity_size;
>
> Please reserve 2 bytes for BBM.
>
> For me it looks like you have a bit free section and a bit ECC
> section. If that simplifies your life, you can just return a single
> section for both.
The first two "tag" bytes seem to be designated for BBM according
to my information. Will adapt the first oob region offset/size.
Do I need to declare the BBM area somehow additionally?
Thanks for the other feedback. Will implement that in v2.
Markus
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: AW: [PATCH 2/2] mtd: nand: realtek-ecc: Add Realtek external ECC engine support
2025-08-24 19:16 ` AW: " markus.stockhausen
@ 2025-08-28 10:07 ` Miquel Raynal
0 siblings, 0 replies; 14+ messages in thread
From: Miquel Raynal @ 2025-08-28 10:07 UTC (permalink / raw)
To: markus.stockhausen; +Cc: richard, vigneshr, linux-mtd
Hi Markus,
On 24/08/2025 at 21:16:39 +02, <markus.stockhausen@gmx.de> wrote:
>> Von: Miquel Raynal <miquel.raynal@bootlin.com>
>> Gesendet: Sonntag, 24. August 2025 18:51
>> ...
>> +/*
>> + * The Realtek ECC engine has two operation modes.
>> + *
>> + * - BCH6 : Generate 10 ECC bytes from 512 data bytes plus 6 tag bytes
>> + * - BCH12: Generate 20 ECC bytes from 512 data bytes plus 6 tag bytes
>>
>> What is a tag? I believe it is Realtek wording and we need to find a
>> more generic translation.
>
> I have no real idea. They simply call it "tag". To be more precise
>
> - tag (lowercase): means 6 oob bytes for one 512 byte block
> - syndrome: means 10 ECC bytes for one 512 byte block.
> - TAG (uppercase): means 16 oob bytes (tag+syndrome)
Ok. Based on your feedback, I believe that might be what we more
commonly call "free bytes". "free" as in "available for the user". In the
OOB area you'll mostly find:
- 2 bytes reserved by MTD to store bad block markers
- Bytes available for the user (end user, or more commonly upper layers
and filesystems. jffs2 does use these free bytes, whereas UBI does not.
* your "tag" and "TAG", difference is probably that "tag" is not ECC
protected (subject to bitflip) whereas "TAG" is part of the
protected area and will be corrected if needed.
- Bytes not available for the user, ie. ECC bytes, which we are being
read/written at a fixed offset by the ECC engine.
> Do I need to declare the BBM area somehow additionally?
No, they are not user accessible nor ECC bytes. They are not exposed by
the ooblayout callbacks we are discussing.
> Thanks for the other feedback. Will implement that in v2.
Cheers!
Good luck,
Miquèl
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 14+ messages in thread