pci: exynos: split into two parts such as Synopsys part and Exynos part

Exynos PCIe IP consists of Synopsys specific part and Exynos specific part. Only core block is a Synopsys designware part; other parts are Exynos specific.
Also, the Synopsys designware part can be shared with other platforms; thus, it can be split two parts such as Synopsys designware part and Exynos specific part.

Signed-off-by Jingoo Han <[email protected]>

Cc Pratyush Anand <[email protected]>

Cc Mohit KUMAR <[email protected]>

---
Changes since v2:

drivers/pci/host/pci-exynos.c new file +21 -0

+	struct resource *elbi_base;
+	struct resource *phy_base;
+	struct resource *block_base;
+	int ret;
+
+	elbi_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!elbi_base) {
+		dev_err(&pdev->dev, "couldn't get elbi base resource\n");
+		return -EINVAL;
+	}
+	exynos_pcie->elbi_base = devm_ioremap_resource(&pdev->dev, elbi_base);
+	if (IS_ERR(exynos_pcie->elbi_base))
+		return PTR_ERR(exynos_pcie->elbi_base);
+
+	phy_base = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (!phy_base) {
+		dev_err(&pdev->dev, "couldn't get phy base resource\n");
+		return -EINVAL;
+	}
+	exynos_pcie->phy_base = devm_ioremap_resource(&pdev->dev, phy_base);
+	if (IS_ERR(exynos_pcie->phy_base))

drivers/pci/host/pci-exynos.c new file +58 -0

