PCI/PM: Always disable PTM for all devices during suspend

PATCH Superseded Bjorn Helgaas 3 participants 22 messages Sep 6, 2022 – Sep 8, 2022 PCI

lore

Share & tools

lore

Bjorn Helgaas

author

2022-09-06 22:23 UTC #0 lore raw

v3 cover PCI/PM: Always disable PTM for all devices during suspend

From: Bjorn Helgaas <[email protected]>

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.

This got long and pretty complicated to read via the patches. The end result of ptm.c might help as a roadmap to where I hoped to go:

https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/tree/drivers/pci/pcie/ptm.c?h=07c2204ab0f3

Basically I wanted to make pci_enable_ptm() and pci_disable_ptm() flip the PCI_PTM_CTRL_ENABLE bit and nothing else, with all the setup based on the PTM Capabilities register done in pci_ptm_init().

Bjorn Helgaas (10):
PCI/PTM: Preserve PTM Root Select
PCI/PTM: Cache PTM Capability offset
PCI/PTM: Add pci_upstream_ptm() helper
PCI/PTM: Separate configuration and enable PCI/PTM: Add pci_disable_ptm() wrapper
PCI/PTM: Add pci_enable_ptm() wrapper
PCI/PTM: Add suspend/resume
PCI/PTM: Move pci_ptm_info() body into its only caller PCI/PTM: Reorder functions in logical order PCI/PM: Always disable PTM for all devices during suspend

 drivers/pci/pci-driver.c |  11 ++
 drivers/pci/pci.c        |  28 +---
 drivers/pci/pci.h        |   6 +-
 drivers/pci/pcie/ptm.c   | 317 ++++++++++++++++++++-------------------
 include/linux/pci.h      |   3 +
 5 files changed, 181 insertions(+), 184 deletions(-)

Signature · 1 line

2.25.1

Bjorn Helgaas

author

2022-09-06 22:24 UTC #1 lore raw

PATCH v3 1/10 PCI/PTM: Preserve PTM Root Select

From: Bjorn Helgaas <[email protected]>

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 <[email protected]>

Cc David E. Box <[email protected]>

---
 drivers/pci/pcie/ptm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/pci/pcie/ptm.c +1 -1

index 368a254e3124..b6a417247ce3 100644
@@ -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);
 }

Signature · 1 line

2.25.1

Bjorn Helgaas

author

2022-09-06 22:24 UTC #2 lore raw

PATCH v3 2/10 PCI/PTM: Cache PTM Capability offset

In reply to Bjorn Helgaas

From: Bjorn Helgaas <[email protected]>

Cache the PTM Capability offset instead of searching for it every time we enable/disable PTM or save/restore PTM state.

Signed-off-by Bjorn Helgaas <[email protected]>

---
 drivers/pci/pcie/ptm.c | 41 +++++++++++++++++------------------------
 include/linux/pci.h    |  1 +
 2 files changed, 18 insertions(+), 24 deletions(-)

drivers/pci/pcie/ptm.c +17 ~~-24~~

index b6a417247ce3..6ac7ff48be57 100644
@@ -31,13 +31,9 @@ static void pci_ptm_info(struct pci_dev *dev)
 
 void pci_disable_ptm(struct pci_dev *dev)
 {
-	int ptm;
+	int ptm = dev->ptm_cap;
 	u16 ctrl;
 
-	if (!pci_is_pcie(dev))
-		return;
-
-	ptm = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_PTM);
 	if (!ptm)
 		return;

@@ -48,14 +44,10 @@ void pci_disable_ptm(struct pci_dev *dev)
 
 void pci_save_ptm_state(struct pci_dev *dev)
 {
-	int ptm;
+	int ptm = dev->ptm_cap;
 	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;

@@ -69,16 +61,15 @@ void pci_save_ptm_state(struct pci_dev *dev)
 
 void pci_restore_ptm_state(struct pci_dev *dev)
 {
+	int ptm = dev->ptm_cap;
 	struct pci_cap_saved_state *save_state;
-	int ptm;
 	u16 *cap;
 
-	if (!pci_is_pcie(dev))
+	if (!ptm)
 		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)
+	if (!save_state)
 		return;
 
 	cap = (u16 *)&save_state->cap.data[0];

@@ -87,7 +78,7 @@ void pci_restore_ptm_state(struct pci_dev *dev)
 
 void pci_ptm_init(struct pci_dev *dev)
 {
-	int pos;
+	int ptm;
 	u32 cap, ctrl;
 	u8 local_clock;
 	struct pci_dev *ups;

@@ -117,13 +108,14 @@ void pci_ptm_init(struct pci_dev *dev)
 		return;
 	}
 
-	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_PTM);
-	if (!pos)
+	ptm = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_PTM);
+	if (!ptm)
 		return;
 
+	dev->ptm_cap = ptm;
 	pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_PTM, sizeof(u16));
 
-	pci_read_config_dword(dev, pos + PCI_PTM_CAP, &cap);
+	pci_read_config_dword(dev, ptm + PCI_PTM_CAP, &cap);
 	local_clock = (cap & PCI_PTM_GRANULARITY_MASK) >> 8;
 
 	/*

@@ -148,7 +140,7 @@ void pci_ptm_init(struct pci_dev *dev)
 	}
 
 	ctrl |= dev->ptm_granularity << 8;
-	pci_write_config_dword(dev, pos + PCI_PTM_CTRL, ctrl);
+	pci_write_config_dword(dev, ptm + PCI_PTM_CTRL, ctrl);
 	dev->ptm_enabled = 1;
 
 	pci_ptm_info(dev);

@@ -156,18 +148,19 @@ void pci_ptm_init(struct pci_dev *dev)
 
 int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
 {
-	int pos;
+	int ptm;
 	u32 cap, ctrl;
 	struct pci_dev *ups;
 
 	if (!pci_is_pcie(dev))
 		return -EINVAL;
 
-	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_PTM);
-	if (!pos)
+	ptm = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_PTM);
+	if (!ptm)
 		return -EINVAL;
 
-	pci_read_config_dword(dev, pos + PCI_PTM_CAP, &cap);
+	dev->ptm_cap = ptm;
+	pci_read_config_dword(dev, ptm + PCI_PTM_CAP, &cap);
 	if (!(cap & PCI_PTM_CAP_REQ))
 		return -EINVAL;

@@ -192,7 +185,7 @@ int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
 
 	ctrl = PCI_PTM_CTRL_ENABLE;
 	ctrl |= dev->ptm_granularity << 8;
-	pci_write_config_dword(dev, pos + PCI_PTM_CTRL, ctrl);
+	pci_write_config_dword(dev, ptm + PCI_PTM_CTRL, ctrl);
 	dev->ptm_enabled = 1;
 
 	pci_ptm_info(dev);

include/linux/pci.h +1 -0

index 060af91bafcd..54be939023a3 100644
@@ -475,6 +475,7 @@ struct pci_dev {
 	unsigned int	broken_cmd_compl:1;	/* No compl for some cmds */
 #endif
 #ifdef CONFIG_PCIE_PTM
