Re: [PATCH v8 4/6] PCI: tegra: Add Tegra264 support

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

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

On Fri, Jul 24, 2026 at 01:58:15PM +0530, Manikanta Maddireddy wrote:
> 
> 
> On 16/07/26 10:37 pm, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Add a driver for the PCIe controller found on NVIDIA Tegra264 SoCs. The
> > driver is very small, with its main purpose being to set up the address
> > translation registers and then creating a standard PCI host using ECAM.
> > 
> > Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > Changes in v8:
> > - remove controller deinitialization, firmware does this already
> > - separately track hotplug support and link up state for clarity
> > - select pinctrl sleep state on suspend for symmetry with resume
> > - fail probe if the link is down and not hotplug-capable
> > - remove WAKE# IRQ support
> > - add err_ prefix to gotos
> > 
> > Changes in v7:
> > - select PCI_ECAM to satisfy the build dependency (Jonathan Hunter)
> > - remove pre-silicon support patch to avoid extra build dependency
> > 
> > Changes in v6:
> > - remove unneeded pm_runtime_disable() call (Sashiko)
> > - do not use noirq suspend/resume callbacks (Sashiko)
> > - wrap PM ops in pm_ptr() macro (Sashiko)
> > - use standard wait times with msleep() (Lukas Wunner)
> > - properly check errors for wake IRQs
> > - fix build failures /o\
> > 
> > Changes in v5:
> > - make PCIE_TEGRA264 symbol tristate
> > - drop dependency on PCI_MSI
> > - reorganize tegra264_pcie struct
> > - use standard wake-gpios property
> > - rename tegra264_pcie_bpmp_set_rp_state() to tegra264_pcie_power_off()
> > - use dev_err() instead of dev_info() for some error messages
> > - add clarifying comment as to why bandwidth requests aren't fatal
> > - address some compiler warnings on 32-bit physical address platforms
> > - drop needless comments
> > - explicitly deinitialize controller on suspend
> > - use devm_pm_runtime_active_enabled()
> > - rename "free" label to "free_ecam"
> > - use dev_err_probe() in more places
> > - reselect default pin state during resume, not probe
> > - return early on absence of wake GPIO
> > - simplify BW value calculation
> > 
> > Changes in v2:
> > - specify generations applicable for PCI_TEGRA driver to avoid confusion
> > - drop SPDX-FileCopyrightText tag
> > - rename link_state to link_up to clarify meaning
> > - replace memset() by an empty initializer
> > - sanity-check only enable BAR regions
> > - bring PCI link out of reset in case firmware didn't
> > - use common wait times instead of defining our own
> > - use core helpers to parse and print PCI link speed
> > - fix multi-line comment
> > - use dev_err_probe() more ubiquitously
> > - fix probe sequence and error cleanup
> > - use DEFINE_NOIRQ_DEV_PM_OPS() to avoid warnings for !PM_SUSPEND
> > - reuse more standard registers and remove unused register definitions
> > - use %pe and ERR_PTR() to print symbolic errors
> > - add signed-off-by from Manikanta as the original author
> > - add myself as author after significantly modifying the driver
> > ---
> >   drivers/pci/controller/Kconfig         |  10 +-
> >   drivers/pci/controller/Makefile        |   1 +
> >   drivers/pci/controller/pcie-tegra264.c | 485 +++++++++++++++++++++++++++++++++
> >   3 files changed, 495 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> > index 2247709ef6d6..3045c8aecc7e 100644
> > --- a/drivers/pci/controller/Kconfig
> > +++ b/drivers/pci/controller/Kconfig
> > @@ -255,7 +255,15 @@ config PCI_TEGRA
> >   	select IRQ_MSI_LIB
> >   	help
> >   	  Say Y here if you want support for the PCIe host controller found
> > -	  on NVIDIA Tegra SoCs.
> > +	  on NVIDIA Tegra SoCs (Tegra20 through Tegra186).
> > +
> > +config PCIE_TEGRA264
> > +	tristate "NVIDIA Tegra264 PCIe controller"
> > +	depends on ARCH_TEGRA || COMPILE_TEST
> > +	select PCI_ECAM
> > +	help
> > +	  Say Y here if you want support for the PCIe host controller found
> > +	  on NVIDIA Tegra264 SoCs.
> >   config PCIE_RCAR_HOST
> >   	bool "Renesas R-Car PCIe controller (host mode)"
> > diff --git a/drivers/pci/controller/Makefile b/drivers/pci/controller/Makefile
> > index ac8db283f0fe..d478743b5142 100644
> > --- a/drivers/pci/controller/Makefile
> > +++ b/drivers/pci/controller/Makefile
> > @@ -7,6 +7,7 @@ obj-$(CONFIG_PCI_HYPERV_INTERFACE) += pci-hyperv-intf.o
> >   obj-$(CONFIG_PCI_MVEBU) += pci-mvebu.o
> >   obj-$(CONFIG_PCI_AARDVARK) += pci-aardvark.o
> >   obj-$(CONFIG_PCI_TEGRA) += pci-tegra.o
> > +obj-$(CONFIG_PCIE_TEGRA264) += pcie-tegra264.o
> >   obj-$(CONFIG_PCI_RCAR_GEN2) += pci-rcar-gen2.o
> >   obj-$(CONFIG_PCIE_RCAR_HOST) += pcie-rcar.o pcie-rcar-host.o
> >   obj-$(CONFIG_PCIE_RCAR_EP) += pcie-rcar.o pcie-rcar-ep.o
> > diff --git a/drivers/pci/controller/pcie-tegra264.c b/drivers/pci/controller/pcie-tegra264.c
> > new file mode 100644
> > index 000000000000..7071f4b3b97b
> > --- /dev/null
> > +++ b/drivers/pci/controller/pcie-tegra264.c
> > @@ -0,0 +1,485 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * PCIe host controller driver for Tegra264 SoC
> > + *
> > + * Copyright (c) 2022-2026, NVIDIA CORPORATION. All rights reserved.
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> 
> GPIO header can be removed since WAKE gpio feature is removed.
> 
> > +#include <linux/init.h>
> > +#include <linux/interconnect.h>
> > +#include <linux/interrupt.h>
> 
> Interrupt header file can be removed.

