Skip to contentSkip to conversation

nand: Add NAND driver for Mikrotik RB4xx series boards

PATCH In review Bert Vermeulen 2 participants 4 messages May 15, 2015 – Aug 20, 2015 2 messages not in the archive

Subsystems (5)
lore
Share & tools
lore
#0 lore raw

PATCH v3 1/1 nand: Add NAND driver for Mikrotik RB4xx series boards

The NAND chip containing the root filesystem is behind an SPI-connected CPLD. This driver uses the spi-rb4xx driver to communicate with the CPLD.

The CPLD also acts as a GPIO expander: the ALE/CLE/NCE pins are set via CPLD commands. Some LEDs on the board are also accessed this way.

---
Changes in v3:
- Add link to parent device
Changes in v2:
- Fix Kconfig dependencies and description
- Move NAND partitions into platform data
- Use gpiod API with lookup table, and devm_ as needed
- Code cleanup

 arch/mips/include/asm/mach-ath79/rb4xx.h |  44 +++
 drivers/mtd/nand/Kconfig                 |   7 +
 drivers/mtd/nand/Makefile                |   1 +
 drivers/mtd/nand/rb4xx_nand.c            | 484 +++++++++++++++++++++++++++++++
 4 files changed, 536 insertions(+)
 create mode 100644 arch/mips/include/asm/mach-ath79/rb4xx.h
 create mode 100644 drivers/mtd/nand/rb4xx_nand.c
arch/mips/include/asm/mach-ath79/rb4xx.h new file +44 -0
new file mode 100644
index 0000000..667ea748
@@ -0,0 +1,44 @@
+/*
+ * SPI driver definitions for the CPLD chip on the Mikrotik RB4xx boards
+ *
+ * Copyright (C) 2010 Gabor Juhos <[email protected]>
+ *
+ * This file was based on the patches for Linux 2.6.27.39 published by
+ * MikroTik for their RouterBoard 4xx series devices.
+ *
+ * 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.
+ */
+
+#include <linux/spi/flash.h>
+
+#define RB4XX_GPIO_CPLD_BASE	32
+
+#define CPLD_GPIO_LED1		0
+#define CPLD_GPIO_LED2		2
+#define CPLD_GPIO_LED3		1
+#define CPLD_GPIO_LED4		3
+#define CPLD_GPIO_FAN		4
+#define CPLD_GPIO_ALE		5
+#define CPLD_GPIO_CLE		6
+#define CPLD_GPIO_NCE		7
+#define CPLD_GPIO_LED5		8
+
+#define CPLD_NUM_GPIOS		9
+
+#define CPLD_CFG_LED1		BIT(CPLD_GPIO_LED1)
+#define CPLD_CFG_LED2		BIT(CPLD_GPIO_LED2)
+#define CPLD_CFG_LED3		BIT(CPLD_GPIO_LED3)
+#define CPLD_CFG_LED4		BIT(CPLD_GPIO_LED4)
+#define CPLD_CFG_FAN		BIT(CPLD_GPIO_FAN)
+#define CPLD_CFG_ALE		BIT(CPLD_GPIO_ALE)
+#define CPLD_CFG_CLE		BIT(CPLD_GPIO_CLE)
+#define CPLD_CFG_NCE		BIT(CPLD_GPIO_NCE)
+#define CPLD_CFG_LED5		BIT(CPLD_GPIO_LED5)
+
+struct rb4xx_cpld_platform_data {
+	unsigned gpio_base;
+	struct flash_platform_data flash_data;
+};
+
drivers/mtd/nand/Kconfig +7 -0
index 5897d8d..873639f 100644
@@ -530,4 +530,11 @@ config MTD_NAND_HISI504
 	help
 	  Enables support for NAND controller on Hisilicon SoC Hip04.
 
+config MTD_NAND_RB4XX
+	tristate "NAND flash driver for Mikrotik RB4xx series boards"
+	depends on MTD_NAND && SPI_RB4XX
+	help
+	  Enables support for the NAND flash chip on Mikrotik Routerboard
+	  RB4xx series.
+
 endif # MTD_NAND
drivers/mtd/nand/Makefile +1 -0
index 582bbd05..56dbd08 100644
@@ -52,5 +52,6 @@ obj-$(CONFIG_MTD_NAND_XWAY)		+= xway_nand.o
 obj-$(CONFIG_MTD_NAND_BCM47XXNFLASH)	+= bcm47xxnflash/
 obj-$(CONFIG_MTD_NAND_SUNXI)		+= sunxi_nand.o
 obj-$(CONFIG_MTD_NAND_HISI504)	        += hisi504_nand.o
