[PATCH v2 0/3] PCI/PM: Always disable PTM for all devices during suspend

[PATCH v2 0/3] PCI/PM: Always disable PTM for all devices during suspend

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

We currently disable PTM for Root Ports during suspend.  Leaving PTM
enabled for downstream devices causes UR errors if they send PTM Requests.
The intent of this series is to:

  - Unconditionally disable PTM during suspend (even if the driver saves
    its own state) by moving the disable from pci_prepare_to_sleep() to
    pci_pm_suspend().

  - Disable PTM for all devices by removing the Root Port condition and
    doing it early in the suspend paths.

  - Explicitly re-enable PTM during resume, which requires new support in
    pci_enable_ptm() for Root Ports and Switch Upstream Ports.


Bjorn Helgaas (3):
  PCI/PTM: Preserve PTM Root Select
  PCI/PTM: Implement pci_enable_ptm() for Root Ports, Switch Upstream
    Ports
  PCI/PM: Always disable PTM for all devices during suspend

 drivers/pci/pci-driver.c | 14 ++++++++++++++
 drivers/pci/pci.c        | 20 --------------------
 drivers/pci/pcie/ptm.c   | 36 ++++++++++++++++++++++++++++--------
 3 files changed, 42 insertions(+), 28 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] PCI/PTM: Preserve PTM Root Select

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

When disabling PTM, there's no need to clear the Root Select bit.  We
disable PTM during suspend, and we want to re-enable it during resume.
Clearing Root Select here makes re-enabling more complicated.

Per PCIe r6.0, sec 7.9.15.3, "When set, if the PTM Enable bit is also Set,
this Time Source is the PTM Root," so if PTM Enable is cleared, the value
of Root Select should be irrelevant.

Preserve Root Select to simplify re-enabling PTM.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Cc: David E. Box <david.e.box@linux.intel.com>
---
 drivers/pci/pcie/ptm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c
index 368a254e3124..b6a417247ce3 100644
--- a/drivers/pci/pcie/ptm.c
+++ b/drivers/pci/pcie/ptm.c
@@ -42,7 +42,7 @@ void pci_disable_ptm(struct pci_dev *dev)
 		return;
 
 	pci_read_config_word(dev, ptm + PCI_PTM_CTRL, &ctrl);
-	ctrl &= ~(PCI_PTM_CTRL_ENABLE | PCI_PTM_CTRL_ROOT);
+	ctrl &= ~PCI_PTM_CTRL_ENABLE;
 	pci_write_config_word(dev, ptm + PCI_PTM_CTRL, ctrl);
 }
 
-- 
2.25.1

[PATCH v2 2/3] PCI/PTM: Implement pci_enable_ptm() for Root Ports, Switch Upstream Ports

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/ptm.c | 34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c
index b6a417247ce3..ad283818f37b 100644
--- a/drivers/pci/pcie/ptm.c
+++ b/drivers/pci/pcie/ptm.c
@@ -167,11 +167,11 @@ int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
 	if (!pos)
 		return -EINVAL;
 
-	pci_read_config_dword(dev, pos + PCI_PTM_CAP, &cap);
-	if (!(cap & PCI_PTM_CAP_REQ))
-		return -EINVAL;
-
 	/*
+	 * Root Ports and Switch Upstream Ports have been configured
+	 * by pci_ptm_init(), so preserve their PCI_PTM_CTRL_ROOT and
+	 * granularity.
+	 *
 	 * For a PCIe Endpoint, PTM is only useful if the endpoint can
 	 * issue PTM requests to upstream devices that have PTM enabled.
 	 *
@@ -179,19 +179,39 @@ int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
 	 * device, so there must be some implementation-specific way to
 	 * associate the endpoint with a time source.
 	 */
-	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ENDPOINT) {
+	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
+	    pci_pcie_type(dev) == PCI_EXP_TYPE_UPSTREAM) {
+		if (pci_pcie_type(dev) == PCI_EXP_TYPE_UPSTREAM) {
+			ups = pci_upstream_bridge(dev);
+			if (!ups || !ups->ptm_enabled)
+				return -EINVAL;
+		}
+
+		pci_read_config_dword(dev, pos + PCI_PTM_CTRL, &ctrl);
+		ctrl |= PCI_PTM_CTRL_ENABLE;
+	} else if (pci_pcie_type(dev) == PCI_EXP_TYPE_ENDPOINT) {
+		pci_read_config_dword(dev, pos + PCI_PTM_CAP, &cap);
+		if (!(cap & PCI_PTM_CAP_REQ))
+			return -EINVAL;
+
 		ups = pci_upstream_bridge(dev);
 		if (!ups || !ups->ptm_enabled)
 			return -EINVAL;
 
 		dev->ptm_granularity = ups->ptm_granularity;
+		ctrl = PCI_PTM_CTRL_ENABLE;
+		ctrl |= dev->ptm_granularity << 8;
 	} else if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) {
+		pci_read_config_dword(dev, pos + PCI_PTM_CAP, &cap);
+		if (!(cap & PCI_PTM_CAP_REQ))
+			return -EINVAL;
+
 		dev->ptm_granularity = 0;
+		ctrl = PCI_PTM_CTRL_ENABLE;
+		ctrl |= dev->ptm_granularity << 8;
 	} else
 		return -EINVAL;
 
-	ctrl = PCI_PTM_CTRL_ENABLE;
-	ctrl |= dev->ptm_granularity << 8;
 	pci_write_config_dword(dev, pos + PCI_PTM_CTRL, ctrl);
 	dev->ptm_enabled = 1;
 
-- 
2.25.1

