[PATCH v16 0/7] power: add power sequence library

[PATCH v16 0/7] power: add power sequence library

[Peter Chen]

This is a follow-up for my last power sequence framework patch set [1].
According to Rob Herring and Ulf Hansson's comments[2]. The kinds of
power sequence instances will be added at postcore_initcall, the match
criteria is compatible string first, if the compatible string is not
matched between dts and library, it will try to use generic power sequence.
	 
The host driver just needs to call of_pwrseq_on/of_pwrseq_off
if only one power sequence instance is needed, for more power sequences
are used, using of_pwrseq_on_list/of_pwrseq_off_list instead (eg, USB hub driver).

In future, if there are special power sequence requirements, the special
power sequence library can be created.

This patch set is tested on i.mx6 sabresx evk using a dts change, I use
two hot-plug devices to simulate this use case, the related binding
change is updated at patch [1/6], The udoo board changes were tested
using my last power sequence patch set.[3]

Except for hard-wired MMC and USB devices, I find the USB ULPI PHY also
need to power on itself before it can be found by ULPI bus.

@Rafael, USB Maintainer has already acked the USB related changes, would you
consider queueing it at v4.13-rc1, sereval guys are waiting for this series.
Thanks.

Changes for v16:
- Change match way between pwrseq instance and device. Currently, it maintains a global
  table for compatible string and corresponding pwrseq library allocation function. The
  match is occurred when host controller tries to carry out power on device, if it is
  matched, it will allocate pwrseq instance for this device. [Patch 2/7]
- Add related compatible string maintained at pwrseq_match_table_list. [Patch 1/7]

Changes for v15:
- Change pwrseq list name at USB hub structure (pwrseq_on_list->pwrseq_list). [Patch 4/7]
- Add USB Maintainer's Ack [Patch 4/7]

Changes for v14:
- Rebase for v4.12-rc1
- Delete some USB sysdev patches which has already merged

Changes for v13:
- Add more design descriptions at design doc and fix one build error
  introduced by v12 wrongly [Patch 2/12]
- Add the last three dts patches which were forgotten at last series
- Move the comment for usb_create_shared_hcd to correct place [Patch 3/12]
- Add sysdev for shared hcd too for xhci-plat.c [Patch 6/12]

Rafael, if the first two power sequence patches are ok for you, would you consider
accept these first, the other USB patches can go through USB tree at v4.12-rc1?

Changes for v12:
- Add design doc and more comments at generic power sequence source file [Patch 2/9]
- Introduce four Arnd Bergmann patches and one my ehci related patches, these patches
  are used to get property DT/firmware information at USB code, and these information
  are needed for power sequence operation at USB. With these five patches, my chipidea
  hack patch in previous patch set can be removed. [Patch 3-7/9]
- Add -ENOENT judgement to avoid USB error if no power sequence library is chosen [9/9]

Changes for v11:
- Fix warning: (USB) selects POWER_SEQUENCE which has unmet direct dependencies (OF)
- Delete redundant copyright statement.
- Change pr_warn to pr_debug at wrseq_find_available_instance
- Refine kerneldoc
- %s/ENONET/ENOENT 
- Allocate pwrseq list node before than carry out power sequence on 
- Add mutex_lock/mutex_lock for pwrseq node browse at pwrseq_find_available_instance
- Add pwrseq_suspend/resume for API both single instance and list 
- Add .pwrseq_suspend/resume for pwrseq_generic.c
- Add pwrseq_suspend_list and pwrseq_resume_list for USB hub suspend
  and resume routine

Changes for v10:
- Improve the kernel-doc for power sequence core, including exported APIs and
  main structure. [Patch 2/8]
- Change Kconfig, and let the user choose power sequence. [Patch 2/8]
- Delete EXPORT_SYMBOL and change related APIs as local, these APIs do not
  be intended to export currently. [Patch 2/8]
- Selete POWER_SEQUENCE at USB core's Kconfig. [Patch 4/8]

Changes for v9:
- Add Vaibhav Hiremath's reviewed-by [Patch 4/8]
- Rebase to v4.9-rc1

Changes for v8:
- Allocate one extra pwrseq instance if pwrseq_get has succeed, it can avoid
  preallocate instances problem which the number of instance is decided at
  compile time, thanks for Heiko Stuebner's suggestion [Patch 2/8]
- Delete pwrseq_compatible_sample.c which is the demo purpose to show compatible
  match method. [Patch 2/8]
- Add Maciej S. Szmigiero's tested-by. [Patch 7/8]

Changes for v7:
- Create kinds of power sequence instance at postcore_initcall, and match
  the instance with node using compatible string, the beneit of this is
  the host driver doesn't need to consider which pwrseq instance needs
  to be used, and pwrseq core will match it, however, it eats some memories
  if less power sequence instances are used. [Patch 2/8]
- Add pwrseq_compatible_sample.c to test match pwrseq using device_id. [Patch 2/8]
- Fix the comments Vaibhav Hiremath adds for error path for clock and do not
  use device_node for parameters at pwrseq_on. [Patch 2/8]
- Simplify the caller to use power sequence, follows Alan's commnets [Patch 4/8]
- Tested three pwrseq instances together using both specific compatible string and
  generic libraries.

Changes for v6:
- Add Matthias Kaehlcke's Reviewed-by and Tested-by. (patch [2/6])
- Change chipidea core of_node assignment for coming user. (patch [5/6])
- Applies Joshua Clayton's three dts changes for two boards,
  the USB device's reg has only #address-cells, but without #size-cells.

Changes for v5:
- Delete pwrseq_register/pwrseq_unregister, which is useless currently
- Fix the linker error when the pwrseq user is compiled as module

Changes for v4:
- Create the patch on next-20160722 
- Fix the of_node is not NULL after chipidea driver is unbinded [Patch 5/6]
- Using more friendly wait method for reset gpio [Patch 2/6]
- Support multiple input clocks [Patch 2/6]
- Add Rob Herring's ack for DT changes
- Add Joshua Clayton's Tested-by

Changes for v3:
- Delete "power-sequence" property at binding-doc, and change related code
  at both library and user code.
- Change binding-doc example node name with Rob's comments
- of_get_named_gpio_flags only gets the gpio, but without setting gpio flags,
  add additional code request gpio with proper gpio flags
- Add Philipp Zabel's Ack and MAINTAINER's entry

Changes for v2:
- Delete "pwrseq" prefix and clock-names for properties at dt binding
- Should use structure not but its pointer for kzalloc
- Since chipidea core has no of_node, let core's of_node equals glue
  layer's at core's probe

[1] http://www.spinics.net/lists/linux-usb/msg142755.html
[2] http://www.spinics.net/lists/linux-usb/msg143106.html
[3] http://www.spinics.net/lists/linux-usb/msg142815.html
[4] http://www.spinics.net/lists/linux-usb/msg152375.html

Joshua Clayton (2):
  ARM: dts: imx6qdl: Enable usb node children with <reg>
  ARM: dts: imx6q-evi: Fix onboard hub reset line

Peter Chen (5):
  binding-doc: power: pwrseq-generic: add binding doc for generic power
    sequence library
  power: add power sequence library
  binding-doc: usb: usb-device: add optional properties for power
    sequence
  usb: core: add power sequence handling for USB devices
  ARM: dts: imx6qdl-udoo.dtsi: fix onboard USB HUB property

 .../bindings/power/pwrseq/pwrseq-generic.txt       |  53 ++++
 .../devicetree/bindings/usb/usb-device.txt         |  10 +-
 Documentation/power/power-sequence/design.rst      |  54 ++++
 MAINTAINERS                                        |   9 +
 arch/arm/boot/dts/imx6q-evi.dts                    |  25 +-
 arch/arm/boot/dts/imx6qdl-udoo.dtsi                |  26 +-
 arch/arm/boot/dts/imx6qdl.dtsi                     |   6 +
 drivers/power/Kconfig                              |   1 +
 drivers/power/Makefile                             |   1 +
 drivers/power/pwrseq/Kconfig                       |  20 ++
 drivers/power/pwrseq/Makefile                      |   2 +
 drivers/power/pwrseq/core.c                        | 293 +++++++++++++++++++++
 drivers/power/pwrseq/pwrseq_generic.c              | 210 +++++++++++++++
 drivers/usb/Kconfig                                |   1 +
 drivers/usb/core/hub.c                             |  49 +++-
 drivers/usb/core/hub.h                             |   1 +
 include/linux/power/pwrseq.h                       |  84 ++++++
 17 files changed, 808 insertions(+), 37 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/power/pwrseq/pwrseq-generic.txt
 create mode 100644 Documentation/power/power-sequence/design.rst
 create mode 100644 drivers/power/pwrseq/Kconfig
 create mode 100644 drivers/power/pwrseq/Makefile
 create mode 100644 drivers/power/pwrseq/core.c
 create mode 100644 drivers/power/pwrseq/pwrseq_generic.c
 create mode 100644 include/linux/power/pwrseq.h

-- 
2.7.4

[PATCH v16 1/7] binding-doc: power: pwrseq-generic: add binding doc for generic power sequence library

[Peter Chen]

Add binding doc for generic power sequence library.

Signed-off-by: Peter Chen <peter.chen@nxp.com>
Acked-by: Philipp Zabel <p.zabel@pengutronix.de>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../bindings/power/pwrseq/pwrseq-generic.txt       | 53 ++++++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/pwrseq/pwrseq-generic.txt

diff --git a/Documentation/devicetree/bindings/power/pwrseq/pwrseq-generic.txt b/Documentation/devicetree/bindings/power/pwrseq/pwrseq-generic.txt
new file mode 100644
index 0000000..3d282ca
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/pwrseq/pwrseq-generic.txt
@@ -0,0 +1,53 @@
+The generic power sequence library
+
+Some hard-wired devices (eg USB/MMC) need to do power sequence before
+the device can be enumerated on the bus, the typical power sequence
+like: enable USB PHY clock, toggle reset pin, etc. But current
+Linux device driver lacks of such code to do it, it may cause some
+hard-wired devices works abnormal or can't be recognized by
+controller at all. The power sequence will be done before this device
+can be found at the bus.
+
+The power sequence properties is under the device node.
+
+Required properties:
+- compatible: should be one of:
+	"usb424,2513"
+	"usb424,2514"
+
+Optional properties:
+- clocks: the input clocks for device.
+- reset-gpios: Should specify the GPIO for reset.
+- reset-duration-us: the duration in microsecond for assert reset signal.
+
+Below is the example of USB power sequence properties on USB device
+nodes which have two level USB hubs.
+
+&usbotg1 {
+	vbus-supply = <&reg_usb_otg1_vbus>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_usb_otg1_id>;
+	status = "okay";
+
+	#address-cells = <1>;
+	#size-cells = <0>;
+	genesys: hub@1 {
+		compatible = "usb424,2513";
+		reg = <1>;
+
+		clocks = <&clks IMX6SX_CLK_CKO>;
+		reset-gpios = <&gpio4 5 GPIO_ACTIVE_LOW>; /* hub reset pin */
+		reset-duration-us = <10>;
+
+		#address-cells = <1>;
+		#size-cells = <0>;
+		asix: ethernet@1 {
+			compatible = "usb424,2514";
+			reg = <1>;
+
+			clocks = <&clks IMX6SX_CLK_IPG>;
+			reset-gpios = <&gpio4 6 GPIO_ACTIVE_LOW>;
+			reset-duration-us = <15>;
+		};
+	};
+};
-- 
2.7.4

[PATCH v16 2/7] power: add power sequence library

[Peter Chen]

We have an well-known problem that the device needs to do some power
sequence before it can be recognized by related host, the typical
example like hard-wired mmc devices and usb devices.

This power sequence is hard to be described at device tree and handled by
related host driver, so we have created a common power sequence
library to cover this requirement. The core code has supplied
some common helpers for host driver, and individual power sequence
libraries handle kinds of power sequence for devices. The pwrseq
librares always need to allocate extra instance for compatible
string match.

pwrseq_generic is intended for general purpose of power sequence, which
handles gpios and clocks currently, and can cover other controls in
future. The host driver just needs to call of_pwrseq_on/of_pwrseq_off
if only one power sequence is needed, else call of_pwrseq_on_list
/of_pwrseq_off_list instead (eg, USB hub driver).

For new power sequence library, it needs to add its compatible string
and allocation function at pwrseq_match_table_list, then the pwrseq
core will match it with DT's, and choose this library at runtime.

Signed-off-by: Peter Chen <peter.chen@nxp.com>
Tested-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
Tested-by Joshua Clayton <stillcompiling@gmail.com>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Tested-by: Matthias Kaehlcke <mka@chromium.org>
---
 Documentation/power/power-sequence/design.rst |  54 +++++
 MAINTAINERS                                   |   9 +
 drivers/power/Kconfig                         |   1 +
 drivers/power/Makefile                        |   1 +
 drivers/power/pwrseq/Kconfig                  |  20 ++
 drivers/power/pwrseq/Makefile                 |   2 +
 drivers/power/pwrseq/core.c                   | 293 ++++++++++++++++++++++++++
 drivers/power/pwrseq/pwrseq_generic.c         | 210 ++++++++++++++++++
 include/linux/power/pwrseq.h                  |  84 ++++++++
 9 files changed, 674 insertions(+)
 create mode 100644 Documentation/power/power-sequence/design.rst
 create mode 100644 drivers/power/pwrseq/Kconfig
 create mode 100644 drivers/power/pwrseq/Makefile
 create mode 100644 drivers/power/pwrseq/core.c
 create mode 100644 drivers/power/pwrseq/pwrseq_generic.c
 create mode 100644 include/linux/power/pwrseq.h

diff --git a/Documentation/power/power-sequence/design.rst b/Documentation/power/power-sequence/design.rst
new file mode 100644
index 0000000..554608e
--- /dev/null
+++ b/Documentation/power/power-sequence/design.rst
@@ -0,0 +1,54 @@
+====================================
+Power Sequence Library
+====================================
+
+:Date: Feb, 2017
+:Author: Peter Chen <peter.chen@nxp.com>
+
+
+Introduction
+============
+
+We have an well-known problem that the device needs to do a power
+sequence before it can be recognized by related host, the typical
+examples are hard-wired mmc devices and usb devices. The host controller
+can't know what kinds of this device is in its bus if the power
+sequence has not done, since the related devices driver's probe calling
+is determined by runtime according to eunumeration results. Besides,
+the devices may have custom power sequence, so the power sequence library
+which is independent with the devices is needed.
+
+Design
+============
+
+The power sequence library includes the core file and customer power
+sequence library. The core file exports interfaces are called by
+host controller driver for power sequence and customer power sequence
+library files to register its power sequence instance to global
+power sequence list. The custom power sequence library creates power
+sequence instance and implement custom power sequence.
+
+Since the power sequence describes hardware design, the description is
+located at board description file, eg, device tree dts file. And
+a specific power sequence belongs to device, so its description
+is under the device node, please refer to:
+Documentation/devicetree/bindings/power/pwrseq/pwrseq-generic.txt
+
+Custom power sequence library allocates one power sequence instance at
+bootup periods using postcore_initcall, this static allocated instance is
+used to compare with device-tree (DT) node to see if this library can be
+used for the node or not. When the result is matched, the core API will
+try to get resourses (->get, implemented at each library) for power
+sequence, if all resources are got, it will try to allocate another
+instance for next possible request from host driver.
+
+Then, the host controller driver can carry out power sequence on for this
+DT node, the library will do corresponding operations, like open clocks,
+toggle gpio, etc. The power sequence off routine will close and free the
+resources, and is called when the parent is removed. And the power
+sequence suspend and resume routine can be called at host driver's
+suspend and resume routine if needed.
+
+The exported interfaces
+.. kernel-doc:: drivers/power/pwrseq/core.c
+   :export:
diff --git a/MAINTAINERS b/MAINTAINERS
index f7d568b..93b07aa 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10188,6 +10188,15 @@ F:	include/linux/pm_*
 F:	include/linux/powercap.h
 F:	drivers/powercap/
 
+POWER SEQUENCE LIBRARY
+M:	Peter Chen <Peter.Chen@nxp.com>
+T:	git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git
+L:	linux-pm@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/power/pwrseq/
+F:	drivers/power/pwrseq/
+F:	include/linux/power/pwrseq.h
+
 POWER SUPPLY CLASS/SUBSYSTEM and DRIVERS
 M:	Sebastian Reichel <sre@kernel.org>
 L:	linux-pm@vger.kernel.org
diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index 63454b5..c1bb046 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -1,3 +1,4 @@
 source "drivers/power/avs/Kconfig"
 source "drivers/power/reset/Kconfig"
 source "drivers/power/supply/Kconfig"
+source "drivers/power/pwrseq/Kconfig"
diff --git a/drivers/power/Makefile b/drivers/power/Makefile
index ff35c71..7db8035 100644
--- a/drivers/power/Makefile
+++ b/drivers/power/Makefile
@@ -1,3 +1,4 @@
 obj-$(CONFIG_POWER_AVS)		+= avs/
 obj-$(CONFIG_POWER_RESET)	+= reset/
 obj-$(CONFIG_POWER_SUPPLY)	+= supply/
