RE: [PATCH v3 5/5] PCI: cadence: Add MSI-X capability to EP driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
> > >




[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux