[RFC v2] TI Reset Driver adapted to the reset core

[RFC v2] TI Reset Driver adapted to the reset core

[Dan Murphy]

All

I have made some big changes to this patchset so I did not put all the
changes in the patches themselves.

In brief

- I removed the object data files for each SoC and moved the data into
the DT.  I updated the binding document as well

- The DT implementation creates a parent reset node with the base offset
for each reset and the children within that parent declares the bit mask

- I created a xlate function in the reset driver to verify that reset
data and that the phandle exists.  The core xlate checks for the nr_resets
which we don't care about since we are not indexed based.

- Made the driver a platform driver as well so this removed the
prm_common patch as well as the header file.

I will add other TI devices once I feel comfortable that the design
is acceptable.

Dan

[RFC] [v2 Patch 1/6] 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 |   56 ++++++++
 drivers/reset/ti/reset-ti.c      |  267 ++++++++++++++++++++++++++++++++++++++
 6 files changed, 334 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

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..4d2a6d5
--- /dev/null
+++ b/drivers/reset/ti/reset-ti-data.h
@@ -0,0 +1,56 @@
+/*
+ * 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/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 {
+	void __iomem *reg_base;
+	u32	rstctrl_offs;
+	u32	rstst_offs;
+	u32	rstctrl_bit;
+	u32	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;
+};
+
+#endif
diff --git a/drivers/reset/ti/reset-ti.c b/drivers/reset/ti/reset-ti.c
new file mode 100644
index 0000000..349f4fb
--- /dev/null
+++ b/drivers/reset/ti/reset-ti.c
@@ -0,0 +1,267 @@
+/*
+ * 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/module.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+#include <linux/slab.h>
+
+#include "reset-ti-data.h"
+
+#define DRIVER_NAME "prcm_reset_ti"
+
+static struct ti_reset_data *ti_data;
+
+static int ti_reset_get_of_data(struct ti_reset_reg_data *reset_data,
+				unsigned long id)
+{
+	struct device_node *dev_node;
+	struct device_node *parent;
+	struct device_node *prcm_parent;
+	struct device_node *reset_parent;
+	int ret = -EINVAL;
+
+	dev_node = of_find_node_by_phandle((phandle) id);
+	if (!dev_node) {
+		pr_err("%s: Cannot find phandle node\n", __func__);
+		return ret;
+	}
+
+	/* node parent */
+	parent = of_get_parent(dev_node);
+	if (!parent) {
+		pr_err("%s: Cannot find parent reset node\n", __func__);
+		return ret;
+	}
+	/* prcm reset parent */
+	reset_parent = of_get_next_parent(parent);
+	if (!reset_parent) {
+		pr_err("%s: Cannot find parent reset node\n", __func__);
+		return ret;
+	}
+	/* PRCM Parent */
+	reset_parent = of_get_parent(reset_parent);
+	if (!prcm_parent) {
+		pr_err("%s: Cannot find parent reset node\n", __func__);
+		return ret;
+	}
+
+	reset_data->reg_base = of_iomap(reset_parent, 0);
+	if (!reset_data->reg_base) {
+		pr_err("%s: Cannot map reset parent.\n", __func__);
+		return ret;
+	}
+
+	ret = of_property_read_u32_index(parent, "reg", 0,
+					 &reset_data->rstctrl_offs);
+	if (ret)
+		return ret;
+
+	ret = of_property_read_u32_index(parent, "reg", 1,
+					 &reset_data->rstst_offs);
+	if (ret)
+		return ret;
+
+	ret = of_property_read_u32(dev_node, "control-bit",
+				   &reset_data->rstctrl_bit);
+	if (ret < 0)
+		pr_err("%s: No entry in %s for rstst_offs\n", __func__,
+			   dev_node->name);
+
+	ret = of_property_read_u32(dev_node, "status-bit",
+				   &reset_data->rstst_bit);
+	if (ret < 0)
+		pr_err("%s: No entry in %s for rstst_offs\n", __func__,
+			   dev_node->name);
+
+	return 0;
+}
+
+static void ti_reset_wait_on_reset(struct reset_controller_dev *rcdev,
+				unsigned long id)
+{
+	struct ti_reset_reg_data *temp_reg_data;
+	void __iomem *status_reg;
+	u32 bit_mask = 0;
+	u32 val = 0;
+
+	temp_reg_data = kzalloc(sizeof(struct ti_reset_reg_data), GFP_KERNEL);
+	ti_reset_get_of_data(temp_reg_data, id);
+
+	/* Clear the reset status bit to reflect the current status */
+	status_reg = temp_reg_data->reg_base + temp_reg_data->rstst_offs;
+	bit_mask = temp_reg_data->rstst_bit;
+	do {
+		val = readl(status_reg);
+		if (!(val & (1 << bit_mask)))
+			break;
+	} while (1);
+}
+
+static int ti_reset_assert(struct reset_controller_dev *rcdev,
+			       unsigned long id)
+{
+	struct ti_reset_reg_data *temp_reg_data;
+	void __iomem *reg;
+	void __iomem *status_reg;
+	u32 status_bit = 0;
+	u32 bit_mask = 0;
+	u32 val = 0;
+
+	temp_reg_data = kzalloc(sizeof(struct ti_reset_reg_data), GFP_KERNEL);
+	ti_reset_get_of_data(temp_reg_data, id);
+
+	/* Clear the reset status bit to reflect the current status */
+	status_reg = temp_reg_data->reg_base + temp_reg_data->rstst_offs;
+	status_bit = temp_reg_data->rstst_bit;
+	writel(1 << status_bit, status_reg);
+
+	reg = temp_reg_data->reg_base + temp_reg_data->rstctrl_offs;
+	bit_mask = temp_reg_data->rstctrl_bit;
+	val = readl(reg);
+	if (!(val & bit_mask)) {
+		val |= bit_mask;
+		writel(val, reg);
+	}
+
+	kfree(temp_reg_data);
+
+	return 0;
+}
+
+static int ti_reset_deassert(struct reset_controller_dev *rcdev,
+			       unsigned long id)
+{
+
+	struct ti_reset_reg_data *temp_reg_data;
+	void __iomem *reg;
+	void __iomem *status_reg;
+	u32 status_bit = 0;
+	u32 bit_mask = 0;
+	u32 val = 0;
+
+	temp_reg_data = kzalloc(sizeof(struct ti_reset_reg_data), GFP_KERNEL);
+	ti_reset_get_of_data(temp_reg_data, id);
+
+	/* Clear the reset status bit to reflect the current status */
+	status_reg = temp_reg_data->reg_base + temp_reg_data->rstst_offs;
+	status_bit = temp_reg_data->rstst_bit;
+	writel(1 << status_bit, status_reg);
+
+	reg = temp_reg_data->reg_base + temp_reg_data->rstctrl_offs;
+	bit_mask = temp_reg_data->rstctrl_bit;
+	val = readl(reg);
+	if (val & bit_mask) {
+		val &= ~bit_mask;
+		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 int ti_reset_xlate(struct reset_controller_dev *rcdev,
+			const struct of_phandle_args *reset_spec)
+{
+	struct device_node *dev_node;
+
+	if (WARN_ON(reset_spec->args_count != rcdev->of_reset_n_cells))
+		return -EINVAL;
+
+	/* Verify that the phandle exists */
+	dev_node = of_find_node_by_phandle((phandle) reset_spec->args[0]);
+	if (!dev_node) {
+		pr_err("%s: Cannot find phandle node\n", __func__);
+		return -EINVAL;
+	}
+
+	return reset_spec->args[0];
+}
+
+static struct reset_control_ops ti_reset_ops = {
+	.reset = ti_reset_reset,
+	.assert = ti_reset_assert,
+	.deassert = ti_reset_deassert,
+};
+
+static int ti_reset_probe(struct platform_device *pdev)
+{
+	struct device_node *resets;
+
+	resets = of_find_node_by_name(NULL, "resets");
+	if (!resets) {
+		pr_err("%s: missing 'resets' child node.\n", __func__);
+		return -EINVAL;
+	}
+
+	ti_data = kzalloc(sizeof(*ti_data), GFP_KERNEL);
+	if (!ti_data)
+		return -ENOMEM;
+
+	ti_data->rcdev.owner = THIS_MODULE;
+	ti_data->rcdev.of_node = resets;
+	ti_data->rcdev.ops = &ti_reset_ops;
+
+	ti_data->rcdev.of_reset_n_cells = 1;
+	ti_data->rcdev.of_xlate = &ti_reset_xlate;
+
+	reset_controller_register(&ti_data->rcdev);
+
+	return 0;
+}
+
+static int ti_reset_remove(struct platform_device *pdev)
+{
+	reset_controller_unregister(&ti_data->rcdev);
+
+	return 0;
+}
+
+static const struct of_device_id ti_reset_of_match[] = {
+	{ .compatible = "ti,omap5-prm" },
+	{ .compatible =	"ti,omap4-prm" },
+	{ .compatible =	"ti,omap5-prm" },
+	{ .compatible =	"ti,dra7-prm" },
+	{ .compatible = "ti,am4-prcm" },
+	{ .compatible =	"ti,am3-prcm" },
+	{},
+};
+
+static struct platform_driver ti_reset_driver = {
+	.probe	= ti_reset_probe,
+	.remove	= ti_reset_remove,
+	.driver	= {
+		.name		= DRIVER_NAME,
+		.owner		= THIS_MODULE,
+		.of_match_table	= of_match_ptr(ti_reset_of_match),
+	},
+};
+module_platform_driver(ti_reset_driver);
+
+MODULE_DESCRIPTION("PRCM reset driver for TI SoCs");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:" DRIVER_NAME);
-- 
1.7.9.5

Re: [RFC] [v2 Patch 1/6] drivers: reset: TI: SoC reset controller support.

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 13955 bytes --]

Hi,

On Mon, May 05, 2014 at 03:09:22PM -0500, Dan Murphy wrote:
> 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.

you should break your commit log at around 72 characters at most.

> 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 |   56 ++++++++
>  drivers/reset/ti/reset-ti.c      |  267 ++++++++++++++++++++++++++++++++++++++
>  6 files changed, 334 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
> 
> 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"

this driver is so small that I'm not sure you need a directory for it.

> 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

don't you want to depend on ARCH_OMAP || COMPILE_TEST ?

> diff --git a/drivers/reset/ti/reset-ti-data.h b/drivers/reset/ti/reset-ti-data.h
> new file mode 100644
> index 0000000..4d2a6d5
> --- /dev/null
> +++ b/drivers/reset/ti/reset-ti-data.h
> @@ -0,0 +1,56 @@
> +/*
> + * PRCM reset driver for TI SoC's
> + *
> + * Copyright 2014 Texas Instruments Inc.

this is not the TI aproved way for copyright notices. Yeah,
nit-picking...

> + * 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/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 {
> +	void __iomem *reg_base;
> +	u32	rstctrl_offs;
> +	u32	rstst_offs;
> +	u32	rstctrl_bit;
> +	u32	rstst_bit;

this structure can be folded into struct ti_reset_data and all of that
can be initialized during probe.

> +};
> +
> +/**
> + * 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;
> +};
> +
> +#endif

this entire file can be moved into the .c file. It's too small and the
only user for these new types is the .c driver anyway.

> diff --git a/drivers/reset/ti/reset-ti.c b/drivers/reset/ti/reset-ti.c
> new file mode 100644
> index 0000000..349f4fb
> --- /dev/null
> +++ b/drivers/reset/ti/reset-ti.c
> @@ -0,0 +1,267 @@
> +/*
> + * PRCM reset driver for TI SoC's
> + *
> + * Copyright 2014 Texas Instruments Inc.
> + *
> + * Author: Dan Murphy <dmurphy@ti.com>

fix copyright notice here too

> + * 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/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset.h>
> +#include <linux/slab.h>
> +
> +#include "reset-ti-data.h"
> +
> +#define DRIVER_NAME "prcm_reset_ti"
> +
> +static struct ti_reset_data *ti_data;

NAK, you *really* don't need and don't to have this global here. It only
creates problems. And avoid arguing that "there's only one reset
controller anyway" because that has bitten us in the past. Also, it's
not a lot of work to make proper use of dev_set/get_drvdata() and
container_of() to find your struct ti_reset_data pointer.

> +static int ti_reset_get_of_data(struct ti_reset_reg_data *reset_data,
> +				unsigned long id)
> +{
> +	struct device_node *dev_node;
> +	struct device_node *parent;
> +	struct device_node *prcm_parent;
> +	struct device_node *reset_parent;
> +	int ret = -EINVAL;
> +
> +	dev_node = of_find_node_by_phandle((phandle) id);
> +	if (!dev_node) {
> +		pr_err("%s: Cannot find phandle node\n", __func__);

pass a struct device pointer as argument so you can convert these to
dev_err().

> +		return ret;
> +	}
> +
> +	/* node parent */
> +	parent = of_get_parent(dev_node);
> +	if (!parent) {
> +		pr_err("%s: Cannot find parent reset node\n", __func__);
> +		return ret;
> +	}
> +	/* prcm reset parent */
> +	reset_parent = of_get_next_parent(parent);
> +	if (!reset_parent) {
> +		pr_err("%s: Cannot find parent reset node\n", __func__);
> +		return ret;
> +	}
> +	/* PRCM Parent */
> +	reset_parent = of_get_parent(reset_parent);
> +	if (!prcm_parent) {
> +		pr_err("%s: Cannot find parent reset node\n", __func__);
> +		return ret;
> +	}
> +
> +	reset_data->reg_base = of_iomap(reset_parent, 0);

please don't. of_iomap() is the stupidest helper in the kernel. Find
your resource using platform_get_resource() and map it with
devm_ioremap_resource() since that will properly request_mem_region()
and correctly map the resource with or without caching.

> +	if (!reset_data->reg_base) {
> +		pr_err("%s: Cannot map reset parent.\n", __func__);
> +		return ret;
> +	}
> +
> +	ret = of_property_read_u32_index(parent, "reg", 0,
> +					 &reset_data->rstctrl_offs);
> +	if (ret)
> +		return ret;
> +
> +	ret = of_property_read_u32_index(parent, "reg", 1,
> +					 &reset_data->rstst_offs);
> +	if (ret)
> +		return ret;
> +
> +	ret = of_property_read_u32(dev_node, "control-bit",
> +				   &reset_data->rstctrl_bit);
> +	if (ret < 0)
> +		pr_err("%s: No entry in %s for rstst_offs\n", __func__,
> +			   dev_node->name);
> +
> +	ret = of_property_read_u32(dev_node, "status-bit",
> +				   &reset_data->rstst_bit);
> +	if (ret < 0)
> +		pr_err("%s: No entry in %s for rstst_offs\n", __func__,
> +			   dev_node->name);
> +
> +	return 0;
> +}
> +
> +static void ti_reset_wait_on_reset(struct reset_controller_dev *rcdev,
> +				unsigned long id)
> +{

right here you should have:

	struct ti_reset_data *ti_data = container_of(rcdev,
		struct ti_reset_data, rcdev);

> +	struct ti_reset_reg_data *temp_reg_data;
> +	void __iomem *status_reg;
> +	u32 bit_mask = 0;
> +	u32 val = 0;
> +
> +	temp_reg_data = kzalloc(sizeof(struct ti_reset_reg_data), GFP_KERNEL);

let's think about this for a second:

user asks for a reset, then ->assert() method gets called, which will
allocate memory, do some crap, free the allocated memory, then you call
your ->deassert() method which will, allocate memory, do something, free
memory, then this method is called which will allocate memory, do
something, free memory.

Note that *all* of your allocations a) are unnecessary and b) will break
resets from inside IRQ context...

