Hi Dongsheng,

On 2018/9/7 18:16, Dongsheng Wang wrote:

On 2018/9/7 16:49, Ran Wang wrote:

quoted

Hi Dongsheng

quoted

On 2018/9/5 11:05, Dongsheng Wang wrote:

Please change your comments style.

On 2018/8/31 11:57, Ran Wang wrote:

quoted

This driver is to provide a independent framework for PM service
provider and consumer to configure system level wake up feature. For
example, RCPM driver could register a callback function on this
platform first, and Flex timer driver who want to enable timer wake
up feature, will call generic API provided by this platform driver,
and then it will trigger RCPM driver to do it. The benefit is to
isolate the user and service, such as flex timer driver will not
have to know the implement details of wakeup function it require.
Besides, it is also easy for service side to upgrade its logic when
design is changed and remain user side unchanged.

Signed-off-by: Ran Wang <redacted>
---
 drivers/soc/fsl/Kconfig   |   14 +++++
 drivers/soc/fsl/Makefile  |    1 +
 drivers/soc/fsl/plat_pm.c |  144

+++++++++++++++++++++++++++++++++++++++++++++

quoted

 include/soc/fsl/plat_pm.h |   22 +++++++
 4 files changed, 181 insertions(+), 0 deletions(-)  create mode
100644 drivers/soc/fsl/plat_pm.c  create mode 100644
include/soc/fsl/plat_pm.h

diff --git a/drivers/soc/fsl/Kconfig b/drivers/soc/fsl/Kconfig index

7a9fb9b..6517412 100644

--- a/drivers/soc/fsl/Kconfig
+++ b/drivers/soc/fsl/Kconfig

@@ -16,3 +16,17 @@ config FSL_GUTS
 	  Initially only reading SVR and registering soc device are supported.
 	  Other guts accesses, such as reading RCW, should eventually be

moved

quoted

 	  into this driver as well.
+
+config FSL_PLAT_PM
+	bool "Freescale platform PM framework"
+	help
+	  This driver is to provide a independent framework for PM service
+	  provider and consumer to configure system level wake up feature.

For

quoted

+	  example, RCPM driver could register a callback function on this
+	  platform first, and Flex timer driver who want to enable timer wake
+	  up feature, will call generic API provided by this platform driver,
+	  and then it will trigger RCPM driver to do it. The benefit is to
+	  isolate the user and service, such as  flex timer driver will not
+	  have to know the implement details of wakeup function it require.
+	  Besides, it is also easy for service side to upgrade its logic when
+	  design changed and remain user side unchanged.

diff --git a/drivers/soc/fsl/Makefile b/drivers/soc/fsl/Makefile

index
44b3beb..8f9db23 100644

--- a/drivers/soc/fsl/Makefile
+++ b/drivers/soc/fsl/Makefile

@@ -6,3 +6,4 @@ obj-$(CONFIG_FSL_DPAA)                 += qbman/
 obj-$(CONFIG_QUICC_ENGINE)		+= qe/
 obj-$(CONFIG_CPM)			+= qe/
 obj-$(CONFIG_FSL_GUTS)			+= guts.o
+obj-$(CONFIG_FSL_PLAT_PM)	+= plat_pm.o

diff --git a/drivers/soc/fsl/plat_pm.c b/drivers/soc/fsl/plat_pm.c
new file mode 100644 index 0000000..19ea14e
--- /dev/null
+++ b/drivers/soc/fsl/plat_pm.c

