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

[Verma, Devendra]

[AMD Official Use Only - AMD Internal Distribution Only]

Thanks Bjorn for reviews.

Regards,
Devendra

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> Sent: Tuesday, September 16, 2025 20:34
> To: Verma, Devendra <Devendra.Verma@xxxxxxx>
> Cc: bhelgaas@xxxxxxxxxx; mani@xxxxxxxxxx; vkoul@xxxxxxxxxx;
> dmaengine@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Simek, Michal <michal.simek@xxxxxxx>
> Subject: Re: [PATCH v2 1/2] dmaengine: dw-edma: Add AMD MDB Endpoint
> Support
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> 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.
>

Instead of the above format, a clear assignment to vendor and cap would be good enough.
Reason for this is, if a third vendor comes and supports the same VSEC=0x6 id with similar
capabilities then it looks bulky to put another clause as given above. Instead of this a cleaner
approach would be to have a single pci_find_vsec_capability() and clear assignment to vendor
and cap variables to make it look cleaner. Eg:
switch (pdev->vendor) {
case PCI_VENDOR_ID_XILINX:
        vendor = pdev->vendor;
        cap = DW_PCIE_XILINX_MDB_VSEC_DMA_ID;
case PCI_VENDOR_ID_SYNOPSYS:
        ...
default:
        return;
}
vsec = pci_find_vsec_capability(pdev, vendor, cap);

Please let me know your thoughts on this.

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

Sure, thanks for this clarification.

> > +     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);
>

Sure, this will be addressed.

> Bjorn