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

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, Mar 19, 2026 at 05:01:08PM +0100, 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: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/pci/controller/Kconfig         |   8 +
>  drivers/pci/controller/Makefile        |   1 +
>  drivers/pci/controller/pcie-tegra264.c | 506 +++++++++++++++++++++++++
>  3 files changed, 515 insertions(+)
>  create mode 100644 drivers/pci/controller/pcie-tegra264.c
> 
> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> index e72ac6934379..36abee65eca1 100644
> --- a/drivers/pci/controller/Kconfig
> +++ b/drivers/pci/controller/Kconfig
> @@ -257,6 +257,14 @@ config PCI_TEGRA
>  	  Say Y here if you want support for the PCIe host controller found
>  	  on NVIDIA Tegra SoCs.

Should the PCI_TEGRA menu item ("NVIDIA Tegra PCIe controller") and
this text be clarified somehow to make them clearly separate?

> +config PCIE_TEGRA264
> +	bool "NVIDIA Tegra264 PCIe controller"
> +	depends on ARCH_TEGRA || COMPILE_TEST
> +	depends on PCI_MSI
> +	help
> +	  Say Y here if you want support for the PCIe host controller found
> +	  on NVIDIA Tegra264 SoCs.

> + * PCIe host controller driver for Tegra264 SoC
> + *
> + * Author: Manikanta Maddireddy <mmaddireddy@nvidia.com>

Manikanta doesn't appear in the signed-off-by or other tags above.
Not really an issue for me; just a question of whether this is
accurate.

> +#define PCIE_LINK_UP_DELAY	10000	/* 10 msec */
> +#define PCIE_LINK_UP_TIMEOUT	1000000	/* 1 s */

Use something from drivers/pci/pci.h if possible.  If not, please add
units suffixes to the name, e.g., it looks like these are in "_US".

PCIE_LINK_WAIT_MAX_RETRIES and PCIE_LINK_WAIT_SLEEP_MS look like
they're used in similar ways in other drivers.

> +/* XAL registers */
> +#define XAL_RC_ECAM_BASE_HI			0x00
> +#define XAL_RC_ECAM_BASE_LO			0x04
> +#define XAL_RC_ECAM_BUSMASK			0x08
> +#define XAL_RC_IO_BASE_HI			0x0c
> ...
> +#define XTL_RC_MGMT_CLOCK_CONTROL		0x47C

Inconsistent use of upper/lower-case hex with above.

