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

[Mika Westerberg]

On Thu, Jan 31, 2019 at 09:26:49AM +0100, Lukas Wunner wrote:
> 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?

DP out adapters include TMU capability. We don't use it in Linux because
it is not really needed for legacy hardware AFAIK.

> 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?)

LR has the thing disabled which makes port space capability walk to
fail. That's why we had the offset hack previously. Access here is for
the SW CM (tb.c).

> 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.)

For legacy hardware I don't think we need to do anything for the TMU
(with the exception of the workaround during cap walk on LR/ER). For
TBT 3 hardware it is taken care by the firmware.

> > 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.

OK.