+obj-$(CONFIG_MTD_NAND_RB4XX)	        += rb4xx_nand.o
 
 nand-objs := nand_base.o nand_bbt.o nand_timings.o
drivers/mtd/nand/rb4xx_nand.c new file +484 -0

484 lines not on this page · raw message

Signature · 1 line
1.9.1

A message that is not in the archive (sent off-list or not yet received): 1431689631-7217-1-git-send-email-bert-Nh2F35Ii2RQ@public.gmane.org

#2 lore raw

+ linux-spi, linux-gpio

Hi Bert,

Quoted 7 lines · Bert Vermeulen

The NAND chip containing the root filesystem is behind an SPI-connected CPLD. This driver uses the spi-rb4xx driver to communicate with the CPLD.

The CPLD also acts as a GPIO expander: the ALE/CLE/NCE pins are set via CPLD commands. Some LEDs on the board are also accessed this way.

Quoted 8 lines

---
Changes in v3:
- Add link to parent device
Changes in v2:
- Fix Kconfig dependencies and description
- Move NAND partitions into platform data
- Use gpiod API with lookup table, and devm_ as needed
- Code cleanup

Sorry for the late review. A few small comments.

Quoted 43 lines
 arch/mips/include/asm/mach-ath79/rb4xx.h |  44 +++
 drivers/mtd/nand/Kconfig                 |   7 +
 drivers/mtd/nand/Makefile                |   1 +
 drivers/mtd/nand/rb4xx_nand.c            | 484 +++++++++++++++++++++++++++++++
 4 files changed, 536 insertions(+)
 create mode 100644 arch/mips/include/asm/mach-ath79/rb4xx.h
 create mode 100644 drivers/mtd/nand/rb4xx_nand.c
arch/mips/include/asm/mach-ath79/rb4xx.h new file +28 -0
new file mode 100644
index 0000000..667ea748
@@ -0,0 +1,44 @@
+/*
+ * SPI driver definitions for the CPLD chip on the Mikrotik RB4xx boards
+ *
+ * Copyright (C) 2010 Gabor Juhos <[email protected]>
+ *
+ * This file was based on the patches for Linux 2.6.27.39 published by
+ * MikroTik for their RouterBoard 4xx series devices.
+ *
+ * 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.
+ */
+
+#include <linux/spi/flash.h>
+
+#define RB4XX_GPIO_CPLD_BASE	32
+
+#define CPLD_GPIO_LED1		0
+#define CPLD_GPIO_LED2		2
+#define CPLD_GPIO_LED3		1
+#define CPLD_GPIO_LED4		3
+#define CPLD_GPIO_FAN		4
+#define CPLD_GPIO_ALE		5
+#define CPLD_GPIO_CLE		6
+#define CPLD_GPIO_NCE		7
+#define CPLD_GPIO_LED5		8
+
+#define CPLD_NUM_GPIOS		9

arch/mips/include/asm/mach-ath79/rb4xx.h line 38

Quoted 10 lines
arch/mips/include/asm/mach-ath79/rb4xx.h new file +10 -0
+
+#define CPLD_CFG_LED1		BIT(CPLD_GPIO_LED1)
+#define CPLD_CFG_LED2		BIT(CPLD_GPIO_LED2)
+#define CPLD_CFG_LED3		BIT(CPLD_GPIO_LED3)
+#define CPLD_CFG_LED4		BIT(CPLD_GPIO_LED4)
+#define CPLD_CFG_FAN		BIT(CPLD_GPIO_FAN)
+#define CPLD_CFG_ALE		BIT(CPLD_GPIO_ALE)
+#define CPLD_CFG_CLE		BIT(CPLD_GPIO_CLE)
+#define CPLD_CFG_NCE		BIT(CPLD_GPIO_NCE)
+#define CPLD_CFG_LED5		BIT(CPLD_GPIO_LED5)

Are the above definitions all really shared with others? I'd really prefer hardware definitions like this stay local to the driver, if possible.

arch/mips/include/asm/mach-ath79/rb4xx.h line 41

