[RFC] TI Reset Driver adapted to the reset core

[RFC] TI Reset Driver adapted to the reset core

[Dan Murphy]

This is a RFC on the TI reset adoption to the Reset framework.

The patchset was derived from work from Rajendra Nayak and Afzal Mohammed
who have had similar code offerings.  One of the major differences here
is the SoC data has been broken out so that the data and code are independent.

There is currently no OMAP4 or prior support added to this patch series.
Once the RFC is complete and the code infrastructure looks to be acceptable
I will add these data sets into the patchset.

Dan 

[RFC 01/11] drivers: reset: TI: SoC reset controller support.

[Dan Murphy]

The TI SoC reset controller support utilizes the
reset controller framework to give device drivers or
function drivers a common set of APIs to call to reset
a module.

The reset-ti is a common interface to the reset framework.
 The register data is retrieved during initialization
 of the reset driver through the reset-ti-data
file.  The array of data is associated with the compatible from the
respective DT entry.

Once the data is available then this is derefenced within the common
interface.

The device driver has the ability to assert, deassert or perform a
complete reset.

This code was derived from previous work by Rajendra Nayak and Afzal Mohammed.
The code was changed to adopt to the reset core and abstract away the SoC information.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 drivers/reset/Kconfig            |    1 +
 drivers/reset/Makefile           |    1 +
 drivers/reset/ti/Kconfig         |    8 ++
 drivers/reset/ti/Makefile        |    1 +
 drivers/reset/ti/reset-ti-data.h |   58 ++++++++++++
 drivers/reset/ti/reset-ti.c      |  195 ++++++++++++++++++++++++++++++++++++++
 include/linux/reset_ti.h         |   25 +++++
 7 files changed, 289 insertions(+)
 create mode 100644 drivers/reset/ti/Kconfig
 create mode 100644 drivers/reset/ti/Makefile
 create mode 100644 drivers/reset/ti/reset-ti-data.h
 create mode 100644 drivers/reset/ti/reset-ti.c
 create mode 100644 include/linux/reset_ti.h

diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index 0615f50..a58d789 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -13,3 +13,4 @@ menuconfig RESET_CONTROLLER
 	  If unsure, say no.
 
 source "drivers/reset/sti/Kconfig"
+source "drivers/reset/ti/Kconfig"
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index 4f60caf..1c8c444 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -1,3 +1,4 @@
 obj-$(CONFIG_RESET_CONTROLLER) += core.o
 obj-$(CONFIG_ARCH_SUNXI) += reset-sunxi.o
 obj-$(CONFIG_ARCH_STI) += sti/
+obj-$(CONFIG_RESET_TI) += ti/
diff --git a/drivers/reset/ti/Kconfig b/drivers/reset/ti/Kconfig
new file mode 100644
index 0000000..dcdce90
--- /dev/null
+++ b/drivers/reset/ti/Kconfig
@@ -0,0 +1,8 @@
+config RESET_TI
+	depends on RESET_CONTROLLER
+	bool "TI reset controller"
+	help
+	  Reset controller support for TI SoC's
+
+	  Reset controller found in TI's AM series of SoC's like
+	  AM335x and AM43x and OMAP SoC's like OMAP5 and DRA7
diff --git a/drivers/reset/ti/Makefile b/drivers/reset/ti/Makefile
new file mode 100644
index 0000000..55ab3f5
--- /dev/null
+++ b/drivers/reset/ti/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_RESET_TI) += reset-ti.o
diff --git a/drivers/reset/ti/reset-ti-data.h b/drivers/reset/ti/reset-ti-data.h
new file mode 100644
index 0000000..6afdf37
--- /dev/null
+++ b/drivers/reset/ti/reset-ti-data.h
@@ -0,0 +1,58 @@
+/*
+ * PRCM reset driver for TI SoC's
+ *
+ * Copyright 2014 Texas Instruments Inc.
+ *
+ * Author: Dan Murphy <dmurphy@ti.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.
+ */
+
+#ifndef _RESET_TI_DATA_H_
+#define _RESET_TI_DATA_H_
+
+#include <linux/kernel.h>
+#include <linux/of_device.h>
+#include <linux/reset-controller.h>
+
+/**
+ * struct ti_reset_reg_data - Structure of the reset register information
+ * 	for a particular SoC.
+ * @rstctrl_offs: This is the reset control offset value from
+ *		from the parent reset node.
+ * @rstst_offs: This is the reset status offset value from
+ *		from the parent reset node.
+ * @rstctrl_bit: This is the reset control bit for the module.
+ * @rstst_bit: This is the reset status bit for the module.
+ *
+ * This structure describes the reset register control and status offsets.
+ * The bits are also defined for the same.
+ */
+struct ti_reset_reg_data {
+	u32	rstctrl_offs;
+	u32	rstst_offs;
+	u8	rstctrl_bit;
+	u8	rstst_bit;
+};
+
+/**
+ * struct ti_reset_data - Structure that contains the reset register data
+ *	as well as the total number of resets for a particular SoC.
+ * @reg_data:	Pointer to the register data structure.
+ * @nr_resets:	Total number of resets for the SoC in the reset array.
+ *
+ * This structure contains a pointer to the register data and the modules
+ * register base.  The number of resets and reset controller device data is
+ * stored within this structure.
+ * 
+ */
+struct ti_reset_data {
+	struct ti_reset_reg_data *reg_data;
+	struct reset_controller_dev rcdev;
+	void __iomem *reg_base;
+	u8	nr_resets;
+};
+
+#endif
diff --git a/drivers/reset/ti/reset-ti.c b/drivers/reset/ti/reset-ti.c
new file mode 100644
index 0000000..1d38069
--- /dev/null
+++ b/drivers/reset/ti/reset-ti.c
@@ -0,0 +1,195 @@
+/*
+ * PRCM reset driver for TI SoC's
+ *
+ * Copyright 2014 Texas Instruments Inc.
+ *
+ * Author: Dan Murphy <dmurphy@ti.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/device.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/slab.h>
+
+#include <linux/reset_ti.h>
+#include <linux/reset.h>
+
+#include "reset-ti-data.h"
+
+static void ti_reset_wait_on_reset(struct reset_controller_dev *rcdev,
+			       unsigned long id)
+{
+	struct ti_reset_data *reset_data = container_of(rcdev,
+													struct ti_reset_data,
+													rcdev);
+	void __iomem *status_reg;
+	u32 val = 0;
+	u8 status_bit = 0;
+
+	if (id < 0) {
+		pr_err("%s: ID passed is invalid\n", __func__);
+		return;
+	}
+
+	/* Clear the reset status bit to reflect the current status */
+	status_reg = reset_data->reg_base + reset_data->reg_data[id].rstst_offs;
+	status_bit = reset_data->reg_data[id].rstst_bit;
+	do {
+		val = readl(status_reg);
+		if (!(val & (1 << status_bit)))
+			break;
+	} while (1);
+}
+
+static int ti_reset_assert(struct reset_controller_dev *rcdev,
+			       unsigned long id)
+{
+	struct ti_reset_data *reset_data = container_of(rcdev,
+													struct ti_reset_data,
+													rcdev);
+	void __iomem *reg;
+	void __iomem *status_reg;
+	u32 val = 0;
+	u8 bit = 0;
+	u8 status_bit = 0;
+
+	if (id < 0) {
+		pr_err("%s: ID passed is invalid\n", __func__);
+		return -ENODEV;
+	}
+
+	/* Clear the reset status bit to reflect the current status */
+	status_reg = reset_data->reg_base + reset_data->reg_data[id].rstst_offs;
+	status_bit = reset_data->reg_data[id].rstst_bit;
+	writel(1 << status_bit, status_reg);
+
+	reg = reset_data->reg_base + reset_data->reg_data[id].rstctrl_offs;
+	bit = reset_data->reg_data[id].rstctrl_bit;
+	val = readl(reg);
+	if (!(val & (1 << bit))) {
+		val |= (1 << bit);
+		writel(val, reg);
+	}
+
+	return 0;
+}
+
+static int ti_reset_deassert(struct reset_controller_dev *rcdev,
+			       unsigned long id)
+{
+
+	struct ti_reset_data *reset_data;
+	void __iomem *reg;
+	void __iomem *status_reg;
+	u32 val = 0;
+	u8 bit = 0;
+	u8 status_bit = 0;
+
+	if (id < 0) {
+		pr_err("%s: reset ID passed is invalid\n", __func__);
+		return -ENODEV;
+	}
+
+	reset_data = container_of(rcdev, struct ti_reset_data, rcdev);
+
+	/* Clear the reset status bit to reflect the current status */
+	status_reg = reset_data->reg_base + reset_data->reg_data[id].rstst_offs;
+	status_bit = reset_data->reg_data[id].rstst_bit;
+	writel(1 << status_bit, status_reg);
+
+	reg = reset_data->reg_base + reset_data->reg_data[id].rstctrl_offs;
+	bit = reset_data->reg_data[id].rstctrl_bit;
+	val = readl(reg);
+	if (val & (1 << bit)) {
+		val &= ~(1 << bit);
+		writel(val, reg);
+	}
+
+	return 0;
+}
+
+static int ti_reset_reset(struct reset_controller_dev *rcdev,
+				  unsigned long id)
+{
+	ti_reset_assert(rcdev, id);
+	ti_reset_deassert(rcdev, id);
+	ti_reset_wait_on_reset(rcdev, id);
+
+	return 0;
+}
+
+static struct reset_control_ops ti_reset_ops = {
+	.reset = ti_reset_reset,
+	.assert = ti_reset_assert,
+	.deassert = ti_reset_deassert,
+};
+
+static const struct of_device_id ti_reset_of_match[] = {
+	{},
+};
+
+const struct of_device_id *ti_reset_get_data(struct device_node *parent)
+{
+	const struct of_device_id *dev_node;
+
+	dev_node = of_match_node(ti_reset_of_match, parent);
+	if (!dev_node) {
+		pr_err("%s: No compatible for resets for %s\n",
+			__func__, parent->name);
+		return NULL;
+	}
+
+	return dev_node;
+}
+
+void __init ti_dt_reset_init(void)
+{
+	struct ti_reset_data *ti_data;
+	struct device_node *parent;
+	struct device_node *resets;
+	const struct of_device_id *dev_node;
+
+	resets = of_find_node_by_name(NULL, "resets");
+	if (!resets) {
+		pr_err("%s: missing 'resets' child node.\n", __func__);
+		return;
+	}
+
+	parent = of_get_parent(resets);
+	if (!parent) {
+		pr_err("%s: Cannot find parent reset node\n", __func__);
+		return;
+	}
+
+	ti_data = kzalloc(sizeof(*ti_data), GFP_KERNEL);
+	if (!ti_data)
+		return;
+
+	dev_node = ti_reset_get_data(resets);
+	if (!dev_node) {
+		pr_err("%s: Cannot find data for node\n", __func__);
+		return;
+	}
+
+	ti_data = (struct ti_reset_data *) dev_node->data;
+
+	ti_data->reg_base = of_iomap(parent, 0);
+	if (!ti_data->reg_base) {
+		pr_err("%s: Cannot map reset parent.\n", __func__);
+		return;
+	}
+
+	ti_data->rcdev.owner = THIS_MODULE;
+	ti_data->rcdev.nr_resets = ti_data->nr_resets;
+	ti_data->rcdev.of_node = resets;
+	ti_data->rcdev.ops = &ti_reset_ops;
+
+	reset_controller_register(&ti_data->rcdev);
+}
diff --git a/include/linux/reset_ti.h b/include/linux/reset_ti.h
new file mode 100644
index 0000000..d18f47f
--- /dev/null
+++ b/include/linux/reset_ti.h
@@ -0,0 +1,25 @@
+/*
+ * TI reset driver support
+ *
+ * Copyright (C) 2014 Texas Instruments, Inc.
+ *
+ * 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 "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef _RESET_TI_H_
+#define _RESET_TI_H_
+
+#ifdef CONFIG_RESET_TI
+void ti_dt_reset_init(void);
+#else
+static inline void ti_dt_reset_init(void){ return; };
+#endif
+
+#endif
-- 
1.7.9.5

[RFC 02/11] drivers: reset: dra7: Add reset data for dra7xx

[Dan Murphy]

Add the reset register data for the dra7xx SoC.
Include the dt-bindings header to properly index
the right reset node.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 drivers/reset/ti/Makefile          |    1 +
 drivers/reset/ti/reset-ti-data.h   |    1 +
 drivers/reset/ti/reset-ti-dra7xx.c |   61 ++++++++++++++++++++++++++++++++++++
 drivers/reset/ti/reset-ti.c        |    3 ++
 4 files changed, 66 insertions(+)
 create mode 100644 drivers/reset/ti/reset-ti-dra7xx.c

diff --git a/drivers/reset/ti/Makefile b/drivers/reset/ti/Makefile
index 55ab3f5..622eb3b 100644
--- a/drivers/reset/ti/Makefile
+++ b/drivers/reset/ti/Makefile
@@ -1 +1,2 @@
 obj-$(CONFIG_RESET_TI) += reset-ti.o
+obj-$(CONFIG_SOC_DRA7XX) += reset-ti-dra7xx.o
diff --git a/drivers/reset/ti/reset-ti-data.h b/drivers/reset/ti/reset-ti-data.h
index 6afdf37..5812ed5 100644
--- a/drivers/reset/ti/reset-ti-data.h
+++ b/drivers/reset/ti/reset-ti-data.h
@@ -55,4 +55,5 @@ struct ti_reset_data {
 	u8	nr_resets;
 };
 
+extern struct ti_reset_data dra7_reset_data;
 #endif
diff --git a/drivers/reset/ti/reset-ti-dra7xx.c b/drivers/reset/ti/reset-ti-dra7xx.c
new file mode 100644
index 0000000..764f83e
--- /dev/null
+++ b/drivers/reset/ti/reset-ti-dra7xx.c
@@ -0,0 +1,61 @@
+/*
+ * dra7xx reset data for PRCM Module
+ *
+ * Copyright 2014 Texas Instruments Inc.
+ *
+ * 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 "reset-ti-data.h"
+
+static struct ti_reset_reg_data dra7_reset_reg_data[] = {
+	{
+		.rstctrl_offs = 0x1D00,
+		.rstctrl_bit = 0x0,
+		.rstst_offs	= 0x1D04,
+		.rstst_bit = 0x0,
+	},
+	{
+		.rstctrl_offs = 0x410,
+		.rstctrl_bit = 0x0,
+		.rstst_offs	= 0x414,
+		.rstst_bit = 0x0,
+	},
+	{
+		.rstctrl_offs = 0x510,
+		.rstctrl_bit = 0x0,
+		.rstst_offs	= 0x514,
+		.rstst_bit = 0x0,
+	}, /* IPU_CPU0 */
+	{
+		.rstctrl_offs = 0x510,
+		.rstctrl_bit = 0x1,
+		.rstst_offs	= 0x514,
+		.rstst_bit = 0x1,
+	}, /* IPU_CPU1 */
+	{
+		.rstctrl_offs = 0x510,
+		.rstctrl_bit = 0x2,
+		.rstst_offs	= 0x514,
+		.rstst_bit = 0x2,
+	}, /* IPU_MMU_CACHE */
+	{
+		.rstctrl_offs = 0xf10,
+		.rstctrl_bit = 0,
+		.rstst_offs	= 0xf14,
+		.rstst_bit	= 0,
+	},
+	{
+		.rstctrl_offs = 0x1310,
+		.rstctrl_bit = 0,
+		.rstst_offs	= 0x1314,
+		.rstst_bit	= 0,
+	},
+};
+
+struct ti_reset_data dra7_reset_data = {
+	.reg_data	= dra7_reset_reg_data,
+	.nr_resets	= ARRAY_SIZE(dra7_reset_reg_data),
+};
diff --git a/drivers/reset/ti/reset-ti.c b/drivers/reset/ti/reset-ti.c
index 1d38069..486b77c 100644
--- a/drivers/reset/ti/reset-ti.c
+++ b/drivers/reset/ti/reset-ti.c
@@ -132,6 +132,9 @@ static struct reset_control_ops ti_reset_ops = {
 };
 
 static const struct of_device_id ti_reset_of_match[] = {
+#ifdef CONFIG_SOC_DRA7XX
+	{ .compatible = "ti,dra7-resets", .data = &dra7_reset_data,},
+#endif
 	{},
 };
 
