[AMD Official Use Only - AMD Internal Distribution Only]
Thank you, Bjorn for reviewing the code!
Please check my response inline.
Regards,
Devendra
> -----Original Message-----
> From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> Sent: Friday, September 5, 2025 22:24
> 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 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 Fri, Sep 05, 2025 at 03:46:58PM +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.
> >
> > Signed-off-by: Devendra K Verma <devendra.verma@xxxxxxx>
> > ---
> > drivers/dma/dw-edma/dw-edma-pcie.c | 83
> +++++++++++++++++++++++++++++++++++++-
> > include/linux/pci_ids.h | 1 +
> > 2 files changed, 82 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/dma/dw-edma/dw-edma-pcie.c
> > b/drivers/dma/dw-edma/dw-edma-pcie.c
> > index 3371e0a7..749067b 100644
> > --- a/drivers/dma/dw-edma/dw-edma-pcie.c
> > +++ b/drivers/dma/dw-edma/dw-edma-pcie.c
> > @@ -18,10 +18,12 @@
> > #include "dw-edma-core.h"
> >
> > #define DW_PCIE_VSEC_DMA_ID 0x6
> > +#define DW_PCIE_AMD_MDB_VSEC_ID 0x20
> > #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)
> > +#define DW_PCIE_AMD_MDB_INVALID_ADDR (~0ULL)
> >
> > #define DW_BLOCK(a, b, c) \
> > { \
> > @@ -50,6 +52,7 @@ struct dw_edma_pcie_data {
> > u8 irqs;
> > u16 wr_ch_cnt;
> > u16 rd_ch_cnt;
> > + u64 phys_addr;
> > };
> >
> > static const struct dw_edma_pcie_data snps_edda_data = { @@ -90,6
> > +93,44 @@ struct dw_edma_pcie_data {
> > .rd_ch_cnt = 2,
> > };
> >
> > +static const struct dw_edma_pcie_data amd_mdb_data = {
> > + /* MDB registers location */
> > + .rg.bar = BAR_0,
> > + .rg.off = 0x00001000, /* 4 Kbytes */
> > + .rg.sz = 0x00002000, /* 8 Kbytes */
> > + /* MDB memory linked list location */
> > + .ll_wr = {
> > + /* Channel 0 - BAR 2, offset 0 Mbytes, size 2 Kbytes */
> > + DW_BLOCK(BAR_2, 0x00000000, 0x00000800)
> > + /* Channel 1 - BAR 2, offset 2 Mbytes, size 2 Kbytes */
> > + DW_BLOCK(BAR_2, 0x00200000, 0x00000800)
> > + },
> > + .ll_rd = {
> > + /* Channel 0 - BAR 2, offset 4 Mbytes, size 2 Kbytes */
> > + DW_BLOCK(BAR_2, 0x00400000, 0x00000800)
> > + /* Channel 1 - BAR 2, offset 6 Mbytes, size 2 Kbytes */
> > + DW_BLOCK(BAR_2, 0x00600000, 0x00000800)
> > + },
> > + /* MDB memory data location */
> > + .dt_wr = {
> > + /* Channel 0 - BAR 2, offset 8 Mbytes, size 2 Kbytes */
> > + DW_BLOCK(BAR_2, 0x00800000, 0x00000800)
> > + /* Channel 1 - BAR 2, offset 9 Mbytes, size 2 Kbytes */
> > + DW_BLOCK(BAR_2, 0x00900000, 0x00000800)
> > + },
> > + .dt_rd = {
> > + /* Channel 0 - BAR 2, offset 10 Mbytes, size 2 Kbytes */
> > + DW_BLOCK(BAR_2, 0x00a00000, 0x00000800)
> > + /* Channel 1 - BAR 2, offset 11 Mbytes, size 2 Kbytes */
> > + DW_BLOCK(BAR_2, 0x00b00000, 0x00000800)
> > + },
> > + /* Other */
> > + .mf = EDMA_MF_HDMA_NATIVE,
> > + .irqs = 1,
> > + .wr_ch_cnt = 2,
> > + .rd_ch_cnt = 2,
> > +};
> > +
> > static int dw_edma_pcie_irq_vector(struct device *dev, unsigned int
> > nr) {
> > return pci_irq_vector(to_pci_dev(dev), nr); @@ -120,9 +161,14 @@
> > static void dw_edma_pcie_get_vsec_dma_data(struct pci_dev *pdev,
> > u32 val, map;
> > u16 vsec;
> > u64 off;
> > + u16 vendor;
> >
> > - vsec = pci_find_vsec_capability(pdev, PCI_VENDOR_ID_SYNOPSYS,
> > - DW_PCIE_VSEC_DMA_ID);
> > + vendor = pdev->vendor;
> > + if (vendor != PCI_VENDOR_ID_SYNOPSYS &&
> > + vendor != PCI_VENDOR_ID_XILINX)
> > + return;
> > +
> > + vsec = pci_find_vsec_capability(pdev, vendor,
> > + DW_PCIE_VSEC_DMA_ID);
>
> The vendor of a device assigns VSEC IDs and determines what each ID means, so
> the semantics of a VSEC Capability are determined by the tuple of (device Vendor
> ID, VSEC ID), where the Vendor ID is the value at 0x00 in config space.
>
As AMD is a vendor for this device, it is determined as VSEC capability to support
some of the functionality not supported by the other vendor Synopsys.
> This code assumes that Synopsys and Xilinx agreed on the same VSEC ID
> (6) and semantics of that Capability.
>
> I assume you know this is true in this particular case, but it is not safe for in general
> because even if other vendors incorporate this same IP into their devices, they may
> choose different VSEC IDs because they may have already assigned the VSEC ID 6
> for something else.
>
> So you should add a comment to the effect that "Synopsys and Xilinx happened to
> use the same VSEC ID and semantics. This may vary for other vendors."
>
Agree with the above suggestion. Will add this in next review.
> The DVSEC Capability is a more generic solution to this problem. The VSEC ID
> namespace is determined by the Vendor ID of the *device*.
>
> By contrast, the DVSEC ID namespace is determined by a Vendor ID in the DVSEC
> Capability itself, not by the Vendor ID of the device.
>
> So AMD could define a DVSEC ID, e.g., 6, and define the semantics of that DVSEC.
> Then devices from *any* vendor could include a DVSEC Capability with
> (PCI_VENDOR_ID_AMD, 6), and generic code could look for that DVSEC
> independent of what is at 0x00 in config space.
>
As AMD itself becomes the vendor for this device, VSEC capability is chosen to support
the functionality missing in the code.
> > if (!vsec)
> > return;
> >
> > @@ -155,6 +201,27 @@ static void dw_edma_pcie_get_vsec_dma_data(struct
> pci_dev *pdev,
> > off <<= 32;
> > off |= val;
> > pdata->rg.off = off;
> > +
> > + /* AMD specific VSEC capability */
> > + if (vendor != PCI_VENDOR_ID_XILINX)
> > + return;
> > +
> > + vsec = pci_find_vsec_capability(pdev, vendor,
> > + DW_PCIE_AMD_MDB_VSEC_ID);
>
> pci_find_vsec_capability() finds a Vendor-Specific Extended Capability defined by
> the vendor of the device (Xilinx in this case).
>
> pci_find_vsec_capability() already checks that dev->vendor matches the vendor
> argument so you don't need the "vendor != PCI_VENDOR_ID_XILINX"
> check above.
>
> DW_PCIE_AMD_MDB_VSEC_ID should include "XILINX" because this ID is in the
> namespace created by PCI_VENDOR_ID_XILINX, i.e., it's somewhere in the
> (PCI_VENDOR_ID_XILINX, x) space.
>
> This code should look something like this (you could add "MDB" or something if it
> makes sense):
>
> #define PCI_VSEC_ID_XILINX_DMA_DATA 0x20
>
Thank you for the suggestion, will go with the DW_PCIE_XILINX_MDB_VSEC_ID
similar to the format already been used in the code.
> vsec = pci_find_vsec_capability(pdev, PCI_VENDOR_ID_XILINX,
> PCI_VSEC_ID_XILINX_DMA_DATA);
>
> > + if (!vsec)
> > + return;
> > +
> > + pci_read_config_dword(pdev, vsec + PCI_VNDR_HEADER, &val);
> > + if (PCI_VNDR_HEADER_ID(val) != 0x20 ||
> > + PCI_VNDR_HEADER_REV(val) != 0x1)
> > + return;
> > +
> > + pci_read_config_dword(pdev, vsec + 0xc, &val);
> > + off = val;
> > + pci_read_config_dword(pdev, vsec + 0x8, &val);
> > + off <<= 32;
> > + off |= val;
> > + pdata->phys_addr = off;
> > }
> >
> > static int dw_edma_pcie_probe(struct pci_dev *pdev, @@ -179,6 +246,7
> > @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
> > }
> >
> > memcpy(vsec_data, pdata, sizeof(struct dw_edma_pcie_data));
> > + vsec_data->phys_addr = DW_PCIE_AMD_MDB_INVALID_ADDR;
> >
> > /*
> > * Tries to find if exists a PCIe Vendor-Specific Extended
> > Capability @@ -186,6 +254,15 @@ static int dw_edma_pcie_probe(struct pci_dev
> *pdev,
> > */
> > dw_edma_pcie_get_vsec_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->phys_addr == DW_PCIE_AMD_MDB_INVALID_ADDR)
> > + return -EINVAL;
> > + }
> > +
> > /* Mapping PCI BAR regions */
> > mask = BIT(vsec_data->rg.bar);
> > for (i = 0; i < vsec_data->wr_ch_cnt; i++) { @@ -367,6 +444,8 @@
> > static void dw_edma_pcie_remove(struct pci_dev *pdev)
> >
> > 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 },
> > { }
> > };
> > MODULE_DEVICE_TABLE(pci, dw_edma_pcie_id_table); diff --git
> > a/include/linux/pci_ids.h b/include/linux/pci_ids.h index
> > 92ffc43..c15607d 100644
> > --- a/include/linux/pci_ids.h
> > +++ b/include/linux/pci_ids.h
> > @@ -636,6 +636,7 @@
> > #define PCI_DEVICE_ID_AMD_HUDSON2_SMBUS 0x780b
> > #define PCI_DEVICE_ID_AMD_HUDSON2_IDE 0x780c
> > #define PCI_DEVICE_ID_AMD_KERNCZ_SMBUS 0x790b
> > +#define PCI_DEVICE_ID_AMD_MDB_B054 0xb054
>
> Unless PCI_DEVICE_ID_AMD_MDB_B054 is used in more than one driver, move
> the #define into the file that uses it. See the note at the top of pci_ids.h.
Agreed. Will move this to the file where it is being used as no other driver is using it now.