+	u16		ptm_cap;		/* PTM Capability */
 	unsigned int	ptm_root:1;
 	unsigned int	ptm_enabled:1;
 	u8		ptm_granularity;

Signature · 1 line

2.25.1

Sathyanarayanan Kuppuswamy

2022-09-06 23:18 UTC #3 lore raw

Re: [PATCH v3 02/10] PCI/PTM: Cache PTM Capability offset

Hi,

Quoted 16 lines · Bjorn Helgaas

From: Bjorn Helgaas <[email protected]>
Cache the PTM Capability offset instead of searching for it every time we enable/disable PTM or save/restore PTM state.
Signed-off-by Bjorn Helgaas <[email protected]>
---
 drivers/pci/pcie/ptm.c | 41 +++++++++++++++++------------------------
 include/linux/pci.h    |  1 +
 2 files changed, 18 insertions(+), 24 deletions(-)
drivers/pci/pcie/ptm.c +0 -0
index b6a417247ce3..6ac7ff48be57 100644

drivers/pci/pcie/ptm.c static void pci_ptm_info(struct pci_dev *dev) · line 34

Quoted 5 lines

drivers/pci/pcie/ptm.c +1 -1

@@ -31,13 +31,9 @@ static void pci_ptm_info(struct pci_dev *dev)
 
 void pci_disable_ptm(struct pci_dev *dev)
 {
-	int ptm;
+	int ptm = dev->ptm_cap;

I think you don't need to store it. Directly use dev->ptm?

Quoted 10 lines

drivers/pci/pcie/ptm.c +0 -4

 	u16 ctrl;
 
-	if (!pci_is_pcie(dev))
-		return;
-
-	ptm = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_PTM);
 	if (!ptm)
 		return;

drivers/pci/pcie/ptm.c void pci_disable_ptm(struct pci_dev *dev) · line 47

Quoted 5 lines

drivers/pci/pcie/ptm.c +1 -1

@@ -48,14 +44,10 @@ void pci_disable_ptm(struct pci_dev *dev)
 
 void pci_save_ptm_state(struct pci_dev *dev)
 {
-	int ptm;
+	int ptm = dev->ptm_cap;

Same as above.

Quoted 11 lines

drivers/pci/pcie/ptm.c +0 -4

 	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;

drivers/pci/pcie/ptm.c void pci_save_ptm_state(struct pci_dev *dev) · line 64

Quoted 4 lines

drivers/pci/pcie/ptm.c +1 -0

@@ -69,16 +61,15 @@ void pci_save_ptm_state(struct pci_dev *dev)
 
 void pci_restore_ptm_state(struct pci_dev *dev)
 {
+	int ptm = dev->ptm_cap;

It can be u16?

Quoted 16 lines

drivers/pci/pcie/ptm.c +2 -4

 	struct pci_cap_saved_state *save_state;
-	int ptm;
 	u16 *cap;
 
-	if (!pci_is_pcie(dev))
+	if (!ptm)
 		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)
+	if (!save_state)
 		return;
 
 	cap = (u16 *)&save_state->cap.data[0];

drivers/pci/pcie/ptm.c void pci_restore_ptm_state(struct pci_dev *dev) · line 81

Quoted 5 lines

drivers/pci/pcie/ptm.c +1 -1

@@ -87,7 +78,7 @@ void pci_restore_ptm_state(struct pci_dev *dev)
 
 void pci_ptm_init(struct pci_dev *dev)
 {
-	int pos;
+	int ptm;

Why rename? Also ptm can be u16

Quoted 75 lines

drivers/pci/pcie/ptm.c +11 -9

 	u32 cap, ctrl;
 	u8 local_clock;
 	struct pci_dev *ups;

@@ -117,13 +108,14 @@ void pci_ptm_init(struct pci_dev *dev)
 		return;
 	}
 
-	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_PTM);
-	if (!pos)
+	ptm = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_PTM);
+	if (!ptm)
 		return;
 
+	dev->ptm_cap = ptm;
 	pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_PTM, sizeof(u16));
 
-	pci_read_config_dword(dev, pos + PCI_PTM_CAP, &cap);
+	pci_read_config_dword(dev, ptm + PCI_PTM_CAP, &cap);
 	local_clock = (cap & PCI_PTM_GRANULARITY_MASK) >> 8;
 
 	/*

@@ -148,7 +140,7 @@ void pci_ptm_init(struct pci_dev *dev)
 	}
 
 	ctrl |= dev->ptm_granularity << 8;
-	pci_write_config_dword(dev, pos + PCI_PTM_CTRL, ctrl);
+	pci_write_config_dword(dev, ptm + PCI_PTM_CTRL, ctrl);
 	dev->ptm_enabled = 1;
 
 	pci_ptm_info(dev);

@@ -156,18 +148,19 @@ void pci_ptm_init(struct pci_dev *dev)
 
 int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
 {
-	int pos;
+	int ptm;
 	u32 cap, ctrl;
 	struct pci_dev *ups;
 
 	if (!pci_is_pcie(dev))
 		return -EINVAL;
 
-	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_PTM);
-	if (!pos)
+	ptm = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_PTM);
+	if (!ptm)
 		return -EINVAL;
 
-	pci_read_config_dword(dev, pos + PCI_PTM_CAP, &cap);
+	dev->ptm_cap = ptm;
+	pci_read_config_dword(dev, ptm + PCI_PTM_CAP, &cap);
 	if (!(cap & PCI_PTM_CAP_REQ))
 		return -EINVAL;

@@ -192,7 +185,7 @@ int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
 
 	ctrl = PCI_PTM_CTRL_ENABLE;
 	ctrl |= dev->ptm_granularity << 8;
-	pci_write_config_dword(dev, pos + PCI_PTM_CTRL, ctrl);
+	pci_write_config_dword(dev, ptm + PCI_PTM_CTRL, ctrl);
 	dev->ptm_enabled = 1;
 
 	pci_ptm_info(dev);

include/linux/pci.h +1 -0

index 060af91bafcd..54be939023a3 100644
@@ -475,6 +475,7 @@ struct pci_dev {
 	unsigned int	broken_cmd_compl:1;	/* No compl for some cmds */
 #endif
 #ifdef CONFIG_PCIE_PTM
+	u16		ptm_cap;		/* PTM Capability */
 	unsigned int	ptm_root:1;
 	unsigned int	ptm_enabled:1;
 	u8		ptm_granularity;

Signature · 2 lines

Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Bjorn Helgaas

author

2022-09-07 15:50 UTC #4 lore raw

Re: [PATCH v3 02/10] PCI/PTM: Cache PTM Capability offset

Quoted 8 lines · Sathyanarayanan Kuppuswamy

