[U-Boot] [PATCH 1/2] Add a reset driver framework/uclass

[U-Boot] [PATCH 1/2] Add a reset driver framework/uclass

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

A reset controller is a hardware module that controls reset signals that
affect other hardware modules or chips.

This patch defines a standard API that connects reset clients (i.e. the
drivers for devices affected by reset signals) to drivers for reset
controllers/providers. Initially, DT is the only supported method for
connecting the two.

The DT binding specification (reset.txt) was taken from Linux kernel
v4.5's Documentation/devicetree/bindings/reset/reset.txt.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
This series depends on my recent mailbox series, solely for a trivial
amount of diff context.

This series will eventually be a dependency for the Tegra186 clock/reset
driver implementation, but that isn't ready yet.

 doc/device-tree-bindings/reset/reset.txt |  75 +++++++++++++++++
 drivers/Kconfig                          |   2 +
 drivers/Makefile                         |   1 +
 drivers/reset/Kconfig                    |  15 ++++
 drivers/reset/Makefile                   |   5 ++
 drivers/reset/reset-uclass.c             | 131 ++++++++++++++++++++++++++++++
 include/dm/uclass-id.h                   |   1 +
 include/reset_client.h                   | 135 +++++++++++++++++++++++++++++++
 include/reset_uclass.h                   |  81 +++++++++++++++++++
 9 files changed, 446 insertions(+)
 create mode 100644 doc/device-tree-bindings/reset/reset.txt
 create mode 100644 drivers/reset/Kconfig
 create mode 100644 drivers/reset/Makefile
 create mode 100644 drivers/reset/reset-uclass.c
 create mode 100644 include/reset_client.h
 create mode 100644 include/reset_uclass.h

diff --git a/doc/device-tree-bindings/reset/reset.txt b/doc/device-tree-bindings/reset/reset.txt
new file mode 100644
index 000000000000..31db6ff84908
--- /dev/null
+++ b/doc/device-tree-bindings/reset/reset.txt
@@ -0,0 +1,75 @@
+= Reset Signal Device Tree Bindings =
+
+This binding is intended to represent the hardware reset signals present
+internally in most IC (SoC, FPGA, ...) designs. Reset signals for whole
+standalone chips are most likely better represented as GPIOs, although there
+are likely to be exceptions to this rule.
+
+Hardware blocks typically receive a reset signal. This signal is generated by
+a reset provider (e.g. power management or clock module) and received by a
+reset consumer (the module being reset, or a module managing when a sub-
+ordinate module is reset). This binding exists to represent the provider and
+consumer, and provide a way to couple the two together.
+
+A reset signal is represented by the phandle of the provider, plus a reset
+specifier - a list of DT cells that represents the reset signal within the
+provider. The length (number of cells) and semantics of the reset specifier
+are dictated by the binding of the reset provider, although common schemes
+are described below.
+
+A word on where to place reset signal consumers in device tree: It is possible
+in hardware for a reset signal to affect multiple logically separate HW blocks
+at once. In this case, it would be unwise to represent this reset signal in
+the DT node of each affected HW block, since if activated, an unrelated block
+may be reset. Instead, reset signals should be represented in the DT node
+where it makes most sense to control it; this may be a bus node if all
+children of the bus are affected by the reset signal, or an individual HW
+block node for dedicated reset signals. The intent of this binding is to give
+appropriate software access to the reset signals in order to manage the HW,
+rather than to slavishly enumerate the reset signal that affects each HW
+block.
+
+= Reset providers =
+
+Required properties:
+#reset-cells:	Number of cells in a reset specifier; Typically 0 for nodes
+		with a single reset output and 1 for nodes with multiple
+		reset outputs.
+
+For example:
+
+	rst: reset-controller {
+		#reset-cells = <1>;
+	};
+
+= Reset consumers =
+
+Required properties:
+resets:		List of phandle and reset specifier pairs, one pair
+		for each reset signal that affects the device, or that the
+		device manages. Note: if the reset provider specifies '0' for
+		#reset-cells, then only the phandle portion of the pair will
+		appear.
+
+Optional properties:
+reset-names:	List of reset signal name strings sorted in the same order as
+		the resets property. Consumers drivers will use reset-names to
+		match reset signal names with reset specifiers.
+
+For example:
+
+	device {
+		resets = <&rst 20>;
+		reset-names = "reset";
+	};
+
+This represents a device with a single reset signal named "reset".
+
+	bus {
+		resets = <&rst 10> <&rst 11> <&rst 12> <&rst 11>;
+		reset-names = "i2s1", "i2s2", "dma", "mixer";
+	};
+
+This represents a bus that controls the reset signal of each of four sub-
+ordinate devices. Consider for example a bus that fails to operate unless no
+child device has reset asserted.
diff --git a/drivers/Kconfig b/drivers/Kconfig
index f2a137ad87fc..f6003a0a593a 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -56,6 +56,8 @@ source "drivers/ram/Kconfig"
 
 source "drivers/remoteproc/Kconfig"
 
+source "drivers/reset/Kconfig"
+
 source "drivers/rtc/Kconfig"
 
 source "drivers/serial/Kconfig"
diff --git a/drivers/Makefile b/drivers/Makefile
index 47fddff084b5..1634efc5fc98 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -65,6 +65,7 @@ obj-$(CONFIG_U_QE) += qe/
 obj-y += mailbox/
 obj-y += memory/
 obj-y += pwm/
+obj-y += reset/
 obj-y += input/
 # SOC specific infrastructure drivers.
 obj-y += soc/
diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
new file mode 100644
index 000000000000..5c449a99d39d
--- /dev/null
+++ b/drivers/reset/Kconfig
@@ -0,0 +1,15 @@
+menu "Reset Controller Support"
+
+config DM_RESET
+	bool "Enable reset controllers using Driver Model"
+	depends on DM && OF_CONTROL
+	help
+	  Enable support for the reset controller driver class. Many hardware
+	  modules are equipped with a reset signal, typically driven by some
+	  reset controller hardware module within the chip. In U-Boot, reset
+	  controller drivers allow control over these reset signals. In some
+	  cases this API is applicable to chips outside the CPU as well,
+	  although driving such reset isgnals using GPIOs may be more
+	  appropriate in this case.
+
+endmenu
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
new file mode 100644
index 000000000000..508608e83f87
--- /dev/null
+++ b/drivers/reset/Makefile
@@ -0,0 +1,5 @@
+# Copyright (c) 2016, NVIDIA CORPORATION.
+#
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_DM_RESET) += reset-uclass.o
diff --git a/drivers/reset/reset-uclass.c b/drivers/reset/reset-uclass.c
new file mode 100644
index 000000000000..ac0614eb9ab9
--- /dev/null
+++ b/drivers/reset/reset-uclass.c
@@ -0,0 +1,131 @@
+/*
+ * Copyright (c) 2016, NVIDIA CORPORATION.
+ *
+ * SPDX-License-Identifier: GPL-2.0
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <fdtdec.h>
+#include <reset_client.h>
+#include <reset_uclass.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+static inline struct reset_ops *reset_dev_ops(struct udevice *dev)
+{
+	return (struct reset_ops *)dev->driver->ops;
+}
+
+static int reset_of_xlate_default(struct reset_ctl *reset_ctl,
+				  struct fdtdec_phandle_args *args)
+{
+	debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
+
+	if (args->args_count != 1) {
+		debug("Invaild args_count: %d\n", args->args_count);
+		return -EINVAL;
+	}
+
+	reset_ctl->id = args->args[0];
+
+	return 0;
+}
+
+int reset_get_by_index(struct udevice *dev, int index,
+		       struct reset_ctl *reset_ctl)
+{
+	struct fdtdec_phandle_args args;
+	int ret;
+	struct udevice *dev_reset;
+	struct reset_ops *ops;
+
+	debug("%s(dev=%p, index=%d, reset_ctl=%p)\n", __func__, dev, index,
+	      reset_ctl);
+
+	ret = fdtdec_parse_phandle_with_args(gd->fdt_blob, dev->of_offset,
+					     "resets", "#reset-cells", 0,
+					     index, &args);
+	if (ret) {
+		debug("%s: fdtdec_parse_phandle_with_args failed: %d\n",
+		      __func__, ret);
+		return ret;
+	}
+
+	ret = uclass_get_device_by_of_offset(UCLASS_RESET, args.node,
+					     &dev_reset);
+	if (ret) {
+		debug("%s: uclass_get_device_by_of_offset failed: %d\n",
+		      __func__, ret);
+		return ret;
+	}
+	ops = reset_dev_ops(dev_reset);
+
+	reset_ctl->dev = dev_reset;
+	if (ops->of_xlate)
+		ret = ops->of_xlate(reset_ctl, &args);
+	else
+		ret = reset_of_xlate_default(reset_ctl, &args);
+	if (ret) {
+		debug("of_xlate() failed: %d\n", ret);
+		return ret;
+	}
+
+	ret = ops->request(reset_ctl);
+	if (ret) {
+		debug("ops->request() failed: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+int reset_get_by_name(struct udevice *dev, const char *name,
+		     struct reset_ctl *reset_ctl)
+{
+	int index;
+
+	debug("%s(dev=%p, name=%s, reset_ctl=%p)\n", __func__, dev, name,
+	      reset_ctl);
+
+	index = fdt_find_string(gd->fdt_blob, dev->of_offset, "reset-names",
+				name);
+	if (index < 0) {
+		debug("fdt_find_string() failed: %d\n", index);
+		return index;
+	}
+
+	return reset_get_by_index(dev, index, reset_ctl);
+}
+
+int reset_free(struct reset_ctl *reset_ctl)
+{
+	struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
+
+	debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
+
+	return ops->free(reset_ctl);
+}
+
+int reset_assert(struct reset_ctl *reset_ctl)
+{
+	struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
+
+	debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
+
+	return ops->rst_assert(reset_ctl);
+}
+
+int reset_deassert(struct reset_ctl *reset_ctl)
+{
+	struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
+
+	debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
+
+	return ops->rst_deassert(reset_ctl);
+}
+
+UCLASS_DRIVER(reset) = {
+	.id		= UCLASS_RESET,
+	.name		= "reset",
+};
diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
index 4e252ae210f7..e31f699cdb65 100644
--- a/include/dm/uclass-id.h
+++ b/include/dm/uclass-id.h
@@ -63,6 +63,7 @@ enum uclass_id {
 	UCLASS_PWRSEQ,		/* Power sequence device */
 	UCLASS_REGULATOR,	/* Regulator device */
 	UCLASS_REMOTEPROC,	/* Remote Processor device */
+	UCLASS_RESET,		/* Reset controller device */
 	UCLASS_RTC,		/* Real time clock device */
 	UCLASS_SERIAL,		/* Serial UART */
 	UCLASS_SPI,		/* SPI bus */
