2015-12-02 23:42:12

by Simon Arlott

[permalink] [raw]
Subject: [PATCH 1/3] mtd: brcmnand: Add brcm,bcm6368-nand device tree binding

Add device tree binding for NAND on the BCM6368.

The BCM6368 has a NAND interrupt register with combined status and enable
registers. It also requires a clock, so add an optional clock to the
common brcmnand binding.

Signed-off-by: Simon Arlott <[email protected]>
---
Renamed from BCM63268, made clock a generic property.

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

diff --git a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt
index 4ff7128..16d7835 100644
--- a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt
+++ b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt
@@ -45,6 +45,8 @@ Required properties:
- #size-cells : <0>

Optional properties:
+- clock : reference to the clock for the NAND controller
+- clock-names : "nand" (required for the above clock)
- brcm,nand-has-wp : Some versions of this IP include a write-protect
(WP) control bit. It is always available on >=
v7.0. Use this property to describe the rare
@@ -72,6 +74,12 @@ we define additional 'compatible' properties and associated register resources w
and enable registers
- reg-names: (required) "nand-int-base"

+ * "brcm,nand-bcm6368"
+ - compatible: should contain "brcm,nand-bcm<soc>", "brcm,nand-bcm6368"
+ - reg: (required) the 'NAND_INTR_BASE' register range, with combined status
+ and enable registers, and boot address registers
+ - reg-names: (required) "nand-intr-base"
+
* "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,27 @@ nand@f0442800 {
};
};
};
+
+nand@10000200 {
+ compatible = "brcm,nand-bcm63168", "brcm,nand-bcm6368",
+ "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>;
+ };
+};
--
2.1.4

--
Simon Arlott


2015-12-02 23:43:22

by Simon Arlott

[permalink] [raw]
Subject: [PATCH 2/3] mtd: brcmnand: Request and enable the clock if present

Attempt to enable a clock named "nand" as some SoCs have a clock for the
controller that needs to be enabled.

Signed-off-by: Simon Arlott <[email protected]>
---
drivers/mtd/nand/brcmnand/brcmnand.c | 69 ++++++++++++++++++++++++++++--------
1 file changed, 54 insertions(+), 15 deletions(-)

diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
index 2c8f67f..0a9cccf 100644
--- a/drivers/mtd/nand/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/brcmnand/brcmnand.c
@@ -11,6 +11,7 @@
* GNU General Public License for more details.
*/

