* [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc
@ 2014-11-04 12:46 Zhou Wang
2014-11-04 12:47 ` [PATCH v4 1/2] mtd: hisilicon: add a new NAND controller driver for " Zhou Wang
` (4 more replies)
0 siblings, 5 replies; 26+ messages in thread
From: Zhou Wang @ 2014-11-04 12:46 UTC (permalink / raw)
To: David Woodhouse, Brian Norris, linux-mtd, devicetree
Cc: mark.rutland, Zhou Wang, pawel.moll, ijc+devicetree, linux-kernel,
haojian.zhuang, wangzhou1, robh+dt, xuwei5, galak, caizhiyong
This patchset adds the support for NAND controller of hisilicon hip04 Soc.
The NAND controller IP was developed by hisilicon and needs a new driver to
support it. This patchset is based on v3.18-rc1. I have tested that NAND flash
controller works fine in Hip04 D01 board.
Changes in v4:
- add mtd->dev.parent = &pdev->dev, thanks Frans Klaver.
Changes in v3:
- Modify code to eliminate some code style warnings.
- add ecc-bits input check.
- avoid using waterfall style in hisi_nfc_cmdfunc().
Changes in v2:
- Remove the patch for device tree, now patchset only has the driver and its
device tree binding documentation.
- Change the file name: hisi_nand.c to hisi504_nand.c.
Changes in v1:
- Remove callback functions out of struct hinfc_host, and call them directly
in relative functions.
- Change hinfc_read and hinfc_write from macros to inline functions.
- Instead of putting pointers, embed struct nand_chip and struct mtd_info in
struct hinfc_host directly.
- rewrite some unclear lines in device tree binding document, correct some
code style error.
Link on v3:
- https://lkml.org/lkml/2014/10/28/386
Link on v2:
- https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg750071.html
Link on v1:
- https://lkml.org/lkml/2014/7/15/198
Zhou Wang (2):
mtd: hisilicon: add a new NAND controller driver for hisilicon hip04 Soc
mtd: hisilicon: add device tree binding documentation
.../devicetree/bindings/mtd/hisi504-nand.txt | 40 +
drivers/mtd/nand/Kconfig | 5 +
drivers/mtd/nand/Makefile | 1 +
drivers/mtd/nand/hisi504_nand.c | 846 ++++++++++++++++++++
4 files changed, 892 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mtd/hisi504-nand.txt
create mode 100644 drivers/mtd/nand/hisi504_nand.c
--
1.7.9.5
^ permalink raw reply [flat|nested] 26+ messages in thread* [PATCH v4 1/2] mtd: hisilicon: add a new NAND controller driver for hisilicon hip04 Soc 2014-11-04 12:46 [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc Zhou Wang @ 2014-11-04 12:47 ` Zhou Wang 2014-11-30 9:35 ` Brian Norris 2014-12-01 9:25 ` Brian Norris 2014-11-04 12:47 ` [PATCH v4 2/2] mtd: hisilicon: add device tree binding documentation Zhou Wang ` (3 subsequent siblings) 4 siblings, 2 replies; 26+ messages in thread From: Zhou Wang @ 2014-11-04 12:47 UTC (permalink / raw) To: David Woodhouse, Brian Norris, linux-mtd, devicetree Cc: mark.rutland, Zhou Wang, pawel.moll, ijc+devicetree, linux-kernel, haojian.zhuang, wangzhou1, robh+dt, xuwei5, galak, caizhiyong Signed-off-by: Zhou Wang <wangzhou.bry@gmail.com> --- drivers/mtd/nand/Kconfig | 5 + drivers/mtd/nand/Makefile | 1 + drivers/mtd/nand/hisi504_nand.c | 846 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 852 insertions(+) create mode 100644 drivers/mtd/nand/hisi504_nand.c diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig index dd10646..e451a08 100644 --- a/drivers/mtd/nand/Kconfig +++ b/drivers/mtd/nand/Kconfig @@ -516,4 +516,9 @@ config MTD_NAND_XWAY Enables support for NAND Flash chips on Lantiq XWAY SoCs. NAND is attached to the External Bus Unit (EBU). +config MTD_NAND_HISI504 + tristate "Support for NAND controller on Hisilicon SoC Hip04" + help + Enables support for NAND controller on Hisilicon SoC Hip04. + endif # MTD_NAND diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile index 9c847e4..fb1b2e4 100644 --- a/drivers/mtd/nand/Makefile +++ b/drivers/mtd/nand/Makefile @@ -50,5 +50,6 @@ obj-$(CONFIG_MTD_NAND_JZ4740) += jz4740_nand.o obj-$(CONFIG_MTD_NAND_GPMI_NAND) += gpmi-nand/ obj-$(CONFIG_MTD_NAND_XWAY) += xway_nand.o obj-$(CONFIG_MTD_NAND_BCM47XXNFLASH) += bcm47xxnflash/ +obj-$(CONFIG_MTD_NAND_HISI504) += hisi504_nand.o nand-objs := nand_base.o nand_bbt.o nand_timings.o diff --git a/drivers/mtd/nand/hisi504_nand.c b/drivers/mtd/nand/hisi504_nand.c new file mode 100644 index 0000000..a169cd8 --- /dev/null +++ b/drivers/mtd/nand/hisi504_nand.c @@ -0,0 +1,846 @@ +/* + * Hisilicon NAND Flash controller driver + * + * Copyright © 2012-2014 HiSilicon Technologies Co., Ltd. + * http://www.hisilicon.com + * + * Author: Zhou Wang <wangzhou.bry@gmail.com> + * The initial developer of the original code is Zhiyong Cai + * <caizhiyong@huawei.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * 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/of.h> +#include <linux/of_mtd.h> +#include <linux/mtd/mtd.h> +#include <linux/sizes.h> +#include <linux/clk.h> +#include <linux/slab.h> +#include <linux/module.h> +#include <linux/delay.h> +#include <linux/interrupt.h> +#include <linux/mtd/nand.h> +#include <linux/dma-mapping.h> +#include <linux/platform_device.h> +#include <linux/mtd/partitions.h> + +#define HINFC504_MAX_CHIP (4) +#define HINFC504_W_LATCH (5) +#define HINFC504_R_LATCH (7) +#define HINFC504_RW_LATCH (3) + +#define HINFC504_NFC_TIMEOUT (2 * HZ) +#define HINFC504_NFC_DMA_TIMEOUT (5 * HZ) +#define HINFC504_CHIP_DELAY (25) + +#define HINFC504_REG_BASE_ADDRESS_LEN (0x100) +#define HINFC504_BUFFER_BASE_ADDRESS_LEN (2048 + 128) + +#define HINFC504_ADDR_CYCLE_MASK 0x4 + +#define HINFC504_CON 0x00 +#define HINFC504_CON_OP_MODE_NORMAL (1U << 0) +#define HINFC504_CON_PAGEISZE_SHIFT (1) +#define HINFC504_CON_PAGESIZE_MASK (0x07) +#define HINFC504_CON_BUS_WIDTH (1U << 4) +#define HINFC504_CON_READY_BUSY_SEL (1U << 8) +#define HINFC504_CON_ECCTYPE_SHIFT (9) +#define HINFC504_CON_ECCTYPE_MASK (0x07) + +#define HINFC504_PWIDTH 0x04 +#define SET_HINFC504_PWIDTH(_w_lcnt, _r_lcnt, _rw_hcnt) \ + ((_w_lcnt) | (((_r_lcnt) & 0x0F) << 4) | (((_rw_hcnt) & 0x0F) << 8)) + +#define HINFC504_CMD 0x0C +#define HINFC504_ADDRL 0x10 +#define HINFC504_ADDRH 0x14 +#define HINFC504_DATA_NUM 0x18 + +#define HINFC504_OP 0x1C +#define HINFC504_OP_READ_DATA_EN (1U << 1) +#define HINFC504_OP_WAIT_READY_EN (1U << 2) +#define HINFC504_OP_CMD2_EN (1U << 3) +#define HINFC504_OP_WRITE_DATA_EN (1U << 4) +#define HINFC504_OP_ADDR_EN (1U << 5) +#define HINFC504_OP_CMD1_EN (1U << 6) +#define HINFC504_OP_NF_CS_SHIFT (7) +#define HINFC504_OP_NF_CS_MASK (3) +#define HINFC504_OP_ADDR_CYCLE_SHIFT (9) +#define HINFC504_OP_ADDR_CYCLE_MASK (7) + +#define HINFC504_STATUS 0x20 +#define HINFC504_READY (1U << 0) + +#define HINFC504_INTEN 0x24 +#define HINFC504_INTEN_DMA (1U << 9) +#define HINFC504_INTEN_UE (1U << 6) +#define HINFC504_INTEN_CE (1U << 5) + +#define HINFC504_INTS 0x28 +#define HINFC504_INTS_DMA (1U << 9) +#define HINFC504_INTS_UE (1U << 6) +#define HINFC504_INTS_CE (1U << 5) + +#define HINFC504_INTCLR 0x2C +#define HINFC504_INTCLR_DMA (1U << 9) +#define HINFC504_INTCLR_UE (1U << 6) +#define HINFC504_INTCLR_CE (1U << 5) + +#define HINFC504_ECC_STATUS 0x5C +#define HINFC504_ECC_1_BIT_SHIFT 16 +#define HINFC504_ECC_16_BIT_SHIFT 12 + +#define HINFC504_DMA_CTRL 0x60 +#define HINFC504_DMA_CTRL_DMA_START (1U << 0) +#define HINFC504_DMA_CTRL_WE (1U << 1) +#define HINFC504_DMA_CTRL_DATA_AREA_EN (1U << 2) +#define HINFC504_DMA_CTRL_OOB_AREA_EN (1U << 3) +#define HINFC504_DMA_CTRL_BURST4_EN (1U << 4) +#define HINFC504_DMA_CTRL_BURST8_EN (1U << 5) +#define HINFC504_DMA_CTRL_BURST16_EN (1U << 6) +#define HINFC504_DMA_CTRL_ADDR_NUM_SHIFT (7) +#define HINFC504_DMA_CTRL_ADDR_NUM_MASK (1) +#define HINFC504_DMA_CTRL_CS_SHIFT (8) +#define HINFC504_DMA_CTRL_CS_MASK (0x03) + +#define HINFC504_DMA_ADDR_DATA 0x64 +#define HINFC504_DMA_ADDR_OOB 0x68 + +#define HINFC504_DMA_LEN 0x6C +#define HINFC504_DMA_LEN_OOB_SHIFT (16) +#define HINFC504_DMA_LEN_OOB_MASK (0xFFF) + +#define HINFC504_DMA_PARA 0x70 +#define HINFC504_DMA_PARA_DATA_RW_EN (1U << 0) +#define HINFC504_DMA_PARA_OOB_RW_EN (1U << 1) +#define HINFC504_DMA_PARA_DATA_EDC_EN (1U << 2) +#define HINFC504_DMA_PARA_OOB_EDC_EN (1U << 3) +#define HINFC504_DMA_PARA_DATA_ECC_EN (1U << 4) +#define HINFC504_DMA_PARA_OOB_ECC_EN (1U << 5) + +#define HINFC_VERSION 0x74 +#define HINFC504_LOG_READ_ADDR 0x7C +#define HINFC504_LOG_READ_LEN 0x80 + +#define HINFC504_NANDINFO_LEN 0x10 + +struct hinfc_host { + struct nand_chip chip; + struct mtd_info mtd; + struct device *dev; + void __iomem *iobase; + struct completion cmd_complete; + unsigned int offset; + unsigned int command; + int chipselect; + unsigned int addr_cycle; + unsigned int addr_value[2]; + unsigned int cache_addr_value[2]; + char *buffer; + dma_addr_t dma_buffer; + dma_addr_t dma_oob; + int version; + unsigned int ecc_bits; + unsigned int irq_status; /* interrupt status */ +}; + +static inline unsigned int hinfc_read(struct hinfc_host *host, unsigned int reg) +{ + return readl(host->iobase + reg); +} + +static inline void hinfc_write(struct hinfc_host *host, unsigned int value, + unsigned int reg) +{ + writel(value, host->iobase + reg); +} + +static void wait_controller_finished(struct hinfc_host *host) +{ + unsigned long timeout = jiffies + HINFC504_NFC_TIMEOUT; + int val; + + while (time_before(jiffies, timeout)) { + val = hinfc_read(host, HINFC504_STATUS); + if (host->command == NAND_CMD_ERASE2) { + /* nfc is ready */ + while (!(val & HINFC504_READY)) { + usleep_range(500, 1000); + val = hinfc_read(host, HINFC504_STATUS); + } + return; + } + + if (val & HINFC504_READY) + return; + } + + /* wait cmd timeout */ + dev_err(host->dev, "Wait NAND controller exec cmd timeout.\n"); +} + +static void hisi_nfc_dma_transfer(struct hinfc_host *host, int todev) +{ + struct mtd_info *mtd = &host->mtd; + struct nand_chip *chip = mtd->priv; + unsigned long val; + int ret; + + hinfc_write(host, host->dma_buffer, HINFC504_DMA_ADDR_DATA); + hinfc_write(host, host->dma_oob, HINFC504_DMA_ADDR_OOB); + + if (chip->ecc.mode == NAND_ECC_NONE) { + hinfc_write(host, ((mtd->oobsize & HINFC504_DMA_LEN_OOB_MASK) + << HINFC504_DMA_LEN_OOB_SHIFT), HINFC504_DMA_LEN); + + hinfc_write(host, HINFC504_DMA_PARA_DATA_RW_EN + | HINFC504_DMA_PARA_OOB_RW_EN, HINFC504_DMA_PARA); + } else { + hinfc_write(host, HINFC504_DMA_PARA_DATA_RW_EN + | HINFC504_DMA_PARA_OOB_RW_EN | HINFC504_DMA_PARA_DATA_EDC_EN + | HINFC504_DMA_PARA_OOB_EDC_EN | HINFC504_DMA_PARA_DATA_ECC_EN + | HINFC504_DMA_PARA_OOB_ECC_EN, HINFC504_DMA_PARA); + } + + val = (HINFC504_DMA_CTRL_DMA_START | HINFC504_DMA_CTRL_BURST4_EN + | HINFC504_DMA_CTRL_BURST8_EN | HINFC504_DMA_CTRL_BURST16_EN + | HINFC504_DMA_CTRL_DATA_AREA_EN | HINFC504_DMA_CTRL_OOB_AREA_EN + | ((host->addr_cycle == 4 ? 1 : 0) + << HINFC504_DMA_CTRL_ADDR_NUM_SHIFT) + | ((host->chipselect & HINFC504_DMA_CTRL_CS_MASK) + << HINFC504_DMA_CTRL_CS_SHIFT)); + + if (todev) + val |= HINFC504_DMA_CTRL_WE; + + hinfc_write(host, val, HINFC504_DMA_CTRL); + + init_completion(&host->cmd_complete); + ret = wait_for_completion_timeout(&host->cmd_complete, + HINFC504_NFC_DMA_TIMEOUT); + + if (!ret) { + dev_err(host->dev, "DMA operation(irq) timeout!\n"); + /* sanity check */ + val = hinfc_read(host, HINFC504_DMA_CTRL); + if (!(val & HINFC504_DMA_CTRL_DMA_START)) + dev_err(host->dev, "dma is already done but without irq ACK!"); + else + dev_err(host->dev, "dma is really timeout!"); + } +} + +static int hisi_nfc_send_cmd_pageprog(struct hinfc_host *host) +{ + host->addr_value[0] &= 0xffff0000; + + hinfc_write(host, host->addr_value[0], HINFC504_ADDRL); + hinfc_write(host, host->addr_value[1], HINFC504_ADDRH); + hinfc_write(host, NAND_CMD_PAGEPROG << 8 | NAND_CMD_SEQIN, + HINFC504_CMD); + + hisi_nfc_dma_transfer(host, 1); + + return 0; +} + +static int hisi_nfc_send_cmd_readstart(struct hinfc_host *host) +{ + struct mtd_info *mtd = &host->mtd; + + if ((host->addr_value[0] == host->cache_addr_value[0]) && + (host->addr_value[1] == host->cache_addr_value[1])) + return 0; + + host->addr_value[0] &= 0xffff0000; + + hinfc_write(host, host->addr_value[0], HINFC504_ADDRL); + hinfc_write(host, host->addr_value[1], HINFC504_ADDRH); + hinfc_write(host, NAND_CMD_READSTART << 8 | NAND_CMD_READ0, + HINFC504_CMD); + + hinfc_write(host, 0, HINFC504_LOG_READ_ADDR); + hinfc_write(host, mtd->writesize + mtd->oobsize, + HINFC504_LOG_READ_LEN); + + hisi_nfc_dma_transfer(host, 0); + + host->cache_addr_value[0] = host->addr_value[0]; + host->cache_addr_value[1] = host->addr_value[1]; + + return 0; +} + +static int hisi_nfc_send_cmd_erase(struct hinfc_host *host) +{ + hinfc_write(host, host->addr_value[0], HINFC504_ADDRL); + hinfc_write(host, (NAND_CMD_ERASE2 << 8) | NAND_CMD_ERASE1, + HINFC504_CMD); + + hinfc_write(host, HINFC504_OP_WAIT_READY_EN + | HINFC504_OP_CMD2_EN + | HINFC504_OP_CMD1_EN + | HINFC504_OP_ADDR_EN + | ((host->chipselect & HINFC504_OP_NF_CS_MASK) + << HINFC504_OP_NF_CS_SHIFT) + | ((host->addr_cycle & HINFC504_OP_ADDR_CYCLE_MASK) + << HINFC504_OP_ADDR_CYCLE_SHIFT), + HINFC504_OP); + + wait_controller_finished(host); + + return 0; +} + +static int hisi_nfc_send_cmd_readid(struct hinfc_host *host) +{ + hinfc_write(host, HINFC504_NANDINFO_LEN, HINFC504_DATA_NUM); + hinfc_write(host, NAND_CMD_READID, HINFC504_CMD); + hinfc_write(host, 0, HINFC504_ADDRL); + + hinfc_write(host, HINFC504_OP_CMD1_EN | HINFC504_OP_ADDR_EN + | HINFC504_OP_READ_DATA_EN + | ((host->chipselect & HINFC504_OP_NF_CS_MASK) + << HINFC504_OP_NF_CS_SHIFT) + | 1 << HINFC504_OP_ADDR_CYCLE_SHIFT, HINFC504_OP); + + wait_controller_finished(host); + + return 0; +} + +static int hisi_nfc_send_cmd_status(struct hinfc_host *host) +{ + hinfc_write(host, HINFC504_NANDINFO_LEN, HINFC504_DATA_NUM); + hinfc_write(host, NAND_CMD_STATUS, HINFC504_CMD); + hinfc_write(host, HINFC504_OP_CMD1_EN + | HINFC504_OP_READ_DATA_EN + | ((host->chipselect & HINFC504_OP_NF_CS_MASK) + << HINFC504_OP_NF_CS_SHIFT), + HINFC504_OP); + + wait_controller_finished(host); + + return 0; +} + +static int hisi_nfc_send_cmd_reset(struct hinfc_host *host, int chipselect) +{ + hinfc_write(host, NAND_CMD_RESET, HINFC504_CMD); + + hinfc_write(host, HINFC504_OP_CMD1_EN + | ((chipselect & HINFC504_OP_NF_CS_MASK) + << HINFC504_OP_NF_CS_SHIFT) + | HINFC504_OP_WAIT_READY_EN, + HINFC504_OP); + + wait_controller_finished(host); + + return 0; +} + +static void hisi_nfc_select_chip(struct mtd_info *mtd, int chipselect) +{ + struct nand_chip *chip = mtd->priv; + struct hinfc_host *host = chip->priv; + + if (chipselect < 0) + return; + + host->chipselect = chipselect; +} + +static uint8_t hisi_nfc_read_byte(struct mtd_info *mtd) +{ + struct nand_chip *chip = mtd->priv; + struct hinfc_host *host = chip->priv; + + if (host->command == NAND_CMD_STATUS) + return readb(chip->IO_ADDR_R); + + host->offset++; + + if (host->command == NAND_CMD_READID) + return readb(chip->IO_ADDR_R + host->offset - 1); + + return readb(host->buffer + host->offset - 1); +} + +static u16 hisi_nfc_read_word(struct mtd_info *mtd) +{ + struct nand_chip *chip = mtd->priv; + struct hinfc_host *host = chip->priv; + + host->offset += 2; + return readw(host->buffer + host->offset - 2); +} + +static void +hisi_nfc_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len) +{ + struct nand_chip *chip = mtd->priv; + struct hinfc_host *host = chip->priv; + + memcpy(host->buffer + host->offset, buf, len); + host->offset += len; +} + +static void hisi_nfc_read_buf(struct mtd_info *mtd, uint8_t *buf, int len) +{ + struct nand_chip *chip = mtd->priv; + struct hinfc_host *host = chip->priv; + + memcpy(buf, host->buffer + host->offset, len); + host->offset += len; +} + +static void set_addr(struct mtd_info *mtd, int column, int page_addr) +{ + struct nand_chip *chip = mtd->priv; + struct hinfc_host *host = chip->priv; + + host->addr_cycle = 0; + host->addr_value[0] = 0; + host->addr_value[1] = 0; + + /* Serially input address */ + if (column != -1) { + /* Adjust columns for 16 bit buswidth */ + if (chip->options & NAND_BUSWIDTH_16) + column >>= 1; + + host->addr_value[0] = column & 0xffff; + host->addr_cycle = 2; + } + if (page_addr != -1) { + host->addr_value[0] |= (page_addr & 0xffff) + << (host->addr_cycle * 8); + host->addr_cycle += 2; + /* One more address cycle for devices > 128MiB */ + if (chip->chipsize > (128 << 20)) { + host->addr_cycle += 1; + if (host->command == NAND_CMD_ERASE1) + host->addr_value[0] |= ((page_addr >> 16) & 0xff) << 16; + else + host->addr_value[1] |= ((page_addr >> 16) & 0xff); + } + } +} + +static void hisi_nfc_cmdfunc(struct mtd_info *mtd, unsigned command, int column, + int page_addr) +{ + struct nand_chip *chip = mtd->priv; + struct hinfc_host *host = chip->priv; + int is_cache_invalid = 1; + unsigned int flag = 0; + + host->command = command; + + switch (command) { + case NAND_CMD_READ0: + case NAND_CMD_READOOB: + if (command == NAND_CMD_READ0) + host->offset = column; + else + host->offset = column + mtd->writesize; + + is_cache_invalid = 0; + set_addr(mtd, column, page_addr); + hisi_nfc_send_cmd_readstart(host); + break; + + case NAND_CMD_SEQIN: + host->offset = column; + set_addr(mtd, column, page_addr); + break; + + case NAND_CMD_ERASE1: + set_addr(mtd, column, page_addr); + break; + + case NAND_CMD_PAGEPROG: + hisi_nfc_send_cmd_pageprog(host); + break; + + case NAND_CMD_ERASE2: + hisi_nfc_send_cmd_erase(host); + break; + + case NAND_CMD_READID: + host->offset = column; + memset(chip->IO_ADDR_R, 0, 0x10); + hisi_nfc_send_cmd_readid(host); + break; + + case NAND_CMD_STATUS: + flag = hinfc_read(host, HINFC504_CON); + if (chip->ecc.mode == NAND_ECC_HW) + hinfc_write(host, + flag && ~(HINFC504_CON_ECCTYPE_MASK << + HINFC504_CON_ECCTYPE_SHIFT), HINFC504_CON); + + host->offset = 0; + memset(chip->IO_ADDR_R, 0, 0x10); + hisi_nfc_send_cmd_status(host); + hinfc_write(host, flag, HINFC504_CON); + break; + + case NAND_CMD_RESET: + hisi_nfc_send_cmd_reset(host, host->chipselect); + break; + + default: + dev_err(host->dev, "Error: unsupported cmd(cmd=%x, col=%x, page=%x)\n", + command, column, page_addr); + } + + if (is_cache_invalid) { + host->cache_addr_value[0] = ~0; + host->cache_addr_value[1] = ~0; + } +} + +static irqreturn_t hinfc_irq_handle(int irq, void *devid) +{ + struct hinfc_host *host = devid; + unsigned int flag; + + flag = hinfc_read(host, HINFC504_INTS); + /* store interrupts state */ + host->irq_status |= flag; + + if (flag & HINFC504_INTS_DMA) { + hinfc_write(host, HINFC504_INTCLR_DMA, HINFC504_INTCLR); + complete(&host->cmd_complete); + } else if (flag & HINFC504_INTS_CE) { + hinfc_write(host, HINFC504_INTCLR_CE, HINFC504_INTCLR); + } else if (flag & HINFC504_INTS_UE) { + hinfc_write(host, HINFC504_INTCLR_UE, HINFC504_INTCLR); + } + + return IRQ_HANDLED; +} + +static int hisi_nand_read_page_hwecc(struct mtd_info *mtd, + struct nand_chip *chip, uint8_t *buf, int oob_required, int page) +{ + struct hinfc_host *host = chip->priv; + int max_bitflips = 0, stat = 0; + + chip->read_buf(mtd, buf, mtd->writesize); + chip->read_buf(mtd, chip->oob_poi, mtd->oobsize); + + /* errors which can not be corrected by ECC */ + if (host->irq_status & HINFC504_INTS_UE) { + mtd->ecc_stats.failed++; + } else if (host->irq_status & HINFC504_INTS_CE) { + /* need add other ECC modes! */ + switch (host->ecc_bits) { + case 1: + stat = hweight8(hinfc_read(host, HINFC504_ECC_STATUS)>> + HINFC504_ECC_1_BIT_SHIFT); + break; + case 6: + stat = hweight16(hinfc_read(host, HINFC504_ECC_STATUS)>> + HINFC504_ECC_16_BIT_SHIFT & 0x0fff); + } + mtd->ecc_stats.corrected += stat; + max_bitflips = max_t(unsigned int, max_bitflips, stat); + } + host->irq_status = 0; + + return max_bitflips; +} + +static int hisi_nand_write_page_hwecc(struct mtd_info *mtd, + struct nand_chip *chip, const uint8_t *buf, int oob_required) +{ + chip->write_buf(mtd, buf, mtd->writesize); + if (oob_required) + chip->write_buf(mtd, chip->oob_poi, mtd->oobsize); + + return 0; +} + +static void hisi_nfc_host_init(struct hinfc_host *host) +{ + struct nand_chip *chip = &host->chip; + unsigned int flag = 0; + + host->version = hinfc_read(host, HINFC_VERSION); + host->addr_cycle = 0; + host->addr_value[0] = 0; + host->addr_value[1] = 0; + host->cache_addr_value[0] = ~0; + host->cache_addr_value[1] = ~0; + host->chipselect = 0; + + /* default page size: 2K, ecc_none. need modify */ + flag = HINFC504_CON_OP_MODE_NORMAL | HINFC504_CON_READY_BUSY_SEL + | ((0x001 & HINFC504_CON_PAGESIZE_MASK) + << HINFC504_CON_PAGEISZE_SHIFT) + | ((0x0 & HINFC504_CON_ECCTYPE_MASK) + << HINFC504_CON_ECCTYPE_SHIFT) + | ((chip->options & NAND_BUSWIDTH_16) ? + HINFC504_CON_BUS_WIDTH : 0); + hinfc_write(host, flag, HINFC504_CON); + + memset(chip->IO_ADDR_R, 0xff, HINFC504_BUFFER_BASE_ADDRESS_LEN); + + hinfc_write(host, SET_HINFC504_PWIDTH(HINFC504_W_LATCH, + HINFC504_R_LATCH, HINFC504_RW_LATCH), HINFC504_PWIDTH); + + /* enable dma irq */ + hinfc_write(host, HINFC504_INTEN_DMA, HINFC504_INTEN); +} + +static struct nand_ecclayout nand_ecc_2K_1bit = { + .oobfree = { {24, 40} } +}; + +static struct nand_ecclayout nand_ecc_2K_16bits = { + .oobavail = 6, + .oobfree = { {2, 6} }, +}; + +static int hisi_nfc_ecc_probe(struct hinfc_host *host) +{ + struct nand_chip *chip = &host->chip; + unsigned int flag; + + chip->ecc.read_page = hisi_nand_read_page_hwecc; + chip->ecc.write_page = hisi_nand_write_page_hwecc; + + switch (host->ecc_bits) { + case 1: + chip->ecc.layout = &nand_ecc_2K_1bit; + chip->ecc.strength = 1; + chip->ecc.size = 512; + break; + case 6: + chip->ecc.layout = &nand_ecc_2K_16bits; + chip->ecc.strength = 16; + chip->ecc.size = 1024; + } + + flag = hinfc_read(host, HINFC504_CON); + /* add ecc type configure */ + flag |= ((host->ecc_bits & HINFC504_CON_ECCTYPE_MASK) + << HINFC504_CON_ECCTYPE_SHIFT); + hinfc_write(host, flag, HINFC504_CON); + + /* enable ecc irq */ + flag = hinfc_read(host, HINFC504_INTEN) & 0xfff; + hinfc_write(host, flag | HINFC504_INTEN_UE | HINFC504_INTEN_CE, + HINFC504_INTEN); + + return 0; +} + +static int hisi_nfc_probe(struct platform_device *pdev) +{ + int ret = 0, irq, buswidth, flag, max_chips = HINFC504_MAX_CHIP; + struct device *dev = &pdev->dev; + struct hinfc_host *host; + struct nand_chip *chip; + struct mtd_info *mtd; + struct resource *res; + struct device_node *np = dev->of_node; + struct mtd_part_parser_data ppdata; + + host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL); + if (!host) + return -ENOMEM; + host->dev = dev; + + platform_set_drvdata(pdev, host); + chip = &host->chip; + mtd = &host->mtd; + + irq = platform_get_irq(pdev, 0); + if (irq < 0) { + dev_err(dev, "no IRQ resource defined\n"); + ret = -ENXIO; + goto err_res; + } + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + host->iobase = devm_ioremap_resource(dev, res); + if (IS_ERR(host->iobase)) { + ret = PTR_ERR(host->iobase); + dev_err(dev, "devm_ioremap_resource[0] fail\n"); + goto err_res; + } + + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); + chip->IO_ADDR_R = chip->IO_ADDR_W = devm_ioremap_resource(dev, res); + if (IS_ERR(chip->IO_ADDR_R)) { + ret = PTR_ERR(chip->IO_ADDR_R); + dev_err(dev, "devm_ioremap_resource[1] fail\n"); + goto err_res; + } + + mtd->priv = chip; + mtd->owner = THIS_MODULE; + mtd->name = "hisi_nand"; + mtd->dev.parent = &pdev->dev; + + chip->priv = host; + chip->cmdfunc = hisi_nfc_cmdfunc; + chip->select_chip = hisi_nfc_select_chip; + chip->read_byte = hisi_nfc_read_byte; + chip->read_word = hisi_nfc_read_word; + chip->write_buf = hisi_nfc_write_buf; + chip->read_buf = hisi_nfc_read_buf; + chip->chip_delay = HINFC504_CHIP_DELAY; + + chip->ecc.mode = of_get_nand_ecc_mode(np); + /* read ecc-bits from dts */ + of_property_read_u32(np, "hisi,nand-ecc-bits", &host->ecc_bits); + if (host->ecc_bits != 0 && host->ecc_bits != 1 && host->ecc_bits != 6) { + ret = -EINVAL; + dev_err(dev, "invalid nand-ecc-bits: %u\n", host->ecc_bits); + goto err_res; + } + + buswidth = of_get_nand_bus_width(np); + if (buswidth == 16) + chip->options |= NAND_BUSWIDTH_16; + + hisi_nfc_host_init(host); + + ret = devm_request_irq(dev, irq, hinfc_irq_handle, IRQF_DISABLED, + "nandc", host); + if (ret) { + dev_err(dev, "failed to request IRQ\n"); + goto err_irq; + } + + ret = nand_scan_ident(mtd, max_chips, NULL); + if (ret) { + ret = -ENODEV; + goto err_ident; + } + + host->buffer = dma_alloc_coherent(dev, mtd->writesize + mtd->oobsize, + &host->dma_buffer, GFP_KERNEL); + if (!host->buffer) { + dev_err(dev, "Can't malloc memory for NAND driver.\n"); + ret = -ENOMEM; + goto err_buf; + } + host->dma_oob = host->dma_buffer + mtd->writesize; + memset(host->buffer, 0xff, mtd->writesize + mtd->oobsize); + + flag = hinfc_read(host, HINFC504_CON); + flag &= ~(HINFC504_CON_PAGESIZE_MASK << HINFC504_CON_PAGEISZE_SHIFT); + switch (mtd->writesize) { + case 2048: + flag |= (0x001 << HINFC504_CON_PAGEISZE_SHIFT); + /* add more pagesize support + * default pagesize has been set in hisi_nfc_host_init + */ + } + hinfc_write(host, flag, HINFC504_CON); + + if (chip->ecc.mode == NAND_ECC_HW) + hisi_nfc_ecc_probe(host); + + nand_scan_tail(mtd); + + ppdata.of_node = np; + ret = mtd_device_parse_register(mtd, NULL, &ppdata, NULL, 0); + if (ret) { + dev_err(dev, "Err MTD partition=%d\n", ret); + goto err_mtd; + } + + return 0; + +err_mtd: + nand_release(mtd); +err_ident: +err_irq: +err_buf: + if (host->buffer) + dma_free_coherent(dev, mtd->writesize + mtd->oobsize, + host->buffer, host->dma_buffer); +err_res: + return ret; +} + +static int hisi_nfc_remove(struct platform_device *pdev) +{ + struct hinfc_host *host = platform_get_drvdata(pdev); + struct mtd_info *mtd = &host->mtd; + + nand_release(mtd); + dma_free_coherent(&pdev->dev, mtd->writesize + mtd->oobsize, + host->buffer, host->dma_buffer); + + return 0; +} + +#ifdef CONFIG_PM_SLEEP +static int hisi_nfc_suspend(struct platform_device *pdev, + pm_message_t state) +{ + struct hinfc_host *host = platform_get_drvdata(pdev); + + while ((hinfc_read(host, HINFC504_STATUS) & 0x1) == 0x0) + ; + + while ((hinfc_read(host, HINFC504_DMA_CTRL)) + & HINFC504_DMA_CTRL_DMA_START) + _cond_resched(); + + return 0; +} + +static int hisi_nfc_resume(struct platform_device *pdev) +{ + int cs; + struct hinfc_host *host = platform_get_drvdata(pdev); + struct nand_chip *chip = &host->chip; + + for (cs = 0; cs < chip->numchips; cs++) + hisi_nfc_send_cmd_reset(host, cs); + hinfc_write(host, SET_HINFC504_PWIDTH(HINFC504_W_LATCH, + HINFC504_R_LATCH, HINFC504_RW_LATCH), HINFC504_PWIDTH); + + return 0; +} +#endif +static SIMPLE_DEV_PM_OPS(hisi_nfc_pm_ops, hisi_nfc_suspend, hisi_nfc_resume); + +static const struct of_device_id nfc_id_table[] = { + { .compatible = "hisilicon,504-nfc" }, + {} +}; +MODULE_DEVICE_TABLE(of, nfc_id_table); + +static struct platform_driver hisi_nfc_driver = { + .driver = { + .name = "hisi_nand", + .of_match_table = of_match_ptr(nfc_id_table), + .pm = &hisi_nfc_pm_ops, + }, + .probe = hisi_nfc_probe, + .remove = hisi_nfc_remove, +}; + +module_platform_driver(hisi_nfc_driver); + +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Zhiyong Cai"); +MODULE_AUTHOR("Zhou Wang"); +MODULE_DESCRIPTION("Hisilicon Nand Flash Controller Driver"); -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v4 1/2] mtd: hisilicon: add a new NAND controller driver for hisilicon hip04 Soc 2014-11-04 12:47 ` [PATCH v4 1/2] mtd: hisilicon: add a new NAND controller driver for " Zhou Wang @ 2014-11-30 9:35 ` Brian Norris 2014-12-01 13:08 ` Zhou Wang 2014-12-01 9:25 ` Brian Norris 1 sibling, 1 reply; 26+ messages in thread From: Brian Norris @ 2014-11-30 9:35 UTC (permalink / raw) To: Zhou Wang Cc: mark.rutland, devicetree, pawel.moll, ijc+devicetree, linux-kernel, haojian.zhuang, wangzhou1, robh+dt, linux-mtd, xuwei5, galak, caizhiyong, David Woodhouse On Tue, Nov 04, 2014 at 08:47:00PM +0800, Zhou Wang wrote: > Signed-off-by: Zhou Wang <wangzhou.bry@gmail.com> This driver mostly looks good. A few comments. > --- > drivers/mtd/nand/Kconfig | 5 + > drivers/mtd/nand/Makefile | 1 + > drivers/mtd/nand/hisi504_nand.c | 846 +++++++++++++++++++++++++++++++++++++++ > 3 files changed, 852 insertions(+) > create mode 100644 drivers/mtd/nand/hisi504_nand.c > > diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig > index dd10646..e451a08 100644 > --- a/drivers/mtd/nand/Kconfig > +++ b/drivers/mtd/nand/Kconfig > @@ -516,4 +516,9 @@ config MTD_NAND_XWAY > Enables support for NAND Flash chips on Lantiq XWAY SoCs. NAND is attached > to the External Bus Unit (EBU). > > +config MTD_NAND_HISI504 > + tristate "Support for NAND controller on Hisilicon SoC Hip04" > + help > + Enables support for NAND controller on Hisilicon SoC Hip04. > + > endif # MTD_NAND You'll want to rebase on l2-mtd.git when you resend. There are some small Kconfig and Makefile conflicts. (I can fix them myself, but if you're going to resend anyway...) > diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile > index 9c847e4..fb1b2e4 100644 > --- a/drivers/mtd/nand/Makefile > +++ b/drivers/mtd/nand/Makefile > @@ -50,5 +50,6 @@ obj-$(CONFIG_MTD_NAND_JZ4740) += jz4740_nand.o > obj-$(CONFIG_MTD_NAND_GPMI_NAND) += gpmi-nand/ > obj-$(CONFIG_MTD_NAND_XWAY) += xway_nand.o > obj-$(CONFIG_MTD_NAND_BCM47XXNFLASH) += bcm47xxnflash/ > +obj-$(CONFIG_MTD_NAND_HISI504) += hisi504_nand.o > > nand-objs := nand_base.o nand_bbt.o nand_timings.o > diff --git a/drivers/mtd/nand/hisi504_nand.c b/drivers/mtd/nand/hisi504_nand.c > new file mode 100644 > index 0000000..a169cd8 > --- /dev/null > +++ b/drivers/mtd/nand/hisi504_nand.c > @@ -0,0 +1,846 @@ > +/* > + * Hisilicon NAND Flash controller driver > + * > + * Copyright © 2012-2014 HiSilicon Technologies Co., Ltd. > + * http://www.hisilicon.com > + * > + * Author: Zhou Wang <wangzhou.bry@gmail.com> > + * The initial developer of the original code is Zhiyong Cai > + * <caizhiyong@huawei.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * 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/of.h> > +#include <linux/of_mtd.h> > +#include <linux/mtd/mtd.h> > +#include <linux/sizes.h> > +#include <linux/clk.h> > +#include <linux/slab.h> > +#include <linux/module.h> > +#include <linux/delay.h> > +#include <linux/interrupt.h> > +#include <linux/mtd/nand.h> > +#include <linux/dma-mapping.h> > +#include <linux/platform_device.h> > +#include <linux/mtd/partitions.h> > + > +#define HINFC504_MAX_CHIP (4) > +#define HINFC504_W_LATCH (5) > +#define HINFC504_R_LATCH (7) > +#define HINFC504_RW_LATCH (3) > + > +#define HINFC504_NFC_TIMEOUT (2 * HZ) > +#define HINFC504_NFC_DMA_TIMEOUT (5 * HZ) > +#define HINFC504_CHIP_DELAY (25) > + > +#define HINFC504_REG_BASE_ADDRESS_LEN (0x100) > +#define HINFC504_BUFFER_BASE_ADDRESS_LEN (2048 + 128) > + > +#define HINFC504_ADDR_CYCLE_MASK 0x4 > + > +#define HINFC504_CON 0x00 > +#define HINFC504_CON_OP_MODE_NORMAL (1U << 0) > +#define HINFC504_CON_PAGEISZE_SHIFT (1) > +#define HINFC504_CON_PAGESIZE_MASK (0x07) > +#define HINFC504_CON_BUS_WIDTH (1U << 4) > +#define HINFC504_CON_READY_BUSY_SEL (1U << 8) > +#define HINFC504_CON_ECCTYPE_SHIFT (9) > +#define HINFC504_CON_ECCTYPE_MASK (0x07) > + > +#define HINFC504_PWIDTH 0x04 > +#define SET_HINFC504_PWIDTH(_w_lcnt, _r_lcnt, _rw_hcnt) \ > + ((_w_lcnt) | (((_r_lcnt) & 0x0F) << 4) | (((_rw_hcnt) & 0x0F) << 8)) > + > +#define HINFC504_CMD 0x0C > +#define HINFC504_ADDRL 0x10 > +#define HINFC504_ADDRH 0x14 > +#define HINFC504_DATA_NUM 0x18 > + > +#define HINFC504_OP 0x1C > +#define HINFC504_OP_READ_DATA_EN (1U << 1) > +#define HINFC504_OP_WAIT_READY_EN (1U << 2) > +#define HINFC504_OP_CMD2_EN (1U << 3) > +#define HINFC504_OP_WRITE_DATA_EN (1U << 4) > +#define HINFC504_OP_ADDR_EN (1U << 5) > +#define HINFC504_OP_CMD1_EN (1U << 6) Might try the BIT() macros here. > +#define HINFC504_OP_NF_CS_SHIFT (7) > +#define HINFC504_OP_NF_CS_MASK (3) > +#define HINFC504_OP_ADDR_CYCLE_SHIFT (9) > +#define HINFC504_OP_ADDR_CYCLE_MASK (7) > + > +#define HINFC504_STATUS 0x20 > +#define HINFC504_READY (1U << 0) > + > +#define HINFC504_INTEN 0x24 > +#define HINFC504_INTEN_DMA (1U << 9) > +#define HINFC504_INTEN_UE (1U << 6) > +#define HINFC504_INTEN_CE (1U << 5) Same here, and in the next few blocks. > + > +#define HINFC504_INTS 0x28 > +#define HINFC504_INTS_DMA (1U << 9) > +#define HINFC504_INTS_UE (1U << 6) > +#define HINFC504_INTS_CE (1U << 5) > + > +#define HINFC504_INTCLR 0x2C > +#define HINFC504_INTCLR_DMA (1U << 9) > +#define HINFC504_INTCLR_UE (1U << 6) > +#define HINFC504_INTCLR_CE (1U << 5) > + > +#define HINFC504_ECC_STATUS 0x5C > +#define HINFC504_ECC_1_BIT_SHIFT 16 > +#define HINFC504_ECC_16_BIT_SHIFT 12 > + > +#define HINFC504_DMA_CTRL 0x60 > +#define HINFC504_DMA_CTRL_DMA_START (1U << 0) > +#define HINFC504_DMA_CTRL_WE (1U << 1) > +#define HINFC504_DMA_CTRL_DATA_AREA_EN (1U << 2) > +#define HINFC504_DMA_CTRL_OOB_AREA_EN (1U << 3) > +#define HINFC504_DMA_CTRL_BURST4_EN (1U << 4) > +#define HINFC504_DMA_CTRL_BURST8_EN (1U << 5) > +#define HINFC504_DMA_CTRL_BURST16_EN (1U << 6) > +#define HINFC504_DMA_CTRL_ADDR_NUM_SHIFT (7) > +#define HINFC504_DMA_CTRL_ADDR_NUM_MASK (1) > +#define HINFC504_DMA_CTRL_CS_SHIFT (8) > +#define HINFC504_DMA_CTRL_CS_MASK (0x03) > + > +#define HINFC504_DMA_ADDR_DATA 0x64 > +#define HINFC504_DMA_ADDR_OOB 0x68 > + > +#define HINFC504_DMA_LEN 0x6C > +#define HINFC504_DMA_LEN_OOB_SHIFT (16) > +#define HINFC504_DMA_LEN_OOB_MASK (0xFFF) > + > +#define HINFC504_DMA_PARA 0x70 > +#define HINFC504_DMA_PARA_DATA_RW_EN (1U << 0) > +#define HINFC504_DMA_PARA_OOB_RW_EN (1U << 1) > +#define HINFC504_DMA_PARA_DATA_EDC_EN (1U << 2) > +#define HINFC504_DMA_PARA_OOB_EDC_EN (1U << 3) > +#define HINFC504_DMA_PARA_DATA_ECC_EN (1U << 4) > +#define HINFC504_DMA_PARA_OOB_ECC_EN (1U << 5) > + > +#define HINFC_VERSION 0x74 > +#define HINFC504_LOG_READ_ADDR 0x7C > +#define HINFC504_LOG_READ_LEN 0x80 > + > +#define HINFC504_NANDINFO_LEN 0x10 > + > +struct hinfc_host { > + struct nand_chip chip; > + struct mtd_info mtd; > + struct device *dev; > + void __iomem *iobase; > + struct completion cmd_complete; > + unsigned int offset; > + unsigned int command; > + int chipselect; > + unsigned int addr_cycle; > + unsigned int addr_value[2]; It looks like you're using addr_value as 32-bit copies of the HW register. Might make sense to declare them as u32, since you care about the width. > + unsigned int cache_addr_value[2]; > + char *buffer; > + dma_addr_t dma_buffer; > + dma_addr_t dma_oob; > + int version; > + unsigned int ecc_bits; > + unsigned int irq_status; /* interrupt status */ > +}; > + > +static inline unsigned int hinfc_read(struct hinfc_host *host, unsigned int reg) > +{ > + return readl(host->iobase + reg); > +} > + > +static inline void hinfc_write(struct hinfc_host *host, unsigned int value, > + unsigned int reg) > +{ > + writel(value, host->iobase + reg); > +} > + > +static void wait_controller_finished(struct hinfc_host *host) > +{ > + unsigned long timeout = jiffies + HINFC504_NFC_TIMEOUT; > + int val; > + > + while (time_before(jiffies, timeout)) { > + val = hinfc_read(host, HINFC504_STATUS); > + if (host->command == NAND_CMD_ERASE2) { > + /* nfc is ready */ > + while (!(val & HINFC504_READY)) { > + usleep_range(500, 1000); > + val = hinfc_read(host, HINFC504_STATUS); > + } > + return; > + } > + > + if (val & HINFC504_READY) > + return; > + } > + > + /* wait cmd timeout */ > + dev_err(host->dev, "Wait NAND controller exec cmd timeout.\n"); > +} > + > +static void hisi_nfc_dma_transfer(struct hinfc_host *host, int todev) > +{ > + struct mtd_info *mtd = &host->mtd; > + struct nand_chip *chip = mtd->priv; > + unsigned long val; > + int ret; > + > + hinfc_write(host, host->dma_buffer, HINFC504_DMA_ADDR_DATA); > + hinfc_write(host, host->dma_oob, HINFC504_DMA_ADDR_OOB); > + > + if (chip->ecc.mode == NAND_ECC_NONE) { > + hinfc_write(host, ((mtd->oobsize & HINFC504_DMA_LEN_OOB_MASK) > + << HINFC504_DMA_LEN_OOB_SHIFT), HINFC504_DMA_LEN); > + > + hinfc_write(host, HINFC504_DMA_PARA_DATA_RW_EN > + | HINFC504_DMA_PARA_OOB_RW_EN, HINFC504_DMA_PARA); > + } else { > + hinfc_write(host, HINFC504_DMA_PARA_DATA_RW_EN > + | HINFC504_DMA_PARA_OOB_RW_EN | HINFC504_DMA_PARA_DATA_EDC_EN > + | HINFC504_DMA_PARA_OOB_EDC_EN | HINFC504_DMA_PARA_DATA_ECC_EN > + | HINFC504_DMA_PARA_OOB_ECC_EN, HINFC504_DMA_PARA); > + } > + > + val = (HINFC504_DMA_CTRL_DMA_START | HINFC504_DMA_CTRL_BURST4_EN > + | HINFC504_DMA_CTRL_BURST8_EN | HINFC504_DMA_CTRL_BURST16_EN > + | HINFC504_DMA_CTRL_DATA_AREA_EN | HINFC504_DMA_CTRL_OOB_AREA_EN > + | ((host->addr_cycle == 4 ? 1 : 0) > + << HINFC504_DMA_CTRL_ADDR_NUM_SHIFT) > + | ((host->chipselect & HINFC504_DMA_CTRL_CS_MASK) > + << HINFC504_DMA_CTRL_CS_SHIFT)); > + > + if (todev) > + val |= HINFC504_DMA_CTRL_WE; > + > + hinfc_write(host, val, HINFC504_DMA_CTRL); > + > + init_completion(&host->cmd_complete); You need to run init_completion() *before* you kick off your I/O. Otherwise, your interrupt handler is racing with this function. > + ret = wait_for_completion_timeout(&host->cmd_complete, > + HINFC504_NFC_DMA_TIMEOUT); > + > + if (!ret) { > + dev_err(host->dev, "DMA operation(irq) timeout!\n"); > + /* sanity check */ > + val = hinfc_read(host, HINFC504_DMA_CTRL); > + if (!(val & HINFC504_DMA_CTRL_DMA_START)) > + dev_err(host->dev, "dma is already done but without irq ACK!"); You're missing a newline at the end of this message. Please check your other prints too. Also, s/dma/DMA/ > + else > + dev_err(host->dev, "dma is really timeout!"); > + } > +} > + > +static int hisi_nfc_send_cmd_pageprog(struct hinfc_host *host) > +{ > + host->addr_value[0] &= 0xffff0000; > + > + hinfc_write(host, host->addr_value[0], HINFC504_ADDRL); > + hinfc_write(host, host->addr_value[1], HINFC504_ADDRH); > + hinfc_write(host, NAND_CMD_PAGEPROG << 8 | NAND_CMD_SEQIN, > + HINFC504_CMD); > + > + hisi_nfc_dma_transfer(host, 1); > + > + return 0; > +} > + > +static int hisi_nfc_send_cmd_readstart(struct hinfc_host *host) > +{ > + struct mtd_info *mtd = &host->mtd; > + > + if ((host->addr_value[0] == host->cache_addr_value[0]) && > + (host->addr_value[1] == host->cache_addr_value[1])) > + return 0; > + > + host->addr_value[0] &= 0xffff0000; > + > + hinfc_write(host, host->addr_value[0], HINFC504_ADDRL); > + hinfc_write(host, host->addr_value[1], HINFC504_ADDRH); > + hinfc_write(host, NAND_CMD_READSTART << 8 | NAND_CMD_READ0, > + HINFC504_CMD); > + > + hinfc_write(host, 0, HINFC504_LOG_READ_ADDR); > + hinfc_write(host, mtd->writesize + mtd->oobsize, > + HINFC504_LOG_READ_LEN); > + > + hisi_nfc_dma_transfer(host, 0); > + > + host->cache_addr_value[0] = host->addr_value[0]; > + host->cache_addr_value[1] = host->addr_value[1]; > + > + return 0; > +} > + > +static int hisi_nfc_send_cmd_erase(struct hinfc_host *host) > +{ > + hinfc_write(host, host->addr_value[0], HINFC504_ADDRL); > + hinfc_write(host, (NAND_CMD_ERASE2 << 8) | NAND_CMD_ERASE1, > + HINFC504_CMD); > + > + hinfc_write(host, HINFC504_OP_WAIT_READY_EN > + | HINFC504_OP_CMD2_EN > + | HINFC504_OP_CMD1_EN > + | HINFC504_OP_ADDR_EN > + | ((host->chipselect & HINFC504_OP_NF_CS_MASK) > + << HINFC504_OP_NF_CS_SHIFT) > + | ((host->addr_cycle & HINFC504_OP_ADDR_CYCLE_MASK) > + << HINFC504_OP_ADDR_CYCLE_SHIFT), > + HINFC504_OP); > + > + wait_controller_finished(host); > + > + return 0; > +} > + > +static int hisi_nfc_send_cmd_readid(struct hinfc_host *host) > +{ > + hinfc_write(host, HINFC504_NANDINFO_LEN, HINFC504_DATA_NUM); > + hinfc_write(host, NAND_CMD_READID, HINFC504_CMD); > + hinfc_write(host, 0, HINFC504_ADDRL); > + > + hinfc_write(host, HINFC504_OP_CMD1_EN | HINFC504_OP_ADDR_EN > + | HINFC504_OP_READ_DATA_EN > + | ((host->chipselect & HINFC504_OP_NF_CS_MASK) > + << HINFC504_OP_NF_CS_SHIFT) > + | 1 << HINFC504_OP_ADDR_CYCLE_SHIFT, HINFC504_OP); > + > + wait_controller_finished(host); > + > + return 0; > +} > + > +static int hisi_nfc_send_cmd_status(struct hinfc_host *host) > +{ > + hinfc_write(host, HINFC504_NANDINFO_LEN, HINFC504_DATA_NUM); > + hinfc_write(host, NAND_CMD_STATUS, HINFC504_CMD); > + hinfc_write(host, HINFC504_OP_CMD1_EN > + | HINFC504_OP_READ_DATA_EN > + | ((host->chipselect & HINFC504_OP_NF_CS_MASK) > + << HINFC504_OP_NF_CS_SHIFT), > + HINFC504_OP); > + > + wait_controller_finished(host); > + > + return 0; > +} > + > +static int hisi_nfc_send_cmd_reset(struct hinfc_host *host, int chipselect) > +{ > + hinfc_write(host, NAND_CMD_RESET, HINFC504_CMD); > + > + hinfc_write(host, HINFC504_OP_CMD1_EN > + | ((chipselect & HINFC504_OP_NF_CS_MASK) > + << HINFC504_OP_NF_CS_SHIFT) > + | HINFC504_OP_WAIT_READY_EN, > + HINFC504_OP); > + > + wait_controller_finished(host); > + > + return 0; > +} > + > +static void hisi_nfc_select_chip(struct mtd_info *mtd, int chipselect) > +{ > + struct nand_chip *chip = mtd->priv; > + struct hinfc_host *host = chip->priv; > + > + if (chipselect < 0) > + return; > + > + host->chipselect = chipselect; > +} > + > +static uint8_t hisi_nfc_read_byte(struct mtd_info *mtd) > +{ > + struct nand_chip *chip = mtd->priv; > + struct hinfc_host *host = chip->priv; > + > + if (host->command == NAND_CMD_STATUS) > + return readb(chip->IO_ADDR_R); > + > + host->offset++; > + > + if (host->command == NAND_CMD_READID) > + return readb(chip->IO_ADDR_R + host->offset - 1); > + > + return readb(host->buffer + host->offset - 1); > +} > + > +static u16 hisi_nfc_read_word(struct mtd_info *mtd) > +{ > + struct nand_chip *chip = mtd->priv; > + struct hinfc_host *host = chip->priv; > + > + host->offset += 2; > + return readw(host->buffer + host->offset - 2); > +} > + > +static void > +hisi_nfc_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len) > +{ > + struct nand_chip *chip = mtd->priv; > + struct hinfc_host *host = chip->priv; > + > + memcpy(host->buffer + host->offset, buf, len); > + host->offset += len; > +} > + > +static void hisi_nfc_read_buf(struct mtd_info *mtd, uint8_t *buf, int len) > +{ > + struct nand_chip *chip = mtd->priv; > + struct hinfc_host *host = chip->priv; > + > + memcpy(buf, host->buffer + host->offset, len); > + host->offset += len; > +} > + > +static void set_addr(struct mtd_info *mtd, int column, int page_addr) > +{ > + struct nand_chip *chip = mtd->priv; > + struct hinfc_host *host = chip->priv; > + > + host->addr_cycle = 0; > + host->addr_value[0] = 0; > + host->addr_value[1] = 0; > + > + /* Serially input address */ > + if (column != -1) { > + /* Adjust columns for 16 bit buswidth */ > + if (chip->options & NAND_BUSWIDTH_16) > + column >>= 1; It doesn't look like you're supporting ONFI parameter pages yet, but you might consider checking for !nand_opcode_8bits(opcode) before adjusting the address. We had to fix some other drivers for this recently. > + > + host->addr_value[0] = column & 0xffff; > + host->addr_cycle = 2; > + } > + if (page_addr != -1) { > + host->addr_value[0] |= (page_addr & 0xffff) > + << (host->addr_cycle * 8); > + host->addr_cycle += 2; > + /* One more address cycle for devices > 128MiB */ > + if (chip->chipsize > (128 << 20)) { > + host->addr_cycle += 1; > + if (host->command == NAND_CMD_ERASE1) > + host->addr_value[0] |= ((page_addr >> 16) & 0xff) << 16; > + else > + host->addr_value[1] |= ((page_addr >> 16) & 0xff); > + } > + } > +} > + > +static void hisi_nfc_cmdfunc(struct mtd_info *mtd, unsigned command, int column, > + int page_addr) > +{ > + struct nand_chip *chip = mtd->priv; > + struct hinfc_host *host = chip->priv; > + int is_cache_invalid = 1; > + unsigned int flag = 0; > + > + host->command = command; > + > + switch (command) { > + case NAND_CMD_READ0: > + case NAND_CMD_READOOB: > + if (command == NAND_CMD_READ0) > + host->offset = column; > + else > + host->offset = column + mtd->writesize; > + > + is_cache_invalid = 0; > + set_addr(mtd, column, page_addr); > + hisi_nfc_send_cmd_readstart(host); > + break; > + > + case NAND_CMD_SEQIN: > + host->offset = column; > + set_addr(mtd, column, page_addr); > + break; > + > + case NAND_CMD_ERASE1: > + set_addr(mtd, column, page_addr); > + break; > + > + case NAND_CMD_PAGEPROG: > + hisi_nfc_send_cmd_pageprog(host); > + break; > + > + case NAND_CMD_ERASE2: > + hisi_nfc_send_cmd_erase(host); > + break; > + > + case NAND_CMD_READID: > + host->offset = column; > + memset(chip->IO_ADDR_R, 0, 0x10); > + hisi_nfc_send_cmd_readid(host); > + break; > + > + case NAND_CMD_STATUS: > + flag = hinfc_read(host, HINFC504_CON); > + if (chip->ecc.mode == NAND_ECC_HW) > + hinfc_write(host, > + flag && ~(HINFC504_CON_ECCTYPE_MASK << > + HINFC504_CON_ECCTYPE_SHIFT), HINFC504_CON); > + > + host->offset = 0; > + memset(chip->IO_ADDR_R, 0, 0x10); > + hisi_nfc_send_cmd_status(host); > + hinfc_write(host, flag, HINFC504_CON); > + break; > + > + case NAND_CMD_RESET: > + hisi_nfc_send_cmd_reset(host, host->chipselect); > + break; > + > + default: > + dev_err(host->dev, "Error: unsupported cmd(cmd=%x, col=%x, page=%x)\n", > + command, column, page_addr); > + } > + > + if (is_cache_invalid) { > + host->cache_addr_value[0] = ~0; > + host->cache_addr_value[1] = ~0; > + } > +} > + > +static irqreturn_t hinfc_irq_handle(int irq, void *devid) > +{ > + struct hinfc_host *host = devid; > + unsigned int flag; > + > + flag = hinfc_read(host, HINFC504_INTS); > + /* store interrupts state */ > + host->irq_status |= flag; > + > + if (flag & HINFC504_INTS_DMA) { > + hinfc_write(host, HINFC504_INTCLR_DMA, HINFC504_INTCLR); > + complete(&host->cmd_complete); > + } else if (flag & HINFC504_INTS_CE) { > + hinfc_write(host, HINFC504_INTCLR_CE, HINFC504_INTCLR); > + } else if (flag & HINFC504_INTS_UE) { > + hinfc_write(host, HINFC504_INTCLR_UE, HINFC504_INTCLR); > + } > + > + return IRQ_HANDLED; > +} > + > +static int hisi_nand_read_page_hwecc(struct mtd_info *mtd, > + struct nand_chip *chip, uint8_t *buf, int oob_required, int page) > +{ > + struct hinfc_host *host = chip->priv; > + int max_bitflips = 0, stat = 0; > + > + chip->read_buf(mtd, buf, mtd->writesize); > + chip->read_buf(mtd, chip->oob_poi, mtd->oobsize); > + > + /* errors which can not be corrected by ECC */ > + if (host->irq_status & HINFC504_INTS_UE) { > + mtd->ecc_stats.failed++; > + } else if (host->irq_status & HINFC504_INTS_CE) { > + /* need add other ECC modes! */ > + switch (host->ecc_bits) { > + case 1: > + stat = hweight8(hinfc_read(host, HINFC504_ECC_STATUS)>> > + HINFC504_ECC_1_BIT_SHIFT); > + break; > + case 6: > + stat = hweight16(hinfc_read(host, HINFC504_ECC_STATUS)>> > + HINFC504_ECC_16_BIT_SHIFT & 0x0fff); > + } > + mtd->ecc_stats.corrected += stat; > + max_bitflips = max_t(unsigned int, max_bitflips, stat); > + } > + host->irq_status = 0; > + > + return max_bitflips; > +} > + > +static int hisi_nand_write_page_hwecc(struct mtd_info *mtd, > + struct nand_chip *chip, const uint8_t *buf, int oob_required) > +{ > + chip->write_buf(mtd, buf, mtd->writesize); > + if (oob_required) > + chip->write_buf(mtd, chip->oob_poi, mtd->oobsize); > + > + return 0; > +} > + > +static void hisi_nfc_host_init(struct hinfc_host *host) > +{ > + struct nand_chip *chip = &host->chip; > + unsigned int flag = 0; > + > + host->version = hinfc_read(host, HINFC_VERSION); > + host->addr_cycle = 0; > + host->addr_value[0] = 0; > + host->addr_value[1] = 0; > + host->cache_addr_value[0] = ~0; > + host->cache_addr_value[1] = ~0; > + host->chipselect = 0; > + > + /* default page size: 2K, ecc_none. need modify */ > + flag = HINFC504_CON_OP_MODE_NORMAL | HINFC504_CON_READY_BUSY_SEL > + | ((0x001 & HINFC504_CON_PAGESIZE_MASK) > + << HINFC504_CON_PAGEISZE_SHIFT) > + | ((0x0 & HINFC504_CON_ECCTYPE_MASK) > + << HINFC504_CON_ECCTYPE_SHIFT) > + | ((chip->options & NAND_BUSWIDTH_16) ? > + HINFC504_CON_BUS_WIDTH : 0); > + hinfc_write(host, flag, HINFC504_CON); > + > + memset(chip->IO_ADDR_R, 0xff, HINFC504_BUFFER_BASE_ADDRESS_LEN); > + > + hinfc_write(host, SET_HINFC504_PWIDTH(HINFC504_W_LATCH, > + HINFC504_R_LATCH, HINFC504_RW_LATCH), HINFC504_PWIDTH); > + > + /* enable dma irq */ > + hinfc_write(host, HINFC504_INTEN_DMA, HINFC504_INTEN); > +} > + > +static struct nand_ecclayout nand_ecc_2K_1bit = { > + .oobfree = { {24, 40} } > +}; > + > +static struct nand_ecclayout nand_ecc_2K_16bits = { > + .oobavail = 6, > + .oobfree = { {2, 6} }, > +}; > + > +static int hisi_nfc_ecc_probe(struct hinfc_host *host) > +{ > + struct nand_chip *chip = &host->chip; > + unsigned int flag; > + > + chip->ecc.read_page = hisi_nand_read_page_hwecc; > + chip->ecc.write_page = hisi_nand_write_page_hwecc; > + > + switch (host->ecc_bits) { > + case 1: > + chip->ecc.layout = &nand_ecc_2K_1bit; > + chip->ecc.strength = 1; > + chip->ecc.size = 512; > + break; > + case 6: > + chip->ecc.layout = &nand_ecc_2K_16bits; > + chip->ecc.strength = 16; > + chip->ecc.size = 1024; > + } > + > + flag = hinfc_read(host, HINFC504_CON); > + /* add ecc type configure */ > + flag |= ((host->ecc_bits & HINFC504_CON_ECCTYPE_MASK) > + << HINFC504_CON_ECCTYPE_SHIFT); > + hinfc_write(host, flag, HINFC504_CON); > + > + /* enable ecc irq */ > + flag = hinfc_read(host, HINFC504_INTEN) & 0xfff; > + hinfc_write(host, flag | HINFC504_INTEN_UE | HINFC504_INTEN_CE, > + HINFC504_INTEN); > + > + return 0; > +} > + > +static int hisi_nfc_probe(struct platform_device *pdev) > +{ > + int ret = 0, irq, buswidth, flag, max_chips = HINFC504_MAX_CHIP; > + struct device *dev = &pdev->dev; > + struct hinfc_host *host; > + struct nand_chip *chip; > + struct mtd_info *mtd; > + struct resource *res; > + struct device_node *np = dev->of_node; > + struct mtd_part_parser_data ppdata; > + > + host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL); > + if (!host) > + return -ENOMEM; > + host->dev = dev; > + > + platform_set_drvdata(pdev, host); > + chip = &host->chip; > + mtd = &host->mtd; > + > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) { > + dev_err(dev, "no IRQ resource defined\n"); > + ret = -ENXIO; > + goto err_res; > + } > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + host->iobase = devm_ioremap_resource(dev, res); > + if (IS_ERR(host->iobase)) { > + ret = PTR_ERR(host->iobase); > + dev_err(dev, "devm_ioremap_resource[0] fail\n"); > + goto err_res; > + } > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > + chip->IO_ADDR_R = chip->IO_ADDR_W = devm_ioremap_resource(dev, res); > + if (IS_ERR(chip->IO_ADDR_R)) { > + ret = PTR_ERR(chip->IO_ADDR_R); > + dev_err(dev, "devm_ioremap_resource[1] fail\n"); > + goto err_res; > + } > + > + mtd->priv = chip; > + mtd->owner = THIS_MODULE; > + mtd->name = "hisi_nand"; > + mtd->dev.parent = &pdev->dev; > + > + chip->priv = host; > + chip->cmdfunc = hisi_nfc_cmdfunc; > + chip->select_chip = hisi_nfc_select_chip; > + chip->read_byte = hisi_nfc_read_byte; > + chip->read_word = hisi_nfc_read_word; > + chip->write_buf = hisi_nfc_write_buf; > + chip->read_buf = hisi_nfc_read_buf; > + chip->chip_delay = HINFC504_CHIP_DELAY; > + > + chip->ecc.mode = of_get_nand_ecc_mode(np); > + /* read ecc-bits from dts */ > + of_property_read_u32(np, "hisi,nand-ecc-bits", &host->ecc_bits); Replace this with of_get_nand_ecc_step_size() and of_get_nand_ecc_strength(). > + if (host->ecc_bits != 0 && host->ecc_bits != 1 && host->ecc_bits != 6) { > + ret = -EINVAL; > + dev_err(dev, "invalid nand-ecc-bits: %u\n", host->ecc_bits); > + goto err_res; > + } > + > + buswidth = of_get_nand_bus_width(np); > + if (buswidth == 16) > + chip->options |= NAND_BUSWIDTH_16; > + > + hisi_nfc_host_init(host); > + > + ret = devm_request_irq(dev, irq, hinfc_irq_handle, IRQF_DISABLED, > + "nandc", host); > + if (ret) { > + dev_err(dev, "failed to request IRQ\n"); > + goto err_irq; > + } > + > + ret = nand_scan_ident(mtd, max_chips, NULL); > + if (ret) { > + ret = -ENODEV; > + goto err_ident; > + } > + > + host->buffer = dma_alloc_coherent(dev, mtd->writesize + mtd->oobsize, > + &host->dma_buffer, GFP_KERNEL); Can you use dmam_alloc_coherent()? That will save you some of the cleanup on error and removal paths. > + if (!host->buffer) { > + dev_err(dev, "Can't malloc memory for NAND driver.\n"); > + ret = -ENOMEM; > + goto err_buf; > + } > + host->dma_oob = host->dma_buffer + mtd->writesize; > + memset(host->buffer, 0xff, mtd->writesize + mtd->oobsize); > + > + flag = hinfc_read(host, HINFC504_CON); > + flag &= ~(HINFC504_CON_PAGESIZE_MASK << HINFC504_CON_PAGEISZE_SHIFT); > + switch (mtd->writesize) { > + case 2048: > + flag |= (0x001 << HINFC504_CON_PAGEISZE_SHIFT); > + /* add more pagesize support > + * default pagesize has been set in hisi_nfc_host_init > + */ Does this mean you can't handle any other page size? You might want to put the words 'TODO' or 'FIXME' in the comment, and you might want to print an error and exit if you see some other value for mtd->writesize. > + } > + hinfc_write(host, flag, HINFC504_CON); > + > + if (chip->ecc.mode == NAND_ECC_HW) > + hisi_nfc_ecc_probe(host); > + > + nand_scan_tail(mtd); Please capture and handle the return value here. > + > + ppdata.of_node = np; > + ret = mtd_device_parse_register(mtd, NULL, &ppdata, NULL, 0); > + if (ret) { > + dev_err(dev, "Err MTD partition=%d\n", ret); > + goto err_mtd; > + } > + > + return 0; > + > +err_mtd: > + nand_release(mtd); > +err_ident: > +err_irq: Do you really need these empty labels? > +err_buf: > + if (host->buffer) > + dma_free_coherent(dev, mtd->writesize + mtd->oobsize, > + host->buffer, host->dma_buffer); This will go away. > +err_res: > + return ret; > +} > + > +static int hisi_nfc_remove(struct platform_device *pdev) > +{ > + struct hinfc_host *host = platform_get_drvdata(pdev); > + struct mtd_info *mtd = &host->mtd; > + > + nand_release(mtd); > + dma_free_coherent(&pdev->dev, mtd->writesize + mtd->oobsize, > + host->buffer, host->dma_buffer); Same here. > + > + return 0; > +} > + > +#ifdef CONFIG_PM_SLEEP > +static int hisi_nfc_suspend(struct platform_device *pdev, > + pm_message_t state) > +{ > + struct hinfc_host *host = platform_get_drvdata(pdev); > + > + while ((hinfc_read(host, HINFC504_STATUS) & 0x1) == 0x0) > + ; > + > + while ((hinfc_read(host, HINFC504_DMA_CTRL)) > + & HINFC504_DMA_CTRL_DMA_START) > + _cond_resched(); It's probably best if these don't spin forever. Can you implement a timeout, and return an error on failure? > + > + return 0; > +} > + > +static int hisi_nfc_resume(struct platform_device *pdev) > +{ > + int cs; > + struct hinfc_host *host = platform_get_drvdata(pdev); > + struct nand_chip *chip = &host->chip; > + > + for (cs = 0; cs < chip->numchips; cs++) > + hisi_nfc_send_cmd_reset(host, cs); > + hinfc_write(host, SET_HINFC504_PWIDTH(HINFC504_W_LATCH, > + HINFC504_R_LATCH, HINFC504_RW_LATCH), HINFC504_PWIDTH); > + > + return 0; > +} > +#endif > +static SIMPLE_DEV_PM_OPS(hisi_nfc_pm_ops, hisi_nfc_suspend, hisi_nfc_resume); > + > +static const struct of_device_id nfc_id_table[] = { > + { .compatible = "hisilicon,504-nfc" }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, nfc_id_table); > + > +static struct platform_driver hisi_nfc_driver = { > + .driver = { > + .name = "hisi_nand", > + .of_match_table = of_match_ptr(nfc_id_table), > + .pm = &hisi_nfc_pm_ops, > + }, > + .probe = hisi_nfc_probe, > + .remove = hisi_nfc_remove, > +}; > + > +module_platform_driver(hisi_nfc_driver); > + > +MODULE_LICENSE("GPL"); > +MODULE_AUTHOR("Zhiyong Cai"); > +MODULE_AUTHOR("Zhou Wang"); > +MODULE_DESCRIPTION("Hisilicon Nand Flash Controller Driver"); Brian ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 1/2] mtd: hisilicon: add a new NAND controller driver for hisilicon hip04 Soc 2014-11-30 9:35 ` Brian Norris @ 2014-12-01 13:08 ` Zhou Wang 2014-12-01 18:14 ` Brian Norris 0 siblings, 1 reply; 26+ messages in thread From: Zhou Wang @ 2014-12-01 13:08 UTC (permalink / raw) To: Brian Norris Cc: mark.rutland, devicetree, pawel.moll, ijc+devicetree, linux-kernel, haojian.zhuang, wangzhou1, robh+dt, linux-mtd, xuwei5, galak, caizhiyong, David Woodhouse On 2014年11月30日 17:35, Brian Norris wrote: > On Tue, Nov 04, 2014 at 08:47:00PM +0800, Zhou Wang wrote: >> Signed-off-by: Zhou Wang <wangzhou.bry@gmail.com> > > This driver mostly looks good. A few comments. > >> --- >> drivers/mtd/nand/Kconfig | 5 + >> drivers/mtd/nand/Makefile | 1 + >> drivers/mtd/nand/hisi504_nand.c | 846 +++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 852 insertions(+) >> create mode 100644 drivers/mtd/nand/hisi504_nand.c >> >> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig >> index dd10646..e451a08 100644 >> --- a/drivers/mtd/nand/Kconfig >> +++ b/drivers/mtd/nand/Kconfig >> @@ -516,4 +516,9 @@ config MTD_NAND_XWAY >> Enables support for NAND Flash chips on Lantiq XWAY SoCs. NAND is attached >> to the External Bus Unit (EBU). >> >> +config MTD_NAND_HISI504 >> + tristate "Support for NAND controller on Hisilicon SoC Hip04" >> + help >> + Enables support for NAND controller on Hisilicon SoC Hip04. >> + >> endif # MTD_NAND > > You'll want to rebase on l2-mtd.git when you resend. There are some > small Kconfig and Makefile conflicts. (I can fix them myself, but if > you're going to resend anyway...) > Hi Brian, I will rebase the patchset on l2-mtd.git when resend. >> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile >> index 9c847e4..fb1b2e4 100644 >> --- a/drivers/mtd/nand/Makefile >> +++ b/drivers/mtd/nand/Makefile >> @@ -50,5 +50,6 @@ obj-$(CONFIG_MTD_NAND_JZ4740) += jz4740_nand.o >> obj-$(CONFIG_MTD_NAND_GPMI_NAND) += gpmi-nand/ >> obj-$(CONFIG_MTD_NAND_XWAY) += xway_nand.o >> obj-$(CONFIG_MTD_NAND_BCM47XXNFLASH) += bcm47xxnflash/ >> +obj-$(CONFIG_MTD_NAND_HISI504) += hisi504_nand.o >> >> nand-objs := nand_base.o nand_bbt.o nand_timings.o >> diff --git a/drivers/mtd/nand/hisi504_nand.c b/drivers/mtd/nand/hisi504_nand.c >> new file mode 100644 >> index 0000000..a169cd8 >> --- /dev/null >> +++ b/drivers/mtd/nand/hisi504_nand.c >> @@ -0,0 +1,846 @@ >> +/* >> + * Hisilicon NAND Flash controller driver >> + * >> + * Copyright © 2012-2014 HiSilicon Technologies Co., Ltd. >> + * http://www.hisilicon.com >> + * >> + * Author: Zhou Wang <wangzhou.bry@gmail.com> >> + * The initial developer of the original code is Zhiyong Cai >> + * <caizhiyong@huawei.com> >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + * >> + * 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/of.h> >> +#include <linux/of_mtd.h> >> +#include <linux/mtd/mtd.h> >> +#include <linux/sizes.h> >> +#include <linux/clk.h> >> +#include <linux/slab.h> >> +#include <linux/module.h> >> +#include <linux/delay.h> >> +#include <linux/interrupt.h> >> +#include <linux/mtd/nand.h> >> +#include <linux/dma-mapping.h> >> +#include <linux/platform_device.h> >> +#include <linux/mtd/partitions.h> >> + >> +#define HINFC504_MAX_CHIP (4) >> +#define HINFC504_W_LATCH (5) >> +#define HINFC504_R_LATCH (7) >> +#define HINFC504_RW_LATCH (3) >> + >> +#define HINFC504_NFC_TIMEOUT (2 * HZ) >> +#define HINFC504_NFC_DMA_TIMEOUT (5 * HZ) >> +#define HINFC504_CHIP_DELAY (25) >> + >> +#define HINFC504_REG_BASE_ADDRESS_LEN (0x100) >> +#define HINFC504_BUFFER_BASE_ADDRESS_LEN (2048 + 128) >> + >> +#define HINFC504_ADDR_CYCLE_MASK 0x4 >> + >> +#define HINFC504_CON 0x00 >> +#define HINFC504_CON_OP_MODE_NORMAL (1U << 0) >> +#define HINFC504_CON_PAGEISZE_SHIFT (1) >> +#define HINFC504_CON_PAGESIZE_MASK (0x07) >> +#define HINFC504_CON_BUS_WIDTH (1U << 4) >> +#define HINFC504_CON_READY_BUSY_SEL (1U << 8) >> +#define HINFC504_CON_ECCTYPE_SHIFT (9) >> +#define HINFC504_CON_ECCTYPE_MASK (0x07) >> + >> +#define HINFC504_PWIDTH 0x04 >> +#define SET_HINFC504_PWIDTH(_w_lcnt, _r_lcnt, _rw_hcnt) \ >> + ((_w_lcnt) | (((_r_lcnt) & 0x0F) << 4) | (((_rw_hcnt) & 0x0F) << 8)) >> + >> +#define HINFC504_CMD 0x0C >> +#define HINFC504_ADDRL 0x10 >> +#define HINFC504_ADDRH 0x14 >> +#define HINFC504_DATA_NUM 0x18 >> + >> +#define HINFC504_OP 0x1C >> +#define HINFC504_OP_READ_DATA_EN (1U << 1) >> +#define HINFC504_OP_WAIT_READY_EN (1U << 2) >> +#define HINFC504_OP_CMD2_EN (1U << 3) >> +#define HINFC504_OP_WRITE_DATA_EN (1U << 4) >> +#define HINFC504_OP_ADDR_EN (1U << 5) >> +#define HINFC504_OP_CMD1_EN (1U << 6) > > Might try the BIT() macros here. will modify this and use BIT(), thanks. > >> +#define HINFC504_OP_NF_CS_SHIFT (7) >> +#define HINFC504_OP_NF_CS_MASK (3) >> +#define HINFC504_OP_ADDR_CYCLE_SHIFT (9) >> +#define HINFC504_OP_ADDR_CYCLE_MASK (7) >> + >> +#define HINFC504_STATUS 0x20 >> +#define HINFC504_READY (1U << 0) >> + >> +#define HINFC504_INTEN 0x24 >> +#define HINFC504_INTEN_DMA (1U << 9) >> +#define HINFC504_INTEN_UE (1U << 6) >> +#define HINFC504_INTEN_CE (1U << 5) > > Same here, and in the next few blocks. > >> + >> +#define HINFC504_INTS 0x28 >> +#define HINFC504_INTS_DMA (1U << 9) >> +#define HINFC504_INTS_UE (1U << 6) >> +#define HINFC504_INTS_CE (1U << 5) >> + >> +#define HINFC504_INTCLR 0x2C >> +#define HINFC504_INTCLR_DMA (1U << 9) >> +#define HINFC504_INTCLR_UE (1U << 6) >> +#define HINFC504_INTCLR_CE (1U << 5) >> + >> +#define HINFC504_ECC_STATUS 0x5C >> +#define HINFC504_ECC_1_BIT_SHIFT 16 >> +#define HINFC504_ECC_16_BIT_SHIFT 12 >> + >> +#define HINFC504_DMA_CTRL 0x60 >> +#define HINFC504_DMA_CTRL_DMA_START (1U << 0) >> +#define HINFC504_DMA_CTRL_WE (1U << 1) >> +#define HINFC504_DMA_CTRL_DATA_AREA_EN (1U << 2) >> +#define HINFC504_DMA_CTRL_OOB_AREA_EN (1U << 3) >> +#define HINFC504_DMA_CTRL_BURST4_EN (1U << 4) >> +#define HINFC504_DMA_CTRL_BURST8_EN (1U << 5) >> +#define HINFC504_DMA_CTRL_BURST16_EN (1U << 6) >> +#define HINFC504_DMA_CTRL_ADDR_NUM_SHIFT (7) >> +#define HINFC504_DMA_CTRL_ADDR_NUM_MASK (1) >> +#define HINFC504_DMA_CTRL_CS_SHIFT (8) >> +#define HINFC504_DMA_CTRL_CS_MASK (0x03) >> + >> +#define HINFC504_DMA_ADDR_DATA 0x64 >> +#define HINFC504_DMA_ADDR_OOB 0x68 >> + >> +#define HINFC504_DMA_LEN 0x6C >> +#define HINFC504_DMA_LEN_OOB_SHIFT (16) >> +#define HINFC504_DMA_LEN_OOB_MASK (0xFFF) >> + >> +#define HINFC504_DMA_PARA 0x70 >> +#define HINFC504_DMA_PARA_DATA_RW_EN (1U << 0) >> +#define HINFC504_DMA_PARA_OOB_RW_EN (1U << 1) >> +#define HINFC504_DMA_PARA_DATA_EDC_EN (1U << 2) >> +#define HINFC504_DMA_PARA_OOB_EDC_EN (1U << 3) >> +#define HINFC504_DMA_PARA_DATA_ECC_EN (1U << 4) >> +#define HINFC504_DMA_PARA_OOB_ECC_EN (1U << 5) >> + >> +#define HINFC_VERSION 0x74 >> +#define HINFC504_LOG_READ_ADDR 0x7C >> +#define HINFC504_LOG_READ_LEN 0x80 >> + >> +#define HINFC504_NANDINFO_LEN 0x10 >> + >> +struct hinfc_host { >> + struct nand_chip chip; >> + struct mtd_info mtd; >> + struct device *dev; >> + void __iomem *iobase; >> + struct completion cmd_complete; >> + unsigned int offset; >> + unsigned int command; >> + int chipselect; >> + unsigned int addr_cycle; >> + unsigned int addr_value[2]; > > It looks like you're using addr_value as 32-bit copies of the HW > register. Might make sense to declare them as u32, since you care about > the width. > Right, thanks. >> + unsigned int cache_addr_value[2]; >> + char *buffer; >> + dma_addr_t dma_buffer; >> + dma_addr_t dma_oob; >> + int version; >> + unsigned int ecc_bits; >> + unsigned int irq_status; /* interrupt status */ >> +}; >> + >> +static inline unsigned int hinfc_read(struct hinfc_host *host, unsigned int reg) >> +{ >> + return readl(host->iobase + reg); >> +} >> + >> +static inline void hinfc_write(struct hinfc_host *host, unsigned int value, >> + unsigned int reg) >> +{ >> + writel(value, host->iobase + reg); >> +} >> + >> +static void wait_controller_finished(struct hinfc_host *host) >> +{ >> + unsigned long timeout = jiffies + HINFC504_NFC_TIMEOUT; >> + int val; >> + >> + while (time_before(jiffies, timeout)) { >> + val = hinfc_read(host, HINFC504_STATUS); >> + if (host->command == NAND_CMD_ERASE2) { >> + /* nfc is ready */ >> + while (!(val & HINFC504_READY)) { >> + usleep_range(500, 1000); >> + val = hinfc_read(host, HINFC504_STATUS); >> + } >> + return; >> + } >> + >> + if (val & HINFC504_READY) >> + return; >> + } >> + >> + /* wait cmd timeout */ >> + dev_err(host->dev, "Wait NAND controller exec cmd timeout.\n"); >> +} >> + >> +static void hisi_nfc_dma_transfer(struct hinfc_host *host, int todev) >> +{ >> + struct mtd_info *mtd = &host->mtd; >> + struct nand_chip *chip = mtd->priv; >> + unsigned long val; >> + int ret; >> + >> + hinfc_write(host, host->dma_buffer, HINFC504_DMA_ADDR_DATA); >> + hinfc_write(host, host->dma_oob, HINFC504_DMA_ADDR_OOB); >> + >> + if (chip->ecc.mode == NAND_ECC_NONE) { >> + hinfc_write(host, ((mtd->oobsize & HINFC504_DMA_LEN_OOB_MASK) >> + << HINFC504_DMA_LEN_OOB_SHIFT), HINFC504_DMA_LEN); >> + >> + hinfc_write(host, HINFC504_DMA_PARA_DATA_RW_EN >> + | HINFC504_DMA_PARA_OOB_RW_EN, HINFC504_DMA_PARA); >> + } else { >> + hinfc_write(host, HINFC504_DMA_PARA_DATA_RW_EN >> + | HINFC504_DMA_PARA_OOB_RW_EN | HINFC504_DMA_PARA_DATA_EDC_EN >> + | HINFC504_DMA_PARA_OOB_EDC_EN | HINFC504_DMA_PARA_DATA_ECC_EN >> + | HINFC504_DMA_PARA_OOB_ECC_EN, HINFC504_DMA_PARA); >> + } >> + >> + val = (HINFC504_DMA_CTRL_DMA_START | HINFC504_DMA_CTRL_BURST4_EN >> + | HINFC504_DMA_CTRL_BURST8_EN | HINFC504_DMA_CTRL_BURST16_EN >> + | HINFC504_DMA_CTRL_DATA_AREA_EN | HINFC504_DMA_CTRL_OOB_AREA_EN >> + | ((host->addr_cycle == 4 ? 1 : 0) >> + << HINFC504_DMA_CTRL_ADDR_NUM_SHIFT) >> + | ((host->chipselect & HINFC504_DMA_CTRL_CS_MASK) >> + << HINFC504_DMA_CTRL_CS_SHIFT)); >> + >> + if (todev) >> + val |= HINFC504_DMA_CTRL_WE; >> + >> + hinfc_write(host, val, HINFC504_DMA_CTRL); >> + >> + init_completion(&host->cmd_complete); > > You need to run init_completion() *before* you kick off your I/O. > Otherwise, your interrupt handler is racing with this function. > Do you mean that it needs put init_completion() before hinfc_write()? If not so, interrupt handler maybe execute before init_completion() which will cause something wrong. >> + ret = wait_for_completion_timeout(&host->cmd_complete, >> + HINFC504_NFC_DMA_TIMEOUT); >> + >> + if (!ret) { >> + dev_err(host->dev, "DMA operation(irq) timeout!\n"); >> + /* sanity check */ >> + val = hinfc_read(host, HINFC504_DMA_CTRL); >> + if (!(val & HINFC504_DMA_CTRL_DMA_START)) >> + dev_err(host->dev, "dma is already done but without irq ACK!"); > > You're missing a newline at the end of this message. Please check your > other prints too. > > Also, s/dma/DMA/ will modify this, thanks! > >> + else >> + dev_err(host->dev, "dma is really timeout!"); >> + } >> +} >> + >> +static int hisi_nfc_send_cmd_pageprog(struct hinfc_host *host) >> +{ >> + host->addr_value[0] &= 0xffff0000; >> + >> + hinfc_write(host, host->addr_value[0], HINFC504_ADDRL); >> + hinfc_write(host, host->addr_value[1], HINFC504_ADDRH); >> + hinfc_write(host, NAND_CMD_PAGEPROG << 8 | NAND_CMD_SEQIN, >> + HINFC504_CMD); >> + >> + hisi_nfc_dma_transfer(host, 1); >> + >> + return 0; >> +} >> + >> +static int hisi_nfc_send_cmd_readstart(struct hinfc_host *host) >> +{ >> + struct mtd_info *mtd = &host->mtd; >> + >> + if ((host->addr_value[0] == host->cache_addr_value[0]) && >> + (host->addr_value[1] == host->cache_addr_value[1])) >> + return 0; >> + >> + host->addr_value[0] &= 0xffff0000; >> + >> + hinfc_write(host, host->addr_value[0], HINFC504_ADDRL); >> + hinfc_write(host, host->addr_value[1], HINFC504_ADDRH); >> + hinfc_write(host, NAND_CMD_READSTART << 8 | NAND_CMD_READ0, >> + HINFC504_CMD); >> + >> + hinfc_write(host, 0, HINFC504_LOG_READ_ADDR); >> + hinfc_write(host, mtd->writesize + mtd->oobsize, >> + HINFC504_LOG_READ_LEN); >> + >> + hisi_nfc_dma_transfer(host, 0); >> + >> + host->cache_addr_value[0] = host->addr_value[0]; >> + host->cache_addr_value[1] = host->addr_value[1]; >> + >> + return 0; >> +} >> + >> +static int hisi_nfc_send_cmd_erase(struct hinfc_host *host) >> +{ >> + hinfc_write(host, host->addr_value[0], HINFC504_ADDRL); >> + hinfc_write(host, (NAND_CMD_ERASE2 << 8) | NAND_CMD_ERASE1, >> + HINFC504_CMD); >> + >> + hinfc_write(host, HINFC504_OP_WAIT_READY_EN >> + | HINFC504_OP_CMD2_EN >> + | HINFC504_OP_CMD1_EN >> + | HINFC504_OP_ADDR_EN >> + | ((host->chipselect & HINFC504_OP_NF_CS_MASK) >> + << HINFC504_OP_NF_CS_SHIFT) >> + | ((host->addr_cycle & HINFC504_OP_ADDR_CYCLE_MASK) >> + << HINFC504_OP_ADDR_CYCLE_SHIFT), >> + HINFC504_OP); >> + >> + wait_controller_finished(host); >> + >> + return 0; >> +} >> + >> +static int hisi_nfc_send_cmd_readid(struct hinfc_host *host) >> +{ >> + hinfc_write(host, HINFC504_NANDINFO_LEN, HINFC504_DATA_NUM); >> + hinfc_write(host, NAND_CMD_READID, HINFC504_CMD); >> + hinfc_write(host, 0, HINFC504_ADDRL); >> + >> + hinfc_write(host, HINFC504_OP_CMD1_EN | HINFC504_OP_ADDR_EN >> + | HINFC504_OP_READ_DATA_EN >> + | ((host->chipselect & HINFC504_OP_NF_CS_MASK) >> + << HINFC504_OP_NF_CS_SHIFT) >> + | 1 << HINFC504_OP_ADDR_CYCLE_SHIFT, HINFC504_OP); >> + >> + wait_controller_finished(host); >> + >> + return 0; >> +} >> + >> +static int hisi_nfc_send_cmd_status(struct hinfc_host *host) >> +{ >> + hinfc_write(host, HINFC504_NANDINFO_LEN, HINFC504_DATA_NUM); >> + hinfc_write(host, NAND_CMD_STATUS, HINFC504_CMD); >> + hinfc_write(host, HINFC504_OP_CMD1_EN >> + | HINFC504_OP_READ_DATA_EN >> + | ((host->chipselect & HINFC504_OP_NF_CS_MASK) >> + << HINFC504_OP_NF_CS_SHIFT), >> + HINFC504_OP); >> + >> + wait_controller_finished(host); >> + >> + return 0; >> +} >> + >> +static int hisi_nfc_send_cmd_reset(struct hinfc_host *host, int chipselect) >> +{ >> + hinfc_write(host, NAND_CMD_RESET, HINFC504_CMD); >> + >> + hinfc_write(host, HINFC504_OP_CMD1_EN >> + | ((chipselect & HINFC504_OP_NF_CS_MASK) >> + << HINFC504_OP_NF_CS_SHIFT) >> + | HINFC504_OP_WAIT_READY_EN, >> + HINFC504_OP); >> + >> + wait_controller_finished(host); >> + >> + return 0; >> +} >> + >> +static void hisi_nfc_select_chip(struct mtd_info *mtd, int chipselect) >> +{ >> + struct nand_chip *chip = mtd->priv; >> + struct hinfc_host *host = chip->priv; >> + >> + if (chipselect < 0) >> + return; >> + >> + host->chipselect = chipselect; >> +} >> + >> +static uint8_t hisi_nfc_read_byte(struct mtd_info *mtd) >> +{ >> + struct nand_chip *chip = mtd->priv; >> + struct hinfc_host *host = chip->priv; >> + >> + if (host->command == NAND_CMD_STATUS) >> + return readb(chip->IO_ADDR_R); >> + >> + host->offset++; >> + >> + if (host->command == NAND_CMD_READID) >> + return readb(chip->IO_ADDR_R + host->offset - 1); >> + >> + return readb(host->buffer + host->offset - 1); >> +} >> + >> +static u16 hisi_nfc_read_word(struct mtd_info *mtd) >> +{ >> + struct nand_chip *chip = mtd->priv; >> + struct hinfc_host *host = chip->priv; >> + >> + host->offset += 2; >> + return readw(host->buffer + host->offset - 2); >> +} >> + >> +static void >> +hisi_nfc_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len) >> +{ >> + struct nand_chip *chip = mtd->priv; >> + struct hinfc_host *host = chip->priv; >> + >> + memcpy(host->buffer + host->offset, buf, len); >> + host->offset += len; >> +} >> + >> +static void hisi_nfc_read_buf(struct mtd_info *mtd, uint8_t *buf, int len) >> +{ >> + struct nand_chip *chip = mtd->priv; >> + struct hinfc_host *host = chip->priv; >> + >> + memcpy(buf, host->buffer + host->offset, len); >> + host->offset += len; >> +} >> + >> +static void set_addr(struct mtd_info *mtd, int column, int page_addr) >> +{ >> + struct nand_chip *chip = mtd->priv; >> + struct hinfc_host *host = chip->priv; >> + >> + host->addr_cycle = 0; >> + host->addr_value[0] = 0; >> + host->addr_value[1] = 0; >> + >> + /* Serially input address */ >> + if (column != -1) { >> + /* Adjust columns for 16 bit buswidth */ >> + if (chip->options & NAND_BUSWIDTH_16) >> + column >>= 1; > > It doesn't look like you're supporting ONFI parameter pages yet, but you > might consider checking for !nand_opcode_8bits(opcode) before adjusting the > address. We had to fix some other drivers for this recently. > sorry, could you give me more clue about this? Do you mean that we should first make sure it is not 8 bits bus width? >> + >> + host->addr_value[0] = column & 0xffff; >> + host->addr_cycle = 2; >> + } >> + if (page_addr != -1) { >> + host->addr_value[0] |= (page_addr & 0xffff) >> + << (host->addr_cycle * 8); >> + host->addr_cycle += 2; >> + /* One more address cycle for devices > 128MiB */ >> + if (chip->chipsize > (128 << 20)) { >> + host->addr_cycle += 1; >> + if (host->command == NAND_CMD_ERASE1) >> + host->addr_value[0] |= ((page_addr >> 16) & 0xff) << 16; >> + else >> + host->addr_value[1] |= ((page_addr >> 16) & 0xff); >> + } >> + } >> +} >> + >> +static void hisi_nfc_cmdfunc(struct mtd_info *mtd, unsigned command, int column, >> + int page_addr) >> +{ >> + struct nand_chip *chip = mtd->priv; >> + struct hinfc_host *host = chip->priv; >> + int is_cache_invalid = 1; >> + unsigned int flag = 0; >> + >> + host->command = command; >> + >> + switch (command) { >> + case NAND_CMD_READ0: >> + case NAND_CMD_READOOB: >> + if (command == NAND_CMD_READ0) >> + host->offset = column; >> + else >> + host->offset = column + mtd->writesize; >> + >> + is_cache_invalid = 0; >> + set_addr(mtd, column, page_addr); >> + hisi_nfc_send_cmd_readstart(host); >> + break; >> + >> + case NAND_CMD_SEQIN: >> + host->offset = column; >> + set_addr(mtd, column, page_addr); >> + break; >> + >> + case NAND_CMD_ERASE1: >> + set_addr(mtd, column, page_addr); >> + break; >> + >> + case NAND_CMD_PAGEPROG: >> + hisi_nfc_send_cmd_pageprog(host); >> + break; >> + >> + case NAND_CMD_ERASE2: >> + hisi_nfc_send_cmd_erase(host); >> + break; >> + >> + case NAND_CMD_READID: >> + host->offset = column; >> + memset(chip->IO_ADDR_R, 0, 0x10); >> + hisi_nfc_send_cmd_readid(host); >> + break; >> + >> + case NAND_CMD_STATUS: >> + flag = hinfc_read(host, HINFC504_CON); >> + if (chip->ecc.mode == NAND_ECC_HW) >> + hinfc_write(host, >> + flag && ~(HINFC504_CON_ECCTYPE_MASK << >> + HINFC504_CON_ECCTYPE_SHIFT), HINFC504_CON); >> + >> + host->offset = 0; >> + memset(chip->IO_ADDR_R, 0, 0x10); >> + hisi_nfc_send_cmd_status(host); >> + hinfc_write(host, flag, HINFC504_CON); >> + break; >> + >> + case NAND_CMD_RESET: >> + hisi_nfc_send_cmd_reset(host, host->chipselect); >> + break; >> + >> + default: >> + dev_err(host->dev, "Error: unsupported cmd(cmd=%x, col=%x, page=%x)\n", >> + command, column, page_addr); >> + } >> + >> + if (is_cache_invalid) { >> + host->cache_addr_value[0] = ~0; >> + host->cache_addr_value[1] = ~0; >> + } >> +} >> + >> +static irqreturn_t hinfc_irq_handle(int irq, void *devid) >> +{ >> + struct hinfc_host *host = devid; >> + unsigned int flag; >> + >> + flag = hinfc_read(host, HINFC504_INTS); >> + /* store interrupts state */ >> + host->irq_status |= flag; >> + >> + if (flag & HINFC504_INTS_DMA) { >> + hinfc_write(host, HINFC504_INTCLR_DMA, HINFC504_INTCLR); >> + complete(&host->cmd_complete); >> + } else if (flag & HINFC504_INTS_CE) { >> + hinfc_write(host, HINFC504_INTCLR_CE, HINFC504_INTCLR); >> + } else if (flag & HINFC504_INTS_UE) { >> + hinfc_write(host, HINFC504_INTCLR_UE, HINFC504_INTCLR); >> + } >> + >> + return IRQ_HANDLED; >> +} >> + >> +static int hisi_nand_read_page_hwecc(struct mtd_info *mtd, >> + struct nand_chip *chip, uint8_t *buf, int oob_required, int page) >> +{ >> + struct hinfc_host *host = chip->priv; >> + int max_bitflips = 0, stat = 0; >> + >> + chip->read_buf(mtd, buf, mtd->writesize); >> + chip->read_buf(mtd, chip->oob_poi, mtd->oobsize); >> + >> + /* errors which can not be corrected by ECC */ >> + if (host->irq_status & HINFC504_INTS_UE) { >> + mtd->ecc_stats.failed++; >> + } else if (host->irq_status & HINFC504_INTS_CE) { >> + /* need add other ECC modes! */ >> + switch (host->ecc_bits) { >> + case 1: >> + stat = hweight8(hinfc_read(host, HINFC504_ECC_STATUS)>> >> + HINFC504_ECC_1_BIT_SHIFT); >> + break; >> + case 6: >> + stat = hweight16(hinfc_read(host, HINFC504_ECC_STATUS)>> >> + HINFC504_ECC_16_BIT_SHIFT & 0x0fff); >> + } >> + mtd->ecc_stats.corrected += stat; >> + max_bitflips = max_t(unsigned int, max_bitflips, stat); >> + } >> + host->irq_status = 0; >> + >> + return max_bitflips; >> +} >> + >> +static int hisi_nand_write_page_hwecc(struct mtd_info *mtd, >> + struct nand_chip *chip, const uint8_t *buf, int oob_required) >> +{ >> + chip->write_buf(mtd, buf, mtd->writesize); >> + if (oob_required) >> + chip->write_buf(mtd, chip->oob_poi, mtd->oobsize); >> + >> + return 0; >> +} >> + >> +static void hisi_nfc_host_init(struct hinfc_host *host) >> +{ >> + struct nand_chip *chip = &host->chip; >> + unsigned int flag = 0; >> + >> + host->version = hinfc_read(host, HINFC_VERSION); >> + host->addr_cycle = 0; >> + host->addr_value[0] = 0; >> + host->addr_value[1] = 0; >> + host->cache_addr_value[0] = ~0; >> + host->cache_addr_value[1] = ~0; >> + host->chipselect = 0; >> + >> + /* default page size: 2K, ecc_none. need modify */ >> + flag = HINFC504_CON_OP_MODE_NORMAL | HINFC504_CON_READY_BUSY_SEL >> + | ((0x001 & HINFC504_CON_PAGESIZE_MASK) >> + << HINFC504_CON_PAGEISZE_SHIFT) >> + | ((0x0 & HINFC504_CON_ECCTYPE_MASK) >> + << HINFC504_CON_ECCTYPE_SHIFT) >> + | ((chip->options & NAND_BUSWIDTH_16) ? >> + HINFC504_CON_BUS_WIDTH : 0); >> + hinfc_write(host, flag, HINFC504_CON); >> + >> + memset(chip->IO_ADDR_R, 0xff, HINFC504_BUFFER_BASE_ADDRESS_LEN); >> + >> + hinfc_write(host, SET_HINFC504_PWIDTH(HINFC504_W_LATCH, >> + HINFC504_R_LATCH, HINFC504_RW_LATCH), HINFC504_PWIDTH); >> + >> + /* enable dma irq */ >> + hinfc_write(host, HINFC504_INTEN_DMA, HINFC504_INTEN); >> +} >> + >> +static struct nand_ecclayout nand_ecc_2K_1bit = { >> + .oobfree = { {24, 40} } >> +}; >> + >> +static struct nand_ecclayout nand_ecc_2K_16bits = { >> + .oobavail = 6, >> + .oobfree = { {2, 6} }, >> +}; >> + >> +static int hisi_nfc_ecc_probe(struct hinfc_host *host) >> +{ >> + struct nand_chip *chip = &host->chip; >> + unsigned int flag; >> + >> + chip->ecc.read_page = hisi_nand_read_page_hwecc; >> + chip->ecc.write_page = hisi_nand_write_page_hwecc; >> + >> + switch (host->ecc_bits) { >> + case 1: >> + chip->ecc.layout = &nand_ecc_2K_1bit; >> + chip->ecc.strength = 1; >> + chip->ecc.size = 512; >> + break; >> + case 6: >> + chip->ecc.layout = &nand_ecc_2K_16bits; >> + chip->ecc.strength = 16; >> + chip->ecc.size = 1024; >> + } >> + >> + flag = hinfc_read(host, HINFC504_CON); >> + /* add ecc type configure */ >> + flag |= ((host->ecc_bits & HINFC504_CON_ECCTYPE_MASK) >> + << HINFC504_CON_ECCTYPE_SHIFT); >> + hinfc_write(host, flag, HINFC504_CON); >> + >> + /* enable ecc irq */ >> + flag = hinfc_read(host, HINFC504_INTEN) & 0xfff; >> + hinfc_write(host, flag | HINFC504_INTEN_UE | HINFC504_INTEN_CE, >> + HINFC504_INTEN); >> + >> + return 0; >> +} >> + >> +static int hisi_nfc_probe(struct platform_device *pdev) >> +{ >> + int ret = 0, irq, buswidth, flag, max_chips = HINFC504_MAX_CHIP; >> + struct device *dev = &pdev->dev; >> + struct hinfc_host *host; >> + struct nand_chip *chip; >> + struct mtd_info *mtd; >> + struct resource *res; >> + struct device_node *np = dev->of_node; >> + struct mtd_part_parser_data ppdata; >> + >> + host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL); >> + if (!host) >> + return -ENOMEM; >> + host->dev = dev; >> + >> + platform_set_drvdata(pdev, host); >> + chip = &host->chip; >> + mtd = &host->mtd; >> + >> + irq = platform_get_irq(pdev, 0); >> + if (irq < 0) { >> + dev_err(dev, "no IRQ resource defined\n"); >> + ret = -ENXIO; >> + goto err_res; >> + } >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + host->iobase = devm_ioremap_resource(dev, res); >> + if (IS_ERR(host->iobase)) { >> + ret = PTR_ERR(host->iobase); >> + dev_err(dev, "devm_ioremap_resource[0] fail\n"); >> + goto err_res; >> + } >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); >> + chip->IO_ADDR_R = chip->IO_ADDR_W = devm_ioremap_resource(dev, res); >> + if (IS_ERR(chip->IO_ADDR_R)) { >> + ret = PTR_ERR(chip->IO_ADDR_R); >> + dev_err(dev, "devm_ioremap_resource[1] fail\n"); >> + goto err_res; >> + } >> + >> + mtd->priv = chip; >> + mtd->owner = THIS_MODULE; >> + mtd->name = "hisi_nand"; >> + mtd->dev.parent = &pdev->dev; >> + >> + chip->priv = host; >> + chip->cmdfunc = hisi_nfc_cmdfunc; >> + chip->select_chip = hisi_nfc_select_chip; >> + chip->read_byte = hisi_nfc_read_byte; >> + chip->read_word = hisi_nfc_read_word; >> + chip->write_buf = hisi_nfc_write_buf; >> + chip->read_buf = hisi_nfc_read_buf; >> + chip->chip_delay = HINFC504_CHIP_DELAY; >> + >> + chip->ecc.mode = of_get_nand_ecc_mode(np); >> + /* read ecc-bits from dts */ >> + of_property_read_u32(np, "hisi,nand-ecc-bits", &host->ecc_bits); > > Replace this with of_get_nand_ecc_step_size() and > of_get_nand_ecc_strength(). OK, will modify this. > >> + if (host->ecc_bits != 0 && host->ecc_bits != 1 && host->ecc_bits != 6) { >> + ret = -EINVAL; >> + dev_err(dev, "invalid nand-ecc-bits: %u\n", host->ecc_bits); >> + goto err_res; >> + } >> + >> + buswidth = of_get_nand_bus_width(np); >> + if (buswidth == 16) >> + chip->options |= NAND_BUSWIDTH_16; >> + >> + hisi_nfc_host_init(host); >> + >> + ret = devm_request_irq(dev, irq, hinfc_irq_handle, IRQF_DISABLED, >> + "nandc", host); >> + if (ret) { >> + dev_err(dev, "failed to request IRQ\n"); >> + goto err_irq; >> + } >> + >> + ret = nand_scan_ident(mtd, max_chips, NULL); >> + if (ret) { >> + ret = -ENODEV; >> + goto err_ident; >> + } >> + >> + host->buffer = dma_alloc_coherent(dev, mtd->writesize + mtd->oobsize, >> + &host->dma_buffer, GFP_KERNEL); > > Can you use dmam_alloc_coherent()? That will save you some of the > cleanup on error and removal paths. > OK, will modify this and related code. >> + if (!host->buffer) { >> + dev_err(dev, "Can't malloc memory for NAND driver.\n"); >> + ret = -ENOMEM; >> + goto err_buf; >> + } >> + host->dma_oob = host->dma_buffer + mtd->writesize; >> + memset(host->buffer, 0xff, mtd->writesize + mtd->oobsize); >> + >> + flag = hinfc_read(host, HINFC504_CON); >> + flag &= ~(HINFC504_CON_PAGESIZE_MASK << HINFC504_CON_PAGEISZE_SHIFT); >> + switch (mtd->writesize) { >> + case 2048: >> + flag |= (0x001 << HINFC504_CON_PAGEISZE_SHIFT); >> + /* add more pagesize support >> + * default pagesize has been set in hisi_nfc_host_init >> + */ > > Does this mean you can't handle any other page size? You might want to > put the words 'TODO' or 'FIXME' in the comment, and you might want to > print an error and exit if you see some other value for mtd->writesize. > Yes, this NAND controller can handle not only 2048 page size. But the NAND controller on hip04-d01 board which I used to test this driver connects with a NAND flash chip which is just 2048 page size. So here I just listed 'case 2048', Maybe I need indicate this by 'TODO:...'? >> + } >> + hinfc_write(host, flag, HINFC504_CON); >> + >> + if (chip->ecc.mode == NAND_ECC_HW) >> + hisi_nfc_ecc_probe(host); >> + >> + nand_scan_tail(mtd); > > Please capture and handle the return value here. > will modify this, thanks! >> + >> + ppdata.of_node = np; >> + ret = mtd_device_parse_register(mtd, NULL, &ppdata, NULL, 0); >> + if (ret) { >> + dev_err(dev, "Err MTD partition=%d\n", ret); >> + goto err_mtd; >> + } >> + >> + return 0; >> + >> +err_mtd: >> + nand_release(mtd); >> +err_ident: >> +err_irq: > > Do you really need these empty labels? > will merge the err_ident, err_irq, err_buf into err_res. >> +err_buf: >> + if (host->buffer) >> + dma_free_coherent(dev, mtd->writesize + mtd->oobsize, >> + host->buffer, host->dma_buffer); > > This will go away. > >> +err_res: >> + return ret; >> +} >> + >> +static int hisi_nfc_remove(struct platform_device *pdev) >> +{ >> + struct hinfc_host *host = platform_get_drvdata(pdev); >> + struct mtd_info *mtd = &host->mtd; >> + >> + nand_release(mtd); >> + dma_free_coherent(&pdev->dev, mtd->writesize + mtd->oobsize, >> + host->buffer, host->dma_buffer); > > Same here. >> + >> + return 0; >> +} >> + >> +#ifdef CONFIG_PM_SLEEP >> +static int hisi_nfc_suspend(struct platform_device *pdev, >> + pm_message_t state) >> +{ >> + struct hinfc_host *host = platform_get_drvdata(pdev); >> + >> + while ((hinfc_read(host, HINFC504_STATUS) & 0x1) == 0x0) >> + ; >> + >> + while ((hinfc_read(host, HINFC504_DMA_CTRL)) >> + & HINFC504_DMA_CTRL_DMA_START) >> + _cond_resched(); > > It's probably best if these don't spin forever. Can you implement a > timeout, and return an error on failure? > will add a timeout here, thanks! >> + >> + return 0; >> +} >> + >> +static int hisi_nfc_resume(struct platform_device *pdev) >> +{ >> + int cs; >> + struct hinfc_host *host = platform_get_drvdata(pdev); >> + struct nand_chip *chip = &host->chip; >> + >> + for (cs = 0; cs < chip->numchips; cs++) >> + hisi_nfc_send_cmd_reset(host, cs); >> + hinfc_write(host, SET_HINFC504_PWIDTH(HINFC504_W_LATCH, >> + HINFC504_R_LATCH, HINFC504_RW_LATCH), HINFC504_PWIDTH); >> + >> + return 0; >> +} >> +#endif >> +static SIMPLE_DEV_PM_OPS(hisi_nfc_pm_ops, hisi_nfc_suspend, hisi_nfc_resume); >> + >> +static const struct of_device_id nfc_id_table[] = { >> + { .compatible = "hisilicon,504-nfc" }, >> + {} >> +}; >> +MODULE_DEVICE_TABLE(of, nfc_id_table); >> + >> +static struct platform_driver hisi_nfc_driver = { >> + .driver = { >> + .name = "hisi_nand", >> + .of_match_table = of_match_ptr(nfc_id_table), >> + .pm = &hisi_nfc_pm_ops, >> + }, >> + .probe = hisi_nfc_probe, >> + .remove = hisi_nfc_remove, >> +}; >> + >> +module_platform_driver(hisi_nfc_driver); >> + >> +MODULE_LICENSE("GPL"); >> +MODULE_AUTHOR("Zhiyong Cai"); >> +MODULE_AUTHOR("Zhou Wang"); >> +MODULE_DESCRIPTION("Hisilicon Nand Flash Controller Driver"); > > Brian > Thanks! Zhou Wang ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 1/2] mtd: hisilicon: add a new NAND controller driver for hisilicon hip04 Soc 2014-12-01 13:08 ` Zhou Wang @ 2014-12-01 18:14 ` Brian Norris 0 siblings, 0 replies; 26+ messages in thread From: Brian Norris @ 2014-12-01 18:14 UTC (permalink / raw) To: Zhou Wang Cc: mark.rutland, devicetree, pawel.moll, ijc+devicetree, linux-kernel, haojian.zhuang, wangzhou1, robh+dt, linux-mtd, xuwei5, galak, caizhiyong, David Woodhouse On Mon, Dec 01, 2014 at 09:08:42PM +0800, Zhou Wang wrote: > On 2014年11月30日 17:35, Brian Norris wrote: > >On Tue, Nov 04, 2014 at 08:47:00PM +0800, Zhou Wang wrote: > >>Signed-off-by: Zhou Wang <wangzhou.bry@gmail.com> [...] > >>+static void hisi_nfc_dma_transfer(struct hinfc_host *host, int todev) > >>+{ > >>+ struct mtd_info *mtd = &host->mtd; > >>+ struct nand_chip *chip = mtd->priv; > >>+ unsigned long val; > >>+ int ret; > >>+ > >>+ hinfc_write(host, host->dma_buffer, HINFC504_DMA_ADDR_DATA); > >>+ hinfc_write(host, host->dma_oob, HINFC504_DMA_ADDR_OOB); > >>+ > >>+ if (chip->ecc.mode == NAND_ECC_NONE) { > >>+ hinfc_write(host, ((mtd->oobsize & HINFC504_DMA_LEN_OOB_MASK) > >>+ << HINFC504_DMA_LEN_OOB_SHIFT), HINFC504_DMA_LEN); > >>+ > >>+ hinfc_write(host, HINFC504_DMA_PARA_DATA_RW_EN > >>+ | HINFC504_DMA_PARA_OOB_RW_EN, HINFC504_DMA_PARA); > >>+ } else { > >>+ hinfc_write(host, HINFC504_DMA_PARA_DATA_RW_EN > >>+ | HINFC504_DMA_PARA_OOB_RW_EN | HINFC504_DMA_PARA_DATA_EDC_EN > >>+ | HINFC504_DMA_PARA_OOB_EDC_EN | HINFC504_DMA_PARA_DATA_ECC_EN > >>+ | HINFC504_DMA_PARA_OOB_ECC_EN, HINFC504_DMA_PARA); > >>+ } > >>+ > >>+ val = (HINFC504_DMA_CTRL_DMA_START | HINFC504_DMA_CTRL_BURST4_EN > >>+ | HINFC504_DMA_CTRL_BURST8_EN | HINFC504_DMA_CTRL_BURST16_EN > >>+ | HINFC504_DMA_CTRL_DATA_AREA_EN | HINFC504_DMA_CTRL_OOB_AREA_EN > >>+ | ((host->addr_cycle == 4 ? 1 : 0) > >>+ << HINFC504_DMA_CTRL_ADDR_NUM_SHIFT) > >>+ | ((host->chipselect & HINFC504_DMA_CTRL_CS_MASK) > >>+ << HINFC504_DMA_CTRL_CS_SHIFT)); > >>+ > >>+ if (todev) > >>+ val |= HINFC504_DMA_CTRL_WE; > >>+ > >>+ hinfc_write(host, val, HINFC504_DMA_CTRL); > >>+ > >>+ init_completion(&host->cmd_complete); > > > >You need to run init_completion() *before* you kick off your I/O. > >Otherwise, your interrupt handler is racing with this function. > > > > Do you mean that it needs put init_completion() before hinfc_write()? > If not so, interrupt handler maybe execute before init_completion() > which will cause something wrong. Yes. init_completion(&host->cmd_complete); should come sometime before hinfc_write(host, val, HINFC504_DMA_CTRL); > >>+ ret = wait_for_completion_timeout(&host->cmd_complete, > >>+ HINFC504_NFC_DMA_TIMEOUT); [...] > >>+static void set_addr(struct mtd_info *mtd, int column, int page_addr) > >>+{ > >>+ struct nand_chip *chip = mtd->priv; > >>+ struct hinfc_host *host = chip->priv; > >>+ > >>+ host->addr_cycle = 0; > >>+ host->addr_value[0] = 0; > >>+ host->addr_value[1] = 0; > >>+ > >>+ /* Serially input address */ > >>+ if (column != -1) { > >>+ /* Adjust columns for 16 bit buswidth */ > >>+ if (chip->options & NAND_BUSWIDTH_16) > >>+ column >>= 1; > > > >It doesn't look like you're supporting ONFI parameter pages yet, but you > >might consider checking for !nand_opcode_8bits(opcode) before adjusting the > >address. We had to fix some other drivers for this recently. > > > > sorry, could you give me more clue about this? Do you mean that we > should first make sure it is not 8 bits bus width? Look for use of nand_opcode_8bits() in nand_base.c. I'm suggesting that you don't necessarily want to "adjust the column" for all commands; there are some opcodes that take their address only on the lower x8 bits, even if the flash has an x16 buswidth. > >>+ > >>+ host->addr_value[0] = column & 0xffff; > >>+ host->addr_cycle = 2; > >>+ } [...] > >>+ switch (mtd->writesize) { > >>+ case 2048: > >>+ flag |= (0x001 << HINFC504_CON_PAGEISZE_SHIFT); > >>+ /* add more pagesize support > >>+ * default pagesize has been set in hisi_nfc_host_init > >>+ */ > > > >Does this mean you can't handle any other page size? You might want to > >put the words 'TODO' or 'FIXME' in the comment, and you might want to > >print an error and exit if you see some other value for mtd->writesize. > > > > Yes, this NAND controller can handle not only 2048 page size. But > the NAND controller on hip04-d01 board which I used to test this > driver connects with a NAND flash chip which is just 2048 page size. > So here I > just listed 'case 2048', Maybe I need indicate this by 'TODO:...'? Not just a TODO; make sure to also error out, so that users don't get unexplained failures if they find themselves with a non-2KB page size NAND. Brian ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 1/2] mtd: hisilicon: add a new NAND controller driver for hisilicon hip04 Soc 2014-11-04 12:47 ` [PATCH v4 1/2] mtd: hisilicon: add a new NAND controller driver for " Zhou Wang 2014-11-30 9:35 ` Brian Norris @ 2014-12-01 9:25 ` Brian Norris 2014-12-01 13:08 ` Zhou Wang 1 sibling, 1 reply; 26+ messages in thread From: Brian Norris @ 2014-12-01 9:25 UTC (permalink / raw) To: Zhou Wang Cc: mark.rutland, devicetree, pawel.moll, ijc+devicetree, linux-kernel, haojian.zhuang, wangzhou1, robh+dt, linux-mtd, xuwei5, galak, caizhiyong, David Woodhouse I forgot to mention these comments: On Tue, Nov 04, 2014 at 08:47:00PM +0800, Zhou Wang wrote: > +static int hisi_nfc_remove(struct platform_device *pdev) > +{ > + struct hinfc_host *host = platform_get_drvdata(pdev); > + struct mtd_info *mtd = &host->mtd; > + > + nand_release(mtd); > + dma_free_coherent(&pdev->dev, mtd->writesize + mtd->oobsize, > + host->buffer, host->dma_buffer); > + > + return 0; > +} > + > +#ifdef CONFIG_PM_SLEEP > +static int hisi_nfc_suspend(struct platform_device *pdev, > + pm_message_t state) This is the wrong prototype. See struct dev_pm_ops. > +{ > + struct hinfc_host *host = platform_get_drvdata(pdev); > + > + while ((hinfc_read(host, HINFC504_STATUS) & 0x1) == 0x0) > + ; > + > + while ((hinfc_read(host, HINFC504_DMA_CTRL)) > + & HINFC504_DMA_CTRL_DMA_START) > + _cond_resched(); > + > + return 0; > +} > + > +static int hisi_nfc_resume(struct platform_device *pdev) Same here. > +{ > + int cs; > + struct hinfc_host *host = platform_get_drvdata(pdev); > + struct nand_chip *chip = &host->chip; > + > + for (cs = 0; cs < chip->numchips; cs++) > + hisi_nfc_send_cmd_reset(host, cs); > + hinfc_write(host, SET_HINFC504_PWIDTH(HINFC504_W_LATCH, > + HINFC504_R_LATCH, HINFC504_RW_LATCH), HINFC504_PWIDTH); > + > + return 0; > +} > +#endif > +static SIMPLE_DEV_PM_OPS(hisi_nfc_pm_ops, hisi_nfc_suspend, hisi_nfc_resume); Brian ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 1/2] mtd: hisilicon: add a new NAND controller driver for hisilicon hip04 Soc 2014-12-01 9:25 ` Brian Norris @ 2014-12-01 13:08 ` Zhou Wang 0 siblings, 0 replies; 26+ messages in thread From: Zhou Wang @ 2014-12-01 13:08 UTC (permalink / raw) To: Brian Norris Cc: mark.rutland, devicetree, pawel.moll, ijc+devicetree, linux-kernel, haojian.zhuang, wangzhou1, robh+dt, linux-mtd, xuwei5, galak, caizhiyong, David Woodhouse On 2014年12月01日 17:25, Brian Norris wrote: > I forgot to mention these comments: > > On Tue, Nov 04, 2014 at 08:47:00PM +0800, Zhou Wang wrote: >> +static int hisi_nfc_remove(struct platform_device *pdev) >> +{ >> + struct hinfc_host *host = platform_get_drvdata(pdev); >> + struct mtd_info *mtd = &host->mtd; >> + >> + nand_release(mtd); >> + dma_free_coherent(&pdev->dev, mtd->writesize + mtd->oobsize, >> + host->buffer, host->dma_buffer); >> + >> + return 0; >> +} >> + >> +#ifdef CONFIG_PM_SLEEP >> +static int hisi_nfc_suspend(struct platform_device *pdev, >> + pm_message_t state) > > This is the wrong prototype. See struct dev_pm_ops. > My mistake here, will modify this, thanks a lot!! >> +{ >> + struct hinfc_host *host = platform_get_drvdata(pdev); >> + >> + while ((hinfc_read(host, HINFC504_STATUS) & 0x1) == 0x0) >> + ; >> + >> + while ((hinfc_read(host, HINFC504_DMA_CTRL)) >> + & HINFC504_DMA_CTRL_DMA_START) >> + _cond_resched(); >> + >> + return 0; >> +} >> + >> +static int hisi_nfc_resume(struct platform_device *pdev) > > Same here. > >> +{ >> + int cs; >> + struct hinfc_host *host = platform_get_drvdata(pdev); >> + struct nand_chip *chip = &host->chip; >> + >> + for (cs = 0; cs < chip->numchips; cs++) >> + hisi_nfc_send_cmd_reset(host, cs); >> + hinfc_write(host, SET_HINFC504_PWIDTH(HINFC504_W_LATCH, >> + HINFC504_R_LATCH, HINFC504_RW_LATCH), HINFC504_PWIDTH); >> + >> + return 0; >> +} >> +#endif >> +static SIMPLE_DEV_PM_OPS(hisi_nfc_pm_ops, hisi_nfc_suspend, hisi_nfc_resume); > > Brian > Thanks, Zhou Wang ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v4 2/2] mtd: hisilicon: add device tree binding documentation 2014-11-04 12:46 [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc Zhou Wang 2014-11-04 12:47 ` [PATCH v4 1/2] mtd: hisilicon: add a new NAND controller driver for " Zhou Wang @ 2014-11-04 12:47 ` Zhou Wang 2014-11-30 8:56 ` Brian Norris 2014-11-30 9:01 ` Brian Norris 2014-11-06 3:05 ` [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc Haojian Zhuang ` (2 subsequent siblings) 4 siblings, 2 replies; 26+ messages in thread From: Zhou Wang @ 2014-11-04 12:47 UTC (permalink / raw) To: David Woodhouse, Brian Norris, linux-mtd, devicetree Cc: mark.rutland, Zhou Wang, pawel.moll, ijc+devicetree, linux-kernel, haojian.zhuang, wangzhou1, robh+dt, xuwei5, galak, caizhiyong Signed-off-by: Zhou Wang <wangzhou.bry@gmail.com> --- .../devicetree/bindings/mtd/hisi504-nand.txt | 40 ++++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 Documentation/devicetree/bindings/mtd/hisi504-nand.txt diff --git a/Documentation/devicetree/bindings/mtd/hisi504-nand.txt b/Documentation/devicetree/bindings/mtd/hisi504-nand.txt new file mode 100644 index 0000000..c8b3988 --- /dev/null +++ b/Documentation/devicetree/bindings/mtd/hisi504-nand.txt @@ -0,0 +1,40 @@ +Hisilicon Hip04 Soc NAND controller DT binding + +Required properties: +- compatible: Should be "hisilicon,504-nfc". +- reg: The first contains base physical address and size of + NAND controller's registers. The second contains base + physical address and size of NAND controller's buffer. +- interrupts: Interrupt number for nfc. +- nand-bus-width: See nand.txt. +- nand-ecc-mode: See nand.txt. +- hisi,nand-ecc-bits: ECC bits type support. + <0>: none ecc + <1>: Can correct 1bit per 512byte. + <6>: Can correct 16bits per 1K byte. +- #address-cells: partition address, should be set 1. +- #size-cells: partition size, should be set 1. + +Flash chip may optionally contain additional sub-nodes describing partitions of +the address space. See partition.txt for more detail. + +Example: + + nand: nand@4020000 { + compatible = "hisilicon,504-nfc"; + reg = <0x4020000 0x10000>, <0x5000000 0x1000>; + interrupts = <0 379 4>; + nand-bus-width = <8>; + nand-ecc-mode = "hw"; + hisi,nand-ecc-bits = <1>; + #address-cells = <1>; + #size-cells = <1>; + + partition@0 { + label = "nand_text"; + reg = <0x00000000 0x00400000>; + }; + + ... + + }; -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v4 2/2] mtd: hisilicon: add device tree binding documentation 2014-11-04 12:47 ` [PATCH v4 2/2] mtd: hisilicon: add device tree binding documentation Zhou Wang @ 2014-11-30 8:56 ` Brian Norris 2014-12-01 13:08 ` Zhou Wang 2014-11-30 9:01 ` Brian Norris 1 sibling, 1 reply; 26+ messages in thread From: Brian Norris @ 2014-11-30 8:56 UTC (permalink / raw) To: Zhou Wang Cc: mark.rutland, devicetree, pawel.moll, ijc+devicetree, linux-kernel, haojian.zhuang, wangzhou1, robh+dt, linux-mtd, xuwei5, galak, caizhiyong, David Woodhouse On Tue, Nov 04, 2014 at 08:47:01PM +0800, Zhou Wang wrote: > Signed-off-by: Zhou Wang <wangzhou.bry@gmail.com> > --- > .../devicetree/bindings/mtd/hisi504-nand.txt | 40 ++++++++++++++++++++ > 1 file changed, 40 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mtd/hisi504-nand.txt > > diff --git a/Documentation/devicetree/bindings/mtd/hisi504-nand.txt b/Documentation/devicetree/bindings/mtd/hisi504-nand.txt > new file mode 100644 > index 0000000..c8b3988 > --- /dev/null > +++ b/Documentation/devicetree/bindings/mtd/hisi504-nand.txt > @@ -0,0 +1,40 @@ > +Hisilicon Hip04 Soc NAND controller DT binding > + > +Required properties: > +- compatible: Should be "hisilicon,504-nfc". > +- reg: The first contains base physical address and size of > + NAND controller's registers. The second contains base > + physical address and size of NAND controller's buffer. > +- interrupts: Interrupt number for nfc. > +- nand-bus-width: See nand.txt. > +- nand-ecc-mode: See nand.txt. Do you support all modes, or just "hw"? Might be worth noting here. > +- hisi,nand-ecc-bits: ECC bits type support. > + <0>: none ecc > + <1>: Can correct 1bit per 512byte. > + <6>: Can correct 16bits per 1K byte. You should re-use the nand-ecc-strength and nand-ecc-step-size properties here. So you'll support these options: nand-ecc-strength=0 nand-ecc-step-size=<don't care> nand-ecc-strength=1 nand-ecc-step-size=512 nand-ecc-strength=16 nand-ecc-step-size=1024 > +- #address-cells: partition address, should be set 1. > +- #size-cells: partition size, should be set 1. > + > +Flash chip may optionally contain additional sub-nodes describing partitions of > +the address space. See partition.txt for more detail. > + > +Example: > + > + nand: nand@4020000 { > + compatible = "hisilicon,504-nfc"; > + reg = <0x4020000 0x10000>, <0x5000000 0x1000>; > + interrupts = <0 379 4>; > + nand-bus-width = <8>; > + nand-ecc-mode = "hw"; > + hisi,nand-ecc-bits = <1>; > + #address-cells = <1>; > + #size-cells = <1>; > + > + partition@0 { > + label = "nand_text"; > + reg = <0x00000000 0x00400000>; > + }; > + > + ... > + > + }; Brian ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 2/2] mtd: hisilicon: add device tree binding documentation 2014-11-30 8:56 ` Brian Norris @ 2014-12-01 13:08 ` Zhou Wang 0 siblings, 0 replies; 26+ messages in thread From: Zhou Wang @ 2014-12-01 13:08 UTC (permalink / raw) To: Brian Norris Cc: mark.rutland, devicetree, pawel.moll, ijc+devicetree, linux-kernel, haojian.zhuang, wangzhou1, robh+dt, linux-mtd, xuwei5, galak, caizhiyong, David Woodhouse On 2014年11月30日 16:56, Brian Norris wrote: > On Tue, Nov 04, 2014 at 08:47:01PM +0800, Zhou Wang wrote: >> Signed-off-by: Zhou Wang <wangzhou.bry@gmail.com> >> --- >> .../devicetree/bindings/mtd/hisi504-nand.txt | 40 ++++++++++++++++++++ >> 1 file changed, 40 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/mtd/hisi504-nand.txt >> >> diff --git a/Documentation/devicetree/bindings/mtd/hisi504-nand.txt b/Documentation/devicetree/bindings/mtd/hisi504-nand.txt >> new file mode 100644 >> index 0000000..c8b3988 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/mtd/hisi504-nand.txt >> @@ -0,0 +1,40 @@ >> +Hisilicon Hip04 Soc NAND controller DT binding >> + >> +Required properties: >> +- compatible: Should be "hisilicon,504-nfc". >> +- reg: The first contains base physical address and size of >> + NAND controller's registers. The second contains base >> + physical address and size of NAND controller's buffer. >> +- interrupts: Interrupt number for nfc. >> +- nand-bus-width: See nand.txt. >> +- nand-ecc-mode: See nand.txt. > > Do you support all modes, or just "hw"? Might be worth noting here. > The driver just supports "hw" mode, will modify this. >> +- hisi,nand-ecc-bits: ECC bits type support. >> + <0>: none ecc >> + <1>: Can correct 1bit per 512byte. >> + <6>: Can correct 16bits per 1K byte. > > You should re-use the nand-ecc-strength and nand-ecc-step-size > properties here. So you'll support these options: > > nand-ecc-strength=0 nand-ecc-step-size=<don't care> > nand-ecc-strength=1 nand-ecc-step-size=512 > nand-ecc-strength=16 nand-ecc-step-size=1024 Thanks, will modify this! > >> +- #address-cells: partition address, should be set 1. >> +- #size-cells: partition size, should be set 1. >> + >> +Flash chip may optionally contain additional sub-nodes describing partitions of >> +the address space. See partition.txt for more detail. >> + >> +Example: >> + >> + nand: nand@4020000 { >> + compatible = "hisilicon,504-nfc"; >> + reg = <0x4020000 0x10000>, <0x5000000 0x1000>; >> + interrupts = <0 379 4>; >> + nand-bus-width = <8>; >> + nand-ecc-mode = "hw"; >> + hisi,nand-ecc-bits = <1>; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + >> + partition@0 { >> + label = "nand_text"; >> + reg = <0x00000000 0x00400000>; >> + }; >> + >> + ... >> + >> + }; > > Brian > Thanks, Zhou Wang ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 2/2] mtd: hisilicon: add device tree binding documentation 2014-11-04 12:47 ` [PATCH v4 2/2] mtd: hisilicon: add device tree binding documentation Zhou Wang 2014-11-30 8:56 ` Brian Norris @ 2014-11-30 9:01 ` Brian Norris 2014-12-01 13:07 ` Zhou Wang 1 sibling, 1 reply; 26+ messages in thread From: Brian Norris @ 2014-11-30 9:01 UTC (permalink / raw) To: Zhou Wang Cc: mark.rutland, devicetree, pawel.moll, ijc+devicetree, linux-kernel, haojian.zhuang, wangzhou1, robh+dt, linux-mtd, xuwei5, galak, caizhiyong, David Woodhouse One more thing: On Tue, Nov 04, 2014 at 08:47:01PM +0800, Zhou Wang wrote: > diff --git a/Documentation/devicetree/bindings/mtd/hisi504-nand.txt b/Documentation/devicetree/bindings/mtd/hisi504-nand.txt > new file mode 100644 > index 0000000..c8b3988 > --- /dev/null > +++ b/Documentation/devicetree/bindings/mtd/hisi504-nand.txt > @@ -0,0 +1,40 @@ > +Hisilicon Hip04 Soc NAND controller DT binding > + > +Required properties: > +- compatible: Should be "hisilicon,504-nfc". I notice you use "NFC" naming throughout. I know others have had complaints about overloading the acronym NFC which sometimes means "Near Field Communication", instead of the intended "NAND Flash Controller." Is there any kind of naming convention within Hisilicon documentation that insists on calling this NFC, or can we avoid that confusing name? If possible, I'd suggest "hisilicon,504-nand" as an alternative. Brian ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 2/2] mtd: hisilicon: add device tree binding documentation 2014-11-30 9:01 ` Brian Norris @ 2014-12-01 13:07 ` Zhou Wang 0 siblings, 0 replies; 26+ messages in thread From: Zhou Wang @ 2014-12-01 13:07 UTC (permalink / raw) To: Brian Norris Cc: mark.rutland, devicetree, pawel.moll, ijc+devicetree, linux-kernel, haojian.zhuang, wangzhou1, robh+dt, linux-mtd, xuwei5, galak, caizhiyong, David Woodhouse On 2014年11月30日 17:01, Brian Norris wrote: > One more thing: > > On Tue, Nov 04, 2014 at 08:47:01PM +0800, Zhou Wang wrote: >> diff --git a/Documentation/devicetree/bindings/mtd/hisi504-nand.txt b/Documentation/devicetree/bindings/mtd/hisi504-nand.txt >> new file mode 100644 >> index 0000000..c8b3988 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/mtd/hisi504-nand.txt >> @@ -0,0 +1,40 @@ >> +Hisilicon Hip04 Soc NAND controller DT binding >> + >> +Required properties: >> +- compatible: Should be "hisilicon,504-nfc". > > I notice you use "NFC" naming throughout. I know others have had > complaints about overloading the acronym NFC which sometimes means "Near > Field Communication", instead of the intended "NAND Flash Controller." > Is there any kind of naming convention within Hisilicon documentation > that insists on calling this NFC, or can we avoid that confusing name? > > If possible, I'd suggest "hisilicon,504-nand" as an alternative. > I think that is a good idear to call "hisilicon,504-nand", in fact, at the beginning, I also confused with the name "nfc". > Brian > Thanks, Zhou Wang ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc 2014-11-04 12:46 [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc Zhou Wang 2014-11-04 12:47 ` [PATCH v4 1/2] mtd: hisilicon: add a new NAND controller driver for " Zhou Wang 2014-11-04 12:47 ` [PATCH v4 2/2] mtd: hisilicon: add device tree binding documentation Zhou Wang @ 2014-11-06 3:05 ` Haojian Zhuang 2014-11-26 6:35 ` Haojian Zhuang 2014-11-30 9:08 ` Brian Norris 4 siblings, 0 replies; 26+ messages in thread From: Haojian Zhuang @ 2014-11-06 3:05 UTC (permalink / raw) To: Zhou Wang Cc: Mark Rutland, devicetree, Pawel Moll, Ian Campbell, linux-kernel@vger.kernel.org, xuwei (O), wangzhou1, Rob Herring, linux-mtd, Kumar Gala, caizhiyong, Brian Norris, David Woodhouse On Tue, Nov 4, 2014 at 8:46 PM, Zhou Wang <wangzhou.bry@gmail.com> wrote: > This patchset adds the support for NAND controller of hisilicon hip04 Soc. > The NAND controller IP was developed by hisilicon and needs a new driver to > support it. This patchset is based on v3.18-rc1. I have tested that NAND flash > controller works fine in Hip04 D01 board. > > Changes in v4: > - add mtd->dev.parent = &pdev->dev, thanks Frans Klaver. > Changes in v3: > - Modify code to eliminate some code style warnings. > - add ecc-bits input check. > - avoid using waterfall style in hisi_nfc_cmdfunc(). > Changes in v2: > - Remove the patch for device tree, now patchset only has the driver and its > device tree binding documentation. > - Change the file name: hisi_nand.c to hisi504_nand.c. > Changes in v1: > - Remove callback functions out of struct hinfc_host, and call them directly > in relative functions. > - Change hinfc_read and hinfc_write from macros to inline functions. > - Instead of putting pointers, embed struct nand_chip and struct mtd_info in > struct hinfc_host directly. > - rewrite some unclear lines in device tree binding document, correct some > code style error. > > Link on v3: > - https://lkml.org/lkml/2014/10/28/386 > Link on v2: > - https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg750071.html > Link on v1: > - https://lkml.org/lkml/2014/7/15/198 > > Zhou Wang (2): > mtd: hisilicon: add a new NAND controller driver for hisilicon hip04 Soc > mtd: hisilicon: add device tree binding documentation > > .../devicetree/bindings/mtd/hisi504-nand.txt | 40 + > drivers/mtd/nand/Kconfig | 5 + > drivers/mtd/nand/Makefile | 1 + > drivers/mtd/nand/hisi504_nand.c | 846 ++++++++++++++++++++ > 4 files changed, 892 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mtd/hisi504-nand.txt > create mode 100644 drivers/mtd/nand/hisi504_nand.c > > -- > 1.7.9.5 > Acked-by: Haojian Zhuang <haojian.zhuang@gmail.com> ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc 2014-11-04 12:46 [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc Zhou Wang ` (2 preceding siblings ...) 2014-11-06 3:05 ` [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc Haojian Zhuang @ 2014-11-26 6:35 ` Haojian Zhuang 2014-11-30 9:08 ` Brian Norris 4 siblings, 0 replies; 26+ messages in thread From: Haojian Zhuang @ 2014-11-26 6:35 UTC (permalink / raw) To: Zhou Wang Cc: Mark Rutland, devicetree, Pawel Moll, Ian Campbell, linux-kernel@vger.kernel.org, xuwei (O), wangzhou1, Rob Herring, linux-mtd, Kumar Gala, caizhiyong, Brian Norris, David Woodhouse On Tue, Nov 4, 2014 at 8:46 PM, Zhou Wang <wangzhou.bry@gmail.com> wrote: > This patchset adds the support for NAND controller of hisilicon hip04 Soc. > The NAND controller IP was developed by hisilicon and needs a new driver to > support it. This patchset is based on v3.18-rc1. I have tested that NAND flash > controller works fine in Hip04 D01 board. > > Changes in v4: > - add mtd->dev.parent = &pdev->dev, thanks Frans Klaver. > Changes in v3: > - Modify code to eliminate some code style warnings. > - add ecc-bits input check. > - avoid using waterfall style in hisi_nfc_cmdfunc(). > Changes in v2: > - Remove the patch for device tree, now patchset only has the driver and its > device tree binding documentation. > - Change the file name: hisi_nand.c to hisi504_nand.c. > Changes in v1: > - Remove callback functions out of struct hinfc_host, and call them directly > in relative functions. > - Change hinfc_read and hinfc_write from macros to inline functions. > - Instead of putting pointers, embed struct nand_chip and struct mtd_info in > struct hinfc_host directly. > - rewrite some unclear lines in device tree binding document, correct some > code style error. > > Link on v3: > - https://lkml.org/lkml/2014/10/28/386 > Link on v2: > - https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg750071.html > Link on v1: > - https://lkml.org/lkml/2014/7/15/198 > > Zhou Wang (2): > mtd: hisilicon: add a new NAND controller driver for hisilicon hip04 Soc > mtd: hisilicon: add device tree binding documentation > > .../devicetree/bindings/mtd/hisi504-nand.txt | 40 + > drivers/mtd/nand/Kconfig | 5 + > drivers/mtd/nand/Makefile | 1 + > drivers/mtd/nand/hisi504_nand.c | 846 ++++++++++++++++++++ > 4 files changed, 892 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mtd/hisi504-nand.txt > create mode 100644 drivers/mtd/nand/hisi504_nand.c > > -- > 1.7.9.5 > Hi David & Brian, How do you think about this nand patch set? Could it be merged into v3.19? Best Regards Haojian ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc 2014-11-04 12:46 [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc Zhou Wang ` (3 preceding siblings ...) 2014-11-26 6:35 ` Haojian Zhuang @ 2014-11-30 9:08 ` Brian Norris 2014-12-01 13:06 ` Zhou Wang 2014-12-11 12:02 ` Zhou Wang 4 siblings, 2 replies; 26+ messages in thread From: Brian Norris @ 2014-11-30 9:08 UTC (permalink / raw) To: Zhou Wang Cc: mark.rutland, devicetree, pawel.moll, ijc+devicetree, linux-kernel, haojian.zhuang, wangzhou1, robh+dt, linux-mtd, xuwei5, galak, caizhiyong, David Woodhouse On Tue, Nov 04, 2014 at 08:46:59PM +0800, Zhou Wang wrote: > This patchset adds the support for NAND controller of hisilicon hip04 Soc. > The NAND controller IP was developed by hisilicon and needs a new driver to > support it. This patchset is based on v3.18-rc1. I have tested that NAND flash > controller works fine in Hip04 D01 board. Have you tested on the MTD test modules (drivers/mtd/tests/*)? This is important, as we seem to regularly get UBIFS bug reports from users whose drivers have not even passed some of the simple tests. Also, it might be worth testing out the UBI tests found in the mtd-utils package. Brian ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc 2014-11-30 9:08 ` Brian Norris @ 2014-12-01 13:06 ` Zhou Wang 2014-12-11 12:02 ` Zhou Wang 1 sibling, 0 replies; 26+ messages in thread From: Zhou Wang @ 2014-12-01 13:06 UTC (permalink / raw) To: Brian Norris Cc: mark.rutland, devicetree, pawel.moll, ijc+devicetree, linux-kernel, haojian.zhuang, wangzhou1, robh+dt, linux-mtd, xuwei5, galak, caizhiyong, David Woodhouse On 2014年11月30日 17:08, Brian Norris wrote: > On Tue, Nov 04, 2014 at 08:46:59PM +0800, Zhou Wang wrote: >> This patchset adds the support for NAND controller of hisilicon hip04 Soc. >> The NAND controller IP was developed by hisilicon and needs a new driver to >> support it. This patchset is based on v3.18-rc1. I have tested that NAND flash >> controller works fine in Hip04 D01 board. > > Have you tested on the MTD test modules (drivers/mtd/tests/*)? This is > important, as we seem to regularly get UBIFS bug reports from users > whose drivers have not even passed some of the simple tests. > > Also, it might be worth testing out the UBI tests found in the mtd-utils > package. > > Brian > Hi Brian, I have not tested on the MTD test modules, I will do this test and the UBI tests ASAP. Thank you very much for so many helpful comments! Best Regards, Zhou Wang ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc 2014-11-30 9:08 ` Brian Norris 2014-12-01 13:06 ` Zhou Wang @ 2014-12-11 12:02 ` Zhou Wang 2014-12-17 1:03 ` Brian Norris 2014-12-17 6:23 ` Brian Norris 1 sibling, 2 replies; 26+ messages in thread From: Zhou Wang @ 2014-12-11 12:02 UTC (permalink / raw) To: Brian Norris Cc: mark.rutland, devicetree, pawel.moll, ijc+devicetree, liguozhu, haojian.zhuang, wangzhou1, robh+dt, linux-mtd, xuwei5, galak, caizhiyong, yubingxu, David Woodhouse On 2014年11月30日 17:08, Brian Norris wrote: > On Tue, Nov 04, 2014 at 08:46:59PM +0800, Zhou Wang wrote: >> This patchset adds the support for NAND controller of hisilicon hip04 Soc. >> The NAND controller IP was developed by hisilicon and needs a new driver to >> support it. This patchset is based on v3.18-rc1. I have tested that NAND flash >> controller works fine in Hip04 D01 board. > > Have you tested on the MTD test modules (drivers/mtd/tests/*)? This is > important, as we seem to regularly get UBIFS bug reports from users > whose drivers have not even passed some of the simple tests. > > Also, it might be worth testing out the UBI tests found in the mtd-utils > package. > > Brian > Hi Brian, I have tested this NAND controller driver on the MTD test modules (drivers/mtd/tests/*). All tests passed except mtd_nandbiterrs.ko. Here is the test log for mtd_nandbiterrs: /home # insmod mtd_nandbiterrs.ko dev=2 page_offset=1 seed=110 mode=0 [ 100.484995] [ 100.490082] ================================================== [ 100.509657] mtd_nandbiterrs: MTD device: 2 [ 100.523413] mtd_nandbiterrs: MTD device size 8388608, eraseblock=131072, page=2048, oob=64 [ 100.551134] mtd_nandbiterrs: Device uses 2 subpages of 1024 bytes [ 100.571585] mtd_nandbiterrs: Using page=1, offset=2048, eraseblock=0 [ 104.431136] mtd_nandbiterrs: incremental biterrors test [ 104.448872] mtd_nandbiterrs: write_page [ 104.463193] mtd_nandbiterrs: rewrite page [ 104.477620] mtd_nandbiterrs: read_page [ 104.490898] mtd_nandbiterrs: verify_page [ 104.504338] mtd_nandbiterrs: Successfully corrected 0 bit errors per subpage [ 104.527985] mtd_nandbiterrs: Inserted biterror @ 0/5 [ 104.544673] mtd_nandbiterrs: Inserted biterror @ 1024/2 [ 104.562197] mtd_nandbiterrs: rewrite page [ 104.576766] mtd_nandbiterrs: read_page [ 104.590052] mtd_nandbiterrs: verify_page [ 104.603252] mtd_nandbiterrs: Error: page offset 0, expected 23, got 03 [ 104.625203] mtd_nandbiterrs: Error: page offset 1024, expected 06, got 02 [ 104.648056] mtd_nandbiterrs: ECC failure, read data is incorrect despite read success insmod: can't insert 'mtd_nandbiterrs.ko': Input/output error The reason for above failure is that: In ECC mode, when rewriting page data to NAND flash, the NAND controller will also produce ECC code and write them to NAND flash as well. So when we read data from NAND flash, there is no need to correct the error bit. We read what we write to the flash. In mtd_nandbiterrs test, We call nand_write_page_raw indeed to perform rewrite operation. My question is that: in this NAND controller hardware design, it is hard to implement hardware specific write_page_raw to write page data without producing ECC code, will this bring some bad effects somewhere? It will be very nice if you and anyone can give me some advice. Thanks, Zhou Wang ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc 2014-12-11 12:02 ` Zhou Wang @ 2014-12-17 1:03 ` Brian Norris 2014-12-17 5:06 ` Zhou Wang 2014-12-17 6:23 ` Brian Norris 1 sibling, 1 reply; 26+ messages in thread From: Brian Norris @ 2014-12-17 1:03 UTC (permalink / raw) To: Zhou Wang Cc: mark.rutland, devicetree, pawel.moll, ijc+devicetree, liguozhu, haojian.zhuang, wangzhou1, robh+dt, linux-mtd, xuwei5, galak, caizhiyong, yubingxu, David Woodhouse On Thu, Dec 11, 2014 at 08:02:27PM +0800, Zhou Wang wrote: > On 2014年11月30日 17:08, Brian Norris wrote: > >Have you tested on the MTD test modules (drivers/mtd/tests/*)? This is > >important, as we seem to regularly get UBIFS bug reports from users > >whose drivers have not even passed some of the simple tests. > > > >Also, it might be worth testing out the UBI tests found in the mtd-utils > >package. > > I have tested this NAND controller driver on the MTD test modules > (drivers/mtd/tests/*). All tests passed except mtd_nandbiterrs.ko. > > Here is the test log for mtd_nandbiterrs: > /home # insmod mtd_nandbiterrs.ko dev=2 page_offset=1 seed=110 mode=0 > [ 100.484995] > [ 100.490082] ================================================== > [ 100.509657] mtd_nandbiterrs: MTD device: 2 > [ 100.523413] mtd_nandbiterrs: MTD device size 8388608, > eraseblock=131072, page=2048, oob=64 > [ 100.551134] mtd_nandbiterrs: Device uses 2 subpages of 1024 bytes > [ 100.571585] mtd_nandbiterrs: Using page=1, offset=2048, eraseblock=0 > [ 104.431136] mtd_nandbiterrs: incremental biterrors test > [ 104.448872] mtd_nandbiterrs: write_page > [ 104.463193] mtd_nandbiterrs: rewrite page > [ 104.477620] mtd_nandbiterrs: read_page > [ 104.490898] mtd_nandbiterrs: verify_page > [ 104.504338] mtd_nandbiterrs: Successfully corrected 0 bit errors > per subpage > [ 104.527985] mtd_nandbiterrs: Inserted biterror @ 0/5 > [ 104.544673] mtd_nandbiterrs: Inserted biterror @ 1024/2 > [ 104.562197] mtd_nandbiterrs: rewrite page > [ 104.576766] mtd_nandbiterrs: read_page > [ 104.590052] mtd_nandbiterrs: verify_page > [ 104.603252] mtd_nandbiterrs: Error: page offset 0, expected 23, got 03 > [ 104.625203] mtd_nandbiterrs: Error: page offset 1024, expected 06, got 02 > [ 104.648056] mtd_nandbiterrs: ECC failure, read data is incorrect > despite read success > insmod: can't insert 'mtd_nandbiterrs.ko': Input/output error > > The reason for above failure is that: > In ECC mode, when rewriting page data to NAND flash, the NAND > controller will also produce ECC code and write them to NAND flash > as well. So when we read data from NAND flash, there is no need to > correct the error bit. We read what we write to the flash. > > In mtd_nandbiterrs test, We call nand_write_page_raw indeed to perform > rewrite operation. My question is that: in this NAND controller hardware > design, it is hard to implement hardware specific write_page_raw to > write page data without producing ECC code, will this bring some bad > effects somewhere? It will be very nice if you and anyone can give > me some advice. As of now, read_page_raw()/write_page_raw() are not required for any normal operation. They are useful for debugging and testing though, with tests like this one. In the future, we might use read_page_raw() for implementing (optional) software-based detection of blank/erased pages that have bitflips in them -- i.e., all 0xff but with a few bitflips. See: http://lists.infradead.org/pipermail/linux-mtd/2014-December/056749.html http://article.gmane.org/gmane.linux.drivers.mtd/52183/ Brian ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc 2014-12-17 1:03 ` Brian Norris @ 2014-12-17 5:06 ` Zhou Wang 0 siblings, 0 replies; 26+ messages in thread From: Zhou Wang @ 2014-12-17 5:06 UTC (permalink / raw) To: Brian Norris Cc: mark.rutland, devicetree, pawel.moll, ijc+devicetree, liguozhu, haojian.zhuang, wangzhou1, robh+dt, linux-mtd, xuwei5, galak, caizhiyong, yubingxu, David Woodhouse On 2014年12月17日 09:03, Brian Norris wrote: > On Thu, Dec 11, 2014 at 08:02:27PM +0800, Zhou Wang wrote: >> On 2014年11月30日 17:08, Brian Norris wrote: >>> Have you tested on the MTD test modules (drivers/mtd/tests/*)? This is >>> important, as we seem to regularly get UBIFS bug reports from users >>> whose drivers have not even passed some of the simple tests. >>> >>> Also, it might be worth testing out the UBI tests found in the mtd-utils >>> package. >> >> I have tested this NAND controller driver on the MTD test modules >> (drivers/mtd/tests/*). All tests passed except mtd_nandbiterrs.ko. >> >> Here is the test log for mtd_nandbiterrs: >> /home # insmod mtd_nandbiterrs.ko dev=2 page_offset=1 seed=110 mode=0 >> [ 100.484995] >> [ 100.490082] ================================================== >> [ 100.509657] mtd_nandbiterrs: MTD device: 2 >> [ 100.523413] mtd_nandbiterrs: MTD device size 8388608, >> eraseblock=131072, page=2048, oob=64 >> [ 100.551134] mtd_nandbiterrs: Device uses 2 subpages of 1024 bytes >> [ 100.571585] mtd_nandbiterrs: Using page=1, offset=2048, eraseblock=0 >> [ 104.431136] mtd_nandbiterrs: incremental biterrors test >> [ 104.448872] mtd_nandbiterrs: write_page >> [ 104.463193] mtd_nandbiterrs: rewrite page >> [ 104.477620] mtd_nandbiterrs: read_page >> [ 104.490898] mtd_nandbiterrs: verify_page >> [ 104.504338] mtd_nandbiterrs: Successfully corrected 0 bit errors >> per subpage >> [ 104.527985] mtd_nandbiterrs: Inserted biterror @ 0/5 >> [ 104.544673] mtd_nandbiterrs: Inserted biterror @ 1024/2 >> [ 104.562197] mtd_nandbiterrs: rewrite page >> [ 104.576766] mtd_nandbiterrs: read_page >> [ 104.590052] mtd_nandbiterrs: verify_page >> [ 104.603252] mtd_nandbiterrs: Error: page offset 0, expected 23, got 03 >> [ 104.625203] mtd_nandbiterrs: Error: page offset 1024, expected 06, got 02 >> [ 104.648056] mtd_nandbiterrs: ECC failure, read data is incorrect >> despite read success >> insmod: can't insert 'mtd_nandbiterrs.ko': Input/output error >> >> The reason for above failure is that: >> In ECC mode, when rewriting page data to NAND flash, the NAND >> controller will also produce ECC code and write them to NAND flash >> as well. So when we read data from NAND flash, there is no need to >> correct the error bit. We read what we write to the flash. >> >> In mtd_nandbiterrs test, We call nand_write_page_raw indeed to perform >> rewrite operation. My question is that: in this NAND controller hardware >> design, it is hard to implement hardware specific write_page_raw to >> write page data without producing ECC code, will this bring some bad >> effects somewhere? It will be very nice if you and anyone can give >> me some advice. > > As of now, read_page_raw()/write_page_raw() are not required for any > normal operation. They are useful for debugging and testing though, with > tests like this one. > > In the future, we might use read_page_raw() for implementing (optional) > software-based detection of blank/erased pages that have bitflips in > them -- i.e., all 0xff but with a few bitflips. See: > > http://lists.infradead.org/pipermail/linux-mtd/2014-December/056749.html > http://article.gmane.org/gmane.linux.drivers.mtd/52183/ > > Brian > Got it, thanks for your explanation, Brian. I will resend the new version of patchset based on v3.19-rc1 later. Regards, Zhou ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc 2014-12-11 12:02 ` Zhou Wang 2014-12-17 1:03 ` Brian Norris @ 2014-12-17 6:23 ` Brian Norris 2014-12-17 11:05 ` Zhou Wang 1 sibling, 1 reply; 26+ messages in thread From: Brian Norris @ 2014-12-17 6:23 UTC (permalink / raw) To: Zhou Wang Cc: mark.rutland, devicetree, Iwo Mergler, pawel.moll, ijc+devicetree, liguozhu, haojian.zhuang, wangzhou1, robh+dt, linux-mtd, xuwei5, galak, caizhiyong, yubingxu, David Woodhouse On Thu, Dec 11, 2014 at 08:02:27PM +0800, Zhou Wang wrote: > I have tested this NAND controller driver on the MTD test modules > (drivers/mtd/tests/*). All tests passed except mtd_nandbiterrs.ko. > > Here is the test log for mtd_nandbiterrs: > /home # insmod mtd_nandbiterrs.ko dev=2 page_offset=1 seed=110 mode=0 > [ 100.484995] > [ 100.490082] ================================================== > [ 100.509657] mtd_nandbiterrs: MTD device: 2 > [ 100.523413] mtd_nandbiterrs: MTD device size 8388608, > eraseblock=131072, page=2048, oob=64 > [ 100.551134] mtd_nandbiterrs: Device uses 2 subpages of 1024 bytes > [ 100.571585] mtd_nandbiterrs: Using page=1, offset=2048, eraseblock=0 > [ 104.431136] mtd_nandbiterrs: incremental biterrors test > [ 104.448872] mtd_nandbiterrs: write_page > [ 104.463193] mtd_nandbiterrs: rewrite page > [ 104.477620] mtd_nandbiterrs: read_page > [ 104.490898] mtd_nandbiterrs: verify_page > [ 104.504338] mtd_nandbiterrs: Successfully corrected 0 bit errors > per subpage > [ 104.527985] mtd_nandbiterrs: Inserted biterror @ 0/5 > [ 104.544673] mtd_nandbiterrs: Inserted biterror @ 1024/2 > [ 104.562197] mtd_nandbiterrs: rewrite page > [ 104.576766] mtd_nandbiterrs: read_page > [ 104.590052] mtd_nandbiterrs: verify_page > [ 104.603252] mtd_nandbiterrs: Error: page offset 0, expected 23, got 03 > [ 104.625203] mtd_nandbiterrs: Error: page offset 1024, expected 06, got 02 > [ 104.648056] mtd_nandbiterrs: ECC failure, read data is incorrect > despite read success > insmod: can't insert 'mtd_nandbiterrs.ko': Input/output error > > The reason for above failure is that: > In ECC mode, when rewriting page data to NAND flash, the NAND > controller will also produce ECC code and write them to NAND flash > as well. So when we read data from NAND flash, there is no need to > correct the error bit. We read what we write to the flash. BTW, your explanation doesn't seem quite right. The problem is that even though mtd_read() didn't report errors, the data doesn't match what's written. It's not that there was "no need to correct the error bit". I'd recommend digging a little more to figure out what's wrong here. You might need to instrument the nandbiterrs test. This is possibly highlighting a driver bug [1]. Brian [1] Besised simply that you didn't implement write_page_raw(). The default nand_write_page_raw() implementation looks just like your non-raw version. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc 2014-12-17 6:23 ` Brian Norris @ 2014-12-17 11:05 ` Zhou Wang 2015-01-13 4:17 ` Brian Norris 0 siblings, 1 reply; 26+ messages in thread From: Zhou Wang @ 2014-12-17 11:05 UTC (permalink / raw) To: Brian Norris Cc: mark.rutland, devicetree, Iwo Mergler, pawel.moll, ijc+devicetree, liguozhu, haojian.zhuang, wangzhou1, robh+dt, linux-mtd, xuwei5, galak, caizhiyong, yubingxu, David Woodhouse On 2014年12月17日 14:23, Brian Norris wrote: > On Thu, Dec 11, 2014 at 08:02:27PM +0800, Zhou Wang wrote: >> I have tested this NAND controller driver on the MTD test modules >> (drivers/mtd/tests/*). All tests passed except mtd_nandbiterrs.ko. >> >> Here is the test log for mtd_nandbiterrs: >> /home # insmod mtd_nandbiterrs.ko dev=2 page_offset=1 seed=110 mode=0 >> [ 100.484995] >> [ 100.490082] ================================================== >> [ 100.509657] mtd_nandbiterrs: MTD device: 2 >> [ 100.523413] mtd_nandbiterrs: MTD device size 8388608, >> eraseblock=131072, page=2048, oob=64 >> [ 100.551134] mtd_nandbiterrs: Device uses 2 subpages of 1024 bytes >> [ 100.571585] mtd_nandbiterrs: Using page=1, offset=2048, eraseblock=0 >> [ 104.431136] mtd_nandbiterrs: incremental biterrors test >> [ 104.448872] mtd_nandbiterrs: write_page >> [ 104.463193] mtd_nandbiterrs: rewrite page >> [ 104.477620] mtd_nandbiterrs: read_page >> [ 104.490898] mtd_nandbiterrs: verify_page >> [ 104.504338] mtd_nandbiterrs: Successfully corrected 0 bit errors >> per subpage >> [ 104.527985] mtd_nandbiterrs: Inserted biterror @ 0/5 >> [ 104.544673] mtd_nandbiterrs: Inserted biterror @ 1024/2 >> [ 104.562197] mtd_nandbiterrs: rewrite page >> [ 104.576766] mtd_nandbiterrs: read_page >> [ 104.590052] mtd_nandbiterrs: verify_page >> [ 104.603252] mtd_nandbiterrs: Error: page offset 0, expected 23, got 03 >> [ 104.625203] mtd_nandbiterrs: Error: page offset 1024, expected 06, got 02 >> [ 104.648056] mtd_nandbiterrs: ECC failure, read data is incorrect >> despite read success >> insmod: can't insert 'mtd_nandbiterrs.ko': Input/output error >> >> The reason for above failure is that: >> In ECC mode, when rewriting page data to NAND flash, the NAND >> controller will also produce ECC code and write them to NAND flash >> as well. So when we read data from NAND flash, there is no need to >> correct the error bit. We read what we write to the flash. > > BTW, your explanation doesn't seem quite right. The problem is that > even though mtd_read() didn't report errors, the data doesn't match > what's written. It's not that there was "no need to correct the error > bit". Maybe I did not express clearly. In the nandbiterrs test, firstly write data to flash with ECC code in oob area, then change some bits and rewrite data to flash with old ECC code in oob area, at last read data out with ECC to test if the "error bits" can be corrected. My explanation is that in rewriting process NAND controller also produces new ECC code of the data and write both data and new ECC code to flash. So in next step we will get what was writen without "correction". > > I'd recommend digging a little more to figure out what's wrong here. You > might need to instrument the nandbiterrs test. This is possibly Thanks, I will do it. > highlighting a driver bug [1]. > > Brian > > [1] Besised simply that you didn't implement write_page_raw(). The > default nand_write_page_raw() implementation looks just like your > non-raw version. Yes. In ECC mode, as the NAND controller must write page as a whole with ECC code, the default nand_write_page_raw() looks just like non-raw version. Thanks a lot, Zhou > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc 2014-12-17 11:05 ` Zhou Wang @ 2015-01-13 4:17 ` Brian Norris 2015-01-22 3:27 ` Zhou Wang 0 siblings, 1 reply; 26+ messages in thread From: Brian Norris @ 2015-01-13 4:17 UTC (permalink / raw) To: Zhou Wang Cc: mark.rutland, devicetree, Iwo Mergler, pawel.moll, ijc+devicetree, liguozhu, haojian.zhuang, wangzhou1, robh+dt, linux-mtd, xuwei5, galak, caizhiyong, yubingxu, David Woodhouse Following up on this last comment from last year's thread: On Wed, Dec 17, 2014 at 07:05:47PM +0800, Zhou Wang wrote: > On 2014年12月17日 14:23, Brian Norris wrote: [...] > >>[ 104.648056] mtd_nandbiterrs: ECC failure, read data is incorrect > >>despite read success > >>insmod: can't insert 'mtd_nandbiterrs.ko': Input/output error > >> > >>The reason for above failure is that: > >>In ECC mode, when rewriting page data to NAND flash, the NAND > >>controller will also produce ECC code and write them to NAND flash > >>as well. So when we read data from NAND flash, there is no need to > >>correct the error bit. We read what we write to the flash. > > > >BTW, your explanation doesn't seem quite right. The problem is that > >even though mtd_read() didn't report errors, the data doesn't match > >what's written. It's not that there was "no need to correct the error > >bit". > > Maybe I did not express clearly. In the nandbiterrs test, firstly > write data to flash with ECC code in oob area, then change some bits > and rewrite data to flash with old ECC code in oob area, at last read > data out with ECC to test if the "error bits" can be corrected. My > explanation is that in rewriting process NAND controller also produces > new ECC code of the data and write both data and new ECC code to flash. > So in next step we will get what was writen without "correction". But we should at least get an -EBADMSG return status, right? If you're "rewriting" the data, this should result in two sets of data written on top of each other, which (depending on the flash layout charecteristics) might turn up as a kind of logical AND of all the data+OOB. This is "probably" not correctable. But that last "probably" leaves room for the possibility you mentioned, I guess; that the ECC code is just correcting the data to look like the second set of (intentionally) erroneous data. > >I'd recommend digging a little more to figure out what's wrong here. You > >might need to instrument the nandbiterrs test. This is possibly > > Thanks, I will do it. > > >highlighting a driver bug [1]. > > > >Brian > > > >[1] Besised simply that you didn't implement write_page_raw(). The > >default nand_write_page_raw() implementation looks just like your > >non-raw version. > > Yes. In ECC mode, as the NAND controller must write page as a whole > with ECC code, the default nand_write_page_raw() looks just like > non-raw version. Are you saying you cannot implement the raw() hooks for this IP? Or just that you haven't yet? The latter is probably OK for now (I'd recommend doing this, or at least mark a TODO in the code), but the former is a little disturbing. Brian ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc 2015-01-13 4:17 ` Brian Norris @ 2015-01-22 3:27 ` Zhou Wang 2015-01-22 8:45 ` Brian Norris 0 siblings, 1 reply; 26+ messages in thread From: Zhou Wang @ 2015-01-22 3:27 UTC (permalink / raw) To: Brian Norris Cc: mark.rutland, Zhou Wang, Iwo Mergler, pawel.moll, devicetree, liguozhu, ijc+devicetree, haojian.zhuang, robh+dt, linux-mtd, xuwei5, galak, caizhiyong, yubingxu, David Woodhouse Hi Brian, Very sorry for late, I made tests again and also had a talk with the NAND controller hardware colleague. Please find my reply below. On 2015/1/13 12:17, Brian Norris wrote: > Following up on this last comment from last year's thread: > > On Wed, Dec 17, 2014 at 07:05:47PM +0800, Zhou Wang wrote: >> On 2014年12月17日 14:23, Brian Norris wrote: > [...] >>>> [ 104.648056] mtd_nandbiterrs: ECC failure, read data is incorrect >>>> despite read success >>>> insmod: can't insert 'mtd_nandbiterrs.ko': Input/output error >>>> >>>> The reason for above failure is that: >>>> In ECC mode, when rewriting page data to NAND flash, the NAND >>>> controller will also produce ECC code and write them to NAND flash >>>> as well. So when we read data from NAND flash, there is no need to >>>> correct the error bit. We read what we write to the flash. >>> >>> BTW, your explanation doesn't seem quite right. The problem is that >>> even though mtd_read() didn't report errors, the data doesn't match >>> what's written. It's not that there was "no need to correct the error >>> bit". >> >> Maybe I did not express clearly. In the nandbiterrs test, firstly >> write data to flash with ECC code in oob area, then change some bits >> and rewrite data to flash with old ECC code in oob area, at last read >> data out with ECC to test if the "error bits" can be corrected. My >> explanation is that in rewriting process NAND controller also produces >> new ECC code of the data and write both data and new ECC code to flash. >> So in next step we will get what was writen without "correction". > > But we should at least get an -EBADMSG return status, right? If you're > "rewriting" the data, this should result in two sets of data written on > top of each other, which (depending on the flash layout charecteristics) > might turn up as a kind of logical AND of all the data+OOB. This is > "probably" not correctable. > > But that last "probably" leaves room for the possibility you mentioned, > I guess; that the ECC code is just correcting the data to look like the > second set of (intentionally) erroneous data. I made testes again in 1bit/ECC and 16bit/ECC modes using 2K(page)+64B(oob) NAND flash. here are the logs, I also printed ECC code in OOB area. Results are: 1. in 16bit/ECC, it will return -EBADMSG as the ECC codes have been broken. 2. in 1bit/ECC, it will not reture -EBADMSG because a hardware design problem. I will explain the detail below. Test logs: 1. in 16bit/ECC(print ECC codes): /home # insmod mtd_nandbiterrs.ko dev=2 page_offset=1 seed=110 mode=0 ================================================== mtd_nandbiterrs: MTD device: 2 mtd_nandbiterrs: MTD device size 8388608, eraseblock=131072, page=2048, oob=64 mtd_nandbiterrs: Device uses 2 subpages of 1024 bytes mtd_nandbiterrs: Using page=1, offset=2048, eraseblock=0 mtd_nandbiterrs: incremental biterrors test mtd_nandbiterrs: write_page ECC code: 96 ec 7e c0 dc 8e 38 68 e7 29 6a f8 f3 c1 9e 4e 9b 2e b7 31 40 46 88 ec cf 65 55 94 28 07 09 3c c5 1c f6 cb 3e c0 26 d7 cf 2d 15 77 59 e8 5f fd a7 cc be 17 cb ee 39 de mtd_nandbiterrs: rewrite page ECC code: 96 ec 7e c0 dc 8e 38 68 e7 29 6a f8 f3 c1 9e 4e 9b 2e b7 31 40 46 88 ec cf 65 55 94 28 07 09 3c c5 1c f6 cb 3e c0 26 d7 cf 2d 15 77 59 e8 5f fd a7 cc be 17 cb ee 39 de mtd_nandbiterrs: read_page ECC code: 96 ec 7e c0 dc 8e 38 68 e7 29 6a f8 f3 c1 9e 4e 9b 2e b7 31 40 46 88 ec cf 65 55 94 28 07 09 3c c5 1c f6 cb 3e c0 26 d7 cf 2d 15 77 59 e8 5f fd a7 cc be 17 cb ee 39 de mtd_nandbiterrs: verify_page mtd_nandbiterrs: Successfully corrected 0 bit errors per subpage mtd_nandbiterrs: Inserted biterror @ 0/5 mtd_nandbiterrs: Inserted biterror @ 1024/2 mtd_nandbiterrs: rewrite page ECC code:(the ECC code in NAND controller buffer, which will be wrote to NAND flash) 11 70 40 2e ab 6e 17 34 1d 2d 2b b0 51 a4 c1 af 05 a6 44 12 25 f1 10 49 7d 0b bd 95 28 07 09 3c c5 1c f6 cb 3e c0 26 d7 cf 2d 15 77 59 e8 5f fd a7 cc be 17 cb ee 39 de mtd_nandbiterrs: read_page ECC code: 10 60 40 00 88 0e 10 20 05 29 2a b0 51 80 80 0e 01 26 04 10 00 40 00 48 4d 01 15 94 28 07 09 3c c5 1c f6 cb 3e c0 26 d7 cf 2d 15 77 59 e8 5f fd a7 cc be 17 cb ee 39 de mtd_nandbiterrs: error: read failed at 0x800 mtd_nandbiterrs: After 1 biterrors per subpage, read reported error -74 mtd_nandbiterrs: finished successfully. ================================================== insmod: can't insert 'mtd_nandbiterrs.ko': Input/output error 2. in 1bit/ECC(print ECC codes): /home # insmod mtd_nandbiterrs.ko dev=2 page_offset=1 seed=110 mode=0 ================================================== mtd_nandbiterrs: MTD device: 2 mtd_nandbiterrs: MTD device size 8388608, eraseblock=131072, page=2048, oob=64 mtd_nandbiterrs: Device uses 4 subpages of 512 bytes mtd_nandbiterrs: Using page=1, offset=2048, eraseblock=0 mtd_nandbiterrs: incremental biterrors test mtd_nandbiterrs: write_page ECC code: ff ff 3f ff ff ff ff ff 3f ff ff ff mtd_nandbiterrs: rewrite page ECC code: ff ff 3f ff ff ff ff ff 3f ff ff ff mtd_nandbiterrs: read_page ECC code: ff ff 3f ff ff ff ff ff 3f ff ff ff mtd_nandbiterrs: verify_page mtd_nandbiterrs: Successfully corrected 0 bit errors per subpage mtd_nandbiterrs: Inserted biterror @ 0/5 mtd_nandbiterrs: Inserted biterror @ 512/6 mtd_nandbiterrs: Inserted biterror @ 1024/2 mtd_nandbiterrs: Inserted biterror @ 1536/6 mtd_nandbiterrs: rewrite page ECC code:(the ECC code in NAND controller buffer, which will be wrote to NAND flash) aa aa 59 aa aa 96 aa aa 66 aa aa 96 mtd_nandbiterrs: read_page ECC code: aa aa 19 aa aa 96 aa aa 26 aa aa 96 mtd_nandbiterrs: Read reported 2 corrected bit errors mtd_nandbiterrs: verify_page mtd_nandbiterrs: Error: page offset 0, expected 23, got 03 mtd_nandbiterrs: Error: page offset 512, expected 69, got 29 mtd_nandbiterrs: Error: page offset 1024, expected 06, got 02 mtd_nandbiterrs: Error: page offset 1536, expected 4c, got 0c mtd_nandbiterrs: ECC failure, read data is incorrect despite read success insmod: can't insert 'mtd_nandbiterrs.ko': Input/output error Reason about above 1bit/ECC test result: As the log above, in rewriting process, the ECC code should be "aa aa 59 aa aa 96 aa aa 66 aa aa 96", however actually it writes ECC code "aa aa 19 aa aa 96 aa aa 26 aa aa 96" to flash. ECC code of the first and third 512B of 2k page in flash are "aa aa 19" which each has 1bit error in ECC code. Taking the first 512B as an example, there are 2bit errors: 1bit in page data and 1bit in related ECC code. It can not correct this kind of 2bit errors in 1bit/ECC mode in this NAND controller, however, it will trigger a correctable interrupt. As a result, software can not find this 1bit error in page data. This is a hardware problem of this NAND controller. I plan to remove the 1bit/ECC mode support in patch of next version. > >>> I'd recommend digging a little more to figure out what's wrong here. You >>> might need to instrument the nandbiterrs test. This is possibly >> >> Thanks, I will do it. >> >>> highlighting a driver bug [1]. >>> >>> Brian >>> >>> [1] Besised simply that you didn't implement write_page_raw(). The >>> default nand_write_page_raw() implementation looks just like your >>> non-raw version. >> >> Yes. In ECC mode, as the NAND controller must write page as a whole >> with ECC code, the default nand_write_page_raw() looks just like >> non-raw version. > > Are you saying you cannot implement the raw() hooks for this IP? Or just > that you haven't yet? The latter is probably OK for now (I'd recommend > doing this, or at least mark a TODO in the code), but the former is a > little disturbing. The function of raw() hooks is just writing the page data to flash, is this right? In none ECC mode, it can write page date alone to flash. But in ECC mode, NAND controller will produce related ECC code automatically, write page data and ECC code to flash. In ECC mode, it can not write page date alone to flash for this NAND controller. As a result, the nandbiterrs test can not pass. I don't know if I have explained these two problems clearly. If still have something confused, please let me know. > > Brian Many thanks for your comments! Zhou Wang > > . > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc 2015-01-22 3:27 ` Zhou Wang @ 2015-01-22 8:45 ` Brian Norris 2015-01-25 8:19 ` Zhou Wang 0 siblings, 1 reply; 26+ messages in thread From: Brian Norris @ 2015-01-22 8:45 UTC (permalink / raw) To: Zhou Wang Cc: mark.rutland, Zhou Wang, Iwo Mergler, pawel.moll, devicetree, liguozhu, ijc+devicetree, haojian.zhuang, robh+dt, linux-mtd, xuwei5, galak, caizhiyong, yubingxu, David Woodhouse Hi Zhou, On Thu, Jan 22, 2015 at 11:27:01AM +0800, Zhou Wang wrote: > Very sorry for late, I made tests again and also had a talk with the > NAND controller hardware colleague. Please find my reply below. No problem. Glad to hear you followed through on this one, as the results were curious. > On 2015/1/13 12:17, Brian Norris wrote: > > On Wed, Dec 17, 2014 at 07:05:47PM +0800, Zhou Wang wrote: > >> On 2014年12月17日 14:23, Brian Norris wrote: > > [...] > >>>> [ 104.648056] mtd_nandbiterrs: ECC failure, read data is incorrect > >>>> despite read success > >>>> insmod: can't insert 'mtd_nandbiterrs.ko': Input/output error [...] > I made testes again in 1bit/ECC and 16bit/ECC modes using 2K(page)+64B(oob) > NAND flash. here are the logs, I also printed ECC code in OOB area. > > Results are: > 1. in 16bit/ECC, it will return -EBADMSG as the ECC codes have been broken. > 2. in 1bit/ECC, it will not reture -EBADMSG because a hardware design problem. > I will explain the detail below. > > Test logs: > 1. in 16bit/ECC(print ECC codes): > > /home # insmod mtd_nandbiterrs.ko dev=2 page_offset=1 seed=110 mode=0 ... > mtd_nandbiterrs: error: read failed at 0x800 > mtd_nandbiterrs: After 1 biterrors per subpage, read reported error -74 ^^^ Ah, that's what I would expect from a driver that doesn't implement the raw() functions. > mtd_nandbiterrs: finished successfully. > ================================================== > insmod: can't insert 'mtd_nandbiterrs.ko': Input/output error > > 2. in 1bit/ECC(print ECC codes): > /home # insmod mtd_nandbiterrs.ko dev=2 page_offset=1 seed=110 mode=0 ... > mtd_nandbiterrs: ECC failure, read data is incorrect despite read success > insmod: can't insert 'mtd_nandbiterrs.ko': Input/output error > > Reason about above 1bit/ECC test result: ... > It can not correct this kind of 2bit errors in 1bit/ECC mode in this NAND > controller, however, it will trigger a correctable interrupt. As a result, > software can not find this 1bit error in page data. IOW, uncorrectable errors are getting reported as corrected bitflips? That does sound bad. > This is a hardware problem of this NAND controller. > I plan to remove the 1bit/ECC mode support in patch of next version. OK, sounds good. 1-bit HW ECC is not really very useful these days anyway, if your higher-bit ECC can serve to replace it. Can the ECC bytes still fit in the same spare area, though? > > Are you saying you cannot implement the raw() hooks for this IP? Or just > > that you haven't yet? The latter is probably OK for now (I'd recommend > > doing this, or at least mark a TODO in the code), but the former is a > > little disturbing. > > The function of raw() hooks is just writing the page data to flash, is this right? Right, just data (and OOB, if calling the _oob_ functions) without any ECC parity bytes. > In none ECC mode, it can write page date alone to flash. But in ECC mode, NAND > controller will produce related ECC code automatically, write page data and ECC code > to flash. In ECC mode, it can not write page date alone to flash for this NAND controller. Perhaps you can switch between ECC mode and non-ECC mode? At any rate, this isn't absolutely required. > As a result, the nandbiterrs test can not pass. > > I don't know if I have explained these two problems clearly. If still have something > confused, please let me know. Brian ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc 2015-01-22 8:45 ` Brian Norris @ 2015-01-25 8:19 ` Zhou Wang 2015-02-06 1:33 ` Brian Norris 0 siblings, 1 reply; 26+ messages in thread From: Zhou Wang @ 2015-01-25 8:19 UTC (permalink / raw) To: Brian Norris Cc: mark.rutland, Zhou Wang, Iwo Mergler, pawel.moll, devicetree, liguozhu, ijc+devicetree, haojian.zhuang, robh+dt, linux-mtd, xuwei5, galak, caizhiyong, yubingxu, David Woodhouse On 2015/1/22 16:45, Brian Norris wrote: > Hi Zhou, > > On Thu, Jan 22, 2015 at 11:27:01AM +0800, Zhou Wang wrote: >> Very sorry for late, I made tests again and also had a talk with the >> NAND controller hardware colleague. Please find my reply below. > > No problem. Glad to hear you followed through on this one, as the > results were curious. > >> On 2015/1/13 12:17, Brian Norris wrote: >>> On Wed, Dec 17, 2014 at 07:05:47PM +0800, Zhou Wang wrote: >>>> On 2014年12月17日 14:23, Brian Norris wrote: >>> [...] >>>>>> [ 104.648056] mtd_nandbiterrs: ECC failure, read data is incorrect >>>>>> despite read success >>>>>> insmod: can't insert 'mtd_nandbiterrs.ko': Input/output error > [...] >> I made testes again in 1bit/ECC and 16bit/ECC modes using 2K(page)+64B(oob) >> NAND flash. here are the logs, I also printed ECC code in OOB area. >> >> Results are: >> 1. in 16bit/ECC, it will return -EBADMSG as the ECC codes have been broken. >> 2. in 1bit/ECC, it will not reture -EBADMSG because a hardware design problem. >> I will explain the detail below. >> >> Test logs: >> 1. in 16bit/ECC(print ECC codes): >> >> /home # insmod mtd_nandbiterrs.ko dev=2 page_offset=1 seed=110 mode=0 > > ... > >> mtd_nandbiterrs: error: read failed at 0x800 >> mtd_nandbiterrs: After 1 biterrors per subpage, read reported error -74 > > ^^^ Ah, that's what I would expect from a driver that doesn't implement > the raw() functions. > >> mtd_nandbiterrs: finished successfully. >> ================================================== >> insmod: can't insert 'mtd_nandbiterrs.ko': Input/output error >> >> 2. in 1bit/ECC(print ECC codes): >> /home # insmod mtd_nandbiterrs.ko dev=2 page_offset=1 seed=110 mode=0 > > ... > >> mtd_nandbiterrs: ECC failure, read data is incorrect despite read success >> insmod: can't insert 'mtd_nandbiterrs.ko': Input/output error >> >> Reason about above 1bit/ECC test result: > > ... > >> It can not correct this kind of 2bit errors in 1bit/ECC mode in this NAND >> controller, however, it will trigger a correctable interrupt. As a result, >> software can not find this 1bit error in page data. > > IOW, uncorrectable errors are getting reported as corrected bitflips? > That does sound bad. Yes, so I will not support 1bit ECC mode in patch of next version. > >> This is a hardware problem of this NAND controller. >> I plan to remove the 1bit/ECC mode support in patch of next version. > > OK, sounds good. 1-bit HW ECC is not really very useful these days > anyway, if your higher-bit ECC can serve to replace it. Can the ECC > bytes still fit in the same spare area, though? > No, different ECC modes have different ECC layouts in oob area. >>> Are you saying you cannot implement the raw() hooks for this IP? Or just >>> that you haven't yet? The latter is probably OK for now (I'd recommend >>> doing this, or at least mark a TODO in the code), but the former is a >>> little disturbing. >> >> The function of raw() hooks is just writing the page data to flash, is this right? > > Right, just data (and OOB, if calling the _oob_ functions) without any > ECC parity bytes. > >> In none ECC mode, it can write page date alone to flash. But in ECC mode, NAND >> controller will produce related ECC code automatically, write page data and ECC code >> to flash. In ECC mode, it can not write page date alone to flash for this NAND controller. > > Perhaps you can switch between ECC mode and non-ECC mode? I also tried to swith to non-ECC mode to implement raw_write, and it is easy to do this. But, event we do this, it will bring me little help. The reason to implement raw_write is to run mtd_nandbiterrs correctly as the discussion above. However, this NAND controller writes data and ECC code to flash using different layouts in different modes. So we should also read data and ECC code from flash, change related bits, then rewrite the data and ECC code to flash in none-ECC mode to run mtd_nandbiterrs. It is not easy to do this. So my plan is not to implement raw_write hook if this function is just used in test. Many thanks for your reply, Zhou Wang . > > At any rate, this isn't absolutely required. > >> As a result, the nandbiterrs test can not pass. >> >> I don't know if I have explained these two problems clearly. If still have something >> confused, please let me know. > > Brian > > . > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc 2015-01-25 8:19 ` Zhou Wang @ 2015-02-06 1:33 ` Brian Norris 0 siblings, 0 replies; 26+ messages in thread From: Brian Norris @ 2015-02-06 1:33 UTC (permalink / raw) To: Zhou Wang Cc: mark.rutland, Zhou Wang, Iwo Mergler, pawel.moll, devicetree, liguozhu, ijc+devicetree, haojian.zhuang, robh+dt, linux-mtd, xuwei5, galak, caizhiyong, yubingxu, David Woodhouse On Sun, Jan 25, 2015 at 04:19:43PM +0800, Zhou Wang wrote: > On 2015/1/22 16:45, Brian Norris wrote: > > Perhaps you can switch between ECC mode and non-ECC mode? > > I also tried to swith to non-ECC mode to implement raw_write, and it is easy to do this. > > But, event we do this, it will bring me little help. The reason to implement raw_write is to > run mtd_nandbiterrs correctly as the discussion above. However, this NAND controller writes > data and ECC code to flash using different layouts in different modes. Ugh, that's no fun. GPMI seems to do a similarly silly thing. > So we should also read data > and ECC code from flash, change related bits, then rewrite the data and ECC code to flash in none-ECC > mode to run mtd_nandbiterrs. It is not easy to do this. > > So my plan is not to implement raw_write hook if this function is just used in test. OK. There may eventually be good reasons to do this still, but it's not important right now. Brian ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2015-02-06 1:33 UTC | newest] Thread overview: 26+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-11-04 12:46 [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc Zhou Wang 2014-11-04 12:47 ` [PATCH v4 1/2] mtd: hisilicon: add a new NAND controller driver for " Zhou Wang 2014-11-30 9:35 ` Brian Norris 2014-12-01 13:08 ` Zhou Wang 2014-12-01 18:14 ` Brian Norris 2014-12-01 9:25 ` Brian Norris 2014-12-01 13:08 ` Zhou Wang 2014-11-04 12:47 ` [PATCH v4 2/2] mtd: hisilicon: add device tree binding documentation Zhou Wang 2014-11-30 8:56 ` Brian Norris 2014-12-01 13:08 ` Zhou Wang 2014-11-30 9:01 ` Brian Norris 2014-12-01 13:07 ` Zhou Wang 2014-11-06 3:05 ` [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc Haojian Zhuang 2014-11-26 6:35 ` Haojian Zhuang 2014-11-30 9:08 ` Brian Norris 2014-12-01 13:06 ` Zhou Wang 2014-12-11 12:02 ` Zhou Wang 2014-12-17 1:03 ` Brian Norris 2014-12-17 5:06 ` Zhou Wang 2014-12-17 6:23 ` Brian Norris 2014-12-17 11:05 ` Zhou Wang 2015-01-13 4:17 ` Brian Norris 2015-01-22 3:27 ` Zhou Wang 2015-01-22 8:45 ` Brian Norris 2015-01-25 8:19 ` Zhou Wang 2015-02-06 1:33 ` Brian Norris
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).