-- 
1.7.9.5

[RFC 03/11] drivers: reset: omap5: Add reset data for omap5

[Dan Murphy]

Add the reset register data for the omap5 SoC.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 drivers/reset/ti/Makefile         |    1 +
 drivers/reset/ti/reset-ti-data.h  |    2 ++
 drivers/reset/ti/reset-ti-omap5.c |   61 +++++++++++++++++++++++++++++++++++++
 drivers/reset/ti/reset-ti.c       |    3 ++
 4 files changed, 67 insertions(+)
 create mode 100644 drivers/reset/ti/reset-ti-omap5.c

diff --git a/drivers/reset/ti/Makefile b/drivers/reset/ti/Makefile
index 622eb3b..4d2d06c 100644
--- a/drivers/reset/ti/Makefile
+++ b/drivers/reset/ti/Makefile
@@ -1,2 +1,3 @@
 obj-$(CONFIG_RESET_TI) += reset-ti.o
 obj-$(CONFIG_SOC_DRA7XX) += reset-ti-dra7xx.o
+obj-$(CONFIG_SOC_OMAP5) += reset-ti-omap5.o
diff --git a/drivers/reset/ti/reset-ti-data.h b/drivers/reset/ti/reset-ti-data.h
index 5812ed5..d3c6d42 100644
--- a/drivers/reset/ti/reset-ti-data.h
+++ b/drivers/reset/ti/reset-ti-data.h
@@ -56,4 +56,6 @@ struct ti_reset_data {
 };
 
 extern struct ti_reset_data dra7_reset_data;
+extern struct ti_reset_data omap5_reset_data;
+
 #endif
diff --git a/drivers/reset/ti/reset-ti-omap5.c b/drivers/reset/ti/reset-ti-omap5.c
new file mode 100644
index 0000000..3e7529c
--- /dev/null
+++ b/drivers/reset/ti/reset-ti-omap5.c
@@ -0,0 +1,61 @@
+/*
+ * OMAP5 reset data for PRCM Module
+ *
+ * Copyright 2014 Texas Instruments Inc.
+ *
+ * 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 "reset-ti-data.h"
+
+static struct ti_reset_reg_data omap5_reset_reg_data[] = {
+	{
+		.rstctrl_offs = 0x1c00,
+		.rstctrl_bit = 0x0,
+		.rstst_offs	= 0x1c04,
+		.rstst_bit = 0x0,
+	},
+	{
+		.rstctrl_offs = 0x410,
+		.rstctrl_bit = 0x0,
+		.rstst_offs	= 0x414,
+		.rstst_bit = 0x0,
+	}, /* DSP_RESET */
+	{
+		.rstctrl_offs = 0x410,
+		.rstctrl_bit = 0x1,
+		.rstst_offs	= 0x414,
+		.rstst_bit = 0x1,
+	}, /* DSP_MMU_CACHE */
+	{
+		.rstctrl_offs = 0x910,
+		.rstctrl_bit = 0x0,
+		.rstst_offs	= 0x914,
+		.rstst_bit = 0x0,
+	}, /* IPU_CPU0 */
+	{
+		.rstctrl_offs = 0x910,
+		.rstctrl_bit = 0x1,
+		.rstst_offs	= 0x914,
+		.rstst_bit = 0x1,
+	}, /* IPU_CPU1 */
+	{
+		.rstctrl_offs = 0x910,
+		.rstctrl_bit = 0x2,
+		.rstst_offs	= 0x914,
+		.rstst_bit = 0x2,
+	}, /* IPU_MMU_CACHE */
+	{
+		.rstctrl_offs = 0x1210,
+		.rstctrl_bit = 0,
+		.rstst_offs	= 0x1214,
+		.rstst_bit	= 0,
+	},
+};
+
+struct ti_reset_data omap5_reset_data = {
+	.reg_data	= omap5_reset_reg_data,
+	.nr_resets	= ARRAY_SIZE(omap5_reset_reg_data),
+};
diff --git a/drivers/reset/ti/reset-ti.c b/drivers/reset/ti/reset-ti.c
index 486b77c..7310827 100644
--- a/drivers/reset/ti/reset-ti.c
+++ b/drivers/reset/ti/reset-ti.c
@@ -135,6 +135,9 @@ static const struct of_device_id ti_reset_of_match[] = {
 #ifdef CONFIG_SOC_DRA7XX
 	{ .compatible = "ti,dra7-resets", .data = &dra7_reset_data,},
 #endif
+#ifdef CONFIG_SOC_OMAP5
+	{ .compatible = "ti,omap5-resets", .data = &omap5_reset_data,},
+#endif
 	{},
 };
 
-- 
1.7.9.5

[RFC 04/11] drivers: reset: am335: Add reset data for am335

[Dan Murphy]

Add the reset register data for the am335 SoC.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 drivers/reset/ti/Makefile          |    1 +
 drivers/reset/ti/reset-ti-am33xx.c |   37 ++++++++++++++++++++++++++++++++++++
 drivers/reset/ti/reset-ti-data.h   |    1 +
 drivers/reset/ti/reset-ti.c        |    3 +++
 4 files changed, 42 insertions(+)
 create mode 100644 drivers/reset/ti/reset-ti-am33xx.c

