| Message ID | 20231212191635.2022520-3-Sanath.S@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for downstream port reset(DPR) | expand |
On 12/12/2023 13:16, Sanath S wrote: > Boot firmware might have created tunnels of its own. Since we cannot > be sure they are usable for us. Tear them down and reset the ports > to handle it as a new hotplug for USB3 routers. s/3/4/ > > Suggested-by: Mario Limonciello <mario.limonciello@amd.com> > Signed-off-by: Sanath S <Sanath.S@amd.com> > --- > drivers/thunderbolt/tb.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c > index fd49f86e0353..febd0b6972e3 100644 > --- a/drivers/thunderbolt/tb.c > +++ b/drivers/thunderbolt/tb.c > @@ -2598,6 +2598,17 @@ static int tb_start(struct tb *tb) > tb_switch_tmu_enable(tb->root_switch); > /* Full scan to discover devices added before the driver was loaded. */ > tb_scan_switch(tb->root_switch); > + /* > + * Boot firmware might have created tunnels of its own. Since we cannot > + * be sure they are usable for us, Tear them down and reset the ports > + * to handle it as new hotplug for USB4 routers. > + */ > + if (tb_switch_is_usb4(tb->root_switch)) { > + tb_switch_discover_tunnels(tb->root_switch, > + &tcm->tunnel_list, false); > + tcm->hotplug_active = true; > + return tb_switch_reset_ports(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 */ Doesn't this cause the following to not run and thus break hotplug? tcm->hotplug_active = true; I think it would be better to do this like this flow: if (tb_switch_is_usb4(tb->root_switch)) { tb_switch_discover_tunnels(tb->root_switch, &tcm->tunnel_list, false); tcm->hotplug_active = true; ret = tb_switch_reset_ports(tb->root_switch); if (ret) return ret; } else { /* keep existing tunnel flow */ } tcm->hotplug_active = true; return 0; That makes it crystal clear that hotplug isn't enabled until it's done being setup, which means either getting the existing tunnels or doing the reset.
On 12/12/2023 13:24, Mario Limonciello wrote: > On 12/12/2023 13:16, Sanath S wrote: >> Boot firmware might have created tunnels of its own. Since we cannot >> be sure they are usable for us. Tear them down and reset the ports >> to handle it as a new hotplug for USB3 routers. > s/3/4/ > >> >> Suggested-by: Mario Limonciello <mario.limonciello@amd.com> >> Signed-off-by: Sanath S <Sanath.S@amd.com> >> --- >> drivers/thunderbolt/tb.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c >> index fd49f86e0353..febd0b6972e3 100644 >> --- a/drivers/thunderbolt/tb.c >> +++ b/drivers/thunderbolt/tb.c >> @@ -2598,6 +2598,17 @@ static int tb_start(struct tb *tb) >> tb_switch_tmu_enable(tb->root_switch); >> /* Full scan to discover devices added before the driver was >> loaded. */ >> tb_scan_switch(tb->root_switch); >> + /* >> + * Boot firmware might have created tunnels of its own. Since we >> cannot >> + * be sure they are usable for us, Tear them down and reset the >> ports >> + * to handle it as new hotplug for USB4 routers. >> + */ >> + if (tb_switch_is_usb4(tb->root_switch)) { >> + tb_switch_discover_tunnels(tb->root_switch, >> + &tcm->tunnel_list, false); >> + tcm->hotplug_active = true; >> + return tb_switch_reset_ports(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 */ > > Doesn't this cause the following to not run and thus break hotplug? > > tcm->hotplug_active = true; > > > I think it would be better to do this like this flow: > > if (tb_switch_is_usb4(tb->root_switch)) { > tb_switch_discover_tunnels(tb->root_switch, > &tcm->tunnel_list, false); > tcm->hotplug_active = true; > ret = tb_switch_reset_ports(tb->root_switch); > if (ret) > return ret; > } else { > /* keep existing tunnel flow */ > } > > tcm->hotplug_active = true; > > return 0; > > That makes it crystal clear that hotplug isn't enabled until it's done > being setup, which means either getting the existing tunnels or doing > the reset. My apologies I completely missed replacement call even though I pasted it. Besides the s/3/4 this looks fine to me. Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
On Wed, Dec 13, 2023 at 12:46:35AM +0530, Sanath S wrote: > Boot firmware might have created tunnels of its own. Since we cannot > be sure they are usable for us. Tear them down and reset the ports > to handle it as a new hotplug for USB3 routers. > > Suggested-by: Mario Limonciello <mario.limonciello@amd.com> > Signed-off-by: Sanath S <Sanath.S@amd.com> > --- > drivers/thunderbolt/tb.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c > index fd49f86e0353..febd0b6972e3 100644 > --- a/drivers/thunderbolt/tb.c > +++ b/drivers/thunderbolt/tb.c > @@ -2598,6 +2598,17 @@ static int tb_start(struct tb *tb) > tb_switch_tmu_enable(tb->root_switch); > /* Full scan to discover devices added before the driver was loaded. */ > tb_scan_switch(tb->root_switch); > + /* > + * Boot firmware might have created tunnels of its own. Since we cannot > + * be sure they are usable for us, Tear them down and reset the ports > + * to handle it as new hotplug for USB4 routers. > + */ > + if (tb_switch_is_usb4(tb->root_switch)) { > + tb_switch_discover_tunnels(tb->root_switch, > + &tcm->tunnel_list, false); Why this is needed? It should be enough, to do simply something like this: if (tb_switch_is_usb4(tb->root_switch)) tb_switch_reset(tb->root_switch); and continue with the rest of the function. The tb_switch_reset() then resets the downstream ports synchronously so when it returns there is nothing to be discover. (We can use the tb_switch_reset() here, it already exists and is performing TBT 1 specific things but you can make it handle downstream port reset in case of USB4). You don't need to touch the ->hotplug_active or anything else here. > + tcm->hotplug_active = true; > + return tb_switch_reset_ports(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 */ > -- > 2.34.1
On Wed, Dec 13, 2023 at 07:49:14AM +0200, Mika Westerberg wrote: > On Wed, Dec 13, 2023 at 12:46:35AM +0530, Sanath S wrote: > > Boot firmware might have created tunnels of its own. Since we cannot > > be sure they are usable for us. Tear them down and reset the ports > > to handle it as a new hotplug for USB3 routers. > > > > Suggested-by: Mario Limonciello <mario.limonciello@amd.com> > > Signed-off-by: Sanath S <Sanath.S@amd.com> > > --- > > drivers/thunderbolt/tb.c | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c > > index fd49f86e0353..febd0b6972e3 100644 > > --- a/drivers/thunderbolt/tb.c > > +++ b/drivers/thunderbolt/tb.c > > @@ -2598,6 +2598,17 @@ static int tb_start(struct tb *tb) > > tb_switch_tmu_enable(tb->root_switch); > > /* Full scan to discover devices added before the driver was loaded. */ > > tb_scan_switch(tb->root_switch); > > + /* > > + * Boot firmware might have created tunnels of its own. Since we cannot > > + * be sure they are usable for us, Tear them down and reset the ports > > + * to handle it as new hotplug for USB4 routers. > > + */ > > + if (tb_switch_is_usb4(tb->root_switch)) { > > + tb_switch_discover_tunnels(tb->root_switch, > > + &tcm->tunnel_list, false); > > Why this is needed? > > It should be enough, to do simply something like this: > > if (tb_switch_is_usb4(tb->root_switch)) > tb_switch_reset(tb->root_switch); Actually this needs to be done only for USB4 v1 routers since we already reset USB4 v2 hosts so something like: /* * Reset USB4 v1 host router to get rid of possible tunnels the * boot firmware created. This makes sure all the tunnels are * created by us and thus have known configuration. * * For USB4 v2 and beyond we do this in nhi_reset() using the * host router reset interface. */ if (usb4_switch_version(tb->root_switch) == 1) tb_switch_reset(tb->root_switch); (possibly add similar comment to the nhi_reset() to refer this one).
On Wed, Dec 13, 2023 at 08:18:06AM +0200, Mika Westerberg wrote: > On Wed, Dec 13, 2023 at 07:49:14AM +0200, Mika Westerberg wrote: > > On Wed, Dec 13, 2023 at 12:46:35AM +0530, Sanath S wrote: > > > Boot firmware might have created tunnels of its own. Since we cannot > > > be sure they are usable for us. Tear them down and reset the ports > > > to handle it as a new hotplug for USB3 routers. > > > > > > Suggested-by: Mario Limonciello <mario.limonciello@amd.com> > > > Signed-off-by: Sanath S <Sanath.S@amd.com> > > > --- > > > drivers/thunderbolt/tb.c | 11 +++++++++++ > > > 1 file changed, 11 insertions(+) > > > > > > diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c > > > index fd49f86e0353..febd0b6972e3 100644 > > > --- a/drivers/thunderbolt/tb.c > > > +++ b/drivers/thunderbolt/tb.c > > > @@ -2598,6 +2598,17 @@ static int tb_start(struct tb *tb) > > > tb_switch_tmu_enable(tb->root_switch); > > > /* Full scan to discover devices added before the driver was loaded. */ > > > tb_scan_switch(tb->root_switch); > > > + /* > > > + * Boot firmware might have created tunnels of its own. Since we cannot > > > + * be sure they are usable for us, Tear them down and reset the ports > > > + * to handle it as new hotplug for USB4 routers. > > > + */ > > > + if (tb_switch_is_usb4(tb->root_switch)) { > > > + tb_switch_discover_tunnels(tb->root_switch, > > > + &tcm->tunnel_list, false); > > > > Why this is needed? > > > > It should be enough, to do simply something like this: > > > > if (tb_switch_is_usb4(tb->root_switch)) > > tb_switch_reset(tb->root_switch); > > Actually this needs to be done only for USB4 v1 routers since we already > reset USB4 v2 hosts so something like: > > /* > * Reset USB4 v1 host router to get rid of possible tunnels the > * boot firmware created. This makes sure all the tunnels are > * created by us and thus have known configuration. > * > * For USB4 v2 and beyond we do this in nhi_reset() using the > * host router reset interface. > */ > if (usb4_switch_version(tb->root_switch) == 1) > tb_switch_reset(tb->root_switch); > > (possibly add similar comment to the nhi_reset() to refer this one). Oh, and would it be possible to tie this with the "host_reset" parameter too somehow? I guess it could be moved to "tb.c" and "tb.h" and then check it from nhi.c as already done and then here so this would become: if (host_reset && usb4_switch_version(tb->root_switch) == 1) tb_switch_reset(tb->root_switch); With the idea that the user has a "chicken bit" to disable this behaviour (and consistent one with USB4 v2). Feel free to make it look nicer though.
On 12/13/2023 11:53 AM, Mika Westerberg wrote: > On Wed, Dec 13, 2023 at 08:18:06AM +0200, Mika Westerberg wrote: >> On Wed, Dec 13, 2023 at 07:49:14AM +0200, Mika Westerberg wrote: >>> On Wed, Dec 13, 2023 at 12:46:35AM +0530, Sanath S wrote: >>>> Boot firmware might have created tunnels of its own. Since we cannot >>>> be sure they are usable for us. Tear them down and reset the ports >>>> to handle it as a new hotplug for USB3 routers. >>>> >>>> Suggested-by: Mario Limonciello <mario.limonciello@amd.com> >>>> Signed-off-by: Sanath S <Sanath.S@amd.com> >>>> --- >>>> drivers/thunderbolt/tb.c | 11 +++++++++++ >>>> 1 file changed, 11 insertions(+) >>>> >>>> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c >>>> index fd49f86e0353..febd0b6972e3 100644 >>>> --- a/drivers/thunderbolt/tb.c >>>> +++ b/drivers/thunderbolt/tb.c >>>> @@ -2598,6 +2598,17 @@ static int tb_start(struct tb *tb) >>>> tb_switch_tmu_enable(tb->root_switch); >>>> /* Full scan to discover devices added before the driver was loaded. */ >>>> tb_scan_switch(tb->root_switch); >>>> + /* >>>> + * Boot firmware might have created tunnels of its own. Since we cannot >>>> + * be sure they are usable for us, Tear them down and reset the ports >>>> + * to handle it as new hotplug for USB4 routers. >>>> + */ >>>> + if (tb_switch_is_usb4(tb->root_switch)) { >>>> + tb_switch_discover_tunnels(tb->root_switch, >>>> + &tcm->tunnel_list, false); >>> Why this is needed? >>> >>> It should be enough, to do simply something like this: >>> >>> if (tb_switch_is_usb4(tb->root_switch)) >>> tb_switch_reset(tb->root_switch); If we don't tear down of tunnels before performing the DPR, the PCIe enumeration is failing. PCIe link is not coming up after DPR. Below log is missing without performing path deactivation before performing DPR and hence PCIe enumeration is not initiated. [ 746.630865] pcieport 0000:00:03.1: pciehp: Slot(0-1): Card present [ 746.630885] pcieport 0000:00:03.1: pciehp: Slot(0-1): Link Up I think when we do a DPR, it internally does some handling with PCI Path Enable bit(PE). So, deactivation of PCIe path is necessary for DPR to work. >> Actually this needs to be done only for USB4 v1 routers since we already >> reset USB4 v2 hosts so something like: >> >> /* >> * Reset USB4 v1 host router to get rid of possible tunnels the >> * boot firmware created. This makes sure all the tunnels are >> * created by us and thus have known configuration. >> * >> * For USB4 v2 and beyond we do this in nhi_reset() using the >> * host router reset interface. >> */ >> if (usb4_switch_version(tb->root_switch) == 1) >> tb_switch_reset(tb->root_switch); >> >> (possibly add similar comment to the nhi_reset() to refer this one). > Oh, and would it be possible to tie this with the "host_reset" parameter > too somehow? I guess it could be moved to "tb.c" and "tb.h" and then > check it from nhi.c as already done and then here so this would become: > > if (host_reset && usb4_switch_version(tb->root_switch) == 1) > tb_switch_reset(tb->root_switch); Is host_reset necessary for USB4 v1 routers ? I did not use host_reset in this case. If its needed, then we have to modify to enable host_reset in nhi.c as well. > With the idea that the user has a "chicken bit" to disable this > behaviour (and consistent one with USB4 v2). Feel free to make it look > nicer though.
On Wed, Dec 13, 2023 at 04:04:57PM +0530, Sanath S wrote: > > On 12/13/2023 11:53 AM, Mika Westerberg wrote: > > On Wed, Dec 13, 2023 at 08:18:06AM +0200, Mika Westerberg wrote: > > > On Wed, Dec 13, 2023 at 07:49:14AM +0200, Mika Westerberg wrote: > > > > On Wed, Dec 13, 2023 at 12:46:35AM +0530, Sanath S wrote: > > > > > Boot firmware might have created tunnels of its own. Since we cannot > > > > > be sure they are usable for us. Tear them down and reset the ports > > > > > to handle it as a new hotplug for USB3 routers. > > > > > > > > > > Suggested-by: Mario Limonciello <mario.limonciello@amd.com> > > > > > Signed-off-by: Sanath S <Sanath.S@amd.com> > > > > > --- > > > > > drivers/thunderbolt/tb.c | 11 +++++++++++ > > > > > 1 file changed, 11 insertions(+) > > > > > > > > > > diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c > > > > > index fd49f86e0353..febd0b6972e3 100644 > > > > > --- a/drivers/thunderbolt/tb.c > > > > > +++ b/drivers/thunderbolt/tb.c > > > > > @@ -2598,6 +2598,17 @@ static int tb_start(struct tb *tb) > > > > > tb_switch_tmu_enable(tb->root_switch); > > > > > /* Full scan to discover devices added before the driver was loaded. */ > > > > > tb_scan_switch(tb->root_switch); > > > > > + /* > > > > > + * Boot firmware might have created tunnels of its own. Since we cannot > > > > > + * be sure they are usable for us, Tear them down and reset the ports > > > > > + * to handle it as new hotplug for USB4 routers. > > > > > + */ > > > > > + if (tb_switch_is_usb4(tb->root_switch)) { > > > > > + tb_switch_discover_tunnels(tb->root_switch, > > > > > + &tcm->tunnel_list, false); > > > > Why this is needed? > > > > > > > > It should be enough, to do simply something like this: > > > > > > > > if (tb_switch_is_usb4(tb->root_switch)) > > > > tb_switch_reset(tb->root_switch); > If we don't tear down of tunnels before performing the DPR, the PCIe > enumeration is failing. > > PCIe link is not coming up after DPR. Below log is missing without > performing path > deactivation before performing DPR and hence PCIe enumeration is not > initiated. > > [ 746.630865] pcieport 0000:00:03.1: pciehp: Slot(0-1): Card present > [ 746.630885] pcieport 0000:00:03.1: pciehp: Slot(0-1): Link Up > > I think when we do a DPR, it internally does some handling with PCI Path > Enable bit(PE). > So, deactivation of PCIe path is necessary for DPR to work. Rigth, it should be enough to reset the protocol adapter config and path config spaces. I guess using discovery at this point is fine too but I would at least check how complex doing the minimal "reset" turns out. I mean in tb_switch_reset() for USB4 v1 routers it can go over all the adapters and perform "cleanup" or so. > > > Actually this needs to be done only for USB4 v1 routers since we already > > > reset USB4 v2 hosts so something like: > > > > > > /* > > > * Reset USB4 v1 host router to get rid of possible tunnels the > > > * boot firmware created. This makes sure all the tunnels are > > > * created by us and thus have known configuration. > > > * > > > * For USB4 v2 and beyond we do this in nhi_reset() using the > > > * host router reset interface. > > > */ > > > if (usb4_switch_version(tb->root_switch) == 1) > > > tb_switch_reset(tb->root_switch); > > > > > > (possibly add similar comment to the nhi_reset() to refer this one). > > Oh, and would it be possible to tie this with the "host_reset" parameter > > too somehow? I guess it could be moved to "tb.c" and "tb.h" and then > > check it from nhi.c as already done and then here so this would become: > > > > if (host_reset && usb4_switch_version(tb->root_switch) == 1) > > tb_switch_reset(tb->root_switch); > > Is host_reset necessary for USB4 v1 routers ? I did not use host_reset in > this case. > If its needed, then we have to modify to enable host_reset in nhi.c as well. Well you are effectively doing that here, no? You "reset" the host router therefore tying this to the same command line parameter makes sense and allows user an "escape hatch" if this turns out breaking things.
On 12/13/2023 5:22 PM, Mika Westerberg wrote: > On Wed, Dec 13, 2023 at 04:04:57PM +0530, Sanath S wrote: >> On 12/13/2023 11:53 AM, Mika Westerberg wrote: >>> On Wed, Dec 13, 2023 at 08:18:06AM +0200, Mika Westerberg wrote: >>>> On Wed, Dec 13, 2023 at 07:49:14AM +0200, Mika Westerberg wrote: >>>>> On Wed, Dec 13, 2023 at 12:46:35AM +0530, Sanath S wrote: >>>>>> Boot firmware might have created tunnels of its own. Since we cannot >>>>>> be sure they are usable for us. Tear them down and reset the ports >>>>>> to handle it as a new hotplug for USB3 routers. >>>>>> >>>>>> Suggested-by: Mario Limonciello <mario.limonciello@amd.com> >>>>>> Signed-off-by: Sanath S <Sanath.S@amd.com> >>>>>> --- >>>>>> drivers/thunderbolt/tb.c | 11 +++++++++++ >>>>>> 1 file changed, 11 insertions(+) >>>>>> >>>>>> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c >>>>>> index fd49f86e0353..febd0b6972e3 100644 >>>>>> --- a/drivers/thunderbolt/tb.c >>>>>> +++ b/drivers/thunderbolt/tb.c >>>>>> @@ -2598,6 +2598,17 @@ static int tb_start(struct tb *tb) >>>>>> tb_switch_tmu_enable(tb->root_switch); >>>>>> /* Full scan to discover devices added before the driver was loaded. */ >>>>>> tb_scan_switch(tb->root_switch); >>>>>> + /* >>>>>> + * Boot firmware might have created tunnels of its own. Since we cannot >>>>>> + * be sure they are usable for us, Tear them down and reset the ports >>>>>> + * to handle it as new hotplug for USB4 routers. >>>>>> + */ >>>>>> + if (tb_switch_is_usb4(tb->root_switch)) { >>>>>> + tb_switch_discover_tunnels(tb->root_switch, >>>>>> + &tcm->tunnel_list, false); >>>>> Why this is needed? >>>>> >>>>> It should be enough, to do simply something like this: >>>>> >>>>> if (tb_switch_is_usb4(tb->root_switch)) >>>>> tb_switch_reset(tb->root_switch); >> If we don't tear down of tunnels before performing the DPR, the PCIe >> enumeration is failing. >> >> PCIe link is not coming up after DPR. Below log is missing without >> performing path >> deactivation before performing DPR and hence PCIe enumeration is not >> initiated. >> >> [ 746.630865] pcieport 0000:00:03.1: pciehp: Slot(0-1): Card present >> [ 746.630885] pcieport 0000:00:03.1: pciehp: Slot(0-1): Link Up >> >> I think when we do a DPR, it internally does some handling with PCI Path >> Enable bit(PE). >> So, deactivation of PCIe path is necessary for DPR to work. > Rigth, it should be enough to reset the protocol adapter config and path > config spaces. I guess using discovery at this point is fine too but I > would at least check how complex doing the minimal "reset" turns out. > > I mean in tb_switch_reset() for USB4 v1 routers it can go over all the > adapters and perform "cleanup" or so. I gave it a thought yesterday and we can do something like this: We are already doing tb_discovery(tb) in tb_start. This would discover the path configuration done by Boot firmware. Now, we can place the tb_switch_reset() right below that api with conditions suggested by you. And tb_switch_reset() would internally DPR for all down steam ports. It can look something like below: /* Find out tunnels created by the boot firmware */ tb_discover_tunnels(tb); /* * Reset USB4 v1 host router to get rid of possible tunnels the * boot firmware created. This makes sure all the tunnels are * created by us and thus have known configuration. * * For USB4 v2 and beyond we do this in nhi_reset() using the * host router reset interface. */ if (host_reset && usb4_switch_version(tb->root_switch) == 1) tb_switch_reset(tb->root_switch); With this, we are making sure while we get a unplug event after doing a DPR, We are clearing all the paths established by Boot firmware. This wouldn't be possible if we had not discovered the paths before we perform DPR. It would create inconsistency for a new hot plug if we have not cleared the path configurations of previous hot unplug events. >>>> Actually this needs to be done only for USB4 v1 routers since we already >>>> reset USB4 v2 hosts so something like: >>>> >>>> /* >>>> * Reset USB4 v1 host router to get rid of possible tunnels the >>>> * boot firmware created. This makes sure all the tunnels are >>>> * created by us and thus have known configuration. >>>> * >>>> * For USB4 v2 and beyond we do this in nhi_reset() using the >>>> * host router reset interface. >>>> */ >>>> if (usb4_switch_version(tb->root_switch) == 1) >>>> tb_switch_reset(tb->root_switch); >>>> >>>> (possibly add similar comment to the nhi_reset() to refer this one). >>> Oh, and would it be possible to tie this with the "host_reset" parameter >>> too somehow? I guess it could be moved to "tb.c" and "tb.h" and then >>> check it from nhi.c as already done and then here so this would become: >>> >>> if (host_reset && usb4_switch_version(tb->root_switch) == 1) >>> tb_switch_reset(tb->root_switch); >> Is host_reset necessary for USB4 v1 routers ? I did not use host_reset in >> this case. >> If its needed, then we have to modify to enable host_reset in nhi.c as well. > Well you are effectively doing that here, no? You "reset" the host > router therefore tying this to the same command line parameter makes > sense and allows user an "escape hatch" if this turns out breaking > things.
On Thu, Dec 14, 2023 at 12:08:34PM +0530, Sanath S wrote: > > On 12/13/2023 5:22 PM, Mika Westerberg wrote: > > On Wed, Dec 13, 2023 at 04:04:57PM +0530, Sanath S wrote: > > > On 12/13/2023 11:53 AM, Mika Westerberg wrote: > > > > On Wed, Dec 13, 2023 at 08:18:06AM +0200, Mika Westerberg wrote: > > > > > On Wed, Dec 13, 2023 at 07:49:14AM +0200, Mika Westerberg wrote: > > > > > > On Wed, Dec 13, 2023 at 12:46:35AM +0530, Sanath S wrote: > > > > > > > Boot firmware might have created tunnels of its own. Since we cannot > > > > > > > be sure they are usable for us. Tear them down and reset the ports > > > > > > > to handle it as a new hotplug for USB3 routers. > > > > > > > > > > > > > > Suggested-by: Mario Limonciello <mario.limonciello@amd.com> > > > > > > > Signed-off-by: Sanath S <Sanath.S@amd.com> > > > > > > > --- > > > > > > > drivers/thunderbolt/tb.c | 11 +++++++++++ > > > > > > > 1 file changed, 11 insertions(+) > > > > > > > > > > > > > > diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c > > > > > > > index fd49f86e0353..febd0b6972e3 100644 > > > > > > > --- a/drivers/thunderbolt/tb.c > > > > > > > +++ b/drivers/thunderbolt/tb.c > > > > > > > @@ -2598,6 +2598,17 @@ static int tb_start(struct tb *tb) > > > > > > > tb_switch_tmu_enable(tb->root_switch); > > > > > > > /* Full scan to discover devices added before the driver was loaded. */ > > > > > > > tb_scan_switch(tb->root_switch); > > > > > > > + /* > > > > > > > + * Boot firmware might have created tunnels of its own. Since we cannot > > > > > > > + * be sure they are usable for us, Tear them down and reset the ports > > > > > > > + * to handle it as new hotplug for USB4 routers. > > > > > > > + */ > > > > > > > + if (tb_switch_is_usb4(tb->root_switch)) { > > > > > > > + tb_switch_discover_tunnels(tb->root_switch, > > > > > > > + &tcm->tunnel_list, false); > > > > > > Why this is needed? > > > > > > > > > > > > It should be enough, to do simply something like this: > > > > > > > > > > > > if (tb_switch_is_usb4(tb->root_switch)) > > > > > > tb_switch_reset(tb->root_switch); > > > If we don't tear down of tunnels before performing the DPR, the PCIe > > > enumeration is failing. > > > > > > PCIe link is not coming up after DPR. Below log is missing without > > > performing path > > > deactivation before performing DPR and hence PCIe enumeration is not > > > initiated. > > > > > > [ 746.630865] pcieport 0000:00:03.1: pciehp: Slot(0-1): Card present > > > [ 746.630885] pcieport 0000:00:03.1: pciehp: Slot(0-1): Link Up > > > > > > I think when we do a DPR, it internally does some handling with PCI Path > > > Enable bit(PE). > > > So, deactivation of PCIe path is necessary for DPR to work. > > Rigth, it should be enough to reset the protocol adapter config and path > > config spaces. I guess using discovery at this point is fine too but I > > would at least check how complex doing the minimal "reset" turns out. > > > > I mean in tb_switch_reset() for USB4 v1 routers it can go over all the > > adapters and perform "cleanup" or so. > I gave it a thought yesterday and we can do something like this: > > We are already doing tb_discovery(tb) in tb_start. This would > discover the path configuration done by Boot firmware. > > Now, we can place the tb_switch_reset() right below that api with > conditions suggested by you. > > And tb_switch_reset() would internally DPR for all down steam ports. > > It can look something like below: > > /* Find out tunnels created by the boot firmware */ > tb_discover_tunnels(tb); > /* > * Reset USB4 v1 host router to get rid of possible tunnels the > * boot firmware created. This makes sure all the tunnels are > * created by us and thus have known configuration. > * > * For USB4 v2 and beyond we do this in nhi_reset() using the > * host router reset interface. > */ > if (host_reset && usb4_switch_version(tb->root_switch) == 1) > tb_switch_reset(tb->root_switch); > > With this, we are making sure while we get a unplug event after doing a DPR, > We are clearing all the paths established by Boot firmware. This wouldn't be > possible > if we had not discovered the paths before we perform DPR. > > It would create inconsistency for a new hot plug if we have not cleared the > path configurations > of previous hot unplug events. Right. I would still check if doing protocol adapter "reset" + path config space clear in tb_switch_reset() is enough and how complex that ends up to be. I think that's all what is needed. If it turns out too complex, yes I guess something like this: /* 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 (host_reset && usb4_switch_version(tb->root_switch) == 1) { struct tb_tunnel *n, *tunnel; list_for_each_entry_safe(tunnel, n, &tcm->tunnel_list, list) tb_deactivate_and_free_tunnel(tunnel); tb_switch_reset(tb->root_switch); } With proper comments would work, no? Regarding "host_reset", I think we can actually keep it in nhi.c and add a parameter to cm_ops->start(reset) that gets passed to the "CM implementation". Or something along those lines. > > > > > Actually this needs to be done only for USB4 v1 routers since we already > > > > > reset USB4 v2 hosts so something like: > > > > > > > > > > /* > > > > > * Reset USB4 v1 host router to get rid of possible tunnels the > > > > > * boot firmware created. This makes sure all the tunnels are > > > > > * created by us and thus have known configuration. > > > > > * > > > > > * For USB4 v2 and beyond we do this in nhi_reset() using the > > > > > * host router reset interface. > > > > > */ > > > > > if (usb4_switch_version(tb->root_switch) == 1) > > > > > tb_switch_reset(tb->root_switch); > > > > > > > > > > (possibly add similar comment to the nhi_reset() to refer this one). > > > > Oh, and would it be possible to tie this with the "host_reset" parameter > > > > too somehow? I guess it could be moved to "tb.c" and "tb.h" and then > > > > check it from nhi.c as already done and then here so this would become: > > > > > > > > if (host_reset && usb4_switch_version(tb->root_switch) == 1) > > > > tb_switch_reset(tb->root_switch); > > > Is host_reset necessary for USB4 v1 routers ? I did not use host_reset in > > > this case. > > > If its needed, then we have to modify to enable host_reset in nhi.c as well. > > Well you are effectively doing that here, no? You "reset" the host > > router therefore tying this to the same command line parameter makes > > sense and allows user an "escape hatch" if this turns out breaking > > things.
On 12/14/2023 12:37 PM, Mika Westerberg wrote: > On Thu, Dec 14, 2023 at 12:08:34PM +0530, Sanath S wrote: >> On 12/13/2023 5:22 PM, Mika Westerberg wrote: >>> On Wed, Dec 13, 2023 at 04:04:57PM +0530, Sanath S wrote: >>>> On 12/13/2023 11:53 AM, Mika Westerberg wrote: >>>>> On Wed, Dec 13, 2023 at 08:18:06AM +0200, Mika Westerberg wrote: >>>>>> On Wed, Dec 13, 2023 at 07:49:14AM +0200, Mika Westerberg wrote: >>>>>>> On Wed, Dec 13, 2023 at 12:46:35AM +0530, Sanath S wrote: >>>>>>>> Boot firmware might have created tunnels of its own. Since we cannot >>>>>>>> be sure they are usable for us. Tear them down and reset the ports >>>>>>>> to handle it as a new hotplug for USB3 routers. >>>>>>>> >>>>>>>> Suggested-by: Mario Limonciello <mario.limonciello@amd.com> >>>>>>>> Signed-off-by: Sanath S <Sanath.S@amd.com> >>>>>>>> --- >>>>>>>> drivers/thunderbolt/tb.c | 11 +++++++++++ >>>>>>>> 1 file changed, 11 insertions(+) >>>>>>>> >>>>>>>> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c >>>>>>>> index fd49f86e0353..febd0b6972e3 100644 >>>>>>>> --- a/drivers/thunderbolt/tb.c >>>>>>>> +++ b/drivers/thunderbolt/tb.c >>>>>>>> @@ -2598,6 +2598,17 @@ static int tb_start(struct tb *tb) >>>>>>>> tb_switch_tmu_enable(tb->root_switch); >>>>>>>> /* Full scan to discover devices added before the driver was loaded. */ >>>>>>>> tb_scan_switch(tb->root_switch); >>>>>>>> + /* >>>>>>>> + * Boot firmware might have created tunnels of its own. Since we cannot >>>>>>>> + * be sure they are usable for us, Tear them down and reset the ports >>>>>>>> + * to handle it as new hotplug for USB4 routers. >>>>>>>> + */ >>>>>>>> + if (tb_switch_is_usb4(tb->root_switch)) { >>>>>>>> + tb_switch_discover_tunnels(tb->root_switch, >>>>>>>> + &tcm->tunnel_list, false); >>>>>>> Why this is needed? >>>>>>> >>>>>>> It should be enough, to do simply something like this: >>>>>>> >>>>>>> if (tb_switch_is_usb4(tb->root_switch)) >>>>>>> tb_switch_reset(tb->root_switch); >>>> If we don't tear down of tunnels before performing the DPR, the PCIe >>>> enumeration is failing. >>>> >>>> PCIe link is not coming up after DPR. Below log is missing without >>>> performing path >>>> deactivation before performing DPR and hence PCIe enumeration is not >>>> initiated. >>>> >>>> [ 746.630865] pcieport 0000:00:03.1: pciehp: Slot(0-1): Card present >>>> [ 746.630885] pcieport 0000:00:03.1: pciehp: Slot(0-1): Link Up >>>> >>>> I think when we do a DPR, it internally does some handling with PCI Path >>>> Enable bit(PE). >>>> So, deactivation of PCIe path is necessary for DPR to work. >>> Rigth, it should be enough to reset the protocol adapter config and path >>> config spaces. I guess using discovery at this point is fine too but I >>> would at least check how complex doing the minimal "reset" turns out. >>> >>> I mean in tb_switch_reset() for USB4 v1 routers it can go over all the >>> adapters and perform "cleanup" or so. >> I gave it a thought yesterday and we can do something like this: >> >> We are already doing tb_discovery(tb) in tb_start. This would >> discover the path configuration done by Boot firmware. >> >> Now, we can place the tb_switch_reset() right below that api with >> conditions suggested by you. >> >> And tb_switch_reset() would internally DPR for all down steam ports. >> >> It can look something like below: >> >> /* Find out tunnels created by the boot firmware */ >> tb_discover_tunnels(tb); >> /* >> * Reset USB4 v1 host router to get rid of possible tunnels the >> * boot firmware created. This makes sure all the tunnels are >> * created by us and thus have known configuration. >> * >> * For USB4 v2 and beyond we do this in nhi_reset() using the >> * host router reset interface. >> */ >> if (host_reset && usb4_switch_version(tb->root_switch) == 1) >> tb_switch_reset(tb->root_switch); >> >> With this, we are making sure while we get a unplug event after doing a DPR, >> We are clearing all the paths established by Boot firmware. This wouldn't be >> possible >> if we had not discovered the paths before we perform DPR. >> >> It would create inconsistency for a new hot plug if we have not cleared the >> path configurations >> of previous hot unplug events. > Right. I would still check if doing protocol adapter "reset" + path > config space clear in tb_switch_reset() is enough and how complex that > ends up to be. I think that's all what is needed. > > If it turns out too complex, yes I guess something like this: > > /* 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 (host_reset && usb4_switch_version(tb->root_switch) == 1) { > struct tb_tunnel *n, *tunnel; > > list_for_each_entry_safe(tunnel, n, &tcm->tunnel_list, list) > tb_deactivate_and_free_tunnel(tunnel); > > tb_switch_reset(tb->root_switch); > } > > With proper comments would work, no? Yes, this works. Tested it too and works fine. Probably we can move tb_deactivate_and_free_tunnel() inside tb_switch_reset() to make it look better. > Regarding "host_reset", I think we can actually keep it in nhi.c and add > a parameter to cm_ops->start(reset) that gets passed to the "CM > implementation". Or something along those lines. I will check along these lines. >>>>>> Actually this needs to be done only for USB4 v1 routers since we already >>>>>> reset USB4 v2 hosts so something like: >>>>>> >>>>>> /* >>>>>> * Reset USB4 v1 host router to get rid of possible tunnels the >>>>>> * boot firmware created. This makes sure all the tunnels are >>>>>> * created by us and thus have known configuration. >>>>>> * >>>>>> * For USB4 v2 and beyond we do this in nhi_reset() using the >>>>>> * host router reset interface. >>>>>> */ >>>>>> if (usb4_switch_version(tb->root_switch) == 1) >>>>>> tb_switch_reset(tb->root_switch); >>>>>> >>>>>> (possibly add similar comment to the nhi_reset() to refer this one). >>>>> Oh, and would it be possible to tie this with the "host_reset" parameter >>>>> too somehow? I guess it could be moved to "tb.c" and "tb.h" and then >>>>> check it from nhi.c as already done and then here so this would become: >>>>> >>>>> if (host_reset && usb4_switch_version(tb->root_switch) == 1) >>>>> tb_switch_reset(tb->root_switch); >>>> Is host_reset necessary for USB4 v1 routers ? I did not use host_reset in >>>> this case. >>>> If its needed, then we have to modify to enable host_reset in nhi.c as well. >>> Well you are effectively doing that here, no? You "reset" the host >>> router therefore tying this to the same command line parameter makes >>> sense and allows user an "escape hatch" if this turns out breaking >>> things.
On Thu, Dec 14, 2023 at 12:50:21PM +0530, Sanath S wrote: > > On 12/14/2023 12:37 PM, Mika Westerberg wrote: > > On Thu, Dec 14, 2023 at 12:08:34PM +0530, Sanath S wrote: > > > On 12/13/2023 5:22 PM, Mika Westerberg wrote: > > > > On Wed, Dec 13, 2023 at 04:04:57PM +0530, Sanath S wrote: > > > > > On 12/13/2023 11:53 AM, Mika Westerberg wrote: > > > > > > On Wed, Dec 13, 2023 at 08:18:06AM +0200, Mika Westerberg wrote: > > > > > > > On Wed, Dec 13, 2023 at 07:49:14AM +0200, Mika Westerberg wrote: > > > > > > > > On Wed, Dec 13, 2023 at 12:46:35AM +0530, Sanath S wrote: > > > > > > > > > Boot firmware might have created tunnels of its own. Since we cannot > > > > > > > > > be sure they are usable for us. Tear them down and reset the ports > > > > > > > > > to handle it as a new hotplug for USB3 routers. > > > > > > > > > > > > > > > > > > Suggested-by: Mario Limonciello <mario.limonciello@amd.com> > > > > > > > > > Signed-off-by: Sanath S <Sanath.S@amd.com> > > > > > > > > > --- > > > > > > > > > drivers/thunderbolt/tb.c | 11 +++++++++++ > > > > > > > > > 1 file changed, 11 insertions(+) > > > > > > > > > > > > > > > > > > diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c > > > > > > > > > index fd49f86e0353..febd0b6972e3 100644 > > > > > > > > > --- a/drivers/thunderbolt/tb.c > > > > > > > > > +++ b/drivers/thunderbolt/tb.c > > > > > > > > > @@ -2598,6 +2598,17 @@ static int tb_start(struct tb *tb) > > > > > > > > > tb_switch_tmu_enable(tb->root_switch); > > > > > > > > > /* Full scan to discover devices added before the driver was loaded. */ > > > > > > > > > tb_scan_switch(tb->root_switch); > > > > > > > > > + /* > > > > > > > > > + * Boot firmware might have created tunnels of its own. Since we cannot > > > > > > > > > + * be sure they are usable for us, Tear them down and reset the ports > > > > > > > > > + * to handle it as new hotplug for USB4 routers. > > > > > > > > > + */ > > > > > > > > > + if (tb_switch_is_usb4(tb->root_switch)) { > > > > > > > > > + tb_switch_discover_tunnels(tb->root_switch, > > > > > > > > > + &tcm->tunnel_list, false); > > > > > > > > Why this is needed? > > > > > > > > > > > > > > > > It should be enough, to do simply something like this: > > > > > > > > > > > > > > > > if (tb_switch_is_usb4(tb->root_switch)) > > > > > > > > tb_switch_reset(tb->root_switch); > > > > > If we don't tear down of tunnels before performing the DPR, the PCIe > > > > > enumeration is failing. > > > > > > > > > > PCIe link is not coming up after DPR. Below log is missing without > > > > > performing path > > > > > deactivation before performing DPR and hence PCIe enumeration is not > > > > > initiated. > > > > > > > > > > [ 746.630865] pcieport 0000:00:03.1: pciehp: Slot(0-1): Card present > > > > > [ 746.630885] pcieport 0000:00:03.1: pciehp: Slot(0-1): Link Up > > > > > > > > > > I think when we do a DPR, it internally does some handling with PCI Path > > > > > Enable bit(PE). > > > > > So, deactivation of PCIe path is necessary for DPR to work. > > > > Rigth, it should be enough to reset the protocol adapter config and path > > > > config spaces. I guess using discovery at this point is fine too but I > > > > would at least check how complex doing the minimal "reset" turns out. > > > > > > > > I mean in tb_switch_reset() for USB4 v1 routers it can go over all the > > > > adapters and perform "cleanup" or so. > > > I gave it a thought yesterday and we can do something like this: > > > > > > We are already doing tb_discovery(tb) in tb_start. This would > > > discover the path configuration done by Boot firmware. > > > > > > Now, we can place the tb_switch_reset() right below that api with > > > conditions suggested by you. > > > > > > And tb_switch_reset() would internally DPR for all down steam ports. > > > > > > It can look something like below: > > > > > > /* Find out tunnels created by the boot firmware */ > > > tb_discover_tunnels(tb); > > > /* > > > * Reset USB4 v1 host router to get rid of possible tunnels the > > > * boot firmware created. This makes sure all the tunnels are > > > * created by us and thus have known configuration. > > > * > > > * For USB4 v2 and beyond we do this in nhi_reset() using the > > > * host router reset interface. > > > */ > > > if (host_reset && usb4_switch_version(tb->root_switch) == 1) > > > tb_switch_reset(tb->root_switch); > > > > > > With this, we are making sure while we get a unplug event after doing a DPR, > > > We are clearing all the paths established by Boot firmware. This wouldn't be > > > possible > > > if we had not discovered the paths before we perform DPR. > > > > > > It would create inconsistency for a new hot plug if we have not cleared the > > > path configurations > > > of previous hot unplug events. > > Right. I would still check if doing protocol adapter "reset" + path > > config space clear in tb_switch_reset() is enough and how complex that > > ends up to be. I think that's all what is needed. > > > > If it turns out too complex, yes I guess something like this: > > > > /* 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 (host_reset && usb4_switch_version(tb->root_switch) == 1) { > > struct tb_tunnel *n, *tunnel; > > > > list_for_each_entry_safe(tunnel, n, &tcm->tunnel_list, list) > > tb_deactivate_and_free_tunnel(tunnel); > > > > tb_switch_reset(tb->root_switch); > > } > > > > With proper comments would work, no? > Yes, this works. Tested it too and works fine. Cool. > Probably we can move tb_deactivate_and_free_tunnel() inside > tb_switch_reset() to make it > look better. 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.
On 12/14/2023 1:02 PM, Mika Westerberg wrote: > On Thu, Dec 14, 2023 at 12:50:21PM +0530, Sanath S wrote: >> On 12/14/2023 12:37 PM, Mika Westerberg wrote: >>> On Thu, Dec 14, 2023 at 12:08:34PM +0530, Sanath S wrote: >>>> On 12/13/2023 5:22 PM, Mika Westerberg wrote: >>>>> On Wed, Dec 13, 2023 at 04:04:57PM +0530, Sanath S wrote: >>>>>> On 12/13/2023 11:53 AM, Mika Westerberg wrote: >>>>>>> On Wed, Dec 13, 2023 at 08:18:06AM +0200, Mika Westerberg wrote: >>>>>>>> On Wed, Dec 13, 2023 at 07:49:14AM +0200, Mika Westerberg wrote: >>>>>>>>> On Wed, Dec 13, 2023 at 12:46:35AM +0530, Sanath S wrote: >>>>>>>>>> Boot firmware might have created tunnels of its own. Since we cannot >>>>>>>>>> be sure they are usable for us. Tear them down and reset the ports >>>>>>>>>> to handle it as a new hotplug for USB3 routers. >>>>>>>>>> >>>>>>>>>> Suggested-by: Mario Limonciello <mario.limonciello@amd.com> >>>>>>>>>> Signed-off-by: Sanath S <Sanath.S@amd.com> >>>>>>>>>> --- >>>>>>>>>> drivers/thunderbolt/tb.c | 11 +++++++++++ >>>>>>>>>> 1 file changed, 11 insertions(+) >>>>>>>>>> >>>>>>>>>> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c >>>>>>>>>> index fd49f86e0353..febd0b6972e3 100644 >>>>>>>>>> --- a/drivers/thunderbolt/tb.c >>>>>>>>>> +++ b/drivers/thunderbolt/tb.c >>>>>>>>>> @@ -2598,6 +2598,17 @@ static int tb_start(struct tb *tb) >>>>>>>>>> tb_switch_tmu_enable(tb->root_switch); >>>>>>>>>> /* Full scan to discover devices added before the driver was loaded. */ >>>>>>>>>> tb_scan_switch(tb->root_switch); >>>>>>>>>> + /* >>>>>>>>>> + * Boot firmware might have created tunnels of its own. Since we cannot >>>>>>>>>> + * be sure they are usable for us, Tear them down and reset the ports >>>>>>>>>> + * to handle it as new hotplug for USB4 routers. >>>>>>>>>> + */ >>>>>>>>>> + if (tb_switch_is_usb4(tb->root_switch)) { >>>>>>>>>> + tb_switch_discover_tunnels(tb->root_switch, >>>>>>>>>> + &tcm->tunnel_list, false); >>>>>>>>> Why this is needed? >>>>>>>>> >>>>>>>>> It should be enough, to do simply something like this: >>>>>>>>> >>>>>>>>> if (tb_switch_is_usb4(tb->root_switch)) >>>>>>>>> tb_switch_reset(tb->root_switch); >>>>>> If we don't tear down of tunnels before performing the DPR, the PCIe >>>>>> enumeration is failing. >>>>>> >>>>>> PCIe link is not coming up after DPR. Below log is missing without >>>>>> performing path >>>>>> deactivation before performing DPR and hence PCIe enumeration is not >>>>>> initiated. >>>>>> >>>>>> [ 746.630865] pcieport 0000:00:03.1: pciehp: Slot(0-1): Card present >>>>>> [ 746.630885] pcieport 0000:00:03.1: pciehp: Slot(0-1): Link Up >>>>>> >>>>>> I think when we do a DPR, it internally does some handling with PCI Path >>>>>> Enable bit(PE). >>>>>> So, deactivation of PCIe path is necessary for DPR to work. >>>>> Rigth, it should be enough to reset the protocol adapter config and path >>>>> config spaces. I guess using discovery at this point is fine too but I >>>>> would at least check how complex doing the minimal "reset" turns out. >>>>> >>>>> I mean in tb_switch_reset() for USB4 v1 routers it can go over all the >>>>> adapters and perform "cleanup" or so. >>>> I gave it a thought yesterday and we can do something like this: >>>> >>>> We are already doing tb_discovery(tb) in tb_start. This would >>>> discover the path configuration done by Boot firmware. >>>> >>>> Now, we can place the tb_switch_reset() right below that api with >>>> conditions suggested by you. >>>> >>>> And tb_switch_reset() would internally DPR for all down steam ports. >>>> >>>> It can look something like below: >>>> >>>> /* Find out tunnels created by the boot firmware */ >>>> tb_discover_tunnels(tb); >>>> /* >>>> * Reset USB4 v1 host router to get rid of possible tunnels the >>>> * boot firmware created. This makes sure all the tunnels are >>>> * created by us and thus have known configuration. >>>> * >>>> * For USB4 v2 and beyond we do this in nhi_reset() using the >>>> * host router reset interface. >>>> */ >>>> if (host_reset && usb4_switch_version(tb->root_switch) == 1) >>>> tb_switch_reset(tb->root_switch); >>>> >>>> With this, we are making sure while we get a unplug event after doing a DPR, >>>> We are clearing all the paths established by Boot firmware. This wouldn't be >>>> possible >>>> if we had not discovered the paths before we perform DPR. >>>> >>>> It would create inconsistency for a new hot plug if we have not cleared the >>>> path configurations >>>> of previous hot unplug events. >>> Right. I would still check if doing protocol adapter "reset" + path >>> config space clear in tb_switch_reset() is enough and how complex that >>> ends up to be. I think that's all what is needed. >>> >>> If it turns out too complex, yes I guess something like this: >>> >>> /* 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 (host_reset && usb4_switch_version(tb->root_switch) == 1) { >>> struct tb_tunnel *n, *tunnel; >>> >>> list_for_each_entry_safe(tunnel, n, &tcm->tunnel_list, list) >>> tb_deactivate_and_free_tunnel(tunnel); >>> >>> tb_switch_reset(tb->root_switch); >>> } >>> >>> With proper comments would work, no? >> Yes, this works. Tested it too and works fine. > Cool. > >> Probably we can move tb_deactivate_and_free_tunnel() inside >> tb_switch_reset() to make it >> look better. > 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 :)
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). 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; + } 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); + } 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; + } + } 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; + + 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;
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. 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) 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. > + } 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); > + } 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. > } 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; > + > + 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; > >
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. > 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'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); > > + } 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; > > + > > + 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; > > > >
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; >>> >>>
On Mon, Dec 18, 2023 at 03:50:26PM +0530, Sanath S wrote: > > 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. The discover part should not do anything (like write the hardware) so perhaps it is just some timing thing (but that's weird too). I think we should do something like this: 1. Disable all enabled protocol adapters (reset them to defaults). 2. Clear all protocol adapter paths. 3. Issue DPR over all enabled USB4 ports. BTW, what you mean "didn't work"? > > > 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. Right makes sense. > > > > + } 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. Agree. > > > > + 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; > > > > > > > >
On 12/18/2023 4:12 PM, Mika Westerberg wrote: > On Mon, Dec 18, 2023 at 03:50:26PM +0530, Sanath S wrote: >> 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. > The discover part should not do anything (like write the hardware) so > perhaps it is just some timing thing (but that's weird too). > > I think we should do something like this: > > 1. Disable all enabled protocol adapters (reset them to defaults). > 2. Clear all protocol adapter paths. > 3. Issue DPR over all enabled USB4 ports. > > BTW, what you mean "didn't work"? Path activation would go fine after DPR like below: [ 15.090905] thunderbolt 0000:c4:00.5: 0:5 <-> 2:9 (PCI): activating [ 15.090932] thunderbolt 0000:c4:00.5: activating PCIe Down path from 0:5 to 2:9 [ 15.091602] thunderbolt 0000:c4:00.5: activating PCIe Up path from 2:9 to 0:5 But, PCIE enumeration doesn't happen (pcie link up will not happen, will not see below logs) [ 15.134223] pcieport 0000:00:03.1: pciehp: Slot(0-1): Card present [ 15.134243] pcieport 0000:00:03.1: pciehp: Slot(0-1): Link Up >>>> 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. > Right makes sense. We never get up_port of protocol adapters here for reset. It's always down_port. So probably when we discover the path, we do path deactivation for both down and up ports. >>>>> + } 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. > Agree. > >>>>> + 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; >>>>> >>>>>
On Mon, Dec 18, 2023 at 04:49:13PM +0530, Sanath S wrote: > > The discover part should not do anything (like write the hardware) so > > perhaps it is just some timing thing (but that's weird too). > > > > I think we should do something like this: > > > > 1. Disable all enabled protocol adapters (reset them to defaults). > > 2. Clear all protocol adapter paths. > > 3. Issue DPR over all enabled USB4 ports. > > > > BTW, what you mean "didn't work"? > Path activation would go fine after DPR like below: > > [ 15.090905] thunderbolt 0000:c4:00.5: 0:5 <-> 2:9 (PCI): activating > [ 15.090932] thunderbolt 0000:c4:00.5: activating PCIe Down path from 0:5 > to 2:9 > [ 15.091602] thunderbolt 0000:c4:00.5: activating PCIe Up path from 2:9 to > 0:5 > > But, PCIE enumeration doesn't happen (pcie link up will not happen, will not > see below logs) > [ 15.134223] pcieport 0000:00:03.1: pciehp: Slot(0-1): Card present > [ 15.134243] pcieport 0000:00:03.1: pciehp: Slot(0-1): Link Up Okay, what if you like reset the PCIe adapter config spaces back to the defaults? Just as an experiment. > > > > > > + } 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. > > Right makes sense. > We never get up_port of protocol adapters here for reset. It's always > down_port. > So probably when we discover the path, we do path deactivation for both down > and up ports. If we are going to do DPR anyway, it should not matter.
On Mon, Dec 18, 2023 at 01:31:51PM +0200, Mika Westerberg wrote: > On Mon, Dec 18, 2023 at 04:49:13PM +0530, Sanath S wrote: > > > The discover part should not do anything (like write the hardware) so > > > perhaps it is just some timing thing (but that's weird too). > > > > > > I think we should do something like this: > > > > > > 1. Disable all enabled protocol adapters (reset them to defaults). > > > 2. Clear all protocol adapter paths. > > > 3. Issue DPR over all enabled USB4 ports. > > > > > > BTW, what you mean "didn't work"? > > Path activation would go fine after DPR like below: > > > > [ 15.090905] thunderbolt 0000:c4:00.5: 0:5 <-> 2:9 (PCI): activating > > [ 15.090932] thunderbolt 0000:c4:00.5: activating PCIe Down path from 0:5 > > to 2:9 > > [ 15.091602] thunderbolt 0000:c4:00.5: activating PCIe Up path from 2:9 to > > 0:5 > > > > But, PCIE enumeration doesn't happen (pcie link up will not happen, will not > > see below logs) > > [ 15.134223] pcieport 0000:00:03.1: pciehp: Slot(0-1): Card present > > [ 15.134243] pcieport 0000:00:03.1: pciehp: Slot(0-1): Link Up > > Okay, what if you like reset the PCIe adapter config spaces back to the > defaults? Just as an experiment. If this turns out to be really complex then I guess it is better to do it like you did originally using discovery but at least it would be nice to see what the end result of this experiment looks like :)
On 12/18/2023 5:53 PM, Mika Westerberg wrote: > On Mon, Dec 18, 2023 at 01:31:51PM +0200, Mika Westerberg wrote: >> On Mon, Dec 18, 2023 at 04:49:13PM +0530, Sanath S wrote: >>>> The discover part should not do anything (like write the hardware) so >>>> perhaps it is just some timing thing (but that's weird too). >>>> >>>> I think we should do something like this: >>>> >>>> 1. Disable all enabled protocol adapters (reset them to defaults). >>>> 2. Clear all protocol adapter paths. >>>> 3. Issue DPR over all enabled USB4 ports. >>>> >>>> BTW, what you mean "didn't work"? >>> Path activation would go fine after DPR like below: >>> >>> [ 15.090905] thunderbolt 0000:c4:00.5: 0:5 <-> 2:9 (PCI): activating >>> [ 15.090932] thunderbolt 0000:c4:00.5: activating PCIe Down path from 0:5 >>> to 2:9 >>> [ 15.091602] thunderbolt 0000:c4:00.5: activating PCIe Up path from 2:9 to >>> 0:5 >>> >>> But, PCIE enumeration doesn't happen (pcie link up will not happen, will not >>> see below logs) >>> [ 15.134223] pcieport 0000:00:03.1: pciehp: Slot(0-1): Card present >>> [ 15.134243] pcieport 0000:00:03.1: pciehp: Slot(0-1): Link Up >> Okay, what if you like reset the PCIe adapter config spaces back to the >> defaults? Just as an experiment. > If this turns out to be really complex then I guess it is better to do > it like you did originally using discovery but at least it would be nice > to see what the end result of this experiment looks like :) Yes, I'll give a try. As an experiment, I tried to compare the path deactivation that occurs at two place. 1. In tb_switch_reset where we are calling tb_path_deactivate_hop(port, i). 2. While we get a unplug event after doing DPR. I observed both have different hop_index and port numbers. So, are we calling tb_path_deactivate_hop with wrong hop ids ? From deactivate tunnel (called while unplug) : [ 3.408268] thunderbolt 0000:c4:00.5: deactivating PCIe Down path from 2:9 to 0:5 [ 3.408282] deactivate hop port = 9 hop_index=8 [ 3.408328] deactivate hop port = 2 hop_index=10 Deactivate from tb_switch_reset() : deactivate hop port = 5 hop_index=8
On Mon, Dec 18, 2023 at 06:35:13PM +0530, Sanath S wrote: > > On 12/18/2023 5:53 PM, Mika Westerberg wrote: > > On Mon, Dec 18, 2023 at 01:31:51PM +0200, Mika Westerberg wrote: > > > On Mon, Dec 18, 2023 at 04:49:13PM +0530, Sanath S wrote: > > > > > The discover part should not do anything (like write the hardware) so > > > > > perhaps it is just some timing thing (but that's weird too). > > > > > > > > > > I think we should do something like this: > > > > > > > > > > 1. Disable all enabled protocol adapters (reset them to defaults). > > > > > 2. Clear all protocol adapter paths. > > > > > 3. Issue DPR over all enabled USB4 ports. > > > > > > > > > > BTW, what you mean "didn't work"? > > > > Path activation would go fine after DPR like below: > > > > > > > > [ 15.090905] thunderbolt 0000:c4:00.5: 0:5 <-> 2:9 (PCI): activating > > > > [ 15.090932] thunderbolt 0000:c4:00.5: activating PCIe Down path from 0:5 > > > > to 2:9 > > > > [ 15.091602] thunderbolt 0000:c4:00.5: activating PCIe Up path from 2:9 to > > > > 0:5 > > > > > > > > But, PCIE enumeration doesn't happen (pcie link up will not happen, will not > > > > see below logs) > > > > [ 15.134223] pcieport 0000:00:03.1: pciehp: Slot(0-1): Card present > > > > [ 15.134243] pcieport 0000:00:03.1: pciehp: Slot(0-1): Link Up > > > Okay, what if you like reset the PCIe adapter config spaces back to the > > > defaults? Just as an experiment. > > If this turns out to be really complex then I guess it is better to do > > it like you did originally using discovery but at least it would be nice > > to see what the end result of this experiment looks like :) > > Yes, I'll give a try. > As an experiment, I tried to compare the path deactivation that occurs at > two place. > 1. In tb_switch_reset where we are calling tb_path_deactivate_hop(port, i). > 2. While we get a unplug event after doing DPR. > > I observed both have different hop_index and port numbers. > So, are we calling tb_path_deactivate_hop with wrong hop ids ? Wrong adapters possibly. > From deactivate tunnel (called while unplug) : > [ 3.408268] thunderbolt 0000:c4:00.5: deactivating PCIe Down path from > 2:9 to 0:5 > [ 3.408282] deactivate hop port = 9 hop_index=8 > [ 3.408328] deactivate hop port = 2 hop_index=10 Definitely should be port = 5 (that's PCIe down in your log) and hop_index = 8 (that's the one used with PCIe). > Deactivate from tb_switch_reset() : > deactivate hop port = 5 hop_index=8 Can you add some more logging and provide me the dmesg or alternativively investigate it yourself. You can use tb_port_dbg() to get the port numbers to the log.
On 12/18/2023 6:48 PM, Mika Westerberg wrote: > On Mon, Dec 18, 2023 at 06:35:13PM +0530, Sanath S wrote: >> On 12/18/2023 5:53 PM, Mika Westerberg wrote: >>> On Mon, Dec 18, 2023 at 01:31:51PM +0200, Mika Westerberg wrote: >>>> On Mon, Dec 18, 2023 at 04:49:13PM +0530, Sanath S wrote: >>>>>> The discover part should not do anything (like write the hardware) so >>>>>> perhaps it is just some timing thing (but that's weird too). >>>>>> >>>>>> I think we should do something like this: >>>>>> >>>>>> 1. Disable all enabled protocol adapters (reset them to defaults). >>>>>> 2. Clear all protocol adapter paths. >>>>>> 3. Issue DPR over all enabled USB4 ports. >>>>>> >>>>>> BTW, what you mean "didn't work"? >>>>> Path activation would go fine after DPR like below: >>>>> >>>>> [ 15.090905] thunderbolt 0000:c4:00.5: 0:5 <-> 2:9 (PCI): activating >>>>> [ 15.090932] thunderbolt 0000:c4:00.5: activating PCIe Down path from 0:5 >>>>> to 2:9 >>>>> [ 15.091602] thunderbolt 0000:c4:00.5: activating PCIe Up path from 2:9 to >>>>> 0:5 >>>>> >>>>> But, PCIE enumeration doesn't happen (pcie link up will not happen, will not >>>>> see below logs) >>>>> [ 15.134223] pcieport 0000:00:03.1: pciehp: Slot(0-1): Card present >>>>> [ 15.134243] pcieport 0000:00:03.1: pciehp: Slot(0-1): Link Up >>>> Okay, what if you like reset the PCIe adapter config spaces back to the >>>> defaults? Just as an experiment. >>> If this turns out to be really complex then I guess it is better to do >>> it like you did originally using discovery but at least it would be nice >>> to see what the end result of this experiment looks like :) I feel it's better to go with discover and then reset for now (as v3). I'll keep this experiment as "to do" and will send out when I crack it down. >> Yes, I'll give a try. >> As an experiment, I tried to compare the path deactivation that occurs at >> two place. >> 1. In tb_switch_reset where we are calling tb_path_deactivate_hop(port, i). >> 2. While we get a unplug event after doing DPR. >> >> I observed both have different hop_index and port numbers. >> So, are we calling tb_path_deactivate_hop with wrong hop ids ? > Wrong adapters possibly. > >> From deactivate tunnel (called while unplug) : >> [ 3.408268] thunderbolt 0000:c4:00.5: deactivating PCIe Down path from >> 2:9 to 0:5 >> [ 3.408282] deactivate hop port = 9 hop_index=8 >> [ 3.408328] deactivate hop port = 2 hop_index=10 > Definitely should be port = 5 (that's PCIe down in your log) and > hop_index = 8 (that's the one used with PCIe). > >> Deactivate from tb_switch_reset() : >> deactivate hop port = 5 hop_index=8 > Can you add some more logging and provide me the dmesg or > alternativively investigate it yourself. You can use tb_port_dbg() to > get the port numbers to the log. I've sent you complete dmesg. Here is the log w.r.t port numbers and path clean up. [ 3.389038] thunderbolt 0000:c4:00.5: 0:3: Downstream port, setting DPR [ 3.389065] Calling usb4_port_reset [ 3.389068] thunderbolt 0000:c4:00.5: 0:4: Found USB3 DOWN [ 3.389193] thunderbolt 0000:c4:00.5: 0:4: In reset, cleaning up path, port->port = 4 hopid = 8 [ 3.389203] thunderbolt 0000:c4:00.5: 0:4: deactivating_hop port = 4 hop_index=8 [ 3.389682] thunderbolt 0000:c4:00.5: 0:5: Found PCI Down [ 3.389811] thunderbolt 0000:c4:00.5: 0:5: In reset, cleaning up path, port->port = 5 hopid = 8 [ 3.389817] thunderbolt 0000:c4:00.5: 0:5: deactivating_hop port = 5 hop_index=8 [ 3.390296] thunderbolt 0000:c4:00.5: 0:6: Found DP IN [ 3.390555] thunderbolt 0000:c4:00.5: 0:6: In reset, cleaning up path, port->port = 6 hopid = 8 [ 3.390558] thunderbolt 0000:c4:00.5: 0:6: deactivating_hop port = 6 hop_index=8 [ 3.390686] thunderbolt 0000:c4:00.5: 0:6: In reset, cleaning up path, port->port = 6 hopid = 9 [ 3.390689] thunderbolt 0000:c4:00.5: 0:6: deactivating_hop port = 6 hop_index=9 [ 3.390816] thunderbolt 0000:c4:00.5: 0:7: Found DP IN [ 3.391077] thunderbolt 0000:c4:00.5: 0:7: In reset, cleaning up path, port->port = 7 hopid = 8 [ 3.391080] thunderbolt 0000:c4:00.5: 0:7: deactivating_hop port = 7 hop_index=8 [ 3.391213] thunderbolt 0000:c4:00.5: 0:7: In reset, cleaning up path, port->port = 7 hopid = 9 [ 3.391217] thunderbolt 0000:c4:00.5: 0:7: deactivating_hop port = 7 hop_index=9 [ 3.391342] Reset success [ 3.391391] thunderbolt 0000:c4:00.5: 0:2: switch unplugged [ 3.391434] thunderbolt 0000:c4:00.5: 0:4 <-> 2:16 (USB3): deactivating [ 3.391471] thunderbolt 0000:c4:00.5: deactivating USB3 Down path from 0:4 to 2:16 [ 3.391477] thunderbolt 0000:c4:00.5: 0:4: deactivating_hop port = 4 hop_index=8 [ 3.391641] thunderbolt 0000:c4:00.5: 2:1: deactivating_hop port = 1 hop_index=9 [ 3.391651] thunderbolt 0000:c4:00.5: deactivating USB3 Up path from 2:16 to 0:4 [ 3.391659] thunderbolt 0000:c4:00.5: 2:16: deactivating_hop port = 16 hop_index=8 [ 3.391664] thunderbolt 0000:c4:00.5: 0:2: deactivating_hop port = 2 hop_index=9 [ 3.391701] thunderbolt 0000:c4:00.6: total paths: 3 [ 3.391720] thunderbolt 0000:c4:00.6: IOMMU DMA protection is disabled [ 3.392027] thunderbolt 0000:c4:00.5: 0:5 <-> 2:9 (PCI): deactivating [ 3.392154] thunderbolt 0000:c4:00.5: deactivating PCIe Down path from 2:9 to 0:5 [ 3.392163] thunderbolt 0000:c4:00.5: 2:9: deactivating_hop port = 9 hop_index=8 [ 3.392170] thunderbolt 0000:c4:00.5: 0:2: deactivating_hop port = 2 hop_index=10 [ 3.392534] thunderbolt 0000:c4:00.5: deactivating PCIe Up path from 0:5 to 2:9 [ 3.392539] thunderbolt 0000:c4:00.5: 0:5: deactivating_hop port = 5 hop_index=8 [ 3.392637] thunderbolt 0000:c4:00.5: 2:1: deactivating_hop port = 1 hop_index=10 [ 3.392799] thunderbolt 0-2: device disconnected But it seems like we are not cleaning up all the paths ?
On Tue, Dec 19, 2023 at 02:41:08PM +0530, Sanath S wrote: > > On 12/18/2023 6:48 PM, Mika Westerberg wrote: > > On Mon, Dec 18, 2023 at 06:35:13PM +0530, Sanath S wrote: > > > On 12/18/2023 5:53 PM, Mika Westerberg wrote: > > > > On Mon, Dec 18, 2023 at 01:31:51PM +0200, Mika Westerberg wrote: > > > > > On Mon, Dec 18, 2023 at 04:49:13PM +0530, Sanath S wrote: > > > > > > > The discover part should not do anything (like write the hardware) so > > > > > > > perhaps it is just some timing thing (but that's weird too). > > > > > > > > > > > > > > I think we should do something like this: > > > > > > > > > > > > > > 1. Disable all enabled protocol adapters (reset them to defaults). > > > > > > > 2. Clear all protocol adapter paths. > > > > > > > 3. Issue DPR over all enabled USB4 ports. > > > > > > > > > > > > > > BTW, what you mean "didn't work"? > > > > > > Path activation would go fine after DPR like below: > > > > > > > > > > > > [ 15.090905] thunderbolt 0000:c4:00.5: 0:5 <-> 2:9 (PCI): activating > > > > > > [ 15.090932] thunderbolt 0000:c4:00.5: activating PCIe Down path from 0:5 > > > > > > to 2:9 > > > > > > [ 15.091602] thunderbolt 0000:c4:00.5: activating PCIe Up path from 2:9 to > > > > > > 0:5 > > > > > > > > > > > > But, PCIE enumeration doesn't happen (pcie link up will not happen, will not > > > > > > see below logs) > > > > > > [ 15.134223] pcieport 0000:00:03.1: pciehp: Slot(0-1): Card present > > > > > > [ 15.134243] pcieport 0000:00:03.1: pciehp: Slot(0-1): Link Up > > > > > Okay, what if you like reset the PCIe adapter config spaces back to the > > > > > defaults? Just as an experiment. > > > > If this turns out to be really complex then I guess it is better to do > > > > it like you did originally using discovery but at least it would be nice > > > > to see what the end result of this experiment looks like :) > I feel it's better to go with discover and then reset for now (as v3). > I'll keep this experiment as "to do" and will send out when I crack it down. Fair enough. > > > Yes, I'll give a try. > > > As an experiment, I tried to compare the path deactivation that occurs at > > > two place. > > > 1. In tb_switch_reset where we are calling tb_path_deactivate_hop(port, i). > > > 2. While we get a unplug event after doing DPR. > > > > > > I observed both have different hop_index and port numbers. > > > So, are we calling tb_path_deactivate_hop with wrong hop ids ? > > Wrong adapters possibly. > > > > > From deactivate tunnel (called while unplug) : > > > [ 3.408268] thunderbolt 0000:c4:00.5: deactivating PCIe Down path from > > > 2:9 to 0:5 > > > [ 3.408282] deactivate hop port = 9 hop_index=8 > > > [ 3.408328] deactivate hop port = 2 hop_index=10 > > Definitely should be port = 5 (that's PCIe down in your log) and > > hop_index = 8 (that's the one used with PCIe). > > > > > Deactivate from tb_switch_reset() : > > > deactivate hop port = 5 hop_index=8 > > Can you add some more logging and provide me the dmesg or > > alternativively investigate it yourself. You can use tb_port_dbg() to > > get the port numbers to the log. > I've sent you complete dmesg. Got it, thanks! > Here is the log w.r.t port numbers and path clean up. > > [ 3.389038] thunderbolt 0000:c4:00.5: 0:3: Downstream port, setting DPR > [ 3.389065] Calling usb4_port_reset > [ 3.389068] thunderbolt 0000:c4:00.5: 0:4: Found USB3 DOWN > [ 3.389193] thunderbolt 0000:c4:00.5: 0:4: In reset, cleaning up path, > port->port = 4 hopid = 8 > [ 3.389203] thunderbolt 0000:c4:00.5: 0:4: deactivating_hop port = 4 > hop_index=8 > [ 3.389682] thunderbolt 0000:c4:00.5: 0:5: Found PCI Down > [ 3.389811] thunderbolt 0000:c4:00.5: 0:5: In reset, cleaning up path, > port->port = 5 hopid = 8 > [ 3.389817] thunderbolt 0000:c4:00.5: 0:5: deactivating_hop port = 5 > hop_index=8 > [ 3.390296] thunderbolt 0000:c4:00.5: 0:6: Found DP IN > [ 3.390555] thunderbolt 0000:c4:00.5: 0:6: In reset, cleaning up path, > port->port = 6 hopid = 8 > [ 3.390558] thunderbolt 0000:c4:00.5: 0:6: deactivating_hop port = 6 > hop_index=8 > [ 3.390686] thunderbolt 0000:c4:00.5: 0:6: In reset, cleaning up path, > port->port = 6 hopid = 9 > [ 3.390689] thunderbolt 0000:c4:00.5: 0:6: deactivating_hop port = 6 > hop_index=9 > [ 3.390816] thunderbolt 0000:c4:00.5: 0:7: Found DP IN > [ 3.391077] thunderbolt 0000:c4:00.5: 0:7: In reset, cleaning up path, > port->port = 7 hopid = 8 > [ 3.391080] thunderbolt 0000:c4:00.5: 0:7: deactivating_hop port = 7 > hop_index=8 > [ 3.391213] thunderbolt 0000:c4:00.5: 0:7: In reset, cleaning up path, > port->port = 7 hopid = 9 > [ 3.391217] thunderbolt 0000:c4:00.5: 0:7: deactivating_hop port = 7 > hop_index=9 > [ 3.391342] Reset success > [ 3.391391] thunderbolt 0000:c4:00.5: 0:2: switch unplugged > [ 3.391434] thunderbolt 0000:c4:00.5: 0:4 <-> 2:16 (USB3): deactivating > [ 3.391471] thunderbolt 0000:c4:00.5: deactivating USB3 Down path from > 0:4 to 2:16 > [ 3.391477] thunderbolt 0000:c4:00.5: 0:4: deactivating_hop port = 4 > hop_index=8 > [ 3.391641] thunderbolt 0000:c4:00.5: 2:1: deactivating_hop port = 1 > hop_index=9 > [ 3.391651] thunderbolt 0000:c4:00.5: deactivating USB3 Up path from 2:16 > to 0:4 > [ 3.391659] thunderbolt 0000:c4:00.5: 2:16: deactivating_hop port = 16 > hop_index=8 > [ 3.391664] thunderbolt 0000:c4:00.5: 0:2: deactivating_hop port = 2 > hop_index=9 > [ 3.391701] thunderbolt 0000:c4:00.6: total paths: 3 > [ 3.391720] thunderbolt 0000:c4:00.6: IOMMU DMA protection is disabled > [ 3.392027] thunderbolt 0000:c4:00.5: 0:5 <-> 2:9 (PCI): deactivating > [ 3.392154] thunderbolt 0000:c4:00.5: deactivating PCIe Down path from > 2:9 to 0:5 > [ 3.392163] thunderbolt 0000:c4:00.5: 2:9: deactivating_hop port = 9 > hop_index=8 > [ 3.392170] thunderbolt 0000:c4:00.5: 0:2: deactivating_hop port = 2 > hop_index=10 > [ 3.392534] thunderbolt 0000:c4:00.5: deactivating PCIe Up path from 0:5 > to 2:9 > [ 3.392539] thunderbolt 0000:c4:00.5: 0:5: deactivating_hop port = 5 > hop_index=8 > [ 3.392637] thunderbolt 0000:c4:00.5: 2:1: deactivating_hop port = 1 > hop_index=10 > [ 3.392799] thunderbolt 0-2: device disconnected > > But it seems like we are not cleaning up all the paths ? To me this looks correct and even your dmesg the PCIe tunnel that gets established after the "reset" seems to be working just fine. I also see that in your log you are doing the discovery before reset even though the original idea was to avoid it. In any case this was a good experiment. I will see if I can get this working on my side if I have some spare time during holidays. I guess we can to with the discovery but taking into account the "host_reset". One additional question though, say we have PCIe tunnel established by the BIOS CM and we do the "reset", that means there will be hot-remove on the PCIe side and then hotplug again, does this slow down the boot considerably? We have some delays there in the PCIe code that might hit us here although I agree that we definitely prefer working system rather than fast-booting non-working system but perhaps the delays are not noticeable by the end-user?
On 12/19/2023 5:56 PM, Mika Westerberg wrote: > On Tue, Dec 19, 2023 at 02:41:08PM +0530, Sanath S wrote: >> On 12/18/2023 6:48 PM, Mika Westerberg wrote: >>> On Mon, Dec 18, 2023 at 06:35:13PM +0530, Sanath S wrote: >>>> On 12/18/2023 5:53 PM, Mika Westerberg wrote: >>>>> On Mon, Dec 18, 2023 at 01:31:51PM +0200, Mika Westerberg wrote: >>>>>> On Mon, Dec 18, 2023 at 04:49:13PM +0530, Sanath S wrote: >>>>>>>> The discover part should not do anything (like write the hardware) so >>>>>>>> perhaps it is just some timing thing (but that's weird too). >>>>>>>> >>>>>>>> I think we should do something like this: >>>>>>>> >>>>>>>> 1. Disable all enabled protocol adapters (reset them to defaults). >>>>>>>> 2. Clear all protocol adapter paths. >>>>>>>> 3. Issue DPR over all enabled USB4 ports. >>>>>>>> >>>>>>>> BTW, what you mean "didn't work"? >>>>>>> Path activation would go fine after DPR like below: >>>>>>> >>>>>>> [ 15.090905] thunderbolt 0000:c4:00.5: 0:5 <-> 2:9 (PCI): activating >>>>>>> [ 15.090932] thunderbolt 0000:c4:00.5: activating PCIe Down path from 0:5 >>>>>>> to 2:9 >>>>>>> [ 15.091602] thunderbolt 0000:c4:00.5: activating PCIe Up path from 2:9 to >>>>>>> 0:5 >>>>>>> >>>>>>> But, PCIE enumeration doesn't happen (pcie link up will not happen, will not >>>>>>> see below logs) >>>>>>> [ 15.134223] pcieport 0000:00:03.1: pciehp: Slot(0-1): Card present >>>>>>> [ 15.134243] pcieport 0000:00:03.1: pciehp: Slot(0-1): Link Up >>>>>> Okay, what if you like reset the PCIe adapter config spaces back to the >>>>>> defaults? Just as an experiment. >>>>> If this turns out to be really complex then I guess it is better to do >>>>> it like you did originally using discovery but at least it would be nice >>>>> to see what the end result of this experiment looks like :) >> I feel it's better to go with discover and then reset for now (as v3). >> I'll keep this experiment as "to do" and will send out when I crack it down. > Fair enough. > >>>> Yes, I'll give a try. >>>> As an experiment, I tried to compare the path deactivation that occurs at >>>> two place. >>>> 1. In tb_switch_reset where we are calling tb_path_deactivate_hop(port, i). >>>> 2. While we get a unplug event after doing DPR. >>>> >>>> I observed both have different hop_index and port numbers. >>>> So, are we calling tb_path_deactivate_hop with wrong hop ids ? >>> Wrong adapters possibly. >>> >>>> From deactivate tunnel (called while unplug) : >>>> [ 3.408268] thunderbolt 0000:c4:00.5: deactivating PCIe Down path from >>>> 2:9 to 0:5 >>>> [ 3.408282] deactivate hop port = 9 hop_index=8 >>>> [ 3.408328] deactivate hop port = 2 hop_index=10 >>> Definitely should be port = 5 (that's PCIe down in your log) and >>> hop_index = 8 (that's the one used with PCIe). >>> >>>> Deactivate from tb_switch_reset() : >>>> deactivate hop port = 5 hop_index=8 >>> Can you add some more logging and provide me the dmesg or >>> alternativively investigate it yourself. You can use tb_port_dbg() to >>> get the port numbers to the log. >> I've sent you complete dmesg. > Got it, thanks! > >> Here is the log w.r.t port numbers and path clean up. >> >> [ 3.389038] thunderbolt 0000:c4:00.5: 0:3: Downstream port, setting DPR >> [ 3.389065] Calling usb4_port_reset >> [ 3.389068] thunderbolt 0000:c4:00.5: 0:4: Found USB3 DOWN >> [ 3.389193] thunderbolt 0000:c4:00.5: 0:4: In reset, cleaning up path, >> port->port = 4 hopid = 8 >> [ 3.389203] thunderbolt 0000:c4:00.5: 0:4: deactivating_hop port = 4 >> hop_index=8 >> [ 3.389682] thunderbolt 0000:c4:00.5: 0:5: Found PCI Down >> [ 3.389811] thunderbolt 0000:c4:00.5: 0:5: In reset, cleaning up path, >> port->port = 5 hopid = 8 >> [ 3.389817] thunderbolt 0000:c4:00.5: 0:5: deactivating_hop port = 5 >> hop_index=8 >> [ 3.390296] thunderbolt 0000:c4:00.5: 0:6: Found DP IN >> [ 3.390555] thunderbolt 0000:c4:00.5: 0:6: In reset, cleaning up path, >> port->port = 6 hopid = 8 >> [ 3.390558] thunderbolt 0000:c4:00.5: 0:6: deactivating_hop port = 6 >> hop_index=8 >> [ 3.390686] thunderbolt 0000:c4:00.5: 0:6: In reset, cleaning up path, >> port->port = 6 hopid = 9 >> [ 3.390689] thunderbolt 0000:c4:00.5: 0:6: deactivating_hop port = 6 >> hop_index=9 >> [ 3.390816] thunderbolt 0000:c4:00.5: 0:7: Found DP IN >> [ 3.391077] thunderbolt 0000:c4:00.5: 0:7: In reset, cleaning up path, >> port->port = 7 hopid = 8 >> [ 3.391080] thunderbolt 0000:c4:00.5: 0:7: deactivating_hop port = 7 >> hop_index=8 >> [ 3.391213] thunderbolt 0000:c4:00.5: 0:7: In reset, cleaning up path, >> port->port = 7 hopid = 9 >> [ 3.391217] thunderbolt 0000:c4:00.5: 0:7: deactivating_hop port = 7 >> hop_index=9 >> [ 3.391342] Reset success >> [ 3.391391] thunderbolt 0000:c4:00.5: 0:2: switch unplugged >> [ 3.391434] thunderbolt 0000:c4:00.5: 0:4 <-> 2:16 (USB3): deactivating >> [ 3.391471] thunderbolt 0000:c4:00.5: deactivating USB3 Down path from >> 0:4 to 2:16 >> [ 3.391477] thunderbolt 0000:c4:00.5: 0:4: deactivating_hop port = 4 >> hop_index=8 >> [ 3.391641] thunderbolt 0000:c4:00.5: 2:1: deactivating_hop port = 1 >> hop_index=9 >> [ 3.391651] thunderbolt 0000:c4:00.5: deactivating USB3 Up path from 2:16 >> to 0:4 >> [ 3.391659] thunderbolt 0000:c4:00.5: 2:16: deactivating_hop port = 16 >> hop_index=8 >> [ 3.391664] thunderbolt 0000:c4:00.5: 0:2: deactivating_hop port = 2 >> hop_index=9 >> [ 3.391701] thunderbolt 0000:c4:00.6: total paths: 3 >> [ 3.391720] thunderbolt 0000:c4:00.6: IOMMU DMA protection is disabled >> [ 3.392027] thunderbolt 0000:c4:00.5: 0:5 <-> 2:9 (PCI): deactivating >> [ 3.392154] thunderbolt 0000:c4:00.5: deactivating PCIe Down path from >> 2:9 to 0:5 >> [ 3.392163] thunderbolt 0000:c4:00.5: 2:9: deactivating_hop port = 9 >> hop_index=8 >> [ 3.392170] thunderbolt 0000:c4:00.5: 0:2: deactivating_hop port = 2 >> hop_index=10 >> [ 3.392534] thunderbolt 0000:c4:00.5: deactivating PCIe Up path from 0:5 >> to 2:9 >> [ 3.392539] thunderbolt 0000:c4:00.5: 0:5: deactivating_hop port = 5 >> hop_index=8 >> [ 3.392637] thunderbolt 0000:c4:00.5: 2:1: deactivating_hop port = 1 >> hop_index=10 >> [ 3.392799] thunderbolt 0-2: device disconnected >> >> But it seems like we are not cleaning up all the paths ? > To me this looks correct and even your dmesg the PCIe tunnel that gets > established after the "reset" seems to be working just fine. I also see > that in your log you are doing the discovery before reset even though > the original idea was to avoid it. I did this as an experiment to collect logs and check if we are resetting the same path config. Just to get a comparison view. > > In any case this was a good experiment. I will see if I can get this > working on my side if I have some spare time during holidays. Sure. I'll keep trying too. > I guess we can to with the discovery but taking into account the > "host_reset". Yes, along with changes in lc.c for <= TBT3 > One additional question though, say we have PCIe tunnel established by > the BIOS CM and we do the "reset", that means there will be hot-remove > on the PCIe side and then hotplug again, does this slow down the boot > considerably? We have some delays there in the PCIe code that might hit > us here although I agree that we definitely prefer working system rather > than fast-booting non-working system but perhaps the delays are not > noticeable by the end-user? I've not observed any delay which is noticeable. As soon as I get the login screen and check dmesg, it would already be enumerated. And moreover, this scenario is applicable only when dock stays connected during reboot or S5.
On Tue, Dec 19, 2023 at 08:05:15PM +0530, Sanath S wrote: > > On 12/19/2023 5:56 PM, Mika Westerberg wrote: > > On Tue, Dec 19, 2023 at 02:41:08PM +0530, Sanath S wrote: > > > On 12/18/2023 6:48 PM, Mika Westerberg wrote: > > > > On Mon, Dec 18, 2023 at 06:35:13PM +0530, Sanath S wrote: > > > > > On 12/18/2023 5:53 PM, Mika Westerberg wrote: > > > > > > On Mon, Dec 18, 2023 at 01:31:51PM +0200, Mika Westerberg wrote: > > > > > > > On Mon, Dec 18, 2023 at 04:49:13PM +0530, Sanath S wrote: > > > > > > > > > The discover part should not do anything (like write the hardware) so > > > > > > > > > perhaps it is just some timing thing (but that's weird too). > > > > > > > > > > > > > > > > > > I think we should do something like this: > > > > > > > > > > > > > > > > > > 1. Disable all enabled protocol adapters (reset them to defaults). > > > > > > > > > 2. Clear all protocol adapter paths. > > > > > > > > > 3. Issue DPR over all enabled USB4 ports. > > > > > > > > > > > > > > > > > > BTW, what you mean "didn't work"? > > > > > > > > Path activation would go fine after DPR like below: > > > > > > > > > > > > > > > > [ 15.090905] thunderbolt 0000:c4:00.5: 0:5 <-> 2:9 (PCI): activating > > > > > > > > [ 15.090932] thunderbolt 0000:c4:00.5: activating PCIe Down path from 0:5 > > > > > > > > to 2:9 > > > > > > > > [ 15.091602] thunderbolt 0000:c4:00.5: activating PCIe Up path from 2:9 to > > > > > > > > 0:5 > > > > > > > > > > > > > > > > But, PCIE enumeration doesn't happen (pcie link up will not happen, will not > > > > > > > > see below logs) > > > > > > > > [ 15.134223] pcieport 0000:00:03.1: pciehp: Slot(0-1): Card present > > > > > > > > [ 15.134243] pcieport 0000:00:03.1: pciehp: Slot(0-1): Link Up > > > > > > > Okay, what if you like reset the PCIe adapter config spaces back to the > > > > > > > defaults? Just as an experiment. > > > > > > If this turns out to be really complex then I guess it is better to do > > > > > > it like you did originally using discovery but at least it would be nice > > > > > > to see what the end result of this experiment looks like :) > > > I feel it's better to go with discover and then reset for now (as v3). > > > I'll keep this experiment as "to do" and will send out when I crack it down. > > Fair enough. > > > > > > > Yes, I'll give a try. > > > > > As an experiment, I tried to compare the path deactivation that occurs at > > > > > two place. > > > > > 1. In tb_switch_reset where we are calling tb_path_deactivate_hop(port, i). > > > > > 2. While we get a unplug event after doing DPR. > > > > > > > > > > I observed both have different hop_index and port numbers. > > > > > So, are we calling tb_path_deactivate_hop with wrong hop ids ? > > > > Wrong adapters possibly. > > > > > > > > > From deactivate tunnel (called while unplug) : > > > > > [ 3.408268] thunderbolt 0000:c4:00.5: deactivating PCIe Down path from > > > > > 2:9 to 0:5 > > > > > [ 3.408282] deactivate hop port = 9 hop_index=8 > > > > > [ 3.408328] deactivate hop port = 2 hop_index=10 > > > > Definitely should be port = 5 (that's PCIe down in your log) and > > > > hop_index = 8 (that's the one used with PCIe). > > > > > > > > > Deactivate from tb_switch_reset() : > > > > > deactivate hop port = 5 hop_index=8 > > > > Can you add some more logging and provide me the dmesg or > > > > alternativively investigate it yourself. You can use tb_port_dbg() to > > > > get the port numbers to the log. > > > I've sent you complete dmesg. > > Got it, thanks! > > > > > Here is the log w.r.t port numbers and path clean up. > > > > > > [ 3.389038] thunderbolt 0000:c4:00.5: 0:3: Downstream port, setting DPR > > > [ 3.389065] Calling usb4_port_reset > > > [ 3.389068] thunderbolt 0000:c4:00.5: 0:4: Found USB3 DOWN > > > [ 3.389193] thunderbolt 0000:c4:00.5: 0:4: In reset, cleaning up path, > > > port->port = 4 hopid = 8 > > > [ 3.389203] thunderbolt 0000:c4:00.5: 0:4: deactivating_hop port = 4 > > > hop_index=8 > > > [ 3.389682] thunderbolt 0000:c4:00.5: 0:5: Found PCI Down > > > [ 3.389811] thunderbolt 0000:c4:00.5: 0:5: In reset, cleaning up path, > > > port->port = 5 hopid = 8 > > > [ 3.389817] thunderbolt 0000:c4:00.5: 0:5: deactivating_hop port = 5 > > > hop_index=8 > > > [ 3.390296] thunderbolt 0000:c4:00.5: 0:6: Found DP IN > > > [ 3.390555] thunderbolt 0000:c4:00.5: 0:6: In reset, cleaning up path, > > > port->port = 6 hopid = 8 > > > [ 3.390558] thunderbolt 0000:c4:00.5: 0:6: deactivating_hop port = 6 > > > hop_index=8 > > > [ 3.390686] thunderbolt 0000:c4:00.5: 0:6: In reset, cleaning up path, > > > port->port = 6 hopid = 9 > > > [ 3.390689] thunderbolt 0000:c4:00.5: 0:6: deactivating_hop port = 6 > > > hop_index=9 > > > [ 3.390816] thunderbolt 0000:c4:00.5: 0:7: Found DP IN > > > [ 3.391077] thunderbolt 0000:c4:00.5: 0:7: In reset, cleaning up path, > > > port->port = 7 hopid = 8 > > > [ 3.391080] thunderbolt 0000:c4:00.5: 0:7: deactivating_hop port = 7 > > > hop_index=8 > > > [ 3.391213] thunderbolt 0000:c4:00.5: 0:7: In reset, cleaning up path, > > > port->port = 7 hopid = 9 > > > [ 3.391217] thunderbolt 0000:c4:00.5: 0:7: deactivating_hop port = 7 > > > hop_index=9 > > > [ 3.391342] Reset success > > > [ 3.391391] thunderbolt 0000:c4:00.5: 0:2: switch unplugged > > > [ 3.391434] thunderbolt 0000:c4:00.5: 0:4 <-> 2:16 (USB3): deactivating > > > [ 3.391471] thunderbolt 0000:c4:00.5: deactivating USB3 Down path from > > > 0:4 to 2:16 > > > [ 3.391477] thunderbolt 0000:c4:00.5: 0:4: deactivating_hop port = 4 > > > hop_index=8 > > > [ 3.391641] thunderbolt 0000:c4:00.5: 2:1: deactivating_hop port = 1 > > > hop_index=9 > > > [ 3.391651] thunderbolt 0000:c4:00.5: deactivating USB3 Up path from 2:16 > > > to 0:4 > > > [ 3.391659] thunderbolt 0000:c4:00.5: 2:16: deactivating_hop port = 16 > > > hop_index=8 > > > [ 3.391664] thunderbolt 0000:c4:00.5: 0:2: deactivating_hop port = 2 > > > hop_index=9 > > > [ 3.391701] thunderbolt 0000:c4:00.6: total paths: 3 > > > [ 3.391720] thunderbolt 0000:c4:00.6: IOMMU DMA protection is disabled > > > [ 3.392027] thunderbolt 0000:c4:00.5: 0:5 <-> 2:9 (PCI): deactivating > > > [ 3.392154] thunderbolt 0000:c4:00.5: deactivating PCIe Down path from > > > 2:9 to 0:5 > > > [ 3.392163] thunderbolt 0000:c4:00.5: 2:9: deactivating_hop port = 9 > > > hop_index=8 > > > [ 3.392170] thunderbolt 0000:c4:00.5: 0:2: deactivating_hop port = 2 > > > hop_index=10 > > > [ 3.392534] thunderbolt 0000:c4:00.5: deactivating PCIe Up path from 0:5 > > > to 2:9 > > > [ 3.392539] thunderbolt 0000:c4:00.5: 0:5: deactivating_hop port = 5 > > > hop_index=8 > > > [ 3.392637] thunderbolt 0000:c4:00.5: 2:1: deactivating_hop port = 1 > > > hop_index=10 > > > [ 3.392799] thunderbolt 0-2: device disconnected > > > > > > But it seems like we are not cleaning up all the paths ? > > To me this looks correct and even your dmesg the PCIe tunnel that gets > > established after the "reset" seems to be working just fine. I also see > > that in your log you are doing the discovery before reset even though > > the original idea was to avoid it. > I did this as an experiment to collect logs and check if we are resetting > the same > path config. Just to get a comparison view. Okay. > > In any case this was a good experiment. I will see if I can get this > > working on my side if I have some spare time during holidays. > Sure. I'll keep trying too. > > I guess we can to with the discovery but taking into account the > > "host_reset". > Yes, along with changes in lc.c for <= TBT3 Right. > > One additional question though, say we have PCIe tunnel established by > > the BIOS CM and we do the "reset", that means there will be hot-remove > > on the PCIe side and then hotplug again, does this slow down the boot > > considerably? We have some delays there in the PCIe code that might hit > > us here although I agree that we definitely prefer working system rather > > than fast-booting non-working system but perhaps the delays are not > > noticeable by the end-user? > I've not observed any delay which is noticeable. As soon as I get the login > screen > and check dmesg, it would already be enumerated. Okay, I need to try it on my side too. > And moreover, this scenario is applicable only when dock stays connected > during reboot or S5. Which is pretty common case. Laptop with a docking station.
On Tue, Dec 19, 2023 at 08:04:24PM +0200, Mika Westerberg wrote: > > > One additional question though, say we have PCIe tunnel established by > > > the BIOS CM and we do the "reset", that means there will be hot-remove > > > on the PCIe side and then hotplug again, does this slow down the boot > > > considerably? We have some delays there in the PCIe code that might hit > > > us here although I agree that we definitely prefer working system rather > > > than fast-booting non-working system but perhaps the delays are not > > > noticeable by the end-user? > > I've not observed any delay which is noticeable. As soon as I get the login > > screen > > and check dmesg, it would already be enumerated. > > Okay, I need to try it on my side too. One additional thing that came to mind. Please check with some device with a real PCIe endpoint. For instance there is the integrated xHCI controller on Intel Titan Ridge and Goshen Ridge based docks. With TR it is easy because it does not support USB4 so xHCI is brought up immediately once there is PCIe tunnel. For GR (the OWC dock you have) it is disabled when the link is USB4 (because USB 3.x is tunneled as well) but you can get it enabled too if you connect it with an active TBT3 cable.
On 12/20/2023 6:28 PM, Mika Westerberg wrote: > On Tue, Dec 19, 2023 at 08:04:24PM +0200, Mika Westerberg wrote: >>>> One additional question though, say we have PCIe tunnel established by >>>> the BIOS CM and we do the "reset", that means there will be hot-remove >>>> on the PCIe side and then hotplug again, does this slow down the boot >>>> considerably? We have some delays there in the PCIe code that might hit >>>> us here although I agree that we definitely prefer working system rather >>>> than fast-booting non-working system but perhaps the delays are not >>>> noticeable by the end-user? >>> I've not observed any delay which is noticeable. As soon as I get the login >>> screen >>> and check dmesg, it would already be enumerated. >> Okay, I need to try it on my side too. > One additional thing that came to mind. Please check with some device > with a real PCIe endpoint. For instance there is the integrated xHCI > controller on Intel Titan Ridge and Goshen Ridge based docks. With TR it > is easy because it does not support USB4 so xHCI is brought up > immediately once there is PCIe tunnel. For GR (the OWC dock you have) it > is disabled when the link is USB4 (because USB 3.x is tunneled as well) > but you can get it enabled too if you connect it with an active TBT3 > cable. Sure. I'll check with these combinations.
On 12/20/2023 10:31 PM, Sanath S wrote: > > On 12/20/2023 6:28 PM, Mika Westerberg wrote: >> On Tue, Dec 19, 2023 at 08:04:24PM +0200, Mika Westerberg wrote: >>>>> One additional question though, say we have PCIe tunnel >>>>> established by >>>>> the BIOS CM and we do the "reset", that means there will be >>>>> hot-remove >>>>> on the PCIe side and then hotplug again, does this slow down the boot >>>>> considerably? We have some delays there in the PCIe code that >>>>> might hit >>>>> us here although I agree that we definitely prefer working system >>>>> rather >>>>> than fast-booting non-working system but perhaps the delays are not >>>>> noticeable by the end-user? >>>> I've not observed any delay which is noticeable. As soon as I get >>>> the login >>>> screen >>>> and check dmesg, it would already be enumerated. >>> Okay, I need to try it on my side too. >> One additional thing that came to mind. Please check with some device >> with a real PCIe endpoint. For instance there is the integrated xHCI >> controller on Intel Titan Ridge and Goshen Ridge based docks. With TR it >> is easy because it does not support USB4 so xHCI is brought up >> immediately once there is PCIe tunnel. For GR (the OWC dock you have) it >> is disabled when the link is USB4 (because USB 3.x is tunneled as well) >> but you can get it enabled too if you connect it with an active TBT3 >> cable. > Sure. I'll check with these combinations. Can you name any docks that meets these requirements ? I'll try to get hold of it here and check.
On Thu, Dec 21, 2023 at 03:01:50PM +0530, Sanath S wrote: > > Sure. I'll check with these combinations. > > Can you name any docks that meets these requirements ? I'll try to get > hold of it here and check. Pretty much every Titan Ridge based dock. For instance Dell WD19TB. I have some hardware here that I can use to try it out too.
On 12/21/2023 3:23 PM, Mika Westerberg wrote: > On Thu, Dec 21, 2023 at 03:01:50PM +0530, Sanath S wrote: >>> Sure. I'll check with these combinations. >> Can you name any docks that meets these requirements ? I'll try to get >> hold of it here and check. > Pretty much every Titan Ridge based dock. For instance Dell WD19TB. > > I have some hardware here that I can use to try it out too. It seems that issue is seen on Dell WD19TB too. So this fix may have to extended to TBT3 as well ? I'll give it a try this week and share the observation. Any luck from your end ?
On Wed, Jan 03, 2024 at 07:45:56PM +0530, Sanath S wrote: > > On 12/21/2023 3:23 PM, Mika Westerberg wrote: > > On Thu, Dec 21, 2023 at 03:01:50PM +0530, Sanath S wrote: > > > > Sure. I'll check with these combinations. > > > Can you name any docks that meets these requirements ? I'll try to get > > > hold of it here and check. > > Pretty much every Titan Ridge based dock. For instance Dell WD19TB. > > > > I have some hardware here that I can use to try it out too. > It seems that issue is seen on Dell WD19TB too. So this fix may have to > extended to TBT3 as well ? Hm, what issue? I thought this works accross all the supported devices due to the DFP, no? > I'll give it a try this week and share the observation. Okay thanks! > Any luck from your end ? I did not have time to try it out yet unfortunately.
On 1/3/2024 10:47 PM, Mika Westerberg wrote: > On Wed, Jan 03, 2024 at 07:45:56PM +0530, Sanath S wrote: >> On 12/21/2023 3:23 PM, Mika Westerberg wrote: >>> On Thu, Dec 21, 2023 at 03:01:50PM +0530, Sanath S wrote: >>>>> Sure. I'll check with these combinations. >>>> Can you name any docks that meets these requirements ? I'll try to get >>>> hold of it here and check. >>> Pretty much every Titan Ridge based dock. For instance Dell WD19TB. >>> >>> I have some hardware here that I can use to try it out too. >> It seems that issue is seen on Dell WD19TB too. So this fix may have to >> extended to TBT3 as well ? > Hm, what issue? I thought this works accross all the supported devices > due to the DFP, no? > >> I'll give it a try this week and share the observation. Got my hands on Dell WD19TB. And it works! Here is lspci -t output with and without fix without fix: +-03.1-[04-62]----00.0-[05-07]--+-02.0-[06]----00.0 | \-04.0-[07]-- With fix: +-03.1-[04-62]----00.0-[05-62]--+-02.0-[06]----00.0 | \-04.0-[07-62]-- I'll send out v3 with with splitting into 2/3 patches(Will see how it looks good). Any other comments, we can take it on v3. > Okay thanks! > >> Any luck from your end ? > I did not have time to try it out yet unfortunately.
On 1/3/2024 10:47 PM, Mika Westerberg wrote: > On Wed, Jan 03, 2024 at 07:45:56PM +0530, Sanath S wrote: >> On 12/21/2023 3:23 PM, Mika Westerberg wrote: >>> On Thu, Dec 21, 2023 at 03:01:50PM +0530, Sanath S wrote: >>>>> Sure. I'll check with these combinations. >>>> Can you name any docks that meets these requirements ? I'll try to get >>>> hold of it here and check. >>> Pretty much every Titan Ridge based dock. For instance Dell WD19TB. >>> >>> I have some hardware here that I can use to try it out too. >> It seems that issue is seen on Dell WD19TB too. So this fix may have to >> extended to TBT3 as well ? > Hm, what issue? I thought this works accross all the supported devices > due to the DFP, no? > >> I'll give it a try this week and share the observation. Got my hands on Dell WD19TB. And it works! Here is lspci -t output with and without fix without fix: +-03.1-[04-62]----00.0-[05-07]--+-02.0-[06]----00.0 | \-04.0-[07]-- With fix: +-03.1-[04-62]----00.0-[05-62]--+-02.0-[06]----00.0 | \-04.0-[07-62]-- I'll send out v3 with with splitting into 2/3 patches(Will see how it looks good). Any other comments, we can take it on v3. > Okay thanks! > >> Any luck from your end ? > I did not have time to try it out yet unfortunately.
On 12/19/2023 5:56 PM, Mika Westerberg wrote: > On Tue, Dec 19, 2023 at 02:41:08PM +0530, Sanath S wrote: >> On 12/18/2023 6:48 PM, Mika Westerberg wrote: >>> On Mon, Dec 18, 2023 at 06:35:13PM +0530, Sanath S wrote: >>>> On 12/18/2023 5:53 PM, Mika Westerberg wrote: >>>>> On Mon, Dec 18, 2023 at 01:31:51PM +0200, Mika Westerberg wrote: >>>>>> On Mon, Dec 18, 2023 at 04:49:13PM +0530, Sanath S wrote: >>>>>>>> The discover part should not do anything (like write the hardware) so >>>>>>>> perhaps it is just some timing thing (but that's weird too). >>>>>>>> >>>>>>>> I think we should do something like this: >>>>>>>> >>>>>>>> 1. Disable all enabled protocol adapters (reset them to defaults). >>>>>>>> 2. Clear all protocol adapter paths. >>>>>>>> 3. Issue DPR over all enabled USB4 ports. >>>>>>>> >>>>>>>> BTW, what you mean "didn't work"? >>>>>>> Path activation would go fine after DPR like below: >>>>>>> >>>>>>> [ 15.090905] thunderbolt 0000:c4:00.5: 0:5 <-> 2:9 (PCI): activating >>>>>>> [ 15.090932] thunderbolt 0000:c4:00.5: activating PCIe Down path from 0:5 >>>>>>> to 2:9 >>>>>>> [ 15.091602] thunderbolt 0000:c4:00.5: activating PCIe Up path from 2:9 to >>>>>>> 0:5 >>>>>>> >>>>>>> But, PCIE enumeration doesn't happen (pcie link up will not happen, will not >>>>>>> see below logs) >>>>>>> [ 15.134223] pcieport 0000:00:03.1: pciehp: Slot(0-1): Card present >>>>>>> [ 15.134243] pcieport 0000:00:03.1: pciehp: Slot(0-1): Link Up >>>>>> Okay, what if you like reset the PCIe adapter config spaces back to the >>>>>> defaults? Just as an experiment. >>>>> If this turns out to be really complex then I guess it is better to do >>>>> it like you did originally using discovery but at least it would be nice >>>>> to see what the end result of this experiment looks like :) >> I feel it's better to go with discover and then reset for now (as v3). >> I'll keep this experiment as "to do" and will send out when I crack it down. I got what we were missing. It's not required to do a discover_tunnel before we tear down. We were resetting the downstream port and do a "continue;" So, we were not cleaning up its path. Actually we have to cleanup its path after DPR. After changing it, It works without any tunnel_discover() api's. + if (tb_port_is_null(port) && !tb_is_upstream_port(port)) { + ret = tb_port_reset(port); + if (ret) + return ret; + continue; >>>>>>>>>>>>>>>>>> Need to be removed + } 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); + } 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; + } + } > Fair enough. > >>>> Yes, I'll give a try. >>>> As an experiment, I tried to compare the path deactivation that occurs at >>>> two place. >>>> 1. In tb_switch_reset where we are calling tb_path_deactivate_hop(port, i). >>>> 2. While we get a unplug event after doing DPR. >>>> >>>> I observed both have different hop_index and port numbers. >>>> So, are we calling tb_path_deactivate_hop with wrong hop ids ? >>> Wrong adapters possibly. >>> >>>> From deactivate tunnel (called while unplug) : >>>> [ 3.408268] thunderbolt 0000:c4:00.5: deactivating PCIe Down path from >>>> 2:9 to 0:5 >>>> [ 3.408282] deactivate hop port = 9 hop_index=8 >>>> [ 3.408328] deactivate hop port = 2 hop_index=10 >>> Definitely should be port = 5 (that's PCIe down in your log) and >>> hop_index = 8 (that's the one used with PCIe). >>> >>>> Deactivate from tb_switch_reset() : >>>> deactivate hop port = 5 hop_index=8 >>> Can you add some more logging and provide me the dmesg or >>> alternativively investigate it yourself. You can use tb_port_dbg() to >>> get the port numbers to the log. >> I've sent you complete dmesg. > Got it, thanks! > >> Here is the log w.r.t port numbers and path clean up. >> >> [ 3.389038] thunderbolt 0000:c4:00.5: 0:3: Downstream port, setting DPR >> [ 3.389065] Calling usb4_port_reset >> [ 3.389068] thunderbolt 0000:c4:00.5: 0:4: Found USB3 DOWN >> [ 3.389193] thunderbolt 0000:c4:00.5: 0:4: In reset, cleaning up path, >> port->port = 4 hopid = 8 >> [ 3.389203] thunderbolt 0000:c4:00.5: 0:4: deactivating_hop port = 4 >> hop_index=8 >> [ 3.389682] thunderbolt 0000:c4:00.5: 0:5: Found PCI Down >> [ 3.389811] thunderbolt 0000:c4:00.5: 0:5: In reset, cleaning up path, >> port->port = 5 hopid = 8 >> [ 3.389817] thunderbolt 0000:c4:00.5: 0:5: deactivating_hop port = 5 >> hop_index=8 >> [ 3.390296] thunderbolt 0000:c4:00.5: 0:6: Found DP IN >> [ 3.390555] thunderbolt 0000:c4:00.5: 0:6: In reset, cleaning up path, >> port->port = 6 hopid = 8 >> [ 3.390558] thunderbolt 0000:c4:00.5: 0:6: deactivating_hop port = 6 >> hop_index=8 >> [ 3.390686] thunderbolt 0000:c4:00.5: 0:6: In reset, cleaning up path, >> port->port = 6 hopid = 9 >> [ 3.390689] thunderbolt 0000:c4:00.5: 0:6: deactivating_hop port = 6 >> hop_index=9 >> [ 3.390816] thunderbolt 0000:c4:00.5: 0:7: Found DP IN >> [ 3.391077] thunderbolt 0000:c4:00.5: 0:7: In reset, cleaning up path, >> port->port = 7 hopid = 8 >> [ 3.391080] thunderbolt 0000:c4:00.5: 0:7: deactivating_hop port = 7 >> hop_index=8 >> [ 3.391213] thunderbolt 0000:c4:00.5: 0:7: In reset, cleaning up path, >> port->port = 7 hopid = 9 >> [ 3.391217] thunderbolt 0000:c4:00.5: 0:7: deactivating_hop port = 7 >> hop_index=9 >> [ 3.391342] Reset success >> [ 3.391391] thunderbolt 0000:c4:00.5: 0:2: switch unplugged >> [ 3.391434] thunderbolt 0000:c4:00.5: 0:4 <-> 2:16 (USB3): deactivating >> [ 3.391471] thunderbolt 0000:c4:00.5: deactivating USB3 Down path from >> 0:4 to 2:16 >> [ 3.391477] thunderbolt 0000:c4:00.5: 0:4: deactivating_hop port = 4 >> hop_index=8 >> [ 3.391641] thunderbolt 0000:c4:00.5: 2:1: deactivating_hop port = 1 >> hop_index=9 >> [ 3.391651] thunderbolt 0000:c4:00.5: deactivating USB3 Up path from 2:16 >> to 0:4 >> [ 3.391659] thunderbolt 0000:c4:00.5: 2:16: deactivating_hop port = 16 >> hop_index=8 >> [ 3.391664] thunderbolt 0000:c4:00.5: 0:2: deactivating_hop port = 2 >> hop_index=9 >> [ 3.391701] thunderbolt 0000:c4:00.6: total paths: 3 >> [ 3.391720] thunderbolt 0000:c4:00.6: IOMMU DMA protection is disabled >> [ 3.392027] thunderbolt 0000:c4:00.5: 0:5 <-> 2:9 (PCI): deactivating >> [ 3.392154] thunderbolt 0000:c4:00.5: deactivating PCIe Down path from >> 2:9 to 0:5 >> [ 3.392163] thunderbolt 0000:c4:00.5: 2:9: deactivating_hop port = 9 >> hop_index=8 >> [ 3.392170] thunderbolt 0000:c4:00.5: 0:2: deactivating_hop port = 2 >> hop_index=10 >> [ 3.392534] thunderbolt 0000:c4:00.5: deactivating PCIe Up path from 0:5 >> to 2:9 >> [ 3.392539] thunderbolt 0000:c4:00.5: 0:5: deactivating_hop port = 5 >> hop_index=8 >> [ 3.392637] thunderbolt 0000:c4:00.5: 2:1: deactivating_hop port = 1 >> hop_index=10 >> [ 3.392799] thunderbolt 0-2: device disconnected >> >> But it seems like we are not cleaning up all the paths ? > To me this looks correct and even your dmesg the PCIe tunnel that gets > established after the "reset" seems to be working just fine. I also see > that in your log you are doing the discovery before reset even though > the original idea was to avoid it. > > In any case this was a good experiment. I will see if I can get this > working on my side if I have some spare time during holidays. > > I guess we can to with the discovery but taking into account the > "host_reset". > > One additional question though, say we have PCIe tunnel established by > the BIOS CM and we do the "reset", that means there will be hot-remove > on the PCIe side and then hotplug again, does this slow down the boot > considerably? We have some delays there in the PCIe code that might hit > us here although I agree that we definitely prefer working system rather > than fast-booting non-working system but perhaps the delays are not > noticeable by the end-user?
On Thu, Jan 04, 2024 at 10:19:20PM +0530, Sanath S wrote: > > On 12/19/2023 5:56 PM, Mika Westerberg wrote: > > On Tue, Dec 19, 2023 at 02:41:08PM +0530, Sanath S wrote: > > > On 12/18/2023 6:48 PM, Mika Westerberg wrote: > > > > On Mon, Dec 18, 2023 at 06:35:13PM +0530, Sanath S wrote: > > > > > On 12/18/2023 5:53 PM, Mika Westerberg wrote: > > > > > > On Mon, Dec 18, 2023 at 01:31:51PM +0200, Mika Westerberg wrote: > > > > > > > On Mon, Dec 18, 2023 at 04:49:13PM +0530, Sanath S wrote: > > > > > > > > > The discover part should not do anything (like write the hardware) so > > > > > > > > > perhaps it is just some timing thing (but that's weird too). > > > > > > > > > > > > > > > > > > I think we should do something like this: > > > > > > > > > > > > > > > > > > 1. Disable all enabled protocol adapters (reset them to defaults). > > > > > > > > > 2. Clear all protocol adapter paths. > > > > > > > > > 3. Issue DPR over all enabled USB4 ports. > > > > > > > > > > > > > > > > > > BTW, what you mean "didn't work"? > > > > > > > > Path activation would go fine after DPR like below: > > > > > > > > > > > > > > > > [ 15.090905] thunderbolt 0000:c4:00.5: 0:5 <-> 2:9 (PCI): activating > > > > > > > > [ 15.090932] thunderbolt 0000:c4:00.5: activating PCIe Down path from 0:5 > > > > > > > > to 2:9 > > > > > > > > [ 15.091602] thunderbolt 0000:c4:00.5: activating PCIe Up path from 2:9 to > > > > > > > > 0:5 > > > > > > > > > > > > > > > > But, PCIE enumeration doesn't happen (pcie link up will not happen, will not > > > > > > > > see below logs) > > > > > > > > [ 15.134223] pcieport 0000:00:03.1: pciehp: Slot(0-1): Card present > > > > > > > > [ 15.134243] pcieport 0000:00:03.1: pciehp: Slot(0-1): Link Up > > > > > > > Okay, what if you like reset the PCIe adapter config spaces back to the > > > > > > > defaults? Just as an experiment. > > > > > > If this turns out to be really complex then I guess it is better to do > > > > > > it like you did originally using discovery but at least it would be nice > > > > > > to see what the end result of this experiment looks like :) > > > I feel it's better to go with discover and then reset for now (as v3). > > > I'll keep this experiment as "to do" and will send out when I crack it down. > I got what we were missing. It's not required to do a discover_tunnel before > we tear down. > > We were resetting the downstream port and do a "continue;" > So, we were not cleaning up its path. Actually we have to cleanup its path > after DPR. > > After changing it, It works without any tunnel_discover() api's. Makes sense, good finding.
On Thu, Jan 04, 2024 at 07:20:05PM +0530, Sanath S wrote: > > On 1/3/2024 10:47 PM, Mika Westerberg wrote: > > On Wed, Jan 03, 2024 at 07:45:56PM +0530, Sanath S wrote: > > > On 12/21/2023 3:23 PM, Mika Westerberg wrote: > > > > On Thu, Dec 21, 2023 at 03:01:50PM +0530, Sanath S wrote: > > > > > > Sure. I'll check with these combinations. > > > > > Can you name any docks that meets these requirements ? I'll try to get > > > > > hold of it here and check. > > > > Pretty much every Titan Ridge based dock. For instance Dell WD19TB. > > > > > > > > I have some hardware here that I can use to try it out too. > > > It seems that issue is seen on Dell WD19TB too. So this fix may have to > > > extended to TBT3 as well ? > > Hm, what issue? I thought this works accross all the supported devices > > due to the DFP, no? > > > > > I'll give it a try this week and share the observation. > Got my hands on Dell WD19TB. And it works! > > Here is lspci -t output with and without fix > > without fix: > +-03.1-[04-62]----00.0-[05-07]--+-02.0-[06]----00.0 > | \-04.0-[07]-- > With fix: > +-03.1-[04-62]----00.0-[05-62]--+-02.0-[06]----00.0 > | \-04.0-[07-62]-- > > I'll send out v3 with with splitting into 2/3 patches(Will see how it looks > good). Okay and you checked also that with the WD19TB (and its integrated xHCI) there are no additional boot delays because of the reset? > Any other comments, we can take it on v3. Sure.
On 1/5/2024 12:38 PM, Mika Westerberg wrote: > On Thu, Jan 04, 2024 at 07:20:05PM +0530, Sanath S wrote: >> On 1/3/2024 10:47 PM, Mika Westerberg wrote: >>> On Wed, Jan 03, 2024 at 07:45:56PM +0530, Sanath S wrote: >>>> On 12/21/2023 3:23 PM, Mika Westerberg wrote: >>>>> On Thu, Dec 21, 2023 at 03:01:50PM +0530, Sanath S wrote: >>>>>>> Sure. I'll check with these combinations. >>>>>> Can you name any docks that meets these requirements ? I'll try to get >>>>>> hold of it here and check. >>>>> Pretty much every Titan Ridge based dock. For instance Dell WD19TB. >>>>> >>>>> I have some hardware here that I can use to try it out too. >>>> It seems that issue is seen on Dell WD19TB too. So this fix may have to >>>> extended to TBT3 as well ? >>> Hm, what issue? I thought this works accross all the supported devices >>> due to the DFP, no? >>> >>>> I'll give it a try this week and share the observation. >> Got my hands on Dell WD19TB. And it works! >> >> Here is lspci -t output with and without fix >> >> without fix: >> +-03.1-[04-62]----00.0-[05-07]--+-02.0-[06]----00.0 >> | \-04.0-[07]-- >> With fix: >> +-03.1-[04-62]----00.0-[05-62]--+-02.0-[06]----00.0 >> | \-04.0-[07-62]-- >> >> I'll send out v3 with with splitting into 2/3 patches(Will see how it looks >> good). > Okay and you checked also that with the WD19TB (and its integrated xHCI) > there are no additional boot delays because of the reset? I did not observe any delays. As now we are not doing any discover as well before resetting. It was ready to use as soon as I see login screen. >> Any other comments, we can take it on v3. > Sure. I've sent v3.
On Mon, Jan 08, 2024 at 10:26:13AM +0530, Sanath S wrote: > > On 1/5/2024 12:38 PM, Mika Westerberg wrote: > > On Thu, Jan 04, 2024 at 07:20:05PM +0530, Sanath S wrote: > > > On 1/3/2024 10:47 PM, Mika Westerberg wrote: > > > > On Wed, Jan 03, 2024 at 07:45:56PM +0530, Sanath S wrote: > > > > > On 12/21/2023 3:23 PM, Mika Westerberg wrote: > > > > > > On Thu, Dec 21, 2023 at 03:01:50PM +0530, Sanath S wrote: > > > > > > > > Sure. I'll check with these combinations. > > > > > > > Can you name any docks that meets these requirements ? I'll try to get > > > > > > > hold of it here and check. > > > > > > Pretty much every Titan Ridge based dock. For instance Dell WD19TB. > > > > > > > > > > > > I have some hardware here that I can use to try it out too. > > > > > It seems that issue is seen on Dell WD19TB too. So this fix may have to > > > > > extended to TBT3 as well ? > > > > Hm, what issue? I thought this works accross all the supported devices > > > > due to the DFP, no? > > > > > > > > > I'll give it a try this week and share the observation. > > > Got my hands on Dell WD19TB. And it works! > > > > > > Here is lspci -t output with and without fix > > > > > > without fix: > > > +-03.1-[04-62]----00.0-[05-07]--+-02.0-[06]----00.0 > > > | \-04.0-[07]-- > > > With fix: > > > +-03.1-[04-62]----00.0-[05-62]--+-02.0-[06]----00.0 > > > | \-04.0-[07-62]-- > > > > > > I'll send out v3 with with splitting into 2/3 patches(Will see how it looks > > > good). > > Okay and you checked also that with the WD19TB (and its integrated xHCI) > > there are no additional boot delays because of the reset? > I did not observe any delays. As now we are not doing any discover as well > before resetting. Okay thanks!
diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c index fd49f86e0353..febd0b6972e3 100644 --- a/drivers/thunderbolt/tb.c +++ b/drivers/thunderbolt/tb.c @@ -2598,6 +2598,17 @@ static int tb_start(struct tb *tb) tb_switch_tmu_enable(tb->root_switch); /* Full scan to discover devices added before the driver was loaded. */ tb_scan_switch(tb->root_switch); + /* + * Boot firmware might have created tunnels of its own. Since we cannot + * be sure they are usable for us, Tear them down and reset the ports + * to handle it as new hotplug for USB4 routers. + */ + if (tb_switch_is_usb4(tb->root_switch)) { + tb_switch_discover_tunnels(tb->root_switch, + &tcm->tunnel_list, false); + tcm->hotplug_active = true; + return tb_switch_reset_ports(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 */
Boot firmware might have created tunnels of its own. Since we cannot be sure they are usable for us. Tear them down and reset the ports to handle it as a new hotplug for USB3 routers. Suggested-by: Mario Limonciello <mario.limonciello@amd.com> Signed-off-by: Sanath S <Sanath.S@amd.com> --- drivers/thunderbolt/tb.c | 11 +++++++++++ 1 file changed, 11 insertions(+)