+
+	pp->irq = platform_get_irq(pdev, 1);
+	if (!pp->irq) {
+		dev_err(&pdev->dev, "failed to get irq\n");
+		return -ENODEV;
+	}
+	ret = devm_request_irq(&pdev->dev, pp->irq, exynos_pcie_irq_handler,
+				IRQF_SHARED, "exynos-pcie", pp);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to request irq\n");
+		return ret;
+	}
+
+	pp->root_bus_nr = -1;
+	pp->ops = &exynos_pcie_host_ops;
+
+	spin_lock_init(&pp->conf_lock);
+	ret = dw_pcie_host_init(pp);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to initialize host\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int __init exynos_pcie_probe(struct platform_device *pdev)
+{
+	struct exynos_pcie *exynos_pcie;
+	struct pcie_port *pp;
+	struct device_node *np = pdev->dev.of_node;
+	int ret;
+
+	exynos_pcie = devm_kzalloc(&pdev->dev, sizeof(*exynos_pcie),
+				GFP_KERNEL);
+	if (!exynos_pcie) {
+		dev_err(&pdev->dev, "no memory for exynos pcie\n");
+		return -ENOMEM;
+	}
+
+	pp = &exynos_pcie->pp;
+
+	pp->dev = &pdev->dev;
+
+	exynos_pcie->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0);
+
+	exynos_pcie->clk = devm_clk_get(&pdev->dev, "pcie");
+	if (IS_ERR(exynos_pcie->clk)) {
+		dev_err(&pdev->dev, "Failed to get pcie rc clock\n");
+		return PTR_ERR(exynos_pcie->clk);
+	}
+	ret = clk_prepare_enable(exynos_pcie->clk);
+	if (ret)
+		return ret;
+
+	exynos_pcie->bus_clk = devm_clk_get(&pdev->dev, "pcie_bus");
+	if (IS_ERR(exynos_pcie->bus_clk)) {
+		dev_err(&pdev->dev, "Failed to get pcie bus clock\n");

drivers/pci/host/pci-exynos.c new file +56 -0

+
+	dw_pci.nr_controllers = 1;
+	dw_pci.private_data = (void **)&pp;
+
+	pci_common_init(&dw_pci);
+	pci_assign_unassigned_resources();
+#ifdef CONFIG_PCI_DOMAINS
+	dw_pci.domain++;
+#endif
+
+	platform_set_drvdata(pdev, exynos_pcie);
+	return 0;
+
+fail_bus_clk:
+	clk_disable_unprepare(exynos_pcie->bus_clk);
+fail_clk:
+	clk_disable_unprepare(exynos_pcie->clk);
+	return ret;
+}
+
+static int __exit exynos_pcie_remove(struct platform_device *pdev)
+{
+	struct exynos_pcie *exynos_pcie = platform_get_drvdata(pdev);
+
+	clk_disable_unprepare(exynos_pcie->bus_clk);
+	clk_disable_unprepare(exynos_pcie->clk);
+
+	return 0;
+}
+
+static const struct of_device_id exynos_pcie_of_match[] = {
+	{ .compatible = "samsung,exynos5440-pcie", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, exynos_pcie_of_match);
+
+static struct platform_driver exynos_pcie_driver = {
+	.remove		= __exit_p(exynos_pcie_remove),
+	.driver = {
+		.name	= "exynos-pcie",
+		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(exynos_pcie_of_match),
+	},
+};
+
+/* Exynos PCIe driver does not allow module unload */
+
+static int __init pcie_init(void)
+{
+	return platform_driver_probe(&exynos_pcie_driver, exynos_pcie_probe);
+}
+subsys_initcall(pcie_init);
+
+MODULE_AUTHOR("Jingoo Han <[email protected]>");
+MODULE_DESCRIPTION("Samsung PCIe host controller driver");
+MODULE_LICENSE("GPL v2");

drivers/pci/host/pcie-designware.c +44 -160

index 26bdbda..f097eff 100644
@@ -1,5 +1,5 @@
 /*
- * PCIe host controller driver for Samsung EXYNOS SoCs
+ * Synopsys Designware PCIe host controller driver
  *
  * Copyright (C) 2013 Samsung Electronics Co., Ltd.
  *		http://www.samsung.com

@@ -11,74 +11,28 @@
  * published by the Free Software Foundation.
  */
 
-#include <linux/clk.h>
-#include <linux/delay.h>
-#include <linux/gpio.h>
-#include <linux/interrupt.h>
 #include <linux/kernel.h>
-#include <linux/list.h>
 #include <linux/module.h>
-#include <linux/of.h>
 #include <linux/of_address.h>
-#include <linux/of_gpio.h>
-#include <linux/of_pci.h>
 #include <linux/pci.h>
 #include <linux/pci_regs.h>
-#include <linux/platform_device.h>
-#include <linux/resource.h>
-#include <linux/signal.h>
-#include <linux/slab.h>
 #include <linux/types.h>
 
-struct pcie_port_info {
-	u32		cfg0_size;
-	u32		cfg1_size;
-	u32		io_size;
-	u32		mem_size;
-	phys_addr_t	io_bus_addr;
-	phys_addr_t	mem_bus_addr;
-};
-
-struct pcie_port {
-	struct device		*dev;
-	u8			controller;
-	u8			root_bus_nr;
-	void __iomem		*dbi_base;
-	void __iomem		*elbi_base;
-	void __iomem		*phy_base;
-	void __iomem		*purple_base;
-	u64			cfg0_base;
-	void __iomem		*va_cfg0_base;
-	u64			cfg1_base;
-	void __iomem		*va_cfg1_base;
-	u64			io_base;
-	u64			mem_base;
-	spinlock_t		conf_lock;
-	struct resource		cfg;
-	struct resource		io;
-	struct resource		mem;
-	struct pcie_port_info	config;
-	struct clk		*clk;
-	struct clk		*bus_clk;
-	int			irq;
-	int			reset_gpio;
-};
-
-/*
- * Exynos PCIe IP consists of Synopsys specific part and Exynos
- * specific part. Only core block is a Synopsys designware part;
- * other parts are Exynos specific.
- */
+#include "pcie-designware.h"
 
 /* Synopsis specific PCIE configuration registers */
 #define PCIE_PORT_LINK_CONTROL		0x710
 #define PORT_LINK_MODE_MASK		(0x3f << 16)
+#define PORT_LINK_MODE_1_LANES		(0x1 << 16)
+#define PORT_LINK_MODE_2_LANES		(0x3 << 16)
 #define PORT_LINK_MODE_4_LANES		(0x7 << 16)
 
 #define PCIE_LINK_WIDTH_SPEED_CONTROL	0x80C
 #define PORT_LOGIC_SPEED_CHANGE		(0x1 << 17)
 #define PORT_LOGIC_LINK_WIDTH_MASK	(0x1ff << 8)
-#define PORT_LOGIC_LINK_WIDTH_4_LANES	(0x7 << 8)
+#define PORT_LOGIC_LINK_WIDTH_1_LANES	(0x1 << 8)
+#define PORT_LOGIC_LINK_WIDTH_2_LANES	(0x2 << 8)
+#define PORT_LOGIC_LINK_WIDTH_4_LANES	(0x4 << 8)
 
 #define PCIE_MSI_ADDR_LO		0x820
 #define PCIE_MSI_ADDR_HI		0x824

@@ -108,69 +62,14 @@ struct pcie_port {
 #define PCIE_ATU_FUNC(x)		(((x) & 0x7) << 16)
 #define PCIE_ATU_UPPER_TARGET		0x91C
 
-/* Exynos specific PCIE configuration registers */
-
-/* PCIe ELBI registers */
-#define PCIE_IRQ_PULSE			0x000
-#define IRQ_INTA_ASSERT			(0x1 << 0)
-#define IRQ_INTB_ASSERT			(0x1 << 2)
-#define IRQ_INTC_ASSERT			(0x1 << 4)
-#define IRQ_INTD_ASSERT			(0x1 << 6)
-#define PCIE_IRQ_LEVEL			0x004
-#define PCIE_IRQ_SPECIAL		0x008
-#define PCIE_IRQ_EN_PULSE		0x00c
-#define PCIE_IRQ_EN_LEVEL		0x010
-#define PCIE_IRQ_EN_SPECIAL		0x014
-#define PCIE_PWR_RESET			0x018
-#define PCIE_CORE_RESET			0x01c
-#define PCIE_CORE_RESET_ENABLE		(0x1 << 0)
-#define PCIE_STICKY_RESET		0x020
-#define PCIE_NONSTICKY_RESET		0x024
-#define PCIE_APP_INIT_RESET		0x028
-#define PCIE_APP_LTSSM_ENABLE		0x02c
-#define PCIE_ELBI_RDLH_LINKUP		0x064
-#define PCIE_ELBI_LTSSM_ENABLE		0x1
-#define PCIE_ELBI_SLV_AWMISC		0x11c
-#define PCIE_ELBI_SLV_ARMISC		0x120
-#define PCIE_ELBI_SLV_DBI_ENABLE	(0x1 << 21)
-
-/* PCIe Purple registers */
-#define PCIE_PHY_GLOBAL_RESET		0x000
-#define PCIE_PHY_COMMON_RESET		0x004
-#define PCIE_PHY_CMN_REG		0x008
-#define PCIE_PHY_MAC_RESET		0x00c
-#define PCIE_PHY_PLL_LOCKED		0x010
-#define PCIE_PHY_TRSVREG_RESET		0x020
-#define PCIE_PHY_TRSV_RESET		0x024
-
-/* PCIe PHY registers */
-#define PCIE_PHY_IMPEDANCE		0x004
-#define PCIE_PHY_PLL_DIV_0		0x008
-#define PCIE_PHY_PLL_BIAS		0x00c
-#define PCIE_PHY_DCC_FEEDBACK		0x014
-#define PCIE_PHY_PLL_DIV_1		0x05c
-#define PCIE_PHY_TRSV0_EMP_LVL		0x084
-#define PCIE_PHY_TRSV0_DRV_LVL		0x088
-#define PCIE_PHY_TRSV0_RXCDR		0x0ac
-#define PCIE_PHY_TRSV0_LVCC		0x0dc
-#define PCIE_PHY_TRSV1_EMP_LVL		0x144
-#define PCIE_PHY_TRSV1_RXCDR		0x16c
-#define PCIE_PHY_TRSV1_LVCC		0x19c
-#define PCIE_PHY_TRSV2_EMP_LVL		0x204
-#define PCIE_PHY_TRSV2_RXCDR		0x22c
-#define PCIE_PHY_TRSV2_LVCC		0x25c
-#define PCIE_PHY_TRSV3_EMP_LVL		0x2c4
-#define PCIE_PHY_TRSV3_RXCDR		0x2ec
-#define PCIE_PHY_TRSV3_LVCC		0x31c
-
-static struct hw_pci exynos_pci;
+unsigned long global_io_offset;
 
 static inline struct pcie_port *sys_to_pcie(struct pci_sys_data *sys)
 {
 	return sys->private_data;
 }
 
-static inline int cfg_read(void *addr, int where, int size, u32 *val)
+int cfg_read(void *addr, int where, int size, u32 *val)
 {
 	*val = readl(addr);
 

@@ -184,7 +83,7 @@ static inline int cfg_read(void *addr, int where, int size, u32 *val)
 	return PCIBIOS_SUCCESSFUL;
 }
 
-static inline int cfg_write(void *addr, int where, int size, u32 val)
+int cfg_write(void *addr, int where, int size, u32 val)
 {
 	if (size == 4)
 		writel(val, addr);

@@ -198,155 +97,230 @@ static inline int cfg_write(void *addr, int where, int size, u32 val)
 	return PCIBIOS_SUCCESSFUL;
 }
 
-static void exynos_pcie_sideband_dbi_w_mode(struct pcie_port *pp, bool on)
-{
-	u32 val;
-
-	if (on) {
-		val = readl(pp->elbi_base + PCIE_ELBI_SLV_AWMISC);
-		val |= PCIE_ELBI_SLV_DBI_ENABLE;
-		writel(val, pp->elbi_base + PCIE_ELBI_SLV_AWMISC);
-	} else {
-		val = readl(pp->elbi_base + PCIE_ELBI_SLV_AWMISC);
-		val &= ~PCIE_ELBI_SLV_DBI_ENABLE;
-		writel(val, pp->elbi_base + PCIE_ELBI_SLV_AWMISC);
-	}
-}
-
-static void exynos_pcie_sideband_dbi_r_mode(struct pcie_port *pp, bool on)
+static inline void dw_pcie_readl_rc(struct pcie_port *pp,
+				void __iomem *dbi_addr, u32 *val)
 {
-	u32 val;
-
-	if (on) {
-		val = readl(pp->elbi_base + PCIE_ELBI_SLV_ARMISC);
-		val |= PCIE_ELBI_SLV_DBI_ENABLE;
-		writel(val, pp->elbi_base + PCIE_ELBI_SLV_ARMISC);
-	} else {
-		val = readl(pp->elbi_base + PCIE_ELBI_SLV_ARMISC);
-		val &= ~PCIE_ELBI_SLV_DBI_ENABLE;
-		writel(val, pp->elbi_base + PCIE_ELBI_SLV_ARMISC);
-	}
-}
-
-static inline void readl_rc(struct pcie_port *pp, void *dbi_base, u32 *val)
-{
-	exynos_pcie_sideband_dbi_r_mode(pp, true);
-	*val = readl(dbi_base);
-	exynos_pcie_sideband_dbi_r_mode(pp, false);
-	return;
+	if (pp->ops->readl_rc)
+		pp->ops->readl_rc(pp, dbi_addr, val);
+	else
+		*val = readl(dbi_addr);
 }
 
-static inline void writel_rc(struct pcie_port *pp, u32 val, void *dbi_base)
+static inline void dw_pcie_writel_rc(struct pcie_port *pp,
+				u32 val, void __iomem *dbi_addr)
 {
-	exynos_pcie_sideband_dbi_w_mode(pp, true);
-	writel(val, dbi_base);
-	exynos_pcie_sideband_dbi_w_mode(pp, false);
-	return;
+	if (pp->ops->writel_rc)
+		pp->ops->writel_rc(pp, val, dbi_addr);
+	else
+		writel(val, dbi_addr);
 }
 
-static int exynos_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
+int dw_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
 				u32 *val)
 {
 	int ret;
 
-	exynos_pcie_sideband_dbi_r_mode(pp, true);
-	ret = cfg_read(pp->dbi_base + (where & ~0x3), where, size, val);
-	exynos_pcie_sideband_dbi_r_mode(pp, false);
+	if (pp->ops->rd_own_conf)
+		ret = pp->ops->rd_own_conf(pp, where, size, val);
+	else
+		ret = cfg_read(pp->dbi_base + (where & ~0x3), where, size, val);
+
 	return ret;
 }
 
-static int exynos_pcie_wr_own_conf(struct pcie_port *pp, int where, int size,
+int dw_pcie_wr_own_conf(struct pcie_port *pp, int where, int size,
 				u32 val)
 {
 	int ret;
 
-	exynos_pcie_sideband_dbi_w_mode(pp, true);
-	ret = cfg_write(pp->dbi_base + (where & ~0x3), where, size, val);
-	exynos_pcie_sideband_dbi_w_mode(pp, false);
+	if (pp->ops->wr_own_conf)
+		ret = pp->ops->wr_own_conf(pp, where, size, val);
+	else
+		ret = cfg_write(pp->dbi_base + (where & ~0x3), where, size,
+				val);
+
 	return ret;
 }
 
-static void exynos_pcie_prog_viewport_cfg0(struct pcie_port *pp, u32 busdev)
+int dw_pcie_link_up(struct pcie_port *pp)
+{
+	if (pp->ops->link_up)
+		return pp->ops->link_up(pp);
+	else
+		return 0;
+}
+
+int dw_pcie_host_init(struct pcie_port *pp)

drivers/pci/host/pcie-designware.c +22 -0

+
+	/* Get the I/O and memory ranges from DT */
+	for_each_of_pci_range(&parser, &range) {
+		unsigned long restype = range.flags & IORESOURCE_TYPE_BITS;
+		if (restype == IORESOURCE_IO) {
+			of_pci_range_to_resource(&range, np, &pp->io);
+			pp->io.name = "I/O";
+			pp->io.start = max_t(resource_size_t,
+					     PCIBIOS_MIN_IO,
+					     range.pci_addr + global_io_offset);
+			pp->io.end = min_t(resource_size_t,
+					   IO_SPACE_LIMIT,
+					   range.pci_addr + range.size
+					   + global_io_offset);
+			pp->config.io_size = resource_size(&pp->io);
+			pp->config.io_bus_addr = range.pci_addr;
+		}
+		if (restype == IORESOURCE_MEM) {
+			of_pci_range_to_resource(&range, np, &pp->mem);
+			pp->mem.name = "MEM";
+			pp->config.mem_size = resource_size(&pp->mem);
+			pp->config.mem_bus_addr = range.pci_addr;

Quoted 6 lines · Jingoo Han

Exynos PCIe IP consists of Synopsys specific part and Exynos specific part. Only core block is a Synopsys designware part; other parts are Exynos specific.
Also, the Synopsys designware part can be shared with other platforms; thus, it can be split two parts such as Synopsys designware part and Exynos specific part.

some more queries and comments..

Quoted 6 lines

Signed-off-by Jingoo Han <[email protected]>

Cc Pratyush Anand <[email protected]>

Cc Mohit KUMAR <[email protected]>

---
Changes since v2:

.
.
<snip>
.
.

drivers/pci/host/pci-exynos.c line 397

Quoted 13 lines

drivers/pci/host/pci-exynos.c new file +13 -0

+
+static struct pcie_host_ops exynos_pcie_host_ops = {
+	.readl_rc = exynos_pcie_readl_rc,
+	.writel_rc = exynos_pcie_writel_rc,
+	.rd_own_conf = exynos_pcie_rd_own_conf,
+	.wr_own_conf = exynos_pcie_wr_own_conf,
+	.link_up = exynos_pcie_link_up,
+	.host_init = exynos_pcie_host_init,
+};
+
+static int add_pcie_port(struct pcie_port *pp, struct platform_device *pdev)
+{
+	struct exynos_pcie *exynos_pcie = to_exynos_pcie(pp);

We can move the exynos_pcie specific initialization to probe and leave only pcie_port initialization here.

Quoted 25 lines

drivers/pci/host/pci-exynos.c new file +25 -0

+	struct resource *elbi_base;
+	struct resource *phy_base;
+	struct resource *block_base;
+	int ret;
+
+	elbi_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!elbi_base) {
+		dev_err(&pdev->dev, "couldn't get elbi base resource\n");
+		return -EINVAL;
+	}
+	exynos_pcie->elbi_base = devm_ioremap_resource(&pdev->dev, elbi_base);
+	if (IS_ERR(exynos_pcie->elbi_base))
+		return PTR_ERR(exynos_pcie->elbi_base);
+
+	phy_base = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (!phy_base) {
+		dev_err(&pdev->dev, "couldn't get phy base resource\n");
+		return -EINVAL;
+	}
+	exynos_pcie->phy_base = devm_ioremap_resource(&pdev->dev, phy_base);
+	if (IS_ERR(exynos_pcie->phy_base))
+		return PTR_ERR(exynos_pcie->phy_base);
+
+	block_base = platform_get_resource(pdev, IORESOURCE_MEM, 2);
+	if (!block_base) {

drivers/pci/host/pci-exynos.c line 428

Quoted 6 lines

drivers/pci/host/pci-exynos.c new file +6 -0

+		dev_err(&pdev->dev, "couldn't get block base resource\n");
+		return -EINVAL;
+	}
+	exynos_pcie->block_base = devm_ioremap_resource(&pdev->dev, block_base);
+	if (IS_ERR(exynos_pcie->block_base))
+		return PTR_ERR(exynos_pcie->block_base);

So all this till here can be moved to probe.

Quoted 63 lines

drivers/pci/host/pci-exynos.c new file +63 -0

+
+	pp->irq = platform_get_irq(pdev, 1);
+	if (!pp->irq) {
+		dev_err(&pdev->dev, "failed to get irq\n");
+		return -ENODEV;
+	}
+	ret = devm_request_irq(&pdev->dev, pp->irq, exynos_pcie_irq_handler,
+				IRQF_SHARED, "exynos-pcie", pp);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to request irq\n");
+		return ret;
+	}
+
+	pp->root_bus_nr = -1;
+	pp->ops = &exynos_pcie_host_ops;
+
+	spin_lock_init(&pp->conf_lock);
+	ret = dw_pcie_host_init(pp);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to initialize host\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int __init exynos_pcie_probe(struct platform_device *pdev)
+{
+	struct exynos_pcie *exynos_pcie;
+	struct pcie_port *pp;
+	struct device_node *np = pdev->dev.of_node;
+	int ret;
+
+	exynos_pcie = devm_kzalloc(&pdev->dev, sizeof(*exynos_pcie),
+				GFP_KERNEL);
+	if (!exynos_pcie) {
+		dev_err(&pdev->dev, "no memory for exynos pcie\n");
+		return -ENOMEM;
+	}
+
+	pp = &exynos_pcie->pp;
+
+	pp->dev = &pdev->dev;
+
+	exynos_pcie->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0);
+
+	exynos_pcie->clk = devm_clk_get(&pdev->dev, "pcie");
+	if (IS_ERR(exynos_pcie->clk)) {
+		dev_err(&pdev->dev, "Failed to get pcie rc clock\n");
+		return PTR_ERR(exynos_pcie->clk);
+	}
+	ret = clk_prepare_enable(exynos_pcie->clk);
+	if (ret)
+		return ret;
+
+	exynos_pcie->bus_clk = devm_clk_get(&pdev->dev, "pcie_bus");
+	if (IS_ERR(exynos_pcie->bus_clk)) {
+		dev_err(&pdev->dev, "Failed to get pcie bus clock\n");
+		ret = PTR_ERR(exynos_pcie->bus_clk);
+		goto fail_clk;
+	}
+	ret = clk_prepare_enable(exynos_pcie->bus_clk);
+	if (ret)

drivers/pci/host/pci-exynos.c line 496

Quoted 5 lines

drivers/pci/host/pci-exynos.c new file +5 -0

+		goto fail_clk;
+
+	ret = add_pcie_port(pp, pdev);
+	if (ret < 0)
+		goto fail_bus_clk;

I think we should move all the below code to designware core file. IMO it should be common everyone who use designware core.

Quoted 20 lines

drivers/pci/host/pci-exynos.c new file +20 -0

+
+	dw_pci.nr_controllers = 1;
+	dw_pci.private_data = (void **)&pp;
+
+	pci_common_init(&dw_pci);
+	pci_assign_unassigned_resources();
+#ifdef CONFIG_PCI_DOMAINS
+	dw_pci.domain++;
+#endif
+
+	platform_set_drvdata(pdev, exynos_pcie);
+	return 0;
+
+fail_bus_clk:
+	clk_disable_unprepare(exynos_pcie->bus_clk);
+fail_clk:
+	clk_disable_unprepare(exynos_pcie->clk);
+	return ret;
+}
+

Quoted 5 lines

drivers/pci/host/pcie-designware.c +5 -0

+int dw_pcie_host_init(struct pcie_port *pp)
+{
+	struct device_node *np = pp->dev->of_node;
+	struct of_pci_range range;
+	struct of_pci_range_parser parser;

drivers/pci/host/pcie-designware.c static inline int cfg_write(void *addr, int where, int size, u32 val) · line 163

Quoted 6 lines

drivers/pci/host/pcie-designware.c +6 -0

+	u32 val;
+
+	if (of_pci_range_parser_init(&parser, np)) {
+		dev_err(pp->dev, "missing ranges property\n");
+		return -EINVAL;
+	}

I have some confusion here w.r.t address space :-s

Quoted 26 lines

drivers/pci/host/pcie-designware.c +26 -0

+
+	/* Get the I/O and memory ranges from DT */
+	for_each_of_pci_range(&parser, &range) {
+		unsigned long restype = range.flags & IORESOURCE_TYPE_BITS;
+		if (restype == IORESOURCE_IO) {
+			of_pci_range_to_resource(&range, np, &pp->io);
+			pp->io.name = "I/O";
+			pp->io.start = max_t(resource_size_t,
+					     PCIBIOS_MIN_IO,
+					     range.pci_addr + global_io_offset);
+			pp->io.end = min_t(resource_size_t,
+					   IO_SPACE_LIMIT,
+					   range.pci_addr + range.size
+					   + global_io_offset);
+			pp->config.io_size = resource_size(&pp->io);
+			pp->config.io_bus_addr = range.pci_addr;
+		}
+		if (restype == IORESOURCE_MEM) {
+			of_pci_range_to_resource(&range, np, &pp->mem);
+			pp->mem.name = "MEM";
+			pp->config.mem_size = resource_size(&pp->mem);
+			pp->config.mem_bus_addr = range.pci_addr;
+		}
+		if (restype == 0) {
+			of_pci_range_to_resource(&range, np, &pp->cfg);
+			pp->config.cfg0_size = resource_size(&pp->cfg)/2;

drivers/pci/host/pcie-designware.c static inline int cfg_write(void *addr, int where, int size, u32 val) · line 195

Quoted 6 lines

drivers/pci/host/pcie-designware.c +6 -0

+			pp->config.cfg1_size = resource_size(&pp->cfg)/2;
+		}
+	}
+
+	pp->dbi_base = devm_ioremap(pp->dev, pp->cfg.start,
+				resource_size(&pp->cfg));

Why is configuraion space divided into two?

Why should it be same as dbi_base?
AFAIK, jacinto6 has a dedicated configuration/io/memory space that is entirely different from dbi_base.

Quoted 16 lines

Quoted 16 lines

+ if (restype == IORESOURCE_MEM) {

Quoted 14 lines

Quoted 14 lines

drivers/pci/host/pcie-designware.c +14 -0

+                   of_pci_range_to_resource(&range, np, &pp->mem);
+                   pp->mem.name = "MEM";
+                   pp->config.mem_size = resource_size(&pp->mem);
+                   pp->config.mem_bus_addr = range.pci_addr;
+           }
+           if (restype == 0) {
+                   of_pci_range_to_resource(&range, np, &pp->cfg);
+                   pp->config.cfg0_size = resource_size(&pp->cfg)/2;
+                   pp->config.cfg1_size = resource_size(&pp->cfg)/2;
+           }
+   }
+
+   pp->dbi_base = devm_ioremap(pp->dev, pp->cfg.start,
+                           resource_size(&pp->cfg));

Quoted 1 line

Quoted 1 line

Why is configuraion space divided into two?

Sorry, I don't know the exact reason.:(
Pratyush Anand may know about this.
Pratyush Anand, could you answer the question?

Also, if you find some problems, please let me know.

Quoted 3 lines

Quoted 3 lines

Why should it be same as dbi_base?
AFAIK, jacinto6 has a dedicated configuration/io/memory space that is entirely different from dbi_base.

According to the Synopsys designware PCIe datasheet,
in chapter 5.1.1 Register Space Layout,
'Port Logic Registers' are placed between (config space 0x0 + 0x700) and (config space 0x0 + 0xFFF).
'dbi_base' is used for reading/writing 'Port Logic Registers'. Exynos are using 'dbi_base' like this. Thus, the base addresses of both 'dbi_base' and configuration/io/memory space are same.

Just now, I looked at Spear PCIe driver.
However, in the case of Spear, the base address of configuration/io/memory space is defined as 0x80000000. The base address of 'Port Logic Registers' is defined as 0xb1000000.
I think that the situation of 'jacinto6' is similar with Spear, right?

Then, I will move pp->dbi_base mapping code from pcie-designware.c to pci-exynos.c.

Quoted 9 lines · Kishon Vijay Abraham I

Quoted 6 lines · Jingoo Han

Exynos PCIe IP consists of Synopsys specific part and Exynos specific part. Only core block is a Synopsys designware part; other parts are Exynos specific.
Also, the Synopsys designware part can be shared with other platforms; thus, it can be split two parts such as Synopsys designware part and Exynos specific part.

some more queries and comments..

Quoted 10 lines

drivers/pci/host/pcie-designware.c static inline int cfg_write(void *addr, int where, int size, u32 val) · line 195

Quoted 8 lines

drivers/pci/host/pcie-designware.c +8 -0

+			of_pci_range_to_resource(&range, np, &pp->cfg);
+			pp->config.cfg0_size = resource_size(&pp->cfg)/2;
+			pp->config.cfg1_size = resource_size(&pp->cfg)/2;
+		}
+	}
+
+	pp->dbi_base = devm_ioremap(pp->dev, pp->cfg.start,
+				resource_size(&pp->cfg));

