[PATCH v3] mtd: rawnand: brcmnand: legacy exec_op implementation
David Regan
dregan at broadcom.com
Fri May 16 18:16:37 PDT 2025
Hi Álvaro,
On Thu, May 15, 2025 at 4:59 PM David Regan <dregan at broadcom.com> wrote:
>
> Hi Álvaro,
>
> On Thu, May 15, 2025 at 12:52 AM Florian Fainelli
> <florian.fainelli at broadcom.com> wrote:
> >
> >
> >
> > On 5/15/2025 7:06 AM, Álvaro Fernández Rojas wrote:
> > > Hi William
> > >
> > > El jue, 15 may 2025 a las 3:42, William Zhang
> > > (<william.zhang at broadcom.com>) escribió:
> > >>
> > >> Hi Alvaro,
> > >>
> > >>> -----Original Message-----
> > >>> From: Álvaro Fernández Rojas <noltari at gmail.com>
> > >>> Sent: Tuesday, May 13, 2025 11:24 PM
> > >>> To: linux-mtd at lists.infradead.org; dregan at broadcom.com;
> > >>> miquel.raynal at bootlin.com; bcm-kernel-feedback-list at broadcom.com;
> > >>> florian.fainelli at broadcom.com; rafal at milecki.pl;
> > >>> computersforpeace at gmail.com; kamal.dasu at broadcom.com;
> > >>> dan.beygelman at broadcom.com; william.zhang at broadcom.com;
> > >>> frieder.schrempf at kontron.de; linux-kernel at vger.kernel.org;
> > >>> vigneshr at ti.com;
> > >>> richard at nod.at; bbrezillon at kernel.org; kdasu.kdev at gmail.com;
> > >>> jaimeliao.tw at gmail.com; kilobyte at angband.pl; jonas.gorski at gmail.com;
> > >>> dgcbueu at gmail.com
> > >>> Cc: Álvaro Fernández Rojas <noltari at gmail.com>
> > >>> Subject: [PATCH v3] mtd: rawnand: brcmnand: legacy exec_op implementation
> > >>>
> > >>> 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>
> > >>> ---
> > >>> drivers/mtd/nand/raw/brcmnand/brcmnand.c | 178
> > >>> ++++++++++++++++++++++-
> > >>> 1 file changed, 172 insertions(+), 6 deletions(-)
> > >>>
> > >>> 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.
> > >>>
> > >>> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > >>> b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > >>> index 17f6d9723df9..f4fabe7ffd9d 100644
> > >>> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > >>> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > >>> @@ -65,6 +65,7 @@ module_param(wp_on, int, 0444);
> > >>> #define CMD_PARAMETER_READ 0x0e
> > >>> #define CMD_PARAMETER_CHANGE_COL 0x0f
> > >>> #define CMD_LOW_LEVEL_OP 0x10
> > >>> +#define CMD_NOT_SUPPORTED 0xff
> > >>>
> > >>> struct brcm_nand_dma_desc {
> > >>> u32 next_desc;
> > >>> @@ -199,6 +200,30 @@ static const u16 flash_dma_regs_v4[] = {
> > >>> [FLASH_DMA_CURRENT_DESC_EXT] = 0x34,
> > >>> };
> > >>>
> > >>> +/* Native command conversion */
> > >>> +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,
> > >> Do we not need to support nand_status_op()?
> > >
> > > We do, but NAND_CMD_STATUS and NAND_CMD_RESET are handled in brcmnand_exec_op():
> > > https://github.com/torvalds/linux/blob/546bce579204685a0b204beebab98c3aa496e651/drivers/mtd/nand/raw/brcmnand/brcmnand.c#L2506-L2523
> > >
> > >
> > >
> > >>
> > >>> + [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,149 @@ 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) {
> > >>> + 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);
> > >>> +
> > >>> + 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);
> > >>> + } else {
> > >>> + dev_err(ctrl->dev, "unsupported instruction type: %d\n",
> > >>> instr->type);
> > >>> + ret = -EINVAL;
> > >>> + }
> > >>> + }
> > >>> +
> > >>> + 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)
> > >>> @@ -2525,11 +2689,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;
> > >>> - }
> > >>> + ctrl->exec_instr(chip, op);
> > >>>
> > >>> if (op->deassert_wp)
> > >>> brcmnand_wp(mtd, 1);
> > >>> @@ -3142,6 +3302,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)
>
> We can probably change this to >= 0x0400 since as Florian mentioned
> LLOP was added
> with version 4.
Sorry we should probably leave this cutoff as it is, version 5, see below..
>
> > >>> + 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.
> > >>> --
> > >>> 2.39.5
> > >
> > > BTW, can anyone from Broadcom confirm any of the following?
> > > 1. There are no low level registers on v2.1 and v2.2 controllers.
> >
> > Correct.
> >
> > > 2. Do low level registers exist on v4.0 controllers? They are defined
> > > on 63268_map_part.h but the NAND drivers I could find never use them.
> >
> > They exist. The changelog for the NAND controller indicates that
> > starting from v4.0 onwards, the NAND LL operation is supported.
> >
> > > 3. Are the low level registers bugged on v4.0 controllers?
> > > 4. Should the low level registers be handled differently on v4.0 controllers?
> > > The issue is that trying to use them on v4.0 controllers for
> > > GET_FEATURES would leave the NAND controller in a weird state that
> > > results in hangs or timeouts while trying to read the NAND.
> > > This happens on the Sercomm H500-s, which is a BCM63268 with a
> > > Macronix NAND that tries to unlock through ONFI.
>
I have confirmed that low level operation registers do not work for
version 4 NAND controller in 63268.
> I don't yet know the answer to this and not sure exactly which NAND
> part you are using
> but if you have just a regular standard NAND that doesn't do anything
> unusual does that
> work with a 63268 through the non-legacy brcmnand_exec_op
> brcmnand_exec_instructions
> path (>= 5) that you split out?
>
> Does the data that comes back from LLOP operations look legitimate?
>
> >
> > That I do not know however, William and David hopefully do know.
> > --
> > Florian
> >
>
> -Dave
-Dave
More information about the linux-mtd
mailing list