[PATCH 0/2] reset driver for NXP LPC18xx family

[PATCH 0/2] reset driver for NXP LPC18xx family

[Joachim Eastwood]

This patch set adds a reset driver for NXP LPC18xx/43xx devices.

There are no dependencies in this patch set. The DTS changes will
be sent separately to arm-soc once this patch set goes upstream.

Base support for the NXP LPC18xx family can be found here:
http://marc.info/?l=linux-arm-kernel&m=143016894704253&w=2

Joachim Eastwood (2):
  reset: add driver for lpc18xx rgu
  doc: dt: add documentation for lpc1850-rgu reset driver

 .../devicetree/bindings/reset/nxp,lpc1850-rgu.txt  |  38 +++
 drivers/reset/Makefile                             |   1 +
 drivers/reset/reset-lpc18xx.c                      | 260 +++++++++++++++++++++
 3 files changed, 299 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/nxp,lpc1850-rgu.txt
 create mode 100644 drivers/reset/reset-lpc18xx.c

-- 
1.8.0

[PATCH 1/2] reset: add driver for lpc18xx rgu

[Joachim Eastwood]

Signed-off-by: Joachim Eastwood <manabian@gmail.com>
---
 drivers/reset/Makefile        |   1 +
 drivers/reset/reset-lpc18xx.c | 260 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 261 insertions(+)
 create mode 100644 drivers/reset/reset-lpc18xx.c

diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index 157d421f755b..1d41feeb2dce 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -1,4 +1,5 @@
 obj-$(CONFIG_RESET_CONTROLLER) += core.o
+obj-$(CONFIG_ARCH_LPC18XX) += reset-lpc18xx.o
 obj-$(CONFIG_ARCH_SOCFPGA) += reset-socfpga.o
 obj-$(CONFIG_ARCH_BERLIN) += reset-berlin.o
 obj-$(CONFIG_ARCH_SUNXI) += reset-sunxi.o
