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

[PATCH v4] 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 | 181 ++++++++++++++++++++++-
 1 file changed, 175 insertions(+), 6 deletions(-)

 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 17f6d9723df9..e6ec394d30f1 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,152 @@ static int brcmnand_op_is_reset(const struct nand_operation *op)
 	return 0;
 }
 
+static int brcmnand_exec_instructions(struct nand_chip *chip,
+				      const struct nand_operation *op)
+{
+	struct brcmnand_host *host = nand_get_controller_data(chip);
+	unsigned int i;
+	int ret = 0;
+
+	for (i = 0; i < op->ninstrs; i++) {
+		ret = brcmnand_exec_instr(host, i, op);
+		if (ret)
+			break;
+	}
+
+	return ret;
+}
+
+static int brcmnand_exec_instructions_legacy(struct nand_chip *chip,
+					     const struct nand_operation *op)
+{
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	struct brcmnand_host *host = nand_get_controller_data(chip);
+	struct brcmnand_controller *ctrl = host->ctrl;
+	const struct nand_op_instr *instr;
+	unsigned int i, j;
+	u8 cmd = CMD_NULL, last_cmd = CMD_NULL;
+	int ret = 0;
+	u64 last_addr;
+
+	for (i = 0; i < op->ninstrs; i++) {
+		instr = &op->instrs[i];
+
+		if (instr->type == NAND_OP_CMD_INSTR) {
+			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 = -EINVAL;
+			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)
@@ -2525,11 +2692,7 @@ static int brcmnand_exec_op(struct nand_chip *chip,
 	if (op->deassert_wp)
 		brcmnand_wp(mtd, 0);
 
-	for (i = 0; i < op->ninstrs; i++) {
-		ret = brcmnand_exec_instr(host, i, op);
-		if (ret)
-			break;
-	}
+	ret = ctrl->exec_instr(chip, op);
 
 	if (op->deassert_wp)
 		brcmnand_wp(mtd, 1);
@@ -3142,6 +3305,12 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
 	if (ret)
 		goto err;
 
+	/* Only v5.0+ controllers have low level ops support */
+	if (ctrl->nand_version >= 0x0500)
+		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

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

[David Regan]

On Wed, May 14, 2025 at 10:08 PM Á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>

Thank you Álvaro

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

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

[Miquel Raynal]

Hello Alvaro,

On 15/05/2025 at 07:07:59 +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>

Since you have a precise list of what is supported and what's not, I'd
have expected the rest of the behavior to be identical. Are these
controllers so different in how to program them? Can't we use the
existing exec_op() implementation without some of the opcodes?


> +/* Native command conversion */

Should mention "legacy" somewhere I guess, and even what legacy means in
this context, which is version(controller) < XXX.

> +static const u8 native_cmd_conv[] = {
> +	[NAND_CMD_READ0]	= CMD_NOT_SUPPORTED,
> +	[NAND_CMD_READ1]	= CMD_NOT_SUPPORTED,
> +	[NAND_CMD_RNDOUT]	= CMD_PARAMETER_CHANGE_COL,
> +	[NAND_CMD_PAGEPROG]	= CMD_NOT_SUPPORTED,
> +	[NAND_CMD_READOOB]	= CMD_NOT_SUPPORTED,
> +	[NAND_CMD_ERASE1]	= CMD_BLOCK_ERASE,
> +	[NAND_CMD_STATUS]	= CMD_NOT_SUPPORTED,
> +	[NAND_CMD_SEQIN]	= CMD_NOT_SUPPORTED,
> +	[NAND_CMD_RNDIN]	= CMD_NOT_SUPPORTED,
> +	[NAND_CMD_READID]	= CMD_DEVICE_ID_READ,
> +	[NAND_CMD_ERASE2]	= CMD_NULL,
> +	[NAND_CMD_PARAM]	= CMD_PARAMETER_READ,
> +	[NAND_CMD_GET_FEATURES]	= CMD_NOT_SUPPORTED,
> +	[NAND_CMD_SET_FEATURES]	= CMD_NOT_SUPPORTED,
> +	[NAND_CMD_RESET]	= CMD_NOT_SUPPORTED,
> +	[NAND_CMD_READSTART]	= CMD_NOT_SUPPORTED,
> +	[NAND_CMD_READCACHESEQ]	= CMD_NOT_SUPPORTED,
> +	[NAND_CMD_READCACHEEND]	= CMD_NOT_SUPPORTED,
> +	[NAND_CMD_RNDOUTSTART]	= CMD_NULL,
> +	[NAND_CMD_CACHEDPROG]	= CMD_NOT_SUPPORTED,
> +};
> +
>  /* Controller feature flags */
>  enum {
>  	BRCMNAND_HAS_1K_SECTORS			= BIT(0),
> @@ -237,6 +262,10 @@ struct brcmnand_controller {
>  	/* List of NAND hosts (one for each chip-select) */
>  	struct list_head host_list;
>  
> +	/* Function to be called from exec_op */
> +	int (*exec_instr)(struct nand_chip *chip,
> +			  const struct nand_operation *op);
> +
>  	/* EDU info, per-transaction */
>  	const u16               *edu_offsets;
>  	void __iomem            *edu_base;
> @@ -2490,14 +2519,152 @@ static int brcmnand_op_is_reset(const struct nand_operation *op)
>  	return 0;
>  }
>  
> +static int brcmnand_exec_instructions(struct nand_chip *chip,
> +				      const struct nand_operation *op)
> +{
> +	struct brcmnand_host *host = nand_get_controller_data(chip);
> +	unsigned int i;
> +	int ret = 0;
> +
> +	for (i = 0; i < op->ninstrs; i++) {
> +		ret = brcmnand_exec_instr(host, i, op);
> +		if (ret)
> +			break;
> +	}
> +
> +	return ret;
> +}
> +
> +static int brcmnand_exec_instructions_legacy(struct nand_chip *chip,
> +					     const struct nand_operation *op)
> +{
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +	struct brcmnand_host *host = nand_get_controller_data(chip);
> +	struct brcmnand_controller *ctrl = host->ctrl;
> +	const struct nand_op_instr *instr;
> +	unsigned int i, j;
> +	u8 cmd = CMD_NULL, last_cmd = CMD_NULL;
> +	int ret = 0;
> +	u64 last_addr;
> +
> +	for (i = 0; i < op->ninstrs; i++) {
> +		instr = &op->instrs[i];
> +
> +		if (instr->type == NAND_OP_CMD_INSTR) {

Could we use a switch case here? Seems more appropriate.

> +			cmd = native_cmd_conv[instr->ctx.cmd.opcode];
> +			if (cmd == CMD_NOT_SUPPORTED) {
> +				dev_err(ctrl->dev, "unsupported cmd=%d\n",
> +					instr->ctx.cmd.opcode);
> +				ret = -EOPNOTSUPP;
> +				break;
> +			}
> +		} else if (instr->type == NAND_OP_ADDR_INSTR) {
> +			u64 addr = 0;
> +
> +			if (cmd == CMD_NULL)
> +				continue;
> +
> +			if (instr->ctx.addr.naddrs > 8) {
> +				dev_err(ctrl->dev, "unsupported naddrs=%u\n",
> +					instr->ctx.addr.naddrs);
> +				ret = -EOPNOTSUPP;
> +				break;
> +			}
> +
> +			for (j = 0; j < instr->ctx.addr.naddrs; j++)
> +				addr |= (instr->ctx.addr.addrs[j]) << (j << 3);
> +
> +			if (cmd == CMD_BLOCK_ERASE)
> +				addr <<= chip->page_shift;
> +			else if (cmd == CMD_PARAMETER_CHANGE_COL)
> +				addr &= ~((u64)(FC_BYTES - 1));
> +
> +			brcmnand_set_cmd_addr(mtd, addr);
> +			brcmnand_send_cmd(host, cmd);
> +			last_addr = addr;
> +			last_cmd = cmd;
> +			cmd = CMD_NULL;
> +			brcmnand_waitfunc(chip);

Waitfunc is a legacy interface, are you sure this is the correct
function call here?

> +
> +			if (last_cmd == CMD_PARAMETER_READ ||
> +			    last_cmd == CMD_PARAMETER_CHANGE_COL) {
> +				/* Copy flash cache word-wise */
> +				u32 *flash_cache = (u32 *)ctrl->flash_cache;
> +
> +				brcmnand_soc_data_bus_prepare(ctrl->soc, true);
> +
> +				/*
> +				 * Must cache the FLASH_CACHE now, since changes in
> +				 * SECTOR_SIZE_1K may invalidate it
> +				 */
> +				for (j = 0; j < FC_WORDS; j++)
> +					/*
> +					 * Flash cache is big endian for parameter pages, at
> +					 * least on STB SoCs
> +					 */
> +					flash_cache[j] = be32_to_cpu(brcmnand_read_fc(ctrl, j));
> +
> +				brcmnand_soc_data_bus_unprepare(ctrl->soc, true);
> +			}
> +		} else if (instr->type == NAND_OP_DATA_IN_INSTR) {
> +			u8 *in = instr->ctx.data.buf.in;
> +
> +			if (last_cmd == CMD_DEVICE_ID_READ) {
> +				u32 val;
> +
> +				if (instr->ctx.data.len > 8) {
> +					dev_err(ctrl->dev, "unsupported len=%u\n",
> +						instr->ctx.data.len);
> +					ret = -EOPNOTSUPP;
> +					break;
> +				}
> +
> +				for (j = 0; j < instr->ctx.data.len; j++) {
> +					if (j == 0)
> +						val = brcmnand_read_reg(ctrl, BRCMNAND_ID);
> +					else if (j == 4)
> +						val = brcmnand_read_reg(ctrl, BRCMNAND_ID_EXT);
> +
> +					in[j] = (val >> (24 - ((j % 4) << 3))) & 0xff;
> +				}
> +			} else if (last_cmd == CMD_PARAMETER_READ ||
> +				   last_cmd == CMD_PARAMETER_CHANGE_COL) {
> +				u64 addr;
> +				u32 offs;
> +
> +				for (j = 0; j < instr->ctx.data.len; j++) {
> +					addr = last_addr + j;
> +					offs = addr & (FC_BYTES - 1);
> +
> +					if (j > 0 && offs == 0)
> +						nand_change_read_column_op(chip, addr, NULL, 0,
> +									   false);
> +
> +					in[j] = ctrl->flash_cache[offs];
> +				}
> +			}
> +		} else if (instr->type == NAND_OP_WAITRDY_INSTR) {
> +			ret = bcmnand_ctrl_poll_status(host, NAND_CTRL_RDY, NAND_CTRL_RDY, 0);
> +			if (ret)
> +				break;
> +		} else {
> +			dev_err(ctrl->dev, "unsupported instruction type: %d\n", instr->type);
> +			ret = -EINVAL;

EOPNOTSUPP I guess?

> +			break;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
>  static int brcmnand_exec_op(struct nand_chip *chip,
>  			    const struct nand_operation *op,
>  			    bool check_only)
>  {
>  	struct brcmnand_host *host = nand_get_controller_data(chip);
> +	struct brcmnand_controller *ctrl = host->ctrl;
>  	struct mtd_info *mtd = nand_to_mtd(chip);
>  	u8 *status;
> -	unsigned int i;
>  	int ret = 0;
>  
>  	if (check_only)
[adding one more context line]
                 return 0;

There is a lot that is unsupported, and this is okay, but you need to
control all these earlier and implement a function that does all the
checks when exec_op() is called with the check_only parameter set (just
above). The support for the old SoCs *must* not return 0 by default and
instead check the validity of the op when requested.

> @@ -2525,11 +2692,7 @@ static int brcmnand_exec_op(struct nand_chip *chip,
>  	if (op->deassert_wp)
>  		brcmnand_wp(mtd, 0);
>  
> -	for (i = 0; i < op->ninstrs; i++) {
> -		ret = brcmnand_exec_instr(host, i, op);
> -		if (ret)
> -			break;
> -	}
> +	ret = ctrl->exec_instr(chip, op);
>  
>  	if (op->deassert_wp)
>  		brcmnand_wp(mtd, 1);
> @@ -3142,6 +3305,12 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
>  	if (ret)
>  		goto err;
>  
> +	/* Only v5.0+ controllers have low level ops support */
> +	if (ctrl->nand_version >= 0x0500)

Did I just read that 4 or 4.1 was the correct boundary instead of 5?

> +		ctrl->exec_instr = brcmnand_exec_instructions;
> +	else
> +		ctrl->exec_instr = brcmnand_exec_instructions_legacy;
> +
>  	/*
>  	 * Most chips have this cache at a fixed offset within 'nand' block.
>  	 * Some must specify this region separately.

Thanks,
Miquèl

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

[Álvaro Fernández Rojas]

Hi Miquel,

El vie, 16 may 2025 a las 16:32, Miquel Raynal
(<miquel.raynal@bootlin.com>) escribió:
>
> Hello Alvaro,
>
> On 15/05/2025 at 07:07:59 +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>
>
> Since you have a precise list of what is supported and what's not, I'd
> have expected the rest of the behavior to be identical. Are these
> controllers so different in how to program them? Can't we use the
> existing exec_op() implementation without some of the opcodes?

As David confirmed, the low level operation registers aren't working
on v4.0 controllers and they don't even exist on v2.1/2.2 controllers.
BTW, I don't have a precise list of what's supported, I'm just playing
trial and error here in order to get older controllers working again.

>
>
> > +/* Native command conversion */
>
> Should mention "legacy" somewhere I guess, and even what legacy means in
> this context, which is version(controller) < XXX.

Ok, I will add it on v5.

>
> > +static const u8 native_cmd_conv[] = {
> > +     [NAND_CMD_READ0]        = CMD_NOT_SUPPORTED,
> > +     [NAND_CMD_READ1]        = CMD_NOT_SUPPORTED,
> > +     [NAND_CMD_RNDOUT]       = CMD_PARAMETER_CHANGE_COL,
> > +     [NAND_CMD_PAGEPROG]     = CMD_NOT_SUPPORTED,
> > +     [NAND_CMD_READOOB]      = CMD_NOT_SUPPORTED,
> > +     [NAND_CMD_ERASE1]       = CMD_BLOCK_ERASE,
> > +     [NAND_CMD_STATUS]       = CMD_NOT_SUPPORTED,
> > +     [NAND_CMD_SEQIN]        = CMD_NOT_SUPPORTED,
> > +     [NAND_CMD_RNDIN]        = CMD_NOT_SUPPORTED,
> > +     [NAND_CMD_READID]       = CMD_DEVICE_ID_READ,
> > +     [NAND_CMD_ERASE2]       = CMD_NULL,
> > +     [NAND_CMD_PARAM]        = CMD_PARAMETER_READ,
> > +     [NAND_CMD_GET_FEATURES] = CMD_NOT_SUPPORTED,
> > +     [NAND_CMD_SET_FEATURES] = CMD_NOT_SUPPORTED,
> > +     [NAND_CMD_RESET]        = CMD_NOT_SUPPORTED,
> > +     [NAND_CMD_READSTART]    = CMD_NOT_SUPPORTED,
> > +     [NAND_CMD_READCACHESEQ] = CMD_NOT_SUPPORTED,
> > +     [NAND_CMD_READCACHEEND] = CMD_NOT_SUPPORTED,
> > +     [NAND_CMD_RNDOUTSTART]  = CMD_NULL,
> > +     [NAND_CMD_CACHEDPROG]   = CMD_NOT_SUPPORTED,
> > +};
> > +
> >  /* Controller feature flags */
> >  enum {
> >       BRCMNAND_HAS_1K_SECTORS                 = BIT(0),
> > @@ -237,6 +262,10 @@ struct brcmnand_controller {
> >       /* List of NAND hosts (one for each chip-select) */
> >       struct list_head host_list;
> >
> > +     /* Function to be called from exec_op */
> > +     int (*exec_instr)(struct nand_chip *chip,
> > +                       const struct nand_operation *op);
> > +
> >       /* EDU info, per-transaction */
> >       const u16               *edu_offsets;
> >       void __iomem            *edu_base;
> > @@ -2490,14 +2519,152 @@ static int brcmnand_op_is_reset(const struct nand_operation *op)
> >       return 0;
> >  }
> >
> > +static int brcmnand_exec_instructions(struct nand_chip *chip,
> > +                                   const struct nand_operation *op)
> > +{
> > +     struct brcmnand_host *host = nand_get_controller_data(chip);
> > +     unsigned int i;
> > +     int ret = 0;
> > +
> > +     for (i = 0; i < op->ninstrs; i++) {
> > +             ret = brcmnand_exec_instr(host, i, op);
> > +             if (ret)
> > +                     break;
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> > +static int brcmnand_exec_instructions_legacy(struct nand_chip *chip,
> > +                                          const struct nand_operation *op)
> > +{
> > +     struct mtd_info *mtd = nand_to_mtd(chip);
> > +     struct brcmnand_host *host = nand_get_controller_data(chip);
> > +     struct brcmnand_controller *ctrl = host->ctrl;
> > +     const struct nand_op_instr *instr;
> > +     unsigned int i, j;
> > +     u8 cmd = CMD_NULL, last_cmd = CMD_NULL;
> > +     int ret = 0;
> > +     u64 last_addr;
> > +
> > +     for (i = 0; i < op->ninstrs; i++) {
> > +             instr = &op->instrs[i];
> > +
> > +             if (instr->type == NAND_OP_CMD_INSTR) {
>
> Could we use a switch case here? Seems more appropriate.

I would rather keep this as an if/else in order to prevent more
indentation or having to define the variables on top.

>
> > +                     cmd = native_cmd_conv[instr->ctx.cmd.opcode];
> > +                     if (cmd == CMD_NOT_SUPPORTED) {
> > +                             dev_err(ctrl->dev, "unsupported cmd=%d\n",
> > +                                     instr->ctx.cmd.opcode);
> > +                             ret = -EOPNOTSUPP;
> > +                             break;
> > +                     }
> > +             } else if (instr->type == NAND_OP_ADDR_INSTR) {
> > +                     u64 addr = 0;
> > +
> > +                     if (cmd == CMD_NULL)
> > +                             continue;
> > +
> > +                     if (instr->ctx.addr.naddrs > 8) {
> > +                             dev_err(ctrl->dev, "unsupported naddrs=%u\n",
> > +                                     instr->ctx.addr.naddrs);
> > +                             ret = -EOPNOTSUPP;
> > +                             break;
> > +                     }
> > +
> > +                     for (j = 0; j < instr->ctx.addr.naddrs; j++)
> > +                             addr |= (instr->ctx.addr.addrs[j]) << (j << 3);
> > +
> > +                     if (cmd == CMD_BLOCK_ERASE)
> > +                             addr <<= chip->page_shift;
> > +                     else if (cmd == CMD_PARAMETER_CHANGE_COL)
> > +                             addr &= ~((u64)(FC_BYTES - 1));
> > +
> > +                     brcmnand_set_cmd_addr(mtd, addr);
> > +                     brcmnand_send_cmd(host, cmd);
> > +                     last_addr = addr;
> > +                     last_cmd = cmd;
> > +                     cmd = CMD_NULL;
> > +                     brcmnand_waitfunc(chip);
>
> Waitfunc is a legacy interface, are you sure this is the correct
> function call here?

I guess this is correct because brcmnand_waitfunc is still used after
each brcmnand_send_cmd call that we still have on the current code...

>
> > +
> > +                     if (last_cmd == CMD_PARAMETER_READ ||
> > +                         last_cmd == CMD_PARAMETER_CHANGE_COL) {
> > +                             /* Copy flash cache word-wise */
> > +                             u32 *flash_cache = (u32 *)ctrl->flash_cache;
> > +
> > +                             brcmnand_soc_data_bus_prepare(ctrl->soc, true);
> > +
> > +                             /*
> > +                              * Must cache the FLASH_CACHE now, since changes in
> > +                              * SECTOR_SIZE_1K may invalidate it
> > +                              */
> > +                             for (j = 0; j < FC_WORDS; j++)
> > +                                     /*
> > +                                      * Flash cache is big endian for parameter pages, at
> > +                                      * least on STB SoCs
> > +                                      */
> > +                                     flash_cache[j] = be32_to_cpu(brcmnand_read_fc(ctrl, j));
> > +
> > +                             brcmnand_soc_data_bus_unprepare(ctrl->soc, true);
> > +                     }
> > +             } else if (instr->type == NAND_OP_DATA_IN_INSTR) {
> > +                     u8 *in = instr->ctx.data.buf.in;
> > +
> > +                     if (last_cmd == CMD_DEVICE_ID_READ) {
> > +                             u32 val;
> > +
> > +                             if (instr->ctx.data.len > 8) {
> > +                                     dev_err(ctrl->dev, "unsupported len=%u\n",
> > +                                             instr->ctx.data.len);
> > +                                     ret = -EOPNOTSUPP;
> > +                                     break;
> > +                             }
> > +
> > +                             for (j = 0; j < instr->ctx.data.len; j++) {
> > +                                     if (j == 0)
> > +                                             val = brcmnand_read_reg(ctrl, BRCMNAND_ID);
> > +                                     else if (j == 4)
> > +                                             val = brcmnand_read_reg(ctrl, BRCMNAND_ID_EXT);
> > +
> > +                                     in[j] = (val >> (24 - ((j % 4) << 3))) & 0xff;
> > +                             }
> > +                     } else if (last_cmd == CMD_PARAMETER_READ ||
> > +                                last_cmd == CMD_PARAMETER_CHANGE_COL) {
> > +                             u64 addr;
> > +                             u32 offs;
> > +
> > +                             for (j = 0; j < instr->ctx.data.len; j++) {
> > +                                     addr = last_addr + j;
> > +                                     offs = addr & (FC_BYTES - 1);
> > +
> > +                                     if (j > 0 && offs == 0)
> > +                                             nand_change_read_column_op(chip, addr, NULL, 0,
> > +                                                                        false);
> > +
> > +                                     in[j] = ctrl->flash_cache[offs];
> > +                             }
> > +                     }
> > +             } else if (instr->type == NAND_OP_WAITRDY_INSTR) {
> > +                     ret = bcmnand_ctrl_poll_status(host, NAND_CTRL_RDY, NAND_CTRL_RDY, 0);
> > +                     if (ret)
> > +                             break;
> > +             } else {
> > +                     dev_err(ctrl->dev, "unsupported instruction type: %d\n", instr->type);
> > +                     ret = -EINVAL;
>
> EOPNOTSUPP I guess?

Ok, I will change that on v5.

>
> > +                     break;
> > +             }
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> >  static int brcmnand_exec_op(struct nand_chip *chip,
> >                           const struct nand_operation *op,
> >                           bool check_only)
> >  {
> >       struct brcmnand_host *host = nand_get_controller_data(chip);
> > +     struct brcmnand_controller *ctrl = host->ctrl;
> >       struct mtd_info *mtd = nand_to_mtd(chip);
> >       u8 *status;
> > -     unsigned int i;
> >       int ret = 0;
> >
> >       if (check_only)
> [adding one more context line]
>                  return 0;
>
> There is a lot that is unsupported, and this is okay, but you need to
> control all these earlier and implement a function that does all the
> checks when exec_op() is called with the check_only parameter set (just
> above). The support for the old SoCs *must* not return 0 by default and
> instead check the validity of the op when requested.

Ok, I will add that in v5.

>
> > @@ -2525,11 +2692,7 @@ static int brcmnand_exec_op(struct nand_chip *chip,
> >       if (op->deassert_wp)
> >               brcmnand_wp(mtd, 0);
> >
> > -     for (i = 0; i < op->ninstrs; i++) {
> > -             ret = brcmnand_exec_instr(host, i, op);
> > -             if (ret)
> > -                     break;
> > -     }
> > +     ret = ctrl->exec_instr(chip, op);
> >
> >       if (op->deassert_wp)
> >               brcmnand_wp(mtd, 1);
> > @@ -3142,6 +3305,12 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
> >       if (ret)
> >               goto err;
> >
> > +     /* Only v5.0+ controllers have low level ops support */
> > +     if (ctrl->nand_version >= 0x0500)
>
> Did I just read that 4 or 4.1 was the correct boundary instead of 5?

David has confirmed that this is correct.

>
> > +             ctrl->exec_instr = brcmnand_exec_instructions;
> > +     else
> > +             ctrl->exec_instr = brcmnand_exec_instructions_legacy;
> > +
> >       /*
> >        * Most chips have this cache at a fixed offset within 'nand' block.
> >        * Some must specify this region separately.
>
> Thanks,
> Miquèl