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

[Kishon Vijay Abraham I]

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.
> 
>> +	} 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