mtd: nand: add Broadcom NAND controller support

These are already-documented common bindings for NAND chips. Let's handle them in nand_base.

If NAND controller drivers need to act on this data before bringing up the NAND chip (e.g., fill out ECC callback functions, change HW modes, etc.), then they can do so between calling nand_scan_ident() and nand_scan_tail().

Signed-off-by Brian Norris <[email protected]>

---
 drivers/mtd/nand/nand_base.c | 41 +++++++++++++++++++++++++++++++++++++++++
 include/linux/mtd/nand.h     |  5 +++++
 2 files changed, 46 insertions(+)

drivers/mtd/nand/nand_base.c +41 -0

index c2e1232cd45c..c6cf775ee431 100644
@@ -48,6 +48,7 @@
 #include <linux/leds.h>
 #include <linux/io.h>
 #include <linux/mtd/partitions.h>
+#include <linux/of_mtd.h>
 
 /* Define default oob placement schemes for large and small page devices */
 static struct nand_ecclayout nand_oob_8 = {

@@ -3798,6 +3799,39 @@ ident_done:
 	return type;
 }
 
+static int nand_dt_init(struct mtd_info *mtd, struct nand_chip *chip,
+			struct device_node *dn)
+{
+	int ecc_mode, ecc_strength, ecc_step;
+
+	if (of_get_nand_bus_width(dn) == 16)
+		chip->options |= NAND_BUSWIDTH_16;
+
+	if (of_get_nand_on_flash_bbt(dn))
+		chip->bbt_options |= NAND_BBT_USE_FLASH;
+
+	ecc_mode = of_get_nand_ecc_mode(dn);
+	ecc_strength = of_get_nand_ecc_strength(dn);
+	ecc_step = of_get_nand_ecc_step_size(dn);
+
+	if ((ecc_step >= 0 && !(ecc_strength >= 0)) ||
+	    (!(ecc_step >= 0) && ecc_strength >= 0)) {
+		pr_err("must set both strength and step size in DT\n");
+		return -EINVAL;
+	}
+
+	if (ecc_mode >= 0)
+		chip->ecc.mode = ecc_mode;
+
+	if (ecc_strength >= 0)
+		chip->ecc.strength = ecc_strength;
+
+	if (ecc_step > 0)
+		chip->ecc.size = ecc_step;
+
+	return 0;
+}
+
 /**
  * nand_scan_ident - [NAND Interface] Scan for the NAND device
  * @mtd: MTD device structure

@@ -3815,6 +3849,13 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
 	int i, nand_maf_id, nand_dev_id;
 	struct nand_chip *chip = mtd->priv;
 	struct nand_flash_dev *type;
+	int ret;
+
+	if (chip->dn) {
+		ret = nand_dt_init(mtd, chip, chip->dn);
+		if (ret)
+			return ret;
+	}
 
 	/* Set the default functions */
 	nand_set_defaults(chip, chip->options & NAND_BUSWIDTH_16);

include/linux/mtd/nand.h +5 -0

index 3d4ea7eb2b68..e0f40e12a2c8 100644
@@ -26,6 +26,8 @@
 
 struct mtd_info;
 struct nand_flash_dev;