+#include <linux/clk.h>
#include <linux/version.h>
#include <linux/module.h>
#include <linux/init.h>
@@ -122,6 +123,9 @@ struct brcmnand_controller {
/* Some SoCs provide custom interrupt status register(s) */
struct brcmnand_soc *soc;

+ /* Some SoCs have a gateable clock for the controller */
+ struct clk *clk;
+
int cmd_pending;
bool dma_pending;
struct completion done;
@@ -2136,10 +2140,24 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
if (IS_ERR(ctrl->nand_base))
return PTR_ERR(ctrl->nand_base);

+ /* Enable clock before using NAND registers */
+ ctrl->clk = devm_clk_get(dev, "nand");
+ if (!IS_ERR(ctrl->clk)) {
+ ret = clk_prepare_enable(ctrl->clk);
+ if (ret)
+ return ret;
+ } else {
+ ret = PTR_ERR(ctrl->clk);
+ if (ret == -EPROBE_DEFER)
+ return ret;
+
+ ctrl->clk = NULL;
+ }
+
/* Initialize NAND revision */
ret = brcmnand_revision_init(ctrl);
if (ret)
- return ret;
+ goto err;

/*
* Most chips have this cache at a fixed offset within 'nand' block.
@@ -2148,8 +2166,10 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "nand-cache");
if (res) {
ctrl->nand_fc = devm_ioremap_resource(dev, res);
- if (IS_ERR(ctrl->nand_fc))
- return PTR_ERR(ctrl->nand_fc);
+ if (IS_ERR(ctrl->nand_fc)) {
+ ret = PTR_ERR(ctrl->nand_fc);
+ goto err;
+ }
} else {
ctrl->nand_fc = ctrl->nand_base +
ctrl->reg_offsets[BRCMNAND_FC_BASE];
@@ -2159,8 +2179,10 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "flash-dma");
if (res) {
ctrl->flash_dma_base = devm_ioremap_resource(dev, res);
- if (IS_ERR(ctrl->flash_dma_base))
- return PTR_ERR(ctrl->flash_dma_base);
+ if (IS_ERR(ctrl->flash_dma_base)) {
+ ret = PTR_ERR(ctrl->flash_dma_base);
+ goto err;
+ }

flash_dma_writel(ctrl, FLASH_DMA_MODE, 1); /* linked-list */
flash_dma_writel(ctrl, FLASH_DMA_ERROR_STATUS, 0);
@@ -2169,13 +2191,16 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
ctrl->dma_desc = dmam_alloc_coherent(dev,
sizeof(*ctrl->dma_desc),
&ctrl->dma_pa, GFP_KERNEL);
- if (!ctrl->dma_desc)
- return -ENOMEM;
+ if (!ctrl->dma_desc) {
+ ret = -ENOMEM;
+ goto err;
+ }

ctrl->dma_irq = platform_get_irq(pdev, 1);
if ((int)ctrl->dma_irq < 0) {
dev_err(dev, "missing FLASH_DMA IRQ\n");
- return -ENODEV;
+ ret = -ENODEV;
+ goto err;
}

ret = devm_request_irq(dev, ctrl->dma_irq,
@@ -2184,7 +2209,7 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
if (ret < 0) {
dev_err(dev, "can't allocate IRQ %d: error %d\n",
ctrl->dma_irq, ret);
- return ret;
+ goto err;
}

dev_info(dev, "enabling FLASH_DMA\n");
@@ -2208,7 +2233,8 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
ctrl->irq = platform_get_irq(pdev, 0);
if ((int)ctrl->irq < 0) {
dev_err(dev, "no IRQ defined\n");
- return -ENODEV;
+ ret = -ENODEV;
+ goto err;
}

/*
@@ -2232,7 +2258,7 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
if (ret < 0) {
dev_err(dev, "can't allocate IRQ %d: error %d\n",
ctrl->irq, ret);
- return ret;
+ goto err;
}

for_each_available_child_of_node(dn, child) {
@@ -2240,8 +2266,10 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
struct brcmnand_host *host;

host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
- if (!host)
- return -ENOMEM;
+ if (!host) {
+ ret = -ENOMEM;
+ goto err;
+ }
host->pdev = pdev;
host->ctrl = ctrl;
host->of_node = child;
@@ -2255,10 +2283,18 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
}

/* No chip-selects could initialize properly */
- if (list_empty(&ctrl->host_list))
- return -ENODEV;
+ if (list_empty(&ctrl->host_list)) {
+ ret = -ENODEV;
+ goto err;
+ }

return 0;
+
+err:
+ if (ctrl->clk)
+ clk_disable_unprepare(ctrl->clk);
+ return ret;
+
}
EXPORT_SYMBOL_GPL(brcmnand_probe);

@@ -2270,6 +2306,9 @@ int brcmnand_remove(struct platform_device *pdev)
list_for_each_entry(host, &ctrl->host_list, node)
nand_release(&host->mtd);

+ if (ctrl->clk)
+ clk_disable_unprepare(ctrl->clk);
+
dev_set_drvdata(&pdev->dev, NULL);

return 0;
--
2.1.4

--
Simon Arlott

2015-12-02 23:44:35

by Simon Arlott

[permalink] [raw]
Subject: [PATCH 3/3] mtd: brcmnand: Add support for the BCM6368

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

As the BCM6328, BCM6362 and BCM6368 all use v2.1 controllers, the first
variant that will work with this driver is the BCM63268 using a v4.0
controller.

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

Signed-off-by: Simon Arlott <[email protected]>
---
Renamed from BCM63268, moved clock to brcmnand.

drivers/mtd/nand/brcmnand/Makefile | 1 +
drivers/mtd/nand/brcmnand/bcm6368_nand.c | 145 +++++++++++++++++++++++++++++++
2 files changed, 146 insertions(+)
create mode 100644 drivers/mtd/nand/brcmnand/bcm6368_nand.c

diff --git a/drivers/mtd/nand/brcmnand/Makefile b/drivers/mtd/nand/brcmnand/Makefile
index 3b1fbfd..b28ffb59 100644
--- a/drivers/mtd/nand/brcmnand/Makefile
+++ b/drivers/mtd/nand/brcmnand/Makefile
@@ -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) += bcm6368_nand.o
obj-$(CONFIG_MTD_NAND_BRCMNAND) += brcmstb_nand.o
obj-$(CONFIG_MTD_NAND_BRCMNAND) += brcmnand.o
diff --git a/drivers/mtd/nand/brcmnand/bcm6368_nand.c b/drivers/mtd/nand/brcmnand/bcm6368_nand.c
new file mode 100644
index 0000000..c347ea5
--- /dev/null
+++ b/drivers/mtd/nand/brcmnand/bcm6368_nand.c
@@ -0,0 +1,145 @@
+/*
+ * 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/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 bcm6368_nand_soc {
+ struct brcmnand_soc soc;
+ void __iomem *base;
+};
+
+#define BCM6368_NAND_INT 0x00
+#define BCM6368_NAND_STATUS_SHIFT 0
+#define BCM6368_NAND_STATUS_MASK (0xfff << BCM6368_NAND_STATUS_SHIFT)
+#define BCM6368_NAND_ENABLE_SHIFT 16
+#define BCM6368_NAND_ENABLE_MASK (0xffff << BCM6368_NAND_ENABLE_SHIFT)
+#define BCM6368_NAND_BASE_ADDR0 0x04
+#define BCM6368_NAND_BASE_ADDR1 0x0c
+
+enum {
+ BCM6368_NP_READ = BIT(0),
+ BCM6368_BLOCK_ERASE = BIT(1),
+ BCM6368_COPY_BACK = BIT(2),
+ BCM6368_PAGE_PGM = BIT(3),
+ BCM6368_CTRL_READY = BIT(4),
+ BCM6368_DEV_RBPIN = BIT(5),
+ BCM6368_ECC_ERR_UNC = BIT(6),
+ BCM6368_ECC_ERR_CORR = BIT(7),
+};
+
+static bool bcm6368_nand_intc_ack(struct brcmnand_soc *soc)
+{
+ struct bcm6368_nand_soc *priv =
+ container_of(soc, struct bcm6368_nand_soc, soc);
+ void __iomem *mmio = priv->base + BCM6368_NAND_INT;
+ u32 val = brcmnand_readl(mmio);
+
+ if (val & (BCM6368_CTRL_READY << BCM6368_NAND_STATUS_SHIFT)) {
+ /* Ack interrupt */
+ val &= ~BCM6368_NAND_STATUS_MASK;
+ val |= BCM6368_CTRL_READY << BCM6368_NAND_STATUS_SHIFT;
+ brcmnand_writel(val, mmio);
+ return true;
+ }
+
+ return false;
+}
+
+static void bcm6368_nand_intc_set(struct brcmnand_soc *soc, bool en)
+{
+ struct bcm6368_nand_soc *priv =
+ container_of(soc, struct bcm6368_nand_soc, soc);
+ void __iomem *mmio = priv->base + BCM6368_NAND_INT;
+ u32 val = brcmnand_readl(mmio);
+
+ /* Don't ack any interrupts */
+ val &= ~BCM6368_NAND_STATUS_MASK;
+
+ if (en)
+ val |= BCM6368_CTRL_READY << BCM6368_NAND_ENABLE_SHIFT;
+ else
+ val &= ~(BCM6368_CTRL_READY << BCM6368_NAND_ENABLE_SHIFT);
+
+ brcmnand_writel(val, mmio);
+}
+
+static int bcm6368_nand_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct bcm6368_nand_soc *priv;
+ struct brcmnand_soc *soc;
+ struct resource *res;
+
+ 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);
+
+ soc->ctlrdy_ack = bcm6368_nand_intc_ack;
+ soc->ctlrdy_set_enabled = bcm6368_nand_intc_set;
+
+ /* Disable and ack all interrupts */
+ brcmnand_writel(0, priv->base + BCM6368_NAND_INT);
+ brcmnand_writel(BCM6368_NAND_STATUS_MASK,
+ priv->base + BCM6368_NAND_INT);
+
+ return brcmnand_probe(pdev, soc);
+}
+
+static const struct of_device_id bcm6368_nand_of_match[] = {
+ { .compatible = "brcm,nand-bcm6368" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, bcm6368_nand_of_match);
+
+static struct platform_driver bcm6368_nand_driver = {
+ .probe = bcm6368_nand_probe,
+ .remove = brcmnand_remove,
+ .driver = {
+ .name = "bcm6368_nand",
+ .pm = &brcmnand_pm_ops,
+ .of_match_table = bcm6368_nand_of_match,
+ }
+};
+module_platform_driver(bcm6368_nand_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Simon Arlott");
+MODULE_DESCRIPTION("NAND driver for BCM6368");
--
2.1.4

--
Simon Arlott

2015-12-03 00:11:54

by Brian Norris

[permalink] [raw]
Subject: Re: [PATCH 2/3] mtd: brcmnand: Request and enable the clock if present

Hi Simon,

On Wed, Dec 02, 2015 at 11:42:44PM +0000, Simon Arlott wrote:
> Attempt to enable a clock named "nand" as some SoCs have a clock for the
> controller that needs to be enabled.
>
> Signed-off-by: Simon Arlott <[email protected]>
> ---
> drivers/mtd/nand/brcmnand/brcmnand.c | 69 ++++++++++++++++++++++++++++--------
> 1 file changed, 54 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
> index 2c8f67f..0a9cccf 100644
> --- a/drivers/mtd/nand/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/brcmnand/brcmnand.c
> @@ -11,6 +11,7 @@
> * GNU General Public License for more details.
> */
>
> +#include <linux/clk.h>
> #include <linux/version.h>
> #include <linux/module.h>
> #include <linux/init.h>
> @@ -122,6 +123,9 @@ struct brcmnand_controller {
> /* Some SoCs provide custom interrupt status register(s) */
> struct brcmnand_soc *soc;
>
> + /* Some SoCs have a gateable clock for the controller */
> + struct clk *clk;
> +
> int cmd_pending;
> bool dma_pending;
> struct completion done;
> @@ -2136,10 +2140,24 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
> if (IS_ERR(ctrl->nand_base))
> return PTR_ERR(ctrl->nand_base);
>
> + /* Enable clock before using NAND registers */
> + ctrl->clk = devm_clk_get(dev, "nand");
> + if (!IS_ERR(ctrl->clk)) {
> + ret = clk_prepare_enable(ctrl->clk);
> + if (ret)
> + return ret;
> + } else {
> + ret = PTR_ERR(ctrl->clk);
> + if (ret == -EPROBE_DEFER)
> + return ret;
> +
> + ctrl->clk = NULL;
> + }
> +
> /* Initialize NAND revision */
> ret = brcmnand_revision_init(ctrl);
> if (ret)
> - return ret;
> + goto err;
>
> /*
> * Most chips have this cache at a fixed offset within 'nand' block.
> @@ -2148,8 +2166,10 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "nand-cache");
> if (res) {
> ctrl->nand_fc = devm_ioremap_resource(dev, res);
> - if (IS_ERR(ctrl->nand_fc))
> - return PTR_ERR(ctrl->nand_fc);
> + if (IS_ERR(ctrl->nand_fc)) {
> + ret = PTR_ERR(ctrl->nand_fc);
> + goto err;
> + }
> } else {
> ctrl->nand_fc = ctrl->nand_base +
> ctrl->reg_offsets[BRCMNAND_FC_BASE];
> @@ -2159,8 +2179,10 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "flash-dma");
> if (res) {
> ctrl->flash_dma_base = devm_ioremap_resource(dev, res);
> - if (IS_ERR(ctrl->flash_dma_base))
> - return PTR_ERR(ctrl->flash_dma_base);
> + if (IS_ERR(ctrl->flash_dma_base)) {
> + ret = PTR_ERR(ctrl->flash_dma_base);
> + goto err;
> + }
>
> flash_dma_writel(ctrl, FLASH_DMA_MODE, 1); /* linked-list */
> flash_dma_writel(ctrl, FLASH_DMA_ERROR_STATUS, 0);
> @@ -2169,13 +2191,16 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
> ctrl->dma_desc = dmam_alloc_coherent(dev,
> sizeof(*ctrl->dma_desc),
> &ctrl->dma_pa, GFP_KERNEL);
> - if (!ctrl->dma_desc)
> - return -ENOMEM;
> + if (!ctrl->dma_desc) {
> + ret = -ENOMEM;
> + goto err;
> + }
>
> ctrl->dma_irq = platform_get_irq(pdev, 1);
> if ((int)ctrl->dma_irq < 0) {
> dev_err(dev, "missing FLASH_DMA IRQ\n");
> - return -ENODEV;
> + ret = -ENODEV;
> + goto err;
> }
>
> ret = devm_request_irq(dev, ctrl->dma_irq,
> @@ -2184,7 +2209,7 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
> if (ret < 0) {
> dev_err(dev, "can't allocate IRQ %d: error %d\n",
> ctrl->dma_irq, ret);
> - return ret;
> + goto err;
> }
>
> dev_info(dev, "enabling FLASH_DMA\n");
> @@ -2208,7 +2233,8 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
> ctrl->irq = platform_get_irq(pdev, 0);
> if ((int)ctrl->irq < 0) {
> dev_err(dev, "no IRQ defined\n");
> - return -ENODEV;
> + ret = -ENODEV;
> + goto err;
> }
>
> /*
> @@ -2232,7 +2258,7 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
> if (ret < 0) {
> dev_err(dev, "can't allocate IRQ %d: error %d\n",
> ctrl->irq, ret);
> - return ret;
> + goto err;
> }
>
> for_each_available_child_of_node(dn, child) {
> @@ -2240,8 +2266,10 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
> struct brcmnand_host *host;
>
> host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
> - if (!host)
> - return -ENOMEM;
> + if (!host) {
> + ret = -ENOMEM;
> + goto err;
> + }
> host->pdev = pdev;
> host->ctrl = ctrl;
> host->of_node = child;

Please submit against l2-mtd.git or linux-next.git:

http://linux-mtd.infradead.org/source.html

> @@ -2255,10 +2283,18 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
> }
>
> /* No chip-selects could initialize properly */
> - if (list_empty(&ctrl->host_list))
> - return -ENODEV;
> + if (list_empty(&ctrl->host_list)) {
> + ret = -ENODEV;
> + goto err;
> + }
>
> return 0;
> +
> +err:
> + if (ctrl->clk)

This NULL check is unnecessary.

> + clk_disable_unprepare(ctrl->clk);
> + return ret;
> +
> }
> EXPORT_SYMBOL_GPL(brcmnand_probe);
>
> @@ -2270,6 +2306,9 @@ int brcmnand_remove(struct platform_device *pdev)
> list_for_each_entry(host, &ctrl->host_list, node)
> nand_release(&host->mtd);
>
> + if (ctrl->clk)

Same here.

> + clk_disable_unprepare(ctrl->clk);
> +
> dev_set_drvdata(&pdev->dev, NULL);
>
> return 0;

Otherwise, LGTM.

Brian

2015-12-04 15:06:33

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH 1/3] mtd: brcmnand: Add brcm,bcm6368-nand device tree binding

On Wed, Dec 02, 2015 at 11:41:26PM +0000, Simon Arlott wrote:
> Add device tree binding for NAND on the BCM6368.
>
> The BCM6368 has a NAND interrupt register with combined status and enable
> registers. It also requires a clock, so add an optional clock to the
> common brcmnand binding.
>
> Signed-off-by: Simon Arlott <[email protected]>

Acked-by: Rob Herring <[email protected]>

> ---
> Renamed from BCM63268, made clock a generic property.
>
> .../devicetree/bindings/mtd/brcm,brcmnand.txt | 32 ++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt
> index 4ff7128..16d7835 100644
> --- a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt
> +++ b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt
> @@ -45,6 +45,8 @@ Required properties:
> - #size-cells : <0>
>
> Optional properties:
> +- clock : reference to the clock for the NAND controller
> +- clock-names : "nand" (required for the above clock)
> - brcm,nand-has-wp : Some versions of this IP include a write-protect
> (WP) control bit. It is always available on >=
> v7.0. Use this property to describe the rare
> @@ -72,6 +74,12 @@ we define additional 'compatible' properties and associated register resources w
> and enable registers
> - reg-names: (required) "nand-int-base"
>
> + * "brcm,nand-bcm6368"
> + - compatible: should contain "brcm,nand-bcm<soc>", "brcm,nand-bcm6368"
> + - reg: (required) the 'NAND_INTR_BASE' register range, with combined status
> + and enable registers, and boot address registers
> + - reg-names: (required) "nand-intr-base"
> +
> * "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,27 @@ nand@f0442800 {
> };
> };
> };
> +
> +nand@10000200 {
> + compatible = "brcm,nand-bcm63168", "brcm,nand-bcm6368",
> + "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>;
> + };
> +};
> --
> 2.1.4
>
> --
> Simon Arlott

