[PATCH v2 1/2] Add save/restore of Precision Time Measurement capability

[PATCH v2 1/2] Add save/restore of Precision Time Measurement capability

[David E. Box]

The PCI subsystem does not currently save and restore the configuration
space for the Precision Time Measurement (PTM) PCIe extended capability
leading to the possibility of the feature returning disabled on S3 resume.
This has been observed on Intel Coffee Lake desktops. Add save/restore of
the PTM control register. This saves the PTM Enable, Root Select, and
Effective Granularity bits.

Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---

Changes from V1:
	- Move save/restore functions to ptm.c
	- Move pci_add_ext_cap_sve_buffer() to pci_ptm_init in ptm.c
	
 drivers/pci/pci.c      |  2 ++
 drivers/pci/pci.h      |  8 ++++++++
 drivers/pci/pcie/ptm.c | 43 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 53 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e578d34095e9..12ba6351c05b 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1566,6 +1566,7 @@ int pci_save_state(struct pci_dev *dev)
 	pci_save_ltr_state(dev);
 	pci_save_dpc_state(dev);
 	pci_save_aer_state(dev);
+	pci_save_ptm_state(dev);
 	return pci_save_vc_state(dev);
 }
 EXPORT_SYMBOL(pci_save_state);
@@ -1677,6 +1678,7 @@ void pci_restore_state(struct pci_dev *dev)
 	pci_restore_vc_state(dev);
 	pci_restore_rebar_state(dev);
 	pci_restore_dpc_state(dev);
+	pci_restore_ptm_state(dev);
 
 	pci_aer_clear_status(dev);
 	pci_restore_aer_state(dev);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index f86cae9aa1f4..62cdacba5954 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -516,6 +516,14 @@ static inline int pci_iov_bus_range(struct pci_bus *bus)
 
 #endif /* CONFIG_PCI_IOV */
 
+#ifdef CONFIG_PCIE_PTM
+void pci_save_ptm_state(struct pci_dev *dev);
+void pci_restore_ptm_state(struct pci_dev *dev);
+#else
+static inline void pci_save_ptm_state(struct pci_dev *dev) {}
+static inline void pci_restore_ptm_state(struct pci_dev *dev) {}
+#endif
+
 unsigned long pci_cardbus_resource_alignment(struct resource *);
 
 static inline resource_size_t pci_resource_alignment(struct pci_dev *dev,
diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c
index 357a454cafa0..6b24a1c9327a 100644
--- a/drivers/pci/pcie/ptm.c
+++ b/drivers/pci/pcie/ptm.c
@@ -29,6 +29,47 @@ static void pci_ptm_info(struct pci_dev *dev)
 		 dev->ptm_root ? " (root)" : "", clock_desc);
 }
 
+void pci_save_ptm_state(struct pci_dev *dev)
+{
+	int ptm;
+	struct pci_cap_saved_state *save_state;
+	u16 *cap;
+
+	if (!pci_is_pcie(dev))
+		return;
+
+	ptm = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_PTM);
+	if (!ptm)
+		return;
+
+	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_PTM);
+	if (!save_state) {
+		pci_err(dev, "no suspend buffer for PTM\n");
+		return;
+	}
+
+	cap = (u16 *)&save_state->cap.data[0];
+	pci_read_config_word(dev, ptm + PCI_PTM_CTRL, cap);
+}
+
+void pci_restore_ptm_state(struct pci_dev *dev)
+{
+	struct pci_cap_saved_state *save_state;
+	int ptm;
+	u16 *cap;
+
+	if (!pci_is_pcie(dev))
+		return;
+
+	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_PTM);
+	ptm = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_PTM);
+	if (!save_state || !ptm)
+		return;
+
+	cap = (u16 *)&save_state->cap.data[0];
+	pci_write_config_word(dev, ptm + PCI_PTM_CTRL, *cap);
+}
+
 void pci_ptm_init(struct pci_dev *dev)
 {
 	int pos;
@@ -65,6 +106,8 @@ void pci_ptm_init(struct pci_dev *dev)
 	if (!pos)
 		return;
 
+	pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_PTM, sizeof(u16));
+
 	pci_read_config_dword(dev, pos + PCI_PTM_CAP, &cap);
 	local_clock = (cap & PCI_PTM_GRANULARITY_MASK) >> 8;
 
