[RFC PATCH 2/2] nand: cavium: Nand flash controller for Cavium ARM64 SOCs
Jan Glauber
jan.glauber at caviumnetworks.com
Wed Mar 29 03:02:56 PDT 2017
On Wed, Mar 29, 2017 at 11:29:32AM +0200, Boris Brezillon wrote:
> Hi Jan,
>
> This is a preliminary review, I might come up with additional comments
> later on.
Hi Boris,
thanks for reviewing the driver. I'm exactly looking for comments about
the design and fundamental things I need to change. I didn't even bother
to run checkpatch on the RFC :).
> On Mon, 27 Mar 2017 18:05:24 +0200
> Jan Glauber <jglauber at cavium.com> wrote:
>
> > Add a driver for the nand flash controller as found on Cavium's
> > ARM64 SOCs.
> >
> > The nand flash controller can support up to 8 chips and is presented
> > as a PCI device. It uses a DMA engine to transfer data between the nand
> > and L2/DRAM and a programmable command queue to issue multiple
> > nand flash commands together.
> >
> > Signed-off-by: Jan Glauber <jglauber at cavium.com>
> > ---
> > drivers/mtd/nand/Kconfig | 6 +
> > drivers/mtd/nand/Makefile | 1 +
> > drivers/mtd/nand/cavium_nand.c | 1160 ++++++++++++++++++++++++++++++++++++++++
> > drivers/mtd/nand/cavium_nand.h | 231 ++++++++
> > 4 files changed, 1398 insertions(+)
> > create mode 100644 drivers/mtd/nand/cavium_nand.c
> > create mode 100644 drivers/mtd/nand/cavium_nand.h
> >
>
> [...]
>
> > +
> > +/* default parameters used for probing chips */
> > +static int default_onfi_timing = 0;
> > +static int default_width = 1; /* 8 bit */
> > +static int default_page_size = 2048;
> > +static struct ndf_set_tm_par_cmd default_timing_parms;
>
> Hm, I don't think this is a good idea to have default parameters. The
> NAND core should provide anything you need in term of NAND chip
> detection, so selection the timings, bus width and page size should not
> be a problem.
If the ONFI information can be parsed without these default I'm fine
with removing them.
> > +
> > +/*
> > + * Get the number of bits required to encode the column bits. This
> > + * does not include bits required for the OOB area.
> > + */
> > +static int ndf_get_column_bits(struct nand_chip *nand)
> > +{
> > + int page_size;
> > +
> > + if (!nand)
> > + page_size = default_page_size;
> > + else
> > + page_size = nand->onfi_params.byte_per_page;
> > + return get_bitmask_order(page_size - 1);
> > +}
>
> Please drop this helper, the page size is already exposed in
> mtd->writesize.
OK.
> > +
> > +irqreturn_t cvm_nfc_isr(int irq, void *dev_id)
> > +{
> > + struct cvm_nfc *tn = dev_id;
> > +
> > + wake_up(&tn->controller.wq);
> > + return IRQ_HANDLED;
> > +}
> > +
> > +/*
> > + * Read a single byte from the temporary buffer. Used after READID
> > + * to get the NAND information and for STATUS.
> > + */
> > +static u8 cvm_nand_read_byte(struct mtd_info *mtd)
> > +{
> > + struct nand_chip *nand = mtd_to_nand(mtd);
> > + struct cvm_nfc *tn = to_cvm_nfc(nand->controller);
> > +
> > + if (tn->use_status)
> > + return *tn->stat;
> > +
> > + if (tn->buf.data_index < tn->buf.data_len)
> > + return tn->buf.dmabuf[tn->buf.data_index++];
> > + else
> > + dev_err(tn->dev, "No data to read\n");
> > +
> > + return 0xff;
> > +}
> > +
> > +/*
> > + * Read a number of pending bytes from the temporary buffer. Used
> > + * to get page and OOB data.
> > + */
> > +static void cvm_nand_read_buf(struct mtd_info *mtd, u8 *buf, int len)
> > +{
> > + struct nand_chip *nand = mtd_to_nand(mtd);
> > + struct cvm_nfc *tn = to_cvm_nfc(nand->controller);
> > +
> > + if (len > tn->buf.data_len - tn->buf.data_index) {
> > + dev_err(tn->dev, "Not enough data for read of %d bytes\n", len);
> > + return;
> > + }
> > +
> > + memcpy(buf, tn->buf.dmabuf + tn->buf.data_index, len);
> > + tn->buf.data_index += len;
> > +}
> > +
> > +static void cvm_nand_write_buf(struct mtd_info *mtd, const u8 *buf, int len)
> > +{
> > + struct nand_chip *nand = mtd_to_nand(mtd);
> > + struct cvm_nfc *tn = to_cvm_nfc(nand->controller);
> > +
> > + memcpy(tn->buf.dmabuf + tn->buf.data_len, buf, len);
> > + tn->buf.data_len += len;
> > +}
>
> It seems that cvm_nand_read/write_byte/buf() are returning data that
> have already been retrieved (problably during the ->cmdfunc() phase).
Yes.
> That's not how it's supposed to work. The core is expecting the data
> transfer to be done when ->read/write_buf() is called. Doing that in
> ->cmdfunc() is risky, because when you're there you have no clue about
> how much bytes the core expect.
It seems to work fine, I've never seen the core trying to do more bytes in
the read/write_buf() then requested in ->cmdfunc().
> [...]
>
> > +static int cvm_nand_set_features(struct mtd_info *mtd,
> > + struct nand_chip *chip, int feature_addr,
> > + u8 *subfeature_para)
>
> Do you really need you own implementation of ->set_feature()?
>
> > +{
> > + struct nand_chip *nand = mtd_to_nand(mtd);
> > + struct cvm_nfc *tn = to_cvm_nfc(nand->controller);
> > + int rc;
> > +
> > + rc = ndf_build_pre_cmd(tn, NAND_CMD_SET_FEATURES, 1, feature_addr, 0);
> > + if (rc)
> > + return rc;
> > +
> > + memcpy(tn->buf.dmabuf, subfeature_para, 4);
> > + memset(tn->buf.dmabuf + 4, 0, 4);
> > +
> > + rc = ndf_queue_cmd_write(tn, 8);
> > + if (rc)
> > + return rc;
> > +
> > + ndf_setup_dma(tn, 0, tn->buf.dmaaddr, 8);
> > +
> > + rc = ndf_wait_for_busy_done(tn);
> > + if (rc)
> > + return rc;
> > +
> > + rc = ndf_build_post_cmd(tn);
> > + if (rc)
> > + return rc;
> > + return 0;
> > +}
> > +
>
> [...]
>
> > +
> > +/*
> > + * Read a page from NAND. If the buffer has room, the out of band
> > + * data will be included.
> > + */
> > +int ndf_page_read(struct cvm_nfc *tn, u64 addr, int len)
> > +{
> > + int rc;
> > +
> > + memset(tn->buf.dmabuf, 0xff, len);
> > + rc = ndf_read(tn, NAND_CMD_READ0, 4, addr, NAND_CMD_READSTART, len);
> > + if (rc)
> > + return rc;
> > +
> > + return rc;
> > +}
> > +
> > +/* Erase a NAND block */
> > +static int ndf_block_erase(struct cvm_nfc *tn, u64 addr)
>
> Ditto.
>
> > +{
> > + struct nand_chip *nand = tn->controller.active;
> > + int row, rc;
> > +
> > + row = addr >> ndf_get_column_bits(nand);
> > + rc = ndf_build_pre_cmd(tn, NAND_CMD_ERASE1, 2, row, NAND_CMD_ERASE2);
> > + if (rc)
> > + return rc;
> > +
> > + /* Wait for R_B to signal erase is complete */
> > + rc = ndf_wait_for_busy_done(tn);
> > + if (rc)
> > + return rc;
> > +
> > + rc = ndf_build_post_cmd(tn);
> > + if (rc)
> > + return rc;
>
> Use tabs and not spaces for indentation (you can run checkpatch to
> identify these coding style issues).
I'll fix these in the next version.
> > +
> > + /* Wait until the command queue is idle */
> > + return ndf_wait_idle(tn);
> > +}
> > +
>
> [...]
>
>
> > +
> > +static void cvm_nand_cmdfunc(struct mtd_info *mtd, unsigned int command,
> > + int column, int page_addr)
>
> Did you try implementing ->cmd_ctrl() instead of ->cmdfunc(). Your
> controller seems to be highly configurable and, at first glance, I think
> you can simplify the driver by implementing ->cmd_ctrl() and relying on
> the default ->cmdfunc() implementation.
>
I've looked at the sunxi_nand driver but ->cmd_ctrl() is very different
from ->cmdfunc() and the later looks like a better match for our controller.
The Cavium controller needs to write the commands (NAND_CMD_READ0, etc.)
into its pseudo instructions (see ndf_queue_cmd_cle()).
So how can I do this low-level stuff with ->cmd_ctrl()?
For instance for reading data I have ndf_page_read() that is used for
both NAND_CMD_READ0 and NAND_CMD_READOOB. Without hacking into ->cmdfunc(),
how would I differentiate between the two commands in read_buf()?
> > +{
> > + struct nand_chip *nand = mtd_to_nand(mtd);
> > + struct cvm_nand_chip *cvm_nand = to_cvm_nand(nand);
> > + struct cvm_nfc *tn = to_cvm_nfc(nand->controller);
> > + int rc;
> > +
> > + tn->selected_chip = cvm_nand->cs;
> > + if (tn->selected_chip < 0 || tn->selected_chip >= NAND_MAX_CHIPS) {
> > + dev_err(tn->dev, "invalid chip select\n");
> > + return;
> > + }
> > +
> > + tn->use_status = false;
> > + cvm_nand->oob_access = false;
> > +
> > + switch (command) {
> > + case NAND_CMD_READID:
> > + tn->buf.data_index = 0;
> > + memset(tn->buf.dmabuf, 0xff, 8);
> > + rc = ndf_read(tn, command, 1, column, 0, 8);
> > + if (rc < 0)
> > + dev_err(tn->dev, "READID failed with %d\n", rc);
> > + else
> > + tn->buf.data_len = rc;
> > + break;
> > +
> > + case NAND_CMD_READOOB:
> > + cvm_nand->oob_access = true;
> > + tn->buf.data_index = 0;
> > + tn->buf.data_len = ndf_page_read(tn,
> > + (page_addr << nand->page_shift) + 0x800,
> > + mtd->oobsize);
> > +
> > + if (tn->buf.data_len < mtd->oobsize) {
> > + dev_err(tn->dev, "READOOB failed with %d\n",
> > + tn->buf.data_len);
> > + tn->buf.data_len = 0;
> > + }
> > + break;
> > +
> > + case NAND_CMD_READ0:
> > + tn->buf.data_index = 0;
> > + tn->buf.data_len = ndf_page_read(tn,
> > + column + (page_addr << nand->page_shift),
> > + (1 << nand->page_shift) + mtd->oobsize);
> > + if (tn->buf.data_len < (1 << nand->page_shift) + mtd->oobsize) {
> > + dev_err(tn->dev, "READ0 failed with %d\n",
> > + tn->buf.data_len);
> > + tn->buf.data_len = 0;
> > + }
> > + break;
> > +
> > + case NAND_CMD_STATUS:
> > + tn->use_status = true;
> > + memset(tn->stat, 0xff, 8);
> > + rc = ndf_read(tn, command, 0, 0, 0, 8);
> > + if (rc < 0)
> > + dev_err(tn->dev, "STATUS failed with %d\n", rc);
> > + break;
> > +
> > + case NAND_CMD_RESET:
> > + tn->buf.data_index = 0;
> > + tn->buf.data_len = 0;
> > + memset(tn->buf.dmabuf, 0xff, tn->buf.dmabuflen);
> > + rc = cvm_nand_reset(tn);
> > + if (rc < 0)
> > + dev_err(tn->dev, "RESET failed with %d\n", rc);
> > + break;
> > +
> > + case NAND_CMD_PARAM:
> > + tn->buf.data_index = column;
> > + memset(tn->buf.dmabuf, 0xff, tn->buf.dmabuflen);
> > + rc = ndf_read(tn, command, 1, 0, 0, 2048);
> > + if (rc < 0)
> > + dev_err(tn->dev, "PARAM failed with %d\n", rc);
> > + else
> > + tn->buf.data_len = rc;
> > + break;
> > +
> > + case NAND_CMD_RNDOUT:
> > + tn->buf.data_index = column;
> > + break;
> > +
> > + case NAND_CMD_ERASE1:
> > + if (ndf_block_erase(tn, page_addr << nand->page_shift))
> > + dev_err(tn->dev, "ERASE1 failed\n");
> > + break;
> > +
> > + case NAND_CMD_ERASE2:
> > + /* We do all erase processing in the first command, so ignore
> > + * this one.
> > + */
> > + break;
> > +
> > + case NAND_CMD_SEQIN:
> > + if (column == mtd->writesize)
> > + cvm_nand->oob_access = true;
> > + tn->buf.data_index = column;
> > + tn->buf.data_len = column;
> > + cvm_nand->selected_page = page_addr;
> > + break;
> > +
> > + case NAND_CMD_PAGEPROG:
> > + rc = ndf_page_write(tn,
> > + cvm_nand->selected_page << nand->page_shift);
> > + if (rc)
> > + dev_err(tn->dev, "PAGEPROG failed with %d\n", rc);
> > + break;
> > +
> > + default:
> > + WARN_ON_ONCE(1);
> > + dev_err(tn->dev, "unhandled nand cmd: %x\n", command);
> > + }
> > +}
> > +
> > +static int cvm_nfc_chip_init_timings(struct cvm_nand_chip *chip,
> > + struct device_node *np)
> > +{
> > + const struct nand_sdr_timings *timings;
> > + int ret, mode;
> > +
> > + mode = onfi_get_async_timing_mode(&chip->nand);
> > + if (mode == ONFI_TIMING_MODE_UNKNOWN) {
> > + mode = chip->nand.onfi_timing_mode_default;
> > + } else {
> > + u8 feature[ONFI_SUBFEATURE_PARAM_LEN] = {};
> > +
> > + mode = fls(mode) - 1;
> > + if (mode < 0)
> > + mode = 0;
> > +
> > + feature[0] = mode;
> > + ret = chip->nand.onfi_set_features(&chip->nand.mtd, &chip->nand,
> > + ONFI_FEATURE_ADDR_TIMING_MODE,
> > + feature);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + timings = onfi_async_timing_mode_to_sdr_timings(mode);
> > + if (IS_ERR(timings))
> > + return PTR_ERR(timings);
> > +
> > + return cvm_nfc_chip_set_timings(chip, timings);
> > +}
>
>
> Please have a look at the ->setup_data_interface() hook [1] instead of
> initializing the timings at probe/init time.
OK.
> [...]
>
> > diff --git a/drivers/mtd/nand/cavium_nand.h b/drivers/mtd/nand/cavium_nand.h
> > new file mode 100644
> > index 0000000..7030c57
> > --- /dev/null
> > +++ b/drivers/mtd/nand/cavium_nand.h
> > @@ -0,0 +1,231 @@
> > +#ifndef _CAVIUM_NAND_H
> > +#define _CAVIUM_NAND_H
> > +
> > +#include <linux/bitops.h>
> > +
> > +/*
> > + * The NDF_CMD queue takes commands between 16 - 128 bit.
> > + * All commands must be 16 bit aligned and are little endian.
> > + * WAIT_STATUS commands must be 64 bit aligned.
> > + * Commands are selected by the 4 bit opcode.
> > + *
> > + * Available Commands:
> > + *
> > + * 16 Bit:
> > + * NOP
> > + * WAIT
> > + * BUS_ACQ, BUS_REL
> > + * CHIP_EN, CHIP_DIS
> > + *
> > + * 32 Bit:
> > + * CLE_CMD
> > + * RD_CMD, RD_EDO_CMD
> > + * WR_CMD
> > + *
> > + * 64 Bit:
> > + * SET_TM_PAR
> > + *
> > + * 96 Bit:
> > + * ALE_CMD
> > + *
> > + * 128 Bit:
> > + * WAIT_STATUS, WAIT_STATUS_ALE
> > + */
> > +
> > +/* NDF Register offsets */
> > +#define NDF_CMD 0x0
> > +#define NDF_MISC 0x8
> > +#define NDF_ECC_CNT 0x10
> > +#define NDF_DRBELL 0x30
> > +#define NDF_ST_REG 0x38 /* status */
> > +#define NDF_INT 0x40
> > +#define NDF_INT_W1S 0x48
> > +#define NDF_DMA_CFG 0x50
> > +#define NDF_DMA_ADR 0x58
> > +#define NDF_INT_ENA_W1C 0x60
> > +#define NDF_INT_ENA_W1S 0x68
> > +
> > +/* NDF command opcodes */
> > +#define NDF_OP_NOP 0x0
> > +#define NDF_OP_SET_TM_PAR 0x1
> > +#define NDF_OP_WAIT 0x2
> > +#define NDF_OP_CHIP_EN_DIS 0x3
> > +#define NDF_OP_CLE_CMD 0x4
> > +#define NDF_OP_ALE_CMD 0x5
> > +#define NDF_OP_WR_CMD 0x8
> > +#define NDF_OP_RD_CMD 0x9
> > +#define NDF_OP_RD_EDO_CMD 0xa
> > +#define NDF_OP_WAIT_STATUS 0xb /* same opcode for WAIT_STATUS_ALE */
> > +#define NDF_OP_BUS_ACQ_REL 0xf
> > +
> > +#define NDF_BUS_ACQUIRE 1
> > +#define NDF_BUS_RELEASE 0
> > +
> > +struct ndf_nop_cmd {
> > + u16 opcode : 4;
> > + u16 nop : 12;
> > +};
> > +
> > +struct ndf_wait_cmd {
> > + u16 opcode : 4;
> > + u16 r_b : 1; /* wait for one cycle or PBUS_WAIT deassert */
> > + u16 : 3;
> > + u16 wlen : 3; /* timing parameter select */
> > + u16 : 5;
> > +};
> > +
> > +struct ndf_bus_cmd {
> > + u16 opcode : 4;
> > + u16 direction : 4; /* 1 = acquire, 0 = release */
> > + u16 : 8;
> > +};
> > +
> > +struct ndf_chip_cmd {
> > + u16 opcode : 4;
> > + u16 chip : 3; /* select chip, 0 = disable */
> > + u16 enable : 1; /* 1 = enable, 0 = disable */
> > + u16 bus_width : 2; /* 10 = 16 bit, 01 = 8 bit */
> > + u16 : 6;
> > +};
> > +
> > +struct ndf_cle_cmd {
> > + u32 opcode : 4;
> > + u32 : 4;
> > + u32 cmd_data : 8; /* command sent to the PBUS AD pins */
> > + u32 clen1 : 3; /* time between PBUS CLE and WE asserts */
> > + u32 clen2 : 3; /* time WE remains asserted */
> > + u32 clen3 : 3; /* time between WE deassert and CLE */
> > + u32 : 7;
> > +};
> > +
> > +/* RD_EDO_CMD uses the same layout as RD_CMD */
> > +struct ndf_rd_cmd {
> > + u32 opcode : 4;
> > + u32 data : 16; /* data bytes */
> > + u32 rlen1 : 3;
> > + u32 rlen2 : 3;
> > + u32 rlen3 : 3;
> > + u32 rlen4 : 3;
> > +};
> > +
> > +struct ndf_wr_cmd {
> > + u32 opcode : 4;
> > + u32 data : 16; /* data bytes */
> > + u32 : 4;
> > + u32 wlen1 : 3;
> > + u32 wlen2 : 3;
> > + u32 : 3;
> > +};
> > +
> > +struct ndf_set_tm_par_cmd {
> > + u64 opcode : 4;
> > + u64 tim_mult : 4; /* multiplier for the seven paramters */
> > + u64 tm_par1 : 8; /* --> Following are the 7 timing parameters that */
> > + u64 tm_par2 : 8; /* specify the number of coprocessor cycles. */
> > + u64 tm_par3 : 8; /* A value of zero means one cycle. */
> > + u64 tm_par4 : 8; /* All values are scaled by tim_mult */
> > + u64 tm_par5 : 8; /* using tim_par * (2 ^ tim_mult). */
> > + u64 tm_par6 : 8;
> > + u64 tm_par7 : 8;
> > +};
> > +
> > +struct ndf_ale_cmd {
> > + u32 opcode : 4;
> > + u32 : 4;
> > + u32 adr_byte_num: 4; /* number of address bytes to be sent */
> > + u32 : 4;
> > + u32 alen1 : 3;
> > + u32 alen2 : 3;
> > + u32 alen3 : 3;
> > + u32 alen4 : 3;
> > + u32 : 4;
> > + u8 adr_byt1;
> > + u8 adr_byt2;
> > + u8 adr_byt3;
> > + u8 adr_byt4;
> > + u8 adr_byt5;
> > + u8 adr_byt6;
> > + u8 adr_byt7;
> > + u8 adr_byt8;
> > +};
> > +
> > +struct ndf_wait_status_cmd {
> > + u32 opcode : 4;
> > + u32 : 4;
> > + u32 data : 8; /* data */
> > + u32 clen1 : 3;
> > + u32 clen2 : 3;
> > + u32 clen3 : 3;
> > + u32 : 8;
> > + u32 ale_ind : 8; /* set to 5 to select WAIT_STATUS_ALE command */
> > + u32 adr_byte_num: 4; /* ALE only: number of address bytes to be sent */
> > + u32 : 4;
> > + u32 alen1 : 3; /* ALE only */
> > + u32 alen2 : 3; /* ALE only */
> > + u32 alen3 : 3; /* ALE only */
> > + u32 alen4 : 3; /* ALE only */
> > + u32 : 4;
> > + u8 adr_byt[4]; /* ALE only */
> > + u32 nine : 4; /* set to 9 */
> > + u32 and_mask : 8;
> > + u32 comp_byte : 8;
> > + u32 rlen1 : 3;
> > + u32 rlen2 : 3;
> > + u32 rlen3 : 3;
> > + u32 rlen4 : 3;
> > +};
> > +
> > +union ndf_cmd {
> > + u64 val[2];
> > + union {
> > + struct ndf_nop_cmd nop;
> > + struct ndf_wait_cmd wait;
> > + struct ndf_bus_cmd bus_acq_rel;
> > + struct ndf_chip_cmd chip_en_dis;
> > + struct ndf_cle_cmd cle_cmd;
> > + struct ndf_rd_cmd rd_cmd;
> > + struct ndf_wr_cmd wr_cmd;
> > + struct ndf_set_tm_par_cmd set_tm_par;
> > + struct ndf_ale_cmd ale_cmd;
> > + struct ndf_wait_status_cmd wait_status;
> > + } u;
> > +};
>
> I need some time to process all these information, but your controller
> seems to be a complex/highly-configurable beast. That's really
> interesting :-).
> I'll come up with more comments/question after reviewing more carefully
> the command creation logic.
Great. I'm afraid out controller is quite different from existing
hardware, at least I didn't find a driver that does things simalar (like
the command building and queueing).
I'm happy to help with any more information you need about our hardware.
Thanks!
Jan
> Thanks,
>
> Boris
>
> [1]http://lxr.free-electrons.com/source/include/linux/mtd/nand.h#L854
More information about the linux-mtd
mailing list