* [PATCH 2/2] soc: nxp: Add a RCPM driver
@ 2016-08-01 9:49 ` Chenhui Zhao
0 siblings, 0 replies; 21+ messages in thread
From: Chenhui Zhao @ 2016-08-01 9:49 UTC (permalink / raw)
To: linux-arm-kernel
The NXP's QorIQ Processors based on ARM Core have a RCPM module
(Run Control and Power Management), which performs all device-level
tasks associated with power management.
This patch mainly implements the wakeup sources configuration before
entering LPM20, a low power state of device-level. The devices can be
waked up by specified sources, such as Flextimer, GPIO and so on.
Signed-off-by: Chenhui Zhao <chenhui.zhao@nxp.com>
---
drivers/soc/fsl/Makefile | 1 +
drivers/soc/fsl/pm/Makefile | 1 +
drivers/soc/fsl/pm/ls-rcpm.c | 144 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 146 insertions(+)
create mode 100644 drivers/soc/fsl/pm/Makefile
create mode 100644 drivers/soc/fsl/pm/ls-rcpm.c
diff --git a/drivers/soc/fsl/Makefile b/drivers/soc/fsl/Makefile
index 203307f..b5adbaf 100644
--- a/drivers/soc/fsl/Makefile
+++ b/drivers/soc/fsl/Makefile
@@ -4,3 +4,4 @@
obj-$(CONFIG_QUICC_ENGINE) += qe/
obj-$(CONFIG_CPM) += qe/
+obj-$(CONFIG_SUSPEND) += pm/
diff --git a/drivers/soc/fsl/pm/Makefile b/drivers/soc/fsl/pm/Makefile
new file mode 100644
index 0000000..e275d63
--- /dev/null
+++ b/drivers/soc/fsl/pm/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_ARCH_LAYERSCAPE) += ls-rcpm.o
diff --git a/drivers/soc/fsl/pm/ls-rcpm.c b/drivers/soc/fsl/pm/ls-rcpm.c
new file mode 100644
index 0000000..c5f2b97
--- /dev/null
+++ b/drivers/soc/fsl/pm/ls-rcpm.c
@@ -0,0 +1,144 @@
+/*
+ * Run Control and Power Management (RCPM) driver
+ *
+ * Copyright 2016 Freescale Semiconductor, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ *
+ */
+#define pr_fmt(fmt) "RCPM: %s: " fmt, __func__
+
+#include <linux/kernel.h>
+#include <linux/io.h>
+#include <linux/of_platform.h>
+#include <linux/of_address.h>
+#include <linux/suspend.h>
+
+/* So far there are not more than two registers */
+#define RCPM_IPPDEXPCR0 0x140
+#define RCPM_IPPDEXPCR1 0x144
+#define RCPM_IPPDEXPCR(x) (RCPM_IPPDEXPCR0 + 4 * x)
+#define RCPM_WAKEUP_CELL_MAX_SIZE 2
+
+/* it reprents the number of the registers RCPM_IPPDEXPCR */
+static unsigned int rcpm_wakeup_cells;
+static void __iomem *rcpm_reg_base;
+static u32 ippdexpcr[RCPM_WAKEUP_CELL_MAX_SIZE];
+
+static inline void rcpm_reg_write(u32 offset, u32 value)
+{
+ iowrite32be(value, rcpm_reg_base + offset);
+}
+
+static inline u32 rcpm_reg_read(u32 offset)
+{
+ return ioread32be(rcpm_reg_base + offset);
+}
+
+static void rcpm_wakeup_fixup(struct device *dev, void *data)
+{
+ struct device_node *node = dev ? dev->of_node : NULL;
+ u32 value[RCPM_WAKEUP_CELL_MAX_SIZE + 1];
+ int ret;
+ int i;
+
+ if (!dev || !node || !device_may_wakeup(dev))
+ return;
+
+ /*
+ * Get the values in the "rcpm-wakeup" property.
+ * Refer to Documentation/devicetree/bindings/soc/fsl/rcpm.txt
+ */
+ ret = of_property_read_u32_array(node, "rcpm-wakeup",
+ value, rcpm_wakeup_cells + 1);
+ if (ret)
+ return;
+
+ pr_debug("wakeup source: the device %s\n", node->full_name);
+
+ for (i = 0; i < rcpm_wakeup_cells; i++)
+ ippdexpcr[i] |= value[i + 1];
+}
+
+static int rcpm_suspend_prepare(void)
+{
+ int i;
+
+ for (i = 0; i < rcpm_wakeup_cells; i++)
+ ippdexpcr[i] = 0;
+
+ dpm_for_each_dev(NULL, rcpm_wakeup_fixup);
+
+ for (i = 0; i < rcpm_wakeup_cells; i++) {
+ rcpm_reg_write(RCPM_IPPDEXPCR(i), ippdexpcr[i]);
+ pr_debug("ippdexpcr%d = 0x%x\n", i, ippdexpcr[i]);
+ }
+
+ return 0;
+}
+
+static int rcpm_suspend_notifier_call(struct notifier_block *bl,
+ unsigned long state,
+ void *unused)
+{
+ switch (state) {
+ case PM_SUSPEND_PREPARE:
+ rcpm_suspend_prepare();
+ break;
+ }
+
+ return NOTIFY_DONE;
+}
+
+static const struct of_device_id rcpm_matches[] = {
+ {
+ .compatible = "fsl,qoriq-rcpm-2.1",
+ },
+ {}
+};
+
+static struct notifier_block rcpm_suspend_notifier = {
+ .notifier_call = rcpm_suspend_notifier_call,
+};
+
+static int __init layerscape_rcpm_init(void)
+{
+ struct device_node *np;
+ int ret;
+
+ np = of_find_matching_node_and_match(NULL, rcpm_matches, NULL);
+ if (!np)
+ return -ENODEV;
+
+ ret = of_property_read_u32_index(np, "fsl,#rcpm-wakeup-cells", 0,
+ &rcpm_wakeup_cells);
+ if (ret) {
+ pr_err("Fail to get \"fsl,#rcpm-wakeup-cells\".\n");
+ return -EINVAL;
+ }
+
+ if (rcpm_wakeup_cells > RCPM_WAKEUP_CELL_MAX_SIZE) {
+ pr_err("The value of \"fsl,#rcpm-wakeup-cells\" is wrong.\n");
+ return -EINVAL;
+ }
+
+ rcpm_reg_base = of_iomap(np, 0);
+ if (!rcpm_reg_base)
+ return -ENOMEM;
+
+ register_pm_notifier(&rcpm_suspend_notifier);
+
+ pr_info("The RCPM driver initialized.\n");
+
+ return 0;
+}
+
+subsys_initcall(layerscape_rcpm_init);
--
1.9.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 2/2] soc: nxp: Add a RCPM driver
@ 2016-08-01 9:49 ` Chenhui Zhao
0 siblings, 0 replies; 21+ messages in thread
From: Chenhui Zhao @ 2016-08-01 9:49 UTC (permalink / raw)
To: devicetree, linux-arm-kernel, linux-kernel
Cc: z.chenhui, Chenhui Zhao, jason.jin
The NXP's QorIQ Processors based on ARM Core have a RCPM module
(Run Control and Power Management), which performs all device-level
tasks associated with power management.
This patch mainly implements the wakeup sources configuration before
entering LPM20, a low power state of device-level. The devices can be
waked up by specified sources, such as Flextimer, GPIO and so on.
Signed-off-by: Chenhui Zhao <chenhui.zhao@nxp.com>
---
drivers/soc/fsl/Makefile | 1 +
drivers/soc/fsl/pm/Makefile | 1 +
drivers/soc/fsl/pm/ls-rcpm.c | 144 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 146 insertions(+)
create mode 100644 drivers/soc/fsl/pm/Makefile
create mode 100644 drivers/soc/fsl/pm/ls-rcpm.c
diff --git a/drivers/soc/fsl/Makefile b/drivers/soc/fsl/Makefile
index 203307f..b5adbaf 100644
--- a/drivers/soc/fsl/Makefile
+++ b/drivers/soc/fsl/Makefile
@@ -4,3 +4,4 @@
obj-$(CONFIG_QUICC_ENGINE) += qe/
obj-$(CONFIG_CPM) += qe/
+obj-$(CONFIG_SUSPEND) += pm/
diff --git a/drivers/soc/fsl/pm/Makefile b/drivers/soc/fsl/pm/Makefile
new file mode 100644
index 0000000..e275d63
--- /dev/null
+++ b/drivers/soc/fsl/pm/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_ARCH_LAYERSCAPE) += ls-rcpm.o
diff --git a/drivers/soc/fsl/pm/ls-rcpm.c b/drivers/soc/fsl/pm/ls-rcpm.c
new file mode 100644
index 0000000..c5f2b97
--- /dev/null
+++ b/drivers/soc/fsl/pm/ls-rcpm.c
@@ -0,0 +1,144 @@
+/*
+ * Run Control and Power Management (RCPM) driver
+ *
+ * Copyright 2016 Freescale Semiconductor, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ *
+ */
+#define pr_fmt(fmt) "RCPM: %s: " fmt, __func__
+
+#include <linux/kernel.h>
+#include <linux/io.h>
+#include <linux/of_platform.h>
+#include <linux/of_address.h>
+#include <linux/suspend.h>
+
+/* So far there are not more than two registers */
+#define RCPM_IPPDEXPCR0 0x140
+#define RCPM_IPPDEXPCR1 0x144
+#define RCPM_IPPDEXPCR(x) (RCPM_IPPDEXPCR0 + 4 * x)
+#define RCPM_WAKEUP_CELL_MAX_SIZE 2
+
+/* it reprents the number of the registers RCPM_IPPDEXPCR */
+static unsigned int rcpm_wakeup_cells;
+static void __iomem *rcpm_reg_base;
+static u32 ippdexpcr[RCPM_WAKEUP_CELL_MAX_SIZE];
+
+static inline void rcpm_reg_write(u32 offset, u32 value)
+{
+ iowrite32be(value, rcpm_reg_base + offset);
+}
+
+static inline u32 rcpm_reg_read(u32 offset)
+{
+ return ioread32be(rcpm_reg_base + offset);
+}
+
+static void rcpm_wakeup_fixup(struct device *dev, void *data)
+{
+ struct device_node *node = dev ? dev->of_node : NULL;
+ u32 value[RCPM_WAKEUP_CELL_MAX_SIZE + 1];
+ int ret;
+ int i;
+
+ if (!dev || !node || !device_may_wakeup(dev))
+ return;
+
+ /*
+ * Get the values in the "rcpm-wakeup" property.
+ * Refer to Documentation/devicetree/bindings/soc/fsl/rcpm.txt
+ */
+ ret = of_property_read_u32_array(node, "rcpm-wakeup",
+ value, rcpm_wakeup_cells + 1);
+ if (ret)
+ return;
+
+ pr_debug("wakeup source: the device %s\n", node->full_name);
+
+ for (i = 0; i < rcpm_wakeup_cells; i++)
+ ippdexpcr[i] |= value[i + 1];
+}
+
+static int rcpm_suspend_prepare(void)
+{
+ int i;
+
+ for (i = 0; i < rcpm_wakeup_cells; i++)
+ ippdexpcr[i] = 0;
+
+ dpm_for_each_dev(NULL, rcpm_wakeup_fixup);
+
+ for (i = 0; i < rcpm_wakeup_cells; i++) {
+ rcpm_reg_write(RCPM_IPPDEXPCR(i), ippdexpcr[i]);
+ pr_debug("ippdexpcr%d = 0x%x\n", i, ippdexpcr[i]);
+ }
+
+ return 0;
+}
+
+static int rcpm_suspend_notifier_call(struct notifier_block *bl,
+ unsigned long state,
+ void *unused)
+{
+ switch (state) {
+ case PM_SUSPEND_PREPARE:
+ rcpm_suspend_prepare();
+ break;
+ }
+
+ return NOTIFY_DONE;
+}
+
+static const struct of_device_id rcpm_matches[] = {
+ {
+ .compatible = "fsl,qoriq-rcpm-2.1",
+ },
+ {}
+};
+
+static struct notifier_block rcpm_suspend_notifier = {
+ .notifier_call = rcpm_suspend_notifier_call,
+};
+
+static int __init layerscape_rcpm_init(void)
+{
+ struct device_node *np;
+ int ret;
+
+ np = of_find_matching_node_and_match(NULL, rcpm_matches, NULL);
+ if (!np)
+ return -ENODEV;
+
+ ret = of_property_read_u32_index(np, "fsl,#rcpm-wakeup-cells", 0,
+ &rcpm_wakeup_cells);
+ if (ret) {
+ pr_err("Fail to get \"fsl,#rcpm-wakeup-cells\".\n");
+ return -EINVAL;
+ }
+
+ if (rcpm_wakeup_cells > RCPM_WAKEUP_CELL_MAX_SIZE) {
+ pr_err("The value of \"fsl,#rcpm-wakeup-cells\" is wrong.\n");
+ return -EINVAL;
+ }
+
+ rcpm_reg_base = of_iomap(np, 0);
+ if (!rcpm_reg_base)
+ return -ENOMEM;
+
+ register_pm_notifier(&rcpm_suspend_notifier);
+
+ pr_info("The RCPM driver initialized.\n");
+
+ return 0;
+}
+
+subsys_initcall(layerscape_rcpm_init);
--
1.9.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 2/2] soc: nxp: Add a RCPM driver
2016-08-01 9:49 ` Chenhui Zhao
(?)
@ 2016-08-01 12:25 ` Arnd Bergmann
-1 siblings, 0 replies; 21+ messages in thread
From: Arnd Bergmann @ 2016-08-01 12:25 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Chenhui Zhao, devicetree, linux-kernel, z.chenhui, jason.jin,
Thomas Gleixner, Jason Cooper, Marc Zyngier
On Monday, August 1, 2016 5:49:03 PM CEST Chenhui Zhao wrote:
> The NXP's QorIQ Processors based on ARM Core have a RCPM module
> (Run Control and Power Management), which performs all device-level
> tasks associated with power management.
>
> This patch mainly implements the wakeup sources configuration before
> entering LPM20, a low power state of device-level. The devices can be
> waked up by specified sources, such as Flextimer, GPIO and so on.
>
> Signed-off-by: Chenhui Zhao <chenhui.zhao@nxp.com>
Adding irqchip maintainers to cc, as this wakeup handling is normally
part of the irq controller.
> +
> +#include <linux/kernel.h>
> +#include <linux/io.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_address.h>
> +#include <linux/suspend.h>
> +
> +/* So far there are not more than two registers */
> +#define RCPM_IPPDEXPCR0 0x140
> +#define RCPM_IPPDEXPCR1 0x144
> +#define RCPM_IPPDEXPCR(x) (RCPM_IPPDEXPCR0 + 4 * x)
> +#define RCPM_WAKEUP_CELL_MAX_SIZE 2
> +
> +/* it reprents the number of the registers RCPM_IPPDEXPCR */
> +static unsigned int rcpm_wakeup_cells;
> +static void __iomem *rcpm_reg_base;
> +static u32 ippdexpcr[RCPM_WAKEUP_CELL_MAX_SIZE];
Can you make these local to the context of whoever
calls into the driver?
> +static void rcpm_wakeup_fixup(struct device *dev, void *data)
> +{
> + struct device_node *node = dev ? dev->of_node : NULL;
> + u32 value[RCPM_WAKEUP_CELL_MAX_SIZE + 1];
> + int ret;
> + int i;
> +
> + if (!dev || !node || !device_may_wakeup(dev))
> + return;
> +
> + /*
> + * Get the values in the "rcpm-wakeup" property.
> + * Refer to Documentation/devicetree/bindings/soc/fsl/rcpm.txt
> + */
> + ret = of_property_read_u32_array(node, "rcpm-wakeup",
> + value, rcpm_wakeup_cells + 1);
My first impression is that you are trying to do something
in a platform specific way that should be handled by common
code here.
You are parsing rcpm_wakeup_cells once for the global node,
but you don't check whether the device that has the rcpm-wakeup
node actually refers to this instance, and that would require
an incompatible change if we ever get an implementation that
has multiple such nodes.
Arnd
^ permalink raw reply [flat|nested] 21+ messages in thread* [PATCH 2/2] soc: nxp: Add a RCPM driver
@ 2016-08-01 12:25 ` Arnd Bergmann
0 siblings, 0 replies; 21+ messages in thread
From: Arnd Bergmann @ 2016-08-01 12:25 UTC (permalink / raw)
To: linux-arm-kernel
On Monday, August 1, 2016 5:49:03 PM CEST Chenhui Zhao wrote:
> The NXP's QorIQ Processors based on ARM Core have a RCPM module
> (Run Control and Power Management), which performs all device-level
> tasks associated with power management.
>
> This patch mainly implements the wakeup sources configuration before
> entering LPM20, a low power state of device-level. The devices can be
> waked up by specified sources, such as Flextimer, GPIO and so on.
>
> Signed-off-by: Chenhui Zhao <chenhui.zhao@nxp.com>
Adding irqchip maintainers to cc, as this wakeup handling is normally
part of the irq controller.
> +
> +#include <linux/kernel.h>
> +#include <linux/io.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_address.h>
> +#include <linux/suspend.h>
> +
> +/* So far there are not more than two registers */
> +#define RCPM_IPPDEXPCR0 0x140
> +#define RCPM_IPPDEXPCR1 0x144
> +#define RCPM_IPPDEXPCR(x) (RCPM_IPPDEXPCR0 + 4 * x)
> +#define RCPM_WAKEUP_CELL_MAX_SIZE 2
> +
> +/* it reprents the number of the registers RCPM_IPPDEXPCR */
> +static unsigned int rcpm_wakeup_cells;
> +static void __iomem *rcpm_reg_base;
> +static u32 ippdexpcr[RCPM_WAKEUP_CELL_MAX_SIZE];
Can you make these local to the context of whoever
calls into the driver?
> +static void rcpm_wakeup_fixup(struct device *dev, void *data)
> +{
> + struct device_node *node = dev ? dev->of_node : NULL;
> + u32 value[RCPM_WAKEUP_CELL_MAX_SIZE + 1];
> + int ret;
> + int i;
> +
> + if (!dev || !node || !device_may_wakeup(dev))
> + return;
> +
> + /*
> + * Get the values in the "rcpm-wakeup" property.
> + * Refer to Documentation/devicetree/bindings/soc/fsl/rcpm.txt
> + */
> + ret = of_property_read_u32_array(node, "rcpm-wakeup",
> + value, rcpm_wakeup_cells + 1);
My first impression is that you are trying to do something
in a platform specific way that should be handled by common
code here.
You are parsing rcpm_wakeup_cells once for the global node,
but you don't check whether the device that has the rcpm-wakeup
node actually refers to this instance, and that would require
an incompatible change if we ever get an implementation that
has multiple such nodes.
Arnd
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/2] soc: nxp: Add a RCPM driver
@ 2016-08-01 12:25 ` Arnd Bergmann
0 siblings, 0 replies; 21+ messages in thread
From: Arnd Bergmann @ 2016-08-01 12:25 UTC (permalink / raw)
To: linux-arm-kernel
Cc: devicetree, z.chenhui, Chenhui Zhao, jason.jin, Marc Zyngier,
linux-kernel, Thomas Gleixner, Jason Cooper
On Monday, August 1, 2016 5:49:03 PM CEST Chenhui Zhao wrote:
> The NXP's QorIQ Processors based on ARM Core have a RCPM module
> (Run Control and Power Management), which performs all device-level
> tasks associated with power management.
>
> This patch mainly implements the wakeup sources configuration before
> entering LPM20, a low power state of device-level. The devices can be
> waked up by specified sources, such as Flextimer, GPIO and so on.
>
> Signed-off-by: Chenhui Zhao <chenhui.zhao@nxp.com>
Adding irqchip maintainers to cc, as this wakeup handling is normally
part of the irq controller.
> +
> +#include <linux/kernel.h>
> +#include <linux/io.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_address.h>
> +#include <linux/suspend.h>
> +
> +/* So far there are not more than two registers */
> +#define RCPM_IPPDEXPCR0 0x140
> +#define RCPM_IPPDEXPCR1 0x144
> +#define RCPM_IPPDEXPCR(x) (RCPM_IPPDEXPCR0 + 4 * x)
> +#define RCPM_WAKEUP_CELL_MAX_SIZE 2
> +
> +/* it reprents the number of the registers RCPM_IPPDEXPCR */
> +static unsigned int rcpm_wakeup_cells;
> +static void __iomem *rcpm_reg_base;
> +static u32 ippdexpcr[RCPM_WAKEUP_CELL_MAX_SIZE];
Can you make these local to the context of whoever
calls into the driver?
> +static void rcpm_wakeup_fixup(struct device *dev, void *data)
> +{
> + struct device_node *node = dev ? dev->of_node : NULL;
> + u32 value[RCPM_WAKEUP_CELL_MAX_SIZE + 1];
> + int ret;
> + int i;
> +
> + if (!dev || !node || !device_may_wakeup(dev))
> + return;
> +
> + /*
> + * Get the values in the "rcpm-wakeup" property.
> + * Refer to Documentation/devicetree/bindings/soc/fsl/rcpm.txt
> + */
> + ret = of_property_read_u32_array(node, "rcpm-wakeup",
> + value, rcpm_wakeup_cells + 1);
My first impression is that you are trying to do something
in a platform specific way that should be handled by common
code here.
You are parsing rcpm_wakeup_cells once for the global node,
but you don't check whether the device that has the rcpm-wakeup
node actually refers to this instance, and that would require
an incompatible change if we ever get an implementation that
has multiple such nodes.
Arnd
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/2] soc: nxp: Add a RCPM driver
2016-08-01 12:25 ` Arnd Bergmann
(?)
@ 2016-08-01 12:55 ` Marc Zyngier
-1 siblings, 0 replies; 21+ messages in thread
From: Marc Zyngier @ 2016-08-01 12:55 UTC (permalink / raw)
To: Arnd Bergmann, linux-arm-kernel
Cc: Chenhui Zhao, devicetree, linux-kernel, z.chenhui, jason.jin,
Thomas Gleixner, Jason Cooper
On 01/08/16 13:25, Arnd Bergmann wrote:
> On Monday, August 1, 2016 5:49:03 PM CEST Chenhui Zhao wrote:
>> The NXP's QorIQ Processors based on ARM Core have a RCPM module
>> (Run Control and Power Management), which performs all device-level
>> tasks associated with power management.
>>
>> This patch mainly implements the wakeup sources configuration before
>> entering LPM20, a low power state of device-level. The devices can be
>> waked up by specified sources, such as Flextimer, GPIO and so on.
>>
>> Signed-off-by: Chenhui Zhao <chenhui.zhao@nxp.com>
>
> Adding irqchip maintainers to cc, as this wakeup handling is normally
> part of the irq controller.
Thanks for looping me in. This very much look like it should be a
stacked irqchip, just like we handle everything else. I'll go and review
the actual patch.
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/2] soc: nxp: Add a RCPM driver
@ 2016-08-01 12:55 ` Marc Zyngier
0 siblings, 0 replies; 21+ messages in thread
From: Marc Zyngier @ 2016-08-01 12:55 UTC (permalink / raw)
To: linux-arm-kernel
On 01/08/16 13:25, Arnd Bergmann wrote:
> On Monday, August 1, 2016 5:49:03 PM CEST Chenhui Zhao wrote:
>> The NXP's QorIQ Processors based on ARM Core have a RCPM module
>> (Run Control and Power Management), which performs all device-level
>> tasks associated with power management.
>>
>> This patch mainly implements the wakeup sources configuration before
>> entering LPM20, a low power state of device-level. The devices can be
>> waked up by specified sources, such as Flextimer, GPIO and so on.
>>
>> Signed-off-by: Chenhui Zhao <chenhui.zhao@nxp.com>
>
> Adding irqchip maintainers to cc, as this wakeup handling is normally
> part of the irq controller.
Thanks for looping me in. This very much look like it should be a
stacked irqchip, just like we handle everything else. I'll go and review
the actual patch.
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/2] soc: nxp: Add a RCPM driver
@ 2016-08-01 12:55 ` Marc Zyngier
0 siblings, 0 replies; 21+ messages in thread
From: Marc Zyngier @ 2016-08-01 12:55 UTC (permalink / raw)
To: Arnd Bergmann, linux-arm-kernel
Cc: devicetree, z.chenhui, Chenhui Zhao, jason.jin, linux-kernel,
Thomas Gleixner, Jason Cooper
On 01/08/16 13:25, Arnd Bergmann wrote:
> On Monday, August 1, 2016 5:49:03 PM CEST Chenhui Zhao wrote:
>> The NXP's QorIQ Processors based on ARM Core have a RCPM module
>> (Run Control and Power Management), which performs all device-level
>> tasks associated with power management.
>>
>> This patch mainly implements the wakeup sources configuration before
>> entering LPM20, a low power state of device-level. The devices can be
>> waked up by specified sources, such as Flextimer, GPIO and so on.
>>
>> Signed-off-by: Chenhui Zhao <chenhui.zhao@nxp.com>
>
> Adding irqchip maintainers to cc, as this wakeup handling is normally
> part of the irq controller.
Thanks for looping me in. This very much look like it should be a
stacked irqchip, just like we handle everything else. I'll go and review
the actual patch.
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/2] soc: nxp: Add a RCPM driver
2016-08-01 12:25 ` Arnd Bergmann
(?)
@ 2016-08-03 3:42 ` Chenhui Zhao
-1 siblings, 0 replies; 21+ messages in thread
From: Chenhui Zhao @ 2016-08-03 3:42 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-arm-kernel, Chenhui Zhao, devicetree, linux-kernel,
jason.jin, Thomas Gleixner, Jason Cooper, Marc Zyngier
On Mon, Aug 1, 2016 at 8:25 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday, August 1, 2016 5:49:03 PM CEST Chenhui Zhao wrote:
>> The NXP's QorIQ Processors based on ARM Core have a RCPM module
>> (Run Control and Power Management), which performs all device-level
>> tasks associated with power management.
>>
>> This patch mainly implements the wakeup sources configuration before
>> entering LPM20, a low power state of device-level. The devices can be
>> waked up by specified sources, such as Flextimer, GPIO and so on.
>>
>> Signed-off-by: Chenhui Zhao <chenhui.zhao@nxp.com>
>
> Adding irqchip maintainers to cc, as this wakeup handling is normally
> part of the irq controller.
>
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/io.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/of_address.h>
>> +#include <linux/suspend.h>
>> +
>> +/* So far there are not more than two registers */
>> +#define RCPM_IPPDEXPCR0 0x140
>> +#define RCPM_IPPDEXPCR1 0x144
>> +#define RCPM_IPPDEXPCR(x) (RCPM_IPPDEXPCR0 + 4 * x)
>> +#define RCPM_WAKEUP_CELL_MAX_SIZE 2
>> +
>> +/* it reprents the number of the registers RCPM_IPPDEXPCR */
>> +static unsigned int rcpm_wakeup_cells;
>> +static void __iomem *rcpm_reg_base;
>> +static u32 ippdexpcr[RCPM_WAKEUP_CELL_MAX_SIZE];
>
> Can you make these local to the context of whoever
> calls into the driver?
>
>
>> +static void rcpm_wakeup_fixup(struct device *dev, void *data)
>> +{
>> + struct device_node *node = dev ? dev->of_node : NULL;
>> + u32 value[RCPM_WAKEUP_CELL_MAX_SIZE + 1];
>> + int ret;
>> + int i;
>> +
>> + if (!dev || !node || !device_may_wakeup(dev))
>> + return;
>> +
>> + /*
>> + * Get the values in the "rcpm-wakeup" property.
>> + * Refer to Documentation/devicetree/bindings/soc/fsl/rcpm.txt
>> + */
>> + ret = of_property_read_u32_array(node, "rcpm-wakeup",c
>> + value, rcpm_wakeup_cells + 1);
>
> My first impression is that you are trying to do something
> in a platform specific way that should be handled by common
> code here.
>
> You are parsing rcpm_wakeup_cells once for the global node,
> but you don't check whether the device that has the rcpm-wakeup
> node actually refers to this instance, and that would require
> an incompatible change if we ever get an implementation that
> has multiple such nodes.
>
> Arnd
The code is specific to the QorIQ platform.
For a given QorIQ SoC, the value of the "fsl,#rcpm-wakeup-cells"
property would not change. Anyway, your suggestion is better getting
the rcpm node from the "rcpm-wakeup" property.
Thanks,
Chenhui
^ permalink raw reply [flat|nested] 21+ messages in thread* [PATCH 2/2] soc: nxp: Add a RCPM driver
@ 2016-08-03 3:42 ` Chenhui Zhao
0 siblings, 0 replies; 21+ messages in thread
From: Chenhui Zhao @ 2016-08-03 3:42 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Aug 1, 2016 at 8:25 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday, August 1, 2016 5:49:03 PM CEST Chenhui Zhao wrote:
>> The NXP's QorIQ Processors based on ARM Core have a RCPM module
>> (Run Control and Power Management), which performs all device-level
>> tasks associated with power management.
>>
>> This patch mainly implements the wakeup sources configuration before
>> entering LPM20, a low power state of device-level. The devices can be
>> waked up by specified sources, such as Flextimer, GPIO and so on.
>>
>> Signed-off-by: Chenhui Zhao <chenhui.zhao@nxp.com>
>
> Adding irqchip maintainers to cc, as this wakeup handling is normally
> part of the irq controller.
>
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/io.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/of_address.h>
>> +#include <linux/suspend.h>
>> +
>> +/* So far there are not more than two registers */
>> +#define RCPM_IPPDEXPCR0 0x140
>> +#define RCPM_IPPDEXPCR1 0x144
>> +#define RCPM_IPPDEXPCR(x) (RCPM_IPPDEXPCR0 + 4 * x)
>> +#define RCPM_WAKEUP_CELL_MAX_SIZE 2
>> +
>> +/* it reprents the number of the registers RCPM_IPPDEXPCR */
>> +static unsigned int rcpm_wakeup_cells;
>> +static void __iomem *rcpm_reg_base;
>> +static u32 ippdexpcr[RCPM_WAKEUP_CELL_MAX_SIZE];
>
> Can you make these local to the context of whoever
> calls into the driver?
>
>
>> +static void rcpm_wakeup_fixup(struct device *dev, void *data)
>> +{
>> + struct device_node *node = dev ? dev->of_node : NULL;
>> + u32 value[RCPM_WAKEUP_CELL_MAX_SIZE + 1];
>> + int ret;
>> + int i;
>> +
>> + if (!dev || !node || !device_may_wakeup(dev))
>> + return;
>> +
>> + /*
>> + * Get the values in the "rcpm-wakeup" property.
>> + * Refer to Documentation/devicetree/bindings/soc/fsl/rcpm.txt
>> + */
>> + ret = of_property_read_u32_array(node, "rcpm-wakeup",c
>> + value, rcpm_wakeup_cells + 1);
>
> My first impression is that you are trying to do something
> in a platform specific way that should be handled by common
> code here.
>
> You are parsing rcpm_wakeup_cells once for the global node,
> but you don't check whether the device that has the rcpm-wakeup
> node actually refers to this instance, and that would require
> an incompatible change if we ever get an implementation that
> has multiple such nodes.
>
> Arnd
The code is specific to the QorIQ platform.
For a given QorIQ SoC, the value of the "fsl,#rcpm-wakeup-cells"
property would not change. Anyway, your suggestion is better getting
the rcpm node from the "rcpm-wakeup" property.
Thanks,
Chenhui
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/2] soc: nxp: Add a RCPM driver
@ 2016-08-03 3:42 ` Chenhui Zhao
0 siblings, 0 replies; 21+ messages in thread
From: Chenhui Zhao @ 2016-08-03 3:42 UTC (permalink / raw)
To: Arnd Bergmann
Cc: devicetree, Chenhui Zhao, jason.jin, Marc Zyngier, linux-kernel,
Thomas Gleixner, linux-arm-kernel, Jason Cooper
On Mon, Aug 1, 2016 at 8:25 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday, August 1, 2016 5:49:03 PM CEST Chenhui Zhao wrote:
>> The NXP's QorIQ Processors based on ARM Core have a RCPM module
>> (Run Control and Power Management), which performs all device-level
>> tasks associated with power management.
>>
>> This patch mainly implements the wakeup sources configuration before
>> entering LPM20, a low power state of device-level. The devices can be
>> waked up by specified sources, such as Flextimer, GPIO and so on.
>>
>> Signed-off-by: Chenhui Zhao <chenhui.zhao@nxp.com>
>
> Adding irqchip maintainers to cc, as this wakeup handling is normally
> part of the irq controller.
>
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/io.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/of_address.h>
>> +#include <linux/suspend.h>
>> +
>> +/* So far there are not more than two registers */
>> +#define RCPM_IPPDEXPCR0 0x140
>> +#define RCPM_IPPDEXPCR1 0x144
>> +#define RCPM_IPPDEXPCR(x) (RCPM_IPPDEXPCR0 + 4 * x)
>> +#define RCPM_WAKEUP_CELL_MAX_SIZE 2
>> +
>> +/* it reprents the number of the registers RCPM_IPPDEXPCR */
>> +static unsigned int rcpm_wakeup_cells;
>> +static void __iomem *rcpm_reg_base;
>> +static u32 ippdexpcr[RCPM_WAKEUP_CELL_MAX_SIZE];
>
> Can you make these local to the context of whoever
> calls into the driver?
>
>
>> +static void rcpm_wakeup_fixup(struct device *dev, void *data)
>> +{
>> + struct device_node *node = dev ? dev->of_node : NULL;
>> + u32 value[RCPM_WAKEUP_CELL_MAX_SIZE + 1];
>> + int ret;
>> + int i;
>> +
>> + if (!dev || !node || !device_may_wakeup(dev))
>> + return;
>> +
>> + /*
>> + * Get the values in the "rcpm-wakeup" property.
>> + * Refer to Documentation/devicetree/bindings/soc/fsl/rcpm.txt
>> + */
>> + ret = of_property_read_u32_array(node, "rcpm-wakeup",c
>> + value, rcpm_wakeup_cells + 1);
>
> My first impression is that you are trying to do something
> in a platform specific way that should be handled by common
> code here.
>
> You are parsing rcpm_wakeup_cells once for the global node,
> but you don't check whether the device that has the rcpm-wakeup
> node actually refers to this instance, and that would require
> an incompatible change if we ever get an implementation that
> has multiple such nodes.
>
> Arnd
The code is specific to the QorIQ platform.
For a given QorIQ SoC, the value of the "fsl,#rcpm-wakeup-cells"
property would not change. Anyway, your suggestion is better getting
the rcpm node from the "rcpm-wakeup" property.
Thanks,
Chenhui
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/2] soc: nxp: Add a RCPM driver
@ 2016-08-01 13:22 ` Marc Zyngier
0 siblings, 0 replies; 21+ messages in thread
From: Marc Zyngier @ 2016-08-01 13:22 UTC (permalink / raw)
To: Chenhui Zhao, devicetree, linux-arm-kernel, linux-kernel
Cc: z.chenhui, jason.jin, Mark Rutland
On 01/08/16 10:49, Chenhui Zhao wrote:
> The NXP's QorIQ Processors based on ARM Core have a RCPM module
> (Run Control and Power Management), which performs all device-level
> tasks associated with power management.
>
> This patch mainly implements the wakeup sources configuration before
> entering LPM20, a low power state of device-level. The devices can be
> waked up by specified sources, such as Flextimer, GPIO and so on.
>
> Signed-off-by: Chenhui Zhao <chenhui.zhao@nxp.com>
> ---
> drivers/soc/fsl/Makefile | 1 +
> drivers/soc/fsl/pm/Makefile | 1 +
> drivers/soc/fsl/pm/ls-rcpm.c | 144 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 146 insertions(+)
> create mode 100644 drivers/soc/fsl/pm/Makefile
> create mode 100644 drivers/soc/fsl/pm/ls-rcpm.c
>
> diff --git a/drivers/soc/fsl/Makefile b/drivers/soc/fsl/Makefile
> index 203307f..b5adbaf 100644
> --- a/drivers/soc/fsl/Makefile
> +++ b/drivers/soc/fsl/Makefile
> @@ -4,3 +4,4 @@
>
> obj-$(CONFIG_QUICC_ENGINE) += qe/
> obj-$(CONFIG_CPM) += qe/
> +obj-$(CONFIG_SUSPEND) += pm/
> diff --git a/drivers/soc/fsl/pm/Makefile b/drivers/soc/fsl/pm/Makefile
> new file mode 100644
> index 0000000..e275d63
> --- /dev/null
> +++ b/drivers/soc/fsl/pm/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_ARCH_LAYERSCAPE) += ls-rcpm.o
> diff --git a/drivers/soc/fsl/pm/ls-rcpm.c b/drivers/soc/fsl/pm/ls-rcpm.c
> new file mode 100644
> index 0000000..c5f2b97
> --- /dev/null
> +++ b/drivers/soc/fsl/pm/ls-rcpm.c
> @@ -0,0 +1,144 @@
> +/*
> + * Run Control and Power Management (RCPM) driver
> + *
> + * Copyright 2016 Freescale Semiconductor, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * 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.
> + *
> + */
> +#define pr_fmt(fmt) "RCPM: %s: " fmt, __func__
> +
> +#include <linux/kernel.h>
> +#include <linux/io.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_address.h>
> +#include <linux/suspend.h>
> +
> +/* So far there are not more than two registers */
> +#define RCPM_IPPDEXPCR0 0x140
> +#define RCPM_IPPDEXPCR1 0x144
> +#define RCPM_IPPDEXPCR(x) (RCPM_IPPDEXPCR0 + 4 * x)
> +#define RCPM_WAKEUP_CELL_MAX_SIZE 2
> +
> +/* it reprents the number of the registers RCPM_IPPDEXPCR */
> +static unsigned int rcpm_wakeup_cells;
> +static void __iomem *rcpm_reg_base;
> +static u32 ippdexpcr[RCPM_WAKEUP_CELL_MAX_SIZE];
> +
> +static inline void rcpm_reg_write(u32 offset, u32 value)
> +{
> + iowrite32be(value, rcpm_reg_base + offset);
> +}
> +
> +static inline u32 rcpm_reg_read(u32 offset)
> +{
> + return ioread32be(rcpm_reg_base + offset);
> +}
> +
> +static void rcpm_wakeup_fixup(struct device *dev, void *data)
> +{
> + struct device_node *node = dev ? dev->of_node : NULL;
> + u32 value[RCPM_WAKEUP_CELL_MAX_SIZE + 1];
> + int ret;
> + int i;
> +
> + if (!dev || !node || !device_may_wakeup(dev))
> + return;
> +
> + /*
> + * Get the values in the "rcpm-wakeup" property.
> + * Refer to Documentation/devicetree/bindings/soc/fsl/rcpm.txt
> + */
> + ret = of_property_read_u32_array(node, "rcpm-wakeup",
> + value, rcpm_wakeup_cells + 1);
> + if (ret)
> + return;
> +
> + pr_debug("wakeup source: the device %s\n", node->full_name);
This looks absolutely dreadful. You are parsing every node for each
device, and apply a set-in-stone configuration.
The normal way to handle this is by making this device an interrupt
controller, and honour the irq_set_wake API. This has already been done
on other FSL/NXP hardware (see the iMX GPC stuff).
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 21+ messages in thread* [PATCH 2/2] soc: nxp: Add a RCPM driver
@ 2016-08-01 13:22 ` Marc Zyngier
0 siblings, 0 replies; 21+ messages in thread
From: Marc Zyngier @ 2016-08-01 13:22 UTC (permalink / raw)
To: linux-arm-kernel
On 01/08/16 10:49, Chenhui Zhao wrote:
> The NXP's QorIQ Processors based on ARM Core have a RCPM module
> (Run Control and Power Management), which performs all device-level
> tasks associated with power management.
>
> This patch mainly implements the wakeup sources configuration before
> entering LPM20, a low power state of device-level. The devices can be
> waked up by specified sources, such as Flextimer, GPIO and so on.
>
> Signed-off-by: Chenhui Zhao <chenhui.zhao@nxp.com>
> ---
> drivers/soc/fsl/Makefile | 1 +
> drivers/soc/fsl/pm/Makefile | 1 +
> drivers/soc/fsl/pm/ls-rcpm.c | 144 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 146 insertions(+)
> create mode 100644 drivers/soc/fsl/pm/Makefile
> create mode 100644 drivers/soc/fsl/pm/ls-rcpm.c
>
> diff --git a/drivers/soc/fsl/Makefile b/drivers/soc/fsl/Makefile
> index 203307f..b5adbaf 100644
> --- a/drivers/soc/fsl/Makefile
> +++ b/drivers/soc/fsl/Makefile
> @@ -4,3 +4,4 @@
>
> obj-$(CONFIG_QUICC_ENGINE) += qe/
> obj-$(CONFIG_CPM) += qe/
> +obj-$(CONFIG_SUSPEND) += pm/
> diff --git a/drivers/soc/fsl/pm/Makefile b/drivers/soc/fsl/pm/Makefile
> new file mode 100644
> index 0000000..e275d63
> --- /dev/null
> +++ b/drivers/soc/fsl/pm/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_ARCH_LAYERSCAPE) += ls-rcpm.o
> diff --git a/drivers/soc/fsl/pm/ls-rcpm.c b/drivers/soc/fsl/pm/ls-rcpm.c
> new file mode 100644
> index 0000000..c5f2b97
> --- /dev/null
> +++ b/drivers/soc/fsl/pm/ls-rcpm.c
> @@ -0,0 +1,144 @@
> +/*
> + * Run Control and Power Management (RCPM) driver
> + *
> + * Copyright 2016 Freescale Semiconductor, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * 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.
> + *
> + */
> +#define pr_fmt(fmt) "RCPM: %s: " fmt, __func__
> +
> +#include <linux/kernel.h>
> +#include <linux/io.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_address.h>
> +#include <linux/suspend.h>
> +
> +/* So far there are not more than two registers */
> +#define RCPM_IPPDEXPCR0 0x140
> +#define RCPM_IPPDEXPCR1 0x144
> +#define RCPM_IPPDEXPCR(x) (RCPM_IPPDEXPCR0 + 4 * x)
> +#define RCPM_WAKEUP_CELL_MAX_SIZE 2
> +
> +/* it reprents the number of the registers RCPM_IPPDEXPCR */
> +static unsigned int rcpm_wakeup_cells;
> +static void __iomem *rcpm_reg_base;
> +static u32 ippdexpcr[RCPM_WAKEUP_CELL_MAX_SIZE];
> +
> +static inline void rcpm_reg_write(u32 offset, u32 value)
> +{
> + iowrite32be(value, rcpm_reg_base + offset);
> +}
> +
> +static inline u32 rcpm_reg_read(u32 offset)
> +{
> + return ioread32be(rcpm_reg_base + offset);
> +}
> +
> +static void rcpm_wakeup_fixup(struct device *dev, void *data)
> +{
> + struct device_node *node = dev ? dev->of_node : NULL;
> + u32 value[RCPM_WAKEUP_CELL_MAX_SIZE + 1];
> + int ret;
> + int i;
> +
> + if (!dev || !node || !device_may_wakeup(dev))
> + return;
> +
> + /*
> + * Get the values in the "rcpm-wakeup" property.
> + * Refer to Documentation/devicetree/bindings/soc/fsl/rcpm.txt
> + */
> + ret = of_property_read_u32_array(node, "rcpm-wakeup",
> + value, rcpm_wakeup_cells + 1);
> + if (ret)
> + return;
> +
> + pr_debug("wakeup source: the device %s\n", node->full_name);
This looks absolutely dreadful. You are parsing every node for each
device, and apply a set-in-stone configuration.
The normal way to handle this is by making this device an interrupt
controller, and honour the irq_set_wake API. This has already been done
on other FSL/NXP hardware (see the iMX GPC stuff).
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/2] soc: nxp: Add a RCPM driver
@ 2016-08-01 13:22 ` Marc Zyngier
0 siblings, 0 replies; 21+ messages in thread
From: Marc Zyngier @ 2016-08-01 13:22 UTC (permalink / raw)
To: Chenhui Zhao, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: z.chenhui-Re5JQEeQqe8AvxtiuMwx3w, jason.jin-3arQi8VN3Tc, Mark Rutland
On 01/08/16 10:49, Chenhui Zhao wrote:
> The NXP's QorIQ Processors based on ARM Core have a RCPM module
> (Run Control and Power Management), which performs all device-level
> tasks associated with power management.
>
> This patch mainly implements the wakeup sources configuration before
> entering LPM20, a low power state of device-level. The devices can be
> waked up by specified sources, such as Flextimer, GPIO and so on.
>
> Signed-off-by: Chenhui Zhao <chenhui.zhao-3arQi8VN3Tc@public.gmane.org>
> ---
> drivers/soc/fsl/Makefile | 1 +
> drivers/soc/fsl/pm/Makefile | 1 +
> drivers/soc/fsl/pm/ls-rcpm.c | 144 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 146 insertions(+)
> create mode 100644 drivers/soc/fsl/pm/Makefile
> create mode 100644 drivers/soc/fsl/pm/ls-rcpm.c
>
> diff --git a/drivers/soc/fsl/Makefile b/drivers/soc/fsl/Makefile
> index 203307f..b5adbaf 100644
> --- a/drivers/soc/fsl/Makefile
> +++ b/drivers/soc/fsl/Makefile
> @@ -4,3 +4,4 @@
>
> obj-$(CONFIG_QUICC_ENGINE) += qe/
> obj-$(CONFIG_CPM) += qe/
> +obj-$(CONFIG_SUSPEND) += pm/
> diff --git a/drivers/soc/fsl/pm/Makefile b/drivers/soc/fsl/pm/Makefile
> new file mode 100644
> index 0000000..e275d63
> --- /dev/null
> +++ b/drivers/soc/fsl/pm/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_ARCH_LAYERSCAPE) += ls-rcpm.o
> diff --git a/drivers/soc/fsl/pm/ls-rcpm.c b/drivers/soc/fsl/pm/ls-rcpm.c
> new file mode 100644
> index 0000000..c5f2b97
> --- /dev/null
> +++ b/drivers/soc/fsl/pm/ls-rcpm.c
> @@ -0,0 +1,144 @@
> +/*
> + * Run Control and Power Management (RCPM) driver
> + *
> + * Copyright 2016 Freescale Semiconductor, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * 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.
> + *
> + */
> +#define pr_fmt(fmt) "RCPM: %s: " fmt, __func__
> +
> +#include <linux/kernel.h>
> +#include <linux/io.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_address.h>
> +#include <linux/suspend.h>
> +
> +/* So far there are not more than two registers */
> +#define RCPM_IPPDEXPCR0 0x140
> +#define RCPM_IPPDEXPCR1 0x144
> +#define RCPM_IPPDEXPCR(x) (RCPM_IPPDEXPCR0 + 4 * x)
> +#define RCPM_WAKEUP_CELL_MAX_SIZE 2
> +
> +/* it reprents the number of the registers RCPM_IPPDEXPCR */
> +static unsigned int rcpm_wakeup_cells;
> +static void __iomem *rcpm_reg_base;
> +static u32 ippdexpcr[RCPM_WAKEUP_CELL_MAX_SIZE];
> +
> +static inline void rcpm_reg_write(u32 offset, u32 value)
> +{
> + iowrite32be(value, rcpm_reg_base + offset);
> +}
> +
> +static inline u32 rcpm_reg_read(u32 offset)
> +{
> + return ioread32be(rcpm_reg_base + offset);
> +}
> +
> +static void rcpm_wakeup_fixup(struct device *dev, void *data)
> +{
> + struct device_node *node = dev ? dev->of_node : NULL;
> + u32 value[RCPM_WAKEUP_CELL_MAX_SIZE + 1];
> + int ret;
> + int i;
> +
> + if (!dev || !node || !device_may_wakeup(dev))
> + return;
> +
> + /*
> + * Get the values in the "rcpm-wakeup" property.
> + * Refer to Documentation/devicetree/bindings/soc/fsl/rcpm.txt
> + */
> + ret = of_property_read_u32_array(node, "rcpm-wakeup",
> + value, rcpm_wakeup_cells + 1);
> + if (ret)
> + return;
> +
> + pr_debug("wakeup source: the device %s\n", node->full_name);
This looks absolutely dreadful. You are parsing every node for each
device, and apply a set-in-stone configuration.
The normal way to handle this is by making this device an interrupt
controller, and honour the irq_set_wake API. This has already been done
on other FSL/NXP hardware (see the iMX GPC stuff).
Thanks,
M.
--
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/2] soc: nxp: Add a RCPM driver
2016-08-01 13:22 ` Marc Zyngier
(?)
@ 2016-08-03 3:28 ` Chenhui Zhao
-1 siblings, 0 replies; 21+ messages in thread
From: Chenhui Zhao @ 2016-08-03 3:28 UTC (permalink / raw)
To: Marc Zyngier
Cc: Chenhui Zhao, devicetree, linux-arm-kernel, linux-kernel,
jason.jin, Mark Rutland
On Mon, Aug 1, 2016 at 9:22 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>
> On 01/08/16 10:49, Chenhui Zhao wrote:
> > The NXP's QorIQ Processors based on ARM Core have a RCPM module
> > (Run Control and Power Management), which performs all device-level
> > tasks associated with power management.
> >
> > This patch mainly implements the wakeup sources configuration before
> > entering LPM20, a low power state of device-level. The devices can be
> > waked up by specified sources, such as Flextimer, GPIO and so on.
> >
> > Signed-off-by: Chenhui Zhao <chenhui.zhao@nxp.com>
> > ---
> > drivers/soc/fsl/Makefile | 1 +
> > drivers/soc/fsl/pm/Makefile | 1 +
> > drivers/soc/fsl/pm/ls-rcpm.c | 144 +++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 146 insertions(+)
> > create mode 100644 drivers/soc/fsl/pm/Makefile
> > create mode 100644 drivers/soc/fsl/pm/ls-rcpm.c
> >
> > diff --git a/drivers/soc/fsl/Makefile b/drivers/soc/fsl/Makefile
> > index 203307f..b5adbaf 100644
> > --- a/drivers/soc/fsl/Makefile
> > +++ b/drivers/soc/fsl/Makefile
> > @@ -4,3 +4,4 @@
> >
> > obj-$(CONFIG_QUICC_ENGINE) += qe/
> > obj-$(CONFIG_CPM) += qe/
> > +obj-$(CONFIG_SUSPEND) += pm/
> > diff --git a/drivers/soc/fsl/pm/Makefile b/drivers/soc/fsl/pm/Makefile
> > new file mode 100644
> > index 0000000..e275d63
> > --- /dev/null
> > +++ b/drivers/soc/fsl/pm/Makefile
> > @@ -0,0 +1 @@
> > +obj-$(CONFIG_ARCH_LAYERSCAPE) += ls-rcpm.o
> > diff --git a/drivers/soc/fsl/pm/ls-rcpm.c b/drivers/soc/fsl/pm/ls-rcpm.c
> > new file mode 100644
> > index 0000000..c5f2b97
> > --- /dev/null
> > +++ b/drivers/soc/fsl/pm/ls-rcpm.c
> > @@ -0,0 +1,144 @@
> > +/*
> > + * Run Control and Power Management (RCPM) driver
> > + *
> > + * Copyright 2016 Freescale Semiconductor, Inc.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * 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.
> > + *
> > + */
> > +#define pr_fmt(fmt) "RCPM: %s: " fmt, __func__
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/io.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/of_address.h>
> > +#include <linux/suspend.h>
> > +
> > +/* So far there are not more than two registers */
> > +#define RCPM_IPPDEXPCR0 0x140
> > +#define RCPM_IPPDEXPCR1 0x144
> > +#define RCPM_IPPDEXPCR(x) (RCPM_IPPDEXPCR0 + 4 * x)
> > +#define RCPM_WAKEUP_CELL_MAX_SIZE 2
> > +
> > +/* it reprents the number of the registers RCPM_IPPDEXPCR */
> > +static unsigned int rcpm_wakeup_cells;
> > +static void __iomem *rcpm_reg_base;
> > +static u32 ippdexpcr[RCPM_WAKEUP_CELL_MAX_SIZE];
> > +
> > +static inline void rcpm_reg_write(u32 offset, u32 value)
> > +{
> > + iowrite32be(value, rcpm_reg_base + offset);
> > +}
> > +
> > +static inline u32 rcpm_reg_read(u32 offset)
> > +{
> > + return ioread32be(rcpm_reg_base + offset);
> > +}
> > +
> > +static void rcpm_wakeup_fixup(struct device *dev, void *data)
> > +{
> > + struct device_node *node = dev ? dev->of_node : NULL;
> > + u32 value[RCPM_WAKEUP_CELL_MAX_SIZE + 1];
> > + int ret;
> > + int i;
> > +
> > + if (!dev || !node || !device_may_wakeup(dev))
> > + return;
> > +
> > + /*
> > + * Get the values in the "rcpm-wakeup" property.
> > + * Refer to Documentation/devicetree/bindings/soc/fsl/rcpm.txt
> > + */
> > + ret = of_property_read_u32_array(node, "rcpm-wakeup",
> > + value, rcpm_wakeup_cells + 1);
> > + if (ret)
> > + return;
> > +
> > + pr_debug("wakeup source: the device %s\n", node->full_name);
>
> This looks absolutely dreadful. You are parsing every node for each
> device, and apply a set-in-stone configuration.
>
> The normal way to handle this is by making this device an interrupt
> controller, and honour the irq_set_wake API. This has already been done
> on other FSL/NXP hardware (see the iMX GPC stuff).
>
> Thanks,
>
> M.
The RCPM IP block is really not an interrupt controller, instead it is
related to power management. The code in this patch is to set the bits
in the IPPDEXPCR register. Setting these bits can make the
corresponding IP block alive during the period of sleep, so that the
IP blocks working as wakeup sources can wake the device. The
"rcpm-wakeup" property records the bit mask which should be set when
the IP block is working as wakeup source. The bit definition may be
different for each QorIQ SoC. The operation is specific to QorIQ
platform, so put the code in drivers/soc/fsl/pm/.
Thanks,
Chenhui
^ permalink raw reply [flat|nested] 21+ messages in thread* [PATCH 2/2] soc: nxp: Add a RCPM driver
@ 2016-08-03 3:28 ` Chenhui Zhao
0 siblings, 0 replies; 21+ messages in thread
From: Chenhui Zhao @ 2016-08-03 3:28 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Aug 1, 2016 at 9:22 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>
> On 01/08/16 10:49, Chenhui Zhao wrote:
> > The NXP's QorIQ Processors based on ARM Core have a RCPM module
> > (Run Control and Power Management), which performs all device-level
> > tasks associated with power management.
> >
> > This patch mainly implements the wakeup sources configuration before
> > entering LPM20, a low power state of device-level. The devices can be
> > waked up by specified sources, such as Flextimer, GPIO and so on.
> >
> > Signed-off-by: Chenhui Zhao <chenhui.zhao@nxp.com>
> > ---
> > drivers/soc/fsl/Makefile | 1 +
> > drivers/soc/fsl/pm/Makefile | 1 +
> > drivers/soc/fsl/pm/ls-rcpm.c | 144 +++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 146 insertions(+)
> > create mode 100644 drivers/soc/fsl/pm/Makefile
> > create mode 100644 drivers/soc/fsl/pm/ls-rcpm.c
> >
> > diff --git a/drivers/soc/fsl/Makefile b/drivers/soc/fsl/Makefile
> > index 203307f..b5adbaf 100644
> > --- a/drivers/soc/fsl/Makefile
> > +++ b/drivers/soc/fsl/Makefile
> > @@ -4,3 +4,4 @@
> >
> > obj-$(CONFIG_QUICC_ENGINE) += qe/
> > obj-$(CONFIG_CPM) += qe/
> > +obj-$(CONFIG_SUSPEND) += pm/
> > diff --git a/drivers/soc/fsl/pm/Makefile b/drivers/soc/fsl/pm/Makefile
> > new file mode 100644
> > index 0000000..e275d63
> > --- /dev/null
> > +++ b/drivers/soc/fsl/pm/Makefile
> > @@ -0,0 +1 @@
> > +obj-$(CONFIG_ARCH_LAYERSCAPE) += ls-rcpm.o
> > diff --git a/drivers/soc/fsl/pm/ls-rcpm.c b/drivers/soc/fsl/pm/ls-rcpm.c
> > new file mode 100644
> > index 0000000..c5f2b97
> > --- /dev/null
> > +++ b/drivers/soc/fsl/pm/ls-rcpm.c
> > @@ -0,0 +1,144 @@
> > +/*
> > + * Run Control and Power Management (RCPM) driver
> > + *
> > + * Copyright 2016 Freescale Semiconductor, Inc.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * 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.
> > + *
> > + */
> > +#define pr_fmt(fmt) "RCPM: %s: " fmt, __func__
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/io.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/of_address.h>
> > +#include <linux/suspend.h>
> > +
> > +/* So far there are not more than two registers */
> > +#define RCPM_IPPDEXPCR0 0x140
> > +#define RCPM_IPPDEXPCR1 0x144
> > +#define RCPM_IPPDEXPCR(x) (RCPM_IPPDEXPCR0 + 4 * x)
> > +#define RCPM_WAKEUP_CELL_MAX_SIZE 2
> > +
> > +/* it reprents the number of the registers RCPM_IPPDEXPCR */
> > +static unsigned int rcpm_wakeup_cells;
> > +static void __iomem *rcpm_reg_base;
> > +static u32 ippdexpcr[RCPM_WAKEUP_CELL_MAX_SIZE];
> > +
> > +static inline void rcpm_reg_write(u32 offset, u32 value)
> > +{
> > + iowrite32be(value, rcpm_reg_base + offset);
> > +}
> > +
> > +static inline u32 rcpm_reg_read(u32 offset)
> > +{
> > + return ioread32be(rcpm_reg_base + offset);
> > +}
> > +
> > +static void rcpm_wakeup_fixup(struct device *dev, void *data)
> > +{
> > + struct device_node *node = dev ? dev->of_node : NULL;
> > + u32 value[RCPM_WAKEUP_CELL_MAX_SIZE + 1];
> > + int ret;
> > + int i;
> > +
> > + if (!dev || !node || !device_may_wakeup(dev))
> > + return;
> > +
> > + /*
> > + * Get the values in the "rcpm-wakeup" property.
> > + * Refer to Documentation/devicetree/bindings/soc/fsl/rcpm.txt
> > + */
> > + ret = of_property_read_u32_array(node, "rcpm-wakeup",
> > + value, rcpm_wakeup_cells + 1);
> > + if (ret)
> > + return;
> > +
> > + pr_debug("wakeup source: the device %s\n", node->full_name);
>
> This looks absolutely dreadful. You are parsing every node for each
> device, and apply a set-in-stone configuration.
>
> The normal way to handle this is by making this device an interrupt
> controller, and honour the irq_set_wake API. This has already been done
> on other FSL/NXP hardware (see the iMX GPC stuff).
>
> Thanks,
>
> M.
The RCPM IP block is really not an interrupt controller, instead it is
related to power management. The code in this patch is to set the bits
in the IPPDEXPCR register. Setting these bits can make the
corresponding IP block alive during the period of sleep, so that the
IP blocks working as wakeup sources can wake the device. The
"rcpm-wakeup" property records the bit mask which should be set when
the IP block is working as wakeup source. The bit definition may be
different for each QorIQ SoC. The operation is specific to QorIQ
platform, so put the code in drivers/soc/fsl/pm/.
Thanks,
Chenhui
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/2] soc: nxp: Add a RCPM driver
@ 2016-08-03 3:28 ` Chenhui Zhao
0 siblings, 0 replies; 21+ messages in thread
From: Chenhui Zhao @ 2016-08-03 3:28 UTC (permalink / raw)
To: Marc Zyngier
Cc: Mark Rutland, devicetree, Chenhui Zhao, jason.jin, linux-kernel,
linux-arm-kernel
On Mon, Aug 1, 2016 at 9:22 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>
> On 01/08/16 10:49, Chenhui Zhao wrote:
> > The NXP's QorIQ Processors based on ARM Core have a RCPM module
> > (Run Control and Power Management), which performs all device-level
> > tasks associated with power management.
> >
> > This patch mainly implements the wakeup sources configuration before
> > entering LPM20, a low power state of device-level. The devices can be
> > waked up by specified sources, such as Flextimer, GPIO and so on.
> >
> > Signed-off-by: Chenhui Zhao <chenhui.zhao@nxp.com>
> > ---
> > drivers/soc/fsl/Makefile | 1 +
> > drivers/soc/fsl/pm/Makefile | 1 +
> > drivers/soc/fsl/pm/ls-rcpm.c | 144 +++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 146 insertions(+)
> > create mode 100644 drivers/soc/fsl/pm/Makefile
> > create mode 100644 drivers/soc/fsl/pm/ls-rcpm.c
> >
> > diff --git a/drivers/soc/fsl/Makefile b/drivers/soc/fsl/Makefile
> > index 203307f..b5adbaf 100644
> > --- a/drivers/soc/fsl/Makefile
> > +++ b/drivers/soc/fsl/Makefile
> > @@ -4,3 +4,4 @@
> >
> > obj-$(CONFIG_QUICC_ENGINE) += qe/
> > obj-$(CONFIG_CPM) += qe/
> > +obj-$(CONFIG_SUSPEND) += pm/
> > diff --git a/drivers/soc/fsl/pm/Makefile b/drivers/soc/fsl/pm/Makefile
> > new file mode 100644
> > index 0000000..e275d63
> > --- /dev/null
> > +++ b/drivers/soc/fsl/pm/Makefile
> > @@ -0,0 +1 @@
> > +obj-$(CONFIG_ARCH_LAYERSCAPE) += ls-rcpm.o
> > diff --git a/drivers/soc/fsl/pm/ls-rcpm.c b/drivers/soc/fsl/pm/ls-rcpm.c
> > new file mode 100644
> > index 0000000..c5f2b97
> > --- /dev/null
> > +++ b/drivers/soc/fsl/pm/ls-rcpm.c
> > @@ -0,0 +1,144 @@
> > +/*
> > + * Run Control and Power Management (RCPM) driver
> > + *
> > + * Copyright 2016 Freescale Semiconductor, Inc.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * 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.
> > + *
> > + */
> > +#define pr_fmt(fmt) "RCPM: %s: " fmt, __func__
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/io.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/of_address.h>
> > +#include <linux/suspend.h>
> > +
> > +/* So far there are not more than two registers */
> > +#define RCPM_IPPDEXPCR0 0x140
> > +#define RCPM_IPPDEXPCR1 0x144
> > +#define RCPM_IPPDEXPCR(x) (RCPM_IPPDEXPCR0 + 4 * x)
> > +#define RCPM_WAKEUP_CELL_MAX_SIZE 2
> > +
> > +/* it reprents the number of the registers RCPM_IPPDEXPCR */
> > +static unsigned int rcpm_wakeup_cells;
> > +static void __iomem *rcpm_reg_base;
> > +static u32 ippdexpcr[RCPM_WAKEUP_CELL_MAX_SIZE];
> > +
> > +static inline void rcpm_reg_write(u32 offset, u32 value)
> > +{
> > + iowrite32be(value, rcpm_reg_base + offset);
> > +}
> > +
> > +static inline u32 rcpm_reg_read(u32 offset)
> > +{
> > + return ioread32be(rcpm_reg_base + offset);
> > +}
> > +
> > +static void rcpm_wakeup_fixup(struct device *dev, void *data)
> > +{
> > + struct device_node *node = dev ? dev->of_node : NULL;
> > + u32 value[RCPM_WAKEUP_CELL_MAX_SIZE + 1];
> > + int ret;
> > + int i;
> > +
> > + if (!dev || !node || !device_may_wakeup(dev))
> > + return;
> > +
> > + /*
> > + * Get the values in the "rcpm-wakeup" property.
> > + * Refer to Documentation/devicetree/bindings/soc/fsl/rcpm.txt
> > + */
> > + ret = of_property_read_u32_array(node, "rcpm-wakeup",
> > + value, rcpm_wakeup_cells + 1);
> > + if (ret)
> > + return;
> > +
> > + pr_debug("wakeup source: the device %s\n", node->full_name);
>
> This looks absolutely dreadful. You are parsing every node for each
> device, and apply a set-in-stone configuration.
>
> The normal way to handle this is by making this device an interrupt
> controller, and honour the irq_set_wake API. This has already been done
> on other FSL/NXP hardware (see the iMX GPC stuff).
>
> Thanks,
>
> M.
The RCPM IP block is really not an interrupt controller, instead it is
related to power management. The code in this patch is to set the bits
in the IPPDEXPCR register. Setting these bits can make the
corresponding IP block alive during the period of sleep, so that the
IP blocks working as wakeup sources can wake the device. The
"rcpm-wakeup" property records the bit mask which should be set when
the IP block is working as wakeup source. The bit definition may be
different for each QorIQ SoC. The operation is specific to QorIQ
platform, so put the code in drivers/soc/fsl/pm/.
Thanks,
Chenhui
^ permalink raw reply [flat|nested] 21+ messages in thread