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

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

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

On Thu, Mar 19, 2026 at 12:46:39PM -0500, Bjorn Helgaas wrote:
> 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?

Good point. I'm adding "(Tegra20 through Tegra186)" to the PCI_TEGRA
option description.

> > +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.

You're right. I should've added a Signed-off-by for Manikanta. I'm
dropping this line here, though, because there's already a MODULE_AUTHOR
line at the end of the file.

> 
> > +#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.

I see a bunch of implementations that use custom defines, if you don't
mind I'll have a go and unifying this a little. Most implementations
seem to use usleep_range(90000, 100000) with 10 retries.

> 
> > +/* 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.

Fixed.

> > +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"?

I've renamed this field to "link_up" so it's less ambiguous.

> > +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

It won't work in this particular case because %pe needs an ERR_PTR-
encoded error code. For the other messages it's probably better to move
to dev_err_probe() as Krzysztof suggested.

> 
> > +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.

Me neither. "= {}" saves two extra lines, so I guess it wins.

> 
> > +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?

Yeah, looks about right. I'm not sure if we really need it in addition
to the polling above, but probably okay for an extra safe margin.

> > +		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 ...");

Guess everybody wants to use this now. I tend not to like it because it
has some issues with wrapping (because of the return, the _probe suffix
and the error code, it tends to go beyond 80 characters, cancelling out
many of the benefits). I see the extra benefit for cases where probe
deferral is a possibility, so that's where I usually do use it.

Anyway, I'll convert these since both you and Krzysztof requested it.

> > +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?

Yes, that's a good point.

Thanks,
Thierry

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