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

David Regan
2025-05-16T00:00:14Z

Hi Álvaro,

On Thu, May 15, 2025 at 12:52 AM Florian Fainelli
<florian.fainelli@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@broadcom.com>) escribió:
> >>
> >> Hi Alvaro,
> >>
> >>> -----Original Message-----
> >>> From: Álvaro Fernández Rojas <noltari@gmail.com>
> >>> Sent: Tuesday, May 13, 2025 11:24 PM
> >>> To: linux-mtd@lists.infradead.org; dregan@broadcom.com;
> >>> miquel.raynal@bootlin.com; bcm-kernel-feedback-list@broadcom.com;
> >>> florian.fainelli@broadcom.com; rafal@milecki.pl;
> >>> computersforpeace@gmail.com; kamal.dasu@broadcom.com;
> >>> dan.beygelman@broadcom.com; william.zhang@broadcom.com;
> >>> frieder.schrempf@kontron.de; linux-kernel@vger.kernel.org;
> >>> vigneshr@ti.com;
> >>> richard@nod.at; bbrezillon@kernel.org; kdasu.kdev@gmail.com;
> >>> jaimeliao.tw@gmail.com; kilobyte@angband.pl; jonas.gorski@gmail.com;
> >>> dgcbueu@gmail.com
> >>> Cc: Álvaro Fernández Rojas <noltari@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@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.

> >>> + 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 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