linux-kernel - Re: [PATCH] mtd: rawnand: cadence: Add support for NV-DDR interface mode

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87bjlwrbqd.fsf@bootlin.com>
Date: Fri, 24 Oct 2025 09:38:02 +0200
From: Miquel Raynal <miquel.raynal@...tlin.com>
To: niravkumarlaxmidas.rabara@...era.com
Cc: richard@....at,  vigneshr@...com,  linux-mtd@...ts.infradead.org,
  linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mtd: rawnand: cadence: Add support for NV-DDR interface
 mode

Hi Niravkumar,

On 24/10/2025 at 15:13:06 +08, niravkumarlaxmidas.rabara@...era.com wrote:

> From: Niravkumar L Rabara <niravkumarlaxmidas.rabara@...era.com>
>
> Add support for NV-DDR mode in the Cadence NAND controller driver.
>
> Signed-off-by: Niravkumar L Rabara <niravkumarlaxmidas.rabara@...era.com>
> ---

Thanks for the patch, very happy to see people implementing this
interface!

[...]

> +	if (dll_phy_gate_open_delay > NVDDR_GATE_CFG_MIN)
> +		ie_start = NVDDR_GATE_CFG_MIN;

Can you double check here? I would expect < instead of > given that you
compare with something you named "minimum". Maybe it is legitimate, just
warning.

> +	else
> +		ie_start = dll_phy_gate_open_delay;
> +
> +	dll_phy_rd_delay = ((nvddr->tDQSCK_max + board_delay) +
> +				(clk_period / 2)) / clk_period;
> +	if (dll_phy_rd_delay <= NVDDR_PHY_RD_DELAY)
> +		rd_del_sel = dll_phy_rd_delay + 2;
> +	else
> +		rd_del_sel = NVDDR_PHY_RD_DELAY_MAX;
> +

[...]

> +static int
> +cadence_nand_setup_interface(struct nand_chip *chip, int chipnr,
> +			     const struct nand_interface_config *conf)
> +{
> +	int ret = 0;
> +
> +	if (nand_interface_is_sdr(conf)) {
> +		const struct nand_sdr_timings *sdr = nand_get_sdr_timings(conf);
> +
> +		if (IS_ERR(sdr))
> +			return PTR_ERR(sdr);
> +
> +		ret = cadence_nand_setup_sdr_interface(chip, sdr);
> +	} else if (chipnr >= 0) {

This isn't very clear. Please make it a separate condition if you think
you must handle this case. Otherwise you're mixing it with the SDR
vs. NVDDR choice, and that's misleading.

> +		const struct nand_nvddr_timings *nvddr = nand_get_nvddr_timings(conf);
> +
> +		if (IS_ERR(nvddr))
> +			return PTR_ERR(nvddr);
> +
> +		ret = cadence_nand_setup_nvddr_interface(chip, nvddr);
> +	}
> +	return ret;
> +}
> +
>  static int cadence_nand_attach_chip(struct nand_chip *chip)
>  {
>  	struct cdns_nand_ctrl *cdns_ctrl = to_cdns_nand_ctrl(chip->controller);

Otherwise looks good to me!
Miquèl