Indeed.

[...]
> > +static int tegra264_pcie_probe(struct platform_device *pdev)
[...]
> > +	err = pm_runtime_get_sync(dev);
> > +	if (err < 0) {
> > +		dev_err_probe(dev, err, "failed to power on device\n");
> > +		goto err_put_bpmp;
> > +	}
> 
> pm_runtime_get_sync() increments the runtime-PM usage counter even when its
> resume operation returns an error. The failure path does not call
> pm_runtime_put_noidle() and bypasses err_put_pm. Prefer
> pm_runtime_resume_and_get(), whose failure semantics avoid the usage-counter
> imbalance, or explicitly balance the failed pm_runtime_get_sync().

Good point. I'll replace this by pm_runtime_resume_and_get().

[...]
> > +static int tegra264_pcie_suspend(struct device *dev)
> > +{
> > +	/*
> > +	 * No need to do anything device-specific here, BPMP handles the
> > +	 * details of taking the link into L2.
> > +	 */
> > +
> > +	return pinctrl_pm_select_sleep_state(dev);
> > +}
> > +
> > +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));
> > +		return err;
> > +	}
> > +
> > +	tegra264_pcie_init(pcie);
> > +
> > +	return 0;
> > +}
> > +
> > +static DEFINE_SIMPLE_DEV_PM_OPS(tegra264_pcie_pm_ops,
> > +				tegra264_pcie_suspend,
> > +				tegra264_pcie_resume);
> 
> PCI subsystem registered pm_noirq callbacks. So, controller should also
> register to pm_noirq callbacks.

Unfortunately that causes illegal uses of msleep() in no-IRQ context.
Sashiko complains about it and is right. I don't observe any runtime
issues with switching to the _noirq variants, but it doesn't seem right
to stick to it.

Is there any reason why this needs to be _noirq, or any benefits if it
was?

Thierry

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