2014-01-14 23:34:44

by Tanmay Inamdar

[permalink] [raw]
Subject: [RFC PATCH V2 0/4] APM X-Gene PCIe controller

This patch adds support for AppliedMicro X-Gene PCIe host controller. The
driver is tested on X-Gene platform with different gen1/2/3 PCIe endpoint
cards.

X-Gene PCIe controller driver has depedency on the pcie arch support for
arm64. The arm64 pcie arch support is not yet part of mainline Linux kernel
and approach for arch support is under discussion with arm64 maintainers.
The reference patch can be found here --> https://lkml.org/lkml/2013/10/23/244

If someone wishes to test PCIe on X-Gene, arch support patch must be applied
before the patches in this patch set.

changes since V1:
1. added PCI domain support
2. reading cpu and pci addresses from device tree to configure regions.
3. got rid of unnecessary wrappers for readl and writel.
4. got rid of endpoint configuration code.
5. added 'dma-ranges' property support to read inbound region configuration.
6. renamed host driver file to 'pci-xgene.c' from 'pcie-xgene.c'
7. dropped 'clock-names' property from bindings
8. added comments whereever requested.

Tanmay Inamdar (4):
pci: APM X-Gene PCIe controller driver
arm64:dts: APM X-Gene PCIe device tree nodes
dt-bindings: pci: xgene pcie device tree bindings
MAINTAINERS: entry for APM X-Gene PCIe host driver

.../devicetree/bindings/pci/xgene-pcie.txt | 45 +
MAINTAINERS | 7 +
arch/arm64/boot/dts/apm-mustang.dts | 4 +
arch/arm64/boot/dts/apm-storm.dtsi | 144 +++
drivers/pci/host/Kconfig | 10 +
drivers/pci/host/Makefile | 1 +
drivers/pci/host/pci-xgene.c | 934 ++++++++++++++++++++
7 files changed, 1145 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pci/xgene-pcie.txt
create mode 100644 drivers/pci/host/pci-xgene.c

--
1.7.9.5


2014-01-14 23:34:52

by Tanmay Inamdar

[permalink] [raw]
Subject: [RFC PATCH V2 1/4] pci: APM X-Gene PCIe controller driver

This patch adds the AppliedMicro X-Gene SOC PCIe controller driver.
X-Gene PCIe controller supports maxmum upto 8 lanes and GEN3 speed.
X-Gene has maximum 5 PCIe ports supported.

Signed-off-by: Tanmay Inamdar <[email protected]>
---
drivers/pci/host/Kconfig | 10 +
drivers/pci/host/Makefile | 1 +
drivers/pci/host/pci-xgene.c | 934 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 945 insertions(+)
create mode 100644 drivers/pci/host/pci-xgene.c

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index 47d46c6..19ce97d 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -33,4 +33,14 @@ config PCI_RCAR_GEN2
There are 3 internal PCI controllers available with a single
built-in EHCI/OHCI host controller present on each one.