[PATCH v2 3/3] PCI/PM: Always disable PTM for all devices during suspend

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

We want to disable PTM on Root Ports because that allows some chips, e.g.,
Intel mobile chips since Coffee Lake, to enter a lower-power PM state.

That means we also have to disable PTM on downstream devices.  PCIe r6.0,
sec 2.2.8, recommends that functions support generation of messages in
non-D0 states, so we have to assume Switch Upstream Ports or Endpoints may
send PTM Requests while in D1, D2, and D3hot.  A PTM message received by a
Downstream Port (including a Root Port) with PTM disabled must be treated
as an Unsupported Request (sec 6.21.3).

PTM was previously disabled only for Root Ports, and it was disabled in
pci_prepare_to_sleep(), which is not called at all if a driver supports
legacy PM or does its own state saving.

Instead, disable PTM early in pci_pm_suspend() and pci_pm_runtime_suspend()
so we do it in all cases.

Previously PTM was disabled *after* saving device state, so the state
restore on resume automatically re-enabled it.  Since we now disable PTM
*before* saving state, we must explicitly re-enable it.

Here's a sample of errors that occur when PTM is disabled only on the Root
Port.  With this topology:

  0000:00:1d.0 Root Port            to [bus 08-71]
  0000:08:00.0 Switch Upstream Port to [bus 09-71]

Kai-Heng reported errors like this:

  pcieport 0000:00:1d.0: AER: Uncorrected (Non-Fatal) error received: 0000:00:1d.0
  pcieport 0000:00:1d.0: PCIe Bus Error: severity=Uncorrected (Non-Fatal), type=Transaction Layer, (Requester ID)
  pcieport 0000:00:1d.0:   device [8086:7ab0] error status/mask=00100000/00004000
  pcieport 0000:00:1d.0:    [20] UnsupReq               (First)
  pcieport 0000:00:1d.0: AER:   TLP Header: 34000000 08000052 00000000 00000000

Decoding TLP header 0x34...... (0011 0100b) and 0x08000052:

  Fmt                         001b  4 DW header, no data
  Type                     1 0100b  Msg (Local - Terminate at Receiver)
  Requester ID  0x0800              Bus 08 Devfn 00.0
  Message Code    0x52  0101 0010b  PTM Request

The 00:1d.0 Root Port logged an Unsupported Request error when it received
a PTM Request with Requester ID 08:00.0.

Fixes: a697f072f5da ("PCI: Disable PTM during suspend to save power")
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=215453
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216210
Based-on: https://lore.kernel.org/r/20220706123244.18056-1-kai.heng.feng@canonical.com
Based-on-patch-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
Reported-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci-driver.c | 14 ++++++++++++++
 drivers/pci/pci.c        | 20 --------------------
 2 files changed, 14 insertions(+), 20 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 2815922ac525..115febaa7e0b 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -772,6 +772,12 @@ static int pci_pm_suspend(struct device *dev)
 	struct pci_dev *pci_dev = to_pci_dev(dev);
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 
+	/*
+	 * Disabling PTM allows some systems, e.g., Intel mobile chips
+	 * since Coffee Lake, to enter a lower-power PM state.
+	 */
+	pci_disable_ptm(pci_dev);
+
 	pci_dev->skip_bus_pm = false;
 
 	if (pci_has_legacy_pm_support(pci_dev))
@@ -982,6 +988,9 @@ static int pci_pm_resume(struct device *dev)
 	if (pci_dev->state_saved)
 		pci_restore_standard_config(pci_dev);
 
+	if (pci_dev->ptm_enabled)
+		pci_enable_ptm(pci_dev, NULL);
+
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_resume(dev);
 
@@ -1269,6 +1278,8 @@ static int pci_pm_runtime_suspend(struct device *dev)
 	pci_power_t prev = pci_dev->current_state;
 	int error;
 
+	pci_disable_ptm(pci_dev);
+
 	/*
 	 * If pci_dev->driver is not set (unbound), we leave the device in D0,
 	 * but it may go to D3cold when the bridge above it runtime suspends.
@@ -1331,6 +1342,9 @@ static int pci_pm_runtime_resume(struct device *dev)
 	 */
 	pci_pm_default_resume_early(pci_dev);
 
+	if (pci_dev->ptm_enabled)
+		pci_enable_ptm(pci_dev, NULL);
+
 	if (!pci_dev->driver)
 		return 0;
 
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 95bc329e74c0..b0e2968c8cca 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2706,16 +2706,6 @@ int pci_prepare_to_sleep(struct pci_dev *dev)
 	if (target_state == PCI_POWER_ERROR)
 		return -EIO;
 
-	/*
-	 * There are systems (for example, Intel mobile chips since Coffee
-	 * Lake) where the power drawn while suspended can be significantly
-	 * reduced by disabling PTM on PCIe root ports as this allows the
-	 * port to enter a lower-power PM state and the SoC to reach a
-	 * lower-power idle state as a whole.
-	 */
-	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT)
-		pci_disable_ptm(dev);
-
 	pci_enable_wake(dev, target_state, wakeup);
 
 	error = pci_set_power_state(dev, target_state);
@@ -2764,16 +2754,6 @@ int pci_finish_runtime_suspend(struct pci_dev *dev)
 	if (target_state == PCI_POWER_ERROR)
 		return -EIO;
 
