[PATCH 2/3] mtd: rawnand: rzn1: Add new NAND controller driver

[Geert Uytterhoeven]

Hi Miquel,

On Fri, Nov 19, 2021 at 10:23 AM Miquel Raynal
<miquel.raynal at bootlin.com> wrote:
> geert at linux-m68k.org wrote on Fri, 19 Nov 2021 09:55:53 +0100:
> > On Thu, Nov 18, 2021 at 12:19 PM Miquel Raynal
> > <miquel.raynal at bootlin.com> wrote:
> > > 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 at bootlin.com>

> > > --- /dev/null
> > > +++ b/drivers/mtd/nand/raw/rzn1-nand-controller.c
> >
> > > +static int rzn1_read_subpage_hw_ecc(struct nand_chip *chip, u32 req_offset,
> > > +                                   u32 req_len, u8 *bufpoi, int page)
> > > +{
> > > +       struct rzn1_nfc *nfc = to_rzn1_nfc(chip->controller);
> > > +       struct mtd_info *mtd = nand_to_mtd(chip);
> > > +       struct rzn1_nand_chip *rzn1_nand = to_rzn1_nand(chip);
> > > +       unsigned int cs = to_nfc_cs(rzn1_nand);
> > > +       unsigned int page_off = round_down(req_offset, chip->ecc.size);
> > > +       unsigned int real_len = round_up(req_offset + req_len - page_off,
> > > +                                        chip->ecc.size);
> > > +       unsigned int start_chunk = page_off / chip->ecc.size;
> > > +       unsigned int nchunks = real_len / chip->ecc.size;
> > > +       unsigned int ecc_off = 2 + (start_chunk * chip->ecc.bytes);
> > > +       struct rzn1_op rop = {
> > > +               .command = COMMAND_INPUT_SEL_AHBS | COMMAND_0(NAND_CMD_READ0) |
> > > +                          COMMAND_2(NAND_CMD_READSTART) | COMMAND_FIFO_SEL |
> > > +                          COMMAND_SEQ_READ_PAGE,
> > > +               .addr0_row = page,
> > > +               .addr0_col = page_off,
> > > +               .len = real_len,
> > > +               .ecc_offset = ECC_OFFSET(mtd->writesize + ecc_off),
> > > +       };
> > > +       unsigned int max_bitflips = 0;
> > > +       u32 ecc_stat;
> > > +       int bf, ret, i;
> >
> > unsigned int i
>
> Strangely I'm used to always set my loop indexes as signed integers,
> but I'll happily change that everywhere in the driver before
> re-submitting.

It depends.  Some of the upper bounds are signed, as dictated by some
field in a struct.

> > > +static int rzn1_nfc_probe(struct platform_device *pdev)
> > > +{
> > > +       struct rzn1_nfc *nfc;
> > > +       int irq, ret;
> > > +
> > > +       nfc = devm_kzalloc(&pdev->dev, sizeof(*nfc), GFP_KERNEL);
> > > +       if (!nfc)
> > > +               return -ENOMEM;
> > > +
> > > +       nfc->dev = &pdev->dev;
> > > +       nand_controller_init(&nfc->controller);
> > > +       nfc->controller.ops = &rzn1_nfc_ops;
> > > +       INIT_LIST_HEAD(&nfc->chips);
> > > +       init_completion(&nfc->complete);
> > > +
> > > +       nfc->regs = devm_platform_ioremap_resource(pdev, 0);
> > > +       if (IS_ERR(nfc->regs))
> > > +               return PTR_ERR(nfc->regs);
> > > +
> > > +       rzn1_nfc_dis_interrupts(nfc);
> > > +       irq = platform_get_irq(pdev, 0);
> >
> > platform_get_irq_optional()
> >
> > > +       if (irq < 0) {
> >
> > What if this is a real error, or -EPROBE_DEFER?
>
> If it's a real error I believe we should still fallback to polling? Or
> do you prefer to only use polling on a fixed condition?

It's debatable: in this case, you have the option to fallback to polling if
it is a real error, in other drivers you haven't.  If it fails for real here,
it will probably fail for real in other drivers, too.

> > > +       {} /* sentinel */
> > > +};
> > > +MODULE_DEVICE_TABLE(of, nfc_id_table);
> > > +
> > > +static struct platform_driver rzn1_nfc_driver = {
> > > +       .driver = {
> > > +               .name   = "renesas-nfc",
> >
> > Perhaps s/nfc/nandc/ everywhere, to avoid confusion with the other nfc?
>
> There are many NAND controller drivers that are abbreviated with nfc
> because it's short and easy to write while still precise, but I have no
> issue rewording nfc into nandc if you prefer.

My main worry is that we ever get a "renesas-nfc" driver for Near
Field Communication.  Both drivers will have the same name, which
will still work with DT (board files are dead), but the output from
dev_*() may be confusing.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds