[PATCH 3/4] PCI: brcmstb: add Broadcom STB PCIe host controller driver

Hi Nicolas,

Am 06.11.19 um 22:45 schrieb Nicolas Saenz Julienne:

looking at the driver suggests me a tristate instead of bool.

+	depends on ARCH_BRCMSTB || BMIPS_GENERIC

please add ARCH_BCM2835 for the Raspberry Pi 4

+	depends on OF
+	depends on SOC_BRCMSTB

Why is this needed?

+	default ARCH_BRCMSTB || BMIPS_GENERIC

also this needs ARCH_BCM2835

36 msec?

+	l23 = RD_FLD(base, PCIE_MISC_PCIE_STATUS, PCIE_LINK_IN_L23);
+	for (i = 0; i < 15 && !l23; i++) {
+		usleep_range(2000, 2400);
+		l23 = RD_FLD(base, PCIE_MISC_PCIE_STATUS, PCIE_LINK_IN_L23);
+	}
+
+	if (!l23)
+		dev_err(pcie->dev, "failed to enter L23\n");

I think most user don't know anything about L23.

How about:

failed to enter low-power link state

+}
+
+static void turn_off(struct brcm_pcie *pcie)
+{
+	void __iomem *base = pcie->base;
+
+	if (brcm_pcie_link_up(pcie))
+		enter_l23(pcie);
+	/* Assert fundamental reset */
+	brcm_pcie_perst_set(pcie, 1);
+	/* Deassert request for L23 in case it was asserted */
+	WR_FLD_RB(base, PCIE_MISC_PCIE_CTRL, PCIE_L23_REQUEST, 0);
+	/* Turn off SerDes */
+	WR_FLD_RB(base, PCIE_MISC_HARD_PCIE_HARD_DEBUG, SERDES_IDDQ, 1);
+	/* Shutdown PCIe bridge */
+	brcm_pcie_bridge_sw_init_set(pcie, 1);
+}
+
+static int brcm_pcie_suspend(struct device *dev)
+{
+	struct brcm_pcie *pcie = dev_get_drvdata(dev);
+
+	turn_off(pcie);
+	clk_disable_unprepare(pcie->clk);
+	pcie->suspended = true;
+
+	return 0;
+}
+
+static int brcm_pcie_resume(struct device *dev)
+{
+	struct brcm_pcie *pcie = dev_get_drvdata(dev);
+	void __iomem *base;
+	int ret;
+
+	base = pcie->base;
+	clk_prepare_enable(pcie->clk);
+
+	/* Take bridge out of reset so we can access the SerDes reg */
+	brcm_pcie_bridge_sw_init_set(pcie, 0);
+
+	/* Turn on SerDes */
+	WR_FLD_RB(base, PCIE_MISC_HARD_PCIE_HARD_DEBUG, SERDES_IDDQ, 0);
+	/* Wait for SerDes to be stable */
+	usleep_range(100, 200);
+
+	ret = brcm_pcie_setup(pcie);
+	if (ret)
+		return ret;
+
+	pcie->suspended = false;
+
+	return 0;
+}
+
+static void _brcm_pcie_remove(struct brcm_pcie *pcie)
+{
+	turn_off(pcie);
+	clk_disable_unprepare(pcie->clk);
+	clk_put(pcie->clk);
+}
+
+static int brcm_pcie_remove(struct platform_device *pdev)
+{
+	struct brcm_pcie *pcie = platform_get_drvdata(pdev);
+
+	pci_stop_root_bus(pcie->root_bus);
+	pci_remove_root_bus(pcie->root_bus);
+	_brcm_pcie_remove(pcie);
+
+	return 0;
+}
+
+static const struct of_device_id brcm_pcie_match[] = {
+	{ .compatible = "brcm,bcm2711-pcie", .data = &bcm2711_cfg },
+	{},
+};
+MODULE_DEVICE_TABLE(of, brcm_pcie_match);
+
+static int brcm_pcie_probe(struct platform_device *pdev)
+{
+	struct device_node *dn = pdev->dev.of_node;
+	const struct of_device_id *of_id;
+	const struct pcie_cfg_data *data;
+	struct resource *res;
+	int ret;
+	struct brcm_pcie *pcie;
+	void __iomem *base;
+	struct pci_host_bridge *bridge;
+	struct pci_bus *child;
+
+	bridge = devm_pci_alloc_host_bridge(&pdev->dev, sizeof(*pcie));
+	if (!bridge)
+		return -ENOMEM;
+
+	pcie = pci_host_bridge_priv(bridge);
+
+	of_id = of_match_node(brcm_pcie_match, dn);
+	if (!of_id) {
+		dev_err(&pdev->dev, "failed to look up compatible string\n");
+		return -EINVAL;
+	}
+
+	data = of_id->data;
+	pcie->reg_offsets = data->offsets;
+	pcie->reg_field_info = data->reg_field_info;
+	pcie->type = data->type;
+	pcie->dn = dn;
+	pcie->dev = &pdev->dev;
+
+	/* We use the domain number as our controller number */
+	pcie->id = of_get_pci_domain_nr(dn);
+	if (pcie->id < 0)
+		return pcie->id;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -EINVAL;
+
+	base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	pcie->clk = of_clk_get_by_name(dn, "sw_pcie");
+	if (IS_ERR(pcie->clk)) {

we should handle EPROBE_DEFER here

+		dev_err(&pdev->dev, "could not get clock\n");
+		pcie->clk = NULL;
+	}
+	pcie->base = base;
+
+	ret = of_pci_get_max_link_speed(dn);
+	pcie->gen = (ret < 0) ? 0 : ret;
+
+	pcie->ssc = of_property_read_bool(dn, "brcm,enable-ssc");
+
+	ret = irq_of_parse_and_map(pdev->dev.of_node, 0);
+	if (ret == 0)
+		/* keep going, as we don't use this intr yet */
+		dev_warn(pcie->dev, "cannot get PCIe interrupt\n");
+	else
+		pcie->irq = ret;
+
+	ret = pci_parse_request_of_pci_ranges(pcie->dev, &bridge->windows,
+					      &bridge->dma_ranges, NULL);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(pcie->clk);
+	if (ret) {
+		dev_err(&pdev->dev, "could not enable clock\n");
+		return ret;
+	}
+
+	ret = brcm_pcie_setup(pcie);
+	if (ret)
+		goto fail;
+
+	bridge->dev.parent = &pdev->dev;
+	bridge->busnr = 0;
+	bridge->ops = &brcm_pcie_ops;
+	bridge->sysdata = pcie;
+	bridge->map_irq = of_irq_parse_and_map_pci;
+	bridge->swizzle_irq = pci_common_swizzle;
+
+	ret = pci_scan_root_bus_bridge(bridge);
+	if (ret < 0) {
+		dev_err(pcie->dev, "Scanning root bridge failed\n");
+		goto fail;
+	}
+
+	pci_assign_unassigned_bus_resources(bridge->bus);
+	list_for_each_entry(child, &bridge->bus->children, node)
+		pcie_bus_configure_settings(child);
+	pci_bus_add_devices(bridge->bus);
+	platform_set_drvdata(pdev, pcie);
+	pcie->root_bus = bridge->bus;
+
+	return 0;
+
+fail:
+	_brcm_pcie_remove(pcie);
+	return ret;
+}
+
+static const struct dev_pm_ops brcm_pcie_pm_ops = {
+	.suspend_noirq = brcm_pcie_suspend,
+	.resume_noirq = brcm_pcie_resume,
+};
+
+static struct platform_driver brcm_pcie_driver = {
+	.probe = brcm_pcie_probe,
+	.remove = brcm_pcie_remove,
+	.driver = {
+		.name = "brcm-pcie",
+		.owner = THIS_MODULE,

This is already done by module_platform_driver

+		.of_match_table = brcm_pcie_match,
+		.pm = &brcm_pcie_pm_ops,
+	},
+};
+
+module_platform_driver(brcm_pcie_driver);
+
+MODULE_LICENSE("GPL v2");

This is a mismatch to the SPDX (GPL 2 and higher), because this says GPL
v2 only

Thanks
Stefan

+MODULE_DESCRIPTION("Broadcom STB PCIe RC driver");
+MODULE_AUTHOR("Broadcom");

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel