mtd: brcmnand: Add brcm,nand-bcm63268 device tree binding

The BCM63268 has a NAND interrupt register with combined status and enable registers.

The BCM63268 has a NAND interrupt register with combined status and enable registers. It also has a clock for the NAND controller that needs to be enabled.

 drivers/mtd/nand/brcmnand/bcm63268_nand.c | 171 ++++++++++++++++++++++++++++++

Add device tree binding for NAND on the BCM63268.

The BCM63268 has a NAND interrupt register with combined status and enable registers.

Signed-off-by Simon Arlott <[email protected]>

---
 .../devicetree/bindings/mtd/brcm,brcmnand.txt      | 33 ++++++++++++++++++++++
 1 file changed, 33 insertions(+)

Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt +0 -0

index 4ff7128..45c6e0c 100644

Quoted 2 lines

+ * "brcm,nand-bcm63268"
+ - compatible: should contain "brcm,nand-bcm<soc>", "brcm,nand-bcm63268"

vendor,<soc>-device is preferred.

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

+ * "brcm,nand-bcm63268"
+ - compatible: should contain "brcm,nand-bcm<soc>", "brcm,nand-bcm63268"

vendor,<soc>-device is preferred.

The BCM63268 has a NAND interrupt register with combined status and enable registers. It also has a clock for the NAND controller that needs to be enabled.

Set up the device by enabling the clock, disabling and acking all interrupts, then handle the CTRL_READY interrupt.

Add a "device_remove" function to struct brcmnand_soc so that the clock can be disabled when the device is removed.

Signed-off-by Simon Arlott <[email protected]>

---

Quoted 4 lines · Rob Herring

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

+ * "brcm,nand-bcm63268"
+ - compatible: should contain "brcm,nand-bcm<soc>", "brcm,nand-bcm63268"

vendor,<soc>-device is preferred.

The existing two bindings use brcm,nand-<soc>, but I've changed this one.

 drivers/mtd/nand/brcmnand/Makefile        |   1 +
 drivers/mtd/nand/brcmnand/bcm63268_nand.c | 174 ++++++++++++++++++++++++++++++
 drivers/mtd/nand/brcmnand/brcmnand.c      |   3 +
 drivers/mtd/nand/brcmnand/brcmnand.h      |   1 +
 4 files changed, 179 insertions(+)
 create mode 100644 drivers/mtd/nand/brcmnand/bcm63268_nand.c

drivers/mtd/nand/brcmnand/Makefile +1 -0

index 3b1fbfd..b83a9ae 100644
@@ -2,5 +2,6 @@
 # more specific iproc_nand.o, for instance
 obj-$(CONFIG_MTD_NAND_BRCMNAND)                += iproc_nand.o
 obj-$(CONFIG_MTD_NAND_BRCMNAND)                += bcm63138_nand.o
+obj-$(CONFIG_MTD_NAND_BRCMNAND)                += bcm63268_nand.o
 obj-$(CONFIG_MTD_NAND_BRCMNAND)                += brcmstb_nand.o
 obj-$(CONFIG_MTD_NAND_BRCMNAND)                += brcmnand.o

drivers/mtd/nand/brcmnand/bcm63268_nand.c new file +117 -0

