* [PATCH v6 1/2] mtd: arasan: Add device tree binding documentation
@ 2016-12-05 4:11 Punnaiah Choudary Kalluri
2016-12-05 4:11 ` [PATCH v6 2/2] mtd: nand: Add support for Arasan Nand Flash Controller Punnaiah Choudary Kalluri
2016-12-05 4:25 ` [PATCH v6 1/2] mtd: arasan: Add device tree binding documentation Marek Vasut
0 siblings, 2 replies; 10+ messages in thread
From: Punnaiah Choudary Kalluri @ 2016-12-05 4:11 UTC (permalink / raw)
To: dwmw2, computersforpeace, boris.brezillon, marek.vasut, richard,
cyrille.pitchen, robh+dt, mark.rutland
Cc: linux-kernel, linux-mtd, devicetree, michals,
kalluripunnaiahchoudary, kpc528, Punnaiah Choudary Kalluri
This patch adds the dts binding document for arasan nand flash
controller.
Signed-off-by: Punnaiah Choudary Kalluri <punnaia@xilinx.com>
Acked-by: Rob Herring <robh@kernel.org>
---
changes in v6:
- Removed num-cs property
- Separated nandchip from nand controller
changes in v5:
- None
Changes in v4:
- Added num-cs property
- Added clock support
Changes in v3:
- None
Changes in v2:
- None
---
.../devicetree/bindings/mtd/arasan_nfc.txt | 38 ++++++++++++++++++++++
1 file changed, 38 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mtd/arasan_nfc.txt
diff --git a/Documentation/devicetree/bindings/mtd/arasan_nfc.txt b/Documentation/devicetree/bindings/mtd/arasan_nfc.txt
new file mode 100644
index 0000000..dcbe7ad
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/arasan_nfc.txt
@@ -0,0 +1,38 @@
+Arasan Nand Flash Controller with ONFI 3.1 support
+
+Required properties:
+- compatible: Should be "arasan,nfc-v3p10"
+- reg: Memory map for module access
+- interrupt-parent: Interrupt controller the interrupt is routed through
+- interrupts: Should contain the interrupt for the device
+- clock-name: List of input clocks - "clk_sys", "clk_flash"
+ (See clock bindings for details)
+- clocks: Clock phandles (see clock bindings for details)
+
+Optional properties:
+- arasan,has-mdma: Enables Dma support
+
+for nand partition information please refer the below file
+Documentation/devicetree/bindings/mtd/partition.txt
+
+Example:
+ nand0: nand@ff100000 {
+ compatible = "arasan,nfc-v3p10"
+ reg = <0x0 0xff100000 0x1000>;
+ clock-name = "clk_sys", "clk_flash"
+ clocks = <&misc_clk &misc_clk>;
+ interrupt-parent = <&gic>;
+ interrupts = <0 14 4>;
+ arasan,has-mdma;
+ #address-cells = <1>;
+ #size-cells = <0>
+
+ nand@0 {
+ reg = <0>
+ partition@0 {
+ label = "filesystem";
+ reg = <0x0 0x0 0x1000000>;
+ };
+ (...)
+ };
+ };
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v6 2/2] mtd: nand: Add support for Arasan Nand Flash Controller
2016-12-05 4:11 [PATCH v6 1/2] mtd: arasan: Add device tree binding documentation Punnaiah Choudary Kalluri
@ 2016-12-05 4:11 ` Punnaiah Choudary Kalluri
2016-12-05 4:40 ` Marek Vasut
2016-12-05 9:16 ` kbuild test robot
2016-12-05 4:25 ` [PATCH v6 1/2] mtd: arasan: Add device tree binding documentation Marek Vasut
1 sibling, 2 replies; 10+ messages in thread
From: Punnaiah Choudary Kalluri @ 2016-12-05 4:11 UTC (permalink / raw)
To: dwmw2, computersforpeace, boris.brezillon, marek.vasut, richard,
cyrille.pitchen, robh+dt, mark.rutland
Cc: linux-kernel, linux-mtd, devicetree, michals,
kalluripunnaiahchoudary, kpc528, Punnaiah Choudary Kalluri
Added the basic driver for Arasan Nand Flash Controller used in
Zynq UltraScale+ MPSoC. It supports only Hw Ecc and upto 24bit
correction.
Signed-off-by: Punnaiah Choudary Kalluri <punnaia@xilinx.com>
---
Chnages in v6:
- Addressed most of the Brian and Boris comments
- Separated the nandchip from the nand controller
- Removed the ecc lookup table from driver
- Now use framework nand waitfunction and readoob
- Fixed the compiler warning
- Adapted the new frameowrk changes related to ecc and ooblayout
- Disabled the clocks after the nand_reelase
- Now using only one completion object
- Boris suggessions like adapting cmd_ctrl and rework on read/write byte
are not implemented and i will patch them later
- Also check_erased_ecc_chunk for erase and check for is_vmalloc_addr will
implement later once the basic driver is mainlined.
Changes in v5:
- Renamed the driver filei as arasan_nand.c
- Fixed all comments relaqted coding style
- Fixed comments related to propagating the errors
- Modified the anfc_write_page_hwecc as per the write_page
prototype
Changes in v4:
- Added support for onfi timing mode configuration
- Added clock supppport
- Added support for multiple chipselects
Changes in v3:
- Removed unused variables
- Avoided busy loop and used jifies based implementation
- Fixed compiler warnings "right shift count >= width of type"
- Removed unneeded codei and improved error reporting
- Added onfi version check to ensure reading the valid address cycles
Changes in v2:
- Added missing of.h to avoid kbuild system report erro
---
drivers/mtd/nand/Kconfig | 8 +
drivers/mtd/nand/Makefile | 1 +
drivers/mtd/nand/arasan_nand.c | 974 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 983 insertions(+)
create mode 100644 drivers/mtd/nand/arasan_nand.c
diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 7b7a887..e831f4e 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -569,4 +569,12 @@ config MTD_NAND_MTK
Enables support for NAND controller on MTK SoCs.
This controller is found on mt27xx, mt81xx, mt65xx SoCs.
+config MTD_NAND_ARASAN
+ tristate "Support for Arasan Nand Flash controller"
+ depends on HAS_IOMEM
+ depends on HAS_DMA
+ help
+ Enables the driver for the Arasan Nand Flash controller on
+ Zynq UltraScale+ MPSoC.
+
endif # MTD_NAND
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index cafde6f..44b8b00 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -58,5 +58,6 @@ obj-$(CONFIG_MTD_NAND_HISI504) += hisi504_nand.o
obj-$(CONFIG_MTD_NAND_BRCMNAND) += brcmnand/
obj-$(CONFIG_MTD_NAND_QCOM) += qcom_nandc.o
obj-$(CONFIG_MTD_NAND_MTK) += mtk_nand.o mtk_ecc.o
+obj-$(CONFIG_MTD_NAND_ARASAN) += arasan_nand.o
nand-objs := nand_base.o nand_bbt.o nand_timings.o
diff --git a/drivers/mtd/nand/arasan_nand.c b/drivers/mtd/nand/arasan_nand.c
new file mode 100644
index 0000000..6b0670e
--- /dev/null
+++ b/drivers/mtd/nand/arasan_nand.c
@@ -0,0 +1,974 @@
+/*
+ * Arasan Nand Flash Controller Driver
+ *
+ * Copyright (C) 2014 - 2015 Xilinx, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it under
+ * the terms of the GNU General Public License version 2 as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ */
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/dma-mapping.h>
+#include <linux/interrupt.h>
+#include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/module.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/nand.h>
+#include <linux/mtd/partitions.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+#define DRIVER_NAME "arasan_nand"
+#define EVNT_TIMEOUT 1000
+#define STATUS_TIMEOUT 2000
+
+#define PKT_OFST 0x00
+#define MEM_ADDR1_OFST 0x04
+#define MEM_ADDR2_OFST 0x08
+#define CMD_OFST 0x0C
+#define PROG_OFST 0x10
+#define INTR_STS_EN_OFST 0x14
+#define INTR_SIG_EN_OFST 0x18
+#define INTR_STS_OFST 0x1C
+#define READY_STS_OFST 0x20
+#define DMA_ADDR1_OFST 0x24
+#define FLASH_STS_OFST 0x28
+#define DATA_PORT_OFST 0x30
+#define ECC_OFST 0x34
+#define ECC_ERR_CNT_OFST 0x38
+#define ECC_SPR_CMD_OFST 0x3C
+#define ECC_ERR_CNT_1BIT_OFST 0x40
+#define ECC_ERR_CNT_2BIT_OFST 0x44
+#define DMA_ADDR0_OFST 0x50
+#define DATA_INTERFACE_REG 0x6C
+
+#define PKT_CNT_SHIFT 12
+
+#define ECC_ENABLE BIT(31)
+#define DMA_EN_MASK GENMASK(27, 26)
+#define DMA_ENABLE 0x2
+#define DMA_EN_SHIFT 26
+#define REG_PAGE_SIZE_MASK GENMASK(25, 23)
+#define REG_PAGE_SIZE_SHIFT 23
+#define REG_PAGE_SIZE_512 0
+#define REG_PAGE_SIZE_1K 5
+#define REG_PAGE_SIZE_2K 1
+#define REG_PAGE_SIZE_4K 2
+#define REG_PAGE_SIZE_8K 3
+#define REG_PAGE_SIZE_16K 4
+#define CMD2_SHIFT 8
+#define ADDR_CYCLES_SHIFT 28
+
+#define XFER_COMPLETE BIT(2)
+#define READ_READY BIT(1)
+#define WRITE_READY BIT(0)
+#define MBIT_ERROR BIT(3)
+#define ERR_INTRPT BIT(4)
+
+#define PROG_PGRD BIT(0)
+#define PROG_ERASE BIT(2)
+#define PROG_STATUS BIT(3)
+#define PROG_PGPROG BIT(4)
+#define PROG_RDID BIT(6)
+#define PROG_RDPARAM BIT(7)
+#define PROG_RST BIT(8)
+#define PROG_GET_FEATURE BIT(9)
+#define PROG_SET_FEATURE BIT(10)
+
+#define ONFI_STATUS_FAIL BIT(0)
+#define ONFI_STATUS_READY BIT(6)
+
+#define PG_ADDR_SHIFT 16
+#define BCH_MODE_SHIFT 25
+#define BCH_EN_SHIFT 27
+#define ECC_SIZE_SHIFT 16
+
+#define MEM_ADDR_MASK GENMASK(7, 0)
+#define BCH_MODE_MASK GENMASK(27, 25)
+
+#define CS_MASK GENMASK(31, 30)
+#define CS_SHIFT 30
+
+#define PAGE_ERR_CNT_MASK GENMASK(16, 8)
+#define PKT_ERR_CNT_MASK GENMASK(7, 0)
+
+#define NVDDR_MODE BIT(9)
+#define NVDDR_TIMING_MODE_SHIFT 3
+
+#define ONFI_ID_LEN 8
+#define TEMP_BUF_SIZE 512
+#define NVDDR_MODE_PACKET_SIZE 8
+#define SDR_MODE_PACKET_SIZE 4
+
+#define ONFI_DATA_INTERFACE_NVDDR (1 << 4)
+
+/**
+ * struct anfc_nand_chip - Defines the nand chip related information
+ * @node: used to store NAND chips into a list.
+ * @chip: NAND chip information structure.
+ * @bch: Bch / Hamming mode enable/disable.
+ * @bchmode: Bch mode.
+ * @eccval: Ecc config value.
+ * @raddr_cycles: Row address cycle information.
+ * @caddr_cycles: Column address cycle information.
+ * @pktsize: Packet size for read / write operation.
+ * @csnum: chipselect number to be used.
+ * @spktsize: Packet size in ddr mode for status operation.
+ * @inftimeval Data interface and timing mode information
+ */
+struct anfc_nand_chip {
+ struct list_head node;
+ struct nand_chip chip;
+ bool bch;
+ u32 bchmode;
+ u32 eccval;
+ u16 raddr_cycles;
+ u16 caddr_cycles;
+ u32 pktsize;
+ int csnum;
+ u32 spktsize;
+ u32 inftimeval;
+};
+
+/**
+ * struct anfc - Defines the Arasan NAND flash driver instance
+ * @controller: base controller structure.
+ * @chips: list of all nand chips attached to the ctrler.
+ * @dev: Pointer to the device structure.
+ * @base: Virtual address of the NAND flash device.
+ * @curr_cmd: Current command issued.
+ * @clk_sys: Pointer to the system clock.
+ * @clk_flash: Pointer to the flash clock.
+ * @dma: Dma enable/disable.
+ * @err: Error identifier.
+ * @iswriteoob: Identifies if oob write operation is required.
+ * @buf: Buffer used for read/write byte operations.
+ * @irq: irq number
+ * @bufshift: Variable used for indexing buffer operation
+ * @csnum: Chip select number currently inuse.
+ * @evnt: Completion event for nand status events.
+ */
+struct anfc {
+ struct nand_hw_control controller;
+ struct list_head chips;
+ struct device *dev;
+ void __iomem *base;
+ int curr_cmd;
+ struct clk *clk_sys;
+ struct clk *clk_flash;
+ bool dma;
+ bool err;
+ bool iswriteoob;
+ u8 buf[TEMP_BUF_SIZE];
+ int irq;
+ u32 bufshift;
+ int csnum;
+ struct completion evnt;
+};
+
+static int anfc_ooblayout_ecc(struct mtd_info *mtd, int section,
+ struct mtd_oob_region *oobregion)
+{
+ struct nand_chip *nand = mtd_to_nand(mtd);
+
+ if (section)
+ return -ERANGE;
+
+ oobregion->length = nand->ecc.total;
+ oobregion->offset = mtd->oobsize - oobregion->length;
+
+ return 0;
+}
+
+static int anfc_ooblayout_free(struct mtd_info *mtd, int section,
+ struct mtd_oob_region *oobregion)
+{
+ struct nand_chip *nand = mtd_to_nand(mtd);
+
+ if (section)
+ return -ERANGE;
+
+ oobregion->offset = 2;
+ oobregion->length = mtd->oobsize - nand->ecc.total - 2;
+
+ return 0;
+}
+
+static const struct mtd_ooblayout_ops anfc_ooblayout_ops = {
+ .ecc = anfc_ooblayout_ecc,
+ .free = anfc_ooblayout_free,
+};
+
+static inline struct anfc_nand_chip *to_anfc_nand(struct nand_chip *nand)
+{
+ return container_of(nand, struct anfc_nand_chip, chip);
+}
+
+static inline struct anfc *to_anfc(struct nand_hw_control *ctrl)
+{
+ return container_of(ctrl, struct anfc, controller);
+}
+
+static u8 anfc_page(u32 pagesize)
+{
+ switch (pagesize) {
+ case 512:
+ return REG_PAGE_SIZE_512;
+ case 1024:
+ return REG_PAGE_SIZE_1K;
+ case 2048:
+ return REG_PAGE_SIZE_2K;
+ case 4096:
+ return REG_PAGE_SIZE_4K;
+ case 8192:
+ return REG_PAGE_SIZE_8K;
+ case 16384:
+ return REG_PAGE_SIZE_16K;
+ default:
+ break;
+ }
+
+ return 0;
+}
+
+static inline void anfc_enable_intrs(struct anfc *nfc, u32 val)
+{
+ writel(val, nfc->base + INTR_STS_EN_OFST);
+ writel(val, nfc->base + INTR_SIG_EN_OFST);
+}
+
+static inline int anfc_wait_for_event(struct anfc *nfc)
+{
+ return wait_for_completion_timeout(&nfc->evnt,
+ msecs_to_jiffies(EVNT_TIMEOUT));
+}
+
+static inline void anfc_setpktszcnt(struct anfc *nfc, u32 pktsize,
+ u32 pktcount)
+{
+ writel(pktsize | (pktcount << PKT_CNT_SHIFT), nfc->base + PKT_OFST);
+}
+
+static inline void anfc_set_eccsparecmd(struct anfc *nfc,
+ struct anfc_nand_chip *achip, u8 cmd1, u8 cmd2)
+{
+ writel(cmd1 | (cmd2 << CMD2_SHIFT) |
+ (achip->caddr_cycles << ADDR_CYCLES_SHIFT),
+ nfc->base + ECC_SPR_CMD_OFST);
+}
+
+static void anfc_setpagecoladdr(struct anfc *nfc, u32 page, u16 col)
+{
+ u32 val;
+
+ writel(col | (page << PG_ADDR_SHIFT), nfc->base + MEM_ADDR1_OFST);
+
+ val = readl(nfc->base + MEM_ADDR2_OFST);
+ val = (val & ~MEM_ADDR_MASK) |
+ ((page >> PG_ADDR_SHIFT) & MEM_ADDR_MASK);
+ writel(val, nfc->base + MEM_ADDR2_OFST);
+}
+
+static void anfc_prepare_cmd(struct anfc *nfc, u8 cmd1, u8 cmd2, u8 dmamode,
+ u32 pagesize, u8 addrcycles)
+{
+ u32 regval;
+
+ regval = cmd1 | (cmd2 << CMD2_SHIFT);
+ if (dmamode && nfc->dma)
+ regval |= DMA_ENABLE << DMA_EN_SHIFT;
+ if (addrcycles)
+ regval |= addrcycles << ADDR_CYCLES_SHIFT;
+ if (pagesize)
+ regval |= anfc_page(pagesize) << REG_PAGE_SIZE_SHIFT;
+ writel(regval, nfc->base + CMD_OFST);
+}
+
+static int anfc_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
+ int page)
+{
+ struct anfc *nfc = to_anfc(chip->controller);
+
+ nfc->iswriteoob = true;
+ chip->cmdfunc(mtd, NAND_CMD_SEQIN, mtd->writesize, page);
+ chip->write_buf(mtd, chip->oob_poi, mtd->oobsize);
+ nfc->iswriteoob = false;
+
+ return 0;
+}
+
+static void anfc_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
+{
+ u32 pktcount, pktsize, eccintr = 0;
+ unsigned int buf_rd_cnt = 0;
+ u32 *bufptr = (u32 *)buf;
+ struct nand_chip *chip = mtd_to_nand(mtd);
+ struct anfc_nand_chip *achip = to_anfc_nand(chip);
+ struct anfc *nfc = to_anfc(chip->controller);
+ dma_addr_t paddr;
+
+ if (nfc->curr_cmd == NAND_CMD_READ0) {
+ pktsize = achip->pktsize;
+ pktcount = DIV_ROUND_UP(mtd->writesize, pktsize);
+ if (!achip->bch)
+ eccintr = MBIT_ERROR;
+ } else {
+ pktsize = len;
+ pktcount = 1;
+ }
+
+ anfc_setpktszcnt(nfc, pktsize, pktcount);
+
+ if (nfc->dma) {
+ paddr = dma_map_single(nfc->dev, buf, len, DMA_FROM_DEVICE);
+ if (dma_mapping_error(nfc->dev, paddr)) {
+ dev_err(nfc->dev, "Read buffer mapping error");
+ return;
+ }
+ lo_hi_writeq(paddr, nfc->base + DMA_ADDR0_OFST);
+ anfc_enable_intrs(nfc, (XFER_COMPLETE | eccintr));
+ writel(PROG_PGRD, nfc->base + PROG_OFST);
+ anfc_wait_for_event(nfc);
+ dma_unmap_single(nfc->dev, paddr, len, DMA_FROM_DEVICE);
+ return;
+ }
+
+ anfc_enable_intrs(nfc, (READ_READY | eccintr));
+ writel(PROG_PGRD, nfc->base + PROG_OFST);
+
+ while (buf_rd_cnt < pktcount) {
+ anfc_wait_for_event(nfc);
+ buf_rd_cnt++;
+
+ if (buf_rd_cnt == pktcount)
+ anfc_enable_intrs(nfc, XFER_COMPLETE);
+
+ readsl(nfc->base + DATA_PORT_OFST, bufptr, pktsize/4);
+ bufptr += (pktsize / 4);
+
+ if (buf_rd_cnt < pktcount)
+ anfc_enable_intrs(nfc, (READ_READY | eccintr));
+ }
+
+ anfc_wait_for_event(nfc);
+}
+
+static void anfc_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len)
+{
+ u32 pktcount, pktsize;
+ unsigned int buf_wr_cnt = 0;
+ u32 *bufptr = (u32 *)buf;
+ struct nand_chip *chip = mtd_to_nand(mtd);
+ struct anfc_nand_chip *achip = to_anfc_nand(chip);
+ struct anfc *nfc = to_anfc(chip->controller);
+ dma_addr_t paddr;
+
+ if (nfc->iswriteoob) {
+ pktsize = len;
+ pktcount = 1;
+ } else {
+ pktsize = achip->pktsize;
+ pktcount = mtd->writesize / pktsize;
+ }
+
+ anfc_setpktszcnt(nfc, pktsize, pktcount);
+
+ if (nfc->dma) {
+ paddr = dma_map_single(nfc->dev, (void *)buf, len,
+ DMA_TO_DEVICE);
+ if (dma_mapping_error(nfc->dev, paddr)) {
+ dev_err(nfc->dev, "Write buffer mapping error");
+ return;
+ }
+ lo_hi_writeq(paddr, nfc->base + DMA_ADDR0_OFST);
+ anfc_enable_intrs(nfc, XFER_COMPLETE);
+ writel(PROG_PGPROG, nfc->base + PROG_OFST);
+ anfc_wait_for_event(nfc);
+ dma_unmap_single(nfc->dev, paddr, len, DMA_TO_DEVICE);
+ return;
+ }
+
+ anfc_enable_intrs(nfc, WRITE_READY);
+ writel(PROG_PGPROG, nfc->base + PROG_OFST);
+
+ while (buf_wr_cnt < pktcount) {
+ anfc_wait_for_event(nfc);
+ buf_wr_cnt++;
+ if (buf_wr_cnt == pktcount)
+ anfc_enable_intrs(nfc, XFER_COMPLETE);
+
+ writesl(nfc->base + DATA_PORT_OFST, bufptr, pktsize/4);
+ bufptr += (pktsize / 4);
+
+ if (buf_wr_cnt < pktcount)
+ anfc_enable_intrs(nfc, WRITE_READY);
+ }
+
+ anfc_wait_for_event(nfc);
+}
+
+static int anfc_read_page_hwecc(struct mtd_info *mtd,
+ struct nand_chip *chip, uint8_t *buf,
+ int oob_required, int page)
+{
+ u32 val;
+ struct anfc *nfc = to_anfc(chip->controller);
+ struct anfc_nand_chip *achip = to_anfc_nand(chip);
+
+ anfc_set_eccsparecmd(nfc, achip, NAND_CMD_RNDOUT, NAND_CMD_RNDOUTSTART);
+
+ val = readl(nfc->base + CMD_OFST);
+ val = val | ECC_ENABLE;
+ writel(val, nfc->base + CMD_OFST);
+
+ chip->read_buf(mtd, buf, mtd->writesize);
+
+ val = readl(nfc->base + ECC_ERR_CNT_OFST);
+ if (achip->bch) {
+ mtd->ecc_stats.corrected += val & PAGE_ERR_CNT_MASK;
+ } else {
+ val = readl(nfc->base + ECC_ERR_CNT_1BIT_OFST);
+ mtd->ecc_stats.corrected += val;
+ val = readl(nfc->base + ECC_ERR_CNT_2BIT_OFST);
+ mtd->ecc_stats.failed += val;
+ /* Clear ecc error count register 1Bit, 2Bit */
+ writel(0x0, nfc->base + ECC_ERR_CNT_1BIT_OFST);
+ writel(0x0, nfc->base + ECC_ERR_CNT_2BIT_OFST);
+ }
+ nfc->err = false;
+
+ if (oob_required)
+ chip->ecc.read_oob(mtd, chip, page);
+
+ return 0;
+}
+
+static int anfc_write_page_hwecc(struct mtd_info *mtd,
+ struct nand_chip *chip, const uint8_t *buf,
+ int oob_required, int page)
+{
+ u32 val;
+ int ret;
+ struct anfc *nfc = to_anfc(chip->controller);
+ struct anfc_nand_chip *achip = to_anfc_nand(chip);
+ uint8_t *ecc_calc = chip->buffers->ecccalc;
+
+ anfc_set_eccsparecmd(nfc, achip, NAND_CMD_RNDIN, 0);
+
+ val = readl(nfc->base + CMD_OFST);
+ val = val | ECC_ENABLE;
+ writel(val, nfc->base + CMD_OFST);
+
+ chip->write_buf(mtd, buf, mtd->writesize);
+
+ if (oob_required) {
+ chip->waitfunc(mtd, chip);
+ chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page);
+ chip->read_buf(mtd, ecc_calc, mtd->oobsize);
+ ret = mtd_ooblayout_set_eccbytes(mtd, ecc_calc, chip->oob_poi,
+ 0, chip->ecc.total);
+ if (ret)
+ return ret;
+ chip->ecc.write_oob(mtd, chip, page);
+ }
+
+ return 0;
+}
+
+static u8 anfc_read_byte(struct mtd_info *mtd)
+{
+ struct nand_chip *chip = mtd_to_nand(mtd);
+ struct anfc *nfc = to_anfc(chip->controller);
+
+ return nfc->buf[nfc->bufshift++];
+}
+
+static void anfc_writefifo(struct anfc *nfc, u32 prog, u32 size, u8 *buf)
+{
+ u32 *bufptr = (u32 *)buf;
+
+ anfc_enable_intrs(nfc, WRITE_READY);
+
+ writel(prog, nfc->base + PROG_OFST);
+ anfc_wait_for_event(nfc);
+
+ anfc_enable_intrs(nfc, XFER_COMPLETE);
+ writesl(nfc->base + DATA_PORT_OFST, bufptr, size/4);
+ anfc_wait_for_event(nfc);
+}
+
+static void anfc_readfifo(struct anfc *nfc, u32 prog, u32 size)
+{
+ u32 *bufptr = (u32 *)nfc->buf;
+
+ anfc_enable_intrs(nfc, READ_READY);
+
+ writel(prog, nfc->base + PROG_OFST);
+ anfc_wait_for_event(nfc);
+
+ anfc_enable_intrs(nfc, XFER_COMPLETE);
+ readsl(nfc->base + DATA_PORT_OFST, bufptr, size/4);
+ anfc_wait_for_event(nfc);
+}
+
+static int anfc_ecc_init(struct mtd_info *mtd,
+ struct nand_ecc_ctrl *ecc)
+{
+ u32 ecc_addr;
+ unsigned int bchmode, steps;
+ struct nand_chip *chip = mtd_to_nand(mtd);
+ struct anfc_nand_chip *achip = to_anfc_nand(chip);
+
+ ecc->mode = NAND_ECC_HW;
+ ecc->read_page = anfc_read_page_hwecc;
+ ecc->write_page = anfc_write_page_hwecc;
+ ecc->write_oob = anfc_write_oob;
+ mtd_set_ooblayout(mtd, &anfc_ooblayout_ops);
+
+ steps = mtd->writesize / chip->ecc_step_ds;
+
+ switch (chip->ecc_strength_ds) {
+ case 12:
+ bchmode = 0x1;
+ break;
+ case 8:
+ bchmode = 0x2;
+ break;
+ case 4:
+ bchmode = 0x3;
+ break;
+ case 24:
+ bchmode = 0x4;
+ break;
+ default:
+ bchmode = 0x0;
+ }
+
+ if (!bchmode)
+ ecc->total = 3 * steps;
+ else
+ ecc->total =
+ DIV_ROUND_UP(fls(8 * chip->ecc_step_ds) *
+ chip->ecc_strength_ds * steps, 8);
+
+ ecc->strength = chip->ecc_strength_ds;
+ ecc->size = chip->ecc_step_ds;
+ ecc->bytes = ecc->total / steps;
+ ecc->steps = steps;
+ achip->bchmode = bchmode;
+ achip->bch = achip->bchmode;
+ ecc_addr = mtd->writesize + (mtd->oobsize - ecc->total);
+
+ achip->eccval = ecc_addr | (ecc->total << ECC_SIZE_SHIFT) |
+ (achip->bch << BCH_EN_SHIFT);
+
+ if (chip->ecc_step_ds >= 1024)
+ achip->pktsize = 1024;
+ else
+ achip->pktsize = 512;
+
+ return 0;
+}
+
+static void anfc_cmd_function(struct mtd_info *mtd,
+ unsigned int cmd, int column, int page_addr)
+{
+ struct nand_chip *chip = mtd_to_nand(mtd);
+ struct anfc_nand_chip *achip = to_anfc_nand(chip);
+ struct anfc *nfc = to_anfc(chip->controller);
+ bool wait = false, read = false;
+ u32 addrcycles, prog;
+ u32 *bufptr = (u32 *)nfc->buf;
+
+ nfc->bufshift = 0;
+ nfc->curr_cmd = cmd;
+
+ if (page_addr == -1)
+ page_addr = 0;
+ if (column == -1)
+ column = 0;
+
+ switch (cmd) {
+ case NAND_CMD_RESET:
+ anfc_prepare_cmd(nfc, cmd, 0, 0, 0, 0);
+ prog = PROG_RST;
+ wait = true;
+ break;
+ case NAND_CMD_SEQIN:
+ addrcycles = achip->raddr_cycles + achip->caddr_cycles;
+ anfc_prepare_cmd(nfc, cmd, NAND_CMD_PAGEPROG, 1,
+ mtd->writesize, addrcycles);
+ anfc_setpagecoladdr(nfc, page_addr, column);
+ break;
+ case NAND_CMD_READOOB:
+ column += mtd->writesize;
+ case NAND_CMD_READ0:
+ case NAND_CMD_READ1:
+ addrcycles = achip->raddr_cycles + achip->caddr_cycles;
+ anfc_prepare_cmd(nfc, NAND_CMD_READ0, NAND_CMD_READSTART, 1,
+ mtd->writesize, addrcycles);
+ anfc_setpagecoladdr(nfc, page_addr, column);
+ break;
+ case NAND_CMD_RNDOUT:
+ anfc_prepare_cmd(nfc, cmd, NAND_CMD_RNDOUTSTART, 1,
+ mtd->writesize, 2);
+ anfc_setpagecoladdr(nfc, page_addr, column);
+ break;
+ case NAND_CMD_PARAM:
+ anfc_prepare_cmd(nfc, cmd, 0, 0, 0, 1);
+ anfc_setpagecoladdr(nfc, page_addr, column);
+ anfc_setpktszcnt(nfc, sizeof(struct nand_onfi_params), 1);
+ anfc_readfifo(nfc, PROG_RDPARAM,
+ sizeof(struct nand_onfi_params));
+ break;
+ case NAND_CMD_READID:
+ anfc_prepare_cmd(nfc, cmd, 0, 0, 0, 1);
+ anfc_setpagecoladdr(nfc, page_addr, column);
+ anfc_setpktszcnt(nfc, ONFI_ID_LEN, 1);
+ anfc_readfifo(nfc, PROG_RDID, ONFI_ID_LEN);
+ break;
+ case NAND_CMD_ERASE1:
+ addrcycles = achip->raddr_cycles;
+ prog = PROG_ERASE;
+ anfc_prepare_cmd(nfc, cmd, NAND_CMD_ERASE2, 0, 0, addrcycles);
+ column = page_addr & 0xffff;
+ page_addr = (page_addr >> PG_ADDR_SHIFT) & 0xffff;
+ anfc_setpagecoladdr(nfc, page_addr, column);
+ wait = true;
+ break;
+ case NAND_CMD_STATUS:
+ anfc_prepare_cmd(nfc, cmd, 0, 0, 0, 0);
+ anfc_setpktszcnt(nfc, achip->spktsize/4, 1);
+ anfc_setpagecoladdr(nfc, page_addr, column);
+ prog = PROG_STATUS;
+ wait = read = true;
+ break;
+ case NAND_CMD_GET_FEATURES:
+ anfc_prepare_cmd(nfc, cmd, 0, 0, 0, 1);
+ anfc_setpagecoladdr(nfc, page_addr, column);
+ anfc_setpktszcnt(nfc, achip->spktsize, 1);
+ anfc_readfifo(nfc, PROG_GET_FEATURE, 4);
+ break;
+ case NAND_CMD_SET_FEATURES:
+ anfc_prepare_cmd(nfc, cmd, 0, 0, 0, 1);
+ anfc_setpagecoladdr(nfc, page_addr, column);
+ anfc_setpktszcnt(nfc, achip->spktsize, 1);
+ break;
+ default:
+ return;
+ }
+
+ if (wait) {
+ anfc_enable_intrs(nfc, XFER_COMPLETE);
+ writel(prog, nfc->base + PROG_OFST);
+ anfc_wait_for_event(nfc);
+ }
+
+ if (read)
+ bufptr[0] = readl(nfc->base + FLASH_STS_OFST);
+}
+
+static void anfc_select_chip(struct mtd_info *mtd, int num)
+{
+ u32 val;
+ struct nand_chip *chip = mtd_to_nand(mtd);
+ struct anfc_nand_chip *achip = to_anfc_nand(chip);
+ struct anfc *nfc = to_anfc(chip->controller);
+
+ if (num == -1)
+ return;
+
+ val = readl(nfc->base + MEM_ADDR2_OFST);
+ val = (val & ~(CS_MASK)) | (achip->csnum << CS_SHIFT);
+ val = (val & ~(BCH_MODE_MASK)) | (achip->bchmode << BCH_MODE_SHIFT);
+ writel(val, nfc->base + MEM_ADDR2_OFST);
+ nfc->csnum = achip->csnum;
+ writel(achip->eccval, nfc->base + ECC_OFST);
+ writel(achip->inftimeval, nfc->base + DATA_INTERFACE_REG);
+}
+
+static irqreturn_t anfc_irq_handler(int irq, void *ptr)
+{
+ struct anfc *nfc = ptr;
+ u32 regval = 0, status;
+
+ status = readl(nfc->base + INTR_STS_OFST);
+ if (status & XFER_COMPLETE) {
+ complete(&nfc->evnt);
+ regval |= XFER_COMPLETE;
+ }
+
+ if (status & READ_READY) {
+ complete(&nfc->evnt);
+ regval |= READ_READY;
+ }
+
+ if (status & WRITE_READY) {
+ complete(&nfc->evnt);
+ regval |= WRITE_READY;
+ }
+
+ if (status & MBIT_ERROR) {
+ nfc->err = true;
+ complete(&nfc->evnt);
+ regval |= MBIT_ERROR;
+ }
+
+ if (regval) {
+ writel(regval, nfc->base + INTR_STS_OFST);
+ writel(0, nfc->base + INTR_STS_EN_OFST);
+ writel(0, nfc->base + INTR_SIG_EN_OFST);
+
+ return IRQ_HANDLED;
+ }
+
+ return IRQ_NONE;
+}
+
+static int anfc_onfi_set_features(struct mtd_info *mtd, struct nand_chip *chip,
+ int addr, uint8_t *subfeature_param)
+{
+ struct anfc *nfc = to_anfc(chip->controller);
+ struct anfc_nand_chip *achip = to_anfc_nand(chip);
+ int status;
+
+ if (!chip->onfi_version || !(le16_to_cpu(chip->onfi_params.opt_cmd)
+ & ONFI_OPT_CMD_SET_GET_FEATURES))
+ return -EINVAL;
+
+ chip->cmdfunc(mtd, NAND_CMD_SET_FEATURES, addr, -1);
+ anfc_writefifo(nfc, PROG_SET_FEATURE, achip->spktsize,
+ subfeature_param);
+
+ status = chip->waitfunc(mtd, chip);
+ if (status & NAND_STATUS_FAIL)
+ return -EIO;
+
+ return 0;
+}
+
+static int anfc_init_timing_mode(struct anfc *nfc,
+ struct anfc_nand_chip *achip)
+{
+ int mode, err;
+ unsigned int feature[2];
+ u32 inftimeval;
+ struct nand_chip *chip = &achip->chip;
+ struct mtd_info *mtd = nand_to_mtd(chip);
+
+ memset(feature, 0, NVDDR_MODE_PACKET_SIZE);
+ /* Get nvddr timing modes */
+ mode = onfi_get_sync_timing_mode(chip) & 0xff;
+ if (!mode) {
+ mode = fls(onfi_get_async_timing_mode(chip)) - 1;
+ inftimeval = mode;
+ } else {
+ mode = fls(mode) - 1;
+ inftimeval = NVDDR_MODE | (mode << NVDDR_TIMING_MODE_SHIFT);
+ mode |= ONFI_DATA_INTERFACE_NVDDR;
+ }
+
+ feature[0] = mode;
+ chip->select_chip(mtd, achip->csnum);
+ err = chip->onfi_set_features(mtd, chip, ONFI_FEATURE_ADDR_TIMING_MODE,
+ (uint8_t *)feature);
+ chip->select_chip(mtd, -1);
+ if (err)
+ return err;
+
+ achip->inftimeval = inftimeval;
+
+ if (mode & ONFI_DATA_INTERFACE_NVDDR)
+ achip->spktsize = NVDDR_MODE_PACKET_SIZE;
+
+ return 0;
+}
+
+static int anfc_nand_chip_init(struct anfc *nfc,
+ struct anfc_nand_chip *anand_chip,
+ struct device_node *np)
+{
+ struct nand_chip *chip = &anand_chip->chip;
+ struct mtd_info *mtd = nand_to_mtd(chip);
+ int ret;
+
+ ret = of_property_read_u32(np, "reg", &anand_chip->csnum);
+ if (ret) {
+ dev_err(nfc->dev, "can't get chip-select\n");
+ return -ENXIO;
+ }
+
+ mtd->name = devm_kasprintf(nfc->dev, GFP_KERNEL, "arasan_nand.%d",
+ anand_chip->csnum);
+ mtd->dev.parent = nfc->dev;
+
+ chip->cmdfunc = anfc_cmd_function;
+ chip->chip_delay = 30;
+ chip->controller = &nfc->controller;
+ chip->read_buf = anfc_read_buf;
+ chip->write_buf = anfc_write_buf;
+ chip->read_byte = anfc_read_byte;
+ chip->options = NAND_BUSWIDTH_AUTO | NAND_NO_SUBPAGE_WRITE;
+ chip->bbt_options = NAND_BBT_USE_FLASH;
+ chip->select_chip = anfc_select_chip;
+ chip->onfi_set_features = anfc_onfi_set_features;
+ nand_set_flash_node(chip, np);
+
+ anand_chip->spktsize = SDR_MODE_PACKET_SIZE;
+ ret = nand_scan_ident(mtd, 1, NULL);
+ if (ret) {
+ dev_err(nfc->dev, "nand_scan_ident for NAND failed\n");
+ return ret;
+ }
+ if (chip->onfi_version) {
+ anand_chip->raddr_cycles = chip->onfi_params.addr_cycles & 0xf;
+ anand_chip->caddr_cycles =
+ (chip->onfi_params.addr_cycles >> 4) & 0xf;
+ } else {
+ /* For non-ONFI devices, configuring the address cyles as 5 */
+ anand_chip->raddr_cycles = 3;
+ anand_chip->caddr_cycles = 2;
+ }
+
+ ret = anfc_init_timing_mode(nfc, anand_chip);
+ if (ret) {
+ dev_err(nfc->dev, "timing mode init failed\n");
+ return ret;
+ }
+
+ ret = anfc_ecc_init(mtd, &chip->ecc);
+ if (ret)
+ return ret;
+
+ ret = nand_scan_tail(mtd);
+ if (ret) {
+ dev_err(nfc->dev, "nand_scan_tail for NAND failed\n");
+ return ret;
+ }
+
+ return mtd_device_register(mtd, NULL, 0);
+}
+
+static int anfc_probe(struct platform_device *pdev)
+{
+ struct anfc *nfc;
+ struct anfc_nand_chip *anand_chip;
+ struct device_node *np = pdev->dev.of_node, *child;
+ struct resource *res;
+ int err;
+
+ nfc = devm_kzalloc(&pdev->dev, sizeof(*nfc), GFP_KERNEL);
+ if (!nfc)
+ return -ENOMEM;
+
+ init_waitqueue_head(&nfc->controller.wq);
+ INIT_LIST_HEAD(&nfc->chips);
+ init_completion(&nfc->evnt);
+ nfc->dev = &pdev->dev;
+ platform_set_drvdata(pdev, nfc);
+ nfc->csnum = -1;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ nfc->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(nfc->base))
+ return PTR_ERR(nfc->base);
+ nfc->dma = of_property_read_bool(pdev->dev.of_node,
+ "arasan,has-mdma");
+ nfc->irq = platform_get_irq(pdev, 0);
+ if (nfc->irq < 0) {
+ dev_err(&pdev->dev, "platform_get_irq failed\n");
+ return -ENXIO;
+ }
+ err = devm_request_irq(&pdev->dev, nfc->irq, anfc_irq_handler,
+ 0, "arasannfc", nfc);
+ if (err)
+ return err;
+ nfc->clk_sys = devm_clk_get(&pdev->dev, "clk_sys");
+ if (IS_ERR(nfc->clk_sys)) {
+ dev_err(&pdev->dev, "sys clock not found.\n");
+ return PTR_ERR(nfc->clk_sys);
+ }
+
+ nfc->clk_flash = devm_clk_get(&pdev->dev, "clk_flash");
+ if (IS_ERR(nfc->clk_flash)) {
+ dev_err(&pdev->dev, "flash clock not found.\n");
+ return PTR_ERR(nfc->clk_flash);
+ }
+
+ err = clk_prepare_enable(nfc->clk_sys);
+ if (err) {
+ dev_err(&pdev->dev, "Unable to enable sys clock.\n");
+ return err;
+ }
+
+ err = clk_prepare_enable(nfc->clk_flash);
+ if (err) {
+ dev_err(&pdev->dev, "Unable to enable flash clock.\n");
+ goto clk_dis_sys;
+ }
+
+ for_each_available_child_of_node(np, child) {
+ anand_chip = devm_kzalloc(&pdev->dev, sizeof(*anand_chip),
+ GFP_KERNEL);
+ if (!anand_chip) {
+ of_node_put(child);
+ err = -ENOMEM;
+ goto nandchip_clean_up;
+ }
+
+ err = anfc_nand_chip_init(nfc, anand_chip, child);
+ if (err) {
+ devm_kfree(&pdev->dev, anand_chip);
+ continue;
+ }
+
+ list_add_tail(&anand_chip->node, &nfc->chips);
+ }
+
+ return 0;
+
+nandchip_clean_up:
+ list_for_each_entry(anand_chip, &nfc->chips, node)
+ nand_release(nand_to_mtd(&anand_chip->chip));
+ clk_disable_unprepare(nfc->clk_flash);
+clk_dis_sys:
+ clk_disable_unprepare(nfc->clk_sys);
+
+ return err;
+}
+
+static int anfc_remove(struct platform_device *pdev)
+{
+ struct anfc *nfc = platform_get_drvdata(pdev);
+ struct anfc_nand_chip *anand_chip;
+
+ list_for_each_entry(anand_chip, &nfc->chips, node)
+ nand_release(nand_to_mtd(&anand_chip->chip));
+
+ clk_disable_unprepare(nfc->clk_sys);
+ clk_disable_unprepare(nfc->clk_flash);
+
+ return 0;
+}
+
+static const struct of_device_id anfc_ids[] = {
+ { .compatible = "arasan,nfc-v3p10" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, anfc_ids);
+
+static struct platform_driver anfc_driver = {
+ .driver = {
+ .name = DRIVER_NAME,
+ .of_match_table = anfc_ids,
+ },
+ .probe = anfc_probe,
+ .remove = anfc_remove,
+};
+module_platform_driver(anfc_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Xilinx, Inc");
+MODULE_DESCRIPTION("Arasan NAND Flash Controller Driver");
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v6 1/2] mtd: arasan: Add device tree binding documentation
2016-12-05 4:11 [PATCH v6 1/2] mtd: arasan: Add device tree binding documentation Punnaiah Choudary Kalluri
2016-12-05 4:11 ` [PATCH v6 2/2] mtd: nand: Add support for Arasan Nand Flash Controller Punnaiah Choudary Kalluri
@ 2016-12-05 4:25 ` Marek Vasut
2016-12-05 8:36 ` Boris Brezillon
2016-12-05 13:24 ` Punnaiah Choudary Kalluri
1 sibling, 2 replies; 10+ messages in thread
From: Marek Vasut @ 2016-12-05 4:25 UTC (permalink / raw)
To: Punnaiah Choudary Kalluri, dwmw2, computersforpeace,
boris.brezillon, richard, cyrille.pitchen, robh+dt, mark.rutland
Cc: linux-kernel, linux-mtd, devicetree, michals,
kalluripunnaiahchoudary, kpc528, Punnaiah Choudary Kalluri
On 12/05/2016 05:11 AM, Punnaiah Choudary Kalluri wrote:
> This patch adds the dts binding document for arasan nand flash
> controller.
>
> Signed-off-by: Punnaiah Choudary Kalluri <punnaia@xilinx.com>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
> changes in v6:
> - Removed num-cs property
> - Separated nandchip from nand controller
> changes in v5:
> - None
> Changes in v4:
> - Added num-cs property
> - Added clock support
> Changes in v3:
> - None
> Changes in v2:
> - None
> ---
> .../devicetree/bindings/mtd/arasan_nfc.txt | 38 ++++++++++++++++++++++
> 1 file changed, 38 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mtd/arasan_nfc.txt
>
> diff --git a/Documentation/devicetree/bindings/mtd/arasan_nfc.txt b/Documentation/devicetree/bindings/mtd/arasan_nfc.txt
> new file mode 100644
> index 0000000..dcbe7ad
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/arasan_nfc.txt
> @@ -0,0 +1,38 @@
> +Arasan Nand Flash Controller with ONFI 3.1 support
Arasan NAND Flash ...
> +Required properties:
> +- compatible: Should be "arasan,nfc-v3p10"
This v3p10 looks like version 3 patchlevel 10, but shouldn't we have
some fallback option which doesn't encode IP version in the compat
string ?
Also, shouldn't quirks be handled by DT props instead of effectively
encoding them into the compatible string ?
> +- reg: Memory map for module access
> +- interrupt-parent: Interrupt controller the interrupt is routed through
> +- interrupts: Should contain the interrupt for the device
> +- clock-name: List of input clocks - "clk_sys", "clk_flash"
> + (See clock bindings for details)
> +- clocks: Clock phandles (see clock bindings for details)
> +
> +Optional properties:
> +- arasan,has-mdma: Enables Dma support
'Enables DMA support' , with DMA in caps.
> +for nand partition information please refer the below file
For NAND ...
> +Documentation/devicetree/bindings/mtd/partition.txt
> +
> +Example:
> + nand0: nand@ff100000 {
> + compatible = "arasan,nfc-v3p10"
> + reg = <0x0 0xff100000 0x1000>;
> + clock-name = "clk_sys", "clk_flash"
> + clocks = <&misc_clk &misc_clk>;
> + interrupt-parent = <&gic>;
> + interrupts = <0 14 4>;
> + arasan,has-mdma;
> + #address-cells = <1>;
> + #size-cells = <0>
> +
> + nand@0 {
> + reg = <0>
> + partition@0 {
> + label = "filesystem";
> + reg = <0x0 0x0 0x1000000>;
> + };
> + (...)
> + };
> + };
>
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v6 2/2] mtd: nand: Add support for Arasan Nand Flash Controller
2016-12-05 4:11 ` [PATCH v6 2/2] mtd: nand: Add support for Arasan Nand Flash Controller Punnaiah Choudary Kalluri
@ 2016-12-05 4:40 ` Marek Vasut
2016-12-05 13:55 ` Punnaiah Choudary Kalluri
2016-12-05 9:16 ` kbuild test robot
1 sibling, 1 reply; 10+ messages in thread
From: Marek Vasut @ 2016-12-05 4:40 UTC (permalink / raw)
To: Punnaiah Choudary Kalluri, dwmw2, computersforpeace,
boris.brezillon, richard, cyrille.pitchen, robh+dt, mark.rutland
Cc: linux-kernel, linux-mtd, devicetree, michals,
kalluripunnaiahchoudary, kpc528, Punnaiah Choudary Kalluri
On 12/05/2016 05:11 AM, Punnaiah Choudary Kalluri wrote:
> Added the basic driver for Arasan Nand Flash Controller used in
> Zynq UltraScale+ MPSoC. It supports only Hw Ecc and upto 24bit
> correction.
Ummm, NAND, ECC, can you fix the acronyms to be in caps ?
> Signed-off-by: Punnaiah Choudary Kalluri <punnaia@xilinx.com>
> ---
> Chnages in v6:
> - Addressed most of the Brian and Boris comments
> - Separated the nandchip from the nand controller
> - Removed the ecc lookup table from driver
> - Now use framework nand waitfunction and readoob
> - Fixed the compiler warning
> - Adapted the new frameowrk changes related to ecc and ooblayout
> - Disabled the clocks after the nand_reelase
> - Now using only one completion object
> - Boris suggessions like adapting cmd_ctrl and rework on read/write byte
> are not implemented and i will patch them later
> - Also check_erased_ecc_chunk for erase and check for is_vmalloc_addr will
> implement later once the basic driver is mainlined.
> Changes in v5:
> - Renamed the driver filei as arasan_nand.c
> - Fixed all comments relaqted coding style
> - Fixed comments related to propagating the errors
> - Modified the anfc_write_page_hwecc as per the write_page
> prototype
> Changes in v4:
> - Added support for onfi timing mode configuration
> - Added clock supppport
> - Added support for multiple chipselects
> Changes in v3:
> - Removed unused variables
> - Avoided busy loop and used jifies based implementation
> - Fixed compiler warnings "right shift count >= width of type"
> - Removed unneeded codei and improved error reporting
> - Added onfi version check to ensure reading the valid address cycles
> Changes in v2:
> - Added missing of.h to avoid kbuild system report erro
> ---
> drivers/mtd/nand/Kconfig | 8 +
> drivers/mtd/nand/Makefile | 1 +
> drivers/mtd/nand/arasan_nand.c | 974 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 983 insertions(+)
> create mode 100644 drivers/mtd/nand/arasan_nand.c
>
> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> index 7b7a887..e831f4e 100644
> --- a/drivers/mtd/nand/Kconfig
> +++ b/drivers/mtd/nand/Kconfig
> @@ -569,4 +569,12 @@ config MTD_NAND_MTK
> Enables support for NAND controller on MTK SoCs.
> This controller is found on mt27xx, mt81xx, mt65xx SoCs.
>
> +config MTD_NAND_ARASAN
> + tristate "Support for Arasan Nand Flash controller"
> + depends on HAS_IOMEM
> + depends on HAS_DMA
> + help
> + Enables the driver for the Arasan Nand Flash controller on
> + Zynq UltraScale+ MPSoC.
> +
> endif # MTD_NAND
> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> index cafde6f..44b8b00 100644
> --- a/drivers/mtd/nand/Makefile
> +++ b/drivers/mtd/nand/Makefile
> @@ -58,5 +58,6 @@ obj-$(CONFIG_MTD_NAND_HISI504) += hisi504_nand.o
> obj-$(CONFIG_MTD_NAND_BRCMNAND) += brcmnand/
> obj-$(CONFIG_MTD_NAND_QCOM) += qcom_nandc.o
> obj-$(CONFIG_MTD_NAND_MTK) += mtk_nand.o mtk_ecc.o
> +obj-$(CONFIG_MTD_NAND_ARASAN) += arasan_nand.o
Keep the list at least reasonably sorted.
> nand-objs := nand_base.o nand_bbt.o nand_timings.o
> diff --git a/drivers/mtd/nand/arasan_nand.c b/drivers/mtd/nand/arasan_nand.c
> new file mode 100644
> index 0000000..6b0670e
> --- /dev/null
> +++ b/drivers/mtd/nand/arasan_nand.c
> @@ -0,0 +1,974 @@
> +/*
> + * Arasan Nand Flash Controller Driver
NAND
> + * Copyright (C) 2014 - 2015 Xilinx, Inc.
It's 2016 now ...
> + * This program is free software; you can redistribute it and/or modify it under
> + * the terms of the GNU General Public License version 2 as published by the
> + * Free Software Foundation; either version 2 of the License, or (at your
> + * option) any later version.
> + */
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/interrupt.h>
> +#include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/module.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/nand.h>
> +#include <linux/mtd/partitions.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +
> +#define DRIVER_NAME "arasan_nand"
> +#define EVNT_TIMEOUT 1000
Rename to EVENT_TIMEOUT_<units> to make this less cryptic
> +#define STATUS_TIMEOUT 2000
DTTO
> +#define PKT_OFST 0x00
> +#define MEM_ADDR1_OFST 0x04
> +#define MEM_ADDR2_OFST 0x08
> +#define CMD_OFST 0x0C
> +#define PROG_OFST 0x10
> +#define INTR_STS_EN_OFST 0x14
> +#define INTR_SIG_EN_OFST 0x18
> +#define INTR_STS_OFST 0x1C
> +#define READY_STS_OFST 0x20
> +#define DMA_ADDR1_OFST 0x24
> +#define FLASH_STS_OFST 0x28
> +#define DATA_PORT_OFST 0x30
> +#define ECC_OFST 0x34
> +#define ECC_ERR_CNT_OFST 0x38
> +#define ECC_SPR_CMD_OFST 0x3C
> +#define ECC_ERR_CNT_1BIT_OFST 0x40
> +#define ECC_ERR_CNT_2BIT_OFST 0x44
> +#define DMA_ADDR0_OFST 0x50
> +#define DATA_INTERFACE_REG 0x6C
Why are some things suffixed with _OFST and some with _REG ? Consistency
please. Using ARASAN_ prefix, ie. #define ARASAN_FOO 0xbar to define
regs would be nice.
> +#define PKT_CNT_SHIFT 12
> +
> +#define ECC_ENABLE BIT(31)
> +#define DMA_EN_MASK GENMASK(27, 26)
> +#define DMA_ENABLE 0x2
> +#define DMA_EN_SHIFT 26
> +#define REG_PAGE_SIZE_MASK GENMASK(25, 23)
> +#define REG_PAGE_SIZE_SHIFT 23
> +#define REG_PAGE_SIZE_512 0
> +#define REG_PAGE_SIZE_1K 5
> +#define REG_PAGE_SIZE_2K 1
> +#define REG_PAGE_SIZE_4K 2
> +#define REG_PAGE_SIZE_8K 3
> +#define REG_PAGE_SIZE_16K 4
> +#define CMD2_SHIFT 8
> +#define ADDR_CYCLES_SHIFT 28
> +
> +#define XFER_COMPLETE BIT(2)
> +#define READ_READY BIT(1)
> +#define WRITE_READY BIT(0)
> +#define MBIT_ERROR BIT(3)
> +#define ERR_INTRPT BIT(4)
> +
> +#define PROG_PGRD BIT(0)
> +#define PROG_ERASE BIT(2)
> +#define PROG_STATUS BIT(3)
> +#define PROG_PGPROG BIT(4)
> +#define PROG_RDID BIT(6)
> +#define PROG_RDPARAM BIT(7)
> +#define PROG_RST BIT(8)
> +#define PROG_GET_FEATURE BIT(9)
> +#define PROG_SET_FEATURE BIT(10)
> +
> +#define ONFI_STATUS_FAIL BIT(0)
> +#define ONFI_STATUS_READY BIT(6)
> +
> +#define PG_ADDR_SHIFT 16
> +#define BCH_MODE_SHIFT 25
> +#define BCH_EN_SHIFT 27
> +#define ECC_SIZE_SHIFT 16
> +
> +#define MEM_ADDR_MASK GENMASK(7, 0)
> +#define BCH_MODE_MASK GENMASK(27, 25)
> +
> +#define CS_MASK GENMASK(31, 30)
> +#define CS_SHIFT 30
> +
> +#define PAGE_ERR_CNT_MASK GENMASK(16, 8)
> +#define PKT_ERR_CNT_MASK GENMASK(7, 0)
> +
> +#define NVDDR_MODE BIT(9)
> +#define NVDDR_TIMING_MODE_SHIFT 3
> +
> +#define ONFI_ID_LEN 8
> +#define TEMP_BUF_SIZE 512
> +#define NVDDR_MODE_PACKET_SIZE 8
> +#define SDR_MODE_PACKET_SIZE 4
> +
> +#define ONFI_DATA_INTERFACE_NVDDR (1 << 4)
BIT() ?
[...]
> +struct anfc {
> + struct nand_hw_control controller;
> + struct list_head chips;
> + struct device *dev;
> + void __iomem *base;
> + int curr_cmd;
> + struct clk *clk_sys;
> + struct clk *clk_flash;
> + bool dma;
> + bool err;
> + bool iswriteoob;
> + u8 buf[TEMP_BUF_SIZE];
> + int irq;
> + u32 bufshift;
> + int csnum;
> + struct completion evnt;
event ?
> +};
[...]
> +static void anfc_prepare_cmd(struct anfc *nfc, u8 cmd1, u8 cmd2, u8 dmamode,
> + u32 pagesize, u8 addrcycles)
> +{
> + u32 regval;
> +
> + regval = cmd1 | (cmd2 << CMD2_SHIFT);
> + if (dmamode && nfc->dma)
> + regval |= DMA_ENABLE << DMA_EN_SHIFT;
> + if (addrcycles)
> + regval |= addrcycles << ADDR_CYCLES_SHIFT;
> + if (pagesize)
> + regval |= anfc_page(pagesize) << REG_PAGE_SIZE_SHIFT;
Drop the if (foo), if it's zero, the regval would be OR'd with zero.
> + writel(regval, nfc->base + CMD_OFST);
> +}
> +
> +static int anfc_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
> + int page)
> +{
> + struct anfc *nfc = to_anfc(chip->controller);
> +
> + nfc->iswriteoob = true;
> + chip->cmdfunc(mtd, NAND_CMD_SEQIN, mtd->writesize, page);
> + chip->write_buf(mtd, chip->oob_poi, mtd->oobsize);
> + nfc->iswriteoob = false;
> +
> + return 0;
> +}
> +
> +static void anfc_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
> +{
> + u32 pktcount, pktsize, eccintr = 0;
> + unsigned int buf_rd_cnt = 0;
> + u32 *bufptr = (u32 *)buf;
> + struct nand_chip *chip = mtd_to_nand(mtd);
> + struct anfc_nand_chip *achip = to_anfc_nand(chip);
> + struct anfc *nfc = to_anfc(chip->controller);
> + dma_addr_t paddr;
> +
> + if (nfc->curr_cmd == NAND_CMD_READ0) {
> + pktsize = achip->pktsize;
> + pktcount = DIV_ROUND_UP(mtd->writesize, pktsize);
> + if (!achip->bch)
> + eccintr = MBIT_ERROR;
> + } else {
> + pktsize = len;
> + pktcount = 1;
> + }
> +
> + anfc_setpktszcnt(nfc, pktsize, pktcount);
> +
> + if (nfc->dma) {
> + paddr = dma_map_single(nfc->dev, buf, len, DMA_FROM_DEVICE);
> + if (dma_mapping_error(nfc->dev, paddr)) {
> + dev_err(nfc->dev, "Read buffer mapping error");
> + return;
> + }
> + lo_hi_writeq(paddr, nfc->base + DMA_ADDR0_OFST);
> + anfc_enable_intrs(nfc, (XFER_COMPLETE | eccintr));
> + writel(PROG_PGRD, nfc->base + PROG_OFST);
> + anfc_wait_for_event(nfc);
> + dma_unmap_single(nfc->dev, paddr, len, DMA_FROM_DEVICE);
Split this function into anfc_read_buf() and then anfc_read_buf_pio()
and anfc_read_buf_dma() to avoid this ugliness. Also, does this handle
any errors in any way ? Looks like it ignores all errors, so please fix.
> + return;
> + }
> +
> + anfc_enable_intrs(nfc, (READ_READY | eccintr));
> + writel(PROG_PGRD, nfc->base + PROG_OFST);
> +
> + while (buf_rd_cnt < pktcount) {
> + anfc_wait_for_event(nfc);
> + buf_rd_cnt++;
> +
> + if (buf_rd_cnt == pktcount)
> + anfc_enable_intrs(nfc, XFER_COMPLETE);
> +
> + readsl(nfc->base + DATA_PORT_OFST, bufptr, pktsize/4);
> + bufptr += (pktsize / 4);
> +
> + if (buf_rd_cnt < pktcount)
> + anfc_enable_intrs(nfc, (READ_READY | eccintr));
> + }
> +
> + anfc_wait_for_event(nfc);
> +}
> +
> +static void anfc_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len)
> +{
> + u32 pktcount, pktsize;
> + unsigned int buf_wr_cnt = 0;
> + u32 *bufptr = (u32 *)buf;
> + struct nand_chip *chip = mtd_to_nand(mtd);
> + struct anfc_nand_chip *achip = to_anfc_nand(chip);
> + struct anfc *nfc = to_anfc(chip->controller);
> + dma_addr_t paddr;
> +
> + if (nfc->iswriteoob) {
> + pktsize = len;
> + pktcount = 1;
> + } else {
> + pktsize = achip->pktsize;
> + pktcount = mtd->writesize / pktsize;
> + }
This looks like a copy of the read path. Can these two functions be
parametrized and merged ?
> + anfc_setpktszcnt(nfc, pktsize, pktcount);
> +
> + if (nfc->dma) {
> + paddr = dma_map_single(nfc->dev, (void *)buf, len,
> + DMA_TO_DEVICE);
> + if (dma_mapping_error(nfc->dev, paddr)) {
> + dev_err(nfc->dev, "Write buffer mapping error");
> + return;
> + }
> + lo_hi_writeq(paddr, nfc->base + DMA_ADDR0_OFST);
> + anfc_enable_intrs(nfc, XFER_COMPLETE);
> + writel(PROG_PGPROG, nfc->base + PROG_OFST);
> + anfc_wait_for_event(nfc);
> + dma_unmap_single(nfc->dev, paddr, len, DMA_TO_DEVICE);
> + return;
> + }
> +
> + anfc_enable_intrs(nfc, WRITE_READY);
> + writel(PROG_PGPROG, nfc->base + PROG_OFST);
> +
> + while (buf_wr_cnt < pktcount) {
> + anfc_wait_for_event(nfc);
> + buf_wr_cnt++;
> + if (buf_wr_cnt == pktcount)
> + anfc_enable_intrs(nfc, XFER_COMPLETE);
> +
> + writesl(nfc->base + DATA_PORT_OFST, bufptr, pktsize/4);
> + bufptr += (pktsize / 4);
> +
> + if (buf_wr_cnt < pktcount)
> + anfc_enable_intrs(nfc, WRITE_READY);
> + }
> +
> + anfc_wait_for_event(nfc);
> +}
[...]
> +static void anfc_writefifo(struct anfc *nfc, u32 prog, u32 size, u8 *buf)
> +{
> + u32 *bufptr = (u32 *)buf;
> +
> + anfc_enable_intrs(nfc, WRITE_READY);
> +
> + writel(prog, nfc->base + PROG_OFST);
> + anfc_wait_for_event(nfc);
> +
> + anfc_enable_intrs(nfc, XFER_COMPLETE);
> + writesl(nfc->base + DATA_PORT_OFST, bufptr, size/4);
use ioread32_rep and iowrite32_rep , otherwise this won't compile on x86
with COMPILE_TEST.
> + anfc_wait_for_event(nfc);
> +}
> +
> +static void anfc_readfifo(struct anfc *nfc, u32 prog, u32 size)
> +{
> + u32 *bufptr = (u32 *)nfc->buf;
> +
> + anfc_enable_intrs(nfc, READ_READY);
> +
> + writel(prog, nfc->base + PROG_OFST);
> + anfc_wait_for_event(nfc);
> +
> + anfc_enable_intrs(nfc, XFER_COMPLETE);
> + readsl(nfc->base + DATA_PORT_OFST, bufptr, size/4);
See above
> + anfc_wait_for_event(nfc);
> +}
[...]
> +static void anfc_select_chip(struct mtd_info *mtd, int num)
> +{
> + u32 val;
> + struct nand_chip *chip = mtd_to_nand(mtd);
> + struct anfc_nand_chip *achip = to_anfc_nand(chip);
> + struct anfc *nfc = to_anfc(chip->controller);
> +
> + if (num == -1)
> + return;
> +
> + val = readl(nfc->base + MEM_ADDR2_OFST);
> + val = (val & ~(CS_MASK)) | (achip->csnum << CS_SHIFT);
> + val = (val & ~(BCH_MODE_MASK)) | (achip->bchmode << BCH_MODE_SHIFT);
Just rewrite this as a series of val &= ~(foo | bar); val |= baz | quux;
for clarity sake.
> + writel(val, nfc->base + MEM_ADDR2_OFST);
> + nfc->csnum = achip->csnum;
> + writel(achip->eccval, nfc->base + ECC_OFST);
> + writel(achip->inftimeval, nfc->base + DATA_INTERFACE_REG);
> +}
> +
> +static irqreturn_t anfc_irq_handler(int irq, void *ptr)
> +{
> + struct anfc *nfc = ptr;
> + u32 regval = 0, status;
> +
> + status = readl(nfc->base + INTR_STS_OFST);
> + if (status & XFER_COMPLETE) {
> + complete(&nfc->evnt);
> + regval |= XFER_COMPLETE;
Can the complete() be invoked multiple times ? That seems a bit odd.
> + }
> +
> + if (status & READ_READY) {
> + complete(&nfc->evnt);
> + regval |= READ_READY;
> + }
> +
> + if (status & WRITE_READY) {
> + complete(&nfc->evnt);
> + regval |= WRITE_READY;
> + }
> +
> + if (status & MBIT_ERROR) {
> + nfc->err = true;
> + complete(&nfc->evnt);
> + regval |= MBIT_ERROR;
> + }
> +
> + if (regval) {
> + writel(regval, nfc->base + INTR_STS_OFST);
> + writel(0, nfc->base + INTR_STS_EN_OFST);
> + writel(0, nfc->base + INTR_SIG_EN_OFST);
> +
> + return IRQ_HANDLED;
> + }
> +
> + return IRQ_NONE;
> +}
> +
> +static int anfc_onfi_set_features(struct mtd_info *mtd, struct nand_chip *chip,
> + int addr, uint8_t *subfeature_param)
> +{
> + struct anfc *nfc = to_anfc(chip->controller);
> + struct anfc_nand_chip *achip = to_anfc_nand(chip);
> + int status;
> +
> + if (!chip->onfi_version || !(le16_to_cpu(chip->onfi_params.opt_cmd)
> + & ONFI_OPT_CMD_SET_GET_FEATURES))
Split this into two conditions to improve readability.
> + return -EINVAL;
> +
> + chip->cmdfunc(mtd, NAND_CMD_SET_FEATURES, addr, -1);
> + anfc_writefifo(nfc, PROG_SET_FEATURE, achip->spktsize,
> + subfeature_param);
> +
> + status = chip->waitfunc(mtd, chip);
> + if (status & NAND_STATUS_FAIL)
> + return -EIO;
> +
> + return 0;
> +}
> +
> +static int anfc_init_timing_mode(struct anfc *nfc,
> + struct anfc_nand_chip *achip)
> +{
> + int mode, err;
> + unsigned int feature[2];
> + u32 inftimeval;
> + struct nand_chip *chip = &achip->chip;
> + struct mtd_info *mtd = nand_to_mtd(chip);
> +
> + memset(feature, 0, NVDDR_MODE_PACKET_SIZE);
> + /* Get nvddr timing modes */
> + mode = onfi_get_sync_timing_mode(chip) & 0xff;
> + if (!mode) {
> + mode = fls(onfi_get_async_timing_mode(chip)) - 1;
> + inftimeval = mode;
> + } else {
> + mode = fls(mode) - 1;
> + inftimeval = NVDDR_MODE | (mode << NVDDR_TIMING_MODE_SHIFT);
> + mode |= ONFI_DATA_INTERFACE_NVDDR;
> + }
> +
> + feature[0] = mode;
> + chip->select_chip(mtd, achip->csnum);
> + err = chip->onfi_set_features(mtd, chip, ONFI_FEATURE_ADDR_TIMING_MODE,
> + (uint8_t *)feature);
> + chip->select_chip(mtd, -1);
> + if (err)
> + return err;
> +
> + achip->inftimeval = inftimeval;
> +
> + if (mode & ONFI_DATA_INTERFACE_NVDDR)
> + achip->spktsize = NVDDR_MODE_PACKET_SIZE;
> +
> + return 0;
> +}
[...]
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Xilinx, Inc");
There should be a contact with email address here.
> +MODULE_DESCRIPTION("Arasan NAND Flash Controller Driver");
>
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v6 1/2] mtd: arasan: Add device tree binding documentation
2016-12-05 4:25 ` [PATCH v6 1/2] mtd: arasan: Add device tree binding documentation Marek Vasut
@ 2016-12-05 8:36 ` Boris Brezillon
2016-12-05 13:56 ` Punnaiah Choudary Kalluri
2016-12-05 16:11 ` Marek Vasut
2016-12-05 13:24 ` Punnaiah Choudary Kalluri
1 sibling, 2 replies; 10+ messages in thread
From: Boris Brezillon @ 2016-12-05 8:36 UTC (permalink / raw)
To: Marek Vasut
Cc: Punnaiah Choudary Kalluri, dwmw2, computersforpeace, richard,
cyrille.pitchen, robh+dt, mark.rutland, linux-kernel, linux-mtd,
devicetree, michals, kalluripunnaiahchoudary, kpc528,
Punnaiah Choudary Kalluri
On Mon, 5 Dec 2016 05:25:54 +0100
Marek Vasut <marek.vasut@gmail.com> wrote:
> On 12/05/2016 05:11 AM, Punnaiah Choudary Kalluri wrote:
> > This patch adds the dts binding document for arasan nand flash
> > controller.
> >
> > Signed-off-by: Punnaiah Choudary Kalluri <punnaia@xilinx.com>
> > Acked-by: Rob Herring <robh@kernel.org>
> > ---
> > changes in v6:
> > - Removed num-cs property
> > - Separated nandchip from nand controller
> > changes in v5:
> > - None
> > Changes in v4:
> > - Added num-cs property
> > - Added clock support
> > Changes in v3:
> > - None
> > Changes in v2:
> > - None
> > ---
> > .../devicetree/bindings/mtd/arasan_nfc.txt | 38 ++++++++++++++++++++++
> > 1 file changed, 38 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/mtd/arasan_nfc.txt
> >
> > diff --git a/Documentation/devicetree/bindings/mtd/arasan_nfc.txt b/Documentation/devicetree/bindings/mtd/arasan_nfc.txt
> > new file mode 100644
> > index 0000000..dcbe7ad
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mtd/arasan_nfc.txt
> > @@ -0,0 +1,38 @@
> > +Arasan Nand Flash Controller with ONFI 3.1 support
>
> Arasan NAND Flash ...
>
> > +Required properties:
> > +- compatible: Should be "arasan,nfc-v3p10"
>
> This v3p10 looks like version 3 patchlevel 10, but shouldn't we have
> some fallback option which doesn't encode IP version in the compat
> string ?
Not necessarily. Usually you define a generic compatible when you have
other reliable means to detect the IP version (a version register for
example).
If you can't detect that at runtime, then providing only specific
compatible strings is a good solution to avoid breaking the DT ABI.
>
> Also, shouldn't quirks be handled by DT props instead of effectively
> encoding them into the compatible string ?
Well, from my experience, it's better to hide as much as possible
behind the compatible. This way, if new quirks are needed for a
specific revision, you can update the driver without having to change
the DT.
>
> > +- reg: Memory map for module access
> > +- interrupt-parent: Interrupt controller the interrupt is routed through
> > +- interrupts: Should contain the interrupt for the device
> > +- clock-name: List of input clocks - "clk_sys", "clk_flash"
> > + (See clock bindings for details)
> > +- clocks: Clock phandles (see clock bindings for details)
> > +
> > +Optional properties:
> > +- arasan,has-mdma: Enables Dma support
>
> 'Enables DMA support' , with DMA in caps.
>
> > +for nand partition information please refer the below file
>
> For NAND ...
>
> > +Documentation/devicetree/bindings/mtd/partition.txt
> > +
> > +Example:
> > + nand0: nand@ff100000 {
> > + compatible = "arasan,nfc-v3p10"
> > + reg = <0x0 0xff100000 0x1000>;
> > + clock-name = "clk_sys", "clk_flash"
> > + clocks = <&misc_clk &misc_clk>;
> > + interrupt-parent = <&gic>;
> > + interrupts = <0 14 4>;
> > + arasan,has-mdma;
> > + #address-cells = <1>;
> > + #size-cells = <0>
> > +
> > + nand@0 {
> > + reg = <0>
> > + partition@0 {
> > + label = "filesystem";
> > + reg = <0x0 0x0 0x1000000>;
> > + };
> > + (...)
> > + };
> > + };
> >
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v6 2/2] mtd: nand: Add support for Arasan Nand Flash Controller
2016-12-05 4:11 ` [PATCH v6 2/2] mtd: nand: Add support for Arasan Nand Flash Controller Punnaiah Choudary Kalluri
2016-12-05 4:40 ` Marek Vasut
@ 2016-12-05 9:16 ` kbuild test robot
1 sibling, 0 replies; 10+ messages in thread
From: kbuild test robot @ 2016-12-05 9:16 UTC (permalink / raw)
To: Punnaiah Choudary Kalluri
Cc: kbuild-all, dwmw2, computersforpeace, boris.brezillon,
marek.vasut, richard, cyrille.pitchen, robh+dt, mark.rutland,
linux-kernel, linux-mtd, devicetree, michals,
kalluripunnaiahchoudary, kpc528, Punnaiah Choudary Kalluri
[-- Attachment #1: Type: text/plain, Size: 3489 bytes --]
Hi Punnaiah,
[auto build test ERROR on mtd/master]
[also build test ERROR on v4.9-rc8 next-20161205]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Punnaiah-Choudary-Kalluri/mtd-arasan-Add-device-tree-binding-documentation/20161205-162929
base: git://git.infradead.org/linux-mtd.git master
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/mtd/nand/arasan_nand.c: In function 'anfc_read_buf':
>> drivers/mtd/nand/arasan_nand.c:348:3: error: implicit declaration of function 'readsl' [-Werror=implicit-function-declaration]
readsl(nfc->base + DATA_PORT_OFST, bufptr, pktsize/4);
^~~~~~
drivers/mtd/nand/arasan_nand.c: In function 'anfc_write_buf':
>> drivers/mtd/nand/arasan_nand.c:402:3: error: implicit declaration of function 'writesl' [-Werror=implicit-function-declaration]
writesl(nfc->base + DATA_PORT_OFST, bufptr, pktsize/4);
^~~~~~~
cc1: some warnings being treated as errors
vim +/readsl +348 drivers/mtd/nand/arasan_nand.c
342 anfc_wait_for_event(nfc);
343 buf_rd_cnt++;
344
345 if (buf_rd_cnt == pktcount)
346 anfc_enable_intrs(nfc, XFER_COMPLETE);
347
> 348 readsl(nfc->base + DATA_PORT_OFST, bufptr, pktsize/4);
349 bufptr += (pktsize / 4);
350
351 if (buf_rd_cnt < pktcount)
352 anfc_enable_intrs(nfc, (READ_READY | eccintr));
353 }
354
355 anfc_wait_for_event(nfc);
356 }
357
358 static void anfc_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len)
359 {
360 u32 pktcount, pktsize;
361 unsigned int buf_wr_cnt = 0;
362 u32 *bufptr = (u32 *)buf;
363 struct nand_chip *chip = mtd_to_nand(mtd);
364 struct anfc_nand_chip *achip = to_anfc_nand(chip);
365 struct anfc *nfc = to_anfc(chip->controller);
366 dma_addr_t paddr;
367
368 if (nfc->iswriteoob) {
369 pktsize = len;
370 pktcount = 1;
371 } else {
372 pktsize = achip->pktsize;
373 pktcount = mtd->writesize / pktsize;
374 }
375
376 anfc_setpktszcnt(nfc, pktsize, pktcount);
377
378 if (nfc->dma) {
379 paddr = dma_map_single(nfc->dev, (void *)buf, len,
380 DMA_TO_DEVICE);
381 if (dma_mapping_error(nfc->dev, paddr)) {
382 dev_err(nfc->dev, "Write buffer mapping error");
383 return;
384 }
385 lo_hi_writeq(paddr, nfc->base + DMA_ADDR0_OFST);
386 anfc_enable_intrs(nfc, XFER_COMPLETE);
387 writel(PROG_PGPROG, nfc->base + PROG_OFST);
388 anfc_wait_for_event(nfc);
389 dma_unmap_single(nfc->dev, paddr, len, DMA_TO_DEVICE);
390 return;
391 }
392
393 anfc_enable_intrs(nfc, WRITE_READY);
394 writel(PROG_PGPROG, nfc->base + PROG_OFST);
395
396 while (buf_wr_cnt < pktcount) {
397 anfc_wait_for_event(nfc);
398 buf_wr_cnt++;
399 if (buf_wr_cnt == pktcount)
400 anfc_enable_intrs(nfc, XFER_COMPLETE);
401
> 402 writesl(nfc->base + DATA_PORT_OFST, bufptr, pktsize/4);
403 bufptr += (pktsize / 4);
404
405 if (buf_wr_cnt < pktcount)
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 56586 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v6 1/2] mtd: arasan: Add device tree binding documentation
2016-12-05 4:25 ` [PATCH v6 1/2] mtd: arasan: Add device tree binding documentation Marek Vasut
2016-12-05 8:36 ` Boris Brezillon
@ 2016-12-05 13:24 ` Punnaiah Choudary Kalluri
1 sibling, 0 replies; 10+ messages in thread
From: Punnaiah Choudary Kalluri @ 2016-12-05 13:24 UTC (permalink / raw)
To: Marek Vasut, dwmw2, computersforpeace, boris.brezillon, richard,
cyrille.pitchen, robh+dt, mark.rutland
Cc: linux-kernel, linux-mtd, devicetree, Michal Simek,
kalluripunnaiahchoudary, kpc528
Hi Marek,
Thanks for the review.
> -----Original Message-----
> From: Marek Vasut [mailto:marek.vasut@gmail.com]
> Sent: Monday, December 05, 2016 9:56 AM
> To: Punnaiah Choudary Kalluri <punnaia@xilinx.com>;
> dwmw2@infradead.org; computersforpeace@gmail.com;
> boris.brezillon@free-electrons.com; richard@nod.at;
> cyrille.pitchen@atmel.com; robh+dt@kernel.org; mark.rutland@arm.com
> Cc: linux-kernel@vger.kernel.org; linux-mtd@lists.infradead.org;
> devicetree@vger.kernel.org; Michal Simek <michals@xilinx.com>;
> kalluripunnaiahchoudary@gmail.com; kpc528@gmail.com; Punnaiah
> Choudary Kalluri <punnaia@xilinx.com>
> Subject: Re: [PATCH v6 1/2] mtd: arasan: Add device tree binding
> documentation
>
> On 12/05/2016 05:11 AM, Punnaiah Choudary Kalluri wrote:
> > This patch adds the dts binding document for arasan nand flash
> > controller.
> >
> > Signed-off-by: Punnaiah Choudary Kalluri <punnaia@xilinx.com>
> > Acked-by: Rob Herring <robh@kernel.org>
> > ---
> > changes in v6:
> > - Removed num-cs property
> > - Separated nandchip from nand controller
> > changes in v5:
> > - None
> > Changes in v4:
> > - Added num-cs property
> > - Added clock support
> > Changes in v3:
> > - None
> > Changes in v2:
> > - None
> > ---
> > .../devicetree/bindings/mtd/arasan_nfc.txt | 38
> ++++++++++++++++++++++
> > 1 file changed, 38 insertions(+)
> > create mode 100644
> Documentation/devicetree/bindings/mtd/arasan_nfc.txt
> >
> > diff --git a/Documentation/devicetree/bindings/mtd/arasan_nfc.txt
> b/Documentation/devicetree/bindings/mtd/arasan_nfc.txt
> > new file mode 100644
> > index 0000000..dcbe7ad
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mtd/arasan_nfc.txt
> > @@ -0,0 +1,38 @@
> > +Arasan Nand Flash Controller with ONFI 3.1 support
>
> Arasan NAND Flash ...
>
> > +Required properties:
> > +- compatible: Should be "arasan,nfc-v3p10"
>
> This v3p10 looks like version 3 patchlevel 10, but shouldn't we have
> some fallback option which doesn't encode IP version in the compat
> string ?
>
This is a third-party IP and v3p10 is the version that we are using in our SOC.
Also this IP doesn’t have the IP version information in the register space to
read dynamically and having generic compatible name. So, any new versions
can be added through of_match_table with config data inside the driver.
> Also, shouldn't quirks be handled by DT props instead of effectively
> encoding them into the compatible string ?
>
I feel the above mentioned method will be more appropriate rather than defining
the quirks through DT properties.
I will fix all other comments
Regards,
Punnaiah
> > +- reg: Memory map for module access
> > +- interrupt-parent: Interrupt controller the interrupt is routed through
> > +- interrupts: Should contain the interrupt for the device
> > +- clock-name: List of input clocks - "clk_sys", "clk_flash"
> > + (See clock bindings for details)
> > +- clocks: Clock phandles (see clock bindings for details)
> > +
> > +Optional properties:
> > +- arasan,has-mdma: Enables Dma support
>
> 'Enables DMA support' , with DMA in caps.
>
> > +for nand partition information please refer the below file
>
> For NAND ...
>
> > +Documentation/devicetree/bindings/mtd/partition.txt
> > +
> > +Example:
> > + nand0: nand@ff100000 {
> > + compatible = "arasan,nfc-v3p10"
> > + reg = <0x0 0xff100000 0x1000>;
> > + clock-name = "clk_sys", "clk_flash"
> > + clocks = <&misc_clk &misc_clk>;
> > + interrupt-parent = <&gic>;
> > + interrupts = <0 14 4>;
> > + arasan,has-mdma;
> > + #address-cells = <1>;
> > + #size-cells = <0>
> > +
> > + nand@0 {
> > + reg = <0>
> > + partition@0 {
> > + label = "filesystem";
> > + reg = <0x0 0x0 0x1000000>;
> > + };
> > + (...)
> > + };
> > + };
> >
>
>
> --
> Best regards,
> Marek Vasut
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v6 2/2] mtd: nand: Add support for Arasan Nand Flash Controller
2016-12-05 4:40 ` Marek Vasut
@ 2016-12-05 13:55 ` Punnaiah Choudary Kalluri
0 siblings, 0 replies; 10+ messages in thread
From: Punnaiah Choudary Kalluri @ 2016-12-05 13:55 UTC (permalink / raw)
To: Marek Vasut, dwmw2, computersforpeace, boris.brezillon, richard,
cyrille.pitchen, robh+dt, mark.rutland
Cc: linux-kernel, linux-mtd, devicetree, Michal Simek,
kalluripunnaiahchoudary, kpc528
Hi Marek,
Thanks for the review and comments.
> -----Original Message-----
> From: Marek Vasut [mailto:marek.vasut@gmail.com]
> Sent: Monday, December 05, 2016 10:10 AM
> To: Punnaiah Choudary Kalluri <punnaia@xilinx.com>;
> dwmw2@infradead.org; computersforpeace@gmail.com;
> boris.brezillon@free-electrons.com; richard@nod.at;
> cyrille.pitchen@atmel.com; robh+dt@kernel.org; mark.rutland@arm.com
> Cc: linux-kernel@vger.kernel.org; linux-mtd@lists.infradead.org;
> devicetree@vger.kernel.org; Michal Simek <michals@xilinx.com>;
> kalluripunnaiahchoudary@gmail.com; kpc528@gmail.com; Punnaiah
> Choudary Kalluri <punnaia@xilinx.com>
> Subject: Re: [PATCH v6 2/2] mtd: nand: Add support for Arasan Nand Flash
> Controller
>
> On 12/05/2016 05:11 AM, Punnaiah Choudary Kalluri wrote:
> > Added the basic driver for Arasan Nand Flash Controller used in
> > Zynq UltraScale+ MPSoC. It supports only Hw Ecc and upto 24bit
> > correction.
>
> Ummm, NAND, ECC, can you fix the acronyms to be in caps ?
>
Ok
> > Signed-off-by: Punnaiah Choudary Kalluri <punnaia@xilinx.com>
> > ---
> > Chnages in v6:
> > - Addressed most of the Brian and Boris comments
> > - Separated the nandchip from the nand controller
> > - Removed the ecc lookup table from driver
> > - Now use framework nand waitfunction and readoob
> > - Fixed the compiler warning
> > - Adapted the new frameowrk changes related to ecc and ooblayout
> > - Disabled the clocks after the nand_reelase
> > - Now using only one completion object
> > - Boris suggessions like adapting cmd_ctrl and rework on read/write byte
> > are not implemented and i will patch them later
> > - Also check_erased_ecc_chunk for erase and check for is_vmalloc_addr
> will
> > implement later once the basic driver is mainlined.
> > Changes in v5:
> > - Renamed the driver filei as arasan_nand.c
> > - Fixed all comments relaqted coding style
> > - Fixed comments related to propagating the errors
> > - Modified the anfc_write_page_hwecc as per the write_page
> > prototype
> > Changes in v4:
> > - Added support for onfi timing mode configuration
> > - Added clock supppport
> > - Added support for multiple chipselects
> > Changes in v3:
> > - Removed unused variables
> > - Avoided busy loop and used jifies based implementation
> > - Fixed compiler warnings "right shift count >= width of type"
> > - Removed unneeded codei and improved error reporting
> > - Added onfi version check to ensure reading the valid address cycles
> > Changes in v2:
> > - Added missing of.h to avoid kbuild system report erro
> > ---
> > drivers/mtd/nand/Kconfig | 8 +
> > drivers/mtd/nand/Makefile | 1 +
> > drivers/mtd/nand/arasan_nand.c | 974
> +++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 983 insertions(+)
> > create mode 100644 drivers/mtd/nand/arasan_nand.c
> >
> > diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> > index 7b7a887..e831f4e 100644
> > --- a/drivers/mtd/nand/Kconfig
> > +++ b/drivers/mtd/nand/Kconfig
> > @@ -569,4 +569,12 @@ config MTD_NAND_MTK
> > Enables support for NAND controller on MTK SoCs.
> > This controller is found on mt27xx, mt81xx, mt65xx SoCs.
> >
> > +config MTD_NAND_ARASAN
> > + tristate "Support for Arasan Nand Flash controller"
> > + depends on HAS_IOMEM
> > + depends on HAS_DMA
> > + help
> > + Enables the driver for the Arasan Nand Flash controller on
> > + Zynq UltraScale+ MPSoC.
> > +
> > endif # MTD_NAND
> > diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> > index cafde6f..44b8b00 100644
> > --- a/drivers/mtd/nand/Makefile
> > +++ b/drivers/mtd/nand/Makefile
> > @@ -58,5 +58,6 @@ obj-$(CONFIG_MTD_NAND_HISI504) +=
> hisi504_nand.o
> > obj-$(CONFIG_MTD_NAND_BRCMNAND) += brcmnand/
> > obj-$(CONFIG_MTD_NAND_QCOM) += qcom_nandc.o
> > obj-$(CONFIG_MTD_NAND_MTK) += mtk_nand.o mtk_ecc.o
> > +obj-$(CONFIG_MTD_NAND_ARASAN) += arasan_nand.o
>
> Keep the list at least reasonably sorted.
Ok
>
> > nand-objs := nand_base.o nand_bbt.o nand_timings.o
> > diff --git a/drivers/mtd/nand/arasan_nand.c
> b/drivers/mtd/nand/arasan_nand.c
> > new file mode 100644
> > index 0000000..6b0670e
> > --- /dev/null
> > +++ b/drivers/mtd/nand/arasan_nand.c
> > @@ -0,0 +1,974 @@
> > +/*
> > + * Arasan Nand Flash Controller Driver
>
> NAND
>
> > + * Copyright (C) 2014 - 2015 Xilinx, Inc.
>
> It's 2016 now ...
Sorry :). Yes, I will update.
>
> > + * This program is free software; you can redistribute it and/or modify it
> under
> > + * the terms of the GNU General Public License version 2 as published by
> the
> > + * Free Software Foundation; either version 2 of the License, or (at your
> > + * option) any later version.
> > + */
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io-64-nonatomic-lo-hi.h>
> > +#include <linux/module.h>
> > +#include <linux/mtd/mtd.h>
> > +#include <linux/mtd/nand.h>
> > +#include <linux/mtd/partitions.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +
> > +#define DRIVER_NAME "arasan_nand"
> > +#define EVNT_TIMEOUT 1000
>
> Rename to EVENT_TIMEOUT_<units> to make this less cryptic
Ok.
>
> > +#define STATUS_TIMEOUT 2000
>
> DTTO
Thanks. Just realized that it is unused. I will remove this.
>
> > +#define PKT_OFST 0x00
> > +#define MEM_ADDR1_OFST 0x04
> > +#define MEM_ADDR2_OFST 0x08
> > +#define CMD_OFST 0x0C
> > +#define PROG_OFST 0x10
> > +#define INTR_STS_EN_OFST 0x14
> > +#define INTR_SIG_EN_OFST 0x18
> > +#define INTR_STS_OFST 0x1C
> > +#define READY_STS_OFST 0x20
> > +#define DMA_ADDR1_OFST 0x24
> > +#define FLASH_STS_OFST 0x28
> > +#define DATA_PORT_OFST 0x30
> > +#define ECC_OFST 0x34
> > +#define ECC_ERR_CNT_OFST 0x38
> > +#define ECC_SPR_CMD_OFST 0x3C
> > +#define ECC_ERR_CNT_1BIT_OFST 0x40
> > +#define ECC_ERR_CNT_2BIT_OFST 0x44
> > +#define DMA_ADDR0_OFST 0x50
> > +#define DATA_INTERFACE_REG 0x6C
>
> Why are some things suffixed with _OFST and some with _REG ? Consistency
> please. Using ARASAN_ prefix, ie. #define ARASAN_FOO 0xbar to define
> regs would be nice.
>
Ok. I will maintain the consistency. Since all these definitions for arasan controller
and I feel adding the prefix is no value and some places it will go beyond 80 col limit.
Different reviewers have different opinions here. I am following the above convention.
> > +#define PKT_CNT_SHIFT 12
> > +
> > +#define ECC_ENABLE BIT(31)
> > +#define DMA_EN_MASK GENMASK(27, 26)
> > +#define DMA_ENABLE 0x2
> > +#define DMA_EN_SHIFT 26
> > +#define REG_PAGE_SIZE_MASK GENMASK(25, 23)
> > +#define REG_PAGE_SIZE_SHIFT 23
> > +#define REG_PAGE_SIZE_512 0
> > +#define REG_PAGE_SIZE_1K 5
> > +#define REG_PAGE_SIZE_2K 1
> > +#define REG_PAGE_SIZE_4K 2
> > +#define REG_PAGE_SIZE_8K 3
> > +#define REG_PAGE_SIZE_16K 4
> > +#define CMD2_SHIFT 8
> > +#define ADDR_CYCLES_SHIFT 28
> > +
> > +#define XFER_COMPLETE BIT(2)
> > +#define READ_READY BIT(1)
> > +#define WRITE_READY BIT(0)
> > +#define MBIT_ERROR BIT(3)
> > +#define ERR_INTRPT BIT(4)
> > +
> > +#define PROG_PGRD BIT(0)
> > +#define PROG_ERASE BIT(2)
> > +#define PROG_STATUS BIT(3)
> > +#define PROG_PGPROG BIT(4)
> > +#define PROG_RDID BIT(6)
> > +#define PROG_RDPARAM BIT(7)
> > +#define PROG_RST BIT(8)
> > +#define PROG_GET_FEATURE BIT(9)
> > +#define PROG_SET_FEATURE BIT(10)
> > +
> > +#define ONFI_STATUS_FAIL BIT(0)
> > +#define ONFI_STATUS_READY BIT(6)
> > +
> > +#define PG_ADDR_SHIFT 16
> > +#define BCH_MODE_SHIFT 25
> > +#define BCH_EN_SHIFT 27
> > +#define ECC_SIZE_SHIFT 16
> > +
> > +#define MEM_ADDR_MASK GENMASK(7, 0)
> > +#define BCH_MODE_MASK GENMASK(27, 25)
> > +
> > +#define CS_MASK GENMASK(31, 30)
> > +#define CS_SHIFT 30
> > +
> > +#define PAGE_ERR_CNT_MASK GENMASK(16, 8)
> > +#define PKT_ERR_CNT_MASK GENMASK(7, 0)
> > +
> > +#define NVDDR_MODE BIT(9)
> > +#define NVDDR_TIMING_MODE_SHIFT 3
> > +
> > +#define ONFI_ID_LEN 8
> > +#define TEMP_BUF_SIZE 512
> > +#define NVDDR_MODE_PACKET_SIZE 8
> > +#define SDR_MODE_PACKET_SIZE 4
> > +
> > +#define ONFI_DATA_INTERFACE_NVDDR (1 << 4)
>
> BIT() ?
Sure.
>
>
> [...]
>
> > +struct anfc {
> > + struct nand_hw_control controller;
> > + struct list_head chips;
> > + struct device *dev;
> > + void __iomem *base;
> > + int curr_cmd;
> > + struct clk *clk_sys;
> > + struct clk *clk_flash;
> > + bool dma;
> > + bool err;
> > + bool iswriteoob;
> > + u8 buf[TEMP_BUF_SIZE];
> > + int irq;
> > + u32 bufshift;
> > + int csnum;
> > + struct completion evnt;
>
> event ?
Ok
>
> > +};
>
> [...]
>
> > +static void anfc_prepare_cmd(struct anfc *nfc, u8 cmd1, u8 cmd2, u8
> dmamode,
> > + u32 pagesize, u8 addrcycles)
> > +{
> > + u32 regval;
> > +
> > + regval = cmd1 | (cmd2 << CMD2_SHIFT);
> > + if (dmamode && nfc->dma)
> > + regval |= DMA_ENABLE << DMA_EN_SHIFT;
> > + if (addrcycles)
> > + regval |= addrcycles << ADDR_CYCLES_SHIFT;
> > + if (pagesize)
> > + regval |= anfc_page(pagesize) << REG_PAGE_SIZE_SHIFT;
>
> Drop the if (foo), if it's zero, the regval would be OR'd with zero.
Ok.
>
> > + writel(regval, nfc->base + CMD_OFST);
> > +}
> > +
> > +static int anfc_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
> > + int page)
> > +{
> > + struct anfc *nfc = to_anfc(chip->controller);
> > +
> > + nfc->iswriteoob = true;
> > + chip->cmdfunc(mtd, NAND_CMD_SEQIN, mtd->writesize, page);
> > + chip->write_buf(mtd, chip->oob_poi, mtd->oobsize);
> > + nfc->iswriteoob = false;
> > +
> > + return 0;
> > +}
> > +
> > +static void anfc_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
> > +{
> > + u32 pktcount, pktsize, eccintr = 0;
> > + unsigned int buf_rd_cnt = 0;
> > + u32 *bufptr = (u32 *)buf;
> > + struct nand_chip *chip = mtd_to_nand(mtd);
> > + struct anfc_nand_chip *achip = to_anfc_nand(chip);
> > + struct anfc *nfc = to_anfc(chip->controller);
> > + dma_addr_t paddr;
> > +
> > + if (nfc->curr_cmd == NAND_CMD_READ0) {
> > + pktsize = achip->pktsize;
> > + pktcount = DIV_ROUND_UP(mtd->writesize, pktsize);
> > + if (!achip->bch)
> > + eccintr = MBIT_ERROR;
> > + } else {
> > + pktsize = len;
> > + pktcount = 1;
> > + }
> > +
> > + anfc_setpktszcnt(nfc, pktsize, pktcount);
> > +
> > + if (nfc->dma) {
> > + paddr = dma_map_single(nfc->dev, buf, len,
> DMA_FROM_DEVICE);
> > + if (dma_mapping_error(nfc->dev, paddr)) {
> > + dev_err(nfc->dev, "Read buffer mapping error");
> > + return;
> > + }
> > + lo_hi_writeq(paddr, nfc->base + DMA_ADDR0_OFST);
> > + anfc_enable_intrs(nfc, (XFER_COMPLETE | eccintr));
> > + writel(PROG_PGRD, nfc->base + PROG_OFST);
> > + anfc_wait_for_event(nfc);
> > + dma_unmap_single(nfc->dev, paddr, len,
> DMA_FROM_DEVICE);
>
> Split this function into anfc_read_buf() and then anfc_read_buf_pio()
> and anfc_read_buf_dma() to avoid this ugliness. Also, does this handle
> any errors in any way ? Looks like it ignores all errors, so please fix.
>
Ok. Regarding errors, it handles the errors except checking for contiguous
Address region for the given address. I will add this in next version.
> > + return;
> > + }
> > +
> > + anfc_enable_intrs(nfc, (READ_READY | eccintr));
> > + writel(PROG_PGRD, nfc->base + PROG_OFST);
> > +
> > + while (buf_rd_cnt < pktcount) {
> > + anfc_wait_for_event(nfc);
> > + buf_rd_cnt++;
> > +
> > + if (buf_rd_cnt == pktcount)
> > + anfc_enable_intrs(nfc, XFER_COMPLETE);
> > +
> > + readsl(nfc->base + DATA_PORT_OFST, bufptr, pktsize/4);
> > + bufptr += (pktsize / 4);
> > +
> > + if (buf_rd_cnt < pktcount)
> > + anfc_enable_intrs(nfc, (READ_READY | eccintr));
> > + }
> > +
> > + anfc_wait_for_event(nfc);
> > +}
> > +
> > +static void anfc_write_buf(struct mtd_info *mtd, const uint8_t *buf, int
> len)
> > +{
> > + u32 pktcount, pktsize;
> > + unsigned int buf_wr_cnt = 0;
> > + u32 *bufptr = (u32 *)buf;
> > + struct nand_chip *chip = mtd_to_nand(mtd);
> > + struct anfc_nand_chip *achip = to_anfc_nand(chip);
> > + struct anfc *nfc = to_anfc(chip->controller);
> > + dma_addr_t paddr;
> > +
> > + if (nfc->iswriteoob) {
> > + pktsize = len;
> > + pktcount = 1;
> > + } else {
> > + pktsize = achip->pktsize;
> > + pktcount = mtd->writesize / pktsize;
> > + }
>
> This looks like a copy of the read path. Can these two functions be
> parametrized and merged ?
>
Ok.
> > + anfc_setpktszcnt(nfc, pktsize, pktcount);
> > +
> > + if (nfc->dma) {
> > + paddr = dma_map_single(nfc->dev, (void *)buf, len,
> > + DMA_TO_DEVICE);
> > + if (dma_mapping_error(nfc->dev, paddr)) {
> > + dev_err(nfc->dev, "Write buffer mapping error");
> > + return;
> > + }
> > + lo_hi_writeq(paddr, nfc->base + DMA_ADDR0_OFST);
> > + anfc_enable_intrs(nfc, XFER_COMPLETE);
> > + writel(PROG_PGPROG, nfc->base + PROG_OFST);
> > + anfc_wait_for_event(nfc);
> > + dma_unmap_single(nfc->dev, paddr, len, DMA_TO_DEVICE);
> > + return;
> > + }
> > +
> > + anfc_enable_intrs(nfc, WRITE_READY);
> > + writel(PROG_PGPROG, nfc->base + PROG_OFST);
> > +
> > + while (buf_wr_cnt < pktcount) {
> > + anfc_wait_for_event(nfc);
> > + buf_wr_cnt++;
> > + if (buf_wr_cnt == pktcount)
> > + anfc_enable_intrs(nfc, XFER_COMPLETE);
> > +
> > + writesl(nfc->base + DATA_PORT_OFST, bufptr, pktsize/4);
> > + bufptr += (pktsize / 4);
> > +
> > + if (buf_wr_cnt < pktcount)
> > + anfc_enable_intrs(nfc, WRITE_READY);
> > + }
> > +
> > + anfc_wait_for_event(nfc);
> > +}
>
>
> [...]
>
> > +static void anfc_writefifo(struct anfc *nfc, u32 prog, u32 size, u8 *buf)
> > +{
> > + u32 *bufptr = (u32 *)buf;
> > +
> > + anfc_enable_intrs(nfc, WRITE_READY);
> > +
> > + writel(prog, nfc->base + PROG_OFST);
> > + anfc_wait_for_event(nfc);
> > +
> > + anfc_enable_intrs(nfc, XFER_COMPLETE);
> > + writesl(nfc->base + DATA_PORT_OFST, bufptr, size/4);
>
> use ioread32_rep and iowrite32_rep , otherwise this won't compile on x86
> with COMPILE_TEST.
>
Ok. I have just got the kbuild notification for x86 system. I will fix this.
> > + anfc_wait_for_event(nfc);
> > +}
> > +
> > +static void anfc_readfifo(struct anfc *nfc, u32 prog, u32 size)
> > +{
> > + u32 *bufptr = (u32 *)nfc->buf;
> > +
> > + anfc_enable_intrs(nfc, READ_READY);
> > +
> > + writel(prog, nfc->base + PROG_OFST);
> > + anfc_wait_for_event(nfc);
> > +
> > + anfc_enable_intrs(nfc, XFER_COMPLETE);
> > + readsl(nfc->base + DATA_PORT_OFST, bufptr, size/4);
>
> See above
>
> > + anfc_wait_for_event(nfc);
> > +}
>
>
> [...]
>
> > +static void anfc_select_chip(struct mtd_info *mtd, int num)
> > +{
> > + u32 val;
> > + struct nand_chip *chip = mtd_to_nand(mtd);
> > + struct anfc_nand_chip *achip = to_anfc_nand(chip);
> > + struct anfc *nfc = to_anfc(chip->controller);
> > +
> > + if (num == -1)
> > + return;
> > +
> > + val = readl(nfc->base + MEM_ADDR2_OFST);
> > + val = (val & ~(CS_MASK)) | (achip->csnum << CS_SHIFT);
> > + val = (val & ~(BCH_MODE_MASK)) | (achip->bchmode <<
> BCH_MODE_SHIFT);
>
> Just rewrite this as a series of val &= ~(foo | bar); val |= baz | quux;
> for clarity sake.
>
Ok.
> > + writel(val, nfc->base + MEM_ADDR2_OFST);
> > + nfc->csnum = achip->csnum;
> > + writel(achip->eccval, nfc->base + ECC_OFST);
> > + writel(achip->inftimeval, nfc->base + DATA_INTERFACE_REG);
> > +}
> > +
> > +static irqreturn_t anfc_irq_handler(int irq, void *ptr)
> > +{
> > + struct anfc *nfc = ptr;
> > + u32 regval = 0, status;
> > +
> > + status = readl(nfc->base + INTR_STS_OFST);
> > + if (status & XFER_COMPLETE) {
> > + complete(&nfc->evnt);
> > + regval |= XFER_COMPLETE;
>
> Can the complete() be invoked multiple times ? That seems a bit odd.
>
I will check and fix this.
> > + }
> > +
> > + if (status & READ_READY) {
> > + complete(&nfc->evnt);
> > + regval |= READ_READY;
> > + }
> > +
> > + if (status & WRITE_READY) {
> > + complete(&nfc->evnt);
> > + regval |= WRITE_READY;
> > + }
> > +
> > + if (status & MBIT_ERROR) {
> > + nfc->err = true;
> > + complete(&nfc->evnt);
> > + regval |= MBIT_ERROR;
> > + }
> > +
> > + if (regval) {
> > + writel(regval, nfc->base + INTR_STS_OFST);
> > + writel(0, nfc->base + INTR_STS_EN_OFST);
> > + writel(0, nfc->base + INTR_SIG_EN_OFST);
> > +
> > + return IRQ_HANDLED;
> > + }
> > +
> > + return IRQ_NONE;
> > +}
> > +
> > +static int anfc_onfi_set_features(struct mtd_info *mtd, struct nand_chip
> *chip,
> > + int addr, uint8_t *subfeature_param)
> > +{
> > + struct anfc *nfc = to_anfc(chip->controller);
> > + struct anfc_nand_chip *achip = to_anfc_nand(chip);
> > + int status;
> > +
> > + if (!chip->onfi_version || !(le16_to_cpu(chip-
> >onfi_params.opt_cmd)
> > + & ONFI_OPT_CMD_SET_GET_FEATURES))
>
> Split this into two conditions to improve readability.
>
Ok.
> > + return -EINVAL;
> > +
> > + chip->cmdfunc(mtd, NAND_CMD_SET_FEATURES, addr, -1);
> > + anfc_writefifo(nfc, PROG_SET_FEATURE, achip->spktsize,
> > + subfeature_param);
> > +
> > + status = chip->waitfunc(mtd, chip);
> > + if (status & NAND_STATUS_FAIL)
> > + return -EIO;
> > +
> > + return 0;
> > +}
> > +
> > +static int anfc_init_timing_mode(struct anfc *nfc,
> > + struct anfc_nand_chip *achip)
> > +{
> > + int mode, err;
> > + unsigned int feature[2];
> > + u32 inftimeval;
> > + struct nand_chip *chip = &achip->chip;
> > + struct mtd_info *mtd = nand_to_mtd(chip);
> > +
> > + memset(feature, 0, NVDDR_MODE_PACKET_SIZE);
> > + /* Get nvddr timing modes */
> > + mode = onfi_get_sync_timing_mode(chip) & 0xff;
> > + if (!mode) {
> > + mode = fls(onfi_get_async_timing_mode(chip)) - 1;
> > + inftimeval = mode;
> > + } else {
> > + mode = fls(mode) - 1;
> > + inftimeval = NVDDR_MODE | (mode <<
> NVDDR_TIMING_MODE_SHIFT);
> > + mode |= ONFI_DATA_INTERFACE_NVDDR;
> > + }
> > +
> > + feature[0] = mode;
> > + chip->select_chip(mtd, achip->csnum);
> > + err = chip->onfi_set_features(mtd, chip,
> ONFI_FEATURE_ADDR_TIMING_MODE,
> > + (uint8_t *)feature);
> > + chip->select_chip(mtd, -1);
> > + if (err)
> > + return err;
> > +
> > + achip->inftimeval = inftimeval;
> > +
> > + if (mode & ONFI_DATA_INTERFACE_NVDDR)
> > + achip->spktsize = NVDDR_MODE_PACKET_SIZE;
> > +
> > + return 0;
> > +}
>
> [...]
>
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Xilinx, Inc");
>
> There should be a contact with email address here.
>
I think there is an alias for Xilinx, Inc in maintainers list.
If not I will update it.
Regards,
Punnaiah
> > +MODULE_DESCRIPTION("Arasan NAND Flash Controller Driver");
> >
>
>
> --
> Best regards,
> Marek Vasut
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v6 1/2] mtd: arasan: Add device tree binding documentation
2016-12-05 8:36 ` Boris Brezillon
@ 2016-12-05 13:56 ` Punnaiah Choudary Kalluri
2016-12-05 16:11 ` Marek Vasut
1 sibling, 0 replies; 10+ messages in thread
From: Punnaiah Choudary Kalluri @ 2016-12-05 13:56 UTC (permalink / raw)
To: Boris Brezillon, Marek Vasut
Cc: dwmw2, computersforpeace, richard, cyrille.pitchen, robh+dt,
mark.rutland, linux-kernel, linux-mtd, devicetree, Michal Simek,
kalluripunnaiahchoudary, kpc528
> -----Original Message-----
> From: Boris Brezillon [mailto:boris.brezillon@free-electrons.com]
> Sent: Monday, December 05, 2016 2:07 PM
> To: Marek Vasut <marek.vasut@gmail.com>
> Cc: Punnaiah Choudary Kalluri <punnaia@xilinx.com>;
> dwmw2@infradead.org; computersforpeace@gmail.com; richard@nod.at;
> cyrille.pitchen@atmel.com; robh+dt@kernel.org; mark.rutland@arm.com;
> linux-kernel@vger.kernel.org; linux-mtd@lists.infradead.org;
> devicetree@vger.kernel.org; Michal Simek <michals@xilinx.com>;
> kalluripunnaiahchoudary@gmail.com; kpc528@gmail.com; Punnaiah
> Choudary Kalluri <punnaia@xilinx.com>
> Subject: Re: [PATCH v6 1/2] mtd: arasan: Add device tree binding
> documentation
>
> On Mon, 5 Dec 2016 05:25:54 +0100
> Marek Vasut <marek.vasut@gmail.com> wrote:
>
> > On 12/05/2016 05:11 AM, Punnaiah Choudary Kalluri wrote:
> > > This patch adds the dts binding document for arasan nand flash
> > > controller.
> > >
> > > Signed-off-by: Punnaiah Choudary Kalluri <punnaia@xilinx.com>
> > > Acked-by: Rob Herring <robh@kernel.org>
> > > ---
> > > changes in v6:
> > > - Removed num-cs property
> > > - Separated nandchip from nand controller changes in v5:
> > > - None
> > > Changes in v4:
> > > - Added num-cs property
> > > - Added clock support
> > > Changes in v3:
> > > - None
> > > Changes in v2:
> > > - None
> > > ---
> > > .../devicetree/bindings/mtd/arasan_nfc.txt | 38
> ++++++++++++++++++++++
> > > 1 file changed, 38 insertions(+)
> > > create mode 100644
> > > Documentation/devicetree/bindings/mtd/arasan_nfc.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/mtd/arasan_nfc.txt
> > > b/Documentation/devicetree/bindings/mtd/arasan_nfc.txt
> > > new file mode 100644
> > > index 0000000..dcbe7ad
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mtd/arasan_nfc.txt
> > > @@ -0,0 +1,38 @@
> > > +Arasan Nand Flash Controller with ONFI 3.1 support
> >
> > Arasan NAND Flash ...
> >
> > > +Required properties:
> > > +- compatible: Should be "arasan,nfc-v3p10"
> >
> > This v3p10 looks like version 3 patchlevel 10, but shouldn't we have
> > some fallback option which doesn't encode IP version in the compat
> > string ?
>
> Not necessarily. Usually you define a generic compatible when you have
> other reliable means to detect the IP version (a version register for
> example).
> If you can't detect that at runtime, then providing only specific compatible
> strings is a good solution to avoid breaking the DT ABI.
Yes. I am also thinking the same. Arasan controller doesn't have the register
to indicate the IP version number.
>
> >
> > Also, shouldn't quirks be handled by DT props instead of effectively
> > encoding them into the compatible string ?
>
> Well, from my experience, it's better to hide as much as possible behind the
> compatible. This way, if new quirks are needed for a specific revision, you can
> update the driver without having to change the DT.
>
Agree.
Regards,
Punnaiah
> >
> > > +- reg: Memory map for module access
> > > +- interrupt-parent: Interrupt controller the interrupt is routed
> > > +through
> > > +- interrupts: Should contain the interrupt for the device
> > > +- clock-name: List of input clocks - "clk_sys", "clk_flash"
> > > + (See clock bindings for details)
> > > +- clocks: Clock phandles (see clock bindings for details)
> > > +
> > > +Optional properties:
> > > +- arasan,has-mdma: Enables Dma support
> >
> > 'Enables DMA support' , with DMA in caps.
> >
> > > +for nand partition information please refer the below file
> >
> > For NAND ...
> >
> > > +Documentation/devicetree/bindings/mtd/partition.txt
> > > +
> > > +Example:
> > > + nand0: nand@ff100000 {
> > > + compatible = "arasan,nfc-v3p10"
> > > + reg = <0x0 0xff100000 0x1000>;
> > > + clock-name = "clk_sys", "clk_flash"
> > > + clocks = <&misc_clk &misc_clk>;
> > > + interrupt-parent = <&gic>;
> > > + interrupts = <0 14 4>;
> > > + arasan,has-mdma;
> > > + #address-cells = <1>;
> > > + #size-cells = <0>
> > > +
> > > + nand@0 {
> > > + reg = <0>
> > > + partition@0 {
> > > + label = "filesystem";
> > > + reg = <0x0 0x0 0x1000000>;
> > > + };
> > > + (...)
> > > + };
> > > + };
> > >
> >
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v6 1/2] mtd: arasan: Add device tree binding documentation
2016-12-05 8:36 ` Boris Brezillon
2016-12-05 13:56 ` Punnaiah Choudary Kalluri
@ 2016-12-05 16:11 ` Marek Vasut
1 sibling, 0 replies; 10+ messages in thread
From: Marek Vasut @ 2016-12-05 16:11 UTC (permalink / raw)
To: Boris Brezillon
Cc: Punnaiah Choudary Kalluri, dwmw2, computersforpeace, richard,
cyrille.pitchen, robh+dt, mark.rutland, linux-kernel, linux-mtd,
devicetree, michals, kalluripunnaiahchoudary, kpc528,
Punnaiah Choudary Kalluri
On 12/05/2016 09:36 AM, Boris Brezillon wrote:
> On Mon, 5 Dec 2016 05:25:54 +0100
> Marek Vasut <marek.vasut@gmail.com> wrote:
>
>> On 12/05/2016 05:11 AM, Punnaiah Choudary Kalluri wrote:
>>> This patch adds the dts binding document for arasan nand flash
>>> controller.
>>>
>>> Signed-off-by: Punnaiah Choudary Kalluri <punnaia@xilinx.com>
>>> Acked-by: Rob Herring <robh@kernel.org>
>>> ---
>>> changes in v6:
>>> - Removed num-cs property
>>> - Separated nandchip from nand controller
>>> changes in v5:
>>> - None
>>> Changes in v4:
>>> - Added num-cs property
>>> - Added clock support
>>> Changes in v3:
>>> - None
>>> Changes in v2:
>>> - None
>>> ---
>>> .../devicetree/bindings/mtd/arasan_nfc.txt | 38 ++++++++++++++++++++++
>>> 1 file changed, 38 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/mtd/arasan_nfc.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/mtd/arasan_nfc.txt b/Documentation/devicetree/bindings/mtd/arasan_nfc.txt
>>> new file mode 100644
>>> index 0000000..dcbe7ad
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/mtd/arasan_nfc.txt
>>> @@ -0,0 +1,38 @@
>>> +Arasan Nand Flash Controller with ONFI 3.1 support
>>
>> Arasan NAND Flash ...
>>
>>> +Required properties:
>>> +- compatible: Should be "arasan,nfc-v3p10"
>>
>> This v3p10 looks like version 3 patchlevel 10, but shouldn't we have
>> some fallback option which doesn't encode IP version in the compat
>> string ?
>
> Not necessarily. Usually you define a generic compatible when you have
> other reliable means to detect the IP version (a version register for
> example).
> If you can't detect that at runtime, then providing only specific
> compatible strings is a good solution to avoid breaking the DT ABI.
Can we detect it at runtime with this hardware ?
>>
>> Also, shouldn't quirks be handled by DT props instead of effectively
>> encoding them into the compatible string ?
>
> Well, from my experience, it's better to hide as much as possible
> behind the compatible. This way, if new quirks are needed for a
> specific revision, you can update the driver without having to change
> the DT.
That's a good point, yes.
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-12-05 16:16 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-05 4:11 [PATCH v6 1/2] mtd: arasan: Add device tree binding documentation Punnaiah Choudary Kalluri
2016-12-05 4:11 ` [PATCH v6 2/2] mtd: nand: Add support for Arasan Nand Flash Controller Punnaiah Choudary Kalluri
2016-12-05 4:40 ` Marek Vasut
2016-12-05 13:55 ` Punnaiah Choudary Kalluri
2016-12-05 9:16 ` kbuild test robot
2016-12-05 4:25 ` [PATCH v6 1/2] mtd: arasan: Add device tree binding documentation Marek Vasut
2016-12-05 8:36 ` Boris Brezillon
2016-12-05 13:56 ` Punnaiah Choudary Kalluri
2016-12-05 16:11 ` Marek Vasut
2016-12-05 13:24 ` Punnaiah Choudary Kalluri
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).