mbox series

[v5,00/10] Add support for RaspberryPi RP1 PCI device using a DT overlay

Message ID cover.1733136811.git.andrea.porta@suse.com
Headers show
Series Add support for RaspberryPi RP1 PCI device using a DT overlay | expand

Message

Andrea della Porta Dec. 2, 2024, 11:19 a.m. UTC
RP1 is an MFD chipset that acts as a south-bridge PCIe endpoint sporting
a pletora of subdevices (i.e.  Ethernet, USB host controller, I2C, PWM,
etc.) whose registers are all reachable starting from an offset from the
BAR address.  The main point here is that while the RP1 as an endpoint
itself is discoverable via usual PCI enumeraiton, the devices it contains
are not discoverable and must be declared e.g. via the devicetree.

This patchset is an attempt to provide a minimum infrastructure to allow
the RP1 chipset to be discovered and perpherals it contains to be added
from a devictree overlay loaded during RP1 PCI endpoint enumeration.
Followup patches should add support for the several peripherals contained
in RP1.

This work is based upon dowstream drivers code and the proposal from RH
et al. (see [1] and [2]). A similar approach is also pursued in [3].

The patches are ordered as follows:

-PATCHES 1 to 4: add binding schemas for clock, gpio and RP1 peripherals.
 They are needed to support the other peripherals, e.g. the ethernet mac
 depends on a clock generated by RP1 and the phy is reset though the
 on-board gpio controller.

-PATCH 5 and 6: add clock and gpio device drivers.

-PATCH 7: the devicetree overlay describing the RP1 chipset. Please
 note that this patch should be taken by the same maintainer that will
 also take patch 11, since txeieh dtso is compiled in as binary blob and is
 closely coupled to the driver.

-PATCH 8: this is the main patch to support RP1 chipset and peripherals
 enabling through dtb overlay. The dtso since is intimately coupled with
 the driver and will be linked in as binary blob in the driver obj.
 The real dtso is in devicetree folder while the dtso in driver folder is
 just a placeholder to include the real dtso.
 In this way it is possible to check the dtso against dt-bindings.
 The reason why drivers/misc has been selected as containing folder
 for this driver can be seen in [6], [7] and [8].

-PATCH 9: add the external clock node (used by RP1) to the main dts.

-PATCH 10: add the relevant kernel CONFIG_ options to defconfig.

This patchset is also a first attempt to be more agnostic wrt hardware
description standards such as OF devicetree and ACPI, where 'agnostic'
means "using DT in coexistence with ACPI", as been already promoted
by e.g. AL (see [4]). Although there's currently no evidence it will also
run out of the box on purely ACPI system, it is a first step towards
that direction.

Please note that albeit this patchset has no prerequisites in order to
be applied cleanly, it still depends on Stanimir's WIP patchset for BCM2712
PCIe controller (see [5]) in order to work at runtime.

Many thanks,
Andrea della Porta

Links:
- [1]: https://lpc.events/event/17/contributions/1421/attachments/1337/2680/LPC2023%20Non-discoverable%20devices%20in%20PCI.pdf
- [2]: https://lore.kernel.org/lkml/20230419231155.GA899497-robh@kernel.org/t/
- [3]: https://lore.kernel.org/all/20240808154658.247873-1-herve.codina@bootlin.com/#t
- [4]: https://lore.kernel.org/all/73e05c77-6d53-4aae-95ac-415456ff0ae4@lunn.ch/
- [5]: https://lore.kernel.org/all/20240626104544.14233-1-svarbanov@suse.de/
- [6]: https://lore.kernel.org/all/20240612140208.GC1504919@google.com/
- [7]: https://lore.kernel.org/all/83f7fa09-d0e6-4f36-a27d-cee08979be2a@app.fastmail.com/
- [8]: https://lore.kernel.org/all/2024081356-mutable-everyday-6f9d@gregkh/


CHANGES IN V5:

PATCH RELATED -------------------------------------------------

- patch 1 and 2: added: 'Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>'

- patch 6 and 10: added 'Reviewed-by: Stefan Wahren <wahrenst@gmx.net>'

- rebased on v6.13-rc1


KCONFIG -----------------------------------------

- drivers/misc/rp1/Kconfig: leading spaces converted to tab


