Re: [PATCH v3 13/15] mtd: rawnand: sunxi: Add support for H616 nand controller

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hello Richard,

On 20/10/2025 at 12:13:09 +02, Richard Genoud <richard.genoud@bootlin.com> wrote:

> The H616 nand controller has the same base as A10/A23, with some
> differences:
> - mdma is based on chained buffers
> - its ECC supports up to 80bit per 1024bytes
> - some registers layouts are a bit different, mainly due do the stronger
>   ECC.
> - it uses USER_DATA_LEN registers along USER_DATA registers.
> - it needs a specific clock for ECC and MBUS.
>
> Introduce the basic support, with ECC and scrambling, but without
> DMA/MDMA.
>
> Tested on Whatsminer H616 board (with and without scrambling, ECC)
>
> Signed-off-by: Richard Genoud <richard.genoud@bootlin.com>

...

>  
> +#define NFC_TIMING_CFG2(tCDQSS, tSC, tCLHZ, tCSS, tWC)		\
> +	((((tCDQSS) & 0x1) << 11) | (((tSC) & 0x3) << 12) |	\
> +	 (((tCLHZ) & 0x3) << 14) | (((tCSS) & 0x3) << 16) |	\
> +	 (((tWC) & 0x3) << 18))
> +
>  /* define bit use in NFC_CMD */
>  #define NFC_CMD_LOW_BYTE_MSK	GENMASK(7, 0)
> -#define NFC_CMD_HIGH_BYTE_MSK	GENMASK(15, 8)
> +#define NFC_CMD_HIGH_BYTE_MSK	GENMASK(15, 8)  // 15-10 reserved on H6

Wrong comment type :-)

> +#define NFC_CMD_ADR_NUM_MSK	GENMASK(9, 8)
>  #define NFC_CMD(x)		(x)
>  #define NFC_ADR_NUM_MSK		GENMASK(18, 16)
>  #define NFC_ADR_NUM(x)		(((x) - 1) << 16)
> @@ -122,6 +156,7 @@
>  #define NFC_SEQ			BIT(25)
>  #define NFC_DATA_SWAP_METHOD	BIT(26)
>  #define NFC_ROW_AUTO_INC	BIT(27)
> +#define NFC_H6_SEND_RND_CMD2	BIT(27)
>  #define NFC_SEND_CMD3		BIT(28)
>  #define NFC_SEND_CMD4		BIT(29)
>  #define NFC_CMD_TYPE_MSK	GENMASK(31, 30)
> @@ -133,6 +168,7 @@
>  #define NFC_READ_CMD_MSK	GENMASK(7, 0)
>  #define NFC_RND_READ_CMD0_MSK	GENMASK(15, 8)
>  #define NFC_RND_READ_CMD1_MSK	GENMASK(23, 16)
> +#define NFC_RND_READ_CMD2_MSK	GENMASK(31, 24)

...

> @@ -858,6 +967,8 @@ static int sunxi_nfc_hw_ecc_read_chunk(struct nand_chip *nand,
>  	if (ret)
>  		return ret;
>  
> +	sunxi_nfc_reset_user_data_len(nfc);
> +	sunxi_nfc_set_user_data_len(nfc, 4, 0);

I'm not sure I understand this properly. Why isn't this a fixed setting?
Also, what is 4? It is not obvious to me and my require either a comment
or a define (or maybe a sizeof()).

>  	sunxi_nfc_randomizer_config(nand, page, false);
>  	sunxi_nfc_randomizer_enable(nand);
>  	writel(NFC_DATA_TRANS | NFC_DATA_SWAP_METHOD | NFC_ECC_OP,
> @@ -968,6 +1079,8 @@ static int sunxi_nfc_hw_ecc_read_chunks_dma(struct nand_chip *nand, uint8_t *buf
>  		return ret;
>  
>  	sunxi_nfc_hw_ecc_enable(nand);
> +	sunxi_nfc_reset_user_data_len(nfc);
> +	sunxi_nfc_set_user_data_len(nfc, 4, 0);
>  	sunxi_nfc_randomizer_config(nand, page, false);
>  	sunxi_nfc_randomizer_enable(nand);
>  
> @@ -1100,6 +1213,8 @@ static int sunxi_nfc_hw_ecc_write_chunk(struct nand_chip *nand,
>  
>  	sunxi_nfc_randomizer_config(nand, page, false);
>  	sunxi_nfc_randomizer_enable(nand);
> +	sunxi_nfc_reset_user_data_len(nfc);
> +	sunxi_nfc_set_user_data_len(nfc, 4, 0);
>  	sunxi_nfc_hw_ecc_set_prot_oob_bytes(nand, oob, 0, bbm, page);
>  
>  	writel(NFC_DATA_TRANS | NFC_DATA_SWAP_METHOD |
> @@ -1344,10 +1459,12 @@ static int sunxi_nfc_hw_ecc_write_page_dma(struct nand_chip *nand,
>  	if (ret)
>  		goto pio_fallback;
>  
> +	sunxi_nfc_reset_user_data_len(nfc);
>  	for (i = 0; i < ecc->steps; i++) {
>  		const u8 *oob = nand->oob_poi + (i * (ecc->bytes + 4));
>  
>  		sunxi_nfc_hw_ecc_set_prot_oob_bytes(nand, oob, i, !i, page);
> +		sunxi_nfc_set_user_data_len(nfc, 4, i);

Here you use it differently, maybe a bit of explanation in a comment
could help.

>  	}
>  
>  	nand_prog_page_begin_op(nand, page, 0, NULL, 0);
> @@ -2148,6 +2265,10 @@ static int sunxi_nfc_probe(struct platform_device *pdev)
>  	if (irq < 0)
>  		return irq;
>  
> +	nfc->caps = of_device_get_match_data(dev);
> +	if (!nfc->caps)
> +		return -EINVAL;
> +
>  	nfc->ahb_clk = devm_clk_get_enabled(dev, "ahb");
>  	if (IS_ERR(nfc->ahb_clk)) {
>  		dev_err(dev, "failed to retrieve ahb clk\n");
> @@ -2160,6 +2281,22 @@ static int sunxi_nfc_probe(struct platform_device *pdev)
>  		return PTR_ERR(nfc->mod_clk);
>  	}
>  

...

>  static const struct sunxi_nfc_caps sunxi_nfc_a10_caps = {
>  	.has_ecc_block_512 = true,
> +	.has_ecc_clk = false,
> +	.has_mbus_clk = false,

As you want, but setting these fields (and below) to false is not
strictly required as they will be set to 0 (which means false,
automatically).

>  	.reg_io_data = NFC_REG_A10_IO_DATA,
>  	.reg_ecc_err_cnt = NFC_REG_A10_ECC_ERR_CNT,
>  	.reg_user_data = NFC_REG_A10_USER_DATA,
> @@ -2242,11 +2383,14 @@ static const struct sunxi_nfc_caps sunxi_nfc_a10_caps = {
>  	.dma_maxburst = 4,
>  	.ecc_strengths = sunxi_ecc_strengths_a10,
>  	.nstrengths = ARRAY_SIZE(sunxi_ecc_strengths_a10),
> +	.max_ecc_steps = 16,
>  	.sram_size = 1024,
>  };
>  

> +static const struct sunxi_nfc_caps sunxi_nfc_h616_caps = {
> +	.has_mdma = false, // H616 supports only chained descriptors

Wrong comment type :-)

LGTM otherwise.

Thanks,
Miquèl