> +	ti_reset_get_of_data(temp_reg_data, id);

this means that *every time* a reset is asserted/deasserted/waited on,
you will parse *ONE MORE TIME* the DTB. Why ? Why don't you parse it
once during probe() and cache the results ?

> +	/* Clear the reset status bit to reflect the current status */
> +	status_reg = temp_reg_data->reg_base + temp_reg_data->rstst_offs;
> +	bit_mask = temp_reg_data->rstst_bit;
> +	do {
> +		val = readl(status_reg);
> +		if (!(val & (1 << bit_mask)))
> +			break;
> +	} while (1);

also, none of these loops have a timeout. You are creating the
possibility of hanging the platform completely if the HW misbehaves.

> +static int ti_reset_assert(struct reset_controller_dev *rcdev,
> +			       unsigned long id)
> +{
> +	struct ti_reset_reg_data *temp_reg_data;
> +	void __iomem *reg;
> +	void __iomem *status_reg;
> +	u32 status_bit = 0;
> +	u32 bit_mask = 0;
> +	u32 val = 0;
> +
> +	temp_reg_data = kzalloc(sizeof(struct ti_reset_reg_data), GFP_KERNEL);
> +	ti_reset_get_of_data(temp_reg_data, id);
> +
> +	/* Clear the reset status bit to reflect the current status */
> +	status_reg = temp_reg_data->reg_base + temp_reg_data->rstst_offs;
> +	status_bit = temp_reg_data->rstst_bit;
> +	writel(1 << status_bit, status_reg);
> +
> +	reg = temp_reg_data->reg_base + temp_reg_data->rstctrl_offs;
> +	bit_mask = temp_reg_data->rstctrl_bit;
> +	val = readl(reg);
> +	if (!(val & bit_mask)) {
> +		val |= bit_mask;
> +		writel(val, reg);
> +	}
> +
> +	kfree(temp_reg_data);

same comments as previous callback

> +	return 0;
> +}
> +
> +static int ti_reset_deassert(struct reset_controller_dev *rcdev,
> +			       unsigned long id)
> +{
> +
> +	struct ti_reset_reg_data *temp_reg_data;
> +	void __iomem *reg;
> +	void __iomem *status_reg;
> +	u32 status_bit = 0;
> +	u32 bit_mask = 0;
> +	u32 val = 0;
> +
> +	temp_reg_data = kzalloc(sizeof(struct ti_reset_reg_data), GFP_KERNEL);
> +	ti_reset_get_of_data(temp_reg_data, id);
> +
> +	/* Clear the reset status bit to reflect the current status */
> +	status_reg = temp_reg_data->reg_base + temp_reg_data->rstst_offs;
> +	status_bit = temp_reg_data->rstst_bit;
> +	writel(1 << status_bit, status_reg);
> +
> +	reg = temp_reg_data->reg_base + temp_reg_data->rstctrl_offs;
> +	bit_mask = temp_reg_data->rstctrl_bit;
> +	val = readl(reg);
> +	if (val & bit_mask) {
> +		val &= ~bit_mask;
> +		writel(val, reg);
> +	}

and here. Also, this is leaking temp_reg_data.

> +
> +	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 int ti_reset_xlate(struct reset_controller_dev *rcdev,
> +			const struct of_phandle_args *reset_spec)
> +{
> +	struct device_node *dev_node;
> +
> +	if (WARN_ON(reset_spec->args_count != rcdev->of_reset_n_cells))
> +		return -EINVAL;
> +
> +	/* Verify that the phandle exists */
> +	dev_node = of_find_node_by_phandle((phandle) reset_spec->args[0]);
> +	if (!dev_node) {
> +		pr_err("%s: Cannot find phandle node\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	return reset_spec->args[0];
> +}
> +
> +static struct reset_control_ops ti_reset_ops = {
> +	.reset = ti_reset_reset,
> +	.assert = ti_reset_assert,
> +	.deassert = ti_reset_deassert,
> +};
> +
> +static int ti_reset_probe(struct platform_device *pdev)
> +{
> +	struct device_node *resets;

here you should have something like:

	struct ti_reset_data *ti_data;

	[...]

	ti_data = devm_kzalloc(sizeof(*ti_data), GFP_KERNEL);
	if (!ti_data)
		return -ENOMEM;

	platform_get_resource(...);

	ti_data->base = devm_ioremap_resource(res);
	if (IS_ERR(ti_data->base))
		return PTR_ERR(ti_data->base);

	platform_set_drvdata(pdev, ti_data);

> +	resets = of_find_node_by_name(NULL, "resets");
> +	if (!resets) {
> +		pr_err("%s: missing 'resets' child node.\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	ti_data = kzalloc(sizeof(*ti_data), GFP_KERNEL);
> +	if (!ti_data)
> +		return -ENOMEM;
> +
> +	ti_data->rcdev.owner = THIS_MODULE;
> +	ti_data->rcdev.of_node = resets;
> +	ti_data->rcdev.ops = &ti_reset_ops;
> +
> +	ti_data->rcdev.of_reset_n_cells = 1;
> +	ti_data->rcdev.of_xlate = &ti_reset_xlate;
> +
> +	reset_controller_register(&ti_data->rcdev);
> +
> +	return 0;
> +}
> +
> +static int ti_reset_remove(struct platform_device *pdev)
> +{

and here:

	struct ti_reset_data *ti_data = platform_get_drvdata(pdev);

	reset_controller_unregister(&ti_data->rcdev);

> +	reset_controller_unregister(&ti_data->rcdev);
> +
> +	return 0;
> +}

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC] [v2 Patch 1/6] drivers: reset: TI: SoC reset controller support.

[Dan Murphy]

Felipe

Thanks for the comments

On 05/05/2014 04:33 PM, Felipe Balbi wrote:
> Hi,
>
> On Mon, May 05, 2014 at 03:09:22PM -0500, Dan Murphy wrote:
>> 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.
> you should break your commit log at around 72 characters at most.

Do you mean 72 characters per line?

>
>> 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 |   56 ++++++++
>>  drivers/reset/ti/reset-ti.c      |  267 ++++++++++++++++++++++++++++++++++++++
>>  6 files changed, 334 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
>>
>> 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"
> this driver is so small that I'm not sure you need a directory for it.

Yeah.  Carry over from v1 when we had the object data files which made
things bigger I will put them back.

>
>> 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
> don't you want to depend on ARCH_OMAP || COMPILE_TEST ?

Yes

>
>> diff --git a/drivers/reset/ti/reset-ti-data.h b/drivers/reset/ti/reset-ti-data.h
>> new file mode 100644
>> index 0000000..4d2a6d5
>> --- /dev/null
>> +++ b/drivers/reset/ti/reset-ti-data.h
>> @@ -0,0 +1,56 @@
>> +/*
>> + * PRCM reset driver for TI SoC's
>> + *
>> + * Copyright 2014 Texas Instruments Inc.
> this is not the TI aproved way for copyright notices. Yeah,
> nit-picking...

Hmm.  Will need to look at other TI files to correct then.

>
>> + * 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/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 {
>> +	void __iomem *reg_base;
>> +	u32	rstctrl_offs;
>> +	u32	rstst_offs;
>> +	u32	rstctrl_bit;
>> +	u32	rstst_bit;
> this structure can be folded into struct ti_reset_data and all of that
> can be initialized during probe.

I could do that.  It will simplify the de-referencing as well.

>> +};
>> +
>> +/**
>> + * 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;
>> +};
>> +
>> +#endif
> this entire file can be moved into the .c file. It's too small and the
> only user for these new types is the .c driver anyway.

This was another carry over from v1 when I had other data within.
So I can collapse this now.

>> diff --git a/drivers/reset/ti/reset-ti.c b/drivers/reset/ti/reset-ti.c
>> new file mode 100644
>> index 0000000..349f4fb
>> --- /dev/null
>> +++ b/drivers/reset/ti/reset-ti.c
>> @@ -0,0 +1,267 @@
>> +/*
>> + * PRCM reset driver for TI SoC's
>> + *
>> + * Copyright 2014 Texas Instruments Inc.
>> + *
>> + * Author: Dan Murphy <dmurphy@ti.com>
> fix copyright notice here too
>
>> + * 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/module.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/reset.h>
>> +#include <linux/slab.h>
>> +
>> +#include "reset-ti-data.h"
>> +
>> +#define DRIVER_NAME "prcm_reset_ti"
>> +
>> +static struct ti_reset_data *ti_data;
> NAK, you *really* don't need and don't to have this global here. It only
> creates problems. And avoid arguing that "there's only one reset
> controller anyway" because that has bitten us in the past. Also, it's
> not a lot of work to make proper use of dev_set/get_drvdata() and
> container_of() to find your struct ti_reset_data pointer.

Agreed will fix

>
>> +static int ti_reset_get_of_data(struct ti_reset_reg_data *reset_data,
>> +				unsigned long id)
>> +{
>> +	struct device_node *dev_node;
>> +	struct device_node *parent;
>> +	struct device_node *prcm_parent;
>> +	struct device_node *reset_parent;
>> +	int ret = -EINVAL;
>> +
>> +	dev_node = of_find_node_by_phandle((phandle) id);
>> +	if (!dev_node) {
>> +		pr_err("%s: Cannot find phandle node\n", __func__);
> pass a struct device pointer as argument so you can convert these to
> dev_err().

Agreed

>
>> +		return ret;
>> +	}
>> +
>> +	/* node parent */
>> +	parent = of_get_parent(dev_node);
>> +	if (!parent) {
>> +		pr_err("%s: Cannot find parent reset node\n", __func__);
>> +		return ret;
>> +	}
>> +	/* prcm reset parent */
>> +	reset_parent = of_get_next_parent(parent);
>> +	if (!reset_parent) {
>> +		pr_err("%s: Cannot find parent reset node\n", __func__);
>> +		return ret;
>> +	}
>> +	/* PRCM Parent */
>> +	reset_parent = of_get_parent(reset_parent);
>> +	if (!prcm_parent) {
>> +		pr_err("%s: Cannot find parent reset node\n", __func__);
>> +		return ret;
>> +	}
>> +
>> +	reset_data->reg_base = of_iomap(reset_parent, 0);
> please don't. of_iomap() is the stupidest helper in the kernel. Find
> your resource using platform_get_resource() and map it with
> devm_ioremap_resource() since that will properly request_mem_region()
> and correctly map the resource with or without caching.

Thanks for the information.  The reason this is here so that if there are multiple PRCM
modules I can just get the base address for the phandle of interest.

>
>> +	if (!reset_data->reg_base) {
>> +		pr_err("%s: Cannot map reset parent.\n", __func__);
>> +		return ret;
>> +	}
>> +
>> +	ret = of_property_read_u32_index(parent, "reg", 0,
>> +					 &reset_data->rstctrl_offs);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = of_property_read_u32_index(parent, "reg", 1,
>> +					 &reset_data->rstst_offs);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = of_property_read_u32(dev_node, "control-bit",
>> +				   &reset_data->rstctrl_bit);
>> +	if (ret < 0)
>> +		pr_err("%s: No entry in %s for rstst_offs\n", __func__,
>> +			   dev_node->name);
>> +
>> +	ret = of_property_read_u32(dev_node, "status-bit",
>> +				   &reset_data->rstst_bit);
>> +	if (ret < 0)
>> +		pr_err("%s: No entry in %s for rstst_offs\n", __func__,
>> +			   dev_node->name);
>> +
>> +	return 0;
>> +}
>> +
>> +static void ti_reset_wait_on_reset(struct reset_controller_dev *rcdev,
>> +				unsigned long id)
>> +{
> right here you should have:
>
> 	struct ti_reset_data *ti_data = container_of(rcdev,
> 		struct ti_reset_data, rcdev);

I had that in v1.  Should have kept that.
I will change

>
>> +	struct ti_reset_reg_data *temp_reg_data;
>> +	void __iomem *status_reg;
>> +	u32 bit_mask = 0;
>> +	u32 val = 0;
>> +
>> +	temp_reg_data = kzalloc(sizeof(struct ti_reset_reg_data), GFP_KERNEL);
> let's think about this for a second:
>
> user asks for a reset, then ->assert() method gets called, which will
> allocate memory, do some crap, free the allocated memory, then you call
> your ->deassert() method which will, allocate memory, do something, free
> memory, then this method is called which will allocate memory, do
> something, free memory.
>
> Note that *all* of your allocations a) are unnecessary and b) will break
> resets from inside IRQ context...

This made also think that this is not thread safe as this reset calls can be called
from multiple callers so I think a semaphore is order for this function plus the other calls.

>
>> +	ti_reset_get_of_data(temp_reg_data, id);
> this means that *every time* a reset is asserted/deasserted/waited on,
> you will parse *ONE MORE TIME* the DTB. Why ? Why don't you parse it
> once during probe() and cache the results ?

Cache it into what, a list?
I had implemented a list before and received comments do not use a list.  Because you
have to iterate through the list every time.  And a list would need to contain some sort
of indexing agent which defeats the purpose of a phandle.  Then the DTB and kernel images
would have to be in sync for the indexes.

If I put it into an array I run into issues with a bounded array and need to introduce
the index anyway.  So passing the phandle is futile which means I have to introduce the
indexing from the V1 series again.

For my information why is parsing the DTB worse then iterating through a list?
Is there a possibility that the DTB will be over written?


>
>> +	/* Clear the reset status bit to reflect the current status */
>> +	status_reg = temp_reg_data->reg_base + temp_reg_data->rstst_offs;
>> +	bit_mask = temp_reg_data->rstst_bit;
>> +	do {
>> +		val = readl(status_reg);
>> +		if (!(val & (1 << bit_mask)))
>> +			break;
>> +	} while (1);
> also, none of these loops have a timeout. You are creating the
> possibility of hanging the platform completely if the HW misbehaves.

Agreed.  Philip commented on that on v1 I overlooked that comment.

>
>> +static int ti_reset_assert(struct reset_controller_dev *rcdev,
>> +			       unsigned long id)
>> +{
>> +	struct ti_reset_reg_data *temp_reg_data;
>> +	void __iomem *reg;
>> +	void __iomem *status_reg;
>> +	u32 status_bit = 0;
>> +	u32 bit_mask = 0;
>> +	u32 val = 0;
>> +
>> +	temp_reg_data = kzalloc(sizeof(struct ti_reset_reg_data), GFP_KERNEL);
>> +	ti_reset_get_of_data(temp_reg_data, id);
>> +
>> +	/* Clear the reset status bit to reflect the current status */
>> +	status_reg = temp_reg_data->reg_base + temp_reg_data->rstst_offs;
>> +	status_bit = temp_reg_data->rstst_bit;
>> +	writel(1 << status_bit, status_reg);
>> +
>> +	reg = temp_reg_data->reg_base + temp_reg_data->rstctrl_offs;
>> +	bit_mask = temp_reg_data->rstctrl_bit;
>> +	val = readl(reg);
>> +	if (!(val & bit_mask)) {
>> +		val |= bit_mask;
>> +		writel(val, reg);
>> +	}
>> +
>> +	kfree(temp_reg_data);
> same comments as previous callback

Same answer here

>
>> +	return 0;
>> +}
>> +
>> +static int ti_reset_deassert(struct reset_controller_dev *rcdev,
>> +			       unsigned long id)
>> +{
>> +
>> +	struct ti_reset_reg_data *temp_reg_data;
>> +	void __iomem *reg;
>> +	void __iomem *status_reg;
>> +	u32 status_bit = 0;
>> +	u32 bit_mask = 0;
>> +	u32 val = 0;
>> +
>> +	temp_reg_data = kzalloc(sizeof(struct ti_reset_reg_data), GFP_KERNEL);
>> +	ti_reset_get_of_data(temp_reg_data, id);
>> +
>> +	/* Clear the reset status bit to reflect the current status */
>> +	status_reg = temp_reg_data->reg_base + temp_reg_data->rstst_offs;
>> +	status_bit = temp_reg_data->rstst_bit;
>> +	writel(1 << status_bit, status_reg);
>> +
>> +	reg = temp_reg_data->reg_base + temp_reg_data->rstctrl_offs;
>> +	bit_mask = temp_reg_data->rstctrl_bit;
>> +	val = readl(reg);
>> +	if (val & bit_mask) {
>> +		val &= ~bit_mask;
>> +		writel(val, reg);
>> +	}
> and here. Also, this is leaking temp_reg_data.

and here

>
>> +
>> +	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 int ti_reset_xlate(struct reset_controller_dev *rcdev,
>> +			const struct of_phandle_args *reset_spec)
>> +{
>> +	struct device_node *dev_node;
>> +
>> +	if (WARN_ON(reset_spec->args_count != rcdev->of_reset_n_cells))
>> +		return -EINVAL;
>> +
>> +	/* Verify that the phandle exists */
>> +	dev_node = of_find_node_by_phandle((phandle) reset_spec->args[0]);
>> +	if (!dev_node) {
>> +		pr_err("%s: Cannot find phandle node\n", __func__);
>> +		return -EINVAL;
>> +	}
>> +
>> +	return reset_spec->args[0];
>> +}
>> +
>> +static struct reset_control_ops ti_reset_ops = {
>> +	.reset = ti_reset_reset,
>> +	.assert = ti_reset_assert,
>> +	.deassert = ti_reset_deassert,
>> +};
>> +
>> +static int ti_reset_probe(struct platform_device *pdev)
>> +{
>> +	struct device_node *resets;
> here you should have something like:
>
> 	struct ti_reset_data *ti_data;
>
> 	[...]
>
> 	ti_data = devm_kzalloc(sizeof(*ti_data), GFP_KERNEL);
> 	if (!ti_data)
> 		return -ENOMEM;
>
> 	platform_get_resource(...);
>
> 	ti_data->base = devm_ioremap_resource(res);
> 	if (IS_ERR(ti_data->base))
> 		return PTR_ERR(ti_data->base);
>
> 	platform_set_drvdata(pdev, ti_data);

agreed.  Did not think of this when I made this a module.

>
>> +	resets = of_find_node_by_name(NULL, "resets");
>> +	if (!resets) {
>> +		pr_err("%s: missing 'resets' child node.\n", __func__);
>> +		return -EINVAL;
>> +	}
>> +
>> +	ti_data = kzalloc(sizeof(*ti_data), GFP_KERNEL);
>> +	if (!ti_data)
>> +		return -ENOMEM;
>> +
>> +	ti_data->rcdev.owner = THIS_MODULE;
>> +	ti_data->rcdev.of_node = resets;
>> +	ti_data->rcdev.ops = &ti_reset_ops;
>> +
>> +	ti_data->rcdev.of_reset_n_cells = 1;
>> +	ti_data->rcdev.of_xlate = &ti_reset_xlate;
>> +
>> +	reset_controller_register(&ti_data->rcdev);
>> +
>> +	return 0;
>> +}
>> +
>> +static int ti_reset_remove(struct platform_device *pdev)
>> +{
> and here:
>
> 	struct ti_reset_data *ti_data = platform_get_drvdata(pdev);
>
> 	reset_controller_unregister(&ti_data->rcdev);
>
>> +	reset_controller_unregister(&ti_data->rcdev);
>> +
>> +	return 0;
>> +}


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

Re: [RFC] [v2 Patch 1/6] drivers: reset: TI: SoC reset controller support.

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 9795 bytes --]

Hi,

[ I had to manually rewrap your email which came with long lines, please
have a read on Documentation/email-clients.txt ]

On Tue, May 06, 2014 at 08:14:04AM -0500, Dan Murphy wrote:
> >> 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.
> > you should break your commit log at around 72 characters at most.
> 
> Do you mean 72 characters per line?

<sartalics> no, that's too much. The entire commit log </sartalics>

Yes, per-line :-)

> >> diff --git a/drivers/reset/ti/reset-ti-data.h b/drivers/reset/ti/reset-ti-data.h
> >> new file mode 100644
> >> index 0000000..4d2a6d5
> >> --- /dev/null
> >> +++ b/drivers/reset/ti/reset-ti-data.h
> >> @@ -0,0 +1,56 @@
> >> +/*
> >> + * PRCM reset driver for TI SoC's
> >> + *
> >> + * Copyright 2014 Texas Instruments Inc.
> > this is not the TI aproved way for copyright notices. Yeah,
> > nit-picking...
> 
> Hmm.  Will need to look at other TI files to correct then.

/**
 * file.c - Short Description
 *
 * Copyright (C) 2010-2011 Texas Instruments Incorporated -  http://www.ti.com
 *
 * Author: Dan Murphy <dmurphy-l0cyMroinI0@public.gmane.org>
 *
 * GPL text goes here
 */

> >> + * Author: Dan Murphy <dmurphy-l0cyMroinI0@public.gmane.org>
> >> + *
> >> + * 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/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 {
> >> +	void __iomem *reg_base;
> >> +	u32	rstctrl_offs;
> >> +	u32	rstst_offs;
> >> +	u32	rstctrl_bit;
> >> +	u32	rstst_bit;
> > this structure can be folded into struct ti_reset_data and all of that
> > can be initialized during probe.
> 
> I could do that.  It will simplify the de-referencing as well.

yeah, and it will prevent constant alloc/free of this structure

> >> diff --git a/drivers/reset/ti/reset-ti.c b/drivers/reset/ti/reset-ti.c
> >> new file mode 100644
> >> index 0000000..349f4fb
> >> --- /dev/null
> >> +++ b/drivers/reset/ti/reset-ti.c
> >> @@ -0,0 +1,267 @@
> >> +/*
> >> + * PRCM reset driver for TI SoC's
> >> + *
> >> + * Copyright 2014 Texas Instruments Inc.
> >> + *
> >> + * Author: Dan Murphy <dmurphy-l0cyMroinI0@public.gmane.org>
> > fix copyright notice here too
> >
> >> + * 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/module.h>
> >> +#include <linux/of_address.h>
> >> +#include <linux/of_device.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/reset.h>
> >> +#include <linux/slab.h>
> >> +
> >> +#include "reset-ti-data.h"
> >> +
> >> +#define DRIVER_NAME "prcm_reset_ti"
> >> +
> >> +static struct ti_reset_data *ti_data;
> > NAK, you *really* don't need and don't to have this global here. It only

don't _want_ to have, missed the verb

> >> +		return ret;
> >> +	}
> >> +
> >> +	/* node parent */
> >> +	parent = of_get_parent(dev_node);
> >> +	if (!parent) {
> >> +		pr_err("%s: Cannot find parent reset node\n", __func__);
> >> +		return ret;
> >> +	}
> >> +	/* prcm reset parent */
> >> +	reset_parent = of_get_next_parent(parent);
> >> +	if (!reset_parent) {
> >> +		pr_err("%s: Cannot find parent reset node\n", __func__);
> >> +		return ret;
> >> +	}
> >> +	/* PRCM Parent */
> >> +	reset_parent = of_get_parent(reset_parent);
> >> +	if (!prcm_parent) {
> >> +		pr_err("%s: Cannot find parent reset node\n", __func__);
> >> +		return ret;
> >> +	}
> >> +
> >> +	reset_data->reg_base = of_iomap(reset_parent, 0);
> > please don't. of_iomap() is the stupidest helper in the kernel. Find
> > your resource using platform_get_resource() and map it with
> > devm_ioremap_resource() since that will properly request_mem_region()
> > and correctly map the resource with or without caching.
> 
> Thanks for the information.  The reason this is here so that if there are multiple PRCM
> modules I can just get the base address for the phandle of interest.

yeah, you can also:

	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
	[...]
	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
	[...]

or even:

	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "my_resource");
	[...]
	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "my_other_resource");
	[...]

