LinuxLists.cc - [PATCH v2 0/5] mtd: Qualcomm NAND controller driver

[permalink] [raw]

Subject: [PATCH v2 1/5] mtd: nand: Create a BBT flag to access bad block markers in raw mode

Some controllers can access the factory bad block marker from OOB only
when they read it in raw mode. When ECC is enabled, these controllers
discard reading/writing bad block markers, preventing access to them
altogether.

The bbt driver assumes MTD_OPS_PLACE_OOB when scanning for bad blocks.
This results in the nand driver's ecc->read_oob() op to be called, which
works with ECC enabled.

Create a new BBT option flag that tells nand_bbt to force the mode to
MTD_OPS_RAW. This would result in the correct op being called for the
underlying nand controller driver.

Signed-off-by: Archit Taneja <[email protected]>
---
drivers/mtd/nand/nand_base.c | 6 +++++-
drivers/mtd/nand/nand_bbt.c | 6 +++++-
include/linux/mtd/bbm.h | 7 +++++++
3 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index ceb68ca..0a0c524 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -394,7 +394,11 @@ static int nand_default_block_markbad(struct mtd_info *mtd, loff_t ofs)
} else {
ops.len = ops.ooblen = 1;
}
- ops.mode = MTD_OPS_PLACE_OOB;
+
+ if (unlikely(chip->bbt_options & NAND_BBT_ACCESS_BBM_RAW))
+ ops.mode = MTD_OPS_RAW;
+ else
+ ops.mode = MTD_OPS_PLACE_OOB;

/* Write to first/last page(s) if necessary */
if (chip->bbt_options & NAND_BBT_SCANLASTPAGE)
diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
index 63a1a36..f2d89c9 100644
--- a/drivers/mtd/nand/nand_bbt.c
+++ b/drivers/mtd/nand/nand_bbt.c
@@ -420,7 +420,11 @@ static int scan_block_fast(struct mtd_info *mtd, struct nand_bbt_descr *bd,
ops.oobbuf = buf;
ops.ooboffs = 0;
ops.datbuf = NULL;
- ops.mode = MTD_OPS_PLACE_OOB;
+
+ if (unlikely(bd->options & NAND_BBT_ACCESS_BBM_RAW))
+ ops.mode = MTD_OPS_RAW;
+ else
+ ops.mode = MTD_OPS_PLACE_OOB;

for (j = 0; j < numpages; j++) {
/*
diff --git a/include/linux/mtd/bbm.h b/include/linux/mtd/bbm.h
index 36bb6a5..f67f84a 100644
--- a/include/linux/mtd/bbm.h
+++ b/include/linux/mtd/bbm.h
@@ -116,6 +116,13 @@ struct nand_bbt_descr {
#define NAND_BBT_NO_OOB_BBM 0x00080000

/*
+ * Force MTD_OPS_RAW mode when trying to access bad block markes from OOB. To
+ * be used by controllers which can access BBM only when ECC is disabled, i.e,
+ * when in RAW access mode
+ */
+#define NAND_BBT_ACCESS_BBM_RAW 0x00100000
+
+/*
* Flag set by nand_create_default_bbt_descr(), marking that the nand_bbt_descr
* was allocated dynamicaly and must be freed in nand_release(). Has no meaning
* in nand_chip.bbt_options.
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

2015-07-21 10:35:15

[permalink] [raw]

Subject: [PATCH v2 2/5] mtd: nand: Qualcomm NAND controller driver

The Qualcomm NAND controller is found in SoCs like IPQ806x, MSM7xx,
MDM9x15 series.

It exists as a sub block inside the IPs EBI2 (External Bus Interface 2)
and QPIC (Qualcomm Parallel Interface Controller). These IPs provide a
broader interface for external slow peripheral devices such as LCD and
NAND/NOR flash memory or SRAM like interfaces.

We add support for the NAND controller found within EBI2. For the SoCs
of our interest, we only use the NAND controller within EBI2. Therefore,
it's safe for us to assume that the NAND controller is a standalone block
within the SoC.

The controller supports 512B, 2kB, 4kB and 8kB page 8-bit and 16-bit NAND
flash devices. It contains a HW ECC block that supports BCH ECC (4, 8 and
16 bit correction/step) and RS ECC(4 bit correction/step) that covers main
and spare data. The controller contains an internal 512 byte page buffer
to which we read/write via DMA. The EBI2 type NAND controller uses ADM DMA
for register read/write and data transfers. The controller performs page
reads and writes at a codeword/step level of 512 bytes. It can support up
to 2 external chips of different configurations.

The driver prepares register read and write configuration descriptors for
each codeword, followed by data descriptors to read or write data from the
controller's internal buffer. It uses a single ADM DMA channel that we get
via dmaengine API. The controller requires 2 ADM CRCIs for command and
data flow control. These are passed via DT.

The ecc layout used by the controller is syndrome like, but we can't use
the standard syndrome ecc ops because of several reasons. First, the amount
of data bytes covered by ecc isn't same in each step. Second, writing to
free oob space requires us writing to the entire step in which the oob
lies. This forces us to create our own ecc ops.

One more difference is how the controller accesses the bad block marker.
The controller ignores reading the marker when ECC is enabled. ECC needs
to be explicity disabled to read or write to the bad block marker. For
this reason, we use the newly created flag NAND_BBT_ACCESS_BBM_RAW to
read the factory provided bad block markers.

Signed-off-by: Archit Taneja <[email protected]>
---
drivers/mtd/nand/Kconfig | 7 +
drivers/mtd/nand/Makefile | 1 +
drivers/mtd/nand/qcom_nandc.c | 2019 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 2027 insertions(+)
create mode 100644 drivers/mtd/nand/qcom_nandc.c

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 5b2806a..31951fc 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -538,4 +538,11 @@ config MTD_NAND_HISI504
help
Enables support for NAND controller on Hisilicon SoC Hip04.

+config MTD_NAND_QCOM
+ tristate "Support for NAND on QCOM SoCs"
+ depends on ARCH_QCOM && QCOM_ADM
+ help
+ Enables support for NAND flash chips on SoCs containing the EBI2 NAND
+ controller. This controller is found on IPQ806x SoC.
+
endif # MTD_NAND
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index 1f897ec..87b6a1d 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -53,5 +53,6 @@ obj-$(CONFIG_MTD_NAND_BCM47XXNFLASH) += bcm47xxnflash/
obj-$(CONFIG_MTD_NAND_SUNXI) += sunxi_nand.o
obj-$(CONFIG_MTD_NAND_HISI504) += hisi504_nand.o
obj-$(CONFIG_MTD_NAND_BRCMNAND) += brcmnand/
+obj-$(CONFIG_MTD_NAND_QCOM) += qcom_nandc.o

nand-objs := nand_base.o nand_bbt.o nand_timings.o
diff --git a/drivers/mtd/nand/qcom_nandc.c b/drivers/mtd/nand/qcom_nandc.c
new file mode 100644
index 0000000..51c284c
--- /dev/null
+++ b/drivers/mtd/nand/qcom_nandc.c
@@ -0,0 +1,2019 @@
+/*
+ * Copyright (c) 2015, The Linux Foundation. All rights reserved.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk.h>
+#include <linux/slab.h>
+#include <linux/interrupt.h>
+#include <linux/bitops.h>
+#include <linux/dma-mapping.h>
+#include <linux/dmaengine.h>
+#include <linux/module.h>
+#include <linux/mtd/nand.h>
+#include <linux/mtd/partitions.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_mtd.h>
+#include <linux/delay.h>
+
+/* NANDc reg offsets */
+#define NAND_FLASH_CMD 0x00
+#define NAND_ADDR0 0x04
+#define NAND_ADDR1 0x08
+#define NAND_FLASH_CHIP_SELECT 0x0c
+#define NAND_EXEC_CMD 0x10
+#define NAND_FLASH_STATUS 0x14
+#define NAND_BUFFER_STATUS 0x18
+#define NAND_DEV0_CFG0 0x20
+#define NAND_DEV0_CFG1 0x24
+#define NAND_DEV0_ECC_CFG 0x28
+#define NAND_DEV1_ECC_CFG 0x2c
+#define NAND_DEV1_CFG0 0x30
+#define NAND_DEV1_CFG1 0x34
+#define NAND_READ_ID 0x40
+#define NAND_READ_STATUS 0x44
+#define NAND_DEV_CMD0 0xa0
+#define NAND_DEV_CMD1 0xa4
+#define NAND_DEV_CMD2 0xa8
+#define NAND_DEV_CMD_VLD 0xac
+#define SFLASHC_BURST_CFG 0xe0
+#define NAND_ERASED_CW_DETECT_CFG 0xe8
+#define NAND_ERASED_CW_DETECT_STATUS 0xec
+#define NAND_EBI2_ECC_BUF_CFG 0xf0
+#define FLASH_BUF_ACC 0x100
+
+#define NAND_CTRL 0xf00
+#define NAND_VERSION 0xf08
+#define NAND_READ_LOCATION_0 0xf20
+#define NAND_READ_LOCATION_1 0xf24
+
+/* NAND_FLASH_CMD bits */
+#define PAGE_ACC BIT(4)
+#define LAST_PAGE BIT(5)
+
+/* NAND_FLASH_CHIP_SELECT bits */
+#define NAND_DEV_SEL 0
+#define DM_EN BIT(2)
+
+/* NAND_FLASH_STATUS bits */
+#define FS_OP_ERR BIT(4)
+#define FS_READY_BSY_N BIT(5)
+#define FS_MPU_ERR BIT(8)
+#define FS_DEVICE_STS_ERR BIT(16)
+#define FS_DEVICE_WP BIT(23)
+
+/* NAND_BUFFER_STATUS bits */
+#define BS_UNCORRECTABLE_BIT BIT(8)
+#define BS_CORRECTABLE_ERR_MSK 0x1f
+
+/* NAND_DEVn_CFG0 bits */
+#define DISABLE_STATUS_AFTER_WRITE 4
+#define CW_PER_PAGE 6
+#define UD_SIZE_BYTES 9
+#define ECC_PARITY_SIZE_BYTES_RS 19
+#define SPARE_SIZE_BYTES 23
+#define NUM_ADDR_CYCLES 27
+#define STATUS_BFR_READ 30
+#define SET_RD_MODE_AFTER_STATUS 31
+
+/* NAND_DEVn_CFG0 bits */
+#define DEV0_CFG1_ECC_DISABLE 0
+#define WIDE_FLASH 1
+#define NAND_RECOVERY_CYCLES 2
+#define CS_ACTIVE_BSY 5
+#define BAD_BLOCK_BYTE_NUM 6
+#define BAD_BLOCK_IN_SPARE_AREA 16
+#define WR_RD_BSY_GAP 17
+#define ENABLE_BCH_ECC 27
+
+/* NAND_DEV0_ECC_CFG bits */
+#define ECC_CFG_ECC_DISABLE 0
+#define ECC_SW_RESET 1
+#define ECC_MODE 4
+#define ECC_PARITY_SIZE_BYTES_BCH 8
+#define ECC_NUM_DATA_BYTES 16
+#define ECC_FORCE_CLK_OPEN 30
+
+/* NAND_DEV_CMD1 bits */
+#define READ_ADDR 0
+
+/* NAND_DEV_CMD_VLD bits */
+#define READ_START_VLD 0
+
+/* NAND_EBI2_ECC_BUF_CFG bits */
+#define NUM_STEPS 0
+
+/* NAND_ERASED_CW_DETECT_CFG bits */
+#define ERASED_CW_ECC_MASK 1
+#define AUTO_DETECT_RES 0
+#define MASK_ECC (1 << ERASED_CW_ECC_MASK)
+#define RESET_ERASED_DET (1 << AUTO_DETECT_RES)
+#define ACTIVE_ERASED_DET (0 << AUTO_DETECT_RES)
+#define CLR_ERASED_PAGE_DET (RESET_ERASED_DET | MASK_ECC)
+#define SET_ERASED_PAGE_DET (ACTIVE_ERASED_DET | MASK_ECC)
+
+/* NAND_ERASED_CW_DETECT_STATUS bits */
+#define PAGE_ALL_ERASED BIT(7)
+#define CODEWORD_ALL_ERASED BIT(6)
+#define PAGE_ERASED BIT(5)
+#define CODEWORD_ERASED BIT(4)
+#define ERASED_PAGE (PAGE_ALL_ERASED | PAGE_ERASED)
+#define ERASED_CW (CODEWORD_ALL_ERASED | CODEWORD_ERASED)
+
+/* Version Mask */
+#define NAND_VERSION_MAJOR_MASK 0xf0000000
+#define NAND_VERSION_MAJOR_SHIFT 28
+#define NAND_VERSION_MINOR_MASK 0x0fff0000
+#define NAND_VERSION_MINOR_SHIFT 16
+
+/* NAND OP_CMDs */
+#define PAGE_READ 0x2
+#define PAGE_READ_WITH_ECC 0x3
+#define PAGE_READ_WITH_ECC_SPARE 0x4
+#define PROGRAM_PAGE 0x6
+#define PAGE_PROGRAM_WITH_ECC 0x7
+#define PROGRAM_PAGE_SPARE 0x9
+#define BLOCK_ERASE 0xa
+#define FETCH_ID 0xb
+#define RESET_DEVICE 0xd
+
+/*
+ * the NAND controller performs reads/writes with ECC in 516 byte chunks.
+ * the driver calls the chunks 'step' or 'codeword' interchangeably
+ */
+#define NANDC_STEP_SIZE 512
+
+/*
+ * the largest page size we support is 8K, this will have 16 steps/codewords
+ * of 512 bytes each
+ */
+#define MAX_NUM_STEPS (SZ_8K / NANDC_STEP_SIZE)
+
+/* we read at most 3 registers per codeword scan */
+#define MAX_REG_RD (3 * MAX_NUM_STEPS)
+
+/* ECC modes */
+#define ECC_NONE BIT(0)
+#define ECC_RS_4BIT BIT(1)
+#define ECC_BCH_4BIT BIT(2)
+#define ECC_BCH_8BIT BIT(3)
+
+struct desc_info {
+ struct list_head list;
+
+ enum dma_transfer_direction dir;
+ struct scatterlist sgl;
+ struct dma_async_tx_descriptor *dma_desc;
+};
+
+/*
+ * holds the current register values that we want to write. acts as a contiguous
+ * chunk of memory which we use to write the controller registers through DMA.
+ */
+struct nandc_regs {
+ u32 cmd;
+ u32 addr0;
+ u32 addr1;
+ u32 chip_sel;
+ u32 exec;
+
+ u32 cfg0;
+ u32 cfg1;
+ u32 ecc_bch_cfg;
+
+ u32 clrflashstatus;
+ u32 clrreadstatus;
+
+ u32 cmd1;
+ u32 vld;
+
+ u32 orig_cmd1;
+ u32 orig_vld;
+
+ u32 ecc_buf_cfg;
+};
+
+/*
+ * @cmd_crci: ADM DMA CRCI for command flow control
+ * @data_crci: ADM DMA CRCI for data flow control
+ * @list: DMA descriptor list (list of desc_infos)
+ * @dma_done: completion param to denote end of last
+ * descriptor in the list
+ * @data_buffer: our local DMA buffer for page read/writes,
+ * used when we can't use the buffer provided
+ * by upper layers directly
+ * @buf_size/count/start: markers for chip->read_buf/write_buf functions
+ * @reg_read_buf: buffer for reading register data via DMA
+ * @reg_read_pos: marker for data read in reg_read_buf
+ * @cfg0, cfg1, cfg0_raw..: NANDc register configurations needed for
+ * ecc/non-ecc mode for the current nand flash
+ * device
+ * @regs: a contiguous chunk of memory for DMA register
+ * writes
+ * @ecc_strength: 4 bit or 8 bit ecc, received via DT
+ * @bus_width: 8 bit or 16 bit NAND bus width, received via DT
+ * @ecc_modes: supported ECC modes by the current controller,
+ * initialized via DT match data
+ * @cw_size: the number of bytes in a single step/codeword
+ * of a page, consisting of all data, ecc, spare
+ * and reserved bytes
+ * @cw_data: the number of bytes within a codeword protected
+ * by ECC
+ * @bch_enabled: flag to tell whether BCH or RS ECC mode is used
+ * @status: value to be returned if NAND_CMD_STATUS command
+ * is executed
+ */
+struct qcom_nandc_data {
+ struct platform_device *pdev;
+ struct device *dev;
+
+ void __iomem *base;
+ struct resource *res;
+
+ struct clk *core_clk;
+ struct clk *aon_clk;
+
+ /* DMA stuff */
+ struct dma_chan *chan;
+ struct dma_slave_config slave_conf;
+ unsigned int cmd_crci;
+ unsigned int data_crci;
+ struct list_head list;
+ struct completion dma_done;
+
+ /* MTD stuff */
+ struct nand_chip chip;
+ struct mtd_info mtd;
+
+ /* local data buffer and markers */
+ u8 *data_buffer;
+ int buf_size;
+ int buf_count;
+ int buf_start;
+
+ /* local buffer to read back registers */
+ u32 *reg_read_buf;
+ dma_addr_t reg_read_paddr;
+ int reg_read_pos;
+
+ /* required configs */
+ u32 cfg0, cfg1;
+ u32 cfg0_raw, cfg1_raw;
+ u32 ecc_buf_cfg;
+ u32 ecc_bch_cfg;
+ u32 clrflashstatus;
+ u32 clrreadstatus;
+ u32 sflashc_burst_cfg;
+ u32 cmd1, vld;
+
+ /* register state */
+ struct nandc_regs *regs;
+
+ /* things we get from DT */
+ int ecc_strength;
+ int bus_width;
+
+ u32 ecc_modes;
+
+ /* misc params */
+ int cw_size;
+ int cw_data;
+ bool use_ecc;
+ bool bch_enabled;
+ u8 status;
+ int last_command;
+};
+
+static inline unsigned int nandc_read(struct qcom_nandc_data *this, int offset)
+{
+ return ioread32(this->base + offset);
+}
+
+static inline void nandc_write(struct qcom_nandc_data *this, int offset,
+ unsigned int val)
+{
+ iowrite32(val, this->base + offset);
+}
+
+/* helper to configure address register values */
+static void set_address(struct qcom_nandc_data *this, u16 column, int page)
+{
+ struct nand_chip *chip = &this->chip;
+ struct nandc_regs *regs = this->regs;
+
+ if (chip->options & NAND_BUSWIDTH_16)
+ column >>= 1;
+
+ regs->addr0 = page << 16 | column;
+ regs->addr1 = page >> 16 & 0xff;
+}
+
+/*
+ * update_rw_regs: set up read/write register values, these will be
+ * written to the NAND controller registers via DMA
+ *
+ * @num_cw: number of steps for the read/write operation
+ * @read: read or write operation
+ */
+static void update_rw_regs(struct qcom_nandc_data *this, int num_cw, bool read)
+{
+ struct nandc_regs *regs = this->regs;
+
+ if (this->use_ecc) {
+ if (read)
+ regs->cmd = PAGE_READ_WITH_ECC | PAGE_ACC | LAST_PAGE;
+ else
+ regs->cmd = PROGRAM_PAGE | PAGE_ACC | LAST_PAGE;
+
+ regs->cfg0 = (this->cfg0 & ~(7U << CW_PER_PAGE)) |
+ (num_cw - 1) << CW_PER_PAGE;
+
+ regs->cfg1 = this->cfg1;
+ regs->ecc_bch_cfg = this->ecc_bch_cfg;
+ } else {
+ if (read)
+ regs->cmd = PAGE_READ | PAGE_ACC | LAST_PAGE;
+ else
+ regs->cmd = PROGRAM_PAGE | PAGE_ACC | LAST_PAGE;
+
+ regs->cfg0 = (this->cfg0_raw & ~(7U << CW_PER_PAGE)) |
+ (num_cw - 1) << CW_PER_PAGE;
+
+ regs->cfg1 = this->cfg1_raw;
+ regs->ecc_bch_cfg = 1 << ECC_CFG_ECC_DISABLE;
+ }
+
+ regs->ecc_buf_cfg = this->ecc_buf_cfg;
+ regs->clrflashstatus = this->clrflashstatus;
+ regs->clrreadstatus = this->clrreadstatus;
+ regs->exec = 1;
+}
+
+/*
+ * write_reg_dma: prepares a descriptor to write a given number of
+ * contiguous registers
+ *
+ * @first: offset of the first register in the contiguous block
+ * @reg: starting address containing the reg values to write
+ * @num_regs: number of registers to write
+ * @flow_control: flow control enabled/disabled
+ */
+static int write_reg_dma(struct qcom_nandc_data *this, int first, u32 *reg,
+ int num_regs, bool flow_control)
+{
+ struct desc_info *desc;
+ struct dma_async_tx_descriptor *dma_desc;
+ struct scatterlist *sgl;
+ int size;
+ int r;
+
+ desc = kzalloc(sizeof(*desc), GFP_KERNEL);
+ if (!desc)
+ return -ENOMEM;
+
+ list_add_tail(&desc->list, &this->list);
+
+ sgl = &desc->sgl;
+
+ size = num_regs * sizeof(u32);
+
+ sg_init_one(sgl, reg, size);
+
+ desc->dir = DMA_MEM_TO_DEV;
+
+ dma_map_sg(this->dev, sgl, 1, desc->dir);
+
+ this->slave_conf.device_fc = flow_control ? 1 : 0;
+ this->slave_conf.dst_addr = this->res->start + first;
+ this->slave_conf.dst_maxburst = 16;
+ this->slave_conf.slave_id = this->cmd_crci;
+
+ r = dmaengine_slave_config(this->chan, &this->slave_conf);
+ if (r) {
+ dev_err(this->dev, "failed to configure dma channel\n");
+ goto err;
+ }
+
+ dma_desc = dmaengine_prep_slave_sg(this->chan, sgl, 1, desc->dir, 0);
+ if (!dma_desc) {
+ dev_err(this->dev, "failed to prepare register write desc\n");
+ r = PTR_ERR(dma_desc);
+ goto err;
+ }
+
+ desc->dma_desc = dma_desc;
+
+ return 0;
+err:
+ kfree(desc);
+
+ return r;
+}
+
+/*
+ * read_reg_dma: prepares a descriptor to read a given number of
+ * contiguous registers to the reg_read_buf pointer
+ *
+ * @first: offset of the first register in the contiguous block
+ * @num_regs: number of registers to read
+ * @flow_control: flow control enabled/disabled
+ */
+static int read_reg_dma(struct qcom_nandc_data *this, int first, int num_regs,
+ bool flow_control)
+{
+ struct desc_info *desc;
+ struct dma_async_tx_descriptor *dma_desc;
+ struct scatterlist *sgl;
+ int size;
+ int r;
+
+ desc = kzalloc(sizeof(*desc), GFP_KERNEL);
+ if (!desc)
+ return -ENOMEM;
+
+ list_add_tail(&desc->list, &this->list);
+
+ sgl = &desc->sgl;
+
+ size = num_regs * sizeof(u32);
+
+ sg_init_one(sgl, this->reg_read_buf + this->reg_read_pos, size);
+
+ desc->dir = DMA_DEV_TO_MEM;
+
+ dma_map_sg(this->dev, sgl, 1, desc->dir);
+
+ this->slave_conf.device_fc = flow_control ? 1 : 0;
+ this->slave_conf.src_addr = this->res->start + first;
+ this->slave_conf.src_maxburst = 16;
+ this->slave_conf.slave_id = this->data_crci;
+
+ r = dmaengine_slave_config(this->chan, &this->slave_conf);
+ if (r) {
+ dev_err(this->dev, "failed to configure dma channel\n");
+ goto err;
+ }
+
+ dma_desc = dmaengine_prep_slave_sg(this->chan, sgl, 1, desc->dir, 0);
+ if (!dma_desc) {
+ dev_err(this->dev, "failed to prepare register read desc\n");
+ r = PTR_ERR(dma_desc);
+ goto err;
+ }
+
+ desc->dma_desc = dma_desc;
+
+ this->reg_read_pos += num_regs;
+
+ return 0;
+err:
+ kfree(desc);
+
+ return r;
+}
+
+/*
+ * read_data_dma: prepares a DMA descriptor to transfer data from the
+ * controller's internal buffer to the buffer 'vaddr'
+ *
+ * @reg_off: offset within the controller's data buffer
+ * @vaddr: virtual address of the buffer we want to write to
+ * @size: DMA transaction size in bytes
+ */
+static int read_data_dma(struct qcom_nandc_data *this, int reg_off, u8 *vaddr,
+ int size)
+{
+ struct desc_info *desc;
+ struct dma_async_tx_descriptor *dma_desc;
+ struct scatterlist *sgl;
+ int r;
+
+ desc = kzalloc(sizeof(*desc), GFP_KERNEL);
+ if (!desc)
+ return -ENOMEM;
+
+ list_add_tail(&desc->list, &this->list);
+
+ sgl = &desc->sgl;
+
+ sg_init_one(sgl, vaddr, size);
+
+ desc->dir = DMA_DEV_TO_MEM;
+
+ r = dma_map_sg(this->dev, sgl, 1, desc->dir);
+ if (r == 0)
+ goto err;
+
+ this->slave_conf.device_fc = 0;
+ this->slave_conf.src_addr = this->res->start + reg_off;
+ this->slave_conf.src_maxburst = 16;
+
+ r = dmaengine_slave_config(this->chan, &this->slave_conf);
+ if (r) {
+ dev_err(this->dev, "failed to configure dma channel\n");
+ goto err;
+ }
+
+ dma_desc = dmaengine_prep_slave_sg(this->chan, sgl, 1, desc->dir, 0);
+ if (!dma_desc) {
+ dev_err(this->dev, "failed to prepare data read desc\n");
+ r = PTR_ERR(dma_desc);
+ goto err;
+ }
+
+ desc->dma_desc = dma_desc;
+
+ return 0;
+err:
+ kfree(desc);
+
+ return r;
+}
+
+/*
+ * write_data_dma: prepares a DMA descriptor to transfer data from
+ * 'vaddr' to the controller's internal buffer
+ *
+ * @reg_off: offset within the controller's data buffer
+ * @vaddr: virtual address of the buffer we want to read from
+ * @size: DMA transaction size in bytes
+ */
+static int write_data_dma(struct qcom_nandc_data *this, int reg_off, u8 *vaddr,
+ int size)
+{
+ struct desc_info *desc;
+ struct dma_async_tx_descriptor *dma_desc;
+ struct scatterlist *sgl;
+ int r;
+
+ desc = kzalloc(sizeof(*desc), GFP_KERNEL);
+ if (!desc)
+ return -ENOMEM;
+
+ list_add_tail(&desc->list, &this->list);
+
+ sgl = &desc->sgl;
+
+ sg_init_one(sgl, vaddr, size);
+
+ desc->dir = DMA_MEM_TO_DEV;
+
+ r = dma_map_sg(this->dev, sgl, 1, desc->dir);
+ if (r == 0)
+ goto err;
+
+ this->slave_conf.device_fc = 0;
+ this->slave_conf.dst_addr = this->res->start + reg_off;
+ this->slave_conf.dst_maxburst = 16;
+
+ r = dmaengine_slave_config(this->chan, &this->slave_conf);
+ if (r) {
+ dev_err(this->dev, "failed to configure dma channel\n");
+ goto err;
+ }
+
+ dma_desc = dmaengine_prep_slave_sg(this->chan, sgl, 1, desc->dir, 0);
+ if (!dma_desc) {
+ dev_err(this->dev, "failed to prepare data write desc\n");
+ r = PTR_ERR(dma_desc);
+ goto err;
+ }
+
+ desc->dma_desc = dma_desc;
+
+ return 0;
+err:
+ kfree(desc);
+
+ return r;
+}
+
+/*
+ * helper to prepare dma descriptors to configure registers needed for reading a
+ * codeword/step in a page
+ */
+static void config_cw_read(struct qcom_nandc_data *this)
+{
+ struct nandc_regs *regs = this->regs;
+
+ write_reg_dma(this, NAND_FLASH_CMD, &regs->cmd, 3, true);
+ write_reg_dma(this, NAND_DEV0_CFG0, &regs->cfg0, 3, false);
+ write_reg_dma(this, NAND_EBI2_ECC_BUF_CFG, &regs->ecc_buf_cfg,
+ 1, false);
+
+ write_reg_dma(this, NAND_EXEC_CMD, &regs->exec, 1, false);
+
+ read_reg_dma(this, NAND_FLASH_STATUS, 2, true);
+ read_reg_dma(this, NAND_ERASED_CW_DETECT_STATUS, 1, false);
+}
+
+/*
+ * helpers to prepare dma descriptors used to configure registers needed for
+ * writing a codeword/step in a page
+ */
+static void config_cw_write_pre(struct qcom_nandc_data *this)
+{
+ struct nandc_regs *regs = this->regs;
+
+ write_reg_dma(this, NAND_FLASH_CMD, &regs->cmd, 3, true);
+ write_reg_dma(this, NAND_DEV0_CFG0, &regs->cfg0, 3, false);
+ write_reg_dma(this, NAND_EBI2_ECC_BUF_CFG, &regs->ecc_buf_cfg,
+ 1, false);
+}
+
+static void config_cw_write_post(struct qcom_nandc_data *this)
+{
+ struct nandc_regs *regs = this->regs;
+
+ write_reg_dma(this, NAND_EXEC_CMD, &regs->exec, 1, false);
+
+ read_reg_dma(this, NAND_FLASH_STATUS, 1, true);
+
+ write_reg_dma(this, NAND_FLASH_STATUS, &regs->clrflashstatus, 1, false);
+ write_reg_dma(this, NAND_READ_STATUS, &regs->clrreadstatus, 1, false);
+}
+
+/*
+ * the following functions are used within chip->cmdfunc() to perform different
+ * NAND_CMD_* commands
+ */
+
+/* sets up descriptors for NAND_CMD_PARAM */
+static int nandc_param(struct qcom_nandc_data *this)
+{
+ struct nandc_regs *regs = this->regs;
+
+ /*
+ * NAND_CMD_PARAM is called before we know much about the FLASH chip
+ * in use. we configure the controller to perform a raw read of 512
+ * bytes to read onfi params
+ */
+ regs->cmd = PAGE_READ | PAGE_ACC | LAST_PAGE;
+ regs->addr0 = 0;
+ regs->addr1 = 0;
+ regs->cfg0 = 0 << CW_PER_PAGE
+ | 512 << UD_SIZE_BYTES
+ | 5 << NUM_ADDR_CYCLES
+ | 0 << SPARE_SIZE_BYTES;
+
+ regs->cfg1 = 7 << NAND_RECOVERY_CYCLES
+ | 0 << CS_ACTIVE_BSY
+ | 17 << BAD_BLOCK_BYTE_NUM
+ | 1 << BAD_BLOCK_IN_SPARE_AREA
+ | 2 << WR_RD_BSY_GAP
+ | 0 << WIDE_FLASH
+ | 1 << DEV0_CFG1_ECC_DISABLE;
+
+ regs->ecc_bch_cfg = 1 << ECC_CFG_ECC_DISABLE;
+
+ /* configure CMD1 and VLD for ONFI param probing */
+ regs->vld = (this->vld & ~(1 << READ_START_VLD))
+ | 0 << READ_START_VLD;
+
+ regs->cmd1 = (this->cmd1 & ~(0xFF << READ_ADDR))
+ | NAND_CMD_PARAM << READ_ADDR;
+
+ regs->exec = 1;
+
+ regs->orig_cmd1 = this->cmd1;
+ regs->orig_vld = this->vld;
+
+ write_reg_dma(this, NAND_DEV_CMD_VLD, &regs->vld, 1, false);
+ write_reg_dma(this, NAND_DEV_CMD1, &regs->cmd1, 1, false);
+
+ this->buf_count = 512;
+ memset(this->data_buffer, 0xff, this->buf_count);
+
+ config_cw_read(this);
+
+ read_data_dma(this, FLASH_BUF_ACC, this->data_buffer, this->buf_count);
+
+ /* restore CMD1 and VLD regs */
+ write_reg_dma(this, NAND_DEV_CMD1, &regs->orig_cmd1, 1, false);
+ write_reg_dma(this, NAND_DEV_CMD_VLD, &regs->orig_vld, 1, false);
+
+ return 0;
+}
+
+/* sets up descriptors for NAND_CMD_ERASE1 */
+static int erase_block(struct qcom_nandc_data *this, int page_addr)
+{
+ struct nandc_regs *regs = this->regs;
+
+ regs->cmd = BLOCK_ERASE | PAGE_ACC | LAST_PAGE;
+ regs->addr0 = page_addr;
+ regs->addr1 = 0;
+ regs->cfg0 = this->cfg0_raw & ~(7 << CW_PER_PAGE);
+ regs->cfg1 = this->cfg1_raw;
+ regs->exec = 1;
+ regs->clrflashstatus = this->clrflashstatus;
+ regs->clrreadstatus = this->clrreadstatus;
+
+ write_reg_dma(this, NAND_FLASH_CMD, &regs->cmd, 3, true);
+ write_reg_dma(this, NAND_DEV0_CFG0, &regs->cfg0, 2, false);
+ write_reg_dma(this, NAND_EXEC_CMD, &regs->exec, 1, false);
+
+ read_reg_dma(this, NAND_FLASH_STATUS, 1, true);
+
+ write_reg_dma(this, NAND_FLASH_STATUS, &regs->clrflashstatus, 1, false);
+ write_reg_dma(this, NAND_READ_STATUS, &regs->clrreadstatus, 1, false);
+
+ return 0;
+}
+
+/* sets up descriptors for NAND_CMD_READID */
+static int read_id(struct qcom_nandc_data *this, int column)
+{
+ struct nandc_regs *regs = this->regs;
+
+ if (column == -1)
+ return 0;
+
+ regs->cmd = FETCH_ID;
+ regs->addr0 = column;
+ regs->addr1 = 0;
+ regs->chip_sel = DM_EN;
+ regs->exec = 1;
+
+ write_reg_dma(this, NAND_FLASH_CMD, &regs->cmd, 4, true);
+ write_reg_dma(this, NAND_EXEC_CMD, &regs->exec, 1, false);
+
+ read_reg_dma(this, NAND_READ_ID, 1, true);
+
+ return 0;
+}
+
+/* sets up descriptors for NAND_CMD_RESET */
+static int reset(struct qcom_nandc_data *this)
+{
+ struct nandc_regs *regs = this->regs;
+
+ regs->cmd = RESET_DEVICE;
+ regs->exec = 1;
+
+ write_reg_dma(this, NAND_FLASH_CMD, &regs->cmd, 1, true);
+ write_reg_dma(this, NAND_EXEC_CMD, &regs->exec, 1, false);
+
+ read_reg_dma(this, NAND_FLASH_STATUS, 1, true);
+
+ return 0;
+}
+
+/* helpers to submit/free our list of dma descriptors */
+static void dma_callback(void *param)
+{
+ struct qcom_nandc_data *this = (struct qcom_nandc_data *) param;
+ struct completion *c = &this->dma_done;
+
+ complete(c);
+}
+
+static int submit_descs(struct qcom_nandc_data *this)
+{
+ struct completion *c = &this->dma_done;
+ struct desc_info *desc;
+ int r;
+
+ init_completion(c);
+
+ list_for_each_entry(desc, &this->list, list) {
+ /*
+ * we add a callback the last descriptor in our list to notify
+ * completion of command
+ */
+ if (list_is_last(&desc->list, &this->list)) {
+ desc->dma_desc->callback = dma_callback;
+ desc->dma_desc->callback_param = this;
+ }
+
+ dmaengine_submit(desc->dma_desc);
+ }
+
+ dma_async_issue_pending(this->chan);
+
+ r = wait_for_completion_timeout(c, msecs_to_jiffies(500));
+ if (!r)
+ return -EINVAL;
+
+ return 0;
+}
+
+static void free_descs(struct qcom_nandc_data *this)
+{
+ struct desc_info *desc, *n;
+
+ list_for_each_entry_safe(desc, n, &this->list, list) {
+ list_del(&desc->list);
+ dma_unmap_sg(this->dev, &desc->sgl, 1, desc->dir);
+ kfree(desc);
+ }
+}
+
+/* reset the register read buffer for next NAND operation */
+static void clear_read_regs(struct qcom_nandc_data *this)
+{
+ this->reg_read_pos = 0;
+ memset(this->reg_read_buf, 0, MAX_REG_RD * sizeof(u32));
+}
+
+static void pre_command(struct qcom_nandc_data *this, int command)
+{
+ this->buf_count = 0;
+ this->buf_start = 0;
+ this->use_ecc = false;
+ this->last_command = command;
+
+ clear_read_regs(this);
+}
+
+/*
+ * this is called after NAND_CMD_PAGEPROG and NAND_CMD_ERASE1 to set our
+ * privately maintained status byte, this status byte can be read after
+ * NAND_CMD_STATUS is called
+ */
+static void parse_erase_write_errors(struct qcom_nandc_data *this, int command)
+{
+ struct nand_chip *chip = &this->chip;
+ struct nand_ecc_ctrl *ecc = &chip->ecc;
+ int num_cw;
+ int i;
+
+ num_cw = command == NAND_CMD_PAGEPROG ? ecc->steps : 1;
+
+ for (i = 0; i < num_cw; i++) {
+ u32 flash_status;
+
+ flash_status = this->reg_read_buf[i];
+
+ if (flash_status & FS_MPU_ERR)
+ this->status &= ~NAND_STATUS_WP;
+
+ if (flash_status & FS_OP_ERR || (i == (num_cw - 1) &&
+ (flash_status & FS_DEVICE_STS_ERR)))
+ this->status |= NAND_STATUS_FAIL;
+ }
+}
+
+static void post_command(struct qcom_nandc_data *this, int command)
+{
+ switch (command) {
+ case NAND_CMD_READID:
+ memcpy(this->data_buffer, this->reg_read_buf, this->buf_count);
+ break;
+ case NAND_CMD_PAGEPROG:
+ case NAND_CMD_ERASE1:
+ parse_erase_write_errors(this, command);
+ break;
+ default:
+ break;
+ }
+}
+
+/*
+ * Implements chip->cmdfunc. It's only used for a limited set of commands.
+ * The rest of the commands wouldn't be called by upper layers. For example,
+ * NAND_CMD_READOOB would never be called because we have our own versions
+ * of read_oob ops for nand_ecc_ctrl.
+ */
+static void qcom_nandc_command(struct mtd_info *mtd, unsigned int command,
+ int column, int page_addr)
+{
+ struct nand_chip *chip = mtd->priv;
+ struct nand_ecc_ctrl *ecc = &chip->ecc;
+ struct qcom_nandc_data *this = chip->priv;
+ bool wait = false;
+ int r = 0;
+
+ pre_command(this, command);
+
+ switch (command) {
+ case NAND_CMD_RESET:
+ r = reset(this);
+ wait = true;
+ break;
+
+ case NAND_CMD_READID:
+ this->buf_count = 4;
+ r = read_id(this, column);
+ wait = true;
+ break;
+
+ case NAND_CMD_PARAM:
+ r = nandc_param(this);
+ wait = true;
+ break;
+
+ case NAND_CMD_ERASE1:
+ r = erase_block(this, page_addr);
+ wait = true;
+ break;
+
+ case NAND_CMD_READ0:
+ /* we read the entire page for now */
+ WARN_ON(column != 0);
+
+ this->use_ecc = true;
+ set_address(this, 0, page_addr);
+ update_rw_regs(this, ecc->steps, true);
+ break;
+
+ case NAND_CMD_SEQIN:
+ WARN_ON(column != 0);
+ set_address(this, 0, page_addr);
+ break;
+
+ case NAND_CMD_PAGEPROG:
+ case NAND_CMD_STATUS:
+ case NAND_CMD_NONE:
+ default:
+ break;
+ }
+
+ if (r) {
+ dev_err(this->dev, "failure executing command %d\n",
+ command);
+ free_descs(this);
+ return;
+ }
+
+ if (wait) {
+ r = submit_descs(this);
+ if (r)
+ dev_err(this->dev,
+ "failure submitting descs for command %d\n",
+ command);
+ }
+
+ free_descs(this);
+
+ post_command(this, command);
+}
+
+/*
+ * when using RS ECC, the NAND controller flags an error when reading an
+ * erased page. however, there are special characters at certain offsets when
+ * we read the erased page. we check here if the page is really empty. if so,
+ * we replace the magic characters with 0xffs
+ */
+static bool empty_page_fixup(struct qcom_nandc_data *this, u8 *data_buf)
+{
+ struct mtd_info *mtd = &this->mtd;
+ struct nand_chip *chip = &this->chip;
+ struct nand_ecc_ctrl *ecc = &chip->ecc;
+ int cwperpage = ecc->steps;
+ int i;
+
+ /* if BCH is enabled, HW will take care of detecting erased pages */
+ if (this->bch_enabled || !this->use_ecc)
+ return false;
+
+ for (i = 0; i < cwperpage; i++) {
+ u8 *empty1, *empty2;
+ u32 flash_status = this->reg_read_buf[3 * i];
+
+ /*
+ * an erased page flags an error in NAND_FLASH_STATUS, check if
+ * the page is erased by looking for 0x54s at offsets 3 and 175
+ * from the beginning of each codeword
+ */
+ if (flash_status & FS_OP_ERR) {
+ empty1 = &data_buf[3 + i * this->cw_data];
+ empty2 = &data_buf[175 + i * this->cw_data];
+
+ /*
+ * the error wasn't because of an erased page, bail out
+ * and let someone else do the error checking
+ */
+ if (!((*empty1 == 0x54 && *empty2 == 0xff) ||
+ (*empty1 == 0xff && *empty2 == 0x54)))
+ return false;
+ }
+ }
+
+ for (i = 0; i < mtd->writesize && (data_buf[i] == 0xff ||
+ (i % this->cw_data == 3 || i % this->cw_data == 175)); i++) {
+ }
+
+ if (i < mtd->writesize)
+ return false;
+
+ /*
+ * the whole page is 0xffs besides the magic offsets, we replace the
+ * 0x54s with 0xffs
+ */
+ for (i = 0; i < cwperpage; i++) {
+ data_buf[3 + i * this->cw_data] = 0xff;
+ data_buf[175 + i * this->cw_data] = 0xff;
+ }
+
+ /*
+ * tell the caller that the page was empty and is fixed up, so that
+ * parse_read_errors() doesn't think it's and error
+ */
+ return true;
+}
+
+/*
+ * reads back status registers set by the controller to notify page read
+ * errors. this is equivalent to what 'ecc->correct()' would do.
+ */
+static int parse_read_errors(struct qcom_nandc_data *this, bool erased_page)
+{
+ struct mtd_info *mtd = &this->mtd;
+ struct nand_chip *chip = &this->chip;
+ struct nand_ecc_ctrl *ecc = &chip->ecc;
+ int cwperpage = ecc->steps;
+ unsigned int max_bitflips = 0;
+ int i;
+
+ for (i = 0; i < cwperpage; i++) {
+ int stat;
+ u32 flash_status = this->reg_read_buf[3 * i];
+ u32 buffer_status = this->reg_read_buf[3 * i + 1];
+ u32 erased_cw_status = this->reg_read_buf[3 * i + 2];
+
+ if (flash_status & (FS_OP_ERR | FS_MPU_ERR)) {
+
+ /* ignore erased codeword errors */
+ if (this->bch_enabled) {
+ if ((erased_cw_status & ERASED_CW) == ERASED_CW)
+ continue;
+ } else if (erased_page) {
+ continue;
+ }
+
+ if (buffer_status & BS_UNCORRECTABLE_BIT) {
+ mtd->ecc_stats.failed++;
+ continue;
+ }
+ }
+
+ stat = buffer_status & BS_CORRECTABLE_ERR_MSK;
+ mtd->ecc_stats.corrected += stat;
+
+ max_bitflips = max_t(unsigned int, max_bitflips, stat);
+ }
+
+ return max_bitflips;
+}
+
+/*
+ * helper to perform the actual page read operation, used by ecc->read_page()
+ * and ecc->read_oob()
+ */
+static int read_page_low(struct qcom_nandc_data *this, u8 *data_buf,
+ u8 *oob_buf)
+{
+ struct nand_chip *chip = &this->chip;
+ struct nand_ecc_ctrl *ecc = &chip->ecc;
+ int i, r;
+
+ /* queue cmd descs for each codeword */
+ for (i = 0; i < ecc->steps; i++) {
+ int data_size, oob_size;
+
+ if (i == (ecc->steps - 1)) {
+ data_size = ecc->size - ((ecc->steps - 1) << 2);
+ oob_size = (ecc->steps << 2) + ecc->bytes;
+ } else {
+ data_size = this->cw_data;
+ oob_size = ecc->bytes;
+ }
+
+ config_cw_read(this);
+
+ if (data_buf)
+ read_data_dma(this, FLASH_BUF_ACC, data_buf, data_size);
+
+ if (oob_buf)
+ read_data_dma(this, FLASH_BUF_ACC + data_size, oob_buf,
+ oob_size);
+
+ if (data_buf)
+ data_buf += data_size;
+ if (oob_buf)
+ oob_buf += oob_size;
+ }
+
+ r = submit_descs(this);
+ if (r)
+ dev_err(this->dev, "failure to read page/oob\n");
+
+ free_descs(this);
+
+ return r;
+}
+
+/*
+ * a helper that copies the last step/codeword of a page (containing free oob)
+ * into our local buffer
+ */
+static int copy_last_cw(struct qcom_nandc_data *this, bool use_ecc, int page)
+{
+ struct nand_chip *chip = &this->chip;
+ struct nand_ecc_ctrl *ecc = &chip->ecc;
+ int size;
+ int r;
+
+ clear_read_regs(this);
+
+ size = use_ecc ? this->cw_data : this->cw_size;
+
+ /* prepare a clean read buffer */
+ memset(this->data_buffer, 0xff, size);
+
+ this->use_ecc = use_ecc;
+ set_address(this, this->cw_size * (ecc->steps - 1), page);
+ update_rw_regs(this, 1, true);
+
+ config_cw_read(this);
+
+ read_data_dma(this, FLASH_BUF_ACC, this->data_buffer, size);
+
+ r = submit_descs(this);
+ if (r)
+ dev_err(this->dev, "failed to copy last codeword\n");
+
+ free_descs(this);
+
+ return r;
+}
+
+/* implements ecc->read_page() */
+static int qcom_nandc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
+ uint8_t *buf, int oob_required, int page)
+{
+ struct qcom_nandc_data *this = chip->priv;
+ u8 *data_buf, *oob_buf = NULL;
+ bool direct_dma, erased_page;
+ int r;
+
+ /*
+ * first try to map the upper buffer directly, else, use our own buf
+ * and memcpy to upper buf
+ */
+ if (virt_addr_valid(buf) && !object_is_on_stack(buf)) {
+ direct_dma = true;
+ data_buf = buf;
+ } else {
+ direct_dma = false;
+ data_buf = this->data_buffer;
+ memset(data_buf, 0xff, mtd->writesize + mtd->oobsize);
+ }
+
+ oob_buf = oob_required ? chip->oob_poi : NULL;
+
+ r = read_page_low(this, data_buf, oob_buf);
+ if (r) {
+ dev_err(this->dev, "failure to read page\n");
+ goto err;
+ }
+
+ erased_page = empty_page_fixup(this, data_buf);
+
+ if (!direct_dma)
+ memcpy(buf, this->data_buffer, mtd->writesize);
+
+ r = parse_read_errors(this, erased_page);
+err:
+ return r;
+}
+
+/* implements ecc->read_oob() */
+static int qcom_nandc_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
+ int page)
+{
+ struct qcom_nandc_data *this = chip->priv;
+ struct nand_ecc_ctrl *ecc = &chip->ecc;
+ int r;
+
+ clear_read_regs(this);
+
+ this->use_ecc = true;
+ set_address(this, 0, page);
+ update_rw_regs(this, ecc->steps, true);
+
+ r = read_page_low(this, NULL, chip->oob_poi);
+ if (r)
+ dev_err(this->dev, "failure to read oob\n");
+
+ return r;
+}
+
+/* implements ecc->read_oob_raw(), used to read the bad block marker flag */
+static int qcom_nandc_read_oob_raw(struct mtd_info *mtd, struct nand_chip *chip,
+ int page)
+{
+ struct qcom_nandc_data *this = chip->priv;
+ struct nand_ecc_ctrl *ecc = &chip->ecc;
+ uint8_t *oob = chip->oob_poi;
+ int start, length;
+ int r;
+
+ /*
+ * configure registers for a raw page read, the address is set to the
+ * beginning of the last codeword, we don't care about reading ecc
+ * portion of oob, just the free stuff
+ */
+ r = copy_last_cw(this, false, page);
+ if (r)
+ return r;
+
+ /*
+ * reading raw oob has 2 parts, first the bad block byte, then the
+ * actual free oob region. perform a memcpy in two steps
+ */
+ start = mtd->writesize - (this->cw_size * (ecc->steps - 1));
+ length = chip->options & NAND_BUSWIDTH_16 ? 2 : 1;
+
+ memcpy(oob, this->data_buffer + start, length);
+
+ oob += length;
+
+ start = this->cw_data - (ecc->steps << 2) + 1;
+ length = ecc->steps << 2;
+
+ memcpy(oob, this->data_buffer + start, length);
+
+ return 0;
+}
+
+/* implements ecc->write_page() */
+static int qcom_nandc_write_page(struct mtd_info *mtd, struct nand_chip *chip,
+ const uint8_t *buf, int oob_required)
+{
+ struct qcom_nandc_data *this = chip->priv;
+ struct nand_ecc_ctrl *ecc = &chip->ecc;
+ u8 *data_buf, *oob_buf;
+ bool direct_dma;
+ int i, r = 0;
+
+ clear_read_regs(this);
+
+ /*
+ * first try to map the upper buffer directly, else, memcpy upper
+ * buffer to our local buffer
+ */
+ direct_dma = virt_addr_valid(buf) && !object_is_on_stack((void *) buf);
+
+ if (!direct_dma) {
+ memcpy(this->data_buffer, buf, mtd->writesize);
+ data_buf = this->data_buffer;
+ } else {
+ data_buf = (u8 *) buf;
+ }
+
+ oob_buf = chip->oob_poi;
+
+ this->use_ecc = true;
+ update_rw_regs(this, ecc->steps, false);
+
+ for (i = 0; i < ecc->steps; i++) {
+ int data_size, oob_size;
+
+ if (i == (ecc->steps - 1)) {
+ data_size = ecc->size - ((ecc->steps - 1) << 2);
+ oob_size = (ecc->steps << 2) + ecc->bytes;
+ } else {
+ data_size = this->cw_data;
+ oob_size = ecc->bytes;
+ }
+
+ config_cw_write_pre(this);
+ write_data_dma(this, FLASH_BUF_ACC, data_buf, data_size);
+
+ /*
+ * we don't really need to write anything to oob for the
+ * first n - 1 codewords since these oob regions just
+ * contain ecc that's written by the controller itself
+ */
+ if (i == (ecc->steps - 1))
+ write_data_dma(this, FLASH_BUF_ACC + data_size,
+ oob_buf, oob_size);
+ config_cw_write_post(this);
+
+ data_buf += data_size;
+ oob_buf += oob_size;
+ }
+
+ r = submit_descs(this);
+ if (r)
+ dev_err(this->dev, "failure to write page\n");
+
+ free_descs(this);
+
+ return r;
+}
+
+/*
+ * implements ecc->write_oob()
+ *
+ * the NAND controller cannot write only data or only oob within a codeword,
+ * since ecc is calculated for the combined codeword. we first copy the
+ * entire contents for the last codeword(data + oob), replace the old oob
+ * with the new one in chip->oob_poi, and then write the entire codeword.
+ * this read-copy-write operation results in a slight perormance loss.
+ */
+static int qcom_nandc_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
+ int page)
+{
+ struct qcom_nandc_data *this = chip->priv;
+ struct nand_ecc_ctrl *ecc = &chip->ecc;
+ uint8_t *oob = chip->oob_poi;
+ int free_boff;
+ int data_size, oob_size;
+ int r, status = 0;
+
+ r = copy_last_cw(this, true, page);
+ if (r)
+ return r;
+
+ clear_read_regs(this);
+
+ /* calculate the data and oob size for the last codeword/step */
+ data_size = ecc->size - ((ecc->steps - 1) << 2);
+ oob_size = (ecc->steps << 2) + ecc->bytes;
+
+ /*
+ * the location of spare data in the oob buffer, we could also use
+ * ecc->layout.oobfree here
+ */
+ free_boff = ecc->bytes * (ecc->steps - 1);
+
+ /* override new oob content to last codeword */
+ memcpy(this->data_buffer + data_size, oob + free_boff, oob_size);
+
+ this->use_ecc = true;
+ set_address(this, this->cw_size * (ecc->steps - 1), page);
+ update_rw_regs(this, 1, false);
+
+ config_cw_write_pre(this);
+ write_data_dma(this, FLASH_BUF_ACC, this->data_buffer,
+ data_size + oob_size);
+ config_cw_write_post(this);
+
+ r = submit_descs(this);
+ if (r)
+ dev_err(this->dev, "failure to write oob\n");
+
+ free_descs(this);
+
+ chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
+
+ status = chip->waitfunc(mtd, chip);
+
+ return status & NAND_STATUS_FAIL ? -EIO : 0;
+}
+
+/* implements ecc->write_oob_raw(), used to write bad block marker flag */
+static int qcom_nandc_write_oob_raw(struct mtd_info *mtd,
+ struct nand_chip *chip, int page)
+{
+ struct qcom_nandc_data *this = chip->priv;
+ struct nand_ecc_ctrl *ecc = &chip->ecc;
+ uint8_t *oob = chip->oob_poi;
+ int start, length;
+ int r, status = 0;
+
+ r = copy_last_cw(this, false, page);
+ if (r)
+ return r;
+
+ clear_read_regs(this);
+
+ /*
+ * writing raw oob has 2 parts, first the bad block region, then the
+ * actual free region
+ */
+ start = mtd->writesize - (this->cw_size * (ecc->steps - 1));
+ length = chip->options & NAND_BUSWIDTH_16 ? 2 : 1;
+
+ memcpy(this->data_buffer + start, oob, length);
+
+ oob += length;
+
+ start = this->cw_data - (ecc->steps << 2) + 1;
+ length = ecc->steps << 2;
+
+ memcpy(this->data_buffer + start, oob, length);
+
+ /* prepare write */
+ this->use_ecc = false;
+ set_address(this, this->cw_size * (ecc->steps - 1), page);
+ update_rw_regs(this, 1, false);
+
+ config_cw_write_pre(this);
+ write_data_dma(this, FLASH_BUF_ACC, this->data_buffer, this->cw_size);
+ config_cw_write_post(this);
+
+ r = submit_descs(this);
+ if (r)
+ dev_err(this->dev, "failure to write updated oob\n");
+
+ free_descs(this);
+
+ chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
+
+ status = chip->waitfunc(mtd, chip);
+
+ return status & NAND_STATUS_FAIL ? -EIO : 0;
+}
+
+/*
+ * the three functions below implement chip->read_byte(), chip->read_buf()
+ * and chip->write_buf() respectively. these aren't used for
+ * reading/writing page data, they are used for smaller data like reading
+ * id, status etc
+ */
+static uint8_t qcom_nandc_read_byte(struct mtd_info *mtd)
+{
+ struct nand_chip *chip = mtd->priv;
+ struct qcom_nandc_data *this = chip->priv;
+ uint8_t *buf = (uint8_t *) this->data_buffer;
+ uint8_t ret = 0x0;
+
+ if (this->last_command == NAND_CMD_STATUS) {
+ ret = this->status;
+
+ this->status = NAND_STATUS_READY | NAND_STATUS_WP;
+
+ return ret;
+ }
+
+ if (this->buf_start < this->buf_count)
+ ret = buf[this->buf_start++];
+
+ return ret;
+}
+
+static void qcom_nandc_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
+{
+ struct nand_chip *chip = mtd->priv;
+ struct qcom_nandc_data *this = chip->priv;
+ int real_len = min_t(size_t, len, this->buf_count - this->buf_start);
+
+ memcpy(buf, this->data_buffer + this->buf_start, real_len);
+ this->buf_start += real_len;
+}
+
+static void qcom_nandc_write_buf(struct mtd_info *mtd, const uint8_t *buf,
+ int len)
+{
+ struct nand_chip *chip = mtd->priv;
+ struct qcom_nandc_data *this = chip->priv;
+ int real_len = min_t(size_t, len, this->buf_count - this->buf_start);
+
+ memcpy(this->data_buffer + this->buf_start, buf, real_len);
+
+ this->buf_start += real_len;
+}
+
+/* we support only one external chip for now */
+static void qcom_nandc_select_chip(struct mtd_info *mtd, int chipnr)
+{
+ struct nand_chip *chip = mtd->priv;
+ struct qcom_nandc_data *this = chip->priv;
+
+ if (chipnr <= 0)
+ return;
+
+ dev_warn(this->dev, "invalid chip select\n");
+}
+
+/*
+ * NAND controller page layout info
+ *
+ * |-----------------------| |---------------------------------|
+ * | xx.......xx| | *********xx.......xx|
+ * | DATA xx..ECC..xx| | DATA **SPARE**xx..ECC..xx|
+ * | (516) xx.......xx| | (516-n*4) **(n*4)**xx.......xx|
+ * | xx.......xx| | *********xx.......xx|
+ * |-----------------------| |---------------------------------|
+ * codeword 1,2..n-1 codeword n
+ * <---(528/532 Bytes)----> <-------(528/532 Bytes)---------->
+ *
+ * n = number of codewords in the page
+ * . = ECC bytes
+ * * = spare bytes
+ * x = unused/reserved bytes
+ *
+ * 2K page: n = 4, spare = 16 bytes
+ * 4K page: n = 8, spare = 32 bytes
+ * 8K page: n = 16, spare = 64 bytes
+ *
+ * the qcom nand controller operates at a sub page/codeword level. each
+ * codeword is 528 and 532 bytes for 4 bit and 8 bit ECC modes respectively.
+ * the number of ECC bytes vary based on the ECC strength and the bus width.
+ *
+ * the first n - 1 codewords contains 516 bytes of user data, the remaining
+ * 12/16 bytes consist of ECC and reserved data. The nth codeword contains
+ * both user data and spare(oobavail) bytes that sum up to 516 bytes.
+ *
+ * the layout described above is used by the controller when the ECC block is
+ * enabled. When we read a page with ECC enabled, the unused/reserved bytes are
+ * skipped and not copied to our internal buffer. therefore, the nand_ecclayout
+ * layouts defined below doesn't consider the positions occupied by the reserved
+ * bytes
+ *
+ * when the ECC block is disabled, one unused byte (or two for 16 bit bus width)
+ * in the last codeword is the position of bad block marker. the bad block
+ * marker cannot be accessed when ECC is enabled.
+ *
+ */
+
+/*
+ * Layouts for different page sizes and ecc modes. We skip the eccpos field
+ * since it isn't needed for this driver
+ */
+
+/* 2K page, 4 bit ECC */
+static struct nand_ecclayout layout_oob_64 = {
+ .eccbytes = 40,
+ .oobfree = {
+ { 30, 16 },
+ },
+};
+
+/* 4K page, 4 bit ECC, 8/16 bit bus width */
+static struct nand_ecclayout layout_oob_128 = {
+ .eccbytes = 80,
+ .oobfree = {
+ { 70, 32 },
+ },
+};
+
+/* 4K page, 8 bit ECC, 8 bit bus width */
+static struct nand_ecclayout layout_oob_224_x8 = {
+ .eccbytes = 104,
+ .oobfree = {
+ { 91, 32 },
+ },
+};
+
+/* 4K page, 8 bit ECC, 16 bit bus width */
+static struct nand_ecclayout layout_oob_224_x16 = {
+ .eccbytes = 112,
+ .oobfree = {
+ { 98, 32 },
+ },
+};
+
+/* 8K page, 4 bit ECC, 8/16 bit bus width */
+static struct nand_ecclayout layout_oob_256 = {
+ .eccbytes = 160,
+ .oobfree = {
+ { 151, 64 },
+ },
+};
+
+/*
+ * this is called before scan_ident, we do some minimal configurations so
+ * that reading ID and ONFI params work
+ */
+static void qcom_nandc_pre_init(struct qcom_nandc_data *this)
+{
+ /* kill onenand */
+ nandc_write(this, SFLASHC_BURST_CFG, 0);
+
+ /* enable ADM DMA */
+ nandc_write(this, NAND_FLASH_CHIP_SELECT, DM_EN);
+
+ /* save the original values of these registers */
+ this->cmd1 = nandc_read(this, NAND_DEV_CMD1);
+ this->vld = nandc_read(this, NAND_DEV_CMD_VLD);
+
+ /* initial status value */
+ this->status = NAND_STATUS_READY | NAND_STATUS_WP;
+}
+
+static int qcom_nandc_ecc_init(struct qcom_nandc_data *this)
+{
+ struct mtd_info *mtd = &this->mtd;
+ struct nand_chip *chip = &this->chip;
+ struct nand_ecc_ctrl *ecc = &chip->ecc;
+ int cwperpage;
+ bool wide_bus;
+
+ /* the nand controller fetches codewords/chunks of 512 bytes */
+ cwperpage = mtd->writesize >> 9;
+
+ /* strength is the net strength of the complete page */
+ ecc->strength = this->ecc_strength * cwperpage;
+
+ wide_bus = chip->options & NAND_BUSWIDTH_16 ? true : false;
+
+ if (ecc->strength >= 32) {
+ /* 8 bit ECC defaults to BCH ECC on all platforms */
+ ecc->bytes = wide_bus ? 14 : 13;
+ } else {
+ /*
+ * if the controller supports BCH for 4 bit ECC, the controller
+ * uses lesser bytes for ECC. If RS is used, the ECC bytes is
+ * always 10 bytes
+ */
+ if (this->ecc_modes & ECC_BCH_4BIT)
+ ecc->bytes = wide_bus ? 8 : 7;
+ else
+ ecc->bytes = 10;
+ }
+
+ /* each step consists of 512 bytes of data */
+ ecc->size = NANDC_STEP_SIZE;
+
+ ecc->read_page = qcom_nandc_read_page;
+ ecc->read_oob = qcom_nandc_read_oob;
+ ecc->write_page = qcom_nandc_write_page;
+ ecc->write_oob = qcom_nandc_write_oob;
+
+ /*
+ * the bad block marker is readable only when we read the page with ECC
+ * disabled. all the ops above run with ECC enabled. We need raw read
+ * and write function for oob in order to access bad block marker.
+ */
+ ecc->read_oob_raw = qcom_nandc_read_oob_raw;
+ ecc->write_oob_raw = qcom_nandc_write_oob_raw;
+
+ switch (mtd->oobsize) {
+ case 64:
+ ecc->layout = &layout_oob_64;
+ break;
+ case 128:
+ ecc->layout = &layout_oob_128;
+ break;
+ case 224:
+ if (wide_bus)
+ ecc->layout = &layout_oob_224_x16;
+ else
+ ecc->layout = &layout_oob_224_x8;
+ break;
+ case 256:
+ ecc->layout = &layout_oob_256;
+ break;
+ default:
+ dev_err(this->dev, "unsupported NAND device, oobsize %d\n",
+ mtd->oobsize);
+ return -ENODEV;
+ }
+
+ ecc->mode = NAND_ECC_HW;
+
+ /* enable ecc by default */
+ this->use_ecc = true;
+
+ /* free old buffer, allocate one with page data + oob size */
+ devm_kfree(this->dev, this->data_buffer);
+
+ this->buf_size = mtd->writesize + mtd->oobsize;
+
+ this->data_buffer = devm_kzalloc(this->dev, this->buf_size, GFP_KERNEL);
+ if (!this->data_buffer)
+ return -ENOMEM;
+
+ return 0;
+}
+
+static void qcom_nandc_hw_post_init(struct qcom_nandc_data *this)
+{
+ struct mtd_info *mtd = &this->mtd;
+ struct nand_chip *chip = &this->chip;
+ struct nand_ecc_ctrl *ecc = &chip->ecc;
+ int cwperpage = mtd->writesize / ecc->size;
+ int spare_bytes, bad_block_byte;
+ bool wide_bus;
+ int ecc_mode = 0;
+
+ wide_bus = chip->options & NAND_BUSWIDTH_16 ? true : false;
+
+ if (ecc->strength >= 32) {
+ this->cw_size = 532;
+
+ spare_bytes = wide_bus ? 0 : 2;
+
+ this->bch_enabled = true;
+ ecc_mode = 1;
+ } else {
+ this->cw_size = 528;
+
+ if (this->ecc_modes & ECC_BCH_4BIT) {
+ spare_bytes = wide_bus ? 2 : 4;
+
+ this->bch_enabled = true;
+ ecc_mode = 0;
+ } else {
+ spare_bytes = wide_bus ? 0 : 1;
+ }
+ }
+
+ /*
+ * DATA_UD_BYTES varies based on whether the read/write command protects
+ * spare data with ECC too. We protect spare data by default, so we set
+ * it to main + spare data, which are 512 and 4 bytes respectively.
+ */
+ this->cw_data = 516;
+
+ bad_block_byte = mtd->writesize - this->cw_size * (cwperpage - 1) + 1;
+
+ this->cfg0 = (cwperpage - 1) << CW_PER_PAGE
+ | this->cw_data << UD_SIZE_BYTES
+ | 0 << DISABLE_STATUS_AFTER_WRITE
+ | 5 << NUM_ADDR_CYCLES
+ | ecc->bytes << ECC_PARITY_SIZE_BYTES_RS
+ | 0 << STATUS_BFR_READ
+ | 1 << SET_RD_MODE_AFTER_STATUS
+ | spare_bytes << SPARE_SIZE_BYTES;
+
+ this->cfg1 = 7 << NAND_RECOVERY_CYCLES
+ | 0 << CS_ACTIVE_BSY
+ | bad_block_byte << BAD_BLOCK_BYTE_NUM
+ | 0 << BAD_BLOCK_IN_SPARE_AREA
+ | 2 << WR_RD_BSY_GAP
+ | wide_bus << WIDE_FLASH
+ | this->bch_enabled << ENABLE_BCH_ECC;
+
+ this->cfg0_raw = (cwperpage - 1) << CW_PER_PAGE
+ | this->cw_size << UD_SIZE_BYTES
+ | 5 << NUM_ADDR_CYCLES
+ | 0 << SPARE_SIZE_BYTES;
+
+ this->cfg1_raw = 7 << NAND_RECOVERY_CYCLES
+ | 0 << CS_ACTIVE_BSY
+ | 17 << BAD_BLOCK_BYTE_NUM
+ | 1 << BAD_BLOCK_IN_SPARE_AREA
+ | 2 << WR_RD_BSY_GAP
+ | wide_bus << WIDE_FLASH
+ | 1 << DEV0_CFG1_ECC_DISABLE;
+
+ this->ecc_bch_cfg = this->bch_enabled << ECC_CFG_ECC_DISABLE
+ | 0 << ECC_SW_RESET
+ | this->cw_data << ECC_NUM_DATA_BYTES
+ | 1 << ECC_FORCE_CLK_OPEN
+ | ecc_mode << ECC_MODE
+ | ecc->bytes << ECC_PARITY_SIZE_BYTES_BCH;
+
+ this->ecc_buf_cfg = 0x203 << NUM_STEPS;
+
+ this->clrflashstatus = FS_READY_BSY_N;
+ this->clrreadstatus = 0xc0;
+
+ dev_dbg(this->dev,
+ "cfg0 %x cfg1 %x ecc_buf_cfg %x ecc_bch cfg %x cw_size %d cw_data %d strength %d parity_bytes %d steps %d\n",
+ this->cfg0, this->cfg1, this->ecc_buf_cfg,
+ this->ecc_bch_cfg, this->cw_size, this->cw_data,
+ ecc->strength, ecc->bytes, cwperpage);
+}
+
+static int qcom_nandc_alloc(struct qcom_nandc_data *this)
+{
+ int r;
+
+ r = dma_set_coherent_mask(this->dev, DMA_BIT_MASK(32));
+ if (r) {
+ dev_err(this->dev, "failed to set DMA mask\n");
+ return r;
+ }
+
+ /*
+ * we don't know the page size of the NAND chip yet, set the buffer size
+ * to 512 bytes for now, that's sufficient for reading ID or ONFI params
+ */
+ this->buf_size = SZ_512;
+
+ this->data_buffer = devm_kzalloc(this->dev, this->buf_size, GFP_KERNEL);
+ if (!this->data_buffer)
+ return -ENOMEM;
+
+ this->regs = devm_kzalloc(this->dev, sizeof(struct nandc_regs),
+ GFP_KERNEL);
+ if (!this->regs)
+ return -ENOMEM;
+
+ this->reg_read_buf = devm_kzalloc(this->dev, MAX_REG_RD * sizeof(u32),
+ GFP_KERNEL);
+ if (!this->reg_read_buf)
+ return -ENOMEM;
+
+ INIT_LIST_HEAD(&this->list);
+
+ this->chan = dma_request_slave_channel(this->dev, "rxtx");
+ if (!this->chan) {
+ dev_err(this->dev, "failed to request slave channel\n");
+ return -ENODEV;
+ }
+
+ return 0;
+}
+
+static void qcom_nandc_unalloc(struct qcom_nandc_data *this)
+{
+ dma_release_channel(this->chan);
+}
+
+static int qcom_nandc_init(struct qcom_nandc_data *this)
+{
+ struct mtd_info *mtd = &this->mtd;
+ struct nand_chip *chip = &this->chip;
+ struct mtd_part_parser_data ppdata = {};
+ int r;
+
+ mtd->priv = chip;
+ mtd->name = "qcom-nandc";
+ mtd->owner = THIS_MODULE;
+
+ chip->priv = this;
+
+ chip->cmdfunc = qcom_nandc_command;
+ chip->select_chip = qcom_nandc_select_chip;
+ chip->read_byte = qcom_nandc_read_byte;
+ chip->read_buf = qcom_nandc_read_buf;
+ chip->write_buf = qcom_nandc_write_buf;
+
+ chip->options |= NAND_NO_SUBPAGE_WRITE;
+ chip->bbt_options = NAND_BBT_ACCESS_BBM_RAW | NAND_BBT_USE_FLASH;
+
+ if (this->bus_width == 16)
+ chip->options |= NAND_BUSWIDTH_16;
+
+ qcom_nandc_pre_init(this);
+
+ r = nand_scan_ident(mtd, 1, NULL);
+ if (r)
+ return r;
+
+ r = qcom_nandc_ecc_init(this);
+ if (r)
+ return r;
+
+ qcom_nandc_hw_post_init(this);
+
+ r = nand_scan_tail(mtd);
+ if (r)
+ return r;
+
+
+ ppdata.of_node = this->dev->of_node;
+ r = mtd_device_parse_register(mtd, NULL, &ppdata, NULL, 0);
+ if (r)
+ return r;
+
+ return 0;
+}
+
+static int qcom_nandc_parse_dt(struct platform_device *pdev)
+{
+ struct device_node *np;
+ struct qcom_nandc_data *this;
+ int r;
+
+ np = pdev->dev.of_node;
+ if (!np)
+ return -ENODEV;
+
+ this = platform_get_drvdata(pdev);
+ if (!this)
+ return -ENODEV;
+
+ this->ecc_strength = of_get_nand_ecc_strength(np);
+ if (this->ecc_strength < 0) {
+ dev_warn(this->dev,
+ "incorrect ecc strength, setting to 4 bits/step\n");
+ this->ecc_strength = 4;
+ }
+
+ this->bus_width = of_get_nand_bus_width(np);
+ if (this->bus_width < 0) {
+ dev_warn(this->dev, "incorrect bus width, setting to 8\n");
+ this->bus_width = 8;
+ }
+
+ r = of_property_read_u32(np, "qcom,cmd-crci", &this->cmd_crci);
+ if (r) {
+ dev_err(this->dev, "command CRCI unspecified\n");
+ return r;
+ }
+
+ r = of_property_read_u32(np, "qcom,data-crci", &this->data_crci);
+ if (r) {
+ dev_err(this->dev, "data CRCI unspecified\n");
+ return r;
+ }
+
+ return 0;
+}
+
+#define EBI2_NANDC_ECC_MODES (ECC_RS_4BIT | ECC_BCH_8BIT)
+
+static const struct of_device_id qcom_nandc_of_match[] = {
+ { .compatible = "qcom,ebi2-nandc",
+ .data = (void *) EBI2_NANDC_ECC_MODES,
+ },
+ {}
+};
+MODULE_DEVICE_TABLE(of, qcom_nandc_of_match);
+
+static int qcom_nandc_probe(struct platform_device *pdev)
+{
+ struct qcom_nandc_data *this;
+ const struct of_device_id *match;
+ int r;
+
+ this = devm_kzalloc(&pdev->dev, sizeof(*this), GFP_KERNEL);
+ if (!this)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, this);
+
+ this->pdev = pdev;
+ this->dev = &pdev->dev;
+
+ match = of_match_node(qcom_nandc_of_match, pdev->dev.of_node);
+ if (!match) {
+ dev_err(&pdev->dev, "unsupported NANDc module\n");
+ return -ENODEV;
+ }
+
+ /* match->data will hold a struct pointer once we support more IPs */
+ this->ecc_modes = (u32) match->data;
+
+ this->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ this->base = devm_ioremap_resource(&pdev->dev, this->res);
+ if (IS_ERR(this->base))
+ return PTR_ERR(this->base);
+
+ this->core_clk = devm_clk_get(&pdev->dev, "core");
+ if (IS_ERR(this->core_clk))
+ return PTR_ERR(this->core_clk);
+
+ this->aon_clk = devm_clk_get(&pdev->dev, "aon");
+ if (IS_ERR(this->aon_clk))
+ return PTR_ERR(this->aon_clk);
+
+ r = qcom_nandc_parse_dt(pdev);
+ if (r)
+ return r;
+
+ r = qcom_nandc_alloc(this);
+ if (r)
+ return r;
+
+ r = clk_prepare_enable(this->core_clk);
+ if (r)
+ goto err_core_clk;
+
+ r = clk_prepare_enable(this->aon_clk);
+ if (r)
+ goto err_aon_clk;
+
+ r = qcom_nandc_init(this);
+ if (r)
+ goto err_init;
+
+ return 0;
+
+err_init:
+ clk_disable_unprepare(this->aon_clk);
+err_aon_clk:
+ clk_disable_unprepare(this->core_clk);
+err_core_clk:
+ qcom_nandc_unalloc(this);
+
+ return r;
+}
+
+static int qcom_nandc_remove(struct platform_device *pdev)
+{
+ struct qcom_nandc_data *this;
+
+ this = platform_get_drvdata(pdev);
+ if (!this)
+ return -ENODEV;
+
+ qcom_nandc_unalloc(this);
+
+ clk_disable_unprepare(this->aon_clk);
+ clk_disable_unprepare(this->core_clk);
+
+ return 0;
+}
+
+static struct platform_driver qcom_nandc_driver = {
+ .driver = {
+ .name = "qcom-nandc",
+ .of_match_table = qcom_nandc_of_match,
+ },
+ .probe = qcom_nandc_probe,
+ .remove = qcom_nandc_remove,
+};
+module_platform_driver(qcom_nandc_driver);
+
+MODULE_AUTHOR("Archit Taneja <[email protected]>");
+MODULE_DESCRIPTION("Qualcomm NAND Controller driver");
+MODULE_LICENSE("GPL");
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

