| Message ID | 1687343842-17881-1-git-send-email-Sanju.Mehta@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [Internal] thunderbolt: Remove enabling/disabling TMU based on CLx | expand |
On Wed, Jun 21, 2023 at 05:37:22AM -0500, Sanjay R Mehta wrote:
> From: Sanath S <Sanath.S@amd.com>
Why "Internal"?
On Wed, Jun 21, 2023 at 05:37:22AM -0500, Sanjay R Mehta wrote: > From: Sanath S <Sanath.S@amd.com> > > Since TMU is enabled by default on Intel SOCs for USB4 before Alpine > Ridge, explicit enabling or disabling of TMU is not required. > > However, the current implementation of enabling or disabling TMU based > on CLx state is inadequate as not all SOCs with CLx disabled have TMU > enabled by default, such as AMD Yellow Carp and Pink Sardine. > > To address this, a quirk named "QUIRK_TMU_DEFAULT_ENABLED" is > implemented to skip the enabling or disabling of TMU for SOCs where it > is already enabled by default, such as Intel SOCs prior to Alpine Ridge. > > Fixes: 7af9da8ce8f9 ("thunderbolt: Add quirk to disable CLx") > Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com> > Signed-off-by: Sanath S <Sanath.S@amd.com> Wrong ordering :(
On 6/21/2023 4:22 PM, Greg KH wrote: > On Wed, Jun 21, 2023 at 05:37:22AM -0500, Sanjay R Mehta wrote: >> From: Sanath S <Sanath.S@amd.com> >> >> Since TMU is enabled by default on Intel SOCs for USB4 before Alpine >> Ridge, explicit enabling or disabling of TMU is not required. >> >> However, the current implementation of enabling or disabling TMU based >> on CLx state is inadequate as not all SOCs with CLx disabled have TMU >> enabled by default, such as AMD Yellow Carp and Pink Sardine. >> >> To address this, a quirk named "QUIRK_TMU_DEFAULT_ENABLED" is >> implemented to skip the enabling or disabling of TMU for SOCs where it >> is already enabled by default, such as Intel SOCs prior to Alpine Ridge. >> >> Fixes: 7af9da8ce8f9 ("thunderbolt: Add quirk to disable CLx") >> Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com> >> Signed-off-by: Sanath S <Sanath.S@amd.com> > > Wrong ordering :( My sincere apologies. Mistakenly I sent you the wrong patch. Will send the correct one. Please ignore this patch.
On Wed, Jun 21, 2023 at 05:37:22AM -0500, Sanjay R Mehta wrote: > From: Sanath S <Sanath.S@amd.com> > > Since TMU is enabled by default on Intel SOCs for USB4 before Alpine > Ridge, explicit enabling or disabling of TMU is not required. > > However, the current implementation of enabling or disabling TMU based > on CLx state is inadequate as not all SOCs with CLx disabled have TMU > enabled by default, such as AMD Yellow Carp and Pink Sardine. > > To address this, a quirk named "QUIRK_TMU_DEFAULT_ENABLED" is > implemented to skip the enabling or disabling of TMU for SOCs where it > is already enabled by default, such as Intel SOCs prior to Alpine Ridge. If it is enabled by default "enabling" it again should not be a problem. Can you elaborate this more?
On 6/21/2023 4:45 PM, Mika Westerberg wrote: > On Wed, Jun 21, 2023 at 05:37:22AM -0500, Sanjay R Mehta wrote: >> From: Sanath S <Sanath.S@amd.com> >> >> Since TMU is enabled by default on Intel SOCs for USB4 before Alpine >> Ridge, explicit enabling or disabling of TMU is not required. >> >> However, the current implementation of enabling or disabling TMU based >> on CLx state is inadequate as not all SOCs with CLx disabled have TMU >> enabled by default, such as AMD Yellow Carp and Pink Sardine. >> >> To address this, a quirk named "QUIRK_TMU_DEFAULT_ENABLED" is >> implemented to skip the enabling or disabling of TMU for SOCs where it >> is already enabled by default, such as Intel SOCs prior to Alpine Ridge. > > If it is enabled by default "enabling" it again should not be a problem. > Can you elaborate this more? Although that is correct, Mika, we are facing an issue of display flickering on Alpine Ridge and older device routers, from the second hotplug onwards. This issue arises as the TMU is enabled and disabled for each plug and unplug. Upon reviewing the old code, it appears that this issue was already addressed with the following code block: /* * No need to enable TMU on devices that don't support CLx since on * these devices e.g. Alpine Ridge and earlier, the TMU mode HiFi * bi-directional is enabled by default. */ if (!tb_switch_is_clx_supported(sw)) return 0; However, it seems that this code has been removed in recent changes, as the CLX-related code has been moved to a different file. Canonical has also reported this issue and has tested this patch that appears to resolve the issue.. If you agree, I will send the V2 patch which will address the comments from Greg.
On Wed, Jun 21, 2023 at 05:48:21PM +0530, Sanjay R Mehta wrote: > > > On 6/21/2023 4:45 PM, Mika Westerberg wrote: > > On Wed, Jun 21, 2023 at 05:37:22AM -0500, Sanjay R Mehta wrote: > >> From: Sanath S <Sanath.S@amd.com> > >> > >> Since TMU is enabled by default on Intel SOCs for USB4 before Alpine > >> Ridge, explicit enabling or disabling of TMU is not required. > >> > >> However, the current implementation of enabling or disabling TMU based > >> on CLx state is inadequate as not all SOCs with CLx disabled have TMU > >> enabled by default, such as AMD Yellow Carp and Pink Sardine. > >> > >> To address this, a quirk named "QUIRK_TMU_DEFAULT_ENABLED" is > >> implemented to skip the enabling or disabling of TMU for SOCs where it > >> is already enabled by default, such as Intel SOCs prior to Alpine Ridge. > > > > If it is enabled by default "enabling" it again should not be a problem. > > Can you elaborate this more? > > Although that is correct, Mika, we are facing an issue of display > flickering on Alpine Ridge and older device routers, from the second > hotplug onwards. This issue arises as the TMU is enabled and disabled > for each plug and unplug. Okay thanks for clarifying. > Upon reviewing the old code, it appears that this issue was already > addressed with the following code block: > > /* > * No need to enable TMU on devices that don't support CLx since on > * these devices e.g. Alpine Ridge and earlier, the TMU mode HiFi > * bi-directional is enabled by default. > */ > if (!tb_switch_is_clx_supported(sw)) > return 0; > > > However, it seems that this code has been removed in recent changes, as > the CLX-related code has been moved to a different file. Yes, I removed it because TMU code should not really be calling CLx functions. However, we have in tb_enable_tmu() this: /* If it is already enabled in correct mode, don't touch it */ if (tb_switch_tmu_is_enabled(sw)) return 0; and tb_switch_tmu_init() reads the hardware state so this code should basically leave TMU enabling untouched on Alpine Ridge for instance. I wonder if you can try with the latest "next" branch and see if it works there or you are already doing so? > Canonical has also reported this issue and has tested this patch that > appears to resolve the issue.. Right, however let's figure out if the problem is already solved with the recent code as above or if not why it does not work as expected. I don't really think we want to add any quirks for this because even in the USB4 spec the TMU of TBT3 devices is expected to be enabled already so this is expected functionality and the driver should be doing the right thing here.
On 6/21/2023 6:24 PM, Mika Westerberg wrote: > On Wed, Jun 21, 2023 at 05:48:21PM +0530, Sanjay R Mehta wrote: >> >> >> On 6/21/2023 4:45 PM, Mika Westerberg wrote: >>> On Wed, Jun 21, 2023 at 05:37:22AM -0500, Sanjay R Mehta wrote: >>>> From: Sanath S <Sanath.S@amd.com> >>>> >>>> Since TMU is enabled by default on Intel SOCs for USB4 before Alpine >>>> Ridge, explicit enabling or disabling of TMU is not required. >>>> >>>> However, the current implementation of enabling or disabling TMU based >>>> on CLx state is inadequate as not all SOCs with CLx disabled have TMU >>>> enabled by default, such as AMD Yellow Carp and Pink Sardine. >>>> >>>> To address this, a quirk named "QUIRK_TMU_DEFAULT_ENABLED" is >>>> implemented to skip the enabling or disabling of TMU for SOCs where it >>>> is already enabled by default, such as Intel SOCs prior to Alpine Ridge. >>> >>> If it is enabled by default "enabling" it again should not be a problem. >>> Can you elaborate this more? >> >> Although that is correct, Mika, we are facing an issue of display >> flickering on Alpine Ridge and older device routers, from the second >> hotplug onwards. This issue arises as the TMU is enabled and disabled >> for each plug and unplug. > > Okay thanks for clarifying. > >> Upon reviewing the old code, it appears that this issue was already >> addressed with the following code block: >> >> /* >> * No need to enable TMU on devices that don't support CLx since on >> * these devices e.g. Alpine Ridge and earlier, the TMU mode HiFi >> * bi-directional is enabled by default. >> */ >> if (!tb_switch_is_clx_supported(sw)) >> return 0; >> >> >> However, it seems that this code has been removed in recent changes, as >> the CLX-related code has been moved to a different file. > > Yes, I removed it because TMU code should not really be calling CLx > functions. > > However, we have in tb_enable_tmu() this: > > /* If it is already enabled in correct mode, don't touch it */ > if (tb_switch_tmu_is_enabled(sw)) > return 0; > > and tb_switch_tmu_init() reads the hardware state so this code should > basically leave TMU enabling untouched on Alpine Ridge for instance. I > wonder if you can try with the latest "next" branch and see if it works > there or you are already doing so? > Yes Mika, we have already verified with the latest thunderbolt/next branch. This patch is built on top of next branch. >> Canonical has also reported this issue and has tested this patch that >> appears to resolve the issue.. > > Right, however let's figure out if the problem is already solved with > the recent code as above or if not why it does not work as expected. I > don't really think we want to add any quirks for this because even in > the USB4 spec the TMU of TBT3 devices is expected to be enabled already > so this is expected functionality and the driver should be doing the > right thing here. Agree. we will have to see what is going wrong in this case.
On Wed, Jun 21, 2023 at 09:37:32PM +0530, Sanjay R Mehta wrote: > > > On 6/21/2023 6:24 PM, Mika Westerberg wrote: > > On Wed, Jun 21, 2023 at 05:48:21PM +0530, Sanjay R Mehta wrote: > >> > >> > >> On 6/21/2023 4:45 PM, Mika Westerberg wrote: > >>> On Wed, Jun 21, 2023 at 05:37:22AM -0500, Sanjay R Mehta wrote: > >>>> From: Sanath S <Sanath.S@amd.com> > >>>> > >>>> Since TMU is enabled by default on Intel SOCs for USB4 before Alpine > >>>> Ridge, explicit enabling or disabling of TMU is not required. > >>>> > >>>> However, the current implementation of enabling or disabling TMU based > >>>> on CLx state is inadequate as not all SOCs with CLx disabled have TMU > >>>> enabled by default, such as AMD Yellow Carp and Pink Sardine. > >>>> > >>>> To address this, a quirk named "QUIRK_TMU_DEFAULT_ENABLED" is > >>>> implemented to skip the enabling or disabling of TMU for SOCs where it > >>>> is already enabled by default, such as Intel SOCs prior to Alpine Ridge. > >>> > >>> If it is enabled by default "enabling" it again should not be a problem. > >>> Can you elaborate this more? > >> > >> Although that is correct, Mika, we are facing an issue of display > >> flickering on Alpine Ridge and older device routers, from the second > >> hotplug onwards. This issue arises as the TMU is enabled and disabled > >> for each plug and unplug. > > > > Okay thanks for clarifying. > > > >> Upon reviewing the old code, it appears that this issue was already > >> addressed with the following code block: > >> > >> /* > >> * No need to enable TMU on devices that don't support CLx since on > >> * these devices e.g. Alpine Ridge and earlier, the TMU mode HiFi > >> * bi-directional is enabled by default. > >> */ > >> if (!tb_switch_is_clx_supported(sw)) > >> return 0; > >> > >> > >> However, it seems that this code has been removed in recent changes, as > >> the CLX-related code has been moved to a different file. > > > > Yes, I removed it because TMU code should not really be calling CLx > > functions. > > > > However, we have in tb_enable_tmu() this: > > > > /* If it is already enabled in correct mode, don't touch it */ > > if (tb_switch_tmu_is_enabled(sw)) > > return 0; > > > > and tb_switch_tmu_init() reads the hardware state so this code should > > basically leave TMU enabling untouched on Alpine Ridge for instance. I > > wonder if you can try with the latest "next" branch and see if it works > > there or you are already doing so? > > > Yes Mika, we have already verified with the latest thunderbolt/next > branch. This patch is built on top of next branch. Okay. > >> Canonical has also reported this issue and has tested this patch that > >> appears to resolve the issue.. > > > > Right, however let's figure out if the problem is already solved with > > the recent code as above or if not why it does not work as expected. I > > don't really think we want to add any quirks for this because even in > > the USB4 spec the TMU of TBT3 devices is expected to be enabled already > > so this is expected functionality and the driver should be doing the > > right thing here. > > Agree. we will have to see what is going wrong in this case. You should be able to see in the dmesg (once thunderbolt.dyndbg=+p is passed in command line) what the initial state of TMU is and how the driver programs it or does it skip the enabling as expected.
On 6/21/2023 9:37 PM, Sanjay R Mehta wrote: > > > On 6/21/2023 6:24 PM, Mika Westerberg wrote: >> On Wed, Jun 21, 2023 at 05:48:21PM +0530, Sanjay R Mehta wrote: >>> >>> >>> On 6/21/2023 4:45 PM, Mika Westerberg wrote: >>>> On Wed, Jun 21, 2023 at 05:37:22AM -0500, Sanjay R Mehta wrote: >>>>> From: Sanath S <Sanath.S@amd.com> >>>>> >>>>> Since TMU is enabled by default on Intel SOCs for USB4 before Alpine >>>>> Ridge, explicit enabling or disabling of TMU is not required. >>>>> >>>>> However, the current implementation of enabling or disabling TMU based >>>>> on CLx state is inadequate as not all SOCs with CLx disabled have TMU >>>>> enabled by default, such as AMD Yellow Carp and Pink Sardine. >>>>> >>>>> To address this, a quirk named "QUIRK_TMU_DEFAULT_ENABLED" is >>>>> implemented to skip the enabling or disabling of TMU for SOCs where it >>>>> is already enabled by default, such as Intel SOCs prior to Alpine Ridge. >>>> >>>> If it is enabled by default "enabling" it again should not be a problem. >>>> Can you elaborate this more? >>> >>> Although that is correct, Mika, we are facing an issue of display >>> flickering on Alpine Ridge and older device routers, from the second >>> hotplug onwards. This issue arises as the TMU is enabled and disabled >>> for each plug and unplug. >> >> Okay thanks for clarifying. >> >>> Upon reviewing the old code, it appears that this issue was already >>> addressed with the following code block: >>> >>> /* >>> * No need to enable TMU on devices that don't support CLx since on >>> * these devices e.g. Alpine Ridge and earlier, the TMU mode HiFi >>> * bi-directional is enabled by default. >>> */ >>> if (!tb_switch_is_clx_supported(sw)) >>> return 0; >>> >>> >>> However, it seems that this code has been removed in recent changes, as >>> the CLX-related code has been moved to a different file. >> >> Yes, I removed it because TMU code should not really be calling CLx >> functions. >> >> However, we have in tb_enable_tmu() this: >> >> /* If it is already enabled in correct mode, don't touch it */ >> if (tb_switch_tmu_is_enabled(sw)) >> return 0; >> >> and tb_switch_tmu_init() reads the hardware state so this code should >> basically leave TMU enabling untouched on Alpine Ridge for instance. I >> wonder if you can try with the latest "next" branch and see if it works >> there or you are already doing so? >> > Yes Mika, we have already verified with the latest thunderbolt/next > branch. This patch is built on top of next branch. > >>> Canonical has also reported this issue and has tested this patch that >>> appears to resolve the issue.. >> >> Right, however let's figure out if the problem is already solved with >> the recent code as above or if not why it does not work as expected. I >> don't really think we want to add any quirks for this because even in >> the USB4 spec the TMU of TBT3 devices is expected to be enabled already >> so this is expected functionality and the driver should be doing the >> right thing here. > > Agree. we will have to see what is going wrong in this case. Hi Mika, When an unplug event occurs, the TMU is disabled by configuring the TSPacketInterval bits in TMU_RTR_CS_3 to 0 using the tb_switch_tmu_rate_write() API, followed by disabling the Time Synchronization Handshake using the DTS bit in TMU_ADP_CS_6. The code snippet for this functionality is present in the tb_switch_tmu_disable() function, as shown below: ``` tb_switch_tmu_rate_write(sw, TB_SWITCH_TMU_RATE_OFF); tb_port_tmu_time_sync_disable(up); ret = tb_port_tmu_time_sync_disable(down); if (ret) return ret; ``` However, we have observed that the tb_switch_tmu_rate_write() function fails to disable the TMU rate, and the code proceeds with disabling the Time Synchronization Handshake. To address this issue, we have modified the code to check the return value of the tb_switch_tmu_rate_write() function and only proceed with disabling the Time Synchronization Handshake if the TMU rate disabling succeeds. The updated code is as follows: ``` ret = tb_switch_tmu_rate_write(sw, tmu_rates[TB_SWITCH_TMU_MODE_OFF]); if (ret) return ret; tb_port_tmu_time_sync_disable(up); ret = tb_port_tmu_time_sync_disable(down); if (ret) return ret; ``` Please let us know your thoughts on this solution. - Sanjay
On Thu, Jul 06, 2023 at 07:18:38PM +0530, Sanjay R Mehta wrote: > > Agree. we will have to see what is going wrong in this case. > > Hi Mika, Hi, > When an unplug event occurs, the TMU is disabled by configuring the > TSPacketInterval bits in TMU_RTR_CS_3 to 0 using the > tb_switch_tmu_rate_write() API, followed by disabling the Time > Synchronization Handshake using the DTS bit in TMU_ADP_CS_6. The code > snippet for this functionality is present in the tb_switch_tmu_disable() > function, as shown below: > > ``` > tb_switch_tmu_rate_write(sw, TB_SWITCH_TMU_RATE_OFF); > tb_port_tmu_time_sync_disable(up); > ret = tb_port_tmu_time_sync_disable(down); > if (ret) > return ret; > ``` > > However, we have observed that the tb_switch_tmu_rate_write() function > fails to disable the TMU rate, and the code proceeds with disabling the > Time Synchronization Handshake. To address this issue, we have modified > the code to check the return value of the tb_switch_tmu_rate_write() > function and only proceed with disabling the Time Synchronization > Handshake if the TMU rate disabling succeeds. The updated code is as > follows: > > ``` > ret = tb_switch_tmu_rate_write(sw, tmu_rates[TB_SWITCH_TMU_MODE_OFF]); > if (ret) > return ret; > tb_port_tmu_time_sync_disable(up); > ret = tb_port_tmu_time_sync_disable(down); > if (ret) > return ret; > ``` > > Please let us know your thoughts on this solution. Makes sense to me.
diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c index aa6e11589c28..5fa9fbf095d2 100644 --- a/drivers/thunderbolt/tb.c +++ b/drivers/thunderbolt/tb.c @@ -923,6 +923,10 @@ static void tb_scan_port(struct tb_port *port) tb_switch_lane_bonding_enable(sw); /* Set the link configured */ tb_switch_configure_link(sw); + /* On Alpine Ridge and earlier Platforms, the TMU mode is enabled by default */ + if (sw->generation < 4 || tb_switch_is_tiger_lake(sw)) + sw->quirks |= QUIRK_TMU_DEFAULT_ENABLED; + /* * CL0s and CL1 are enabled and supported together. * Silently ignore CLx enabling in case CLx is not supported. diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h index 58df106aaa5e..9252d3875c08 100644 --- a/drivers/thunderbolt/tb.h +++ b/drivers/thunderbolt/tb.h @@ -27,6 +27,8 @@ #define QUIRK_FORCE_POWER_LINK_CONTROLLER BIT(0) /* Disable CLx if not supported */ #define QUIRK_NO_CLX BIT(1) +/* TMU enabled by default */ +#define QUIRK_TMU_DEFAULT_ENABLED BIT(2) /** * struct tb_nvm - Structure holding NVM information diff --git a/drivers/thunderbolt/tmu.c b/drivers/thunderbolt/tmu.c index c926fb71c43d..8e38ab5aae15 100644 --- a/drivers/thunderbolt/tmu.c +++ b/drivers/thunderbolt/tmu.c @@ -387,6 +387,9 @@ int tb_switch_tmu_disable(struct tb_switch *sw) if (sw->tmu.rate == TB_SWITCH_TMU_RATE_OFF) return 0; + if (sw->quirks & QUIRK_TMU_DEFAULT_ENABLED) + return 0; + if (tb_route(sw)) { bool unidirectional = sw->tmu.unidirectional; struct tb_port *down, *up; @@ -643,6 +646,9 @@ int tb_switch_tmu_enable(struct tb_switch *sw) if (tb_switch_tmu_is_enabled(sw)) return 0; + if (sw->quirks & QUIRK_TMU_DEFAULT_ENABLED) + return 0; + if (tb_switch_is_titan_ridge(sw) && unidirectional) { ret = tb_switch_tmu_disable_objections(sw); if (ret)