diff --git a/drivers/reset/ti/Makefile b/drivers/reset/ti/Makefile
index 4d2d06c..f0dbd01 100644
--- a/drivers/reset/ti/Makefile
+++ b/drivers/reset/ti/Makefile
@@ -1,3 +1,4 @@
 obj-$(CONFIG_RESET_TI) += reset-ti.o
 obj-$(CONFIG_SOC_DRA7XX) += reset-ti-dra7xx.o
 obj-$(CONFIG_SOC_OMAP5) += reset-ti-omap5.o
+obj-$(CONFIG_SOC_AM33XX) += reset-ti-am33xx.o
diff --git a/drivers/reset/ti/reset-ti-am33xx.c b/drivers/reset/ti/reset-ti-am33xx.c
new file mode 100644
index 0000000..d28f41b
--- /dev/null
+++ b/drivers/reset/ti/reset-ti-am33xx.c
@@ -0,0 +1,37 @@
+/*
+ * am33xx reset data for PRCM Module
+ *
+ * Copyright 2014 Texas Instruments Inc.
+ *
+ * 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 "reset-ti-data.h"
+
+static struct ti_reset_reg_data am335x_reset_reg_data[] = {
+	{
+		.rstctrl_offs = 0xf00,
+		.rstctrl_bit = 0,
+		.rstst_offs	= 0xf08,
+		.rstst_bit	= 0,
+	},
+	{
+		.rstctrl_offs = 0x1104,
+		.rstctrl_bit = 0,
+		.rstst_offs	= 0x1114,
+		.rstst_bit	= 0,
+	},
+	{
+		.rstctrl_offs = 0x0D00,
+		.rstctrl_bit = 3,
+		.rstst_offs	= 0x0D0C,
+		.rstst_bit	= 5,
+	},
+};
+
+struct ti_reset_data am335x_reset_data = {
+	.reg_data	= am335x_reset_reg_data,
+	.nr_resets	= ARRAY_SIZE(am335x_reset_reg_data),
+};
diff --git a/drivers/reset/ti/reset-ti-data.h b/drivers/reset/ti/reset-ti-data.h
index d3c6d42..48a1476 100644
--- a/drivers/reset/ti/reset-ti-data.h
+++ b/drivers/reset/ti/reset-ti-data.h
@@ -57,5 +57,6 @@ struct ti_reset_data {
 
 extern struct ti_reset_data dra7_reset_data;
 extern struct ti_reset_data omap5_reset_data;
+extern struct ti_reset_data am335x_reset_data;
 
 #endif
diff --git a/drivers/reset/ti/reset-ti.c b/drivers/reset/ti/reset-ti.c
index 7310827..146353f 100644
--- a/drivers/reset/ti/reset-ti.c
+++ b/drivers/reset/ti/reset-ti.c
@@ -138,6 +138,9 @@ static const struct of_device_id ti_reset_of_match[] = {
 #ifdef CONFIG_SOC_OMAP5
 	{ .compatible = "ti,omap5-resets", .data = &omap5_reset_data,},
 #endif
+#ifdef CONFIG_SOC_AM33XX
+	{ .compatible = "ti,am335x-resets", .data = &am335x_reset_data,},
+#endif
 	{},
 };
 
-- 
1.7.9.5

[RFC 05/11] drivers: reset: am43xx: Add reset data for am43xx

[Dan Murphy]

Add the reset register data for the am43xx SoC.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 drivers/reset/ti/Makefile          |    1 +
 drivers/reset/ti/reset-ti-am43xx.c |   43 ++++++++++++++++++++++++++++++++++++
 drivers/reset/ti/reset-ti-data.h   |    1 +
 3 files changed, 45 insertions(+)
 create mode 100644 drivers/reset/ti/reset-ti-am43xx.c

diff --git a/drivers/reset/ti/Makefile b/drivers/reset/ti/Makefile
index f0dbd01..3d362c2 100644
--- a/drivers/reset/ti/Makefile
+++ b/drivers/reset/ti/Makefile
@@ -2,3 +2,4 @@ obj-$(CONFIG_RESET_TI) += reset-ti.o
 obj-$(CONFIG_SOC_DRA7XX) += reset-ti-dra7xx.o
 obj-$(CONFIG_SOC_OMAP5) += reset-ti-omap5.o
 obj-$(CONFIG_SOC_AM33XX) += reset-ti-am33xx.o
+obj-$(CONFIG_SOC_AM43XX) += reset-ti-am43xx.o
diff --git a/drivers/reset/ti/reset-ti-am43xx.c b/drivers/reset/ti/reset-ti-am43xx.c
new file mode 100644
index 0000000..cfb283b
--- /dev/null
+++ b/drivers/reset/ti/reset-ti-am43xx.c
@@ -0,0 +1,43 @@
+/*
+ * am43xx reset data for PRCM Module
+ *
+ * Copyright 2014 Texas Instruments Inc.
+ *
+ * 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 "reset-ti-data.h"
+
+static struct ti_reset_reg_data am43x_reset_reg_data[] = {
+	{
+		.rstctrl_offs = 0x4000,
+		.rstctrl_bit = 0,
+		.rstst_offs	= 0x4004,
+		.rstst_bit	= 0,
+	},
+	{
+		.rstctrl_offs = 0x810,
+		.rstctrl_bit = 1,
+		.rstst_offs	= 0x814,
+		.rstst_bit	= 0,
+	},
+	{
+		.rstctrl_offs	= 0x410,
+		.rstst_offs	= 0x414,
+		.rstctrl_bit	= 0,
+		.rstst_bit	= 0,
+	},
+	{
+		.rstctrl_offs	= 0x2010,
+		.rstst_offs	= 0x2014,
+		.rstctrl_bit	= 3,
+		.rstst_bit	= 5,
+	},
+};
+
+struct ti_reset_data am43x_reset_data = {
+	.reg_data	= am43x_reset_reg_data,
+	.nr_resets	= ARRAY_SIZE(am43x_reset_reg_data),
+};
diff --git a/drivers/reset/ti/reset-ti-data.h b/drivers/reset/ti/reset-ti-data.h
index 48a1476..f56f1a5 100644
--- a/drivers/reset/ti/reset-ti-data.h
+++ b/drivers/reset/ti/reset-ti-data.h
@@ -58,5 +58,6 @@ struct ti_reset_data {
 extern struct ti_reset_data dra7_reset_data;
 extern struct ti_reset_data omap5_reset_data;
 extern struct ti_reset_data am335x_reset_data;
+extern struct ti_reset_data am43x_reset_data;
 
 #endif
-- 
1.7.9.5

[RFC 06/11] ARM: OMAP: Add reset init to prcm file

[Dan Murphy]

Call the reset init API to initialize the
reset framework and data for the related SoC.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 arch/arm/mach-omap2/prm_common.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/mach-omap2/prm_common.c b/arch/arm/mach-omap2/prm_common.c
index b4c4ab9..7e95f8e 100644
--- a/arch/arm/mach-omap2/prm_common.c
+++ b/arch/arm/mach-omap2/prm_common.c
@@ -27,6 +27,7 @@
 #include <linux/of_address.h>
 #include <linux/clk-provider.h>
 #include <linux/clk/ti.h>
+#include <linux/reset_ti.h>
 
 #include "soc.h"
 #include "prm2xxx_3xxx.h"
@@ -523,10 +524,13 @@ int __init of_prcm_init(void)
 		mem = of_iomap(np, 0);
 		clk_memmaps[memmap_index] = mem;
 		ti_dt_clk_init_provider(np, memmap_index);
+
 		memmap_index++;
 	}
 
 	ti_dt_clockdomains_setup();
 
+	ti_dt_reset_init();
+
 	return 0;
 }
-- 
1.7.9.5

[RFC 07/11] ARM: TI: Describe the ti reset DT entries

[Dan Murphy]

Describe the TI reset DT entries for TI SoC's.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 .../devicetree/bindings/reset/ti,reset.txt         |   34 ++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/ti,reset.txt

diff --git a/Documentation/devicetree/bindings/reset/ti,reset.txt b/Documentation/devicetree/bindings/reset/ti,reset.txt
new file mode 100644
index 0000000..19f5603
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/ti,reset.txt
@@ -0,0 +1,34 @@
+Texas Instruments Reset Controller
+======================================
+Please also refer to reset.txt in this directory for common reset
+controller binding usage.
+
+Specifying the reset entries for the IP module
+==============================================
+Parent module:
+This is the module node that contains the reset registers and bits.
+
+example:
+			prcm_resets: resets {
+				compatible = "ti,dra7-resets";
+				#reset-cells = <1>;
+			};
+
+Required Properties:
+compatible - Should be "ti-<Soc>-resets
+
+DRA7xx:
+Compatible - ti,dra7-resets
+Reset indices - include/dt-bindings/reset/ti,dra7-resets.h
+
+OMAP5:
+Compatible - ti,omap5-resets
+Reset indices - include/dt-bindings/reset/ti,omap5-resets.h
+
+AM33xx:
+Compatible - ti-am335x-resets
+Reset indices - include/dt-bindings/reset/ti,am33xx-resets.h
+
+AM43xx:
+Compatible - ti,am4372-resets
+Reset indices - include/dt-bindings/reset/ti,am437x-resets.h
-- 
1.7.9.5

[RFC 08/11] ARM: dts: am33xx: Add prcm_resets node

[Dan Murphy]

Add the prcm_resets node to the prcm parent node.
Add the dt-bindings header to the DT file

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 arch/arm/boot/dts/am33xx.dtsi                |    6 ++++++
 include/dt-bindings/reset/ti,am33xx-resets.h |   18 ++++++++++++++++++
 2 files changed, 24 insertions(+)
 create mode 100644 include/dt-bindings/reset/ti,am33xx-resets.h

diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index 07f283c..df9d9c6 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -10,6 +10,7 @@
 
 #include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/pinctrl/am33xx.h>
+#include <dt-bindings/reset/ti,am33xx-resets.h>
 
 #include "skeleton.dtsi"
 
@@ -117,6 +118,11 @@
 
 			prcm_clockdomains: clockdomains {
 			};
+
+			prcm_resets: resets {
+				compatible = "ti,am335x-resets";
+				#reset-cells = <1>;
+			};
 		};
 
 		scrm: scrm@44e10000 {
diff --git a/include/dt-bindings/reset/ti,am33xx-resets.h b/include/dt-bindings/reset/ti,am33xx-resets.h
new file mode 100644
index 0000000..dfe7954
--- /dev/null
+++ b/include/dt-bindings/reset/ti,am33xx-resets.h
@@ -0,0 +1,18 @@
+/*
+ * AM33xx reset index for PRCM Module
+ *
+ * Copyright 2014 Texas Instruments Inc.
+ *
+ * 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.
+ */
+
+#ifndef _DT_BINDINGS_RESET_TI_AM33XX_H
+#define _DT_BINDINGS_RESET_TI_AM33XX_H
+
+#define RESET_DEVICE_RESET			0
+#define RESET_GFX_RESET				1
+#define RESET_PER_RESET				2
+
+#endif
-- 
1.7.9.5

