lkml.org 
[lkml]   [2012]   [Feb]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [RFC PATCH 1/6] ACPI: Introduce ACPI D3_COLD state support
    Date
    On Tuesday, February 14, 2012, Zhang Rui wrote:
    > Hi, Rafael,
    >
    > On 一, 2012-02-13 at 21:25 +0100, Rafael J. Wysocki wrote:
    > > On Monday, February 13, 2012, Lin Ming wrote:
    > > > From: Zhang Rui <rui.zhang@intel.com>
    > > >
    > > > If a device has _PR3._ON, it means the device supports D3_HOT.
    > > > If a device has _PR3._OFF, it means the device supports D3_COLD.
    > > > Add the ability to validate and enter D3_COLD state in ACPI.
    > > >
    > > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
    > >
    > > This is supposed to be ACPI 5.0 support, right?
    > >
    > No, D3_HOT is introduced in ACPI spec 4.0.
    > According to the spec, _PR3 is used for devices that support both
    > D3(D3_COLD) and D3HOT.

    Yes, it does.

    > The confusion here is that Linux D3 equals ACPICA D3HOT and Linux
    > D3_COLD equals ACPICA D3.
    > For example, when enter Linux ACPI D3, the reference count of ACPI Power
    > Resources in _PR3 is increased by one.

    That's correct.

    > > So can anyone please tell me what part of the ACPI 5.0 spec is the
    > > basis of this patch, because I can't see that immediately?
    > >
    > > The only places where D3Cold is _mentioned_ are Section 7.2.12 (_PRE, which
    > > appears to be new in 5.0), Section 7.2.20 (_S0W), Section 7.2.21 (_S1W),
    > > Section 7.2.22 (_S2W), Section 7.2.23 (_S3W) and Section 7.2.24 (_S4W).
    > > None of them mentions those _PR3._ON and _PR3._OFF things above.
    > >
    > > Moreover, my understanding of the spec is that D3Cold means all of the
    > > power resources returned by _PR3 are "off" (whereas some of them will be
    > > "on" in D3hot).
    > >
    > Agreed.
    >
    > > > ---
    > > > drivers/acpi/power.c | 4 ++--
    > > > drivers/acpi/scan.c | 10 +++++++++-
    > > > 2 files changed, 11 insertions(+), 3 deletions(-)
    > > >
    > > > diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c
    > > > index 9ac2a9f..0d681fb 100644
    > > > --- a/drivers/acpi/power.c
    > > > +++ b/drivers/acpi/power.c
    > > > @@ -500,14 +500,14 @@ int acpi_power_transition(struct acpi_device *device, int state)
    > > > {
    > > > int result;
    > > >
    > > > - if (!device || (state < ACPI_STATE_D0) || (state > ACPI_STATE_D3))
    > > > + if (!device || (state < ACPI_STATE_D0) || (state > ACPI_STATE_D3_COLD))
    > > > return -EINVAL;
    > > >
    > > > if (device->power.state == state)
    > > > return 0;
    > > >
    > > > if ((device->power.state < ACPI_STATE_D0)
    > > > - || (device->power.state > ACPI_STATE_D3))
    > > > + || (device->power.state > ACPI_STATE_D3_COLD))
    > > > return -ENODEV;
    > > >
    > > > /* TBD: Resources must be ordered. */
    > > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
    > > > index 8ab80ba..a9d4391 100644
    > > > --- a/drivers/acpi/scan.c
    > > > +++ b/drivers/acpi/scan.c
    > > > @@ -881,8 +881,16 @@ static int acpi_bus_get_power_flags(struct acpi_device *device)
    > > >
    > > > device->power.flags.power_resources = 1;
    > > > ps->flags.valid = 1;
    > > > - for (j = 0; j < ps->resources.count; j++)
    > > > + for (j = 0; j < ps->resources.count; j++) {
    > > > acpi_bus_add_power_resource(ps->resources.handles[j]);
    > > > + /* Check for D3_COLD support. _PR3._OFF equals D3_COLD ? */
    > > > + if (i == ACPI_STATE_D3) {
    > > > + if (j == 0)
    > > > + device->power.states[ACPI_STATE_D3_COLD].flags.valid = 1;
    > > > + status = acpi_get_handle(ps->resources.handles[j], "_OFF", &handle);
    > > > + device->power.states[ACPI_STATE_D3_COLD].flags.valid &= ACPI_SUCCESS(status);
    > > > + }
    > > > + }
    > >
    > > Sorry, but this doesn't make sense to me. Power resources always have
    > > the _OFF method, right?
    > >
    > I'm not sure.

    That would be explicitly against the spec that says that power resources
    are *required* to have _ON, _OFF and _STA.

    > I thought I had seen ACPI Power Resources without _OFF
    > control method somewhere in bugzilla, but I can not find it out now.

    That, clearly, is a firmware bug.

    > Hmm, how about set D3_COLD support if _PR3 exists, but leave a warning
    > message if _OFF doesn't exist, for now?

    I don't think we need to set D3_COLD support at all. In fact, it is always
    supported (as I said, if all power resources used by a device are off, the
    device is in D3_COLD pretty much by definition).

    Thanks,
    Rafael
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2012-02-14 23:27    [W:0.038 / U:30.556 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site