2015-07-21 10:35:17

[permalink] [raw]

Subject: [PATCH v2 3/5] dt/bindings: qcom_nandc: Add DT bindings

Add DT bindings document for the Qualcomm NAND controller driver.

Cc: [email protected]

Signed-off-by: Archit Taneja <[email protected]>
---
.../devicetree/bindings/mtd/qcom_nandc.txt | 48 ++++++++++++++++++++++
1 file changed, 48 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mtd/qcom_nandc.txt

diff --git a/Documentation/devicetree/bindings/mtd/qcom_nandc.txt b/Documentation/devicetree/bindings/mtd/qcom_nandc.txt
new file mode 100644
index 0000000..e24c77a
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/qcom_nandc.txt
@@ -0,0 +1,48 @@
+* Qualcomm NAND controller
+
+Required properties:
+- compatible: should be "qcom,ebi2-nand" for IPQ806x
+- reg: MMIO address range
+- clocks: must contain core clock and always on clock
+- clock-names: must contain "core" for the core clock and "aon" for the
+ always on clock
+- dmas: DMA specifier, consisting of a phandle to the ADM DMA
+ controller node and the channel number to be used for
+ NAND. Refer to dma.txt and qcom_adm.txt for more details
+- dma-names: must be "rxtx"
+- qcom,cmd-crci: must contain the ADM command type CRCI block instance
+ number specified for the NAND controller on the given
+ platform
+- qcom,data-crci: must contain the ADM data type CRCI block instance
+ number specified for the NAND controller on the given
+ platform
+
+Optional properties:
+- nand-bus-width: bus width. Must be 8 or 16. If not present, 8 is chosen
+ as default
+
+- nand-ecc-strength: number of bits to correct per ECC step. Must be 4 or 8
+ bits. If not present, 4 is chosen as default
+
+The device tree may optionally contain sub-nodes describing partitions of the
+address space. See partition.txt for more detail.
+
+Example:
+
+nand@0x1ac00000 {
+ compatible = "qcom,ebi2-nandc";
+ reg = <0x1ac00000 0x800>;
+
+ clocks = <&gcc EBI2_CLK>,
+ <&gcc EBI2_AON_CLK>;
+ clock-names = "core", "aon";
+
+ dmas = <&adm_dma 3>;
+ dma-names = "rxtx";
+ qcom,cmd-crci = <15>;
+ qcom,data-crci = <3>;
+
+ partition@0 {
+ ...
+ };
+};
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

