答复: 答复: [PATCH v6] mfd: Add support for RTS5250S power saving

Commit Message

Bjorn Helgaas Dec. 27, 2017, 11:37 p.m. UTC

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, [e-mail]
> > wrote:
> > > > > From: Rui Feng <[e-mail]>
> > > >
> > > > 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 <[e-mail]>
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.

Comments

Bjorn Helgaas Jan. 17, 2018, 9:01 p.m. UTC | #1

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, [e-mail]
> > > wrote:
> > > > > > From: Rui Feng <[e-mail]>
> > > > >
> > > > > 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 <[e-mail]>
> 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

冯锐 Jan. 19, 2018, 7:38 a.m. UTC | #2

> 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,

> > > > > > [e-mail]

> > > > wrote:

> > > > > > > From: Rui Feng <[e-mail]>

> > > > > >

> > > > > > 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 <[e-mail]>

> > 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.

Bjorn Helgaas Jan. 30, 2018, 7:18 p.m. UTC | #3

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,
> > > > > > > [e-mail]
> > > > > wrote:
> > > > > > > > From: Rui Feng <[e-mail]>
> > > > > > >
> > > > > > > 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

冯锐 Feb. 1, 2018, 8:45 a.m. UTC | #4

> 

> 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,

> > > > > > > > [e-mail]

> > > > > > wrote:

> > > > > > > > > From: Rui Feng <[e-mail]>

> > > > > > > >

> > > > > > > > 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.

Patch

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