RP1 MISC DRIVER -----------------------------------

- added a comment to describe ovcs_id field from struct rp1_dev

- added braces around a single line if statement followed by a braced
  else condition

- removed a double space character


GPIO/PINCTRL --------------------------------------

- moved a multiplication (*) sign to the line it belongs to (code style
  fixup)


DTS -----------------------------------------

- Moved clk_rp1_xosc from SoC DTS to board DTS


BINDINGS ------------------------------------

- pci1de4,1.yaml: adjusted ranges and reg properties in the example
  to better reflect the address from RP1 documentation. These refelcts
  the same convention used in the dtso

- raspberrypi,rp1-gpio.yaml: fixed 'pins' pattern to go at most up to 53

- raspberrypi,rp1-gpio.yaml: using single quotes consistently over double
  quotes

- raspberrypi,rp1-gpio.yaml: added some pin config option supported by the
  hw/driver but missing in the schema


Andrea della Porta (10):
  dt-bindings: clock: Add RaspberryPi RP1 clock bindings
  dt-bindings: pinctrl: Add RaspberryPi RP1 gpio/pinctrl/pinmux bindings
  dt-bindings: pci: Add common schema for devices accessible through PCI
    BARs
  dt-bindings: misc: Add device specific bindings for RaspberryPi RP1
  clk: rp1: Add support for clocks provided by RP1
  pinctrl: rp1: Implement RaspberryPi RP1 gpio support
  arm64: dts: rp1: Add support for RaspberryPi's RP1 device
  misc: rp1: RaspberryPi RP1 misc driver
  arm64: dts: bcm2712: Add external clock for RP1 chipset on Rpi5
  arm64: defconfig: Enable RP1 misc/clock/gpio drivers

 .../clock/raspberrypi,rp1-clocks.yaml         |   58 +
 .../devicetree/bindings/misc/pci1de4,1.yaml   |   73 +
 .../devicetree/bindings/pci/pci-ep-bus.yaml   |   58 +
 .../pinctrl/raspberrypi,rp1-gpio.yaml         |  198 +++
 MAINTAINERS                                   |   14 +
 .../boot/dts/broadcom/bcm2712-rpi-5-b.dts     |    7 +
 arch/arm64/boot/dts/broadcom/rp1.dtso         |   58 +
 arch/arm64/configs/defconfig                  |    3 +
 drivers/clk/Kconfig                           |    9 +
 drivers/clk/Makefile                          |    1 +
 drivers/clk/clk-rp1.c                         | 1527 +++++++++++++++++
 drivers/misc/Kconfig                          |    1 +
 drivers/misc/Makefile                         |    1 +
 drivers/misc/rp1/Kconfig                      |   21 +
 drivers/misc/rp1/Makefile                     |    3 +
 drivers/misc/rp1/rp1-pci.dtso                 |    8 +
 drivers/misc/rp1/rp1_pci.c                    |  366 ++++
 drivers/misc/rp1/rp1_pci.h                    |   14 +
 drivers/pci/quirks.c                          |    1 +
 drivers/pinctrl/Kconfig                       |   11 +
 drivers/pinctrl/Makefile                      |    1 +
 drivers/pinctrl/pinctrl-rp1.c                 |  789 +++++++++
 .../clock/raspberrypi,rp1-clocks.h            |   61 +
 include/linux/pci_ids.h                       |    3 +
 24 files changed, 3286 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml
 create mode 100644 Documentation/devicetree/bindings/misc/pci1de4,1.yaml
 create mode 100644 Documentation/devicetree/bindings/pci/pci-ep-bus.yaml
 create mode 100644 Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml
 create mode 100644 arch/arm64/boot/dts/broadcom/rp1.dtso
 create mode 100644 drivers/clk/clk-rp1.c
 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
 create mode 100644 drivers/misc/rp1/rp1_pci.h
 create mode 100644 drivers/pinctrl/pinctrl-rp1.c
 create mode 100644 include/dt-bindings/clock/raspberrypi,rp1-clocks.h

Comments

