Messages in this thread | | | Date | Mon, 18 Dec 2023 15:50:26 +0530 | Subject | Re: [Patch v2 2/2] thunderbolt: Teardown tunnels and reset downstream ports created by boot firmware | From | Sanath S <> |
| |
On 12/15/2023 7:32 PM, Mika Westerberg wrote: > On Fri, Dec 15, 2023 at 07:24:07PM +0530, Sanath S wrote: >> On 12/15/2023 5:25 PM, Mika Westerberg wrote: >>> On Thu, Dec 14, 2023 at 09:00:29PM +0530, Sanath S wrote: >>>>> Unfortunately that's not possible because tb_switch_reset() lives in >>>>> switch.s (and should live there) and tb_deactivate_and_free_tunnel() is >>>>> part of tb.c (as should be). This is actually why I would like to try >>>>> the "reset" protocol adapters + their path config spaces in >>>>> tb_switch_reset() as then that would work with any router and does not >>>>> need to have any knowledge about tunnels or tb.c internals. >>>> Ok. >>>> To perform a protocol adapter reset we would require upstream and downstream >>>> adapter ports. So that we can use tb_port_write() set the ADP_PCIE_CS_0.Path >>>> Enable bits. >>>> Challenge here is to get the upstream port without discovering. I'll think >>>> more on this line >>>> if its possible to reset the protocol adapters and its path. >>>> >>>> If you have any reference here or any idea already in mind, let me know, I >>>> can give it a try :) >>> Here is something I had in mind. Only build tested now, and not split >>> into proper patches. This would make it possible to reset any router if >>> we ever need that (not sure if we want to add the TBT2/3 bits now). Let >>> me know if you see problems with this. >>> >>> Feel free to use this code as you like (or ignore completely). >> Thanks Mika for your suggestion. >> It looks good to me and I'll be checking this on Monday. > Okay. Unfortunately this is not working :( >> I have one doubt on protocol adapter reset part which I have mentioned >> below. >>> diff --git a/drivers/thunderbolt/domain.c b/drivers/thunderbolt/domain.c >>> index ec7b5f65804e..31f3da4e6a08 100644 >>> --- a/drivers/thunderbolt/domain.c >>> +++ b/drivers/thunderbolt/domain.c >>> @@ -423,6 +423,7 @@ struct tb *tb_domain_alloc(struct tb_nhi *nhi, int timeout_msec, size_t privsize >>> /** >>> * tb_domain_add() - Add domain to the system >>> * @tb: Domain to add >>> + * @reset: Issue reset to the host router >>> * >>> * Starts the domain and adds it to the system. Hotplugging devices will >>> * work after this has been returned successfully. In order to remove >>> @@ -431,7 +432,7 @@ struct tb *tb_domain_alloc(struct tb_nhi *nhi, int timeout_msec, size_t privsize >>> * >>> * Return: %0 in case of success and negative errno in case of error >>> */ >>> -int tb_domain_add(struct tb *tb) >>> +int tb_domain_add(struct tb *tb, bool reset) >>> { >>> int ret; >>> @@ -460,7 +461,7 @@ int tb_domain_add(struct tb *tb) >>> /* Start the domain */ >>> if (tb->cm_ops->start) { >>> - ret = tb->cm_ops->start(tb); >>> + ret = tb->cm_ops->start(tb, reset); >>> if (ret) >>> goto err_domain_del; >>> } >>> diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c >>> index d8b9c734abd3..623aa81a8833 100644 >>> --- a/drivers/thunderbolt/icm.c >>> +++ b/drivers/thunderbolt/icm.c >>> @@ -2144,7 +2144,7 @@ static int icm_runtime_resume(struct tb *tb) >>> return 0; >>> } >>> -static int icm_start(struct tb *tb) >>> +static int icm_start(struct tb *tb, bool not_used) >>> { >>> struct icm *icm = tb_priv(tb); >>> int ret; >>> diff --git a/drivers/thunderbolt/lc.c b/drivers/thunderbolt/lc.c >>> index 633970fbe9b0..63cb4b6afb71 100644 >>> --- a/drivers/thunderbolt/lc.c >>> +++ b/drivers/thunderbolt/lc.c >>> @@ -6,6 +6,8 @@ >>> * Author: Mika Westerberg <mika.westerberg@linux.intel.com> >>> */ >>> +#include <linux/delay.h> >>> + >>> #include "tb.h" >>> /** >>> @@ -45,6 +47,49 @@ static int find_port_lc_cap(struct tb_port *port) >>> return sw->cap_lc + start + phys * size; >>> } >>> +/** >>> + * tb_lc_reset_port() - Trigger downstream port reset through LC >>> + * @port: Port that is reset >>> + * >>> + * Triggers downstream port reset through link controller registers. >>> + * Returns %0 in case of success negative errno otherwise. Only supports >>> + * non-USB4 routers with link controller (that's Thunderbolt 2 and >>> + * Thunderbolt 3). >>> + */ >>> +int tb_lc_reset_port(struct tb_port *port) >>> +{ >>> + struct tb_switch *sw = port->sw; >>> + int cap, ret; >>> + u32 mode; >>> + >>> + if (sw->generation < 2) >>> + return -EINVAL; >>> + >>> + cap = find_port_lc_cap(port); >>> + if (cap < 0) >>> + return cap; >>> + >>> + ret = tb_sw_read(sw, &mode, TB_CFG_SWITCH, cap + TB_LC_PORT_MODE, 1); >>> + if (ret) >>> + return ret; >>> + >>> + mode |= TB_LC_PORT_MODE_DPR; >>> + >>> + ret = tb_sw_write(sw, &mode, TB_CFG_SWITCH, cap + TB_LC_PORT_MODE, 1); >>> + if (ret) >>> + return ret; >>> + >>> + fsleep(10000); >>> + >>> + ret = tb_sw_read(sw, &mode, TB_CFG_SWITCH, cap + TB_LC_PORT_MODE, 1); >>> + if (ret) >>> + return ret; >>> + >>> + mode &= ~TB_LC_PORT_MODE_DPR; >>> + >>> + return tb_sw_write(sw, &mode, TB_CFG_SWITCH, cap + TB_LC_PORT_MODE, 1); >>> +} >>> + >>> static int tb_lc_set_port_configured(struct tb_port *port, bool configured) >>> { >>> bool upstream = tb_is_upstream_port(port); >>> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c >>> index fb4f46e51753..b22023fae60d 100644 >>> --- a/drivers/thunderbolt/nhi.c >>> +++ b/drivers/thunderbolt/nhi.c >>> @@ -1221,7 +1221,7 @@ static void nhi_check_iommu(struct tb_nhi *nhi) >>> str_enabled_disabled(port_ok)); >>> } >>> -static void nhi_reset(struct tb_nhi *nhi) >>> +static bool nhi_reset(struct tb_nhi *nhi) >>> { >>> ktime_t timeout; >>> u32 val; >>> @@ -1229,11 +1229,11 @@ static void nhi_reset(struct tb_nhi *nhi) >>> val = ioread32(nhi->iobase + REG_CAPS); >>> /* Reset only v2 and later routers */ >>> if (FIELD_GET(REG_CAPS_VERSION_MASK, val) < REG_CAPS_VERSION_2) >>> - return; >>> + return false; >>> if (!host_reset) { >>> dev_dbg(&nhi->pdev->dev, "skipping host router reset\n"); >>> - return; >>> + return false; >>> } >>> iowrite32(REG_RESET_HRR, nhi->iobase + REG_RESET); >>> @@ -1244,12 +1244,14 @@ static void nhi_reset(struct tb_nhi *nhi) >>> val = ioread32(nhi->iobase + REG_RESET); >>> if (!(val & REG_RESET_HRR)) { >>> dev_warn(&nhi->pdev->dev, "host router reset successful\n"); >>> - return; >>> + return true; >>> } >>> usleep_range(10, 20); >>> } while (ktime_before(ktime_get(), timeout)); >>> dev_warn(&nhi->pdev->dev, "timeout resetting host router\n"); >>> + >>> + return false; >>> } >>> static int nhi_init_msi(struct tb_nhi *nhi) >>> @@ -1331,6 +1333,7 @@ static int nhi_probe(struct pci_dev *pdev, const struct pci_device_id *id) >>> struct device *dev = &pdev->dev; >>> struct tb_nhi *nhi; >>> struct tb *tb; >>> + bool reset; >>> int res; >>> if (!nhi_imr_valid(pdev)) >>> @@ -1365,7 +1368,11 @@ static int nhi_probe(struct pci_dev *pdev, const struct pci_device_id *id) >>> nhi_check_quirks(nhi); >>> nhi_check_iommu(nhi); >>> - nhi_reset(nhi); >>> + /* >>> + * Only USB4 v2 hosts support host reset so if we already did >>> + * that then don't do it again when the domain is initialized. >>> + */ >>> + reset = nhi_reset(nhi) ? false : host_reset; >>> res = nhi_init_msi(nhi); >>> if (res) >>> @@ -1392,7 +1399,7 @@ static int nhi_probe(struct pci_dev *pdev, const struct pci_device_id *id) >>> dev_dbg(dev, "NHI initialized, starting thunderbolt\n"); >>> - res = tb_domain_add(tb); >>> + res = tb_domain_add(tb, reset); >>> if (res) { >>> /* >>> * At this point the RX/TX rings might already have been >>> diff --git a/drivers/thunderbolt/path.c b/drivers/thunderbolt/path.c >>> index 091a81bbdbdc..f760e54cd9bd 100644 >>> --- a/drivers/thunderbolt/path.c >>> +++ b/drivers/thunderbolt/path.c >>> @@ -446,6 +446,19 @@ static int __tb_path_deactivate_hop(struct tb_port *port, int hop_index, >>> return -ETIMEDOUT; >>> } >>> +/** >>> + * tb_path_deactivate_hop() - Deactivate one path in path config space >>> + * @port: Lane or protocol adapter >>> + * @hop_index: HopID of the path to be cleared >>> + * >>> + * This deactivates or clears a single path config space entry at >>> + * @hop_index. Returns %0 in success and negative errno otherwise. >>> + */ >>> +int tb_path_deactivate_hop(struct tb_port *port, int hop_index) >>> +{ >>> + return __tb_path_deactivate_hop(port, hop_index, true); >>> +} >>> + >>> static void __tb_path_deactivate_hops(struct tb_path *path, int first_hop) >>> { >>> int i, res; >>> diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c >>> index 900114ba4371..c4f486629a2b 100644 >>> --- a/drivers/thunderbolt/switch.c >>> +++ b/drivers/thunderbolt/switch.c >>> @@ -676,6 +676,13 @@ int tb_port_disable(struct tb_port *port) >>> return __tb_port_enable(port, false); >>> } >>> +static int tb_port_reset(struct tb_port *port) >>> +{ >>> + if (tb_switch_is_usb4(port->sw)) >>> + return usb4_port_reset(port); >>> + return tb_lc_reset_port(port); >>> +} >>> + >>> /* >>> * tb_init_port() - initialize a port >>> * >>> @@ -1532,20 +1539,64 @@ static void tb_dump_switch(const struct tb *tb, const struct tb_switch *sw) >>> } >>> /** >>> - * tb_switch_reset() - reconfigure route, enable and send TB_CFG_PKG_RESET >>> - * @sw: Switch to reset >>> + * tb_switch_reset() - Perform reset to the router >>> + * @sw: Router to reset >>> * >>> - * Return: Returns 0 on success or an error code on failure. >>> + * Issues reset to the router. Can be used for any router. Returns %0 >>> + * on success or an error code on failure. >>> */ >>> int tb_switch_reset(struct tb_switch *sw) >>> { >>> struct tb_cfg_result res; >>> - if (sw->generation > 1) >>> - return 0; >>> + tb_sw_dbg(sw, "resetting router\n"); >>> + >>> + if (sw->generation > 1) { >>> + struct tb_port *port; >>> + >>> + tb_switch_for_each_port(sw, port) { >>> + int i, ret; >>> + >>> + /* >>> + * For lane adapters we issue downstream port >>> + * reset. That clears up also the path config >>> + * spaces. >>> + * >>> + * For protocol adapters we disable the path and >>> + * clear path config space one by one (from 8 to >>> + * Max Input HopID of the adapter). >>> + */ >>> + if (tb_port_is_null(port) && !tb_is_upstream_port(port)) { >>> + ret = tb_port_reset(port); >>> + if (ret) >>> + return ret; >>> + continue; >> I had thought in similar lines when you told about reset protocol adapters. >> >> But, here we are traversing through all the ports, what if we get to perform >> the DPR first ? >> and then the PCIe, USB and DP reset. This may cause unplug event before we >> tear down >> protocol adapters and its path configuration. (I may be wrong) > Yeah, it could be that it is better first disable protocol adapters and > then do the DPR or so. I gave it a try doing a DPR later and before disabling protocol adapters. Both the methods didn't work.
However, when I do it like below, it works.
tb_switch_discover_tunnels(tb->root_switch, &tcm->tunnel_list, false); ret = tb_switch_reset(tb->root_switch);
We are missing something here. >> I'll check the behavior on Monday and update. >> >> Assuming this works, I can incorporate the suggestion and send out v3 with >> appropriate tags ? It can be split into 3 patches. > Sure. > > Bonus points if you can drop some more lines from that :) > >>> + } else if (tb_port_is_usb3_down(port) || >>> + tb_port_is_usb3_up(port)) { >>> + tb_usb3_port_enable(port, false); >>> + } else if (tb_port_is_dpin(port) || >>> + tb_port_is_dpout(port)) { >>> + tb_dp_port_enable(port, false); >>> + } else if (tb_port_is_pcie_down(port) || >>> + tb_port_is_pcie_up(port)) { >>> + tb_pci_port_enable(port, false); Here, as per spec it would be better if we first teardown it for DOWN path and then the UP path. >>> + } else { >>> + continue; >>> + } >>> - tb_sw_dbg(sw, "resetting switch\n"); >>> + /* Cleanup path config space of protocol adapter */ >>> + for (i = TB_PATH_MIN_HOPID; >>> + i <= port->config.max_in_hop_id; i++) { >>> + ret = tb_path_deactivate_hop(port, i); >>> + if (ret) >>> + return ret; >>> + } >>> + } >>> + >>> + return 0; >>> + } >>> + /* Thunderbolt 1 uses the "reset" config space packet */ >>> res.err = tb_sw_write(sw, ((u32 *) &sw->config) + 2, >>> TB_CFG_SWITCH, 2, 2); >>> if (res.err) >>> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c >>> index 8bc3985df749..5e5e3aebe018 100644 >>> --- a/drivers/thunderbolt/tb.c >>> +++ b/drivers/thunderbolt/tb.c >>> @@ -2590,7 +2590,7 @@ static int tb_scan_finalize_switch(struct device *dev, void *data) >>> return 0; >>> } >>> -static int tb_start(struct tb *tb) >>> +static int tb_start(struct tb *tb, bool reset) >>> { >>> struct tb_cm *tcm = tb_priv(tb); >>> int ret; >>> @@ -2631,12 +2631,22 @@ static int tb_start(struct tb *tb) >>> tb_switch_tmu_configure(tb->root_switch, TB_SWITCH_TMU_MODE_LOWRES); >>> /* Enable TMU if it is off */ >>> tb_switch_tmu_enable(tb->root_switch); >>> - /* Full scan to discover devices added before the driver was loaded. */ >>> - tb_scan_switch(tb->root_switch); >>> - /* Find out tunnels created by the boot firmware */ >>> - tb_discover_tunnels(tb); >>> - /* Add DP resources from the DP tunnels created by the boot firmware */ >>> - tb_discover_dp_resources(tb); >>> + >>> + if (reset && usb4_switch_version(tb->root_switch) == 1) { >>> + ret = tb_switch_reset(tb->root_switch); >>> + if (ret) { >>> + tb_sw_warn(tb->root_switch, "failed to reset\n"); >>> + return ret; >>> + } >>> + >> Ok. So idea is to drop reset for <= TBT3 currently. > Yes, there are some older Apple systems that "benefit" from the > discovery so I would keep it there for them. > >>> } else { >>> + /* Full scan to discover devices added before the driver was loaded. */ >>> + tb_scan_switch(tb->root_switch); >>> + /* Find out tunnels created by the boot firmware */ >>> + tb_discover_tunnels(tb); >>> + /* Add DP resources from the DP tunnels created by the boot firmware */ >>> + tb_discover_dp_resources(tb); >>> + } >>> + >>> /* >>> * If the boot firmware did not create USB 3.x tunnels create them >>> * now for the whole topology. >>> @@ -2702,7 +2712,7 @@ static int tb_resume_noirq(struct tb *tb) >>> { >>> struct tb_cm *tcm = tb_priv(tb); >>> struct tb_tunnel *tunnel, *n; >>> - unsigned int usb3_delay = 0; >>> + unsigned int usb3_delay; >>> LIST_HEAD(tunnels); >>> tb_dbg(tb, "resuming...\n"); >>> @@ -2715,19 +2725,7 @@ static int tb_resume_noirq(struct tb *tb) >>> tb_free_unplugged_children(tb->root_switch); >>> tb_restore_children(tb->root_switch); >>> - /* >>> - * If we get here from suspend to disk the boot firmware or the >>> - * restore kernel might have created tunnels of its own. Since >>> - * we cannot be sure they are usable for us we find and tear >>> - * them down. >>> - */ >>> - tb_switch_discover_tunnels(tb->root_switch, &tunnels, false); >>> - list_for_each_entry_safe_reverse(tunnel, n, &tunnels, list) { >>> - if (tb_tunnel_is_usb3(tunnel)) >>> - usb3_delay = 500; >>> - tb_tunnel_deactivate(tunnel); >>> - tb_tunnel_free(tunnel); >>> - } >>> + usb3_delay = tb_switch_is_usb4(tb->root_switch) ? 500 : 0; >>> /* Re-create our tunnels now */ >>> list_for_each_entry_safe(tunnel, n, &tcm->tunnel_list, list) { >>> diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h >>> index 7ad55f5966f3..2bc8eca965ed 100644 >>> --- a/drivers/thunderbolt/tb.h >>> +++ b/drivers/thunderbolt/tb.h >>> @@ -489,7 +489,7 @@ struct tb_path { >>> */ >>> struct tb_cm_ops { >>> int (*driver_ready)(struct tb *tb); >>> - int (*start)(struct tb *tb); >>> + int (*start)(struct tb *tb, bool reset); >>> void (*stop)(struct tb *tb); >>> int (*suspend_noirq)(struct tb *tb); >>> int (*resume_noirq)(struct tb *tb); >>> @@ -752,7 +752,7 @@ int tb_xdomain_init(void); >>> void tb_xdomain_exit(void); >>> struct tb *tb_domain_alloc(struct tb_nhi *nhi, int timeout_msec, size_t privsize); >>> -int tb_domain_add(struct tb *tb); >>> +int tb_domain_add(struct tb *tb, bool reset); >>> void tb_domain_remove(struct tb *tb); >>> int tb_domain_suspend_noirq(struct tb *tb); >>> int tb_domain_resume_noirq(struct tb *tb); >>> @@ -1156,6 +1156,7 @@ struct tb_path *tb_path_alloc(struct tb *tb, struct tb_port *src, int src_hopid, >>> void tb_path_free(struct tb_path *path); >>> int tb_path_activate(struct tb_path *path); >>> void tb_path_deactivate(struct tb_path *path); >>> +int tb_path_deactivate_hop(struct tb_port *port, int hop_index); >>> bool tb_path_is_invalid(struct tb_path *path); >>> bool tb_path_port_on_path(const struct tb_path *path, >>> const struct tb_port *port); >>> @@ -1175,6 +1176,7 @@ int tb_drom_read(struct tb_switch *sw); >>> int tb_drom_read_uid_only(struct tb_switch *sw, u64 *uid); >>> int tb_lc_read_uuid(struct tb_switch *sw, u32 *uuid); >>> +int tb_lc_reset_port(struct tb_port *port); >>> int tb_lc_configure_port(struct tb_port *port); >>> void tb_lc_unconfigure_port(struct tb_port *port); >>> int tb_lc_configure_xdomain(struct tb_port *port); >>> @@ -1307,6 +1309,7 @@ void usb4_switch_remove_ports(struct tb_switch *sw); >>> int usb4_port_unlock(struct tb_port *port); >>> int usb4_port_hotplug_enable(struct tb_port *port); >>> +int usb4_port_reset(struct tb_port *port); >>> int usb4_port_configure(struct tb_port *port); >>> void usb4_port_unconfigure(struct tb_port *port); >>> int usb4_port_configure_xdomain(struct tb_port *port, struct tb_xdomain *xd); >>> diff --git a/drivers/thunderbolt/tb_regs.h b/drivers/thunderbolt/tb_regs.h >>> index 87e4795275fe..efcae298b370 100644 >>> --- a/drivers/thunderbolt/tb_regs.h >>> +++ b/drivers/thunderbolt/tb_regs.h >>> @@ -389,6 +389,7 @@ struct tb_regs_port_header { >>> #define PORT_CS_18_CSA BIT(22) >>> #define PORT_CS_18_TIP BIT(24) >>> #define PORT_CS_19 0x13 >>> +#define PORT_CS_19_DPR BIT(0) >>> #define PORT_CS_19_PC BIT(3) >>> #define PORT_CS_19_PID BIT(4) >>> #define PORT_CS_19_WOC BIT(16) >>> @@ -584,6 +585,9 @@ struct tb_regs_hop { >>> #define TB_LC_POWER 0x740 >>> /* Link controller registers */ >>> +#define TB_LC_PORT_MODE 0x26 >>> +#define TB_LC_PORT_MODE_DPR BIT(0) >>> + >>> #define TB_LC_CS_42 0x2a >>> #define TB_LC_CS_42_USB_PLUGGED BIT(31) >>> diff --git a/drivers/thunderbolt/usb4.c b/drivers/thunderbolt/usb4.c >>> index 675d1ed62372..9e002bf73d2e 100644 >>> --- a/drivers/thunderbolt/usb4.c >>> +++ b/drivers/thunderbolt/usb4.c >>> @@ -1107,6 +1107,45 @@ int usb4_port_hotplug_enable(struct tb_port *port) >>> return tb_port_write(port, &val, TB_CFG_PORT, ADP_CS_5, 1); >>> } >>> +/** >>> + * usb4_port_reset() - Issue downstream port reset >>> + * @port: USB4 port to reset >>> + * >>> + * Issues downstream port reset to @port. >>> + */ >>> +int usb4_port_reset(struct tb_port *port) >>> +{ >>> + int ret; >>> + u32 val; >>> + >>> + if (!port->cap_usb4) >>> + return -EINVAL; >>> + We've to drop this check here and check it before calling for that port. Otherwise, I it may throw -EINVAL and results in probe failure. >>> + ret = tb_port_read(port, &val, TB_CFG_PORT, >>> + port->cap_usb4 + PORT_CS_19, 1); >>> + if (ret) >>> + return ret; >>> + >>> + val |= PORT_CS_19_DPR; >>> + >>> + ret = tb_port_write(port, &val, TB_CFG_PORT, >>> + port->cap_usb4 + PORT_CS_19, 1); >>> + if (ret) >>> + return ret; >>> + >>> + fsleep(10000); >>> + >>> + ret = tb_port_read(port, &val, TB_CFG_PORT, >>> + port->cap_usb4 + PORT_CS_19, 1); >>> + if (ret) >>> + return ret; >>> + >>> + val &= ~PORT_CS_19_DPR; >>> + >>> + return tb_port_write(port, &val, TB_CFG_PORT, >>> + port->cap_usb4 + PORT_CS_19, 1); >>> +} >>> + >>> static int usb4_port_set_configured(struct tb_port *port, bool configured) >>> { >>> int ret; >>> >>>
| |