Why is configuraion space divided into two?

Sorry, I don't know the exact reason. :(
Pratyush Anand may know about this.
Pratyush Anand, could you answer the question?

Also, if you find some problems, please let me know.

Quoted 3 lines

Why should it be same as dbi_base?
AFAIK, jacinto6 has a dedicated configuration/io/memory space that is entirely different from dbi_base.

According to the Synopsys designware PCIe datasheet,
in chapter 5.1.1 Register Space Layout,
'Port Logic Registers' are placed between (config space 0x0 + 0x700) and (config space 0x0 + 0xFFF).
'dbi_base' is used for reading/writing 'Port Logic Registers'. Exynos are using 'dbi_base' like this. Thus, the base addresses of both 'dbi_base' and configuration/io/memory space are same.

Just now, I looked at Spear PCIe driver.
However, in the case of Spear, the base address of configuration/io/memory space is defined as 0x80000000. The base address of 'Port Logic Registers' is defined as 0xb1000000.
I think that the situation of 'jacinto6' is similar with Spear, right?

Then, I will move pp->dbi_base mapping code from pcie-designware.c to pci-exynos.c.

Quoted 11 lines · Jingoo Han

Quoted 9 lines · Kishon Vijay Abraham I

Quoted 6 lines · Jingoo Han