+config PCI_XGENE
+ bool "X-Gene PCIe controller"
+ depends on ARCH_XGENE
+ depends on OF
+ select PCIEPORTBUS
+ help
+ Say Y here if you want internal PCI support on APM X-Gene SoC.
+ There are 5 internal PCIe ports available. Each port is GEN3 capable
+ and have varied lanes from x1 to x8.
+
endmenu
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 13fb333..34c7c36 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -4,3 +4,4 @@ obj-$(CONFIG_PCI_IMX6) += pci-imx6.o
obj-$(CONFIG_PCI_MVEBU) += pci-mvebu.o
obj-$(CONFIG_PCI_TEGRA) += pci-tegra.o
obj-$(CONFIG_PCI_RCAR_GEN2) += pci-rcar-gen2.o
+obj-$(CONFIG_PCI_XGENE) += pci-xgene.o
diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
new file mode 100644
index 0000000..54b2d4f
--- /dev/null
+++ b/drivers/pci/host/pci-xgene.c
@@ -0,0 +1,934 @@
+/**
+ * APM X-Gene PCIe Driver
+ *
+ * Copyright (c) 2013 Applied Micro Circuits Corporation.
+ *
+ * Author: Tanmay Inamdar <[email protected]>.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+#include <linux/clk-private.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/jiffies.h>
+#include <linux/memblock.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_pci.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <asm/pcibios.h>
+
+#define PCIECORE_LTSSM 0x4c
+#define PCIECORE_CTLANDSTATUS 0x50
+#define PIPE_PHY_RATE_RD(src) ((0xc000 & (u32)(src)) >> 0xe)
+#define INTXSTATUSMASK 0x6c
+#define PIM1_1L 0x80
+#define IBAR2 0x98
+#define IR2MSK 0x9c
+#define PIM2_1L 0xa0
+#define IBAR3L 0xb4
+#define IR3MSKL 0xbc
+#define PIM3_1L 0xc4
+#define OMR1BARL 0x100
+#define OMR2BARL 0x118
+#define CFGBARL 0x154
+#define CFGBARH 0x158
+#define CFGCTL 0x15c
+#define RTDID 0x160
+#define BRIDGE_CFG_0 0x2000
+#define BRIDGE_CFG_1 0x2004
+#define BRIDGE_CFG_4 0x2010
+#define BRIDGE_CFG_32 0x2030
+#define BRIDGE_CFG_14 0x2038
+#define BRIDGE_8G_CFG_0 0x2100
+#define BRIDGE_8G_CFG_4 0x2110
+#define BRIDGE_8G_CFG_8 0x2120
+#define BRIDGE_8G_CFG_9 0x2124
+#define BRIDGE_8G_CFG_10 0x2128
+#define BRIDGE_8G_CFG_11 0x212c
+#define BRIDGE_CTRL_1 0x2204
+#define BRIDGE_CTRL_2 0x2208
+#define BRIDGE_CTRL_5 0x2214
+#define BRIDGE_STATUS_0 0x2600
+#define MEM_RAM_SHUTDOWN 0xd070
+#define BLOCK_MEM_RDY 0xd074
+
+#define PCI_PRIMARY_BUS_MASK 0x00ffffff
+#define REVISION_ID_MASK 0x000000ff
+#define SLOT_IMPLEMENTED_MASK 0x04000000
+#define DEVICE_PORT_TYPE_MASK 0x03c00000
+#define ADVT_INFINITE_CREDITS 0x00000200
+#define PM_FORCE_RP_MODE_MASK 0x00000400
+#define SWITCH_PORT_MODE_MASK 0x00000800
+#define CLASS_CODE_MASK 0xffffff00
+#define LINK_UP_MASK 0x00000100
+#define AER_OPTIONAL_ERROR_EN 0xffc00000
+#define DWNSTRM_EQ_SKP_PHS_2_3 0x00010000
+#define DIRECT_TO_5GTS_MASK 0x00020000
+#define SUPPORT_5GTS_MASK 0x00010000
+#define DIRECT_TO_8GTS_MASK 0x00008000
+#define SUPPORT_8GTS_MASK 0x00004000
+#define XGENE_PCIE_DEV_CTRL 0x2f0f
+#define AXI_EP_CFG_ACCESS 0x10000
+#define ENABLE_ASPM 0x08000000
+#define XGENE_PORT_TYPE_RC 0x05000000
+#define BLOCK_MEM_RDY_VAL 0xFFFFFFFF
+#define EN_COHERENCY 0xF0000000
+#define EN_REG 0x00000001
+#define OB_LO_IO 0x00000002
+#define XGENE_PCIE_VENDORID 0xE008
+#define XGENE_PCIE_DEVICEID 0xE004
+#define XGENE_PCIE_TIMEOUT (500*1000) /* us */
+#define XGENE_LTSSM_DETECT_WAIT 20
+#define XGENE_LTSSM_L0_WAIT 4
+#define XGENE_PCIE_MAX_PORTS 5
+#define SZ_1T (SZ_1G*1024ULL)
+
+struct xgene_res_cfg {
+ struct resource res;
+ u64 pci_addr;
+};
+
+struct xgene_pcie_port {
+ struct device_node *node;
+ struct xgene_res_cfg mem;
+ struct xgene_res_cfg io;
+ u8 link_up;
+ u8 link_speed;
+ u32 first_busno;
+ void *csr_base;
+ void *cfg_base;
+ u64 cfg_addr;
+ struct device *dev;
+ struct clk *clk;
+};
+
+static inline u32 pcie_bar_low_val(u32 addr, u32 flags)
+{
+ return (addr & PCI_BASE_ADDRESS_MEM_MASK) | flags;
+}
+
+static inline u32 eq_pre_cursor_lane0_set(u32 dst, u32 src)
+{
+ return (dst & ~0xff) | (src & 0xff);
+}
+
+static inline u32 eq_pre_cursor_lane1_set(u32 dst, u32 src)
+{
+ return (dst & ~0xff0000) | ((src << 0x10) & 0xff0000);
+}
+
+static inline struct xgene_pcie_port *
+xgene_pcie_sys_to_port(struct pci_sys_data *sys)
+{
+ return sys->private_data;
+}
+
+static inline struct xgene_pcie_port *
+xgene_pcie_bus_to_port(struct pci_bus *bus)
+{
+ struct pci_sys_data *sys = bus->sysdata;
+ return xgene_pcie_sys_to_port(sys);
+}
+
+/* PCIE Configuration Out/In */
+static inline void xgene_pcie_cfg_out32(void *addr, u32 val)
+{
+ writel(val, addr);
+}
+
+static inline void xgene_pcie_cfg_out16(void *addr, u16 val)
+{
+ u64 temp_addr = (u64) addr & ~0x3;
+ u32 val32 = readl((void *)temp_addr);
+
+ switch ((u64) addr & 0x3) {
+ case 2:
+ val32 &= ~0xFFFF0000;
+ val32 |= (u32) val << 16;
+ break;
+ case 0:
+ default:
+ val32 &= ~0xFFFF;
+ val32 |= val;
+ break;
+ }
+ writel(val32, (void *)temp_addr);
+}
+
+static inline void xgene_pcie_cfg_out8(void *addr, u8 val)
+{
+ phys_addr_t temp_addr = (u64) addr & ~0x3;
+ u32 val32 = readl((void *)temp_addr);
+
+ switch ((u64) addr & 0x3) {
+ case 0:
+ val32 &= ~0xFF;
+ val32 |= val;
+ break;
+ case 1:
+ val32 &= ~0xFF00;
+ val32 |= (u32) val << 8;
+ break;
+ case 2:
+ val32 &= ~0xFF0000;
+ val32 |= (u32) val << 16;
+ break;
+ case 3:
+ default:
+ val32 &= ~0xFF000000;
+ val32 |= (u32) val << 24;
+ break;
+ }
+ writel(val32, (void *)temp_addr);
+}
+
+static inline void xgene_pcie_cfg_in32(void *addr, u32 *val)
+{
+ *val = readl(addr);
+}
+
+static inline void xgene_pcie_cfg_in16(void *addr, u16 *val)
+{
+ u64 temp_addr = (u64)addr & ~0x3;
+ u32 val32;
+
+ val32 = readl((void *)temp_addr);
+
+ switch ((u64)addr & 0x3) {
+ case 2:
+ *val = val32 >> 16;
+ break;
+ case 0:
+ default:
+ *val = val32;
+ break;
+ }
+}
+
+static inline void xgene_pcie_cfg_in8(void *addr, u8 *val)
+{
+ u64 temp_addr = (u64)addr & ~0x3;
+ u32 val32;
+
+ val32 = readl((void *)temp_addr);
+
+ switch ((u64)addr & 0x3) {
+ case 3:
+ *val = val32 >> 24;
+ break;
+ case 2:
+ *val = val32 >> 16;
+ break;
+ case 1:
+ *val = val32 >> 8;
+ break;
+ case 0:
+ default:
+ *val = val32;
+ break;
+ }
+}
+
+/* When the address bit [17:16] is 2'b01, the Configuration access will be
+ * treated as Type 1 and it will be forwarded to external PCIe device.
+ */
+static void __iomem *xgene_pcie_get_cfg_base(struct pci_bus *bus)
+{
+ struct xgene_pcie_port *port = xgene_pcie_bus_to_port(bus);
+ u64 addr = (u64)port->cfg_base;
+
+ if (bus->number >= (port->first_busno + 1))
+ addr |= AXI_EP_CFG_ACCESS;
+
+ return (void *)addr;
+}
+
+/* For Configuration request, RTDID register is used as Bus Number,
+ * Device Number and Function number of the header fields.
+ */
+static void xgene_pcie_set_rtdid_reg(struct pci_bus *bus, uint devfn)
+{
+ struct xgene_pcie_port *port = xgene_pcie_bus_to_port(bus);
+ unsigned int b, d, f;
+ u32 rtdid_val = 0;
+
+ b = bus->number;
+ d = PCI_SLOT(devfn);
+ f = PCI_FUNC(devfn);
+
+ if (bus->number == port->first_busno)
+ rtdid_val = (b << 24) | (d << 19) | (f << 16);
+ else if (bus->number >= (port->first_busno + 1))
+ rtdid_val = (port->first_busno << 24) |
+ (b << 8) | (d << 3) | f;
+
+ writel(rtdid_val, port->csr_base + RTDID);
+ /* read the register back to ensure flush */
+ readl(port->csr_base + RTDID);
+}
+
+static int xgene_pcie_read_config(struct pci_bus *bus, unsigned int devfn,
+ int offset, int len, u32 *val)
+{
+ struct xgene_pcie_port *port = xgene_pcie_bus_to_port(bus);
+ void __iomem *addr;
+ u8 val8;
+ u16 val16;
+
+ if ((pci_is_root_bus(bus) && devfn != 0) || !port->link_up)
+ return PCIBIOS_DEVICE_NOT_FOUND;
+
+ xgene_pcie_set_rtdid_reg(bus, devfn);
+ addr = xgene_pcie_get_cfg_base(bus);
+ switch (len) {
+ case 1:
+ xgene_pcie_cfg_in8(addr + offset, &val8);
+ *val = val8;
+ break;
+ case 2:
+ xgene_pcie_cfg_in16(addr + offset, &val16);
+ *val = val16;
+ break;
+ default:
+ xgene_pcie_cfg_in32(addr + offset, val);
+ break;
+ }
+ return PCIBIOS_SUCCESSFUL;
+}
+
+static int xgene_pcie_write_config(struct pci_bus *bus, unsigned int devfn,
+ int offset, int len, u32 val)
+{
+ struct xgene_pcie_port *port = xgene_pcie_bus_to_port(bus);
+ void __iomem *addr;
+
+ if ((pci_is_root_bus(bus) && devfn != 0) || !port->link_up)
+ return PCIBIOS_DEVICE_NOT_FOUND;
+
+ xgene_pcie_set_rtdid_reg(bus, devfn);
+ addr = xgene_pcie_get_cfg_base(bus);
+ switch (len) {
+ case 1:
+ xgene_pcie_cfg_out8(addr + offset, (u8) val);
+ break;
+ case 2:
+ xgene_pcie_cfg_out16(addr + offset, (u16) val);
+ break;
+ default:
+ xgene_pcie_cfg_out32(addr + offset, val);
+ break;
+ }
+ return PCIBIOS_SUCCESSFUL;
+}
+
+static struct pci_ops xgene_pcie_ops = {
+ .read = xgene_pcie_read_config,
+ .write = xgene_pcie_write_config
+};
+
+static void xgene_pcie_setup_lanes(struct xgene_pcie_port *port)
+{
+ void *csr_base = port->csr_base;
+ u32 val;
+
+ val = readl(csr_base + BRIDGE_8G_CFG_8);
+ val = eq_pre_cursor_lane0_set(val, 0x7);
+ val = eq_pre_cursor_lane1_set(val, 0x7);
+ writel(val, csr_base + BRIDGE_8G_CFG_8);
+
+ val = readl(csr_base + BRIDGE_8G_CFG_9);
+ val = eq_pre_cursor_lane0_set(val, 0x7);
+ val = eq_pre_cursor_lane1_set(val, 0x7);
+ writel(val, csr_base + BRIDGE_8G_CFG_9);
+
+ val = readl(csr_base + BRIDGE_8G_CFG_10);
+ val = eq_pre_cursor_lane0_set(val, 0x7);
+ val = eq_pre_cursor_lane1_set(val, 0x7);
+ writel(val, csr_base + BRIDGE_8G_CFG_10);
+
+ val = readl(csr_base + BRIDGE_8G_CFG_11);
+ val = eq_pre_cursor_lane0_set(val, 0x7);
+ val = eq_pre_cursor_lane1_set(val, 0x7);
+ writel(val, csr_base + BRIDGE_8G_CFG_11);
+
+ val = readl(csr_base + BRIDGE_8G_CFG_4);
+ val = (val & ~0x30) | (1 << 4);
+ writel(val, csr_base + BRIDGE_8G_CFG_4);
+}
+
+static void xgene_pcie_setup_link(struct xgene_pcie_port *port)
+{
+ void *csr_base = port->csr_base;
+ u32 val;
+
+ val = readl(csr_base + BRIDGE_CFG_14);
+ val |= DIRECT_TO_8GTS_MASK;
+ val |= SUPPORT_5GTS_MASK;
+ val |= SUPPORT_8GTS_MASK;
+ val |= DIRECT_TO_5GTS_MASK;
+ writel(val, csr_base + BRIDGE_CFG_14);
+
+ val = readl(csr_base + BRIDGE_CFG_14);
+ val &= ~ADVT_INFINITE_CREDITS;
+ writel(val, csr_base + BRIDGE_CFG_14);
+
+ val = readl(csr_base + BRIDGE_8G_CFG_0);
+ val |= (val & ~0xf) | 7;
+ val |= (val & ~0xf00) | ((7 << 8) & 0xf00);
+ writel(val, csr_base + BRIDGE_8G_CFG_0);
+
+ val = readl(csr_base + BRIDGE_8G_CFG_0);
+ val |= DWNSTRM_EQ_SKP_PHS_2_3;
+ writel(val, csr_base + BRIDGE_8G_CFG_0);
+}
+
+static void xgene_pcie_program_core(void *csr_base)
+{
+ u32 val;
+
+ val = readl(csr_base + BRIDGE_CFG_0);
+ val |= AER_OPTIONAL_ERROR_EN;
+ writel(val, csr_base + BRIDGE_CFG_0);
+ writel(0x0, csr_base + INTXSTATUSMASK);
+ val = readl(csr_base + BRIDGE_CTRL_1);
+ val = (val & ~0xffff) | XGENE_PCIE_DEV_CTRL;
+ writel(val, csr_base + BRIDGE_CTRL_1);
+}
+
+static u64 xgene_pcie_set_ib_mask(void *csr_base, u32 addr, u32 flags, u64 size)
+{
+ u64 mask = (~(size - 1) & PCI_BASE_ADDRESS_MEM_MASK) | flags;
+ u32 val32 = 0;
+ u32 val;
+
+ val32 = readl(csr_base + addr);
+ val = (val32 & 0x0000ffff) | (lower_32_bits(mask) << 16);
+ writel(val, csr_base + addr);
+
+ val32 = readl(csr_base + addr + 0x04);
+ val = (val32 & 0xffff0000) | (lower_32_bits(mask) >> 16);
+ writel(val, csr_base + addr + 0x04);
+
+ val32 = readl(csr_base + addr + 0x04);
+ val = (val32 & 0x0000ffff) | (upper_32_bits(mask) << 16);
+ writel(val, csr_base + addr + 0x04);
+
+ val32 = readl(csr_base + addr + 0x08);
+ val = (val32 & 0xffff0000) | (upper_32_bits(mask) >> 16);
+ writel(val, csr_base + addr + 0x08);
+
+ return mask;
+}
+
+static void xgene_pcie_poll_linkup(struct xgene_pcie_port *port, u32 *lanes)
+{
+ void *csr_base = port->csr_base;
+ u32 val32;
+ u64 start_time, time;
+
+ /*
+ * A component enters the LTSSM Detect state within
+ * 20ms of the end of fundamental core reset.
+ */
+ msleep(XGENE_LTSSM_DETECT_WAIT);
+ port->link_up = 0;
+ start_time = jiffies;
+ do {
+ val32 = readl(csr_base + PCIECORE_CTLANDSTATUS);
+ if (val32 & LINK_UP_MASK) {
+ port->link_up = 1;
+ port->link_speed = PIPE_PHY_RATE_RD(val32);
+ val32 = readl(csr_base + BRIDGE_STATUS_0);
+ *lanes = val32 >> 26;
+ }
+ time = jiffies_to_msecs(jiffies - start_time);
+ } while ((!port->link_up) || (time <= XGENE_LTSSM_L0_WAIT));
+}
+
+static void xgene_pcie_setup_root_complex(struct xgene_pcie_port *port)
+{
+ void *csr_base = port->csr_base;
+ u32 val;
+
+ val = (XGENE_PCIE_DEVICEID << 16) | XGENE_PCIE_VENDORID;
+ writel(val, csr_base + BRIDGE_CFG_0);
+
+ val = readl(csr_base + BRIDGE_CFG_1);
+ val &= ~CLASS_CODE_MASK;
+ val |= PCI_CLASS_BRIDGE_PCI << 16;
+ writel(val, csr_base + BRIDGE_CFG_1);
+
+ val = readl(csr_base + BRIDGE_CFG_14);
+ val |= SWITCH_PORT_MODE_MASK;
+ val &= ~PM_FORCE_RP_MODE_MASK;
+ writel(val, csr_base + BRIDGE_CFG_14);
+ xgene_pcie_setup_link(port);
+ xgene_pcie_setup_lanes(port);
+ val = readl(csr_base + BRIDGE_CTRL_5);
+ val &= ~DEVICE_PORT_TYPE_MASK;
+ val |= XGENE_PORT_TYPE_RC;
+ writel(val, csr_base + BRIDGE_CTRL_5);
+
+ val = readl(csr_base + BRIDGE_CTRL_2);
+ val |= ENABLE_ASPM;
+ writel(val, csr_base + BRIDGE_CTRL_2);
+
+ val = readl(csr_base + BRIDGE_CFG_32);
+ writel(val | (1 << 19), csr_base + BRIDGE_CFG_32);
+}
+
+/* Return 0 on success */
+static int xgene_pcie_init_ecc(struct xgene_pcie_port *port)
+{
+ void *csr_base = port->csr_base;
+ int timeout = XGENE_PCIE_TIMEOUT;
+ u32 val;
+
+ val = readl(csr_base + MEM_RAM_SHUTDOWN);
+ if (val == 0)
+ return 0;
+ writel(0x0, csr_base + MEM_RAM_SHUTDOWN);
+ do {
+ val = readl(csr_base + BLOCK_MEM_RDY);
+ udelay(1);
+ } while ((val != BLOCK_MEM_RDY_VAL) && timeout--);
+
+ return !(timeout > 0);
+}
+
+static int xgene_pcie_init_port(struct xgene_pcie_port *port)
+{
+ int rc;
+
+ port->clk = clk_get(port->dev, NULL);
+ if (IS_ERR_OR_NULL(port->clk)) {
+ dev_err(port->dev, "clock not available\n");
+ return -ENODEV;
+ }
+
+ rc = clk_prepare_enable(port->clk);
+ if (rc) {
+ dev_err(port->dev, "clock enable failed\n");
+ return rc;
+ }
+
+ rc = xgene_pcie_init_ecc(port);
+ if (rc) {
+ dev_err(port->dev, "memory init failed\n");
+ return rc;
+ }
+
+ return 0;
+}
+
+struct device_node *pcibios_get_phb_of_node(struct pci_bus *bus)
+{
+ struct xgene_pcie_port *port = xgene_pcie_bus_to_port(bus);
+
+ return of_node_get(port->node);
+}
+
+static void xgene_pcie_fixup_bridge(struct pci_dev *dev)
+{
+ int i;
+
+ /* Hide the PCI host BARs from the kernel as their content doesn't
+ * fit well in the resource management
+ */
+ for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
+ dev->resource[i].start = dev->resource[i].end = 0;
+ dev->resource[i].flags = 0;
+ }
+ dev_info(&dev->dev, "Hiding X-Gene pci host bridge resources %s\n",
+ pci_name(dev));
+}
+DECLARE_PCI_FIXUP_HEADER(XGENE_PCIE_VENDORID, XGENE_PCIE_DEVICEID,
+ xgene_pcie_fixup_bridge);
+
+static void xgene_pcie_setup_primary_bus(struct xgene_pcie_port *port,
+ u32 first_busno, u32 last_busno)
+{
+ u32 val;
+ void *cfg_addr = port->cfg_base;
+
+ val = readl(cfg_addr + PCI_PRIMARY_BUS);
+ val &= ~PCI_PRIMARY_BUS_MASK;
+ val |= (last_busno << 16) | ((first_busno + 1) << 8) | (first_busno);
+ writel(val, cfg_addr + PCI_PRIMARY_BUS);
+}
+
+/*
+ * read configuration values from DTS
+ */
+static int xgene_pcie_read_dts_config(struct xgene_pcie_port *port)
+{
+ struct device_node *np = port->node;
+ struct resource csr_res;
+ struct resource cfg_res;
+
+ /* Get CSR space registers address */
+ if (of_address_to_resource(np, 0, &csr_res))
+ return -EINVAL;
+
+ port->csr_base = devm_ioremap_nocache(port->dev, csr_res.start,
+ resource_size(&csr_res));
+ if (port->csr_base == NULL)
+ return -ENOMEM;
+
+ /* Get CFG space registers address */
+ if (of_address_to_resource(np, 1, &cfg_res))
+ return -EINVAL;
+
+ port->cfg_addr = cfg_res.start;
+ port->cfg_base = devm_ioremap_nocache(port->dev, cfg_res.start,
+ resource_size(&cfg_res));
+ if (port->csr_base == NULL)
+ return -ENOMEM;
+
+ return 0;
+}
+
+static void xgene_pcie_setup_ob_reg(struct xgene_pcie_port *port,
+ u32 addr, u32 restype)
+{
+ struct resource *res = NULL;
+ void *base = port->csr_base + addr;
+ resource_size_t size;
+ u64 cpu_addr = 0;
+ u64 pci_addr = 0;
+ u64 mask = 0;
+ u32 min_size = 0;
+ u32 flag = EN_REG;
+
+ switch (restype) {
+ case IORESOURCE_MEM:
+ res = &port->mem.res;
+ pci_addr = port->mem.pci_addr;
+ min_size = SZ_128M;
+ break;
+ case IORESOURCE_IO:
+ res = &port->io.res;
+ pci_addr = port->io.pci_addr;
+ min_size = 128;
+ flag |= OB_LO_IO;
+ break;
+ }
+ size = resource_size(res);
+ if (size >= min_size)
+ mask = ~(size - 1) | flag;
+ else
+ dev_warn(port->dev, "res size 0x%llx less than minimum 0x%x\n",
+ (u64)size, min_size);
+ cpu_addr = res->start;
+ writel(lower_32_bits(cpu_addr), base);
+ writel(upper_32_bits(cpu_addr), base + 0x04);
+ writel(lower_32_bits(mask), base + 0x08);
+ writel(upper_32_bits(mask), base + 0x0c);
+ writel(lower_32_bits(pci_addr), base + 0x10);
+ writel(upper_32_bits(pci_addr), base + 0x14);
+}
+
+static void xgene_pcie_setup_cfg_reg(struct xgene_pcie_port *port)
+{
+ void *csr_base = port->csr_base;
+ u64 addr = port->cfg_addr;
+
+ writel(lower_32_bits(addr), csr_base + CFGBARL);
+ writel(upper_32_bits(addr), csr_base + CFGBARH);
+ writel(EN_REG, csr_base + CFGCTL);
+}
+
+static int xgene_pcie_parse_map_ranges(struct xgene_pcie_port *port)
+{
+ struct device_node *np = port->node;
+ struct of_pci_range range;
+ struct of_pci_range_parser parser;
+ struct device *dev = port->dev;
+
+ if (of_pci_range_parser_init(&parser, np)) {
+ dev_err(dev, "missing ranges property\n");
+ return -EINVAL;
+ }
+
+ /* Get the I/O, memory, config ranges from DT */
+ for_each_of_pci_range(&parser, &range) {
+ struct resource *res = NULL;
+ u64 restype = range.flags & IORESOURCE_TYPE_BITS;
+ u64 end = range.cpu_addr + range.size - 1;
+ dev_dbg(port->dev, "0x%08x 0x%016llx..0x%016llx -> 0x%016llx\n",
+ range.flags, range.cpu_addr, end, range.pci_addr);
+
+ switch (restype) {
+ case IORESOURCE_IO:
+ res = &port->io.res;
+ port->io.pci_addr = range.pci_addr;
+ of_pci_range_to_resource(&range, np, res);
+ xgene_pcie_setup_ob_reg(port, OMR1BARL, restype);
+ break;
+ case IORESOURCE_MEM:
+ res = &port->mem.res;
+ port->mem.pci_addr = range.pci_addr;
+ of_pci_range_to_resource(&range, np, res);
+ xgene_pcie_setup_ob_reg(port, OMR2BARL, restype);
+ break;
+ default:
+ dev_err(dev, "invalid io resource!");
+ return -EINVAL;
+ }
+ }
+ xgene_pcie_setup_cfg_reg(port);
+ return 0;
+}
+
+static int pci_dma_range_parser_init(struct of_pci_range_parser *parser,
+ struct device_node *node)
+{
+ const int na = 3, ns = 2;
+ int rlen;
+
+ parser->node = node;
+ parser->pna = of_n_addr_cells(node);
+ parser->np = parser->pna + na + ns;
+
+ parser->range = of_get_property(node, "dma-ranges", &rlen);
+ if (parser->range == NULL)
+ return -ENOENT;
+
+ parser->end = parser->range + rlen / sizeof(__be32);
+
+ return 0;
+}
+
+static void xgene_pcie_setup_pims(void *addr, u64 pim, u64 size)
+{
+ writel(lower_32_bits(pim), addr);
+ writel(upper_32_bits(pim) | EN_COHERENCY, addr + 0x04);
+ writel(lower_32_bits(size), addr + 0x10);
+ writel(upper_32_bits(size), addr + 0x14);
+}
+
+static void xgene_pcie_setup_ib_reg(struct xgene_pcie_port *port,
+ struct of_pci_range *range, u64 restype,
+ u32 region)
+{
+ void *csr_base = port->csr_base;
+ void *cfg_base = port->cfg_base;
+ void *bar_addr;
+ void *pim_addr;
+ u64 cpu_addr = range->cpu_addr;
+ u64 pci_addr = range->pci_addr;
+ u64 size = range->size;
+ u64 mask = ~(size - 1) | EN_REG;
+ u32 flags = PCI_BASE_ADDRESS_MEM_TYPE_64;
+ u32 bar_low;
+
+ if (restype == PCI_BASE_ADDRESS_MEM_PREFETCH)
+ flags |= PCI_BASE_ADDRESS_MEM_PREFETCH;
+
+ bar_low = pcie_bar_low_val((u32)cpu_addr, flags);
+ switch (region) {
+ case 0:
+ xgene_pcie_set_ib_mask(csr_base, BRIDGE_CFG_4, flags, size);
+ bar_addr = cfg_base + PCI_BASE_ADDRESS_0;
+ writel(bar_low, bar_addr);
+ writel(upper_32_bits(cpu_addr), bar_addr + 0x4);
+ pim_addr = csr_base + PIM1_1L;
+ break;
+ case 1:
+ bar_addr = csr_base + IBAR2;
+ writel(bar_low, bar_addr);
+ writel(lower_32_bits(mask), csr_base + IR2MSK);
+ pim_addr = csr_base + PIM2_1L;
+ break;
+ case 2:
+ bar_addr = csr_base + IBAR3L;
+ writel(bar_low, bar_addr);
+ writel(upper_32_bits(cpu_addr), bar_addr + 0x4);
+ writel(lower_32_bits(mask), csr_base + IR3MSKL);
+ writel(upper_32_bits(mask), csr_base + IR3MSKL + 0x4);
+ pim_addr = csr_base + PIM3_1L;
+ break;
+ }
+ xgene_pcie_setup_pims(pim_addr, pci_addr, size);
+}
+
+/* X-Gene PCIe support maximum 3 inbound memory regions
+ * This function helps to select a region based on size of region
+ */
+static int xgene_pcie_select_ib_reg(u64 size)
+{
+ static u8 ib_reg_mask;
+
+ if ((size > 4) && (size < SZ_16M) && !(ib_reg_mask & (1 << 1))) {
+ ib_reg_mask |= (1 << 1);
+ return 1;
+ }
+
+ if ((size > SZ_1K) && (size < SZ_1T) && !(ib_reg_mask & (1 << 0))) {
+ ib_reg_mask |= (1 << 0);
+ return 0;
+ }
+
+ if ((size > SZ_1M) && (size < SZ_1T) && !(ib_reg_mask & (1 << 2))) {
+ ib_reg_mask |= (1 << 2);
+ return 2;
+ }
+ return -EINVAL;
+}
+
+static int xgene_pcie_parse_map_dma_ranges(struct xgene_pcie_port *port)
+{
+ struct device_node *np = port->node;
+ struct of_pci_range range;
+ struct of_pci_range_parser parser;
+ struct device *dev = port->dev;
+ int region;
+
+ if (pci_dma_range_parser_init(&parser, np)) {
+ dev_err(dev, "missing dma-ranges property\n");
+ return -EINVAL;
+ }
+
+ /* Get the dma-ranges from DT */
+ for_each_of_pci_range(&parser, &range) {
+ u64 restype = range.flags & IORESOURCE_TYPE_BITS;
+ u64 end = range.cpu_addr + range.size - 1;
+ dev_dbg(port->dev, "0x%08x 0x%016llx..0x%016llx -> 0x%016llx\n",
+ range.flags, range.cpu_addr, end, range.pci_addr);
+ region = xgene_pcie_select_ib_reg(range.size);
+ if (region == -EINVAL) {
+ dev_warn(port->dev, "invalid pcie dma-range config\n");
+ continue;
+ }
+ xgene_pcie_setup_ib_reg(port, &range, restype, region);
+ }
+ return 0;
+}
+
+static int xgene_pcie_setup(int nr, struct pci_sys_data *sys)
+{
+ struct xgene_pcie_port *pp = xgene_pcie_sys_to_port(sys);
+
+ if (pp == NULL)
+ return 0;
+
+ sys->mem_offset = pp->mem.res.start - pp->mem.pci_addr;
+ pci_add_resource_offset(&sys->resources, &pp->mem.res,
+ sys->mem_offset);
+ return 1;
+}
+
+static struct pci_bus __init *xgene_pcie_scan_bus(int nr,
+ struct pci_sys_data *sys)
+{
+ struct xgene_pcie_port *pp = xgene_pcie_sys_to_port(sys);
+
+ pp->first_busno = sys->busnr;
+ xgene_pcie_setup_primary_bus(pp, sys->busnr, 0xff);
+ return pci_scan_root_bus(NULL, sys->busnr, &xgene_pcie_ops,
+ sys, &sys->resources);
+}
+
+static struct hw_pci xgene_pcie_hw __initdata = {
+ .nr_controllers = XGENE_PCIE_MAX_PORTS,
+ .setup = xgene_pcie_setup,
+ .scan = xgene_pcie_scan_bus,
+ .map_irq = of_irq_parse_and_map_pci,
+};
+
+static int __init xgene_pcie_probe_bridge(struct platform_device *pdev)
+{
+ struct device_node *np = of_node_get(pdev->dev.of_node);
+ struct xgene_pcie_port *port;
+ static int index;
+ u32 lanes = 0;
+ int ret;
+
+ port = devm_kzalloc(&pdev->dev, sizeof(*port), GFP_KERNEL);
+ if (port == NULL)
+ return -ENOMEM;
+ port->node = np;
+ port->dev = &pdev->dev;
+
+ ret = xgene_pcie_read_dts_config(port);
+ if (ret)
+ return ret;
+
+ ret = xgene_pcie_init_port(port);
+ if (ret)
+ goto skip;
+ xgene_pcie_program_core(port->csr_base);
+ xgene_pcie_setup_root_complex(port);
+ ret = xgene_pcie_parse_map_ranges(port);
+ if (ret)
+ goto skip;
+ ret = xgene_pcie_parse_map_dma_ranges(port);
+ if (ret)
+ goto skip;
+ xgene_pcie_poll_linkup(port, &lanes);
+skip:
+ if (!port->link_up)
+ dev_info(port->dev, "(rc) link down\n");
+ else
+ dev_info(port->dev, "(rc) x%d gen-%d link up\n",
+ lanes, port->link_speed + 1);
+#ifdef CONFIG_PCI_DOMAINS
+ xgene_pcie_hw.domain++;
+#endif
+ xgene_pcie_hw.private_data[index++] = port;
+ platform_set_drvdata(pdev, port);
+ return 0;
+}
+
+static const struct of_device_id xgene_pcie_match_table[] __initconst = {
+ {.compatible = "apm,xgene-pcie",},
+ {},
+};
+
+static struct platform_driver xgene_pcie_driver = {
+ .driver = {
+ .name = "xgene-pcie",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(xgene_pcie_match_table),
+ },
+};
+
+static int __init xgene_pcie_init(void)
+{
+ void *private;
+ int ret;
+
+ pr_info("X-Gene: PCIe driver\n");
+
+ /* allocate private data to keep xgene_pcie_port information */
+ private = kzalloc((XGENE_PCIE_MAX_PORTS * sizeof(void *)), GFP_KERNEL);
+ if (private == NULL)
+ return -ENOMEM;
+ xgene_pcie_hw.private_data = private;
+ ret = platform_driver_probe(&xgene_pcie_driver,
+ xgene_pcie_probe_bridge);
+ if (ret)
+ return ret;
+ pci_common_init(&xgene_pcie_hw);
+ return 0;
+}
+
+module_init(xgene_pcie_init);
+
+MODULE_AUTHOR("Tanmay Inamdar <[email protected]>");
+MODULE_DESCRIPTION("APM X-Gene PCIe driver");
+MODULE_LICENSE("GPL v2");
--
1.7.9.5