[RFC 09/11] ARM: dts: am4372: Add prcm_resets node

[Dan Murphy]

Add the prcm_resets node to the prcm parent node.
Add the dt-bindings header to the DT file

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 arch/arm/boot/dts/am4372.dtsi                |    6 ++++++
 include/dt-bindings/reset/ti,am437x-resets.h |   19 +++++++++++++++++++
 2 files changed, 25 insertions(+)
 create mode 100644 include/dt-bindings/reset/ti,am437x-resets.h

diff --git a/arch/arm/boot/dts/am4372.dtsi b/arch/arm/boot/dts/am4372.dtsi
index 36d523a..f8bd4ca 100644
--- a/arch/arm/boot/dts/am4372.dtsi
+++ b/arch/arm/boot/dts/am4372.dtsi
@@ -10,6 +10,7 @@
 
 #include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
+#include <dt-bindings/reset/ti,am437x-resets.h>
 
 #include "skeleton.dtsi"
 
@@ -84,6 +85,11 @@
 
 			prcm_clockdomains: clockdomains {
 			};
+
+			prcm_resets: resets {
+				compatible = "ti,am4372-resets";
+				#reset-cells = <1>;
+			};
 		};
 
 		scrm: scrm@44e10000 {
diff --git a/include/dt-bindings/reset/ti,am437x-resets.h b/include/dt-bindings/reset/ti,am437x-resets.h
new file mode 100644
index 0000000..cf3e329
--- /dev/null
+++ b/include/dt-bindings/reset/ti,am437x-resets.h
@@ -0,0 +1,19 @@
+/*
+ * AM437x reset index for PRCM Module
+ *
+ * Copyright 2014 Texas Instruments Inc.
+ *
+ * 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.
+ */
+
+#ifndef _DT_BINDINGS_RESET_TI_AM437X_H
+#define _DT_BINDINGS_RESET_TI_AM437X_H
+
+#define RESET_DEVICE_RESET			0
+#define RESET_ICSS_RESET			1
+#define RESET_GFX_RESET				2
+#define RESET_PER_RESET				3
+
+#endif
-- 
1.7.9.5

[RFC 10/11] ARM: dts: dra7: Add prm_resets node

[Dan Murphy]

Add the prcm_resets node to the prm parent node.
Add the dt-bindings header to the DT file

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 arch/arm/boot/dts/dra7.dtsi                |    6 ++++++
 include/dt-bindings/reset/ti,dra7-resets.h |   22 ++++++++++++++++++++++
 2 files changed, 28 insertions(+)
 create mode 100644 include/dt-bindings/reset/ti,dra7-resets.h

diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
index 149b550..3507730 100644
--- a/arch/arm/boot/dts/dra7.dtsi
+++ b/arch/arm/boot/dts/dra7.dtsi
@@ -9,6 +9,7 @@
 
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/pinctrl/dra.h>
+#include <dt-bindings/reset/ti,dra7-resets.h>
 
 #include "skeleton.dtsi"
 
@@ -120,6 +121,11 @@
 
 			prm_clockdomains: clockdomains {
 			};
+
+			prm_resets: resets {
+				compatible = "ti,dra7-resets";
+				#reset-cells = <1>;
+			};
 		};
 
 		cm_core_aon: cm_core_aon@4a005000 {
diff --git a/include/dt-bindings/reset/ti,dra7-resets.h b/include/dt-bindings/reset/ti,dra7-resets.h
new file mode 100644
index 0000000..89be87b
--- /dev/null
+++ b/include/dt-bindings/reset/ti,dra7-resets.h
@@ -0,0 +1,22 @@
+/*
+ * DRA7xx reset index for PRCM Module
+ *
+ * Copyright 2014 Texas Instruments Inc.
+ *
+ * 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.
+ */
+
+#ifndef _DT_BINDINGS_RESET_TI_DRA7_H
+#define _DT_BINDINGS_RESET_TI_DRA7_H
+
+#define RESET_DEVICE_RESET			0
+#define RESET_DSP1_RESET			1
+#define RESET_IPU_CPU0_RESET		2
+#define RESET_IPU_CPU1_RESET		3
+#define RESET_IPU_MMU_CACHE_RESET	4
+#define RESET_IVA_RESET				5
+#define RESET_PCIE_RESET			6
+
+#endif
-- 
1.7.9.5

[RFC 11/11] ARM: dts: omap5: Add prm_resets node

[Dan Murphy]

Add the prm_resets node to the prm parent node.
Add the dt-bindings header to the DT file

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 arch/arm/boot/dts/omap5.dtsi                |    6 ++++++
 include/dt-bindings/reset/ti,omap5-resets.h |   22 ++++++++++++++++++++++
 2 files changed, 28 insertions(+)
 create mode 100644 include/dt-bindings/reset/ti,omap5-resets.h

diff --git a/arch/arm/boot/dts/omap5.dtsi b/arch/arm/boot/dts/omap5.dtsi
index f8c9855..82eebe7 100644
--- a/arch/arm/boot/dts/omap5.dtsi
+++ b/arch/arm/boot/dts/omap5.dtsi
@@ -10,6 +10,7 @@
 #include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/pinctrl/omap.h>
+#include <dt-bindings/reset/ti,omap5-resets.h>
 
 #include "skeleton.dtsi"
 
@@ -134,6 +135,11 @@
 
 			prm_clockdomains: clockdomains {
 			};
+
+			prm_resets: resets {
+				compatible = "ti,omap5-resets";
+				#reset-cells = <1>;
+			};
 		};
 
 		cm_core_aon: cm_core_aon@4a004000 {
diff --git a/include/dt-bindings/reset/ti,omap5-resets.h b/include/dt-bindings/reset/ti,omap5-resets.h
new file mode 100644
index 0000000..33bb295
--- /dev/null
+++ b/include/dt-bindings/reset/ti,omap5-resets.h
@@ -0,0 +1,22 @@
+/*
+ * OMAP5 reset index for PRCM Module
+ *
+ * Copyright 2014 Texas Instruments Inc.
+ *
+ * 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.
+ */
+
+#ifndef _DT_BINDINGS_RESET_TI_OMAP5_H
+#define _DT_BINDINGS_RESET_TI_OMAP5_H
+
+#define RESET_DEVICE_RESET			0
+#define RESET_DSP_RESET				1
+#define RESET_DSP_MMU_CACHE_RESET	2
+#define RESET_IPU_CPU0_RESET		3
+#define RESET_IPU_CPU1_RESET		4
+#define RESET_IPU_MMU_CACHE_RESET	5
+#define RESET_IVA_RESET				6
+
+#endif
-- 
1.7.9.5

Re: [RFC 08/11] ARM: dts: am33xx: Add prcm_resets node

[Arnd Bergmann]

On Tuesday 29 April 2014 15:19:47 Dan Murphy wrote:
> + * AM33xx reset index for PRCM Module
> + *
> + * Copyright 2014 Texas Instruments Inc.
> + *
> + * 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.
> + */
> +
> +#ifndef _DT_BINDINGS_RESET_TI_AM33XX_H
> +#define _DT_BINDINGS_RESET_TI_AM33XX_H
> +
> +#define RESET_DEVICE_RESET                     0
> +#define RESET_GFX_RESET                                1
> +#define RESET_PER_RESET                                2
> +
> +#endif

Interfaces like this should only be used if you can't use hardware
numbers, in general. If these numbers are in the data sheet, just
put them directly into the dts file, as we do for interrupt numbers,
gpio numbers, register offsets etc.

If you have made them up to define an interface between the driver
and DT because there is no usable hardare ID, I'd suggest just using
a single file across all SoCs that have this driver, and have
a unified name space.

	Arnd

Re: [RFC 01/11] drivers: reset: TI: SoC reset controller support.

[Arnd Bergmann]

On Tuesday 29 April 2014 15:19:40 Dan Murphy wrote:
> +#ifndef _RESET_TI_H_
> +#define _RESET_TI_H_
> +
> +#ifdef CONFIG_RESET_TI
> +void ti_dt_reset_init(void);
> +#else
> +static inline void ti_dt_reset_init(void){ return; };
> +#endif

Why can't this be a regular platform device driver that gets
initialized through an initcall rather than get called from
platform code?

	Arnd

Re: [RFC 08/11] ARM: dts: am33xx: Add prcm_resets node

[Tony Lindgren]

* Arnd Bergmann <arnd@arndb.de> [140429 13:35]:
> On Tuesday 29 April 2014 15:19:47 Dan Murphy wrote:
> > + * AM33xx reset index for PRCM Module
> > + *
> > + * Copyright 2014 Texas Instruments Inc.
> > + *
> > + * 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.
> > + */
> > +
> > +#ifndef _DT_BINDINGS_RESET_TI_AM33XX_H
> > +#define _DT_BINDINGS_RESET_TI_AM33XX_H
> > +
> > +#define RESET_DEVICE_RESET                     0
> > +#define RESET_GFX_RESET                                1
> > +#define RESET_PER_RESET                                2
> > +
> > +#endif
> 
> Interfaces like this should only be used if you can't use hardware
> numbers, in general. If these numbers are in the data sheet, just
> put them directly into the dts file, as we do for interrupt numbers,
> gpio numbers, register offsets etc.
> 
> If you have made them up to define an interface between the driver
> and DT because there is no usable hardare ID, I'd suggest just using
> a single file across all SoCs that have this driver, and have
> a unified name space.

Also, it's a bit unclear how the reset controller phandle is used
referenced and used by the consumer device.. Maybe setting that up
first in a Linux generic way is a good point starting point.

Maybe something like this along the same way as clocks are set up
(completely untested):

&reset1 {
	iva_reset: reset1 {
		reg = /bits/ 8 <0>;
	};
	gfx_reset: reset1 {
		reg = /bits/ 8 <1>;
	};
	...
};

&iva {
	compatible = "ti,ivahd";
	resets = <&reset1 1>;
	...
};

Regards,

Tony

Re: [RFC 01/11] drivers: reset: TI: SoC reset controller support.

[Philipp Zabel]

Hi Dan,

Am Dienstag, den 29.04.2014, 15:19 -0500 schrieb Dan Murphy:
> The TI SoC reset controller support utilizes the
> reset controller framework to give device drivers or
> function drivers a common set of APIs to call to reset
> a module.
> 
> The reset-ti is a common interface to the reset framework.
>  The register data is retrieved during initialization
>  of the reset driver through the reset-ti-data
> file.  The array of data is associated with the compatible from the
> respective DT entry.
> 
> Once the data is available then this is derefenced within the common
> interface.
> 
> The device driver has the ability to assert, deassert or perform a
> complete reset.
> 
> This code was derived from previous work by Rajendra Nayak and Afzal Mohammed.
> The code was changed to adopt to the reset core and abstract away the SoC information.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
>  drivers/reset/Kconfig            |    1 +
>  drivers/reset/Makefile           |    1 +
>  drivers/reset/ti/Kconfig         |    8 ++
>  drivers/reset/ti/Makefile        |    1 +
>  drivers/reset/ti/reset-ti-data.h |   58 ++++++++++++
>  drivers/reset/ti/reset-ti.c      |  195 ++++++++++++++++++++++++++++++++++++++
>  include/linux/reset_ti.h         |   25 +++++
>  7 files changed, 289 insertions(+)
>  create mode 100644 drivers/reset/ti/Kconfig
>  create mode 100644 drivers/reset/ti/Makefile
>  create mode 100644 drivers/reset/ti/reset-ti-data.h
>  create mode 100644 drivers/reset/ti/reset-ti.c
>  create mode 100644 include/linux/reset_ti.h
> 
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 0615f50..a58d789 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -13,3 +13,4 @@ menuconfig RESET_CONTROLLER
>  	  If unsure, say no.
>  
>  source "drivers/reset/sti/Kconfig"
> +source "drivers/reset/ti/Kconfig"
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 4f60caf..1c8c444 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -1,3 +1,4 @@
>  obj-$(CONFIG_RESET_CONTROLLER) += core.o
>  obj-$(CONFIG_ARCH_SUNXI) += reset-sunxi.o
>  obj-$(CONFIG_ARCH_STI) += sti/
> +obj-$(CONFIG_RESET_TI) += ti/
> diff --git a/drivers/reset/ti/Kconfig b/drivers/reset/ti/Kconfig
> new file mode 100644
> index 0000000..dcdce90
> --- /dev/null
> +++ b/drivers/reset/ti/Kconfig
> @@ -0,0 +1,8 @@
> +config RESET_TI
> +	depends on RESET_CONTROLLER
> +	bool "TI reset controller"
> +	help
> +	  Reset controller support for TI SoC's
> +
> +	  Reset controller found in TI's AM series of SoC's like
> +	  AM335x and AM43x and OMAP SoC's like OMAP5 and DRA7
> diff --git a/drivers/reset/ti/Makefile b/drivers/reset/ti/Makefile
> new file mode 100644
> index 0000000..55ab3f5
> --- /dev/null
> +++ b/drivers/reset/ti/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_RESET_TI) += reset-ti.o
> diff --git a/drivers/reset/ti/reset-ti-data.h b/drivers/reset/ti/reset-ti-data.h
> new file mode 100644
> index 0000000..6afdf37
> --- /dev/null
> +++ b/drivers/reset/ti/reset-ti-data.h
> @@ -0,0 +1,58 @@
> +/*
> + * PRCM reset driver for TI SoC's
> + *
> + * Copyright 2014 Texas Instruments Inc.
> + *
> + * Author: Dan Murphy <dmurphy@ti.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.
> + */
> +
> +#ifndef _RESET_TI_DATA_H_
> +#define _RESET_TI_DATA_H_
> +
> +#include <linux/kernel.h>
> +#include <linux/of_device.h>
> +#include <linux/reset-controller.h>
> +
> +/**
> + * struct ti_reset_reg_data - Structure of the reset register information
> + * 	for a particular SoC.
> + * @rstctrl_offs: This is the reset control offset value from
> + *		from the parent reset node.
> + * @rstst_offs: This is the reset status offset value from
> + *		from the parent reset node.
> + * @rstctrl_bit: This is the reset control bit for the module.
> + * @rstst_bit: This is the reset status bit for the module.
> + *
> + * This structure describes the reset register control and status offsets.
> + * The bits are also defined for the same.
> + */
> +struct ti_reset_reg_data {
> +	u32	rstctrl_offs;
> +	u32	rstst_offs;
> +	u8	rstctrl_bit;
> +	u8	rstst_bit;

You are only ever using these as (1 << rstctrl_bit) and as
(1 << rstst_bit). You could store the mask here directly,
like the regulator framework does.

> +};
> +
> +/**
> + * struct ti_reset_data - Structure that contains the reset register data
> + *	as well as the total number of resets for a particular SoC.
> + * @reg_data:	Pointer to the register data structure.
> + * @nr_resets:	Total number of resets for the SoC in the reset array.
> + *
> + * This structure contains a pointer to the register data and the modules
> + * register base.  The number of resets and reset controller device data is
> + * stored within this structure.
> + * 

    trailing whitespace

> + */
> +struct ti_reset_data {
> +	struct ti_reset_reg_data *reg_data;
> +	struct reset_controller_dev rcdev;
> +	void __iomem *reg_base;
> +	u8	nr_resets;
> +};
> +
> +#endif
> diff --git a/drivers/reset/ti/reset-ti.c b/drivers/reset/ti/reset-ti.c
> new file mode 100644
> index 0000000..1d38069
> --- /dev/null
> +++ b/drivers/reset/ti/reset-ti.c
> @@ -0,0 +1,195 @@
> +/*
> + * PRCM reset driver for TI SoC's
> + *
> + * Copyright 2014 Texas Instruments Inc.
> + *
> + * Author: Dan Murphy <dmurphy@ti.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/device.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/slab.h>
> +
> +#include <linux/reset_ti.h>
> +#include <linux/reset.h>
> +
> +#include "reset-ti-data.h"
> +
> +static void ti_reset_wait_on_reset(struct reset_controller_dev *rcdev,
> +			       unsigned long id)
> +{
> +	struct ti_reset_data *reset_data = container_of(rcdev,
> +													struct ti_reset_data,
> +													rcdev);

Please consider taking a few steps to the left.

> +	void __iomem *status_reg;
> +	u32 val = 0;
> +	u8 status_bit = 0;
> +
> +	if (id < 0) {
> +		pr_err("%s: ID passed is invalid\n", __func__);
> +		return;
> +	}
> +
> +	/* Clear the reset status bit to reflect the current status */
> +	status_reg = reset_data->reg_base + reset_data->reg_data[id].rstst_offs;
> +	status_bit = reset_data->reg_data[id].rstst_bit;
> +	do {
> +		val = readl(status_reg);
> +		if (!(val & (1 << status_bit)))
> +			break;
> +	} while (1);