-	/*
-	 * There are systems (for example, Intel mobile chips since Coffee
-	 * Lake) where the power drawn while suspended can be significantly
-	 * reduced by disabling PTM on PCIe root ports as this allows the
-	 * port to enter a lower-power PM state and the SoC to reach a
-	 * lower-power idle state as a whole.
-	 */
-	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT)
-		pci_disable_ptm(dev);
-
 	__pci_enable_wake(dev, target_state, pci_dev_run_wake(dev));
 
 	error = pci_set_power_state(dev, target_state);
-- 
2.25.1

Re: [PATCH v2 2/3] PCI/PTM: Implement pci_enable_ptm() for Root Ports, Switch Upstream Ports

[Sathyanarayanan Kuppuswamy]

On 9/2/22 4:35 PM, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/pcie/ptm.c | 34 +++++++++++++++++++++++++++-------
>  1 file changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c
> index b6a417247ce3..ad283818f37b 100644
> --- a/drivers/pci/pcie/ptm.c
> +++ b/drivers/pci/pcie/ptm.c
> @@ -167,11 +167,11 @@ int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
>  	if (!pos)
>  		return -EINVAL;
>  
> -	pci_read_config_dword(dev, pos + PCI_PTM_CAP, &cap);
> -	if (!(cap & PCI_PTM_CAP_REQ))
> -		return -EINVAL;
> -

May be saving PCI_PTM_CAP_REQ enabled state here and using it below only for
endpoints checks will reduce the code duplication?

>  	/*
> +	 * Root Ports and Switch Upstream Ports have been configured
> +	 * by pci_ptm_init(), so preserve their PCI_PTM_CTRL_ROOT and
> +	 * granularity.
> +	 *
>  	 * For a PCIe Endpoint, PTM is only useful if the endpoint can
>  	 * issue PTM requests to upstream devices that have PTM enabled.
>  	 *
> @@ -179,19 +179,39 @@ int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
>  	 * device, so there must be some implementation-specific way to
>  	 * associate the endpoint with a time source.
>  	 */
> -	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ENDPOINT) {
> +	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> +	    pci_pcie_type(dev) == PCI_EXP_TYPE_UPSTREAM) {
> +		if (pci_pcie_type(dev) == PCI_EXP_TYPE_UPSTREAM) {
> +			ups = pci_upstream_bridge(dev);
> +			if (!ups || !ups->ptm_enabled)
> +				return -EINVAL;
> +		}
> +
> +		pci_read_config_dword(dev, pos + PCI_PTM_CTRL, &ctrl);

Why read PCI_PTM_CTRL state only for root and upstream ports? The same logic
will work for endpoints and RC endpoints right? 

What not use dev->ptm_granularity for root ports?

> +		ctrl |= PCI_PTM_CTRL_ENABLE;
> +	} else if (pci_pcie_type(dev) == PCI_EXP_TYPE_ENDPOINT) {
> +		pci_read_config_dword(dev, pos + PCI_PTM_CAP, &cap);
> +		if (!(cap & PCI_PTM_CAP_REQ))
> +			return -EINVAL;
> +
>  		ups = pci_upstream_bridge(dev);
>  		if (!ups || !ups->ptm_enabled)
>  			return -EINVAL;
>  
>  		dev->ptm_granularity = ups->ptm_granularity;
> +		ctrl = PCI_PTM_CTRL_ENABLE;
> +		ctrl |= dev->ptm_granularity << 8;
>  	} else if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) {
> +		pci_read_config_dword(dev, pos + PCI_PTM_CAP, &cap);
> +		if (!(cap & PCI_PTM_CAP_REQ))
> +			return -EINVAL;
> +
>  		dev->ptm_granularity = 0;
> +		ctrl = PCI_PTM_CTRL_ENABLE;
> +		ctrl |= dev->ptm_granularity << 8;
>  	} else
>  		return -EINVAL;
>  
> -	ctrl = PCI_PTM_CTRL_ENABLE;
> -	ctrl |= dev->ptm_granularity << 8;
>  	pci_write_config_dword(dev, pos + PCI_PTM_CTRL, ctrl);
>  	dev->ptm_enabled = 1;
>  

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v2 1/3] PCI/PTM: Preserve PTM Root Select

[Sathyanarayanan Kuppuswamy]

Hi,

On 9/2/22 4:35 PM, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> When disabling PTM, there's no need to clear the Root Select bit.  We
> disable PTM during suspend, and we want to re-enable it during resume.
> Clearing Root Select here makes re-enabling more complicated.
> 
> Per PCIe r6.0, sec 7.9.15.3, "When set, if the PTM Enable bit is also Set,
> this Time Source is the PTM Root," so if PTM Enable is cleared, the value
> of Root Select should be irrelevant.
> 
> Preserve Root Select to simplify re-enabling PTM.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> Cc: David E. Box <david.e.box@linux.intel.com>
> ---

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

>  drivers/pci/pcie/ptm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c
> index 368a254e3124..b6a417247ce3 100644
> --- a/drivers/pci/pcie/ptm.c
> +++ b/drivers/pci/pcie/ptm.c
> @@ -42,7 +42,7 @@ void pci_disable_ptm(struct pci_dev *dev)
>  		return;
>  
>  	pci_read_config_word(dev, ptm + PCI_PTM_CTRL, &ctrl);
> -	ctrl &= ~(PCI_PTM_CTRL_ENABLE | PCI_PTM_CTRL_ROOT);
> +	ctrl &= ~PCI_PTM_CTRL_ENABLE;
>  	pci_write_config_word(dev, ptm + PCI_PTM_CTRL, ctrl);
>  }
>  

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v2 3/3] PCI/PM: Always disable PTM for all devices during suspend

[Sathyanarayanan Kuppuswamy]

