| Message ID | d1362766e3e966f78591129de918046a4b892c18.1738963156.git.andrea.porta@suse.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for RaspberryPi RP1 PCI device using a DT overlay | expand |
Hi Andrea, Am 07.02.25 um 22:31 schrieb Andrea della Porta: > The RaspberryPi RP1 is a PCI multi function device containing > peripherals ranging from Ethernet to USB controller, I2C, SPI > and others. > > Implement a bare minimum driver to operate the RP1, leveraging > actual OF based driver implementations for the on-board peripherals > by loading a devicetree overlay during driver probe. > > The peripherals are accessed by mapping MMIO registers starting > from PCI BAR1 region. > > With the overlay approach we can achieve more generic and agnostic > approach to managing this chipset, being that it is a PCI endpoint > and could possibly be reused in other hw implementations. The > presented approach is also used by Bootlin's Microchip LAN966x > patchset (see link) as well, for a similar chipset. > > The reason why this driver is contained in drivers/misc has > been paved by Bootlin's LAN966X driver, which first used the > overlay approach to implement non discoverable peripherals behind a > PCI bus. For RP1, the same arguments apply: it's not used as an SoC > since the driver code is not running on-chip and is not like an MFD > since it does not really need all the MFD infrastructure (shared regs, > etc.). So, for this particular use, misc has been proposed and deemed > as a good choice. For further details about that please check the links. > > This driver is heavily based on downstream code from RaspberryPi > Foundation, and the original author is Phil Elwell. > > Link:https://datasheets.raspberrypi.com/rp1/rp1-peripherals.pdf > Link:https://lore.kernel.org/all/20240612140208.GC1504919@google.com/ > Link:https://lore.kernel.org/all/83f7fa09-d0e6-4f36-a27d-cee08979be2a@app.fastmail.com/ > Link:https://lore.kernel.org/all/2024081356-mutable-everyday-6f9d@gregkh/ > Link:https://lore.kernel.org/all/20240808154658.247873-1-herve.codina@bootlin.com/ > > Signed-off-by: Andrea della Porta<andrea.porta@suse.com> > --- > MAINTAINERS | 1 + > drivers/misc/Kconfig | 1 + > drivers/misc/Makefile | 1 + > drivers/misc/rp1/Kconfig | 20 +++ > drivers/misc/rp1/Makefile | 3 + > drivers/misc/rp1/rp1-pci.dtso | 8 + > drivers/misc/rp1/rp1_pci.c | 305 ++++++++++++++++++++++++++++++++++ > drivers/pci/quirks.c | 1 + > include/linux/pci_ids.h | 3 + > 9 files changed, 343 insertions(+) > create mode 100644 drivers/misc/rp1/Kconfig > create mode 100644 drivers/misc/rp1/Makefile > create mode 100644 drivers/misc/rp1/rp1-pci.dtso > create mode 100644 drivers/misc/rp1/rp1_pci.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 4cb38064694e..54f9e09f02ed 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -19757,6 +19757,7 @@ F: Documentation/devicetree/bindings/misc/pci1de4,1.yaml > F: Documentation/devicetree/bindings/pci/pci-ep-bus.yaml > F: Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml > F: drivers/clk/clk-rp1.c > +F: drivers/misc/rp1/ > F: drivers/pinctrl/pinctrl-rp1.c > F: include/dt-bindings/clock/rp1.h > F: include/dt-bindings/misc/rp1.h > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig > index 56bc72c7ce4a..af8c3be967bf 100644 > --- a/drivers/misc/Kconfig > +++ b/drivers/misc/Kconfig > @@ -649,4 +649,5 @@ source "drivers/misc/uacce/Kconfig" > source "drivers/misc/pvpanic/Kconfig" > source "drivers/misc/mchp_pci1xxxx/Kconfig" > source "drivers/misc/keba/Kconfig" > +source "drivers/misc/rp1/Kconfig" > endmenu > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile > index 545aad06d088..5df79dd90c9c 100644 > --- a/drivers/misc/Makefile > +++ b/drivers/misc/Makefile > @@ -75,3 +75,4 @@ lan966x-pci-objs := lan966x_pci.o > lan966x-pci-objs += lan966x_pci.dtbo.o > obj-$(CONFIG_MCHP_LAN966X_PCI) += lan966x-pci.o > obj-y += keba/ > +obj-$(CONFIG_MISC_RP1) += rp1/ > diff --git a/drivers/misc/rp1/Kconfig b/drivers/misc/rp1/Kconfig > new file mode 100644 > index 000000000000..cfb02b2f186c > --- /dev/null > +++ b/drivers/misc/rp1/Kconfig > @@ -0,0 +1,20 @@ > +# SPDX-License-Identifier: GPL-2.0-only > +# > +# RaspberryPi RP1 misc device > +# > + > +config MISC_RP1 > + tristate "RaspberryPi RP1 PCIe support" > + depends on OF_IRQ && OF_OVERLAY && PCI_MSI && PCI_QUIRKS > + select PCI_DYNAMIC_OF_NODES > + help > + Support the RP1 peripheral chip found on Raspberry Pi 5 board. > + > + This device supports several sub-devices including e.g. Ethernet > + controller, USB controller, I2C, SPI and UART. > + > + The driver is responsible for enabling the DT node once the PCIe > + endpoint has been configured, and handling interrupts. > + > + This driver uses an overlay to load other drivers to support for > + RP1 internal sub-devices. > diff --git a/drivers/misc/rp1/Makefile b/drivers/misc/rp1/Makefile > new file mode 100644 > index 000000000000..508b4cb05627 > --- /dev/null > +++ b/drivers/misc/rp1/Makefile > @@ -0,0 +1,3 @@ > +# SPDX-License-Identifier: GPL-2.0-only > +obj-$(CONFIG_MISC_RP1) += rp1-pci.o > +rp1-pci-objs := rp1_pci.o rp1-pci.dtbo.o > diff --git a/drivers/misc/rp1/rp1-pci.dtso b/drivers/misc/rp1/rp1-pci.dtso > new file mode 100644 > index 000000000000..0bf2f4bb18e6 > --- /dev/null > +++ b/drivers/misc/rp1/rp1-pci.dtso > @@ -0,0 +1,8 @@ > +// SPDX-License-Identifier: (GPL-2.0 OR MIT) > + > +/* the dts overlay is included from the dts directory so > + * it can be possible to check it with CHECK_DTBS while > + * also compile it from the driver source directory. > + */ > + > +#include "arm64/broadcom/rp1.dtso" > diff --git a/drivers/misc/rp1/rp1_pci.c b/drivers/misc/rp1/rp1_pci.c > new file mode 100644 > index 000000000000..c2d76e01bf57 > --- /dev/null > +++ b/drivers/misc/rp1/rp1_pci.c > @@ -0,0 +1,305 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2018-24 Raspberry Pi Ltd. > + * All rights reserved. > + */ > + > +#include <linux/err.h> > +#include <linux/interrupt.h> > +#include <linux/irq.h> > +#include <linux/irqchip/chained_irq.h> > +#include <linux/irqdomain.h> > +#include <linux/module.h> > +#include <linux/msi.h> > +#include <linux/of_platform.h> > +#include <linux/pci.h> > +#include <linux/platform_device.h> > + > +#define RP1_HW_IRQ_MASK GENMASK(5, 0) > + > +#define REG_SET 0x800 > +#define REG_CLR 0xc00 > + > +/* MSI-X CFG registers start at 0x8 */ > +#define MSIX_CFG(x) (0x8 + (4 * (x))) > + > +#define MSIX_CFG_IACK_EN BIT(3) > +#define MSIX_CFG_IACK BIT(2) > +#define MSIX_CFG_ENABLE BIT(0) > + > +/* Address map */ > +#define RP1_PCIE_APBS_BASE 0x108000 > + > +/* Interrupts */ > +#define RP1_INT_END 61 > + > +/* Embedded dtbo symbols created by cmd_wrap_S_dtb in scripts/Makefile.lib */ > +extern char __dtbo_rp1_pci_begin[]; > +extern char __dtbo_rp1_pci_end[]; > + > +struct rp1_dev { > + struct pci_dev *pdev; > + struct irq_domain *domain; > + struct irq_data *pcie_irqds[64]; > + void __iomem *bar1; > + int ovcs_id; /* overlay changeset id */ > + bool level_triggered_irq[RP1_INT_END]; > +}; > + > +static void msix_cfg_set(struct rp1_dev *rp1, unsigned int hwirq, u32 value) > +{ > + iowrite32(value, rp1->bar1 + RP1_PCIE_APBS_BASE + REG_SET + MSIX_CFG(hwirq)); > +} > + > +static void msix_cfg_clr(struct rp1_dev *rp1, unsigned int hwirq, u32 value) > +{ > + iowrite32(value, rp1->bar1 + RP1_PCIE_APBS_BASE + REG_CLR + MSIX_CFG(hwirq)); > +} > + > +static void rp1_mask_irq(struct irq_data *irqd) > +{ > + struct rp1_dev *rp1 = irqd->domain->host_data; > + struct irq_data *pcie_irqd = rp1->pcie_irqds[irqd->hwirq]; > + > + pci_msi_mask_irq(pcie_irqd); > +} > + > +static void rp1_unmask_irq(struct irq_data *irqd) > +{ > + struct rp1_dev *rp1 = irqd->domain->host_data; > + struct irq_data *pcie_irqd = rp1->pcie_irqds[irqd->hwirq]; > + > + pci_msi_unmask_irq(pcie_irqd); > +} > + > +static int rp1_irq_set_type(struct irq_data *irqd, unsigned int type) > +{ > + struct rp1_dev *rp1 = irqd->domain->host_data; > + unsigned int hwirq = (unsigned int)irqd->hwirq; > + > + switch (type) { > + case IRQ_TYPE_LEVEL_HIGH: > + dev_dbg(&rp1->pdev->dev, "MSIX IACK EN for irq %d\n", hwirq); This looks a little bit inconsistent. Only this type has a debug message. So either we drop this or add at least a message for IRQ_TYPE_EDGE_RISING, too. Btw the format specifier looks wrong (unsigned int vs %d). > + msix_cfg_set(rp1, hwirq, MSIX_CFG_IACK_EN); > + rp1->level_triggered_irq[hwirq] = true; > + break; > + case IRQ_TYPE_EDGE_RISING: > + msix_cfg_clr(rp1, hwirq, MSIX_CFG_IACK_EN); > + rp1->level_triggered_irq[hwirq] = false; > + break; > + default: > + return -EINVAL; It would be nice to document why only IRQ_TYPE_LEVEL_HIGH and IRQ_TYPE_EDGE_RISING are supported. In case it's a software limitation, this function would be a good place. In case this is a hardware limitation this should be in the binding. > + } > + > + return 0; > +} > + > +static struct irq_chip rp1_irq_chip = { > + .name = "rp1_irq_chip", > + .irq_mask = rp1_mask_irq, > + .irq_unmask = rp1_unmask_irq, > + .irq_set_type = rp1_irq_set_type, > +}; > + > +static void rp1_chained_handle_irq(struct irq_desc *desc) > +{ > + unsigned int hwirq = desc->irq_data.hwirq & RP1_HW_IRQ_MASK; > + struct rp1_dev *rp1 = irq_desc_get_handler_data(desc); > + struct irq_chip *chip = irq_desc_get_chip(desc); > + unsigned int virq; > + > + chained_irq_enter(chip, desc); > + > + virq = irq_find_mapping(rp1->domain, hwirq); > + generic_handle_irq(virq); > + if (rp1->level_triggered_irq[hwirq]) > + msix_cfg_set(rp1, hwirq, MSIX_CFG_IACK); > + > + chained_irq_exit(chip, desc); > +} > + > +static int rp1_irq_xlate(struct irq_domain *d, struct device_node *node, > + const u32 *intspec, unsigned int intsize, > + unsigned long *out_hwirq, unsigned int *out_type) > +{ > + struct rp1_dev *rp1 = d->host_data; > + struct irq_data *pcie_irqd; > + unsigned long hwirq; > + int pcie_irq; > + int ret; > + > + ret = irq_domain_xlate_twocell(d, node, intspec, intsize, > + &hwirq, out_type); > + if (ret) > + return ret; > + > + pcie_irq = pci_irq_vector(rp1->pdev, hwirq); > + pcie_irqd = irq_get_irq_data(pcie_irq); > + rp1->pcie_irqds[hwirq] = pcie_irqd; > + *out_hwirq = hwirq; > + > + return 0; > +} > + > +static int rp1_irq_activate(struct irq_domain *d, struct irq_data *irqd, > + bool reserve) > +{ > + struct rp1_dev *rp1 = d->host_data; > + > + msix_cfg_set(rp1, (unsigned int)irqd->hwirq, MSIX_CFG_ENABLE); > + > + return 0; > +} > + > +static void rp1_irq_deactivate(struct irq_domain *d, struct irq_data *irqd) > +{ > + struct rp1_dev *rp1 = d->host_data; > + > + msix_cfg_clr(rp1, (unsigned int)irqd->hwirq, MSIX_CFG_ENABLE); > +} > + > +static const struct irq_domain_ops rp1_domain_ops = { > + .xlate = rp1_irq_xlate, > + .activate = rp1_irq_activate, > + .deactivate = rp1_irq_deactivate, > +}; > + > +static void rp1_unregister_interrupts(struct pci_dev *pdev) > +{ > + struct rp1_dev *rp1 = pci_get_drvdata(pdev); > + int irq, i; > + > + if (rp1->domain) { > + for (i = 0; i < RP1_INT_END; i++) { > + irq = irq_find_mapping(rp1->domain, i); > + irq_dispose_mapping(irq); > + } > + > + irq_domain_remove(rp1->domain); > + } > + > + pci_free_irq_vectors(pdev); > +} > + > +static int rp1_probe(struct pci_dev *pdev, const struct pci_device_id *id) > +{ > + u32 dtbo_size = __dtbo_rp1_pci_end - __dtbo_rp1_pci_begin; > + void *dtbo_start = __dtbo_rp1_pci_begin; > + struct device *dev = &pdev->dev; > + struct device_node *rp1_node; > + struct rp1_dev *rp1; > + int err = 0; > + int i; > + > + rp1_node = dev_of_node(dev); > + if (!rp1_node) { > + dev_err(dev, "Missing of_node for device\n"); > + return -EINVAL; > + } > + > + rp1 = devm_kzalloc(&pdev->dev, sizeof(*rp1), GFP_KERNEL); > + if (!rp1) > + return -ENOMEM; > + > + rp1->pdev = pdev; > + > + if (pci_resource_len(pdev, 1) <= 0x10000) { > + dev_err(&pdev->dev, > + "Not initialised - is the firmware running?\n"); > + return -EINVAL; > + } > + > + err = pcim_enable_device(pdev); > + if (err < 0) > + return dev_err_probe(&pdev->dev, err, > + "Enabling PCI device has failed"); > + > + rp1->bar1 = pcim_iomap(pdev, 1, 0); > + if (!rp1->bar1) { > + dev_err(&pdev->dev, "Cannot map PCI BAR\n"); > + return -EIO; > + } > + > + pci_set_master(pdev); > + > + err = pci_alloc_irq_vectors(pdev, RP1_INT_END, RP1_INT_END, > + PCI_IRQ_MSIX); > + if (err < 0) { > + return dev_err_probe(&pdev->dev, err, > + "pci_alloc_irq_vectors failed"); > + } else if (err != RP1_INT_END) { > + dev_err(&pdev->dev, "Cannot allocate enough interrupts\n"); > + return -EINVAL; > + } > + > + pci_set_drvdata(pdev, rp1); > + rp1->domain = irq_domain_add_linear(rp1_node, RP1_INT_END, > + &rp1_domain_ops, rp1); > + if (!rp1->domain) { > + dev_err(&pdev->dev, "Error creating IRQ domain\n"); > + err = -ENOMEM; > + goto err_unregister_interrupts; > + } > + > + for (i = 0; i < RP1_INT_END; i++) { > + unsigned int irq = irq_create_mapping(rp1->domain, i); > + > + if (!irq) { > + dev_err(&pdev->dev, "failed to create irq mapping\n"); > + err = -EINVAL; > + goto err_unregister_interrupts; > + } > + > + irq_set_chip_and_handler(irq, &rp1_irq_chip, handle_level_irq); > + irq_set_probe(irq); > + irq_set_chained_handler_and_data(pci_irq_vector(pdev, i), > + rp1_chained_handle_irq, rp1); > + } > + > + err = of_overlay_fdt_apply(dtbo_start, dtbo_size, &rp1->ovcs_id, rp1_node); > + if (err) > + goto err_unregister_interrupts; > + > + err = of_platform_default_populate(rp1_node, NULL, dev); > + if (err) > + goto err_unload_overlay; I think in this case it's worth to add a suitable dev_err() here. Thanks
Hi Stefan, On 15:21 Sat 08 Feb , Stefan Wahren wrote: > Hi Andrea, > > Am 07.02.25 um 22:31 schrieb Andrea della Porta: > > The RaspberryPi RP1 is a PCI multi function device containing > > peripherals ranging from Ethernet to USB controller, I2C, SPI > > and others. ... > > +static int rp1_irq_set_type(struct irq_data *irqd, unsigned int type) > > +{ > > + struct rp1_dev *rp1 = irqd->domain->host_data; > > + unsigned int hwirq = (unsigned int)irqd->hwirq; > > + > > + switch (type) { > > + case IRQ_TYPE_LEVEL_HIGH: > > + dev_dbg(&rp1->pdev->dev, "MSIX IACK EN for irq %d\n", hwirq); > This looks a little bit inconsistent. Only this type has a debug > message. So either we drop this or add at least a message for I think that this is indeed asymmetric. That warning says that the 'special' IACK management is engaged for level triggered interrupt, which is mandatory in order to avoid missing further interrupts without the performance loss of busy-polling for active interrupts. This is explained in par. 6.2 of: https://datasheets.raspberrypi.com/rp1/rp1-peripherals.pdf The point is that we're not stating the type of the interrupt (edge/level triggered), but we warn that we're enabling a mechanism useful for one type only (level triggered). > IRQ_TYPE_EDGE_RISING, too. Btw the format specifier looks wrong > (unsigned int vs %d). Ack. > > + msix_cfg_set(rp1, hwirq, MSIX_CFG_IACK_EN); > > + rp1->level_triggered_irq[hwirq] = true; > > + break; > > + case IRQ_TYPE_EDGE_RISING: > > + msix_cfg_clr(rp1, hwirq, MSIX_CFG_IACK_EN); > > + rp1->level_triggered_irq[hwirq] = false; > > + break; > > + default: > > + return -EINVAL; > It would be nice to document why only IRQ_TYPE_LEVEL_HIGH and > IRQ_TYPE_EDGE_RISING are supported. In case it's a software limitation, > this function would be a good place. In case this is a hardware > limitation this should be in the binding. All ints are level-triggered. I guess I should add a short comment in the bindings. > > + } > > + > > + return 0; > > +} > > + > > +static struct irq_chip rp1_irq_chip = { > > + .name = "rp1_irq_chip", > > + .irq_mask = rp1_mask_irq, > > + .irq_unmask = rp1_unmask_irq, > > + .irq_set_type = rp1_irq_set_type, > > +}; ... > > + irq_set_chip_and_handler(irq, &rp1_irq_chip, handle_level_irq); > > + irq_set_probe(irq); > > + irq_set_chained_handler_and_data(pci_irq_vector(pdev, i), > > + rp1_chained_handle_irq, rp1); > > + } > > + > > + err = of_overlay_fdt_apply(dtbo_start, dtbo_size, &rp1->ovcs_id, rp1_node); > > + if (err) > > + goto err_unregister_interrupts; > > + > > + err = of_platform_default_populate(rp1_node, NULL, dev); > > + if (err) > > + goto err_unload_overlay; > I think in this case it's worth to add a suitable dev_err() here. Ack. Many thanks, Andrea > > Thanks
Hello, Even though this is not for the PCI sub-system directly, I had a very brief look over the code. I hope you don't mind. As such, a few nit picks, nothing blocking. > +# RaspberryPi RP1 misc device Would this be better if it matched the "tristate" description below? > +config MISC_RP1 > + tristate "RaspberryPi RP1 PCIe support" > + depends on OF_IRQ && OF_OVERLAY && PCI_MSI && PCI_QUIRKS > + select PCI_DYNAMIC_OF_NODES > + help > + Support the RP1 peripheral chip found on Raspberry Pi 5 board. > + > + This device supports several sub-devices including e.g. Ethernet > + controller, USB controller, I2C, SPI and UART. > + > + The driver is responsible for enabling the DT node once the PCIe > + endpoint has been configured, and handling interrupts. > + > + This driver uses an overlay to load other drivers to support for > + RP1 internal sub-devices. > +/* the dts overlay is included from the dts directory so /* * The dts overlay is included from the dts directory so To make the code comment match rest of the style. > +/* > + * Copyright (c) 2018-24 Raspberry Pi Ltd. > + * All rights reserved. Copyright (c) 2018-2025 Raspberry Pi Ltd. To spell the current year fully, plus update it to 2025 already. I would also add an extra newline here to split the two apart a bit. > + if (pci_resource_len(pdev, 1) <= 0x10000) { > + dev_err(&pdev->dev, > + "Not initialised - is the firmware running?\n"); > + return -EINVAL; > + } The American spelling in the above might be better. But I don't have strong opinions here. It seems more popular in error messages. > + err = pci_alloc_irq_vectors(pdev, RP1_INT_END, RP1_INT_END, > + PCI_IRQ_MSIX); > + if (err < 0) { > + return dev_err_probe(&pdev->dev, err, > + "pci_alloc_irq_vectors failed"); Missing a new line at the end, but also... return dev_err_probe(&pdev->dev, err, "Failed to allocate MSI-X vectors\n"); Or, something like this over this the function name. Perhaps exposing error code could be useful to the end user? If so then something like this: return dev_err_probe(&pdev->dev, err, "Failed to allocate MSI-X vectors, err=%d\n", err); Here and other errors where appropriate. > + for (i = 0; i < RP1_INT_END; i++) { > + unsigned int irq = irq_create_mapping(rp1->domain, i); > + > + if (!irq) { > + dev_err(&pdev->dev, "failed to create irq mapping\n"); dev_err(&pdev->dev, "Failed to create IRQ mapping\n"); To make the error message capitalisation consistent. > +static const struct pci_device_id dev_id_table[] = { > + { PCI_DEVICE(PCI_VENDOR_ID_RPI, PCI_DEVICE_ID_RPI_RP1_C0), }, > + { 0, } { } Would probably be sufficient. > +MODULE_AUTHOR("Phil Elwell <phil@raspberrypi.com>"); > +MODULE_AUTHOR("Andrea della Porta <andrea.porta@suse.com>"); > +MODULE_DESCRIPTION("RP1 wrapper"); RaspberryPi RP1 misc device To match the Kconfig comment in the above description or the one from the "tristate" also in Kconfig. Thank you for all the work here! Krzysztof
Hi Krzysztof, On 17:37 Fri 14 Mar , Krzysztof Wilczynski wrote: > Hello, > > Even though this is not for the PCI sub-system directly, I had a very brief > look over the code. I hope you don't mind. Highly appreciated, thanks! > > As such, a few nit picks, nothing blocking. > > > +# RaspberryPi RP1 misc device > > Would this be better if it matched the "tristate" description below? Ack. > > > +config MISC_RP1 > > + tristate "RaspberryPi RP1 PCIe support" > > + depends on OF_IRQ && OF_OVERLAY && PCI_MSI && PCI_QUIRKS > > + select PCI_DYNAMIC_OF_NODES > > + help > > + Support the RP1 peripheral chip found on Raspberry Pi 5 board. > > + > > + This device supports several sub-devices including e.g. Ethernet > > + controller, USB controller, I2C, SPI and UART. > > + > > + The driver is responsible for enabling the DT node once the PCIe > > + endpoint has been configured, and handling interrupts. > > + > > + This driver uses an overlay to load other drivers to support for > > + RP1 internal sub-devices. > > > +/* the dts overlay is included from the dts directory so > > /* > * The dts overlay is included from the dts directory so > > To make the code comment match rest of the style. Ack. > > > +/* > > + * Copyright (c) 2018-24 Raspberry Pi Ltd. > > + * All rights reserved. > > Copyright (c) 2018-2025 Raspberry Pi Ltd. > > To spell the current year fully, plus update it to 2025 already. > > I would also add an extra newline here to split the two apart a bit. Ack. > > > + if (pci_resource_len(pdev, 1) <= 0x10000) { > > + dev_err(&pdev->dev, > > + "Not initialised - is the firmware running?\n"); > > + return -EINVAL; > > + } > > The American spelling in the above might be better. But I don't have > strong opinions here. It seems more popular in error messages. I agree. > > > + err = pci_alloc_irq_vectors(pdev, RP1_INT_END, RP1_INT_END, > > + PCI_IRQ_MSIX); > > + if (err < 0) { > > + return dev_err_probe(&pdev->dev, err, > > + "pci_alloc_irq_vectors failed"); > > Missing a new line at the end, but also... > > return dev_err_probe(&pdev->dev, err, > "Failed to allocate MSI-X vectors\n"); Ack. > > Or, something like this over this the function name. Perhaps exposing > error code could be useful to the end user? If so then something like this: > > return dev_err_probe(&pdev->dev, err, > "Failed to allocate MSI-X vectors, err=%d\n", err); dev_err_probe() should already print the err code, no need to add it. > > Here and other errors where appropriate. I've changed dev_err() to dev_err_probe() in cases where the error code is not evident (i.e. hardcoded) from the source. > > > + for (i = 0; i < RP1_INT_END; i++) { > > + unsigned int irq = irq_create_mapping(rp1->domain, i); > > + > > + if (!irq) { > > + dev_err(&pdev->dev, "failed to create irq mapping\n"); > > dev_err(&pdev->dev, "Failed to create IRQ mapping\n"); > > To make the error message capitalisation consistent. Ack. > > > +static const struct pci_device_id dev_id_table[] = { > > + { PCI_DEVICE(PCI_VENDOR_ID_RPI, PCI_DEVICE_ID_RPI_RP1_C0), }, > > + { 0, } > > { } > > Would probably be sufficient. Ack. > > > +MODULE_AUTHOR("Phil Elwell <phil@raspberrypi.com>"); > > +MODULE_AUTHOR("Andrea della Porta <andrea.porta@suse.com>"); > > +MODULE_DESCRIPTION("RP1 wrapper"); > > RaspberryPi RP1 misc device > > To match the Kconfig comment in the above description or the one from the > "tristate" also in Kconfig. Ack. > > Thank you for all the work here! Thank you so much for your review! Andrea > > Krzysztof
Hi Stefan, On 18:34 Thu 20 Feb , Andrea della Porta wrote: > Hi Stefan, > > On 15:21 Sat 08 Feb , Stefan Wahren wrote: > > Hi Andrea, > > > > Am 07.02.25 um 22:31 schrieb Andrea della Porta: > > > + msix_cfg_set(rp1, hwirq, MSIX_CFG_IACK_EN); > > > + rp1->level_triggered_irq[hwirq] = true; > > > + break; > > > + case IRQ_TYPE_EDGE_RISING: > > > + msix_cfg_clr(rp1, hwirq, MSIX_CFG_IACK_EN); > > > + rp1->level_triggered_irq[hwirq] = false; > > > + break; > > > + default: > > > + return -EINVAL; > > It would be nice to document why only IRQ_TYPE_LEVEL_HIGH and > > IRQ_TYPE_EDGE_RISING are supported. In case it's a software limitation, > > this function would be a good place. In case this is a hardware > > limitation this should be in the binding. > > All ints are level-triggered. I guess I should add a short comment in > the bindings. > Quick errata: s/level-triggered/active high/ Thanks, Andrea
Hello, [...] > > Or, something like this over this the function name. Perhaps exposing > > error code could be useful to the end user? If so then something like this: > > > > return dev_err_probe(&pdev->dev, err, > > "Failed to allocate MSI-X vectors, err=%d\n", err); > > dev_err_probe() should already print the err code, no need to add it. Ahh... I did a copy-paste of the code from above purely for illustration, sorry for the confusion here! > > Here and other errors where appropriate. > > I've changed dev_err() to dev_err_probe() in cases where the error code > is not evident (i.e. hardcoded) from the source. Makes sense. > Thank you so much for your review! Pleasure! Thank you for all the work here! Krzysztof
diff --git a/MAINTAINERS b/MAINTAINERS index 4cb38064694e..54f9e09f02ed 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -19757,6 +19757,7 @@ F: Documentation/devicetree/bindings/misc/pci1de4,1.yaml F: Documentation/devicetree/bindings/pci/pci-ep-bus.yaml F: Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml F: drivers/clk/clk-rp1.c +F: drivers/misc/rp1/ F: drivers/pinctrl/pinctrl-rp1.c F: include/dt-bindings/clock/rp1.h F: include/dt-bindings/misc/rp1.h diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index 56bc72c7ce4a..af8c3be967bf 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -649,4 +649,5 @@ source "drivers/misc/uacce/Kconfig" source "drivers/misc/pvpanic/Kconfig" source "drivers/misc/mchp_pci1xxxx/Kconfig" source "drivers/misc/keba/Kconfig" +source "drivers/misc/rp1/Kconfig" endmenu diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index 545aad06d088..5df79dd90c9c 100644 --- a/drivers/misc/Makefile +++ b/drivers/misc/Makefile @@ -75,3 +75,4 @@ lan966x-pci-objs := lan966x_pci.o lan966x-pci-objs += lan966x_pci.dtbo.o obj-$(CONFIG_MCHP_LAN966X_PCI) += lan966x-pci.o obj-y += keba/ +obj-$(CONFIG_MISC_RP1) += rp1/ diff --git a/drivers/misc/rp1/Kconfig b/drivers/misc/rp1/Kconfig new file mode 100644 index 000000000000..cfb02b2f186c --- /dev/null +++ b/drivers/misc/rp1/Kconfig @@ -0,0 +1,20 @@ +# SPDX-License-Identifier: GPL-2.0-only +# +# RaspberryPi RP1 misc device +# + +config MISC_RP1 + tristate "RaspberryPi RP1 PCIe support" + depends on OF_IRQ && OF_OVERLAY && PCI_MSI && PCI_QUIRKS + select PCI_DYNAMIC_OF_NODES + help + Support the RP1 peripheral chip found on Raspberry Pi 5 board. + + This device supports several sub-devices including e.g. Ethernet + controller, USB controller, I2C, SPI and UART. + + The driver is responsible for enabling the DT node once the PCIe + endpoint has been configured, and handling interrupts. + + This driver uses an overlay to load other drivers to support for + RP1 internal sub-devices. diff --git a/drivers/misc/rp1/Makefile b/drivers/misc/rp1/Makefile new file mode 100644 index 000000000000..508b4cb05627 --- /dev/null +++ b/drivers/misc/rp1/Makefile @@ -0,0 +1,3 @@ +# SPDX-License-Identifier: GPL-2.0-only +obj-$(CONFIG_MISC_RP1) += rp1-pci.o +rp1-pci-objs := rp1_pci.o rp1-pci.dtbo.o diff --git a/drivers/misc/rp1/rp1-pci.dtso b/drivers/misc/rp1/rp1-pci.dtso new file mode 100644 index 000000000000..0bf2f4bb18e6 --- /dev/null +++ b/drivers/misc/rp1/rp1-pci.dtso @@ -0,0 +1,8 @@ +// SPDX-License-Identifier: (GPL-2.0 OR MIT) + +/* the dts overlay is included from the dts directory so + * it can be possible to check it with CHECK_DTBS while + * also compile it from the driver source directory. + */ + +#include "arm64/broadcom/rp1.dtso" diff --git a/drivers/misc/rp1/rp1_pci.c b/drivers/misc/rp1/rp1_pci.c new file mode 100644 index 000000000000..c2d76e01bf57 --- /dev/null +++ b/drivers/misc/rp1/rp1_pci.c @@ -0,0 +1,305 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (c) 2018-24 Raspberry Pi Ltd. + * All rights reserved. + */ + +#include <linux/err.h> +#include <linux/interrupt.h> +#include <linux/irq.h> +#include <linux/irqchip/chained_irq.h> +#include <linux/irqdomain.h> +#include <linux/module.h> +#include <linux/msi.h> +#include <linux/of_platform.h> +#include <linux/pci.h> +#include <linux/platform_device.h> + +#define RP1_HW_IRQ_MASK GENMASK(5, 0) + +#define REG_SET 0x800 +#define REG_CLR 0xc00 + +/* MSI-X CFG registers start at 0x8 */ +#define MSIX_CFG(x) (0x8 + (4 * (x))) + +#define MSIX_CFG_IACK_EN BIT(3) +#define MSIX_CFG_IACK BIT(2) +#define MSIX_CFG_ENABLE BIT(0) + +/* Address map */ +#define RP1_PCIE_APBS_BASE 0x108000 + +/* Interrupts */ +#define RP1_INT_END 61 + +/* Embedded dtbo symbols created by cmd_wrap_S_dtb in scripts/Makefile.lib */ +extern char __dtbo_rp1_pci_begin[]; +extern char __dtbo_rp1_pci_end[]; + +struct rp1_dev { + struct pci_dev *pdev; + struct irq_domain *domain; + struct irq_data *pcie_irqds[64]; + void __iomem *bar1; + int ovcs_id; /* overlay changeset id */ + bool level_triggered_irq[RP1_INT_END]; +}; + +static void msix_cfg_set(struct rp1_dev *rp1, unsigned int hwirq, u32 value) +{ + iowrite32(value, rp1->bar1 + RP1_PCIE_APBS_BASE + REG_SET + MSIX_CFG(hwirq)); +} + +static void msix_cfg_clr(struct rp1_dev *rp1, unsigned int hwirq, u32 value) +{ + iowrite32(value, rp1->bar1 + RP1_PCIE_APBS_BASE + REG_CLR + MSIX_CFG(hwirq)); +} + +static void rp1_mask_irq(struct irq_data *irqd) +{ + struct rp1_dev *rp1 = irqd->domain->host_data; + struct irq_data *pcie_irqd = rp1->pcie_irqds[irqd->hwirq]; + + pci_msi_mask_irq(pcie_irqd); +} + +static void rp1_unmask_irq(struct irq_data *irqd) +{ + struct rp1_dev *rp1 = irqd->domain->host_data; + struct irq_data *pcie_irqd = rp1->pcie_irqds[irqd->hwirq]; + + pci_msi_unmask_irq(pcie_irqd); +} + +static int rp1_irq_set_type(struct irq_data *irqd, unsigned int type) +{ + struct rp1_dev *rp1 = irqd->domain->host_data; + unsigned int hwirq = (unsigned int)irqd->hwirq; + + switch (type) { + case IRQ_TYPE_LEVEL_HIGH: + dev_dbg(&rp1->pdev->dev, "MSIX IACK EN for irq %d\n", hwirq); + msix_cfg_set(rp1, hwirq, MSIX_CFG_IACK_EN); + rp1->level_triggered_irq[hwirq] = true; + break; + case IRQ_TYPE_EDGE_RISING: + msix_cfg_clr(rp1, hwirq, MSIX_CFG_IACK_EN); + rp1->level_triggered_irq[hwirq] = false; + break; + default: + return -EINVAL; + } + + return 0; +} + +static struct irq_chip rp1_irq_chip = { + .name = "rp1_irq_chip", + .irq_mask = rp1_mask_irq, + .irq_unmask = rp1_unmask_irq, + .irq_set_type = rp1_irq_set_type, +}; + +static void rp1_chained_handle_irq(struct irq_desc *desc) +{ + unsigned int hwirq = desc->irq_data.hwirq & RP1_HW_IRQ_MASK; + struct rp1_dev *rp1 = irq_desc_get_handler_data(desc); + struct irq_chip *chip = irq_desc_get_chip(desc); + unsigned int virq; + + chained_irq_enter(chip, desc); + + virq = irq_find_mapping(rp1->domain, hwirq); + generic_handle_irq(virq); + if (rp1->level_triggered_irq[hwirq]) + msix_cfg_set(rp1, hwirq, MSIX_CFG_IACK); + + chained_irq_exit(chip, desc); +} + +static int rp1_irq_xlate(struct irq_domain *d, struct device_node *node, + const u32 *intspec, unsigned int intsize, + unsigned long *out_hwirq, unsigned int *out_type) +{ + struct rp1_dev *rp1 = d->host_data; + struct irq_data *pcie_irqd; + unsigned long hwirq; + int pcie_irq; + int ret; + + ret = irq_domain_xlate_twocell(d, node, intspec, intsize, + &hwirq, out_type); + if (ret) + return ret; + + pcie_irq = pci_irq_vector(rp1->pdev, hwirq); + pcie_irqd = irq_get_irq_data(pcie_irq); + rp1->pcie_irqds[hwirq] = pcie_irqd; + *out_hwirq = hwirq; + + return 0; +} + +static int rp1_irq_activate(struct irq_domain *d, struct irq_data *irqd, + bool reserve) +{ + struct rp1_dev *rp1 = d->host_data; + + msix_cfg_set(rp1, (unsigned int)irqd->hwirq, MSIX_CFG_ENABLE); + + return 0; +} + +static void rp1_irq_deactivate(struct irq_domain *d, struct irq_data *irqd) +{ + struct rp1_dev *rp1 = d->host_data; + + msix_cfg_clr(rp1, (unsigned int)irqd->hwirq, MSIX_CFG_ENABLE); +} + +static const struct irq_domain_ops rp1_domain_ops = { + .xlate = rp1_irq_xlate, + .activate = rp1_irq_activate, + .deactivate = rp1_irq_deactivate, +}; + +static void rp1_unregister_interrupts(struct pci_dev *pdev) +{ + struct rp1_dev *rp1 = pci_get_drvdata(pdev); + int irq, i; + + if (rp1->domain) { + for (i = 0; i < RP1_INT_END; i++) { + irq = irq_find_mapping(rp1->domain, i); + irq_dispose_mapping(irq); + } + + irq_domain_remove(rp1->domain); + } + + pci_free_irq_vectors(pdev); +} + +static int rp1_probe(struct pci_dev *pdev, const struct pci_device_id *id) +{ + u32 dtbo_size = __dtbo_rp1_pci_end - __dtbo_rp1_pci_begin; + void *dtbo_start = __dtbo_rp1_pci_begin; + struct device *dev = &pdev->dev; + struct device_node *rp1_node; + struct rp1_dev *rp1; + int err = 0; + int i; + + rp1_node = dev_of_node(dev); + if (!rp1_node) { + dev_err(dev, "Missing of_node for device\n"); + return -EINVAL; + } + + rp1 = devm_kzalloc(&pdev->dev, sizeof(*rp1), GFP_KERNEL); + if (!rp1) + return -ENOMEM; + + rp1->pdev = pdev; + + if (pci_resource_len(pdev, 1) <= 0x10000) { + dev_err(&pdev->dev, + "Not initialised - is the firmware running?\n"); + return -EINVAL; + } + + err = pcim_enable_device(pdev); + if (err < 0) + return dev_err_probe(&pdev->dev, err, + "Enabling PCI device has failed"); + + rp1->bar1 = pcim_iomap(pdev, 1, 0); + if (!rp1->bar1) { + dev_err(&pdev->dev, "Cannot map PCI BAR\n"); + return -EIO; + } + + pci_set_master(pdev); + + err = pci_alloc_irq_vectors(pdev, RP1_INT_END, RP1_INT_END, + PCI_IRQ_MSIX); + if (err < 0) { + return dev_err_probe(&pdev->dev, err, + "pci_alloc_irq_vectors failed"); + } else if (err != RP1_INT_END) { + dev_err(&pdev->dev, "Cannot allocate enough interrupts\n"); + return -EINVAL; + } + + pci_set_drvdata(pdev, rp1); + rp1->domain = irq_domain_add_linear(rp1_node, RP1_INT_END, + &rp1_domain_ops, rp1); + if (!rp1->domain) { + dev_err(&pdev->dev, "Error creating IRQ domain\n"); + err = -ENOMEM; + goto err_unregister_interrupts; + } + + for (i = 0; i < RP1_INT_END; i++) { + unsigned int irq = irq_create_mapping(rp1->domain, i); + + if (!irq) { + dev_err(&pdev->dev, "failed to create irq mapping\n"); + err = -EINVAL; + goto err_unregister_interrupts; + } + + irq_set_chip_and_handler(irq, &rp1_irq_chip, handle_level_irq); + irq_set_probe(irq); + irq_set_chained_handler_and_data(pci_irq_vector(pdev, i), + rp1_chained_handle_irq, rp1); + } + + err = of_overlay_fdt_apply(dtbo_start, dtbo_size, &rp1->ovcs_id, rp1_node); + if (err) + goto err_unregister_interrupts; + + err = of_platform_default_populate(rp1_node, NULL, dev); + if (err) + goto err_unload_overlay; + + return 0; + +err_unload_overlay: + of_overlay_remove(&rp1->ovcs_id); +err_unregister_interrupts: + rp1_unregister_interrupts(pdev); + + return err; +} + +static void rp1_remove(struct pci_dev *pdev) +{ + struct rp1_dev *rp1 = pci_get_drvdata(pdev); + struct device *dev = &pdev->dev; + + of_platform_depopulate(dev); + of_overlay_remove(&rp1->ovcs_id); + rp1_unregister_interrupts(pdev); +} + +static const struct pci_device_id dev_id_table[] = { + { PCI_DEVICE(PCI_VENDOR_ID_RPI, PCI_DEVICE_ID_RPI_RP1_C0), }, + { 0, } +}; +MODULE_DEVICE_TABLE(pci, dev_id_table); + +static struct pci_driver rp1_driver = { + .name = KBUILD_MODNAME, + .id_table = dev_id_table, + .probe = rp1_probe, + .remove = rp1_remove, +}; + +module_pci_driver(rp1_driver); + +MODULE_AUTHOR("Phil Elwell <phil@raspberrypi.com>"); +MODULE_AUTHOR("Andrea della Porta <andrea.porta@suse.com>"); +MODULE_DESCRIPTION("RP1 wrapper"); +MODULE_LICENSE("GPL"); diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index b84ff7bade82..4dfda527c76b 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -6283,6 +6283,7 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_XILINX, 0x5020, of_pci_make_dev_node); DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_XILINX, 0x5021, of_pci_make_dev_node); DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_REDHAT, 0x0005, of_pci_make_dev_node); DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_EFAR, 0x9660, of_pci_make_dev_node); +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_RPI, PCI_DEVICE_ID_RPI_RP1_C0, of_pci_make_dev_node); /* * Devices known to require a longer delay before first config space access diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h index de5deb1a0118..b7a2c0fd589e 100644 --- a/include/linux/pci_ids.h +++ b/include/linux/pci_ids.h @@ -2620,6 +2620,9 @@ #define PCI_VENDOR_ID_TEKRAM 0x1de1 #define PCI_DEVICE_ID_TEKRAM_DC290 0xdc29 +#define PCI_VENDOR_ID_RPI 0x1de4 +#define PCI_DEVICE_ID_RPI_RP1_C0 0x0001 + #define PCI_VENDOR_ID_ALIBABA 0x1ded #define PCI_VENDOR_ID_CXL 0x1e98
The RaspberryPi RP1 is a PCI multi function device containing peripherals ranging from Ethernet to USB controller, I2C, SPI and others. Implement a bare minimum driver to operate the RP1, leveraging actual OF based driver implementations for the on-board peripherals by loading a devicetree overlay during driver probe. The peripherals are accessed by mapping MMIO registers starting from PCI BAR1 region. With the overlay approach we can achieve more generic and agnostic approach to managing this chipset, being that it is a PCI endpoint and could possibly be reused in other hw implementations. The presented approach is also used by Bootlin's Microchip LAN966x patchset (see link) as well, for a similar chipset. The reason why this driver is contained in drivers/misc has been paved by Bootlin's LAN966X driver, which first used the overlay approach to implement non discoverable peripherals behind a PCI bus. For RP1, the same arguments apply: it's not used as an SoC since the driver code is not running on-chip and is not like an MFD since it does not really need all the MFD infrastructure (shared regs, etc.). So, for this particular use, misc has been proposed and deemed as a good choice. For further details about that please check the links. This driver is heavily based on downstream code from RaspberryPi Foundation, and the original author is Phil Elwell. Link: https://datasheets.raspberrypi.com/rp1/rp1-peripherals.pdf Link: https://lore.kernel.org/all/20240612140208.GC1504919@google.com/ Link: https://lore.kernel.org/all/83f7fa09-d0e6-4f36-a27d-cee08979be2a@app.fastmail.com/ Link: https://lore.kernel.org/all/2024081356-mutable-everyday-6f9d@gregkh/ Link: https://lore.kernel.org/all/20240808154658.247873-1-herve.codina@bootlin.com/ Signed-off-by: Andrea della Porta <andrea.porta@suse.com> --- MAINTAINERS | 1 + drivers/misc/Kconfig | 1 + drivers/misc/Makefile | 1 + drivers/misc/rp1/Kconfig | 20 +++ drivers/misc/rp1/Makefile | 3 + drivers/misc/rp1/rp1-pci.dtso | 8 + drivers/misc/rp1/rp1_pci.c | 305 ++++++++++++++++++++++++++++++++++ drivers/pci/quirks.c | 1 + include/linux/pci_ids.h | 3 + 9 files changed, 343 insertions(+) create mode 100644 drivers/misc/rp1/Kconfig create mode 100644 drivers/misc/rp1/Makefile create mode 100644 drivers/misc/rp1/rp1-pci.dtso create mode 100644 drivers/misc/rp1/rp1_pci.c