[PATCH 1/2] PCI: designware: change MSI-related pcie_host_ops

[PATCH 1/2] PCI: designware: change MSI-related pcie_host_ops

[Minghuan Lian]

1. Change original get_msi_data() to get_msi_addr(), since this
function returns MSI message address.
2. Add a new function get_msi_data() to return MSI message data.

Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com>
---
 drivers/pci/host/pcie-designware.c | 11 ++++++++---
 drivers/pci/host/pcie-designware.h |  3 ++-
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index 52bd3a1..c84913e 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -373,12 +373,17 @@ static int dw_msi_setup_irq(struct msi_chip *chip, struct pci_dev *pdev,
 	 */
 	desc->msi_attrib.multiple = msgvec;
 
-	if (pp->ops->get_msi_data)
-		msg.address_lo = pp->ops->get_msi_data(pp);
+	if (pp->ops->get_msi_addr)
+		msg.address_lo = pp->ops->get_msi_addr(pp);
 	else
 		msg.address_lo = virt_to_phys((void *)pp->msi_data);
 	msg.address_hi = 0x0;
-	msg.data = pos;
+
+	if (pp->ops->get_msi_data)
+		msg.data = pp->ops->get_msi_data(pp, pos);
+	else
+		msg.data = pos;
+
 	write_msi_msg(irq, &msg);
 
 	return 0;
diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
index daf81f9..7f08807 100644
--- a/drivers/pci/host/pcie-designware.h
+++ b/drivers/pci/host/pcie-designware.h
@@ -73,7 +73,8 @@ struct pcie_host_ops {
 	void (*host_init)(struct pcie_port *pp);
 	void (*msi_set_irq)(struct pcie_port *pp, int irq);
 	void (*msi_clear_irq)(struct pcie_port *pp, int irq);
-	u32 (*get_msi_data)(struct pcie_port *pp);
+	u32 (*get_msi_addr)(struct pcie_port *pp);
+	u32 (*get_msi_data)(struct pcie_port *pp, int pos);
 };
 
 int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val);
-- 
1.9.1

[PATCH 1/2] PCI: designware: change MSI-related pcie_host_ops

[Minghuan Lian]

1. Change original get_msi_data() to get_msi_addr(), since this
function returns MSI message address.
2. Add a new function get_msi_data() to return MSI message data.

Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com>
---
 drivers/pci/host/pcie-designware.c | 11 ++++++++---
 drivers/pci/host/pcie-designware.h |  3 ++-
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index 52bd3a1..c84913e 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -373,12 +373,17 @@ static int dw_msi_setup_irq(struct msi_chip *chip, struct pci_dev *pdev,
 	 */
 	desc->msi_attrib.multiple = msgvec;
 
-	if (pp->ops->get_msi_data)
-		msg.address_lo = pp->ops->get_msi_data(pp);
+	if (pp->ops->get_msi_addr)
+		msg.address_lo = pp->ops->get_msi_addr(pp);
 	else
 		msg.address_lo = virt_to_phys((void *)pp->msi_data);
 	msg.address_hi = 0x0;
-	msg.data = pos;
+
+	if (pp->ops->get_msi_data)
+		msg.data = pp->ops->get_msi_data(pp, pos);
+	else
+		msg.data = pos;
+
 	write_msi_msg(irq, &msg);
 
 	return 0;
diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
index daf81f9..7f08807 100644
--- a/drivers/pci/host/pcie-designware.h
+++ b/drivers/pci/host/pcie-designware.h
@@ -73,7 +73,8 @@ struct pcie_host_ops {
 	void (*host_init)(struct pcie_port *pp);
 	void (*msi_set_irq)(struct pcie_port *pp, int irq);
 	void (*msi_clear_irq)(struct pcie_port *pp, int irq);
-	u32 (*get_msi_data)(struct pcie_port *pp);
+	u32 (*get_msi_addr)(struct pcie_port *pp);
+	u32 (*get_msi_data)(struct pcie_port *pp, int pos);
 };
 
 int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val);
-- 
1.9.1

Re: [PATCH 1/2] PCI: designware: change MSI-related pcie_host_ops

[Bjorn Helgaas]

On Thu, Sep 4, 2014 at 12:45 PM, Minghuan Lian
<Minghuan.Lian@freescale.com> wrote:
> 1. Change original get_msi_data() to get_msi_addr(), since this
> function returns MSI message address.
> 2. Add a new function get_msi_data() to return MSI message data.

This looks like two patches: one that merely renames a function, with
no functional change at all, and a second that adds a new function.
Please split them.

> Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com>
> ---
>  drivers/pci/host/pcie-designware.c | 11 ++++++++---
>  drivers/pci/host/pcie-designware.h |  3 ++-
>  2 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index 52bd3a1..c84913e 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -373,12 +373,17 @@ static int dw_msi_setup_irq(struct msi_chip *chip, struct pci_dev *pdev,
>          */
>         desc->msi_attrib.multiple = msgvec;
>
> -       if (pp->ops->get_msi_data)
> -               msg.address_lo = pp->ops->get_msi_data(pp);
> +       if (pp->ops->get_msi_addr)
> +               msg.address_lo = pp->ops->get_msi_addr(pp);
>         else
>                 msg.address_lo = virt_to_phys((void *)pp->msi_data);
>         msg.address_hi = 0x0;
> -       msg.data = pos;
> +
> +       if (pp->ops->get_msi_data)
> +               msg.data = pp->ops->get_msi_data(pp, pos);
> +       else
> +               msg.data = pos;
> +
>         write_msi_msg(irq, &msg);
>
>         return 0;
> diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
> index daf81f9..7f08807 100644
> --- a/drivers/pci/host/pcie-designware.h
> +++ b/drivers/pci/host/pcie-designware.h
> @@ -73,7 +73,8 @@ struct pcie_host_ops {
>         void (*host_init)(struct pcie_port *pp);
>         void (*msi_set_irq)(struct pcie_port *pp, int irq);
>         void (*msi_clear_irq)(struct pcie_port *pp, int irq);
> -       u32 (*get_msi_data)(struct pcie_port *pp);
> +       u32 (*get_msi_addr)(struct pcie_port *pp);
> +       u32 (*get_msi_data)(struct pcie_port *pp, int pos);
>  };
>
>  int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val);
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] PCI: designware: change MSI-related pcie_host_ops

[Bjorn Helgaas]

On Thu, Sep 4, 2014 at 12:45 PM, Minghuan Lian
<Minghuan.Lian@freescale.com> wrote:
> 1. Change original get_msi_data() to get_msi_addr(), since this
> function returns MSI message address.
> 2. Add a new function get_msi_data() to return MSI message data.

This looks like two patches: one that merely renames a function, with
no functional change at all, and a second that adds a new function.
Please split them.

> Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com>
> ---
>  drivers/pci/host/pcie-designware.c | 11 ++++++++---
>  drivers/pci/host/pcie-designware.h |  3 ++-
>  2 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index 52bd3a1..c84913e 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -373,12 +373,17 @@ static int dw_msi_setup_irq(struct msi_chip *chip, struct pci_dev *pdev,
>          */
>         desc->msi_attrib.multiple = msgvec;
>
> -       if (pp->ops->get_msi_data)
> -               msg.address_lo = pp->ops->get_msi_data(pp);
> +       if (pp->ops->get_msi_addr)
> +               msg.address_lo = pp->ops->get_msi_addr(pp);
>         else
>                 msg.address_lo = virt_to_phys((void *)pp->msi_data);
>         msg.address_hi = 0x0;
> -       msg.data = pos;
> +
> +       if (pp->ops->get_msi_data)
> +               msg.data = pp->ops->get_msi_data(pp, pos);
> +       else
> +               msg.data = pos;
> +
>         write_msi_msg(irq, &msg);
>
>         return 0;
> diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
> index daf81f9..7f08807 100644
> --- a/drivers/pci/host/pcie-designware.h
> +++ b/drivers/pci/host/pcie-designware.h
> @@ -73,7 +73,8 @@ struct pcie_host_ops {
>         void (*host_init)(struct pcie_port *pp);
>         void (*msi_set_irq)(struct pcie_port *pp, int irq);
>         void (*msi_clear_irq)(struct pcie_port *pp, int irq);
> -       u32 (*get_msi_data)(struct pcie_port *pp);
> +       u32 (*get_msi_addr)(struct pcie_port *pp);
> +       u32 (*get_msi_data)(struct pcie_port *pp, int pos);
>  };
>
>  int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val);
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

答复: [PATCH 1/2] PCI: designware: change MSI-related pcie_host_ops

[Minghuan.Lian at freescale.com]

Hi Bjorn,

Thanks for your comment.
I will split them in next version.
Currently, the get_msi_addr() function returns u32 address, but MSI message address is 64bits.
Should we change get_msi_addr() to return u64 address?

Thanks,
Minghuan

________________________________________
???: Bjorn Helgaas <bhelgaas@google.com>
????: 2014?9?4? 21:20
???: Lian Minghuan-B31939
??: linux-pci at vger.kernel.org; linux-arm; Zang Roy-R61911; Hu Mingkai-B21284
??: Re: [PATCH 1/2] PCI: designware: change MSI-related pcie_host_ops

On Thu, Sep 4, 2014 at 12:45 PM, Minghuan Lian
<Minghuan.Lian@freescale.com> wrote:
> 1. Change original get_msi_data() to get_msi_addr(), since this
> function returns MSI message address.
> 2. Add a new function get_msi_data() to return MSI message data.

This looks like two patches: one that merely renames a function, with
no functional change at all, and a second that adds a new function.
Please split them.

> Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com>
> ---
>  drivers/pci/host/pcie-designware.c | 11 ++++++++---
>  drivers/pci/host/pcie-designware.h |  3 ++-
>  2 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index 52bd3a1..c84913e 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -373,12 +373,17 @@ static int dw_msi_setup_irq(struct msi_chip *chip, struct pci_dev *pdev,
>          */
>         desc->msi_attrib.multiple = msgvec;
>
> -       if (pp->ops->get_msi_data)
> -               msg.address_lo = pp->ops->get_msi_data(pp);
> +       if (pp->ops->get_msi_addr)
> +               msg.address_lo = pp->ops->get_msi_addr(pp);
>         else
>                 msg.address_lo = virt_to_phys((void *)pp->msi_data);
>         msg.address_hi = 0x0;
> -       msg.data = pos;
> +
> +       if (pp->ops->get_msi_data)
> +               msg.data = pp->ops->get_msi_data(pp, pos);
> +       else
> +               msg.data = pos;
> +
>         write_msi_msg(irq, &msg);
>
>         return 0;
> diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
> index daf81f9..7f08807 100644
> --- a/drivers/pci/host/pcie-designware.h
> +++ b/drivers/pci/host/pcie-designware.h
> @@ -73,7 +73,8 @@ struct pcie_host_ops {
>         void (*host_init)(struct pcie_port *pp);
>         void (*msi_set_irq)(struct pcie_port *pp, int irq);
>         void (*msi_clear_irq)(struct pcie_port *pp, int irq);
> -       u32 (*get_msi_data)(struct pcie_port *pp);
> +       u32 (*get_msi_addr)(struct pcie_port *pp);
> +       u32 (*get_msi_data)(struct pcie_port *pp, int pos);
>  };
>
>  int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val);
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Minghuan Lian]

Add support for Freescale Layerscape PCIe controller. This driver
re-uses the designware core code.

Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com>
---
 .../devicetree/bindings/pci/fsl,ls-pcie.txt        |  41 +++
 drivers/pci/host/Kconfig                           |   7 +
 drivers/pci/host/Makefile                          |   1 +
 drivers/pci/host/pci-layerscape.c                  | 315 +++++++++++++++++++++
 include/linux/pci_ids.h                            |   1 +
 5 files changed, 365 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/fsl,ls-pcie.txt
 create mode 100644 drivers/pci/host/pci-layerscape.c

diff --git a/Documentation/devicetree/bindings/pci/fsl,ls-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,ls-pcie.txt
new file mode 100644
index 0000000..af2ee1c0
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/fsl,ls-pcie.txt
@@ -0,0 +1,41 @@
+Freescale Layerscape PCIe controller
+
+This PCIe host controller is based on the Synopsis Designware PCIe IP
+and thus inherits all the common properties defined in designware-pcie.txt.
+
+Required properties:
+- compatible: should contain "fsl,ls-pcie", to identify the platform,
+  plus an identifier for specific instance such as "fsl,ls1021a-pcie"
+- reg: base addresses and lengths of the PCIe controller
+- interrupts: A list of interrupt outputs of the controller. Must contain an
+  entry for each entry in the interrupt-names property.
+- interrupt-names: Must include the following entries:
+  "intr": The interrupt that is asserted for controller interrupts
+  "msi": The interrupt that is asserted when an MSI is received
+  "pme": The interrupt that is asserted when PME state changes
+
+Example:
+
+	pcie@3400000 {
+		compatible = "fsl,ls1021a-pcie", "fsl,ls-pcie", "snps,dw-pcie";
+		reg = <0x00 0x03400000 0x0 0x00010000   /* controller registers */
+		       0x40 0x00000000 0x0 0x00080000>; /* configuration space */
+		reg-names = "regs", "config";
+		interrupts = <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH>, /* controller interrupt */
+			     <GIC_SPI 179 IRQ_TYPE_LEVEL_HIGH>, /* MSI interrupt */
+			     <GIC_SPI 181 IRQ_TYPE_LEVEL_HIGH>; /* PME interrupt */
+		interrupt-names = "intr", "msi", "pme";
+		#address-cells = <3>;
+		#size-cells = <2>;
+		device_type = "pci";
+		num-lanes = <4>;
+		bus-range = <0x0 0xff>;
+		ranges = <0x81000000 0x0 0x00000000 0x40 0x10000000 0x0 0x00010000   /* downstream I/O */
+			  0x82000000 0x0 0x00000000 0x41 0x00000000 0x1 0x00000000>; /* non-prefetchable memory */
+		#interrupt-cells = <1>;
+		interrupt-map-mask = <0 0 0 7>;
+		interrupt-map = <0000 0 0 1 &gic GIC_SPI 91  IRQ_TYPE_LEVEL_HIGH>,
+				<0000 0 0 2 &gic GIC_SPI 188 IRQ_TYPE_LEVEL_HIGH>,
+				<0000 0 0 3 &gic GIC_SPI 190 IRQ_TYPE_LEVEL_HIGH>,
+				<0000 0 0 4 &gic GIC_SPI 192 IRQ_TYPE_LEVEL_HIGH>;
+	};
diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index 8922c37..e0d7bbf 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -63,4 +63,11 @@ config PCIE_SPEAR13XX
 	help
 	  Say Y here if you want PCIe support on SPEAr13XX SoCs.
 
+config PCI_LAYERSCAPE
+	bool "Freescale Layerscape PCIe controller"
+	depends on SOC_LS1021A
+	select PCIE_DW
+	help
+	  Say Y here if you want PCIe controller support on Layerscape SoCs.
+
 endmenu
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index d0e88f1..54cd9b2 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -8,3 +8,4 @@ obj-$(CONFIG_PCI_RCAR_GEN2) += pci-rcar-gen2.o
 obj-$(CONFIG_PCI_RCAR_GEN2_PCIE) += pcie-rcar.o
 obj-$(CONFIG_PCI_HOST_GENERIC) += pci-host-generic.o
 obj-$(CONFIG_PCIE_SPEAR13XX) += pcie-spear13xx.o
+obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
diff --git a/drivers/pci/host/pci-layerscape.c b/drivers/pci/host/pci-layerscape.c
new file mode 100644
index 0000000..077a20b
--- /dev/null
+++ b/drivers/pci/host/pci-layerscape.c
@@ -0,0 +1,315 @@
+/*
+ * PCIe host controller driver for Freescale Layerscape SoCs
+ *
+ * Copyright (C) 2014 Freescale Semiconductor.
+ *
+  * Author: Minghuan Lian <Minghuan.Lian@freescale.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/of_pci.h>
+#include <linux/of_platform.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/resource.h>
+
+#include "pcie-designware.h"
+
+/* PEX1/2 Misc Ports Status Register */
+#define PEXMSCPORTSR(pex_idx)	(0x94 + (pex_idx) * 4)
+#define LTSSM_STATE_SHIFT	20
+#define LTSSM_STATE_MASK	0x3f
+#define LTSSM_PCIE_L0		0x11 /* L0 state */
+
+#define SCFG_SPIMSICR_OFF	0x40
+#define SCFG_SPIMSICR_ADDR	0x1570040
+#define SCFG_SPIMSICLRCR_OFF	0x90
+#define SCFG_SCFGREVCR_OFF	0x200
+#define SCFG_BIT_REVERSE	0xFFFFFFFF
+#define SCFG_NO_BIT_REVERSE	0x0
+
+#define MSI_LS1021A_DATA(pex_idx)	(0xb3 + pex_idx)
+
+/* Symbol Timer Register and Filter Mask Register 1 */
+#define PCIE_STRFMR1 0x71c
+
+#define PCIE_LS1021A_BASE	0x3400000
+#define PCIE_LS1021A_REG_SIZE	0x0100000
+
+struct ls_pcie {
+	struct list_head node;
+	struct device *dev;
+	struct pci_bus *bus;
+	void __iomem *dbi;
+	void __iomem *scfg;
+	struct pcie_port pp;
+	int id;
+	int index;
+	int irq;
+	int msi_irq;
+	int pme_irq;
+};
+
+static LIST_HEAD(ls_pcie_list);
+
+#define to_ls_pcie(x)	container_of(x, struct ls_pcie, pp)
+
+static int ls_pcie_link_up(struct pcie_port *pp)
+{
+	u32 rc, tmp;
+	struct ls_pcie *pcie = to_ls_pcie(pp);
+
+	tmp = ioread32(pcie->scfg + SCFG_SCFGREVCR_OFF);
+	iowrite32(SCFG_NO_BIT_REVERSE, pcie->scfg + SCFG_SCFGREVCR_OFF);
+
+	rc = (ioread32(pcie->scfg + PEXMSCPORTSR(pcie->index)) >>
+	     LTSSM_STATE_SHIFT) & LTSSM_STATE_MASK;
+
+	iowrite32(tmp, pcie->scfg + SCFG_SCFGREVCR_OFF);
+
+	if (rc < LTSSM_PCIE_L0)
+		return 0;
+
+	return 1;
+}
+
+static u32 ls_pcie_get_msi_addr(struct pcie_port *pp)
+{
+	return SCFG_SPIMSICR_ADDR;
+}
+
+static u32 ls_pcie_get_msi_data(struct pcie_port *pp, int pos)
+{
+	struct ls_pcie *pcie = to_ls_pcie(pp);
+
+	if (pcie->id == PCI_DEVICE_ID_LS1021A)
+		return MSI_LS1021A_DATA(pcie->index);
+
+	return pos;
+}
+
+static irqreturn_t ls_pcie_msi_irq_handler(int irq, void *data)
+{
+	struct pcie_port *pp = data;
+	struct ls_pcie *pcie = to_ls_pcie(pp);
+	unsigned int msi_irq, processed = 0;
+
+	if (pcie->id == PCI_DEVICE_ID_LS1021A) {
+		/* clear the interrupt */
+		iowrite32(MSI_LS1021A_DATA(pcie->index),
+			  pcie->scfg + SCFG_SPIMSICLRCR_OFF);
+
+		msi_irq = irq_find_mapping(pp->irq_domain, 0);
+		if (msi_irq)
+			generic_handle_irq(msi_irq);
+		else
+			/*
+			 * that's weird who triggered this?
+			 * just clear it
+			 */
+			dev_info(pcie->dev, "unexpected MSI\n");
+
+		processed++;
+	}
+
+	return processed > 0 ? IRQ_HANDLED : IRQ_NONE;
+}
+
+static void ls_pcie_msi_clear_irq(struct pcie_port *pp, int irq)
+{
+}
+
+static void ls_pcie_msi_set_irq(struct pcie_port *pp, int irq)
+{
+	struct ls_pcie *pcie = to_ls_pcie(pp);
+
+	/* MSI needs to set SCFG bit reverse */
+	iowrite32(SCFG_BIT_REVERSE, pcie->scfg + SCFG_SCFGREVCR_OFF);
+}
+
+static void ls_pcie_msi_fixup(struct pcie_port *pp)
+{
+	struct ls_pcie *pcie = to_ls_pcie(pp);
+	int i;
+
+	if (pcie->id != PCI_DEVICE_ID_LS1021A)
+		return;
+
+	/**
+	 * LS1021A has only one MSI interrupt
+	 * Set all msi interrupts as used except the first one
+	 */
+	for (i = 1; i < MAX_MSI_IRQS; i++)
+		set_bit(i, pp->msi_irq_in_use);
+}
+
+static void ls_pcie_host_init(struct pcie_port *pp)
+{
+	struct ls_pcie *pcie = to_ls_pcie(pp);
+	int count = 0;
+	u32 val;
+
+	dw_pcie_setup_rc(pp);
+
+	while (!ls_pcie_link_up(pp)) {
+		usleep_range(100, 1000);
+		count++;
+		if (count >= 200) {
+			dev_err(pp->dev, "phy link never came up\n");
+			return;
+		}
+	}
+
+	/* Workaround for internal TKT228622 to fix the INTx hang issue */
+	val = ioread32(pcie->dbi + PCIE_STRFMR1);
+	val &= 0xffff;
+	iowrite32(val, pcie->dbi + PCIE_STRFMR1);
+
+	ls_pcie_msi_fixup(pp);
+}
+
+static struct pcie_host_ops ls_pcie_host_ops = {
+	.link_up = ls_pcie_link_up,
+	.host_init = ls_pcie_host_init,
+	.msi_set_irq = ls_pcie_msi_set_irq,
+	.msi_clear_irq = ls_pcie_msi_clear_irq,
+	.get_msi_addr = ls_pcie_get_msi_addr,
+	.get_msi_data = ls_pcie_get_msi_data,
+};
+
+static int ls_add_pcie_port(struct ls_pcie *pcie)
+{
+	struct pcie_port *pp;
+	int ret;
+
+	if (!pcie)
+		return -EINVAL;
+
+	pp = &pcie->pp;
+	pp->dev = pcie->dev;
+	pp->dbi_base = pcie->dbi;
+	pp->msi_irq = pcie->msi_irq;
+
+	if (IS_ENABLED(CONFIG_PCI_MSI)) {
+		ret = devm_request_irq(pp->dev, pp->msi_irq,
+					ls_pcie_msi_irq_handler,
+					IRQF_SHARED, "ls-pcie-msi", pp);
+		if (ret) {
+			dev_err(pp->dev, "failed to request msi irq\n");
+			return ret;
+		}
+	}
+
+	pp->root_bus_nr = -1;
+	pp->ops = &ls_pcie_host_ops;
+
+	ret = dw_pcie_host_init(pp);
+	if (ret) {
+		dev_err(pp->dev, "failed to initialize host\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int __init ls_pcie_probe(struct platform_device *pdev)
+{
+	struct ls_pcie *pcie;
+	struct resource *dbi_base;
+	struct device_node *np;
+	int ret;
+
+	pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
+	if (!pcie)
+		return -ENOMEM;
+
+	pcie->dev = &pdev->dev;
+
+	dbi_base = platform_get_resource_byname(pdev, IORESOURCE_MEM, "regs");
+	if (!dbi_base) {
+		dev_err(&pdev->dev, "missing *regs* space\n");
+		return -ENODEV;
+	}
+
+	pcie->dbi = devm_ioremap_resource(&pdev->dev, dbi_base);
+	if (IS_ERR(pcie->dbi))
+		return PTR_ERR(pcie->dbi);
+
+	if (of_device_is_compatible(pdev->dev.of_node, "fsl,ls1021a-pcie")) {
+		pcie->id = PCI_DEVICE_ID_LS1021A;
+		pcie->index = (dbi_base->start - PCIE_LS1021A_BASE) /
+			      PCIE_LS1021A_REG_SIZE;
+
+		/* map SCFG register */
+		np = of_find_compatible_node(NULL, NULL, "fsl,ls1021a-scfg");
+		pcie->scfg = of_iomap(np, 0);
+		if (!pcie->scfg) {
+			dev_err(&pdev->dev, "unable to find SCFG registers\n");
+			return PTR_ERR(pcie->scfg);
+		}
+	}
+
+	/* request interrupt */
+	pcie->irq = platform_get_irq_byname(pdev, "intr");
+	if (pcie->irq < 0) {
+		dev_err(&pdev->dev, "failed to get IRQ: %d\n", pcie->irq);
+		return pcie->irq;
+	}
+
+	pcie->msi_irq = platform_get_irq_byname(pdev, "msi");
+	if (pcie->msi_irq < 0) {
+		dev_err(&pdev->dev,
+			"failed to get MSI IRQ: %d\n", pcie->msi_irq);
+		return pcie->msi_irq;
+	}
+
+	pcie->pme_irq = platform_get_irq_byname(pdev, "pme");
+	if (pcie->pme_irq < 0) {
+		dev_err(&pdev->dev,
+			"failed to get PME IRQ: %d\n", pcie->pme_irq);
+		return pcie->pme_irq;
+	}
+
+	ret = ls_add_pcie_port(pcie);
+	if (ret < 0)
+		return ret;
+
+	platform_set_drvdata(pdev, pcie);
+	list_add(&pcie->node, &ls_pcie_list);
+
+	return 0;
+}
+
+static const struct of_device_id ls_pcie_of_match[] = {
+	{ .compatible = "fsl,ls-pcie" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, ls_pcie_of_match);
+
+static struct platform_driver ls_pcie_driver = {
+	.driver = {
+		.name = "layerscape-pcie",
+		.owner = THIS_MODULE,
+		.of_match_table = ls_pcie_of_match,
+	},
+};
+
+/* Freescale PCIe driver does not allow module unload */
+static int __init ls_pcie_init(void)
+{
+	return platform_driver_probe(&ls_pcie_driver, ls_pcie_probe);
+}
+subsys_initcall(ls_pcie_init);
+
+MODULE_AUTHOR("Minghuan Lian <Minghuan.Lian@freescale.com>");
+MODULE_DESCRIPTION("Freescale Layerscape PCIe host controller driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 6ed0bb7..26d4c30 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2442,6 +2442,7 @@
 #define PCI_DEVICE_ID_P5020		0x0421
 #define PCI_DEVICE_ID_P5010E		0x0428
 #define PCI_DEVICE_ID_P5010		0x0429
+#define PCI_DEVICE_ID_LS1021A		0x0e0b
 #define PCI_DEVICE_ID_MPC8641		0x7010
 #define PCI_DEVICE_ID_MPC8641D		0x7011
 #define PCI_DEVICE_ID_MPC8610		0x7018
-- 
1.9.1

[PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Minghuan Lian]

Add support for Freescale Layerscape PCIe controller. This driver
re-uses the designware core code.

Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com>
---
 .../devicetree/bindings/pci/fsl,ls-pcie.txt        |  41 +++
 drivers/pci/host/Kconfig                           |   7 +
 drivers/pci/host/Makefile                          |   1 +
 drivers/pci/host/pci-layerscape.c                  | 315 +++++++++++++++++++++
 include/linux/pci_ids.h                            |   1 +
 5 files changed, 365 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/fsl,ls-pcie.txt
 create mode 100644 drivers/pci/host/pci-layerscape.c

diff --git a/Documentation/devicetree/bindings/pci/fsl,ls-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,ls-pcie.txt
new file mode 100644
index 0000000..af2ee1c0
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/fsl,ls-pcie.txt
@@ -0,0 +1,41 @@
+Freescale Layerscape PCIe controller
+
+This PCIe host controller is based on the Synopsis Designware PCIe IP
+and thus inherits all the common properties defined in designware-pcie.txt.
+
+Required properties:
+- compatible: should contain "fsl,ls-pcie", to identify the platform,
+  plus an identifier for specific instance such as "fsl,ls1021a-pcie"
+- reg: base addresses and lengths of the PCIe controller
+- interrupts: A list of interrupt outputs of the controller. Must contain an
+  entry for each entry in the interrupt-names property.
+- interrupt-names: Must include the following entries:
+  "intr": The interrupt that is asserted for controller interrupts
+  "msi": The interrupt that is asserted when an MSI is received
+  "pme": The interrupt that is asserted when PME state changes
+
+Example:
+
+	pcie at 3400000 {
+		compatible = "fsl,ls1021a-pcie", "fsl,ls-pcie", "snps,dw-pcie";
+		reg = <0x00 0x03400000 0x0 0x00010000   /* controller registers */
+		       0x40 0x00000000 0x0 0x00080000>; /* configuration space */
+		reg-names = "regs", "config";
+		interrupts = <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH>, /* controller interrupt */
+			     <GIC_SPI 179 IRQ_TYPE_LEVEL_HIGH>, /* MSI interrupt */
+			     <GIC_SPI 181 IRQ_TYPE_LEVEL_HIGH>; /* PME interrupt */
+		interrupt-names = "intr", "msi", "pme";
+		#address-cells = <3>;
+		#size-cells = <2>;
+		device_type = "pci";
+		num-lanes = <4>;
+		bus-range = <0x0 0xff>;
+		ranges = <0x81000000 0x0 0x00000000 0x40 0x10000000 0x0 0x00010000   /* downstream I/O */
+			  0x82000000 0x0 0x00000000 0x41 0x00000000 0x1 0x00000000>; /* non-prefetchable memory */
+		#interrupt-cells = <1>;
+		interrupt-map-mask = <0 0 0 7>;
+		interrupt-map = <0000 0 0 1 &gic GIC_SPI 91  IRQ_TYPE_LEVEL_HIGH>,
+				<0000 0 0 2 &gic GIC_SPI 188 IRQ_TYPE_LEVEL_HIGH>,
+				<0000 0 0 3 &gic GIC_SPI 190 IRQ_TYPE_LEVEL_HIGH>,
+				<0000 0 0 4 &gic GIC_SPI 192 IRQ_TYPE_LEVEL_HIGH>;
+	};
diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index 8922c37..e0d7bbf 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -63,4 +63,11 @@ config PCIE_SPEAR13XX
 	help
 	  Say Y here if you want PCIe support on SPEAr13XX SoCs.
 
+config PCI_LAYERSCAPE
+	bool "Freescale Layerscape PCIe controller"
+	depends on SOC_LS1021A
+	select PCIE_DW
+	help
+	  Say Y here if you want PCIe controller support on Layerscape SoCs.
+
 endmenu
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index d0e88f1..54cd9b2 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -8,3 +8,4 @@ obj-$(CONFIG_PCI_RCAR_GEN2) += pci-rcar-gen2.o
 obj-$(CONFIG_PCI_RCAR_GEN2_PCIE) += pcie-rcar.o
 obj-$(CONFIG_PCI_HOST_GENERIC) += pci-host-generic.o
 obj-$(CONFIG_PCIE_SPEAR13XX) += pcie-spear13xx.o
+obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
diff --git a/drivers/pci/host/pci-layerscape.c b/drivers/pci/host/pci-layerscape.c
new file mode 100644
index 0000000..077a20b
--- /dev/null
+++ b/drivers/pci/host/pci-layerscape.c
@@ -0,0 +1,315 @@
+/*
+ * PCIe host controller driver for Freescale Layerscape SoCs
+ *
+ * Copyright (C) 2014 Freescale Semiconductor.
+ *
+  * Author: Minghuan Lian <Minghuan.Lian@freescale.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/of_pci.h>
+#include <linux/of_platform.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/resource.h>
+
+#include "pcie-designware.h"
+
+/* PEX1/2 Misc Ports Status Register */
+#define PEXMSCPORTSR(pex_idx)	(0x94 + (pex_idx) * 4)
+#define LTSSM_STATE_SHIFT	20
+#define LTSSM_STATE_MASK	0x3f
+#define LTSSM_PCIE_L0		0x11 /* L0 state */
+
+#define SCFG_SPIMSICR_OFF	0x40
+#define SCFG_SPIMSICR_ADDR	0x1570040
+#define SCFG_SPIMSICLRCR_OFF	0x90
+#define SCFG_SCFGREVCR_OFF	0x200
+#define SCFG_BIT_REVERSE	0xFFFFFFFF
+#define SCFG_NO_BIT_REVERSE	0x0
+
+#define MSI_LS1021A_DATA(pex_idx)	(0xb3 + pex_idx)
+
+/* Symbol Timer Register and Filter Mask Register 1 */
+#define PCIE_STRFMR1 0x71c
+
+#define PCIE_LS1021A_BASE	0x3400000
+#define PCIE_LS1021A_REG_SIZE	0x0100000
+
+struct ls_pcie {
+	struct list_head node;
+	struct device *dev;
+	struct pci_bus *bus;
+	void __iomem *dbi;
+	void __iomem *scfg;
+	struct pcie_port pp;
+	int id;
+	int index;
+	int irq;
+	int msi_irq;
+	int pme_irq;
+};
+
+static LIST_HEAD(ls_pcie_list);
+
+#define to_ls_pcie(x)	container_of(x, struct ls_pcie, pp)
+
+static int ls_pcie_link_up(struct pcie_port *pp)
+{
+	u32 rc, tmp;
+	struct ls_pcie *pcie = to_ls_pcie(pp);
+
+	tmp = ioread32(pcie->scfg + SCFG_SCFGREVCR_OFF);
+	iowrite32(SCFG_NO_BIT_REVERSE, pcie->scfg + SCFG_SCFGREVCR_OFF);
+
+	rc = (ioread32(pcie->scfg + PEXMSCPORTSR(pcie->index)) >>
+	     LTSSM_STATE_SHIFT) & LTSSM_STATE_MASK;
+
+	iowrite32(tmp, pcie->scfg + SCFG_SCFGREVCR_OFF);
+
+	if (rc < LTSSM_PCIE_L0)
+		return 0;
+
+	return 1;
+}
+
+static u32 ls_pcie_get_msi_addr(struct pcie_port *pp)
+{
+	return SCFG_SPIMSICR_ADDR;
+}
+
+static u32 ls_pcie_get_msi_data(struct pcie_port *pp, int pos)
+{
+	struct ls_pcie *pcie = to_ls_pcie(pp);
+
+	if (pcie->id == PCI_DEVICE_ID_LS1021A)
+		return MSI_LS1021A_DATA(pcie->index);
+
+	return pos;
+}
+
+static irqreturn_t ls_pcie_msi_irq_handler(int irq, void *data)
+{
+	struct pcie_port *pp = data;
+	struct ls_pcie *pcie = to_ls_pcie(pp);
+	unsigned int msi_irq, processed = 0;
+
+	if (pcie->id == PCI_DEVICE_ID_LS1021A) {
+		/* clear the interrupt */
+		iowrite32(MSI_LS1021A_DATA(pcie->index),
+			  pcie->scfg + SCFG_SPIMSICLRCR_OFF);
+
+		msi_irq = irq_find_mapping(pp->irq_domain, 0);
+		if (msi_irq)
+			generic_handle_irq(msi_irq);
+		else
+			/*
+			 * that's weird who triggered this?
+			 * just clear it
+			 */
+			dev_info(pcie->dev, "unexpected MSI\n");
+
+		processed++;
+	}
+
+	return processed > 0 ? IRQ_HANDLED : IRQ_NONE;
+}
+
+static void ls_pcie_msi_clear_irq(struct pcie_port *pp, int irq)
+{
+}
+
+static void ls_pcie_msi_set_irq(struct pcie_port *pp, int irq)
+{
+	struct ls_pcie *pcie = to_ls_pcie(pp);
+
+	/* MSI needs to set SCFG bit reverse */
+	iowrite32(SCFG_BIT_REVERSE, pcie->scfg + SCFG_SCFGREVCR_OFF);
+}
+
+static void ls_pcie_msi_fixup(struct pcie_port *pp)
+{
+	struct ls_pcie *pcie = to_ls_pcie(pp);
+	int i;
+
+	if (pcie->id != PCI_DEVICE_ID_LS1021A)
+		return;
+
+	/**
+	 * LS1021A has only one MSI interrupt
+	 * Set all msi interrupts as used except the first one
+	 */
+	for (i = 1; i < MAX_MSI_IRQS; i++)
+		set_bit(i, pp->msi_irq_in_use);
+}
+
+static void ls_pcie_host_init(struct pcie_port *pp)
+{
+	struct ls_pcie *pcie = to_ls_pcie(pp);
+	int count = 0;
+	u32 val;
+
+	dw_pcie_setup_rc(pp);
+
+	while (!ls_pcie_link_up(pp)) {
+		usleep_range(100, 1000);
+		count++;
+		if (count >= 200) {
+			dev_err(pp->dev, "phy link never came up\n");
+			return;
+		}
+	}
+
+	/* Workaround for internal TKT228622 to fix the INTx hang issue */
+	val = ioread32(pcie->dbi + PCIE_STRFMR1);
+	val &= 0xffff;
+	iowrite32(val, pcie->dbi + PCIE_STRFMR1);
+
+	ls_pcie_msi_fixup(pp);
+}
+
+static struct pcie_host_ops ls_pcie_host_ops = {
+	.link_up = ls_pcie_link_up,
+	.host_init = ls_pcie_host_init,
+	.msi_set_irq = ls_pcie_msi_set_irq,
+	.msi_clear_irq = ls_pcie_msi_clear_irq,
+	.get_msi_addr = ls_pcie_get_msi_addr,
+	.get_msi_data = ls_pcie_get_msi_data,
+};
+
+static int ls_add_pcie_port(struct ls_pcie *pcie)
+{
+	struct pcie_port *pp;
+	int ret;
+
+	if (!pcie)
+		return -EINVAL;
+
+	pp = &pcie->pp;
+	pp->dev = pcie->dev;
+	pp->dbi_base = pcie->dbi;
+	pp->msi_irq = pcie->msi_irq;
+
+	if (IS_ENABLED(CONFIG_PCI_MSI)) {
+		ret = devm_request_irq(pp->dev, pp->msi_irq,
+					ls_pcie_msi_irq_handler,
+					IRQF_SHARED, "ls-pcie-msi", pp);
+		if (ret) {
+			dev_err(pp->dev, "failed to request msi irq\n");
+			return ret;
+		}
+	}
+
+	pp->root_bus_nr = -1;
+	pp->ops = &ls_pcie_host_ops;
+
+	ret = dw_pcie_host_init(pp);
+	if (ret) {
+		dev_err(pp->dev, "failed to initialize host\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int __init ls_pcie_probe(struct platform_device *pdev)
+{
+	struct ls_pcie *pcie;
+	struct resource *dbi_base;
+	struct device_node *np;
+	int ret;
+
+	pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
+	if (!pcie)
+		return -ENOMEM;
+
+	pcie->dev = &pdev->dev;
+
+	dbi_base = platform_get_resource_byname(pdev, IORESOURCE_MEM, "regs");
+	if (!dbi_base) {
+		dev_err(&pdev->dev, "missing *regs* space\n");
+		return -ENODEV;
+	}
+
+	pcie->dbi = devm_ioremap_resource(&pdev->dev, dbi_base);
+	if (IS_ERR(pcie->dbi))
+		return PTR_ERR(pcie->dbi);
+
+	if (of_device_is_compatible(pdev->dev.of_node, "fsl,ls1021a-pcie")) {
+		pcie->id = PCI_DEVICE_ID_LS1021A;
+		pcie->index = (dbi_base->start - PCIE_LS1021A_BASE) /
+			      PCIE_LS1021A_REG_SIZE;
+
+		/* map SCFG register */
+		np = of_find_compatible_node(NULL, NULL, "fsl,ls1021a-scfg");
+		pcie->scfg = of_iomap(np, 0);
+		if (!pcie->scfg) {
+			dev_err(&pdev->dev, "unable to find SCFG registers\n");
+			return PTR_ERR(pcie->scfg);
+		}
+	}
+
+	/* request interrupt */
+	pcie->irq = platform_get_irq_byname(pdev, "intr");
+	if (pcie->irq < 0) {
+		dev_err(&pdev->dev, "failed to get IRQ: %d\n", pcie->irq);
+		return pcie->irq;
+	}
+
+	pcie->msi_irq = platform_get_irq_byname(pdev, "msi");
+	if (pcie->msi_irq < 0) {
+		dev_err(&pdev->dev,
+			"failed to get MSI IRQ: %d\n", pcie->msi_irq);
+		return pcie->msi_irq;
+	}
+
+	pcie->pme_irq = platform_get_irq_byname(pdev, "pme");
+	if (pcie->pme_irq < 0) {
+		dev_err(&pdev->dev,
+			"failed to get PME IRQ: %d\n", pcie->pme_irq);
+		return pcie->pme_irq;
+	}
+
+	ret = ls_add_pcie_port(pcie);
+	if (ret < 0)
+		return ret;
+
+	platform_set_drvdata(pdev, pcie);
+	list_add(&pcie->node, &ls_pcie_list);
+
+	return 0;
+}
+
+static const struct of_device_id ls_pcie_of_match[] = {
+	{ .compatible = "fsl,ls-pcie" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, ls_pcie_of_match);
+
+static struct platform_driver ls_pcie_driver = {
+	.driver = {
+		.name = "layerscape-pcie",
+		.owner = THIS_MODULE,
+		.of_match_table = ls_pcie_of_match,
+	},
+};
+
+/* Freescale PCIe driver does not allow module unload */
+static int __init ls_pcie_init(void)
+{
+	return platform_driver_probe(&ls_pcie_driver, ls_pcie_probe);
+}
+subsys_initcall(ls_pcie_init);
+
+MODULE_AUTHOR("Minghuan Lian <Minghuan.Lian@freescale.com>");
+MODULE_DESCRIPTION("Freescale Layerscape PCIe host controller driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 6ed0bb7..26d4c30 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2442,6 +2442,7 @@
 #define PCI_DEVICE_ID_P5020		0x0421
 #define PCI_DEVICE_ID_P5010E		0x0428
 #define PCI_DEVICE_ID_P5010		0x0429
+#define PCI_DEVICE_ID_LS1021A		0x0e0b
 #define PCI_DEVICE_ID_MPC8641		0x7010
 #define PCI_DEVICE_ID_MPC8641D		0x7011
 #define PCI_DEVICE_ID_MPC8610		0x7018
-- 
1.9.1

Re: [PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Arnd Bergmann]

On Thursday 04 September 2014 18:45:38 Minghuan Lian wrote:

> +
> +#define PCIE_LS1021A_BASE	0x3400000
> +#define PCIE_LS1021A_REG_SIZE	0x0100000

This does not belong here. All addresses must come from DT,
and you should not do the calculation below based on the address.

> +struct ls_pcie {
> +	struct list_head node;
> +	struct device *dev;
> +	struct pci_bus *bus;
> +	void __iomem *dbi;
> +	void __iomem *scfg;
> +	struct pcie_port pp;
> +	int id;
> +	int index;
> +	int irq;
> +	int msi_irq;
> +	int pme_irq;
> +};

irq and pme_irq seem to be completely unused, so better
not add them until they are actually used.

The scfg registers seem to belong to another device that
is responsible for multiple instances. Unfortunately,
this "fsl,ls1021a-scfg" device is not documented anywhere
that I can see.

Is this some sort of syscon node, or is it specific to the
pcie controller(s)?

> +static LIST_HEAD(ls_pcie_list);

Don't maintain your own lists please.

> +static int ls_pcie_link_up(struct pcie_port *pp)
> +{
> +	u32 rc, tmp;
> +	struct ls_pcie *pcie = to_ls_pcie(pp);
> +
> +	tmp = ioread32(pcie->scfg + SCFG_SCFGREVCR_OFF);
> +	iowrite32(SCFG_NO_BIT_REVERSE, pcie->scfg + SCFG_SCFGREVCR_OFF);
> +
> +	rc = (ioread32(pcie->scfg + PEXMSCPORTSR(pcie->index)) >>
> +	     LTSSM_STATE_SHIFT) & LTSSM_STATE_MASK;
> +
> +	iowrite32(tmp, pcie->scfg + SCFG_SCFGREVCR_OFF);
> +
> +	if (rc < LTSSM_PCIE_L0)
> +		return 0;
> +
> +	return 1;
> +}

This looks like it is really a phy driver, although a pretty minimal
one.

> +static u32 ls_pcie_get_msi_addr(struct pcie_port *pp)
> +{
> +	return SCFG_SPIMSICR_ADDR;
> +}
> +
> +static u32 ls_pcie_get_msi_data(struct pcie_port *pp, int pos)
> +{
> +	struct ls_pcie *pcie = to_ls_pcie(pp);
> +
> +	if (pcie->id == PCI_DEVICE_ID_LS1021A)
> +		return MSI_LS1021A_DATA(pcie->index);
> +
> +	return pos;
> +}

My impression is that you have two distinct MSI controller
implementations, one for LS1021A and the other one for everything
else. How about using separate pcie_host_ops for them, possibly
also moving them into separate files?

A good way to deal with this is to put the pointers to the two
pcie_host_ops into the data fields of the ls_pcie_of_match table.

> +/* Freescale PCIe driver does not allow module unload */
> +static int __init ls_pcie_init(void)
> +{
> +	return platform_driver_probe(&ls_pcie_driver, ls_pcie_probe);
> +}
> +subsys_initcall(ls_pcie_init);

Better change to platform_driver_register, so you can use deferred probing.

	Arnd

[PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Arnd Bergmann]

On Thursday 04 September 2014 18:45:38 Minghuan Lian wrote:

> +
> +#define PCIE_LS1021A_BASE	0x3400000
> +#define PCIE_LS1021A_REG_SIZE	0x0100000

This does not belong here. All addresses must come from DT,
and you should not do the calculation below based on the address.

> +struct ls_pcie {
> +	struct list_head node;
> +	struct device *dev;
> +	struct pci_bus *bus;
> +	void __iomem *dbi;
> +	void __iomem *scfg;
> +	struct pcie_port pp;
> +	int id;
> +	int index;
> +	int irq;
> +	int msi_irq;
> +	int pme_irq;
> +};

irq and pme_irq seem to be completely unused, so better
not add them until they are actually used.

The scfg registers seem to belong to another device that
is responsible for multiple instances. Unfortunately,
this "fsl,ls1021a-scfg" device is not documented anywhere
that I can see.

Is this some sort of syscon node, or is it specific to the
pcie controller(s)?

> +static LIST_HEAD(ls_pcie_list);

Don't maintain your own lists please.

> +static int ls_pcie_link_up(struct pcie_port *pp)
> +{
> +	u32 rc, tmp;
> +	struct ls_pcie *pcie = to_ls_pcie(pp);
> +
> +	tmp = ioread32(pcie->scfg + SCFG_SCFGREVCR_OFF);
> +	iowrite32(SCFG_NO_BIT_REVERSE, pcie->scfg + SCFG_SCFGREVCR_OFF);
> +
> +	rc = (ioread32(pcie->scfg + PEXMSCPORTSR(pcie->index)) >>
> +	     LTSSM_STATE_SHIFT) & LTSSM_STATE_MASK;
> +
> +	iowrite32(tmp, pcie->scfg + SCFG_SCFGREVCR_OFF);
> +
> +	if (rc < LTSSM_PCIE_L0)
> +		return 0;
> +
> +	return 1;
> +}

This looks like it is really a phy driver, although a pretty minimal
one.

> +static u32 ls_pcie_get_msi_addr(struct pcie_port *pp)
> +{
> +	return SCFG_SPIMSICR_ADDR;
> +}
> +
> +static u32 ls_pcie_get_msi_data(struct pcie_port *pp, int pos)
> +{
> +	struct ls_pcie *pcie = to_ls_pcie(pp);
> +
> +	if (pcie->id == PCI_DEVICE_ID_LS1021A)
> +		return MSI_LS1021A_DATA(pcie->index);
> +
> +	return pos;
> +}

My impression is that you have two distinct MSI controller
implementations, one for LS1021A and the other one for everything
else. How about using separate pcie_host_ops for them, possibly
also moving them into separate files?

A good way to deal with this is to put the pointers to the two
pcie_host_ops into the data fields of the ls_pcie_of_match table.

> +/* Freescale PCIe driver does not allow module unload */
> +static int __init ls_pcie_init(void)
> +{
> +	return platform_driver_probe(&ls_pcie_driver, ls_pcie_probe);
> +}
> +subsys_initcall(ls_pcie_init);

Better change to platform_driver_register, so you can use deferred probing.

	Arnd

Re: [PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Arnd Bergmann]

On Thursday 04 September 2014 14:14:48 Arnd Bergmann wrote:
> 
> My impression is that you have two distinct MSI controller
> implementations, one for LS1021A and the other one for everything
> else. How about using separate pcie_host_ops for them, possibly
> also moving them into separate files?
> 

One more thing: you should really use the msi-parent property to
find the MSI controller: Sooner or later this PCI block is likely
to end up in a real product that has GICv2m or GICv3 support, so
you will have to pick which of the two MSI controllers to use.

	Arnd

[PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Arnd Bergmann]

On Thursday 04 September 2014 14:14:48 Arnd Bergmann wrote:
> 
> My impression is that you have two distinct MSI controller
> implementations, one for LS1021A and the other one for everything
> else. How about using separate pcie_host_ops for them, possibly
> also moving them into separate files?
> 

One more thing: you should really use the msi-parent property to
find the MSI controller: Sooner or later this PCI block is likely
to end up in a real product that has GICv2m or GICv3 support, so
you will have to pick which of the two MSI controllers to use.

	Arnd

Re: [PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Arnd Bergmann]

On Thursday 04 September 2014 15:51:22 Arnd Bergmann wrote:
> On Thursday 04 September 2014 14:14:48 Arnd Bergmann wrote:
> > 
> > My impression is that you have two distinct MSI controller
> > implementations, one for LS1021A and the other one for everything
> > else. How about using separate pcie_host_ops for them, possibly
> > also moving them into separate files?
> > 
> 
> One more thing: you should really use the msi-parent property to
> find the MSI controller: Sooner or later this PCI block is likely
> to end up in a real product that has GICv2m or GICv3 support, so
> you will have to pick which of the two MSI controllers to use.

Ah, I missed the fact that LS1021A is a real product already, I was
confusing it with the LS2085A patches that are also on the list
at the moment and that only work in a simulator at present.

Note that LS2085A does have GICv3. I don't know if it has a similar
PCIe implementation, but if it does, the DT representation should
definitely provide a way to pick the MSI controller (you will always
want the GIC in practice).

	Arnd

[PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Arnd Bergmann]

On Thursday 04 September 2014 15:51:22 Arnd Bergmann wrote:
> On Thursday 04 September 2014 14:14:48 Arnd Bergmann wrote:
> > 
> > My impression is that you have two distinct MSI controller
> > implementations, one for LS1021A and the other one for everything
> > else. How about using separate pcie_host_ops for them, possibly
> > also moving them into separate files?
> > 
> 
> One more thing: you should really use the msi-parent property to
> find the MSI controller: Sooner or later this PCI block is likely
> to end up in a real product that has GICv2m or GICv3 support, so
> you will have to pick which of the two MSI controllers to use.

Ah, I missed the fact that LS1021A is a real product already, I was
confusing it with the LS2085A patches that are also on the list
at the moment and that only work in a simulator at present.

Note that LS2085A does have GICv3. I don't know if it has a similar
PCIe implementation, but if it does, the DT representation should
definitely provide a way to pick the MSI controller (you will always
want the GIC in practice).

	Arnd

答复: [PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Minghuan.Lian at freescale.com]

Hi Arnd,

Please see my comments inline.
________________________________________
???: Arnd Bergmann <arnd@arndb.de>
????: 2014?9?4? 20:14
???: linux-arm-kernel at lists.infradead.org
??: Lian Minghuan-B31939; linux-pci at vger.kernel.org; Hu Mingkai-B21284; Zang Roy-R61911
??: Re: [PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

On Thursday 04 September 2014 18:45:38 Minghuan Lian wrote:

> +
> +#define PCIE_LS1021A_BASE    0x3400000
> +#define PCIE_LS1021A_REG_SIZE        0x0100000

This does not belong here. All addresses must come from DT,
and you should not do the calculation below based on the address.

[Minghuan] LS1021A contains two PCI controllers. I use them to calculation the PCI controller index.
There are several separate registers for PEX1 and PEX2 in SCFG register space. It is hard to get them address from DTS. Could you tell me a way to get PCI controller index?


> +struct ls_pcie {
> +     struct list_head node;
> +     struct device *dev;
> +     struct pci_bus *bus;
> +     void __iomem *dbi;
> +     void __iomem *scfg;
> +     struct pcie_port pp;
> +     int id;
> +     int index;
> +     int irq;
> +     int msi_irq;
> +     int pme_irq;
> +};

irq and pme_irq seem to be completely unused, so better
not add them until they are actually used.
[Minghuan] Ok, I will remove them.

The scfg registers seem to belong to another device that
is responsible for multiple instances. Unfortunately,
this "fsl,ls1021a-scfg" device is not documented anywhere
that I can see.

Is this some sort of syscon node, or is it specific to the
pcie controller(s)?

[Minghaun] SCFG provides SoC specific configuration and status
registers for the device including PCI USB eTSEC SATA ...
The platform patches that contains SCFG code are being upstream.


> +static LIST_HEAD(ls_pcie_list);

Don't maintain your own lists please.

[Minghuan] Ok, I will remove it.

> +static int ls_pcie_link_up(struct pcie_port *pp)
> +{
> +     u32 rc, tmp;
> +     struct ls_pcie *pcie = to_ls_pcie(pp);
> +
> +     tmp = ioread32(pcie->scfg + SCFG_SCFGREVCR_OFF);
> +     iowrite32(SCFG_NO_BIT_REVERSE, pcie->scfg + SCFG_SCFGREVCR_OFF);
> +
> +     rc = (ioread32(pcie->scfg + PEXMSCPORTSR(pcie->index)) >>
> +          LTSSM_STATE_SHIFT) & LTSSM_STATE_MASK;
> +
> +     iowrite32(tmp, pcie->scfg + SCFG_SCFGREVCR_OFF);
> +
> +     if (rc < LTSSM_PCIE_L0)
> +             return 0;
> +
> +     return 1;
> +}

This looks like it is really a phy driver, although a pretty minimal
one.

[Minghuan] I read SCFG register. SCFG provides SoC specific configuration and status
registers for the device including PCI USB eTSEC SATA ...

> +static u32 ls_pcie_get_msi_addr(struct pcie_port *pp)
> +{
> +     return SCFG_SPIMSICR_ADDR;
> +}
> +
> +static u32 ls_pcie_get_msi_data(struct pcie_port *pp, int pos)
> +{
> +     struct ls_pcie *pcie = to_ls_pcie(pp);
> +
> +     if (pcie->id == PCI_DEVICE_ID_LS1021A)
> +             return MSI_LS1021A_DATA(pcie->index);
> +
> +     return pos;
> +}

My impression is that you have two distinct MSI controller
implementations, one for LS1021A and the other one for everything
else. How about using separate pcie_host_ops for them, possibly
also moving them into separate files?

A good way to deal with this is to put the pointers to the two
pcie_host_ops into the data fields of the ls_pcie_of_match table.

[Minghuan] LS1021 contains the same two PCI controllers PEX1 and PEX2.
both PEX1 and PEX use the same MSI address, but different MSI message data.

> +/* Freescale PCIe driver does not allow module unload */
> +static int __init ls_pcie_init(void)
> +{
> +     return platform_driver_probe(&ls_pcie_driver, ls_pcie_probe);
> +}
> +subsys_initcall(ls_pcie_init);

Better change to platform_driver_register, so you can use deferred probing.
[Minghuan] I will try.
        Arnd

Re: 答复: [PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Arnd Bergmann]

On Friday 05 September 2014 07:22:08 Minghuan.Lian@freescale.com wrote:
> Hi Arnd,
> 
> Please see my comments inline.

Please use the usual reply style in the future, using '>' characters to
properly give attribution. I'm fixing this up manually in my reply
here, so others can follow the discussion.

> > On Thursday 04 September 2014 18:45:38 Minghuan Lian wrote:
> > > +
> > > +#define PCIE_LS1021A_BASE    0x3400000
> > > +#define PCIE_LS1021A_REG_SIZE        0x0100000
> > 
> > This does not belong here. All addresses must come from DT,
> > and you should not do the calculation below based on the address.
> [Minghuan] LS1021A contains two PCI controllers. I use them to 
> calculation the PCI controller index.
> There are several separate registers for PEX1 and PEX2 in SCFG
> register space. It is hard to get them address from DTS. Could
> you tell me a way to get PCI controller index?

The easiest way would be to put the index as a property in the
device tree itself.

> > +struct ls_pcie {
> > +     struct list_head node;
> > +     struct device *dev;
> > +     struct pci_bus *bus;
> > +     void __iomem *dbi;
> > +     void __iomem *scfg;
> > +     struct pcie_port pp;
> > +     int id;
> > +     int index;
> > +     int irq;
> > +     int msi_irq;
> > +     int pme_irq;
> > +};
> 
> irq and pme_irq seem to be completely unused, so better
> not add them until they are actually used.
> [Minghuan] Ok, I will remove them.
> 
> The scfg registers seem to belong to another device that
> is responsible for multiple instances. Unfortunately,
> this "fsl,ls1021a-scfg" device is not documented anywhere
> that I can see.
> 
> Is this some sort of syscon node, or is it specific to the
> pcie controller(s)?
> 
> [Minghaun] SCFG provides SoC specific configuration and status
> registers for the device including PCI USB eTSEC SATA ...
> The platform patches that contains SCFG code are being upstream.

This sounds like it should be a syscon node, and you should use
syscon_regmap_lookup_by_phandle() to find the scfg node from each
of those drivers, rather than scanning the DT yourself in each
of the drivers using it.

This can also be a place to put the index, such as 

	scfg = <&scfg 0>;

for the first PCIe node. The probe function then extracts both
the phandle for the syscon and the index from that property.

> > +static int ls_pcie_link_up(struct pcie_port *pp)
> > +{
> > +     u32 rc, tmp;
> > +     struct ls_pcie *pcie = to_ls_pcie(pp);
> > +
> > +     tmp = ioread32(pcie->scfg + SCFG_SCFGREVCR_OFF);
> > +     iowrite32(SCFG_NO_BIT_REVERSE, pcie->scfg + SCFG_SCFGREVCR_OFF);
> > +
> > +     rc = (ioread32(pcie->scfg + PEXMSCPORTSR(pcie->index)) >>
> > +          LTSSM_STATE_SHIFT) & LTSSM_STATE_MASK;
> > +
> > +     iowrite32(tmp, pcie->scfg + SCFG_SCFGREVCR_OFF);
> > +
> > +     if (rc < LTSSM_PCIE_L0)
> > +             return 0;
> > +
> > +     return 1;
> > +}
> 
> This looks like it is really a phy driver, although a pretty minimal
> one.
> 
> [Minghuan] I read SCFG register. SCFG provides SoC specific configuration
> and status registers for the device including PCI USB eTSEC SATA ...

I'm guessing that a lot of that is phy related information though.
A nicer way to deal with this, compared to using syscon directly is
to have a phy driver for your chip, and have that deal with all
the phy related setup. In this case you would have a reference in
the client driver like

	phys = <&scfgphy LS1021A_PHY_PCIE0>;
	phy-names = "pcie";

And in the pcie driver you just call devm_phy_get(dev, "pcie") to get a reference
to that phy node, and then you can use the phy API to perform actions on
it (init, power_on, power_off, exit).

This keeps all knowledge about the phy registers inside of the scfg area
in one place, and you only have to add a new phy driver for a future SoC
that has the same PCIe core but different scfg.

> > +static u32 ls_pcie_get_msi_addr(struct pcie_port *pp)
> > +{
> > +     return SCFG_SPIMSICR_ADDR;
> > +}
> > +
> > +static u32 ls_pcie_get_msi_data(struct pcie_port *pp, int pos)
> > +{
> > +     struct ls_pcie *pcie = to_ls_pcie(pp);
> > +
> > +     if (pcie->id == PCI_DEVICE_ID_LS1021A)
> > +             return MSI_LS1021A_DATA(pcie->index);
> > +
> > +     return pos;
> > +}
> 
> My impression is that you have two distinct MSI controller
> implementations, one for LS1021A and the other one for everything
> else. How about using separate pcie_host_ops for them, possibly
> also moving them into separate files?
> 
> A good way to deal with this is to put the pointers to the two
> pcie_host_ops into the data fields of the ls_pcie_of_match table.
> 
> [Minghuan] LS1021 contains the same two PCI controllers PEX1 and PEX2.
> both PEX1 and PEX use the same MSI address, but different MSI message data.

I mean LS1021 is different from other chips here, since you compare
the pcie->id value.

	Arnd

答复: [PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Arnd Bergmann]

On Friday 05 September 2014 07:22:08 Minghuan.Lian at freescale.com wrote:
> Hi Arnd,
> 
> Please see my comments inline.

Please use the usual reply style in the future, using '>' characters to
properly give attribution. I'm fixing this up manually in my reply
here, so others can follow the discussion.

> > On Thursday 04 September 2014 18:45:38 Minghuan Lian wrote:
> > > +
> > > +#define PCIE_LS1021A_BASE    0x3400000
> > > +#define PCIE_LS1021A_REG_SIZE        0x0100000
> > 
> > This does not belong here. All addresses must come from DT,
> > and you should not do the calculation below based on the address.
> [Minghuan] LS1021A contains two PCI controllers. I use them to 
> calculation the PCI controller index.
> There are several separate registers for PEX1 and PEX2 in SCFG
> register space. It is hard to get them address from DTS. Could
> you tell me a way to get PCI controller index?

The easiest way would be to put the index as a property in the
device tree itself.

> > +struct ls_pcie {
> > +     struct list_head node;
> > +     struct device *dev;
> > +     struct pci_bus *bus;
> > +     void __iomem *dbi;
> > +     void __iomem *scfg;
> > +     struct pcie_port pp;
> > +     int id;
> > +     int index;
> > +     int irq;
> > +     int msi_irq;
> > +     int pme_irq;
> > +};
> 
> irq and pme_irq seem to be completely unused, so better
> not add them until they are actually used.
> [Minghuan] Ok, I will remove them.
> 
> The scfg registers seem to belong to another device that
> is responsible for multiple instances. Unfortunately,
> this "fsl,ls1021a-scfg" device is not documented anywhere
> that I can see.
> 
> Is this some sort of syscon node, or is it specific to the
> pcie controller(s)?
> 
> [Minghaun] SCFG provides SoC specific configuration and status
> registers for the device including PCI USB eTSEC SATA ...
> The platform patches that contains SCFG code are being upstream.

This sounds like it should be a syscon node, and you should use
syscon_regmap_lookup_by_phandle() to find the scfg node from each
of those drivers, rather than scanning the DT yourself in each
of the drivers using it.

This can also be a place to put the index, such as 

	scfg = <&scfg 0>;

for the first PCIe node. The probe function then extracts both
the phandle for the syscon and the index from that property.

> > +static int ls_pcie_link_up(struct pcie_port *pp)
> > +{
> > +     u32 rc, tmp;
> > +     struct ls_pcie *pcie = to_ls_pcie(pp);
> > +
> > +     tmp = ioread32(pcie->scfg + SCFG_SCFGREVCR_OFF);
> > +     iowrite32(SCFG_NO_BIT_REVERSE, pcie->scfg + SCFG_SCFGREVCR_OFF);
> > +
> > +     rc = (ioread32(pcie->scfg + PEXMSCPORTSR(pcie->index)) >>
> > +          LTSSM_STATE_SHIFT) & LTSSM_STATE_MASK;
> > +
> > +     iowrite32(tmp, pcie->scfg + SCFG_SCFGREVCR_OFF);
> > +
> > +     if (rc < LTSSM_PCIE_L0)
> > +             return 0;
> > +
> > +     return 1;
> > +}
> 
> This looks like it is really a phy driver, although a pretty minimal
> one.
> 
> [Minghuan] I read SCFG register. SCFG provides SoC specific configuration
> and status registers for the device including PCI USB eTSEC SATA ...

I'm guessing that a lot of that is phy related information though.
A nicer way to deal with this, compared to using syscon directly is
to have a phy driver for your chip, and have that deal with all
the phy related setup. In this case you would have a reference in
the client driver like

	phys = <&scfgphy LS1021A_PHY_PCIE0>;
	phy-names = "pcie";

And in the pcie driver you just call devm_phy_get(dev, "pcie") to get a reference
to that phy node, and then you can use the phy API to perform actions on
it (init, power_on, power_off, exit).

This keeps all knowledge about the phy registers inside of the scfg area
in one place, and you only have to add a new phy driver for a future SoC
that has the same PCIe core but different scfg.

> > +static u32 ls_pcie_get_msi_addr(struct pcie_port *pp)
> > +{
> > +     return SCFG_SPIMSICR_ADDR;
> > +}
> > +
> > +static u32 ls_pcie_get_msi_data(struct pcie_port *pp, int pos)
> > +{
> > +     struct ls_pcie *pcie = to_ls_pcie(pp);
> > +
> > +     if (pcie->id == PCI_DEVICE_ID_LS1021A)
> > +             return MSI_LS1021A_DATA(pcie->index);
> > +
> > +     return pos;
> > +}
> 
> My impression is that you have two distinct MSI controller
> implementations, one for LS1021A and the other one for everything
> else. How about using separate pcie_host_ops for them, possibly
> also moving them into separate files?
> 
> A good way to deal with this is to put the pointers to the two
> pcie_host_ops into the data fields of the ls_pcie_of_match table.
> 
> [Minghuan] LS1021 contains the same two PCI controllers PEX1 and PEX2.
> both PEX1 and PEX use the same MSI address, but different MSI message data.

I mean LS1021 is different from other chips here, since you compare
the pcie->id value.

	Arnd

Re: 答复: [PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Lian Minghuan-B31939]

Hi Arnd,

I am sorry for missed reply style and thank you very much for fixing it.
please see my comments inline.

Thanks,
Minghuan

On 2014年09月05日 08:44, Arnd Bergmann wrote:
> On Friday 05 September 2014 07:22:08 Minghuan.Lian@freescale.com wrote:
>> Hi Arnd,
>>
>> Please see my comments inline.
> Please use the usual reply style in the future, using '>' characters to
> properly give attribution. I'm fixing this up manually in my reply
> here, so others can follow the discussion.
>
>>> On Thursday 04 September 2014 18:45:38 Minghuan Lian wrote:
>>>> +
>>>> +#define PCIE_LS1021A_BASE    0x3400000
>>>> +#define PCIE_LS1021A_REG_SIZE        0x0100000
>>> This does not belong here. All addresses must come from DT,
>>> and you should not do the calculation below based on the address.
>> [Minghuan] LS1021A contains two PCI controllers. I use them to
>> calculation the PCI controller index.
>> There are several separate registers for PEX1 and PEX2 in SCFG
>> register space. It is hard to get them address from DTS. Could
>> you tell me a way to get PCI controller index?
> The easiest way would be to put the index as a property in the
> device tree itself.
[Minghuan] I want to use  scfg = <&scfg 0> you mentioned below.
>>> +struct ls_pcie {
>>> +     struct list_head node;
>>> +     struct device *dev;
>>> +     struct pci_bus *bus;
>>> +     void __iomem *dbi;
>>> +     void __iomem *scfg;
>>> +     struct pcie_port pp;
>>> +     int id;
>>> +     int index;
>>> +     int irq;
>>> +     int msi_irq;
>>> +     int pme_irq;
>>> +};
>> irq and pme_irq seem to be completely unused, so better
>> not add them until they are actually used.
>> [Minghuan] Ok, I will remove them.
>>
>> The scfg registers seem to belong to another device that
>> is responsible for multiple instances. Unfortunately,
>> this "fsl,ls1021a-scfg" device is not documented anywhere
>> that I can see.
>>
>> Is this some sort of syscon node, or is it specific to the
>> pcie controller(s)?
>>
>> [Minghaun] SCFG provides SoC specific configuration and status
>> registers for the device including PCI USB eTSEC SATA ...
>> The platform patches that contains SCFG code are being upstream.
> This sounds like it should be a syscon node, and you should use
> syscon_regmap_lookup_by_phandle() to find the scfg node from each
> of those drivers, rather than scanning the DT yourself in each
> of the drivers using it.
>
> This can also be a place to put the index, such as
>
> 	scfg = <&scfg 0>;
>
> for the first PCIe node. The probe function then extracts both
> the phandle for the syscon and the index from that property.
[Minghuan] I discussed with my colleague. They worry about performance 
degradation if using regmap API,
because there are some fast device use scfg. We tend to use a simple way 
to map andread/write scfg directly.
>>> +static int ls_pcie_link_up(struct pcie_port *pp)
>>> +{
>>> +     u32 rc, tmp;
>>> +     struct ls_pcie *pcie = to_ls_pcie(pp);
>>> +
>>> +     tmp = ioread32(pcie->scfg + SCFG_SCFGREVCR_OFF);
>>> +     iowrite32(SCFG_NO_BIT_REVERSE, pcie->scfg + SCFG_SCFGREVCR_OFF);
>>> +
>>> +     rc = (ioread32(pcie->scfg + PEXMSCPORTSR(pcie->index)) >>
>>> +          LTSSM_STATE_SHIFT) & LTSSM_STATE_MASK;
>>> +
>>> +     iowrite32(tmp, pcie->scfg + SCFG_SCFGREVCR_OFF);
>>> +
>>> +     if (rc < LTSSM_PCIE_L0)
>>> +             return 0;
>>> +
>>> +     return 1;
>>> +}
>> This looks like it is really a phy driver, although a pretty minimal
>> one.
>>
>> [Minghuan] I read SCFG register. SCFG provides SoC specific configuration
>> and status registers for the device including PCI USB eTSEC SATA ...
> I'm guessing that a lot of that is phy related information though.
> A nicer way to deal with this, compared to using syscon directly is
> to have a phy driver for your chip, and have that deal with all
> the phy related setup. In this case you would have a reference in
> the client driver like
>
> 	phys = <&scfgphy LS1021A_PHY_PCIE0>;
> 	phy-names = "pcie";
>
> And in the pcie driver you just call devm_phy_get(dev, "pcie") to get a reference
> to that phy node, and then you can use the phy API to perform actions on
> it (init, power_on, power_off, exit).
>
> This keeps all knowledge about the phy registers inside of the scfg area
> in one place, and you only have to add a new phy driver for a future SoC
> that has the same PCIe core but different scfg.
[Minghuan] SCFG does not contains power_on power_off or other PCI 
control registers.
We only  get link up and MSI related status via SCFG. So I think it is 
not enough to call scfg
PCIe phy.
>>> +static u32 ls_pcie_get_msi_addr(struct pcie_port *pp)
>>> +{
>>> +     return SCFG_SPIMSICR_ADDR;
>>> +}
>>> +
>>> +static u32 ls_pcie_get_msi_data(struct pcie_port *pp, int pos)
>>> +{
>>> +     struct ls_pcie *pcie = to_ls_pcie(pp);
>>> +
>>> +     if (pcie->id == PCI_DEVICE_ID_LS1021A)
>>> +             return MSI_LS1021A_DATA(pcie->index);
>>> +
>>> +     return pos;
>>> +}
>> My impression is that you have two distinct MSI controller
>> implementations, one for LS1021A and the other one for everything
>> else. How about using separate pcie_host_ops for them, possibly
>> also moving them into separate files?
>>
>> A good way to deal with this is to put the pointers to the two
>> pcie_host_ops into the data fields of the ls_pcie_of_match table.
>>
>> [Minghuan] LS1021 contains the same two PCI controllers PEX1 and PEX2.
>> both PEX1 and PEX use the same MSI address, but different MSI message data.
> I mean LS1021 is different from other chips here, since you compare
> the pcie->id value.
[Minghuan] I see. I will change it.
> 	Arnd

答复: [PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Lian Minghuan-B31939]

Hi Arnd,

I am sorry for missed reply style and thank you very much for fixing it.
please see my comments inline.

Thanks,
Minghuan

On 2014?09?05? 08:44, Arnd Bergmann wrote:
> On Friday 05 September 2014 07:22:08 Minghuan.Lian at freescale.com wrote:
>> Hi Arnd,
>>
>> Please see my comments inline.
> Please use the usual reply style in the future, using '>' characters to
> properly give attribution. I'm fixing this up manually in my reply
> here, so others can follow the discussion.
>
>>> On Thursday 04 September 2014 18:45:38 Minghuan Lian wrote:
>>>> +
>>>> +#define PCIE_LS1021A_BASE    0x3400000
>>>> +#define PCIE_LS1021A_REG_SIZE        0x0100000
>>> This does not belong here. All addresses must come from DT,
>>> and you should not do the calculation below based on the address.
>> [Minghuan] LS1021A contains two PCI controllers. I use them to
>> calculation the PCI controller index.
>> There are several separate registers for PEX1 and PEX2 in SCFG
>> register space. It is hard to get them address from DTS. Could
>> you tell me a way to get PCI controller index?
> The easiest way would be to put the index as a property in the
> device tree itself.
[Minghuan] I want to use  scfg = <&scfg 0> you mentioned below.
>>> +struct ls_pcie {
>>> +     struct list_head node;
>>> +     struct device *dev;
>>> +     struct pci_bus *bus;
>>> +     void __iomem *dbi;
>>> +     void __iomem *scfg;
>>> +     struct pcie_port pp;
>>> +     int id;
>>> +     int index;
>>> +     int irq;
>>> +     int msi_irq;
>>> +     int pme_irq;
>>> +};
>> irq and pme_irq seem to be completely unused, so better
>> not add them until they are actually used.
>> [Minghuan] Ok, I will remove them.
>>
>> The scfg registers seem to belong to another device that
>> is responsible for multiple instances. Unfortunately,
>> this "fsl,ls1021a-scfg" device is not documented anywhere
>> that I can see.
>>
>> Is this some sort of syscon node, or is it specific to the
>> pcie controller(s)?
>>
>> [Minghaun] SCFG provides SoC specific configuration and status
>> registers for the device including PCI USB eTSEC SATA ...
>> The platform patches that contains SCFG code are being upstream.
> This sounds like it should be a syscon node, and you should use
> syscon_regmap_lookup_by_phandle() to find the scfg node from each
> of those drivers, rather than scanning the DT yourself in each
> of the drivers using it.
>
> This can also be a place to put the index, such as
>
> 	scfg = <&scfg 0>;
>
> for the first PCIe node. The probe function then extracts both
> the phandle for the syscon and the index from that property.
[Minghuan] I discussed with my colleague. They worry about performance 
degradation if using regmap API,
because there are some fast device use scfg. We tend to use a simple way 
to map andread/write scfg directly.
>>> +static int ls_pcie_link_up(struct pcie_port *pp)
>>> +{
>>> +     u32 rc, tmp;
>>> +     struct ls_pcie *pcie = to_ls_pcie(pp);
>>> +
>>> +     tmp = ioread32(pcie->scfg + SCFG_SCFGREVCR_OFF);
>>> +     iowrite32(SCFG_NO_BIT_REVERSE, pcie->scfg + SCFG_SCFGREVCR_OFF);
>>> +
>>> +     rc = (ioread32(pcie->scfg + PEXMSCPORTSR(pcie->index)) >>
>>> +          LTSSM_STATE_SHIFT) & LTSSM_STATE_MASK;
>>> +
>>> +     iowrite32(tmp, pcie->scfg + SCFG_SCFGREVCR_OFF);
>>> +
>>> +     if (rc < LTSSM_PCIE_L0)
>>> +             return 0;
>>> +
>>> +     return 1;
>>> +}
>> This looks like it is really a phy driver, although a pretty minimal
>> one.
>>
>> [Minghuan] I read SCFG register. SCFG provides SoC specific configuration
>> and status registers for the device including PCI USB eTSEC SATA ...
> I'm guessing that a lot of that is phy related information though.
> A nicer way to deal with this, compared to using syscon directly is
> to have a phy driver for your chip, and have that deal with all
> the phy related setup. In this case you would have a reference in
> the client driver like
>
> 	phys = <&scfgphy LS1021A_PHY_PCIE0>;
> 	phy-names = "pcie";
>
> And in the pcie driver you just call devm_phy_get(dev, "pcie") to get a reference
> to that phy node, and then you can use the phy API to perform actions on
> it (init, power_on, power_off, exit).
>
> This keeps all knowledge about the phy registers inside of the scfg area
> in one place, and you only have to add a new phy driver for a future SoC
> that has the same PCIe core but different scfg.
[Minghuan] SCFG does not contains power_on power_off or other PCI 
control registers.
We only  get link up and MSI related status via SCFG. So I think it is 
not enough to call scfg
PCIe phy.
>>> +static u32 ls_pcie_get_msi_addr(struct pcie_port *pp)
>>> +{
>>> +     return SCFG_SPIMSICR_ADDR;
>>> +}
>>> +
>>> +static u32 ls_pcie_get_msi_data(struct pcie_port *pp, int pos)
>>> +{
>>> +     struct ls_pcie *pcie = to_ls_pcie(pp);
>>> +
>>> +     if (pcie->id == PCI_DEVICE_ID_LS1021A)
>>> +             return MSI_LS1021A_DATA(pcie->index);
>>> +
>>> +     return pos;
>>> +}
>> My impression is that you have two distinct MSI controller
>> implementations, one for LS1021A and the other one for everything
>> else. How about using separate pcie_host_ops for them, possibly
>> also moving them into separate files?
>>
>> A good way to deal with this is to put the pointers to the two
>> pcie_host_ops into the data fields of the ls_pcie_of_match table.
>>
>> [Minghuan] LS1021 contains the same two PCI controllers PEX1 and PEX2.
>> both PEX1 and PEX use the same MSI address, but different MSI message data.
> I mean LS1021 is different from other chips here, since you compare
> the pcie->id value.
[Minghuan] I see. I will change it.
> 	Arnd

Re: 答复: [PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Arnd Bergmann]

On Tuesday 09 September 2014 17:25:57 Lian Minghuan-B31939 wrote:
> On 2014年09月05日 08:44, Arnd Bergmann wrote:
> > On Friday 05 September 2014 07:22:08 Minghuan.Lian@freescale.com wrote:
> >>> +struct ls_pcie {
> >>> +     struct list_head node;
> >>> +     struct device *dev;
> >>> +     struct pci_bus *bus;
> >>> +     void __iomem *dbi;
> >>> +     void __iomem *scfg;
> >>> +     struct pcie_port pp;
> >>> +     int id;
> >>> +     int index;
> >>> +     int irq;
> >>> +     int msi_irq;
> >>> +     int pme_irq;
> >>> +};
> >> irq and pme_irq seem to be completely unused, so better
> >> not add them until they are actually used.
> >> [Minghuan] Ok, I will remove them.
> >>
> >> The scfg registers seem to belong to another device that
> >> is responsible for multiple instances. Unfortunately,
> >> this "fsl,ls1021a-scfg" device is not documented anywhere
> >> that I can see.
> >>
> >> Is this some sort of syscon node, or is it specific to the
> >> pcie controller(s)?
> >>
> >> [Minghaun] SCFG provides SoC specific configuration and status
> >> registers for the device including PCI USB eTSEC SATA ...
> >> The platform patches that contains SCFG code are being upstream.
> > This sounds like it should be a syscon node, and you should use
> > syscon_regmap_lookup_by_phandle() to find the scfg node from each
> > of those drivers, rather than scanning the DT yourself in each
> > of the drivers using it.
> >
> > This can also be a place to put the index, such as
> >
> > 	scfg = <&scfg 0>;
> >
> > for the first PCIe node. The probe function then extracts both
> > the phandle for the syscon and the index from that property.
> [Minghuan] I discussed with my colleague. They worry about performance 
> degradation if using regmap API,
> because there are some fast device use scfg. We tend to use a simple way 
> to map andread/write scfg directly.

I see. In this case, I would probably create a separate msi controller
driver that owns the "fsl,ls1021a-scfg" device, and is referenced
through the "msi-parent" property in the pcie controller.

You can use of_pci_find_msi_chip_by_node() to get the msi_chip
instance and then connect that to your pci host. This will also
take care of the case where you may want to use the main GICv3
on a future SoC.

> >>> +static int ls_pcie_link_up(struct pcie_port *pp)
> >>> +{
> >>> +     u32 rc, tmp;
> >>> +     struct ls_pcie *pcie = to_ls_pcie(pp);
> >>> +
> >>> +     tmp = ioread32(pcie->scfg + SCFG_SCFGREVCR_OFF);
> >>> +     iowrite32(SCFG_NO_BIT_REVERSE, pcie->scfg + SCFG_SCFGREVCR_OFF);
> >>> +
> >>> +     rc = (ioread32(pcie->scfg + PEXMSCPORTSR(pcie->index)) >>
> >>> +          LTSSM_STATE_SHIFT) & LTSSM_STATE_MASK;
> >>> +
> >>> +     iowrite32(tmp, pcie->scfg + SCFG_SCFGREVCR_OFF);
> >>> +
> >>> +     if (rc < LTSSM_PCIE_L0)
> >>> +             return 0;
> >>> +
> >>> +     return 1;
> >>> +}
> >> This looks like it is really a phy driver, although a pretty minimal
> >> one.
> >>
> >> [Minghuan] I read SCFG register. SCFG provides SoC specific configuration
> >> and status registers for the device including PCI USB eTSEC SATA ...
> > I'm guessing that a lot of that is phy related information though.
> > A nicer way to deal with this, compared to using syscon directly is
> > to have a phy driver for your chip, and have that deal with all
> > the phy related setup. In this case you would have a reference in
> > the client driver like
> >
> > 	phys = <&scfgphy LS1021A_PHY_PCIE0>;
> > 	phy-names = "pcie";
> >
> > And in the pcie driver you just call devm_phy_get(dev, "pcie") to get a reference
> > to that phy node, and then you can use the phy API to perform actions on
> > it (init, power_on, power_off, exit).
> >
> > This keeps all knowledge about the phy registers inside of the scfg area
> > in one place, and you only have to add a new phy driver for a future SoC
> > that has the same PCIe core but different scfg.
> [Minghuan] SCFG does not contains power_on power_off or other PCI 
> control registers.
> We only  get link up and MSI related status via SCFG. So I think it is 
> not enough to call scfg PCIe phy.

Ok, then use syscon for this one. Note that we are currently changing
the syscon code to make it easier for the same device to be both syscon
and driven by another device driver such as the msi_chip above.

	Arnd

答复: [PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Arnd Bergmann]

On Tuesday 09 September 2014 17:25:57 Lian Minghuan-B31939 wrote:
> On 2014?09?05? 08:44, Arnd Bergmann wrote:
> > On Friday 05 September 2014 07:22:08 Minghuan.Lian at freescale.com wrote:
> >>> +struct ls_pcie {
> >>> +     struct list_head node;
> >>> +     struct device *dev;
> >>> +     struct pci_bus *bus;
> >>> +     void __iomem *dbi;
> >>> +     void __iomem *scfg;
> >>> +     struct pcie_port pp;
> >>> +     int id;
> >>> +     int index;
> >>> +     int irq;
> >>> +     int msi_irq;
> >>> +     int pme_irq;
> >>> +};
> >> irq and pme_irq seem to be completely unused, so better
> >> not add them until they are actually used.
> >> [Minghuan] Ok, I will remove them.
> >>
> >> The scfg registers seem to belong to another device that
> >> is responsible for multiple instances. Unfortunately,
> >> this "fsl,ls1021a-scfg" device is not documented anywhere
> >> that I can see.
> >>
> >> Is this some sort of syscon node, or is it specific to the
> >> pcie controller(s)?
> >>
> >> [Minghaun] SCFG provides SoC specific configuration and status
> >> registers for the device including PCI USB eTSEC SATA ...
> >> The platform patches that contains SCFG code are being upstream.
> > This sounds like it should be a syscon node, and you should use
> > syscon_regmap_lookup_by_phandle() to find the scfg node from each
> > of those drivers, rather than scanning the DT yourself in each
> > of the drivers using it.
> >
> > This can also be a place to put the index, such as
> >
> > 	scfg = <&scfg 0>;
> >
> > for the first PCIe node. The probe function then extracts both
> > the phandle for the syscon and the index from that property.
> [Minghuan] I discussed with my colleague. They worry about performance 
> degradation if using regmap API,
> because there are some fast device use scfg. We tend to use a simple way 
> to map andread/write scfg directly.

I see. In this case, I would probably create a separate msi controller
driver that owns the "fsl,ls1021a-scfg" device, and is referenced
through the "msi-parent" property in the pcie controller.

You can use of_pci_find_msi_chip_by_node() to get the msi_chip
instance and then connect that to your pci host. This will also
take care of the case where you may want to use the main GICv3
on a future SoC.

> >>> +static int ls_pcie_link_up(struct pcie_port *pp)
> >>> +{
> >>> +     u32 rc, tmp;
> >>> +     struct ls_pcie *pcie = to_ls_pcie(pp);
> >>> +
> >>> +     tmp = ioread32(pcie->scfg + SCFG_SCFGREVCR_OFF);
> >>> +     iowrite32(SCFG_NO_BIT_REVERSE, pcie->scfg + SCFG_SCFGREVCR_OFF);
> >>> +
> >>> +     rc = (ioread32(pcie->scfg + PEXMSCPORTSR(pcie->index)) >>
> >>> +          LTSSM_STATE_SHIFT) & LTSSM_STATE_MASK;
> >>> +
> >>> +     iowrite32(tmp, pcie->scfg + SCFG_SCFGREVCR_OFF);
> >>> +
> >>> +     if (rc < LTSSM_PCIE_L0)
> >>> +             return 0;
> >>> +
> >>> +     return 1;
> >>> +}
> >> This looks like it is really a phy driver, although a pretty minimal
> >> one.
> >>
> >> [Minghuan] I read SCFG register. SCFG provides SoC specific configuration
> >> and status registers for the device including PCI USB eTSEC SATA ...
> > I'm guessing that a lot of that is phy related information though.
> > A nicer way to deal with this, compared to using syscon directly is
> > to have a phy driver for your chip, and have that deal with all
> > the phy related setup. In this case you would have a reference in
> > the client driver like
> >
> > 	phys = <&scfgphy LS1021A_PHY_PCIE0>;
> > 	phy-names = "pcie";
> >
> > And in the pcie driver you just call devm_phy_get(dev, "pcie") to get a reference
> > to that phy node, and then you can use the phy API to perform actions on
> > it (init, power_on, power_off, exit).
> >
> > This keeps all knowledge about the phy registers inside of the scfg area
> > in one place, and you only have to add a new phy driver for a future SoC
> > that has the same PCIe core but different scfg.
> [Minghuan] SCFG does not contains power_on power_off or other PCI 
> control registers.
> We only  get link up and MSI related status via SCFG. So I think it is 
> not enough to call scfg PCIe phy.

Ok, then use syscon for this one. Note that we are currently changing
the syscon code to make it easier for the same device to be both syscon
and driven by another device driver such as the msi_chip above.

	Arnd

Re: 答复: [PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Lian Minghuan-B31939]

Hi Arnd,

please see my comments inline.

On 2014年09月09日 09:56, Arnd Bergmann wrote:
> On Tuesday 09 September 2014 17:25:57 Lian Minghuan-B31939 wrote:
>> On 2014年09月05日 08:44, Arnd Bergmann wrote:
>>> On Friday 05 September 2014 07:22:08 Minghuan.Lian@freescale.com wrote:
>>>>> +struct ls_pcie {
>>>>> +     struct list_head node;
>>>>> +     struct device *dev;
>>>>> +     struct pci_bus *bus;
>>>>> +     void __iomem *dbi;
>>>>> +     void __iomem *scfg;
>>>>> +     struct pcie_port pp;
>>>>> +     int id;
>>>>> +     int index;
>>>>> +     int irq;
>>>>> +     int msi_irq;
>>>>> +     int pme_irq;
>>>>> +};
>>>> irq and pme_irq seem to be completely unused, so better
>>>> not add them until they are actually used.
>>>> [Minghuan] Ok, I will remove them.
>>>>
>>>> The scfg registers seem to belong to another device that
>>>> is responsible for multiple instances. Unfortunately,
>>>> this "fsl,ls1021a-scfg" device is not documented anywhere
>>>> that I can see.
>>>>
>>>> Is this some sort of syscon node, or is it specific to the
>>>> pcie controller(s)?
>>>>
>>>> [Minghaun] SCFG provides SoC specific configuration and status
>>>> registers for the device including PCI USB eTSEC SATA ...
>>>> The platform patches that contains SCFG code are being upstream.
>>> This sounds like it should be a syscon node, and you should use
>>> syscon_regmap_lookup_by_phandle() to find the scfg node from each
>>> of those drivers, rather than scanning the DT yourself in each
>>> of the drivers using it.
>>>
>>> This can also be a place to put the index, such as
>>>
>>> 	scfg = <&scfg 0>;
>>>
>>> for the first PCIe node. The probe function then extracts both
>>> the phandle for the syscon and the index from that property.
>> [Minghuan] I discussed with my colleague. They worry about performance
>> degradation if using regmap API,
>> because there are some fast device use scfg. We tend to use a simple way
>> to map andread/write scfg directly.
> I see. In this case, I would probably create a separate msi controller
> driver that owns the "fsl,ls1021a-scfg" device, and is referenced
> through the "msi-parent" property in the pcie controller.
>
> You can use of_pci_find_msi_chip_by_node() to get the msi_chip
> instance and then connect that to your pci host. This will also
> take care of the case where you may want to use the main GICv3
> on a future SoC.
[Minghuan] There is something wrong with LS1021A MSI hardware that it 
only supports one interrupt not 32 interrupts.  Now, I do not want to 
create a separate msi controller driver just for incorrect hardware.
I may provide complete MSI driver for the new hardware when it is ready.
>>>>> +static int ls_pcie_link_up(struct pcie_port *pp)
>>>>> +{
>>>>> +     u32 rc, tmp;
>>>>> +     struct ls_pcie *pcie = to_ls_pcie(pp);
>>>>> +
>>>>> +     tmp = ioread32(pcie->scfg + SCFG_SCFGREVCR_OFF);
>>>>> +     iowrite32(SCFG_NO_BIT_REVERSE, pcie->scfg + SCFG_SCFGREVCR_OFF);
>>>>> +
>>>>> +     rc = (ioread32(pcie->scfg + PEXMSCPORTSR(pcie->index)) >>
>>>>> +          LTSSM_STATE_SHIFT) & LTSSM_STATE_MASK;
>>>>> +
>>>>> +     iowrite32(tmp, pcie->scfg + SCFG_SCFGREVCR_OFF);
>>>>> +
>>>>> +     if (rc < LTSSM_PCIE_L0)
>>>>> +             return 0;
>>>>> +
>>>>> +     return 1;
>>>>> +}
>>>> This looks like it is really a phy driver, although a pretty minimal
>>>> one.
>>>>
>>>> [Minghuan] I read SCFG register. SCFG provides SoC specific configuration
>>>> and status registers for the device including PCI USB eTSEC SATA ...
>>> I'm guessing that a lot of that is phy related information though.
>>> A nicer way to deal with this, compared to using syscon directly is
>>> to have a phy driver for your chip, and have that deal with all
>>> the phy related setup. In this case you would have a reference in
>>> the client driver like
>>>
>>> 	phys = <&scfgphy LS1021A_PHY_PCIE0>;
>>> 	phy-names = "pcie";
>>>
>>> And in the pcie driver you just call devm_phy_get(dev, "pcie") to get a reference
>>> to that phy node, and then you can use the phy API to perform actions on
>>> it (init, power_on, power_off, exit).
>>>
>>> This keeps all knowledge about the phy registers inside of the scfg area
>>> in one place, and you only have to add a new phy driver for a future SoC
>>> that has the same PCIe core but different scfg.
>> [Minghuan] SCFG does not contains power_on power_off or other PCI
>> control registers.
>> We only  get link up and MSI related status via SCFG. So I think it is
>> not enough to call scfg PCIe phy.
> Ok, then use syscon for this one. Note that we are currently changing
> the syscon code to make it easier for the same device to be both syscon
> and driven by another device driver such as the msi_chip above.
[Minghuan] I will try
> 	Arnd

答复: [PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Lian Minghuan-B31939]

Hi Arnd,

please see my comments inline.

On 2014?09?09? 09:56, Arnd Bergmann wrote:
> On Tuesday 09 September 2014 17:25:57 Lian Minghuan-B31939 wrote:
>> On 2014?09?05? 08:44, Arnd Bergmann wrote:
>>> On Friday 05 September 2014 07:22:08 Minghuan.Lian at freescale.com wrote:
>>>>> +struct ls_pcie {
>>>>> +     struct list_head node;
>>>>> +     struct device *dev;
>>>>> +     struct pci_bus *bus;
>>>>> +     void __iomem *dbi;
>>>>> +     void __iomem *scfg;
>>>>> +     struct pcie_port pp;
>>>>> +     int id;
>>>>> +     int index;
>>>>> +     int irq;
>>>>> +     int msi_irq;
>>>>> +     int pme_irq;
>>>>> +};
>>>> irq and pme_irq seem to be completely unused, so better
>>>> not add them until they are actually used.
>>>> [Minghuan] Ok, I will remove them.
>>>>
>>>> The scfg registers seem to belong to another device that
>>>> is responsible for multiple instances. Unfortunately,
>>>> this "fsl,ls1021a-scfg" device is not documented anywhere
>>>> that I can see.
>>>>
>>>> Is this some sort of syscon node, or is it specific to the
>>>> pcie controller(s)?
>>>>
>>>> [Minghaun] SCFG provides SoC specific configuration and status
>>>> registers for the device including PCI USB eTSEC SATA ...
>>>> The platform patches that contains SCFG code are being upstream.
>>> This sounds like it should be a syscon node, and you should use
>>> syscon_regmap_lookup_by_phandle() to find the scfg node from each
>>> of those drivers, rather than scanning the DT yourself in each
>>> of the drivers using it.
>>>
>>> This can also be a place to put the index, such as
>>>
>>> 	scfg = <&scfg 0>;
>>>
>>> for the first PCIe node. The probe function then extracts both
>>> the phandle for the syscon and the index from that property.
>> [Minghuan] I discussed with my colleague. They worry about performance
>> degradation if using regmap API,
>> because there are some fast device use scfg. We tend to use a simple way
>> to map andread/write scfg directly.
> I see. In this case, I would probably create a separate msi controller
> driver that owns the "fsl,ls1021a-scfg" device, and is referenced
> through the "msi-parent" property in the pcie controller.
>
> You can use of_pci_find_msi_chip_by_node() to get the msi_chip
> instance and then connect that to your pci host. This will also
> take care of the case where you may want to use the main GICv3
> on a future SoC.
[Minghuan] There is something wrong with LS1021A MSI hardware that it 
only supports one interrupt not 32 interrupts.  Now, I do not want to 
create a separate msi controller driver just for incorrect hardware.
I may provide complete MSI driver for the new hardware when it is ready.
>>>>> +static int ls_pcie_link_up(struct pcie_port *pp)
>>>>> +{
>>>>> +     u32 rc, tmp;
>>>>> +     struct ls_pcie *pcie = to_ls_pcie(pp);
>>>>> +
>>>>> +     tmp = ioread32(pcie->scfg + SCFG_SCFGREVCR_OFF);
>>>>> +     iowrite32(SCFG_NO_BIT_REVERSE, pcie->scfg + SCFG_SCFGREVCR_OFF);
>>>>> +
>>>>> +     rc = (ioread32(pcie->scfg + PEXMSCPORTSR(pcie->index)) >>
>>>>> +          LTSSM_STATE_SHIFT) & LTSSM_STATE_MASK;
>>>>> +
>>>>> +     iowrite32(tmp, pcie->scfg + SCFG_SCFGREVCR_OFF);
>>>>> +
>>>>> +     if (rc < LTSSM_PCIE_L0)
>>>>> +             return 0;
>>>>> +
>>>>> +     return 1;
>>>>> +}
>>>> This looks like it is really a phy driver, although a pretty minimal
>>>> one.
>>>>
>>>> [Minghuan] I read SCFG register. SCFG provides SoC specific configuration
>>>> and status registers for the device including PCI USB eTSEC SATA ...
>>> I'm guessing that a lot of that is phy related information though.
>>> A nicer way to deal with this, compared to using syscon directly is
>>> to have a phy driver for your chip, and have that deal with all
>>> the phy related setup. In this case you would have a reference in
>>> the client driver like
>>>
>>> 	phys = <&scfgphy LS1021A_PHY_PCIE0>;
>>> 	phy-names = "pcie";
>>>
>>> And in the pcie driver you just call devm_phy_get(dev, "pcie") to get a reference
>>> to that phy node, and then you can use the phy API to perform actions on
>>> it (init, power_on, power_off, exit).
>>>
>>> This keeps all knowledge about the phy registers inside of the scfg area
>>> in one place, and you only have to add a new phy driver for a future SoC
>>> that has the same PCIe core but different scfg.
>> [Minghuan] SCFG does not contains power_on power_off or other PCI
>> control registers.
>> We only  get link up and MSI related status via SCFG. So I think it is
>> not enough to call scfg PCIe phy.
> Ok, then use syscon for this one. Note that we are currently changing
> the syscon code to make it easier for the same device to be both syscon
> and driven by another device driver such as the msi_chip above.
[Minghuan] I will try
> 	Arnd

Re: 答复: [PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Arnd Bergmann]

On Tuesday 09 September 2014 18:46:59 Lian Minghuan-B31939 wrote:
> On 2014年09月09日 09:56, Arnd Bergmann wrote:
> > On Tuesday 09 September 2014 17:25:57 Lian Minghuan-B31939 wrote:
> >> [Minghuan] I discussed with my colleague. They worry about performance
> >> degradation if using regmap API,
> >> because there are some fast device use scfg. We tend to use a simple way
> >> to map andread/write scfg directly.
> >
> > I see. In this case, I would probably create a separate msi controller
> > driver that owns the "fsl,ls1021a-scfg" device, and is referenced
> > through the "msi-parent" property in the pcie controller.
> >
> > You can use of_pci_find_msi_chip_by_node() to get the msi_chip
> > instance and then connect that to your pci host. This will also
> > take care of the case where you may want to use the main GICv3
>
> > on a future SoC.
> [Minghuan] There is something wrong with LS1021A MSI hardware that it 
> only supports one interrupt not 32 interrupts.  Now, I do not want to 
> create a separate msi controller driver just for incorrect hardware.
> I may provide complete MSI driver for the new hardware when it is ready.

Would you just leave out MSI support for the LS1021A PCIe variant?
I guess that's fine because all device drivers should also support
legacy interrupts and there is no performance gain in MSI in this
case.

	Arnd

答复: [PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Arnd Bergmann]

On Tuesday 09 September 2014 18:46:59 Lian Minghuan-B31939 wrote:
> On 2014?09?09? 09:56, Arnd Bergmann wrote:
> > On Tuesday 09 September 2014 17:25:57 Lian Minghuan-B31939 wrote:
> >> [Minghuan] I discussed with my colleague. They worry about performance
> >> degradation if using regmap API,
> >> because there are some fast device use scfg. We tend to use a simple way
> >> to map andread/write scfg directly.
> >
> > I see. In this case, I would probably create a separate msi controller
> > driver that owns the "fsl,ls1021a-scfg" device, and is referenced
> > through the "msi-parent" property in the pcie controller.
> >
> > You can use of_pci_find_msi_chip_by_node() to get the msi_chip
> > instance and then connect that to your pci host. This will also
> > take care of the case where you may want to use the main GICv3
>
> > on a future SoC.
> [Minghuan] There is something wrong with LS1021A MSI hardware that it 
> only supports one interrupt not 32 interrupts.  Now, I do not want to 
> create a separate msi controller driver just for incorrect hardware.
> I may provide complete MSI driver for the new hardware when it is ready.

Would you just leave out MSI support for the LS1021A PCIe variant?
I guess that's fine because all device drivers should also support
legacy interrupts and there is no performance gain in MSI in this
case.

	Arnd

Re: 答复: [PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Lian Minghuan-B31939]

On 2014年09月09日 10:50, Arnd Bergmann wrote:
> On Tuesday 09 September 2014 18:46:59 Lian Minghuan-B31939 wrote:
>> On 2014年09月09日 09:56, Arnd Bergmann wrote:
>>> On Tuesday 09 September 2014 17:25:57 Lian Minghuan-B31939 wrote:
>>>> [Minghuan] I discussed with my colleague. They worry about performance
>>>> degradation if using regmap API,
>>>> because there are some fast device use scfg. We tend to use a simple way
>>>> to map andread/write scfg directly.
>>> I see. In this case, I would probably create a separate msi controller
>>> driver that owns the "fsl,ls1021a-scfg" device, and is referenced
>>> through the "msi-parent" property in the pcie controller.
>>>
>>> You can use of_pci_find_msi_chip_by_node() to get the msi_chip
>>> instance and then connect that to your pci host. This will also
>>> take care of the case where you may want to use the main GICv3
>>> on a future SoC.
>> [Minghuan] There is something wrong with LS1021A MSI hardware that it
>> only supports one interrupt not 32 interrupts.  Now, I do not want to
>> create a separate msi controller driver just for incorrect hardware.
>> I may provide complete MSI driver for the new hardware when it is ready.
> Would you just leave out MSI support for the LS1021A PCIe variant?
> I guess that's fine because all device drivers should also support
> legacy interrupts and there is no performance gain in MSI in this
> case.
[Minghuan] I have added MSI support for LS1021A PCIe just reserved 31 
interrupts as used.

> 	Arnd

答复: [PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Lian Minghuan-B31939]

On 2014?09?09? 10:50, Arnd Bergmann wrote:
> On Tuesday 09 September 2014 18:46:59 Lian Minghuan-B31939 wrote:
>> On 2014?09?09? 09:56, Arnd Bergmann wrote:
>>> On Tuesday 09 September 2014 17:25:57 Lian Minghuan-B31939 wrote:
>>>> [Minghuan] I discussed with my colleague. They worry about performance
>>>> degradation if using regmap API,
>>>> because there are some fast device use scfg. We tend to use a simple way
>>>> to map andread/write scfg directly.
>>> I see. In this case, I would probably create a separate msi controller
>>> driver that owns the "fsl,ls1021a-scfg" device, and is referenced
>>> through the "msi-parent" property in the pcie controller.
>>>
>>> You can use of_pci_find_msi_chip_by_node() to get the msi_chip
>>> instance and then connect that to your pci host. This will also
>>> take care of the case where you may want to use the main GICv3
>>> on a future SoC.
>> [Minghuan] There is something wrong with LS1021A MSI hardware that it
>> only supports one interrupt not 32 interrupts.  Now, I do not want to
>> create a separate msi controller driver just for incorrect hardware.
>> I may provide complete MSI driver for the new hardware when it is ready.
> Would you just leave out MSI support for the LS1021A PCIe variant?
> I guess that's fine because all device drivers should also support
> legacy interrupts and there is no performance gain in MSI in this
> case.
[Minghuan] I have added MSI support for LS1021A PCIe just reserved 31 
interrupts as used.

> 	Arnd

Re: 答复: [PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Arnd Bergmann]

On Tuesday 09 September 2014 19:16:01 Lian Minghuan-B31939 wrote:
> On 2014年09月09日 10:50, Arnd Bergmann wrote:
> > On Tuesday 09 September 2014 18:46:59 Lian Minghuan-B31939 wrote:
> >> On 2014年09月09日 09:56, Arnd Bergmann wrote:
> >>> On Tuesday 09 September 2014 17:25:57 Lian Minghuan-B31939 wrote:
> >>>> [Minghuan] I discussed with my colleague. They worry about performance
> >>>> degradation if using regmap API,
> >>>> because there are some fast device use scfg. We tend to use a simple way
> >>>> to map andread/write scfg directly.
> >>> I see. In this case, I would probably create a separate msi controller
> >>> driver that owns the "fsl,ls1021a-scfg" device, and is referenced
> >>> through the "msi-parent" property in the pcie controller.
> >>>
> >>> You can use of_pci_find_msi_chip_by_node() to get the msi_chip
> >>> instance and then connect that to your pci host. This will also
> >>> take care of the case where you may want to use the main GICv3
> >>> on a future SoC.
> >>
> >> [Minghuan] There is something wrong with LS1021A MSI hardware that it
> >> only supports one interrupt not 32 interrupts.  Now, I do not want to
> >> create a separate msi controller driver just for incorrect hardware.
> >> I may provide complete MSI driver for the new hardware when it is ready.
> >
> > Would you just leave out MSI support for the LS1021A PCIe variant?
> > I guess that's fine because all device drivers should also support
> > legacy interrupts and there is no performance gain in MSI in this
> > case.
>
> [Minghuan] I have added MSI support for LS1021A PCIe just reserved 31 
> interrupts as used.

I don't understand your logic then. If LS1021A has an incorrect MSI
implementation, and you may want to reuse the PCIe driver with a
future chip that either includes a correct MSI implementation, or
with one that uses the GICv3 instead, isn't that even more reason
to split out the MSI support into a separate driver?

That way you can at least separate the normal code path from the
broken one and don't need any special run-time or compile-conditionals
beyond calling of_pci_find_msi_chip_by_node().

	Arnd

答复: [PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Arnd Bergmann]

On Tuesday 09 September 2014 19:16:01 Lian Minghuan-B31939 wrote:
> On 2014?09?09? 10:50, Arnd Bergmann wrote:
> > On Tuesday 09 September 2014 18:46:59 Lian Minghuan-B31939 wrote:
> >> On 2014?09?09? 09:56, Arnd Bergmann wrote:
> >>> On Tuesday 09 September 2014 17:25:57 Lian Minghuan-B31939 wrote:
> >>>> [Minghuan] I discussed with my colleague. They worry about performance
> >>>> degradation if using regmap API,
> >>>> because there are some fast device use scfg. We tend to use a simple way
> >>>> to map andread/write scfg directly.
> >>> I see. In this case, I would probably create a separate msi controller
> >>> driver that owns the "fsl,ls1021a-scfg" device, and is referenced
> >>> through the "msi-parent" property in the pcie controller.
> >>>
> >>> You can use of_pci_find_msi_chip_by_node() to get the msi_chip
> >>> instance and then connect that to your pci host. This will also
> >>> take care of the case where you may want to use the main GICv3
> >>> on a future SoC.
> >>
> >> [Minghuan] There is something wrong with LS1021A MSI hardware that it
> >> only supports one interrupt not 32 interrupts.  Now, I do not want to
> >> create a separate msi controller driver just for incorrect hardware.
> >> I may provide complete MSI driver for the new hardware when it is ready.
> >
> > Would you just leave out MSI support for the LS1021A PCIe variant?
> > I guess that's fine because all device drivers should also support
> > legacy interrupts and there is no performance gain in MSI in this
> > case.
>
> [Minghuan] I have added MSI support for LS1021A PCIe just reserved 31 
> interrupts as used.

I don't understand your logic then. If LS1021A has an incorrect MSI
implementation, and you may want to reuse the PCIe driver with a
future chip that either includes a correct MSI implementation, or
with one that uses the GICv3 instead, isn't that even more reason
to split out the MSI support into a separate driver?

That way you can at least separate the normal code path from the
broken one and don't need any special run-time or compile-conditionals
beyond calling of_pci_find_msi_chip_by_node().

	Arnd

Re: 答复: [PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Lian Minghuan-B31939]

Hi Arnd,

On 2014年09月09日 11:58, Arnd Bergmann wrote:
> On Tuesday 09 September 2014 19:16:01 Lian Minghuan-B31939 wrote:
>> On 2014年09月09日 10:50, Arnd Bergmann wrote:
>>> On Tuesday 09 September 2014 18:46:59 Lian Minghuan-B31939 wrote:
>>>> On 2014年09月09日 09:56, Arnd Bergmann wrote:
>>>>> On Tuesday 09 September 2014 17:25:57 Lian Minghuan-B31939 wrote:
>>>>>> [Minghuan] I discussed with my colleague. They worry about performance
>>>>>> degradation if using regmap API,
>>>>>> because there are some fast device use scfg. We tend to use a simple way
>>>>>> to map andread/write scfg directly.
>>>>> I see. In this case, I would probably create a separate msi controller
>>>>> driver that owns the "fsl,ls1021a-scfg" device, and is referenced
>>>>> through the "msi-parent" property in the pcie controller.
>>>>>
>>>>> You can use of_pci_find_msi_chip_by_node() to get the msi_chip
>>>>> instance and then connect that to your pci host. This will also
>>>>> take care of the case where you may want to use the main GICv3
>>>>> on a future SoC.
>>>> [Minghuan] There is something wrong with LS1021A MSI hardware that it
>>>> only supports one interrupt not 32 interrupts.  Now, I do not want to
>>>> create a separate msi controller driver just for incorrect hardware.
>>>> I may provide complete MSI driver for the new hardware when it is ready.
>>> Would you just leave out MSI support for the LS1021A PCIe variant?
>>> I guess that's fine because all device drivers should also support
>>> legacy interrupts and there is no performance gain in MSI in this
>>> case.
>> [Minghuan] I have added MSI support for LS1021A PCIe just reserved 31
>> interrupts as used.
> I don't understand your logic then. If LS1021A has an incorrect MSI
> implementation, and you may want to reuse the PCIe driver with a
> future chip that either includes a correct MSI implementation, or
> with one that uses the GICv3 instead, isn't that even more reason
> to split out the MSI support into a separate driver?
>
> That way you can at least separate the normal code path from the
> broken one and don't need any special run-time or compile-conditionals
> beyond calling of_pci_find_msi_chip_by_node().
[Minghuan] The new MSI hardware implementation is not ready, I do not 
know how to describe its dts node
and how to implement its driver. When it is ready, I will implement the 
driver and add a workaround
for the current incorrect MSI. The current MSI driver is based on 
pci-designware.
> 	Arnd

答复: [PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Lian Minghuan-B31939]

Hi Arnd,

On 2014?09?09? 11:58, Arnd Bergmann wrote:
> On Tuesday 09 September 2014 19:16:01 Lian Minghuan-B31939 wrote:
>> On 2014?09?09? 10:50, Arnd Bergmann wrote:
>>> On Tuesday 09 September 2014 18:46:59 Lian Minghuan-B31939 wrote:
>>>> On 2014?09?09? 09:56, Arnd Bergmann wrote:
>>>>> On Tuesday 09 September 2014 17:25:57 Lian Minghuan-B31939 wrote:
>>>>>> [Minghuan] I discussed with my colleague. They worry about performance
>>>>>> degradation if using regmap API,
>>>>>> because there are some fast device use scfg. We tend to use a simple way
>>>>>> to map andread/write scfg directly.
>>>>> I see. In this case, I would probably create a separate msi controller
>>>>> driver that owns the "fsl,ls1021a-scfg" device, and is referenced
>>>>> through the "msi-parent" property in the pcie controller.
>>>>>
>>>>> You can use of_pci_find_msi_chip_by_node() to get the msi_chip
>>>>> instance and then connect that to your pci host. This will also
>>>>> take care of the case where you may want to use the main GICv3
>>>>> on a future SoC.
>>>> [Minghuan] There is something wrong with LS1021A MSI hardware that it
>>>> only supports one interrupt not 32 interrupts.  Now, I do not want to
>>>> create a separate msi controller driver just for incorrect hardware.
>>>> I may provide complete MSI driver for the new hardware when it is ready.
>>> Would you just leave out MSI support for the LS1021A PCIe variant?
>>> I guess that's fine because all device drivers should also support
>>> legacy interrupts and there is no performance gain in MSI in this
>>> case.
>> [Minghuan] I have added MSI support for LS1021A PCIe just reserved 31
>> interrupts as used.
> I don't understand your logic then. If LS1021A has an incorrect MSI
> implementation, and you may want to reuse the PCIe driver with a
> future chip that either includes a correct MSI implementation, or
> with one that uses the GICv3 instead, isn't that even more reason
> to split out the MSI support into a separate driver?
>
> That way you can at least separate the normal code path from the
> broken one and don't need any special run-time or compile-conditionals
> beyond calling of_pci_find_msi_chip_by_node().
[Minghuan] The new MSI hardware implementation is not ready, I do not 
know how to describe its dts node
and how to implement its driver. When it is ready, I will implement the 
driver and add a workaround
for the current incorrect MSI. The current MSI driver is based on 
pci-designware.
> 	Arnd

Re: [PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Bjorn Helgaas]

On Thu, Sep 4, 2014 at 12:45 PM, Minghuan Lian
<Minghuan.Lian@freescale.com> wrote:
> Add support for Freescale Layerscape PCIe controller. This driver
> re-uses the designware core code.
>
> Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com>
> ---
>  .../devicetree/bindings/pci/fsl,ls-pcie.txt        |  41 +++
>  drivers/pci/host/Kconfig                           |   7 +
>  drivers/pci/host/Makefile                          |   1 +
>  drivers/pci/host/pci-layerscape.c                  | 315 +++++++++++++++++++++
>  include/linux/pci_ids.h                            |   1 +
>  5 files changed, 365 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/fsl,ls-pcie.txt
>  create mode 100644 drivers/pci/host/pci-layerscape.c

> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 6ed0bb7..26d4c30 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -2442,6 +2442,7 @@
>  #define PCI_DEVICE_ID_P5020            0x0421
>  #define PCI_DEVICE_ID_P5010E           0x0428
>  #define PCI_DEVICE_ID_P5010            0x0429
> +#define PCI_DEVICE_ID_LS1021A          0x0e0b

This is only used in one file and should not be in pci_ids.h (per the
comment at the top of pci_ids.h).

>  #define PCI_DEVICE_ID_MPC8641          0x7010
>  #define PCI_DEVICE_ID_MPC8641D         0x7011
>  #define PCI_DEVICE_ID_MPC8610          0x7018
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Bjorn Helgaas]

On Thu, Sep 4, 2014 at 12:45 PM, Minghuan Lian
<Minghuan.Lian@freescale.com> wrote:
> Add support for Freescale Layerscape PCIe controller. This driver
> re-uses the designware core code.
>
> Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com>
> ---
>  .../devicetree/bindings/pci/fsl,ls-pcie.txt        |  41 +++
>  drivers/pci/host/Kconfig                           |   7 +
>  drivers/pci/host/Makefile                          |   1 +
>  drivers/pci/host/pci-layerscape.c                  | 315 +++++++++++++++++++++
>  include/linux/pci_ids.h                            |   1 +
>  5 files changed, 365 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/fsl,ls-pcie.txt
>  create mode 100644 drivers/pci/host/pci-layerscape.c

> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 6ed0bb7..26d4c30 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -2442,6 +2442,7 @@
>  #define PCI_DEVICE_ID_P5020            0x0421
>  #define PCI_DEVICE_ID_P5010E           0x0428
>  #define PCI_DEVICE_ID_P5010            0x0429
> +#define PCI_DEVICE_ID_LS1021A          0x0e0b

This is only used in one file and should not be in pci_ids.h (per the
comment at the top of pci_ids.h).

>  #define PCI_DEVICE_ID_MPC8641          0x7010
>  #define PCI_DEVICE_ID_MPC8641D         0x7011
>  #define PCI_DEVICE_ID_MPC8610          0x7018
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

答复: [PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Minghuan.Lian at freescale.com]

Hi Bjorn,

Thanks for your comments, I will fix it.

Thanks,
Minghuan
________________________________________
???: Bjorn Helgaas <bhelgaas@google.com>
????: 2014?9?4? 21:24
???: Lian Minghuan-B31939
??: linux-pci at vger.kernel.org; linux-arm; Zang Roy-R61911; Hu Mingkai-B21284
??: Re: [PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

On Thu, Sep 4, 2014 at 12:45 PM, Minghuan Lian
<Minghuan.Lian@freescale.com> wrote:
> Add support for Freescale Layerscape PCIe controller. This driver
> re-uses the designware core code.
>
> Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com>
> ---
>  .../devicetree/bindings/pci/fsl,ls-pcie.txt        |  41 +++
>  drivers/pci/host/Kconfig                           |   7 +
>  drivers/pci/host/Makefile                          |   1 +
>  drivers/pci/host/pci-layerscape.c                  | 315 +++++++++++++++++++++
>  include/linux/pci_ids.h                            |   1 +
>  5 files changed, 365 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/fsl,ls-pcie.txt
>  create mode 100644 drivers/pci/host/pci-layerscape.c

> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 6ed0bb7..26d4c30 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -2442,6 +2442,7 @@
>  #define PCI_DEVICE_ID_P5020            0x0421
>  #define PCI_DEVICE_ID_P5010E           0x0428
>  #define PCI_DEVICE_ID_P5010            0x0429
> +#define PCI_DEVICE_ID_LS1021A          0x0e0b

This is only used in one file and should not be in pci_ids.h (per the
comment at the top of pci_ids.h).

>  #define PCI_DEVICE_ID_MPC8641          0x7010
>  #define PCI_DEVICE_ID_MPC8641D         0x7011
>  #define PCI_DEVICE_ID_MPC8610          0x7018
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Fabio Estevam]

On Thu, Sep 4, 2014 at 3:45 PM, Minghuan Lian
<Minghuan.Lian@freescale.com> wrote:
> Add support for Freescale Layerscape PCIe controller. This driver
> re-uses the designware core code.

It seems that this is the same IP as on mx6, right?

Is it possible to adapt drivers/pci/host/pci-imx6.c to work on Layerscape?

> +#define PCIE_LS1021A_BASE      0x3400000
> +#define PCIE_LS1021A_REG_SIZE  0x0100000

You should retrieve base and reg size from the dtsi file.

[PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Fabio Estevam]

On Thu, Sep 4, 2014 at 3:45 PM, Minghuan Lian
<Minghuan.Lian@freescale.com> wrote:
> Add support for Freescale Layerscape PCIe controller. This driver
> re-uses the designware core code.

It seems that this is the same IP as on mx6, right?

Is it possible to adapt drivers/pci/host/pci-imx6.c to work on Layerscape?

> +#define PCIE_LS1021A_BASE      0x3400000
> +#define PCIE_LS1021A_REG_SIZE  0x0100000

You should retrieve base and reg size from the dtsi file.

Re: [PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Arnd Bergmann]

On Thursday 04 September 2014 17:21:48 Fabio Estevam wrote:
> On Thu, Sep 4, 2014 at 3:45 PM, Minghuan Lian
> <Minghuan.Lian@freescale.com> wrote:
> > Add support for Freescale Layerscape PCIe controller. This driver
> > re-uses the designware core code.
> 
> It seems that this is the same IP as on mx6, right?
> 
> Is it possible to adapt drivers/pci/host/pci-imx6.c to work on Layerscape?

For all I can tell, the common parts are already shared in pcie-designware.c,
while the new file contains only those parts that are specific to layerscape
and different from imx.

If there are parts in there that are the same between layerscape and imx,
the implementation should also be moved to a shared file.

	Arnd

[PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Arnd Bergmann]

On Thursday 04 September 2014 17:21:48 Fabio Estevam wrote:
> On Thu, Sep 4, 2014 at 3:45 PM, Minghuan Lian
> <Minghuan.Lian@freescale.com> wrote:
> > Add support for Freescale Layerscape PCIe controller. This driver
> > re-uses the designware core code.
> 
> It seems that this is the same IP as on mx6, right?
> 
> Is it possible to adapt drivers/pci/host/pci-imx6.c to work on Layerscape?

For all I can tell, the common parts are already shared in pcie-designware.c,
while the new file contains only those parts that are specific to layerscape
and different from imx.

If there are parts in there that are the same between layerscape and imx,
the implementation should also be moved to a shared file.

	Arnd

答复: [PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Minghuan.Lian at freescale.com]

Hi Arnd,

Thanks for your comments.

There are no same parts between Layerscape and imx.
I think we should use a new file to implement Layerscape PCI driver.

Thanks,
Minghuan
________________________________________
???: Arnd Bergmann <arnd@arndb.de>
????: 2014?9?5? 5:12
???: linux-arm-kernel at lists.infradead.org
??: Fabio Estevam; Lian Minghuan-B31939; Wood Scott-B07421; linux-pci at vger.kernel.org; Hu Mingkai-B21284; Zang Roy-R61911
??: Re: [PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

On Thursday 04 September 2014 17:21:48 Fabio Estevam wrote:
> On Thu, Sep 4, 2014 at 3:45 PM, Minghuan Lian
> <Minghuan.Lian@freescale.com> wrote:
> > Add support for Freescale Layerscape PCIe controller. This driver
> > re-uses the designware core code.
>
> It seems that this is the same IP as on mx6, right?
>
> Is it possible to adapt drivers/pci/host/pci-imx6.c to work on Layerscape?

For all I can tell, the common parts are already shared in pcie-designware.c,
while the new file contains only those parts that are specific to layerscape
and different from imx.

If there are parts in there that are the same between layerscape and imx,
the implementation should also be moved to a shared file.

        Arnd

答复: [PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Minghuan.Lian at freescale.com]

Hi Fabio,

There are no same parts between Layerscape and imx6 except the same PCIe IP.
I think it is better Layerscape uses a separate file. 

Yes, I retrieve base and size from dtsi file, and then use them and defined PCIE_LS1021A_BASE to calculate controller index. Our SoC contains two PCI controllers.
I can remove  PCIE_LS1021A_BASE, but I must find a way to get controller index.
Because SCFG(The supplemental configuration unit, provides SoC specific configuration and status
registers for the device.) includes several separate register for PEX1 and PEX2. 
and I do not want to add several 'reg' entry to describe the SCFG PEX registers.

Thanks,
Minghaun 
________________________________________
???: Fabio Estevam <festevam@gmail.com>
????: 2014?9?5? 4:21
???: Lian Minghuan-B31939
??: linux-pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org; Zang Roy-R61911; Hu Mingkai-B21284; Wood Scott-B07421
??: Re: [PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

On Thu, Sep 4, 2014 at 3:45 PM, Minghuan Lian
<Minghuan.Lian@freescale.com> wrote:
> Add support for Freescale Layerscape PCIe controller. This driver
> re-uses the designware core code.

It seems that this is the same IP as on mx6, right?

Is it possible to adapt drivers/pci/host/pci-imx6.c to work on Layerscape?

> +#define PCIE_LS1021A_BASE      0x3400000
> +#define PCIE_LS1021A_REG_SIZE  0x0100000

You should retrieve base and reg size from the dtsi file.