2019-10-01 11:39:42

by Mika Westerberg

Subject: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4

USB4 is a public spec based on Thunderbolt protocol. There are some
differences in register layouts and flows. In addition to PCIe and DP
tunneling, USB4 supports tunneling of USB 3.x. USB4 is also backward
compatible with Thunderbolt 3 (and older generations but the spec only
talks about 3rd generation). USB4 compliant devices can be identified by
checking USB4 version field in router configuration space.

This patch adds initial support for USB4 compliant hosts and devices
which enables following features provided by the existing functionality
in the driver:

- PCIe tunneling
- Display Port tunneling
- Host and device NVM firmware upgrade
- P2P networking

This brings the USB4 support to the same level that we already have for
Thunderbolt 1, 2 and 3 devices.

Note the spec talks about host and device "routers" but in the driver we
still use term "switch" in most places. Both can be used interchangeably.

This also updates the Kconfig entry accordingly.

Co-developed-by: Rajmohan Mani <[email protected]>
Signed-off-by: Rajmohan Mani <[email protected]>
Signed-off-by: Mika Westerberg <[email protected]>
---
drivers/thunderbolt/Kconfig | 9 +-
drivers/thunderbolt/Makefile | 2 +-
drivers/thunderbolt/eeprom.c | 53 ++-
drivers/thunderbolt/nhi.c | 3 +
drivers/thunderbolt/nhi.h | 2 +
drivers/thunderbolt/switch.c | 384 +++++++++++++-----
drivers/thunderbolt/tb.c | 20 +-
drivers/thunderbolt/tb.h | 36 ++
drivers/thunderbolt/tb_regs.h | 36 +-
drivers/thunderbolt/tunnel.c | 10 +-
drivers/thunderbolt/usb4.c | 722 ++++++++++++++++++++++++++++++++++
drivers/thunderbolt/xdomain.c | 6 +
12 files changed, 1162 insertions(+), 121 deletions(-)
create mode 100644 drivers/thunderbolt/usb4.c

diff --git a/drivers/thunderbolt/Kconfig b/drivers/thunderbolt/Kconfig
index fd9adca898ff..8193ec310bae 100644
--- a/drivers/thunderbolt/Kconfig
+++ b/drivers/thunderbolt/Kconfig
@@ -1,6 +1,6 @@
# SPDX-License-Identifier: GPL-2.0-only
menuconfig THUNDERBOLT
- tristate "Thunderbolt support"
+ tristate "USB4 (Thunderbolt) support"
depends on PCI
depends on X86 || COMPILE_TEST
select APPLE_PROPERTIES if EFI_STUB && X86
@@ -9,9 +9,10 @@ menuconfig THUNDERBOLT
select CRYPTO_HASH
select NVMEM
help
- Thunderbolt Controller driver. This driver is required if you
- want to hotplug Thunderbolt devices on Apple hardware or on PCs
- with Intel Falcon Ridge or newer.
+ USB4 (Thunderbolt) driver. USB4 is the public spec based on
+ Thunderbolt 3 protocol. This driver is required if you want to
+ hotplug Thunderbolt and USB4 compliant devices on Apple
+ hardware or on PCs with Intel Falcon Ridge or newer.

To compile this driver a module, choose M here. The module will be
called thunderbolt.
diff --git a/drivers/thunderbolt/Makefile b/drivers/thunderbolt/Makefile
index 001187c577bf..c0b2fd73dfbd 100644
--- a/drivers/thunderbolt/Makefile
+++ b/drivers/thunderbolt/Makefile
@@ -1,4 +1,4 @@
# SPDX-License-Identifier: GPL-2.0-only
obj-${CONFIG_THUNDERBOLT} := thunderbolt.o
thunderbolt-objs := nhi.o nhi_ops.o ctl.o tb.o switch.o cap.o path.o tunnel.o eeprom.o
-thunderbolt-objs += domain.o dma_port.o icm.o property.o xdomain.o lc.o
+thunderbolt-objs += domain.o dma_port.o icm.o property.o xdomain.o lc.o usb4.o
diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c
index 540e0105bcc0..921d164b3f35 100644
--- a/drivers/thunderbolt/eeprom.c
+++ b/drivers/thunderbolt/eeprom.c
@@ -487,6 +487,37 @@ static int tb_drom_copy_nvm(struct tb_switch *sw, u16 *size)
return ret;
}

+static int usb4_copy_host_drom(struct tb_switch *sw, u16 *size)
+{
+ int ret;
+
+ ret = usb4_switch_drom_read(sw, 14, size, sizeof(*size));
+ if (ret)
+ return ret;
+
+ /* Size includes CRC8 + UID + CRC32 */
+ *size += 1 + 8 + 4;
+ sw->drom = kzalloc(*size, GFP_KERNEL);
+ if (!sw->drom)
+ return -ENOMEM;
+
+ ret = usb4_switch_drom_read(sw, 0, sw->drom, *size);
+ if (ret) {
+ kfree(sw->drom);
+ sw->drom = NULL;
+ }
+
+ return ret;
+}
+
+static int tb_drom_read_n(struct tb_switch *sw, u16 offset, u8 *val,
+ size_t count)
+{
+ if (tb_switch_is_usb4(sw))
+ return usb4_switch_drom_read(sw, offset, val, count);
+ return tb_eeprom_read_n(sw, offset, val, count);
+}
+
/**
* tb_drom_read - copy drom to sw->drom and parse it
*/
@@ -512,14 +543,26 @@ int tb_drom_read(struct tb_switch *sw)
goto parse;

/*
- * The root switch contains only a dummy drom (header only,
- * no entries). Hardcode the configuration here.
+ * USB4 hosts may support reading DROM through router
+ * operations.
*/
- tb_drom_read_uid_only(sw, &sw->uid);
+ if (tb_switch_is_usb4(sw)) {
+ usb4_switch_read_uid(sw, &sw->uid);
+ if (!usb4_copy_host_drom(sw, &size))
+ goto parse;
+ } else {
+ /*
+ * The root switch contains only a dummy drom
+ * (header only, no entries). Hardcode the
+ * configuration here.
+ */
+ tb_drom_read_uid_only(sw, &sw->uid);
+ }
+
return 0;
}

- res = tb_eeprom_read_n(sw, 14, (u8 *) &size, 2);
+ res = tb_drom_read_n(sw, 14, (u8 *) &size, 2);
if (res)
return res;
size &= 0x3ff;
@@ -533,7 +576,7 @@ int tb_drom_read(struct tb_switch *sw)
sw->drom = kzalloc(size, GFP_KERNEL);
if (!sw->drom)
return -ENOMEM;
- res = tb_eeprom_read_n(sw, 0, sw->drom, size);
+ res = tb_drom_read_n(sw, 0, sw->drom, size);
if (res)
goto err;

diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
index 641b21b54460..1be491ecbb45 100644
--- a/drivers/thunderbolt/nhi.c
+++ b/drivers/thunderbolt/nhi.c
@@ -1271,6 +1271,9 @@ static struct pci_device_id nhi_ids[] = {
{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ICL_NHI1),
.driver_data = (kernel_ulong_t)&icl_nhi_ops },

+ /* Any USB4 compliant host */
+ { PCI_DEVICE_CLASS(PCI_CLASS_SERIAL_USB_USB4, ~0) },
+
{ 0,}
};

diff --git a/drivers/thunderbolt/nhi.h b/drivers/thunderbolt/nhi.h
index b7b973949f8e..5d276ee9b38e 100644
--- a/drivers/thunderbolt/nhi.h
+++ b/drivers/thunderbolt/nhi.h
@@ -74,4 +74,6 @@ extern const struct tb_nhi_ops icl_nhi_ops;
#define PCI_DEVICE_ID_INTEL_ICL_NHI1 0x8a0d
#define PCI_DEVICE_ID_INTEL_ICL_NHI0 0x8a17

+#define PCI_CLASS_SERIAL_USB_USB4 0x0c0340
+
#endif
diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index 8bc5d46011f8..2ccd1004920e 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -163,10 +163,12 @@ static int nvm_validate_and_write(struct tb_switch *sw)
image_size -= hdr_size;
}

+ if (tb_switch_is_usb4(sw))
+ return usb4_switch_nvm_write(sw, 0, buf, image_size);
return dma_port_flash_write(sw->dma_port, 0, buf, image_size);
}

-static int nvm_authenticate_host(struct tb_switch *sw)
+static int nvm_authenticate_host_dma_port(struct tb_switch *sw)
{
int ret;

@@ -195,7 +197,7 @@ static int nvm_authenticate_host(struct tb_switch *sw)
return 0;
}

-static int nvm_authenticate_device(struct tb_switch *sw)
+static int nvm_authenticate_device_dma_port(struct tb_switch *sw)
{
int ret, retries = 10;

@@ -232,6 +234,80 @@ static int nvm_authenticate_device(struct tb_switch *sw)
return -ETIMEDOUT;
}

+static void nvm_authenticate_start_dma_port(struct tb_switch *sw)
+{
+ struct pci_dev *root_port;
+
+ /*
+ * During host router NVM upgrade we should not allow root port to
+ * go into D3cold because some root ports cannot trigger PME
+ * itself. To be on the safe side keep the root port in D0 during
+ * the whole upgrade process.
+ */
+ root_port = pci_find_pcie_root_port(sw->tb->nhi->pdev);
+ if (root_port)
+ pm_runtime_get_noresume(&root_port->dev);
+}
+
+static void nvm_authenticate_complete_dma_port(struct tb_switch *sw)
+{
+ struct pci_dev *root_port;
+
+ root_port = pci_find_pcie_root_port(sw->tb->nhi->pdev);
+ if (root_port)
+ pm_runtime_put(&root_port->dev);
+}
+
+static inline bool nvm_readable(struct tb_switch *sw)
+{
+ if (tb_switch_is_usb4(sw)) {
+ /*
+ * USB4 devices must support NVM operations but it is
+ * optional for hosts. Therefore we query the NVM sector
+ * size here and if it is supported assume NVM
+ * operations are implemented.
+ */
+ return usb4_switch_nvm_sector_size(sw) > 0;
+ }
+
+ /* Thunderbolt 2 and 3 devices support NVM through DMA port */
+ return !!sw->dma_port;
+}
+
+static inline bool nvm_upgradeable(struct tb_switch *sw)
+{
+ if (sw->no_nvm_upgrade)
+ return false;
+ return nvm_readable(sw);
+}
+
+static inline int nvm_read(struct tb_switch *sw, unsigned int address,
+ void *buf, size_t size)
+{
+ if (tb_switch_is_usb4(sw))
+ return usb4_switch_nvm_read(sw, address, buf, size);
+ return dma_port_flash_read(sw->dma_port, address, buf, size);
+}
+
+static int nvm_authenticate(struct tb_switch *sw)
+{
+ int ret;
+
+ if (tb_switch_is_usb4(sw))
+ return usb4_switch_nvm_authenticate(sw);
+
+ if (!tb_route(sw)) {
+ nvm_authenticate_start_dma_port(sw);
+ ret = nvm_authenticate_host_dma_port(sw);
+ if (ret)
+ nvm_authenticate_complete_dma_port(sw);
+ } else {
+ ret = nvm_authenticate_device_dma_port(sw);
+ }
+
+ return ret;
+}
+
static int tb_switch_nvm_read(void *priv, unsigned int offset, void *val,
size_t bytes)
{
@@ -245,7 +321,7 @@ static int tb_switch_nvm_read(void *priv, unsigned int offset, void *val,
goto out;
}

- ret = dma_port_flash_read(sw->dma_port, offset, val, bytes);
+ ret = nvm_read(sw, offset, val, bytes);
mutex_unlock(&sw->tb->lock);

out:
@@ -322,9 +398,21 @@ static int tb_switch_nvm_add(struct tb_switch *sw)
u32 val;
int ret;

- if (!sw->dma_port)
+ if (!nvm_readable(sw))
return 0;

+ /*
+ * The NVM format of non-Intel hardware is not known so
+ * currently restrict NVM upgrade for Intel hardware. We may
+ * relax this in the future when we learn other NVM formats.
+ */
+ if (sw->config.vendor_id != PCI_VENDOR_ID_INTEL) {
+ dev_info(&sw->dev,
+ "NVM format of vendor %#x is not known, disabling NVM upgrade\n",
+ sw->config.vendor_id);
+ return 0;
+ }
+
nvm = kzalloc(sizeof(*nvm), GFP_KERNEL);
if (!nvm)
return -ENOMEM;
@@ -339,8 +427,7 @@ static int tb_switch_nvm_add(struct tb_switch *sw)
if (!sw->safe_mode) {
u32 nvm_size, hdr_size;

- ret = dma_port_flash_read(sw->dma_port, NVM_FLASH_SIZE, &val,
- sizeof(val));
+ ret = nvm_read(sw, NVM_FLASH_SIZE, &val, sizeof(val));
if (ret)
goto err_ida;

@@ -348,8 +435,7 @@ static int tb_switch_nvm_add(struct tb_switch *sw)
nvm_size = (SZ_1M << (val & 7)) / 8;
nvm_size = (nvm_size - hdr_size) / 2;

- ret = dma_port_flash_read(sw->dma_port, NVM_VERSION, &val,
- sizeof(val));
+ ret = nvm_read(sw, NVM_VERSION, &val, sizeof(val));
if (ret)
goto err_ida;

@@ -600,6 +686,24 @@ int tb_port_clear_counter(struct tb_port *port, int counter)
return tb_port_write(port, zero, TB_CFG_COUNTERS, 3 * counter, 3);
}

+/**
+ * tb_port_unlock() - Unlock downstream port
+ * @port: Port to unlock
+ *
+ * Needed for USB4 but can be called for any CIO/USB4 ports. Makes the
+ * downstream router accessible for CM.
+ */
+int tb_port_unlock(struct tb_port *port)
+{
+ if (tb_switch_is_icm(port->sw))
+ return 0;
+ if (!tb_port_is_null(port))
+ return -EINVAL;
+ if (tb_switch_is_usb4(port->sw))
+ return usb4_port_unlock(port);
+ return 0;
+}
+
/**
* tb_init_port() - initialize a port
*
@@ -631,6 +735,10 @@ static int tb_init_port(struct tb_port *port)
port->cap_phy = cap;
else
tb_port_WARN(port, "non switch port without a PHY\n");
+
+ cap = tb_port_find_cap(port, TB_PORT_CAP_USB4);
+ if (cap > 0)
+ port->cap_usb4 = cap;
} else if (port->port != 0) {
cap = tb_port_find_cap(port, TB_PORT_CAP_ADAP);
if (cap > 0)
@@ -1069,20 +1177,38 @@ int tb_dp_port_enable(struct tb_port *port, bool enable)

/* switch utility functions */

-static void tb_dump_switch(struct tb *tb, struct tb_regs_switch_header *sw)
+static const char *tb_switch_generation_name(const struct tb_switch *sw)
+{
+ switch (sw->generation) {
+ case 1:
+ return "Thunderbolt 1";
+ case 2:
+ return "Thunderbolt 2";
+ case 3:
+ return "Thunderbolt 3";
+ case 4:
+ return "USB4";
+ default:
+ return "Unknown";
+ }
+}
+
+static void tb_dump_switch(const struct tb *tb, const struct tb_switch *sw)
{
- tb_dbg(tb, " Switch: %x:%x (Revision: %d, TB Version: %d)\n",
- sw->vendor_id, sw->device_id, sw->revision,
- sw->thunderbolt_version);
- tb_dbg(tb, " Max Port Number: %d\n", sw->max_port_number);
+ const struct tb_regs_switch_header *regs = &sw->config;
+
+ tb_dbg(tb, " %s Switch: %x:%x (Revision: %d, TB Version: %d)\n",
+ tb_switch_generation_name(sw), regs->vendor_id, regs->device_id,
+ regs->revision, regs->thunderbolt_version);
+ tb_dbg(tb, " Max Port Number: %d\n", regs->max_port_number);
tb_dbg(tb, " Config:\n");
tb_dbg(tb,
" Upstream Port Number: %d Depth: %d Route String: %#llx Enabled: %d, PlugEventsDelay: %dms\n",
- sw->upstream_port_number, sw->depth,
- (((u64) sw->route_hi) << 32) | sw->route_lo,
- sw->enabled, sw->plug_events_delay);
+ regs->upstream_port_number, regs->depth,
+ (((u64) regs->route_hi) << 32) | regs->route_lo,
+ regs->enabled, regs->plug_events_delay);
tb_dbg(tb, " unknown1: %#x unknown4: %#x\n",
- sw->__unknown1, sw->__unknown4);
+ regs->__unknown1, regs->__unknown4);
}

/**
@@ -1129,6 +1255,10 @@ static int tb_plug_events_active(struct tb_switch *sw, bool active)
if (res)
return res;

+ /* Plug events are always enabled in USB4 */
+ if (tb_switch_is_usb4(sw))
+ return 0;
+
res = tb_sw_read(sw, &data, TB_CFG_SWITCH, sw->cap_plug_events + 1, 1);
if (res)
return res;
@@ -1321,30 +1451,6 @@ static ssize_t link_width_show(struct device *dev,
}
static DEVICE_ATTR_RO(link_width);

