[V2,3/3] soc: fsl: add RCPM driver
diff mbox series

Message ID 20190517033946.30763-3-ran.wang_1@nxp.com
State New
Headers show
Series
  • [V2,1/3] PM: wakeup: Add routine to help fetch wakeup source object.
Related show

Commit Message

Ran Wang May 17, 2019, 3:39 a.m. UTC
The NXP's QorIQ Processors based on ARM Core have RCPM module
(Run Control and Power Management), which performs all device-level
tasks associated with power management such as wakeup source control.

This driver depends on PM wakeup source framework which help to
collect wake information.

Signed-off-by: Ran Wang <ran.wang_1@nxp.com>
---
Change in v2:
	- Rebase Kconfig and Makefile update to latest mainline.

 drivers/soc/fsl/Kconfig  |    8 +++
 drivers/soc/fsl/Makefile |    1 +
 drivers/soc/fsl/rcpm.c   |  124 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 133 insertions(+), 0 deletions(-)
 create mode 100644 drivers/soc/fsl/rcpm.c

Comments

Pavel Machek May 19, 2019, 9:38 p.m. UTC | #1
Hi!


> +
> +struct rcpm {
> +	unsigned int wakeup_cells;
> +	void __iomem *ippdexpcr_base;
> +	bool	little_endian;
> +};

Inconsistent whitespace