2015-07-21 10:35:13

[permalink] [raw]

Subject: [PATCH v2 4/5] arm: qcom: dts: Add NAND controller node for ipq806x

The nand controller in IPQ806x is of the 'EBI2 type'. Use the corresponding
compatible string.

Cc: [email protected]
Signed-off-by: Archit Taneja <[email protected]>
---
arch/arm/boot/dts/qcom-ipq8064.dtsi | 15 +++++++++++++++
1 file changed, 15 insertions(+)

diff --git a/arch/arm/boot/dts/qcom-ipq8064.dtsi b/arch/arm/boot/dts/qcom-ipq8064.dtsi
index 1e1b3f0..08dc2ef 100644
--- a/arch/arm/boot/dts/qcom-ipq8064.dtsi
+++ b/arch/arm/boot/dts/qcom-ipq8064.dtsi
@@ -350,5 +350,20 @@
status = "disabled";
};

+ nand@0x1ac00000 {
+ compatible = "qcom,ebi2-nandc";
+ reg = <0x1ac00000 0x800>;
+
+ clocks = <&gcc EBI2_CLK>,
+ <&gcc EBI2_AON_CLK>;
+ clock-names = "core", "aon";
+
+ dmas = <&adm_dma 3>;
+ dma-names = "rxtx";
+ qcom,cmd-crci = <15>;
+ qcom,data-crci = <3>;
+
+ status = "disabled";
+ };
};
};
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

2015-07-21 10:35:57

[permalink] [raw]

Subject: [PATCH v2 5/5] arm: qcom: dts: Enale NAND node on IPQ8064 AP148 platform

Enable the NAND controller node on the AP148 platform. Provide pinmux
information.

Cc: [email protected]
Signed-off-by: Archit Taneja <[email protected]>
---
arch/arm/boot/dts/qcom-ipq8064-ap148.dts | 36 ++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)

diff --git a/arch/arm/boot/dts/qcom-ipq8064-ap148.dts b/arch/arm/boot/dts/qcom-ipq8064-ap148.dts
index 7f9ea50..03fd6b7 100644
--- a/arch/arm/boot/dts/qcom-ipq8064-ap148.dts
+++ b/arch/arm/boot/dts/qcom-ipq8064-ap148.dts
@@ -30,6 +30,28 @@
bias-none;
};
};
+ nand_pins: nand_pins {
+ mux {
+ pins = "gpio34", "gpio35", "gpio36",
+ "gpio37", "gpio38", "gpio39",
+ "gpio40", "gpio41", "gpio42",
+ "gpio43", "gpio44", "gpio45",
+ "gpio46", "gpio47";
+ function = "nand";
+ drive-strength = <10>;
+ bias-disable;
+ };
+ pullups {
+ pins = "gpio39";
+ bias-pull-up;
+ };
+ hold {
+ pins = "gpio40", "gpio41", "gpio42",
+ "gpio43", "gpio44", "gpio45",
+ "gpio46", "gpio47";
+ bias-bus-hold;
+ };
+ };
};

gsbi@16300000 {
@@ -93,5 +115,19 @@
sata@29000000 {
status = "ok";
};
+
+ nand@0x1ac00000 {
+ status = "ok";
+
+ pinctrl-0 = <&nand_pins>;
+ pinctrl-names = "default";
+
+ nand-ecc-strength = <4>;
+ nand-bus-width = <8>;
+ };
};
};
+
+&adm_dma {
+ status = "ok";
+};
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

2015-07-24 18:57:23

[permalink] [raw]

Subject: Re: [PATCH v2 3/5] dt/bindings: qcom_nandc: Add DT bindings

On Tue, Jul 21, 2015 at 04:04:44PM +0530, Archit Taneja wrote:
> Add DT bindings document for the Qualcomm NAND controller driver.
>
> Cc: [email protected]
>
> Signed-off-by: Archit Taneja <[email protected]>
> ---

Acked-by: Andy Gross <[email protected]>

--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

2015-07-24 18:58:21

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] arm: qcom: dts: Enale NAND node on IPQ8064 AP148 platform

On Tue, Jul 21, 2015 at 04:04:46PM +0530, Archit Taneja wrote:
> Enable the NAND controller node on the AP148 platform. Provide pinmux
> information.
>
> Cc: [email protected]
> Signed-off-by: Archit Taneja <[email protected]>
> ---

Looks fine.

Reviewed-by: Andy Gross <[email protected]>

--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

2015-07-24 18:59:21

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] arm: qcom: dts: Enale NAND node on IPQ8064 AP148 platform

On Tue, Jul 21, 2015 at 04:04:46PM +0530, Archit Taneja wrote:
> Enable the NAND controller node on the AP148 platform. Provide pinmux
> information.
>
> Cc: [email protected]
> Signed-off-by: Archit Taneja <[email protected]>
> ---

One nit though. The subject mispells Enable.

--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

2015-07-24 19:01:10

[permalink] [raw]

Subject: Re: [PATCH v2 1/5] mtd: nand: Create a BBT flag to access bad block markers in raw mode

On Tue, Jul 21, 2015 at 04:04:42PM +0530, Archit Taneja wrote:
> Some controllers can access the factory bad block marker from OOB only
> when they read it in raw mode. When ECC is enabled, these controllers
> discard reading/writing bad block markers, preventing access to them
> altogether.
>
> The bbt driver assumes MTD_OPS_PLACE_OOB when scanning for bad blocks.
> This results in the nand driver's ecc->read_oob() op to be called, which
> works with ECC enabled.
>
> Create a new BBT option flag that tells nand_bbt to force the mode to
> MTD_OPS_RAW. This would result in the correct op being called for the
> underlying nand controller driver.
>
> Signed-off-by: Archit Taneja <[email protected]>
> ---

Reviewed-by: Andy Gross <[email protected]>

--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

2015-07-24 19:01:56

[permalink] [raw]

Subject: Re: [PATCH v2 4/5] arm: qcom: dts: Add NAND controller node for ipq806x

On Tue, Jul 21, 2015 at 04:04:45PM +0530, Archit Taneja wrote:
> The nand controller in IPQ806x is of the 'EBI2 type'. Use the corresponding
> compatible string.
>
> Cc: [email protected]
> Signed-off-by: Archit Taneja <[email protected]>
> ---

Reviewed-by: Andy Gross <[email protected]>

--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

2015-07-24 19:37:26

[permalink] [raw]

Subject: Re: [PATCH v2 3/5] dt/bindings: qcom_nandc: Add DT bindings

