This driver is to provide a independent framework for PM service
provider and consumer to configure system level wake up feature. For
example, RCPM driver could register a callback function on this
platform first, and Flex timer driver who want to enable timer wake
up feature, will call generic API provided by this platform driver,
and then it will trigger RCPM driver to do it. The benefit is to
isolate the user and service, such as flex timer driver will not have
to know the implement details of wakeup function it require. Besides,
it is also easy for service side to upgrade its logic when design is
changed and remain user side unchanged.
Signed-off-by: Ran Wang <redacted>
---
drivers/soc/fsl/Kconfig | 14 +++++
drivers/soc/fsl/Makefile | 1 +
drivers/soc/fsl/plat_pm.c | 144 +++++++++++++++++++++++++++++++++++++++++++++
include/soc/fsl/plat_pm.h | 22 +++++++
4 files changed, 181 insertions(+), 0 deletions(-)
create mode 100644 drivers/soc/fsl/plat_pm.c
create mode 100644 include/soc/fsl/plat_pm.h
@@ -0,0 +1,144 @@+// SPDX-License-Identifier: GPL-2.0+//+// plat_pm.c - Freescale platform PM framework+//+// Copyright 2018 NXP+//+// Author: Ran Wang <ran.wang_1@nxp.com>,++#include<linux/kernel.h>+#include<linux/device.h>+#include<linux/list.h>+#include<linux/slab.h>+#include<linux/err.h>+#include<soc/fsl/plat_pm.h>+++structplat_pm_t{+structlist_headnode;+fsl_plat_pm_handlehandle;+void*handle_priv;+spinlock_tlock;+};++staticstructplat_pm_tplat_pm;++// register_fsl_platform_wakeup_source - Register callback function to plat_pm+// @handle: Pointer to handle PM feature requirement+// @handle_priv: Handler specific data struct+//+// Return 0 on success other negative errno+intregister_fsl_platform_wakeup_source(fsl_plat_pm_handlehandle,+void*handle_priv)+{+structplat_pm_t*p;+unsignedlongflags;++if(!handle){+pr_err("FSL plat_pm: Handler invalid, reject\n");+return-EINVAL;+}++p=kmalloc(sizeof(*p),GFP_KERNEL);+if(!p)+return-ENOMEM;++p->handle=handle;+p->handle_priv=handle_priv;++spin_lock_irqsave(&plat_pm.lock,flags);+list_add_tail(&p->node,&plat_pm.node);+spin_unlock_irqrestore(&plat_pm.lock,flags);++return0;+}+EXPORT_SYMBOL_GPL(register_fsl_platform_wakeup_source);++// Deregister_fsl_platform_wakeup_source - deregister callback function+// @handle_priv: Handler specific data struct+//+// Return 0 on success other negative errno+intderegister_fsl_platform_wakeup_source(void*handle_priv)+{+structplat_pm_t*p,*tmp;+unsignedlongflags;++spin_lock_irqsave(&plat_pm.lock,flags);+list_for_each_entry_safe(p,tmp,&plat_pm.node,node){+if(p->handle_priv==handle_priv){+list_del(&p->node);+kfree(p);+}+}+spin_unlock_irqrestore(&plat_pm.lock,flags);+return0;+}+EXPORT_SYMBOL_GPL(deregister_fsl_platform_wakeup_source);++// fsl_platform_wakeup_config - Configure wakeup source by calling handlers+// @dev: pointer to user's device struct+// @flag: to tell enable or disable wakeup source+//+// Return 0 on success other negative errno+intfsl_platform_wakeup_config(structdevice*dev,boolflag)+{+structplat_pm_t*p;+intret;+boolsuccess_handled;+unsignedlongflags;++success_handled=false;++// Will consider success if at least one callback return 0.+// Also, rest handles still get oppertunity to be executed+spin_lock_irqsave(&plat_pm.lock,flags);+list_for_each_entry(p,&plat_pm.node,node){+if(p->handle){+ret=p->handle(dev,flag,p->handle_priv);+if(!ret)+success_handled=true;+elseif(ret!=-ENODEV){+pr_err("FSL plat_pm: Failed to config wakeup source:%d\n",ret);+returnret;+}+}else+pr_warn("FSL plat_pm: Invalid handler detected, skip\n");+}+spin_unlock_irqrestore(&plat_pm.lock,flags);++if(success_handled==false){+pr_err("FSL plat_pm: Cannot find the matchhed handler for wakeup source config\n");+return-ENODEV;+}++return0;+}++// fsl_platform_wakeup_enable - Enable wakeup source+// @dev: pointer to user's device struct+//+// Return 0 on success other negative errno+intfsl_platform_wakeup_enable(structdevice*dev)+{+returnfsl_platform_wakeup_config(dev,true);+}+EXPORT_SYMBOL_GPL(fsl_platform_wakeup_enable);++// fsl_platform_wakeup_disable - Disable wakeup source+// @dev: pointer to user's device struct+//+// Return 0 on success other negative errno+intfsl_platform_wakeup_disable(structdevice*dev)+{+returnfsl_platform_wakeup_config(dev,false);+}+EXPORT_SYMBOL_GPL(fsl_platform_wakeup_disable);++staticint__initfsl_plat_pm_init(void)+{+spin_lock_init(&plat_pm.lock);+INIT_LIST_HEAD(&plat_pm.node);+return0;+}++core_initcall(fsl_plat_pm_init);
@@ -5,8 +5,6 @@ and power management. Required properites: - reg : Offset and length of the register set of the RCPM block.- - fsl,#rcpm-wakeup-cells : The number of IPPDEXPCR register cells in the- fsl,rcpm-wakeup property. - compatible : Must contain a chip-specific RCPM block compatible string and (if applicable) may contain a chassis-version RCPM compatible string. Chip-specific strings are of the form "fsl,<chip>-rcpm",
@@ -27,37 +25,23 @@ Chassis Version Example Chips --------------- ------------------------------- 1.0 p4080, p5020, p5040, p2041, p3041 2.0 t4240, b4860, b4420-2.1 t1040, ls1021+2.1 t1040, ls1021, ls1012++Optional properties:+ - big-endian : Indicate RCPM registers is big-endian. A RCPM node+ that doesn't have this property will be regarded as little-endian.+ - <property 'compatible' string of consumer device> : This string+ is referred by RCPM driver to judge if the consumer (such as flex timer)+ is able to be regards as wakeup source or not, such as 'fsl,ls1012a-ftm'.+ Further, this property will carry the bit mask info to control+ coresponding wake up source. Example: The RCPM node for T4240: rcpm: global-utilities@e2000 { compatible = "fsl,t4240-rcpm", "fsl,qoriq-rcpm-2.0"; reg = <0xe2000 0x1000>;- fsl,#rcpm-wakeup-cells = <2>;- };--* Freescale RCPM Wakeup Source Device Tree Bindings---------------------------------------------Required fsl,rcpm-wakeup property should be added to a device node if the device-can be used as a wakeup source.-- - fsl,rcpm-wakeup: Consists of a phandle to the rcpm node and the IPPDEXPCR- register cells. The number of IPPDEXPCR register cells is defined in- "fsl,#rcpm-wakeup-cells" in the rcpm node. The first register cell is- the bit mask that should be set in IPPDEXPCR0, and the second register- cell is for IPPDEXPCR1, and so on.-- Note: IPPDEXPCR(IP Powerdown Exception Control Register) provides a- mechanism for keeping certain blocks awake during STANDBY and MEM, in- order to use them as wake-up sources.--Example:- lpuart0: serial@2950000 {- compatible = "fsl,ls1021a-lpuart";- reg = <0x0 0x2950000 0x0 0x1000>;- interrupts = <GIC_SPI 80 IRQ_TYPE_LEVEL_HIGH>;- clocks = <&sysclk>;- clock-names = "ipg";- fsl,rcpm-wakeup = <&rcpm 0x0 0x40000000>;+ big-endian;+ fsl,ls1012a-ftm = <0x20000>;+ fsl,pfe = <0xf0000020>; };
"dt-bindings: soc: ..." for the subject
It is obvious reading the diff you are removing fsl,#rcpm-wakeup-cell.
What is not obvious is why? The commit msg should answer that.
You also are mixing several things in this patch like adding ls1012
which you don't mention. Please split.
Signed-off-by: Ran Wang <redacted>
---
Documentation/devicetree/bindings/soc/fsl/rcpm.txt | 42 ++++++-------------
1 files changed, 13 insertions(+), 29 deletions(-)
-----Original Message-----
From: Rob Herring <robh@kernel.org>
Sent: Tuesday, September 04, 2018 09:25
To: Ran Wang <redacted>
Cc: Leo Li <redacted>; Mark Rutland <mark.rutland@arm.com>;
linuxppc-dev@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org;
devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/3] Documentation: dt: binding: fsl: update property
description for RCPM
=20
On Fri, Aug 31, 2018 at 11:52:18AM +0800, Ran Wang wrote:
=20
"dt-bindings: soc: ..." for the subject
=20
It is obvious reading the diff you are removing fsl,#rcpm-wakeup-cell.
What is not obvious is why? The commit msg should answer that.
Sure, I will add this in next version patch.
You also are mixing several things in this patch like adding ls1012 which=
you
don't mention. Please split.
Got it, will split them and add more information in next version.
=20
Ran
quoted
Signed-off-by: Ran Wang <redacted>
---
Documentation/devicetree/bindings/soc/fsl/rcpm.txt | 42 ++++++------=
From: Scott Wood <oss@buserror.net> Date: 2018-09-07 20:22:56
On Fri, 2018-08-31 at 11:52 +0800, Ran Wang wrote:
+Optional properties:
+ - big-endian : Indicate RCPM registers is big-endian. A RCPM node
+ that doesn't have this property will be regarded as little-endian.
You've just broken all the existing powerpc device trees that are big-endian
and have no big-endian property.
+ - <property 'compatible' string of consumer device> : This string
+ is referred by RCPM driver to judge if the consumer (such as flex timer)
+ is able to be regards as wakeup source or not, such as 'fsl,ls1012a-
ftm'.
+ Further, this property will carry the bit mask info to control
+ coresponding wake up source.
What will you do if there are multiple instances of a device with the same
compatible, and different wakeup bits? Plus, it's an awkward design in
general, and you don't describe what the value actually means (bits in which
register?). What was wrong with the existing binding? Alternatively, use the
clock bindings.
On Mon, Sep 10, 2018 at 3:46 AM Ran Wang [off-list ref] wrote:
Hi Scott,
On 2018/9/8 4:23, Scott Wood wrote:
quoted
On Fri, 2018-08-31 at 11:52 +0800, Ran Wang wrote:
quoted
+Optional properties:
+ - big-endian : Indicate RCPM registers is big-endian. A RCPM node
+ that doesn't have this property will be regarded as little-endian.
You've just broken all the existing powerpc device trees that are big-endian
and have no big-endian property.
Yes, powerpc RCPM driver (arch/powerpc/sysdev/fsl_rcpm.c) will not refer
to big-endian. However, I think if Layerscape RCPM driver use different compatible
id (such as ' fsl,qoriq-rcpm-2.2'), it might be safe. Is that OK?
For Layerscape Chassis(gen3) based chips, the Reference Manual named
the power management IP as COP_PMU instead of RCPM which makes sense
to me as the register map is also quite different from the reg map of
RCPM. So I think it is reasonable to create a new binding and driver
for the Chassis Gen3 based PMU. However, the
arch/powerpc/sysdev/fsl_rcpm.c driver probably should be moved to
drivers/soc/fsl, as the Gen2.x Chassis and RCPM IP are also used in
some non-PowerPC Layerscape SoCs. From what I know, all the RCPM
based IP are all big endian, so there is no need to add this property
for the old binding.
quoted
quoted
+ - <property 'compatible' string of consumer device> : This string
+ is referred by RCPM driver to judge if the consumer (such as flex timer)
+ is able to be regards as wakeup source or not, such as
+ 'fsl,ls1012a-
ftm'.
+ Further, this property will carry the bit mask info to control
+ coresponding wake up source.
What will you do if there are multiple instances of a device with the same
compatible, and different wakeup bits?
You got me! This is a problem in current version. Well, we have to decouple wake up
source IP and RCPM driver. That's why I add a plat_pm driver to prevent wake up IP
knowing any information of RCPM. So in current context, one idea come to me is to
redesign property ' fsl,ls1012a-ftm = <0x20000>;', change to 'fsl,ls1012a-ftm = <&ftm0 0x20000>;'
What do you say? And could you tell me which API I can use to check if that device's
name is ftm0 (for example we want to find a node ftm0: ftm@29d000)?
quoted
Plus, it's an awkward design in
general, and you don't describe what the value actually means (bits in which
register?).
Yes, I admit my design looks ugly and not flexible and extensible enough. However, for above reason,
do you have any good idea, please? :)
Why do you have to move the wakeup information from device nodes to
the RCPM/PMU node? For information related to both on-chip device and
SoC integration, the information normally goes into the node of
on-chip device instead of the integration IP's device node. Existing
example like: interrupt, clock, memory map, and etc. It is much
cleaner than listing all the interrupts in the interrupt controller's
node, right?
As to the register information, I can explain here in details (will add to commit
message of next version): There is a RCPM HW block which has register named IPPDEXPCR.
It controls whether prevent certain IP (such as timer, usb, etc) from entering low
power mode when system suspended. So it's necessary to program it if we want one
of those IP work as a wakeup source. However, different Layerscape SoCs have different bit vs.
IP mapping layout. So I have to record this information in device tree and fetched by RCPM driver
directly.
Do I need to list all SoC's mapping information in this binding doc for reference?
quoted
What was wrong with the existing binding?
There was one version of RCPM patch which requiring property 'fsl,#rcpm-wakeup-cells' but was not
accepted by upstream finally. Now we will no need it any longer due to new design allow case of multiple
RCPM devices existing case.
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 FSL platform PM driver framework which help to
isolate user and PM service provider (such as RCPM driver).
Signed-off-by: Chenhui Zhao <redacted>
Signed-off-by: Ying Zhang <redacted>
Signed-off-by: Ran Wang <redacted>
---
drivers/soc/fsl/Kconfig | 6 ++
drivers/soc/fsl/Makefile | 1 +
drivers/soc/fsl/ls-rcpm.c | 153 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 160 insertions(+), 0 deletions(-)
create mode 100644 drivers/soc/fsl/ls-rcpm.c
@@ -0,0 +1,153 @@+// SPDX-License-Identifier: GPL-2.0+//+// plat_pm.c - Freescale Layerscape RCPM driver+//+// Copyright 2018 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<soc/fsl/plat_pm.h>++#define MAX_COMPATIBLE_NUM 10++structrcpm_t{+structdevice*dev;+void__iomem*ippdexpcr_addr;+boolbig_endian;/* Big/Little endian of RCPM module */+};++// rcpm_handle - Configure RCPM reg according to wake up source request+// @user_dev: pointer to user's device struct+// @flag: to enable(true) or disable(false) wakeup source+// @handle_priv: pointer to struct rcpm_t instance+//+// Return 0 on success other negative errno+staticintrcpm_handle(structdevice*user_dev,boolflag,void*handle_priv)+{+structrcpm_t*rcpm;+boolbig_endian;+constchar*dev_compatible_array[MAX_COMPATIBLE_NUM];+void__iomem*ippdexpcr_addr;+u32ippdexpcr;+u32set_bit;+intret,num,i;++rcpm=handle_priv;+big_endian=rcpm->big_endian;+ippdexpcr_addr=rcpm->ippdexpcr_addr;++num=device_property_read_string_array(user_dev,"compatible",+dev_compatible_array,MAX_COMPATIBLE_NUM);+if(num<0)+returnnum;++for(i=0;i<num;i++){+if(!device_property_present(rcpm->dev,+dev_compatible_array[i]))+continue;+else{+ret=device_property_read_u32(rcpm->dev,+dev_compatible_array[i],&set_bit);+if(ret)+returnret;++if(!device_property_present(rcpm->dev,+dev_compatible_array[i]))+return-ENODEV;+else{+ret=device_property_read_u32(rcpm->dev,+dev_compatible_array[i],&set_bit);+if(ret)+returnret;++if(big_endian)+ippdexpcr=ioread32be(ippdexpcr_addr);+else+ippdexpcr=ioread32(ippdexpcr_addr);++if(flag)+ippdexpcr|=set_bit;+else+ippdexpcr&=~set_bit;++if(big_endian){+iowrite32be(ippdexpcr,ippdexpcr_addr);+ippdexpcr=ioread32be(ippdexpcr_addr);+}else+iowrite32(ippdexpcr,ippdexpcr_addr);++return0;+}+}+}++return-ENODEV;+}++staticintls_rcpm_probe(structplatform_device*pdev)+{+structresource*r;+structrcpm_t*rcpm;++r=platform_get_resource(pdev,IORESOURCE_MEM,0);+if(!r)+return-ENODEV;++rcpm=kmalloc(sizeof(*rcpm),GFP_KERNEL);+if(!rcpm)+return-ENOMEM;++rcpm->big_endian=device_property_read_bool(&pdev->dev,"big-endian");++rcpm->ippdexpcr_addr=devm_ioremap_resource(&pdev->dev,r);+if(IS_ERR(rcpm->ippdexpcr_addr))+returnPTR_ERR(rcpm->ippdexpcr_addr);++rcpm->dev=&pdev->dev;+platform_set_drvdata(pdev,rcpm);++returnregister_fsl_platform_wakeup_source(rcpm_handle,rcpm);+}++staticintls_rcpm_remove(structplatform_device*pdev)+{+structrcpm_t*rcpm;++rcpm=platform_get_drvdata(pdev);+deregister_fsl_platform_wakeup_source(rcpm);+kfree(rcpm);++return0;+}++staticconststructof_device_idls_rcpm_of_match[]={+{.compatible="fsl,qoriq-rcpm-2.1",},+{}+};+MODULE_DEVICE_TABLE(of,ls_rcpm_of_match);++staticstructplatform_driverls_rcpm_driver={+.driver={+.name="ls-rcpm",+.of_match_table=ls_rcpm_of_match,+},+.probe=ls_rcpm_probe,+.remove=ls_rcpm_remove,+};++staticint__initls_rcpm_init(void)+{+returnplatform_driver_register(&ls_rcpm_driver);+}+subsys_initcall(ls_rcpm_init);++staticvoid__exitls_rcpm_exit(void)+{+platform_driver_unregister(&ls_rcpm_driver);+}+module_exit(ls_rcpm_exit);
Please change your comments style.=0A=
=0A=
On 2018/8/31 11:56, Ran Wang wrote:=0A=
The NXP's QorIQ Processors based on ARM Core have RCPM module (Run=0A=
Control and Power Management), which performs all device-level=0A=
tasks associated with power management such as wakeup source control.=0A=
=0A=
This driver depends on FSL platform PM driver framework which help to=0A=
isolate user and PM service provider (such as RCPM driver).=0A=
=0A=
Signed-off-by: Chenhui Zhao <redacted>=0A=
Signed-off-by: Ying Zhang <redacted>=0A=
Signed-off-by: Ran Wang <redacted>=0A=
---=0A=
drivers/soc/fsl/Kconfig | 6 ++=0A=
drivers/soc/fsl/Makefile | 1 +=0A=
drivers/soc/fsl/ls-rcpm.c | 153 +++++++++++++++++++++++++++++++++++++++=
design changed and remain user side unchanged.=0A=
+=0A=
+config LS_RCPM=0A=
+ bool "Freescale RCPM support"=0A=
+ depends on (FSL_PLAT_PM)=0A=
+ help=0A=
+ This feature is to enable specified wakeup source for system sleep.=
On Tue, Sep 4, 2018 at 9:58 PM Wang, Dongsheng
[off-list ref] wrote:
Please change your comments style.
Although this doesn't get into the Linux kernel coding style
documentation yet, Linus seems changed his mind to prefer // than /*
*/ comment style now. https://lkml.org/lkml/2017/11/25/133 So the
// style should be acceptable for now.
On 2018/8/31 11:56, Ran Wang wrote:
quoted
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 FSL platform PM driver framework which help to
isolate user and PM service provider (such as RCPM driver).
Signed-off-by: Chenhui Zhao <redacted>
Signed-off-by: Ying Zhang <redacted>
Signed-off-by: Ran Wang <redacted>
---
drivers/soc/fsl/Kconfig | 6 ++
drivers/soc/fsl/Makefile | 1 +
drivers/soc/fsl/ls-rcpm.c | 153 +++++++++++++++++++++++++++++++++++++=
Besides, it is also easy for service side to upgrade its logic =
when
quoted
design changed and remain user side unchanged.
+
+config LS_RCPM
+ bool "Freescale RCPM support"
+ depends on (FSL_PLAT_PM)
+ help
+ This feature is to enable specified wakeup source for system sl=
On Fri, Sep 7, 2018 at 4:51 AM Ran Wang [off-list ref] wrote:
Hi Leo,
On September 05, 2018 at 11:22 Yang Li wrote:
quoted
-----Original Message-----
From: Li Yang <redacted>
Sent: Wednesday, September 05, 2018 11:22
To: dongsheng.wang@hxt-semitech.com
Cc: Ran Wang <redacted>; Rob Herring <robh+dt@kernel.org>;
Mark Rutland [off-list ref]; open list:OPEN FIRMWARE AND
FLATTENED DEVICE TREE BINDINGS [off-list ref]; linuxppc-
dev [off-list ref]; lkml [off-list ref];
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE <linux-arm-
kernel@lists.infradead.org>
Subject: Re: [PATCH 3/3] soc: fsl: add RCPM driver
On Tue, Sep 4, 2018 at 9:58 PM Wang, Dongsheng <dongsheng.wang@hxt-
semitech.com> wrote:
quoted
Please change your comments style.
Although this doesn't get into the Linux kernel coding style documentation
yet, Linus seems changed his mind to prefer // than /*
*/ comment style now.
https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml
.org%2Flkml%2F2017%2F11%2F25%2F133&data=02%7C01%7Cran.wang_
1%40nxp.com%7Cc0d88e6690384e02b95108d612dec235%7C686ea1d3bc2b4c
6fa92cd99c5c301635%7C0%7C0%7C636717145285126200&sdata=JIoCZp
WhRyW76EqgSflfTDA1f0gMQGKa%2FcbvSc5CO%2Fw%3D&reserved=0
So the
// style should be acceptable for now.
quoted
On 2018/8/31 11:56, Ran Wang wrote:
quoted
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 FSL platform PM driver framework which help
to isolate user and PM service provider (such as RCPM driver).
Signed-off-by: Chenhui Zhao <redacted>
Signed-off-by: Ying Zhang <redacted>
Signed-off-by: Ran Wang <redacted>
---
drivers/soc/fsl/Kconfig | 6 ++
drivers/soc/fsl/Makefile | 1 +
drivers/soc/fsl/ls-rcpm.c | 153
+++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 160 insertions(+), 0 deletions(-) create mode
100644 drivers/soc/fsl/ls-rcpm.c
diff --git a/drivers/soc/fsl/Kconfig b/drivers/soc/fsl/Kconfig index
The file name here is not the same as the real file name.
Got it, will correct it.
quoted
quoted
quoted
+//
+// Copyright 2018 NXP
+//
+// Author: Ran Wang [off-list ref],
Where do you need the comma in the end?
My bad, will remove comma in next version.
quoted
quoted
quoted
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/of_address.h>
+#include <linux/slab.h>
+#include <soc/fsl/plat_pm.h>
+
+#define MAX_COMPATIBLE_NUM 10
+
+struct rcpm_t {
+ struct device *dev;
+ void __iomem *ippdexpcr_addr;
+ bool big_endian; /* Big/Little endian of RCPM module */
+};
+
+// rcpm_handle - Configure RCPM reg according to wake up source
+request // @user_dev: pointer to user's device struct // @flag: to
+enable(true) or disable(false) wakeup source // @handle_priv:
+pointer to struct rcpm_t instance // // Return 0 on success other
+negative errno
Although Linus preferred this // comment style. I'm not sure if this will be
handled correctly by the kernel-doc compiler.
https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fww
w.kernel.org%2Fdoc%2Fhtml%2Fv4.18%2Fdoc-guide%2Fkernel-
doc.html&data=02%7C01%7Cran.wang_1%40nxp.com%7Cc0d88e669038
4e02b95108d612dec235%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
%7C636717145285126200&sdata=H7GkUNOLVG%2FCcZESzhtHBeHCbO9
%2FK4k9EdH30Cxq2%2BM%3D&reserved=0
So, do you think I need to change all comment style back to '/* ... */' ?
Actually I feel a little bit confused here.
I think Linus's comment about // comment style applies to normal code
comment. But kernel-doc comment is a special kind of code comment
that needs to meet certain requirements. People can use the
scripts/kernel-doc tool to generate readable API documents from the
source code. It looks like you wanted to make the function
description aligned with the kernel-doc format, but kernel-doc
specifically requires to use the /* */ style(at least for now).
Regards,
Leo
From: Scott Wood <oss@buserror.net> Date: 2018-09-07 20:25:12
On Fri, 2018-08-31 at 11:52 +0800, Ran Wang wrote:
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 FSL platform PM driver framework which help to
isolate user and PM service provider (such as RCPM driver).
Signed-off-by: Chenhui Zhao <redacted>
Signed-off-by: Ying Zhang <redacted>
Signed-off-by: Ran Wang <redacted>
---
drivers/soc/fsl/Kconfig | 6 ++
drivers/soc/fsl/Makefile | 1 +
drivers/soc/fsl/ls-rcpm.c | 153
+++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 160 insertions(+), 0 deletions(-)
create mode 100644 drivers/soc/fsl/ls-rcpm.c
Is there a reason why this is LS-specific, or could it be used with PPC RCPM
blocks?
Please change your comments style.=0A=
=0A=
On 2018/8/31 11:57, Ran Wang wrote:=0A=
This driver is to provide a independent framework for PM service=0A=
provider and consumer to configure system level wake up feature. For=0A=
example, RCPM driver could register a callback function on this=0A=
platform first, and Flex timer driver who want to enable timer wake=0A=
up feature, will call generic API provided by this platform driver,=0A=
and then it will trigger RCPM driver to do it. The benefit is to=0A=
isolate the user and service, such as flex timer driver will not have=0A=
to know the implement details of wakeup function it require. Besides,=0A=
it is also easy for service side to upgrade its logic when design is=0A=
changed and remain user side unchanged.=0A=
=0A=
Signed-off-by: Ran Wang <redacted>=0A=
---=0A=
drivers/soc/fsl/Kconfig | 14 +++++=0A=
drivers/soc/fsl/Makefile | 1 +=0A=
drivers/soc/fsl/plat_pm.c | 144 +++++++++++++++++++++++++++++++++++++++=
Other guts accesses, such as reading RCW, should eventually be moved=
=0A=
into this driver as well.=0A=
+=0A=
+config FSL_PLAT_PM=0A=
+ bool "Freescale platform PM framework"=0A=
+ help=0A=
+ This driver is to provide a independent framework for PM service=0A=
+ provider and consumer to configure system level wake up feature. For=
=0A=
+ example, RCPM driver could register a callback function on this=0A=
+ platform first, and Flex timer driver who want to enable timer wake=
=0A=
+ up feature, will call generic API provided by this platform driver,=
=0A=
+ and then it will trigger RCPM driver to do it. The benefit is to=0A=
+ isolate the user and service, such as flex timer driver will not=0A=
+ have to know the implement details of wakeup function it require.=0A=
+ Besides, it is also easy for service side to upgrade its logic when=
=0A=
quoted hunk
+ design changed and remain user side unchanged.=0A=
On 2018/9/5 11:05, Dongsheng Wang wrote:
=20
Please change your comments style.
=20
On 2018/8/31 11:57, Ran Wang wrote:
quoted
This driver is to provide a independent framework for PM service
provider and consumer to configure system level wake up feature. For
example, RCPM driver could register a callback function on this
platform first, and Flex timer driver who want to enable timer wake up
feature, will call generic API provided by this platform driver, and
then it will trigger RCPM driver to do it. The benefit is to isolate
the user and service, such as flex timer driver will not have to know
the implement details of wakeup function it require. Besides, it is
also easy for service side to upgrade its logic when design is changed
and remain user side unchanged.
Signed-off-by: Ran Wang <redacted>
---
drivers/soc/fsl/Kconfig | 14 +++++
drivers/soc/fsl/Makefile | 1 +
drivers/soc/fsl/plat_pm.c | 144
Other guts accesses, such as reading RCW, should eventually be
moved
quoted
into this driver as well.
+
+config FSL_PLAT_PM
+ bool "Freescale platform PM framework"
+ help
+ This driver is to provide a independent framework for PM service
+ provider and consumer to configure system level wake up feature.
For
quoted
+ example, RCPM driver could register a callback function on this
+ platform first, and Flex timer driver who want to enable timer wake
+ up feature, will call generic API provided by this platform driver,
+ and then it will trigger RCPM driver to do it. The benefit is to
+ isolate the user and service, such as flex timer driver will not
+ have to know the implement details of wakeup function it require.
+ Besides, it is also easy for service side to upgrade its logic when
+ design changed and remain user side unchanged.
diff --git a/drivers/soc/fsl/Makefile b/drivers/soc/fsl/Makefile index
diff --git a/drivers/soc/fsl/plat_pm.c b/drivers/soc/fsl/plat_pm.c new
file mode 100644 index 0000000..19ea14e
--- /dev/null+++ b/drivers/soc/fsl/plat_pm.c
@@ -0,0 +1,144 @@+// SPDX-License-Identifier: GPL-2.0+//+// plat_pm.c - Freescale platform PM framework // // Copyright 2018+NXP// // Author: Ran Wang <ran.wang_1@nxp.com>,++#include<linux/kernel.h>+#include<linux/device.h>+#include<linux/list.h>+#include<linux/slab.h>+#include<linux/err.h>+#include<soc/fsl/plat_pm.h>+++structplat_pm_t{+structlist_headnode;+fsl_plat_pm_handlehandle;+void*handle_priv;+spinlock_tlock;+};++staticstructplat_pm_tplat_pm;++// register_fsl_platform_wakeup_source - Register callback function+toplat_pm// @handle: Pointer to handle PM feature requirement //+@handle_priv:Handlerspecificdatastruct// // Return 0 on success+othernegativeerrnoint+register_fsl_platform_wakeup_source(fsl_plat_pm_handlehandle,+void*handle_priv)+{+structplat_pm_t*p;+unsignedlongflags;++if(!handle){+pr_err("FSL plat_pm: Handler invalid, reject\n");+return-EINVAL;+}++p=3Dkmalloc(sizeof(*p),GFP_KERNEL);+if(!p)+return-ENOMEM;++p->handle=3Dhandle;+p->handle_priv=3Dhandle_priv;++spin_lock_irqsave(&plat_pm.lock,flags);+list_add_tail(&p->node,&plat_pm.node);+spin_unlock_irqrestore(&plat_pm.lock,flags);++return0;+}+EXPORT_SYMBOL_GPL(register_fsl_platform_wakeup_source);++// Deregister_fsl_platform_wakeup_source - deregister callback+function// @handle_priv: Handler specific data struct // // Return 0+onsuccessothernegativeerrnoint+deregister_fsl_platform_wakeup_source(void*handle_priv){+structplat_pm_t*p,*tmp;+unsignedlongflags;++spin_lock_irqsave(&plat_pm.lock,flags);+list_for_each_entry_safe(p,tmp,&plat_pm.node,node){+if(p->handle_priv=3D=3Dhandle_priv){+list_del(&p->node);+kfree(p);+}+}+spin_unlock_irqrestore(&plat_pm.lock,flags);+return0;+}+EXPORT_SYMBOL_GPL(deregister_fsl_platform_wakeup_source);++// fsl_platform_wakeup_config - Configure wakeup source by calling+handlers// @dev: pointer to user's device struct // @flag: to tell+enableordisablewakeupsource// // Return 0 on success other+negativeerrnointfsl_platform_wakeup_config(structdevice*dev,+boolflag){+structplat_pm_t*p;+intret;+boolsuccess_handled;+unsignedlongflags;++success_handled=3Dfalse;++// Will consider success if at least one callback return 0.+// Also, rest handles still get oppertunity to be executed+spin_lock_irqsave(&plat_pm.lock,flags);+list_for_each_entry(p,&plat_pm.node,node){+if(p->handle){+ret=3Dp->handle(dev,flag,p->handle_priv);+if(!ret)+success_handled=3Dtrue;
Miss a break?
Actually my idea is to allow more than one registered handler to handle thi=
s
request, so I define a flag rather than return to indicated if there is at =
least one handler successfully
do it. This design might give more flexibility to framework when running.
quoted
+ else if (ret !=3D -ENODEV) {
+ pr_err("FSL plat_pm: Failed to config wakeup
source:%d\n", ret);
Please unlock before return.
Yes, will fix it in next version, thanks for pointing out!
+ }
+ spin_unlock_irqrestore(&plat_pm.lock, flags);
+
+ if (success_handled =3D=3D false) {
+ pr_err("FSL plat_pm: Cannot find the matchhed handler for
wakeup source config\n");
quoted
+ return -ENODEV;
+ }
Add this into the loop.
My design is that if the 1st handler return -ENODEV to indicated this devic=
e it doesn't support,=20
then the framework will continue try 2nd handler...
So I think it is needed to place this checking out of loop, what do you say=
?
Regards,
Ran
On 2018/9/5 11:05, Dongsheng Wang wrote:=0A=
=0A=
Please change your comments style.=0A=
=0A=
On 2018/8/31 11:57, Ran Wang wrote:=0A=
quoted
This driver is to provide a independent framework for PM service=0A=
provider and consumer to configure system level wake up feature. For=0A=
example, RCPM driver could register a callback function on this=0A=
platform first, and Flex timer driver who want to enable timer wake up=
=0A=
quoted
quoted
feature, will call generic API provided by this platform driver, and=0A=
then it will trigger RCPM driver to do it. The benefit is to isolate=0A=
the user and service, such as flex timer driver will not have to know=
=0A=
quoted
quoted
the implement details of wakeup function it require. Besides, it is=0A=
also easy for service side to upgrade its logic when design is changed=
=0A=
quoted
quoted
and remain user side unchanged.=0A=
=0A=
Signed-off-by: Ran Wang <redacted>=0A=
---=0A=
drivers/soc/fsl/Kconfig | 14 +++++=0A=
drivers/soc/fsl/Makefile | 1 +=0A=
drivers/soc/fsl/plat_pm.c | 144=0A=
Other guts accesses, such as reading RCW, should eventually be=0A=
moved=0A=
quoted
into this driver as well.=0A=
+=0A=
+config FSL_PLAT_PM=0A=
+ bool "Freescale platform PM framework"=0A=
+ help=0A=
+ This driver is to provide a independent framework for PM service=0A=
+ provider and consumer to configure system level wake up feature.=0A=
For=0A=
quoted
+ example, RCPM driver could register a callback function on this=0A=
+ platform first, and Flex timer driver who want to enable timer wake=
=0A=
quoted
quoted
+ up feature, will call generic API provided by this platform driver,=
=0A=
quoted
quoted
+ and then it will trigger RCPM driver to do it. The benefit is to=0A=
+ isolate the user and service, such as flex timer driver will not=
=0A=
quoted
quoted
+ have to know the implement details of wakeup function it require.=
=0A=
quoted
quoted
+ Besides, it is also easy for service side to upgrade its logic when=
=0A=
quoted
quoted
+ design changed and remain user side unchanged.=0A=
+on success other negative errno int=0A=
+deregister_fsl_platform_wakeup_source(void *handle_priv) {=0A=
+ struct plat_pm_t *p, *tmp;=0A=
+ unsigned long flags;=0A=
+=0A=
+ spin_lock_irqsave(&plat_pm.lock, flags);=0A=
+ list_for_each_entry_safe(p, tmp, &plat_pm.node, node) {=0A=
+ if (p->handle_priv =3D=3D handle_priv) {=0A=
+ list_del(&p->node);=0A=
+ kfree(p);=0A=
+ }=0A=
+ }=0A=
+ spin_unlock_irqrestore(&plat_pm.lock, flags);=0A=
+ return 0;=0A=
+}=0A=
+EXPORT_SYMBOL_GPL(deregister_fsl_platform_wakeup_source);=0A=
+=0A=
+// fsl_platform_wakeup_config - Configure wakeup source by calling=0A=
+handlers // @dev: pointer to user's device struct // @flag: to tell=0A=
+enable or disable wakeup source // // Return 0 on success other=0A=
+negative errno int fsl_platform_wakeup_config(struct device *dev,=0A=
+bool flag) {=0A=
+ struct plat_pm_t *p;=0A=
+ int ret;=0A=
+ bool success_handled;=0A=
+ unsigned long flags;=0A=
+=0A=
+ success_handled =3D false;=0A=
+=0A=
+ // Will consider success if at least one callback return 0.=0A=
+ // Also, rest handles still get oppertunity to be executed=0A=
+ spin_lock_irqsave(&plat_pm.lock, flags);=0A=
+ list_for_each_entry(p, &plat_pm.node, node) {=0A=
+ if (p->handle) {=0A=
+ ret =3D p->handle(dev, flag, p->handle_priv);=0A=
+ if (!ret)=0A=
+ success_handled =3D true;=0A=
Miss a break?=0A=
Actually my idea is to allow more than one registered handler to handle t=
his=0A=
request, so I define a flag rather than return to indicated if there is a=
t least one handler successfully=0A=
do it. This design might give more flexibility to framework when running.=
=0A=
There is only one flag(success_handled) here, how did know which handler=0A=
failed?=0A=
=0A=
BTW, I don't think we need this flag. We can only use the return value.=0A=
=0A=
Cheers,=0A=
Dongsheng=0A=
=0A=
quoted
quoted
+ else if (ret !=3D -ENODEV) {=0A=
+ pr_err("FSL plat_pm: Failed to config wakeup=0A=
source:%d\n", ret);=0A=
Please unlock before return.=0A=
Yes, will fix it in next version, thanks for pointing out!=0A=
=0A=
Hi Dongsheng,
On 2018/9/7 18:16, Dongsheng Wang wrote:
=20
On 2018/9/7 16:49, Ran Wang wrote:
quoted
Hi Dongsheng
quoted
On 2018/9/5 11:05, Dongsheng Wang wrote:
Please change your comments style.
On 2018/8/31 11:57, Ran Wang wrote:
quoted
This driver is to provide a independent framework for PM service
provider and consumer to configure system level wake up feature. For
example, RCPM driver could register a callback function on this
platform first, and Flex timer driver who want to enable timer wake
up feature, will call generic API provided by this platform driver,
and then it will trigger RCPM driver to do it. The benefit is to
isolate the user and service, such as flex timer driver will not
have to know the implement details of wakeup function it require.
Besides, it is also easy for service side to upgrade its logic when
design is changed and remain user side unchanged.
Signed-off-by: Ran Wang <redacted>
---
drivers/soc/fsl/Kconfig | 14 +++++
drivers/soc/fsl/Makefile | 1 +
drivers/soc/fsl/plat_pm.c | 144
Other guts accesses, such as reading RCW, should eventually be
moved
quoted
into this driver as well.
+
+config FSL_PLAT_PM
+ bool "Freescale platform PM framework"
+ help
+ This driver is to provide a independent framework for PM service
+ provider and consumer to configure system level wake up feature.
For
quoted
+ example, RCPM driver could register a callback function on this
+ platform first, and Flex timer driver who want to enable timer wa=
ke
quoted
quoted
quoted
+ up feature, will call generic API provided by this platform drive=
r,
quoted
quoted
quoted
+ and then it will trigger RCPM driver to do it. The benefit is to
+ isolate the user and service, such as flex timer driver will not
+ have to know the implement details of wakeup function it require.
+ Besides, it is also easy for service side to upgrade its logic wh=
@@ -0,0 +1,144 @@+// SPDX-License-Identifier: GPL-2.0 // // plat_pm.c - Freescale+platformPMframework// // Copyright 2018 NXP // // Author: Ran+Wang<ran.wang_1@nxp.com>,++#include<linux/kernel.h>+#include<linux/device.h>+#include<linux/list.h>+#include<linux/slab.h>+#include<linux/err.h>+#include<soc/fsl/plat_pm.h>+++structplat_pm_t{+structlist_headnode;+fsl_plat_pm_handlehandle;+void*handle_priv;+spinlock_tlock;+};++staticstructplat_pm_tplat_pm;++// register_fsl_platform_wakeup_source - Register callback function+toplat_pm// @handle: Pointer to handle PM feature requirement //+@handle_priv:Handlerspecificdatastruct// // Return 0 on+successothernegativeerrnoint+register_fsl_platform_wakeup_source(fsl_plat_pm_handlehandle,+void*handle_priv)+{+structplat_pm_t*p;+unsignedlongflags;++if(!handle){+pr_err("FSL plat_pm: Handler invalid, reject\n");+return-EINVAL;+}++p=3Dkmalloc(sizeof(*p),GFP_KERNEL);+if(!p)+return-ENOMEM;++p->handle=3Dhandle;+p->handle_priv=3Dhandle_priv;++spin_lock_irqsave(&plat_pm.lock,flags);+list_add_tail(&p->node,&plat_pm.node);+spin_unlock_irqrestore(&plat_pm.lock,flags);++return0;+}+EXPORT_SYMBOL_GPL(register_fsl_platform_wakeup_source);++// Deregister_fsl_platform_wakeup_source - deregister callback+function// @handle_priv: Handler specific data struct // // Return+0onsuccessothernegativeerrnoint+deregister_fsl_platform_wakeup_source(void*handle_priv){+structplat_pm_t*p,*tmp;+unsignedlongflags;++spin_lock_irqsave(&plat_pm.lock,flags);+list_for_each_entry_safe(p,tmp,&plat_pm.node,node){+if(p->handle_priv=3D=3Dhandle_priv){+list_del(&p->node);+kfree(p);+}+}+spin_unlock_irqrestore(&plat_pm.lock,flags);+return0;+}+EXPORT_SYMBOL_GPL(deregister_fsl_platform_wakeup_source);++// fsl_platform_wakeup_config - Configure wakeup source by calling+handlers// @dev: pointer to user's device struct // @flag: to tell+enableordisablewakeupsource// // Return 0 on success other+negativeerrnointfsl_platform_wakeup_config(structdevice*dev,+boolflag){+structplat_pm_t*p;+intret;+boolsuccess_handled;+unsignedlongflags;++success_handled=3Dfalse;++// Will consider success if at least one callback return 0.+// Also, rest handles still get oppertunity to be executed+spin_lock_irqsave(&plat_pm.lock,flags);+list_for_each_entry(p,&plat_pm.node,node){+if(p->handle){+ret=3Dp->handle(dev,flag,p->handle_priv);+if(!ret)+success_handled=3Dtrue;
Miss a break?
Actually my idea is to allow more than one registered handler to
handle this request, so I define a flag rather than return to
indicated if there is at least one handler successfully do it. This des=
ign
might give more flexibility to framework when running.
There is only one flag(success_handled) here, how did know which handler
failed?
=20
BTW, I don't think we need this flag. We can only use the return value.
Well, the plat_pm driver will not handle most errors returned by registered
handlers, except -NODEV. For -NODEV, plat_pm driver consider that handler
cannot support this request and will go to call next one.=20
Besides, actually it doesn't restrict that request can be served by only on=
e=20
handler. So I add that flag to cover the case of more than one handler can=
=20
successfully support and others might return -NODEV.
Regards,
Ran
Cheers,
Dongsheng
=20
quoted
quoted
quoted
+ else if (ret !=3D -ENODEV) {
+ pr_err("FSL plat_pm: Failed to config wakeup
source:%d\n", ret);
Please unlock before return.
Yes, will fix it in next version, thanks for pointing out!
From: Scott Wood <oss@buserror.net> Date: 2018-09-07 20:35:15
On Fri, 2018-08-31 at 11:52 +0800, Ran Wang wrote:
quoted hunk
This driver is to provide a independent framework for PM service
provider and consumer to configure system level wake up feature. For
example, RCPM driver could register a callback function on this
platform first, and Flex timer driver who want to enable timer wake
up feature, will call generic API provided by this platform driver,
and then it will trigger RCPM driver to do it. The benefit is to
isolate the user and service, such as flex timer driver will not have
to know the implement details of wakeup function it require. Besides,
it is also easy for service side to upgrade its logic when design is
changed and remain user side unchanged.
Signed-off-by: Ran Wang <redacted>
---
drivers/soc/fsl/Kconfig | 14 +++++
drivers/soc/fsl/Makefile | 1 +
drivers/soc/fsl/plat_pm.c | 144
+++++++++++++++++++++++++++++++++++++++++++++
include/soc/fsl/plat_pm.h | 22 +++++++
4 files changed, 181 insertions(+), 0 deletions(-)
create mode 100644 drivers/soc/fsl/plat_pm.c
create mode 100644 include/soc/fsl/plat_pm.h
This name seems to be simultaneously too generic (for something that is likely
intended only for use with certain Freescale/NXP chip families) and too
specific (for something that seems to be general infrastructure with no real
hardware dependencies).
What specific problems with Linux's generic wakeup infrastructure is this
trying to solve, and why would those problems not be better solved there?
Also, you should CC linux-pm on these patches.
-Scott