[PATCH v2 2/2] mtd: nand: realtek-ecc: Add Realtek external ECC engine support

[Miquel Raynal]

Hello Markus,

On 28/08/2025 at 10:34:08 -04, Markus Stockhausen <markus.stockhausen at 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 at gmx.de>
> ---
> Changes in v2:
> - include bitfield.h
> - fill mtd->ecc_stats.corrected
> - Reduce chattiness with dev_dbg()
> - Convert return codes from context based to function based
> - Add more documentation about vendor specifications
> - Convert variables from vendor style to kernel style (e.g. tag)

Thanks for the changes, a few more comments and we'll be good.

...

> + * Altogether this gives currently the following block layout.
> + *
> + * +------+------+------+------+-----+------+------+------+------+-----+-----+-----+-----+
> + * |  512 |  512 |  512 |  512 |   2 |    4 |    6 |    6 |    6 |  10 |  10 |  10 |  10 |
> + * +------+------+------+------+-----+------+------+------+------+-----+-----+-----+-----+
> + * | data | data | data | data | BBI | free | free | free | free | ECC | ECC | ECC | ECC |
> + * +------+------+------+------+-----+------+------+------+------+-----+-----+-----+-----+

Thanks, very informative.

...

> +static int rtl_ecc_wait_for_engine(struct rtl_ecc_ctx *ctx)
> +{
> +	struct rtl_ecc_engine *rtlc = ctx->rtlc;
> +	int ret, status, bitflips;
> +	bool all_one;
> +
> +	/*
> +	 * The ECC engine needs 6-8 us to encode/decode a BCH6 syndrome for 512 bytes of data
> +	 * and 6 free 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);

Polling delay matters, but if you reach the timeout you're already in a
faulty situation so the responsiveness is less a concern in this
situation. I fear if the machine is loaded and if the triggers get
delayed somehow you might easily reach the timeout. In general I'd
advocate in favour of a default 1s timeout.

> +	if (ret)
> +		return ret;
> +
> +	ret = FIELD_GET(RTL_ECC_RESULT, status);
> +	all_one = FIELD_GET(RTL_ECC_ALL_ONE, status);

Maybe you should check whether all_one remains true if there are
correctable bitflips in your buffer (you may use nandflipbits for that
purpose). If not, you shall probably spend extra time when checking for
ret checking if there are less bitflips than the ECC controller can
correct compared to ones in the *entire* buffer, which would indicate
that you are really in the presence of an erased page, see:
https://elixir.bootlin.com/linux/v6.16.5/source/drivers/mtd/nand/raw/nand_base.c#L2841

This helper may be moved if you need it.

> +	bitflips = FIELD_GET(RTL_ECC_CORR_COUNT, status);
> +
> +	/* For erased blocks (all bits one) error status can be ignored */
> +	if (ret && all_one)

Can be simplified?

    if (all_one)
        ret = 0;

> +		ret = 0;
> +
> +	return ret ? -EBADMSG : bitflips;
> +}
> +
> +static int rtl_ecc_run_engine(struct rtl_ecc_ctx *ctx, char *data, char *free,
> +			      char *parity, int operation)
> +{
> +	struct rtl_ecc_engine *rtlc = ctx->rtlc;
> +	char *buf_parity = rtlc->buf + RTL_ECC_BLOCK_SIZE + RTL_ECC_FREE_SIZE;
> +	char *buf_free = rtlc->buf + RTL_ECC_BLOCK_SIZE;
> +	char *buf_data = rtlc->buf;
> +	int ret;
> +
> +	mutex_lock(&rtlc->lock);
> +
> +	memcpy(buf_data, data, RTL_ECC_BLOCK_SIZE);
> +	memcpy(buf_free, free, RTL_ECC_FREE_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);
> +	ret = rtl_ecc_wait_for_engine(ctx);
> +	dma_sync_single_for_cpu(rtlc->dev, rtlc->buf_dma, RTL_ECC_DMA_SIZE, DMA_FROM_DEVICE);
> +
> +	if (ret >= 0) {
> +		memcpy(data, buf_data, RTL_ECC_BLOCK_SIZE);
> +		memcpy(free, buf_free, RTL_ECC_FREE_SIZE);
> +		memcpy(parity, buf_parity, ctx->parity_size);
> +	}
> +
> +	mutex_unlock(&rtlc->lock);
> +
> +	return ret;
> +}
> +
> +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, *free, *parity;
> +	int ret = 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;
> +
> +	free = req->oobbuf.in;
> +	data = req->databuf.in;
> +	parity = req->oobbuf.in + ctx->steps * RTL_ECC_FREE_SIZE;
> +
> +	for (int i = 0; i < ctx->steps; i++) {
> +		ret |= rtl_ecc_run_engine(ctx, data, free, parity, RTL_ECC_OP_ENCODE);
> +
> +		free += RTL_ECC_FREE_SIZE;
> +		data += RTL_ECC_BLOCK_SIZE;
> +		parity += ctx->parity_size;
> +	}
> +
> +	if (unlikely(ret))
> +		dev_dbg(rtlc->dev, "ECC calculation failed\n");
> +
> +	return ret ? -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, *free, *parity;
> +	bool failure = false;
> +	int bitflips = 0;
> +	int ret;
> +
> +	if (req->mode == MTD_OPS_RAW)
> +		return 0;
> +
> +	if (req->type == NAND_PAGE_WRITE) {
> +		nand_ecc_restore_req(&ctx->req_ctx, req);
> +		return 0;
> +	}
> +
> +	free = req->oobbuf.in;
> +	data = req->databuf.in;
> +	parity = req->oobbuf.in + ctx->steps * RTL_ECC_FREE_SIZE;
> +
> +	for (int i = 0 ; i < ctx->steps; i++) {
> +		ret = rtl_ecc_run_engine(ctx, data, free, parity, RTL_ECC_OP_DECODE);
> +
> +		if (unlikely(ret < 0)) {
> +			failure = true;
> +			mtd->ecc_stats.failed ++;

Extra space                                  ^

> +		} else {
> +			mtd->ecc_stats.corrected += ret;
> +			bitflips = max_t(unsigned int, bitflips, ret);
> +		}
> +
> +		free += RTL_ECC_FREE_SIZE;
> +		data += RTL_ECC_BLOCK_SIZE;
> +		parity += ctx->parity_size;
> +	}
> +
> +	nand_ecc_restore_req(&ctx->req_ctx, req);
> +
> +	if (unlikely(failure))
> +		dev_dbg(rtlc->dev, "ECC correction failed\n");
> +	else if (unlikely(bitflips > 2))
> +		dev_dbg(rtlc->dev, "%d bitflips detected\n", bitflips);
> +
> +	return failure ? -EBADMSG : bitflips;
> +}

Rest lgtm.

Thanks,
Miquèl