On 07/08/15 08:42, Ley Foon Tan wrote:

I'd prefer to see the more common idiom:

	for (loop = 0; loop < TLP_LOOP; loop++) {

+               tlp_read_rx(pcie, &tlp_rp_regdata);
+               if (tlp_rp_regdata.ctrl & RP_RXCPL_EOP) {
+                       if (value)
+                               *value = tlp_rp_regdata.reg0;
+                       return PCIBIOS_SUCCESSFUL;
+               }

Don't you need some form of relaxation here? A delay of some sort?
Repeatedly polling the HW is usually not helping it to recover...

+       }
+
+       return -ENOENT;
+}
+
+static void tlp_write_packet(struct altera_pcie *pcie, u32 *headers, u32 data)
+{
+       struct tlp_rp_regpair_t tlp_rp_regdata;
+
+       tlp_rp_regdata.reg0 = headers[0];
+       tlp_rp_regdata.reg1 = headers[1];
+       tlp_rp_regdata.ctrl = RP_TX_SOP;
+       tlp_write_tx(pcie, &tlp_rp_regdata);
+
+       tlp_rp_regdata.reg0 = headers[2];
+       tlp_rp_regdata.reg1 = data;
+       tlp_rp_regdata.ctrl = RP_TX_EOP;
+       tlp_write_tx(pcie, &tlp_rp_regdata);
+}
+
+static int tlp_cfg_dword_read(struct altera_pcie *pcie, u8 bus, u32 devfn,
+                             int where, u32 *value)
+{
+       int ret;
+       u32 headers[TLP_HDR_SIZE];
+
+       if (bus == pcie->root_bus_nr)
+               headers[0] = TLP_CFG_DW0(TLP_FMTTYPE_CFGRD0);
+       else
+               headers[0] = TLP_CFG_DW0(TLP_FMTTYPE_CFGRD1);
+
+       headers[1] = TLP_CFG_DW1(TLP_REQ_ID(pcie->root_bus_nr, devfn),
+                                       TLP_READ_TAG);
+       headers[2] = TLP_CFG_DW2(bus, devfn, where);
+
+       tlp_write_packet(pcie, headers, 0);
+
+       ret = tlp_read_packet(pcie, value);
+       if (ret)
+               *value = ~0UL;  /* return 0xFFFFFFFF if error */
+
+       return ret;
+}
+
+static int tlp_cfg_dword_write(struct altera_pcie *pcie, u8 bus, u32 devfn,
+                              int where, u32 value)
+{
+       u32 headers[TLP_HDR_SIZE];
+
+       if (bus == pcie->root_bus_nr)
+               headers[0] = TLP_CFG_DW0(TLP_FMTTYPE_CFGWR0);
+       else
+               headers[0] = TLP_CFG_DW0(TLP_FMTTYPE_CFGWR1);
+
+       headers[1] = TLP_CFG_DW1(TLP_REQ_ID(pcie->root_bus_nr, devfn),
+                                       TLP_WRITE_TAG);
+       headers[2] = TLP_CFG_DW2(bus, devfn, where);
+
+       tlp_write_packet(pcie, headers, value);
+
+       tlp_read_packet(pcie, NULL);
+
+       /* Keep an eye out for changes to the root bus number */
+       if ((bus == pcie->root_bus_nr) && (where == PCI_PRIMARY_BUS))
+               pcie->root_bus_nr = (u8)(value);
+
+       return PCIBIOS_SUCCESSFUL;
+}
+
+static int altera_pcie_cfg_read(struct pci_bus *bus, unsigned int devfn,
+                               int where, int size, u32 *value)
+{
+       struct altera_pcie *pcie = bus->sysdata;
+       int ret;
+
+       if (!altera_pcie_valid_config(pcie, bus, PCI_SLOT(devfn))) {
+               *value = ~0UL;
+               return PCIBIOS_DEVICE_NOT_FOUND;
+       }
+
+       ret = tlp_cfg_dword_read(pcie, bus->number, devfn,
+                                (where & ~DWORD_MASK), value);
+       if (ret != PCIBIOS_SUCCESSFUL)
+               return ret;
+
+       if (size == 1)
+               *value = (*value >> (8 * (where & 0x3))) & 0xff;
+       else if (size == 2)
+               *value = (*value >> (8 * (where & 0x2))) & 0xffff;
+
+       return PCIBIOS_SUCCESSFUL;
+}
+
+static int altera_pcie_cfg_write(struct pci_bus *bus, unsigned int devfn,
+                                int where, int size, u32 value)
+{
+       struct altera_pcie *pcie = bus->sysdata;
+       u32 data32 = value;
+       u32 shift = 8 * (where & 3);
+       int ret;
+
+       if (!altera_pcie_valid_config(pcie, bus, PCI_SLOT(devfn)))
+               return PCIBIOS_DEVICE_NOT_FOUND;
+
+       /* write partial */
+       if (size != sizeof(u32)) {
+               ret = tlp_cfg_dword_read(pcie, bus->number, devfn,
+                                        where & ~DWORD_MASK, &data32);
+               if (ret)
+                       return ret;
+
+               if (size == 2)
+                       data32 = (data32 & ~(0xffff << shift)) |
+                                       ((value & 0xffff) << shift);
+                else if (size == 1)
+                       data32 = (data32 & ~(0xff << shift)) |
+                                       ((value & 0xff) << shift);
+       }
+
+       return tlp_cfg_dword_write(pcie, bus->number, devfn,
+               (where & ~DWORD_MASK), data32);
+}
+
+static struct pci_ops altera_pcie_ops = {
+       .read = altera_pcie_cfg_read,
+       .write = altera_pcie_cfg_write,
+};
+
+static int altera_pcie_intx_map(struct irq_domain *domain, unsigned int irq,
+                               irq_hw_number_t hwirq)
+{
+       irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq);
+       irq_set_chip_data(irq, domain->host_data);
+       set_irq_flags(irq, IRQF_VALID);
+
+       return 0;
+}
+
+static const struct irq_domain_ops intx_domain_ops = {
+       .map = altera_pcie_intx_map,
+};
+
+static void altera_pcie_isr(unsigned int irq, struct irq_desc *desc)
+{
+       struct irq_chip *chip = irq_desc_get_chip(desc);
+       struct altera_pcie *pcie;
+       unsigned long status;
+       u32 bit;
+       u32 virq;
+
+       chained_irq_enter(chip, desc);
+       pcie = irq_desc_get_handler_data(desc);
+
+       do {
+               status = cra_readl(pcie, P2A_INT_STATUS) & P2A_INT_STS_ALL;
+               if (!status)
+                       break;
+
+               do {
+                       bit = find_first_bit(&status, INTX_NUM);
+                       /* clear interrupts */
+                       cra_writel(pcie, 1 << bit, P2A_INT_STATUS);
+
+                       virq = irq_find_mapping(pcie->irq_domain, bit + 1);
+                       if (virq)
+                               generic_handle_irq(virq);
+                       else
+                               dev_info(&pcie->pdev->dev, "unexpected IRQ\n");
+
+                       /* Clear the bit from status and repeat without reading
+                        * again status register. */
+                       __clear_bit(bit, &status);
+               } while (status);
+       } while (1);
+
+       chained_irq_exit(chip, desc);
+}
+
+static void altera_pcie_release_of_pci_ranges(struct altera_pcie *pcie)
+{
+       pci_free_resource_list(&pcie->resources);
+}
+
+static int altera_pcie_parse_request_of_pci_ranges(struct altera_pcie *pcie)
+{
+       int err, res_valid = 0;
+       struct device *dev = &pcie->pdev->dev;
+       struct device_node *np = dev->of_node;
+       resource_size_t iobase;
+       struct resource_entry *win;
+
+       err = of_pci_get_host_bridge_resources(np, 0, 0xff, &pcie->resources,
+                                              &iobase);
+       if (err)
+               return err;
+
+       resource_list_for_each_entry(win, &pcie->resources) {
+               struct resource *parent, *res = win->res;
+
+               switch (resource_type(res)) {
+               case IORESOURCE_MEM:
+                       parent = &iomem_resource;
+                       res_valid |= !(res->flags & IORESOURCE_PREFETCH);
+                       break;
+               default:
+                       continue;
+               }
+
+               err = devm_request_resource(dev, parent, res);
+               if (err)
+                       goto out_release_res;
+       }
+
+       if (!res_valid) {
+               dev_err(dev, "non-prefetchable memory resource required\n");
+               err = -EINVAL;
+               goto out_release_res;
+       }
+
+       return 0;
+
+out_release_res:
+       altera_pcie_release_of_pci_ranges(pcie);
+       return err;
+}
+
+static void altera_pcie_free_irq_domain(struct altera_pcie *pcie)
+{
+       int i;
+       u32 irq;
+
+       for (i = 0; i < INTX_NUM; i++) {
+               irq = irq_find_mapping(pcie->irq_domain, i);
+               if (irq > 0)
+                       irq_dispose_mapping(irq);
+       }
+
+       irq_domain_remove(pcie->irq_domain);
+}
+
+static int altera_pcie_init_irq_domain(struct altera_pcie *pcie)
+{
+       struct device *dev = &pcie->pdev->dev;
+       struct device_node *node = dev->of_node;
+
+       /* Setup INTx */
+       pcie->irq_domain = irq_domain_add_linear(node, INTX_NUM,
+                                       &intx_domain_ops, pcie);
+       if (!pcie->irq_domain) {
+               dev_err(dev, "Failed to get a INTx IRQ domain\n");
+               return PTR_ERR(pcie->irq_domain);

Err, no. irq_domain_add returns NULL on error, so PTR_ERR(NULL) is still
going to be zero.

+       }
+
+       return 0;
+}
+
+static int altera_pcie_parse_dt(struct altera_pcie *pcie)
+{
+       struct resource *cra;
+       struct platform_device *pdev = pcie->pdev;
+
+       cra = platform_get_resource_byname(pdev, IORESOURCE_MEM, "Cra");

Probably worth checking that this hasn't failed.

+       pcie->cra_base = devm_ioremap_resource(&pdev->dev, cra);
+       if (IS_ERR(pcie->cra_base)) {
+               dev_err(&pdev->dev, "get Cra resource failed\n");
+               return PTR_ERR(pcie->cra_base);
+       }
+
+       /* setup IRQ */
+       pcie->irq = platform_get_irq(pdev, 0);
+       if (pcie->irq <= 0) {
+               dev_err(&pdev->dev, "failed to get IRQ: %d\n", pcie->irq);
+               return -EINVAL;
+       }
+
+       irq_set_chained_handler_and_data(pcie->irq, altera_pcie_isr, pcie);
+
+       return 0;
+}
+
+static int altera_pcie_probe(struct platform_device *pdev)
+{
+       struct altera_pcie *pcie;
+       struct pci_bus *bus;
+       int ret;
+
+       pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
+       if (!pcie)
+               return -ENOMEM;
+
+       pcie->pdev = pdev;
+
+       ret = altera_pcie_parse_dt(pcie);
+       if (ret) {
+               dev_err(&pdev->dev, "Parsing DT failed\n");
+               return ret;
+       }
+
+       INIT_LIST_HEAD(&pcie->resources);
+
+       ret = altera_pcie_parse_request_of_pci_ranges(pcie);
+       if (ret) {
+               dev_err(&pdev->dev, "Failed add resources\n");
+               return ret;
+       }
+
+       ret = altera_pcie_init_irq_domain(pcie);
+       if (ret) {
+               dev_err(&pdev->dev, "Failed creating IRQ Domain\n");
+               return ret;
+       }
+
+       pcie->root_bus_nr = 0;
+
+       /* clear all interrupts */
+       cra_writel(pcie, P2A_INT_STS_ALL, P2A_INT_STATUS);
+       /* enable all interrupts */
+       cra_writel(pcie, P2A_INT_ENA_ALL, P2A_INT_ENABLE);
+
+       bus = pci_scan_root_bus(&pdev->dev, pcie->root_bus_nr, &altera_pcie_ops,
+                               pcie, &pcie->resources);
+       if (!bus)
+               return -ENOMEM;
+
+       pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
+       pci_assign_unassigned_bus_resources(bus);
+       pci_bus_add_devices(bus);
+
+       platform_set_drvdata(pdev, pcie);
+       return ret;
+}
+
+static int __exit altera_pcie_remove(struct platform_device *pdev)
+{
+       struct altera_pcie *pcie = platform_get_drvdata(pdev);
+
+       altera_pcie_free_irq_domain(pcie);
+       platform_set_drvdata(pdev, NULL);
+       return 0;
+}
+
+static const struct of_device_id altera_pcie_of_match[] = {
+       { .compatible = "altr,pcie-root-port-1.0", },
+       {},
+};
+MODULE_DEVICE_TABLE(of, altera_pcie_of_match);
+
+static struct platform_driver altera_pcie_driver = {
+       .probe          = altera_pcie_probe,
+       .remove         = altera_pcie_remove,
+       .driver = {
+               .name   = "altera-pcie",
+               .of_match_table = altera_pcie_of_match,
+       },
+};
+
+module_platform_driver(altera_pcie_driver);
+
+MODULE_AUTHOR("Ley Foon Tan [off-list ref]");
+MODULE_DESCRIPTION("Altera PCIe host controller driver");
+MODULE_LICENSE("GPL v2");
--
1.8.2.1

This is starting to look better. You should get someone to review the DT
parts though.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...