Exynos PCIe IP consists of Synopsys specific part and Exynos specific part. Only core block is a Synopsys designware part; other parts are Exynos specific.
Also, the Synopsys designware part can be shared with other platforms; thus, it can be split two parts such as Synopsys designware part and Exynos specific part.

some more queries and comments..

.
.
<snip>
.
.

Quoted 36 lines

Quoted 10 lines

drivers/pci/host/pcie-designware.c static inline int cfg_write(void *addr, int where, int size, u32 val) · line 195

Quoted 8 lines

drivers/pci/host/pcie-designware.c +8 -0

+			of_pci_range_to_resource(&range, np, &pp->cfg);
+			pp->config.cfg0_size = resource_size(&pp->cfg)/2;
+			pp->config.cfg1_size = resource_size(&pp->cfg)/2;
+		}
+	}
+
+	pp->dbi_base = devm_ioremap(pp->dev, pp->cfg.start,
+				resource_size(&pp->cfg));

Why is configuraion space divided into two?

Sorry, I don't know the exact reason. :(
Pratyush Anand may know about this.
Pratyush Anand, could you answer the question?

Also, if you find some problems, please let me know.

Quoted 3 lines

Why should it be same as dbi_base?
AFAIK, jacinto6 has a dedicated configuration/io/memory space that is entirely different from dbi_base.

According to the Synopsys designware PCIe datasheet,
in chapter 5.1.1 Register Space Layout,
'Port Logic Registers' are placed between (config space 0x0 + 0x700) and (config space 0x0 + 0xFFF).
'dbi_base' is used for reading/writing 'Port Logic Registers'. Exynos are using 'dbi_base' like this. Thus, the base addresses of both 'dbi_base' and configuration/io/memory space are same.

Just now, I looked at Spear PCIe driver.
However, in the case of Spear, the base address of configuration/io/memory space is defined as 0x80000000. The base address of 'Port Logic Registers' is defined as 0xb1000000.
I think that the situation of 'jacinto6' is similar with Spear, right?

Quoted 2 lines

Then, I will move pp->dbi_base mapping code from pcie-designware.c to pci-exynos.c.

I think you need not move this to exynos (since registers in dbi_base is common for all platforms) but modify the pcie-designware.c to use different address space for dbi_base. In your case, you'll duplicate the address space for dbi_base and configuration space.