> +struct tegra264_pcie {
> +	struct device *dev;
> +	bool link_state;

"link_state" being true/false doesn't read quite right because
true/false isn't a "state".  I guess true means "link is up"?

> +static int tegra264_pcie_parse_dt(struct tegra264_pcie *pcie)
> +{
> +	int err;
> +
> +	pcie->wake_gpio = devm_gpiod_get_optional(pcie->dev, "nvidia,pex-wake",
> +						  GPIOD_IN);
> +	if (IS_ERR(pcie->wake_gpio))
> +		return PTR_ERR(pcie->wake_gpio);
> +
> +	if (pcie->wake_gpio) {
> +		device_init_wakeup(pcie->dev, true);
> +
> +		err = gpiod_to_irq(pcie->wake_gpio);
> +		if (err < 0) {
> +			dev_err(pcie->dev, "failed to get wake IRQ: %d\n", err);

Does %pe work here (and below)?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/core-api/printk-formats.rst?id=v6.19#n96

> +static void tegra264_pcie_bpmp_set_rp_state(struct tegra264_pcie *pcie)
> +{
> +	struct tegra_bpmp_message msg;
> +	struct mrq_pcie_request req;
> +	int err;
> +
> +	memset(&req, 0, sizeof(req));

I think "struct mrq_pcie_request req = {}" is equivalent, also for
msg.  No real preference from me.

> +static void tegra264_pcie_init(struct tegra264_pcie *pcie)
> +{
> +	u32 value;
> +
> +	if (!tegra_is_silicon()) {
> +		dev_info(pcie->dev,
> +			 "skipping link state for PCIe #%u in simulation\n",
> +			 pcie->ctl_id);
> +		pcie->link_state = true;
> +		return;
> +	}
> +
> +	/* Poll every 10 msec for 1 sec to link up */
> +	readl_poll_timeout(pcie->ecam + XTL_RC_PCIE_CFG_LINK_CONTROL_STATUS,
> +		value, value & XTL_RC_PCIE_CFG_LINK_CONTROL_STATUS_DLL_ACTIVE,
> +		PCIE_LINK_UP_DELAY, PCIE_LINK_UP_TIMEOUT);
> +
> +	if (value & XTL_RC_PCIE_CFG_LINK_CONTROL_STATUS_DLL_ACTIVE) {
> +		/* Per PCIe r5.0, 6.6.1 wait for 100ms after DLL up */
> +		msleep(100);

Looks like possibly PCIE_RESET_CONFIG_WAIT_MS?

> +		dev_info(pcie->dev, "PCIe #%u link is up (speed: %d)\n",
> +			 pcie->ctl_id, (value & 0xf0000) >> 16);
> +		pcie->link_state = true;
> +		tegra264_pcie_icc_set(pcie);
> +	} else {
> +		dev_info(pcie->dev, "PCIe #%u link is down\n", pcie->ctl_id);
> +
> +		value = readl(pcie->xtl + XTL_RC_MGMT_CLOCK_CONTROL);
> +
> +		/** Set link state only when link fails and no hot-plug feature is present */

/* (not /**), and wrap to fit in 78 columns.

> +		if ((value & XTL_RC_MGMT_CLOCK_CONTROL_PEX_CLKREQ_I_N_PIN_USE_CONV_TO_PRSNT) == 0) {
> +			dev_info(pcie->dev,
> +				 "PCIe #%u link is down and not hotplug-capable, turning off\n",
> +				 pcie->ctl_id);
> +			tegra264_pcie_bpmp_set_rp_state(pcie);
> +			pcie->link_state = false;
> +		} else {
> +			pcie->link_state = true;
> +		}
> +	}
> +}
> +
> +static int tegra264_pcie_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct pci_host_bridge *bridge;
> +	struct tegra264_pcie *pcie;
> +	struct resource_entry *bus;
> +	struct resource *res;
> +	int err;
> +
> +	bridge = devm_pci_alloc_host_bridge(dev, sizeof(struct tegra264_pcie));
> +	if (!bridge) {
> +		dev_err(dev, "failed to allocate host bridge\n");
> +		return -ENOMEM;

Looks like this and others below could use:

  if (!bridge)
    return dev_err_probe(dev, -ENOMEM, "failed ...");

> +static const struct dev_pm_ops tegra264_pcie_pm_ops = {
> +	.resume_noirq = tegra264_pcie_resume_noirq,
> +	.suspend_noirq = tegra264_pcie_suspend_noirq,
> +};

Should this use DEFINE_NOIRQ_DEV_PM_OPS() or similar to avoid warnings
when CONFIG_PM_SLEEP, etc are not enabled?

> +static const struct of_device_id tegra264_pcie_of_match[] = {
> +	{
> +		.compatible = "nvidia,tegra264-pcie",
> +	},
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, tegra264_pcie_of_match);
> +
> +static struct platform_driver tegra264_pcie_driver = {
> +	.probe = tegra264_pcie_probe,
> +	.remove = tegra264_pcie_remove,
> +	.driver = {
> +		.name = "tegra264-pcie",
> +		.pm = &tegra264_pcie_pm_ops,
> +		.of_match_table = tegra264_pcie_of_match,
> +	},
> +};
> +module_platform_driver(tegra264_pcie_driver);
> +
> +MODULE_AUTHOR("Manikanta Maddireddy <mmaddireddy@nvidia.com>");
> +MODULE_DESCRIPTION("NVIDIA Tegra264 PCIe host controller driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.52.0
>