On 07/21/2015 03:34 AM, Archit Taneja wrote:
> +
> +nand@0x1ac00000 {

s/0x//

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

2015-07-24 19:39:46

[permalink] [raw]

Subject: Re: [PATCH v2 2/5] mtd: nand: Qualcomm NAND controller driver

On Tue, Jul 21, 2015 at 04:04:43PM +0530, Archit Taneja wrote:

<snip>

>
> Signed-off-by: Archit Taneja <[email protected]>
> ---

Reviewed-by: Andy Gross <[email protected]>

--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

2015-07-25 00:51:54

[permalink] [raw]

Subject: Re: [PATCH v2 2/5] mtd: nand: Qualcomm NAND controller driver

On 07/21/2015 03:34 AM, Archit Taneja wrote:
> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> index 5b2806a..31951fc 100644
> --- a/drivers/mtd/nand/Kconfig
> +++ b/drivers/mtd/nand/Kconfig
> @@ -538,4 +538,11 @@ config MTD_NAND_HISI504
> help
> Enables support for NAND controller on Hisilicon SoC Hip04.
>
> +config MTD_NAND_QCOM
> + tristate "Support for NAND on QCOM SoCs"
> + depends on ARCH_QCOM && QCOM_ADM

This is sort of annoying that the menu won't show up unless the ADM
driver is also enabled (which would be in a completely different area of
the configurator). Perhaps drop that requirement because it isn't
required to build?

> + help
> + Enables support for NAND flash chips on SoCs containing the EBI2 NAND
> + controller. This controller is found on IPQ806x SoC.
> +
> endif # MTD_NAND
> diff --git a/drivers/mtd/nand/qcom_nandc.c b/drivers/mtd/nand/qcom_nandc.c
> new file mode 100644
> index 0000000..51c284c
> --- /dev/null
> +++ b/drivers/mtd/nand/qcom_nandc.c
> @@ -0,0 +1,2019 @@
> +/*
> + * Copyright (c) 2015, The Linux Foundation. All rights reserved.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/slab.h>
> +#include <linux/interrupt.h>

Where is this used?

> +#include <linux/bitops.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/dmaengine.h>
> +#include <linux/module.h>
> +#include <linux/mtd/nand.h>
> +#include <linux/mtd/partitions.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_mtd.h>
> +#include <linux/delay.h>
> +
[..]
> +/*
> + * the NAND controller performs reads/writes with ECC in 516 byte chunks.
> + * the driver calls the chunks 'step' or 'codeword' interchangeably
> + */
> +#define NANDC_STEP_SIZE 512
> +
> +/*
> + * the largest page size we support is 8K, this will have 16 steps/codewords
> + * of 512 bytes each
> + */
> +#define MAX_NUM_STEPS (SZ_8K / NANDC_STEP_SIZE)
> +
> +/* we read at most 3 registers per codeword scan */
> +#define MAX_REG_RD (3 * MAX_NUM_STEPS)
> +
> +/* ECC modes */
> +#define ECC_NONE BIT(0)
> +#define ECC_RS_4BIT BIT(1)
> +#define ECC_BCH_4BIT BIT(2)
> +#define ECC_BCH_8BIT BIT(3)
> +
> +struct desc_info {
> + struct list_head list;
> +
> + enum dma_transfer_direction dir;
> + struct scatterlist sgl;
> + struct dma_async_tx_descriptor *dma_desc;
> +};
> +
> +/*
> + * holds the current register values that we want to write. acts as a contiguous
> + * chunk of memory which we use to write the controller registers through DMA.
> + */
> +struct nandc_regs {
> + u32 cmd;
> + u32 addr0;
> + u32 addr1;
> + u32 chip_sel;
> + u32 exec;
> +
> + u32 cfg0;
> + u32 cfg1;
> + u32 ecc_bch_cfg;
> +
> + u32 clrflashstatus;
> + u32 clrreadstatus;
> +
> + u32 cmd1;
> + u32 vld;
> +
> + u32 orig_cmd1;
> + u32 orig_vld;
> +
> + u32 ecc_buf_cfg;
> +};
> +
> +/*
> + * @cmd_crci: ADM DMA CRCI for command flow control
> + * @data_crci: ADM DMA CRCI for data flow control
> + * @list: DMA descriptor list (list of desc_infos)
> + * @dma_done: completion param to denote end of last
> + * descriptor in the list
> + * @data_buffer: our local DMA buffer for page read/writes,
> + * used when we can't use the buffer provided
> + * by upper layers directly
> + * @buf_size/count/start: markers for chip->read_buf/write_buf functions
> + * @reg_read_buf: buffer for reading register data via DMA
> + * @reg_read_pos: marker for data read in reg_read_buf
> + * @cfg0, cfg1, cfg0_raw..: NANDc register configurations needed for
> + * ecc/non-ecc mode for the current nand flash
> + * device
> + * @regs: a contiguous chunk of memory for DMA register
> + * writes
> + * @ecc_strength: 4 bit or 8 bit ecc, received via DT
> + * @bus_width: 8 bit or 16 bit NAND bus width, received via DT
> + * @ecc_modes: supported ECC modes by the current controller,
> + * initialized via DT match data
> + * @cw_size: the number of bytes in a single step/codeword
> + * of a page, consisting of all data, ecc, spare
> + * and reserved bytes
> + * @cw_data: the number of bytes within a codeword protected
> + * by ECC
> + * @bch_enabled: flag to tell whether BCH or RS ECC mode is used
> + * @status: value to be returned if NAND_CMD_STATUS command
> + * is executed
> + */
> +struct qcom_nandc_data {
> + struct platform_device *pdev;
> + struct device *dev;
> +
> + void __iomem *base;
> + struct resource *res;
> +
> + struct clk *core_clk;
> + struct clk *aon_clk;
> +
> + /* DMA stuff */
> + struct dma_chan *chan;
> + struct dma_slave_config slave_conf;
> + unsigned int cmd_crci;
> + unsigned int data_crci;
> + struct list_head list;
> + struct completion dma_done;
> +
> + /* MTD stuff */
> + struct nand_chip chip;
> + struct mtd_info mtd;
> +
> + /* local data buffer and markers */
> + u8 *data_buffer;
> + int buf_size;
> + int buf_count;
> + int buf_start;
> +
> + /* local buffer to read back registers */
> + u32 *reg_read_buf;
> + dma_addr_t reg_read_paddr;

Is this used?

> + int reg_read_pos;
> +
> + /* required configs */
> + u32 cfg0, cfg1;
> + u32 cfg0_raw, cfg1_raw;
> + u32 ecc_buf_cfg;
> + u32 ecc_bch_cfg;
> + u32 clrflashstatus;
> + u32 clrreadstatus;
> + u32 sflashc_burst_cfg;
> + u32 cmd1, vld;
> +
> + /* register state */
> + struct nandc_regs *regs;
> +
> + /* things we get from DT */
> + int ecc_strength;
> + int bus_width;
> +
> + u32 ecc_modes;
> +
> + /* misc params */
> + int cw_size;
> + int cw_data;
> + bool use_ecc;
> + bool bch_enabled;
> + u8 status;
> + int last_command;
> +};
> +
> +static inline unsigned int nandc_read(struct qcom_nandc_data *this, int offset)
> +{
> + return ioread32(this->base + offset);
> +}
> +
> +static inline void nandc_write(struct qcom_nandc_data *this, int offset,
> + unsigned int val)
> +{
> + iowrite32(val, this->base + offset);
> +}

Perhaps these should take and return u32s to match the signature of
ioread32/iowrite32

> +
> +/* helper to configure address register values */
> +static void set_address(struct qcom_nandc_data *this, u16 column, int page)
> +{
> + struct nand_chip *chip = &this->chip;
> + struct nandc_regs *regs = this->regs;
> +
> + if (chip->options & NAND_BUSWIDTH_16)
> + column >>= 1;
> +
> + regs->addr0 = page << 16 | column;
> + regs->addr1 = page >> 16 & 0xff;
> +}
> +
> +/*
> + * update_rw_regs: set up read/write register values, these will be
> + * written to the NAND controller registers via DMA
> + *
> + * @num_cw: number of steps for the read/write operation
> + * @read: read or write operation
> + */
> +static void update_rw_regs(struct qcom_nandc_data *this, int num_cw, bool read)
> +{
> + struct nandc_regs *regs = this->regs;
> +
> + if (this->use_ecc) {
> + if (read)
> + regs->cmd = PAGE_READ_WITH_ECC | PAGE_ACC | LAST_PAGE;
> + else
> + regs->cmd = PROGRAM_PAGE | PAGE_ACC | LAST_PAGE;
> +
> + regs->cfg0 = (this->cfg0 & ~(7U << CW_PER_PAGE)) |
> + (num_cw - 1) << CW_PER_PAGE;
> +
> + regs->cfg1 = this->cfg1;
> + regs->ecc_bch_cfg = this->ecc_bch_cfg;
> + } else {
> + if (read)
> + regs->cmd = PAGE_READ | PAGE_ACC | LAST_PAGE;
> + else
> + regs->cmd = PROGRAM_PAGE | PAGE_ACC | LAST_PAGE;
> +
> + regs->cfg0 = (this->cfg0_raw & ~(7U << CW_PER_PAGE)) |
> + (num_cw - 1) << CW_PER_PAGE;
> +
> + regs->cfg1 = this->cfg1_raw;
> + regs->ecc_bch_cfg = 1 << ECC_CFG_ECC_DISABLE;
> + }

These two arms are almost exactly the same, except regs->cmd has
PAGE_READ_WITH_ECC vs PAGE_READ and regs->ecc_bch_cfg is different. It
should be possible to push the use_ecc case down into the two places
that need it and reduce lines.

> +
> + regs->ecc_buf_cfg = this->ecc_buf_cfg;
> + regs->clrflashstatus = this->clrflashstatus;
> + regs->clrreadstatus = this->clrreadstatus;
> + regs->exec = 1;
> +}
> +
> +/*
> + * write_reg_dma: prepares a descriptor to write a given number of
> + * contiguous registers
> + *
> + * @first: offset of the first register in the contiguous block
> + * @reg: starting address containing the reg values to write
> + * @num_regs: number of registers to write
> + * @flow_control: flow control enabled/disabled
> + */
> +static int write_reg_dma(struct qcom_nandc_data *this, int first, u32 *reg,
> + int num_regs, bool flow_control)
> +{
> + struct desc_info *desc;
> + struct dma_async_tx_descriptor *dma_desc;
> + struct scatterlist *sgl;
> + int size;
> + int r;
> +
> + desc = kzalloc(sizeof(*desc), GFP_KERNEL);
> + if (!desc)
> + return -ENOMEM;
> +
> + list_add_tail(&desc->list, &this->list);
> +
> + sgl = &desc->sgl;
> +
> + size = num_regs * sizeof(u32);
> +
> + sg_init_one(sgl, reg, size);
> +
> + desc->dir = DMA_MEM_TO_DEV;
> +
> + dma_map_sg(this->dev, sgl, 1, desc->dir);
> +
> + this->slave_conf.device_fc = flow_control ? 1 : 0;
> + this->slave_conf.dst_addr = this->res->start + first;
> + this->slave_conf.dst_maxburst = 16;
> + this->slave_conf.slave_id = this->cmd_crci;
> +
> + r = dmaengine_slave_config(this->chan, &this->slave_conf);
> + if (r) {
> + dev_err(this->dev, "failed to configure dma channel\n");
> + goto err;
> + }
> +
> + dma_desc = dmaengine_prep_slave_sg(this->chan, sgl, 1, desc->dir, 0);
> + if (!dma_desc) {
> + dev_err(this->dev, "failed to prepare register write desc\n");
> + r = PTR_ERR(dma_desc);

PTR_ERR(NULL) doesn't make sense. It would be nice if the DMA engine
APIs were documented so we knew if it returned NULL or an error pointer
on failure.

> + goto err;
> + }
> +
> + desc->dma_desc = dma_desc;
> +
> + return 0;
> +err:
> + kfree(desc);
> +
> + return r;
> +}
> +
> +/*

BTW, this isn't kernel doc. That would need two asterisks.

> + * read_reg_dma: prepares a descriptor to read a given number of
> + * contiguous registers to the reg_read_buf pointer
> + *
> + * @first: offset of the first register in the contiguous block
> + * @num_regs: number of registers to read
> + * @flow_control: flow control enabled/disabled
> + */
> +static int read_reg_dma(struct qcom_nandc_data *this, int first, int num_regs,
> + bool flow_control)
> +{
> + struct desc_info *desc;
> + struct dma_async_tx_descriptor *dma_desc;
> + struct scatterlist *sgl;
> + int size;
> + int r;
> +
> + desc = kzalloc(sizeof(*desc), GFP_KERNEL);
> + if (!desc)
> + return -ENOMEM;
> +
> + list_add_tail(&desc->list, &this->list);
> +
> + sgl = &desc->sgl;
> +
> + size = num_regs * sizeof(u32);
> +
> + sg_init_one(sgl, this->reg_read_buf + this->reg_read_pos, size);
> +
> + desc->dir = DMA_DEV_TO_MEM;
> +
> + dma_map_sg(this->dev, sgl, 1, desc->dir);
> +
> + this->slave_conf.device_fc = flow_control ? 1 : 0;

device_fc is a bool, so why the 1 : 0 trick? flow_control is already bool.

> + this->slave_conf.src_addr = this->res->start + first;
> + this->slave_conf.src_maxburst = 16;
> + this->slave_conf.slave_id = this->data_crci;
> +
> + r = dmaengine_slave_config(this->chan, &this->slave_conf);
> + if (r) {
> + dev_err(this->dev, "failed to configure dma channel\n");
> + goto err;
> + }
> +
> + dma_desc = dmaengine_prep_slave_sg(this->chan, sgl, 1, desc->dir, 0);
> + if (!dma_desc) {
> + dev_err(this->dev, "failed to prepare register read desc\n");
> + r = PTR_ERR(dma_desc);

Same problem here for PTR_ERR. Would be nice to figure out a way to
consolidate these DMA functions. They're only subtly different.

> + goto err;
> + }
> +
> + desc->dma_desc = dma_desc;
> +
> + this->reg_read_pos += num_regs;
> +
> + return 0;
> +err:
> + kfree(desc);
> +
> + return r;
> +}
> +
[..]
> +
> +/*
> + * write_data_dma: prepares a DMA descriptor to transfer data from
> + * 'vaddr' to the controller's internal buffer
> + *
> + * @reg_off: offset within the controller's data buffer
> + * @vaddr: virtual address of the buffer we want to read from
> + * @size: DMA transaction size in bytes
> + */
> +static int write_data_dma(struct qcom_nandc_data *this, int reg_off, u8 *vaddr,

const u8 *vaddr?

> + int size)
> +{
> + struct desc_info *desc;
> + struct dma_async_tx_descriptor *dma_desc;
> + struct scatterlist *sgl;
> + int r;
> +
> + desc = kzalloc(sizeof(*desc), GFP_KERNEL);
> + if (!desc)
> + return -ENOMEM;
> +
> + list_add_tail(&desc->list, &this->list);
> +
> + sgl = &desc->sgl;
> +
> + sg_init_one(sgl, vaddr, size);
> +
> + desc->dir = DMA_MEM_TO_DEV;
> +
> + r = dma_map_sg(this->dev, sgl, 1, desc->dir);
> + if (r == 0)
> + goto err;

Should we return an error in this case? Looks like return 0.

> +
> + this->slave_conf.device_fc = 0;
> + this->slave_conf.dst_addr = this->res->start + reg_off;
> + this->slave_conf.dst_maxburst = 16;

Is there any reason why slave_conf can't be on the stack? Otherwise it's
odd that it's overwritten a few times before we submit the descriptors,
so it must be copied by the dmaengine provider, but that isn't clear at
all from the code. If it isn't copied, perhaps it should be part of the
desc_info structure. If it is copied I wonder why it isn't const in the
function signature.

> +
> + r = dmaengine_slave_config(this->chan, &this->slave_conf);
> + if (r) {
> + dev_err(this->dev, "failed to configure dma channel\n");
> + goto err;
> + }
> +
> + dma_desc = dmaengine_prep_slave_sg(this->chan, sgl, 1, desc->dir, 0);
> + if (!dma_desc) {
> + dev_err(this->dev, "failed to prepare data write desc\n");
> + r = PTR_ERR(dma_desc);
> + goto err;
> + }
> +
> + desc->dma_desc = dma_desc;
> +
> + return 0;
> +err:
> + kfree(desc);
> +
> + return r;
> +}
> +
> +/*
> + * helper to prepare dma descriptors to configure registers needed for reading a
> + * codeword/step in a page
> + */
> +static void config_cw_read(struct qcom_nandc_data *this)
> +{
> + struct nandc_regs *regs = this->regs;
> +
> + write_reg_dma(this, NAND_FLASH_CMD, &regs->cmd, 3, true);

Maybe it would be better to have a case statement inside
{write,read}_reg_dma() that looked at the second argument and matched it
up with an offset in regs. Then this could be

write_reg_dma(this, NAND_FLASH_CMD, 3, true);

and we wouldn't have to worry about having the wrong argument for
parameter 2 and parameter 3. It may even be that we always read the same
number of registers too? In which case we could move parameter 4 into
the case statement too?

> + write_reg_dma(this, NAND_DEV0_CFG0, &regs->cfg0, 3, false);
> + write_reg_dma(this, NAND_EBI2_ECC_BUF_CFG, &regs->ecc_buf_cfg,
> + 1, false);
> +
> + write_reg_dma(this, NAND_EXEC_CMD, &regs->exec, 1, false);
> +
> + read_reg_dma(this, NAND_FLASH_STATUS, 2, true);
> + read_reg_dma(this, NAND_ERASED_CW_DETECT_STATUS, 1, false);
> +}
> +
[...]
> +/* sets up descriptors for NAND_CMD_RESET */
> +static int reset(struct qcom_nandc_data *this)
> +{
> + struct nandc_regs *regs = this->regs;
> +
> + regs->cmd = RESET_DEVICE;
> + regs->exec = 1;
> +
> + write_reg_dma(this, NAND_FLASH_CMD, &regs->cmd, 1, true);
> + write_reg_dma(this, NAND_EXEC_CMD, &regs->exec, 1, false);
> +
> + read_reg_dma(this, NAND_FLASH_STATUS, 1, true);
> +
> + return 0;
> +}
> +
> +/* helpers to submit/free our list of dma descriptors */
> +static void dma_callback(void *param)
> +{
> + struct qcom_nandc_data *this = (struct qcom_nandc_data *) param;

Useless cast.

> + struct completion *c = &this->dma_done;
> +
> + complete(c);
> +}
> +
> +static int submit_descs(struct qcom_nandc_data *this)
> +{
> + struct completion *c = &this->dma_done;
> + struct desc_info *desc;
> + int r;
> +
> + init_completion(c);
> +
> + list_for_each_entry(desc, &this->list, list) {
> + /*
> + * we add a callback the last descriptor in our list to notify

to the last?

> + * completion of command
> + */
> + if (list_is_last(&desc->list, &this->list)) {
> + desc->dma_desc->callback = dma_callback;
> + desc->dma_desc->callback_param = this;
> + }
> +
> + dmaengine_submit(desc->dma_desc);
> + }
> +
> + dma_async_issue_pending(this->chan);
> +
> + r = wait_for_completion_timeout(c, msecs_to_jiffies(500));
> + if (!r)
> + return -EINVAL;

Why not -ETIMEDOUT?

> +
> + return 0;
> +}
> +
[...]
> +
> +/*
> + * when using RS ECC, the NAND controller flags an error when reading an
> + * erased page. however, there are special characters at certain offsets when
> + * we read the erased page. we check here if the page is really empty. if so,
> + * we replace the magic characters with 0xffs
> + */
> +static bool empty_page_fixup(struct qcom_nandc_data *this, u8 *data_buf)
> +{
> + struct mtd_info *mtd = &this->mtd;
> + struct nand_chip *chip = &this->chip;
> + struct nand_ecc_ctrl *ecc = &chip->ecc;
> + int cwperpage = ecc->steps;
> + int i;
> +
> + /* if BCH is enabled, HW will take care of detecting erased pages */
> + if (this->bch_enabled || !this->use_ecc)
> + return false;
> +
> + for (i = 0; i < cwperpage; i++) {
> + u8 *empty1, *empty2;
> + u32 flash_status = this->reg_read_buf[3 * i];
> +
> + /*
> + * an erased page flags an error in NAND_FLASH_STATUS, check if
> + * the page is erased by looking for 0x54s at offsets 3 and 175
> + * from the beginning of each codeword
> + */
> + if (flash_status & FS_OP_ERR) {
> + empty1 = &data_buf[3 + i * this->cw_data];
> + empty2 = &data_buf[175 + i * this->cw_data];
> +
> + /*
> + * the error wasn't because of an erased page, bail out
> + * and let someone else do the error checking
> + */
> + if (!((*empty1 == 0x54 && *empty2 == 0xff) ||
> + (*empty1 == 0xff && *empty2 == 0x54)))

Why are we using pointers? Just use u8 empty1, empty2 = data_buf[...] ?

> + return false;
> + }
> + }
> +
> + for (i = 0; i < mtd->writesize && (data_buf[i] == 0xff ||
> + (i % this->cw_data == 3 || i % this->cw_data == 175)); i++) {
> + }

This might be clearer like so:

for (i = 0; i < mtd->writesize; i++) {
if (i % this->cw_data == 3 || i % this->cw_data == 175);
continue;
if (data_buf[i] != 0xff)
return false;
}

and then drop the if after the loop. Actually since we're checking the
whole page it may be better to do this:

+
+ /*
+ * the whole page is 0xffs besides the magic offsets, we replace the
+ * 0x54s with 0xffs
+ */
+ for (i = 0; i < cwperpage; i++) {
+ data_buf[3 + i * this->cw_data] = 0xff;
+ data_buf[175 + i * this->cw_data] = 0xff;
+ }
+

and then

return memchr_inv(data_buf, 0xff, mtd->writesize) ? false : true ;

the memchr_inv() is optimized to find the bad characters 8 bytes at a time.

> +
> + if (i < mtd->writesize)
> + return false;
> +
> + /*
> + * the whole page is 0xffs besides the magic offsets, we replace the
> + * 0x54s with 0xffs
> + */
> + for (i = 0; i < cwperpage; i++) {
> + data_buf[3 + i * this->cw_data] = 0xff;
> + data_buf[175 + i * this->cw_data] = 0xff;
> + }
> +
> + /*
> + * tell the caller that the page was empty and is fixed up, so that
> + * parse_read_errors() doesn't think it's and error
> + */
> + return true;
> +}
> +
> +/*
> + * reads back status registers set by the controller to notify page read
> + * errors. this is equivalent to what 'ecc->correct()' would do.
> + */
> +static int parse_read_errors(struct qcom_nandc_data *this, bool erased_page)
> +{
> + struct mtd_info *mtd = &this->mtd;
> + struct nand_chip *chip = &this->chip;
> + struct nand_ecc_ctrl *ecc = &chip->ecc;
> + int cwperpage = ecc->steps;
> + unsigned int max_bitflips = 0;
> + int i;
> +
> + for (i = 0; i < cwperpage; i++) {
> + int stat;
> + u32 flash_status = this->reg_read_buf[3 * i];
> + u32 buffer_status = this->reg_read_buf[3 * i + 1];
> + u32 erased_cw_status = this->reg_read_buf[3 * i + 2];

struct stats {
u32 flash;
u32 buffer;
u32 erased_cw;
};

struct stats *buf = (struct stat *)this->reg_read_buf;

for (i = 0; i < cwperpage; i++, buf++) {
int stat;
if (buf->flash & (FS_OP_ERR | FS_MPU_ERR))

?

Also, is the buffer little endian? If so, that should be a __le32 flash,
__le32 buffer, etc. up there and then le32_to_cpu().

> +
> + if (flash_status & (FS_OP_ERR | FS_MPU_ERR)) {
> +
> + /* ignore erased codeword errors */
> + if (this->bch_enabled) {
> + if ((erased_cw_status & ERASED_CW) == ERASED_CW)
> + continue;
> + } else if (erased_page) {
> + continue;
> + }
> +
> + if (buffer_status & BS_UNCORRECTABLE_BIT) {
> + mtd->ecc_stats.failed++;
> + continue;
> + }
> + }
> +
> + stat = buffer_status & BS_CORRECTABLE_ERR_MSK;
> + mtd->ecc_stats.corrected += stat;
> +
> + max_bitflips = max_t(unsigned int, max_bitflips, stat);
> + }
> +
> + return max_bitflips;
> +}
> +
> +/*
> + * helper to perform the actual page read operation, used by ecc->read_page()
> + * and ecc->read_oob()
> + */
> +static int read_page_low(struct qcom_nandc_data *this, u8 *data_buf,
> + u8 *oob_buf)
> +{
> + struct nand_chip *chip = &this->chip;
> + struct nand_ecc_ctrl *ecc = &chip->ecc;
> + int i, r;
> +
> + /* queue cmd descs for each codeword */
> + for (i = 0; i < ecc->steps; i++) {
> + int data_size, oob_size;
> +
> + if (i == (ecc->steps - 1)) {
> + data_size = ecc->size - ((ecc->steps - 1) << 2);
> + oob_size = (ecc->steps << 2) + ecc->bytes;
> + } else {
> + data_size = this->cw_data;
> + oob_size = ecc->bytes;
> + }
> +
> + config_cw_read(this);
> +
> + if (data_buf)
> + read_data_dma(this, FLASH_BUF_ACC, data_buf, data_size);
> +
> + if (oob_buf)
> + read_data_dma(this, FLASH_BUF_ACC + data_size, oob_buf,
> + oob_size);
> +
> + if (data_buf)
> + data_buf += data_size;
> + if (oob_buf)
> + oob_buf += oob_size;
> + }
> +
> + r = submit_descs(this);
> + if (r)
> + dev_err(this->dev, "failure to read page/oob\n");
> +
> + free_descs(this);
> +
> + return r;
> +}
> +
> +/*
> + * a helper that copies the last step/codeword of a page (containing free oob)
> + * into our local buffer
> + */
> +static int copy_last_cw(struct qcom_nandc_data *this, bool use_ecc, int page)
> +{
> + struct nand_chip *chip = &this->chip;
> + struct nand_ecc_ctrl *ecc = &chip->ecc;
> + int size;
> + int r;
> +
> + clear_read_regs(this);
> +
> + size = use_ecc ? this->cw_data : this->cw_size;
> +
> + /* prepare a clean read buffer */
> + memset(this->data_buffer, 0xff, size);
> +
> + this->use_ecc = use_ecc;
> + set_address(this, this->cw_size * (ecc->steps - 1), page);
> + update_rw_regs(this, 1, true);
> +
> + config_cw_read(this);
> +
> + read_data_dma(this, FLASH_BUF_ACC, this->data_buffer, size);
> +
> + r = submit_descs(this);
> + if (r)
> + dev_err(this->dev, "failed to copy last codeword\n");
> +
> + free_descs(this);
> +
> + return r;
> +}
> +
> +/* implements ecc->read_page() */
> +static int qcom_nandc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
> + uint8_t *buf, int oob_required, int page)
> +{
> + struct qcom_nandc_data *this = chip->priv;
> + u8 *data_buf, *oob_buf = NULL;
> + bool direct_dma, erased_page;
> + int r;
> +
> + /*
> + * first try to map the upper buffer directly, else, use our own buf
> + * and memcpy to upper buf
> + */
> + if (virt_addr_valid(buf) && !object_is_on_stack(buf)) {
> + direct_dma = true;
> + data_buf = buf;
> + } else {
> + direct_dma = false;
> + data_buf = this->data_buffer;
> + memset(data_buf, 0xff, mtd->writesize + mtd->oobsize);
> + }
> +
> + oob_buf = oob_required ? chip->oob_poi : NULL;
> +
> + r = read_page_low(this, data_buf, oob_buf);
> + if (r) {
> + dev_err(this->dev, "failure to read page\n");
> + goto err;

return r?

> + }
> +
> + erased_page = empty_page_fixup(this, data_buf);
> +
> + if (!direct_dma)
> + memcpy(buf, this->data_buffer, mtd->writesize);
> +
> + r = parse_read_errors(this, erased_page);
> +err:
> + return r;

and then return parse_read_errors()?

> +}
> +
> +/* implements ecc->read_oob() */
> +static int qcom_nandc_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
> + int page)
> +{
> + struct qcom_nandc_data *this = chip->priv;
> + struct nand_ecc_ctrl *ecc = &chip->ecc;
> + int r;
> +
> + clear_read_regs(this);
> +
> + this->use_ecc = true;
> + set_address(this, 0, page);
> + update_rw_regs(this, ecc->steps, true);
> +
> + r = read_page_low(this, NULL, chip->oob_poi);
> + if (r)
> + dev_err(this->dev, "failure to read oob\n");
> +
> + return r;
> +}
> +
> +/* implements ecc->read_oob_raw(), used to read the bad block marker flag */
> +static int qcom_nandc_read_oob_raw(struct mtd_info *mtd, struct nand_chip *chip,
> + int page)
> +{
> + struct qcom_nandc_data *this = chip->priv;
> + struct nand_ecc_ctrl *ecc = &chip->ecc;
> + uint8_t *oob = chip->oob_poi;
> + int start, length;
> + int r;
> +
> + /*
> + * configure registers for a raw page read, the address is set to the
> + * beginning of the last codeword, we don't care about reading ecc
> + * portion of oob, just the free stuff
> + */
> + r = copy_last_cw(this, false, page);
> + if (r)
> + return r;
> +
> + /*
> + * reading raw oob has 2 parts, first the bad block byte, then the
> + * actual free oob region. perform a memcpy in two steps
> + */
> + start = mtd->writesize - (this->cw_size * (ecc->steps - 1));
> + length = chip->options & NAND_BUSWIDTH_16 ? 2 : 1;
> +
> + memcpy(oob, this->data_buffer + start, length);
> +
> + oob += length;
> +
> + start = this->cw_data - (ecc->steps << 2) + 1;
> + length = ecc->steps << 2;
> +
> + memcpy(oob, this->data_buffer + start, length);
> +
> + return 0;
> +}
> +
> +/* implements ecc->write_page() */
> +static int qcom_nandc_write_page(struct mtd_info *mtd, struct nand_chip *chip,
> + const uint8_t *buf, int oob_required)
> +{
> + struct qcom_nandc_data *this = chip->priv;
> + struct nand_ecc_ctrl *ecc = &chip->ecc;
> + u8 *data_buf, *oob_buf;
> + bool direct_dma;
> + int i, r = 0;
> +
> + clear_read_regs(this);
> +
> + /*
> + * first try to map the upper buffer directly, else, memcpy upper
> + * buffer to our local buffer
> + */
> + direct_dma = virt_addr_valid(buf) && !object_is_on_stack((void *) buf);

We should change object_is_on_stack() to take a const pointer.

Are we guaranteed that this is called within the same context as where
the buffer is passed to this function? Otherwise this stack check isn't
going to work because object_is_on_stack() will silently fail.

> +
> + if (!direct_dma) {
> + memcpy(this->data_buffer, buf, mtd->writesize
> );
> + data_buf = this->data_buffer;
> + } else {
> + data_buf = (u8 *) buf;

Shouldn't need to cast away const...

> + }
> +
> + oob_buf = chip->oob_poi;
> +
> + this->use_ecc = true;
> + update_rw_regs(this, ecc->steps, false);
> +
> + for (i = 0; i < ecc->steps; i++) {
> + int data_size, oob_size;
> +
> + if (i == (ecc->steps - 1)) {
> + data_size = ecc->size - ((ecc->steps - 1) << 2);
> + oob_size = (ecc->steps << 2) + ecc->bytes;
> + } else {
> + data_size = this->cw_data;
> + oob_size = ecc->bytes;
> + }
> +
> + config_cw_write_pre(this);
> + write_data_dma(this, FLASH_BUF_ACC, data_buf, data_size);
> +
> + /*
> + * we don't really need to write anything to oob for the
> + * first n - 1 codewords since these oob regions just
> + * contain ecc that's written by the controller itself
> + */
> + if (i == (ecc->steps - 1))
> + write_data_dma(this, FLASH_BUF_ACC + data_size,
> + oob_buf, oob_size);
> + config_cw_write_post(this);
> +
> + data_buf += data_size;
> + oob_buf += oob_size;
> + }
> +
> + r = submit_descs(this);
> + if (r)
> + dev_err(this->dev, "failure to write page\n");
> +
> + free_descs(this);
> +
> + return r;
> +}
> +
> +/*
> + * implements ecc->write_oob()
> + *
> + * the NAND controller cannot write only data or only oob within a codeword,
> + * since ecc is calculated for the combined codeword. we first copy the
> + * entire contents for the last codeword(data + oob), replace the old oob
> + * with the new one in chip->oob_poi, and then write the entire codeword.
> + * this read-copy-write operation results in a slight perormance loss.
> + */
> +static int qcom_nandc_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
> + int page)
> +{
> + struct qcom_nandc_data *this = chip->priv;
> + struct nand_ecc_ctrl *ecc = &chip->ecc;
> + uint8_t *oob = chip->oob_poi;
> + int free_boff;
> + int data_size, oob_size;
> + int r, status = 0;
> +
> + r = copy_last_cw(this, true, page);
> + if (r)
> + return r;
> +
> + clear_read_regs(this);
> +
> + /* calculate the data and oob size for the last codeword/step */
> + data_size = ecc->size - ((ecc->steps - 1) << 2);
> + oob_size = (ecc->steps << 2) + ecc->bytes;
> +
> + /*
> + * the location of spare data in the oob buffer, we could also use
> + * ecc->layout.oobfree here
> + */
> + free_boff = ecc->bytes * (ecc->steps - 1);
> +
> + /* override new oob content to last codeword */
> + memcpy(this->data_buffer + data_size, oob + free_boff, oob_size);
> +
> + this->use_ecc = true;
> + set_address(this, this->cw_size * (ecc->steps - 1), page);
> + update_rw_regs(this, 1, false);
> +
> + config_cw_write_pre(this);
> + write_data_dma(this, FLASH_BUF_ACC, this->data_buffer,
> + data_size + oob_size);
> + config_cw_write_post(this);
> +
> + r = submit_descs(this);
> + if (r)
> + dev_err(this->dev, "failure to write oob\n");
> +
> + free_descs(this);
> +
> + chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
> +
> + status = chip->waitfunc(mtd, chip);

Should we wait if the submission failed?

> +
> + return status & NAND_STATUS_FAIL ? -EIO : 0;
> +}
> +
> +/* implements ecc->write_oob_raw(), used to write bad block marker flag */
> +static int qcom_nandc_write_oob_raw(struct mtd_info *mtd,
> + struct nand_chip *chip, int page)
> +{
> + struct qcom_nandc_data *this = chip->priv;
> + struct nand_ecc_ctrl *ecc = &chip->ecc;
> + uint8_t *oob = chip->oob_poi;
> + int start, length;
> + int r, status = 0;
> +
> + r = copy_last_cw(this, false, page);
> + if (r)
> + return r;
> +
> + clear_read_regs(this);
> +
> + /*
> + * writing raw oob has 2 parts, first the bad block region, then the
> + * actual free region
> + */
> + start = mtd->writesize - (this->cw_size * (ecc->steps - 1));
> + length = chip->options & NAND_BUSWIDTH_16 ? 2 : 1;
> +
> + memcpy(this->data_buffer + start, oob, length);
> +
> + oob += length;
> +
> + start = this->cw_data - (ecc->steps << 2) + 1;
> + length = ecc->steps << 2;
> +
> + memcpy(this->data_buffer + start, oob, length);
> +
> + /* prepare write */
> + this->use_ecc = false;
> + set_address(this, this->cw_size * (ecc->steps - 1), page);
> + update_rw_regs(this, 1, false);
> +
> + config_cw_write_pre(this);
> + write_data_dma(this, FLASH_BUF_ACC, this->data_buffer, this->cw_size);
> + config_cw_write_post(this);
> +
> + r = submit_descs(this);
> + if (r)
> + dev_err(this->dev, "failure to write updated oob\n");
> +
> + free_descs(this);
> +
> + chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
> +
> + status = chip->waitfunc(mtd, chip);

Ditto.

> +
> + return status & NAND_STATUS_FAIL ? -EIO : 0;
> +}
> +
> +/*
> + * the three functions below implement chip->read_byte(), chip->read_buf()
> + * and chip->write_buf() respectively. these aren't used for
> + * reading/writing page data, they are used for smaller data like reading
> + * id, status etc
> + */
> +static uint8_t qcom_nandc_read_byte(struct mtd_info *mtd)
> +{
> + struct nand_chip *chip = mtd->priv;
> + struct qcom_nandc_data *this = chip->priv;
> + uint8_t *buf = (uint8_t *) this->data_buffer;

isn't it already uint8_t?

> + uint8_t ret = 0x0;
> +
> + if (this->last_command == NAND_CMD_STATUS) {
> + ret = this->status;
> +
> + this->status = NAND_STATUS_READY | NAND_STATUS_WP;
> +
> + return ret;
> + }
> +
> + if (this->buf_start < this->buf_count)
> + ret = buf[this->buf_start++];
> +
> + return ret;
> +}
> +
[...]
> +
> +static int qcom_nandc_alloc(struct qcom_nandc_data *this)
> +{
> + int r;
> +
> + r = dma_set_coherent_mask(this->dev, DMA_BIT_MASK(32));
> + if (r) {
> + dev_err(this->dev, "failed to set DMA mask\n");
> + return r;
> + }
> +
> + /*
> + * we don't know the page size of the NAND chip yet, set the buffer size
> + * to 512 bytes for now, that's sufficient for reading ID or ONFI params
> + */
> + this->buf_size = SZ_512;
> +
> + this->data_buffer = devm_kzalloc(this->dev, this->buf_size, GFP_KERNEL);
> + if (!this->data_buffer)
> + return -ENOMEM;
> +
> + this->regs = devm_kzalloc(this->dev, sizeof(struct nandc_regs),

sizeof(*this->regs)?

> + GFP_KERNEL);
> + if (!this->regs)
> + return -ENOMEM;
> +
> + this->reg_read_buf = devm_kzalloc(this->dev, MAX_REG_RD * sizeof(u32),
> + GFP_KERNEL);

devm_kcalloc() and sizeof(*this->reg_read_buf) ?

> + if (!this->reg_read_buf)
> + return -ENOMEM;
> +
> + INIT_LIST_HEAD(&this->list);
> +
> + this->chan = dma_request_slave_channel(this->dev, "rxtx");
> + if (!this->chan) {
> + dev_err(this->dev, "failed to request slave channel\n");
> + return -ENODEV;
> + }
> +
> + return 0;
> +}
> +
> +static void qcom_nandc_unalloc(struct qcom_nandc_data *this)
> +{
> + dma_release_channel(this->chan);
> +}
> +
> +static int qcom_nandc_init(struct qcom_nandc_data *this)
> +{
> + struct mtd_info *mtd = &this->mtd;
> + struct nand_chip *chip = &this->chip;
> + struct mtd_part_parser_data ppdata = {};
> + int r;
> +
> + mtd->priv = chip;
> + mtd->name = "qcom-nandc";
> + mtd->owner = THIS_MODULE;
> +
> + chip->priv = this;
> +
> + chip->cmdfunc = qcom_nandc_command;
> + chip->select_chip = qcom_nandc_select_chip;
> + chip->read_byte = qcom_nandc_read_byte;
> + chip->read_buf = qcom_nandc_read_buf;
> + chip->write_buf = qcom_nandc_write_buf;
> +
> + chip->options |= NAND_NO_SUBPAGE_WRITE;
> + chip->bbt_options = NAND_BBT_ACCESS_BBM_RAW | NAND_BBT_USE_FLASH;
> +
> + if (this->bus_width == 16)
> + chip->options |= NAND_BUSWIDTH_16;
> +
> + qcom_nandc_pre_init(this);
> +
> + r = nand_scan_ident(mtd, 1, NULL);
> + if (r)
> + return r;
> +
> + r = qcom_nandc_ecc_init(this);
> + if (r)
> + return r;
> +
> + qcom_nandc_hw_post_init(this);
> +
> + r = nand_scan_tail(mtd);
> + if (r)
> + return r;
> +
> +

Weird double newline here.

> + ppdata.of_node = this->dev->of_node;
> + r = mtd_device_parse_register(mtd, NULL, &ppdata, NULL, 0);
> + if (r)
> + return r;
> +
> + return 0;

Could be simplified to return mtd_device_parse_register(...).

> +}
> +
[..]
> +
> +static int qcom_nandc_remove(struct platform_device *pdev)
> +{
> + struct qcom_nandc_data *this;
> +
> + this = platform_get_drvdata(pdev);
> + if (!this)
> + return -ENODEV;

This seems impossible, so why check for it?

> +
> + qcom_nandc_unalloc(this);
> +
> + clk_disable_unprepare(this->aon_clk);
> + clk_disable_unprepare(this->core_clk);
> +
> + return 0;
> +}
> +
> +static struct platform_driver qcom_nandc_driver = {
> + .driver = {
> + .name = "qcom-nandc",
> + .of_match_table = qcom_nandc_of_match,
> + },
> + .probe = qcom_nandc_probe,
> + .remove = qcom_nandc_remove,
> +};
> +module_platform_driver(qcom_nandc_driver);
> +
> +MODULE_AUTHOR("Archit Taneja <[email protected]>");
> +MODULE_DESCRIPTION("Qualcomm NAND Controller driver");
> +MODULE_LICENSE("GPL");

