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

From: Nicolas Saenz Julienne
Date: Fri Nov 08 2019 - 06:13:31 EST


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:
> > From: Jim Quinlan <james.quinlan@xxxxxxxxxxxx>
> >
> > 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 <james.quinlan@xxxxxxxxxxxx>
> > Co-developed-by: Nicolas Saenz Julienne <nsaenzjulienne@xxxxxxx>
> > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@xxxxxxx>
> > ---
> > 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.
> > + 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?

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

> > + default ARCH_BRCMSTB || BMIPS_GENERIC
> also this needs ARCH_BCM2835
> > + 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 :)

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

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

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

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

Attachment: signature.asc
Description: This is a digitally signed message part