new file mode 100644
index 0000000..88b32fa
@@ -0,0 +1,174 @@
+/*
+ * Copyright 2015 Simon Arlott
+ *
+ * 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.
+ *
+ * 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.
+ *
+ * Derived from bcm63138_nand.c:
+ * Copyright © 2015 Broadcom Corporation
+ *
+ * Derived from bcm963xx_4.12L.06B_consumer/shared/opensource/include/bcm963xx/63268_map_part.h:
+ * Copyright 2000-2010 Broadcom Corporation
+ *
+ * Derived from bcm963xx_4.12L.06B_consumer/shared/opensource/flash/nandflash.c:
+ * Copyright 2000-2010 Broadcom Corporation
+ */
+
+#include <linux/device.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include "brcmnand.h"
+
+struct bcm63268_nand_soc {
+       struct brcmnand_soc soc;
+       void __iomem *base;
+       struct clk *clk;
+};
+
+#define BCM63268_NAND_INT              0x00
+#define  BCM63268_NAND_STATUS_SHIFT    0
+#define  BCM63268_NAND_STATUS_MASK     (0xfff << BCM63268_NAND_STATUS_SHIFT)
+#define  BCM63268_NAND_ENABLE_SHIFT    16
+#define  BCM63268_NAND_ENABLE_MASK     (0xffff << BCM63268_NAND_ENABLE_SHIFT)
+#define BCM63268_NAND_BASE_ADDR0       0x04
+#define BCM63268_NAND_BASE_ADDR1       0x0c
+
+enum {
+       BCM63268_NP_READ        = BIT(0),
+       BCM63268_BLOCK_ERASE    = BIT(1),
+       BCM63268_COPY_BACK      = BIT(2),
+       BCM63268_PAGE_PGM       = BIT(3),
+       BCM63268_CTRL_READY     = BIT(4),
+       BCM63268_DEV_RBPIN      = BIT(5),
+       BCM63268_ECC_ERR_UNC    = BIT(6),
+       BCM63268_ECC_ERR_CORR   = BIT(7),
+};
+
+static bool bcm63268_nand_intc_ack(struct brcmnand_soc *soc)
+{
+       struct bcm63268_nand_soc *priv =
+                       container_of(soc, struct bcm63268_nand_soc, soc);
+       void __iomem *mmio = priv->base + BCM63268_NAND_INT;
+       u32 val = brcmnand_readl(mmio);
+
+       if (val & (BCM63268_CTRL_READY << BCM63268_NAND_STATUS_SHIFT)) {
+               /* Ack interrupt */
+               val &= ~BCM63268_NAND_STATUS_MASK;
+               val |= BCM63268_CTRL_READY << BCM63268_NAND_STATUS_SHIFT;
+               brcmnand_writel(val, mmio);
+               return true;
+       }
+
+       return false;
+}
+
+static void bcm63268_nand_intc_set(struct brcmnand_soc *soc, bool en)
+{
+       struct bcm63268_nand_soc *priv =
+                       container_of(soc, struct bcm63268_nand_soc, soc);
+       void __iomem *mmio = priv->base + BCM63268_NAND_INT;
+       u32 val = brcmnand_readl(mmio);
+
+       /* Don't ack any interrupts */
+       val &= ~BCM63268_NAND_STATUS_MASK;
+
+       if (en)
+               val |= BCM63268_CTRL_READY << BCM63268_NAND_ENABLE_SHIFT;
+       else
+               val &= ~(BCM63268_CTRL_READY << BCM63268_NAND_ENABLE_SHIFT);
+
+       brcmnand_writel(val, mmio);
+}
+
+static void bcm63268_nand_remove(struct brcmnand_soc *soc)
+{
+       struct bcm63268_nand_soc *priv =
+                       container_of(soc, struct bcm63268_nand_soc, soc);
+
+       clk_disable_unprepare(priv->clk);
+       clk_put(priv->clk);
+}
+
+static int bcm63268_nand_probe(struct platform_device *pdev)
+{
+       struct device *dev = &pdev->dev;
+       struct bcm63268_nand_soc *priv;
+       struct brcmnand_soc *soc;
+       struct resource *res;
+       int ret;
+
+       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+       if (!priv)
+               return -ENOMEM;
+       soc = &priv->soc;
+

drivers/mtd/nand/brcmnand/bcm63268_nand.c new file +47 -0

+       if (IS_ERR(priv->clk))
+               return PTR_ERR(priv->clk);
+
+       ret = clk_prepare_enable(priv->clk);
+       if (ret) {
+               clk_put(priv->clk);
+               return ret;
+       }
+
+       soc->ctlrdy_ack = bcm63268_nand_intc_ack;
+       soc->ctlrdy_set_enabled = bcm63268_nand_intc_set;
+       soc->remove_device = bcm63268_nand_remove;
+
+       /* Disable and ack all interrupts  */
+       brcmnand_writel(0, priv->base + BCM63268_NAND_INT);
+       brcmnand_writel(BCM63268_NAND_STATUS_MASK,
+                       priv->base + BCM63268_NAND_INT);
+
+       ret = brcmnand_probe(pdev, soc);
+       if (ret) {
+               clk_disable_unprepare(priv->clk);
+               clk_put(priv->clk);
+       }
+
+       return ret;
+}
+
+static const struct of_device_id bcm63268_nand_of_match[] = {
+       { .compatible = "brcm,bcm63268-nand" },
+       {},
+};
+MODULE_DEVICE_TABLE(of, bcm63268_nand_of_match);
+
+static struct platform_driver bcm63268_nand_driver = {
+       .probe                  = bcm63268_nand_probe,
+       .remove                 = brcmnand_remove,
+       .driver = {
+               .name           = "bcm63268_nand",
+               .pm             = &brcmnand_pm_ops,
+               .of_match_table = bcm63268_nand_of_match,
+       }
+};
+module_platform_driver(bcm63268_nand_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Simon Arlott");
+MODULE_DESCRIPTION("NAND driver for BCM63268");

drivers/mtd/nand/brcmnand/brcmnand.c +0 -0

index 2c8f67f..7b4988f 100644

drivers/mtd/nand/brcmnand/brcmnand.c +0 -0

        dev_set_drvdata(&pdev->dev, NULL);
 
        return 0;

drivers/mtd/nand/brcmnand/brcmnand.h +0 -0

index ef5eabb..5c5dc2d 100644

Quoted 1 line · Simon Arlott

+ priv->clk = of_clk_get(dev->of_node, 0);

Why not use a named clock here? That way you can make use of devm_clk_get and don't need to care about putting it.

Quoted 23 lines

drivers/mtd/nand/brcmnand/bcm63268_nand.c new file +15 -0

+
+static struct platform_driver bcm63268_nand_driver = {
+       .probe                  = bcm63268_nand_probe,
+       .remove                 = brcmnand_remove,
+       .driver = {
+               .name           = "bcm63268_nand",
+               .pm             = &brcmnand_pm_ops,
+               .of_match_table = bcm63268_nand_of_match,
+       }
+};
+module_platform_driver(bcm63268_nand_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Simon Arlott");
+MODULE_DESCRIPTION("NAND driver for BCM63268");

drivers/mtd/nand/brcmnand/brcmnand.c +0 -0

index 2c8f67f..7b4988f 100644
@@ -2270,6 +2270,9 @@ int brcmnand_remove(struct platform_device *pdev)
        list_for_each_entry(host, &ctrl->host_list, node)
                nand_release(&host->mtd);
 

drivers/mtd/nand/brcmnand/brcmnand.c int brcmnand_remove(struct platform_device *pdev) · line 2275

Quoted 3 lines

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

+       if (ctrl->soc && ctrl->soc->remove_device)
+               ctrl->soc->remove_device(ctrl->soc);
+

This is a bit weird, why don't you just use your own .remove callback for the driver that like you did for .probe? then you won't need to add a remove_device callback at all.

Quoted 8 lines

drivers/mtd/nand/brcmnand/brcmnand.c +0 -0

        dev_set_drvdata(&pdev->dev, NULL);
 
        return 0;

drivers/mtd/nand/brcmnand/brcmnand.h +0 -0

index ef5eabb..5c5dc2d 100644

drivers/mtd/nand/brcmnand/brcmnand.h struct brcmnand_soc { · line 30

Quoted 7 lines

drivers/mtd/nand/brcmnand/brcmnand.h +1 -0

@@ -24,6 +24,7 @@ struct brcmnand_soc {
        bool (*ctlrdy_ack)(struct brcmnand_soc *soc);
        void (*ctlrdy_set_enabled)(struct brcmnand_soc *soc, bool en);
        void (*prepare_data_bus)(struct brcmnand_soc *soc, bool prepare);
+       void (*remove_device)(struct brcmnand_soc *soc);
 };
 
 static inline void brcmnand_soc_data_bus_prepare(struct brcmnand_soc *soc)

Jonas

The BCM63268 has a NAND interrupt register with combined status and enable registers. It also has a clock for the NAND controller that needs to be enabled.

Set up the device by enabling the clock, disabling and acking all interrupts, then handle the CTRL_READY interrupt.

Add a "device_remove" function to struct brcmnand_soc so that the clock can be disabled when the device is removed.

Signed-off-by Simon Arlott <[email protected]>

---

Quoted 4 lines · Rob Herring

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

+ * "brcm,nand-bcm63268"
+ - compatible: should contain "brcm,nand-bcm<soc>", "brcm,nand-bcm63268"

vendor,<soc>-device is preferred.

The existing two bindings use brcm,nand-<soc>, but I've changed this one.

Quoted 16 lines · Simon Arlott

The BCM63268 has a NAND interrupt register with combined status and enable registers. It also has a clock for the NAND controller that needs to be enabled.

Set up the device by enabling the clock, disabling and acking all interrupts, then handle the CTRL_READY interrupt.

Add a "device_remove" function to struct brcmnand_soc so that the clock can be disabled when the device is removed.

Signed-off-by Simon Arlott <[email protected]>

---

Quoted 3 lines · Rob Herring

Quoted 3 lines

Quoted 2 lines

Quoted 2 lines

+ * "brcm,nand-bcm63268"
+ - compatible: should contain "brcm,nand-bcm<soc>", "brcm,nand-bcm63268"

Quoted 3 lines

Quoted 1 line

Quoted 1 line

vendor,<soc>-device is preferred.

The existing two bindings use brcm,nand-<soc>, but I've changed this one.

Could we stick with the existing binding naming convention of using:

brcm,nand-<soc> just so automated tools or other things can match this one too, and +1 for consistency?

Other than, that, same comment as Jonas, why do we we need the device_remove callback to be called from the main driver down to this one?

Quoted 21 lines · Florian Fainelli

Quoted 19 lines · Simon Arlott

The BCM63268 has a NAND interrupt register with combined status and enable registers. It also has a clock for the NAND controller that needs to be enabled.

Set up the device by enabling the clock, disabling and acking all interrupts, then handle the CTRL_READY interrupt.

Add a "device_remove" function to struct brcmnand_soc so that the clock can be disabled when the device is removed.

Signed-off-by Simon Arlott <[email protected]>

---

Quoted 4 lines · Rob Herring

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

+ * "brcm,nand-bcm63268"
+ - compatible: should contain "brcm,nand-bcm<soc>", "brcm,nand-bcm63268"

vendor,<soc>-device is preferred.

The existing two bindings use brcm,nand-<soc>, but I've changed this one.

Quoted 4 lines

Could we stick with the existing binding naming convention of using:

brcm,nand-<soc> just so automated tools or other things can match this one too, and +1 for consistency?

I could submit another patch renaming the existing bindings to brcm,<soc>-nand, and add that to the drivers? Then they'd be consistent.

Quoted 3 lines

Other than, that, same comment as Jonas, why do we we need the device_remove callback to be called from the main driver down to this one?

I'll add a "struct brcmnand_soc *brcmnand_get_socdata(struct device *)" instead so that I can access the soc data before calling brcmnand_remove.

The BCM63268 has a NAND interrupt register with combined status and enable registers. It also has a clock for the NAND controller that needs to be enabled.

Set up the device by enabling the clock, disabling and acking all interrupts, then handle the CTRL_READY interrupt.

Add a brcmnand_get_socdata() function so that bcm63268_nand can obtain its data and disable the clock when the device is removed.

Signed-off-by Simon Arlott <[email protected]>

---
 drivers/mtd/nand/brcmnand/Makefile        |   1 +
 drivers/mtd/nand/brcmnand/bcm63268_nand.c | 179 ++++++++++++++++++++++++++++++
 drivers/mtd/nand/brcmnand/brcmnand.c      |   7 ++
 drivers/mtd/nand/brcmnand/brcmnand.h      |   1 +
 4 files changed, 188 insertions(+)
 create mode 100644 drivers/mtd/nand/brcmnand/bcm63268_nand.c

drivers/mtd/nand/brcmnand/brcmnand.c +0 -0

index 2c8f67f..99ca69e 100644

Quoted 9 lines · Simon Arlott

The BCM63268 has a NAND interrupt register with combined status and enable registers. It also has a clock for the NAND controller that needs to be enabled.

Set up the device by enabling the clock, disabling and acking all interrupts, then handle the CTRL_READY interrupt.

Add a brcmnand_get_socdata() function so that bcm63268_nand can obtain its data and disable the clock when the device is removed.

To me this now mostly looks good, one thing though ... (snip)

Quoted 8 lines

drivers/mtd/nand/brcmnand/brcmnand.c +0 -0

index 2c8f67f..99ca69e 100644
@@ -2262,6 +2262,13 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
 }
 EXPORT_SYMBOL_GPL(brcmnand_probe);
 

drivers/mtd/nand/brcmnand/brcmnand.c int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc) · line 2271

Quoted 7 lines

drivers/mtd/nand/brcmnand/brcmnand.c +7 -0

+struct brcmnand_soc *brcmnand_get_socdata(struct platform_device *pdev)
+{
+       struct brcmnand_controller *ctrl = dev_get_drvdata(&pdev->dev);
+
+       return ctrl ? ctrl->soc : NULL;
+}
+

Don't you need to EXPORT_SYMBOL_GPL this one in case you build brcmnand as module?

Quoted 21 lines · Jonas Gorski

Quoted 9 lines · Simon Arlott

The BCM63268 has a NAND interrupt register with combined status and enable registers. It also has a clock for the NAND controller that needs to be enabled.

Set up the device by enabling the clock, disabling and acking all interrupts, then handle the CTRL_READY interrupt.

Add a brcmnand_get_socdata() function so that bcm63268_nand can obtain its data and disable the clock when the device is removed.

To me this now mostly looks good, one thing though ... (snip)

Quoted 8 lines

drivers/mtd/nand/brcmnand/brcmnand.c +0 -0

index 2c8f67f..99ca69e 100644
@@ -2262,6 +2262,13 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
 }
 EXPORT_SYMBOL_GPL(brcmnand_probe);
 

