Hi Stefan,
thanks for the review!

On Thu, 2019-11-07 at 18:50 +0100, Stefan Wahren wrote:

Hi Nicolas,

Am 06.11.19 um 22:45 schrieb Nicolas Saenz Julienne:

quoted

From: Jim Quinlan <redacted>

This commit adds the basic Broadcom STB PCIe controller.  Missing is the
ability to process MSI. This functionality is added in a subsequent
commit.

The PCIe block contains an MDIO interface.  This is a local interface
only accessible by the PCIe controller.  It cannot be used or shared
by any other HW.  As such, the small amount of code for this
controller is included in this driver as there is little upside to put
it elsewhere.

This is based on Jim's original submission[1] but adapted and tailored
specifically to bcm2711's needs (that's the Raspberry Pi 4). Support for
the rest of the brcmstb family will soon follow once we get support for
multiple dma-ranges in dma/direct.

[1] https://patchwork.kernel.org/patch/10605959/

Signed-off-by: Jim Quinlan <redacted>
Co-developed-by: Nicolas Saenz Julienne <redacted>
Signed-off-by: Nicolas Saenz Julienne <redacted>
---
 drivers/pci/controller/Kconfig        |  12 +
 drivers/pci/controller/Makefile       |   1 +
 drivers/pci/controller/pcie-brcmstb.c | 973 ++++++++++++++++++++++++++
 3 files changed, 986 insertions(+)
 create mode 100644 drivers/pci/controller/pcie-brcmstb.c

diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index f5de9119e8d3..8b3aae91d8af 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig

@@ -281,6 +281,18 @@ config VMD
 	  To compile this driver as a module, choose M here: the
 	  module will be called vmd.

+config PCIE_BRCMSTB
+	bool "Broadcom Brcmstb PCIe host controller"

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

quoted

+	depends on ARCH_BRCMSTB || BMIPS_GENERIC

please add ARCH_BCM2835 for the Raspberry Pi 4

quoted

+	depends on OF
+	depends on SOC_BRCMSTB

Why is this needed?

It's not, I missed it. For now, I edited it so it only depends on ARCH_BCM2835.

quoted

+	default ARCH_BRCMSTB || BMIPS_GENERIC

also this needs ARCH_BCM2835

quoted

+	help
+	  Say Y here to enable PCIe host controller support for
+	  Broadcom Settop Box SOCs.  A Broadcom SOC will may have
+	  multiple host controllers as opposed to a single host
+	  controller with multiple ports.
+
 config PCI_HYPERV_INTERFACE
 	tristate "Hyper-V PCI Interface"
 	depends on X86 && HYPERV && PCI_MSI && PCI_MSI_IRQ_DOMAIN && X86_64

diff --git a/drivers/pci/controller/Makefile

b/drivers/pci/controller/Makefile
index a2a22c9d91af..3fc0b0cf5b5b 100644

--- a/drivers/pci/controller/Makefile
+++ b/drivers/pci/controller/Makefile

@@ -30,6 +30,7 @@ obj-$(CONFIG_PCIE_MEDIATEK) += pcie-mediatek.o
 obj-$(CONFIG_PCIE_MOBIVEIL) += pcie-mobiveil.o
 obj-$(CONFIG_PCIE_TANGO_SMP8759) += pcie-tango.o
 obj-$(CONFIG_VMD) += vmd.o
+obj-$(CONFIG_PCIE_BRCMSTB) += pcie-brcmstb.o
 # pcie-hisi.o quirks are needed even without CONFIG_PCIE_DW
 obj-y				+= dwc/

diff --git a/drivers/pci/controller/pcie-brcmstb.c

b/drivers/pci/controller/pcie-brcmstb.c
new file mode 100644
index 000000000000..880ec11d06a1

--- /dev/null
+++ b/drivers/pci/controller/pcie-brcmstb.c

@@ -0,0 +1,973 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2009 - 2019 Broadcom */
+
+#include <linux/clk.h>
+#include <linux/compiler.h>
+#include <linux/delay.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/irqdomain.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/log2.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_pci.h>
+#include <linux/of_platform.h>
+#include <linux/pci.h>
+#include <linux/printk.h>
+#include <linux/sizes.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/types.h>
+
+#include "../pci.h"
+

...

+
+/* L23 is a low-power PCIe link state */
+static void enter_l23(struct brcm_pcie *pcie)
+{
+	void __iomem *base = pcie->base;
+	int l23, i;
+
+	/* assert request for L23 */
+	WR_FLD_RB(base, PCIE_MISC_PCIE_CTRL, PCIE_L23_REQUEST, 1);
+
+	/* Wait up to 30 msec for L23 */

36 msec?

Yes :)

quoted

+	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

Noted

quoted

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

Yes, I'll keep it in mind.

quoted

+		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

Noted

quoted

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

Noted

Regards,
Nicolas