Re: [PATCH v2 1/2] dmaengine: dw-edma: Add AMD MDB Endpoint Support

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Sep 16, 2025 at 04:13:18PM +0530, Devendra K Verma wrote:
> AMD MDB PCIe endpoint support. For AMD specific support
> added the following
>   - AMD supported PCIe Device IDs and Vendor ID (Xilinx).
>   - AMD MDB specific driver data
>   - AMD MDB specific VSEC capability to retrieve the device DDR
>     base address.

> +/* Synopsys */
>  #define DW_PCIE_VSEC_DMA_ID			0x6
>  #define DW_PCIE_VSEC_DMA_BAR			GENMASK(10, 8)
>  #define DW_PCIE_VSEC_DMA_MAP			GENMASK(2, 0)
>  #define DW_PCIE_VSEC_DMA_WR_CH			GENMASK(9, 0)
>  #define DW_PCIE_VSEC_DMA_RD_CH			GENMASK(25, 16)
>  
> +/* AMD MDB specific defines */
> +#define DW_PCIE_XILINX_MDB_VSEC_DMA_ID		0x6
> +#define DW_PCIE_XILINX_MDB_VSEC_ID		0x20
> +#define PCI_DEVICE_ID_AMD_MDB_B054		0xb054
> +#define DW_PCIE_AMD_MDB_INVALID_ADDR		(~0ULL)

> @@ -120,9 +213,22 @@ static void dw_edma_pcie_get_vsec_dma_data(struct pci_dev *pdev,
>  	u32 val, map;
>  	u16 vsec;
>  	u64 off;
> +	u16 vendor = pdev->vendor;
> +	int cap;
>  
> -	vsec = pci_find_vsec_capability(pdev, PCI_VENDOR_ID_SYNOPSYS,
> -					DW_PCIE_VSEC_DMA_ID);
> +	/*
> +	 * Synopsys and AMD (Xilinx) use the same VSEC ID for the purpose
> +	 * of map, channel counts, etc.
> +	 */
> +	if (vendor != PCI_VENDOR_ID_SYNOPSYS ||
> +	    vendor != PCI_VENDOR_ID_XILINX)
> +		return;
> +
> +	cap = DW_PCIE_VSEC_DMA_ID;
> +	if (vendor == PCI_VENDOR_ID_XILINX)
> +		cap = DW_PCIE_XILINX_MDB_VSEC_ID;
> +
> +	vsec = pci_find_vsec_capability(pdev, vendor, cap);

This looks correct, so it's OK as-is.  But it does require more
analysis to verify than it would if you did it like this:

  vsec = pci_find_vsec_capability(pdev, PCI_VENDOR_ID_SYNOPSYS,
                                  DW_PCIE_SYNOPSYS_VSEC_DMA_ID);
  if (!vsec) {
    vsec = pci_find_vsec_capability(pdev, PCI_VENDOR_ID_XILINX,
                                    DW_PCIE_XILINX_VSEC_DMA_ID);
    if (!vsec)
      return;
  }

This way it's obvious from the pci_find_vsec_capability() calls
themselves (and could potentially be checked by coccinelle, etc) that
we're using the Vendor ID and VSEC ID correctly.

> +	/* AMD specific VSEC capability */

This should say "Xilinx specific VSEC capability" because the Vendor
ID in the device is PCI_VENDOR_ID_XILINX.  We shouldn't have to look
up the corporate ownership history and figure out that AMD acquired
Xilinx.  That's not relevant in this context.

> +	vsec = pci_find_vsec_capability(pdev, vendor,
> +					DW_PCIE_XILINX_MDB_VSEC_ID);

But this one is wrong.  We do know that the device Vendor ID is either
PCI_VENDOR_ID_SYNOPSYS or PCI_VENDOR_ID_XILINX from above, but we
*don't* know what VSEC ID 0x20 means for Synopsys devices.

We only know what VSEC ID 0x20 means for Xilinx devices.  So this has
to be:

  vsec = pci_find_vsec_capability(pdev, PCI_VENDOR_ID_XILINX,
                                  DW_PCIE_XILINX_MDB_VSEC_ID);

Bjorn