Quoted 3 lines
arch/mips/include/asm/mach-ath79/rb4xx.h new file +3 -0
+
+struct rb4xx_cpld_platform_data {
+	unsigned gpio_base;

^^ This looks like you're trying to support a deprecated feature. I see this comment on gpio_chip::base:

@base: identifies the first GPIO number handled by this chip; or, if negative during registration, requests dynamic ID allocation.
DEPRECATION: providing anything non-negative and nailing the base base offset of GPIO chips is deprecated. Please pass -1 as base to let gpiolib select the chip base in all possible cases. We want to get rid of the static GPIO number space in the long run.

So you probably need to kill the gpio_base code.

arch/mips/include/asm/mach-ath79/rb4xx.h line 42

Quoted 1 line
arch/mips/include/asm/mach-ath79/rb4xx.h new file +1 -0
+	struct flash_platform_data flash_data;

Do you really need this platform data? This helps to make the driver a little less portable. And if you really need to configure a few things (e.g., partitions), you can switch to device tree instead.

arch/mips/include/asm/mach-ath79/rb4xx.h line 44

Quoted 2 lines
arch/mips/include/asm/mach-ath79/rb4xx.h new file +2 -0
+};
+

^^^ git-am warns: new blank line at EOF.

drivers/mtd/nand/Kconfig config MTD_NAND_HISI504 · line 535

Quoted 11 lines
drivers/mtd/nand/Kconfig +3 -0
index 5897d8d..873639f 100644
@@ -530,4 +530,11 @@ config MTD_NAND_HISI504
 	help
 	  Enables support for NAND controller on Hisilicon SoC Hip04.
 
+config MTD_NAND_RB4XX
+	tristate "NAND flash driver for Mikrotik RB4xx series boards"
+	depends on MTD_NAND && SPI_RB4XX

Is this really dependent on the SPI driver? At least for the sake of compile-testing, I think that's not true, and even more so if you kill off the platform header / platform data.

And the MTD_NAND dependency is redundant, as the config entry is already enclosed within the 'if MTD_NAND' block.

So maybe this?

	depends on SPI_RB4XX || COMPILE_TEST
Quoted 363 lines
drivers/mtd/nand/Kconfig +4 -0
+	help
+	  Enables support for the NAND flash chip on Mikrotik Routerboard
+	  RB4xx series.
+
 endif # MTD_NAND
drivers/mtd/nand/Makefile +1 -0
index 582bbd05..56dbd08 100644
@@ -52,5 +52,6 @@ obj-$(CONFIG_MTD_NAND_XWAY)		+= xway_nand.o
 obj-$(CONFIG_MTD_NAND_BCM47XXNFLASH)	+= bcm47xxnflash/
 obj-$(CONFIG_MTD_NAND_SUNXI)		+= sunxi_nand.o
 obj-$(CONFIG_MTD_NAND_HISI504)	        += hisi504_nand.o
+obj-$(CONFIG_MTD_NAND_RB4XX)	        += rb4xx_nand.o
 
 nand-objs := nand_base.o nand_bbt.o nand_timings.o
