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
