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. > https://urldefense.proofpoint.com/v2/url?u=https-3A__marc.info_-3Fl-3Dlinux-2Dpci-26m-3D150905742808166-26w- > 3D2&d=DwIBAg&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=LDGB- > PXmJGU9sCMpKn4c15MjsHicdeOctfSqs7UVh3E&m=M8MJw9hOHC5swoDp9s- > oR5ti6kik8WaCAxXwwD0tOyM&s=IGI3ooXsUFpF6K7jdtqacaos4jbAPNx-SHyJfJ81bsU&e= > > > 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. 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 > >
