linux-acpi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/8] thunderbolt: Intel Ice Lake support
@ 2019-07-05  9:57 Mika Westerberg
  2019-07-05  9:57 ` [PATCH 1/8] thunderbolt: Correct path indices for PCIe tunnel Mika Westerberg
                   ` (8 more replies)
  0 siblings, 9 replies; 30+ messages in thread
From: Mika Westerberg @ 2019-07-05  9:57 UTC (permalink / raw)
  To: linux-kernel
  Cc: Andreas Noever, Michael Jamet, Yehezkel Bernat,
	Rafael J . Wysocki, Len Brown, Lukas Wunner, Mario.Limonciello,
	Anthony Wong, Mika Westerberg, linux-acpi

Hi all,

With the exception of the first patch which is fix, this series enables
Thunderbolt on Intel Ice Lake. Biggest difference from the previous
controllers is that the Thunderbolt controller is now integrated as part of
the SoC. The firmware messages pretty much follow Titan Ridge but there are
some differences as well (such as the new RTD3 veto notification). Also Ice
Lake does not implement security levels so DMA protection is handled by IOMMU.

This is v5.4 material but I'm sending it out now because I will be on
vacation next 4 weeks mostly without internet access. When I get back I'll
gather all the comments and update the series accordingly.

Thanks!

Mika Westerberg (8):
  thunderbolt: Correct path indices for PCIe tunnel
  thunderbolt: Move NVM upgrade support flag to struct icm
  thunderbolt: Use 32-bit writes when writing ring producer/consumer
  thunderbolt: Do not fail adding switch if some port is not implemented
  thunderbolt: Hide switch attributes that are not set
  thunderbolt: Expose active parts of NVM even if upgrade is not supported
  thunderbolt: Add support for Intel Ice Lake
  ACPI / property: Add two new Thunderbolt property GUIDs to the list

 drivers/acpi/property.c        |   6 +
 drivers/thunderbolt/ctl.c      |  23 ++-
 drivers/thunderbolt/icm.c      | 169 +++++++++++++++++--
 drivers/thunderbolt/nhi.c      | 300 ++++++++++++++++++++++++++++++++-
 drivers/thunderbolt/nhi.h      |   2 +
 drivers/thunderbolt/nhi_regs.h |  25 +++
 drivers/thunderbolt/switch.c   |  52 ++++--
 drivers/thunderbolt/tb_msgs.h  |  16 +-
 drivers/thunderbolt/tunnel.c   |   4 +-
 include/linux/thunderbolt.h    |   2 +
 10 files changed, 553 insertions(+), 46 deletions(-)

-- 
2.20.1


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 1/8] thunderbolt: Correct path indices for PCIe tunnel
  2019-07-05  9:57 [PATCH 0/8] thunderbolt: Intel Ice Lake support Mika Westerberg
@ 2019-07-05  9:57 ` Mika Westerberg
  2019-07-05  9:57 ` [PATCH 2/8] thunderbolt: Move NVM upgrade support flag to struct icm Mika Westerberg
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 30+ messages in thread
From: Mika Westerberg @ 2019-07-05  9:57 UTC (permalink / raw)
  To: linux-kernel
  Cc: Andreas Noever, Michael Jamet, Yehezkel Bernat,
	Rafael J . Wysocki, Len Brown, Lukas Wunner, Mario.Limonciello,
	Anthony Wong, Mika Westerberg, linux-acpi

PCIe tunnel path indices got mixed up when we added support for tunnels
between switches that are not adjacent. This did not affect the
functionality as it is just an index but fix it now nevertheless to make
the code easier to understand.

Reported-by: Rajmohan Mani <rajmohan.mani@intel.com>
Fixes: 8c7acaaf020f ("thunderbolt: Extend tunnel creation to more than 2 adjacent switches")
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/thunderbolt/tunnel.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/thunderbolt/tunnel.c b/drivers/thunderbolt/tunnel.c
index 31d0234837e4..5a99234826e7 100644
--- a/drivers/thunderbolt/tunnel.c
+++ b/drivers/thunderbolt/tunnel.c
@@ -211,7 +211,7 @@ struct tb_tunnel *tb_tunnel_alloc_pci(struct tb *tb, struct tb_port *up,
 		return NULL;
 	}
 	tb_pci_init_path(path);
-	tunnel->paths[TB_PCI_PATH_UP] = path;
+	tunnel->paths[TB_PCI_PATH_DOWN] = path;
 
 	path = tb_path_alloc(tb, up, TB_PCI_HOPID, down, TB_PCI_HOPID, 0,
 			     "PCIe Up");
@@ -220,7 +220,7 @@ struct tb_tunnel *tb_tunnel_alloc_pci(struct tb *tb, struct tb_port *up,
 		return NULL;
 	}
 	tb_pci_init_path(path);
-	tunnel->paths[TB_PCI_PATH_DOWN] = path;
+	tunnel->paths[TB_PCI_PATH_UP] = path;
 
 	return tunnel;
 }
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 2/8] thunderbolt: Move NVM upgrade support flag to struct icm
  2019-07-05  9:57 [PATCH 0/8] thunderbolt: Intel Ice Lake support Mika Westerberg
  2019-07-05  9:57 ` [PATCH 1/8] thunderbolt: Correct path indices for PCIe tunnel Mika Westerberg
@ 2019-07-05  9:57 ` Mika Westerberg
  2019-07-05 10:52   ` Yehezkel Bernat
  2019-07-05  9:57 ` [PATCH 3/8] thunderbolt: Use 32-bit writes when writing ring producer/consumer Mika Westerberg
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 30+ messages in thread
From: Mika Westerberg @ 2019-07-05  9:57 UTC (permalink / raw)
  To: linux-kernel
  Cc: Andreas Noever, Michael Jamet, Yehezkel Bernat,
	Rafael J . Wysocki, Len Brown, Lukas Wunner, Mario.Limonciello,
	Anthony Wong, Mika Westerberg, linux-acpi

This is depends on the controller and on the platform/CPU we are
running. Move it to struct icm so we can set it per controller.

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

diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c
index fbdcef56a676..2a56d9478b34 100644
--- a/drivers/thunderbolt/icm.c
+++ b/drivers/thunderbolt/icm.c
@@ -55,6 +55,7 @@
  * @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)
+ * @can_upgrade_nvm: Can the NVM firmware be upgrade on this controller
  * @is_supported: Checks if we can support ICM on this controller
  * @cio_reset: Trigger CIO reset
  * @get_mode: Read and return the ICM firmware mode (optional)
@@ -74,6 +75,7 @@ struct icm {
 	int vnd_cap;
 	bool safe_mode;
 	bool rpm;
+	bool can_upgrade_nvm;
 	bool (*is_supported)(struct tb *tb);
 	int (*cio_reset)(struct tb *tb);
 	int (*get_mode)(struct tb *tb);
@@ -1913,12 +1915,7 @@ static int icm_start(struct tb *tb)
 	if (IS_ERR(tb->root_switch))
 		return PTR_ERR(tb->root_switch);
 
-	/*
-	 * NVM upgrade has not been tested on Apple systems and they
-	 * don't provide images publicly either. To be on the safe side
-	 * prevent root switch NVM upgrade on Macs for now.
-	 */
-	tb->root_switch->no_nvm_upgrade = x86_apple_machine;
+	tb->root_switch->no_nvm_upgrade = !icm->can_upgrade_nvm;
 	tb->root_switch->rpm = icm->rpm;
 
 	ret = tb_switch_add(tb->root_switch);
@@ -2021,6 +2018,7 @@ struct tb *icm_probe(struct tb_nhi *nhi)
 	switch (nhi->pdev->device) {
 	case PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_NHI:
 	case PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_NHI:
+		icm->can_upgrade_nvm = true;
 		icm->is_supported = icm_fr_is_supported;
 		icm->get_route = icm_fr_get_route;
 		icm->save_devices = icm_fr_save_devices;
@@ -2038,6 +2036,13 @@ struct tb *icm_probe(struct tb_nhi *nhi)
 	case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_NHI:
 	case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_2C_NHI:
 		icm->max_boot_acl = ICM_AR_PREBOOT_ACL_ENTRIES;
+		/*
+		 * NVM upgrade has not been tested on Apple systems and
+		 * they don't provide images publicly either. To be on
+		 * the safe side prevent root switch NVM upgrade on Macs
+		 * for now.
+		 */
+		icm->can_upgrade_nvm = !x86_apple_machine;
 		icm->is_supported = icm_ar_is_supported;
 		icm->cio_reset = icm_ar_cio_reset;
 		icm->get_mode = icm_ar_get_mode;
@@ -2054,6 +2059,7 @@ struct tb *icm_probe(struct tb_nhi *nhi)
 	case PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_NHI:
 	case PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_NHI:
 		icm->max_boot_acl = ICM_AR_PREBOOT_ACL_ENTRIES;
+		icm->can_upgrade_nvm = true;
 		icm->is_supported = icm_ar_is_supported;
 		icm->cio_reset = icm_tr_cio_reset;
 		icm->get_mode = icm_ar_get_mode;
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 3/8] thunderbolt: Use 32-bit writes when writing ring producer/consumer
  2019-07-05  9:57 [PATCH 0/8] thunderbolt: Intel Ice Lake support Mika Westerberg
  2019-07-05  9:57 ` [PATCH 1/8] thunderbolt: Correct path indices for PCIe tunnel Mika Westerberg
  2019-07-05  9:57 ` [PATCH 2/8] thunderbolt: Move NVM upgrade support flag to struct icm Mika Westerberg
@ 2019-07-05  9:57 ` Mika Westerberg
  2019-07-05 11:09   ` Yehezkel Bernat
  2019-07-05  9:57 ` [PATCH 4/8] thunderbolt: Do not fail adding switch if some port is not implemented Mika Westerberg
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 30+ messages in thread
From: Mika Westerberg @ 2019-07-05  9:57 UTC (permalink / raw)
  To: linux-kernel
  Cc: Andreas Noever, Michael Jamet, Yehezkel Bernat,
	Rafael J . Wysocki, Len Brown, Lukas Wunner, Mario.Limonciello,
	Anthony Wong, Mika Westerberg, linux-acpi

The register access should be using 32-bit reads/writes according to the
datasheet. With the previous generation hardware 16-bit writes have been
working but starting with ICL this is not the case anymore so fix
producer/consumer register update to use correct width register address.

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

diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
index 27fbe62c7ddd..09242653da67 100644
--- a/drivers/thunderbolt/nhi.c
+++ b/drivers/thunderbolt/nhi.c
@@ -143,9 +143,24 @@ static void __iomem *ring_options_base(struct tb_ring *ring)
 	return io;
 }
 
-static void ring_iowrite16desc(struct tb_ring *ring, u32 value, u32 offset)
+static void ring_iowrite_prod(struct tb_ring *ring, u16 prod)
 {
-	iowrite16(value, ring_desc_base(ring) + offset);
+	u32 val;
+
+	val = ioread32(ring_desc_base(ring) + 8);
+	val &= 0x0000ffff;
+	val |= prod << 16;
+	iowrite32(val, ring_desc_base(ring) + 8);
+}
+
+static void ring_iowrite_cons(struct tb_ring *ring, u16 cons)
+{
+	u32 val;
+
+	val = ioread32(ring_desc_base(ring) + 8);
+	val &= 0xffff0000;
+	val |= cons;
+	iowrite32(val, ring_desc_base(ring) + 8);
 }
 
 static void ring_iowrite32desc(struct tb_ring *ring, u32 value, u32 offset)
@@ -197,7 +212,10 @@ static void ring_write_descriptors(struct tb_ring *ring)
 			descriptor->sof = frame->sof;
 		}
 		ring->head = (ring->head + 1) % ring->size;
-		ring_iowrite16desc(ring, ring->head, ring->is_tx ? 10 : 8);
+		if (ring->is_tx)
+			ring_iowrite_prod(ring, ring->head);
+		else
+			ring_iowrite_cons(ring, ring->head);
 	}
 }
 
@@ -662,7 +680,7 @@ void tb_ring_stop(struct tb_ring *ring)
 
 	ring_iowrite32options(ring, 0, 0);
 	ring_iowrite64desc(ring, 0, 0);
-	ring_iowrite16desc(ring, 0, ring->is_tx ? 10 : 8);
+	ring_iowrite32desc(ring, 0, 8);
 	ring_iowrite32desc(ring, 0, 12);
 	ring->head = 0;
 	ring->tail = 0;
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 4/8] thunderbolt: Do not fail adding switch if some port is not implemented
  2019-07-05  9:57 [PATCH 0/8] thunderbolt: Intel Ice Lake support Mika Westerberg
                   ` (2 preceding siblings ...)
  2019-07-05  9:57 ` [PATCH 3/8] thunderbolt: Use 32-bit writes when writing ring producer/consumer Mika Westerberg
@ 2019-07-05  9:57 ` Mika Westerberg
  2019-07-05  9:57 ` [PATCH 5/8] thunderbolt: Hide switch attributes that are not set Mika Westerberg
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 30+ messages in thread
From: Mika Westerberg @ 2019-07-05  9:57 UTC (permalink / raw)
  To: linux-kernel
  Cc: Andreas Noever, Michael Jamet, Yehezkel Bernat,
	Rafael J . Wysocki, Len Brown, Lukas Wunner, Mario.Limonciello,
	Anthony Wong, Mika Westerberg, linux-acpi

There are two ways to mark a port as unimplemented. Typical way is to
return port type as TB_TYPE_INACTIVE when its config space is read.
Alternatively if the port is not physically present (such as ports 10
and 11 in ICL) reading from port config space returns
TB_CFG_ERROR_INVALID_CONFIG_SPACE instead. Currently the driver bails
out from adding the switch if it receives any error during port
inititialization which is wrong.

Handle this properly and just leave the port as TB_TYPE_INACTIVE before
continuing to the next port.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/thunderbolt/ctl.c    | 23 +++++++++++++++++++----
 drivers/thunderbolt/switch.c |  8 +++++++-
 2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/drivers/thunderbolt/ctl.c b/drivers/thunderbolt/ctl.c
index 2427d73be731..2ec1af8f7968 100644
--- a/drivers/thunderbolt/ctl.c
+++ b/drivers/thunderbolt/ctl.c
@@ -930,6 +930,23 @@ struct tb_cfg_result tb_cfg_write_raw(struct tb_ctl *ctl, const void *buffer,
 	return res;
 }
 
+static int tb_cfg_get_error(struct tb_ctl *ctl, enum tb_cfg_space space,
+			    const struct tb_cfg_result *res)
+{
+	/*
+	 * For unimplemented ports access to port config space may return
+	 * TB_CFG_ERROR_INVALID_CONFIG_SPACE (alternatively their type is
+	 * set to TB_TYPE_INACTIVE). In the former case return -ENODEV so
+	 * that the caller can mark the port as disabled.
+	 */
+	if (space == TB_CFG_PORT &&
+	    res->tb_error == TB_CFG_ERROR_INVALID_CONFIG_SPACE)
+		return -ENODEV;
+
+	tb_cfg_print_error(ctl, res);
+	return -EIO;
+}
+
 int tb_cfg_read(struct tb_ctl *ctl, void *buffer, u64 route, u32 port,
 		enum tb_cfg_space space, u32 offset, u32 length)
 {
@@ -942,8 +959,7 @@ int tb_cfg_read(struct tb_ctl *ctl, void *buffer, u64 route, u32 port,
 
 	case 1:
 		/* Thunderbolt error, tb_error holds the actual number */
-		tb_cfg_print_error(ctl, &res);
-		return -EIO;
+		return tb_cfg_get_error(ctl, space, &res);
 
 	case -ETIMEDOUT:
 		tb_ctl_warn(ctl, "timeout reading config space %u from %#x\n",
@@ -969,8 +985,7 @@ int tb_cfg_write(struct tb_ctl *ctl, const void *buffer, u64 route, u32 port,
 
 	case 1:
 		/* Thunderbolt error, tb_error holds the actual number */
-		tb_cfg_print_error(ctl, &res);
-		return -EIO;
+		return tb_cfg_get_error(ctl, space, &res);
 
 	case -ETIMEDOUT:
 		tb_ctl_warn(ctl, "timeout writing config space %u to %#x\n",
diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index 10b56c66fec3..eac62ff1b85c 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -611,8 +611,14 @@ static int tb_init_port(struct tb_port *port)
 	int cap;
 
 	res = tb_port_read(port, &port->config, TB_CFG_PORT, 0, 8);
-	if (res)
+	if (res) {
+		if (res == -ENODEV) {
+			tb_dbg(port->sw->tb, " Port %d: not implemented\n",
+			       port->port);
+			return 0;
+		}
 		return res;
+	}
 
 	/* Port 0 is the switch itself and has no PHY. */
 	if (port->config.type == TB_TYPE_PORT && port->port != 0) {
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 5/8] thunderbolt: Hide switch attributes that are not set
  2019-07-05  9:57 [PATCH 0/8] thunderbolt: Intel Ice Lake support Mika Westerberg
                   ` (3 preceding siblings ...)
  2019-07-05  9:57 ` [PATCH 4/8] thunderbolt: Do not fail adding switch if some port is not implemented Mika Westerberg
@ 2019-07-05  9:57 ` Mika Westerberg
  2019-07-05  9:57 ` [PATCH 6/8] thunderbolt: Expose active parts of NVM even if upgrade is not supported Mika Westerberg
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 30+ messages in thread
From: Mika Westerberg @ 2019-07-05  9:57 UTC (permalink / raw)
  To: linux-kernel
  Cc: Andreas Noever, Michael Jamet, Yehezkel Bernat,
	Rafael J . Wysocki, Len Brown, Lukas Wunner, Mario.Limonciello,
	Anthony Wong, Mika Westerberg, linux-acpi

Thunderbolt host routers may not always contain DROM that includes
device identification information. This is mostly needed for Ice Lake
systems but some Falcon Ridge controllers on PCs also do not have DROM.

In that case hide the identification attributes.

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

diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index eac62ff1b85c..e84067084dcd 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -1337,7 +1337,19 @@ static umode_t switch_attr_is_visible(struct kobject *kobj,
 	struct device *dev = container_of(kobj, struct device, kobj);
 	struct tb_switch *sw = tb_to_switch(dev);
 
-	if (attr == &dev_attr_key.attr) {
+	if (attr == &dev_attr_device.attr) {
+		if (!sw->device)
+			return 0;
+	} else if (attr == &dev_attr_device_name.attr) {
+		if (!sw->device_name)
+			return 0;
+	} else if (attr == &dev_attr_vendor.attr)  {
+		if (!sw->vendor)
+			return 0;
+	} else if (attr == &dev_attr_vendor_name.attr)  {
+		if (!sw->vendor_name)
+			return 0;
+	} else if (attr == &dev_attr_key.attr) {
 		if (tb_route(sw) &&
 		    sw->tb->security_level == TB_SECURITY_SECURE &&
 		    sw->security_level == TB_SECURITY_SECURE)
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 6/8] thunderbolt: Expose active parts of NVM even if upgrade is not supported
  2019-07-05  9:57 [PATCH 0/8] thunderbolt: Intel Ice Lake support Mika Westerberg
                   ` (4 preceding siblings ...)
  2019-07-05  9:57 ` [PATCH 5/8] thunderbolt: Hide switch attributes that are not set Mika Westerberg
@ 2019-07-05  9:57 ` Mika Westerberg
  2019-07-05  9:57 ` [PATCH 7/8] thunderbolt: Add support for Intel Ice Lake Mika Westerberg
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 30+ messages in thread
From: Mika Westerberg @ 2019-07-05  9:57 UTC (permalink / raw)
  To: linux-kernel
  Cc: Andreas Noever, Michael Jamet, Yehezkel Bernat,
	Rafael J . Wysocki, Len Brown, Lukas Wunner, Mario.Limonciello,
	Anthony Wong, Mika Westerberg, linux-acpi

Ice Lake Thunderbolt controller NVM firmware is part of the BIOS image
which means it is not writable through the DMA port anymore. However, we
can still read it so we can keep nvm_version and active parts of NVM.
This way users still can find out the active NVM version and other
potentially useful information directly from Linux.

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

diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index e84067084dcd..e318e9714a38 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -364,12 +364,14 @@ static int tb_switch_nvm_add(struct tb_switch *sw)
 		nvm->active = nvm_dev;
 	}
 
-	nvm_dev = register_nvmem(sw, nvm->id, NVM_MAX_SIZE, false);
-	if (IS_ERR(nvm_dev)) {
-		ret = PTR_ERR(nvm_dev);
-		goto err_nvm_active;
+	if (!sw->no_nvm_upgrade) {
+		nvm_dev = register_nvmem(sw, nvm->id, NVM_MAX_SIZE, false);
+		if (IS_ERR(nvm_dev)) {
+			ret = PTR_ERR(nvm_dev);
+			goto err_nvm_active;
+		}
+		nvm->non_active = nvm_dev;
 	}
-	nvm->non_active = nvm_dev;
 
 	sw->nvm = nvm;
 	return 0;
@@ -398,7 +400,8 @@ static void tb_switch_nvm_remove(struct tb_switch *sw)
 	if (!nvm->authenticating)
 		nvm_clear_auth_status(sw);
 
-	nvmem_unregister(nvm->non_active);
+	if (nvm->non_active)
+		nvmem_unregister(nvm->non_active);
 	if (nvm->active)
 		nvmem_unregister(nvm->active);
 	ida_simple_remove(&nvm_ida, nvm->id);
@@ -1355,8 +1358,11 @@ static umode_t switch_attr_is_visible(struct kobject *kobj,
 		    sw->security_level == TB_SECURITY_SECURE)
 			return attr->mode;
 		return 0;
-	} else if (attr == &dev_attr_nvm_authenticate.attr ||
-		   attr == &dev_attr_nvm_version.attr) {
+	} else if (attr == &dev_attr_nvm_authenticate.attr) {
+		if (sw->dma_port && !sw->no_nvm_upgrade)
+			return attr->mode;
+		return 0;
+	} else if (attr == &dev_attr_nvm_version.attr) {
 		if (sw->dma_port)
 			return attr->mode;
 		return 0;
@@ -1707,13 +1713,17 @@ static int tb_switch_add_dma_port(struct tb_switch *sw)
 		break;
 	}
 
-	if (sw->no_nvm_upgrade)
+	/* Root switch DMA port requires running firmware */
+	if (!tb_route(sw) && sw->config.enabled)
 		return 0;
 
 	sw->dma_port = dma_port_alloc(sw);
 	if (!sw->dma_port)
 		return 0;
 
+	if (sw->no_nvm_upgrade)
+		return 0;
+
 	/*
 	 * Check status of the previous flash authentication. If there
 	 * is one we need to power cycle the switch in any case to make
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 7/8] thunderbolt: Add support for Intel Ice Lake
  2019-07-05  9:57 [PATCH 0/8] thunderbolt: Intel Ice Lake support Mika Westerberg
                   ` (5 preceding siblings ...)
  2019-07-05  9:57 ` [PATCH 6/8] thunderbolt: Expose active parts of NVM even if upgrade is not supported Mika Westerberg
@ 2019-07-05  9:57 ` Mika Westerberg
  2019-07-05 14:44   ` Yehezkel Bernat
  2019-08-04 18:25   ` Lukas Wunner
  2019-07-05  9:58 ` [PATCH 8/8] ACPI / property: Add two new Thunderbolt property GUIDs to the list Mika Westerberg
  2019-07-05 10:33 ` [PATCH 0/8] thunderbolt: Intel Ice Lake support Mika Westerberg
  8 siblings, 2 replies; 30+ messages in thread
From: Mika Westerberg @ 2019-07-05  9:57 UTC (permalink / raw)
  To: linux-kernel
  Cc: Andreas Noever, Michael Jamet, Yehezkel Bernat,
	Rafael J . Wysocki, Len Brown, Lukas Wunner, Mario.Limonciello,
	Anthony Wong, Mika Westerberg, linux-acpi

The Thunderbolt controller is integrated into the Ice Lake CPU itself
and requires special flows to power it on and off using force power bit
in NHI VSEC registers. Runtime PM (RTD3) and Sx flows also differ from
the discrete solutions. Now the firmware notifies the driver whether
RTD3 entry or exit are possible. The driver is responsible of sending
Go2Sx command through link controller mailbox when system enters Sx
states (suspend-to-mem/disk). Rest of the ICM firwmare flows follow
Titan Ridge.

Signed-off-by: Raanan Avargil <raanan.avargil@intel.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/thunderbolt/icm.c      | 151 ++++++++++++++++--
 drivers/thunderbolt/nhi.c      | 274 ++++++++++++++++++++++++++++++++-
 drivers/thunderbolt/nhi.h      |   2 +
 drivers/thunderbolt/nhi_regs.h |  25 +++
 drivers/thunderbolt/switch.c   |   2 +
 drivers/thunderbolt/tb_msgs.h  |  16 +-
 include/linux/thunderbolt.h    |   2 +
 7 files changed, 453 insertions(+), 19 deletions(-)

diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c
index 2a56d9478b34..e882e79b32a4 100644
--- a/drivers/thunderbolt/icm.c
+++ b/drivers/thunderbolt/icm.c
@@ -56,16 +56,19 @@
  * @max_boot_acl: Maximum number of preboot ACL entries (%0 if not supported)
  * @rpm: Does the controller support runtime PM (RTD3)
  * @can_upgrade_nvm: Can the NVM firmware be upgrade on this controller
+ * @veto: Is RTD3 veto in effect
  * @is_supported: Checks if we can support ICM on this controller
  * @cio_reset: Trigger CIO reset
  * @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
+ * @set_uuid: Set UUID for the root switch (optional)
  * @device_connected: Handle device connected ICM message
  * @device_disconnected: Handle device disconnected ICM message
  * @xdomain_connected - Handle XDomain connected ICM message
  * @xdomain_disconnected - Handle XDomain disconnected ICM message
+ * @rtd3_veto: Handle RTD3 veto notification ICM message
  */
 struct icm {
 	struct mutex request_lock;
@@ -76,6 +79,7 @@ struct icm {
 	bool safe_mode;
 	bool rpm;
 	bool can_upgrade_nvm;
+	bool veto;
 	bool (*is_supported)(struct tb *tb);
 	int (*cio_reset)(struct tb *tb);
 	int (*get_mode)(struct tb *tb);
@@ -84,6 +88,7 @@ struct icm {
 	int (*driver_ready)(struct tb *tb,
 			    enum tb_security_level *security_level,
 			    size_t *nboot_acl, bool *rpm);
+	void (*set_uuid)(struct tb *tb);
 	void (*device_connected)(struct tb *tb,
 				 const struct icm_pkg_header *hdr);
 	void (*device_disconnected)(struct tb *tb,
@@ -92,6 +97,7 @@ struct icm {
 				  const struct icm_pkg_header *hdr);
 	void (*xdomain_disconnected)(struct tb *tb,
 				     const struct icm_pkg_header *hdr);
+	void (*rtd3_veto)(struct tb *tb, const struct icm_pkg_header *hdr);
 };
 
 struct icm_notification {
@@ -519,14 +525,16 @@ static int icm_fr_disconnect_xdomain_paths(struct tb *tb, struct tb_xdomain *xd)
 	return 0;
 }
 
-static void add_switch(struct tb_switch *parent_sw, u64 route,
-		       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)
+static struct tb_switch *add_switch(struct tb_switch *parent_sw, u64 route,
+				    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;
+	int ret;
 
 	pm_runtime_get_sync(&parent_sw->dev);
 
@@ -557,14 +565,18 @@ static void add_switch(struct tb_switch *parent_sw, u64 route,
 	tb_port_at(route, parent_sw)->remote = tb_upstream_port(sw);
 	tb_upstream_port(sw)->remote = tb_port_at(route, parent_sw);
 
-	if (tb_switch_add(sw)) {
+	ret = tb_switch_add(sw);
+	if (ret) {
 		tb_port_at(tb_route(sw), parent_sw)->remote = NULL;
 		tb_switch_put(sw);
+		sw = ERR_PTR(ret);
 	}
 
 out:
 	pm_runtime_mark_last_busy(&parent_sw->dev);
 	pm_runtime_put_autosuspend(&parent_sw->dev);
+
+	return sw;
 }
 
 static void update_switch(struct tb_switch *parent_sw, struct tb_switch *sw,
@@ -1086,7 +1098,8 @@ static int icm_tr_disconnect_xdomain_paths(struct tb *tb, struct tb_xdomain *xd)
 }
 
 static void
-icm_tr_device_connected(struct tb *tb, const struct icm_pkg_header *hdr)
+__icm_tr_device_connected(struct tb *tb, const struct icm_pkg_header *hdr,
+			  bool force_rtd3)
 {
 	const struct icm_tr_event_device_connected *pkg =
 		(const struct icm_tr_event_device_connected *)hdr;
@@ -1151,13 +1164,21 @@ icm_tr_device_connected(struct tb *tb, const struct icm_pkg_header *hdr)
 		return;
 	}
 
-	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);
+	sw = 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);
+	if (!IS_ERR(sw) && force_rtd3)
+		sw->rpm = true;
 
 	tb_switch_put(parent_sw);
 }
 
+static void
+icm_tr_device_connected(struct tb *tb, const struct icm_pkg_header *hdr)
+{
+	__icm_tr_device_connected(tb, hdr, false);
+}
+
 static void
 icm_tr_device_disconnected(struct tb *tb, const struct icm_pkg_header *hdr)
 {
@@ -1468,6 +1489,72 @@ static int icm_ar_set_boot_acl(struct tb *tb, const uuid_t *uuids,
 	return 0;
 }
 
+static int
+icm_icl_driver_ready(struct tb *tb, enum tb_security_level *security_level,
+		    size_t *nboot_acl, bool *rpm)
+{
+	struct icm_tr_pkg_driver_ready_response reply;
+	struct icm_pkg_driver_ready request = {
+		.hdr.code = ICM_DRIVER_READY,
+	};
+	int ret;
+
+	memset(&reply, 0, sizeof(reply));
+	ret = icm_request(tb, &request, sizeof(request), &reply, sizeof(reply),
+			  1, 20000);
+	if (ret)
+		return ret;
+
+	/* Ice Lake always supports RTD3 */
+	if (rpm)
+		*rpm = true;
+
+	return 0;
+}
+
+static void icm_icl_set_uuid(struct tb *tb)
+{
+	struct tb_nhi *nhi = tb->nhi;
+	u32 uuid[4];
+
+	pci_read_config_dword(nhi->pdev, VS_CAP_10, &uuid[0]);
+	pci_read_config_dword(nhi->pdev, VS_CAP_11, &uuid[1]);
+	uuid[2] = 0xffffffff;
+	uuid[3] = 0xffffffff;
+
+	tb->root_switch->uuid = kmemdup(uuid, sizeof(uuid), GFP_KERNEL);
+}
+
+static void
+icm_icl_device_connected(struct tb *tb, const struct icm_pkg_header *hdr)
+{
+	__icm_tr_device_connected(tb, hdr, true);
+}
+
+static void icm_icl_rtd3_veto(struct tb *tb, const struct icm_pkg_header *hdr)
+{
+	const struct icm_icl_event_rtd3_veto *pkg =
+		(const struct icm_icl_event_rtd3_veto *)hdr;
+	struct icm *icm = tb_priv(tb);
+
+	tb_dbg(tb, "ICM rtd3 veto=0x%08x\n", pkg->veto_reason);
+
+	if (pkg->veto_reason) {
+		if (!icm->veto) {
+			icm->veto = true;
+			/* Keep the domain powered while veto is in effect */
+			pm_runtime_get(&tb->dev);
+		}
+	} else {
+		if (icm->veto) {
+			icm->veto = false;
+			/* Allow the domain suspend now */
+			pm_runtime_mark_last_busy(&tb->dev);
+			pm_runtime_put_autosuspend(&tb->dev);
+		}
+	}
+}
+
 static void icm_handle_notification(struct work_struct *work)
 {
 	struct icm_notification *n = container_of(work, typeof(*n), work);
@@ -1495,6 +1582,9 @@ static void icm_handle_notification(struct work_struct *work)
 		case ICM_EVENT_XDOMAIN_DISCONNECTED:
 			icm->xdomain_disconnected(tb, n->pkg);
 			break;
+		case ICM_EVENT_RTD3_VETO:
+			icm->rtd3_veto(tb, n->pkg);
+			break;
 		}
 	}
 
@@ -1853,6 +1943,18 @@ static void icm_complete(struct tb *tb)
 	if (tb->nhi->going_away)
 		return;
 
+	/*
+	 * If RTD3 was vetoed before we entered system suspend allow it
+	 * again now before driver ready is sent. Firmware sends a new RTD3
+	 * veto if it is still the case after we have sent it driver ready
+	 * command.
+	 */
+	if (icm->veto) {
+		icm->veto = false;
+		pm_runtime_mark_last_busy(&tb->dev);
+		pm_runtime_put_autosuspend(&tb->dev);
+	}
+
 	icm_unplug_children(tb->root_switch);
 
 	/*
@@ -1918,6 +2020,9 @@ static int icm_start(struct tb *tb)
 	tb->root_switch->no_nvm_upgrade = !icm->can_upgrade_nvm;
 	tb->root_switch->rpm = icm->rpm;
 
+	if (icm->set_uuid)
+		icm->set_uuid(tb);
+
 	ret = tb_switch_add(tb->root_switch);
 	if (ret) {
 		tb_switch_put(tb->root_switch);
@@ -2002,6 +2107,19 @@ static const struct tb_cm_ops icm_tr_ops = {
 	.disconnect_xdomain_paths = icm_tr_disconnect_xdomain_paths,
 };
 
+/* Ice Lake */
+static const struct tb_cm_ops icm_icl_ops = {
+	.driver_ready = icm_driver_ready,
+	.start = icm_start,
+	.stop = icm_stop,
+	.complete = icm_complete,
+	.runtime_suspend = icm_runtime_suspend,
+	.runtime_resume = icm_runtime_resume,
+	.handle_event = icm_handle_event,
+	.approve_xdomain_paths = icm_tr_approve_xdomain_paths,
+	.disconnect_xdomain_paths = icm_tr_disconnect_xdomain_paths,
+};
+
 struct tb *icm_probe(struct tb_nhi *nhi)
 {
 	struct icm *icm;
@@ -2070,6 +2188,19 @@ struct tb *icm_probe(struct tb_nhi *nhi)
 		icm->xdomain_disconnected = icm_tr_xdomain_disconnected;
 		tb->cm_ops = &icm_tr_ops;
 		break;
+
+	case PCI_DEVICE_ID_INTEL_ICL_NHI0:
+	case PCI_DEVICE_ID_INTEL_ICL_NHI1:
+		icm->is_supported = icm_ar_is_supported;
+		icm->driver_ready = icm_icl_driver_ready;
+		icm->set_uuid = icm_icl_set_uuid;
+		icm->device_connected = icm_icl_device_connected;
+		icm->device_disconnected = icm_tr_device_disconnected;
+		icm->xdomain_connected = icm_tr_xdomain_connected;
+		icm->xdomain_disconnected = icm_tr_xdomain_disconnected;
+		icm->rtd3_veto = icm_icl_rtd3_veto;
+		tb->cm_ops = &icm_icl_ops;
+		break;
 	}
 
 	if (!icm->is_supported || !icm->is_supported(tb)) {
diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
index 09242653da67..1af873c8127c 100644
--- a/drivers/thunderbolt/nhi.c
+++ b/drivers/thunderbolt/nhi.c
@@ -16,6 +16,8 @@
 #include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/delay.h>
+#include <linux/property.h>
+#include <linux/suspend.h>
 
 #include "nhi.h"
 #include "nhi_regs.h"
@@ -38,6 +40,60 @@
 #define MSIX_MAX_VECS		16
 
 #define NHI_MAILBOX_TIMEOUT	500 /* ms */
+#define LC_MAILBOX_TIMEOUT	500 /* ms */
+
+enum lc_mailbox_cmd {
+	LC_GO2SX = 0x02,
+	LC_GO2SX_NO_WAKE = 0x03,
+	LC_PREPARE_FOR_RESET = 0x21,
+};
+
+/**
+ * struct tb_nhi_ops - NHI specific optional operations
+ * @force_power: Issue force power for the NHI
+ * @set_ltr: Program LTR requirement values for the NHI
+ * @lc_mailbox_cmd: Send mailbox command to the link controller
+ * @lc_mailbox_cmd_complete: Wait for the previous command to complete
+ */
+struct tb_nhi_ops {
+	int (*force_power)(struct tb_nhi *nhi, bool power);
+	void (*set_ltr)(struct tb_nhi *nhi);
+	void (*lc_mailbox_cmd)(struct tb_nhi *nhi, enum lc_mailbox_cmd cmd);
+	int (*lc_mailbox_cmd_complete)(struct tb_nhi *nhi, int timeout);
+};
+
+static inline int nhi_power_up(struct tb_nhi *nhi)
+{
+	if (nhi->ops && nhi->ops->force_power)
+		return nhi->ops->force_power(nhi, true);
+	return 0;
+}
+
+static inline int nhi_power_down(struct tb_nhi *nhi)
+{
+	if (nhi->ops && nhi->ops->force_power)
+		return nhi->ops->force_power(nhi, false);
+	return 0;
+}
+
+static inline void nhi_set_ltr(struct tb_nhi *nhi)
+{
+	if (nhi->ops && nhi->ops->set_ltr)
+		nhi->ops->set_ltr(nhi);
+}
+
+static inline void lc_mailbox_cmd(struct tb_nhi *nhi, enum lc_mailbox_cmd cmd)
+{
+	if (nhi->ops && nhi->ops->lc_mailbox_cmd)
+		nhi->ops->lc_mailbox_cmd(nhi, cmd);
+}
+
+static inline int lc_mailbox_cmd_complete(struct tb_nhi *nhi, int timeout)
+{
+	if (nhi->ops && nhi->ops->lc_mailbox_cmd_complete)
+		return nhi->ops->lc_mailbox_cmd_complete(nhi, timeout);
+	return 0;
+}
 
 static int ring_interrupt_index(struct tb_ring *ring)
 {
@@ -863,12 +919,85 @@ static irqreturn_t nhi_msi(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static int nhi_suspend_noirq(struct device *dev)
+static int nhi_device_connected(struct device *dev, void *data)
+{
+	return tb_is_switch(dev);
+}
+
+static int nhi_suspend_power_down(struct tb *tb)
+{
+	int ret;
+
+	/*
+	 * If there is no device connected we need to perform an additional
+	 * handshake through LC mailbox and force power down before
+	 * entering D3.
+	 */
+	ret = device_for_each_child(&tb->root_switch->dev, NULL,
+				    nhi_device_connected);
+	if (!ret) {
+		lc_mailbox_cmd(tb->nhi, LC_PREPARE_FOR_RESET);
+		ret = lc_mailbox_cmd_complete(tb->nhi,
+					      LC_MAILBOX_TIMEOUT);
+		if (ret)
+			return ret;
+
+		return nhi_power_down(tb->nhi);
+	}
+
+	return 0;
+}
+
+static int __nhi_suspend_noirq(struct device *dev, bool wakeup)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct tb *tb = pci_get_drvdata(pdev);
+	int ret;
+
+	ret = tb_domain_suspend_noirq(tb);
+	if (ret)
+		return ret;
+
+	if (pm_suspend_via_firmware()) {
+		enum lc_mailbox_cmd cmd = wakeup ? LC_GO2SX : LC_GO2SX_NO_WAKE;
+
+		lc_mailbox_cmd(tb->nhi, cmd);
+		ret = lc_mailbox_cmd_complete(tb->nhi, LC_MAILBOX_TIMEOUT);
+		if (ret)
+			return ret;
+	} else {
+		ret = nhi_suspend_power_down(tb);
+	}
+
+	return ret;
+}
+
+static int nhi_suspend_noirq(struct device *dev)
+{
+	return __nhi_suspend_noirq(dev, device_may_wakeup(dev));
+}
+
+static bool nhi_wake_supported(struct pci_dev *pdev)
+{
+	u8 val;
+
+	/*
+	 * If power rails are sustainable for wakeup from S4 this
+	 * property is set by the BIOS.
+	 */
+	if (device_property_read_u8(&pdev->dev, "WAKE_SUPPORTED", &val))
+		return !!val;
+
+	return true;
+}
+
+static int nhi_poweroff_noirq(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	bool wakeup;
 
-	return tb_domain_suspend_noirq(tb);
+	wakeup = device_may_wakeup(dev) && nhi_wake_supported(pdev);
+	return __nhi_suspend_noirq(dev, wakeup);
 }
 
 static void nhi_enable_int_throttling(struct tb_nhi *nhi)
@@ -891,16 +1020,23 @@ static int nhi_resume_noirq(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct tb *tb = pci_get_drvdata(pdev);
+	int ret;
+
+	ret = nhi_power_up(tb->nhi);
+	if (ret)
+		return ret;
 
 	/*
 	 * Check that the device is still there. It may be that the user
 	 * unplugged last device which causes the host controller to go
 	 * away on PCs.
 	 */
-	if (!pci_device_is_present(pdev))
+	if (!pci_device_is_present(pdev)) {
 		tb->nhi->going_away = true;
-	else
+	} else {
+		nhi_set_ltr(tb->nhi);
 		nhi_enable_int_throttling(tb->nhi);
+	}
 
 	return tb_domain_resume_noirq(tb);
 }
@@ -933,16 +1069,28 @@ static int nhi_runtime_suspend(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct tb *tb = pci_get_drvdata(pdev);
+	int ret;
 
-	return tb_domain_runtime_suspend(tb);
+	ret = tb_domain_runtime_suspend(tb);
+	if (ret)
+		return ret;
+
+	return nhi_suspend_power_down(tb);
 }
 
 static int nhi_runtime_resume(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct tb *tb = pci_get_drvdata(pdev);
+	int ret;
 
+	ret = nhi_power_up(tb->nhi);
+	if (ret)
+		return ret;
+
+	nhi_set_ltr(tb->nhi);
 	nhi_enable_int_throttling(tb->nhi);
+
 	return tb_domain_runtime_resume(tb);
 }
 
@@ -970,6 +1118,7 @@ static void nhi_shutdown(struct tb_nhi *nhi)
 		flush_work(&nhi->interrupt_work);
 	}
 	ida_destroy(&nhi->msix_ida);
+	nhi_power_down(nhi);
 }
 
 static int nhi_init_msi(struct tb_nhi *nhi)
@@ -1014,12 +1163,27 @@ static int nhi_init_msi(struct tb_nhi *nhi)
 	return 0;
 }
 
+static bool nhi_imr_valid(struct pci_dev *pdev)
+{
+	u8 val;
+
+	if (!device_property_read_u8(&pdev->dev, "IMR_VALID", &val))
+		return !!val;
+
+	return true;
+}
+
 static int nhi_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
 	struct tb_nhi *nhi;
 	struct tb *tb;
 	int res;
 
+	if (!nhi_imr_valid(pdev)) {
+		dev_warn(&pdev->dev, "firmware image not valid, aborting\n");
+		return -ENODEV;
+	}
+
 	res = pcim_enable_device(pdev);
 	if (res) {
 		dev_err(&pdev->dev, "cannot enable PCI device, aborting\n");
@@ -1037,6 +1201,7 @@ static int nhi_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		return -ENOMEM;
 
 	nhi->pdev = pdev;
+	nhi->ops = (const struct tb_nhi_ops *)id->driver_data;
 	/* cannot fail - table is allocated bin pcim_iomap_regions */
 	nhi->iobase = pcim_iomap_table(pdev)[0];
 	nhi->hop_count = ioread32(nhi->iobase + REG_HOP_COUNT) & 0x3ff;
@@ -1080,6 +1245,14 @@ static int nhi_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	dev_dbg(&nhi->pdev->dev, "NHI initialized, starting thunderbolt\n");
 
+	res = nhi_power_up(nhi);
+	if (res) {
+		tb_domain_put(tb);
+		return res;
+	}
+
+	nhi_set_ltr(nhi);
+
 	res = tb_domain_add(tb);
 	if (res) {
 		/*
@@ -1111,6 +1284,7 @@ static void nhi_remove(struct pci_dev *pdev)
 
 	tb_domain_remove(tb);
 	nhi_shutdown(nhi);
+	nhi_power_down(nhi);
 }
 
 /*
@@ -1129,12 +1303,98 @@ static const struct dev_pm_ops nhi_pm_ops = {
 	.restore_noirq = nhi_resume_noirq,
 	.suspend = nhi_suspend,
 	.freeze = nhi_suspend,
+	.poweroff_noirq = nhi_poweroff_noirq,
 	.poweroff = nhi_suspend,
 	.complete = nhi_complete,
 	.runtime_suspend = nhi_runtime_suspend,
 	.runtime_resume = nhi_runtime_resume,
 };
 
+/* Ice Lake specific NHI operations */
+
+static int icl_nhi_force_power(struct tb_nhi *nhi, bool power)
+{
+	u32 vs_cap;
+
+	pci_read_config_dword(nhi->pdev, VS_CAP_22, &vs_cap);
+	if (power) {
+		vs_cap &= ~VS_CAP_22_DMA_DELAY_MASK;
+		vs_cap |= 0x22 << VS_CAP_22_DMA_DELAY_SHIFT;
+		vs_cap |= VS_CAP_22_FORCE_POWER;
+	} else {
+		vs_cap &= ~VS_CAP_22_FORCE_POWER;
+	}
+	pci_write_config_dword(nhi->pdev, VS_CAP_22, vs_cap);
+
+	if (power) {
+		unsigned int retries = 10;
+		u32 val;
+
+		/* Wait until the firmware tells it is up and running */
+		do {
+			pci_read_config_dword(nhi->pdev, VS_CAP_9, &val);
+			if (val & VS_CAP_9_FW_READY)
+				return 0;
+			msleep(250);
+		} while (--retries);
+
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+
+static void icl_nhi_set_ltr(struct tb_nhi *nhi)
+{
+	u32 max_ltr, ltr;
+
+	pci_read_config_dword(nhi->pdev, VS_CAP_16, &max_ltr);
+	max_ltr &= 0xffff;
+	/* Program the same value for both snoop and no-snoop */
+	ltr = max_ltr << 16 | max_ltr;
+	pci_write_config_dword(nhi->pdev, VS_CAP_15, ltr);
+}
+
+static void icl_nhi_lc_mailbox_cmd(struct tb_nhi *nhi, enum lc_mailbox_cmd cmd)
+{
+	u32 data;
+
+	pci_read_config_dword(nhi->pdev, VS_CAP_19, &data);
+	data = (cmd << VS_CAP_19_CMD_SHIFT) & VS_CAP_19_CMD_MASK;
+	pci_write_config_dword(nhi->pdev, VS_CAP_19, data | VS_CAP_19_VALID);
+}
+
+static int icl_nhi_lc_mailbox_cmd_complete(struct tb_nhi *nhi, int timeout)
+{
+	unsigned long end;
+	u32 data;
+
+	if (!timeout)
+		goto clear;
+
+	end = jiffies + msecs_to_jiffies(timeout);
+	do {
+		pci_read_config_dword(nhi->pdev, VS_CAP_18, &data);
+		if (data & VS_CAP_18_DONE)
+			goto clear;
+		msleep(100);
+	} while (time_before(jiffies, end));
+
+	return -ETIMEDOUT;
+
+clear:
+	/* Clear the valid bit */
+	pci_write_config_dword(nhi->pdev, VS_CAP_19, 0);
+	return 0;
+}
+
+static const struct tb_nhi_ops icl_nhi_ops = {
+	.force_power = icl_nhi_force_power,
+	.set_ltr = icl_nhi_set_ltr,
+	.lc_mailbox_cmd = icl_nhi_lc_mailbox_cmd,
+	.lc_mailbox_cmd_complete = icl_nhi_lc_mailbox_cmd_complete,
+};
+
 static struct pci_device_id nhi_ids[] = {
 	/*
 	 * We have to specify class, the TB bridges use the same device and
@@ -1176,6 +1436,10 @@ static struct pci_device_id nhi_ids[] = {
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_USBONLY_NHI) },
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_NHI) },
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_NHI) },
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ICL_NHI0),
+	  .driver_data = (kernel_ulong_t)&icl_nhi_ops },
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ICL_NHI1),
+	  .driver_data = (kernel_ulong_t)&icl_nhi_ops },
 
 	{ 0,}
 };
diff --git a/drivers/thunderbolt/nhi.h b/drivers/thunderbolt/nhi.h
index 1b5d47ecd3ed..dad691d1b704 100644
--- a/drivers/thunderbolt/nhi.h
+++ b/drivers/thunderbolt/nhi.h
@@ -51,5 +51,7 @@ enum nhi_fw_mode nhi_mailbox_mode(struct tb_nhi *nhi);
 #define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_BRIDGE	0x15ea
 #define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_NHI		0x15eb
 #define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_DD_BRIDGE	0x15ef
+#define PCI_DEVICE_ID_INTEL_ICL_NHI1			0x8a0d
+#define PCI_DEVICE_ID_INTEL_ICL_NHI0			0x8a17
 
 #endif
diff --git a/drivers/thunderbolt/nhi_regs.h b/drivers/thunderbolt/nhi_regs.h
index a60bd98c1d04..8754e7dd63d5 100644
--- a/drivers/thunderbolt/nhi_regs.h
+++ b/drivers/thunderbolt/nhi_regs.h
@@ -124,4 +124,29 @@ struct ring_desc {
 #define REG_FW_STS_ICM_EN_INVERT	BIT(1)
 #define REG_FW_STS_ICM_EN		BIT(0)
 
+/* ICL NHI VSEC registers */
+
+/* FW ready */
+#define VS_CAP_9			0xc8
+#define VS_CAP_9_FW_READY		BIT(31)
+/* UUID */
+#define VS_CAP_10			0xcc
+#define VS_CAP_11			0xd0
+/* LTR */
+#define VS_CAP_15			0xe0
+#define VS_CAP_16			0xe4
+/* TBT2PCIe */
+#define VS_CAP_18			0xec
+#define VS_CAP_18_DONE			BIT(0)
+/* PCIe2TBT */
+#define VS_CAP_19			0xf0
+#define VS_CAP_19_VALID			BIT(0)
+#define VS_CAP_19_CMD_SHIFT		1
+#define VS_CAP_19_CMD_MASK		GENMASK(7, 1)
+/* Force power */
+#define VS_CAP_22			0xfc
+#define VS_CAP_22_FORCE_POWER		BIT(1)
+#define VS_CAP_22_DMA_DELAY_MASK	GENMASK(31, 24)
+#define VS_CAP_22_DMA_DELAY_SHIFT	24
+
 #endif
diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index e318e9714a38..b896aa78fa9f 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -1470,6 +1470,8 @@ static int tb_switch_get_generation(struct tb_switch *sw)
 	case PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_BRIDGE:
 	case PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_BRIDGE:
 	case PCI_DEVICE_ID_INTEL_TITAN_RIDGE_DD_BRIDGE:
+	case PCI_DEVICE_ID_INTEL_ICL_NHI0:
+	case PCI_DEVICE_ID_INTEL_ICL_NHI1:
 		return 3;
 
 	default:
diff --git a/drivers/thunderbolt/tb_msgs.h b/drivers/thunderbolt/tb_msgs.h
index afbe1d29bb03..4b641e4ee0c5 100644
--- a/drivers/thunderbolt/tb_msgs.h
+++ b/drivers/thunderbolt/tb_msgs.h
@@ -104,10 +104,11 @@ enum icm_pkg_code {
 };
 
 enum icm_event_code {
-	ICM_EVENT_DEVICE_CONNECTED = 3,
-	ICM_EVENT_DEVICE_DISCONNECTED = 4,
-	ICM_EVENT_XDOMAIN_CONNECTED = 6,
-	ICM_EVENT_XDOMAIN_DISCONNECTED = 7,
+	ICM_EVENT_DEVICE_CONNECTED = 0x3,
+	ICM_EVENT_DEVICE_DISCONNECTED = 0x4,
+	ICM_EVENT_XDOMAIN_CONNECTED = 0x6,
+	ICM_EVENT_XDOMAIN_DISCONNECTED = 0x7,
+	ICM_EVENT_RTD3_VETO = 0xa,
 };
 
 struct icm_pkg_header {
@@ -463,6 +464,13 @@ struct icm_tr_pkg_disconnect_xdomain_response {
 	uuid_t remote_uuid;
 };
 
+/* Ice Lake messages */
+
+struct icm_icl_event_rtd3_veto {
+	struct icm_pkg_header hdr;
+	u32 veto_reason;
+};
+
 /* XDomain messages */
 
 struct tb_xdomain_header {
diff --git a/include/linux/thunderbolt.h b/include/linux/thunderbolt.h
index 2d7e012db03f..ece782ef5466 100644
--- a/include/linux/thunderbolt.h
+++ b/include/linux/thunderbolt.h
@@ -429,6 +429,7 @@ static inline struct tb_xdomain *tb_service_parent(struct tb_service *svc)
  * @lock: Must be held during ring creation/destruction. Is acquired by
  *	  interrupt_work when dispatching interrupts to individual rings.
  * @pdev: Pointer to the PCI device
+ * @ops: NHI specific optional ops
  * @iobase: MMIO space of the NHI
  * @tx_rings: All Tx rings available on this host controller
  * @rx_rings: All Rx rings available on this host controller
@@ -442,6 +443,7 @@ static inline struct tb_xdomain *tb_service_parent(struct tb_service *svc)
 struct tb_nhi {
 	spinlock_t lock;
 	struct pci_dev *pdev;
+	const struct tb_nhi_ops *ops;
 	void __iomem *iobase;
 	struct tb_ring **tx_rings;
 	struct tb_ring **rx_rings;
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 8/8] ACPI / property: Add two new Thunderbolt property GUIDs to the list
  2019-07-05  9:57 [PATCH 0/8] thunderbolt: Intel Ice Lake support Mika Westerberg
                   ` (6 preceding siblings ...)
  2019-07-05  9:57 ` [PATCH 7/8] thunderbolt: Add support for Intel Ice Lake Mika Westerberg
@ 2019-07-05  9:58 ` Mika Westerberg
  2019-07-09  8:18   ` Rafael J. Wysocki
  2019-07-05 10:33 ` [PATCH 0/8] thunderbolt: Intel Ice Lake support Mika Westerberg
  8 siblings, 1 reply; 30+ messages in thread
From: Mika Westerberg @ 2019-07-05  9:58 UTC (permalink / raw)
  To: linux-kernel
  Cc: Andreas Noever, Michael Jamet, Yehezkel Bernat,
	Rafael J . Wysocki, Len Brown, Lukas Wunner, Mario.Limonciello,
	Anthony Wong, Mika Westerberg, linux-acpi

Ice Lake Thunderbolt controller includes two new device property
compatible properties that we need to be able to extract in the driver
so add them to the growing array of GUIDs.

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

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index da3ced297f19..07cbacbab861 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -39,6 +39,12 @@ static const guid_t prp_guids[] = {
 	/* External facing port GUID: efcc06cc-73ac-4bc3-bff0-76143807c389 */
 	GUID_INIT(0xefcc06cc, 0x73ac, 0x4bc3,
 		  0xbf, 0xf0, 0x76, 0x14, 0x38, 0x07, 0xc3, 0x89),
+	/* TBT GUID for IMR_VALID: c44d002f-69f9-4e7d-a904-a7baabdf43f7 */
+	GUID_INIT(0xc44d002f, 0x69f9, 0x4e7d,
+		  0xa9, 0x04, 0xa7, 0xba, 0xab, 0xdf, 0x43, 0xf7),
+	/* TBT GUID for WAKE_SUPPORTED: 6c501103-c189-4296-ba72-9bf5a26ebe5d */
+	GUID_INIT(0x6c501103, 0xc189, 0x4296,
+		  0xba, 0x72, 0x9b, 0xf5, 0xa2, 0x6e, 0xbe, 0x5d),
 };
 
 /* ACPI _DSD data subnodes GUID: dbb8e3e6-5886-4ba6-8795-1319f52a966b */
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [PATCH 0/8] thunderbolt: Intel Ice Lake support
  2019-07-05  9:57 [PATCH 0/8] thunderbolt: Intel Ice Lake support Mika Westerberg
                   ` (7 preceding siblings ...)
  2019-07-05  9:58 ` [PATCH 8/8] ACPI / property: Add two new Thunderbolt property GUIDs to the list Mika Westerberg
@ 2019-07-05 10:33 ` Mika Westerberg
  2019-07-05 14:56   ` Yehezkel Bernat
  8 siblings, 1 reply; 30+ messages in thread
From: Mika Westerberg @ 2019-07-05 10:33 UTC (permalink / raw)
  To: linux-kernel
  Cc: Andreas Noever, Michael Jamet, Yehezkel Bernat,
	Rafael J . Wysocki, Len Brown, Lukas Wunner, Mario.Limonciello,
	Anthony Wong, linux-acpi, Rajmohan Mani, Raanan Avargil

On Fri, Jul 05, 2019 at 12:57:52PM +0300, Mika Westerberg wrote:
> Hi all,
> 
> With the exception of the first patch which is fix, this series enables
> Thunderbolt on Intel Ice Lake. Biggest difference from the previous
> controllers is that the Thunderbolt controller is now integrated as part of
> the SoC. The firmware messages pretty much follow Titan Ridge but there are
> some differences as well (such as the new RTD3 veto notification). Also Ice
> Lake does not implement security levels so DMA protection is handled by IOMMU.
> 
> This is v5.4 material but I'm sending it out now because I will be on
> vacation next 4 weeks mostly without internet access. When I get back I'll
> gather all the comments and update the series accordingly.
> 
> Thanks!
> 
> Mika Westerberg (8):
>   thunderbolt: Correct path indices for PCIe tunnel
>   thunderbolt: Move NVM upgrade support flag to struct icm
>   thunderbolt: Use 32-bit writes when writing ring producer/consumer
>   thunderbolt: Do not fail adding switch if some port is not implemented
>   thunderbolt: Hide switch attributes that are not set
>   thunderbolt: Expose active parts of NVM even if upgrade is not supported
>   thunderbolt: Add support for Intel Ice Lake
>   ACPI / property: Add two new Thunderbolt property GUIDs to the list

Forgot to Cc Raanan and Raj, now added. Sorry about that. The patch
series can also be viewed here:

  https://lore.kernel.org/lkml/20190705095800.43534-1-mika.westerberg@linux.intel.com/T/#m9cb5a393dfc79f1c2212d0787b6bad5b689db6bd

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/8] thunderbolt: Move NVM upgrade support flag to struct icm
  2019-07-05  9:57 ` [PATCH 2/8] thunderbolt: Move NVM upgrade support flag to struct icm Mika Westerberg
@ 2019-07-05 10:52   ` Yehezkel Bernat
  2019-07-05 10:58     ` Mika Westerberg
  0 siblings, 1 reply; 30+ messages in thread
From: Yehezkel Bernat @ 2019-07-05 10:52 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: LKML, Andreas Noever, Michael Jamet, Rafael J . Wysocki,
	Len Brown, Lukas Wunner, Mario Limonciello, Anthony Wong,
	linux-acpi

On Fri, Jul 5, 2019 at 12:58 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> @@ -1913,12 +1915,7 @@ static int icm_start(struct tb *tb)
>         if (IS_ERR(tb->root_switch))
>                 return PTR_ERR(tb->root_switch);
>
> -       /*
> -        * NVM upgrade has not been tested on Apple systems and they
> -        * don't provide images publicly either. To be on the safe side
> -        * prevent root switch NVM upgrade on Macs for now.
> -        */
> -       tb->root_switch->no_nvm_upgrade = x86_apple_machine;
> +       tb->root_switch->no_nvm_upgrade = !icm->can_upgrade_nvm;
>         tb->root_switch->rpm = icm->rpm;
>
>         ret = tb_switch_add(tb->root_switch);
> @@ -2021,6 +2018,7 @@ struct tb *icm_probe(struct tb_nhi *nhi)
>         switch (nhi->pdev->device) {
>         case PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_NHI:
>         case PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_NHI:
> +               icm->can_upgrade_nvm = true;
>                 icm->is_supported = icm_fr_is_supported;
>                 icm->get_route = icm_fr_get_route;
>                 icm->save_devices = icm_fr_save_devices;
> @@ -2038,6 +2036,13 @@ struct tb *icm_probe(struct tb_nhi *nhi)
>         case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_NHI:
>         case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_2C_NHI:
>                 icm->max_boot_acl = ICM_AR_PREBOOT_ACL_ENTRIES;
> +               /*
> +                * NVM upgrade has not been tested on Apple systems and
> +                * they don't provide images publicly either. To be on
> +                * the safe side prevent root switch NVM upgrade on Macs
> +                * for now.
> +                */
> +               icm->can_upgrade_nvm = !x86_apple_machine;
>                 icm->is_supported = icm_ar_is_supported;
>                 icm->cio_reset = icm_ar_cio_reset;
>                 icm->get_mode = icm_ar_get_mode;
> @@ -2054,6 +2059,7 @@ struct tb *icm_probe(struct tb_nhi *nhi)
>         case PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_NHI:
>         case PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_NHI:
>                 icm->max_boot_acl = ICM_AR_PREBOOT_ACL_ENTRIES;
> +               icm->can_upgrade_nvm = true;

Shouldn't this be also !x86_apple_machine just like AR?
(For FR, we don't use ICM on Apple machines, as much as I remember, so it's fine
to enable it there unconditionally for ICM code path.)

>                 icm->is_supported = icm_ar_is_supported;
>                 icm->cio_reset = icm_tr_cio_reset;
>                 icm->get_mode = icm_ar_get_mode;
> --
> 2.20.1
>

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/8] thunderbolt: Move NVM upgrade support flag to struct icm
  2019-07-05 10:52   ` Yehezkel Bernat
@ 2019-07-05 10:58     ` Mika Westerberg
  2019-07-09 15:11       ` Mario.Limonciello
  0 siblings, 1 reply; 30+ messages in thread
From: Mika Westerberg @ 2019-07-05 10:58 UTC (permalink / raw)
  To: Yehezkel Bernat
  Cc: LKML, Andreas Noever, Michael Jamet, Rafael J . Wysocki,
	Len Brown, Lukas Wunner, Mario Limonciello, Anthony Wong,
	linux-acpi

On Fri, Jul 05, 2019 at 01:52:49PM +0300, Yehezkel Bernat wrote:
> > @@ -2054,6 +2059,7 @@ struct tb *icm_probe(struct tb_nhi *nhi)
> >         case PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_NHI:
> >         case PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_NHI:
> >                 icm->max_boot_acl = ICM_AR_PREBOOT_ACL_ENTRIES;
> > +               icm->can_upgrade_nvm = true;
> 
> Shouldn't this be also !x86_apple_machine just like AR?
> (For FR, we don't use ICM on Apple machines, as much as I remember, so it's fine
> to enable it there unconditionally for ICM code path.)

Yes, good point. I'll fix it up.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 3/8] thunderbolt: Use 32-bit writes when writing ring producer/consumer
  2019-07-05  9:57 ` [PATCH 3/8] thunderbolt: Use 32-bit writes when writing ring producer/consumer Mika Westerberg
@ 2019-07-05 11:09   ` Yehezkel Bernat
  2019-07-05 11:24     ` Mika Westerberg
  2019-07-05 16:04     ` David Laight
  0 siblings, 2 replies; 30+ messages in thread
From: Yehezkel Bernat @ 2019-07-05 11:09 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: LKML, Andreas Noever, Michael Jamet, Rafael J . Wysocki,
	Len Brown, Lukas Wunner, Mario Limonciello, Anthony Wong,
	linux-acpi

On Fri, Jul 5, 2019 at 12:58 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> The register access should be using 32-bit reads/writes according to the
> datasheet. With the previous generation hardware 16-bit writes have been
> working but starting with ICL this is not the case anymore so fix
> producer/consumer register update to use correct width register address.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/thunderbolt/nhi.c | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
> index 27fbe62c7ddd..09242653da67 100644
> --- a/drivers/thunderbolt/nhi.c
> +++ b/drivers/thunderbolt/nhi.c
> @@ -143,9 +143,24 @@ static void __iomem *ring_options_base(struct tb_ring *ring)
>         return io;
>  }
>
> -static void ring_iowrite16desc(struct tb_ring *ring, u32 value, u32 offset)
> +static void ring_iowrite_prod(struct tb_ring *ring, u16 prod)
>  {
> -       iowrite16(value, ring_desc_base(ring) + offset);
> +       u32 val;
> +
> +       val = ioread32(ring_desc_base(ring) + 8);
> +       val &= 0x0000ffff;
> +       val |= prod << 16;
> +       iowrite32(val, ring_desc_base(ring) + 8);
> +}
> +
> +static void ring_iowrite_cons(struct tb_ring *ring, u16 cons)
> +{
> +       u32 val;
> +
> +       val = ioread32(ring_desc_base(ring) + 8);
> +       val &= 0xffff0000;
> +       val |= cons;
> +       iowrite32(val, ring_desc_base(ring) + 8);
>  }
>
>  static void ring_iowrite32desc(struct tb_ring *ring, u32 value, u32 offset)
> @@ -197,7 +212,10 @@ static void ring_write_descriptors(struct tb_ring *ring)
>                         descriptor->sof = frame->sof;
>                 }
>                 ring->head = (ring->head + 1) % ring->size;
> -               ring_iowrite16desc(ring, ring->head, ring->is_tx ? 10 : 8);
> +               if (ring->is_tx)
> +                       ring_iowrite_prod(ring, ring->head);
> +               else
> +                       ring_iowrite_cons(ring, ring->head);

Really a matter of taste, but maybe you want to consider having a single
function, with a 3rd parameter, bool is_tx.
The calls here will be unified to:
        ring_iowrite(ring, ring->head, ring->is_tx);
(No condition is needed here).

The implementation uses the new parameter to decide which part of the register
to mask, reducing the code duplication (in my eyes):

        val = ioread32(ring_desc_base(ring) + 8);
        if (is_tx) {
                val &= 0x0000ffff;
                val |= value << 16;
        } else {
                val &= 0xffff0000;
                val |= value;
        }
        iowrite32(val, ring_desc_base(ring) + 8);

I'm not sure if it improves the readability or makes it worse. Your call.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 3/8] thunderbolt: Use 32-bit writes when writing ring producer/consumer
  2019-07-05 11:09   ` Yehezkel Bernat
@ 2019-07-05 11:24     ` Mika Westerberg
  2019-07-05 16:04     ` David Laight
  1 sibling, 0 replies; 30+ messages in thread
From: Mika Westerberg @ 2019-07-05 11:24 UTC (permalink / raw)
  To: Yehezkel Bernat
  Cc: LKML, Andreas Noever, Michael Jamet, Rafael J . Wysocki,
	Len Brown, Lukas Wunner, Mario Limonciello, Anthony Wong,
	linux-acpi

On Fri, Jul 05, 2019 at 02:09:44PM +0300, Yehezkel Bernat wrote:
> On Fri, Jul 5, 2019 at 12:58 PM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > The register access should be using 32-bit reads/writes according to the
> > datasheet. With the previous generation hardware 16-bit writes have been
> > working but starting with ICL this is not the case anymore so fix
> > producer/consumer register update to use correct width register address.
> >
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  drivers/thunderbolt/nhi.c | 26 ++++++++++++++++++++++----
> >  1 file changed, 22 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
> > index 27fbe62c7ddd..09242653da67 100644
> > --- a/drivers/thunderbolt/nhi.c
> > +++ b/drivers/thunderbolt/nhi.c
> > @@ -143,9 +143,24 @@ static void __iomem *ring_options_base(struct tb_ring *ring)
> >         return io;
> >  }
> >
> > -static void ring_iowrite16desc(struct tb_ring *ring, u32 value, u32 offset)
> > +static void ring_iowrite_prod(struct tb_ring *ring, u16 prod)
> >  {
> > -       iowrite16(value, ring_desc_base(ring) + offset);
> > +       u32 val;
> > +
> > +       val = ioread32(ring_desc_base(ring) + 8);
> > +       val &= 0x0000ffff;
> > +       val |= prod << 16;
> > +       iowrite32(val, ring_desc_base(ring) + 8);
> > +}
> > +
> > +static void ring_iowrite_cons(struct tb_ring *ring, u16 cons)
> > +{
> > +       u32 val;
> > +
> > +       val = ioread32(ring_desc_base(ring) + 8);
> > +       val &= 0xffff0000;
> > +       val |= cons;
> > +       iowrite32(val, ring_desc_base(ring) + 8);
> >  }
> >
> >  static void ring_iowrite32desc(struct tb_ring *ring, u32 value, u32 offset)
> > @@ -197,7 +212,10 @@ static void ring_write_descriptors(struct tb_ring *ring)
> >                         descriptor->sof = frame->sof;
> >                 }
> >                 ring->head = (ring->head + 1) % ring->size;
> > -               ring_iowrite16desc(ring, ring->head, ring->is_tx ? 10 : 8);
> > +               if (ring->is_tx)
> > +                       ring_iowrite_prod(ring, ring->head);
> > +               else
> > +                       ring_iowrite_cons(ring, ring->head);
> 
> Really a matter of taste, but maybe you want to consider having a single
> function, with a 3rd parameter, bool is_tx.
> The calls here will be unified to:
>         ring_iowrite(ring, ring->head, ring->is_tx);
> (No condition is needed here).

I like the idea. We could even make it

	ring_iowrite_prod_cons(ring);

and then use ring->head and ring->is_tx inside.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 7/8] thunderbolt: Add support for Intel Ice Lake
  2019-07-05  9:57 ` [PATCH 7/8] thunderbolt: Add support for Intel Ice Lake Mika Westerberg
@ 2019-07-05 14:44   ` Yehezkel Bernat
  2019-07-05 14:51     ` Mika Westerberg
  2019-08-04 18:25   ` Lukas Wunner
  1 sibling, 1 reply; 30+ messages in thread
From: Yehezkel Bernat @ 2019-07-05 14:44 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: LKML, Andreas Noever, Michael Jamet, Rafael J . Wysocki,
	Len Brown, Lukas Wunner, Mario Limonciello, Anthony Wong,
	linux-acpi, raanan.avargil

On Fri, Jul 5, 2019 at 12:58 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> +static void icm_icl_rtd3_veto(struct tb *tb, const struct icm_pkg_header *hdr)
> +{
> +       const struct icm_icl_event_rtd3_veto *pkg =
> +               (const struct icm_icl_event_rtd3_veto *)hdr;
> +       struct icm *icm = tb_priv(tb);
> +
> +       tb_dbg(tb, "ICM rtd3 veto=0x%08x\n", pkg->veto_reason);
> +
> +       if (pkg->veto_reason) {
> +               if (!icm->veto) {
> +                       icm->veto = true;
> +                       /* Keep the domain powered while veto is in effect */
> +                       pm_runtime_get(&tb->dev);
> +               }
> +       } else {
> +               if (icm->veto) {
> +                       icm->veto = false;
> +                       /* Allow the domain suspend now */
> +                       pm_runtime_mark_last_busy(&tb->dev);
> +                       pm_runtime_put_autosuspend(&tb->dev);

Handling the removal of the veto is duplicated below. Worth introducing as a
helper function?

> +               }
> +       }
> +}
> +

...

> @@ -1853,6 +1943,18 @@ static void icm_complete(struct tb *tb)
>         if (tb->nhi->going_away)
>                 return;
>
> +       /*
> +        * If RTD3 was vetoed before we entered system suspend allow it
> +        * again now before driver ready is sent. Firmware sends a new RTD3
> +        * veto if it is still the case after we have sent it driver ready
> +        * command.
> +        */
> +       if (icm->veto) {
> +               icm->veto = false;
> +               pm_runtime_mark_last_busy(&tb->dev);
> +               pm_runtime_put_autosuspend(&tb->dev);
> +       }
> +

Here is the duplication.


> +static int nhi_suspend_power_down(struct tb *tb)
> +{
> +       int ret;
> +
> +       /*
> +        * If there is no device connected we need to perform an additional
> +        * handshake through LC mailbox and force power down before
> +        * entering D3.
> +        */
> +       ret = device_for_each_child(&tb->root_switch->dev, NULL,
> +                                   nhi_device_connected);
> +       if (!ret) {
> +               lc_mailbox_cmd(tb->nhi, LC_PREPARE_FOR_RESET);
> +               ret = lc_mailbox_cmd_complete(tb->nhi,
> +                                             LC_MAILBOX_TIMEOUT);
> +               if (ret)
> +                       return ret;
> +
> +               return nhi_power_down(tb->nhi);

Just to be sure: unforce power is done only if no device is connected?
My understanding of the comment above was that unforce power should be done
anyway (so it should be outside of this if block), and the difference between
the cases is only about the additional LC mailbox message. I guess I misread it.

> +       }
> +
> +       return 0;
> +}
> +

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 7/8] thunderbolt: Add support for Intel Ice Lake
  2019-07-05 14:44   ` Yehezkel Bernat
@ 2019-07-05 14:51     ` Mika Westerberg
  2019-07-05 15:02       ` Yehezkel Bernat
  0 siblings, 1 reply; 30+ messages in thread
From: Mika Westerberg @ 2019-07-05 14:51 UTC (permalink / raw)
  To: Yehezkel Bernat
  Cc: LKML, Andreas Noever, Michael Jamet, Rafael J . Wysocki,
	Len Brown, Lukas Wunner, Mario Limonciello, Anthony Wong,
	linux-acpi, raanan.avargil

On Fri, Jul 05, 2019 at 05:44:01PM +0300, Yehezkel Bernat wrote:
> On Fri, Jul 5, 2019 at 12:58 PM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > +static void icm_icl_rtd3_veto(struct tb *tb, const struct icm_pkg_header *hdr)
> > +{
> > +       const struct icm_icl_event_rtd3_veto *pkg =
> > +               (const struct icm_icl_event_rtd3_veto *)hdr;
> > +       struct icm *icm = tb_priv(tb);
> > +
> > +       tb_dbg(tb, "ICM rtd3 veto=0x%08x\n", pkg->veto_reason);
> > +
> > +       if (pkg->veto_reason) {
> > +               if (!icm->veto) {
> > +                       icm->veto = true;
> > +                       /* Keep the domain powered while veto is in effect */
> > +                       pm_runtime_get(&tb->dev);
> > +               }
> > +       } else {
> > +               if (icm->veto) {
> > +                       icm->veto = false;
> > +                       /* Allow the domain suspend now */
> > +                       pm_runtime_mark_last_busy(&tb->dev);
> > +                       pm_runtime_put_autosuspend(&tb->dev);
> 
> Handling the removal of the veto is duplicated below. Worth introducing as a
> helper function?
> 
> > +               }
> > +       }
> > +}
> > +
> 
> ...
> 
> > @@ -1853,6 +1943,18 @@ static void icm_complete(struct tb *tb)
> >         if (tb->nhi->going_away)
> >                 return;
> >
> > +       /*
> > +        * If RTD3 was vetoed before we entered system suspend allow it
> > +        * again now before driver ready is sent. Firmware sends a new RTD3
> > +        * veto if it is still the case after we have sent it driver ready
> > +        * command.
> > +        */
> > +       if (icm->veto) {
> > +               icm->veto = false;
> > +               pm_runtime_mark_last_busy(&tb->dev);
> > +               pm_runtime_put_autosuspend(&tb->dev);
> > +       }
> > +
> 
> Here is the duplication.

Indeed, I'll put it to a helper function.

> > +static int nhi_suspend_power_down(struct tb *tb)
> > +{
> > +       int ret;
> > +
> > +       /*
> > +        * If there is no device connected we need to perform an additional
> > +        * handshake through LC mailbox and force power down before
> > +        * entering D3.
> > +        */
> > +       ret = device_for_each_child(&tb->root_switch->dev, NULL,
> > +                                   nhi_device_connected);
> > +       if (!ret) {
> > +               lc_mailbox_cmd(tb->nhi, LC_PREPARE_FOR_RESET);
> > +               ret = lc_mailbox_cmd_complete(tb->nhi,
> > +                                             LC_MAILBOX_TIMEOUT);
> > +               if (ret)
> > +                       return ret;
> > +
> > +               return nhi_power_down(tb->nhi);
> 
> Just to be sure: unforce power is done only if no device is connected?
> My understanding of the comment above was that unforce power should be done
> anyway (so it should be outside of this if block), and the difference between
> the cases is only about the additional LC mailbox message. I guess I misread it.

nhi_power_down() should be only called if no device was connected so it
should be in correct place. I can try to clarify the comment a bit,
though.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 0/8] thunderbolt: Intel Ice Lake support
  2019-07-05 10:33 ` [PATCH 0/8] thunderbolt: Intel Ice Lake support Mika Westerberg
@ 2019-07-05 14:56   ` Yehezkel Bernat
  0 siblings, 0 replies; 30+ messages in thread
From: Yehezkel Bernat @ 2019-07-05 14:56 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: LKML, Andreas Noever, Michael Jamet, Rafael J . Wysocki,
	Len Brown, Lukas Wunner, Mario Limonciello, Anthony Wong,
	linux-acpi, Rajmohan Mani, Raanan Avargil

On Fri, Jul 5, 2019 at 1:33 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> On Fri, Jul 05, 2019 at 12:57:52PM +0300, Mika Westerberg wrote:
> > Hi all,
> >
> > With the exception of the first patch which is fix, this series enables
> > Thunderbolt on Intel Ice Lake. Biggest difference from the previous
> > controllers is that the Thunderbolt controller is now integrated as part of
> > the SoC. The firmware messages pretty much follow Titan Ridge but there are
> > some differences as well (such as the new RTD3 veto notification). Also Ice
> > Lake does not implement security levels so DMA protection is handled by IOMMU.
> >
> > This is v5.4 material but I'm sending it out now because I will be on
> > vacation next 4 weeks mostly without internet access. When I get back I'll
> > gather all the comments and update the series accordingly.
> >
> > Thanks!
> >
> > Mika Westerberg (8):
> >   thunderbolt: Correct path indices for PCIe tunnel
> >   thunderbolt: Move NVM upgrade support flag to struct icm
> >   thunderbolt: Use 32-bit writes when writing ring producer/consumer
> >   thunderbolt: Do not fail adding switch if some port is not implemented
> >   thunderbolt: Hide switch attributes that are not set
> >   thunderbolt: Expose active parts of NVM even if upgrade is not supported
> >   thunderbolt: Add support for Intel Ice Lake
> >   ACPI / property: Add two new Thunderbolt property GUIDs to the list
>
> Forgot to Cc Raanan and Raj, now added. Sorry about that. The patch
> series can also be viewed here:
>
>   https://lore.kernel.org/lkml/20190705095800.43534-1-mika.westerberg@linux.intel.com/T/#m9cb5a393dfc79f1c2212d0787b6bad5b689db6bd

Besides a few comments, LGTM.

For Thunderbolt patches,
Reviewed-by: Yehezkel Bernat <YehezkelShB@gmail.com>

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 7/8] thunderbolt: Add support for Intel Ice Lake
  2019-07-05 14:51     ` Mika Westerberg
@ 2019-07-05 15:02       ` Yehezkel Bernat
  0 siblings, 0 replies; 30+ messages in thread
From: Yehezkel Bernat @ 2019-07-05 15:02 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: LKML, Andreas Noever, Michael Jamet, Rafael J . Wysocki,
	Len Brown, Lukas Wunner, Mario Limonciello, Anthony Wong,
	linux-acpi, Raanan Avargil

On Fri, Jul 5, 2019 at 5:51 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> > > +static int nhi_suspend_power_down(struct tb *tb)
> > > +{
> > > +       int ret;
> > > +
> > > +       /*
> > > +        * If there is no device connected we need to perform an additional
> > > +        * handshake through LC mailbox and force power down before
> > > +        * entering D3.
> > > +        */
> > > +       ret = device_for_each_child(&tb->root_switch->dev, NULL,
> > > +                                   nhi_device_connected);
> > > +       if (!ret) {
> > > +               lc_mailbox_cmd(tb->nhi, LC_PREPARE_FOR_RESET);
> > > +               ret = lc_mailbox_cmd_complete(tb->nhi,
> > > +                                             LC_MAILBOX_TIMEOUT);
> > > +               if (ret)
> > > +                       return ret;
> > > +
> > > +               return nhi_power_down(tb->nhi);
> >
> > Just to be sure: unforce power is done only if no device is connected?
> > My understanding of the comment above was that unforce power should be done
> > anyway (so it should be outside of this if block), and the difference between
> > the cases is only about the additional LC mailbox message. I guess I misread it.
>
> nhi_power_down() should be only called if no device was connected so it
> should be in correct place. I can try to clarify the comment a bit,
> though.

Maybe adding the word "both" ("to perform both an additional") will make it
clearer. Maybe removing the "additional" (which to my ears sounds like "an
additional operation besides the normal one, to unforce power") is enough.
Again, your call. I'm not sure it's strictly needed, maybe it's just me.

Thanks!

^ permalink raw reply	[flat|nested] 30+ messages in thread

* RE: [PATCH 3/8] thunderbolt: Use 32-bit writes when writing ring producer/consumer
  2019-07-05 11:09   ` Yehezkel Bernat
  2019-07-05 11:24     ` Mika Westerberg
@ 2019-07-05 16:04     ` David Laight
  2019-08-07 16:13       ` Mika Westerberg
  1 sibling, 1 reply; 30+ messages in thread
From: David Laight @ 2019-07-05 16:04 UTC (permalink / raw)
  To: 'Yehezkel Bernat', Mika Westerberg
  Cc: LKML, Andreas Noever, Michael Jamet, Rafael J . Wysocki,
	Len Brown, Lukas Wunner, Mario Limonciello, Anthony Wong,
	linux-acpi

From: Yehezkel Bernat
> Sent: 05 July 2019 12:10
> On Fri, Jul 5, 2019 at 12:58 PM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > The register access should be using 32-bit reads/writes according to the
> > datasheet. With the previous generation hardware 16-bit writes have been
> > working but starting with ICL this is not the case anymore so fix
> > producer/consumer register update to use correct width register address.
> >
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  drivers/thunderbolt/nhi.c | 26 ++++++++++++++++++++++----
> >  1 file changed, 22 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
> > index 27fbe62c7ddd..09242653da67 100644
> > --- a/drivers/thunderbolt/nhi.c
> > +++ b/drivers/thunderbolt/nhi.c
> > @@ -143,9 +143,24 @@ static void __iomem *ring_options_base(struct tb_ring *ring)
> >         return io;
> >  }
> >
> > -static void ring_iowrite16desc(struct tb_ring *ring, u32 value, u32 offset)
> > +static void ring_iowrite_prod(struct tb_ring *ring, u16 prod)
> >  {
> > -       iowrite16(value, ring_desc_base(ring) + offset);
> > +       u32 val;
> > +
> > +       val = ioread32(ring_desc_base(ring) + 8);
> > +       val &= 0x0000ffff;
> > +       val |= prod << 16;
> > +       iowrite32(val, ring_desc_base(ring) + 8);
> > +}
> > +
> > +static void ring_iowrite_cons(struct tb_ring *ring, u16 cons)
> > +{
> > +       u32 val;
> > +
> > +       val = ioread32(ring_desc_base(ring) + 8);
> > +       val &= 0xffff0000;
> > +       val |= cons;
> > +       iowrite32(val, ring_desc_base(ring) + 8);
> >  }
> >
> >  static void ring_iowrite32desc(struct tb_ring *ring, u32 value, u32 offset)
> > @@ -197,7 +212,10 @@ static void ring_write_descriptors(struct tb_ring *ring)
> >                         descriptor->sof = frame->sof;
> >                 }
> >                 ring->head = (ring->head + 1) % ring->size;
> > -               ring_iowrite16desc(ring, ring->head, ring->is_tx ? 10 : 8);
> > +               if (ring->is_tx)
> > +                       ring_iowrite_prod(ring, ring->head);
> > +               else
> > +                       ring_iowrite_cons(ring, ring->head);
> 
> Really a matter of taste, but maybe you want to consider having a single
> function, with a 3rd parameter, bool is_tx.
> The calls here will be unified to:
>         ring_iowrite(ring, ring->head, ring->is_tx);
> (No condition is needed here).
> 
> The implementation uses the new parameter to decide which part of the register
> to mask, reducing the code duplication (in my eyes):
> 
>         val = ioread32(ring_desc_base(ring) + 8);
>         if (is_tx) {
>                 val &= 0x0000ffff;
>                 val |= value << 16;
>         } else {
>                 val &= 0xffff0000;
>                 val |= value;
>         }
>         iowrite32(val, ring_desc_base(ring) + 8);
> 
> I'm not sure if it improves the readability or makes it worse. Your call.

Gah, that is all horrid beyond belief.
If a 32bit write is valid then the hardware must not be updating
the other 16 bits.
In which case the driver knows what they should be.
So it can do a single 32bit write of the required value.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 8/8] ACPI / property: Add two new Thunderbolt property GUIDs to the list
  2019-07-05  9:58 ` [PATCH 8/8] ACPI / property: Add two new Thunderbolt property GUIDs to the list Mika Westerberg
@ 2019-07-09  8:18   ` Rafael J. Wysocki
  0 siblings, 0 replies; 30+ messages in thread
From: Rafael J. Wysocki @ 2019-07-09  8:18 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: Linux Kernel Mailing List, Andreas Noever, Michael Jamet,
	Yehezkel Bernat, Rafael J . Wysocki, Len Brown, Lukas Wunner,
	Mario Limonciello, Anthony Wong, ACPI Devel Maling List

On Fri, Jul 5, 2019 at 11:58 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> Ice Lake Thunderbolt controller includes two new device property
> compatible properties that we need to be able to extract in the driver
> so add them to the growing array of GUIDs.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/acpi/property.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index da3ced297f19..07cbacbab861 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -39,6 +39,12 @@ static const guid_t prp_guids[] = {
>         /* External facing port GUID: efcc06cc-73ac-4bc3-bff0-76143807c389 */
>         GUID_INIT(0xefcc06cc, 0x73ac, 0x4bc3,
>                   0xbf, 0xf0, 0x76, 0x14, 0x38, 0x07, 0xc3, 0x89),
> +       /* TBT GUID for IMR_VALID: c44d002f-69f9-4e7d-a904-a7baabdf43f7 */

I'd prefer Thunderbolt to be spelled in full here (and below).

LGTM otherwise.

> +       GUID_INIT(0xc44d002f, 0x69f9, 0x4e7d,
> +                 0xa9, 0x04, 0xa7, 0xba, 0xab, 0xdf, 0x43, 0xf7),
> +       /* TBT GUID for WAKE_SUPPORTED: 6c501103-c189-4296-ba72-9bf5a26ebe5d */
> +       GUID_INIT(0x6c501103, 0xc189, 0x4296,
> +                 0xba, 0x72, 0x9b, 0xf5, 0xa2, 0x6e, 0xbe, 0x5d),
>  };
>
>  /* ACPI _DSD data subnodes GUID: dbb8e3e6-5886-4ba6-8795-1319f52a966b */
> --
> 2.20.1
>

^ permalink raw reply	[flat|nested] 30+ messages in thread

* RE: [PATCH 2/8] thunderbolt: Move NVM upgrade support flag to struct icm
  2019-07-05 10:58     ` Mika Westerberg
@ 2019-07-09 15:11       ` Mario.Limonciello
  2019-08-05 13:15         ` Mika Westerberg
  0 siblings, 1 reply; 30+ messages in thread
From: Mario.Limonciello @ 2019-07-09 15:11 UTC (permalink / raw)
  To: mika.westerberg, yehezkelshb
  Cc: linux-kernel, andreas.noever, michael.jamet, rjw, lenb, lukas,
	anthony.wong, linux-acpi

> -----Original Message-----
> From: Mika Westerberg <mika.westerberg@linux.intel.com>
> Sent: Friday, July 5, 2019 5:58 AM
> To: Yehezkel Bernat
> Cc: LKML; Andreas Noever; Michael Jamet; Rafael J . Wysocki; Len Brown; Lukas
> Wunner; Limonciello, Mario; Anthony Wong; linux-acpi@vger.kernel.org
> Subject: Re: [PATCH 2/8] thunderbolt: Move NVM upgrade support flag to struct
> icm
> 
> 
> [EXTERNAL EMAIL]
> 
> On Fri, Jul 05, 2019 at 01:52:49PM +0300, Yehezkel Bernat wrote:
> > > @@ -2054,6 +2059,7 @@ struct tb *icm_probe(struct tb_nhi *nhi)
> > >         case PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_NHI:
> > >         case PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_NHI:
> > >                 icm->max_boot_acl = ICM_AR_PREBOOT_ACL_ENTRIES;
> > > +               icm->can_upgrade_nvm = true;
> >
> > Shouldn't this be also !x86_apple_machine just like AR?
> > (For FR, we don't use ICM on Apple machines, as much as I remember, so it's fine
> > to enable it there unconditionally for ICM code path.)
> 
> Yes, good point. I'll fix it up.

Another thought - does the TR or AR ID's setting can_upgrade_nvm to !x86_apple_machine
show up in anything like a dock or is it only host controllers?  If it's in docks, then it might be worth
only blocking on apple if it's a host.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 7/8] thunderbolt: Add support for Intel Ice Lake
  2019-07-05  9:57 ` [PATCH 7/8] thunderbolt: Add support for Intel Ice Lake Mika Westerberg
  2019-07-05 14:44   ` Yehezkel Bernat
@ 2019-08-04 18:25   ` Lukas Wunner
  2019-08-05 14:16     ` Mika Westerberg
  1 sibling, 1 reply; 30+ messages in thread
From: Lukas Wunner @ 2019-08-04 18:25 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: linux-kernel, Andreas Noever, Michael Jamet, Yehezkel Bernat,
	Rafael J . Wysocki, Len Brown, Mario.Limonciello, Anthony Wong,
	linux-acpi

On Fri, Jul 05, 2019 at 12:57:59PM +0300, Mika Westerberg wrote:
> @@ -891,16 +1020,23 @@ static int nhi_resume_noirq(struct device *dev)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
>  	struct tb *tb = pci_get_drvdata(pdev);
> +	int ret;
> +
> +	ret = nhi_power_up(tb->nhi);

Some general thoughts:

You're enabling Force Power during the resume_noirq phase of the NHI.

If TBT devices were already connected before system sleep, the PCI core
walks down the hierarchy during resume_noirq, puts each device into D0 and
restores config space.  For this to work, the NHI must have set up tunnels
to attached devices.  But it can hardly do that until Force Power is
enabled.  Yet I cannot see any provision here which causes the hotplug
bridges to wait for the NHI to finish its resume_noirq phase.

Don't you need that?

On Macs we have quirk_apple_wait_for_thunderbolt() to resolve this issue.

And a commit has been rotting on my development branch for years
which replaces quirk_apple_wait_for_thunderbolt() with device links:

https://github.com/l1k/linux/commit/8cbb1d589660


Also, you recently posted a patch stating that on Ice Lake, the NHI and
hotplug bridges share an ACPI Power Resource:

https://patchwork.kernel.org/patch/11015233/

How does that relate to the Force Power and Go2Sx dance implemented in
the present patch?  Does the ACPI Power Resource toggle the same Force
Power and Go2Sx registers as this patch here?  Or is that an additional
power management mechanism?  If Force Power is off on the NHI, are the
hotplug bridges' config and mmio spaces accessible?  All of this should
be documented in code comments or at least the commit message.


There's a quirk for Macs with Cactus Ridge to perform a Go2Sx dance
during the suspend_noirq phase, quirk_apple_poweroff_thunderbolt().
Can this be reimplemented using the infrastructure you're adding
in this patch?


On Macs with Thunderbolt 1 and 2, there are ACPI methods to toggle
Force Power in the NHI's namespace.  However disabling Force Power
not only cuts power to the NHI but also to the integrated PCIe switch.
For this reason, I've implemented power management on these Macs using
a struct dev_pm_domain which is assigned to the upstream bridge of
the integrated PCIe switch.  That way, power isn't cut until the
Thunderbolt controller's top-most device in the hierarchy is runtime
suspended:

https://github.com/l1k/linux/commit/4db7f0b1f5c9

I'm wondering if it would make sense to similarly assign a dev_pm_domain
to the NHI on Ice Lake.


One thing that bothers me a bit with the present patch is that nhi.c
is cluttered up with code specific to Ice Lake.  That doesn't seem
sustainable going forward as I expect we'll have to add plenty of
other quirks once USB4 chipsets become available.  Ideally the file
should contain only (or mostly) generic code and quirks should be
contained in separate files which need not be compiled in on
unrelated arches.  That's why in the above-linked commit to add
Apple-specific power management, I put all the code in a pm_apple.c
and nhi.c only calls tb_pm_apple_init() / _fini(), which become
empty inline stubs if CONFIG_ACPI or CONFIG_PM isn't enabled.


> --- a/drivers/thunderbolt/nhi.c
> +++ b/drivers/thunderbolt/nhi.c
> @@ -38,6 +40,60 @@
>  #define MSIX_MAX_VECS		16
>  
>  #define NHI_MAILBOX_TIMEOUT	500 /* ms */
> +#define LC_MAILBOX_TIMEOUT	500 /* ms */
> +
> +enum lc_mailbox_cmd {
> +	LC_GO2SX = 0x02,
> +	LC_GO2SX_NO_WAKE = 0x03,
> +	LC_PREPARE_FOR_RESET = 0x21,
> +};

Shouldn't these live together with the other LC macros in tb_regs.h?


> +	ret = device_for_each_child(&tb->root_switch->dev, NULL,
> +				    nhi_device_connected);
> +	if (!ret) {
> +		lc_mailbox_cmd(tb->nhi, LC_PREPARE_FOR_RESET);
> +		ret = lc_mailbox_cmd_complete(tb->nhi,
> +					      LC_MAILBOX_TIMEOUT);
> +		if (ret)
> +			return ret;
> +
> +		return nhi_power_down(tb->nhi);
> +	}
> +
> +	return 0;
> +}

Why not:

	if (ret)
		return 0;

and that way reduce the indentation of the code in your if-block by 1 tab?

Thanks,

Lukas

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/8] thunderbolt: Move NVM upgrade support flag to struct icm
  2019-07-09 15:11       ` Mario.Limonciello
@ 2019-08-05 13:15         ` Mika Westerberg
  0 siblings, 0 replies; 30+ messages in thread
From: Mika Westerberg @ 2019-08-05 13:15 UTC (permalink / raw)
  To: Mario.Limonciello
  Cc: yehezkelshb, linux-kernel, andreas.noever, michael.jamet, rjw,
	lenb, lukas, anthony.wong, linux-acpi

On Tue, Jul 09, 2019 at 03:11:15PM +0000, Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Sent: Friday, July 5, 2019 5:58 AM
> > To: Yehezkel Bernat
> > Cc: LKML; Andreas Noever; Michael Jamet; Rafael J . Wysocki; Len Brown; Lukas
> > Wunner; Limonciello, Mario; Anthony Wong; linux-acpi@vger.kernel.org
> > Subject: Re: [PATCH 2/8] thunderbolt: Move NVM upgrade support flag to struct
> > icm
> > 
> > 
> > [EXTERNAL EMAIL]
> > 
> > On Fri, Jul 05, 2019 at 01:52:49PM +0300, Yehezkel Bernat wrote:
> > > > @@ -2054,6 +2059,7 @@ struct tb *icm_probe(struct tb_nhi *nhi)
> > > >         case PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_NHI:
> > > >         case PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_NHI:
> > > >                 icm->max_boot_acl = ICM_AR_PREBOOT_ACL_ENTRIES;
> > > > +               icm->can_upgrade_nvm = true;
> > >
> > > Shouldn't this be also !x86_apple_machine just like AR?
> > > (For FR, we don't use ICM on Apple machines, as much as I remember, so it's fine
> > > to enable it there unconditionally for ICM code path.)
> > 
> > Yes, good point. I'll fix it up.
> 
> Another thought - does the TR or AR ID's setting can_upgrade_nvm to !x86_apple_machine
> show up in anything like a dock or is it only host controllers?  If it's in docks, then it might be worth
> only blocking on apple if it's a host.

It affects only hosts so on Apple system you can't upgrade host NVM but
docks and other devices you can.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 7/8] thunderbolt: Add support for Intel Ice Lake
  2019-08-04 18:25   ` Lukas Wunner
@ 2019-08-05 14:16     ` Mika Westerberg
  0 siblings, 0 replies; 30+ messages in thread
From: Mika Westerberg @ 2019-08-05 14:16 UTC (permalink / raw)
  To: Lukas Wunner
  Cc: linux-kernel, Andreas Noever, Michael Jamet, Yehezkel Bernat,
	Rafael J . Wysocki, Len Brown, Mario.Limonciello, Anthony Wong,
	linux-acpi

On Sun, Aug 04, 2019 at 08:25:10PM +0200, Lukas Wunner wrote:
> On Fri, Jul 05, 2019 at 12:57:59PM +0300, Mika Westerberg wrote:
> > @@ -891,16 +1020,23 @@ static int nhi_resume_noirq(struct device *dev)
> >  {
> >  	struct pci_dev *pdev = to_pci_dev(dev);
> >  	struct tb *tb = pci_get_drvdata(pdev);
> > +	int ret;
> > +
> > +	ret = nhi_power_up(tb->nhi);
> 
> Some general thoughts:
> 
> You're enabling Force Power during the resume_noirq phase of the NHI.
> 
> If TBT devices were already connected before system sleep, the PCI core
> walks down the hierarchy during resume_noirq, puts each device into D0 and
> restores config space.  For this to work, the NHI must have set up tunnels
> to attached devices.  But it can hardly do that until Force Power is
> enabled.  Yet I cannot see any provision here which causes the hotplug
> bridges to wait for the NHI to finish its resume_noirq phase.
> 
> Don't you need that?

For the FW CM we don't because there is some magic ASL as part of the
ACPI power resources (shared between root ports and NHI) that causes the
firmware to establish tunnels as part of _ON() method.

> On Macs we have quirk_apple_wait_for_thunderbolt() to resolve this issue.
> 
> And a commit has been rotting on my development branch for years
> which replaces quirk_apple_wait_for_thunderbolt() with device links:
> 
> https://github.com/l1k/linux/commit/8cbb1d589660
> 
> 
> Also, you recently posted a patch stating that on Ice Lake, the NHI and
> hotplug bridges share an ACPI Power Resource:
> 
> https://patchwork.kernel.org/patch/11015233/
> 
> How does that relate to the Force Power and Go2Sx dance implemented in
> the present patch?  Does the ACPI Power Resource toggle the same Force
> Power and Go2Sx registers as this patch here?  Or is that an additional
> power management mechanism?

It is an additional mechanism (but I think it also touches force power
at some point). Force power here is not the same as with the discrete
chips. AFAIK it pretty much only loads and starts the firmware whereas
in discrete it actually controls power to the chip.

> If Force Power is off on the NHI, are the hotplug bridges' config and
> mmio spaces accessible?

Yes, they are all accessible.

> All of this should be documented in code comments or at least the
> commit message.

Will do.

> There's a quirk for Macs with Cactus Ridge to perform a Go2Sx dance
> during the suspend_noirq phase, quirk_apple_poweroff_thunderbolt().
> Can this be reimplemented using the infrastructure you're adding
> in this patch?

It might be possible to implement ->force_power for Cactus Ridge and do
the ACPI magic there instead. I'll try and see what it looks like.

> On Macs with Thunderbolt 1 and 2, there are ACPI methods to toggle
> Force Power in the NHI's namespace.  However disabling Force Power
> not only cuts power to the NHI but also to the integrated PCIe switch.
> For this reason, I've implemented power management on these Macs using
> a struct dev_pm_domain which is assigned to the upstream bridge of
> the integrated PCIe switch.  That way, power isn't cut until the
> Thunderbolt controller's top-most device in the hierarchy is runtime
> suspended:
> 
> https://github.com/l1k/linux/commit/4db7f0b1f5c9
> 
> I'm wondering if it would make sense to similarly assign a dev_pm_domain
> to the NHI on Ice Lake.

Using PM domain here does not feel right to me. The force power thing
here is not really about power (but I'm calling it that because that's
the what we use internally) so I think it should be part of normal
driver flows.

> One thing that bothers me a bit with the present patch is that nhi.c
> is cluttered up with code specific to Ice Lake.  That doesn't seem
> sustainable going forward as I expect we'll have to add plenty of
> other quirks once USB4 chipsets become available.  Ideally the file
> should contain only (or mostly) generic code and quirks should be
> contained in separate files which need not be compiled in on
> unrelated arches.

I originally intented to have ICL support in icm.c but I realized that
the same steps are most likely needed on Apple systems as well (I don't
have any so can't tell if that's really the case) and since these flows
are for the NHI I figured that's the correct place for it.

I agree with you that moving chip specific things to separate files
makes sense (such as icl.c and so on) in the long run to keep the driver
maintainable.

> That's why in the above-linked commit to add
> Apple-specific power management, I put all the code in a pm_apple.c
> and nhi.c only calls tb_pm_apple_init() / _fini(), which become
> empty inline stubs if CONFIG_ACPI or CONFIG_PM isn't enabled.
> 
> 
> > --- a/drivers/thunderbolt/nhi.c
> > +++ b/drivers/thunderbolt/nhi.c
> > @@ -38,6 +40,60 @@
> >  #define MSIX_MAX_VECS		16
> >  
> >  #define NHI_MAILBOX_TIMEOUT	500 /* ms */
> > +#define LC_MAILBOX_TIMEOUT	500 /* ms */
> > +
> > +enum lc_mailbox_cmd {
> > +	LC_GO2SX = 0x02,
> > +	LC_GO2SX_NO_WAKE = 0x03,
> > +	LC_PREPARE_FOR_RESET = 0x21,
> > +};
> 
> Shouldn't these live together with the other LC macros in tb_regs.h?

I've used tb_regs.h to keep registers and definitions that belong to
Thunderbolt fabric. The other LC macros are there because they are used
to access LC over TBT.

Here we access it through NHI and only use it in nhi.c.

No strong feelings moving it to tb_regs.h if you still think that's
more suitable.

> > +	ret = device_for_each_child(&tb->root_switch->dev, NULL,
> > +				    nhi_device_connected);
> > +	if (!ret) {
> > +		lc_mailbox_cmd(tb->nhi, LC_PREPARE_FOR_RESET);
> > +		ret = lc_mailbox_cmd_complete(tb->nhi,
> > +					      LC_MAILBOX_TIMEOUT);
> > +		if (ret)
> > +			return ret;
> > +
> > +		return nhi_power_down(tb->nhi);
> > +	}
> > +
> > +	return 0;
> > +}
> 
> Why not:
> 
> 	if (ret)
> 		return 0;
> 
> and that way reduce the indentation of the code in your if-block by 1 tab?

Indeed. Will do.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 3/8] thunderbolt: Use 32-bit writes when writing ring producer/consumer
  2019-07-05 16:04     ` David Laight
@ 2019-08-07 16:13       ` Mika Westerberg
  2019-08-07 16:22         ` David Laight
  0 siblings, 1 reply; 30+ messages in thread
From: Mika Westerberg @ 2019-08-07 16:13 UTC (permalink / raw)
  To: David Laight
  Cc: 'Yehezkel Bernat',
	LKML, Andreas Noever, Michael Jamet, Rafael J . Wysocki,
	Len Brown, Lukas Wunner, Mario Limonciello, Anthony Wong,
	linux-acpi

On Fri, Jul 05, 2019 at 04:04:19PM +0000, David Laight wrote:
> > Really a matter of taste, but maybe you want to consider having a single
> > function, with a 3rd parameter, bool is_tx.
> > The calls here will be unified to:
> >         ring_iowrite(ring, ring->head, ring->is_tx);
> > (No condition is needed here).
> > 
> > The implementation uses the new parameter to decide which part of the register
> > to mask, reducing the code duplication (in my eyes):
> > 
> >         val = ioread32(ring_desc_base(ring) + 8);
> >         if (is_tx) {
> >                 val &= 0x0000ffff;
> >                 val |= value << 16;
> >         } else {
> >                 val &= 0xffff0000;
> >                 val |= value;
> >         }
> >         iowrite32(val, ring_desc_base(ring) + 8);
> > 
> > I'm not sure if it improves the readability or makes it worse. Your call.
> 
> Gah, that is all horrid beyond belief.
> If a 32bit write is valid then the hardware must not be updating
> the other 16 bits.
> In which case the driver knows what they should be.
> So it can do a single 32bit write of the required value.

I'm not entirely sure I understand what you say above. Can you shed some
light on this by a concrete example how it should look like? :-)

^ permalink raw reply	[flat|nested] 30+ messages in thread

* RE: [PATCH 3/8] thunderbolt: Use 32-bit writes when writing ring producer/consumer
  2019-08-07 16:13       ` Mika Westerberg
@ 2019-08-07 16:22         ` David Laight
  2019-08-07 16:36           ` 'Mika Westerberg'
  0 siblings, 1 reply; 30+ messages in thread
From: David Laight @ 2019-08-07 16:22 UTC (permalink / raw)
  To: 'Mika Westerberg'
  Cc: 'Yehezkel Bernat',
	LKML, Andreas Noever, Michael Jamet, Rafael J . Wysocki,
	Len Brown, Lukas Wunner, Mario Limonciello, Anthony Wong,
	linux-acpi

From: Mika Westerberg
> Sent: 07 August 2019 17:14
> To: David Laight
> 
> On Fri, Jul 05, 2019 at 04:04:19PM +0000, David Laight wrote:
> > > Really a matter of taste, but maybe you want to consider having a single
> > > function, with a 3rd parameter, bool is_tx.
> > > The calls here will be unified to:
> > >         ring_iowrite(ring, ring->head, ring->is_tx);
> > > (No condition is needed here).
> > >
> > > The implementation uses the new parameter to decide which part of the register
> > > to mask, reducing the code duplication (in my eyes):
> > >
> > >         val = ioread32(ring_desc_base(ring) + 8);
> > >         if (is_tx) {
> > >                 val &= 0x0000ffff;
> > >                 val |= value << 16;
> > >         } else {
> > >                 val &= 0xffff0000;
> > >                 val |= value;
> > >         }
> > >         iowrite32(val, ring_desc_base(ring) + 8);
> > >
> > > I'm not sure if it improves the readability or makes it worse. Your call.
> >
> > Gah, that is all horrid beyond belief.
> > If a 32bit write is valid then the hardware must not be updating
> > the other 16 bits.
> > In which case the driver knows what they should be.
> > So it can do a single 32bit write of the required value.
> 
> I'm not entirely sure I understand what you say above. Can you shed some
> light on this by a concrete example how it should look like? :-)

