Re: [PATCH V2 3/3] soc: fsl: add RCPM driver
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 to that version. Best regards, Pavel -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany signature.asc Description: Digital signature
RE: [PATCH V2 3/3] soc: fsl: add RCPM driver
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
Re: [PATCH V2 3/3] soc: fsl: add RCPM driver
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 -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany signature.asc Description: Digital signature
RE: [PATCH V2 3/3] soc: fsl: add RCPM driver
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
Re: [PATCH V2 3/3] soc: fsl: add RCPM driver
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 -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany signature.asc Description: Digital signature
RE: [PATCH V2 3/3] soc: fsl: add RCPM driver
Hi Pavel, On Monday, May 20, 2019 05:39, Pavel Machek wrote: > > Hi! > > > > + > > +struct rcpm { > > + unsigned int wakeup_cells; > > + void __iomem *ippdexpcr_base; > > + boollittle_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 = >dev; > > + struct resource *r; > > + struct rcpm *rcpm; > > + int ret; > > Whitespace. OK, will update, thanks for your review. Regards, Ran
Re: [PATCH V2 3/3] soc: fsl: add RCPM driver
Hi! > + > +struct rcpm { > + unsigned int wakeup_cells; > + void __iomem *ippdexpcr_base; > + boollittle_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 = >dev; > + struct resource *r; > + struct rcpm *rcpm; > + int ret; Whitespace. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
[PATCH V2 3/3] soc: fsl: add 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 --- 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 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 000..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 , + +#include +#include +#include +#include +#include +#include +#include + +#define RCPM_WAKEUP_CELL_MAX_SIZE 7 + +struct rcpm { + unsigned int wakeup_cells; + void __iomem *ippdexpcr_base; + boollittle_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 = >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(>dev, r); + if (IS_ERR(rcpm->ippdexpcr_base)) { + ret = PTR_ERR(rcpm->ippdexpcr_base); + return ret; + } + + rcpm->little_endian = device_property_read_bool( + >dev, "little-endian"); + + ret =