Review
Quoted 4 lines · Bjorn Helgaas
void pci_disable_ptm(struct pci_dev *dev)
{
(quoted diff) +1 -1
-	int ptm;
+	int ptm = dev->ptm_cap;
I think you don't need to store it. Directly use dev->ptm?

True, no need, but the value is used three times in this function, so I think the variable reduces clutter overall.

Quoted 5 lines

Review
Quoted 3 lines
void pci_restore_ptm_state(struct pci_dev *dev)
{
(quoted diff) +1 -0
+	int ptm = dev->ptm_cap;
It can be u16?

Done, thanks! I see that in ee8b1c478a9f ("PCI: Return u16 from pci_find_ext_capability() and similar"), I forgot to change the inline stub from int to u16. I'll add a patch to do that. Probably not a prerequisite, since the stub is for !CONFIG_PCI and this code won't be compiled at all in that case.

Quoted 6 lines

Review
Quoted 4 lines
void pci_ptm_init(struct pci_dev *dev)
{
(quoted diff) +1 -1
-	int pos;
+	int ptm;
Why rename? Also ptm can be u16

"ptm" conveys more information than "pos" and I think it's worth using the same name for all the functions.

Thank you!

Bjorn

Bjorn Helgaas

author

2022-09-06 22:24 UTC #5 lore raw

PATCH v3 3/10 PCI/PTM: Add pci_upstream_ptm() helper

In reply to Bjorn Helgaas

From: Bjorn Helgaas <[email protected]>

PTM requires an unbroken path of PTM-supporting devices between the PTM Root and the ultimate PTM Requester, but if a Switch supports PTM, only the Upstream Port can have a PTM Capability; the Downstream Ports do not.

Previously we copied the PTM configuration from the Switch Upstream Port to the Downstream Ports so dev->ptm_enabled for any device implied that all the upstream devices support PTM.

Instead of making it look like Downstream Ports have their own PTM config, add pci_upstream_ptm(), which returns the upstream device that has a PTM Capability (either a Root Port or a Switch Upstream Port).

Signed-off-by Bjorn Helgaas <[email protected]>

---
 drivers/pci/pcie/ptm.c | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

drivers/pci/pcie/ptm.c +25 ~~-14~~

index 6ac7ff48be57..8729c3e452ee 100644
@@ -76,6 +76,29 @@ void pci_restore_ptm_state(struct pci_dev *dev)
 	pci_write_config_word(dev, ptm + PCI_PTM_CTRL, *cap);
 }
 
+/*
+ * If the next upstream device supports PTM, return it; otherwise return
+ * NULL.  PTM Messages are local, so both link partners must support it.
+ */
+static struct pci_dev *pci_upstream_ptm(struct pci_dev *dev)
+{
+	struct pci_dev *ups = pci_upstream_bridge(dev);
+
+	/*
+	 * Switch Downstream Ports are not permitted to have a PTM
+	 * capability; their PTM behavior is controlled by the Upstream
+	 * Port (PCIe r5.0, sec 7.9.16), so if the upstream bridge is a
+	 * Switch Downstream Port, look up one more level.
+	 */
+	if (ups && pci_pcie_type(ups) == PCI_EXP_TYPE_DOWNSTREAM)
+		ups = pci_upstream_bridge(ups);
+
+	if (ups && ups->ptm_cap)
+		return ups;
+
+	return NULL;
+}
+
 void pci_ptm_init(struct pci_dev *dev)
 {
 	int ptm;

@@ -95,19 +118,6 @@ void pci_ptm_init(struct pci_dev *dev)
 	     pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END))
 		return;
 
-	/*
-	 * Switch Downstream Ports are not permitted to have a PTM
-	 * capability; their PTM behavior is controlled by the Upstream
-	 * Port (PCIe r5.0, sec 7.9.16).
-	 */
-	ups = pci_upstream_bridge(dev);
-	if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM &&
-	    ups && ups->ptm_enabled) {
-		dev->ptm_granularity = ups->ptm_granularity;
-		dev->ptm_enabled = 1;
-		return;
-	}
-
 	ptm = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_PTM);
 	if (!ptm)
 		return;

@@ -124,6 +134,7 @@ void pci_ptm_init(struct pci_dev *dev)
 	 * the spec recommendation (PCIe r3.1, sec 7.32.3), select the
 	 * furthest upstream Time Source as the PTM Root.
 	 */
+	ups = pci_upstream_ptm(dev);
 	if (ups && ups->ptm_enabled) {
 		ctrl = PCI_PTM_CTRL_ENABLE;
 		if (ups->ptm_granularity == 0)

@@ -173,7 +184,7 @@ int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
 	 * associate the endpoint with a time source.
 	 */
 	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ENDPOINT) {
-		ups = pci_upstream_bridge(dev);
+		ups = pci_upstream_ptm(dev);
 		if (!ups || !ups->ptm_enabled)
 			return -EINVAL;

Signature · 1 line

2.25.1

Bjorn Helgaas

author

2022-09-06 22:24 UTC #6 lore raw

PATCH v3 4/10 PCI/PTM: Separate configuration and enable

In reply to Bjorn Helgaas

From: Bjorn Helgaas <[email protected]>

PTM configuration and enabling were previously mixed together:
pci_ptm_init() collected granularity info and enabled PTM for Root Ports and Switch Upstream Ports; pci_enable_ptm() did the same for Endpoints.

Move everything related to the PTM Capability register to pci_ptm_init() for all devices, and everything related to the PTM Control register to pci_enable_ptm().

Signed-off-by Bjorn Helgaas <[email protected]>

---
 drivers/pci/pcie/ptm.c | 81 ++++++++++++++----------------------------
 1 file changed, 27 insertions(+), 54 deletions(-)

drivers/pci/pcie/ptm.c +27 ~~-54~~

index 8729c3e452ee..ac51cd84793f 100644
@@ -102,22 +102,12 @@ static struct pci_dev *pci_upstream_ptm(struct pci_dev *dev)
 void pci_ptm_init(struct pci_dev *dev)
 {
 	int ptm;
-	u32 cap, ctrl;
-	u8 local_clock;
+	u32 cap;
 	struct pci_dev *ups;
 
 	if (!pci_is_pcie(dev))
 		return;
 
-	/*
-	 * Enable PTM only on interior devices (root ports, switch ports,
-	 * etc.) on the assumption that it causes no link traffic until an
-	 * endpoint enables it.
-	 */
-	if ((pci_pcie_type(dev) == PCI_EXP_TYPE_ENDPOINT ||
-	     pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END))
-		return;
-
 	ptm = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_PTM);
 	if (!ptm)
 		return;

@@ -126,76 +116,59 @@ void pci_ptm_init(struct pci_dev *dev)
 	pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_PTM, sizeof(u16));
 
 	pci_read_config_dword(dev, ptm + PCI_PTM_CAP, &cap);
-	local_clock = (cap & PCI_PTM_GRANULARITY_MASK) >> 8;
+	dev->ptm_granularity = (cap & PCI_PTM_GRANULARITY_MASK) >> 8;
 
 	/*
-	 * There's no point in enabling PTM unless it's enabled in the
-	 * upstream device or this device can be a PTM Root itself.  Per
-	 * the spec recommendation (PCIe r3.1, sec 7.32.3), select the
+	 * Per the spec recommendation (PCIe r6.0, sec 7.9.15.3), select the
 	 * furthest upstream Time Source as the PTM Root.
 	 */
 	ups = pci_upstream_ptm(dev);
