Messages in this thread | | | Date | Thu, 31 Jan 2019 09:26:49 +0100 | From | Lukas Wunner <> | Subject | Re: [PATCH 03/28] thunderbolt: Enable TMU access when accessing port space on legacy devices |
| |
On Wed, Jan 30, 2019 at 11:37:05AM +0200, Mika Westerberg wrote: > On Tue, Jan 29, 2019 at 10:58:58PM +0100, Lukas Wunner wrote: > > On Tue, Jan 29, 2019 at 06:01:18PM +0300, Mika Westerberg wrote: > > > +int tb_port_find_cap(struct tb_port *port, enum tb_port_cap cap) > > > +{ > > > + int ret; > > > + > > > + ret = tb_port_enable_tmu(port, true); > > > + if (ret) > > > + return ret; > > > + > > > + ret = __tb_port_find_cap(port, cap); > > > + > > > + tb_port_enable_tmu(port, false); > > > + > > > + return ret; > > > +} > > > > Would there be a downside to setting the TMU bit on all ports all the time > > (e.g. on switch enumeration)? > > You mean turn it on once and keep it like that? Quick experimentation > with a couple of LR devices did not show any side-effects so I guess we > could just turn it on once during the enumeration in affected devices.
I guess I'm confused because I thought TMU is used for clock synchronization across the daisy-chain, so I would expect it to be available only on ports of type TB_TYPE_PORT, not on adapters of any type. Yet a code comment in the current implementation of tb_port_find_cap() talks about TMU on DP out adapters?
This patch says that the bit that's being modified is used for "access" to the TMU. So apparently the bit is not about enablement of clock synchronization, but only about granting access to the TMU? (For whom?)
Doesn't TMU synchronization need to be enabled on all devices on the daisy chain for Thunderbolt to function? (BTW in MacBooks with dual Alpine Ridge, e.g. MacBookPro13,3, the two controllers' TMU in and out pins are wired together, presumably this allows for clock synchronization across Thunderbolt chips in the same machine.)
> Main reason it is now in cap.c is because we only need it to be enabled > during the cap walk and that allows keeping both workarounds in the same > file close to the place where it is used.
I was only wondering whether the bit should be set all the time because I was worrying that clock synchronization would otherwise not be enabled. And if tb_port_find_cap() is called frequently, setting the bit only once provides for a small performance improvement. OTOH it would then be necessary to restore the bit when coming out of (runtime or system) suspend. I guess I don't have a strong opinion on this because I don't fully grasp what the bit is for.
Thanks,
Lukas
| |