Quoted 10 lines

drivers/mtd/nand/brcmnand/brcmnand.c int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc) · line 2271

Quoted 7 lines

drivers/mtd/nand/brcmnand/brcmnand.c +7 -0

+struct brcmnand_soc *brcmnand_get_socdata(struct platform_device *pdev)
+{
+       struct brcmnand_controller *ctrl = dev_get_drvdata(&pdev->dev);
+
+       return ctrl ? ctrl->soc : NULL;
+}
+

Don't you need to EXPORT_SYMBOL_GPL this one in case you build brcmnand as module?

No, because all of the code is built into the one object file:

obj-$(CONFIG_MTD_NAND_BRCMNAND)         += iproc_nand.o
obj-$(CONFIG_MTD_NAND_BRCMNAND)         += bcm63138_nand.o
obj-$(CONFIG_MTD_NAND_BRCMNAND)         += bcm63268_nand.o
obj-$(CONFIG_MTD_NAND_BRCMNAND)         += brcmstb_nand.o
obj-$(CONFIG_MTD_NAND_BRCMNAND)         += brcmnand.o

The BCM63268 has a NAND interrupt register with combined status and enable registers. It also has a clock for the NAND controller that needs to be enabled.

Set up the device by enabling the clock, disabling and acking all interrupts, then handle the CTRL_READY interrupt.

Add a brcmnand_get_socdata() function so that bcm63268_nand can obtain its data and disable the clock when the device is removed.

Signed-off-by Simon Arlott <[email protected]>

---
Added EXPORT_SYMBOL_GPL(brcmnand_get_socdata)

(As the brcmnand module must be loaded first its compatible string will apply to any existing devices before the soc-specific module can be loaded.)

 drivers/mtd/nand/brcmnand/Makefile        |   1 +
 drivers/mtd/nand/brcmnand/bcm63268_nand.c | 179 ++++++++++++++++++++++++++++++
 drivers/mtd/nand/brcmnand/brcmnand.c      |   8 ++
 drivers/mtd/nand/brcmnand/brcmnand.h      |   1 +
 4 files changed, 189 insertions(+)
 create mode 100644 drivers/mtd/nand/brcmnand/bcm63268_nand.c

drivers/mtd/nand/brcmnand/Makefile +1 -0

index 3b1fbfd..b83a9ae 100644
@@ -2,5 +2,6 @@
 # more specific iproc_nand.o, for instance
 obj-$(CONFIG_MTD_NAND_BRCMNAND)		+= iproc_nand.o
 obj-$(CONFIG_MTD_NAND_BRCMNAND)		+= bcm63138_nand.o
+obj-$(CONFIG_MTD_NAND_BRCMNAND)		+= bcm63268_nand.o
 obj-$(CONFIG_MTD_NAND_BRCMNAND)		+= brcmstb_nand.o
 obj-$(CONFIG_MTD_NAND_BRCMNAND)		+= brcmnand.o

drivers/mtd/nand/brcmnand/bcm63268_nand.c new file +110 -0

new file mode 100644
index 0000000..70ad907
@@ -0,0 +1,179 @@
+/*
+ * Copyright 2015 Simon Arlott
+ *
+ * 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.
+ *
+ * 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.
+ *
+ * Derived from bcm63138_nand.c:
+ * Copyright © 2015 Broadcom Corporation
+ *
+ * Derived from bcm963xx_4.12L.06B_consumer/shared/opensource/include/bcm963xx/63268_map_part.h:
+ * Copyright 2000-2010 Broadcom Corporation
+ *
+ * Derived from bcm963xx_4.12L.06B_consumer/shared/opensource/flash/nandflash.c:
+ * Copyright 2000-2010 Broadcom Corporation
+ */
+
+#include <linux/device.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include "brcmnand.h"
+
+struct bcm63268_nand_soc {
+	struct brcmnand_soc soc;
+	void __iomem *base;
+	struct clk *clk;
+};
+
+#define BCM63268_NAND_INT		0x00
+#define  BCM63268_NAND_STATUS_SHIFT	0
+#define  BCM63268_NAND_STATUS_MASK	(0xfff << BCM63268_NAND_STATUS_SHIFT)
+#define  BCM63268_NAND_ENABLE_SHIFT	16
+#define  BCM63268_NAND_ENABLE_MASK	(0xffff << BCM63268_NAND_ENABLE_SHIFT)
+#define BCM63268_NAND_BASE_ADDR0	0x04
+#define BCM63268_NAND_BASE_ADDR1	0x0c
+
+enum {
+	BCM63268_NP_READ	= BIT(0),
+	BCM63268_BLOCK_ERASE	= BIT(1),
+	BCM63268_COPY_BACK	= BIT(2),
+	BCM63268_PAGE_PGM	= BIT(3),
+	BCM63268_CTRL_READY	= BIT(4),
+	BCM63268_DEV_RBPIN	= BIT(5),
+	BCM63268_ECC_ERR_UNC	= BIT(6),
+	BCM63268_ECC_ERR_CORR	= BIT(7),
+};
+
+static bool bcm63268_nand_intc_ack(struct brcmnand_soc *soc)
+{
+	struct bcm63268_nand_soc *priv =
+			container_of(soc, struct bcm63268_nand_soc, soc);
+	void __iomem *mmio = priv->base + BCM63268_NAND_INT;
+	u32 val = brcmnand_readl(mmio);
+
+	if (val & (BCM63268_CTRL_READY << BCM63268_NAND_STATUS_SHIFT)) {
+		/* Ack interrupt */
+		val &= ~BCM63268_NAND_STATUS_MASK;
+		val |= BCM63268_CTRL_READY << BCM63268_NAND_STATUS_SHIFT;
+		brcmnand_writel(val, mmio);
+		return true;
+	}
+
+	return false;
+}
+
+static void bcm63268_nand_intc_set(struct brcmnand_soc *soc, bool en)
+{
+	struct bcm63268_nand_soc *priv =
+			container_of(soc, struct bcm63268_nand_soc, soc);
+	void __iomem *mmio = priv->base + BCM63268_NAND_INT;
+	u32 val = brcmnand_readl(mmio);
+
+	/* Don't ack any interrupts */
+	val &= ~BCM63268_NAND_STATUS_MASK;
+
+	if (en)
+		val |= BCM63268_CTRL_READY << BCM63268_NAND_ENABLE_SHIFT;
+	else
+		val &= ~(BCM63268_CTRL_READY << BCM63268_NAND_ENABLE_SHIFT);
+
+	brcmnand_writel(val, mmio);
+}
+
+static int bcm63268_nand_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct bcm63268_nand_soc *priv;
+	struct brcmnand_soc *soc;
+	struct resource *res;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+	soc = &priv->soc;
+
+	res = platform_get_resource_byname(pdev,
+		IORESOURCE_MEM, "nand-intr-base");

