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

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

[Álvaro Fernández Rojas]

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,
+	[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)
+		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


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Jonas Gorski]

Hi,

On Wed, May 14, 2025 at 8:23 AM Á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>
> ---
>  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,
> +       [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);

Should here be a check for success and break/abort otherwise so you
don't continue if there are more instructions?

> +               } else {
> +                       dev_err(ctrl->dev, "unsupported instruction type: %d\n", instr->type);
> +                       ret = -EINVAL;

Should here be a break so you don't continue if there are more instructions?

> +               }
> +       }
> +
> +       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);

You are ignoring the return value now, so ret will stay at 0 even if
exec_instr() returns an error.

>
>         if (op->deassert_wp)
>                 brcmnand_wp(mtd, 1);

Best regards,
Jonas

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[William Zhang]

[-- Attachment #1.1: Type: text/plain, Size: 9588 bytes --]

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()?

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

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4199 bytes --]

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Álvaro Fernández Rojas]

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

Best regards,
Álvaro.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Florian Fainelli]

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

That I do not know however, William and David hopefully do know.
-- 
Florian


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[David Regan]

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[David Regan]

Hi Álvaro,

On Thu, May 15, 2025 at 4:59 PM David Regan <dregan@broadcom.com> wrote:
>
> 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.

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/