Hi Phil,
phil.edworthy@xxxxxxxxxxx wrote on Fri, 19 Nov 2021 15:04:55 +0000:
> Hi Miquel,
>
> Thanks for your patches.
>
> > Introduce Renesas RZ/N1x NAND controller driver which supports:
> > - All ONFI timing modes
> > - Different configurations of its internal ECC controller
> > - On-die (not tested) and software ECC support
> > - Several chips (not tested)
> > - Subpage accesses
> > - DMA and PIO
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
> > ---
> It might be worth mentioning that this controller was originally provided
> by Evatronix, then Cadence bought the Evatronix IP business.
True I've added this in the commit log as well as the main header.
> > + rzn1_nand->tim_gen_seq2 =
> > + TIM_GEN_SEQ2_D8(TO_CYCLES64(sdr->tRR_min + sdr->tREA_max,
> > period_ns)) |
> > + TIM_GEN_SEQ2_D9(TO_CYCLES64(sdr->tRR_min, period_ns)) |
> > + TIM_GEN_SEQ2_D10(TO_CYCLES64(cle - cyc, period_ns)) |
> > + TIM_GEN_SEQ2_D11(TO_CYCLES64(bef_dly, period_ns));
> In our out-of-tree driver for this controller [1], we allowed time for
> the signal to propagate through the RZ/N1 io buffers; We just added 5ns
> to all the timings. Don't you need to do something similar? I vaguely
> recall that some values programmed to 0 caused issues.
I have not experienced any issues yet, I don't think with these
calculations there are many values set to 0 anyway. But thanks for the
feedback, if anybody reports errors with these timings on another board
then we'll know where to look.
> > +static int rzn1_nand_chip_init(struct rzn1_nfc *nfc, struct device_node
> ...
> > + for (i = 0; i < nsels; i++) {
> > + ret = of_property_read_u32_index(np, "reg", i, &cs);
> > + if (ret) {
> > + dev_err(nfc->dev, "Incomplete reg property (%d)\n",
> > + ret);
> > + return ret;
> > + }
> Check cs is <= nr of bits in assigned_cs.
I believe you mean that I should check that we don't request
non-existing CS values (< 4). I've just added such a check, thanks. The
fact that a CS might be selected twice is handled by the test_and_set()
call right after.
> > +static int rzn1_nfc_probe(struct platform_device *pdev) {
> ...
> > + nfc->regs = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(nfc->regs))
> > + return PTR_ERR(nfc->regs);
> > +
> > + rzn1_nfc_dis_interrupts(nfc);
> You need to enable the clock before writing to the registers.
Absolutely, thanks for the remainder.
> > +static struct platform_driver rzn1_nfc_driver = {
> > + .driver = {
> > + .name = "renesas-nfc",
> nit: whitespace
>
> btw, there was an attempt to upstream a driver for this IP a few years back, see [2]
Didn't know about this attempt :)
>
> Thanks
> Phil
>
> [1] https://github.com/renesas-rz/rzn1_linux/blob/rzn1-stable-v4.19/drivers/mtd/nand/raw/evatronix_nand.c
> [2] https://patchwork.ozlabs.org/patch/629096/
>
>
>
Thanks,
Miquèl
