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

[William Zhang <william.zhang@broadcom.com>]

[-- Attachment #1.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 #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4199 bytes --]

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

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