Re: [PATCH v7 5/7] PCI: cadence: Add support to configure virtual functions

[Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>]

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@ti.com>
> >> Acked-by: Tom Joseph <tjoseph@cadence.com>
> >> ---
> >>  .../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