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

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Dec 12, 2025 at 05:50:55PM +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.
> ...

> +++ b/drivers/dma/dw-edma/dw-edma-pcie.c

> +/* 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)

These should include "SYNOPSYS" since they are specific to
PCI_VENDOR_ID_SYNOPSYS.  Add corresponding XILINX #defines below for
the XILINX VSEC.  They'll be the same masks.

> +/* AMD MDB (Xilinx) 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

Looks odd to me that PCI_DEVICE_ID_AMD_MDB_B054 goes with
PCI_VENDOR_ID_XILINX.  To me this would make more sense as
PCI_DEVICE_ID_XILINX_B054.  Move it so it's not in the middle of the
VSEC-related things.

> +#define DW_PCIE_AMD_MDB_INVALID_ADDR		(~0ULL)

It looks like this is related to the value from
DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG and is only used for Xilinx, so
should be named similarly, e.g., DW_PCIE_XILINX_MDB_INVALID_ADDR, and
moved to be next to it.

> +#define DW_PCIE_XILINX_LL_OFF_GAP		0x200000
> +#define DW_PCIE_XILINX_LL_SIZE			0x800
> +#define DW_PCIE_XILINX_DT_OFF_GAP		0x100000
> +#define DW_PCIE_XILINX_DT_SIZE			0x800

These LL/DT gap and size #defines don't look like they're directly
related to the VSEC, so they should at least be moved after the
DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG #defines, since those *are* part of
the VSEC.

> +#define DW_PCIE_XILINX_MDB_VSEC_HDR_ID		0x20

DW_PCIE_XILINX_MDB_VSEC_HDR_ID is pointless and should be removed.
See below.

> +#define DW_PCIE_XILINX_MDB_VSEC_REV		0x1
> +#define DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_HIGH	0xc
> +#define DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_LOW	0x8

> +static const struct dw_edma_pcie_data amd_mdb_data = {

This is a confusing mix of "xilinx" and "amd_mdb".  The DEVICE_ID
#define uses "AMD_MDB".  The other #defines mostly use XILINX.  This
data structure uses "amd_mdb".  The function uses "xilinx".

Since this patch only applies to PCI_VENDOR_ID_XILINX, I would make
this "xilinx_mdb_data".  If new devices come with a different vendor
ID, e.g., AMD, you can add a corresponding block for that.

> +static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
> +					     struct dw_edma_pcie_data *pdata)
> +{
> +	u32 val, map;
> +	u16 vsec;
> +	u64 off;
> +
> +	vsec = pci_find_vsec_capability(pdev, PCI_VENDOR_ID_XILINX,
> +					DW_PCIE_XILINX_MDB_VSEC_DMA_ID);
> +	if (!vsec)
> +		return;
> +
> +	pci_read_config_dword(pdev, vsec + PCI_VNDR_HEADER, &val);
> +	if (PCI_VNDR_HEADER_REV(val) != 0x00 ||
> +	    PCI_VNDR_HEADER_LEN(val) != 0x18)
> +		return;
> +
> +	pci_dbg(pdev, "Detected PCIe Vendor-Specific Extended Capability DMA\n");

Perhaps reword this to "Detected Xilinx Vendor-Specific Extended
Capability DMA", and the one in dw_edma_pcie_get_synopsys_dma_data()
to similarly mention "Synopsys" to reinforce the fact that these are
Xilinx-specific and Synopsys-specific.

I think the REV and LEN checks are unnecessary; see below.

> +	pci_read_config_dword(pdev, vsec + 0x8, &val);
> +	map = FIELD_GET(DW_PCIE_VSEC_DMA_MAP, val);

Should use XILINX #defines.  Another reason for adding "SYNOPSYS" to
the #defines for the Synopsys VSEC.

> +	vsec = pci_find_vsec_capability(pdev, PCI_VENDOR_ID_XILINX,
> +					DW_PCIE_XILINX_MDB_VSEC_ID);
> +	if (!vsec)
> +		return;
> +
> +	pci_read_config_dword(pdev, vsec + PCI_VNDR_HEADER, &val);
> +	if (PCI_VNDR_HEADER_ID(val) != DW_PCIE_XILINX_MDB_VSEC_HDR_ID ||

pci_find_vsec_capability() already checks that PCI_VNDR_HEADER_ID() ==
DW_PCIE_XILINX_MDB_VSEC_ID, so there's no need to check this again.

> +	    PCI_VNDR_HEADER_REV(val) != DW_PCIE_XILINX_MDB_VSEC_REV)

I know this is copied from dw_edma_pcie_get_vsec_dma_data(), but I
think it's a bad idea to check for the exact revision because it
forces a change to existing, working code if there's ever a device
with a new revision of this VSEC ID.

If there are new revisions of this VSEC, they should preserve the
semantics of previous revisions.  If there was a rev 0 of this VSEC, I
think we should check for PCI_VNDR_HEADER_REV() >= 1.  If rev 1 was
the first revision, you could skip the check altogether.

If rev 2 *adds* new registers or functionality, we would have to add
new code to support that, and *that* code should check for
PCI_VNDR_HEADER_REV() >= 2.

I think the REV and LEN checking in dw_edma_pcie_get_vsec_dma_data()
is also too aggressive.

>  static int dw_edma_pcie_probe(struct pci_dev *pdev,
>  			      const struct pci_device_id *pid)
>  {
> @@ -179,12 +318,34 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
>  	}
>  
>  	memcpy(vsec_data, pdata, sizeof(struct dw_edma_pcie_data));
> +	vsec_data->devmem_phys_off = DW_PCIE_AMD_MDB_INVALID_ADDR;

Seems weird to set devmem_phys_off here since it's only used for
PCI_VENDOR_ID_XILINX.  Couldn't this be done in
dw_edma_pcie_get_xilinx_dma_data()?

> -	dw_edma_pcie_get_vsec_dma_data(pdev, vsec_data);
> +	dw_edma_pcie_get_synopsys_dma_data(pdev, vsec_data);
> +	dw_edma_pcie_get_xilinx_dma_data(pdev, vsec_data);
> +
> +	if (pdev->vendor == PCI_VENDOR_ID_XILINX) {
> +		/*
> +		 * There is no valid address found for the LL memory
> +		 * space on the device side.
> +		 */
> +		if (vsec_data->devmem_phys_off == DW_PCIE_AMD_MDB_INVALID_ADDR)
> +			return -ENOMEM;
> +
> +		/*
> +		 * Configure the channel LL and data blocks if number of
> +		 * channels enabled in VSEC capability are more than the
> +		 * channels configured in amd_mdb_data.
> +		 */
> +		dw_edma_set_chan_region_offset(vsec_data, BAR_2, 0,
> +					       DW_PCIE_XILINX_LL_OFF_GAP,
> +					       DW_PCIE_XILINX_LL_SIZE,
> +					       DW_PCIE_XILINX_DT_OFF_GAP,
> +					       DW_PCIE_XILINX_DT_SIZE);
> +	}

This PCI_VENDOR_ID_XILINX block looks like maybe it would make sense
inside dw_edma_pcie_get_xilinx_dma_data()?  That function could look
like:

  dw_edma_pcie_get_xilinx_dma_data(...)
  {
    if (pdev->vendor != PCI_VENDOR_ID_XILINX)
      return;

    pdata->devmem_phys_off = DW_PCIE_XILINX_MDB_INVALID_ADDR;
    ...

>  static const struct pci_device_id dw_edma_pcie_id_table[] = {
>  	{ PCI_DEVICE_DATA(SYNOPSYS, EDDA, &snps_edda_data) },
> +	{ PCI_VDEVICE(XILINX, PCI_DEVICE_ID_AMD_MDB_B054),
> +	  (kernel_ulong_t)&amd_mdb_data },