-	if (ups && ups->ptm_enabled) {
-		ctrl = PCI_PTM_CTRL_ENABLE;
+	if (ups) {
 		if (ups->ptm_granularity == 0)
 			dev->ptm_granularity = 0;
-		else if (ups->ptm_granularity > local_clock)
+		else if (ups->ptm_granularity > dev->ptm_granularity)
 			dev->ptm_granularity = ups->ptm_granularity;
-	} else {
-		if (cap & PCI_PTM_CAP_ROOT) {
-			ctrl = PCI_PTM_CTRL_ENABLE | PCI_PTM_CTRL_ROOT;
-			dev->ptm_root = 1;
-			dev->ptm_granularity = local_clock;
-		} else
-			return;
+	} else if (cap & PCI_PTM_CAP_ROOT) {
+		dev->ptm_root = 1;
+	} else if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) {
+
+		/*
+		 * Per sec 7.9.15.3, this should be the Local Clock
+		 * Granularity of the associated Time Source.  But it
+		 * doesn't say how to find that Time Source.
+		 */
+		dev->ptm_granularity = 0;
 	}
 
-	ctrl |= dev->ptm_granularity << 8;
-	pci_write_config_dword(dev, ptm + PCI_PTM_CTRL, ctrl);
-	dev->ptm_enabled = 1;
-
-	pci_ptm_info(dev);
+	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
+	    pci_pcie_type(dev) == PCI_EXP_TYPE_UPSTREAM)
+		pci_enable_ptm(dev, NULL);
 }
 
 int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
 {
-	int ptm;
-	u32 cap, ctrl;
+	int ptm = dev->ptm_cap;
 	struct pci_dev *ups;
+	u32 ctrl;
 
-	if (!pci_is_pcie(dev))
-		return -EINVAL;
-
-	ptm = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_PTM);
 	if (!ptm)
 		return -EINVAL;
 
-	dev->ptm_cap = ptm;
-	pci_read_config_dword(dev, ptm + PCI_PTM_CAP, &cap);
-	if (!(cap & PCI_PTM_CAP_REQ))
-		return -EINVAL;
-
 	/*
-	 * For a PCIe Endpoint, PTM is only useful if the endpoint can
-	 * issue PTM requests to upstream devices that have PTM enabled.
-	 *
-	 * For Root Complex Integrated Endpoints, there is no upstream
-	 * device, so there must be some implementation-specific way to
-	 * associate the endpoint with a time source.
+	 * If this device is not a PTM Root, the upstream link partner must
+	 * have PTM enabled before we can enable PTM.
 	 */
-	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ENDPOINT) {
+	if (!dev->ptm_root) {
 		ups = pci_upstream_ptm(dev);
 		if (!ups || !ups->ptm_enabled)
 			return -EINVAL;
-
-		dev->ptm_granularity = ups->ptm_granularity;
-	} else if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) {
-		dev->ptm_granularity = 0;
-	} else
-		return -EINVAL;
+	}
 
 	ctrl = PCI_PTM_CTRL_ENABLE;
 	ctrl |= dev->ptm_granularity << 8;
+	if (dev->ptm_root)
+		ctrl |= PCI_PTM_CTRL_ROOT;
+
 	pci_write_config_dword(dev, ptm + PCI_PTM_CTRL, ctrl);
 	dev->ptm_enabled = 1;

Signature · 1 line

2.25.1

Mika Westerberg

2022-09-07 05:25 UTC #7 lore raw

Re: [PATCH v3 04/10] PCI/PTM: Separate configuration and enable

drivers/pci/pcie/ptm.c void pci_ptm_init(struct pci_dev *dev) · line 134

Quoted 2 lines · Bjorn Helgaas

drivers/pci/pcie/ptm.c +2 -0

+	} else if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) {
+

Unnecessary empty line.

Quoted 6 lines

drivers/pci/pcie/ptm.c +6 -0

+		/*
+		 * Per sec 7.9.15.3, this should be the Local Clock
+		 * Granularity of the associated Time Source.  But it
+		 * doesn't say how to find that Time Source.
+		 */
+		dev->ptm_granularity = 0;

Bjorn Helgaas

author

2022-09-06 22:24 UTC #8 lore raw

PATCH v3 5/10 PCI/PTM: Add pci_disable_ptm() wrapper

In reply to Bjorn Helgaas

From: Bjorn Helgaas <[email protected]>

Implement pci_disable_ptm() as a wrapper around an internal __pci_disable_ptm() that we can use during suspend to disable PTM without clearing dev->ptm_enabled. A future commit will re-enable PTM during resume when dev->ptm_enabled is set.

Export pci_disable_ptm(), which *does* clear dev->ptm_enabled, for use by drivers that want to disable PTM.

Signed-off-by Bjorn Helgaas <[email protected]>

---
 drivers/pci/pci.h      | 2 --
 drivers/pci/pcie/ptm.c | 9 ++++++++-
 include/linux/pci.h    | 2 ++
 3 files changed, 10 insertions(+), 3 deletions(-)

drivers/pci/pci.h +0 -2

index 785f31086313..91a465460d0f 100644
@@ -507,11 +507,9 @@ 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 *);

drivers/pci/pcie/ptm.c +8 -1

index ac51cd84793f..762299984469 100644
@@ -29,7 +29,7 @@ static void pci_ptm_info(struct pci_dev *dev)
 		 dev->ptm_root ? " (root)" : "", clock_desc);
 }
 
-void pci_disable_ptm(struct pci_dev *dev)
+static void __pci_disable_ptm(struct pci_dev *dev)
 {
 	int ptm = dev->ptm_cap;
 	u16 ctrl;

@@ -42,6 +42,13 @@ void pci_disable_ptm(struct pci_dev *dev)
 	pci_write_config_word(dev, ptm + PCI_PTM_CTRL, ctrl);
 }
 
+void pci_disable_ptm(struct pci_dev *dev)
+{
+	__pci_disable_ptm(dev);
+	dev->ptm_enabled = 0;
+}
+EXPORT_SYMBOL(pci_disable_ptm);
+
 void pci_save_ptm_state(struct pci_dev *dev)
 {
 	int ptm = dev->ptm_cap;

include/linux/pci.h +2 -0

index 54be939023a3..cb5f796e3319 100644
@@ -1678,10 +1678,12 @@ bool pci_ats_disabled(void);
 
 #ifdef CONFIG_PCIE_PTM
 int pci_enable_ptm(struct pci_dev *dev, u8 *granularity);
+void pci_disable_ptm(struct pci_dev *dev);
 bool pcie_ptm_enabled(struct pci_dev *dev);
 #else
 static inline int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
 { return -EINVAL; }
+static inline void pci_disable_ptm(struct pci_dev *dev) { }
 static inline bool pcie_ptm_enabled(struct pci_dev *dev)
 { return false; }
 #endif

Signature · 1 line

2.25.1

Mika Westerberg

2022-09-07 05:28 UTC #9 lore raw

Re: [PATCH v3 05/10] PCI/PTM: Add pci_disable_ptm() wrapper

drivers/pci/pcie/ptm.c void pci_disable_ptm(struct pci_dev *dev) · line 43

Quoted 3 lines · Bjorn Helgaas

drivers/pci/pcie/ptm.c +0 -0

@@ -42,6 +42,13 @@ void pci_disable_ptm(struct pci_dev *dev)
 	pci_write_config_word(dev, ptm + PCI_PTM_CTRL, ctrl);
 }