Rob Herring (Arm) Dec. 10, 2024, 10:48 p.m. UTC | #1
On Mon, Dec 02, 2024 at 12:19:32PM +0100, Andrea della Porta wrote:
> 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.
> 
> Since the gpio line names should be provided by the user, there
> is an interface through configfs that allows the userspace to
> load a DT overlay that will provide gpio-line-names property.
> The interface can be invoked like this:
> 
> cat rpi-rp1-gpios-5-b.dtbo > /sys/kernel/config/rp1-cfg/gpio_set_names

Where's the configfs support? This looks like a stale comment.

If this is the general purpose configfs interface for overlays, that's 
likely never going upstream. If you want this support, then you should 
load your overlay with the firmware loader instead of building it into 
the kernel. You can of course add that later.

> and is designed to be similar to what users are already used to
> from distro with downstream kernel.
> 
> For reasons why this driver is contained in drivers/misc, 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      |  21 ++
>  drivers/misc/rp1/Makefile     |   3 +
>  drivers/misc/rp1/rp1-pci.dtso |   8 +
>  drivers/misc/rp1/rp1_pci.c    | 366 ++++++++++++++++++++++++++++++++++
>  drivers/misc/rp1/rp1_pci.h    |  14 ++
>  drivers/pci/quirks.c          |   1 +
>  include/linux/pci_ids.h       |   3 +
>  10 files changed, 419 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
>  create mode 100644 drivers/misc/rp1/rp1_pci.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index fbdd8594aa7e..d67ba6d10aa8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19583,6 +19583,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 09cbe3f0ab1e..ffa4d8315c35 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -651,4 +651,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 40bf953185c7..3b6b07a23aac 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -74,3 +74,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..15c443e13389
> --- /dev/null
> +++ b/drivers/misc/rp1/Kconfig
> @@ -0,0 +1,21 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# RaspberryPi RP1 misc device
> +#
> +
> +config MISC_RP1
> +	tristate "RaspberryPi RP1 PCIe support"
> +	depends on OF_IRQ && PCI_MSI && PCI_QUIRKS
> +	select OF_OVERLAY
> +	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.

I haven't tried, but I think if you set CHECK_DTBS=y, then this file 
will get checked. Applying it to the base DT will not though.

> + */
> +
> +#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..0dd345341c6f
> --- /dev/null
> +++ b/drivers/misc/rp1/rp1_pci.c
> @@ -0,0 +1,366 @@
> +// 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>
> +
> +#include "rp1_pci.h"
> +
> +#define RP1_DRIVER_NAME		"rp1"
> +
> +#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_IO_BANK0	0
> +#define RP1_INT_IO_BANK1	1
> +#define RP1_INT_IO_BANK2	2
> +#define RP1_INT_AUDIO_IN	3
> +#define RP1_INT_AUDIO_OUT	4
> +#define RP1_INT_PWM0		5
> +#define RP1_INT_ETH		6
> +#define RP1_INT_I2C0		7
> +#define RP1_INT_I2C1		8
> +#define RP1_INT_I2C2		9
> +#define RP1_INT_I2C3		10
> +#define RP1_INT_I2C4		11
> +#define RP1_INT_I2C5		12
> +#define RP1_INT_I2C6		13
> +#define RP1_INT_I2S0		14
> +#define RP1_INT_I2S1		15
> +#define RP1_INT_I2S2		16
> +#define RP1_INT_SDIO0		17
> +#define RP1_INT_SDIO1		18
> +#define RP1_INT_SPI0		19
> +#define RP1_INT_SPI1		20
> +#define RP1_INT_SPI2		21
> +#define RP1_INT_SPI3		22
> +#define RP1_INT_SPI4		23
> +#define RP1_INT_SPI5		24
> +#define RP1_INT_UART0		25
> +#define RP1_INT_TIMER_0		26
> +#define RP1_INT_TIMER_1		27
> +#define RP1_INT_TIMER_2		28
> +#define RP1_INT_TIMER_3		29
> +#define RP1_INT_USBHOST0	30
> +#define RP1_INT_USBHOST0_0	31
> +#define RP1_INT_USBHOST0_1	32
> +#define RP1_INT_USBHOST0_2	33
> +#define RP1_INT_USBHOST0_3	34
> +#define RP1_INT_USBHOST1	35
> +#define RP1_INT_USBHOST1_0	36
> +#define RP1_INT_USBHOST1_1	37
> +#define RP1_INT_USBHOST1_2	38
> +#define RP1_INT_USBHOST1_3	39
> +#define RP1_INT_DMA		40
> +#define RP1_INT_PWM1		41
> +#define RP1_INT_UART1		42
> +#define RP1_INT_UART2		43
> +#define RP1_INT_UART3		44
> +#define RP1_INT_UART4		45
> +#define RP1_INT_UART5		46
> +#define RP1_INT_MIPI0		47
> +#define RP1_INT_MIPI1		48
> +#define RP1_INT_VIDEO_OUT	49
> +#define RP1_INT_PIO_0		50
> +#define RP1_INT_PIO_1		51
> +#define RP1_INT_ADC_FIFO	52
> +#define RP1_INT_PCIE_OUT	53
> +#define RP1_INT_SPI6		54
> +#define RP1_INT_SPI7		55
> +#define RP1_INT_SPI8		56
> +#define RP1_INT_SYSCFG		58
> +#define RP1_INT_CLOCKS_DEFAULT	59
> +#define RP1_INT_VBUSCTRL	60
> +#define RP1_INT_PROC_MISC	57

