Re: [PATCH Internal] thunderbolt: Remove enabling/disabling TMU based on CLx

[Mika Westerberg <mika.westerberg@linux.intel.com>]

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.