From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Patel, Mayurkumar" Subject: RE: [PATCH V8 4/5] PCI/ASPM: save power on values during bridge init Date: Fri, 21 Apr 2017 07:46:27 +0000 Message-ID: <92EBB4272BF81E4089A7126EC1E7B2846676C7EF@IRSMSX101.ger.corp.intel.com> References: <1491627351-1111-1-git-send-email-okaya@codeaurora.org> <1491627351-1111-5-git-send-email-okaya@codeaurora.org> <20170414214452.GA21870@bhelgaas-glaptop.roam.corp.google.com> <66168dde-7719-6f74-3f06-8e4724dd2918@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Sinan Kaya , Bjorn Helgaas Cc: Rajat Jain , Myron Stowe , David Daney , "linux-pci@vger.kernel.org" , Timur Tabi , "linux-kernel@vger.kernel.org" , Julia Lawall , Bjorn Helgaas , linux-arm-msm , Rajat Jain , Yinghai Lu , Shawn Lin , linux-arm List-Id: linux-arm-msm@vger.kernel.org Hi Bjorn/Kaya, > >On 4/17/2017 12:38 PM, Bjorn Helgaas wrote: >>> Like you said, what do we do by default is the question. Should we opt >>> for safe like we are doing, or try to save some power. >> I think safety is paramount. Every user should be able to boot safely >> without any kernel parameters. We don't want users to have a problem >> booting and then have to search for a workaround like booting with >> "pcie_aspm=off". Most users will never do that. >> > >OK, no problem with leaving the behavior as it is. > >My initial approach was #2. We knew this way that user had full control >over the ASPM policy by changing the BIOS option. Then, Mayurkumar >complained that ASPM is not enabled following a hotplug insertion to an >empty slot. That's when I switched to #3 as it sounded like a good thing >to have for us. > >> Here's a long-term strawman proposal, see what you think: >> >> - Deprecate CONFIG_PCIEASPM_DEFAULT, CONFIG_PCIEASPM_POWERSAVE, etc. >> - Default aspm_policy is POLICY_DEFAULT always. >> - POLICY_DEFAULT means Linux doesn't touch anything: if BIOS enabled >> ASPM, we leave it that way; we leave ASPM disabled on hot-added >> devices. > I am also ok with leaving the same behavior as now. But still following is something open I feel besides, Which may be there in your comments redundantly. The current problem is, pcie_aspm_exit_link_state() disables the ASPM configuration even if POLICY_DEFAULT was set. I am seeing already following problem(or may be influence) with it. The Endpoint I have does not have does not have "Presence detect change" mechanism. Hot plug is working with Link status events. When link is in L1 or L1SS and if EP is powered off, no Link status change event are triggered (It might be the expected behavior in L1 or L1SS). When next time EP is powered on there are link down and link up events coming one after other. BIOS enables ASPM on Root port and Endpoint, but while processing link status down, pcie_aspm_exit_link_state() clears the ASPM already which were enabled by BIOS. If we want to follow above approach then shall we consider having something similar as following? diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index 1dfa10c..bf5be6d 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -940,7 +940,8 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev) parent_link = link->parent; /* All functions are removed, so just disable ASPM for the link */ - pcie_config_aspm_link(link, 0); + if (aspm_policy != POLICY_DEFAULT) + pcie_config_aspm_link(link, 0); list_del(&link->sibling); list_del(&link->link); /* Clock PM is for endpoint device */ >I can easily see people complaining the other way around. There >could be some boot FW that doesn't know what ASPM is and this particular >system could rely on the compile time option for enabling power options. >Maybe, a command line option will be required for them to keep the existing >behavior. > >> - Deprecate kernel boot parameters (possibly keep pcie_aspm=off for >> debugging use). >> - Use /sys/module/pcie_aspm/parameters/policy for run-time >> system-wide control, including for future hot-added devices. >> - Remove CONFIG_PCIEASPM_DEBUG and enable that code always, so we >> have fine-grained run-time control. >> > >Runtime control sounds like a better plan. We need hooks into the system >power management policy. > >>> Maybe, we are missing a HPP option from the PCI spec. >> That's an interesting idea. _HPX does have provision for manipulating >> Link Control bits (see ACPI r5.0, sec 6.2.8.3), but I don't think very >> many systems implement it. And there's currently no connection >> between program_hpp_type2() and aspm.c, so I'm a little worried that >> we might have issues if a system did implement an _HPX that sets any >> of the ASPM bits. > >I looked at the spec some more. These are there to restore the register >settings following hotplug insertion. I agree it won't play nice with ASPM >as the control bits need to be enabled in coordination with the upstream >device. > >I think the right approach is to let the userspace feed the required >policy to the kernel like you suggested. Then, it needs to be per port >via link_state to have the most flexibility. > > >-- >Sinan Kaya >Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. >Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. Intel Deutschland GmbH Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de Managing Directors: Christin Eisenschmid, Christian Lamprechter Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1036163AbdDUHqq (ORCPT ); Fri, 21 Apr 2017 03:46:46 -0400 Received: from mga07.intel.com ([134.134.136.100]:51013 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1036145AbdDUHqh (ORCPT ); Fri, 21 Apr 2017 03:46:37 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.37,229,1488873600"; d="scan'208";a="77126009" From: "Patel, Mayurkumar" To: Sinan Kaya , Bjorn Helgaas CC: Bjorn Helgaas , Rajat Jain , "Rajat Jain" , David Daney , "linux-pci@vger.kernel.org" , Timur Tabi , "linux-kernel@vger.kernel.org" , Julia Lawall , linux-arm-msm , Yinghai Lu , Shawn Lin , linux-arm , Myron Stowe Subject: RE: [PATCH V8 4/5] PCI/ASPM: save power on values during bridge init Thread-Topic: [PATCH V8 4/5] PCI/ASPM: save power on values during bridge init Thread-Index: AQHSsCR5lQf5ZNPCu0maCYKOKGfA0KHCEjuAgAMi3oCAACqMAIAACP0AgARYXoCAABQGgIAEPdjQ Date: Fri, 21 Apr 2017 07:46:27 +0000 Message-ID: <92EBB4272BF81E4089A7126EC1E7B2846676C7EF@IRSMSX101.ger.corp.intel.com> References: <1491627351-1111-1-git-send-email-okaya@codeaurora.org> <1491627351-1111-5-git-send-email-okaya@codeaurora.org> <20170414214452.GA21870@bhelgaas-glaptop.roam.corp.google.com> <66168dde-7719-6f74-3f06-8e4724dd2918@codeaurora.org> In-Reply-To: Accept-Language: de-DE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 10.0.102.7 dlp-reaction: no-action x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id v3L7krYQ021947 Hi Bjorn/Kaya, > >On 4/17/2017 12:38 PM, Bjorn Helgaas wrote: >>> Like you said, what do we do by default is the question. Should we opt >>> for safe like we are doing, or try to save some power. >> I think safety is paramount. Every user should be able to boot safely >> without any kernel parameters. We don't want users to have a problem >> booting and then have to search for a workaround like booting with >> "pcie_aspm=off". Most users will never do that. >> > >OK, no problem with leaving the behavior as it is. > >My initial approach was #2. We knew this way that user had full control >over the ASPM policy by changing the BIOS option. Then, Mayurkumar >complained that ASPM is not enabled following a hotplug insertion to an >empty slot. That's when I switched to #3 as it sounded like a good thing >to have for us. > >> Here's a long-term strawman proposal, see what you think: >> >> - Deprecate CONFIG_PCIEASPM_DEFAULT, CONFIG_PCIEASPM_POWERSAVE, etc. >> - Default aspm_policy is POLICY_DEFAULT always. >> - POLICY_DEFAULT means Linux doesn't touch anything: if BIOS enabled >> ASPM, we leave it that way; we leave ASPM disabled on hot-added >> devices. > I am also ok with leaving the same behavior as now. But still following is something open I feel besides, Which may be there in your comments redundantly. The current problem is, pcie_aspm_exit_link_state() disables the ASPM configuration even if POLICY_DEFAULT was set. I am seeing already following problem(or may be influence) with it. The Endpoint I have does not have does not have "Presence detect change" mechanism. Hot plug is working with Link status events. When link is in L1 or L1SS and if EP is powered off, no Link status change event are triggered (It might be the expected behavior in L1 or L1SS). When next time EP is powered on there are link down and link up events coming one after other. BIOS enables ASPM on Root port and Endpoint, but while processing link status down, pcie_aspm_exit_link_state() clears the ASPM already which were enabled by BIOS. If we want to follow above approach then shall we consider having something similar as following? diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index 1dfa10c..bf5be6d 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -940,7 +940,8 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev) parent_link = link->parent; /* All functions are removed, so just disable ASPM for the link */ - pcie_config_aspm_link(link, 0); + if (aspm_policy != POLICY_DEFAULT) + pcie_config_aspm_link(link, 0); list_del(&link->sibling); list_del(&link->link); /* Clock PM is for endpoint device */ >I can easily see people complaining the other way around. There >could be some boot FW that doesn't know what ASPM is and this particular >system could rely on the compile time option for enabling power options. >Maybe, a command line option will be required for them to keep the existing >behavior. > >> - Deprecate kernel boot parameters (possibly keep pcie_aspm=off for >> debugging use). >> - Use /sys/module/pcie_aspm/parameters/policy for run-time >> system-wide control, including for future hot-added devices. >> - Remove CONFIG_PCIEASPM_DEBUG and enable that code always, so we >> have fine-grained run-time control. >> > >Runtime control sounds like a better plan. We need hooks into the system >power management policy. > >>> Maybe, we are missing a HPP option from the PCI spec. >> That's an interesting idea. _HPX does have provision for manipulating >> Link Control bits (see ACPI r5.0, sec 6.2.8.3), but I don't think very >> many systems implement it. And there's currently no connection >> between program_hpp_type2() and aspm.c, so I'm a little worried that >> we might have issues if a system did implement an _HPX that sets any >> of the ASPM bits. > >I looked at the spec some more. These are there to restore the register >settings following hotplug insertion. I agree it won't play nice with ASPM >as the control bits need to be enabled in coordination with the upstream >device. > >I think the right approach is to let the userspace feed the required >policy to the kernel like you suggested. Then, it needs to be per port >via link_state to have the most flexibility. > > >-- >Sinan Kaya >Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. >Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. Intel Deutschland GmbH Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de Managing Directors: Christin Eisenschmid, Christian Lamprechter Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Return-Path: From: "Patel, Mayurkumar" To: Sinan Kaya , Bjorn Helgaas CC: Bjorn Helgaas , Rajat Jain , "Rajat Jain" , David Daney , "linux-pci@vger.kernel.org" , Timur Tabi , "linux-kernel@vger.kernel.org" , Julia Lawall , linux-arm-msm , Yinghai Lu , Shawn Lin , linux-arm , Myron Stowe Subject: RE: [PATCH V8 4/5] PCI/ASPM: save power on values during bridge init Date: Fri, 21 Apr 2017 07:46:27 +0000 Message-ID: <92EBB4272BF81E4089A7126EC1E7B2846676C7EF@IRSMSX101.ger.corp.intel.com> References: <1491627351-1111-1-git-send-email-okaya@codeaurora.org> <1491627351-1111-5-git-send-email-okaya@codeaurora.org> <20170414214452.GA21870@bhelgaas-glaptop.roam.corp.google.com> <66168dde-7719-6f74-3f06-8e4724dd2918@codeaurora.org> In-Reply-To: Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 List-ID: SGkgQmpvcm4vS2F5YSwNCg0KDQo+DQo+T24gNC8xNy8yMDE3IDEyOjM4IFBNLCBCam9ybiBIZWxn YWFzIHdyb3RlOg0KPj4+IExpa2UgeW91IHNhaWQsIHdoYXQgZG8gd2UgZG8gYnkgZGVmYXVsdCBp cyB0aGUgcXVlc3Rpb24uIFNob3VsZCB3ZSBvcHQNCj4+PiBmb3Igc2FmZSBsaWtlIHdlIGFyZSBk b2luZywgb3IgdHJ5IHRvIHNhdmUgc29tZSBwb3dlci4NCj4+IEkgdGhpbmsgc2FmZXR5IGlzIHBh cmFtb3VudC4gIEV2ZXJ5IHVzZXIgc2hvdWxkIGJlIGFibGUgdG8gYm9vdCBzYWZlbHkNCj4+IHdp dGhvdXQgYW55IGtlcm5lbCBwYXJhbWV0ZXJzLiAgV2UgZG9uJ3Qgd2FudCB1c2VycyB0byBoYXZl IGEgcHJvYmxlbQ0KPj4gYm9vdGluZyBhbmQgdGhlbiBoYXZlIHRvIHNlYXJjaCBmb3IgYSB3b3Jr YXJvdW5kIGxpa2UgYm9vdGluZyB3aXRoDQo+PiAicGNpZV9hc3BtPW9mZiIuICBNb3N0IHVzZXJz IHdpbGwgbmV2ZXIgZG8gdGhhdC4NCj4+DQo+DQo+T0ssIG5vIHByb2JsZW0gd2l0aCBsZWF2aW5n IHRoZSBiZWhhdmlvciBhcyBpdCBpcy4NCj4NCj5NeSBpbml0aWFsIGFwcHJvYWNoIHdhcyAjMi4g V2Uga25ldyB0aGlzIHdheSB0aGF0IHVzZXIgaGFkIGZ1bGwgY29udHJvbA0KPm92ZXIgdGhlIEFT UE0gcG9saWN5IGJ5IGNoYW5naW5nIHRoZSBCSU9TIG9wdGlvbi4gVGhlbiwgTWF5dXJrdW1hcg0K PmNvbXBsYWluZWQgdGhhdCBBU1BNIGlzIG5vdCBlbmFibGVkIGZvbGxvd2luZyBhIGhvdHBsdWcg aW5zZXJ0aW9uIHRvIGFuDQo+ZW1wdHkgc2xvdC4gVGhhdCdzIHdoZW4gSSBzd2l0Y2hlZCB0byAj MyBhcyBpdCBzb3VuZGVkIGxpa2UgYSBnb29kIHRoaW5nDQo+dG8gaGF2ZSBmb3IgdXMuDQo+DQo+ PiBIZXJlJ3MgYSBsb25nLXRlcm0gc3RyYXdtYW4gcHJvcG9zYWwsIHNlZSB3aGF0IHlvdSB0aGlu azoNCj4+DQo+PiAgIC0gRGVwcmVjYXRlIENPTkZJR19QQ0lFQVNQTV9ERUZBVUxULCBDT05GSUdf UENJRUFTUE1fUE9XRVJTQVZFLCBldGMuDQo+PiAgIC0gRGVmYXVsdCBhc3BtX3BvbGljeSBpcyBQ T0xJQ1lfREVGQVVMVCBhbHdheXMuDQo+PiAgIC0gUE9MSUNZX0RFRkFVTFQgbWVhbnMgTGludXgg ZG9lc24ndCB0b3VjaCBhbnl0aGluZzogaWYgQklPUyBlbmFibGVkDQo+PiBBU1BNLCB3ZSBsZWF2 ZSBpdCB0aGF0IHdheTsgd2UgbGVhdmUgQVNQTSBkaXNhYmxlZCBvbiBob3QtYWRkZWQNCj4+IGRl dmljZXMuDQo+DQpJIGFtIGFsc28gb2sgd2l0aCBsZWF2aW5nIHRoZSBzYW1lIGJlaGF2aW9yIGFz IG5vdy4NCkJ1dCBzdGlsbCBmb2xsb3dpbmcgaXMgc29tZXRoaW5nIG9wZW4gSSBmZWVsIGJlc2lk ZXMsIFdoaWNoIG1heSBiZSB0aGVyZSBpbiB5b3VyIGNvbW1lbnRzIHJlZHVuZGFudGx5Lg0KVGhl IGN1cnJlbnQgcHJvYmxlbSBpcywgcGNpZV9hc3BtX2V4aXRfbGlua19zdGF0ZSgpIGRpc2FibGVz IHRoZSBBU1BNIGNvbmZpZ3VyYXRpb24gZXZlbg0KaWYgUE9MSUNZX0RFRkFVTFQgd2FzIHNldC4N CkkgYW0gc2VlaW5nIGFscmVhZHkgZm9sbG93aW5nIHByb2JsZW0ob3IgbWF5IGJlIGluZmx1ZW5j ZSkgd2l0aCBpdC4gVGhlIEVuZHBvaW50IEkgaGF2ZSBkb2VzIG5vdCBoYXZlDQpkb2VzIG5vdCBo YXZlICJQcmVzZW5jZSBkZXRlY3QgY2hhbmdlIiBtZWNoYW5pc20uIEhvdCBwbHVnIGlzIHdvcmtp bmcgd2l0aCBMaW5rIHN0YXR1cyBldmVudHMuDQpXaGVuIGxpbmsgaXMgaW4gTDEgb3IgTDFTUyBh bmQgaWYgRVAgaXMgcG93ZXJlZCBvZmYsIG5vIExpbmsgc3RhdHVzIGNoYW5nZSBldmVudCBhcmUg dHJpZ2dlcmVkIChJdCBtaWdodCBiZQ0KdGhlIGV4cGVjdGVkIGJlaGF2aW9yIGluIEwxIG9yIEwx U1MpLiAgV2hlbiBuZXh0IHRpbWUgRVAgaXMgcG93ZXJlZCBvbiB0aGVyZSBhcmUgbGluayBkb3du IGFuZA0KbGluayB1cCBldmVudHMgY29taW5nIG9uZSBhZnRlciBvdGhlci4gQklPUyBlbmFibGVz IEFTUE0gb24gUm9vdCBwb3J0IGFuZCBFbmRwb2ludCwgYnV0IHdoaWxlDQpwcm9jZXNzaW5nIGxp bmsgc3RhdHVzIGRvd24sIHBjaWVfYXNwbV9leGl0X2xpbmtfc3RhdGUoKSBjbGVhcnMgdGhlIEFT UE0gYWxyZWFkeSB3aGljaCB3ZXJlIGVuYWJsZWQgYnkgQklPUy4gDQpJZiB3ZSB3YW50IHRvIGZv bGxvdyBhYm92ZSBhcHByb2FjaCB0aGVuIHNoYWxsIHdlIGNvbnNpZGVyIGhhdmluZyBzb21ldGhp bmcgc2ltaWxhciBhcyBmb2xsb3dpbmc/DQoNCmRpZmYgLS1naXQgYS9kcml2ZXJzL3BjaS9wY2ll L2FzcG0uYyBiL2RyaXZlcnMvcGNpL3BjaWUvYXNwbS5jDQppbmRleCAxZGZhMTBjLi5iZjViZTZk IDEwMDY0NA0KLS0tIGEvZHJpdmVycy9wY2kvcGNpZS9hc3BtLmMNCisrKyBiL2RyaXZlcnMvcGNp L3BjaWUvYXNwbS5jDQpAQCAtOTQwLDcgKzk0MCw4IEBAIHZvaWQgcGNpZV9hc3BtX2V4aXRfbGlu a19zdGF0ZShzdHJ1Y3QgcGNpX2RldiAqcGRldikNCiAgICAgICAgcGFyZW50X2xpbmsgPSBsaW5r LT5wYXJlbnQ7DQoNCiAgICAgICAgLyogQWxsIGZ1bmN0aW9ucyBhcmUgcmVtb3ZlZCwgc28ganVz dCBkaXNhYmxlIEFTUE0gZm9yIHRoZSBsaW5rICovDQotICAgICAgIHBjaWVfY29uZmlnX2FzcG1f bGluayhsaW5rLCAwKTsNCisgICAgICAgaWYgKGFzcG1fcG9saWN5ICE9IFBPTElDWV9ERUZBVUxU KQ0KKyAgICAgICAgICAgICAgIHBjaWVfY29uZmlnX2FzcG1fbGluayhsaW5rLCAwKTsNCiAgICAg ICAgbGlzdF9kZWwoJmxpbmstPnNpYmxpbmcpOw0KICAgICAgICBsaXN0X2RlbCgmbGluay0+bGlu ayk7DQogICAgICAgIC8qIENsb2NrIFBNIGlzIGZvciBlbmRwb2ludCBkZXZpY2UgKi8NCiANCg0K PkkgY2FuIGVhc2lseSBzZWUgcGVvcGxlIGNvbXBsYWluaW5nIHRoZSBvdGhlciB3YXkgYXJvdW5k LiBUaGVyZQ0KPmNvdWxkIGJlIHNvbWUgYm9vdCBGVyB0aGF0IGRvZXNuJ3Qga25vdyB3aGF0IEFT UE0gaXMgYW5kIHRoaXMgcGFydGljdWxhcg0KPnN5c3RlbSBjb3VsZCByZWx5IG9uIHRoZSBjb21w aWxlIHRpbWUgb3B0aW9uIGZvciBlbmFibGluZyBwb3dlciBvcHRpb25zLg0KPk1heWJlLCBhIGNv bW1hbmQgbGluZSBvcHRpb24gd2lsbCBiZSByZXF1aXJlZCBmb3IgdGhlbSB0byBrZWVwIHRoZSBl eGlzdGluZw0KPmJlaGF2aW9yLg0KPg0KPj4gICAtIERlcHJlY2F0ZSBrZXJuZWwgYm9vdCBwYXJh bWV0ZXJzIChwb3NzaWJseSBrZWVwIHBjaWVfYXNwbT1vZmYgZm9yDQo+PiBkZWJ1Z2dpbmcgdXNl KS4NCj4+ICAgLSBVc2UgL3N5cy9tb2R1bGUvcGNpZV9hc3BtL3BhcmFtZXRlcnMvcG9saWN5IGZv ciBydW4tdGltZQ0KPj4gc3lzdGVtLXdpZGUgIGNvbnRyb2wsIGluY2x1ZGluZyBmb3IgZnV0dXJl IGhvdC1hZGRlZCBkZXZpY2VzLg0KPj4gICAtIFJlbW92ZSBDT05GSUdfUENJRUFTUE1fREVCVUcg YW5kIGVuYWJsZSB0aGF0IGNvZGUgYWx3YXlzLCBzbyB3ZQ0KPj4gaGF2ZSBmaW5lLWdyYWluZWQg cnVuLXRpbWUgY29udHJvbC4NCj4+DQo+DQo+UnVudGltZSBjb250cm9sIHNvdW5kcyBsaWtlIGEg YmV0dGVyIHBsYW4uIFdlIG5lZWQgaG9va3MgaW50byB0aGUgc3lzdGVtDQo+cG93ZXIgbWFuYWdl bWVudCBwb2xpY3kuDQo+DQo+Pj4gTWF5YmUsIHdlIGFyZSBtaXNzaW5nIGEgSFBQIG9wdGlvbiBm cm9tIHRoZSBQQ0kgc3BlYy4NCj4+IFRoYXQncyBhbiBpbnRlcmVzdGluZyBpZGVhLiAgX0hQWCBk b2VzIGhhdmUgcHJvdmlzaW9uIGZvciBtYW5pcHVsYXRpbmcNCj4+IExpbmsgQ29udHJvbCBiaXRz IChzZWUgQUNQSSByNS4wLCBzZWMgNi4yLjguMyksIGJ1dCBJIGRvbid0IHRoaW5rIHZlcnkNCj4+ IG1hbnkgc3lzdGVtcyBpbXBsZW1lbnQgaXQuICBBbmQgdGhlcmUncyBjdXJyZW50bHkgbm8gY29u bmVjdGlvbg0KPj4gYmV0d2VlbiBwcm9ncmFtX2hwcF90eXBlMigpIGFuZCBhc3BtLmMsIHNvIEkn bSBhIGxpdHRsZSB3b3JyaWVkIHRoYXQNCj4+IHdlIG1pZ2h0IGhhdmUgaXNzdWVzIGlmIGEgc3lz dGVtIGRpZCBpbXBsZW1lbnQgYW4gX0hQWCB0aGF0IHNldHMgYW55DQo+PiBvZiB0aGUgQVNQTSBi aXRzLg0KPg0KPkkgbG9va2VkIGF0IHRoZSBzcGVjIHNvbWUgbW9yZS4gVGhlc2UgYXJlIHRoZXJl IHRvIHJlc3RvcmUgdGhlIHJlZ2lzdGVyDQo+c2V0dGluZ3MgZm9sbG93aW5nIGhvdHBsdWcgaW5z ZXJ0aW9uLiBJIGFncmVlIGl0IHdvbid0IHBsYXkgbmljZSB3aXRoIEFTUE0NCj5hcyB0aGUgY29u dHJvbCBiaXRzIG5lZWQgdG8gYmUgZW5hYmxlZCBpbiBjb29yZGluYXRpb24gd2l0aCB0aGUgdXBz dHJlYW0NCj5kZXZpY2UuDQo+DQo+SSB0aGluayB0aGUgcmlnaHQgYXBwcm9hY2ggaXMgdG8gbGV0 IHRoZSB1c2Vyc3BhY2UgZmVlZCB0aGUgcmVxdWlyZWQNCj5wb2xpY3kgdG8gdGhlIGtlcm5lbCBs aWtlIHlvdSBzdWdnZXN0ZWQuIFRoZW4sIGl0IG5lZWRzIHRvIGJlIHBlciBwb3J0DQo+dmlhIGxp bmtfc3RhdGUgdG8gaGF2ZSB0aGUgbW9zdCBmbGV4aWJpbGl0eS4NCj4NCj4NCj4tLQ0KPlNpbmFu IEtheWENCj5RdWFsY29tbSBEYXRhY2VudGVyIFRlY2hub2xvZ2llcywgSW5jLiBhcyBhbiBhZmZp bGlhdGUgb2YgUXVhbGNvbW0gVGVjaG5vbG9naWVzLCBJbmMuDQo+UXVhbGNvbW0gVGVjaG5vbG9n aWVzLCBJbmMuIGlzIGEgbWVtYmVyIG9mIHRoZSBDb2RlIEF1cm9yYSBGb3J1bSwgYSBMaW51eCBG b3VuZGF0aW9uIENvbGxhYm9yYXRpdmUgUHJvamVjdC4NCkludGVsIERldXRzY2hsYW5kIEdtYkgK UmVnaXN0ZXJlZCBBZGRyZXNzOiBBbSBDYW1wZW9uIDEwLTEyLCA4NTU3OSBOZXViaWJlcmcsIEdl cm1hbnkKVGVsOiArNDkgODkgOTkgODg1My0wLCB3d3cuaW50ZWwuZGUKTWFuYWdpbmcgRGlyZWN0 b3JzOiBDaHJpc3RpbiBFaXNlbnNjaG1pZCwgQ2hyaXN0aWFuIExhbXByZWNodGVyCkNoYWlycGVy c29uIG9mIHRoZSBTdXBlcnZpc29yeSBCb2FyZDogTmljb2xlIExhdQpSZWdpc3RlcmVkIE9mZmlj ZTogTXVuaWNoCkNvbW1lcmNpYWwgUmVnaXN0ZXI6IEFtdHNnZXJpY2h0IE11ZW5jaGVuIEhSQiAx ODY5MjgK From mboxrd@z Thu Jan 1 00:00:00 1970 From: mayurkumar.patel@intel.com (Patel, Mayurkumar) Date: Fri, 21 Apr 2017 07:46:27 +0000 Subject: [PATCH V8 4/5] PCI/ASPM: save power on values during bridge init In-Reply-To: References: <1491627351-1111-1-git-send-email-okaya@codeaurora.org> <1491627351-1111-5-git-send-email-okaya@codeaurora.org> <20170414214452.GA21870@bhelgaas-glaptop.roam.corp.google.com> <66168dde-7719-6f74-3f06-8e4724dd2918@codeaurora.org> Message-ID: <92EBB4272BF81E4089A7126EC1E7B2846676C7EF@IRSMSX101.ger.corp.intel.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Bjorn/Kaya, > >On 4/17/2017 12:38 PM, Bjorn Helgaas wrote: >>> Like you said, what do we do by default is the question. Should we opt >>> for safe like we are doing, or try to save some power. >> I think safety is paramount. Every user should be able to boot safely >> without any kernel parameters. We don't want users to have a problem >> booting and then have to search for a workaround like booting with >> "pcie_aspm=off". Most users will never do that. >> > >OK, no problem with leaving the behavior as it is. > >My initial approach was #2. We knew this way that user had full control >over the ASPM policy by changing the BIOS option. Then, Mayurkumar >complained that ASPM is not enabled following a hotplug insertion to an >empty slot. That's when I switched to #3 as it sounded like a good thing >to have for us. > >> Here's a long-term strawman proposal, see what you think: >> >> - Deprecate CONFIG_PCIEASPM_DEFAULT, CONFIG_PCIEASPM_POWERSAVE, etc. >> - Default aspm_policy is POLICY_DEFAULT always. >> - POLICY_DEFAULT means Linux doesn't touch anything: if BIOS enabled >> ASPM, we leave it that way; we leave ASPM disabled on hot-added >> devices. > I am also ok with leaving the same behavior as now. But still following is something open I feel besides, Which may be there in your comments redundantly. The current problem is, pcie_aspm_exit_link_state() disables the ASPM configuration even if POLICY_DEFAULT was set. I am seeing already following problem(or may be influence) with it. The Endpoint I have does not have does not have "Presence detect change" mechanism. Hot plug is working with Link status events. When link is in L1 or L1SS and if EP is powered off, no Link status change event are triggered (It might be the expected behavior in L1 or L1SS). When next time EP is powered on there are link down and link up events coming one after other. BIOS enables ASPM on Root port and Endpoint, but while processing link status down, pcie_aspm_exit_link_state() clears the ASPM already which were enabled by BIOS. If we want to follow above approach then shall we consider having something similar as following? diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index 1dfa10c..bf5be6d 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -940,7 +940,8 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev) parent_link = link->parent; /* All functions are removed, so just disable ASPM for the link */ - pcie_config_aspm_link(link, 0); + if (aspm_policy != POLICY_DEFAULT) + pcie_config_aspm_link(link, 0); list_del(&link->sibling); list_del(&link->link); /* Clock PM is for endpoint device */ >I can easily see people complaining the other way around. There >could be some boot FW that doesn't know what ASPM is and this particular >system could rely on the compile time option for enabling power options. >Maybe, a command line option will be required for them to keep the existing >behavior. > >> - Deprecate kernel boot parameters (possibly keep pcie_aspm=off for >> debugging use). >> - Use /sys/module/pcie_aspm/parameters/policy for run-time >> system-wide control, including for future hot-added devices. >> - Remove CONFIG_PCIEASPM_DEBUG and enable that code always, so we >> have fine-grained run-time control. >> > >Runtime control sounds like a better plan. We need hooks into the system >power management policy. > >>> Maybe, we are missing a HPP option from the PCI spec. >> That's an interesting idea. _HPX does have provision for manipulating >> Link Control bits (see ACPI r5.0, sec 6.2.8.3), but I don't think very >> many systems implement it. And there's currently no connection >> between program_hpp_type2() and aspm.c, so I'm a little worried that >> we might have issues if a system did implement an _HPX that sets any >> of the ASPM bits. > >I looked at the spec some more. These are there to restore the register >settings following hotplug insertion. I agree it won't play nice with ASPM >as the control bits need to be enabled in coordination with the upstream >device. > >I think the right approach is to let the userspace feed the required >policy to the kernel like you suggested. Then, it needs to be per port >via link_state to have the most flexibility. > > >-- >Sinan Kaya >Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. >Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. Intel Deutschland GmbH Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de Managing Directors: Christin Eisenschmid, Christian Lamprechter Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928