-static void nvm_authenticate_start(struct tb_switch *sw)
-{
- struct pci_dev *root_port;
-
- /*
- * During host router NVM upgrade we should not allow root port to
- * go into D3cold because some root ports cannot trigger PME
- * itself. To be on the safe side keep the root port in D0 during
- * the whole upgrade process.
- */
- root_port = pci_find_pcie_root_port(sw->tb->nhi->pdev);
- if (root_port)
- pm_runtime_get_noresume(&root_port->dev);
-}
-
-static void nvm_authenticate_complete(struct tb_switch *sw)
-{
- struct pci_dev *root_port;
-
- root_port = pci_find_pcie_root_port(sw->tb->nhi->pdev);
- if (root_port)
- pm_runtime_put(&root_port->dev);
-}
-
static ssize_t nvm_authenticate_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
@@ -1393,19 +1499,7 @@ static ssize_t nvm_authenticate_store(struct device *dev,
goto exit_unlock;

sw->nvm->authenticating = true;
-
- if (!tb_route(sw)) {
- /*
- * Keep root port from suspending as long as the
- * NVM upgrade process is running.
- */
- nvm_authenticate_start(sw);
- ret = nvm_authenticate_host(sw);
- if (ret)
- nvm_authenticate_complete(sw);
- } else {
- ret = nvm_authenticate_device(sw);
- }
+ ret = nvm_authenticate(sw);
}