2015-12-04 16:04:44

by Jonas Gorski

[permalink] [raw]
Subject: Re: [PATCH 1/3] mtd: brcmnand: Add brcm,bcm6368-nand device tree binding

On Thu, Dec 3, 2015 at 12:41 AM, Simon Arlott <[email protected]> wrote:
> Add device tree binding for NAND on the BCM6368.
>
> The BCM6368 has a NAND interrupt register with combined status and enable
> registers. It also requires a clock, so add an optional clock to the
> common brcmnand binding.
>
> Signed-off-by: Simon Arlott <[email protected]>
> ---
> Renamed from BCM63268, made clock a generic property.
>
> .../devicetree/bindings/mtd/brcm,brcmnand.txt | 32 ++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt
> index 4ff7128..16d7835 100644
> --- a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt
> +++ b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt
> @@ -45,6 +45,8 @@ Required properties:
> - #size-cells : <0>
>
> Optional properties:
> +- clock : reference to the clock for the NAND controller
> +- clock-names : "nand" (required for the above clock)
> - brcm,nand-has-wp : Some versions of this IP include a write-protect
> (WP) control bit. It is always available on >=
> v7.0. Use this property to describe the rare
> @@ -72,6 +74,12 @@ we define additional 'compatible' properties and associated register resources w
> and enable registers
> - reg-names: (required) "nand-int-base"
>
> + * "brcm,nand-bcm6368"
> + - compatible: should contain "brcm,nand-bcm<soc>", "brcm,nand-bcm6368"
> + - reg: (required) the 'NAND_INTR_BASE' register range, with combined status
> + and enable registers, and boot address registers
> + - reg-names: (required) "nand-intr-base"