drivers/mtd/nand/brcmnand/bcm63268_nand.c new file +59 -0

+
+	ret = clk_prepare_enable(priv->clk);
+	if (ret)
+		return ret;
+
+	soc->ctlrdy_ack = bcm63268_nand_intc_ack;
+	soc->ctlrdy_set_enabled = bcm63268_nand_intc_set;
+
+	/* Disable and ack all interrupts  */
+	brcmnand_writel(0, priv->base + BCM63268_NAND_INT);
+	brcmnand_writel(BCM63268_NAND_STATUS_MASK,
+			priv->base + BCM63268_NAND_INT);
+
+	ret = brcmnand_probe(pdev, soc);
+	if (ret)
+		clk_disable_unprepare(priv->clk);
+
+	return ret;
+}
+
+static int bcm63268_nand_remove(struct platform_device *pdev)
+{
+	struct brcmnand_soc *soc = brcmnand_get_socdata(pdev);
+	struct bcm63268_nand_soc *priv = NULL;
+	int ret;
+
+	if (soc)
+		priv = container_of(soc, struct bcm63268_nand_soc, soc);
+
+	ret = brcmnand_remove(pdev);
+	if (ret)
+		return ret;
+
+	if (priv)
+		clk_disable_unprepare(priv->clk);
+
+	return 0;
+}
+
+static const struct of_device_id bcm63268_nand_of_match[] = {
+	{ .compatible = "brcm,nand-bcm63268" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, bcm63268_nand_of_match);
+
+static struct platform_driver bcm63268_nand_driver = {
+	.probe			= bcm63268_nand_probe,
+	.remove			= bcm63268_nand_remove,
+	.driver = {
+		.name		= "bcm63268_nand",
+		.pm		= &brcmnand_pm_ops,
+		.of_match_table	= bcm63268_nand_of_match,
+	}
+};
+module_platform_driver(bcm63268_nand_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Simon Arlott");
+MODULE_DESCRIPTION("NAND driver for BCM63268");

drivers/mtd/nand/brcmnand/brcmnand.c +0 -0

index 2c8f67f..5f26b8a 100644

drivers/mtd/nand/brcmnand/brcmnand.c +1 -0

+
 int brcmnand_remove(struct platform_device *pdev)
 {
 	struct brcmnand_controller *ctrl = dev_get_drvdata(&pdev->dev);

drivers/mtd/nand/brcmnand/brcmnand.h +0 -0

index ef5eabb..6680419 100644

+ Broadcom list + Kamal

Hi Simon,

Quoted 14 lines · Simon Arlott

The BCM63268 has a NAND interrupt register with combined status and enable registers. It also has a clock for the NAND controller that needs to be enabled.

Set up the device by enabling the clock, disabling and acking all interrupts, then handle the CTRL_READY interrupt.

Add a brcmnand_get_socdata() function so that bcm63268_nand can obtain its data and disable the clock when the device is removed.

Signed-off-by Simon Arlott <[email protected]>

---
Added EXPORT_SYMBOL_GPL(brcmnand_get_socdata)

Quoted 3 lines

(As the brcmnand module must be loaded first its compatible string will apply to any existing devices before the soc-specific module can be loaded.)

What's this comment supposed to mean? The brcmnand module will not directly probe any devices. It doesn't register any driver structs by itself.

(BTW given that, it probably doesn't need its MODULE_DEVICE_TABLE.)

Quoted 141 lines

 drivers/mtd/nand/brcmnand/Makefile        |   1 +
 drivers/mtd/nand/brcmnand/bcm63268_nand.c | 179 ++++++++++++++++++++++++++++++
 drivers/mtd/nand/brcmnand/brcmnand.c      |   8 ++
 drivers/mtd/nand/brcmnand/brcmnand.h      |   1 +
 4 files changed, 189 insertions(+)
 create mode 100644 drivers/mtd/nand/brcmnand/bcm63268_nand.c

drivers/mtd/nand/brcmnand/Makefile +1 -0

index 3b1fbfd..b83a9ae 100644
@@ -2,5 +2,6 @@
 # more specific iproc_nand.o, for instance
 obj-$(CONFIG_MTD_NAND_BRCMNAND)		+= iproc_nand.o
 obj-$(CONFIG_MTD_NAND_BRCMNAND)		+= bcm63138_nand.o
+obj-$(CONFIG_MTD_NAND_BRCMNAND)		+= bcm63268_nand.o
 obj-$(CONFIG_MTD_NAND_BRCMNAND)		+= brcmstb_nand.o
 obj-$(CONFIG_MTD_NAND_BRCMNAND)		+= brcmnand.o

drivers/mtd/nand/brcmnand/bcm63268_nand.c new file +117 -0

new file mode 100644
index 0000000..70ad907
@@ -0,0 +1,179 @@
+/*
+ * Copyright 2015 Simon Arlott
+ *
+ * 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.
+ *
+ * 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.
+ *
+ * Derived from bcm63138_nand.c:
+ * Copyright © 2015 Broadcom Corporation
+ *
+ * Derived from bcm963xx_4.12L.06B_consumer/shared/opensource/include/bcm963xx/63268_map_part.h:
+ * Copyright 2000-2010 Broadcom Corporation
+ *
+ * Derived from bcm963xx_4.12L.06B_consumer/shared/opensource/flash/nandflash.c:
+ * Copyright 2000-2010 Broadcom Corporation
+ */
+
+#include <linux/device.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include "brcmnand.h"
+
+struct bcm63268_nand_soc {
+	struct brcmnand_soc soc;
+	void __iomem *base;
+	struct clk *clk;
+};
+
+#define BCM63268_NAND_INT		0x00
+#define  BCM63268_NAND_STATUS_SHIFT	0
+#define  BCM63268_NAND_STATUS_MASK	(0xfff << BCM63268_NAND_STATUS_SHIFT)
+#define  BCM63268_NAND_ENABLE_SHIFT	16
+#define  BCM63268_NAND_ENABLE_MASK	(0xffff << BCM63268_NAND_ENABLE_SHIFT)
+#define BCM63268_NAND_BASE_ADDR0	0x04
+#define BCM63268_NAND_BASE_ADDR1	0x0c
+
+enum {
+	BCM63268_NP_READ	= BIT(0),
+	BCM63268_BLOCK_ERASE	= BIT(1),
+	BCM63268_COPY_BACK	= BIT(2),
+	BCM63268_PAGE_PGM	= BIT(3),
+	BCM63268_CTRL_READY	= BIT(4),
+	BCM63268_DEV_RBPIN	= BIT(5),
+	BCM63268_ECC_ERR_UNC	= BIT(6),
+	BCM63268_ECC_ERR_CORR	= BIT(7),
+};
+
+static bool bcm63268_nand_intc_ack(struct brcmnand_soc *soc)
+{
+	struct bcm63268_nand_soc *priv =
+			container_of(soc, struct bcm63268_nand_soc, soc);
+	void __iomem *mmio = priv->base + BCM63268_NAND_INT;
+	u32 val = brcmnand_readl(mmio);
+
+	if (val & (BCM63268_CTRL_READY << BCM63268_NAND_STATUS_SHIFT)) {
+		/* Ack interrupt */
+		val &= ~BCM63268_NAND_STATUS_MASK;
+		val |= BCM63268_CTRL_READY << BCM63268_NAND_STATUS_SHIFT;
+		brcmnand_writel(val, mmio);
+		return true;
+	}
+
+	return false;
+}
+
+static void bcm63268_nand_intc_set(struct brcmnand_soc *soc, bool en)
+{
+	struct bcm63268_nand_soc *priv =
+			container_of(soc, struct bcm63268_nand_soc, soc);
+	void __iomem *mmio = priv->base + BCM63268_NAND_INT;
+	u32 val = brcmnand_readl(mmio);
+
+	/* Don't ack any interrupts */
+	val &= ~BCM63268_NAND_STATUS_MASK;
+
+	if (en)
+		val |= BCM63268_CTRL_READY << BCM63268_NAND_ENABLE_SHIFT;
+	else
+		val &= ~(BCM63268_CTRL_READY << BCM63268_NAND_ENABLE_SHIFT);
+
+	brcmnand_writel(val, mmio);
+}
+
+static int bcm63268_nand_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct bcm63268_nand_soc *priv;
+	struct brcmnand_soc *soc;
+	struct resource *res;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+	soc = &priv->soc;
+
+	res = platform_get_resource_byname(pdev,
+		IORESOURCE_MEM, "nand-intr-base");
+	if (!res)
+		return -EINVAL;
+
+	priv->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+

drivers/mtd/nand/brcmnand/bcm63268_nand.c line 120

Quoted 3 lines

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

+	priv->clk = devm_clk_get(&pdev->dev, "nand");
+	if (IS_ERR(priv->clk))
+		return PTR_ERR(priv->clk);

Perhaps we should put this clock handling in brcmnand.c? Just have it treat the clock as optional (i.e., ignore errors except for EPROBE_DEFER?), so we don't fail if no clock was provided? This could help other platforms too, if they gain clock support.

If we do this, you'll want to document the clock in the common binding, not the bcm63268-specific part.

Also, could it help to disable/enable the clock during suspend/resume? If you move it to brcmnand.c, this would also be pretty simple.

Quoted 67 lines

drivers/mtd/nand/brcmnand/bcm63268_nand.c new file +59 -0

+
+	ret = clk_prepare_enable(priv->clk);
+	if (ret)
+		return ret;
+
+	soc->ctlrdy_ack = bcm63268_nand_intc_ack;
+	soc->ctlrdy_set_enabled = bcm63268_nand_intc_set;
+
+	/* Disable and ack all interrupts  */
+	brcmnand_writel(0, priv->base + BCM63268_NAND_INT);
+	brcmnand_writel(BCM63268_NAND_STATUS_MASK,
+			priv->base + BCM63268_NAND_INT);
+
+	ret = brcmnand_probe(pdev, soc);
+	if (ret)
+		clk_disable_unprepare(priv->clk);
+
+	return ret;
+}
+
+static int bcm63268_nand_remove(struct platform_device *pdev)
+{
+	struct brcmnand_soc *soc = brcmnand_get_socdata(pdev);
+	struct bcm63268_nand_soc *priv = NULL;
+	int ret;
+
+	if (soc)
+		priv = container_of(soc, struct bcm63268_nand_soc, soc);
+
+	ret = brcmnand_remove(pdev);
+	if (ret)
+		return ret;
+
+	if (priv)
+		clk_disable_unprepare(priv->clk);
+
+	return 0;
+}
+
+static const struct of_device_id bcm63268_nand_of_match[] = {
+	{ .compatible = "brcm,nand-bcm63268" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, bcm63268_nand_of_match);
+
+static struct platform_driver bcm63268_nand_driver = {
+	.probe			= bcm63268_nand_probe,
+	.remove			= bcm63268_nand_remove,
+	.driver = {
+		.name		= "bcm63268_nand",
+		.pm		= &brcmnand_pm_ops,
+		.of_match_table	= bcm63268_nand_of_match,
+	}
+};
+module_platform_driver(bcm63268_nand_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Simon Arlott");
+MODULE_DESCRIPTION("NAND driver for BCM63268");

drivers/mtd/nand/brcmnand/brcmnand.c +0 -0

index 2c8f67f..5f26b8a 100644
@@ -2262,6 +2262,14 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
 }
 EXPORT_SYMBOL_GPL(brcmnand_probe);
 

drivers/mtd/nand/brcmnand/brcmnand.c int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc) · line 2271

Quoted 7 lines

drivers/mtd/nand/brcmnand/brcmnand.c +7 -0

+struct brcmnand_soc *brcmnand_get_socdata(struct platform_device *pdev)
+{
+	struct brcmnand_controller *ctrl = dev_get_drvdata(&pdev->dev);
+
+	return ctrl ? ctrl->soc : NULL;
+}
+EXPORT_SYMBOL_GPL(brcmnand_get_socdata);

If you move the clk handling to the core brcmnand.c, then you won't need this still.

Quoted 9 lines

drivers/mtd/nand/brcmnand/brcmnand.c +1 -0

+
 int brcmnand_remove(struct platform_device *pdev)
 {
 	struct brcmnand_controller *ctrl = dev_get_drvdata(&pdev->dev);

drivers/mtd/nand/brcmnand/brcmnand.h +0 -0

index ef5eabb..6680419 100644

drivers/mtd/nand/brcmnand/brcmnand.h static inline void brcmnand_writel(u32 val, void __iomem *addr) · line 71

Quoted 7 lines

drivers/mtd/nand/brcmnand/brcmnand.h +1 -0

@@ -65,6 +65,7 @@ static inline void brcmnand_writel(u32 val, void __iomem *addr)
 
 int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc);
 int brcmnand_remove(struct platform_device *pdev);
+struct brcmnand_soc *brcmnand_get_socdata(struct platform_device *pdev);
 
 extern const struct dev_pm_ops brcmnand_pm_ops;
 

Brian

Quoted 23 lines · Brian Norris

Quoted 22 lines · Simon Arlott

drivers/mtd/nand/brcmnand/bcm63268_nand.c new file +22 -0

+static int bcm63268_nand_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct bcm63268_nand_soc *priv;
+	struct brcmnand_soc *soc;
+	struct resource *res;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+	soc = &priv->soc;
+
+	res = platform_get_resource_byname(pdev,
+		IORESOURCE_MEM, "nand-intr-base");
+	if (!res)
+		return -EINVAL;
+
+	priv->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+

Quoted 8 lines

drivers/mtd/nand/brcmnand/bcm63268_nand.c line 120

Quoted 3 lines

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

+	priv->clk = devm_clk_get(&pdev->dev, "nand");
+	if (IS_ERR(priv->clk))
+		return PTR_ERR(priv->clk);

Perhaps we should put this clock handling in brcmnand.c? Just have it treat the clock as optional (i.e., ignore errors except for EPROBE_DEFER?), so we don't fail if no clock was provided? This could help other platforms too, if they gain clock support.

Unless most soc variants have clocks I'd prefer to leave it in this module.

Quoted 5 lines

If we do this, you'll want to document the clock in the common binding, not the bcm63268-specific part.

Also, could it help to disable/enable the clock during suspend/resume? If you move it to brcmnand.c, this would also be pretty simple.

Alternatively, it could proxy the brcmnand_pm_ops functions. I don't have any way to test suspend/resume.

Quoted 28 lines

Quoted 28 lines

drivers/mtd/nand/brcmnand/bcm63268_nand.c new file +19 -0

+
+	ret = clk_prepare_enable(priv->clk);
+	if (ret)
+		return ret;
+
+	soc->ctlrdy_ack = bcm63268_nand_intc_ack;
+	soc->ctlrdy_set_enabled = bcm63268_nand_intc_set;
+
+	/* Disable and ack all interrupts  */
+	brcmnand_writel(0, priv->base + BCM63268_NAND_INT);
+	brcmnand_writel(BCM63268_NAND_STATUS_MASK,
+			priv->base + BCM63268_NAND_INT);
+
+	ret = brcmnand_probe(pdev, soc);
+	if (ret)
+		clk_disable_unprepare(priv->clk);
+
+	return ret;
+}

drivers/mtd/nand/brcmnand/brcmnand.c +0 -0

index 2c8f67f..5f26b8a 100644
@@ -2262,6 +2262,14 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
 }
 EXPORT_SYMBOL_GPL(brcmnand_probe);
 

Quoted 10 lines

drivers/mtd/nand/brcmnand/brcmnand.c int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc) · line 2271