Since you export these, I suggest adding kernel-doc to explain how these are supposed to be used in drivers (pre-conditions etc.).

drivers/pci/pcie/ptm.c void pci_disable_ptm(struct pci_dev *dev) · line 50

Quoted 6 lines

drivers/pci/pcie/ptm.c +6 -0

+void pci_disable_ptm(struct pci_dev *dev)
+{
+	__pci_disable_ptm(dev);
+	dev->ptm_enabled = 0;
+}
+EXPORT_SYMBOL(pci_disable_ptm);

EXPORT_SYMBOL_GPL()?

Bjorn Helgaas

author

2022-09-07 21:12 UTC #10 lore raw

Re: [PATCH v3 05/10] PCI/PTM: Add pci_disable_ptm() wrapper

Quoted 7 lines · Mika Westerberg

drivers/pci/pcie/ptm.c void pci_disable_ptm(struct pci_dev *dev) · line 43
Quoted 3 lines · Bjorn Helgaas
drivers/pci/pcie/ptm.c +0 -0
@@ -42,6 +42,13 @@ void pci_disable_ptm(struct pci_dev *dev)
 	pci_write_config_word(dev, ptm + PCI_PTM_CTRL, ctrl);
 }
Since you export these, I suggest adding kernel-doc to explain how these are supposed to be used in drivers (pre-conditions etc.).

Currently there really aren't any preconditions, so kernel-doc would repeat the function name and parameters without adding any real information, but I think it would be good to add a few explanatory comments. It always seems obvious when writing it, but it's never so obvious without all the context ;)

Quoted 8 lines

drivers/pci/pcie/ptm.c void pci_disable_ptm(struct pci_dev *dev) · line 50
Quoted 6 lines
drivers/pci/pcie/ptm.c +6 -0
+void pci_disable_ptm(struct pci_dev *dev)
+{
+	__pci_disable_ptm(dev);
+	dev->ptm_enabled = 0;
+}
+EXPORT_SYMBOL(pci_disable_ptm);
EXPORT_SYMBOL_GPL()?

I don't feel strongly either way, but am inclined to do the same as pci_enable_ptm() and pcie_ptm_enabled(), which are both EXPORT_SYMBOL. We could change all of them at once if it's worthwhile. Currently there's only one caller (igc) in the tree.

Bjorn

Bjorn Helgaas

author

2022-09-06 22:24 UTC #11 lore raw

PATCH v3 6/10 PCI/PTM: Add pci_enable_ptm() wrapper

In reply to Bjorn Helgaas

From: Bjorn Helgaas <[email protected]>

Implement pci_enable_ptm() as a wrapper around an internal __pci_enable_ptm() that we can use during resume to enable PTM without emitting log messages.

Signed-off-by Bjorn Helgaas <[email protected]>

---
 drivers/pci/pcie/ptm.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

drivers/pci/pcie/ptm.c +12 -1

index 762299984469..4a9f045126ca 100644
@@ -152,7 +152,7 @@ void pci_ptm_init(struct pci_dev *dev)
 		pci_enable_ptm(dev, NULL);
 }
 
-int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
+static int __pci_enable_ptm(struct pci_dev *dev)
 {
 	int ptm = dev->ptm_cap;
 	struct pci_dev *ups;

@@ -177,6 +177,17 @@ int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
 		ctrl |= PCI_PTM_CTRL_ROOT;
 
 	pci_write_config_dword(dev, ptm + PCI_PTM_CTRL, ctrl);
+	return 0;
+}
+
+int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
+{
+	int rc;
+
+	rc = __pci_enable_ptm(dev);
+	if (rc)
+		return rc;
+
 	dev->ptm_enabled = 1;
 
 	pci_ptm_info(dev);

Signature · 1 line

2.25.1

Mika Westerberg

2022-09-07 05:29 UTC #12 lore raw

Re: [PATCH v3 06/10] PCI/PTM: Add pci_enable_ptm() wrapper

Quoted 25 lines · Bjorn Helgaas

From: Bjorn Helgaas <[email protected]>
Implement pci_enable_ptm() as a wrapper around an internal __pci_enable_ptm() that we can use during resume to enable PTM without emitting log messages.
Signed-off-by Bjorn Helgaas <[email protected]>
---
 drivers/pci/pcie/ptm.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
drivers/pci/pcie/ptm.c +1 -1
index 762299984469..4a9f045126ca 100644
@@ -152,7 +152,7 @@ void pci_ptm_init(struct pci_dev *dev)
 		pci_enable_ptm(dev, NULL);
 }
 
-int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
+static int __pci_enable_ptm(struct pci_dev *dev)
 {
 	int ptm = dev->ptm_cap;
 	struct pci_dev *ups;

drivers/pci/pcie/ptm.c int pci_enable_ptm(struct pci_dev *dev, u8 *granularity) · line 182

Quoted 6 lines

drivers/pci/pcie/ptm.c +3 -0

@@ -177,6 +177,17 @@ int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
 		ctrl |= PCI_PTM_CTRL_ROOT;
 
 	pci_write_config_dword(dev, ptm + PCI_PTM_CTRL, ctrl);
+	return 0;
+}
+

Same comment here about kernel-doc.

Quoted 13 lines

drivers/pci/pcie/ptm.c +8 -0

+int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
+{
+	int rc;
+
+	rc = __pci_enable_ptm(dev);
+	if (rc)
+		return rc;
+
 	dev->ptm_enabled = 1;
 
 	pci_ptm_info(dev);

Signature · 1 line

2.25.1

Sathyanarayanan Kuppuswamy

2022-09-08 20:18 UTC #13 lore raw

Re: [PATCH v3 06/10] PCI/PTM: Add pci_enable_ptm() wrapper

In reply to Bjorn Helgaas

Quoted 5 lines · Bjorn Helgaas

From: Bjorn Helgaas <[email protected]>
Implement pci_enable_ptm() as a wrapper around an internal __pci_enable_ptm() that we can use during resume to enable PTM without emitting log messages.

Also add a note about not updating ptm_enabled in __pci_enable_ptm()

Quoted 37 lines

Signed-off-by Bjorn Helgaas <[email protected]>

---
 drivers/pci/pcie/ptm.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

drivers/pci/pcie/ptm.c +12 -1

index 762299984469..4a9f045126ca 100644
@@ -152,7 +152,7 @@ void pci_ptm_init(struct pci_dev *dev)
 		pci_enable_ptm(dev, NULL);
 }
 
