Shaohua Li wrote:
On Fri, Jun 26, 2009 at 10:59:20AM +0800, Kenji Kaneshige wrote:
Shaohua Li wrote:
On Fri, Jun 19, 2009 at 01:28:44PM +0800, Kenji Kaneshige wrote:
Shaohua Li wrote:
On Fri, Jun 19, 2009 at 11:19:49AM +0800, Kenji Kaneshige wrote:
Shaohua Li wrote:
On Fri, Jun 19, 2009 at 05:28:41AM +0800, Jesse Barnes wrote:
On Thu, 21 May 2009 11:10:06 +0900
Kenji Kaneshige <kaneshige.kenji@xxxxxxxxxxxxxx> wrote:
Hi,
This patch changes ASPM driver to enable more detailed ASPM
configuration. With this patch,
- ASPM can be enabled partially in the hierarchy.
- L0s can be managed for each direction (upstream direction and
downstream direction) of the link.
Well, these are nice features, but what kind of machine does you test?
Does it have several level hierarchy? The most powerful system I saw
just has two levels, which sounds not worth adding partial aspm.
Thank you very much for comments.
I think it is useful also for the system that doesn't have so
many levels of hierarchy, such as mobile systems. Here is "lspci -t"
output on my environment with some comments. My system also has
just two levels, but with the patch, upstream L0s is enabled on the
link 0000:15.00.0 (switch downstream port) to 0000:16:00.0 (endpoint)
in powersave mode.
Can you send me the output of lspci -vvvxxx please?
Here it is. Please note that I'm using pcie_aspm=force option.
Hi,
can you please split the patch into two patches, then I will have more detail
review/test. Also please don't do cleanup in the patches for new feature.
I had a simple look at the patch, and found at least one issue:
A link might have multiple child links, we need check all child links's latency to
enable the parent's ASPM. In pcie_config_aspm_path(), the patch seems not check
peer link.
My understanding is that we can enable parent ASPM even if one
or more child links don't meet the latency requirement and ASPM
is disabled on them. In this case, parent link will never enter
the upstream direction L0s nor L1 state because parent link's
downstream component (switch upstream port) never becomes idle.
I thought we need to check other links, considering below case:
Say we have three links, parent/child1/child2. When you do partial aspm
enabling, endpoints in child1 have latency requirement and the result is child1
is enabled and parent is disabled. The we check child2, and latency requirement
meets and then you enable child2 and parent. This will makes endpoints in link1
hasn't required latency and breaks them.
In this case, my patch doesn't enable ASPM on parent the link.
My patch is doing as follows:
1) Detect parent link and initialize 'aspm_support'(*1) and
'aspm_capable'(*1). At this point 'aspm_capable' is equal to
'aspm_support'(*2) because no endpoints are detected at this
point. And enable ASPM based on 'aspm_capable' on parent.
The states are as follows at this point (note that all ASPM
states are mixed up in this example).
parent: Support+, Capable+, Enabled+
child1: N/A , N/A , N/A
child2: N/A , N/A , N/A
(*1): Each bits in 'aspm_capable' is associated to upstream and
downstream direction L0s and L1. The bit is set if the link
supports associated ASPM state and meets latency requirement.
(*2): Each bits in 'aspm_support' is associated to upstream and
downstream direction L0s and L1. The bit is set if the link
supports associated ASPM state.
2) Detect child1 link and initialize 'aspm_support' and 'aspm_capable'.
First, 'aspm_capable' is initialized by 'aspm_support' and then
updated based on endpoint's acceptable latency. The parent's
'aspm_capable' is also updated here. In this update, bits can be
unset, but never be set. In your example, 'aspm_capable'
is unset because it doesn't meets latency requirement. And then
configure ASPM based on 'aspm_capable' on child1 and parent.
The states are as follows at this point.
parent: Support+, Capable-, Enabled-
child1: Support+, Capable+, Enabled+
child2: N/A , N/A , N/A
3) Detect child2 link and initialize 'aspm_support' and 'aspm_capable'.
First, 'aspm_capable' is initialized by 'aspm_support' and then
updated based on endpoint's acceptable latency. The parent's
'aspm_capable' is also updated here, but bits are never set in this
update. In your example, parent meets latency requirement for
endpoint under child2 link, but 'aspm_capable' of parent is never
changed. And then configure ASPM based on 'aspm_capable' on child2
and parent.
The states are as follows at the end.
parent: Support+, Capable-, Enabled-
child1: Support+, Capable+, Enabled+
child2: Support+, Capable+, Enabled+
Thanks,
Kenji Kaneshige
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