> >> +	struct ti_reset_reg_data *temp_reg_data;
> >> +	void __iomem *status_reg;
> >> +	u32 bit_mask = 0;
> >> +	u32 val = 0;
> >> +
> >> +	temp_reg_data = kzalloc(sizeof(struct ti_reset_reg_data), GFP_KERNEL);
> > let's think about this for a second:
> >
> > user asks for a reset, then ->assert() method gets called, which will
> > allocate memory, do some crap, free the allocated memory, then you call
> > your ->deassert() method which will, allocate memory, do something, free
> > memory, then this method is called which will allocate memory, do
> > something, free memory.
> >
> > Note that *all* of your allocations a) are unnecessary and b) will break
> > resets from inside IRQ context...
> 
> This made also think that this is not thread safe as this reset calls
> can be called from multiple callers so I think a semaphore is order
> for this function plus the other calls.

right

> >> +	ti_reset_get_of_data(temp_reg_data, id);
> > this means that *every time* a reset is asserted/deasserted/waited on,
> > you will parse *ONE MORE TIME* the DTB. Why ? Why don't you parse it
> > once during probe() and cache the results ?
> 
> Cache it into what, a list?

into your struct ti_reset_data:

	of_get_property_u32(foo, "bar", &ti_data->bar);

following accesses you just need to read ti_data->bar.

