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

[Niravkumar L Rabara]

On 24/10/2025 3:38 pm, Miquel Raynal wrote:
> Hi Niravkumar,
>
> On 24/10/2025 at 15:13:06 +08, niravkumarlaxmidas.rabara@xxxxxxxxxx wrote:
>
> > From: Niravkumar L Rabara <niravkumarlaxmidas.rabara@xxxxxxxxxx>
> >
> > Add support for NV-DDR mode in the Cadence NAND controller driver.
> >
> > Signed-off-by: Niravkumar L Rabara <niravkumarlaxmidas.rabara@xxxxxxxxxx>
> > ---
>
> 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