Why all these defines which will never be used because they come from 
DT?

Rob
Andrea della Porta Dec. 16, 2024, 2:08 p.m. UTC | #2
Hi Rob,

On 16:48 Tue 10 Dec     , Rob Herring wrote:
> On Mon, Dec 02, 2024 at 12:19:32PM +0100, Andrea della Porta wrote:
> > 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.
> > 
> > Since the gpio line names should be provided by the user, there
> > is an interface through configfs that allows the userspace to
> > load a DT overlay that will provide gpio-line-names property.
> > The interface can be invoked like this:
> > 
> > cat rpi-rp1-gpios-5-b.dtbo > /sys/kernel/config/rp1-cfg/gpio_set_names
> 
> Where's the configfs support? This looks like a stale comment.

Yes, you are right. Thanks for pointing that out, I've dropped that
paragraph, since there is no gpio name support in this specific patchset
anymore.

> 
> If this is the general purpose configfs interface for overlays, that's 
> likely never going upstream. If you want this support, then you should 
> load your overlay with the firmware loader instead of building it into 
> the kernel. You can of course add that later.

I think the best thing would be to leave the actual base overlay compiled
inside the driver as it is now, and to prepare a secondary overlay for the
gpio names to be loaded as a binary blob through request_firmware(). Does
it make sense? Of course, the gpio name overlay support would be a future
patchset of its own.

> 
> > and is designed to be similar to what users are already used to
> > from distro with downstream kernel.
> > 
> > For reasons why this driver is contained in drivers/misc, 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      |  21 ++
> >  drivers/misc/rp1/Makefile     |   3 +
> >  drivers/misc/rp1/rp1-pci.dtso |   8 +
> >  drivers/misc/rp1/rp1_pci.c    | 366 ++++++++++++++++++++++++++++++++++
> >  drivers/misc/rp1/rp1_pci.h    |  14 ++
> >  drivers/pci/quirks.c          |   1 +
> >  include/linux/pci_ids.h       |   3 +
> >  10 files changed, 419 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
> >  create mode 100644 drivers/misc/rp1/rp1_pci.h
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index fbdd8594aa7e..d67ba6d10aa8 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -19583,6 +19583,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 09cbe3f0ab1e..ffa4d8315c35 100644
> > --- a/drivers/misc/Kconfig
> > +++ b/drivers/misc/Kconfig
> > @@ -651,4 +651,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 40bf953185c7..3b6b07a23aac 100644
> > --- a/drivers/misc/Makefile
> > +++ b/drivers/misc/Makefile
> > @@ -74,3 +74,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..15c443e13389
> > --- /dev/null
> > +++ b/drivers/misc/rp1/Kconfig
> > @@ -0,0 +1,21 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +#
> > +# RaspberryPi RP1 misc device
> > +#
> > +
> > +config MISC_RP1
> > +	tristate "RaspberryPi RP1 PCIe support"
> > +	depends on OF_IRQ && PCI_MSI && PCI_QUIRKS
> > +	select OF_OVERLAY
> > +	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.
> 
> I haven't tried, but I think if you set CHECK_DTBS=y, then this file 
> will get checked. Applying it to the base DT will not though.