Also I have one more query.
In your dt binding, your pci address and cpu address is the same. But the pci address should start at 0x00000000 and end at 0xffffffff (for 32bit). Shouldn't the cpu address map to something within this range of pci address?

Quoted 5 lines

Also I have one more query.
In your dt binding, your pci address and cpu address is the same. But the pci address should start at 0x00000000 and end at 0xffffffff (for 32bit). Shouldn't the cpu address map to something within this range of pci address?

Sorry, I cannot answer it exactly.
DT binding was confirmed by Arnd Bergmann.
He will answer it exactly.

Quoted 28 lines · Kishon Vijay Abraham I

Quoted 11 lines · Jingoo Han

Quoted 9 lines · Kishon Vijay Abraham I

Quoted 6 lines · Jingoo Han

Exynos PCIe IP consists of Synopsys specific part and Exynos specific part. Only core block is a Synopsys designware part; other parts are Exynos specific.
Also, the Synopsys designware part can be shared with other platforms; thus, it can be split two parts such as Synopsys designware part and Exynos specific part.

some more queries and comments..

.
.
<snip>
.
.

Quoted 11 lines

Quoted 10 lines

drivers/pci/host/pcie-designware.c static inline int cfg_write(void *addr, int where, int size, u32 val) · line 195

Quoted 8 lines

drivers/pci/host/pcie-designware.c +8 -0

+			of_pci_range_to_resource(&range, np, &pp->cfg);
+			pp->config.cfg0_size = resource_size(&pp->cfg)/2;
+			pp->config.cfg1_size = resource_size(&pp->cfg)/2;
+		}
+	}
+
+	pp->dbi_base = devm_ioremap(pp->dev, pp->cfg.start,
+				resource_size(&pp->cfg));

Why is configuraion space divided into two?

Quoted 5 lines

Quoted 5 lines

Sorry, I don't know the exact reason. :(
Pratyush Anand may know about this.
Pratyush Anand, could you answer the question?

Also, if you find some problems, please let me know.

Quoted 17 lines

Quoted 17 lines

.
.

Quoted 15 lines

Quoted 10 lines

drivers/pci/host/pcie-designware.c static inline int cfg_write(void *addr, int where, int size, u32 val) · line 195

Quoted 8 lines

drivers/pci/host/pcie-designware.c +8 -0

+			of_pci_range_to_resource(&range, np, &pp->cfg);
+			pp->config.cfg0_size = resource_size(&pp->cfg)/2;
+			pp->config.cfg1_size = resource_size(&pp->cfg)/2;
+		}
+	}
+
+	pp->dbi_base = devm_ioremap(pp->dev, pp->cfg.start,
+				resource_size(&pp->cfg));

Why is configuraion space divided into two?

Sorry, I don't know the exact reason. :(
Pratyush Anand may know about this.
Pratyush Anand, could you answer the question?

Quoted 1 line

Quoted 1 line

Quoted 1 line

Also, if you find some problems, please let me know.

One more query..

Where is inbound translation configuration done in your driver? how should it be done?

Quoted 17 lines · Kishon Vijay Abraham I

Quoted 17 lines

Quoted 17 lines

.
.

Quoted 15 lines

Quoted 10 lines

Review

Quoted 8 lines

(quoted diff) +8 -0

+			of_pci_range_to_resource(&range, np, &pp->cfg);
+			pp->config.cfg0_size = resource_size(&pp->cfg)/2;
+			pp->config.cfg1_size = resource_size(&pp->cfg)/2;
+		}
+	}
+
+	pp->dbi_base = devm_ioremap(pp->dev, pp->cfg.start,
+				resource_size(&pp->cfg));

Why is configuraion space divided into two?

Sorry, I don't know the exact reason. :(
Pratyush Anand may know about this.
Pratyush Anand, could you answer the question?

Quoted 6 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

Also, if you find some problems, please let me know.

One more query..

Where is inbound translation configuration done in your driver? how should it be done?

Hi Kishon,

Sorry, I cannot understand your question exactly.
However, the following thread would be helpful.

http://www.spinics.net/lists/arm-kernel/msg252078.html
https://lkml.org/lkml/2013/6/17/890

From this conversation, It seems that you

Best regards.
Jingoo Han

Hi Jingoo,

Quoted 18 lines · Jingoo Han

Quoted 17 lines · Kishon Vijay Abraham I

Quoted 17 lines

Quoted 17 lines

.
.

Quoted 15 lines

Quoted 10 lines

Review

Quoted 8 lines

(quoted diff) +8 -0

+			of_pci_range_to_resource(&range, np, &pp->cfg);
+			pp->config.cfg0_size = resource_size(&pp->cfg)/2;
+			pp->config.cfg1_size = resource_size(&pp->cfg)/2;
+		}
+	}
+
+	pp->dbi_base = devm_ioremap(pp->dev, pp->cfg.start,
+				resource_size(&pp->cfg));

Why is configuraion space divided into two?

Sorry, I don't know the exact reason. :(
Pratyush Anand may know about this.
Pratyush Anand, could you answer the question?

Quoted 7 lines

Quoted 6 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

Also, if you find some problems, please let me know.

One more query..

Where is inbound translation configuration done in your driver? how should it be done?

Yes, Kishon is right. Inbound translation configuration is missing in your code and I think it should be implemented.

Quoted 7 lines

Hi Kishon,

Sorry, I cannot understand your question exactly.
However, the following thread would be helpful.

http://www.spinics.net/lists/arm-kernel/msg252078.html
https://lkml.org/lkml/2013/6/17/890

From this conversation, It seems that you
have tested this driver and it works fine without inbound translation function. I am sure that you would have tested a PCIe card with DMA capability such as PCIe2USB or PCIe2Ethernet. Since it worked, it means that by default your controller is supporting one to one mapping in case of inbound transaction even when address translation is enabled.

Hi,

Quoted 41 lines · Pratyush Anand

Hi Jingoo,

Quoted 18 lines · Jingoo Han

Quoted 17 lines · Kishon Vijay Abraham I

Quoted 17 lines

Quoted 17 lines

.
.

Quoted 15 lines

Quoted 10 lines

Review

Quoted 8 lines

(quoted diff) +8 -0

+			of_pci_range_to_resource(&range, np, &pp->cfg);
+			pp->config.cfg0_size = resource_size(&pp->cfg)/2;
+			pp->config.cfg1_size = resource_size(&pp->cfg)/2;
+		}
+	}
+
+	pp->dbi_base = devm_ioremap(pp->dev, pp->cfg.start,
+				resource_size(&pp->cfg));

Why is configuraion space divided into two?

Sorry, I don't know the exact reason. :(
Pratyush Anand may know about this.
Pratyush Anand, could you answer the question?

Quoted 7 lines

Quoted 6 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

Also, if you find some problems, please let me know.

One more query..

Where is inbound translation configuration done in your driver? how should it be done?

Yes, Kishon is right. Inbound translation configuration is missing in your code and I think it should be implemented.

Quoted 7 lines

Hi Kishon,

Sorry, I cannot understand your question exactly.
However, the following thread would be helpful.

http://www.spinics.net/lists/arm-kernel/msg252078.html
https://lkml.org/lkml/2013/6/17/890

Quoted 6 lines

From this conversation, It seems that you
have tested this driver and it works fine without inbound translation function. I am sure that you would have tested a PCIe card with DMA capability such as PCIe2USB or PCIe2Ethernet. Since it worked, it means that by default your controller is supporting one to one mapping in case of inbound transaction even when address translation is enabled.

btw, I'm testing Ethernet controller: Realtek Semiconductor Co., Ltd. RTL8111/8168B PCI Express Gigabit Ethernet controller.

when I do ifconfig eth0 up, I get *r8169 0000:01:00.0 eth0: link up.* But I dont receive any packets and ping also fails and the tx and rx packet count is also 0. Could it be related to inbound translation?

Thanks
Kishon

Hi Kishon,

Quoted 54 lines · Kishon Vijay Abraham I

Hi,

Quoted 41 lines · Pratyush Anand

Hi Jingoo,

Quoted 18 lines · Jingoo Han

Quoted 17 lines · Kishon Vijay Abraham I

Quoted 17 lines

Quoted 17 lines

.
.

Quoted 15 lines

Quoted 10 lines

Review

Quoted 8 lines

(quoted diff) +8 -0