diff --git a/include/reset_client.h b/include/reset_client.h
new file mode 100644
index 000000000000..3bdc8256853a
--- /dev/null
+++ b/include/reset_client.h
@@ -0,0 +1,135 @@
+/*
+ * Copyright (c) 2016, NVIDIA CORPORATION.
+ *
+ * SPDX-License-Identifier: GPL-2.0
+ */
+
+#ifndef _RESET_CLIENT_H
+#define _RESET_CLIENT_H
+
+/**
+ * A reset is a hardware signal indicating that a HW module (or IP block, or
+ * sometimes an entire off-CPU chip) reset all of its internal state to some
+ * known-good initial state. Drivers will often reset HW modules when they
+ * begin execution to ensure that hardware correctly responds to all requests,
+ * or in response to some error condition. Reset signals are often controlled
+ * externally to the HW module being reset, by an entity this API calls a reset
+ * controller. This API provides a standard means for drivers to request that
+ * reset controllers set or clear reset signals.
+ *
+ * A driver that implements UCLASS_RESET is a reset controller or provider. A
+ * controller will often implement multiple separate reset signals, since the
+ * hardware it manages often has this capability. reset_uclass.h describes the
+ * interface which reset controllers must implement.
+ *
+ * Reset consumers/clients are the HW modules affected by reset signals. This
+ * header file describes the API used by drivers for those HW modules.
+ */
+
+struct udevice;
+
+/**
+ * struct reset_ctl - A handle to (allowing control of) a single reset signal.
+ *
+ * Clients provide storage for reset control handles. The content of the
+ * structure is managed solely by the reset API and reset drivers. A reset
+ * control struct is initialized by "get"ing the reset control struct. The
+ * reset control struct is passed to all other reset APIs to identify which
+ * reset signal to operate upon.
+ *
+ * @dev: The device which implements the reset signal.
+ * @id: The reset signal ID within the provider.
+ *
+ * Currently, the reset API assumes that a single integer ID is enough to
+ * identify and configure any reset signal for any reset provider. If this
+ * assumption becomes invalid in the future, the struct could be expanded to
+ * either (a) add more fields to allow reset providers to store additional
+ * information, or (b) replace the id field with an opaque pointer, which the
+ * provider would dynamically allocated during its .of_xlate op, and process
+ * during is .request op. This may require the addition of an extra op to clean
+ * up the allocation.
+ */
+struct reset_ctl {
+	struct udevice *dev;
+	/*
+	 * Written by of_xlate. We assume a single id is enough for now. In the
+	 * future, we might add more fields here.
+	 */
+	unsigned long id;
+};
+
+/**
+ * reset_get_by_index - Get/request a reset signal by integer index.
+ *
+ * This looks up and requests a reset signal. The index is relative to the
+ * client device; each device is assumed to have n reset signals associated
+ * with it somehow, and this function finds and requests one of them. The
+ * mapping of client device reset signal indices to provider reset signals may
+ * be via device-tree properties, board-provided mapping tables, or some other
+ * mechanism.
+ *
+ * @dev:	The client device.
+ * @index:	The index of the reset signal to request, within the client's
+ *		list of reset signals.
+ * @reset_ctl	A pointer to a reset control struct to initialize.
+ * @return 0 if OK, or a negative error code.
+ */
+int reset_get_by_index(struct udevice *dev, int index,
+		       struct reset_ctl *reset_ctl);
+
+/**
+ * reset_get_by_name - Get/request a reset signal by name.
+ *
+ * This looks up and requests a reset signal. The name is relative to the
+ * client device; each device is assumed to have n reset signals associated
+ * with it somehow, and this function finds and requests one of them. The
+ * mapping of client device reset signal names to provider reset signal may be
+ * via device-tree properties, board-provided mapping tables, or some other
+ * mechanism.
+ *
+ * @dev:	The client device.
+ * @name:	The name of the reset signal to request, within the client's
+ *		list of reset signals.
+ * @reset_ctl:	A pointer to a reset control struct to initialize.
+ * @return 0 if OK, or a negative error code.
+ */
+int reset_get_by_name(struct udevice *dev, const char *name,
+		      struct reset_ctl *reset_ctl);
+
+/**
+ * reset_free - Free a previously requested reset signal.
+ *
+ * @reset_ctl:	A reset control struct that was previously successfully
+ *		requested by reset_get_by_*().
+ * @return 0 if OK, or a negative error code.
+ */
+int reset_free(struct reset_ctl *reset_ctl);
+
+/**
+ * reset_assert - Assert a reset signal.
+ *
+ * This function will assert the specified reset signal, thus resetting the
+ * affected HW module(s). Depending on the reset controller hardware, the reset
+ * signal will either stay asserted until reset_deassert() is called, or the
+ * hardware may autonomously clear the reset signal itself.
+ *
+ * @reset_ctl:	A reset control struct that was previously successfully
+ *		requested by reset_get_by_*().
+ * @return 0 if OK, or a negative error code.
+ */
+int reset_assert(struct reset_ctl *reset_ctl);
+
+/**
+ * reset_deassert - Deassert a reset signal.
+ *
+ * This function will deassert the specified reset signal, thus releasing the
+ * affected HW modules() from reset, and allowing them to continue normal
+ * operation.
+ *
+ * @reset_ctl:	A reset control struct that was previously successfully
+ *		requested by reset_get_by_*().
+ * @return 0 if OK, or a negative error code.
+ */
+int reset_deassert(struct reset_ctl *reset_ctl);
+
+#endif
diff --git a/include/reset_uclass.h b/include/reset_uclass.h
new file mode 100644
index 000000000000..6e27a7bbd1cf
--- /dev/null
+++ b/include/reset_uclass.h
@@ -0,0 +1,81 @@
+/*
+ * Copyright (c) 2016, NVIDIA CORPORATION.
+ *
+ * SPDX-License-Identifier: GPL-2.0
+ */
+
+#ifndef _RESET_UCLASS_H
+#define _RESET_UCLASS_H
+
+/* See reset_client.h for background documentation. */
+
+#include <reset_client.h>
+
+struct udevice;
+
+/**
+ * struct reset_ops - The functions that a reset controller driver must
+ * implement.
+ */
+struct reset_ops {
+	/**
+	 * of_xlate - Translate a client's device-tree (OF) reset specifier.
+	 *
+	 * The reset core calls this function as the first step in implementing
+	 * a client's reset_get_by_*() call.
+	 *
+	 * If this function pointer is set to NULL, the reset core will use a
+	 * default implementation, which assumes #reset-cells = <1>, and that
+	 * the DT cell contains a simple integer reset signal ID.
+	 *
+	 * At present, the reset API solely supports device-tree. If this
+	 * changes, other xxx_xlate() functions may be added to support those
+	 * other mechanisms.
+	 *
+	 * @reset_ctl:	The reset control struct to hold the translation result.
+	 * @args:	The reset specifier values from device tree.
+	 * @return 0 if OK, or a negative error code.
+	 */
+	int (*of_xlate)(struct reset_ctl *reset_ctl,
+			struct fdtdec_phandle_args *args);
+	/**
+	 * request - Request a translated reset control.
+	 *
+	 * The reset core calls this function as the second step in
+	 * implementing a client's reset_get_by_*() call, following a
+	 * successful xxx_xlate() call.
+	 *
+	 * @reset_ctl:	The reset control struct to request; this has been
+	 *		filled in by a previoux xxx_xlate() function call.
+	 * @return 0 if OK, or a negative error code.
+	 */
+	int (*request)(struct reset_ctl *reset_ctl);
+	/**
+	 * free - Free a previously requested reset control.
+	 *
+	 * This is the implementation of the client reset_free() API.
+	 *
+	 * @reset_ctl:	The reset control to free.
+	 * @return 0 if OK, or a negative error code.
+	 */
+	int (*free)(struct reset_ctl *reset_ctl);
+	/**
+	 * rst_assert - Assert a reset signal.
+	 *
+	 * Note: This function is named rst_assert not assert to avoid
+	 * conflicting with global macro assert().
+	 *
+	 * @reset_ctl:	The reset signal to assert.
+	 * @return 0 if OK, or a negative error code.
+	 */
+	int (*rst_assert)(struct reset_ctl *reset_ctl);
+	/**
+	 * rst_deassert - Deassert a reset signal.
+	 *
+	 * @reset_ctl:	The reset signal to deassert.
+	 * @return 0 if OK, or a negative error code.
+	 */
+	int (*rst_deassert)(struct reset_ctl *reset_ctl);
+};
+
+#endif
-- 
2.8.2

[U-Boot] [PATCH 2/2] reset: implement a reset test

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

This adds a sandbox reset implementation (provider), a test client
device, instantiates them both from Sandbox's DT, and adds a DM test
that excercises everything.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 arch/sandbox/dts/test.dts          |  11 ++++
 arch/sandbox/include/asm/reset.h   |  21 ++++++++
 configs/sandbox_defconfig          |   2 +
 drivers/reset/Kconfig              |   8 +++
 drivers/reset/Makefile             |   2 +
 drivers/reset/sandbox-reset-test.c |  55 +++++++++++++++++++
 drivers/reset/sandbox-reset.c      | 108 +++++++++++++++++++++++++++++++++++++
 test/dm/Makefile                   |   1 +
 test/dm/reset.c                    |  39 ++++++++++++++
 9 files changed, 247 insertions(+)
 create mode 100644 arch/sandbox/include/asm/reset.h
 create mode 100644 drivers/reset/sandbox-reset-test.c
 create mode 100644 drivers/reset/sandbox-reset.c
 create mode 100644 test/dm/reset.c

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index 686c215aea72..879b30e3ccc6 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -259,6 +259,17 @@
 		compatible = "sandbox,reset";
 	};
 
