| Message ID | 20250224073117.767210-4-thippeswamy.havalige@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for AMD MDB IP as Root Port | expand |
On 24/02/2025 08:31, Thippeswamy Havalige wrote: > Add support for AMD MDB (Multimedia DMA Bridge) IP core as Root Port. > > The Versal2 devices include MDB Module. The integrated block for MDB along > with the integrated bridge can function as PCIe Root Port controller at > Gen5 32-Gb/s operation per lane. > > Bridge supports error and legacy interrupts and are handled using platform > specific interrupt line in Versal2. > > Signed-off-by: Thippeswamy Havalige <thippeswamy.havalige@amd.com> > | Reported-by: kernel test robot <lkp@intel.com> > | Closes: > | https://lore.kernel.org/oe-kbuild-all/202502191741.xrVmEAG4-lkp@intel. > | com/ Tags never start with | and are not wrapped. But anyway, robot did not report this patch. Drop these. Best regards, Krzysztof
On Mon, Feb 24, 2025 at 01:01:17PM +0530, Thippeswamy Havalige wrote: > Add support for AMD MDB (Multimedia DMA Bridge) IP core as Root Port. > > The Versal2 devices include MDB Module. The integrated block for MDB along > with the integrated bridge can function as PCIe Root Port controller at > Gen5 32-Gb/s operation per lane. Please use GT/s > > Bridge supports error and legacy interrupts and are handled using platform > specific interrupt line in Versal2. > > Signed-off-by: Thippeswamy Havalige <thippeswamy.havalige@amd.com> > | Reported-by: kernel test robot <lkp@intel.com> > | Closes: > | https://lore.kernel.org/oe-kbuild-all/202502191741.xrVmEAG4-lkp@intel. > | com/ These tags are irrelevant. > --- > changes in v2: > ------------- > - Update Gen5 speed in the patch description. > - Modify Kconfig file. > - Update string _leg_ to intx. > - Get platform structure through automic variables. > - Remove _rp_ in function. > Changes in v3: > -------------- > -None. > Changes in v4: > -------------- > -None. > Changes in v5: > -------------- > -None. > Changes in v6: > -------------- > - Remove pdev automatic variable. > - Update register name to slcr. > - Fix whitespace. > - remove Spurious extra line. > - Update Legacy to INTx. > - Add space before (SLCR). > - Update menuconfig description. > Changes in v7: > -------------- > - None. > Changes in v8: > -------------- > - Remove inline keyword. > - Fix indentations. > - Add AMD MDB prefix to interrupt names. > - Remove Kernel doc. > - Fix return types. > - Modify dev_warn to dev_warn_once. > - Add Intx handler & callbacks. > Changes in v10: > --------------- > - Add intx assert & deassert macros. > - Move amd_mdb_pcie_init_port function. > - Add kernel doc for error warning messages. > Changes in v11: > --------------- > - Remove intx deassert macro & generic handler. > - Update Kconfig description. > - Update INTx mask macro to handle only asser bits. > - Move INTx handler. > - Address other review comments. > Changes in v12: > --------------- > - ADD TLP_IR_DISABLE_MISC register. > - Modify intx call back function > Changes in v13: > - Add kernel doc for intx_irq > Changes in v14: > -------------- > - Modify mask in intx_irq_mask/unmask functions. > - Modify mask in intx_flow handler. > --- > drivers/pci/controller/dwc/Kconfig | 11 + > drivers/pci/controller/dwc/Makefile | 1 + > drivers/pci/controller/dwc/pcie-amd-mdb.c | 481 ++++++++++++++++++++++ > 3 files changed, 493 insertions(+) > create mode 100644 drivers/pci/controller/dwc/pcie-amd-mdb.c > > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig > index b6d6778b0698..882f4b558133 100644 > --- a/drivers/pci/controller/dwc/Kconfig > +++ b/drivers/pci/controller/dwc/Kconfig > @@ -27,6 +27,17 @@ config PCIE_AL > required only for DT-based platforms. ACPI platforms with the > Annapurna Labs PCIe controller don't need to enable this. > > +config PCIE_AMD_MDB > + bool "AMD MDB Versal2 PCIe Host controller" > + depends on OF || COMPILE_TEST > + depends on PCI && PCI_MSI > + select PCIE_DW_HOST > + help > + Say Y here if you want to enable PCIe controller support on AMD > + Versal2 SoCs. The AMD MDB Versal2 PCIe controller is based on > + DesignWare IP and therefore the driver re-uses the Designware core > + functions to implement the driver. > + > config PCI_MESON > tristate "Amlogic Meson PCIe controller" > default m if ARCH_MESON > diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile > index a8308d9ea986..ae27eda6ec5e 100644 > --- a/drivers/pci/controller/dwc/Makefile > +++ b/drivers/pci/controller/dwc/Makefile > @@ -3,6 +3,7 @@ obj-$(CONFIG_PCIE_DW) += pcie-designware.o > obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o > obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o > obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o > +obj-$(CONFIG_PCIE_AMD_MDB) += pcie-amd-mdb.o > obj-$(CONFIG_PCIE_BT1) += pcie-bt1.o > obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o > obj-$(CONFIG_PCI_EXYNOS) += pci-exynos.o > diff --git a/drivers/pci/controller/dwc/pcie-amd-mdb.c b/drivers/pci/controller/dwc/pcie-amd-mdb.c > new file mode 100644 > index 000000000000..01d27cb5a70d > --- /dev/null > +++ b/drivers/pci/controller/dwc/pcie-amd-mdb.c > @@ -0,0 +1,481 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * PCIe host controller driver for AMD MDB PCIe Bridge > + * > + * Copyright (C) 2024-2025, Advanced Micro Devices, Inc. > + */ > + > +#include <linux/clk.h> > +#include <linux/delay.h> > +#include <linux/gpio.h> > +#include <linux/interrupt.h> > +#include <linux/irqdomain.h> > +#include <linux/kernel.h> > +#include <linux/init.h> > +#include <linux/of_device.h> > +#include <linux/pci.h> > +#include <linux/platform_device.h> > +#include <linux/resource.h> > +#include <linux/types.h> > + > +#include "pcie-designware.h" > + > +#define AMD_MDB_TLP_IR_STATUS_MISC 0x4C0 > +#define AMD_MDB_TLP_IR_MASK_MISC 0x4C4 > +#define AMD_MDB_TLP_IR_ENABLE_MISC 0x4C8 > +#define AMD_MDB_TLP_IR_DISABLE_MISC 0x4CC > + > +#define AMD_MDB_TLP_PCIE_INTX_MASK GENMASK(23, 16) > + > +#define AMD_MDB_PCIE_INTR_INTX_ASSERT(x) BIT((x) * 2) How does these values correspond to the AMD_MDB_TLP_PCIE_INTX_MASK? These values could be: 0, 2, 4, and 6 corresponding to: 0b01010101? Looks wierd. > + > +/* Interrupt registers definitions */ > +#define AMD_MDB_PCIE_INTR_CMPL_TIMEOUT 15 > +#define AMD_MDB_PCIE_INTR_INTX 16 > +#define AMD_MDB_PCIE_INTR_PM_PME_RCVD 24 > +#define AMD_MDB_PCIE_INTR_PME_TO_ACK_RCVD 25 > +#define AMD_MDB_PCIE_INTR_MISC_CORRECTABLE 26 > +#define AMD_MDB_PCIE_INTR_NONFATAL 27 > +#define AMD_MDB_PCIE_INTR_FATAL 28 > + > +#define IMR(x) BIT(AMD_MDB_PCIE_INTR_ ##x) > +#define AMD_MDB_PCIE_IMR_ALL_MASK \ > + ( \ > + IMR(CMPL_TIMEOUT) | \ > + IMR(PM_PME_RCVD) | \ > + IMR(PME_TO_ACK_RCVD) | \ > + IMR(MISC_CORRECTABLE) | \ > + IMR(NONFATAL) | \ > + IMR(FATAL) | \ > + AMD_MDB_TLP_PCIE_INTX_MASK \ > + ) > + > +/** > + * struct amd_mdb_pcie - PCIe port information > + * @pci: DesignWare PCIe controller structure > + * @slcr: MDB System Level Control and Status Register (SLCR) Base > + * @intx_domain: INTx IRQ domain pointer > + * @mdb_domain: MDB IRQ domain pointer > + * @intx_irq: INTx IRQ interrupt number > + */ > +struct amd_mdb_pcie { > + struct dw_pcie pci; > + void __iomem *slcr; > + struct irq_domain *intx_domain; > + struct irq_domain *mdb_domain; > + int intx_irq; > +}; > + > +static const struct dw_pcie_host_ops amd_mdb_pcie_host_ops = { > +}; > + > +static inline u32 pcie_read(struct amd_mdb_pcie *pcie, u32 reg) > +{ > + return readl_relaxed(pcie->slcr + reg); > +} I think I already commented on these helpers. Please get rid of them. I don't see any value in this new driver. Moreover, 'inline' keywords are not required. > + > +static inline void pcie_write(struct amd_mdb_pcie *pcie, > + u32 val, u32 reg) > +{ > + writel_relaxed(val, pcie->slcr + reg); > +} > + > +static void amd_mdb_intx_irq_mask(struct irq_data *data) > +{ > + struct amd_mdb_pcie *pcie = irq_data_get_irq_chip_data(data); > + struct dw_pcie *pci = &pcie->pci; > + struct dw_pcie_rp *port = &pci->pp; > + unsigned long flags; > + u32 val; > + > + raw_spin_lock_irqsave(&port->lock, flags); > + val = FIELD_PREP(AMD_MDB_TLP_PCIE_INTX_MASK, > + AMD_MDB_PCIE_INTR_INTX_ASSERT(data->hwirq)); > + > + /* > + * Writing '1' to a bit in AMD_MDB_TLP_IR_DISABLE_MISC disables that > + * interrupt, writing '0' has no effect. > + */ > + pcie_write(pcie, val, AMD_MDB_TLP_IR_DISABLE_MISC); > + raw_spin_unlock_irqrestore(&port->lock, flags); > +} > + > +static void amd_mdb_intx_irq_unmask(struct irq_data *data) > +{ > + struct amd_mdb_pcie *pcie = irq_data_get_irq_chip_data(data); > + struct dw_pcie *pci = &pcie->pci; > + struct dw_pcie_rp *port = &pci->pp; > + unsigned long flags; > + u32 val; > + > + raw_spin_lock_irqsave(&port->lock, flags); > + val = FIELD_PREP(AMD_MDB_TLP_PCIE_INTX_MASK, > + AMD_MDB_PCIE_INTR_INTX_ASSERT(data->hwirq)); > + > + /* > + * Writing '1' to a bit in AMD_MDB_TLP_IR_ENABLE_MISC enables that interrupt. > + * Writing '0' has no effect. > + */ > + pcie_write(pcie, val, AMD_MDB_TLP_IR_ENABLE_MISC); > + raw_spin_unlock_irqrestore(&port->lock, flags); > +} > + > +static struct irq_chip amd_mdb_intx_irq_chip = { > + .name = "AMD MDB INTx", > + .irq_mask = amd_mdb_intx_irq_mask, > + .irq_unmask = amd_mdb_intx_irq_unmask, > +}; > + > +/** > + * amd_mdb_pcie_intx_map - Set the handler for the INTx and mark IRQ > + * as valid > + * @domain: IRQ domain > + * @irq: Virtual IRQ number > + * @hwirq: HW interrupt number > + * > + * Return: Always returns 0. > + */ > +static int amd_mdb_pcie_intx_map(struct irq_domain *domain, > + unsigned int irq, irq_hw_number_t hwirq) > +{ > + irq_set_chip_and_handler(irq, &amd_mdb_intx_irq_chip, > + handle_level_irq); > + irq_set_chip_data(irq, domain->host_data); > + irq_set_status_flags(irq, IRQ_LEVEL); > + > + return 0; > +} > + > +/* INTx IRQ Domain operations */ > +static const struct irq_domain_ops amd_intx_domain_ops = { > + .map = amd_mdb_pcie_intx_map, > +}; > + > +static irqreturn_t dw_pcie_rp_intx_flow(int irq, void *args) What does the _flow suffix mean? > +{ > + struct amd_mdb_pcie *pcie = args; > + unsigned long val; > + int i, int_status; > + > + val = pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC); You don't need port->lock here? > + int_status = FIELD_GET(AMD_MDB_TLP_PCIE_INTX_MASK, val); You don't need to ack/clear the IRQ? > + > + for (i = 0; i < PCI_NUM_INTX; i++) { > + if (int_status & AMD_MDB_PCIE_INTR_INTX_ASSERT(i)) > + generic_handle_domain_irq(pcie->intx_domain, i); > + } > + > + return IRQ_HANDLED; > +} > + > +#define _IC(x, s)[AMD_MDB_PCIE_INTR_ ## x] = { __stringify(x), s } > + > +static const struct { > + const char *sym; > + const char *str; > +} intr_cause[32] = { > + _IC(CMPL_TIMEOUT, "completion timeout"), > + _IC(PM_PME_RCVD, "PM_PME message received"), > + _IC(PME_TO_ACK_RCVD, "PME_TO_ACK message received"), > + _IC(MISC_CORRECTABLE, "Correctable error message"), > + _IC(NONFATAL, "Non fatal error message"), > + _IC(FATAL, "Fatal error message"), > +}; > + > +static void amd_mdb_event_irq_mask(struct irq_data *d) > +{ > + struct amd_mdb_pcie *pcie = irq_data_get_irq_chip_data(d); > + struct dw_pcie *pci = &pcie->pci; > + struct dw_pcie_rp *port = &pci->pp; > + u32 val; > + > + raw_spin_lock(&port->lock); > + val = pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC); This register is accessed in the IRQ handler also. So don't you need raw_spin_lock_irq{save/restore}? > + val &= ~BIT(d->hwirq); > + pcie_write(pcie, val, AMD_MDB_TLP_IR_STATUS_MISC); > + raw_spin_unlock(&port->lock); > +} > + > +static void amd_mdb_event_irq_unmask(struct irq_data *d) > +{ > + struct amd_mdb_pcie *pcie = irq_data_get_irq_chip_data(d); > + struct dw_pcie *pci = &pcie->pci; > + struct dw_pcie_rp *port = &pci->pp; > + u32 val; > + > + raw_spin_lock(&port->lock); Same here > + val = pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC); > + val |= BIT(d->hwirq); > + pcie_write(pcie, val, AMD_MDB_TLP_IR_STATUS_MISC); > + raw_spin_unlock(&port->lock); > +} > + > +static struct irq_chip amd_mdb_event_irq_chip = { > + .name = "AMD MDB RC-Event", > + .irq_mask = amd_mdb_event_irq_mask, > + .irq_unmask = amd_mdb_event_irq_unmask, > +}; > + > +static int amd_mdb_pcie_event_map(struct irq_domain *domain, > + unsigned int irq, irq_hw_number_t hwirq) > +{ > + irq_set_chip_and_handler(irq, &amd_mdb_event_irq_chip, > + handle_level_irq); > + irq_set_chip_data(irq, domain->host_data); > + irq_set_status_flags(irq, IRQ_LEVEL); > + > + return 0; > +} > + > +static const struct irq_domain_ops event_domain_ops = { > + .map = amd_mdb_pcie_event_map, > +}; > + > +static irqreturn_t amd_mdb_pcie_event_flow(int irq, void *args) > +{ > + struct amd_mdb_pcie *pcie = args; > + unsigned long val; > + int i; > + > + val = pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC); Here also, no lock? > + val &= ~pcie_read(pcie, AMD_MDB_TLP_IR_MASK_MISC); > + for_each_set_bit(i, &val, 32) > + generic_handle_domain_irq(pcie->mdb_domain, i); > + pcie_write(pcie, val, AMD_MDB_TLP_IR_STATUS_MISC); > + > + return IRQ_HANDLED; > +} > + > +static void amd_mdb_pcie_free_irq_domains(struct amd_mdb_pcie *pcie) > +{ > + if (pcie->intx_domain) { > + irq_domain_remove(pcie->intx_domain); > + pcie->intx_domain = NULL; > + } > + > + if (pcie->mdb_domain) { > + irq_domain_remove(pcie->mdb_domain); > + pcie->mdb_domain = NULL; > + } > +} > + > +static int amd_mdb_pcie_init_port(struct amd_mdb_pcie *pcie) > +{ > + /* Disable all TLP Interrupts */ > + pcie_write(pcie, AMD_MDB_PCIE_IMR_ALL_MASK, > + AMD_MDB_TLP_IR_DISABLE_MISC); > + > + /* Clear pending TLP interrupts */ > + pcie_write(pcie, pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC) & > + AMD_MDB_PCIE_IMR_ALL_MASK, > + AMD_MDB_TLP_IR_STATUS_MISC); > + > + /* Enable all TLP Interrupts */ > + pcie_write(pcie, AMD_MDB_PCIE_IMR_ALL_MASK, > + AMD_MDB_TLP_IR_ENABLE_MISC); > + > + return 0; > +} > + > +/** > + * amd_mdb_pcie_init_irq_domains - Initialize IRQ domain > + * @pcie: PCIe port information > + * @pdev: platform device > + * Return: '0' on success and error value on failure > + */ > +static int amd_mdb_pcie_init_irq_domains(struct amd_mdb_pcie *pcie, > + struct platform_device *pdev) > +{ > + struct dw_pcie *pci = &pcie->pci; > + struct dw_pcie_rp *pp = &pci->pp; > + struct device *dev = &pdev->dev; > + struct device_node *node = dev->of_node; > + struct device_node *pcie_intc_node; > + > + /* Setup INTx */ This comment is quite misleading. This Intc node is not specific to INTx as far as I can see. > + pcie_intc_node = of_get_next_child(node, NULL); > + if (!pcie_intc_node) { > + dev_err(dev, "No PCIe Intc node found\n"); > + return -ENODATA; -ENODEV > + } > + > + pcie->mdb_domain = irq_domain_add_linear(pcie_intc_node, 32, > + &event_domain_ops, pcie); > + if (!pcie->mdb_domain) { > + dev_err(dev, "Failed to add mdb_domain\n"); > + goto out; > + } > + > + irq_domain_update_bus_token(pcie->mdb_domain, DOMAIN_BUS_NEXUS); > + > + pcie->intx_domain = irq_domain_add_linear(pcie_intc_node, PCI_NUM_INTX, > + &amd_intx_domain_ops, pcie); > + if (!pcie->intx_domain) { > + dev_err(dev, "Failed to add intx_domain\n"); > + goto mdb_out; > + } > + > + of_node_put(pcie_intc_node); > + irq_domain_update_bus_token(pcie->intx_domain, DOMAIN_BUS_WIRED); > + > + raw_spin_lock_init(&pp->lock); > + > + return 0; > +mdb_out: > + amd_mdb_pcie_free_irq_domains(pcie); > +out: > + of_node_put(pcie_intc_node); > + > + return -ENOMEM; No please. This is just asking for trouble. Please pass the relevant errno. > +} > + > +static irqreturn_t amd_mdb_pcie_intr_handler(int irq, void *args) > +{ > + struct amd_mdb_pcie *pcie = args; > + struct device *dev; > + struct irq_data *d; > + > + dev = pcie->pci.dev; > + > + /* > + * In future, error reporting will be hooked to the AER subsystem. > + * Currently, the driver prints a warning message to the user. > + */ > + d = irq_domain_get_irq_data(pcie->mdb_domain, irq); > + if (intr_cause[d->hwirq].str) > + dev_warn(dev, "%s\n", intr_cause[d->hwirq].str); > + else > + dev_warn_once(dev, "Unknown IRQ %ld\n", d->hwirq); > + > + return IRQ_HANDLED; > +} > + > +static int amd_mdb_setup_irq(struct amd_mdb_pcie *pcie, > + struct platform_device *pdev) > +{ > + struct dw_pcie *pci = &pcie->pci; > + struct dw_pcie_rp *pp = &pci->pp; > + struct device *dev = &pdev->dev; > + int i, irq, err; > + > + amd_mdb_pcie_init_port(pcie); > + > + pp->irq = platform_get_irq(pdev, 0); > + if (pp->irq < 0) > + return pp->irq; > + > + for (i = 0; i < ARRAY_SIZE(intr_cause); i++) { > + if (!intr_cause[i].str) > + continue; > + irq = irq_create_mapping(pcie->mdb_domain, i); > + if (!irq) { > + dev_err(dev, "Failed to map mdb domain interrupt\n"); > + return -ENOMEM; > + } > + err = devm_request_irq(dev, irq, amd_mdb_pcie_intr_handler, > + IRQF_SHARED | IRQF_NO_THREAD, > + intr_cause[i].sym, pcie); Aren't these IRQs just part of a single IRQ? I'm wondering why do you need to represent them individually instead of having a single IRQ handler. Btw, you are not disposing these IRQs anywhere. Especially in error paths. > + if (err) { > + dev_err(dev, "Failed to request IRQ %d\n", irq); > + return err; > + } > + } > + > + pcie->intx_irq = irq_create_mapping(pcie->mdb_domain, > + AMD_MDB_PCIE_INTR_INTX); > + if (!pcie->intx_irq) { > + dev_err(dev, "Failed to map INTx interrupt\n"); > + return -ENXIO; > + } > + > + err = devm_request_irq(dev, pcie->intx_irq, > + dw_pcie_rp_intx_flow, > + IRQF_SHARED | IRQF_NO_THREAD, NULL, pcie); > + if (err) { > + dev_err(dev, "Failed to request INTx IRQ %d\n", irq); > + return err; > + } > + > + /* Plug the main event handler */ > + err = devm_request_irq(dev, pp->irq, amd_mdb_pcie_event_flow, > + IRQF_SHARED | IRQF_NO_THREAD, "amd_mdb pcie_irq", Why is this a SHARED IRQ? - Mani
Hi Krzysztof, Thank you for reviewing, Will remove it in next patch. Regards, Thippeswamy H -----Original Message----- From: Krzysztof Kozlowski <krzk@kernel.org> Sent: Monday, February 24, 2025 1:28 PM To: Havalige, Thippeswamy <thippeswamy.havalige@amd.com>; bhelgaas@google.com; lpieralisi@kernel.org; kw@linux.com; manivannan.sadhasivam@linaro.org; robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org Cc: linux-pci@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Simek, Michal <michal.simek@amd.com>; Gogada, Bharat Kumar <bharat.kumar.gogada@amd.com>; jingoohan1@gmail.com Subject: Re: [PATCH v14 3/3] PCI: amd-mdb: Add AMD MDB Root Port driver On 24/02/2025 08:31, Thippeswamy Havalige wrote: > Add support for AMD MDB (Multimedia DMA Bridge) IP core as Root Port. > > The Versal2 devices include MDB Module. The integrated block for MDB > along with the integrated bridge can function as PCIe Root Port > controller at > Gen5 32-Gb/s operation per lane. > > Bridge supports error and legacy interrupts and are handled using > platform specific interrupt line in Versal2. > > Signed-off-by: Thippeswamy Havalige <thippeswamy.havalige@amd.com> > | Reported-by: kernel test robot <lkp@intel.com> > | Closes: > | https://lore.kernel.org/oe-kbuild-all/202502191741.xrVmEAG4-lkp@intel. > | com/ Tags never start with | and are not wrapped. But anyway, robot did not report this patch. Drop these. Best regards, Krzysztof
Hi Manivannan, -----Original Message----- From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> Sent: Monday, February 24, 2025 3:00 PM To: Havalige, Thippeswamy <thippeswamy.havalige@amd.com> Cc: bhelgaas@google.com; lpieralisi@kernel.org; kw@linux.com; robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org; linux-pci@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Simek, Michal <michal.simek@amd.com>; Gogada, Bharat Kumar <bharat.kumar.gogada@amd.com>; jingoohan1@gmail.com Subject: Re: [PATCH v14 3/3] PCI: amd-mdb: Add AMD MDB Root Port driver On Mon, Feb 24, 2025 at 01:01:17PM +0530, Thippeswamy Havalige wrote: > Add support for AMD MDB (Multimedia DMA Bridge) IP core as Root Port. > > The Versal2 devices include MDB Module. The integrated block for MDB > along with the integrated bridge can function as PCIe Root Port > controller at > Gen5 32-Gb/s operation per lane. Please use GT/s Thank you for reviewing, ll update this in next patch. > > Bridge supports error and legacy interrupts and are handled using > platform specific interrupt line in Versal2. > > Signed-off-by: Thippeswamy Havalige <thippeswamy.havalige@amd.com> > | Reported-by: kernel test robot <lkp@intel.com> > | Closes: > | https://lore.kernel.org/oe-kbuild-all/202502191741.xrVmEAG4-lkp@intel. > | com/ These tags are irrelevant. Thank you for reviewing, Will remove this in next patch. > --- > changes in v2: > ------------- > - Update Gen5 speed in the patch description. > - Modify Kconfig file. > - Update string _leg_ to intx. > - Get platform structure through automic variables. > - Remove _rp_ in function. > Changes in v3: > -------------- > -None. > Changes in v4: > -------------- > -None. > Changes in v5: > -------------- > -None. > Changes in v6: > -------------- > - Remove pdev automatic variable. > - Update register name to slcr. > - Fix whitespace. > - remove Spurious extra line. > - Update Legacy to INTx. > - Add space before (SLCR). > - Update menuconfig description. > Changes in v7: > -------------- > - None. > Changes in v8: > -------------- > - Remove inline keyword. > - Fix indentations. > - Add AMD MDB prefix to interrupt names. > - Remove Kernel doc. > - Fix return types. > - Modify dev_warn to dev_warn_once. > - Add Intx handler & callbacks. > Changes in v10: > --------------- > - Add intx assert & deassert macros. > - Move amd_mdb_pcie_init_port function. > - Add kernel doc for error warning messages. > Changes in v11: > --------------- > - Remove intx deassert macro & generic handler. > - Update Kconfig description. > - Update INTx mask macro to handle only asser bits. > - Move INTx handler. > - Address other review comments. > Changes in v12: > --------------- > - ADD TLP_IR_DISABLE_MISC register. > - Modify intx call back function > Changes in v13: > - Add kernel doc for intx_irq > Changes in v14: > -------------- > - Modify mask in intx_irq_mask/unmask functions. > - Modify mask in intx_flow handler. > --- > drivers/pci/controller/dwc/Kconfig | 11 + > drivers/pci/controller/dwc/Makefile | 1 + > drivers/pci/controller/dwc/pcie-amd-mdb.c | 481 > ++++++++++++++++++++++ > 3 files changed, 493 insertions(+) > create mode 100644 drivers/pci/controller/dwc/pcie-amd-mdb.c > > diff --git a/drivers/pci/controller/dwc/Kconfig > b/drivers/pci/controller/dwc/Kconfig > index b6d6778b0698..882f4b558133 100644 > --- a/drivers/pci/controller/dwc/Kconfig > +++ b/drivers/pci/controller/dwc/Kconfig > @@ -27,6 +27,17 @@ config PCIE_AL > required only for DT-based platforms. ACPI platforms with the > Annapurna Labs PCIe controller don't need to enable this. > > +config PCIE_AMD_MDB > + bool "AMD MDB Versal2 PCIe Host controller" > + depends on OF || COMPILE_TEST > + depends on PCI && PCI_MSI > + select PCIE_DW_HOST > + help > + Say Y here if you want to enable PCIe controller support on AMD > + Versal2 SoCs. The AMD MDB Versal2 PCIe controller is based on > + DesignWare IP and therefore the driver re-uses the Designware core > + functions to implement the driver. > + > config PCI_MESON > tristate "Amlogic Meson PCIe controller" > default m if ARCH_MESON > diff --git a/drivers/pci/controller/dwc/Makefile > b/drivers/pci/controller/dwc/Makefile > index a8308d9ea986..ae27eda6ec5e 100644 > --- a/drivers/pci/controller/dwc/Makefile > +++ b/drivers/pci/controller/dwc/Makefile > @@ -3,6 +3,7 @@ obj-$(CONFIG_PCIE_DW) += pcie-designware.o > obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o > obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o > obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o > +obj-$(CONFIG_PCIE_AMD_MDB) += pcie-amd-mdb.o > obj-$(CONFIG_PCIE_BT1) += pcie-bt1.o > obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o > obj-$(CONFIG_PCI_EXYNOS) += pci-exynos.o diff --git > a/drivers/pci/controller/dwc/pcie-amd-mdb.c > b/drivers/pci/controller/dwc/pcie-amd-mdb.c > new file mode 100644 > index 000000000000..01d27cb5a70d > --- /dev/null > +++ b/drivers/pci/controller/dwc/pcie-amd-mdb.c > @@ -0,0 +1,481 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * PCIe host controller driver for AMD MDB PCIe Bridge > + * > + * Copyright (C) 2024-2025, Advanced Micro Devices, Inc. > + */ > + > +#include <linux/clk.h> > +#include <linux/delay.h> > +#include <linux/gpio.h> > +#include <linux/interrupt.h> > +#include <linux/irqdomain.h> > +#include <linux/kernel.h> > +#include <linux/init.h> > +#include <linux/of_device.h> > +#include <linux/pci.h> > +#include <linux/platform_device.h> > +#include <linux/resource.h> > +#include <linux/types.h> > + > +#include "pcie-designware.h" > + > +#define AMD_MDB_TLP_IR_STATUS_MISC 0x4C0 > +#define AMD_MDB_TLP_IR_MASK_MISC 0x4C4 > +#define AMD_MDB_TLP_IR_ENABLE_MISC 0x4C8 > +#define AMD_MDB_TLP_IR_DISABLE_MISC 0x4CC > + > +#define AMD_MDB_TLP_PCIE_INTX_MASK GENMASK(23, 16) > + > +#define AMD_MDB_PCIE_INTR_INTX_ASSERT(x) BIT((x) * 2) How does these values correspond to the AMD_MDB_TLP_PCIE_INTX_MASK? These values could be: 0, 2, 4, and 6 corresponding to: 0b01010101? Looks wierd. Thank you for reviewing, Yes in register status/Enable/Disable register bits are laid in this way. > + > +/* Interrupt registers definitions */ > +#define AMD_MDB_PCIE_INTR_CMPL_TIMEOUT 15 > +#define AMD_MDB_PCIE_INTR_INTX 16 > +#define AMD_MDB_PCIE_INTR_PM_PME_RCVD 24 > +static inline u32 pcie_read(struct amd_mdb_pcie *pcie, u32 reg) { > + return readl_relaxed(pcie->slcr + reg); } I think I already commented on these helpers. Please get rid of them. I don't see any value in this new driver. Moreover, 'inline' keywords are not required. Thanks for the review. While I agree to remove the 'inline', I would like pcie_read/pcie_write APIs. Could you please clarify the reason for explicitly removing pcie_read/pcie_write here? If I remove the pcie_read/pcie_write, it will require changes in multiple places throughout the code." > + > +static inline void pcie_write(struct amd_mdb_pcie *pcie, > + u32 val, u32 reg) > +{ > + writel_relaxed(val, pcie->slcr + reg); } > + > +static const struct irq_domain_ops amd_intx_domain_ops = { > + .map = amd_mdb_pcie_intx_map, > +}; > + > +static irqreturn_t dw_pcie_rp_intx_flow(int irq, void *args) What does the _flow suffix mean? Thanks for reviewing, The _flow suffix in the function name dw_pcie_rp_intx_flow indicates that the function handles the flow or processing related to interrupt handling for the PCIe root port's INTx interrupts through generic_handle_domain_irq. > +{ > + struct amd_mdb_pcie *pcie = args; > + unsigned long val; > + int i, int_status; > + > + val = pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC); You don't need port->lock here? Thank you for reviewing. In this case, I'm simply reading the status of the INTX register bits without modifying any registers. Since no shared resources are being updated or accessed concurrently, there’s no need for a lock here. > + int_status = FIELD_GET(AMD_MDB_TLP_PCIE_INTX_MASK, val); You don't need to ack/clear the IRQ? - Thank you for reviewing, Thank you for reviewing. In this case, I am using IRQ domains, and the generic_handle_domain_irq function will invoke the necessary irq_mask and irq_unmask operations internally, which will take care of clearing the IRQ. > + > + for (i = 0; i < PCI_NUM_INTX; i++) { > + if (int_status & AMD_MDB_PCIE_INTR_INTX_ASSERT(i)) > + generic_handle_domain_irq(pcie->intx_domain, i); > + } > + > + return IRQ_HANDLED; > +} > + > +static void amd_mdb_event_irq_mask(struct irq_data *d) { > + struct amd_mdb_pcie *pcie = irq_data_get_irq_chip_data(d); > + struct dw_pcie *pci = &pcie->pci; > + struct dw_pcie_rp *port = &pci->pp; > + u32 val; > + > + raw_spin_lock(&port->lock); > + val = pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC); This register is accessed in the IRQ handler also. So don't you need raw_spin_lock_irq{save/restore}? - Thank you for reviewing, In handler I am just reading the status & calling this irq_mask/irq_unmask API's I don't need to have save/restore spin_lock_irq's here. > + val &= ~BIT(d->hwirq); > + pcie_write(pcie, val, AMD_MDB_TLP_IR_STATUS_MISC); > + raw_spin_unlock(&port->lock); > +} > + > +static void amd_mdb_event_irq_unmask(struct irq_data *d) { > + struct amd_mdb_pcie *pcie = irq_data_get_irq_chip_data(d); > + struct dw_pcie *pci = &pcie->pci; > + struct dw_pcie_rp *port = &pci->pp; > + u32 val; > + > + raw_spin_lock(&port->lock); Same here > + val = pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC); > + val |= BIT(d->hwirq); > + pcie_write(pcie, val, AMD_MDB_TLP_IR_STATUS_MISC); > + raw_spin_unlock(&port->lock); > +} > + > +static struct irq_chip amd_mdb_event_irq_chip = { > + .name = "AMD MDB RC-Event", > + .irq_mask = amd_mdb_event_irq_mask, > + .irq_unmask = amd_mdb_event_irq_unmask, > +}; > + > +static int amd_mdb_pcie_event_map(struct irq_domain *domain, > + unsigned int irq, irq_hw_number_t hwirq) { > + irq_set_chip_and_handler(irq, &amd_mdb_event_irq_chip, > + handle_level_irq); > + irq_set_chip_data(irq, domain->host_data); > + irq_set_status_flags(irq, IRQ_LEVEL); > + > + return 0; > +} > + > +static const struct irq_domain_ops event_domain_ops = { > + .map = amd_mdb_pcie_event_map, > +}; > + > +static irqreturn_t amd_mdb_pcie_event_flow(int irq, void *args) { > + struct amd_mdb_pcie *pcie = args; > + unsigned long val; > + int i; > + > + val = pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC); Here also, no lock? - Thank you for reviewing, In handler I am just reading the status & calling this irq_mask/irq_unmask API's I don't need to have save/restore spin_lock_irq's here. > + val &= ~pcie_read(pcie, AMD_MDB_TLP_IR_MASK_MISC); > + for_each_set_bit(i, &val, 32) > + generic_handle_domain_irq(pcie->mdb_domain, i); > + pcie_write(pcie, val, AMD_MDB_TLP_IR_STATUS_MISC); > + > + return IRQ_HANDLED; > +} > + > +static void amd_mdb_pcie_free_irq_domains(struct amd_mdb_pcie *pcie) > +{ > + if (pcie->intx_domain) { > + irq_domain_remove(pcie->intx_domain); > + pcie->intx_domain = NULL; > + } > + > + if (pcie->mdb_domain) { > + irq_domain_remove(pcie->mdb_domain); > + pcie->mdb_domain = NULL; > + } > +} > + > +static int amd_mdb_pcie_init_port(struct amd_mdb_pcie *pcie) { > + /* Disable all TLP Interrupts */ > + pcie_write(pcie, AMD_MDB_PCIE_IMR_ALL_MASK, > + AMD_MDB_TLP_IR_DISABLE_MISC); > + > + /* Clear pending TLP interrupts */ > + pcie_write(pcie, pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC) & > + AMD_MDB_PCIE_IMR_ALL_MASK, > + AMD_MDB_TLP_IR_STATUS_MISC); > + > + /* Enable all TLP Interrupts */ > + pcie_write(pcie, AMD_MDB_PCIE_IMR_ALL_MASK, > + AMD_MDB_TLP_IR_ENABLE_MISC); > + > + return 0; > +} > + > +/** > + * amd_mdb_pcie_init_irq_domains - Initialize IRQ domain > + * @pcie: PCIe port information > + * @pdev: platform device > + * Return: '0' on success and error value on failure */ static int > +amd_mdb_pcie_init_irq_domains(struct amd_mdb_pcie *pcie, > + struct platform_device *pdev) > +{ > + struct dw_pcie *pci = &pcie->pci; > + struct dw_pcie_rp *pp = &pci->pp; > + struct device *dev = &pdev->dev; > + struct device_node *node = dev->of_node; > + struct device_node *pcie_intc_node; > + > + /* Setup INTx */ This comment is quite misleading. This Intc node is not specific to INTx as far as I can see. Thank you for reviewing, I ll remove this comment. > + pcie_intc_node = of_get_next_child(node, NULL); > + if (!pcie_intc_node) { > + dev_err(dev, "No PCIe Intc node found\n"); > + return -ENODATA; -ENODEV Thank you for reviewing, Ll update this in next patch. > + } > + > + pcie->mdb_domain = irq_domain_add_linear(pcie_intc_node, 32, > + &event_domain_ops, pcie); > + if (!pcie->mdb_domain) { > + dev_err(dev, "Failed to add mdb_domain\n"); > + goto out; > + } > + > + irq_domain_update_bus_token(pcie->mdb_domain, DOMAIN_BUS_NEXUS); > + > + pcie->intx_domain = irq_domain_add_linear(pcie_intc_node, PCI_NUM_INTX, > + &amd_intx_domain_ops, pcie); > + if (!pcie->intx_domain) { > + dev_err(dev, "Failed to add intx_domain\n"); > + goto mdb_out; > + } > + > + of_node_put(pcie_intc_node); > + irq_domain_update_bus_token(pcie->intx_domain, DOMAIN_BUS_WIRED); > + > + raw_spin_lock_init(&pp->lock); > + > + return 0; > +mdb_out: > + amd_mdb_pcie_free_irq_domains(pcie); > +out: > + of_node_put(pcie_intc_node); > + > + return -ENOMEM; No please. This is just asking for trouble. Please pass the relevant errno. Thank you for reviewing, ll update this in next patch. > +} > + > +static irqreturn_t amd_mdb_pcie_intr_handler(int irq, void *args) { > + struct amd_mdb_pcie *pcie = args; > + struct device *dev; > + struct irq_data *d; > + > + dev = pcie->pci.dev; > + > + /* > + * In future, error reporting will be hooked to the AER subsystem. > + * Currently, the driver prints a warning message to the user. > + */ > + d = irq_domain_get_irq_data(pcie->mdb_domain, irq); > + if (intr_cause[d->hwirq].str) > + dev_warn(dev, "%s\n", intr_cause[d->hwirq].str); > + else > + dev_warn_once(dev, "Unknown IRQ %ld\n", d->hwirq); > + > + return IRQ_HANDLED; > +} > + > +static int amd_mdb_setup_irq(struct amd_mdb_pcie *pcie, > + struct platform_device *pdev) { > + struct dw_pcie *pci = &pcie->pci; > + struct dw_pcie_rp *pp = &pci->pp; > + struct device *dev = &pdev->dev; > + int i, irq, err; > + > + amd_mdb_pcie_init_port(pcie); > + > + pp->irq = platform_get_irq(pdev, 0); > + if (pp->irq < 0) > + return pp->irq; > + > + for (i = 0; i < ARRAY_SIZE(intr_cause); i++) { > + if (!intr_cause[i].str) > + continue; > + irq = irq_create_mapping(pcie->mdb_domain, i); > + if (!irq) { > + dev_err(dev, "Failed to map mdb domain interrupt\n"); > + return -ENOMEM; > + } > + err = devm_request_irq(dev, irq, amd_mdb_pcie_intr_handler, > + IRQF_SHARED | IRQF_NO_THREAD, > + intr_cause[i].sym, pcie); Aren't these IRQs just part of a single IRQ? I'm wondering why do you need to represent them individually instead of having a single IRQ handler. Btw, you are not disposing these IRQs anywhere. Especially in error paths. Thank you for reviewing. This code is based on the work authored by Marc Zyngier and Bjorn during the development of our CPM drivers, and it follows the same design principles. The individual IRQ handling is consistent with that approach. For reference, you can review the driver here: pcie-xilinx-cpm.c. All of your comments have been incorporated into this driver as requested. > + if (err) { > + dev_err(dev, "Failed to request IRQ %d\n", irq); > + return err; > + } > + } > + > + pcie->intx_irq = irq_create_mapping(pcie->mdb_domain, > + AMD_MDB_PCIE_INTR_INTX); > + if (!pcie->intx_irq) { > + dev_err(dev, "Failed to map INTx interrupt\n"); > + return -ENXIO; > + } > + > + err = devm_request_irq(dev, pcie->intx_irq, > + dw_pcie_rp_intx_flow, > + IRQF_SHARED | IRQF_NO_THREAD, NULL, pcie); > + if (err) { > + dev_err(dev, "Failed to request INTx IRQ %d\n", irq); > + return err; > + } > + > + /* Plug the main event handler */ > + err = devm_request_irq(dev, pp->irq, amd_mdb_pcie_event_flow, > + IRQF_SHARED | IRQF_NO_THREAD, "amd_mdb pcie_irq", Why is this a SHARED IRQ? Thank you for reviewing. The IRQ is shared because all the events, errors, and INTx interrupts are routed through the same IRQ line, so multiple handlers need to be able to respond to the same interrupt. - Mani Regards, Thippeswamy H
On Mon, Feb 24, 2025 at 11:05:19AM +0000, Havalige, Thippeswamy wrote: [...] > > +#define AMD_MDB_TLP_IR_STATUS_MISC 0x4C0 > > +#define AMD_MDB_TLP_IR_MASK_MISC 0x4C4 > > +#define AMD_MDB_TLP_IR_ENABLE_MISC 0x4C8 > > +#define AMD_MDB_TLP_IR_DISABLE_MISC 0x4CC > > + > > +#define AMD_MDB_TLP_PCIE_INTX_MASK GENMASK(23, 16) > > + > > +#define AMD_MDB_PCIE_INTR_INTX_ASSERT(x) BIT((x) * 2) > > How does these values correspond to the AMD_MDB_TLP_PCIE_INTX_MASK? These values could be: 0, 2, 4, and 6 corresponding to: 0b01010101? Looks wierd. I don't know if it is your mailer issue or your workflow. Looks like my review comments are copy pasted here. So it becomes harder to distinguish between my previous comments and your replies. Please fix it. > Thank you for reviewing, Yes in register status/Enable/Disable register bits are laid in this way. > > > + > > +/* Interrupt registers definitions */ > > +#define AMD_MDB_PCIE_INTR_CMPL_TIMEOUT 15 > > +#define AMD_MDB_PCIE_INTR_INTX 16 > > +#define AMD_MDB_PCIE_INTR_PM_PME_RCVD 24 > > > > +static inline u32 pcie_read(struct amd_mdb_pcie *pcie, u32 reg) { > > + return readl_relaxed(pcie->slcr + reg); } > > I think I already commented on these helpers. Please get rid of them. I don't see any value in this new driver. Moreover, 'inline' keywords are not required. > Thanks for the review. While I agree to remove the 'inline', I would like pcie_read/pcie_write APIs. Could you please clarify the reason for explicitly removing pcie_read/pcie_write here? > If I remove the pcie_read/pcie_write, it will require changes in multiple places throughout the code." What value does the helper really add? It just wraps the {readl/writel}_relaxed calls. Plus it hides which kind of I/O accessors are used. So I don't see a value in keeping them. > > > + > > +static inline void pcie_write(struct amd_mdb_pcie *pcie, > > + u32 val, u32 reg) > > +{ > > + writel_relaxed(val, pcie->slcr + reg); } > > + > > +static const struct irq_domain_ops amd_intx_domain_ops = { > > + .map = amd_mdb_pcie_intx_map, > > +}; > > + > > +static irqreturn_t dw_pcie_rp_intx_flow(int irq, void *args) > > What does the _flow suffix mean? > Thanks for reviewing, The _flow suffix in the function name dw_pcie_rp_intx_flow indicates that the function handles the flow or processing related to interrupt handling for the PCIe root port's INTx interrupts through generic_handle_domain_irq. > (Please wrap your replies to 80 column width) So it is the regular interrupt handler. I don't see a necessity to add the _flow suffix. > > +{ > > + struct amd_mdb_pcie *pcie = args; > > + unsigned long val; > > + int i, int_status; > > + > > + val = pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC); > > You don't need port->lock here? > Thank you for reviewing. In this case, I'm simply reading the status of the INTX register bits without modifying any registers. > Since no shared resources are being updated or accessed concurrently, there’s no need for a lock here. > What if the handler and mask/unmask functions are executed in different CPUs? Sharing the same register without lock feels nervous. Locking principle is that you would lock both read as well as write. > > > + int_status = FIELD_GET(AMD_MDB_TLP_PCIE_INTX_MASK, val); > > You don't need to ack/clear the IRQ? > - Thank you for reviewing, Thank you for reviewing. In this case, I am using IRQ domains, and the generic_handle_domain_irq function will invoke the necessary irq_mask and irq_unmask operations internally, which will take care of clearing the IRQ. > Ok. > > + > > + for (i = 0; i < PCI_NUM_INTX; i++) { > > + if (int_status & AMD_MDB_PCIE_INTR_INTX_ASSERT(i)) > > + generic_handle_domain_irq(pcie->intx_domain, i); > > + } > > + > > + return IRQ_HANDLED; > > +} > > + > > +static void amd_mdb_event_irq_mask(struct irq_data *d) { > > + struct amd_mdb_pcie *pcie = irq_data_get_irq_chip_data(d); > > + struct dw_pcie *pci = &pcie->pci; > > + struct dw_pcie_rp *port = &pci->pp; > > + u32 val; > > + > > + raw_spin_lock(&port->lock); > > + val = pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC); > > This register is accessed in the IRQ handler also. So don't you need raw_spin_lock_irq{save/restore}? > - Thank you for reviewing, In handler I am just reading the status & calling this irq_mask/irq_unmask API's I don't need to have save/restore spin_lock_irq's here. > Same as above. > > + val &= ~BIT(d->hwirq); > > + pcie_write(pcie, val, AMD_MDB_TLP_IR_STATUS_MISC); > > + raw_spin_unlock(&port->lock); > > +} > > + [...] > > + for (i = 0; i < ARRAY_SIZE(intr_cause); i++) { > > + if (!intr_cause[i].str) > > + continue; > > + irq = irq_create_mapping(pcie->mdb_domain, i); > > + if (!irq) { > > + dev_err(dev, "Failed to map mdb domain interrupt\n"); > > + return -ENOMEM; > > + } > > + err = devm_request_irq(dev, irq, amd_mdb_pcie_intr_handler, > > + IRQF_SHARED | IRQF_NO_THREAD, > > + intr_cause[i].sym, pcie); > > Aren't these IRQs just part of a single IRQ? I'm wondering why do you need to represent them individually instead of having a single IRQ handler. > > Btw, you are not disposing these IRQs anywhere. Especially in error paths. > Thank you for reviewing. This code is based on the work authored by Marc Zyngier and Bjorn during the development of our CPM drivers, and it follows the same design principles. The individual IRQ handling is consistent with that approach. > For reference, you can review the driver here: pcie-xilinx-cpm.c. All of your comments have been incorporated into this driver as requested. > Ok for the separate IRQ question. But you still need to dispose the IRQs in error path. > > + if (err) { > > + dev_err(dev, "Failed to request IRQ %d\n", irq); > > + return err; > > + } > > + } > > + > > + pcie->intx_irq = irq_create_mapping(pcie->mdb_domain, > > + AMD_MDB_PCIE_INTR_INTX); > > + if (!pcie->intx_irq) { > > + dev_err(dev, "Failed to map INTx interrupt\n"); > > + return -ENXIO; > > + } > > + > > + err = devm_request_irq(dev, pcie->intx_irq, > > + dw_pcie_rp_intx_flow, > > + IRQF_SHARED | IRQF_NO_THREAD, NULL, pcie); > > + if (err) { > > + dev_err(dev, "Failed to request INTx IRQ %d\n", irq); > > + return err; > > + } > > + > > + /* Plug the main event handler */ > > + err = devm_request_irq(dev, pp->irq, amd_mdb_pcie_event_flow, > > + IRQF_SHARED | IRQF_NO_THREAD, "amd_mdb pcie_irq", > > Why is this a SHARED IRQ? > Thank you for reviewing. The IRQ is shared because all the events, errors, and INTx interrupts are routed through the same IRQ line, so multiple handlers need to be able to respond to the same interrupt. IIUC, you have a single handler for this IRQ and that handler is invoking other handlers (for events, INTx etc...). So I don't see how this IRQ is shared. Shared IRQ is only required if multiple handlers are sharing the same IRQ line. But that is not the case here afaics. - Mani
diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig index b6d6778b0698..882f4b558133 100644 --- a/drivers/pci/controller/dwc/Kconfig +++ b/drivers/pci/controller/dwc/Kconfig @@ -27,6 +27,17 @@ config PCIE_AL required only for DT-based platforms. ACPI platforms with the Annapurna Labs PCIe controller don't need to enable this. +config PCIE_AMD_MDB + bool "AMD MDB Versal2 PCIe Host controller" + depends on OF || COMPILE_TEST + depends on PCI && PCI_MSI + select PCIE_DW_HOST + help + Say Y here if you want to enable PCIe controller support on AMD + Versal2 SoCs. The AMD MDB Versal2 PCIe controller is based on + DesignWare IP and therefore the driver re-uses the Designware core + functions to implement the driver. + config PCI_MESON tristate "Amlogic Meson PCIe controller" default m if ARCH_MESON diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile index a8308d9ea986..ae27eda6ec5e 100644 --- a/drivers/pci/controller/dwc/Makefile +++ b/drivers/pci/controller/dwc/Makefile @@ -3,6 +3,7 @@ obj-$(CONFIG_PCIE_DW) += pcie-designware.o obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o +obj-$(CONFIG_PCIE_AMD_MDB) += pcie-amd-mdb.o obj-$(CONFIG_PCIE_BT1) += pcie-bt1.o obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o obj-$(CONFIG_PCI_EXYNOS) += pci-exynos.o diff --git a/drivers/pci/controller/dwc/pcie-amd-mdb.c b/drivers/pci/controller/dwc/pcie-amd-mdb.c new file mode 100644 index 000000000000..01d27cb5a70d --- /dev/null +++ b/drivers/pci/controller/dwc/pcie-amd-mdb.c @@ -0,0 +1,481 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * PCIe host controller driver for AMD MDB PCIe Bridge + * + * Copyright (C) 2024-2025, Advanced Micro Devices, Inc. + */ + +#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/gpio.h> +#include <linux/interrupt.h> +#include <linux/irqdomain.h> +#include <linux/kernel.h> +#include <linux/init.h> +#include <linux/of_device.h> +#include <linux/pci.h> +#include <linux/platform_device.h> +#include <linux/resource.h> +#include <linux/types.h> + +#include "pcie-designware.h" + +#define AMD_MDB_TLP_IR_STATUS_MISC 0x4C0 +#define AMD_MDB_TLP_IR_MASK_MISC 0x4C4 +#define AMD_MDB_TLP_IR_ENABLE_MISC 0x4C8 +#define AMD_MDB_TLP_IR_DISABLE_MISC 0x4CC + +#define AMD_MDB_TLP_PCIE_INTX_MASK GENMASK(23, 16) + +#define AMD_MDB_PCIE_INTR_INTX_ASSERT(x) BIT((x) * 2) + +/* Interrupt registers definitions */ +#define AMD_MDB_PCIE_INTR_CMPL_TIMEOUT 15 +#define AMD_MDB_PCIE_INTR_INTX 16 +#define AMD_MDB_PCIE_INTR_PM_PME_RCVD 24 +#define AMD_MDB_PCIE_INTR_PME_TO_ACK_RCVD 25 +#define AMD_MDB_PCIE_INTR_MISC_CORRECTABLE 26 +#define AMD_MDB_PCIE_INTR_NONFATAL 27 +#define AMD_MDB_PCIE_INTR_FATAL 28 + +#define IMR(x) BIT(AMD_MDB_PCIE_INTR_ ##x) +#define AMD_MDB_PCIE_IMR_ALL_MASK \ + ( \ + IMR(CMPL_TIMEOUT) | \ + IMR(PM_PME_RCVD) | \ + IMR(PME_TO_ACK_RCVD) | \ + IMR(MISC_CORRECTABLE) | \ + IMR(NONFATAL) | \ + IMR(FATAL) | \ + AMD_MDB_TLP_PCIE_INTX_MASK \ + ) + +/** + * struct amd_mdb_pcie - PCIe port information + * @pci: DesignWare PCIe controller structure + * @slcr: MDB System Level Control and Status Register (SLCR) Base + * @intx_domain: INTx IRQ domain pointer + * @mdb_domain: MDB IRQ domain pointer + * @intx_irq: INTx IRQ interrupt number + */ +struct amd_mdb_pcie { + struct dw_pcie pci; + void __iomem *slcr; + struct irq_domain *intx_domain; + struct irq_domain *mdb_domain; + int intx_irq; +}; + +static const struct dw_pcie_host_ops amd_mdb_pcie_host_ops = { +}; + +static inline u32 pcie_read(struct amd_mdb_pcie *pcie, u32 reg) +{ + return readl_relaxed(pcie->slcr + reg); +} + +static inline void pcie_write(struct amd_mdb_pcie *pcie, + u32 val, u32 reg) +{ + writel_relaxed(val, pcie->slcr + reg); +} + +static void amd_mdb_intx_irq_mask(struct irq_data *data) +{ + struct amd_mdb_pcie *pcie = irq_data_get_irq_chip_data(data); + struct dw_pcie *pci = &pcie->pci; + struct dw_pcie_rp *port = &pci->pp; + unsigned long flags; + u32 val; + + raw_spin_lock_irqsave(&port->lock, flags); + val = FIELD_PREP(AMD_MDB_TLP_PCIE_INTX_MASK, + AMD_MDB_PCIE_INTR_INTX_ASSERT(data->hwirq)); + + /* + * Writing '1' to a bit in AMD_MDB_TLP_IR_DISABLE_MISC disables that + * interrupt, writing '0' has no effect. + */ + pcie_write(pcie, val, AMD_MDB_TLP_IR_DISABLE_MISC); + raw_spin_unlock_irqrestore(&port->lock, flags); +} + +static void amd_mdb_intx_irq_unmask(struct irq_data *data) +{ + struct amd_mdb_pcie *pcie = irq_data_get_irq_chip_data(data); + struct dw_pcie *pci = &pcie->pci; + struct dw_pcie_rp *port = &pci->pp; + unsigned long flags; + u32 val; + + raw_spin_lock_irqsave(&port->lock, flags); + val = FIELD_PREP(AMD_MDB_TLP_PCIE_INTX_MASK, + AMD_MDB_PCIE_INTR_INTX_ASSERT(data->hwirq)); + + /* + * Writing '1' to a bit in AMD_MDB_TLP_IR_ENABLE_MISC enables that interrupt. + * Writing '0' has no effect. + */ + pcie_write(pcie, val, AMD_MDB_TLP_IR_ENABLE_MISC); + raw_spin_unlock_irqrestore(&port->lock, flags); +} + +static struct irq_chip amd_mdb_intx_irq_chip = { + .name = "AMD MDB INTx", + .irq_mask = amd_mdb_intx_irq_mask, + .irq_unmask = amd_mdb_intx_irq_unmask, +}; + +/** + * amd_mdb_pcie_intx_map - Set the handler for the INTx and mark IRQ + * as valid + * @domain: IRQ domain + * @irq: Virtual IRQ number + * @hwirq: HW interrupt number + * + * Return: Always returns 0. + */ +static int amd_mdb_pcie_intx_map(struct irq_domain *domain, + unsigned int irq, irq_hw_number_t hwirq) +{ + irq_set_chip_and_handler(irq, &amd_mdb_intx_irq_chip, + handle_level_irq); + irq_set_chip_data(irq, domain->host_data); + irq_set_status_flags(irq, IRQ_LEVEL); + + return 0; +} + +/* INTx IRQ Domain operations */ +static const struct irq_domain_ops amd_intx_domain_ops = { + .map = amd_mdb_pcie_intx_map, +}; + +static irqreturn_t dw_pcie_rp_intx_flow(int irq, void *args) +{ + struct amd_mdb_pcie *pcie = args; + unsigned long val; + int i, int_status; + + val = pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC); + int_status = FIELD_GET(AMD_MDB_TLP_PCIE_INTX_MASK, val); + + for (i = 0; i < PCI_NUM_INTX; i++) { + if (int_status & AMD_MDB_PCIE_INTR_INTX_ASSERT(i)) + generic_handle_domain_irq(pcie->intx_domain, i); + } + + return IRQ_HANDLED; +} + +#define _IC(x, s)[AMD_MDB_PCIE_INTR_ ## x] = { __stringify(x), s } + +static const struct { + const char *sym; + const char *str; +} intr_cause[32] = { + _IC(CMPL_TIMEOUT, "completion timeout"), + _IC(PM_PME_RCVD, "PM_PME message received"), + _IC(PME_TO_ACK_RCVD, "PME_TO_ACK message received"), + _IC(MISC_CORRECTABLE, "Correctable error message"), + _IC(NONFATAL, "Non fatal error message"), + _IC(FATAL, "Fatal error message"), +}; + +static void amd_mdb_event_irq_mask(struct irq_data *d) +{ + struct amd_mdb_pcie *pcie = irq_data_get_irq_chip_data(d); + struct dw_pcie *pci = &pcie->pci; + struct dw_pcie_rp *port = &pci->pp; + u32 val; + + raw_spin_lock(&port->lock); + val = pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC); + val &= ~BIT(d->hwirq); + pcie_write(pcie, val, AMD_MDB_TLP_IR_STATUS_MISC); + raw_spin_unlock(&port->lock); +} + +static void amd_mdb_event_irq_unmask(struct irq_data *d) +{ + struct amd_mdb_pcie *pcie = irq_data_get_irq_chip_data(d); + struct dw_pcie *pci = &pcie->pci; + struct dw_pcie_rp *port = &pci->pp; + u32 val; + + raw_spin_lock(&port->lock); + val = pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC); + val |= BIT(d->hwirq); + pcie_write(pcie, val, AMD_MDB_TLP_IR_STATUS_MISC); + raw_spin_unlock(&port->lock); +} + +static struct irq_chip amd_mdb_event_irq_chip = { + .name = "AMD MDB RC-Event", + .irq_mask = amd_mdb_event_irq_mask, + .irq_unmask = amd_mdb_event_irq_unmask, +}; + +static int amd_mdb_pcie_event_map(struct irq_domain *domain, + unsigned int irq, irq_hw_number_t hwirq) +{ + irq_set_chip_and_handler(irq, &amd_mdb_event_irq_chip, + handle_level_irq); + irq_set_chip_data(irq, domain->host_data); + irq_set_status_flags(irq, IRQ_LEVEL); + + return 0; +} + +static const struct irq_domain_ops event_domain_ops = { + .map = amd_mdb_pcie_event_map, +}; + +static irqreturn_t amd_mdb_pcie_event_flow(int irq, void *args) +{ + struct amd_mdb_pcie *pcie = args; + unsigned long val; + int i; + + val = pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC); + val &= ~pcie_read(pcie, AMD_MDB_TLP_IR_MASK_MISC); + for_each_set_bit(i, &val, 32) + generic_handle_domain_irq(pcie->mdb_domain, i); + pcie_write(pcie, val, AMD_MDB_TLP_IR_STATUS_MISC); + + return IRQ_HANDLED; +} + +static void amd_mdb_pcie_free_irq_domains(struct amd_mdb_pcie *pcie) +{ + if (pcie->intx_domain) { + irq_domain_remove(pcie->intx_domain); + pcie->intx_domain = NULL; + } + + if (pcie->mdb_domain) { + irq_domain_remove(pcie->mdb_domain); + pcie->mdb_domain = NULL; + } +} + +static int amd_mdb_pcie_init_port(struct amd_mdb_pcie *pcie) +{ + /* Disable all TLP Interrupts */ + pcie_write(pcie, AMD_MDB_PCIE_IMR_ALL_MASK, + AMD_MDB_TLP_IR_DISABLE_MISC); + + /* Clear pending TLP interrupts */ + pcie_write(pcie, pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC) & + AMD_MDB_PCIE_IMR_ALL_MASK, + AMD_MDB_TLP_IR_STATUS_MISC); + + /* Enable all TLP Interrupts */ + pcie_write(pcie, AMD_MDB_PCIE_IMR_ALL_MASK, + AMD_MDB_TLP_IR_ENABLE_MISC); + + return 0; +} + +/** + * amd_mdb_pcie_init_irq_domains - Initialize IRQ domain + * @pcie: PCIe port information + * @pdev: platform device + * Return: '0' on success and error value on failure + */ +static int amd_mdb_pcie_init_irq_domains(struct amd_mdb_pcie *pcie, + struct platform_device *pdev) +{ + struct dw_pcie *pci = &pcie->pci; + struct dw_pcie_rp *pp = &pci->pp; + struct device *dev = &pdev->dev; + struct device_node *node = dev->of_node; + struct device_node *pcie_intc_node; + + /* Setup INTx */ + pcie_intc_node = of_get_next_child(node, NULL); + if (!pcie_intc_node) { + dev_err(dev, "No PCIe Intc node found\n"); + return -ENODATA; + } + + pcie->mdb_domain = irq_domain_add_linear(pcie_intc_node, 32, + &event_domain_ops, pcie); + if (!pcie->mdb_domain) { + dev_err(dev, "Failed to add mdb_domain\n"); + goto out; + } + + irq_domain_update_bus_token(pcie->mdb_domain, DOMAIN_BUS_NEXUS); + + pcie->intx_domain = irq_domain_add_linear(pcie_intc_node, PCI_NUM_INTX, + &amd_intx_domain_ops, pcie); + if (!pcie->intx_domain) { + dev_err(dev, "Failed to add intx_domain\n"); + goto mdb_out; + } + + of_node_put(pcie_intc_node); + irq_domain_update_bus_token(pcie->intx_domain, DOMAIN_BUS_WIRED); + + raw_spin_lock_init(&pp->lock); + + return 0; +mdb_out: + amd_mdb_pcie_free_irq_domains(pcie); +out: + of_node_put(pcie_intc_node); + + return -ENOMEM; +} + +static irqreturn_t amd_mdb_pcie_intr_handler(int irq, void *args) +{ + struct amd_mdb_pcie *pcie = args; + struct device *dev; + struct irq_data *d; + + dev = pcie->pci.dev; + + /* + * In future, error reporting will be hooked to the AER subsystem. + * Currently, the driver prints a warning message to the user. + */ + d = irq_domain_get_irq_data(pcie->mdb_domain, irq); + if (intr_cause[d->hwirq].str) + dev_warn(dev, "%s\n", intr_cause[d->hwirq].str); + else + dev_warn_once(dev, "Unknown IRQ %ld\n", d->hwirq); + + return IRQ_HANDLED; +} + +static int amd_mdb_setup_irq(struct amd_mdb_pcie *pcie, + struct platform_device *pdev) +{ + struct dw_pcie *pci = &pcie->pci; + struct dw_pcie_rp *pp = &pci->pp; + struct device *dev = &pdev->dev; + int i, irq, err; + + amd_mdb_pcie_init_port(pcie); + + pp->irq = platform_get_irq(pdev, 0); + if (pp->irq < 0) + return pp->irq; + + for (i = 0; i < ARRAY_SIZE(intr_cause); i++) { + if (!intr_cause[i].str) + continue; + irq = irq_create_mapping(pcie->mdb_domain, i); + if (!irq) { + dev_err(dev, "Failed to map mdb domain interrupt\n"); + return -ENOMEM; + } + err = devm_request_irq(dev, irq, amd_mdb_pcie_intr_handler, + IRQF_SHARED | IRQF_NO_THREAD, + intr_cause[i].sym, pcie); + if (err) { + dev_err(dev, "Failed to request IRQ %d\n", irq); + return err; + } + } + + pcie->intx_irq = irq_create_mapping(pcie->mdb_domain, + AMD_MDB_PCIE_INTR_INTX); + if (!pcie->intx_irq) { + dev_err(dev, "Failed to map INTx interrupt\n"); + return -ENXIO; + } + + err = devm_request_irq(dev, pcie->intx_irq, + dw_pcie_rp_intx_flow, + IRQF_SHARED | IRQF_NO_THREAD, NULL, pcie); + if (err) { + dev_err(dev, "Failed to request INTx IRQ %d\n", irq); + return err; + } + + /* Plug the main event handler */ + err = devm_request_irq(dev, pp->irq, amd_mdb_pcie_event_flow, + IRQF_SHARED | IRQF_NO_THREAD, "amd_mdb pcie_irq", + pcie); + if (err) { + dev_err(dev, "Failed to request event IRQ %d\n", pp->irq); + return err; + } + + return 0; +} + +static int amd_mdb_add_pcie_port(struct amd_mdb_pcie *pcie, + struct platform_device *pdev) +{ + struct dw_pcie *pci = &pcie->pci; + struct dw_pcie_rp *pp = &pci->pp; + struct device *dev = &pdev->dev; + int ret; + + pcie->slcr = devm_platform_ioremap_resource_byname(pdev, "slcr"); + if (IS_ERR(pcie->slcr)) + return PTR_ERR(pcie->slcr); + + ret = amd_mdb_pcie_init_irq_domains(pcie, pdev); + if (ret) + return ret; + + ret = amd_mdb_setup_irq(pcie, pdev); + if (ret) { + dev_err(dev, "Failed to set up interrupts\n"); + goto out; + } + + pp->ops = &amd_mdb_pcie_host_ops; + + ret = dw_pcie_host_init(pp); + if (ret) { + dev_err(dev, "Failed to initialize host\n"); + goto out; + } + + return 0; + +out: + amd_mdb_pcie_free_irq_domains(pcie); + return ret; +} + +static int amd_mdb_pcie_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct amd_mdb_pcie *pcie; + struct dw_pcie *pci; + + pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL); + if (!pcie) + return -ENOMEM; + + pci = &pcie->pci; + pci->dev = dev; + + platform_set_drvdata(pdev, pcie); + + return amd_mdb_add_pcie_port(pcie, pdev); +} + +static const struct of_device_id amd_mdb_pcie_of_match[] = { + { + .compatible = "amd,versal2-mdb-host", + }, + {}, +}; + +static struct platform_driver amd_mdb_pcie_driver = { + .driver = { + .name = "amd-mdb-pcie", + .of_match_table = amd_mdb_pcie_of_match, + .suppress_bind_attrs = true, + }, + .probe = amd_mdb_pcie_probe, +}; +builtin_platform_driver(amd_mdb_pcie_driver);
Add support for AMD MDB (Multimedia DMA Bridge) IP core as Root Port. The Versal2 devices include MDB Module. The integrated block for MDB along with the integrated bridge can function as PCIe Root Port controller at Gen5 32-Gb/s operation per lane. Bridge supports error and legacy interrupts and are handled using platform specific interrupt line in Versal2. Signed-off-by: Thippeswamy Havalige <thippeswamy.havalige@amd.com> | Reported-by: kernel test robot <lkp@intel.com> | Closes: | https://lore.kernel.org/oe-kbuild-all/202502191741.xrVmEAG4-lkp@intel. | com/ --- changes in v2: ------------- - Update Gen5 speed in the patch description. - Modify Kconfig file. - Update string _leg_ to intx. - Get platform structure through automic variables. - Remove _rp_ in function. Changes in v3: -------------- -None. Changes in v4: -------------- -None. Changes in v5: -------------- -None. Changes in v6: -------------- - Remove pdev automatic variable. - Update register name to slcr. - Fix whitespace. - remove Spurious extra line. - Update Legacy to INTx. - Add space before (SLCR). - Update menuconfig description. Changes in v7: -------------- - None. Changes in v8: -------------- - Remove inline keyword. - Fix indentations. - Add AMD MDB prefix to interrupt names. - Remove Kernel doc. - Fix return types. - Modify dev_warn to dev_warn_once. - Add Intx handler & callbacks. Changes in v10: --------------- - Add intx assert & deassert macros. - Move amd_mdb_pcie_init_port function. - Add kernel doc for error warning messages. Changes in v11: --------------- - Remove intx deassert macro & generic handler. - Update Kconfig description. - Update INTx mask macro to handle only asser bits. - Move INTx handler. - Address other review comments. Changes in v12: --------------- - ADD TLP_IR_DISABLE_MISC register. - Modify intx call back function Changes in v13: - Add kernel doc for intx_irq Changes in v14: -------------- - Modify mask in intx_irq_mask/unmask functions. - Modify mask in intx_flow handler. --- drivers/pci/controller/dwc/Kconfig | 11 + drivers/pci/controller/dwc/Makefile | 1 + drivers/pci/controller/dwc/pcie-amd-mdb.c | 481 ++++++++++++++++++++++ 3 files changed, 493 insertions(+) create mode 100644 drivers/pci/controller/dwc/pcie-amd-mdb.c