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, [email protected] wrote:
> > > From: Rui Feng <[email protected]>
> >
> > 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.
> > > 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.
If you need to do something beyond that, we can talk about adding
something new.
There are quite a few other things in include/linux/pci-aspm.h, but
they're all for internal use in the PCI core. I'll move them to
drivers/pci/pci.h to avoid confusion.
> > > +static void rts5249_set_aspm(struct rtsx_pcr *pcr, bool enable) {
> > > + struct rtsx_cr_option *option = &pcr->option;
> > > + u8 val = 0;
> > > +
> > > + if (pcr->aspm_enabled == enable)
> > > + return;
> > > +
> > > + if (option->dev_aspm_mode == DEV_ASPM_DYNAMIC) {
> > > + if (enable)
> > > + val = pcr->aspm_en;
> > > + rtsx_pci_update_cfg_byte(pcr,
> > > + pcr->pcie_cap + PCI_EXP_LNKCTL,
> > > + ASPM_MASK_NEG, val);
> >
> > This stomps on whatever ASPM configuration the PCI core did.
> We disable/enable aspm dynamic in order to improve read/write performance and more stable,
> so we don't allow PCI core do it.
This is pretty vague. Can you be any more specific? If there are
performance or stability problems in the PCI core ASPM support, I'd
prefer to fix those instead of working around them in every driver.
Bjorn
> 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, [email protected]
> wrote:
> > > > From: Rui Feng <[email protected]>
> > >
> > > 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?
> If you need to do something beyond that, we can talk about adding something
> new.
>
> There are quite a few other things in include/linux/pci-aspm.h, but they're all
> for internal use in the PCI core. I'll move them to drivers/pci/pci.h to avoid
> confusion.
>
> > > > +static void rts5249_set_aspm(struct rtsx_pcr *pcr, bool enable) {
> > > > + struct rtsx_cr_option *option = &pcr->option;
> > > > + u8 val = 0;
> > > > +
> > > > + if (pcr->aspm_enabled == enable)
> > > > + return;
> > > > +
> > > > + if (option->dev_aspm_mode == DEV_ASPM_DYNAMIC) {
> > > > + if (enable)
> > > > + val = pcr->aspm_en;
> > > > + rtsx_pci_update_cfg_byte(pcr,
> > > > + pcr->pcie_cap + PCI_EXP_LNKCTL,
> > > > + ASPM_MASK_NEG, val);
> > >
> > > This stomps on whatever ASPM configuration the PCI core did.
> > We disable/enable aspm dynamic in order to improve read/write
> > performance and more stable, so we don't allow PCI core do it.
>
> This is pretty vague. Can you be any more specific? If there are
> performance or stability problems in the PCI core ASPM support, I'd prefer to
> fix those instead of working around them in every driver.
>
It's device's defect not PCI core, so no need to fix PCI core.
> Bjorn
>
> ------Please consider the environment before printing this e-mail.
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, [email protected]
> > wrote:
> > > > > From: Rui Feng <[email protected]>
> > > >
> > > > 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 <[email protected]>
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
On Wed, Dec 27, 2017 at 05:37:50PM -0600, Bjorn Helgaas wrote:
> 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, [email protected]
> > > wrote:
> > > > > > From: Rui Feng <[email protected]>
> > > > >
> > > > > 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?
Hi Rui,
Any thoughts on the patch below?
> commit 209930d809fa602b8aafdd171b26719cee6c6649
> Author: Bjorn Helgaas <[email protected]>
> 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
> On Wed, Dec 27, 2017 at 05:37:50PM -0600, Bjorn Helgaas wrote:
> > 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,
> > > > > > [email protected]
> > > > wrote:
> > > > > > > From: Rui Feng <[email protected]>
> > > > > >
> > > > > > 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?
>
> Hi Rui,
>
> Any thoughts on the patch below?
I'm busy with other work, the patch seems ok, I will test it later.
>
> > commit 209930d809fa602b8aafdd171b26719cee6c6649
> > Author: Bjorn Helgaas <[email protected]>
> > 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
>
> ------Please consider the environment before printing this e-mail.
On Fri, Jan 19, 2018 at 07:38:22AM +0000, 冯锐 wrote:
> > On Wed, Dec 27, 2017 at 05:37:50PM -0600, Bjorn Helgaas wrote:
> > > 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,
> > > > > > > [email protected]
> > > > > wrote:
> > > > > > > > From: Rui Feng <[email protected]>
> > > > > > >
> > > > > > > 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?
> >
> > Hi Rui,
> >
> > Any thoughts on the patch below?
>
> I'm busy with other work, the patch seems ok, I will test it later.
Any idea when you might get back to this?
If you can't do it, I can try doing it myself, but of course, I don't
know the details about the device errata and I wouldn't be able to
test it.
Bjorn
>
> On Fri, Jan 19, 2018 at 07:38:22AM +0000, 冯锐 wrote:
> > > On Wed, Dec 27, 2017 at 05:37:50PM -0600, Bjorn Helgaas wrote:
> > > > 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,
> > > > > > > > [email protected]
> > > > > > wrote:
> > > > > > > > > From: Rui Feng <[email protected]>
> > > > > > > >
> > > > > > > > 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?
> > >
> > > Hi Rui,
> > >
> > > Any thoughts on the patch below?
> >
> > I'm busy with other work, the patch seems ok, I will test it later.
>
> Any idea when you might get back to this?
>
> If you can't do it, I can try doing it myself, but of course, I don't know the details
> about the device errata and I wouldn't be able to test it.
>
Of course, you can do it first.
> Bjorn
>
> ------Please consider the environment before printing this e-mail.