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

Álvaro Fernández Rojas
2025-05-23T18:09:36Z

Hi Miquèl,

El vie, 23 may 2025 a las 16:42, Miquel Raynal
(<miquel.raynal@bootlin.com>) escribió:
>
> On 21/05/2025 at 10:03:25 +02, Álvaro Fernández Rojas <noltari@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@gmail.com>
> > Reviewed-by: David Regan <dregan@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.