The driver must know both the tx and rx ring values, so:
	iowrite32(tx_val << 16 | rx_val, ring_desc_base(ring) + 8);

The ioread32() is likely to be very slow - you only want to do them
if absolutely necessary.
The speed of the iowrite32() doesn't matter (much) since it is almost
certainly 'posted' and execution continues while the bus cycle is
in progress.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 3/8] thunderbolt: Use 32-bit writes when writing ring producer/consumer
  2019-08-07 16:22         ` David Laight
@ 2019-08-07 16:36           ` 'Mika Westerberg'
  2019-08-07 16:41             ` David Laight
  0 siblings, 1 reply; 30+ messages in thread
From: 'Mika Westerberg' @ 2019-08-07 16:36 UTC (permalink / raw)
  To: David Laight
  Cc: 'Yehezkel Bernat',
	LKML, Andreas Noever, Michael Jamet, Rafael J . Wysocki,
	Len Brown, Lukas Wunner, Mario Limonciello, Anthony Wong,
	linux-acpi

On Wed, Aug 07, 2019 at 04:22:26PM +0000, David Laight wrote:
> From: Mika Westerberg
> > Sent: 07 August 2019 17:14
> > To: David Laight
> > 
> > On Fri, Jul 05, 2019 at 04:04:19PM +0000, David Laight wrote:
> > > > Really a matter of taste, but maybe you want to consider having a single
> > > > function, with a 3rd parameter, bool is_tx.
> > > > The calls here will be unified to:
> > > >         ring_iowrite(ring, ring->head, ring->is_tx);
> > > > (No condition is needed here).
> > > >
> > > > The implementation uses the new parameter to decide which part of the register
> > > > to mask, reducing the code duplication (in my eyes):
> > > >
> > > >         val = ioread32(ring_desc_base(ring) + 8);
> > > >         if (is_tx) {
> > > >                 val &= 0x0000ffff;
> > > >                 val |= value << 16;
> > > >         } else {
> > > >                 val &= 0xffff0000;
> > > >                 val |= value;
> > > >         }
> > > >         iowrite32(val, ring_desc_base(ring) + 8);
> > > >
> > > > I'm not sure if it improves the readability or makes it worse. Your call.
> > >
> > > Gah, that is all horrid beyond belief.
> > > If a 32bit write is valid then the hardware must not be updating
> > > the other 16 bits.
> > > In which case the driver knows what they should be.
> > > So it can do a single 32bit write of the required value.
> > 
> > I'm not entirely sure I understand what you say above. Can you shed some
> > light on this by a concrete example how it should look like? :-)
> 
> The driver must know both the tx and rx ring values, so:
> 	iowrite32(tx_val << 16 | rx_val, ring_desc_base(ring) + 8);
>

I see. However, prod or cons side gets updated by the hardware as it
processes buffers and other side is only updated by the driver. I'm not
sure the above works here.

> The ioread32() is likely to be very slow - you only want to do them
> if absolutely necessary.
> The speed of the iowrite32() doesn't matter (much) since it is almost
> certainly 'posted' and execution continues while the bus cycle is
> in progress.

OK thanks for the explanation.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* RE: [PATCH 3/8] thunderbolt: Use 32-bit writes when writing ring producer/consumer
  2019-08-07 16:36           ` 'Mika Westerberg'