Quoted 7 lines

drivers/mtd/nand/brcmnand/brcmnand.c +7 -0

+struct brcmnand_soc *brcmnand_get_socdata(struct platform_device *pdev)
+{
+	struct brcmnand_controller *ctrl = dev_get_drvdata(&pdev->dev);
+
+	return ctrl ? ctrl->soc : NULL;
+}
+EXPORT_SYMBOL_GPL(brcmnand_get_socdata);

If you move the clk handling to the core brcmnand.c, then you won't need this still.

Would you prefer a clock name in the soc data structure that is used to call devm_clk_get()?

Add device tree binding for NAND on the BCM63268.

The BCM63268 has a NAND interrupt register with combined status and enable registers.

Signed-off-by Simon Arlott <[email protected]>

---
 .../devicetree/bindings/mtd/brcm,brcmnand.txt      | 35 ++++++++++++++++++++++
 1 file changed, 35 insertions(+)

Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt +35 -0

index 4ff7128..f2a71c8 100644
@@ -72,6 +72,14 @@ we define additional 'compatible' properties and associated register resources w
        and enable registers
      - reg-names: (required) "nand-int-base"
 
+   * "brcm,nand-bcm63268"
+     - compatible: should contain "brcm,nand-bcm<soc>", "brcm,nand-bcm63268"
+     - reg: (required) the 'NAND_INTR_BASE' register range, with combined status
+       and enable registers, and boot address registers
+     - reg-names: (required) "nand-intr-base"
+     - clock: (required) reference to the clock for the NAND controller
+     - clock-names: (required) "nand"
+
    * "brcm,nand-iproc"
      - reg: (required) the "IDM" register range, for interrupt enable and APB
        bus access endianness configuration, and the "EXT" register range,