drivers/mtd/nand/rb4xx_nand.c new file +341 -0
new file mode 100644
index 0000000..842c848
@@ -0,0 +1,484 @@
+/*
+ * NAND flash driver for the MikroTik RouterBoard 4xx series
+ *
+ * Copyright (C) 2008-2011 Gabor Juhos <[email protected]>
+ * Copyright (C) 2008 Imre Kaloz <[email protected]>
+ * Copyright (C) 2015 Bert Vermeulen <[email protected]>
+ *
+ * This file was based on the driver for Linux 2.6.22 published by
+ * MikroTik for their RouterBoard 4xx series devices.
+ *
+ * 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.
+ */
+
+#include <linux/mtd/nand.h>
+#include <linux/spi/spi.h>
+#include <linux/platform_device.h>
+#include <linux/gpio/driver.h>
+#include <linux/gpio/consumer.h>
+
+#include <asm/mach-ath79/rb4xx.h>
+
+#define CPLD_CMD_WRITE_NAND	0x08
+#define CPLD_CMD_WRITE_CFG	0x09
+#define CPLD_CMD_READ_NAND	0x0a
+#define CPLD_CMD_READ_FAST	0x0b
+#define CPLD_CMD_LED5_ON	0x0c
+#define CPLD_CMD_LED5_OFF	0x0d
+
+struct rb4xx_nand_info {
+	struct nand_chip chip;
+	struct mtd_info mtd;
+	struct spi_device *spi_dev;
+	struct gpio_desc *gpio_rdy;
+	struct gpio_desc *gpio_ale;
+	struct gpio_desc *gpio_cle;
+	struct gpio_desc *gpio_nce;
+};
+
+struct rb4xx_cpld {
+	struct spi_device *spi;
+	struct mutex lock;
+	struct gpio_chip chip;
+	unsigned int config;
+};
+
+/*
+ * We need to use the OLD Yaffs-1 OOB layout, otherwise the RB bootloader
+ * will not be able to find the kernel that we load.
+ */
+static struct nand_ecclayout rb4xx_nand_ecclayout = {
+	.eccbytes = 6,
+	.eccpos = { 8, 9, 10, 13, 14, 15 },
+	.oobavail = 9,
+	.oobfree = { { 0, 4 }, { 6, 2 }, { 11, 2 }, { 4, 1 } }
+};
+
+static inline struct rb4xx_cpld *gpio_to_cpld(struct gpio_chip *chip)
+{
+	return container_of(chip, struct rb4xx_cpld, chip);
+}
+
+static int rb4xx_cpld_write_cmd(struct rb4xx_cpld *cpld, unsigned char cmd)
+{
+	struct spi_transfer t;
+	struct spi_message m;
+
+	spi_message_init(&m);
+
+	memset(&t, 0, sizeof(t));
+	t.tx_buf = &cmd;
+	t.len = sizeof(cmd);
+	spi_message_add_tail(&t, &m);
+	return spi_sync(cpld->spi, &m);
+}
+
+static int rb4xx_cpld_write_cfg(struct rb4xx_cpld *cpld, unsigned char config)
+{
+	struct spi_transfer t;
+	struct spi_message m;
+	unsigned char cmd[2] = {
+		CPLD_CMD_WRITE_CFG, config
+	};
+
+	spi_message_init(&m);
+
+	memset(&t, 0, sizeof(t));
+	t.tx_buf = cmd;
+	t.len = sizeof(cmd);
+	spi_message_add_tail(&t, &m);
+	return spi_sync(cpld->spi, &m);
+}
+
+static int rb4xx_cpld_change_cfg(struct rb4xx_cpld *cpld, u32 mask, u32 value)
+{
+	unsigned int config;
+	int ret;
+
+	config = cpld->config & ~mask;
+	config |= value;
+
+	if (mask & 0xff) {
+		ret = rb4xx_cpld_write_cfg(cpld, config);
+		if (ret)
+			return ret;
+	}
+
+	if (mask & CPLD_CFG_LED5) {
+		if (value & CPLD_CFG_LED5)
+			ret = rb4xx_cpld_write_cmd(cpld, CPLD_CMD_LED5_ON);
+		else
+			ret = rb4xx_cpld_write_cmd(cpld, CPLD_CMD_LED5_OFF);
+		if (ret)
+			return ret;
+	}
+
+	cpld->config = config;
+
+	return 0;
+}
+
+static int rb4xx_cpld_gpio_get(struct gpio_chip *chip, unsigned offset)
+{
+	struct rb4xx_cpld *cpld = gpio_to_cpld(chip);
+	int value;
+
+	mutex_lock(&cpld->lock);
+	value = (cpld->config >> offset) & 1;
+	mutex_unlock(&cpld->lock);
+
+	return value;
+}
+
+static void rb4xx_cpld_gpio_set(struct gpio_chip *chip,
+				unsigned offset, int value)
+{
+	struct rb4xx_cpld *cpld = gpio_to_cpld(chip);
+
+	mutex_lock(&cpld->lock);
+	rb4xx_cpld_change_cfg(cpld, (1 << offset), !!value << offset);
+	mutex_unlock(&cpld->lock);
+}
+
+static int rb4xx_cpld_gpio_direction_input(struct gpio_chip *chip,
+					   unsigned offset)
+{
+	return -EOPNOTSUPP;
+}
+
+static int rb4xx_cpld_gpio_direction_output(struct gpio_chip *chip,
+					    unsigned offset, int value)
+{
+	struct rb4xx_cpld *cpld = gpio_to_cpld(chip);
+	int ret;
+
+	mutex_lock(&cpld->lock);
+	ret = rb4xx_cpld_change_cfg(cpld, (1 << offset), !!value << offset);
+	mutex_unlock(&cpld->lock);
+
+	return ret;
+}
+
+static int rb4xx_cpld_read(struct spi_device *spi, unsigned char *rx_buf,
+			   unsigned len)
+{
+	struct spi_message m;
+	static const unsigned char cmd[2] = {
+		CPLD_CMD_READ_NAND, 0
+	};
+	struct spi_transfer t[2] = {
+		{
+			.tx_buf = &cmd,
+			.len = sizeof(cmd),
+		}, {
+			.rx_buf = rx_buf,
+			.len = len,
+		},
+	};
+
+	spi_message_init(&m);
+	spi_message_add_tail(&t[0], &m);
+	spi_message_add_tail(&t[1], &m);
+	return spi_sync(spi, &m);
+}
+
+static int rb4xx_cpld_write(struct spi_device *spi, const u8 *buf, int len)
+{
+	struct spi_message m;
+	static const unsigned char cmd = CPLD_CMD_WRITE_NAND;
+	struct spi_transfer t[3] = {
+		{
+			.tx_buf = &cmd,
+			.len = sizeof(cmd),
+		}, {
+			.tx_buf = buf,
+			.len = len,
+			.tx_nbits = SPI_NBITS_DUAL,
+		}, {
+			.len = 1,
+			.tx_nbits = SPI_NBITS_DUAL,
+		},
+	};
+
+	spi_message_init(&m);
+	spi_message_add_tail(&t[0], &m);
+	spi_message_add_tail(&t[1], &m);
+	spi_message_add_tail(&t[2], &m);
+	return spi_sync(spi, &m);
+}
+
+static int rb4xx_nand_dev_ready(struct mtd_info *mtd)
+{
+	struct nand_chip *chip = mtd->priv;
+	struct rb4xx_nand_info *info = chip->priv;
+
+	return gpiod_get_value_cansleep(info->gpio_rdy);
+}
+
+static void rb4xx_nand_cmd_ctrl(struct mtd_info *mtd, int cmd,
+				unsigned int ctrl)
+{
+	struct nand_chip *chip = mtd->priv;
+	struct rb4xx_nand_info *info = chip->priv;
+	u8 data = cmd;
+	int ret;
+
+	if (ctrl & NAND_CTRL_CHANGE) {
+		gpiod_set_value_cansleep(info->gpio_cle,
+					(ctrl & NAND_CLE) ? 1 : 0);
+		gpiod_set_value_cansleep(info->gpio_ale,
+					(ctrl & NAND_ALE) ? 1 : 0);
+		gpiod_set_value_cansleep(info->gpio_nce,
+					(ctrl & NAND_NCE) ? 0 : 1);
+	}
+
+	if (cmd != NAND_CMD_NONE) {
+		ret = rb4xx_cpld_write(info->spi_dev, &data, 1);
+		if (ret)
+			pr_err("rb4xx_nand: write cmd failed, error %d\n", ret);
+	}
+}
+
+static unsigned char rb4xx_nand_read_byte(struct mtd_info *mtd)
+{
+	struct nand_chip *chip = mtd->priv;
+	struct rb4xx_nand_info *info = chip->priv;
+	unsigned char data;
+	int ret;
+
+	ret = rb4xx_cpld_read(info->spi_dev, &data, 1);
+	if (ret) {
+		pr_err("rb4xx_nand: read data failed, error %d\n", ret);
+		return 0xff;
+	}
+
+	return data;
+}
+
+static void rb4xx_nand_write_buf(struct mtd_info *mtd, const unsigned char *buf,
+				 int len)
+{
+	struct nand_chip *chip = mtd->priv;
+	struct rb4xx_nand_info *info = chip->priv;
+	int ret;
+
+	ret = rb4xx_cpld_write(info->spi_dev, buf, len);
+	if (ret)
+		pr_err("rb4xx_nand: write buf failed, error %d\n", ret);
+}
+
+static void rb4xx_nand_read_buf(struct mtd_info *mtd, unsigned char *buf,
+				int len)
+{
+	struct nand_chip *chip = mtd->priv;
+	struct rb4xx_nand_info *info = chip->priv;
+	int ret;
+
+	ret = rb4xx_cpld_read(info->spi_dev, buf, len);
+	if (ret)
+		pr_err("rb4xx_nand: read buf failed, error %d\n", ret);
+}
+
+static int rb4xx_cpld_gpio_init(struct rb4xx_cpld *cpld, unsigned int base)
+{
+	int ret;
+
+	cpld->chip.label = "rb4xx-cpld";
+	cpld->chip.base = base;
+	cpld->chip.ngpio = CPLD_NUM_GPIOS;
+	cpld->chip.can_sleep = 1;
+	cpld->chip.dev = &cpld->spi->dev;
+	cpld->chip.get = rb4xx_cpld_gpio_get;
+	cpld->chip.set = rb4xx_cpld_gpio_set;
+	cpld->chip.direction_input = rb4xx_cpld_gpio_direction_input;
+	cpld->chip.direction_output = rb4xx_cpld_gpio_direction_output;
+
+	ret = gpiochip_add(&cpld->chip);
+	if (ret)
+		dev_err(&cpld->spi->dev, "adding GPIO chip failed, error %d\n",
+			ret);
+
+	return ret;
+}
+
+static int rb4xx_cpld_probe(struct spi_device *spi)
+{
+	struct rb4xx_cpld *cpld;
+	struct rb4xx_cpld_platform_data *pdata;
+	int ret;
+
+	pdata = dev_get_platdata(&spi->dev);
+	if (!pdata)
+		return -ENODATA;
+
+	cpld = devm_kzalloc(&spi->dev, sizeof(*cpld), GFP_KERNEL);
+	if (!cpld)
+		return -ENOMEM;
+
+	mutex_init(&cpld->lock);
+	cpld->spi = spi_dev_get(spi);
+	dev_set_drvdata(&spi->dev, cpld);
+
+	ret = spi_setup(spi);
+	if (ret) {
+		dev_err(&spi->dev, "SPI setup failed, error %d\n", ret);
+		return ret;
+	}
+
+	return rb4xx_cpld_gpio_init(cpld, pdata->gpio_base);
+}
+
+static int rb4xx_cpld_remove(struct spi_device *spi)
+{
+	struct rb4xx_cpld *cpld;
+
+	cpld = dev_get_drvdata(&spi->dev);
+	gpiochip_remove(&cpld->chip);
+	mutex_destroy(&cpld->lock);
+
+	return 0;

drivers/mtd/nand/rb4xx_nand.c line 351

Quoted 10 lines
drivers/mtd/nand/rb4xx_nand.c new file +10 -0
+}
+
+static struct spi_driver rb4xx_cpld_driver = {
+	.probe = rb4xx_cpld_probe,
+	.remove = rb4xx_cpld_remove,
+	.driver = {
+		.name = "rb4xx-cpld",
+		.owner = THIS_MODULE,
+	},
+};

