[PATCH 3/3] mtd: rawnand: brcmnand: exec_op implementation
dregan at mail.com
dregan at mail.com
Tue Oct 10 21:04:17 PDT 2023
Hi Miquel,
>
> Hello,
>
> dregan at mail.com wrote on Tue, 10 Oct 2023 04:49:41 +0200:
>
> > exec_op implementation for Broadcom STB, Broadband and iProc SoC
> > This adds exec_op and removes the legacy interface. Based on changes
> > proposed by Boris Brezillon.
> >
> > https://github.com/bbrezillon/linux/commit/4ec6f8d8d83f5aaca5d1877f02d48da96d41fcba
> > https://github.com/bbrezillon/linux/commit/11b4acffd761c4928652d7028d19fcd6f45e4696
> >
> > Signed-off-by: David Regan <dregan at mail.com>
> >
...
> > - ret = bcmnand_ctrl_poll_status(ctrl,
> > + ret = bcmnand_ctrl_poll_status(host,
>
> Please make this change in a separate preparation patch.
I will add a 3/4 patch to the series which separates out this change.
...
> > +
> > + if (check_only)
> > + return 0;
>
> You initially reported (in your first submission I guess) a number of
> limitations regarding the number of successive address and data cycles.
> If these limitations are real, they must checked. If you need to do
> that, please get inspiration from:
>
> https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/arasan-nand-controller.c#L904
I believe that was a misunderstanding on my part about how the command
parser was matching a command. There should be no such limitation now
as we can handle all commands.
>
> > +
> > + if (op->deassert_wp)
> > + brcmnand_wp(mtd, 0);
> > +
> > + for (i = 0; i < op->ninstrs; i++) {
> > + instr = &op->instrs[i];
> > +
> > + if ((op->instrs[0].type == NAND_OP_CMD_INSTR) &&
> > + (op->instrs[0].ctx.cmd.opcode == NAND_CMD_STATUS)) {
> > + if (instr->type == NAND_OP_DATA_IN_INSTR) {
>
> I would create two helpers: one to check wether it is a full status
> or reset operation and do these checks outside of this for loop.
> And here handle almost all the cases.
I will move the status and reset checks out of the loop.
...
> >
> > chip->controller = &ctrl->controller;
> >
> This space can go away.
Will do.
>
> > + ctrl->controller.controller_wp = 1;
> > +
> > /*
> > * The bootloader might have configured 16bit mode but
> > * NAND READID command only works in 8bit mode. We force
> > --
> > 2.37.3
> >
> >
>
> Otherwise looks pretty good.
>
> Thanks,
> Miquèl
>
Thanks!
-Dave
More information about the linux-mtd
mailing list