@@ -148,3 +156,30 @@ nand@f0442800 {
 		};
 	};
 };
+
+nand@10000200 {
+	compatible = "brcm,nand-bcm63168", "brcm,nand-bcm63268",
+		"brcm,brcmnand-v4.0", "brcm,brcmnand";
+	reg = <0x10000200 0x180>,
+	      <0x10000600 0x200>,
+	      <0x100000b0 0x10>;
+	reg-names = "nand", "nand-cache", "nand-intr-base";
+	interrupt-parent = <&periph_intc>;
+	interrupts = <50>;
+	clocks = <&periph_clk 20>;
+	clock-names = "nand";
+
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	nand0: nandcs@0 {
+		compatible = "brcm,nandcs";
+		reg = <0>;
+		nand-on-flash-bbt;
+		nand-ecc-strength = <1>;
+		nand-ecc-step-size = <512>;
+
+		#address-cells = <0>;
+		#size-cells = <0>;
+	};
+};

Signature · 4 lines

2.1.4

-- 
Simon Arlott

Add device tree binding for NAND on the BCM63268.

The BCM63268 has a NAND interrupt register with combined status and enable registers.

Signed-off-by Simon Arlott <[email protected]>

---
 .../devicetree/bindings/mtd/brcm,brcmnand.txt      | 35 ++++++++++++++++++++++
 1 file changed, 35 insertions(+)

Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt +0 -0

index 4ff7128..f2a71c8 100644

Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt +21 -0

+     - reg: (required) the 'NAND_INTR_BASE' register range, with combined status
+       and enable registers, and boot address registers
+     - reg-names: (required) "nand-intr-base"
+     - clock: (required) reference to the clock for the NAND controller
+     - clock-names: (required) "nand"
+
    * "brcm,nand-iproc"
      - reg: (required) the "IDM" register range, for interrupt enable and APB
        bus access endianness configuration, and the "EXT" register range,

@@ -148,3 +156,30 @@ nand@f0442800 {
 		};
 	};
 };
