| Message ID | 1389742458-7693-2-git-send-email-tinamdar@apm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <tinamdar@apm.com> 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
On Wed, Jan 15, 2014 at 4:39 AM, Arnd Bergmann <arnd@arndb.de> 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 <tinamdar@apm.com> > > 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
On Friday 17 January 2014, Tanmay Inamdar wrote: > On Wed, Jan 15, 2014 at 4:39 AM, Arnd Bergmann <arnd@arndb.de> 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 <tinamdar@apm.com> > > > > 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
On Thu, Jan 16, 2014 at 5:10 PM, Tanmay Inamdar <tinamdar@apm.com> wrote: > On Wed, Jan 15, 2014 at 4:39 AM, Arnd Bergmann <arnd@arndb.de> 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 <tinamdar@apm.com> >> >> 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
On Friday 24 January 2014 13:28:22 Tanmay Inamdar wrote: > On Thu, Jan 16, 2014 at 5:10 PM, Tanmay Inamdar <tinamdar@apm.com> wrote: > > On Wed, Jan 15, 2014 at 4:39 AM, Arnd Bergmann <arnd@arndb.de> 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
On Sat, Jan 25, 2014 at 12:11 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Friday 24 January 2014 13:28:22 Tanmay Inamdar wrote: >> On Thu, Jan 16, 2014 at 5:10 PM, Tanmay Inamdar <tinamdar@apm.com> wrote: >> > On Wed, Jan 15, 2014 at 4:39 AM, Arnd Bergmann <arnd@arndb.de> 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
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 <arnd@arndb.de> wrote: > On Friday 24 January 2014 13:28:22 Tanmay Inamdar wrote: >> On Thu, Jan 16, 2014 at 5:10 PM, Tanmay Inamdar <tinamdar@apm.com> wrote: >> > On Wed, Jan 15, 2014 at 4:39 AM, Arnd Bergmann <arnd@arndb.de> 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
On Mon, Jan 27, 2014 at 4:55 PM, Bjorn Helgaas <bhelgaas@google.com> 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 <arnd@arndb.de> wrote: >> On Friday 24 January 2014 13:28:22 Tanmay Inamdar wrote: >>> On Thu, Jan 16, 2014 at 5:10 PM, Tanmay Inamdar <tinamdar@apm.com> wrote: >>> > On Wed, Jan 15, 2014 at 4:39 AM, Arnd Bergmann <arnd@arndb.de> 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
On Monday 27 January 2014, Tanmay Inamdar wrote: > On Sat, Jan 25, 2014 at 12:11 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Friday 24 January 2014 13:28:22 Tanmay Inamdar wrote: > >> On Thu, Jan 16, 2014 at 5:10 PM, Tanmay Inamdar <tinamdar@apm.com> wrote: > >> > On Wed, Jan 15, 2014 at 4:39 AM, Arnd Bergmann <arnd@arndb.de> 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
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 <tinamdar@apm.com>. + * + * 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 <tinamdar@apm.com>"); +MODULE_DESCRIPTION("APM X-Gene PCIe driver"); +MODULE_LICENSE("GPL v2");
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 <tinamdar@apm.com> --- 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