On 9/2/22 4:35 PM, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> We want to disable PTM on Root Ports because that allows some chips, e.g.,
> Intel mobile chips since Coffee Lake, to enter a lower-power PM state.
> 
> That means we also have to disable PTM on downstream devices.  PCIe r6.0,
> sec 2.2.8, recommends that functions support generation of messages in
> non-D0 states, so we have to assume Switch Upstream Ports or Endpoints may
> send PTM Requests while in D1, D2, and D3hot.  A PTM message received by a
> Downstream Port (including a Root Port) with PTM disabled must be treated
> as an Unsupported Request (sec 6.21.3).
> 
> PTM was previously disabled only for Root Ports, and it was disabled in
> pci_prepare_to_sleep(), which is not called at all if a driver supports
> legacy PM or does its own state saving.
> 
> Instead, disable PTM early in pci_pm_suspend() and pci_pm_runtime_suspend()
> so we do it in all cases.
> 
> Previously PTM was disabled *after* saving device state, so the state
> restore on resume automatically re-enabled it.  Since we now disable PTM
> *before* saving state, we must explicitly re-enable it.
> 
> Here's a sample of errors that occur when PTM is disabled only on the Root
> Port.  With this topology:
> 
>   0000:00:1d.0 Root Port            to [bus 08-71]
>   0000:08:00.0 Switch Upstream Port to [bus 09-71]
> 
> Kai-Heng reported errors like this:
> 
>   pcieport 0000:00:1d.0: AER: Uncorrected (Non-Fatal) error received: 0000:00:1d.0
>   pcieport 0000:00:1d.0: PCIe Bus Error: severity=Uncorrected (Non-Fatal), type=Transaction Layer, (Requester ID)
>   pcieport 0000:00:1d.0:   device [8086:7ab0] error status/mask=00100000/00004000
>   pcieport 0000:00:1d.0:    [20] UnsupReq               (First)
>   pcieport 0000:00:1d.0: AER:   TLP Header: 34000000 08000052 00000000 00000000
> 
> Decoding TLP header 0x34...... (0011 0100b) and 0x08000052:
> 
>   Fmt                         001b  4 DW header, no data
>   Type                     1 0100b  Msg (Local - Terminate at Receiver)
>   Requester ID  0x0800              Bus 08 Devfn 00.0
>   Message Code    0x52  0101 0010b  PTM Request
> 
> The 00:1d.0 Root Port logged an Unsupported Request error when it received
> a PTM Request with Requester ID 08:00.0.
> 
> Fixes: a697f072f5da ("PCI: Disable PTM during suspend to save power")
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=215453
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216210
> Based-on: https://lore.kernel.org/r/20220706123244.18056-1-kai.heng.feng@canonical.com
> Based-on-patch-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Reported-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/pci-driver.c | 14 ++++++++++++++
>  drivers/pci/pci.c        | 20 --------------------
>  2 files changed, 14 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 2815922ac525..115febaa7e0b 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -772,6 +772,12 @@ static int pci_pm_suspend(struct device *dev)
>  	struct pci_dev *pci_dev = to_pci_dev(dev);
>  	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>  
> +	/*
> +	 * Disabling PTM allows some systems, e.g., Intel mobile chips
> +	 * since Coffee Lake, to enter a lower-power PM state.
> +	 */
> +	pci_disable_ptm(pci_dev);

I think you can use "if (pci_dev->ptm_enabled)" check for pci_disable_ptm()
as well. This will avoid unnecessary checks in pci_disable_ptm().

> +
>  	pci_dev->skip_bus_pm = false;
>  
>  	if (pci_has_legacy_pm_support(pci_dev))
> @@ -982,6 +988,9 @@ static int pci_pm_resume(struct device *dev)
>  	if (pci_dev->state_saved)
>  		pci_restore_standard_config(pci_dev);
>  
> +	if (pci_dev->ptm_enabled)
> +		pci_enable_ptm(pci_dev, NULL);
> +
>  	if (pci_has_legacy_pm_support(pci_dev))
>  		return pci_legacy_resume(dev);
>  
> @@ -1269,6 +1278,8 @@ static int pci_pm_runtime_suspend(struct device *dev)
>  	pci_power_t prev = pci_dev->current_state;
>  	int error;
>  
> +	pci_disable_ptm(pci_dev);
> +
>  	/*
>  	 * If pci_dev->driver is not set (unbound), we leave the device in D0,
>  	 * but it may go to D3cold when the bridge above it runtime suspends.
> @@ -1331,6 +1342,9 @@ static int pci_pm_runtime_resume(struct device *dev)
>  	 */
>  	pci_pm_default_resume_early(pci_dev);
>  
> +	if (pci_dev->ptm_enabled)
> +		pci_enable_ptm(pci_dev, NULL);
> +
>  	if (!pci_dev->driver)
>  		return 0;
>  
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 95bc329e74c0..b0e2968c8cca 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2706,16 +2706,6 @@ int pci_prepare_to_sleep(struct pci_dev *dev)
>  	if (target_state == PCI_POWER_ERROR)
>  		return -EIO;
>  
> -	/*
> -	 * There are systems (for example, Intel mobile chips since Coffee
> -	 * Lake) where the power drawn while suspended can be significantly
> -	 * reduced by disabling PTM on PCIe root ports as this allows the
> -	 * port to enter a lower-power PM state and the SoC to reach a
> -	 * lower-power idle state as a whole.
> -	 */
> -	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT)
> -		pci_disable_ptm(dev);
> -
>  	pci_enable_wake(dev, target_state, wakeup);
>  
>  	error = pci_set_power_state(dev, target_state);
> @@ -2764,16 +2754,6 @@ int pci_finish_runtime_suspend(struct pci_dev *dev)
>  	if (target_state == PCI_POWER_ERROR)
>  		return -EIO;
>  
> -	/*
> -	 * There are systems (for example, Intel mobile chips since Coffee
> -	 * Lake) where the power drawn while suspended can be significantly
> -	 * reduced by disabling PTM on PCIe root ports as this allows the
> -	 * port to enter a lower-power PM state and the SoC to reach a
> -	 * lower-power idle state as a whole.
> -	 */
> -	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT)
> -		pci_disable_ptm(dev);
> -
>  	__pci_enable_wake(dev, target_state, pci_dev_run_wake(dev));
>  
>  	error = pci_set_power_state(dev, target_state);

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v2 3/3] PCI/PM: Always disable PTM for all devices during suspend

