Re: [PATCH 03/28] thunderbolt: Enable TMU access when accessing port space on legacy devices

[Lukas Wunner <lukas@wunner.de>]

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