Signed-off-by: Alexey Brodkin <[email protected]>
Reviewed-by: Ezequiel Garcia <[email protected]>
Cc: Ezequiel Garcia <[email protected]>
Cc: Vineet Gupta <[email protected]>
cc: Brian Norris <[email protected]>
Cc: Grant Likely <[email protected]>
Cc: David Woodhouse <[email protected]>
Cc: Francois Bedard <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
In this re-spin I addressed Ezequiel comments.
Changes compared to v1:
* Minor code clean-up
* Fixed typos
* axs_flag_is_set() replaced with axs_flag_wait_and_reset()
* axs_flag_wait_and_reset() has built-in check for timeout instead of endless
while() loop initially used
Documentation/devicetree/bindings/mtd/axs-nand.txt | 17 +
drivers/mtd/nand/Kconfig | 6 +
drivers/mtd/nand/Makefile | 1 +
drivers/mtd/nand/axs_nand.c | 411 +++++++++++++++++++++
4 files changed, 435 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mtd/axs-nand.txt
create mode 100644 drivers/mtd/nand/axs_nand.c
diff --git a/Documentation/devicetree/bindings/mtd/axs-nand.txt b/Documentation/devicetree/bindings/mtd/axs-nand.txt
new file mode 100644
index 0000000..c522b7f
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/axs-nand.txt
@@ -0,0 +1,17 @@
+* Synopsys AXS NAND controller
+
+Required properties:
+ - compatible: must be "snps,axs-nand"
+ - reg: physical base address and size of the registers map
+
+The device tree may optionally contain sub-nodes describing partitions of the
+address space. See partition.txt for more detail.
+
+Examples:
+
+flash@0x16000 {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ compatible = "snps,axs-nand";
+ reg = < 0x16000 0x200 >;
+};
diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 93ae6a6..3661822 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -510,4 +510,10 @@ 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_AXS
+ tristate "Support for NAND on Synopsys AXS development board"
+ help
+ Enables support for NAND Flash chips on Synopsys AXS development
+ boards.
+
endif # MTD_NAND
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index 542b568..635a918 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -49,5 +49,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_AXS) += axs_nand.o
nand-objs := nand_base.o nand_bbt.o
diff --git a/drivers/mtd/nand/axs_nand.c b/drivers/mtd/nand/axs_nand.c
new file mode 100644
index 0000000..9ee21b6
--- /dev/null
+++ b/drivers/mtd/nand/axs_nand.c
@@ -0,0 +1,411 @@
+/*
+ * Copyright (C) 2014 Synopsys, Inc. (http://www.synopsys.com)
+ *
+ * Driver for NAND controller on Synopsys AXS development board.
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License v2. See the file COPYING in the main directory of this archive for
+ * more details.
+ */
+
+#include <linux/delay.h>
+#include <linux/dma-mapping.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/nand.h>
+#include <linux/mtd/partitions.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/io.h>
+
+/*
+ * There's an issue with DMA'd data if data buffer is cached.
+ * So to make NAND storage available for now we'll map data buffer in
+ * uncached memory.
+ *
+ * As soon as issue with cached buffer is resolved following define to be
+ * removed as well as sources it enables.
+ */
+#define DATA_BUFFER_UNCACHED
+
+#define BUS_WIDTH 8 /* AXI data bus width in bytes */
+
+/* DMA buffer descriptor masks */
+#define BD_STAT_OWN (1 << 31)
+#define BD_STAT_BD_FIRST (1 << 3)
+#define BD_STAT_BD_LAST (1 << 2)
+#define BD_SIZES_BUFFER1_MASK 0xfff
+
+#define BD_STAT_BD_COMPLETE (BD_STAT_BD_FIRST | BD_STAT_BD_LAST)
+
+/* Controller command types */
+#define B_CT_ADDRESS (0x0 << 16) /* Address operation */
+#define B_CT_COMMAND (0x1 << 16) /* Command operation */
+#define B_CT_WRITE (0x2 << 16) /* Write operation */
+#define B_CT_READ (0x3 << 16) /* Read operation */
+
+/* Controller command options */
+#define B_WFR (1 << 19) /* 1b - Wait for ready */
+#define B_LC (1 << 18) /* 1b - Last cycle */
+#define B_IWC (1 << 13) /* 1b - Interrupt when complete */
+
+enum {
+ NAND_ISR_DATAREQUIRED = 0,
+ NAND_ISR_TXUNDERFLOW,
+ NAND_ISR_TXOVERFLOW,
+ NAND_ISR_DATAAVAILABLE,
+ NAND_ISR_RXUNDERFLOW,
+ NAND_ISR_RXOVERFLOW,
+ NAND_ISR_TXDMACOMPLETE,
+ NAND_ISR_RXDMACOMPLETE,
+ NAND_ISR_DESCRIPTORUNAVAILABLE,
+ NAND_ISR_CMDDONE,
+ NAND_ISR_CMDAVAILABLE,
+ NAND_ISR_CMDERROR,
+ NAND_ISR_DATATRANSFEROVER,
+ NAND_ISR_NONE
+};
+
+enum {
+ AC_FIFO = 0, /* Address and command fifo */
+ IDMAC_BDADDR = 0x18, /* IDMAC descriptor list base address */
+ INT_STATUS = 0x118, /* Interrupt status register */
+ INT_CLR_STATUS = 0x120 /* Interrupt clear status register */
+};
+
+#define AXS_BUF_SIZE (PAGE_ALIGN(NAND_MAX_PAGESIZE + NAND_MAX_OOBSIZE))
+
+struct asx_nand_bd {
+ __le32 status; /* DES0 */
+ __le32 sizes; /* DES1 */
+ dma_addr_t buffer_ptr0; /* DES2 */
+ dma_addr_t buffer_ptr1; /* DES3 */
+};
+
+struct axs_nand_host {
+ struct nand_chip nand_chip;
+ struct mtd_info mtd;
+ void __iomem *io_base;
+ struct device *dev;
+ struct asx_nand_bd *bd; /* Buffer descriptor */
+ dma_addr_t bd_dma; /* DMA handle for buffer descriptor */
+ uint8_t *db; /* Data buffer */
+ dma_addr_t db_dma; /* DMA handle for data buffer */
+};
+
+/**
+ * reg_set - Sets register with provided value.
+ * @host: Pointer to private data structure.
+ * @reg: Register offset from base address.
+ * @value: Value to set in register.
+ */
+static inline void reg_set(struct axs_nand_host *host, int reg, int value)
+{
+ iowrite32(value, host->io_base + reg);
+}
+
+/**
+ * reg_get - Gets value of specified register.
+ * @host: Pointer to private data structure.
+ * @reg: Register offset from base address.
+ *
+ * returns: Value of requested register.
+ */
+static inline unsigned int reg_get(struct axs_nand_host *host, int reg)
+{
+ return ioread32(host->io_base + reg);
+}
+
+/* Maximum number of milliseconds we wait for flag to appear */
+#define AXS_FLAG_WAIT_DELAY 1000
+
+/**
+ * axs_flag_wait_and_reset - Waits until requested flag in INT_STATUS register
+ * is set by HW and resets it by writing "1" in INT_CLR_STATUS.
+ * @host: Pointer to private data structure.
+ * @flag: Bit/flag offset in INT_STATUS register
+ */
+static void axs_flag_wait_and_reset(struct axs_nand_host *host, int flag)
+{
+ unsigned int i;
+
+ for (i = 0; i < AXS_FLAG_WAIT_DELAY * 100; i++) {
+ unsigned int status = reg_get(host, INT_STATUS);
+
+ if (status & (1 << flag)) {
+ reg_set(host, INT_CLR_STATUS, 1 << flag);
+ return;
+ }
+
+ udelay(10);
+ }
+
+ /*
+ * Since we cannot report this problem any further than
+ * axs_nand_{write|read}_buf() letting user know there's a problem.
+ */
+ dev_err(host->dev, "Waited too long (%d s.) for flag/bit %d\n",
+ AXS_FLAG_WAIT_DELAY, flag);
+}
+
+/**
+ * axs_nand_write_buf - write buffer to chip
+ * @mtd: MTD device structure
+ * @buf: Data buffer
+ * @len: Number of bytes to write
+ */
+static void axs_nand_write_buf(struct mtd_info *mtd,
+ const uint8_t *buf, int len)
+{
+ struct nand_chip *this = mtd->priv;
+ struct axs_nand_host *host = this->priv;
+
+ memcpy(host->db, buf, len);
+#ifndef DATA_BUFFER_UNCACHED
+ dma_sync_single_for_device(host->dev, host->db_dma, len, DMA_TO_DEVICE);
+#endif
+
+ /* Setup buffer descriptor */
+ host->bd->status = BD_STAT_OWN | BD_STAT_BD_COMPLETE;
+ host->bd->sizes = cpu_to_le32(ALIGN(len, BUS_WIDTH) &
+ BD_SIZES_BUFFER1_MASK);
+ host->bd->buffer_ptr0 = cpu_to_le32(host->db_dma);
+ host->bd->buffer_ptr1 = 0;
+
+ /* Issue "write" command */
+ reg_set(host, AC_FIFO, B_CT_WRITE | B_WFR | B_IWC | B_LC | (len - 1));
+
+ /* Wait for NAND command and DMA to complete */
+ axs_flag_wait_and_reset(host, NAND_ISR_CMDDONE);
+ axs_flag_wait_and_reset(host, NAND_ISR_TXDMACOMPLETE);
+}
+
+/**
+ * axs_nand_read_buf - read chip data into buffer
+ * @mtd: MTD device structure
+ * @buf: Buffer to store data
+ * @len: Number of bytes to read
+ */
+static void axs_nand_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
+{
+ struct nand_chip *this = mtd->priv;
+ struct axs_nand_host *host = this->priv;
+
+ /* Setup buffer descriptor */
+ host->bd->status = BD_STAT_OWN | BD_STAT_BD_COMPLETE;
+ host->bd->sizes = cpu_to_le32(ALIGN(len, BUS_WIDTH) &
+ BD_SIZES_BUFFER1_MASK);
+ host->bd->buffer_ptr0 = cpu_to_le32(host->db_dma);
+ host->bd->buffer_ptr1 = 0;
+
+ /* Issue "read" command */
+ reg_set(host, AC_FIFO, B_CT_READ | B_WFR | B_IWC | B_LC | (len - 1));
+
+ /* Wait for NAND command and DMA to complete */
+ axs_flag_wait_and_reset(host, NAND_ISR_CMDDONE);
+ axs_flag_wait_and_reset(host, NAND_ISR_RXDMACOMPLETE);
+
+#ifndef DATA_BUFFER_UNCACHED
+ dma_sync_single_for_cpu(host->dev, host->db_dma, len, DMA_FROM_DEVICE);
+#endif
+ if (buf != host->db)
+ memcpy(buf, host->db, len);
+}
+
+/**
+ * axs_nand_read_byte - read one byte from the chip
+ * @mtd: MTD device structure
+ *
+ * returns: read data byte
+ */
+static uint8_t axs_nand_read_byte(struct mtd_info *mtd)
+{
+ struct nand_chip *this = mtd->priv;
+ struct axs_nand_host *host = this->priv;
+
+ axs_nand_read_buf(mtd, host->db, sizeof(uint8_t));
+ return (uint8_t)host->db[0];
+}
+
+/**
+ * axs_nand_read_word - read one word from the chip
+ * @mtd: MTD device structure
+ *
+ * returns: read data word
+ */
+static uint16_t axs_nand_read_word(struct mtd_info *mtd)
+{
+ struct nand_chip *this = mtd->priv;
+ struct axs_nand_host *host = this->priv;
+
+ axs_nand_read_buf(mtd, host->db, sizeof(uint16_t));
+ return (uint16_t)host->db[0];
+}
+
+/**
+ * axs_nand_cmd_ctrl - hardware specific access to control-lines
+ * @mtd: MTD device structure
+ * @cmd: NAND command
+ * @ctrl: NAND control options
+ */
+static void axs_nand_cmd_ctrl(struct mtd_info *mtd, int cmd,
+ unsigned int ctrl)
+{
+ struct nand_chip *nand_chip = mtd->priv;
+ struct axs_nand_host *host = nand_chip->priv;
+
+ if (cmd == NAND_CMD_NONE)
+ return;
+
+ cmd = cmd & 0xff;
+
+ switch (ctrl & (NAND_ALE | NAND_CLE)) {
+ /* Address */
+ case NAND_ALE:
+ cmd |= B_CT_ADDRESS;
+ break;
+
+ /* Command */
+ case NAND_CLE:
+ cmd |= B_CT_COMMAND | B_WFR;
+
+ break;
+
+ default:
+ dev_err(host->dev, "Unknown ctrl %#x\n", ctrl);
+ return;
+ }
+
+ reg_set(host, AC_FIFO, cmd | B_LC);
+ axs_flag_wait_and_reset(host, NAND_ISR_CMDDONE);
+}
+
+static int axs_nand_probe(struct platform_device *pdev)
+{
+ struct mtd_part_parser_data ppdata;
+ struct nand_chip *nand_chip;
+ struct axs_nand_host *host;
+ struct resource res_regs;
+ struct mtd_info *mtd;
+ int err;
+
+ if (!pdev->dev.of_node)
+ return -ENODEV;
+
+ /* Get registers base address from device tree */
+ err = of_address_to_resource(pdev->dev.of_node, 0, &res_regs);
+ if (err) {
+ dev_err(&pdev->dev, "Failed to retrieve registers base from device tree\n");
+ return -ENODEV;
+ }
+
+ /* Allocate memory for the device structure (and zero it) */
+ host = kzalloc(sizeof(struct axs_nand_host), GFP_KERNEL);
+ if (!host)
+ return -ENOMEM;
+
+ host->io_base = devm_ioremap_resource(&pdev->dev, &res_regs);
+ if (IS_ERR(host->io_base)) {
+ err = PTR_ERR(host->io_base);
+ goto out;
+ }
+ dev_dbg(&pdev->dev, "Registers base address is 0x%p\n", host->io_base);
+
+ host->bd = dmam_alloc_coherent(&pdev->dev, sizeof(struct asx_nand_bd),
+ &host->bd_dma, GFP_KERNEL);
+ if (!host->bd) {
+ dev_err(&pdev->dev, "Failed to allocate buffer descriptor\n");
+ err = -ENOMEM;
+ goto out;
+ }
+
+ memset(host->bd, 0, sizeof(struct asx_nand_bd));
+
+#ifdef DATA_BUFFER_UNCACHED
+ host->db = dmam_alloc_coherent(&pdev->dev, AXS_BUF_SIZE,
+#else
+ host->db = dmam_alloc_noncoherent(&pdev->dev, AXS_BUF_SIZE,
+#endif
+ &host->db_dma, GFP_KERNEL);
+ if (!host->db) {
+ dev_err(&pdev->dev, "Failed to allocate data buffer\n");
+ err = -ENOMEM;
+ goto out;
+ }
+ dev_dbg(&pdev->dev, "Data buffer mapped @ %p, DMA @ %x\n", host->db,
+ host->db_dma);
+
+ mtd = &host->mtd;
+ nand_chip = &host->nand_chip;
+ host->dev = &pdev->dev;
+
+ nand_chip->priv = host;
+ mtd->priv = nand_chip;
+ mtd->name = "axs_nand";
+ mtd->owner = THIS_MODULE;
+ mtd->dev.parent = &pdev->dev;
+ ppdata.of_node = pdev->dev.of_node;
+
+ nand_chip->cmd_ctrl = axs_nand_cmd_ctrl;
+ nand_chip->read_byte = axs_nand_read_byte;
+ nand_chip->read_word = axs_nand_read_word;
+ nand_chip->write_buf = axs_nand_write_buf;
+ nand_chip->read_buf = axs_nand_read_buf;
+ nand_chip->ecc.mode = NAND_ECC_SOFT;
+
+ dev_set_drvdata(&pdev->dev, host);
+
+ reg_set(host, IDMAC_BDADDR, host->bd_dma);
+
+ err = nand_scan(mtd, 1);
+ if (err)
+ goto out1;
+
+ err = mtd_device_parse_register(mtd, NULL, &ppdata, NULL, 0);
+ if (!err)
+ return err;
+
+ nand_release(mtd);
+
+out1:
+ dev_set_drvdata(&pdev->dev, NULL);
+out:
+ kfree(host);
+ return err;
+}
+
+static int axs_nand_remove(struct platform_device *ofdev)
+{
+ struct axs_nand_host *host = dev_get_drvdata(&ofdev->dev);
+ struct mtd_info *mtd = &host->mtd;
+
+ nand_release(mtd);
+ dev_set_drvdata(&ofdev->dev, NULL);
+ kfree(host);
+
+ return 0;
+}
+
+static const struct of_device_id axs_nand_dt_ids[] = {
+ { .compatible = "snps,axs-nand", },
+ { /* Sentinel */ }
+};
+
+MODULE_DEVICE_TABLE(of, axs_nand_match);
+
+static struct platform_driver axs_nand_driver = {
+ .probe = axs_nand_probe,
+ .remove = axs_nand_remove,
+ .driver = {
+ .name = "asx-nand",
+ .owner = THIS_MODULE,
+ .of_match_table = axs_nand_dt_ids,
+ },
+};
+
+module_platform_driver(axs_nand_driver);
+
+MODULE_AUTHOR("Alexey Brodkin <[email protected]>");
+MODULE_DESCRIPTION("NAND driver for Synopsys AXS development board");
+MODULE_LICENSE("GPL");
--
1.9.0
On Apr 04, Alexey Brodkin wrote:
> Signed-off-by: Alexey Brodkin <[email protected]>
>
Maybe it would be nice adding some driver description here, so the commit
log actually says something useful about the commit.
[..]
> Reviewed-by: Ezequiel Garcia <[email protected]>
>
> +/**
> + * axs_flag_wait_and_reset - Waits until requested flag in INT_STATUS register
> + * is set by HW and resets it by writing "1" in INT_CLR_STATUS.
> + * @host: Pointer to private data structure.
> + * @flag: Bit/flag offset in INT_STATUS register
> + */
> +static void axs_flag_wait_and_reset(struct axs_nand_host *host, int flag)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < AXS_FLAG_WAIT_DELAY * 100; i++) {
> + unsigned int status = reg_get(host, INT_STATUS);
> +
> + if (status & (1 << flag)) {
> + reg_set(host, INT_CLR_STATUS, 1 << flag);
> + return;
> + }
> +
> + udelay(10);
> + }
> +
> + /*
> + * Since we cannot report this problem any further than
> + * axs_nand_{write|read}_buf() letting user know there's a problem.
> + */
> + dev_err(host->dev, "Waited too long (%d s.) for flag/bit %d\n",
> + AXS_FLAG_WAIT_DELAY, flag);
> +}
Hm... I'm not sure the above is really true.
The NAND core uses the replaceable chip->waitfunc callback to check the
status of issued commands. See for instance:
static int nand_write_oob_std(struct mtd_info *mtd, struct nand_chip *chip,
int page)
{
int status = 0;
const uint8_t *buf = chip->oob_poi;
int length = mtd->oobsize;
chip->cmdfunc(mtd, NAND_CMD_SEQIN, mtd->writesize, page);
chip->write_buf(mtd, buf, length);
/* Send command to program the OOB data */
chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
status = chip->waitfunc(mtd, chip);
return status & NAND_STATUS_FAIL ? -EIO : 0;
}
On the other side, if you are clearing the flags in axs_flag_wait_and_reset()
it might be a bit hard for you to get this right.
IOW, I'm not saying you *must* do this, but instead suggesting that you take
a look at waitfunc() and see if it helps report a proper error in the
read/write path.
--
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
Hi Ezequiel,
On Fri, 2014-04-04 at 11:09 -0300, Ezequiel Garcia wrote:
> On Apr 04, Alexey Brodkin wrote:
> > Signed-off-by: Alexey Brodkin <[email protected]>
> >
>
> Maybe it would be nice adding some driver description here, so the commit
> log actually says something useful about the commit.
Well, I thought about it but frankly I'm not sure if there's anything
else to add to commit title "driver for NAND controller used on Synopsys
AXS dev boards".
Do you think more info may make sense?
> > +/**
> > + * axs_flag_wait_and_reset - Waits until requested flag in INT_STATUS register
> > + * is set by HW and resets it by writing "1" in INT_CLR_STATUS.
> > + * @host: Pointer to private data structure.
> > + * @flag: Bit/flag offset in INT_STATUS register
> > + */
> > +static void axs_flag_wait_and_reset(struct axs_nand_host *host, int flag)
> > +{
> > + unsigned int i;
> > +
> > + for (i = 0; i < AXS_FLAG_WAIT_DELAY * 100; i++) {
> > + unsigned int status = reg_get(host, INT_STATUS);
> > +
> > + if (status & (1 << flag)) {
> > + reg_set(host, INT_CLR_STATUS, 1 << flag);
> > + return;
> > + }
> > +
> > + udelay(10);
> > + }
> > +
> > + /*
> > + * Since we cannot report this problem any further than
> > + * axs_nand_{write|read}_buf() letting user know there's a problem.
> > + */
> > + dev_err(host->dev, "Waited too long (%d s.) for flag/bit %d\n",
> > + AXS_FLAG_WAIT_DELAY, flag);
> > +}
>
> Hm... I'm not sure the above is really true.
>
> The NAND core uses the replaceable chip->waitfunc callback to check the
> status of issued commands. See for instance:
>
> static int nand_write_oob_std(struct mtd_info *mtd, struct nand_chip *chip,
> int page)
> {
> int status = 0;
> const uint8_t *buf = chip->oob_poi;
> int length = mtd->oobsize;
>
> chip->cmdfunc(mtd, NAND_CMD_SEQIN, mtd->writesize, page);
> chip->write_buf(mtd, buf, length);
> /* Send command to program the OOB data */
> chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
>
> status = chip->waitfunc(mtd, chip);
>
> return status & NAND_STATUS_FAIL ? -EIO : 0;
> }
>
> On the other side, if you are clearing the flags in axs_flag_wait_and_reset()
> it might be a bit hard for you to get this right.
>
> IOW, I'm not saying you *must* do this, but instead suggesting that you take
> a look at waitfunc() and see if it helps report a proper error in the
> read/write path.
As I may understand from generic implementation of "waitfunc" in
"nand_base.c" it checks status of NAND chip itself - which IMHO makes
sense. And this is done via NAND_CMD_STATUS command sent to chip through
NAND controller.
In AXS NAND driver you may see 2 types of wait checks:
1. NAND_ISR_CMDDONE - this flag means that NAND controller has just
executed provided command (i.e. set proper signal on control lines of
NAND chip etc), but it doesn't mean NAND chip itself has completed
command execution. That's why "waitfunc" is not direct replacement here.
2. NAND_ISR_{RX|TX}DMACOMPLETE - this flag indicates completion of DMA
transfer to/from NAND chip to RAM. "waitfunc" won't work here as well.
I hope this explanation makes sense.
Regards,
Alexey
Hi Ezequiel,
On Mon, 2014-04-07 at 10:13 +0400, Alexey Brodkin wrote:
> Hi Ezequiel,
>
> On Fri, 2014-04-04 at 11:09 -0300, Ezequiel Garcia wrote:
> > On Apr 04, Alexey Brodkin wrote:
> > > Signed-off-by: Alexey Brodkin <[email protected]>
> > >
> >
> > Maybe it would be nice adding some driver description here, so the commit
> > log actually says something useful about the commit.
>
> Well, I thought about it but frankly I'm not sure if there's anything
> else to add to commit title "driver for NAND controller used on Synopsys
> AXS dev boards".
>
> Do you think more info may make sense?
>
> > > +/**
> > > + * axs_flag_wait_and_reset - Waits until requested flag in INT_STATUS register
> > > + * is set by HW and resets it by writing "1" in INT_CLR_STATUS.
> > > + * @host: Pointer to private data structure.
> > > + * @flag: Bit/flag offset in INT_STATUS register
> > > + */
> > > +static void axs_flag_wait_and_reset(struct axs_nand_host *host, int flag)
> > > +{
> > > + unsigned int i;
> > > +
> > > + for (i = 0; i < AXS_FLAG_WAIT_DELAY * 100; i++) {
> > > + unsigned int status = reg_get(host, INT_STATUS);
> > > +
> > > + if (status & (1 << flag)) {
> > > + reg_set(host, INT_CLR_STATUS, 1 << flag);
> > > + return;
> > > + }
> > > +
> > > + udelay(10);
> > > + }
> > > +
> > > + /*
> > > + * Since we cannot report this problem any further than
> > > + * axs_nand_{write|read}_buf() letting user know there's a problem.
> > > + */
> > > + dev_err(host->dev, "Waited too long (%d s.) for flag/bit %d\n",
> > > + AXS_FLAG_WAIT_DELAY, flag);
> > > +}
> >
> > Hm... I'm not sure the above is really true.
> >
> > The NAND core uses the replaceable chip->waitfunc callback to check the
> > status of issued commands. See for instance:
> >
> > static int nand_write_oob_std(struct mtd_info *mtd, struct nand_chip *chip,
> > int page)
> > {
> > int status = 0;
> > const uint8_t *buf = chip->oob_poi;
> > int length = mtd->oobsize;
> >
> > chip->cmdfunc(mtd, NAND_CMD_SEQIN, mtd->writesize, page);
> > chip->write_buf(mtd, buf, length);
> > /* Send command to program the OOB data */
> > chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
> >
> > status = chip->waitfunc(mtd, chip);
> >
> > return status & NAND_STATUS_FAIL ? -EIO : 0;
> > }
> >
> > On the other side, if you are clearing the flags in axs_flag_wait_and_reset()
> > it might be a bit hard for you to get this right.
> >
> > IOW, I'm not saying you *must* do this, but instead suggesting that you take
> > a look at waitfunc() and see if it helps report a proper error in the
> > read/write path.
>
> As I may understand from generic implementation of "waitfunc" in
> "nand_base.c" it checks status of NAND chip itself - which IMHO makes
> sense. And this is done via NAND_CMD_STATUS command sent to chip through
> NAND controller.
>
> In AXS NAND driver you may see 2 types of wait checks:
> 1. NAND_ISR_CMDDONE - this flag means that NAND controller has just
> executed provided command (i.e. set proper signal on control lines of
> NAND chip etc), but it doesn't mean NAND chip itself has completed
> command execution. That's why "waitfunc" is not direct replacement here.
>
> 2. NAND_ISR_{RX|TX}DMACOMPLETE - this flag indicates completion of DMA
> transfer to/from NAND chip to RAM. "waitfunc" won't work here as well.
>
> I hope this explanation makes sense.
Please treat this message as a gentle reminder.
I'm wondering if my explanation in the previous email makes any sense or
I still need to fix stuff.
Regards,
Alexey
On Apr 11, Alexey Brodkin wrote:
> Hi Ezequiel,
>
> On Mon, 2014-04-07 at 10:13 +0400, Alexey Brodkin wrote:
> > Hi Ezequiel,
> >
> > On Fri, 2014-04-04 at 11:09 -0300, Ezequiel Garcia wrote:
> > > On Apr 04, Alexey Brodkin wrote:
> > > > Signed-off-by: Alexey Brodkin <[email protected]>
> > > >
> > >
> > > Maybe it would be nice adding some driver description here, so the commit
> > > log actually says something useful about the commit.
> >
> > Well, I thought about it but frankly I'm not sure if there's anything
> > else to add to commit title "driver for NAND controller used on Synopsys
> > AXS dev boards".
> >
> > Do you think more info may make sense?
> >
> > > > +/**
> > > > + * axs_flag_wait_and_reset - Waits until requested flag in INT_STATUS register
> > > > + * is set by HW and resets it by writing "1" in INT_CLR_STATUS.
> > > > + * @host: Pointer to private data structure.
> > > > + * @flag: Bit/flag offset in INT_STATUS register
> > > > + */
> > > > +static void axs_flag_wait_and_reset(struct axs_nand_host *host, int flag)
> > > > +{
> > > > + unsigned int i;
> > > > +
> > > > + for (i = 0; i < AXS_FLAG_WAIT_DELAY * 100; i++) {
> > > > + unsigned int status = reg_get(host, INT_STATUS);
> > > > +
> > > > + if (status & (1 << flag)) {
> > > > + reg_set(host, INT_CLR_STATUS, 1 << flag);
> > > > + return;
> > > > + }
> > > > +
> > > > + udelay(10);
> > > > + }
> > > > +
> > > > + /*
> > > > + * Since we cannot report this problem any further than
> > > > + * axs_nand_{write|read}_buf() letting user know there's a problem.
> > > > + */
> > > > + dev_err(host->dev, "Waited too long (%d s.) for flag/bit %d\n",
> > > > + AXS_FLAG_WAIT_DELAY, flag);
> > > > +}
> > >
> > > Hm... I'm not sure the above is really true.
> > >
> > > The NAND core uses the replaceable chip->waitfunc callback to check the
> > > status of issued commands. See for instance:
> > >
> > > static int nand_write_oob_std(struct mtd_info *mtd, struct nand_chip *chip,
> > > int page)
> > > {
> > > int status = 0;
> > > const uint8_t *buf = chip->oob_poi;
> > > int length = mtd->oobsize;
> > >
> > > chip->cmdfunc(mtd, NAND_CMD_SEQIN, mtd->writesize, page);
> > > chip->write_buf(mtd, buf, length);
> > > /* Send command to program the OOB data */
> > > chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
> > >
> > > status = chip->waitfunc(mtd, chip);
> > >
> > > return status & NAND_STATUS_FAIL ? -EIO : 0;
> > > }
> > >
> > > On the other side, if you are clearing the flags in axs_flag_wait_and_reset()
> > > it might be a bit hard for you to get this right.
> > >
> > > IOW, I'm not saying you *must* do this, but instead suggesting that you take
> > > a look at waitfunc() and see if it helps report a proper error in the
> > > read/write path.
> >
> > As I may understand from generic implementation of "waitfunc" in
> > "nand_base.c" it checks status of NAND chip itself - which IMHO makes
> > sense. And this is done via NAND_CMD_STATUS command sent to chip through
> > NAND controller.
> >
> > In AXS NAND driver you may see 2 types of wait checks:
> > 1. NAND_ISR_CMDDONE - this flag means that NAND controller has just
> > executed provided command (i.e. set proper signal on control lines of
> > NAND chip etc), but it doesn't mean NAND chip itself has completed
> > command execution. That's why "waitfunc" is not direct replacement here.
> >
> > 2. NAND_ISR_{RX|TX}DMACOMPLETE - this flag indicates completion of DMA
> > transfer to/from NAND chip to RAM. "waitfunc" won't work here as well.
> >
> > I hope this explanation makes sense.
>
> Please treat this message as a gentle reminder.
> I'm wondering if my explanation in the previous email makes any sense or
> I still need to fix stuff.
Well, I was merely suggesting to look into using waitfunc(), so you definitely
don't need to fix anything (at least from my side).
Just as a comment, regarding your explanation above, I think you can override
the waitfunc() and check the NAND_CMD_STATUS, together with the check for your
ISR flags. However, you know more about your hardware than me, so this is
*just* a suggestion.
The driver looks fine broadly speaking. I guess it's up to Brian now to provide
further feedback.
--
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
Dear Brian,
On Fri, 2014-04-11 at 12:07 -0300, [email protected]
wrote:
> On Apr 11, Alexey Brodkin wrote:
> > Hi Ezequiel,
> >
> > On Mon, 2014-04-07 at 10:13 +0400, Alexey Brodkin wrote:
> > > Hi Ezequiel,
> > >
> > > On Fri, 2014-04-04 at 11:09 -0300, Ezequiel Garcia wrote:
> > > > On Apr 04, Alexey Brodkin wrote:
> > > > > Signed-off-by: Alexey Brodkin <[email protected]>
> > > > >
> > > >
> > > > Maybe it would be nice adding some driver description here, so the commit
> > > > log actually says something useful about the commit.
> > >
> > > Well, I thought about it but frankly I'm not sure if there's anything
> > > else to add to commit title "driver for NAND controller used on Synopsys
> > > AXS dev boards".
> > >
> > > Do you think more info may make sense?
> > >
> > > > > +/**
> > > > > + * axs_flag_wait_and_reset - Waits until requested flag in INT_STATUS register
> > > > > + * is set by HW and resets it by writing "1" in INT_CLR_STATUS.
> > > > > + * @host: Pointer to private data structure.
> > > > > + * @flag: Bit/flag offset in INT_STATUS register
> > > > > + */
> > > > > +static void axs_flag_wait_and_reset(struct axs_nand_host *host, int flag)
> > > > > +{
> > > > > + unsigned int i;
> > > > > +
> > > > > + for (i = 0; i < AXS_FLAG_WAIT_DELAY * 100; i++) {
> > > > > + unsigned int status = reg_get(host, INT_STATUS);
> > > > > +
> > > > > + if (status & (1 << flag)) {
> > > > > + reg_set(host, INT_CLR_STATUS, 1 << flag);
> > > > > + return;
> > > > > + }
> > > > > +
> > > > > + udelay(10);
> > > > > + }
> > > > > +
> > > > > + /*
> > > > > + * Since we cannot report this problem any further than
> > > > > + * axs_nand_{write|read}_buf() letting user know there's a problem.
> > > > > + */
> > > > > + dev_err(host->dev, "Waited too long (%d s.) for flag/bit %d\n",
> > > > > + AXS_FLAG_WAIT_DELAY, flag);
> > > > > +}
> > > >
> > > > Hm... I'm not sure the above is really true.
> > > >
> > > > The NAND core uses the replaceable chip->waitfunc callback to check the
> > > > status of issued commands. See for instance:
> > > >
> > > > static int nand_write_oob_std(struct mtd_info *mtd, struct nand_chip *chip,
> > > > int page)
> > > > {
> > > > int status = 0;
> > > > const uint8_t *buf = chip->oob_poi;
> > > > int length = mtd->oobsize;
> > > >
> > > > chip->cmdfunc(mtd, NAND_CMD_SEQIN, mtd->writesize, page);
> > > > chip->write_buf(mtd, buf, length);
> > > > /* Send command to program the OOB data */
> > > > chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
> > > >
> > > > status = chip->waitfunc(mtd, chip);
> > > >
> > > > return status & NAND_STATUS_FAIL ? -EIO : 0;
> > > > }
> > > >
> > > > On the other side, if you are clearing the flags in axs_flag_wait_and_reset()
> > > > it might be a bit hard for you to get this right.
> > > >
> > > > IOW, I'm not saying you *must* do this, but instead suggesting that you take
> > > > a look at waitfunc() and see if it helps report a proper error in the
> > > > read/write path.
> > >
> > > As I may understand from generic implementation of "waitfunc" in
> > > "nand_base.c" it checks status of NAND chip itself - which IMHO makes
> > > sense. And this is done via NAND_CMD_STATUS command sent to chip through
> > > NAND controller.
> > >
> > > In AXS NAND driver you may see 2 types of wait checks:
> > > 1. NAND_ISR_CMDDONE - this flag means that NAND controller has just
> > > executed provided command (i.e. set proper signal on control lines of
> > > NAND chip etc), but it doesn't mean NAND chip itself has completed
> > > command execution. That's why "waitfunc" is not direct replacement here.
> > >
> > > 2. NAND_ISR_{RX|TX}DMACOMPLETE - this flag indicates completion of DMA
> > > transfer to/from NAND chip to RAM. "waitfunc" won't work here as well.
> > >
> > > I hope this explanation makes sense.
> >
> > Please treat this message as a gentle reminder.
> > I'm wondering if my explanation in the previous email makes any sense or
> > I still need to fix stuff.
>
> Well, I was merely suggesting to look into using waitfunc(), so you definitely
> don't need to fix anything (at least from my side).
>
> Just as a comment, regarding your explanation above, I think you can override
> the waitfunc() and check the NAND_CMD_STATUS, together with the check for your
> ISR flags. However, you know more about your hardware than me, so this is
> *just* a suggestion.
>
> The driver looks fine broadly speaking. I guess it's up to Brian now to provide
> further feedback.
I'm wondering if there's a chance for you to look at this patch anytime
soon?
Regards,
Alexey