+	resetc: reset-ctl {
+		compatible = "sandbox,reset-ctl";
+		#reset-cells = <1>;
+	};
+
+	reset-ctl-test {
+		compatible = "sandbox,reset-ctl-test";
+		resets = <&resetc 100>, <&resetc 2>;
+		reset-names = "other", "test";
+	};
+
 	rproc_1: rproc at 1 {
 		compatible = "sandbox,test-processor";
 		remoteproc-name = "remoteproc-test-dev1";
diff --git a/arch/sandbox/include/asm/reset.h b/arch/sandbox/include/asm/reset.h
new file mode 100644
index 000000000000..7146aa5ab270
--- /dev/null
+++ b/arch/sandbox/include/asm/reset.h
@@ -0,0 +1,21 @@
+/*
+ * Copyright (c) 2016, NVIDIA CORPORATION.
+ *
+ * SPDX-License-Identifier: GPL-2.0
+ */
+
+#ifndef __SANDBOX_RESET_H
+#define __SANDBOX_RESET_H
+
+#include <common.h>
+
+struct udevice;
+
+int sandbox_reset_query(struct udevice *dev, unsigned long id);
+
+int sandbox_reset_test_get(struct udevice *dev);
+int sandbox_reset_test_assert(struct udevice *dev);
+int sandbox_reset_test_deassert(struct udevice *dev);
+int sandbox_reset_test_free(struct udevice *dev);
+
+#endif
diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
index 8613da2c15f5..9562a4ff73a9 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -170,3 +170,5 @@ CONFIG_UT_ENV=y
 CONFIG_MISC=y
 CONFIG_DM_MAILBOX=y
 CONFIG_SANDBOX_MBOX=y
+CONFIG_DM_RESET=y
+CONFIG_SANDBOX_RESET=y
diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index 5c449a99d39d..0fe8cc3827f1 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -12,4 +12,12 @@ config DM_RESET
 	  although driving such reset isgnals using GPIOs may be more
 	  appropriate in this case.
 
+config SANDBOX_RESET
+	bool "Enable the sandbox reset test driver"
+	depends on DM_MAILBOX && SANDBOX
+	help
+	  Enable support for a test reset controller implementation, which
+	  simply accepts requests to reset various HW modules without actually
+	  doing anything beyond a little error checking.
+
 endmenu
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index 508608e83f87..71f3b219613e 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -3,3 +3,5 @@
 # SPDX-License-Identifier: GPL-2.0
 
 obj-$(CONFIG_DM_RESET) += reset-uclass.o
+obj-$(CONFIG_SANDBOX_MBOX) += sandbox-reset.o
+obj-$(CONFIG_SANDBOX_MBOX) += sandbox-reset-test.o
diff --git a/drivers/reset/sandbox-reset-test.c b/drivers/reset/sandbox-reset-test.c
new file mode 100644
index 000000000000..6fe588e40ba6
--- /dev/null
+++ b/drivers/reset/sandbox-reset-test.c
@@ -0,0 +1,55 @@
+/*
+ * Copyright (c) 2016, NVIDIA CORPORATION.
+ *
+ * SPDX-License-Identifier: GPL-2.0
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <reset_client.h>
+#include <asm/io.h>
+#include <asm/reset.h>
+
+struct sandbox_reset_test {
+	struct reset_ctl ctl;
+};
+
+int sandbox_reset_test_get(struct udevice *dev)
+{
+	struct sandbox_reset_test *sbrt = dev_get_priv(dev);
+
+	return reset_get_by_name(dev, "test", &sbrt->ctl);
+}
+
+int sandbox_reset_test_assert(struct udevice *dev)
+{
+	struct sandbox_reset_test *sbrt = dev_get_priv(dev);
+
+	return reset_assert(&sbrt->ctl);
+}
+
+int sandbox_reset_test_deassert(struct udevice *dev)
+{
+	struct sandbox_reset_test *sbrt = dev_get_priv(dev);
+
+	return reset_deassert(&sbrt->ctl);
+}
+
+int sandbox_reset_test_free(struct udevice *dev)
+{
+	struct sandbox_reset_test *sbrt = dev_get_priv(dev);
+
+	return reset_free(&sbrt->ctl);
+}
+
+static const struct udevice_id sandbox_reset_test_ids[] = {
+	{ .compatible = "sandbox,reset-ctl-test" },
+	{ }
+};
+
+U_BOOT_DRIVER(sandbox_reset_test) = {
+	.name = "sandbox_reset_test",
+	.id = UCLASS_MISC,
+	.of_match = sandbox_reset_test_ids,
+	.priv_auto_alloc_size = sizeof(struct sandbox_reset_test),
+};
diff --git a/drivers/reset/sandbox-reset.c b/drivers/reset/sandbox-reset.c
new file mode 100644
index 000000000000..e07a95991f7f
--- /dev/null
+++ b/drivers/reset/sandbox-reset.c
@@ -0,0 +1,108 @@
+/*
+ * Copyright (c) 2016, NVIDIA CORPORATION.
+ *
+ * SPDX-License-Identifier: GPL-2.0
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <reset_uclass.h>
+#include <asm/io.h>
+#include <asm/reset.h>
+
+#define SANDBOX_RESET_SIGNALS 3
+
+struct sandbox_reset_signal {
+	bool asserted;
+};
+
+struct sandbox_reset {
+	struct sandbox_reset_signal signals[SANDBOX_RESET_SIGNALS];
+};
+
+static int sandbox_reset_request(struct reset_ctl *reset_ctl)
+{
+	debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
+
+	if (reset_ctl->id >= SANDBOX_RESET_SIGNALS)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int sandbox_reset_free(struct reset_ctl *reset_ctl)
+{
+	debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
+
+	return 0;
+}
+
+static int sandbox_reset_assert(struct reset_ctl *reset_ctl)
+{
+	struct sandbox_reset *sbr = dev_get_priv(reset_ctl->dev);
+
+	debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
+
+	sbr->signals[reset_ctl->id].asserted = true;
+
+	return 0;
+}
+
+static int sandbox_reset_deassert(struct reset_ctl *reset_ctl)
+{
+	struct sandbox_reset *sbr = dev_get_priv(reset_ctl->dev);
+
+	debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
+
+	sbr->signals[reset_ctl->id].asserted = false;
+
+	return 0;
+}
+
+static int sandbox_reset_bind(struct udevice *dev)
+{
+	debug("%s(dev=%p)\n", __func__, dev);
+
+	return 0;
+}
+
+static int sandbox_reset_probe(struct udevice *dev)
+{
+	debug("%s(dev=%p)\n", __func__, dev);
+
+	return 0;
+}
+
+static const struct udevice_id sandbox_reset_ids[] = {
+	{ .compatible = "sandbox,reset-ctl" },
+	{ }
+};
+
+struct reset_ops sandbox_reset_reset_ops = {
+	.request = sandbox_reset_request,
+	.free = sandbox_reset_free,
+	.rst_assert = sandbox_reset_assert,
+	.rst_deassert = sandbox_reset_deassert,
+};
+
+U_BOOT_DRIVER(sandbox_reset) = {
+	.name = "sandbox_reset",
+	.id = UCLASS_RESET,
+	.of_match = sandbox_reset_ids,
+	.bind = sandbox_reset_bind,
+	.probe = sandbox_reset_probe,
+	.priv_auto_alloc_size = sizeof(struct sandbox_reset),
+	.ops = &sandbox_reset_reset_ops,
+};
+
+int sandbox_reset_query(struct udevice *dev, unsigned long id)
+{
+	struct sandbox_reset *sbr = dev_get_priv(dev);
+
+	debug("%s(dev=%p, id=%ld)\n", __func__, dev, id);
+
+	if (id >= SANDBOX_RESET_SIGNALS)
+		return -EINVAL;
+
+	return sbr->signals[id].asserted;
+}
diff --git a/test/dm/Makefile b/test/dm/Makefile
index 9eaf04b9ba98..cad3374e43d6 100644
--- a/test/dm/Makefile
+++ b/test/dm/Makefile
@@ -27,6 +27,7 @@ obj-$(CONFIG_DM_PCI) += pci.o
 obj-$(CONFIG_RAM) += ram.o
 obj-y += regmap.o
 obj-$(CONFIG_REMOTEPROC) += remoteproc.o
+obj-$(CONFIG_DM_RESET) += reset.o
 obj-$(CONFIG_SYSRESET) += sysreset.o
 obj-$(CONFIG_DM_RTC) += rtc.o
 obj-$(CONFIG_DM_SPI_FLASH) += sf.o
diff --git a/test/dm/reset.c b/test/dm/reset.c
new file mode 100644
index 000000000000..0ae8031540c2
--- /dev/null
+++ b/test/dm/reset.c
@@ -0,0 +1,39 @@
+/*
+ * Copyright (c) 2016, NVIDIA CORPORATION.
+ *
+ * SPDX-License-Identifier: GPL-2.0
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <dm/test.h>
+#include <asm/reset.h>
+#include <test/ut.h>
+
+/* This must match the specifier for mbox-names="test" in the DT node */
+#define TEST_RESET_ID 2
+
+static int dm_test_reset(struct unit_test_state *uts)
+{
+	struct udevice *dev_reset;
+	struct udevice *dev_test;
+
+	ut_assertok(uclass_get_device_by_name(UCLASS_RESET, "reset-ctl",
+					      &dev_reset));
+	ut_asserteq(0, sandbox_reset_query(dev_reset, TEST_RESET_ID));
+
+	ut_assertok(uclass_get_device_by_name(UCLASS_MISC, "reset-ctl-test",
+					      &dev_test));
+	ut_assertok(sandbox_reset_test_get(dev_test));
+
+	ut_assertok(sandbox_reset_test_assert(dev_test));
+	ut_asserteq(1, sandbox_reset_query(dev_reset, TEST_RESET_ID));
+
+	ut_assertok(sandbox_reset_test_deassert(dev_test));
+	ut_asserteq(0, sandbox_reset_query(dev_reset, TEST_RESET_ID));
+
+	ut_assertok(sandbox_reset_test_free(dev_test));
+
+	return 0;
+}
+DM_TEST(dm_test_reset, DM_TESTF_SCAN_FDT);
-- 
2.8.2

[U-Boot] [PATCH 1/2] Add a reset driver framework/uclass

[Simon Glass]

Hi Stephen,

On 17 May 2016 at 10:46, Stephen Warren <swarren@wwwdotorg.org> wrote:
> From: Stephen Warren <swarren@nvidia.com>
>
> A reset controller is a hardware module that controls reset signals that
> affect other hardware modules or chips.
>
> This patch defines a standard API that connects reset clients (i.e. the
> drivers for devices affected by reset signals) to drivers for reset
> controllers/providers. Initially, DT is the only supported method for
> connecting the two.
>
> The DT binding specification (reset.txt) was taken from Linux kernel
> v4.5's Documentation/devicetree/bindings/reset/reset.txt.
>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
> This series depends on my recent mailbox series, solely for a trivial
> amount of diff context.
>
> This series will eventually be a dependency for the Tegra186 clock/reset
> driver implementation, but that isn't ready yet.
>
>  doc/device-tree-bindings/reset/reset.txt |  75 +++++++++++++++++
>  drivers/Kconfig                          |   2 +
>  drivers/Makefile                         |   1 +
>  drivers/reset/Kconfig                    |  15 ++++
>  drivers/reset/Makefile                   |   5 ++
>  drivers/reset/reset-uclass.c             | 131 ++++++++++++++++++++++++++++++
>  include/dm/uclass-id.h                   |   1 +
>  include/reset_client.h                   | 135 +++++++++++++++++++++++++++++++
>  include/reset_uclass.h                   |  81 +++++++++++++++++++
>  9 files changed, 446 insertions(+)
>  create mode 100644 doc/device-tree-bindings/reset/reset.txt
>  create mode 100644 drivers/reset/Kconfig
>  create mode 100644 drivers/reset/Makefile
>  create mode 100644 drivers/reset/reset-uclass.c
>  create mode 100644 include/reset_client.h
>  create mode 100644 include/reset_uclass.h
>

This loks fine but for one thing, below.

> diff --git a/doc/device-tree-bindings/reset/reset.txt b/doc/device-tree-bindings/reset/reset.txt
> new file mode 100644
> index 000000000000..31db6ff84908
> --- /dev/null
> +++ b/doc/device-tree-bindings/reset/reset.txt
> @@ -0,0 +1,75 @@
> += Reset Signal Device Tree Bindings =
> +
> +This binding is intended to represent the hardware reset signals present
> +internally in most IC (SoC, FPGA, ...) designs. Reset signals for whole
> +standalone chips are most likely better represented as GPIOs, although there
> +are likely to be exceptions to this rule.
> +
> +Hardware blocks typically receive a reset signal. This signal is generated by
> +a reset provider (e.g. power management or clock module) and received by a
> +reset consumer (the module being reset, or a module managing when a sub-
> +ordinate module is reset). This binding exists to represent the provider and
> +consumer, and provide a way to couple the two together.
> +
> +A reset signal is represented by the phandle of the provider, plus a reset
> +specifier - a list of DT cells that represents the reset signal within the
> +provider. The length (number of cells) and semantics of the reset specifier
> +are dictated by the binding of the reset provider, although common schemes
> +are described below.
> +
> +A word on where to place reset signal consumers in device tree: It is possible
> +in hardware for a reset signal to affect multiple logically separate HW blocks
> +at once. In this case, it would be unwise to represent this reset signal in
> +the DT node of each affected HW block, since if activated, an unrelated block
> +may be reset. Instead, reset signals should be represented in the DT node
> +where it makes most sense to control it; this may be a bus node if all
> +children of the bus are affected by the reset signal, or an individual HW
> +block node for dedicated reset signals. The intent of this binding is to give
> +appropriate software access to the reset signals in order to manage the HW,
> +rather than to slavishly enumerate the reset signal that affects each HW
> +block.
> +
> += Reset providers =
> +
> +Required properties:
> +#reset-cells:  Number of cells in a reset specifier; Typically 0 for nodes
> +               with a single reset output and 1 for nodes with multiple
> +               reset outputs.
> +
> +For example:
> +
> +       rst: reset-controller {
> +               #reset-cells = <1>;
> +       };
> +
> += Reset consumers =
> +
> +Required properties:
> +resets:                List of phandle and reset specifier pairs, one pair
> +               for each reset signal that affects the device, or that the
> +               device manages. Note: if the reset provider specifies '0' for
> +               #reset-cells, then only the phandle portion of the pair will
> +               appear.
> +
> +Optional properties:
> +reset-names:   List of reset signal name strings sorted in the same order as
> +               the resets property. Consumers drivers will use reset-names to
> +               match reset signal names with reset specifiers.
> +
> +For example:
> +
> +       device {
> +               resets = <&rst 20>;
> +               reset-names = "reset";
> +       };
> +
> +This represents a device with a single reset signal named "reset".
> +
> +       bus {
> +               resets = <&rst 10> <&rst 11> <&rst 12> <&rst 11>;
> +               reset-names = "i2s1", "i2s2", "dma", "mixer";
> +       };
> +
> +This represents a bus that controls the reset signal of each of four sub-
> +ordinate devices. Consider for example a bus that fails to operate unless no
> +child device has reset asserted.
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index f2a137ad87fc..f6003a0a593a 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -56,6 +56,8 @@ source "drivers/ram/Kconfig"
>
>  source "drivers/remoteproc/Kconfig"
>
> +source "drivers/reset/Kconfig"
> +
>  source "drivers/rtc/Kconfig"
>
>  source "drivers/serial/Kconfig"
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 47fddff084b5..1634efc5fc98 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -65,6 +65,7 @@ obj-$(CONFIG_U_QE) += qe/
>  obj-y += mailbox/
>  obj-y += memory/
>  obj-y += pwm/
> +obj-y += reset/
>  obj-y += input/
>  # SOC specific infrastructure drivers.
>  obj-y += soc/
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> new file mode 100644
> index 000000000000..5c449a99d39d
> --- /dev/null
> +++ b/drivers/reset/Kconfig
> @@ -0,0 +1,15 @@
> +menu "Reset Controller Support"
> +
> +config DM_RESET
> +       bool "Enable reset controllers using Driver Model"
> +       depends on DM && OF_CONTROL
> +       help
> +         Enable support for the reset controller driver class. Many hardware
> +         modules are equipped with a reset signal, typically driven by some
> +         reset controller hardware module within the chip. In U-Boot, reset
> +         controller drivers allow control over these reset signals. In some
> +         cases this API is applicable to chips outside the CPU as well,
> +         although driving such reset isgnals using GPIOs may be more
> +         appropriate in this case.
> +
> +endmenu
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> new file mode 100644
> index 000000000000..508608e83f87
> --- /dev/null
> +++ b/drivers/reset/Makefile
> @@ -0,0 +1,5 @@
> +# Copyright (c) 2016, NVIDIA CORPORATION.
> +#
> +# SPDX-License-Identifier: GPL-2.0
> +
> +obj-$(CONFIG_DM_RESET) += reset-uclass.o
> diff --git a/drivers/reset/reset-uclass.c b/drivers/reset/reset-uclass.c
> new file mode 100644
> index 000000000000..ac0614eb9ab9
> --- /dev/null
> +++ b/drivers/reset/reset-uclass.c
> @@ -0,0 +1,131 @@
> +/*
> + * Copyright (c) 2016, NVIDIA CORPORATION.
> + *
> + * SPDX-License-Identifier: GPL-2.0
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <fdtdec.h>
> +#include <reset_client.h>
> +#include <reset_uclass.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +static inline struct reset_ops *reset_dev_ops(struct udevice *dev)
> +{
> +       return (struct reset_ops *)dev->driver->ops;
> +}
> +
> +static int reset_of_xlate_default(struct reset_ctl *reset_ctl,
> +                                 struct fdtdec_phandle_args *args)
> +{
> +       debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
> +
> +       if (args->args_count != 1) {
> +               debug("Invaild args_count: %d\n", args->args_count);
> +               return -EINVAL;
> +       }
> +
> +       reset_ctl->id = args->args[0];
> +
> +       return 0;
> +}
> +
> +int reset_get_by_index(struct udevice *dev, int index,
> +                      struct reset_ctl *reset_ctl)
> +{
> +       struct fdtdec_phandle_args args;
> +       int ret;
> +       struct udevice *dev_reset;
> +       struct reset_ops *ops;
> +
> +       debug("%s(dev=%p, index=%d, reset_ctl=%p)\n", __func__, dev, index,
> +             reset_ctl);
> +
> +       ret = fdtdec_parse_phandle_with_args(gd->fdt_blob, dev->of_offset,
> +                                            "resets", "#reset-cells", 0,
> +                                            index, &args);
> +       if (ret) {
> +               debug("%s: fdtdec_parse_phandle_with_args failed: %d\n",
> +                     __func__, ret);
> +               return ret;
> +       }
> +
> +       ret = uclass_get_device_by_of_offset(UCLASS_RESET, args.node,
> +                                            &dev_reset);
> +       if (ret) {
> +               debug("%s: uclass_get_device_by_of_offset failed: %d\n",
> +                     __func__, ret);
> +               return ret;
> +       }
> +       ops = reset_dev_ops(dev_reset);
> +
> +       reset_ctl->dev = dev_reset;
> +       if (ops->of_xlate)
> +               ret = ops->of_xlate(reset_ctl, &args);
> +       else
> +               ret = reset_of_xlate_default(reset_ctl, &args);
> +       if (ret) {
> +               debug("of_xlate() failed: %d\n", ret);
> +               return ret;
> +       }
> +
> +       ret = ops->request(reset_ctl);
> +       if (ret) {
> +               debug("ops->request() failed: %d\n", ret);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +int reset_get_by_name(struct udevice *dev, const char *name,
> +                    struct reset_ctl *reset_ctl)
> +{
> +       int index;
> +
> +       debug("%s(dev=%p, name=%s, reset_ctl=%p)\n", __func__, dev, name,
> +             reset_ctl);
> +
> +       index = fdt_find_string(gd->fdt_blob, dev->of_offset, "reset-names",
> +                               name);
> +       if (index < 0) {
> +               debug("fdt_find_string() failed: %d\n", index);
> +               return index;
> +       }
> +
> +       return reset_get_by_index(dev, index, reset_ctl);
> +}
> +
> +int reset_free(struct reset_ctl *reset_ctl)
> +{
> +       struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
> +
> +       debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
> +
> +       return ops->free(reset_ctl);
> +}
> +
> +int reset_assert(struct reset_ctl *reset_ctl)
> +{
> +       struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
> +
> +       debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
> +
> +       return ops->rst_assert(reset_ctl);
> +}
> +
> +int reset_deassert(struct reset_ctl *reset_ctl)
> +{
> +       struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
> +
> +       debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
> +
> +       return ops->rst_deassert(reset_ctl);
> +}
> +
> +UCLASS_DRIVER(reset) = {
> +       .id             = UCLASS_RESET,
> +       .name           = "reset",
> +};
> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> index 4e252ae210f7..e31f699cdb65 100644
> --- a/include/dm/uclass-id.h
> +++ b/include/dm/uclass-id.h
> @@ -63,6 +63,7 @@ enum uclass_id {
>         UCLASS_PWRSEQ,          /* Power sequence device */
>         UCLASS_REGULATOR,       /* Regulator device */
>         UCLASS_REMOTEPROC,      /* Remote Processor device */
> +       UCLASS_RESET,           /* Reset controller device */
>         UCLASS_RTC,             /* Real time clock device */
>         UCLASS_SERIAL,          /* Serial UART */
>         UCLASS_SPI,             /* SPI bus */
> diff --git a/include/reset_client.h b/include/reset_client.h
> new file mode 100644
> index 000000000000..3bdc8256853a
> --- /dev/null
> +++ b/include/reset_client.h
> @@ -0,0 +1,135 @@
> +/*
> + * Copyright (c) 2016, NVIDIA CORPORATION.
> + *
> + * SPDX-License-Identifier: GPL-2.0
> + */
> +
> +#ifndef _RESET_CLIENT_H
> +#define _RESET_CLIENT_H
> +
> +/**
> + * A reset is a hardware signal indicating that a HW module (or IP block, or
> + * sometimes an entire off-CPU chip) reset all of its internal state to some
> + * known-good initial state. Drivers will often reset HW modules when they
> + * begin execution to ensure that hardware correctly responds to all requests,
> + * or in response to some error condition. Reset signals are often controlled
> + * externally to the HW module being reset, by an entity this API calls a reset
> + * controller. This API provides a standard means for drivers to request that
> + * reset controllers set or clear reset signals.
> + *
> + * A driver that implements UCLASS_RESET is a reset controller or provider. A
> + * controller will often implement multiple separate reset signals, since the
> + * hardware it manages often has this capability. reset_uclass.h describes the
> + * interface which reset controllers must implement.
> + *
> + * Reset consumers/clients are the HW modules affected by reset signals. This
> + * header file describes the API used by drivers for those HW modules.
> + */
> +
> +struct udevice;
> +
> +/**
> + * struct reset_ctl - A handle to (allowing control of) a single reset signal.
> + *
> + * Clients provide storage for reset control handles. The content of the
> + * structure is managed solely by the reset API and reset drivers. A reset
> + * control struct is initialized by "get"ing the reset control struct. The
> + * reset control struct is passed to all other reset APIs to identify which
> + * reset signal to operate upon.
> + *
> + * @dev: The device which implements the reset signal.
> + * @id: The reset signal ID within the provider.
> + *
> + * Currently, the reset API assumes that a single integer ID is enough to
> + * identify and configure any reset signal for any reset provider. If this
> + * assumption becomes invalid in the future, the struct could be expanded to
> + * either (a) add more fields to allow reset providers to store additional
> + * information, or (b) replace the id field with an opaque pointer, which the
> + * provider would dynamically allocated during its .of_xlate op, and process
> + * during is .request op. This may require the addition of an extra op to clean
> + * up the allocation.
> + */
> +struct reset_ctl {
> +       struct udevice *dev;
> +       /*
> +        * Written by of_xlate. We assume a single id is enough for now. In the
> +        * future, we might add more fields here.
> +        */
> +       unsigned long id;

This feels like an obfuscation to me. Why not just use an int instead
of this struct? This is what clock does.

> +};
> +
> +/**
> + * reset_get_by_index - Get/request a reset signal by integer index.
> + *
> + * This looks up and requests a reset signal. The index is relative to the
> + * client device; each device is assumed to have n reset signals associated
> + * with it somehow, and this function finds and requests one of them. The
> + * mapping of client device reset signal indices to provider reset signals may
> + * be via device-tree properties, board-provided mapping tables, or some other
> + * mechanism.
> + *
> + * @dev:       The client device.
> + * @index:     The index of the reset signal to request, within the client's
> + *             list of reset signals.
> + * @reset_ctl  A pointer to a reset control struct to initialize.
> + * @return 0 if OK, or a negative error code.
> + */
> +int reset_get_by_index(struct udevice *dev, int index,
> +                      struct reset_ctl *reset_ctl);
> +
> +/**
> + * reset_get_by_name - Get/request a reset signal by name.
> + *
> + * This looks up and requests a reset signal. The name is relative to the
> + * client device; each device is assumed to have n reset signals associated
> + * with it somehow, and this function finds and requests one of them. The
> + * mapping of client device reset signal names to provider reset signal may be
> + * via device-tree properties, board-provided mapping tables, or some other
> + * mechanism.
> + *
> + * @dev:       The client device.
> + * @name:      The name of the reset signal to request, within the client's
> + *             list of reset signals.
> + * @reset_ctl: A pointer to a reset control struct to initialize.
> + * @return 0 if OK, or a negative error code.
> + */
> +int reset_get_by_name(struct udevice *dev, const char *name,
> +                     struct reset_ctl *reset_ctl);
> +
> +/**
> + * reset_free - Free a previously requested reset signal.
> + *
> + * @reset_ctl: A reset control struct that was previously successfully
> + *             requested by reset_get_by_*().
> + * @return 0 if OK, or a negative error code.
> + */
> +int reset_free(struct reset_ctl *reset_ctl);
> +
> +/**
> + * reset_assert - Assert a reset signal.
> + *
> + * This function will assert the specified reset signal, thus resetting the
> + * affected HW module(s). Depending on the reset controller hardware, the reset
> + * signal will either stay asserted until reset_deassert() is called, or the
> + * hardware may autonomously clear the reset signal itself.
> + *
> + * @reset_ctl: A reset control struct that was previously successfully
> + *             requested by reset_get_by_*().
> + * @return 0 if OK, or a negative error code.
> + */
> +int reset_assert(struct reset_ctl *reset_ctl);
> +
> +/**
> + * reset_deassert - Deassert a reset signal.
> + *
> + * This function will deassert the specified reset signal, thus releasing the
> + * affected HW modules() from reset, and allowing them to continue normal
> + * operation.
> + *
> + * @reset_ctl: A reset control struct that was previously successfully
> + *             requested by reset_get_by_*().
> + * @return 0 if OK, or a negative error code.
> + */
> +int reset_deassert(struct reset_ctl *reset_ctl);
> +
> +#endif
> diff --git a/include/reset_uclass.h b/include/reset_uclass.h
> new file mode 100644
> index 000000000000..6e27a7bbd1cf
> --- /dev/null
> +++ b/include/reset_uclass.h
> @@ -0,0 +1,81 @@
> +/*
> + * Copyright (c) 2016, NVIDIA CORPORATION.
> + *
> + * SPDX-License-Identifier: GPL-2.0
> + */
> +
> +#ifndef _RESET_UCLASS_H
> +#define _RESET_UCLASS_H
> +
> +/* See reset_client.h for background documentation. */
> +
> +#include <reset_client.h>
> +
> +struct udevice;
> +
> +/**
> + * struct reset_ops - The functions that a reset controller driver must
> + * implement.
> + */
> +struct reset_ops {
> +       /**
> +        * of_xlate - Translate a client's device-tree (OF) reset specifier.
> +        *
> +        * The reset core calls this function as the first step in implementing
> +        * a client's reset_get_by_*() call.
> +        *
> +        * If this function pointer is set to NULL, the reset core will use a
> +        * default implementation, which assumes #reset-cells = <1>, and that
> +        * the DT cell contains a simple integer reset signal ID.
> +        *
> +        * At present, the reset API solely supports device-tree. If this
> +        * changes, other xxx_xlate() functions may be added to support those
> +        * other mechanisms.
> +        *
> +        * @reset_ctl:  The reset control struct to hold the translation result.
> +        * @args:       The reset specifier values from device tree.
> +        * @return 0 if OK, or a negative error code.
> +        */
> +       int (*of_xlate)(struct reset_ctl *reset_ctl,
> +                       struct fdtdec_phandle_args *args);
> +       /**
> +        * request - Request a translated reset control.
> +        *
> +        * The reset core calls this function as the second step in
> +        * implementing a client's reset_get_by_*() call, following a
> +        * successful xxx_xlate() call.
> +        *
> +        * @reset_ctl:  The reset control struct to request; this has been
> +        *              filled in by a previoux xxx_xlate() function call.
> +        * @return 0 if OK, or a negative error code.
> +        */
> +       int (*request)(struct reset_ctl *reset_ctl);
> +       /**
> +        * free - Free a previously requested reset control.
> +        *
> +        * This is the implementation of the client reset_free() API.
> +        *
> +        * @reset_ctl:  The reset control to free.
> +        * @return 0 if OK, or a negative error code.
> +        */
> +       int (*free)(struct reset_ctl *reset_ctl);
> +       /**
> +        * rst_assert - Assert a reset signal.
> +        *
> +        * Note: This function is named rst_assert not assert to avoid
> +        * conflicting with global macro assert().
> +        *
> +        * @reset_ctl:  The reset signal to assert.
> +        * @return 0 if OK, or a negative error code.
> +        */
> +       int (*rst_assert)(struct reset_ctl *reset_ctl);
> +       /**
> +        * rst_deassert - Deassert a reset signal.
> +        *
> +        * @reset_ctl:  The reset signal to deassert.
> +        * @return 0 if OK, or a negative error code.
> +        */
> +       int (*rst_deassert)(struct reset_ctl *reset_ctl);
> +};
> +
> +#endif
> --
> 2.8.2
>

Regards,
Simon

[U-Boot] [PATCH 2/2] reset: implement a reset test

[Simon Glass]

Hi Stephen,

On 17 May 2016 at 10:46, Stephen Warren <swarren@wwwdotorg.org> wrote:
> From: Stephen Warren <swarren@nvidia.com>
>
> This adds a sandbox reset implementation (provider), a test client
> device, instantiates them both from Sandbox's DT, and adds a DM test
> that excercises everything.
>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  arch/sandbox/dts/test.dts          |  11 ++++
>  arch/sandbox/include/asm/reset.h   |  21 ++++++++
>  configs/sandbox_defconfig          |   2 +
>  drivers/reset/Kconfig              |   8 +++
>  drivers/reset/Makefile             |   2 +
>  drivers/reset/sandbox-reset-test.c |  55 +++++++++++++++++++
>  drivers/reset/sandbox-reset.c      | 108 +++++++++++++++++++++++++++++++++++++
>  test/dm/Makefile                   |   1 +
>  test/dm/reset.c                    |  39 ++++++++++++++
>  9 files changed, 247 insertions(+)
>  create mode 100644 arch/sandbox/include/asm/reset.h
>  create mode 100644 drivers/reset/sandbox-reset-test.c
>  create mode 100644 drivers/reset/sandbox-reset.c
>  create mode 100644 test/dm/reset.c

Apart from struct reset_ctl this looks fine.

Regards,
Simon

[U-Boot] [PATCH 1/2] Add a reset driver framework/uclass

[Stephen Warren]

On 05/17/2016 03:56 PM, Simon Glass wrote:
> Hi Stephen,
>
> On 17 May 2016 at 10:46, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> A reset controller is a hardware module that controls reset signals that
>> affect other hardware modules or chips.
>>
>> This patch defines a standard API that connects reset clients (i.e. the
>> drivers for devices affected by reset signals) to drivers for reset
>> controllers/providers. Initially, DT is the only supported method for
>> connecting the two.
>>
>> The DT binding specification (reset.txt) was taken from Linux kernel
>> v4.5's Documentation/devicetree/bindings/reset/reset.txt.

(trimming the quoted text is useful...)

>> diff --git a/include/reset_client.h b/include/reset_client.h

>> +/**
>> + * struct reset_ctl - A handle to (allowing control of) a single reset signal.
>> + *
>> + * Clients provide storage for reset control handles. The content of the
>> + * structure is managed solely by the reset API and reset drivers. A reset
>> + * control struct is initialized by "get"ing the reset control struct. The
>> + * reset control struct is passed to all other reset APIs to identify which
>> + * reset signal to operate upon.
>> + *
>> + * @dev: The device which implements the reset signal.
>> + * @id: The reset signal ID within the provider.
>> + *
>> + * Currently, the reset API assumes that a single integer ID is enough to
>> + * identify and configure any reset signal for any reset provider. If this
>> + * assumption becomes invalid in the future, the struct could be expanded to
>> + * either (a) add more fields to allow reset providers to store additional
>> + * information, or (b) replace the id field with an opaque pointer, which the
>> + * provider would dynamically allocated during its .of_xlate op, and process
>> + * during is .request op. This may require the addition of an extra op to clean
>> + * up the allocation.
>> + */
>> +struct reset_ctl {
>> +       struct udevice *dev;
>> +       /*
>> +        * Written by of_xlate. We assume a single id is enough for now. In the
>> +        * future, we might add more fields here.
>> +        */
>> +       unsigned long id;
>
> This feels like an obfuscation to me. Why not just use an int instead
> of this struct? This is what clock does.

DT allows multiple cells for all resource specifiers (GPIOs, mailboxes, 
clocks, resets, interrupts). There are actual real-world cases (at least 
in DTs in the Linux kernel, even if not in U-Boot[1]) of specifiers of 
more than a single cell. It may not be possible to pack those multiple 
cells into a single integer, and semantically doesn't make much sense to 
do so if it didn't make sense to do so in DT. Consequently, we must plan 
for this case by allowing for client resource handles more capable than 
a single integer. The drivers I'm currently working on do use only a 
single integer, so that's all I've put into this struct for now. 
However, I fully expect other driver authors to need to add additional 
fields here, as explained in the comments quoted above.

Note that this scheme is identical to the mailbox driver that you 
already ack'd.

It's also an abstraction not an obfuscation; clients should not need to 
know/care about how drivers-that-provide-resources or DT or any other 
scheme represents/configures resources; clients should simply get a 
single opaque handle that allows them to manipulate the resource through 
standard APIs. Forcing clients to store a separate provider device 
handle and ID integer (and in the future, any additional fields 
required) puts too heavy a burden on the resource consumer, and exposes 
knowledge to them that they do not need.

Re: the clock API: I'm planning on changing that to this style too, for 
the same reasons. Additionally, clk_get_by_index() currently returns 
part of the resource handle (the resource's integer ID) as the return 
value, and part (the provider udevice pointer) as an "out" function 
parameter, which I found extremely confusing and non-obvious when 
reading the function signature. With the scheme in this patch, a single 
value is returned to the caller by the "get" function, which makes life 
much simpler.

[U-Boot] [PATCH 1/2] Add a reset driver framework/uclass

[Stephen Warren]

On 05/18/2016 10:54 AM, Stephen Warren wrote:
> On 05/17/2016 03:56 PM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On 17 May 2016 at 10:46, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> From: Stephen Warren <swarren@nvidia.com>
>>>
>>> A reset controller is a hardware module that controls reset signals that
>>> affect other hardware modules or chips.
>>>
>>> This patch defines a standard API that connects reset clients (i.e. the
>>> drivers for devices affected by reset signals) to drivers for reset
>>> controllers/providers. Initially, DT is the only supported method for
>>> connecting the two.
>>>
>>> The DT binding specification (reset.txt) was taken from Linux kernel
>>> v4.5's Documentation/devicetree/bindings/reset/reset.txt.

Simon, any further thoughts on this?

[U-Boot] [PATCH 1/2] Add a reset driver framework/uclass

[Simon Glass]

Hi Stephen,

On 2 June 2016 at 09:59, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 05/18/2016 10:54 AM, Stephen Warren wrote:
>>
>> On 05/17/2016 03:56 PM, Simon Glass wrote:
>>>
>>> Hi Stephen,
>>>
>>> On 17 May 2016 at 10:46, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>>
>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>
>>>> A reset controller is a hardware module that controls reset signals that
>>>> affect other hardware modules or chips.
>>>>
>>>> This patch defines a standard API that connects reset clients (i.e. the
>>>> drivers for devices affected by reset signals) to drivers for reset
>>>> controllers/providers. Initially, DT is the only supported method for
>>>> connecting the two.
>>>>
>>>> The DT binding specification (reset.txt) was taken from Linux kernel
>>>> v4.5's Documentation/devicetree/bindings/reset/reset.txt.
>
>
> Simon, any further thoughts on this?

It looks fine to me. This brings it in line with GPIOs and I think
that is a good thing.

But can you change reset_client.h to reset.h and reset_uclass.h to
reset-uclass.h?

Otherwise:

Acked-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 2/2] reset: implement a reset test

[Simon Glass]

On 17 May 2016 at 14:56, Simon Glass <sjg@chromium.org> wrote:
> Hi Stephen,
>
> On 17 May 2016 at 10:46, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> This adds a sandbox reset implementation (provider), a test client
>> device, instantiates them both from Sandbox's DT, and adds a DM test
>> that excercises everything.
>>
>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>> ---
>>  arch/sandbox/dts/test.dts          |  11 ++++
>>  arch/sandbox/include/asm/reset.h   |  21 ++++++++
>>  configs/sandbox_defconfig          |   2 +
>>  drivers/reset/Kconfig              |   8 +++
>>  drivers/reset/Makefile             |   2 +
>>  drivers/reset/sandbox-reset-test.c |  55 +++++++++++++++++++
>>  drivers/reset/sandbox-reset.c      | 108 +++++++++++++++++++++++++++++++++++++
>>  test/dm/Makefile                   |   1 +
>>  test/dm/reset.c                    |  39 ++++++++++++++
>>  9 files changed, 247 insertions(+)
>>  create mode 100644 arch/sandbox/include/asm/reset.h
>>  create mode 100644 drivers/reset/sandbox-reset-test.c
>>  create mode 100644 drivers/reset/sandbox-reset.c
>>  create mode 100644 test/dm/reset.c
>
> Apart from struct reset_ctl this looks fine.

Based on the discussion on the other thread I am happy with this too.

Acked-by: Simon Glass <sjg@chromium.org>