-int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
+static int __pci_enable_ptm(struct pci_dev *dev)
 {
 	int ptm = dev->ptm_cap;
 	struct pci_dev *ups;

@@ -177,6 +177,17 @@ int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
 		ctrl |= PCI_PTM_CTRL_ROOT;
 
 	pci_write_config_dword(dev, ptm + PCI_PTM_CTRL, ctrl);
+	return 0;
+}
+
+int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
+{
+	int rc;
+
+	rc = __pci_enable_ptm(dev);
+	if (rc)
+		return rc;
+
 	dev->ptm_enabled = 1;
 
 	pci_ptm_info(dev);

Signature · 2 lines

Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Bjorn Helgaas

author

2022-09-06 22:24 UTC #14 lore raw

PATCH v3 7/10 PCI/PTM: Add suspend/resume

In reply to Bjorn Helgaas

From: Bjorn Helgaas <[email protected]>

---
 drivers/pci/pci.c      |  4 ++--
 drivers/pci/pci.h      |  4 ++++
 drivers/pci/pcie/ptm.c | 15 +++++++++++++++
 3 files changed, 21 insertions(+), 2 deletions(-)

drivers/pci/pci.c +2 -2

index 95bc329e74c0..83818f81577d 100644
@@ -2714,7 +2714,7 @@ int pci_prepare_to_sleep(struct pci_dev *dev)
 	 * lower-power idle state as a whole.
 	 */
 	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT)
-		pci_disable_ptm(dev);
+		pci_suspend_ptm(dev);
 
 	pci_enable_wake(dev, target_state, wakeup);

@@ -2772,7 +2772,7 @@ int pci_finish_runtime_suspend(struct pci_dev *dev)
 	 * lower-power idle state as a whole.
 	 */
 	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT)
-		pci_disable_ptm(dev);
+		pci_suspend_ptm(dev);
 
 	__pci_enable_wake(dev, target_state, pci_dev_run_wake(dev));

drivers/pci/pci.h +4 -0

index 91a465460d0f..ce4a277e3f41 100644
@@ -507,9 +507,13 @@ 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_suspend_ptm(struct pci_dev *dev);
+void pci_resume_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_suspend_ptm(struct pci_dev *dev) { }
+static inline void pci_resume_ptm(struct pci_dev *dev) { }
 #endif
 
 unsigned long pci_cardbus_resource_alignment(struct resource *);

drivers/pci/pcie/ptm.c +15 -0

index 4a9f045126ca..8ac844212436 100644
@@ -198,6 +198,21 @@ int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
 }
 EXPORT_SYMBOL(pci_enable_ptm);
 
+/*
+ * Disable PTM, but leave dev->ptm_enabled so we silently re-enable it on
+ * resume.
+ */
+void pci_suspend_ptm(struct pci_dev *dev)
+{
+	__pci_disable_ptm(dev);
+}
+
+void pci_resume_ptm(struct pci_dev *dev)
+{
+	if (dev->ptm_enabled)
+		__pci_enable_ptm(dev);
+}
+
 bool pcie_ptm_enabled(struct pci_dev *dev)
 {
 	if (!dev)

Signature · 1 line

2.25.1

Mika Westerberg

2022-09-07 05:30 UTC #15 lore raw

Re: [PATCH v3 07/10] PCI/PTM: Add suspend/resume

Quoted 2 lines · Bjorn Helgaas

From: Bjorn Helgaas <[email protected]>

I think it is good to explain here why this patch is needed. Even if just one sentence.

Quoted 51 lines

---
 drivers/pci/pci.c      |  4 ++--
 drivers/pci/pci.h      |  4 ++++
 drivers/pci/pcie/ptm.c | 15 +++++++++++++++
 3 files changed, 21 insertions(+), 2 deletions(-)

drivers/pci/pci.c +2 -2

index 95bc329e74c0..83818f81577d 100644
@@ -2714,7 +2714,7 @@ int pci_prepare_to_sleep(struct pci_dev *dev)
 	 * lower-power idle state as a whole.
 	 */
 	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT)
-		pci_disable_ptm(dev);
+		pci_suspend_ptm(dev);
 
 	pci_enable_wake(dev, target_state, wakeup);

@@ -2772,7 +2772,7 @@ int pci_finish_runtime_suspend(struct pci_dev *dev)
 	 * lower-power idle state as a whole.
 	 */
 	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT)
-		pci_disable_ptm(dev);
+		pci_suspend_ptm(dev);
 
 	__pci_enable_wake(dev, target_state, pci_dev_run_wake(dev));

drivers/pci/pci.h +4 -0

index 91a465460d0f..ce4a277e3f41 100644
@@ -507,9 +507,13 @@ 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_suspend_ptm(struct pci_dev *dev);
+void pci_resume_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_suspend_ptm(struct pci_dev *dev) { }
+static inline void pci_resume_ptm(struct pci_dev *dev) { }
 #endif
 
 unsigned long pci_cardbus_resource_alignment(struct resource *);

drivers/pci/pcie/ptm.c +0 -0

index 4a9f045126ca..8ac844212436 100644

drivers/pci/pcie/ptm.c int pci_enable_ptm(struct pci_dev *dev, u8 *granularity) · line 204

Quoted 7 lines

drivers/pci/pcie/ptm.c +4 -0

@@ -198,6 +198,21 @@ int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
 }
 EXPORT_SYMBOL(pci_enable_ptm);
 
+/*
+ * Disable PTM, but leave dev->ptm_enabled so we silently re-enable it on
+ * resume.
+ */

Proper kernel-doc for both.

Quoted 16 lines

drivers/pci/pcie/ptm.c +11 -0

