[patch/RESEND 2.6.29-rc3-git] NAND: davinci_nand driver

[patch/RESEND 2.6.29-rc3-git] NAND: davinci_nand driver

[David Brownell]

From: David Brownell <dbrownell@users.sourceforge.net>

This is a device driver for the NAND flash controller found on the
various DaVinci family chips.  It handles up to four SoC chipselects,
and some flavors of secondary chipselect (e.g. based on upper bits
of the address bus) as used with some multichip packages.  (Including
the 2 GByte chips used on some TI devel boards.)

The 1-bit ECC hardware is supported (3 bytes ECC per 512 bytes data);
but not yet the newer 4-bit ECC (10 bytes ECC per 512 bytes data), as
available on chips like the DM355 or OMAP-L137 and needed with the
more error-prone MLC NAND chips.

This is a cleaned-up version of code that's been in use for several
years now; sanity checked with the new drivers/mtd/tests.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
NOTE:  matches current version in DaVinci GIT tree, which means
it depends on <mach/cpu.h> which isn't (yet) in mainline.

ALSO:  includes <mach/nand.h> ... arguably that should be part
of the same set of patches that include <mach/cpu.h> and board
setup code patches that use that platform data.


 arch/arm/mach-davinci/include/mach/nand.h |   80 +++
 drivers/mtd/nand/Kconfig                  |    7 
 drivers/mtd/nand/Makefile                 |    1 
 drivers/mtd/nand/davinci_nand.c           |  587 ++++++++++++++++++++++++++++
 4 files changed, 675 insertions(+)

--- /dev/null
+++ b/arch/arm/mach-davinci/include/mach/nand.h
@@ -0,0 +1,80 @@
+/*
+ * mach-davinci/nand.h
+ *
+ * Copyright (C) 2006 Texas Instruments.
+ *
+ * Ported to 2.6.23 Copyright (C) 2008 by
+ *   Sander Huijsen <Shuijsen@optelecom-nkf.com>
+ *   Troy Kisky <troy.kisky@boundarydevices.com>
+ *   Dirk Behme <Dirk.Behme@gmail.com>
+ *
+ * --------------------------------------------------------------------------
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#ifndef __ARCH_ARM_DAVINCI_NAND_H
+#define __ARCH_ARM_DAVINCI_NAND_H
+
+#include <linux/mtd/nand.h>
+
+#define NRCSR_OFFSET		0x00
+#define AWCCR_OFFSET		0x04
+#define A1CR_OFFSET		0x10
+#define NANDFCR_OFFSET		0x60
+#define NANDFSR_OFFSET		0x64
+#define NANDF1ECC_OFFSET	0x70
+
+/* 4-bit ECC syndrome registers */
+#define NAND_4BIT_ECC_LOAD_OFFSET	0xbc
+#define NAND_4BIT_ECC1_OFFSET		0xc0
+#define NAND_4BIT_ECC2_OFFSET		0xc4
+#define NAND_4BIT_ECC3_OFFSET		0xc8
+#define NAND_4BIT_ECC4_OFFSET		0xcc
+#define NAND_ERR_ADD1_OFFSET		0xd0
+#define NAND_ERR_ADD2_OFFSET		0xd4
+#define NAND_ERR_ERRVAL1_OFFSET		0xd8
+#define NAND_ERR_ERRVAL2_OFFSET		0xdc
+
+/* NOTE:  boards don't need to use these address bits
+ * for ALE/CLE unless they support booting from NAND.
+ * They're used unless platform data overrides them.
+ */
+#define	MASK_ALE		0x08
+#define	MASK_CLE		0x10
+
+struct davinci_nand_pdata {		/* platform_data */
+	u32			mask_ale;
+	u32			mask_cle;
+
+	/* for packages using two chipselects */
+	u32			mask_chipsel;
+
+	/* board's default static partition info */
+	struct mtd_partition	*parts;
+	unsigned		nr_parts;
+
+	/* none  == NAND_ECC_NONE (strongly *not* advised!!)
+	 * soft  == NAND_ECC_SOFT
+	 * 1-bit == NAND_ECC_HW
+	 * 4-bit == NAND_ECC_HW_SYNDROME (not on all chips)
+	 */
+	nand_ecc_modes_t	ecc_mode;
+
+	/* e.g. NAND_BUSWIDTH_16 or NAND_USE_FLASH_BBT */
+	unsigned		options;
+};
+
+#endif	/* __ARCH_ARM_DAVINCI_NAND_H */
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -427,4 +427,11 @@ config MTD_NAND_SH_FLCTL
 	  Several Renesas SuperH CPU has FLCTL. This option enables support
 	  for NAND Flash using FLCTL. This driver support SH7723.
 
+config MTD_NAND_DAVINCI
+        tristate "Support NAND on DaVinci SoC"
+        depends on ARCH_DAVINCI
+        help
+	  Enable the driver for NAND flash chips on Texas Instruments
+	  DaVinci processors.
+
 endif # MTD_NAND
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -36,5 +36,6 @@ obj-$(CONFIG_MTD_NAND_FSL_ELBC)		+= fsl_
 obj-$(CONFIG_MTD_NAND_FSL_UPM)		+= fsl_upm.o
 obj-$(CONFIG_MTD_NAND_SH_FLCTL)		+= sh_flctl.o
 obj-$(CONFIG_MTD_NAND_MXC)		+= mxc_nand.o
+obj-$(CONFIG_MTD_NAND_DAVINCI)		+= davinci_nand.o
 
 nand-objs := nand_base.o nand_bbt.o
