Re: [PATCH v10 3/3] PCI: amd-mdb: Add AMD MDB Root Port driver

[Bjorn Helgaas <helgaas@kernel.org>]

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