+void pci_suspend_ptm(struct pci_dev *dev)
+{
+	__pci_disable_ptm(dev);
+}
+
+void pci_resume_ptm(struct pci_dev *dev)
+{
+	if (dev->ptm_enabled)
+		__pci_enable_ptm(dev);
+}
+
 bool pcie_ptm_enabled(struct pci_dev *dev)
 {
 	if (!dev)

Signature · 1 line

2.25.1

Bjorn Helgaas

author

2022-09-07 16:45 UTC #16 lore raw

Re: [PATCH v3 07/10] PCI/PTM: Add suspend/resume

Quoted 5 lines · Mika Westerberg

Quoted 1 line · Bjorn Helgaas
From: Bjorn Helgaas <[email protected]>
I think it is good to explain here why this patch is needed. Even if just one sentence.

Absolutely, sorry I missed this! I see I also forgot the SoB.

Bjorn Helgaas

author

2022-09-06 22:24 UTC #17 lore raw

PATCH v3 8/10 PCI/PTM: Move pci_ptm_info() body into its only caller

In reply to Bjorn Helgaas

From: Bjorn Helgaas <[email protected]>

pci_ptm_info() is simple and is only called by pci_enable_ptm(). Move the entire body there.

Signed-off-by Bjorn Helgaas <[email protected]>

---
 drivers/pci/pcie/ptm.c | 39 +++++++++++++++++----------------------
 1 file changed, 17 insertions(+), 22 deletions(-)

drivers/pci/pcie/ptm.c +17 ~~-22~~

index 8ac844212436..d65f5af9700d 100644
@@ -9,26 +9,6 @@
 #include <linux/pci.h>
 #include "../pci.h"
 
-static void pci_ptm_info(struct pci_dev *dev)
-{
-	char clock_desc[8];
-
-	switch (dev->ptm_granularity) {
-	case 0:
-		snprintf(clock_desc, sizeof(clock_desc), "unknown");
-		break;
-	case 255:
-		snprintf(clock_desc, sizeof(clock_desc), ">254ns");
-		break;
-	default:
-		snprintf(clock_desc, sizeof(clock_desc), "%uns",
-			 dev->ptm_granularity);
-		break;
-	}
-	pci_info(dev, "PTM enabled%s, %s granularity\n",
-		 dev->ptm_root ? " (root)" : "", clock_desc);
-}
-
 static void __pci_disable_ptm(struct pci_dev *dev)
 {
 	int ptm = dev->ptm_cap;

@@ -183,6 +163,7 @@ static int __pci_enable_ptm(struct pci_dev *dev)
 int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
 {
 	int rc;
+	char clock_desc[8];
 
 	rc = __pci_enable_ptm(dev);
 	if (rc)

@@ -190,10 +171,24 @@ int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
 
 	dev->ptm_enabled = 1;
 
-	pci_ptm_info(dev);
-
 	if (granularity)
 		*granularity = dev->ptm_granularity;
+
+	switch (dev->ptm_granularity) {
+	case 0:
+		snprintf(clock_desc, sizeof(clock_desc), "unknown");
+		break;
+	case 255:
+		snprintf(clock_desc, sizeof(clock_desc), ">254ns");
+		break;
+	default:
+		snprintf(clock_desc, sizeof(clock_desc), "%uns",
+			 dev->ptm_granularity);
+		break;
+	}
+	pci_info(dev, "PTM enabled%s, %s granularity\n",
+		 dev->ptm_root ? " (root)" : "", clock_desc);
+
 	return 0;
 }
 EXPORT_SYMBOL(pci_enable_ptm);

Signature · 1 line

2.25.1

Bjorn Helgaas

author

2022-09-06 22:24 UTC #18 lore raw

PATCH v3 9/10 PCI/PTM: Reorder functions in logical order

In reply to Bjorn Helgaas

From: Bjorn Helgaas <[email protected]>

pci_enable_ptm() and pci_disable_ptm() were separated. pci_save_ptm_state() and pci_restore_ptm_state() dangled at the top. Move them to logical places.

Signed-off-by Bjorn Helgaas <[email protected]>

---
 drivers/pci/pcie/ptm.c | 108 ++++++++++++++++++++---------------------
 1 file changed, 54 insertions(+), 54 deletions(-)

drivers/pci/pcie/ptm.c +54 ~~-54~~

index d65f5af9700d..6c09e00a7b51 100644
@@ -9,60 +9,6 @@
 #include <linux/pci.h>
 #include "../pci.h"
 
-static void __pci_disable_ptm(struct pci_dev *dev)
-{
-	int ptm = dev->ptm_cap;
-	u16 ctrl;
-
-	if (!ptm)
-		return;
-
-	pci_read_config_word(dev, ptm + PCI_PTM_CTRL, &ctrl);
-	ctrl &= ~PCI_PTM_CTRL_ENABLE;
-	pci_write_config_word(dev, ptm + PCI_PTM_CTRL, ctrl);
-}
-
-void pci_disable_ptm(struct pci_dev *dev)
-{
-	__pci_disable_ptm(dev);
-	dev->ptm_enabled = 0;
-}
-EXPORT_SYMBOL(pci_disable_ptm);
-
-void pci_save_ptm_state(struct pci_dev *dev)
-{
-	int ptm = dev->ptm_cap;
-	struct pci_cap_saved_state *save_state;
-	u16 *cap;
-
-	if (!ptm)
-		return;
-
-	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_PTM);
-	if (!save_state)
-		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)
-{
-	int ptm = dev->ptm_cap;
-	struct pci_cap_saved_state *save_state;
-	u16 *cap;
-
-	if (!ptm)
-		return;
-
-	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_PTM);
-	if (!save_state)
-		return;
-
-	cap = (u16 *)&save_state->cap.data[0];
-	pci_write_config_word(dev, ptm + PCI_PTM_CTRL, *cap);
-}
-
 /*
  * If the next upstream device supports PTM, return it; otherwise return
  * NULL.  PTM Messages are local, so both link partners must support it.

@@ -132,6 +78,40 @@ void pci_ptm_init(struct pci_dev *dev)
 		pci_enable_ptm(dev, NULL);
 }
 
+void pci_save_ptm_state(struct pci_dev *dev)
+{
+	int ptm = dev->ptm_cap;
+	struct pci_cap_saved_state *save_state;
+	u16 *cap;
+
+	if (!ptm)
+		return;
+
+	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_PTM);
+	if (!save_state)
+		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)
+{
+	int ptm = dev->ptm_cap;
+	struct pci_cap_saved_state *save_state;
+	u16 *cap;
+
+	if (!ptm)
+		return;
+
+	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_PTM);
+	if (!save_state)
+		return;
+
+	cap = (u16 *)&save_state->cap.data[0];
+	pci_write_config_word(dev, ptm + PCI_PTM_CTRL, *cap);
+}
+
 static int __pci_enable_ptm(struct pci_dev *dev)
 {
 	int ptm = dev->ptm_cap;

@@ -193,6 +173,26 @@ int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
 }
 EXPORT_SYMBOL(pci_enable_ptm);
 
+static void __pci_disable_ptm(struct pci_dev *dev)
+{
+	int ptm = dev->ptm_cap;
+	u16 ctrl;
+
+	if (!ptm)
+		return;
+
+	pci_read_config_word(dev, ptm + PCI_PTM_CTRL, &ctrl);
+	ctrl &= ~PCI_PTM_CTRL_ENABLE;
+	pci_write_config_word(dev, ptm + PCI_PTM_CTRL, ctrl);
+}
+
+void pci_disable_ptm(struct pci_dev *dev)
+{
+	__pci_disable_ptm(dev);
+	dev->ptm_enabled = 0;
+}
+EXPORT_SYMBOL(pci_disable_ptm);
+
 /*
  * Disable PTM, but leave dev->ptm_enabled so we silently re-enable it on
  * resume.

Signature · 1 line

2.25.1

Sathyanarayanan Kuppuswamy

2022-09-08 20:15 UTC #19 lore raw

Re: [PATCH v3 09/10] PCI/PTM: Reorder functions in logical order

Quoted 6 lines · Bjorn Helgaas

From: Bjorn Helgaas <[email protected]>
pci_enable_ptm() and pci_disable_ptm() were separated. pci_save_ptm_state() and pci_restore_ptm_state() dangled at the top. Move them to logical places.

Maybe add "No functional changes"? It will give clear meaning.

Quoted 138 lines

Signed-off-by Bjorn Helgaas <[email protected]>

---
 drivers/pci/pcie/ptm.c | 108 ++++++++++++++++++++---------------------
 1 file changed, 54 insertions(+), 54 deletions(-)

drivers/pci/pcie/ptm.c +54 ~~-54~~

index d65f5af9700d..6c09e00a7b51 100644
@@ -9,60 +9,6 @@
 #include <linux/pci.h>
 #include "../pci.h"
 
-static void __pci_disable_ptm(struct pci_dev *dev)
-{
-	int ptm = dev->ptm_cap;
-	u16 ctrl;
-
-	if (!ptm)
-		return;
-
-	pci_read_config_word(dev, ptm + PCI_PTM_CTRL, &ctrl);
-	ctrl &= ~PCI_PTM_CTRL_ENABLE;
-	pci_write_config_word(dev, ptm + PCI_PTM_CTRL, ctrl);
-}
-
-void pci_disable_ptm(struct pci_dev *dev)
-{
-	__pci_disable_ptm(dev);
-	dev->ptm_enabled = 0;
-}
-EXPORT_SYMBOL(pci_disable_ptm);
-
-void pci_save_ptm_state(struct pci_dev *dev)
-{
-	int ptm = dev->ptm_cap;
-	struct pci_cap_saved_state *save_state;
-	u16 *cap;
-
-	if (!ptm)
-		return;
-
-	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_PTM);
-	if (!save_state)
-		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)
-{
-	int ptm = dev->ptm_cap;
-	struct pci_cap_saved_state *save_state;
-	u16 *cap;
-
-	if (!ptm)
-		return;
-
-	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_PTM);
-	if (!save_state)
-		return;
-
-	cap = (u16 *)&save_state->cap.data[0];
-	pci_write_config_word(dev, ptm + PCI_PTM_CTRL, *cap);
-}
-
 /*
  * If the next upstream device supports PTM, return it; otherwise return
  * NULL.  PTM Messages are local, so both link partners must support it.

@@ -132,6 +78,40 @@ void pci_ptm_init(struct pci_dev *dev)
 		pci_enable_ptm(dev, NULL);
 }
 
+void pci_save_ptm_state(struct pci_dev *dev)
+{
+	int ptm = dev->ptm_cap;
+	struct pci_cap_saved_state *save_state;
+	u16 *cap;
+
+	if (!ptm)
+		return;
+
+	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_PTM);
+	if (!save_state)
+		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)
+{
+	int ptm = dev->ptm_cap;
+	struct pci_cap_saved_state *save_state;
+	u16 *cap;
+
+	if (!ptm)
+		return;
+
+	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_PTM);
+	if (!save_state)
+		return;
+
+	cap = (u16 *)&save_state->cap.data[0];
+	pci_write_config_word(dev, ptm + PCI_PTM_CTRL, *cap);
+}
+
 static int __pci_enable_ptm(struct pci_dev *dev)
 {
 	int ptm = dev->ptm_cap;

@@ -193,6 +173,26 @@ int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
 }
 EXPORT_SYMBOL(pci_enable_ptm);
 
+static void __pci_disable_ptm(struct pci_dev *dev)
+{
+	int ptm = dev->ptm_cap;
+	u16 ctrl;
+
+	if (!ptm)
+		return;
+
+	pci_read_config_word(dev, ptm + PCI_PTM_CTRL, &ctrl);
+	ctrl &= ~PCI_PTM_CTRL_ENABLE;
+	pci_write_config_word(dev, ptm + PCI_PTM_CTRL, ctrl);
+}
+
+void pci_disable_ptm(struct pci_dev *dev)
+{
+	__pci_disable_ptm(dev);
+	dev->ptm_enabled = 0;
+}
+EXPORT_SYMBOL(pci_disable_ptm);
+
 /*
  * Disable PTM, but leave dev->ptm_enabled so we silently re-enable it on
  * resume.

Signature · 2 lines

Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Bjorn Helgaas

author

2022-09-08 20:58 UTC #20 lore raw

Re: [PATCH v3 09/10] PCI/PTM: Reorder functions in logical order

Quoted 11 lines · Sathyanarayanan Kuppuswamy

Quoted 6 lines · Bjorn Helgaas
From: Bjorn Helgaas <[email protected]>
pci_enable_ptm() and pci_disable_ptm() were separated. pci_save_ptm_state() and pci_restore_ptm_state() dangled at the top. Move them to logical places.
Maybe add "No functional changes"? It will give clear meaning.

Done, thanks!

Bjorn Helgaas

author

2022-09-06 22:24 UTC #21 lore raw

PATCH v3 10/10 PCI/PM: Always disable PTM for all devices during suspend

In reply to Bjorn Helgaas

From: Bjorn Helgaas <[email protected]>

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 in pci_pm_resume() and pci_pm_runtime_resume().

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: [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

Reported-by Kai-Heng Feng <[email protected]>

Signed-off-by Bjorn Helgaas <[email protected]>

---
 drivers/pci/pci-driver.c | 11 +++++++++++
 drivers/pci/pci.c        | 28 ++--------------------------
 2 files changed, 13 insertions(+), 26 deletions(-)

drivers/pci/pci-driver.c +11 -0

index 2815922ac525..107d77f3c846 100644
@@ -774,6 +774,12 @@ static int pci_pm_suspend(struct device *dev)
 
 	pci_dev->skip_bus_pm = false;
 
+	/*
+	 * Disabling PTM allows some systems, e.g., Intel mobile chips
+	 * since Coffee Lake, to enter a lower-power PM state.
+	 */
+	pci_suspend_ptm(pci_dev);
+
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_suspend(dev, PMSG_SUSPEND);

@@ -982,6 +988,8 @@ static int pci_pm_resume(struct device *dev)
 	if (pci_dev->state_saved)
 		pci_restore_standard_config(pci_dev);
 
+	pci_resume_ptm(pci_dev);
+
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_resume(dev);

@@ -1269,6 +1277,8 @@ static int pci_pm_runtime_suspend(struct device *dev)
 	pci_power_t prev = pci_dev->current_state;
 	int error;
 
+	pci_suspend_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.

@@ -1330,6 +1340,7 @@ static int pci_pm_runtime_resume(struct device *dev)
 	 * D3cold when the bridge above it runtime suspended.
 	 */
 	pci_pm_default_resume_early(pci_dev);
+	pci_resume_ptm(pci_dev);
 
 	if (!pci_dev->driver)
 		return 0;

drivers/pci/pci.c +2 ~~-26~~

index 83818f81577d..107afa0a5b03 100644
@@ -2706,24 +2706,12 @@ 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_suspend_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;
 }

@@ -2764,24 +2752,12 @@ 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_suspend_ptm(dev);
-
 	__pci_enable_wake(dev, target_state, pci_dev_run_wake(dev));
 
 	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;
 }

Signature · 1 line

2.25.1