[kernel test robot]

Hi Bjorn,

I love your patch! Yet something to improve:

[auto build test ERROR on helgaas-pci/next]
[also build test ERROR on linus/master v6.0-rc3 next-20220901]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Bjorn-Helgaas/PCI-PM-Always-disable-PTM-for-all-devices-during-suspend/20220903-073808
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: riscv-randconfig-r042-20220901 (https://download.01.org/0day-ci/archive/20220903/202209031120.PkNnRSt8-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project c55b41d5199d2394dd6cdb8f52180d8b81d809d4)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install riscv cross compiling tool for clang build
        # apt-get install binutils-riscv64-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/809e1c954b459ee37193c4ab9fa843243fbd7fa9
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Bjorn-Helgaas/PCI-PM-Always-disable-PTM-for-all-devices-during-suspend/20220903-073808
        git checkout 809e1c954b459ee37193c4ab9fa843243fbd7fa9
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/pci/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/pci/pci-driver.c:1350:15: error: no member named 'ptm_enabled' in 'struct pci_dev'
           if (pci_dev->ptm_enabled)
               ~~~~~~~  ^
   1 error generated.


vim +1350 drivers/pci/pci-driver.c

  1335	
  1336	static int pci_pm_runtime_resume(struct device *dev)
  1337	{
  1338		struct pci_dev *pci_dev = to_pci_dev(dev);
  1339		const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
  1340		pci_power_t prev_state = pci_dev->current_state;
  1341		int error = 0;
  1342	
  1343		/*
  1344		 * Restoring config space is necessary even if the device is not bound
  1345		 * to a driver because although we left it in D0, it may have gone to
  1346		 * D3cold when the bridge above it runtime suspended.
  1347		 */
  1348		pci_pm_default_resume_early(pci_dev);
  1349	
> 1350		if (pci_dev->ptm_enabled)
  1351			pci_enable_ptm(pci_dev, NULL);
  1352	
  1353		if (!pci_dev->driver)
  1354			return 0;
  1355	
  1356		pci_fixup_device(pci_fixup_resume_early, pci_dev);
  1357		pci_pm_default_resume(pci_dev);
  1358	
  1359		if (prev_state == PCI_D3cold)
  1360			pci_pm_bridge_power_up_actions(pci_dev);
  1361	
  1362		if (pm && pm->runtime_resume)
  1363			error = pm->runtime_resume(dev);
  1364	
  1365		return error;
  1366	}
  1367	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v2 3/3] PCI/PM: Always disable PTM for all devices during suspend

[kernel test robot]

Hi Bjorn,

I love your patch! Yet something to improve:

[auto build test ERROR on helgaas-pci/next]
[also build test ERROR on linus/master v6.0-rc3 next-20220901]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Bjorn-Helgaas/PCI-PM-Always-disable-PTM-for-all-devices-during-suspend/20220903-073808
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: x86_64-randconfig-a015 (https://download.01.org/0day-ci/archive/20220903/202209031256.McjuB8mz-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-5) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/809e1c954b459ee37193c4ab9fa843243fbd7fa9
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Bjorn-Helgaas/PCI-PM-Always-disable-PTM-for-all-devices-during-suspend/20220903-073808
        git checkout 809e1c954b459ee37193c4ab9fa843243fbd7fa9
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/pci/pci-driver.c: In function 'pci_pm_runtime_resume':
>> drivers/pci/pci-driver.c:1350:22: error: 'struct pci_dev' has no member named 'ptm_enabled'; did you mean 'ats_enabled'?
    1350 |         if (pci_dev->ptm_enabled)
         |                      ^~~~~~~~~~~
         |                      ats_enabled


vim +1350 drivers/pci/pci-driver.c

  1335	
  1336	static int pci_pm_runtime_resume(struct device *dev)
  1337	{
  1338		struct pci_dev *pci_dev = to_pci_dev(dev);
  1339		const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
  1340		pci_power_t prev_state = pci_dev->current_state;
  1341		int error = 0;
  1342	
  1343		/*
  1344		 * Restoring config space is necessary even if the device is not bound
  1345		 * to a driver because although we left it in D0, it may have gone to
  1346		 * D3cold when the bridge above it runtime suspended.
  1347		 */
  1348		pci_pm_default_resume_early(pci_dev);
  1349	
> 1350		if (pci_dev->ptm_enabled)
  1351			pci_enable_ptm(pci_dev, NULL);
  1352	
  1353		if (!pci_dev->driver)
  1354			return 0;
  1355	
  1356		pci_fixup_device(pci_fixup_resume_early, pci_dev);
  1357		pci_pm_default_resume(pci_dev);
  1358	
  1359		if (prev_state == PCI_D3cold)
  1360			pci_pm_bridge_power_up_actions(pci_dev);
  1361	
  1362		if (pm && pm->runtime_resume)
  1363			error = pm->runtime_resume(dev);
  1364	
  1365		return error;
  1366	}
  1367	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v2 3/3] PCI/PM: Always disable PTM for all devices during suspend

