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

[Miquel Raynal]

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