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

[Alan Douglas <adouglas@cadence.com>]

Hi Lorenzo,

On 17 September 2018 16:26 Lorenzo Pieralisi wrote:
> On Tue, Sep 04, 2018 at 09:32:56AM +0000, Alan Douglas wrote:
> > Hi,
> > On 04 September 2018 05:46, Kishon Vijay Abraham I wrote:
> > > On Wednesday 15 August 2018 07:19 PM, Alan Douglas wrote:
> > > > Add set_msix and get_msix functions to driver, and handle
> > > > PCI_EPC_IRQ_MSIX request in raise_irq.  BAR5 is used for
> > > > the MSI-X vectors.
> > > >
> > > > Signed-off-by: Alan Douglas <adouglas@cadence.com>
> > > > ---
> > > >   drivers/pci/controller/pcie-cadence-ep.c | 107 ++++++++++++++++++++++++++++++-
> > > >   drivers/pci/controller/pcie-cadence.h    |   1 +
> > > >   2 files changed, 107 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/pci/controller/pcie-cadence-ep.c b/drivers/pci/controller/pcie-cadence-ep.c
> > > > index 1248d75..259b9a6 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			0x5
> > > >
> > > >   /**
> > > >    * struct cdns_pcie_ep - private data for this PCIe endpoint controller driver
> > > > @@ -255,6 +256,43 @@ 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;
> > > > +
> > > > +	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 */
> > > > +	cdns_pcie_ep_fn_writel(pcie, fn, 0xb4, CDNS_PCIE_EP_MSIX_BAR);
> > >
> > > Please add a macro for MSIX table offset.
> > > I think it relies on endpoint function driver to invoke set_bar for
> > > BAR5? It's possible a function driver can invoke set_msix without set_bar.
> > >
> > I'll add a macro for MSIX table offset.
> > Yes, it relies on the BAR being set up already.  I'll add code to check BAR
> > exists and is large enough, and create it if not.
> >
> > > > +	/* Set PBA BAR and offset.  BAR must match MSIX BAR */
> > > > +	cdns_pcie_ep_fn_writel(pcie, fn, 0xb8, 0x10000 | CDNS_PCIE_EP_MSIX_BAR);
> > >
> > > Here too add a macro for PBA.
> > > How did you get the 0x10000?
> > I'll add a macro for PBA.
> > The 0x10000 (32*2048) is to allow space in the BAR for 2048 MSI-X vectors beneath
> > the PBA. Actually, I could change this to 32*interrupts so that the BAR size can be
> > minimized depending on the number of MSI-X programmed.
> >
> > Thanks for your comments,
> > Alan
> 
> Hi Alan,
> 
> should I expect a v3 for this series to address Kishon's comments ?
> Please let me know, the previous fixes look OK to me.
> 
I'm preparing a v3 which will check that the BAR to be used for MSI-X
has been set up, and will return -EINVAL if not.  I couldn't find a
suitable way to allocate the BAR inside the set_msix function.
It will also address Kishon's other comments.

I expect to send it tomorrow, just back from holiday today.

Regards,
Alan