Hi, On 20 September 2018 17:12, Alan Douglas wrote: > To: 'Lorenzo Pieralisi' <lorenzo.pieralisi@xxxxxxx> > Cc: kishon@xxxxxx; bhelgaas@xxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; gustavo.pimentel@xxxxxxxxxxxx; cyrille.pitchen@xxxxxxxxxxx > Subject: RE: [PATCH v3 5/5] PCI: cadence: Add MSI-X capability to EP driver > > Hi, > On 20 September 2018 11:15, Lorenzo Pieralisi wrote: > > On Tue, Sep 18, 2018 at 04:21:45PM +0100, Alan Douglas wrote: > > > Add set_msix and get_msix functions to driver, and handle > > > PCI_EPC_IRQ_MSIX request in raise_irq. BAR4 is used for > > > the MSI-X vectors. > > > > https://marc.info/?l=linux-pci&m=150905742808166&w=2 > > > I will add more explanation in v4, and check for other issues I missed. > > > > > > Signed-off-by: Alan Douglas <adouglas@xxxxxxxxxxx> > > > --- > > > drivers/pci/controller/pcie-cadence-ep.c | 129 ++++++++++++++++++++++++++++++- > > > drivers/pci/controller/pcie-cadence.h | 7 ++ > > > 2 files changed, 135 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/pci/controller/pcie-cadence-ep.c b/drivers/pci/controller/pcie-cadence-ep.c > > > index 1248d75..dbe76ab 100644 > > > --- a/drivers/pci/controller/pcie-cadence-ep.c > > > +++ b/drivers/pci/controller/pcie-cadence-ep.c > > > @@ -16,6 +16,7 @@ > > > #define CDNS_PCIE_EP_MIN_APERTURE 128 /* 128 bytes */ > > > #define CDNS_PCIE_EP_IRQ_PCI_ADDR_NONE 0x1 > > > #define CDNS_PCIE_EP_IRQ_PCI_ADDR_LEGACY 0x3 > > > +#define CDNS_PCIE_EP_MSIX_BAR 0x4 > > > > > > /** > > > * struct cdns_pcie_ep - private data for this PCIe endpoint controller driver > > > @@ -255,6 +256,65 @@ static int cdns_pcie_ep_get_msi(struct pci_epc *epc, u8 fn) > > > return mme; > > > } > > > > > > +static int cdns_pcie_ep_get_msix(struct pci_epc *epc, u8 func_no) > > > +{ > > > + struct cdns_pcie_ep *ep = epc_get_drvdata(epc); > > > + struct cdns_pcie *pcie = &ep->pcie; > > > + u32 cap = CDNS_PCIE_EP_FUNC_MSIX_CAP_OFFSET; > > > + u32 val, reg; > > > + > > > + reg = cap + PCI_MSIX_FLAGS; > > > + val = cdns_pcie_ep_fn_readw(pcie, func_no, reg); > > > + if (!(val & PCI_MSIX_FLAGS_ENABLE)) > > > + return -EINVAL; > > > + > > > + val &= PCI_MSIX_FLAGS_QSIZE; > > > + > > > + return val; > > > +} > > > + > > > +static int cdns_pcie_ep_set_msix(struct pci_epc *epc, u8 fn, u16 interrupts) > > > +{ > > > + struct cdns_pcie_ep *ep = epc_get_drvdata(epc); > > > + struct cdns_pcie *pcie = &ep->pcie; > > > + u32 cap = CDNS_PCIE_EP_FUNC_MSIX_CAP_OFFSET; > > > + u32 val, reg, tblsize, b, cfg, ap, ctrl; > > > + > > > + /* Check that the BAR has already been configured, and is large > > > + * enough, and fail if not. > > > + */ > > > + b = CDNS_PCIE_EP_MSIX_BAR; > > > + if (b < BAR_4) > > > + reg = CDNS_PCIE_LM_EP_FUNC_BAR_CFG0(fn); > > > + else > > > + reg = CDNS_PCIE_LM_EP_FUNC_BAR_CFG1(fn); > > > + cfg = cdns_pcie_readl(pcie, reg); > > > + ctrl = CDNS_PCIE_LM_EP_FUNC_BAR_CFG_VAL(b, cfg); > > > + if (!(ctrl & CDNS_PCIE_LM_BAR_CFG_CTRL_MEM_32BITS)) > > > + return -EINVAL; > > > + ap = CDNS_PCIE_LM_EP_FUNC_BAR_APERTURE_VAL(b, cfg); > > > + tblsize = fls64(interrupts * 32 - 1); > > > + /* Need (1<<tblsize)*2 bytes for vector table + PBA table */ > > > + if (ap + 6 < tblsize) > > > > This is cryptic, you have to explain what it does (inclusive of that > > hardcoded 6 value). > > > I'll expand that, and replace 6 with (CDNS_PCIE_EP_MIN_APERTURE - 1) > I mean that the BAR has to be at least twice the size of the vector table, > to allow for PBA as well. BAR size is 2^(aperture + 7) > > > > + return -EINVAL; > > > + > > > + reg = cap + PCI_MSIX_FLAGS; > > > + val = cdns_pcie_ep_fn_readw(pcie, fn, reg); > > > + val &= ~PCI_MSIX_FLAGS_QSIZE; > > > + val |= interrupts; > > > + cdns_pcie_ep_fn_writew(pcie, fn, reg, val); > > > + > > > + /* Set MSIX BAR and offset */ > > > + reg = cap + PCI_MSIX_TABLE; > > > + cdns_pcie_ep_fn_writel(pcie, fn, reg, b); > > > + > > > + /* Set PBA BAR and offset. BAR must match MSIX BAR */ > > > + reg = cap + PCI_MSIX_PBA; > > > + cdns_pcie_ep_fn_writel(pcie, fn, reg, (1UL << tblsize) | b); > > > + > > > + return 0; > > > +} > > > + > > > static void cdns_pcie_ep_assert_intx(struct cdns_pcie_ep *ep, u8 fn, > > > u8 intx, bool is_asserted) > > > { > > > @@ -366,8 +426,69 @@ static int cdns_pcie_ep_send_msi_irq(struct cdns_pcie_ep *ep, u8 fn, > > > return 0; > > > } > > > > > > +static int cdns_pcie_ep_send_msix_irq(struct cdns_pcie_ep *ep, u8 fn, > > > + u16 interrupt_num) > > > +{ > > > + struct cdns_pcie *pcie = &ep->pcie; > > > + u32 cap = CDNS_PCIE_EP_FUNC_MSIX_CAP_OFFSET; > > > + u16 flags; > > > + u64 pci_addr_mask = 0xff; > > > + u16 tbl_offset = 0; > > > + u32 bar_addr_upper, bar_addr_lower; > > > + u32 msg_addr_upper, msg_addr_lower; > > > + u32 msg_data; > > > + u64 tbl_addr, msg_addr; > > > + void __iomem *msix_tbl; > > > + > > > + /* Check whether the MSI-X feature has been enabled by the PCI host. */ > > > + flags = cdns_pcie_ep_fn_readw(pcie, fn, cap + PCI_MSIX_FLAGS); > > > + if (!(flags & PCI_MSIX_FLAGS_ENABLE)) > > > + return -EINVAL; > > > + /* We want local address, not address on host. Table is at offset 0 */ > > > + bar_addr_lower = cdns_pcie_readl(pcie, > > > + CDNS_PCIE_AT_IB_EP_FUNC_BAR_ADDR0(fn, CDNS_PCIE_EP_MSIX_BAR)); > > > + bar_addr_upper = cdns_pcie_readl(pcie, > > > + CDNS_PCIE_AT_IB_EP_FUNC_BAR_ADDR1(fn, CDNS_PCIE_EP_MSIX_BAR)); > > > + > > > + tbl_addr = ((u64)bar_addr_upper) << 32 | bar_addr_lower; > > > + tbl_addr += (tbl_offset + ((interrupt_num - 1) * PCI_MSIX_ENTRY_SIZE)); > > > + tbl_addr &= PCI_BASE_ADDRESS_MEM_MASK; > > > + msix_tbl = phys_to_virt(tbl_addr); > > > > phys_to_virt() coupled with iounmap() below, this does not look OK > > to me. > I missed that, I propose to change this to ioremap_nocache() / iounmap() > It was working for me in testing because the BAR is already mapped and iounmap() > didn't have any effect. > > > > IIUC you want to map the BAR in the EP system address space to read the > > tables entries, where the BAR content is set by the host system but > > phys_to_virt() is not the way to do it. > > > > Please explain to me if my reading of the code is correct. > Yes, I want to map the BAR in EP system address space. > The physical address (in EP space) for the BAR is in the inbound address > translation registers which are read into bar_addr_lower & bar_addr_upper. > I want to map this into EP system address space, so will use ioremap_nocache() > instead unless you propose otherwise. Actually, I found some problems to use ioremap_nocache(), I run into a WARN_ON in ioremap_caller() because the BAR memory is in RAM. It is allocated using dma_alloc_coherent() in pci_epf_alloc_space() So, my problem is to get the virtual address of the memory from the physical address. I don't have the virtual address that was returned by dma_alloc_coherent(), I can't get this from the endpoint driver core, but I don't see a way to convert the physical address to DMA address. phys_to_virt() seems to work in my setup, but I understand that this is not suitable from your comments, and from reading the DMA documentation. dma_map_resource() looks close to what I want, but I see it can't be used for RAM either. I'm now searching for another solution to this, but looks like I may need to find a way to get the original virtual address returned by dma_alloc_coherent(). If you have any other suggestions they will be welcome! > > Thanks for your review, > Alan > > > > Lorenzo > > > > > + if (!msix_tbl) > > > + return -EINVAL; > > > + > > > + msg_addr_lower = readl(msix_tbl + PCI_MSIX_ENTRY_LOWER_ADDR); > > > + msg_addr_upper = readl(msix_tbl + PCI_MSIX_ENTRY_UPPER_ADDR); > > > + msg_addr = ((u64)msg_addr_upper) << 32 | msg_addr_lower; > > > + > > > + msg_data = readl(msix_tbl + PCI_MSIX_ENTRY_VECTOR_CTRL); > > > + if (msg_data & 0x1) > > > + return -EINVAL; > > > + > > > + msg_data = readl(msix_tbl + PCI_MSIX_ENTRY_DATA); > > > + > > > + iounmap(msix_tbl); > > > + > > > + /* Set the outbound region if needed. */ > > > + if (unlikely(ep->irq_pci_addr != (msg_addr & ~pci_addr_mask) || > > > + ep->irq_pci_fn != fn)) { > > > + /* First region was reserved for IRQ writes. */ > > > + cdns_pcie_set_outbound_region(pcie, fn, 0, > > > + false, > > > + ep->irq_phys_addr, > > > + msg_addr & ~pci_addr_mask, > > > + pci_addr_mask + 1); > > > + ep->irq_pci_addr = (msg_addr & ~pci_addr_mask); > > > + ep->irq_pci_fn = fn; > > > + } > > > + writel(msg_data, ep->irq_cpu_addr + (msg_addr & pci_addr_mask)); > > > + > > > + return 0; > > > +} > > > + > > > static int cdns_pcie_ep_raise_irq(struct pci_epc *epc, u8 fn, > > > - enum pci_epc_irq_type type, u8 interrupt_num) > > > + enum pci_epc_irq_type type, > > > + u16 interrupt_num) > > > { > > > struct cdns_pcie_ep *ep = epc_get_drvdata(epc); > > > u32 link_status; > > > @@ -384,6 +505,9 @@ static int cdns_pcie_ep_raise_irq(struct pci_epc *epc, u8 fn, > > > case PCI_EPC_IRQ_MSI: > > > return cdns_pcie_ep_send_msi_irq(ep, fn, interrupt_num); > > > > > > + case PCI_EPC_IRQ_MSIX: > > > + return cdns_pcie_ep_send_msix_irq(ep, fn, interrupt_num); > > > + > > > default: > > > break; > > > } > > > @@ -430,6 +554,8 @@ static int cdns_pcie_ep_start(struct pci_epc *epc) > > > .unmap_addr = cdns_pcie_ep_unmap_addr, > > > .set_msi = cdns_pcie_ep_set_msi, > > > .get_msi = cdns_pcie_ep_get_msi, > > > + .set_msix = cdns_pcie_ep_set_msix, > > > + .get_msix = cdns_pcie_ep_get_msix, > > > .raise_irq = cdns_pcie_ep_raise_irq, > > > .start = cdns_pcie_ep_start, > > > }; > > > @@ -501,6 +627,7 @@ static int cdns_pcie_ep_probe(struct platform_device *pdev) > > > } > > > > > > epc_set_drvdata(epc, ep); > > > + epc->features |= EPC_FEATURE_MSIX_AVAILABLE; > > > > > > if (of_property_read_u8(np, "max-functions", &epc->max_functions) < 0) > > > epc->max_functions = 1; > > > diff --git a/drivers/pci/controller/pcie-cadence.h b/drivers/pci/controller/pcie-cadence.h > > > index 4bb2733..946f6ee 100644 > > > --- a/drivers/pci/controller/pcie-cadence.h > > > +++ b/drivers/pci/controller/pcie-cadence.h > > > @@ -52,6 +52,12 @@ > > > (GENMASK(7, 5) << ((b) * 8)) > > > #define CDNS_PCIE_LM_EP_FUNC_BAR_CFG_BAR_CTRL(b, c) \ > > > (((c) << ((b) * 8 + 5)) & CDNS_PCIE_LM_EP_FUNC_BAR_CFG_BAR_CTRL_MASK(b)) > > > +#define CDNS_PCIE_LM_EP_FUNC_BAR_CFG_VAL(b, c) \ > > > + (((c) & CDNS_PCIE_LM_EP_FUNC_BAR_CFG_BAR_CTRL_MASK((b) % 4)) \ > > > + >> (((b) % 4) * 8 + 5)) > > > +#define CDNS_PCIE_LM_EP_FUNC_BAR_APERTURE_VAL(b, c) \ > > > + (((c) & CDNS_PCIE_LM_EP_FUNC_BAR_CFG_BAR_APERTURE_MASK((b) % 4)) \ > > > + >> (((b) % 4) * 8)) > > > > > > /* Endpoint Function Configuration Register */ > > > #define CDNS_PCIE_LM_EP_FUNC_CFG (CDNS_PCIE_LM_BASE + 0x02c0) > > > @@ -93,6 +99,7 @@ > > > #define CDNS_PCIE_EP_FUNC_BASE(fn) (((fn) << 12) & GENMASK(19, 12)) > > > > > > #define CDNS_PCIE_EP_FUNC_MSI_CAP_OFFSET 0x90 > > > +#define CDNS_PCIE_EP_FUNC_MSIX_CAP_OFFSET 0xb0 > > > > > > /* > > > * Root Port Registers (PCI configuration space for the root port function) > > > -- > > > 1.9.0 > > >