exit_unlock:
@@ -1515,11 +1609,11 @@ static umode_t switch_attr_is_visible(struct kobject *kobj,
return attr->mode;
return 0;
} else if (attr == &dev_attr_nvm_authenticate.attr) {
- if (sw->dma_port && !sw->no_nvm_upgrade)
+ if (nvm_upgradeable(sw))
return attr->mode;
return 0;
} else if (attr == &dev_attr_nvm_version.attr) {
- if (sw->dma_port)
+ if (nvm_readable(sw))
return attr->mode;
return 0;
} else if (attr == &dev_attr_boot.attr) {
@@ -1631,6 +1725,9 @@ static int tb_switch_get_generation(struct tb_switch *sw)
return 3;

default:
+ if (tb_switch_is_usb4(sw))
+ return 4;
+
/*
* For unknown switches assume generation to be 1 to be
* on the safe side.
@@ -1641,6 +1738,19 @@ static int tb_switch_get_generation(struct tb_switch *sw)
}
}

+static bool tb_switch_exceeds_max_depth(const struct tb_switch *sw, int depth)
+{
+ int max_depth;
+
+ if (tb_switch_is_usb4(sw) ||
+ (sw->tb->root_switch && tb_switch_is_usb4(sw->tb->root_switch)))
+ max_depth = USB4_SWITCH_MAX_DEPTH;
+ else
+ max_depth = TB_SWITCH_MAX_DEPTH;
+
+ return depth > max_depth;
+}
+
/**
* tb_switch_alloc() - allocate a switch
* @tb: Pointer to the owning domain
@@ -1662,10 +1772,16 @@ struct tb_switch *tb_switch_alloc(struct tb *tb, struct device *parent,
int upstream_port;
int i, ret, depth;

- /* Make sure we do not exceed maximum topology limit */
+ /* Unlock the downstream port so we can access the switch below */
+ if (route) {
+ struct tb_switch *parent_sw = tb_to_switch(parent);
+ struct tb_port *down;
+
+ down = tb_port_at(route, parent_sw);
+ tb_port_unlock(down);
+ }
+
depth = tb_route_length(route);
- if (depth > TB_SWITCH_MAX_DEPTH)
- return ERR_PTR(-EADDRNOTAVAIL);

upstream_port = tb_cfg_get_upstream_port(tb->ctl, route);
if (upstream_port < 0)
@@ -1680,8 +1796,10 @@ struct tb_switch *tb_switch_alloc(struct tb *tb, struct device *parent,
if (ret)
goto err_free_sw_ports;

+ sw->generation = tb_switch_get_generation(sw);
+
tb_dbg(tb, "current switch config:\n");
- tb_dump_switch(tb, &sw->config);
+ tb_dump_switch(tb, sw);

/* configure switch */
sw->config.upstream_port_number = upstream_port;
@@ -1690,6 +1808,10 @@ struct tb_switch *tb_switch_alloc(struct tb *tb, struct device *parent,
sw->config.route_lo = lower_32_bits(route);
sw->config.enabled = 0;

+ /* Make sure we do not exceed maximum topology limit */
+ if (tb_switch_exceeds_max_depth(sw, depth))
+ return ERR_PTR(-EADDRNOTAVAIL);
+
/* initialize ports */
sw->ports = kcalloc(sw->config.max_port_number + 1, sizeof(*sw->ports),
GFP_KERNEL);
@@ -1704,14 +1826,9 @@ struct tb_switch *tb_switch_alloc(struct tb *tb, struct device *parent,
sw->ports[i].port = i;
}

- sw->generation = tb_switch_get_generation(sw);
-
ret = tb_switch_find_vse_cap(sw, TB_VSE_CAP_PLUG_EVENTS);
- if (ret < 0) {
- tb_sw_warn(sw, "cannot find TB_VSE_CAP_PLUG_EVENTS aborting\n");
- goto err_free_sw_ports;
- }
- sw->cap_plug_events = ret;
+ if (ret > 0)
+ sw->cap_plug_events = ret;

ret = tb_switch_find_vse_cap(sw, TB_VSE_CAP_LINK_CONTROLLER);
if (ret > 0)
@@ -1782,7 +1899,8 @@ tb_switch_alloc_safe_mode(struct tb *tb, struct device *parent, u64 route)
*
* Call this function before the switch is added to the system. It will
* upload configuration to the switch and makes it available for the
- * connection manager to use.
+ * connection manager to use. Can be called to the switch again after
+ * resume from low power states to re-initialize it.
*
* Return: %0 in case of success and negative errno in case of failure
*/
@@ -1793,21 +1911,50 @@ int tb_switch_configure(struct tb_switch *sw)
int ret;

route = tb_route(sw);
- tb_dbg(tb, "initializing Switch at %#llx (depth: %d, up port: %d)\n",
- route, tb_route_length(route), sw->config.upstream_port_number);

- if (sw->config.vendor_id != PCI_VENDOR_ID_INTEL)
- tb_sw_warn(sw, "unknown switch vendor id %#x\n",
- sw->config.vendor_id);
+ tb_dbg(tb, "%s Switch at %#llx (depth: %d, up port: %d)\n",
+ sw->config.enabled ? "restoring " : "initializing", route,
+ tb_route_length(route), sw->config.upstream_port_number);

sw->config.enabled = 1;

- /* upload configuration */
- ret = tb_sw_write(sw, 1 + (u32 *)&sw->config, TB_CFG_SWITCH, 1, 3);
- if (ret)
- return ret;
+ if (tb_switch_is_usb4(sw)) {
+ /*
+ * For USB4 devices, we need to program the CM version
+ * accordingly so that it knows to expose all the
+ * additional capabilities.
+ */
+ sw->config.cmuv = USB4_VERSION_1_0;
+
+ /* Enumerate the switch */
+ ret = tb_sw_write(sw, (u32 *)&sw->config + 1, TB_CFG_SWITCH,
+ ROUTER_CS_1, 4);
+ if (ret)
+ return ret;
+
+ ret = usb4_switch_setup(sw);
+ if (ret)
+ return ret;
+
+ ret = usb4_switch_configure_link(sw);
+ } else {
+ if (sw->config.vendor_id != PCI_VENDOR_ID_INTEL)
+ tb_sw_warn(sw, "unknown switch vendor id %#x\n",
+ sw->config.vendor_id);
+
+ if (!sw->cap_plug_events) {
+ tb_sw_warn(sw, "cannot find TB_VSE_CAP_PLUG_EVENTS aborting\n");
+ return -ENODEV;
+ }
+
+ /* Enumerate the switch */
+ ret = tb_sw_write(sw, (u32 *)&sw->config + 1, TB_CFG_SWITCH,
+ ROUTER_CS_1, 3);
+ if (ret)
+ return ret;

- ret = tb_lc_configure_link(sw);
+ ret = tb_lc_configure_link(sw);
+ }
if (ret)
return ret;

@@ -1816,18 +1963,32 @@ int tb_switch_configure(struct tb_switch *sw)

static int tb_switch_set_uuid(struct tb_switch *sw)
{
+ bool uid = false;
u32 uuid[4];
int ret;

if (sw->uuid)
return 0;

- /*
- * The newer controllers include fused UUID as part of link
- * controller specific registers
- */
- ret = tb_lc_read_uuid(sw, uuid);
- if (ret) {
+ if (tb_switch_is_usb4(sw)) {
+ ret = usb4_switch_read_uid(sw, &sw->uid);
+ if (ret)
+ return ret;
+ uid = true;
+ } else {
+ /*
+ * The newer controllers include fused UUID as part of
+ * link controller specific registers
+ */
+ ret = tb_lc_read_uuid(sw, uuid);
+ if (ret) {
+ if (ret != -EINVAL)
+ return ret;
+ uid = true;
+ }
+ }
+
+ if (uid) {
/*
* ICM generates UUID based on UID and fills the upper
* two words with ones. This is not strictly following
@@ -1893,7 +2054,7 @@ static int tb_switch_add_dma_port(struct tb_switch *sw)

/* Now we can allow root port to suspend again */
if (!tb_route(sw))
- nvm_authenticate_complete(sw);
+ nvm_authenticate_complete_dma_port(sw);

if (status) {
tb_sw_info(sw, "switch flash authentication failed\n");
@@ -1950,6 +2111,8 @@ static bool tb_switch_lane_bonding_possible(struct tb_switch *sw)
if (!up->dual_link_port || !up->dual_link_port->remote)
return false;

+ if (tb_switch_is_usb4(sw))
+ return usb4_switch_lane_bonding_possible(sw);
return tb_lc_lane_bonding_possible(sw);
}

@@ -2177,7 +2340,11 @@ void tb_switch_remove(struct tb_switch *sw)

if (!sw->is_unplugged)
tb_plug_events_active(sw, false);
- tb_lc_unconfigure_link(sw);
+
+ if (tb_switch_is_usb4(sw))
+ usb4_switch_unconfigure_link(sw);
+ else
+ tb_lc_unconfigure_link(sw);

tb_switch_nvm_remove(sw);

@@ -2232,7 +2399,10 @@ int tb_switch_resume(struct tb_switch *sw)
return err;
}

- err = tb_drom_read_uid_only(sw, &uid);
+ if (tb_switch_is_usb4(sw))
+ err = usb4_switch_read_uid(sw, &uid);
+ else
+ err = tb_drom_read_uid_only(sw, &uid);
if (err) {
tb_sw_warn(sw, "uid read failed\n");
return err;
@@ -2245,16 +2415,7 @@ int tb_switch_resume(struct tb_switch *sw)
}
}

- /* upload configuration */
- err = tb_sw_write(sw, 1 + (u32 *) &sw->config, TB_CFG_SWITCH, 1, 3);
- if (err)
- return err;
-
- err = tb_lc_configure_link(sw);
- if (err)
- return err;
-
- err = tb_plug_events_active(sw, true);
+ err = tb_switch_configure(sw);
if (err)
return err;

@@ -2269,8 +2430,14 @@ int tb_switch_resume(struct tb_switch *sw)
tb_sw_set_unplugged(port->remote->sw);
else if (port->xdomain)
port->xdomain->is_unplugged = true;
- } else if (tb_port_has_remote(port)) {
- if (tb_switch_resume(port->remote->sw)) {
+ } else if (tb_port_has_remote(port) || port->xdomain) {
+ /*
+ * Always unlock the port so the downstream
+ * switch/domain is accessible.
+ */
+ if (tb_port_unlock(port))
+ tb_port_warn(port, "failed to unlock port\n");
+ if (port->remote && tb_switch_resume(port->remote->sw)) {
tb_port_warn(port,
"lost during suspend, disconnecting\n");
tb_sw_set_unplugged(port->remote->sw);
@@ -2290,7 +2457,10 @@ void tb_switch_suspend(struct tb_switch *sw)
tb_switch_for_each_remote_port(sw, i)
tb_switch_suspend(sw->ports[i].remote->sw);

- tb_lc_set_sleep(sw);
+ if (tb_switch_is_usb4(sw))
+ usb4_switch_set_sleep(sw);
+ else
+ tb_lc_set_sleep(sw);
}

/**
@@ -2303,6 +2473,8 @@ void tb_switch_suspend(struct tb_switch *sw)
*/
bool tb_switch_query_dp_resource(struct tb_switch *sw, struct tb_port *in)
{
+ if (tb_switch_is_usb4(sw))
+ return usb4_switch_query_dp_resource(sw, in);
return tb_lc_dp_sink_query(sw, in);
}

@@ -2317,6 +2489,8 @@ bool tb_switch_query_dp_resource(struct tb_switch *sw, struct tb_port *in)
*/
int tb_switch_alloc_dp_resource(struct tb_switch *sw, struct tb_port *in)
{
+ if (tb_switch_is_usb4(sw))
+ return usb4_switch_alloc_dp_resource(sw, in);
return tb_lc_dp_sink_alloc(sw, in);
}

@@ -2330,10 +2504,16 @@ int tb_switch_alloc_dp_resource(struct tb_switch *sw, struct tb_port *in)
*/
void tb_switch_dealloc_dp_resource(struct tb_switch *sw, struct tb_port *in)
{
- if (tb_lc_dp_sink_dealloc(sw, in)) {
+ int ret;
+
+ if (tb_switch_is_usb4(sw))
+ ret = usb4_switch_dealloc_dp_resource(sw, in);
+ else
+ ret = tb_lc_dp_sink_dealloc(sw, in);
+
+ if (ret)
tb_sw_warn(sw, "failed to de-allocate DP resource for port %d\n",
in->port);
- }
}

struct tb_sw_lookup {
diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
index eab93e32cc4f..24e37e47dc48 100644
--- a/drivers/thunderbolt/tb.c
+++ b/drivers/thunderbolt/tb.c
@@ -370,12 +370,15 @@ static struct tb_port *tb_find_unused_port(struct tb_switch *sw,
static struct tb_port *tb_find_pcie_down(struct tb_switch *sw,
const struct tb_port *port)
{
+ struct tb_port *down = NULL;
+
/*
* To keep plugging devices consistently in the same PCIe
- * hierarchy, do mapping here for root switch downstream PCIe
- * ports.
+ * hierarchy, do mapping here for switch downstream PCIe ports.
*/
- if (!tb_route(sw)) {
+ if (tb_switch_is_usb4(sw)) {
+ down = usb4_switch_map_pcie_down(sw, port);
+ } else if (!tb_route(sw)) {
int phy_port = tb_phy_port_from_link(port->port);
int index;

@@ -395,12 +398,17 @@ static struct tb_port *tb_find_pcie_down(struct tb_switch *sw,
/* Validate the hard-coding */
if (WARN_ON(index > sw->config.max_port_number))
goto out;
- if (WARN_ON(!tb_port_is_pcie_down(&sw->ports[index])))
+
+ down = &sw->ports[index];
+ }
+
+ if (down) {
+ if (WARN_ON(!tb_port_is_pcie_down(down)))
goto out;
- if (WARN_ON(tb_pci_port_is_enabled(&sw->ports[index])))
+ if (WARN_ON(tb_pci_port_is_enabled(down)))
goto out;

- return &sw->ports[index];
+ return down;
}

out:
diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index f4ce739bfd7a..60f237020d1b 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -44,6 +44,7 @@ struct tb_switch_nvm {

#define TB_SWITCH_KEY_SIZE 32
#define TB_SWITCH_MAX_DEPTH 6
+#define USB4_SWITCH_MAX_DEPTH 5

/**
* struct tb_switch - a thunderbolt switch
@@ -129,6 +130,7 @@ struct tb_switch {
* @xdomain: Remote host (%NULL if not connected)
* @cap_phy: Offset, zero if not found
* @cap_adap: Offset of the adapter specific capability (%0 if not present)
+ * @cap_usb4: Offset to the USB4 port capability (%0 if not present)
* @port: Port number on switch
* @disabled: Disabled by eeprom
* @bonded: true if the port is bonded (two lanes combined as one)
@@ -146,6 +148,7 @@ struct tb_port {
struct tb_xdomain *xdomain;
int cap_phy;
int cap_adap;
+ int cap_usb4;
u8 port;
bool disabled;
bool bonded;
@@ -661,6 +664,17 @@ static inline bool tb_switch_is_tr(const struct tb_switch *sw)
}
}

+/**
+ * tb_switch_is_usb4() - Is the switch USB4 compliant
+ * @sw: Switch to check
+ *
+ * Returns true if the @sw is USB4 compliant router, false otherwise.
+ */
+static inline bool tb_switch_is_usb4(const struct tb_switch *sw)
+{
+ return sw->config.thunderbolt_version == USB4_VERSION_1_0;
+}
+
/**
* tb_switch_is_icm() - Is the switch handled by ICM firmware
* @sw: Switch to check
@@ -686,6 +700,7 @@ int tb_wait_for_port(struct tb_port *port, bool wait_if_unplugged);
int tb_port_add_nfc_credits(struct tb_port *port, int credits);
int tb_port_set_initial_credits(struct tb_port *port, u32 credits);
int tb_port_clear_counter(struct tb_port *port, int counter);
+int tb_port_unlock(struct tb_port *port);
int tb_port_alloc_in_hopid(struct tb_port *port, int hopid, int max_hopid);
void tb_port_release_in_hopid(struct tb_port *port, int hopid);
int tb_port_alloc_out_hopid(struct tb_port *port, int hopid, int max_hopid);
@@ -760,4 +775,25 @@ void tb_xdomain_remove(struct tb_xdomain *xd);
struct tb_xdomain *tb_xdomain_find_by_link_depth(struct tb *tb, u8 link,
u8 depth);

+int usb4_switch_setup(struct tb_switch *sw);
+int usb4_switch_read_uid(struct tb_switch *sw, u64 *uid);
+int usb4_switch_drom_read(struct tb_switch *sw, unsigned int address, void *buf,
+ size_t size);
+int usb4_switch_configure_link(struct tb_switch *sw);
+void usb4_switch_unconfigure_link(struct tb_switch *sw);
+bool usb4_switch_lane_bonding_possible(struct tb_switch *sw);
+int usb4_switch_set_sleep(struct tb_switch *sw);
+int usb4_switch_nvm_sector_size(struct tb_switch *sw);
+int usb4_switch_nvm_read(struct tb_switch *sw, unsigned int address, void *buf,
+ size_t size);
+int usb4_switch_nvm_write(struct tb_switch *sw, unsigned int address,
+ const void *buf, size_t size);
+int usb4_switch_nvm_authenticate(struct tb_switch *sw);
+bool usb4_switch_query_dp_resource(struct tb_switch *sw, struct tb_port *in);
+int usb4_switch_alloc_dp_resource(struct tb_switch *sw, struct tb_port *in);
+int usb4_switch_dealloc_dp_resource(struct tb_switch *sw, struct tb_port *in);
+struct tb_port *usb4_switch_map_pcie_down(struct tb_switch *sw,
+ const struct tb_port *port);
+
+int usb4_port_unlock(struct tb_port *port);
#endif
diff --git a/drivers/thunderbolt/tb_regs.h b/drivers/thunderbolt/tb_regs.h
index 7ee45b73c7f7..47f73f992412 100644
--- a/drivers/thunderbolt/tb_regs.h
+++ b/drivers/thunderbolt/tb_regs.h
@@ -41,6 +41,7 @@ enum tb_port_cap {
TB_PORT_CAP_TIME1 = 0x03,
TB_PORT_CAP_ADAP = 0x04,
TB_PORT_CAP_VSE = 0x05,
+ TB_PORT_CAP_USB4 = 0x06,
};

enum tb_port_state {
@@ -164,10 +165,36 @@ struct tb_regs_switch_header {
* milliseconds. Writing 0x00 is interpreted
* as 255ms.
*/
- u32 __unknown4:16;
+ u32 cmuv:8;
+ u32 __unknown4:8;
u32 thunderbolt_version:8;
} __packed;

+/* USB4 version 1.0 */
+#define USB4_VERSION_1_0 0x20
+
+#define ROUTER_CS_1 0x01
+#define ROUTER_CS_4 0x04
+#define ROUTER_CS_5 0x05
+#define ROUTER_CS_5_SLP BIT(0)
+#define ROUTER_CS_5_C3S BIT(23)
+#define ROUTER_CS_5_PTO BIT(24)
+#define ROUTER_CS_5_HCO BIT(26)
+#define ROUTER_CS_5_CV BIT(31)
+#define ROUTER_CS_6 0x06
+#define ROUTER_CS_6_SLPR BIT(0)
+#define ROUTER_CS_6_TNS BIT(1)
+#define ROUTER_CS_6_HCI BIT(18)
+#define ROUTER_CS_6_CR BIT(25)
+#define ROUTER_CS_7 0x07
+#define ROUTER_CS_9 0x09
+#define ROUTER_CS_25 0x19
+#define ROUTER_CS_26 0x1a
+#define ROUTER_CS_26_STATUS_MASK GENMASK(29, 24)
+#define ROUTER_CS_26_STATUS_SHIFT 24
+#define ROUTER_CS_26_ONS BIT(30)
+#define ROUTER_CS_26_OV BIT(31)
+
enum tb_port_type {
TB_TYPE_INACTIVE = 0x000000,
TB_TYPE_PORT = 0x000001,
@@ -216,6 +243,7 @@ struct tb_regs_port_header {
#define ADP_CS_4_NFC_BUFFERS_MASK GENMASK(9, 0)
#define ADP_CS_4_TOTAL_BUFFERS_MASK GENMASK(29, 20)
#define ADP_CS_4_TOTAL_BUFFERS_SHIFT 20
+#define ADP_CS_4_LCK BIT(31)
#define ADP_CS_5 0x05
#define ADP_CS_5_LCA_MASK GENMASK(28, 22)
#define ADP_CS_5_LCA_SHIFT 22
@@ -237,6 +265,12 @@ struct tb_regs_port_header {
#define LANE_ADP_CS_1_CURRENT_WIDTH_MASK GENMASK(25, 20)
#define LANE_ADP_CS_1_CURRENT_WIDTH_SHIFT 20

+/* USB4 port registers */
+#define PORT_CS_18 0x12
+#define PORT_CS_18_BE BIT(8)
+#define PORT_CS_19 0x13
+#define PORT_CS_19_PC BIT(3)
+
/* Display Port adapter registers */
#define ADP_DP_CS_0 0x00
#define ADP_DP_CS_0_VIDEO_HOPID_MASK GENMASK(26, 16)
diff --git a/drivers/thunderbolt/tunnel.c b/drivers/thunderbolt/tunnel.c
index 4ef5bc8f912b..e8e387c4211f 100644
--- a/drivers/thunderbolt/tunnel.c
+++ b/drivers/thunderbolt/tunnel.c
@@ -243,6 +243,12 @@ struct tb_tunnel *tb_tunnel_alloc_pci(struct tb *tb, struct tb_port *up,
return tunnel;
}

+static bool tb_dp_is_usb4(const struct tb_switch *sw)
+{
+ /* Titan Ridge DP adapters need the same treatment as USB4 */
+ return tb_switch_is_usb4(sw) || tb_switch_is_tr(sw);
+}
+
static int tb_dp_cm_handshake(struct tb_port *in, struct tb_port *out)
{
int timeout = 10;
@@ -250,7 +256,7 @@ static int tb_dp_cm_handshake(struct tb_port *in, struct tb_port *out)
int ret;

/* Both ends need to support this */
- if (!tb_switch_is_tr(in->sw) || !tb_switch_is_tr(out->sw))
+ if (!tb_dp_is_usb4(in->sw) || !tb_dp_is_usb4(out->sw))
return 0;

ret = tb_port_read(out, &val, TB_CFG_PORT,
@@ -520,7 +526,7 @@ static int tb_dp_consumed_bandwidth(struct tb_tunnel *tunnel)
u32 val, rate = 0, lanes = 0;
int ret;

- if (tb_switch_is_tr(sw)) {
+ if (tb_dp_is_usb4(sw)) {
int timeout = 10;

/*
diff --git a/drivers/thunderbolt/usb4.c b/drivers/thunderbolt/usb4.c
new file mode 100644
index 000000000000..4b0997292b43
--- /dev/null
+++ b/drivers/thunderbolt/usb4.c
@@ -0,0 +1,722 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * USB4 specific functionality
+ *
+ * Copyright (C) 2019, Intel Corporation
+ * Authors: Mika Westerberg <[email protected]>
+ * Rajmohan Mani <[email protected]>
+ */
+
+#include <linux/delay.h>
+#include <linux/ktime.h>
+
+#include "tb.h"
+
+#define USB4_DATA_DWORDS 16
+#define USB4_DATA_RETRIES 3
+
+enum usb4_switch_op {
+ USB4_SWITCH_OP_QUERY_DP_RESOURCE = 0x10,
+ USB4_SWITCH_OP_ALLOC_DP_RESOURCE = 0x11,
+ USB4_SWITCH_OP_DEALLOC_DP_RESOURCE = 0x12,
+ USB4_SWITCH_OP_NVM_WRITE = 0x20,
+ USB4_SWITCH_OP_NVM_AUTH = 0x21,
+ USB4_SWITCH_OP_NVM_READ = 0x22,
+ USB4_SWITCH_OP_NVM_SET_OFFSET = 0x23,
+ USB4_SWITCH_OP_DROM_READ = 0x24,
+ USB4_SWITCH_OP_NVM_SECTOR_SIZE = 0x25,
+};
+
+#define USB4_NVM_READ_OFFSET_MASK GENMASK(23, 2)
+#define USB4_NVM_READ_OFFSET_SHIFT 2
+#define USB4_NVM_READ_LENGTH_MASK GENMASK(27, 24)
+#define USB4_NVM_READ_LENGTH_SHIFT 24
+
+#define USB4_NVM_SET_OFFSET_MASK USB4_NVM_READ_OFFSET_MASK
+#define USB4_NVM_SET_OFFSET_SHIFT USB4_NVM_READ_OFFSET_SHIFT
+
+#define USB4_DROM_ADDRESS_MASK GENMASK(14, 2)
+#define USB4_DROM_ADDRESS_SHIFT 2
+#define USB4_DROM_SIZE_MASK GENMASK(19, 15)
+#define USB4_DROM_SIZE_SHIFT 15
+
+#define USB4_NVM_SECTOR_SIZE_MASK GENMASK(23, 0)
+
+typedef int (*read_block_fn)(struct tb_switch *, unsigned int, void *, size_t);
+typedef int (*write_block_fn)(struct tb_switch *, const void *, size_t);
+
+static int usb4_switch_wait_for_bit(struct tb_switch *sw, u32 offset, u32 bit,
+ u32 value, int timeout_msec)
+{
+ ktime_t timeout = ktime_add_ms(ktime_get(), timeout_msec);
+
+ do {
+ u32 val;
+ int ret;
+
+ ret = tb_sw_read(sw, &val, TB_CFG_SWITCH, offset, 1);
+ if (ret)
+ return ret;
+
+ if ((val & bit) == value)
+ return 0;
+
+ usleep_range(50, 100);
+ } while (ktime_before(ktime_get(), timeout));
+
+ return -ETIMEDOUT;
+}
+
+static int usb4_switch_op_read_data(struct tb_switch *sw, void *data,
+ size_t dwords)
+{
+ if (dwords > USB4_DATA_DWORDS)
+ return -EINVAL;
+
+ return tb_sw_read(sw, data, TB_CFG_SWITCH, ROUTER_CS_9, dwords);
+}
+
+static int usb4_switch_op_write_data(struct tb_switch *sw, const void *data,
+ size_t dwords)
+{
+ if (dwords > USB4_DATA_DWORDS)
+ return -EINVAL;
+
+ return tb_sw_write(sw, data, TB_CFG_SWITCH, ROUTER_CS_9, dwords);
+}
+
+static int usb4_switch_op_read_metadata(struct tb_switch *sw, u32 *metadata)
+{
+ return tb_sw_read(sw, metadata, TB_CFG_SWITCH, ROUTER_CS_25, 1);
+}
+
+static int usb4_switch_op_write_metadata(struct tb_switch *sw, u32 metadata)
+{
+ return tb_sw_write(sw, &metadata, TB_CFG_SWITCH, ROUTER_CS_25, 1);
+}
+
+static int usb4_switch_do_read_data(struct tb_switch *sw, u16 address,
+ void *buf, size_t size, read_block_fn read_block)
+{
+ unsigned int retries = USB4_DATA_RETRIES;
+ unsigned int offset;
+
+ offset = address & 3;
+ address = address & ~3;
+
+ do {
+ size_t nbytes = min_t(size_t, size, USB4_DATA_DWORDS * 4);
+ unsigned int dwaddress, dwords;
+ u8 data[USB4_DATA_DWORDS * 4];
+ int ret;
+
+ dwaddress = address / 4;
+ dwords = ALIGN(nbytes, 4) / 4;
+
+ ret = read_block(sw, dwaddress, data, dwords);
+ if (ret) {
+ if (ret == -ETIMEDOUT) {
+ if (retries--)
+ continue;
+ ret = -EIO;
+ }
+ return ret;
+ }
+
+ memcpy(buf, data + offset, nbytes);
+
+ size -= nbytes;
+ address += nbytes;
+ buf += nbytes;
+ } while (size > 0);
+
+ return 0;
+}
+
+static int usb4_switch_do_write_data(struct tb_switch *sw, u16 address,
+ const void *buf, size_t size, write_block_fn write_next_block)
+{
+ unsigned int retries = USB4_DATA_RETRIES;
+ unsigned int offset;
+
+ offset = address & 3;
+ address = address & ~3;
+
+ do {
+ u32 nbytes = min_t(u32, size, USB4_DATA_DWORDS * 4);
+ u8 data[USB4_DATA_DWORDS * 4];
+ int ret;
+
+ memcpy(data + offset, buf, nbytes);
+
+ ret = write_next_block(sw, data, nbytes / 4);
+ if (ret) {
+ if (ret == -ETIMEDOUT) {
+ if (retries--)
+ continue;
+ ret = -EIO;
+ }
+ return ret;
+ }
+
+ size -= nbytes;
+ address += nbytes;
+ buf += nbytes;
+ } while (size > 0);
+
+ return 0;
+}
+
+static int usb4_switch_op(struct tb_switch *sw, u16 opcode, u8 *status)
+{
+ u32 val;
+ int ret;
+
+ val = opcode | ROUTER_CS_26_OV;
+ ret = tb_sw_write(sw, &val, TB_CFG_SWITCH, ROUTER_CS_26, 1);
+ if (ret)
+ return ret;
+
+ ret = usb4_switch_wait_for_bit(sw, ROUTER_CS_26, ROUTER_CS_26_OV, 0, 500);
+ if (ret)
+ return ret;
+
+ ret = tb_sw_read(sw, &val, TB_CFG_SWITCH, ROUTER_CS_26, 1);
+ if (val & ROUTER_CS_26_ONS)
+ return -EOPNOTSUPP;
+
+ *status = (val & ROUTER_CS_26_STATUS_MASK) >> ROUTER_CS_26_STATUS_SHIFT;
+ return 0;
+}
+
+/**
+ * usb4_switch_setup() - Additional setup for USB4 device
+ * @sw: USB4 router to setup
+ *
+ * USB4 routers need additional settings in order to enable all the
+ * tunneling. This function enables USB and PCIe tunneling if it can be
+ * enabled (e.g the parent switch also supports them). If USB tunneling
+ * is not available for some reason (like that there is Thunderbolt 3
+ * switch upstream) then the internal xHCI controller is enabled
+ * instead.
+ */
+int usb4_switch_setup(struct tb_switch *sw)
+{
+ struct tb_switch *parent;
+ bool tbt3, xhci;
+ u32 val = 0;
+ int ret;
+
+ if (!tb_route(sw))
+ return 0;
+
+ ret = tb_sw_read(sw, &val, TB_CFG_SWITCH, ROUTER_CS_6, 1);
+ if (ret)
+ return ret;
+
+ xhci = val & ROUTER_CS_6_HCI;
+ tbt3 = !(val & ROUTER_CS_6_TNS);
+
+ tb_sw_dbg(sw, "TBT3 support: %s, xHCI: %s\n",
+ tbt3 ? "yes" : "no", xhci ? "yes" : "no");
+
+ ret = tb_sw_read(sw, &val, TB_CFG_SWITCH, ROUTER_CS_5, 1);
+ if (ret)
+ return ret;
+
+ parent = tb_switch_parent(sw);
+
+ /* Only enable PCIe tunneling if the parent router supports it */
+ if (tb_switch_find_port(parent, TB_TYPE_PCIE_DOWN)) {
+ val |= ROUTER_CS_5_PTO;
+ /* xHCI can be enabled if PCIe tunneling is supported */
+ if (xhci & ROUTER_CS_6_HCI)
+ val |= ROUTER_CS_5_HCO;
+ }
+
+ /* TBT3 supported by the CM */
+ val |= ROUTER_CS_5_C3S;
+ /* Tunneling configuration is ready now */
+ val |= ROUTER_CS_5_CV;
+
+ ret = tb_sw_write(sw, &val, TB_CFG_SWITCH, ROUTER_CS_5, 1);
+ if (ret)
+ return ret;
+
+ return usb4_switch_wait_for_bit(sw, ROUTER_CS_6, ROUTER_CS_6_CR,
+ ROUTER_CS_6_CR, 50);
+}
+
+/**
+ * usb4_switch_read_uid() - Read UID from USB4 router
+ * @sw: USB4 router
+ *
+ * Reads 64-bit UID from USB4 router config space.
+ */
+int usb4_switch_read_uid(struct tb_switch *sw, u64 *uid)
+{
+ return tb_sw_read(sw, uid, TB_CFG_SWITCH, ROUTER_CS_7, 2);
+}
+
+static int usb4_switch_drom_read_block(struct tb_switch *sw,
+ unsigned int dwaddress, void *buf,
+ size_t dwords)
+{
+ u8 status = 0;
+ u32 metadata;
+ int ret;
+
+ metadata = (dwords << USB4_DROM_SIZE_SHIFT) & USB4_DROM_SIZE_MASK;
+ metadata |= (dwaddress << USB4_DROM_ADDRESS_SHIFT) &
+ USB4_DROM_ADDRESS_MASK;
+
+ ret = usb4_switch_op_write_metadata(sw, metadata);
+ if (ret)
+ return ret;
+
+ ret = usb4_switch_op(sw, USB4_SWITCH_OP_DROM_READ, &status);
+ if (ret)
+ return ret;
+
+ if (status)
+ return -EIO;
+
+ return usb4_switch_op_read_data(sw, buf, dwords);
+}
+
+/**
+ * usb4_switch_drom_read() - Read arbitrary bytes from USB4 router DROM
+ * @sw: USB4 router
+ *
+ * Uses USB4 router operations to read router DROM. For devices this
+ * should always work but for hosts it may return %-EOPNOTSUPP in which
+ * case the host router does not have DROM.
+ */
+int usb4_switch_drom_read(struct tb_switch *sw, unsigned int address, void *buf,
+ size_t size)
+{
+ return usb4_switch_do_read_data(sw, address, buf, size,
+ usb4_switch_drom_read_block);
+}
+
+static int usb4_set_port_configured(struct tb_port *port, bool configured)
+{
+ int ret;
+ u32 val;
+
+ ret = tb_port_read(port, &val, TB_CFG_PORT,
+ port->cap_usb4 + PORT_CS_19, 1);
+ if (ret)
+ return ret;
+
+ if (configured)
+ val |= PORT_CS_19_PC;
+ else
+ val &= ~PORT_CS_19_PC;
+
+ return tb_port_write(port, &val, TB_CFG_PORT,
+ port->cap_usb4 + PORT_CS_19, 1);
+}
+
+/**
+ * usb4_switch_configure_link() - Set upstream USB4 link configured
+ * @sw: USB4 router
+ *
+ * Sets the upstream USB4 link to be configured for power management
+ * purposes.
+ */
+int usb4_switch_configure_link(struct tb_switch *sw)
+{
+ struct tb_port *up;
+
+ if (!tb_route(sw))
+ return 0;
+
+ up = tb_upstream_port(sw);
+ return usb4_set_port_configured(up, true);
+}
+
+/**
+ * usb4_switch_unconfigure_link() - Un-set upstream USB4 link configuration
+ * @sw: USB4 router
+ *
+ * Reverse of usb4_switch_configure_link().
+ */
+void usb4_switch_unconfigure_link(struct tb_switch *sw)
+{
+ struct tb_port *up;
+
+ if (sw->is_unplugged || !tb_route(sw))
+ return;
+
+ up = tb_upstream_port(sw);
+ usb4_set_port_configured(up, false);
+}
+
+/**
+ * usb4_switch_lane_bonding_possible() - Are conditions met for lane bonding
+ * @sw: USB4 router
+ *
+ * Checks whether conditions are met so that lane bonding can be
+ * established with the upstream router. Call only for device routers.
+ */
+bool usb4_switch_lane_bonding_possible(struct tb_switch *sw)
+{
+ struct tb_port *up;
+ int ret;
+ u32 val;
+
+ up = tb_upstream_port(sw);
+ ret = tb_port_read(up, &val, TB_CFG_PORT, up->cap_usb4 + PORT_CS_18, 1);
+ if (ret)
+ return false;
+
+ return !!(val & PORT_CS_18_BE);
+}
+
+/**
+ * usb4_switch_set_sleep() - Set sleep bit in order to enable low power states
+ * @sw: USB4 router
+ *
+ * Sets sleep bit for the router and waits for sleep ready to be
+ * asserted.
+ */
+int usb4_switch_set_sleep(struct tb_switch *sw)
+{
+ int ret;
+ u32 val;
+
+ ret = tb_sw_read(sw, &val, TB_CFG_SWITCH, ROUTER_CS_5, 1);
+ if (ret)
+ return ret;
+
+ val |= ROUTER_CS_5_SLP;
+ ret = tb_sw_write(sw, &val, TB_CFG_SWITCH, ROUTER_CS_5, 1);
+ if (ret)
+ return ret;
+
+ /* Wait for sleep ready bit */
+ return usb4_switch_wait_for_bit(sw, ROUTER_CS_6, ROUTER_CS_6_SLPR,
+ ROUTER_CS_6_SLPR, 500);
+}
+
+/**
+ * usb4_switch_nvm_sector_size() - Return router NVM sector size
+ * @sw: USB4 router
+ *
+ * If the router supports NVM operations this function returns the NVM
+ * sector size in bytes. If NVM operations are not supported returns
+ * %-EOPNOTSUPP.
+ */
+int usb4_switch_nvm_sector_size(struct tb_switch *sw)
+{
+ u32 metadata;
+ u8 status;
+ int ret;
+
+ ret = usb4_switch_op(sw, USB4_SWITCH_OP_NVM_SECTOR_SIZE, &status);
+ if (ret)
+ return ret;
+
+ if (status)
+ return status == 0x2 ? -EOPNOTSUPP : -EIO;
+
+ ret = usb4_switch_op_read_metadata(sw, &metadata);
+ if (ret)
+ return ret;
+
+ return metadata & USB4_NVM_SECTOR_SIZE_MASK;
+}
+
+static int usb4_switch_nvm_read_block(struct tb_switch *sw,
+ unsigned int dwaddress, void *buf, size_t dwords)
+{
+ u8 status = 0;
+ u32 metadata;
+ int ret;
+
+ metadata = (dwords << USB4_NVM_READ_LENGTH_SHIFT) &
+ USB4_NVM_READ_LENGTH_MASK;
+ metadata |= (dwaddress << USB4_NVM_READ_OFFSET_SHIFT) &
+ USB4_NVM_READ_OFFSET_MASK;
+
+ ret = usb4_switch_op_write_metadata(sw, metadata);
+ if (ret)
+ return ret;
+
+ ret = usb4_switch_op(sw, USB4_SWITCH_OP_NVM_READ, &status);
+ if (ret)
+ return ret;
+
+ if (status)
+ return -EIO;
+
+ return usb4_switch_op_read_data(sw, buf, dwords);
+}
+
+/**
+ * usb4_switch_nvm_read() - Read arbitrary bytes from router NVM
+ * @sw: USB4 router
+ * @address: Starting address in bytes
+ * @buf: Read data is placed here
+ * @size: How many bytes to read
+ *
+ * Reads NVM contents of the router. If NVM is not supported returns
+ * %-EOPNOTSUPP.
+ */
+int usb4_switch_nvm_read(struct tb_switch *sw, unsigned int address, void *buf,
+ size_t size)
+{
+ return usb4_switch_do_read_data(sw, address, buf, size,
+ usb4_switch_nvm_read_block);
+}
+
+static int usb4_switch_nvm_set_offset(struct tb_switch *sw,
+ unsigned int address)
+{
+ u32 metadata, dwaddress;
+ u8 status = 0;
+ int ret;
+
+ dwaddress = address / 4;
+ metadata = (dwaddress << USB4_NVM_SET_OFFSET_SHIFT) &
+ USB4_NVM_SET_OFFSET_MASK;
+
+ ret = usb4_switch_op_write_metadata(sw, metadata);
+ if (ret)
+ return ret;
+
+ ret = usb4_switch_op(sw, USB4_SWITCH_OP_NVM_SET_OFFSET, &status);
+ if (ret)
+ return ret;
+
+ return status ? -EIO : 0;
+}
+
+static int usb4_switch_nvm_write_next_block(struct tb_switch *sw,
+ const void *buf, size_t dwords)
+{
+ u8 status;
+ int ret;
+
+ ret = usb4_switch_op_write_data(sw, buf, dwords);
+ if (ret)
+ return ret;
+
+ ret = usb4_switch_op(sw, USB4_SWITCH_OP_NVM_WRITE, &status);
+ if (ret)
+ return ret;
+
+ return status ? -EIO : 0;
+}
+
+/**
+ * usb4_switch_nvm_write() - Write to the router NVM
+ * @sw: USB4 router
+ * @address: Start address where to write in bytes
+ * @buf: Pointer to the data to write
+ * @size: Size of @buf in bytes
+ *
+ * Writes @buf to the router NVM using USB4 router operations. If NVM
+ * write is not supported returns %-EOPNOTSUPP.
+ */
+int usb4_switch_nvm_write(struct tb_switch *sw, unsigned int address,
+ const void *buf, size_t size)
+{
+ int ret;
+
+ ret = usb4_switch_nvm_set_offset(sw, address);
+ if (ret)
+ return ret;
+
+ return usb4_switch_do_write_data(sw, address, buf, size,
+ usb4_switch_nvm_write_next_block);
+}
+
+/**
+ * usb4_switch_nvm_authenticate() - Authenticate new NVM
+ * @sw: USB4 router
+ *
+ * After the new NVM has been written via usb4_switch_nvm_write(), this
+ * function triggers NVM authentication process. If the authentication
+ * is successful the router is power cycled and the new NVM starts
+ * running. In case of failure returns negative errno.
+ */
+int usb4_switch_nvm_authenticate(struct tb_switch *sw)
+{
+ u8 status = 0;
+ int ret;
+
+ ret = usb4_switch_op(sw, USB4_SWITCH_OP_NVM_AUTH, &status);
+ if (ret)
+ return ret;
+
+ switch (status) {
+ case 0x0:
+ tb_sw_dbg(sw, "NVM authentication successful\n");
+ return 0;
+ case 0x1:
+ return -EINVAL;
+ case 0x2:
+ return -EAGAIN;
+ case 0x3:
+ return -EOPNOTSUPP;
+ default:
+ return -EIO;
+ }
+}
+
+/**
+ * usb4_switch_query_dp_resource() - Query availability of DP IN resource
+ * @sw: USB4 router
+ * @in: DP IN adapter
+ *
+ * For DP tunneling this function can be used to query availability of
+ * DP IN resource. Returns true if the resource is available for DP
+ * tunneling, false otherwise.
+ */
+bool usb4_switch_query_dp_resource(struct tb_switch *sw, struct tb_port *in)
+{
+ u8 status;
+ int ret;
+
+ ret = usb4_switch_op_write_metadata(sw, in->port);
+ if (ret)
+ return false;
+
+ ret = usb4_switch_op(sw, USB4_SWITCH_OP_QUERY_DP_RESOURCE, &status);
+ /*
+ * If DP resource allocation is not supported assume it is
+ * always available.
+ */
+ if (ret == -EOPNOTSUPP)
+ return true;
+ else if (ret)
+ return false;
+
+ return !status;
+}
+
+/**
+ * usb4_switch_alloc_dp_resource() - Allocate DP IN resource
+ * @sw: USB4 router
+ * @in: DP IN adapter
+ *
+ * Allocates DP IN resource for DP tunneling using USB4 router
+ * operations. If the resource was allocated returns %0. Otherwise
+ * returns negative errno, in particular %-EBUSY if the resource is
+ * already allocated.
+ */
+int usb4_switch_alloc_dp_resource(struct tb_switch *sw, struct tb_port *in)
+{
+ u8 status;
+ int ret;
+
+ ret = usb4_switch_op_write_metadata(sw, in->port);
+ if (ret)
+ return ret;
+
+ ret = usb4_switch_op(sw, USB4_SWITCH_OP_ALLOC_DP_RESOURCE, &status);
+ if (ret == -EOPNOTSUPP)
+ return 0;
+ else if (ret)
+ return ret;
+
+ return status ? -EBUSY : 0;
+}
+
+/**
+ * usb4_switch_dealloc_dp_resource() - Releases allocated DP IN resource
+ * @sw: USB4 router
+ * @in: DP IN adapter
+ *
+ * Releases the previously allocated DP IN resource.
+ */
+int usb4_switch_dealloc_dp_resource(struct tb_switch *sw, struct tb_port *in)
+{
+ u8 status;
+ int ret;
+
+ ret = usb4_switch_op_write_metadata(sw, in->port);
+ if (ret)
+ return ret;
+
+ ret = usb4_switch_op(sw, USB4_SWITCH_OP_DEALLOC_DP_RESOURCE, &status);
+ if (ret == -EOPNOTSUPP)
+ return 0;
+ else if (ret)
+ return ret;
+
+ return status ? -EIO : 0;
+}
+
+static int usb4_port_idx(const struct tb_switch *sw, const struct tb_port *port)
+{
+ int i, usb4_idx = 0;
+
+ /* Assume port is primary */
+ tb_switch_for_each_port(sw, i) {
+ if (tb_is_upstream_port(&sw->ports[i]))
+ continue;
+ if (!tb_port_is_null(&sw->ports[i]))
+ continue;
+ if (!sw->ports[i].link_nr) {
+ if (&sw->ports[i] == port)
+ break;
+ usb4_idx++;
+ }
+ }
+
+ return usb4_idx;
+}
+
+/**
+ * usb4_switch_map_pcie_down() - Map USB4 port to a PCIe downstream adapter
+ * @sw: USB4 router
+ * @port: USB4 port
+ *
+ * USB4 routers have direct mapping between USB4 ports and PCIe
+ * downstream adapters where the PCIe topology is extended. This
+ * function returns the corresponding downstream PCIe adapter or %NULL
+ * if no such mapping was possible.
+ */
+struct tb_port *usb4_switch_map_pcie_down(struct tb_switch *sw,
+ const struct tb_port *port)
+{
+ int usb4_idx = usb4_port_idx(sw, port);
+ int i, pcie_idx = 0;
+
+ /* Find PCIe down port matching usb4_port */
+ tb_switch_for_each_port(sw, i) {
+ if (!tb_port_is_pcie_down(&sw->ports[i]))
+ continue;
+
+ if (pcie_idx == usb4_idx &&
+ !tb_pci_port_is_enabled(&sw->ports[i]))
+ return &sw->ports[i];
+
+ pcie_idx++;
+ }
+
+ return NULL;
+}
+
+/**
+ * usb4_port_unlock() - Unlock USB4 downstream port
+ * @port: USB4 port to unlock
+ *
+ * Unlocks USB4 downstream port so that the connection manager can
+ * access the router below this port.
+ */
+int usb4_port_unlock(struct tb_port *port)
+{
+ int ret;
+ u32 val;
+
+ ret = tb_port_read(port, &val, TB_CFG_PORT, ADP_CS_4, 1);
+ if (ret)
+ return ret;
+
+ val &= ~ADP_CS_4_LCK;
+ return tb_port_write(port, &val, TB_CFG_PORT, ADP_CS_4, 1);
+}
diff --git a/drivers/thunderbolt/xdomain.c b/drivers/thunderbolt/xdomain.c
index 37ef0b4da1cf..11c4fc40aa81 100644
--- a/drivers/thunderbolt/xdomain.c
+++ b/drivers/thunderbolt/xdomain.c
@@ -1220,7 +1220,13 @@ struct tb_xdomain *tb_xdomain_alloc(struct tb *tb, struct device *parent,
u64 route, const uuid_t *local_uuid,
const uuid_t *remote_uuid)
{
+ struct tb_switch *parent_sw = tb_to_switch(parent);
struct tb_xdomain *xd;
+ struct tb_port *down;
+
+ /* Make sure the downstream domain is accessible */
+ down = tb_port_at(route, parent_sw);
+ tb_port_unlock(down);

xd = kzalloc(sizeof(*xd), GFP_KERNEL);
if (!xd)
--
2.23.0

2019-10-01 12:51:47

by Greg Kroah-Hartman

Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4

On Tue, Oct 01, 2019 at 02:38:25PM +0300, Mika Westerberg wrote:
> USB4 is a public spec based on Thunderbolt protocol. There are some
> differences in register layouts and flows. In addition to PCIe and DP
> tunneling, USB4 supports tunneling of USB 3.x. USB4 is also backward
> compatible with Thunderbolt 3 (and older generations but the spec only
> talks about 3rd generation). USB4 compliant devices can be identified by
> checking USB4 version field in router configuration space.
>
> This patch adds initial support for USB4 compliant hosts and devices
> which enables following features provided by the existing functionality
> in the driver:
>
> - PCIe tunneling
> - Display Port tunneling
> - Host and device NVM firmware upgrade
> - P2P networking
>
> This brings the USB4 support to the same level that we already have for
> Thunderbolt 1, 2 and 3 devices.
>
> Note the spec talks about host and device "routers" but in the driver we
> still use term "switch" in most places. Both can be used interchangeably.
>
> This also updates the Kconfig entry accordingly.
>
> Co-developed-by: Rajmohan Mani <[email protected]>
> Signed-off-by: Rajmohan Mani <[email protected]>
> Signed-off-by: Mika Westerberg <[email protected]>
> ---
> drivers/thunderbolt/Kconfig | 9 +-
> drivers/thunderbolt/Makefile | 2 +-
> drivers/thunderbolt/eeprom.c | 53 ++-
> drivers/thunderbolt/nhi.c | 3 +
> drivers/thunderbolt/nhi.h | 2 +
> drivers/thunderbolt/switch.c | 384 +++++++++++++-----
> drivers/thunderbolt/tb.c | 20 +-
> drivers/thunderbolt/tb.h | 36 ++
> drivers/thunderbolt/tb_regs.h | 36 +-
> drivers/thunderbolt/tunnel.c | 10 +-
> drivers/thunderbolt/usb4.c | 722 ++++++++++++++++++++++++++++++++++
> drivers/thunderbolt/xdomain.c | 6 +
> 12 files changed, 1162 insertions(+), 121 deletions(-)
> create mode 100644 drivers/thunderbolt/usb4.c
>
> diff --git a/drivers/thunderbolt/Kconfig b/drivers/thunderbolt/Kconfig
> index fd9adca898ff..8193ec310bae 100644
> --- a/drivers/thunderbolt/Kconfig
> +++ b/drivers/thunderbolt/Kconfig
> @@ -1,6 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0-only
> menuconfig THUNDERBOLT
> - tristate "Thunderbolt support"
> + tristate "USB4 (Thunderbolt) support"
> depends on PCI
> depends on X86 || COMPILE_TEST
> select APPLE_PROPERTIES if EFI_STUB && X86
> @@ -9,9 +9,10 @@ menuconfig THUNDERBOLT
> select CRYPTO_HASH
> select NVMEM
> help
> - Thunderbolt Controller driver. This driver is required if you
> - want to hotplug Thunderbolt devices on Apple hardware or on PCs
> - with Intel Falcon Ridge or newer.
> + USB4 (Thunderbolt) driver. USB4 is the public spec based on
> + Thunderbolt 3 protocol. This driver is required if you want to
> + hotplug Thunderbolt and USB4 compliant devices on Apple
> + hardware or on PCs with Intel Falcon Ridge or newer.

Wait, did "old" thunderbolt just get re-branded as USB4?

Because if I have an "old" laptop that needs Thunderbolt support, how am
I going to know it is now called USB4 instead?

Shouldn't there just be a new USB4 option that only enables/builds the
USB4 stuff if selected? Why would I want all of this additional code on
my old system if it's not going to do anything at all?

Or am I confused?

thanks,

greg k-h

2019-10-01 13:11:55

by Mika Westerberg

Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4

On Tue, Oct 01, 2019 at 02:47:48PM +0200, Greg Kroah-Hartman wrote:
> > - Thunderbolt Controller driver. This driver is required if you
> > - want to hotplug Thunderbolt devices on Apple hardware or on PCs
> > - with Intel Falcon Ridge or newer.
> > + USB4 (Thunderbolt) driver. USB4 is the public spec based on
> > + Thunderbolt 3 protocol. This driver is required if you want to
> > + hotplug Thunderbolt and USB4 compliant devices on Apple
> > + hardware or on PCs with Intel Falcon Ridge or newer.
>
> Wait, did "old" thunderbolt just get re-branded as USB4?

Not but the driver started supporting USB4 as well :)

USB4 is pretty much public spec of Thunderbolt 3 but with some
differences in register layouts (this is because Thunderbolt uses some
vendor specific capabilities which are now moved to more "standard"
places).

> Because if I have an "old" laptop that needs Thunderbolt support, how am
> I going to know it is now called USB4 instead?

Well the Kconfig option tries to have both names there:

tristate "USB4 (Thunderbolt) support"

and then

USB4 (Thunderbolt) driver. USB4 is the public spec based on
Thunderbolt 3 protocol. This driver is required if you want to hotplug
Thunderbolt and USB4 compliant devices on Apple hardware or on PCs
with Intel Falcon Ridge or newer.

and the Kconfig option is still CONFIG_THUNDERBOLT. I know this is
confusing but I don't have better ideas how we can advertise both. I
borrowed this "format" from firewire.

> Shouldn't there just be a new USB4 option that only enables/builds the
> USB4 stuff if selected? Why would I want all of this additional code on
> my old system if it's not going to do anything at all?

USB4 devices are backward compatible with Thunderbolt 3 so you should be
able to plug in USB4 device to your old Thunderbolt 3 laptop for
example. It goes the other way as well. Some things are optional but for
example USB4 hubs must support also Thunderbolt 3.

Does that clarify?

2019-10-01 14:55:45

by Greg Kroah-Hartman

Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4

On Tue, Oct 01, 2019 at 04:09:05PM +0300, Mika Westerberg wrote:
> On Tue, Oct 01, 2019 at 02:47:48PM +0200, Greg Kroah-Hartman wrote:
> > > - Thunderbolt Controller driver. This driver is required if you
> > > - want to hotplug Thunderbolt devices on Apple hardware or on PCs
> > > - with Intel Falcon Ridge or newer.
> > > + USB4 (Thunderbolt) driver. USB4 is the public spec based on
> > > + Thunderbolt 3 protocol. This driver is required if you want to
> > > + hotplug Thunderbolt and USB4 compliant devices on Apple
> > > + hardware or on PCs with Intel Falcon Ridge or newer.
> >
> > Wait, did "old" thunderbolt just get re-branded as USB4?
>
> Not but the driver started supporting USB4 as well :)
>
> USB4 is pretty much public spec of Thunderbolt 3 but with some
> differences in register layouts (this is because Thunderbolt uses some
> vendor specific capabilities which are now moved to more "standard"
> places).

Ok, then we need to rename the Kconfig option as well, otherwise no one
will "know" that this changed, so they will not be prompted for it.

> > Because if I have an "old" laptop that needs Thunderbolt support, how am
> > I going to know it is now called USB4 instead?
>
> Well the Kconfig option tries to have both names there:
>
> tristate "USB4 (Thunderbolt) support"
>
> and then
>
> USB4 (Thunderbolt) driver. USB4 is the public spec based on
> Thunderbolt 3 protocol. This driver is required if you want to hotplug
> Thunderbolt and USB4 compliant devices on Apple hardware or on PCs
> with Intel Falcon Ridge or newer.
>
> and the Kconfig option is still CONFIG_THUNDERBOLT. I know this is
> confusing but I don't have better ideas how we can advertise both. I
> borrowed this "format" from firewire.

CONFIG_USB4 instead?

> > Shouldn't there just be a new USB4 option that only enables/builds the
> > USB4 stuff if selected? Why would I want all of this additional code on
> > my old system if it's not going to do anything at all?
>
> USB4 devices are backward compatible with Thunderbolt 3 so you should be
> able to plug in USB4 device to your old Thunderbolt 3 laptop for
> example. It goes the other way as well. Some things are optional but for
> example USB4 hubs must support also Thunderbolt 3.
>
> Does that clarify?

Yes, it does, looks like marketing just renamed an old functioning
system into a "brand new one!" :)

thanks,

greg k-h

2019-10-01 14:59:46

by Mario Limonciello

Subject: RE: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4

> -----Original Message-----
> From: Greg Kroah-Hartman <[email protected]>
> Sent: Tuesday, October 1, 2019 9:54 AM
> To: Mika Westerberg
> Cc: [email protected]; Andreas Noever; Michael Jamet; Yehezkel Bernat;
> Rajmohan Mani; Nicholas Johnson; Lukas Wunner; Alan Stern; Limonciello, Mario;
> Anthony Wong; [email protected]
> Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4
>
>
> [EXTERNAL EMAIL]
>
> On Tue, Oct 01, 2019 at 04:09:05PM +0300, Mika Westerberg wrote:
> > On Tue, Oct 01, 2019 at 02:47:48PM +0200, Greg Kroah-Hartman wrote:
> > > > - Thunderbolt Controller driver. This driver is required if you
> > > > - want to hotplug Thunderbolt devices on Apple hardware or on PCs
> > > > - with Intel Falcon Ridge or newer.
> > > > + USB4 (Thunderbolt) driver. USB4 is the public spec based on
> > > > + Thunderbolt 3 protocol. This driver is required if you want to
> > > > + hotplug Thunderbolt and USB4 compliant devices on Apple
> > > > + hardware or on PCs with Intel Falcon Ridge or newer.
> > >
> > > Wait, did "old" thunderbolt just get re-branded as USB4?
> >
> > Not but the driver started supporting USB4 as well :)
> >
> > USB4 is pretty much public spec of Thunderbolt 3 but with some
> > differences in register layouts (this is because Thunderbolt uses some
> > vendor specific capabilities which are now moved to more "standard"
> > places).
>
> Ok, then we need to rename the Kconfig option as well, otherwise no one
> will "know" that this changed, so they will not be prompted for it.
>
> > > Because if I have an "old" laptop that needs Thunderbolt support, how am
> > > I going to know it is now called USB4 instead?
> >
> > Well the Kconfig option tries to have both names there:
> >
> > tristate "USB4 (Thunderbolt) support"
> >
> > and then
> >
> > USB4 (Thunderbolt) driver. USB4 is the public spec based on
> > Thunderbolt 3 protocol. This driver is required if you want to hotplug
> > Thunderbolt and USB4 compliant devices on Apple hardware or on PCs
> > with Intel Falcon Ridge or newer.
> >
> > and the Kconfig option is still CONFIG_THUNDERBOLT. I know this is
> > confusing but I don't have better ideas how we can advertise both. I
> > borrowed this "format" from firewire.
>
> CONFIG_USB4 instead?

How about CONFIG_USB4_PCIE?

I think that will help align that certain aspects of USB4 can be built optionally.

>
> > > Shouldn't there just be a new USB4 option that only enables/builds the
> > > USB4 stuff if selected? Why would I want all of this additional code on
> > > my old system if it's not going to do anything at all?
> >
> > USB4 devices are backward compatible with Thunderbolt 3 so you should be
> > able to plug in USB4 device to your old Thunderbolt 3 laptop for
> > example. It goes the other way as well. Some things are optional but for
> > example USB4 hubs must support also Thunderbolt 3.
> >

If PCIe tunnels are an optional feature in USB4, how can it be mandatory to support
Thunderbolt 3?

> > Does that clarify?
>
> Yes, it does, looks like marketing just renamed an old functioning
> system into a "brand new one!" :)
>
> thanks,
>
> greg k-h

2019-10-01 15:09:26

by Mika Westerberg

Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4

On Tue, Oct 01, 2019 at 04:53:54PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Oct 01, 2019 at 04:09:05PM +0300, Mika Westerberg wrote:
> > On Tue, Oct 01, 2019 at 02:47:48PM +0200, Greg Kroah-Hartman wrote:
> > > > - Thunderbolt Controller driver. This driver is required if you
> > > > - want to hotplug Thunderbolt devices on Apple hardware or on PCs
> > > > - with Intel Falcon Ridge or newer.
> > > > + USB4 (Thunderbolt) driver. USB4 is the public spec based on
> > > > + Thunderbolt 3 protocol. This driver is required if you want to
> > > > + hotplug Thunderbolt and USB4 compliant devices on Apple
> > > > + hardware or on PCs with Intel Falcon Ridge or newer.
> > >
> > > Wait, did "old" thunderbolt just get re-branded as USB4?
> >
> > Not but the driver started supporting USB4 as well :)
> >
> > USB4 is pretty much public spec of Thunderbolt 3 but with some
> > differences in register layouts (this is because Thunderbolt uses some
> > vendor specific capabilities which are now moved to more "standard"
> > places).
>
> Ok, then we need to rename the Kconfig option as well, otherwise no one
> will "know" that this changed, so they will not be prompted for it.
>
> > > Because if I have an "old" laptop that needs Thunderbolt support, how am
> > > I going to know it is now called USB4 instead?
> >
> > Well the Kconfig option tries to have both names there:
> >
> > tristate "USB4 (Thunderbolt) support"
> >
> > and then
> >
> > USB4 (Thunderbolt) driver. USB4 is the public spec based on
> > Thunderbolt 3 protocol. This driver is required if you want to hotplug
> > Thunderbolt and USB4 compliant devices on Apple hardware or on PCs
> > with Intel Falcon Ridge or newer.
> >
> > and the Kconfig option is still CONFIG_THUNDERBOLT. I know this is
> > confusing but I don't have better ideas how we can advertise both. I
> > borrowed this "format" from firewire.
>
> CONFIG_USB4 instead?

OK, but does that break existing .configs? I mean if you have already
CONFIG_THUNDERBOLT in your .config/defconfig does it now just get
dropped silently?

For example firewire has CONFIG_FIREWIRE even though the "standard" name
is IEEE 1394. I was thinking maybe we can do the same for
USB4/Thunderbolt?

2019-10-01 15:15:13

by Mika Westerberg

Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4

On Tue, Oct 01, 2019 at 02:59:06PM +0000, [email protected] wrote:
>
>
> > -----Original Message-----
> > From: Greg Kroah-Hartman <[email protected]>
> > Sent: Tuesday, October 1, 2019 9:54 AM
> > To: Mika Westerberg
> > Cc: [email protected]; Andreas Noever; Michael Jamet; Yehezkel Bernat;
> > Rajmohan Mani; Nicholas Johnson; Lukas Wunner; Alan Stern; Limonciello, Mario;
> > Anthony Wong; [email protected]
> > Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4
> >
> >
> > [EXTERNAL EMAIL]
> >
> > On Tue, Oct 01, 2019 at 04:09:05PM +0300, Mika Westerberg wrote:
> > > On Tue, Oct 01, 2019 at 02:47:48PM +0200, Greg Kroah-Hartman wrote:
> > > > > - Thunderbolt Controller driver. This driver is required if you
> > > > > - want to hotplug Thunderbolt devices on Apple hardware or on PCs
> > > > > - with Intel Falcon Ridge or newer.
> > > > > + USB4 (Thunderbolt) driver. USB4 is the public spec based on
> > > > > + Thunderbolt 3 protocol. This driver is required if you want to
> > > > > + hotplug Thunderbolt and USB4 compliant devices on Apple
> > > > > + hardware or on PCs with Intel Falcon Ridge or newer.
> > > >
> > > > Wait, did "old" thunderbolt just get re-branded as USB4?
> > >
> > > Not but the driver started supporting USB4 as well :)
> > >
> > > USB4 is pretty much public spec of Thunderbolt 3 but with some
> > > differences in register layouts (this is because Thunderbolt uses some
> > > vendor specific capabilities which are now moved to more "standard"
> > > places).
> >
> > Ok, then we need to rename the Kconfig option as well, otherwise no one
> > will "know" that this changed, so they will not be prompted for it.
> >
> > > > Because if I have an "old" laptop that needs Thunderbolt support, how am
> > > > I going to know it is now called USB4 instead?
> > >
> > > Well the Kconfig option tries to have both names there:
> > >
> > > tristate "USB4 (Thunderbolt) support"
> > >
> > > and then
> > >
> > > USB4 (Thunderbolt) driver. USB4 is the public spec based on
> > > Thunderbolt 3 protocol. This driver is required if you want to hotplug
> > > Thunderbolt and USB4 compliant devices on Apple hardware or on PCs
> > > with Intel Falcon Ridge or newer.
> > >
> > > and the Kconfig option is still CONFIG_THUNDERBOLT. I know this is
> > > confusing but I don't have better ideas how we can advertise both. I
> > > borrowed this "format" from firewire.
> >
> > CONFIG_USB4 instead?
>
> How about CONFIG_USB4_PCIE?
>
> I think that will help align that certain aspects of USB4 can be built optionally.

But this is not about PCIe - it is just one type of a tunnel that is
optional with USB4.

> > > > Shouldn't there just be a new USB4 option that only enables/builds the
> > > > USB4 stuff if selected? Why would I want all of this additional code on
> > > > my old system if it's not going to do anything at all?
> > >
> > > USB4 devices are backward compatible with Thunderbolt 3 so you should be
> > > able to plug in USB4 device to your old Thunderbolt 3 laptop for
> > > example. It goes the other way as well. Some things are optional but for
> > > example USB4 hubs must support also Thunderbolt 3.
> > >
>
> If PCIe tunnels are an optional feature in USB4, how can it be mandatory to support
> Thunderbolt 3?

It is mandatory for USB4 hubs. For peripheral devices and hosts
Thunderbolt 3 support is optional. So for example you could have USB4
host that does not enter Thunderbolt 3 alternate mode at all so it only
supports USB4 devices.

2019-10-01 15:21:11

by Greg Kroah-Hartman

Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4

On Tue, Oct 01, 2019 at 06:07:34PM +0300, Mika Westerberg wrote:
> On Tue, Oct 01, 2019 at 04:53:54PM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Oct 01, 2019 at 04:09:05PM +0300, Mika Westerberg wrote:
> > > On Tue, Oct 01, 2019 at 02:47:48PM +0200, Greg Kroah-Hartman wrote:
> > > > > - Thunderbolt Controller driver. This driver is required if you
> > > > > - want to hotplug Thunderbolt devices on Apple hardware or on PCs
> > > > > - with Intel Falcon Ridge or newer.
> > > > > + USB4 (Thunderbolt) driver. USB4 is the public spec based on
> > > > > + Thunderbolt 3 protocol. This driver is required if you want to
> > > > > + hotplug Thunderbolt and USB4 compliant devices on Apple
> > > > > + hardware or on PCs with Intel Falcon Ridge or newer.
> > > >
> > > > Wait, did "old" thunderbolt just get re-branded as USB4?
> > >
> > > Not but the driver started supporting USB4 as well :)
> > >
> > > USB4 is pretty much public spec of Thunderbolt 3 but with some
> > > differences in register layouts (this is because Thunderbolt uses some
> > > vendor specific capabilities which are now moved to more "standard"
> > > places).
> >
> > Ok, then we need to rename the Kconfig option as well, otherwise no one
> > will "know" that this changed, so they will not be prompted for it.
> >
> > > > Because if I have an "old" laptop that needs Thunderbolt support, how am
> > > > I going to know it is now called USB4 instead?
> > >
> > > Well the Kconfig option tries to have both names there:
> > >
> > > tristate "USB4 (Thunderbolt) support"
> > >
> > > and then
> > >
> > > USB4 (Thunderbolt) driver. USB4 is the public spec based on
> > > Thunderbolt 3 protocol. This driver is required if you want to hotplug
> > > Thunderbolt and USB4 compliant devices on Apple hardware or on PCs
> > > with Intel Falcon Ridge or newer.
> > >
> > > and the Kconfig option is still CONFIG_THUNDERBOLT. I know this is
> > > confusing but I don't have better ideas how we can advertise both. I
> > > borrowed this "format" from firewire.
> >
> > CONFIG_USB4 instead?
>
> OK, but does that break existing .configs? I mean if you have already
> CONFIG_THUNDERBOLT in your .config/defconfig does it now just get
> dropped silently?

Yup. And then you get asked about CONFIG_USB4

> For example firewire has CONFIG_FIREWIRE even though the "standard" name
> is IEEE 1394. I was thinking maybe we can do the same for
> USB4/Thunderbolt?

It comes down to the what do you want to do for the next 20+ years,
explain to people that "to get USB4 support, enable CONFIG_THUNDERBOLT"?

:)

thanks,

greg k-h

2019-10-01 15:23:32

by Greg Kroah-Hartman

Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4

On Tue, Oct 01, 2019 at 02:59:06PM +0000, [email protected] wrote:
>
>
> > -----Original Message-----
> > From: Greg Kroah-Hartman <[email protected]>
> > Sent: Tuesday, October 1, 2019 9:54 AM
> > To: Mika Westerberg
> > Cc: [email protected]; Andreas Noever; Michael Jamet; Yehezkel Bernat;
> > Rajmohan Mani; Nicholas Johnson; Lukas Wunner; Alan Stern; Limonciello, Mario;
> > Anthony Wong; [email protected]
> > Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4
> >
> >
> > [EXTERNAL EMAIL]
> >
> > On Tue, Oct 01, 2019 at 04:09:05PM +0300, Mika Westerberg wrote:
> > > On Tue, Oct 01, 2019 at 02:47:48PM +0200, Greg Kroah-Hartman wrote:
> > > > > - Thunderbolt Controller driver. This driver is required if you
> > > > > - want to hotplug Thunderbolt devices on Apple hardware or on PCs
> > > > > - with Intel Falcon Ridge or newer.
> > > > > + USB4 (Thunderbolt) driver. USB4 is the public spec based on
> > > > > + Thunderbolt 3 protocol. This driver is required if you want to
> > > > > + hotplug Thunderbolt and USB4 compliant devices on Apple
> > > > > + hardware or on PCs with Intel Falcon Ridge or newer.
> > > >
> > > > Wait, did "old" thunderbolt just get re-branded as USB4?
> > >
> > > Not but the driver started supporting USB4 as well :)
> > >
> > > USB4 is pretty much public spec of Thunderbolt 3 but with some
> > > differences in register layouts (this is because Thunderbolt uses some
> > > vendor specific capabilities which are now moved to more "standard"
> > > places).
> >
> > Ok, then we need to rename the Kconfig option as well, otherwise no one
> > will "know" that this changed, so they will not be prompted for it.
> >
> > > > Because if I have an "old" laptop that needs Thunderbolt support, how am
> > > > I going to know it is now called USB4 instead?
> > >
> > > Well the Kconfig option tries to have both names there:
> > >
> > > tristate "USB4 (Thunderbolt) support"
> > >
> > > and then
> > >
> > > USB4 (Thunderbolt) driver. USB4 is the public spec based on
> > > Thunderbolt 3 protocol. This driver is required if you want to hotplug
> > > Thunderbolt and USB4 compliant devices on Apple hardware or on PCs
> > > with Intel Falcon Ridge or newer.
> > >
> > > and the Kconfig option is still CONFIG_THUNDERBOLT. I know this is
> > > confusing but I don't have better ideas how we can advertise both. I
> > > borrowed this "format" from firewire.
> >
> > CONFIG_USB4 instead?
>
> How about CONFIG_USB4_PCIE?
>
> I think that will help align that certain aspects of USB4 can be built optionally.

What aspects? We don't have that here at all.

I guess the parts of USB4 that are not just this "hook up the PCIe
lane" that need to still be developed?

thanks,
greg k-h

2019-10-01 15:28:26

by Mika Westerberg

Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4

On Tue, Oct 01, 2019 at 05:19:35PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Oct 01, 2019 at 06:07:34PM +0300, Mika Westerberg wrote:
> > On Tue, Oct 01, 2019 at 04:53:54PM +0200, Greg Kroah-Hartman wrote:
> > > On Tue, Oct 01, 2019 at 04:09:05PM +0300, Mika Westerberg wrote:
> > > > On Tue, Oct 01, 2019 at 02:47:48PM +0200, Greg Kroah-Hartman wrote:
> > > > > > - Thunderbolt Controller driver. This driver is required if you
> > > > > > - want to hotplug Thunderbolt devices on Apple hardware or on PCs
> > > > > > - with Intel Falcon Ridge or newer.
> > > > > > + USB4 (Thunderbolt) driver. USB4 is the public spec based on
> > > > > > + Thunderbolt 3 protocol. This driver is required if you want to
> > > > > > + hotplug Thunderbolt and USB4 compliant devices on Apple
> > > > > > + hardware or on PCs with Intel Falcon Ridge or newer.
> > > > >
> > > > > Wait, did "old" thunderbolt just get re-branded as USB4?
> > > >
> > > > Not but the driver started supporting USB4 as well :)
> > > >
> > > > USB4 is pretty much public spec of Thunderbolt 3 but with some
> > > > differences in register layouts (this is because Thunderbolt uses some
> > > > vendor specific capabilities which are now moved to more "standard"
> > > > places).
> > >
> > > Ok, then we need to rename the Kconfig option as well, otherwise no one
> > > will "know" that this changed, so they will not be prompted for it.
> > >
> > > > > Because if I have an "old" laptop that needs Thunderbolt support, how am
> > > > > I going to know it is now called USB4 instead?
> > > >
> > > > Well the Kconfig option tries to have both names there:
> > > >
> > > > tristate "USB4 (Thunderbolt) support"
> > > >
> > > > and then
> > > >
> > > > USB4 (Thunderbolt) driver. USB4 is the public spec based on
> > > > Thunderbolt 3 protocol. This driver is required if you want to hotplug
> > > > Thunderbolt and USB4 compliant devices on Apple hardware or on PCs
> > > > with Intel Falcon Ridge or newer.
> > > >
> > > > and the Kconfig option is still CONFIG_THUNDERBOLT. I know this is
> > > > confusing but I don't have better ideas how we can advertise both. I
> > > > borrowed this "format" from firewire.
> > >
> > > CONFIG_USB4 instead?
> >
> > OK, but does that break existing .configs? I mean if you have already
> > CONFIG_THUNDERBOLT in your .config/defconfig does it now just get
> > dropped silently?
>
> Yup. And then you get asked about CONFIG_USB4

I see.

> > For example firewire has CONFIG_FIREWIRE even though the "standard" name
> > is IEEE 1394. I was thinking maybe we can do the same for
> > USB4/Thunderbolt?
>
> It comes down to the what do you want to do for the next 20+ years,
> explain to people that "to get USB4 support, enable CONFIG_THUNDERBOLT"?

That's a very good point indeed :)

2019-10-01 15:36:10

by Mika Westerberg

Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4

On Tue, Oct 01, 2019 at 05:22:13PM +0200, Greg KH wrote:
> On Tue, Oct 01, 2019 at 02:59:06PM +0000, [email protected] wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg Kroah-Hartman <[email protected]>
> > > Sent: Tuesday, October 1, 2019 9:54 AM
> > > To: Mika Westerberg
> > > Cc: [email protected]; Andreas Noever; Michael Jamet; Yehezkel Bernat;
> > > Rajmohan Mani; Nicholas Johnson; Lukas Wunner; Alan Stern; Limonciello, Mario;
> > > Anthony Wong; [email protected]
> > > Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4
> > >
> > >
> > > [EXTERNAL EMAIL]
> > >
> > > On Tue, Oct 01, 2019 at 04:09:05PM +0300, Mika Westerberg wrote:
> > > > On Tue, Oct 01, 2019 at 02:47:48PM +0200, Greg Kroah-Hartman wrote:
> > > > > > - Thunderbolt Controller driver. This driver is required if you
> > > > > > - want to hotplug Thunderbolt devices on Apple hardware or on PCs
> > > > > > - with Intel Falcon Ridge or newer.
> > > > > > + USB4 (Thunderbolt) driver. USB4 is the public spec based on
> > > > > > + Thunderbolt 3 protocol. This driver is required if you want to
> > > > > > + hotplug Thunderbolt and USB4 compliant devices on Apple
> > > > > > + hardware or on PCs with Intel Falcon Ridge or newer.
> > > > >
> > > > > Wait, did "old" thunderbolt just get re-branded as USB4?
> > > >
> > > > Not but the driver started supporting USB4 as well :)
> > > >
> > > > USB4 is pretty much public spec of Thunderbolt 3 but with some
> > > > differences in register layouts (this is because Thunderbolt uses some
> > > > vendor specific capabilities which are now moved to more "standard"
> > > > places).
> > >
> > > Ok, then we need to rename the Kconfig option as well, otherwise no one
> > > will "know" that this changed, so they will not be prompted for it.
> > >
> > > > > Because if I have an "old" laptop that needs Thunderbolt support, how am
> > > > > I going to know it is now called USB4 instead?
> > > >
> > > > Well the Kconfig option tries to have both names there:
> > > >
> > > > tristate "USB4 (Thunderbolt) support"
> > > >
> > > > and then
> > > >
> > > > USB4 (Thunderbolt) driver. USB4 is the public spec based on
> > > > Thunderbolt 3 protocol. This driver is required if you want to hotplug
> > > > Thunderbolt and USB4 compliant devices on Apple hardware or on PCs
> > > > with Intel Falcon Ridge or newer.
> > > >
> > > > and the Kconfig option is still CONFIG_THUNDERBOLT. I know this is
> > > > confusing but I don't have better ideas how we can advertise both. I
> > > > borrowed this "format" from firewire.
> > >
> > > CONFIG_USB4 instead?
> >
> > How about CONFIG_USB4_PCIE?
> >
> > I think that will help align that certain aspects of USB4 can be built optionally.
>
> What aspects? We don't have that here at all.
>
> I guess the parts of USB4 that are not just this "hook up the PCIe
> lane" that need to still be developed?

Actually PCIe tunneling is already there in the driver. USB4 has one bit
that is needed to be set before PCIe tunneling can happen (we do that in
patch 17/22) but other than that the existing PCIe tunneling code in the
driver works as is.

2019-10-01 16:51:45

by Oliver Neukum

Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4

Am Dienstag, den 01.10.2019, 18:07 +0300 schrieb Mika Westerberg:

Hi,

> OK, but does that break existing .configs? I mean if you have already
> CONFIG_THUNDERBOLT in your .config/defconfig does it now just get
> dropped silently?

People will have to look at this new stuff anyway.

> For example firewire has CONFIG_FIREWIRE even though the "standard" name
> is IEEE 1394. I was thinking maybe we can do the same for
> USB4/Thunderbolt

USB and Thunderbolt used to be distinct protocols. Whereas Firewire
was just a colloquial name for IEEE1394. Please be wordy here.
"Unified support for USB4 and Thunderbolt4"

Regards
Oliver

2019-10-01 17:05:42

by Mario Limonciello

Subject: RE: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4

> @@ -322,9 +398,21 @@ static int tb_switch_nvm_add(struct tb_switch *sw)
> u32 val;
> int ret;
>
> - if (!sw->dma_port)
> + if (!nvm_readable(sw))
> return 0;
>
> + /*
> + * The NVM format of non-Intel hardware is not known so
> + * currently restrict NVM upgrade for Intel hardware. We may
> + * relax this in the future when we learn other NVM formats.
> + */
> + if (sw->config.vendor_id != PCI_VENDOR_ID_INTEL) {
> + dev_info(&sw->dev,
> + "NVM format of vendor %#x is not known, disabling NVM
> upgrade\n",
> + sw->config.vendor_id);
> + return 0;
> + }
> +

Don't you actually have an attribute you can use here for this exact purpose that you
could be setting rather than returning immediately?
sw->no_nvm_upgrade

Then potentially you can at least let users "dump out" the nvm on !Intel but don't let
it be written until ready to relax further.

> nvm = kzalloc(sizeof(*nvm), GFP_KERNEL);
> if (!nvm)
> return -ENOMEM;

2019-10-01 18:15:17

by Mario Limonciello

Subject: RE: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4

> -----Original Message-----
> From: Limonciello, Mario
> Sent: Tuesday, October 1, 2019 12:05 PM
> To: 'Mika Westerberg'; [email protected]
> Cc: Andreas Noever; Michael Jamet; Yehezkel Bernat; Rajmohan Mani; Nicholas
> Johnson; Lukas Wunner; Greg Kroah-Hartman; Alan Stern; Anthony Wong; linux-
> [email protected]
> Subject: RE: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4
>
> > @@ -322,9 +398,21 @@ static int tb_switch_nvm_add(struct tb_switch *sw)
> > u32 val;
> > int ret;
> >
> > - if (!sw->dma_port)
> > + if (!nvm_readable(sw))
> > return 0;
> >
> > + /*
> > + * The NVM format of non-Intel hardware is not known so
> > + * currently restrict NVM upgrade for Intel hardware. We may
> > + * relax this in the future when we learn other NVM formats.
> > + */
> > + if (sw->config.vendor_id != PCI_VENDOR_ID_INTEL) {
> > + dev_info(&sw->dev,
> > + "NVM format of vendor %#x is not known, disabling NVM
> > upgrade\n",
> > + sw->config.vendor_id);
> > + return 0;
> > + }
> > +
>
> Don't you actually have an attribute you can use here for this exact purpose that
> you
> could be setting rather than returning immediately?
> sw->no_nvm_upgrade
>

One more thought; would you consider exporting to sysfs sw->config.vendor_id?
Maybe an attribute that is switch_vendor?

Userland fwupd also does validation on the NVM and will need to follow this.
The same check will go into fwupd to match the vendor and lack of nvm_non_active0
to mark the device as not updatable. When the checks in the kernel get relaxed,
some NVM parsing will have to make it over to fwupd too to relax the check at that point.

> Then potentially you can at least let users "dump out" the nvm on !Intel but don't
> let
> it be written until ready to relax further.
>
> > nvm = kzalloc(sizeof(*nvm), GFP_KERNEL);
> > if (!nvm)
> > return -ENOMEM;

2019-10-02 08:59:08

by Mika Westerberg

Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4

On Tue, Oct 01, 2019 at 05:05:09PM +0000, [email protected] wrote:
> > @@ -322,9 +398,21 @@ static int tb_switch_nvm_add(struct tb_switch *sw)
> > u32 val;
> > int ret;
> >
> > - if (!sw->dma_port)
> > + if (!nvm_readable(sw))
> > return 0;
> >
> > + /*
> > + * The NVM format of non-Intel hardware is not known so
> > + * currently restrict NVM upgrade for Intel hardware. We may
> > + * relax this in the future when we learn other NVM formats.
> > + */
> > + if (sw->config.vendor_id != PCI_VENDOR_ID_INTEL) {
> > + dev_info(&sw->dev,
> > + "NVM format of vendor %#x is not known, disabling NVM
> > upgrade\n",
> > + sw->config.vendor_id);
> > + return 0;
> > + }
> > +
>
> Don't you actually have an attribute you can use here for this exact purpose that you
> could be setting rather than returning immediately?
> sw->no_nvm_upgrade
>
> Then potentially you can at least let users "dump out" the nvm on !Intel but don't let
> it be written until ready to relax further.

Problem is that we currently read NVM version and size from a known
location in the NVM. If we don't know the format we can't do that so
that would mean we need to expose active NVM with some size and hide
nvm_version. I would rather have this support included in the kernel
driver and expose standard set of attributes but no strong feelings from
one way or another.

2019-10-02 09:00:00

by Mika Westerberg

Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4

On Tue, Oct 01, 2019 at 06:14:23PM +0000, [email protected] wrote:
> One more thought; would you consider exporting to sysfs sw->config.vendor_id?
> Maybe an attribute that is switch_vendor?
>
> Userland fwupd also does validation on the NVM and will need to follow this.
> The same check will go into fwupd to match the vendor and lack of nvm_non_active0
> to mark the device as not updatable. When the checks in the kernel get relaxed,
> some NVM parsing will have to make it over to fwupd too to relax the check at that point.

The original idea was that the kernel does the basic validation and
userspace then does more complex checks. Currently you can compare the
two NVM images (active one and the new) and find that information there.
I think fwupd is doing just that already. Is that not enough?

2019-10-02 09:00:38

by Mika Westerberg

Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4

On Tue, Oct 01, 2019 at 06:27:42PM +0200, Oliver Neukum wrote:
> Am Dienstag, den 01.10.2019, 18:07 +0300 schrieb Mika Westerberg:
>
> Hi,
>
> > OK, but does that break existing .configs? I mean if you have already
> > CONFIG_THUNDERBOLT in your .config/defconfig does it now just get
> > dropped silently?
>
> People will have to look at this new stuff anyway.
>
> > For example firewire has CONFIG_FIREWIRE even though the "standard" name
> > is IEEE 1394. I was thinking maybe we can do the same for
> > USB4/Thunderbolt
>
> USB and Thunderbolt used to be distinct protocols. Whereas Firewire
> was just a colloquial name for IEEE1394. Please be wordy here.
> "Unified support for USB4 and Thunderbolt4"

OK.

I've been thinking this bit more and since Thunderbolt will stick around
as well (it basically implements all the optional USB4 features and
more) so would it make sense to have the Kconfig option be
CONFIG_THUNDERBOLT_USB4 (or CONFIG_USB4_THUNDERBOLT)? That should cover
both.

Comments?

Also does anyone have any thoughts about keeping the driver under
drivers/thunderbolt vs. moving it under usb like
drivers/usb/thunderbolt? I'm thinking if anyone not familiar with this
tries to enable support for USB4 so the first place he/she probably
looks is under "USB support" menuconfig entry.

2019-10-02 09:02:01

by Greg Kroah-Hartman

Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4

On Wed, Oct 02, 2019 at 11:30:34AM +0300, Mika Westerberg wrote:
> On Tue, Oct 01, 2019 at 06:27:42PM +0200, Oliver Neukum wrote:
> > Am Dienstag, den 01.10.2019, 18:07 +0300 schrieb Mika Westerberg:
> >
> > Hi,
> >
> > > OK, but does that break existing .configs? I mean if you have already
> > > CONFIG_THUNDERBOLT in your .config/defconfig does it now just get
> > > dropped silently?
> >
> > People will have to look at this new stuff anyway.
> >
> > > For example firewire has CONFIG_FIREWIRE even though the "standard" name
> > > is IEEE 1394. I was thinking maybe we can do the same for
> > > USB4/Thunderbolt
> >
> > USB and Thunderbolt used to be distinct protocols. Whereas Firewire
> > was just a colloquial name for IEEE1394. Please be wordy here.
> > "Unified support for USB4 and Thunderbolt4"
>
> OK.
>
> I've been thinking this bit more and since Thunderbolt will stick around
> as well (it basically implements all the optional USB4 features and
> more) so would it make sense to have the Kconfig option be
> CONFIG_THUNDERBOLT_USB4 (or CONFIG_USB4_THUNDERBOLT)? That should cover
> both.

I would stick with CONFIG_USB4 but put both in the Kconfig text. Again,
it will be easier to handle this over time.

> Comments?
>
> Also does anyone have any thoughts about keeping the driver under
> drivers/thunderbolt vs. moving it under usb like
> drivers/usb/thunderbolt? I'm thinking if anyone not familiar with this
> tries to enable support for USB4 so the first place he/she probably
> looks is under "USB support" menuconfig entry.

You are not sharing/needing any of the drivers/usb/ code just yet,
right? I imagine that will happen "soon" and when it does, then sure,
moving stuff is fine with me.

thanks,

greg k-h

2019-10-02 09:02:01

by Mika Westerberg

Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4

On Wed, Oct 02, 2019 at 10:39:54AM +0200, Greg Kroah-Hartman wrote:
> On Wed, Oct 02, 2019 at 11:30:34AM +0300, Mika Westerberg wrote:
> > On Tue, Oct 01, 2019 at 06:27:42PM +0200, Oliver Neukum wrote:
> > > Am Dienstag, den 01.10.2019, 18:07 +0300 schrieb Mika Westerberg:
> > >
> > > Hi,
> > >
> > > > OK, but does that break existing .configs? I mean if you have already
> > > > CONFIG_THUNDERBOLT in your .config/defconfig does it now just get
> > > > dropped silently?
> > >
> > > People will have to look at this new stuff anyway.
> > >
> > > > For example firewire has CONFIG_FIREWIRE even though the "standard" name
> > > > is IEEE 1394. I was thinking maybe we can do the same for
> > > > USB4/Thunderbolt
> > >
> > > USB and Thunderbolt used to be distinct protocols. Whereas Firewire
> > > was just a colloquial name for IEEE1394. Please be wordy here.
> > > "Unified support for USB4 and Thunderbolt4"
> >
> > OK.
> >
> > I've been thinking this bit more and since Thunderbolt will stick around
> > as well (it basically implements all the optional USB4 features and
> > more) so would it make sense to have the Kconfig option be
> > CONFIG_THUNDERBOLT_USB4 (or CONFIG_USB4_THUNDERBOLT)? That should cover
> > both.
>
> I would stick with CONFIG_USB4 but put both in the Kconfig text. Again,
> it will be easier to handle this over time.

OK, thanks Greg!

> > Comments?
> >
> > Also does anyone have any thoughts about keeping the driver under
> > drivers/thunderbolt vs. moving it under usb like
> > drivers/usb/thunderbolt? I'm thinking if anyone not familiar with this
> > tries to enable support for USB4 so the first place he/she probably
> > looks is under "USB support" menuconfig entry.
>
> You are not sharing/needing any of the drivers/usb/ code just yet,
> right?

Yes, that's correct.

> I imagine that will happen "soon" and when it does, then sure,
> moving stuff is fine with me.

OK thanks!

2019-10-02 15:59:29

by Mario Limonciello

Subject: RE: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4

> -----Original Message-----
> From: Mika Westerberg <[email protected]>
> Sent: Wednesday, October 2, 2019 3:35 AM
> To: Limonciello, Mario
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4
>
>
> [EXTERNAL EMAIL]
>
> On Tue, Oct 01, 2019 at 05:05:09PM +0000, [email protected] wrote:
> > > @@ -322,9 +398,21 @@ static int tb_switch_nvm_add(struct tb_switch *sw)
> > > u32 val;
> > > int ret;
> > >
> > > - if (!sw->dma_port)
> > > + if (!nvm_readable(sw))
> > > return 0;
> > >
> > > + /*
> > > + * The NVM format of non-Intel hardware is not known so
> > > + * currently restrict NVM upgrade for Intel hardware. We may
> > > + * relax this in the future when we learn other NVM formats.
> > > + */
> > > + if (sw->config.vendor_id != PCI_VENDOR_ID_INTEL) {
> > > + dev_info(&sw->dev,
> > > + "NVM format of vendor %#x is not known, disabling
> NVM
> > > upgrade\n",
> > > + sw->config.vendor_id);
> > > + return 0;
> > > + }
> > > +
> >
> > Don't you actually have an attribute you can use here for this exact purpose
> that you
> > could be setting rather than returning immediately?
> > sw->no_nvm_upgrade
> >
> > Then potentially you can at least let users "dump out" the nvm on !Intel but
> don't let
> > it be written until ready to relax further.
>
> Problem is that we currently read NVM version and size from a known
> location in the NVM. If we don't know the format we can't do that so
> that would mean we need to expose active NVM with some size and hide
> nvm_version. I would rather have this support included in the kernel
> driver and expose standard set of attributes but no strong feelings from
> one way or another.

I agree with you; this is a safer and smarter approach to wait until details of format
for other vendors is known and export attributes only when it is. This will also
encourage other vendors to open up their format if the only way to access NVM is
to document the headers.

2019-10-02 16:07:19

by Mario Limonciello

Subject: RE: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4

> -----Original Message-----
> From: Mika Westerberg <[email protected]>
> Sent: Wednesday, October 2, 2019 3:39 AM
> To: Limonciello, Mario
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4
>
>
> [EXTERNAL EMAIL]
>
> On Tue, Oct 01, 2019 at 06:14:23PM +0000, [email protected] wrote:
> > One more thought; would you consider exporting to sysfs sw-
> >config.vendor_id?
> > Maybe an attribute that is switch_vendor?
> >
> > Userland fwupd also does validation on the NVM and will need to follow this.
> > The same check will go into fwupd to match the vendor and lack of
> nvm_non_active0
> > to mark the device as not updatable. When the checks in the kernel get
> relaxed,
> > some NVM parsing will have to make it over to fwupd too to relax the check at
> that point.
>
> The original idea was that the kernel does the basic validation and
> userspace then does more complex checks. Currently you can compare the
> two NVM images (active one and the new) and find that information there.
> I think fwupd is doing just that already. Is that not enough?

I guess for fwupd we can do this without the extra attribute:

1) If no NVM files or nvm_version: means unsupported vendor -show that messaging somewhere.
This means kernel doesn't know the vendor.

2) If NVM file and nvm_version: means kernel knows it
*fwupd checks the vendor offset for intel.
-> intel: good, proceed
->something else: fwupd needs to learn the format for the new vendor, show messaging

There is the unlikely case that kernel knows new vendor format and vendor NVM happened to have
same value as Intel vendor ID in same location of Intel NVM but another meaning.
Hopefully that doesn't happen :)

2019-10-02 16:12:14

by Yehezkel Bernat

Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4

On Wed, Oct 2, 2019 at 6:09 PM <[email protected]> wrote:
>
> > -----Original Message-----
> > From: Mika Westerberg <[email protected]>
> > Sent: Wednesday, October 2, 2019 3:39 AM
> > To: Limonciello, Mario
> > Cc: [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected]
> > Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4
> >
> >
> > [EXTERNAL EMAIL]
> >
> > On Tue, Oct 01, 2019 at 06:14:23PM +0000, [email protected] wrote:
> > > One more thought; would you consider exporting to sysfs sw-
> > >config.vendor_id?
> > > Maybe an attribute that is switch_vendor?
> > >
> > > Userland fwupd also does validation on the NVM and will need to follow this.
> > > The same check will go into fwupd to match the vendor and lack of
> > nvm_non_active0
> > > to mark the device as not updatable. When the checks in the kernel get
> > relaxed,
> > > some NVM parsing will have to make it over to fwupd too to relax the check at
> > that point.
> >
> > The original idea was that the kernel does the basic validation and
> > userspace then does more complex checks. Currently you can compare the
> > two NVM images (active one and the new) and find that information there.
> > I think fwupd is doing just that already. Is that not enough?
>
> I guess for fwupd we can do this without the extra attribute:
>
> 1) If no NVM files or nvm_version: means unsupported vendor -show that messaging somewhere.
> This means kernel doesn't know the vendor.
>
> 2) If NVM file and nvm_version: means kernel knows it
> *fwupd checks the vendor offset for intel.
> -> intel: good, proceed
> ->something else: fwupd needs to learn the format for the new vendor, show messaging
>
> There is the unlikely case that kernel knows new vendor format and vendor NVM happened to have
> same value as Intel vendor ID in same location of Intel NVM but another meaning.
> Hopefully that doesn't happen :)

It's not even "same location - another meaning", the vendor ID comes from the
DROM section, so it takes a few internal jumps inside the NVM to find the
location. One of the "pointers" or section headers will be broken for sure.

And after this, we need to find the NVM in LVFS and it has to pass validation in
a few other locations. The chances are so low that I'd think it isn't worth
worrying about it.

2019-10-02 16:16:44

by Mario Limonciello

Subject: RE: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4

> -----Original Message-----
> From: Yehezkel Bernat <[email protected]>
> Sent: Wednesday, October 2, 2019 10:37 AM
> To: Limonciello, Mario
> Cc: Mika Westerberg; [email protected]; Andreas Noever; Michael
> Jamet; Rajmohan Mani; [email protected]; Lukas
> Wunner; [email protected]; [email protected]; Anthony
> Wong; LKML
> Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4
>
>
> [EXTERNAL EMAIL]
>
> On Wed, Oct 2, 2019 at 6:09 PM <[email protected]> wrote:
> >
> > > -----Original Message-----
> > > From: Mika Westerberg <[email protected]>
> > > Sent: Wednesday, October 2, 2019 3:39 AM
> > > To: Limonciello, Mario
> > > Cc: [email protected]; [email protected];
> > > [email protected]; [email protected];
> [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; [email protected]
> > > Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4
> > >
> > >
> > > [EXTERNAL EMAIL]
> > >
> > > On Tue, Oct 01, 2019 at 06:14:23PM +0000, [email protected]
> wrote:
> > > > One more thought; would you consider exporting to sysfs sw-
> > > >config.vendor_id?
> > > > Maybe an attribute that is switch_vendor?
> > > >
> > > > Userland fwupd also does validation on the NVM and will need to follow
> this.
> > > > The same check will go into fwupd to match the vendor and lack of
> > > nvm_non_active0
> > > > to mark the device as not updatable. When the checks in the kernel get
> > > relaxed,
> > > > some NVM parsing will have to make it over to fwupd too to relax the check
> at
> > > that point.
> > >
> > > The original idea was that the kernel does the basic validation and
> > > userspace then does more complex checks. Currently you can compare the
> > > two NVM images (active one and the new) and find that information there.
> > > I think fwupd is doing just that already. Is that not enough?
> >
> > I guess for fwupd we can do this without the extra attribute:
> >
> > 1) If no NVM files or nvm_version: means unsupported vendor -show that
> messaging somewhere.
> > This means kernel doesn't know the vendor.
> >
> > 2) If NVM file and nvm_version: means kernel knows it
> > *fwupd checks the vendor offset for intel.
> > -> intel: good, proceed
> > ->something else: fwupd needs to learn the format for the new vendor, show
> messaging
> >
> > There is the unlikely case that kernel knows new vendor format and vendor
> NVM happened to have
> > same value as Intel vendor ID in same location of Intel NVM but another
> meaning.
> > Hopefully that doesn't happen :)
>
> It's not even "same location - another meaning", the vendor ID comes from the
> DROM section, so it takes a few internal jumps inside the NVM to find the
> location. One of the "pointers" or section headers will be broken for sure.
>
> And after this, we need to find the NVM in LVFS and it has to pass validation in
> a few other locations. The chances are so low that I'd think it isn't worth
> worrying about it.

And now I remember why the back of my mind was having this thought of wanting
sysfs attribute in the first place. The multiple jumps means that a lot more of the
NVM has to be dumped to get that data, which slows down fwupd startup significantly.
However the kernel has this information handy already from thunderbolt init and can
easily export an attribute which can then come from udev with no startup penalty.

2019-10-03 08:02:19

by Mika Westerberg

Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4

On Wed, Oct 02, 2019 at 04:00:55PM +0000, [email protected] wrote:
> > It's not even "same location - another meaning", the vendor ID comes from the
> > DROM section, so it takes a few internal jumps inside the NVM to find the
> > location. One of the "pointers" or section headers will be broken for sure.
> >
> > And after this, we need to find the NVM in LVFS and it has to pass validation in
> > a few other locations. The chances are so low that I'd think it isn't worth
> > worrying about it.
>
> And now I remember why the back of my mind was having this thought of wanting
> sysfs attribute in the first place. The multiple jumps means that a lot more of the
> NVM has to be dumped to get that data, which slows down fwupd startup significantly.

IIRC currently fwupd does two reads of total 128 bytes from the active
NVM. Is that really slowing down fwupd startup significantly?

> However the kernel has this information handy already from thunderbolt init and can
> easily export an attribute which can then come from udev with no startup penalty.

Indeed kernel has this information but I'm bit hesitant to add new
attributes if that same information is already available to the
userspace rather easily. IMHO we can always add this to the driver later
as we learn new NVM formats that require more parsing from fwupd side
slowing it down considerably.

2019-10-03 14:59:09

by Mario Limonciello

Subject: RE: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4

> -----Original Message-----
> From: Mika Westerberg <[email protected]>
> Sent: Thursday, October 3, 2019 3:00 AM
> To: Limonciello, Mario
> Cc: [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4
>
>
> [EXTERNAL EMAIL]
>
> On Wed, Oct 02, 2019 at 04:00:55PM +0000, [email protected] wrote:
> > > It's not even "same location - another meaning", the vendor ID comes from
> the
> > > DROM section, so it takes a few internal jumps inside the NVM to find the
> > > location. One of the "pointers" or section headers will be broken for sure.
> > >
> > > And after this, we need to find the NVM in LVFS and it has to pass validation
> in
> > > a few other locations. The chances are so low that I'd think it isn't worth
> > > worrying about it.
> >
> > And now I remember why the back of my mind was having this thought of
> wanting
> > sysfs attribute in the first place. The multiple jumps means that a lot more of
> the
> > NVM has to be dumped to get that data, which slows down fwupd startup
> significantly.
>
> IIRC currently fwupd does two reads of total 128 bytes from the active
> NVM. Is that really slowing down fwupd startup significantly?

Yeah, I timed it with fwupd. Here's the averages:

Without doing the reads to jump to this it's 0:00.06 seconds to probe a tree of
Host controller and dock plugged in.

With doing the reads and just host controller:
0:04.40 seconds

With doing the reads and host controller and dock plugged in:
0:10.73 seconds

>
> > However the kernel has this information handy already from thunderbolt init
> and can
> > easily export an attribute which can then come from udev with no startup
> penalty.
>
> Indeed kernel has this information but I'm bit hesitant to add new
> attributes if that same information is already available to the
> userspace rather easily. IMHO we can always add this to the driver later
> as we learn new NVM formats that require more parsing from fwupd side
> slowing it down considerably.

I guess we can wait until new NVM formats come and cross the bridge at that point.
If we encounter a new NVM format that kernel recognizes but old fwupd doesn't it
will reject it anyway (just the error won't be super clear why).

2019-10-04 08:26:09

by Mika Westerberg

Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4

On Thu, Oct 03, 2019 at 02:41:11PM +0000, [email protected] wrote:
> > -----Original Message-----
> > From: Mika Westerberg <[email protected]>
> > Sent: Thursday, October 3, 2019 3:00 AM
> > To: Limonciello, Mario
> > Cc: [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]
> > Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4
> >
> >
> > [EXTERNAL EMAIL]
> >
> > On Wed, Oct 02, 2019 at 04:00:55PM +0000, [email protected] wrote:
> > > > It's not even "same location - another meaning", the vendor ID comes from
> > the
> > > > DROM section, so it takes a few internal jumps inside the NVM to find the
> > > > location. One of the "pointers" or section headers will be broken for sure.
> > > >
> > > > And after this, we need to find the NVM in LVFS and it has to pass validation
> > in
> > > > a few other locations. The chances are so low that I'd think it isn't worth
> > > > worrying about it.
> > >
> > > And now I remember why the back of my mind was having this thought of
> > wanting
> > > sysfs attribute in the first place. The multiple jumps means that a lot more of
> > the
> > > NVM has to be dumped to get that data, which slows down fwupd startup
> > significantly.
> >
> > IIRC currently fwupd does two reads of total 128 bytes from the active
> > NVM. Is that really slowing down fwupd startup significantly?
>
> Yeah, I timed it with fwupd. Here's the averages:
>
> Without doing the reads to jump to this it's 0:00.06 seconds to probe a tree of
> Host controller and dock plugged in.
>
> With doing the reads and just host controller:
> 0:04.40 seconds
>
> With doing the reads and host controller and dock plugged in:
> 0:10.73 seconds

OK, it clearly takes time to read them. I wonder if this includes
powering up the controller?

Also if you can get the hw_vendor_id and hw_product_id from the kernel
does that mean you don't need to do the two reads or you still need
those?

2019-10-04 08:27:36

by Yehezkel Bernat

Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4

On Fri, Oct 4, 2019 at 10:54 AM Mika Westerberg
<[email protected]> wrote:
>
> On Thu, Oct 03, 2019 at 02:41:11PM +0000, [email protected] wrote:
> > > -----Original Message-----
> > > From: Mika Westerberg <[email protected]>
> > > Sent: Thursday, October 3, 2019 3:00 AM
> > > To: Limonciello, Mario
> > > Cc: [email protected]; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; [email protected]; [email protected];
> > > [email protected]; [email protected]
> > > Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4
> > >
> > >
> > > [EXTERNAL EMAIL]
> > >
> > > On Wed, Oct 02, 2019 at 04:00:55PM +0000, [email protected] wrote:
> > > > > It's not even "same location - another meaning", the vendor ID comes from
> > > the
> > > > > DROM section, so it takes a few internal jumps inside the NVM to find the
> > > > > location. One of the "pointers" or section headers will be broken for sure.
> > > > >
> > > > > And after this, we need to find the NVM in LVFS and it has to pass validation
> > > in
> > > > > a few other locations. The chances are so low that I'd think it isn't worth
> > > > > worrying about it.
> > > >
> > > > And now I remember why the back of my mind was having this thought of
> > > wanting
> > > > sysfs attribute in the first place. The multiple jumps means that a lot more of
> > > the
> > > > NVM has to be dumped to get that data, which slows down fwupd startup
> > > significantly.
> > >
> > > IIRC currently fwupd does two reads of total 128 bytes from the active
> > > NVM. Is that really slowing down fwupd startup significantly?
> >
> > Yeah, I timed it with fwupd. Here's the averages:
> >
> > Without doing the reads to jump to this it's 0:00.06 seconds to probe a tree of
> > Host controller and dock plugged in.
> >
> > With doing the reads and just host controller:
> > 0:04.40 seconds
> >
> > With doing the reads and host controller and dock plugged in:
> > 0:10.73 seconds
>
> OK, it clearly takes time to read them. I wonder if this includes
> powering up the controller?
>
> Also if you can get the hw_vendor_id and hw_product_id from the kernel
> does that mean you don't need to do the two reads or you still need
> those?

Are those the chip vendor or the OEM, in case they are different?

Thinking about it again, I'd guess it shouldn't matter much, if the chip is from
Intel, the FW supports NVM upgrade, isn't it?

2019-10-04 08:33:05

by Mika Westerberg

Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4

On Fri, Oct 04, 2019 at 11:07:34AM +0300, Yehezkel Bernat wrote:
> > Also if you can get the hw_vendor_id and hw_product_id from the kernel
> > does that mean you don't need to do the two reads or you still need
> > those?
>
> Are those the chip vendor or the OEM, in case they are different?

Those are the actual USB4 hardware maker values, directly from
ROUTER_CS_0 (p. 287 in the USB4 spec). This almost certainly differ from
the OEM values from DROM we currently expose.

> Thinking about it again, I'd guess it shouldn't matter much, if the chip is from
> Intel, the FW supports NVM upgrade, isn't it?

So the bottom line is that if the kernel thinks the router supports NVM
upgrade it exposes the nvm_active/nvm_non_active files etc. I think
fwupd uses this information to display user whether the device can be
upgraded or not (for example ICL cannot as the NVM is part of BIOS).

Exposing hw_vendor_id and hw_product_id may speed up fwupd because it
does not need to go over the active NVM to figure out whether the new
image is for the correct controller.

2019-10-04 11:58:53

by Yehezkel Bernat

Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4

On Fri, Oct 4, 2019 at 11:19 AM Mika Westerberg
<[email protected]> wrote:
>
> On Fri, Oct 04, 2019 at 11:07:34AM +0300, Yehezkel Bernat wrote:
> > > Also if you can get the hw_vendor_id and hw_product_id from the kernel
> > > does that mean you don't need to do the two reads or you still need
> > > those?
> >
> > Are those the chip vendor or the OEM, in case they are different?
>
> Those are the actual USB4 hardware maker values, directly from
> ROUTER_CS_0 (p. 287 in the USB4 spec). This almost certainly differ from
> the OEM values from DROM we currently expose.

Makes sense to me. Userspace can learn the relevant IDs that their NVM format is
known.

>
> > Thinking about it again, I'd guess it shouldn't matter much, if the chip is from
> > Intel, the FW supports NVM upgrade, isn't it?
>
> So the bottom line is that if the kernel thinks the router supports NVM
> upgrade it exposes the nvm_active/nvm_non_active files etc. I think
> fwupd uses this information to display user whether the device can be
> upgraded or not (for example ICL cannot as the NVM is part of BIOS).

Yes, fwupd already takes this into account, but the question here is how to
handle cases that NVM is available but the format isn't known to
userspace (yet).

>
> Exposing hw_vendor_id and hw_product_id may speed up fwupd because it
> does not need to go over the active NVM to figure out whether the new
> image is for the correct controller.

It's not about finding the relevant image for upgrade (which must be searched
for by looking in the DROM vendor/product values), but about the question if the
NVM format is known to userspace and skip the parsing work if it's anyway going
to fail.

So yes, I think exposing vendor ID (and maybe also product ID) can improve the
situation.

Thanks!

2019-10-04 14:07:31

by Mario Limonciello

Subject: RE: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4

>
> On Fri, Oct 4, 2019 at 11:19 AM Mika Westerberg
> <[email protected]> wrote:
> >
> > On Fri, Oct 04, 2019 at 11:07:34AM +0300, Yehezkel Bernat wrote:
> > > > Also if you can get the hw_vendor_id and hw_product_id from the kernel
> > > > does that mean you don't need to do the two reads or you still need
> > > > those?
> > >
> > > Are those the chip vendor or the OEM, in case they are different?
> >
> > Those are the actual USB4 hardware maker values, directly from
> > ROUTER_CS_0 (p. 287 in the USB4 spec). This almost certainly differ from
> > the OEM values from DROM we currently expose.
>
> Makes sense to me. Userspace can learn the relevant IDs that their NVM format
> is
> known.
>
> >
> > > Thinking about it again, I'd guess it shouldn't matter much, if the chip is from
> > > Intel, the FW supports NVM upgrade, isn't it?
> >
> > So the bottom line is that if the kernel thinks the router supports NVM
> > upgrade it exposes the nvm_active/nvm_non_active files etc. I think
> > fwupd uses this information to display user whether the device can be
> > upgraded or not (for example ICL cannot as the NVM is part of BIOS).
>
> Yes, fwupd already takes this into account, but the question here is how to
> handle cases that NVM is available but the format isn't known to
> userspace (yet).

Exactly.

>
> >
> > Exposing hw_vendor_id and hw_product_id may speed up fwupd because it
> > does not need to go over the active NVM to figure out whether the new
> > image is for the correct controller.
>
> It's not about finding the relevant image for upgrade (which must be searched
> for by looking in the DROM vendor/product values), but about the question if the
> NVM format is known to userspace and skip the parsing work if it's anyway going
> to fail.
>
> So yes, I think exposing vendor ID (and maybe also product ID) can improve the
> situation.
>

Currently at probe time everything comes from udev except for the bit indicating
running in "native" mode or not. Just enough chunks of the NVM are read to determine
that (IE no reading up through DROM or jumping around).

If Christian's patch to export generation is accepted I think that we could move that check
to only read -native if generation < 3.

And if you export the hw_vendor_id and hw_product_id fields then that means USB4 devices
would require no reading from NVM at "probe" since we don't have to read a -native bit.

We would still of course read and analyze the NVM when it comes time to flash a new device
though.

2019-10-04 14:22:51

by Mika Westerberg

Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4

+Christian

On Fri, Oct 04, 2019 at 02:05:46PM +0000, [email protected] wrote:
> >
> > On Fri, Oct 4, 2019 at 11:19 AM Mika Westerberg
> > <[email protected]> wrote:
> > >
> > > On Fri, Oct 04, 2019 at 11:07:34AM +0300, Yehezkel Bernat wrote:
> > > > > Also if you can get the hw_vendor_id and hw_product_id from the kernel
> > > > > does that mean you don't need to do the two reads or you still need
> > > > > those?
> > > >
> > > > Are those the chip vendor or the OEM, in case they are different?
> > >
> > > Those are the actual USB4 hardware maker values, directly from
> > > ROUTER_CS_0 (p. 287 in the USB4 spec). This almost certainly differ from
> > > the OEM values from DROM we currently expose.
> >
> > Makes sense to me. Userspace can learn the relevant IDs that their NVM format
> > is
> > known.
> >
> > >
> > > > Thinking about it again, I'd guess it shouldn't matter much, if the chip is from
> > > > Intel, the FW supports NVM upgrade, isn't it?
> > >
> > > So the bottom line is that if the kernel thinks the router supports NVM
> > > upgrade it exposes the nvm_active/nvm_non_active files etc. I think
> > > fwupd uses this information to display user whether the device can be
> > > upgraded or not (for example ICL cannot as the NVM is part of BIOS).
> >
> > Yes, fwupd already takes this into account, but the question here is how to
> > handle cases that NVM is available but the format isn't known to
> > userspace (yet).
>
> Exactly.
>
> >
> > >
> > > Exposing hw_vendor_id and hw_product_id may speed up fwupd because it
> > > does not need to go over the active NVM to figure out whether the new
> > > image is for the correct controller.
> >
> > It's not about finding the relevant image for upgrade (which must be searched
> > for by looking in the DROM vendor/product values), but about the question if the
> > NVM format is known to userspace and skip the parsing work if it's anyway going
> > to fail.
> >
> > So yes, I think exposing vendor ID (and maybe also product ID) can improve the
> > situation.
> >
>
> Currently at probe time everything comes from udev except for the bit indicating
> running in "native" mode or not. Just enough chunks of the NVM are read to determine
> that (IE no reading up through DROM or jumping around).
>
> If Christian's patch to export generation is accepted I think that we could move that check
> to only read -native if generation < 3.
>
> And if you export the hw_vendor_id and hw_product_id fields then that means USB4 devices
> would require no reading from NVM at "probe" since we don't have to read a -native bit.