> +static int rcpm_pm_prepare(struct device *dev)
> +{
> +	struct device_node *np = dev->of_node;
> +	struct wakeup_source *ws;
> +	struct rcpm *rcpm;
> +	u32 value[RCPM_WAKEUP_CELL_MAX_SIZE + 1], tmp;
> +	int i, ret;
> +
> +	rcpm = dev_get_drvdata(dev);
> +	if (!rcpm)
> +		return -EINVAL;
> +
> +	/* Begin with first registered wakeup source */
> +	ws = wakeup_source_get_next(NULL);
> +	while (ws) {

while (ws = wakeup_source_get_next(NULL)) ?


> +static int rcpm_probe(struct platform_device *pdev)
> +{
> +	struct device	*dev = &pdev->dev;
> +	struct resource *r;
> +	struct rcpm		*rcpm;
> +	int ret;

Whitespace.

								Pavel
Ran Wang May 20, 2019, 6:48 a.m. UTC | #2
Hi Pavel,

On Monday, May 20, 2019 05:39, Pavel Machek wrote:
> 
> Hi!
> 
> 
> > +
> > +struct rcpm {
> > +	unsigned int wakeup_cells;
> > +	void __iomem *ippdexpcr_base;
> > +	bool	little_endian;
> > +};
> 
> Inconsistent whitespace

OK, will make them aligned.

> 
> > +static int rcpm_pm_prepare(struct device *dev) {
> > +	struct device_node *np = dev->of_node;
> > +	struct wakeup_source *ws;
> > +	struct rcpm *rcpm;
> > +	u32 value[RCPM_WAKEUP_CELL_MAX_SIZE + 1], tmp;
> > +	int i, ret;
> > +
> > +	rcpm = dev_get_drvdata(dev);
> > +	if (!rcpm)
> > +		return -EINVAL;
> > +
> > +	/* Begin with first registered wakeup source */
> > +	ws = wakeup_source_get_next(NULL);
> > +	while (ws) {
> 
> while (ws = wakeup_source_get_next(NULL)) ?

Actually, we only pass NULL to wakeup_source_get_next() at very first
call to get 1st wakeup source. Then in the while loop, we will fetch
next source but not 1st, that's different. I am afraid your suggestion
is not quite correct.

> 
> > +static int rcpm_probe(struct platform_device *pdev) {
> > +	struct device	*dev = &pdev->dev;
> > +	struct resource *r;
> > +	struct rcpm		*rcpm;
> > +	int ret;
> 
> Whitespace.

OK, will update, thanks for your review.

Regards,
Ran
Pavel Machek May 20, 2019, 8:56 a.m. UTC | #3
Hi!

> > > +static int rcpm_pm_prepare(struct device *dev) {
> > > +	struct device_node *np = dev->of_node;
> > > +	struct wakeup_source *ws;
> > > +	struct rcpm *rcpm;
> > > +	u32 value[RCPM_WAKEUP_CELL_MAX_SIZE + 1], tmp;
> > > +	int i, ret;
> > > +
> > > +	rcpm = dev_get_drvdata(dev);
> > > +	if (!rcpm)
> > > +		return -EINVAL;
> > > +
> > > +	/* Begin with first registered wakeup source */
> > > +	ws = wakeup_source_get_next(NULL);
> > > +	while (ws) {
> > 
> > while (ws = wakeup_source_get_next(NULL)) ?
> 
> Actually, we only pass NULL to wakeup_source_get_next() at very first
> call to get 1st wakeup source. Then in the while loop, we will fetch
> next source but not 1st, that's different. I am afraid your suggestion
> is not quite correct.

Sorry, I seen your next version before seeing this explanation.

You are right, but the current code is "interesting". What about

    ws = NULL;
    while (ws = wakeup_source_get_next(NULL)) ...

then?

Best regards,
								Pavel
Ran Wang May 20, 2019, 9:03 a.m. UTC | #4
Hi Pavel,

On Monday, May 20, 2019 16:57, Pavel Machek wrote:
> 
> Hi!
> 
> > > > +static int rcpm_pm_prepare(struct device *dev) {
> > > > +	struct device_node *np = dev->of_node;
> > > > +	struct wakeup_source *ws;
> > > > +	struct rcpm *rcpm;
> > > > +	u32 value[RCPM_WAKEUP_CELL_MAX_SIZE + 1], tmp;
> > > > +	int i, ret;
> > > > +
> > > > +	rcpm = dev_get_drvdata(dev);
> > > > +	if (!rcpm)
> > > > +		return -EINVAL;
> > > > +
> > > > +	/* Begin with first registered wakeup source */
> > > > +	ws = wakeup_source_get_next(NULL);
> > > > +	while (ws) {
> > >
> > > while (ws = wakeup_source_get_next(NULL)) ?
> >
> > Actually, we only pass NULL to wakeup_source_get_next() at very first
> > call to get 1st wakeup source. Then in the while loop, we will fetch
> > next source but not 1st, that's different. I am afraid your suggestion
> > is not quite correct.
> 
> Sorry, I seen your next version before seeing this explanation.
> 
> You are right, but the current code is "interesting". What about
> 
>     ws = NULL;
>     while (ws = wakeup_source_get_next(NULL)) ...
> 
> then?

Did you mean:
     ws = NULL;
     while (ws = wakeup_source_get_next(ws)) ...

   Yes, that will be the same to my original logic, do you recommend to change
to this? :)

Regards,
Ran
Pavel Machek May 20, 2019, 9:07 a.m. UTC | #5
On Mon 2019-05-20 09:03:50, Ran Wang wrote:
> Hi Pavel,
> 
> On Monday, May 20, 2019 16:57, Pavel Machek wrote:
> > 
> > Hi!
> > 
> > > > > +static int rcpm_pm_prepare(struct device *dev) {
> > > > > +	struct device_node *np = dev->of_node;
> > > > > +	struct wakeup_source *ws;
> > > > > +	struct rcpm *rcpm;
> > > > > +	u32 value[RCPM_WAKEUP_CELL_MAX_SIZE + 1], tmp;
> > > > > +	int i, ret;
> > > > > +
> > > > > +	rcpm = dev_get_drvdata(dev);
> > > > > +	if (!rcpm)
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	/* Begin with first registered wakeup source */
> > > > > +	ws = wakeup_source_get_next(NULL);
> > > > > +	while (ws) {
> > > >
> > > > while (ws = wakeup_source_get_next(NULL)) ?
> > >
> > > Actually, we only pass NULL to wakeup_source_get_next() at very first
> > > call to get 1st wakeup source. Then in the while loop, we will fetch
> > > next source but not 1st, that's different. I am afraid your suggestion
> > > is not quite correct.
> > 
> > Sorry, I seen your next version before seeing this explanation.
> > 
> > You are right, but the current code is "interesting". What about
> > 
> >     ws = NULL;
> >     while (ws = wakeup_source_get_next(NULL)) ...
> > 
> > then?
> 
> Did you mean:
>      ws = NULL;
>      while (ws = wakeup_source_get_next(ws)) ...
> 
>    Yes, that will be the same to my original logic, do you recommend to change
> to this? :)

Yes please. It will be less confusing to the reader.

Thanks (and sorry for cross-talk),
								Pavel
Ran Wang May 20, 2019, 9:17 a.m. UTC | #6
Hi Pavel,

On Monday, May 20, 2019 17:08 Pavel Machek wrote:
> > > Hi!
> > >
> > > > > > +static int rcpm_pm_prepare(struct device *dev) {
> > > > > > +	struct device_node *np = dev->of_node;
> > > > > > +	struct wakeup_source *ws;
> > > > > > +	struct rcpm *rcpm;
> > > > > > +	u32 value[RCPM_WAKEUP_CELL_MAX_SIZE + 1], tmp;
> > > > > > +	int i, ret;
> > > > > > +
> > > > > > +	rcpm = dev_get_drvdata(dev);
> > > > > > +	if (!rcpm)
> > > > > > +		return -EINVAL;
> > > > > > +
> > > > > > +	/* Begin with first registered wakeup source */
> > > > > > +	ws = wakeup_source_get_next(NULL);
> > > > > > +	while (ws) {
> > > > >
> > > > > while (ws = wakeup_source_get_next(NULL)) ?
> > > >
> > > > Actually, we only pass NULL to wakeup_source_get_next() at very
> > > > first call to get 1st wakeup source. Then in the while loop, we
> > > > will fetch next source but not 1st, that's different. I am afraid
> > > > your suggestion is not quite correct.
> > >
> > > Sorry, I seen your next version before seeing this explanation.
> > >
> > > You are right, but the current code is "interesting". What about
> > >
> > >     ws = NULL;
> > >     while (ws = wakeup_source_get_next(NULL)) ...
> > >
> > > then?
> >
> > Did you mean:
> >      ws = NULL;
> >      while (ws = wakeup_source_get_next(ws)) ...
> >
> >    Yes, that will be the same to my original logic, do you recommend
> > to change to this? :)
> 
> Yes please. It will be less confusing to the reader.

OK, if no other comment, I will work out v4, fix this and extra ','
 
> Thanks (and sorry for cross-talk),

That's OK, thanks for your time.

Regards,
Ran
Pavel Machek May 20, 2019, 9:24 a.m. UTC | #7
Hi!

> > > > You are right, but the current code is "interesting". What about
> > > >
> > > >     ws = NULL;
> > > >     while (ws = wakeup_source_get_next(NULL)) ...
> > > >
> > > > then?
> > >
> > > Did you mean:
> > >      ws = NULL;
> > >      while (ws = wakeup_source_get_next(ws)) ...
> > >
> > >    Yes, that will be the same to my original logic, do you recommend
> > > to change to this? :)
> > 
> > Yes please. It will be less confusing to the reader.
> 
> OK, if no other comment, I will work out v4, fix this and extra ','
>  
> > Thanks (and sorry for cross-talk),
> 
> That's OK, thanks for your time.

You can add

Acked-by: Pavel Machek <pavel@ucw.cz>

to that version.

Best regards,
								Pavel

Patch
diff mbox series

diff --git a/drivers/soc/fsl/Kconfig b/drivers/soc/fsl/Kconfig
index 61f8e14..8e84e40 100644
--- a/drivers/soc/fsl/Kconfig
+++ b/drivers/soc/fsl/Kconfig
@@ -29,4 +29,12 @@  config FSL_MC_DPIO
 	  other DPAA2 objects. This driver does not expose the DPIO
 	  objects individually, but groups them under a service layer
 	  API.
+
+config FSL_RCPM
+	bool "Freescale RCPM support"
+	depends on PM_SLEEP
+	help
+	  The NXP's QorIQ Processors based on ARM Core have RCPM module
+	  (Run Control and Power Management), which performs all device-level
+	  tasks associated with power management, such as wakeup source control.
 endmenu
diff --git a/drivers/soc/fsl/Makefile b/drivers/soc/fsl/Makefile
index 803ef1b..c1be6ee 100644
--- a/drivers/soc/fsl/Makefile
+++ b/drivers/soc/fsl/Makefile
@@ -7,3 +7,4 @@  obj-$(CONFIG_QUICC_ENGINE)		+= qe/
 obj-$(CONFIG_CPM)			+= qe/
 obj-$(CONFIG_FSL_GUTS)			+= guts.o
 obj-$(CONFIG_FSL_MC_DPIO) 		+= dpio/
+obj-$(CONFIG_FSL_RCPM)		+= rcpm.o
diff --git a/drivers/soc/fsl/rcpm.c b/drivers/soc/fsl/rcpm.c
new file mode 100644
index 0000000..b817319
--- /dev/null
+++ b/drivers/soc/fsl/rcpm.c
@@ -0,0 +1,124 @@ 
+// SPDX-License-Identifier: GPL-2.0
+//
+// rcpm.c - Freescale QorIQ RCPM driver
+//
+// Copyright 2019 NXP
+//
+// Author: Ran Wang <ran.wang_1@nxp.com>,
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/of_address.h>
+#include <linux/slab.h>
+#include <linux/suspend.h>
+#include <linux/kernel.h>
+
+#define RCPM_WAKEUP_CELL_MAX_SIZE	7
+
+struct rcpm {
+	unsigned int wakeup_cells;
+	void __iomem *ippdexpcr_base;
+	bool	little_endian;
+};
+
+static int rcpm_pm_prepare(struct device *dev)
+{
+	struct device_node *np = dev->of_node;
+	struct wakeup_source *ws;
+	struct rcpm *rcpm;
+	u32 value[RCPM_WAKEUP_CELL_MAX_SIZE + 1], tmp;
+	int i, ret;
+
+	rcpm = dev_get_drvdata(dev);
+	if (!rcpm)
+		return -EINVAL;
+
+	/* Begin with first registered wakeup source */
+	ws = wakeup_source_get_next(NULL);
+	while (ws) {
+		ret = device_property_read_u32_array(ws->attached_dev,
+				"fsl,rcpm-wakeup", value, rcpm->wakeup_cells + 1);
+
+		/*  Wakeup source should refer to current rcpm device */
+		if (ret || (np->phandle != value[0])) {
+			dev_info(dev, "%s doesn't refer to this rcpm\n",
+					ws->name);
+			ws = wakeup_source_get_next(ws);
+			continue;
+		}
+
+		for (i = 0; i < rcpm->wakeup_cells; i++) {
+			/* We can only OR related bits */
+			if (value[i + 1]) {
+				if (rcpm->little_endian) {
+					tmp = ioread32(rcpm->ippdexpcr_base + i * 4);
+					tmp |= value[i + 1];
+					iowrite32(tmp, rcpm->ippdexpcr_base + i * 4);
+				} else {
+					tmp = ioread32be(rcpm->ippdexpcr_base + i * 4);
+					tmp |= value[i + 1];
+					iowrite32be(tmp, rcpm->ippdexpcr_base + i * 4);
+				}
+			}
+		}
+		ws = wakeup_source_get_next(ws);
+	}
+
+	return 0;
+}
+
+static const struct dev_pm_ops rcpm_pm_ops = {
+	.prepare =  rcpm_pm_prepare,
+};
+
+static int rcpm_probe(struct platform_device *pdev)
+{
+	struct device	*dev = &pdev->dev;
+	struct resource *r;
+	struct rcpm		*rcpm;
+	int ret;
+
+	rcpm = devm_kzalloc(dev, sizeof(*rcpm), GFP_KERNEL);
+	if (!rcpm)
+		return -ENOMEM;
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!r)
+		return -ENODEV;
+
+	rcpm->ippdexpcr_base = devm_ioremap_resource(&pdev->dev, r);
+	if (IS_ERR(rcpm->ippdexpcr_base)) {
+		ret =  PTR_ERR(rcpm->ippdexpcr_base);
+		return ret;
+	}
+
+	rcpm->little_endian = device_property_read_bool(
+			&pdev->dev, "little-endian");
+
+	ret = device_property_read_u32(&pdev->dev,
+			"fsl,#rcpm-wakeup-cells", &rcpm->wakeup_cells);
+	if (ret)
+		return ret;
+
+	dev_set_drvdata(&pdev->dev, rcpm);
+
+	return 0;
+}
+
+static const struct of_device_id rcpm_of_match[] = {
+	{ .compatible = "fsl,qoriq-rcpm-2.1+", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, rcpm_of_match);
+
+static struct platform_driver rcpm_driver = {
+	.driver = {
+		.name = "rcpm",
+		.of_match_table = rcpm_of_match,
+		.pm	= &rcpm_pm_ops,
+	},
+	.probe = rcpm_probe,
+};
+
+module_platform_driver(rcpm_driver);