Re: [PATCH v7 3/4] PCI: tegra: Add Tegra264 support

[Thierry Reding <thierry.reding@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 8181 bytes --]

On Thu, Jun 18, 2026 at 09:26:33AM +0200, Manivannan Sadhasivam wrote:
> On Wed, Jun 17, 2026 at 06:01:30PM +0200, Thierry Reding wrote:
[...]
> > +static int tegra264_pcie_parse_dt(struct tegra264_pcie *pcie)
> > +{
> > +	struct device *dev = pcie->dev;
> > +	int err;
> > +
> > +	pcie->wake_gpio = devm_gpiod_get_optional(dev, "wake", GPIOD_IN);
> > +	if (IS_ERR(pcie->wake_gpio))
> > +		return PTR_ERR(pcie->wake_gpio);
> > +
> > +	if (!pcie->wake_gpio)
> > +		return 0;
> > +
> > +	err = gpiod_to_irq(pcie->wake_gpio);
> > +	if (err < 0)
> > +		return dev_err_probe(dev, err, "failed to get wake IRQ\n");
> > +
> > +	pcie->wake_irq = (unsigned int)err;
> > +
> > +	err = devm_device_init_wakeup(dev);
> > +	if (err < 0)
> > +		return dev_err_probe(dev, err, "failed to initialize wakeup\n");
> > +
> > +	err = devm_pm_set_wake_irq(dev, pcie->wake_irq);
> > +	if (err < 0)
> > +		return dev_err_probe(dev, err, "failed to set wakeup IRQ\n");
> > +
> 
> I'd really like to get rid of custom WAKE# handling in the controller drivers.
> Krishna is trying to add generic WAKE# handling in the PCI core and I'd suggest
> you to take a look at the patches:
> https://lore.kernel.org/linux-pci/20260511-wakeirq_support-v10-2-c10af9c9eb8c@oss.qualcomm.com/
> 
> But this also means that you need to use switch to Root Port binding to move the
> Root Port properties out of the controller node. This is something we are
> mandating for the new controllers. Not a big change though...
> 
> Reference:
> 
> Documentation/devicetree/bindings/pci/spacemit,k1-pcie-host.yaml#n80

Okay, let me look into it.

[...]
> > +	err = devm_pm_runtime_set_active_enabled(dev);
> 
> I belive this has to come after pm_runtime_get_sync() because at this point, the
> controller is not enabled.

Okay, I'll change that.

> > +	if (err < 0) {
> > +		dev_err_probe(dev, err, "failed to enable runtime PM\n");
> > +		goto put_bpmp;
> > +	}
> > +
> > +	err = pm_runtime_get_sync(dev);
> > +	if (err < 0) {
> > +		dev_err_probe(dev, err, "failed to power on device\n");
> > +		goto put_bpmp;
> > +	}
> > +
> > +	/* sanity check that programmed ranges match what's in DT */
> > +	if (!tegra264_pcie_check_ranges(pdev)) {
> > +		err = -EINVAL;
> > +		goto put_pm;
> > +	}
> > +
> > +	pcie->cfg = pci_ecam_create(dev, res, bus->res, &pci_generic_ecam_ops);
> > +	if (IS_ERR(pcie->cfg)) {
> > +		err = dev_err_probe(dev, PTR_ERR(pcie->cfg),
> > +				    "failed to create ECAM\n");
> > +		goto put_pm;
> > +	}
> > +
> > +	bridge->ops = (struct pci_ops *)&pci_generic_ecam_ops.pci_ops;
> > +	bridge->sysdata = pcie->cfg;
> > +	pcie->ecam = pcie->cfg->win;
> > +
> > +	tegra264_pcie_init(pcie);
> > +
> > +	if (!pcie->link_up)
> > +		return 0;
> 
> So not hotplug support? Also, you do not want the driver to error out? I'm
> wondering what's the use then?

Hotplug is supported via pciehp. We skip probing the host bridge if no
link was detected because there's simply nothing attached to the port,
otherwise the link would've come up.

pcie->link_up is slightly misleading because it actually means something
along the lines of "link could be up at some point", either during probe
or after some hotplug event later on. It is only ever false if there's
no link during probe and hotplug isn't supported at all.

> > +
> > +	err = pci_host_probe(bridge);
> > +	if (err < 0) {
> > +		dev_err_probe(dev, err, "failed to register host\n");
> > +		goto free_ecam;
> > +	}
> > +
> > +	return 0;
> > +
> > +free_ecam:
> 
> Nit: Prefix 'err' for the labels.

I don't see any benefit of adding a prefix. Seems pretty redundant, but
I also don't feel too strongly about it, so I can add it.

> > +	pci_ecam_free(pcie->cfg);
> > +put_pm:
> > +	pm_runtime_put_sync(dev);
> > +put_bpmp:
> > +	tegra_bpmp_put(pcie->bpmp);
> > +
> > +	return err;
> > +}
> > +
> > +static void tegra264_pcie_remove(struct platform_device *pdev)
> > +{
> > +	struct tegra264_pcie *pcie = platform_get_drvdata(pdev);
> > +
> > +	/*
> > +	 * If we undo tegra264_pcie_init() then link goes down and need
> > +	 * controller reset to bring up the link again. Remove intention is
> > +	 * to clean up the root bridge and re-enumerate during bind.
> 
> But the controller will be consuming power even if PCIe is not used. Do you
> really want that? Can't tegra264_pcie_init() handle the initialization? I'm
> wondering how tegra264_pcie_deinit() in tegra264_pcie_suspend() works then.

I had to clarify this with the PCI team and they indicated that
tegra264_pcie_deinit() is actually useless and maybe even harmful. The
reason is that there's a processor on these boards (BPMP) that takes
care of power sequencing and it will automatically take the PCI links
to L2 on suspend and assert PERST#.

Another reason why we don't want to reset the entire controller is that
it is already set up during early boot by UEFI and the kernel driver
does not redo the entire initialization.

So yes, I think a little bit of power consumption is the compromise that
we will have to live with. In the bigger picture it's probably not going
to be noticeable in most cases, and given that these are embedded
platforms we'll likely see fixed configurations most of the time and the
case where we remove the PCIe host controller will not be common.

> > +	 */
> > +	pci_lock_rescan_remove();
> > +	pci_stop_root_bus(pcie->bridge->bus);
> > +	pci_remove_root_bus(pcie->bridge->bus);
> > +	pci_unlock_rescan_remove();
> > +
> > +	pm_runtime_put_sync(&pdev->dev);
> > +	tegra_bpmp_put(pcie->bpmp);
> > +	pci_ecam_free(pcie->cfg);
> > +}
> > +
> > +static int tegra264_pcie_suspend(struct device *dev)
> > +{
> > +	struct tegra264_pcie *pcie = dev_get_drvdata(dev);
> > +	int err;
> > +
> > +	tegra264_pcie_deinit(pcie);
> > +
> > +	if (pcie->wake_gpio && device_may_wakeup(dev)) {
> > +		err = enable_irq_wake(pcie->wake_irq);
> > +		if (err < 0)
> > +			dev_err(dev, "failed to enable wake IRQ: %pe\n",
> > +				ERR_PTR(err));
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int tegra264_pcie_resume(struct device *dev)
> > +{
> > +	struct tegra264_pcie *pcie = dev_get_drvdata(dev);
> > +	int err;
> > +
> > +	err = pinctrl_pm_select_default_state(dev);
> > +	if (err < 0)
> > +		dev_err(dev, "failed to configure sideband pins: %pe\n",
> > +			ERR_PTR(err));
> 
> Please remind me if you justified this manual pinctrl handling before.

This is just regular pinctrl PM boilerplate. There's plenty of other
drivers where we do this, too. We want this because some of the pins
get configured to non-default states on boot/resume, so doing this
here ensures they are muxed correctly.

> > +
> > +	if (pcie->wake_gpio && device_may_wakeup(dev)) {
> > +		err = disable_irq_wake(pcie->wake_irq);
> > +		if (err < 0)
> > +			dev_err(dev, "failed to disable wake IRQ: %pe\n",
> > +				ERR_PTR(err));
> > +	}
> > +
> > +	if (pcie->link_up == false)
> > +		return 0;
> 
> How is this possible? If 'pcie->link_up' was 'false' during probe(), then it is
> going to stay until tegra264_pcie_init() is called below.

Yes, this keeps confusing me, too. The purpose of this is to skip
initialization if we've already determined during probe that there is
never going to be a link. link_up will be false if and only if there was
no link during probe and we don't expect there ever will be a link
because there is no hotplug support.

Maybe a different name for link_up could help here? maybe_link_up
perhaps? I don't know if that's any clearer, but I also couldn't come up
with a better name.

Or maybe we should split this into two booleans, since we're essentially
trying to use one boolean to track a tristate. What we want to know is
if a link is truly up and if the controller should be kept powered for
the case where hotplug is supported.

I suppose we could do:

	bool link_up; /* track the link state */
	bool supports_hotplug; /* track whether port is hotpluggable */

That would make the code a bit cleaner and easier to follow.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]