[PATCH 0/5] thunderbolt: Add support for runtime PM

[PATCH 0/5] thunderbolt: Add support for runtime PM

[Mika Westerberg]

Hi all,

In recent PCs such as Lenovo X1 Carbon 6th generation the Thunderbolt
controller is in RTD3 mode (Runtime D3). This is different from the
previous modes because now the controller is present most of the time (it
still will be hot-removed/hot-added during NVM firmware upgrade). Because
of that we need to dynamically power it down whenever possible to save some
power.

This patch series adds Linux runtime PM support for the Thunderbolt host
controller driver using ICM firmware but it should be generic enough for
future additions to allow similar functionality for the older Apple
hardware as well (even though those system do not support full RTD3, it
still makes it possible for the host controller to go to low power state if
cable is not connected).

With these patches the driver automatically runtime suspends the host
controller after being idle for 15s. The connected Thunderbolt devices (if
any) need to support RTD3 mode as well. Typically all 3rd generation
devices (Alpine Ridge, Titan Ridge) support this.

However, while this provides some power savings, there is more work to do
in order to allow powering down the PCIe root port leading to the
Thunderbolt PCIe hierarchy. This work is still in progress.

Mika Westerberg (5):
  thunderbolt: Use 64-bit DMA mask if supported by the platform
  thunderbolt: Do not unnecessarily call ICM get route
  thunderbolt: No need to take tb->lock in domain suspend/complete
  thunderbolt: Use correct ICM commands in system suspend
  thunderbolt: Add support for runtime PM

 drivers/thunderbolt/domain.c  |  55 ++++++++---
 drivers/thunderbolt/icm.c     | 172 ++++++++++++++++++++++++++++------
 drivers/thunderbolt/nhi.c     |  46 ++++++++-
 drivers/thunderbolt/switch.c  |  65 ++++++++++++-
 drivers/thunderbolt/tb.h      |  10 ++
 drivers/thunderbolt/tb_msgs.h |   4 +
 drivers/thunderbolt/xdomain.c |  18 ++++
 7 files changed, 323 insertions(+), 47 deletions(-)

-- 
2.17.1

[PATCH 1/5] thunderbolt: Use 64-bit DMA mask if supported by the platform

[Mika Westerberg]

PCI defaults to 32-bit DMA mask but this device is capable of full
64-bit addressing, so make sure we first try 64-bit DMA mask before
falling back to the default 32-bit.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/thunderbolt/nhi.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
index f5a33e88e676..e3b7695fe37e 100644
--- a/drivers/thunderbolt/nhi.c
+++ b/drivers/thunderbolt/nhi.c
@@ -1015,6 +1015,14 @@ static int nhi_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	spin_lock_init(&nhi->lock);
 
+	res = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
+	if (res)
+		res = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
+	if (res) {
+		dev_err(&pdev->dev, "failed to set DMA mask\n");
+		return res;
+	}
+
 	pci_set_master(pdev);
 
 	tb = icm_probe(nhi);
-- 
2.17.1

[PATCH 2/5] thunderbolt: Do not unnecessarily call ICM get route

[Mika Westerberg]

This command is not really fast and can make resume time slower. We only
need to get route again if the link was changed and during initial
device connected message.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/thunderbolt/icm.c | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c
index 500911f16498..ad4eeaa59b16 100644
--- a/drivers/thunderbolt/icm.c
+++ b/drivers/thunderbolt/icm.c
@@ -534,20 +534,13 @@ icm_fr_device_connected(struct tb *tb, const struct icm_pkg_header *hdr)
 		return;
 	}
 