> I had implemented a list before and received comments do not use a
> list.  Because you have to iterate through the list every time.  And a
> list would need to contain some sort of indexing agent which defeats
> the purpose of a phandle.  Then the DTB and kernel images would have
> to be in sync for the indexes.
> 
> If I put it into an array I run into issues with a bounded array and
> need to introduce the index anyway.  So passing the phandle is futile
> which means I have to introduce the indexing from the V1 series again.
> 
> For my information why is parsing the DTB worse then iterating through
> a list?  Is there a possibility that the DTB will be over written?

what are you talking about ?? Look at what how you're parsing your DTB:

+       ret = of_property_read_u32_index(parent, "reg", 0,
+                                        &reset_data->rstctrl_offs);
+       if (ret)
+               return ret;
+
+       ret = of_property_read_u32_index(parent, "reg", 1,
+                                        &reset_data->rstst_offs);
+       if (ret)
+               return ret;
+
+       ret = of_property_read_u32(dev_node, "control-bit",
+                                  &reset_data->rstctrl_bit);
+       if (ret < 0)
+               pr_err("%s: No entry in %s for rstst_offs\n", __func__,
+                          dev_node->name);
+
+       ret = of_property_read_u32(dev_node, "status-bit",
+                                  &reset_data->rstst_bit);
+       if (ret < 0)
+               pr_err("%s: No entry in %s for rstst_offs\n", __func__,
+                          dev_node->name);

