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-kernel@vger.kernel.org;
galak@kernel.crashing.org; Li Yang-R58472
Subject: Re: [PATCH v5 4/5] fsl_pmc: Add API to enable device as wakeup
event source
=20
On 06/04/2012 06:36 AM, Zhao Chenhui wrote:

quoted

On Fri, Jun 01, 2012 at 05:08:52PM -0500, Scott Wood wrote:

quoted

On 05/11/2012 06:53 AM, Zhao Chenhui wrote:

quoted

Add APIs for setting wakeup source and lossless Ethernet in low power

modes.

quoted

quoted

quoted

These APIs can be used by wake-on-packet feature.

Signed-off-by: Dave Liu <redacted>
Signed-off-by: Li Yang <redacted>
Signed-off-by: Jin Qing <redacted>
Signed-off-by: Zhao Chenhui <redacted>
---
 arch/powerpc/sysdev/fsl_pmc.c |   71

++++++++++++++++++++++++++++++++++++++++-

quoted

quoted

quoted

 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

quoted

quoted

quoted

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		0x00020000
 #define POWMGTCSR_DPSLP		0x00100000
+#define POWMGTCSR_LOSSLESS	0x00400000
 	__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

quoted

quoted

quoted

+ * Error code depending on the platform is returned if both the

platform and

quoted

quoted

quoted

+ * the native mechanism fail to enable the generation of wake-up

events

quoted

quoted

quoted

+ */
+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

quoted

quoted

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.
=20
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 objec=
t oriented interface design.=20

=20

quoted

quoted

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.

=20
It would be nice to see how this is used when reviewing this.
=20

quoted

quoted

quoted

+{
+	int ret =3D 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.

=20
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 p=
roperties.

Leo