-	ret = icm->get_route(tb, link, depth, &route);
-	if (ret) {
-		tb_err(tb, "failed to find route string for switch at %u.%u\n",
-		       link, depth);
-		return;
-	}
-
 	sw = tb_switch_find_by_uuid(tb, &pkg->ep_uuid);
 	if (sw) {
 		u8 phy_port, sw_phy_port;
 
 		parent_sw = tb_to_switch(sw->dev.parent);
-		sw_phy_port = phy_port_from_route(tb_route(sw), sw->depth);
-		phy_port = phy_port_from_route(route, depth);
+		sw_phy_port = tb_phy_port_from_link(sw->link);
+		phy_port = tb_phy_port_from_link(link);
 
 		/*
 		 * On resume ICM will send us connected events for the
@@ -559,6 +552,22 @@ icm_fr_device_connected(struct tb *tb, const struct icm_pkg_header *hdr)
 		 */
 		if (sw->depth == depth && sw_phy_port == phy_port &&
 		    !!sw->authorized == authorized) {
+			/*
+			 * It was enumerated through another link so update
+			 * route string accordingly.
+			 */
+			if (sw->link != link) {
+				ret = icm->get_route(tb, link, depth, &route);
+				if (ret) {
+					tb_err(tb, "failed to update route string for switch at %u.%u\n",
+					       link, depth);
+					tb_switch_put(sw);
+					return;
+				}
+			} else {
+				route = tb_route(sw);
+			}
+
 			update_switch(parent_sw, sw, route, pkg->connection_id,
 				      pkg->connection_key, link, depth, boot);
 			tb_switch_put(sw);
@@ -607,6 +616,14 @@ icm_fr_device_connected(struct tb *tb, const struct icm_pkg_header *hdr)
 		return;
 	}
 
+	ret = icm->get_route(tb, link, depth, &route);
+	if (ret) {
+		tb_err(tb, "failed to find route string for switch at %u.%u\n",
+		       link, depth);
+		tb_switch_put(parent_sw);
+		return;
+	}
+
 	add_switch(parent_sw, route, &pkg->ep_uuid, pkg->connection_id,
 		   pkg->connection_key, link, depth, security_level,
 		   authorized, boot);
-- 
2.17.1

[PATCH 3/5] thunderbolt: No need to take tb->lock in domain suspend/complete

[Mika Westerberg]

If the connection manager implementation needs to touch the domain
structures it ought to take the lock itself. Currently only ICM
implements these hooks and it does not need the lock because we there
will be no notifications before driver ready message is sent to it.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/thunderbolt/domain.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/drivers/thunderbolt/domain.c b/drivers/thunderbolt/domain.c
index 6281266b8ec0..b34e7f118fcf 100644
--- a/drivers/thunderbolt/domain.c
+++ b/drivers/thunderbolt/domain.c
@@ -505,26 +505,13 @@ int tb_domain_resume_noirq(struct tb *tb)
 
 int tb_domain_suspend(struct tb *tb)
 {
-	int ret;
-
-	mutex_lock(&tb->lock);
-	if (tb->cm_ops->suspend) {
-		ret = tb->cm_ops->suspend(tb);
-		if (ret) {
-			mutex_unlock(&tb->lock);
-			return ret;
-		}
-	}
-	mutex_unlock(&tb->lock);
-	return 0;
+	return tb->cm_ops->suspend ? tb->cm_ops->suspend(tb) : 0;
 }
 
 void tb_domain_complete(struct tb *tb)
 {
-	mutex_lock(&tb->lock);
 	if (tb->cm_ops->complete)
 		tb->cm_ops->complete(tb);
-	mutex_unlock(&tb->lock);
 }
 
 /**
-- 
2.17.1

[PATCH 4/5] thunderbolt: Use correct ICM commands in system suspend

[Mika Westerberg]

The correct way to put the ICM into suspend state is to send it
NHI_MAILBOX_DRV_UNLOADS mailbox command. NHI_MAILBOX_SAVE_DEVS is not
needed on Intel Titan Ridge so we can skip it.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/thunderbolt/icm.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c
index ad4eeaa59b16..5ed3f8a31b0a 100644
--- a/drivers/thunderbolt/icm.c
+++ b/drivers/thunderbolt/icm.c
@@ -60,6 +60,7 @@
  * @is_supported: Checks if we can support ICM on this controller
  * @get_mode: Read and return the ICM firmware mode (optional)
  * @get_route: Find a route string for given switch
+ * @save_devices: Ask ICM to save devices to ACL when suspending (optional)
  * @driver_ready: Send driver ready message to ICM
  * @device_connected: Handle device connected ICM message
  * @device_disconnected: Handle device disconnected ICM message
@@ -76,6 +77,7 @@ struct icm {
 	bool (*is_supported)(struct tb *tb);
 	int (*get_mode)(struct tb *tb);
 	int (*get_route)(struct tb *tb, u8 link, u8 depth, u64 *route);
+	void (*save_devices)(struct tb *tb);
 	int (*driver_ready)(struct tb *tb,
 			    enum tb_security_level *security_level,
 			    size_t *nboot_acl);
@@ -258,6 +260,11 @@ static int icm_fr_get_route(struct tb *tb, u8 link, u8 depth, u64 *route)
 	return ret;
 }
 
+static void icm_fr_save_devices(struct tb *tb)
+{
+	nhi_mailbox_cmd(tb->nhi, NHI_MAILBOX_SAVE_DEVS, 0);
+}
+
 static int
 icm_fr_driver_ready(struct tb *tb, enum tb_security_level *security_level,
 		    size_t *nboot_acl)
@@ -1665,13 +1672,12 @@ static int icm_driver_ready(struct tb *tb)
 
 static int icm_suspend(struct tb *tb)
 {
-	int ret;
+	struct icm *icm = tb_priv(tb);
 
-	ret = nhi_mailbox_cmd(tb->nhi, NHI_MAILBOX_SAVE_DEVS, 0);
-	if (ret)
-		tb_info(tb, "Ignoring mailbox command error (%d) in %s\n",
-			ret, __func__);
+	if (icm->save_devices)
+		icm->save_devices(tb);
 
+	nhi_mailbox_cmd(tb->nhi, NHI_MAILBOX_DRV_UNLOADS, 0);
 	return 0;
 }
 
@@ -1879,6 +1885,7 @@ struct tb *icm_probe(struct tb_nhi *nhi)
 	case PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_NHI:
 		icm->is_supported = icm_fr_is_supported;
 		icm->get_route = icm_fr_get_route;
+		icm->save_devices = icm_fr_save_devices;
 		icm->driver_ready = icm_fr_driver_ready;
 		icm->device_connected = icm_fr_device_connected;
 		icm->device_disconnected = icm_fr_device_disconnected;
@@ -1896,6 +1903,7 @@ struct tb *icm_probe(struct tb_nhi *nhi)
 		icm->is_supported = icm_ar_is_supported;
 		icm->get_mode = icm_ar_get_mode;
 		icm->get_route = icm_ar_get_route;
+		icm->save_devices = icm_fr_save_devices;
 		icm->driver_ready = icm_ar_driver_ready;
 		icm->device_connected = icm_fr_device_connected;
 		icm->device_disconnected = icm_fr_device_disconnected;
-- 
2.17.1

[PATCH 5/5] thunderbolt: Add support for runtime PM

[Mika Westerberg]

When Thunderbolt host controller is set to RTD3 mode (Runtime D3) it is
present all the time. Because of this it is important to runtime suspend
the controller whenever possible. In case of ICM we have following rules
which all needs to be true before the host controller can be put to D3:

  - The controller firmware reports to support RTD3
  - All the connected devices announce support for RTD3
  - There is no active XDomain connection

Implement this using standard Linux runtime PM APIs so that when all the
children devices are runtime suspended, the Thunderbolt host controller
PCI device is runtime suspended as well. The ICM firmware then starts
powering down power domains towards RTD3 but it can prevent this if it
detects that there is an active Display Port stream (this is not visible
to the software, though).

The Thunderbolt host controller will be runtime resumed either when
there is a remote wake event (device is connected or disconnected), or
when there is access from userspace that requires hardware access.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/thunderbolt/domain.c  |  42 +++++++++++-
 drivers/thunderbolt/icm.c     | 119 ++++++++++++++++++++++++++++++----
 drivers/thunderbolt/nhi.c     |  38 ++++++++++-
 drivers/thunderbolt/switch.c  |  65 +++++++++++++++++--
 drivers/thunderbolt/tb.h      |  10 +++
 drivers/thunderbolt/tb_msgs.h |   4 ++
 drivers/thunderbolt/xdomain.c |  18 +++++
 7 files changed, 276 insertions(+), 20 deletions(-)

diff --git a/drivers/thunderbolt/domain.c b/drivers/thunderbolt/domain.c
index b34e7f118fcf..37cf5cfc76df 100644
--- a/drivers/thunderbolt/domain.c
+++ b/drivers/thunderbolt/domain.c
@@ -12,6 +12,7 @@
 #include <linux/device.h>
 #include <linux/idr.h>
 #include <linux/module.h>
+#include <linux/pm_runtime.h>
 #include <linux/slab.h>
 #include <linux/random.h>
 #include <crypto/hash.h>
@@ -132,6 +133,8 @@ static ssize_t boot_acl_show(struct device *dev, struct device_attribute *attr,
 	if (!uuids)
 		return -ENOMEM;
 
+	pm_runtime_get_sync(&tb->dev);
+
 	if (mutex_lock_interruptible(&tb->lock)) {
 		ret = -ERESTARTSYS;
 		goto out;
@@ -153,7 +156,10 @@ static ssize_t boot_acl_show(struct device *dev, struct device_attribute *attr,
 	}
 
 out:
+	pm_runtime_mark_last_busy(&tb->dev);
+	pm_runtime_put_autosuspend(&tb->dev);
 	kfree(uuids);
+
 	return ret;
 }
 
@@ -208,13 +214,18 @@ static ssize_t boot_acl_store(struct device *dev, struct device_attribute *attr,
 		goto err_free_acl;
 	}
 
+	pm_runtime_get_sync(&tb->dev);
+
 	if (mutex_lock_interruptible(&tb->lock)) {
 		ret = -ERESTARTSYS;
-		goto err_free_acl;
+		goto err_rpm_put;
 	}
 	ret = tb->cm_ops->set_boot_acl(tb, acl, tb->nboot_acl);
 	mutex_unlock(&tb->lock);
 
+err_rpm_put:
+	pm_runtime_mark_last_busy(&tb->dev);
+	pm_runtime_put_autosuspend(&tb->dev);
 err_free_acl:
 	kfree(acl);
 err_free_str:
@@ -426,6 +437,13 @@ int tb_domain_add(struct tb *tb)
 	/* This starts event processing */
 	mutex_unlock(&tb->lock);
 
+	pm_runtime_no_callbacks(&tb->dev);
+	pm_runtime_set_active(&tb->dev);
+	pm_runtime_enable(&tb->dev);
+	pm_runtime_set_autosuspend_delay(&tb->dev, TB_AUTOSUSPEND_DELAY);
+	pm_runtime_mark_last_busy(&tb->dev);
+	pm_runtime_use_autosuspend(&tb->dev);
+
 	return 0;
 
 err_domain_del:
@@ -514,6 +532,28 @@ void tb_domain_complete(struct tb *tb)
 		tb->cm_ops->complete(tb);
 }
 
