Hello Keguang,
I guess I am mostly fine with the driver, it's probably time to merge
it. Just a few final changes below, I plan on merging it at -rc1.
> + case NAND_CMD_READSTART:
> + if (!op->is_read)
> + return -EOPNOTSUPP;
> + op->cmd_reg = LS1X_NAND_CMD_READ;
> + break;
> + case NAND_CMD_RNDOUT:
> + op->is_change_column = true;
> + break;
> + case NAND_CMD_RNDOUTSTART:
> + if (!op->is_change_column)
> + return -EOPNOTSUPP;
> + op->cmd_reg = LS1X_NAND_CMD_READ;
> + break;
> + default:
> + dev_err(host->dev, "unsupported opcode: %u\n", opcode);
No error message in the normal path. This should be a debug log at
most. This function is called in the check_only path.
> + return -EOPNOTSUPP;
> + }
> +
> + return 0;
> +}
...
> +static int ls1x_nand_read_id_type_exec(struct nand_chip *chip, const struct nand_subop *subop)
> +{
> + struct ls1x_nand_host *host = nand_get_controller_data(chip);
> + struct ls1x_nand_op op = {};
> + int i, ret;
> + union {
> + char ids[5];
> + struct {
> + int idl;
> + char idh;
> + };
> + } nand_id;
> +
> + ret = ls1x_nand_misc_type_exec(chip, subop, &op);
> + if (ret) {
> + dev_err(host->dev, "failed to read ID! %d\n", ret);
No print here, it's not useful.
> + return ret;
> + }
> +
> + nand_id.idl = readl(host->reg_base + LS1X_NAND_IDL);
> + nand_id.idh = readb(host->reg_base + LS1X_NAND_IDH_STATUS);
> +
> + for (i = 0; i < min(sizeof(nand_id.ids), op.orig_len); i++)
> + op.buf[i] = nand_id.ids[sizeof(nand_id.ids) - 1 - i];
> +
> + return ret;
> +}
...
> +static int ls1x_nand_is_valid_cmd(struct device *dev, u8 opcode)
> +{
> + if (opcode == NAND_CMD_STATUS || opcode == NAND_CMD_RESET || opcode == NAND_CMD_READID)
> + return 0;
> +
> + dev_err(dev, "unsupported opcode: %x", opcode);
Ditto
> +
> + return -EOPNOTSUPP;
> +}
> +
> +static int ls1x_nand_is_valid_cmd_seq(struct device *dev, u8 opcode1, u8 opcode2)
> +{
> + if (opcode1 == NAND_CMD_RNDOUT && opcode2 == NAND_CMD_RNDOUTSTART)
> + return 0;
> +
> + if (opcode1 == NAND_CMD_READ0 && opcode2 == NAND_CMD_READSTART)
> + return 0;
> +
> + if (opcode1 == NAND_CMD_ERASE1 && opcode2 == NAND_CMD_ERASE2)
> + return 0;
> +
> + if (opcode1 == NAND_CMD_SEQIN && opcode2 == NAND_CMD_PAGEPROG)
> + return 0;
> +
> + dev_err(dev, "unsupported opcode sequence: %x %x", opcode1,
> opcode2);
Ditto
> +
> + return -EOPNOTSUPP;
> +}
...
> +static int ls1x_nand_attach_chip(struct nand_chip *chip)
> +{
> + struct ls1x_nand_host *host = nand_get_controller_data(chip);
> + u64 chipsize = nanddev_target_size(&chip->base);
> + int cell_size = 0;
> +
> + switch (chipsize) {
> + case SZ_128M:
> + cell_size = 0x0;
> + break;
> + case SZ_256M:
> + cell_size = 0x1;
> + break;
> + case SZ_512M:
> + cell_size = 0x2;
> + break;
> + case SZ_1G:
> + cell_size = 0x3;
> + break;
> + case SZ_2G:
> + cell_size = 0x4;
> + break;
> + case SZ_4G:
> + cell_size = 0x5;
> + break;
> + case SZ_8G:
> + cell_size = 0x6;
> + break;
> + case SZ_16G:
> + cell_size = 0x7;
> + break;
> + default:
> + dev_err(host->dev, "unsupported chip size: %llu MB\n", chipsize);
> + return -EOPNOTSUPP;
EINVAL
> + }
> +
> + switch (chip->ecc.engine_type) {
> + case NAND_ECC_ENGINE_TYPE_NONE:
> + dev_info(host->dev, "No ECC\n");
Please drop
> + break;
> + case NAND_ECC_ENGINE_TYPE_SOFT:
> + dev_info(host->dev, "using SW ECC\n");
Drop
> + break;
> + default:
> + dev_err(host->dev, "ECC mode %d not supported\n",
> chip->ecc.engine_type);
Drop
> + return -EINVAL;
> + }
> +
> + /* set cell size */
> + regmap_update_bits(host->regmap, LS1X_NAND_PARAM, LS1X_NAND_CELL_SIZE_MASK,
> + FIELD_PREP(LS1X_NAND_CELL_SIZE_MASK, cell_size));
> +
> + regmap_update_bits(host->regmap, LS1X_NAND_TIMING, LS1X_NAND_HOLD_CYCLE_MASK,
> + FIELD_PREP(LS1X_NAND_HOLD_CYCLE_MASK, host->data->hold_cycle));
> +
> + regmap_update_bits(host->regmap, LS1X_NAND_TIMING, LS1X_NAND_WAIT_CYCLE_MASK,
> + FIELD_PREP(LS1X_NAND_WAIT_CYCLE_MASK, host->data->wait_cycle));
> +
> + chip->ecc.read_page_raw = nand_monolithic_read_page_raw;
> + chip->ecc.write_page_raw = nand_monolithic_write_page_raw;
> +
> + return 0;
> +}
> +
> +static const struct nand_controller_ops ls1x_nand_controller_ops = {
> + .exec_op = ls1x_nand_exec_op,
> + .attach_chip = ls1x_nand_attach_chip,
> +};
> +
> +static void ls1x_nand_controller_cleanup(struct ls1x_nand_host *host)
> +{
> + if (host->dma_chan)
> + dma_release_channel(host->dma_chan);
> +}
> +
> +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;
> + 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));
You can drop this one as well
> +
> + return 0;
> +}
Thanks,
Miquèl