This should say GPL v2 explicitly.

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

2015-07-28 04:34:19

[permalink] [raw]

Subject: Re: [PATCH v2 2/5] mtd: nand: Qualcomm NAND controller driver

Hi,

On 07/25/2015 06:21 AM, Stephen Boyd wrote:
> On 07/21/2015 03:34 AM, Archit Taneja wrote:
>> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
>> index 5b2806a..31951fc 100644
>> --- a/drivers/mtd/nand/Kconfig
>> +++ b/drivers/mtd/nand/Kconfig
>> @@ -538,4 +538,11 @@ config MTD_NAND_HISI504
>> help
>> Enables support for NAND controller on Hisilicon SoC Hip04.
>>
>> +config MTD_NAND_QCOM
>> + tristate "Support for NAND on QCOM SoCs"
>> + depends on ARCH_QCOM && QCOM_ADM
>
> This is sort of annoying that the menu won't show up unless the ADM
> driver is also enabled (which would be in a completely different area of
> the configurator). Perhaps drop that requirement because it isn't
> required to build?

That makes sense. I'll drop the QCOM_ADM requirement.

>
>> + help
>> + Enables support for NAND flash chips on SoCs containing the EBI2 NAND
>> + controller. This controller is found on IPQ806x SoC.
>> +
>> endif # MTD_NAND
>> diff --git a/drivers/mtd/nand/qcom_nandc.c b/drivers/mtd/nand/qcom_nandc.c
>> new file mode 100644
>> index 0000000..51c284c
>> --- /dev/null
>> +++ b/drivers/mtd/nand/qcom_nandc.c
>> @@ -0,0 +1,2019 @@
>> +/*
>> + * Copyright (c) 2015, The Linux Foundation. All rights reserved.
>> + *
>> + * This software is licensed under the terms of the GNU General Public
>> + * License version 2, as published by the Free Software Foundation, and
>> + * may be copied, distributed, and modified under those terms.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/slab.h>
>> +#include <linux/interrupt.h>
>
> Where is this used?

It isn't. I'll remove it.

>
>> +#include <linux/bitops.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/dmaengine.h>
>> +#include <linux/module.h>
>> +#include <linux/mtd/nand.h>
>> +#include <linux/mtd/partitions.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/of_mtd.h>
>> +#include <linux/delay.h>
>> +
> [..]
>> +/*
>> + * the NAND controller performs reads/writes with ECC in 516 byte chunks.
>> + * the driver calls the chunks 'step' or 'codeword' interchangeably
>> + */
>> +#define NANDC_STEP_SIZE 512
>> +
>> +/*
>> + * the largest page size we support is 8K, this will have 16 steps/codewords
>> + * of 512 bytes each
>> + */
>> +#define MAX_NUM_STEPS (SZ_8K / NANDC_STEP_SIZE)
>> +
>> +/* we read at most 3 registers per codeword scan */
>> +#define MAX_REG_RD (3 * MAX_NUM_STEPS)
>> +
>> +/* ECC modes */
>> +#define ECC_NONE BIT(0)
>> +#define ECC_RS_4BIT BIT(1)
>> +#define ECC_BCH_4BIT BIT(2)
>> +#define ECC_BCH_8BIT BIT(3)
>> +
>> +struct desc_info {
>> + struct list_head list;
>> +
>> + enum dma_transfer_direction dir;
>> + struct scatterlist sgl;
>> + struct dma_async_tx_descriptor *dma_desc;
>> +};
>> +
>> +/*
>> + * holds the current register values that we want to write. acts as a contiguous
>> + * chunk of memory which we use to write the controller registers through DMA.
>> + */
>> +struct nandc_regs {
>> + u32 cmd;
>> + u32 addr0;
>> + u32 addr1;
>> + u32 chip_sel;
>> + u32 exec;
>> +
>> + u32 cfg0;
>> + u32 cfg1;
>> + u32 ecc_bch_cfg;
>> +
>> + u32 clrflashstatus;
>> + u32 clrreadstatus;
>> +
>> + u32 cmd1;
>> + u32 vld;
>> +
>> + u32 orig_cmd1;
>> + u32 orig_vld;
>> +
>> + u32 ecc_buf_cfg;
>> +};
>> +
>> +/*
>> + * @cmd_crci: ADM DMA CRCI for command flow control
>> + * @data_crci: ADM DMA CRCI for data flow control
>> + * @list: DMA descriptor list (list of desc_infos)
>> + * @dma_done: completion param to denote end of last
>> + * descriptor in the list
>> + * @data_buffer: our local DMA buffer for page read/writes,
>> + * used when we can't use the buffer provided
>> + * by upper layers directly
>> + * @buf_size/count/start: markers for chip->read_buf/write_buf functions
>> + * @reg_read_buf: buffer for reading register data via DMA
>> + * @reg_read_pos: marker for data read in reg_read_buf
>> + * @cfg0, cfg1, cfg0_raw..: NANDc register configurations needed for
>> + * ecc/non-ecc mode for the current nand flash
>> + * device
>> + * @regs: a contiguous chunk of memory for DMA register
>> + * writes
>> + * @ecc_strength: 4 bit or 8 bit ecc, received via DT
>> + * @bus_width: 8 bit or 16 bit NAND bus width, received via DT
>> + * @ecc_modes: supported ECC modes by the current controller,
>> + * initialized via DT match data
>> + * @cw_size: the number of bytes in a single step/codeword
>> + * of a page, consisting of all data, ecc, spare
>> + * and reserved bytes
>> + * @cw_data: the number of bytes within a codeword protected
>> + * by ECC
>> + * @bch_enabled: flag to tell whether BCH or RS ECC mode is used
>> + * @status: value to be returned if NAND_CMD_STATUS command
>> + * is executed
>> + */
>> +struct qcom_nandc_data {
>> + struct platform_device *pdev;
>> + struct device *dev;
>> +
>> + void __iomem *base;
>> + struct resource *res;
>> +
>> + struct clk *core_clk;
>> + struct clk *aon_clk;
>> +
>> + /* DMA stuff */
>> + struct dma_chan *chan;
>> + struct dma_slave_config slave_conf;
>> + unsigned int cmd_crci;
>> + unsigned int data_crci;
>> + struct list_head list;
>> + struct completion dma_done;
>> +
>> + /* MTD stuff */
>> + struct nand_chip chip;
>> + struct mtd_info mtd;
>> +
>> + /* local data buffer and markers */
>> + u8 *data_buffer;
>> + int buf_size;
>> + int buf_count;
>> + int buf_start;
>> +
>> + /* local buffer to read back registers */
>> + u32 *reg_read_buf;
>> + dma_addr_t reg_read_paddr;
>
> Is this used?

No, it isn't. It's a leftover from when I used dma_alloc_coherent for
this buffer. Will remove it.

>
>> + int reg_read_pos;
>> +
>> + /* required configs */
>> + u32 cfg0, cfg1;
>> + u32 cfg0_raw, cfg1_raw;
>> + u32 ecc_buf_cfg;
>> + u32 ecc_bch_cfg;
>> + u32 clrflashstatus;
>> + u32 clrreadstatus;
>> + u32 sflashc_burst_cfg;
>> + u32 cmd1, vld;
>> +
>> + /* register state */
>> + struct nandc_regs *regs;
>> +
>> + /* things we get from DT */
>> + int ecc_strength;
>> + int bus_width;
>> +
>> + u32 ecc_modes;
>> +
>> + /* misc params */
>> + int cw_size;
>> + int cw_data;
>> + bool use_ecc;
>> + bool bch_enabled;
>> + u8 status;
>> + int last_command;
>> +};
>> +
>> +static inline unsigned int nandc_read(struct qcom_nandc_data *this, int offset)
>> +{
>> + return ioread32(this->base + offset);
>> +}
>> +
>> +static inline void nandc_write(struct qcom_nandc_data *this, int offset,
>> + unsigned int val)
>> +{
>> + iowrite32(val, this->base + offset);
>> +}
>
> Perhaps these should take and return u32s to match the signature of
> ioread32/iowrite32

I'll fix this.