+			of_pci_range_to_resource(&range, np, &pp->cfg);
+			pp->config.cfg0_size = resource_size(&pp->cfg)/2;
+			pp->config.cfg1_size = resource_size(&pp->cfg)/2;
+		}
+	}
+
+	pp->dbi_base = devm_ioremap(pp->dev, pp->cfg.start,
+				resource_size(&pp->cfg));

Why is configuraion space divided into two?

Sorry, I don't know the exact reason. :(
Pratyush Anand may know about this.
Pratyush Anand, could you answer the question?

Quoted 7 lines

Quoted 6 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

Also, if you find some problems, please let me know.

One more query..

Where is inbound translation configuration done in your driver? how should it be done?

Yes, Kishon is right. Inbound translation configuration is missing in your code and I think it should be implemented.

Quoted 7 lines

Hi Kishon,

Sorry, I cannot understand your question exactly.
However, the following thread would be helpful.

http://www.spinics.net/lists/arm-kernel/msg252078.html
https://lkml.org/lkml/2013/6/17/890

Quoted 6 lines

From this conversation, It seems that you
have tested this driver and it works fine without inbound translation function. I am sure that you would have tested a PCIe card with DMA capability such as PCIe2USB or PCIe2Ethernet. Since it worked, it means that by default your controller is supporting one to one mapping in case of inbound transaction even when address translation is enabled.

btw, I'm testing Ethernet controller: Realtek Semiconductor Co., Ltd. RTL8111/8168B PCI Express Gigabit Ethernet controller.

Quoted 3 lines

when I do ifconfig eth0 up, I get *r8169 0000:01:00.0 eth0: link up.* But I dont receive any packets and ping also fails and the tx and rx packet count is also 0. Could it be related to inbound translation?

A PCIe analyser log would tell a definite cause. Most likely either inbound translation is not working or INTx/MSI is not working.

Hi,

Quoted 13 lines · Pratyush Anand

Hi Kishon,

Quoted 10 lines · Kishon Vijay Abraham I

Hi,

Quoted 7 lines · Pratyush Anand

Hi Jingoo,

Quoted 3 lines · Jingoo Han

Quoted 2 lines · Kishon Vijay Abraham I

Quoted 2 lines

Quoted 2 lines

.
.

Quoted 6 lines

Quoted 3 lines

when I do ifconfig eth0 up, I get *r8169 0000:01:00.0 eth0: link up.* But I dont receive any packets and ping also fails and the tx and rx packet count is also 0. Could it be related to inbound translation?

A PCIe analyser log would tell a definite cause. Most likely either inbound translation is not working or INTx/MSI is not working.

I have enabled only legacy interrupts. Whenever I connect or disconnect ethernet cable I get link up/link down message and also the interrupt count for eth0 increases. So I'm not doubting INTx interrupts as such.

btw configuring inbound translation once in dw_pcie_host_init enough is it? I mean we use the same registers for configuring outbound translation also no? So doesn't the inbound configuration gets lost?

Thanks
Kishon

Quoted 16 lines · Kishon Vijay Abraham I

Hi,

Quoted 13 lines · Pratyush Anand

Hi Kishon,

Quoted 10 lines · Kishon Vijay Abraham I

Hi,

Quoted 7 lines · Pratyush Anand

Hi Jingoo,

Quoted 3 lines · Jingoo Han

Quoted 2 lines · Kishon Vijay Abraham I

Quoted 2 lines

Quoted 2 lines

.
.

[...]

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

when I do ifconfig eth0 up, I get *r8169 0000:01:00.0 eth0: link up.* But I dont receive any packets and ping also fails and the tx and rx packet count is also 0. Could it be related to inbound translation?

Quoted 6 lines

Quoted 2 lines

A PCIe analyser log would tell a definite cause. Most likely either inbound translation is not working or INTx/MSI is not working.

I have enabled only legacy interrupts. Whenever I connect or disconnect ethernet cable I get link up/link down message and also the interrupt count for eth0 increases. So I'm not doubting INTx interrupts as such.

Quoted 4 lines

btw configuring inbound translation once in dw_pcie_host_init enough is it? I mean we use the same registers for configuring outbound translation also no? So doesn't the inbound configuration gets lost?

No, you write at the same register, but you program direction as inbound. There are different resources for each viewport in inbound and outbound direction.

Regards
Pratyush

Quoted 3 lines

Thanks
Kishon

Quoted 24 lines · Pratyush Anand

Quoted 16 lines · Kishon Vijay Abraham I

Hi,

Quoted 13 lines · Pratyush Anand

Hi Kishon,

Quoted 10 lines · Kishon Vijay Abraham I

Hi,

Quoted 7 lines · Pratyush Anand

Hi Jingoo,

Quoted 3 lines · Jingoo Han

Quoted 2 lines · Kishon Vijay Abraham I

Quoted 2 lines

Quoted 2 lines

.
.

[...]

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

when I do ifconfig eth0 up, I get *r8169 0000:01:00.0 eth0: link up.* But I dont receive any packets and ping also fails and the tx and rx packet count is also 0. Could it be related to inbound translation?

Quoted 6 lines

Quoted 6 lines

Quoted 2 lines

A PCIe analyser log would tell a definite cause. Most likely either inbound translation is not working or INTx/MSI is not working.

I have enabled only legacy interrupts. Whenever I connect or disconnect ethernet cable I get link up/link down message and also the interrupt count for eth0 increases. So I'm not doubting INTx interrupts as such.

Just a question, what is the MRRS of your RC? if it is 128, can you

try with passing pci=pcie_bus_peer2peer in your bootargs.

Again, analyser will help a lot in diagnosing such issues.

Quoted 24 lines · Pratyush Anand

Quoted 16 lines · Kishon Vijay Abraham I

Hi,

Quoted 13 lines · Pratyush Anand

Hi Kishon,

Quoted 10 lines · Kishon Vijay Abraham I

Hi,

Quoted 7 lines · Pratyush Anand

Hi Jingoo,

Quoted 3 lines · Jingoo Han

Quoted 2 lines · Kishon Vijay Abraham I

Quoted 2 lines

Quoted 2 lines

.
.

[...]

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

when I do ifconfig eth0 up, I get *r8169 0000:01:00.0 eth0: link up.* But I dont receive any packets and ping also fails and the tx and rx packet count is also 0. Could it be related to inbound translation?

Quoted 6 lines

Quoted 6 lines

Quoted 2 lines

A PCIe analyser log would tell a definite cause. Most likely either inbound translation is not working or INTx/MSI is not working.

I have enabled only legacy interrupts. Whenever I connect or disconnect ethernet cable I get link up/link down message and also the interrupt count for eth0 increases. So I'm not doubting INTx interrupts as such.

Just a question, what is the MRRS of your RC? if it is 128, can you try with passing pci=pcie_bus_peer2peer in your bootargs.

Again, analyser will help a lot in diagnosing such issues.

Regards
Pratyush

Quoted 8 lines

Quoted 4 lines

btw configuring inbound translation once in dw_pcie_host_init enough is it? I mean we use the same registers for configuring outbound translation also no? So doesn't the inbound configuration gets lost?

No, you write at the same register, but you program direction as inbound. There are different resources for each viewport in inbound and outbound direction.

{
        u32 val;
        void __iomem *val1;
        void __iomem *dbi_base = pp->dbi_base;

        /* Program viewport 0 : INBOUND : MEMORY*/
        val = PCIE_ATU_REGION_INBOUND | (0 & 0xF);
        dw_pcie_writel_rc(pp, val, dbi_base + PCIE_ATU_VIEWPORT);
        val1 = ioremap(0x80000000, 0x5fffffff);

        dw_pcie_writel_rc(pp, 0x80000000, dbi_base + PCIE_ATU_LOWER_BASE);
        dw_pcie_writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_BASE);
        /* in_mem_size must be in power of 2 */
        dw_pcie_writel_rc(pp, 0x5FFFFFFF, dbi_base + PCIE_ATU_LIMIT);
        dw_pcie_writel_rc(pp, 0x80000000, dbi_base + PCIE_ATU_LOWER_TARGET);
        dw_pcie_writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_TARGET);

I somehow starting to doubt the DMA address programmed in the ethernet card which is in my RAM address range (0x80000000 to 0xBFFFFFFF). Should this address be programmed in the BAR of the ethernet card? How should it be done?

Quoted 9 lines · Kishon Vijay Abraham I

