From: Sudeep Holla <sudeep.holla@arm.com>
To: "Prakash, Prashanth" <pprakash@codeaurora.org>,
linux-acpi@vger.kernel.org,
"Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Sudeep Holla <sudeep.holla@arm.com>,
linux-kernel@vger.kernel.org,
Vikas Sajjan <vikas.cha.sajjan@hpe.com>, Sunil <sunil.vl@hpe.com>,
Ashwin Chaugule <ashwin.chaugule@linaro.org>,
Al Stone <al.stone@linaro.org>,
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Subject: Re: [PATCH v5 2/5] ACPI / processor_idle: Add support for Low Power Idle(LPI) states
Date: Thu, 19 May 2016 14:26:29 +0100 [thread overview]
Message-ID: <573DBF05.1090701@arm.com> (raw)
In-Reply-To: <573CBEC3.4040506@codeaurora.org>
On 18/05/16 20:13, Prakash, Prashanth wrote:
>
>
> On 5/18/2016 11:37 AM, Sudeep Holla wrote:
>>
>>
>> On 17/05/16 18:46, Prakash, Prashanth wrote:
>>> Hi Sudeep,
>>>
>>> On 5/11/2016 9:37 AM, Sudeep Holla wrote:
>>>> +
>>>> +static int acpi_processor_get_lpi_info(struct acpi_processor *pr)
>>>> +{
>>>> + int ret, i;
>>>> + struct acpi_lpi_states_array *info;
>>>> + struct acpi_device *d = NULL;
>>>> + acpi_handle handle = pr->handle, pr_ahandle;
>>>> + acpi_status status;
>>>> +
>>>> + if (!osc_pc_lpi_support_confirmed)
>>>> + return -EOPNOTSUPP;
>>>> +
>>>> + max_leaf_depth = 0;
>>>> + if (!acpi_has_method(handle, "_LPI"))
>>>> + return -EINVAL;
>>>> + flat_state_cnt = 0;
>>>> +
>>>> + while (ACPI_SUCCESS(status = acpi_get_parent(handle, &pr_ahandle))) {
>>>> + if (!acpi_has_method(handle, "_LPI"))
>>>> + continue;
>>>> +
>>>> + acpi_bus_get_device(handle, &d);
>>>> + if (!strcmp(acpi_device_hid(d), ACPI_PROCESSOR_CONTAINER_HID))
>>>> + break;
>>>> +
>>>> + max_leaf_depth++;
>>>> + handle = pr_ahandle;
>>>> + }
>>>> +
>>> In the above loop, we break when we find a device with HID ==
>>> ACPI_PROCESSOR_CONTAINER_HID. Shouldn't we continue to parse as long as the
>>> parent HID == ACPI_PROCESSOR_CONTAINER_HID? This is required to make sure we
>>> parse states in levels higher than cluster level in processor hierarchy.
>>>
>>
>> Yes, thanks for pointing that out. With just clusters in _LPI on my dev
>> board, I missed it.
>>
> Same reason, I failed to notice it all this time :)
No worries.
>>> Also, I think it might be safe to break out of the loop if we didn't find
>>> _LPI package, instead of continuing. Given the presence of LPI entry:
>>> "Enabled Parent State", I can't think of a non-ambiguous scenario where we
>>> might find LPI packages in state N and N+2, but not in N+1, as we will not
>>> be able to figure out which state in N enables which states in N+2.
>>> Thoughts?
>>
>> Though I admit I haven't thought in detail on how to deal with the
>> asymmetric topology, but that was the reason why I continue instead of
>> breaking.
>>
>> Excerpts from the spec: "... This example is symmetric but that is not a
>> requirement. For example, a system may contain a different number of
>> processors in different containers or an asymmetric hierarchy where one
>> side of the topology tree is deeper than another...."
>>
> If it addresses asymmetric topology, sure we can keep as it doesn't impact other
> scenarios. Also, we need to set handle=pr_ahandle prior to the continue statement.
>
Yes I noticed it yesterday, the more I think, I feel we can break out of
the loop. At any level, we need to have container nodes and that must
contain _LPI irrespective of asymmetricity. So you were right. I have
fixed accordingly and have pushed out on my branch.
--
Regards,
Sudeep
next prev parent reply other threads:[~2016-05-19 13:26 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-11 15:37 [PATCH v5 0/5] ACPI / processor_idle: Add ACPI v6.0 LPI support Sudeep Holla
2016-05-11 15:37 ` [PATCH v5 1/5] ACPI / processor_idle: introduce ACPI_PROCESSOR_CSTATE Sudeep Holla
2016-05-11 15:37 ` Sudeep Holla
2016-05-11 16:23 ` Rafael J. Wysocki
2016-05-11 16:23 ` Rafael J. Wysocki
2016-05-11 16:57 ` Sudeep Holla
2016-05-11 16:57 ` Sudeep Holla
[not found] ` <CAJvTdKnJPZ9Nfib=CqBczMP4BERqfqAzeSR-+jjFOGZR51oVmg@mail.gmail.com>
2016-05-11 18:28 ` Len Brown
2016-05-11 18:28 ` Len Brown
2016-05-12 9:10 ` Sudeep Holla
2016-05-12 9:10 ` Sudeep Holla
2016-05-12 13:21 ` [UPDATE][PATCH v5] " Sudeep Holla
2016-05-11 15:37 ` [PATCH v5 2/5] ACPI / processor_idle: Add support for Low Power Idle(LPI) states Sudeep Holla
2016-05-17 17:46 ` Prakash, Prashanth
2016-05-18 17:37 ` Sudeep Holla
2016-05-18 19:13 ` Prakash, Prashanth
2016-05-19 13:26 ` Sudeep Holla [this message]
2016-06-10 17:38 ` Sudeep Holla
2016-06-13 21:05 ` Rafael J. Wysocki
2016-06-14 14:24 ` Sudeep Holla
2016-05-11 15:37 ` [PATCH v5 3/5] arm64: cpuidle: drop __init section marker to arm_cpuidle_init Sudeep Holla
2016-05-11 15:37 ` Sudeep Holla
2016-05-11 15:37 ` [PATCH v5 4/5] arm64: add support for ACPI Low Power Idle(LPI) Sudeep Holla
2016-05-11 15:37 ` Sudeep Holla
2016-06-10 12:50 ` Lorenzo Pieralisi
2016-06-10 12:50 ` Lorenzo Pieralisi
2016-06-13 16:27 ` Daniel Lezcano
2016-06-13 16:27 ` Daniel Lezcano
2016-06-13 4:47 ` Sajjan, Vikas C
2016-06-13 4:47 ` Sajjan, Vikas C
2016-06-13 9:40 ` Sudeep Holla
2016-06-13 9:40 ` Sudeep Holla
2016-05-11 15:37 ` [PATCH v5 5/5] ACPI : enable ACPI_PROCESSOR_IDLE on ARM64 Sudeep Holla
2016-05-11 15:37 ` Sudeep Holla
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=573DBF05.1090701@arm.com \
--to=sudeep.holla@arm.com \
--cc=al.stone@linaro.org \
--cc=ashwin.chaugule@linaro.org \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=lorenzo.pieralisi@arm.com \
--cc=pprakash@codeaurora.org \
--cc=rjw@rjwysocki.net \
--cc=sunil.vl@hpe.com \
--cc=vikas.cha.sajjan@hpe.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.