Re: [PATCH v2 06/10] PCI: layerscape: Modify the way of getting capability with different PEX

[Andrew Murray]

On Fri, Aug 23, 2019 at 04:13:30AM +0000, Xiaowei Bao wrote:
> 
> 
> > -----Original Message-----
> > From: Kishon Vijay Abraham I <kishon@xxxxxx>
> > Sent: 2019å8æ23æ 11:40
> > To: Xiaowei Bao <xiaowei.bao@xxxxxxx>; bhelgaas@xxxxxxxxxx;
> > robh+dt@xxxxxxxxxx; mark.rutland@xxxxxxx; shawnguo@xxxxxxxxxx; Leo Li
> > <leoyang.li@xxxxxxx>; lorenzo.pieralisi@xxxxxx
> > <lorenzo.pieralisi@xxxxxxx>; arnd@xxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx;
> > M.h. Lian <minghuan.lian@xxxxxxx>; Mingkai Hu <mingkai.hu@xxxxxxx>;
> > Roy Zang <roy.zang@xxxxxxx>; jingoohan1@xxxxxxxxx;
> > gustavo.pimentel@xxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx;
> > devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linuxppc-dev@xxxxxxxxxxxxxxxx;
> > andrew.murray@xxxxxxx
> > Subject: Re: [PATCH v2 06/10] PCI: layerscape: Modify the way of getting
> > capability with different PEX
> > 
> > Hi,
> > 
> > (Fixed Lorenzo's email address. All the patches in the series have wrong email
> > id)
> > 
> > On 23/08/19 8:09 AM, Xiaowei Bao wrote:
> > >
> > >
> > >> -----Original Message-----
> > >> From: Kishon Vijay Abraham I <kishon@xxxxxx>
> > >> Sent: 2019å8æ22æ 19:44
> > >> To: Xiaowei Bao <xiaowei.bao@xxxxxxx>; bhelgaas@xxxxxxxxxx;
> > >> robh+dt@xxxxxxxxxx; mark.rutland@xxxxxxx; shawnguo@xxxxxxxxxx; Leo
> > Li
> > >> <leoyang.li@xxxxxxx>; lorenzo.pieralisi@xxxxxx; arnd@xxxxxxxx;
> > >> gregkh@xxxxxxxxxxxxxxxxxxx; M.h. Lian <minghuan.lian@xxxxxxx>;
> > >> Mingkai Hu <mingkai.hu@xxxxxxx>; Roy Zang <roy.zang@xxxxxxx>;
> > >> jingoohan1@xxxxxxxxx; gustavo.pimentel@xxxxxxxxxxxx;
> > >> linux-pci@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> > >> linux-kernel@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> > >> linuxppc-dev@xxxxxxxxxxxxxxxx; andrew.murray@xxxxxxx
> > >> Subject: Re: [PATCH v2 06/10] PCI: layerscape: Modify the way of
> > >> getting capability with different PEX
> > >>
> > >> Hi,
> > >>
> > >> On 22/08/19 4:52 PM, Xiaowei Bao wrote:
> > >>> The different PCIe controller in one board may be have different
> > >>> capability of MSI or MSIX, so change the way of getting the MSI
> > >>> capability, make it more flexible.
> > >>
> > >> please use different pci_epc_features table for different boards.
> > > Thanks, I think that it will be more flexible to dynamically get MSI
> > > or MSIX capability, Thus, we will not need to define the pci_epc_feature for
> > different boards.
> > 
> > Is the restriction because you cannot have different compatible for different
> > boards?
> Sorry, I am not very clear what your mean, I think even if I use the same compatible
> with different boards, each boards will enter the probe function, in there I will get
> the MSI or MSIX PCIe capability of the current controller in this board. Why do I need
> to define the pci_epc_feature for different boards? 

At present you determine how to set the [msi,msix]_capable flags of
pci_epc_features based on reading the function capabilities at probe
time. Instead of doing this, is it possible that you can determine the flags
based on the compatible type alone? For example, is the MSI/MSIX capability
the same for all fsl,ls2088a-pcie-ep devices?

If it isn't *necessary* to probe for this information at probe time, then
you could instead create a static pci_epc_features structure and assign
it to something in your drvdata. This may provide some benefits.

The dw_pcie_ep_get_features function would then look like:

static const struct pci_epc_features*
ls_pcie_ep_get_features(struct dw_pcie_ep *ep)
{
	struct dw_pcie *pci = to_dw_pcie_from_pp(ep);
	struct ls_pcie_ep *pcie = dev_get_drvdata(pci->dev);
	return pcie->epc_features;
}

