Re: [PATCH v11 2/2] mtd: rawnand: Add Loongson-1 NAND Controller Driver

[Miquel Raynal]

Hello Keguang,

On 17/12/2024 at 18:16:50 +08, Keguang Zhang via B4 Relay <devnull+keguang.zhang.gmail.com@xxxxxxxxxx> wrote:

> +static int ls1x_nand_op_cmd_mapping(struct nand_chip *chip, struct ls1x_nand_op *op, u8 opcode)
> +{
> +	struct ls1x_nand_host *host = nand_get_controller_data(chip);
> +	int ret = 0;

This return code is unused.

> +
> +	op->row_start = chip->page_shift + 1;
> +
> +	/* The controller abstracts the following NAND operations. */
> +	switch (opcode) {
> +	case NAND_CMD_STATUS:
> +		op->cmd_reg = LS1X_NAND_CMD_STATUS;
> +		break;
> +	case NAND_CMD_RESET:
> +		op->cmd_reg = LS1X_NAND_CMD_RESET;
> +		break;
> +	case NAND_CMD_READID:
> +		op->is_readid = true;
> +		op->cmd_reg = LS1X_NAND_CMD_READID;
> +		break;
> +	case NAND_CMD_ERASE1:
> +		op->is_erase = true;
> +		op->addrs_offset = 2;
> +		break;
> +	case NAND_CMD_ERASE2:
> +		if (!op->is_erase)
> +			return -EOPNOTSUPP;
> +		/* During erasing, row_start differs from the default value. */

...

> +static void ls1x_nand_trigger_op(struct ls1x_nand_host *host, struct ls1x_nand_op *op)
> +{
> +	struct nand_chip *chip = &host->chip;
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +	int col0 = op->addrs[0];
> +	short col;
> +
> +	/* restore row address for column change */
> +	if (op->is_change_column) {
> +		op->addr2_reg = readl(host->reg_base + LS1X_NAND_ADDR2);
> +		op->addr1_reg = readl(host->reg_base + LS1X_NAND_ADDR1);
> +		op->addr1_reg &= ~(mtd->writesize - 1);
> +	}

This looks very suspicious. You should not have to do that and to be
honest, I don't undertand what this means.

> +
> +	if (!IS_ALIGNED(col0, chip->buf_align)) {
> +		col0 = ALIGN_DOWN(op->addrs[0], chip->buf_align);
> +		op->aligned_offset = op->addrs[0] - col0;
> +		op->addrs[0] = col0;
> +	}
> +
> +	if (host->data->parse_address)
> +		host->data->parse_address(op);
> +
> +	/* set address */
> +	writel(op->addr1_reg, host->reg_base + LS1X_NAND_ADDR1);
> +	writel(op->addr2_reg, host->reg_base + LS1X_NAND_ADDR2);
> +
> +	/* set operation length */
> +	if (op->is_write || op->is_read || op->is_change_column)
> +		op->len = ALIGN(op->orig_len + op->aligned_offset, chip->buf_align);
> +	else if (op->is_erase)
> +		op->len = 1;
> +	else
> +		op->len = op->orig_len;
> +
> +	writel(op->len, host->reg_base + LS1X_NAND_OP_NUM);
> +
> +	/* set operation area */
> +	col = op->addrs[1] << BITS_PER_BYTE | op->addrs[0];
> +	if (op->orig_len && !op->is_readid) {
> +		if (col < mtd->writesize)
> +			op->cmd_reg |= LS1X_NAND_CMD_OP_MAIN;
> +
> +		op->cmd_reg |= LS1X_NAND_CMD_OP_SPARE;
> +	}
> +
> +	/* set operation scope */
> +	if (host->data->op_scope_field) {
> +		unsigned int op_scope;
> +
> +		switch (op->cmd_reg & LS1X_NAND_CMD_OP_AREA_MASK) {
> +		case LS1X_NAND_CMD_OP_MAIN:
> +			op_scope = mtd->writesize;
> +			break;
> +		case LS1X_NAND_CMD_OP_SPARE:
> +			op_scope = mtd->oobsize;
> +			break;
> +		case LS1X_NAND_CMD_OP_AREA_MASK:
> +			op_scope = mtd->writesize + mtd->oobsize;
> +			break;
> +		default:
> +			op_scope = 0;
> +			break;
> +		}

Please get rid of this extra step. I'm not a big fan of it, but this can
be very well simplified and this whole switch removed.

> +
> +		op_scope <<= __ffs(host->data->op_scope_field);
> +		regmap_update_bits(host->regmap, LS1X_NAND_PARAM,
> +				   host->data->op_scope_field, op_scope);
> +	}
> +
> +	/* set command */
> +	writel(op->cmd_reg, host->reg_base + LS1X_NAND_CMD);
> +
> +	/* trigger operation */
> +	regmap_write_bits(host->regmap, LS1X_NAND_CMD, LS1X_NAND_CMD_VALID, LS1X_NAND_CMD_VALID);
> +}
> +

...

> +static const struct nand_op_parser ls1x_nand_op_parser = NAND_OP_PARSER(
> +	NAND_OP_PARSER_PATTERN(
> +		ls1x_nand_read_id_type_exec,
> +		NAND_OP_PARSER_PAT_CMD_ELEM(false),
> +		NAND_OP_PARSER_PAT_ADDR_ELEM(false, LS1X_NAND_MAX_ADDR_CYC),
> +		NAND_OP_PARSER_PAT_DATA_IN_ELEM(false, 8)),
> +	NAND_OP_PARSER_PATTERN(
> +		ls1x_nand_read_status_type_exec,
> +		NAND_OP_PARSER_PAT_CMD_ELEM(false),
> +		NAND_OP_PARSER_PAT_DATA_IN_ELEM(false, 1)),
> +	NAND_OP_PARSER_PATTERN(
> +		ls1x_nand_zerolen_type_exec,
> +		NAND_OP_PARSER_PAT_CMD_ELEM(false),
> +		NAND_OP_PARSER_PAT_WAITRDY_ELEM(false)),
> +	NAND_OP_PARSER_PATTERN(
> +		ls1x_nand_zerolen_type_exec,
> +		NAND_OP_PARSER_PAT_CMD_ELEM(false),
> +		NAND_OP_PARSER_PAT_ADDR_ELEM(false, LS1X_NAND_MAX_ADDR_CYC),
> +		NAND_OP_PARSER_PAT_CMD_ELEM(false),
> +		NAND_OP_PARSER_PAT_WAITRDY_ELEM(false)),
> +	NAND_OP_PARSER_PATTERN(
> +		ls1x_nand_data_type_exec,
> +		NAND_OP_PARSER_PAT_CMD_ELEM(false),
> +		NAND_OP_PARSER_PAT_ADDR_ELEM(false, LS1X_NAND_MAX_ADDR_CYC),
> +		NAND_OP_PARSER_PAT_CMD_ELEM(false),
> +		NAND_OP_PARSER_PAT_WAITRDY_ELEM(true),
> +		NAND_OP_PARSER_PAT_DATA_IN_ELEM(false, 0)),
> +	NAND_OP_PARSER_PATTERN(
> +		ls1x_nand_data_type_exec,
> +		NAND_OP_PARSER_PAT_CMD_ELEM(false),
> +		NAND_OP_PARSER_PAT_ADDR_ELEM(false, LS1X_NAND_MAX_ADDR_CYC),
> +		NAND_OP_PARSER_PAT_DATA_OUT_ELEM(false, 0),
> +		NAND_OP_PARSER_PAT_CMD_ELEM(false),
> +		NAND_OP_PARSER_PAT_WAITRDY_ELEM(true)),
> +	);
> +
> +static inline bool ls1x_nand_is_valid_cmd(u8 opcode)
> +{
> +	return opcode == NAND_CMD_RESET ||
> +	       opcode == NAND_CMD_READID ||
> +	       opcode == NAND_CMD_ERASE1 ||
> +	       opcode == NAND_CMD_ERASE2 ||
> +	       opcode == NAND_CMD_STATUS ||
> +	       opcode == NAND_CMD_SEQIN ||
> +	       opcode == NAND_CMD_PAGEPROG ||
> +	       opcode == NAND_CMD_RNDOUT ||
> +	       opcode == NAND_CMD_RNDOUTSTART ||
> +	       opcode == NAND_CMD_READ0 ||
> +	       opcode == NAND_CMD_READSTART;
> +}
> +
> +static inline bool ls1x_nand_is_cmd_sequence(const struct nand_op_instr *instr1,
> +					     const struct nand_op_instr *instr2)
> +{
> +	return instr1->type == NAND_OP_CMD_INSTR && instr2->type == NAND_OP_CMD_INSTR;
> +}
> +
> +static inline bool ls1x_nand_is_erase_sequence(const struct nand_op_instr *instr1,
> +					       const struct nand_op_instr *instr2)
> +{
> +	return instr1->ctx.cmd.opcode == NAND_CMD_ERASE1 &&
> +	       instr2->ctx.cmd.opcode == NAND_CMD_ERASE2;
> +}
> +
> +static inline bool ls1x_nand_is_write_sequence(const struct nand_op_instr *instr1,
> +					       const struct nand_op_instr *instr2)
> +{
> +	return instr1->ctx.cmd.opcode == NAND_CMD_SEQIN &&
> +	       instr2->ctx.cmd.opcode == NAND_CMD_PAGEPROG;
> +}
> +
> +static inline bool ls1x_nand_is_read_sequence(const struct nand_op_instr *instr1,
> +					      const struct nand_op_instr *instr2)
> +{
> +	return (instr1->ctx.cmd.opcode == NAND_CMD_READ0 &&
> +		instr2->ctx.cmd.opcode == NAND_CMD_READSTART) ||
> +	       (instr1->ctx.cmd.opcode == NAND_CMD_RNDOUT &&
> +		instr2->ctx.cmd.opcode == NAND_CMD_RNDOUTSTART);
> +}
> +
> +static int ls1x_nand_check_op(struct nand_chip *chip, const struct nand_operation *op)
> +{
> +	const struct nand_op_instr *instr;
> +	int op_id;
> +
> +	for (op_id = 0; op_id < op->ninstrs; op_id++) {
> +		instr = &op->instrs[op_id];
> +
> +		switch (instr->type) {
> +		case NAND_OP_CMD_INSTR:
> +			if (!ls1x_nand_is_valid_cmd(instr->ctx.cmd.opcode))
> +				return -EOPNOTSUPP;
> +			break;
> +		case NAND_OP_ADDR_INSTR:
> +			if (instr->ctx.addr.naddrs > LS1X_NAND_MAX_ADDR_CYC)
> +				return -EOPNOTSUPP;
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	if (op->ninstrs == 4 &&
> +	    ls1x_nand_is_cmd_sequence(&op->instrs[0], &op->instrs[2]) &&
> +	    !ls1x_nand_is_erase_sequence(&op->instrs[0], &op->instrs[2]))
> +		return -EOPNOTSUPP;
> +
> +	if (op->ninstrs == 5) {
> +		if (ls1x_nand_is_cmd_sequence(&op->instrs[0], &op->instrs[2]) &&
> +		    !ls1x_nand_is_read_sequence(&op->instrs[0], &op->instrs[2]))
> +			return -EOPNOTSUPP;
> +
> +		if (ls1x_nand_is_cmd_sequence(&op->instrs[0], &op->instrs[3]) &&
> +		    !ls1x_nand_is_write_sequence(&op->instrs[0], &op->instrs[3]))
> +			return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ls1x_nand_exec_op(struct nand_chip *chip,
> +			     const struct nand_operation *op,
> +			     bool check_only)
> +{
> +	if (check_only)
> +		return ls1x_nand_check_op(chip, op);
> +

It lookse like you're re-encoding all your requirements in
ls1x_nand_check_op(), whereas nand_op_parser_exec_op(check_only := true)
should give you already a certain number of verifications which are
skipped here. I'd suggest to improve this to avoid repetitions between
the two. Of course the second part of nand_check_op is necessary, but
the initial checks seem redundant and would better be performed by the parser.

> +	return nand_op_parser_exec_op(chip, &ls1x_nand_op_parser, op, check_only);
> +}
> +
> +static int ls1x_nand_attach_chip(struct nand_chip *chip)
> +{

...

> +static int ls1x_nand_controller_init(struct ls1x_nand_host *host)
> +{
> +	struct device *dev = host->dev;
> +	struct dma_chan *chan;
> +	struct dma_slave_config cfg = {};
> +	int ret;
> +
> +	host->regmap = devm_regmap_init_mmio(dev, host->reg_base, &ls1x_nand_regmap_config);
> +	if (IS_ERR(host->regmap))
> +		return dev_err_probe(dev, PTR_ERR(host->regmap), "failed to init regmap\n");
> +
> +	chan = dma_request_chan(dev, "rxtx");
> +	if (IS_ERR(chan))
> +		return dev_err_probe(dev, PTR_ERR(chan), "failed to request DMA channel\n");
> +	host->dma_chan = chan;
> +
> +	cfg.src_addr = host->dma_base;
> +	cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> +	cfg.dst_addr = host->dma_base;

Don't you need a dma_addr_t here instead? You shall remap the resource.

> +	cfg.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> +	ret = dmaengine_slave_config(host->dma_chan, &cfg);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to config DMA channel\n");
> +
> +	init_completion(&host->dma_complete);
> +
> +	dev_dbg(dev, "got %s for %s access\n", dma_chan_name(host->dma_chan), dev_name(dev));
> +
> +	return 0;
> +}
> +
> +static int ls1x_nand_chip_init(struct ls1x_nand_host *host)
> +{
> +	struct device *dev = host->dev;
> +	int nchips = of_get_child_count(dev->of_node);
> +	struct device_node *chip_np;
> +	struct nand_chip *chip = &host->chip;
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +	int ret = 0;
> +
> +	if (nchips != 1)
> +		return dev_err_probe(dev, -EINVAL, "Currently one NAND chip supported\n");
> +
> +	chip_np = of_get_next_child(dev->of_node, NULL);
> +	if (!chip_np)
> +		return dev_err_probe(dev, -ENODEV, "failed to get child node for NAND chip\n");
> +
> +	chip->controller = &host->controller;
> +	chip->options = NAND_NO_SUBPAGE_WRITE | NAND_USES_DMA | NAND_BROKEN_XD;
> +	chip->buf_align = 16;
> +	nand_set_controller_data(chip, host);
> +	nand_set_flash_node(chip, chip_np);
> +
> +	mtd->dev.parent = dev;
> +	mtd->name = "ls1x-nand";

No, the name is gonna be filled automatically when you call
nand_set_flash_node IIRC.

> +	mtd->owner = THIS_MODULE;
> +
> +	ret = nand_scan(chip, 1);
> +	if (ret) {
> +		of_node_put(chip_np);
> +		return ret;
> +	}
> +

It looks like your controller does not support any ECC correction, if
that's the case you must make sure it's properly handled in attach_chip
hook by refusing to probe if the on_host engine is used.

Thanks,
Miquèl