@ 2019-08-07 16:41             ` David Laight
  2019-08-08  9:57               ` 'Mika Westerberg'
  0 siblings, 1 reply; 30+ messages in thread
From: David Laight @ 2019-08-07 16:41 UTC (permalink / raw)
  To: 'Mika Westerberg'
  Cc: 'Yehezkel Bernat',
	LKML, Andreas Noever, Michael Jamet, Rafael J . Wysocki,
	Len Brown, Lukas Wunner, Mario Limonciello, Anthony Wong,
	linux-acpi

From: 'Mika Westerberg' [mailto:mika.westerberg@linux.intel.com]
> Sent: 07 August 2019 17:36
> 
> On Wed, Aug 07, 2019 at 04:22:26PM +0000, David Laight wrote:
> > From: Mika Westerberg
> > > Sent: 07 August 2019 17:14
> > > To: David Laight
> > >
> > > On Fri, Jul 05, 2019 at 04:04:19PM +0000, David Laight wrote:
> > > > > Really a matter of taste, but maybe you want to consider having a single
> > > > > function, with a 3rd parameter, bool is_tx.
> > > > > The calls here will be unified to:
> > > > >         ring_iowrite(ring, ring->head, ring->is_tx);
> > > > > (No condition is needed here).
> > > > >
> > > > > The implementation uses the new parameter to decide which part of the register
> > > > > to mask, reducing the code duplication (in my eyes):
> > > > >
> > > > >         val = ioread32(ring_desc_base(ring) + 8);
> > > > >         if (is_tx) {
> > > > >                 val &= 0x0000ffff;
> > > > >                 val |= value << 16;
> > > > >         } else {
> > > > >                 val &= 0xffff0000;
> > > > >                 val |= value;
> > > > >         }
> > > > >         iowrite32(val, ring_desc_base(ring) + 8);
> > > > >
> > > > > I'm not sure if it improves the readability or makes it worse. Your call.
> > > >
> > > > Gah, that is all horrid beyond belief.
> > > > If a 32bit write is valid then the hardware must not be updating
> > > > the other 16 bits.
> > > > In which case the driver knows what they should be.
> > > > So it can do a single 32bit write of the required value.
> > >
> > > I'm not entirely sure I understand what you say above. Can you shed some
> > > light on this by a concrete example how it should look like? :-)
> >
> > The driver must know both the tx and rx ring values, so:
> > 	iowrite32(tx_val << 16 | rx_val, ring_desc_base(ring) + 8);
> >
> 
> I see. However, prod or cons side gets updated by the hardware as it
> processes buffers and other side is only updated by the driver. I'm not
> sure the above works here.