2014-01-14 23:35:17

by Tanmay Inamdar

[permalink] [raw]
Subject: [RFC PATCH V2 4/4] MAINTAINERS: entry for APM X-Gene PCIe host driver

Add entry for AppliedMicro X-Gene PCIe host driver.

Signed-off-by: Tanmay Inamdar <[email protected]>
---
MAINTAINERS | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 6c20792..9e3ed53 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6531,6 +6531,13 @@ L: [email protected]
S: Maintained
F: drivers/pci/host/*designware*

+PCI DRIVER FOR APPLIEDMICRO XGENE
+M: Tanmay Inamdar <[email protected]>
+L: [email protected]
+L: [email protected]
+S: Maintained
+F: drivers/pci/host/pci-xgene.c
+
PCMCIA SUBSYSTEM
P: Linux PCMCIA Team
L: [email protected]
--
1.7.9.5

2014-01-14 23:35:21

by Tanmay Inamdar

[permalink] [raw]
Subject: [RFC PATCH V2 3/4] dt-bindings: pci: xgene pcie device tree bindings

This patch adds the bindings for X-Gene PCIe driver. The driver resides
under 'drivers/pci/host/pci-xgene.c' file.

Signed-off-by: Tanmay Inamdar <[email protected]>
---
.../devicetree/bindings/pci/xgene-pcie.txt | 45 ++++++++++++++++++++
1 file changed, 45 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pci/xgene-pcie.txt

diff --git a/Documentation/devicetree/bindings/pci/xgene-pcie.txt b/Documentation/devicetree/bindings/pci/xgene-pcie.txt
new file mode 100644
index 0000000..19b9c28
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/xgene-pcie.txt
@@ -0,0 +1,45 @@
+* AppliedMicro X-Gene PCIe interface
+
+Required properties:
+- status: Either "ok" or "disabled".
+- device_type: set to "pci"
+- compatible: should contain "xgene,pcie" to identify the core.
+- reg: base addresses and lengths of the pcie controller configuration
+ space register.
+- #address-cells: set to <3>
+- #size-cells: set to <2>
+- ranges: ranges for the outbound memory, I/O regions.
+- dma-ranges: ranges for the inbound memory regions.
+- #interrupt-cells: set to <1>
+- interrupt-map-mask and interrupt-map: standard PCI properties
+ to define the mapping of the PCIe interface to interrupt
+ numbers.
+- clocks: from common clock binding: handle to pci clock.
+
+Example:
+
+SoC specific DT Entry:
+ pcie0: pcie@1f2b0000 {
+ status = "disabled";
+ device_type = "pci";
+ compatible = "apm,xgene-pcie";
+ #interrupt-cells = <1>;
+ #size-cells = <2>;
+ #address-cells = <3>;
+ reg = < 0x00 0x1f2b0000 0x0 0x00010000
+ 0xe0 0xd0000000 0x0 0x00200000>;
+ ranges = <0x01000000 0x00 0x00000000 0xe0 0x00000000 0x00 0x00010000 /* io */
+ 0x02000000 0x00 0x10000000 0xe0 0x10000000 0x00 0x80000000>; /* mem */
+ dma-ranges = <0x42000000 0x40 0x00000000 0x40 0x00000000 0x40 0x00000000>;
+ interrupt-map-mask = <0x0 0x0 0x0 0x7>;
+ interrupt-map = <0x0 0x0 0x0 0x1 &gic 0x0 0xc2 0x1
+ 0x0 0x0 0x0 0x2 &gic 0x0 0xc3 0x1
+ 0x0 0x0 0x0 0x3 &gic 0x0 0xc4 0x1
+ 0x0 0x0 0x0 0x4 &gic 0x0 0xc5 0x1>;
+ clocks = <&pcie0clk 0>;
+ };
+
+Board specific DT Entry:
+ &pcie0 {
+ status = "ok";
+ };
--
1.7.9.5

2014-01-14 23:35:28

by Tanmay Inamdar

[permalink] [raw]
Subject: [RFC PATCH V2 2/4] arm64:dts: APM X-Gene PCIe device tree nodes

This patch adds the device tree nodes for APM X-Gene PCIe controller and
PCIe clock interface. Since X-Gene SOC supports maximum 5 ports, 5 dts
nodes are added.

Signed-off-by: Tanmay Inamdar <[email protected]>
---
arch/arm64/boot/dts/apm-mustang.dts | 4 +
arch/arm64/boot/dts/apm-storm.dtsi | 144 +++++++++++++++++++++++++++++++++++
2 files changed, 148 insertions(+)

diff --git a/arch/arm64/boot/dts/apm-mustang.dts b/arch/arm64/boot/dts/apm-mustang.dts
index 1247ca1..ab2b95f 100644
--- a/arch/arm64/boot/dts/apm-mustang.dts
+++ b/arch/arm64/boot/dts/apm-mustang.dts
@@ -24,3 +24,7 @@
reg = < 0x1 0x00000000 0x0 0x80000000 >; /* Updated by bootloader */
};
};
+
+&pcie0 {
+ status = "ok";
+};
diff --git a/arch/arm64/boot/dts/apm-storm.dtsi b/arch/arm64/boot/dts/apm-storm.dtsi
index d37d736..6b8b995 100644
--- a/arch/arm64/boot/dts/apm-storm.dtsi
+++ b/arch/arm64/boot/dts/apm-storm.dtsi
@@ -176,6 +176,150 @@
reg-names = "csr-reg";
clock-output-names = "eth8clk";
};
+
+ pcie0clk: pcie0clk@1f2bc000 {
+ compatible = "apm,xgene-device-clock";
+ #clock-cells = <1>;
+ clocks = <&socplldiv2 0>;
+ reg = <0x0 0x1f2bc000 0x0 0x1000>;
+ reg-names = "csr-reg";
+ clock-output-names = "pcie0clk";
+ };
+
+ pcie1clk: pcie1clk@1f2cc000 {
+ compatible = "apm,xgene-device-clock";
+ #clock-cells = <1>;
+ clocks = <&socplldiv2 0>;
+ reg = <0x0 0x1f2cc000 0x0 0x1000>;
+ reg-names = "csr-reg";
+ clock-output-names = "pcie1clk";
+ };
+
+ pcie2clk: pcie2clk@1f2dc000 {
+ compatible = "apm,xgene-device-clock";
+ #clock-cells = <1>;
+ clocks = <&socplldiv2 0>;
+ reg = <0x0 0x1f2dc000 0x0 0x1000>;
+ reg-names = "csr-reg";
+ clock-output-names = "pcie2clk";
+ };
+
+ pcie3clk: pcie3clk@1f50c000 {
+ compatible = "apm,xgene-device-clock";
+ #clock-cells = <1>;
+ clocks = <&socplldiv2 0>;
+ reg = <0x0 0x1f50c000 0x0 0x1000>;
+ reg-names = "csr-reg";
+ clock-output-names = "pcie3clk";
+ };
+
+ pcie4clk: pcie4clk@1f51c000 {
+ compatible = "apm,xgene-device-clock";
+ #clock-cells = <1>;
+ clocks = <&socplldiv2 0>;
+ reg = <0x0 0x1f51c000 0x0 0x1000>;
+ reg-names = "csr-reg";
+ clock-output-names = "pcie4clk";
+ };
+ };
+
+ pcie0: pcie@1f2b0000 {
+ status = "disabled";
+ device_type = "pci";
+ compatible = "apm,xgene-pcie";
+ #interrupt-cells = <1>;
+ #size-cells = <2>;
+ #address-cells = <3>;
+ reg = < 0x00 0x1f2b0000 0x0 0x00010000
+ 0xe0 0xd0000000 0x0 0x00200000>;
+ ranges = <0x01000000 0x00 0x00000000 0xe0 0x00000000 0x00 0x00010000 /* io */
+ 0x02000000 0x00 0x10000000 0xe0 0x10000000 0x00 0x80000000>; /* mem */
+ dma-ranges = <0x42000000 0x40 0x00000000 0x40 0x00000000 0x40 0x00000000>;
+ interrupt-map-mask = <0x0 0x0 0x0 0x7>;
+ interrupt-map = <0x0 0x0 0x0 0x1 &gic 0x0 0xc2 0x1
+ 0x0 0x0 0x0 0x2 &gic 0x0 0xc3 0x1
+ 0x0 0x0 0x0 0x3 &gic 0x0 0xc4 0x1
+ 0x0 0x0 0x0 0x4 &gic 0x0 0xc5 0x1>;
+ clocks = <&pcie0clk 0>;
+ };
+
+ pcie1: pcie@1f2c0000 {
+ status = "disabled";
+ device_type = "pci";
+ compatible = "apm,xgene-pcie";
+ #interrupt-cells = <1>;
+ #size-cells = <2>;
+ #address-cells = <3>;
+ reg = < 0x00 0x1f2c0000 0x0 0x00010000
+ 0xd0 0xd0000000 0x0 0x00200000>;
+ ranges = <0x01000000 0x0 0x00000000 0xd0 0x00000000 0x00 0x00010000 /* io */
+ 0x02000000 0x0 0x10000000 0xd0 0x10000000 0x00 0x80000000>; /* mem */
+ dma-ranges = <0x42000000 0x40 0x00000000 0x40 0x00000000 0x40 0x00000000>;
+ interrupt-map-mask = <0x0 0x0 0x0 0x7>;
+ interrupt-map = <0x0 0x0 0x0 0x1 &gic 0x0 0xc8 0x1
+ 0x0 0x0 0x0 0x2 &gic 0x0 0xc9 0x1
+ 0x0 0x0 0x0 0x3 &gic 0x0 0xca 0x1
+ 0x0 0x0 0x0 0x4 &gic 0x0 0xcb 0x1>;
+ clocks = <&pcie1clk 0>;
+ };
+
+ pcie2: pcie@1f2d0000 {
+ status = "disabled";
+ device_type = "pci";
+ compatible = "apm,xgene-pcie";
+ #interrupt-cells = <1>;
+ #size-cells = <2>;
+ #address-cells = <3>;
+ reg = < 0x00 0x1f2d0000 0x0 0x00010000
+ 0x90 0xd0000000 0x0 0x00200000>;
+ ranges = <0x01000000 0x0 0x00000000 0x90 0x00000000 0x0 0x00010000 /* io */
+ 0x02000000 0x0 0x10000000 0x90 0x10000000 0x0 0x80000000>; /* mem */
+ dma-ranges = <0x42000000 0x40 0x00000000 0x40 0x00000000 0x40 0x00000000>;
+ interrupt-map-mask = <0x0 0x0 0x0 0x7>;
+ interrupt-map = <0x0 0x0 0x0 0x1 &gic 0x0 0xce 0x1
+ 0x0 0x0 0x0 0x2 &gic 0x0 0xcf 0x1
+ 0x0 0x0 0x0 0x3 &gic 0x0 0xd0 0x1
+ 0x0 0x0 0x0 0x4 &gic 0x0 0xd1 0x1>;
+ clocks = <&pcie2clk 0>;
+ };
+
+ pcie3: pcie@1f500000 {
+ status = "disabled";
+ device_type = "pci";
+ compatible = "apm,xgene-pcie";
+ #interrupt-cells = <1>;
+ #size-cells = <2>;
+ #address-cells = <3>;
+ reg = < 0x00 0x1f500000 0x0 0x00010000
+ 0xa0 0xd0000000 0x0 0x00200000>;
+ ranges = <0x01000000 0x0 0x00000000 0xa0 0x00000000 0x0 0x00010000 /* mem */
+ 0x02000000 0x0 0x10000000 0xa0 0x10000000 0x0 0x80000000>; /* io */
+ dma-ranges = <0x42000000 0x40 0x00000000 0x40 0x00000000 0x40 0x00000000>;
+ interrupt-map-mask = <0x0 0x0 0x0 0x7>;
+ interrupt-map = <0x0 0x0 0x0 0x1 &gic 0x0 0xd4 0x1
+ 0x0 0x0 0x0 0x2 &gic 0x0 0xd5 0x1
+ 0x0 0x0 0x0 0x3 &gic 0x0 0xd6 0x1
+ 0x0 0x0 0x0 0x4 &gic 0x0 0xd7 0x1>;
+ clocks = <&pcie3clk 0>;
+ };
+
+ pcie4: pcie@1f510000 {
+ status = "disabled";
+ device_type = "pci";
+ compatible = "apm,xgene-pcie";
+ #interrupt-cells = <1>;
+ #size-cells = <2>;
+ #address-cells = <3>;
+ reg = <0x00 0x1f510000 0x0 0x00010000>;
+ ranges = <0x01000000 0x0 0x00000000 0xc0 0x00000000 0x0 0x00010000 /* io */
+ 0x02000000 0x0 0x10000000 0xc0 0x10000000 0x0 0x80000000>; /* mem */
+ dma-ranges = <0x42000000 0x40 0x00000000 0x40 0x00000000 0x40 0x00000000>;
+ interrupt-map-mask = <0x0 0x0 0x0 0x7>;
+ interrupt-map = <0x0 0x0 0x0 0x1 &gic 0x0 0xda 0x1
+ 0x0 0x0 0x0 0x2 &gic 0x0 0xdb 0x1
+ 0x0 0x0 0x0 0x3 &gic 0x0 0xdc 0x1
+ 0x0 0x0 0x0 0x4 &gic 0x0 0xdd 0x1>;
+ clocks = <&pcie4clk 0>;
};

