Re: [PATCH v5 4/5] fsl_pmc: Add API to enable device as wakeup event source

From: Li Yang <hidden>
Date: 2012-06-06 04:06:25
Also in: lkml

On Wed, Jun 6, 2012 at 2:05 AM, Scott Wood [off-list ref] wrote:

On 06/05/2012 11:49 AM, Li Yang-R58472 wrote:

quoted

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

+int mpc85xx_pmc_set_wake(struct platform_device *pdev, bool
+enable)

Why does it have to be a platform_device? =C2=A0Would a bare device=

_node

quoted
quoted
quoted
quoted
work
quoted
quoted
here? =C2=A0If it's for stuff like device_may_wakeup() that could b=

e in

quoted

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? =C2=A0The low level mechanism for influen=

cing

quoted

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 so=

me

quoted

reason you found the device by directly looking for it rather than goin=

quoted

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. =C2=A0I 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. =C2=

=A0I

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.

quoted

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? =C2=A0Wouldn't this d=

epend

quoted

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. =C2=A0It is equivalent to checking the
compatible for etsec. =C2=A0It does not convey any information about wh=

ether

quoted

It can be described either by explicit feature property or by the
compatible. =C2=A0I don't think it is a problem that we choose one again=

st

quoted

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). =C2=A0But my point wa=

that it says nothing about whether the eTSEC will still be functioning
during a low power state.

quoted

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.

quoted

If it behaves differently for low power modes in the future, we could
address that by adding additional property.

quoted

How is fsl,wake-os-filer relevant to this decision? =C2=A0When will it =

be set

quoted

but not fsl,magic-packet?

I mean either fsl,magic-packet or fsl,wake-on-filer shows it can be a
wakeup source. =C2=A0Currently we don't have an SoC to have wake-on-file=

quoted

while not wake-on-magic. =C2=A0But I think it's better to consider both =

as

quoted

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.

quoted

What about devices other than ethernet? =C2=A0What about differences be=

tween

quoted

ordinary sleep and deep sleep?

There is no difference for sleep and deep sleep for all wakeup sources c=

urrently.

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). =C2=A0You 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 least for now.

- Leo

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help