I feel some deja vu here (did I make this comment before?); why do you need to embed both a SPI and a platform driver here? It appears that the NAND controller actually sits on the SPI bus, so this really is not a platform driver; it's a SPI driver. Can you just kill off all the platform_driver stuff and merge the probe functions?

Or if I'm missing something big, please do enlighten.

drivers/mtd/nand/rb4xx_nand.c line 364

Quoted 13 lines
drivers/mtd/nand/rb4xx_nand.c new file +13 -0
+
+static int rb4xx_nand_probe(struct platform_device *pdev)
+{
+	struct device *spi_dev;
+	struct rb4xx_nand_info	*info;
+	struct rb4xx_cpld_platform_data *pdata;
+	int ret;
+
+	/*
+	 * NAND chip is behind the CPLD on SPI bus 0 CS 1. Allow this
+	 * to silently fail the second time around.
+	 */
+	spi_register_driver(&rb4xx_cpld_driver);

If you can do the above, then this "silently fails" stuff goes away.

drivers/mtd/nand/rb4xx_nand.c line 370

Quoted 6 lines
drivers/mtd/nand/rb4xx_nand.c new file +6 -0
+
+	/*
+	 * Getting a handle on the SPI device above will fail the first time,
+	 * but should succeed whenever that driver gets probed.
+	 */
+	spi_dev = bus_find_device_by_name(&spi_bus_type, NULL, "spi0.1");

This is really fragile, as you're using platform knowledge and naming expectations as your only way to connect the NAND and SPI "devices". If you merge the two drivers, you don't need this.

If merging the devices doesn't work, then you might either try something with device tree + phandles to get the reference, or else you can just pass pointers to the struct spi_device via platform data, rather than trying to infer the relationship.

Quoted 104 lines
drivers/mtd/nand/rb4xx_nand.c new file +104 -0
+	if (!spi_dev)
+		return -EPROBE_DEFER;
+
+	pdata = dev_get_platdata(spi_dev);
+	if (!pdata)
+		return -ENODATA;
+
+	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	info->gpio_ale = devm_gpiod_get(&pdev->dev, "NAND ALE", GPIOD_OUT_LOW);
+	if (IS_ERR(info->gpio_ale)) {
+		dev_err(&pdev->dev, "unable to request gpio NAND ALE: %ld\n",
+			PTR_ERR(info->gpio_ale));
+		return -ENOENT;
+	}
+
+	info->gpio_cle = devm_gpiod_get(&pdev->dev, "NAND CLE", GPIOD_OUT_LOW);
+	if (IS_ERR(info->gpio_cle)) {
+		dev_err(&pdev->dev, "unable to request gpio NAND CLE: %ld\n",
+			PTR_ERR(info->gpio_cle));
+		return -ENOENT;
+	}
+
+	info->gpio_nce = devm_gpiod_get(&pdev->dev, "NAND NCE", GPIOD_OUT_LOW);
+	if (IS_ERR(info->gpio_nce)) {
+		dev_err(&pdev->dev, "unable to request gpio NAND NCE: %ld\n",
+			PTR_ERR(info->gpio_nce));
+		return -ENOENT;
+	}
+
+	/* Platform GPIO, has no device to manage it. */
+	info->gpio_rdy = gpiod_get(NULL, "NAND RDY", GPIOD_IN);
+	if (IS_ERR(info->gpio_rdy)) {
+		dev_err(&pdev->dev, "unable to request gpio RDY: %ld\n",
+			PTR_ERR(info->gpio_rdy));
+		return -ENOENT;
+	}
+
+	info->spi_dev = container_of(spi_dev, struct spi_device, dev);
+
+	info->chip.priv	= info;
+	info->mtd.priv	= &info->chip;
+	info->mtd.owner	= THIS_MODULE;
+	info->mtd.dev.parent = &pdev->dev;
+
+	info->chip.cmd_ctrl	= rb4xx_nand_cmd_ctrl;
+	info->chip.dev_ready	= rb4xx_nand_dev_ready;
+	info->chip.read_byte	= rb4xx_nand_read_byte;
+	info->chip.write_buf	= rb4xx_nand_write_buf;
+	info->chip.read_buf	= rb4xx_nand_read_buf;
+
+	info->chip.chip_delay	= 25;
+	info->chip.ecc.mode	= NAND_ECC_SOFT;
+
+	platform_set_drvdata(pdev, info);
+
+	ret = nand_scan_ident(&info->mtd, 1, NULL);
+	if (ret) {
+		ret = -ENXIO;
+		goto err_gpio;
+	}
+
+	if (info->mtd.writesize == 512)
+		info->chip.ecc.layout = &rb4xx_nand_ecclayout;
+
+	ret = nand_scan_tail(&info->mtd);
+	if (ret) {
+		ret = -ENXIO;
+		goto err_gpio;
+	}
+
+	ret = mtd_device_register(&info->mtd, pdata->flash_data.parts,
+				  pdata->flash_data.nr_parts);
+	if (ret)
+		goto err_release_nand;
+
+	return 0;
+
+err_release_nand:
+	nand_release(&info->mtd);
+err_gpio:
+	gpiod_put(info->gpio_rdy);
+
+	return ret;
+}
+
+static int rb4xx_nand_remove(struct platform_device *pdev)
+{
+	struct rb4xx_nand_info *info = platform_get_drvdata(pdev);
+
+	nand_release(&info->mtd);
+	gpiod_put(info->gpio_rdy);
+	spi_unregister_driver(&rb4xx_cpld_driver);
+
+	return 0;
+}
+
+static struct platform_driver rb4xx_nand_driver = {
+	.probe = rb4xx_nand_probe,
+	.remove = rb4xx_nand_remove,
+	.driver = {
+		.name = "rb4xx-nand",

drivers/mtd/nand/rb4xx_nand.c line 484

Quoted 10 lines
drivers/mtd/nand/rb4xx_nand.c new file +10 -0
+	},
+};
+
+module_platform_driver(rb4xx_nand_driver);
+
+MODULE_DESCRIPTION("NAND flash driver for RouterBoard 4xx series");
+MODULE_AUTHOR("Gabor Juhos <[email protected]>");
+MODULE_AUTHOR("Imre Kaloz <[email protected]>");
+MODULE_AUTHOR("Bert Vermeulen <[email protected]>");
+MODULE_LICENSE("GPL v2");