+
+nand@10000200 {
+	compatible = "brcm,nand-bcm63168", "brcm,nand-bcm63268",
+		"brcm,brcmnand-v4.0", "brcm,brcmnand";
+	reg = <0x10000200 0x180>,
+	      <0x10000600 0x200>,
+	      <0x100000b0 0x10>;
+	reg-names = "nand", "nand-cache", "nand-intr-base";
+	interrupt-parent = <&periph_intc>;
+	interrupts = <50>;
+	clocks = <&periph_clk 20>;
+	clock-names = "nand";
+
+	#address-cells = <1>;
+	#size-cells = <0>;

+ Broadcom list + Kamal

Quoted 19 lines · Simon Arlott

Add device tree binding for NAND on the BCM63268.

The BCM63268 has a NAND interrupt register with combined status and enable registers.

Signed-off-by Simon Arlott <[email protected]>

---
 .../devicetree/bindings/mtd/brcm,brcmnand.txt      | 35 ++++++++++++++++++++++
 1 file changed, 35 insertions(+)

Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt +0 -0

index 4ff7128..f2a71c8 100644
@@ -72,6 +72,14 @@ we define additional 'compatible' properties and associated register resources w
        and enable registers
      - reg-names: (required) "nand-int-base"
 

Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt we define additional 'compatible' properties and associated register resources w · line 76

Quoted 2 lines

Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt +2 -0

+   * "brcm,nand-bcm63268"
+     - compatible: should contain "brcm,nand-bcm<soc>", "brcm,nand-bcm63268"

Looks like you're aiming to support bcm63168? Is bcm63268 the first chip to include this style of register then? The numbering seems backwards, but that may just be reality.

From a registers location/layout, clocks, etc standpoint, there is no

+ Broadcom list + Kamal

Quoted 19 lines · Simon Arlott

Add device tree binding for NAND on the BCM63268.

The BCM63268 has a NAND interrupt register with combined status and enable registers.

Signed-off-by Simon Arlott <[email protected]>

---
 .../devicetree/bindings/mtd/brcm,brcmnand.txt      | 35 ++++++++++++++++++++++
 1 file changed, 35 insertions(+)

Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt +0 -0

index 4ff7128..f2a71c8 100644
@@ -72,6 +72,14 @@ we define additional 'compatible' properties and associated register resources w
        and enable registers
      - reg-names: (required) "nand-int-base"
 

Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt we define additional 'compatible' properties and associated register resources w · line 76

Quoted 2 lines

Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt +2 -0

+   * "brcm,nand-bcm63268"
+     - compatible: should contain "brcm,nand-bcm<soc>", "brcm,nand-bcm63268"

Looks like you're aiming to support bcm63168? Is bcm63268 the first chip to include this style of register then? The numbering seems backwards, but that may just be reality.

2015-12-02 11:05 GMT-08:00 Brian Norris <[email protected]>:

Quoted 22 lines

+ Broadcom list + Kamal

Quoted 19 lines · Simon Arlott

Add device tree binding for NAND on the BCM63268.

The BCM63268 has a NAND interrupt register with combined status and enable registers.

Signed-off-by Simon Arlott <[email protected]>

---
 .../devicetree/bindings/mtd/brcm,brcmnand.txt      | 35 ++++++++++++++++++++++
 1 file changed, 35 insertions(+)

Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt +0 -0

index 4ff7128..f2a71c8 100644
@@ -72,6 +72,14 @@ we define additional 'compatible' properties and associated register resources w
        and enable registers
      - reg-names: (required) "nand-int-base"
 

Quoted 6 lines

Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt we define additional 'compatible' properties and associated register resources w · line 76

Quoted 2 lines

Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt +2 -0

+   * "brcm,nand-bcm63268"
+     - compatible: should contain "brcm,nand-bcm<soc>", "brcm,nand-bcm63268"

Looks like you're aiming to support bcm63168? Is bcm63268 the first chip to include this style of register then? The numbering seems backwards, but that may just be reality.

6362 (NAND rev 2.1, ann. Sep 8, 2009), 6368 (v0.1?!?, ann. Jan 7, 2009) and 6328 (v2.1, can't find release date) are earlier chips that have an identical combined interrupt enable + status register and a NAND controller within the same 32-bits word, so these would qualify as a better compatible string for this specific addition integration stub here. I would gowith 6368 here then?

Quoted 30 lines · Florian Fainelli

2015-12-02 11:05 GMT-08:00 Brian Norris <[email protected]>:

Quoted 22 lines

+ Broadcom list + Kamal

Quoted 19 lines · Simon Arlott

Add device tree binding for NAND on the BCM63268.

The BCM63268 has a NAND interrupt register with combined status and enable registers.

Signed-off-by Simon Arlott <[email protected]>

---
 .../devicetree/bindings/mtd/brcm,brcmnand.txt      | 35 ++++++++++++++++++++++
 1 file changed, 35 insertions(+)

Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt +0 -0

index 4ff7128..f2a71c8 100644
@@ -72,6 +72,14 @@ we define additional 'compatible' properties and associated register resources w
        and enable registers
      - reg-names: (required) "nand-int-base"
 

Quoted 6 lines

Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt we define additional 'compatible' properties and associated register resources w · line 76

Quoted 2 lines

Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt +2 -0

+   * "brcm,nand-bcm63268"
+     - compatible: should contain "brcm,nand-bcm<soc>", "brcm,nand-bcm63268"

Looks like you're aiming to support bcm63168? Is bcm63268 the first chip to include this style of register then? The numbering seems backwards, but that may just be reality.

Quoted 7 lines

6362 (NAND rev 2.1, ann. Sep 8, 2009), 6368 (v0.1?!?, ann. Jan 7, 2009) and 6328 (v2.1, can't find release date) are earlier chips that have an identical combined interrupt enable + status register and a NAND controller within the same 32-bits word, so these would qualify as a better compatible string for this specific addition integration stub here. I would gowith 6368 here then?

I could change it to 6368 but there's no documented NAND_INTR_BASE for it. Only the 63268 and 6818 have a #define for NAND_INTR_BASE.

+ Broadcom list + Kamal

Quoted 19 lines · Simon Arlott

Add device tree binding for NAND on the BCM63268.

The BCM63268 has a NAND interrupt register with combined status and enable registers.

Signed-off-by Simon Arlott <[email protected]>

---
 .../devicetree/bindings/mtd/brcm,brcmnand.txt      | 35 ++++++++++++++++++++++
 1 file changed, 35 insertions(+)

Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt +0 -0

index 4ff7128..f2a71c8 100644
@@ -72,6 +72,14 @@ we define additional 'compatible' properties and associated register resources w
        and enable registers
      - reg-names: (required) "nand-int-base"
 

Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt we define additional 'compatible' properties and associated register resources w · line 76

Quoted 2 lines

Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt +2 -0

+   * "brcm,nand-bcm63268"
+     - compatible: should contain "brcm,nand-bcm<soc>", "brcm,nand-bcm63268"

Looks like you're aiming to support bcm63168? Is bcm63268 the first chip to include this style of register then? The numbering seems backwards, but that may just be reality.

Quoted 35 lines

Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt +28 -0

+     - reg: (required) the 'NAND_INTR_BASE' register range, with combined status
+       and enable registers, and boot address registers
+     - reg-names: (required) "nand-intr-base"
+     - clock: (required) reference to the clock for the NAND controller
+     - clock-names: (required) "nand"
+
    * "brcm,nand-iproc"
      - reg: (required) the "IDM" register range, for interrupt enable and APB
        bus access endianness configuration, and the "EXT" register range,

@@ -148,3 +156,30 @@ nand@f0442800 {
 		};
 	};
 };
+
+nand@10000200 {
+	compatible = "brcm,nand-bcm63168", "brcm,nand-bcm63268",
+		"brcm,brcmnand-v4.0", "brcm,brcmnand";
+	reg = <0x10000200 0x180>,
+	      <0x10000600 0x200>,
+	      <0x100000b0 0x10>;
+	reg-names = "nand", "nand-cache", "nand-intr-base";
+	interrupt-parent = <&periph_intc>;
+	interrupts = <50>;
+	clocks = <&periph_clk 20>;
+	clock-names = "nand";
+
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	nand0: nandcs@0 {
+		compatible = "brcm,nandcs";
+		reg = <0>;
+		nand-on-flash-bbt;
+		nand-ecc-strength = <1>;
+		nand-ecc-step-size = <512>;

Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt nand@f0442800 { · line 183

Quoted 3 lines

Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt +3 -0

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

What are these {address,size}-cells for? If you need them for partitioning, then those are wrong -- they shouldn't be zero. Maybe just drop them? (I can cut them out when applying, if that's the only change to make.)

Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt nand@f0442800 { · line 185

Quoted 2 lines

Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt +2 -0

+	};
+};

Brian

Quoted 6 lines

Quoted 6 lines

Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt +6 -0

+	nand0: nandcs@0 {
+		compatible = "brcm,nandcs";
+		reg = <0>;
+		nand-on-flash-bbt;
+		nand-ecc-strength = <1>;
+		nand-ecc-step-size = <512>;

Quoted 8 lines

Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt nand@f0442800 { · line 183

Quoted 3 lines

Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt +3 -0

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

What are these {address,size}-cells for? If you need them for partitioning, then those are wrong -- they shouldn't be zero. Maybe just drop them? (I can cut them out when applying, if that's the only change to make.)

This is the correct way to do partitioning, there would be a "partitions" block with no address/size that contains the partitions as child nodes. [Documentation/devicetree/bindings/mtd/partition.txt]

I think they're also implicit so you can just remove those two lines.

From ePAPR: "The #address-cells and #size-cells properties are not

I've created a bcm963268part driver so there won't need to be any partitions in DT for bcm63268.

Quoted 2 lines

Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt nand@f0442800 { · line 185

Quoted 2 lines

Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt +2 -0

+	};
+};

Hi,

Quoted 7 lines · Simon Arlott

Quoted 5 lines

Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt nand@f0442800 { · line 183

Quoted 5 lines

Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt +5 -0

+	nand0: nandcs@0 {
+		compatible = "brcm,nandcs";
+
+		#address-cells = <0>;
+		#size-cells = <0>;

I think they're also implicit so you can just remove those two lines.

From ePAPR: "The #address-cells and #size-cells properties are not inherited from ancestors in the device tree. They shall be explicitly defined."

But we don't want to define them. I'd drop them, at least from the example, as they're not relevant.

Quoted 2 lines

I've created a bcm963268part driver so there won't need to be any partitions in DT for bcm63268.

Just curious, do you plan to submit this driver? We're working on

matching up partition parsers to flash devices via device tree of_match_table's, so you could do something like this:

	nand0: nandcs@0 {
		compatible = "brcm,nandcs";
		...

		partitions {
			compatible = "brcm,bcm963268-partitions";
			...
		};
	};

Quoted 5 lines · Brian Norris

Quoted 2 lines · Simon Arlott

I've created a bcm963268part driver so there won't need to be any partitions in DT for bcm63268.

Just curious, do you plan to submit this driver? We're working on

Yes, it's just the most recent one I've been working on. I still have USBH and IUDMA to submit

Quoted 7 lines

matching up partition parsers to flash devices via device tree of_match_table's, so you could do something like this:

	nand0: nandcs@0 {
		compatible = "brcm,nandcs";
		...

Quoted 5 lines

		partitions {
			compatible = "brcm,bcm963268-partitions";
			...
		};
	};

I modified brcmnand to look for a machine matching "brcm,bcm963268", but

that way is ok with me. Presumably "ofpart" defers to another matching partition parser?

Is there a patch for that method of parser detection available?

Quoted 1 line · Simon Arlott

Is there a patch for that method of parser detection available?

I have something working here, but I haven't had time to finish cleaning it up and submitting it. There's an older patch that works similarly, though it has some deficiencies:

http://patchwork.ozlabs.org/patch/475988/

The main difference between that and my (yet-to-be-submitted) proposal is that I'd like parsers to opt in by adding a proper of_match_table with non-Linux-specific DT bindings, and then we can drop the "linux,..." naming and make it a reasonably generic property.

Hi Simon,

Quoted 23 lines · Simon Arlott

Quoted 5 lines · Brian Norris

Quoted 2 lines · Simon Arlott

I've created a bcm963268part driver so there won't need to be any partitions in DT for bcm63268.

Just curious, do you plan to submit this driver? We're working on

Yes, it's just the most recent one I've been working on. I still have USBH and IUDMA to submit

Quoted 12 lines

matching up partition parsers to flash devices via device tree of_match_table's, so you could do something like this:

	nand0: nandcs@0 {
		compatible = "brcm,nandcs";
		...

		partitions {
			compatible = "brcm,bcm963268-partitions";
			...
		};
	};

Quoted 1 line

I modified brcmnand to look for a machine matching "brcm,bcm963268", but

Like this?

http://patchwork.ozlabs.org/patch/473180/

I'd like to avoid that (hence the "Rejected" status).

Quoted 2 lines

that way is ok with me. Presumably "ofpart" defers to another matching partition parser?

Yes, "ofpart" is for specifying the entire partition table in the device tree as subnodes of either the flash node or of the flash's "partitions" subnode. It's not the most flexible, but it does work generically.

Quoted 1 line

Is there a patch for that method of parser detection available?

I have something working here, but I haven't had time to finish cleaning it up and submitting it. There's an older patch that works similarly, though it has some deficiencies:

http://patchwork.ozlabs.org/patch/475988/

The main difference between that and my (yet-to-be-submitted) proposal is that I'd like parsers to opt in by adding a proper of_match_table with non-Linux-specific DT bindings, and then we can drop the "linux,..." naming and make it a reasonably generic property.

Regards,
Brian

Quoted 24 lines · Brian Norris

Quoted 23 lines · Simon Arlott

Quoted 5 lines · Brian Norris

Quoted 2 lines · Simon Arlott

I've created a bcm963268part driver so there won't need to be any partitions in DT for bcm63268.

Just curious, do you plan to submit this driver? We're working on

Yes, it's just the most recent one I've been working on. I still have USBH and IUDMA to submit

Quoted 12 lines

matching up partition parsers to flash devices via device tree of_match_table's, so you could do something like this:

	nand0: nandcs@0 {
		compatible = "brcm,nandcs";
		...

		partitions {
			compatible = "brcm,bcm963268-partitions";
			...
		};
	};

Quoted 7 lines

Quoted 1 line

I modified brcmnand to look for a machine matching "brcm,bcm963268", but

Like this?

http://patchwork.ozlabs.org/patch/473180/

I'd like to avoid that (hence the "Rejected" status).

I exported default_mtd_part_types, copied it, and then added to it:

Quoted 13 lines

Quoted 2 lines

that way is ok with me. Presumably "ofpart" defers to another matching partition parser?

Yes, "ofpart" is for specifying the entire partition table in the device tree as subnodes of either the flash node or of the flash's "partitions" subnode. It's not the most flexible, but it does work generically.

Quoted 1 line

Is there a patch for that method of parser detection available?

I have something working here, but I haven't had time to finish cleaning it up and submitting it. There's an older patch that works similarly, though it has some deficiencies:

Quoted 6 lines

http://patchwork.ozlabs.org/patch/475988/

The main difference between that and my (yet-to-be-submitted) proposal is that I'd like parsers to opt in by adding a proper of_match_table with non-Linux-specific DT bindings, and then we can drop the "linux,..." naming and make it a reasonably generic property.

I'll submit my parser without any means of using it,

let me know if you
want me to work on a patch for a match table.

Add device tree binding for NAND on the BCM63268.

The BCM63268 has a NAND interrupt register with combined status and enable registers.

Signed-off-by Simon Arlott <[email protected]>

---

Quoted 4 lines · Rob Herring

Quoted 2 lines

+ * "brcm,nand-bcm63268"
+ - compatible: should contain "brcm,nand-bcm<soc>", "brcm,nand-bcm63268"

vendor,<soc>-device is preferred.

The existing two bindings use brcm,nand-<soc>, but I've changed this one.

 .../devicetree/bindings/mtd/brcm,brcmnand.txt      | 33 ++++++++++++++++++++++
 1 file changed, 33 insertions(+)

Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt +33 -0

index 4ff7128..2644d6c 100644
@@ -72,6 +72,13 @@ we define additional 'compatible' properties and associated register resources w
        and enable registers
      - reg-names: (required) "nand-int-base"
 
+   * "brcm,bcm63268-nand"
+     - compatible: should contain "brcm,bcm<soc>-nand", "brcm,bcm63268-nand"
+     - reg: (required) the 'NAND_INTR_BASE' register range, with combined status
+       and enable registers, and boot address registers
+     - reg-names: (required) "nand-intr-base"
+     - clock: (required) reference to the clock for NAND controller
+
    * "brcm,nand-iproc"
      - reg: (required) the "IDM" register range, for interrupt enable and APB
        bus access endianness configuration, and the "EXT" register range,

@@ -148,3 +155,29 @@ nand@f0442800 {
 		};
 	};
 };
+
+nand@0x10000200 {
+	compatible = "brcm,bcm63168-nand", "brcm,bcm63268-nand",
+		"brcm,brcmnand-v4.0", "brcm,brcmnand";
+	reg = <0x10000200 0x180>,
+	      <0x10000600 0x200>,
+	      <0x100000b0 0x10>;
+	reg-names = "nand", "nand-cache", "nand-intr-base";
+	interrupt-parent = <&periph_intc>;
+	interrupts = <50>;
+	clocks = <&periph_clk 20>;
+
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	nand0: nandcs@0 {
+		compatible = "brcm,nandcs";
+		reg = <0>;
+		nand-on-flash-bbt;
+		nand-ecc-strength = <1>;
+		nand-ecc-step-size = <512>;
+
+		#address-cells = <0>;
+		#size-cells = <0>;
+	};
+};

Signature · 8 lines

2.1.4

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