[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20180820184013.57fd7b5c@bbrezillon>
Date: Mon, 20 Aug 2018 18:40:13 +0200
From: Boris Brezillon <boris.brezillon@...tlin.com>
To: Naga Sureshkumar Relli <naga.sureshkumar.relli@...inx.com>
Cc: <miquel.raynal@...tlin.com>, <richard@....at>,
<dwmw2@...radead.org>, <computersforpeace@...il.com>,
<marek.vasut@...il.com>, <kyungmin.park@...sung.com>,
<absahu@...eaurora.org>, <peterpandong@...ron.com>,
<frieder.schrempf@...eet.de>, <linux-mtd@...ts.infradead.org>,
<linux-kernel@...r.kernel.org>, <michals@...inx.com>,
<nagasureshkumarrelli@...il.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.relli@...inx.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:
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@...inx.com>
> + * Author: Naga Sureshkumar Relli <nagasure@...inx.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.
> +#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"?
> +#define EVNT_TIMEOUT_MSEC 1000
It's unusual to have EVNT, it's usually EVT or EVENT.
> +#define STATUS_TIMEOUT 2000
Is it in milliseconds? Please add the proper _<UNIT> prefix here.
> +
> +#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?
> +
> +struct anfc_op {
> + s32 cmnds[4];
^ cmds?
And why is it an s32 and not a u32?
> + 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.
> +
> +/**
> + * 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?
> + * @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?
> + * @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.
> + 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().
> +
> + 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.
> +{
> + 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.
> + 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.
> + 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?
> +
> + 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.
> +
> +
> + 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.
> +
> + 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.
> +
> + 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.
> +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.
> + 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.
> +
> +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.
> +
> + 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.
Also, please run checkpatch --strict and fix all errors and warnings
(unless you have a good reason not to).
Thanks,
Boris
Powered by blists - more mailing lists