@@ -0,0 +1,144 @@
+// SPDX-License-Identifier: GPL-2.0 // // plat_pm.c - Freescale
+platform PM framework // // Copyright 2018 NXP // // Author: Ran
+Wang <ran.wang_1@nxp.com>,
+
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <linux/list.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <soc/fsl/plat_pm.h>
+
+
+struct plat_pm_t {
+	struct list_head node;
+	fsl_plat_pm_handle handle;
+	void *handle_priv;
+	spinlock_t	lock;
+};
+
+static struct plat_pm_t plat_pm;
+
+// register_fsl_platform_wakeup_source - Register callback function
+to plat_pm // @handle: Pointer to handle PM feature requirement //
+ at handle_priv: Handler specific data struct // // Return 0 on
+success other negative errno int
+register_fsl_platform_wakeup_source(fsl_plat_pm_handle handle,
+		void *handle_priv)
+{
+	struct plat_pm_t *p;
+	unsigned long	flags;
+
+	if (!handle) {
+		pr_err("FSL plat_pm: Handler invalid, reject\n");
+		return -EINVAL;
+	}
+
+	p = kmalloc(sizeof(*p), GFP_KERNEL);
+	if (!p)
+		return -ENOMEM;
+
+	p->handle = handle;
+	p->handle_priv = handle_priv;
+
+	spin_lock_irqsave(&plat_pm.lock, flags);
+	list_add_tail(&p->node, &plat_pm.node);
+	spin_unlock_irqrestore(&plat_pm.lock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(register_fsl_platform_wakeup_source);
+
+// Deregister_fsl_platform_wakeup_source - deregister callback
+function // @handle_priv: Handler specific data struct // // Return
+0 on success other negative errno int
+deregister_fsl_platform_wakeup_source(void *handle_priv) {
+	struct plat_pm_t *p, *tmp;
+	unsigned long	flags;
+
+	spin_lock_irqsave(&plat_pm.lock, flags);
+	list_for_each_entry_safe(p, tmp, &plat_pm.node, node) {
+		if (p->handle_priv == handle_priv) {
+			list_del(&p->node);
+			kfree(p);
+		}
+	}
+	spin_unlock_irqrestore(&plat_pm.lock, flags);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(deregister_fsl_platform_wakeup_source);
+
+// fsl_platform_wakeup_config - Configure wakeup source by calling
+handlers // @dev: pointer to user's device struct // @flag: to tell
+enable or disable wakeup source // // Return 0 on success other
+negative errno int fsl_platform_wakeup_config(struct device *dev,
+bool flag) {
+	struct plat_pm_t *p;
+	int ret;
+	bool success_handled;
+	unsigned long	flags;
+
+	success_handled = false;
+
+	// Will consider success if at least one callback return 0.
+	// Also, rest handles still get oppertunity to be executed
+	spin_lock_irqsave(&plat_pm.lock, flags);
+	list_for_each_entry(p, &plat_pm.node, node) {
+		if (p->handle) {
+			ret = p->handle(dev, flag, p->handle_priv);
+			if (!ret)
+				success_handled = true;

Miss a break?

Actually my idea is to allow more than one registered handler to
handle this request, so I define a flag rather than return to
indicated if there is at least one handler successfully do it. This design

might give more flexibility to framework when running.
There is only one flag(success_handled) here, how did know which handler
failed?

BTW, I don't think we need this flag. We can only use the return value.

Well, the plat_pm driver will not handle most errors returned by registered
handlers, except -NODEV. For -NODEV, plat_pm driver consider that handler
cannot support this request and will go to call next one. 

Besides, actually it doesn't restrict that request can be served by only one 
handler. So I add that flag to cover the case of more than one handler can 
successfully support and others might return -NODEV.

Regards,
Ran

Cheers,
Dongsheng

quoted

quoted

quoted

+			else if (ret != -ENODEV) {
+				pr_err("FSL plat_pm: Failed to config wakeup

source:%d\n", ret);
Please unlock before return.

Yes, will fix it in next version, thanks for pointing out!

quoted

quoted

+				return ret;
+			}
+		} else
+			pr_warn("FSL plat_pm: Invalid handler detected,

skip\n");

quoted

+	}
+	spin_unlock_irqrestore(&plat_pm.lock, flags);
+
+	if (success_handled == false) {
+		pr_err("FSL plat_pm: Cannot find the matchhed handler for

wakeup source config\n");

quoted

+		return -ENODEV;
+	}

Add this into the loop.

My design is that if the 1st handler return -ENODEV to indicated this
device it doesn't support, then the framework will continue try 2nd

handler...

quoted

So I think it is needed to place this checking out of loop, what do you say?

Regards,
Ran

quoted

quoted

+
+	return 0;
+}
+
+// fsl_platform_wakeup_enable - Enable wakeup source // @dev:
+pointer to user's device struct // // Return 0 on success other
+negative errno int fsl_platform_wakeup_enable(struct device *dev) {
+	return fsl_platform_wakeup_config(dev, true); }
+EXPORT_SYMBOL_GPL(fsl_platform_wakeup_enable);
+
+// fsl_platform_wakeup_disable - Disable wakeup source // @dev:
+pointer to user's device struct // // Return 0 on success other
+negative errno int fsl_platform_wakeup_disable(struct device *dev) {
+	return fsl_platform_wakeup_config(dev, false); }
+EXPORT_SYMBOL_GPL(fsl_platform_wakeup_disable);
+
+static int __init fsl_plat_pm_init(void) {
+	spin_lock_init(&plat_pm.lock);
+	INIT_LIST_HEAD(&plat_pm.node);
+	return 0;
+}
+
+core_initcall(fsl_plat_pm_init);

diff --git a/include/soc/fsl/plat_pm.h b/include/soc/fsl/plat_pm.h
new file mode 100644 index 0000000..bbe151e
--- /dev/null
+++ b/include/soc/fsl/plat_pm.h

@@ -0,0 +1,22 @@
+// SPDX-License-Identifier: GPL-2.0 // // plat_pm.h - Freescale
+platform PM Header // // Copyright 2018 NXP // // Author: Ran Wang
+<ran.wang_1@nxp.com>,
+
+#ifndef __FSL_PLAT_PM_H
+#define __FSL_PLAT_PM_H
+
+typedef int (*fsl_plat_pm_handle)(struct device *dev, bool flag,
+		void *handle_priv);
+
+int register_fsl_platform_wakeup_source(fsl_plat_pm_handle handle,
+		void *handle_priv);
+int deregister_fsl_platform_wakeup_source(void *handle_priv); int
+fsl_platform_wakeup_config(struct device *dev, bool flag); int
+fsl_platform_wakeup_enable(struct device *dev); int
+fsl_platform_wakeup_disable(struct device *dev);
+
+#endif	// __FSL_PLAT_PM_H