-- 
2.20.1

[PATCH v2 2/2] PCI: Disable PTM during suspend to save power

[David E. Box]

There are systems (for example, Intel based mobile platforms since Coffee
Lake) where the power drawn while suspended can be significantly reduced by
disabling Precision Time Measurement (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. To save this power, disable the PTM feature on root
ports during pci_prepare_to_sleep() and pci_finish_runtime_suspend().  The
feature will be returned to its previous state during restore and error
recovery.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=209361
Reported-by: Len Brown <len.brown@intel.com>
Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
Changes from V1:

	- Add pci_disable_ptm() to ptm.c
	- Move ptm disabling from pci_save_ptm_state() to
	  pci_prepare_to_sleep() and pci_finish_runtime_suspend() as
	  suggested by Rafael.
	- Comment change suggested by Rafael

 drivers/pci/pci.c      | 25 ++++++++++++++++++++++++-
 drivers/pci/pci.h      |  2 ++
 drivers/pci/pcie/ptm.c | 17 +++++++++++++++++
 3 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 12ba6351c05b..71dd5d7cbded 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2608,12 +2608,24 @@ 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);
 
-	if (error)
+	if (error) {
 		pci_enable_wake(dev, target_state, false);
+		pci_restore_ptm_state(dev);
+	}
 
 	return error;
 }
@@ -2651,12 +2663,23 @@ int pci_finish_runtime_suspend(struct pci_dev *dev)
 
 	dev->runtime_d3cold = target_state == PCI_D3cold;
 
