Message-ID: <87o6vsd3tb.fsf@bootlin.com>
Date: Fri, 16 May 2025 16:32:16 +0200
From: Miquel Raynal <miquel.raynal@...tlin.com>
To: Álvaro Fernández Rojas <noltari@...il.com>
Cc: linux-mtd@...ts.infradead.org,  dregan@...adcom.com,
  bcm-kernel-feedback-list@...adcom.com,  florian.fainelli@...adcom.com,
  rafal@...ecki.pl,  computersforpeace@...il.com,  kamal.dasu@...adcom.com,
  dan.beygelman@...adcom.com,  william.zhang@...adcom.com,
  frieder.schrempf@...tron.de,  linux-kernel@...r.kernel.org,
  vigneshr@...com,  richard@....at,  bbrezillon@...nel.org,
  kdasu.kdev@...il.com,  jaimeliao.tw@...il.com,  kilobyte@...band.pl,
  jonas.gorski@...il.com,  dgcbueu@...il.com
Subject: Re: [PATCH v4] mtd: rawnand: brcmnand: legacy exec_op implementation

Hello Alvaro,

On 15/05/2025 at 07:07:59 +02, Álvaro Fernández Rojas <noltari@...il.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@...il.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

Powered by blists - more mailing lists