Can't we use the same name as bcm63138, i.e. nand-int-base?

> +
> * "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,27 @@ nand@f0442800 {
> };
> };
> };
> +
> +nand@10000200 {
> + compatible = "brcm,nand-bcm63168", "brcm,nand-bcm6368",
> + "brcm,brcmnand-v4.0", "brcm,brcmnand";

I know it's now much too late, but this is IMHO a very odd way of
defining that this is a v4 nand, but uses bcm6368 compatible
interrupts, as bcm6368 is a much older, unsupported nand revision.


Jonas

2015-12-04 21:30:09

by Simon Arlott

[permalink] [raw]
Subject: Re: [PATCH 1/3] mtd: brcmnand: Add brcm,bcm6368-nand device tree binding

On Fri, December 4, 2015 16:04, Jonas Gorski wrote:
> On Thu, Dec 3, 2015 at 12:41 AM, Simon Arlott <[email protected]> wrote:
>> + * "brcm,nand-bcm6368"
>> + - compatible: should contain "brcm,nand-bcm<soc>", "brcm,nand-bcm6368"
>> + - reg: (required) the 'NAND_INTR_BASE' register range, with combined status
>> + and enable registers, and boot address registers
>> + - reg-names: (required) "nand-intr-base"
>
> Can't we use the same name as bcm63138, i.e. nand-int-base?

Brian,

Before I change this, is there anything else in the patch series that needs to
be changed?

(I'll keep the comment referring to "NAND_INTR_BASE" the same because that's the name
in the original #define for this hardware.)

--
Simon Arlott

2015-12-09 20:02:42

by Brian Norris

[permalink] [raw]
Subject: Re: [PATCH 1/3] mtd: brcmnand: Add brcm,bcm6368-nand device tree binding

On Fri, Dec 04, 2015 at 09:29:55PM -0000, Simon Arlott wrote:
> On Fri, December 4, 2015 16:04, Jonas Gorski wrote:
> > On Thu, Dec 3, 2015 at 12:41 AM, Simon Arlott <[email protected]> wrote:
> >> + * "brcm,nand-bcm6368"
> >> + - compatible: should contain "brcm,nand-bcm<soc>", "brcm,nand-bcm6368"
> >> + - reg: (required) the 'NAND_INTR_BASE' register range, with combined status
> >> + and enable registers, and boot address registers
> >> + - reg-names: (required) "nand-intr-base"
> >
> > Can't we use the same name as bcm63138, i.e. nand-int-base?
>
> Brian,
>
> Before I change this, is there anything else in the patch series that needs to
> be changed?

No, I think you covered my comments in your latest series:

http://lists.infradead.org/pipermail/linux-mtd/2015-December/064004.html

I don't know about Jonas's comments about using bcm6368, even though
bcm6368 is a much older NAND core. I had similar thoughts when Florian
first proposed it, but I'm not sure I have a much better suggestion.
We're trying to describe two slightly different tracks of IP: the core
NAND controller, which has a defined revision (2.x, 4.0, etc.), and the
accessory interrupt bits, which are mostly constant across a product
line / class of SoCs and aren't really versioned.

So I guess I'm OK with the usage of the bcm6368 compatible string.

Regards,
Brian