Re: [PATCH 1/4] thunderbolt: add platform nhi driver

["mika.westerberg@linux.intel.com" <mika.westerberg@linux.intel.com>]

Hi,

On Wed, Nov 04, 2026 at 07:34:07AM +0000, Chunfeng Yun (云春峰) wrote:
> > 
> > > +{
> > > +	/*
> > > +	 * avoid device link fail for pcie tunnel
> > > +	 */
> > 
> > What's this? 
> explain why call these functions, without enable pm of nhi->dev,
> tb_acpi_add_link() -> device_link_add() will fail

Do you have the _DSD with the "usb4-host-interface"? tb_acpi_add_link()
should take care of runtime resuming both.

> > And the comment style is not the expected.
> will fix it.
> 
> > 
> > > +	pm_runtime_set_active(nhi->dev);
> > > +	pm_runtime_get_noresume(nhi->dev);
> > > +	devm_pm_runtime_enable(nhi->dev);
> > 
> > This means we never runtime suspend? Then this is not good.
> No, just enable the device's pm and get a count to link pcie/usb4's
> device, and will release later.
> 
> > 
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static irqreturn_t nhi_plat_isr(int irq, void *data)
> > > +{
> > > +	struct tb_nhi *nhi = data;
> > 
> > Did you run checkpatch.pl against this? If not please do.
> > 
> > > +	schedule_work(&nhi->interrupt_work);
> > > +	return IRQ_HANDLED;
> > > +}
> > > +
> > > +static int nhi_plat_init_irq(struct tb_nhi *nhi)
> > > +{
> > > +	int ret;
> > > +
> > > +	INIT_WORK(&nhi->interrupt_work, nhi_interrupt_work);
> > > +
> > > +	ret = devm_request_irq(nhi->dev, nhi->irq, nhi_plat_isr,
> > > IRQF_NO_SUSPEND,
> > > +			       dev_name(nhi->dev), nhi);
> > > +	if (ret)
> > > +		return dev_err_probe(nhi->dev, ret, "request irq
> > > failed\n");
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void nhi_plat_exit(struct tb_nhi *nhi)
> > > +{
> > > +	devm_free_irq(nhi->dev, nhi->irq, nhi);
> > > +	flush_work(&nhi->interrupt_work);
> > > +
> > > +	pm_runtime_put_noidle(nhi->dev);
> > > +	pm_runtime_disable(nhi->dev);
> > > +	pm_runtime_set_suspended(nhi->dev);
> > > +}
> > > +
> > > +static const struct tb_nhi_ops nhi_plat_default_ops = {
> > > +	.init = nhi_plat_init,
> > > +	.shutdown = nhi_plat_exit,
> > > +	.init_interrupts = nhi_plat_init_irq,
> > > +};
> > > +
> > > +static int nhi_plat_probe(struct platform_device *pdev)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	struct tb_nhi *nhi;
> > > +
> > > +	nhi = devm_kzalloc(dev, sizeof(*nhi), GFP_KERNEL);
> > > +	if (!nhi)
> > > +		return -ENOMEM;
> > > +
> > > +	nhi->dev = dev;
> > > +	nhi->ops = &nhi_plat_default_ops;
> > > +
> > > +	nhi->iobase = devm_platform_ioremap_resource(pdev, 0);
> > > +	if (IS_ERR(nhi->iobase))
> > > +		return PTR_ERR(nhi->iobase);
> > > +
> > > +	nhi->irq = platform_get_irq(pdev, 0);
> > > +	if (nhi->irq < 0)
> > > +		return dev_err_probe(dev, nhi->irq, "get irq
> > > failed\n");
> > > +
> > > +	return nhi_probe(nhi);
> > > +}
> > > +
> > > +static void nhi_plat_do_remove(struct platform_device *pdev, bool
> > > reset)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	struct tb *tb = platform_get_drvdata(pdev);
> > > +	struct tb_nhi *nhi = tb->nhi;
> > > +
> > > +	/*
> > > +	 * On system shutdown/reboot force a host router reset so
> > > the
> > > +	 * CM asserts DPR on connected tbt3 devices before the
> > > router tree
> > > +	 * is removed (see tb_stop()).
> > > +	 */
> > > +	if (reset)
> > > +		nhi->host_reset = true;
> > > +
> > > +	pm_runtime_get_sync(dev);
> > > +	pm_runtime_dont_use_autosuspend(dev);
> > > +
> > > +	tb_domain_remove(tb);
> > > +	wait_for_completion(&nhi->domain_released);
> > > +	nhi_shutdown(nhi);
> > > +}
> > > +
> > > +static void nhi_plat_remove(struct platform_device *pdev)
> > > +{
> > > +	nhi_plat_do_remove(pdev, false);
> > > +}
> > > +
> > > +static void nhi_plat_shutdown(struct platform_device *pdev)
> > > +{
> > > +	nhi_plat_do_remove(pdev, true);
> > > +}
> > > +
> > > +
> > > +#ifdef CONFIG_OF
> > > +static const struct of_device_id nhi_plat_of_match[] = {
> > > +	{ .compatible = "thunderbolt-platform" },
> > 
> > Not sure if this is correct? Should this have mediatek,... ?
> still need a common one, and also add a mediatek,thunderbolt?
> 
> but the current platform is running on uefi/acpi but not based on dts,
> 
> should I add a dt-binding for it?

Then no, that will be added when there is user for that.

> > > +	{ }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, nhi_plat_of_match);
> > > +#endif
> > > +
> > > +#ifdef CONFIG_ACPI
> > > +static const struct acpi_device_id nhi_plat_acpi_match[] = {
> > > +	{ "ACPI0015", 0 },
> > 
> > Same here, where is this ID defined in the ACPI spec?
> see acpi spec release 6.5, section 5.6.7 Device Class-Specific Objects,
> Table 5.205: ACPI Device IDs
> 
> ACPI0015
> USB4 host interface device. See Links to ACPI-Related Documents under
> the heading “USB4 Host Interface Specification”

Okay, I missed that. Then this is fine. Thanks.

> > > +	{ }
> > > +};
> > > +MODULE_DEVICE_TABLE(acpi, nhi_plat_acpi_match);
> > > +#endif
> > > +
> > > +static struct platform_driver nhi_plat_driver = {
> > > +	.probe = nhi_plat_probe,
> > > +	.remove = nhi_plat_remove,
> > > +	.shutdown = nhi_plat_shutdown,
> > > +	.driver = {
> > > +		.name = "thunderbolt",
> > > +		.of_match_table = of_match_ptr(nhi_plat_of_match),
> > > +		.acpi_match_table = ACPI_PTR(nhi_plat_acpi_match),
> > > +		.pm = &nhi_pm_ops,
> > > +	},
> > > +};
> > > +
> > > +module_platform_driver(nhi_plat_driver);
> > > +MODULE_ALIAS("platform:thunderbolt");
> > > +MODULE_DESCRIPTION("Thunderbolt/USB4 platform host interface
> > > driver");
> > > +MODULE_LICENSE("GPL v2");
> > > diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> > > index e86845762e22..6dfdc47e05d2 100644
> > > --- a/drivers/thunderbolt/tb.h
> > > +++ b/drivers/thunderbolt/tb.h
> > > @@ -768,8 +768,6 @@ extern const struct device_type
> > > tb_retimer_type;
> > >  extern const struct device_type tb_switch_type;
> > >  extern const struct device_type usb4_port_device_type;
> > >  
> > > -int tb_domain_init(void);
> > > -void tb_domain_exit(void);
> > >  int tb_xdomain_init(void);
> > >  void tb_xdomain_exit(void);
> > >  
> > > diff --git a/include/linux/thunderbolt.h
> > > b/include/linux/thunderbolt.h
> > > index 57502da29080..212c8fea6571 100644
> > > --- a/include/linux/thunderbolt.h
> > > +++ b/include/linux/thunderbolt.h
> > > @@ -532,6 +532,7 @@ struct tb_nhi {
> > >  	u32 *interrupt_mask;
> > >  	bool going_away;
> > >  	bool iommu_dma_protection;
> > > +	int irq;
> > 
> > You are missing kernel-doc.
> will fix it.
> 
> Thanks
> 
> > 
> > >  	struct work_struct interrupt_work;
> > >  	u32 hop_count;
> > >  	unsigned long quirks;
> > > -- 
> > > 2.43.0
> > 
>