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

[Niravkumar L Rabara <niravkumarlaxmidas.rabara@altera.com>]

On 24/10/2025 3:38 pm, Miquel Raynal wrote:
> Hi Niravkumar,
> 
> On 24/10/2025 at 15:13:06 +08, niravkumarlaxmidas.rabara@altera.com wrote:
> 
>> From: Niravkumar L Rabara <niravkumarlaxmidas.rabara@altera.com>
>>
>> Add support for NV-DDR mode in the Cadence NAND controller driver.
>>
>> Signed-off-by: Niravkumar L Rabara <niravkumarlaxmidas.rabara@altera.com>
>> ---
> 
> Thanks for the patch, very happy to see people implementing this
> interface!

Thanks for the review comments.

> 
> [...]
> 
>> +	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.

I have double checked, the logic is correct. May be I shouldn't use _MIN 
to avoid confusion.
In v2 I will change NVDDR_GATE_CFG_MIN to NVDDR_GATE_CFG_STD.

> 
>> +	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.
Noted.
I will make a separate condition check as below in v2.

-       } else if (chipnr >= 0) {
-               const struct nand_nvddr_timings *nvddr = 
nand_get_nvddr_timings(conf);
+       } else {
+               if (chipnr < 0)
+                       return ret;

+               const struct nand_nvddr_timings *nvddr = 
nand_get_nvddr_timings(conf);
> 
>> +		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