[Rafael J. Wysocki]

On Sat, Sep 3, 2022 at 1:59 AM Sathyanarayanan Kuppuswamy
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>
>
>
> On 9/2/22 4:35 PM, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> >
> > We want to disable PTM on Root Ports because that allows some chips, e.g.,
> > Intel mobile chips since Coffee Lake, to enter a lower-power PM state.
> >
> > That means we also have to disable PTM on downstream devices.  PCIe r6.0,
> > sec 2.2.8, recommends that functions support generation of messages in
> > non-D0 states, so we have to assume Switch Upstream Ports or Endpoints may
> > send PTM Requests while in D1, D2, and D3hot.  A PTM message received by a
> > Downstream Port (including a Root Port) with PTM disabled must be treated
> > as an Unsupported Request (sec 6.21.3).
> >
> > PTM was previously disabled only for Root Ports, and it was disabled in
> > pci_prepare_to_sleep(), which is not called at all if a driver supports
> > legacy PM or does its own state saving.
> >
> > Instead, disable PTM early in pci_pm_suspend() and pci_pm_runtime_suspend()
> > so we do it in all cases.
> >
> > Previously PTM was disabled *after* saving device state, so the state
> > restore on resume automatically re-enabled it.  Since we now disable PTM
> > *before* saving state, we must explicitly re-enable it.
> >
> > Here's a sample of errors that occur when PTM is disabled only on the Root
> > Port.  With this topology:
> >
> >   0000:00:1d.0 Root Port            to [bus 08-71]
> >   0000:08:00.0 Switch Upstream Port to [bus 09-71]
> >
> > Kai-Heng reported errors like this:
> >
> >   pcieport 0000:00:1d.0: AER: Uncorrected (Non-Fatal) error received: 0000:00:1d.0
> >   pcieport 0000:00:1d.0: PCIe Bus Error: severity=Uncorrected (Non-Fatal), type=Transaction Layer, (Requester ID)
> >   pcieport 0000:00:1d.0:   device [8086:7ab0] error status/mask=00100000/00004000
> >   pcieport 0000:00:1d.0:    [20] UnsupReq               (First)
> >   pcieport 0000:00:1d.0: AER:   TLP Header: 34000000 08000052 00000000 00000000
> >
> > Decoding TLP header 0x34...... (0011 0100b) and 0x08000052:
> >
> >   Fmt                         001b  4 DW header, no data
> >   Type                     1 0100b  Msg (Local - Terminate at Receiver)
> >   Requester ID  0x0800              Bus 08 Devfn 00.0
> >   Message Code    0x52  0101 0010b  PTM Request
> >
> > The 00:1d.0 Root Port logged an Unsupported Request error when it received
> > a PTM Request with Requester ID 08:00.0.
> >
> > Fixes: a697f072f5da ("PCI: Disable PTM during suspend to save power")
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=215453
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216210
> > Based-on: https://lore.kernel.org/r/20220706123244.18056-1-kai.heng.feng@canonical.com
> > Based-on-patch-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > Reported-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >  drivers/pci/pci-driver.c | 14 ++++++++++++++
> >  drivers/pci/pci.c        | 20 --------------------
> >  2 files changed, 14 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > index 2815922ac525..115febaa7e0b 100644
> > --- a/drivers/pci/pci-driver.c
> > +++ b/drivers/pci/pci-driver.c
> > @@ -772,6 +772,12 @@ static int pci_pm_suspend(struct device *dev)
> >       struct pci_dev *pci_dev = to_pci_dev(dev);
> >       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> >
> > +     /*
> > +      * Disabling PTM allows some systems, e.g., Intel mobile chips
> > +      * since Coffee Lake, to enter a lower-power PM state.
> > +      */
> > +     pci_disable_ptm(pci_dev);
>
> I think you can use "if (pci_dev->ptm_enabled)" check for pci_disable_ptm()
> as well. This will avoid unnecessary checks in pci_disable_ptm().

Or use that check in pci_disable_ptm() instead of the pci_is_pcie() one.

Also, I would remae pci_disable_ptm() to pci_suspend_ptm() (because
its role is to temporarily disable PTM for system-wide suspend) and
introduc pci_resume_ptm() that will do

if (pci_dev->ptm_enabled)
        pci_enable_ptm(pci_dev, NULL);