Right. So I'm thinking instead of sw->generation what if we expose three
new attributes:

hw_vendor_id - Hardware Vendor ID read from ROUTER_CS_0.
hw_product_id - Hardware Product ID read from ROUTER_CS_0.
hw_version - Hardware USB4 version read from ROUTER_CS_4.

This should allow userspace to determine what exactly the device is and
which version it is. For example USB4 routers the hw_version is 0x20.

@Christian, would this work for bolt as well?

2019-10-04 14:23:51

by Mario Limonciello

Subject: RE: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4

> -----Original Message-----
> From: Limonciello, Mario
> Sent: Friday, October 4, 2019 9:06 AM
> To: 'Yehezkel Bernat'; Mika Westerberg
> Cc: [email protected]; Andreas Noever; Michael Jamet; Rajmohan Mani;
> [email protected]; Lukas Wunner;
> [email protected]; [email protected]; Anthony Wong; LKML
> Subject: RE: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4
>
> >
> > On Fri, Oct 4, 2019 at 11:19 AM Mika Westerberg
> > <[email protected]> wrote:
> > >
> > > On Fri, Oct 04, 2019 at 11:07:34AM +0300, Yehezkel Bernat wrote:
> > > > > Also if you can get the hw_vendor_id and hw_product_id from the kernel
> > > > > does that mean you don't need to do the two reads or you still need
> > > > > those?
> > > >
> > > > Are those the chip vendor or the OEM, in case they are different?
> > >
> > > Those are the actual USB4 hardware maker values, directly from
> > > ROUTER_CS_0 (p. 287 in the USB4 spec). This almost certainly differ from
> > > the OEM values from DROM we currently expose.
> >
> > Makes sense to me. Userspace can learn the relevant IDs that their NVM
> format
> > is
> > known.
> >
> > >
> > > > Thinking about it again, I'd guess it shouldn't matter much, if the chip is from
> > > > Intel, the FW supports NVM upgrade, isn't it?
> > >
> > > So the bottom line is that if the kernel thinks the router supports NVM
> > > upgrade it exposes the nvm_active/nvm_non_active files etc. I think
> > > fwupd uses this information to display user whether the device can be
> > > upgraded or not (for example ICL cannot as the NVM is part of BIOS).
> >
> > Yes, fwupd already takes this into account, but the question here is how to
> > handle cases that NVM is available but the format isn't known to
> > userspace (yet).
>
> Exactly.
>
> >
> > >
> > > Exposing hw_vendor_id and hw_product_id may speed up fwupd because it
> > > does not need to go over the active NVM to figure out whether the new
> > > image is for the correct controller.
> >
> > It's not about finding the relevant image for upgrade (which must be searched
> > for by looking in the DROM vendor/product values), but about the question if
> the
> > NVM format is known to userspace and skip the parsing work if it's anyway
> going
> > to fail.
> >
> > So yes, I think exposing vendor ID (and maybe also product ID) can improve the
> > situation.
> >
>
> Currently at probe time everything comes from udev except for the bit indicating
> running in "native" mode or not. Just enough chunks of the NVM are read to
> determine
> that (IE no reading up through DROM or jumping around).
>
> If Christian's patch to export generation is accepted I think that we could move
> that check
> to only read -native if generation < 3.
>