If the hardware updates the other half of the 32bit word it doesn't ever work.
In that case you must do 16bit writes.
If the hardware is ignoring the byte-enables it is broken and unusable.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 3/8] thunderbolt: Use 32-bit writes when writing ring producer/consumer
  2019-08-07 16:41             ` David Laight
@ 2019-08-08  9:57               ` 'Mika Westerberg'
  2019-08-12  9:01                 ` David Laight
  0 siblings, 1 reply; 30+ messages in thread
From: 'Mika Westerberg' @ 2019-08-08  9:57 UTC (permalink / raw)
  To: David Laight
  Cc: 'Yehezkel Bernat',
	LKML, Andreas Noever, Michael Jamet, Rafael J . Wysocki,
	Len Brown, Lukas Wunner, Mario Limonciello, Anthony Wong,
	linux-acpi

On Wed, Aug 07, 2019 at 04:41:30PM +0000, David Laight wrote:
> From: 'Mika Westerberg' [mailto:mika.westerberg@linux.intel.com]
> > Sent: 07 August 2019 17:36
> > 
> > On Wed, Aug 07, 2019 at 04:22:26PM +0000, David Laight wrote:
> > > From: Mika Westerberg
> > > > Sent: 07 August 2019 17:14
> > > > To: David Laight
> > > >
> > > > On Fri, Jul 05, 2019 at 04:04:19PM +0000, David Laight wrote:
> > > > > > Really a matter of taste, but maybe you want to consider having a single
> > > > > > function, with a 3rd parameter, bool is_tx.
> > > > > > The calls here will be unified to:
> > > > > >         ring_iowrite(ring, ring->head, ring->is_tx);
> > > > > > (No condition is needed here).
> > > > > >
> > > > > > The implementation uses the new parameter to decide which part of the register
> > > > > > to mask, reducing the code duplication (in my eyes):
> > > > > >
> > > > > >         val = ioread32(ring_desc_base(ring) + 8);
> > > > > >         if (is_tx) {
> > > > > >                 val &= 0x0000ffff;
> > > > > >                 val |= value << 16;
> > > > > >         } else {
> > > > > >                 val &= 0xffff0000;
> > > > > >                 val |= value;
> > > > > >         }
> > > > > >         iowrite32(val, ring_desc_base(ring) + 8);
> > > > > >
> > > > > > I'm not sure if it improves the readability or makes it worse. Your call.
> > > > >
> > > > > Gah, that is all horrid beyond belief.
> > > > > If a 32bit write is valid then the hardware must not be updating
> > > > > the other 16 bits.
> > > > > In which case the driver knows what they should be.
> > > > > So it can do a single 32bit write of the required value.
> > > >
> > > > I'm not entirely sure I understand what you say above. Can you shed some
> > > > light on this by a concrete example how it should look like? :-)
> > >
> > > The driver must know both the tx and rx ring values, so:
> > > 	iowrite32(tx_val << 16 | rx_val, ring_desc_base(ring) + 8);
> > >
> > 
> > I see. However, prod or cons side gets updated by the hardware as it
> > processes buffers and other side is only updated by the driver. I'm not
> > sure the above works here.
> 
> If the hardware updates the other half of the 32bit word it doesn't ever work.
>
> In that case you must do 16bit writes.
> If the hardware is ignoring the byte-enables it is broken and unusable.

It is quite usable as I'm running this code on real hardware ;-) but
32-bit access is needed.

The low or high 16-bits are read-only depending on the ring (Tx or Rx)
and updated by the hardware. It ignores writes to that part so we could
do this for Tx ring:

	iowrite32(prod << 16, ring_desc_base(ring) + 8);

and this for Rx ring:

	iowrite32(cons, ring_desc_base(ring) + 8);

Do you see any issues with this?

^ permalink raw reply	[flat|nested] 30+ messages in thread

* RE: [PATCH 3/8] thunderbolt: Use 32-bit writes when writing ring producer/consumer
  2019-08-08  9:57               ` 'Mika Westerberg'
