RE: [PATCH v2 08/10] PCI: layerscape: Add EP mode support for ls1088a and ls2088a

[Xiaowei Bao]

> -----Original Message-----
> From: Andrew Murray <andrew.murray@xxxxxxx>
> Sent: 2019年8月23日 22:28
> To: Xiaowei Bao <xiaowei.bao@xxxxxxx>
> Cc: bhelgaas@xxxxxxxxxx; robh+dt@xxxxxxxxxx; mark.rutland@xxxxxxx;
> shawnguo@xxxxxxxxxx; Leo Li <leoyang.li@xxxxxxx>; kishon@xxxxxx;
> 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
> Subject: Re: [PATCH v2 08/10] PCI: layerscape: Add EP mode support for
> ls1088a and ls2088a
> 
> On Thu, Aug 22, 2019 at 07:22:40PM +0800, Xiaowei Bao wrote:
> > Add PCIe EP mode support for ls1088a and ls2088a, there are some
> > difference between LS1 and LS2 platform, so refactor the code of the
> > EP driver.
> >
> > Signed-off-by: Xiaowei Bao <xiaowei.bao@xxxxxxx>
> > ---
> > v2:
> >  - New mechanism for layerscape EP driver.
> 
> Was there a v1 of this patch?
> 
> >
> >  drivers/pci/controller/dwc/pci-layerscape-ep.c | 76
> > ++++++++++++++++++++------
> >  1 file changed, 58 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > index 7ca5fe8..2a66f07 100644
> > --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > @@ -20,27 +20,29 @@
> >
> >  #define PCIE_DBI2_OFFSET		0x1000	/* DBI2 base address*/
> >
> > -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)
> > +
> > +struct ls_pcie_ep_drvdata {
> > +	u32				func_offset;
> > +	const struct dw_pcie_ep_ops	*ops;
> > +	const struct dw_pcie_ops	*dw_pcie_ops;
> >  };
> >
> > -#define to_ls_pcie_ep(x)	dev_get_drvdata((x)->dev)
> > +struct ls_pcie_ep {
> > +	struct dw_pcie			*pci;
> > +	struct pci_epc_features		*ls_epc;
> > +	const struct ls_pcie_ep_drvdata *drvdata; };
> >
> >  static int ls_pcie_establish_link(struct dw_pcie *pci)  {
> >  	return 0;
> >  }
> >
> > -static const struct dw_pcie_ops ls_pcie_ep_ops = {
> > +static const struct dw_pcie_ops dw_ls_pcie_ep_ops = {
> >  	.start_link = ls_pcie_establish_link,  };
> >
> > -static const struct of_device_id ls_pcie_ep_of_match[] = {
> > -	{ .compatible = "fsl,ls-pcie-ep",},
> > -	{ },
> > -};
> > -
> >  static const struct pci_epc_features*  ls_pcie_ep_get_features(struct
> > dw_pcie_ep *ep)  { @@ -82,10 +84,44 @@ static int
> > ls_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> >  	}
> >  }
> >
> > -static const struct dw_pcie_ep_ops pcie_ep_ops = {
> > +static unsigned int ls_pcie_ep_func_conf_select(struct dw_pcie_ep *ep,
> > +						u8 func_no)
> > +{
> > +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > +	struct ls_pcie_ep *pcie = to_ls_pcie_ep(pci);
> > +	u8 header_type;
> > +
> > +	header_type = ioread8(pci->dbi_base + PCI_HEADER_TYPE);
> > +
> > +	if (header_type & (1 << 7))
> > +		return pcie->drvdata->func_offset * func_no;
> > +	else
> > +		return 0;
> 
> It looks like there isn't a PCI define for multi function, the nearest I could find
> was PCI_HEADER_TYPE_MULTIDEVICE in hotplug/ibmphp.h. A comment
> above the test might be helpful to explain the test.

OK, I will add a comment above this code.

> 
> As the ls_pcie_ep_drvdata structures are static, the unset .func_offset will be
> initialised to 0, so you could just drop the test above.

Due to the different PCIe controller have different property, e.g. PCIe controller1 support
multiple function feature, but PCIe controller2 don't support this feature, so I need to check
which controller support it and return the correct offset value, but each board only have one
ls_pcie_ep_drvdata, ^_^.

> 
> However something to the effect of the following may help spot
> misconfiguration:
> 
> WARN_ON(func_no && !pcie->drvdata->func_offset); return
> pcie->drvdata->func_offset * func_no;
> 
> The WARN is probably quite useful as if you are attempting to use non-zero
> functions and func_offset isn't set - then things may appear to work normally
> but actually will break horribly.

As discussion before, I think the func_offset should not depends on the function
number, even if other platforms of NXP may be use write registers way to access 
the different function config space. 

I have added the comments above the code, as follow, do you have any advice?
+static unsigned int ls_pcie_ep_func_conf_select(struct dw_pcie_ep *ep,
+                                               u8 func_no)
+{
+       struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+       struct ls_pcie_ep *pcie = to_ls_pcie_ep(pci);
+       u8 header_type;
+
+       header_type = ioread8(pci->dbi_base + PCI_HEADER_TYPE);
+
+       /*
+        * Read the Header Type register of config space to check
+        * whether this PCI device support the multiple function.
+        */
+       if (header_type & (1 << 7))
+               return pcie->drvdata->func_offset * func_no;
+
+       return 0;
+}

Thanks a lot for your detail comments.

> 
> Thanks,
> 
> Andrew Murray
> 
> > +}
> > +
> > +static const struct dw_pcie_ep_ops ls_pcie_ep_ops = {
> >  	.ep_init = ls_pcie_ep_init,
> >  	.raise_irq = ls_pcie_ep_raise_irq,
> >  	.get_features = ls_pcie_ep_get_features,
> > +	.func_conf_select = ls_pcie_ep_func_conf_select, };
> > +
> > +static const struct ls_pcie_ep_drvdata ls1_ep_drvdata = {
> > +	.ops = &ls_pcie_ep_ops,
> > +	.dw_pcie_ops = &dw_ls_pcie_ep_ops,
> > +};
> > +
> > +static const struct ls_pcie_ep_drvdata ls2_ep_drvdata = {
> > +	.func_offset = 0x20000,
> > +	.ops = &ls_pcie_ep_ops,
> > +	.dw_pcie_ops = &dw_ls_pcie_ep_ops,
> > +};
> > +
> > +static const struct of_device_id ls_pcie_ep_of_match[] = {
> > +	{ .compatible = "fsl,ls1046a-pcie-ep", .data = &ls1_ep_drvdata },
> > +	{ .compatible = "fsl,ls1088a-pcie-ep", .data = &ls2_ep_drvdata },
> > +	{ .compatible = "fsl,ls2088a-pcie-ep", .data = &ls2_ep_drvdata },
> > +	{ },
> >  };
> >
> >  static int __init ls_add_pcie_ep(struct ls_pcie_ep *pcie, @@ -98,7
> > +134,7 @@ static int __init ls_add_pcie_ep(struct ls_pcie_ep *pcie,
> >  	int ret;
> >
> >  	ep = &pci->ep;
> > -	ep->ops = &pcie_ep_ops;
> > +	ep->ops = pcie->drvdata->ops;
> >
> >  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> "addr_space");
> >  	if (!res)
> > @@ -137,14 +173,11 @@ static int __init ls_pcie_ep_probe(struct
> platform_device *pdev)
> >  	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))
> > -		return PTR_ERR(pci->dbi_base);
> > +	pcie->drvdata = of_device_get_match_data(dev);
> >
> > -	pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_OFFSET;
> >  	pci->dev = dev;
> > -	pci->ops = &ls_pcie_ep_ops;
> > +	pci->ops = pcie->drvdata->dw_pcie_ops;
> > +
> >  	pcie->pci = pci;
> >
> >  	ls_epc->linkup_notifier = false,
> > @@ -152,6 +185,13 @@ static int __init ls_pcie_ep_probe(struct
> > platform_device *pdev)
> >
> >  	pcie->ls_epc = ls_epc;
> >
> > +	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))
> > +		return PTR_ERR(pci->dbi_base);
> > +
> > +	pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_OFFSET;
> > +
> >  	platform_set_drvdata(pdev, pcie);
> >
> >  	ret = ls_add_pcie_ep(pcie, pdev);
> > --
> > 2.9.5
> >