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

[PATCH v5] 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>
Reviewed-by: David Regan <dregan@broadcom.com>
---
 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 222 ++++++++++++++++++++++-
 1 file changed, 215 insertions(+), 7 deletions(-)

 v5: add changes requested by Miquèl Raynal:
  - Mention and explain legacy in native_cmd_conv.
  - EOPNOTSUPP instead of EINVAL for instr->type else.
  - Implement missing check_only functionality.

 v4: add changes requested by Jonas Gorski:
  - Add missing breaks in brcmnand_exec_instructions_legacy.
  - Restore missing ret assignment in brcmnand_exec_op.

 v3: add changes requested by Florian and other improvements:
  - Add associative array for native command conversion.
  - Add function pointer to brcmnand_controller for exec_instr
    functionality.
  - Fix CMD_BLOCK_ERASE address.
  - Drop NAND_CMD_READOOB support.

 v2: multiple improvements:
  - Use proper native commands for checks.
  - Fix NAND_CMD_PARAM/NAND_CMD_RNDOUT addr calculation.
  - Remove host->last_addr usage.
  - Remove sector_size_1k since it only applies to v5.0+ controllers.
  - Remove brcmnand_wp since it doesn't exist for < v5.0 controllers.
  - Use j instead of i for flash_cache loop.

diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index 299dd2bca5b4..5ed79ffa271c 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 for legacy controllers (< v5.0) */
+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,12 @@ struct brcmnand_controller {
 	/* List of NAND hosts (one for each chip-select) */
 	struct list_head host_list;
 
+	/* Functions to be called from exec_op */
+	int (*check_instr)(struct nand_chip *chip,
+			   const struct nand_operation *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;
@@ -2478,18 +2509,190 @@ static int brcmnand_op_is_reset(const struct nand_operation *op)
 	return 0;
 }
 
+static int brcmnand_check_instructions(struct nand_chip *chip,
+				       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_check_instructions_legacy(struct nand_chip *chip,
+					      const struct nand_operation *op)
+{
+	const struct nand_op_instr *instr;
+	unsigned int i;
+	u8 cmd;
+
+	for (i = 0; i < op->ninstrs; i++) {
+		instr = &op->instrs[i];
+
+		switch (instr->type) {
+		case NAND_OP_CMD_INSTR:
+			cmd = native_cmd_conv[instr->ctx.cmd.opcode];
+			if (cmd == CMD_NOT_SUPPORTED)
+				return -EOPNOTSUPP;
+			break;
+		case NAND_OP_ADDR_INSTR:
+		case NAND_OP_DATA_IN_INSTR:
+		case NAND_OP_WAITRDY_INSTR:
+			break;
+		default:
+			return -EOPNOTSUPP;
+		}
+	}
+
+	return 0;
+}
+
+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);
+			if (ret)
+				break;
+		} else {
+			dev_err(ctrl->dev, "unsupported instruction type: %d\n", instr->type);
+			ret = -EOPNOTSUPP;
+			break;
+		}
+	}
+
+	return ret;
+}
+
 static int brcmnand_exec_op(struct nand_chip *chip,
 			    const struct nand_operation *op,
 			    bool check_only)
 {
 	struct brcmnand_host *host = nand_get_controller_data(chip);
+	struct brcmnand_controller *ctrl = host->ctrl;
 	struct mtd_info *mtd = nand_to_mtd(chip);
 	u8 *status;
-	unsigned int i;
 	int ret = 0;
 
 	if (check_only)
-		return 0;
+		return ctrl->check_instr(chip, op);
 
 	if (brcmnand_op_is_status(op)) {
 		status = op->instrs[1].ctx.data.buf.in;
@@ -2513,11 +2716,7 @@ static int brcmnand_exec_op(struct nand_chip *chip,
 	if (op->deassert_wp)
 		brcmnand_wp(mtd, 0);
 
-	for (i = 0; i < op->ninstrs; i++) {
-		ret = brcmnand_exec_instr(host, i, op);
-		if (ret)
-			break;
-	}
+	ret = ctrl->exec_instr(chip, op);
 
 	if (op->deassert_wp)
 		brcmnand_wp(mtd, 1);
@@ -3130,6 +3329,15 @@ 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->check_instr = brcmnand_check_instructions;
+		ctrl->exec_instr = brcmnand_exec_instructions;
+	} else {
+		ctrl->check_instr = brcmnand_check_instructions_legacy;
+		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

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

[Florian Fainelli]

On 5/21/25 01:03, Álvaro Fernández Rojas wrote:
> Commit 3c8260ce7663 ("mtd: rawnand: brcmnand: exec_op implementation")
> removed legacy interface functions, breaking < v5.0 controllers support.
> In order to fix older controllers we need to add an alternative exec_op
> implementation which doesn't rely on low level registers.
> 
> Fixes: 3c8260ce7663 ("mtd: rawnand: brcmnand: exec_op implementation")
> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> Reviewed-by: David Regan <dregan@broadcom.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

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

[William Zhang]

[-- Attachment #1: Type: text/plain, Size: 11302 bytes --]

> -----Original Message-----
> From: Álvaro Fernández Rojas <noltari@gmail.com>
> Sent: Wednesday, May 21, 2025 1:03 AM
> 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 v5] 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>
> Reviewed-by: David Regan <dregan@broadcom.com>
> ---
>  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 222
> ++++++++++++++++++++++-
>  1 file changed, 215 insertions(+), 7 deletions(-)
>
>  v5: add changes requested by Miquèl Raynal:
>   - Mention and explain legacy in native_cmd_conv.
>   - EOPNOTSUPP instead of EINVAL for instr->type else.
>   - Implement missing check_only functionality.
>
>  v4: add changes requested by Jonas Gorski:
>   - Add missing breaks in brcmnand_exec_instructions_legacy.
>   - Restore missing ret assignment in brcmnand_exec_op.
>
>  v3: add changes requested by Florian and other improvements:
>   - Add associative array for native command conversion.
>   - Add function pointer to brcmnand_controller for exec_instr
>     functionality.
>   - Fix CMD_BLOCK_ERASE address.
>   - Drop NAND_CMD_READOOB support.
>
>  v2: multiple improvements:
>   - Use proper native commands for checks.
>   - Fix NAND_CMD_PARAM/NAND_CMD_RNDOUT addr calculation.
>   - Remove host->last_addr usage.
>   - Remove sector_size_1k since it only applies to v5.0+ controllers.
>   - Remove brcmnand_wp since it doesn't exist for < v5.0 controllers.
>   - Use j instead of i for flash_cache loop.
>
> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> index 299dd2bca5b4..5ed79ffa271c 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 for legacy controllers (< v5.0) */
> +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,12 @@ struct brcmnand_controller {
>  	/* List of NAND hosts (one for each chip-select) */
>  	struct list_head host_list;
>
> +	/* Functions to be called from exec_op */
> +	int (*check_instr)(struct nand_chip *chip,
> +			   const struct nand_operation *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;
> @@ -2478,18 +2509,190 @@ static int brcmnand_op_is_reset(const struct
> nand_operation *op)
>  	return 0;
>  }
>
> +static int brcmnand_check_instructions(struct nand_chip *chip,
> +				       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_check_instructions_legacy(struct nand_chip *chip,
> +					      const struct nand_operation *op)
> +{
> +	const struct nand_op_instr *instr;
> +	unsigned int i;
> +	u8 cmd;
> +
> +	for (i = 0; i < op->ninstrs; i++) {
> +		instr = &op->instrs[i];
> +
> +		switch (instr->type) {
> +		case NAND_OP_CMD_INSTR:
> +			cmd = native_cmd_conv[instr->ctx.cmd.opcode];
> +			if (cmd == CMD_NOT_SUPPORTED)
> +				return -EOPNOTSUPP;
> +			break;
> +		case NAND_OP_ADDR_INSTR:
> +		case NAND_OP_DATA_IN_INSTR:
> +		case NAND_OP_WAITRDY_INSTR:
> +			break;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +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);
> +			if (ret)
> +				break;
> +		} else {
> +			dev_err(ctrl->dev, "unsupported instruction type: %d\n",
> instr->type);
> +			ret = -EOPNOTSUPP;
> +			break;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
>  static int brcmnand_exec_op(struct nand_chip *chip,
>  			    const struct nand_operation *op,
>  			    bool check_only)
>  {
>  	struct brcmnand_host *host = nand_get_controller_data(chip);
> +	struct brcmnand_controller *ctrl = host->ctrl;
>  	struct mtd_info *mtd = nand_to_mtd(chip);
>  	u8 *status;
> -	unsigned int i;
>  	int ret = 0;
>
>  	if (check_only)
> -		return 0;
> +		return ctrl->check_instr(chip, op);
>
>  	if (brcmnand_op_is_status(op)) {
>  		status = op->instrs[1].ctx.data.buf.in;
> @@ -2513,11 +2716,7 @@ static int brcmnand_exec_op(struct nand_chip *chip,
>  	if (op->deassert_wp)
>  		brcmnand_wp(mtd, 0);
>
> -	for (i = 0; i < op->ninstrs; i++) {
> -		ret = brcmnand_exec_instr(host, i, op);
> -		if (ret)
> -			break;
> -	}
> +	ret = ctrl->exec_instr(chip, op);
>
>  	if (op->deassert_wp)
>  		brcmnand_wp(mtd, 1);
> @@ -3130,6 +3329,15 @@ 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->check_instr = brcmnand_check_instructions;
> +		ctrl->exec_instr = brcmnand_exec_instructions;
> +	} else {
> +		ctrl->check_instr = brcmnand_check_instructions_legacy;
> +		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

Reviewed-by: William Zhang <william.zhang@broadcom.com>

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

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

[David Regan]

On Wed, May 21, 2025 at 1:03 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>
> Reviewed-by: David Regan <dregan@broadcom.com>
> ---
>  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 222 ++++++++++++++++++++++-
>  1 file changed, 215 insertions(+), 7 deletions(-)
>
>  v5: add changes requested by Miquèl Raynal:
>   - Mention and explain legacy in native_cmd_conv.
>   - EOPNOTSUPP instead of EINVAL for instr->type else.
>   - Implement missing check_only functionality.
>
>  v4: add changes requested by Jonas Gorski:
>   - Add missing breaks in brcmnand_exec_instructions_legacy.
>   - Restore missing ret assignment in brcmnand_exec_op.
>
>  v3: add changes requested by Florian and other improvements:
>   - Add associative array for native command conversion.
>   - Add function pointer to brcmnand_controller for exec_instr
>     functionality.
>   - Fix CMD_BLOCK_ERASE address.
>   - Drop NAND_CMD_READOOB support.
>
>  v2: multiple improvements:
>   - Use proper native commands for checks.
>   - Fix NAND_CMD_PARAM/NAND_CMD_RNDOUT addr calculation.
>   - Remove host->last_addr usage.
>   - Remove sector_size_1k since it only applies to v5.0+ controllers.
>   - Remove brcmnand_wp since it doesn't exist for < v5.0 controllers.
>   - Use j instead of i for flash_cache loop.
>
> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> index 299dd2bca5b4..5ed79ffa271c 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 for legacy controllers (< v5.0) */
> +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,12 @@ struct brcmnand_controller {
>         /* List of NAND hosts (one for each chip-select) */
>         struct list_head host_list;
>
> +       /* Functions to be called from exec_op */
> +       int (*check_instr)(struct nand_chip *chip,
> +                          const struct nand_operation *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;
> @@ -2478,18 +2509,190 @@ static int brcmnand_op_is_reset(const struct nand_operation *op)
>         return 0;
>  }
>
> +static int brcmnand_check_instructions(struct nand_chip *chip,
> +                                      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_check_instructions_legacy(struct nand_chip *chip,
> +                                             const struct nand_operation *op)
> +{
> +       const struct nand_op_instr *instr;
> +       unsigned int i;
> +       u8 cmd;
> +
> +       for (i = 0; i < op->ninstrs; i++) {
> +               instr = &op->instrs[i];
> +
> +               switch (instr->type) {
> +               case NAND_OP_CMD_INSTR:
> +                       cmd = native_cmd_conv[instr->ctx.cmd.opcode];
> +                       if (cmd == CMD_NOT_SUPPORTED)
> +                               return -EOPNOTSUPP;
> +                       break;
> +               case NAND_OP_ADDR_INSTR:
> +               case NAND_OP_DATA_IN_INSTR:
> +               case NAND_OP_WAITRDY_INSTR:
> +                       break;
> +               default:
> +                       return -EOPNOTSUPP;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
> +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);
> +                       if (ret)
> +                               break;
> +               } else {
> +                       dev_err(ctrl->dev, "unsupported instruction type: %d\n", instr->type);
> +                       ret = -EOPNOTSUPP;
> +                       break;
> +               }
> +       }
> +
> +       return ret;
> +}
> +
>  static int brcmnand_exec_op(struct nand_chip *chip,
>                             const struct nand_operation *op,
>                             bool check_only)
>  {
>         struct brcmnand_host *host = nand_get_controller_data(chip);
> +       struct brcmnand_controller *ctrl = host->ctrl;
>         struct mtd_info *mtd = nand_to_mtd(chip);
>         u8 *status;
> -       unsigned int i;
>         int ret = 0;
>
>         if (check_only)
> -               return 0;
> +               return ctrl->check_instr(chip, op);
>
>         if (brcmnand_op_is_status(op)) {
>                 status = op->instrs[1].ctx.data.buf.in;
> @@ -2513,11 +2716,7 @@ static int brcmnand_exec_op(struct nand_chip *chip,
>         if (op->deassert_wp)
>                 brcmnand_wp(mtd, 0);
>
> -       for (i = 0; i < op->ninstrs; i++) {
> -               ret = brcmnand_exec_instr(host, i, op);
> -               if (ret)
> -                       break;
> -       }
> +       ret = ctrl->exec_instr(chip, op);
>
>         if (op->deassert_wp)
>                 brcmnand_wp(mtd, 1);
> @@ -3130,6 +3329,15 @@ 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->check_instr = brcmnand_check_instructions;
> +               ctrl->exec_instr = brcmnand_exec_instructions;
> +       } else {
> +               ctrl->check_instr = brcmnand_check_instructions_legacy;
> +               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
>

Reviewed-by: David Regan <dregan@broadcom.com>

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

[Miquel Raynal]

On 21/05/2025 at 10:03:25 +02, Álvaro Fernández Rojas <noltari@gmail.com> wrote:

> Commit 3c8260ce7663 ("mtd: rawnand: brcmnand: exec_op implementation")
> removed legacy interface functions, breaking < v5.0 controllers support.
> In order to fix older controllers we need to add an alternative exec_op
> implementation which doesn't rely on low level registers.
>
> Fixes: 3c8260ce7663 ("mtd: rawnand: brcmnand: exec_op implementation")
> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> Reviewed-by: David Regan <dregan@broadcom.com>
> ---
>  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 222 ++++++++++++++++++++++-
>  1 file changed, 215 insertions(+), 7 deletions(-)
>
>  v5: add changes requested by Miquèl Raynal:
>   - Mention and explain legacy in native_cmd_conv.
>   - EOPNOTSUPP instead of EINVAL for instr->type else.
>   - Implement missing check_only functionality.
>
>  v4: add changes requested by Jonas Gorski:
>   - Add missing breaks in brcmnand_exec_instructions_legacy.
>   - Restore missing ret assignment in brcmnand_exec_op.
>
>  v3: add changes requested by Florian and other improvements:
>   - Add associative array for native command conversion.
>   - Add function pointer to brcmnand_controller for exec_instr
>     functionality.
>   - Fix CMD_BLOCK_ERASE address.
>   - Drop NAND_CMD_READOOB support.
>
>  v2: multiple improvements:
>   - Use proper native commands for checks.
>   - Fix NAND_CMD_PARAM/NAND_CMD_RNDOUT addr calculation.
>   - Remove host->last_addr usage.
>   - Remove sector_size_1k since it only applies to v5.0+ controllers.
>   - Remove brcmnand_wp since it doesn't exist for < v5.0 controllers.
>   - Use j instead of i for flash_cache loop.
>

...

> +static int brcmnand_check_instructions_legacy(struct nand_chip *chip,
> +					      const struct nand_operation *op)
> +{
> +	const struct nand_op_instr *instr;
> +	unsigned int i;
> +	u8 cmd;
> +
> +	for (i = 0; i < op->ninstrs; i++) {
> +		instr = &op->instrs[i];
> +
> +		switch (instr->type) {
> +		case NAND_OP_CMD_INSTR:
> +			cmd = native_cmd_conv[instr->ctx.cmd.opcode];
> +			if (cmd == CMD_NOT_SUPPORTED)
> +				return -EOPNOTSUPP;
> +			break;
> +		case NAND_OP_ADDR_INSTR:
> +		case NAND_OP_DATA_IN_INSTR:

No NAND_OP_DATA_OUT_INSTR?

> +		case NAND_OP_WAITRDY_INSTR:
> +			break;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +	}
> +
> +	return 0;
> +}

Rest lgtm.

Thanks,
Miquèl

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

[David Regan]

Hi Álvaro,

On Wed, May 21, 2025 at 1:03 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>
> Reviewed-by: David Regan <dregan@broadcom.com>
> ---
>  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 222 ++++++++++++++++++++++-
>  1 file changed, 215 insertions(+), 7 deletions(-)
>
>  v5: add changes requested by Miquèl Raynal:
>   - Mention and explain legacy in native_cmd_conv.
>   - EOPNOTSUPP instead of EINVAL for instr->type else.
>   - Implement missing check_only functionality.
>
>  v4: add changes requested by Jonas Gorski:
>   - Add missing breaks in brcmnand_exec_instructions_legacy.
>   - Restore missing ret assignment in brcmnand_exec_op.
>
>  v3: add changes requested by Florian and other improvements:
>   - Add associative array for native command conversion.
>   - Add function pointer to brcmnand_controller for exec_instr
>     functionality.
>   - Fix CMD_BLOCK_ERASE address.
>   - Drop NAND_CMD_READOOB support.
>
>  v2: multiple improvements:
>   - Use proper native commands for checks.
>   - Fix NAND_CMD_PARAM/NAND_CMD_RNDOUT addr calculation.
>   - Remove host->last_addr usage.
>   - Remove sector_size_1k since it only applies to v5.0+ controllers.
>   - Remove brcmnand_wp since it doesn't exist for < v5.0 controllers.
>   - Use j instead of i for flash_cache loop.
>
> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> index 299dd2bca5b4..5ed79ffa271c 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 for legacy controllers (< v5.0) */
> +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,12 @@ struct brcmnand_controller {
>         /* List of NAND hosts (one for each chip-select) */
>         struct list_head host_list;
>
> +       /* Functions to be called from exec_op */
> +       int (*check_instr)(struct nand_chip *chip,
> +                          const struct nand_operation *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;
> @@ -2478,18 +2509,190 @@ static int brcmnand_op_is_reset(const struct nand_operation *op)
>         return 0;
>  }
>
> +static int brcmnand_check_instructions(struct nand_chip *chip,
> +                                      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_check_instructions_legacy(struct nand_chip *chip,
> +                                             const struct nand_operation *op)
> +{
> +       const struct nand_op_instr *instr;
> +       unsigned int i;
> +       u8 cmd;
> +
> +       for (i = 0; i < op->ninstrs; i++) {
> +               instr = &op->instrs[i];
> +
> +               switch (instr->type) {
> +               case NAND_OP_CMD_INSTR:
> +                       cmd = native_cmd_conv[instr->ctx.cmd.opcode];
> +                       if (cmd == CMD_NOT_SUPPORTED)
> +                               return -EOPNOTSUPP;
> +                       break;
> +               case NAND_OP_ADDR_INSTR:
> +               case NAND_OP_DATA_IN_INSTR:
> +               case NAND_OP_WAITRDY_INSTR:
> +                       break;
> +               default:
> +                       return -EOPNOTSUPP;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
> +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++)

Forgot to mention, not critical, but if you end up releasing a follow
on patch could you
please put braces around this part?

> +                                       /*
> +                                        * Flash cache is big endian for parameter pages, at
> +                                        * least on STB SoCs
> +                                        */
> +                                       flash_cache[j] = be32_to_cpu(brcmnand_read_fc(ctrl, j));
> +
> +                               brcmnand_soc_data_bus_unprepare(ctrl->soc, true);
> +                       }
> +               } else if (instr->type == NAND_OP_DATA_IN_INSTR) {
> +                       u8 *in = instr->ctx.data.buf.in;
> +
> +                       if (last_cmd == CMD_DEVICE_ID_READ) {
> +                               u32 val;
> +
> +                               if (instr->ctx.data.len > 8) {
> +                                       dev_err(ctrl->dev, "unsupported len=%u\n",
> +                                               instr->ctx.data.len);
> +                                       ret = -EOPNOTSUPP;
> +                                       break;
> +                               }
> +
> +                               for (j = 0; j < instr->ctx.data.len; j++) {
> +                                       if (j == 0)
> +                                               val = brcmnand_read_reg(ctrl, BRCMNAND_ID);
> +                                       else if (j == 4)
> +                                               val = brcmnand_read_reg(ctrl, BRCMNAND_ID_EXT);
> +
> +                                       in[j] = (val >> (24 - ((j % 4) << 3))) & 0xff;
> +                               }
> +                       } else if (last_cmd == CMD_PARAMETER_READ ||
> +                                  last_cmd == CMD_PARAMETER_CHANGE_COL) {
> +                               u64 addr;
> +                               u32 offs;
> +
> +                               for (j = 0; j < instr->ctx.data.len; j++) {
> +                                       addr = last_addr + j;
> +                                       offs = addr & (FC_BYTES - 1);
> +
> +                                       if (j > 0 && offs == 0)
> +                                               nand_change_read_column_op(chip, addr, NULL, 0,
> +                                                                          false);
> +
> +                                       in[j] = ctrl->flash_cache[offs];
> +                               }
> +                       }
> +               } else if (instr->type == NAND_OP_WAITRDY_INSTR) {
> +                       ret = bcmnand_ctrl_poll_status(host, NAND_CTRL_RDY, NAND_CTRL_RDY, 0);
> +                       if (ret)
> +                               break;
> +               } else {
> +                       dev_err(ctrl->dev, "unsupported instruction type: %d\n", instr->type);
> +                       ret = -EOPNOTSUPP;
> +                       break;
> +               }
> +       }
> +
> +       return ret;
> +}
> +
>  static int brcmnand_exec_op(struct nand_chip *chip,
>                             const struct nand_operation *op,
>                             bool check_only)
>  {
>         struct brcmnand_host *host = nand_get_controller_data(chip);
> +       struct brcmnand_controller *ctrl = host->ctrl;
>         struct mtd_info *mtd = nand_to_mtd(chip);
>         u8 *status;
> -       unsigned int i;
>         int ret = 0;
>
>         if (check_only)
> -               return 0;
> +               return ctrl->check_instr(chip, op);
>
>         if (brcmnand_op_is_status(op)) {
>                 status = op->instrs[1].ctx.data.buf.in;
> @@ -2513,11 +2716,7 @@ static int brcmnand_exec_op(struct nand_chip *chip,
>         if (op->deassert_wp)
>                 brcmnand_wp(mtd, 0);
>
> -       for (i = 0; i < op->ninstrs; i++) {
> -               ret = brcmnand_exec_instr(host, i, op);
> -               if (ret)
> -                       break;
> -       }
> +       ret = ctrl->exec_instr(chip, op);
>
>         if (op->deassert_wp)
>                 brcmnand_wp(mtd, 1);
> @@ -3130,6 +3329,15 @@ 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->check_instr = brcmnand_check_instructions;
> +               ctrl->exec_instr = brcmnand_exec_instructions;
> +       } else {
> +               ctrl->check_instr = brcmnand_check_instructions_legacy;
> +               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
>

Reviewed-by: David Regan <dregan@broadcom.com>

Thanks!

-Dave

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

[Álvaro Fernández Rojas]

Hi Miquèl,

El vie, 23 may 2025 a las 16:42, Miquel Raynal
(<miquel.raynal@bootlin.com>) escribió:
>
> On 21/05/2025 at 10:03:25 +02, Álvaro Fernández Rojas <noltari@gmail.com> wrote:
>
> > Commit 3c8260ce7663 ("mtd: rawnand: brcmnand: exec_op implementation")
> > removed legacy interface functions, breaking < v5.0 controllers support.
> > In order to fix older controllers we need to add an alternative exec_op
> > implementation which doesn't rely on low level registers.
> >
> > Fixes: 3c8260ce7663 ("mtd: rawnand: brcmnand: exec_op implementation")
> > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> > Reviewed-by: David Regan <dregan@broadcom.com>
> > ---
> >  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 222 ++++++++++++++++++++++-
> >  1 file changed, 215 insertions(+), 7 deletions(-)
> >
> >  v5: add changes requested by Miquèl Raynal:
> >   - Mention and explain legacy in native_cmd_conv.
> >   - EOPNOTSUPP instead of EINVAL for instr->type else.
> >   - Implement missing check_only functionality.
> >
> >  v4: add changes requested by Jonas Gorski:
> >   - Add missing breaks in brcmnand_exec_instructions_legacy.
> >   - Restore missing ret assignment in brcmnand_exec_op.
> >
> >  v3: add changes requested by Florian and other improvements:
> >   - Add associative array for native command conversion.
> >   - Add function pointer to brcmnand_controller for exec_instr
> >     functionality.
> >   - Fix CMD_BLOCK_ERASE address.
> >   - Drop NAND_CMD_READOOB support.
> >
> >  v2: multiple improvements:
> >   - Use proper native commands for checks.
> >   - Fix NAND_CMD_PARAM/NAND_CMD_RNDOUT addr calculation.
> >   - Remove host->last_addr usage.
> >   - Remove sector_size_1k since it only applies to v5.0+ controllers.
> >   - Remove brcmnand_wp since it doesn't exist for < v5.0 controllers.
> >   - Use j instead of i for flash_cache loop.
> >
>
> ...
>
> > +static int brcmnand_check_instructions_legacy(struct nand_chip *chip,
> > +                                           const struct nand_operation *op)
> > +{
> > +     const struct nand_op_instr *instr;
> > +     unsigned int i;
> > +     u8 cmd;
> > +
> > +     for (i = 0; i < op->ninstrs; i++) {
> > +             instr = &op->instrs[i];
> > +
> > +             switch (instr->type) {
> > +             case NAND_OP_CMD_INSTR:
> > +                     cmd = native_cmd_conv[instr->ctx.cmd.opcode];
> > +                     if (cmd == CMD_NOT_SUPPORTED)
> > +                             return -EOPNOTSUPP;
> > +                     break;
> > +             case NAND_OP_ADDR_INSTR:
> > +             case NAND_OP_DATA_IN_INSTR:
>
> No NAND_OP_DATA_OUT_INSTR?

AFAIK, the legacy functions were only using it for
NAND_CMD_SET_FEATURES, which we don't support:
https://github.com/torvalds/linux/blob/c86b63b82fde4f96ee94dde827a5f28ff5adeb57/drivers/mtd/nand/raw/brcmnand/brcmnand.c#L1922-L1938

The other uses I could find are already covered by our
chip->ecc.read/write functions.

In any case I've tested the patch for reading, erasing and writing the
NAND and so far I haven't found any unsupported error apart from
NAND_CMD_GET_FEATURES with a Macronix NAND in the Sercom H500-s
(BCM63268).
I believe it's used for unlocking the NAND, which isn't needed in that device.

>
> > +             case NAND_OP_WAITRDY_INSTR:
> > +                     break;
> > +             default:
> > +                     return -EOPNOTSUPP;
> > +             }
> > +     }
> > +
> > +     return 0;
> > +}
>
> Rest lgtm.
>
> Thanks,
> Miquèl

Best regards,
Álvaro.

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

[Miquel Raynal]

On 23/05/2025 at 20:08:56 +02, Álvaro Fernández Rojas <noltari@gmail.com> wrote:

> Hi Miquèl,
>
> El vie, 23 may 2025 a las 16:42, Miquel Raynal
> (<miquel.raynal@bootlin.com>) escribió:
>>
>> On 21/05/2025 at 10:03:25 +02, Álvaro Fernández Rojas <noltari@gmail.com> wrote:
>>
>> > Commit 3c8260ce7663 ("mtd: rawnand: brcmnand: exec_op implementation")
>> > removed legacy interface functions, breaking < v5.0 controllers support.
>> > In order to fix older controllers we need to add an alternative exec_op
>> > implementation which doesn't rely on low level registers.
>> >
>> > Fixes: 3c8260ce7663 ("mtd: rawnand: brcmnand: exec_op implementation")
>> > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
>> > Reviewed-by: David Regan <dregan@broadcom.com>
>> > ---
>> >  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 222 ++++++++++++++++++++++-
>> >  1 file changed, 215 insertions(+), 7 deletions(-)
>> >
>> >  v5: add changes requested by Miquèl Raynal:
>> >   - Mention and explain legacy in native_cmd_conv.
>> >   - EOPNOTSUPP instead of EINVAL for instr->type else.
>> >   - Implement missing check_only functionality.
>> >
>> >  v4: add changes requested by Jonas Gorski:
>> >   - Add missing breaks in brcmnand_exec_instructions_legacy.
>> >   - Restore missing ret assignment in brcmnand_exec_op.
>> >
>> >  v3: add changes requested by Florian and other improvements:
>> >   - Add associative array for native command conversion.
>> >   - Add function pointer to brcmnand_controller for exec_instr
>> >     functionality.
>> >   - Fix CMD_BLOCK_ERASE address.
>> >   - Drop NAND_CMD_READOOB support.
>> >
>> >  v2: multiple improvements:
>> >   - Use proper native commands for checks.
>> >   - Fix NAND_CMD_PARAM/NAND_CMD_RNDOUT addr calculation.
>> >   - Remove host->last_addr usage.
>> >   - Remove sector_size_1k since it only applies to v5.0+ controllers.
>> >   - Remove brcmnand_wp since it doesn't exist for < v5.0 controllers.
>> >   - Use j instead of i for flash_cache loop.
>> >
>>
>> ...
>>
>> > +static int brcmnand_check_instructions_legacy(struct nand_chip *chip,
>> > +                                           const struct nand_operation *op)
>> > +{
>> > +     const struct nand_op_instr *instr;
>> > +     unsigned int i;
>> > +     u8 cmd;
>> > +
>> > +     for (i = 0; i < op->ninstrs; i++) {
>> > +             instr = &op->instrs[i];
>> > +
>> > +             switch (instr->type) {
>> > +             case NAND_OP_CMD_INSTR:
>> > +                     cmd = native_cmd_conv[instr->ctx.cmd.opcode];
>> > +                     if (cmd == CMD_NOT_SUPPORTED)
>> > +                             return -EOPNOTSUPP;
>> > +                     break;
>> > +             case NAND_OP_ADDR_INSTR:
>> > +             case NAND_OP_DATA_IN_INSTR:
>>
>> No NAND_OP_DATA_OUT_INSTR?
>
> AFAIK, the legacy functions were only using it for
> NAND_CMD_SET_FEATURES, which we don't support:
> https://github.com/torvalds/linux/blob/c86b63b82fde4f96ee94dde827a5f28ff5adeb57/drivers/mtd/nand/raw/brcmnand/brcmnand.c#L1922-L1938
>
> The other uses I could find are already covered by our
> chip->ecc.read/write functions.
>
> In any case I've tested the patch for reading, erasing and writing the
> NAND and so far I haven't found any unsupported error apart from
> NAND_CMD_GET_FEATURES with a Macronix NAND in the Sercom H500-s
> (BCM63268).
> I believe it's used for unlocking the NAND, which isn't needed in that
> device.

Well, you are restoring an old behavior so I won't ask for a better
support, but you should normally allow software ECC engines (and even no
engine at all) and in this case the core will require a write path. I
honestly think it is not very complex to implement but if someone is
lacking this feature it can be added later.

Please just fix the braces in the for loop that was reported, but no
hurry, I'll only take this after -rc1.

Thanks,
Miquèl

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

[Miquel Raynal]

Hello,

>> AFAIK, the legacy functions were only using it for
>> NAND_CMD_SET_FEATURES, which we don't support:
>> https://github.com/torvalds/linux/blob/c86b63b82fde4f96ee94dde827a5f28ff5adeb57/drivers/mtd/nand/raw/brcmnand/brcmnand.c#L1922-L1938
>>
>> The other uses I could find are already covered by our
>> chip->ecc.read/write functions.
>>
>> In any case I've tested the patch for reading, erasing and writing the
>> NAND and so far I haven't found any unsupported error apart from
>> NAND_CMD_GET_FEATURES with a Macronix NAND in the Sercom H500-s
>> (BCM63268).
>> I believe it's used for unlocking the NAND, which isn't needed in that
>> device.
>
> Well, you are restoring an old behavior so I won't ask for a better
> support, but you should normally allow software ECC engines (and even no
> engine at all) and in this case the core will require a write path. I
> honestly think it is not very complex to implement but if someone is
> lacking this feature it can be added later.
>
> Please just fix the braces in the for loop that was reported, but no
> hurry, I'll only take this after -rc1.

Nevermind, I'm applying now. You can send a follow-up patch for -rc1 if
you want.