Re: [PATCH v3 2/5] pci:host: Add Altera PCIe host controller driver

[Ley Foon Tan]

On Fri, Aug 7, 2015 at 10:34 PM, Marc Zyngier <marc.zyngier@xxxxxxx> wrote:
> On 07/08/15 08:42, Ley Foon Tan wrote:
>> This patch adds the Altera PCIe host controller driver.
>>
>> Signed-off-by: Ley Foon Tan <lftan@xxxxxxxxxx>
>> ---
>>  drivers/pci/host/Kconfig       |   7 +
>>  drivers/pci/host/Makefile      |   1 +
>>  drivers/pci/host/pcie-altera.c | 532 +++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 540 insertions(+)
>>  create mode 100644 drivers/pci/host/pcie-altera.c

<snip>

>> +static int tlp_read_packet(struct altera_pcie *pcie, u32 *value)
>> +{
>> +       u8 loop;
>> +       struct tlp_rp_regpair_t tlp_rp_regdata;
>> +
>> +       for (loop = TLP_LOOP; loop > 0; loop--) {
>
> I'd prefer to see the more common idiom:
>
>         for (loop = 0; loop < TLP_LOOP; loop++) {
Okay, will change this.

>> +               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...
Yes, will add udelay here.

<snip>

>> +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.

Change to return -ENOMEM.

>
>> +       }
>> +
>> +       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.
Noted. Will add error checking.
>
>> +       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 <lftan@xxxxxxxxxx>");
>> +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.
I have CC-ed DT maintainers. Hope someone can help to review.


Thanks for reviewing.

Regards
Ley Foon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/