diff --git a/drivers/reset/reset-lpc18xx.c b/drivers/reset/reset-lpc18xx.c
new file mode 100644
index 000000000000..9564d78ab9d2
--- /dev/null
+++ b/drivers/reset/reset-lpc18xx.c
@@ -0,0 +1,260 @@
+/*
+ * Reset driver for NXP LPC18xx/43xx Reset Generation Unit (RGU).
+ *
+ * Copyright (C) 2014 Joachim Eastwood <manabian@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/reboot.h>
+#include <linux/reset-controller.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#include <asm/system_misc.h>
+
+/* LPC18xx RGU registers */
+#define LPC18XX_RGU_CTRL0		0x100
+#define LPC18XX_RGU_CTRL1		0x104
+#define LPC18XX_RGU_ACTIVE_STATUS0	0x150
+#define LPC18XX_RGU_ACTIVE_STATUS1	0x154
+
+#define LPC18XX_RGU_RESETS_PER_REG	32
+
+/* Internal reset outputs */
+#define LPC18XX_RGU_CORE_RST	0
+#define LPC18XX_RGU_M0SUB_RST	12
+#define LPC18XX_RGU_M0APP_RST	56
+
+struct lpc18xx_rgu_data {
+	struct reset_controller_dev rcdev;
+	struct clk *clk_delay;
+	struct clk *clk_reg;
+	void __iomem *base;
+	spinlock_t lock;
+	u32 delay_us;
+};
+
+#define to_rgu_data(rcdev) container_of(rcdev, struct lpc18xx_rgu_data, rcdev)
+
+static void __iomem *rgu_base;
+
+static int lpc18xx_rgu_restart(struct notifier_block *this, unsigned long mode,
+			       void *cmd)
+{
+	writel(BIT(LPC18XX_RGU_CORE_RST), rgu_base + LPC18XX_RGU_CTRL0);
+	mdelay(2000);
+
+	pr_emerg("%s: unable to restart system\n", __func__);
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block lpc18xx_rgu_restart_nb = {
+	.notifier_call = lpc18xx_rgu_restart,
+	.priority = 192,
+};
+
+/*
+ * The LPC18xx RGU has mostly self-deasserting resets except for the
+ * two reset lines going to the internal Cortex-M0 cores.
+ *
+ * To prevent the M0 cores from accidentally getting deasserting the
+ * status register must be check and bits in control register set to
+ * preserve the state.
+ */
+static void lpc18xx_rgu_setclear_reset(struct reset_controller_dev *rcdev,
+				       unsigned long id, bool set)
+{
+	struct lpc18xx_rgu_data *rc = to_rgu_data(rcdev);
+	u32 stat_offset = LPC18XX_RGU_ACTIVE_STATUS0;
+	u32 ctrl_offset = LPC18XX_RGU_CTRL0;
+	unsigned long flags;
+	u32 stat, rst_bit;
+
+	stat_offset += (id / LPC18XX_RGU_RESETS_PER_REG) * sizeof(u32);
+	ctrl_offset += (id / LPC18XX_RGU_RESETS_PER_REG) * sizeof(u32);
+	rst_bit = 1 << (id % LPC18XX_RGU_RESETS_PER_REG);
+
+	spin_lock_irqsave(&rc->lock, flags);
+	stat = ~readl(rc->base + stat_offset);
+	if (set)
+		writel(stat | rst_bit, rc->base + ctrl_offset);
+	else
+		writel(stat & ~rst_bit, rc->base + ctrl_offset);
+	spin_unlock_irqrestore(&rc->lock, flags);
+}
+
+static int lpc18xx_rgu_reset(struct reset_controller_dev *rcdev,
+			     unsigned long id)
+{
+	struct lpc18xx_rgu_data *rc = to_rgu_data(rcdev);
+
+	lpc18xx_rgu_setclear_reset(rcdev, id, true);
+	udelay(rc->delay_us);
+
+	return 0;
+}
+
+static int lpc18xx_rgu_assert(struct reset_controller_dev *rcdev,
+			      unsigned long id)
+{
+	return lpc18xx_rgu_reset(rcdev, id);
+}
+
+/* Only M0 cores require explicit reset deassert */
+static int lpc18xx_rgu_deassert(struct reset_controller_dev *rcdev,
+				unsigned long id)
+{
+	switch (id) {
+	case LPC18XX_RGU_M0SUB_RST:
+	case LPC18XX_RGU_M0APP_RST:
+		lpc18xx_rgu_setclear_reset(rcdev, id, false);
+	}
+
+	return 0;
+}
+
+static int lpc18xx_rgu_status(struct reset_controller_dev *rcdev,
+			      unsigned long id)
+{
+	struct lpc18xx_rgu_data *rc = to_rgu_data(rcdev);
+	u32 bit, offset = LPC18XX_RGU_ACTIVE_STATUS0;
+
+	offset += (id / LPC18XX_RGU_RESETS_PER_REG) * sizeof(u32);
+	bit = 1 << (id % LPC18XX_RGU_RESETS_PER_REG);
+
+	return !(readl(rc->base + offset) & bit);
+}
+
+static struct reset_control_ops lpc18xx_rgu_ops = {
+	.reset		= lpc18xx_rgu_reset,
+	.assert		= lpc18xx_rgu_assert,
+	.deassert	= lpc18xx_rgu_deassert,
+	.status		= lpc18xx_rgu_status,
+};
+
+static int lpc18xx_rgu_probe(struct platform_device *pdev)
+{
+	struct lpc18xx_rgu_data *rc;
+	struct resource *res;
+	u32 fcclk, firc;
+	int ret;
+
+	rc = devm_kzalloc(&pdev->dev, sizeof(*rc), GFP_KERNEL);
+	if (!rc)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	rc->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(rc->base))
+		return PTR_ERR(rc->base);
+
+	rc->clk_reg = devm_clk_get(&pdev->dev, "reg");
+	if (IS_ERR(rc->clk_reg)) {
+		dev_err(&pdev->dev, "reg clock not found\n");
+		return PTR_ERR(rc->clk_reg);
+	}
+
+	rc->clk_delay = devm_clk_get(&pdev->dev, "delay");
+	if (IS_ERR(rc->clk_delay)) {
+		dev_err(&pdev->dev, "delay clock not found\n");
+		return PTR_ERR(rc->clk_delay);
+	}
+
+	ret = clk_prepare_enable(rc->clk_reg);
+	if (ret) {
+		dev_err(&pdev->dev, "unable to enable reg clock\n");
+		return ret;
+	}
+
+	ret = clk_prepare_enable(rc->clk_delay);
+	if (ret) {
+		dev_err(&pdev->dev, "unable to enable delay clock\n");
+		goto dis_clk_reg;
+	}
+
+	fcclk = clk_get_rate(rc->clk_reg) / USEC_PER_SEC;
+	firc = clk_get_rate(rc->clk_delay) / USEC_PER_SEC;
+	if (fcclk == 0 || firc == 0)
+		rc->delay_us = 2;
+	else
+		rc->delay_us = DIV_ROUND_UP(fcclk, firc * firc);
+
+	spin_lock_init(&rc->lock);
+
+	rc->rcdev.owner = THIS_MODULE;
+	rc->rcdev.nr_resets = 64;
+	rc->rcdev.ops = &lpc18xx_rgu_ops;
+	rc->rcdev.of_node = pdev->dev.of_node;
+
+	platform_set_drvdata(pdev, rc);
+
+	ret = reset_controller_register(&rc->rcdev);
+	if (ret) {
+		dev_err(&pdev->dev, "unable to register device\n");
+		goto dis_clks;
+	}
+
+	rgu_base = rc->base;
+	ret = register_restart_handler(&lpc18xx_rgu_restart_nb);
+	if (ret)
+		dev_warn(&pdev->dev, "failed to register restart handler\n");
+
+	return 0;
+
+dis_clks:
+	clk_disable_unprepare(rc->clk_delay);
+dis_clk_reg:
+	clk_disable_unprepare(rc->clk_reg);
+
+	return ret;
+}
+
+static int lpc18xx_rgu_remove(struct platform_device *pdev)
+{
+	struct lpc18xx_rgu_data *rc = platform_get_drvdata(pdev);
+	int ret;
+
+	ret = unregister_restart_handler(&lpc18xx_rgu_restart_nb);
+	if (ret)
+		dev_warn(&pdev->dev, "failed to unregister restart handler\n");
+
+	reset_controller_unregister(&rc->rcdev);
+
+	clk_disable_unprepare(rc->clk_delay);
+	clk_disable_unprepare(rc->clk_reg);
+
+	return 0;
+}
+
+static const struct of_device_id lpc18xx_rgu_match[] = {
+	{ .compatible = "nxp,lpc1850-rgu" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, lpc18xx_rgu_match);
+
+static struct platform_driver lpc18xx_rgu_driver = {
+	.probe	= lpc18xx_rgu_probe,
+	.remove	= lpc18xx_rgu_remove,
+	.driver	= {
+		.name		= "lpc18xx-reset",
+		.of_match_table	= lpc18xx_rgu_match,
+	},
+};
+module_platform_driver(lpc18xx_rgu_driver);
+
+MODULE_AUTHOR("Joachim Eastwood <manabian@gmail.com>");
+MODULE_DESCRIPTION("Reset driver for LPC18xx/43xx RGU");
+MODULE_LICENSE("GPL v2");
-- 
1.8.0

[PATCH 2/2] doc: dt: add documentation for lpc1850-rgu reset driver

[Joachim Eastwood]

Signed-off-by: Joachim Eastwood <manabian@gmail.com>
---
 .../devicetree/bindings/reset/nxp,lpc1850-rgu.txt  | 38 ++++++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/nxp,lpc1850-rgu.txt

diff --git a/Documentation/devicetree/bindings/reset/nxp,lpc1850-rgu.txt b/Documentation/devicetree/bindings/reset/nxp,lpc1850-rgu.txt
new file mode 100644
index 000000000000..45dbd6087731
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/nxp,lpc1850-rgu.txt
@@ -0,0 +1,38 @@
+NXP LPC1850  Reset Generation Unit (RGU)
+========================================
+
+Please also refer to reset.txt in this directory for common reset
+controller binding usage.
+
+Required properties:
+- compatible: Should be "nxp,lpc1850-rgu"
+- reg: register base and length
+- clocks: phandle and clock specifier to RGU clocks
+- clock-names: should contain "delay" and "reg"
+- #reset-cells: should be 1
+
+Refer to NXP LPC18xx or LPC43xx user manual for a description of the
+reset signals and the connected block/peripheral.
+
+Reset provider example:
+rgu: reset-controller at 40053000 {
+	compatible = "nxp,lpc1850-rgu";
+	reg = <0x40053000 0x1000>;
+	clocks = <&cgu BASE_SAFE_CLK>, <&ccu1 CLK_CPU_BUS>;
+	clock-names = "delay", "reg";
+	#reset-cells = <1>;
+};
+
+Reset consumer example:
+mac: ethernet at 40010000 {
+	compatible = "nxp,lpc1850-dwmac", "snps,dwmac-3.611", "snps,dwmac";
+	reg = <0x40010000 0x2000>;
+	interrupts = <5>;
+	interrupt-names = "macirq";
+	clocks = <&ccu1 CLK_CPU_ETHERNET>;
+	clock-names = "stmmaceth";
+	resets = <&rgu 22>;
+	reset-names = "stmmaceth";
+	status = "disabled";
+};
+
-- 
1.8.0

[PATCH 1/2] reset: add driver for lpc18xx rgu

[Philipp Zabel]

Hi Joachim,

thank you for the patch! Some comments below:

Am Dienstag, den 28.04.2015, 00:30 +0200 schrieb Joachim Eastwood:
> Signed-off-by: Joachim Eastwood <manabian@gmail.com>
> ---
>  drivers/reset/Makefile        |   1 +
>  drivers/reset/reset-lpc18xx.c | 260 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 261 insertions(+)
>  create mode 100644 drivers/reset/reset-lpc18xx.c
> 
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 157d421f755b..1d41feeb2dce 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -1,4 +1,5 @@
>  obj-$(CONFIG_RESET_CONTROLLER) += core.o
> +obj-$(CONFIG_ARCH_LPC18XX) += reset-lpc18xx.o
>  obj-$(CONFIG_ARCH_SOCFPGA) += reset-socfpga.o
>  obj-$(CONFIG_ARCH_BERLIN) += reset-berlin.o
>  obj-$(CONFIG_ARCH_SUNXI) += reset-sunxi.o
> diff --git a/drivers/reset/reset-lpc18xx.c b/drivers/reset/reset-lpc18xx.c
> new file mode 100644
> index 000000000000..9564d78ab9d2
> --- /dev/null
> +++ b/drivers/reset/reset-lpc18xx.c
> @@ -0,0 +1,260 @@
> +/*
> + * Reset driver for NXP LPC18xx/43xx Reset Generation Unit (RGU).
> + *
> + * Copyright (C) 2014 Joachim Eastwood <manabian@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/reboot.h>
> +#include <linux/reset-controller.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +
> +#include <asm/system_misc.h>

Are all of these necessary? I don't see why of_address.h and
system_misc.h are included, for example.

> +/* LPC18xx RGU registers */
> +#define LPC18XX_RGU_CTRL0		0x100
> +#define LPC18XX_RGU_CTRL1		0x104
> +#define LPC18XX_RGU_ACTIVE_STATUS0	0x150
> +#define LPC18XX_RGU_ACTIVE_STATUS1	0x154
> +
> +#define LPC18XX_RGU_RESETS_PER_REG	32
> +
> +/* Internal reset outputs */
> +#define LPC18XX_RGU_CORE_RST	0
> +#define LPC18XX_RGU_M0SUB_RST	12
> +#define LPC18XX_RGU_M0APP_RST	56

In the LPC18xx User Manual these are marked as reserved, I think using
the LPC43XX_ prefix here could avoid confusion. Or are there also
LPC18xx that have these reset lines?

> +
> +struct lpc18xx_rgu_data {
> +	struct reset_controller_dev rcdev;
> +	struct clk *clk_delay;
> +	struct clk *clk_reg;
> +	void __iomem *base;
> +	spinlock_t lock;
> +	u32 delay_us;
> +};
> +
> +#define to_rgu_data(rcdev) container_of(rcdev, struct lpc18xx_rgu_data, rcdev)
> +
> +static void __iomem *rgu_base;
> +
> +static int lpc18xx_rgu_restart(struct notifier_block *this, unsigned long mode,
> +			       void *cmd)
> +{
> +	writel(BIT(LPC18XX_RGU_CORE_RST), rgu_base + LPC18XX_RGU_CTRL0);
> +	mdelay(2000);
> +
> +	pr_emerg("%s: unable to restart system\n", __func__);
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block lpc18xx_rgu_restart_nb = {
> +	.notifier_call = lpc18xx_rgu_restart,
> +	.priority = 192,
> +};
> +
> +/*
> + * The LPC18xx RGU has mostly self-deasserting resets except for the
> + * two reset lines going to the internal Cortex-M0 cores.
> + *
> + * To prevent the M0 cores from accidentally getting deasserting the

"To prevent the M0 core resets from accidentally getting deasserted" ?

> + * status register must be check and bits in control register set to
> + * preserve the state.
> + */
> +static void lpc18xx_rgu_setclear_reset(struct reset_controller_dev *rcdev,
> +				       unsigned long id, bool set)
> +{
> +	struct lpc18xx_rgu_data *rc = to_rgu_data(rcdev);
> +	u32 stat_offset = LPC18XX_RGU_ACTIVE_STATUS0;
> +	u32 ctrl_offset = LPC18XX_RGU_CTRL0;
> +	unsigned long flags;
> +	u32 stat, rst_bit;
> +
> +	stat_offset += (id / LPC18XX_RGU_RESETS_PER_REG) * sizeof(u32);
> +	ctrl_offset += (id / LPC18XX_RGU_RESETS_PER_REG) * sizeof(u32);
> +	rst_bit = 1 << (id % LPC18XX_RGU_RESETS_PER_REG);
> +
> +	spin_lock_irqsave(&rc->lock, flags);
> +	stat = ~readl(rc->base + stat_offset);
> +	if (set)
> +		writel(stat | rst_bit, rc->base + ctrl_offset);
> +	else
> +		writel(stat & ~rst_bit, rc->base + ctrl_offset);
> +	spin_unlock_irqrestore(&rc->lock, flags);
> +}
> +
> +static int lpc18xx_rgu_reset(struct reset_controller_dev *rcdev,
> +			     unsigned long id)
> +{
> +	struct lpc18xx_rgu_data *rc = to_rgu_data(rcdev);
> +
> +	lpc18xx_rgu_setclear_reset(rcdev, id, true);
> +	udelay(rc->delay_us);
> +
> +	return 0;
> +}

This is missing a deassert for the M0 resets, see below.

> +
> +static int lpc18xx_rgu_assert(struct reset_controller_dev *rcdev,
> +			      unsigned long id)
> +{
> +	return lpc18xx_rgu_reset(rcdev, id);
> +}

lpc18xx_rgu_assert shouldn't include the delay. Could you have
lpc18xx_rgu_assert call lpc18xx_rgu_setclear_reset directly?

Let lpc18xx_rgu_reset then call lpc18xx_rgu_assert, udelay, and
lpc18xx_rgu_deassert. Or could you wait for the active status bit to
clear instead of the fixed delay for the self-clearing resets?

> +/* Only M0 cores require explicit reset deassert */
> +static int lpc18xx_rgu_deassert(struct reset_controller_dev *rcdev,
> +				unsigned long id)
> +{
> +	switch (id) {
> +	case LPC18XX_RGU_M0SUB_RST:
> +	case LPC18XX_RGU_M0APP_RST:
> +		lpc18xx_rgu_setclear_reset(rcdev, id, false);
> +	}
> +
> +	return 0;
> +}

If writing 0 to the self-clearing reset bits does nothing,
lpc18xx_rgu_setclear_reset(... false) could be called unconditionally.

[...]

regards
Philipp

[PATCH 2/2] doc: dt: add documentation for lpc1850-rgu reset driver

[Philipp Zabel]

Hi Joachim,

please Cc: devicetree at vger.kernel.org for new device tree binding
documentation.

Am Dienstag, den 28.04.2015, 00:30 +0200 schrieb Joachim Eastwood:
> Signed-off-by: Joachim Eastwood <manabian@gmail.com>
> ---
>  .../devicetree/bindings/reset/nxp,lpc1850-rgu.txt  | 38 ++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/nxp,lpc1850-rgu.txt
> 
> diff --git a/Documentation/devicetree/bindings/reset/nxp,lpc1850-rgu.txt b/Documentation/devicetree/bindings/reset/nxp,lpc1850-rgu.txt
> new file mode 100644
> index 000000000000..45dbd6087731
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/nxp,lpc1850-rgu.txt
> @@ -0,0 +1,38 @@
> +NXP LPC1850  Reset Generation Unit (RGU)
> +========================================
> +
> +Please also refer to reset.txt in this directory for common reset
> +controller binding usage.
> +
> +Required properties:
> +- compatible: Should be "nxp,lpc1850-rgu"
> +- reg: register base and length
> +- clocks: phandle and clock specifier to RGU clocks
> +- clock-names: should contain "delay" and "reg"
> +- #reset-cells: should be 1
> +
> +Refer to NXP LPC18xx or LPC43xx user manual for a description of the
> +reset signals and the connected block/peripheral.
> +
> +Reset provider example:
> +rgu: reset-controller at 40053000 {
> +	compatible = "nxp,lpc1850-rgu";
> +	reg = <0x40053000 0x1000>;
> +	clocks = <&cgu BASE_SAFE_CLK>, <&ccu1 CLK_CPU_BUS>;
> +	clock-names = "delay", "reg";
> +	#reset-cells = <1>;
> +};
> +
> +Reset consumer example:
> +mac: ethernet at 40010000 {
> +	compatible = "nxp,lpc1850-dwmac", "snps,dwmac-3.611", "snps,dwmac";
> +	reg = <0x40010000 0x2000>;
> +	interrupts = <5>;
> +	interrupt-names = "macirq";
> +	clocks = <&ccu1 CLK_CPU_ETHERNET>;
> +	clock-names = "stmmaceth";
> +	resets = <&rgu 22>;

I'd like the reset numbers to be documented in the binding docs. The
reset ordering seems to be the same across all users of this core?

Personally, I'd even prefer #defines like you have for the clock
indices. I know others dislike this, though.

> +	reset-names = "stmmaceth";
> +	status = "disabled";
> +};
> +
 ^^
Superfluous whitespace.

regards
Philipp

[PATCH 1/2] reset: add driver for lpc18xx rgu

[Joachim Eastwood]

On 4 May 2015 at 10:00, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Hi Joachim,
>
> thank you for the patch! Some comments below:
>
> Am Dienstag, den 28.04.2015, 00:30 +0200 schrieb Joachim Eastwood:
>> Signed-off-by: Joachim Eastwood <manabian@gmail.com>
>> ---
>>  drivers/reset/Makefile        |   1 +
>>  drivers/reset/reset-lpc18xx.c | 260 ++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 261 insertions(+)
>>  create mode 100644 drivers/reset/reset-lpc18xx.c
>>
>> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
>> index 157d421f755b..1d41feeb2dce 100644
>> --- a/drivers/reset/Makefile
>> +++ b/drivers/reset/Makefile
>> @@ -1,4 +1,5 @@
>>  obj-$(CONFIG_RESET_CONTROLLER) += core.o
>> +obj-$(CONFIG_ARCH_LPC18XX) += reset-lpc18xx.o
>>  obj-$(CONFIG_ARCH_SOCFPGA) += reset-socfpga.o
>>  obj-$(CONFIG_ARCH_BERLIN) += reset-berlin.o
>>  obj-$(CONFIG_ARCH_SUNXI) += reset-sunxi.o
>> diff --git a/drivers/reset/reset-lpc18xx.c b/drivers/reset/reset-lpc18xx.c
>> new file mode 100644
>> index 000000000000..9564d78ab9d2
>> --- /dev/null
>> +++ b/drivers/reset/reset-lpc18xx.c
>> @@ -0,0 +1,260 @@
>> +/*
>> + * Reset driver for NXP LPC18xx/43xx Reset Generation Unit (RGU).
>> + *
>> + * Copyright (C) 2014 Joachim Eastwood <manabian@gmail.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/reboot.h>
>> +#include <linux/reset-controller.h>
>> +#include <linux/slab.h>
>> +#include <linux/types.h>
>> +
>> +#include <asm/system_misc.h>
>
> Are all of these necessary? I don't see why of_address.h and
> system_misc.h are included, for example.

You are right, I'll clean them up.

>> +/* LPC18xx RGU registers */
>> +#define LPC18XX_RGU_CTRL0            0x100
>> +#define LPC18XX_RGU_CTRL1            0x104
>> +#define LPC18XX_RGU_ACTIVE_STATUS0   0x150
>> +#define LPC18XX_RGU_ACTIVE_STATUS1   0x154
>> +
>> +#define LPC18XX_RGU_RESETS_PER_REG   32
>> +
>> +/* Internal reset outputs */
>> +#define LPC18XX_RGU_CORE_RST 0
>> +#define LPC18XX_RGU_M0SUB_RST        12
>> +#define LPC18XX_RGU_M0APP_RST        56
>
> In the LPC18xx User Manual these are marked as reserved, I think using
> the LPC43XX_ prefix here could avoid confusion. Or are there also
> LPC18xx that have these reset lines?

No, they are only used on the 43xx series. I'll change the prefix.

I'll also add some comments in the DT doc on which reset lines that
are available on the different devices.

>> +
>> +struct lpc18xx_rgu_data {
>> +     struct reset_controller_dev rcdev;
>> +     struct clk *clk_delay;
>> +     struct clk *clk_reg;
>> +     void __iomem *base;
>> +     spinlock_t lock;
>> +     u32 delay_us;
>> +};
>> +
>> +#define to_rgu_data(rcdev) container_of(rcdev, struct lpc18xx_rgu_data, rcdev)
>> +
>> +static void __iomem *rgu_base;
>> +
>> +static int lpc18xx_rgu_restart(struct notifier_block *this, unsigned long mode,
>> +                            void *cmd)
>> +{
>> +     writel(BIT(LPC18XX_RGU_CORE_RST), rgu_base + LPC18XX_RGU_CTRL0);
>> +     mdelay(2000);
>> +
>> +     pr_emerg("%s: unable to restart system\n", __func__);
>> +
>> +     return NOTIFY_DONE;
>> +}
>> +
>> +static struct notifier_block lpc18xx_rgu_restart_nb = {
>> +     .notifier_call = lpc18xx_rgu_restart,
>> +     .priority = 192,
>> +};
>> +
>> +/*
>> + * The LPC18xx RGU has mostly self-deasserting resets except for the
>> + * two reset lines going to the internal Cortex-M0 cores.
>> + *
>> + * To prevent the M0 cores from accidentally getting deasserting the
>
> "To prevent the M0 core resets from accidentally getting deasserted" ?

That sounds better. I'll use it in the next version.

>> + * status register must be check and bits in control register set to
>> + * preserve the state.
>> + */
>> +static void lpc18xx_rgu_setclear_reset(struct reset_controller_dev *rcdev,
>> +                                    unsigned long id, bool set)
>> +{
>> +     struct lpc18xx_rgu_data *rc = to_rgu_data(rcdev);
>> +     u32 stat_offset = LPC18XX_RGU_ACTIVE_STATUS0;
>> +     u32 ctrl_offset = LPC18XX_RGU_CTRL0;
>> +     unsigned long flags;
>> +     u32 stat, rst_bit;
>> +
>> +     stat_offset += (id / LPC18XX_RGU_RESETS_PER_REG) * sizeof(u32);
>> +     ctrl_offset += (id / LPC18XX_RGU_RESETS_PER_REG) * sizeof(u32);
>> +     rst_bit = 1 << (id % LPC18XX_RGU_RESETS_PER_REG);
>> +
>> +     spin_lock_irqsave(&rc->lock, flags);
>> +     stat = ~readl(rc->base + stat_offset);
>> +     if (set)
>> +             writel(stat | rst_bit, rc->base + ctrl_offset);
>> +     else
>> +             writel(stat & ~rst_bit, rc->base + ctrl_offset);
>> +     spin_unlock_irqrestore(&rc->lock, flags);
>> +}
>> +
>> +static int lpc18xx_rgu_reset(struct reset_controller_dev *rcdev,
>> +                          unsigned long id)
>> +{
>> +     struct lpc18xx_rgu_data *rc = to_rgu_data(rcdev);
>> +
>> +     lpc18xx_rgu_setclear_reset(rcdev, id, true);
>> +     udelay(rc->delay_us);
>> +
>> +     return 0;
>> +}
>
> This is missing a deassert for the M0 resets, see below.
>
>> +
>> +static int lpc18xx_rgu_assert(struct reset_controller_dev *rcdev,
>> +                           unsigned long id)
>> +{
>> +     return lpc18xx_rgu_reset(rcdev, id);
>> +}
>
> lpc18xx_rgu_assert shouldn't include the delay. Could you have
> lpc18xx_rgu_assert call lpc18xx_rgu_setclear_reset directly?

I agree. I'll change it.

> Let lpc18xx_rgu_reset then call lpc18xx_rgu_assert, udelay, and
> lpc18xx_rgu_deassert.

Ok, that does indeed sound like a better call chain. I'll reorganize
the code a bit.

> Or could you wait for the active status bit to
> clear instead of the fixed delay for the self-clearing resets?

I am not sure. The manual explicitly mentions a software delay.

>From manual:
  Remark: The reset delay is counted in IRC clock cycles. If the
  core frequency CCLK is much higher than the IRC frequency, add
  a software delay of fCCLK/fIRC clock cycles between resetting
  and accessing any of the peripheral blocks.

So I think it's best to keep using the delay.

>> +/* Only M0 cores require explicit reset deassert */
>> +static int lpc18xx_rgu_deassert(struct reset_controller_dev *rcdev,
>> +                             unsigned long id)
>> +{
>> +     switch (id) {
>> +     case LPC18XX_RGU_M0SUB_RST:
>> +     case LPC18XX_RGU_M0APP_RST:
>> +             lpc18xx_rgu_setclear_reset(rcdev, id, false);
>> +     }
>> +
>> +     return 0;
>> +}
>
> If writing 0 to the self-clearing reset bits does nothing,
> lpc18xx_rgu_setclear_reset(... false) could be called unconditionally.

I added the check to document that the M0 reset are special. The next
version of the patch set will move it around a bit. If you still don't
like it can remove it then.


Thanks for your time reviewing.

regards,
Joachim Eastwood

[PATCH 2/2] doc: dt: add documentation for lpc1850-rgu reset driver

[Joachim Eastwood]

On 4 May 2015 at 10:51, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Hi Joachim,
>
> please Cc: devicetree at vger.kernel.org for new device tree binding
> documentation.

okey, sorry about that. Will do for the next version.

> Am Dienstag, den 28.04.2015, 00:30 +0200 schrieb Joachim Eastwood:
>> Signed-off-by: Joachim Eastwood <manabian@gmail.com>
>> ---
>>  .../devicetree/bindings/reset/nxp,lpc1850-rgu.txt  | 38 ++++++++++++++++++++++
>>  1 file changed, 38 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/reset/nxp,lpc1850-rgu.txt
>>
>> diff --git a/Documentation/devicetree/bindings/reset/nxp,lpc1850-rgu.txt b/Documentation/devicetree/bindings/reset/nxp,lpc1850-rgu.txt
>> new file mode 100644
>> index 000000000000..45dbd6087731
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/reset/nxp,lpc1850-rgu.txt
>> @@ -0,0 +1,38 @@
>> +NXP LPC1850  Reset Generation Unit (RGU)
>> +========================================
>> +
>> +Please also refer to reset.txt in this directory for common reset
>> +controller binding usage.
>> +
>> +Required properties:
>> +- compatible: Should be "nxp,lpc1850-rgu"
>> +- reg: register base and length
>> +- clocks: phandle and clock specifier to RGU clocks
>> +- clock-names: should contain "delay" and "reg"
>> +- #reset-cells: should be 1
>> +
>> +Refer to NXP LPC18xx or LPC43xx user manual for a description of the
>> +reset signals and the connected block/peripheral.
>> +
>> +Reset provider example:
>> +rgu: reset-controller at 40053000 {
>> +     compatible = "nxp,lpc1850-rgu";
>> +     reg = <0x40053000 0x1000>;
>> +     clocks = <&cgu BASE_SAFE_CLK>, <&ccu1 CLK_CPU_BUS>;
>> +     clock-names = "delay", "reg";
>> +     #reset-cells = <1>;
>> +};
>> +
>> +Reset consumer example:
>> +mac: ethernet at 40010000 {
>> +     compatible = "nxp,lpc1850-dwmac", "snps,dwmac-3.611", "snps,dwmac";
>> +     reg = <0x40010000 0x2000>;
>> +     interrupts = <5>;
>> +     interrupt-names = "macirq";
>> +     clocks = <&ccu1 CLK_CPU_ETHERNET>;
>> +     clock-names = "stmmaceth";
>> +     resets = <&rgu 22>;
>
> I'd like the reset numbers to be documented in the binding docs.

Yes, that is good idea. I'll add it.

>The reset ordering seems to be the same across all users of this
>core?

Yes, there are no overlapping reset numbers between the LPC18xx and
LPC43xx. The new lines that were added on LPC43xx used reserved
numbers on the LPC18xx.

> Personally, I'd even prefer #defines like you have for the clock
> indices. I know others dislike this, though.

Like Arnd I prefer the raw numbers in DT. At least for this controller
where the number are consistent between the parts and there is a nice
table in the data sheet.

So if you don't mind I would like to keep using the raw numbers.

>> +     reset-names = "stmmaceth";
>> +     status = "disabled";
>> +};
>> +
>  ^^
> Superfluous whitespace.

I'll remove it.


Thanks for reviewing.

regards,
Joachim Eastwood