@ 2019-08-12  9:01                 ` David Laight
  0 siblings, 0 replies; 30+ messages in thread
From: David Laight @ 2019-08-12  9:01 UTC (permalink / raw)
  To: 'Mika Westerberg'
  Cc: 'Yehezkel Bernat',
	LKML, Andreas Noever, Michael Jamet, Rafael J . Wysocki,
	Len Brown, Lukas Wunner, Mario Limonciello, Anthony Wong,
	linux-acpi

From: 'Mika Westerberg'
> Sent: 08 August 2019 10:58
> On Wed, Aug 07, 2019 at 04:41:30PM +0000, David Laight wrote:
> > From: 'Mika Westerberg' [mailto:mika.westerberg@linux.intel.com]
> > > Sent: 07 August 2019 17:36
> > >
> > > On Wed, Aug 07, 2019 at 04:22:26PM +0000, David Laight wrote:
> > > > From: Mika Westerberg
> > > > > Sent: 07 August 2019 17:14
> > > > > To: David Laight
> > > > >
> > > > > On Fri, Jul 05, 2019 at 04:04:19PM +0000, David Laight wrote:
> > > > > > > Really a matter of taste, but maybe you want to consider having a single
> > > > > > > function, with a 3rd parameter, bool is_tx.
> > > > > > > The calls here will be unified to:
> > > > > > >         ring_iowrite(ring, ring->head, ring->is_tx);
> > > > > > > (No condition is needed here).
> > > > > > >
> > > > > > > The implementation uses the new parameter to decide which part of the register
> > > > > > > to mask, reducing the code duplication (in my eyes):
> > > > > > >
> > > > > > >         val = ioread32(ring_desc_base(ring) + 8);
> > > > > > >         if (is_tx) {
> > > > > > >                 val &= 0x0000ffff;
> > > > > > >                 val |= value << 16;
> > > > > > >         } else {
> > > > > > >                 val &= 0xffff0000;
> > > > > > >                 val |= value;
> > > > > > >         }
> > > > > > >         iowrite32(val, ring_desc_base(ring) + 8);
> > > > > > >
> > > > > > > I'm not sure if it improves the readability or makes it worse. Your call.
> > > > > >
> > > > > > Gah, that is all horrid beyond belief.
> > > > > > If a 32bit write is valid then the hardware must not be updating
> > > > > > the other 16 bits.
> > > > > > In which case the driver knows what they should be.
> > > > > > So it can do a single 32bit write of the required value.
> > > > >
> > > > > I'm not entirely sure I understand what you say above. Can you shed some
> > > > > light on this by a concrete example how it should look like? :-)
> > > >
> > > > The driver must know both the tx and rx ring values, so:
> > > > 	iowrite32(tx_val << 16 | rx_val, ring_desc_base(ring) + 8);
> > > >
> > >
> > > I see. However, prod or cons side gets updated by the hardware as it
> > > processes buffers and other side is only updated by the driver. I'm not
> > > sure the above works here.
> >
> > If the hardware updates the other half of the 32bit word it doesn't ever work.
> >
> > In that case you must do 16bit writes.
> > If the hardware is ignoring the byte-enables it is broken and unusable.
> 
> It is quite usable as I'm running this code on real hardware ;-) but
> 32-bit access is needed.
> 
> The low or high 16-bits are read-only depending on the ring (Tx or Rx)
> and updated by the hardware. It ignores writes to that part so we could
> do this for Tx ring:
> 
> 	iowrite32(prod << 16, ring_desc_base(ring) + 8);
> 
> and this for Rx ring:
> 
> 	iowrite32(cons, ring_desc_base(ring) + 8);
> 
> Do you see any issues with this?

No, just comment that the hardware ignores the write to the other bytes.

You probably want separate functions - to remove the mispredicted branch.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)


^ permalink raw reply	[flat|nested] 30+ messages in thread

end of thread, other threads:[~2019-08-12  9:01 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-05  9:57 [PATCH 0/8] thunderbolt: Intel Ice Lake support Mika Westerberg
2019-07-05  9:57 ` [PATCH 1/8] thunderbolt: Correct path indices for PCIe tunnel Mika Westerberg
2019-07-05  9:57 ` [PATCH 2/8] thunderbolt: Move NVM upgrade support flag to struct icm Mika Westerberg
2019-07-05 10:52   ` Yehezkel Bernat
2019-07-05 10:58     ` Mika Westerberg
2019-07-09 15:11       ` Mario.Limonciello
2019-08-05 13:15         ` Mika Westerberg
2019-07-05  9:57 ` [PATCH 3/8] thunderbolt: Use 32-bit writes when writing ring producer/consumer Mika Westerberg
2019-07-05 11:09   ` Yehezkel Bernat
2019-07-05 11:24     ` Mika Westerberg
2019-07-05 16:04     ` David Laight
2019-08-07 16:13       ` Mika Westerberg
2019-08-07 16:22         ` David Laight
2019-08-07 16:36           ` 'Mika Westerberg'
2019-08-07 16:41             ` David Laight
2019-08-08  9:57               ` 'Mika Westerberg'
2019-08-12  9:01                 ` David Laight
2019-07-05  9:57 ` [PATCH 4/8] thunderbolt: Do not fail adding switch if some port is not implemented Mika Westerberg
2019-07-05  9:57 ` [PATCH 5/8] thunderbolt: Hide switch attributes that are not set Mika Westerberg
2019-07-05  9:57 ` [PATCH 6/8] thunderbolt: Expose active parts of NVM even if upgrade is not supported Mika Westerberg
2019-07-05  9:57 ` [PATCH 7/8] thunderbolt: Add support for Intel Ice Lake Mika Westerberg
2019-07-05 14:44   ` Yehezkel Bernat
2019-07-05 14:51     ` Mika Westerberg
2019-07-05 15:02       ` Yehezkel Bernat
2019-08-04 18:25   ` Lukas Wunner
2019-08-05 14:16     ` Mika Westerberg
2019-07-05  9:58 ` [PATCH 8/8] ACPI / property: Add two new Thunderbolt property GUIDs to the list Mika Westerberg
2019-07-09  8:18   ` Rafael J. Wysocki
2019-07-05 10:33 ` [PATCH 0/8] thunderbolt: Intel Ice Lake support Mika Westerberg
2019-07-05 14:56   ` Yehezkel Bernat

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).