+int tb_domain_runtime_suspend(struct tb *tb)
+{
+	if (tb->cm_ops->runtime_suspend) {
+		int ret = tb->cm_ops->runtime_suspend(tb);
+		if (ret)
+			return ret;
+	}
+	tb_ctl_stop(tb->ctl);
+	return 0;
+}
+
+int tb_domain_runtime_resume(struct tb *tb)
+{
+	tb_ctl_start(tb->ctl);
+	if (tb->cm_ops->runtime_resume) {
+		int ret = tb->cm_ops->runtime_resume(tb);
+		if (ret)
+			return ret;
+	}
+	return 0;
+}
+
 /**
  * tb_domain_approve_switch() - Approve switch
  * @tb: Domain the switch belongs to
diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c
index 5ed3f8a31b0a..47f00752a3c2 100644
--- a/drivers/thunderbolt/icm.c
+++ b/drivers/thunderbolt/icm.c
@@ -15,6 +15,7 @@
 #include <linux/delay.h>
 #include <linux/mutex.h>
 #include <linux/pci.h>
+#include <linux/pm_runtime.h>
 #include <linux/platform_data/x86/apple.h>
 #include <linux/sizes.h>
 #include <linux/slab.h>
@@ -57,6 +58,7 @@
  *	     (only set when @upstream_port is not %NULL)
  * @safe_mode: ICM is in safe mode
  * @max_boot_acl: Maximum number of preboot ACL entries (%0 if not supported)
+ * @rpm: Does the controller support runtime PM (RTD3)
  * @is_supported: Checks if we can support ICM on this controller
  * @get_mode: Read and return the ICM firmware mode (optional)
  * @get_route: Find a route string for given switch
@@ -74,13 +76,14 @@ struct icm {
 	size_t max_boot_acl;
 	int vnd_cap;
 	bool safe_mode;
+	bool rpm;
 	bool (*is_supported)(struct tb *tb);
 	int (*get_mode)(struct tb *tb);
 	int (*get_route)(struct tb *tb, u8 link, u8 depth, u64 *route);
 	void (*save_devices)(struct tb *tb);
 	int (*driver_ready)(struct tb *tb,
 			    enum tb_security_level *security_level,
-			    size_t *nboot_acl);
+			    size_t *nboot_acl, bool *rpm);
 	void (*device_connected)(struct tb *tb,
 				 const struct icm_pkg_header *hdr);
 	void (*device_disconnected)(struct tb *tb,
@@ -97,6 +100,47 @@ struct icm_notification {
 	struct tb *tb;
 };
 
+struct ep_name_entry {
+	u8 len;
+	u8 type;
+	u8 data[0];
+};
+
+#define EP_NAME_INTEL_VSS	0x10
+
+/* Intel Vendor specific structure */
+struct intel_vss {
+	u16 vendor;
+	u16 model;
+	u8 mc;
+	u8 flags;
+	u16 pci_devid;
+	u32 nvm_version;
+};
+
+#define INTEL_VSS_FLAGS_RTD3	BIT(0)
+
+static const struct intel_vss *parse_intel_vss(const void *ep_name, size_t size)
+{
+	const void *end = ep_name + size;
+
+	while (ep_name < end) {
+		const struct ep_name_entry *ep = ep_name;
+
+		if (!ep->len)
+			break;
+		if (ep_name + ep->len > end)
+			break;
+
+		if (ep->type == EP_NAME_INTEL_VSS)
+			return (const struct intel_vss *)ep->data;
+
+		ep_name += ep->len;
+	}
+
+	return NULL;
+}
+
 static inline struct tb *icm_to_tb(struct icm *icm)
 {
 	return ((void *)icm - sizeof(struct tb));
@@ -267,7 +311,7 @@ static void icm_fr_save_devices(struct tb *tb)
 
 static int
 icm_fr_driver_ready(struct tb *tb, enum tb_security_level *security_level,
-		    size_t *nboot_acl)
+		    size_t *nboot_acl, bool *rpm)
 {
 	struct icm_fr_pkg_driver_ready_response reply;
 	struct icm_pkg_driver_ready request = {
@@ -417,15 +461,19 @@ static int icm_fr_disconnect_xdomain_paths(struct tb *tb, struct tb_xdomain *xd)
 }
 
 static void add_switch(struct tb_switch *parent_sw, u64 route,
-		       const uuid_t *uuid, u8 connection_id, u8 connection_key,
+		       const uuid_t *uuid, const u8 *ep_name,
+		       size_t ep_name_size, u8 connection_id, u8 connection_key,
 		       u8 link, u8 depth, enum tb_security_level security_level,
 		       bool authorized, bool boot)
 {
+	const struct intel_vss *vss;
 	struct tb_switch *sw;
 
+	pm_runtime_get_sync(&parent_sw->dev);
+
 	sw = tb_switch_alloc(parent_sw->tb, &parent_sw->dev, route);
 	if (!sw)
-		return;
+		goto out;
 
 	sw->uuid = kmemdup(uuid, sizeof(*uuid), GFP_KERNEL);
 	sw->connection_id = connection_id;
@@ -436,6 +484,10 @@ static void add_switch(struct tb_switch *parent_sw, u64 route,
 	sw->security_level = security_level;
 	sw->boot = boot;
 
+	vss = parse_intel_vss(ep_name, ep_name_size);
+	if (vss)
+		sw->rpm = !!(vss->flags & INTEL_VSS_FLAGS_RTD3);
+
 	/* Link the two switches now */
 	tb_port_at(route, parent_sw)->remote = tb_upstream_port(sw);
 	tb_upstream_port(sw)->remote = tb_port_at(route, parent_sw);
@@ -443,8 +495,11 @@ static void add_switch(struct tb_switch *parent_sw, u64 route,
 	if (tb_switch_add(sw)) {
 		tb_port_at(tb_route(sw), parent_sw)->remote = NULL;
 		tb_switch_put(sw);
-		return;
 	}
+
+out:
+	pm_runtime_mark_last_busy(&parent_sw->dev);
+	pm_runtime_put_autosuspend(&parent_sw->dev);
 }
 
 static void update_switch(struct tb_switch *parent_sw, struct tb_switch *sw,
@@ -484,9 +539,11 @@ static void add_xdomain(struct tb_switch *sw, u64 route,
 {
 	struct tb_xdomain *xd;
 
+	pm_runtime_get_sync(&sw->dev);
+
 	xd = tb_xdomain_alloc(sw->tb, &sw->dev, route, local_uuid, remote_uuid);
 	if (!xd)
-		return;
+		goto out;
 
 	xd->link = link;
 	xd->depth = depth;
@@ -494,6 +551,10 @@ static void add_xdomain(struct tb_switch *sw, u64 route,
 	tb_port_at(route, sw)->xdomain = xd;
 
 	tb_xdomain_add(xd);
+
+out:
+	pm_runtime_mark_last_busy(&sw->dev);
+	pm_runtime_put_autosuspend(&sw->dev);
 }
 
 static void update_xdomain(struct tb_xdomain *xd, u64 route, u8 link)
@@ -631,7 +692,8 @@ icm_fr_device_connected(struct tb *tb, const struct icm_pkg_header *hdr)
 		return;
 	}
 
-	add_switch(parent_sw, route, &pkg->ep_uuid, pkg->connection_id,
+	add_switch(parent_sw, route, &pkg->ep_uuid, (const u8 *)pkg->ep_name,
+		   sizeof(pkg->ep_name), pkg->connection_id,
 		   pkg->connection_key, link, depth, security_level,
 		   authorized, boot);
 
@@ -781,7 +843,7 @@ icm_fr_xdomain_disconnected(struct tb *tb, const struct icm_pkg_header *hdr)
 
 static int
 icm_tr_driver_ready(struct tb *tb, enum tb_security_level *security_level,
-		    size_t *nboot_acl)
+		    size_t *nboot_acl, bool *rpm)
 {
 	struct icm_tr_pkg_driver_ready_response reply;
 	struct icm_pkg_driver_ready request = {
@@ -800,6 +862,9 @@ icm_tr_driver_ready(struct tb *tb, enum tb_security_level *security_level,
 	if (nboot_acl)
 		*nboot_acl = (reply.info & ICM_TR_INFO_BOOT_ACL_MASK) >>
 				ICM_TR_INFO_BOOT_ACL_SHIFT;
+	if (rpm)
+		*rpm = !!(reply.hdr.flags & ICM_TR_FLAGS_RTD3);
+
 	return 0;
 }
 
@@ -1029,7 +1094,8 @@ icm_tr_device_connected(struct tb *tb, const struct icm_pkg_header *hdr)
 		return;
 	}
 
-	add_switch(parent_sw, route, &pkg->ep_uuid, pkg->connection_id,
+	add_switch(parent_sw, route, &pkg->ep_uuid, (const u8 *)pkg->ep_name,
+		   sizeof(pkg->ep_name), pkg->connection_id,
 		   0, 0, 0, security_level, authorized, boot);
 
 	tb_switch_put(parent_sw);
@@ -1208,7 +1274,7 @@ static int icm_ar_get_mode(struct tb *tb)
 
 static int
 icm_ar_driver_ready(struct tb *tb, enum tb_security_level *security_level,
-		    size_t *nboot_acl)
+		    size_t *nboot_acl, bool *rpm)
 {
 	struct icm_ar_pkg_driver_ready_response reply;
 	struct icm_pkg_driver_ready request = {
@@ -1227,6 +1293,9 @@ icm_ar_driver_ready(struct tb *tb, enum tb_security_level *security_level,
 	if (nboot_acl && (reply.info & ICM_AR_INFO_BOOT_ACL_SUPPORTED))
 		*nboot_acl = (reply.info & ICM_AR_INFO_BOOT_ACL_MASK) >>
 				ICM_AR_INFO_BOOT_ACL_SHIFT;
+	if (rpm)
+		*rpm = !!(reply.hdr.flags & ICM_AR_FLAGS_RTD3);
+
 	return 0;
 }
 
@@ -1380,13 +1449,13 @@ static void icm_handle_event(struct tb *tb, enum tb_cfg_pkg_type type,
 
 static int
 __icm_driver_ready(struct tb *tb, enum tb_security_level *security_level,
-		   size_t *nboot_acl)
+		   size_t *nboot_acl, bool *rpm)
 {
 	struct icm *icm = tb_priv(tb);
 	unsigned int retries = 50;
 	int ret;
 
-	ret = icm->driver_ready(tb, security_level, nboot_acl);
+	ret = icm->driver_ready(tb, security_level, nboot_acl, rpm);
 	if (ret) {
 		tb_err(tb, "failed to send driver ready to ICM\n");
 		return ret;
@@ -1656,7 +1725,8 @@ static int icm_driver_ready(struct tb *tb)
 		return 0;
 	}
 
-	ret = __icm_driver_ready(tb, &tb->security_level, &tb->nboot_acl);
+	ret = __icm_driver_ready(tb, &tb->security_level, &tb->nboot_acl,
+				 &icm->rpm);
 	if (ret)
 		return ret;
 
@@ -1762,7 +1832,7 @@ static void icm_complete(struct tb *tb)
 	 * Now all existing children should be resumed, start events
 	 * from ICM to get updated status.
 	 */
-	__icm_driver_ready(tb, NULL, NULL);
+	__icm_driver_ready(tb, NULL, NULL, NULL);
 
 	/*
 	 * We do not get notifications of devices that have been
@@ -1772,6 +1842,22 @@ static void icm_complete(struct tb *tb)
 	queue_delayed_work(tb->wq, &icm->rescan_work, msecs_to_jiffies(500));
 }
 
+static int icm_runtime_suspend(struct tb *tb)
+{
+	nhi_mailbox_cmd(tb->nhi, NHI_MAILBOX_DRV_UNLOADS, 0);
+	return 0;
+}
+
+static int icm_runtime_resume(struct tb *tb)
+{
+	/*
+	 * We can reuse the same resume functionality than with system
+	 * suspend.
+	 */
+	icm_complete(tb);
+	return 0;
+}
+
 static int icm_start(struct tb *tb)
 {
 	struct icm *icm = tb_priv(tb);
@@ -1790,6 +1876,7 @@ static int icm_start(struct tb *tb)
 	 * prevent root switch NVM upgrade on Macs for now.
 	 */
 	tb->root_switch->no_nvm_upgrade = x86_apple_machine;
+	tb->root_switch->rpm = icm->rpm;
 
 	ret = tb_switch_add(tb->root_switch);
 	if (ret) {
@@ -1838,6 +1925,8 @@ static const struct tb_cm_ops icm_ar_ops = {
 	.stop = icm_stop,
 	.suspend = icm_suspend,
 	.complete = icm_complete,
+	.runtime_suspend = icm_runtime_suspend,
+	.runtime_resume = icm_runtime_resume,
 	.handle_event = icm_handle_event,
 	.get_boot_acl = icm_ar_get_boot_acl,
 	.set_boot_acl = icm_ar_set_boot_acl,
@@ -1856,6 +1945,8 @@ static const struct tb_cm_ops icm_tr_ops = {
 	.stop = icm_stop,
 	.suspend = icm_suspend,
 	.complete = icm_complete,
+	.runtime_suspend = icm_runtime_suspend,
+	.runtime_resume = icm_runtime_resume,
 	.handle_event = icm_handle_event,
 	.get_boot_acl = icm_ar_get_boot_acl,
 	.set_boot_acl = icm_ar_set_boot_acl,
diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
index e3b7695fe37e..88cff05a1808 100644
--- a/drivers/thunderbolt/nhi.c
+++ b/drivers/thunderbolt/nhi.c
@@ -900,7 +900,32 @@ static void nhi_complete(struct device *dev)
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct tb *tb = pci_get_drvdata(pdev);
 
-	tb_domain_complete(tb);
+	/*
+	 * If we were runtime suspended when system suspend started,
+	 * schedule runtime resume now. It should bring the domain back
+	 * to functional state.
+	 */
+	if (pm_runtime_suspended(&pdev->dev))
+		pm_runtime_resume(&pdev->dev);
+	else
+		tb_domain_complete(tb);
+}
+
+static int nhi_runtime_suspend(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct tb *tb = pci_get_drvdata(pdev);
+
+	return tb_domain_runtime_suspend(tb);
+}
+
+static int nhi_runtime_resume(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct tb *tb = pci_get_drvdata(pdev);
+
+	nhi_enable_int_throttling(tb->nhi);
+	return tb_domain_runtime_resume(tb);
 }
 
 static void nhi_shutdown(struct tb_nhi *nhi)
@@ -1048,6 +1073,11 @@ static int nhi_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	}
 	pci_set_drvdata(pdev, tb);
 
+	pm_runtime_allow(&pdev->dev);
+	pm_runtime_set_autosuspend_delay(&pdev->dev, TB_AUTOSUSPEND_DELAY);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_put_autosuspend(&pdev->dev);
+
 	return 0;
 }
 
@@ -1056,6 +1086,10 @@ static void nhi_remove(struct pci_dev *pdev)
 	struct tb *tb = pci_get_drvdata(pdev);
 	struct tb_nhi *nhi = tb->nhi;
 
+	pm_runtime_get_sync(&pdev->dev);
+	pm_runtime_dont_use_autosuspend(&pdev->dev);
+	pm_runtime_forbid(&pdev->dev);
+
 	tb_domain_remove(tb);
 	nhi_shutdown(nhi);
 }
@@ -1078,6 +1112,8 @@ static const struct dev_pm_ops nhi_pm_ops = {
 	.freeze = nhi_suspend,
 	.poweroff = nhi_suspend,
 	.complete = nhi_complete,
+	.runtime_suspend = nhi_runtime_suspend,
+	.runtime_resume = nhi_runtime_resume,
 };
 
 static struct pci_device_id nhi_ids[] = {
diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index 25758671ddf4..7442bc4c6433 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -8,6 +8,7 @@
 #include <linux/delay.h>
 #include <linux/idr.h>
 #include <linux/nvmem-provider.h>
+#include <linux/pm_runtime.h>
 #include <linux/sizes.h>
 #include <linux/slab.h>
 #include <linux/vmalloc.h>
@@ -236,8 +237,14 @@ static int tb_switch_nvm_read(void *priv, unsigned int offset, void *val,
 			      size_t bytes)
 {
 	struct tb_switch *sw = priv;
+	int ret;
+
+	pm_runtime_get_sync(&sw->dev);
+	ret = dma_port_flash_read(sw->dma_port, offset, val, bytes);
+	pm_runtime_mark_last_busy(&sw->dev);
+	pm_runtime_put_autosuspend(&sw->dev);
 
-	return dma_port_flash_read(sw->dma_port, offset, val, bytes);
+	return ret;
 }
 
 static int tb_switch_nvm_write(void *priv, unsigned int offset, void *val,
@@ -722,6 +729,7 @@ static int tb_switch_set_authorized(struct tb_switch *sw, unsigned int val)
 	 * the new tunnel too early.
 	 */
 	pci_lock_rescan_remove();
+	pm_runtime_get_sync(&sw->dev);
 
 	switch (val) {
 	/* Approve switch */
@@ -742,6 +750,8 @@ static int tb_switch_set_authorized(struct tb_switch *sw, unsigned int val)
 		break;
 	}
 
+	pm_runtime_mark_last_busy(&sw->dev);
+	pm_runtime_put_autosuspend(&sw->dev);
 	pci_unlock_rescan_remove();
 
 	if (!ret) {
@@ -888,9 +898,18 @@ static ssize_t nvm_authenticate_store(struct device *dev,
 	nvm_clear_auth_status(sw);
 
 	if (val) {
+		if (!sw->nvm->buf) {
+			ret = -EINVAL;
+			goto exit_unlock;
+		}
+
+		pm_runtime_get_sync(&sw->dev);
 		ret = nvm_validate_and_write(sw);
-		if (ret)
+		if (ret) {
+			pm_runtime_mark_last_busy(&sw->dev);
+			pm_runtime_put_autosuspend(&sw->dev);
 			goto exit_unlock;
+		}
 
 		sw->nvm->authenticating = true;
 
@@ -898,6 +917,8 @@ static ssize_t nvm_authenticate_store(struct device *dev,
 			ret = nvm_authenticate_host(sw);
 		else
 			ret = nvm_authenticate_device(sw);
+		pm_runtime_mark_last_busy(&sw->dev);
+		pm_runtime_put_autosuspend(&sw->dev);
 	}
 
 exit_unlock:
@@ -1023,9 +1044,29 @@ static void tb_switch_release(struct device *dev)
 	kfree(sw);
 }
 
+/*
+ * Currently only need to provide the callbacks. Everything else is handled
+ * in the connection manager.
+ */
+static int __maybe_unused tb_switch_runtime_suspend(struct device *dev)
+{
+	return 0;
+}
+
+static int __maybe_unused tb_switch_runtime_resume(struct device *dev)
+{
+	return 0;
+}
+
+static const struct dev_pm_ops tb_switch_pm_ops = {
+	SET_RUNTIME_PM_OPS(tb_switch_runtime_suspend, tb_switch_runtime_resume,
+			   NULL)
+};
+
 struct device_type tb_switch_type = {
 	.name = "thunderbolt_device",
 	.release = tb_switch_release,
+	.pm = &tb_switch_pm_ops,
 };
 
 static int tb_switch_get_generation(struct tb_switch *sw)
@@ -1365,10 +1406,21 @@ int tb_switch_add(struct tb_switch *sw)
 		return ret;
 
 	ret = tb_switch_nvm_add(sw);
-	if (ret)
+	if (ret) {
 		device_del(&sw->dev);
+		return ret;
+	}
 
-	return ret;
+	pm_runtime_set_active(&sw->dev);
+	if (sw->rpm) {
+		pm_runtime_set_autosuspend_delay(&sw->dev, TB_AUTOSUSPEND_DELAY);
+		pm_runtime_use_autosuspend(&sw->dev);
+		pm_runtime_mark_last_busy(&sw->dev);
+		pm_runtime_enable(&sw->dev);
+		pm_request_autosuspend(&sw->dev);
+	}
+
+	return 0;
 }
 
 /**
@@ -1383,6 +1435,11 @@ void tb_switch_remove(struct tb_switch *sw)
 {
 	int i;
 
+	if (sw->rpm) {
+		pm_runtime_get_sync(&sw->dev);
+		pm_runtime_disable(&sw->dev);
+	}
+
 	/* port 0 is the switch itself and never has a remote */
 	for (i = 1; i <= sw->config.max_port_number; i++) {
 		if (tb_is_upstream_port(&sw->ports[i]))
diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index 9d9f0ca16bfb..5067d69d0501 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -67,6 +67,7 @@ struct tb_switch_nvm {
  * @no_nvm_upgrade: Prevent NVM upgrade of this switch
  * @safe_mode: The switch is in safe-mode
  * @boot: Whether the switch was already authorized on boot or not
+ * @rpm: The switch supports runtime PM
  * @authorized: Whether the switch is authorized by user or policy
  * @work: Work used to automatically authorize a switch
  * @security_level: Switch supported security level
@@ -101,6 +102,7 @@ struct tb_switch {
 	bool no_nvm_upgrade;
 	bool safe_mode;
 	bool boot;
+	bool rpm;
 	unsigned int authorized;
 	struct work_struct work;
 	enum tb_security_level security_level;
@@ -199,6 +201,8 @@ struct tb_path {
  * @resume_noirq: Connection manager specific resume_noirq
  * @suspend: Connection manager specific suspend
  * @complete: Connection manager specific complete
+ * @runtime_suspend: Connection manager specific runtime_suspend
+ * @runtime_resume: Connection manager specific runtime_resume
  * @handle_event: Handle thunderbolt event
  * @get_boot_acl: Get boot ACL list
  * @set_boot_acl: Set boot ACL list
@@ -217,6 +221,8 @@ struct tb_cm_ops {
 	int (*resume_noirq)(struct tb *tb);
 	int (*suspend)(struct tb *tb);
 	void (*complete)(struct tb *tb);
+	int (*runtime_suspend)(struct tb *tb);
+	int (*runtime_resume)(struct tb *tb);
 	void (*handle_event)(struct tb *tb, enum tb_cfg_pkg_type,
 			     const void *buf, size_t size);
 	int (*get_boot_acl)(struct tb *tb, uuid_t *uuids, size_t nuuids);
@@ -235,6 +241,8 @@ static inline void *tb_priv(struct tb *tb)
 	return (void *)tb->privdata;
 }
 
+#define TB_AUTOSUSPEND_DELAY		15000 /* ms */
+
 /* helper functions & macros */
 
 /**
@@ -364,6 +372,8 @@ int tb_domain_suspend_noirq(struct tb *tb);
 int tb_domain_resume_noirq(struct tb *tb);
 int tb_domain_suspend(struct tb *tb);
 void tb_domain_complete(struct tb *tb);
+int tb_domain_runtime_suspend(struct tb *tb);
+int tb_domain_runtime_resume(struct tb *tb);
 int tb_domain_approve_switch(struct tb *tb, struct tb_switch *sw);
 int tb_domain_approve_switch_key(struct tb *tb, struct tb_switch *sw);
 int tb_domain_challenge_switch_key(struct tb *tb, struct tb_switch *sw);
diff --git a/drivers/thunderbolt/tb_msgs.h b/drivers/thunderbolt/tb_msgs.h
index bc13f8d6b804..2487e162c885 100644
--- a/drivers/thunderbolt/tb_msgs.h
+++ b/drivers/thunderbolt/tb_msgs.h
@@ -286,6 +286,8 @@ struct icm_ar_pkg_driver_ready_response {
 	u16 info;
 };
 
+#define ICM_AR_FLAGS_RTD3		BIT(6)
+
 #define ICM_AR_INFO_SLEVEL_MASK		GENMASK(3, 0)
 #define ICM_AR_INFO_BOOT_ACL_SHIFT	7
 #define ICM_AR_INFO_BOOT_ACL_MASK	GENMASK(11, 7)
@@ -333,6 +335,8 @@ struct icm_tr_pkg_driver_ready_response {
 	u16 reserved2;
 };
 
+#define ICM_TR_FLAGS_RTD3		BIT(6)
+
 #define ICM_TR_INFO_SLEVEL_MASK		GENMASK(2, 0)
 #define ICM_TR_INFO_BOOT_ACL_SHIFT	7
 #define ICM_TR_INFO_BOOT_ACL_MASK	GENMASK(12, 7)
diff --git a/drivers/thunderbolt/xdomain.c b/drivers/thunderbolt/xdomain.c
index 8abb4e843085..db8bece63327 100644
--- a/drivers/thunderbolt/xdomain.c
+++ b/drivers/thunderbolt/xdomain.c
@@ -13,6 +13,7 @@
 #include <linux/device.h>
 #include <linux/kmod.h>
 #include <linux/module.h>
+#include <linux/pm_runtime.h>
 #include <linux/utsname.h>
 #include <linux/uuid.h>
 #include <linux/workqueue.h>
@@ -1129,6 +1130,14 @@ struct tb_xdomain *tb_xdomain_alloc(struct tb *tb, struct device *parent,
 	xd->dev.groups = xdomain_attr_groups;
 	dev_set_name(&xd->dev, "%u-%llx", tb->index, route);
 
+	/*
+	 * This keeps the DMA powered on as long as we have active
+	 * connection to another host.
+	 */
+	pm_runtime_set_active(&xd->dev);
+	pm_runtime_get_noresume(&xd->dev);
+	pm_runtime_enable(&xd->dev);
+
 	return xd;
 
 err_free_local_uuid:
@@ -1174,6 +1183,15 @@ void tb_xdomain_remove(struct tb_xdomain *xd)
 
 	device_for_each_child_reverse(&xd->dev, xd, unregister_service);
 
+	/*
+	 * Undo runtime PM here explicitly because it is possible that
+	 * the XDomain was never added to the bus and thus device_del()
+	 * is not called for it (device_del() would handle this otherwise).
+	 */
+	pm_runtime_disable(&xd->dev);
+	pm_runtime_put_noidle(&xd->dev);
+	pm_runtime_set_suspended(&xd->dev);
+
 	if (!device_is_registered(&xd->dev))
 		put_device(&xd->dev);
 	else
-- 
2.17.1

Re: [PATCH 0/5] thunderbolt: Add support for runtime PM

[Mika Westerberg]

On Mon, Jun 18, 2018 at 02:07:26PM +0300, Mika Westerberg wrote:
> Hi all,
> 
> In recent PCs such as Lenovo X1 Carbon 6th generation the Thunderbolt
> controller is in RTD3 mode (Runtime D3). This is different from the
> previous modes because now the controller is present most of the time (it
> still will be hot-removed/hot-added during NVM firmware upgrade). Because
> of that we need to dynamically power it down whenever possible to save some
> power.
> 
> This patch series adds Linux runtime PM support for the Thunderbolt host
> controller driver using ICM firmware but it should be generic enough for
> future additions to allow similar functionality for the older Apple
> hardware as well (even though those system do not support full RTD3, it
> still makes it possible for the host controller to go to low power state if
> cable is not connected).
> 
> With these patches the driver automatically runtime suspends the host
> controller after being idle for 15s. The connected Thunderbolt devices (if
> any) need to support RTD3 mode as well. Typically all 3rd generation
> devices (Alpine Ridge, Titan Ridge) support this.
> 
> However, while this provides some power savings, there is more work to do
> in order to allow powering down the PCIe root port leading to the
> Thunderbolt PCIe hierarchy. This work is still in progress.
> 
> Mika Westerberg (5):
>   thunderbolt: Use 64-bit DMA mask if supported by the platform
>   thunderbolt: Do not unnecessarily call ICM get route
>   thunderbolt: No need to take tb->lock in domain suspend/complete
>   thunderbolt: Use correct ICM commands in system suspend
>   thunderbolt: Add support for runtime PM

All applied to thunderbolt.git/next.

Re: [PATCH 5/5] thunderbolt: Add support for runtime PM

[Lukas Wunner]

On Mon, Jun 18, 2018 at 02:07:31PM +0300, Mika Westerberg wrote:
> --- a/drivers/thunderbolt/domain.c
> +++ b/drivers/thunderbolt/domain.c
> @@ -132,6 +133,8 @@ static ssize_t boot_acl_show(struct device *dev, struct device_attribute *attr,
>  	if (!uuids)
>  		return -ENOMEM;
>  
> +	pm_runtime_get_sync(&tb->dev);
> +
>  	if (mutex_lock_interruptible(&tb->lock)) {
>  		ret = -ERESTARTSYS;
>  		goto out;
[snip]
> @@ -426,6 +437,13 @@ int tb_domain_add(struct tb *tb)
>  	/* This starts event processing */
>  	mutex_unlock(&tb->lock);
>  
> +	pm_runtime_no_callbacks(&tb->dev);
> +	pm_runtime_set_active(&tb->dev);
> +	pm_runtime_enable(&tb->dev);
> +	pm_runtime_set_autosuspend_delay(&tb->dev, TB_AUTOSUSPEND_DELAY);
> +	pm_runtime_mark_last_busy(&tb->dev);
> +	pm_runtime_use_autosuspend(&tb->dev);
> +
>  	return 0;
>  
>  err_domain_del:

You're setting pm_runtime_no_callbacks() on the domain.  A side effect of
setting this flag is that whenever the domain's device is runtime resumed,
it's parent (the NHI) is *not* runtime resumed, see this comment in
rpm_resume():

	/*
	 * See if we can skip waking up the parent.  This is safe only if
	 * power.no_callbacks is set, because otherwise we don't know whether
	 * the resume will actually succeed.
	 */

Above, you're runtime resuming the domain in boot_acl_show().  So if the
NHI is runtime suspended while that sysfs attribute is accessed, it won't
be runtime resumed.  Is that actually what you want?


> @@ -514,6 +532,28 @@ void tb_domain_complete(struct tb *tb)
>  		tb->cm_ops->complete(tb);
>  }
>  
> +int tb_domain_runtime_suspend(struct tb *tb)
> +{
> +	if (tb->cm_ops->runtime_suspend) {
> +		int ret = tb->cm_ops->runtime_suspend(tb);
> +		if (ret)
> +			return ret;
> +	}
> +	tb_ctl_stop(tb->ctl);
> +	return 0;
> +}
> +
> +int tb_domain_runtime_resume(struct tb *tb)
> +{
> +	tb_ctl_start(tb->ctl);
> +	if (tb->cm_ops->runtime_resume) {
> +		int ret = tb->cm_ops->runtime_resume(tb);
> +		if (ret)
> +			return ret;
> +	}
> +	return 0;
> +}
> +
>  /**
>   * tb_domain_approve_switch() - Approve switch
>   * @tb: Domain the switch belongs to
> --- a/drivers/thunderbolt/nhi.c
> +++ b/drivers/thunderbolt/nhi.c
> @@ -900,7 +900,32 @@ static void nhi_complete(struct device *dev)
>  	struct pci_dev *pdev = to_pci_dev(dev);
>  	struct tb *tb = pci_get_drvdata(pdev);
>  
> -	tb_domain_complete(tb);
> +	/*
> +	 * If we were runtime suspended when system suspend started,
> +	 * schedule runtime resume now. It should bring the domain back
> +	 * to functional state.
> +	 */
> +	if (pm_runtime_suspended(&pdev->dev))
> +		pm_runtime_resume(&pdev->dev);
> +	else
> +		tb_domain_complete(tb);
> +}
> +
> +static int nhi_runtime_suspend(struct device *dev)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct tb *tb = pci_get_drvdata(pdev);
> +
> +	return tb_domain_runtime_suspend(tb);
> +}
> +
> +static int nhi_runtime_resume(struct device *dev)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct tb *tb = pci_get_drvdata(pdev);
> +
> +	nhi_enable_int_throttling(tb->nhi);
> +	return tb_domain_runtime_resume(tb);
>  }

You're invoking tb_domain_runtime_suspend() from nhi_runtime_suspend(),
same for ->runtime_resume.

Wouldn't it make more sense to make tb_domain_runtime_suspend() the
->runtime_suspend callback of the domain instead of mixing it together
with NHI runtime suspend?

BTW, what's the purpose of nhi_enable_int_throttling()?


> --- a/drivers/thunderbolt/switch.c
> +++ b/drivers/thunderbolt/switch.c
> +/*
> + * Currently only need to provide the callbacks. Everything else is handled
> + * in the connection manager.
> + */
> +static int __maybe_unused tb_switch_runtime_suspend(struct device *dev)
> +{
> +	return 0;
> +}
> +
> +static int __maybe_unused tb_switch_runtime_resume(struct device *dev)
> +{
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops tb_switch_pm_ops = {
> +	SET_RUNTIME_PM_OPS(tb_switch_runtime_suspend, tb_switch_runtime_resume,
> +			   NULL)
> +};
> +
>  struct device_type tb_switch_type = {
>  	.name = "thunderbolt_device",
>  	.release = tb_switch_release,
> +	.pm = &tb_switch_pm_ops,
>  };

Looking at the call sites of RPM_GET_CALLBACK(), I'm under the impression
that if no callbacks are defined, the PM core will simply assume success.
Then you don't need to define any PM callbacks for tb_switch.  Am I missing
something?

Thanks,

Lukas

Re: [PATCH 5/5] thunderbolt: Add support for runtime PM

[Mika Westerberg]

On Sat, Jul 07, 2018 at 03:38:15PM +0200, Lukas Wunner wrote:
> On Mon, Jun 18, 2018 at 02:07:31PM +0300, Mika Westerberg wrote:
> > --- a/drivers/thunderbolt/domain.c
> > +++ b/drivers/thunderbolt/domain.c
> > @@ -132,6 +133,8 @@ static ssize_t boot_acl_show(struct device *dev, struct device_attribute *attr,
> >  	if (!uuids)
> >  		return -ENOMEM;
> >  
> > +	pm_runtime_get_sync(&tb->dev);
> > +
> >  	if (mutex_lock_interruptible(&tb->lock)) {
> >  		ret = -ERESTARTSYS;
> >  		goto out;
> [snip]
> > @@ -426,6 +437,13 @@ int tb_domain_add(struct tb *tb)
> >  	/* This starts event processing */
> >  	mutex_unlock(&tb->lock);
> >  
> > +	pm_runtime_no_callbacks(&tb->dev);
> > +	pm_runtime_set_active(&tb->dev);
> > +	pm_runtime_enable(&tb->dev);
> > +	pm_runtime_set_autosuspend_delay(&tb->dev, TB_AUTOSUSPEND_DELAY);
> > +	pm_runtime_mark_last_busy(&tb->dev);
> > +	pm_runtime_use_autosuspend(&tb->dev);
> > +
> >  	return 0;
> >  
> >  err_domain_del:
> 
> You're setting pm_runtime_no_callbacks() on the domain.  A side effect of
> setting this flag is that whenever the domain's device is runtime resumed,
> it's parent (the NHI) is *not* runtime resumed, see this comment in
> rpm_resume():
> 
> 	/*
> 	 * See if we can skip waking up the parent.  This is safe only if
> 	 * power.no_callbacks is set, because otherwise we don't know whether
> 	 * the resume will actually succeed.
> 	 */
> 
> Above, you're runtime resuming the domain in boot_acl_show().  So if the
> NHI is runtime suspended while that sysfs attribute is accessed, it won't
> be runtime resumed.  Is that actually what you want?

No, it should be runtime resumed when domain is. Looking at the code in
question bit more deeper:

        /*
         * See if we can skip waking up the parent.  This is safe only if
         * power.no_callbacks is set, because otherwise we don't know whether
         * the resume will actually succeed.
         */
        if (dev->power.no_callbacks && !parent && dev->parent) {
                spin_lock_nested(&dev->parent->power.lock, SINGLE_DEPTH_NESTING);
                if (dev->parent->power.disable_depth > 0
                    || dev->parent->power.ignore_children
                    || dev->parent->power.runtime_status == RPM_ACTIVE) {
                        atomic_inc(&dev->parent->power.child_count);
                        spin_unlock(&dev->parent->power.lock);
                        retval = 1;
                        goto no_callback;       /* Assume success. */
                }
                spin_unlock(&dev->parent->power.lock);
        }

So skipping waking the parent can only happen if any of the following
conditions are true:

  - Parent has runtime PM disabled
  - Parent has ignore_children set
  - Parent is already resumed

As far I can tell there can't be situation you describe that the parent would
not be runtime resumed when the domain is.

> > @@ -514,6 +532,28 @@ void tb_domain_complete(struct tb *tb)
> >  		tb->cm_ops->complete(tb);
> >  }
> >  
> > +int tb_domain_runtime_suspend(struct tb *tb)
> > +{
> > +	if (tb->cm_ops->runtime_suspend) {
> > +		int ret = tb->cm_ops->runtime_suspend(tb);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +	tb_ctl_stop(tb->ctl);
> > +	return 0;
> > +}
> > +
> > +int tb_domain_runtime_resume(struct tb *tb)
> > +{
> > +	tb_ctl_start(tb->ctl);
> > +	if (tb->cm_ops->runtime_resume) {
> > +		int ret = tb->cm_ops->runtime_resume(tb);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +	return 0;
> > +}
> > +
> >  /**
> >   * tb_domain_approve_switch() - Approve switch
> >   * @tb: Domain the switch belongs to
> > --- a/drivers/thunderbolt/nhi.c
> > +++ b/drivers/thunderbolt/nhi.c
> > @@ -900,7 +900,32 @@ static void nhi_complete(struct device *dev)
> >  	struct pci_dev *pdev = to_pci_dev(dev);
> >  	struct tb *tb = pci_get_drvdata(pdev);
> >  
> > -	tb_domain_complete(tb);
> > +	/*
> > +	 * If we were runtime suspended when system suspend started,
> > +	 * schedule runtime resume now. It should bring the domain back
> > +	 * to functional state.
> > +	 */
> > +	if (pm_runtime_suspended(&pdev->dev))
> > +		pm_runtime_resume(&pdev->dev);
> > +	else
> > +		tb_domain_complete(tb);
> > +}
> > +
> > +static int nhi_runtime_suspend(struct device *dev)
> > +{
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> > +	struct tb *tb = pci_get_drvdata(pdev);
> > +
> > +	return tb_domain_runtime_suspend(tb);
> > +}
> > +
> > +static int nhi_runtime_resume(struct device *dev)
> > +{
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> > +	struct tb *tb = pci_get_drvdata(pdev);
> > +
> > +	nhi_enable_int_throttling(tb->nhi);
> > +	return tb_domain_runtime_resume(tb);
> >  }
> 
> You're invoking tb_domain_runtime_suspend() from nhi_runtime_suspend(),
> same for ->runtime_resume.
> 
> Wouldn't it make more sense to make tb_domain_runtime_suspend() the
> ->runtime_suspend callback of the domain instead of mixing it together
> with NHI runtime suspend?

You mean let the PM core to handle this for domain? Maybe but currently we do
the same for other callbacks as well so this just follows that.

> BTW, what's the purpose of nhi_enable_int_throttling()?

It changes how fast interrupts get delivered and when to start throttling.
Mostly needed in P2P functionality (but should not do any harm for control
channel traffic). See also 8c6bba10fb92 ("thunderbolt: Configure interrupt
throttling for all interrupts").

> > --- a/drivers/thunderbolt/switch.c
> > +++ b/drivers/thunderbolt/switch.c
> > +/*
> > + * Currently only need to provide the callbacks. Everything else is handled
> > + * in the connection manager.
> > + */
> > +static int __maybe_unused tb_switch_runtime_suspend(struct device *dev)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused tb_switch_runtime_resume(struct device *dev)
> > +{
> > +	return 0;
> > +}
> > +
> > +static const struct dev_pm_ops tb_switch_pm_ops = {
> > +	SET_RUNTIME_PM_OPS(tb_switch_runtime_suspend, tb_switch_runtime_resume,
> > +			   NULL)
> > +};
> > +
> >  struct device_type tb_switch_type = {
> >  	.name = "thunderbolt_device",
> >  	.release = tb_switch_release,
> > +	.pm = &tb_switch_pm_ops,
> >  };
> 
> Looking at the call sites of RPM_GET_CALLBACK(), I'm under the impression
> that if no callbacks are defined, the PM core will simply assume success.
> Then you don't need to define any PM callbacks for tb_switch.  Am I missing
> something?

If you don't define them, RPM_GET_CALLBACK() returns NULL and subsequent call
to rpm_callback(NULL, dev) then returns -ENOSYS which is failure.

Re: [PATCH 5/5] thunderbolt: Add support for runtime PM

[Lukas Wunner]

On Sat, Jul 07, 2018 at 05:25:53PM +0300, Mika Westerberg wrote:
> On Sat, Jul 07, 2018 at 03:38:15PM +0200, Lukas Wunner wrote:
> > You're setting pm_runtime_no_callbacks() on the domain.  A side effect of
> > setting this flag is that whenever the domain's device is runtime resumed,
> > it's parent (the NHI) is *not* runtime resumed, see this comment in
> > rpm_resume():
> > 
> > 	/*
> > 	 * See if we can skip waking up the parent.  This is safe only if
> > 	 * power.no_callbacks is set, because otherwise we don't know whether
> > 	 * the resume will actually succeed.
> > 	 */
> > 
> > Above, you're runtime resuming the domain in boot_acl_show().  So if the
> > NHI is runtime suspended while that sysfs attribute is accessed, it won't
> > be runtime resumed.  Is that actually what you want?
> 
> No, it should be runtime resumed when domain is. Looking at the code in
> question bit more deeper:
[snip]
> So skipping waking the parent can only happen if any of the following
> conditions are true:
> 
>   - Parent has runtime PM disabled
>   - Parent has ignore_children set
>   - Parent is already resumed
> 
> As far I can tell there can't be situation you describe that the parent would
> not be runtime resumed when the domain is.

Okay, missed that.

Then why aren't you using pm_runtime_no_callbacks() on switches as well?
Wouldn't that obviate the need to declare those empty runtime PM callbacks?


> > BTW, what's the purpose of nhi_enable_int_throttling()?
> 
> It changes how fast interrupts get delivered and when to start throttling.
> Mostly needed in P2P functionality (but should not do any harm for control
> channel traffic). See also 8c6bba10fb92 ("thunderbolt: Configure interrupt
> throttling for all interrupts").

Understood, thanks.

Lukas

Re: [PATCH 5/5] thunderbolt: Add support for runtime PM

[Mika Westerberg]

On Sat, Jul 07, 2018 at 04:43:48PM +0200, Lukas Wunner wrote:
> On Sat, Jul 07, 2018 at 05:25:53PM +0300, Mika Westerberg wrote:
> > On Sat, Jul 07, 2018 at 03:38:15PM +0200, Lukas Wunner wrote:
> > > You're setting pm_runtime_no_callbacks() on the domain.  A side effect of
> > > setting this flag is that whenever the domain's device is runtime resumed,
> > > it's parent (the NHI) is *not* runtime resumed, see this comment in
> > > rpm_resume():
> > > 
> > > 	/*
> > > 	 * See if we can skip waking up the parent.  This is safe only if
> > > 	 * power.no_callbacks is set, because otherwise we don't know whether
> > > 	 * the resume will actually succeed.
> > > 	 */
> > > 
> > > Above, you're runtime resuming the domain in boot_acl_show().  So if the
> > > NHI is runtime suspended while that sysfs attribute is accessed, it won't
> > > be runtime resumed.  Is that actually what you want?
> > 
> > No, it should be runtime resumed when domain is. Looking at the code in
> > question bit more deeper:
> [snip]
> > So skipping waking the parent can only happen if any of the following
> > conditions are true:
> > 
> >   - Parent has runtime PM disabled
> >   - Parent has ignore_children set
> >   - Parent is already resumed
> > 
> > As far I can tell there can't be situation you describe that the parent would
> > not be runtime resumed when the domain is.
> 
> Okay, missed that.
> 
> Then why aren't you using pm_runtime_no_callbacks() on switches as well?
> Wouldn't that obviate the need to declare those empty runtime PM callbacks?

Domain is a kind of object that does not have a real physical device. It
is just an abstraction of a Thunderbolt domain formed by the host
controller (NHI) and the connection manager.  Therefore we power manage
it with the parent device (NHI). This is pretty much the purpose of
pm_runtime_no_callbacks().

However, switches do have a real physical device that can be power
managed. Furthermore we may need to add switch specific power management
logic at some point.

Re: [PATCH 5/5] thunderbolt: Add support for runtime PM

[Lukas Wunner]

On Mon, Jun 18, 2018 at 02:07:31PM +0300, Mika Westerberg wrote:
> Implement this using standard Linux runtime PM APIs so that when all the
> children devices are runtime suspended, the Thunderbolt host controller
> PCI device is runtime suspended as well. The ICM firmware then starts
> powering down power domains towards RTD3 but it can prevent this if it
> detects that there is an active Display Port stream (this is not visible
> to the software, though).
> 
> The Thunderbolt host controller will be runtime resumed either when
> there is a remote wake event (device is connected or disconnected), or
> when there is access from userspace that requires hardware access.

IIUC, if there is no xdomain, after 15 s all switch devices as well as
the domain device and the NHI will have runtime suspended.  The control
channel is torn down as well, so you can no longer receive notifications
over it.  Then how is wakeup of the NHI signalled on hotplug/unplug?
Do you get a PME for the NHI device?  Because I'm fairly certain that
I do not get a PME for the Light Ridge in my MacBook Pro, but I'll test
this once more and modify negotiate_os_control() to grant PME control
to the OS.

Thanks,

Lukas

Re: [PATCH 5/5] thunderbolt: Add support for runtime PM

[Mika Westerberg]

On Sat, Jul 07, 2018 at 11:14:01PM +0200, Lukas Wunner wrote:
> On Mon, Jun 18, 2018 at 02:07:31PM +0300, Mika Westerberg wrote:
> > Implement this using standard Linux runtime PM APIs so that when all the
> > children devices are runtime suspended, the Thunderbolt host controller
> > PCI device is runtime suspended as well. The ICM firmware then starts
> > powering down power domains towards RTD3 but it can prevent this if it
> > detects that there is an active Display Port stream (this is not visible
> > to the software, though).
> > 
> > The Thunderbolt host controller will be runtime resumed either when
> > there is a remote wake event (device is connected or disconnected), or
> > when there is access from userspace that requires hardware access.
> 
> IIUC, if there is no xdomain, after 15 s all switch devices as well as
> the domain device and the NHI will have runtime suspended.  The control
> channel is torn down as well, so you can no longer receive notifications
> over it.  Then how is wakeup of the NHI signalled on hotplug/unplug?
> Do you get a PME for the NHI device?

Yes, it sends PME.

> Because I'm fairly certain that
> I do not get a PME for the Light Ridge in my MacBook Pro, but I'll test
> this once more and modify negotiate_os_control() to grant PME control
> to the OS.

I think in case of Apple hardware, they handle the in some different
means than PME (possibly part of chipset driver or ACPI method/event).

Re: [PATCH 5/5] thunderbolt: Add support for runtime PM

[Yehezkel Bernat]

On Sun, Jul 8, 2018 at 10:31 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> On Sat, Jul 07, 2018 at 11:14:01PM +0200, Lukas Wunner wrote:
> >
> > Because I'm fairly certain that
> > I do not get a PME for the Light Ridge in my MacBook Pro, but I'll test
> > this once more and modify negotiate_os_control() to grant PME control
> > to the OS.
>
> I think in case of Apple hardware, they handle the in some different
> means than PME (possibly part of chipset driver or ACPI method/event).

In addition to what already mentioned, many things have changed around power
management during Alpine Ridge development, some of them came later as FW
updates (and BIOS changes). Comparing Alpine ridge to Light Ridge here is
comparing oranges to, well, Apples.

RE: [PATCH 5/5] thunderbolt: Add support for runtime PM

[Mario.Limonciello]

> -----Original Message-----
> From: Yehezkel Bernat [mailto:yehezkelshb@gmail.com]
> Sent: Sunday, July 8, 2018 2:56 AM
> To: Mika Westerberg
> Cc: lukas@wunner.de; LKML; Andreas Noever; michael.jamet@intel.com;
> rjw@rjwysocki.net; christian@kellner.me; Limonciello, Mario
> Subject: Re: [PATCH 5/5] thunderbolt: Add support for runtime PM
> 
> On Sun, Jul 8, 2018 at 10:31 AM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > On Sat, Jul 07, 2018 at 11:14:01PM +0200, Lukas Wunner wrote:
> > >
> > > Because I'm fairly certain that
> > > I do not get a PME for the Light Ridge in my MacBook Pro, but I'll test
> > > this once more and modify negotiate_os_control() to grant PME control
> > > to the OS.
> >
> > I think in case of Apple hardware, they handle the in some different
> > means than PME (possibly part of chipset driver or ACPI method/event).
> 
> In addition to what already mentioned, many things have changed around power
> management during Alpine Ridge development, some of them came later as FW
> updates (and BIOS changes). Comparing Alpine ridge to Light Ridge here is
> comparing oranges to, well, Apples.

In practice I don't anticipate anyone in the industry outside of Apple wiring up TBT
with RTD3 unless it's at least Alpine Ridge or newer.

Re: [PATCH 5/5] thunderbolt: Add support for runtime PM

[Mika Westerberg]

On Mon, Jul 09, 2018 at 04:20:00AM +0000, Mario.Limonciello@dell.com wrote:
> In practice I don't anticipate anyone in the industry outside of Apple wiring up TBT
> with RTD3 unless it's at least Alpine Ridge or newer.

I don't think Apple does it even with Alpine Ridge. I have one Macbook
here with AR and it for sure does not do any kind of runtime PM when
there is a device connected.