{
        u32 val;
        void __iomem *val1;
        void __iomem *dbi_base = pp->dbi_base;

        /* Program viewport 0 : INBOUND : MEMORY*/
        val = PCIE_ATU_REGION_INBOUND | (0 & 0xF);
        dw_pcie_writel_rc(pp, val, dbi_base + PCIE_ATU_VIEWPORT);
        val1 = ioremap(0x80000000, 0x5fffffff);

The ioremap here makes no sense at all, and I suspect it will fail anyway, because you exhaust the vmalloc area size, but since the value is not used anywhere, it won't matter.

Quoted 6 lines

        dw_pcie_writel_rc(pp, 0x80000000, dbi_base + PCIE_ATU_LOWER_BASE);
        dw_pcie_writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_BASE);
        /* in_mem_size must be in power of 2 */
        dw_pcie_writel_rc(pp, 0x5FFFFFFF, dbi_base + PCIE_ATU_LIMIT);
        dw_pcie_writel_rc(pp, 0x80000000, dbi_base + PCIE_ATU_LOWER_TARGET);
        dw_pcie_writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_TARGET);

These numbers need to come from somewhere, you shouldn't just hardcode them,

I guess you should either program an inbound window covering the entire 64-bit address space, or you should look at the top-level "memory" nodes to find the location of physical RAM.

I can't see anything wrong with the way it's set up though, unless you have an IOMMU. Can you confirm that there is no IOMMU (aka SMMU) in your system that handles the PCIe root complex?

Quoted 4 lines

I somehow starting to doubt the DMA address programmed in the ethernet card which is in my RAM address range (0x80000000 to 0xBFFFFFFF). Should this address be programmed in the BAR of the ethernet card? How should it be done?

No, it should not be in the BAR. The ethernet device driver calls dma_map_* or pci_map_* interfaces to get a valid token that can be passed into the device registers that are starting the DMA. You have to ensure that the dma_map_ops for the device return the value that is set up in the translation.

The normal case is an identity mapping between device DMA space and host memory space, i.e. PCIE_ATU_LOWER_TARGET == PCIE_ATU_LOWER_BASE, so in the dma_map_single implementation, phys_addr_t == dma_addr_t.

If you set up the dma_addr_t space to start at 0 instead, you have to add the offset in the dma_map_ops.

Hi Arnd,

Thanks for replying :-)

Quoted 19 lines · Arnd Bergmann

Quoted 9 lines · Kishon Vijay Abraham I

{
        u32 val;
        void __iomem *val1;
        void __iomem *dbi_base = pp->dbi_base;

        /* Program viewport 0 : INBOUND : MEMORY*/
        val = PCIE_ATU_REGION_INBOUND | (0 & 0xF);
        dw_pcie_writel_rc(pp, val, dbi_base + PCIE_ATU_VIEWPORT);
        val1 = ioremap(0x80000000, 0x5fffffff);

The ioremap here makes no sense at all, and I suspect it will fail anyway, because you exhaust the vmalloc area size, but since the value is not used anywhere, it won't matter.

Quoted 4 lines

        dw_pcie_writel_rc(pp, 0x80000000, dbi_base + PCIE_ATU_LOWER_BASE);
        dw_pcie_writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_BASE);
        /* in_mem_size must be in power of 2 */
        dw_pcie_writel_rc(pp, 0x5FFFFFFF, dbi_base + PCIE_ATU_LIMIT);

Quoted 4 lines

Quoted 2 lines

        dw_pcie_writel_rc(pp, 0x80000000, dbi_base + PCIE_ATU_LOWER_TARGET);
        dw_pcie_writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_TARGET);

These numbers need to come from somewhere, you shouldn't just hardcode them,

right. I'm still in the process of getting it work ;-)

Quoted 8 lines

I guess you should either program an inbound window covering the entire 64-bit address space, or you should look at the top-level "memory" nodes to find the location of physical RAM.

I can't see anything wrong with the way it's set up though, unless you have an IOMMU. Can you confirm that there is no IOMMU (aka SMMU) in your system that handles the PCIe root complex?

There is a MMU for PCIe root complex but that's disabled.

Quoted 14 lines

Quoted 4 lines

I somehow starting to doubt the DMA address programmed in the ethernet card which is in my RAM address range (0x80000000 to 0xBFFFFFFF). Should this address be programmed in the BAR of the ethernet card? How should it be done?

No, it should not be in the BAR. The ethernet device driver calls dma_map_* or pci_map_* interfaces to get a valid token that can be passed into the device registers that are starting the DMA. You have to ensure that the dma_map_ops for the device return the value that is set up in the translation.

The normal case is an identity mapping between device DMA space and host memory space, i.e. PCIE_ATU_LOWER_TARGET == PCIE_ATU_LOWER_BASE, so in the dma_map_single implementation, phys_addr_t == dma_addr_t.

Quoted 2 lines

If you set up the dma_addr_t space to start at 0 instead, you have to add the offset in the dma_map_ops.

My DMA address is in 0x80000000 to 0xBFFFFFFF range and I program my inbound translation for this range. Not sure what is missing still :-(

Thanks
Kishon

Hi Kishon,

Quoted 20 lines · Kishon Vijay Abraham I

Hi Arnd,

Thanks for replying :-)

Quoted 15 lines · Arnd Bergmann

Quoted 9 lines · Kishon Vijay Abraham I

{
        u32 val;
        void __iomem *val1;
        void __iomem *dbi_base = pp->dbi_base;

        /* Program viewport 0 : INBOUND : MEMORY*/
        val = PCIE_ATU_REGION_INBOUND | (0 & 0xF);
        dw_pcie_writel_rc(pp, val, dbi_base + PCIE_ATU_VIEWPORT);
        val1 = ioremap(0x80000000, 0x5fffffff);

The ioremap here makes no sense at all, and I suspect it will fail anyway, because you exhaust the vmalloc area size, but since the value is not used anywhere, it won't matter.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

        dw_pcie_writel_rc(pp, 0x80000000, dbi_base + PCIE_ATU_LOWER_BASE);
        dw_pcie_writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_BASE);
        /* in_mem_size must be in power of 2 */
        dw_pcie_writel_rc(pp, 0x5FFFFFFF, dbi_base + PCIE_ATU_LIMIT);

This is wrong. You should program here 0xBFFFFFFF.

Translation rule is as follows:

Region between "Start Address" and "End Address" is translated to "Target Address" with region size = "End Address" - "Start Address".
Where: Start Address = (PCIE_ATU_UPPER_BASE | PCIE_ATU_LOWER_BASE)
End Address = (PCIE_ATU_UPPER_BASE | PCIE_ATU_LIMIT) Target Address = (PCIE_ATU_UPPER_TARGET | PCIE_ATU_LOWER_TARGET)

Quoted 33 lines

Quoted 4 lines

Quoted 2 lines

        dw_pcie_writel_rc(pp, 0x80000000, dbi_base + PCIE_ATU_LOWER_TARGET);
        dw_pcie_writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_TARGET);

These numbers need to come from somewhere, you shouldn't just hardcode them,

right. I'm still in the process of getting it work ;-)

Quoted 8 lines

I guess you should either program an inbound window covering the entire 64-bit address space, or you should look at the top-level "memory" nodes to find the location of physical RAM.

I can't see anything wrong with the way it's set up though, unless you have an IOMMU. Can you confirm that there is no IOMMU (aka SMMU) in your system that handles the PCIe root complex?

There is a MMU for PCIe root complex but that's disabled.

Quoted 16 lines

Quoted 4 lines

I somehow starting to doubt the DMA address programmed in the ethernet card which is in my RAM address range (0x80000000 to 0xBFFFFFFF). Should this address be programmed in the BAR of the ethernet card? How should it be done?

No, it should not be in the BAR. The ethernet device driver calls dma_map_* or pci_map_* interfaces to get a valid token that can be passed into the device registers that are starting the DMA. You have to ensure that the dma_map_ops for the device return the value that is set up in the translation.

The normal case is an identity mapping between device DMA space and host memory space, i.e. PCIE_ATU_LOWER_TARGET == PCIE_ATU_LOWER_BASE, so in the dma_map_single implementation, phys_addr_t == dma_addr_t.

If you set up the dma_addr_t space to start at 0 instead, you have to add the offset in the dma_map_ops.

Quoted 2 lines

My DMA address is in 0x80000000 to 0xBFFFFFFF range and I program my inbound translation for this range. Not sure what is missing still :-(

Hope, above modification helps. Let me know.

Regards
Pratyush

Quoted 3 lines

Thanks
Kishon

Hi Pratyush,

Quoted 24 lines · Pratyush Anand

Hi Kishon,

Quoted 20 lines · Kishon Vijay Abraham I

Hi Arnd,

Thanks for replying :-)

Quoted 15 lines · Arnd Bergmann