why can't you do that from probe and cache the results inside struct
ti_reset_data ? IOW:

	probe()
	{
		[ ... ]

		ret = of_property_read_u32_index(parent, "reg", 0,
				&ti_data->rstctrl_offs);
		[ ... ]

		ret = of_property_read_u32_index(parent, "reg", 1,
				&ti_data->rstst_offs);
		[ ... ]

		ret = of_property_read_u32(dev_node, "control-bit",
				&ti_data->rstctrl_bit);
		[ ... ]

		ret = of_property_read_u32(dev_node, "status-bit",
				&ti_data->rstst_bit);
		[ ... ]

		return 0;
	}

cheers

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[RFC] [v2 Patch 2/6] 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-l0cyMroinI0@public.gmane.org>
---
 .../devicetree/bindings/reset/ti,reset.txt         |  103 ++++++++++++++++++++
 1 file changed, 103 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..9d5c29c
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/ti,reset.txt
@@ -0,0 +1,103 @@
+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 parent properties:
+- compatible : Should be one of,
+		"ti,omap4-prm" for OMAP4 PRM instances
+		"ti,omap5-prm" for OMAP5 PRM instances
+		"ti,dra7-prm" for DRA7xx PRM instances
+		"ti,am4-prcm" for AM43xx PRCM instances
+		"ti,am3-prcm" for AM33xx PRCM instances
+
+Required child reset property:
+- compatible : Should be
+		"resets" for All TI SoCs
+
+example:
+		prm: prm@4ae06000 {
+			compatible = "ti,omap5-prm";
+			reg = <0x4ae06000 0x3000>;
+
+			prm_resets: resets {
+				#address-cells = <1>;
+				#size-cells = <1>;
+				#reset-cells = <1>;
+			};
+		};
+
+
+Reset node declaration
+==============================================
+The reset node is declared in a parent child relationship.  The main parent
+is the PRCM module which contains the base address.  The first child within
+the reset parent declares the target modules reset name.  This is followed by
+the control and status offsets.
+
+Within the first reset child node is a secondary child node which declares the
+reset signal of interest.  Under this node the control and status bits
+are declared.  These bits declare the bit mask for the target reset.
+
+
+Required properties:
+reg - This is the register offset from the PRCM parent.
+	This must be declared as:
+
+	reg = <control register offset>,
+		  <status register offset>;
+
+control-bit - This is the bit within the register which controls the reset
+	of the target module.  This is declared as a bit mask for the register.
+status-bit - This is the bit within the register which contains the status of
+	the reset of the target module.
+	This is declared as a bit mask for the register.
+
+example:
+&prm_resets {
+	dsp_rstctrl {
+		reg = <0x1c00>,
+			  <0x1c04>;
+
+		dsp_reset: dsp_reset {
+			control-bit = <0x01>;
+			status-bit = <0x01>;
+		};
+	};
+};
+
+
+
+Client Node Declaration
+==============================================
+This is the consumer of the parent node to declare what resets this
+particular module is interested in.
+
+example:
+	src: src@55082000 {
+		    resets = <&reset_src phandle>;
+			reset-names = "<reset_name>";
+	};
+
+Required Properties:
+reset_src - This is the parent DT entry for the reset controller
+phandle - This is the phandle of the specific reset to be used by the clien
+	driver.
+reset-names - This is the reset name of module that the device driver
+	needs to be able to reset.  This value must correspond to a value within
+	the reset controller array.
+
+example:
+resets = <&prm_resets &dsp_mmu_reset>;
+reset-names = "dsp_mmu_reset";
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] [v2 Patch 2/6] ARM: TI: Describe the ti reset DT entries

