Re: [LINUX PATCH v12 3/3] mtd: rawnand: arasan: Add support for Arasan NAND Flash Controller

[Boris Brezillon]

Hi Naga,

Just a preliminary review. I still think we have problems with how you
execute NAND operations. You seem to assume that read/write operation
are always page write/read operation with a size aligned on a page
size. This is wrong, either the controller is able to execute the exact
operation that has been requested or it returns -ENOTSUPP.

On Fri, 9 Nov 2018 10:30:41 +0530
Naga Sureshkumar Relli <naga.sureshkumar.relli@xxxxxxxxxx> wrote:

> +
> +/**
> + * struct anfc_nand_chip - Defines the nand chip related information
> + * @node:		Used to store NAND chips into a list.
> + * @chip:		NAND chip information structure.
> + * @strength:		Bch or Hamming mode enable/disable.

The name is still confusing. BTW, can't you deduce Hamming vs BCH from
the strength? Hamming strength is 1, while BCH strengths seem to start
at 4.

> + * @ecc_strength:	Ecc strength 4.8/12/16.

				      ^/

> + * @eccval:		Ecc config value.
> + * @spare_caddr_cycles:	Column address cycle information for spare area.
> + * @pktsize:		Packet size for read / write operation.
> + * @csnum:		chipselect number to be used.
> + * @spktsize:		Packet size in ddr mode for status operation.
> + * @inftimeval:		Data interface and timing mode information
> + */
> +struct anfc_nand_chip {
> +	struct list_head node;
> +	struct nand_chip chip;
> +	bool strength;
> +	u32 ecc_strength;
> +	u32 eccval;
> +	u16 spare_caddr_cycles;
> +	u32 pktsize;
> +	int csnum;
> +	u32 spktsize;
> +	u32 inftimeval;
> +};
> +
> +/**
> + * struct anfc_nand_controller - Defines the Arasan NAND flash controller
> + *				 driver instance
> + * @controller:		base controller structure.
> + * @chips:		list of all nand chips attached to the ctrler.
> + * @dev:		Pointer to the device structure.
> + * @base:		Virtual address of the NAND flash device.
> + * @clk_sys:		Pointer to the system clock.
> + * @clk_flash:		Pointer to the flash clock.
> + * @dma:		Dma enable/disable.
> + * @buf:		Buffer used for read/write byte operations.
> + * @irq:		irq number

You should need this field. Just get the irq num in you probe path an
register an irq handler with devm_request_irq().

> + * @bufshift:		Variable used for indexing buffer operation
> + * @csnum:		Chip select number currently inuse.

						     ^ in use

> + * @event:		Completion event for nand status events.
> + * @status:		Status of the flash device.
> + * @prog:		Used to initiate controller operations.
> + */
> +struct anfc_nand_controller {
> +	struct nand_controller controller;
> +	struct list_head chips;
> +	struct device *dev;
> +	void __iomem *base;
> +	struct clk *clk_sys;
> +	struct clk *clk_flash;
> +	int irq;
> +	int csnum;
> +	struct completion event;
> +	int status;
> +	u8 buf[TEMP_BUF_SIZE];

Allocate this buf dynamically.

> +	u32 eccval;
> +};

> +static int anfc_page_write_type_exec(struct nand_chip *chip,
> +				     const struct nand_subop *subop)
> +{
> +	const struct nand_op_instr *instr;
> +	struct anfc_nand_chip *achip = to_anfc_nand(chip);
> +	struct anfc_nand_controller *nfc = to_anfc(chip->controller);
> +	unsigned int op_id, len;
> +	struct anfc_op nfc_op = {};
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +
> +	anfc_parse_instructions(chip, subop, &nfc_op);
> +	instr = nfc_op.data_instr;
> +	op_id = nfc_op.data_instr_idx;
> +	anfc_prepare_cmd(chip, nfc_op.cmds[0], nfc_op.cmds[1], 1,
> +			 mtd->writesize, nfc_op.naddrcycles);
> +	anfc_setpagecoladdr(nfc, nfc_op.row, nfc_op.col);
> +	if (!nfc_op.data_instr)
> +		return 0;
> +
> +	len = nand_subop_get_data_len(subop, op_id);
> +	anfc_write_data_op(chip,  (char *)instr->ctx.data.buf.out,

				^ extra white space here
and please drop the cast.

Can you please run checkpatch --strict prior to submitting patches?

> +			   mtd->writesize,
> +			   DIV_ROUND_UP(mtd->writesize, achip->pktsize),

No, that's wrong. You should use instr->ctx.data.len here, and the
DIV_ROUND_UP() thing is a bit scary. That means you might be writing
more data than requested.

> +			   achip->pktsize);
> +
> +	return 0;
> +}
> +

