Hi Boris, Thanks for the review.
> -----Original Message----- > From: Boris Brezillon [mailto:boris.brezil...@bootlin.com] > Sent: Monday, August 20, 2018 10:10 PM > To: Naga Sureshkumar Relli <nagas...@xilinx.com> > Cc: miquel.ray...@bootlin.com; rich...@nod.at; dw...@infradead.org; > computersforpe...@gmail.com; marek.va...@gmail.com; kyungmin.p...@samsung.com; > abs...@codeaurora.org; peterpand...@micron.com; frieder.schre...@exceet.de; > linux- > m...@lists.infradead.org; linux-kernel@vger.kernel.org; Michal Simek > <mich...@xilinx.com>; > nagasureshkumarre...@gmail.com > 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.re...@xilinx.com> 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 <punn...@xilinx.com> > > + * Author: Naga Sureshkumar Relli <nagas...@xilinx.com> > > + * > > + */ > > +#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