Is the status bit guaranteed to clear after a few cycles?

> +}
> +
> +static int ti_reset_assert(struct reset_controller_dev *rcdev,
> +			       unsigned long id)
> +{
> +	struct ti_reset_data *reset_data = container_of(rcdev,
> +													struct ti_reset_data,
> +													rcdev);

Very long lines again.

> +	void __iomem *reg;
> +	void __iomem *status_reg;
> +	u32 val = 0;
> +	u8 bit = 0;
> +	u8 status_bit = 0;
> +
> +	if (id < 0) {

The id parameter is _unsigned_ long.

> +		pr_err("%s: ID passed is invalid\n", __func__);
> +		return -ENODEV;
> +	}
> +
> +	/* Clear the reset status bit to reflect the current status */
> +	status_reg = reset_data->reg_base + reset_data->reg_data[id].rstst_offs;
> +	status_bit = reset_data->reg_data[id].rstst_bit;
> +	writel(1 << status_bit, status_reg);

See the first comment, all the left shifts could be done by the compiler
if you store the bit mask instead of the bit offset.

> +	reg = reset_data->reg_base + reset_data->reg_data[id].rstctrl_offs;
> +	bit = reset_data->reg_data[id].rstctrl_bit;
> +	val = readl(reg);
> +	if (!(val & (1 << bit))) {
> +		val |= (1 << bit);
> +		writel(val, reg);
> +	}
> +
> +	return 0;
> +}
> +
> +static int ti_reset_deassert(struct reset_controller_dev *rcdev,
> +			       unsigned long id)
> +{
> +
> +	struct ti_reset_data *reset_data;
> +	void __iomem *reg;
> +	void __iomem *status_reg;
> +	u32 val = 0;
> +	u8 bit = 0;
> +	u8 status_bit = 0;
> +
> +	if (id < 0) {

Again, unsigned.

> +		pr_err("%s: reset ID passed is invalid\n", __func__);
> +		return -ENODEV;
> +	}
> +
> +	reset_data = container_of(rcdev, struct ti_reset_data, rcdev);
> +
> +	/* Clear the reset status bit to reflect the current status */
> +	status_reg = reset_data->reg_base + reset_data->reg_data[id].rstst_offs;
> +	status_bit = reset_data->reg_data[id].rstst_bit;
> +	writel(1 << status_bit, status_reg);
> +
> +	reg = reset_data->reg_base + reset_data->reg_data[id].rstctrl_offs;
> +	bit = reset_data->reg_data[id].rstctrl_bit;
> +	val = readl(reg);
> +	if (val & (1 << bit)) {
> +		val &= ~(1 << bit);
> +		writel(val, reg);
> +	}
> +
> +	return 0;
> +}
> +
> +static int ti_reset_reset(struct reset_controller_dev *rcdev,
> +				  unsigned long id)
> +{
> +	ti_reset_assert(rcdev, id);
> +	ti_reset_deassert(rcdev, id);
> +	ti_reset_wait_on_reset(rcdev, id);
> +
> +	return 0;
> +}
> +
> +static struct reset_control_ops ti_reset_ops = {
> +	.reset = ti_reset_reset,
> +	.assert = ti_reset_assert,
> +	.deassert = ti_reset_deassert,
> +};
> +
> +static const struct of_device_id ti_reset_of_match[] = {
> +	{},
> +};
> +
> +const struct of_device_id *ti_reset_get_data(struct device_node *parent)
> +{
> +	const struct of_device_id *dev_node;
> +
> +	dev_node = of_match_node(ti_reset_of_match, parent);
> +	if (!dev_node) {
> +		pr_err("%s: No compatible for resets for %s\n",
> +			__func__, parent->name);
> +		return NULL;
> +	}
> +
> +	return dev_node;
> +}
> +
> +void __init ti_dt_reset_init(void)

Is there a reason not to just register this as a platform device?