This also means you can revert "[v3,03/11] PCI: designware-ep: Move the".

Is this what you had in mind Kishon?

Thanks,

Andrew Murray

> > 
> > Thanks
> > Kishon
> > 
> > >>
> > >> Thanks
> > >> Kishon
> > >>>
> > >>> Signed-off-by: Xiaowei Bao <xiaowei.bao@xxxxxxx>
> > >>> ---
> > >>> v2:
> > >>>  - Remove the repeated assignment code.
> > >>>
> > >>>  drivers/pci/controller/dwc/pci-layerscape-ep.c | 26
> > >>> +++++++++++++++++++-------
> > >>>  1 file changed, 19 insertions(+), 7 deletions(-)
> > >>>
> > >>> diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > >>> b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > >>> index 4e92a95..8461f62 100644
> > >>> --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > >>> +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > >>> @@ -22,6 +22,7 @@
> > >>>
> > >>>  struct ls_pcie_ep {
> > >>>  	struct dw_pcie		*pci;
> > >>> +	struct pci_epc_features	*ls_epc;
> > >>>  };
> > >>>
> > >>>  #define to_ls_pcie_ep(x)	dev_get_drvdata((x)->dev)
> > >>> @@ -40,25 +41,26 @@ static const struct of_device_id
> > >> ls_pcie_ep_of_match[] = {
> > >>>  	{ },
> > >>>  };
> > >>>
> > >>> -static const struct pci_epc_features ls_pcie_epc_features = {
> > >>> -	.linkup_notifier = false,
> > >>> -	.msi_capable = true,
> > >>> -	.msix_capable = false,
> > >>> -};
> > >>> -
> > >>>  static const struct pci_epc_features*
> > >>> ls_pcie_ep_get_features(struct dw_pcie_ep *ep)  {
> > >>> -	return &ls_pcie_epc_features;
> > >>> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > >>> +	struct ls_pcie_ep *pcie = to_ls_pcie_ep(pci);
> > >>> +
> > >>> +	return pcie->ls_epc;
> > >>>  }
> > >>>
> > >>>  static void ls_pcie_ep_init(struct dw_pcie_ep *ep)  {
> > >>>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > >>> +	struct ls_pcie_ep *pcie = to_ls_pcie_ep(pci);
> > >>>  	enum pci_barno bar;
> > >>>
> > >>>  	for (bar = BAR_0; bar <= BAR_5; bar++)
> > >>>  		dw_pcie_ep_reset_bar(pci, bar);
> > >>> +
> > >>> +	pcie->ls_epc->msi_capable = ep->msi_cap ? true : false;
> > >>> +	pcie->ls_epc->msix_capable = ep->msix_cap ? true : false;
> > >>>  }
> > >>>
> > >>>  static int ls_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> > >>> @@
> > >>> -118,6 +120,7 @@ static int __init ls_pcie_ep_probe(struct
> > >>> platform_device
> > >> *pdev)
> > >>>  	struct device *dev = &pdev->dev;
> > >>>  	struct dw_pcie *pci;
> > >>>  	struct ls_pcie_ep *pcie;
> > >>> +	struct pci_epc_features *ls_epc;
> > >>>  	struct resource *dbi_base;
> > >>>  	int ret;
> > >>>
> > >>> @@ -129,6 +132,10 @@ static int __init ls_pcie_ep_probe(struct
> > >> platform_device *pdev)
> > >>>  	if (!pci)
> > >>>  		return -ENOMEM;
> > >>>
> > >>> +	ls_epc = devm_kzalloc(dev, sizeof(*ls_epc), GFP_KERNEL);
> > >>> +	if (!ls_epc)
> > >>> +		return -ENOMEM;
> > >>> +
> > >>>  	dbi_base = platform_get_resource_byname(pdev,
> > IORESOURCE_MEM,
> > >> "regs");
> > >>>  	pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base);
> > >>>  	if (IS_ERR(pci->dbi_base))
> > >>> @@ -139,6 +146,11 @@ static int __init ls_pcie_ep_probe(struct
> > >> platform_device *pdev)
> > >>>  	pci->ops = &ls_pcie_ep_ops;
> > >>>  	pcie->pci = pci;
> > >>>
> > >>> +	ls_epc->linkup_notifier = false,
> > >>> +	ls_epc->bar_fixed_64bit = (1 << BAR_2) | (1 << BAR_4),
> > >>> +
> > >>> +	pcie->ls_epc = ls_epc;
> > >>> +
> > >>>  	platform_set_drvdata(pdev, pcie);
> > >>>
> > >>>  	ret = ls_add_pcie_ep(pcie, pdev);
> > >>>