> > +
> >       pci_dev->skip_bus_pm = false;
> >
> >       if (pci_has_legacy_pm_support(pci_dev))
> > @@ -982,6 +988,9 @@ static int pci_pm_resume(struct device *dev)
> >       if (pci_dev->state_saved)
> >               pci_restore_standard_config(pci_dev);
> >
> > +     if (pci_dev->ptm_enabled)
> > +             pci_enable_ptm(pci_dev, NULL);
> > +
> >       if (pci_has_legacy_pm_support(pci_dev))
> >               return pci_legacy_resume(dev);
> >
> > @@ -1269,6 +1278,8 @@ static int pci_pm_runtime_suspend(struct device *dev)
> >       pci_power_t prev = pci_dev->current_state;
> >       int error;
> >
> > +     pci_disable_ptm(pci_dev);
> > +
> >       /*
> >        * If pci_dev->driver is not set (unbound), we leave the device in D0,
> >        * but it may go to D3cold when the bridge above it runtime suspends.
> > @@ -1331,6 +1342,9 @@ static int pci_pm_runtime_resume(struct device *dev)
> >        */
> >       pci_pm_default_resume_early(pci_dev);
> >
> > +     if (pci_dev->ptm_enabled)
> > +             pci_enable_ptm(pci_dev, NULL);
> > +
> >       if (!pci_dev->driver)
> >               return 0;
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 95bc329e74c0..b0e2968c8cca 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -2706,16 +2706,6 @@ int pci_prepare_to_sleep(struct pci_dev *dev)
> >       if (target_state == PCI_POWER_ERROR)
> >               return -EIO;
> >
> > -     /*
> > -      * There are systems (for example, Intel mobile chips since Coffee
> > -      * Lake) where the power drawn while suspended can be significantly
> > -      * reduced by disabling PTM on PCIe root ports as this allows the
> > -      * port to enter a lower-power PM state and the SoC to reach a
> > -      * lower-power idle state as a whole.
> > -      */
> > -     if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT)
> > -             pci_disable_ptm(dev);
> > -
> >       pci_enable_wake(dev, target_state, wakeup);
> >
> >       error = pci_set_power_state(dev, target_state);
> > @@ -2764,16 +2754,6 @@ int pci_finish_runtime_suspend(struct pci_dev *dev)
> >       if (target_state == PCI_POWER_ERROR)
> >               return -EIO;
> >
> > -     /*
> > -      * There are systems (for example, Intel mobile chips since Coffee
> > -      * Lake) where the power drawn while suspended can be significantly
> > -      * reduced by disabling PTM on PCIe root ports as this allows the
> > -      * port to enter a lower-power PM state and the SoC to reach a
> > -      * lower-power idle state as a whole.
> > -      */
> > -     if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT)
> > -             pci_disable_ptm(dev);
> > -
> >       __pci_enable_wake(dev, target_state, pci_dev_run_wake(dev));
> >
> >       error = pci_set_power_state(dev, target_state);
>
> --
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer

Re: [PATCH v2 2/3] PCI/PTM: Implement pci_enable_ptm() for Root Ports, Switch Upstream Ports

[Rafael J. Wysocki]

On Sat, Sep 3, 2022 at 1:35 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/pcie/ptm.c | 34 +++++++++++++++++++++++++++-------
>  1 file changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c
> index b6a417247ce3..ad283818f37b 100644
> --- a/drivers/pci/pcie/ptm.c
> +++ b/drivers/pci/pcie/ptm.c
> @@ -167,11 +167,11 @@ int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
>         if (!pos)
>                 return -EINVAL;
>
> -       pci_read_config_dword(dev, pos + PCI_PTM_CAP, &cap);
> -       if (!(cap & PCI_PTM_CAP_REQ))
> -               return -EINVAL;
> -
>         /*
> +        * Root Ports and Switch Upstream Ports have been configured
> +        * by pci_ptm_init(), so preserve their PCI_PTM_CTRL_ROOT and
> +        * granularity.
> +        *
>          * For a PCIe Endpoint, PTM is only useful if the endpoint can
>          * issue PTM requests to upstream devices that have PTM enabled.
>          *
> @@ -179,19 +179,39 @@ int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
>          * device, so there must be some implementation-specific way to
>          * associate the endpoint with a time source.
>          */
> -       if (pci_pcie_type(dev) == PCI_EXP_TYPE_ENDPOINT) {
> +       if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> +           pci_pcie_type(dev) == PCI_EXP_TYPE_UPSTREAM) {
> +               if (pci_pcie_type(dev) == PCI_EXP_TYPE_UPSTREAM) {
> +                       ups = pci_upstream_bridge(dev);
> +                       if (!ups || !ups->ptm_enabled)
> +                               return -EINVAL;
> +               }
> +
> +               pci_read_config_dword(dev, pos + PCI_PTM_CTRL, &ctrl);
> +               ctrl |= PCI_PTM_CTRL_ENABLE;
> +       } else if (pci_pcie_type(dev) == PCI_EXP_TYPE_ENDPOINT) {
> +               pci_read_config_dword(dev, pos + PCI_PTM_CAP, &cap);
> +               if (!(cap & PCI_PTM_CAP_REQ))
> +                       return -EINVAL;
> +
>                 ups = pci_upstream_bridge(dev);
>                 if (!ups || !ups->ptm_enabled)
>                         return -EINVAL;
>
>                 dev->ptm_granularity = ups->ptm_granularity;
> +               ctrl = PCI_PTM_CTRL_ENABLE;
> +               ctrl |= dev->ptm_granularity << 8;
>         } else if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) {
> +               pci_read_config_dword(dev, pos + PCI_PTM_CAP, &cap);
> +               if (!(cap & PCI_PTM_CAP_REQ))
> +                       return -EINVAL;
> +
>                 dev->ptm_granularity = 0;
> +               ctrl = PCI_PTM_CTRL_ENABLE;
> +               ctrl |= dev->ptm_granularity << 8;
>         } else
>                 return -EINVAL;

I would do

if ((pci_pcie_type(dev) == PCI_EXP_TYPE_UPSTREAM || pci_pcie_type(dev)
== PCI_EXP_TYPE_ENDPOINT)) {
        ups = pci_upstream_bridge(dev);
        if (!ups || !ups->ptm_enabled)
                return -EINVAL;

        dev->ptm_granularity = ups->ptm_granularity;
}

switch(pci_pcie_type(dev)) {
case PCI_EXP_TYPE_ROOT_PORT:
case PCI_EXP_TYPE_UPSTREAM:
        pci_read_config_dword(dev, pos + PCI_PTM_CTRL, &ctrl);
        ctrl |= PCI_PTM_CTRL_ENABLE;
        break;
case PCI_EXP_TYPE_ENDPOINT:
case PCI_EXP_TYPE_RC_END:
        ctrl = PCI_PTM_CTRL_ENABLE;
        break;
default:
        return -EINVAL;
}

