Re: [PATCH 16/19] thunderbolt: Add Apple Silicon support

[Sven Peter]

Hi,

thanks for the very fast and detailed review! Will address those points 
for v2, some comments below:

On 9/1/26 12:09, Mika Westerberg wrote:
> Hi,
>
> On Sun, Aug 30, 2026 at 10:19:34PM +0200, Sven Peter wrote:
> > Add a platform driver for the ACIO host router complex and Native Host
> > Interface (NHI) found on Apple Silicon SoCs.
[...]
> > +
> > +	/*
> > +	 * The firmware samples the ring configuration when the valid bit is set and E2E flow
> > +	 * control never engages when configured afterwards. Write everything at once like macOS.
> > +	 */
> > +	writel(flags | e2e_flags, options);
> I think we can do this flow in the generic parts too. It makes the driver
> follow the CM guide more closely.

sure, I can adjust that as well.

> > +}
> > +
> > +
> > +
> > +	ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(42));
> 42?

42! don't ask me why but that's the number of address lines they hooked 
up. I'll add a comment.

[...]
> > +
> > +	anhi->nhi.tx_rings = devm_kcalloc(&pdev->dev, anhi->nhi.hop_count,
> > +					  sizeof(*anhi->nhi.tx_rings), GFP_KERNEL);
> > +	anhi->nhi.rx_rings = devm_kcalloc(&pdev->dev, anhi->nhi.hop_count,
> > +					  sizeof(*anhi->nhi.rx_rings), GFP_KERNEL);
> > +	if (!anhi->nhi.tx_rings || !anhi->nhi.rx_rings) {
> > +		ret = -ENOMEM;
> > +		goto err;
> > +	}
> > +
> > +	anhi->nhi.dev = &pdev->dev;
> > +	init_completion(&anhi->nhi.domain_released);
> > +	anhi->tb = tb_probe(&anhi->nhi);
> You do need to setup device links for the tunneled protocols as well. Have
> you checked if they describe these in DT? I would expect so.

I'll look into that, the DT already has the ports which I'll need for 
notifications to PCIe and DP later anyway and I should be able to add 
the device links then as well. Right now only USB3 tunnels work (by 
accident: I'm not following what macOS does and will probably need a 
notification once I get to suspend/resume as well) but I'll see if I can 
already add device links. Either way, the dt-binding already has enough 
to describe the connections to dwc3/pcie/dp through the graph 
ports/endpoints.

> > +		tb_domain_put(anhi->tb);
> > +		wait_for_completion(&anhi->nhi.domain_released);
> > +		goto err;
> > +	}
> > +
> > +	mutex_lock(&anhi->tb->lock);
> > +
> > +	if (!anhi->tb->root_switch->drom) {
> > +		dev_err(anhi->dev, "No valid host DROM in the device tree\n");
> > +		ret = -EINVAL;
> > +		goto err_unlock_tb_domain;
> > +	}
> > +
> > +	cap_apple = tb_switch_find_vse_cap(anhi->tb->root_switch, TB_VSE_CAP_APPLE);
> This should not be done here. It belongs to the CM.
>
> We can do it in tb_start() for example, because only Apple silicon has the
> cap.

Ok, I'll find it in there and just expose it to apple.c somehow then to 
be able to write the cable info from here.

[...]

> > +	case TYPEC_THUNDERBOLT_SWITCH_TBT:
> Probably cannot affect these anymore but if can then _TB instead.

That was only introduced in the first patches but after Heikki's review 
it'll go away anyway. I'll make sure to TB instead of TBT anywhere else 
though!



Thanks,


Sven