From patchwork Wed Dec 27 23:37:50 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Bjorn Helgaas X-Patchwork-Id: 10134329 X-Patchwork-Delegate: bhelgaas@google.com Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id A80FB60318 for ; Wed, 27 Dec 2017 23:38:12 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 9A3602D290 for ; Wed, 27 Dec 2017 23:38:12 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 8F0692D478; Wed, 27 Dec 2017 23:38:12 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id AE17B2D290 for ; Wed, 27 Dec 2017 23:38:10 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752262AbdL0Xh5 (ORCPT ); Wed, 27 Dec 2017 18:37:57 -0500 Received: from mail.kernel.org ([198.145.29.99]:39718 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751963AbdL0Xh4 (ORCPT ); Wed, 27 Dec 2017 18:37:56 -0500 Received: from localhost (unknown [69.71.4.159]) (using TLSv1.2 with cipher DHE-RSA-AES128-SHA (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 6CD032187C; Wed, 27 Dec 2017 23:37:52 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 6CD032187C Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=helgaas@kernel.org Date: Wed, 27 Dec 2017 17:37:50 -0600 From: Bjorn Helgaas To: =?utf-8?B?5Yav6ZSQ?= Cc: "lee.jones@linaro.org" , "linux-kernel@vger.kernel.org" , Hans de Goede , Dave Jiang , "linux-pci@vger.kernel.org" Subject: Re: =?utf-8?B?562U5aSNOiDnrZTlpI0=?= =?utf-8?Q?=3A?= [PATCH v6] mfd: Add support for RTS5250S power saving Message-ID: <20171227233750.GB79892@bhelgaas-glaptop.roam.corp.google.com> References: <1504772799-15173-1-git-send-email-rui_feng@realsil.com.cn> <20171214222522.GL30595@bhelgaas-glaptop.roam.corp.google.com> <2A308283684ECD4B896628E09AF5361E01A8150B@RS-MBS01.realsil.com.cn> <20171215151522.GT30595@bhelgaas-glaptop.roam.corp.google.com> <2A308283684ECD4B896628E09AF5361E01A8193C@RS-MBS01.realsil.com.cn> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <2A308283684ECD4B896628E09AF5361E01A8193C@RS-MBS01.realsil.com.cn> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Tue, Dec 19, 2017 at 08:15:24AM +0000, 冯锐 wrote: > > On Fri, Dec 15, 2017 at 09:42:45AM +0000, 冯锐 wrote: > > > > [+cc Hans, Dave, linux-pci] > > > > > > > > On Thu, Sep 07, 2017 at 04:26:39PM +0800, rui_feng@realsil.com.cn > > wrote: > > > > > From: Rui Feng > > > > > > > > I wish this had been posted to linux-pci before being merged. > > > > > > > > I'm concerned because some of this appears to overlap and conflict > > > > with PCI core management of ASPM. > > > > > > > > I assume these devices advertise ASPM support in their Link > > > > Capabilites registers, right? If so, why isn't the existing PCI > > > > core ASPM support sufficient? > > > > > > > When L1SS is configured, the device(hardware) can't enter L1SS status > > > automatically, it need driver(software) to do some work to achieve the > > function. > > > > So this is a hardware defect in the device? As far as I know, ASPM and L1SS > > are specified such that they should work without special driver support. > > > Yes, you can say that. > > > > > > Enable power saving for RTS5250S as following steps: > > > > > 1.Set 0xFE58 to enable clock power management. > > > > > > > > Is this clock power management something specific to RTS5250S, or is > > > > it standard PCIe architected stuff? > > > > > > > 0xFE58 is specific register to RTS5250S not standard PCIe architected stuff. > > > > OK. I asked because devices often mirror architected PCIe config things in > > device-specific MMIO space, and if I squint just right, I can sort of match up the > > register bits you used with things in the PCIe spec. > > > > > > > 2.Check cfg space whether support L1SS or not. > > > > > > > > This sounds like standard PCIe ASPM L1 Substates, right? > > > > > > > Yes. > > > > > > > > 3.If support L1SS, set 0xFF03 to free clkreq. > > > > > 4.When entering idle status, enable aspm > > > > > and set parameters for L1SS and LTR. > > > > > 5.Wnen entering run status, disable aspm > > > > > and set parameters for L1SS and LTR. > > > > > > > > In general, drivers should not configure ASPM, L1SS, and LTR > > > > themselves; the PCI core should do that. > > > > > > > > If a driver needs to tweak ASPM at run-time, it should use > > > > interfaces exported by the PCI core to do so. > > > > > > > Which interface I can use to set ASPM? I use "pci_write_config_byte" now. > > > > What do you need to do? include/linux/pci-aspm.h exports > > pci_disable_link_state(), which is mainly used to avoid ASPM > > states that have hardware errata. > > > I want to enable ASPM(L0 -> L1) and disable ASPM(L1 -> L0), which > interface can I use? You can use pci_disable_link_state() to disable usage of L1. Currently there is no corresponding pci_enable_link_state(). What if we added something like the following (untested)? Would that work for you? commit 209930d809fa602b8aafdd171b26719cee6c6649 Author: Bjorn Helgaas Date: Wed Dec 27 16:56:26 2017 -0600 PCI/ASPM: Add pci_enable_link_state() Some drivers want control over the ASPM states their device is allowed to use. We already have a pci_disable_link_state(), and drivers can use that to prevent the device from entering L0 or L1s. Add a corresponding pci_enable_link_state() so a driver can enable use of L0 or L1s again. diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index 3b9b4d50cd98..ca217195f800 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -1028,6 +1028,67 @@ void pcie_aspm_powersave_config_link(struct pci_dev *pdev) up_read(&pci_bus_sem); } +/** + * pci_enable_link_state - Enable device's link state, so the link may + * enter specific states. Note that if the BIOS didn't grant ASPM + * control to the OS, this does nothing because we can't touch the LNKCTL + * register. + * + * @pdev: PCI device + * @state: ASPM link state to enable + */ +void pci_enable_link_state(struct pci_dev *pdev, int state) +{ + struct pci_dev *parent = pdev->bus->self; + struct pcie_link_state *link; + u32 lnkcap; + + if (!pci_is_pcie(pdev)) + return; + + if (pdev->has_secondary_link) + parent = pdev; + if (!parent || !parent->link_state) + return; + + /* + * A driver requested that ASPM be enabled on this device, but + * if we don't have permission to manage ASPM (e.g., on ACPI + * systems we have to observe the FADT ACPI_FADT_NO_ASPM bit and + * the _OSC method), we can't honor that request. Windows has + * a similar mechanism using "PciASPMOptOut", which is also + * ignored in this situation. + */ + if (aspm_disabled) { + dev_warn(&pdev->dev, "can't enable ASPM; OS doesn't have ASPM control\n"); + return; + } + + down_read(&pci_bus_sem); + mutex_lock(&aspm_lock); + link = parent->link_state; + if (state & PCIE_LINK_STATE_L0S) + link->aspm_disable &= ~ASPM_STATE_L0S; + if (state & PCIE_LINK_STATE_L1) + link->aspm_disable &= ~ASPM_STATE_L1; + pcie_config_aspm_link(link, policy_to_aspm_state(link)); + + /* + * Enable Clock Power Management if requested by the driver, + * supported by the device, and allowed by the current policy. + */ + if (state & PCIE_LINK_STATE_CLKPM) { + pcie_capability_read_dword(pdev, PCI_EXP_LNKCAP, &lnkcap); + if (lnkcap & PCI_EXP_LNKCAP_CLKPM) { + link->clkpm_capable = 1; + pcie_set_clkpm(link, policy_to_clkpm_state(link)); + } + } + mutex_unlock(&aspm_lock); + up_read(&pci_bus_sem); +} +EXPORT_SYMBOL(pci_enable_link_state); + static void __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem) { struct pci_dev *parent = pdev->bus->self; diff --git a/include/linux/pci-aspm.h b/include/linux/pci-aspm.h index df28af5cef21..cd0736b384ae 100644 --- a/include/linux/pci-aspm.h +++ b/include/linux/pci-aspm.h @@ -24,10 +24,12 @@ #define PCIE_LINK_STATE_CLKPM 4 #ifdef CONFIG_PCIEASPM +void pci_enable_link_state(struct pci_dev *pdev, int state); void pci_disable_link_state(struct pci_dev *pdev, int state); void pci_disable_link_state_locked(struct pci_dev *pdev, int state); void pcie_no_aspm(void); #else +static inline void pci_enable_link_state(struct pci_dev *pdev, int state) { } static inline void pci_disable_link_state(struct pci_dev *pdev, int state) { } static inline void pcie_no_aspm(void) { } #endif