Re: [PATCH v5 4/5] fsl_pmc: Add API to enable device as wakeup event source
On 06/05/2012 11:06 PM, Li Yang wrote: On Wed, Jun 6, 2012 at 2:05 AM, Scott Wood scottw...@freescale.com wrote: You ignored what about devices other than ethernet. No, I haven't. Other devices are so at least for now. I don't understand that last sentence. Other devices are what? -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v5 4/5] fsl_pmc: Add API to enable device as wakeup event source
On Thu, Jun 7, 2012 at 2:29 AM, Scott Wood scottw...@freescale.com wrote: On 06/05/2012 11:06 PM, Li Yang wrote: On Wed, Jun 6, 2012 at 2:05 AM, Scott Wood scottw...@freescale.com wrote: You ignored what about devices other than ethernet. No, I haven't. Other devices are so at least for now. I don't understand that last sentence. Other devices are what? Probably I misunderstood your question what about devices other than ethernet. Did you mean how would other devices other than ethernet know how to set it? Other wakeup capable devices can call the API when it is up and running. It will be the pmc driver's responsibility to find out if that specific device can be configured as a wakeup source for the SoC. Leo ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v5 4/5] fsl_pmc: Add API to enable device as wakeup event source
On 06/04/2012 11:08 PM, Li Yang-R58472 wrote: -Original Message- From: Wood Scott-B07421 Sent: Tuesday, June 05, 2012 7:03 AM To: Zhao Chenhui-B35336 Cc: linuxppc-dev@lists.ozlabs.org; linux-ker...@vger.kernel.org; ga...@kernel.crashing.org; Li Yang-R58472 Subject: Re: [PATCH v5 4/5] fsl_pmc: Add API to enable device as wakeup event source On 06/04/2012 06:36 AM, Zhao Chenhui wrote: On Fri, Jun 01, 2012 at 05:08:52PM -0500, Scott Wood wrote: On 05/11/2012 06:53 AM, Zhao Chenhui wrote: +int mpc85xx_pmc_set_wake(struct platform_device *pdev, bool enable) Why does it have to be a platform_device? Would a bare device_node work here? If it's for stuff like device_may_wakeup() that could be in a platform_device wrapper function. It does not have to be a platform_device. I think it can be a struct device. Why does it even need that? The low level mechanism for influencing PMCDR should only need a device node, not a Linux device struct. It does no harm to pass the device structure and makes more sense for object oriented interface design. It does do harm if you don't have a device structure to pass, if for some reason you found the device by directly looking for it rather than going through the device model. Who is setting can_wakeup for these devices? The device driver is responsible to set can_wakeup. How would the device driver know how to set it? Wouldn't this depend on the particular SoC and low power mode? It is based on the fsl,magic-packet and fsl,wake-on-filer device tree properties. fsl,magic-packet was a mistake. It is equivalent to checking the compatible for etsec. It does not convey any information about whether the eTSEC is still active in a given low power mode. How is fsl,wake-os-filer relevant to this decision? When will it be set but not fsl,magic-packet? What about devices other than ethernet? What about differences between ordinary sleep and deep sleep? -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH v5 4/5] fsl_pmc: Add API to enable device as wakeup event source
-Original Message- From: Wood Scott-B07421 Sent: Wednesday, June 06, 2012 12:12 AM To: Li Yang-R58472 Cc: Wood Scott-B07421; Zhao Chenhui-B35336; linuxppc-dev@lists.ozlabs.org; linux-ker...@vger.kernel.org; ga...@kernel.crashing.org Subject: Re: [PATCH v5 4/5] fsl_pmc: Add API to enable device as wakeup event source On 06/04/2012 11:08 PM, Li Yang-R58472 wrote: -Original Message- From: Wood Scott-B07421 Sent: Tuesday, June 05, 2012 7:03 AM To: Zhao Chenhui-B35336 Cc: linuxppc-dev@lists.ozlabs.org; linux-ker...@vger.kernel.org; ga...@kernel.crashing.org; Li Yang-R58472 Subject: Re: [PATCH v5 4/5] fsl_pmc: Add API to enable device as wakeup event source On 06/04/2012 06:36 AM, Zhao Chenhui wrote: On Fri, Jun 01, 2012 at 05:08:52PM -0500, Scott Wood wrote: On 05/11/2012 06:53 AM, Zhao Chenhui wrote: +int mpc85xx_pmc_set_wake(struct platform_device *pdev, bool +enable) Why does it have to be a platform_device? Would a bare device_node work here? If it's for stuff like device_may_wakeup() that could be in a platform_device wrapper function. It does not have to be a platform_device. I think it can be a struct device. Why does it even need that? The low level mechanism for influencing PMCDR should only need a device node, not a Linux device struct. It does no harm to pass the device structure and makes more sense for object oriented interface design. It does do harm if you don't have a device structure to pass, if for some reason you found the device by directly looking for it rather than going through the device model. Whether or not a device is a wakeup source not only depends on the SoC specification but also the configuration and current state for the device. I only expect the device driver to have this knowledge and call this function rather than some standalone platform code. Therefore I don't think your concern matters. Who is setting can_wakeup for these devices? The device driver is responsible to set can_wakeup. How would the device driver know how to set it? Wouldn't this depend on the particular SoC and low power mode? It is based on the fsl,magic-packet and fsl,wake-on-filer device tree properties. fsl,magic-packet was a mistake. It is equivalent to checking the compatible for etsec. It does not convey any information about whether It can be described either by explicit feature property or by the compatible. I don't think it is a problem that we choose one against another. the eTSEC is still active in a given low power mode. Whether or not the eTSEC is still active in both sleep and deep sleep is only depending on if we set it to be a wakeup source. If it behaves differently for low power modes in the future, we could address that by adding additional property. How is fsl,wake-os-filer relevant to this decision? When will it be set but not fsl,magic-packet? I mean either fsl,magic-packet or fsl,wake-on-filer shows it can be a wakeup source. Currently we don't have an SoC to have wake-on-filer while not wake-on-magic. But I think it's better to consider both as they are independent features. What about devices other than ethernet? What about differences between ordinary sleep and deep sleep? There is no difference for sleep and deep sleep for all wakeup sources currently. We can address the problem if it is not the case in the future. Leo ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v5 4/5] fsl_pmc: Add API to enable device as wakeup event source
On 06/05/2012 11:49 AM, Li Yang-R58472 wrote: -Original Message- From: Wood Scott-B07421 Sent: Wednesday, June 06, 2012 12:12 AM To: Li Yang-R58472 Cc: Wood Scott-B07421; Zhao Chenhui-B35336; linuxppc-dev@lists.ozlabs.org; linux-ker...@vger.kernel.org; ga...@kernel.crashing.org Subject: Re: [PATCH v5 4/5] fsl_pmc: Add API to enable device as wakeup event source On 06/04/2012 11:08 PM, Li Yang-R58472 wrote: -Original Message- From: Wood Scott-B07421 Sent: Tuesday, June 05, 2012 7:03 AM To: Zhao Chenhui-B35336 Cc: linuxppc-dev@lists.ozlabs.org; linux-ker...@vger.kernel.org; ga...@kernel.crashing.org; Li Yang-R58472 Subject: Re: [PATCH v5 4/5] fsl_pmc: Add API to enable device as wakeup event source On 06/04/2012 06:36 AM, Zhao Chenhui wrote: On Fri, Jun 01, 2012 at 05:08:52PM -0500, Scott Wood wrote: On 05/11/2012 06:53 AM, Zhao Chenhui wrote: +int mpc85xx_pmc_set_wake(struct platform_device *pdev, bool +enable) Why does it have to be a platform_device? Would a bare device_node work here? If it's for stuff like device_may_wakeup() that could be in a platform_device wrapper function. It does not have to be a platform_device. I think it can be a struct device. Why does it even need that? The low level mechanism for influencing PMCDR should only need a device node, not a Linux device struct. It does no harm to pass the device structure and makes more sense for object oriented interface design. It does do harm if you don't have a device structure to pass, if for some reason you found the device by directly looking for it rather than going through the device model. Whether or not a device is a wakeup source not only depends on the SoC specification but also the configuration and current state for the device. I only expect the device driver to have this knowledge and call this function rather than some standalone platform code. Therefore I don't think your concern matters. First, I think it's bad API to force the passing of a higher level object when a lower level object would suffice (and there are no legitimate future-proofing or abstraction reasons for hiding the lower level object). But regardless, the entity you call a device driver may or may not use the standard driver model (e.g. look at PCI root complexes), and your assumption about what platform code knows may or may not be correct. I could just as well say that only platform code knows about the SoC clock/power routing during a given low power state. Who is setting can_wakeup for these devices? The device driver is responsible to set can_wakeup. How would the device driver know how to set it? Wouldn't this depend on the particular SoC and low power mode? It is based on the fsl,magic-packet and fsl,wake-on-filer device tree properties. fsl,magic-packet was a mistake. It is equivalent to checking the compatible for etsec. It does not convey any information about whether It can be described either by explicit feature property or by the compatible. I don't think it is a problem that we choose one against another. I do think it's a problem, because it's unnecessarily complicated, and more error prone (we probably didn't have fsl,magic-packet on all the SoCs that support it before the .dtsi refactoring). But my point was that it says nothing about whether the eTSEC will still be functioning during a low power state. the eTSEC is still active in a given low power mode. Whether or not the eTSEC is still active in both sleep and deep sleep is only depending on if we set it to be a wakeup source. Only because we happen to support eTSEC as a wakeup source on all SoCs. If it behaves differently for low power modes in the future, we could address that by adding additional property. How is fsl,wake-os-filer relevant to this decision? When will it be set but not fsl,magic-packet? I mean either fsl,magic-packet or fsl,wake-on-filer shows it can be a wakeup source. Currently we don't have an SoC to have wake-on-filer while not wake-on-magic. But I think it's better to consider both as they are independent features. You're not willing to consider an SoC where waking on eTSEC is unsupported, but you're willing to consider an eTSEC that has wake-on-filer but magic packet support has for some reason been dropped? What about devices other than ethernet? What about differences between ordinary sleep and deep sleep? There is no difference for sleep and deep sleep for all wakeup sources currently. I recall being able to wake an mpc85xx chip (maybe mpc8544?) from ordinary sleep using the DUART (even though the manual suggests that only external interrupts can be used -- not even eTSEC). You can't do that in deep sleep. You ignored what about devices other than ethernet. -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v5 4/5] fsl_pmc: Add API to enable device as wakeup event source
On Wed, Jun 6, 2012 at 2:05 AM, Scott Wood scottw...@freescale.com wrote: On 06/05/2012 11:49 AM, Li Yang-R58472 wrote: On 06/04/2012 06:36 AM, Zhao Chenhui wrote: On Fri, Jun 01, 2012 at 05:08:52PM -0500, Scott Wood wrote: On 05/11/2012 06:53 AM, Zhao Chenhui wrote: +int mpc85xx_pmc_set_wake(struct platform_device *pdev, bool +enable) Why does it have to be a platform_device? Would a bare device_node work here? If it's for stuff like device_may_wakeup() that could be in a platform_device wrapper function. It does not have to be a platform_device. I think it can be a struct device. Why does it even need that? The low level mechanism for influencing PMCDR should only need a device node, not a Linux device struct. It does no harm to pass the device structure and makes more sense for object oriented interface design. It does do harm if you don't have a device structure to pass, if for some reason you found the device by directly looking for it rather than going through the device model. Whether or not a device is a wakeup source not only depends on the SoC specification but also the configuration and current state for the device. I only expect the device driver to have this knowledge and call this function rather than some standalone platform code. Therefore I don't think your concern matters. First, I think it's bad API to force the passing of a higher level object when a lower level object would suffice (and there are no legitimate future-proofing or abstraction reasons for hiding the lower level object). But regardless, the entity you call a device driver may or may not use the standard driver model (e.g. look at PCI root complexes), and your assumption about what platform code knows may or may not be correct. I could just as well say that only platform code knows about the SoC clock/power routing during a given low power state. Good point. We need to fix such non-standard drivers. The new PM framework depends a lot on the standard Linux Driver Model. We need to change our drivers to make them work better with PM. Also we have already submitted a patch series to change the PCI root complex driver in that regard. Who is setting can_wakeup for these devices? The device driver is responsible to set can_wakeup. How would the device driver know how to set it? Wouldn't this depend on the particular SoC and low power mode? It is based on the fsl,magic-packet and fsl,wake-on-filer device tree properties. fsl,magic-packet was a mistake. It is equivalent to checking the compatible for etsec. It does not convey any information about whether It can be described either by explicit feature property or by the compatible. I don't think it is a problem that we choose one against another. I do think it's a problem, because it's unnecessarily complicated, and more error prone (we probably didn't have fsl,magic-packet on all the SoCs that support it before the .dtsi refactoring). But my point was that it says nothing about whether the eTSEC will still be functioning during a low power state. the eTSEC is still active in a given low power mode. Whether or not the eTSEC is still active in both sleep and deep sleep is only depending on if we set it to be a wakeup source. Only because we happen to support eTSEC as a wakeup source on all SoCs. If it behaves differently for low power modes in the future, we could address that by adding additional property. How is fsl,wake-os-filer relevant to this decision? When will it be set but not fsl,magic-packet? I mean either fsl,magic-packet or fsl,wake-on-filer shows it can be a wakeup source. Currently we don't have an SoC to have wake-on-filer while not wake-on-magic. But I think it's better to consider both as they are independent features. You're not willing to consider an SoC where waking on eTSEC is unsupported, but you're willing to consider an eTSEC that has wake-on-filer but magic packet support has for some reason been dropped? Good findings. :) I think it's fine to keep the extra that have already been done as long as it does no harm. But we should stop adding more extras that are not necessary for now. What about devices other than ethernet? What about differences between ordinary sleep and deep sleep? There is no difference for sleep and deep sleep for all wakeup sources currently. I recall being able to wake an mpc85xx chip (maybe mpc8544?) from ordinary sleep using the DUART (even though the manual suggests that only external interrupts can be used -- not even eTSEC). You can't do that in deep sleep. I doubt that as the blocks are clock gated in sleep. We only have MPC8536 and P1022 in the e500 family to support deep sleep now. I agree with you the sleep and deep sleep can imply different wakeup source in the future. But can we worry about it later? You ignored what about devices other than ethernet. No, I haven't. Other devices are so at
Re: [PATCH v5 4/5] fsl_pmc: Add API to enable device as wakeup event source
On Fri, Jun 01, 2012 at 05:08:52PM -0500, Scott Wood wrote:
On 05/11/2012 06:53 AM, Zhao Chenhui wrote:
Add APIs for setting wakeup source and lossless Ethernet in low power modes.
These APIs can be used by wake-on-packet feature.
Signed-off-by: Dave Liu dave...@freescale.com
Signed-off-by: Li Yang le...@freescale.com
Signed-off-by: Jin Qing b24...@freescale.com
Signed-off-by: Zhao Chenhui chenhui.z...@freescale.com
---
arch/powerpc/sysdev/fsl_pmc.c | 71
-
arch/powerpc/sysdev/fsl_soc.h |9 +
2 files changed, 79 insertions(+), 1 deletions(-)
diff --git a/arch/powerpc/sysdev/fsl_pmc.c b/arch/powerpc/sysdev/fsl_pmc.c
index 1dc6e9e..c1170f7 100644
--- a/arch/powerpc/sysdev/fsl_pmc.c
+++ b/arch/powerpc/sysdev/fsl_pmc.c
@@ -34,6 +34,7 @@ struct pmc_regs {
__be32 powmgtcsr;
#define POWMGTCSR_SLP 0x0002
#define POWMGTCSR_DPSLP0x0010
+#define POWMGTCSR_LOSSLESS 0x0040
__be32 res3[2];
__be32 pmcdr;
};
@@ -43,6 +44,74 @@ static unsigned int pmc_flag;
#define PMC_SLEEP 0x1
#define PMC_DEEP_SLEEP 0x2
+#define PMC_LOSSLESS 0x4
+
+/**
+ * mpc85xx_pmc_set_wake - enable devices as wakeup event source
+ * @pdev: platform device affected
+ * @enable: True to enable event generation; false to disable
+ *
+ * This enables the device as a wakeup event source, or disables it.
+ *
+ * RETURN VALUE:
+ * 0 is returned on success
+ * -EINVAL is returned if device is not supposed to wake up the system
+ * Error code depending on the platform is returned if both the platform
and
+ * the native mechanism fail to enable the generation of wake-up events
+ */
+int mpc85xx_pmc_set_wake(struct platform_device *pdev, bool enable)
Why does it have to be a platform_device? Would a bare device_node work
here? If it's for stuff like device_may_wakeup() that could be in a
platform_device wrapper function.
It does not have to be a platform_device. I think it can be a struct device.
Where does this get called from? I don't see an example user in this
patchset.
It will be used by a gianfar related patch. I plan to submit that patch
after these patches accepted.
+{
+ int ret = 0;
+ struct device_node *clk_np;
+ u32 *prop;
+ u32 pmcdr_mask;
+
+ if (!pmc_regs) {
+ pr_err(%s: PMC is unavailable\n, __func__);
+ return -ENODEV;
+ }
+
+ if (enable !device_may_wakeup(pdev-dev))
+ return -EINVAL;
Who is setting can_wakeup for these devices?
The device driver is responsible to set can_wakeup.
+ clk_np = of_parse_phandle(pdev-dev.of_node, fsl,pmc-handle, 0);
+ if (!clk_np)
+ return -EINVAL;
+
+ prop = (u32 *)of_get_property(clk_np, fsl,pmcdr-mask, NULL);
Don't cast the const away.
OK.
+ if (!prop) {
+ ret = -EINVAL;
+ goto out;
+ }
+ pmcdr_mask = be32_to_cpup(prop);
+
+ if (enable)
+ /* clear to enable clock in low power mode */
+ clrbits32(pmc_regs-pmcdr, pmcdr_mask);
+ else
+ setbits32(pmc_regs-pmcdr, pmcdr_mask);
What is the default PMCDR if this function is never called? Should init
to all bits set on PM driver probe (or maybe limit it to defined bits
only, though that's a little harder to do generically).
-Scot
The default PMCDR is defined separately by individual chip.
I agree with you. I will have a try.
-Chenhui
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v5 4/5] fsl_pmc: Add API to enable device as wakeup event source
On 06/04/2012 06:36 AM, Zhao Chenhui wrote:
On Fri, Jun 01, 2012 at 05:08:52PM -0500, Scott Wood wrote:
On 05/11/2012 06:53 AM, Zhao Chenhui wrote:
Add APIs for setting wakeup source and lossless Ethernet in low power modes.
These APIs can be used by wake-on-packet feature.
Signed-off-by: Dave Liu dave...@freescale.com
Signed-off-by: Li Yang le...@freescale.com
Signed-off-by: Jin Qing b24...@freescale.com
Signed-off-by: Zhao Chenhui chenhui.z...@freescale.com
---
arch/powerpc/sysdev/fsl_pmc.c | 71
-
arch/powerpc/sysdev/fsl_soc.h |9 +
2 files changed, 79 insertions(+), 1 deletions(-)
diff --git a/arch/powerpc/sysdev/fsl_pmc.c b/arch/powerpc/sysdev/fsl_pmc.c
index 1dc6e9e..c1170f7 100644
--- a/arch/powerpc/sysdev/fsl_pmc.c
+++ b/arch/powerpc/sysdev/fsl_pmc.c
@@ -34,6 +34,7 @@ struct pmc_regs {
__be32 powmgtcsr;
#define POWMGTCSR_SLP 0x0002
#define POWMGTCSR_DPSLP0x0010
+#define POWMGTCSR_LOSSLESS 0x0040
__be32 res3[2];
__be32 pmcdr;
};
@@ -43,6 +44,74 @@ static unsigned int pmc_flag;
#define PMC_SLEEP 0x1
#define PMC_DEEP_SLEEP 0x2
+#define PMC_LOSSLESS 0x4
+
+/**
+ * mpc85xx_pmc_set_wake - enable devices as wakeup event source
+ * @pdev: platform device affected
+ * @enable: True to enable event generation; false to disable
+ *
+ * This enables the device as a wakeup event source, or disables it.
+ *
+ * RETURN VALUE:
+ * 0 is returned on success
+ * -EINVAL is returned if device is not supposed to wake up the system
+ * Error code depending on the platform is returned if both the platform
and
+ * the native mechanism fail to enable the generation of wake-up events
+ */
+int mpc85xx_pmc_set_wake(struct platform_device *pdev, bool enable)
Why does it have to be a platform_device? Would a bare device_node work
here? If it's for stuff like device_may_wakeup() that could be in a
platform_device wrapper function.
It does not have to be a platform_device. I think it can be a struct device.
Why does it even need that? The low level mechanism for influencing
PMCDR should only need a device node, not a Linux device struct.
Where does this get called from? I don't see an example user in this
patchset.
It will be used by a gianfar related patch. I plan to submit that patch
after these patches accepted.
It would be nice to see how this is used when reviewing this.
+{
+ int ret = 0;
+ struct device_node *clk_np;
+ u32 *prop;
+ u32 pmcdr_mask;
+
+ if (!pmc_regs) {
+ pr_err(%s: PMC is unavailable\n, __func__);
+ return -ENODEV;
+ }
+
+ if (enable !device_may_wakeup(pdev-dev))
+ return -EINVAL;
Who is setting can_wakeup for these devices?
The device driver is responsible to set can_wakeup.
How would the device driver know how to set it? Wouldn't this depend on
the particular SoC and low power mode?
-Scott
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH v5 4/5] fsl_pmc: Add API to enable device as wakeup event source
-Original Message-
From: Wood Scott-B07421
Sent: Tuesday, June 05, 2012 7:03 AM
To: Zhao Chenhui-B35336
Cc: linuxppc-dev@lists.ozlabs.org; linux-ker...@vger.kernel.org;
ga...@kernel.crashing.org; Li Yang-R58472
Subject: Re: [PATCH v5 4/5] fsl_pmc: Add API to enable device as wakeup
event source
On 06/04/2012 06:36 AM, Zhao Chenhui wrote:
On Fri, Jun 01, 2012 at 05:08:52PM -0500, Scott Wood wrote:
On 05/11/2012 06:53 AM, Zhao Chenhui wrote:
Add APIs for setting wakeup source and lossless Ethernet in low power
modes.
These APIs can be used by wake-on-packet feature.
Signed-off-by: Dave Liu dave...@freescale.com
Signed-off-by: Li Yang le...@freescale.com
Signed-off-by: Jin Qing b24...@freescale.com
Signed-off-by: Zhao Chenhui chenhui.z...@freescale.com
---
arch/powerpc/sysdev/fsl_pmc.c | 71
-
arch/powerpc/sysdev/fsl_soc.h |9 +
2 files changed, 79 insertions(+), 1 deletions(-)
diff --git a/arch/powerpc/sysdev/fsl_pmc.c
b/arch/powerpc/sysdev/fsl_pmc.c
index 1dc6e9e..c1170f7 100644
--- a/arch/powerpc/sysdev/fsl_pmc.c
+++ b/arch/powerpc/sysdev/fsl_pmc.c
@@ -34,6 +34,7 @@ struct pmc_regs {
__be32 powmgtcsr;
#define POWMGTCSR_SLP0x0002
#define POWMGTCSR_DPSLP 0x0010
+#define POWMGTCSR_LOSSLESS 0x0040
__be32 res3[2];
__be32 pmcdr;
};
@@ -43,6 +44,74 @@ static unsigned int pmc_flag;
#define PMC_SLEEP0x1
#define PMC_DEEP_SLEEP 0x2
+#define PMC_LOSSLESS 0x4
+
+/**
+ * mpc85xx_pmc_set_wake - enable devices as wakeup event source
+ * @pdev: platform device affected
+ * @enable: True to enable event generation; false to disable
+ *
+ * This enables the device as a wakeup event source, or disables it.
+ *
+ * RETURN VALUE:
+ * 0 is returned on success
+ * -EINVAL is returned if device is not supposed to wake up the
system
+ * Error code depending on the platform is returned if both the
platform and
+ * the native mechanism fail to enable the generation of wake-up
events
+ */
+int mpc85xx_pmc_set_wake(struct platform_device *pdev, bool enable)
Why does it have to be a platform_device? Would a bare device_node
work
here? If it's for stuff like device_may_wakeup() that could be in a
platform_device wrapper function.
It does not have to be a platform_device. I think it can be a struct
device.
Why does it even need that? The low level mechanism for influencing
PMCDR should only need a device node, not a Linux device struct.
It does no harm to pass the device structure and makes more sense for object
oriented interface design.
Where does this get called from? I don't see an example user in this
patchset.
It will be used by a gianfar related patch. I plan to submit that patch
after these patches accepted.
It would be nice to see how this is used when reviewing this.
+{
+ int ret = 0;
+ struct device_node *clk_np;
+ u32 *prop;
+ u32 pmcdr_mask;
+
+ if (!pmc_regs) {
+ pr_err(%s: PMC is unavailable\n, __func__);
+ return -ENODEV;
+ }
+
+ if (enable !device_may_wakeup(pdev-dev))
+ return -EINVAL;
Who is setting can_wakeup for these devices?
The device driver is responsible to set can_wakeup.
How would the device driver know how to set it? Wouldn't this depend on
the particular SoC and low power mode?
It is based on the fsl,magic-packet and fsl,wake-on-filer device tree
properties.
Leo
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v5 4/5] fsl_pmc: Add API to enable device as wakeup event source
On 05/11/2012 06:53 AM, Zhao Chenhui wrote:
Add APIs for setting wakeup source and lossless Ethernet in low power modes.
These APIs can be used by wake-on-packet feature.
Signed-off-by: Dave Liu dave...@freescale.com
Signed-off-by: Li Yang le...@freescale.com
Signed-off-by: Jin Qing b24...@freescale.com
Signed-off-by: Zhao Chenhui chenhui.z...@freescale.com
---
arch/powerpc/sysdev/fsl_pmc.c | 71
-
arch/powerpc/sysdev/fsl_soc.h |9 +
2 files changed, 79 insertions(+), 1 deletions(-)
diff --git a/arch/powerpc/sysdev/fsl_pmc.c b/arch/powerpc/sysdev/fsl_pmc.c
index 1dc6e9e..c1170f7 100644
--- a/arch/powerpc/sysdev/fsl_pmc.c
+++ b/arch/powerpc/sysdev/fsl_pmc.c
@@ -34,6 +34,7 @@ struct pmc_regs {
__be32 powmgtcsr;
#define POWMGTCSR_SLP0x0002
#define POWMGTCSR_DPSLP 0x0010
+#define POWMGTCSR_LOSSLESS 0x0040
__be32 res3[2];
__be32 pmcdr;
};
@@ -43,6 +44,74 @@ static unsigned int pmc_flag;
#define PMC_SLEEP0x1
#define PMC_DEEP_SLEEP 0x2
+#define PMC_LOSSLESS 0x4
+
+/**
+ * mpc85xx_pmc_set_wake - enable devices as wakeup event source
+ * @pdev: platform device affected
+ * @enable: True to enable event generation; false to disable
+ *
+ * This enables the device as a wakeup event source, or disables it.
+ *
+ * RETURN VALUE:
+ * 0 is returned on success
+ * -EINVAL is returned if device is not supposed to wake up the system
+ * Error code depending on the platform is returned if both the platform and
+ * the native mechanism fail to enable the generation of wake-up events
+ */
+int mpc85xx_pmc_set_wake(struct platform_device *pdev, bool enable)
Why does it have to be a platform_device? Would a bare device_node work
here? If it's for stuff like device_may_wakeup() that could be in a
platform_device wrapper function.
Where does this get called from? I don't see an example user in this
patchset.
+{
+ int ret = 0;
+ struct device_node *clk_np;
+ u32 *prop;
+ u32 pmcdr_mask;
+
+ if (!pmc_regs) {
+ pr_err(%s: PMC is unavailable\n, __func__);
+ return -ENODEV;
+ }
+
+ if (enable !device_may_wakeup(pdev-dev))
+ return -EINVAL;
Who is setting can_wakeup for these devices?
+ clk_np = of_parse_phandle(pdev-dev.of_node, fsl,pmc-handle, 0);
+ if (!clk_np)
+ return -EINVAL;
+
+ prop = (u32 *)of_get_property(clk_np, fsl,pmcdr-mask, NULL);
Don't cast the const away.
+ if (!prop) {
+ ret = -EINVAL;
+ goto out;
+ }
+ pmcdr_mask = be32_to_cpup(prop);
+
+ if (enable)
+ /* clear to enable clock in low power mode */
+ clrbits32(pmc_regs-pmcdr, pmcdr_mask);
+ else
+ setbits32(pmc_regs-pmcdr, pmcdr_mask);
What is the default PMCDR if this function is never called? Should init
to all bits set on PM driver probe (or maybe limit it to defined bits
only, though that's a little harder to do generically).
-Scot
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