> +{
> +	struct ti_reset_data *ti_data;
> +	struct device_node *parent;
> +	struct device_node *resets;
> +	const struct of_device_id *dev_node;
> +
> +	resets = of_find_node_by_name(NULL, "resets");
> +	if (!resets) {
> +		pr_err("%s: missing 'resets' child node.\n", __func__);
> +		return;
> +	}
> +
> +	parent = of_get_parent(resets);
> +	if (!parent) {
> +		pr_err("%s: Cannot find parent reset node\n", __func__);
> +		return;
> +	}
> +
> +	ti_data = kzalloc(sizeof(*ti_data), GFP_KERNEL);
> +	if (!ti_data)
> +		return;
> +
> +	dev_node = ti_reset_get_data(resets);
> +	if (!dev_node) {
> +		pr_err("%s: Cannot find data for node\n", __func__);
> +		return;
> +	}
> +
> +	ti_data = (struct ti_reset_data *) dev_node->data;
> +
> +	ti_data->reg_base = of_iomap(parent, 0);
> +	if (!ti_data->reg_base) {
> +		pr_err("%s: Cannot map reset parent.\n", __func__);
> +		return;
> +	}
> +
> +	ti_data->rcdev.owner = THIS_MODULE;
> +	ti_data->rcdev.nr_resets = ti_data->nr_resets;
> +	ti_data->rcdev.of_node = resets;
> +	ti_data->rcdev.ops = &ti_reset_ops;
> +
> +	reset_controller_register(&ti_data->rcdev);
> +}
> diff --git a/include/linux/reset_ti.h b/include/linux/reset_ti.h
> new file mode 100644
> index 0000000..d18f47f
> --- /dev/null
> +++ b/include/linux/reset_ti.h
> @@ -0,0 +1,25 @@
> +/*
> + * TI reset driver support
> + *
> + * Copyright (C) 2014 Texas Instruments, Inc.
> + *
> + * 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 "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef _RESET_TI_H_
> +#define _RESET_TI_H_
> +
> +#ifdef CONFIG_RESET_TI
> +void ti_dt_reset_init(void);
> +#else
> +static inline void ti_dt_reset_init(void){ return; };
> +#endif
> +
> +#endif

regards
Philipp

Re: [RFC 01/11] drivers: reset: TI: SoC reset controller support.

[Dan Murphy]

Philipp and Arnd

Thank you for the comments

On 04/30/2014 03:20 AM, Philipp Zabel wrote:
> Hi Dan,
>
> Am Dienstag, den 29.04.2014, 15:19 -0500 schrieb Dan Murphy:
>> The TI SoC reset controller support utilizes the
>> reset controller framework to give device drivers or
>> function drivers a common set of APIs to call to reset
>> a module.
>>
>> The reset-ti is a common interface to the reset framework.
>>  The register data is retrieved during initialization
>>  of the reset driver through the reset-ti-data
>> file.  The array of data is associated with the compatible from the
>> respective DT entry.
>>
>> Once the data is available then this is derefenced within the common
>> interface.
>>
>> The device driver has the ability to assert, deassert or perform a
>> complete reset.
>>
>> This code was derived from previous work by Rajendra Nayak and Afzal Mohammed.
>> The code was changed to adopt to the reset core and abstract away the SoC information.
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>>  drivers/reset/Kconfig            |    1 +
>>  drivers/reset/Makefile           |    1 +
>>  drivers/reset/ti/Kconfig         |    8 ++
>>  drivers/reset/ti/Makefile        |    1 +
>>  drivers/reset/ti/reset-ti-data.h |   58 ++++++++++++
>>  drivers/reset/ti/reset-ti.c      |  195 ++++++++++++++++++++++++++++++++++++++
>>  include/linux/reset_ti.h         |   25 +++++
>>  7 files changed, 289 insertions(+)
>>  create mode 100644 drivers/reset/ti/Kconfig
>>  create mode 100644 drivers/reset/ti/Makefile
>>  create mode 100644 drivers/reset/ti/reset-ti-data.h
>>  create mode 100644 drivers/reset/ti/reset-ti.c
>>  create mode 100644 include/linux/reset_ti.h
>>
>> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
>> index 0615f50..a58d789 100644
>> --- a/drivers/reset/Kconfig
>> +++ b/drivers/reset/Kconfig
>> @@ -13,3 +13,4 @@ menuconfig RESET_CONTROLLER
>>  	  If unsure, say no.
>>  
>>  source "drivers/reset/sti/Kconfig"
>> +source "drivers/reset/ti/Kconfig"
>> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
>> index 4f60caf..1c8c444 100644
>> --- a/drivers/reset/Makefile
>> +++ b/drivers/reset/Makefile
>> @@ -1,3 +1,4 @@
>>  obj-$(CONFIG_RESET_CONTROLLER) += core.o
>>  obj-$(CONFIG_ARCH_SUNXI) += reset-sunxi.o
>>  obj-$(CONFIG_ARCH_STI) += sti/
>> +obj-$(CONFIG_RESET_TI) += ti/
>> diff --git a/drivers/reset/ti/Kconfig b/drivers/reset/ti/Kconfig
>> new file mode 100644
>> index 0000000..dcdce90
>> --- /dev/null
>> +++ b/drivers/reset/ti/Kconfig
>> @@ -0,0 +1,8 @@
>> +config RESET_TI
>> +	depends on RESET_CONTROLLER
>> +	bool "TI reset controller"
>> +	help
>> +	  Reset controller support for TI SoC's
>> +
>> +	  Reset controller found in TI's AM series of SoC's like
>> +	  AM335x and AM43x and OMAP SoC's like OMAP5 and DRA7
>> diff --git a/drivers/reset/ti/Makefile b/drivers/reset/ti/Makefile
>> new file mode 100644
>> index 0000000..55ab3f5
>> --- /dev/null
>> +++ b/drivers/reset/ti/Makefile
>> @@ -0,0 +1 @@
>> +obj-$(CONFIG_RESET_TI) += reset-ti.o
>> diff --git a/drivers/reset/ti/reset-ti-data.h b/drivers/reset/ti/reset-ti-data.h
>> new file mode 100644
>> index 0000000..6afdf37
>> --- /dev/null
>> +++ b/drivers/reset/ti/reset-ti-data.h
>> @@ -0,0 +1,58 @@
>> +/*
>> + * PRCM reset driver for TI SoC's
>> + *
>> + * Copyright 2014 Texas Instruments Inc.
>> + *
>> + * Author: Dan Murphy <dmurphy@ti.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.
>> + */
>> +
>> +#ifndef _RESET_TI_DATA_H_
>> +#define _RESET_TI_DATA_H_
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/of_device.h>
>> +#include <linux/reset-controller.h>
>> +
>> +/**
>> + * struct ti_reset_reg_data - Structure of the reset register information
>> + * 	for a particular SoC.
>> + * @rstctrl_offs: This is the reset control offset value from
>> + *		from the parent reset node.
>> + * @rstst_offs: This is the reset status offset value from
>> + *		from the parent reset node.
>> + * @rstctrl_bit: This is the reset control bit for the module.
>> + * @rstst_bit: This is the reset status bit for the module.
>> + *
>> + * This structure describes the reset register control and status offsets.
>> + * The bits are also defined for the same.
>> + */
>> +struct ti_reset_reg_data {
>> +	u32	rstctrl_offs;
>> +	u32	rstst_offs;
>> +	u8	rstctrl_bit;
>> +	u8	rstst_bit;
> You are only ever using these as (1 << rstctrl_bit) and as
> (1 << rstst_bit). You could store the mask here directly,
> like the regulator framework does.
>

Yes we can store the mask as opposed to the bit.
I will look into it for v2.

>> +};
>> +
>> +/**
>> + * struct ti_reset_data - Structure that contains the reset register data
>> + *	as well as the total number of resets for a particular SoC.
>> + * @reg_data:	Pointer to the register data structure.
>> + * @nr_resets:	Total number of resets for the SoC in the reset array.
>> + *
>> + * This structure contains a pointer to the register data and the modules
>> + * register base.  The number of resets and reset controller device data is
>> + * stored within this structure.
>> + * 
>     trailing whitespace

Got it will fix v2.

>> + */
>> +struct ti_reset_data {
>> +	struct ti_reset_reg_data *reg_data;
>> +	struct reset_controller_dev rcdev;
>> +	void __iomem *reg_base;
>> +	u8	nr_resets;
>> +};
>> +
>> +#endif
>> diff --git a/drivers/reset/ti/reset-ti.c b/drivers/reset/ti/reset-ti.c
>> new file mode 100644
>> index 0000000..1d38069
>> --- /dev/null
>> +++ b/drivers/reset/ti/reset-ti.c
>> @@ -0,0 +1,195 @@
>> +/*
>> + * PRCM reset driver for TI SoC's
>> + *
>> + * Copyright 2014 Texas Instruments Inc.
>> + *
>> + * Author: Dan Murphy <dmurphy@ti.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/device.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_device.h>
>> +#include <linux/slab.h>
>> +
>> +#include <linux/reset_ti.h>
>> +#include <linux/reset.h>
>> +
>> +#include "reset-ti-data.h"
>> +
>> +static void ti_reset_wait_on_reset(struct reset_controller_dev *rcdev,
>> +			       unsigned long id)
>> +{
>> +	struct ti_reset_data *reset_data = container_of(rcdev,
>> +													struct ti_reset_data,
>> +													rcdev);
> Please consider taking a few steps to the left.

Got it will fix v2.

>
>> +	void __iomem *status_reg;
>> +	u32 val = 0;
>> +	u8 status_bit = 0;
>> +
>> +	if (id < 0) {
>> +		pr_err("%s: ID passed is invalid\n", __func__);
>> +		return;
>> +	}
>> +
>> +	/* Clear the reset status bit to reflect the current status */
>> +	status_reg = reset_data->reg_base + reset_data->reg_data[id].rstst_offs;
>> +	status_bit = reset_data->reg_data[id].rstst_bit;
>> +	do {
>> +		val = readl(status_reg);
>> +		if (!(val & (1 << status_bit)))
>> +			break;
>> +	} while (1);
> Is the status bit guaranteed to clear after a few cycles?

I will look into this question