Thanks,
Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#3 lore raw
Quoted 15 lines · Brian Norris

drivers/mtd/nand/rb4xx_nand.c line 351

Quoted 8 lines
drivers/mtd/nand/rb4xx_nand.c new file +8 -0
+static struct spi_driver rb4xx_cpld_driver = {
+	.probe = rb4xx_cpld_probe,
+	.remove = rb4xx_cpld_remove,
+	.driver = {
+		.name = "rb4xx-cpld",
+		.owner = THIS_MODULE,
+	},
+};

I feel some deja vu here (did I make this comment before?); why do you need to embed both a SPI and a platform driver here? It appears that the NAND controller actually sits on the SPI bus, so this really is not a platform driver; it's a SPI driver. Can you just kill off all the platform_driver stuff and merge the probe functions?

Quoted 1 line

Or if I'm missing something big, please do enlighten.

This driver was specifically refused by the SPI subsystem maintainer:

http://www.linux-mips.org/archives/linux-mips/2015-03/msg00422.html

He suggested the MFD subsystem, where it was also refused:

http://lkml.iu.edu/hypermail/linux/kernel/1504.0/03162.html

Andy Shevchenko suggested drivers/misc, so I mailed Arnd Bergmann and asked if he would accept it there.

Arnd took a good look at the driver and suggested the MTD/NAND subsystem. This is after all a driver for a NAND chip controller. The controller just happens to reside in a CPLD with custom firmware, which is controlled via SPI and has some GPIO controller functionality thrown in.