--- /dev/null
+++ b/drivers/mtd/nand/davinci_nand.c
@@ -0,0 +1,587 @@
+/*
+ * davinci_nand.c - NAND Flash Driver for DaVinci family chips
+ *
+ * Copyright (C) 2006 Texas Instruments.
+ *
+ * Port to 2.6.23 Copyright (C) 2008 by:
+ *   Sander Huijsen <Shuijsen@optelecom-nkf.com>
+ *   Troy Kisky <troy.kisky@boundarydevices.com>
+ *   Dirk Behme <Dirk.Behme@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/err.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/mtd/nand.h>
+#include <linux/mtd/partitions.h>
+
+#include <mach/cpu.h>
+#include <mach/nand.h>
+
+#include <asm/mach-types.h>
+
+
+#ifdef CONFIG_MTD_PARTITIONS
+static inline int mtd_has_partitions(void) { return 1; }
+#else
+static inline int mtd_has_partitions(void) { return 0; }
+#endif
+
+#ifdef CONFIG_MTD_CMDLINE_PARTS
+static inline int mtd_has_cmdlinepart(void) { return 1; }
+#else
+static inline int mtd_has_cmdlinepart(void) { return 0; }
+#endif
+
+
+/*
+ * This is a device driver for the NAND flash controller found on the
+ * various DaVinci family chips.  It handles up to four SoC chipselects,
+ * and some flavors of secondary chipselect (e.g. based on A12) as used
+ * with multichip packages.
+ *
+ * The 1-bit ECC hardware is supported, but not yet the newer 4-bit ECC
+ * available on chips like the DM355 and OMAP-L137 and needed with the
+ * more error-prone MLC NAND chips.
+ *
+ * This driver assumes EM_WAIT connects all the NAND devices' RDY/nBUSY
+ * outputs in a "wire-AND" configuration, with no per-chip signals.
+ */
+struct davinci_nand_info {
+	struct mtd_info		mtd;
+	struct nand_chip	chip;
+
+	struct device		*dev;
+	struct clk		*clk;
+	bool			partitioned;
+
+	void __iomem		*base;
+	void __iomem		*vaddr;
+
+	u32			ioaddr;
+	u32			current_cs;
+
+	u32			mask_chipsel;
+	u32			mask_ale;
+	u32			mask_cle;
+
+	u32			core_chipsel;
+};
+
+static DEFINE_SPINLOCK(davinci_nand_lock);
+
+#define to_davinci_nand(m) container_of(m, struct davinci_nand_info, mtd)
+
+
+static inline unsigned int davinci_nand_readl(struct davinci_nand_info *info,
+		int offset)
+{
+	return __raw_readl(info->base + offset);
+}
+
+static inline void davinci_nand_writel(struct davinci_nand_info *info,
+		int offset, unsigned long value)
+{
+	__raw_writel(value, info->base + offset);
+}
+
+/*----------------------------------------------------------------------*/
+
+/*
+ * Access to hardware control lines:  ALE, CLE, secondary chipselect.
+ */
+
+static void nand_davinci_hwcontrol(struct mtd_info *mtd, int cmd,
+				   unsigned int ctrl)
+{
+	struct davinci_nand_info	*info = to_davinci_nand(mtd);
+	u32				addr = info->current_cs;
+	struct nand_chip		*nand = mtd->priv;
+
+	/* Did the control lines change? */
+	if (ctrl & NAND_CTRL_CHANGE) {
+		if ((ctrl & NAND_CTRL_CLE) == NAND_CTRL_CLE)
+			addr |= info->mask_cle;
+		else if ((ctrl & NAND_CTRL_ALE) == NAND_CTRL_ALE)
+			addr |= info->mask_ale;
+
+		nand->IO_ADDR_W = (void __iomem __force *)addr;
+	}
+
+	if (cmd != NAND_CMD_NONE)
+		iowrite8(cmd, nand->IO_ADDR_W);
+}
+
+static void nand_davinci_select_chip(struct mtd_info *mtd, int chip)
+{
+	struct davinci_nand_info	*info = to_davinci_nand(mtd);
+	u32				addr = info->ioaddr;
+
+	/* maybe kick in a second chipselect */
+	if (chip > 0)
+		addr |= info->mask_chipsel;
+	info->current_cs = addr;
+
+	info->chip.IO_ADDR_W = (void __iomem __force *)addr;
+	info->chip.IO_ADDR_R = info->chip.IO_ADDR_W;
+}
+
+/*----------------------------------------------------------------------*/
+
+/*
+ * 1-bit hardware ECC ... context maintained for each core chipselect
+ */
+
+static inline u32 nand_davinci_readecc_1bit(struct mtd_info *mtd)
+{
+	struct davinci_nand_info *info = to_davinci_nand(mtd);
+
+	return davinci_nand_readl(info, NANDF1ECC_OFFSET
+			+ 4 * info->core_chipsel);
+}
+
+static void nand_davinci_hwctl_1bit(struct mtd_info *mtd, int mode)
+{
+	struct davinci_nand_info *info;
+	u32 retval;
+	unsigned long flags;
+
+	info = to_davinci_nand(mtd);
+
+	/* Reset ECC hardware */
+	nand_davinci_readecc_1bit(mtd);
+
+	spin_lock_irqsave(&davinci_nand_lock, flags);
+
+	/* Restart ECC hardware */
+	retval = davinci_nand_readl(info, NANDFCR_OFFSET);
+	retval |= BIT(8 + info->core_chipsel);
+	davinci_nand_writel(info, NANDFCR_OFFSET, retval);
+
+	spin_unlock_irqrestore(&davinci_nand_lock, flags);
+}
+
+/*
+ * Read hardware ECC value and pack into three bytes
+ */
+static int nand_davinci_calculate_1bit(struct mtd_info *mtd,
+				      const u_char *dat, u_char *ecc_code)
+{
+	unsigned int ecc_val = nand_davinci_readecc_1bit(mtd);
+	unsigned int tmp = (ecc_val & 0x0fff) | ((ecc_val & 0x0fff0000) >> 4);
+
+	/* invert so that erased block ecc is correct */
+	tmp = ~tmp;
+	ecc_code[0] = (u_char)(tmp);
+	ecc_code[1] = (u_char)(tmp >> 8);
+	ecc_code[2] = (u_char)(tmp >> 16);
+
+	return 0;
+}
+
+static int nand_davinci_correct_1bit(struct mtd_info *mtd, u_char *dat,
+				     u_char *read_ecc, u_char *calc_ecc)
+{
+	struct nand_chip *chip = mtd->priv;
+	u_int32_t eccNand = read_ecc[0] | (read_ecc[1] << 8) |
+					  (read_ecc[2] << 16);
+	u_int32_t eccCalc = calc_ecc[0] | (calc_ecc[1] << 8) |
+					  (calc_ecc[2] << 16);
+	u_int32_t diff = eccCalc ^ eccNand;
+
+	if (diff) {
+		if ((((diff >> 12) ^ diff) & 0xfff) == 0xfff) {
+			/* Correctable error */
+			if ((diff >> (12 + 3)) < chip->ecc.size) {
+				dat[diff >> (12 + 3)] ^= BIT((diff >> 12) & 7);
+				return 1;
+			} else {
+				return -1;
+			}
+		} else if (!(diff & (diff - 1))) {
+			/* Single bit ECC error in the ECC itself,
+			 * nothing to fix */
+			return 1;
+		} else {
+			/* Uncorrectable error */
+			return -1;
+		}
+
+	}
+	return 0;
+}
+
+/*----------------------------------------------------------------------*/
+
+/*
+ * NOTE:  NAND boot requires ALE == EM_A[1], CLE == EM_A[2], so that's
+ * how these chips are normally wired.  This translates to both 8 and 16
+ * bit busses using ALE == BIT(3) in byte addresses, and CLE == BIT(4).
+ *
+ * For now we assume that configuration, or any other one which ignores
+ * the two LSBs for NAND access ... so we can issue 32-bit reads/writes
+ * and have that transparently morphed into multiple NAND operations.
+ */
+static void nand_davinci_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
+{
+	struct nand_chip *chip = mtd->priv;
+
+	if ((0x03 & ((unsigned)buf)) == 0 && (0x03 & len) == 0)
+		ioread32_rep(chip->IO_ADDR_R, buf, len >> 2);
+	else if ((0x01 & ((unsigned)buf)) == 0 && (0x01 & len) == 0)
+		ioread16_rep(chip->IO_ADDR_R, buf, len >> 1);
+	else
+		ioread8_rep(chip->IO_ADDR_R, buf, len);
+}
+
+static void nand_davinci_write_buf(struct mtd_info *mtd,
+		const uint8_t *buf, int len)
+{
+	struct nand_chip *chip = mtd->priv;
+
+	if ((0x03 & ((unsigned)buf)) == 0 && (0x03 & len) == 0)
+		iowrite32_rep(chip->IO_ADDR_R, buf, len >> 2);
+	else if ((0x01 & ((unsigned)buf)) == 0 && (0x01 & len) == 0)
+		iowrite16_rep(chip->IO_ADDR_R, buf, len >> 1);
+	else
+		iowrite8_rep(chip->IO_ADDR_R, buf, len);
+}
+
+/*
+ * Check hardware register for wait status. Returns 1 if device is ready,
+ * 0 if it is still busy.
+ */
+static int nand_davinci_dev_ready(struct mtd_info *mtd)
+{
+	struct davinci_nand_info *info = to_davinci_nand(mtd);
+
+	return davinci_nand_readl(info, NANDFSR_OFFSET) & BIT(0);
+}
+
+static void __init nand_dm6446evm_flash_init(struct davinci_nand_info *info)
+{
+	u32 regval, tmp;
+
+	/*
+	 * NAND FLASH timings @ PLL1 == 459 MHz
+	 *  - AEMIF.CLK freq   = PLL1/6 = 459/6 = 76.5 MHz
+	 *  - AEMIF.CLK period = 1/76.5 MHz = 13.1 ns
+	 */
+	regval = 0
+		| (0 << 31)           /* selectStrobe */
+		| (0 << 30)           /* extWait (never with NAND) */
+		| (1 << 26)           /* writeSetup      10 ns */
+		| (3 << 20)           /* writeStrobe     40 ns */
+		| (1 << 17)           /* writeHold       10 ns */
+		| (0 << 13)           /* readSetup       10 ns */
+		| (3 << 7)            /* readStrobe      60 ns */
+		| (0 << 4)            /* readHold        10 ns */
+		| (3 << 2)            /* turnAround      ?? ns */
+		| (0 << 0)            /* asyncSize       8-bit bus */
+		;
+	tmp = davinci_nand_readl(info, A1CR_OFFSET);
+	if (tmp != regval) {
+		dev_dbg(info->dev, "Warning: NAND config: Set A1CR " \
+		       "reg to 0x%08x, was 0x%08x, should be done by " \
+		       "bootloader.\n", regval, tmp);
+		davinci_nand_writel(info, A1CR_OFFSET, regval);
+	}
+}
+
+/*----------------------------------------------------------------------*/
+
+static int __init nand_davinci_probe(struct platform_device *pdev)
+{
+	struct davinci_nand_pdata	*pdata = pdev->dev.platform_data;
+	struct davinci_nand_info	*info;
+	struct resource			*res1;
+	struct resource			*res2;
+	void __iomem			*vaddr;
+	void __iomem			*base;
+	int				ret;
+	u32				val;
+	nand_ecc_modes_t		ecc_mode;
+
+	/* which external chipselect will we be managing? */
+	if (pdev->id < 0 || pdev->id > 3)
+		return -ENODEV;
+
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info) {
+		dev_err(&pdev->dev, "unable to allocate memory\n");
+		ret = -ENOMEM;
+		goto err_nomem;
+	}
+
+	platform_set_drvdata(pdev, info);
+
+	res1 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	res2 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (!res1 || !res2) {
+		dev_err(&pdev->dev, "resource missing\n");
+		ret = -EINVAL;
+		goto err_res;
+	}
+
+	vaddr = ioremap(res1->start, res1->end - res1->start);
+	base = ioremap(res2->start, res2->end - res2->start);
+	if (!vaddr || !base) {
+		dev_err(&pdev->dev, "ioremap failed\n");
+		ret = -EINVAL;
+		goto err_ioremap;
+
+	}
+
+	info->dev		= &pdev->dev;
+	info->base		= base;
+	info->vaddr		= vaddr;
+
+	info->mtd.priv		= &info->chip;
+	info->mtd.name		= dev_name(&pdev->dev);
+	info->mtd.owner		= THIS_MODULE;
+
+	info->chip.IO_ADDR_R	= vaddr;
+	info->chip.IO_ADDR_W	= vaddr;
+	info->chip.chip_delay	= 0;
+	info->chip.select_chip	= nand_davinci_select_chip;
+
+	/* options such as NAND_USE_FLASH_BBT or 16-bit widths */
+	info->chip.options	= pdata ? pdata->options : 0;
+
+	info->ioaddr		= (u32 __force) vaddr;
+
+	info->current_cs	= info->ioaddr;
+	info->core_chipsel	= pdev->id;
+	info->mask_chipsel	= pdata->mask_chipsel;
+
+	/* use nandboot-capable ALE/CLE masks by default */
+	if (pdata && pdata->mask_ale)
+		info->mask_ale	= pdata->mask_cle;
+	else
+		info->mask_ale	= MASK_ALE;
+	if (pdata && pdata->mask_cle)
+		info->mask_cle	= pdata->mask_cle;
+	else
+		info->mask_cle	= MASK_CLE;
+
+	/* Set address of hardware control function */
+	info->chip.cmd_ctrl	= nand_davinci_hwcontrol;
+	info->chip.dev_ready	= nand_davinci_dev_ready;
+
+	/* Speed up buffer I/O */
+	info->chip.read_buf     = nand_davinci_read_buf;
+	info->chip.write_buf    = nand_davinci_write_buf;
+
+	/* use board-specific ECC config; else, the best available */
+	if (pdata)
+		ecc_mode = pdata->ecc_mode;
+	else if (cpu_is_davinci_dm355())
+		ecc_mode = NAND_ECC_HW_SYNDROME;
+	else
+		ecc_mode = NAND_ECC_HW;
+
+	switch (ecc_mode) {
+	case NAND_ECC_NONE:
+	case NAND_ECC_SOFT:
+		break;
+	case NAND_ECC_HW:
+		info->chip.ecc.calculate = nand_davinci_calculate_1bit;
+		info->chip.ecc.correct = nand_davinci_correct_1bit;
+		info->chip.ecc.hwctl = nand_davinci_hwctl_1bit;
+		info->chip.ecc.size = 512;
+		info->chip.ecc.bytes = 3;
+		break;
+	case NAND_ECC_HW_SYNDROME:
+		/* FIXME implement */
+		info->chip.ecc.size = 512;
+		info->chip.ecc.bytes = 10;
+
+		dev_warn(&pdev->dev, "4-bit ECC nyet supported\n");
+		/* FALL THROUGH */
+	default:
+		ret = -EINVAL;
+		goto err_ecc;
+	}
+	info->chip.ecc.mode = ecc_mode;
+
+	info->clk = clk_get(&pdev->dev, "AEMIFCLK");
+	if (IS_ERR(info->clk)) {
+		ret = PTR_ERR(info->clk);
+		dev_dbg(&pdev->dev, "unable to get AEMIFCLK, err %d\n", ret);
+		goto err_clk;
+	}
+
+	ret = clk_enable(info->clk);
+	if (ret < 0) {
+		dev_dbg(&pdev->dev, "unable to enable AEMIFCLK, err %d\n", ret);
+		goto err_clk_enable;
+	}
+
+	/* EMIF timings should normally be set by the boot loader,
+	 * especially after boot-from-NAND.  The *only* reason to
+	 * have this special casing for the DM6446 EVM is to work
+	 * with boot-from-NOR ... with CS0 manually re-jumpered
+	 * (after startup) so it addresses the NAND flash, not NOR.
+	 * Even for dev boards, that's unusually rude...
+	 */
+	if (machine_is_davinci_evm())
+		nand_dm6446evm_flash_init(info);
+
+	spin_lock_irq(&davinci_nand_lock);
+
+	/* put CSxNAND into NAND mode */
+	val = davinci_nand_readl(info, NANDFCR_OFFSET);
+	val |= BIT(info->core_chipsel);
+	davinci_nand_writel(info, NANDFCR_OFFSET, val);
+
+	spin_unlock_irq(&davinci_nand_lock);
+
+	/* Scan to find existence of the device(s) */
+	ret = nand_scan(&info->mtd, pdata->mask_chipsel ? 2 : 1);
+	if (ret < 0) {
+		dev_dbg(&pdev->dev, "no NAND chip(s) found\n");
+		goto err_scan;
+	}
+
+	if (mtd_has_partitions()) {
+		struct mtd_partition	*mtd_parts = NULL;
+		int			mtd_parts_nb = 0;
+
+		if (mtd_has_cmdlinepart()) {
+			static const char *probes[] __initconst =
+				{ "cmdlinepart", NULL };
+
+			const char		*master_name;
+
+			/* Set info->mtd.name = 0 temporarily */
+			master_name		= info->mtd.name;
+			info->mtd.name		= (char *)0;
+
+			/* info->mtd.name == 0, means: don't bother checking
+			   <mtd-id> */
+			mtd_parts_nb = parse_mtd_partitions(&info->mtd, probes,
+							    &mtd_parts, 0);
+
+			/* Restore info->mtd.name */
+			info->mtd.name = master_name;
+		}
+
+		if (mtd_parts_nb <= 0 && pdata) {
+			mtd_parts = pdata->parts;
+			mtd_parts_nb = pdata->nr_parts;
+		}
+
+		/* Register any partitions */
+		if (mtd_parts_nb > 0) {
+			ret = add_mtd_partitions(&info->mtd,
+					mtd_parts, mtd_parts_nb);
+			if (ret == 0)
+				info->partitioned = true;
+		}
+
+	} else if (pdata && pdata->nr_parts) {
+		dev_warn(&pdev->dev, "ignoring %d default partitions on %s\n",
+				pdata->nr_parts, info->mtd.name);
+	}
+
+	/* If there's no partition info, just package the whole chip
+	 * as a single MTD device.
+	 */
+	if (!info->partitioned)
+		ret = add_mtd_device(&info->mtd) ? -ENODEV : 0;
+
+	if (ret < 0)
+		goto err_scan;
+
+	val = davinci_nand_readl(info, NRCSR_OFFSET);
+	dev_info(&pdev->dev, "controller rev. %d.%d\n",
+	       (val >> 8) & 0xff, val & 0xff);
+
+	return 0;
+
+err_scan:
+	clk_disable(info->clk);
+
+err_clk_enable:
+	clk_put(info->clk);
+
+err_ecc:
+err_clk:
+	if (base)
+		iounmap(base);
+	if (vaddr)
+		iounmap(vaddr);
+
+err_ioremap:
+	kfree(info);
+
+err_nomem:
+err_res:
+	return ret;
+}
+
+static int __exit nand_davinci_remove(struct platform_device *pdev)
+{
+	struct davinci_nand_info *info = platform_get_drvdata(pdev);
+	int status;
+
+	if (mtd_has_partitions() && info->partitioned)
+		status = del_mtd_partitions(&info->mtd);
+	else
+		status = del_mtd_device(&info->mtd);
+
+	iounmap(info->base);
+	iounmap(info->vaddr);
+
+	nand_release(&info->mtd);
+
+	clk_disable(info->clk);
+	clk_put(info->clk);
+
+	kfree(info);
+
+	return 0;
+}
+
+static struct platform_driver nand_davinci_driver = {
+	.remove		= __exit_p(nand_davinci_remove),
+	.driver		= {
+		.name	= "davinci_nand",
+	},
+};
+MODULE_ALIAS("platform:davinci_nand");
+
+static int __init nand_davinci_init(void)
+{
+	return platform_driver_probe(&nand_davinci_driver, nand_davinci_probe);
+}
+module_init(nand_davinci_init);
+
+static void __exit nand_davinci_exit(void)
+{
+	platform_driver_unregister(&nand_davinci_driver);
+}
+module_exit(nand_davinci_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Texas Instruments");
+MODULE_DESCRIPTION("Davinci NAND flash driver");
+

Re: [patch/RESEND 2.6.29-rc3-git] NAND: davinci_nand driver

[Thiago Galesi]

OK a couple of things
 +
> +
> +#ifdef CONFIG_MTD_PARTITIONS
> +static inline int mtd_has_partitions(void) { return 1; }
> +#else
> +static inline int mtd_has_partitions(void) { return 0; }
> +#endif
> +
> +#ifdef CONFIG_MTD_CMDLINE_PARTS
> +static inline int mtd_has_cmdlinepart(void) { return 1; }
> +#else
> +static inline int mtd_has_cmdlinepart(void) { return 0; }
> +#endif

I'm not sure stylewise this is the best way. Even though #ifdefs stays
outside of functions, this is a case where maybe it's better to put it
inside or rething the need / usage for these functions.

> +static int nand_davinci_calculate_1bit(struct mtd_info *mtd,
> +                                     const u_char *dat, u_char *ecc_code)

(and others)

My beef here is the use of u_char. This is really not a standart C
type or a standart Linux type (u8/u32 etc), so we should aim for those
(yes, I hate typing unsigned blah blah blah) but you can use u8 for
that.

> +static void nand_davinci_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
> +{
> +       struct nand_chip *chip = mtd->priv;
> +
> +       if ((0x03 & ((unsigned)buf)) == 0 && (0x03 & len) == 0)
> +               ioread32_rep(chip->IO_ADDR_R, buf, len >> 2);
> +       else if ((0x01 & ((unsigned)buf)) == 0 && (0x01 & len) == 0)

What are those 0x03 and 0x01 (and other places as well), you'll have
to spell out those, preferably using defines.

-- 
-
Thiago Galesi

Re: [patch/RESEND 2.6.29-rc3-git] NAND: davinci_nand driver

[David Brownell]

On Tuesday 24 February 2009, Thiago Galesi wrote:
> OK a couple of things
>  +
> > +
> > +#ifdef CONFIG_MTD_PARTITIONS
> > +static inline int mtd_has_partitions(void) { return 1; }
> > +#else
> > +static inline int mtd_has_partitions(void) { return 0; }
> > +#endif
> > +
> > +#ifdef CONFIG_MTD_CMDLINE_PARTS
> > +static inline int mtd_has_cmdlinepart(void) { return 1; }
> > +#else
> > +static inline int mtd_has_cmdlinepart(void) { return 0; }
> > +#endif
> 
> I'm not sure stylewise this is the best way. Even though #ifdefs stays
> outside of functions, this is a case where maybe it's better to put it
> inside or rething the need / usage for these functions.

My preference would be to have those functions live in the
MTD headers.

Needing #ifdefs in the body of probe() is *extremely* error
prone.  Having function versions makes it harder to commit
a lot of the common errors I've seen, like having some mix
of options gratuitously break compiles because of missing
code fragments or variable declarations.  It also makes it
easier to see when code is obviously doing the wrong thing.

Note that #ifdefs-in-functions is contrary to standard
kernel coding practices.  It persists in the MTD code
mostly due to legacy drivers, from what I can tell, which
do so because ... there's been no better option, such as
having those functions.


> > +static int nand_davinci_calculate_1bit(struct mtd_info *mtd,
> > +                                     const u_char *dat, u_char *ecc_code)
> 
> (and others)
> 
> My beef here is the use of u_char. This is really not a standart C
> type or a standart Linux type (u8/u32 etc), so we should aim for those
> (yes, I hate typing unsigned blah blah blah) but you can use u8 for
> that.

I too would like to see <linux/mtd/*.h> use u8/u32/etc.  But
in this case, it's just doing what the MTD framework does.
If I used u8/u32/etc the usual feedback would be "do what
all the other drivers do", "match the interface decls", etc.


> > +static void nand_davinci_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
> > +{
> > +       struct nand_chip *chip = mtd->priv;
> > +
> > +       if ((0x03 & ((unsigned)buf)) == 0 && (0x03 & len) == 0)
> > +               ioread32_rep(chip->IO_ADDR_R, buf, len >> 2);
> > +       else if ((0x01 & ((unsigned)buf)) == 0 && (0x01 & len) == 0)
> 
> What are those 0x03 and 0x01 (and other places as well), you'll have
> to spell out those, preferably using defines.

The "0x03" is "low two bits", "0x01" is "low one bit", etc.
Those functions can't be used except when memory address
is "naturally" aligned, and the data length likewise.

You might have an argument if you suggested a comment were
appropriate ... but those are very common idioms, used all
over the kernel without #defines that I've ever seen.  Did
you have suggestions for pre-existing symbols to use?

- Dave

Re: [patch/RESEND 2.6.29-rc3-git] NAND: davinci_nand driver

[Thiago Galesi]

On Wed, Feb 25, 2009 at 1:12 AM, David Brownell <david-b@pacbell.net> wrote:
> On Tuesday 24 February 2009, Thiago Galesi wrote:
>> OK a couple of things
>>  +
>> > +
>> > +#ifdef CONFIG_MTD_PARTITIONS
>> > +static inline int mtd_has_partitions(void) { return 1; }
>> > +#else
>> > +static inline int mtd_has_partitions(void) { return 0; }
>> > +#endif
>
> My preference would be to have those functions live in the
> MTD headers.
>
> Needing #ifdefs in the body of probe() is *extremely* error
> prone.  Having function versions makes it harder to commit
> a lot of the common errors I've seen, like having some mix
> of options gratuitously break compiles because of missing
> code fragments or variable declarations.  It also makes it
> easier to see when code is obviously doing the wrong thing.

Agreed. Especially about having #ifdefs in probe, no question about
that, this way is _much_better_

>
> Note that #ifdefs-in-functions is contrary to standard
> kernel coding practices.

Yes, I know (and agree with) that 100% :)

>
> I too would like to see <linux/mtd/*.h> use u8/u32/etc.  But
> in this case, it's just doing what the MTD framework does.
> If I used u8/u32/etc the usual feedback would be "do what
> all the other drivers do", "match the interface decls", etc.

Yes, that happens a lot. But a movement towards 'the right way' is
always welcome.

>
>> > +static void nand_davinci_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
>> > +{
>> > +       struct nand_chip *chip = mtd->priv;
>> > +
>> > +       if ((0x03 & ((unsigned)buf)) == 0 && (0x03 & len) == 0)
>> > +               ioread32_rep(chip->IO_ADDR_R, buf, len >> 2);
>> > +       else if ((0x01 & ((unsigned)buf)) == 0 && (0x01 & len) == 0)
>>
>> What are those 0x03 and 0x01 (and other places as well), you'll have
>> to spell out those, preferably using defines.
>
> The "0x03" is "low two bits", "0x01" is "low one bit", etc.
> Those functions can't be used except when memory address
> is "naturally" aligned, and the data length likewise.
>
> You might have an argument if you suggested a comment were
> appropriate ... but those are very common idioms,

Oh, ok, now I get it. But this is still confusing. (Yes, put a comment there)
No need for the ifdefs though.

Also, in the case of non-aligned acesses what is commonly done goes like this:
you write the small unaligned part with 8/16 bit ops, then the rest
with 32 bit ops.

Maybe it's really not worth speedwise to do all of this, but this is
ARM after all :)


-- 
-
Thiago Galesi

Re: [patch/RESEND 2.6.29-rc3-git] NAND: davinci_nand driver

[Andrew Morton]

On Tue, 24 Feb 2009 15:09:54 -0800
David Brownell <david-b@pacbell.net> wrote:

> From: David Brownell <dbrownell@users.sourceforge.net>
> 
> This is a device driver for the NAND flash controller found on the
> various DaVinci family chips.  It handles up to four SoC chipselects,
> and some flavors of secondary chipselect (e.g. based on upper bits
> of the address bus) as used with some multichip packages.  (Including
> the 2 GByte chips used on some TI devel boards.)
> 
> The 1-bit ECC hardware is supported (3 bytes ECC per 512 bytes data);
> but not yet the newer 4-bit ECC (10 bytes ECC per 512 bytes data), as
> available on chips like the DM355 or OMAP-L137 and needed with the
> more error-prone MLC NAND chips.
> 
> This is a cleaned-up version of code that's been in use for several
> years now; sanity checked with the new drivers/mtd/tests.
> 
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
>
> ...
>
> + * Copyright (C) 2006 Texas Instruments.
> + *
> + * Ported to 2.6.23 Copyright (C) 2008 by
> + *   Sander Huijsen <Shuijsen@optelecom-nkf.com>
> + *   Troy Kisky <troy.kisky@boundarydevices.com>
> + *   Dirk Behme <Dirk.Behme@gmail.com>

hm.  What's the story with authorship, attributions and signoffs here?

>
> ...
>
> +#ifdef CONFIG_MTD_PARTITIONS
> +static inline int mtd_has_partitions(void) { return 1; }
> +#else
> +static inline int mtd_has_partitions(void) { return 0; }
> +#endif
> +
> +#ifdef CONFIG_MTD_CMDLINE_PARTS
> +static inline int mtd_has_cmdlinepart(void) { return 1; }
> +#else
> +static inline int mtd_has_cmdlinepart(void) { return 0; }
> +#endif

These definitions shouldn't be buried in a .c file.

>
> ...
>
> +static void nand_davinci_hwctl_1bit(struct mtd_info *mtd, int mode)
> +{
> +	struct davinci_nand_info *info;
> +	u32 retval;

The identifier `retval' is usually used to identify the value which
this function will return.

> +	unsigned long flags;
> +
> +	info = to_davinci_nand(mtd);
> +
> +	/* Reset ECC hardware */
> +	nand_davinci_readecc_1bit(mtd);
> +
> +	spin_lock_irqsave(&davinci_nand_lock, flags);
> +
> +	/* Restart ECC hardware */
> +	retval = davinci_nand_readl(info, NANDFCR_OFFSET);
> +	retval |= BIT(8 + info->core_chipsel);
> +	davinci_nand_writel(info, NANDFCR_OFFSET, retval);
> +
> +	spin_unlock_irqrestore(&davinci_nand_lock, flags);
> +}
> +
> +/*
> + * Read hardware ECC value and pack into three bytes
> + */
> +static int nand_davinci_calculate_1bit(struct mtd_info *mtd,
> +				      const u_char *dat, u_char *ecc_code)
> +{
> +	unsigned int ecc_val = nand_davinci_readecc_1bit(mtd);
> +	unsigned int tmp = (ecc_val & 0x0fff) | ((ecc_val & 0x0fff0000) >> 4);

argh.

> +	/* invert so that erased block ecc is correct */
> +	tmp = ~tmp;
> +	ecc_code[0] = (u_char)(tmp);
> +	ecc_code[1] = (u_char)(tmp >> 8);
> +	ecc_code[2] = (u_char)(tmp >> 16);

Is there some library facility which is being open-coded here?

> +	return 0;
> +}
> +
>
> ...
>
> +static int __init nand_davinci_probe(struct platform_device *pdev)
> +{
> +	struct davinci_nand_pdata	*pdata = pdev->dev.platform_data;
> +	struct davinci_nand_info	*info;
> +	struct resource			*res1;
> +	struct resource			*res2;
> +	void __iomem			*vaddr;
> +	void __iomem			*base;
> +	int				ret;
> +	u32				val;
> +	nand_ecc_modes_t		ecc_mode;
> +
> +	/* which external chipselect will we be managing? */
> +	if (pdev->id < 0 || pdev->id > 3)
> +		return -ENODEV;
> +
> +	info = kzalloc(sizeof(*info), GFP_KERNEL);
> +	if (!info) {
> +		dev_err(&pdev->dev, "unable to allocate memory\n");
> +		ret = -ENOMEM;
> +		goto err_nomem;
> +	}
> +
> +	platform_set_drvdata(pdev, info);
> +
> +	res1 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	res2 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	if (!res1 || !res2) {
> +		dev_err(&pdev->dev, "resource missing\n");
> +		ret = -EINVAL;
> +		goto err_res;

This leaks `info'.

Please check all the error path unwinding here.

> +	}
> +
>
> ...
>

Re: [patch/RESEND 2.6.29-rc3-git] NAND: davinci_nand driver

[David Brownell]

On Thursday 26 February 2009, Andrew Morton wrote:

> > + * Copyright (C) 2006 Texas Instruments.
> > + *
> > + * Ported to 2.6.23 Copyright (C) 2008 by
> > + *   Sander Huijsen <Shuijsen@optelecom-nkf.com>
> > + *   Troy Kisky <troy.kisky@boundarydevices.com>
> > + *   Dirk Behme <Dirk.Behme@gmail.com>
> 
> hm.  What's the story with authorship, attributions and signoffs here?

Written by TI (PSP team in India, ISTR) with no individual
authorship credited, and shipped with a MontaVista 2.6.10
kernel.  Ported as noted; I could presumably add my own
copyright given recent updates I've made.  Likewise Felipe
Balbi.  Kevin Hilman has signed off on various patches as
part of merging them to the DaVinci tree.

(To the TI team reading this via the DaVinci list:  I think
Andrew is hinting that a Signed-off-By from a TI person
would be a Nice Thing.  Same for Dirk, and maybe others.)


> > ...
> >
> > +#ifdef CONFIG_MTD_PARTITIONS
> > +static inline int mtd_has_partitions(void) { return 1; }
> > +#else
> > +static inline int mtd_has_partitions(void) { return 0; }
> > +#endif
> > +
> > +#ifdef CONFIG_MTD_CMDLINE_PARTS
> > +static inline int mtd_has_cmdlinepart(void) { return 1; }
> > +#else
> > +static inline int mtd_has_cmdlinepart(void) { return 0; }
> > +#endif
> 
> These definitions shouldn't be buried in a .c file.

I will send along a patch to move them to <linux/mtd/...> headers,
now that there seems to be a bit of recognition that the current
ifdef-centric approach in the MTD mapping drivers is trouble.  ;)


> >
> > ...
> >
> > +static void nand_davinci_hwctl_1bit(struct mtd_info *mtd, int mode)
> > +{
> > +	struct davinci_nand_info *info;
> > +	u32 retval;
> 
> The identifier `retval' is usually used to identify the value which
> this function will return.

True; resolved in the appended fixup patch.


> > +static int nand_davinci_calculate_1bit(struct mtd_info *mtd,
> > +				      const u_char *dat, u_char *ecc_code)
> > +{
> > +	unsigned int ecc_val = nand_davinci_readecc_1bit(mtd);
> > +	unsigned int tmp = (ecc_val & 0x0fff) | ((ecc_val & 0x0fff0000) >> 4);
> 
> argh.

It seems the best-dressed pirates have parrots named "argh"!  ;)


> > +	/* invert so that erased block ecc is correct */
> > +	tmp = ~tmp;
> > +	ecc_code[0] = (u_char)(tmp);
> > +	ecc_code[1] = (u_char)(tmp >> 8);
> > +	ecc_code[2] = (u_char)(tmp >> 16);
> 
> Is there some library facility which is being open-coded here?

I don't know of such a facility:  24-bit integer into 3-byte buffer.


> > +	return 0;
> > +}
> > +
> >
> > ...
> >
> > +static int __init nand_davinci_probe(struct platform_device *pdev)
> > +{
> > +		... deletia ...
> > +
> > +	info = kzalloc(sizeof(*info), GFP_KERNEL);
> > +	if (!info) {
> > +		dev_err(&pdev->dev, "unable to allocate memory\n");
> > +		ret = -ENOMEM;
> > +		goto err_nomem;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, info);
> > +
> > +	res1 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	res2 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > +	if (!res1 || !res2) {
> > +		dev_err(&pdev->dev, "resource missing\n");
> > +		ret = -EINVAL;
> > +		goto err_res;
> 
> This leaks `info'.
> 
> Please check all the error path unwinding here.

OK -- that does look buggish.

(Kevin -- I suggest you merge this to the DaVinci tree to
make the eventual resync-with-mainline easier.)

=====================
From: David Brownell <dbrownell@users.sourceforge.net>

Minor fixes to the davinci-nand driver patch.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 drivers/mtd/nand/davinci_nand.c |   35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

--- a/drivers/mtd/nand/davinci_nand.c
+++ b/drivers/mtd/nand/davinci_nand.c
@@ -161,7 +161,7 @@ static inline u32 nand_davinci_readecc_1
 static void nand_davinci_hwctl_1bit(struct mtd_info *mtd, int mode)
 {
 	struct davinci_nand_info *info;
-	u32 retval;
+	u32 nandcfr;
 	unsigned long flags;
 
 	info = to_davinci_nand(mtd);
@@ -172,9 +172,9 @@ static void nand_davinci_hwctl_1bit(stru
 	spin_lock_irqsave(&davinci_nand_lock, flags);
 
 	/* Restart ECC hardware */
-	retval = davinci_nand_readl(info, NANDFCR_OFFSET);
-	retval |= BIT(8 + info->core_chipsel);
-	davinci_nand_writel(info, NANDFCR_OFFSET, retval);
+	nandcfr = davinci_nand_readl(info, NANDFCR_OFFSET);
+	nandcfr |= BIT(8 + info->core_chipsel);
+	davinci_nand_writel(info, NANDFCR_OFFSET, nandcfr);
 
 	spin_unlock_irqrestore(&davinci_nand_lock, flags);
 }
@@ -186,13 +186,13 @@ static int nand_davinci_calculate_1bit(s
 				      const u_char *dat, u_char *ecc_code)
 {
 	unsigned int ecc_val = nand_davinci_readecc_1bit(mtd);
-	unsigned int tmp = (ecc_val & 0x0fff) | ((ecc_val & 0x0fff0000) >> 4);
+	unsigned int ecc24 = (ecc_val & 0x0fff) | ((ecc_val & 0x0fff0000) >> 4);
 
 	/* invert so that erased block ecc is correct */
-	tmp = ~tmp;
-	ecc_code[0] = (u_char)(tmp);
-	ecc_code[1] = (u_char)(tmp >> 8);
-	ecc_code[2] = (u_char)(tmp >> 16);
+	ecc24 = ~ecc24;
+	ecc_code[0] = (u_char)(ecc24);
+	ecc_code[1] = (u_char)(ecc24 >> 8);
+	ecc_code[2] = (u_char)(ecc24 >> 16);
 
 	return 0;
 }
@@ -278,7 +278,7 @@ static int nand_davinci_dev_ready(struct
 
 static void __init nand_dm6446evm_flash_init(struct davinci_nand_info *info)
 {
-	u32 regval, tmp;
+	u32 regval, a1cr;
 
 	/*
 	 * NAND FLASH timings @ PLL1 == 459 MHz
@@ -297,11 +297,11 @@ static void __init nand_dm6446evm_flash_
 		| (3 << 2)            /* turnAround      ?? ns */
 		| (0 << 0)            /* asyncSize       8-bit bus */
 		;
-	tmp = davinci_nand_readl(info, A1CR_OFFSET);
-	if (tmp != regval) {
+	a1cr = davinci_nand_readl(info, A1CR_OFFSET);
+	if (a1cr != regval) {
 		dev_dbg(info->dev, "Warning: NAND config: Set A1CR " \
 		       "reg to 0x%08x, was 0x%08x, should be done by " \
-		       "bootloader.\n", regval, tmp);
+		       "bootloader.\n", regval, a1cr);
 		davinci_nand_writel(info, A1CR_OFFSET, regval);
 	}
 }
@@ -338,7 +338,7 @@ static int __init nand_davinci_probe(str
 	if (!res1 || !res2) {
 		dev_err(&pdev->dev, "resource missing\n");
 		ret = -EINVAL;
-		goto err_res;
+		goto err_nomem;
 	}
 
 	vaddr = ioremap(res1->start, res1->end - res1->start);
@@ -347,7 +347,6 @@ static int __init nand_davinci_probe(str
 		dev_err(&pdev->dev, "ioremap failed\n");
 		ret = -EINVAL;
 		goto err_ioremap;
-
 	}
 
 	info->dev		= &pdev->dev;
@@ -525,16 +524,14 @@ err_clk_enable:
 
 err_ecc:
 err_clk:
+err_ioremap:
 	if (base)
 		iounmap(base);
 	if (vaddr)
 		iounmap(vaddr);
 
-err_ioremap:
-	kfree(info);
-
 err_nomem:
-err_res:
+	kfree(info);
 	return ret;
 }
 

[patch 2.6.29-rc6-mmotm] MTD: partitioning utility predicates

[David Brownell]

From: David Brownell <dbrownell@users.sourceforge.net>

Move mtd_has_partitions() and mtd_has_cmdlinepart() inlines from
a DaVinci-specific driver to the <linux/mtd/partitions.h> header.

Use those to eliminate #ifdefs in two drivers which had their
own definitions of mtd_has_partitions().

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
Quite a lot of other MTD drivers could benefit from using use one
or both of these to remove #ifdeffery.  Maybe some Janitors
would like to help?

 drivers/mtd/devices/m25p80.c        |   17 ++++++-----------
 drivers/mtd/devices/mtd_dataflash.c |   16 ++++++----------
 drivers/mtd/nand/davinci_nand.c     |   13 -------------
 include/linux/mtd/partitions.h      |   12 ++++++++++++
 4 files changed, 24 insertions(+), 34 deletions(-)

--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -65,12 +65,6 @@
 #define FAST_READ_DUMMY_BYTE 0
 #endif
 
-#ifdef CONFIG_MTD_PARTITIONS
-#define	mtd_has_partitions()	(1)
-#else
-#define	mtd_has_partitions()	(0)
-#endif
-
 /****************************************************************************/
 
 struct m25p {
@@ -708,12 +702,13 @@ static int __devinit m25p_probe(struct s
 		struct mtd_partition	*parts = NULL;
 		int			nr_parts = 0;
 
-#ifdef CONFIG_MTD_CMDLINE_PARTS
-		static const char *part_probes[] = { "cmdlinepart", NULL, };
+		if (mtd_has_cmdlinepart()) {
+			static const char *part_probes[]
+					= { "cmdlinepart", NULL, };
 
-		nr_parts = parse_mtd_partitions(&flash->mtd,
-				part_probes, &parts, 0);
-#endif
+			nr_parts = parse_mtd_partitions(&flash->mtd,
+					part_probes, &parts, 0);
+		}
 
 		if (nr_parts <= 0 && data && data->parts) {
 			parts = data->parts;
--- a/drivers/mtd/devices/mtd_dataflash.c
+++ b/drivers/mtd/devices/mtd_dataflash.c
@@ -98,12 +98,6 @@ struct dataflash {
 	struct mtd_info		mtd;
 };
 
-#ifdef CONFIG_MTD_PARTITIONS
-#define	mtd_has_partitions()	(1)
-#else
-#define	mtd_has_partitions()	(0)
-#endif
-
 /* ......................................................................... */
 
 /*
@@ -682,11 +676,13 @@ add_dataflash_otp(struct spi_device *spi
 		struct mtd_partition	*parts;
 		int			nr_parts = 0;
 
-#ifdef CONFIG_MTD_CMDLINE_PARTS
-		static const char *part_probes[] = { "cmdlinepart", NULL, };
+		if (mtd_has_cmdlinepart()) {
+			static const char *part_probes[]
+					= { "cmdlinepart", NULL, };
 
-		nr_parts = parse_mtd_partitions(device, part_probes, &parts, 0);
-#endif
+			nr_parts = parse_mtd_partitions(device,
+					part_probes, &parts, 0);
+		}
 
 		if (nr_parts <= 0 && pdata && pdata->parts) {
 			parts = pdata->parts;
--- a/drivers/mtd/nand/davinci_nand.c
+++ b/drivers/mtd/nand/davinci_nand.c
@@ -39,19 +39,6 @@
 #include <asm/mach-types.h>
 
 
-#ifdef CONFIG_MTD_PARTITIONS
-static inline int mtd_has_partitions(void) { return 1; }
-#else
-static inline int mtd_has_partitions(void) { return 0; }
-#endif
-
-#ifdef CONFIG_MTD_CMDLINE_PARTS
-static inline int mtd_has_cmdlinepart(void) { return 1; }
-#else
-static inline int mtd_has_cmdlinepart(void) { return 0; }
-#endif
-
-
 /*
  * This is a device driver for the NAND flash controller found on the
  * various DaVinci family chips.  It handles up to four SoC chipselects,
--- a/include/linux/mtd/partitions.h
+++ b/include/linux/mtd/partitions.h
@@ -76,4 +76,16 @@ int __devinit of_mtd_parse_partitions(st
                                       struct device_node *node,
                                       struct mtd_partition **pparts);
 
+#ifdef CONFIG_MTD_PARTITIONS
+static inline int mtd_has_partitions(void) { return 1; }
+#else
+static inline int mtd_has_partitions(void) { return 0; }
+#endif
+
+#ifdef CONFIG_MTD_CMDLINE_PARTS
+static inline int mtd_has_cmdlinepart(void) { return 1; }
+#else
+static inline int mtd_has_cmdlinepart(void) { return 0; }
+#endif
+
 #endif

Re: [patch/RESEND 2.6.29-rc3-git] NAND: davinci_nand driver

[Felipe Balbi]

On Thu, Feb 26, 2009 at 03:15:21PM -0800, David Brownell wrote:
> (To the TI team reading this via the DaVinci list:  I think
> Andrew is hinting that a Signed-off-By from a TI person
> would be a Nice Thing.  Same for Dirk, and maybe others.)

If it's really necessary, feel free to add mine:

Signed-off-by: Felipe Balbi <me@felipebalbi.com>

-- 
balbi

Re: [patch/RESEND 2.6.29-rc3-git] NAND: davinci_nand driver

[Kevin Hilman]

David Brownell <david-b@pacbell.net> writes:

> On Thursday 26 February 2009, Andrew Morton wrote:
>
>> > + * Copyright (C) 2006 Texas Instruments.
>> > + *
>> > + * Ported to 2.6.23 Copyright (C) 2008 by
>> > + *   Sander Huijsen <Shuijsen@optelecom-nkf.com>
>> > + *   Troy Kisky <troy.kisky@boundarydevices.com>
>> > + *   Dirk Behme <Dirk.Behme@gmail.com>
>> 
>> hm.  What's the story with authorship, attributions and signoffs here?
>
> Written by TI (PSP team in India, ISTR) with no individual
> authorship credited, and shipped with a MontaVista 2.6.10
> kernel.  Ported as noted; I could presumably add my own
> copyright given recent updates I've made.  Likewise Felipe
> Balbi.  Kevin Hilman has signed off on various patches as
> part of merging them to the DaVinci tree.
>
> (To the TI team reading this via the DaVinci list:  I think
> Andrew is hinting that a Signed-off-By from a TI person
> would be a Nice Thing.  Same for Dirk, and maybe others.)
>
>
>> > ...
>> >
>> > +#ifdef CONFIG_MTD_PARTITIONS
>> > +static inline int mtd_has_partitions(void) { return 1; }
>> > +#else
>> > +static inline int mtd_has_partitions(void) { return 0; }
>> > +#endif
>> > +
>> > +#ifdef CONFIG_MTD_CMDLINE_PARTS
>> > +static inline int mtd_has_cmdlinepart(void) { return 1; }
>> > +#else
>> > +static inline int mtd_has_cmdlinepart(void) { return 0; }
>> > +#endif
>> 
>> These definitions shouldn't be buried in a .c file.
>
> I will send along a patch to move them to <linux/mtd/...> headers,
> now that there seems to be a bit of recognition that the current
> ifdef-centric approach in the MTD mapping drivers is trouble.  ;)
>
>
>> >
>> > ...
>> >
>> > +static void nand_davinci_hwctl_1bit(struct mtd_info *mtd, int mode)
>> > +{
>> > +	struct davinci_nand_info *info;
>> > +	u32 retval;
>> 
>> The identifier `retval' is usually used to identify the value which
>> this function will return.
>
> True; resolved in the appended fixup patch.
>
>
>> > +static int nand_davinci_calculate_1bit(struct mtd_info *mtd,
>> > +				      const u_char *dat, u_char *ecc_code)
>> > +{
>> > +	unsigned int ecc_val = nand_davinci_readecc_1bit(mtd);
>> > +	unsigned int tmp = (ecc_val & 0x0fff) | ((ecc_val & 0x0fff0000) >> 4);
>> 
>> argh.
>
> It seems the best-dressed pirates have parrots named "argh"!  ;)
>
>
>> > +	/* invert so that erased block ecc is correct */
>> > +	tmp = ~tmp;
>> > +	ecc_code[0] = (u_char)(tmp);
>> > +	ecc_code[1] = (u_char)(tmp >> 8);
>> > +	ecc_code[2] = (u_char)(tmp >> 16);
>> 
>> Is there some library facility which is being open-coded here?
>
> I don't know of such a facility:  24-bit integer into 3-byte buffer.
>
>
>> > +	return 0;
>> > +}
>> > +
>> >
>> > ...
>> >
>> > +static int __init nand_davinci_probe(struct platform_device *pdev)
>> > +{
>> > +		... deletia ...
>> > +
>> > +	info = kzalloc(sizeof(*info), GFP_KERNEL);
>> > +	if (!info) {
>> > +		dev_err(&pdev->dev, "unable to allocate memory\n");
>> > +		ret = -ENOMEM;
>> > +		goto err_nomem;
>> > +	}
>> > +
>> > +	platform_set_drvdata(pdev, info);
>> > +
>> > +	res1 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> > +	res2 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> > +	if (!res1 || !res2) {
>> > +		dev_err(&pdev->dev, "resource missing\n");
>> > +		ret = -EINVAL;
>> > +		goto err_res;
>> 
>> This leaks `info'.
>> 
>> Please check all the error path unwinding here.
>
> OK -- that does look buggish.
>
> (Kevin -- I suggest you merge this to the DaVinci tree to
> make the eventual resync-with-mainline easier.)
>

Done.

It also needs this cleanup bit below which is in DaVinci git now that
we've deprecated the use of the davinci cpu_is_* macros in drivers.

This could be just folded into current patch if desired.

Thanks,

Kevin


commit 1bacc33ccc9bd0f3c109bf8a8550e9b6f99397bd
Author: Kevin Hilman <khilman@deeprootsystems.com>
Date:   Thu Feb 26 17:15:18 2009 -0800

    MTD: NAND: drop usage of cpu_is_* macro
    
    Usage of davinci-specific cpu_is macros is not allowed in drivers.
    These options should be passed in through platform_data.
    
    Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>

diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
index aa70b4e..a2f78ad 100644
--- a/drivers/mtd/nand/davinci_nand.c
+++ b/drivers/mtd/nand/davinci_nand.c
@@ -33,7 +33,6 @@
 #include <linux/mtd/nand.h>
 #include <linux/mtd/partitions.h>
 
-#include <mach/cpu.h>
 #include <mach/nand.h>
 
 #include <asm/mach-types.h>
@@ -392,8 +391,6 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
 	/* use board-specific ECC config; else, the best available */
 	if (pdata)
 		ecc_mode = pdata->ecc_mode;
-	else if (cpu_is_davinci_dm355())
-		ecc_mode = NAND_ECC_HW_SYNDROME;
 	else
 		ecc_mode = NAND_ECC_HW;
 

RE: [patch/RESEND 2.6.29-rc3-git] NAND: davinci_nand driver

[Rajashekhara, Sudhakar]

I and one of my colleague Vinod, who is no more with TI, worked on this driver. I can sign-off this driver.

Signed-off-by: Sudhakar Rajashekhara <sudhakar.raj@ti.com>

Regards, Sudhakar
> 
> On Thu, Feb 26, 2009 at 03:15:21PM -0800, David Brownell wrote:
> > (To the TI team reading this via the DaVinci list:  I think Andrew 
> > is hinting that a Signed-off-By from a TI person would be a Nice 
> > Thing.  Same for Dirk, and maybe others.)
> 
> If it's really necessary, feel free to add mine:
> 
> Signed-off-by: Felipe Balbi <me@felipebalbi.com>
> 
> --
> balbi
> 
> _______________________________________________
> Davinci-linux-open-source mailing list 
> Davinci-linux-open-source@linux.davincidsp.com
> http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source