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

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Dec 10, 2025 at 10:26:38PM +0900, Manivannan Sadhasivam wrote:
> On Wed, Dec 10, 2025 at 11:40:04AM +0000, Verma, Devendra wrote:
> > > -----Original Message-----
> > > From: Manivannan Sadhasivam <mani@kernel.org>
> > > On Fri, Nov 21, 2025 at 05:04:54PM +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

> > > > +/* AMD MDB (Xilinx) specific defines */
> > > > +#define DW_PCIE_XILINX_MDB_VSEC_DMA_ID               0x6

> > > > static void dw_edma_pcie_get_vsec_dma_data(struct pci_dev *pdev,

> > > > +      * Synopsys and AMD (Xilinx) use the same VSEC ID for the
> > > > + purpose
> > >
> > > Same or different?
> > 
> > It is the same capability for which Synosys and AMD (Xilinx) share
> > the common value.
> 
> This is confusing. You are searching for either DW_PCIE_VSEC_DMA_ID
> or DW_PCIE_XILINX_MDB_VSEC_DMA_ID below, which means that the VSEC
> IDs are different.

This is perennially confusing.

Since this is a "Vendor-Specific" (not a "Designated Vendor-Specific")
capability, we must search for the per-vendor VSEC ID, i.e.,
DW_PCIE_VSEC_DMA_ID for PCI_VENDOR_ID_SYNOPSYS devices or
DW_PCIE_XILINX_MDB_VSEC_DMA_ID for PCI_VENDOR_ID_XILINX devices.

The fact that DW_PCIE_VSEC_DMA_ID == DW_PCIE_XILINX_MDB_VSEC_DMA_ID is
a coincidence and is not relevant to the code.  The comment that
"Synopsys and AMD (Xilinx) use the same VSEC ID for the purpose"
should be removed because it adds confusion and the code doesn't rely
on that.

However, the subsequent code DOES rely on the fact that the Synopsys
and the Xilinx VSECs have the same *registers* at the same offsets:

        pci_read_config_dword(pdev, vsec + 0x8, &val);
        map = FIELD_GET(DW_PCIE_VSEC_DMA_MAP, val);
        pdata->rg.bar = FIELD_GET(DW_PCIE_VSEC_DMA_BAR, val);

        pci_read_config_dword(pdev, vsec + 0xc, &val);
        pdata->wr_ch_cnt = min_t(u16, pdata->wr_ch_cnt,
                                 FIELD_GET(DW_PCIE_VSEC_DMA_WR_CH, val));
        pdata->rd_ch_cnt = min_t(u16, pdata->rd_ch_cnt,
                                 FIELD_GET(DW_PCIE_VSEC_DMA_RD_CH, val));

        pci_read_config_dword(pdev, vsec + 0x14, &val);
        off = val;

        pci_read_config_dword(pdev, vsec + 0x10, &val);

The VSEC ID *values* are not relevant, but the fact that the registers
in the Synopsys and the Xilinx capabilities are identical *is*
important and not obvious, so if you share the code that reads those
registers, there should be a comment about that.

The normal way to use VSEC is to look for a capability for a single
vendor and interpret it using code for that specific vendor.  I think
I would split this into separate dw_edma_pcie_get_synopsys_dma_data()
and dw_edma_pcie_get_xilinx_dma_data() functions to make it obvious
that these are indeed controlled by different vendors, e.g.,

  dw_edma_pcie_get_synopsys_dma_data()
  {
    vsec = pci_find_vsec_capability(pdev, PCI_VENDOR_ID_SYNOPSYS,
                                    DW_PCIE_VSEC_DMA_ID);
    if (!vsec)
      return;

    pci_read_config_dword(pdev, vsec + 0x8, &val);
    ...
  }

  dw_edma_pcie_get_xilinx_dma_data()
  {
    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 + 0x8, &val);
    ...

    vsec = pci_find_vsec_capability(pdev, PCI_VENDOR_ID_XILINX,
                                    DW_PCIE_XILINX_MDB_VSEC_ID);
    if (!vsec)
      return;
    ...
  }

It's safe to call both of them for all devices like this:

  dw_edma_pcie_probe
  {
    ...
    dw_edma_pcie_get_synopsys_dma_data(pdev, vsec_data);
    dw_edma_pcie_get_xilinx_dma_data(pdev, vsec_data);
    ...
  }

Most of the code in dw_edma_pcie_get_synopsys_dma_data() would be
duplicated, but that's OK and acknowledges the fact that Synopsys and
Xilinx can evolve that VSEC independently.

Bjorn