serial0: serial@1c020000 {
--
1.7.9.5

2014-01-15 09:58:25

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [RFC PATCH V2 3/4] dt-bindings: pci: xgene pcie device tree bindings

On Wednesday 15 January 2014, Tanmay Inamdar wrote:
> This patch adds the bindings for X-Gene PCIe driver. The driver resides
> under 'drivers/pci/host/pci-xgene.c' file.
>
> Signed-off-by: Tanmay Inamdar <[email protected]>
> ---
> .../devicetree/bindings/pci/xgene-pcie.txt | 45 ++++++++++++++++++++
> 1 file changed, 45 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pci/xgene-pcie.txt
>
> diff --git a/Documentation/devicetree/bindings/pci/xgene-pcie.txt b/Documentation/devicetree/bindings/pci/xgene-pcie.txt
> new file mode 100644
> index 0000000..19b9c28
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/xgene-pcie.txt
> @@ -0,0 +1,45 @@
> +* AppliedMicro X-Gene PCIe interface
> +
> +Required properties:
> +- status: Either "ok" or "disabled".

"status" isn't normally a required property. The default interpretation is
that a node without a status property is active.

> +- device_type: set to "pci"
> +- compatible: should contain "xgene,pcie" to identify the core.
> +- reg: base addresses and lengths of the pcie controller configuration
> + space register.

Doesn't match the code or example: You only list one area here, but
you actually need to register sets.

Looks good otherwise now.

Arnd

2014-01-15 12:40:11

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [RFC PATCH V2 1/4] pci: APM X-Gene PCIe controller driver

On Wednesday 15 January 2014, Tanmay Inamdar wrote:
> This patch adds the AppliedMicro X-Gene SOC PCIe controller driver.
> X-Gene PCIe controller supports maxmum upto 8 lanes and GEN3 speed.
> X-Gene has maximum 5 PCIe ports supported.
>
> Signed-off-by: Tanmay Inamdar <[email protected]>

This already looks much better than the first version, but I have a more
comments. Most importantly, it would help to know how the root ports
are structured. Is this a standard root complex and multiple ports,
multiple root complexes with one port each, or a nonstandard organization
that is a mix of those two models?

> +
> +/* When the address bit [17:16] is 2'b01, the Configuration access will be
> + * treated as Type 1 and it will be forwarded to external PCIe device.
> + */
> +static void __iomem *xgene_pcie_get_cfg_base(struct pci_bus *bus)
> +{
> + struct xgene_pcie_port *port = xgene_pcie_bus_to_port(bus);
> + u64 addr = (u64)port->cfg_base;
> +
> + if (bus->number >= (port->first_busno + 1))
> + addr |= AXI_EP_CFG_ACCESS;
> +
> + return (void *)addr;
> +}

Wrong type, it should be 'void __iomem *'. Also you can't assume that
bit operations work on virtual __iomem addresses, so it should be better
to just add a constant integer to the pointer, which is a valid
operation.

I also wonder why you need to do this at all. If there isn't a global
config space for all ports, but rather a separate type0/type1 config
cycle based on the bus number, I see that as an indication that the
ports are in fact separate domains and should each start with bus 0.

> +static void xgene_pcie_setup_lanes(struct xgene_pcie_port *port)
> +{
> + void *csr_base = port->csr_base;
> + u32 val;
> +
> + val = readl(csr_base + BRIDGE_8G_CFG_8);
> + val = eq_pre_cursor_lane0_set(val, 0x7);
> + val = eq_pre_cursor_lane1_set(val, 0x7);
> + writel(val, csr_base + BRIDGE_8G_CFG_8);
> +
> + val = readl(csr_base + BRIDGE_8G_CFG_9);
> + val = eq_pre_cursor_lane0_set(val, 0x7);
> + val = eq_pre_cursor_lane1_set(val, 0x7);
> + writel(val, csr_base + BRIDGE_8G_CFG_9);
> +
> + val = readl(csr_base + BRIDGE_8G_CFG_10);
> + val = eq_pre_cursor_lane0_set(val, 0x7);
> + val = eq_pre_cursor_lane1_set(val, 0x7);
> + writel(val, csr_base + BRIDGE_8G_CFG_10);
> +
> + val = readl(csr_base + BRIDGE_8G_CFG_11);
> + val = eq_pre_cursor_lane0_set(val, 0x7);
> + val = eq_pre_cursor_lane1_set(val, 0x7);
> + writel(val, csr_base + BRIDGE_8G_CFG_11);
> +
> + val = readl(csr_base + BRIDGE_8G_CFG_4);
> + val = (val & ~0x30) | (1 << 4);
> + writel(val, csr_base + BRIDGE_8G_CFG_4);
> +}

Please document what you are actually setting here. If the configuration
of the lanes is always the same, why do you have to set it here. If not,
why do you set constant values?

> +static void xgene_pcie_setup_link(struct xgene_pcie_port *port)
> +{
> + void *csr_base = port->csr_base;
> + u32 val;
> +
> + val = readl(csr_base + BRIDGE_CFG_14);
> + val |= DIRECT_TO_8GTS_MASK;
> + val |= SUPPORT_5GTS_MASK;
> + val |= SUPPORT_8GTS_MASK;
> + val |= DIRECT_TO_5GTS_MASK;
> + writel(val, csr_base + BRIDGE_CFG_14);
> +
> + val = readl(csr_base + BRIDGE_CFG_14);
> + val &= ~ADVT_INFINITE_CREDITS;
> + writel(val, csr_base + BRIDGE_CFG_14);
> +
> + val = readl(csr_base + BRIDGE_8G_CFG_0);
> + val |= (val & ~0xf) | 7;
> + val |= (val & ~0xf00) | ((7 << 8) & 0xf00);
> + writel(val, csr_base + BRIDGE_8G_CFG_0);
> +
> + val = readl(csr_base + BRIDGE_8G_CFG_0);
> + val |= DWNSTRM_EQ_SKP_PHS_2_3;
> + writel(val, csr_base + BRIDGE_8G_CFG_0);
> +}

Same here.

> +static void xgene_pcie_program_core(void *csr_base)
> +{
> + u32 val;
> +
> + val = readl(csr_base + BRIDGE_CFG_0);
> + val |= AER_OPTIONAL_ERROR_EN;
> + writel(val, csr_base + BRIDGE_CFG_0);
> + writel(0x0, csr_base + INTXSTATUSMASK);
> + val = readl(csr_base + BRIDGE_CTRL_1);
> + val = (val & ~0xffff) | XGENE_PCIE_DEV_CTRL;
> + writel(val, csr_base + BRIDGE_CTRL_1);
> +}

'program_core'?

> +static void xgene_pcie_poll_linkup(struct xgene_pcie_port *port, u32 *lanes)
> +{
> + void *csr_base = port->csr_base;
> + u32 val32;
> + u64 start_time, time;
> +
> + /*
> + * A component enters the LTSSM Detect state within
> + * 20ms of the end of fundamental core reset.
> + */
> + msleep(XGENE_LTSSM_DETECT_WAIT);
> + port->link_up = 0;
> + start_time = jiffies;
> + do {
> + val32 = readl(csr_base + PCIECORE_CTLANDSTATUS);
> + if (val32 & LINK_UP_MASK) {
> + port->link_up = 1;
> + port->link_speed = PIPE_PHY_RATE_RD(val32);
> + val32 = readl(csr_base + BRIDGE_STATUS_0);
> + *lanes = val32 >> 26;
> + }
> + time = jiffies_to_msecs(jiffies - start_time);
> + } while ((!port->link_up) || (time <= XGENE_LTSSM_L0_WAIT));
> +}

Maybe another msleep() in the loop? It seems weird to first do an
unconditional sleep but then busy-wait for the result.

> +static void xgene_pcie_setup_primary_bus(struct xgene_pcie_port *port,
> + u32 first_busno, u32 last_busno)
> +{
> + u32 val;
> + void *cfg_addr = port->cfg_base;
> +
> + val = readl(cfg_addr + PCI_PRIMARY_BUS);
> + val &= ~PCI_PRIMARY_BUS_MASK;
> + val |= (last_busno << 16) | ((first_busno + 1) << 8) | (first_busno);
> + writel(val, cfg_addr + PCI_PRIMARY_BUS);
> +}

Please explain what you are doing here. As mentioned above, I would expect
that each domain has visibility of all 255 buses. You shouldn't need any hacks
where you try to artificially squeeze the ports into a single domain when
they are separate in hardware.

> +/*
> + * read configuration values from DTS
> + */
> +static int xgene_pcie_read_dts_config(struct xgene_pcie_port *port)

The comment and function name don't seem to match what the function
does. The main purpose of this function seems to be to ioremap
the resources, which have nothing to with configuration.

> +{
> + struct device_node *np = port->node;
> + struct resource csr_res;
> + struct resource cfg_res;
> +
> + /* Get CSR space registers address */
> + if (of_address_to_resource(np, 0, &csr_res))
> + return -EINVAL;
> +
> + port->csr_base = devm_ioremap_nocache(port->dev, csr_res.start,
> + resource_size(&csr_res));

You can also use platform_get_resource() to access the resource
that is already there, rather than creating another one.

> +static void xgene_pcie_setup_ob_reg(struct xgene_pcie_port *port,
> + u32 addr, u32 restype)
> +{
> + struct resource *res = NULL;
> + void *base = port->csr_base + addr;
> + resource_size_t size;
> + u64 cpu_addr = 0;
> + u64 pci_addr = 0;
> + u64 mask = 0;
> + u32 min_size = 0;

A general note: don't initialize local variables to a bogus valus (e.g. 0)
in their declaration. It prevents the compiler from warning about
incorrect uses.

> + u32 flag = EN_REG;

This one on the other hand is ok, because you are actually going to
use that value.

> + switch (restype) {
> + case IORESOURCE_MEM:
> + res = &port->mem.res;
> + pci_addr = port->mem.pci_addr;
> + min_size = SZ_128M;
> + break;
> + case IORESOURCE_IO:
> + res = &port->io.res;
> + pci_addr = port->io.pci_addr;
> + min_size = 128;
> + flag |= OB_LO_IO;
> + break;
> + }

I assume this works ok, but seems wrong in one detail: If the resource
is marked IORESOURCE_IO, res->start is supposed to be in I/O space, not
in memory space, which would make it the wrong number to program
into the hardware registers.

> +static int xgene_pcie_parse_map_ranges(struct xgene_pcie_port *port)
> +{
> + struct device_node *np = port->node;
> + struct of_pci_range range;
> + struct of_pci_range_parser parser;
> + struct device *dev = port->dev;
> +
> + if (of_pci_range_parser_init(&parser, np)) {
> + dev_err(dev, "missing ranges property\n");
> + return -EINVAL;
> + }
> +
> + /* Get the I/O, memory, config ranges from DT */

The comment needs updating now that you don't read config space here any more.

> +/* X-Gene PCIe support maximum 3 inbound memory regions
> + * This function helps to select a region based on size of region
> + */
> +static int xgene_pcie_select_ib_reg(u64 size)
> +{
> + static u8 ib_reg_mask;
> +
> + if ((size > 4) && (size < SZ_16M) && !(ib_reg_mask & (1 << 1))) {
> + ib_reg_mask |= (1 << 1);
> + return 1;
> + }
> +
> + if ((size > SZ_1K) && (size < SZ_1T) && !(ib_reg_mask & (1 << 0))) {
> + ib_reg_mask |= (1 << 0);
> + return 0;
> + }
> +
> + if ((size > SZ_1M) && (size < SZ_1T) && !(ib_reg_mask & (1 << 2))) {
> + ib_reg_mask |= (1 << 2);
> + return 2;
> + }
> + return -EINVAL;
> +}

Shouldn't the ib_reg_mask variable be per host bridge? Static variables
are dangerous if you ever get multiple instances of the hardware in one
system.

> +static int xgene_pcie_parse_map_dma_ranges(struct xgene_pcie_port *port)
> +{
> + struct device_node *np = port->node;
> + struct of_pci_range range;
> + struct of_pci_range_parser parser;
> + struct device *dev = port->dev;
> + int region;
> +
> + if (pci_dma_range_parser_init(&parser, np)) {
> + dev_err(dev, "missing dma-ranges property\n");
> + return -EINVAL;
> + }
> +
> + /* Get the dma-ranges from DT */
> + for_each_of_pci_range(&parser, &range) {
> + u64 restype = range.flags & IORESOURCE_TYPE_BITS;
> + u64 end = range.cpu_addr + range.size - 1;
> + dev_dbg(port->dev, "0x%08x 0x%016llx..0x%016llx -> 0x%016llx\n",
> + range.flags, range.cpu_addr, end, range.pci_addr);
> + region = xgene_pcie_select_ib_reg(range.size);
> + if (region == -EINVAL) {
> + dev_warn(port->dev, "invalid pcie dma-range config\n");
> + continue;
> + }
> + xgene_pcie_setup_ib_reg(port, &range, restype, region);
> + }
> + return 0;
> +}

I guess is could even be a local variable in this function, which you pass
by reference.

> +
> +static int xgene_pcie_setup(int nr, struct pci_sys_data *sys)
> +{
> + struct xgene_pcie_port *pp = xgene_pcie_sys_to_port(sys);
> +
> + if (pp == NULL)
> + return 0;
> +
> + sys->mem_offset = pp->mem.res.start - pp->mem.pci_addr;
> + pci_add_resource_offset(&sys->resources, &pp->mem.res,
> + sys->mem_offset);
> + return 1;
> +}

Please follow the regular error handling conventions, which are to
pass a negative errno value on error and zero on success.

Also, what would be a reason for the port to be zero here? If
it's something that can't happen in practice, don't try to handle
it gracefully. You can use BUG_ON() for fatal conditions that
are supposed to be impossible to reach.

> +static struct pci_bus __init *xgene_pcie_scan_bus(int nr,
> + struct pci_sys_data *sys)
> +{
> + struct xgene_pcie_port *pp = xgene_pcie_sys_to_port(sys);
> +
> + pp->first_busno = sys->busnr;
> + xgene_pcie_setup_primary_bus(pp, sys->busnr, 0xff);
> + return pci_scan_root_bus(NULL, sys->busnr, &xgene_pcie_ops,
> + sys, &sys->resources);
> +}

You have a number of helper functions that don't seem to gain
much at all. Just move the call to pci_scan_root_bus() into
xgene_pcie_setup_primary_bus() here, and use that as the .scan
callback.


> + if (!port->link_up)
> + dev_info(port->dev, "(rc) link down\n");
> + else
> + dev_info(port->dev, "(rc) x%d gen-%d link up\n",
> + lanes, port->link_speed + 1);
> +#ifdef CONFIG_PCI_DOMAINS
> + xgene_pcie_hw.domain++;
> +#endif
> + xgene_pcie_hw.private_data[index++] = port;
> + platform_set_drvdata(pdev, port);
> + return 0;
> +}

Do you have multiple domains or not? I don't see how it can work if you
make the domain setup conditional on a kernel configuration option.
If you in fact have multiple domains, make sure in Kconfig that
CONFIG_PCI_DOMAINS is enabled. Otherwise don't mess with the domain
number...

> +static const struct of_device_id xgene_pcie_match_table[] __initconst = {
> + {.compatible = "apm,xgene-pcie",},
> + {},
> +};

Another general note: Your "compatible" strings are rather unspecific.
Do you have a version number for this IP block? I suppose that it's related
to one that has been used in other chips before, or will be used in future
chips, if it's not actually licensed from some other company.

> +static int __init xgene_pcie_init(void)
> +{
> + void *private;
> + int ret;
> +
> + pr_info("X-Gene: PCIe driver\n");
> +
> + /* allocate private data to keep xgene_pcie_port information */
> + private = kzalloc((XGENE_PCIE_MAX_PORTS * sizeof(void *)), GFP_KERNEL);

This should not be done unconditionally: There is no point in printing
a message or allocating memory if you don't actually run on a system
with this device.

> + if (private == NULL)
> + return -ENOMEM;

Style: if you are testing for an object, just write 'if (private)' or
'if (!private)', but don't compare against NULL.

> + xgene_pcie_hw.private_data = private;
> + ret = platform_driver_probe(&xgene_pcie_driver,
> + xgene_pcie_probe_bridge);
> + if (ret)
> + return ret;
> + pci_common_init(&xgene_pcie_hw);
> + return 0;

This seems wrong: You should not use platform_driver_probe() because
that has issues with deferred probing.

Arnd

2014-01-17 01:10:53

by Tanmay Inamdar

[permalink] [raw]
Subject: Re: [RFC PATCH V2 1/4] pci: APM X-Gene PCIe controller driver

On Wed, Jan 15, 2014 at 4:39 AM, Arnd Bergmann <[email protected]> wrote:
> On Wednesday 15 January 2014, Tanmay Inamdar wrote:
>> This patch adds the AppliedMicro X-Gene SOC PCIe controller driver.
>> X-Gene PCIe controller supports maxmum upto 8 lanes and GEN3 speed.
>> X-Gene has maximum 5 PCIe ports supported.
>>
>> Signed-off-by: Tanmay Inamdar <[email protected]>
>
> This already looks much better than the first version, but I have a more
> comments. Most importantly, it would help to know how the root ports
> are structured. Is this a standard root complex and multiple ports,
> multiple root complexes with one port each, or a nonstandard organization
> that is a mix of those two models?

This is multiple root complexes with one port each.

>
>> +
>> +/* When the address bit [17:16] is 2'b01, the Configuration access will be
>> + * treated as Type 1 and it will be forwarded to external PCIe device.
>> + */
>> +static void __iomem *xgene_pcie_get_cfg_base(struct pci_bus *bus)
>> +{
>> + struct xgene_pcie_port *port = xgene_pcie_bus_to_port(bus);
>> + u64 addr = (u64)port->cfg_base;
>> +
>> + if (bus->number >= (port->first_busno + 1))
>> + addr |= AXI_EP_CFG_ACCESS;
>> +
>> + return (void *)addr;
>> +}
>
> Wrong type, it should be 'void __iomem *'. Also you can't assume that
> bit operations work on virtual __iomem addresses, so it should be better
> to just add a constant integer to the pointer, which is a valid
> operation.

ok.

>
> I also wonder why you need to do this at all. If there isn't a global
> config space for all ports, but rather a separate type0/type1 config
> cycle based on the bus number, I see that as an indication that the
> ports are in fact separate domains and should each start with bus 0.

It is not a standard ECAM layout. We also have a separate RTDID
register as well to program bus, device, function. While accessing EP
config space, we have to set the bit 17:16 as 2b'01. The same config
space address is utilized for enabling a customized nonstandard PCIe
DMA feature. The bits are defined to differentiate the access purpose.
The feature is not supported in this driver yet.

Secondly I don't think it will matter if each port starts with bus 0.
As long as we set the correct BDF in RTDID and set correct bits in
config address, the config reads and writes would work. Right?

>
>> +static void xgene_pcie_setup_lanes(struct xgene_pcie_port *port)
>> +{
>> + void *csr_base = port->csr_base;
>> + u32 val;
>> +
>> + val = readl(csr_base + BRIDGE_8G_CFG_8);
>> + val = eq_pre_cursor_lane0_set(val, 0x7);
>> + val = eq_pre_cursor_lane1_set(val, 0x7);
>> + writel(val, csr_base + BRIDGE_8G_CFG_8);
>> +
>> + val = readl(csr_base + BRIDGE_8G_CFG_9);
>> + val = eq_pre_cursor_lane0_set(val, 0x7);
>> + val = eq_pre_cursor_lane1_set(val, 0x7);
>> + writel(val, csr_base + BRIDGE_8G_CFG_9);
>> +
>> + val = readl(csr_base + BRIDGE_8G_CFG_10);
>> + val = eq_pre_cursor_lane0_set(val, 0x7);
>> + val = eq_pre_cursor_lane1_set(val, 0x7);
>> + writel(val, csr_base + BRIDGE_8G_CFG_10);
>> +
>> + val = readl(csr_base + BRIDGE_8G_CFG_11);
>> + val = eq_pre_cursor_lane0_set(val, 0x7);
>> + val = eq_pre_cursor_lane1_set(val, 0x7);
>> + writel(val, csr_base + BRIDGE_8G_CFG_11);
>> +
>> + val = readl(csr_base + BRIDGE_8G_CFG_4);
>> + val = (val & ~0x30) | (1 << 4);
>> + writel(val, csr_base + BRIDGE_8G_CFG_4);
>> +}
>
> Please document what you are actually setting here. If the configuration
> of the lanes is always the same, why do you have to set it here. If not,
> why do you set constant values?

Good point. Let me check if these values should be constant or tune-able.

>
>> +static void xgene_pcie_setup_link(struct xgene_pcie_port *port)
>> +{
>> + void *csr_base = port->csr_base;
>> + u32 val;
>> +
>> + val = readl(csr_base + BRIDGE_CFG_14);
>> + val |= DIRECT_TO_8GTS_MASK;
>> + val |= SUPPORT_5GTS_MASK;
>> + val |= SUPPORT_8GTS_MASK;
>> + val |= DIRECT_TO_5GTS_MASK;
>> + writel(val, csr_base + BRIDGE_CFG_14);
>> +
>> + val = readl(csr_base + BRIDGE_CFG_14);
>> + val &= ~ADVT_INFINITE_CREDITS;
>> + writel(val, csr_base + BRIDGE_CFG_14);
>> +
>> + val = readl(csr_base + BRIDGE_8G_CFG_0);
>> + val |= (val & ~0xf) | 7;
>> + val |= (val & ~0xf00) | ((7 << 8) & 0xf00);
>> + writel(val, csr_base + BRIDGE_8G_CFG_0);
>> +
>> + val = readl(csr_base + BRIDGE_8G_CFG_0);
>> + val |= DWNSTRM_EQ_SKP_PHS_2_3;
>> + writel(val, csr_base + BRIDGE_8G_CFG_0);
>> +}
>
> Same here.
>
>> +static void xgene_pcie_program_core(void *csr_base)
>> +{
>> + u32 val;
>> +
>> + val = readl(csr_base + BRIDGE_CFG_0);
>> + val |= AER_OPTIONAL_ERROR_EN;
>> + writel(val, csr_base + BRIDGE_CFG_0);
>> + writel(0x0, csr_base + INTXSTATUSMASK);
>> + val = readl(csr_base + BRIDGE_CTRL_1);
>> + val = (val & ~0xffff) | XGENE_PCIE_DEV_CTRL;
>> + writel(val, csr_base + BRIDGE_CTRL_1);
>> +}
>
> 'program_core'?

Some of the PCIe core related misc configurations.

>
>> +static void xgene_pcie_poll_linkup(struct xgene_pcie_port *port, u32 *lanes)
>> +{
>> + void *csr_base = port->csr_base;
>> + u32 val32;
>> + u64 start_time, time;
>> +
>> + /*
>> + * A component enters the LTSSM Detect state within
>> + * 20ms of the end of fundamental core reset.
>> + */
>> + msleep(XGENE_LTSSM_DETECT_WAIT);
>> + port->link_up = 0;
>> + start_time = jiffies;
>> + do {
>> + val32 = readl(csr_base + PCIECORE_CTLANDSTATUS);
>> + if (val32 & LINK_UP_MASK) {
>> + port->link_up = 1;
>> + port->link_speed = PIPE_PHY_RATE_RD(val32);
>> + val32 = readl(csr_base + BRIDGE_STATUS_0);
>> + *lanes = val32 >> 26;
>> + }
>> + time = jiffies_to_msecs(jiffies - start_time);
>> + } while ((!port->link_up) || (time <= XGENE_LTSSM_L0_WAIT));
>> +}
>
> Maybe another msleep() in the loop? It seems weird to first do an
> unconditional sleep but then busy-wait for the result.

ok.

>
>> +static void xgene_pcie_setup_primary_bus(struct xgene_pcie_port *port,
>> + u32 first_busno, u32 last_busno)
>> +{
>> + u32 val;
>> + void *cfg_addr = port->cfg_base;
>> +
>> + val = readl(cfg_addr + PCI_PRIMARY_BUS);
>> + val &= ~PCI_PRIMARY_BUS_MASK;
>> + val |= (last_busno << 16) | ((first_busno + 1) << 8) | (first_busno);
>> + writel(val, cfg_addr + PCI_PRIMARY_BUS);
>> +}
>
> Please explain what you are doing here. As mentioned above, I would expect
> that each domain has visibility of all 255 buses. You shouldn't need any hacks
> where you try to artificially squeeze the ports into a single domain when
> they are separate in hardware.

ok. I will check and get back.

>
>> +/*
>> + * read configuration values from DTS
>> + */
>> +static int xgene_pcie_read_dts_config(struct xgene_pcie_port *port)
>
> The comment and function name don't seem to match what the function
> does. The main purpose of this function seems to be to ioremap
> the resources, which have nothing to with configuration.

ok.

>
>> +{
>> + struct device_node *np = port->node;
>> + struct resource csr_res;
>> + struct resource cfg_res;
>> +
>> + /* Get CSR space registers address */
>> + if (of_address_to_resource(np, 0, &csr_res))
>> + return -EINVAL;
>> +
>> + port->csr_base = devm_ioremap_nocache(port->dev, csr_res.start,
>> + resource_size(&csr_res));
>
> You can also use platform_get_resource() to access the resource
> that is already there, rather than creating another one.

ok.

>
>> +static void xgene_pcie_setup_ob_reg(struct xgene_pcie_port *port,
>> + u32 addr, u32 restype)
>> +{
>> + struct resource *res = NULL;
>> + void *base = port->csr_base + addr;
>> + resource_size_t size;
>> + u64 cpu_addr = 0;
>> + u64 pci_addr = 0;
>> + u64 mask = 0;
>> + u32 min_size = 0;
>
> A general note: don't initialize local variables to a bogus valus (e.g. 0)
> in their declaration. It prevents the compiler from warning about
> incorrect uses.

ok.

>
>> + u32 flag = EN_REG;
>
> This one on the other hand is ok, because you are actually going to
> use that value.
>
>> + switch (restype) {
>> + case IORESOURCE_MEM:
>> + res = &port->mem.res;
>> + pci_addr = port->mem.pci_addr;
>> + min_size = SZ_128M;
>> + break;
>> + case IORESOURCE_IO:
>> + res = &port->io.res;
>> + pci_addr = port->io.pci_addr;
>> + min_size = 128;
>> + flag |= OB_LO_IO;
>> + break;
>> + }
>
> I assume this works ok, but seems wrong in one detail: If the resource
> is marked IORESOURCE_IO, res->start is supposed to be in I/O space, not
> in memory space, which would make it the wrong number to program
> into the hardware registers.

Yes for using ioport resource. However we have decided to defer using
it since 'pci_ioremap_io' is not yet supported from arm64 side.

>From HW point of view, for memory mapped IO space, it is nothing but a
piece taken out of the ranges in address map for outbound accesses. So
while configuring registers from SOC side, it should take the CPU
address which is address from SOC address map. Right?

Later on we can have a separate io resource like 'realio' similar to
what pci-mvebu.c does.

>
>> +static int xgene_pcie_parse_map_ranges(struct xgene_pcie_port *port)
>> +{
>> + struct device_node *np = port->node;
>> + struct of_pci_range range;
>> + struct of_pci_range_parser parser;
>> + struct device *dev = port->dev;
>> +
>> + if (of_pci_range_parser_init(&parser, np)) {
>> + dev_err(dev, "missing ranges property\n");
>> + return -EINVAL;
>> + }
>> +
>> + /* Get the I/O, memory, config ranges from DT */
>
> The comment needs updating now that you don't read config space here any more.

ok.

>
>> +/* X-Gene PCIe support maximum 3 inbound memory regions
>> + * This function helps to select a region based on size of region
>> + */
>> +static int xgene_pcie_select_ib_reg(u64 size)
>> +{
>> + static u8 ib_reg_mask;
>> +
>> + if ((size > 4) && (size < SZ_16M) && !(ib_reg_mask & (1 << 1))) {
>> + ib_reg_mask |= (1 << 1);
>> + return 1;
>> + }
>> +
>> + if ((size > SZ_1K) && (size < SZ_1T) && !(ib_reg_mask & (1 << 0))) {
>> + ib_reg_mask |= (1 << 0);
>> + return 0;
>> + }
>> +
>> + if ((size > SZ_1M) && (size < SZ_1T) && !(ib_reg_mask & (1 << 2))) {
>> + ib_reg_mask |= (1 << 2);
>> + return 2;
>> + }
>> + return -EINVAL;
>> +}
>
> Shouldn't the ib_reg_mask variable be per host bridge? Static variables
> are dangerous if you ever get multiple instances of the hardware in one
> system.

Yes. You are right. Thanks.

>
>> +static int xgene_pcie_parse_map_dma_ranges(struct xgene_pcie_port *port)
>> +{
>> + struct device_node *np = port->node;
>> + struct of_pci_range range;
>> + struct of_pci_range_parser parser;
>> + struct device *dev = port->dev;
>> + int region;
>> +
>> + if (pci_dma_range_parser_init(&parser, np)) {
>> + dev_err(dev, "missing dma-ranges property\n");
>> + return -EINVAL;
>> + }
>> +
>> + /* Get the dma-ranges from DT */
>> + for_each_of_pci_range(&parser, &range) {
>> + u64 restype = range.flags & IORESOURCE_TYPE_BITS;
>> + u64 end = range.cpu_addr + range.size - 1;
>> + dev_dbg(port->dev, "0x%08x 0x%016llx..0x%016llx -> 0x%016llx\n",
>> + range.flags, range.cpu_addr, end, range.pci_addr);
>> + region = xgene_pcie_select_ib_reg(range.size);
>> + if (region == -EINVAL) {
>> + dev_warn(port->dev, "invalid pcie dma-range config\n");
>> + continue;
>> + }
>> + xgene_pcie_setup_ib_reg(port, &range, restype, region);
>> + }
>> + return 0;
>> +}
>
> I guess is could even be a local variable in this function, which you pass
> by reference.
>
>> +
>> +static int xgene_pcie_setup(int nr, struct pci_sys_data *sys)
>> +{
>> + struct xgene_pcie_port *pp = xgene_pcie_sys_to_port(sys);
>> +
>> + if (pp == NULL)
>> + return 0;
>> +
>> + sys->mem_offset = pp->mem.res.start - pp->mem.pci_addr;
>> + pci_add_resource_offset(&sys->resources, &pp->mem.res,
>> + sys->mem_offset);
>> + return 1;
>> +}
>
> Please follow the regular error handling conventions, which are to
> pass a negative errno value on error and zero on success.

ok.

>
> Also, what would be a reason for the port to be zero here? If
> it's something that can't happen in practice, don't try to handle
> it gracefully. You can use BUG_ON() for fatal conditions that
> are supposed to be impossible to reach.

This function is a hook to upper layer. We register nr_controllers in
hw_pci structure as MAX_PORTS we support. It can happen that number of
ports actually enabled from device tree are less than the number in
nr_controllers.

>
>> +static struct pci_bus __init *xgene_pcie_scan_bus(int nr,
>> + struct pci_sys_data *sys)
>> +{
>> + struct xgene_pcie_port *pp = xgene_pcie_sys_to_port(sys);
>> +
>> + pp->first_busno = sys->busnr;
>> + xgene_pcie_setup_primary_bus(pp, sys->busnr, 0xff);
>> + return pci_scan_root_bus(NULL, sys->busnr, &xgene_pcie_ops,
>> + sys, &sys->resources);
>> +}
>
> You have a number of helper functions that don't seem to gain
> much at all. Just move the call to pci_scan_root_bus() into
> xgene_pcie_setup_primary_bus() here, and use that as the .scan
> callback.
>

I can do that if I can get rid of setup_primary_bus api. Let me check.

>
>> + if (!port->link_up)
>> + dev_info(port->dev, "(rc) link down\n");
>> + else
>> + dev_info(port->dev, "(rc) x%d gen-%d link up\n",
>> + lanes, port->link_speed + 1);
>> +#ifdef CONFIG_PCI_DOMAINS
>> + xgene_pcie_hw.domain++;
>> +#endif
>> + xgene_pcie_hw.private_data[index++] = port;
>> + platform_set_drvdata(pdev, port);
>> + return 0;
>> +}
>
> Do you have multiple domains or not? I don't see how it can work if you
> make the domain setup conditional on a kernel configuration option.
> If you in fact have multiple domains, make sure in Kconfig that
> CONFIG_PCI_DOMAINS is enabled. Otherwise don't mess with the domain
> number...

It is enabled in Kconfig.

>
>> +static const struct of_device_id xgene_pcie_match_table[] __initconst = {
>> + {.compatible = "apm,xgene-pcie",},
>> + {},
>> +};
>
> Another general note: Your "compatible" strings are rather unspecific.
> Do you have a version number for this IP block? I suppose that it's related
> to one that has been used in other chips before, or will be used in future
> chips, if it's not actually licensed from some other company.

I will have to check this.

>
>> +static int __init xgene_pcie_init(void)
>> +{
>> + void *private;
>> + int ret;
>> +
>> + pr_info("X-Gene: PCIe driver\n");
>> +
>> + /* allocate private data to keep xgene_pcie_port information */
>> + private = kzalloc((XGENE_PCIE_MAX_PORTS * sizeof(void *)), GFP_KERNEL);
>
> This should not be done unconditionally: There is no point in printing
> a message or allocating memory if you don't actually run on a system
> with this device.

I am doing this here because I have one instance of hw_pci structure
with multiple pcie controllers. I can't do it from probe since it will
be called once per instance in device tree.

>
>> + if (private == NULL)
>> + return -ENOMEM;
>
> Style: if you are testing for an object, just write 'if (private)' or
> 'if (!private)', but don't compare against NULL.

ok.

>
>> + xgene_pcie_hw.private_data = private;
>> + ret = platform_driver_probe(&xgene_pcie_driver,
>> + xgene_pcie_probe_bridge);
>> + if (ret)
>> + return ret;
>> + pci_common_init(&xgene_pcie_hw);
>> + return 0;
>
> This seems wrong: You should not use platform_driver_probe() because
> that has issues with deferred probing.

I think 'platform_driver_probe' prevents the deferred probing.
'pci_common_init' needs to be called only once with current driver
structure. The probes for all pcie ports should be finished (ports
initialized) before 'pci_common_init' gets called.

>
> Arnd

2014-01-17 01:17:29

by Tanmay Inamdar

[permalink] [raw]
Subject: Re: [RFC PATCH V2 3/4] dt-bindings: pci: xgene pcie device tree bindings

On Wed, Jan 15, 2014 at 1:57 AM, Arnd Bergmann <[email protected]> wrote:
> On Wednesday 15 January 2014, Tanmay Inamdar wrote:
>> This patch adds the bindings for X-Gene PCIe driver. The driver resides
>> under 'drivers/pci/host/pci-xgene.c' file.
>>
>> Signed-off-by: Tanmay Inamdar <[email protected]>
>> ---
>> .../devicetree/bindings/pci/xgene-pcie.txt | 45 ++++++++++++++++++++
>> 1 file changed, 45 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/pci/xgene-pcie.txt
>>
>> diff --git a/Documentation/devicetree/bindings/pci/xgene-pcie.txt b/Documentation/devicetree/bindings/pci/xgene-pcie.txt
>> new file mode 100644
>> index 0000000..19b9c28
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pci/xgene-pcie.txt
>> @@ -0,0 +1,45 @@
>> +* AppliedMicro X-Gene PCIe interface
>> +
>> +Required properties:
>> +- status: Either "ok" or "disabled".
>
> "status" isn't normally a required property. The default interpretation is
> that a node without a status property is active.

ok. I will move 'status' under optional properties.

>
>> +- device_type: set to "pci"
>> +- compatible: should contain "xgene,pcie" to identify the core.
>> +- reg: base addresses and lengths of the pcie controller configuration
>> + space register.
>
> Doesn't match the code or example: You only list one area here, but
> you actually need to register sets.

ok. I will update

>
> Looks good otherwise now.
>
> Arnd

2014-01-17 15:07:47

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [RFC PATCH V2 1/4] pci: APM X-Gene PCIe controller driver

On Friday 17 January 2014, Tanmay Inamdar wrote:
> On Wed, Jan 15, 2014 at 4:39 AM, Arnd Bergmann <[email protected]> wrote:
> > On Wednesday 15 January 2014, Tanmay Inamdar wrote:
> >> This patch adds the AppliedMicro X-Gene SOC PCIe controller driver.
> >> X-Gene PCIe controller supports maxmum upto 8 lanes and GEN3 speed.
> >> X-Gene has maximum 5 PCIe ports supported.
> >>
> >> Signed-off-by: Tanmay Inamdar <[email protected]>
> >
> > This already looks much better than the first version, but I have a more
> > comments. Most importantly, it would help to know how the root ports
> > are structured. Is this a standard root complex and multiple ports,
> > multiple root complexes with one port each, or a nonstandard organization
> > that is a mix of those two models?
>
> This is multiple root complexes with one port each.

Ok.

> > I also wonder why you need to do this at all. If there isn't a global
> > config space for all ports, but rather a separate type0/type1 config
> > cycle based on the bus number, I see that as an indication that the
> > ports are in fact separate domains and should each start with bus 0.
>
> It is not a standard ECAM layout. We also have a separate RTDID
> register as well to program bus, device, function. While accessing EP
> config space, we have to set the bit 17:16 as 2b'01. The same config
> space address is utilized for enabling a customized nonstandard PCIe
> DMA feature. The bits are defined to differentiate the access purpose.
> The feature is not supported in this driver yet.
>
> Secondly I don't think it will matter if each port starts with bus 0.
> As long as we set the correct BDF in RTDID and set correct bits in
> config address, the config reads and writes would work. Right?

Yes, I think the current code will work (aside from my other comment),
but it seems unnecessary to do this if you use pci domains correctly:

If you have one pci domain and one root port per root complex, you
would not get any config space cycles for the root port and can
do away with the special case here, as well as with the
xgene_pcie_fixup_bridge() that seems to be a special case to
avoid touching the root ports as well.

> >
> >> + switch (restype) {
> >> + case IORESOURCE_MEM:
> >> + res = &port->mem.res;
> >> + pci_addr = port->mem.pci_addr;
> >> + min_size = SZ_128M;
> >> + break;
> >> + case IORESOURCE_IO:
> >> + res = &port->io.res;
> >> + pci_addr = port->io.pci_addr;
> >> + min_size = 128;
> >> + flag |= OB_LO_IO;
> >> + break;
> >> + }
> >
> > I assume this works ok, but seems wrong in one detail: If the resource
> > is marked IORESOURCE_IO, res->start is supposed to be in I/O space, not
> > in memory space, which would make it the wrong number to program
> > into the hardware registers.

Sorry, I was actually wrong with my statement above and I thought I had
deleted my comment before sending the mail.

> Yes for using ioport resource. However we have decided to defer using
> it since 'pci_ioremap_io' is not yet supported from arm64 side.
>
> From HW point of view, for memory mapped IO space, it is nothing but a
> piece taken out of the ranges in address map for outbound accesses. So
> while configuring registers from SOC side, it should take the CPU
> address which is address from SOC address map. Right?
>
> Later on we can have a separate io resource like 'realio' similar to
> what pci-mvebu.c does.

I think your code is actually correct here, but please also add the
pci_ioremap_io implementation for arm64 in a separate patch in this
series. It shouldn't be hard and we need it for every pci host driver
anyway.

> >> +static int xgene_pcie_setup(int nr, struct pci_sys_data *sys)
> >> +{
> >> + struct xgene_pcie_port *pp = xgene_pcie_sys_to_port(sys);
> >> +
> >> + if (pp == NULL)
> >> + return 0;
> >> +
> >> + sys->mem_offset = pp->mem.res.start - pp->mem.pci_addr;
> >> + pci_add_resource_offset(&sys->resources, &pp->mem.res,
> >> + sys->mem_offset);
> >> + return 1;
> >> +}

> > Also, what would be a reason for the port to be zero here? If
> > it's something that can't happen in practice, don't try to handle
> > it gracefully. You can use BUG_ON() for fatal conditions that
> > are supposed to be impossible to reach.
>
> This function is a hook to upper layer. We register nr_controllers in
> hw_pci structure as MAX_PORTS we support. It can happen that number of
> ports actually enabled from device tree are less than the number in
> nr_controllers.

I see. I'll comment more on this below.

> >> + if (!port->link_up)
> >> + dev_info(port->dev, "(rc) link down\n");
> >> + else
> >> + dev_info(port->dev, "(rc) x%d gen-%d link up\n",
> >> + lanes, port->link_speed + 1);
> >> +#ifdef CONFIG_PCI_DOMAINS
> >> + xgene_pcie_hw.domain++;
> >> +#endif
> >> + xgene_pcie_hw.private_data[index++] = port;
> >> + platform_set_drvdata(pdev, port);
> >> + return 0;
> >> +}
> >
> > Do you have multiple domains or not? I don't see how it can work if you
> > make the domain setup conditional on a kernel configuration option.
> > If you in fact have multiple domains, make sure in Kconfig that
> > CONFIG_PCI_DOMAINS is enabled. Otherwise don't mess with the domain
> > number...
>
> It is enabled in Kconfig.

Ok, then remove the #ifdef here.

> >> +static int __init xgene_pcie_init(void)
> >> +{
> >> + void *private;
> >> + int ret;
> >> +
> >> + pr_info("X-Gene: PCIe driver\n");
> >> +
> >> + /* allocate private data to keep xgene_pcie_port information */
> >> + private = kzalloc((XGENE_PCIE_MAX_PORTS * sizeof(void *)), GFP_KERNEL);
> >
> > This should not be done unconditionally: There is no point in printing
> > a message or allocating memory if you don't actually run on a system
> > with this device.
>
> I am doing this here because I have one instance of hw_pci structure
> with multiple pcie controllers. I can't do it from probe since it will
> be called once per instance in device tree.
>
> >
> >> + xgene_pcie_hw.private_data = private;
> >> + ret = platform_driver_probe(&xgene_pcie_driver,
> >> + xgene_pcie_probe_bridge);
> >> + if (ret)
> >> + return ret;
> >> + pci_common_init(&xgene_pcie_hw);
> >> + return 0;
> >
> > This seems wrong: You should not use platform_driver_probe() because
> > that has issues with deferred probing.
>
> I think 'platform_driver_probe' prevents the deferred probing.
> 'pci_common_init' needs to be called only once with current driver
> structure. The probes for all pcie ports should be finished (ports
> initialized) before 'pci_common_init' gets called.

I think it would be cleaner for dynamically registered host controllers
to call pci_common_init_dev() with nr_controllers=1 once for each root
complex in the probe() function. This should take care of a few other
things I've mentioned as well.

Arnd

2014-01-24 21:28:26

by Tanmay Inamdar

[permalink] [raw]
Subject: Re: [RFC PATCH V2 1/4] pci: APM X-Gene PCIe controller driver

On Thu, Jan 16, 2014 at 5:10 PM, Tanmay Inamdar <[email protected]> wrote:
> On Wed, Jan 15, 2014 at 4:39 AM, Arnd Bergmann <[email protected]> wrote:
>> On Wednesday 15 January 2014, Tanmay Inamdar wrote:
>>> This patch adds the AppliedMicro X-Gene SOC PCIe controller driver.
>>> X-Gene PCIe controller supports maxmum upto 8 lanes and GEN3 speed.
>>> X-Gene has maximum 5 PCIe ports supported.
>>>
>>> Signed-off-by: Tanmay Inamdar <[email protected]>
>>
>> This already looks much better than the first version, but I have a more
>> comments. Most importantly, it would help to know how the root ports
>> are structured. Is this a standard root complex and multiple ports,
>> multiple root complexes with one port each, or a nonstandard organization
>> that is a mix of those two models?
>
> This is multiple root complexes with one port each.
>
>>
>>> +
>>> +/* When the address bit [17:16] is 2'b01, the Configuration access will be
>>> + * treated as Type 1 and it will be forwarded to external PCIe device.
>>> + */
>>> +static void __iomem *xgene_pcie_get_cfg_base(struct pci_bus *bus)
>>> +{
>>> + struct xgene_pcie_port *port = xgene_pcie_bus_to_port(bus);
>>> + u64 addr = (u64)port->cfg_base;
>>> +
>>> + if (bus->number >= (port->first_busno + 1))
>>> + addr |= AXI_EP_CFG_ACCESS;
>>> +
>>> + return (void *)addr;
>>> +}
>>
>> Wrong type, it should be 'void __iomem *'. Also you can't assume that
>> bit operations work on virtual __iomem addresses, so it should be better
>> to just add a constant integer to the pointer, which is a valid
>> operation.
>
> ok.
>
>>
>> I also wonder why you need to do this at all. If there isn't a global
>> config space for all ports, but rather a separate type0/type1 config
>> cycle based on the bus number, I see that as an indication that the
>> ports are in fact separate domains and should each start with bus 0.
>
> It is not a standard ECAM layout. We also have a separate RTDID
> register as well to program bus, device, function. While accessing EP
> config space, we have to set the bit 17:16 as 2b'01. The same config
> space address is utilized for enabling a customized nonstandard PCIe
> DMA feature. The bits are defined to differentiate the access purpose.
> The feature is not supported in this driver yet.
>
> Secondly I don't think it will matter if each port starts with bus 0.
> As long as we set the correct BDF in RTDID and set correct bits in
> config address, the config reads and writes would work. Right?
>
>>
>>> +static void xgene_pcie_setup_lanes(struct xgene_pcie_port *port)
>>> +{
>>> + void *csr_base = port->csr_base;
>>> + u32 val;
>>> +
>>> + val = readl(csr_base + BRIDGE_8G_CFG_8);
>>> + val = eq_pre_cursor_lane0_set(val, 0x7);
>>> + val = eq_pre_cursor_lane1_set(val, 0x7);
>>> + writel(val, csr_base + BRIDGE_8G_CFG_8);
>>> +
>>> + val = readl(csr_base + BRIDGE_8G_CFG_9);
>>> + val = eq_pre_cursor_lane0_set(val, 0x7);
>>> + val = eq_pre_cursor_lane1_set(val, 0x7);
>>> + writel(val, csr_base + BRIDGE_8G_CFG_9);
>>> +
>>> + val = readl(csr_base + BRIDGE_8G_CFG_10);
>>> + val = eq_pre_cursor_lane0_set(val, 0x7);
>>> + val = eq_pre_cursor_lane1_set(val, 0x7);
>>> + writel(val, csr_base + BRIDGE_8G_CFG_10);
>>> +
>>> + val = readl(csr_base + BRIDGE_8G_CFG_11);
>>> + val = eq_pre_cursor_lane0_set(val, 0x7);
>>> + val = eq_pre_cursor_lane1_set(val, 0x7);
>>> + writel(val, csr_base + BRIDGE_8G_CFG_11);
>>> +
>>> + val = readl(csr_base + BRIDGE_8G_CFG_4);
>>> + val = (val & ~0x30) | (1 << 4);
>>> + writel(val, csr_base + BRIDGE_8G_CFG_4);
>>> +}
>>
>> Please document what you are actually setting here. If the configuration
>> of the lanes is always the same, why do you have to set it here. If not,
>> why do you set constant values?
>
> Good point. Let me check if these values should be constant or tune-able.
>
>>
>>> +static void xgene_pcie_setup_link(struct xgene_pcie_port *port)
>>> +{
>>> + void *csr_base = port->csr_base;
>>> + u32 val;
>>> +
>>> + val = readl(csr_base + BRIDGE_CFG_14);
>>> + val |= DIRECT_TO_8GTS_MASK;
>>> + val |= SUPPORT_5GTS_MASK;
>>> + val |= SUPPORT_8GTS_MASK;
>>> + val |= DIRECT_TO_5GTS_MASK;
>>> + writel(val, csr_base + BRIDGE_CFG_14);
>>> +
>>> + val = readl(csr_base + BRIDGE_CFG_14);
>>> + val &= ~ADVT_INFINITE_CREDITS;
>>> + writel(val, csr_base + BRIDGE_CFG_14);
>>> +
>>> + val = readl(csr_base + BRIDGE_8G_CFG_0);
>>> + val |= (val & ~0xf) | 7;
>>> + val |= (val & ~0xf00) | ((7 << 8) & 0xf00);
>>> + writel(val, csr_base + BRIDGE_8G_CFG_0);
>>> +
>>> + val = readl(csr_base + BRIDGE_8G_CFG_0);
>>> + val |= DWNSTRM_EQ_SKP_PHS_2_3;
>>> + writel(val, csr_base + BRIDGE_8G_CFG_0);
>>> +}
>>
>> Same here.
>>
>>> +static void xgene_pcie_program_core(void *csr_base)
>>> +{
>>> + u32 val;
>>> +
>>> + val = readl(csr_base + BRIDGE_CFG_0);
>>> + val |= AER_OPTIONAL_ERROR_EN;
>>> + writel(val, csr_base + BRIDGE_CFG_0);
>>> + writel(0x0, csr_base + INTXSTATUSMASK);
>>> + val = readl(csr_base + BRIDGE_CTRL_1);
>>> + val = (val & ~0xffff) | XGENE_PCIE_DEV_CTRL;
>>> + writel(val, csr_base + BRIDGE_CTRL_1);
>>> +}
>>
>> 'program_core'?
>
> Some of the PCIe core related misc configurations.
>
>>
>>> +static void xgene_pcie_poll_linkup(struct xgene_pcie_port *port, u32 *lanes)
>>> +{
>>> + void *csr_base = port->csr_base;
>>> + u32 val32;
>>> + u64 start_time, time;
>>> +
>>> + /*
>>> + * A component enters the LTSSM Detect state within
>>> + * 20ms of the end of fundamental core reset.
>>> + */
>>> + msleep(XGENE_LTSSM_DETECT_WAIT);
>>> + port->link_up = 0;
>>> + start_time = jiffies;
>>> + do {
>>> + val32 = readl(csr_base + PCIECORE_CTLANDSTATUS);
>>> + if (val32 & LINK_UP_MASK) {
>>> + port->link_up = 1;
>>> + port->link_speed = PIPE_PHY_RATE_RD(val32);
>>> + val32 = readl(csr_base + BRIDGE_STATUS_0);
>>> + *lanes = val32 >> 26;
>>> + }
>>> + time = jiffies_to_msecs(jiffies - start_time);
>>> + } while ((!port->link_up) || (time <= XGENE_LTSSM_L0_WAIT));
>>> +}
>>
>> Maybe another msleep() in the loop? It seems weird to first do an
>> unconditional sleep but then busy-wait for the result.
>
> ok.

This loop can execute for maximum 4 msec. So putting msleep(1) won't
get us much.

>
>>
>>> +static void xgene_pcie_setup_primary_bus(struct xgene_pcie_port *port,
>>> + u32 first_busno, u32 last_busno)
>>> +{
>>> + u32 val;
>>> + void *cfg_addr = port->cfg_base;
>>> +
>>> + val = readl(cfg_addr + PCI_PRIMARY_BUS);
>>> + val &= ~PCI_PRIMARY_BUS_MASK;
>>> + val |= (last_busno << 16) | ((first_busno + 1) << 8) | (first_busno);
>>> + writel(val, cfg_addr + PCI_PRIMARY_BUS);
>>> +}
>>
>> Please explain what you are doing here. As mentioned above, I would expect
>> that each domain has visibility of all 255 buses. You shouldn't need any hacks
>> where you try to artificially squeeze the ports into a single domain when
>> they are separate in hardware.
>
> ok. I will check and get back.

You are right. I have removed this hack. It will be fixed in next version.

>
>>
>>> +/*
>>> + * read configuration values from DTS
>>> + */
>>> +static int xgene_pcie_read_dts_config(struct xgene_pcie_port *port)
>>
>> The comment and function name don't seem to match what the function
>> does. The main purpose of this function seems to be to ioremap
>> the resources, which have nothing to with configuration.
>
> ok.
>
>>
>>> +{
>>> + struct device_node *np = port->node;
>>> + struct resource csr_res;
>>> + struct resource cfg_res;
>>> +
>>> + /* Get CSR space registers address */
>>> + if (of_address_to_resource(np, 0, &csr_res))
>>> + return -EINVAL;
>>> +
>>> + port->csr_base = devm_ioremap_nocache(port->dev, csr_res.start,
>>> + resource_size(&csr_res));
>>
>> You can also use platform_get_resource() to access the resource
>> that is already there, rather than creating another one.
>
> ok.
>
>>
>>> +static void xgene_pcie_setup_ob_reg(struct xgene_pcie_port *port,
>>> + u32 addr, u32 restype)
>>> +{
>>> + struct resource *res = NULL;
>>> + void *base = port->csr_base + addr;
>>> + resource_size_t size;
>>> + u64 cpu_addr = 0;
>>> + u64 pci_addr = 0;
>>> + u64 mask = 0;
>>> + u32 min_size = 0;
>>
>> A general note: don't initialize local variables to a bogus valus (e.g. 0)
>> in their declaration. It prevents the compiler from warning about
>> incorrect uses.
>
> ok.
>
>>
>>> + u32 flag = EN_REG;
>>
>> This one on the other hand is ok, because you are actually going to
>> use that value.
>>
>>> + switch (restype) {
>>> + case IORESOURCE_MEM:
>>> + res = &port->mem.res;
>>> + pci_addr = port->mem.pci_addr;
>>> + min_size = SZ_128M;
>>> + break;
>>> + case IORESOURCE_IO:
>>> + res = &port->io.res;
>>> + pci_addr = port->io.pci_addr;
>>> + min_size = 128;
>>> + flag |= OB_LO_IO;
>>> + break;
>>> + }
>>
>> I assume this works ok, but seems wrong in one detail: If the resource
>> is marked IORESOURCE_IO, res->start is supposed to be in I/O space, not
>> in memory space, which would make it the wrong number to program
>> into the hardware registers.
>
> Yes for using ioport resource. However we have decided to defer using
> it since 'pci_ioremap_io' is not yet supported from arm64 side.
>
> From HW point of view, for memory mapped IO space, it is nothing but a
> piece taken out of the ranges in address map for outbound accesses. So
> while configuring registers from SOC side, it should take the CPU
> address which is address from SOC address map. Right?
>
> Later on we can have a separate io resource like 'realio' similar to
> what pci-mvebu.c does.
>
>>
>>> +static int xgene_pcie_parse_map_ranges(struct xgene_pcie_port *port)
>>> +{
>>> + struct device_node *np = port->node;
>>> + struct of_pci_range range;
>>> + struct of_pci_range_parser parser;
>>> + struct device *dev = port->dev;
>>> +
>>> + if (of_pci_range_parser_init(&parser, np)) {
>>> + dev_err(dev, "missing ranges property\n");
>>> + return -EINVAL;
>>> + }
>>> +
>>> + /* Get the I/O, memory, config ranges from DT */
>>
>> The comment needs updating now that you don't read config space here any more.
>
> ok.
>
>>
>>> +/* X-Gene PCIe support maximum 3 inbound memory regions
>>> + * This function helps to select a region based on size of region
>>> + */
>>> +static int xgene_pcie_select_ib_reg(u64 size)
>>> +{
>>> + static u8 ib_reg_mask;
>>> +
>>> + if ((size > 4) && (size < SZ_16M) && !(ib_reg_mask & (1 << 1))) {
>>> + ib_reg_mask |= (1 << 1);
>>> + return 1;
>>> + }
>>> +
>>> + if ((size > SZ_1K) && (size < SZ_1T) && !(ib_reg_mask & (1 << 0))) {
>>> + ib_reg_mask |= (1 << 0);
>>> + return 0;
>>> + }
>>> +
>>> + if ((size > SZ_1M) && (size < SZ_1T) && !(ib_reg_mask & (1 << 2))) {
>>> + ib_reg_mask |= (1 << 2);
>>> + return 2;
>>> + }
>>> + return -EINVAL;
>>> +}
>>
>> Shouldn't the ib_reg_mask variable be per host bridge? Static variables
>> are dangerous if you ever get multiple instances of the hardware in one
>> system.
>
> Yes. You are right. Thanks.
>
>>
>>> +static int xgene_pcie_parse_map_dma_ranges(struct xgene_pcie_port *port)
>>> +{
>>> + struct device_node *np = port->node;
>>> + struct of_pci_range range;
>>> + struct of_pci_range_parser parser;
>>> + struct device *dev = port->dev;
>>> + int region;
>>> +
>>> + if (pci_dma_range_parser_init(&parser, np)) {
>>> + dev_err(dev, "missing dma-ranges property\n");
>>> + return -EINVAL;
>>> + }
>>> +
>>> + /* Get the dma-ranges from DT */
>>> + for_each_of_pci_range(&parser, &range) {
>>> + u64 restype = range.flags & IORESOURCE_TYPE_BITS;
>>> + u64 end = range.cpu_addr + range.size - 1;
>>> + dev_dbg(port->dev, "0x%08x 0x%016llx..0x%016llx -> 0x%016llx\n",
>>> + range.flags, range.cpu_addr, end, range.pci_addr);
>>> + region = xgene_pcie_select_ib_reg(range.size);
>>> + if (region == -EINVAL) {
>>> + dev_warn(port->dev, "invalid pcie dma-range config\n");
>>> + continue;
>>> + }
>>> + xgene_pcie_setup_ib_reg(port, &range, restype, region);
>>> + }
>>> + return 0;
>>> +}
>>
>> I guess is could even be a local variable in this function, which you pass
>> by reference.
>>
>>> +
>>> +static int xgene_pcie_setup(int nr, struct pci_sys_data *sys)
>>> +{
>>> + struct xgene_pcie_port *pp = xgene_pcie_sys_to_port(sys);
>>> +
>>> + if (pp == NULL)
>>> + return 0;
>>> +
>>> + sys->mem_offset = pp->mem.res.start - pp->mem.pci_addr;
>>> + pci_add_resource_offset(&sys->resources, &pp->mem.res,
>>> + sys->mem_offset);
>>> + return 1;
>>> +}
>>
>> Please follow the regular error handling conventions, which are to
>> pass a negative errno value on error and zero on success.
>
> ok.
>
>>
>> Also, what would be a reason for the port to be zero here? If
>> it's something that can't happen in practice, don't try to handle
>> it gracefully. You can use BUG_ON() for fatal conditions that
>> are supposed to be impossible to reach.
>
> This function is a hook to upper layer. We register nr_controllers in
> hw_pci structure as MAX_PORTS we support. It can happen that number of
> ports actually enabled from device tree are less than the number in
> nr_controllers.
>
>>
>>> +static struct pci_bus __init *xgene_pcie_scan_bus(int nr,
>>> + struct pci_sys_data *sys)
>>> +{
>>> + struct xgene_pcie_port *pp = xgene_pcie_sys_to_port(sys);
>>> +
>>> + pp->first_busno = sys->busnr;
>>> + xgene_pcie_setup_primary_bus(pp, sys->busnr, 0xff);
>>> + return pci_scan_root_bus(NULL, sys->busnr, &xgene_pcie_ops,
>>> + sys, &sys->resources);
>>> +}
>>
>> You have a number of helper functions that don't seem to gain
>> much at all. Just move the call to pci_scan_root_bus() into
>> xgene_pcie_setup_primary_bus() here, and use that as the .scan
>> callback.
>>
>
> I can do that if I can get rid of setup_primary_bus api. Let me check.
>
>>
>>> + if (!port->link_up)
>>> + dev_info(port->dev, "(rc) link down\n");
>>> + else
>>> + dev_info(port->dev, "(rc) x%d gen-%d link up\n",
>>> + lanes, port->link_speed + 1);
>>> +#ifdef CONFIG_PCI_DOMAINS
>>> + xgene_pcie_hw.domain++;
>>> +#endif
>>> + xgene_pcie_hw.private_data[index++] = port;
>>> + platform_set_drvdata(pdev, port);
>>> + return 0;
>>> +}
>>
>> Do you have multiple domains or not? I don't see how it can work if you
>> make the domain setup conditional on a kernel configuration option.
>> If you in fact have multiple domains, make sure in Kconfig that
>> CONFIG_PCI_DOMAINS is enabled. Otherwise don't mess with the domain
>> number...
>
> It is enabled in Kconfig.
>
>>
>>> +static const struct of_device_id xgene_pcie_match_table[] __initconst = {
>>> + {.compatible = "apm,xgene-pcie",},
>>> + {},
>>> +};
>>
>> Another general note: Your "compatible" strings are rather unspecific.
>> Do you have a version number for this IP block? I suppose that it's related
>> to one that has been used in other chips before, or will be used in future
>> chips, if it's not actually licensed from some other company.
>
> I will have to check this.
>

We have decided to stick with current compatible string for now.

>>
>>> +static int __init xgene_pcie_init(void)
>>> +{
>>> + void *private;
>>> + int ret;
>>> +
>>> + pr_info("X-Gene: PCIe driver\n");
>>> +
>>> + /* allocate private data to keep xgene_pcie_port information */
>>> + private = kzalloc((XGENE_PCIE_MAX_PORTS * sizeof(void *)), GFP_KERNEL);
>>
>> This should not be done unconditionally: There is no point in printing
>> a message or allocating memory if you don't actually run on a system
>> with this device.
>
> I am doing this here because I have one instance of hw_pci structure
> with multiple pcie controllers. I can't do it from probe since it will
> be called once per instance in device tree.
>
>>
>>> + if (private == NULL)
>>> + return -ENOMEM;
>>
>> Style: if you are testing for an object, just write 'if (private)' or
>> 'if (!private)', but don't compare against NULL.
>
> ok.
>
>>
>>> + xgene_pcie_hw.private_data = private;
>>> + ret = platform_driver_probe(&xgene_pcie_driver,
>>> + xgene_pcie_probe_bridge);
>>> + if (ret)
>>> + return ret;
>>> + pci_common_init(&xgene_pcie_hw);
>>> + return 0;
>>
>> This seems wrong: You should not use platform_driver_probe() because
>> that has issues with deferred probing.
>
> I think 'platform_driver_probe' prevents the deferred probing.
> 'pci_common_init' needs to be called only once with current driver
> structure. The probes for all pcie ports should be finished (ports
> initialized) before 'pci_common_init' gets called.
>
>>
>> Arnd

2014-01-25 20:12:01

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [RFC PATCH V2 1/4] pci: APM X-Gene PCIe controller driver

On Friday 24 January 2014 13:28:22 Tanmay Inamdar wrote:
> On Thu, Jan 16, 2014 at 5:10 PM, Tanmay Inamdar <[email protected]> wrote:
> > On Wed, Jan 15, 2014 at 4:39 AM, Arnd Bergmann <[email protected]> wrote:
> >> On Wednesday 15 January 2014, Tanmay Inamdar wrote:

> >>
> >>> +static void xgene_pcie_poll_linkup(struct xgene_pcie_port *port, u32 *lanes)
> >>> +{
> >>> + void *csr_base = port->csr_base;
> >>> + u32 val32;
> >>> + u64 start_time, time;
> >>> +
> >>> + /*
> >>> + * A component enters the LTSSM Detect state within
> >>> + * 20ms of the end of fundamental core reset.
> >>> + */
> >>> + msleep(XGENE_LTSSM_DETECT_WAIT);
> >>> + port->link_up = 0;
> >>> + start_time = jiffies;
> >>> + do {
> >>> + val32 = readl(csr_base + PCIECORE_CTLANDSTATUS);
> >>> + if (val32 & LINK_UP_MASK) {
> >>> + port->link_up = 1;
> >>> + port->link_speed = PIPE_PHY_RATE_RD(val32);
> >>> + val32 = readl(csr_base + BRIDGE_STATUS_0);
> >>> + *lanes = val32 >> 26;
> >>> + }
> >>> + time = jiffies_to_msecs(jiffies - start_time);
> >>> + } while ((!port->link_up) || (time <= XGENE_LTSSM_L0_WAIT));
> >>> +}
> >>
> >> Maybe another msleep() in the loop? It seems weird to first do an
> >> unconditional sleep but then busy-wait for the result.
> >
> > ok.
>
> This loop can execute for maximum 4 msec. So putting msleep(1) won't
> get us much.

4 msec is still quite a long time for a busy loop that can be spent doing
useful work in another thread.

> >>
> >> Another general note: Your "compatible" strings are rather unspecific.
> >> Do you have a version number for this IP block? I suppose that it's related
> >> to one that has been used in other chips before, or will be used in future
> >> chips, if it's not actually licensed from some other company.
> >
> > I will have to check this.
> >
>
> We have decided to stick with current compatible string for now.

Can you elaborate on your reasoning? Does this mean X-Gene is a one-off
product and you won't be doing any new chips based on the same hardware
components?

Arnd

2014-01-27 22:54:32

by Tanmay Inamdar

[permalink] [raw]
Subject: Re: [RFC PATCH V2 1/4] pci: APM X-Gene PCIe controller driver

On Sat, Jan 25, 2014 at 12:11 PM, Arnd Bergmann <[email protected]> wrote:
> On Friday 24 January 2014 13:28:22 Tanmay Inamdar wrote:
>> On Thu, Jan 16, 2014 at 5:10 PM, Tanmay Inamdar <[email protected]> wrote:
>> > On Wed, Jan 15, 2014 at 4:39 AM, Arnd Bergmann <[email protected]> wrote:
>> >> On Wednesday 15 January 2014, Tanmay Inamdar wrote:
>
>> >>
>> >>> +static void xgene_pcie_poll_linkup(struct xgene_pcie_port *port, u32 *lanes)
>> >>> +{
>> >>> + void *csr_base = port->csr_base;
>> >>> + u32 val32;
>> >>> + u64 start_time, time;
>> >>> +
>> >>> + /*
>> >>> + * A component enters the LTSSM Detect state within
>> >>> + * 20ms of the end of fundamental core reset.
>> >>> + */
>> >>> + msleep(XGENE_LTSSM_DETECT_WAIT);
>> >>> + port->link_up = 0;
>> >>> + start_time = jiffies;
>> >>> + do {
>> >>> + val32 = readl(csr_base + PCIECORE_CTLANDSTATUS);
>> >>> + if (val32 & LINK_UP_MASK) {
>> >>> + port->link_up = 1;
>> >>> + port->link_speed = PIPE_PHY_RATE_RD(val32);
>> >>> + val32 = readl(csr_base + BRIDGE_STATUS_0);
>> >>> + *lanes = val32 >> 26;
>> >>> + }
>> >>> + time = jiffies_to_msecs(jiffies - start_time);
>> >>> + } while ((!port->link_up) || (time <= XGENE_LTSSM_L0_WAIT));
>> >>> +}
>> >>
>> >> Maybe another msleep() in the loop? It seems weird to first do an
>> >> unconditional sleep but then busy-wait for the result.
>> >
>> > ok.
>>
>> This loop can execute for maximum 4 msec. So putting msleep(1) won't
>> get us much.
>
> 4 msec is still quite a long time for a busy loop that can be spent doing
> useful work in another thread.
>

Right. If 'msleep(1)' is used, then 'checkpatch' throws a warning
saying that it can actually sleep for 20ms in some cases. I will check
if 'usleep_range' is useful here.

>> >>
>> >> Another general note: Your "compatible" strings are rather unspecific.
>> >> Do you have a version number for this IP block? I suppose that it's related
>> >> to one that has been used in other chips before, or will be used in future
>> >> chips, if it's not actually licensed from some other company.
>> >
>> > I will have to check this.
>> >
>>
>> We have decided to stick with current compatible string for now.
>
> Can you elaborate on your reasoning? Does this mean X-Gene is a one-off
> product and you won't be doing any new chips based on the same hardware
> components?

The current convention is to key upon the family name - X-Gene. Future
chips will also be a part of X-Gene family. Right now it is unclear if
there are any obvious feature additions to be done in Linux PCIe
driver. Until then same driver is expected to work as is in future
chips.

>
> Arnd

2014-01-28 00:55:51

by Bjorn Helgaas

[permalink] [raw]
Subject: Re: [RFC PATCH V2 1/4] pci: APM X-Gene PCIe controller driver

We're only seeing Arnd's side of the conversation on linux-pci.
Tanmay, are your messages being rejected because they're too "fancy",
per the definition here: http://vger.kernel.org/majordomo-info.html ?

On Sat, Jan 25, 2014 at 1:11 PM, Arnd Bergmann <[email protected]> wrote:
> On Friday 24 January 2014 13:28:22 Tanmay Inamdar wrote:
>> On Thu, Jan 16, 2014 at 5:10 PM, Tanmay Inamdar <[email protected]> wrote:
>> > On Wed, Jan 15, 2014 at 4:39 AM, Arnd Bergmann <[email protected]> wrote:
>> >> On Wednesday 15 January 2014, Tanmay Inamdar wrote:
>
>> >>
>> >>> +static void xgene_pcie_poll_linkup(struct xgene_pcie_port *port, u32 *lanes)
>> >>> +{
>> >>> + void *csr_base = port->csr_base;
>> >>> + u32 val32;
>> >>> + u64 start_time, time;
>> >>> +
>> >>> + /*
>> >>> + * A component enters the LTSSM Detect state within
>> >>> + * 20ms of the end of fundamental core reset.
>> >>> + */
>> >>> + msleep(XGENE_LTSSM_DETECT_WAIT);
>> >>> + port->link_up = 0;
>> >>> + start_time = jiffies;
>> >>> + do {
>> >>> + val32 = readl(csr_base + PCIECORE_CTLANDSTATUS);
>> >>> + if (val32 & LINK_UP_MASK) {
>> >>> + port->link_up = 1;
>> >>> + port->link_speed = PIPE_PHY_RATE_RD(val32);
>> >>> + val32 = readl(csr_base + BRIDGE_STATUS_0);
>> >>> + *lanes = val32 >> 26;
>> >>> + }
>> >>> + time = jiffies_to_msecs(jiffies - start_time);
>> >>> + } while ((!port->link_up) || (time <= XGENE_LTSSM_L0_WAIT));
>> >>> +}
>> >>
>> >> Maybe another msleep() in the loop? It seems weird to first do an
>> >> unconditional sleep but then busy-wait for the result.
>> >
>> > ok.
>>
>> This loop can execute for maximum 4 msec. So putting msleep(1) won't
>> get us much.
>
> 4 msec is still quite a long time for a busy loop that can be spent doing
> useful work in another thread.
>
>> >>
>> >> Another general note: Your "compatible" strings are rather unspecific.
>> >> Do you have a version number for this IP block? I suppose that it's related
>> >> to one that has been used in other chips before, or will be used in future
>> >> chips, if it's not actually licensed from some other company.
>> >
>> > I will have to check this.
>> >
>>
>> We have decided to stick with current compatible string for now.
>
> Can you elaborate on your reasoning? Does this mean X-Gene is a one-off
> product and you won't be doing any new chips based on the same hardware
> components?
>
> Arnd

2014-01-28 02:02:24

by Tanmay Inamdar

[permalink] [raw]
Subject: Re: [RFC PATCH V2 1/4] pci: APM X-Gene PCIe controller driver

On Mon, Jan 27, 2014 at 4:55 PM, Bjorn Helgaas <[email protected]> wrote:
> We're only seeing Arnd's side of the conversation on linux-pci.
> Tanmay, are your messages being rejected because they're too "fancy",
> per the definition here: http://vger.kernel.org/majordomo-info.html ?
>

Thanks for pointing out. I am not sure though what's being detected as
fancy. I checked that my emails are received as plaintext on
majordomo. They are also displayed fine on LKML.

In this email, I have tried to keep the format of to: and cc: same as
first email in the thread. Not sure if this fixes the problem.

Again.. sorry for spamming.

> On Sat, Jan 25, 2014 at 1:11 PM, Arnd Bergmann <[email protected]> wrote:
>> On Friday 24 January 2014 13:28:22 Tanmay Inamdar wrote:
>>> On Thu, Jan 16, 2014 at 5:10 PM, Tanmay Inamdar <[email protected]> wrote:
>>> > On Wed, Jan 15, 2014 at 4:39 AM, Arnd Bergmann <[email protected]> wrote:
>>> >> On Wednesday 15 January 2014, Tanmay Inamdar wrote:
>>
>>> >>
>>> >>> +static void xgene_pcie_poll_linkup(struct xgene_pcie_port *port, u32 *lanes)
>>> >>> +{
>>> >>> + void *csr_base = port->csr_base;
>>> >>> + u32 val32;
>>> >>> + u64 start_time, time;
>>> >>> +
>>> >>> + /*
>>> >>> + * A component enters the LTSSM Detect state within
>>> >>> + * 20ms of the end of fundamental core reset.
>>> >>> + */
>>> >>> + msleep(XGENE_LTSSM_DETECT_WAIT);
>>> >>> + port->link_up = 0;
>>> >>> + start_time = jiffies;
>>> >>> + do {
>>> >>> + val32 = readl(csr_base + PCIECORE_CTLANDSTATUS);
>>> >>> + if (val32 & LINK_UP_MASK) {
>>> >>> + port->link_up = 1;
>>> >>> + port->link_speed = PIPE_PHY_RATE_RD(val32);
>>> >>> + val32 = readl(csr_base + BRIDGE_STATUS_0);
>>> >>> + *lanes = val32 >> 26;
>>> >>> + }
>>> >>> + time = jiffies_to_msecs(jiffies - start_time);
>>> >>> + } while ((!port->link_up) || (time <= XGENE_LTSSM_L0_WAIT));
>>> >>> +}
>>> >>
>>> >> Maybe another msleep() in the loop? It seems weird to first do an
>>> >> unconditional sleep but then busy-wait for the result.
>>> >
>>> > ok.
>>>
>>> This loop can execute for maximum 4 msec. So putting msleep(1) won't
>>> get us much.
>>
>> 4 msec is still quite a long time for a busy loop that can be spent doing
>> useful work in another thread.
>>
>>> >>
>>> >> Another general note: Your "compatible" strings are rather unspecific.
>>> >> Do you have a version number for this IP block? I suppose that it's related
>>> >> to one that has been used in other chips before, or will be used in future
>>> >> chips, if it's not actually licensed from some other company.
>>> >
>>> > I will have to check this.
>>> >
>>>
>>> We have decided to stick with current compatible string for now.
>>
>> Can you elaborate on your reasoning? Does this mean X-Gene is a one-off
>> product and you won't be doing any new chips based on the same hardware
>> components?
>>
>> Arnd

2014-01-29 19:36:56

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [RFC PATCH V2 1/4] pci: APM X-Gene PCIe controller driver

On Monday 27 January 2014, Tanmay Inamdar wrote:
> On Sat, Jan 25, 2014 at 12:11 PM, Arnd Bergmann <[email protected]> wrote:
> > On Friday 24 January 2014 13:28:22 Tanmay Inamdar wrote:
> >> On Thu, Jan 16, 2014 at 5:10 PM, Tanmay Inamdar <[email protected]> wrote:
> >> > On Wed, Jan 15, 2014 at 4:39 AM, Arnd Bergmann <[email protected]> wrote:
> >> >> On Wednesday 15 January 2014, Tanmay Inamdar wrote:
> >> >>
> >> >> Maybe another msleep() in the loop? It seems weird to first do an
> >> >> unconditional sleep but then busy-wait for the result.
> >> >
> >> > ok.
> >>
> >> This loop can execute for maximum 4 msec. So putting msleep(1) won't
> >> get us much.
> >
> > 4 msec is still quite a long time for a busy loop that can be spent doing
> > useful work in another thread.
> >
>
> Right. If 'msleep(1)' is used, then 'checkpatch' throws a warning
> saying that it can actually sleep for 20ms in some cases. I will check
> if 'usleep_range' is useful here.

Sound good. This is really a false positive from checkpatch though,
with the timeout handling in place, everything's fine even with
msleep(1).

> >> >>
> >> >> Another general note: Your "compatible" strings are rather unspecific.
> >> >> Do you have a version number for this IP block? I suppose that it's related
> >> >> to one that has been used in other chips before, or will be used in future
> >> >> chips, if it's not actually licensed from some other company.
> >> >
> >> > I will have to check this.
> >> >
> >>
> >> We have decided to stick with current compatible string for now.
> >
> > Can you elaborate on your reasoning? Does this mean X-Gene is a one-off
> > product and you won't be doing any new chips based on the same hardware
> > components?
>
> The current convention is to key upon the family name - X-Gene. Future
> chips will also be a part of X-Gene family. Right now it is unclear if
> there are any obvious feature additions to be done in Linux PCIe
> driver. Until then same driver is expected to work as is in future
> chips.

This is not enough for me. Of course you hope that things keep working,
but experience shows that sometimes hardware has slight differences that
you need to work around later. It's better to always be specific and
at least as a secondary identifier list the exact model of the component,
or if that is not know, the model of the SoC. The driver can bind to
the most generic string, but in DT you should have a specific one as
well.

You could for instance have something like

compatible = "apm,xgene-1234w78-pcie", "thirdparty,pcie-1.23", "apm,xgene-pcie", "thirdparty,pcie";

as an example where you licensed the pcie block version 1.23 from a company
named thirdparty and integrated it into the xgene variant with product
code 1234w78.

Arnd