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@xxxxxxxxxxx>
> > > ---
> > > 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.
Thanks,
Lorenzo