I realize it's an odd hardware setup, and I can't fix that, but this really does seem to be the best place for it.

I can deal with your other comments on the patch, but I'd really like some agreement on where this driver will be accepted first. I've come full circle with getting referred back to the SPI subsystem.

Signature · 5 lines
Bert Vermeulen        [email protected]          email/xmpp

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

A message that is not in the archive (sent off-list or not yet received): [email protected]

#5 lore raw
Quoted 17 lines · Bert Vermeulen
Quoted 15 lines · Brian Norris

drivers/mtd/nand/rb4xx_nand.c line 351

Quoted 8 lines
drivers/mtd/nand/rb4xx_nand.c new file +8 -0
+static struct spi_driver rb4xx_cpld_driver = {
+	.probe = rb4xx_cpld_probe,
+	.remove = rb4xx_cpld_remove,
+	.driver = {
+		.name = "rb4xx-cpld",
+		.owner = THIS_MODULE,
+	},
+};

I feel some deja vu here (did I make this comment before?); why do you need to embed both a SPI and a platform driver here? It appears that the NAND controller actually sits on the SPI bus, so this really is not a platform driver; it's a SPI driver. Can you just kill off all the platform_driver stuff and merge the probe functions?

Quoted 5 lines
Quoted 1 line

Or if I'm missing something big, please do enlighten.