+struct device_node;
+
 /* Scan and identify a NAND device */
 extern int nand_scan(struct mtd_info *mtd, int max_chips);
 /*

@@ -542,6 +544,7 @@ struct nand_buffers {
  *			flash device
  * @IO_ADDR_W:		[BOARDSPECIFIC] address to write the 8 I/O lines of the
  *			flash device.
+ * @dn:			[BOARDSPECIFIC] device node describing this instance
  * @read_byte:		[REPLACEABLE] read one byte from the chip
  * @read_word:		[REPLACEABLE] read one word from the chip
  * @write_byte:		[REPLACEABLE] write a single byte to the chip on the

@@ -644,6 +647,8 @@ struct nand_chip {
 	void __iomem *IO_ADDR_R;
 	void __iomem *IO_ADDR_W;
 
+	struct device_node *dn;
+
 	uint8_t (*read_byte)(struct mtd_info *mtd);
 	u16 (*read_word)(struct mtd_info *mtd);
 	void (*write_byte)(struct mtd_info *mtd, uint8_t byte);

Signature · 1 line

1.9.1

drivers/mtd/nand/brcmstb_nand.c line 363

Quoted 13 lines · On Wednesday 06 May 2015 10:59:47 Brian Norris wrote:

drivers/mtd/nand/brcmstb_nand.c new file +12 -0

+
+static inline u32 nand_readreg(struct brcmnand_controller *ctrl, u32 offs)
+{
+       return __raw_readl(ctrl->nand_base + offs);
+}
+
+static inline void nand_writereg(struct brcmnand_controller *ctrl, u32 offs,
+                                u32 val)
+{
+       __raw_writel(val, ctrl->nand_base + offs);
+}
+

You had mentioned previously that there might be an endianess issue in this driver.

I think this won't work on big-endian architectures other than MIPS, so it would be good to either list in the DT the endianess of the device and use appropriate accessors here, or hardcode it based on the architecture (using ioread32_be in big-endian mips, but readl elsewhere).

Using __raw_writel has another problem regarding the DMA capability of this driver, as it will not flush any write buffers or synchronize caches before sending data off to the device, so you risk data corruption.

Also, the
compiler can choose to split up the 32-bit word access into byte accesses, which on most hardware does not do what you want.

Quoted 17 lines · Arnd Bergmann

drivers/mtd/nand/brcmstb_nand.c line 363

Quoted 13 lines · On Wednesday 06 May 2015 10:59:47 Brian Norris wrote:

drivers/mtd/nand/brcmstb_nand.c new file +12 -0

+
+static inline u32 nand_readreg(struct brcmnand_controller *ctrl, u32 offs)
+{
+       return __raw_readl(ctrl->nand_base + offs);
+}
+
+static inline void nand_writereg(struct brcmnand_controller *ctrl, u32 offs,
+                                u32 val)
+{
+       __raw_writel(val, ctrl->nand_base + offs);
+}
+

You had mentioned previously that there might be an endianess issue in this driver.

Might. I have a patch already, but I failed to boot a BE kernel, so I kept it out for now. If you don't mind, I'd prefer patching something like this once it's testable on ARM BE. This *is*, however, extensively tested on MIPS (LE and BE) and ARM (LE).

Quoted 5 lines

I think this won't work on big-endian architectures other than MIPS, so it would be good to either list in the DT the endianess of the device and use appropriate accessors here, or hardcode it based on the architecture (using ioread32_be in big-endian mips, but readl elsewhere).

I suspect we wouldn't need a DT property but could just special-case MIPS BE, as you note.

Quoted 3 lines

Using __raw_writel has another problem regarding the DMA capability of this driver, as it will not flush any write buffers or synchronize caches before sending data off to the device, so you risk data corruption.

We use mb() before kicking off DMA or other commands.

Quoted 3 lines

Also, the
compiler can choose to split up the 32-bit word access into byte accesses, which on most hardware does not do what you want.

Huh? Wouldn't that break just about every driver in existence? And how is writel() any different than __raw_writel() in that regard? From include/asm-generic/io.h:

static inline void writel(u32 value, volatile void __iomem *addr)

{
        __raw_writel(__cpu_to_le32(value), addr);
}

And BTW, splitting isn't possible on ARM. From
arch/arm/include/asm/io.h:

static inline void __raw_writel(u32 val, volatile void __iomem *addr)

{
        asm volatile("str %1, %0"
                     : "+Qo" (*(volatile u32 __force *)addr)
                     : "r" (val));
}

Brian

Quoted 19 lines · Brian Norris

Quoted 17 lines · Arnd Bergmann

drivers/mtd/nand/brcmstb_nand.c line 363

Quoted 13 lines · On Wednesday 06 May 2015 10:59:47 Brian Norris wrote:

drivers/mtd/nand/brcmstb_nand.c new file +12 -0

+
+static inline u32 nand_readreg(struct brcmnand_controller *ctrl, u32 offs)
+{
+       return __raw_readl(ctrl->nand_base + offs);
+}
+
+static inline void nand_writereg(struct brcmnand_controller *ctrl, u32 offs,
+                                u32 val)
+{
+       __raw_writel(val, ctrl->nand_base + offs);
+}
+

You had mentioned previously that there might be an endianess issue in this driver.

Quoted 4 lines

Might. I have a patch already, but I failed to boot a BE kernel, so I kept it out for now. If you don't mind, I'd prefer patching something like this once it's testable on ARM BE. This *is*, however, extensively tested on MIPS (LE and BE) and ARM (LE).

Correct, extensive test and pass all MTD test cases. We should eventually be able to test this on a working ARM BE platform, within the next couple months.

Quoted 7 lines

Quoted 4 lines

I think this won't work on big-endian architectures other than MIPS, so it would be good to either list in the DT the endianess of the device and use appropriate accessors here, or hardcode it based on the architecture (using ioread32_be in big-endian mips, but readl elsewhere).

I suspect we wouldn't need a DT property but could just special-case MIPS BE, as you note.

Quoted 5 lines

Quoted 3 lines

Using __raw_writel has another problem regarding the DMA capability of this driver, as it will not flush any write buffers or synchronize caches before sending data off to the device, so you risk data corruption.

We use mb() before kicking off DMA or other commands.

Quoted 16 lines

Quoted 3 lines

Also, the
compiler can choose to split up the 32-bit word access into byte accesses, which on most hardware does not do what you want.

Huh? Wouldn't that break just about every driver in existence? And how is writel() any different than __raw_writel() in that regard? From include/asm-generic/io.h:

static inline void writel(u32 value, volatile void __iomem *addr)

{
        __raw_writel(__cpu_to_le32(value), addr);
}

And BTW, splitting isn't possible on ARM. From
arch/arm/include/asm/io.h:

Quoted 7 lines

static inline void __raw_writel(u32 val, volatile void __iomem *addr)

{
        asm volatile("str %1, %0"
                     : "+Qo" (*(volatile u32 __force *)addr)
                     : "r" (val));
}

Quoted 4 lines · On Wednesday 06 May 2015 14:18:47 Ray Jui wrote:

Quoted 2 lines · Brian Norris

Quoted 1 line · Arnd Bergmann

On Wednesday 06 May 2015 10:59:47 Brian Norris wrote:

There is one twist here that I forgot to mention:

This loop in brcmnand_read_by_pio() and the respective one in brcmnand_write_by_pio():

drivers/mtd/nand/brcmstb_nand.c new file +3 -0

+               if (likely(buf))
+                       for (j = 0; j < FC_WORDS; j++, buf++)
+                               *buf = brcmnand_read_fc(ctrl, j);

should be converted to use ioread32_rep().

There are two reasons for
this:

a) accessing the flash data is inherently different from accessing an mmio register, and you want the bytes to end up in memory in the same order that they are in flash.

ioread32_rep() uses __raw_readl()
internally for this purpose, except on architectures that have a byte flipping hardware on the bus interface.

b) The implementation is optimized on ARM and will likely give you higher throughput than a manual loop using readl().

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

Using __raw_writel has another problem regarding the DMA capability of this driver, as it will not flush any write buffers or synchronize caches before sending data off to the device, so you risk data corruption.

We use mb() before kicking off DMA or other commands.

Ok, that should work, but will be a stronger barrier than necessary on some architectures. On ARM, mb() is 'dsb(); outer_sync();', while readl only needs a 'dsb()' and writel() can use dsb(st) that is slightly weaker than a full dsb().

Quoted 16 lines

Quoted 16 lines

Quoted 3 lines

Also, the
compiler can choose to split up the 32-bit word access into byte accesses, which on most hardware does not do what you want.

Huh? Wouldn't that break just about every driver in existence? And how is writel() any different than __raw_writel() in that regard? From include/asm-generic/io.h:

static inline void writel(u32 value, volatile void __iomem *addr)

{
        __raw_writel(__cpu_to_le32(value), addr);
}

And BTW, splitting isn't possible on ARM. From
arch/arm/include/asm/io.h:

Quoted 7 lines

Quoted 7 lines

static inline void __raw_writel(u32 val, volatile void __iomem *addr)

{
        asm volatile("str %1, %0"
                     : "+Qo" (*(volatile u32 __force *)addr)
                     : "r" (val));
}

Ah right, we changed that one to simplify KVM support. It used to just do a volatile store for __raw_* but use an assembly for writel_relaxed().

Quoted 5 lines · Arnd Bergmann

Quoted 4 lines · On Wednesday 06 May 2015 14:18:47 Ray Jui wrote:

Quoted 2 lines · Brian Norris

Quoted 1 line · Arnd Bergmann

On Wednesday 06 May 2015 10:59:47 Brian Norris wrote:

[...]

Quoted 9 lines

There is one twist here that I forgot to mention:

This loop in brcmnand_read_by_pio() and the respective one in brcmnand_write_by_pio():

drivers/mtd/nand/brcmstb_nand.c new file +3 -0

+               if (likely(buf))
+                       for (j = 0; j < FC_WORDS; j++, buf++)
+                               *buf = brcmnand_read_fc(ctrl, j);

Quoted 1 line

should be converted to use ioread32_rep().

Huh? That's completely wrong. You're assuming I have a single-register FIFO, when in fact, I have a memory-mapped hardware buffer. Maybe you're looking for memcpy_{to,from}io()? I see this is not optimized at all, though.

Quoted 7 lines

There are two reasons for
this:

a) accessing the flash data is inherently different from accessing an mmio register, and you want the bytes to end up in memory in the same order that they are in flash.

Right, which is why it's a separate helper function in my driver, and it will stay with __raw_{read,write}l().

Quoted 6 lines

ioread32_rep() uses __raw_readl()
internally for this purpose, except on architectures that have a byte flipping hardware on the bus interface.

b) The implementation is optimized on ARM and will likely give you higher throughput than a manual loop using readl().

You suggested the wrong helper, and the "right" helper is *not* optimized. It even has comments saying "this needs to be optimized".

Quoted 25 lines

Quoted 17 lines

Quoted 17 lines

Quoted 4 lines

Also, the
compiler can choose to split up the 32-bit word access into byte accesses, which on most hardware does not do what you want.

Huh? Wouldn't that break just about every driver in existence? And how is writel() any different than __raw_writel() in that regard? From include/asm-generic/io.h:

static inline void writel(u32 value, volatile void __iomem *addr)

{
        __raw_writel(__cpu_to_le32(value), addr);
}

And BTW, splitting isn't possible on ARM. From
arch/arm/include/asm/io.h:

Quoted 7 lines

Quoted 7 lines

static inline void __raw_writel(u32 val, volatile void __iomem *addr)

{
        asm volatile("str %1, %0"
                     : "+Qo" (*(volatile u32 __force *)addr)
                     : "r" (val));
}

Quoted 2 lines

Ah right, we changed that one to simplify KVM support. It used to just do a volatile store for __raw_* but use an assembly for writel_relaxed().

While the ARM case is rock-solid in my favor, I would appreciate an answer to the asm-generic case too; do you really expect that any sane compiler would break up word-aligned volatile stores into smaller (e.g., 8-bit) stores? As I said, I think that means every driver written in C is broken, not just the ones using your pet enemies, __raw_{read,write}l().

Quoted 10 lines · On Wednesday 06 May 2015 14:18:47 Ray Jui wrote:

Quoted 8 lines · Brian Norris

Quoted 2 lines · Arnd Bergmann

You had mentioned previously that there might be an endianess issue in this driver.

Might. I have a patch already, but I failed to boot a BE kernel, so I kept it out for now. If you don't mind, I'd prefer patching something like this once it's testable on ARM BE. This *is*, however, extensively tested on MIPS (LE and BE) and ARM (LE).

Quoted 3 lines

Correct, extensive test and pass all MTD test cases. We should eventually be able to test this on a working ARM BE platform, within the next couple months.

I'm fine with not testing it on ARM, as long as the code is written in a way that is plausible to be correct and not obviously broken.

Would this satisfy you?

From: Brian Norris <[email protected]>
Date: Tue, 5 May 2015 17:46:42 -0700
Subject: [PATCH] mtd: brcmstb_nand: fixup endianness assumptions

All users of this controller (MIPS or ARM) have previously used native I/O (__raw{read,write}l()) to access registers. This is normal for the MIPS case, where BMIPS chips often have a boot-strap that configures not only the CPU, but also all the busing, to use a given endianness. However, newer ARM cores support switching to big endian mode at runtime, which would leave us with different bus and CPU endianness. For these cases, we should use the endian-switching accessors, so we continue to access the NAND core in little endian mode.

Suggested by Arnd.

Signed-off-by Brian Norris <[email protected]>

---

drivers/mtd/nand/brcmstb_nand.c +20 -0

+       /*
+        * Some SoCs integrate this controller (e.g., its interrupt bits) in
+        * interesting ways
+        */
+       if (of_property_read_bool(dn, "brcm,nand-soc")) {
+               struct device_node *soc_dn;
+
+               soc_dn = of_parse_phandle(dn, "brcm,nand-soc", 0);
+               if (!soc_dn)
+                       return -ENODEV;
+
+               ctrl->soc = devm_brcmnand_probe_soc(dev, soc_dn);
+               if (!ctrl->soc) {
+                       dev_err(dev, "could not probe SoC data\n");
+                       of_node_put(soc_dn);
+                       return -ENODEV;
+               }
+
+               ret = devm_request_irq(dev, ctrl->irq, brcmnand_irq, 0,
+                                      DRV_NAME, ctrl);

Quoted 27 lines · On Wednesday 06 May 2015 10:59:50 Brian Norris wrote:

drivers/mtd/nand/brcmstb_nand.c +27 -0

+       /*
+        * Some SoCs integrate this controller (e.g., its interrupt bits) in
+        * interesting ways
+        */
+       if (of_property_read_bool(dn, "brcm,nand-soc")) {
+               struct device_node *soc_dn;
+
+               soc_dn = of_parse_phandle(dn, "brcm,nand-soc", 0);
+               if (!soc_dn)
+                       return -ENODEV;
+
+               ctrl->soc = devm_brcmnand_probe_soc(dev, soc_dn);
+               if (!ctrl->soc) {
+                       dev_err(dev, "could not probe SoC data\n");
+                       of_node_put(soc_dn);
+                       return -ENODEV;
+               }
+
+               ret = devm_request_irq(dev, ctrl->irq, brcmnand_irq, 0,
+                                      DRV_NAME, ctrl);
+
+               /* Enable interrupt */
+               ctrl->soc->ctlrdy_set_enabled(ctrl->soc, true);
+
+               of_node_put(soc_dn);
+       } else {
+               /* Use standard interrupt infrastructure */

drivers/mtd/nand/brcmstb_nand.c static int brcmnand_probe(struct platform_device *pdev) · line 2191

Quoted 4 lines

drivers/mtd/nand/brcmstb_nand.c +3 -0

+               ret = devm_request_irq(dev, ctrl->irq, brcmnand_ctlrdy_irq, 0,
+                                      DRV_NAME, ctrl);
+       }

It looks to me like this should be handled as a nested irqchip, so the node you look up gets used as the "interrupt-parent" instead, making the behavior of this SoC transparent to the nand driver.

We recently merged nested irqdomain support as well, which might help here, or might not be needed.

Quoted 28 lines · Arnd Bergmann

Quoted 27 lines · On Wednesday 06 May 2015 10:59:50 Brian Norris wrote:

drivers/mtd/nand/brcmstb_nand.c +27 -0

+       /*
+        * Some SoCs integrate this controller (e.g., its interrupt bits) in
+        * interesting ways
+        */
+       if (of_property_read_bool(dn, "brcm,nand-soc")) {
+               struct device_node *soc_dn;
+
+               soc_dn = of_parse_phandle(dn, "brcm,nand-soc", 0);
+               if (!soc_dn)
+                       return -ENODEV;
+
+               ctrl->soc = devm_brcmnand_probe_soc(dev, soc_dn);
+               if (!ctrl->soc) {
+                       dev_err(dev, "could not probe SoC data\n");
+                       of_node_put(soc_dn);
+                       return -ENODEV;
+               }
+
+               ret = devm_request_irq(dev, ctrl->irq, brcmnand_irq, 0,
+                                      DRV_NAME, ctrl);
+
+               /* Enable interrupt */
+               ctrl->soc->ctlrdy_set_enabled(ctrl->soc, true);
+
+               of_node_put(soc_dn);
+       } else {
+               /* Use standard interrupt infrastructure */

Quoted 8 lines

drivers/mtd/nand/brcmstb_nand.c static int brcmnand_probe(struct platform_device *pdev) · line 2191

Quoted 4 lines

drivers/mtd/nand/brcmstb_nand.c +3 -0

+               ret = devm_request_irq(dev, ctrl->irq, brcmnand_ctlrdy_irq, 0,
+                                      DRV_NAME, ctrl);
+       }

It looks to me like this should be handled as a nested irqchip, so the node you look up gets used as the "interrupt-parent" instead, making the behavior of this SoC transparent to the nand driver.

You snipped the rest of the patch, which involves more than just IRQ handling. The same registers touch both interrupts and data bus endian configuration, so it can't possibly be done transparently to the NAND driver.

Quoted 2 lines

We recently merged nested irqdomain support as well, which might help here, or might not be needed.

I'm not familiar with nested irqdomains. Do they address anything like the above problem?

Brian

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

Quoted 28 lines · Arnd Bergmann

Quoted 27 lines · On Wednesday 06 May 2015 10:59:50 Brian Norris wrote:

drivers/mtd/nand/brcmstb_nand.c +27 -0

+       /*
+        * Some SoCs integrate this controller (e.g., its interrupt bits) in
+        * interesting ways
+        */
+       if (of_property_read_bool(dn, "brcm,nand-soc")) {
+               struct device_node *soc_dn;
+
+               soc_dn = of_parse_phandle(dn, "brcm,nand-soc", 0);
+               if (!soc_dn)
+                       return -ENODEV;
+
+               ctrl->soc = devm_brcmnand_probe_soc(dev, soc_dn);
+               if (!ctrl->soc) {
+                       dev_err(dev, "could not probe SoC data\n");
+                       of_node_put(soc_dn);
+                       return -ENODEV;
+               }
+
+               ret = devm_request_irq(dev, ctrl->irq, brcmnand_irq, 0,
+                                      DRV_NAME, ctrl);
+
+               /* Enable interrupt */
+               ctrl->soc->ctlrdy_set_enabled(ctrl->soc, true);
+
+               of_node_put(soc_dn);
+       } else {
+               /* Use standard interrupt infrastructure */

Quoted 8 lines

drivers/mtd/nand/brcmstb_nand.c static int brcmnand_probe(struct platform_device *pdev) · line 2191

Quoted 4 lines

drivers/mtd/nand/brcmstb_nand.c +3 -0

+               ret = devm_request_irq(dev, ctrl->irq, brcmnand_ctlrdy_irq, 0,
+                                      DRV_NAME, ctrl);
+       }

It looks to me like this should be handled as a nested irqchip, so the node you look up gets used as the "interrupt-parent" instead, making the behavior of this SoC transparent to the nand driver.

You snipped the rest of the patch, which involves more than just IRQ handling. The same registers touch both interrupts and data bus endian configuration, so it can't possibly be done transparently to the NAND driver.

Quoted 2 lines

We recently merged nested irqdomain support as well, which might help here, or might not be needed.

I'm not familiar with nested irqdomains. Do they address anything like the above problem?

Quoted 38 lines · On Wednesday 06 May 2015 13:49:10 Brian Norris wrote:

Quoted 36 lines · Arnd Bergmann

Quoted 21 lines · On Wednesday 06 May 2015 10:59:50 Brian Norris wrote:

drivers/mtd/nand/brcmstb_nand.c +20 -0

+       /*
+        * Some SoCs integrate this controller (e.g., its interrupt bits) in
+        * interesting ways
+        */
+       if (of_property_read_bool(dn, "brcm,nand-soc")) {
+               struct device_node *soc_dn;
+
+               soc_dn = of_parse_phandle(dn, "brcm,nand-soc", 0);
+               if (!soc_dn)
+                       return -ENODEV;
+
+               ctrl->soc = devm_brcmnand_probe_soc(dev, soc_dn);
+               if (!ctrl->soc) {
+                       dev_err(dev, "could not probe SoC data\n");
+                       of_node_put(soc_dn);
+                       return -ENODEV;
+               }
+
+               ret = devm_request_irq(dev, ctrl->irq, brcmnand_irq, 0,
+                                      DRV_NAME, ctrl);

drivers/mtd/nand/brcmstb_nand.c static int brcmnand_probe(struct platform_device *pdev) · line 2191

Quoted 10 lines

drivers/mtd/nand/brcmstb_nand.c +10 -0

+
+               /* Enable interrupt */
+               ctrl->soc->ctlrdy_set_enabled(ctrl->soc, true);
+
+               of_node_put(soc_dn);
+       } else {
+               /* Use standard interrupt infrastructure */
+               ret = devm_request_irq(dev, ctrl->irq, brcmnand_ctlrdy_irq, 0,
+                                      DRV_NAME, ctrl);
+       }

It looks to me like this should be handled as a nested irqchip, so the node you look up gets used as the "interrupt-parent" instead, making the behavior of this SoC transparent to the nand driver.

Quoted 4 lines

You snipped the rest of the patch, which involves more than just IRQ handling. The same registers touch both interrupts and data bus endian configuration, so it can't possibly be done transparently to the NAND driver.

Anything else in there?

The bus configuration would just involve writing
a constant value in some register, right?

Doing that in the irqchip
is also a bit ugly, but may still be better overall than doing it the way you have above.

Quoted 5 lines

Quoted 2 lines

We recently merged nested irqdomain support as well, which might help here, or might not be needed.

I'm not familiar with nested irqdomains. Do they address anything like the above problem?

The problem that nested irqdomains address is when an interrupt is handled by two irqchips, in particular when one irqchip handles a virtual interrupt number that was claimed by another irqchip already.

Quoted 39 lines · Arnd Bergmann

Quoted 38 lines · On Wednesday 06 May 2015 13:49:10 Brian Norris wrote:

Quoted 36 lines · Arnd Bergmann

Quoted 21 lines · On Wednesday 06 May 2015 10:59:50 Brian Norris wrote:

drivers/mtd/nand/brcmstb_nand.c +20 -0

+       /*
+        * Some SoCs integrate this controller (e.g., its interrupt bits) in
+        * interesting ways
+        */
+       if (of_property_read_bool(dn, "brcm,nand-soc")) {
+               struct device_node *soc_dn;
+
+               soc_dn = of_parse_phandle(dn, "brcm,nand-soc", 0);
+               if (!soc_dn)
+                       return -ENODEV;
+
+               ctrl->soc = devm_brcmnand_probe_soc(dev, soc_dn);
+               if (!ctrl->soc) {
+                       dev_err(dev, "could not probe SoC data\n");
+                       of_node_put(soc_dn);
+                       return -ENODEV;
+               }
+
+               ret = devm_request_irq(dev, ctrl->irq, brcmnand_irq, 0,
+                                      DRV_NAME, ctrl);

drivers/mtd/nand/brcmstb_nand.c static int brcmnand_probe(struct platform_device *pdev) · line 2191

Quoted 10 lines

drivers/mtd/nand/brcmstb_nand.c +10 -0

+
+               /* Enable interrupt */
+               ctrl->soc->ctlrdy_set_enabled(ctrl->soc, true);
+
+               of_node_put(soc_dn);
+       } else {
+               /* Use standard interrupt infrastructure */
+               ret = devm_request_irq(dev, ctrl->irq, brcmnand_ctlrdy_irq, 0,
+                                      DRV_NAME, ctrl);
+       }

It looks to me like this should be handled as a nested irqchip, so the node you look up gets used as the "interrupt-parent" instead, making the behavior of this SoC transparent to the nand driver.

Quoted 6 lines

Quoted 4 lines

You snipped the rest of the patch, which involves more than just IRQ handling. The same registers touch both interrupts and data bus endian configuration, so it can't possibly be done transparently to the NAND driver.

Anything else in there?

Looks like miscellaneous NAND-related control bits. AXI and APB endian configuration; several interrupt-enable bits (we only use one for now); a clock-enable; and some timing test mode bits.

Quoted 2 lines

The bus configuration would just involve writing
a constant value in some register, right?

I'm not an expert on the Cygnus/iProc chips, but I believe the answer is
no: we *must* reconfigure the bus before and after each data transaction, because it affects the rest of the core too. Others on the CC list can probably elaborate.

Quoted 11 lines · Arnd Bergmann

Quoted 9 lines · On Wednesday 06 May 2015 13:49:10 Brian Norris wrote:

Quoted 3 lines · Arnd Bergmann

It looks to me like this should be handled as a nested irqchip, so the node you look up gets used as the "interrupt-parent" instead, making the behavior of this SoC transparent to the nand driver.

You snipped the rest of the patch, which involves more than just IRQ handling. The same registers touch both interrupts and data bus endian configuration, so it can't possibly be done transparently to the NAND driver.

Quoted 1 line

Anything else in there?

Looks like miscellaneous NAND-related control bits. AXI and APB endian configuration; several interrupt-enable bits (we only use one for now); a clock-enable; and some timing test mode bits.

Quoted 2 lines

The bus configuration would just involve writing
a constant value in some register, right?

I'm not an expert on the Cygnus/iProc chips, but I believe the answer is
no: we *must* reconfigure the bus before and after each data transaction, because it affects the rest of the core too. Others on the CC list can probably elaborate.

Quoted 3 lines

Doing that in the irqchip
is also a bit ugly, but may still be better overall than doing it the way you have above.

Well, the Cygnus/iProc case is more complicated as I mention. But I agree that other cases could be nicer, like bcm63138 (which only has separate interrupt status/enable registers). Do you expect a new irqchip driver for every arrangement of registers like this? There are a few similar chips that have status/enable registers in different orders, and some that combine them into a single word. Do we really need a new irqchip driver for each one? I might have done that for bcm63138 and bcm3384, except that I thought I'd have to fall back to this extra per-SoC support driver for Cygnus anyway.

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

We recently merged nested irqdomain support as well, which might help here, or might not be needed.

I'm not familiar with nested irqdomains. Do they address anything like the above problem?

Quoted 3 lines

The problem that nested irqdomains address is when an interrupt is handled by two irqchips, in particular when one irqchip handles a virtual interrupt number that was claimed by another irqchip already.

I'll do some reading on that, but it definitely doesn't address the main problem here.

Quoted 4 lines · On Thursday 07 May 2015 11:42:46 Brian Norris wrote:

Quoted 2 lines · Arnd Bergmann

The bus configuration would just involve writing
a constant value in some register, right?

Quoted 4 lines

I'm not an expert on the Cygnus/iProc chips, but I believe the answer is
no: we *must* reconfigure the bus before and after each data transaction, because it affects the rest of the core too. Others on the CC list can probably elaborate.

That would not be a problem I think: the irqchip driver would always get initialized first, because the main driver would get an -EPROBE_DEFER when requesting the interrupt line otherwise.

Quoted 4 lines

Quoted 3 lines

Doing that in the irqchip
is also a bit ugly, but may still be better overall than doing it the way you have above.

Quoted 9 lines

Well, the Cygnus/iProc case is more complicated as I mention. But I agree that other cases could be nicer, like bcm63138 (which only has separate interrupt status/enable registers). Do you expect a new irqchip driver for every arrangement of registers like this? There are a few similar chips that have status/enable registers in different orders, and some that combine them into a single word. Do we really need a new irqchip driver for each one? I might have done that for bcm63138 and bcm3384, except that I thought I'd have to fall back to this extra per-SoC support driver for Cygnus anyway.

I assumed this one was the only odd one.

Quoted 10 lines

Quoted 9 lines

Quoted 5 lines

Quoted 2 lines

We recently merged nested irqdomain support as well, which might help here, or might not be needed.

I'm not familiar with nested irqdomains. Do they address anything like the above problem?

The problem that nested irqdomains address is when an interrupt is handled by two irqchips, in particular when one irqchip handles a virtual interrupt number that was claimed by another irqchip already.

Quoted 2 lines

I'll do some reading on that, but it definitely doesn't address the main problem here.

Ok, back to the drawing board then: How about turning the probe order around and splitting the SoC-specific part out into its own platform_driver:

Instead of bus_prepare/bus_unprepare for bcm63138, you'd get a

bcm63138_nand_driver with its own probe() function that calls the common probe function. That would make the soc specific parts better contained and match how we normally do abstractions of similar drivers.

Quoted 10 lines · Arnd Bergmann

Quoted 8 lines · On Thursday 07 May 2015 11:42:46 Brian Norris wrote:

Quoted 2 lines · Arnd Bergmann

The bus configuration would just involve writing
a constant value in some register, right?

I'm not an expert on the Cygnus/iProc chips, but I believe the answer is
no: we *must* reconfigure the bus before and after each data transaction, because it affects the rest of the core too. Others on the CC list can probably elaborate.

Quoted 3 lines

That would not be a problem I think: the irqchip driver would always get initialized first, because the main driver would get an -EPROBE_DEFER when requesting the interrupt line otherwise.

Huh? I wasn't worried about initialization order. I was worried about the fact that the "NAND" and "SoC" portions are very integrated when handling the data path. And I think you agreed that this means we can't do a straight-up irqchip.

Quoted 14 lines

Quoted 4 lines

Quoted 3 lines

Doing that in the irqchip
is also a bit ugly, but may still be better overall than doing it the way you have above.

Quoted 9 lines

Well, the Cygnus/iProc case is more complicated as I mention. But I agree that other cases could be nicer, like bcm63138 (which only has separate interrupt status/enable registers). Do you expect a new irqchip driver for every arrangement of registers like this? There are a few similar chips that have status/enable registers in different orders, and some that combine them into a single word. Do we really need a new irqchip driver for each one? I might have done that for bcm63138 and bcm3384, except that I thought I'd have to fall back to this extra per-SoC support driver for Cygnus anyway.

Quoted 1 line

I assumed this one was the only odd one.

Yes, the Cygnus/iProc are the oddest. The others (BCM33xx (not yet supported) and BCM63xxx) just have separate one-off interrupt register blocks.

To be clear, since I'm not sure if you're confused below:

* Cygnus is a family of chips using the IPROC architecture, coming from the Infrastructure/Networking Group; there are BCMxxxx numbers noted in arch/arm/mach-bcm/Kconfig for them, but I usually just refer to the Cygnus family or the IPROC architecture.

* BCM63xxx is a class of DSL chips from the Broadband/Connectivity Group.

Quoted 13 lines

Quoted 12 lines

Quoted 9 lines

Quoted 5 lines

Quoted 2 lines

We recently merged nested irqdomain support as well, which might help here, or might not be needed.

I'm not familiar with nested irqdomains. Do they address anything like the above problem?

The problem that nested irqdomains address is when an interrupt is handled by two irqchips, in particular when one irqchip handles a virtual interrupt number that was claimed by another irqchip already.

I'll do some reading on that, but it definitely doesn't address the main problem here.

Quoted 4 lines

Ok, back to the drawing board then: How about turning the probe order around and splitting the SoC-specific part out into its own platform_driver:

Instead of bus_prepare/bus_unprepare for bcm63138, you'd get a

bcm63138 does not need the bus_{,un}prepare. That's for IPROC/Cygnus.

Quoted 4 lines

bcm63138_nand_driver with its own probe() function that calls the common probe function. That would make the soc specific parts better contained and match how we normally do abstractions of similar drivers.

OK, so I can imagine this might require changing the DT binding a bit [1] (is that your goal?). But what's the intended software difference? [2] I'll still be passing around the same sorts of callbacks from the 'iproc_nand' probe to the common probe function.

Brian

[1] e.g.:

       nand: nand@18046000 {
               compatible = "brcm,iproc-nand", "brcm,brcmnand-v6.1", "brcm,brcmnand";
               reg = <0x18046000 0x600>, <0xf8105408 0x600>, <0x18046f00 0x20>;
               reg-names = "nand", "iproc-idm", "iproc-ext";
               interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;

               #address-cells = <1>;
               #size-cells = <0>;

               brcm,nand-has-wp;
       };

This captures the extra "iproc-*" register ranges. Then we could have the iproc_nand driver bind against "brcm,iproc-nand", then call into the common probe, which would then accept/reject based on "brcm,brcmnand-vX.Y".

[2] The DT structure from [1] could actually accommodate either driver structure just fine. So maybe that means it's a better hardware description?

Quoted 1 line · On Friday 08 May 2015 12:38:50 Brian Norris wrote:

On Fri, May 08, 2015 at 03:41:10PM +0200, Arnd Bergmann wrote:

Quoted 7 lines

To be clear, since I'm not sure if you're confused below:

* Cygnus is a family of chips using the IPROC architecture, coming from the Infrastructure/Networking Group; there are BCMxxxx numbers noted in arch/arm/mach-bcm/Kconfig for them, but I usually just refer to the Cygnus family or the IPROC architecture.

Quoted 2 lines

* BCM63xxx is a class of DSL chips from the Broadband/Connectivity Group.

Thanks for the clarification, I think that is roughly what I thought it was, but I'm still not sure about brcmstb. Is that related to bcm63xxx or separate?

Quoted 9 lines

Quoted 4 lines

bcm63138_nand_driver with its own probe() function that calls the common probe function. That would make the soc specific parts better contained and match how we normally do abstractions of similar drivers.

OK, so I can imagine this might require changing the DT binding a bit [1] (is that your goal?). But what's the intended software difference? [2] I'll still be passing around the same sorts of callbacks from the 'iproc_nand' probe to the common probe function.

Quoted 16 lines

Brian

[1] e.g.:

       nand: nand@18046000 {
               compatible = "brcm,iproc-nand", "brcm,brcmnand-v6.1", "brcm,brcmnand";
               reg = <0x18046000 0x600>, <0xf8105408 0x600>, <0x18046f00 0x20>;
               reg-names = "nand", "iproc-idm", "iproc-ext";
               interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;

               #address-cells = <1>;
               #size-cells = <0>;

               brcm,nand-has-wp;
       };

Quoted 8 lines

This captures the extra "iproc-*" register ranges. Then we could have the iproc_nand driver bind against "brcm,iproc-nand", then call into the common probe, which would then accept/reject based on "brcm,brcmnand-vX.Y".

[2] The DT structure from [1] could actually accommodate either driver structure just fine. So maybe that means it's a better hardware description?

Yes, I think this makes sense overall. Regarding the specific example, can you clarify how the register areas in iproc are structured?

The 0xf8105408 and 0x18046f00 start addresses are not aligned to large powers of two, which often indicates that they are part of some other, larger, unit that might need to have a driver of its own, so before we specify a binding like the one you proposed above I'd like to make sure we're not getting ourselves into trouble later.

Quoted 2 lines · Arnd Bergmann

Quoted 1 line · On Friday 08 May 2015 12:38:50 Brian Norris wrote:

On Fri, May 08, 2015 at 03:41:10PM +0200, Arnd Bergmann wrote:

[...]

Quoted 7 lines

Quoted 7 lines

To be clear, since I'm not sure if you're confused below:

* Cygnus is a family of chips using the IPROC architecture, coming from the Infrastructure/Networking Group; there are BCMxxxx numbers noted in arch/arm/mach-bcm/Kconfig for them, but I usually just refer to the Cygnus family or the IPROC architecture.

Quoted 5 lines

Quoted 2 lines

* BCM63xxx is a class of DSL chips from the Broadband/Connectivity Group.

Thanks for the clarification, I think that is roughly what I thought it was, but I'm still not sure about brcmstb. Is that related to bcm63xxx or separate?

I think arch/arm/mach-bcm/Kconfig has the best summary. brcmstb is separate; BCM7xxx is generally (always?) Set-Top Box.

Another potentially confusing point: the main driver is named 'brcsmtb_nand' since the NAND core (and driver) originated from STB chips. But that core was applied to other non-STB chips, and so the driver has been extended.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

bcm63138_nand_driver with its own probe() function that calls the common probe function. That would make the soc specific parts better contained and match how we normally do abstractions of similar drivers.

Quoted 4 lines

Quoted 4 lines

OK, so I can imagine this might require changing the DT binding a bit [1] (is that your goal?). But what's the intended software difference? [2] I'll still be passing around the same sorts of callbacks from the 'iproc_nand' probe to the common probe function.

^^ before getting bogged down on the DT details (which can be changed independently), I'd like to address this point.

Quoted 9 lines

Yes, I think this makes sense overall. Regarding the specific example, can you clarify how the register areas in iproc are structured?

The 0xf8105408 and 0x18046f00 start addresses are not aligned to large powers of two, which often indicates that they are part of some other, larger, unit that might need to have a driver of its own, so before we specify a binding like the one you proposed above I'd like to make sure we're not getting ourselves into trouble later.

I may want the Cygnus guys to speak up here, partly for technical expertise and partly to know how much they care to share...

<0xf8105408 0x600>: covers a series of NAND_IDM registers. NAND has a few bits we don't care about (for debugging, logging, and resetting), as well as its interrupt enable bits. The adjacent blocks cover similar IDM blocks for other cores (SPI, PNOR, DDR), and they are similarly unaligned. Not sure why, exactly; probably just a compact layout.

<0x18046f00 0x20>: a series of 8 NAND interrupt registers, each word containing a single bit representing status/clear. There is nothing between the "nand" range and this range, and the SPI core register range follows.

So I think these are pretty clearly-delineated register ranges for NAND, and the alignment is not really missing anything. Adjacent hardware (e.g., SPI) is independent, though pieces look similar. For one, it has similar:

* interrupt enable bits in the IDM range (0xf8106408 to 0xf8106a00); and
* interrupt status/clear following the SPI block (0x180473a0 to 0x180473b8)

Quoted 13 lines · On Friday 08 May 2015 13:47:25 Brian Norris wrote:

Quoted 11 lines · Arnd Bergmann

Quoted 10 lines · On Friday 08 May 2015 12:38:50 Brian Norris wrote:

Quoted 4 lines · Arnd Bergmann

bcm63138_nand_driver with its own probe() function that calls the common probe function. That would make the soc specific parts better contained and match how we normally do abstractions of similar drivers.

OK, so I can imagine this might require changing the DT binding a bit [1] (is that your goal?). But what's the intended software difference? [2] I'll still be passing around the same sorts of callbacks from the 'iproc_nand' probe to the common probe function.

Quoted 2 lines

^^ before getting bogged down on the DT details (which can be changed independently), I'd like to address this point.

The intended change is to make it work according to Documentation/driver-model/design-patterns.txt

basically, by having all the shared code be a "library" module that gets called by the actual hardware specific drivers, rather than having the shared code be the central driver that fans out into all possible subdrivers.

Quoted 28 lines

Quoted 8 lines

Yes, I think this makes sense overall. Regarding the specific example, can you clarify how the register areas in iproc are structured?

The 0xf8105408 and 0x18046f00 start addresses are not aligned to large powers of two, which often indicates that they are part of some other, larger, unit that might need to have a driver of its own, so before we specify a binding like the one you proposed above I'd like to make sure we're not getting ourselves into trouble later.

I may want the Cygnus guys to speak up here, partly for technical expertise and partly to know how much they care to share...

<0xf8105408 0x600>: covers a series of NAND_IDM registers. NAND has a few bits we don't care about (for debugging, logging, and resetting), as well as its interrupt enable bits. The adjacent blocks cover similar IDM blocks for other cores (SPI, PNOR, DDR), and they are similarly unaligned. Not sure why, exactly; probably just a compact layout.

<0x18046f00 0x20>: a series of 8 NAND interrupt registers, each word containing a single bit representing status/clear. There is nothing between the "nand" range and this range, and the SPI core register range follows.

So I think these are pretty clearly-delineated register ranges for NAND, and the alignment is not really missing anything. Adjacent hardware (e.g., SPI) is independent, though pieces look similar. For one, it has similar:

Quoted 4 lines

* interrupt enable bits in the IDM range (0xf8106408 to 0xf8106a00); and
* interrupt status/clear following the SPI block (0x180473a0 to 0x180473b8)

This would in turn indicate that we should treat these ranges as an irqchip that handles all sorts of devices, but it really depends on the particular register layout.

Quoted 2 lines · Arnd Bergmann

Quoted 1 line · On Friday 08 May 2015 12:38:50 Brian Norris wrote:

On Fri, May 08, 2015 at 03:41:10PM +0200, Arnd Bergmann wrote:

[...]

Quoted 7 lines

Quoted 7 lines

To be clear, since I'm not sure if you're confused below:

* Cygnus is a family of chips using the IPROC architecture, coming from the Infrastructure/Networking Group; there are BCMxxxx numbers noted in arch/arm/mach-bcm/Kconfig for them, but I usually just refer to the Cygnus family or the IPROC architecture.

Quoted 5 lines

Quoted 2 lines

* BCM63xxx is a class of DSL chips from the Broadband/Connectivity Group.

Thanks for the clarification, I think that is roughly what I thought it was, but I'm still not sure about brcmstb. Is that related to bcm63xxx or separate?

I think arch/arm/mach-bcm/Kconfig has the best summary. brcmstb is separate; BCM7xxx is generally (always?) Set-Top Box.

Another potentially confusing point: the main driver is named 'brcsmtb_nand' since the NAND core (and driver) originated from STB chips. But that core was applied to other non-STB chips, and so the driver has been extended.

Quoted 9 lines

Quoted 9 lines

Quoted 4 lines

bcm63138_nand_driver with its own probe() function that calls the common probe function. That would make the soc specific parts better contained and match how we normally do abstractions of similar drivers.

OK, so I can imagine this might require changing the DT binding a bit [1] (is that your goal?). But what's the intended software difference? [2] I'll still be passing around the same sorts of callbacks from the 'iproc_nand' probe to the common probe function.

^^ before getting bogged down on the DT details (which can be changed independently), I'd like to address this point.

Quoted 25 lines

Quoted 16 lines

Brian

[1] e.g.:

       nand: nand@18046000 {
               compatible = "brcm,iproc-nand", "brcm,brcmnand-v6.1", "brcm,brcmnand";
               reg = <0x18046000 0x600>, <0xf8105408 0x600>, <0x18046f00 0x20>;
               reg-names = "nand", "iproc-idm", "iproc-ext";
               interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;

               #address-cells = <1>;
               #size-cells = <0>;

               brcm,nand-has-wp;
       };

Quoted 8 lines

This captures the extra "iproc-*" register ranges. Then we could have the iproc_nand driver bind against "brcm,iproc-nand", then call into the common probe, which would then accept/reject based on "brcm,brcmnand-vX.Y".

[2] The DT structure from [1] could actually accommodate either driver structure just fine. So maybe that means it's a better hardware description?

Quoted 8 lines

Yes, I think this makes sense overall. Regarding the specific example, can you clarify how the register areas in iproc are structured?

The 0xf8105408 and 0x18046f00 start addresses are not aligned to large powers of two, which often indicates that they are part of some other, larger, unit that might need to have a driver of its own, so before we specify a binding like the one you proposed above I'd like to make sure we're not getting ourselves into trouble later.

I may want the Cygnus guys to speak up here, partly for technical expertise and partly to know how much they care to share...

<0xf8105408 0x600>: covers a series of NAND_IDM registers. NAND has a few bits we don't care about (for debugging, logging, and resetting), as well as its interrupt enable bits. The adjacent blocks cover similar IDM blocks for other cores (SPI, PNOR, DDR), and they are similarly unaligned. Not sure why, exactly; probably just a compact layout.

<0x18046f00 0x20>: a series of 8 NAND interrupt registers, each word containing a single bit representing status/clear. There is nothing between the "nand" range and this range, and the SPI core register range follows.

So I think these are pretty clearly-delineated register ranges for NAND, and the alignment is not really missing anything. Adjacent hardware (e.g., SPI) is independent, though pieces look similar. For one, it has similar:

* interrupt enable bits in the IDM range (0xf8106408 to 0xf8106a00); and
* interrupt status/clear following the SPI block (0x180473a0 to 0x180473b8)

Brian

Quoted 38 lines · On Wednesday 06 May 2015 13:49:10 Brian Norris wrote:

Quoted 28 lines · Arnd Bergmann

Quoted 27 lines · On Wednesday 06 May 2015 10:59:50 Brian Norris wrote:

drivers/mtd/nand/brcmstb_nand.c +27 -0

+       /*
+        * Some SoCs integrate this controller (e.g., its interrupt bits) in
+        * interesting ways
+        */
+       if (of_property_read_bool(dn, "brcm,nand-soc")) {
+               struct device_node *soc_dn;
+
+               soc_dn = of_parse_phandle(dn, "brcm,nand-soc", 0);
+               if (!soc_dn)
+                       return -ENODEV;
+
+               ctrl->soc = devm_brcmnand_probe_soc(dev, soc_dn);
+               if (!ctrl->soc) {
+                       dev_err(dev, "could not probe SoC data\n");
+                       of_node_put(soc_dn);
+                       return -ENODEV;
+               }
+
+               ret = devm_request_irq(dev, ctrl->irq, brcmnand_irq, 0,
+                                      DRV_NAME, ctrl);
+
+               /* Enable interrupt */
+               ctrl->soc->ctlrdy_set_enabled(ctrl->soc, true);
+
+               of_node_put(soc_dn);
+       } else {
+               /* Use standard interrupt infrastructure */

Quoted 8 lines

drivers/mtd/nand/brcmstb_nand.c static int brcmnand_probe(struct platform_device *pdev) · line 2191

Quoted 4 lines

drivers/mtd/nand/brcmstb_nand.c +3 -0

+               ret = devm_request_irq(dev, ctrl->irq, brcmnand_ctlrdy_irq, 0,
+                                      DRV_NAME, ctrl);
+       }

It looks to me like this should be handled as a nested irqchip, so the node you look up gets used as the "interrupt-parent" instead, making the behavior of this SoC transparent to the nand driver.

Quoted 4 lines

You snipped the rest of the patch, which involves more than just IRQ handling. The same registers touch both interrupts and data bus endian configuration, so it can't possibly be done transparently to the NAND driver.

Anything else in there? The bus configuration would just involve writing a constant value in some register, right? Doing that in the irqchip is also a bit ugly, but may still be better overall than doing it the way you have above.

Hi,

This is version 3 of support for the Broadcom BCM7xxx Set-Top Box NAND controller. This controller has been used in a variety of Broadcom SoCs.

This series now adds support for a few new chips: BCM63138, and the iProc chip family. These add an additional 6 new patches to the original 4. If the only comments end up on the latter 6 patches, the first 4 might be worth merging independently.

Summary changelog:

v1 -> v2:
* add NAND to DTS for BCM7445 / BCM97445SVMB
* rename DT binding file to have 'brcm,' prefix
* catch DMA mapping errors
* fixup timeout / error messages (hex, remove misleading info)
* MODULE_LICENSE("GPL v2")
* fix incorrect comments
* print why we fail, when checking for supported controller revisions
* disable prefetch when using Flash DMA (see FIXME); will re-enable once we get a good erased-page verification scheme merged

v2 -> v3:
* rebase to v4.1-rc1
* add SoC-specific infrastructure, to help support other SoCs:
- add BCM63138 support
- add iProc/Cygnus support
* disable prefetch on v6.1

Brian Norris (8):
mtd: nand: add common DT init code
Documentation: devicetree: add binding doc for Broadcom NAND
controller
mtd: nand: add NAND driver for Broadcom STB NAND controller
ARM: bcm7445: add NAND to DTS
Documentation: devicetree: brcmstb_nand: add 'brcm,nand-soc' bindings
mtd: brcmstb_nand: add SoC-specific support
mtd: brcsmtb_nand_soc: add support for BCM63138
ARM: bcm63138: add NAND DT support

Ray Jui (2):
mtd: brcsmtb_nand_soc: add iProc support
ARM: dts: cygnus: Enable NAND support for Cygnus

 .../devicetree/bindings/mtd/brcm,brcmstb-nand.txt  |  147 ++
 arch/arm/boot/dts/bcm-cygnus.dtsi                  |   20 +
 arch/arm/boot/dts/bcm63138.dtsi                    |   17 +
 arch/arm/boot/dts/bcm7445-bcm97445svmb.dts         |   23 +
 arch/arm/boot/dts/bcm7445.dtsi                     |   22 +
 arch/arm/boot/dts/bcm958300k.dts                   |   16 +
 arch/arm/boot/dts/bcm963138dvt.dts                 |   12 +
 drivers/mtd/nand/Kconfig                           |    8 +
 drivers/mtd/nand/Makefile                          |    2 +
 drivers/mtd/nand/brcmnand.h                        |   56 +
 drivers/mtd/nand/brcmstb_nand.c                    | 2263 ++++++++++++++++++++
 drivers/mtd/nand/brcmstb_nand_soc.c                |  244 +++
 drivers/mtd/nand/nand_base.c                       |   41 +
 include/linux/mtd/nand.h                           |    5 +
 14 files changed, 2876 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/brcm,brcmstb-nand.txt
 create mode 100644 drivers/mtd/nand/brcmnand.h
 create mode 100644 drivers/mtd/nand/brcmstb_nand.c
 create mode 100644 drivers/mtd/nand/brcmstb_nand_soc.c