On Friday, May 20, 2016 6:29:16 PM CEST Shawn Lin wrote: > +static int rockchip_pcie_wr_own_conf(struct rockchip_pcie_port *pp, > + int where, int size, u32 val) > +{ > + u32 tmp; > + int offset; > + > + offset = (where & (~0x3)); > + tmp = readl(pp->apb_base + PCIE_RC_CONFIG_BASE + offset); > + if (size == 4) { > + writel(val, pp->apb_base + PCIE_RC_CONFIG_BASE + where); > + } else if (size == 2) { > + if (where & 0x2) > + tmp = ((tmp & 0xffff) | (val << 16)); > + else > + tmp = ((tmp & 0xffff0000) | val); > + > + writel(tmp, pp->apb_base + PCIE_RC_CONFIG_BASE + offset); > + } else if (size == 1) { > + if ((where & 0x3) == 0) > + tmp = ((tmp & (~0xff)) | val); > + else if ((where & 0x3) == 1) > + tmp = ((tmp & (~0xff00)) | (val << 8)); > + else if ((where & 0x3) == 2) > + tmp = ((tmp & (~0xff0000)) | (val << 16)); > + else if ((where & 0x3) == 3) > + tmp = ((tmp & (~0xff000000)) | (val << 24)); > + > + writel(tmp, pp->apb_base + PCIE_RC_CONFIG_BASE + offset); > + } else { > + return PCIBIOS_BAD_REGISTER_NUMBER; > + } > + return PCIBIOS_SUCCESSFUL; > +} Why can't you access the individual sub-word registers here? > + > +static int rockchip_pcie_rd_other_conf(struct rockchip_pcie_port *pp, > + struct pci_bus *bus, u32 devfn, > + int where, int size, u32 *val) > +{ > + u32 busdev; > + > + busdev = PCIE_ECAM_ADDR(bus->number, PCI_SLOT(devfn), > + PCI_FUNC(devfn), where); > + > + if (busdev & (size - 1)) { > + *val = 0; > + return PCIBIOS_BAD_REGISTER_NUMBER; > + } > + > + if (size == 4) { > + *val = readl(pp->reg_base + busdev); > + } else if (size == 2) { > + *val = readw(pp->reg_base + busdev); > + } else if (size == 1) { > + *val = readb(pp->reg_base + busdev); > + } else { > + *val = 0; > + return PCIBIOS_BAD_REGISTER_NUMBER; > + } > + return PCIBIOS_SUCCESSFUL; > +} > + This looks like the normal ECAM operations, you could just call those. > + while (time_before(jiffies, timeout)) { > + regmap_read(port->grf, port->pcie_status, &status); > + if ((status & (1 << 9))) { > + dev_info(port->dev, "pll locked!\n"); > + err = 0; > + break; > + } > + } Maybe add an msleep(1) here to avoid busy-looping? > + for (reg_no = 0; reg_no < (port->mem_size >> 20); reg_no++) { > + err = rockchip_pcie_prog_ob_atu(port, reg_no + 1, > + AXI_WRAPPER_MEM_WRITE, > + 20 - 1, > + port->mem_bus_addr + > + (reg_no << 20), > + 0); > + if (err) { > + dev_err(dev, "Program RC outbound atu failed\n"); > + return err; > + } > + } What if there is more than one outbound memory window, e.g. prefetchable and non-prefetchable? Where do you set the I/O window? > + err = rockchip_pcie_prog_ib_atu(port, 2, 32 - 1, 0x0, 0); > + if (err) { > + dev_err(dev, "Program RC inbound atu failed\n"); > + return err; > + } And this doesn't seem to reflect the DMA ranges. > + > + port->root_bus_nr = port->busn->start; > + if (IS_ENABLED(CONFIG_PCI_MSI)) { > + bus = pci_scan_root_bus_msi(port->dev, port->root_bus_nr, > + &rockchip_pcie_ops, port, &res, > + port->msi); > + } else { > + bus = pci_scan_root_bus(&pdev->dev, 0, > + &rockchip_pcie_ops, port, &res); PCI_MSI is selected unconditionally from Kconfig for this driver, so no need for the compile-time check here. > + > +static int rockchip_pcie_remove(struct platform_device *pdev) > +{ > + struct rockchip_pcie_port *port = platform_get_drvdata(pdev); > + > + clk_disable_unprepare(port->hclk_pcie); > + clk_disable_unprepare(port->aclk_perf_pcie); > + clk_disable_unprepare(port->aclk_pcie); > + clk_disable_unprepare(port->clk_pciephy_ref); > + > + return 0; > +} You don't seem to remove the child devices here. Have you tried unloading the module? Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html