Hello.
On 05/12/2014 02:57 PM, Phil Edworthy wrote:
I'm investigating an imprecise external abort occurring once userland is
started when I have NetMos PCIe serial card inserted and the '8250_pci' driver
enabled and I have found some issues in this driver, while at it...
This PCIe Host driver currently does not support MSI, so cards
fall back to INTx interrupts.
Signed-off-by: Phil Edworthy <phil.edworthy@xxxxxxxxxxx>
[...]
diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
new file mode 100644
index 0000000..3c524b9
--- /dev/null
+++ b/drivers/pci/host/pcie-rcar.c
@@ -0,0 +1,768 @@
[...]
+#define PCI_MAX_RESOURCES 4
As a side note, this risks collision with <linux/pci*.h>...
+static void pci_write_reg(struct rcar_pcie *pcie, unsigned long val,
+ unsigned long reg)
+{
+ writel(val, pcie->base + reg);
+}
+
+static unsigned long pci_read_reg(struct rcar_pcie *pcie, unsigned long reg)
+{
+ return readl(pcie->base + reg);
+}
As a side note, these functions are hardly needed, and risk collision too...
+
+enum {
+ PCI_ACCESS_READ,
+ PCI_ACCESS_WRITE,
These risk collision too...
+static void rcar_pcie_setup_window(int win, struct resource *res,
+ struct rcar_pcie *pcie)
As a side note, 'res' parameter is hardly needed here, as the function
always gets
called with the resources contained within 'struct rcar_pcie'...
+{
+ /* Setup PCIe address space mappings for each resource */
+ resource_size_t size;
+ u32 mask;
+
+ pci_write_reg(pcie, 0x00000000, PCIEPTCTLR(win));
+
+ /*
+ * The PAMR mask is calculated in units of 128Bytes, which
+ * keeps things pretty simple.
+ */
+ size = resource_size(res);
+ mask = (roundup_pow_of_two(size) / SZ_128) - 1;
+ pci_write_reg(pcie, mask << 7, PCIEPAMR(win));
+
+ pci_write_reg(pcie, upper_32_bits(res->start), PCIEPARH(win));
+ pci_write_reg(pcie, lower_32_bits(res->start), PCIEPARL(win));
My investigation showed and printk() here confirmed that instead of a PCI
bus address here we have CPU address written to these registers:
rcar_pcie_setup_window: window 0, resource [io 0xfe100000-0xfe1fffff]
rcar_pcie_setup_window: window 1, resource [mem 0xfe200000-0xfe3fffff]
rcar_pcie_setup_window: window 2, resource [mem 0x30000000-0x37ffffff]
rcar_pcie_setup_window: window 3, resource [mem 0x38000000-0x3fffffff pref]
rcar-pcie fe000000.pcie: PCI host bridge to bus 0000:00
+
+ /* First resource is for IO */
+ mask = PAR_ENABLE;
+ if (res->flags & IORESOURCE_IO)
+ mask |= IO_SPACE;
For the memory space this works OK as you're identity-mapping the memory
ranges in your device trees. However, for the I/O space this means that it
won't work as the BARs in the PCIe devices get programmed with the PCI bus
addresses but the PCIe window translation register is programmed with a CPU
address which don't at all match (given your device trees) and hence one can't
access the card's I/O mapped registers at all...
+
+ pci_write_reg(pcie, mask, PCIEPTCTLR(win));
+}
+
+static int rcar_pcie_setup(int nr, struct pci_sys_data *sys)
+{
+ struct rcar_pcie *pcie = sys_to_pcie(sys);
+ struct resource *res;
+ int i;
+
+ pcie->root_bus_nr = -1;
+
+ /* Setup PCI resources */
+ for (i = 0; i < PCI_MAX_RESOURCES; i++) {
+
+ res = &pcie->res[i];
+ if (!res->flags)
+ continue;
+
+ rcar_pcie_setup_window(i, res, pcie);
+
+ if (res->flags & IORESOURCE_IO)
+ pci_ioremap_io(nr * SZ_64K, res->start);
I'm not sure why are you not calling pci_add_resource() for I/O space...
Also, this sets up only 64 KiB of I/O ports while your device tree describes
I/O space 1 MiB is size.
+ else
+ pci_add_resource(&sys->resources, res);
+ }
+ pci_add_resource(&sys->resources, &pcie->busn);
+
+ return 1;
+}
[...]
+static int rcar_pcie_hw_init(struct rcar_pcie *pcie)
+{
+ int err;
+
+ /* Begin initialization */
+ pci_write_reg(pcie, 0, PCIETCTLR);
+
+ /* Set mode */
+ pci_write_reg(pcie, 1, PCIEMSR);
+
+ /*
+ * Initial header for port config space is type 1, set the device
+ * class to match. Hardware takes care of propagating the IDSETR
+ * settings, so there is no need to bother with a quirk.
+ */
+ pci_write_reg(pcie, PCI_CLASS_BRIDGE_PCI << 16, IDSETR1);
Hm, shouldn't this be a host bridge? I've noticed that the bridge's I/O
and memory base/limit registers are left uninitialized even though the BARs of
the PICe devices behind this bridge are assigned.
+
+ /*
+ * Setup Secondary Bus Number & Subordinate Bus Number, even though
+ * they aren't used, to avoid bridge being detected as broken.
+ */
+ rcar_rmw32(pcie, RCONF(PCI_SECONDARY_BUS), 0xff, 1);
+ rcar_rmw32(pcie, RCONF(PCI_SUBORDINATE_BUS), 0xff, 1);
+
+ /* Initialize default capabilities. */
+ rcar_rmw32(pcie, REXPCAP(0), 0, PCI_CAP_ID_EXP);
+ rcar_rmw32(pcie, REXPCAP(PCI_EXP_FLAGS),
+ PCI_EXP_FLAGS_TYPE, PCI_EXP_TYPE_ROOT_PORT << 4);
+ rcar_rmw32(pcie, RCONF(PCI_HEADER_TYPE), 0x7f,
+ PCI_HEADER_TYPE_BRIDGE);
+
+ /* Enable data link layer active state reporting */
+ rcar_rmw32(pcie, REXPCAP(PCI_EXP_LNKCAP), 0, PCI_EXP_LNKCAP_DLLLARC);
+
+ /* Write out the physical slot number = 0 */
+ rcar_rmw32(pcie, REXPCAP(PCI_EXP_SLTCAP), PCI_EXP_SLTCAP_PSN, 0);
+
+ /* Set the completion timer timeout to the maximum 50ms. */
+ rcar_rmw32(pcie, TLCTLR+1, 0x3f, 50);
Missing spaces around '+'...
+
+ /* Terminate list of capabilities (Next Capability Offset=0) */
+ rcar_rmw32(pcie, RVCCAP(0), 0xfff0, 0);
+
+ /* Enable MAC data scrambling. */
+ rcar_rmw32(pcie, MACCTLR, SCRAMBLE_DISABLE, 0);
Doesn't the comment contradict the code here?
+
+ /* Finish initialization - establish a PCI Express link */
+ pci_write_reg(pcie, CFINIT, PCIETCTLR);
+
+ /* This will timeout if we don't have a link. */
+ err = rcar_pcie_wait_for_dl(pcie);
+ if (err)
+ return err;
+
+ /* Enable INTx interrupts */
+ rcar_rmw32(pcie, PCIEINTXR, 0, 0xF << 8);
+
+ /* Enable slave Bus Mastering */
+ rcar_rmw32(pcie, RCONF(PCI_STATUS), PCI_STATUS_DEVSEL_MASK,
+ PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER |
+ PCI_STATUS_CAP_LIST | PCI_STATUS_DEVSEL_FAST);
Hmm, you're mixing up PCI control/status registers' bits here; they're
two 16-bit registers! So you're writing to 3 reserved LSBs of the PCI status
register...
+static int rcar_pcie_get_resources(struct platform_device *pdev,
+ struct rcar_pcie *pcie)
+{
+ struct resource res;
+ int err;
+
+ err = of_address_to_resource(pdev->dev.of_node, 0, &res);
BTW, you could use platfrom_get_resource() and save on your local variable
and the error check -- devm_ioremap_resource() does it.
+ if (err)
+ return err;
+
+ pcie->clk = devm_clk_get(&pdev->dev, "pcie");
+ if (IS_ERR(pcie->clk)) {
+ dev_err(pcie->dev, "cannot get platform clock\n");
+ return PTR_ERR(pcie->clk);
+ }
+ err = clk_prepare_enable(pcie->clk);
+ if (err)
+ goto fail_clk;
+
+ pcie->bus_clk = devm_clk_get(&pdev->dev, "pcie_bus");
+ if (IS_ERR(pcie->bus_clk)) {
+ dev_err(pcie->dev, "cannot get pcie bus clock\n");
+ err = PTR_ERR(pcie->bus_clk);
+ goto fail_clk;
+ }
+ err = clk_prepare_enable(pcie->bus_clk);
+ if (err)
+ goto err_map_reg;
+
+ pcie->base = devm_ioremap_resource(&pdev->dev, &res);
+ if (IS_ERR(pcie->base)) {
+ err = PTR_ERR(pcie->base);
+ goto err_map_reg;
+ }
+
+ return 0;
+
+err_map_reg:
+ clk_disable_unprepare(pcie->bus_clk);
+fail_clk:
+ clk_disable_unprepare(pcie->clk);
+
+ return err;
+}
+
+static int rcar_pcie_inbound_ranges(struct rcar_pcie *pcie,
+ struct of_pci_range *range,
+ int *index)
+{
+ u64 restype = range->flags;
+ u64 cpu_addr = range->cpu_addr;
+ u64 cpu_end = range->cpu_addr + range->size;
+ u64 pci_addr = range->pci_addr;
+ u32 flags = LAM_64BIT | LAR_ENABLE;
+ u64 mask;
+ u64 size;
+ int idx = *index;
+
+ if (restype & IORESOURCE_PREFETCH)
+ flags |= LAM_PREFETCH;
+
+ /*
+ * If the size of the range is larger than the alignment of the start
+ * address, we have to use multiple entries to perform the mapping.
+ */
+ if (cpu_addr > 0) {
+ unsigned long nr_zeros = __ffs64(cpu_addr);
+ u64 alignment = 1ULL << nr_zeros;
Missing newline...
+ size = min(range->size, alignment);
+ } else {
+ size = range->size;
+ }
+ /* Hardware supports max 4GiB inbound region */
+ size = min(size, 1ULL << 32);
+
+ mask = roundup_pow_of_two(size) - 1;
+ mask &= ~0xf;
+
+ while (cpu_addr < cpu_end) {
+ /*
+ * Set up 64-bit inbound regions as the range parser doesn't
+ * distinguish between 32 and 64-bit types.
+ */
+ pci_write_reg(pcie, lower_32_bits(pci_addr), PCIEPRAR(idx));
+ pci_write_reg(pcie, lower_32_bits(cpu_addr), PCIELAR(idx));
+ pci_write_reg(pcie, lower_32_bits(mask) | flags, PCIELAMR(idx));
+
+ pci_write_reg(pcie, upper_32_bits(pci_addr), PCIEPRAR(idx+1));
+ pci_write_reg(pcie, upper_32_bits(cpu_addr), PCIELAR(idx+1));
+ pci_write_reg(pcie, 0, PCIELAMR(idx+1));
Missing spaces around '+'...
+
+ pci_addr += size;
+ cpu_addr += size;
+ idx += 2;
+
+ if (idx > MAX_NR_INBOUND_MAPS) {
+ dev_err(pcie->dev, "Failed to map inbound regions!\n");
+ return -EINVAL;
+ }
+ }
+ *index = idx;
+
+ 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)
+ return -ENOENT;
+
+ parser->end = parser->range + rlen / sizeof(__be32);
+ return 0;
+}
Erm, AFAIK "dma-ranges" is a standard property, shouldn't its parsing be
placed in some generic place like drivers/of/address.c?
[...]
+static int rcar_pcie_probe(struct platform_device *pdev)
+{
+ struct rcar_pcie *pcie;
+ unsigned int data;
+ struct of_pci_range range;
+ struct of_pci_range_parser parser;
+ const struct of_device_id *of_id;
+ int err, win = 0;
+ int (*hw_init_fn)(struct rcar_pcie *);
+
+ pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
+ if (!pcie)
+ return -ENOMEM;
+
+ pcie->dev = &pdev->dev;
+ platform_set_drvdata(pdev, pcie);
+
+ /* Get the bus range */
+ if (of_pci_parse_bus_range(pdev->dev.of_node, &pcie->busn)) {
+ dev_err(&pdev->dev, "failed to parse bus-range property\n");
+ return -EINVAL;
+ }
+
+ if (of_pci_range_parser_init(&parser, pdev->dev.of_node)) {
+ dev_err(&pdev->dev, "missing ranges property\n");
+ return -EINVAL;
+ }
+
+ err = rcar_pcie_get_resources(pdev, pcie);
+ if (err < 0) {
+ dev_err(&pdev->dev, "failed to request resources: %d\n", err);
+ return err;
+ }
+
+ for_each_of_pci_range(&parser, &range) {
+ of_pci_range_to_resource(&range, pdev->dev.of_node,
+ &pcie->res[win++]);
This function call is probably no good here as it fetches into the 'start'
field of a 'struct resource' a CPU address instead of a PCI address...
+
+ if (win > PCI_MAX_RESOURCES)
+ break;
+ }
+
+ err = rcar_pcie_parse_map_dma_ranges(pcie, pdev->dev.of_node);
+ if (err)
+ return err;
+
+ of_id = of_match_device(rcar_pcie_of_match, pcie->dev);
+ if (!of_id || !of_id->data)
+ return -EINVAL;
+ hw_init_fn = of_id->data;
+
+ /* Failure to get a link might just be that no cards are inserted */
+ err = hw_init_fn(pcie);
+ if (err) {
+ dev_info(&pdev->dev, "PCIe link down\n");
+ return 0;
Not quite sure why you exit normally here without enabling the hardware.
I think the internal bridge should be visible regardless of whether link is
detected or not...
+ }
+
+ data = pci_read_reg(pcie, MACSR);
+ dev_info(&pdev->dev, "PCIe x%d: link up\n", (data >> 20) & 0x3f);
+
+ rcar_pcie_enable(pcie);
+
+ return 0;
+}
[...]
WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