>> +}
>> +
>> +static int ti_reset_assert(struct reset_controller_dev *rcdev,
>> +			       unsigned long id)
>> +{
>> +	struct ti_reset_data *reset_data = container_of(rcdev,
>> +													struct ti_reset_data,
>> +													rcdev);
> Very long lines again.

Yep.  Will fix v2 copy/paste problem

>
>> +	void __iomem *reg;
>> +	void __iomem *status_reg;
>> +	u32 val = 0;
>> +	u8 bit = 0;
>> +	u8 status_bit = 0;
>> +
>> +	if (id < 0) {
> The id parameter is _unsigned_ long.

OK got it.

>
>> +		pr_err("%s: ID passed is invalid\n", __func__);
>> +		return -ENODEV;
>> +	}
>> +
>> +	/* Clear the reset status bit to reflect the current status */
>> +	status_reg = reset_data->reg_base + reset_data->reg_data[id].rstst_offs;
>> +	status_bit = reset_data->reg_data[id].rstst_bit;
>> +	writel(1 << status_bit, status_reg);
> See the first comment, all the left shifts could be done by the compiler
> if you store the bit mask instead of the bit offset.

Per my reply I will look into this

>
>> +	reg = reset_data->reg_base + reset_data->reg_data[id].rstctrl_offs;
>> +	bit = reset_data->reg_data[id].rstctrl_bit;
>> +	val = readl(reg);
>> +	if (!(val & (1 << bit))) {
>> +		val |= (1 << bit);
>> +		writel(val, reg);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int ti_reset_deassert(struct reset_controller_dev *rcdev,
>> +			       unsigned long id)
>> +{
>> +
>> +	struct ti_reset_data *reset_data;
>> +	void __iomem *reg;
>> +	void __iomem *status_reg;
>> +	u32 val = 0;
>> +	u8 bit = 0;
>> +	u8 status_bit = 0;
>> +
>> +	if (id < 0) {
> Again, unsigned.
>
>> +		pr_err("%s: reset ID passed is invalid\n", __func__);
>> +		return -ENODEV;
>> +	}
>> +
>> +	reset_data = container_of(rcdev, struct ti_reset_data, rcdev);
>> +
>> +	/* Clear the reset status bit to reflect the current status */
>> +	status_reg = reset_data->reg_base + reset_data->reg_data[id].rstst_offs;
>> +	status_bit = reset_data->reg_data[id].rstst_bit;
>> +	writel(1 << status_bit, status_reg);
>> +
>> +	reg = reset_data->reg_base + reset_data->reg_data[id].rstctrl_offs;
>> +	bit = reset_data->reg_data[id].rstctrl_bit;
>> +	val = readl(reg);
>> +	if (val & (1 << bit)) {
>> +		val &= ~(1 << bit);
>> +		writel(val, reg);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int ti_reset_reset(struct reset_controller_dev *rcdev,
>> +				  unsigned long id)
>> +{
>> +	ti_reset_assert(rcdev, id);
>> +	ti_reset_deassert(rcdev, id);
>> +	ti_reset_wait_on_reset(rcdev, id);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct reset_control_ops ti_reset_ops = {
>> +	.reset = ti_reset_reset,
>> +	.assert = ti_reset_assert,
>> +	.deassert = ti_reset_deassert,
>> +};
>> +
>> +static const struct of_device_id ti_reset_of_match[] = {
>> +	{},
>> +};
>> +
>> +const struct of_device_id *ti_reset_get_data(struct device_node *parent)
>> +{
>> +	const struct of_device_id *dev_node;
>> +
>> +	dev_node = of_match_node(ti_reset_of_match, parent);
>> +	if (!dev_node) {
>> +		pr_err("%s: No compatible for resets for %s\n",
>> +			__func__, parent->name);
>> +		return NULL;
>> +	}
>> +
>> +	return dev_node;
>> +}
>> +
>> +void __init ti_dt_reset_init(void)
> Is there a reason not to just register this as a platform device?

To answer this comment as well as Arnd's.  To be honest this was a remnant of my development.
You are both correct and this can be called during the init as a platform driver.

I will fix this in v2.  Which will in turn remove one patch as well as a header file.

Dan

<snip>


-- 
------------------
Dan Murphy

Re: [RFC 08/11] ARM: dts: am33xx: Add prcm_resets node

[Dan Murphy]

Tony and Arnd

Thanks for the comments

On 04/29/2014 07:22 PM, Tony Lindgren wrote:
> * Arnd Bergmann <arnd@arndb.de> [140429 13:35]:
>> On Tuesday 29 April 2014 15:19:47 Dan Murphy wrote:
>>> + * AM33xx reset index for PRCM Module
>>> + *
>>> + * Copyright 2014 Texas Instruments Inc.
>>> + *
>>> + * 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.
>>> + */
>>> +
>>> +#ifndef _DT_BINDINGS_RESET_TI_AM33XX_H
>>> +#define _DT_BINDINGS_RESET_TI_AM33XX_H
>>> +
>>> +#define RESET_DEVICE_RESET                     0
>>> +#define RESET_GFX_RESET                                1
>>> +#define RESET_PER_RESET                                2
>>> +
>>> +#endif
>> Interfaces like this should only be used if you can't use hardware
>> numbers, in general. If these numbers are in the data sheet, just
>> put them directly into the dts file, as we do for interrupt numbers,
>> gpio numbers, register offsets etc.
>>
>> If you have made them up to define an interface between the driver
>> and DT because there is no usable hardare ID, I'd suggest just using
>> a single file across all SoCs that have this driver, and have
>> a unified name space.
> Also, it's a bit unclear how the reset controller phandle is used
> referenced and used by the consumer device.. Maybe setting that up
> first in a Linux generic way is a good point starting point.
>
> Maybe something like this along the same way as clocks are set up
> (completely untested):
>
> &reset1 {
> 	iva_reset: reset1 {
> 		reg = /bits/ 8 <0>;
> 	};
> 	gfx_reset: reset1 {
> 		reg = /bits/ 8 <1>;
> 	};
> 	...
> };
>
> &iva {
> 	compatible = "ti,ivahd";
> 	resets = <&reset1 1>;
> 	...
> };

I had something very similar to this when I was developing this driver but moved away from this.

Following the clocks implementation I had a separate dtsi for resets for each device and had the data defined like so
for each SoC.

&prcm_resets {
       device_reset: device_reset {
               rstctrl_offs = <0x1104>;
               ctrl_bit-shift = <0>;
               rstst_offs      = <0x1114>;
               sts_bit-shift   = <0>;
       };

       gpu_reset: gpu_reset {
               rstctrl_offs = <0x0D00>;
               ctrl_bit-shift = <3>;
               rstst_offs      = <0x0D0C>;
               sts_bit-shift = <5>;
       };
};

And then any client interested in a specific reset driver would include this

resets = <&prcm_resets &gpu_reset>;
reset-names = "gpu_reset";

Our reset code would then retrieve the register data through the phandle instead of an index.

Thoughts?

Dan

> Regards,
>
> Tony


-- 
------------------
Dan Murphy

Re: [RFC 08/11] ARM: dts: am33xx: Add prcm_resets node

[Tony Lindgren]

* Dan Murphy <dmurphy@ti.com> [140430 11:00]:
> Tony and Arnd
> 
> Thanks for the comments
> 
> On 04/29/2014 07:22 PM, Tony Lindgren wrote:
> > * Arnd Bergmann <arnd@arndb.de> [140429 13:35]:
> >> On Tuesday 29 April 2014 15:19:47 Dan Murphy wrote:
> >>> + * AM33xx reset index for PRCM Module
> >>> + *
> >>> + * Copyright 2014 Texas Instruments Inc.
> >>> + *
> >>> + * 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.
> >>> + */
> >>> +
> >>> +#ifndef _DT_BINDINGS_RESET_TI_AM33XX_H
> >>> +#define _DT_BINDINGS_RESET_TI_AM33XX_H
> >>> +
> >>> +#define RESET_DEVICE_RESET                     0
> >>> +#define RESET_GFX_RESET                                1
> >>> +#define RESET_PER_RESET                                2
> >>> +
> >>> +#endif
> >> Interfaces like this should only be used if you can't use hardware
> >> numbers, in general. If these numbers are in the data sheet, just
> >> put them directly into the dts file, as we do for interrupt numbers,
> >> gpio numbers, register offsets etc.
> >>
> >> If you have made them up to define an interface between the driver
> >> and DT because there is no usable hardare ID, I'd suggest just using
> >> a single file across all SoCs that have this driver, and have
> >> a unified name space.
> > Also, it's a bit unclear how the reset controller phandle is used
> > referenced and used by the consumer device.. Maybe setting that up
> > first in a Linux generic way is a good point starting point.
> >
> > Maybe something like this along the same way as clocks are set up
> > (completely untested):
> >
> > &reset1 {
> > 	iva_reset: reset1 {
> > 		reg = /bits/ 8 <0>;
> > 	};
> > 	gfx_reset: reset1 {
> > 		reg = /bits/ 8 <1>;
> > 	};
> > 	...
> > };
> >
> > &iva {
> > 	compatible = "ti,ivahd";
> > 	resets = <&reset1 1>;
> > 	...
> > };
> 
> I had something very similar to this when I was developing this driver but moved away from this.
> 
> Following the clocks implementation I had a separate dtsi for resets for each device and had the data defined like so
> for each SoC.
> 
> &prcm_resets {
>        device_reset: device_reset {
>                rstctrl_offs = <0x1104>;
>                ctrl_bit-shift = <0>;
>                rstst_offs      = <0x1114>;
>                sts_bit-shift   = <0>;
>        };
> 
>        gpu_reset: gpu_reset {
>                rstctrl_offs = <0x0D00>;
>                ctrl_bit-shift = <3>;
>                rstst_offs      = <0x0D0C>;
>                sts_bit-shift = <5>;
>        };
> };
> 
> And then any client interested in a specific reset driver would include this
> 
> resets = <&prcm_resets &gpu_reset>;
> reset-names = "gpu_reset";
> 
> Our reset code would then retrieve the register data through the phandle instead of an index.
> 
> Thoughts?

Using phandles makes sense here because it avoids the indexing. Indexing
has a problem of needing to be in sync with the .dts files and the
device driver(s). Using an index also easily causes misuse of virtual
indexes being added that no longer describe the hardware at all.

Regards,

Tony

Re: [RFC 08/11] ARM: dts: am33xx: Add prcm_resets node

[Dan Murphy]

Tony

On 04/30/2014 01:10 PM, Tony Lindgren wrote:
> * Dan Murphy <dmurphy@ti.com> [140430 11:00]:
>> Tony and Arnd
>>
>> Thanks for the comments
>>
>> On 04/29/2014 07:22 PM, Tony Lindgren wrote:
>>> * Arnd Bergmann <arnd@arndb.de> [140429 13:35]:
>>>> On Tuesday 29 April 2014 15:19:47 Dan Murphy wrote:
>>>>> + * AM33xx reset index for PRCM Module
>>>>> + *
>>>>> + * Copyright 2014 Texas Instruments Inc.
>>>>> + *
>>>>> + * 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.
>>>>> + */
>>>>> +
>>>>> +#ifndef _DT_BINDINGS_RESET_TI_AM33XX_H
>>>>> +#define _DT_BINDINGS_RESET_TI_AM33XX_H
>>>>> +
>>>>> +#define RESET_DEVICE_RESET                     0
>>>>> +#define RESET_GFX_RESET                                1
>>>>> +#define RESET_PER_RESET                                2
>>>>> +
>>>>> +#endif
>>>> Interfaces like this should only be used if you can't use hardware
>>>> numbers, in general. If these numbers are in the data sheet, just
>>>> put them directly into the dts file, as we do for interrupt numbers,
>>>> gpio numbers, register offsets etc.
>>>>
>>>> If you have made them up to define an interface between the driver
>>>> and DT because there is no usable hardare ID, I'd suggest just using
>>>> a single file across all SoCs that have this driver, and have
>>>> a unified name space.
>>> Also, it's a bit unclear how the reset controller phandle is used
>>> referenced and used by the consumer device.. Maybe setting that up
>>> first in a Linux generic way is a good point starting point.
>>>
>>> Maybe something like this along the same way as clocks are set up
>>> (completely untested):
>>>
>>> &reset1 {
>>> 	iva_reset: reset1 {
>>> 		reg = /bits/ 8 <0>;
>>> 	};
>>> 	gfx_reset: reset1 {
>>> 		reg = /bits/ 8 <1>;
>>> 	};
>>> 	...
>>> };
>>>
>>> &iva {
>>> 	compatible = "ti,ivahd";
>>> 	resets = <&reset1 1>;
>>> 	...
>>> };
>> I had something very similar to this when I was developing this driver but moved away from this.
>>
>> Following the clocks implementation I had a separate dtsi for resets for each device and had the data defined like so
>> for each SoC.
>>
>> &prcm_resets {
>>        device_reset: device_reset {
>>                rstctrl_offs = <0x1104>;
>>                ctrl_bit-shift = <0>;
>>                rstst_offs      = <0x1114>;
>>                sts_bit-shift   = <0>;
>>        };
>>
>>        gpu_reset: gpu_reset {
>>                rstctrl_offs = <0x0D00>;
>>                ctrl_bit-shift = <3>;
>>                rstst_offs      = <0x0D0C>;
>>                sts_bit-shift = <5>;
>>        };
>> };
>>
>> And then any client interested in a specific reset driver would include this
>>
>> resets = <&prcm_resets &gpu_reset>;
>> reset-names = "gpu_reset";
>>
>> Our reset code would then retrieve the register data through the phandle instead of an index.
>>
>> Thoughts?
> Using phandles makes sense here because it avoids the indexing. Indexing
> has a problem of needing to be in sync with the .dts files and the
> device driver(s). Using an index also easily causes misuse of virtual
> indexes being added that no longer describe the hardware at all.

Thanks.  What about placing register data in the dts files?  Is there any issue with this concept?

Dan

> Regards,
>
> Tony


-- 
------------------
Dan Murphy

Re: [RFC 08/11] ARM: dts: am33xx: Add prcm_resets node

[Tony Lindgren]

* Dan Murphy <dmurphy@ti.com> [140430 11:14]:
> On 04/30/2014 01:10 PM, Tony Lindgren wrote:
> > * Dan Murphy <dmurphy@ti.com> [140430 11:00]:
> >> Tony and Arnd
> >>
> >> Thanks for the comments
> >>
> >> On 04/29/2014 07:22 PM, Tony Lindgren wrote:
> >>> * Arnd Bergmann <arnd@arndb.de> [140429 13:35]:
> >>>> On Tuesday 29 April 2014 15:19:47 Dan Murphy wrote:
> >>>>> + * AM33xx reset index for PRCM Module
> >>>>> + *
> >>>>> + * Copyright 2014 Texas Instruments Inc.
> >>>>> + *
> >>>>> + * 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.
> >>>>> + */
> >>>>> +
> >>>>> +#ifndef _DT_BINDINGS_RESET_TI_AM33XX_H
> >>>>> +#define _DT_BINDINGS_RESET_TI_AM33XX_H
> >>>>> +
> >>>>> +#define RESET_DEVICE_RESET                     0
> >>>>> +#define RESET_GFX_RESET                                1
> >>>>> +#define RESET_PER_RESET                                2
> >>>>> +
> >>>>> +#endif
> >>>> Interfaces like this should only be used if you can't use hardware
> >>>> numbers, in general. If these numbers are in the data sheet, just
> >>>> put them directly into the dts file, as we do for interrupt numbers,
> >>>> gpio numbers, register offsets etc.
> >>>>
> >>>> If you have made them up to define an interface between the driver
> >>>> and DT because there is no usable hardare ID, I'd suggest just using
> >>>> a single file across all SoCs that have this driver, and have
> >>>> a unified name space.
> >>> Also, it's a bit unclear how the reset controller phandle is used
> >>> referenced and used by the consumer device.. Maybe setting that up
> >>> first in a Linux generic way is a good point starting point.
> >>>
> >>> Maybe something like this along the same way as clocks are set up
> >>> (completely untested):
> >>>
> >>> &reset1 {
> >>> 	iva_reset: reset1 {
> >>> 		reg = /bits/ 8 <0>;
> >>> 	};
> >>> 	gfx_reset: reset1 {
> >>> 		reg = /bits/ 8 <1>;
> >>> 	};
> >>> 	...
> >>> };
> >>>
> >>> &iva {
> >>> 	compatible = "ti,ivahd";
> >>> 	resets = <&reset1 1>;
> >>> 	...
> >>> };
> >> I had something very similar to this when I was developing this driver but moved away from this.
> >>
> >> Following the clocks implementation I had a separate dtsi for resets for each device and had the data defined like so
> >> for each SoC.
> >>
> >> &prcm_resets {
> >>        device_reset: device_reset {
> >>                rstctrl_offs = <0x1104>;
> >>                ctrl_bit-shift = <0>;
> >>                rstst_offs      = <0x1114>;
> >>                sts_bit-shift   = <0>;
> >>        };
> >>
> >>        gpu_reset: gpu_reset {
> >>                rstctrl_offs = <0x0D00>;
> >>                ctrl_bit-shift = <3>;
> >>                rstst_offs      = <0x0D0C>;
> >>                sts_bit-shift = <5>;
> >>        };
> >> };
> >>
> >> And then any client interested in a specific reset driver would include this
> >>
> >> resets = <&prcm_resets &gpu_reset>;
> >> reset-names = "gpu_reset";
> >>
> >> Our reset code would then retrieve the register data through the phandle instead of an index.
> >>
> >> Thoughts?
> > Using phandles makes sense here because it avoids the indexing. Indexing
> > has a problem of needing to be in sync with the .dts files and the
> > device driver(s). Using an index also easily causes misuse of virtual
> > indexes being added that no longer describe the hardware at all.
> 
> Thanks.  What about placing register data in the dts files?  Is there any issue with this concept?

I don't have issues with that but others may. In this case it seems
like you should get away just defining few different types of reset
controllers without adding any any custom properties?

In your example above, the rstctrl_offs should be just standard
reg entry as an offset from the prcm_resets base address. Then you
you only really need the bit shift and the type of the reset
controller? And the type could be the compatible flag?

Regards,

Tony

Re: [RFC 08/11] ARM: dts: am33xx: Add prcm_resets node

[Dan Murphy]

Tony

On 04/30/2014 05:33 PM, Tony Lindgren wrote:
> * Dan Murphy <dmurphy@ti.com> [140430 11:14]:
>> On 04/30/2014 01:10 PM, Tony Lindgren wrote:
>>> * Dan Murphy <dmurphy@ti.com> [140430 11:00]:
>>>> Tony and Arnd
>>>>
>>>> Thanks for the comments
>>>>
>>>> On 04/29/2014 07:22 PM, Tony Lindgren wrote:
>>>>> * Arnd Bergmann <arnd@arndb.de> [140429 13:35]:
>>>>>> On Tuesday 29 April 2014 15:19:47 Dan Murphy wrote:
>>>>>>> + * AM33xx reset index for PRCM Module
>>>>>>> + *
>>>>>>> + * Copyright 2014 Texas Instruments Inc.
>>>>>>> + *
>>>>>>> + * 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.
>>>>>>> + */
>>>>>>> +
>>>>>>> +#ifndef _DT_BINDINGS_RESET_TI_AM33XX_H
>>>>>>> +#define _DT_BINDINGS_RESET_TI_AM33XX_H
>>>>>>> +
>>>>>>> +#define RESET_DEVICE_RESET                     0
>>>>>>> +#define RESET_GFX_RESET                                1
>>>>>>> +#define RESET_PER_RESET                                2
>>>>>>> +
>>>>>>> +#endif
>>>>>> Interfaces like this should only be used if you can't use hardware
>>>>>> numbers, in general. If these numbers are in the data sheet, just
>>>>>> put them directly into the dts file, as we do for interrupt numbers,
>>>>>> gpio numbers, register offsets etc.
>>>>>>
>>>>>> If you have made them up to define an interface between the driver
>>>>>> and DT because there is no usable hardare ID, I'd suggest just using
>>>>>> a single file across all SoCs that have this driver, and have
>>>>>> a unified name space.
>>>>> Also, it's a bit unclear how the reset controller phandle is used
>>>>> referenced and used by the consumer device.. Maybe setting that up
>>>>> first in a Linux generic way is a good point starting point.
>>>>>
>>>>> Maybe something like this along the same way as clocks are set up
>>>>> (completely untested):
>>>>>
>>>>> &reset1 {
>>>>> 	iva_reset: reset1 {
>>>>> 		reg = /bits/ 8 <0>;
>>>>> 	};
>>>>> 	gfx_reset: reset1 {
>>>>> 		reg = /bits/ 8 <1>;
>>>>> 	};
>>>>> 	...
>>>>> };
>>>>>
>>>>> &iva {
>>>>> 	compatible = "ti,ivahd";
>>>>> 	resets = <&reset1 1>;
>>>>> 	...
>>>>> };
>>>> I had something very similar to this when I was developing this driver but moved away from this.
>>>>
>>>> Following the clocks implementation I had a separate dtsi for resets for each device and had the data defined like so
>>>> for each SoC.
>>>>
>>>> &prcm_resets {
>>>>        device_reset: device_reset {
>>>>                rstctrl_offs = <0x1104>;
>>>>                ctrl_bit-shift = <0>;
>>>>                rstst_offs      = <0x1114>;
>>>>                sts_bit-shift   = <0>;
>>>>        };
>>>>
>>>>        gpu_reset: gpu_reset {
>>>>                rstctrl_offs = <0x0D00>;
>>>>                ctrl_bit-shift = <3>;
>>>>                rstst_offs      = <0x0D0C>;
>>>>                sts_bit-shift = <5>;
>>>>        };
>>>> };
>>>>
>>>> And then any client interested in a specific reset driver would include this
>>>>
>>>> resets = <&prcm_resets &gpu_reset>;
>>>> reset-names = "gpu_reset";
>>>>
>>>> Our reset code would then retrieve the register data through the phandle instead of an index.
>>>>
>>>> Thoughts?
>>> Using phandles makes sense here because it avoids the indexing. Indexing
>>> has a problem of needing to be in sync with the .dts files and the
>>> device driver(s). Using an index also easily causes misuse of virtual
>>> indexes being added that no longer describe the hardware at all.
>> Thanks.  What about placing register data in the dts files?  Is there any issue with this concept?
> I don't have issues with that but others may. In this case it seems
> like you should get away just defining few different types of reset
> controllers without adding any any custom properties?
>
> In your example above, the rstctrl_offs should be just standard
> reg entry as an offset from the prcm_resets base address. Then you
> you only really need the bit shift and the type of the reset
> controller? And the type could be the compatible flag?

Well the only issue I have with declaring the reg with the standard dt entry
is I have two register offsets here.

I will have to see if I can do this per the standard.
I may have to have children called control and status and define the reg and bits that way.

Dan

> Regards,
>
> Tony
>


-- 
------------------
Dan Murphy