This driver was specifically refused by the SPI subsystem maintainer:

http://www.linux-mips.org/archives/linux-mips/2015-03/msg00422.html

We're speaking past each other here a bit: I was recommending a SPI *device* driver, not a host controller driver [1]. (This device is definitely not a SPI controller!)

Quoted 3 lines

He suggested the MFD subsystem, where it was also refused:

http://lkml.iu.edu/hypermail/linux/kernel/1504.0/03162.html

I honestly can't straighten out what the definition of an MFD is, so I can't blame you for getting that one "wrong."

Quoted 7 lines

Andy Shevchenko suggested drivers/misc, so I mailed Arnd Bergmann and asked if he would accept it there.

Arnd took a good look at the driver and suggested the MTD/NAND subsystem. This is after all a driver for a NAND chip controller. The controller just happens to reside in a CPLD with custom firmware, which is controlled via SPI and has some GPIO controller functionality thrown in.

I think Arnd was right, from the looks of it. I don't mind the GPIO controller functionality thrown in, as long as someone from linux-gpio can review that part. I just minded the structure of the driver itself, in which you register both a platfrom and a spi driver (spi_register_driver() and module_platform_driver()).

AFAICT, the driver structure and your above comment suggest that the entire device essentially sits on the SPI bus (not the platform bus), so it seems like it can all be initialized behind module_spi_driver().

Quoted 6 lines

I realize it's an odd hardware setup, and I can't fix that, but this really does seem to be the best place for it.

I can deal with your other comments on the patch, but I'd really like some agreement on where this driver will be accepted first. I've come full circle with getting referred back to the SPI subsystem.

Sorry again for the confusion. To be clear, I'm *not* suggesting sending this back to the SPI subsystem.

Brian

[1] I guess the SPI documentation (Documentation/spi/spi-summary) calls these "controller" vs. "protocol" drivers; your driver is a "protocol" driver and can find a home wherever that protocol makes sense. In this case, you're mostly doing NAND flash, therefore it goes in drivers/mtd/.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html