On Tue, Aug 03, 2021 at 08:26:42PM +0530, Kishon Vijay Abraham I wrote:
> Hi Lorenzo,
>
> On 03/08/21 5:15 pm, Lorenzo Pieralisi wrote:
> > On Tue, Aug 03, 2021 at 10:33:08AM +0530, Kishon Vijay Abraham I wrote:
> >> Now that support for SR-IOV is added in PCIe endpoint core, add support
> >> to configure virtual functions in the Cadence PCIe EP driver.
> >>
> >> Signed-off-by: Kishon Vijay Abraham I <kishon@xxxxxx>
> >> Acked-by: Tom Joseph <tjoseph@xxxxxxxxxxx>
> >> ---
> >> .../pci/controller/cadence/pcie-cadence-ep.c | 241 +++++++++++++++---
> >> drivers/pci/controller/cadence/pcie-cadence.h | 7 +
> >> 2 files changed, 217 insertions(+), 31 deletions(-)
> >>
> >> diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> >> index 912a15be8bfd..791915054ff4 100644
> >> --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
> >> +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> >> @@ -20,7 +20,18 @@ static int cdns_pcie_ep_write_header(struct pci_epc *epc, u8 fn, u8 vfn,
> >> struct pci_epf_header *hdr)
> >> {
> >> struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
> >> + u32 cap = CDNS_PCIE_EP_FUNC_SRIOV_CAP_OFFSET;
> >> struct cdns_pcie *pcie = &ep->pcie;
> >> + u32 reg;
> >> +
> >> + if (vfn > 1) {
> >> + dev_dbg(&epc->dev, "Only Virtual Function #1 has deviceID\n");
> >> + return 0;
> >
> > Shouldn't this return an error ?
>
> Since the same function driver could be used for physical function and
> virtual function, I tried to avoid adding any additional case specific
> for virtual function in the function driver.
>
> If we want to return an error here, then the function driver should be
> modified to not invoke writeheader for vfn > 1.
Well, I see it the other way around. If writing the header for vfn > 1
is an error it must be reported as such and handled accordingly.
As it stands - it looks like we do nothing and everything is just
fine, which is weird.
Thanks,
Lorenzo
> >> + } else if (vfn == 1) {
> >> + reg = cap + PCI_SRIOV_VF_DID;
> >> + cdns_pcie_ep_fn_writew(pcie, fn, reg, hdr->deviceid);
> >> + return 0;
> >> + }
> >>
> >> cdns_pcie_ep_fn_writew(pcie, fn, PCI_DEVICE_ID, hdr->deviceid);
> >> cdns_pcie_ep_fn_writeb(pcie, fn, PCI_REVISION_ID, hdr->revid);
> >> @@ -51,12 +62,14 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn, u8 vfn,
> >> struct pci_epf_bar *epf_bar)
> >> {
> >> struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
> >> + u32 cap = CDNS_PCIE_EP_FUNC_SRIOV_CAP_OFFSET;
> >> struct cdns_pcie_epf *epf = &ep->epf[fn];
> >> struct cdns_pcie *pcie = &ep->pcie;
> >> dma_addr_t bar_phys = epf_bar->phys_addr;
> >> enum pci_barno bar = epf_bar->barno;
> >> int flags = epf_bar->flags;
> >> u32 addr0, addr1, reg, cfg, b, aperture, ctrl;
> >> + u32 first_vf_offset, stride;
> >> u64 sz;
> >>
> >> /* BAR size is 2^(aperture + 7) */
> >> @@ -92,26 +105,50 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn, u8 vfn,
> >>
> >> addr0 = lower_32_bits(bar_phys);
> >> addr1 = upper_32_bits(bar_phys);
> >> +
> >> + if (vfn == 1) {
> >> + /* All virtual functions use the same BAR config */
> >> + if (bar < BAR_4) {
> >> + reg = CDNS_PCIE_LM_EP_VFUNC_BAR_CFG0(fn);
> >> + b = bar;
> >> + } else {
> >> + reg = CDNS_PCIE_LM_EP_VFUNC_BAR_CFG1(fn);
> >> + b = bar - BAR_4;
> >> + }
> >> + } else if (vfn == 0) {
> >> + /* BAR configuration for physical function */
> >> + if (bar < BAR_4) {
> >> + reg = CDNS_PCIE_LM_EP_FUNC_BAR_CFG0(fn);
> >> + b = bar;
> >> + } else {
> >> + reg = CDNS_PCIE_LM_EP_FUNC_BAR_CFG1(fn);
> >> + b = bar - BAR_4;
> >> + }
> >> + }
> >
> > Code in both branches is almost identical except for what is
> > assigned to reg, it is not fundamental but maybe it can be rewritten
> > more concisely.
>
> okay.. let me think.
>
> Thanks
> Kishon
