| Message ID | 20190517033946.30763-3-ran.wang_1@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2,1/3] PM: wakeup: Add routine to help fetch wakeup source object. | expand |
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
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
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
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
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
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
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
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);
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