[PATCH v5] mtd: rawnand: brcmnand: legacy exec_op implementation

[Álvaro Fernández Rojas]

Hi Miquèl,

El vie, 23 may 2025 a las 16:42, Miquel Raynal
(<miquel.raynal at bootlin.com>) escribió:
>
> On 21/05/2025 at 10:03:25 +02, Álvaro Fernández Rojas <noltari at gmail.com> wrote:
>
> > Commit 3c8260ce7663 ("mtd: rawnand: brcmnand: exec_op implementation")
> > removed legacy interface functions, breaking < v5.0 controllers support.
> > In order to fix older controllers we need to add an alternative exec_op
> > implementation which doesn't rely on low level registers.
> >
> > Fixes: 3c8260ce7663 ("mtd: rawnand: brcmnand: exec_op implementation")
> > Signed-off-by: Álvaro Fernández Rojas <noltari at gmail.com>
> > Reviewed-by: David Regan <dregan at broadcom.com>
> > ---
> >  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 222 ++++++++++++++++++++++-
> >  1 file changed, 215 insertions(+), 7 deletions(-)
> >
> >  v5: add changes requested by Miquèl Raynal:
> >   - Mention and explain legacy in native_cmd_conv.
> >   - EOPNOTSUPP instead of EINVAL for instr->type else.
> >   - Implement missing check_only functionality.
> >
> >  v4: add changes requested by Jonas Gorski:
> >   - Add missing breaks in brcmnand_exec_instructions_legacy.
> >   - Restore missing ret assignment in brcmnand_exec_op.
> >
> >  v3: add changes requested by Florian and other improvements:
> >   - Add associative array for native command conversion.
> >   - Add function pointer to brcmnand_controller for exec_instr
> >     functionality.
> >   - Fix CMD_BLOCK_ERASE address.
> >   - Drop NAND_CMD_READOOB support.
> >
> >  v2: multiple improvements:
> >   - Use proper native commands for checks.
> >   - Fix NAND_CMD_PARAM/NAND_CMD_RNDOUT addr calculation.
> >   - Remove host->last_addr usage.
> >   - Remove sector_size_1k since it only applies to v5.0+ controllers.
> >   - Remove brcmnand_wp since it doesn't exist for < v5.0 controllers.
> >   - Use j instead of i for flash_cache loop.
> >
>
> ...
>
> > +static int brcmnand_check_instructions_legacy(struct nand_chip *chip,
> > +                                           const struct nand_operation *op)
> > +{
> > +     const struct nand_op_instr *instr;
> > +     unsigned int i;
> > +     u8 cmd;
> > +
> > +     for (i = 0; i < op->ninstrs; i++) {
> > +             instr = &op->instrs[i];
> > +
> > +             switch (instr->type) {
> > +             case NAND_OP_CMD_INSTR:
> > +                     cmd = native_cmd_conv[instr->ctx.cmd.opcode];
> > +                     if (cmd == CMD_NOT_SUPPORTED)
> > +                             return -EOPNOTSUPP;
> > +                     break;
> > +             case NAND_OP_ADDR_INSTR:
> > +             case NAND_OP_DATA_IN_INSTR:
>
> No NAND_OP_DATA_OUT_INSTR?

AFAIK, the legacy functions were only using it for
NAND_CMD_SET_FEATURES, which we don't support:
https://github.com/torvalds/linux/blob/c86b63b82fde4f96ee94dde827a5f28ff5adeb57/drivers/mtd/nand/raw/brcmnand/brcmnand.c#L1922-L1938

The other uses I could find are already covered by our
chip->ecc.read/write functions.

In any case I've tested the patch for reading, erasing and writing the
NAND and so far I haven't found any unsupported error apart from
NAND_CMD_GET_FEATURES with a Macronix NAND in the Sercom H500-s
(BCM63268).
I believe it's used for unlocking the NAND, which isn't needed in that device.

>
> > +             case NAND_OP_WAITRDY_INSTR:
> > +                     break;
> > +             default:
> > +                     return -EOPNOTSUPP;
> > +             }
> > +     }
> > +
> > +     return 0;
> > +}
>
> Rest lgtm.
>
> Thanks,
> Miquèl

Best regards,
Álvaro.