RE: [LINUX PATCH v10 2/2] mtd: rawnand: arasan: Add support for Arasan NAND Flash Controller

[Naga Sureshkumar Relli <nagasure@xxxxxxxxxx>]

Hi Boris,

Thanks for the review.

> -----Original Message-----
> From: Boris Brezillon [mailto:boris.brezillon@xxxxxxxxxxx]
> Sent: Monday, August 20, 2018 10:10 PM
> To: Naga Sureshkumar Relli <nagasure@xxxxxxxxxx>
> Cc: miquel.raynal@xxxxxxxxxxx; richard@xxxxxx; dwmw2@xxxxxxxxxxxxx;
> computersforpeace@xxxxxxxxx; marek.vasut@xxxxxxxxx; kyungmin.park@xxxxxxxxxxx;
> absahu@xxxxxxxxxxxxxx; peterpandong@xxxxxxxxxx; frieder.schrempf@xxxxxxxxx; linux-
> mtd@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Michal Simek <michals@xxxxxxxxxx>;
> nagasureshkumarrelli@xxxxxxxxx
> Subject: Re: [LINUX PATCH v10 2/2] mtd: rawnand: arasan: Add support for Arasan
> NAND Flash Controller
> 
> Hi Naga,
> 
> On Fri, 17 Aug 2018 18:49:24 +0530
> Naga Sureshkumar Relli <naga.sureshkumar.relli@xxxxxxxxxx> wrote:
> 
> >
> > +config MTD_NAND_ARASAN
> > +	tristate "Support for Arasan Nand Flash controller"
> > +	depends on HAS_IOMEM
> > +	depends on HAS_DMA
> 
> Just nitpicking, but you can place them on the same line:
Ok, I will update it next version.
> 
> 	depends on HAS_IOMEM && HAS_DMA
> 
> > +	help
> > +	  Enables the driver for the Arasan Nand Flash controller on
> > +	  Zynq Ultrascale+ MPSoC.
> > +
> >  endif # MTD_NAND
> > diff --git a/drivers/mtd/nand/raw/Makefile
> > b/drivers/mtd/nand/raw/Makefile index d5a5f98..ccb8d56 100644
> > --- a/drivers/mtd/nand/raw/Makefile
> > +++ b/drivers/mtd/nand/raw/Makefile
> > @@ -57,6 +57,7 @@ obj-$(CONFIG_MTD_NAND_BRCMNAND)		+=
> brcmnand/
> >  obj-$(CONFIG_MTD_NAND_QCOM)		+= qcom_nandc.o
> >  obj-$(CONFIG_MTD_NAND_MTK)		+= mtk_ecc.o mtk_nand.o
> >  obj-$(CONFIG_MTD_NAND_TEGRA)		+= tegra_nand.o
> > +obj-$(CONFIG_MTD_NAND_ARASAN)		+= arasan_nand.o
> >
> >  nand-objs := nand_base.o nand_bbt.o nand_timings.o nand_ids.o
> > nand-objs += nand_amd.o diff --git
> > a/drivers/mtd/nand/raw/arasan_nand.c
> > b/drivers/mtd/nand/raw/arasan_nand.c
> > new file mode 100644
> > index 0000000..e4f1f80
> > --- /dev/null
> > +++ b/drivers/mtd/nand/raw/arasan_nand.c
> > @@ -0,0 +1,1350 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Arasan NAND Flash Controller Driver
> > + *
> > + * Copyright (C) 2014 - 2017 Xilinx, Inc.
> > + * Author: Punnaiah Choudary Kalluri <punnaia@xxxxxxxxxx>
> > + * Author: Naga Sureshkumar Relli <nagasure@xxxxxxxxxx>
> > + *
> > + */
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io-64-nonatomic-lo-hi.h> #include <linux/module.h>
> > +#include <linux/mtd/mtd.h> #include <linux/mtd/rawnand.h> #include
> > +<linux/mtd/partitions.h> #include <linux/of.h> #include
> > +<linux/platform_device.h> #include <linux/slab.h>
> 
> Blank line missing here.
Ok, I will update it next version.
> 
> > +#define DRIVER_NAME			"arasan_nand"
> 
> Please define drop this definition and pass the string directly to
> driver->name. The driver is for a nand controller, so make it clear,
> also, it's just a detail but I prefer '-' to '_', so, how about "arasan-nand-controller"?
Yes, it looks good. 
I will update it next version.
> 
> > +#define EVNT_TIMEOUT_MSEC		1000
> 
> It's unusual to have EVNT, it's usually EVT or EVENT.
Ok, I will update it in next version.
> 
> > +#define STATUS_TIMEOUT			2000
> 
> Is it in milliseconds? Please add the proper _<UNIT> prefix here.
Yes, it is in milliseconds.
Ok I will add.
> 
> > +
> > +#define PKT_OFST			0x00
> > +#define MEM_ADDR1_OFST			0x04
> > +#define MEM_ADDR2_OFST			0x08
> > +#define CMD_OFST			0x0C
> > +#define PROG_OFST			0x10
> > +#define INTR_STS_EN_OFST		0x14
> > +#define INTR_SIG_EN_OFST		0x18
> > +#define INTR_STS_OFST			0x1C
> > +#define READY_STS_OFST			0x20
> > +#define DMA_ADDR1_OFST			0x24
> > +#define FLASH_STS_OFST			0x28
> > +#define DATA_PORT_OFST			0x30
> > +#define ECC_OFST			0x34
> > +#define ECC_ERR_CNT_OFST		0x38
> > +#define ECC_SPR_CMD_OFST		0x3C
> > +#define ECC_ERR_CNT_1BIT_OFST		0x40
> > +#define ECC_ERR_CNT_2BIT_OFST		0x44
> > +#define DMA_ADDR0_OFST			0x50
> > +#define DATA_INTERFACE_OFST		0x6C
> > +
> > +#define PKT_CNT_SHIFT			12
> > +
> > +#define ECC_ENABLE			BIT(31)
> > +#define DMA_EN_MASK			GENMASK(27, 26)
> > +#define DMA_ENABLE			0x2
> > +#define DMA_EN_SHIFT			26
> > +#define REG_PAGE_SIZE_SHIFT		23
> > +#define REG_PAGE_SIZE_512		0
> > +#define REG_PAGE_SIZE_1K		5
> > +#define REG_PAGE_SIZE_2K		1
> > +#define REG_PAGE_SIZE_4K		2
> > +#define REG_PAGE_SIZE_8K		3
> > +#define REG_PAGE_SIZE_16K		4
> > +#define CMD2_SHIFT			8
> > +#define ADDR_CYCLES_SHIFT		28
> > +
> > +#define XFER_COMPLETE			BIT(2)
> > +#define READ_READY			BIT(1)
> > +#define WRITE_READY			BIT(0)
> > +#define MBIT_ERROR			BIT(3)
> > +
> > +#define PROG_PGRD			BIT(0)
> > +#define PROG_ERASE			BIT(2)
> > +#define PROG_STATUS			BIT(3)
> > +#define PROG_PGPROG			BIT(4)
> > +#define PROG_RDID			BIT(6)
> > +#define PROG_RDPARAM			BIT(7)
> > +#define PROG_RST			BIT(8)
> > +#define PROG_GET_FEATURE		BIT(9)
> > +#define PROG_SET_FEATURE		BIT(10)
> > +
> > +#define PG_ADDR_SHIFT			16
> > +#define BCH_MODE_SHIFT			25
> > +#define BCH_EN_SHIFT			27
> > +#define ECC_SIZE_SHIFT			16
> > +
> > +#define MEM_ADDR_MASK			GENMASK(7, 0)
> > +#define BCH_MODE_MASK			GENMASK(27, 25)
> > +
> > +#define CS_MASK				GENMASK(31, 30)
> > +#define CS_SHIFT			30
> > +
> > +#define PAGE_ERR_CNT_MASK		GENMASK(16, 8)
> > +#define PKT_ERR_CNT_MASK		GENMASK(7, 0)
> > +
> > +#define NVDDR_MODE			BIT(9)
> > +#define NVDDR_TIMING_MODE_SHIFT		3
> > +
> > +#define ONFI_ID_LEN			8
> > +#define TEMP_BUF_SIZE			1024
> > +#define NVDDR_MODE_PACKET_SIZE		8
> > +#define SDR_MODE_PACKET_SIZE		4
> > +
> > +#define ONFI_DATA_INTERFACE_NVDDR      BIT(4)
> > +#define EVENT_MASK	(XFER_COMPLETE | READ_READY | WRITE_READY |
> MBIT_ERROR)
> > +
> > +#define SDR_MODE_DEFLT_FREQ		80000000
> > +#define COL_ROW_ADDR(pos, val)			(((val) & 0xFF) << (8 * (pos)))
> 
> Can you try to group registers offsets with their fields?
Ok, I will update.
> 
> > +
> > +struct anfc_op {
> > +	s32 cmnds[4];
> 
> 	    ^ cmds?
Ok, I will correct it.
> 
> And why is it an s32 and not a u32?
To monitor NAND_CMD_STATUS.
Sometimes core will just send status command without reading back the status data and later
It will try to read one byte using ->exec_op().
So Arasan has FLASH_STS register and whenever we initiate a status command, the controller
Will update this register with the value returned by the flash device.
So we need to return this value when core is asking about 1 byte status value without issuing the command.
And in driver we are using memset(nfc_op, 0, sizeof(struct anfc_op)), this will make cmnds[4] to zeros but 0x0 is also
NAND_CMD_READ0, so inorder to differentiate whether to give status data or not, I just assigned 
	nfc_op->cmnds[0] = NAND_CMD_NONE;

May be this case we can now eliminate as per your suggestion(defining a separate hook for each pattern) and thanks for that.
> 
> > +	u32 type;
> > +	u32 len;
> > +	u32 naddrs;
> > +	u32 col;
> > +	u32 row;
> > +	unsigned int data_instr_idx;
> > +	unsigned int rdy_timeout_ms;
> > +	unsigned int rdy_delay_ns;
> > +	const struct nand_op_instr *data_instr; };
> 
> Please make sure you actually need to redefine all these fields. Looks like some them could be
> extracted directly from the nand_op_instr objects.
Ok, all these values are getting updated in anfc_parse_instructions()
> 
> > +
> > +/**
> > + * struct anfc_nand_chip - Defines the nand chip related information
> > + * @node:		used to store NAND chips into a list.
> > + * @chip:		NAND chip information structure.
> > + * @bch:		Bch / Hamming mode enable/disable.
> > + * @bchmode:		Bch mode.
> 
> What's the difference between bch and bchmode?
@bch -> to select error correction method(either BCH or Hamming)
@bchmode -> to select ECC correctability (4/8/12/24 bit ECC)
> 
> > + * @eccval:		Ecc config value.
> > + * @raddr_cycles:	Row address cycle information.
> > + * @caddr_cycles:	Column address cycle information.
> > + * @pktsize:		Packet size for read / write operation.
> > + * @csnum:		chipselect number to be used.
> 
> So that means you only support chips with a single CS, right?
Yes
> 
> > + * @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 bch;
> > +	u32 bchmode;
> > +	u32 eccval;
> > +	u16 raddr_cycles;
> > +	u16 caddr_cycles;
> > +	u32 pktsize;
> > +	int csnum;
> > +	u32 spktsize;
> > +	u32 inftimeval;
> > +};
> > +
> 
> [...]
> 
> > +
> > +static void anfc_rw_dma_op(struct mtd_info *mtd, uint8_t *buf, int len,
> > +			    bool do_read, u32 prog)
> > +{
> > +	dma_addr_t paddr;
> > +	struct nand_chip *chip = mtd_to_nand(mtd);
> > +	struct anfc_nand_controller *nfc = to_anfc(chip->controller);
> > +	struct anfc_nand_chip *achip = to_anfc_nand(chip);
> > +	u32 eccintr = 0, dir;
> > +	u32 pktsize = len, pktcount = 1;
> > +
> > +	if (((nfc->curr_cmd == NAND_CMD_READ0)) ||
> > +	    (nfc->curr_cmd == NAND_CMD_SEQIN && !nfc->iswriteoob)) {
> 
> No, really, this looks wrong. If you need special handling for the
> read_page() case, implement it in the chip->ecc.read_page[_raw]() hooks.
Let me check this.
> 
> > +		pktsize = achip->pktsize;
> > +		pktcount = DIV_ROUND_UP(mtd->writesize, pktsize);
> > +	}
> > +	anfc_setpktszcnt(nfc, pktsize, pktcount);
> > +
> > +	if (!achip->bch && nfc->curr_cmd == NAND_CMD_READ0)
> > +		eccintr = MBIT_ERROR;
> 
> Again, this should go in chip->ecc.read_page().
Let me try this approach, implementing ecc.read_page().
> 
> > +
> > +	if (do_read)
> > +		dir = DMA_FROM_DEVICE;
> > +	else
> > +		dir = DMA_TO_DEVICE;
> > +
> > +	paddr = dma_map_single(nfc->dev, buf, len, dir);
> > +	if (dma_mapping_error(nfc->dev, paddr)) {
> > +		dev_err(nfc->dev, "Read buffer mapping error");
> > +		return;
> > +	}
> > +	writel(paddr, nfc->base + DMA_ADDR0_OFST);
> > +	writel((paddr >> 32), nfc->base + DMA_ADDR1_OFST);
> > +	anfc_enable_intrs(nfc, (XFER_COMPLETE | eccintr));
> > +	writel(prog, nfc->base + PROG_OFST);
> > +	anfc_wait_for_event(nfc);
> > +	dma_unmap_single(nfc->dev, paddr, len, dir); }
> > +
> > +static void anfc_rw_pio_op(struct mtd_info *mtd, uint8_t *buf, int len,
> > +			    bool do_read, int prog)
> > +{
> > +	struct nand_chip *chip = mtd_to_nand(mtd);
> > +	struct anfc_nand_controller *nfc = to_anfc(chip->controller);
> > +	struct anfc_nand_chip *achip = to_anfc_nand(chip);
> > +	u32 *bufptr = (u32 *)buf;
> > +	u32 cnt = 0, intr = 0;
> > +	u32 pktsize = len, pktcount = 1;
> > +
> > +	anfc_config_dma(nfc, 0);
> > +
> > +	if (((nfc->curr_cmd == NAND_CMD_READ0)) ||
> > +	    (nfc->curr_cmd == NAND_CMD_SEQIN && !nfc->iswriteoob)) {
> > +		pktsize = achip->pktsize;
> > +		pktcount = DIV_ROUND_UP(mtd->writesize, pktsize);
> > +	}
> > +	anfc_setpktszcnt(nfc, pktsize, pktcount);
> > +
> > +	if (!achip->bch && nfc->curr_cmd == NAND_CMD_READ0)
> > +		intr = MBIT_ERROR;
> > +
> > +	if (do_read)
> > +		intr |= READ_READY;
> > +	else
> > +		intr |= WRITE_READY;
> > +
> > +	anfc_enable_intrs(nfc, intr);
> > +	writel(prog, nfc->base + PROG_OFST);
> > +	while (cnt < pktcount) {
> > +
> > +		anfc_wait_for_event(nfc);
> > +		cnt++;
> > +		if (cnt == pktcount)
> > +			anfc_enable_intrs(nfc, XFER_COMPLETE);
> > +		if (do_read)
> > +			ioread32_rep(nfc->base + DATA_PORT_OFST, bufptr,
> > +				     pktsize / 4);
> > +		else
> > +			iowrite32_rep(nfc->base + DATA_PORT_OFST, bufptr,
> > +				      pktsize / 4);
> > +		bufptr += (pktsize / 4);
> > +		if (cnt < pktcount)
> > +			anfc_enable_intrs(nfc, intr);
> > +	}
> > +	anfc_wait_for_event(nfc);
> > +}
> > +
> > +static void anfc_read_data_op(struct mtd_info *mtd, uint8_t *buf, int
> > +len)
> 
> Pass a nand_chip object directly and use u8 instead of uint8_t. This applies to all other
> internal functions you define.
Ok, i will do that.
> 
> > +{
> > +	struct nand_chip *chip = mtd_to_nand(mtd);
> > +	struct anfc_nand_controller *nfc = to_anfc(chip->controller);
> > +
> > +	if (nfc->dma && !is_vmalloc_addr(buf))
> 
> You should use virt_is_valid() not is_vmalloc_addr(). Alternatively, you can just set the
> NAND_USE_BOUNCE_BUFFER flag in chip->options, and you'll be guaranteed to have a
> DMA-able buffer passed to the
> chip->ecc.read/write_page_[raw]() hooks.
Sure, I will update it.
> 
> > +		anfc_rw_dma_op(mtd, buf, len, 1, PROG_PGRD);
> > +	else
> > +		anfc_rw_pio_op(mtd, buf, len, 1, PROG_PGRD); }
> > +
> > +static void anfc_write_data_op(struct mtd_info *mtd, const uint8_t *buf,
> > +			       int len)
> > +{
> > +	struct nand_chip *chip = mtd_to_nand(mtd);
> > +	struct anfc_nand_controller *nfc = to_anfc(chip->controller);
> > +
> > +	if (nfc->dma && !is_vmalloc_addr(buf))
> > +		anfc_rw_dma_op(mtd, (char *)buf, len, 0, PROG_PGPROG);
> > +	else
> > +		anfc_rw_pio_op(mtd, (char *)buf, len, 0, PROG_PGPROG); }
> > +
> > +static int anfc_prep_nand_instr(struct mtd_info *mtd, int cmd,
> > +				struct nand_chip *chip,  int col, int page) {
> > +	u8 addrs[5];
> > +
> > +	struct nand_op_instr instrs[] = {
> > +		NAND_OP_CMD(cmd, PSEC_TO_NSEC(1)),
> 
> Where do you get that delay from? Please don't use random delays.
As per your suggestion I will use core helpers, and I will remove these random delays.
> 
> > +		NAND_OP_ADDR(3, addrs, 0),
> > +	};
> > +	struct nand_operation op = NAND_OPERATION(instrs);
> > +
> > +	if (mtd->writesize <= 512) {
> > +		addrs[0] = col;
> > +		if (page != -1) {
> > +			addrs[1] = page;
> > +			addrs[2] = page >> 8;
> > +			instrs[1].ctx.addr.naddrs = 3;
> > +			if (chip->options & NAND_ROW_ADDR_3) {
> > +				addrs[3] = page >> 16;
> > +				instrs[1].ctx.addr.naddrs += 1;
> > +			}
> > +		} else {
> > +			instrs[1].ctx.addr.naddrs = 1;
> > +		}
> > +	} else {
> > +		addrs[0] = col;
> > +		addrs[1] = col >> 8;
> > +		if (page != -1) {
> > +			addrs[2] = page;
> > +			addrs[3] = page >> 8;
> > +			instrs[1].ctx.addr.naddrs = 4;
> > +			if (chip->options & NAND_ROW_ADDR_3) {
> > +				addrs[4] = page >> 16;
> > +				instrs[1].ctx.addr.naddrs += 1;
> > +			}
> > +		} else {
> > +			instrs[1].ctx.addr.naddrs = 2;
> > +		}
> > +	}
> 
> Hm, why do you need to do that? The core already provide appropriate helpers abstracting
> that for you. What's missing?
I didn't find any helper API that will do this command and page framing or maybe I am wrong.
The issue here is, I have to update command and address fields. I found one helper nand_fill_column_cycles(),
But it is static.
And also I just referred the driver drivers/mtd/nand/raw/nand_hynix.c, where hynix_nand_reg_write_op() is
Doing similar stuff, updating addr value.
And let me try as per your suggestion(use directly chip->oob_poi) if that works, I will remove all this code.

> 
> > +
> > +	return nand_exec_op(chip, &op);
> > +}
> > +
> > +static int anfc_nand_wait(struct mtd_info *mtd, struct nand_chip
> > +*chip) {
> > +	u8 status;
> > +	int ret;
> > +	unsigned long timeo;
> > +
> > +	/*
> > +	 * Apply this short delay always to ensure that we do wait tWB in any
> > +	 * case on any machine.
> > +	 */
> > +	ndelay(100);
> > +	timeo = jiffies + msecs_to_jiffies(STATUS_TIMEOUT);
> > +	do {
> > +		ret = nand_status_op(chip, &status);
> > +		if (ret)
> > +			return ret;
> > +
> > +		if (status & NAND_STATUS_READY)
> > +			break;
> > +		cond_resched();
> > +	} while (time_before(jiffies, timeo));
> 
> We have an helper doing that for you. Plus, ->waitfunc() should not be implemented when -
> >exec_op() is implemented.
Ok, got it, I will change it.
> 
> > +
> > +
> > +	return status;
> > +}
> > +
> > +static int anfc_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
> > +			  int page)
> > +{
> > +	struct anfc_nand_controller *nfc = to_anfc(chip->controller);
> > +
> > +	nfc->iswriteoob = true;
> > +	anfc_prep_nand_instr(mtd, NAND_CMD_SEQIN, chip, mtd->writesize, page);
> > +	anfc_write_data_op(mtd, chip->oob_poi, mtd->oobsize);
> > +	nfc->iswriteoob = false;
> 
> I'm really not a big fan of this ->iswriteoob var. Not sure why it's used for.
This is to differentiate whether the current operation is an OOB read or Page read.
Anyway, as you pointed, I will use chip->ecc.read/write_page_[raw]() hooks, which will eliminate these also.
> 
> > +
> > +	return 0;
> > +}
> 
> 
> > +static int anfc_write_page_hwecc(struct mtd_info *mtd,
> > +				 struct nand_chip *chip, const uint8_t *buf,
> > +				 int oob_required, int page)
> > +{
> > +	int ret;
> > +	struct anfc_nand_controller *nfc = to_anfc(chip->controller);
> > +	struct anfc_nand_chip *achip = to_anfc_nand(chip);
> > +	u8 status;
> > +	u8 *ecc_calc = chip->ecc.calc_buf;
> > +
> > +	ret = nand_prog_page_begin_op(chip, page, 0, NULL, 0);
> > +	if (ret)
> > +		return ret;
> > +
> > +	anfc_set_eccsparecmd(nfc, achip, NAND_CMD_RNDIN, 0);
> > +	anfc_config_ecc(nfc, true);
> > +	anfc_write_data_op(mtd, buf, mtd->writesize);
> > +
> > +	if (oob_required) {
> > +		status = anfc_nand_wait(mtd, chip);
> > +		if (status & NAND_STATUS_FAIL)
> > +			return -EIO;
> > +
> > +		anfc_prep_nand_instr(mtd, NAND_CMD_READOOB, chip, 0, page);
> > +		anfc_read_data_op(mtd, ecc_calc, mtd->oobsize);
> > +		ret = mtd_ooblayout_set_eccbytes(mtd, ecc_calc, chip->oob_poi,
> > +						 0, chip->ecc.total);
> > +		if (ret)
> > +			return ret;
> 
> No, that's not how we do. Just take the OOB bytes placed in
> chip->oob_poi.
Ok, let me check this.
> 
> > +
> > +		chip->ecc.write_oob(mtd, chip, page);
> > +	}
> > +	status = anfc_nand_wait(mtd, chip);
> > +	if (status & NAND_STATUS_FAIL)
> > +		return -EIO;
> > +
> > +	anfc_config_ecc(nfc, false);
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * anfc_get_mode_frm_timings - Converts sdr timing values to
> > +respective modes
> > + * @sdr: SDR NAND chip timings structure
> > + * Arasan NAND controller has Data Interface Register (0x6C)
> > + * which has timing mode configurations and need to program only the
> > +modes but
> > + * not timings. So this function returns SDR timing mode from SDR
> > +timing values
> > + *
> > + * Return: 	SDR timing mode on success, a negative error code otherwise.
> > + */
> > +static int anfc_get_mode_frm_timings(const struct nand_sdr_timings
> > +*sdr) {
> > +	if (sdr->tRC_min <= 20000)
> > +		return 5;
> > +	else if (sdr->tRC_min <= 25000)
> > +		return 4;
> > +	else if (sdr->tRC_min <= 30000)
> > +		return 3;
> > +	else if (sdr->tRC_min <= 35000)
> > +		return 2;
> > +	else if (sdr->tRC_min <= 50000)
> > +		return 1;
> > +	else if (sdr->tRC_min <= 100000)
> > +		return 0;
> > +	else
> > +		return -1;
> > +}
> 
> I'm sure we can add an onfi_timing_mode field in nand_sdr_timings so that you don't have to
> guess it based on ->tRC.
Ok, then its fine.
> 
> > +static int anfc_erase_function(struct nand_chip *chip,
> > +				   struct anfc_op nfc_op)
> > +{
> > +
> > +	struct anfc_nand_chip *achip = to_anfc_nand(chip);
> > +	struct anfc_nand_controller *nfc = to_anfc(chip->controller);
> > +
> > +	nfc->prog = PROG_ERASE;
> > +	anfc_prepare_cmd(nfc, nfc_op.cmnds[0], NAND_CMD_ERASE2, 0, 0,
> 
> Please don't guess the opcodes. The pattern descriptors are just here to describe a sequence of
> CMD, ADDR and DATA cycles, nothing more. The CMD opcodes can be tweaked if needed as
> long as the sequence is the same.
Ok, sure I will just use the pattern commands.
> 
> > +			 achip->raddr_cycles);
> > +	nfc_op.col = nfc_op.row & 0xffff;
> > +	nfc_op.row = (nfc_op.row >> PG_ADDR_SHIFT) & 0xffff;
> > +	anfc_setpagecoladdr(nfc, nfc_op.row, nfc_op.col);
> > +
> > +	anfc_enable_intrs(nfc, XFER_COMPLETE);
> > +	writel(nfc->prog, nfc->base + PROG_OFST);
> > +	anfc_wait_for_event(nfc);
> > +
> > +	return 0;
> > +}
> 
> > +static int anfc_reset_type_exec(struct nand_chip *chip,
> > +				   const struct nand_subop *subop) {
> > +	struct anfc_op nfc_op = {};
> > +	struct anfc_nand_controller *nfc = to_anfc(chip->controller);
> > +
> > +	anfc_parse_instructions(chip, subop, &nfc_op);
> > +	anfc_prepare_cmd(nfc, nfc_op.cmnds[0], 0, 0, 0, 0);
> > +	nfc->prog = PROG_RST;
> > +	anfc_enable_intrs(nfc, XFER_COMPLETE);
> > +	writel(nfc->prog, nfc->base + PROG_OFST);
> > +	anfc_wait_for_event(nfc);
> > +
> > +	return 0;
> > +}
> 
> This one looks correct.
Thanks and I will implement separate hooks for each pattern.
> 
> > +
> > +static const struct nand_op_parser anfc_op_parser = NAND_OP_PARSER
> > +	(NAND_OP_PARSER_PATTERN
> > +		(anfc_exec_op_cmd,
> > +		NAND_OP_PARSER_PAT_CMD_ELEM(false),
> > +		NAND_OP_PARSER_PAT_ADDR_ELEM(false, 7),
> > +		NAND_OP_PARSER_PAT_WAITRDY_ELEM(false),
> > +		NAND_OP_PARSER_PAT_DATA_IN_ELEM(true, 2048)),
> > +	NAND_OP_PARSER_PATTERN
> > +		(anfc_exec_op_cmd,
> > +		NAND_OP_PARSER_PAT_DATA_IN_ELEM(true, 2048)),
> > +	NAND_OP_PARSER_PATTERN
> > +		(anfc_exec_op_cmd,
> > +		NAND_OP_PARSER_PAT_CMD_ELEM(false),
> > +		NAND_OP_PARSER_PAT_ADDR_ELEM(false, 7),
> > +		NAND_OP_PARSER_PAT_CMD_ELEM(false),
> > +		NAND_OP_PARSER_PAT_WAITRDY_ELEM(false),
> > +		NAND_OP_PARSER_PAT_DATA_IN_ELEM(true, 2048)),
> > +	NAND_OP_PARSER_PATTERN
> > +		(anfc_exec_op_cmd,
> > +		NAND_OP_PARSER_PAT_CMD_ELEM(false),
> > +		NAND_OP_PARSER_PAT_ADDR_ELEM(false, 8),
> > +		NAND_OP_PARSER_PAT_DATA_OUT_ELEM(false, 2048),
> > +		//NAND_OP_PARSER_PAT_CMD_ELEM(true),
> > +		NAND_OP_PARSER_PAT_WAITRDY_ELEM(true)),
> > +	NAND_OP_PARSER_PATTERN
> > +		(anfc_exec_op_cmd,
> > +		NAND_OP_PARSER_PAT_CMD_ELEM(false),
> > +		NAND_OP_PARSER_PAT_ADDR_ELEM(false, 8),
> > +		NAND_OP_PARSER_PAT_CMD_ELEM(true),
> > +		NAND_OP_PARSER_PAT_DATA_IN_ELEM(true, 2048)),
> > +	NAND_OP_PARSER_PATTERN
> > +		(anfc_reset_type_exec,
> > +		NAND_OP_PARSER_PAT_CMD_ELEM(false),
> > +		NAND_OP_PARSER_PAT_WAITRDY_ELEM(false)),
> 
> And the reset pattern desc looks correct too.
> 
> > +	NAND_OP_PARSER_PATTERN
> > +		(anfc_status_type_exec,
> > +		NAND_OP_PARSER_PAT_CMD_ELEM(false),
> > +		NAND_OP_PARSER_PAT_DATA_IN_ELEM(true, 1)),
> > +	);
> > +
> 
> > +
> > +static void anfc_select_chip(struct mtd_info *mtd, int num) {
> > +
> > +	u32 val;
> > +	struct nand_chip *chip = mtd_to_nand(mtd);
> > +	struct anfc_nand_chip *achip = to_anfc_nand(chip);
> > +	struct anfc_nand_controller *nfc = to_anfc(chip->controller);
> > +
> > +	if (num == -1)
> > +		return;
> 
> You only support one CS per chip, so maybe you should check that num < 1.
Ok, I will update it.
> 
> > +
> > +	val = readl(nfc->base + MEM_ADDR2_OFST);
> > +	val &= (val & ~(CS_MASK | BCH_MODE_MASK));
> > +	val |= (achip->csnum << CS_SHIFT) | (achip->bchmode << BCH_MODE_SHIFT);
> > +	writel(val, nfc->base + MEM_ADDR2_OFST);
> > +	nfc->csnum = achip->csnum;
> > +	writel(achip->eccval, nfc->base + ECC_OFST);
> > +	writel(achip->inftimeval, nfc->base + DATA_INTERFACE_OFST); }
> > +
> > +static irqreturn_t anfc_irq_handler(int irq, void *ptr) {
> > +	struct anfc_nand_controller *nfc = ptr;
> > +	u32 status;
> > +
> > +	status = readl(nfc->base + INTR_STS_OFST);
> > +	if (status & EVENT_MASK) {
> > +		complete(&nfc->event);
> > +		writel((status & EVENT_MASK), nfc->base + INTR_STS_OFST);
> 
> 			^ parens uneeded here
> 
> > +		writel(0, nfc->base + INTR_STS_EN_OFST);
> > +		writel(0, nfc->base + INTR_SIG_EN_OFST);
> > +		return IRQ_HANDLED;
> > +	}
> > +
> > +	return IRQ_NONE;
> > +}
> 
> I haven't finished reviewing the driver but there are still a bunch of things that look strange,
> for instance, your ->read/write_page() implementation looks suspicious. Let's discuss that
> before you send a new version.
Ok, I will wait for it.
> 
> Also, please run checkpatch --strict and fix all errors and warnings (unless you have a good
> reason not to).
Sure, I will do that and thanks for your time.

Thanks,
Naga Sureshkumar Relli
> 
> Thanks,
> 
> Boris

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/