LinuxPPC-Dev Archive on lore.kernel.org
 help / color / Atom feed
From: Xiaowei Bao <xiaowei.bao@nxp.com>
To: Andrew Murray <andrew.murray@arm.com>
Cc: "mark.rutland@arm.com" <mark.rutland@arm.com>,
	Roy Zang <roy.zang@nxp.com>,
	"lorenzo.pieralisi@arm.co" <lorenzo.pieralisi@arm.co>,
	"arnd@arndb.de" <arnd@arndb.de>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"kishon@ti.com" <kishon@ti.com>,
	"M.h. Lian" <minghuan.lian@nxp.com>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"gustavo.pimentel@synopsys.com" <gustavo.pimentel@synopsys.com>,
	"jingoohan1@gmail.com" <jingoohan1@gmail.com>,
	"bhelgaas@google.com" <bhelgaas@google.com>,
	Leo Li <leoyang.li@nxp.com>,
	"shawnguo@kernel.org" <shawnguo@kernel.org>,
	Mingkai Hu <mingkai.hu@nxp.com>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>
Subject: RE: [PATCH v2 08/10] PCI: layerscape: Add EP mode support for ls1088a and ls2088a
Date: Mon, 26 Aug 2019 09:49:35 +0000
Message-ID: <AM5PR04MB3299B100D1029E90945CF7DDF5A10@AM5PR04MB3299.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <20190823142756.GI14582@e119886-lin.cambridge.arm.com>



> -----Original Message-----
> From: Andrew Murray <andrew.murray@arm.com>
> Sent: 2019年8月23日 22:28
> To: Xiaowei Bao <xiaowei.bao@nxp.com>
> Cc: bhelgaas@google.com; robh+dt@kernel.org; mark.rutland@arm.com;
> shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; kishon@ti.com;
> lorenzo.pieralisi@arm.co; arnd@arndb.de; gregkh@linuxfoundation.org; M.h.
> Lian <minghuan.lian@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; Roy
> Zang <roy.zang@nxp.com>; jingoohan1@gmail.com;
> gustavo.pimentel@synopsys.com; linux-pci@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linuxppc-dev@lists.ozlabs.org
> 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@nxp.com>
> > ---
> > 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
> >

  parent reply index

Thread overview: 50+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-22 11:22 [PATCH v2 01/10] PCI: designware-ep: Add multiple PFs support for DWC Xiaowei Bao
2019-08-22 11:22 ` [PATCH v2 02/10] PCI: designware-ep: Add the doorbell mode of MSI-X in EP mode Xiaowei Bao
2019-08-23 13:35   ` Andrew Murray
2019-08-23 23:51     ` Xiaowei Bao
2019-08-22 11:22 ` [PATCH v2 03/10] PCI: designware-ep: Move the function of getting MSI capability forward Xiaowei Bao
2019-08-23 13:38   ` Andrew Murray
2019-08-24  0:20     ` Xiaowei Bao
2019-08-22 11:22 ` [PATCH v2 04/10] dt-bindings: pci: layerscape-pci: add compatible strings for ls1088a and ls2088a Xiaowei Bao
2019-08-27 22:26   ` Rob Herring
2019-08-29  9:19     ` Xiaowei Bao
2019-08-22 11:22 ` [PATCH v2 05/10] PCI: layerscape: Fix some format issue of the code Xiaowei Bao
2019-08-23 13:45   ` Andrew Murray
2019-08-24  0:00     ` Xiaowei Bao
2019-08-22 11:22 ` [PATCH v2 06/10] PCI: layerscape: Modify the way of getting capability with different PEX Xiaowei Bao
2019-08-22 11:43   ` Kishon Vijay Abraham I
2019-08-23  2:39     ` Xiaowei Bao
2019-08-23  3:39       ` Kishon Vijay Abraham I
2019-08-23  4:13         ` Xiaowei Bao
2019-09-02 13:36           ` Andrew Murray
2019-09-03  2:11             ` Xiaowei Bao
2019-08-22 11:22 ` [PATCH v2 07/10] PCI: layerscape: Modify the MSIX to the doorbell way Xiaowei Bao
2019-08-23 13:58   ` Andrew Murray
2019-08-24  0:08     ` Xiaowei Bao
2019-08-27 13:25       ` Andrew Murray
2019-08-28  2:49         ` Xiaowei Bao
2019-08-29  5:13         ` Kishon Vijay Abraham I
2019-11-05 12:37           ` Lorenzo Pieralisi
2019-11-06  9:33             ` Xiaowei Bao
2019-11-06  9:40           ` Gustavo Pimentel
2019-11-06 10:03             ` Xiaowei Bao
2019-11-06 13:39             ` Kishon Vijay Abraham I
2019-11-06 15:40               ` Gustavo Pimentel
2019-08-22 11:22 ` [PATCH v2 08/10] PCI: layerscape: Add EP mode support for ls1088a and ls2088a Xiaowei Bao
2019-08-23 14:27   ` Andrew Murray
2019-08-24  0:18     ` Xiaowei Bao
2019-08-24  6:45       ` christophe leroy
2019-08-25  3:07         ` Xiaowei Bao
2019-08-27 14:48           ` Andrew Murray
2019-08-28  3:25             ` Xiaowei Bao
2019-08-26  9:49     ` Xiaowei Bao [this message]
2019-08-27 13:34       ` Andrew Murray
2019-08-28  4:29         ` Xiaowei Bao
2019-08-28  9:01           ` Andrew Murray
2019-08-29  2:03             ` Xiaowei Bao
2019-08-22 11:22 ` [PATCH v2 09/10] arm64: dts: layerscape: Add PCIe EP node for ls1088a Xiaowei Bao
2019-08-22 11:22 ` [PATCH v2 10/10] misc: pci_endpoint_test: Add LS1088a in pci_device_id table Xiaowei Bao
2019-08-23 13:25 ` [PATCH v2 01/10] PCI: designware-ep: Add multiple PFs support for DWC Andrew Murray
2019-08-23 23:50   ` Xiaowei Bao
2019-08-27 13:10     ` Andrew Murray
2019-08-28  7:22       ` Xiaowei Bao

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=AM5PR04MB3299B100D1029E90945CF7DDF5A10@AM5PR04MB3299.eurprd04.prod.outlook.com \
    --to=xiaowei.bao@nxp.com \
    --cc=andrew.murray@arm.com \
    --cc=arnd@arndb.de \
    --cc=bhelgaas@google.com \
    --cc=devicetree@vger.kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=gustavo.pimentel@synopsys.com \
    --cc=jingoohan1@gmail.com \
    --cc=kishon@ti.com \
    --cc=leoyang.li@nxp.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=lorenzo.pieralisi@arm.co \
    --cc=mark.rutland@arm.com \
    --cc=minghuan.lian@nxp.com \
    --cc=mingkai.hu@nxp.com \
    --cc=robh+dt@kernel.org \
    --cc=roy.zang@nxp.com \
    --cc=shawnguo@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LinuxPPC-Dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linuxppc-dev/0 linuxppc-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linuxppc-dev linuxppc-dev/ https://lore.kernel.org/linuxppc-dev \
		linuxppc-dev@lists.ozlabs.org linuxppc-dev@ozlabs.org
	public-inbox-index linuxppc-dev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.ozlabs.lists.linuxppc-dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git