Sorry for the typo; generation < 4.

> And if you export the hw_vendor_id and hw_product_id fields then that means
> USB4 devices
> would require no reading from NVM at "probe" since we don't have to read a -
> native bit.
>
> We would still of course read and analyze the NVM when it comes time to flash a
> new device
> though.

2019-10-04 15:19:40

by Mika Westerberg

Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4

On Fri, Oct 04, 2019 at 05:02:37PM +0200, Christian Kellner wrote:
> Should work. What would the value be for Thunderbolt 3 (and before)? I
> guess '0' if I am looking at the right thing (bits 31:24 in
> ROUTER_CS_4)?

Yes, it would be 0x10 and below that depending on the generation.

> Is there any harm of also having the 'generation' exposed
> as well? I like the simplicity of the mapping from that value to
> Thunderbolt/USB4 standard version (e.g. I would show that in 'boltctl
> list'); 'hw_version' will need a bit more "interpreting".

If generation is the only thing you need, we can export that now and
forget hw_version :)

2019-10-04 15:24:10

by Christian Kellner

Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4

On Fri, 2019-10-04 at 18:16 +0300, Mika Westerberg wrote:
> > Is there any harm of also having the 'generation' exposed
> > as well? I like the simplicity of the mapping from that value to
> > Thunderbolt/USB4 standard version (e.g. I would show that in
> > 'boltctl
> > list'); 'hw_version' will need a bit more "interpreting".
>
> If generation is the only thing you need, we can export that now and
> forget hw_version :)
Sounds good to me, that is should indeed be good enough for bolt.