On Tue, Feb 11, 2025 at 12:08:51PM +0530, Thippeswamy Havalige wrote:
> Add support for AMD MDB (Multimedia DMA Bridge) IP core as Root Port.
> +++ 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.
Wrap to fit in 75-78 columns like the rest of the file. This gets
chopped off in an 80 column menuconfig window.
> +++ b/drivers/pci/controller/dwc/pcie-amd-mdb.c
> +#define AMD_MDB_TLP_PCIE_INTX_MASK GENMASK(23, 16)
> +
> +#define AMD_MDB_PCIE_INTX_BIT(x) FIELD_PREP(AMD_MDB_TLP_PCIE_INTX_MASK, BIT(x))
> +
> +#define AMD_MDB_PCIE_INTR_INTX_ASSERT(x) BIT((x) * 2)
> +#define AMD_MDB_PCIE_INTR_INTX_DEASSERT(x) BIT(((x) * 2) + 1)
> +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 = pcie_read(pcie, AMD_MDB_TLP_IR_MASK_MISC);
> + val &= ~AMD_MDB_PCIE_INTX_BIT(data->hwirq);
This doesn't look right to me. hwirq should be 0, 1, 2, or 3 (INTA,
INTB, INTC, or INTD):
AMD_MDB_PCIE_INTX_BIT(0) == 0001 0000 (INTA assert)
AMD_MDB_PCIE_INTX_BIT(1) == 0002 0000 (INTA deassert)
AMD_MDB_PCIE_INTX_BIT(2) == 0004 0000 (INTB assert)
AMD_MDB_PCIE_INTX_BIT(3) == 0008 0000 (INTB deassert)
Maybe the AMD_MDB_TLP_IR_ENABLE_MISC register is laid out differently
than AMD_MDB_TLP_IR_STATUS_MISC? If so, and you're updating a
four-bit field, it needs a different GENMASK.
> + pcie_write(pcie, val, AMD_MDB_TLP_IR_ENABLE_MISC);
This *looks* like it's supposed to be a read/modify/write of
AMD_MDB_TLP_IR_MASK_MISC, but you read AMD_MDB_TLP_IR_MASK_MISC and
then write AMD_MDB_TLP_IR_ENABLE_MISC. Same below in
amd_mdb_intx_irq_unmask().
> + 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 = pcie_read(pcie, AMD_MDB_TLP_IR_MASK_MISC);
> + val |= AMD_MDB_PCIE_INTX_BIT(data->hwirq);
> + pcie_write(pcie, val, AMD_MDB_TLP_IR_ENABLE_MISC);
> + raw_spin_unlock_irqrestore(&port->lock, flags);
> +}
> +static irqreturn_t dw_pcie_rp_intx_flow(int irq, void *args)
It'd be nice if this were close in the source file to the INTx
mask/unmask above.
> +{
> + struct amd_mdb_pcie *pcie = args;
> + unsigned long val;
> + int i;
> +
> + val = FIELD_GET(AMD_MDB_TLP_PCIE_INTX_MASK,
> + pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC));
> +
> + for (i = 0; i < PCI_NUM_INTX; i++) {
> + if (val & AMD_MDB_PCIE_INTR_INTX_ASSERT(i))
> + generic_handle_domain_irq(pcie->intx_domain, i);
> + if (val & AMD_MDB_PCIE_INTR_INTX_DEASSERT(i)
> + generic_handle_domain_irq(pcie->intx_domain, (i * 2) + 1);
Why call generic_handle_domain_irq() for deassert? No other drivers
do that AFAIK. If you do need it, "(i * 2) + 1" looks completely
wrong; it should be the hwirq value.
> + }
> +
> + return IRQ_HANDLED;
> +}
> +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;
> +
> + /**
/* (not /**)
> + * 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)
> +{
> ...
> +
> + /* 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);
Wrap to fit in 80 columns like the rest of the file.
Bjorn
