Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1753905AbbKWPnP (ORCPT <rfc822;w@1wt.eu>);
	Mon, 23 Nov 2015 10:43:15 -0500
Received: from arrakis.dune.hu ([78.24.191.176]:45841 "EHLO arrakis.dune.hu"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1753632AbbKWPnL convert rfc822-to-8bit (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Mon, 23 Nov 2015 10:43:11 -0500
MIME-Version: 1.0
In-Reply-To: <56523EFF.9050502@simon.arlott.org.uk>
References: <56506D55.3000907@simon.arlott.org.uk> <20151122215945.GA5930@rob-hp-laptop>
 <56523E85.905@simon.arlott.org.uk> <56523EFF.9050502@simon.arlott.org.uk>
From: Jonas Gorski <jogo@openwrt.org>
Date: Mon, 23 Nov 2015 16:42:41 +0100
X-Gmail-Original-Message-ID: <CAOiHx=mZpycf962D0snStBMDA=Pnexdrk6wok9MVQdHaLN8fAQ@mail.gmail.com>
Message-ID: <CAOiHx=mZpycf962D0snStBMDA=Pnexdrk6wok9MVQdHaLN8fAQ@mail.gmail.com>
Subject: Re: [PATCH (v4) 2/2] mtd: brcmnand: Add support for the BCM63268
To: Simon Arlott <simon@fire.lp0.eu>
Cc: Rob Herring <robh@kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        Brian Norris <computersforpeace@gmail.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        David Woodhouse <dwmw2@infradead.org>,
        MTD Maling List <linux-mtd@lists.infradead.org>,
        Pawel Moll <pawel.moll@arm.com>, Mark Rutland <mark.rutland@arm.com>,
        Ian Campbell <ijc+devicetree@hellion.org.uk>,
        Kumar Gala <galak@codeaurora.org>,
        Florian Fainelli <f.fainelli@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 9692
Lines: 263

On Sun, Nov 22, 2015 at 11:17 PM, Simon Arlott <simon@fire.lp0.eu> wrote:
> 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 <simon@fire.lp0.eu>
> ---
> On 22/11/15 21:59, Rob Herring wrote:
>>> >> + * "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
>
> diff --git a/drivers/mtd/nand/brcmnand/Makefile b/drivers/mtd/nand/brcmnand/Makefile
> index 3b1fbfd..b83a9ae 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)                += bcm63268_nand.o
>  obj-$(CONFIG_MTD_NAND_BRCMNAND)                += brcmstb_nand.o
>  obj-$(CONFIG_MTD_NAND_BRCMNAND)                += brcmnand.o
> diff --git a/drivers/mtd/nand/brcmnand/bcm63268_nand.c b/drivers/mtd/nand/brcmnand/bcm63268_nand.c
> new file mode 100644
> index 0000000..88b32fa
> --- /dev/null
> +++ b/drivers/mtd/nand/brcmnand/bcm63268_nand.c
> @@ -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;
> +
> +       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);
> +
> +       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.

> +       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");
> diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
> index 2c8f67f..7b4988f 100644
> --- a/drivers/mtd/nand/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/brcmnand/brcmnand.c
> @@ -2270,6 +2270,9 @@ int brcmnand_remove(struct platform_device *pdev)
>         list_for_each_entry(host, &ctrl->host_list, node)
>                 nand_release(&host->mtd);
>
> +       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.

>         dev_set_drvdata(&pdev->dev, NULL);
>
>         return 0;
> diff --git a/drivers/mtd/nand/brcmnand/brcmnand.h b/drivers/mtd/nand/brcmnand/brcmnand.h
> index ef5eabb..5c5dc2d 100644
> --- a/drivers/mtd/nand/brcmnand/brcmnand.h
> +++ b/drivers/mtd/nand/brcmnand/brcmnand.h
> @@ -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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/