[Tony Lindgren]

* Dan Murphy <dmurphy-l0cyMroinI0@public.gmane.org> [140505 13:10]:
> +
> +Required parent properties:
> +- compatible : Should be one of,
> +		"ti,omap4-prm" for OMAP4 PRM instances
> +		"ti,omap5-prm" for OMAP5 PRM instances
> +		"ti,dra7-prm" for DRA7xx PRM instances
> +		"ti,am4-prcm" for AM43xx PRCM instances
> +		"ti,am3-prcm" for AM33xx PRCM instances
> +
> +Required child reset property:
> +- compatible : Should be
> +		"resets" for All TI SoCs
> +
> +example:
> +		prm: prm@4ae06000 {
> +			compatible = "ti,omap5-prm";
> +			reg = <0x4ae06000 0x3000>;
> +
> +			prm_resets: resets {
> +				#address-cells = <1>;
> +				#size-cells = <1>;
> +				#reset-cells = <1>;
> +			};
> +		};

The reg entries you have in the example below has different format
compared to this?

> +Reset node declaration
> +==============================================
> +The reset node is declared in a parent child relationship.  The main parent
> +is the PRCM module which contains the base address.  The first child within
> +the reset parent declares the target modules reset name.  This is followed by
> +the control and status offsets.
> +
> +Within the first reset child node is a secondary child node which declares the
> +reset signal of interest.  Under this node the control and status bits
> +are declared.  These bits declare the bit mask for the target reset.
> +
> +
> +Required properties:
> +reg - This is the register offset from the PRCM parent.
> +	This must be declared as:
> +
> +	reg = <control register offset>,
> +		  <status register offset>;
> +
> +control-bit - This is the bit within the register which controls the reset
> +	of the target module.  This is declared as a bit mask for the register.
> +status-bit - This is the bit within the register which contains the status of
> +	the reset of the target module.
> +	This is declared as a bit mask for the register.
> +
> +example:
> +&prm_resets {
> +	dsp_rstctrl {
> +		reg = <0x1c00>,
> +			  <0x1c04>;

Shouldn't this be start and size instead of start and end here?

> +		dsp_reset: dsp_reset {
> +			control-bit = <0x01>;
> +			status-bit = <0x01>;
> +		};
> +	};
> +};

Are the control-bit and status-bit always the same? If so, you can
keep that knowlede private to the the driver.

And maybe you can have the bit offset in a reg property here to
avoid adding any custom properties? Something like:

	dsp_reset: reset@1 {
		reg = 1;
	};

If reg is not suitable for that, it seems that some generic property
to describe the bit offset is needed by quite a few drivers
anyways, for things like clocks and regulators. 

> +Client Node Declaration
> +==============================================
> +This is the consumer of the parent node to declare what resets this
> +particular module is interested in.
> +
> +example:
> +	src: src@55082000 {
> +		    resets = <&reset_src phandle>;
> +			reset-names = "<reset_name>";
> +	};
> +
> +Required Properties:
> +reset_src - This is the parent DT entry for the reset controller
> +phandle - This is the phandle of the specific reset to be used by the clien
> +	driver.
> +reset-names - This is the reset name of module that the device driver
> +	needs to be able to reset.  This value must correspond to a value within
> +	the reset controller array.
> +
> +example:
> +resets = <&prm_resets &dsp_mmu_reset>;
> +reset-names = "dsp_mmu_reset";

This part looks OK to me, not sure if we need the reset-names property
if we have one already why not.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] [v2 Patch 2/6] ARM: TI: Describe the ti reset DT entries

[Dan Murphy]

Tony

Thanks for the comments

On 05/05/2014 05:01 PM, Tony Lindgren wrote:
> * Dan Murphy <dmurphy@ti.com> [140505 13:10]:
>> +
>> +Required parent properties:
>> +- compatible : Should be one of,
>> +		"ti,omap4-prm" for OMAP4 PRM instances
>> +		"ti,omap5-prm" for OMAP5 PRM instances
>> +		"ti,dra7-prm" for DRA7xx PRM instances
>> +		"ti,am4-prcm" for AM43xx PRCM instances
>> +		"ti,am3-prcm" for AM33xx PRCM instances
>> +
>> +Required child reset property:
>> +- compatible : Should be
>> +		"resets" for All TI SoCs
>> +
>> +example:
>> +		prm: prm@4ae06000 {
>> +			compatible = "ti,omap5-prm";
>> +			reg = <0x4ae06000 0x3000>;
>> +
>> +			prm_resets: resets {
>> +				#address-cells = <1>;
>> +				#size-cells = <1>;
>> +				#reset-cells = <1>;
>> +			};
>> +		};
> The reg entries you have in the example below has different format
> compared to this?

This is the parent to the resets.  The reg entries below are for
the child reset signals.

>
>> +Reset node declaration
>> +==============================================
>> +The reset node is declared in a parent child relationship.  The main parent
>> +is the PRCM module which contains the base address.  The first child within
>> +the reset parent declares the target modules reset name.  This is followed by
>> +the control and status offsets.
>> +
>> +Within the first reset child node is a secondary child node which declares the
>> +reset signal of interest.  Under this node the control and status bits
>> +are declared.  These bits declare the bit mask for the target reset.
>> +
>> +
>> +Required properties:
>> +reg - This is the register offset from the PRCM parent.
>> +	This must be declared as:
>> +
>> +	reg = <control register offset>,
>> +		  <status register offset>;
>> +
>> +control-bit - This is the bit within the register which controls the reset
>> +	of the target module.  This is declared as a bit mask for the register.
>> +status-bit - This is the bit within the register which contains the status of
>> +	the reset of the target module.
>> +	This is declared as a bit mask for the register.
>> +
>> +example:
>> +&prm_resets {
>> +	dsp_rstctrl {
>> +		reg = <0x1c00>,
>> +			  <0x1c04>;
> Shouldn't this be start and size instead of start and end here?

I could do start and size.  But the size will be the same for each register.
AM33xx really hurts the consistency model here as the other patches in the series
it appears that the control and status are consistent but AM33xx the registers are all over
the place.

I have not looked at OMAP2->4 yet for control and status register offsets.

>> +		dsp_reset: dsp_reset {
>> +			control-bit = <0x01>;
>> +			status-bit = <0x01>;
>> +		};
>> +	};
>> +};
> Are the control-bit and status-bit always the same? If so, you can
> keep that knowlede private to the the driver.

No there is a case in the am33xx resets file where the status and control bits are not the same.

>
> And maybe you can have the bit offset in a reg property here to
> avoid adding any custom properties? Something like:
>
> 	dsp_reset: reset@1 {
> 		reg = 1;
> 	};
>
> If reg is not suitable for that, it seems that some generic property
> to describe the bit offset is needed by quite a few drivers
> anyways, for things like clocks and regulators. 

I will have to look into this.  Maybe a generic entry called bit-offset
that defines the bit or a mask for the registers.  It can mimic the reg
entry.