I think we're saying the same thing here, which is that the dtso can be
checked by passing CHECK_DTB=y but it has to be located somewhere under
arch/*/boot/dts. Or maybe I'm misunderstandig what you mean here?

> 
> > + */
> > +
> > +#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..0dd345341c6f
> > --- /dev/null
> > +++ b/drivers/misc/rp1/rp1_pci.c
> > @@ -0,0 +1,366 @@
> > +// 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>
> > +
> > +#include "rp1_pci.h"
> > +
> > +#define RP1_DRIVER_NAME		"rp1"
> > +
> > +#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_IO_BANK0	0
> > +#define RP1_INT_IO_BANK1	1
> > +#define RP1_INT_IO_BANK2	2
> > +#define RP1_INT_AUDIO_IN	3
> > +#define RP1_INT_AUDIO_OUT	4
> > +#define RP1_INT_PWM0		5
> > +#define RP1_INT_ETH		6
> > +#define RP1_INT_I2C0		7
> > +#define RP1_INT_I2C1		8
> > +#define RP1_INT_I2C2		9
> > +#define RP1_INT_I2C3		10
> > +#define RP1_INT_I2C4		11
> > +#define RP1_INT_I2C5		12
> > +#define RP1_INT_I2C6		13
> > +#define RP1_INT_I2S0		14
> > +#define RP1_INT_I2S1		15
> > +#define RP1_INT_I2S2		16
> > +#define RP1_INT_SDIO0		17
> > +#define RP1_INT_SDIO1		18
> > +#define RP1_INT_SPI0		19
> > +#define RP1_INT_SPI1		20
> > +#define RP1_INT_SPI2		21
> > +#define RP1_INT_SPI3		22
> > +#define RP1_INT_SPI4		23
> > +#define RP1_INT_SPI5		24
> > +#define RP1_INT_UART0		25
> > +#define RP1_INT_TIMER_0		26
> > +#define RP1_INT_TIMER_1		27
> > +#define RP1_INT_TIMER_2		28
> > +#define RP1_INT_TIMER_3		29
> > +#define RP1_INT_USBHOST0	30
> > +#define RP1_INT_USBHOST0_0	31
> > +#define RP1_INT_USBHOST0_1	32
> > +#define RP1_INT_USBHOST0_2	33
> > +#define RP1_INT_USBHOST0_3	34
> > +#define RP1_INT_USBHOST1	35
> > +#define RP1_INT_USBHOST1_0	36
> > +#define RP1_INT_USBHOST1_1	37
> > +#define RP1_INT_USBHOST1_2	38
> > +#define RP1_INT_USBHOST1_3	39
> > +#define RP1_INT_DMA		40
> > +#define RP1_INT_PWM1		41
> > +#define RP1_INT_UART1		42
> > +#define RP1_INT_UART2		43
> > +#define RP1_INT_UART3		44
> > +#define RP1_INT_UART4		45
> > +#define RP1_INT_UART5		46
> > +#define RP1_INT_MIPI0		47
> > +#define RP1_INT_MIPI1		48
> > +#define RP1_INT_VIDEO_OUT	49
> > +#define RP1_INT_PIO_0		50
> > +#define RP1_INT_PIO_1		51
> > +#define RP1_INT_ADC_FIFO	52
> > +#define RP1_INT_PCIE_OUT	53
> > +#define RP1_INT_SPI6		54
> > +#define RP1_INT_SPI7		55
> > +#define RP1_INT_SPI8		56
> > +#define RP1_INT_SYSCFG		58
> > +#define RP1_INT_CLOCKS_DEFAULT	59
> > +#define RP1_INT_VBUSCTRL	60
> > +#define RP1_INT_PROC_MISC	57
> 
> Why all these defines which will never be used because they come from 
> DT?
>

Right, those defines where originally designed to be included from dts, but
previous discussion deemed interrupt numbers to be hardcoded instead of being
specified as mnemonics. In the driver source code I just use RP1_INT_END as the
number of interrupts but I thought that the specific interrupt numbers should
be documented in some way or another. Since no one is currently referencing
those defines, would it be better to just turn those in a multiline comment
just to describe them in a more compact form?

Many thanks,
Andrea