>
>> +
>> +/* helper to configure address register values */
>> +static void set_address(struct qcom_nandc_data *this, u16 column, int page)
>> +{
>> + struct nand_chip *chip = &this->chip;
>> + struct nandc_regs *regs = this->regs;
>> +
>> + if (chip->options & NAND_BUSWIDTH_16)
>> + column >>= 1;
>> +
>> + regs->addr0 = page << 16 | column;
>> + regs->addr1 = page >> 16 & 0xff;
>> +}
>> +
>> +/*
>> + * update_rw_regs: set up read/write register values, these will be
>> + * written to the NAND controller registers via DMA
>> + *
>> + * @num_cw: number of steps for the read/write operation
>> + * @read: read or write operation
>> + */
>> +static void update_rw_regs(struct qcom_nandc_data *this, int num_cw, bool read)
>> +{
>> + struct nandc_regs *regs = this->regs;
>> +
>> + if (this->use_ecc) {
>> + if (read)
>> + regs->cmd = PAGE_READ_WITH_ECC | PAGE_ACC | LAST_PAGE;
>> + else
>> + regs->cmd = PROGRAM_PAGE | PAGE_ACC | LAST_PAGE;
>> +
>> + regs->cfg0 = (this->cfg0 & ~(7U << CW_PER_PAGE)) |
>> + (num_cw - 1) << CW_PER_PAGE;
>> +
>> + regs->cfg1 = this->cfg1;
>> + regs->ecc_bch_cfg = this->ecc_bch_cfg;
>> + } else {
>> + if (read)
>> + regs->cmd = PAGE_READ | PAGE_ACC | LAST_PAGE;
>> + else
>> + regs->cmd = PROGRAM_PAGE | PAGE_ACC | LAST_PAGE;
>> +
>> + regs->cfg0 = (this->cfg0_raw & ~(7U << CW_PER_PAGE)) |
>> + (num_cw - 1) << CW_PER_PAGE;
>> +
>> + regs->cfg1 = this->cfg1_raw;
>> + regs->ecc_bch_cfg = 1 << ECC_CFG_ECC_DISABLE;
>> + }
>
> These two arms are almost exactly the same, except regs->cmd has
> PAGE_READ_WITH_ECC vs PAGE_READ and regs->ecc_bch_cfg is different. It
> should be possible to push the use_ecc case down into the two places
> that need it and reduce lines.

cfg0 and cfg1 are set differently too, they need to be set to cfg0_raw
and cfg1_raw when use_ecc isn't set.

The only one line that I can save is the extra common PROGRAM_PAGE
assignment that's same irrespective of use_ecc. I'll do that.

>
>> +
>> + regs->ecc_buf_cfg = this->ecc_buf_cfg;
>> + regs->clrflashstatus = this->clrflashstatus;
>> + regs->clrreadstatus = this->clrreadstatus;
>> + regs->exec = 1;
>> +}
>> +
>> +/*
>> + * write_reg_dma: prepares a descriptor to write a given number of
>> + * contiguous registers
>> + *
>> + * @first: offset of the first register in the contiguous block
>> + * @reg: starting address containing the reg values to write
>> + * @num_regs: number of registers to write
>> + * @flow_control: flow control enabled/disabled
>> + */
>> +static int write_reg_dma(struct qcom_nandc_data *this, int first, u32 *reg,
>> + int num_regs, bool flow_control)
>> +{
>> + struct desc_info *desc;
>> + struct dma_async_tx_descriptor *dma_desc;
>> + struct scatterlist *sgl;
>> + int size;
>> + int r;
>> +
>> + desc = kzalloc(sizeof(*desc), GFP_KERNEL);
>> + if (!desc)
>> + return -ENOMEM;
>> +
>> + list_add_tail(&desc->list, &this->list);
>> +
>> + sgl = &desc->sgl;
>> +
>> + size = num_regs * sizeof(u32);
>> +
>> + sg_init_one(sgl, reg, size);
>> +
>> + desc->dir = DMA_MEM_TO_DEV;
>> +
>> + dma_map_sg(this->dev, sgl, 1, desc->dir);
>> +
>> + this->slave_conf.device_fc = flow_control ? 1 : 0;
>> + this->slave_conf.dst_addr = this->res->start + first;
>> + this->slave_conf.dst_maxburst = 16;
>> + this->slave_conf.slave_id = this->cmd_crci;
>> +
>> + r = dmaengine_slave_config(this->chan, &this->slave_conf);
>> + if (r) {
>> + dev_err(this->dev, "failed to configure dma channel\n");
>> + goto err;
>> + }
>> +
>> + dma_desc = dmaengine_prep_slave_sg(this->chan, sgl, 1, desc->dir, 0);
>> + if (!dma_desc) {
>> + dev_err(this->dev, "failed to prepare register write desc\n");
>> + r = PTR_ERR(dma_desc);
>
> PTR_ERR(NULL) doesn't make sense. It would be nice if the DMA engine
> APIs were documented so we knew if it returned NULL or an error pointer
> on failure.

Yes, this is wrong. I have to fix this habit of mixing error pointer and
NULL return values. Will fix it.

>
>> + goto err;
>> + }
>> +
>> + desc->dma_desc = dma_desc;
>> +
>> + return 0;
>> +err:
>> + kfree(desc);
>> +
>> + return r;
>> +}
>> +
>> +/*
>
> BTW, this isn't kernel doc. That would need two asterisks.

I didn't intend them to be a part of the kernel doc. I kept a kernel
doc like format at a few places since it was legible.

>
>> + * read_reg_dma: prepares a descriptor to read a given number of
>> + * contiguous registers to the reg_read_buf pointer
>> + *
>> + * @first: offset of the first register in the contiguous block
>> + * @num_regs: number of registers to read
>> + * @flow_control: flow control enabled/disabled
>> + */
>> +static int read_reg_dma(struct qcom_nandc_data *this, int first, int num_regs,
>> + bool flow_control)
>> +{
>> + struct desc_info *desc;
>> + struct dma_async_tx_descriptor *dma_desc;
>> + struct scatterlist *sgl;
>> + int size;
>> + int r;
>> +
>> + desc = kzalloc(sizeof(*desc), GFP_KERNEL);
>> + if (!desc)
>> + return -ENOMEM;
>> +
>> + list_add_tail(&desc->list, &this->list);
>> +
>> + sgl = &desc->sgl;
>> +
>> + size = num_regs * sizeof(u32);
>> +
>> + sg_init_one(sgl, this->reg_read_buf + this->reg_read_pos, size);
>> +
>> + desc->dir = DMA_DEV_TO_MEM;
>> +
>> + dma_map_sg(this->dev, sgl, 1, desc->dir);
>> +
>> + this->slave_conf.device_fc = flow_control ? 1 : 0;
>
> device_fc is a bool, so why the 1 : 0 trick? flow_control is already bool.

I'll fix this.

>
>> + this->slave_conf.src_addr = this->res->start + first;
>> + this->slave_conf.src_maxburst = 16;
>> + this->slave_conf.slave_id = this->data_crci;
>> +
>> + r = dmaengine_slave_config(this->chan, &this->slave_conf);
>> + if (r) {
>> + dev_err(this->dev, "failed to configure dma channel\n");
>> + goto err;
>> + }
>> +
>> + dma_desc = dmaengine_prep_slave_sg(this->chan, sgl, 1, desc->dir, 0);
>> + if (!dma_desc) {
>> + dev_err(this->dev, "failed to prepare register read desc\n");
>> + r = PTR_ERR(dma_desc);
>
> Same problem here for PTR_ERR. Would be nice to figure out a way to
> consolidate these DMA functions. They're only subtly different.

I'll try this out.

>
>> + goto err;
>> + }
>> +
>> + desc->dma_desc = dma_desc;
>> +
>> + this->reg_read_pos += num_regs;
>> +
>> + return 0;
>> +err:
>> + kfree(desc);
>> +
>> + return r;
>> +}
>> +
> [..]
>> +
>> +/*
>> + * write_data_dma: prepares a DMA descriptor to transfer data from
>> + * 'vaddr' to the controller's internal buffer
>> + *
>> + * @reg_off: offset within the controller's data buffer
>> + * @vaddr: virtual address of the buffer we want to read from
>> + * @size: DMA transaction size in bytes
>> + */
>> +static int write_data_dma(struct qcom_nandc_data *this, int reg_off, u8 *vaddr,
>
> const u8 *vaddr?
>
>> + int size)
>> +{
>> + struct desc_info *desc;
>> + struct dma_async_tx_descriptor *dma_desc;
>> + struct scatterlist *sgl;
>> + int r;
>> +
>> + desc = kzalloc(sizeof(*desc), GFP_KERNEL);
>> + if (!desc)
>> + return -ENOMEM;
>> +
>> + list_add_tail(&desc->list, &this->list);
>> +
>> + sgl = &desc->sgl;
>> +
>> + sg_init_one(sgl, vaddr, size);
>> +
>> + desc->dir = DMA_MEM_TO_DEV;
>> +
>> + r = dma_map_sg(this->dev, sgl, 1, desc->dir);
>> + if (r == 0)
>> + goto err;
>
> Should we return an error in this case? Looks like return 0.

dma_map_sg returns the number of sg entries successfully mapped. In
this case, it should be 1.

>
>> +
>> + this->slave_conf.device_fc = 0;
>> + this->slave_conf.dst_addr = this->res->start + reg_off;
>> + this->slave_conf.dst_maxburst = 16;
>
> Is there any reason why slave_conf can't be on the stack? Otherwise it's
> odd that it's overwritten a few times before we submit the descriptors,
> so it must be copied by the dmaengine provider, but that isn't clear at
> all from the code. If it isn't copied, perhaps it should be part of the
> desc_info structure. If it is copied I wonder why it isn't const in the
> function signature.

The dmaengine drivers either memcpy slave_config in their
device_config() dmaengine op, or populate their local members reading
params in the passed slave_config.

I'll move slave_conf to stack. As you said, the config argument
in dmaengine_slave_config should ideally use const.

>
>> +
>> + r = dmaengine_slave_config(this->chan, &this->slave_conf);
>> + if (r) {
>> + dev_err(this->dev, "failed to configure dma channel\n");
>> + goto err;
>> + }
>> +
>> + dma_desc = dmaengine_prep_slave_sg(this->chan, sgl, 1, desc->dir, 0);
>> + if (!dma_desc) {
>> + dev_err(this->dev, "failed to prepare data write desc\n");
>> + r = PTR_ERR(dma_desc);
>> + goto err;
>> + }
>> +
>> + desc->dma_desc = dma_desc;
>> +
>> + return 0;
>> +err:
>> + kfree(desc);
>> +
>> + return r;
>> +}
>> +
>> +/*
>> + * helper to prepare dma descriptors to configure registers needed for reading a
>> + * codeword/step in a page
>> + */
>> +static void config_cw_read(struct qcom_nandc_data *this)
>> +{
>> + struct nandc_regs *regs = this->regs;
>> +
>> + write_reg_dma(this, NAND_FLASH_CMD, &regs->cmd, 3, true);
>
> Maybe it would be better to have a case statement inside
> {write,read}_reg_dma() that looked at the second argument and matched it
> up with an offset in regs. Then this could be
>
> write_reg_dma(this, NAND_FLASH_CMD, 3, true);

That's a good idea. However, we have at least one programming seqeunce
(in nandc_param) where we need to write two different values to the same
register. In such a case, we need two different locations to store the
two values.

I can split up the programming sequence into two parts such that we
won't write to the same register twice. But doing this for the sake of
reducing an argument to write_reg_dma seems a bit unnecessary.

>
> and we wouldn't have to worry about having the wrong argument for
> parameter 2 and parameter 3. It may even be that we always read the same
> number of registers too? In which case we could move parameter 4 into
> the case statement too?

Parameter 4 can vary for the same starting register. It won't be
possible to move that.

The last parameter (flow_control) is actually limited to a few
registers, I will move that into the function.

>
>> + write_reg_dma(this, NAND_DEV0_CFG0, &regs->cfg0, 3, false);
>> + write_reg_dma(this, NAND_EBI2_ECC_BUF_CFG, &regs->ecc_buf_cfg,
>> + 1, false);
>> +
>> + write_reg_dma(this, NAND_EXEC_CMD, &regs->exec, 1, false);
>> +
>> + read_reg_dma(this, NAND_FLASH_STATUS, 2, true);
>> + read_reg_dma(this, NAND_ERASED_CW_DETECT_STATUS, 1, false);
>> +}
>> +
> [...]
>> +/* sets up descriptors for NAND_CMD_RESET */
>> +static int reset(struct qcom_nandc_data *this)
>> +{
>> + struct nandc_regs *regs = this->regs;
>> +
>> + regs->cmd = RESET_DEVICE;
>> + regs->exec = 1;
>> +
>> + write_reg_dma(this, NAND_FLASH_CMD, &regs->cmd, 1, true);
>> + write_reg_dma(this, NAND_EXEC_CMD, &regs->exec, 1, false);
>> +
>> + read_reg_dma(this, NAND_FLASH_STATUS, 1, true);
>> +
>> + return 0;
>> +}
>> +
>> +/* helpers to submit/free our list of dma descriptors */
>> +static void dma_callback(void *param)
>> +{
>> + struct qcom_nandc_data *this = (struct qcom_nandc_data *) param;
>
> Useless cast.

Will fix it.

>
>> + struct completion *c = &this->dma_done;
>> +
>> + complete(c);
>> +}
>> +
>> +static int submit_descs(struct qcom_nandc_data *this)
>> +{
>> + struct completion *c = &this->dma_done;
>> + struct desc_info *desc;
>> + int r;
>> +
>> + init_completion(c);
>> +
>> + list_for_each_entry(desc, &this->list, list) {
>> + /*
>> + * we add a callback the last descriptor in our list to notify
>
> to the last?

Yes. I'll fix this.

>
>> + * completion of command
>> + */
>> + if (list_is_last(&desc->list, &this->list)) {
>> + desc->dma_desc->callback = dma_callback;
>> + desc->dma_desc->callback_param = this;
>> + }
>> +
>> + dmaengine_submit(desc->dma_desc);
>> + }
>> +
>> + dma_async_issue_pending(this->chan);
>> +
>> + r = wait_for_completion_timeout(c, msecs_to_jiffies(500));
>> + if (!r)
>> + return -EINVAL;
>
> Why not -ETIMEDOUT?

I'll replace this with -ETIMEDOUT

>
>> +
>> + return 0;
>> +}
>> +
> [...]
>> +
>> +/*
>> + * when using RS ECC, the NAND controller flags an error when reading an
>> + * erased page. however, there are special characters at certain offsets when
>> + * we read the erased page. we check here if the page is really empty. if so,
>> + * we replace the magic characters with 0xffs
>> + */
>> +static bool empty_page_fixup(struct qcom_nandc_data *this, u8 *data_buf)
>> +{
>> + struct mtd_info *mtd = &this->mtd;
>> + struct nand_chip *chip = &this->chip;
>> + struct nand_ecc_ctrl *ecc = &chip->ecc;
>> + int cwperpage = ecc->steps;
>> + int i;
>> +
>> + /* if BCH is enabled, HW will take care of detecting erased pages */
>> + if (this->bch_enabled || !this->use_ecc)
>> + return false;
>> +
>> + for (i = 0; i < cwperpage; i++) {
>> + u8 *empty1, *empty2;
>> + u32 flash_status = this->reg_read_buf[3 * i];
>> +
>> + /*
>> + * an erased page flags an error in NAND_FLASH_STATUS, check if
>> + * the page is erased by looking for 0x54s at offsets 3 and 175
>> + * from the beginning of each codeword
>> + */
>> + if (flash_status & FS_OP_ERR) {
>> + empty1 = &data_buf[3 + i * this->cw_data];
>> + empty2 = &data_buf[175 + i * this->cw_data];
>> +
>> + /*
>> + * the error wasn't because of an erased page, bail out
>> + * and let someone else do the error checking
>> + */
>> + if (!((*empty1 == 0x54 && *empty2 == 0xff) ||
>> + (*empty1 == 0xff && *empty2 == 0x54)))
>
> Why are we using pointers? Just use u8 empty1, empty2 = data_buf[...] ?

Yes. Not sure why I did this.
>
>> + return false;
>> + }
>> + }
>> +
>> + for (i = 0; i < mtd->writesize && (data_buf[i] == 0xff ||
>> + (i % this->cw_data == 3 || i % this->cw_data == 175)); i++) {
>> + }
>
> This might be clearer like so:
>
> for (i = 0; i < mtd->writesize; i++) {
> if (i % this->cw_data == 3 || i % this->cw_data == 175);
> continue;
> if (data_buf[i] != 0xff)
> return false;
> }
>
> and then drop the if after the loop. Actually since we're checking the
> whole page it may be better to do this:
>
> +
> + /*
> + * the whole page is 0xffs besides the magic offsets, we replace the
> + * 0x54s with 0xffs
> + */
> + for (i = 0; i < cwperpage; i++) {
> + data_buf[3 + i * this->cw_data] = 0xff;
> + data_buf[175 + i * this->cw_data] = 0xff;
> + }
> +
>
>
> and then
>
> return memchr_inv(data_buf, 0xff, mtd->writesize) ? false : true ;
>
> the memchr_inv() is optimized to find the bad characters 8 bytes at a time.

That's a good idea. Although, if memchr_inv doesn't report that the
buffer comprises entirely of 0xffs, we will need to revert the magic
offsets back to their original values, since in that case, 0x54 could
have been valid data.

I guess it'll still be faster than the linear check. I'll try to change it.

>
>> +
>> + if (i < mtd->writesize)
>> + return false;
>> +
>> + /*
>> + * the whole page is 0xffs besides the magic offsets, we replace the
>> + * 0x54s with 0xffs
>> + */
>> + for (i = 0; i < cwperpage; i++) {
>> + data_buf[3 + i * this->cw_data] = 0xff;
>> + data_buf[175 + i * this->cw_data] = 0xff;
>> + }
>> +
>> + /*
>> + * tell the caller that the page was empty and is fixed up, so that
>> + * parse_read_errors() doesn't think it's and error
>> + */
>> + return true;
>> +}
>> +
>> +/*
>> + * reads back status registers set by the controller to notify page read
>> + * errors. this is equivalent to what 'ecc->correct()' would do.
>> + */
>> +static int parse_read_errors(struct qcom_nandc_data *this, bool erased_page)
>> +{
>> + struct mtd_info *mtd = &this->mtd;
>> + struct nand_chip *chip = &this->chip;
>> + struct nand_ecc_ctrl *ecc = &chip->ecc;
>> + int cwperpage = ecc->steps;
>> + unsigned int max_bitflips = 0;
>> + int i;
>> +
>> + for (i = 0; i < cwperpage; i++) {
>> + int stat;
>> + u32 flash_status = this->reg_read_buf[3 * i];
>> + u32 buffer_status = this->reg_read_buf[3 * i + 1];
>> + u32 erased_cw_status = this->reg_read_buf[3 * i + 2];
>
> struct stats {
> u32 flash;
> u32 buffer;
> u32 erased_cw;
> };
>
> struct stats *buf = (struct stat *)this->reg_read_buf;
>
> for (i = 0; i < cwperpage; i++, buf++) {
> int stat;
> if (buf->flash & (FS_OP_ERR | FS_MPU_ERR))
>
> ?

This does look neat. I'll switch to this.

>
> Also, is the buffer little endian? If so, that should be a __le32 flash,
> __le32 buffer, etc. up there and then le32_to_cpu().

It is. I'll update.

>
>> +
>> + if (flash_status & (FS_OP_ERR | FS_MPU_ERR)) {
>> +
>> + /* ignore erased codeword errors */
>> + if (this->bch_enabled) {
>> + if ((erased_cw_status & ERASED_CW) == ERASED_CW)
>> + continue;
>> + } else if (erased_page) {
>> + continue;
>> + }
>> +
>> + if (buffer_status & BS_UNCORRECTABLE_BIT) {
>> + mtd->ecc_stats.failed++;
>> + continue;
>> + }
>> + }
>> +
>> + stat = buffer_status & BS_CORRECTABLE_ERR_MSK;
>> + mtd->ecc_stats.corrected += stat;
>> +
>> + max_bitflips = max_t(unsigned int, max_bitflips, stat);
>> + }
>> +
>> + return max_bitflips;
>> +}
>> +
>> +/*
>> + * helper to perform the actual page read operation, used by ecc->read_page()
>> + * and ecc->read_oob()
>> + */
>> +static int read_page_low(struct qcom_nandc_data *this, u8 *data_buf,
>> + u8 *oob_buf)
>> +{
>> + struct nand_chip *chip = &this->chip;
>> + struct nand_ecc_ctrl *ecc = &chip->ecc;
>> + int i, r;
>> +
>> + /* queue cmd descs for each codeword */
>> + for (i = 0; i < ecc->steps; i++) {
>> + int data_size, oob_size;
>> +
>> + if (i == (ecc->steps - 1)) {
>> + data_size = ecc->size - ((ecc->steps - 1) << 2);
>> + oob_size = (ecc->steps << 2) + ecc->bytes;
>> + } else {
>> + data_size = this->cw_data;
>> + oob_size = ecc->bytes;
>> + }
>> +
>> + config_cw_read(this);
>> +
>> + if (data_buf)
>> + read_data_dma(this, FLASH_BUF_ACC, data_buf, data_size);
>> +
>> + if (oob_buf)
>> + read_data_dma(this, FLASH_BUF_ACC + data_size, oob_buf,
>> + oob_size);
>> +
>> + if (data_buf)
>> + data_buf += data_size;
>> + if (oob_buf)
>> + oob_buf += oob_size;
>> + }
>> +
>> + r = submit_descs(this);
>> + if (r)
>> + dev_err(this->dev, "failure to read page/oob\n");
>> +
>> + free_descs(this);
>> +
>> + return r;
>> +}
>> +
>> +/*
>> + * a helper that copies the last step/codeword of a page (containing free oob)
>> + * into our local buffer
>> + */
>> +static int copy_last_cw(struct qcom_nandc_data *this, bool use_ecc, int page)
>> +{
>> + struct nand_chip *chip = &this->chip;
>> + struct nand_ecc_ctrl *ecc = &chip->ecc;
>> + int size;
>> + int r;
>> +
>> + clear_read_regs(this);
>> +
>> + size = use_ecc ? this->cw_data : this->cw_size;
>> +
>> + /* prepare a clean read buffer */
>> + memset(this->data_buffer, 0xff, size);
>> +
>> + this->use_ecc = use_ecc;
>> + set_address(this, this->cw_size * (ecc->steps - 1), page);
>> + update_rw_regs(this, 1, true);
>> +
>> + config_cw_read(this);
>> +
>> + read_data_dma(this, FLASH_BUF_ACC, this->data_buffer, size);
>> +
>> + r = submit_descs(this);
>> + if (r)
>> + dev_err(this->dev, "failed to copy last codeword\n");
>> +
>> + free_descs(this);
>> +
>> + return r;
>> +}
>> +
>> +/* implements ecc->read_page() */
>> +static int qcom_nandc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
>> + uint8_t *buf, int oob_required, int page)
>> +{
>> + struct qcom_nandc_data *this = chip->priv;
>> + u8 *data_buf, *oob_buf = NULL;
>> + bool direct_dma, erased_page;
>> + int r;
>> +
>> + /*
>> + * first try to map the upper buffer directly, else, use our own buf
>> + * and memcpy to upper buf
>> + */
>> + if (virt_addr_valid(buf) && !object_is_on_stack(buf)) {
>> + direct_dma = true;
>> + data_buf = buf;
>> + } else {
>> + direct_dma = false;
>> + data_buf = this->data_buffer;
>> + memset(data_buf, 0xff, mtd->writesize + mtd->oobsize);
>> + }
>> +
>> + oob_buf = oob_required ? chip->oob_poi : NULL;
>> +
>> + r = read_page_low(this, data_buf, oob_buf);
>> + if (r) {
>> + dev_err(this->dev, "failure to read page\n");
>> + goto err;
>
> return r?
>
>> + }
>> +
>> + erased_page = empty_page_fixup(this, data_buf);
>> +
>> + if (!direct_dma)
>> + memcpy(buf, this->data_buffer, mtd->writesize);
>> +
>> + r = parse_read_errors(this, erased_page);
>> +err:
>> + return r;
>
> and then return parse_read_errors()?

Will do this.

>
>> +}
>> +
>> +/* implements ecc->read_oob() */
>> +static int qcom_nandc_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
>> + int page)
>> +{
>> + struct qcom_nandc_data *this = chip->priv;
>> + struct nand_ecc_ctrl *ecc = &chip->ecc;
>> + int r;
>> +
>> + clear_read_regs(this);
>> +
>> + this->use_ecc = true;
>> + set_address(this, 0, page);
>> + update_rw_regs(this, ecc->steps, true);
>> +
>> + r = read_page_low(this, NULL, chip->oob_poi);
>> + if (r)
>> + dev_err(this->dev, "failure to read oob\n");
>> +
>> + return r;
>> +}
>> +
>> +/* implements ecc->read_oob_raw(), used to read the bad block marker flag */
>> +static int qcom_nandc_read_oob_raw(struct mtd_info *mtd, struct nand_chip *chip,
>> + int page)
>> +{
>> + struct qcom_nandc_data *this = chip->priv;
>> + struct nand_ecc_ctrl *ecc = &chip->ecc;
>> + uint8_t *oob = chip->oob_poi;
>> + int start, length;
>> + int r;
>> +
>> + /*
>> + * configure registers for a raw page read, the address is set to the
>> + * beginning of the last codeword, we don't care about reading ecc
>> + * portion of oob, just the free stuff
>> + */
>> + r = copy_last_cw(this, false, page);
>> + if (r)
>> + return r;
>> +
>> + /*
>> + * reading raw oob has 2 parts, first the bad block byte, then the
>> + * actual free oob region. perform a memcpy in two steps
>> + */
>> + start = mtd->writesize - (this->cw_size * (ecc->steps - 1));
>> + length = chip->options & NAND_BUSWIDTH_16 ? 2 : 1;
>> +
>> + memcpy(oob, this->data_buffer + start, length);
>> +
>> + oob += length;
>> +
>> + start = this->cw_data - (ecc->steps << 2) + 1;
>> + length = ecc->steps << 2;
>> +
>> + memcpy(oob, this->data_buffer + start, length);
>> +
>> + return 0;
>> +}
>> +
>> +/* implements ecc->write_page() */
>> +static int qcom_nandc_write_page(struct mtd_info *mtd, struct nand_chip *chip,
>> + const uint8_t *buf, int oob_required)
>> +{
>> + struct qcom_nandc_data *this = chip->priv;
>> + struct nand_ecc_ctrl *ecc = &chip->ecc;
>> + u8 *data_buf, *oob_buf;
>> + bool direct_dma;
>> + int i, r = 0;
>> +
>> + clear_read_regs(this);
>> +
>> + /*
>> + * first try to map the upper buffer directly, else, memcpy upper
>> + * buffer to our local buffer
>> + */
>> + direct_dma = virt_addr_valid(buf) && !object_is_on_stack((void *) buf);
>
> We should change object_is_on_stack() to take a const pointer.

Yeah. That makes it two funcs. This and dmaengine_slave_config.

>
> Are we guaranteed that this is called within the same context as where
> the buffer is passed to this function? Otherwise this stack check isn't
> going to work because object_is_on_stack() will silently fail.

These are funcs are finally tied to mtd ops. I think in normal operation
it'll be the same context. I'll still cross check. The aim of the check
is to prevent a memcpy of the buffer to/from the layer above. A false
negative will result in a slower read/write operation.

>
>> +
>> + if (!direct_dma) {
>> + memcpy(this->data_buffer, buf, mtd->writesize
>> );
>> + data_buf = this->data_buffer;
>> + } else {
>> + data_buf = (u8 *) buf;
>
> Shouldn't need to cast away const...

Right. I'll remove this.

>
>> + }
>> +
>> + oob_buf = chip->oob_poi;
>> +
>> + this->use_ecc = true;
>> + update_rw_regs(this, ecc->steps, false);
>> +
>> + for (i = 0; i < ecc->steps; i++) {
>> + int data_size, oob_size;
>> +
>> + if (i == (ecc->steps - 1)) {
>> + data_size = ecc->size - ((ecc->steps - 1) << 2);
>> + oob_size = (ecc->steps << 2) + ecc->bytes;
>> + } else {
>> + data_size = this->cw_data;
>> + oob_size = ecc->bytes;
>> + }
>> +
>> + config_cw_write_pre(this);
>> + write_data_dma(this, FLASH_BUF_ACC, data_buf, data_size);
>> +
>> + /*
>> + * we don't really need to write anything to oob for the
>> + * first n - 1 codewords since these oob regions just
>> + * contain ecc that's written by the controller itself
>> + */
>> + if (i == (ecc->steps - 1))
>> + write_data_dma(this, FLASH_BUF_ACC + data_size,
>> + oob_buf, oob_size);
>> + config_cw_write_post(this);
>> +
>> + data_buf += data_size;
>> + oob_buf += oob_size;
>> + }
>> +
>> + r = submit_descs(this);
>> + if (r)
>> + dev_err(this->dev, "failure to write page\n");
>> +
>> + free_descs(this);
>> +
>> + return r;
>> +}
>> +
>> +/*
>> + * implements ecc->write_oob()
>> + *
>> + * the NAND controller cannot write only data or only oob within a codeword,
>> + * since ecc is calculated for the combined codeword. we first copy the
>> + * entire contents for the last codeword(data + oob), replace the old oob
>> + * with the new one in chip->oob_poi, and then write the entire codeword.
>> + * this read-copy-write operation results in a slight perormance loss.
>> + */
>> +static int qcom_nandc_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
>> + int page)
>> +{
>> + struct qcom_nandc_data *this = chip->priv;
>> + struct nand_ecc_ctrl *ecc = &chip->ecc;
>> + uint8_t *oob = chip->oob_poi;
>> + int free_boff;
>> + int data_size, oob_size;
>> + int r, status = 0;
>> +
>> + r = copy_last_cw(this, true, page);
>> + if (r)
>> + return r;
>> +
>> + clear_read_regs(this);
>> +
>> + /* calculate the data and oob size for the last codeword/step */
>> + data_size = ecc->size - ((ecc->steps - 1) << 2);
>> + oob_size = (ecc->steps << 2) + ecc->bytes;
>> +
>> + /*
>> + * the location of spare data in the oob buffer, we could also use
>> + * ecc->layout.oobfree here
>> + */
>> + free_boff = ecc->bytes * (ecc->steps - 1);
>> +
>> + /* override new oob content to last codeword */
>> + memcpy(this->data_buffer + data_size, oob + free_boff, oob_size);
>> +
>> + this->use_ecc = true;
>> + set_address(this, this->cw_size * (ecc->steps - 1), page);
>> + update_rw_regs(this, 1, false);
>> +
>> + config_cw_write_pre(this);
>> + write_data_dma(this, FLASH_BUF_ACC, this->data_buffer,
>> + data_size + oob_size);
>> + config_cw_write_post(this);
>> +
>> + r = submit_descs(this);
>> + if (r)
>> + dev_err(this->dev, "failure to write oob\n");
>> +
>> + free_descs(this);
>> +
>> + chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
>> +
>> + status = chip->waitfunc(mtd, chip);
>
> Should we wait if the submission failed?

That's a good point. We don't need to call waitfunc if dma
itself failed. Will fix it.

>
>> +
>> + return status & NAND_STATUS_FAIL ? -EIO : 0;
>> +}
>> +
>> +/* implements ecc->write_oob_raw(), used to write bad block marker flag */
>> +static int qcom_nandc_write_oob_raw(struct mtd_info *mtd,
>> + struct nand_chip *chip, int page)
>> +{
>> + struct qcom_nandc_data *this = chip->priv;
>> + struct nand_ecc_ctrl *ecc = &chip->ecc;
>> + uint8_t *oob = chip->oob_poi;
>> + int start, length;
>> + int r, status = 0;
>> +
>> + r = copy_last_cw(this, false, page);
>> + if (r)
>> + return r;
>> +
>> + clear_read_regs(this);
>> +
>> + /*
>> + * writing raw oob has 2 parts, first the bad block region, then the
>> + * actual free region
>> + */
>> + start = mtd->writesize - (this->cw_size * (ecc->steps - 1));
>> + length = chip->options & NAND_BUSWIDTH_16 ? 2 : 1;
>> +
>> + memcpy(this->data_buffer + start, oob, length);
>> +
>> + oob += length;
>> +
>> + start = this->cw_data - (ecc->steps << 2) + 1;
>> + length = ecc->steps << 2;
>> +
>> + memcpy(this->data_buffer + start, oob, length);
>> +
>> + /* prepare write */
>> + this->use_ecc = false;
>> + set_address(this, this->cw_size * (ecc->steps - 1), page);
>> + update_rw_regs(this, 1, false);
>> +
>> + config_cw_write_pre(this);
>> + write_data_dma(this, FLASH_BUF_ACC, this->data_buffer, this->cw_size);
>> + config_cw_write_post(this);
>> +
>> + r = submit_descs(this);
>> + if (r)
>> + dev_err(this->dev, "failure to write updated oob\n");
>> +
>> + free_descs(this);
>> +
>> + chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
>> +
>> + status = chip->waitfunc(mtd, chip);
>
> Ditto.
>
>> +
>> + return status & NAND_STATUS_FAIL ? -EIO : 0;
>> +}
>> +
>> +/*
>> + * the three functions below implement chip->read_byte(), chip->read_buf()
>> + * and chip->write_buf() respectively. these aren't used for
>> + * reading/writing page data, they are used for smaller data like reading
>> + * id, status etc
>> + */
>> +static uint8_t qcom_nandc_read_byte(struct mtd_info *mtd)
>> +{
>> + struct nand_chip *chip = mtd->priv;
>> + struct qcom_nandc_data *this = chip->priv;
>> + uint8_t *buf = (uint8_t *) this->data_buffer;
>
> isn't it already uint8_t?

Yes.

>
>> + uint8_t ret = 0x0;
>> +
>> + if (this->last_command == NAND_CMD_STATUS) {
>> + ret = this->status;
>> +
>> + this->status = NAND_STATUS_READY | NAND_STATUS_WP;
>> +
>> + return ret;
>> + }
>> +
>> + if (this->buf_start < this->buf_count)
>> + ret = buf[this->buf_start++];
>> +
>> + return ret;
>> +}
>> +
> [...]
>> +
>> +static int qcom_nandc_alloc(struct qcom_nandc_data *this)
>> +{
>> + int r;
>> +
>> + r = dma_set_coherent_mask(this->dev, DMA_BIT_MASK(32));
>> + if (r) {
>> + dev_err(this->dev, "failed to set DMA mask\n");
>> + return r;
>> + }
>> +
>> + /*
>> + * we don't know the page size of the NAND chip yet, set the buffer size
>> + * to 512 bytes for now, that's sufficient for reading ID or ONFI params
>> + */
>> + this->buf_size = SZ_512;
>> +
>> + this->data_buffer = devm_kzalloc(this->dev, this->buf_size, GFP_KERNEL);
>> + if (!this->data_buffer)
>> + return -ENOMEM;
>> +
>> + this->regs = devm_kzalloc(this->dev, sizeof(struct nandc_regs),
>
> sizeof(*this->regs)?
>
>> + GFP_KERNEL);
>> + if (!this->regs)
>> + return -ENOMEM;
>> +
>> + this->reg_read_buf = devm_kzalloc(this->dev, MAX_REG_RD * sizeof(u32),
>> + GFP_KERNEL);
>
> devm_kcalloc() and sizeof(*this->reg_read_buf) ?

Sure.

>
>> + if (!this->reg_read_buf)
>> + return -ENOMEM;
>> +
>> + INIT_LIST_HEAD(&this->list);
>> +
>> + this->chan = dma_request_slave_channel(this->dev, "rxtx");
>> + if (!this->chan) {
>> + dev_err(this->dev, "failed to request slave channel\n");
>> + return -ENODEV;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void qcom_nandc_unalloc(struct qcom_nandc_data *this)
>> +{
>> + dma_release_channel(this->chan);
>> +}
>> +
>> +static int qcom_nandc_init(struct qcom_nandc_data *this)
>> +{
>> + struct mtd_info *mtd = &this->mtd;
>> + struct nand_chip *chip = &this->chip;
>> + struct mtd_part_parser_data ppdata = {};
>> + int r;
>> +
>> + mtd->priv = chip;
>> + mtd->name = "qcom-nandc";
>> + mtd->owner = THIS_MODULE;
>> +
>> + chip->priv = this;
>> +
>> + chip->cmdfunc = qcom_nandc_command;
>> + chip->select_chip = qcom_nandc_select_chip;
>> + chip->read_byte = qcom_nandc_read_byte;
>> + chip->read_buf = qcom_nandc_read_buf;
>> + chip->write_buf = qcom_nandc_write_buf;
>> +
>> + chip->options |= NAND_NO_SUBPAGE_WRITE;
>> + chip->bbt_options = NAND_BBT_ACCESS_BBM_RAW | NAND_BBT_USE_FLASH;
>> +
>> + if (this->bus_width == 16)
>> + chip->options |= NAND_BUSWIDTH_16;
>> +
>> + qcom_nandc_pre_init(this);
>> +
>> + r = nand_scan_ident(mtd, 1, NULL);
>> + if (r)
>> + return r;
>> +
>> + r = qcom_nandc_ecc_init(this);
>> + if (r)
>> + return r;
>> +
>> + qcom_nandc_hw_post_init(this);
>> +
>> + r = nand_scan_tail(mtd);
>> + if (r)
>> + return r;
>> +
>> +
>
> Weird double newline here.
>
>> + ppdata.of_node = this->dev->of_node;
>> + r = mtd_device_parse_register(mtd, NULL, &ppdata, NULL, 0);
>> + if (r)
>> + return r;
>> +
>> + return 0;
>
> Could be simplified to return mtd_device_parse_register(...).
>
>> +}
>> +
> [..]
>> +
>> +static int qcom_nandc_remove(struct platform_device *pdev)
>> +{
>> + struct qcom_nandc_data *this;
>> +
>> + this = platform_get_drvdata(pdev);
>> + if (!this)
>> + return -ENODEV;
>
> This seems impossible, so why check for it?

Will fix this.

>
>> +
>> + qcom_nandc_unalloc(this);
>> +
>> + clk_disable_unprepare(this->aon_clk);
>> + clk_disable_unprepare(this->core_clk);
>> +
>> + return 0;
>> +}
>> +
>> +static struct platform_driver qcom_nandc_driver = {
>> + .driver = {
>> + .name = "qcom-nandc",
>> + .of_match_table = qcom_nandc_of_match,
>> + },
>> + .probe = qcom_nandc_probe,
>> + .remove = qcom_nandc_remove,
>> +};
>> +module_platform_driver(qcom_nandc_driver);
>> +
>> +MODULE_AUTHOR("Archit Taneja <[email protected]>");
>> +MODULE_DESCRIPTION("Qualcomm NAND Controller driver");
>> +MODULE_LICENSE("GPL");
>
> This should say GPL v2 explicitly.

I'll update this.

Thanks for the thorough review!

Archit

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

2015-07-29 01:48:50

[permalink] [raw]

Subject: Re: [PATCH v2 2/5] mtd: nand: Qualcomm NAND controller driver

On 07/27/2015 09:34 PM, Archit Taneja wrote:
> Hi,
>
> On 07/25/2015 06:21 AM, Stephen Boyd wrote:
>> On 07/21/2015 03:34 AM, Archit Taneja wrote:
>>
>>> + int size)
>>> +{
>>> + struct desc_info *desc;
>>> + struct dma_async_tx_descriptor *dma_desc;
>>> + struct scatterlist *sgl;
>>> + int r;
>>> +
>>> + desc = kzalloc(sizeof(*desc), GFP_KERNEL);
>>> + if (!desc)
>>> + return -ENOMEM;
>>> +
>>> + list_add_tail(&desc->list, &this->list);
>>> +
>>> + sgl = &desc->sgl;
>>> +
>>> + sg_init_one(sgl, vaddr, size);
>>> +
>>> + desc->dir = DMA_MEM_TO_DEV;
>>> +
>>> + r = dma_map_sg(this->dev, sgl, 1, desc->dir);
>>> + if (r == 0)
>>> + goto err;
>>
>> Should we return an error in this case? Looks like return 0.
>
> dma_map_sg returns the number of sg entries successfully mapped. In
> this case, it should be 1.

Right, but this function returns 0 (success?) if we failed to map anything.

>
>>
>>> +
>>> + this->slave_conf.device_fc = 0;
>>> + this->slave_conf.dst_addr = this->res->start + reg_off;
>>> + this->slave_conf.dst_maxburst = 16;
>>
>> Is there any reason why slave_conf can't be on the stack? Otherwise it's
>> odd that it's overwritten a few times before we submit the descriptors,
>> so it must be copied by the dmaengine provider, but that isn't clear at
>> all from the code. If it isn't copied, perhaps it should be part of the
>> desc_info structure. If it is copied I wonder why it isn't const in the
>> function signature.
>
> The dmaengine drivers either memcpy slave_config in their
> device_config() dmaengine op, or populate their local members reading
> params in the passed slave_config.
>
> I'll move slave_conf to stack. As you said, the config argument
> in dmaengine_slave_config should ideally use const.

Cool, someone should send a patch.

>
>>
>>> +
>>> + r = dmaengine_slave_config(this->chan, &this->slave_conf);
>>> + if (r) {
>>> + dev_err(this->dev, "failed to configure dma channel\n");
>>> + goto err;
>>> + }
>>> +
>>> + dma_desc = dmaengine_prep_slave_sg(this->chan, sgl, 1,
>>> desc->dir, 0);
>>> + if (!dma_desc) {
>>> + dev_err(this->dev, "failed to prepare data write desc\n");
>>> + r = PTR_ERR(dma_desc);
>>> + goto err;
>>> + }
>>> +
>>> + desc->dma_desc = dma_desc;
>>> +
>>> + return 0;
>>> +err:
>>> + kfree(desc);
>>> +
>>> + return r;
>>> +}
>>> +
>>> +/*
>>> + * helper to prepare dma descriptors to configure registers needed
>>> for reading a
>>> + * codeword/step in a page
>>> + */
>>> +static void config_cw_read(struct qcom_nandc_data *this)
>>> +{
>>> + struct nandc_regs *regs = this->regs;
>>> +
>>> + write_reg_dma(this, NAND_FLASH_CMD, &regs->cmd, 3, true);
>>
>> Maybe it would be better to have a case statement inside
>> {write,read}_reg_dma() that looked at the second argument and matched it
>> up with an offset in regs. Then this could be
>>
>> write_reg_dma(this, NAND_FLASH_CMD, 3, true);
>
> That's a good idea. However, we have at least one programming seqeunce
> (in nandc_param) where we need to write two different values to the
> same register. In such a case, we need two different locations to
> store the two values.
>
> I can split up the programming sequence into two parts such that we
> won't write to the same register twice. But doing this for the sake of
> reducing an argument to write_reg_dma seems a bit unnecessary.
>

Or you could have two #defines that indicate the different usage? Like
NAND_CMD_FOO and NAND_CMD_BAR that both map to the same register but
uses different locations as storage.

>>
>> Are we guaranteed that this is called within the same context as where
>> the buffer is passed to this function? Otherwise this stack check isn't
>> going to work because object_is_on_stack() will silently fail.
>
> These are funcs are finally tied to mtd ops. I think in normal operation
> it'll be the same context. I'll still cross check. The aim of the check
> is to prevent a memcpy of the buffer to/from the layer above. A false
> negative will result in a slower read/write operation.

Right. It would be nice if the mtd layer was DMA aware and could
indicate to drivers that DMA on the buffer is allowable or not. Trying
to figure it out if the buffer is in lowmem after the buffer is mapped
is error prone, which is why not a lot of usage of object_is_on_stack()
exists. Honestly I'm confused, I thought the DMA APIs would "do the
right thing" when highmem was passed into the mapping APIs, but maybe
I'm wrong. I'll have to look.

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

2015-07-29 05:14:51

[permalink] [raw]

Subject: Re: [PATCH v2 2/5] mtd: nand: Qualcomm NAND controller driver

On 07/29/2015 07:18 AM, Stephen Boyd wrote:
> On 07/27/2015 09:34 PM, Archit Taneja wrote:
>> Hi,
>>
>> On 07/25/2015 06:21 AM, Stephen Boyd wrote:
>>> On 07/21/2015 03:34 AM, Archit Taneja wrote:
>>>
>>>> + int size)
>>>> +{Looks like a
>>>> + struct desc_info *desc;
>>>> + struct dma_async_tx_descriptor *dma_desc;
>>>> + struct scatterlist *sgl;
>>>> + int r;
>>>> +
>>>> + desc = kzalloc(sizeof(*desc), GFP_KERNEL);
>>>> + if (!desc)
>>>> + return -ENOMEM;
>>>> +
>>>> + list_add_tail(&desc->list, &this->list);
>>>> +
>>>> + sgl = &desc->sgl;
>>>> +
>>>> + sg_init_one(sgl, vaddr, size);
>>>> +
>>>> + desc->dir = DMA_MEM_TO_DEV;
>>>> +
>>>> + r = dma_map_sg(this->dev, sgl, 1, desc->dir);
>>>> + if (r == 0)
>>>> + goto err;
>>>
>>> Should we return an error in this case? Looks like return 0.
>>
>> dma_map_sg returns the number of sg entries successfully mapped. In
>> this case, it should be 1.
>
> Right, but this function returns 0 (success?) if we failed to map anything.

Yes. The return value is number of entries successfully mapped.
dma_map_sg is a macro that is replaced by dma_map_sg_attrs. Its comment
says:

"dma_maps_sg_attrs returns 0 on error and > 0 on success. It should
never return a value < 0."

>
>>
>>>
>>>> +
>>>> + this->slave_conf.device_fc = 0;
>>>> + this->slave_conf.dst_addr = this->res->start + reg_off;
>>>> + this->slave_conf.dst_maxburst = 16;
>>>
>>> Is there any reason why slave_conf can't be on the stack? Otherwise it's
>>> odd that it's overwritten a few times before we submit the descriptors,
>>> so it must be copied by the dmaengine provider, but that isn't clear at
>>> all from the code. If it isn't copied, perhaps it should be part of the
>>> desc_info structure. If it is copied I wonder why it isn't const in the
>>> function signature.
>>
>> The dmaengine drivers either memcpy slave_config in their
>> device_config() dmaengine op, or populate their local members reading
>> params in the passed slave_config.
>>
>> I'll move slave_conf to stack. As you said, the config argument
>> in dmaengine_slave_config should ideally use const.
>
> Cool, someone should send a patch.
>
>>
>>>
>>>> +
>>>> + r = dmaengine_slave_config(this->chan, &this->slave_conf);
>>>> + if (r) {
>>>> + dev_err(this->dev, "failed to configure dma channel\n");
>>>> + goto err;
>>>> + }
>>>> +
>>>> + dma_desc = dmaengine_prep_slave_sg(this->chan, sgl, 1,
>>>> desc->dir, 0);
>>>> + if (!dma_desc) {
>>>> + dev_err(this->dev, "failed to prepare data write desc\n");
>>>> + r = PTR_ERR(dma_desc);
>>>> + goto err;
>>>> + }
>>>> +
>>>> + desc->dma_desc = dma_desc;
>>>> +
>>>> + return 0;
>>>> +err:
>>>> + kfree(desc);
>>>> +
>>>> + return r;
>>>> +}
>>>> +
>>>> +/*
>>>> + * helper to prepare dma descriptors to configure registers needed
>>>> for reading a
>>>> + * codeword/step in a page
>>>> + */
>>>> +static void config_cw_read(struct qcom_nandc_data *this)
>>>> +{
>>>> + struct nandc_regs *regs = this->regs;
>>>> +
>>>> + write_reg_dma(this, NAND_FLASH_CMD, &regs->cmd, 3, true);
>>>
>>> Maybe it would be better to have a case statement inside
>>> {write,read}_reg_dma() that looked at the second argument and matched it
>>> up with an offset in regs. Then this could be
>>>
>>> write_reg_dma(this, NAND_FLASH_CMD, 3, true);
>>
>> That's a good idea. However, we have at least one programming seqeunce
>> (in nandc_param) where we need to write two different values to the
>> same register. In such a case, we need two different locations to
>> store the two values.
>>
>> I can split up the programming sequence into two parts such that we
>> won't write to the same register twice. But doing this for the sake of
>> reducing an argument to write_reg_dma seems a bit unnecessary.
>>
>
> Or you could have two #defines that indicate the different usage? Like
> NAND_CMD_FOO and NAND_CMD_BAR that both map to the same register but
> uses different locations as storage.

Yeah, that seems like a good option. I'll try this out.

>
>>>
>>> Are we guaranteed that this is called within the same context as where
>>> the buffer is passed to this function? Otherwise this stack check isn't
>>> going to work because object_is_on_stack() will silently fail.
>>
>> These are funcs are finally tied to mtd ops. I think in normal operation
>> it'll be the same context. I'll still cross check. The aim of the check
>> is to prevent a memcpy of the buffer to/from the layer above. A false
>> negative will result in a slower read/write operation.
>
> Right. It would be nice if the mtd layer was DMA aware and could
> indicate to drivers that DMA on the buffer is allowable or not. Trying
> to figure it out if the buffer is in lowmem after the buffer is mapped
> is error prone, which is why not a lot of usage of object_is_on_stack()
> exists. Honestly I'm confused, I thought the DMA APIs would "do the
> right thing" when highmem was passed into the mapping APIs, but maybe
> I'm wrong. I'll have to look.

It looks like NAND_USE_BOUNCE_BUFFER does just that. If we set this
flag, the nand_base layer provides a DMA-able buffer even if the
filesystem didn't.

No one was using this flag when I last checked. A new driver
brcmnand now uses it. I'll give this a try.

Archit

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

2015-07-29 18:34:03

[permalink] [raw]

Subject: Re: [PATCH v2 2/5] mtd: nand: Qualcomm NAND controller driver

On 07/29, Archit Taneja wrote:
> On 07/29/2015 07:18 AM, Stephen Boyd wrote:
> >On 07/27/2015 09:34 PM, Archit Taneja wrote:
> >>Hi,
> >>
> >>On 07/25/2015 06:21 AM, Stephen Boyd wrote:
> >>>On 07/21/2015 03:34 AM, Archit Taneja wrote:
> >>>
> >>>>+ int size)
> >>>>+{Looks like a
> >>>>+ struct desc_info *desc;
> >>>>+ struct dma_async_tx_descriptor *dma_desc;
> >>>>+ struct scatterlist *sgl;
> >>>>+ int r;
> >>>>+
> >>>>+ desc = kzalloc(sizeof(*desc), GFP_KERNEL);
> >>>>+ if (!desc)
> >>>>+ return -ENOMEM;
> >>>>+
> >>>>+ list_add_tail(&desc->list, &this->list);
> >>>>+
> >>>>+ sgl = &desc->sgl;
> >>>>+
> >>>>+ sg_init_one(sgl, vaddr, size);
> >>>>+
> >>>>+ desc->dir = DMA_MEM_TO_DEV;
> >>>>+
> >>>>+ r = dma_map_sg(this->dev, sgl, 1, desc->dir);
> >>>>+ if (r == 0)
> >>>>+ goto err;
> >>>
> >>>Should we return an error in this case? Looks like return 0.
> >>
> >>dma_map_sg returns the number of sg entries successfully mapped. In
> >>this case, it should be 1.
> >
> >Right, but this function returns 0 (success?) if we failed to map anything.
>
> Yes. The return value is number of entries successfully mapped.
> dma_map_sg is a macro that is replaced by dma_map_sg_attrs. Its
> comment
> says:
>
> "dma_maps_sg_attrs returns 0 on error and > 0 on success. It should
> never return a value < 0."

Yes, and so this function that calls dma_map_sg() is going to
return 0 to the caller when it didn't do what it was asked to do?

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

2015-07-30 06:53:55