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

[Richard GENOUD <richard.genoud@bootlin.com>]

Le 22/10/2025 à 11:18, Miquel Raynal a écrit :
> 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 :-)
ok

> 
>> +#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()).

Yes, indeed. There are '4' all over the place, maybe a bit of tidy up is 
needed in a separate patch.
(and some explanations)

I'll resend a v4 with that.

> 
>>   	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.
> 
yes, definitely

>>   	}
>>   
>>   	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).
> 
yes, I'll remove them.

>>   	.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 :-)
I'll drop this line also.

> 
> LGTM otherwise.
> 
> Thanks,
> Miquèl
Thanks!

-- 
Richard Genoud, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com