> +
> +static int anfc_probe(struct platform_device *pdev)
> +{
> +	struct anfc_nand_controller *nfc;
> +	struct anfc_nand_chip *anand_chip;
> +	struct device_node *np = pdev->dev.of_node, *child;
> +	struct resource *res;
> +	int err;
> +
> +	nfc = devm_kzalloc(&pdev->dev, sizeof(*nfc), GFP_KERNEL);
> +	if (!nfc)
> +		return -ENOMEM;
> +
> +	nand_controller_init(&nfc->controller);
> +	INIT_LIST_HEAD(&nfc->chips);
> +	init_completion(&nfc->event);
> +	nfc->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, nfc);
> +	nfc->csnum = -1;
> +	nfc->controller.ops = &anfc_nand_controller_ops;

Add a blank line here please

> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	nfc->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(nfc->base))
> +		return PTR_ERR(nfc->base);

and here

> +	nfc->irq = platform_get_irq(pdev, 0);
> +	if (nfc->irq < 0) {
> +		dev_err(&pdev->dev, "platform_get_irq failed\n");
> +		return -ENXIO;
> +	}

and here

> +	dma_set_mask(&pdev->dev, DMA_BIT_MASK(64));

Is this really needed?

> +	err = devm_request_irq(&pdev->dev, nfc->irq, anfc_irq_handler,
> +			       0, "arasannfc", nfc);
> +	if (err)
> +		return err;

Add a blank line here too.

> +	nfc->clk_sys = devm_clk_get(&pdev->dev, "clk_sys");
> +	if (IS_ERR(nfc->clk_sys)) {
> +		dev_err(&pdev->dev, "sys clock not found.\n");
> +		return PTR_ERR(nfc->clk_sys);
> +	}
> +
> +	nfc->clk_flash = devm_clk_get(&pdev->dev, "clk_flash");
> +	if (IS_ERR(nfc->clk_flash)) {
> +		dev_err(&pdev->dev, "flash clock not found.\n");
> +		return PTR_ERR(nfc->clk_flash);
> +	}
> +
> +	err = clk_prepare_enable(nfc->clk_sys);
> +	if (err) {
> +		dev_err(&pdev->dev, "Unable to enable sys clock.\n");
> +		return err;
> +	}
> +
> +	err = clk_prepare_enable(nfc->clk_flash);
> +	if (err) {
> +		dev_err(&pdev->dev, "Unable to enable flash clock.\n");
> +		goto clk_dis_sys;
> +	}
> +
> +	for_each_available_child_of_node(np, child) {
> +		anand_chip = devm_kzalloc(&pdev->dev, sizeof(*anand_chip),
> +					  GFP_KERNEL);
> +		if (!anand_chip) {
> +			of_node_put(child);
> +			err = -ENOMEM;
> +			goto nandchip_clean_up;
> +		}

and here.

> +		err = anfc_nand_chip_init(nfc, anand_chip, child);
> +		if (err) {
> +			devm_kfree(&pdev->dev, anand_chip);

We usually try to avoid calling devm_kfree(). I guess you do it to
avoid keeping the chip obj around when init failed, but this should
be rare enough so we can actually ignore it and let the mem allocated
for the NFC lifetime.

> +			continue;
> +		}
> +
> +		list_add_tail(&anand_chip->node, &nfc->chips);
> +	}
> +	return 0;
> +
> +nandchip_clean_up:
> +	list_for_each_entry(anand_chip, &nfc->chips, node)
> +		nand_release(&anand_chip->chip);

Blank line here.

> +	clk_disable_unprepare(nfc->clk_flash);

Blank line here.

> +clk_dis_sys:
> +	clk_disable_unprepare(nfc->clk_sys);
> +
> +	return err;
> +}

Regards,

Boris