[{"id":3672836,"web_url":"http://patchwork.ozlabs.org/comment/3672836/","msgid":"","list_archive_url":null,"date":"2026-04-02T17:32:02","subject":"Re: [PATCH v4 3/4] PCI: tegra: Add Tegra264 support","submitter":{"id":78905,"url":"http://patchwork.ozlabs.org/api/people/78905/","name":"Manivannan Sadhasivam","email":"mani@kernel.org"},"content":"On Thu, Apr 02, 2026 at 04:27:37PM +0200, Thierry Reding wrote:\n> From: Thierry Reding \n> \n> Add a driver for the PCIe controller found on NVIDIA Tegra264 SoCs. The\n> driver is very small, with its main purpose being to set up the address\n> translation registers and then creating a standard PCI host using ECAM.\n> \n> Signed-off-by: Manikanta Maddireddy \n> Signed-off-by: Thierry Reding \n\nWhat is the rationale for adding a new driver? Can't you reuse the existing one?\nIf so, that should be mentioned in the description.\n\n> ---\n> Changes in v2:\n> - specify generations applicable for PCI_TEGRA driver to avoid confusion\n> - drop SPDX-FileCopyrightText tag\n> - rename link_state to link_up to clarify meaning\n> - replace memset() by an empty initializer\n> - sanity-check only enable BAR regions\n> - bring PCI link out of reset in case firmware didn't\n> - use common wait times instead of defining our own\n> - use core helpers to parse and print PCI link speed\n> - fix multi-line comment\n> - use dev_err_probe() more ubiquitously\n> - fix probe sequence and error cleanup\n> - use DEFINE_NOIRQ_DEV_PM_OPS() to avoid warnings for !PM_SUSPEND\n> - reuse more standard registers and remove unused register definitions\n> - use %pe and ERR_PTR() to print symbolic errors\n> - add signed-off-by from Manikanta as the original author\n> - add myself as author after significantly modifying the driver\n> ---\n> drivers/pci/controller/Kconfig | 10 +-\n> drivers/pci/controller/Makefile | 1 +\n> drivers/pci/controller/pcie-tegra264.c | 527 +++++++++++++++++++++++++++++++++\n> 3 files changed, 537 insertions(+), 1 deletion(-)\n> \n> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig\n> index 5aaed8ac6e44..6ead04f7bd6e 100644\n> --- a/drivers/pci/controller/Kconfig\n> +++ b/drivers/pci/controller/Kconfig\n> @@ -254,7 +254,15 @@ config PCI_TEGRA\n> \tselect IRQ_MSI_LIB\n> \thelp\n> \t Say Y here if you want support for the PCIe host controller found\n> -\t on NVIDIA Tegra SoCs.\n> +\t on NVIDIA Tegra SoCs (Tegra20 through Tegra186).\n> +\n> +config PCIE_TEGRA264\n> +\tbool \"NVIDIA Tegra264 PCIe controller\"\n\nThis driver seems to be using external MSI controller. So it can be built as a\nmodule. Also, you have the remove() callback for some reason.\n\n> +\tdepends on ARCH_TEGRA || COMPILE_TEST\n> +\tdepends on PCI_MSI\n\nWhy?\n\n> +\thelp\n> +\t Say Y here if you want support for the PCIe host controller found\n> +\t on NVIDIA Tegra264 SoCs.\n> \n> config PCIE_RCAR_HOST\n> \tbool \"Renesas R-Car PCIe controller (host mode)\"\n> diff --git a/drivers/pci/controller/Makefile b/drivers/pci/controller/Makefile\n> index ac8db283f0fe..d478743b5142 100644\n> --- a/drivers/pci/controller/Makefile\n> +++ b/drivers/pci/controller/Makefile\n> @@ -7,6 +7,7 @@ obj-$(CONFIG_PCI_HYPERV_INTERFACE) += pci-hyperv-intf.o\n> obj-$(CONFIG_PCI_MVEBU) += pci-mvebu.o\n> obj-$(CONFIG_PCI_AARDVARK) += pci-aardvark.o\n> obj-$(CONFIG_PCI_TEGRA) += pci-tegra.o\n> +obj-$(CONFIG_PCIE_TEGRA264) += pcie-tegra264.o\n> obj-$(CONFIG_PCI_RCAR_GEN2) += pci-rcar-gen2.o\n> obj-$(CONFIG_PCIE_RCAR_HOST) += pcie-rcar.o pcie-rcar-host.o\n> obj-$(CONFIG_PCIE_RCAR_EP) += pcie-rcar.o pcie-rcar-ep.o\n> diff --git a/drivers/pci/controller/pcie-tegra264.c b/drivers/pci/controller/pcie-tegra264.c\n> new file mode 100644\n> index 000000000000..3ce1ad971bdb\n> --- /dev/null\n> +++ b/drivers/pci/controller/pcie-tegra264.c\n> @@ -0,0 +1,527 @@\n> +// SPDX-License-Identifier: GPL-2.0-only\n> +/*\n> + * PCIe host controller driver for Tegra264 SoC\n> + *\n> + * Copyright (c) 2022-2026, NVIDIA CORPORATION. All rights reserved.\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \"../pci.h\"\n> +\n> +/* XAL registers */\n> +#define XAL_RC_ECAM_BASE_HI\t\t\t0x00\n> +#define XAL_RC_ECAM_BASE_LO\t\t\t0x04\n> +#define XAL_RC_ECAM_BUSMASK\t\t\t0x08\n> +#define XAL_RC_IO_BASE_HI\t\t\t0x0c\n> +#define XAL_RC_IO_BASE_LO\t\t\t0x10\n> +#define XAL_RC_IO_LIMIT_HI\t\t\t0x14\n> +#define XAL_RC_IO_LIMIT_LO\t\t\t0x18\n> +#define XAL_RC_MEM_32BIT_BASE_HI\t\t0x1c\n> +#define XAL_RC_MEM_32BIT_BASE_LO\t\t0x20\n> +#define XAL_RC_MEM_32BIT_LIMIT_HI\t\t0x24\n> +#define XAL_RC_MEM_32BIT_LIMIT_LO\t\t0x28\n> +#define XAL_RC_MEM_64BIT_BASE_HI\t\t0x2c\n> +#define XAL_RC_MEM_64BIT_BASE_LO\t\t0x30\n> +#define XAL_RC_MEM_64BIT_LIMIT_HI\t\t0x34\n> +#define XAL_RC_MEM_64BIT_LIMIT_LO\t\t0x38\n> +#define XAL_RC_BAR_CNTL_STANDARD\t\t0x40\n> +#define XAL_RC_BAR_CNTL_STANDARD_IOBAR_EN\tBIT(0)\n> +#define XAL_RC_BAR_CNTL_STANDARD_32B_BAR_EN\tBIT(1)\n> +#define XAL_RC_BAR_CNTL_STANDARD_64B_BAR_EN\tBIT(2)\n> +\n> +/* XTL registers */\n> +#define XTL_RC_PCIE_CFG_LINK_STATUS\t\t0x5a\n> +\n> +#define XTL_RC_MGMT_PERST_CONTROL\t\t0x218\n> +#define XTL_RC_MGMT_PERST_CONTROL_PERST_O_N\tBIT(0)\n> +\n> +#define XTL_RC_MGMT_CLOCK_CONTROL\t\t0x47c\n> +#define XTL_RC_MGMT_CLOCK_CONTROL_PEX_CLKREQ_I_N_PIN_USE_CONV_TO_PRSNT\tBIT(9)\n> +\n> +struct tegra264_pcie {\n> +\tstruct device *dev;\n> +\tbool link_up;\n\nKeep bool types at the end to avoid holes.\n\n> +\n> +\t/* I/O memory */\n> +\tvoid __iomem *xal;\n> +\tvoid __iomem *xtl;\n> +\tvoid __iomem *ecam;\n> +\n> +\t/* bridge configuration */\n> +\tstruct pci_config_window *cfg;\n> +\tstruct pci_host_bridge *bridge;\n> +\n> +\t/* wake IRQ */\n> +\tstruct gpio_desc *wake_gpio;\n> +\tunsigned int wake_irq;\n> +\n> +\t/* BPMP and bandwidth management */\n> +\tstruct icc_path *icc_path;\n> +\tstruct tegra_bpmp *bpmp;\n> +\tu32 ctl_id;\n> +};\n> +\n> +static int tegra264_pcie_parse_dt(struct tegra264_pcie *pcie)\n> +{\n> +\tint err;\n> +\n> +\tpcie->wake_gpio = devm_gpiod_get_optional(pcie->dev, \"nvidia,pex-wake\",\n\nYou should switch to standard 'wake-gpios' property.\n\n> +\t\t\t\t\t\t GPIOD_IN);\n> +\tif (IS_ERR(pcie->wake_gpio))\n> +\t\treturn PTR_ERR(pcie->wake_gpio);\n> +\n> +\tif (pcie->wake_gpio) {\n\nSince you are bailing out above, you don't need this check.\n\n> +\t\tdevice_init_wakeup(pcie->dev, true);\n> +\n> +\t\terr = gpiod_to_irq(pcie->wake_gpio);\n> +\t\tif (err < 0) {\n> +\t\t\tdev_err(pcie->dev, \"failed to get wake IRQ: %pe\\n\",\n> +\t\t\t\tERR_PTR(err));\n> +\t\t\treturn err;\n> +\t\t}\n> +\n> +\t\tpcie->wake_irq = (unsigned int)err;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +static void tegra264_pcie_bpmp_set_rp_state(struct tegra264_pcie *pcie)\n\nI don't think this function name is self explanatory. Looks like it is turning\noff the PCIe controller, so how about tegra264_pcie_power_off()?\n\n> +{\n> +\tstruct tegra_bpmp_message msg = {};\n> +\tstruct mrq_pcie_request req = {};\n> +\tint err;\n> +\n> +\treq.cmd = CMD_PCIE_RP_CONTROLLER_OFF;\n> +\treq.rp_ctrlr_off.rp_controller = pcie->ctl_id;\n> +\n> +\tmsg.mrq = MRQ_PCIE;\n> +\tmsg.tx.data = &req;\n> +\tmsg.tx.size = sizeof(req);\n> +\n> +\terr = tegra_bpmp_transfer(pcie->bpmp, &msg);\n> +\tif (err)\n> +\t\tdev_info(pcie->dev, \"failed to turn off PCIe #%u: %pe\\n\",\n\nWhy not dev_err()?\n\n> +\t\t\t pcie->ctl_id, ERR_PTR(err));\n> +\n> +\tif (msg.rx.ret)\n> +\t\tdev_info(pcie->dev, \"failed to turn off PCIe #%u: %d\\n\",\n\nSame here.\n\n> +\t\t\t pcie->ctl_id, msg.rx.ret);\n> +}\n> +\n> +static void tegra264_pcie_icc_set(struct tegra264_pcie *pcie)\n> +{\n> +\tu32 value, speed, width, bw;\n> +\tint err;\n> +\n> +\tvalue = readw(pcie->ecam + XTL_RC_PCIE_CFG_LINK_STATUS);\n> +\tspeed = FIELD_GET(PCI_EXP_LNKSTA_CLS, value);\n> +\twidth = FIELD_GET(PCI_EXP_LNKSTA_NLW, value);\n> +\n> +\tbw = width * (PCIE_SPEED2MBS_ENC(speed) / BITS_PER_BYTE);\n> +\tvalue = MBps_to_icc(bw);\n\nSo this becomes, 'width * (PCIE_SPEED2MBS_ENC(speed) / 8) * 1000 / 8'. But don't\nyou want, 'width * (PCIE_SPEED2MBS_ENC(speed)) * 1000 / 8'?\n\n> +\n> +\terr = icc_set_bw(pcie->icc_path, bw, bw);\n> +\tif (err < 0)\n> +\t\tdev_err(pcie->dev,\n> +\t\t\t\"failed to request bandwidth (%u MBps): %pe\\n\",\n> +\t\t\tbw, ERR_PTR(err));\n\nSo you don't want to error out if this fails?\n\n> +}\n> +\n> +/*\n> + * The various memory regions used by the controller (I/O, memory, ECAM) are\n> + * set up during early boot and have hardware-level protections in place. If\n> + * the DT ranges don't match what's been setup, the controller won't be able\n> + * to write the address endpoints properly, so make sure to validate that DT\n> + * and firmware programming agree on these ranges.\n> + */\n> +static bool tegra264_pcie_check_ranges(struct platform_device *pdev)\n> +{\n> +\tstruct tegra264_pcie *pcie = platform_get_drvdata(pdev);\n> +\tstruct device_node *np = pcie->dev->of_node;\n> +\tstruct of_pci_range_parser parser;\n> +\tphys_addr_t phys, limit, hi, lo;\n> +\tstruct of_pci_range range;\n> +\tstruct resource *res;\n> +\tbool status = true;\n> +\tu32 value;\n> +\tint err;\n> +\n> +\terr = of_pci_range_parser_init(&parser, np);\n> +\tif (err < 0)\n> +\t\treturn false;\n> +\n> +\tfor_each_of_pci_range(&parser, &range) {\n> +\t\tunsigned int addr_hi, addr_lo, limit_hi, limit_lo, enable;\n> +\t\tunsigned long type = range.flags & IORESOURCE_TYPE_BITS;\n> +\t\tphys_addr_t start, end, mask;\n> +\t\tconst char *region = NULL;\n> +\n> +\t\tend = range.cpu_addr + range.size - 1;\n> +\t\tstart = range.cpu_addr;\n> +\n> +\t\tswitch (type) {\n> +\t\tcase IORESOURCE_IO:\n> +\t\t\taddr_hi = XAL_RC_IO_BASE_HI;\n> +\t\t\taddr_lo = XAL_RC_IO_BASE_LO;\n> +\t\t\tlimit_hi = XAL_RC_IO_LIMIT_HI;\n> +\t\t\tlimit_lo = XAL_RC_IO_LIMIT_LO;\n> +\t\t\tenable = XAL_RC_BAR_CNTL_STANDARD_IOBAR_EN;\n> +\t\t\tmask = SZ_64K - 1;\n> +\t\t\tregion = \"I/O\";\n> +\t\t\tbreak;\n> +\n> +\t\tcase IORESOURCE_MEM:\n> +\t\t\tif (range.flags & IORESOURCE_PREFETCH) {\n> +\t\t\t\taddr_hi = XAL_RC_MEM_64BIT_BASE_HI;\n> +\t\t\t\taddr_lo = XAL_RC_MEM_64BIT_BASE_LO;\n> +\t\t\t\tlimit_hi = XAL_RC_MEM_64BIT_LIMIT_HI;\n> +\t\t\t\tlimit_lo = XAL_RC_MEM_64BIT_LIMIT_LO;\n> +\t\t\t\tenable = XAL_RC_BAR_CNTL_STANDARD_64B_BAR_EN;\n> +\t\t\t\tregion = \"prefetchable memory\";\n> +\t\t\t} else {\n> +\t\t\t\taddr_hi = XAL_RC_MEM_32BIT_BASE_HI;\n> +\t\t\t\taddr_lo = XAL_RC_MEM_32BIT_BASE_LO;\n> +\t\t\t\tlimit_hi = XAL_RC_MEM_32BIT_LIMIT_HI;\n> +\t\t\t\tlimit_lo = XAL_RC_MEM_32BIT_LIMIT_LO;\n> +\t\t\t\tenable = XAL_RC_BAR_CNTL_STANDARD_32B_BAR_EN;\n> +\t\t\t\tregion = \"memory\";\n> +\t\t\t}\n> +\n> +\t\t\tmask = SZ_1M - 1;\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\t/* not interested in anything that's not I/O or memory */\n> +\t\tif (!region)\n> +\t\t\tcontinue;\n> +\n> +\t\t/* don't check regions that haven't been enabled */\n> +\t\tvalue = readl(pcie->xal + XAL_RC_BAR_CNTL_STANDARD);\n> +\t\tif ((value & enable) == 0)\n> +\t\t\tcontinue;\n> +\n> +\t\thi = readl(pcie->xal + addr_hi);\n> +\t\tlo = readl(pcie->xal + addr_lo);\n> +\t\tphys = hi << 32 | lo;\n> +\n> +\t\thi = readl(pcie->xal + limit_hi);\n> +\t\tlo = readl(pcie->xal + limit_lo);\n> +\t\tlimit = hi << 32 | lo | mask;\n> +\n> +\t\tif (phys != start || limit != end) {\n> +\t\t\tdev_err(pcie->dev,\n> +\t\t\t\t\"%s region mismatch: %pap-%pap -> %pap-%pap\\n\",\n> +\t\t\t\tregion, &phys, &limit, &start, &end);\n> +\t\t\tstatus = false;\n> +\t\t}\n> +\t}\n> +\n> +\tres = platform_get_resource_byname(pdev, IORESOURCE_MEM, \"ecam\");\n> +\tif (!res)\n> +\t\treturn false;\n> +\n> +\thi = readl(pcie->xal + XAL_RC_ECAM_BASE_HI);\n> +\tlo = readl(pcie->xal + XAL_RC_ECAM_BASE_LO);\n> +\tphys = hi << 32 | lo;\n> +\n> +\tvalue = readl(pcie->xal + XAL_RC_ECAM_BUSMASK);\n> +\tlimit = phys + ((value + 1) << 20) - 1;\n> +\n> +\tif (phys != res->start || limit != res->end) {\n> +\t\tdev_err(pcie->dev,\n> +\t\t\t\"ECAM region mismatch: %pap-%pap -> %pap-%pap\\n\",\n> +\t\t\t&phys, &limit, &res->start, &res->end);\n> +\t\tstatus = false;\n> +\t}\n> +\n> +\treturn status;\n> +}\n> +\n> +static bool tegra264_pcie_link_up(struct tegra264_pcie *pcie,\n> +\t\t\t\t enum pci_bus_speed *speed)\n> +{\n> +\tu16 value = readw(pcie->ecam + XTL_RC_PCIE_CFG_LINK_STATUS);\n> +\n> +\tif (value & PCI_EXP_LNKSTA_DLLLA) {\n> +\t\tif (speed)\n> +\t\t\t*speed = pcie_link_speed[FIELD_GET(PCI_EXP_LNKSTA_CLS,\n> +\t\t\t\t\t\t\t value)];\n> +\n> +\t\treturn true;\n> +\t}\n> +\n> +\treturn false;\n> +}\n> +\n> +static void tegra264_pcie_init(struct tegra264_pcie *pcie)\n> +{\n> +\tenum pci_bus_speed speed;\n> +\tunsigned int i;\n> +\tu32 value;\n> +\n> +\t/* bring the link out of reset */\n\ns/link/controller or endpoint?\n\n> +\tvalue = readl(pcie->xtl + XTL_RC_MGMT_PERST_CONTROL);\n> +\tvalue |= XTL_RC_MGMT_PERST_CONTROL_PERST_O_N;\n> +\twritel(value, pcie->xtl + XTL_RC_MGMT_PERST_CONTROL);\n> +\n> +\tif (!tegra_is_silicon()) {\n\nThis looks like some pre-silicon validation thing. Do you really want it to be\npresent in the upstream driver?\n\n> +\t\tdev_info(pcie->dev,\n> +\t\t\t \"skipping link state for PCIe #%u in simulation\\n\",\n> +\t\t\t pcie->ctl_id);\n> +\t\tpcie->link_up = true;\n> +\t\treturn;\n> +\t}\n> +\n> +\tfor (i = 0; i < PCIE_LINK_WAIT_MAX_RETRIES; i++) {\n> +\t\tif (tegra264_pcie_link_up(pcie, NULL))\n> +\t\t\tbreak;\n> +\n> +\t\tusleep_range(PCIE_LINK_WAIT_US_MIN, PCIE_LINK_WAIT_US_MAX);\n> +\t}\n> +\n> +\tif (tegra264_pcie_link_up(pcie, &speed)) {\n\nWhy are you doing it for the second time?\n\n> +\t\t/* Per PCIe r5.0, 6.6.1 wait for 100ms after DLL up */\n\nNo need of this comment.\n\n> +\t\tmsleep(PCIE_RESET_CONFIG_WAIT_MS);\n> +\n> +\t\tdev_info(pcie->dev, \"PCIe #%u link is up (speed: %s)\\n\",\n> +\t\t\t pcie->ctl_id, pci_speed_string(speed));\n> +\t\ttegra264_pcie_icc_set(pcie);\n> +\t\tpcie->link_up = true;\n> +\t} else {\n> +\t\tdev_info(pcie->dev, \"PCIe #%u link is down\\n\", pcie->ctl_id);\n> +\n> +\t\tvalue = readl(pcie->xtl + XTL_RC_MGMT_CLOCK_CONTROL);\n> +\n> +\t\t/*\n> +\t\t * Set link state only when link fails and no hot-plug feature\n> +\t\t * is present.\n> +\t\t */\n> +\t\tif ((value & XTL_RC_MGMT_CLOCK_CONTROL_PEX_CLKREQ_I_N_PIN_USE_CONV_TO_PRSNT) == 0) {\n> +\t\t\tdev_info(pcie->dev,\n> +\t\t\t\t \"PCIe #%u link is down and not hotplug-capable, turning off\\n\",\n> +\t\t\t\t pcie->ctl_id);\n> +\t\t\ttegra264_pcie_bpmp_set_rp_state(pcie);\n> +\t\t\tpcie->link_up = false;\n> +\t\t} else {\n> +\t\t\tpcie->link_up = true;\n> +\t\t}\n> +\t}\n> +}\n> +\n> +static int tegra264_pcie_probe(struct platform_device *pdev)\n> +{\n> +\tstruct device *dev = &pdev->dev;\n> +\tstruct pci_host_bridge *bridge;\n> +\tstruct tegra264_pcie *pcie;\n> +\tstruct resource_entry *bus;\n> +\tstruct resource *res;\n> +\tint err;\n> +\n> +\tbridge = devm_pci_alloc_host_bridge(dev, sizeof(struct tegra264_pcie));\n> +\tif (!bridge)\n> +\t\treturn dev_err_probe(dev, -ENOMEM,\n> +\t\t\t\t \"failed to allocate host bridge\\n\");\n> +\n> +\tpcie = pci_host_bridge_priv(bridge);\n> +\tplatform_set_drvdata(pdev, pcie);\n> +\tpcie->bridge = bridge;\n> +\tpcie->dev = dev;\n> +\n> +\terr = pinctrl_pm_select_default_state(dev);\n\nI questioned this before:\nhttps://lore.kernel.org/linux-pci/o5sxxdikdjwd76zsedvkpsl54nw6wrhopwsflt43y5st67mrub@uuw3yfjfqthd/\n\n> +\tif (err < 0)\n> +\t\treturn dev_err_probe(dev, err,\n> +\t\t\t\t \"failed to configure sideband pins\\n\");\n> +\n> +\terr = tegra264_pcie_parse_dt(pcie);\n> +\tif (err < 0)\n> +\t\treturn dev_err_probe(dev, err, \"failed to parse device tree\");\n> +\n> +\tpcie->xal = devm_platform_ioremap_resource_byname(pdev, \"xal\");\n> +\tif (IS_ERR(pcie->xal))\n> +\t\treturn dev_err_probe(dev, PTR_ERR(pcie->xal),\n> +\t\t\t\t \"failed to map XAL memory\\n\");\n> +\n> +\tpcie->xtl = devm_platform_ioremap_resource_byname(pdev, \"xtl-pri\");\n> +\tif (IS_ERR(pcie->xtl))\n> +\t\treturn dev_err_probe(dev, PTR_ERR(pcie->xtl),\n> +\t\t\t\t \"failed to map XTL-PRI memory\\n\");\n> +\n> +\tbus = resource_list_first_type(&bridge->windows, IORESOURCE_BUS);\n> +\tif (!bus)\n> +\t\treturn dev_err_probe(dev, -ENODEV,\n> +\t\t\t\t \"failed to get bus resources\\n\");\n> +\n> +\tres = platform_get_resource_byname(pdev, IORESOURCE_MEM, \"ecam\");\n> +\tif (!res)\n> +\t\treturn dev_err_probe(dev, -ENXIO,\n> +\t\t\t\t \"failed to get ECAM resource\\n\");\n> +\n> +\tpcie->icc_path = devm_of_icc_get(&pdev->dev, \"write\");\n> +\tif (IS_ERR(pcie->icc_path))\n> +\t\treturn dev_err_probe(&pdev->dev, PTR_ERR(pcie->icc_path),\n> +\t\t\t\t \"failed to get ICC\");\n> +\n> +\t/*\n> +\t * Parse BPMP property only for silicon, as interaction with BPMP is\n> +\t * not needed for other platforms.\n> +\t */\n> +\tif (tegra_is_silicon()) {\n> +\t\tpcie->bpmp = tegra_bpmp_get_with_id(dev, &pcie->ctl_id);\n> +\t\tif (IS_ERR(pcie->bpmp))\n> +\t\t\treturn dev_err_probe(dev, PTR_ERR(pcie->bpmp),\n> +\t\t\t\t\t \"failed to get BPMP\\n\");\n> +\t}\n> +\n\npm_runtime_set_active()\n\n> +\tpm_runtime_enable(dev);\n\ndevm_pm_runtime_enable()?\n\n> +\tpm_runtime_get_sync(dev);\n> +\n> +\t/* sanity check that programmed ranges match what's in DT */\n> +\tif (!tegra264_pcie_check_ranges(pdev)) {\n> +\t\terr = -EINVAL;\n> +\t\tgoto put_pm;\n> +\t}\n> +\n> +\tpcie->cfg = pci_ecam_create(dev, res, bus->res, &pci_generic_ecam_ops);\n> +\tif (IS_ERR(pcie->cfg)) {\n> +\t\terr = dev_err_probe(dev, PTR_ERR(pcie->cfg),\n> +\t\t\t\t \"failed to create ECAM\\n\");\n> +\t\tgoto put_pm;\n> +\t}\n> +\n> +\tbridge->ops = (struct pci_ops *)&pci_generic_ecam_ops.pci_ops;\n> +\tbridge->sysdata = pcie->cfg;\n> +\tpcie->ecam = pcie->cfg->win;\n> +\n> +\ttegra264_pcie_init(pcie);\n> +\n> +\tif (!pcie->link_up)\n> +\t\tgoto free;\n\ngoto free_ecam;\n\n> +\n> +\terr = pci_host_probe(bridge);\n> +\tif (err < 0) {\n> +\t\tdev_err(dev, \"failed to register host: %pe\\n\", ERR_PTR(err));\n\ndev_err_probe()\n\n> +\t\tgoto free;\n> +\t}\n> +\n> +\treturn err;\n\nreturn 0;\n\n> +\n> +free:\n> +\tpci_ecam_free(pcie->cfg);\n> +put_pm:\n> +\tpm_runtime_put_sync(dev);\n> +\tpm_runtime_disable(dev);\n> +\n> +\tif (tegra_is_silicon())\n> +\t\ttegra_bpmp_put(pcie->bpmp);\n> +\n> +\treturn err;\n> +}\n> +\n> +static void tegra264_pcie_remove(struct platform_device *pdev)\n> +{\n> +\tstruct tegra264_pcie *pcie = platform_get_drvdata(pdev);\n> +\n> +\t/*\n> +\t * If we undo tegra264_pcie_init() then link goes down and need\n> +\t * controller reset to bring up the link again. Remove intention is\n> +\t * to clean up the root bridge and re-enumerate during bind.\n> +\t */\n> +\tpci_lock_rescan_remove();\n> +\tpci_stop_root_bus(pcie->bridge->bus);\n> +\tpci_remove_root_bus(pcie->bridge->bus);\n> +\tpci_unlock_rescan_remove();\n> +\n> +\tpm_runtime_put_sync(&pdev->dev);\n> +\tpm_runtime_disable(&pdev->dev);\n> +\n> +\tif (tegra_is_silicon())\n> +\t\ttegra_bpmp_put(pcie->bpmp);\n> +\n> +\tpci_ecam_free(pcie->cfg);\n> +}\n> +\n> +static int tegra264_pcie_suspend_noirq(struct device *dev)\n> +{\n> +\tstruct tegra264_pcie *pcie = dev_get_drvdata(dev);\n> +\tint err;\n> +\n> +\tif (pcie->wake_gpio && device_may_wakeup(dev)) {\n> +\t\terr = enable_irq_wake(pcie->wake_irq);\n> +\t\tif (err < 0)\n> +\t\t\tdev_err(dev, \"failed to enable wake IRQ: %pe\\n\",\n> +\t\t\t\tERR_PTR(err));\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +static int tegra264_pcie_resume_noirq(struct device *dev)\n> +{\n> +\tstruct tegra264_pcie *pcie = dev_get_drvdata(dev);\n> +\tint err;\n> +\n> +\tif (pcie->wake_gpio && device_may_wakeup(dev)) {\n> +\t\terr = disable_irq_wake(pcie->wake_irq);\n> +\t\tif (err < 0)\n> +\t\t\tdev_err(dev, \"failed to disable wake IRQ: %pe\\n\",\n> +\t\t\t\tERR_PTR(err));\n> +\t}\n> +\n> +\tif (pcie->link_up == false)\n> +\t\treturn 0;\n> +\n> +\ttegra264_pcie_init(pcie);\n> +\n\nWhy do you need init() here without deinit() in tegra264_pcie_suspend_noirq()?\n\n- Mani","headers":{"Return-Path":"\n ","X-Original-To":["incoming@patchwork.ozlabs.org","linux-pci@vger.kernel.org"],"Delivered-To":"patchwork-incoming@legolas.ozlabs.org","Authentication-Results":["legolas.ozlabs.org;\n\tdkim=pass (2048-bit key;\n unprotected) header.d=kernel.org header.i=@kernel.org header.a=rsa-sha256\n header.s=k20201202 header.b=uujFBkm8;\n\tdkim-atps=neutral","legolas.ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org\n (client-ip=2600:3c0a:e001:db::12fc:5321; helo=sea.lore.kernel.org;\n envelope-from=linux-pci+bounces-51761-incoming=patchwork.ozlabs.org@vger.kernel.org;\n receiver=patchwork.ozlabs.org)","smtp.subspace.kernel.org;\n\tdkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org\n header.b=\"uujFBkm8\"","smtp.subspace.kernel.org;\n arc=none smtp.client-ip=10.30.226.201"],"Received":["from sea.lore.kernel.org (sea.lore.kernel.org\n [IPv6:2600:3c0a:e001:db::12fc:5321])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\t key-exchange x25519 server-signature ECDSA (secp384r1) server-digest SHA384)\n\t(No client certificate requested)\n\tby legolas.ozlabs.org (Postfix) with ESMTPS id 4fmpsd0vsbz1xtJ\n\tfor ; Fri, 03 Apr 2026 04:35:33 +1100 (AEDT)","from smtp.subspace.kernel.org (conduit.subspace.kernel.org\n [100.90.174.1])\n\tby sea.lore.kernel.org (Postfix) with ESMTP id D9B983006B76\n\tfor ; Thu, 2 Apr 2026 17:32:24 +0000 (UTC)","from localhost.localdomain (localhost.localdomain [127.0.0.1])\n\tby smtp.subspace.kernel.org (Postfix) with ESMTP id 39F5735B62A;\n\tThu, 2 Apr 2026 17:32:24 +0000 (UTC)","from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org\n [10.30.226.201])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby smtp.subspace.kernel.org (Postfix) with ESMTPS id 1434133A717;\n\tThu, 2 Apr 2026 17:32:23 +0000 (UTC)","by smtp.kernel.org (Postfix) with ESMTPSA id CFE74C116C6;\n\tThu, 2 Apr 2026 17:32:06 +0000 (UTC)"],"ARC-Seal":"i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116;\n\tt=1775151144; cv=none;\n b=cnvXCZ79q0uvte0a59yHloGwWBggGPZwsZT2rwML1B18SRGQp2d20ZYer8cZO2wc5u1Myd3RZ2m7hl69mQ495TZQeDq8Ls3lkUVxMAlIRwLV4mBmiQFqXLRGLOezoJfhIWKm9si3lHfaCVt5VEy9O1V+V9ELI6Bu6aunElXnBGk=","ARC-Message-Signature":"i=1; a=rsa-sha256; d=subspace.kernel.org;\n\ts=arc-20240116; t=1775151144; c=relaxed/simple;\n\tbh=jRSIv8LioujVpT978cG++b4luP/nCgUJUTakC1huHso=;\n\th=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version:\n\t Content-Type:Content-Disposition:In-Reply-To;\n b=s/+b5gGYSE1q+I4/c3cuAt9RSnHN1AfYev2E/cq937XvCoLJk7bc4IdCj79x99jFoUeV2S6QriHJD7QZrN2Y0PsRwwIA6FyvgGkeOvDwG7/wC2qunN7uoEUbcK8jrQ8sCc2Npe+X30tytoiOJMGLaRFE/D3VAnZaBEMLBdy+qLI=","ARC-Authentication-Results":"i=1; smtp.subspace.kernel.org;\n dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org\n header.b=uujFBkm8; arc=none smtp.client-ip=10.30.226.201","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org;\n\ts=k20201202; t=1775151143;\n\tbh=jRSIv8LioujVpT978cG++b4luP/nCgUJUTakC1huHso=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uujFBkm8qRDxJlbtJTRBJz0//a/OE09OeLOdoNWJKpBUotc9f4pZEqpnIHcXkS0/B\n\t IokxgkzrvgGRFsqY4AvMiFYbhtfgf0/oZTNX0XcVSxuwvvmQQbV/dJGEQBmCUFKbtc\n\t hbB9mUtt0tYS0zTwgCuWgsDVYPtNfL5NwYCScLAaUJu5rfYu+fisL5qCzM30JLAODG\n\t AF1wEV+PKUTaKiIjP9O0/2MrX/Nq8Ya72p26h3YoIIRhcoL4gWXASOxItoxIrY06DZ\n\t uyPTpemhbz239kDAPncJS7TuNJEucfsI6itm8P9apcjM/mIvCKlngLblnc62YheoAI\n\t QDsulNPyHB4Mw==","Date":"Thu, 2 Apr 2026 23:02:02 +0530","From":"Manivannan Sadhasivam ","To":"Thierry Reding ","Cc":"Bjorn Helgaas ,\n Lorenzo Pieralisi , Krzysztof =?utf-8?q?Wilczy?=\n\t=?utf-8?q?=C5=84ski?= ,\n Rob Herring , Krzysztof Kozlowski ,\n Conor Dooley ,\n Thierry Reding ,\n Jonathan Hunter ,\n Karthikeyan Mitran ,\n Hou Zhiqiang ,\n Thomas Petazzoni ,\n Pali =?utf-8?b?Um9ow6Fy?= ,\n Michal Simek , Kevin Xie ,\n linux-pci@vger.kernel.org, devicetree@vger.kernel.org,\n linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org,\n linux-arm-kernel@lists.infradead.org, Thierry Reding ,\n Manikanta Maddireddy ","Subject":"Re: [PATCH v4 3/4] PCI: tegra: Add Tegra264 support","Message-ID":"","References":"<20260402-tegra264-pcie-v4-0-21e2e19987e8@nvidia.com>\n <20260402-tegra264-pcie-v4-3-21e2e19987e8@nvidia.com>","Precedence":"bulk","X-Mailing-List":"linux-pci@vger.kernel.org","List-Id":"","List-Subscribe":"","List-Unsubscribe":"","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20260402-tegra264-pcie-v4-3-21e2e19987e8@nvidia.com>"}},{"id":3674029,"web_url":"http://patchwork.ozlabs.org/comment/3674029/","msgid":"","list_archive_url":null,"date":"2026-04-07T09:38:28","subject":"Re: [PATCH v4 3/4] PCI: tegra: Add Tegra264 support","submitter":{"id":92481,"url":"http://patchwork.ozlabs.org/api/people/92481/","name":"Thierry Reding","email":"thierry.reding@kernel.org"},"content":"On Thu, Apr 02, 2026 at 11:02:02PM +0530, Manivannan Sadhasivam wrote:\n> On Thu, Apr 02, 2026 at 04:27:37PM +0200, Thierry Reding wrote:\n> > From: Thierry Reding \n> > \n> > Add a driver for the PCIe controller found on NVIDIA Tegra264 SoCs. The\n> > driver is very small, with its main purpose being to set up the address\n> > translation registers and then creating a standard PCI host using ECAM.\n> > \n> > Signed-off-by: Manikanta Maddireddy \n> > Signed-off-by: Thierry Reding \n> \n> What is the rationale for adding a new driver? Can't you reuse the existing one?\n> If so, that should be mentioned in the description.\n\nWhich existing one? Tegra PCI controllers for previou generations\n(Tegra194 and Tegra234) were DesignWare IP, but Tegra264 is an internal\nIP, so the programming is entirely different. I'll add something to that\neffect to the commit message.\n\n> > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig\n> > index 5aaed8ac6e44..6ead04f7bd6e 100644\n> > --- a/drivers/pci/controller/Kconfig\n> > +++ b/drivers/pci/controller/Kconfig\n> > @@ -254,7 +254,15 @@ config PCI_TEGRA\n> > \tselect IRQ_MSI_LIB\n> > \thelp\n> > \t Say Y here if you want support for the PCIe host controller found\n> > -\t on NVIDIA Tegra SoCs.\n> > +\t on NVIDIA Tegra SoCs (Tegra20 through Tegra186).\n> > +\n> > +config PCIE_TEGRA264\n> > +\tbool \"NVIDIA Tegra264 PCIe controller\"\n> \n> This driver seems to be using external MSI controller. So it can be built as a\n> module. Also, you have the remove() callback for some reason.\n\nOkay, I can turn this into a tristate symbol.\n\n> > +\tdepends on ARCH_TEGRA || COMPILE_TEST\n> > +\tdepends on PCI_MSI\n> \n> Why?\n\nI suppose it's not necessary in the sense of it being a build\ndependency. At runtime, however, the root complex is not useful if PCI\nMSI is not enabled. We can drop this dependency and rely on .config to\nhave it enabled as needed.\n\n> > diff --git a/drivers/pci/controller/pcie-tegra264.c b/drivers/pci/controller/pcie-tegra264.c\n> > new file mode 100644\n> > index 000000000000..3ce1ad971bdb\n> > --- /dev/null\n> > +++ b/drivers/pci/controller/pcie-tegra264.c\n[...]\n> > +struct tegra264_pcie {\n> > +\tstruct device *dev;\n> > +\tbool link_up;\n> \n> Keep bool types at the end to avoid holes.\n\nDone.\n\n> > +static int tegra264_pcie_parse_dt(struct tegra264_pcie *pcie)\n> > +{\n> > +\tint err;\n> > +\n> > +\tpcie->wake_gpio = devm_gpiod_get_optional(pcie->dev, \"nvidia,pex-wake\",\n> \n> You should switch to standard 'wake-gpios' property.\n\nWill do.\n\n> > +\t\t\t\t\t\t GPIOD_IN);\n> > +\tif (IS_ERR(pcie->wake_gpio))\n> > +\t\treturn PTR_ERR(pcie->wake_gpio);\n> > +\n> > +\tif (pcie->wake_gpio) {\n> \n> Since you are bailing out above, you don't need this check.\n\nI think we still want to have this check to handle the case of optional\nwake GPIOs. Not all controllers may have this wired up and\ndevm_gpiod_get_optional() will return NULL (not an ERR_PTR()-encoded\nerror) if the wake-gpios property is missing.\n\n> > +static void tegra264_pcie_bpmp_set_rp_state(struct tegra264_pcie *pcie)\n> \n> I don't think this function name is self explanatory. Looks like it is turning\n> off the PCIe controller, so how about tegra264_pcie_power_off()?\n\nAgreed. The name is a relic from when this was potentially being used to\ntoggle on and off the controller. But it's only used for disabling, so\ntegra264_pcie__power_off() sounds much better.\n\n> > +{\n> > +\tstruct tegra_bpmp_message msg = {};\n> > +\tstruct mrq_pcie_request req = {};\n> > +\tint err;\n> > +\n> > +\treq.cmd = CMD_PCIE_RP_CONTROLLER_OFF;\n> > +\treq.rp_ctrlr_off.rp_controller = pcie->ctl_id;\n> > +\n> > +\tmsg.mrq = MRQ_PCIE;\n> > +\tmsg.tx.data = &req;\n> > +\tmsg.tx.size = sizeof(req);\n> > +\n> > +\terr = tegra_bpmp_transfer(pcie->bpmp, &msg);\n> > +\tif (err)\n> > +\t\tdev_info(pcie->dev, \"failed to turn off PCIe #%u: %pe\\n\",\n> \n> Why not dev_err()?\n> \n> > +\t\t\t pcie->ctl_id, ERR_PTR(err));\n> > +\n> > +\tif (msg.rx.ret)\n> > +\t\tdev_info(pcie->dev, \"failed to turn off PCIe #%u: %d\\n\",\n> \n> Same here.\n\nThese are not fatal errors and are safe to ignore. dev_err() seemed too\nstrong for this. They also really shouldn't happen. Though I now realize\nthat's a bad argument, or rather, actually an argument for making them\ndev_err() so that they do stand out if they really should happen.\n\n> \n> > +\t\t\t pcie->ctl_id, msg.rx.ret);\n> > +}\n> > +\n> > +static void tegra264_pcie_icc_set(struct tegra264_pcie *pcie)\n> > +{\n> > +\tu32 value, speed, width, bw;\n> > +\tint err;\n> > +\n> > +\tvalue = readw(pcie->ecam + XTL_RC_PCIE_CFG_LINK_STATUS);\n> > +\tspeed = FIELD_GET(PCI_EXP_LNKSTA_CLS, value);\n> > +\twidth = FIELD_GET(PCI_EXP_LNKSTA_NLW, value);\n> > +\n> > +\tbw = width * (PCIE_SPEED2MBS_ENC(speed) / BITS_PER_BYTE);\n> > +\tvalue = MBps_to_icc(bw);\n> \n> So this becomes, 'width * (PCIE_SPEED2MBS_ENC(speed) / 8) * 1000 / 8'. But don't\n> you want, 'width * (PCIE_SPEED2MBS_ENC(speed)) * 1000 / 8'?\n\nThis is M*B*ps_to_icc(), not M*b*ps_to_icc(), so we do in fact get the\nlatter. I almost fell for this as well because I got confused by some of\nthese macros being all-caps and other times the case actually mattering.\n\n> > +\terr = icc_set_bw(pcie->icc_path, bw, bw);\n> > +\tif (err < 0)\n> > +\t\tdev_err(pcie->dev,\n> > +\t\t\t\"failed to request bandwidth (%u MBps): %pe\\n\",\n> > +\t\t\tbw, ERR_PTR(err));\n> \n> So you don't want to error out if this fails?\n\nNo. This is not a fatal error and the system will continue to work,\nalbeit perhaps at suboptimal performance. Given that Ethernet and mass\nstorage are connected to these, a failure to set the bandwidth and\nerroring out here may leave the system unusable, but continuing on would\nlet the system boot and update firmware, kernel or whatever to recover.\n\nI'll add a comment explaining this.\n\n[...]\n> > +static void tegra264_pcie_init(struct tegra264_pcie *pcie)\n> > +{\n> > +\tenum pci_bus_speed speed;\n> > +\tunsigned int i;\n> > +\tu32 value;\n> > +\n> > +\t/* bring the link out of reset */\n> \n> s/link/controller or endpoint?\n\nThis controls the PERST# signal, so I guess \"endpoint\" would be more\ncorrect.\n\n> > +\tvalue = readl(pcie->xtl + XTL_RC_MGMT_PERST_CONTROL);\n> > +\tvalue |= XTL_RC_MGMT_PERST_CONTROL_PERST_O_N;\n> > +\twritel(value, pcie->xtl + XTL_RC_MGMT_PERST_CONTROL);\n> > +\n> > +\tif (!tegra_is_silicon()) {\n> \n> This looks like some pre-silicon validation thing. Do you really want it to be\n> present in the upstream driver?\n\nAt this point there is silicon for this chip, but we've been trying to\nget some of the pre-silicon code merged upstream as well because\noccasionally people will want to run upstream on simulation, even after\nsilicon is available. At other times we may want to reuse these drivers\non future chips during pre-silicon validation.\n\nObviously there needs to be a balance. We don't want to have excessive\namounts of code specifically for pre-silicon validation, but in\nrelatively simple cases like this it is useful.\n\n> \n> > +\t\tdev_info(pcie->dev,\n> > +\t\t\t \"skipping link state for PCIe #%u in simulation\\n\",\n> > +\t\t\t pcie->ctl_id);\n> > +\t\tpcie->link_up = true;\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tfor (i = 0; i < PCIE_LINK_WAIT_MAX_RETRIES; i++) {\n> > +\t\tif (tegra264_pcie_link_up(pcie, NULL))\n> > +\t\t\tbreak;\n> > +\n> > +\t\tusleep_range(PCIE_LINK_WAIT_US_MIN, PCIE_LINK_WAIT_US_MAX);\n> > +\t}\n> > +\n> > +\tif (tegra264_pcie_link_up(pcie, &speed)) {\n> \n> Why are you doing it for the second time?\n\nIt's just a last-resort check to see if it's really not come up after\nthe retries. Also, in this call we're actually interested in retrieving\nthe detected link speed.\n\n> \n> > +\t\t/* Per PCIe r5.0, 6.6.1 wait for 100ms after DLL up */\n> \n> No need of this comment.\n\nFair enough. This was perhaps more useful in earlier versions of the\npatch before the line below used the standardize wait time.\n\n[...]\n> > +static int tegra264_pcie_probe(struct platform_device *pdev)\n> > +{\n> > +\tstruct device *dev = &pdev->dev;\n> > +\tstruct pci_host_bridge *bridge;\n> > +\tstruct tegra264_pcie *pcie;\n> > +\tstruct resource_entry *bus;\n> > +\tstruct resource *res;\n> > +\tint err;\n> > +\n> > +\tbridge = devm_pci_alloc_host_bridge(dev, sizeof(struct tegra264_pcie));\n> > +\tif (!bridge)\n> > +\t\treturn dev_err_probe(dev, -ENOMEM,\n> > +\t\t\t\t \"failed to allocate host bridge\\n\");\n> > +\n> > +\tpcie = pci_host_bridge_priv(bridge);\n> > +\tplatform_set_drvdata(pdev, pcie);\n> > +\tpcie->bridge = bridge;\n> > +\tpcie->dev = dev;\n> > +\n> > +\terr = pinctrl_pm_select_default_state(dev);\n> \n> I questioned this before:\n> https://lore.kernel.org/linux-pci/o5sxxdikdjwd76zsedvkpsl54nw6wrhopwsflt43y5st67mrub@uuw3yfjfqthd/\n\nI'll remove this. Looks like we should be fine with just relying on the\ndefault state being set by the pinctrl core. We might need to move it\ninto the resume callback.\n\n> > +\tif (err < 0)\n> > +\t\treturn dev_err_probe(dev, err,\n> > +\t\t\t\t \"failed to configure sideband pins\\n\");\n> > +\n> > +\terr = tegra264_pcie_parse_dt(pcie);\n> > +\tif (err < 0)\n> > +\t\treturn dev_err_probe(dev, err, \"failed to parse device tree\");\n> > +\n> > +\tpcie->xal = devm_platform_ioremap_resource_byname(pdev, \"xal\");\n> > +\tif (IS_ERR(pcie->xal))\n> > +\t\treturn dev_err_probe(dev, PTR_ERR(pcie->xal),\n> > +\t\t\t\t \"failed to map XAL memory\\n\");\n> > +\n> > +\tpcie->xtl = devm_platform_ioremap_resource_byname(pdev, \"xtl-pri\");\n> > +\tif (IS_ERR(pcie->xtl))\n> > +\t\treturn dev_err_probe(dev, PTR_ERR(pcie->xtl),\n> > +\t\t\t\t \"failed to map XTL-PRI memory\\n\");\n> > +\n> > +\tbus = resource_list_first_type(&bridge->windows, IORESOURCE_BUS);\n> > +\tif (!bus)\n> > +\t\treturn dev_err_probe(dev, -ENODEV,\n> > +\t\t\t\t \"failed to get bus resources\\n\");\n> > +\n> > +\tres = platform_get_resource_byname(pdev, IORESOURCE_MEM, \"ecam\");\n> > +\tif (!res)\n> > +\t\treturn dev_err_probe(dev, -ENXIO,\n> > +\t\t\t\t \"failed to get ECAM resource\\n\");\n> > +\n> > +\tpcie->icc_path = devm_of_icc_get(&pdev->dev, \"write\");\n> > +\tif (IS_ERR(pcie->icc_path))\n> > +\t\treturn dev_err_probe(&pdev->dev, PTR_ERR(pcie->icc_path),\n> > +\t\t\t\t \"failed to get ICC\");\n> > +\n> > +\t/*\n> > +\t * Parse BPMP property only for silicon, as interaction with BPMP is\n> > +\t * not needed for other platforms.\n> > +\t */\n> > +\tif (tegra_is_silicon()) {\n> > +\t\tpcie->bpmp = tegra_bpmp_get_with_id(dev, &pcie->ctl_id);\n> > +\t\tif (IS_ERR(pcie->bpmp))\n> > +\t\t\treturn dev_err_probe(dev, PTR_ERR(pcie->bpmp),\n> > +\t\t\t\t\t \"failed to get BPMP\\n\");\n> > +\t}\n> > +\n> \n> pm_runtime_set_active()\n> \n> > +\tpm_runtime_enable(dev);\n> \n> devm_pm_runtime_enable()?\n\nLooks like I can even use devm_pm_runtime_set_active_enabled() to\ncombine the two.\n\n> \n> > +\tpm_runtime_get_sync(dev);\n> > +\n> > +\t/* sanity check that programmed ranges match what's in DT */\n> > +\tif (!tegra264_pcie_check_ranges(pdev)) {\n> > +\t\terr = -EINVAL;\n> > +\t\tgoto put_pm;\n> > +\t}\n> > +\n> > +\tpcie->cfg = pci_ecam_create(dev, res, bus->res, &pci_generic_ecam_ops);\n> > +\tif (IS_ERR(pcie->cfg)) {\n> > +\t\terr = dev_err_probe(dev, PTR_ERR(pcie->cfg),\n> > +\t\t\t\t \"failed to create ECAM\\n\");\n> > +\t\tgoto put_pm;\n> > +\t}\n> > +\n> > +\tbridge->ops = (struct pci_ops *)&pci_generic_ecam_ops.pci_ops;\n> > +\tbridge->sysdata = pcie->cfg;\n> > +\tpcie->ecam = pcie->cfg->win;\n> > +\n> > +\ttegra264_pcie_init(pcie);\n> > +\n> > +\tif (!pcie->link_up)\n> > +\t\tgoto free;\n> \n> goto free_ecam;\n\nIt's not clear to me, but are you suggesting to rename the existing\n\"free\" label to \"free_ecam\"? I can do that.\n\n> > +\terr = pci_host_probe(bridge);\n> > +\tif (err < 0) {\n> > +\t\tdev_err(dev, \"failed to register host: %pe\\n\", ERR_PTR(err));\n> \n> dev_err_probe()\n\nOkay.\n\n> \n> > +\t\tgoto free;\n> > +\t}\n> > +\n> > +\treturn err;\n> \n> return 0;\n\nDone.\n\n[...]\n> > +static int tegra264_pcie_resume_noirq(struct device *dev)\n> > +{\n> > +\tstruct tegra264_pcie *pcie = dev_get_drvdata(dev);\n> > +\tint err;\n> > +\n> > +\tif (pcie->wake_gpio && device_may_wakeup(dev)) {\n> > +\t\terr = disable_irq_wake(pcie->wake_irq);\n> > +\t\tif (err < 0)\n> > +\t\t\tdev_err(dev, \"failed to disable wake IRQ: %pe\\n\",\n> > +\t\t\t\tERR_PTR(err));\n> > +\t}\n> > +\n> > +\tif (pcie->link_up == false)\n> > +\t\treturn 0;\n> > +\n> > +\ttegra264_pcie_init(pcie);\n> > +\n> \n> Why do you need init() here without deinit() in tegra264_pcie_suspend_noirq()?\n\nThat's because when we come out of suspend the link may have gone down\nagain, so we need to take the endpoint out of reset to retrigger the\nlink training. I think we could possibly explicitly clear that PERST_O_N\nbit in the PERST_CONTROL register in a new tegra264_pcie_deinit() to\nmirror what tegra264_pcie_init() does, but it's automatically done by\nfirmware anyway, so not needed.\n\nThierry","headers":{"Return-Path":"\n ","X-Original-To":["incoming@patchwork.ozlabs.org","linux-pci@vger.kernel.org"],"Delivered-To":"patchwork-incoming@legolas.ozlabs.org","Authentication-Results":["legolas.ozlabs.org;\n\tdkim=pass (2048-bit key;\n unprotected) header.d=kernel.org header.i=@kernel.org header.a=rsa-sha256\n header.s=k20201202 header.b=Rk87Mz5+;\n\tdkim-atps=neutral","legolas.ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org\n (client-ip=172.234.253.10; helo=sea.lore.kernel.org;\n envelope-from=linux-pci+bounces-52017-incoming=patchwork.ozlabs.org@vger.kernel.org;\n receiver=patchwork.ozlabs.org)","smtp.subspace.kernel.org;\n\tdkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org\n header.b=\"Rk87Mz5+\"","smtp.subspace.kernel.org;\n arc=none smtp.client-ip=10.30.226.201"],"Received":["from sea.lore.kernel.org (sea.lore.kernel.org [172.234.253.10])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\t key-exchange x25519)\n\t(No client certificate requested)\n\tby legolas.ozlabs.org (Postfix) with ESMTPS id 4fqh551CZ5z1xtJ\n\tfor ; Tue, 07 Apr 2026 19:40:25 +1000 (AEST)","from smtp.subspace.kernel.org (conduit.subspace.kernel.org\n [100.90.174.1])\n\tby sea.lore.kernel.org (Postfix) with ESMTP id 6C917300B107\n\tfor ; Tue, 7 Apr 2026 09:38:32 +0000 (UTC)","from localhost.localdomain (localhost.localdomain [127.0.0.1])\n\tby smtp.subspace.kernel.org (Postfix) with ESMTP id AF28B3A3E9A;\n\tTue, 7 Apr 2026 09:38:31 +0000 (UTC)","from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org\n [10.30.226.201])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby smtp.subspace.kernel.org (Postfix) with ESMTPS id 8AD4738AC93;\n\tTue, 7 Apr 2026 09:38:31 +0000 (UTC)","by smtp.kernel.org (Postfix) with ESMTPSA id 9EB40C116C6;\n\tTue, 7 Apr 2026 09:38:30 +0000 (UTC)"],"ARC-Seal":"i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116;\n\tt=1775554711; cv=none;\n b=kWxRJVLMDudJPxI+n5M5TPWfas8OS9rIoz7SYaqM3t6xWdRjiieaBvaqPDy/sytwgE+C/Ku8agTPIYRBwPK7jJS4MXzlGZ/a5yrk7W9iQGIjh7EVP7N0CstmZszPBmXFdOfvJQC6Dlidoid7tAaanGneHubwI3sakbldMAB6Ikg=","ARC-Message-Signature":"i=1; a=rsa-sha256; d=subspace.kernel.org;\n\ts=arc-20240116; t=1775554711; c=relaxed/simple;\n\tbh=Uclf5ajKGlirtFS+exys4PG+9RGNZbkt5MlvGo9XdQ0=;\n\th=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version:\n\t Content-Type:Content-Disposition:In-Reply-To;\n b=BSkAcIb6HgnY/+Wg2lAyN5WhlKQCZxXzxTsC9C8CGq8VhdEynxIcIt8LirUkK4DG5qo2cMAO636gDArsA1nm3FOp5R4vlL8BRJ9FkfDM0ikPDetxYFULN7m0ubDG9r5qpiR/m5odENlePkkiICJH4QIRWUZ/63xZJUpw8uQzrvY=","ARC-Authentication-Results":"i=1; smtp.subspace.kernel.org;\n dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org\n header.b=Rk87Mz5+; arc=none smtp.client-ip=10.30.226.201","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org;\n\ts=k20201202; t=1775554711;\n\tbh=Uclf5ajKGlirtFS+exys4PG+9RGNZbkt5MlvGo9XdQ0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Rk87Mz5+8zNd8ViFTM/x+ma8Z16WtyLvt2IZj/EOooJFfHKz2LnBAYdzKKWuTKIHx\n\t tkds+wFJe5Y3EyrOGGuhGN/AI7qM5eje9RNBKhdS8HcNVh+o6E6RLNB1mGhd9fMd46\n\t uDfXG34YKhgyX7CoeypvOWcNvvKBubM+9Ozopw84vdJP3oEcoBU9MVJPeFNvmuhPdt\n\t Tp71MMFoOMwuR/ZtO7xcJTFYc0eO2eXSTnw0ikAJOtcbz598y9rtbo0tx2VMwb2I3h\n\t Aq+i8aMGWY3Mt4h5h2Ignec1MGLZh0WGN5QgiCuq5F+BHADfShsaluYcUeBZtpk2+u\n\t iIGNguEMCdO3Q==","Date":"Tue, 7 Apr 2026 11:38:28 +0200","From":"Thierry Reding ","To":"Manivannan Sadhasivam ","Cc":"Bjorn Helgaas ,\n Lorenzo Pieralisi , Krzysztof =?utf-8?q?Wilczy?=\n\t=?utf-8?q?=C5=84ski?= ,\n Rob Herring , Krzysztof Kozlowski ,\n Conor Dooley ,\n Thierry Reding ,\n Jonathan Hunter ,\n Karthikeyan Mitran ,\n Hou Zhiqiang ,\n Thomas Petazzoni ,\n Pali =?utf-8?b?Um9ow6Fy?= ,\n Michal Simek , Kevin Xie ,\n linux-pci@vger.kernel.org, devicetree@vger.kernel.org,\n linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org,\n linux-arm-kernel@lists.infradead.org, Thierry Reding ,\n Manikanta Maddireddy ","Subject":"Re: [PATCH v4 3/4] PCI: tegra: Add Tegra264 support","Message-ID":"","References":"<20260402-tegra264-pcie-v4-0-21e2e19987e8@nvidia.com>\n <20260402-tegra264-pcie-v4-3-21e2e19987e8@nvidia.com>\n ","Precedence":"bulk","X-Mailing-List":"linux-pci@vger.kernel.org","List-Id":"","List-Subscribe":"","List-Unsubscribe":"","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha512;\n\tprotocol=\"application/pgp-signature\"; boundary=\"q44iycnkojb2hlgm\"","Content-Disposition":"inline","In-Reply-To":""}},{"id":3675983,"web_url":"http://patchwork.ozlabs.org/comment/3675983/","msgid":"","list_archive_url":null,"date":"2026-04-10T16:34:20","subject":"Re: [PATCH v4 3/4] PCI: tegra: Add Tegra264 support","submitter":{"id":78905,"url":"http://patchwork.ozlabs.org/api/people/78905/","name":"Manivannan Sadhasivam","email":"mani@kernel.org"},"content":"On Tue, Apr 07, 2026 at 11:38:28AM +0200, Thierry Reding wrote:\n> On Thu, Apr 02, 2026 at 11:02:02PM +0530, Manivannan Sadhasivam wrote:\n> > On Thu, Apr 02, 2026 at 04:27:37PM +0200, Thierry Reding wrote:\n> > > From: Thierry Reding \n> > > \n> > > Add a driver for the PCIe controller found on NVIDIA Tegra264 SoCs. The\n> > > driver is very small, with its main purpose being to set up the address\n> > > translation registers and then creating a standard PCI host using ECAM.\n> > > \n> > > Signed-off-by: Manikanta Maddireddy \n> > > Signed-off-by: Thierry Reding \n> > \n> > What is the rationale for adding a new driver? Can't you reuse the existing one?\n> > If so, that should be mentioned in the description.\n> \n> Which existing one? Tegra PCI controllers for previou generations\n> (Tegra194 and Tegra234) were DesignWare IP, but Tegra264 is an internal\n> IP, so the programming is entirely different. I'll add something to that\n> effect to the commit message.\n> \n\nYikes! I completely missed that this is a non-dwc driver. Sorry for the noise.\n\n> > > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig\n> > > index 5aaed8ac6e44..6ead04f7bd6e 100644\n> > > --- a/drivers/pci/controller/Kconfig\n> > > +++ b/drivers/pci/controller/Kconfig\n> > > @@ -254,7 +254,15 @@ config PCI_TEGRA\n> > > \tselect IRQ_MSI_LIB\n> > > \thelp\n> > > \t Say Y here if you want support for the PCIe host controller found\n> > > -\t on NVIDIA Tegra SoCs.\n> > > +\t on NVIDIA Tegra SoCs (Tegra20 through Tegra186).\n> > > +\n> > > +config PCIE_TEGRA264\n> > > +\tbool \"NVIDIA Tegra264 PCIe controller\"\n> > \n> > This driver seems to be using external MSI controller. So it can be built as a\n> > module. Also, you have the remove() callback for some reason.\n> \n> Okay, I can turn this into a tristate symbol.\n> \n> > > +\tdepends on ARCH_TEGRA || COMPILE_TEST\n> > > +\tdepends on PCI_MSI\n> > \n> > Why?\n> \n> I suppose it's not necessary in the sense of it being a build\n> dependency. At runtime, however, the root complex is not useful if PCI\n> MSI is not enabled. We can drop this dependency and rely on .config to\n> have it enabled as needed.\n> \n\nYes. I think the rationale is to depend on the symbols that the driver needs for\nbuild dependency.\n\n> > > diff --git a/drivers/pci/controller/pcie-tegra264.c b/drivers/pci/controller/pcie-tegra264.c\n> > > new file mode 100644\n> > > index 000000000000..3ce1ad971bdb\n> > > --- /dev/null\n> > > +++ b/drivers/pci/controller/pcie-tegra264.c\n> [...]\n> > > +struct tegra264_pcie {\n> > > +\tstruct device *dev;\n> > > +\tbool link_up;\n> > \n> > Keep bool types at the end to avoid holes.\n> \n> Done.\n> \n> > > +static int tegra264_pcie_parse_dt(struct tegra264_pcie *pcie)\n> > > +{\n> > > +\tint err;\n> > > +\n> > > +\tpcie->wake_gpio = devm_gpiod_get_optional(pcie->dev, \"nvidia,pex-wake\",\n> > \n> > You should switch to standard 'wake-gpios' property.\n> \n> Will do.\n> \n> > > +\t\t\t\t\t\t GPIOD_IN);\n> > > +\tif (IS_ERR(pcie->wake_gpio))\n> > > +\t\treturn PTR_ERR(pcie->wake_gpio);\n> > > +\n> > > +\tif (pcie->wake_gpio) {\n> > \n> > Since you are bailing out above, you don't need this check.\n> \n> I think we still want to have this check to handle the case of optional\n> wake GPIOs. Not all controllers may have this wired up and\n> devm_gpiod_get_optional() will return NULL (not an ERR_PTR()-encoded\n> error) if the wake-gpios property is missing.\n> \n\nOk. In that case you can just bail out:\n\tif (!pcie->wake_gpio)\n\t\treturn 0;\n\n> > > +static void tegra264_pcie_bpmp_set_rp_state(struct tegra264_pcie *pcie)\n> > \n> > I don't think this function name is self explanatory. Looks like it is turning\n> > off the PCIe controller, so how about tegra264_pcie_power_off()?\n> \n> Agreed. The name is a relic from when this was potentially being used to\n> toggle on and off the controller. But it's only used for disabling, so\n> tegra264_pcie__power_off() sounds much better.\n> \n> > > +{\n> > > +\tstruct tegra_bpmp_message msg = {};\n> > > +\tstruct mrq_pcie_request req = {};\n> > > +\tint err;\n> > > +\n> > > +\treq.cmd = CMD_PCIE_RP_CONTROLLER_OFF;\n> > > +\treq.rp_ctrlr_off.rp_controller = pcie->ctl_id;\n> > > +\n> > > +\tmsg.mrq = MRQ_PCIE;\n> > > +\tmsg.tx.data = &req;\n> > > +\tmsg.tx.size = sizeof(req);\n> > > +\n> > > +\terr = tegra_bpmp_transfer(pcie->bpmp, &msg);\n> > > +\tif (err)\n> > > +\t\tdev_info(pcie->dev, \"failed to turn off PCIe #%u: %pe\\n\",\n> > \n> > Why not dev_err()?\n> > \n> > > +\t\t\t pcie->ctl_id, ERR_PTR(err));\n> > > +\n> > > +\tif (msg.rx.ret)\n> > > +\t\tdev_info(pcie->dev, \"failed to turn off PCIe #%u: %d\\n\",\n> > \n> > Same here.\n> \n> These are not fatal errors and are safe to ignore. dev_err() seemed too\n> strong for this. They also really shouldn't happen. Though I now realize\n> that's a bad argument, or rather, actually an argument for making them\n> dev_err() so that they do stand out if they really should happen.\n> \n> > \n> > > +\t\t\t pcie->ctl_id, msg.rx.ret);\n> > > +}\n> > > +\n> > > +static void tegra264_pcie_icc_set(struct tegra264_pcie *pcie)\n> > > +{\n> > > +\tu32 value, speed, width, bw;\n> > > +\tint err;\n> > > +\n> > > +\tvalue = readw(pcie->ecam + XTL_RC_PCIE_CFG_LINK_STATUS);\n> > > +\tspeed = FIELD_GET(PCI_EXP_LNKSTA_CLS, value);\n> > > +\twidth = FIELD_GET(PCI_EXP_LNKSTA_NLW, value);\n> > > +\n> > > +\tbw = width * (PCIE_SPEED2MBS_ENC(speed) / BITS_PER_BYTE);\n> > > +\tvalue = MBps_to_icc(bw);\n> > \n> > So this becomes, 'width * (PCIE_SPEED2MBS_ENC(speed) / 8) * 1000 / 8'. But don't\n> > you want, 'width * (PCIE_SPEED2MBS_ENC(speed)) * 1000 / 8'?\n> \n> This is M*B*ps_to_icc(), not M*b*ps_to_icc(), so we do in fact get the\n> latter. I almost fell for this as well because I got confused by some of\n> these macros being all-caps and other times the case actually mattering.\n> \n\nOops, I was misleaded too. But you can simply do:\n\tbw = Mbps_to_icc(width * PCIE_SPEED2MBS_ENC(speed));\n\n> > > +\terr = icc_set_bw(pcie->icc_path, bw, bw);\n\nAnd here you were setting the MBps, not Kbps.\n\n> > > +\tif (err < 0)\n> > > +\t\tdev_err(pcie->dev,\n> > > +\t\t\t\"failed to request bandwidth (%u MBps): %pe\\n\",\n> > > +\t\t\tbw, ERR_PTR(err));\n> > \n> > So you don't want to error out if this fails?\n> \n> No. This is not a fatal error and the system will continue to work,\n> albeit perhaps at suboptimal performance. Given that Ethernet and mass\n> storage are connected to these, a failure to set the bandwidth and\n> erroring out here may leave the system unusable, but continuing on would\n> let the system boot and update firmware, kernel or whatever to recover.\n> \n> I'll add a comment explaining this.\n> \n\nYeah, that'll help.\n\n> [...]\n> > > +static void tegra264_pcie_init(struct tegra264_pcie *pcie)\n> > > +{\n> > > +\tenum pci_bus_speed speed;\n> > > +\tunsigned int i;\n> > > +\tu32 value;\n> > > +\n> > > +\t/* bring the link out of reset */\n> > \n> > s/link/controller or endpoint?\n> \n> This controls the PERST# signal, so I guess \"endpoint\" would be more\n> correct.\n> \n\nYes!\n\n> > > +\tvalue = readl(pcie->xtl + XTL_RC_MGMT_PERST_CONTROL);\n> > > +\tvalue |= XTL_RC_MGMT_PERST_CONTROL_PERST_O_N;\n> > > +\twritel(value, pcie->xtl + XTL_RC_MGMT_PERST_CONTROL);\n> > > +\n> > > +\tif (!tegra_is_silicon()) {\n> > \n> > This looks like some pre-silicon validation thing. Do you really want it to be\n> > present in the upstream driver?\n> \n> At this point there is silicon for this chip, but we've been trying to\n> get some of the pre-silicon code merged upstream as well because\n> occasionally people will want to run upstream on simulation, even after\n> silicon is available. At other times we may want to reuse these drivers\n> on future chips during pre-silicon validation.\n> \n> Obviously there needs to be a balance. We don't want to have excessive\n> amounts of code specifically for pre-silicon validation, but in\n> relatively simple cases like this it is useful.\n> \n\nOk, fine with me.\n\n> > \n> > > +\t\tdev_info(pcie->dev,\n> > > +\t\t\t \"skipping link state for PCIe #%u in simulation\\n\",\n> > > +\t\t\t pcie->ctl_id);\n> > > +\t\tpcie->link_up = true;\n> > > +\t\treturn;\n> > > +\t}\n> > > +\n> > > +\tfor (i = 0; i < PCIE_LINK_WAIT_MAX_RETRIES; i++) {\n> > > +\t\tif (tegra264_pcie_link_up(pcie, NULL))\n> > > +\t\t\tbreak;\n> > > +\n> > > +\t\tusleep_range(PCIE_LINK_WAIT_US_MIN, PCIE_LINK_WAIT_US_MAX);\n> > > +\t}\n> > > +\n\n[...]\n\n> > > +\tpm_runtime_get_sync(dev);\n> > > +\n> > > +\t/* sanity check that programmed ranges match what's in DT */\n> > > +\tif (!tegra264_pcie_check_ranges(pdev)) {\n> > > +\t\terr = -EINVAL;\n> > > +\t\tgoto put_pm;\n> > > +\t}\n> > > +\n> > > +\tpcie->cfg = pci_ecam_create(dev, res, bus->res, &pci_generic_ecam_ops);\n> > > +\tif (IS_ERR(pcie->cfg)) {\n> > > +\t\terr = dev_err_probe(dev, PTR_ERR(pcie->cfg),\n> > > +\t\t\t\t \"failed to create ECAM\\n\");\n> > > +\t\tgoto put_pm;\n> > > +\t}\n> > > +\n> > > +\tbridge->ops = (struct pci_ops *)&pci_generic_ecam_ops.pci_ops;\n> > > +\tbridge->sysdata = pcie->cfg;\n> > > +\tpcie->ecam = pcie->cfg->win;\n> > > +\n> > > +\ttegra264_pcie_init(pcie);\n> > > +\n> > > +\tif (!pcie->link_up)\n> > > +\t\tgoto free;\n> > \n> > goto free_ecam;\n> \n> It's not clear to me, but are you suggesting to rename the existing\n> \"free\" label to \"free_ecam\"? I can do that.\n> \n\nYeah, I was just asking for a rename.\n\n> > > +\terr = pci_host_probe(bridge);\n> > > +\tif (err < 0) {\n> > > +\t\tdev_err(dev, \"failed to register host: %pe\\n\", ERR_PTR(err));\n> > \n> > dev_err_probe()\n> \n> Okay.\n> \n> > \n> > > +\t\tgoto free;\n> > > +\t}\n> > > +\n> > > +\treturn err;\n> > \n> > return 0;\n> \n> Done.\n> \n> [...]\n> > > +static int tegra264_pcie_resume_noirq(struct device *dev)\n> > > +{\n> > > +\tstruct tegra264_pcie *pcie = dev_get_drvdata(dev);\n> > > +\tint err;\n> > > +\n> > > +\tif (pcie->wake_gpio && device_may_wakeup(dev)) {\n> > > +\t\terr = disable_irq_wake(pcie->wake_irq);\n> > > +\t\tif (err < 0)\n> > > +\t\t\tdev_err(dev, \"failed to disable wake IRQ: %pe\\n\",\n> > > +\t\t\t\tERR_PTR(err));\n> > > +\t}\n> > > +\n> > > +\tif (pcie->link_up == false)\n> > > +\t\treturn 0;\n> > > +\n> > > +\ttegra264_pcie_init(pcie);\n> > > +\n> > \n> > Why do you need init() here without deinit() in tegra264_pcie_suspend_noirq()?\n> \n> That's because when we come out of suspend the link may have gone down\n> again, so we need to take the endpoint out of reset to retrigger the\n> link training. I think we could possibly explicitly clear that PERST_O_N\n> bit in the PERST_CONTROL register in a new tegra264_pcie_deinit() to\n> mirror what tegra264_pcie_init() does, but it's automatically done by\n> firmware anyway, so not needed.\n> \n\nHmm, so firmware asserts PERST# at the end of suspend? It is not clear to me why\nit is doing so. But for symmetry I'd like to do it in tegra264_pcie_deinit().\n\nAlso, I'm not certain about the 'pcie->link_up' check here. If it is 'false',\nthen probe() should've failed. So why do you need the check here anyway?\n\nMaybe you should get rid of this check and return the link status from\ntegra264_pcie_init() directly?\n\n- Mani","headers":{"Return-Path":"\n ","X-Original-To":["incoming@patchwork.ozlabs.org","linux-pci@vger.kernel.org"],"Delivered-To":"patchwork-incoming@legolas.ozlabs.org","Authentication-Results":["legolas.ozlabs.org;\n\tdkim=pass (2048-bit key;\n unprotected) header.d=kernel.org header.i=@kernel.org header.a=rsa-sha256\n header.s=k20201202 header.b=h6yRzHum;\n\tdkim-atps=neutral","legolas.ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org\n (client-ip=2600:3c15:e001:75::12fc:5321; helo=sin.lore.kernel.org;\n envelope-from=linux-pci+bounces-52339-incoming=patchwork.ozlabs.org@vger.kernel.org;\n receiver=patchwork.ozlabs.org)","smtp.subspace.kernel.org;\n\tdkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org\n header.b=\"h6yRzHum\"","smtp.subspace.kernel.org;\n arc=none smtp.client-ip=10.30.226.201"],"Received":["from sin.lore.kernel.org (sin.lore.kernel.org\n [IPv6:2600:3c15:e001:75::12fc:5321])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\t key-exchange x25519 server-signature ECDSA (secp384r1) server-digest SHA384)\n\t(No client certificate requested)\n\tby legolas.ozlabs.org (Postfix) with ESMTPS id 4fsjJB4TDRz1y2d\n\tfor ; Sat, 11 Apr 2026 02:42:02 +1000 (AEST)","from smtp.subspace.kernel.org (conduit.subspace.kernel.org\n [100.90.174.1])\n\tby sin.lore.kernel.org (Postfix) with ESMTP id 80846300514E\n\tfor ; Fri, 10 Apr 2026 16:34:34 +0000 (UTC)","from localhost.localdomain (localhost.localdomain [127.0.0.1])\n\tby smtp.subspace.kernel.org (Postfix) with ESMTP id 58CD73DCDB2;\n\tFri, 10 Apr 2026 16:34:33 +0000 (UTC)","from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org\n [10.30.226.201])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby smtp.subspace.kernel.org (Postfix) with ESMTPS id 332AE3DCD8E;\n\tFri, 10 Apr 2026 16:34:32 +0000 (UTC)","by smtp.kernel.org (Postfix) with ESMTPSA id 2098AC19421;\n\tFri, 10 Apr 2026 16:34:24 +0000 (UTC)"],"ARC-Seal":"i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116;\n\tt=1775838873; cv=none;\n b=nPx5jN7pT5k8f8+7hF1q7pkyGNoMnd3ssl91+YZF1EoQnrL6oDDBlGwDxqQ9rC1au/TugLCOnv2LF2KEflYMBCjw5hlBlnQlVST/ArlFN74H26fi8EZ+ioQlxX0up7vJk8ZyDfqCjeDhGCa+zhMu8LoCaEIQb4hZPHrIi2/zIyY=","ARC-Message-Signature":"i=1; a=rsa-sha256; d=subspace.kernel.org;\n\ts=arc-20240116; t=1775838873; c=relaxed/simple;\n\tbh=0GNmJYWWFmVhVV4nIg36bDblcBd/4S2M8hMuiDQOjUk=;\n\th=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version:\n\t Content-Type:Content-Disposition:In-Reply-To;\n b=t7etfmih4Tf6udtpHtZjIqtOqDh8BMD4Su7onp1NZyJEkdhcJkj7IK2FCXbArQNHqXOmFqpHHJOuKbo09vpL/MJmm5w2Fu0MBRyBi/87LaogJrY+uLw1ZJ+BitOy4efbCT8Au0pGoBwSxhyeYKJEeQ9OlcuLyxe5wEg2mjQ/HWU=","ARC-Authentication-Results":"i=1; smtp.subspace.kernel.org;\n dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org\n header.b=h6yRzHum; arc=none smtp.client-ip=10.30.226.201","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org;\n\ts=k20201202; t=1775838872;\n\tbh=0GNmJYWWFmVhVV4nIg36bDblcBd/4S2M8hMuiDQOjUk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=h6yRzHumYoqUvW46IDcJv1biGCgOPkVYHTUQJAaejVyV7yGNVQJD+bZkpzsdPp/rz\n\t fDEMkqIQLmW3rSuKyrw1YbMsPwg2IqL+q9i/IaY+YrHznB2IeUwuAO7L1QnmSYtxMn\n\t En1ZeXUAkbMjJ10NH+F0uKR6SiHNvRE8ATe7whYZh9bbFh2w4zGdzRo9islLpHmw+a\n\t ahyobHPsqJ/PnCPQNt2AiNLrW7JKbmQSliRGjI6G1rQIJ7IpUEJgsl3DwxHmAOmaLx\n\t ZtVkMqX6UXr8VzRLZtfzqZEVQ7hhWAaN1QOxNfyvtcs1LfZHwEui7WZ47ODNMdsZOX\n\t khotrbWqMUKmA==","Date":"Fri, 10 Apr 2026 22:04:20 +0530","From":"Manivannan Sadhasivam ","To":"Thierry Reding ","Cc":"Bjorn Helgaas ,\n Lorenzo Pieralisi , Krzysztof =?utf-8?q?Wilczy?=\n\t=?utf-8?q?=C5=84ski?= ,\n Rob Herring , Krzysztof Kozlowski ,\n Conor Dooley ,\n Thierry Reding ,\n Jonathan Hunter ,\n Karthikeyan Mitran ,\n Hou Zhiqiang ,\n Thomas Petazzoni ,\n Pali =?utf-8?b?Um9ow6Fy?= ,\n Michal Simek , Kevin Xie ,\n linux-pci@vger.kernel.org, devicetree@vger.kernel.org,\n linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org,\n linux-arm-kernel@lists.infradead.org, Thierry Reding ,\n Manikanta Maddireddy ","Subject":"Re: [PATCH v4 3/4] PCI: tegra: Add Tegra264 support","Message-ID":"","References":"<20260402-tegra264-pcie-v4-0-21e2e19987e8@nvidia.com>\n <20260402-tegra264-pcie-v4-3-21e2e19987e8@nvidia.com>\n \n ","Precedence":"bulk","X-Mailing-List":"linux-pci@vger.kernel.org","List-Id":"","List-Subscribe":"","List-Unsubscribe":"","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":""}}]