>> +Client Node Declaration
>> +==============================================
>> +This is the consumer of the parent node to declare what resets this
>> +particular module is interested in.
>> +
>> +example:
>> +	src: src@55082000 {
>> +		    resets = <&reset_src phandle>;
>> +			reset-names = "<reset_name>";
>> +	};
>> +
>> +Required Properties:
>> +reset_src - This is the parent DT entry for the reset controller
>> +phandle - This is the phandle of the specific reset to be used by the clien
>> +	driver.
>> +reset-names - This is the reset name of module that the device driver
>> +	needs to be able to reset.  This value must correspond to a value within
>> +	the reset controller array.
>> +
>> +example:
>> +resets = <&prm_resets &dsp_mmu_reset>;
>> +reset-names = "dsp_mmu_reset";
> This part looks OK to me, not sure if we need the reset-names property
> if we have one already why not.

reset-names are part of the reset.txt file.  I could probably drop the resets and reset-name information here
and point to the reset.txt file for further explanation.

>
> Regards,
>
> Tony


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

Re: [RFC] [v2 Patch 2/6] ARM: TI: Describe the ti reset DT entries

[Suman Anna]

Dan,

On 05/06/2014 08:18 AM, Murphy, Dan wrote:
> Tony
> 
> Thanks for the comments
> 
> On 05/05/2014 05:01 PM, Tony Lindgren wrote:
>> * Dan Murphy <dmurphy@ti.com> [140505 13:10]:
>>> +
>>> +Required parent properties:
>>> +- compatible : Should be one of,
>>> +		"ti,omap4-prm" for OMAP4 PRM instances
>>> +		"ti,omap5-prm" for OMAP5 PRM instances
>>> +		"ti,dra7-prm" for DRA7xx PRM instances
>>> +		"ti,am4-prcm" for AM43xx PRCM instances
>>> +		"ti,am3-prcm" for AM33xx PRCM instances
>>> +
>>> +Required child reset property:
>>> +- compatible : Should be
>>> +		"resets" for All TI SoCs
>>> +
>>> +example:
>>> +		prm: prm@4ae06000 {
>>> +			compatible = "ti,omap5-prm";
>>> +			reg = <0x4ae06000 0x3000>;
>>> +
>>> +			prm_resets: resets {
>>> +				#address-cells = <1>;
>>> +				#size-cells = <1>;
>>> +				#reset-cells = <1>;
>>> +			};
>>> +		};
>> The reg entries you have in the example below has different format
>> compared to this?

Right, the #size-cells should have been 0, or each reg field updated
with an additional field value of 4 as the registers are all of constant
width - 4 bytes.

> 
> This is the parent to the resets.  The reg entries below are for
> the child reset signals.
> 
>>
>>> +Reset node declaration
>>> +==============================================
>>> +The reset node is declared in a parent child relationship.  The main parent
>>> +is the PRCM module which contains the base address.  The first child within
>>> +the reset parent declares the target modules reset name.  This is followed by
>>> +the control and status offsets.
>>> +
>>> +Within the first reset child node is a secondary child node which declares the
>>> +reset signal of interest.  Under this node the control and status bits
>>> +are declared.  These bits declare the bit mask for the target reset.
>>> +
>>> +
>>> +Required properties:
>>> +reg - This is the register offset from the PRCM parent.
>>> +	This must be declared as:
>>> +
>>> +	reg = <control register offset>,
>>> +		  <status register offset>;
>>> +
>>> +control-bit - This is the bit within the register which controls the reset
>>> +	of the target module.  This is declared as a bit mask for the register.
>>> +status-bit - This is the bit within the register which contains the status of
>>> +	the reset of the target module.
>>> +	This is declared as a bit mask for the register.
>>> +
>>> +example:
>>> +&prm_resets {
>>> +	dsp_rstctrl {
>>> +		reg = <0x1c00>,
>>> +			  <0x1c04>;
>> Shouldn't this be start and size instead of start and end here?

These are two registers - one for control and one for status.

> 
> I could do start and size.  But the size will be the same for each register.
> AM33xx really hurts the consistency model here as the other patches in the series
> it appears that the control and status are consistent but AM33xx the registers are all over
> the place.
> 
> I have not looked at OMAP2->4 yet for control and status register offsets.
> 
>>> +		dsp_reset: dsp_reset {
>>> +			control-bit = <0x01>;
>>> +			status-bit = <0x01>;
>>> +		};
>>> +	};
>>> +};
>> Are the control-bit and status-bit always the same? If so, you can
>> keep that knowlede private to the the driver.
> 
> No there is a case in the am33xx resets file where the status and control bits are not the same.

Do we really need two separate properties to represent this, as these
are essentially the relevant bits in the corresponding reg elements.
I guess this is somewhat same as Tony's suggestion about using the reg
field for these sub-nodes, in which case, you would have to add another
#address-cells as 1 for each of the child reset nodes.

regards
Suman

> 
>>
>> And maybe you can have the bit offset in a reg property here to
>> avoid adding any custom properties? Something like:
>>
>> 	dsp_reset: reset@1 {
>> 		reg = 1;
>> 	};
>>
>> If reg is not suitable for that, it seems that some generic property
>> to describe the bit offset is needed by quite a few drivers
>> anyways, for things like clocks and regulators.
> 
> I will have to look into this.  Maybe a generic entry called bit-offset
> that defines the bit or a mask for the registers.  It can mimic the reg
> entry.
> 
>>> +Client Node Declaration
>>> +==============================================
>>> +This is the consumer of the parent node to declare what resets this
>>> +particular module is interested in.
>>> +
>>> +example:
>>> +	src: src@55082000 {
>>> +		    resets = <&reset_src phandle>;
>>> +			reset-names = "<reset_name>";
>>> +	};
>>> +
>>> +Required Properties:
>>> +reset_src - This is the parent DT entry for the reset controller
>>> +phandle - This is the phandle of the specific reset to be used by the clien
>>> +	driver.
>>> +reset-names - This is the reset name of module that the device driver
>>> +	needs to be able to reset.  This value must correspond to a value within
>>> +	the reset controller array.
>>> +
>>> +example:
>>> +resets = <&prm_resets &dsp_mmu_reset>;
>>> +reset-names = "dsp_mmu_reset";
>> This part looks OK to me, not sure if we need the reset-names property
>> if we have one already why not.
> 
> reset-names are part of the reset.txt file.  I could probably drop the resets and reset-name information here
> and point to the reset.txt file for further explanation.
> 
>>
>> Regards,
>>
>> Tony
> 
> 

[RFC] [v2 Patch 3/6] ARM: dts: am33xx: Add prcm_resets node

[Dan Murphy]

Add the prcm_resets node to the prcm parent node.

Add the am33xx_resets file to define the
am33xx reset lines that are handled by this reset
framework.

Signed-off-by: Dan Murphy <dmurphy-l0cyMroinI0@public.gmane.org>
---
 arch/arm/boot/dts/am33xx-resets.dtsi |   42 ++++++++++++++++++++++++++++++++++
 arch/arm/boot/dts/am33xx.dtsi        |    7 ++++++
 2 files changed, 49 insertions(+)
 create mode 100644 arch/arm/boot/dts/am33xx-resets.dtsi

diff --git a/arch/arm/boot/dts/am33xx-resets.dtsi b/arch/arm/boot/dts/am33xx-resets.dtsi
new file mode 100644
index 0000000..9260626
--- /dev/null
+++ b/arch/arm/boot/dts/am33xx-resets.dtsi
@@ -0,0 +1,42 @@
+/*
+ * Device Tree Source for AM33XX reset data
+ *
+ * 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.
+ */
+
+&prcm_resets {
+	gfx_rstctrl {
+		reg = <0x1104>,
+			  <0x1114>;
+
+		gfx_reset: gfx_reset {
+			control-bit = <0x01>;
+			status-bit = <0x01>;
+		};
+	};
+
+	per_rstctrl {
+		reg = <0xD00>,
+			  <0xD0C>;
+
+		iva_reset: iva_reset {
+			control-bit = <0x04>;
+			status-bit = <0x10>;
+		};
+	};
+
+	device_rstctrl {
+		reg = <0xf00>,
+			  <0xf08>;
+
+		device_reset: device_reset {
+			control-bit = <0x01>;
+			status-bit = <0x01>;
+		};
+	};
+
+};
diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index cb6811e..6b1a6df 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -117,6 +117,12 @@
 
 			prcm_clockdomains: clockdomains {
 			};
+
+			prcm_resets: resets {
+				#address-cells = <1>;
+				#size-cells = <1>;
+				#reset-cells = <1>;
+			};
 		};
 
 		scrm: scrm@44e10000 {
@@ -833,3 +839,4 @@
 };
 
 /include/ "am33xx-clocks.dtsi"
+/include/ "am33xx-resets.dtsi"
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC] [v2 Patch 5/6] ARM: dts: dra7: Add prm_resets node

[Dan Murphy]

Add the prcm_resets node to the prm parent node.

Add the draxx_resets file to define the
dra7xx reset lines that are handled by this reset
framework.

Signed-off-by: Dan Murphy <dmurphy-l0cyMroinI0@public.gmane.org>
---
 arch/arm/boot/dts/dra7.dtsi          |    7 +++
 arch/arm/boot/dts/dra7xx-resets.dtsi |   82 ++++++++++++++++++++++++++++++++++
 2 files changed, 89 insertions(+)
 create mode 100644 arch/arm/boot/dts/dra7xx-resets.dtsi

diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
index 149b550..c008996 100644
--- a/arch/arm/boot/dts/dra7.dtsi
+++ b/arch/arm/boot/dts/dra7.dtsi
@@ -120,6 +120,12 @@
 
 			prm_clockdomains: clockdomains {
 			};
+
+			prm_resets: resets {
+				#address-cells = <1>;
+				#size-cells = <1>;
+				#reset-cells = <1>;
+			};
 		};
 
 		cm_core_aon: cm_core_aon@4a005000 {
@@ -793,3 +799,4 @@
 };
 
 /include/ "dra7xx-clocks.dtsi"
+/include/ "dra7xx-resets.dtsi"
diff --git a/arch/arm/boot/dts/dra7xx-resets.dtsi b/arch/arm/boot/dts/dra7xx-resets.dtsi
new file mode 100644
index 0000000..4c4966d
--- /dev/null
+++ b/arch/arm/boot/dts/dra7xx-resets.dtsi
@@ -0,0 +1,82 @@
+/*
+ * Device Tree Source for DRA7XX reset data
+ *
+ * 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.
+ */
+
+&prm_resets {
+	dsp_rstctrl {
+		reg = <0x410>,
+			  <0x414>;
+
+		dsp_reset: dsp_reset {
+			control-bit = <0x01>;
+			status-bit = <0x01>;
+		};
+
+		dsp_mmu_reset: dsp_mmu_reset {
+			control-bit = <0x02>;
+			status-bit = <0x02>;
+		};
+	};
+
+	ipu_rstctrl {
+		reg = <0x510>,
+			  <0x514>;
+
+		ipu_cpu0_reset: ipu_cpu0_reset {
+			control-bit = <0x01>;
+			status-bit = <0x01>;
+		};
+
+		ipu_cpu1_reset: ipu_cpu1_reset {
+			control-bit = <0x02>;
+			status-bit = <0x02>;
+		};
+
+		ipu_mmu_reset: ipu_mmu_reset {
+			control-bit = <0x04>;
+			status-bit = <0x04>;
+		};
+	};
+
+	iva_rstctrl {
+		reg = <0xf10>,
+			  <0xf14>;
+
+		iva_reset: iva_reset {
+			control-bit = <0x01>;
+			status-bit = <0x01>;
+		};
+	};
+
+	pcie_rstctrl {
+		reg = <0x1310>,
+			  <0x1314>;
+
+		pcie1_reset: pcie1_reset {
+			control-bit = <0x01>;
+			status-bit = <0x01>;
+		};
+
+		pcie2_reset: pcie2_reset {
+			control-bit = <0x01>;
+			status-bit = <0x01>;
+		};
+	};
+
+	device_rstctrl {
+		reg = <0x1D00>,
+			  <0x1D04>;
+
+		device_reset: device_reset {
+			control-bit = <0x01>;
+			status-bit = <0x01>;
+		};
+	};
+
+};
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC] [v2 Patch 4/6] ARM: dts: am4372: Add prcm_resets node

[Dan Murphy]

Add the prcm_resets node to the prcm parent node.

Add the am34xx_resets file to define the
am34xx reset lines that are handled by this reset
framework.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 arch/arm/boot/dts/am4372.dtsi        |    7 +++++
 arch/arm/boot/dts/am43xx-resets.dtsi |   52 ++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+)
 create mode 100644 arch/arm/boot/dts/am43xx-resets.dtsi

diff --git a/arch/arm/boot/dts/am4372.dtsi b/arch/arm/boot/dts/am4372.dtsi
index d1f8707..e1ba7ed 100644
--- a/arch/arm/boot/dts/am4372.dtsi
+++ b/arch/arm/boot/dts/am4372.dtsi
@@ -84,6 +84,12 @@
 
 			prcm_clockdomains: clockdomains {
 			};
+
+			prcm_resets: resets {
+				#address-cells = <1>;
+				#size-cells = <1>;
+				#reset-cells = <1>;
+			};
 		};
 
 		scrm: scrm@44e10000 {
@@ -739,3 +745,4 @@
 };
 
 /include/ "am43xx-clocks.dtsi"
+/include/ "am43xx-resets.dtsi"
diff --git a/arch/arm/boot/dts/am43xx-resets.dtsi b/arch/arm/boot/dts/am43xx-resets.dtsi
new file mode 100644
index 0000000..ef338ba
--- /dev/null
+++ b/arch/arm/boot/dts/am43xx-resets.dtsi
@@ -0,0 +1,52 @@
+/*
+ * Device Tree Source for AM43XX reset data
+ *
+ * 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.
+ */
+
+&prcm_resets {
+	icss_rstctrl {
+		reg = <0x810>,
+			  <0x814>;
+
+		icss_reset: icss_reset {
+			control-bit = <0x01>;
+			status-bit = <0x01>;
+		};
+	};
+
+	gfx_rstctrl {
+		reg = <0x410>,
+			  <0x414>;
+
+		gfx_reset: gfx_reset {
+			control-bit = <0x01>;
+			status-bit = <0x01>;
+		};
+	};
+
+	per_rstctrl {
+		reg = <0x2010>,
+			  <0x2014>;
+
+		iva_reset: iva_reset {
+			control-bit = <0x01>;
+			status-bit = <0x01>;
+		};
+	};
+
+	device_rstctrl {
+		reg = <0x4000>,
+			  <0x4004>;
+
+		device_reset: device_reset {
+			control-bit = <0x01>;
+			status-bit = <0x01>;
+		};
+	};
+
+};
-- 
1.7.9.5

[RFC] [v2 Patch 6/6] ARM: dts: omap5: Add prm_resets node

[Dan Murphy]

Add the prm_resets node to the prm parent node.

Add the omap54xx_resets file to define the
omap5 reset lines that are handled by this reset
framework.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 arch/arm/boot/dts/omap5.dtsi           |    7 ++++
 arch/arm/boot/dts/omap54xx-resets.dtsi |   66 ++++++++++++++++++++++++++++++++
 2 files changed, 73 insertions(+)
 create mode 100644 arch/arm/boot/dts/omap54xx-resets.dtsi

diff --git a/arch/arm/boot/dts/omap5.dtsi b/arch/arm/boot/dts/omap5.dtsi
index f8c9855..b6e3c4c 100644
--- a/arch/arm/boot/dts/omap5.dtsi
+++ b/arch/arm/boot/dts/omap5.dtsi
@@ -134,6 +134,12 @@
 
 			prm_clockdomains: clockdomains {
 			};
+
+			prm_resets: resets {
+				#address-cells = <1>;
+				#size-cells = <1>;
+				#reset-cells = <1>;
+			};
 		};
 
 		cm_core_aon: cm_core_aon@4a004000 {
@@ -873,3 +879,4 @@
 };
 
 /include/ "omap54xx-clocks.dtsi"
+/include/ "omap54xx-resets.dtsi"
diff --git a/arch/arm/boot/dts/omap54xx-resets.dtsi b/arch/arm/boot/dts/omap54xx-resets.dtsi
new file mode 100644
index 0000000..cba6f52
--- /dev/null
+++ b/arch/arm/boot/dts/omap54xx-resets.dtsi
@@ -0,0 +1,66 @@
+/*
+ * Device Tree Source for OMAP5 reset data
+ *
+ * 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.
+ */
+
+&prm_resets {
+	dsp_rstctrl {
+		reg = <0x1c00>,
+			  <0x1c04>;
+
+		dsp_reset: dsp_reset {
+			control-bit = <0x01>;
+			status-bit = <0x01>;
+		};
+
+		dsp_mmu_reset: dsp_mmu_reset {
+			control-bit = <0x02>;
+			status-bit = <0x02>;
+		};
+	};
+
+	ipu_rstctrl {
+		reg = <0x910>,
+			  <0x914>;
+
+		ipu_cpu0_reset: ipu_cpu0_reset {
+			control-bit = <0x01>;
+			status-bit = <0x01>;
+		};
+
+		ipu_cpu1_reset: ipu_cpu1_reset {
+			control-bit = <0x02>;
+			status-bit = <0x02>;
+		};
+
+		ipu_mmu_reset: ipu_mmu_reset {
+			control-bit = <0x04>;
+			status-bit = <0x04>;
+		};
+	};
+
+	iva_rstctrl {
+		reg = <0x1210>,
+			  <0x1214>;
+
+		iva_reset: iva_reset {
+			control-bit = <0x01>;
+			status-bit = <0x01>;
+		};
+	};
+
+	device_rstctrl {
+		reg = <0x1c00>,
+			  <0x1c04>;
+
+		device_reset: device_reset {
+			control-bit = <0x01>;
+			status-bit = <0x01>;
+		};
+	};
+};
-- 
1.7.9.5