Quoted 9 lines · Kishon Vijay Abraham I

{
        u32 val;
        void __iomem *val1;
        void __iomem *dbi_base = pp->dbi_base;

        /* Program viewport 0 : INBOUND : MEMORY*/
        val = PCIE_ATU_REGION_INBOUND | (0 & 0xF);
        dw_pcie_writel_rc(pp, val, dbi_base + PCIE_ATU_VIEWPORT);
        val1 = ioremap(0x80000000, 0x5fffffff);

The ioremap here makes no sense at all, and I suspect it will fail anyway, because you exhaust the vmalloc area size, but since the value is not used anywhere, it won't matter.

Quoted 6 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

        dw_pcie_writel_rc(pp, 0x80000000, dbi_base + PCIE_ATU_LOWER_BASE);
        dw_pcie_writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_BASE);
        /* in_mem_size must be in power of 2 */
        dw_pcie_writel_rc(pp, 0x5FFFFFFF, dbi_base + PCIE_ATU_LIMIT);

This is wrong. You should program here 0xBFFFFFFF.

That dint help :-(

Btw if we hadn't programmed inbound translation table, the address will go untranslated (according to the data book). I guess that's how it was working for Jingoo Han.

**
3.10.4
Inbound iATU Operation

When there is no match, then the address is untranslated **

Quoted 47 lines

Translation rule is as follows:

Region between "Start Address" and "End Address" is translated to "Target Address" with region size = "End Address" - "Start Address".
Where: Start Address = (PCIE_ATU_UPPER_BASE | PCIE_ATU_LOWER_BASE)
End Address = (PCIE_ATU_UPPER_BASE | PCIE_ATU_LIMIT) Target Address = (PCIE_ATU_UPPER_TARGET | PCIE_ATU_LOWER_TARGET)

Quoted 30 lines

Quoted 4 lines

Quoted 2 lines

        dw_pcie_writel_rc(pp, 0x80000000, dbi_base + PCIE_ATU_LOWER_TARGET);
        dw_pcie_writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_TARGET);

These numbers need to come from somewhere, you shouldn't just hardcode them,

right. I'm still in the process of getting it work ;-)

Quoted 8 lines

I guess you should either program an inbound window covering the entire 64-bit address space, or you should look at the top-level "memory" nodes to find the location of physical RAM.

I can't see anything wrong with the way it's set up though, unless you have an IOMMU. Can you confirm that there is no IOMMU (aka SMMU) in your system that handles the PCIe root complex?

There is a MMU for PCIe root complex but that's disabled.

Quoted 14 lines

Quoted 4 lines

I somehow starting to doubt the DMA address programmed in the ethernet card which is in my RAM address range (0x80000000 to 0xBFFFFFFF). Should this address be programmed in the BAR of the ethernet card? How should it be done?

No, it should not be in the BAR. The ethernet device driver calls dma_map_* or pci_map_* interfaces to get a valid token that can be passed into the device registers that are starting the DMA. You have to ensure that the dma_map_ops for the device return the value that is set up in the translation.

The normal case is an identity mapping between device DMA space and host memory space, i.e. PCIE_ATU_LOWER_TARGET == PCIE_ATU_LOWER_BASE, so in the dma_map_single implementation, phys_addr_t == dma_addr_t.

Quoted 5 lines

Quoted 2 lines

If you set up the dma_addr_t space to start at 0 instead, you have to add the offset in the dma_map_ops.

My DMA address is in 0x80000000 to 0xBFFFFFFF range and I program my inbound translation for this range. Not sure what is missing still :-(

Hope, above modification helps. Let me know.

Quoted 5 lines

Regards
Pratyush

Quoted 3 lines

Thanks
Kishon

Thanks
Kishon

Btw if we hadn't programmed inbound translation table, the address will go untranslated (according to the data book). I guess that's how it was working for Jingoo Han.

**
3.10.4
Inbound iATU Operation

When there is no match, then the address is untranslated **

Quoted 8 lines · Kishon Vijay Abraham I

Btw if we hadn't programmed inbound translation table, the address will go untranslated (according to the data book). I guess that's how it was working for Jingoo Han.

**
3.10.4
Inbound iATU Operation

Quoted 3 lines

When there is no match, then the address is untranslated **

Well, that should work just as well, since you have a 1:1 translation anyway. Do you get the same error without the translation?

Quoted 9 lines · Arnd Bergmann

Quoted 8 lines · Kishon Vijay Abraham I

Btw if we hadn't programmed inbound translation table, the address will go untranslated (according to the data book). I guess that's how it was working for Jingoo Han.

**
3.10.4
Inbound iATU Operation

Quoted 6 lines

Quoted 3 lines

When there is no match, then the address is untranslated **

Well, that should work just as well, since you have a 1:1 translation anyway. Do you get the same error without the translation?

Yes. I get the same non-fatal error interrupt in RC.

Quoted 10 lines · Kishon Vijay Abraham I

Quoted 9 lines · Arnd Bergmann

Quoted 8 lines · Kishon Vijay Abraham I

Btw if we hadn't programmed inbound translation table, the address will go untranslated (according to the data book). I guess that's how it was working for Jingoo Han.

**
3.10.4
Inbound iATU Operation

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

When there is no match, then the address is untranslated **

Well, that should work just as well, since you have a 1:1 translation anyway. Do you get the same error without the translation?

Yes. I get the same non-fatal error interrupt in RC.

Ok, then I guess the translation is actually not at fault here but something else. I would recommend looking at the IOMMU as the potential culprit. Maybe having it disabled means that no DMA is going through, rather than all DMA going through untranslated. Another possibility is that the IOMMU is set up so that when disabled, it maps DMA address 0 to the start of RAM, rather than identity mapping DMA address 0x80000000 there. If that's the case, you either have to use the IOMMU, or set up the mapping in the root complex to revert it.

Quoted 7 lines · Pratyush Anand

From this conversation, It seems that you
have tested this driver and it works fine without inbound translation function. I am sure that you would have tested a PCIe card with DMA capability such as PCIe2USB or PCIe2Ethernet. Since it worked, it means that by default your controller is supporting one to one mapping in case of inbound transaction even when address translation is enabled.

btw, I'm testing Ethernet controller: Realtek Semiconductor Co., Ltd. RTL8111/8168B PCI Express Gigabit Ethernet controller.

when I do ifconfig eth0 up, I get *r8169 0000:01:00.0 eth0: link up.* But I dont receive any packets and ping also fails and the tx and rx packet count is also 0. Could it be related to inbound translation?

Quoted 11 lines · Jingoo Han

Quoted 9 lines · Kishon Vijay Abraham I

Quoted 6 lines · Jingoo Han

Exynos PCIe IP consists of Synopsys specific part and Exynos specific part. Only core block is a Synopsys designware part; other parts are Exynos specific.
Also, the Synopsys designware part can be shared with other platforms; thus, it can be split two parts such as Synopsys designware part and Exynos specific part.

some more queries and comments..

.
.
<snip>
.
.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/pci/host/pcie-designware.c +8 -0

+			of_pci_range_to_resource(&range, np, &pp->cfg);
+			pp->config.cfg0_size = resource_size(&pp->cfg)/2;
+			pp->config.cfg1_size = resource_size(&pp->cfg)/2;
+		}
+	}
+
+	pp->dbi_base = devm_ioremap(pp->dev, pp->cfg.start,
+				resource_size(&pp->cfg));

Quoted 19 lines

Quoted 4 lines

Why should it be same as dbi_base?
AFAIK, jacinto6 has a dedicated configuration/io/memory space that is entirely different from dbi_base.

According to the Synopsys designware PCIe datasheet,
in chapter 5.1.1 Register Space Layout,
'Port Logic Registers' are placed between (config space 0x0 + 0x700) and (config space 0x0 + 0xFFF).
'dbi_base' is used for reading/writing 'Port Logic Registers'. Exynos are using 'dbi_base' like this. Thus, the base addresses of both 'dbi_base' and configuration/io/memory space are same.

Just now, I looked at Spear PCIe driver.
However, in the case of Spear, the base address of configuration/io/memory space is defined as 0x80000000. The base address of 'Port Logic Registers' is defined as 0xb1000000.
I think that the situation of 'jacinto6' is similar with Spear, right?

Quoted 2 lines

Then, I will move pp->dbi_base mapping code from pcie-designware.c to pci-exynos.c.

I think you need not move this to exynos (since registers in dbi_base is common for all platforms) but modify the pcie-designware.c to use different address space for dbi_base. In your case, you'll duplicate the address space for dbi_base and configuration space.