> Rob
Andrea della Porta Jan. 9, 2025, 2:13 p.m. UTC | #3
Hi Rob,

On 15:08 Mon 16 Dec     , Andrea della Porta wrote:
> Hi Rob,
> 
> On 16:48 Tue 10 Dec     , Rob Herring wrote:
> > On Mon, Dec 02, 2024 at 12:19:32PM +0100, Andrea della Porta wrote:
> > > The RaspberryPi RP1 is a PCI multi function device containing
> > > peripherals ranging from Ethernet to USB controller, I2C, SPI
> > > and others.

...

> > > +#define RP1_INT_ADC_FIFO	52
> > > +#define RP1_INT_PCIE_OUT	53
> > > +#define RP1_INT_SPI6		54
> > > +#define RP1_INT_SPI7		55
> > > +#define RP1_INT_SPI8		56
> > > +#define RP1_INT_SYSCFG		58
> > > +#define RP1_INT_CLOCKS_DEFAULT	59
> > > +#define RP1_INT_VBUSCTRL	60
> > > +#define RP1_INT_PROC_MISC	57
> > 
> > Why all these defines which will never be used because they come from 
> > DT?
> >
> 
> Right, those defines where originally designed to be included from dts, but
> previous discussion deemed interrupt numbers to be hardcoded instead of being
> specified as mnemonics. In the driver source code I just use RP1_INT_END as the
> number of interrupts but I thought that the specific interrupt numbers should
> be documented in some way or another. Since no one is currently referencing
> those defines, would it be better to just turn those in a multiline comment
> just to describe them in a more compact form?

So, here's a couple of proposals about the interrupt defines:

- since they were banned from devicetree, and are not used anywhere in the code,
  turn them into a (admittedly long) multiline comment, so they are still at
  least documented

- since they were banned from devicetree, and are not use anywhere in the code,
  just drop them, we don't currently need them after all

Not sure what's the best way here, anyone can advise?

Many thanks,
Andrea
Herve Codina Jan. 9, 2025, 2:50 p.m. UTC | #4
Hi Andrea,

On Thu, 9 Jan 2025 15:13:42 +0100
Andrea della Porta <andrea.porta@suse.com> wrote:

> Hi Rob,
> 
> On 15:08 Mon 16 Dec     , Andrea della Porta wrote:
> > Hi Rob,
> > 
> > On 16:48 Tue 10 Dec     , Rob Herring wrote:  
> > > On Mon, Dec 02, 2024 at 12:19:32PM +0100, Andrea della Porta wrote:  
> > > > The RaspberryPi RP1 is a PCI multi function device containing
> > > > peripherals ranging from Ethernet to USB controller, I2C, SPI
> > > > and others.  
> 
> ...
> 
> > > > +#define RP1_INT_ADC_FIFO	52
> > > > +#define RP1_INT_PCIE_OUT	53
> > > > +#define RP1_INT_SPI6		54
> > > > +#define RP1_INT_SPI7		55
> > > > +#define RP1_INT_SPI8		56
> > > > +#define RP1_INT_SYSCFG		58
> > > > +#define RP1_INT_CLOCKS_DEFAULT	59
> > > > +#define RP1_INT_VBUSCTRL	60
> > > > +#define RP1_INT_PROC_MISC	57  
> > > 
> > > Why all these defines which will never be used because they come from 
> > > DT?
> > >  
> > 
> > Right, those defines where originally designed to be included from dts, but
> > previous discussion deemed interrupt numbers to be hardcoded instead of being
> > specified as mnemonics. In the driver source code I just use RP1_INT_END as the
> > number of interrupts but I thought that the specific interrupt numbers should
> > be documented in some way or another. Since no one is currently referencing
> > those defines, would it be better to just turn those in a multiline comment
> > just to describe them in a more compact form?  
> 
> So, here's a couple of proposals about the interrupt defines:
> 
> - since they were banned from devicetree, and are not used anywhere in the code,
>   turn them into a (admittedly long) multiline comment, so they are still at
>   least documented
> 
> - since they were banned from devicetree, and are not use anywhere in the code,
>   just drop them, we don't currently need them after all
> 
> Not sure what's the best way here, anyone can advise?

Maybe in the #interrupt-cells description in the device-tree binding?

