| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <795aa141-e977-4628-826f-f65715afaf9d@bootlin.com>
Date: Tue, 28 Oct 2025 08:30:13 +0100
From: Richard GENOUD <richard.genoud@...tlin.com>
To: Miquel Raynal <miquel.raynal@...tlin.com>
Cc: Richard Weinberger <richard@....at>, Vignesh Raghavendra
<vigneshr@...com>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Chen-Yu Tsai <wens@...e.org>,
Jernej Skrabec <jernej.skrabec@...il.com>,
Samuel Holland <samuel@...lland.org>,
Uwe Kleine-König <u.kleine-koenig@...libre.com>,
Wentao Liang <vulab@...as.ac.cn>, Johan Hovold <johan@...nel.org>,
Maxime Ripard <mripard@...nel.org>,
Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
linux-mtd@...ts.infradead.org, devicetree@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 13/15] mtd: rawnand: sunxi: Add support for H616 nand
controller
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@...tlin.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@...tlin.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
Powered by blists - more mailing lists