From patchwork Fri Jun 19 03:19:49 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kenji Kaneshige X-Patchwork-Id: 31285 Received: from vger.kernel.org (vger.kernel.org [209.132.176.167]) by demeter.kernel.org (8.14.2/8.14.2) with ESMTP id n5J3KGXl000706 for ; Fri, 19 Jun 2009 03:20:17 GMT Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752205AbZFSDUN (ORCPT ); Thu, 18 Jun 2009 23:20:13 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1752781AbZFSDUM (ORCPT ); Thu, 18 Jun 2009 23:20:12 -0400 Received: from fgwmail7.fujitsu.co.jp ([192.51.44.37]:53378 "EHLO fgwmail7.fujitsu.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752205AbZFSDUL (ORCPT ); Thu, 18 Jun 2009 23:20:11 -0400 Received: from m1.gw.fujitsu.co.jp ([10.0.50.71]) by fgwmail7.fujitsu.co.jp (Fujitsu Gateway) with ESMTP id n5J3KBpM005113 for (envelope-from kaneshige.kenji@jp.fujitsu.com); Fri, 19 Jun 2009 12:20:12 +0900 Received: from smail (m1 [127.0.0.1]) by outgoing.m1.gw.fujitsu.co.jp (Postfix) with ESMTP id 834E845DD71 for ; Fri, 19 Jun 2009 12:20:11 +0900 (JST) Received: from s1.gw.fujitsu.co.jp (s1.gw.fujitsu.co.jp [10.0.50.91]) by m1.gw.fujitsu.co.jp (Postfix) with ESMTP id 4322545DE54 for ; Fri, 19 Jun 2009 12:20:11 +0900 (JST) Received: from s1.gw.fujitsu.co.jp (localhost.localdomain [127.0.0.1]) by s1.gw.fujitsu.co.jp (Postfix) with ESMTP id 0DACF1DB8041 for ; Fri, 19 Jun 2009 12:20:11 +0900 (JST) Received: from ml13.s.css.fujitsu.com (ml13.s.css.fujitsu.com [10.249.87.103]) by s1.gw.fujitsu.co.jp (Postfix) with ESMTP id 8E590E08008 for ; Fri, 19 Jun 2009 12:20:10 +0900 (JST) Received: from ml13.css.fujitsu.com (ml13 [127.0.0.1]) by ml13.s.css.fujitsu.com (Postfix) with ESMTP id 61950FD000B; Fri, 19 Jun 2009 12:20:10 +0900 (JST) Received: from [127.0.0.1] (unknown [10.124.100.137]) by ml13.s.css.fujitsu.com (Postfix) with ESMTP id A00B6FD0004; Fri, 19 Jun 2009 12:20:09 +0900 (JST) Message-ID: <4A3B03D5.7090200@jp.fujitsu.com> Date: Fri, 19 Jun 2009 12:19:49 +0900 From: Kenji Kaneshige User-Agent: Thunderbird 2.0.0.21 (Windows/20090302) MIME-Version: 1.0 To: Shaohua Li CC: Jesse Barnes , "linux-pci@vger.kernel.org" , "Zhang, Yanmin" Subject: Re: [PATCH] PCI ASPM: more detailed ASPM configuration References: <4A14B7FE.5030206@jp.fujitsu.com> <20090618142841.37d0f3c5@jbarnes-g45> <20090619010200.GA31027@sli10-desk.sh.intel.com> In-Reply-To: <20090619010200.GA31027@sli10-desk.sh.intel.com> Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org 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 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. [kanesige@localhost develop]$ /sbin/lspci -t -[0000:00]-+-00.0 +-01.0-[0000:04-13]-- L0s(UP) enabled L0s(DW) enabled(*) +-02.0-[0000:14-29]--+-00.0-[0000:15-28]--+-00.0-[0000:16]----00.0 | | +-01.0-[0000:18-27]-- L0s(DW) enabled(*) | | \-02.0-[0000:28]--+-00.0 | | \-00.1 | \-00.3-[0000:29]-- +-03.0-[0000:2a-2d]----00.0-[0000:2b-2d]--+-02.0-[0000:2c]--+-00.0 | | \-00.1 L0s(DW) enabled(*) | \-04.0-[0000:2d]----00.0 +-04.0-[0000:2e-4f]----00.0-[0000:2f-4f]--+-02.0-[0000:30-3f]--+-00.0 | | \-00.1 | \-04.0-[0000:40-4f]--+-00.0 | \-00.1 +-06.0-[0000:51-73]----00.0-[0000:52-73]--+-02.0-[0000:54-63]--+-00.0 | | \-00.1 | \-04.0-[0000:64-73]----00.0-[0000:65]--+-08.0 | \-08.1 +-10.0 +-10.1 +-10.2 +-10.3 +-11.0 +-11.3 +-13.0 +-15.0 +-16.0 +-1c.0-[0000:75-83]-- +-1d.0 +-1d.1 +-1d.2 +-1d.3 +-1d.7 +-1e.0-[0000:84]-- +-1f.0 +-1f.2 \-1f.3 (*) Downstream direction L0s is enabled but it has no effect because Downstream direction L0s is disabled on its upstream links. > The second feature seems more reasonable. I think I should have implement those features as separate patches. I'll do it if needed. > ASPM is still broken in a lot of devices, before we go far, we need to make > sure the feature is really required. I tested that pci_disable_link_state() first, actually :) By the way, I've tested the patches with debugging patch that give us the information like below through /proc. I'm attaching it for your information. LINK (addr: ffff88083c5fc840, pdev: ffff88083c604240, name: 0000:00:02.0) root: ffff88083c5fc840, parent (null) ASPM enabled: L0sUP-, L0sDW-, L1- ASPM capable: L0sUP-, L0sDW+, L1- ASPM support: L0sUP+, L0sDW+, L1- ASPM disable: L0sUP+, L0sDW+, L1+ ASPM default: L0sUP-, L0sDW-, L1- ASPM latency: L0sUP 5000, L0sDW 256, L1 65000 ClockPM: capable 0, enabled 0, default 0 LINK (addr: ffff88083c5fcb58, pdev: ffff88083c572968, name: 0000:15:02.0) root: ffff88083c5fc840, parent ffff88083c5fc840 ASPM enabled: L0sUP-, L0sDW+, L1- ASPM capable: L0sUP-, L0sDW+, L1- ASPM support: L0sUP-, L0sDW+, L1- ASPM disable: L0sUP-, L0sDW-, L1- ASPM default: L0sUP-, L0sDW-, L1- ASPM latency: L0sUP 256, L0sDW 128, L1 65000 ClockPM: capable 0, enabled 0, default 0 EP[0000:28:00.0] accept l0s 512, l1 64000 EP[0000:28:00.1] accept l0s 512, l1 64000 LINK (addr: ffff88083c5fca50, pdev: ffff88083c5718d8, name: 0000:15:00.0) root: ffff88083c5fc840, parent ffff88083c5fc840 ASPM enabled: L0sUP+, L0sDW+, L1- ASPM capable: L0sUP+, L0sDW+, L1- ASPM support: L0sUP+, L0sDW+, L1- ASPM disable: L0sUP-, L0sDW-, L1- ASPM default: L0sUP-, L0sDW-, L1- ASPM latency: L0sUP 5000, L0sDW 2048, L1 65000 ClockPM: capable 0, enabled 0, default 0 EP[0000:16:00.0] accept l0s 4294967295, l1 4294967295 Thanks, Kenji Kaneshige drivers/pci/pcie/aspm.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 113 insertions(+) --- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Index: 20090521/drivers/pci/pcie/aspm.c =================================================================== --- 20090521.orig/drivers/pci/pcie/aspm.c +++ 20090521/drivers/pci/pcie/aspm.c @@ -19,6 +19,8 @@ #include #include #include +#include +#include #include "../pci.h" #ifdef MODULE_PARAM_PREFIX @@ -859,6 +861,117 @@ void pcie_aspm_remove_sysfs_dev_files(st sysfs_remove_file_from_group(&pdev->dev.kobj, &dev_attr_clk_ctl.attr, power_group); } + +static void show_link(struct seq_file *p, + struct pcie_link_state *link, int depth) +{ + struct pci_dev *child, *parent = link->pdev; + struct pci_bus *linkbus = parent->subordinate; + struct pcie_link_state *childlink; + int indent = depth * 4; + + seq_printf(p, + "%*sLINK (addr: %p, pdev: %p, name: %s)\n" + "%*s root: %p, parent %p\n" + "%*s ASPM enabled: L0sUP%c, L0sDW%c, L1%c\n" + "%*s ASPM capable: L0sUP%c, L0sDW%c, L1%c\n" + "%*s ASPM support: L0sUP%c, L0sDW%c, L1%c\n" + "%*s ASPM disable: L0sUP%c, L0sDW%c, L1%c\n" + "%*s ASPM default: L0sUP%c, L0sDW%c, L1%c\n" + "%*s ASPM latency: L0sUP %u, L0sDW %u, L1 %u\n" + "%*s ClockPM: capable %u, enabled %u, default %u\n", + indent, "", link, parent, pci_name(parent), + indent, "", link->root, link->parent, + indent, "", + (link->aspm_enabled & ASPM_STATE_L0S_UP) ? '+' : '-', + (link->aspm_enabled & ASPM_STATE_L0S_DW) ? '+' : '-', + (link->aspm_enabled & ASPM_STATE_L1) ? '+' : '-', + indent, "", + (link->aspm_capable & ASPM_STATE_L0S_UP) ? '+' : '-', + (link->aspm_capable & ASPM_STATE_L0S_DW) ? '+' : '-', + (link->aspm_capable & ASPM_STATE_L1) ? '+' : '-', + indent, "", + (link->aspm_support & ASPM_STATE_L0S_UP) ? '+' : '-', + (link->aspm_support & ASPM_STATE_L0S_DW) ? '+' : '-', + (link->aspm_support & ASPM_STATE_L1) ? '+' : '-', + indent, "", + (link->aspm_disable & ASPM_STATE_L0S_UP) ? '+' : '-', + (link->aspm_disable & ASPM_STATE_L0S_DW) ? '+' : '-', + (link->aspm_disable & ASPM_STATE_L1) ? '+' : '-', + indent, "", + (link->aspm_default & ASPM_STATE_L0S_UP) ? '+' : '-', + (link->aspm_default & ASPM_STATE_L0S_DW) ? '+' : '-', + (link->aspm_default & ASPM_STATE_L1) ? '+' : '-', + indent, "", + link->latency_up.l0s, link->latency_dw.l0s, + max_t(u32, link->latency_up.l1, link->latency_dw.l1), + indent, "", link->clkpm_capable, + link->clkpm_enabled, link->clkpm_default); + list_for_each_entry(child, &linkbus->devices, bus_list) { + int func = PCI_FUNC(child->devfn); + if ((child->pcie_type != PCI_EXP_TYPE_ENDPOINT) && + (child->pcie_type != PCI_EXP_TYPE_LEG_END)) + continue; + seq_printf(p, "%*s EP[%s] accept l0s %u, l1 %u\n", + indent, "", pci_name(child), + link->acceptable[func].l0s, + link->acceptable[func].l1); + } + seq_printf(p, "\n"); + + list_for_each_entry(childlink, &link->children, link) + show_link(p, childlink, (depth + 1)); +} + +static int show_pcie_link_state(struct seq_file *p, void *v) +{ + struct pcie_link_state *link; + + mutex_lock(&aspm_lock); + list_for_each_entry(link, &link_list, sibling) { + if (!link->parent) + show_link(p, link, 0); + } + mutex_unlock(&aspm_lock); + return 0; +} + +static int pcie_link_state_open(struct inode *inode, struct file *file) +{ + size_t size = 128 * 1024; + char *buf; + int res; + struct seq_file *m; + + buf = kmalloc(size, GFP_KERNEL); + if (!buf) + return -ENOMEM; + + res = single_open(file, show_pcie_link_state, NULL); + if (!res) { + m = file->private_data; + m->buf = buf; + m->size = size; + } else + kfree(buf); + return res; +} + +static const struct file_operations pcie_link_state_operations = { + .open = pcie_link_state_open, + .read = seq_read, + .llseek = seq_lseek, + .release = single_release, +}; + +static int pcie_aspm_init_proc(void) +{ + if (aspm_disabled) + return 0; + proc_create("pcie_link_state", 0, NULL, &pcie_link_state_operations); + return 0; +} +late_initcall(pcie_aspm_init_proc); #endif static int __init pcie_aspm_disable(char *str)