quoted

-----Original Message-----
From: Andrew Murray <redacted>
Sent: 2019年8月27日 21:34
To: Xiaowei Bao <redacted>
Cc: bhelgaas@google.com; robh+dt@kernel.org; mark.rutland@arm.com;
shawnguo@kernel.org; Leo Li [off-list ref]; kishon@ti.com;
lorenzo.pieralisi@arm.co; arnd@arndb.de; gregkh@linuxfoundation.org; M.h.
Lian [off-list ref]; Mingkai Hu [off-list ref]; Roy
Zang [off-list ref]; 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 Mon, Aug 26, 2019 at 09:49:35AM +0000, Xiaowei Bao wrote:

quoted

quoted

-----Original Message-----
From: Andrew Murray <redacted>
Sent: 2019年8月23日 22:28
To: Xiaowei Bao <redacted>
Cc: bhelgaas@google.com; robh+dt@kernel.org; mark.rutland@arm.com;
shawnguo@kernel.org; Leo Li [off-list ref]; kishon@ti.com;
lorenzo.pieralisi@arm.co; arnd@arndb.de; gregkh@linuxfoundation.org;

M.h.

quoted

quoted

Lian [off-list ref]; Mingkai Hu [off-list ref]; Roy
Zang [off-list ref]; 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:

quoted

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 <redacted>
---
v2:
 - New mechanism for layerscape EP driver.

Was there a v1 of this patch?

quoted

 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,

quoted

quoted

quoted

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

quoted

OK, I will add a comment above this code.

quoted

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, ^_^.

Yes but if they don't support the feature then func_offset will be 0.

quoted

quoted

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 agree that func_offset is an optional parameter. But if you are attempting to
determine the offset of a function and you are given a non-zero function
number - then something has gone wrong if func_offset is 0.

I have understood you means, maybe I need to set a flag in the driver_data struct,
because I may add other platform of NXP, these platform use the write register 
method to access different function, e.g. 
write func_num to register, then we can access this func_num config space.

I will modify the code like this? Do you have better advice?
Case1:

diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c b/drivers/pci/controller/dwc/pci-layerscape-ep.c
index 004a7e8..8a0d6df 100644
--- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
+++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c

@@ -23,6 +23,7 @@
 #define to_ls_pcie_ep(x)       dev_get_drvdata((x)->dev)

 struct ls_pcie_ep_drvdata {
+       u8                              func_config_flag;
        u32                             func_offset;
        const struct dw_pcie_ep_ops     *ops;
        const struct dw_pcie_ops        *dw_pcie_ops;

@@ -97,8 +98,14 @@ static unsigned int ls_pcie_ep_func_conf_select(struct dw_pcie_ep *ep,
         * 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;
+       if (header_type & (1 << 7)) {
+               if (pcie->drvdata->func_config_flag) {
+                       iowrite32((func_num << n), pci->dbi_base + PCI_XXXX_XXX);
+               } else {
+                       WARN_ON(func_no && !pcie->drvdata->func_offset);
+                       return pcie->drvdata->func_offset * func_no;
+               }
+       }

        return 0;
 }

Of course, I don't need to set the flag this time, because I don't use the second method(write
register method), so the code like this:
case2:
+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;

	   of course, this code is not requied, due to the 
	   pcie->drvdata->func_offset is 0, but I think this is more clear
	   if use this code.
       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)) {
			   WARN_ON(func_no && !pcie->drvdata->func_offset);
               return pcie->drvdata->func_offset * func_no; 
		}
		
       return 0;
}

Or like this:
Case3:
+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);

	   WARN_ON(func_no && !pcie->drvdata->func_offset);
       return pcie->drvdata->func_offset * func_no;