+obj-$(CONFIG_POWER_SEQUENCE)	+= pwrseq/
diff --git a/drivers/power/pwrseq/Kconfig b/drivers/power/pwrseq/Kconfig
new file mode 100644
index 0000000..c6b3569
--- /dev/null
+++ b/drivers/power/pwrseq/Kconfig
@@ -0,0 +1,20 @@
+#
+# Power Sequence library
+#
+
+menuconfig POWER_SEQUENCE
+	bool "Power sequence control"
+	help
+	   It is used for drivers which needs to do power sequence
+	   (eg, turn on clock, toggle reset gpio) before the related
+	   devices can be found by hardware, eg, USB bus.
+
+if POWER_SEQUENCE
+
+config PWRSEQ_GENERIC
+	bool "Generic power sequence control"
+	depends on OF
+	help
+	   This is the generic power sequence control library, and is
+	   supposed to support common power sequence usage.
+endif
diff --git a/drivers/power/pwrseq/Makefile b/drivers/power/pwrseq/Makefile
new file mode 100644
index 0000000..ad82389
--- /dev/null
+++ b/drivers/power/pwrseq/Makefile
@@ -0,0 +1,2 @@
+obj-$(CONFIG_POWER_SEQUENCE) += core.o
+obj-$(CONFIG_PWRSEQ_GENERIC) += pwrseq_generic.o
diff --git a/drivers/power/pwrseq/core.c b/drivers/power/pwrseq/core.c
new file mode 100644
index 0000000..6b78a66
--- /dev/null
+++ b/drivers/power/pwrseq/core.c
@@ -0,0 +1,293 @@
+/*
+ * core.c	power sequence core file
+ *
+ * Copyright (C) 2016 Freescale Semiconductor, Inc.
+ * Author: Peter Chen <peter.chen@nxp.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  of
+ * the License as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.
+ */
+
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/slab.h>
+#include <linux/power/pwrseq.h>
+
+/*
+ * Static power sequence match table
+ * - Add compatible (the same with dts node) and related allocation function.
+ * - Update related binding doc.
+ */
+static const struct of_device_id pwrseq_match_table_list[] = {
+	{ .compatible = "usb424,2513", .data = &pwrseq_generic_alloc_instance},
+	{ .compatible = "usb424,2514", .data = &pwrseq_generic_alloc_instance},
+	{ /* sentinel */ }
+};
+
+static int pwrseq_get(struct device_node *np, struct pwrseq *p)
+{
+	if (p && p->get)
+		return p->get(np, p);
+
+	return -ENOTSUPP;
+}
+
+static int pwrseq_on(struct pwrseq *p)
+{
+	if (p && p->on)
+		return p->on(p);
+
+	return -ENOTSUPP;
+}
+
+static void pwrseq_off(struct pwrseq *p)
+{
+	if (p && p->off)
+		p->off(p);
+}
+
+static void pwrseq_put(struct pwrseq *p)
+{
+	if (p && p->put)
+		p->put(p);
+}
+
+/**
+ * of_pwrseq_on - Carry out power sequence on for device node
+ *
+ * @np: the device node would like to power on
+ *
+ * Carry out a single device power on.  If multiple devices
+ * need to be handled, use of_pwrseq_on_list() instead.
+ *
+ * Return a pointer to the power sequence instance on success, or NULL if
+ * not exist, or an error code on failure.
+ */
+struct pwrseq *of_pwrseq_on(struct device_node *np)
+{
+	struct pwrseq *pwrseq;
+	int ret;
+	const struct of_device_id *of_id;
+	struct pwrseq *(*alloc_instance)(void);
+
+	of_id  = of_match_node(pwrseq_match_table_list, np);
+	if (!of_id)
+		return NULL;
+
+	alloc_instance = of_id->data;
+	/* Allocate pwrseq instance */
+	pwrseq = alloc_instance();
+	if (IS_ERR(pwrseq))
+		return pwrseq;
+
+	ret = pwrseq_get(np, pwrseq);
+	if (ret)
+		goto pwr_put;
+
+	ret = pwrseq_on(pwrseq);
+	if (ret)
+		goto pwr_put;
+
+	return pwrseq;
+
+pwr_put:
+	pwrseq_put(pwrseq);
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(of_pwrseq_on);
+
+/**
+ * of_pwrseq_off - Carry out power sequence off for this pwrseq instance
+ *
+ * @pwrseq: the pwrseq instance which related device would like to be off
+ *
+ * This API is used to power off single device, it is the opposite
+ * operation for of_pwrseq_on.
+ */
+void of_pwrseq_off(struct pwrseq *pwrseq)
+{
+	pwrseq_off(pwrseq);
+	pwrseq_put(pwrseq);
+}
+EXPORT_SYMBOL_GPL(of_pwrseq_off);
+
+/**
+ * of_pwrseq_on_list - Carry out power sequence on for list
+ *
+ * @np: the device node would like to power on
+ * @head: the list head for pwrseq list on this bus
+ *
+ * This API is used to power on multiple devices at single bus.
+ * If there are several devices on bus (eg, USB bus), uses this
+ * this API. Otherwise, use of_pwrseq_on instead. After the device
+ * is powered on successfully, it will be added to pwrseq list for
+ * this bus. The caller needs to use mutex_lock for concurrent.
+ *
+ * Return 0 on success, or -ENOENT if not exist, or an error value on failure.
+ */
+int of_pwrseq_on_list(struct device_node *np, struct list_head *head)
+{
+	struct pwrseq *pwrseq;
+	struct pwrseq_list_per_dev *pwrseq_list_node;
+
+	pwrseq_list_node = kzalloc(sizeof(*pwrseq_list_node), GFP_KERNEL);
+	if (!pwrseq_list_node)
+		return -ENOMEM;
+
+	pwrseq = of_pwrseq_on(np);
+	if (!pwrseq)
+		return -ENOENT;
+
+	if (IS_ERR(pwrseq)) {
+		kfree(pwrseq_list_node);
+		return PTR_ERR(pwrseq);
+	}
+
+	pwrseq_list_node->pwrseq = pwrseq;
+	list_add(&pwrseq_list_node->list, head);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_pwrseq_on_list);
+
+/**
+ * of_pwrseq_off_list - Carry out power sequence off for the list
+ *
+ * @head: the list head for pwrseq instance list on this bus
+ *
+ * This API is used to power off all devices on this bus, it is
+ * the opposite operation for of_pwrseq_on_list.
+ * The caller needs to use mutex_lock for concurrent.
+ */
+void of_pwrseq_off_list(struct list_head *head)
+{
+	struct pwrseq *pwrseq;
+	struct pwrseq_list_per_dev *pwrseq_list_node, *tmp_node;
+
+	list_for_each_entry_safe(pwrseq_list_node, tmp_node, head, list) {
+		pwrseq = pwrseq_list_node->pwrseq;
+		of_pwrseq_off(pwrseq);
+		list_del(&pwrseq_list_node->list);
+		kfree(pwrseq_list_node);
+	}
+}
+EXPORT_SYMBOL_GPL(of_pwrseq_off_list);
+
+/**
+ * pwrseq_suspend - Carry out power sequence suspend for this pwrseq instance
+ *
+ * @pwrseq: the pwrseq instance
+ *
+ * This API is used to do suspend operation on pwrseq instance.
+ *
+ * Return 0 on success, or an error value otherwise.
+ */
+int pwrseq_suspend(struct pwrseq *p)
+{
+	int ret = 0;
+
+	if (p && p->suspend)
+		ret = p->suspend(p);
+	else
+		return ret;
+
+	if (!ret)
+		p->suspended = true;
+	else
+		pr_err("%s failed\n", __func__);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(pwrseq_suspend);
+
+/**
+ * pwrseq_resume - Carry out power sequence resume for this pwrseq instance
+ *
+ * @pwrseq: the pwrseq instance
+ *
+ * This API is used to do resume operation on pwrseq instance.
+ *
+ * Return 0 on success, or an error value otherwise.
+ */
+int pwrseq_resume(struct pwrseq *p)
+{
+	int ret = 0;
+
+	if (p && p->resume)
+		ret = p->resume(p);
+	else
+		return ret;
+
+	if (!ret)
+		p->suspended = false;
+	else
+		pr_err("%s failed\n", __func__);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(pwrseq_resume);
+
+/**
+ * pwrseq_suspend_list - Carry out power sequence suspend for list
+ *
+ * @head: the list head for pwrseq instance list on this bus
+ *
+ * This API is used to do suspend on all power sequence instances on this bus.
+ * The caller needs to use mutex_lock for concurrent.
+ */
+int pwrseq_suspend_list(struct list_head *head)
+{
+	struct pwrseq *pwrseq;
+	struct pwrseq_list_per_dev *pwrseq_list_node;
+	int ret = 0;
+
+	list_for_each_entry(pwrseq_list_node, head, list) {
+		ret = pwrseq_suspend(pwrseq_list_node->pwrseq);
+		if (ret)
+			break;
+	}
+
+	if (ret) {
+		list_for_each_entry(pwrseq_list_node, head, list) {
+			pwrseq = pwrseq_list_node->pwrseq;
+			if (pwrseq->suspended)
+				pwrseq_resume(pwrseq);
+		}
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(pwrseq_suspend_list);
+
+/**
+ * pwrseq_resume_list - Carry out power sequence resume for the list
+ *
+ * @head: the list head for pwrseq instance list on this bus
+ *
+ * This API is used to do resume on all power sequence instances on this bus.
+ * The caller needs to use mutex_lock for concurrent.
+ */
+int pwrseq_resume_list(struct list_head *head)
+{
+	struct pwrseq_list_per_dev *pwrseq_list_node;
+	int ret = 0;
+
+	list_for_each_entry(pwrseq_list_node, head, list) {
+		ret = pwrseq_resume(pwrseq_list_node->pwrseq);
+		if (ret)
+			break;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(pwrseq_resume_list);
diff --git a/drivers/power/pwrseq/pwrseq_generic.c b/drivers/power/pwrseq/pwrseq_generic.c
new file mode 100644
index 0000000..b7bbd6c5
--- /dev/null
+++ b/drivers/power/pwrseq/pwrseq_generic.c
@@ -0,0 +1,210 @@
+/*
+ * pwrseq_generic.c	Generic power sequence handling
+ *
+ * Copyright (C) 2016 Freescale Semiconductor, Inc.
+ * Author: Peter Chen <peter.chen@nxp.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  of
+ * the License as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/slab.h>
+
+#include <linux/power/pwrseq.h>
+
+struct pwrseq_generic {
+	struct pwrseq pwrseq;
+	struct gpio_desc *gpiod_reset;
+	struct clk *clks[PWRSEQ_MAX_CLKS];
+	u32 duration_us;
+	bool suspended;
+};
+
+#define to_generic_pwrseq(p) container_of(p, struct pwrseq_generic, pwrseq)
+
+static int pwrseq_generic_suspend(struct pwrseq *pwrseq)
+{
+	struct pwrseq_generic *pwrseq_gen = to_generic_pwrseq(pwrseq);
+	int clk;
+
+	for (clk = PWRSEQ_MAX_CLKS - 1; clk >= 0; clk--)
+		clk_disable_unprepare(pwrseq_gen->clks[clk]);
+
+	pwrseq_gen->suspended = true;
+	return 0;
+}
+
+static int pwrseq_generic_resume(struct pwrseq *pwrseq)
+{
+	struct pwrseq_generic *pwrseq_gen = to_generic_pwrseq(pwrseq);
+	int clk, ret = 0;
+
+	for (clk = 0; clk < PWRSEQ_MAX_CLKS && pwrseq_gen->clks[clk]; clk++) {
+		ret = clk_prepare_enable(pwrseq_gen->clks[clk]);
+		if (ret) {
+			pr_err("Can't enable clock, ret=%d\n", ret);
+			goto err_disable_clks;
+		}
+	}
+
+	pwrseq_gen->suspended = false;
+	return ret;
+
+err_disable_clks:
+	while (--clk >= 0)
+		clk_disable_unprepare(pwrseq_gen->clks[clk]);
+
+	return ret;
+}
+
+static void pwrseq_generic_put(struct pwrseq *pwrseq)
+{
+	struct pwrseq_generic *pwrseq_gen = to_generic_pwrseq(pwrseq);
+	int clk;
+
+	if (pwrseq_gen->gpiod_reset)
+		gpiod_put(pwrseq_gen->gpiod_reset);
+
+	for (clk = 0; clk < PWRSEQ_MAX_CLKS; clk++)
+		clk_put(pwrseq_gen->clks[clk]);
+
+	kfree(pwrseq_gen);
+}
+
+static void pwrseq_generic_off(struct pwrseq *pwrseq)
+{
+	struct pwrseq_generic *pwrseq_gen = to_generic_pwrseq(pwrseq);
+	int clk;
+
+	if (pwrseq_gen->suspended)
+		return;
+
+	for (clk = PWRSEQ_MAX_CLKS - 1; clk >= 0; clk--)
+		clk_disable_unprepare(pwrseq_gen->clks[clk]);
+}
+
+static int pwrseq_generic_on(struct pwrseq *pwrseq)
+{
+	struct pwrseq_generic *pwrseq_gen = to_generic_pwrseq(pwrseq);
+	int clk, ret = 0;
+	struct gpio_desc *gpiod_reset = pwrseq_gen->gpiod_reset;
+
+	for (clk = 0; clk < PWRSEQ_MAX_CLKS && pwrseq_gen->clks[clk]; clk++) {
+		ret = clk_prepare_enable(pwrseq_gen->clks[clk]);
+		if (ret) {
+			pr_err("Can't enable clock, ret=%d\n", ret);
+			goto err_disable_clks;
+		}
+	}
+
+	if (gpiod_reset) {
+		u32 duration_us = pwrseq_gen->duration_us;
+
+		if (duration_us <= 10)
+			udelay(10);
+		else
+			usleep_range(duration_us, duration_us + 100);
+		gpiod_set_value(gpiod_reset, 0);
+	}
+
+	return ret;
+
+err_disable_clks:
+	while (--clk >= 0)
+		clk_disable_unprepare(pwrseq_gen->clks[clk]);
+
+	return ret;
+}
+
+static int pwrseq_generic_get(struct device_node *np, struct pwrseq *pwrseq)
+{
+	struct pwrseq_generic *pwrseq_gen = to_generic_pwrseq(pwrseq);
+	enum of_gpio_flags flags;
+	int reset_gpio, clk, ret = 0;
+
+	for (clk = 0; clk < PWRSEQ_MAX_CLKS; clk++) {
+		pwrseq_gen->clks[clk] = of_clk_get(np, clk);
+		if (IS_ERR(pwrseq_gen->clks[clk])) {
+			ret = PTR_ERR(pwrseq_gen->clks[clk]);
+			if (ret != -ENOENT)
+				goto err_put_clks;
+			pwrseq_gen->clks[clk] = NULL;
+			break;
+		}
+	}
+
+	reset_gpio = of_get_named_gpio_flags(np, "reset-gpios", 0, &flags);
+	if (gpio_is_valid(reset_gpio)) {
+		unsigned long gpio_flags;
+
+		if (flags & OF_GPIO_ACTIVE_LOW)
+			gpio_flags = GPIOF_ACTIVE_LOW | GPIOF_OUT_INIT_LOW;
+		else
+			gpio_flags = GPIOF_OUT_INIT_HIGH;
+
+		ret = gpio_request_one(reset_gpio, gpio_flags,
+				"pwrseq-reset-gpios");
+		if (ret)
+			goto err_put_clks;
+
+		pwrseq_gen->gpiod_reset = gpio_to_desc(reset_gpio);
+		of_property_read_u32(np, "reset-duration-us",
+				&pwrseq_gen->duration_us);
+	} else if (reset_gpio == -ENOENT) {
+		; /* no such gpio */
+	} else {
+		ret = reset_gpio;
+		pr_err("Failed to get reset gpio on %s, err = %d\n",
+				np->full_name, reset_gpio);
+		goto err_put_clks;
+	}
+
+	return 0;
+
+err_put_clks:
+	while (--clk >= 0)
+		clk_put(pwrseq_gen->clks[clk]);
+	return ret;
+}
+
+/**
+ * pwrseq_generic_alloc_instance - power sequence instance allocation
+ *
+ * This function is used to allocate one generic power sequence instance,
+ * it is called when the system boots up and after one power sequence
+ * instance is got successfully.
+ *
+ * Return zero on success or an error code otherwise.
+ */
+struct pwrseq *pwrseq_generic_alloc_instance(void)
+{
+	struct pwrseq_generic *pwrseq_gen;
+
+	pwrseq_gen = kzalloc(sizeof(*pwrseq_gen), GFP_KERNEL);
+	if (!pwrseq_gen)
+		return ERR_PTR(-ENOMEM);
+
+	pwrseq_gen->pwrseq.get = pwrseq_generic_get;
+	pwrseq_gen->pwrseq.on = pwrseq_generic_on;
+	pwrseq_gen->pwrseq.off = pwrseq_generic_off;
+	pwrseq_gen->pwrseq.put = pwrseq_generic_put;
+	pwrseq_gen->pwrseq.suspend = pwrseq_generic_suspend;
+	pwrseq_gen->pwrseq.resume = pwrseq_generic_resume;
+
+	return &pwrseq_gen->pwrseq;
+}
diff --git a/include/linux/power/pwrseq.h b/include/linux/power/pwrseq.h
new file mode 100644
index 0000000..c5b278f
--- /dev/null
+++ b/include/linux/power/pwrseq.h
@@ -0,0 +1,84 @@
+#ifndef __LINUX_PWRSEQ_H
+#define __LINUX_PWRSEQ_H
+
+#include <linux/of.h>
+
+#define PWRSEQ_MAX_CLKS		3
+
+/**
+ * struct pwrseq - the power sequence structure
+ * @pwrseq_of_match_table: the OF device id table this pwrseq library supports
+ * @node: the list pointer to be added to pwrseq list
+ * @get: the API is used to get pwrseq instance from the device node
+ * @on: do power on for this pwrseq instance
+ * @off: do power off for this pwrseq instance
+ * @put: release the resources on this pwrseq instance
+ * @suspend: do suspend operation on this pwrseq instance
+ * @resume: do resume operation on this pwrseq instance
+ */
+struct pwrseq {
+	const struct of_device_id *pwrseq_of_match_table;
+	struct list_head node;
+	int (*get)(struct device_node *np, struct pwrseq *p);
+	int (*on)(struct pwrseq *p);
+	void (*off)(struct pwrseq *p);
+	void (*put)(struct pwrseq *p);
+	int (*suspend)(struct pwrseq *p);
+	int (*resume)(struct pwrseq *p);
+	bool suspended;
+};
+
+/* used for power sequence instance list in one driver */
+struct pwrseq_list_per_dev {
+	struct pwrseq *pwrseq;
+	struct list_head list;
+};
+
+#if IS_ENABLED(CONFIG_POWER_SEQUENCE)
+struct pwrseq *of_pwrseq_on(struct device_node *np);
+void of_pwrseq_off(struct pwrseq *pwrseq);
+int of_pwrseq_on_list(struct device_node *np, struct list_head *head);
+void of_pwrseq_off_list(struct list_head *head);
+int pwrseq_suspend(struct pwrseq *p);
+int pwrseq_resume(struct pwrseq *p);
+int pwrseq_suspend_list(struct list_head *head);
+int pwrseq_resume_list(struct list_head *head);
+#else
+static inline struct pwrseq *of_pwrseq_on(struct device_node *np)
+{
+	return NULL;
+}
+static void of_pwrseq_off(struct pwrseq *pwrseq) {}
+static int of_pwrseq_on_list(struct device_node *np, struct list_head *head)
+{
+	return 0;
+}
+static void of_pwrseq_off_list(struct list_head *head) {}
+static int pwrseq_suspend(struct pwrseq *p)
+{
+	return 0;
+}
+static int pwrseq_resume(struct pwrseq *p)
+{
+	return 0;
+}
+static int pwrseq_suspend_list(struct list_head *head)
+{
+	return 0;
+}
+static int pwrseq_resume_list(struct list_head *head)
+{
+	return 0;
+}
+#endif /* CONFIG_POWER_SEQUENCE */
+
+#if IS_ENABLED(CONFIG_PWRSEQ_GENERIC)
+extern struct pwrseq *pwrseq_generic_alloc_instance(void);
+#else
+static inline struct pwrseq *pwrseq_generic_alloc_instance(void)
+{
+	return ERR_PTR(-ENOTSUPP);
+}
+#endif /* CONFIG_PWRSEQ_GENERIC */
+
+#endif  /* __LINUX_PWRSEQ_H */
-- 
2.7.4

[PATCH v16 3/7] binding-doc: usb: usb-device: add optional properties for power sequence

[Peter Chen]

Add optional properties for power sequence.

Signed-off-by: Peter Chen <peter.chen@nxp.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/usb/usb-device.txt | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/usb/usb-device.txt b/Documentation/devicetree/bindings/usb/usb-device.txt
index 1c35e7b..3661dd2 100644
--- a/Documentation/devicetree/bindings/usb/usb-device.txt
+++ b/Documentation/devicetree/bindings/usb/usb-device.txt
@@ -13,6 +13,10 @@ Required properties:
 - reg: the port number which this device is connecting to, the range
   is 1-31.
 
+Optional properties:
+power sequence properties, see
+Documentation/devicetree/bindings/power/pwrseq/pwrseq-generic.txt for detail
+
 Example:
 
 &usb1 {
@@ -21,8 +25,12 @@ Example:
 	#address-cells = <1>;
 	#size-cells = <0>;
 
-	hub: genesys@1 {
+	genesys: hub@1 {
 		compatible = "usb5e3,608";
 		reg = <1>;
+
+		clocks = <&clks IMX6SX_CLK_CKO>;
+		reset-gpios = <&gpio4 5 GPIO_ACTIVE_LOW>; /* hub reset pin */
+		reset-duration-us = <10>;
 	};
 }
-- 
2.7.4

[PATCH v16 4/7] usb: core: add power sequence handling for USB devices

[Peter Chen]

Some hard-wired USB devices need to do power sequence to let the
device work normally, the typical power sequence like: enable USB
PHY clock, toggle reset pin, etc. But current Linux USB driver
lacks of such code to do it, it may cause some hard-wired USB devices
works abnormal or can't be recognized by controller at all.

In this patch, it calls power sequence library APIs to finish
the power sequence events. It will do power on sequence at hub's
probe for all devices under this hub (includes root hub).
At hub_disconnect, it will do power off sequence which is at powered
on list.

Signed-off-by: Peter Chen <peter.chen@nxp.com>
Tested-by Joshua Clayton <stillcompiling@gmail.com>
Tested-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
Reviewed-by: Vaibhav Hiremath <hvaibhav.linux@gmail.com>
Acked-by: Alan Stern <stern@rowland.harvard.edu>
---
 drivers/usb/Kconfig    |  1 +
 drivers/usb/core/hub.c | 49 +++++++++++++++++++++++++++++++++++++++++++++----
 drivers/usb/core/hub.h |  1 +
 3 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
index 939a63b..b6f626e 100644
--- a/drivers/usb/Kconfig
+++ b/drivers/usb/Kconfig
@@ -39,6 +39,7 @@ config USB
 	tristate "Support for Host-side USB"
 	depends on USB_ARCH_HAS_HCD
 	select USB_COMMON
+	select POWER_SEQUENCE
 	select NLS  # for UTF-8 strings
 	---help---
 	  Universal Serial Bus (USB) is a specification for a serial bus
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 9dca59e..0439d4f 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -28,6 +28,7 @@
 #include <linux/mutex.h>
 #include <linux/random.h>
 #include <linux/pm_qos.h>
+#include <linux/power/pwrseq.h>
 
 #include <linux/uaccess.h>
 #include <asm/byteorder.h>
@@ -1619,6 +1620,7 @@ static void hub_disconnect(struct usb_interface *intf)
 	hub->error = 0;
 	hub_quiesce(hub, HUB_DISCONNECT);
 
+	of_pwrseq_off_list(&hub->pwrseq_list);
 	mutex_lock(&usb_port_peer_mutex);
 
 	/* Avoid races with recursively_mark_NOTATTACHED() */
@@ -1646,12 +1648,42 @@ static void hub_disconnect(struct usb_interface *intf)
 	kref_put(&hub->kref, hub_release);
 }
 
+#ifdef CONFIG_OF
+static int hub_of_pwrseq_on(struct usb_hub *hub)
+{
+	struct device *parent;
+	struct usb_device *hdev = hub->hdev;
+	struct device_node *np;
+	int ret;
+
+	if (hdev->parent)
+		parent = &hdev->dev;
+	else
+		parent = bus_to_hcd(hdev->bus)->self.sysdev;
+
+	for_each_child_of_node(parent->of_node, np) {
+		ret = of_pwrseq_on_list(np, &hub->pwrseq_list);
+		/* Maybe no power sequence library is chosen */
+		if (ret && ret != -ENOENT)
+			return ret;
+	}
+
+	return 0;
+}
+#else
+static int hub_of_pwrseq_on(struct usb_hub *hub)
+{
+	return 0;
+}
+#endif
+
 static int hub_probe(struct usb_interface *intf, const struct usb_device_id *id)
 {
 	struct usb_host_interface *desc;
 	struct usb_endpoint_descriptor *endpoint;
 	struct usb_device *hdev;
 	struct usb_hub *hub;
+	int ret = -ENODEV;
 
 	desc = intf->cur_altsetting;
 	hdev = interface_to_usbdev(intf);
@@ -1756,6 +1788,7 @@ static int hub_probe(struct usb_interface *intf, const struct usb_device_id *id)
 	INIT_DELAYED_WORK(&hub->leds, led_work);
 	INIT_DELAYED_WORK(&hub->init_work, NULL);
 	INIT_WORK(&hub->events, hub_event);
+	INIT_LIST_HEAD(&hub->pwrseq_list);
 	usb_get_intf(intf);
 	usb_get_dev(hdev);
 
@@ -1769,11 +1802,14 @@ static int hub_probe(struct usb_interface *intf, const struct usb_device_id *id)
 	if (id->driver_info & HUB_QUIRK_CHECK_PORT_AUTOSUSPEND)
 		hub->quirk_check_port_auto_suspend = 1;
 
-	if (hub_configure(hub, endpoint) >= 0)
-		return 0;
+	if (hub_configure(hub, endpoint) >= 0) {
+		ret = hub_of_pwrseq_on(hub);
+		if (!ret)
+			return 0;
+	}
 
 	hub_disconnect(intf);
-	return -ENODEV;
+	return ret;
 }
 
 static int
@@ -3593,14 +3629,19 @@ static int hub_suspend(struct usb_interface *intf, pm_message_t msg)
 
 	/* stop hub_wq and related activity */
 	hub_quiesce(hub, HUB_SUSPEND);
-	return 0;
+	return pwrseq_suspend_list(&hub->pwrseq_list);
 }
 
 static int hub_resume(struct usb_interface *intf)
 {
 	struct usb_hub *hub = usb_get_intfdata(intf);
+	int ret;
 
 	dev_dbg(&intf->dev, "%s\n", __func__);
+	ret = pwrseq_resume_list(&hub->pwrseq_list);
+	if (ret)
+		return ret;
+
 	hub_activate(hub, HUB_RESUME);
 	return 0;
 }
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index 34c1a7e..27f56a6 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -78,6 +78,7 @@ struct usb_hub {
 	struct delayed_work	init_work;
 	struct work_struct      events;
 	struct usb_port		**ports;
+	struct list_head	pwrseq_list; /* powered pwrseq node list */
 };
 
 /**
-- 
2.7.4

[PATCH v16 5/7] ARM: dts: imx6qdl: Enable usb node children with <reg>

[Peter Chen]

From: Joshua Clayton <stillcompiling@gmail.com>

Give usb nodes #address and #size attributes, so that a child node
representing a permanently connected device such as an onboard hub may
be addressed with a <reg> attribute

Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 arch/arm/boot/dts/imx6qdl.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
index e426faa..8c064cb 100644
--- a/arch/arm/boot/dts/imx6qdl.dtsi
+++ b/arch/arm/boot/dts/imx6qdl.dtsi
@@ -945,6 +945,8 @@
 
 			usbh1: usb@02184200 {
 				compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
+				#address-cells = <1>;
+				#size-cells = <0>;
 				reg = <0x02184200 0x200>;
 				interrupts = <0 40 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&clks IMX6QDL_CLK_USBOH3>;
@@ -959,6 +961,8 @@
 
 			usbh2: usb@02184400 {
 				compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
+				#address-cells = <1>;
+				#size-cells = <0>;
 				reg = <0x02184400 0x200>;
 				interrupts = <0 41 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&clks IMX6QDL_CLK_USBOH3>;
@@ -972,6 +976,8 @@
 
 			usbh3: usb@02184600 {
 				compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
+				#address-cells = <1>;
+				#size-cells = <0>;
 				reg = <0x02184600 0x200>;
 				interrupts = <0 42 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&clks IMX6QDL_CLK_USBOH3>;
-- 
2.7.4

[PATCH v16 6/7] ARM: dts: imx6qdl-udoo.dtsi: fix onboard USB HUB property

[Peter Chen]

The current dts describes USB HUB's property at USB controller's
entry, it is improper. The USB HUB should be the child node
under USB controller, and power sequence properties are under
it. Besides, using gpio pinctrl setting for USB2415's reset pin.

Signed-off-by: Peter Chen <peter.chen@nxp.com>
Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
Tested-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
---
 arch/arm/boot/dts/imx6qdl-udoo.dtsi | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/arch/arm/boot/dts/imx6qdl-udoo.dtsi b/arch/arm/boot/dts/imx6qdl-udoo.dtsi
index c96c91d..a173de2 100644
--- a/arch/arm/boot/dts/imx6qdl-udoo.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-udoo.dtsi
@@ -9,6 +9,8 @@
  *
  */
 
+#include <dt-bindings/gpio/gpio.h>
+
 / {
 	aliases {
 		backlight = &backlight;
@@ -58,17 +60,6 @@
 		#address-cells = <1>;
 		#size-cells = <0>;
 
-		reg_usb_h1_vbus: regulator@0 {
-			compatible = "regulator-fixed";
-			reg = <0>;
-			regulator-name = "usb_h1_vbus";
-			regulator-min-microvolt = <5000000>;
-			regulator-max-microvolt = <5000000>;
-			enable-active-high;
-			startup-delay-us = <2>; /* USB2415 requires a POR of 1 us minimum */
-			gpio = <&gpio7 12 0>;
-		};
-
 		reg_panel: regulator@1 {
 			compatible = "regulator-fixed";
 			reg = <1>;
@@ -188,7 +179,7 @@
 
 		pinctrl_usbh: usbhgrp {
 			fsl,pins = <
-				MX6QDL_PAD_GPIO_17__GPIO7_IO12 0x80000000
+				MX6QDL_PAD_GPIO_17__GPIO7_IO12	0x1b0b0
 				MX6QDL_PAD_NANDF_CS2__CCM_CLKO2 0x130b0
 			>;
 		};
@@ -259,9 +250,16 @@
 &usbh1 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_usbh>;
-	vbus-supply = <&reg_usb_h1_vbus>;
-	clocks = <&clks IMX6QDL_CLK_CKO>;
 	status = "okay";
+
+	usb2415: hub@1 {
+		compatible = "usb424,2514";
+		reg = <1>;
+
+		clocks = <&clks IMX6QDL_CLK_CKO>;
+		reset-gpios = <&gpio7 12 GPIO_ACTIVE_LOW>;
+		reset-duration-us = <3000>;
+	};
 };
 
 &usdhc3 {
-- 
2.7.4

[PATCH v16 7/7] ARM: dts: imx6q-evi: Fix onboard hub reset line

[Peter Chen]

From: Joshua Clayton <stillcompiling@gmail.com>

Previously the onboard hub was made to work by treating its
reset gpio as a regulator enable.
Get rid of that kludge now that pwseq has added reset gpio support
Move pin muxing the hub reset pin into the usbh1 group

Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 arch/arm/boot/dts/imx6q-evi.dts | 25 +++++++------------------
 1 file changed, 7 insertions(+), 18 deletions(-)

diff --git a/arch/arm/boot/dts/imx6q-evi.dts b/arch/arm/boot/dts/imx6q-evi.dts
index fd2220a..24fe093 100644
--- a/arch/arm/boot/dts/imx6q-evi.dts
+++ b/arch/arm/boot/dts/imx6q-evi.dts
@@ -54,18 +54,6 @@
 		reg = <0x10000000 0x40000000>;
 	};
 
-	reg_usbh1_vbus: regulator-usbhubreset {
-		compatible = "regulator-fixed";
-		regulator-name = "usbh1_vbus";
-		regulator-min-microvolt = <5000000>;
-		regulator-max-microvolt = <5000000>;
-		enable-active-high;
-		startup-delay-us = <2>;
-		pinctrl-names = "default";
-		pinctrl-0 = <&pinctrl_usbh1_hubreset>;
-		gpio = <&gpio7 12 GPIO_ACTIVE_HIGH>;
-	};
-
 	reg_usb_otg_vbus: regulator-usbotgvbus {
 		compatible = "regulator-fixed";
 		regulator-name = "usb_otg_vbus";
@@ -204,12 +192,18 @@
 };
 
 &usbh1 {
-	vbus-supply = <&reg_usbh1_vbus>;
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_usbh1>;
 	dr_mode = "host";
 	disable-over-current;
 	status = "okay";
+
+	usb2415host: hub@1 {
+		compatible = "usb424,2513";
+		reg = <1>;
+		reset-gpios = <&gpio7 12 GPIO_ACTIVE_LOW>;
+		reset-duration-us = <3000>;
+	};
 };
 
 &usbotg {
@@ -465,11 +459,6 @@
 			MX6QDL_PAD_GPIO_3__USB_H1_OC 0x1b0b0
 			/* usbh1_b OC */
 			MX6QDL_PAD_GPIO_0__GPIO1_IO00 0x1b0b0
-		>;
-	};
-
-	pinctrl_usbh1_hubreset: usbh1hubresetgrp {
-		fsl,pins = <
 			MX6QDL_PAD_GPIO_17__GPIO7_IO12 0x1b0b0
 		>;
 	};
-- 
2.7.4

Re: [PATCH v16 2/7] power: add power sequence library

[Rafael J. Wysocki]

On Wednesday, June 21, 2017 02:42:03 PM Peter Chen wrote:
> We have an well-known problem that the device needs to do some power
> sequence before it can be recognized by related host, the typical
> example like hard-wired mmc devices and usb devices.
> 
> This power sequence is hard to be described at device tree and handled by
> related host driver, so we have created a common power sequence
> library to cover this requirement. The core code has supplied
> some common helpers for host driver, and individual power sequence
> libraries handle kinds of power sequence for devices. The pwrseq
> librares always need to allocate extra instance for compatible
> string match.
> 
> pwrseq_generic is intended for general purpose of power sequence, which
> handles gpios and clocks currently, and can cover other controls in
> future. The host driver just needs to call of_pwrseq_on/of_pwrseq_off
> if only one power sequence is needed, else call of_pwrseq_on_list
> /of_pwrseq_off_list instead (eg, USB hub driver).
> 
> For new power sequence library, it needs to add its compatible string
> and allocation function at pwrseq_match_table_list, then the pwrseq
> core will match it with DT's, and choose this library at runtime.
> 
> Signed-off-by: Peter Chen <peter.chen@nxp.com>
> Tested-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> Tested-by Joshua Clayton <stillcompiling@gmail.com>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> Tested-by: Matthias Kaehlcke <mka@chromium.org>

The executive summary here is that I'm not going to apply this patch (and the
following series depending on it), because in my opinion it is misguided and
I quite fundamentally disagree with the basic concept.

The code has problems too, but we first need to agree on the basics.

> ---
>  Documentation/power/power-sequence/design.rst |  54 +++++
>  MAINTAINERS                                   |   9 +
>  drivers/power/Kconfig                         |   1 +
>  drivers/power/Makefile                        |   1 +
>  drivers/power/pwrseq/Kconfig                  |  20 ++
>  drivers/power/pwrseq/Makefile                 |   2 +
>  drivers/power/pwrseq/core.c                   | 293 ++++++++++++++++++++++++++
>  drivers/power/pwrseq/pwrseq_generic.c         | 210 ++++++++++++++++++
>  include/linux/power/pwrseq.h                  |  84 ++++++++
>  9 files changed, 674 insertions(+)
>  create mode 100644 Documentation/power/power-sequence/design.rst
>  create mode 100644 drivers/power/pwrseq/Kconfig
>  create mode 100644 drivers/power/pwrseq/Makefile
>  create mode 100644 drivers/power/pwrseq/core.c
>  create mode 100644 drivers/power/pwrseq/pwrseq_generic.c
>  create mode 100644 include/linux/power/pwrseq.h
> 
> diff --git a/Documentation/power/power-sequence/design.rst b/Documentation/power/power-sequence/design.rst
> new file mode 100644
> index 0000000..554608e
> --- /dev/null
> +++ b/Documentation/power/power-sequence/design.rst
> @@ -0,0 +1,54 @@
> +====================================
> +Power Sequence Library
> +====================================
> +
> +:Date: Feb, 2017
> +:Author: Peter Chen <peter.chen@nxp.com>
> +
> +
> +Introduction
> +============
> +
> +We have an well-known problem that the device needs to do a power
> +sequence before it can be recognized by related host, the typical
> +examples are hard-wired mmc devices and usb devices. The host controller
> +can't know what kinds of this device is in its bus if the power
> +sequence has not done, since the related devices driver's probe calling
> +is determined by runtime according to eunumeration results. Besides,
> +the devices may have custom power sequence, so the power sequence library
> +which is independent with the devices is needed.

In other words, devices can be in different power states and there are power
states in which they are not accessible to software.  In order to be accessed
by software, they need to be put into power states in which that is actually
possible.

That has been known for over 20 years and by no means it is limited to MMC
or USB.

Moreover, power states of devices generally depend on the platform and if
the same IP block is used in two different platforms (or even in two different
SoCs for that matter), the power states of it can be defined differently in
each case.

It also is quite well known how to define a power state of a device.  There
usually is a list of power resources (clocks, regulators, etc) that have to
be "on" for the device to be in the given power state.  In addition to that,
it may be necessary to carry out some operations after turning those
power resources "on" in order to trigger the power state change (like
toggling a GPIO or writing to a specific register etc).

Now, of course, it is not generally guaranteed that devices will be in
software-accessible power states initially, so it is necessary to put them
into those power states before trying to access them.

Again, this isn't anything new, but it has always been about power states.

By defining a "power sequence" you generally define two power states for
the device, say "on" and "off", and a way to switch between them, but what
about simply treating power states as the primary concept in the first place?

That would be more general, because there are platforms with more than
two power states per device.  In those cases, the power state choice may
depend on additional factors, like say during system suspend it may matter
whether or not the device is a wakeup one (and it may not be able to
generate wakeup signals from certain power states).  Using a single "power
sequence" would not be sufficient then.

> +
> +Design
> +============
> +
> +The power sequence library includes the core file and customer power
> +sequence library. The core file exports interfaces are called by
> +host controller driver for power sequence and customer power sequence
> +library files to register its power sequence instance to global
> +power sequence list. The custom power sequence library creates power
> +sequence instance and implement custom power sequence.

The above paragraph is really hard to decode.

There is no explanation of basic concepts at all.  What exactly do you mean by
"power sequence"?  What is a "power sequence instance"?  What do you mean
by "custom power sequence"?

> +
> +Since the power sequence describes hardware design, the description is
> +located at board description file, eg, device tree dts file. And
> +a specific power sequence belongs to device, so its description
> +is under the device node, please refer to:
> +Documentation/devicetree/bindings/power/pwrseq/pwrseq-generic.txt

This, again, is really difficult to understand, especially the difference between
"the power sequence" and "a specific power sequence" is quite unclear.

> +
> +Custom power sequence library allocates one power sequence instance at
> +bootup periods using postcore_initcall, this static allocated instance is
> +used to compare with device-tree (DT) node to see if this library can be
> +used for the node or not.

Same here.

> When the result is matched, the core API will
> +try to get resourses (->get, implemented at each library) for power
> +sequence, if all resources are got, it will try to allocate another
> +instance for next possible request from host driver.
> +
> +Then, the host controller driver can carry out power sequence on for this
> +DT node, the library will do corresponding operations, like open clocks,
> +toggle gpio, etc. The power sequence off routine will close and free the
> +resources, and is called when the parent is removed. And the power
> +sequence suspend and resume routine can be called at host driver's
> +suspend and resume routine if needed.

Let me try to say what I understood from this.

If there is a DT node with a matching "compatible" string, the initialization
code will try to acquire resources needed for "power sequencing" of the
corresponding device.  It is expected that host controller drivers will
then use the provided helper functions to turn the subordinate devices
"on" during enumeration and "off" on the (host controller) driver removal.
They also are expected to use the "suspend" and "resume" helpers when
the host controller is suspended or resumed, respectively.

If the above is what you intended, then it is not the right model IMO.

In particular, I don't see why the controller driver suspend and resume
should operate "power sequences" of subordinate devices directly as the
subordinate devices are expected to be suspended (in which case they
also should have been turned "off" already) when the controller is
suspending and the controller resume need not affect the subordinate
devices at all.

Moreover, it assumes that all of the platforms including the IP block driven
by the controller driver in question will use the "power sequences" model,
but what if there are more generally defined power domains on them?

It looks like the power management should really be carried out by an
additional layer of code to avoid imposing platform dependencies on
controller drivers.

Further, what if there are "power sequences" for the host controllers
themselves?  Who's expected to operate those "power sequences" then?

> +
> +The exported interfaces
> +.. kernel-doc:: drivers/power/pwrseq/core.c
> +   :export:
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f7d568b..93b07aa 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10188,6 +10188,15 @@ F:	include/linux/pm_*
>  F:	include/linux/powercap.h
>  F:	drivers/powercap/
>  
> +POWER SEQUENCE LIBRARY
> +M:	Peter Chen <Peter.Chen@nxp.com>
> +T:	git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git
> +L:	linux-pm@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/power/pwrseq/
> +F:	drivers/power/pwrseq/
> +F:	include/linux/power/pwrseq.h
> +
>  POWER SUPPLY CLASS/SUBSYSTEM and DRIVERS
>  M:	Sebastian Reichel <sre@kernel.org>
>  L:	linux-pm@vger.kernel.org
> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> index 63454b5..c1bb046 100644
> --- a/drivers/power/Kconfig
> +++ b/drivers/power/Kconfig
> @@ -1,3 +1,4 @@
>  source "drivers/power/avs/Kconfig"
>  source "drivers/power/reset/Kconfig"
>  source "drivers/power/supply/Kconfig"
> +source "drivers/power/pwrseq/Kconfig"
> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
> index ff35c71..7db8035 100644
> --- a/drivers/power/Makefile
> +++ b/drivers/power/Makefile
> @@ -1,3 +1,4 @@
>  obj-$(CONFIG_POWER_AVS)		+= avs/
>  obj-$(CONFIG_POWER_RESET)	+= reset/
>  obj-$(CONFIG_POWER_SUPPLY)	+= supply/
> +obj-$(CONFIG_POWER_SEQUENCE)	+= pwrseq/
> diff --git a/drivers/power/pwrseq/Kconfig b/drivers/power/pwrseq/Kconfig
> new file mode 100644
> index 0000000..c6b3569
> --- /dev/null
> +++ b/drivers/power/pwrseq/Kconfig
> @@ -0,0 +1,20 @@
> +#
> +# Power Sequence library
> +#
> +
> +menuconfig POWER_SEQUENCE
> +	bool "Power sequence control"
> +	help
> +	   It is used for drivers which needs to do power sequence
> +	   (eg, turn on clock, toggle reset gpio) before the related
> +	   devices can be found by hardware, eg, USB bus.

So the majority of people trying to configure their kernels will have no idea
whether or not they should enable this option.

Ideally, it should be clear from the description when the option is needed
or it should just be selected automatically by configurations in which it may
be necessary in principle.

> +
> +if POWER_SEQUENCE
> +
> +config PWRSEQ_GENERIC
> +	bool "Generic power sequence control"
> +	depends on OF
> +	help
> +	   This is the generic power sequence control library, and is
> +	   supposed to support common power sequence usage.

This is total fluff.  Either change the description to be more specific, so
anyone can understand it, or just don't add the config option at all.

> +endif
> diff --git a/drivers/power/pwrseq/Makefile b/drivers/power/pwrseq/Makefile
> new file mode 100644
> index 0000000..ad82389
> --- /dev/null
> +++ b/drivers/power/pwrseq/Makefile
> @@ -0,0 +1,2 @@
> +obj-$(CONFIG_POWER_SEQUENCE) += core.o
> +obj-$(CONFIG_PWRSEQ_GENERIC) += pwrseq_generic.o
> diff --git a/drivers/power/pwrseq/core.c b/drivers/power/pwrseq/core.c
> new file mode 100644
> index 0000000..6b78a66
> --- /dev/null
> +++ b/drivers/power/pwrseq/core.c
> @@ -0,0 +1,293 @@
> +/*
> + * core.c	power sequence core file
> + *
> + * Copyright (C) 2016 Freescale Semiconductor, Inc.
> + * Author: Peter Chen <peter.chen@nxp.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  of
> + * the License as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.
> + */
> +
> +#include <linux/list.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +#include <linux/power/pwrseq.h>
> +
> +/*
> + * Static power sequence match table
> + * - Add compatible (the same with dts node) and related allocation function.
> + * - Update related binding doc.
> + */
> +static const struct of_device_id pwrseq_match_table_list[] = {
> +	{ .compatible = "usb424,2513", .data = &pwrseq_generic_alloc_instance},
> +	{ .compatible = "usb424,2514", .data = &pwrseq_generic_alloc_instance},
> +	{ /* sentinel */ }
> +};

So what exactly is the purpose of this?

> +
> +static int pwrseq_get(struct device_node *np, struct pwrseq *p)
> +{
> +	if (p && p->get)
> +		return p->get(np, p);
> +
> +	return -ENOTSUPP;
> +}
> +
> +static int pwrseq_on(struct pwrseq *p)
> +{
> +	if (p && p->on)
> +		return p->on(p);
> +
> +	return -ENOTSUPP;
> +}
> +
> +static void pwrseq_off(struct pwrseq *p)
> +{
> +	if (p && p->off)
> +		p->off(p);
> +}
> +
> +static void pwrseq_put(struct pwrseq *p)
> +{
> +	if (p && p->put)
> +		p->put(p);
> +}
> +
> +/**
> + * of_pwrseq_on - Carry out power sequence on for device node
> + *
> + * @np: the device node would like to power on
> + *
> + * Carry out a single device power on.  If multiple devices
> + * need to be handled, use of_pwrseq_on_list() instead.
> + *
> + * Return a pointer to the power sequence instance on success, or NULL if
> + * not exist, or an error code on failure.
> + */

So I guess, because that hasn't been clearly stated anywhere, that host
controller drivers are expected to call this routine in order to turn the
subordinate devices "on" before probing them in case they need to be
"power sequenced".

> +struct pwrseq *of_pwrseq_on(struct device_node *np)
> +{
> +	struct pwrseq *pwrseq;
> +	int ret;
> +	const struct of_device_id *of_id;
> +	struct pwrseq *(*alloc_instance)(void);
> +
> +	of_id  = of_match_node(pwrseq_match_table_list, np);
> +	if (!of_id)
> +		return NULL;
> +
> +	alloc_instance = of_id->data;
> +	/* Allocate pwrseq instance */
> +	pwrseq = alloc_instance();
> +	if (IS_ERR(pwrseq))
> +		return pwrseq;
> +
> +	ret = pwrseq_get(np, pwrseq);
> +	if (ret)
> +		goto pwr_put;
> +
> +	ret = pwrseq_on(pwrseq);
> +	if (ret)
> +		goto pwr_put;
> +
> +	return pwrseq;
> +
> +pwr_put:
> +	pwrseq_put(pwrseq);
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(of_pwrseq_on);
> +
> +/**
> + * of_pwrseq_off - Carry out power sequence off for this pwrseq instance
> + *
> + * @pwrseq: the pwrseq instance which related device would like to be off
> + *
> + * This API is used to power off single device, it is the opposite
> + * operation for of_pwrseq_on.
> + */
> +void of_pwrseq_off(struct pwrseq *pwrseq)
> +{
> +	pwrseq_off(pwrseq);
> +	pwrseq_put(pwrseq);
> +}
> +EXPORT_SYMBOL_GPL(of_pwrseq_off);
> +
> +/**
> + * of_pwrseq_on_list - Carry out power sequence on for list
> + *
> + * @np: the device node would like to power on
> + * @head: the list head for pwrseq list on this bus
> + *
> + * This API is used to power on multiple devices at single bus.
> + * If there are several devices on bus (eg, USB bus), uses this
> + * this API. Otherwise, use of_pwrseq_on instead. After the device
> + * is powered on successfully, it will be added to pwrseq list for
> + * this bus. The caller needs to use mutex_lock for concurrent.
> + *
> + * Return 0 on success, or -ENOENT if not exist, or an error value on failure.
> + */

Here the caller seems to be expected to allocate a list_head upfront and then
pass it to every invocation of this routine in order to create a list of devices
that have been turned "on".

Also, this is expected to be called by controller drivers for the subordinate
devices on the bus before probing those devices and the callers are
responsible for mutual exclusion.

> +int of_pwrseq_on_list(struct device_node *np, struct list_head *head)
> +{
> +	struct pwrseq *pwrseq;
> +	struct pwrseq_list_per_dev *pwrseq_list_node;
> +
> +	pwrseq_list_node = kzalloc(sizeof(*pwrseq_list_node), GFP_KERNEL);
> +	if (!pwrseq_list_node)
> +		return -ENOMEM;
> +
> +	pwrseq = of_pwrseq_on(np);
> +	if (!pwrseq)
> +		return -ENOENT;
> +
> +	if (IS_ERR(pwrseq)) {
> +		kfree(pwrseq_list_node);
> +		return PTR_ERR(pwrseq);
> +	}
> +
> +	pwrseq_list_node->pwrseq = pwrseq;
> +	list_add(&pwrseq_list_node->list, head);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_pwrseq_on_list);
> +
> +/**
> + * of_pwrseq_off_list - Carry out power sequence off for the list
> + *
> + * @head: the list head for pwrseq instance list on this bus
> + *
> + * This API is used to power off all devices on this bus, it is
> + * the opposite operation for of_pwrseq_on_list.
> + * The caller needs to use mutex_lock for concurrent.
> + */
> +void of_pwrseq_off_list(struct list_head *head)
> +{
> +	struct pwrseq *pwrseq;
> +	struct pwrseq_list_per_dev *pwrseq_list_node, *tmp_node;
> +
> +	list_for_each_entry_safe(pwrseq_list_node, tmp_node, head, list) {
> +		pwrseq = pwrseq_list_node->pwrseq;
> +		of_pwrseq_off(pwrseq);
> +		list_del(&pwrseq_list_node->list);
> +		kfree(pwrseq_list_node);
> +	}
> +}
> +EXPORT_SYMBOL_GPL(of_pwrseq_off_list);
> +
> +/**
> + * pwrseq_suspend - Carry out power sequence suspend for this pwrseq instance
> + *
> + * @pwrseq: the pwrseq instance
> + *
> + * This API is used to do suspend operation on pwrseq instance.
> + *
> + * Return 0 on success, or an error value otherwise.
> + */
> +int pwrseq_suspend(struct pwrseq *p)
> +{
> +	int ret = 0;
> +
> +	if (p && p->suspend)
> +		ret = p->suspend(p);
> +	else
> +		return ret;
> +
> +	if (!ret)
> +		p->suspended = true;
> +	else
> +		pr_err("%s failed\n", __func__);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(pwrseq_suspend);
> +
> +/**
> + * pwrseq_resume - Carry out power sequence resume for this pwrseq instance
> + *
> + * @pwrseq: the pwrseq instance
> + *
> + * This API is used to do resume operation on pwrseq instance.
> + *
> + * Return 0 on success, or an error value otherwise.
> + */
> +int pwrseq_resume(struct pwrseq *p)
> +{
> +	int ret = 0;
> +
> +	if (p && p->resume)
> +		ret = p->resume(p);
> +	else
> +		return ret;
> +
> +	if (!ret)
> +		p->suspended = false;
> +	else
> +		pr_err("%s failed\n", __func__);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(pwrseq_resume);
> +
> +/**
> + * pwrseq_suspend_list - Carry out power sequence suspend for list
> + *
> + * @head: the list head for pwrseq instance list on this bus
> + *
> + * This API is used to do suspend on all power sequence instances on this bus.
> + * The caller needs to use mutex_lock for concurrent.
> + */

This appears to be totally misguided.

If my understanding of the idea is correct, this is supposed to operate
on subordinate devices below a controller whose driver is expected to
call it.  However, all of those devices should be suspended already at
this point and they should be "off" now.

You basically artificially create a power domain here without any particular
need for that.

> +int pwrseq_suspend_list(struct list_head *head)
> +{
> +	struct pwrseq *pwrseq;
> +	struct pwrseq_list_per_dev *pwrseq_list_node;
> +	int ret = 0;
> +
> +	list_for_each_entry(pwrseq_list_node, head, list) {
> +		ret = pwrseq_suspend(pwrseq_list_node->pwrseq);
> +		if (ret)
> +			break;
> +	}
> +
> +	if (ret) {
> +		list_for_each_entry(pwrseq_list_node, head, list) {
> +			pwrseq = pwrseq_list_node->pwrseq;
> +			if (pwrseq->suspended)
> +				pwrseq_resume(pwrseq);
> +		}
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(pwrseq_suspend_list);
> +
> +/**
> + * pwrseq_resume_list - Carry out power sequence resume for the list
> + *
> + * @head: the list head for pwrseq instance list on this bus
> + *
> + * This API is used to do resume on all power sequence instances on this bus.
> + * The caller needs to use mutex_lock for concurrent.
> + */
> +int pwrseq_resume_list(struct list_head *head)
> +{
> +	struct pwrseq_list_per_dev *pwrseq_list_node;
> +	int ret = 0;
> +
> +	list_for_each_entry(pwrseq_list_node, head, list) {
> +		ret = pwrseq_resume(pwrseq_list_node->pwrseq);
> +		if (ret)
> +			break;
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(pwrseq_resume_list);
> diff --git a/drivers/power/pwrseq/pwrseq_generic.c b/drivers/power/pwrseq/pwrseq_generic.c
> new file mode 100644
> index 0000000..b7bbd6c5
> --- /dev/null
> +++ b/drivers/power/pwrseq/pwrseq_generic.c
> @@ -0,0 +1,210 @@
> +/*
> + * pwrseq_generic.c	Generic power sequence handling
> + *
> + * Copyright (C) 2016 Freescale Semiconductor, Inc.
> + * Author: Peter Chen <peter.chen@nxp.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  of
> + * the License as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> +#include <linux/slab.h>
> +
> +#include <linux/power/pwrseq.h>
> +

No description.

> +struct pwrseq_generic {
> +	struct pwrseq pwrseq;
> +	struct gpio_desc *gpiod_reset;
> +	struct clk *clks[PWRSEQ_MAX_CLKS];
> +	u32 duration_us;
> +	bool suspended;
> +};
> +
> +#define to_generic_pwrseq(p) container_of(p, struct pwrseq_generic, pwrseq)
> +
> +static int pwrseq_generic_suspend(struct pwrseq *pwrseq)
> +{
> +	struct pwrseq_generic *pwrseq_gen = to_generic_pwrseq(pwrseq);
> +	int clk;
> +
> +	for (clk = PWRSEQ_MAX_CLKS - 1; clk >= 0; clk--)
> +		clk_disable_unprepare(pwrseq_gen->clks[clk]);
> +
> +	pwrseq_gen->suspended = true;
> +	return 0;
> +}

So what exactly is the difference between "suspend" and "off" here?

> +
> +static int pwrseq_generic_resume(struct pwrseq *pwrseq)
> +{
> +	struct pwrseq_generic *pwrseq_gen = to_generic_pwrseq(pwrseq);
> +	int clk, ret = 0;
> +
> +	for (clk = 0; clk < PWRSEQ_MAX_CLKS && pwrseq_gen->clks[clk]; clk++) {
> +		ret = clk_prepare_enable(pwrseq_gen->clks[clk]);
> +		if (ret) {
> +			pr_err("Can't enable clock, ret=%d\n", ret);
> +			goto err_disable_clks;
> +		}
> +	}
> +
> +	pwrseq_gen->suspended = false;
> +	return ret;
> +
> +err_disable_clks:
> +	while (--clk >= 0)
> +		clk_disable_unprepare(pwrseq_gen->clks[clk]);
> +
> +	return ret;
> +}
> +
> +static void pwrseq_generic_put(struct pwrseq *pwrseq)
> +{
> +	struct pwrseq_generic *pwrseq_gen = to_generic_pwrseq(pwrseq);
> +	int clk;
> +
> +	if (pwrseq_gen->gpiod_reset)
> +		gpiod_put(pwrseq_gen->gpiod_reset);
> +
> +	for (clk = 0; clk < PWRSEQ_MAX_CLKS; clk++)
> +		clk_put(pwrseq_gen->clks[clk]);
> +
> +	kfree(pwrseq_gen);
> +}
> +
> +static void pwrseq_generic_off(struct pwrseq *pwrseq)
> +{
> +	struct pwrseq_generic *pwrseq_gen = to_generic_pwrseq(pwrseq);
> +	int clk;
> +
> +	if (pwrseq_gen->suspended)
> +		return;
> +
> +	for (clk = PWRSEQ_MAX_CLKS - 1; clk >= 0; clk--)
> +		clk_disable_unprepare(pwrseq_gen->clks[clk]);
> +}
> +
> +static int pwrseq_generic_on(struct pwrseq *pwrseq)
> +{
> +	struct pwrseq_generic *pwrseq_gen = to_generic_pwrseq(pwrseq);
> +	int clk, ret = 0;
> +	struct gpio_desc *gpiod_reset = pwrseq_gen->gpiod_reset;
> +
> +	for (clk = 0; clk < PWRSEQ_MAX_CLKS && pwrseq_gen->clks[clk]; clk++) {
> +		ret = clk_prepare_enable(pwrseq_gen->clks[clk]);
> +		if (ret) {
> +			pr_err("Can't enable clock, ret=%d\n", ret);
> +			goto err_disable_clks;
> +		}
> +	}
> +
> +	if (gpiod_reset) {
> +		u32 duration_us = pwrseq_gen->duration_us;
> +
> +		if (duration_us <= 10)
> +			udelay(10);
> +		else
> +			usleep_range(duration_us, duration_us + 100);
> +		gpiod_set_value(gpiod_reset, 0);
> +	}
> +
> +	return ret;
> +
> +err_disable_clks:
> +	while (--clk >= 0)
> +		clk_disable_unprepare(pwrseq_gen->clks[clk]);
> +
> +	return ret;
> +}
> +
> +static int pwrseq_generic_get(struct device_node *np, struct pwrseq *pwrseq)
> +{
> +	struct pwrseq_generic *pwrseq_gen = to_generic_pwrseq(pwrseq);
> +	enum of_gpio_flags flags;
> +	int reset_gpio, clk, ret = 0;
> +
> +	for (clk = 0; clk < PWRSEQ_MAX_CLKS; clk++) {
> +		pwrseq_gen->clks[clk] = of_clk_get(np, clk);
> +		if (IS_ERR(pwrseq_gen->clks[clk])) {
> +			ret = PTR_ERR(pwrseq_gen->clks[clk]);
> +			if (ret != -ENOENT)
> +				goto err_put_clks;
> +			pwrseq_gen->clks[clk] = NULL;
> +			break;
> +		}
> +	}
> +
> +	reset_gpio = of_get_named_gpio_flags(np, "reset-gpios", 0, &flags);
> +	if (gpio_is_valid(reset_gpio)) {
> +		unsigned long gpio_flags;
> +
> +		if (flags & OF_GPIO_ACTIVE_LOW)
> +			gpio_flags = GPIOF_ACTIVE_LOW | GPIOF_OUT_INIT_LOW;
> +		else
> +			gpio_flags = GPIOF_OUT_INIT_HIGH;
> +
> +		ret = gpio_request_one(reset_gpio, gpio_flags,
> +				"pwrseq-reset-gpios");
> +		if (ret)
> +			goto err_put_clks;
> +
> +		pwrseq_gen->gpiod_reset = gpio_to_desc(reset_gpio);
> +		of_property_read_u32(np, "reset-duration-us",
> +				&pwrseq_gen->duration_us);
> +	} else if (reset_gpio == -ENOENT) {
> +		; /* no such gpio */
> +	} else {
> +		ret = reset_gpio;
> +		pr_err("Failed to get reset gpio on %s, err = %d\n",
> +				np->full_name, reset_gpio);
> +		goto err_put_clks;
> +	}
> +
> +	return 0;
> +
> +err_put_clks:
> +	while (--clk >= 0)
> +		clk_put(pwrseq_gen->clks[clk]);
> +	return ret;
> +}
> +

No comments whatever in this file above this point.

> +/**
> + * pwrseq_generic_alloc_instance - power sequence instance allocation
> + *
> + * This function is used to allocate one generic power sequence instance,
> + * it is called when the system boots up and after one power sequence
> + * instance is got successfully.

Who is responsible for calling it?

> + *
> + * Return zero on success or an error code otherwise.
> + */
> +struct pwrseq *pwrseq_generic_alloc_instance(void)
> +{
> +	struct pwrseq_generic *pwrseq_gen;
> +
> +	pwrseq_gen = kzalloc(sizeof(*pwrseq_gen), GFP_KERNEL);
> +	if (!pwrseq_gen)
> +		return ERR_PTR(-ENOMEM);
> +
> +	pwrseq_gen->pwrseq.get = pwrseq_generic_get;
> +	pwrseq_gen->pwrseq.on = pwrseq_generic_on;
> +	pwrseq_gen->pwrseq.off = pwrseq_generic_off;
> +	pwrseq_gen->pwrseq.put = pwrseq_generic_put;
> +	pwrseq_gen->pwrseq.suspend = pwrseq_generic_suspend;
> +	pwrseq_gen->pwrseq.resume = pwrseq_generic_resume;

What about ->pwrseq_of_match_table?

> +
> +	return &pwrseq_gen->pwrseq;
> +}
> diff --git a/include/linux/power/pwrseq.h b/include/linux/power/pwrseq.h
> new file mode 100644
> index 0000000..c5b278f
> --- /dev/null
> +++ b/include/linux/power/pwrseq.h
> @@ -0,0 +1,84 @@
> +#ifndef __LINUX_PWRSEQ_H
> +#define __LINUX_PWRSEQ_H
> +
> +#include <linux/of.h>
> +
> +#define PWRSEQ_MAX_CLKS		3

Why is the number 3?

Some description of how the stuff below is supposed to be used would be useful
here.

> +
> +/**
> + * struct pwrseq - the power sequence structure
> + * @pwrseq_of_match_table: the OF device id table this pwrseq library supports
> + * @node: the list pointer to be added to pwrseq list
> + * @get: the API is used to get pwrseq instance from the device node
> + * @on: do power on for this pwrseq instance
> + * @off: do power off for this pwrseq instance
> + * @put: release the resources on this pwrseq instance
> + * @suspend: do suspend operation on this pwrseq instance
> + * @resume: do resume operation on this pwrseq instance
> + */
> +struct pwrseq {
> +	const struct of_device_id *pwrseq_of_match_table;
> +	struct list_head node;
> +	int (*get)(struct device_node *np, struct pwrseq *p);
> +	int (*on)(struct pwrseq *p);
> +	void (*off)(struct pwrseq *p);
> +	void (*put)(struct pwrseq *p);
> +	int (*suspend)(struct pwrseq *p);
> +	int (*resume)(struct pwrseq *p);
> +	bool suspended;
> +};
> +
> +/* used for power sequence instance list in one driver */
> +struct pwrseq_list_per_dev {
> +	struct pwrseq *pwrseq;
> +	struct list_head list;
> +};
> +
> +#if IS_ENABLED(CONFIG_POWER_SEQUENCE)
> +struct pwrseq *of_pwrseq_on(struct device_node *np);
> +void of_pwrseq_off(struct pwrseq *pwrseq);
> +int of_pwrseq_on_list(struct device_node *np, struct list_head *head);
> +void of_pwrseq_off_list(struct list_head *head);
> +int pwrseq_suspend(struct pwrseq *p);
> +int pwrseq_resume(struct pwrseq *p);
> +int pwrseq_suspend_list(struct list_head *head);
> +int pwrseq_resume_list(struct list_head *head);
> +#else
> +static inline struct pwrseq *of_pwrseq_on(struct device_node *np)
> +{
> +	return NULL;
> +}
> +static void of_pwrseq_off(struct pwrseq *pwrseq) {}
> +static int of_pwrseq_on_list(struct device_node *np, struct list_head *head)
> +{
> +	return 0;
> +}
> +static void of_pwrseq_off_list(struct list_head *head) {}
> +static int pwrseq_suspend(struct pwrseq *p)
> +{
> +	return 0;
> +}
> +static int pwrseq_resume(struct pwrseq *p)
> +{
> +	return 0;
> +}
> +static int pwrseq_suspend_list(struct list_head *head)
> +{
> +	return 0;
> +}
> +static int pwrseq_resume_list(struct list_head *head)
> +{
> +	return 0;
> +}
> +#endif /* CONFIG_POWER_SEQUENCE */
> +
> +#if IS_ENABLED(CONFIG_PWRSEQ_GENERIC)
> +extern struct pwrseq *pwrseq_generic_alloc_instance(void);
> +#else
> +static inline struct pwrseq *pwrseq_generic_alloc_instance(void)
> +{
> +	return ERR_PTR(-ENOTSUPP);
> +}
> +#endif /* CONFIG_PWRSEQ_GENERIC */
> +
> +#endif  /* __LINUX_PWRSEQ_H */
> 

Overall, no, sorry.

Thanks,
Rafael

Re: [PATCH v16 2/7] power: add power sequence library

[Peter Chen]

On Wed, Jul 05, 2017 at 02:44:56AM +0200, Rafael J. Wysocki wrote:
> On Wednesday, June 21, 2017 02:42:03 PM Peter Chen wrote:
> > We have an well-known problem that the device needs to do some power
> > sequence before it can be recognized by related host, the typical
> > example like hard-wired mmc devices and usb devices.
> > 
> > This power sequence is hard to be described at device tree and handled by
> > related host driver, so we have created a common power sequence
> > library to cover this requirement. The core code has supplied
> > some common helpers for host driver, and individual power sequence
> > libraries handle kinds of power sequence for devices. The pwrseq
> > librares always need to allocate extra instance for compatible
> > string match.
> > 
> > pwrseq_generic is intended for general purpose of power sequence, which
> > handles gpios and clocks currently, and can cover other controls in
> > future. The host driver just needs to call of_pwrseq_on/of_pwrseq_off
> > if only one power sequence is needed, else call of_pwrseq_on_list
> > /of_pwrseq_off_list instead (eg, USB hub driver).
> > 
> > For new power sequence library, it needs to add its compatible string
> > and allocation function at pwrseq_match_table_list, then the pwrseq
> > core will match it with DT's, and choose this library at runtime.
> > 
> > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > Tested-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> > Tested-by Joshua Clayton <stillcompiling@gmail.com>
> > Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> > Tested-by: Matthias Kaehlcke <mka@chromium.org>
> 
> The executive summary here is that I'm not going to apply this patch (and the
> following series depending on it), because in my opinion it is misguided and
> I quite fundamentally disagree with the basic concept.
> 
> The code has problems too, but we first need to agree on the basics.
> 

Thanks for valuable comments, let's agree on the concept and design
first.

> > ---
> >  Documentation/power/power-sequence/design.rst |  54 +++++
> >  MAINTAINERS                                   |   9 +
> >  drivers/power/Kconfig                         |   1 +
> >  drivers/power/Makefile                        |   1 +
> >  drivers/power/pwrseq/Kconfig                  |  20 ++
> >  drivers/power/pwrseq/Makefile                 |   2 +
> >  drivers/power/pwrseq/core.c                   | 293 ++++++++++++++++++++++++++
> >  drivers/power/pwrseq/pwrseq_generic.c         | 210 ++++++++++++++++++
> >  include/linux/power/pwrseq.h                  |  84 ++++++++
> >  9 files changed, 674 insertions(+)
> >  create mode 100644 Documentation/power/power-sequence/design.rst
> >  create mode 100644 drivers/power/pwrseq/Kconfig
> >  create mode 100644 drivers/power/pwrseq/Makefile
> >  create mode 100644 drivers/power/pwrseq/core.c
> >  create mode 100644 drivers/power/pwrseq/pwrseq_generic.c
> >  create mode 100644 include/linux/power/pwrseq.h
> > 
> > diff --git a/Documentation/power/power-sequence/design.rst b/Documentation/power/power-sequence/design.rst
> > new file mode 100644
> > index 0000000..554608e
> > --- /dev/null
> > +++ b/Documentation/power/power-sequence/design.rst
> > @@ -0,0 +1,54 @@
> > +====================================
> > +Power Sequence Library
> > +====================================
> > +
> > +:Date: Feb, 2017
> > +:Author: Peter Chen <peter.chen@nxp.com>
> > +
> > +
> > +Introduction
> > +============
> > +
> > +We have an well-known problem that the device needs to do a power
> > +sequence before it can be recognized by related host, the typical
> > +examples are hard-wired mmc devices and usb devices. The host controller
> > +can't know what kinds of this device is in its bus if the power
> > +sequence has not done, since the related devices driver's probe calling
> > +is determined by runtime according to eunumeration results. Besides,
> > +the devices may have custom power sequence, so the power sequence library
> > +which is independent with the devices is needed.
> 
> In other words, devices can be in different power states and there are power
> states in which they are not accessible to software.  In order to be accessed
> by software, they need to be put into power states in which that is actually
> possible.
> 
> That has been known for over 20 years and by no means it is limited to MMC
> or USB.
> 
> Moreover, power states of devices generally depend on the platform and if
> the same IP block is used in two different platforms (or even in two different
> SoCs for that matter), the power states of it can be defined differently in
> each case.
> 
> It also is quite well known how to define a power state of a device.  There
> usually is a list of power resources (clocks, regulators, etc) that have to
> be "on" for the device to be in the given power state.  In addition to that,
> it may be necessary to carry out some operations after turning those
> power resources "on" in order to trigger the power state change (like
> toggling a GPIO or writing to a specific register etc).
> 
> Now, of course, it is not generally guaranteed that devices will be in
> software-accessible power states initially, so it is necessary to put them
> into those power states before trying to access them.
> 
> Again, this isn't anything new, but it has always been about power states.
> 
> By defining a "power sequence" you generally define two power states for
> the device, say "on" and "off", and a way to switch between them, but what
> about simply treating power states as the primary concept in the first place?
> 
> That would be more general, because there are platforms with more than
> two power states per device.  In those cases, the power state choice may
> depend on additional factors, like say during system suspend it may matter
> whether or not the device is a wakeup one (and it may not be able to
> generate wakeup signals from certain power states).  Using a single "power
> sequence" would not be sufficient then.

I agree on the concept of "power state" for it,
but I have several questions for above:

- Can I write new code for it or I need to depend on something?
I find there is already "power state" concept at documentation.
Documentation/ABI/testing/sysfs-devices-power_state
- If I can write the new code for it, except the problems I want
to fix, are there any other use cases I need to consider?
> 
> > +
> > +Design
> > +============
> > +
> > +The power sequence library includes the core file and customer power
> > +sequence library. The core file exports interfaces are called by
> > +host controller driver for power sequence and customer power sequence
> > +library files to register its power sequence instance to global
> > +power sequence list. The custom power sequence library creates power
> > +sequence instance and implement custom power sequence.
> 
> The above paragraph is really hard to decode.
> 
> There is no explanation of basic concepts at all.  What exactly do you mean by
> "power sequence"?  What is a "power sequence instance"?  What do you mean
> by "custom power sequence"?

"power sequence" means the sequence for power on/off device
"power sequence instance" means when carry out power on, it
need to allocate one "power sequence" structure, and this structure
includes all operations during power on/off sequence.
"custom power sequence" means different devices need different
power on sequence, eg, one may need to open regulator before
toggle gpio, but another may need opposite sequence.

> 
> > +
> > +Since the power sequence describes hardware design, the description is
> > +located at board description file, eg, device tree dts file. And
> > +a specific power sequence belongs to device, so its description
> > +is under the device node, please refer to:
> > +Documentation/devicetree/bindings/power/pwrseq/pwrseq-generic.txt
> 
> This, again, is really difficult to understand, especially the difference between
> "the power sequence" and "a specific power sequence" is quite unclear.
> 

"a specific power sequence" for one custom power sequence

> > +
> > +Custom power sequence library allocates one power sequence instance at
> > +bootup periods using postcore_initcall, this static allocated instance is
> > +used to compare with device-tree (DT) node to see if this library can be
> > +used for the node or not.
> 
> Same here.
> 
> > When the result is matched, the core API will
> > +try to get resourses (->get, implemented at each library) for power
> > +sequence, if all resources are got, it will try to allocate another
> > +instance for next possible request from host driver.
> > +
> > +Then, the host controller driver can carry out power sequence on for this
> > +DT node, the library will do corresponding operations, like open clocks,
> > +toggle gpio, etc. The power sequence off routine will close and free the
> > +resources, and is called when the parent is removed. And the power
> > +sequence suspend and resume routine can be called at host driver's
> > +suspend and resume routine if needed.
> 
> Let me try to say what I understood from this.
> 
> If there is a DT node with a matching "compatible" string, the initialization
> code will try to acquire resources needed for "power sequencing" of the
> corresponding device.  It is expected that host controller drivers will
> then use the provided helper functions to turn the subordinate devices
> "on" during enumeration and "off" on the (host controller) driver removal.
> They also are expected to use the "suspend" and "resume" helpers when
> the host controller is suspended or resumed, respectively.
> 
> If the above is what you intended, then it is not the right model IMO.
> 
> In particular, I don't see why the controller driver suspend and resume
> should operate "power sequences" of subordinate devices directly as the
> subordinate devices are expected to be suspended (in which case they
> also should have been turned "off" already) when the controller is
> suspending and the controller resume need not affect the subordinate
> devices at all.

For example, one device's clock needs to be opened before it can be
seen by the controller, but the device's clock isn't controlled by
controller driver, controller driver only operate its own clocks.
So, after the controller suspends the device, it can turn off
device's clock for power consumption, and when the controller
resumes, it needs to turn on the device's clock before talking
to device.
> 
> Moreover, it assumes that all of the platforms including the IP block driven
> by the controller driver in question will use the "power sequences" model,
> but what if there are more generally defined power domains on them?

No, the IP block doesn't need this "power sequence", the power stuffs
(clock, gpio, regulator) are described at firmware (eg, DT), and power
operations are carried out during the probe, the device and driver match
happens before power operation.

But for devices on the bus (bus is controlled by controller), the match
information (eg, product id/vendor id) is stored at device's firmware, the
controller device needs to talk with its child first to get the match
information before the child device's probe.

> 
> It looks like the power management should really be carried out by an
> additional layer of code to avoid imposing platform dependencies on
> controller drivers.
> 
> Further, what if there are "power sequences" for the host controllers
> themselves?  Who's expected to operate those "power sequences" then?

See above.

-- 

Best Regards,
Peter Chen

Re: [PATCH v16 2/7] power: add power sequence library

[Rafael J. Wysocki]

On Wednesday, July 05, 2017 07:54:00 PM Peter Chen wrote:
> On Wed, Jul 05, 2017 at 02:44:56AM +0200, Rafael J. Wysocki wrote:
> > On Wednesday, June 21, 2017 02:42:03 PM Peter Chen wrote:
> > > We have an well-known problem that the device needs to do some power
> > > sequence before it can be recognized by related host, the typical
> > > example like hard-wired mmc devices and usb devices.
> > > 
> > > This power sequence is hard to be described at device tree and handled by
> > > related host driver, so we have created a common power sequence
> > > library to cover this requirement. The core code has supplied
> > > some common helpers for host driver, and individual power sequence
> > > libraries handle kinds of power sequence for devices. The pwrseq
> > > librares always need to allocate extra instance for compatible
> > > string match.
> > > 
> > > pwrseq_generic is intended for general purpose of power sequence, which
> > > handles gpios and clocks currently, and can cover other controls in
> > > future. The host driver just needs to call of_pwrseq_on/of_pwrseq_off
> > > if only one power sequence is needed, else call of_pwrseq_on_list
> > > /of_pwrseq_off_list instead (eg, USB hub driver).
> > > 
> > > For new power sequence library, it needs to add its compatible string
> > > and allocation function at pwrseq_match_table_list, then the pwrseq
> > > core will match it with DT's, and choose this library at runtime.
> > > 
> > > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > > Tested-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> > > Tested-by Joshua Clayton <stillcompiling@gmail.com>
> > > Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> > > Tested-by: Matthias Kaehlcke <mka@chromium.org>
> > 
> > The executive summary here is that I'm not going to apply this patch (and the
> > following series depending on it), because in my opinion it is misguided and
> > I quite fundamentally disagree with the basic concept.
> > 
> > The code has problems too, but we first need to agree on the basics.
> > 
> 
> Thanks for valuable comments, let's agree on the concept and design
> first.
> 
> > > ---
> > >  Documentation/power/power-sequence/design.rst |  54 +++++
> > >  MAINTAINERS                                   |   9 +
> > >  drivers/power/Kconfig                         |   1 +
> > >  drivers/power/Makefile                        |   1 +
> > >  drivers/power/pwrseq/Kconfig                  |  20 ++
> > >  drivers/power/pwrseq/Makefile                 |   2 +
> > >  drivers/power/pwrseq/core.c                   | 293 ++++++++++++++++++++++++++
> > >  drivers/power/pwrseq/pwrseq_generic.c         | 210 ++++++++++++++++++
> > >  include/linux/power/pwrseq.h                  |  84 ++++++++
> > >  9 files changed, 674 insertions(+)
> > >  create mode 100644 Documentation/power/power-sequence/design.rst
> > >  create mode 100644 drivers/power/pwrseq/Kconfig
> > >  create mode 100644 drivers/power/pwrseq/Makefile
> > >  create mode 100644 drivers/power/pwrseq/core.c
> > >  create mode 100644 drivers/power/pwrseq/pwrseq_generic.c
> > >  create mode 100644 include/linux/power/pwrseq.h
> > > 
> > > diff --git a/Documentation/power/power-sequence/design.rst b/Documentation/power/power-sequence/design.rst
> > > new file mode 100644
> > > index 0000000..554608e
> > > --- /dev/null
> > > +++ b/Documentation/power/power-sequence/design.rst
> > > @@ -0,0 +1,54 @@
> > > +====================================
> > > +Power Sequence Library
> > > +====================================
> > > +
> > > +:Date: Feb, 2017
> > > +:Author: Peter Chen <peter.chen@nxp.com>
> > > +
> > > +
> > > +Introduction
> > > +============
> > > +
> > > +We have an well-known problem that the device needs to do a power
> > > +sequence before it can be recognized by related host, the typical
> > > +examples are hard-wired mmc devices and usb devices. The host controller
> > > +can't know what kinds of this device is in its bus if the power
> > > +sequence has not done, since the related devices driver's probe calling
> > > +is determined by runtime according to eunumeration results. Besides,
> > > +the devices may have custom power sequence, so the power sequence library
> > > +which is independent with the devices is needed.
> > 
> > In other words, devices can be in different power states and there are power
> > states in which they are not accessible to software.  In order to be accessed
> > by software, they need to be put into power states in which that is actually
> > possible.
> > 
> > That has been known for over 20 years and by no means it is limited to MMC
> > or USB.
> > 
> > Moreover, power states of devices generally depend on the platform and if
> > the same IP block is used in two different platforms (or even in two different
> > SoCs for that matter), the power states of it can be defined differently in
> > each case.
> > 
> > It also is quite well known how to define a power state of a device.  There
> > usually is a list of power resources (clocks, regulators, etc) that have to
> > be "on" for the device to be in the given power state.  In addition to that,
> > it may be necessary to carry out some operations after turning those
> > power resources "on" in order to trigger the power state change (like
> > toggling a GPIO or writing to a specific register etc).
> > 
> > Now, of course, it is not generally guaranteed that devices will be in
> > software-accessible power states initially, so it is necessary to put them
> > into those power states before trying to access them.
> > 
> > Again, this isn't anything new, but it has always been about power states.
> > 
> > By defining a "power sequence" you generally define two power states for
> > the device, say "on" and "off", and a way to switch between them, but what
> > about simply treating power states as the primary concept in the first place?
> > 
> > That would be more general, because there are platforms with more than
> > two power states per device.  In those cases, the power state choice may
> > depend on additional factors, like say during system suspend it may matter
> > whether or not the device is a wakeup one (and it may not be able to
> > generate wakeup signals from certain power states).  Using a single "power
> > sequence" would not be sufficient then.
> 
> I agree on the concept of "power state" for it,
> but I have several questions for above:
> 
> - Can I write new code for it or I need to depend on something?

There is nothing this code needs to depend on AFAICS, but there are existing
solutions in this problem space (ACPI power management, genpd), so it needs to
be careful enough about possible overlaps etc.

> I find there is already "power state" concept at documentation.
> Documentation/ABI/testing/sysfs-devices-power_state

This is ACPI-specific and only in sysfs directories representing ACPI device
objects (which aren't physical devices).

Anyway, since ACPI covers the problem space you are working in already,
your code has to be mutually exclusive with it.

> - If I can write the new code for it, except the problems I want
> to fix, are there any other use cases I need to consider?

I would start simple and focus on the particular problem at hand, that is
devices with two power states ("on" and "off") where the "on" state
depends on a number of clocks and/or GPIOs.  Still, I'd also avoid making
design choices that might prevent it from being extended in the future
if need be.

One major problem I can see is how to "attach" the power states framework
to a particular device (once we have discovered that it should be used with
that device).

For bus types that don't do power management of their own you could follow
ACPI (and genpd) and provide a PM domain for this purpose, but bus types
doing their own PM (like USB) will probably need to be treated differently.
In those cases the bus type code will have to know that it should call some
helpers to switch power states of devices.

> > 
> > > +
> > > +Design
> > > +============
> > > +
> > > +The power sequence library includes the core file and customer power
> > > +sequence library. The core file exports interfaces are called by
> > > +host controller driver for power sequence and customer power sequence
> > > +library files to register its power sequence instance to global
> > > +power sequence list. The custom power sequence library creates power
> > > +sequence instance and implement custom power sequence.
> > 
> > The above paragraph is really hard to decode.
> > 
> > There is no explanation of basic concepts at all.  What exactly do you mean by
> > "power sequence"?  What is a "power sequence instance"?  What do you mean
> > by "custom power sequence"?
> 
> "power sequence" means the sequence for power on/off device
> "power sequence instance" means when carry out power on, it
> need to allocate one "power sequence" structure, and this structure
> includes all operations during power on/off sequence.
> "custom power sequence" means different devices need different
> power on sequence, eg, one may need to open regulator before
> toggle gpio, but another may need opposite sequence.

OK, confusing. :-)

> > 
> > > +
> > > +Since the power sequence describes hardware design, the description is
> > > +located at board description file, eg, device tree dts file. And
> > > +a specific power sequence belongs to device, so its description
> > > +is under the device node, please refer to:
> > > +Documentation/devicetree/bindings/power/pwrseq/pwrseq-generic.txt
> > 
> > This, again, is really difficult to understand, especially the difference between
> > "the power sequence" and "a specific power sequence" is quite unclear.
> > 
> 
> "a specific power sequence" for one custom power sequence
> 
> > > +
> > > +Custom power sequence library allocates one power sequence instance at
> > > +bootup periods using postcore_initcall, this static allocated instance is
> > > +used to compare with device-tree (DT) node to see if this library can be
> > > +used for the node or not.
> > 
> > Same here.
> > 
> > > When the result is matched, the core API will
> > > +try to get resourses (->get, implemented at each library) for power
> > > +sequence, if all resources are got, it will try to allocate another
> > > +instance for next possible request from host driver.
> > > +
> > > +Then, the host controller driver can carry out power sequence on for this
> > > +DT node, the library will do corresponding operations, like open clocks,
> > > +toggle gpio, etc. The power sequence off routine will close and free the
> > > +resources, and is called when the parent is removed. And the power
> > > +sequence suspend and resume routine can be called at host driver's
> > > +suspend and resume routine if needed.
> > 
> > Let me try to say what I understood from this.
> > 
> > If there is a DT node with a matching "compatible" string, the initialization
> > code will try to acquire resources needed for "power sequencing" of the
> > corresponding device.  It is expected that host controller drivers will
> > then use the provided helper functions to turn the subordinate devices
> > "on" during enumeration and "off" on the (host controller) driver removal.
> > They also are expected to use the "suspend" and "resume" helpers when
> > the host controller is suspended or resumed, respectively.
> > 
> > If the above is what you intended, then it is not the right model IMO.
> > 
> > In particular, I don't see why the controller driver suspend and resume
> > should operate "power sequences" of subordinate devices directly as the
> > subordinate devices are expected to be suspended (in which case they
> > also should have been turned "off" already) when the controller is
> > suspending and the controller resume need not affect the subordinate
> > devices at all.
> 
> For example, one device's clock needs to be opened before it can be
> seen by the controller, but the device's clock isn't controlled by
> controller driver, controller driver only operate its own clocks.
> So, after the controller suspends the device, it can turn off
> device's clock for power consumption, and when the controller
> resumes, it needs to turn on the device's clock before talking
> to device.

Well, the device isn't suspended by the controller.  It is suspended
by the driver operating it.  And "suspended" basically means, it can be turned
off right away regardless of what the controller driver is doing.

Of course, it has to be turned on whenever the controller driver (or any other
piece of code for that matter) attempts to access it, but that's what the
runtime PM framework is for.

Anyway, (runtime) suspend/resume of subordinate devices is basically
independent of the controller suspend/resume except that some buses
require the controller to be "on" as long as at least one subordinate device
under it is "on" (for example, turning the controller off might cut power from
the subordinate device).

> > 
> > Moreover, it assumes that all of the platforms including the IP block driven
> > by the controller driver in question will use the "power sequences" model,
> > but what if there are more generally defined power domains on them?
> 
> No, the IP block doesn't need this "power sequence", the power stuffs
> (clock, gpio, regulator) are described at firmware (eg, DT), and power
> operations are carried out during the probe, the device and driver match
> happens before power operation.
> 
> But for devices on the bus (bus is controlled by controller), the match
> information (eg, product id/vendor id) is stored at device's firmware, the
> controller device needs to talk with its child first to get the match
> information before the child device's probe.

Actually, that may vary from bus to bus.  In some cases the enumeration is
entirely firmware-based and in some cases devices are discoverable.

I guess you mean the latter situation and you are saying that subordinate
devices may need to be turned "on" (by means of a "power sequence") in order
to be enumerated.

Fair enough, but this is not what I meant.

The concern was that if drivers started to use "power sequences", they would
become incompatible with alternative solutions in the same problem space,
like genpd, but that need not be the case, so never mind.

> > 
> > It looks like the power management should really be carried out by an
> > additional layer of code to avoid imposing platform dependencies on
> > controller drivers.
> > 
> > Further, what if there are "power sequences" for the host controllers
> > themselves?  Who's expected to operate those "power sequences" then?
> 
> See above.

Let me rephrase.  Assume that your host controller is a platform device and
it is not "on" when the SoC comes up.  It has to be turned "on" via a "power
sequence".  Who's expected to do that for it?

Thanks,
Rafael

Re: [PATCH v16 2/7] power: add power sequence library

[Peter Chen]

On Fri, Jul 07, 2017 at 03:13:48AM +0200, Rafael J. Wysocki wrote:
> > 
> > - Can I write new code for it or I need to depend on something?
> 
> There is nothing this code needs to depend on AFAICS, but there are existing
> solutions in this problem space (ACPI power management, genpd), so it needs to
> be careful enough about possible overlaps etc.
> 
> > I find there is already "power state" concept at documentation.
> > Documentation/ABI/testing/sysfs-devices-power_state
> 
> This is ACPI-specific and only in sysfs directories representing ACPI device
> objects (which aren't physical devices).
> 
> Anyway, since ACPI covers the problem space you are working in already,
> your code has to be mutually exclusive with it.
> 
> > - If I can write the new code for it, except the problems I want
> > to fix, are there any other use cases I need to consider?
> 
> I would start simple and focus on the particular problem at hand, that is
> devices with two power states ("on" and "off") where the "on" state
> depends on a number of clocks and/or GPIOs.  Still, I'd also avoid making
> design choices that might prevent it from being extended in the future
> if need be.
> 
> One major problem I can see is how to "attach" the power states framework
> to a particular device (once we have discovered that it should be used with
> that device).
> 
> For bus types that don't do power management of their own you could follow
> ACPI (and genpd) and provide a PM domain for this purpose, but bus types
> doing their own PM (like USB) will probably need to be treated differently.
> In those cases the bus type code will have to know that it should call some
> helpers to switch power states of devices.
> 

After thinking more, using a power state framework is seems too heavy
for this use case. This use case is just do some clock and gpio
operations before device is created, and do some put operations
after device is deleted. We just need some helpers in one structure
(called "power sequence" or "power state") for this purpose.

For the use case, the clock and gpio operation can be done after device
is created, the power domain is more suitable.

> > > 
> > > Moreover, it assumes that all of the platforms including the IP block driven
> > > by the controller driver in question will use the "power sequences" model,
> > > but what if there are more generally defined power domains on them?
> > 
> > No, the IP block doesn't need this "power sequence", the power stuffs
> > (clock, gpio, regulator) are described at firmware (eg, DT), and power
> > operations are carried out during the probe, the device and driver match
> > happens before power operation.
> > 
> > But for devices on the bus (bus is controlled by controller), the match
> > information (eg, product id/vendor id) is stored at device's firmware, the
> > controller device needs to talk with its child first to get the match
> > information before the child device's probe.
> 
> Actually, that may vary from bus to bus.  In some cases the enumeration is
> entirely firmware-based and in some cases devices are discoverable.
> 
> I guess you mean the latter situation and you are saying that subordinate
> devices may need to be turned "on" (by means of a "power sequence") in order
> to be enumerated.
> 
> Fair enough, but this is not what I meant.
> 
> The concern was that if drivers started to use "power sequences", they would
> become incompatible with alternative solutions in the same problem space,
> like genpd, but that need not be the case, so never mind.

"power sequence" is just used before the device is created and after the
device is deleted from the bus. I don't want to overlap with power
domain.

> 
> > > 
> > > It looks like the power management should really be carried out by an
> > > additional layer of code to avoid imposing platform dependencies on
> > > controller drivers.
> > > 
> > > Further, what if there are "power sequences" for the host controllers
> > > themselves?  Who's expected to operate those "power sequences" then?
> > 
> > See above.
> 
> Let me rephrase.  Assume that your host controller is a platform device and
> it is not "on" when the SoC comes up.  It has to be turned "on" via a "power
> sequence".  Who's expected to do that for it?
> 

We can use power domain for it, doesn't need "power sequence" in this
series introduces.

-- 

Best Regards,
Peter Chen

Re: [PATCH v16 2/7] power: add power sequence library

[Rafael J. Wysocki]

On Friday, July 07, 2017 04:01:07 PM Peter Chen wrote:
> On Fri, Jul 07, 2017 at 03:13:48AM +0200, Rafael J. Wysocki wrote:
> > > 
> > > - Can I write new code for it or I need to depend on something?
> > 
> > There is nothing this code needs to depend on AFAICS, but there are existing
> > solutions in this problem space (ACPI power management, genpd), so it needs to
> > be careful enough about possible overlaps etc.
> > 
> > > I find there is already "power state" concept at documentation.
> > > Documentation/ABI/testing/sysfs-devices-power_state
> > 
> > This is ACPI-specific and only in sysfs directories representing ACPI device
> > objects (which aren't physical devices).
> > 
> > Anyway, since ACPI covers the problem space you are working in already,
> > your code has to be mutually exclusive with it.
> > 
> > > - If I can write the new code for it, except the problems I want
> > > to fix, are there any other use cases I need to consider?
> > 
> > I would start simple and focus on the particular problem at hand, that is
> > devices with two power states ("on" and "off") where the "on" state
> > depends on a number of clocks and/or GPIOs.  Still, I'd also avoid making
> > design choices that might prevent it from being extended in the future
> > if need be.
> > 
> > One major problem I can see is how to "attach" the power states framework
> > to a particular device (once we have discovered that it should be used with
> > that device).
> > 
> > For bus types that don't do power management of their own you could follow
> > ACPI (and genpd) and provide a PM domain for this purpose, but bus types
> > doing their own PM (like USB) will probably need to be treated differently.
> > In those cases the bus type code will have to know that it should call some
> > helpers to switch power states of devices.
> > 
> 
> After thinking more, using a power state framework is seems too heavy
> for this use case. This use case is just do some clock and gpio
> operations before device is created, and do some put operations
> after device is deleted. We just need some helpers in one structure
> (called "power sequence" or "power state") for this purpose.
> 
> For the use case, the clock and gpio operation can be done after device
> is created, the power domain is more suitable.

There is a problem with PM domains that they only provide hooks for runtime PM
and system suspend/resume (including hibernation) and not for generic
"power up" and "power down" operations that may need to be carried out at
probe time before the runtime PM framework can be used (and analogously
at remove time).

I would consider starting with the patch below or similar.

Then you can define something like POWER_STATE_SEQUENCE type for your
case and basically use almost what you have already with it, except that
struct pwrsec_generic will now become struct power_state_sequence and
struct power_state_info will be embedded in it instead of struct pwrsec.

The major comceptual difference is that ->power_up and ->power_down are
now available at the level of the device that needs the power sequence and
pm_device_power_up/down() can be used wherever necessary (in the code,
in a bus type, in a controller driver or even in the driver for this particular
device).

Most likely you will need a PM domain in addition to that, but mostly to avoid
code duplication.

And in the future other types of power state definitions may be hooked up
to that, including ACPI etc.


Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/power/common.c |   35 +++++++++++++++++++++++++++++++++++
 include/linux/pm.h          |   33 +++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+)

Index: linux-pm/include/linux/pm.h
===================================================================
--- linux-pm.orig/include/linux/pm.h
+++ linux-pm/include/linux/pm.h
@@ -550,6 +550,30 @@ struct pm_subsys_data {
 #endif
 };
 
+enum power_state_type {
+	POWER_STATE_GENERIC = 0,
+};
+
+/**
+ * struct power_state_info - information related to device power states.
+ *
+ * @type: Power states definition type.
+ * @power_up: Device power up method.
+ * @power_down: Device power down method.
+ *
+ * @power_up is expected to put the device into a power state in which it can
+ * be operated by software (it doesn't have to be the full power state in
+ * principle as long as the device will respond to all software accesses in
+ * this state) and @power_down is expected to put the device into the lowest
+ * power state the device can be put into given all of the applicable
+ * constraints and limitations (it may not mean completely off).
+ */
+struct power_state_info {
+	enum power_state_type type;
+	int (*power_up)(struct device *dev);
+	int (*power_down)(struct device *dev);
+};
+
 struct dev_pm_info {
 	pm_message_t		power_state;
 	unsigned int		can_wakeup:1;
@@ -600,6 +624,7 @@ struct dev_pm_info {
 	unsigned long		active_jiffies;
 	unsigned long		suspended_jiffies;
 	unsigned long		accounting_timestamp;
+	struct power_state_info	*state;
 #endif
 	struct pm_subsys_data	*subsys_data;  /* Owned by the subsystem. */
 	void (*set_latency_tolerance)(struct device *, s32);
@@ -610,6 +635,14 @@ extern void update_pm_runtime_accounting
 extern int dev_pm_get_subsys_data(struct device *dev);
 extern void dev_pm_put_subsys_data(struct device *dev);
 
+#ifdef CONFIG_PM
+extern int pm_device_power_up(struct device *dev);
+extern int pm_device_power_down(struct device *dev);
+#else
+static inline int pm_device_power_up(struct device *dev) { return 0; }
+static inline int pm_device_power_down(struct device *dev) { return 0; }
+#endif /* CONFIG_PM */
+
 /**
  * struct dev_pm_domain - power management domain representation.
  *
Index: linux-pm/drivers/base/power/common.c
===================================================================
--- linux-pm.orig/drivers/base/power/common.c
+++ linux-pm/drivers/base/power/common.c
@@ -152,3 +152,38 @@ void dev_pm_domain_set(struct device *de
 	device_pm_check_callbacks(dev);
 }
 EXPORT_SYMBOL_GPL(dev_pm_domain_set);
+
+/**
+ * pm_device_power_up - Power up a device using the power states framework.
+ * @dev: Device to power up.
+ *
+ * Put the device into a power state in which it will respond to all software
+ * accesses (that may not mean maximum power) using the callback provided
+ * through the device power state framework, if present.
+ */
+int pm_device_power_up(struct device *dev)
+{
+	if (dev->power.state && dev->power.state->power_up)
+		return dev->power.state->power_up(dev);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pm_device_power_up);
+
+/**
+ * pm_device_power_down - Power down a device using the power states framework.
+ * @dev: Device to power down.
+ *
+ * Put the device into the lowest power state it can be put into given the
+ * applicable constraints and limitations (that may not mean maximum power)
+ * using the callback provided through the device power state framework, if
+ * present.
+ */
+int pm_device_power_down(struct device *dev)
+{
+	if (dev->power.state && dev->power.state->power_down)
+		return dev->power.state->power_down(dev);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pm_device_power_down);

Re: [PATCH v16 2/7] power: add power sequence library

[Peter Chen]

On Fri, Jul 07, 2017 at 03:03:06PM +0200, Rafael J. Wysocki wrote:
> On Friday, July 07, 2017 04:01:07 PM Peter Chen wrote:
> > On Fri, Jul 07, 2017 at 03:13:48AM +0200, Rafael J. Wysocki wrote:
> > > > 
> > > > - Can I write new code for it or I need to depend on something?
> > > 
> > > There is nothing this code needs to depend on AFAICS, but there are existing
> > > solutions in this problem space (ACPI power management, genpd), so it needs to
> > > be careful enough about possible overlaps etc.
> > > 
> > > > I find there is already "power state" concept at documentation.
> > > > Documentation/ABI/testing/sysfs-devices-power_state
> > > 
> > > This is ACPI-specific and only in sysfs directories representing ACPI device
> > > objects (which aren't physical devices).
> > > 
> > > Anyway, since ACPI covers the problem space you are working in already,
> > > your code has to be mutually exclusive with it.
> > > 
> > > > - If I can write the new code for it, except the problems I want
> > > > to fix, are there any other use cases I need to consider?
> > > 
> > > I would start simple and focus on the particular problem at hand, that is
> > > devices with two power states ("on" and "off") where the "on" state
> > > depends on a number of clocks and/or GPIOs.  Still, I'd also avoid making
> > > design choices that might prevent it from being extended in the future
> > > if need be.
> > > 
> > > One major problem I can see is how to "attach" the power states framework
> > > to a particular device (once we have discovered that it should be used with
> > > that device).
> > > 
> > > For bus types that don't do power management of their own you could follow
> > > ACPI (and genpd) and provide a PM domain for this purpose, but bus types
> > > doing their own PM (like USB) will probably need to be treated differently.
> > > In those cases the bus type code will have to know that it should call some
> > > helpers to switch power states of devices.
> > > 
> > 
> > After thinking more, using a power state framework is seems too heavy
> > for this use case. This use case is just do some clock and gpio
> > operations before device is created, and do some put operations
> > after device is deleted. We just need some helpers in one structure
> > (called "power sequence" or "power state") for this purpose.
> > 
> > For the use case, the clock and gpio operation can be done after device
> > is created, the power domain is more suitable.
> 
> There is a problem with PM domains that they only provide hooks for runtime PM
> and system suspend/resume (including hibernation) and not for generic
> "power up" and "power down" operations that may need to be carried out at
> probe time before the runtime PM framework can be used (and analogously
> at remove time).
> 
> I would consider starting with the patch below or similar.
> 
> Then you can define something like POWER_STATE_SEQUENCE type for your
> case and basically use almost what you have already with it, except that
> struct pwrsec_generic will now become struct power_state_sequence and
> struct power_state_info will be embedded in it instead of struct pwrsec.
> 
> The major comceptual difference is that ->power_up and ->power_down are
> now available at the level of the device that needs the power sequence and
> pm_device_power_up/down() can be used wherever necessary (in the code,
> in a bus type, in a controller driver or even in the driver for this particular
> device).

Rafeal, thanks for your patch.

The biggest problem for my use case is the device is still not created.
How can I call pm_device_power_up(dev)?

Peter
> 
> Most likely you will need a PM domain in addition to that, but mostly to avoid
> code duplication.
> 
> And in the future other types of power state definitions may be hooked up
> to that, including ACPI etc.
> 
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/base/power/common.c |   35 +++++++++++++++++++++++++++++++++++
>  include/linux/pm.h          |   33 +++++++++++++++++++++++++++++++++
>  2 files changed, 68 insertions(+)
> 
> Index: linux-pm/include/linux/pm.h
> ===================================================================
> --- linux-pm.orig/include/linux/pm.h
> +++ linux-pm/include/linux/pm.h
> @@ -550,6 +550,30 @@ struct pm_subsys_data {
>  #endif
>  };
>  
> +enum power_state_type {
> +	POWER_STATE_GENERIC = 0,
> +};
> +
> +/**
> + * struct power_state_info - information related to device power states.
> + *
> + * @type: Power states definition type.
> + * @power_up: Device power up method.
> + * @power_down: Device power down method.
> + *
> + * @power_up is expected to put the device into a power state in which it can
> + * be operated by software (it doesn't have to be the full power state in
> + * principle as long as the device will respond to all software accesses in
> + * this state) and @power_down is expected to put the device into the lowest
> + * power state the device can be put into given all of the applicable
> + * constraints and limitations (it may not mean completely off).
> + */
> +struct power_state_info {
> +	enum power_state_type type;
> +	int (*power_up)(struct device *dev);
> +	int (*power_down)(struct device *dev);
> +};
> +
>  struct dev_pm_info {
>  	pm_message_t		power_state;
>  	unsigned int		can_wakeup:1;
> @@ -600,6 +624,7 @@ struct dev_pm_info {
>  	unsigned long		active_jiffies;
>  	unsigned long		suspended_jiffies;
>  	unsigned long		accounting_timestamp;
> +	struct power_state_info	*state;
>  #endif
>  	struct pm_subsys_data	*subsys_data;  /* Owned by the subsystem. */
>  	void (*set_latency_tolerance)(struct device *, s32);
> @@ -610,6 +635,14 @@ extern void update_pm_runtime_accounting
>  extern int dev_pm_get_subsys_data(struct device *dev);
>  extern void dev_pm_put_subsys_data(struct device *dev);
>  
> +#ifdef CONFIG_PM
> +extern int pm_device_power_up(struct device *dev);
> +extern int pm_device_power_down(struct device *dev);
> +#else
> +static inline int pm_device_power_up(struct device *dev) { return 0; }
> +static inline int pm_device_power_down(struct device *dev) { return 0; }
> +#endif /* CONFIG_PM */
> +
>  /**
>   * struct dev_pm_domain - power management domain representation.
>   *
> Index: linux-pm/drivers/base/power/common.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/common.c
> +++ linux-pm/drivers/base/power/common.c
> @@ -152,3 +152,38 @@ void dev_pm_domain_set(struct device *de
>  	device_pm_check_callbacks(dev);
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_domain_set);
> +
> +/**
> + * pm_device_power_up - Power up a device using the power states framework.
> + * @dev: Device to power up.
> + *
> + * Put the device into a power state in which it will respond to all software
> + * accesses (that may not mean maximum power) using the callback provided
> + * through the device power state framework, if present.
> + */
> +int pm_device_power_up(struct device *dev)
> +{
> +	if (dev->power.state && dev->power.state->power_up)
> +		return dev->power.state->power_up(dev);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(pm_device_power_up);
> +
> +/**
> + * pm_device_power_down - Power down a device using the power states framework.
> + * @dev: Device to power down.
> + *
> + * Put the device into the lowest power state it can be put into given the
> + * applicable constraints and limitations (that may not mean maximum power)
> + * using the callback provided through the device power state framework, if
> + * present.
> + */
> +int pm_device_power_down(struct device *dev)
> +{
> +	if (dev->power.state && dev->power.state->power_down)
> +		return dev->power.state->power_down(dev);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(pm_device_power_down);
> 

-- 

Best Regards,
Peter Chen

Re: [PATCH v16 2/7] power: add power sequence library

[Rafael J. Wysocki]

On Saturday, July 08, 2017 01:51:15 PM Peter Chen wrote:
> On Fri, Jul 07, 2017 at 03:03:06PM +0200, Rafael J. Wysocki wrote:
> > On Friday, July 07, 2017 04:01:07 PM Peter Chen wrote:
> > > On Fri, Jul 07, 2017 at 03:13:48AM +0200, Rafael J. Wysocki wrote:
> > > > > 
> > > > > - Can I write new code for it or I need to depend on something?
> > > > 
> > > > There is nothing this code needs to depend on AFAICS, but there are existing
> > > > solutions in this problem space (ACPI power management, genpd), so it needs to
> > > > be careful enough about possible overlaps etc.
> > > > 
> > > > > I find there is already "power state" concept at documentation.
> > > > > Documentation/ABI/testing/sysfs-devices-power_state
> > > > 
> > > > This is ACPI-specific and only in sysfs directories representing ACPI device
> > > > objects (which aren't physical devices).
> > > > 
> > > > Anyway, since ACPI covers the problem space you are working in already,
> > > > your code has to be mutually exclusive with it.
> > > > 
> > > > > - If I can write the new code for it, except the problems I want
> > > > > to fix, are there any other use cases I need to consider?
> > > > 
> > > > I would start simple and focus on the particular problem at hand, that is
> > > > devices with two power states ("on" and "off") where the "on" state
> > > > depends on a number of clocks and/or GPIOs.  Still, I'd also avoid making
> > > > design choices that might prevent it from being extended in the future
> > > > if need be.
> > > > 
> > > > One major problem I can see is how to "attach" the power states framework
> > > > to a particular device (once we have discovered that it should be used with
> > > > that device).
> > > > 
> > > > For bus types that don't do power management of their own you could follow
> > > > ACPI (and genpd) and provide a PM domain for this purpose, but bus types
> > > > doing their own PM (like USB) will probably need to be treated differently.
> > > > In those cases the bus type code will have to know that it should call some
> > > > helpers to switch power states of devices.
> > > > 
> > > 
> > > After thinking more, using a power state framework is seems too heavy
> > > for this use case. This use case is just do some clock and gpio
> > > operations before device is created, and do some put operations
> > > after device is deleted. We just need some helpers in one structure
> > > (called "power sequence" or "power state") for this purpose.
> > > 
> > > For the use case, the clock and gpio operation can be done after device
> > > is created, the power domain is more suitable.
> > 
> > There is a problem with PM domains that they only provide hooks for runtime PM
> > and system suspend/resume (including hibernation) and not for generic
> > "power up" and "power down" operations that may need to be carried out at
> > probe time before the runtime PM framework can be used (and analogously
> > at remove time).
> > 
> > I would consider starting with the patch below or similar.
> > 
> > Then you can define something like POWER_STATE_SEQUENCE type for your
> > case and basically use almost what you have already with it, except that
> > struct pwrsec_generic will now become struct power_state_sequence and
> > struct power_state_info will be embedded in it instead of struct pwrsec.
> > 
> > The major comceptual difference is that ->power_up and ->power_down are
> > now available at the level of the device that needs the power sequence and
> > pm_device_power_up/down() can be used wherever necessary (in the code,
> > in a bus type, in a controller driver or even in the driver for this particular
> > device).
> 
> Rafeal, thanks for your patch.
> 
> The biggest problem for my use case is the device is still not created.
> How can I call pm_device_power_up(dev)?

Can you please elaborate on that a bit?

You surely need a device object before probing the device and why would the
device be accessed before that point?

I guess you have a bus with devices that are discoverable in principle, but
they cannot be discovered before being powered up, so you need the information
on which devices to power up in a DT, right?

Thanks,
Rafael

Re: [PATCH v16 2/7] power: add power sequence library

[Peter Chen]

On Sat, Jul 08, 2017 at 02:14:56PM +0200, Rafael J. Wysocki wrote:
> On Saturday, July 08, 2017 01:51:15 PM Peter Chen wrote:
> > On Fri, Jul 07, 2017 at 03:03:06PM +0200, Rafael J. Wysocki wrote:
> > > On Friday, July 07, 2017 04:01:07 PM Peter Chen wrote:
> > > > On Fri, Jul 07, 2017 at 03:13:48AM +0200, Rafael J. Wysocki wrote:
> > > > > > 
> > > > > > - Can I write new code for it or I need to depend on something?
> > > > > 
> > > > > There is nothing this code needs to depend on AFAICS, but there are existing
> > > > > solutions in this problem space (ACPI power management, genpd), so it needs to
> > > > > be careful enough about possible overlaps etc.
> > > > > 
> > > > > > I find there is already "power state" concept at documentation.
> > > > > > Documentation/ABI/testing/sysfs-devices-power_state
> > > > > 
> > > > > This is ACPI-specific and only in sysfs directories representing ACPI device
> > > > > objects (which aren't physical devices).
> > > > > 
> > > > > Anyway, since ACPI covers the problem space you are working in already,
> > > > > your code has to be mutually exclusive with it.
> > > > > 
> > > > > > - If I can write the new code for it, except the problems I want
> > > > > > to fix, are there any other use cases I need to consider?
> > > > > 
> > > > > I would start simple and focus on the particular problem at hand, that is
> > > > > devices with two power states ("on" and "off") where the "on" state
> > > > > depends on a number of clocks and/or GPIOs.  Still, I'd also avoid making
> > > > > design choices that might prevent it from being extended in the future
> > > > > if need be.
> > > > > 
> > > > > One major problem I can see is how to "attach" the power states framework
> > > > > to a particular device (once we have discovered that it should be used with
> > > > > that device).
> > > > > 
> > > > > For bus types that don't do power management of their own you could follow
> > > > > ACPI (and genpd) and provide a PM domain for this purpose, but bus types
> > > > > doing their own PM (like USB) will probably need to be treated differently.
> > > > > In those cases the bus type code will have to know that it should call some
> > > > > helpers to switch power states of devices.
> > > > > 
> > > > 
> > > > After thinking more, using a power state framework is seems too heavy
> > > > for this use case. This use case is just do some clock and gpio
> > > > operations before device is created, and do some put operations
> > > > after device is deleted. We just need some helpers in one structure
> > > > (called "power sequence" or "power state") for this purpose.
> > > > 
> > > > For the use case, the clock and gpio operation can be done after device
> > > > is created, the power domain is more suitable.
> > > 
> > > There is a problem with PM domains that they only provide hooks for runtime PM
> > > and system suspend/resume (including hibernation) and not for generic
> > > "power up" and "power down" operations that may need to be carried out at
> > > probe time before the runtime PM framework can be used (and analogously
> > > at remove time).
> > > 
> > > I would consider starting with the patch below or similar.
> > > 
> > > Then you can define something like POWER_STATE_SEQUENCE type for your
> > > case and basically use almost what you have already with it, except that
> > > struct pwrsec_generic will now become struct power_state_sequence and
> > > struct power_state_info will be embedded in it instead of struct pwrsec.
> > > 
> > > The major comceptual difference is that ->power_up and ->power_down are
> > > now available at the level of the device that needs the power sequence and
> > > pm_device_power_up/down() can be used wherever necessary (in the code,
> > > in a bus type, in a controller driver or even in the driver for this particular
> > > device).
> > 
> > Rafeal, thanks for your patch.
> > 
> > The biggest problem for my use case is the device is still not created.
> > How can I call pm_device_power_up(dev)?
> 
> Can you please elaborate on that a bit?
> 

Sorry, I should describe more.

Let's take USB bus as an example, when the new USB device is at the
host port, the device structure at device model is not created until
it is discoverable by the USB bus. If this new USB device needs to be
powered on before can be discoverable by the bus, the device structure
will be not created without powering on operation. The code usb_alloc_dev
(drivers/usb/core/usb.c) is only called for discoverable device.

Unlike the other bus, eg, platform bus, it creates device structure
according to DT node. The USB bus was designed for hot plug model, the
device structure is for discoverable device. In recent years, we begin
to have some hard-wired USB device, Eg, onboard USB-hub, onboard USB 4G
Modem, etc at the market. It needs some board level power operation before
it can be found by the USB bus. This patch set is designed primarily for
fix this kind of problem. You will see at at pwrseq_generic.c, we use DT
version clock API of_clk_get and DT version gpio API of_get_named_gpio_flags
instead of device structure version, like devm_clk_get and
devm_gpiod_get_optional.

MMC system has similar use case, it creates power sequence platform
device for this issue, but all those power stuffs (clock, gpio, etc)
may not be suitable as a dedicated virtual device at DT, they are belonged
to one physical device, so this patch set is created to see if this issue
can be fixed better.

> You surely need a device object before probing the device and why would the
> device be accessed before that point?

See above.

> 
> I guess you have a bus with devices that are discoverable in principle, but
> they cannot be discovered before being powered up,
       
Yes.

> so you need the information
> on which devices to power up in a DT, right?
> 

The bus will power up all device nodes in this bus according to DT
information, the device structure has not created at this time.

-- 

Best Regards,
Peter Chen

Re: [PATCH v16 2/7] power: add power sequence library

[Rafael J. Wysocki]

On Monday, July 10, 2017 10:28:15 AM Peter Chen wrote:
> On Sat, Jul 08, 2017 at 02:14:56PM +0200, Rafael J. Wysocki wrote:
> > On Saturday, July 08, 2017 01:51:15 PM Peter Chen wrote:
> > > On Fri, Jul 07, 2017 at 03:03:06PM +0200, Rafael J. Wysocki wrote:
> > > > On Friday, July 07, 2017 04:01:07 PM Peter Chen wrote:
> > > > > On Fri, Jul 07, 2017 at 03:13:48AM +0200, Rafael J. Wysocki wrote:
> > > > > > > 
> > > > > > > - Can I write new code for it or I need to depend on something?
> > > > > > 
> > > > > > There is nothing this code needs to depend on AFAICS, but there are existing
> > > > > > solutions in this problem space (ACPI power management, genpd), so it needs to
> > > > > > be careful enough about possible overlaps etc.
> > > > > > 
> > > > > > > I find there is already "power state" concept at documentation.
> > > > > > > Documentation/ABI/testing/sysfs-devices-power_state
> > > > > > 
> > > > > > This is ACPI-specific and only in sysfs directories representing ACPI device
> > > > > > objects (which aren't physical devices).
> > > > > > 
> > > > > > Anyway, since ACPI covers the problem space you are working in already,
> > > > > > your code has to be mutually exclusive with it.
> > > > > > 
> > > > > > > - If I can write the new code for it, except the problems I want
> > > > > > > to fix, are there any other use cases I need to consider?
> > > > > > 
> > > > > > I would start simple and focus on the particular problem at hand, that is
> > > > > > devices with two power states ("on" and "off") where the "on" state
> > > > > > depends on a number of clocks and/or GPIOs.  Still, I'd also avoid making
> > > > > > design choices that might prevent it from being extended in the future
> > > > > > if need be.
> > > > > > 
> > > > > > One major problem I can see is how to "attach" the power states framework
> > > > > > to a particular device (once we have discovered that it should be used with
> > > > > > that device).
> > > > > > 
> > > > > > For bus types that don't do power management of their own you could follow
> > > > > > ACPI (and genpd) and provide a PM domain for this purpose, but bus types
> > > > > > doing their own PM (like USB) will probably need to be treated differently.
> > > > > > In those cases the bus type code will have to know that it should call some
> > > > > > helpers to switch power states of devices.
> > > > > > 
> > > > > 
> > > > > After thinking more, using a power state framework is seems too heavy
> > > > > for this use case. This use case is just do some clock and gpio
> > > > > operations before device is created, and do some put operations
> > > > > after device is deleted. We just need some helpers in one structure
> > > > > (called "power sequence" or "power state") for this purpose.
> > > > > 
> > > > > For the use case, the clock and gpio operation can be done after device
> > > > > is created, the power domain is more suitable.
> > > > 
> > > > There is a problem with PM domains that they only provide hooks for runtime PM
> > > > and system suspend/resume (including hibernation) and not for generic
> > > > "power up" and "power down" operations that may need to be carried out at
> > > > probe time before the runtime PM framework can be used (and analogously
> > > > at remove time).
> > > > 
> > > > I would consider starting with the patch below or similar.
> > > > 
> > > > Then you can define something like POWER_STATE_SEQUENCE type for your
> > > > case and basically use almost what you have already with it, except that
> > > > struct pwrsec_generic will now become struct power_state_sequence and
> > > > struct power_state_info will be embedded in it instead of struct pwrsec.
> > > > 
> > > > The major comceptual difference is that ->power_up and ->power_down are
> > > > now available at the level of the device that needs the power sequence and
> > > > pm_device_power_up/down() can be used wherever necessary (in the code,
> > > > in a bus type, in a controller driver or even in the driver for this particular
> > > > device).
> > > 
> > > Rafeal, thanks for your patch.
> > > 
> > > The biggest problem for my use case is the device is still not created.
> > > How can I call pm_device_power_up(dev)?
> > 
> > Can you please elaborate on that a bit?
> > 
> 
> Sorry, I should describe more.
> 
> Let's take USB bus as an example, when the new USB device is at the
> host port, the device structure at device model is not created until
> it is discoverable by the USB bus. If this new USB device needs to be
> powered on before can be discoverable by the bus, the device structure
> will be not created without powering on operation. The code usb_alloc_dev
> (drivers/usb/core/usb.c) is only called for discoverable device.
> 
> Unlike the other bus, eg, platform bus, it creates device structure
> according to DT node. The USB bus was designed for hot plug model, the
> device structure is for discoverable device. In recent years, we begin
> to have some hard-wired USB device, Eg, onboard USB-hub, onboard USB 4G
> Modem, etc at the market. It needs some board level power operation before
> it can be found by the USB bus. This patch set is designed primarily for
> fix this kind of problem. You will see at at pwrseq_generic.c, we use DT
> version clock API of_clk_get and DT version gpio API of_get_named_gpio_flags
> instead of device structure version, like devm_clk_get and
> devm_gpiod_get_optional.
> 
> MMC system has similar use case, it creates power sequence platform
> device for this issue, but all those power stuffs (clock, gpio, etc)
> may not be suitable as a dedicated virtual device at DT, they are belonged
> to one physical device, so this patch set is created to see if this issue
> can be fixed better.

OK, thanks for the explanation.

The above needs to be part of your problem statement.

> > You surely need a device object before probing the device and why would the
> > device be accessed before that point?
> 
> See above.
> 
> > 
> > I guess you have a bus with devices that are discoverable in principle, but
> > they cannot be discovered before being powered up,
>        
> Yes.

I was missing that point before, sorry about that.

I can only say that this wasn't particularly clear from you patch changelogs etc.

> > so you need the information
> > on which devices to power up in a DT, right?
> > 
> 
> The bus will power up all device nodes in this bus according to DT
> information, the device structure has not created at this time.

OK

I still think that the information on power resources depended on by devices
should be used for power management as well as for the initial power-up.

The most straightforward way to arrange for that would be to make it possible
to find the DT node matching the device after the device has been discovered
and struct device created for it, say by USB.  That would require adding some
more information on the device to the DT node, probably.

Then, the DT device nodes would be used for the initial power-up and next, after
discovering a device, you'd do a lookup in the DT, find the node matching it
and read the power resuources information from there to populate the device's
power state structure.  From that point on you can simply use the interface I
suggested.

Thanks,
Rafael

Re: [PATCH v16 2/7] power: add power sequence library

[Peter Chen]

On Mon, Jul 17, 2017 at 03:39:07PM +0200, Rafael J. Wysocki wrote:
> > Sorry, I should describe more.
> > 
> > Let's take USB bus as an example, when the new USB device is at the
> > host port, the device structure at device model is not created until
> > it is discoverable by the USB bus. If this new USB device needs to be
> > powered on before can be discoverable by the bus, the device structure
> > will be not created without powering on operation. The code usb_alloc_dev
> > (drivers/usb/core/usb.c) is only called for discoverable device.
> > 
> > Unlike the other bus, eg, platform bus, it creates device structure
> > according to DT node. The USB bus was designed for hot plug model, the
> > device structure is for discoverable device. In recent years, we begin
> > to have some hard-wired USB device, Eg, onboard USB-hub, onboard USB 4G
> > Modem, etc at the market. It needs some board level power operation before
> > it can be found by the USB bus. This patch set is designed primarily for
> > fix this kind of problem. You will see at at pwrseq_generic.c, we use DT
> > version clock API of_clk_get and DT version gpio API of_get_named_gpio_flags
> > instead of device structure version, like devm_clk_get and
> > devm_gpiod_get_optional.
> > 
> > MMC system has similar use case, it creates power sequence platform
> > device for this issue, but all those power stuffs (clock, gpio, etc)
> > may not be suitable as a dedicated virtual device at DT, they are belonged
> > to one physical device, so this patch set is created to see if this issue
> > can be fixed better.
> 
> OK, thanks for the explanation.
> 
> The above needs to be part of your problem statement.

Ok, I will add it to cover letter.

> > 
> > The bus will power up all device nodes in this bus according to DT
> > information, the device structure has not created at this time.
> 
> OK
> 
> I still think that the information on power resources depended on by devices
> should be used for power management as well as for the initial power-up.
> 
> The most straightforward way to arrange for that would be to make it possible
> to find the DT node matching the device after the device has been discovered
> and struct device created for it, say by USB.  That would require adding some
> more information on the device to the DT node, probably.

After the device is created, the device node structure is under struct
device, say dev->of_node. The most difficulty for this issue is the
device creation is dynamic and is after the physical device is
discovered by the bus, the initial power-up is needed before the device
can be discovered by the bus.

> 
> Then, the DT device nodes would be used for the initial power-up and next, after
> discovering a device, you'd do a lookup in the DT, find the node matching it
> and read the power resuources information from there to populate the device's
> power state structure.  From that point on you can simply use the interface I
> suggested.
> 

Just like I said above, without initial power-up, the device can't be
discovered by the bus.

-- 

Best Regards,
Peter Chen

Re: [PATCH v16 2/7] power: add power sequence library

[Rafael J. Wysocki]

On Tue, Jul 18, 2017 at 6:29 AM, Peter Chen <hzpeterchen@gmail.com> wrote:
> On Mon, Jul 17, 2017 at 03:39:07PM +0200, Rafael J. Wysocki wrote:
>> > Sorry, I should describe more.
>> >
>> > Let's take USB bus as an example, when the new USB device is at the
>> > host port, the device structure at device model is not created until
>> > it is discoverable by the USB bus. If this new USB device needs to be
>> > powered on before can be discoverable by the bus, the device structure
>> > will be not created without powering on operation. The code usb_alloc_dev
>> > (drivers/usb/core/usb.c) is only called for discoverable device.
>> >
>> > Unlike the other bus, eg, platform bus, it creates device structure
>> > according to DT node. The USB bus was designed for hot plug model, the
>> > device structure is for discoverable device. In recent years, we begin
>> > to have some hard-wired USB device, Eg, onboard USB-hub, onboard USB 4G
>> > Modem, etc at the market. It needs some board level power operation before
>> > it can be found by the USB bus. This patch set is designed primarily for
>> > fix this kind of problem. You will see at at pwrseq_generic.c, we use DT
>> > version clock API of_clk_get and DT version gpio API of_get_named_gpio_flags
>> > instead of device structure version, like devm_clk_get and
>> > devm_gpiod_get_optional.
>> >
>> > MMC system has similar use case, it creates power sequence platform
>> > device for this issue, but all those power stuffs (clock, gpio, etc)
>> > may not be suitable as a dedicated virtual device at DT, they are belonged
>> > to one physical device, so this patch set is created to see if this issue
>> > can be fixed better.
>>
>> OK, thanks for the explanation.
>>
>> The above needs to be part of your problem statement.
>
> Ok, I will add it to cover letter.
>
>> >
>> > The bus will power up all device nodes in this bus according to DT
>> > information, the device structure has not created at this time.
>>
>> OK
>>
>> I still think that the information on power resources depended on by devices
>> should be used for power management as well as for the initial power-up.
>>
>> The most straightforward way to arrange for that would be to make it possible
>> to find the DT node matching the device after the device has been discovered
>> and struct device created for it, say by USB.  That would require adding some
>> more information on the device to the DT node, probably.
>
> After the device is created, the device node structure is under struct
> device, say dev->of_node. The most difficulty for this issue is the
> device creation is dynamic and is after the physical device is
> discovered by the bus, the initial power-up is needed before the device
> can be discovered by the bus.

So you power up all devices on the bus using the information from
of_nodes upfront.

Then you scan the bus and discover devices.  For each of them, once it
has been discovered, you look up a matching of_node and initialize
power_state from there.

>>
>> Then, the DT device nodes would be used for the initial power-up and next, after
>> discovering a device, you'd do a lookup in the DT, find the node matching it
>> and read the power resuources information from there to populate the device's
>> power state structure.  From that point on you can simply use the interface I
>> suggested.
>>
>
> Just like I said above, without initial power-up, the device can't be
> discovered by the bus.

Right.

So you do the power up upfront, as per the above.

Thanks,
Rafael

Re: [PATCH v16 2/7] power: add power sequence library

[Peter Chen]

On Tue, Jul 18, 2017 at 07:06:05PM +0200, Rafael J. Wysocki wrote:
> On Tue, Jul 18, 2017 at 6:29 AM, Peter Chen <hzpeterchen@gmail.com> wrote:
> > On Mon, Jul 17, 2017 at 03:39:07PM +0200, Rafael J. Wysocki wrote:
> >> > Sorry, I should describe more.
> >> >
> >> > Let's take USB bus as an example, when the new USB device is at the
> >> > host port, the device structure at device model is not created until
> >> > it is discoverable by the USB bus. If this new USB device needs to be
> >> > powered on before can be discoverable by the bus, the device structure
> >> > will be not created without powering on operation. The code usb_alloc_dev
> >> > (drivers/usb/core/usb.c) is only called for discoverable device.
> >> >
> >> > Unlike the other bus, eg, platform bus, it creates device structure
> >> > according to DT node. The USB bus was designed for hot plug model, the
> >> > device structure is for discoverable device. In recent years, we begin
> >> > to have some hard-wired USB device, Eg, onboard USB-hub, onboard USB 4G
> >> > Modem, etc at the market. It needs some board level power operation before
> >> > it can be found by the USB bus. This patch set is designed primarily for
> >> > fix this kind of problem. You will see at at pwrseq_generic.c, we use DT
> >> > version clock API of_clk_get and DT version gpio API of_get_named_gpio_flags
> >> > instead of device structure version, like devm_clk_get and
> >> > devm_gpiod_get_optional.
> >> >
> >> > MMC system has similar use case, it creates power sequence platform
> >> > device for this issue, but all those power stuffs (clock, gpio, etc)
> >> > may not be suitable as a dedicated virtual device at DT, they are belonged
> >> > to one physical device, so this patch set is created to see if this issue
> >> > can be fixed better.
> >>
> >> OK, thanks for the explanation.
> >>
> >> The above needs to be part of your problem statement.
> >
> > Ok, I will add it to cover letter.
> >
> >> >
> >> > The bus will power up all device nodes in this bus according to DT
> >> > information, the device structure has not created at this time.
> >>
> >> OK
> >>
> >> I still think that the information on power resources depended on by devices
> >> should be used for power management as well as for the initial power-up.
> >>
> >> The most straightforward way to arrange for that would be to make it possible
> >> to find the DT node matching the device after the device has been discovered
> >> and struct device created for it, say by USB.  That would require adding some
> >> more information on the device to the DT node, probably.
> >
> > After the device is created, the device node structure is under struct
> > device, say dev->of_node. The most difficulty for this issue is the
> > device creation is dynamic and is after the physical device is
> > discovered by the bus, the initial power-up is needed before the device
> > can be discovered by the bus.
> 
> So you power up all devices on the bus using the information from
> of_nodes upfront.
> 

I think your mean call pm_device_power_up(struct device *dev) for bus
level device (eg, USB HUB for USB), yeah, I can do that. But we still
have below problem:

Where we can put the kinds of power sequence implementation
(eg, pwrseq_generic.c) and the match mechanism between device
node and kinds of power sequence(eg, of_pwrseq_on at core.c)? These
stuffs can be shared among subsystems, and is better to use
one framework for it. MMC subsystem adds such things at its core
folder (drivers/mmc/core/pwrseq*) currently.

-- 

Best Regards,
Peter Chen

Re: [PATCH v16 2/7] power: add power sequence library

[Rafael J. Wysocki]

On Wednesday, July 19, 2017 10:56:00 AM Peter Chen wrote:
> On Tue, Jul 18, 2017 at 07:06:05PM +0200, Rafael J. Wysocki wrote:
> > On Tue, Jul 18, 2017 at 6:29 AM, Peter Chen <hzpeterchen@gmail.com> wrote:
> > > On Mon, Jul 17, 2017 at 03:39:07PM +0200, Rafael J. Wysocki wrote:
> > >> > Sorry, I should describe more.
> > >> >
> > >> > Let's take USB bus as an example, when the new USB device is at the
> > >> > host port, the device structure at device model is not created until
> > >> > it is discoverable by the USB bus. If this new USB device needs to be
> > >> > powered on before can be discoverable by the bus, the device structure
> > >> > will be not created without powering on operation. The code usb_alloc_dev
> > >> > (drivers/usb/core/usb.c) is only called for discoverable device.
> > >> >
> > >> > Unlike the other bus, eg, platform bus, it creates device structure
> > >> > according to DT node. The USB bus was designed for hot plug model, the
> > >> > device structure is for discoverable device. In recent years, we begin
> > >> > to have some hard-wired USB device, Eg, onboard USB-hub, onboard USB 4G
> > >> > Modem, etc at the market. It needs some board level power operation before
> > >> > it can be found by the USB bus. This patch set is designed primarily for
> > >> > fix this kind of problem. You will see at at pwrseq_generic.c, we use DT
> > >> > version clock API of_clk_get and DT version gpio API of_get_named_gpio_flags
> > >> > instead of device structure version, like devm_clk_get and
> > >> > devm_gpiod_get_optional.
> > >> >
> > >> > MMC system has similar use case, it creates power sequence platform
> > >> > device for this issue, but all those power stuffs (clock, gpio, etc)
> > >> > may not be suitable as a dedicated virtual device at DT, they are belonged
> > >> > to one physical device, so this patch set is created to see if this issue
> > >> > can be fixed better.
> > >>
> > >> OK, thanks for the explanation.
> > >>
> > >> The above needs to be part of your problem statement.
> > >
> > > Ok, I will add it to cover letter.
> > >
> > >> >
> > >> > The bus will power up all device nodes in this bus according to DT
> > >> > information, the device structure has not created at this time.
> > >>
> > >> OK
> > >>
> > >> I still think that the information on power resources depended on by devices
> > >> should be used for power management as well as for the initial power-up.
> > >>
> > >> The most straightforward way to arrange for that would be to make it possible
> > >> to find the DT node matching the device after the device has been discovered
> > >> and struct device created for it, say by USB.  That would require adding some
> > >> more information on the device to the DT node, probably.
> > >
> > > After the device is created, the device node structure is under struct
> > > device, say dev->of_node. The most difficulty for this issue is the
> > > device creation is dynamic and is after the physical device is
> > > discovered by the bus, the initial power-up is needed before the device
> > > can be discovered by the bus.
> > 
> > So you power up all devices on the bus using the information from
> > of_nodes upfront.
> > 
> 
> I think your mean call pm_device_power_up(struct device *dev) for bus
> level device (eg, USB HUB for USB), yeah, I can do that. But we still
> have below problem:
> 
> Where we can put the kinds of power sequence implementation
> (eg, pwrseq_generic.c) and the match mechanism between device
> node and kinds of power sequence(eg, of_pwrseq_on at core.c)? These
> stuffs can be shared among subsystems, and is better to use
> one framework for it. MMC subsystem adds such things at its core
> folder (drivers/mmc/core/pwrseq*) currently.

The power sequence handling code can be generic IMO, because it doesn't
depend on what bus the devices are on.  It just needs to provide some
services to its users, but you need a clean interface between it and its
users, of course.

There basically are two categories of devices, let's refer to them as USB-type
and MMC-type.  The difference between them, as far as power sequences go,
boils down to initialization and discovery.

For the USB-type ones you need an upfront power-up pass over of_nodes
under the bus controller.  That needs to be initiated by the controller driver,
but may be carried out by the common code.

After that, you need to match of_nodes to devices while discovering them.
That also needs to be initiated by the bus controller, but there may be
a helper function in the common code for that.  It may take a struct device
for the just discovered device and the bus controller's of_node as arguments,
look for an of_node under the bus controller matching the device and then
attach the power sequence information to the struct device as appropriate.

After the power sequence information has been attached to the struct
device, it can just be used for controlling its power state in a generic
fashion.

In the MMC-type devices case the discovery if through of_node lookup IIUC,
so then you may power up things as you discover them and attach the
power sequence information to their struct device objects right away.

Again, once you have done that, the power sequence information is
there and it can be used for controlling the power states of devices
generically.

Thanks,
Rafael

Re: [PATCH v16 2/7] power: add power sequence library

[Peter Chen]

On Wed, Jul 19, 2017 at 01:34:11PM +0200, Rafael J. Wysocki wrote:
> On Wednesday, July 19, 2017 10:56:00 AM Peter Chen wrote:
> > On Tue, Jul 18, 2017 at 07:06:05PM +0200, Rafael J. Wysocki wrote:
> > > On Tue, Jul 18, 2017 at 6:29 AM, Peter Chen <hzpeterchen@gmail.com> wrote:
> > > > On Mon, Jul 17, 2017 at 03:39:07PM +0200, Rafael J. Wysocki wrote:
> > > >> > Sorry, I should describe more.
> > > >> >
> > > >> > Let's take USB bus as an example, when the new USB device is at the
> > > >> > host port, the device structure at device model is not created until
> > > >> > it is discoverable by the USB bus. If this new USB device needs to be
> > > >> > powered on before can be discoverable by the bus, the device structure
> > > >> > will be not created without powering on operation. The code usb_alloc_dev
> > > >> > (drivers/usb/core/usb.c) is only called for discoverable device.
> > > >> >
> > > >> > Unlike the other bus, eg, platform bus, it creates device structure
> > > >> > according to DT node. The USB bus was designed for hot plug model, the
> > > >> > device structure is for discoverable device. In recent years, we begin
> > > >> > to have some hard-wired USB device, Eg, onboard USB-hub, onboard USB 4G
> > > >> > Modem, etc at the market. It needs some board level power operation before
> > > >> > it can be found by the USB bus. This patch set is designed primarily for
> > > >> > fix this kind of problem. You will see at at pwrseq_generic.c, we use DT
> > > >> > version clock API of_clk_get and DT version gpio API of_get_named_gpio_flags
> > > >> > instead of device structure version, like devm_clk_get and
> > > >> > devm_gpiod_get_optional.
> > > >> >
> > > >> > MMC system has similar use case, it creates power sequence platform
> > > >> > device for this issue, but all those power stuffs (clock, gpio, etc)
> > > >> > may not be suitable as a dedicated virtual device at DT, they are belonged
> > > >> > to one physical device, so this patch set is created to see if this issue
> > > >> > can be fixed better.
> > > >>
> > > >> OK, thanks for the explanation.
> > > >>
> > > >> The above needs to be part of your problem statement.
> > > >
> > > > Ok, I will add it to cover letter.
> > > >
> > > >> >
> > > >> > The bus will power up all device nodes in this bus according to DT
> > > >> > information, the device structure has not created at this time.
> > > >>
> > > >> OK
> > > >>
> > > >> I still think that the information on power resources depended on by devices
> > > >> should be used for power management as well as for the initial power-up.
> > > >>
> > > >> The most straightforward way to arrange for that would be to make it possible
> > > >> to find the DT node matching the device after the device has been discovered
> > > >> and struct device created for it, say by USB.  That would require adding some
> > > >> more information on the device to the DT node, probably.
> > > >
> > > > After the device is created, the device node structure is under struct
> > > > device, say dev->of_node. The most difficulty for this issue is the
> > > > device creation is dynamic and is after the physical device is
> > > > discovered by the bus, the initial power-up is needed before the device
> > > > can be discovered by the bus.
> > > 
> > > So you power up all devices on the bus using the information from
> > > of_nodes upfront.
> > > 
> > 
> > I think your mean call pm_device_power_up(struct device *dev) for bus
> > level device (eg, USB HUB for USB), yeah, I can do that. But we still
> > have below problem:
> > 
> > Where we can put the kinds of power sequence implementation
> > (eg, pwrseq_generic.c) and the match mechanism between device
> > node and kinds of power sequence(eg, of_pwrseq_on at core.c)? These
> > stuffs can be shared among subsystems, and is better to use
> > one framework for it. MMC subsystem adds such things at its core
> > folder (drivers/mmc/core/pwrseq*) currently.
> 
> The power sequence handling code can be generic IMO, because it doesn't
> depend on what bus the devices are on.  It just needs to provide some
> services to its users, but you need a clean interface between it and its
> users, of course.
> 
> There basically are two categories of devices, let's refer to them as USB-type
> and MMC-type.  The difference between them, as far as power sequences go,
> boils down to initialization and discovery.
> 
> For the USB-type ones you need an upfront power-up pass over of_nodes
> under the bus controller.  That needs to be initiated by the controller driver,
> but may be carried out by the common code.

The upfront power-up can't use pm_device_power_up API, since the
power sequence at device node is for the devices under the controller
device, it has to use the power sequence APIs directly, eg,
of_pwrseq_on_list or of_pwrseq_on.

> 
> After that, you need to match of_nodes to devices while discovering them.
> That also needs to be initiated by the bus controller, but there may be
> a helper function in the common code for that.  It may take a struct device
> for the just discovered device and the bus controller's of_node as arguments,
> look for an of_node under the bus controller matching the device and then
> attach the power sequence information to the struct device as appropriate.
> 

Yes, we can have a helper at pwrseq core, eg dev_add_pwrseq(dev->of_node)
to do that, and call it at device_pm_init. At dev_add_pwrseq, we search
the global pwrseq list, and do match according to the address of
of_node, if matches, return the pointer of struct pwrseq.

> After the power sequence information has been attached to the struct
> device, it can just be used for controlling its power state in a generic
> fashion.

At least for USB use case, there is a problem here for this patch set, it
doesn't need to control power state (on/off) during its struct device
life-cycle, after the struct device is de-inintialized (call put_device(dev)),
its (initial) power state is still on, its power state will be off when its
parent (USB HUB or controller device) is going to be deleted.

So, it is hard for me to have the use case to call pm_device_power_up
and pm_device_power_off, the only thing I can do at my patch set
is to set dev.state->type = POWER_STATE_ON when tries to add
struct pwrseq to struct device.

> 
> In the MMC-type devices case the discovery if through of_node lookup IIUC,
> so then you may power up things as you discover them and attach the
> power sequence information to their struct device objects right away.
> 
> Again, once you have done that, the power sequence information is
> there and it can be used for controlling the power states of devices
> generically.

Like I mentioned above, I can implement these helpers for that, but I
only can update power state information for device, but no place to
call and define its dev.state->power_on and dev.state->power_off. 

-- 

Best Regards,
Peter Chen