In your patch 4, you describe this interrupt controller and you have:
  '#interrupt-cells':
    const: 2
    description:
      Specifies respectively the interrupt number and flags as defined
      in include/dt-bindings/interrupt-controller/irq.h.

In this description, why not add the supported interrupt number values?
    description: |
      Specifies respectively the interrupt number and flags as defined
      in include/dt-bindings/interrupt-controller/irq.h.
      The supported values for the interrupt number are:
        - IO BANK0: 0
        - IO BANK1: 1
...

Or something similar.

This kind of description is already available. For instance:
  https://elixir.bootlin.com/linux/v6.13-rc1/source/Documentation/devicetree/bindings/dma/fsl,imx-sdma.yaml#L64

Does it make sense?

Best regards,
Hervé
Andrea della Porta Jan. 9, 2025, 3:53 p.m. UTC | #5
Hi Herve,

On 15:50 Thu 09 Jan     , Herve Codina wrote:
> Hi Andrea,
> 
> On Thu, 9 Jan 2025 15:13:42 +0100
> Andrea della Porta <andrea.porta@suse.com> wrote:
> 
> > Hi Rob,
> > 
> > On 15:08 Mon 16 Dec     , Andrea della Porta wrote:
> > > Hi Rob,
> > > 
> > > On 16:48 Tue 10 Dec     , Rob Herring wrote:  
> > > > On Mon, Dec 02, 2024 at 12:19:32PM +0100, Andrea della Porta wrote:  
> > > > > The RaspberryPi RP1 is a PCI multi function device containing
> > > > > peripherals ranging from Ethernet to USB controller, I2C, SPI
> > > > > and others.  
> > 
> > ...
> > 
> > > > > +#define RP1_INT_ADC_FIFO	52
> > > > > +#define RP1_INT_PCIE_OUT	53
> > > > > +#define RP1_INT_SPI6		54
> > > > > +#define RP1_INT_SPI7		55
> > > > > +#define RP1_INT_SPI8		56
> > > > > +#define RP1_INT_SYSCFG		58
> > > > > +#define RP1_INT_CLOCKS_DEFAULT	59
> > > > > +#define RP1_INT_VBUSCTRL	60
> > > > > +#define RP1_INT_PROC_MISC	57  
> > > > 
> > > > Why all these defines which will never be used because they come from 
> > > > DT?
> > > >  
> > > 
> > > Right, those defines where originally designed to be included from dts, but
> > > previous discussion deemed interrupt numbers to be hardcoded instead of being
> > > specified as mnemonics. In the driver source code I just use RP1_INT_END as the
> > > number of interrupts but I thought that the specific interrupt numbers should
> > > be documented in some way or another. Since no one is currently referencing
> > > those defines, would it be better to just turn those in a multiline comment
> > > just to describe them in a more compact form?  
> > 
> > So, here's a couple of proposals about the interrupt defines:
> > 
> > - since they were banned from devicetree, and are not used anywhere in the code,
> >   turn them into a (admittedly long) multiline comment, so they are still at
> >   least documented
> > 
> > - since they were banned from devicetree, and are not use anywhere in the code,
> >   just drop them, we don't currently need them after all
> > 
> > Not sure what's the best way here, anyone can advise?
> 
> Maybe in the #interrupt-cells description in the device-tree binding?
> 
> In your patch 4, you describe this interrupt controller and you have:
>   '#interrupt-cells':
>     const: 2
>     description:
>       Specifies respectively the interrupt number and flags as defined
>       in include/dt-bindings/interrupt-controller/irq.h.
> 
> In this description, why not add the supported interrupt number values?
>     description: |
>       Specifies respectively the interrupt number and flags as defined
>       in include/dt-bindings/interrupt-controller/irq.h.
>       The supported values for the interrupt number are:
>         - IO BANK0: 0
>         - IO BANK1: 1
> ...
> 
> Or something similar.
> 
> This kind of description is already available. For instance:
>   https://elixir.bootlin.com/linux/v6.13-rc1/source/Documentation/devicetree/bindings/dma/fsl,imx-sdma.yaml#L64
> 
> Does it make sense?

Seems fine to me, if there's no concern from anyone I will procede like that.
Thanks for the suggestion.

Andrea

> 
> Best regards,
> Hervé