>
> -       ctrl = PCI_PTM_CTRL_ENABLE;
> -       ctrl |= dev->ptm_granularity << 8;

And I wouldn't remove the line above.

Note that for root ports dev->ptm_granularity must be set and reflect
the register setting or else the code wouldn't have worked for
downstream components.

>         pci_write_config_dword(dev, pos + PCI_PTM_CTRL, ctrl);
>         dev->ptm_enabled = 1;
>
> --

Re: [PATCH v2 2/3] PCI/PTM: Implement pci_enable_ptm() for Root Ports, Switch Upstream Ports

[Rafael J. Wysocki]

On Sat, Sep 3, 2022 at 7:40 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Sat, Sep 3, 2022 at 1:35 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > From: Bjorn Helgaas <bhelgaas@google.com>
> >
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >  drivers/pci/pcie/ptm.c | 34 +++++++++++++++++++++++++++-------
> >  1 file changed, 27 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c
> > index b6a417247ce3..ad283818f37b 100644
> > --- a/drivers/pci/pcie/ptm.c
> > +++ b/drivers/pci/pcie/ptm.c
> > @@ -167,11 +167,11 @@ int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
> >         if (!pos)
> >                 return -EINVAL;
> >
> > -       pci_read_config_dword(dev, pos + PCI_PTM_CAP, &cap);
> > -       if (!(cap & PCI_PTM_CAP_REQ))
> > -               return -EINVAL;
> > -
> >         /*
> > +        * Root Ports and Switch Upstream Ports have been configured
> > +        * by pci_ptm_init(), so preserve their PCI_PTM_CTRL_ROOT and
> > +        * granularity.
> > +        *
> >          * For a PCIe Endpoint, PTM is only useful if the endpoint can
> >          * issue PTM requests to upstream devices that have PTM enabled.
> >          *
> > @@ -179,19 +179,39 @@ int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
> >          * device, so there must be some implementation-specific way to
> >          * associate the endpoint with a time source.
> >          */
> > -       if (pci_pcie_type(dev) == PCI_EXP_TYPE_ENDPOINT) {
> > +       if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> > +           pci_pcie_type(dev) == PCI_EXP_TYPE_UPSTREAM) {
> > +               if (pci_pcie_type(dev) == PCI_EXP_TYPE_UPSTREAM) {
> > +                       ups = pci_upstream_bridge(dev);
> > +                       if (!ups || !ups->ptm_enabled)
> > +                               return -EINVAL;
> > +               }
> > +
> > +               pci_read_config_dword(dev, pos + PCI_PTM_CTRL, &ctrl);
> > +               ctrl |= PCI_PTM_CTRL_ENABLE;
> > +       } else if (pci_pcie_type(dev) == PCI_EXP_TYPE_ENDPOINT) {
> > +               pci_read_config_dword(dev, pos + PCI_PTM_CAP, &cap);
> > +               if (!(cap & PCI_PTM_CAP_REQ))
> > +                       return -EINVAL;
> > +
> >                 ups = pci_upstream_bridge(dev);
> >                 if (!ups || !ups->ptm_enabled)
> >                         return -EINVAL;
> >
> >                 dev->ptm_granularity = ups->ptm_granularity;
> > +               ctrl = PCI_PTM_CTRL_ENABLE;
> > +               ctrl |= dev->ptm_granularity << 8;
> >         } else if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) {
> > +               pci_read_config_dword(dev, pos + PCI_PTM_CAP, &cap);
> > +               if (!(cap & PCI_PTM_CAP_REQ))
> > +                       return -EINVAL;
> > +
> >                 dev->ptm_granularity = 0;
> > +               ctrl = PCI_PTM_CTRL_ENABLE;
> > +               ctrl |= dev->ptm_granularity << 8;
> >         } else
> >                 return -EINVAL;
>
> I would do
>
> if ((pci_pcie_type(dev) == PCI_EXP_TYPE_UPSTREAM || pci_pcie_type(dev)
> == PCI_EXP_TYPE_ENDPOINT)) {
>         ups = pci_upstream_bridge(dev);
>         if (!ups || !ups->ptm_enabled)
>                 return -EINVAL;
>
>         dev->ptm_granularity = ups->ptm_granularity;
> }
>
> switch(pci_pcie_type(dev)) {
> case PCI_EXP_TYPE_ROOT_PORT:
> case PCI_EXP_TYPE_UPSTREAM:
>         pci_read_config_dword(dev, pos + PCI_PTM_CTRL, &ctrl);
>         ctrl |= PCI_PTM_CTRL_ENABLE;
>         break;
> case PCI_EXP_TYPE_ENDPOINT:
> case PCI_EXP_TYPE_RC_END:

I missed the cap check here, sorry.

>         ctrl = PCI_PTM_CTRL_ENABLE;
>         break;
> default:
>         return -EINVAL;
> }
>
> >
> > -       ctrl = PCI_PTM_CTRL_ENABLE;
> > -       ctrl |= dev->ptm_granularity << 8;
>
> And I wouldn't remove the line above.
>
> Note that for root ports dev->ptm_granularity must be set and reflect
> the register setting or else the code wouldn't have worked for
> downstream components.
>
> >         pci_write_config_dword(dev, pos + PCI_PTM_CTRL, ctrl);
> >         dev->ptm_enabled = 1;
> >
> > --