+	/*
+	 * 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);
 
 	if (error) {
 		pci_enable_wake(dev, target_state, false);
+		pci_restore_ptm_state(dev);
 		dev->runtime_d3cold = false;
 	}
 
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 62cdacba5954..4a478ca7e3b5 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -519,9 +519,11 @@ static inline int pci_iov_bus_range(struct pci_bus *bus)
 #ifdef CONFIG_PCIE_PTM
 void pci_save_ptm_state(struct pci_dev *dev);
 void pci_restore_ptm_state(struct pci_dev *dev);
+void pci_disable_ptm(struct pci_dev *dev);
 #else
 static inline void pci_save_ptm_state(struct pci_dev *dev) {}
 static inline void pci_restore_ptm_state(struct pci_dev *dev) {}
+static inline void pci_disable_ptm(struct pci_dev *dev) {}
 #endif
 
 unsigned long pci_cardbus_resource_alignment(struct resource *);
diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c
index 6b24a1c9327a..95d4eef2c9e8 100644
--- a/drivers/pci/pcie/ptm.c
+++ b/drivers/pci/pcie/ptm.c
@@ -29,6 +29,23 @@ static void pci_ptm_info(struct pci_dev *dev)
 		 dev->ptm_root ? " (root)" : "", clock_desc);
 }
 
+void pci_disable_ptm(struct pci_dev *dev)
+{
+	int ptm;
+	u16 ctrl;
+
+	if (!pci_is_pcie(dev))
+		return;
+
+	ptm = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_PTM);
+	if (!ptm)
+		return;
+
+	pci_read_config_word(dev, ptm + PCI_PTM_CTRL, &ctrl);
+	ctrl &= ~(PCI_PTM_CTRL_ENABLE | PCI_PTM_CTRL_ROOT);
+	pci_write_config_word(dev, ptm + PCI_PTM_CTRL, ctrl);
+}
+
 void pci_save_ptm_state(struct pci_dev *dev)
 {
 	int ptm;
-- 
2.20.1

Re: [PATCH v2 1/2] Add save/restore of Precision Time Measurement capability

[Bjorn Helgaas]

On Mon, Dec 07, 2020 at 02:39:50PM -0800, David E. Box wrote:
> The PCI subsystem does not currently save and restore the configuration
> space for the Precision Time Measurement (PTM) PCIe extended capability
> leading to the possibility of the feature returning disabled on S3 resume.
> This has been observed on Intel Coffee Lake desktops. Add save/restore of
> the PTM control register. This saves the PTM Enable, Root Select, and
> Effective Granularity bits.
> 
> Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>

Applied both to pci/ptm for v5.11, thanks!

> ---
> 
> Changes from V1:
> 	- Move save/restore functions to ptm.c
> 	- Move pci_add_ext_cap_sve_buffer() to pci_ptm_init in ptm.c
> 	
>  drivers/pci/pci.c      |  2 ++
>  drivers/pci/pci.h      |  8 ++++++++
>  drivers/pci/pcie/ptm.c | 43 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 53 insertions(+)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e578d34095e9..12ba6351c05b 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1566,6 +1566,7 @@ int pci_save_state(struct pci_dev *dev)
>  	pci_save_ltr_state(dev);
>  	pci_save_dpc_state(dev);
>  	pci_save_aer_state(dev);
> +	pci_save_ptm_state(dev);
>  	return pci_save_vc_state(dev);
>  }
>  EXPORT_SYMBOL(pci_save_state);
> @@ -1677,6 +1678,7 @@ void pci_restore_state(struct pci_dev *dev)
>  	pci_restore_vc_state(dev);
>  	pci_restore_rebar_state(dev);
>  	pci_restore_dpc_state(dev);
> +	pci_restore_ptm_state(dev);
>  
>  	pci_aer_clear_status(dev);
>  	pci_restore_aer_state(dev);
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index f86cae9aa1f4..62cdacba5954 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -516,6 +516,14 @@ static inline int pci_iov_bus_range(struct pci_bus *bus)
>  
>  #endif /* CONFIG_PCI_IOV */
>  
> +#ifdef CONFIG_PCIE_PTM
> +void pci_save_ptm_state(struct pci_dev *dev);
> +void pci_restore_ptm_state(struct pci_dev *dev);
> +#else
> +static inline void pci_save_ptm_state(struct pci_dev *dev) {}
> +static inline void pci_restore_ptm_state(struct pci_dev *dev) {}
> +#endif
> +
>  unsigned long pci_cardbus_resource_alignment(struct resource *);
>  
>  static inline resource_size_t pci_resource_alignment(struct pci_dev *dev,
> diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c
> index 357a454cafa0..6b24a1c9327a 100644
> --- a/drivers/pci/pcie/ptm.c
> +++ b/drivers/pci/pcie/ptm.c
> @@ -29,6 +29,47 @@ static void pci_ptm_info(struct pci_dev *dev)
>  		 dev->ptm_root ? " (root)" : "", clock_desc);
>  }
>  
> +void pci_save_ptm_state(struct pci_dev *dev)
> +{
> +	int ptm;
> +	struct pci_cap_saved_state *save_state;
> +	u16 *cap;
> +
> +	if (!pci_is_pcie(dev))
> +		return;
> +
> +	ptm = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_PTM);
> +	if (!ptm)
> +		return;
> +
> +	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_PTM);
> +	if (!save_state) {
> +		pci_err(dev, "no suspend buffer for PTM\n");
> +		return;
> +	}
> +
> +	cap = (u16 *)&save_state->cap.data[0];
> +	pci_read_config_word(dev, ptm + PCI_PTM_CTRL, cap);
> +}
> +
> +void pci_restore_ptm_state(struct pci_dev *dev)
> +{
> +	struct pci_cap_saved_state *save_state;
> +	int ptm;
> +	u16 *cap;
> +
> +	if (!pci_is_pcie(dev))
> +		return;
> +
> +	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_PTM);
> +	ptm = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_PTM);
> +	if (!save_state || !ptm)
> +		return;
> +
> +	cap = (u16 *)&save_state->cap.data[0];
> +	pci_write_config_word(dev, ptm + PCI_PTM_CTRL, *cap);
> +}
> +
>  void pci_ptm_init(struct pci_dev *dev)
>  {
>  	int pos;
> @@ -65,6 +106,8 @@ void pci_ptm_init(struct pci_dev *dev)
>  	if (!pos)
>  		return;
>  
> +	pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_PTM, sizeof(u16));
> +
>  	pci_read_config_dword(dev, pos + PCI_PTM_CAP, &cap);
>  	local_clock = (cap & PCI_PTM_GRANULARITY_MASK) >> 8;
>  
> -- 
> 2.20.1
>