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

[Manivannan Sadhasivam]

On Wed, Dec 10, 2025 at 10:32:28AM -0600, Bjorn Helgaas wrote:
> 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@xxxxxxxxxx>
> > > > 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.
> 

Yes, it will make it clear and obvious!

- Mani

-- 
மணிவண்ணன் சதாசிவம்