[PATCH v4] mtd: rawnand: brcmnand: legacy exec_op implementation
Álvaro Fernández Rojas
noltari at gmail.com
Sun May 18 13:34:18 PDT 2025
Hi Miquel,
El vie, 16 may 2025 a las 16:32, Miquel Raynal
(<miquel.raynal at bootlin.com>) escribió:
>
> Hello Alvaro,
>
> On 15/05/2025 at 07:07:59 +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>
>
> Since you have a precise list of what is supported and what's not, I'd
> have expected the rest of the behavior to be identical. Are these
> controllers so different in how to program them? Can't we use the
> existing exec_op() implementation without some of the opcodes?
As David confirmed, the low level operation registers aren't working
on v4.0 controllers and they don't even exist on v2.1/2.2 controllers.
BTW, I don't have a precise list of what's supported, I'm just playing
trial and error here in order to get older controllers working again.
>
>
> > +/* Native command conversion */
>
> Should mention "legacy" somewhere I guess, and even what legacy means in
> this context, which is version(controller) < XXX.
Ok, I will add it on v5.
>
> > +static const u8 native_cmd_conv[] = {
> > + [NAND_CMD_READ0] = CMD_NOT_SUPPORTED,
> > + [NAND_CMD_READ1] = CMD_NOT_SUPPORTED,
> > + [NAND_CMD_RNDOUT] = CMD_PARAMETER_CHANGE_COL,
> > + [NAND_CMD_PAGEPROG] = CMD_NOT_SUPPORTED,
> > + [NAND_CMD_READOOB] = CMD_NOT_SUPPORTED,
> > + [NAND_CMD_ERASE1] = CMD_BLOCK_ERASE,
> > + [NAND_CMD_STATUS] = CMD_NOT_SUPPORTED,
> > + [NAND_CMD_SEQIN] = CMD_NOT_SUPPORTED,
> > + [NAND_CMD_RNDIN] = CMD_NOT_SUPPORTED,
> > + [NAND_CMD_READID] = CMD_DEVICE_ID_READ,
> > + [NAND_CMD_ERASE2] = CMD_NULL,
> > + [NAND_CMD_PARAM] = CMD_PARAMETER_READ,
> > + [NAND_CMD_GET_FEATURES] = CMD_NOT_SUPPORTED,
> > + [NAND_CMD_SET_FEATURES] = CMD_NOT_SUPPORTED,
> > + [NAND_CMD_RESET] = CMD_NOT_SUPPORTED,
> > + [NAND_CMD_READSTART] = CMD_NOT_SUPPORTED,
> > + [NAND_CMD_READCACHESEQ] = CMD_NOT_SUPPORTED,
> > + [NAND_CMD_READCACHEEND] = CMD_NOT_SUPPORTED,
> > + [NAND_CMD_RNDOUTSTART] = CMD_NULL,
> > + [NAND_CMD_CACHEDPROG] = CMD_NOT_SUPPORTED,
> > +};
> > +
> > /* Controller feature flags */
> > enum {
> > BRCMNAND_HAS_1K_SECTORS = BIT(0),
> > @@ -237,6 +262,10 @@ struct brcmnand_controller {
> > /* List of NAND hosts (one for each chip-select) */
> > struct list_head host_list;
> >
> > + /* Function to be called from exec_op */
> > + int (*exec_instr)(struct nand_chip *chip,
> > + const struct nand_operation *op);
> > +
> > /* EDU info, per-transaction */
> > const u16 *edu_offsets;
> > void __iomem *edu_base;
> > @@ -2490,14 +2519,152 @@ static int brcmnand_op_is_reset(const struct nand_operation *op)
> > return 0;
> > }
> >
> > +static int brcmnand_exec_instructions(struct nand_chip *chip,
> > + const struct nand_operation *op)
> > +{
> > + struct brcmnand_host *host = nand_get_controller_data(chip);
> > + unsigned int i;
> > + int ret = 0;
> > +
> > + for (i = 0; i < op->ninstrs; i++) {
> > + ret = brcmnand_exec_instr(host, i, op);
> > + if (ret)
> > + break;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int brcmnand_exec_instructions_legacy(struct nand_chip *chip,
> > + const struct nand_operation *op)
> > +{
> > + struct mtd_info *mtd = nand_to_mtd(chip);
> > + struct brcmnand_host *host = nand_get_controller_data(chip);
> > + struct brcmnand_controller *ctrl = host->ctrl;
> > + const struct nand_op_instr *instr;
> > + unsigned int i, j;
> > + u8 cmd = CMD_NULL, last_cmd = CMD_NULL;
> > + int ret = 0;
> > + u64 last_addr;
> > +
> > + for (i = 0; i < op->ninstrs; i++) {
> > + instr = &op->instrs[i];
> > +
> > + if (instr->type == NAND_OP_CMD_INSTR) {
>
> Could we use a switch case here? Seems more appropriate.
I would rather keep this as an if/else in order to prevent more
indentation or having to define the variables on top.
>
> > + cmd = native_cmd_conv[instr->ctx.cmd.opcode];
> > + if (cmd == CMD_NOT_SUPPORTED) {
> > + dev_err(ctrl->dev, "unsupported cmd=%d\n",
> > + instr->ctx.cmd.opcode);
> > + ret = -EOPNOTSUPP;
> > + break;
> > + }
> > + } else if (instr->type == NAND_OP_ADDR_INSTR) {
> > + u64 addr = 0;
> > +
> > + if (cmd == CMD_NULL)
> > + continue;
> > +
> > + if (instr->ctx.addr.naddrs > 8) {
> > + dev_err(ctrl->dev, "unsupported naddrs=%u\n",
> > + instr->ctx.addr.naddrs);
> > + ret = -EOPNOTSUPP;
> > + break;
> > + }
> > +
> > + for (j = 0; j < instr->ctx.addr.naddrs; j++)
> > + addr |= (instr->ctx.addr.addrs[j]) << (j << 3);
> > +
> > + if (cmd == CMD_BLOCK_ERASE)
> > + addr <<= chip->page_shift;
> > + else if (cmd == CMD_PARAMETER_CHANGE_COL)
> > + addr &= ~((u64)(FC_BYTES - 1));
> > +
> > + brcmnand_set_cmd_addr(mtd, addr);
> > + brcmnand_send_cmd(host, cmd);
> > + last_addr = addr;
> > + last_cmd = cmd;
> > + cmd = CMD_NULL;
> > + brcmnand_waitfunc(chip);
>
> Waitfunc is a legacy interface, are you sure this is the correct
> function call here?
I guess this is correct because brcmnand_waitfunc is still used after
each brcmnand_send_cmd call that we still have on the current code...
>
> > +
> > + if (last_cmd == CMD_PARAMETER_READ ||
> > + last_cmd == CMD_PARAMETER_CHANGE_COL) {
> > + /* Copy flash cache word-wise */
> > + u32 *flash_cache = (u32 *)ctrl->flash_cache;
> > +
> > + brcmnand_soc_data_bus_prepare(ctrl->soc, true);
> > +
> > + /*
> > + * Must cache the FLASH_CACHE now, since changes in
> > + * SECTOR_SIZE_1K may invalidate it
> > + */
> > + for (j = 0; j < FC_WORDS; j++)
> > + /*
> > + * Flash cache is big endian for parameter pages, at
> > + * least on STB SoCs
> > + */
> > + flash_cache[j] = be32_to_cpu(brcmnand_read_fc(ctrl, j));
> > +
> > + brcmnand_soc_data_bus_unprepare(ctrl->soc, true);
> > + }
> > + } else if (instr->type == NAND_OP_DATA_IN_INSTR) {
> > + u8 *in = instr->ctx.data.buf.in;
> > +
> > + if (last_cmd == CMD_DEVICE_ID_READ) {
> > + u32 val;
> > +
> > + if (instr->ctx.data.len > 8) {
> > + dev_err(ctrl->dev, "unsupported len=%u\n",
> > + instr->ctx.data.len);
> > + ret = -EOPNOTSUPP;
> > + break;
> > + }
> > +
> > + for (j = 0; j < instr->ctx.data.len; j++) {
> > + if (j == 0)
> > + val = brcmnand_read_reg(ctrl, BRCMNAND_ID);
> > + else if (j == 4)
> > + val = brcmnand_read_reg(ctrl, BRCMNAND_ID_EXT);
> > +
> > + in[j] = (val >> (24 - ((j % 4) << 3))) & 0xff;
> > + }
> > + } else if (last_cmd == CMD_PARAMETER_READ ||
> > + last_cmd == CMD_PARAMETER_CHANGE_COL) {
> > + u64 addr;
> > + u32 offs;
> > +
> > + for (j = 0; j < instr->ctx.data.len; j++) {
> > + addr = last_addr + j;
> > + offs = addr & (FC_BYTES - 1);
> > +
> > + if (j > 0 && offs == 0)
> > + nand_change_read_column_op(chip, addr, NULL, 0,
> > + false);
> > +
> > + in[j] = ctrl->flash_cache[offs];
> > + }
> > + }
> > + } else if (instr->type == NAND_OP_WAITRDY_INSTR) {
> > + ret = bcmnand_ctrl_poll_status(host, NAND_CTRL_RDY, NAND_CTRL_RDY, 0);
> > + if (ret)
> > + break;
> > + } else {
> > + dev_err(ctrl->dev, "unsupported instruction type: %d\n", instr->type);
> > + ret = -EINVAL;
>
> EOPNOTSUPP I guess?
Ok, I will change that on v5.
>
> > + break;
> > + }
> > + }
> > +
> > + return ret;
> > +}
> > +
> > static int brcmnand_exec_op(struct nand_chip *chip,
> > const struct nand_operation *op,
> > bool check_only)
> > {
> > struct brcmnand_host *host = nand_get_controller_data(chip);
> > + struct brcmnand_controller *ctrl = host->ctrl;
> > struct mtd_info *mtd = nand_to_mtd(chip);
> > u8 *status;
> > - unsigned int i;
> > int ret = 0;
> >
> > if (check_only)
> [adding one more context line]
> return 0;
>
> There is a lot that is unsupported, and this is okay, but you need to
> control all these earlier and implement a function that does all the
> checks when exec_op() is called with the check_only parameter set (just
> above). The support for the old SoCs *must* not return 0 by default and
> instead check the validity of the op when requested.
Ok, I will add that in v5.
>
> > @@ -2525,11 +2692,7 @@ static int brcmnand_exec_op(struct nand_chip *chip,
> > if (op->deassert_wp)
> > brcmnand_wp(mtd, 0);
> >
> > - for (i = 0; i < op->ninstrs; i++) {
> > - ret = brcmnand_exec_instr(host, i, op);
> > - if (ret)
> > - break;
> > - }
> > + ret = ctrl->exec_instr(chip, op);
> >
> > if (op->deassert_wp)
> > brcmnand_wp(mtd, 1);
> > @@ -3142,6 +3305,12 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
> > if (ret)
> > goto err;
> >
> > + /* Only v5.0+ controllers have low level ops support */
> > + if (ctrl->nand_version >= 0x0500)
>
> Did I just read that 4 or 4.1 was the correct boundary instead of 5?
David has confirmed that this is correct.
>
> > + ctrl->exec_instr = brcmnand_exec_instructions;
> > + else
> > + ctrl->exec_instr = brcmnand_exec_instructions_legacy;
> > +
> > /*
> > * Most chips have this cache at a fixed offset within 'nand' block.
> > * Some must specify this region separately.
>
> Thanks,
> Miquèl
More information about the linux-mtd
mailing list