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

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

[Andrea della Porta]

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 V6:

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

- patch 2: added: Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
- patch 3 and 4: added: 'Reviewed-by: Rob Herring (Arm) <robh@kernel.org>'
- patch 8: removed a stale paragraph from git commit message regarding
  gpio renaming through configfs


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

- interrupts definitions moved as documentation from rp1_pci.c to
  pci1de4,1.yaml binding 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   |  135 ++
 .../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                    |  305 ++++
 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, 3287 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

-- 
2.35.3

[PATCH v6 01/10] dt-bindings: clock: Add RaspberryPi RP1 clock bindings

[Andrea della Porta]

Add device tree bindings for the clock generator found in RP1 multi
function device, and relative entries in MAINTAINERS file.

Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../clock/raspberrypi,rp1-clocks.yaml         | 58 ++++++++++++++++++
 MAINTAINERS                                   |  6 ++
 .../clock/raspberrypi,rp1-clocks.h            | 61 +++++++++++++++++++
 3 files changed, 125 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml
 create mode 100644 include/dt-bindings/clock/raspberrypi,rp1-clocks.h

diff --git a/Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml b/Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml
new file mode 100644
index 000000000000..b2670cf7403a
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml
@@ -0,0 +1,58 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/raspberrypi,rp1-clocks.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: RaspberryPi RP1 clock generator
+
+maintainers:
+  - Andrea della Porta <andrea.porta@suse.com>
+
+description: |
+  The RP1 contains a clock generator designed as three PLLs (CORE, AUDIO,
+  VIDEO), and each PLL output can be programmed though dividers to generate
+  the clocks to drive the sub-peripherals embedded inside the chipset.
+
+  Link to datasheet:
+  https://datasheets.raspberrypi.com/rp1/rp1-peripherals.pdf
+
+properties:
+  compatible:
+    const: raspberrypi,rp1-clocks
+
+  reg:
+    maxItems: 1
+
+  '#clock-cells':
+    const: 1
+    description:
+      The available clocks are defined in
+      include/dt-bindings/clock/raspberrypi,rp1-clocks.h.
+
+  clocks:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - '#clock-cells'
+  - clocks
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/raspberrypi,rp1-clocks.h>
+
+    rp1 {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        clocks@c040018000 {
+            compatible = "raspberrypi,rp1-clocks";
+            reg = <0xc0 0x40018000 0x0 0x10038>;
+            #clock-cells = <1>;
+            clocks = <&clk_rp1_xosc>;
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 1e930c7a58b1..8ced4ed32c00 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19574,6 +19574,12 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.yaml
 F:	drivers/media/platform/raspberrypi/rp1-cfe/
 
+RASPBERRY PI RP1 PCI DRIVER
+M:	Andrea della Porta <andrea.porta@suse.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml
+F:	include/dt-bindings/clock/rp1.h
+
 RC-CORE / LIRC FRAMEWORK
 M:	Sean Young <sean@mess.org>
 L:	linux-media@vger.kernel.org
diff --git a/include/dt-bindings/clock/raspberrypi,rp1-clocks.h b/include/dt-bindings/clock/raspberrypi,rp1-clocks.h
new file mode 100644
index 000000000000..248efb895f35
--- /dev/null
+++ b/include/dt-bindings/clock/raspberrypi,rp1-clocks.h
@@ -0,0 +1,61 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * Copyright (C) 2021 Raspberry Pi Ltd.
+ */
+
+#ifndef __DT_BINDINGS_CLOCK_RASPBERRYPI_RP1
+#define __DT_BINDINGS_CLOCK_RASPBERRYPI_RP1
+
+#define RP1_PLL_SYS_CORE		0
+#define RP1_PLL_AUDIO_CORE		1
+#define RP1_PLL_VIDEO_CORE		2
+
+#define RP1_PLL_SYS			3
+#define RP1_PLL_AUDIO			4
+#define RP1_PLL_VIDEO			5
+
+#define RP1_PLL_SYS_PRI_PH		6
+#define RP1_PLL_SYS_SEC_PH		7
+#define RP1_PLL_AUDIO_PRI_PH		8
+
+#define RP1_PLL_SYS_SEC			9
+#define RP1_PLL_AUDIO_SEC		10
+#define RP1_PLL_VIDEO_SEC		11
+
+#define RP1_CLK_SYS			12
+#define RP1_CLK_SLOW_SYS		13
+#define RP1_CLK_DMA			14
+#define RP1_CLK_UART			15
+#define RP1_CLK_ETH			16
+#define RP1_CLK_PWM0			17
+#define RP1_CLK_PWM1			18
+#define RP1_CLK_AUDIO_IN		19
+#define RP1_CLK_AUDIO_OUT		20
+#define RP1_CLK_I2S			21
+#define RP1_CLK_MIPI0_CFG		22
+#define RP1_CLK_MIPI1_CFG		23
+#define RP1_CLK_PCIE_AUX		24
+#define RP1_CLK_USBH0_MICROFRAME	25
+#define RP1_CLK_USBH1_MICROFRAME	26
+#define RP1_CLK_USBH0_SUSPEND		27
+#define RP1_CLK_USBH1_SUSPEND		28
+#define RP1_CLK_ETH_TSU			29
+#define RP1_CLK_ADC			30
+#define RP1_CLK_SDIO_TIMER		31
+#define RP1_CLK_SDIO_ALT_SRC		32
+#define RP1_CLK_GP0			33
+#define RP1_CLK_GP1			34
+#define RP1_CLK_GP2			35
+#define RP1_CLK_GP3			36
+#define RP1_CLK_GP4			37
+#define RP1_CLK_GP5			38
+#define RP1_CLK_VEC			39
+#define RP1_CLK_DPI			40
+#define RP1_CLK_MIPI0_DPI		41
+#define RP1_CLK_MIPI1_DPI		42
+
+/* Extra PLL output channels - RP1B0 only */
+#define RP1_PLL_VIDEO_PRI_PH		43
+#define RP1_PLL_AUDIO_TERN		44
+
+#endif
-- 
2.35.3

[PATCH v6 02/10] dt-bindings: pinctrl: Add RaspberryPi RP1 gpio/pinctrl/pinmux bindings

[Andrea della Porta]

Add device tree bindings for the gpio/pin/mux controller that is part of
the RP1 multi function device, and relative entries in MAINTAINERS file.

Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 .../pinctrl/raspberrypi,rp1-gpio.yaml         | 198 ++++++++++++++++++
 MAINTAINERS                                   |   2 +
 2 files changed, 200 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml

diff --git a/Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml b/Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml
new file mode 100644
index 000000000000..9528f8675b3b
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml
@@ -0,0 +1,198 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pinctrl/raspberrypi,rp1-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: RaspberryPi RP1 GPIO/Pinconf/Pinmux Controller submodule
+
+maintainers:
+  - Andrea della Porta <andrea.porta@suse.com>
+
+description:
+  The RP1 chipset is a Multi Function Device containing, among other
+  sub-peripherals, a gpio/pinconf/mux controller whose 54 pins are grouped
+  into 3 banks.
+  It works also as an interrupt controller for those gpios.
+
+properties:
+  compatible:
+    const: raspberrypi,rp1-gpio
+
+  reg:
+    maxItems: 3
+    description: One reg specifier for each one of the 3 pin banks.
+
+  '#gpio-cells':
+    description: The first cell is the pin number and the second cell is used
+      to specify the flags (see include/dt-bindings/gpio/gpio.h).
+    const: 2
+
+  gpio-controller: true
+
+  gpio-ranges:
+    maxItems: 1
+
+  gpio-line-names:
+    maxItems: 54
+
+  interrupts:
+    maxItems: 3
+    description: One interrupt specifier for each one of the 3 pin banks.
+
+  '#interrupt-cells':
+    description:
+      Specifies the Bank number [0, 1, 2] and Flags as defined in
+      include/dt-bindings/interrupt-controller/irq.h.
+    const: 2
+
+  interrupt-controller: true
+
+patternProperties:
+  '-state$':
+    oneOf:
+      - $ref: '#/$defs/raspberrypi-rp1-state'
+      - patternProperties:
+          '-pins$':
+            $ref: '#/$defs/raspberrypi-rp1-state'
+        additionalProperties: false
+
+$defs:
+  raspberrypi-rp1-state:
+    allOf:
+      - $ref: pincfg-node.yaml#
+      - $ref: pinmux-node.yaml#
+
+    description:
+      Pin controller client devices use pin configuration subnodes (children
+      and grandchildren) for desired pin configuration.
+      Client device subnodes use below standard properties.
+
+    properties:
+      pins:
+        description:
+          List of gpio pins affected by the properties specified in this
+          subnode.
+        items:
+          pattern: '^gpio([0-9]|[1-4][0-9]|5[0-3])$'
+
+      function:
+        enum: [ alt0, alt1, alt2, alt3, alt4, gpio, alt6, alt7, alt8, none,
+                aaud, dcd0, dpi, dsi0_te_ext, dsi1_te_ext, dsr0, dtr0, gpclk0,
+                gpclk1, gpclk2, gpclk3, gpclk4, gpclk5, i2c0, i2c1, i2c2, i2c3,
+                i2c4, i2c5, i2c6, i2s0, i2s1, i2s2, ir, mic, pcie_clkreq_n,
+                pio, proc_rio, pwm0, pwm1, ri0, sd0, sd1, spi0, spi1, spi2,
+                spi3, spi4, spi5, spi6, spi7, spi8, uart0, uart1, uart2, uart3,
+                uart4, uart5, vbus0, vbus1, vbus2, vbus3 ]
+
+        description:
+          Specify the alternative function to be configured for the specified
+          pins.
+
+      bias-disable: true
+      bias-pull-down: true
+      bias-pull-up: true
+      input-enable: true
+      input-schmitt-enable: true
+      output-enable: true
+      output-high: true
+      output-low: true
+      slew-rate:
+        description: 0 is slow slew rate, 1 is fast slew rate
+        enum: [ 0, 1 ]
+      drive-strength:
+        enum: [ 2, 4, 8, 12 ]
+
+    additionalProperties: false
+
+allOf:
+  - $ref: pinctrl.yaml#
+
+required:
+  - reg
+  - compatible
+  - '#gpio-cells'
+  - gpio-controller
+  - interrupts
+  - '#interrupt-cells'
+  - interrupt-controller
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    rp1 {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        rp1_gpio: pinctrl@c0400d0000 {
+            reg = <0xc0 0x400d0000  0x0 0xc000>,
+                  <0xc0 0x400e0000  0x0 0xc000>,
+                  <0xc0 0x400f0000  0x0 0xc000>;
+            compatible = "raspberrypi,rp1-gpio";
+            gpio-controller;
+            #gpio-cells = <2>;
+            interrupt-controller;
+            #interrupt-cells = <2>;
+            interrupts = <0 IRQ_TYPE_LEVEL_HIGH>,
+                         <1 IRQ_TYPE_LEVEL_HIGH>,
+                         <2 IRQ_TYPE_LEVEL_HIGH>;
+            gpio-line-names =
+                   "ID_SDA", // GPIO0
+                   "ID_SCL", // GPIO1
+                   "GPIO2", "GPIO3", "GPIO4", "GPIO5", "GPIO6",
+                   "GPIO7", "GPIO8", "GPIO9", "GPIO10", "GPIO11",
+                   "GPIO12", "GPIO13", "GPIO14", "GPIO15", "GPIO16",
+                   "GPIO17", "GPIO18", "GPIO19", "GPIO20", "GPIO21",
+                   "GPIO22", "GPIO23", "GPIO24", "GPIO25", "GPIO26",
+                   "GPIO27",
+                   "PCIE_RP1_WAKE", // GPIO28
+                   "FAN_TACH", // GPIO29
+                   "HOST_SDA", // GPIO30
+                   "HOST_SCL", // GPIO31
+                   "ETH_RST_N", // GPIO32
+                   "", // GPIO33
+                   "CD0_IO0_MICCLK", // GPIO34
+                   "CD0_IO0_MICDAT0", // GPIO35
+                   "RP1_PCIE_CLKREQ_N", // GPIO36
+                   "", // GPIO37
+                   "CD0_SDA", // GPIO38
+                   "CD0_SCL", // GPIO39
+                   "CD1_SDA", // GPIO40
+                   "CD1_SCL", // GPIO41
+                   "USB_VBUS_EN", // GPIO42
+                   "USB_OC_N", // GPIO43
+                   "RP1_STAT_LED", // GPIO44
+                   "FAN_PWM", // GPIO45
+                   "CD1_IO0_MICCLK", // GPIO46
+                   "2712_WAKE", // GPIO47
+                   "CD1_IO1_MICDAT1", // GPIO48
+                   "EN_MAX_USB_CUR", // GPIO49
+                   "", // GPIO50
+                   "", // GPIO51
+                   "", // GPIO52
+                   ""; // GPIO53
+
+            rp1-i2s0-default-state {
+                function = "i2s0";
+                pins = "gpio18", "gpio19", "gpio20", "gpio21";
+                bias-disable;
+            };
+
+            rp1-uart0-default-state {
+                txd-pins {
+                    function = "uart0";
+                    pins = "gpio14";
+                    bias-disable;
+                };
+
+                rxd-pins {
+                    function = "uart0";
+                    pins = "gpio15";
+                    bias-pull-up;
+                };
+            };
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 8ced4ed32c00..a547e863807c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19578,7 +19578,9 @@ RASPBERRY PI RP1 PCI DRIVER
 M:	Andrea della Porta <andrea.porta@suse.com>
 S:	Maintained
 F:	Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml
+F:	Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml
 F:	include/dt-bindings/clock/rp1.h
+F:	include/dt-bindings/misc/rp1.h
 
 RC-CORE / LIRC FRAMEWORK
 M:	Sean Young <sean@mess.org>
-- 
2.35.3

[PATCH v6 03/10] dt-bindings: pci: Add common schema for devices accessible through PCI BARs

[Andrea della Porta]

Common YAML schema for devices that exports internal peripherals through
PCI BARs. The BARs are exposed as simple-buses through which the
peripherals can be accessed.

This is not intended to be used as a standalone binding, but should be
included by device specific bindings.

Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
---
 .../devicetree/bindings/pci/pci-ep-bus.yaml   | 58 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 59 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/pci-ep-bus.yaml

diff --git a/Documentation/devicetree/bindings/pci/pci-ep-bus.yaml b/Documentation/devicetree/bindings/pci/pci-ep-bus.yaml
new file mode 100644
index 000000000000..33479a5b40c6
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/pci-ep-bus.yaml
@@ -0,0 +1,58 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/pci-ep-bus.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Common Properties for PCI MFD EP with Peripherals Addressable from BARs
+
+maintainers:
+  - Andrea della Porta  <andrea.porta@suse.com>
+
+description:
+  Define a generic node representing a PCI endpoint which contains several sub-
+  peripherals. The peripherals can be accessed through one or more BARs.
+  This common schema is intended to be referenced from device tree bindings, and
+  does not represent a device tree binding by itself.
+
+properties:
+  '#address-cells':
+    const: 3
+
+  '#size-cells':
+    const: 2
+
+  ranges:
+    minItems: 1
+    maxItems: 6
+    items:
+      maxItems: 8
+      additionalItems: true
+      items:
+        - maximum: 5  # The BAR number
+        - const: 0
+        - const: 0
+
+patternProperties:
+  '^pci-ep-bus@[0-5]$':
+    type: object
+    description:
+      One node for each BAR used by peripherals contained in the PCI endpoint.
+      Each node represent a bus on which peripherals are connected.
+      This allows for some segmentation, e.g. one peripheral is accessible
+      through BAR0 and another through BAR1, and you don't want the two
+      peripherals to be able to act on the other BAR. Alternatively, when
+      different peripherals need to share BARs, you can define only one node
+      and use 'ranges' property to map all the used BARs.
+
+    additionalProperties: true
+
+    properties:
+      compatible:
+        const: simple-bus
+
+    required:
+      - compatible
+
+additionalProperties: true
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index a547e863807c..ceed86755da4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19578,6 +19578,7 @@ RASPBERRY PI RP1 PCI DRIVER
 M:	Andrea della Porta <andrea.porta@suse.com>
 S:	Maintained
 F:	Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml
+F:	Documentation/devicetree/bindings/pci/pci-ep-bus.yaml
 F:	Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml
 F:	include/dt-bindings/clock/rp1.h
 F:	include/dt-bindings/misc/rp1.h
-- 
2.35.3

[PATCH v6 04/10] dt-bindings: misc: Add device specific bindings for RaspberryPi RP1

[Andrea della Porta]

The RP1 is a MFD that exposes its peripherals through PCI BARs. This
schema is intended as minimal support for the clock generator and
gpio controller peripherals which are accessible through BAR1.

Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
---
 .../devicetree/bindings/misc/pci1de4,1.yaml   | 135 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 2 files changed, 136 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/pci1de4,1.yaml

diff --git a/Documentation/devicetree/bindings/misc/pci1de4,1.yaml b/Documentation/devicetree/bindings/misc/pci1de4,1.yaml
new file mode 100644
index 000000000000..034c7077b27d
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/pci1de4,1.yaml
@@ -0,0 +1,135 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/misc/pci1de4,1.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: RaspberryPi RP1 MFD PCI device
+
+maintainers:
+  - Andrea della Porta <andrea.porta@suse.com>
+
+description:
+  The RaspberryPi RP1 is a PCI multi function device containing
+  peripherals ranging from Ethernet to USB controller, I2C, SPI
+  and others.
+  The peripherals are accessed by addressing the PCI BAR1 region.
+
+allOf:
+  - $ref: /schemas/pci/pci-ep-bus.yaml
+
+properties:
+  compatible:
+    additionalItems: true
+    maxItems: 3
+    items:
+      - const: pci1de4,1
+
+  '#interrupt-cells':
+    const: 2
+    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
+        - IO BANK2: 2
+        - AUDIO IN: 3
+        - AUDIO OUT: 4
+        - PWM0: 5
+        - ETH: 6
+        - I2C0: 7
+        - I2C1: 8
+        - I2C2: 9
+        - I2C3: 10
+        - I2C4: 11
+        - I2C5: 12
+        - I2C6: 13
+        - I2S0: 14
+        - I2S1: 15
+        - I2S2: 16
+        - SDIO0: 17
+        - SDIO1: 18
+        - SPI0: 19
+        - SPI1: 20
+        - SPI2: 21
+        - SPI3: 22
+        - SPI4: 23
+        - SPI5: 24
+        - UART0: 25
+        - TIMER0: 26
+        - TIMER1: 27
+        - TIMER2: 28
+        - TIMER3: 29
+        - USB HOST0: 30
+        - USB HOST0-0: 31
+        - USB HOST0-1: 32
+        - USB HOST0-2: 33
+        - USB HOST0-3: 34
+        - USB HOST1: 35
+        - USB HOST1-0: 36
+        - USB HOST1-1: 37
+        - USB HOST1-2: 38
+        - USB HOST1-3: 39
+        - DMA: 40
+        - PWM1: 41
+        - UART1: 42
+        - UART2: 43
+        - UART3: 44
+        - UART4: 45
+        - UART5: 46
+        - MIPI0: 47
+        - MIPI1: 48
+        - VIDEO OUT: 49
+        - PIO0: 50
+        - PIO1: 51
+        - ADC FIFO: 52
+        - PCIE OUT: 53
+        - SPI6: 54
+        - SPI7: 55
+        - SPI8: 56
+        - PROC MISC: 57
+        - SYSCFG: 58
+        - CLOCKS DEFAULT: 59
+        - VBUSCTRL: 60
+
+  interrupt-controller: true
+
+unevaluatedProperties: false
+
+required:
+  - compatible
+  - '#interrupt-cells'
+  - interrupt-controller
+  - pci-ep-bus@1
+
+examples:
+  - |
+    pci {
+        #address-cells = <3>;
+        #size-cells = <2>;
+
+        rp1@0,0 {
+            compatible = "pci1de4,1";
+            ranges = <0x01 0x00 0x00000000  0x82010000 0x00 0x00  0x00 0x400000>;
+            #address-cells = <3>;
+            #size-cells = <2>;
+            interrupt-controller;
+            #interrupt-cells = <2>;
+
+            pci_ep_bus: pci-ep-bus@1 {
+                compatible = "simple-bus";
+                ranges = <0x00 0x40000000  0x01 0x00 0x00000000  0x00 0x00400000>;
+                dma-ranges = <0x10 0x00000000  0x43000000 0x10 0x00000000  0x10 0x00000000>;
+                #address-cells = <2>;
+                #size-cells = <2>;
+
+                rp1_clocks: clocks@40018000 {
+                    compatible = "raspberrypi,rp1-clocks";
+                    reg = <0x00 0x40018000 0x0 0x10038>;
+                    #clock-cells = <1>;
+                    clocks = <&clk_rp1_xosc>;
+                };
+            };
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index ceed86755da4..4bc77e3fa80d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19578,6 +19578,7 @@ RASPBERRY PI RP1 PCI DRIVER
 M:	Andrea della Porta <andrea.porta@suse.com>
 S:	Maintained
 F:	Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml
+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:	include/dt-bindings/clock/rp1.h
-- 
2.35.3

[PATCH v6 05/10] clk: rp1: Add support for clocks provided by RP1

[Andrea della Porta]

RaspberryPi RP1 is an MFD providing, among other peripherals, several
clock generators and PLLs that drives the sub-peripherals.
Add the driver to support the clock providers.

Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
---
 MAINTAINERS           |    1 +
 drivers/clk/Kconfig   |    9 +
 drivers/clk/Makefile  |    1 +
 drivers/clk/clk-rp1.c | 1527 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 1538 insertions(+)
 create mode 100644 drivers/clk/clk-rp1.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 4bc77e3fa80d..9ad37eb45a51 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19581,6 +19581,7 @@ F:	Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml
 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:	include/dt-bindings/clock/rp1.h
 F:	include/dt-bindings/misc/rp1.h
 
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 713573b6c86c..cff90de71409 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -88,6 +88,15 @@ config COMMON_CLK_RK808
 	  These multi-function devices have two fixed-rate oscillators, clocked at 32KHz each.
 	  Clkout1 is always on, Clkout2 can off by control register.
 
+config COMMON_CLK_RP1
+	tristate "Raspberry Pi RP1-based clock support"
+	depends on MISC_RP1 || COMPILE_TEST
+	default MISC_RP1
+	help
+	  Enable common clock framework support for Raspberry Pi RP1.
+	  This multi-function device has 3 main PLLs and several clock
+	  generators to drive the internal sub-peripherals.
+
 config COMMON_CLK_HI655X
 	tristate "Clock driver for Hi655x" if EXPERT
 	depends on (MFD_HI655X_PMIC || COMPILE_TEST)
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index bf4bd45adc3a..ff3993ed7e09 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -84,6 +84,7 @@ obj-$(CONFIG_CLK_LS1028A_PLLDIG)	+= clk-plldig.o
 obj-$(CONFIG_COMMON_CLK_PWM)		+= clk-pwm.o
 obj-$(CONFIG_CLK_QORIQ)			+= clk-qoriq.o
 obj-$(CONFIG_COMMON_CLK_RK808)		+= clk-rk808.o
+obj-$(CONFIG_COMMON_CLK_RP1)            += clk-rp1.o
 obj-$(CONFIG_COMMON_CLK_HI655X)		+= clk-hi655x.o
 obj-$(CONFIG_COMMON_CLK_S2MPS11)	+= clk-s2mps11.o
 obj-$(CONFIG_COMMON_CLK_SCMI)           += clk-scmi.o
diff --git a/drivers/clk/clk-rp1.c b/drivers/clk/clk-rp1.c
new file mode 100644
index 000000000000..d3789f5f835e
--- /dev/null
+++ b/drivers/clk/clk-rp1.c
@@ -0,0 +1,1527 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2023 Raspberry Pi Ltd.
+ *
+ * Clock driver for RP1 PCIe multifunction chip.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk-provider.h>
+#include <linux/regmap.h>
+#include <linux/math64.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/units.h>
+
+#include <dt-bindings/clock/raspberrypi,rp1-clocks.h>
+
+#define PLL_SYS_OFFSET			0x08000
+#define PLL_SYS_CS			(PLL_SYS_OFFSET + 0x00)
+#define PLL_SYS_PWR			(PLL_SYS_OFFSET + 0x04)
+#define PLL_SYS_FBDIV_INT		(PLL_SYS_OFFSET + 0x08)
+#define PLL_SYS_FBDIV_FRAC		(PLL_SYS_OFFSET + 0x0c)
+#define PLL_SYS_PRIM			(PLL_SYS_OFFSET + 0x10)
+#define PLL_SYS_SEC			(PLL_SYS_OFFSET + 0x14)
+
+#define PLL_AUDIO_OFFSET		0x0c000
+#define PLL_AUDIO_CS			(PLL_AUDIO_OFFSET + 0x00)
+#define PLL_AUDIO_PWR			(PLL_AUDIO_OFFSET + 0x04)
+#define PLL_AUDIO_FBDIV_INT		(PLL_AUDIO_OFFSET + 0x08)
+#define PLL_AUDIO_FBDIV_FRAC		(PLL_AUDIO_OFFSET + 0x0c)
+#define PLL_AUDIO_PRIM			(PLL_AUDIO_OFFSET + 0x10)
+#define PLL_AUDIO_SEC			(PLL_AUDIO_OFFSET + 0x14)
+#define PLL_AUDIO_TERN			(PLL_AUDIO_OFFSET + 0x18)
+
+#define PLL_VIDEO_OFFSET		0x10000
+#define PLL_VIDEO_CS			(PLL_VIDEO_OFFSET + 0x00)
+#define PLL_VIDEO_PWR			(PLL_VIDEO_OFFSET + 0x04)
+#define PLL_VIDEO_FBDIV_INT		(PLL_VIDEO_OFFSET + 0x08)
+#define PLL_VIDEO_FBDIV_FRAC		(PLL_VIDEO_OFFSET + 0x0c)
+#define PLL_VIDEO_PRIM			(PLL_VIDEO_OFFSET + 0x10)
+#define PLL_VIDEO_SEC			(PLL_VIDEO_OFFSET + 0x14)
+
+#define GPCLK_OE_CTRL			0x00000
+
+#define CLK_SYS_OFFSET			0x00014
+#define CLK_SYS_CTRL			(CLK_SYS_OFFSET + 0x00)
+#define CLK_SYS_DIV_INT			(CLK_SYS_OFFSET + 0x04)
+#define CLK_SYS_SEL			(CLK_SYS_OFFSET + 0x0c)
+
+#define CLK_SLOW_OFFSET			0x00024
+#define CLK_SLOW_SYS_CTRL		(CLK_SLOW_OFFSET + 0x00)
+#define CLK_SLOW_SYS_DIV_INT		(CLK_SLOW_OFFSET + 0x04)
+#define CLK_SLOW_SYS_SEL		(CLK_SLOW_OFFSET + 0x0c)
+
+#define CLK_DMA_OFFSET			0x00044
+#define CLK_DMA_CTRL			(CLK_DMA_OFFSET + 0x00)
+#define CLK_DMA_DIV_INT			(CLK_DMA_OFFSET + 0x04)
+#define CLK_DMA_SEL			(CLK_DMA_OFFSET + 0x0c)
+
+#define CLK_UART_OFFSET			0x00054
+#define CLK_UART_CTRL			(CLK_UART_OFFSET + 0x00)
+#define CLK_UART_DIV_INT		(CLK_UART_OFFSET + 0x04)
+#define CLK_UART_SEL			(CLK_UART_OFFSET + 0x0c)
+
+#define CLK_ETH_OFFSET			0x00064
+#define CLK_ETH_CTRL			(CLK_ETH_OFFSET + 0x00)
+#define CLK_ETH_DIV_INT			(CLK_ETH_OFFSET + 0x04)
+#define CLK_ETH_SEL			(CLK_ETH_OFFSET + 0x0c)
+
+#define CLK_PWM0_OFFSET			0x00074
+#define CLK_PWM0_CTRL			(CLK_PWM0_OFFSET + 0x00)
+#define CLK_PWM0_DIV_INT		(CLK_PWM0_OFFSET + 0x04)
+#define CLK_PWM0_DIV_FRAC		(CLK_PWM0_OFFSET + 0x08)
+#define CLK_PWM0_SEL			(CLK_PWM0_OFFSET + 0x0c)
+
+#define CLK_PWM1_OFFSET			0x00084
+#define CLK_PWM1_CTRL			(CLK_PWM1_OFFSET + 0x00)
+#define CLK_PWM1_DIV_INT		(CLK_PWM1_OFFSET + 0x04)
+#define CLK_PWM1_DIV_FRAC		(CLK_PWM1_OFFSET + 0x08)
+#define CLK_PWM1_SEL			(CLK_PWM1_OFFSET + 0x0c)
+
+#define CLK_AUDIO_IN_OFFSET		0x00094
+#define CLK_AUDIO_IN_CTRL		(CLK_AUDIO_IN_OFFSET + 0x00)
+#define CLK_AUDIO_IN_DIV_INT		(CLK_AUDIO_IN_OFFSET + 0x04)
+#define CLK_AUDIO_IN_SEL		(CLK_AUDIO_IN_OFFSET + 0x0c)
+
+#define CLK_AUDIO_OUT_OFFSET		0x000a4
+#define CLK_AUDIO_OUT_CTRL		(CLK_AUDIO_OUT_OFFSET + 0x00)
+#define CLK_AUDIO_OUT_DIV_INT		(CLK_AUDIO_OUT_OFFSET + 0x04)
+#define CLK_AUDIO_OUT_SEL		(CLK_AUDIO_OUT_OFFSET + 0x0c)
+
+#define CLK_I2S_OFFSET			0x000b4
+#define CLK_I2S_CTRL			(CLK_I2S_OFFSET + 0x00)
+#define CLK_I2S_DIV_INT			(CLK_I2S_OFFSET + 0x04)
+#define CLK_I2S_SEL			(CLK_I2S_OFFSET + 0x0c)
+
+#define CLK_MIPI0_CFG_OFFSET		0x000c4
+#define CLK_MIPI0_CFG_CTRL		(CLK_MIPI0_CFG_OFFSET + 0x00)
+#define CLK_MIPI0_CFG_DIV_INT		(CLK_MIPI0_CFG_OFFSET + 0x04)
+#define CLK_MIPI0_CFG_SEL		(CLK_MIPI0_CFG_OFFSET + 0x0c)
+
+#define CLK_MIPI1_CFG_OFFSET		0x000d4
+#define CLK_MIPI1_CFG_CTRL		(CLK_MIPI1_CFG_OFFSET + 0x00)
+#define CLK_MIPI1_CFG_DIV_INT		(CLK_MIPI1_CFG_OFFSET + 0x04)
+#define CLK_MIPI1_CFG_SEL		(CLK_MIPI1_CFG_OFFSET + 0x0c)
+
+#define CLK_PCIE_AUX_OFFSET		0x000e4
+#define CLK_PCIE_AUX_CTRL		(CLK_PCIE_AUX_OFFSET + 0x00)
+#define CLK_PCIE_AUX_DIV_INT		(CLK_PCIE_AUX_OFFSET + 0x04)
+#define CLK_PCIE_AUX_SEL		(CLK_PCIE_AUX_OFFSET + 0x0c)
+
+#define CLK_USBH0_MICROFRAME_OFFSET	0x000f4
+#define CLK_USBH0_MICROFRAME_CTRL	(CLK_USBH0_MICROFRAME_OFFSET + 0x00)
+#define CLK_USBH0_MICROFRAME_DIV_INT	(CLK_USBH0_MICROFRAME_OFFSET + 0x04)
+#define CLK_USBH0_MICROFRAME_SEL	(CLK_USBH0_MICROFRAME_OFFSET + 0x0c)
+
+#define CLK_USBH1_MICROFRAME_OFFSET	0x00104
+#define CLK_USBH1_MICROFRAME_CTRL	(CLK_USBH1_MICROFRAME_OFFSET + 0x00)
+#define CLK_USBH1_MICROFRAME_DIV_INT	(CLK_USBH1_MICROFRAME_OFFSET + 0x04)
+#define CLK_USBH1_MICROFRAME_SEL	(CLK_USBH1_MICROFRAME_OFFSET + 0x0c)
+
+#define CLK_USBH0_SUSPEND_OFFSET	0x00114
+#define CLK_USBH0_SUSPEND_CTRL		(CLK_USBH0_SUSPEND_OFFSET + 0x00)
+#define CLK_USBH0_SUSPEND_DIV_INT	(CLK_USBH0_SUSPEND_OFFSET + 0x04)
+#define CLK_USBH0_SUSPEND_SEL		(CLK_USBH0_SUSPEND_OFFSET + 0x0c)
+
+#define CLK_USBH1_SUSPEND_OFFSET	0x00124
+#define CLK_USBH1_SUSPEND_CTRL		(CLK_USBH1_SUSPEND_OFFSET + 0x00)
+#define CLK_USBH1_SUSPEND_DIV_INT	(CLK_USBH1_SUSPEND_OFFSET + 0x04)
+#define CLK_USBH1_SUSPEND_SEL		(CLK_USBH1_SUSPEND_OFFSET + 0x0c)
+
+#define CLK_ETH_TSU_OFFSET		0x00134
+#define CLK_ETH_TSU_CTRL		(CLK_ETH_TSU_OFFSET + 0x00)
+#define CLK_ETH_TSU_DIV_INT		(CLK_ETH_TSU_OFFSET + 0x04)
+#define CLK_ETH_TSU_SEL			(CLK_ETH_TSU_OFFSET + 0x0c)
+
+#define CLK_ADC_OFFSET			0x00144
+#define CLK_ADC_CTRL			(CLK_ADC_OFFSET + 0x00)
+#define CLK_ADC_DIV_INT			(CLK_ADC_OFFSET + 0x04)
+#define CLK_ADC_SEL			(CLK_ADC_OFFSET + 0x0c)
+
+#define CLK_SDIO_TIMER_OFFSET		0x00154
+#define CLK_SDIO_TIMER_CTRL		(CLK_SDIO_TIMER_OFFSET + 0x00)
+#define CLK_SDIO_TIMER_DIV_INT		(CLK_SDIO_TIMER_OFFSET + 0x04)
+#define CLK_SDIO_TIMER_SEL		(CLK_SDIO_TIMER_OFFSET + 0x0c)
+
+#define CLK_SDIO_ALT_SRC_OFFSET		0x00164
+#define CLK_SDIO_ALT_SRC_CTRL		(CLK_SDIO_ALT_SRC_OFFSET + 0x00)
+#define CLK_SDIO_ALT_SRC_DIV_INT	(CLK_SDIO_ALT_SRC_OFFSET + 0x04)
+#define CLK_SDIO_ALT_SRC_SEL		(CLK_SDIO_ALT_SRC_OFFSET + 0x0c)
+
+#define CLK_GP0_OFFSET			0x00174
+#define CLK_GP0_CTRL			(CLK_GP0_OFFSET + 0x00)
+#define CLK_GP0_DIV_INT			(CLK_GP0_OFFSET + 0x04)
+#define CLK_GP0_DIV_FRAC		(CLK_GP0_OFFSET + 0x08)
+#define CLK_GP0_SEL			(CLK_GP0_OFFSET + 0x0c)
+
+#define CLK_GP1_OFFSET			0x00184
+#define CLK_GP1_CTRL			(CLK_GP1_OFFSET + 0x00)
+#define CLK_GP1_DIV_INT			(CLK_GP1_OFFSET + 0x04)
+#define CLK_GP1_DIV_FRAC		(CLK_GP1_OFFSET + 0x08)
+#define CLK_GP1_SEL			(CLK_GP1_OFFSET + 0x0c)
+
+#define CLK_GP2_OFFSET			0x00194
+#define CLK_GP2_CTRL			(CLK_GP2_OFFSET + 0x00)
+#define CLK_GP2_DIV_INT			(CLK_GP2_OFFSET + 0x04)
+#define CLK_GP2_DIV_FRAC		(CLK_GP2_OFFSET + 0x08)
+#define CLK_GP2_SEL			(CLK_GP2_OFFSET + 0x0c)
+
+#define CLK_GP3_OFFSET			0x001a4
+#define CLK_GP3_CTRL			(CLK_GP3_OFFSET + 0x00)
+#define CLK_GP3_DIV_INT			(CLK_GP3_OFFSET + 0x04)
+#define CLK_GP3_DIV_FRAC		(CLK_GP3_OFFSET + 0x08)
+#define CLK_GP3_SEL			(CLK_GP3_OFFSET + 0x0c)
+
+#define CLK_GP4_OFFSET			0x001b4
+#define CLK_GP4_CTRL			(CLK_GP4_OFFSET + 0x00)
+#define CLK_GP4_DIV_INT			(CLK_GP4_OFFSET + 0x04)
+#define CLK_GP4_DIV_FRAC		(CLK_GP4_OFFSET + 0x08)
+#define CLK_GP4_SEL			(CLK_GP4_OFFSET + 0x0c)
+
+#define CLK_GP5_OFFSET			0x001c4
+#define CLK_GP5_CTRL			(CLK_GP5_OFFSET + 0x00)
+#define CLK_GP5_DIV_INT			(CLK_GP5_OFFSET + 0x04)
+#define CLK_GP5_DIV_FRAC		(CLK_GP5_OFFSET + 0x08)
+#define CLK_GP5_SEL			(CLK_GP5_OFFSET + 0x0c)
+
+#define CLK_SYS_RESUS_CTRL		0x0020c
+
+#define CLK_SLOW_SYS_RESUS_CTRL		0x00214
+
+#define FC0_OFFSET			0x0021c
+#define FC0_REF_KHZ			(FC0_OFFSET + 0x00)
+#define FC0_MIN_KHZ			(FC0_OFFSET + 0x04)
+#define FC0_MAX_KHZ			(FC0_OFFSET + 0x08)
+#define FC0_DELAY			(FC0_OFFSET + 0x0c)
+#define FC0_INTERVAL			(FC0_OFFSET + 0x10)
+#define FC0_SRC				(FC0_OFFSET + 0x14)
+#define FC0_STATUS			(FC0_OFFSET + 0x18)
+#define FC0_RESULT			(FC0_OFFSET + 0x1c)
+#define FC_SIZE				0x20
+#define FC_COUNT			8
+#define FC_NUM(idx, off)		((idx) * 32 + (off))
+
+#define AUX_SEL				1
+
+#define VIDEO_CLOCKS_OFFSET		0x4000
+#define VIDEO_CLK_VEC_CTRL		(VIDEO_CLOCKS_OFFSET + 0x0000)
+#define VIDEO_CLK_VEC_DIV_INT		(VIDEO_CLOCKS_OFFSET + 0x0004)
+#define VIDEO_CLK_VEC_SEL		(VIDEO_CLOCKS_OFFSET + 0x000c)
+#define VIDEO_CLK_DPI_CTRL		(VIDEO_CLOCKS_OFFSET + 0x0010)
+#define VIDEO_CLK_DPI_DIV_INT		(VIDEO_CLOCKS_OFFSET + 0x0014)
+#define VIDEO_CLK_DPI_SEL		(VIDEO_CLOCKS_OFFSET + 0x001c)
+#define VIDEO_CLK_MIPI0_DPI_CTRL	(VIDEO_CLOCKS_OFFSET + 0x0020)
+#define VIDEO_CLK_MIPI0_DPI_DIV_INT	(VIDEO_CLOCKS_OFFSET + 0x0024)
+#define VIDEO_CLK_MIPI0_DPI_DIV_FRAC	(VIDEO_CLOCKS_OFFSET + 0x0028)
+#define VIDEO_CLK_MIPI0_DPI_SEL		(VIDEO_CLOCKS_OFFSET + 0x002c)
+#define VIDEO_CLK_MIPI1_DPI_CTRL	(VIDEO_CLOCKS_OFFSET + 0x0030)
+#define VIDEO_CLK_MIPI1_DPI_DIV_INT	(VIDEO_CLOCKS_OFFSET + 0x0034)
+#define VIDEO_CLK_MIPI1_DPI_DIV_FRAC	(VIDEO_CLOCKS_OFFSET + 0x0038)
+#define VIDEO_CLK_MIPI1_DPI_SEL		(VIDEO_CLOCKS_OFFSET + 0x003c)
+
+#define DIV_INT_8BIT_MAX		GENMASK(7, 0)	/* max divide for most clocks */
+#define DIV_INT_16BIT_MAX		GENMASK(15, 0)	/* max divide for GPx, PWM */
+#define DIV_INT_24BIT_MAX               GENMASK(23, 0)	/* max divide for CLK_SYS */
+
+#define FC0_STATUS_DONE			BIT(4)
+#define FC0_STATUS_RUNNING		BIT(8)
+#define FC0_RESULT_FRAC_SHIFT		5
+
+#define PLL_PRIM_DIV1_SHIFT		16
+#define PLL_PRIM_DIV1_WIDTH		3
+#define PLL_PRIM_DIV1_MASK		GENMASK(PLL_PRIM_DIV1_SHIFT + \
+						PLL_PRIM_DIV1_WIDTH - 1, \
+						PLL_PRIM_DIV1_SHIFT)
+#define PLL_PRIM_DIV2_SHIFT		12
+#define PLL_PRIM_DIV2_WIDTH		3
+#define PLL_PRIM_DIV2_MASK		GENMASK(PLL_PRIM_DIV2_SHIFT + \
+						PLL_PRIM_DIV2_WIDTH - 1, \
+						PLL_PRIM_DIV2_SHIFT)
+
+#define PLL_SEC_DIV_SHIFT		8
+#define PLL_SEC_DIV_WIDTH		5
+#define PLL_SEC_DIV_MASK		GENMASK(PLL_SEC_DIV_SHIFT + \
+						PLL_SEC_DIV_WIDTH - 1, \
+						PLL_SEC_DIV_SHIFT)
+
+#define PLL_CS_LOCK			BIT(31)
+#define PLL_CS_REFDIV_SHIFT		0
+
+#define PLL_PWR_PD			BIT(0)
+#define PLL_PWR_DACPD			BIT(1)
+#define PLL_PWR_DSMPD			BIT(2)
+#define PLL_PWR_POSTDIVPD		BIT(3)
+#define PLL_PWR_4PHASEPD		BIT(4)
+#define PLL_PWR_VCOPD			BIT(5)
+#define PLL_PWR_MASK			GENMASK(5, 0)
+
+#define PLL_SEC_RST			BIT(16)
+#define PLL_SEC_IMPL			BIT(31)
+
+/* PLL phase output for both PRI and SEC */
+#define PLL_PH_EN			BIT(4)
+#define PLL_PH_PHASE_SHIFT		0
+
+#define RP1_PLL_PHASE_0			0
+#define RP1_PLL_PHASE_90		1
+#define RP1_PLL_PHASE_180		2
+#define RP1_PLL_PHASE_270		3
+
+/* Clock fields for all clocks */
+#define CLK_CTRL_ENABLE			BIT(11)
+#define CLK_CTRL_AUXSRC_SHIFT		5
+#define CLK_CTRL_AUXSRC_WIDTH		5
+#define CLK_CTRL_AUXSRC_MASK		GENMASK(CLK_CTRL_AUXSRC_SHIFT + \
+						CLK_CTRL_AUXSRC_WIDTH - 1, \
+						CLK_CTRL_AUXSRC_SHIFT)
+#define CLK_CTRL_SRC_SHIFT		0
+#define CLK_DIV_FRAC_BITS		16
+
+#define LOCK_TIMEOUT_US			100000
+#define LOCK_POLL_DELAY_US		5
+
+#define MAX_CLK_PARENTS			16
+
+/*
+ * Secondary PLL channel output divider table.
+ * Divider values range from 8 to 19.
+ * Invalid values default to 19
+ */
+static const struct clk_div_table pll_sec_div_table[] = {
+	{ 0x00, 19 },
+	{ 0x01, 19 },
+	{ 0x02, 19 },
+	{ 0x03, 19 },
+	{ 0x04, 19 },
+	{ 0x05, 19 },
+	{ 0x06, 19 },
+	{ 0x07, 19 },
+	{ 0x08,  8 },
+	{ 0x09,  9 },
+	{ 0x0a, 10 },
+	{ 0x0b, 11 },
+	{ 0x0c, 12 },
+	{ 0x0d, 13 },
+	{ 0x0e, 14 },
+	{ 0x0f, 15 },
+	{ 0x10, 16 },
+	{ 0x11, 17 },
+	{ 0x12, 18 },
+	{ 0x13, 19 },
+	{ 0x14, 19 },
+	{ 0x15, 19 },
+	{ 0x16, 19 },
+	{ 0x17, 19 },
+	{ 0x18, 19 },
+	{ 0x19, 19 },
+	{ 0x1a, 19 },
+	{ 0x1b, 19 },
+	{ 0x1c, 19 },
+	{ 0x1d, 19 },
+	{ 0x1e, 19 },
+	{ 0x1f, 19 },
+	{ 0 }
+};
+
+struct rp1_clockman {
+	struct device *dev;
+	void __iomem *regs;
+	struct regmap *regmap;
+	spinlock_t regs_lock; /* spinlock for all clocks */
+
+	/* Must be last */
+	struct clk_hw_onecell_data onecell;
+};
+
+struct rp1_pll_core_data {
+	u32 cs_reg;
+	u32 pwr_reg;
+	u32 fbdiv_int_reg;
+	u32 fbdiv_frac_reg;
+	u32 fc0_src;
+};
+
+struct rp1_pll_data {
+	u32 ctrl_reg;
+	u32 fc0_src;
+};
+
+struct rp1_pll_ph_data {
+	unsigned int phase;
+	unsigned int fixed_divider;
+	u32 ph_reg;
+	u32 fc0_src;
+};
+
+struct rp1_pll_divider_data {
+	u32 sec_reg;
+	u32 fc0_src;
+};
+
+struct rp1_clock_data {
+	int num_std_parents;
+	int num_aux_parents;
+	u32 oe_mask;
+	u32 clk_src_mask;
+	u32 ctrl_reg;
+	u32 div_int_reg;
+	u32 div_frac_reg;
+	u32 sel_reg;
+	u32 div_int_max;
+	unsigned long max_freq;
+	u32 fc0_src;
+};
+
+struct rp1_clk_desc {
+	struct clk_hw *(*clk_register)(struct rp1_clockman *clockman,
+				       struct rp1_clk_desc *desc);
+	const void *data;
+	struct clk_hw hw;
+	struct rp1_clockman *clockman;
+	unsigned long cached_rate;
+	struct clk_divider div;
+};
+
+static inline
+void clockman_write(struct rp1_clockman *clockman, u32 reg, u32 val)
+{
+	regmap_write(clockman->regmap, reg, val);
+}
+
+static inline u32 clockman_read(struct rp1_clockman *clockman, u32 reg)
+{
+	u32 val;
+
+	regmap_read(clockman->regmap, reg, &val);
+
+	return val;
+}
+
+static int rp1_pll_core_is_on(struct clk_hw *hw)
+{
+	struct rp1_clk_desc *pll_core = container_of(hw, struct rp1_clk_desc, hw);
+	struct rp1_clockman *clockman = pll_core->clockman;
+	const struct rp1_pll_core_data *data = pll_core->data;
+
+	u32 pwr = clockman_read(clockman, data->pwr_reg);
+
+	return (pwr & PLL_PWR_PD) || (pwr & PLL_PWR_POSTDIVPD);
+}
+
+static int rp1_pll_core_on(struct clk_hw *hw)
+{
+	struct rp1_clk_desc *pll_core = container_of(hw, struct rp1_clk_desc, hw);
+	struct rp1_clockman *clockman = pll_core->clockman;
+	const struct rp1_pll_core_data *data = pll_core->data;
+
+	u32 fbdiv_frac, val;
+	int ret;
+
+	spin_lock(&clockman->regs_lock);
+
+	if (!(clockman_read(clockman, data->cs_reg) & PLL_CS_LOCK)) {
+		/* Reset to a known state. */
+		clockman_write(clockman, data->pwr_reg, PLL_PWR_MASK);
+		clockman_write(clockman, data->fbdiv_int_reg, 20);
+		clockman_write(clockman, data->fbdiv_frac_reg, 0);
+		clockman_write(clockman, data->cs_reg, 1 << PLL_CS_REFDIV_SHIFT);
+	}
+
+	/* Come out of reset. */
+	fbdiv_frac = clockman_read(clockman, data->fbdiv_frac_reg);
+	clockman_write(clockman, data->pwr_reg, fbdiv_frac ? 0 : PLL_PWR_DSMPD);
+	spin_unlock(&clockman->regs_lock);
+
+	/* Wait for the PLL to lock. */
+	ret = regmap_read_poll_timeout(clockman->regmap, data->cs_reg, val,
+				       val & PLL_CS_LOCK,
+				       LOCK_POLL_DELAY_US, LOCK_TIMEOUT_US);
+	if (ret)
+		dev_err(clockman->dev, "%s: can't lock PLL\n",
+			clk_hw_get_name(hw));
+
+	return ret;
+}
+
+static void rp1_pll_core_off(struct clk_hw *hw)
+{
+	struct rp1_clk_desc *pll_core = container_of(hw, struct rp1_clk_desc, hw);
+	struct rp1_clockman *clockman = pll_core->clockman;
+	const struct rp1_pll_core_data *data = pll_core->data;
+
+	spin_lock(&clockman->regs_lock);
+	clockman_write(clockman, data->pwr_reg, 0);
+	spin_unlock(&clockman->regs_lock);
+}
+
+static inline unsigned long get_pll_core_divider(struct clk_hw *hw,
+						 unsigned long rate,
+						 unsigned long parent_rate,
+						 u32 *div_int, u32 *div_frac)
+{
+	u32 fbdiv_int, fbdiv_frac;
+	unsigned long calc_rate;
+	u64 shifted_fbdiv_int;
+	u64 div_fp64; /* 32.32 fixed point fraction. */
+
+	/* Factor of reference clock to VCO frequency. */
+	div_fp64 = (u64)(rate) << 32;
+	div_fp64 = DIV_ROUND_CLOSEST_ULL(div_fp64, parent_rate);
+
+	/* Round the fractional component at 24 bits. */
+	div_fp64 += 1 << (32 - 24 - 1);
+
+	fbdiv_int = div_fp64 >> 32;
+	fbdiv_frac = (div_fp64 >> (32 - 24)) & 0xffffff;
+
+	shifted_fbdiv_int = (u64)fbdiv_int << 24;
+	calc_rate = (u64)parent_rate * (shifted_fbdiv_int + fbdiv_frac);
+	calc_rate += BIT(23);
+	calc_rate >>= 24;
+
+	*div_int = fbdiv_int;
+	*div_frac = fbdiv_frac;
+
+	return calc_rate;
+}
+
+static int rp1_pll_core_set_rate(struct clk_hw *hw,
+				 unsigned long rate, unsigned long parent_rate)
+{
+	struct rp1_clk_desc *pll_core = container_of(hw, struct rp1_clk_desc, hw);
+	struct rp1_clockman *clockman = pll_core->clockman;
+	const struct rp1_pll_core_data *data = pll_core->data;
+
+	unsigned long calc_rate;
+	u32 fbdiv_int, fbdiv_frac;
+
+	/* Disable dividers to start with. */
+	spin_lock(&clockman->regs_lock);
+	clockman_write(clockman, data->fbdiv_int_reg, 0);
+	clockman_write(clockman, data->fbdiv_frac_reg, 0);
+	spin_unlock(&clockman->regs_lock);
+
+	calc_rate = get_pll_core_divider(hw, rate, parent_rate,
+					 &fbdiv_int, &fbdiv_frac);
+
+	spin_lock(&clockman->regs_lock);
+	clockman_write(clockman, data->pwr_reg, fbdiv_frac ? 0 : PLL_PWR_DSMPD);
+	clockman_write(clockman, data->fbdiv_int_reg, fbdiv_int);
+	clockman_write(clockman, data->fbdiv_frac_reg, fbdiv_frac);
+	spin_unlock(&clockman->regs_lock);
+
+	/* Check that reference frequency is no greater than VCO / 16. */
+	if (WARN_ON_ONCE(parent_rate > (rate / 16)))
+		return -ERANGE;
+
+	pll_core->cached_rate = calc_rate;
+
+	spin_lock(&clockman->regs_lock);
+	/* Don't need to divide ref unless parent_rate > (output freq / 16) */
+	clockman_write(clockman, data->cs_reg,
+		       clockman_read(clockman, data->cs_reg) |
+				     (1 << PLL_CS_REFDIV_SHIFT));
+	spin_unlock(&clockman->regs_lock);
+
+	return 0;
+}
+
+static unsigned long rp1_pll_core_recalc_rate(struct clk_hw *hw,
+					      unsigned long parent_rate)
+{
+	struct rp1_clk_desc *pll_core = container_of(hw, struct rp1_clk_desc, hw);
+	struct rp1_clockman *clockman = pll_core->clockman;
+	const struct rp1_pll_core_data *data = pll_core->data;
+
+	u32 fbdiv_int, fbdiv_frac;
+	unsigned long calc_rate;
+	u64 shifted_fbdiv_int;
+
+	fbdiv_int = clockman_read(clockman, data->fbdiv_int_reg);
+	fbdiv_frac = clockman_read(clockman, data->fbdiv_frac_reg);
+
+	shifted_fbdiv_int = (u64)fbdiv_int << 24;
+	calc_rate = (u64)parent_rate * (shifted_fbdiv_int + fbdiv_frac);
+	calc_rate += BIT(23);
+	calc_rate >>= 24;
+
+	return calc_rate;
+}
+
+static long rp1_pll_core_round_rate(struct clk_hw *hw, unsigned long rate,
+				    unsigned long *parent_rate)
+{
+	u32 fbdiv_int, fbdiv_frac;
+
+	return get_pll_core_divider(hw, rate, *parent_rate,
+				    &fbdiv_int, &fbdiv_frac);
+}
+
+static void get_pll_prim_dividers(unsigned long rate, unsigned long parent_rate,
+				  u32 *divider1, u32 *divider2)
+{
+	unsigned int div1, div2;
+	unsigned int best_div1 = 7, best_div2 = 7;
+	unsigned long best_rate_diff =
+		abs_diff(DIV_ROUND_CLOSEST(parent_rate, best_div1 * best_div2), rate);
+	unsigned long rate_diff, calc_rate;
+
+	for (div1 = 1; div1 <= 7; div1++) {
+		for (div2 = 1; div2 <= div1; div2++) {
+			calc_rate = DIV_ROUND_CLOSEST(parent_rate, div1 * div2);
+			rate_diff = abs_diff(calc_rate, rate);
+
+			if (calc_rate == rate) {
+				best_div1 = div1;
+				best_div2 = div2;
+				goto done;
+			} else if (rate_diff < best_rate_diff) {
+				best_div1 = div1;
+				best_div2 = div2;
+				best_rate_diff = rate_diff;
+			}
+		}
+	}
+
+done:
+	*divider1 = best_div1;
+	*divider2 = best_div2;
+}
+
+static int rp1_pll_set_rate(struct clk_hw *hw,
+			    unsigned long rate, unsigned long parent_rate)
+{
+	struct rp1_clk_desc *pll = container_of(hw, struct rp1_clk_desc, hw);
+	struct rp1_clockman *clockman = pll->clockman;
+	const struct rp1_pll_data *data = pll->data;
+
+	u32 prim, prim_div1, prim_div2;
+
+	get_pll_prim_dividers(rate, parent_rate, &prim_div1, &prim_div2);
+
+	spin_lock(&clockman->regs_lock);
+	prim = clockman_read(clockman, data->ctrl_reg);
+	prim &= ~PLL_PRIM_DIV1_MASK;
+	prim |= FIELD_PREP(PLL_PRIM_DIV1_MASK, prim_div1);
+	prim &= ~PLL_PRIM_DIV2_MASK;
+	prim |= FIELD_PREP(PLL_PRIM_DIV2_MASK, prim_div2);
+	clockman_write(clockman, data->ctrl_reg, prim);
+	spin_unlock(&clockman->regs_lock);
+
+	return 0;
+}
+
+static unsigned long rp1_pll_recalc_rate(struct clk_hw *hw,
+					 unsigned long parent_rate)
+{
+	struct rp1_clk_desc *pll = container_of(hw, struct rp1_clk_desc, hw);
+	struct rp1_clockman *clockman = pll->clockman;
+	const struct rp1_pll_data *data = pll->data;
+	u32 prim, prim_div1, prim_div2;
+
+	prim = clockman_read(clockman, data->ctrl_reg);
+	prim_div1 = (prim & PLL_PRIM_DIV1_MASK) >> PLL_PRIM_DIV1_SHIFT;
+	prim_div2 = (prim & PLL_PRIM_DIV2_MASK) >> PLL_PRIM_DIV2_SHIFT;
+
+	if (!prim_div1 || !prim_div2) {
+		dev_err(clockman->dev, "%s: (%s) zero divider value\n",
+			__func__, clk_hw_get_name(hw));
+		return 0;
+	}
+
+	return DIV_ROUND_CLOSEST(parent_rate, prim_div1 * prim_div2);
+}
+
+static long rp1_pll_round_rate(struct clk_hw *hw, unsigned long rate,
+			       unsigned long *parent_rate)
+{
+	u32 div1, div2;
+
+	get_pll_prim_dividers(rate, *parent_rate, &div1, &div2);
+
+	return DIV_ROUND_CLOSEST(*parent_rate, div1 * div2);
+}
+
+static int rp1_pll_ph_is_on(struct clk_hw *hw)
+{
+	struct rp1_clk_desc *pll_ph = container_of(hw, struct rp1_clk_desc, hw);
+	struct rp1_clockman *clockman = pll_ph->clockman;
+	const struct rp1_pll_ph_data *data = pll_ph->data;
+
+	return !!(clockman_read(clockman, data->ph_reg) & PLL_PH_EN);
+}
+
+static int rp1_pll_ph_on(struct clk_hw *hw)
+{
+	struct rp1_clk_desc *pll_ph = container_of(hw, struct rp1_clk_desc, hw);
+	struct rp1_clockman *clockman = pll_ph->clockman;
+	const struct rp1_pll_ph_data *data = pll_ph->data;
+	u32 ph_reg;
+
+	/* TODO: ensure pri/sec is enabled! */
+	spin_lock(&clockman->regs_lock);
+	ph_reg = clockman_read(clockman, data->ph_reg);
+	ph_reg |= data->phase << PLL_PH_PHASE_SHIFT;
+	ph_reg |= PLL_PH_EN;
+	clockman_write(clockman, data->ph_reg, ph_reg);
+	spin_unlock(&clockman->regs_lock);
+
+	return 0;
+}
+
+static void rp1_pll_ph_off(struct clk_hw *hw)
+{
+	struct rp1_clk_desc *pll_ph = container_of(hw, struct rp1_clk_desc, hw);
+	struct rp1_clockman *clockman = pll_ph->clockman;
+	const struct rp1_pll_ph_data *data = pll_ph->data;
+
+	spin_lock(&clockman->regs_lock);
+	clockman_write(clockman, data->ph_reg,
+		       clockman_read(clockman, data->ph_reg) & ~PLL_PH_EN);
+	spin_unlock(&clockman->regs_lock);
+}
+
+static unsigned long rp1_pll_ph_recalc_rate(struct clk_hw *hw,
+					    unsigned long parent_rate)
+{
+	struct rp1_clk_desc *pll_ph = container_of(hw, struct rp1_clk_desc, hw);
+	const struct rp1_pll_ph_data *data = pll_ph->data;
+
+	return parent_rate / data->fixed_divider;
+}
+
+static long rp1_pll_ph_round_rate(struct clk_hw *hw, unsigned long rate,
+				  unsigned long *parent_rate)
+{
+	struct rp1_clk_desc *pll_ph = container_of(hw, struct rp1_clk_desc, hw);
+	const struct rp1_pll_ph_data *data = pll_ph->data;
+
+	return *parent_rate / data->fixed_divider;
+}
+
+static int rp1_pll_divider_is_on(struct clk_hw *hw)
+{
+	struct rp1_clk_desc *divider = container_of(hw, struct rp1_clk_desc, div.hw);
+	struct rp1_clockman *clockman = divider->clockman;
+	const struct rp1_pll_data *data = divider->data;
+
+	return !(clockman_read(clockman, data->ctrl_reg) & PLL_SEC_RST);
+}
+
+static int rp1_pll_divider_on(struct clk_hw *hw)
+{
+	struct rp1_clk_desc *divider = container_of(hw, struct rp1_clk_desc, div.hw);
+	struct rp1_clockman *clockman = divider->clockman;
+	const struct rp1_pll_data *data = divider->data;
+
+	spin_lock(&clockman->regs_lock);
+	/* Check the implementation bit is set! */
+	WARN_ON(!(clockman_read(clockman, data->ctrl_reg) & PLL_SEC_IMPL));
+	clockman_write(clockman, data->ctrl_reg,
+		       clockman_read(clockman, data->ctrl_reg) & ~PLL_SEC_RST);
+	spin_unlock(&clockman->regs_lock);
+
+	return 0;
+}
+
+static void rp1_pll_divider_off(struct clk_hw *hw)
+{
+	struct rp1_clk_desc *divider = container_of(hw, struct rp1_clk_desc, div.hw);
+	struct rp1_clockman *clockman = divider->clockman;
+	const struct rp1_pll_data *data = divider->data;
+
+	spin_lock(&clockman->regs_lock);
+	clockman_write(clockman, data->ctrl_reg,
+		       clockman_read(clockman, data->ctrl_reg) | PLL_SEC_RST);
+	spin_unlock(&clockman->regs_lock);
+}
+
+static int rp1_pll_divider_set_rate(struct clk_hw *hw,
+				    unsigned long rate,
+				    unsigned long parent_rate)
+{
+	struct rp1_clk_desc *divider = container_of(hw, struct rp1_clk_desc, div.hw);
+	struct rp1_clockman *clockman = divider->clockman;
+	const struct rp1_pll_data *data = divider->data;
+	u32 div, sec;
+
+	div = DIV_ROUND_UP_ULL(parent_rate, rate);
+	div = clamp(div, 8u, 19u);
+
+	spin_lock(&clockman->regs_lock);
+	sec = clockman_read(clockman, data->ctrl_reg);
+	sec &= ~PLL_SEC_DIV_MASK;
+	sec |= FIELD_PREP(PLL_SEC_DIV_MASK, div);
+
+	/* Must keep the divider in reset to change the value. */
+	sec |= PLL_SEC_RST;
+	clockman_write(clockman, data->ctrl_reg, sec);
+
+	/* TODO: must sleep 10 pll vco cycles */
+	sec &= ~PLL_SEC_RST;
+	clockman_write(clockman, data->ctrl_reg, sec);
+	spin_unlock(&clockman->regs_lock);
+
+	return 0;
+}
+
+static unsigned long rp1_pll_divider_recalc_rate(struct clk_hw *hw,
+						 unsigned long parent_rate)
+{
+	return clk_divider_ops.recalc_rate(hw, parent_rate);
+}
+
+static long rp1_pll_divider_round_rate(struct clk_hw *hw,
+				       unsigned long rate,
+				       unsigned long *parent_rate)
+{
+	return clk_divider_ops.round_rate(hw, rate, parent_rate);
+}
+
+static int rp1_clock_is_on(struct clk_hw *hw)
+{
+	struct rp1_clk_desc *clock = container_of(hw, struct rp1_clk_desc, hw);
+	struct rp1_clockman *clockman = clock->clockman;
+	const struct rp1_clock_data *data = clock->data;
+
+	return !!(clockman_read(clockman, data->ctrl_reg) & CLK_CTRL_ENABLE);
+}
+
+static unsigned long rp1_clock_recalc_rate(struct clk_hw *hw,
+					   unsigned long parent_rate)
+{
+	struct rp1_clk_desc *clock = container_of(hw, struct rp1_clk_desc, hw);
+	struct rp1_clockman *clockman = clock->clockman;
+	const struct rp1_clock_data *data = clock->data;
+	u64 calc_rate;
+	u64 div;
+
+	u32 frac;
+
+	div = clockman_read(clockman, data->div_int_reg);
+	frac = (data->div_frac_reg != 0) ?
+		clockman_read(clockman, data->div_frac_reg) : 0;
+
+	/* If the integer portion of the divider is 0, treat it as 2^16 */
+	if (!div)
+		div = 1 << 16;
+
+	div = (div << CLK_DIV_FRAC_BITS) | (frac >> (32 - CLK_DIV_FRAC_BITS));
+
+	calc_rate = (u64)parent_rate << CLK_DIV_FRAC_BITS;
+	calc_rate = div64_u64(calc_rate, div);
+
+	return calc_rate;
+}
+
+static int rp1_clock_on(struct clk_hw *hw)
+{
+	struct rp1_clk_desc *clock = container_of(hw, struct rp1_clk_desc, hw);
+	struct rp1_clockman *clockman = clock->clockman;
+	const struct rp1_clock_data *data = clock->data;
+
+	spin_lock(&clockman->regs_lock);
+	clockman_write(clockman, data->ctrl_reg,
+		       clockman_read(clockman, data->ctrl_reg) | CLK_CTRL_ENABLE);
+	/* If this is a GPCLK, turn on the output-enable */
+	if (data->oe_mask)
+		clockman_write(clockman, GPCLK_OE_CTRL,
+			       clockman_read(clockman, GPCLK_OE_CTRL) | data->oe_mask);
+	spin_unlock(&clockman->regs_lock);
+
+	return 0;
+}
+
+static void rp1_clock_off(struct clk_hw *hw)
+{
+	struct rp1_clk_desc *clock = container_of(hw, struct rp1_clk_desc, hw);
+	struct rp1_clockman *clockman = clock->clockman;
+	const struct rp1_clock_data *data = clock->data;
+
+	spin_lock(&clockman->regs_lock);
+	clockman_write(clockman, data->ctrl_reg,
+		       clockman_read(clockman, data->ctrl_reg) & ~CLK_CTRL_ENABLE);
+	/* If this is a GPCLK, turn off the output-enable */
+	if (data->oe_mask)
+		clockman_write(clockman, GPCLK_OE_CTRL,
+			       clockman_read(clockman, GPCLK_OE_CTRL) & ~data->oe_mask);
+	spin_unlock(&clockman->regs_lock);
+}
+
+static u32 rp1_clock_choose_div(unsigned long rate, unsigned long parent_rate,
+				const struct rp1_clock_data *data)
+{
+	u64 div;
+
+	/*
+	 * Due to earlier rounding, calculated parent_rate may differ from
+	 * expected value. Don't fail on a small discrepancy near unity divide.
+	 */
+	if (!rate || rate > parent_rate + (parent_rate >> CLK_DIV_FRAC_BITS))
+		return 0;
+
+	/*
+	 * Always express div in fixed-point format for fractional division;
+	 * If no fractional divider is present, the fraction part will be zero.
+	 */
+	if (data->div_frac_reg) {
+		div = (u64)parent_rate << CLK_DIV_FRAC_BITS;
+		div = DIV_ROUND_CLOSEST_ULL(div, rate);
+	} else {
+		div = DIV_ROUND_CLOSEST_ULL(parent_rate, rate);
+		div <<= CLK_DIV_FRAC_BITS;
+	}
+
+	div = clamp(div,
+		    1ull << CLK_DIV_FRAC_BITS,
+		    (u64)data->div_int_max << CLK_DIV_FRAC_BITS);
+
+	return div;
+}
+
+static u8 rp1_clock_get_parent(struct clk_hw *hw)
+{
+	struct rp1_clk_desc *clock = container_of(hw, struct rp1_clk_desc, hw);
+	struct rp1_clockman *clockman = clock->clockman;
+	const struct rp1_clock_data *data = clock->data;
+	u32 sel, ctrl;
+	u8 parent;
+
+	/* Sel is one-hot, so find the first bit set */
+	sel = clockman_read(clockman, data->sel_reg);
+	parent = ffs(sel) - 1;
+
+	/* sel == 0 implies the parent clock is not enabled yet. */
+	if (!sel) {
+		/* Read the clock src from the CTRL register instead */
+		ctrl = clockman_read(clockman, data->ctrl_reg);
+		parent = (ctrl & data->clk_src_mask) >> CLK_CTRL_SRC_SHIFT;
+	}
+
+	if (parent >= data->num_std_parents)
+		parent = AUX_SEL;
+
+	if (parent == AUX_SEL) {
+		/*
+		 * Clock parent is an auxiliary source, so get the parent from
+		 * the AUXSRC register field.
+		 */
+		ctrl = clockman_read(clockman, data->ctrl_reg);
+		parent = (ctrl & CLK_CTRL_AUXSRC_MASK) >> CLK_CTRL_AUXSRC_SHIFT;
+		parent += data->num_std_parents;
+	}
+
+	return parent;
+}
+
+static int rp1_clock_set_parent(struct clk_hw *hw, u8 index)
+{
+	struct rp1_clk_desc *clock = container_of(hw, struct rp1_clk_desc, hw);
+	struct rp1_clockman *clockman = clock->clockman;
+	const struct rp1_clock_data *data = clock->data;
+	u32 ctrl, sel;
+
+	spin_lock(&clockman->regs_lock);
+	ctrl = clockman_read(clockman, data->ctrl_reg);
+
+	if (index >= data->num_std_parents) {
+		/* This is an aux source request */
+		if (index >= data->num_std_parents + data->num_aux_parents) {
+			spin_unlock(&clockman->regs_lock);
+			return -EINVAL;
+		}
+
+		/* Select parent from aux list */
+		ctrl &= ~CLK_CTRL_AUXSRC_MASK;
+		ctrl |= FIELD_PREP(CLK_CTRL_AUXSRC_MASK, index - data->num_std_parents);
+		/* Set src to aux list */
+		ctrl &= ~data->clk_src_mask;
+		ctrl |= (AUX_SEL << CLK_CTRL_SRC_SHIFT) & data->clk_src_mask;
+	} else {
+		ctrl &= ~data->clk_src_mask;
+		ctrl |= (index << CLK_CTRL_SRC_SHIFT) & data->clk_src_mask;
+	}
+
+	clockman_write(clockman, data->ctrl_reg, ctrl);
+	spin_unlock(&clockman->regs_lock);
+
+	sel = rp1_clock_get_parent(hw);
+	WARN(sel != index, "(%s): Parent index req %u returned back %u\n",
+	     clk_hw_get_name(hw), index, sel);
+
+	return 0;
+}
+
+static int rp1_clock_set_rate_and_parent(struct clk_hw *hw,
+					 unsigned long rate,
+					 unsigned long parent_rate,
+					 u8 parent)
+{
+	struct rp1_clk_desc *clock = container_of(hw, struct rp1_clk_desc, hw);
+	struct rp1_clockman *clockman = clock->clockman;
+	const struct rp1_clock_data *data = clock->data;
+	u32 div = rp1_clock_choose_div(rate, parent_rate, data);
+
+	WARN(rate > 4000000000ll, "rate is -ve (%d)\n", (int)rate);
+
+	if (WARN(!div,
+		 "clk divider calculated as 0! (%s, rate %ld, parent rate %ld)\n",
+		 clk_hw_get_name(hw), rate, parent_rate))
+		div = 1 << CLK_DIV_FRAC_BITS;
+
+	spin_lock(&clockman->regs_lock);
+
+	clockman_write(clockman, data->div_int_reg, div >> CLK_DIV_FRAC_BITS);
+	if (data->div_frac_reg)
+		clockman_write(clockman, data->div_frac_reg, div << (32 - CLK_DIV_FRAC_BITS));
+
+	spin_unlock(&clockman->regs_lock);
+
+	if (parent != 0xff)
+		rp1_clock_set_parent(hw, parent);
+
+	return 0;
+}
+
+static int rp1_clock_set_rate(struct clk_hw *hw, unsigned long rate,
+			      unsigned long parent_rate)
+{
+	return rp1_clock_set_rate_and_parent(hw, rate, parent_rate, 0xff);
+}
+
+static void rp1_clock_choose_div_and_prate(struct clk_hw *hw,
+					   int parent_idx,
+					   unsigned long rate,
+					   unsigned long *prate,
+					   unsigned long *calc_rate)
+{
+	struct rp1_clk_desc *clock = container_of(hw, struct rp1_clk_desc, hw);
+	const struct rp1_clock_data *data = clock->data;
+	struct clk_hw *parent;
+	u32 div;
+	u64 tmp;
+
+	parent = clk_hw_get_parent_by_index(hw, parent_idx);
+
+	*prate = clk_hw_get_rate(parent);
+	div = rp1_clock_choose_div(rate, *prate, data);
+
+	if (!div) {
+		*calc_rate = 0;
+		return;
+	}
+
+	/* Recalculate to account for rounding errors */
+	tmp = (u64)*prate << CLK_DIV_FRAC_BITS;
+	tmp = div_u64(tmp, div);
+
+	/*
+	 * Prevent overclocks - if all parent choices result in
+	 * a downstream clock in excess of the maximum, then the
+	 * call to set the clock will fail.
+	 */
+	if (tmp > data->max_freq)
+		*calc_rate = 0;
+	else
+		*calc_rate = tmp;
+}
+
+static int rp1_clock_determine_rate(struct clk_hw *hw,
+				    struct clk_rate_request *req)
+{
+	struct clk_hw *parent, *best_parent = NULL;
+	unsigned long best_rate = 0;
+	unsigned long best_prate = 0;
+	unsigned long best_rate_diff = ULONG_MAX;
+	unsigned long prate, calc_rate;
+	size_t i;
+
+	/*
+	 * If the NO_REPARENT flag is set, try to use existing parent.
+	 */
+	if ((clk_hw_get_flags(hw) & CLK_SET_RATE_NO_REPARENT)) {
+		i = rp1_clock_get_parent(hw);
+		parent = clk_hw_get_parent_by_index(hw, i);
+		if (parent) {
+			rp1_clock_choose_div_and_prate(hw, i, req->rate, &prate,
+						       &calc_rate);
+			if (calc_rate > 0) {
+				req->best_parent_hw = parent;
+				req->best_parent_rate = prate;
+				req->rate = calc_rate;
+				return 0;
+			}
+		}
+	}
+
+	/*
+	 * Select parent clock that results in the closest rate (lower or
+	 * higher)
+	 */
+	for (i = 0; i < clk_hw_get_num_parents(hw); i++) {
+		parent = clk_hw_get_parent_by_index(hw, i);
+		if (!parent)
+			continue;
+
+		rp1_clock_choose_div_and_prate(hw, i, req->rate, &prate,
+					       &calc_rate);
+
+		if (abs_diff(calc_rate, req->rate) < best_rate_diff) {
+			best_parent = parent;
+			best_prate = prate;
+			best_rate = calc_rate;
+			best_rate_diff = abs_diff(calc_rate, req->rate);
+
+			if (best_rate_diff == 0)
+				break;
+		}
+	}
+
+	if (best_rate == 0)
+		return -EINVAL;
+
+	req->best_parent_hw = best_parent;
+	req->best_parent_rate = best_prate;
+	req->rate = best_rate;
+
+	return 0;
+}
+
+static const struct clk_ops rp1_pll_core_ops = {
+	.is_prepared = rp1_pll_core_is_on,
+	.prepare = rp1_pll_core_on,
+	.unprepare = rp1_pll_core_off,
+	.set_rate = rp1_pll_core_set_rate,
+	.recalc_rate = rp1_pll_core_recalc_rate,
+	.round_rate = rp1_pll_core_round_rate,
+};
+
+static const struct clk_ops rp1_pll_ops = {
+	.set_rate = rp1_pll_set_rate,
+	.recalc_rate = rp1_pll_recalc_rate,
+	.round_rate = rp1_pll_round_rate,
+};
+
+static const struct clk_ops rp1_pll_ph_ops = {
+	.is_prepared = rp1_pll_ph_is_on,
+	.prepare = rp1_pll_ph_on,
+	.unprepare = rp1_pll_ph_off,
+	.recalc_rate = rp1_pll_ph_recalc_rate,
+	.round_rate = rp1_pll_ph_round_rate,
+};
+
+static const struct clk_ops rp1_pll_divider_ops = {
+	.is_prepared = rp1_pll_divider_is_on,
+	.prepare = rp1_pll_divider_on,
+	.unprepare = rp1_pll_divider_off,
+	.set_rate = rp1_pll_divider_set_rate,
+	.recalc_rate = rp1_pll_divider_recalc_rate,
+	.round_rate = rp1_pll_divider_round_rate,
+};
+
+static const struct clk_ops rp1_clk_ops = {
+	.is_prepared = rp1_clock_is_on,
+	.prepare = rp1_clock_on,
+	.unprepare = rp1_clock_off,
+	.recalc_rate = rp1_clock_recalc_rate,
+	.get_parent = rp1_clock_get_parent,
+	.set_parent = rp1_clock_set_parent,
+	.set_rate_and_parent = rp1_clock_set_rate_and_parent,
+	.set_rate = rp1_clock_set_rate,
+	.determine_rate = rp1_clock_determine_rate,
+};
+
+static struct clk_hw *rp1_register_pll(struct rp1_clockman *clockman,
+				       struct rp1_clk_desc *desc)
+{
+	int ret;
+
+	desc->clockman = clockman;
+
+	ret = devm_clk_hw_register(clockman->dev, &desc->hw);
+
+	if (ret)
+		return ERR_PTR(ret);
+
+	return &desc->hw;
+}
+
+static struct clk_hw *rp1_register_pll_divider(struct rp1_clockman *clockman,
+					       struct rp1_clk_desc *desc)
+{
+	const struct rp1_pll_data *divider_data = desc->data;
+	int ret;
+
+	desc->div.reg = clockman->regs + divider_data->ctrl_reg;
+	desc->div.shift = PLL_SEC_DIV_SHIFT;
+	desc->div.width = PLL_SEC_DIV_WIDTH;
+	desc->div.flags = CLK_DIVIDER_ROUND_CLOSEST;
+	desc->div.lock = &clockman->regs_lock;
+	desc->div.hw.init = desc->hw.init;
+	desc->div.table = pll_sec_div_table;
+
+	desc->clockman = clockman;
+
+	ret = devm_clk_hw_register(clockman->dev, &desc->div.hw);
+
+	if (ret)
+		return ERR_PTR(ret);
+
+	return &desc->div.hw;
+}
+
+static struct clk_hw *rp1_register_clock(struct rp1_clockman *clockman,
+					 struct rp1_clk_desc *desc)
+{
+	const struct rp1_clock_data *clock_data = desc->data;
+	int ret;
+
+	if (WARN_ON_ONCE(MAX_CLK_PARENTS <
+	       clock_data->num_std_parents + clock_data->num_aux_parents))
+		return NULL;
+
+	/* There must be a gap for the AUX selector */
+	if (WARN_ON_ONCE(clock_data->num_std_parents > AUX_SEL &&
+			 desc->hw.init->parent_data[AUX_SEL].index != -1))
+		return NULL;
+
+	desc->clockman = clockman;
+
+	ret = devm_clk_hw_register(clockman->dev, &desc->hw);
+
+	if (ret)
+		return ERR_PTR(ret);
+
+	return &desc->hw;
+}
+
+/* Assignment helper macros for different clock types. */
+#define _REGISTER(f, ...)	{ .clk_register = f, __VA_ARGS__ }
+
+#define CLK_DATA(type, ...)	.data = &(struct type) { __VA_ARGS__ }
+
+#define REGISTER_PLL(...)	_REGISTER(&rp1_register_pll,		\
+					  __VA_ARGS__)
+
+#define REGISTER_PLL_DIV(...)	_REGISTER(&rp1_register_pll_divider,	\
+					  __VA_ARGS__)
+
+#define REGISTER_CLK(...)	_REGISTER(&rp1_register_clock,		\
+					  __VA_ARGS__)
+
+static struct rp1_clk_desc pll_sys_core_desc = REGISTER_PLL(
+	.hw.init = CLK_HW_INIT_PARENTS_DATA(
+		"pll_sys_core",
+		(const struct clk_parent_data[]) { { .index = 0 } },
+		&rp1_pll_core_ops,
+		CLK_IS_CRITICAL
+	),
+	CLK_DATA(rp1_pll_core_data,
+		 .cs_reg = PLL_SYS_CS,
+		 .pwr_reg = PLL_SYS_PWR,
+		 .fbdiv_int_reg = PLL_SYS_FBDIV_INT,
+		 .fbdiv_frac_reg = PLL_SYS_FBDIV_FRAC,
+	)
+);
+
+static struct rp1_clk_desc pll_audio_core_desc = REGISTER_PLL(
+	.hw.init = CLK_HW_INIT_PARENTS_DATA(
+		"pll_audio_core",
+		(const struct clk_parent_data[]) { { .index = 0 } },
+		&rp1_pll_core_ops,
+		CLK_IS_CRITICAL
+	),
+	CLK_DATA(rp1_pll_core_data,
+		 .cs_reg = PLL_AUDIO_CS,
+		 .pwr_reg = PLL_AUDIO_PWR,
+		 .fbdiv_int_reg = PLL_AUDIO_FBDIV_INT,
+		 .fbdiv_frac_reg = PLL_AUDIO_FBDIV_FRAC,
+	)
+);
+
+static struct rp1_clk_desc pll_video_core_desc = REGISTER_PLL(
+	.hw.init = CLK_HW_INIT_PARENTS_DATA(
+		"pll_video_core",
+		(const struct clk_parent_data[]) { { .index = 0 } },
+		&rp1_pll_core_ops,
+		CLK_IS_CRITICAL
+	),
+	CLK_DATA(rp1_pll_core_data,
+		 .cs_reg = PLL_VIDEO_CS,
+		 .pwr_reg = PLL_VIDEO_PWR,
+		 .fbdiv_int_reg = PLL_VIDEO_FBDIV_INT,
+		 .fbdiv_frac_reg = PLL_VIDEO_FBDIV_FRAC,
+	)
+);
+
+static struct rp1_clk_desc pll_sys_desc = REGISTER_PLL(
+	.hw.init = CLK_HW_INIT_PARENTS_DATA(
+		"pll_sys",
+		(const struct clk_parent_data[]) {
+			{ .hw = &pll_sys_core_desc.hw }
+		},
+		&rp1_pll_ops,
+		0
+	),
+	CLK_DATA(rp1_pll_data,
+		 .ctrl_reg = PLL_SYS_PRIM,
+		 .fc0_src = FC_NUM(0, 2),
+	)
+);
+
+static struct rp1_clk_desc pll_sys_sec_desc = REGISTER_PLL_DIV(
+	.hw.init = CLK_HW_INIT_PARENTS_DATA(
+		"pll_sys_sec",
+		(const struct clk_parent_data[]) {
+			{ .hw = &pll_sys_core_desc.hw }
+		},
+		&rp1_pll_divider_ops,
+		0
+	),
+	CLK_DATA(rp1_pll_data,
+		 .ctrl_reg = PLL_SYS_SEC,
+		 .fc0_src = FC_NUM(2, 2),
+	)
+);
+
+static struct rp1_clk_desc clk_eth_tsu_desc = REGISTER_CLK(
+	.hw.init = CLK_HW_INIT_PARENTS_DATA(
+		"clk_eth_tsu",
+		(const struct clk_parent_data[]) { { .index = 0 } },
+		&rp1_clk_ops,
+		0
+	),
+	CLK_DATA(rp1_clock_data,
+		 .num_std_parents = 0,
+		 .num_aux_parents = 1,
+		 .ctrl_reg = CLK_ETH_TSU_CTRL,
+		 .div_int_reg = CLK_ETH_TSU_DIV_INT,
+		 .sel_reg = CLK_ETH_TSU_SEL,
+		 .div_int_max = DIV_INT_8BIT_MAX,
+		 .max_freq = 50 * HZ_PER_MHZ,
+		 .fc0_src = FC_NUM(5, 7),
+	)
+);
+
+static const struct clk_parent_data clk_eth_parents[] = {
+	{ .hw = &pll_sys_sec_desc.div.hw },
+	{ .hw = &pll_sys_desc.hw },
+};
+
+static struct rp1_clk_desc clk_eth_desc = REGISTER_CLK(
+	.hw.init = CLK_HW_INIT_PARENTS_DATA(
+		"clk_eth",
+		clk_eth_parents,
+		&rp1_clk_ops,
+		0
+	),
+	CLK_DATA(rp1_clock_data,
+		 .num_std_parents = 0,
+		 .num_aux_parents = 2,
+		 .ctrl_reg = CLK_ETH_CTRL,
+		 .div_int_reg = CLK_ETH_DIV_INT,
+		 .sel_reg = CLK_ETH_SEL,
+		 .div_int_max = DIV_INT_8BIT_MAX,
+		 .max_freq = 125 * HZ_PER_MHZ,
+		 .fc0_src = FC_NUM(4, 6),
+	)
+);
+
+static const struct clk_parent_data clk_sys_parents[] = {
+	{ .index = 0 },
+	{ .index = -1 },
+	{ .hw = &pll_sys_desc.hw },
+};
+
+static struct rp1_clk_desc clk_sys_desc = REGISTER_CLK(
+	.hw.init = CLK_HW_INIT_PARENTS_DATA(
+		"clk_sys",
+		clk_sys_parents,
+		&rp1_clk_ops,
+		CLK_IS_CRITICAL
+	),
+	CLK_DATA(rp1_clock_data,
+		 .num_std_parents = 3,
+		 .num_aux_parents = 0,
+		 .ctrl_reg = CLK_SYS_CTRL,
+		 .div_int_reg = CLK_SYS_DIV_INT,
+		 .sel_reg = CLK_SYS_SEL,
+		 .div_int_max = DIV_INT_24BIT_MAX,
+		 .max_freq = 200 * HZ_PER_MHZ,
+		 .fc0_src = FC_NUM(0, 4),
+		 .clk_src_mask = 0x3,
+	)
+);
+
+static struct rp1_clk_desc pll_sys_pri_ph_desc = REGISTER_PLL(
+	.hw.init = CLK_HW_INIT_PARENTS_DATA(
+		"pll_sys_pri_ph",
+		(const struct clk_parent_data[]) {
+			{ .hw = &pll_sys_desc.hw }
+		},
+		&rp1_pll_ph_ops,
+		0
+	),
+	CLK_DATA(rp1_pll_ph_data,
+		 .ph_reg = PLL_SYS_PRIM,
+		 .fixed_divider = 2,
+		 .phase = RP1_PLL_PHASE_0,
+		 .fc0_src = FC_NUM(1, 2),
+	)
+);
+
+static struct rp1_clk_desc *const clk_desc_array[] = {
+	[RP1_PLL_SYS_CORE] = &pll_sys_core_desc,
+	[RP1_PLL_AUDIO_CORE] = &pll_audio_core_desc,
+	[RP1_PLL_VIDEO_CORE] = &pll_video_core_desc,
+	[RP1_PLL_SYS] = &pll_sys_desc,
+	[RP1_CLK_ETH_TSU] = &clk_eth_tsu_desc,
+	[RP1_CLK_ETH] = &clk_eth_desc,
+	[RP1_CLK_SYS] = &clk_sys_desc,
+	[RP1_PLL_SYS_PRI_PH] = &pll_sys_pri_ph_desc,
+	[RP1_PLL_SYS_SEC] = &pll_sys_sec_desc,
+};
+
+static const struct regmap_range rp1_reg_ranges[] = {
+	regmap_reg_range(PLL_SYS_CS, PLL_SYS_SEC),
+	regmap_reg_range(PLL_AUDIO_CS, PLL_AUDIO_TERN),
+	regmap_reg_range(PLL_VIDEO_CS, PLL_VIDEO_SEC),
+	regmap_reg_range(GPCLK_OE_CTRL, GPCLK_OE_CTRL),
+	regmap_reg_range(CLK_SYS_CTRL, CLK_SYS_DIV_INT),
+	regmap_reg_range(CLK_SYS_SEL, CLK_SYS_SEL),
+	regmap_reg_range(CLK_SLOW_SYS_CTRL, CLK_SLOW_SYS_DIV_INT),
+	regmap_reg_range(CLK_SLOW_SYS_SEL, CLK_SLOW_SYS_SEL),
+	regmap_reg_range(CLK_DMA_CTRL, CLK_DMA_DIV_INT),
+	regmap_reg_range(CLK_DMA_SEL, CLK_DMA_SEL),
+	regmap_reg_range(CLK_UART_CTRL, CLK_UART_DIV_INT),
+	regmap_reg_range(CLK_UART_SEL, CLK_UART_SEL),
+	regmap_reg_range(CLK_ETH_CTRL, CLK_ETH_DIV_INT),
+	regmap_reg_range(CLK_ETH_SEL, CLK_ETH_SEL),
+	regmap_reg_range(CLK_PWM0_CTRL, CLK_PWM0_SEL),
+	regmap_reg_range(CLK_PWM1_CTRL, CLK_PWM1_SEL),
+	regmap_reg_range(CLK_AUDIO_IN_CTRL, CLK_AUDIO_IN_DIV_INT),
+	regmap_reg_range(CLK_AUDIO_IN_SEL, CLK_AUDIO_IN_SEL),
+	regmap_reg_range(CLK_AUDIO_OUT_CTRL, CLK_AUDIO_OUT_DIV_INT),
+	regmap_reg_range(CLK_AUDIO_OUT_SEL, CLK_AUDIO_OUT_SEL),
+	regmap_reg_range(CLK_I2S_CTRL, CLK_I2S_DIV_INT),
+	regmap_reg_range(CLK_I2S_SEL, CLK_I2S_SEL),
+	regmap_reg_range(CLK_MIPI0_CFG_CTRL, CLK_MIPI0_CFG_DIV_INT),
+	regmap_reg_range(CLK_MIPI0_CFG_SEL, CLK_MIPI0_CFG_SEL),
+	regmap_reg_range(CLK_MIPI1_CFG_CTRL, CLK_MIPI1_CFG_DIV_INT),
+	regmap_reg_range(CLK_MIPI1_CFG_SEL, CLK_MIPI1_CFG_SEL),
+	regmap_reg_range(CLK_PCIE_AUX_CTRL, CLK_PCIE_AUX_DIV_INT),
+	regmap_reg_range(CLK_PCIE_AUX_SEL, CLK_PCIE_AUX_SEL),
+	regmap_reg_range(CLK_USBH0_MICROFRAME_CTRL, CLK_USBH0_MICROFRAME_DIV_INT),
+	regmap_reg_range(CLK_USBH0_MICROFRAME_SEL, CLK_USBH0_MICROFRAME_SEL),
+	regmap_reg_range(CLK_USBH1_MICROFRAME_CTRL, CLK_USBH1_MICROFRAME_DIV_INT),
+	regmap_reg_range(CLK_USBH1_MICROFRAME_SEL, CLK_USBH1_MICROFRAME_SEL),
+	regmap_reg_range(CLK_USBH0_SUSPEND_CTRL, CLK_USBH0_SUSPEND_DIV_INT),
+	regmap_reg_range(CLK_USBH0_SUSPEND_SEL, CLK_USBH0_SUSPEND_SEL),
+	regmap_reg_range(CLK_USBH1_SUSPEND_CTRL, CLK_USBH1_SUSPEND_DIV_INT),
+	regmap_reg_range(CLK_USBH1_SUSPEND_SEL, CLK_USBH1_SUSPEND_SEL),
+	regmap_reg_range(CLK_ETH_TSU_CTRL, CLK_ETH_TSU_DIV_INT),
+	regmap_reg_range(CLK_ETH_TSU_SEL, CLK_ETH_TSU_SEL),
+	regmap_reg_range(CLK_ADC_CTRL, CLK_ADC_DIV_INT),
+	regmap_reg_range(CLK_ADC_SEL, CLK_ADC_SEL),
+	regmap_reg_range(CLK_SDIO_TIMER_CTRL, CLK_SDIO_TIMER_DIV_INT),
+	regmap_reg_range(CLK_SDIO_TIMER_SEL, CLK_SDIO_TIMER_SEL),
+	regmap_reg_range(CLK_SDIO_ALT_SRC_CTRL, CLK_SDIO_ALT_SRC_DIV_INT),
+	regmap_reg_range(CLK_SDIO_ALT_SRC_SEL, CLK_SDIO_ALT_SRC_SEL),
+	regmap_reg_range(CLK_GP0_CTRL, CLK_GP0_SEL),
+	regmap_reg_range(CLK_GP1_CTRL, CLK_GP1_SEL),
+	regmap_reg_range(CLK_GP2_CTRL, CLK_GP2_SEL),
+	regmap_reg_range(CLK_GP3_CTRL, CLK_GP3_SEL),
+	regmap_reg_range(CLK_GP4_CTRL, CLK_GP4_SEL),
+	regmap_reg_range(CLK_GP5_CTRL, CLK_GP5_SEL),
+	regmap_reg_range(CLK_SYS_RESUS_CTRL, CLK_SYS_RESUS_CTRL),
+	regmap_reg_range(CLK_SLOW_SYS_RESUS_CTRL, CLK_SLOW_SYS_RESUS_CTRL),
+	regmap_reg_range(FC0_REF_KHZ, FC0_RESULT),
+	regmap_reg_range(VIDEO_CLK_VEC_CTRL, VIDEO_CLK_VEC_DIV_INT),
+	regmap_reg_range(VIDEO_CLK_VEC_SEL, VIDEO_CLK_DPI_DIV_INT),
+	regmap_reg_range(VIDEO_CLK_DPI_SEL, VIDEO_CLK_MIPI1_DPI_SEL),
+};
+
+static const struct regmap_access_table rp1_reg_table = {
+	.yes_ranges = rp1_reg_ranges,
+	.n_yes_ranges = ARRAY_SIZE(rp1_reg_ranges),
+};
+
+static const struct regmap_config rp1_clk_regmap_cfg = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.max_register = PLL_VIDEO_SEC,
+	.name = "rp1-clk",
+	.rd_table = &rp1_reg_table,
+};
+
+static int rp1_clk_probe(struct platform_device *pdev)
+{
+	const size_t asize = ARRAY_SIZE(clk_desc_array);
+	struct rp1_clk_desc *desc;
+	struct device *dev = &pdev->dev;
+	struct rp1_clockman *clockman;
+	struct clk_hw **hws;
+	unsigned int i;
+
+	clockman = devm_kzalloc(dev, struct_size(clockman, onecell.hws, asize),
+				GFP_KERNEL);
+	if (!clockman)
+		return -ENOMEM;
+
+	spin_lock_init(&clockman->regs_lock);
+	clockman->dev = dev;
+
+	clockman->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(clockman->regs))
+		return PTR_ERR(clockman->regs);
+
+	clockman->regmap = devm_regmap_init_mmio(dev, clockman->regs,
+						 &rp1_clk_regmap_cfg);
+	if (IS_ERR(clockman->regmap)) {
+		dev_err_probe(dev, PTR_ERR(clockman->regmap),
+			      "could not init clock regmap\n");
+		return PTR_ERR(clockman->regmap);
+	}
+
+	clockman->onecell.num = asize;
+	hws = clockman->onecell.hws;
+
+	for (i = 0; i < asize; i++) {
+		desc = clk_desc_array[i];
+		if (desc && desc->clk_register && desc->data) {
+			hws[i] = desc->clk_register(clockman, desc);
+			if (IS_ERR_OR_NULL(hws[i]))
+				dev_err_probe(dev, PTR_ERR(hws[i]),
+					      "Unable to register clock: %s\n",
+					      clk_hw_get_name(hws[i]));
+		}
+	}
+
+	platform_set_drvdata(pdev, clockman);
+
+	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
+					   &clockman->onecell);
+}
+
+static const struct of_device_id rp1_clk_of_match[] = {
+	{ .compatible = "raspberrypi,rp1-clocks" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, rp1_clk_of_match);
+
+static struct platform_driver rp1_clk_driver = {
+	.driver = {
+		.name = "rp1-clk",
+		.of_match_table = rp1_clk_of_match,
+	},
+	.probe = rp1_clk_probe,
+};
+
+module_platform_driver(rp1_clk_driver);
+
+MODULE_AUTHOR("Naushir Patuck <naush@raspberrypi.com>");
+MODULE_AUTHOR("Andrea della Porta <andrea.porta@suse.com>");
+MODULE_DESCRIPTION("RP1 clock driver");
+MODULE_LICENSE("GPL");
-- 
2.35.3

[PATCH v6 06/10] pinctrl: rp1: Implement RaspberryPi RP1 gpio support

[Andrea della Porta]

The RP1 is an MFD supporting a gpio controller and /pinmux/pinctrl.
Add minimum support for the gpio only portion. The driver is in
pinctrl folder since upcoming patches will add the pinmux/pinctrl
support where the gpio part can be seen as an addition.

Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Stefan Wahren <wahrenst@gmx.net>
---
 MAINTAINERS                   |   1 +
 drivers/pinctrl/Kconfig       |  11 +
 drivers/pinctrl/Makefile      |   1 +
 drivers/pinctrl/pinctrl-rp1.c | 789 ++++++++++++++++++++++++++++++++++
 4 files changed, 802 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-rp1.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 9ad37eb45a51..9b8e87b8d6de 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19582,6 +19582,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/pinctrl/pinctrl-rp1.c
 F:	include/dt-bindings/clock/rp1.h
 F:	include/dt-bindings/misc/rp1.h
 
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 95a8e2b9a614..53b265cabc26 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -612,6 +612,17 @@ config PINCTRL_MLXBF3
 	  each pin. This driver can also be built as a module called
 	  pinctrl-mlxbf3.
 
+config PINCTRL_RP1
+	tristate "Pinctrl driver for RP1"
+	depends on MISC_RP1
+	default MISC_RP1
+	select PINMUX
+	select PINCONF
+	select GENERIC_PINCONF
+	help
+	  Enable the gpio and pinctrl/mux driver for RaspberryPi RP1
+	  multi function device.
+
 source "drivers/pinctrl/actions/Kconfig"
 source "drivers/pinctrl/aspeed/Kconfig"
 source "drivers/pinctrl/bcm/Kconfig"
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index fba1c56624c0..6fb77de58545 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -48,6 +48,7 @@ obj-$(CONFIG_PINCTRL_PIC32)	+= pinctrl-pic32.o
 obj-$(CONFIG_PINCTRL_PISTACHIO)	+= pinctrl-pistachio.o
 obj-$(CONFIG_PINCTRL_RK805)	+= pinctrl-rk805.o
 obj-$(CONFIG_PINCTRL_ROCKCHIP)	+= pinctrl-rockchip.o
+obj-$(CONFIG_PINCTRL_RP1)       += pinctrl-rp1.o
 obj-$(CONFIG_PINCTRL_SCMI)	+= pinctrl-scmi.o
 obj-$(CONFIG_PINCTRL_SINGLE)	+= pinctrl-single.o
 obj-$(CONFIG_PINCTRL_ST) 	+= pinctrl-st.o
diff --git a/drivers/pinctrl/pinctrl-rp1.c b/drivers/pinctrl/pinctrl-rp1.c
new file mode 100644
index 000000000000..0d081f75d2ec
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-rp1.c
@@ -0,0 +1,789 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for Raspberry Pi RP1 GPIO unit
+ *
+ * Copyright (C) 2023 Raspberry Pi Ltd.
+ *
+ * This driver is inspired by:
+ * pinctrl-bcm2835.c, please see original file for copyright information
+ */
+
+#include <linux/gpio/driver.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define MODULE_NAME "pinctrl-rp1"
+#define RP1_NUM_GPIOS	54
+#define RP1_NUM_BANKS	3
+
+#define RP1_INT_EDGE_FALLING		BIT(0)
+#define RP1_INT_EDGE_RISING		BIT(1)
+#define RP1_INT_LEVEL_LOW		BIT(2)
+#define RP1_INT_LEVEL_HIGH		BIT(3)
+#define RP1_INT_MASK			GENMASK(3, 0)
+#define RP1_INT_EDGE_BOTH		(RP1_INT_EDGE_FALLING |	\
+					 RP1_INT_EDGE_RISING)
+
+#define RP1_FSEL_COUNT			9
+
+#define RP1_FSEL_ALT0			0x00
+#define RP1_FSEL_GPIO			0x05
+#define RP1_FSEL_NONE			0x09
+#define RP1_FSEL_NONE_HW		0x1f
+
+#define RP1_PAD_DRIVE_2MA		0x0
+#define RP1_PAD_DRIVE_4MA		0x1
+#define RP1_PAD_DRIVE_8MA		0x2
+#define RP1_PAD_DRIVE_12MA		0x3
+
+enum {
+	RP1_PUD_OFF			= 0,
+	RP1_PUD_DOWN			= 1,
+	RP1_PUD_UP			= 2,
+};
+
+enum {
+	RP1_DIR_OUTPUT			= 0,
+	RP1_DIR_INPUT			= 1,
+};
+
+enum {
+	RP1_OUTOVER_PERI		= 0,
+	RP1_OUTOVER_INVPERI		= 1,
+	RP1_OUTOVER_LOW			= 2,
+	RP1_OUTOVER_HIGH		= 3,
+};
+
+enum {
+	RP1_OEOVER_PERI			= 0,
+	RP1_OEOVER_INVPERI		= 1,
+	RP1_OEOVER_DISABLE		= 2,
+	RP1_OEOVER_ENABLE		= 3,
+};
+
+enum {
+	RP1_INOVER_PERI			= 0,
+	RP1_INOVER_INVPERI		= 1,
+	RP1_INOVER_LOW			= 2,
+	RP1_INOVER_HIGH			= 3,
+};
+
+enum {
+	RP1_GPIO_CTRL_IRQRESET_SET		= 0,
+	RP1_GPIO_CTRL_INT_CLR			= 1,
+	RP1_GPIO_CTRL_INT_SET			= 2,
+	RP1_GPIO_CTRL_OEOVER			= 3,
+	RP1_GPIO_CTRL_FUNCSEL			= 4,
+	RP1_GPIO_CTRL_OUTOVER			= 5,
+	RP1_GPIO_CTRL				= 6,
+};
+
+enum {
+	RP1_INTE_SET			= 0,
+	RP1_INTE_CLR			= 1,
+};
+
+enum {
+	RP1_RIO_OUT_SET			= 0,
+	RP1_RIO_OUT_CLR			= 1,
+	RP1_RIO_OE			= 2,
+	RP1_RIO_OE_SET			= 3,
+	RP1_RIO_OE_CLR			= 4,
+	RP1_RIO_IN			= 5,
+};
+
+enum {
+	RP1_PAD_SLEWFAST		= 0,
+	RP1_PAD_SCHMITT			= 1,
+	RP1_PAD_PULL			= 2,
+	RP1_PAD_DRIVE			= 3,
+	RP1_PAD_IN_ENABLE		= 4,
+	RP1_PAD_OUT_DISABLE		= 5,
+};
+
+static const struct reg_field rp1_gpio_fields[] = {
+	[RP1_GPIO_CTRL_IRQRESET_SET]	= REG_FIELD(0x2004, 28, 28),
+	[RP1_GPIO_CTRL_INT_CLR]		= REG_FIELD(0x3004, 20, 23),
+	[RP1_GPIO_CTRL_INT_SET]		= REG_FIELD(0x2004, 20, 23),
+	[RP1_GPIO_CTRL_OEOVER]		= REG_FIELD(0x0004, 14, 15),
+	[RP1_GPIO_CTRL_FUNCSEL]		= REG_FIELD(0x0004, 0, 4),
+	[RP1_GPIO_CTRL_OUTOVER]		= REG_FIELD(0x0004, 12, 13),
+	[RP1_GPIO_CTRL]			= REG_FIELD(0x0004, 0, 31),
+};
+
+static const struct reg_field rp1_inte_fields[] = {
+	[RP1_INTE_SET]			= REG_FIELD(0x2000, 0, 0),
+	[RP1_INTE_CLR]			= REG_FIELD(0x3000, 0, 0),
+};
+
+static const struct reg_field rp1_rio_fields[] = {
+	[RP1_RIO_OUT_SET]		= REG_FIELD(0x2000, 0, 0),
+	[RP1_RIO_OUT_CLR]		= REG_FIELD(0x3000, 0, 0),
+	[RP1_RIO_OE]			= REG_FIELD(0x0004, 0, 0),
+	[RP1_RIO_OE_SET]		= REG_FIELD(0x2004, 0, 0),
+	[RP1_RIO_OE_CLR]		= REG_FIELD(0x3004, 0, 0),
+	[RP1_RIO_IN]			= REG_FIELD(0x0008, 0, 0),
+};
+
+static const struct reg_field rp1_pad_fields[] = {
+	[RP1_PAD_SLEWFAST]		= REG_FIELD(0, 0, 0),
+	[RP1_PAD_SCHMITT]		= REG_FIELD(0, 1, 1),
+	[RP1_PAD_PULL]			= REG_FIELD(0, 2, 3),
+	[RP1_PAD_DRIVE]			= REG_FIELD(0, 4, 5),
+	[RP1_PAD_IN_ENABLE]		= REG_FIELD(0, 6, 6),
+	[RP1_PAD_OUT_DISABLE]		= REG_FIELD(0, 7, 7),
+};
+
+struct rp1_iobank_desc {
+	int min_gpio;
+	int num_gpios;
+	int gpio_offset;
+	int inte_offset;
+	int ints_offset;
+	int rio_offset;
+	int pads_offset;
+};
+
+struct rp1_pin_info {
+	u8 num;
+	u8 bank;
+	u8 offset;
+	u8 fsel;
+	u8 irq_type;
+
+	struct regmap_field *gpio[ARRAY_SIZE(rp1_gpio_fields)];
+	struct regmap_field *rio[ARRAY_SIZE(rp1_rio_fields)];
+	struct regmap_field *inte[ARRAY_SIZE(rp1_inte_fields)];
+	struct regmap_field *pad[ARRAY_SIZE(rp1_pad_fields)];
+};
+
+struct rp1_pinctrl {
+	struct device *dev;
+	void __iomem *gpio_base;
+	void __iomem *rio_base;
+	void __iomem *pads_base;
+	int irq[RP1_NUM_BANKS];
+	struct rp1_pin_info pins[RP1_NUM_GPIOS];
+
+	struct pinctrl_dev *pctl_dev;
+	struct gpio_chip gpio_chip;
+	struct pinctrl_gpio_range gpio_range;
+
+	raw_spinlock_t irq_lock[RP1_NUM_BANKS];
+};
+
+static const struct rp1_iobank_desc rp1_iobanks[RP1_NUM_BANKS] = {
+	/*         gpio   inte    ints     rio    pads */
+	{  0, 28, 0x0000, 0x011c, 0x0124, 0x0000, 0x0004 },
+	{ 28,  6, 0x4000, 0x411c, 0x4124, 0x4000, 0x4004 },
+	{ 34, 20, 0x8000, 0x811c, 0x8124, 0x8000, 0x8004 },
+};
+
+static int rp1_pinconf_set(struct rp1_pin_info *pin,
+			   unsigned int offset, unsigned long *configs,
+			   unsigned int num_configs);
+
+static struct rp1_pin_info *rp1_get_pin(struct gpio_chip *chip,
+					unsigned int offset)
+{
+	struct rp1_pinctrl *pc = gpiochip_get_data(chip);
+
+	if (pc && offset < RP1_NUM_GPIOS)
+		return &pc->pins[offset];
+	return NULL;
+}
+
+static void rp1_input_enable(struct rp1_pin_info *pin, int value)
+{
+	regmap_field_write(pin->pad[RP1_PAD_IN_ENABLE], !!value);
+}
+
+static void rp1_output_enable(struct rp1_pin_info *pin, int value)
+{
+	regmap_field_write(pin->pad[RP1_PAD_OUT_DISABLE], !value);
+}
+
+static u32 rp1_get_fsel(struct rp1_pin_info *pin)
+{
+	u32 oeover, fsel;
+
+	regmap_field_read(pin->gpio[RP1_GPIO_CTRL_OEOVER], &oeover);
+	regmap_field_read(pin->gpio[RP1_GPIO_CTRL_FUNCSEL], &fsel);
+
+	if (oeover != RP1_OEOVER_PERI || fsel >= RP1_FSEL_COUNT)
+		fsel = RP1_FSEL_NONE;
+
+	return fsel;
+}
+
+static void rp1_set_fsel(struct rp1_pin_info *pin, u32 fsel)
+{
+	if (fsel >= RP1_FSEL_COUNT)
+		fsel = RP1_FSEL_NONE_HW;
+
+	rp1_input_enable(pin, 1);
+	rp1_output_enable(pin, 1);
+
+	if (fsel == RP1_FSEL_NONE) {
+		regmap_field_write(pin->gpio[RP1_GPIO_CTRL_OEOVER], RP1_OEOVER_DISABLE);
+	} else {
+		regmap_field_write(pin->gpio[RP1_GPIO_CTRL_OUTOVER], RP1_OUTOVER_PERI);
+		regmap_field_write(pin->gpio[RP1_GPIO_CTRL_OEOVER], RP1_OEOVER_PERI);
+	}
+
+	regmap_field_write(pin->gpio[RP1_GPIO_CTRL_FUNCSEL], fsel);
+}
+
+static int rp1_get_dir(struct rp1_pin_info *pin)
+{
+	unsigned int val;
+
+	regmap_field_read(pin->rio[RP1_RIO_OE], &val);
+
+	return !val ? RP1_DIR_INPUT : RP1_DIR_OUTPUT;
+}
+
+static void rp1_set_dir(struct rp1_pin_info *pin, bool is_input)
+{
+	int reg = is_input ? RP1_RIO_OE_CLR : RP1_RIO_OE_SET;
+
+	regmap_field_write(pin->rio[reg], 1);
+}
+
+static int rp1_get_value(struct rp1_pin_info *pin)
+{
+	unsigned int val;
+
+	regmap_field_read(pin->rio[RP1_RIO_IN], &val);
+
+	return !!val;
+}
+
+static void rp1_set_value(struct rp1_pin_info *pin, int value)
+{
+	/* Assume the pin is already an output */
+	int reg = value ? RP1_RIO_OUT_SET : RP1_RIO_OUT_CLR;
+
+	regmap_field_write(pin->rio[reg], 1);
+}
+
+static int rp1_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct rp1_pin_info *pin = rp1_get_pin(chip, offset);
+	int ret;
+
+	if (!pin)
+		return -EINVAL;
+
+	ret = rp1_get_value(pin);
+
+	return ret;
+}
+
+static void rp1_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
+{
+	struct rp1_pin_info *pin = rp1_get_pin(chip, offset);
+
+	if (pin)
+		rp1_set_value(pin, value);
+}
+
+static int rp1_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
+{
+	struct rp1_pin_info *pin = rp1_get_pin(chip, offset);
+	u32 fsel;
+
+	if (!pin)
+		return -EINVAL;
+
+	fsel = rp1_get_fsel(pin);
+	if (fsel != RP1_FSEL_GPIO)
+		return -EINVAL;
+
+	return (rp1_get_dir(pin) == RP1_DIR_OUTPUT) ?
+		GPIO_LINE_DIRECTION_OUT :
+		GPIO_LINE_DIRECTION_IN;
+}
+
+static int rp1_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
+{
+	struct rp1_pin_info *pin = rp1_get_pin(chip, offset);
+
+	if (!pin)
+		return -EINVAL;
+	rp1_set_dir(pin, RP1_DIR_INPUT);
+	rp1_set_fsel(pin, RP1_FSEL_GPIO);
+
+	return 0;
+}
+
+static int rp1_gpio_direction_output(struct gpio_chip *chip, unsigned int offset,
+				     int value)
+{
+	struct rp1_pin_info *pin = rp1_get_pin(chip, offset);
+
+	if (!pin)
+		return -EINVAL;
+	rp1_set_value(pin, value);
+	rp1_set_dir(pin, RP1_DIR_OUTPUT);
+	rp1_set_fsel(pin, RP1_FSEL_GPIO);
+
+	return 0;
+}
+
+static int rp1_gpio_set_config(struct gpio_chip *chip, unsigned int offset,
+			       unsigned long config)
+{
+	struct rp1_pin_info *pin = rp1_get_pin(chip, offset);
+	unsigned long configs[] = { config };
+
+	return rp1_pinconf_set(pin, offset, configs,
+			       ARRAY_SIZE(configs));
+}
+
+static const struct gpio_chip rp1_gpio_chip = {
+	.label = MODULE_NAME,
+	.owner = THIS_MODULE,
+	.request = gpiochip_generic_request,
+	.free = gpiochip_generic_free,
+	.direction_input = rp1_gpio_direction_input,
+	.direction_output = rp1_gpio_direction_output,
+	.get_direction = rp1_gpio_get_direction,
+	.get = rp1_gpio_get,
+	.set = rp1_gpio_set,
+	.base = -1,
+	.set_config = rp1_gpio_set_config,
+	.ngpio = RP1_NUM_GPIOS,
+	.can_sleep = false,
+};
+
+static void rp1_gpio_irq_handler(struct irq_desc *desc)
+{
+	struct gpio_chip *chip = irq_desc_get_handler_data(desc);
+	struct irq_chip *host_chip = irq_desc_get_chip(desc);
+	struct rp1_pinctrl *pc = gpiochip_get_data(chip);
+	const struct rp1_iobank_desc *bank;
+	int irq = irq_desc_get_irq(desc);
+	unsigned long ints;
+	int bit_pos;
+
+	if (pc->irq[0] == irq)
+		bank = &rp1_iobanks[0];
+	else if (pc->irq[1] == irq)
+		bank = &rp1_iobanks[1];
+	else
+		bank = &rp1_iobanks[2];
+
+	chained_irq_enter(host_chip, desc);
+
+	ints = readl(pc->gpio_base + bank->ints_offset);
+	for_each_set_bit(bit_pos, &ints, 32) {
+		struct rp1_pin_info *pin = rp1_get_pin(chip, bit_pos);
+
+		regmap_field_write(pin->gpio[RP1_GPIO_CTRL_IRQRESET_SET], 1);
+		generic_handle_irq(irq_linear_revmap(pc->gpio_chip.irq.domain,
+						     bank->gpio_offset + bit_pos));
+	}
+
+	chained_irq_exit(host_chip, desc);
+}
+
+static void rp1_gpio_irq_config(struct rp1_pin_info *pin, bool enable)
+{
+	int reg = enable ? RP1_INTE_SET : RP1_INTE_CLR;
+
+	regmap_field_write(pin->inte[reg], 1);
+	if (!enable)
+		/* Clear any latched events */
+		regmap_field_write(pin->gpio[RP1_GPIO_CTRL_IRQRESET_SET], 1);
+}
+
+static void rp1_gpio_irq_enable(struct irq_data *data)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+	unsigned int gpio = irqd_to_hwirq(data);
+	struct rp1_pin_info *pin = rp1_get_pin(chip, gpio);
+
+	rp1_gpio_irq_config(pin, true);
+}
+
+static void rp1_gpio_irq_disable(struct irq_data *data)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+	unsigned int gpio = irqd_to_hwirq(data);
+	struct rp1_pin_info *pin = rp1_get_pin(chip, gpio);
+
+	rp1_gpio_irq_config(pin, false);
+}
+
+static int rp1_irq_set_type(struct rp1_pin_info *pin, unsigned int type)
+{
+	u32 irq_flags;
+
+	switch (type) {
+	case IRQ_TYPE_NONE:
+		irq_flags = 0;
+		break;
+	case IRQ_TYPE_EDGE_RISING:
+		irq_flags = RP1_INT_EDGE_RISING;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		irq_flags = RP1_INT_EDGE_FALLING;
+		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		irq_flags = RP1_INT_EDGE_RISING | RP1_INT_EDGE_FALLING;
+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		irq_flags = RP1_INT_LEVEL_HIGH;
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		irq_flags = RP1_INT_LEVEL_LOW;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	/* Clear them all */
+	regmap_field_write(pin->gpio[RP1_GPIO_CTRL_INT_CLR], RP1_INT_MASK);
+
+	/* Set those that are needed */
+	regmap_field_write(pin->gpio[RP1_GPIO_CTRL_INT_SET], irq_flags);
+	pin->irq_type = type;
+
+	return 0;
+}
+
+static int rp1_gpio_irq_set_type(struct irq_data *data, unsigned int type)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+	unsigned int gpio = irqd_to_hwirq(data);
+	struct rp1_pin_info *pin = rp1_get_pin(chip, gpio);
+	struct rp1_pinctrl *pc = gpiochip_get_data(chip);
+	int bank = pin->bank;
+	unsigned long flags;
+	int ret;
+
+	raw_spin_lock_irqsave(&pc->irq_lock[bank], flags);
+
+	ret = rp1_irq_set_type(pin, type);
+	if (!ret) {
+		if (type & IRQ_TYPE_EDGE_BOTH)
+			irq_set_handler_locked(data, handle_edge_irq);
+		else
+			irq_set_handler_locked(data, handle_level_irq);
+	}
+
+	raw_spin_unlock_irqrestore(&pc->irq_lock[bank], flags);
+
+	return ret;
+}
+
+static void rp1_gpio_irq_ack(struct irq_data *data)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+	unsigned int gpio = irqd_to_hwirq(data);
+	struct rp1_pin_info *pin = rp1_get_pin(chip, gpio);
+
+	/* Clear any latched events */
+	regmap_field_write(pin->gpio[RP1_GPIO_CTRL_IRQRESET_SET], 1);
+}
+
+static struct irq_chip rp1_gpio_irq_chip = {
+	.name = MODULE_NAME,
+	.irq_enable = rp1_gpio_irq_enable,
+	.irq_disable = rp1_gpio_irq_disable,
+	.irq_set_type = rp1_gpio_irq_set_type,
+	.irq_ack = rp1_gpio_irq_ack,
+	.irq_mask = rp1_gpio_irq_disable,
+	.irq_unmask = rp1_gpio_irq_enable,
+	.flags = IRQCHIP_IMMUTABLE,
+};
+
+static void rp1_pull_config_set(struct rp1_pin_info *pin, unsigned int arg)
+{
+	regmap_field_write(pin->pad[RP1_PAD_PULL], arg & 0x3);
+}
+
+static int rp1_pinconf_set(struct rp1_pin_info *pin, unsigned int offset,
+			   unsigned long *configs, unsigned int num_configs)
+{
+	u32 param, arg;
+	int i;
+
+	if (!pin)
+		return -EINVAL;
+
+	for (i = 0; i < num_configs; i++) {
+		param = pinconf_to_config_param(configs[i]);
+		arg = pinconf_to_config_argument(configs[i]);
+
+		switch (param) {
+		case PIN_CONFIG_BIAS_DISABLE:
+			rp1_pull_config_set(pin, RP1_PUD_OFF);
+			break;
+
+		case PIN_CONFIG_BIAS_PULL_DOWN:
+			rp1_pull_config_set(pin, RP1_PUD_DOWN);
+			break;
+
+		case PIN_CONFIG_BIAS_PULL_UP:
+			rp1_pull_config_set(pin, RP1_PUD_UP);
+			break;
+
+		case PIN_CONFIG_INPUT_ENABLE:
+			rp1_input_enable(pin, arg);
+			break;
+
+		case PIN_CONFIG_OUTPUT_ENABLE:
+			rp1_output_enable(pin, arg);
+			break;
+
+		case PIN_CONFIG_OUTPUT:
+			rp1_set_value(pin, arg);
+			rp1_set_dir(pin, RP1_DIR_OUTPUT);
+			rp1_set_fsel(pin, RP1_FSEL_GPIO);
+			break;
+
+		case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
+			regmap_field_write(pin->pad[RP1_PAD_SCHMITT], !!arg);
+			break;
+
+		case PIN_CONFIG_SLEW_RATE:
+			regmap_field_write(pin->pad[RP1_PAD_SLEWFAST], !!arg);
+			break;
+
+		case PIN_CONFIG_DRIVE_STRENGTH:
+			switch (arg) {
+			case 2:
+				arg = RP1_PAD_DRIVE_2MA;
+				break;
+			case 4:
+				arg = RP1_PAD_DRIVE_4MA;
+				break;
+			case 8:
+				arg = RP1_PAD_DRIVE_8MA;
+				break;
+			case 12:
+				arg = RP1_PAD_DRIVE_12MA;
+				break;
+			default:
+				return -ENOTSUPP;
+			}
+			regmap_field_write(pin->pad[RP1_PAD_DRIVE], arg);
+			break;
+
+		default:
+			return -ENOTSUPP;
+
+		} /* switch param type */
+	} /* for each config */
+
+	return 0;
+}
+
+static const struct of_device_id rp1_pinctrl_match[] = {
+	{ .compatible = "raspberrypi,rp1-gpio" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, rp1_pinctrl_match);
+
+static struct rp1_pinctrl rp1_pinctrl_data = {};
+
+static const struct regmap_config rp1_pinctrl_regmap_cfg = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.fast_io = true,
+	.name = "rp1-pinctrl",
+};
+
+static int rp1_gen_regfield(struct device *dev,
+			    const struct reg_field *array,
+			    size_t array_size,
+			    int reg_off,
+			    int pin_off,
+			    bool additive_offset,
+			    struct regmap *regmap,
+			    struct regmap_field *out[])
+{
+	struct reg_field regfield;
+	int k;
+
+	for (k = 0; k < array_size; k++) {
+		regfield = array[k];
+		regfield.reg = (additive_offset ? regfield.reg : 0) + reg_off;
+		if (pin_off >= 0) {
+			regfield.lsb = pin_off;
+			regfield.msb = regfield.lsb;
+		}
+		out[k] = devm_regmap_field_alloc(dev, regmap, regfield);
+
+		if (IS_ERR(out[k]))
+			return PTR_ERR(out[k]);
+	}
+
+	return 0;
+}
+
+static int rp1_pinctrl_probe(struct platform_device *pdev)
+{
+	struct regmap *gpio_regmap, *rio_regmap, *pads_regmap;
+	struct rp1_pinctrl *pc = &rp1_pinctrl_data;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct gpio_irq_chip *girq;
+	int err, i;
+
+	pc->dev = dev;
+	pc->gpio_chip = rp1_gpio_chip;
+	pc->gpio_chip.parent = dev;
+
+	pc->gpio_base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(pc->gpio_base))
+		return dev_err_probe(dev, PTR_ERR(pc->gpio_base), "could not get GPIO IO memory\n");
+
+	pc->rio_base = devm_platform_ioremap_resource(pdev, 1);
+	if (IS_ERR(pc->rio_base))
+		return dev_err_probe(dev, PTR_ERR(pc->rio_base), "could not get RIO IO memory\n");
+
+	pc->pads_base = devm_platform_ioremap_resource(pdev, 2);
+	if (IS_ERR(pc->pads_base))
+		return dev_err_probe(dev, PTR_ERR(pc->pads_base), "could not get PADS IO memory\n");
+
+	gpio_regmap = devm_regmap_init_mmio(dev, pc->gpio_base,
+					    &rp1_pinctrl_regmap_cfg);
+	if (IS_ERR(gpio_regmap))
+		return dev_err_probe(dev, PTR_ERR(gpio_regmap), "could not init GPIO regmap\n");
+
+	rio_regmap = devm_regmap_init_mmio(dev, pc->rio_base,
+					   &rp1_pinctrl_regmap_cfg);
+	if (IS_ERR(rio_regmap))
+		return dev_err_probe(dev, PTR_ERR(rio_regmap), "could not init RIO regmap\n");
+
+	pads_regmap = devm_regmap_init_mmio(dev, pc->pads_base,
+					    &rp1_pinctrl_regmap_cfg);
+	if (IS_ERR(pads_regmap))
+		return dev_err_probe(dev, PTR_ERR(pads_regmap), "could not init PADS regmap\n");
+
+	for (i = 0; i < RP1_NUM_BANKS; i++) {
+		const struct rp1_iobank_desc *bank = &rp1_iobanks[i];
+		int j;
+
+		for (j = 0; j < bank->num_gpios; j++) {
+			struct rp1_pin_info *pin =
+				&pc->pins[bank->min_gpio + j];
+			int reg_off;
+
+			pin->num = bank->min_gpio + j;
+			pin->bank = i;
+			pin->offset = j;
+
+			reg_off = bank->gpio_offset + pin->offset *
+				  sizeof(u32) * 2;
+			err = rp1_gen_regfield(dev,
+					       rp1_gpio_fields,
+					       ARRAY_SIZE(rp1_gpio_fields),
+					       reg_off,
+					       -1,
+					       true,
+					       gpio_regmap,
+					       pin->gpio);
+
+			if (err)
+				return dev_err_probe(dev, err,
+						     "Unable to allocate regmap for gpio\n");
+
+			reg_off = bank->inte_offset;
+			err = rp1_gen_regfield(dev,
+					       rp1_inte_fields,
+					       ARRAY_SIZE(rp1_inte_fields),
+					       reg_off,
+					       pin->offset,
+					       true,
+					       gpio_regmap,
+					       pin->inte);
+
+			if (err)
+				return dev_err_probe(dev, err,
+						     "Unable to allocate regmap for inte\n");
+
+			reg_off = bank->rio_offset;
+			err = rp1_gen_regfield(dev,
+					       rp1_rio_fields,
+					       ARRAY_SIZE(rp1_rio_fields),
+					       reg_off,
+					       pin->offset,
+					       true,
+					       rio_regmap,
+					       pin->rio);
+
+			if (err)
+				return dev_err_probe(dev, err,
+						     "Unable to allocate regmap for rio\n");
+
+			reg_off = bank->pads_offset + pin->offset * sizeof(u32);
+			err = rp1_gen_regfield(dev,
+					       rp1_pad_fields,
+					       ARRAY_SIZE(rp1_pad_fields),
+					       reg_off,
+					       -1,
+					       false,
+					       pads_regmap,
+					       pin->pad);
+
+			if (err)
+				return dev_err_probe(dev, err,
+						     "Unable to allocate regmap for pad\n");
+		}
+
+		raw_spin_lock_init(&pc->irq_lock[i]);
+	}
+
+	girq = &pc->gpio_chip.irq;
+	girq->chip = &rp1_gpio_irq_chip;
+	girq->parent_handler = rp1_gpio_irq_handler;
+	girq->num_parents = RP1_NUM_BANKS;
+	girq->parents = pc->irq;
+	girq->default_type = IRQ_TYPE_NONE;
+	girq->handler = handle_level_irq;
+
+	/*
+	 * Use the same handler for all groups: this is necessary
+	 * since we use one gpiochip to cover all lines - the
+	 * irq handler then needs to figure out which group and
+	 * bank that was firing the IRQ and look up the per-group
+	 * and bank data.
+	 */
+	for (i = 0; i < RP1_NUM_BANKS; i++) {
+		pc->irq[i] = irq_of_parse_and_map(np, i);
+		if (!pc->irq[i]) {
+			girq->num_parents = i;
+			break;
+		}
+	}
+
+	platform_set_drvdata(pdev, pc);
+
+	err = devm_gpiochip_add_data(dev, &pc->gpio_chip, pc);
+	if (err)
+		return dev_err_probe(dev, err, "could not add GPIO chip\n");
+
+	return 0;
+}
+
+static struct platform_driver rp1_pinctrl_driver = {
+	.probe = rp1_pinctrl_probe,
+	.driver = {
+		.name = MODULE_NAME,
+		.of_match_table = rp1_pinctrl_match,
+		.suppress_bind_attrs = true,
+	},
+};
+module_platform_driver(rp1_pinctrl_driver);
+
+MODULE_AUTHOR("Phil Elwell <phil@raspberrypi.com>");
+MODULE_AUTHOR("Andrea della Porta <andrea.porta@suse.com>");
+MODULE_DESCRIPTION("RP1 pinctrl/gpio driver");
+MODULE_LICENSE("GPL");
-- 
2.35.3

[PATCH v6 07/10] arm64: dts: rp1: Add support for RaspberryPi's RP1 device

[Andrea della Porta]

RaspberryPi RP1 is a multi function PCI endpoint device that
exposes several subperipherals via PCI BAR.
Add a dtb overlay that will be compiled into a binary blob
and linked in the RP1 driver.
This overlay offers just minimal support to represent the
RP1 device itself, the sub-peripherals will be added by
future patches.

Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
---
NOTE: this patch should be taken by the same maintainer that will take
"[PATCH v6 08/10] misc: rp1: RaspberryPi RP1 misc driver", since they
are closely related in terms of compiling.

 MAINTAINERS                           |  1 +
 arch/arm64/boot/dts/broadcom/rp1.dtso | 58 +++++++++++++++++++++++++++
 2 files changed, 59 insertions(+)
 create mode 100644 arch/arm64/boot/dts/broadcom/rp1.dtso

diff --git a/MAINTAINERS b/MAINTAINERS
index 9b8e87b8d6de..fbdd8594aa7e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19577,6 +19577,7 @@ F:	drivers/media/platform/raspberrypi/rp1-cfe/
 RASPBERRY PI RP1 PCI DRIVER
 M:	Andrea della Porta <andrea.porta@suse.com>
 S:	Maintained
+F:	arch/arm64/boot/dts/broadcom/rp1.dtso
 F:	Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml
 F:	Documentation/devicetree/bindings/misc/pci1de4,1.yaml
 F:	Documentation/devicetree/bindings/pci/pci-ep-bus.yaml
diff --git a/arch/arm64/boot/dts/broadcom/rp1.dtso b/arch/arm64/boot/dts/broadcom/rp1.dtso
new file mode 100644
index 000000000000..cdff061e2750
--- /dev/null
+++ b/arch/arm64/boot/dts/broadcom/rp1.dtso
@@ -0,0 +1,58 @@
+// SPDX-License-Identifier: (GPL-2.0 OR MIT)
+
+#include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/interrupt-controller/irq.h>
+#include <dt-bindings/clock/raspberrypi,rp1-clocks.h>
+
+/dts-v1/;
+/plugin/;
+
+/ {
+	fragment@0 {
+		target-path="";
+		__overlay__ {
+			compatible = "pci1de4,1";
+			#address-cells = <3>;
+			#size-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
+
+			pci_ep_bus: pci-ep-bus@1 {
+				compatible = "simple-bus";
+				ranges = <0x00 0x40000000  0x01 0x00 0x00000000  0x00 0x00400000>;
+				dma-ranges = <0x10 0x00000000  0x43000000 0x10 0x00000000  0x10 0x00000000>;
+				#address-cells = <2>;
+				#size-cells = <2>;
+
+				rp1_clocks: clocks@40018000 {
+					compatible = "raspberrypi,rp1-clocks";
+					reg = <0x00 0x40018000 0x0 0x10038>;
+					#clock-cells = <1>;
+					clocks = <&clk_rp1_xosc>;
+					assigned-clocks = <&rp1_clocks RP1_PLL_SYS_CORE>,
+							  <&rp1_clocks RP1_PLL_SYS>,
+							  <&rp1_clocks RP1_PLL_SYS_SEC>,
+							  <&rp1_clocks RP1_CLK_SYS>;
+					assigned-clock-rates = <1000000000>, // RP1_PLL_SYS_CORE
+							       <200000000>,  // RP1_PLL_SYS
+							       <125000000>,  // RP1_PLL_SYS_SEC
+							       <200000000>;  // RP1_CLK_SYS
+				};
+
+				rp1_gpio: pinctrl@400d0000 {
+					compatible = "raspberrypi,rp1-gpio";
+					reg = <0x00 0x400d0000  0x0 0xc000>,
+					      <0x00 0x400e0000  0x0 0xc000>,
+					      <0x00 0x400f0000  0x0 0xc000>;
+					gpio-controller;
+					#gpio-cells = <2>;
+					interrupt-controller;
+					#interrupt-cells = <2>;
+					interrupts = <0 IRQ_TYPE_LEVEL_HIGH>,
+						     <1 IRQ_TYPE_LEVEL_HIGH>,
+						     <2 IRQ_TYPE_LEVEL_HIGH>;
+				};
+			};
+		};
+	};
+};
-- 
2.35.3

[PATCH v6 08/10] misc: rp1: RaspberryPi RP1 misc driver

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

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    | 305 ++++++++++++++++++++++++++++++++++
 drivers/misc/rp1/rp1_pci.h    |  14 ++
 drivers/pci/quirks.c          |   1 +
 include/linux/pci_ids.h       |   3 +
 10 files changed, 358 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.
+ */
+
+#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..3e8ba3fa7fd5
--- /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>
+
+#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_END		61
+
+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		= RP1_DRIVER_NAME,
+	.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/misc/rp1/rp1_pci.h b/drivers/misc/rp1/rp1_pci.h
new file mode 100644
index 000000000000..7982f13bad9b
--- /dev/null
+++ b/drivers/misc/rp1/rp1_pci.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * Copyright (c) 2018-24 Raspberry Pi Ltd.
+ * All rights reserved.
+ */
+
+#ifndef _RP1_EXTERN_H_
+#define _RP1_EXTERN_H_
+
+extern char __dtbo_rp1_pci_begin[];
+extern char __dtbo_rp1_pci_end[];
+
+#endif
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 76f4df75b08a..e9a0fb707a4a 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -6269,6 +6269,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 d2402bf4aea2..9232b131a41e 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2615,6 +2615,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
-- 
2.35.3

[PATCH v6 09/10] arm64: dts: bcm2712: Add external clock for RP1 chipset on Rpi5

[Andrea della Porta]

The RP1 found on Raspberry Pi 5 board needs an external crystal at 50MHz.
Add clk_rp1_xosc node to provide that.

Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
---
 arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts b/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts
index 2bdbb6780242..ae3c0c153cea 100644
--- a/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts
+++ b/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts
@@ -16,6 +16,13 @@ chosen: chosen {
 		stdout-path = "serial10:115200n8";
 	};
 
+	clk_rp1_xosc: clock-50000000 {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-output-names = "rp1-xosc";
+		clock-frequency = <50000000>;
+	};
+
 	/* Will be filled by the bootloader */
 	memory@0 {
 		device_type = "memory";
-- 
2.35.3

[PATCH v6 10/10] arm64: defconfig: Enable RP1 misc/clock/gpio drivers

[Andrea della Porta]

Select the RP1 drivers needed to operate the PCI endpoint containing
several peripherals such as Ethernet and USB Controller. This chip is
present on RaspberryPi 5.

Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
Reviewed-by: Stefan Wahren <wahrenst@gmx.net>
---
 arch/arm64/configs/defconfig | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index c62831e61586..91b39026dc56 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -609,6 +609,7 @@ CONFIG_PINCTRL_QCM2290=y
 CONFIG_PINCTRL_QCS404=y
 CONFIG_PINCTRL_QDF2XXX=y
 CONFIG_PINCTRL_QDU1000=y
+CONFIG_PINCTRL_RP1=m
 CONFIG_PINCTRL_SA8775P=y
 CONFIG_PINCTRL_SC7180=y
 CONFIG_PINCTRL_SC7280=y
@@ -690,6 +691,7 @@ CONFIG_SENSORS_RASPBERRYPI_HWMON=m
 CONFIG_SENSORS_SL28CPLD=m
 CONFIG_SENSORS_INA2XX=m
 CONFIG_SENSORS_INA3221=m
+CONFIG_MISC_RP1=m
 CONFIG_THERMAL_GOV_POWER_ALLOCATOR=y
 CONFIG_CPU_THERMAL=y
 CONFIG_DEVFREQ_THERMAL=y
@@ -1272,6 +1274,7 @@ CONFIG_COMMON_CLK_CS2000_CP=y
 CONFIG_COMMON_CLK_FSL_SAI=y
 CONFIG_COMMON_CLK_S2MPS11=y
 CONFIG_COMMON_CLK_PWM=y
+CONFIG_COMMON_CLK_RP1=m
 CONFIG_COMMON_CLK_RS9_PCIE=y
 CONFIG_COMMON_CLK_VC3=y
 CONFIG_COMMON_CLK_VC5=y
-- 
2.35.3

Re: [PATCH v6 08/10] misc: rp1: RaspberryPi RP1 misc driver

[Greg Kroah-Hartman]

On Mon, Jan 13, 2025 at 03:58:07PM +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.
> 
> For reasons why this driver is contained in drivers/misc, please
> check the links.

Links aren't always around all the time, please document it here why
this is needed, and then links can "add to" that summary.

> 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    | 305 ++++++++++++++++++++++++++++++++++
>  drivers/misc/rp1/rp1_pci.h    |  14 ++
>  drivers/pci/quirks.c          |   1 +
>  include/linux/pci_ids.h       |   3 +
>  10 files changed, 358 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.
> + */
> +
> +#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..3e8ba3fa7fd5
> --- /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>
> +
> +#include "rp1_pci.h"

Why does a self-contained .c file need a .h file?  Please put it all in
here.

> +
> +#define RP1_DRIVER_NAME		"rp1"

KBUILD_MODNAME?

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



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

Do your writes need a read to flush them properly?  Or can they handle
this automatically?

thanks,

greg k-h

Re: [PATCH v6 02/10] dt-bindings: pinctrl: Add RaspberryPi RP1 gpio/pinctrl/pinmux bindings

[Florian Fainelli]

On 1/13/2025 6:58 AM, Andrea della Porta wrote:
> Add device tree bindings for the gpio/pin/mux controller that is part of
> the RP1 multi function device, and relative entries in MAINTAINERS file.
> 
> Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH v6 03/10] dt-bindings: pci: Add common schema for devices accessible through PCI BARs

[Florian Fainelli]

On 1/13/2025 6:58 AM, Andrea della Porta wrote:
> Common YAML schema for devices that exports internal peripherals through
> PCI BARs. The BARs are exposed as simple-buses through which the
> peripherals can be accessed.
> 
> This is not intended to be used as a standalone binding, but should be
> included by device specific bindings.
> 
> Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH v6 01/10] dt-bindings: clock: Add RaspberryPi RP1 clock bindings

[Florian Fainelli]

On 1/13/2025 6:58 AM, Andrea della Porta wrote:
> Add device tree bindings for the clock generator found in RP1 multi
> function device, and relative entries in MAINTAINERS file.
> 
> Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
>   .../clock/raspberrypi,rp1-clocks.yaml         | 58 ++++++++++++++++++
>   MAINTAINERS                                   |  6 ++
>   .../clock/raspberrypi,rp1-clocks.h            | 61 +++++++++++++++++++
>   3 files changed, 125 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml
>   create mode 100644 include/dt-bindings/clock/raspberrypi,rp1-clocks.h
> 
> diff --git a/Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml b/Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml
> new file mode 100644
> index 000000000000..b2670cf7403a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml
> @@ -0,0 +1,58 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/raspberrypi,rp1-clocks.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: RaspberryPi RP1 clock generator
> +
> +maintainers:
> +  - Andrea della Porta <andrea.porta@suse.com>
> +
> +description: |
> +  The RP1 contains a clock generator designed as three PLLs (CORE, AUDIO,
> +  VIDEO), and each PLL output can be programmed though dividers to generate
> +  the clocks to drive the sub-peripherals embedded inside the chipset.

typo: though -> through

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH v6 09/10] arm64: dts: bcm2712: Add external clock for RP1 chipset on Rpi5

[Florian Fainelli]

On 1/13/2025 6:58 AM, Andrea della Porta wrote:
> The RP1 found on Raspberry Pi 5 board needs an external crystal at 50MHz.
> Add clk_rp1_xosc node to provide that.
> 
> Signed-off-by: Andrea della Porta <andrea.porta@suse.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>

I don't expect conflicts so this could go through the tree that will 
merge the misc driver (Greg?).
-- 
Florian

Re: [PATCH v6 07/10] arm64: dts: rp1: Add support for RaspberryPi's RP1 device

[Florian Fainelli]

On 1/13/2025 6:58 AM, Andrea della Porta wrote:
> RaspberryPi RP1 is a multi function PCI endpoint device that
> exposes several subperipherals via PCI BAR.
> Add a dtb overlay that will be compiled into a binary blob
> and linked in the RP1 driver.
> This overlay offers just minimal support to represent the
> RP1 device itself, the sub-peripherals will be added by
> future patches.
> 
> Signed-off-by: Andrea della Porta <andrea.porta@suse.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>

Same comment as patch 9, I am happy if Greg takes this patch directly 
through his tree.
-- 
Florian

Re: [PATCH v6 04/10] dt-bindings: misc: Add device specific bindings for RaspberryPi RP1

[Florian Fainelli]

On 1/13/2025 6:58 AM, Andrea della Porta wrote:
> The RP1 is a MFD that exposes its peripherals through PCI BARs. This
> schema is intended as minimal support for the clock generator and
> gpio controller peripherals which are accessible through BAR1.
> 
> Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH v6 08/10] misc: rp1: RaspberryPi RP1 misc driver

[Andrea della Porta]

Hi Greg,

On 12:47 Fri 17 Jan     , Greg Kroah-Hartman wrote:
> On Mon, Jan 13, 2025 at 03:58:07PM +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.
> > 
> > For reasons why this driver is contained in drivers/misc, please
> > check the links.
> 
> Links aren't always around all the time, please document it here why
> this is needed, and then links can "add to" that summary.

Ack.

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

...

> > diff --git a/drivers/misc/rp1/rp1_pci.c b/drivers/misc/rp1/rp1_pci.c
> > new file mode 100644
> > index 000000000000..3e8ba3fa7fd5
> > --- /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>
> > +
> > +#include "rp1_pci.h"
> 
> Why does a self-contained .c file need a .h file?  Please put it all in
> here.

I agree with you. Indeed, the very first version of this patch had the header
file placed inside the .c, but I received concerns about it and some advice to
do it differently, as you can see here:
https://lore.kernel.org/all/ZtWDpaqUG9d9yPPf@apocalypse/
so I've changed it accordingly in V2. So right now I'm not sure what the
acceptable behaviour should be ...

> 
> > +
> > +#define RP1_DRIVER_NAME		"rp1"
> 
> KBUILD_MODNAME?

Right. Thanks for pointing that out.

> 
> > +
> > +#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
> 
> 
> 
> > +
> > +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));
> 
> Do your writes need a read to flush them properly?  Or can they handle
> this automatically?
>

I had some thoughts with RaspberryPi foundation folks to double check it, and it
seems that there should be no need to readback the value (unless we want to go
really paranoid), so I would avoid that since in case of level handled interrupt
we would end up reading the register on every triggering interrupts.

Many thanks,
Andrea
 
> thanks,
> 
> greg k-h

Re: [PATCH v6 08/10] misc: rp1: RaspberryPi RP1 misc driver

[Greg Kroah-Hartman]

On Tue, Jan 21, 2025 at 09:43:37AM +0100, Andrea della Porta wrote:
> Hi Greg,
> 
> On 12:47 Fri 17 Jan     , Greg Kroah-Hartman wrote:
> > On Mon, Jan 13, 2025 at 03:58:07PM +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.
> > > 
> > > For reasons why this driver is contained in drivers/misc, please
> > > check the links.
> > 
> > Links aren't always around all the time, please document it here why
> > this is needed, and then links can "add to" that summary.
> 
> Ack.
> 
> > 
> > > 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
> 
> ...
> 
> > > diff --git a/drivers/misc/rp1/rp1_pci.c b/drivers/misc/rp1/rp1_pci.c
> > > new file mode 100644
> > > index 000000000000..3e8ba3fa7fd5
> > > --- /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>
> > > +
> > > +#include "rp1_pci.h"
> > 
> > Why does a self-contained .c file need a .h file?  Please put it all in
> > here.
> 
> I agree with you. Indeed, the very first version of this patch had the header
> file placed inside the .c, but I received concerns about it and some advice to
> do it differently, as you can see here:
> https://lore.kernel.org/all/ZtWDpaqUG9d9yPPf@apocalypse/
> so I've changed it accordingly in V2. So right now I'm not sure what the
> acceptable behaviour should be ...

It's a pretty simple rule:
	Only use a .h file if multiple .c files need to see the symbol.

So no .h file is needed here.

> > > +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));
> > 
> > Do your writes need a read to flush them properly?  Or can they handle
> > this automatically?
> >
> 
> I had some thoughts with RaspberryPi foundation folks to double check it, and it
> seems that there should be no need to readback the value (unless we want to go
> really paranoid), so I would avoid that since in case of level handled interrupt
> we would end up reading the register on every triggering interrupts.

Ok, if it passes testing, that's fine, hopefully it works properly, but
if not, you now have a trail to go and fix it in the future :)

thanks,

greg k-h

Re: [PATCH v6 08/10] misc: rp1: RaspberryPi RP1 misc driver

[Andrea della Porta]

Hi Greg,

On 09:48 Tue 21 Jan     , Greg Kroah-Hartman wrote:
> On Tue, Jan 21, 2025 at 09:43:37AM +0100, Andrea della Porta wrote:
> > Hi Greg,
> > 
> > On 12:47 Fri 17 Jan     , Greg Kroah-Hartman wrote:
> > > On Mon, Jan 13, 2025 at 03:58:07PM +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.
> > > > 
> > > > For reasons why this driver is contained in drivers/misc, please
> > > > check the links.
> > > 
> > > Links aren't always around all the time, please document it here why
> > > this is needed, and then links can "add to" that summary.
> > 
> > Ack.
> > 
> > > 
> > > > 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
> > 
> > ...
> > 
> > > > diff --git a/drivers/misc/rp1/rp1_pci.c b/drivers/misc/rp1/rp1_pci.c
> > > > new file mode 100644
> > > > index 000000000000..3e8ba3fa7fd5
> > > > --- /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>
> > > > +
> > > > +#include "rp1_pci.h"
> > > 
> > > Why does a self-contained .c file need a .h file?  Please put it all in
> > > here.
> > 
> > I agree with you. Indeed, the very first version of this patch had the header
> > file placed inside the .c, but I received concerns about it and some advice to
> > do it differently, as you can see here:
> > https://lore.kernel.org/all/ZtWDpaqUG9d9yPPf@apocalypse/
> > so I've changed it accordingly in V2. So right now I'm not sure what the
> > acceptable behaviour should be ...
> 
> It's a pretty simple rule:
> 	Only use a .h file if multiple .c files need to see the symbol.
> 
> So no .h file is needed here.

Perfect, I'll revert back that two lines to V1 then. Please be aware
though that this will trigger the following checkpatch warning:

WARNING: externs should be avoided in .c files

> 
> > > > +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));
> > > 
> > > Do your writes need a read to flush them properly?  Or can they handle
> > > this automatically?
> > >
> > 
> > I had some thoughts with RaspberryPi foundation folks to double check it, and it
> > seems that there should be no need to readback the value (unless we want to go
> > really paranoid), so I would avoid that since in case of level handled interrupt
> > we would end up reading the register on every triggering interrupts.
> 
> Ok, if it passes testing, that's fine, hopefully it works properly, but
> if not, you now have a trail to go and fix it in the future :)

Sure :)

Many thanks,
Andrea

> 
> thanks,
> 
> greg k-h

Re: [PATCH v6 08/10] misc: rp1: RaspberryPi RP1 misc driver

[Greg Kroah-Hartman]

On Tue, Jan 21, 2025 at 02:59:21PM +0100, Andrea della Porta wrote:
> Hi Greg,
> 
> On 09:48 Tue 21 Jan     , Greg Kroah-Hartman wrote:
> > On Tue, Jan 21, 2025 at 09:43:37AM +0100, Andrea della Porta wrote:
> > > Hi Greg,
> > > 
> > > On 12:47 Fri 17 Jan     , Greg Kroah-Hartman wrote:
> > > > On Mon, Jan 13, 2025 at 03:58:07PM +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.
> > > > > 
> > > > > For reasons why this driver is contained in drivers/misc, please
> > > > > check the links.
> > > > 
> > > > Links aren't always around all the time, please document it here why
> > > > this is needed, and then links can "add to" that summary.
> > > 
> > > Ack.
> > > 
> > > > 
> > > > > 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
> > > 
> > > ...
> > > 
> > > > > diff --git a/drivers/misc/rp1/rp1_pci.c b/drivers/misc/rp1/rp1_pci.c
> > > > > new file mode 100644
> > > > > index 000000000000..3e8ba3fa7fd5
> > > > > --- /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>
> > > > > +
> > > > > +#include "rp1_pci.h"
> > > > 
> > > > Why does a self-contained .c file need a .h file?  Please put it all in
> > > > here.
> > > 
> > > I agree with you. Indeed, the very first version of this patch had the header
> > > file placed inside the .c, but I received concerns about it and some advice to
> > > do it differently, as you can see here:
> > > https://lore.kernel.org/all/ZtWDpaqUG9d9yPPf@apocalypse/
> > > so I've changed it accordingly in V2. So right now I'm not sure what the
> > > acceptable behaviour should be ...
> > 
> > It's a pretty simple rule:
> > 	Only use a .h file if multiple .c files need to see the symbol.
> > 
> > So no .h file is needed here.
> 
> Perfect, I'll revert back that two lines to V1 then. Please be aware
> though that this will trigger the following checkpatch warning:
> 
> WARNING: externs should be avoided in .c files

Well where are those externs defined at?  Shouldn't there be a .h file
for them somewhere in the tree if they really are global?

thanks,

greg k-h

Re: [PATCH v6 08/10] misc: rp1: RaspberryPi RP1 misc driver

[Andrea della Porta]

Hi Greg,

On 15:18 Tue 21 Jan     , Greg Kroah-Hartman wrote:
> On Tue, Jan 21, 2025 at 02:59:21PM +0100, Andrea della Porta wrote:
> > Hi Greg,
> > 
> > On 09:48 Tue 21 Jan     , Greg Kroah-Hartman wrote:
> > > On Tue, Jan 21, 2025 at 09:43:37AM +0100, Andrea della Porta wrote:
> > > > Hi Greg,
> > > > 
> > > > On 12:47 Fri 17 Jan     , Greg Kroah-Hartman wrote:
> > > > > On Mon, Jan 13, 2025 at 03:58:07PM +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.
> > > > > > 
> > > > > > For reasons why this driver is contained in drivers/misc, please
> > > > > > check the links.
> > > > > 
> > > > > Links aren't always around all the time, please document it here why
> > > > > this is needed, and then links can "add to" that summary.
> > > > 
> > > > Ack.
> > > > 
> > > > > 
> > > > > > 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
> > > > 
> > > > ...
> > > > 
> > > > > > diff --git a/drivers/misc/rp1/rp1_pci.c b/drivers/misc/rp1/rp1_pci.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..3e8ba3fa7fd5
> > > > > > --- /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>
> > > > > > +
> > > > > > +#include "rp1_pci.h"
> > > > > 
> > > > > Why does a self-contained .c file need a .h file?  Please put it all in
> > > > > here.
> > > > 
> > > > I agree with you. Indeed, the very first version of this patch had the header
> > > > file placed inside the .c, but I received concerns about it and some advice to
> > > > do it differently, as you can see here:
> > > > https://lore.kernel.org/all/ZtWDpaqUG9d9yPPf@apocalypse/
> > > > so I've changed it accordingly in V2. So right now I'm not sure what the
> > > > acceptable behaviour should be ...
> > > 
> > > It's a pretty simple rule:
> > > 	Only use a .h file if multiple .c files need to see the symbol.
> > > 
> > > So no .h file is needed here.
> > 
> > Perfect, I'll revert back that two lines to V1 then. Please be aware
> > though that this will trigger the following checkpatch warning:
> > 
> > WARNING: externs should be avoided in .c files
> 
> Well where are those externs defined at?  Shouldn't there be a .h file
> for them somewhere in the tree if they really are global?

Those symbols are deined in drivers/misc/rp1/rp1-pci.dtbo.S (added by
this patchset) and created by cmd_wrap_S_dtb in scripts/Makefile.lib.
They are just placeholders that contains rp1-pci.dtbo as
a binary blob, in order for the driver (rp1_pci.c) to be able to use
the binary buffer representing the overlay and address it from the
driver probe function.
So there's no other reference from outside rp1_pci.c to those two symbols.
In comparison, this is the very same approach used by a recently accepted
patch involving drivers/misc/lan966x_pci.c, which also has the two externs
in it and triggers the same checkpatch warning.

Many thanks,
Andrea

> 
> thanks,
> 
> greg k-h

Re: [PATCH v6 08/10] misc: rp1: RaspberryPi RP1 misc driver

[Greg Kroah-Hartman]

On Tue, Jan 21, 2025 at 03:38:42PM +0100, Andrea della Porta wrote:
> Hi Greg,
> 
> On 15:18 Tue 21 Jan     , Greg Kroah-Hartman wrote:
> > On Tue, Jan 21, 2025 at 02:59:21PM +0100, Andrea della Porta wrote:
> > > Hi Greg,
> > > 
> > > On 09:48 Tue 21 Jan     , Greg Kroah-Hartman wrote:
> > > > On Tue, Jan 21, 2025 at 09:43:37AM +0100, Andrea della Porta wrote:
> > > > > Hi Greg,
> > > > > 
> > > > > On 12:47 Fri 17 Jan     , Greg Kroah-Hartman wrote:
> > > > > > On Mon, Jan 13, 2025 at 03:58:07PM +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.
> > > > > > > 
> > > > > > > For reasons why this driver is contained in drivers/misc, please
> > > > > > > check the links.
> > > > > > 
> > > > > > Links aren't always around all the time, please document it here why
> > > > > > this is needed, and then links can "add to" that summary.
> > > > > 
> > > > > Ack.
> > > > > 
> > > > > > 
> > > > > > > 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
> > > > > 
> > > > > ...
> > > > > 
> > > > > > > diff --git a/drivers/misc/rp1/rp1_pci.c b/drivers/misc/rp1/rp1_pci.c
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..3e8ba3fa7fd5
> > > > > > > --- /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>
> > > > > > > +
> > > > > > > +#include "rp1_pci.h"
> > > > > > 
> > > > > > Why does a self-contained .c file need a .h file?  Please put it all in
> > > > > > here.
> > > > > 
> > > > > I agree with you. Indeed, the very first version of this patch had the header
> > > > > file placed inside the .c, but I received concerns about it and some advice to
> > > > > do it differently, as you can see here:
> > > > > https://lore.kernel.org/all/ZtWDpaqUG9d9yPPf@apocalypse/
> > > > > so I've changed it accordingly in V2. So right now I'm not sure what the
> > > > > acceptable behaviour should be ...
> > > > 
> > > > It's a pretty simple rule:
> > > > 	Only use a .h file if multiple .c files need to see the symbol.
> > > > 
> > > > So no .h file is needed here.
> > > 
> > > Perfect, I'll revert back that two lines to V1 then. Please be aware
> > > though that this will trigger the following checkpatch warning:
> > > 
> > > WARNING: externs should be avoided in .c files
> > 
> > Well where are those externs defined at?  Shouldn't there be a .h file
> > for them somewhere in the tree if they really are global?
> 
> Those symbols are deined in drivers/misc/rp1/rp1-pci.dtbo.S (added by
> this patchset) and created by cmd_wrap_S_dtb in scripts/Makefile.lib.
> They are just placeholders that contains rp1-pci.dtbo as
> a binary blob, in order for the driver (rp1_pci.c) to be able to use
> the binary buffer representing the overlay and address it from the
> driver probe function.
> So there's no other reference from outside rp1_pci.c to those two symbols.
> In comparison, this is the very same approach used by a recently accepted
> patch involving drivers/misc/lan966x_pci.c, which also has the two externs
> in it and triggers the same checkpatch warning.

Ok, that's fine, checkpatch is just a hint, not a hard-and-fast-rule.

thanks,

greg k-h

Re: [PATCH v6 08/10] misc: rp1: RaspberryPi RP1 misc driver

[Andrew Lunn]

> > Well where are those externs defined at?  Shouldn't there be a .h file
> > for them somewhere in the tree if they really are global?
> 
> Those symbols are deined in drivers/misc/rp1/rp1-pci.dtbo.S (added by
> this patchset) and created by cmd_wrap_S_dtb in scripts/Makefile.lib.

Could cmd_wrap_S_dtb be extended to actually produce a header?

But that might confuse static analysers?

	Andrew

Re: [PATCH v6 08/10] misc: rp1: RaspberryPi RP1 misc driver

[Andrea della Porta]

Hi Andrew,

On 15:53 Tue 21 Jan     , Andrew Lunn wrote:
> > > Well where are those externs defined at?  Shouldn't there be a .h file
> > > for them somewhere in the tree if they really are global?
> > 
> > Those symbols are deined in drivers/misc/rp1/rp1-pci.dtbo.S (added by
> > this patchset) and created by cmd_wrap_S_dtb in scripts/Makefile.lib.
> 
> Could cmd_wrap_S_dtb be extended to actually produce a header?

It will produce a header nonetheless, one which is needed by just one .c
file violating Greg's rule about having a header only if referenced by more
than one source and the symbol is truly global, IMHO.

Thanks,
Andrea

> 
> But that might confuse static analysers?
> 
> 	Andrew

Re: [PATCH v6 08/10] misc: rp1: RaspberryPi RP1 misc driver

[Herve Codina]

Hi Andrea,

On Tue, 21 Jan 2025 15:49:04 +0100
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Tue, Jan 21, 2025 at 03:38:42PM +0100, Andrea della Porta wrote:
> > Hi Greg,
> > 
> > On 15:18 Tue 21 Jan     , Greg Kroah-Hartman wrote:  
> > > On Tue, Jan 21, 2025 at 02:59:21PM +0100, Andrea della Porta wrote:  
> > > > Hi Greg,
> > > > 
> > > > On 09:48 Tue 21 Jan     , Greg Kroah-Hartman wrote:  
> > > > > On Tue, Jan 21, 2025 at 09:43:37AM +0100, Andrea della Porta wrote:  
> > > > > > Hi Greg,
> > > > > > 
> > > > > > On 12:47 Fri 17 Jan     , Greg Kroah-Hartman wrote:  
> > > > > > > On Mon, Jan 13, 2025 at 03:58:07PM +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.
> > > > > > > > 
> > > > > > > > For reasons why this driver is contained in drivers/misc, please
> > > > > > > > check the links.  
> > > > > > > 
> > > > > > > Links aren't always around all the time, please document it here why
> > > > > > > this is needed, and then links can "add to" that summary.  
> > > > > > 
> > > > > > Ack.
> > > > > >   
> > > > > > >   
> > > > > > > > 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  
> > > > > > 
> > > > > > ...
> > > > > >   
> > > > > > > > diff --git a/drivers/misc/rp1/rp1_pci.c b/drivers/misc/rp1/rp1_pci.c
> > > > > > > > new file mode 100644
> > > > > > > > index 000000000000..3e8ba3fa7fd5
> > > > > > > > --- /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>
> > > > > > > > +
> > > > > > > > +#include "rp1_pci.h"  
> > > > > > > 
> > > > > > > Why does a self-contained .c file need a .h file?  Please put it all in
> > > > > > > here.  
> > > > > > 
> > > > > > I agree with you. Indeed, the very first version of this patch had the header
> > > > > > file placed inside the .c, but I received concerns about it and some advice to
> > > > > > do it differently, as you can see here:
> > > > > > https://lore.kernel.org/all/ZtWDpaqUG9d9yPPf@apocalypse/
> > > > > > so I've changed it accordingly in V2. So right now I'm not sure what the
> > > > > > acceptable behaviour should be ...  
> > > > > 
> > > > > It's a pretty simple rule:
> > > > > 	Only use a .h file if multiple .c files need to see the symbol.
> > > > > 
> > > > > So no .h file is needed here.  
> > > > 
> > > > Perfect, I'll revert back that two lines to V1 then. Please be aware
> > > > though that this will trigger the following checkpatch warning:
> > > > 
> > > > WARNING: externs should be avoided in .c files  
> > > 
> > > Well where are those externs defined at?  Shouldn't there be a .h file
> > > for them somewhere in the tree if they really are global?  
> > 
> > Those symbols are deined in drivers/misc/rp1/rp1-pci.dtbo.S (added by
> > this patchset) and created by cmd_wrap_S_dtb in scripts/Makefile.lib.
> > They are just placeholders that contains rp1-pci.dtbo as
> > a binary blob, in order for the driver (rp1_pci.c) to be able to use
> > the binary buffer representing the overlay and address it from the
> > driver probe function.
> > So there's no other reference from outside rp1_pci.c to those two symbols.
> > In comparison, this is the very same approach used by a recently accepted
> > patch involving drivers/misc/lan966x_pci.c, which also has the two externs
> > in it and triggers the same checkpatch warning.  
> 
> Ok, that's fine, checkpatch is just a hint, not a hard-and-fast-rule.
> 

Maybe just to avoid confusion for future readers, you can add a comment as I
did for the lan966x:
  https://elixir.bootlin.com/linux/v6.13-rc3/source/drivers/misc/lan966x_pci.c#L21

This will not avoid the warning but will give an explanation to people
looking closer at this checkpatch warning.

Best regards,
Hervé

Re: [PATCH v6 08/10] misc: rp1: RaspberryPi RP1 misc driver

[Andrea della Porta]

Hi Herve,

On 16:15 Tue 21 Jan     , Herve Codina wrote:
> Hi Andrea,
> 
> On Tue, 21 Jan 2025 15:49:04 +0100
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> > On Tue, Jan 21, 2025 at 03:38:42PM +0100, Andrea della Porta wrote:
> > > Hi Greg,
> > > 
> > > On 15:18 Tue 21 Jan     , Greg Kroah-Hartman wrote:  
> > > > On Tue, Jan 21, 2025 at 02:59:21PM +0100, Andrea della Porta wrote:  
> > > > > Hi Greg,
> > > > > 
> > > > > On 09:48 Tue 21 Jan     , Greg Kroah-Hartman wrote:  
> > > > > > On Tue, Jan 21, 2025 at 09:43:37AM +0100, Andrea della Porta wrote:  
> > > > > > > Hi Greg,
> > > > > > > 
> > > > > > > On 12:47 Fri 17 Jan     , Greg Kroah-Hartman wrote:  
> > > > > > > > On Mon, Jan 13, 2025 at 03:58:07PM +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.
> > > > > > > > > 
> > > > > > > > > For reasons why this driver is contained in drivers/misc, please
> > > > > > > > > check the links.  
> > > > > > > > 
> > > > > > > > Links aren't always around all the time, please document it here why
> > > > > > > > this is needed, and then links can "add to" that summary.  
> > > > > > > 
> > > > > > > Ack.
> > > > > > >   
> > > > > > > >   
> > > > > > > > > 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  
> > > > > > > 
> > > > > > > ...
> > > > > > >   
> > > > > > > > > diff --git a/drivers/misc/rp1/rp1_pci.c b/drivers/misc/rp1/rp1_pci.c
> > > > > > > > > new file mode 100644
> > > > > > > > > index 000000000000..3e8ba3fa7fd5
> > > > > > > > > --- /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>
> > > > > > > > > +
> > > > > > > > > +#include "rp1_pci.h"  
> > > > > > > > 
> > > > > > > > Why does a self-contained .c file need a .h file?  Please put it all in
> > > > > > > > here.  
> > > > > > > 
> > > > > > > I agree with you. Indeed, the very first version of this patch had the header
> > > > > > > file placed inside the .c, but I received concerns about it and some advice to
> > > > > > > do it differently, as you can see here:
> > > > > > > https://lore.kernel.org/all/ZtWDpaqUG9d9yPPf@apocalypse/
> > > > > > > so I've changed it accordingly in V2. So right now I'm not sure what the
> > > > > > > acceptable behaviour should be ...  
> > > > > > 
> > > > > > It's a pretty simple rule:
> > > > > > 	Only use a .h file if multiple .c files need to see the symbol.
> > > > > > 
> > > > > > So no .h file is needed here.  
> > > > > 
> > > > > Perfect, I'll revert back that two lines to V1 then. Please be aware
> > > > > though that this will trigger the following checkpatch warning:
> > > > > 
> > > > > WARNING: externs should be avoided in .c files  
> > > > 
> > > > Well where are those externs defined at?  Shouldn't there be a .h file
> > > > for them somewhere in the tree if they really are global?  
> > > 
> > > Those symbols are deined in drivers/misc/rp1/rp1-pci.dtbo.S (added by
> > > this patchset) and created by cmd_wrap_S_dtb in scripts/Makefile.lib.
> > > They are just placeholders that contains rp1-pci.dtbo as
> > > a binary blob, in order for the driver (rp1_pci.c) to be able to use
> > > the binary buffer representing the overlay and address it from the
> > > driver probe function.
> > > So there's no other reference from outside rp1_pci.c to those two symbols.
> > > In comparison, this is the very same approach used by a recently accepted
> > > patch involving drivers/misc/lan966x_pci.c, which also has the two externs
> > > in it and triggers the same checkpatch warning.  
> > 
> > Ok, that's fine, checkpatch is just a hint, not a hard-and-fast-rule.
> > 
> 
> Maybe just to avoid confusion for future readers, you can add a comment as I
> did for the lan966x:
>   https://elixir.bootlin.com/linux/v6.13-rc3/source/drivers/misc/lan966x_pci.c#L21
> 
> This will not avoid the warning but will give an explanation to people
> looking closer at this checkpatch warning.

Good advice, thanks Herve! Added...

Regards,
Andrea

> 
> Best regards,
> Hervé

Re: [PATCH v6 05/10] clk: rp1: Add support for clocks provided by RP1

[Bjorn Helgaas]

On Mon, Jan 13, 2025 at 03:58:04PM +0100, Andrea della Porta wrote:
> RaspberryPi RP1 is an MFD providing, among other peripherals, several
> clock generators and PLLs that drives the sub-peripherals.
> Add the driver to support the clock providers.

> +#define PLL_PRIM_DIV1_SHIFT		16
> +#define PLL_PRIM_DIV1_WIDTH		3
> +#define PLL_PRIM_DIV1_MASK		GENMASK(PLL_PRIM_DIV1_SHIFT + \
> +						PLL_PRIM_DIV1_WIDTH - 1, \
> +						PLL_PRIM_DIV1_SHIFT)
> +
> +#define PLL_PRIM_DIV2_SHIFT          12
> +#define PLL_PRIM_DIV2_WIDTH          3
> +#define PLL_PRIM_DIV2_MASK           GENMASK(PLL_PRIM_DIV2_SHIFT + \
> +                                             PLL_PRIM_DIV2_WIDTH - 1, \
> +                                             PLL_PRIM_DIV2_SHIFT)

Maybe this is standard drivers/clk style, but this seems like overkill
to me.  I think this would be sufficient and easier to read:

  #define PLL_PRIM_DIV1_MASK   GENMASK(18, 16)
  #define PLL_PRIM_DIV2_MASK   GENMASK(14, 12)

> +static unsigned long rp1_pll_recalc_rate(struct clk_hw *hw,
> +					 unsigned long parent_rate)
> +{
> +	struct rp1_clk_desc *pll = container_of(hw, struct rp1_clk_desc, hw);
> +	struct rp1_clockman *clockman = pll->clockman;
> +	const struct rp1_pll_data *data = pll->data;
> +	u32 prim, prim_div1, prim_div2;
> +
> +	prim = clockman_read(clockman, data->ctrl_reg);
> +	prim_div1 = (prim & PLL_PRIM_DIV1_MASK) >> PLL_PRIM_DIV1_SHIFT;
> +	prim_div2 = (prim & PLL_PRIM_DIV2_MASK) >> PLL_PRIM_DIV2_SHIFT;

And then here, I think you can just use FIELD_GET():

  prim_div1 = FIELD_GET(PLL_PRIM_DIV1_MASK, prim);
  prim_div2 = FIELD_GET(PLL_PRIM_DIV2_MASK, prim);

It looks like the same could be done for PLL_SEC_DIV_MASK,
PLL_CS_REFDIV_SHIFT, PLL_PH_PHASE_SHIFT, CLK_CTRL_AUXSRC_MASK, etc.

Re: [PATCH v6 05/10] clk: rp1: Add support for clocks provided by RP1

[Andrea della Porta]

Hi Bjorn,

On 17:44 Mon 03 Feb     , Bjorn Helgaas wrote:
> On Mon, Jan 13, 2025 at 03:58:04PM +0100, Andrea della Porta wrote:
> > RaspberryPi RP1 is an MFD providing, among other peripherals, several
> > clock generators and PLLs that drives the sub-peripherals.
> > Add the driver to support the clock providers.
> 
> > +#define PLL_PRIM_DIV1_SHIFT		16
> > +#define PLL_PRIM_DIV1_WIDTH		3
> > +#define PLL_PRIM_DIV1_MASK		GENMASK(PLL_PRIM_DIV1_SHIFT + \
> > +						PLL_PRIM_DIV1_WIDTH - 1, \
> > +						PLL_PRIM_DIV1_SHIFT)
> > +
> > +#define PLL_PRIM_DIV2_SHIFT          12
> > +#define PLL_PRIM_DIV2_WIDTH          3
> > +#define PLL_PRIM_DIV2_MASK           GENMASK(PLL_PRIM_DIV2_SHIFT + \
> > +                                             PLL_PRIM_DIV2_WIDTH - 1, \
> > +                                             PLL_PRIM_DIV2_SHIFT)
> 
> Maybe this is standard drivers/clk style, but this seems like overkill
> to me.  I think this would be sufficient and easier to read:
> 
>   #define PLL_PRIM_DIV1_MASK   GENMASK(18, 16)
>   #define PLL_PRIM_DIV2_MASK   GENMASK(14, 12)

Ack.

> 
> > +static unsigned long rp1_pll_recalc_rate(struct clk_hw *hw,
> > +					 unsigned long parent_rate)
> > +{
> > +	struct rp1_clk_desc *pll = container_of(hw, struct rp1_clk_desc, hw);
> > +	struct rp1_clockman *clockman = pll->clockman;
> > +	const struct rp1_pll_data *data = pll->data;
> > +	u32 prim, prim_div1, prim_div2;
> > +
> > +	prim = clockman_read(clockman, data->ctrl_reg);
> > +	prim_div1 = (prim & PLL_PRIM_DIV1_MASK) >> PLL_PRIM_DIV1_SHIFT;
> > +	prim_div2 = (prim & PLL_PRIM_DIV2_MASK) >> PLL_PRIM_DIV2_SHIFT;
> 
> And then here, I think you can just use FIELD_GET():
> 
>   prim_div1 = FIELD_GET(PLL_PRIM_DIV1_MASK, prim);
>   prim_div2 = FIELD_GET(PLL_PRIM_DIV2_MASK, prim);
> 
> It looks like the same could be done for PLL_SEC_DIV_MASK,
> PLL_CS_REFDIV_SHIFT, PLL_PH_PHASE_SHIFT, CLK_CTRL_AUXSRC_MASK, etc.

Ack.

Regards,
Andrea

Re: [PATCH v6 00/10] Add support for RaspberryPi RP1 PCI device using a DT overlay

[Herve Codina]

Hi Phil,

On Thu, 13 Feb 2025 15:18:45 +0000
Phil Elwell <phil@raspberrypi.com> wrote:

> Hi Andrea,
> 
> The problem with this approach (loading an overlay from the RP1 PCIe
> driver), and it's one that I have raised with you offline, is that
> (unless anyone can prove otherwise) it becomes impossible to create a
> Pi 5 DTS file which makes use of the RP1's resources. How do you
> declare something as simple as a button wired to an RP1 GPIO, or fan
> connected to a PWM output?

The driver could be improved in a second step.
For instance, it could load the dtbo from user-space using request_firmare()
instead of loading the embedded dtbo.

> 
> If this is the preferred route to upstream adoption, I would prefer it
> if rp1.dtso could be split in two - an rp1.dtsi similar to what we
> have downstream, and an rp1.dtso that #includes it. In this way we can
> keep the patching and duplication to a minimum.

Indeed, having a rp1.dtsi avoid duplication but how the rp1.dtso in
the the kernel sources could include user customization (button, fan, ...)
without being modified ?
At least we have to '#include <my_rp1_customizations.dtsi>'.

Requesting the dtbo from user-space allows to let the user to create
its own dtso without the need to modify the one in kernel sources.

Does it make sense ?

Best regards,
Hervé

Re: [PATCH v6 00/10] Add support for RaspberryPi RP1 PCI device using a DT overlay

[Andrew Lunn]

On Thu, Feb 13, 2025 at 05:14:35PM +0100, Herve Codina wrote:
> Hi Phil,
> 
> On Thu, 13 Feb 2025 15:18:45 +0000
> Phil Elwell <phil@raspberrypi.com> wrote:
> 
> > Hi Andrea,
> > 
> > The problem with this approach (loading an overlay from the RP1 PCIe
> > driver), and it's one that I have raised with you offline, is that
> > (unless anyone can prove otherwise) it becomes impossible to create a
> > Pi 5 DTS file which makes use of the RP1's resources. How do you
> > declare something as simple as a button wired to an RP1 GPIO, or fan
> > connected to a PWM output?

Where is this button or fan? On a pluggable board? Isn't that what
overlays are for, and they are stackable. So when you probe the
pluggable board via its eeprom etc, you find the overlay and load it?

Or do you mean a custom board, which has a CPU, RP1 and the button and
fan are directly on this custom board? You then want a board DTS which
includes all these pieces?

	Andrew

Re: [PATCH v6 00/10] Add support for RaspberryPi RP1 PCI device using a DT overlay

[Phil Elwell]

Hi Hervé,

On Thu, 13 Feb 2025 at 16:14, Herve Codina <herve.codina@bootlin.com> wrote:
>
> Hi Phil,
>
> On Thu, 13 Feb 2025 15:18:45 +0000
> Phil Elwell <phil@raspberrypi.com> wrote:
>
> > Hi Andrea,
> >
> > The problem with this approach (loading an overlay from the RP1 PCIe
> > driver), and it's one that I have raised with you offline, is that
> > (unless anyone can prove otherwise) it becomes impossible to create a
> > Pi 5 DTS file which makes use of the RP1's resources. How do you
> > declare something as simple as a button wired to an RP1 GPIO, or fan
> > connected to a PWM output?
>
> The driver could be improved in a second step.
> For instance, it could load the dtbo from user-space using request_firmare()
> instead of loading the embedded dtbo.
>
> >
> > If this is the preferred route to upstream adoption, I would prefer it
> > if rp1.dtso could be split in two - an rp1.dtsi similar to what we
> > have downstream, and an rp1.dtso that #includes it. In this way we can
> > keep the patching and duplication to a minimum.
>
> Indeed, having a rp1.dtsi avoid duplication but how the rp1.dtso in
> the the kernel sources could include user customization (button, fan, ...)
> without being modified ?
> At least we have to '#include <my_rp1_customizations.dtsi>'.
>
> Requesting the dtbo from user-space allows to let the user to create
> its own dtso without the need to modify the one in kernel sources.
>
> Does it make sense ?

I think I understand what you are saying, but at this point the RP1
overlay would no longer be an RP1 overlay - it would be an
RP1-and-everything-connected-to-it overlay, which is inherently
board-specific. Which user-space process do you think would be
responsible for loading this alternative overlay, choosing carefully
based on the platform it is running on? Doesn't that place quite a
burden on all the OS maintainers who up to now have just needed a
kernel and a bunch of dtb files?

If it is considered essential that the upstream Pi 5 dts file does not
include RP1 and its children, then Raspberry Pi are going to have to
walk a different path until we've seen how that can work. By splitting
rp1.dtso as I suggested, and perhaps providing an alternative helper
function that only applies the built-in overlay if the device node
doesn't already exist, we get to stay as close to upstream as
possible.

Phil

Re: [PATCH v6 00/10] Add support for RaspberryPi RP1 PCI device using a DT overlay

[Phil Elwell]

Hi Andrew,

On Thu, 13 Feb 2025 at 16:27, Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Thu, Feb 13, 2025 at 05:14:35PM +0100, Herve Codina wrote:
> > Hi Phil,
> >
> > On Thu, 13 Feb 2025 15:18:45 +0000
> > Phil Elwell <phil@raspberrypi.com> wrote:
> >
> > > Hi Andrea,
> > >
> > > The problem with this approach (loading an overlay from the RP1 PCIe
> > > driver), and it's one that I have raised with you offline, is that
> > > (unless anyone can prove otherwise) it becomes impossible to create a
> > > Pi 5 DTS file which makes use of the RP1's resources. How do you
> > > declare something as simple as a button wired to an RP1 GPIO, or fan
> > > connected to a PWM output?
>
> Where is this button or fan? On a pluggable board? Isn't that what
> overlays are for, and they are stackable. So when you probe the
> pluggable board via its eeprom etc, you find the overlay and load it?

In the Raspberry Pi ecosystem it would be the firmware that applies
the overlay, and it can't do that if the resources the overlay refers
to are not yet present in the dtb.

> Or do you mean a custom board, which has a CPU, RP1 and the button and
> fan are directly on this custom board? You then want a board DTS which
> includes all these pieces?

That depends on whether you count the Raspberry Pi 5 as a custom board.

Phil

Re: [PATCH v6 00/10] Add support for RaspberryPi RP1 PCI device using a DT overlay

[Herve Codina]

On Thu, 13 Feb 2025 16:30:44 +0000
Phil Elwell <phil@raspberrypi.com> wrote:

> Hi Andrew,
> 
> On Thu, 13 Feb 2025 at 16:27, Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Thu, Feb 13, 2025 at 05:14:35PM +0100, Herve Codina wrote:  
> > > Hi Phil,
> > >
> > > On Thu, 13 Feb 2025 15:18:45 +0000
> > > Phil Elwell <phil@raspberrypi.com> wrote:
> > >  
> > > > Hi Andrea,
> > > >
> > > > The problem with this approach (loading an overlay from the RP1 PCIe
> > > > driver), and it's one that I have raised with you offline, is that
> > > > (unless anyone can prove otherwise) it becomes impossible to create a
> > > > Pi 5 DTS file which makes use of the RP1's resources. How do you
> > > > declare something as simple as a button wired to an RP1 GPIO, or fan
> > > > connected to a PWM output?  
> >
> > Where is this button or fan? On a pluggable board? Isn't that what
> > overlays are for, and they are stackable. So when you probe the
> > pluggable board via its eeprom etc, you find the overlay and load it?  
> 
> In the Raspberry Pi ecosystem it would be the firmware that applies
> the overlay, and it can't do that if the resources the overlay refers
> to are not yet present in the dtb.

What do you mean by the 'the resources are not yet present in the dtb' ?

Also what you call the 'firmware' is the bootloader ? the kernel ?
Can you tell me who is the 'firmware' what is the mecanisme it uses to
load the overlay.

Best regards,
Hervé


> 
> > Or do you mean a custom board, which has a CPU, RP1 and the button and
> > fan are directly on this custom board? You then want a board DTS which
> > includes all these pieces?  
> 
> That depends on whether you count the Raspberry Pi 5 as a custom board.
> 

Re: [PATCH v6 00/10] Add support for RaspberryPi RP1 PCI device using a DT overlay

[Phil Elwell]

Hervé,

On Thu, 13 Feb 2025 at 17:07, Herve Codina <herve.codina@bootlin.com> wrote:
>
> On Thu, 13 Feb 2025 16:30:44 +0000
> Phil Elwell <phil@raspberrypi.com> wrote:
>
> > Hi Andrew,
> >
> > On Thu, 13 Feb 2025 at 16:27, Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > On Thu, Feb 13, 2025 at 05:14:35PM +0100, Herve Codina wrote:
> > > > Hi Phil,
> > > >
> > > > On Thu, 13 Feb 2025 15:18:45 +0000
> > > > Phil Elwell <phil@raspberrypi.com> wrote:
> > > >
> > > > > Hi Andrea,
> > > > >
> > > > > The problem with this approach (loading an overlay from the RP1 PCIe
> > > > > driver), and it's one that I have raised with you offline, is that
> > > > > (unless anyone can prove otherwise) it becomes impossible to create a
> > > > > Pi 5 DTS file which makes use of the RP1's resources. How do you
> > > > > declare something as simple as a button wired to an RP1 GPIO, or fan
> > > > > connected to a PWM output?
> > >
> > > Where is this button or fan? On a pluggable board? Isn't that what
> > > overlays are for, and they are stackable. So when you probe the
> > > pluggable board via its eeprom etc, you find the overlay and load it?
> >
> > In the Raspberry Pi ecosystem it would be the firmware that applies
> > the overlay, and it can't do that if the resources the overlay refers
> > to are not yet present in the dtb.
>
> What do you mean by the 'the resources are not yet present in the dtb' ?

Consider the fan connector on the Pi 5 PCB. It is wired to GPIO 45 on
RP1. In our current Pi 5 dtb there is an instance of pwm-fan whose
"pwms" property links to rp1_pwm node, which declares a PWM block on
RP1. Similarly, the camera and display ports make use of I2C
interfaces on RP1. The camera and display overlays, applied by the
firmware, have references to those interfaces.

If RP1 is not present in the dtb then neither of those scenarios -
board features and overlays applied the firmware - will work because
the necessary nodes and symbols are not present until the kernel has
started running, at which point dtb has been handed over.

> Also what you call the 'firmware' is the bootloader ? the kernel ?
> Can you tell me who is the 'firmware' what is the mecanisme it uses to
> load the overlay.

In the case of the Pi 5, the firmware is an EEPROM image containing
code run by the VPU embedded processor. It can load overlays using the
same mechanisms it uses to load the kernel - SD/EMMC, NVME, USB, TFTP,
etc. The same problem would exist for U-boot. Even though RP1 has a
standard XHCI controller, U-boot wouldn't be able to detect it and
make use of it, say to load a kernel from a USB stick, because it
isn't declared in the DTB.

Phil

Re: [PATCH v6 00/10] Add support for RaspberryPi RP1 PCI device using a DT overlay

[Andrew Lunn]

> > Or do you mean a custom board, which has a CPU, RP1 and the button and
> > fan are directly on this custom board? You then want a board DTS which
> > includes all these pieces?
> 
> That depends on whether you count the Raspberry Pi 5 as a custom board.

So you mean the Pi 5 board would itself make use of the resources the
RP1 device has? They are not simply connected to headers for plugin
boards, but used by the main board? Hence you want to describe them in
the board .DTS file.

	Andrew

Re: [PATCH v6 00/10] Add support for RaspberryPi RP1 PCI device using a DT overlay

[Andrew Lunn]

> etc. The same problem would exist for U-boot. Even though RP1 has a
> standard XHCI controller

Although it is a standard XHCI controller, i assume it is not a PCIe
XHCI controller which can just be enumerated as a PCIe device?

	Andrew

Re: [PATCH v6 00/10] Add support for RaspberryPi RP1 PCI device using a DT overlay

[Phil Elwell]

On Thu, 13 Feb 2025 at 17:45, Andrew Lunn <andrew@lunn.ch> wrote:
>
> > > Or do you mean a custom board, which has a CPU, RP1 and the button and
> > > fan are directly on this custom board? You then want a board DTS which
> > > includes all these pieces?
> >
> > That depends on whether you count the Raspberry Pi 5 as a custom board.
>
> So you mean the Pi 5 board would itself make use of the resources the
> RP1 device has? They are not simply connected to headers for plugin
> boards, but used by the main board? Hence you want to describe them in
> the board .DTS file.

That's correct. But even for plug-in devices, those which are on
non-discoverable buses need overlays to declare them, which causes a
problem when the overlay application happens before the kernel is
started.

Phil

Re: [PATCH v6 00/10] Add support for RaspberryPi RP1 PCI device using a DT overlay

[Phil Elwell]

On Thu, 13 Feb 2025 at 17:51, Andrew Lunn <andrew@lunn.ch> wrote:
>
> > etc. The same problem would exist for U-boot. Even though RP1 has a
> > standard XHCI controller
>
> Although it is a standard XHCI controller, i assume it is not a PCIe
> XHCI controller which can just be enumerated as a PCIe device?

There are too many sub-devices in RP1 for them each to get their own
function. Instead, they all share a single function, with DT to
declare them.

Phil

Re: [PATCH v6 00/10] Add support for RaspberryPi RP1 PCI device using a DT overlay

[Herve Codina]

Hi Phil,

On Thu, 13 Feb 2025 17:57:37 +0000
Phil Elwell <phil@raspberrypi.com> wrote:

> On Thu, 13 Feb 2025 at 17:45, Andrew Lunn <andrew@lunn.ch> wrote:
> >  
> > > > Or do you mean a custom board, which has a CPU, RP1 and the button and
> > > > fan are directly on this custom board? You then want a board DTS which
> > > > includes all these pieces?  
> > >
> > > That depends on whether you count the Raspberry Pi 5 as a custom board.  
> >
> > So you mean the Pi 5 board would itself make use of the resources the
> > RP1 device has? They are not simply connected to headers for plugin
> > boards, but used by the main board? Hence you want to describe them in
> > the board .DTS file.  
> 
> That's correct. But even for plug-in devices, those which are on
> non-discoverable buses need overlays to declare them, which causes a
> problem when the overlay application happens before the kernel is
> started.
> 

Hum, I see.

We worked on overlay usage on non-discoverable buses wired to a connector
and we did a talk about issues we are facing on at Plumber [0].

You can also find our big picture in [1] and a last contribution introducing
export-symbols feature in [2]. export-symbols is also under discussion on
some other threads.

Also, we proposed the i2c bus extensions feature [3] whose goal is to allow
an addon board to add devices on an i2c bus provided by a base board and
wired to an connector the addon board is connected to.

Maybe in your case, you can decouple resources (gpio, pwm) provided by the
addon board and used by the base board using also nexus node.

We use a nexus node [4] (not presented at the Plumbers talk because the idea
came during 'out of talk' discussions in Plumbers) in order to allow our
addon board to use resources provided by the base board.

In your case, if I understood, you are in the other direction but why not
using also a nexus node to decouple and translate resources in this other
direction ?

Don't know if this idea can help but feel free to ask for some more
information if needed.

[0] https://lpc.events/event/18/contributions/1696/
[1] https://lore.kernel.org/lkml/20240917-hotplug-drm-bridge-v4-0-bc4dfee61be6@bootlin.com/
[2] https://lore.kernel.org/all/20241209151830.95723-1-herve.codina@bootlin.com/
[3] https://lore.kernel.org/all/20250205173918.600037-1-herve.codina@bootlin.com/
[4] https://github.com/devicetree-org/devicetree-specification/blob/v0.4/source/chapter2-devicetree-basics.rst#nexus-nodes-and-specifier-mapping

Best regards,
Hervé

Re: [PATCH v6 00/10] Add support for RaspberryPi RP1 PCI device using a DT overlay

[Phil Elwell]

Once more, with plain text, which I'd hoped the Android GMail client
would work out for itself.

On Thu, 13 Feb 2025, 18:53 Herve Codina, <herve.codina@bootlin.com> wrote:
>
> Hi Phil,
>
> On Thu, 13 Feb 2025 17:57:37 +0000
> Phil Elwell <phil@raspberrypi.com> wrote:
>
> > On Thu, 13 Feb 2025 at 17:45, Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > > > Or do you mean a custom board, which has a CPU, RP1 and the button and
> > > > > fan are directly on this custom board? You then want a board DTS which
> > > > > includes all these pieces?
> > > >
> > > > That depends on whether you count the Raspberry Pi 5 as a custom board.
> > >
> > > So you mean the Pi 5 board would itself make use of the resources the
> > > RP1 device has? They are not simply connected to headers for plugin
> > > boards, but used by the main board? Hence you want to describe them in
> > > the board .DTS file.
> >
> > That's correct. But even for plug-in devices, those which are on
> > non-discoverable buses need overlays to declare them, which causes a
> > problem when the overlay application happens before the kernel is
> > started.
> >
>
> Hum, I see.
>
> We worked on overlay usage on non-discoverable buses wired to a connector
> and we did a talk about issues we are facing on at Plumber [0].
>
> You can also find our big picture in [1] and a last contribution introducing
> export-symbols feature in [2]. export-symbols is also under discussion on
> some other threads.
>
> Also, we proposed the i2c bus extensions feature [3] whose goal is to allow
> an addon board to add devices on an i2c bus provided by a base board and
> wired to an connector the addon board is connected to.
>
> Maybe in your case, you can decouple resources (gpio, pwm) provided by the
> addon board and used by the base board using also nexus node.
>
> We use a nexus node [4] (not presented at the Plumbers talk because the idea
> came during 'out of talk' discussions in Plumbers) in order to allow our
> addon board to use resources provided by the base board.
>
> In your case, if I understood, you are in the other direction but why not
> using also a nexus node to decouple and translate resources in this other
> direction ?
>
> Don't know if this idea can help but feel free to ask for some more
> information if needed.

Nexus nodes look interesting - I see them as adding a layer of
abstraction such that, for example, boards can declare which of their
specific resources performs a common function so that clients can
treat them all the same. We do the same thing in a limited way by
using common labels on nodes, but this goes much further.

In the case of Pi 5 and RP1, I imagine you are proposing that the Pi 5
dtb declares the connector node and the overlay fills in the content
with references to its GPIO controller, PWM controller etc. However, I
think the overlay would also have to be board specific because it's
not possible to patch part of a property from an overlay, so you'd end
up overwriting the GPIO number as well as the controller reference.

What is needed to make this work is the ability to cope with
unresolved references in the base dtb, to be resolved as each overlay
is applied, with runtime checking that each reference is resolved
before it is used, all of which sounds like a nightmare. Plus, we
really don't want to have to change the way all our camera and display
overlays work on all Raspberry Pis just to accommodate somebody's idea
of how RP1 should be handled.

Besides, Occam's razor surely applies.

Phil

Re: [PATCH v6 00/10] Add support for RaspberryPi RP1 PCI device using a DT overlay

[Herve Codina]

Hi Phil,

On Thu, 13 Feb 2025 20:15:06 +0000
Phil Elwell <phil@raspberrypi.com> wrote:

> Once more, with plain text, which I'd hoped the Android GMail client
> would work out for itself.
> 
> On Thu, 13 Feb 2025, 18:53 Herve Codina, <herve.codina@bootlin.com> wrote:
> >
> > Hi Phil,
> >
> > On Thu, 13 Feb 2025 17:57:37 +0000
> > Phil Elwell <phil@raspberrypi.com> wrote:
> >  
> > > On Thu, 13 Feb 2025 at 17:45, Andrew Lunn <andrew@lunn.ch> wrote:  
> > > >  
> > > > > > Or do you mean a custom board, which has a CPU, RP1 and the button and
> > > > > > fan are directly on this custom board? You then want a board DTS which
> > > > > > includes all these pieces?  
> > > > >
> > > > > That depends on whether you count the Raspberry Pi 5 as a custom board.  
> > > >
> > > > So you mean the Pi 5 board would itself make use of the resources the
> > > > RP1 device has? They are not simply connected to headers for plugin
> > > > boards, but used by the main board? Hence you want to describe them in
> > > > the board .DTS file.  
> > >
> > > That's correct. But even for plug-in devices, those which are on
> > > non-discoverable buses need overlays to declare them, which causes a
> > > problem when the overlay application happens before the kernel is
> > > started.
> > >  
> >
> > Hum, I see.
> >
> > We worked on overlay usage on non-discoverable buses wired to a connector
> > and we did a talk about issues we are facing on at Plumber [0].
> >
> > You can also find our big picture in [1] and a last contribution introducing
> > export-symbols feature in [2]. export-symbols is also under discussion on
> > some other threads.
> >
> > Also, we proposed the i2c bus extensions feature [3] whose goal is to allow
> > an addon board to add devices on an i2c bus provided by a base board and
> > wired to an connector the addon board is connected to.
> >
> > Maybe in your case, you can decouple resources (gpio, pwm) provided by the
> > addon board and used by the base board using also nexus node.
> >
> > We use a nexus node [4] (not presented at the Plumbers talk because the idea
> > came during 'out of talk' discussions in Plumbers) in order to allow our
> > addon board to use resources provided by the base board.
> >
> > In your case, if I understood, you are in the other direction but why not
> > using also a nexus node to decouple and translate resources in this other
> > direction ?
> >
> > Don't know if this idea can help but feel free to ask for some more
> > information if needed.  
> 
> Nexus nodes look interesting - I see them as adding a layer of
> abstraction such that, for example, boards can declare which of their
> specific resources performs a common function so that clients can
> treat them all the same. We do the same thing in a limited way by
> using common labels on nodes, but this goes much further.
> 
> In the case of Pi 5 and RP1, I imagine you are proposing that the Pi 5
> dtb declares the connector node and the overlay fills in the content
> with references to its GPIO controller, PWM controller etc. However, I
> think the overlay would also have to be board specific because it's
> not possible to patch part of a property from an overlay, so you'd end
> up overwriting the GPIO number as well as the controller reference.
> 
> What is needed to make this work is the ability to cope with
> unresolved references in the base dtb, to be resolved as each overlay
> is applied, with runtime checking that each reference is resolved
> before it is used, all of which sounds like a nightmare. Plus, we
> really don't want to have to change the way all our camera and display
> overlays work on all Raspberry Pis just to accommodate somebody's idea
> of how RP1 should be handled.

Just to be clear, my comments were not there to tell you how RP1 should
work. I just proposed ideas without trying to force anything and I can
fully understand that ideas proposed don't feed your needs.

Sorry if my approach was misunderstood.

Best regards,
Hervé

Re: [PATCH v6 00/10] Add support for RaspberryPi RP1 PCI device using a DT overlay

[Phil Elwell]

On Thu, 13 Feb 2025, 21:06 Herve Codina, <herve.codina@bootlin.com> wrote:
>
> Hi Phil,
>
> On Thu, 13 Feb 2025 20:15:06 +0000
> Phil Elwell <phil@raspberrypi.com> wrote:
>
> > Once more, with plain text, which I'd hoped the Android GMail client
> > would work out for itself.
> >
> > On Thu, 13 Feb 2025, 18:53 Herve Codina, <herve.codina@bootlin.com> wrote:
> > >
> > > Hi Phil,
> > >
> > > On Thu, 13 Feb 2025 17:57:37 +0000
> > > Phil Elwell <phil@raspberrypi.com> wrote:
> > >
> > > > On Thu, 13 Feb 2025 at 17:45, Andrew Lunn <andrew@lunn.ch> wrote:
> > > > >
> > > > > > > Or do you mean a custom board, which has a CPU, RP1 and the button and
> > > > > > > fan are directly on this custom board? You then want a board DTS which
> > > > > > > includes all these pieces?
> > > > > >
> > > > > > That depends on whether you count the Raspberry Pi 5 as a custom board.
> > > > >
> > > > > So you mean the Pi 5 board would itself make use of the resources the
> > > > > RP1 device has? They are not simply connected to headers for plugin
> > > > > boards, but used by the main board? Hence you want to describe them in
> > > > > the board .DTS file.
> > > >
> > > > That's correct. But even for plug-in devices, those which are on
> > > > non-discoverable buses need overlays to declare them, which causes a
> > > > problem when the overlay application happens before the kernel is
> > > > started.
> > > >
> > >
> > > Hum, I see.
> > >
> > > We worked on overlay usage on non-discoverable buses wired to a connector
> > > and we did a talk about issues we are facing on at Plumber [0].
> > >
> > > You can also find our big picture in [1] and a last contribution introducing
> > > export-symbols feature in [2]. export-symbols is also under discussion on
> > > some other threads.
> > >
> > > Also, we proposed the i2c bus extensions feature [3] whose goal is to allow
> > > an addon board to add devices on an i2c bus provided by a base board and
> > > wired to an connector the addon board is connected to.
> > >
> > > Maybe in your case, you can decouple resources (gpio, pwm) provided by the
> > > addon board and used by the base board using also nexus node.
> > >
> > > We use a nexus node [4] (not presented at the Plumbers talk because the idea
> > > came during 'out of talk' discussions in Plumbers) in order to allow our
> > > addon board to use resources provided by the base board.
> > >
> > > In your case, if I understood, you are in the other direction but why not
> > > using also a nexus node to decouple and translate resources in this other
> > > direction ?
> > >
> > > Don't know if this idea can help but feel free to ask for some more
> > > information if needed.
> >
> > Nexus nodes look interesting - I see them as adding a layer of
> > abstraction such that, for example, boards can declare which of their
> > specific resources performs a common function so that clients can
> > treat them all the same. We do the same thing in a limited way by
> > using common labels on nodes, but this goes much further.
> >
> > In the case of Pi 5 and RP1, I imagine you are proposing that the Pi 5
> > dtb declares the connector node and the overlay fills in the content
> > with references to its GPIO controller, PWM controller etc. However, I
> > think the overlay would also have to be board specific because it's
> > not possible to patch part of a property from an overlay, so you'd end
> > up overwriting the GPIO number as well as the controller reference.
> >
> > What is needed to make this work is the ability to cope with
> > unresolved references in the base dtb, to be resolved as each overlay
> > is applied, with runtime checking that each reference is resolved
> > before it is used, all of which sounds like a nightmare. Plus, we
> > really don't want to have to change the way all our camera and display
> > overlays work on all Raspberry Pis just to accommodate somebody's idea
> > of how RP1 should be handled.
>
> Just to be clear, my comments were not there to tell you how RP1 should
> work. I just proposed ideas without trying to force anything and I can
> fully understand that ideas proposed don't feed your needs.
>
> Sorry if my approach was misunderstood.

I feel I've been misunderstood - I appreciate your ideas.

Perhaps it would help if you could outline how you think we could
apply your suggestions?

Thanks,

Phil

Re: [PATCH v6 00/10] Add support for RaspberryPi RP1 PCI device using a DT overlay

[Herve Codina]

Hi Phil,

On Thu, 13 Feb 2025 21:12:43 +0000
Phil Elwell <phil@raspberrypi.com> wrote:

> On Thu, 13 Feb 2025, 21:06 Herve Codina, <herve.codina@bootlin.com> wrote:
> >
> > Hi Phil,
> >
> > On Thu, 13 Feb 2025 20:15:06 +0000
> > Phil Elwell <phil@raspberrypi.com> wrote:
> >  
> > > Once more, with plain text, which I'd hoped the Android GMail client
> > > would work out for itself.
> > >
> > > On Thu, 13 Feb 2025, 18:53 Herve Codina, <herve.codina@bootlin.com> wrote:  
> > > >
> > > > Hi Phil,
> > > >
> > > > On Thu, 13 Feb 2025 17:57:37 +0000
> > > > Phil Elwell <phil@raspberrypi.com> wrote:
> > > >  
> > > > > On Thu, 13 Feb 2025 at 17:45, Andrew Lunn <andrew@lunn.ch> wrote:  
> > > > > >  
> > > > > > > > Or do you mean a custom board, which has a CPU, RP1 and the button and
> > > > > > > > fan are directly on this custom board? You then want a board DTS which
> > > > > > > > includes all these pieces?  
> > > > > > >
> > > > > > > That depends on whether you count the Raspberry Pi 5 as a custom board.  
> > > > > >
> > > > > > So you mean the Pi 5 board would itself make use of the resources the
> > > > > > RP1 device has? They are not simply connected to headers for plugin
> > > > > > boards, but used by the main board? Hence you want to describe them in
> > > > > > the board .DTS file.  
> > > > >
> > > > > That's correct. But even for plug-in devices, those which are on
> > > > > non-discoverable buses need overlays to declare them, which causes a
> > > > > problem when the overlay application happens before the kernel is
> > > > > started.
> > > > >  
> > > >
> > > > Hum, I see.
> > > >
> > > > We worked on overlay usage on non-discoverable buses wired to a connector
> > > > and we did a talk about issues we are facing on at Plumber [0].
> > > >
> > > > You can also find our big picture in [1] and a last contribution introducing
> > > > export-symbols feature in [2]. export-symbols is also under discussion on
> > > > some other threads.
> > > >
> > > > Also, we proposed the i2c bus extensions feature [3] whose goal is to allow
> > > > an addon board to add devices on an i2c bus provided by a base board and
> > > > wired to an connector the addon board is connected to.
> > > >
> > > > Maybe in your case, you can decouple resources (gpio, pwm) provided by the
> > > > addon board and used by the base board using also nexus node.
> > > >
> > > > We use a nexus node [4] (not presented at the Plumbers talk because the idea
> > > > came during 'out of talk' discussions in Plumbers) in order to allow our
> > > > addon board to use resources provided by the base board.
> > > >
> > > > In your case, if I understood, you are in the other direction but why not
> > > > using also a nexus node to decouple and translate resources in this other
> > > > direction ?
> > > >
> > > > Don't know if this idea can help but feel free to ask for some more
> > > > information if needed.  
> > >
> > > Nexus nodes look interesting - I see them as adding a layer of
> > > abstraction such that, for example, boards can declare which of their
> > > specific resources performs a common function so that clients can
> > > treat them all the same. We do the same thing in a limited way by
> > > using common labels on nodes, but this goes much further.
> > >
> > > In the case of Pi 5 and RP1, I imagine you are proposing that the Pi 5
> > > dtb declares the connector node and the overlay fills in the content
> > > with references to its GPIO controller, PWM controller etc. However, I
> > > think the overlay would also have to be board specific because it's
> > > not possible to patch part of a property from an overlay, so you'd end
> > > up overwriting the GPIO number as well as the controller reference.
> > >
> > > What is needed to make this work is the ability to cope with
> > > unresolved references in the base dtb, to be resolved as each overlay
> > > is applied, with runtime checking that each reference is resolved
> > > before it is used, all of which sounds like a nightmare. Plus, we
> > > really don't want to have to change the way all our camera and display
> > > overlays work on all Raspberry Pis just to accommodate somebody's idea
> > > of how RP1 should be handled.  
> >
> > Just to be clear, my comments were not there to tell you how RP1 should
> > work. I just proposed ideas without trying to force anything and I can
> > fully understand that ideas proposed don't feed your needs.
> >
> > Sorry if my approach was misunderstood.  
> 
> I feel I've been misunderstood - I appreciate your ideas.
> 
> Perhaps it would help if you could outline how you think we could
> apply your suggestions?
> 

I was thinking about what your mentioned, i.e. the overlay fill the nexus node.
No sure to understand why the overlay should patch some properties.
Also where are the unresolved references in that case. The base DT refers to
the Nexus node.
The issue will probably be that the translation performed by the nexus node is
not available until the overlay is applied. The consumer will see errors other
than PROBE_DEFER when if probes while the overlay is not applied.

Also, the solution will lead to memory leak at runtime. Indeed, the overlay
add properties in an already existing node.
If the overlay is applied by the Kernel itself, this lead to memory leak when
the overlay is removed.
Indeed, an overlay can add/remove node without any issue but it cannot
add/remove properties to/from existing nodes.

In the case described here, the nexus node is already present in the DT and the
overlay add/remove properties to/from this existing node.

I haven't got any better idea for the moment in order to have this kind of
'reverse' Nexus node.

Best regards,
Hervé

Re: [PATCH v6 00/10] Add support for RaspberryPi RP1 PCI device using a DT overlay

[Phil Elwell]

Hi Hervé,

On Mon, 17 Feb 2025 at 15:53, Herve Codina <herve.codina@bootlin.com> wrote:
>
> Hi Phil,
>
> On Thu, 13 Feb 2025 21:12:43 +0000
> Phil Elwell <phil@raspberrypi.com> wrote:
>
> > On Thu, 13 Feb 2025, 21:06 Herve Codina, <herve.codina@bootlin.com> wrote:
> > >
> > > Hi Phil,
> > >
> > > On Thu, 13 Feb 2025 20:15:06 +0000
> > > Phil Elwell <phil@raspberrypi.com> wrote:
> > >
> > > > Once more, with plain text, which I'd hoped the Android GMail client
> > > > would work out for itself.
> > > >
> > > > On Thu, 13 Feb 2025, 18:53 Herve Codina, <herve.codina@bootlin.com> wrote:
> > > > >
> > > > > Hi Phil,
> > > > >
> > > > > On Thu, 13 Feb 2025 17:57:37 +0000
> > > > > Phil Elwell <phil@raspberrypi.com> wrote:
> > > > >
> > > > > > On Thu, 13 Feb 2025 at 17:45, Andrew Lunn <andrew@lunn.ch> wrote:
> > > > > > >
> > > > > > > > > Or do you mean a custom board, which has a CPU, RP1 and the button and
> > > > > > > > > fan are directly on this custom board? You then want a board DTS which
> > > > > > > > > includes all these pieces?
> > > > > > > >
> > > > > > > > That depends on whether you count the Raspberry Pi 5 as a custom board.
> > > > > > >
> > > > > > > So you mean the Pi 5 board would itself make use of the resources the
> > > > > > > RP1 device has? They are not simply connected to headers for plugin
> > > > > > > boards, but used by the main board? Hence you want to describe them in
> > > > > > > the board .DTS file.
> > > > > >
> > > > > > That's correct. But even for plug-in devices, those which are on
> > > > > > non-discoverable buses need overlays to declare them, which causes a
> > > > > > problem when the overlay application happens before the kernel is
> > > > > > started.
> > > > > >
> > > > >
> > > > > Hum, I see.
> > > > >
> > > > > We worked on overlay usage on non-discoverable buses wired to a connector
> > > > > and we did a talk about issues we are facing on at Plumber [0].
> > > > >
> > > > > You can also find our big picture in [1] and a last contribution introducing
> > > > > export-symbols feature in [2]. export-symbols is also under discussion on
> > > > > some other threads.
> > > > >
> > > > > Also, we proposed the i2c bus extensions feature [3] whose goal is to allow
> > > > > an addon board to add devices on an i2c bus provided by a base board and
> > > > > wired to an connector the addon board is connected to.
> > > > >
> > > > > Maybe in your case, you can decouple resources (gpio, pwm) provided by the
> > > > > addon board and used by the base board using also nexus node.
> > > > >
> > > > > We use a nexus node [4] (not presented at the Plumbers talk because the idea
> > > > > came during 'out of talk' discussions in Plumbers) in order to allow our
> > > > > addon board to use resources provided by the base board.
> > > > >
> > > > > In your case, if I understood, you are in the other direction but why not
> > > > > using also a nexus node to decouple and translate resources in this other
> > > > > direction ?
> > > > >
> > > > > Don't know if this idea can help but feel free to ask for some more
> > > > > information if needed.
> > > >
> > > > Nexus nodes look interesting - I see them as adding a layer of
> > > > abstraction such that, for example, boards can declare which of their
> > > > specific resources performs a common function so that clients can
> > > > treat them all the same. We do the same thing in a limited way by
> > > > using common labels on nodes, but this goes much further.
> > > >
> > > > In the case of Pi 5 and RP1, I imagine you are proposing that the Pi 5
> > > > dtb declares the connector node and the overlay fills in the content
> > > > with references to its GPIO controller, PWM controller etc. However, I
> > > > think the overlay would also have to be board specific because it's
> > > > not possible to patch part of a property from an overlay, so you'd end
> > > > up overwriting the GPIO number as well as the controller reference.
> > > >
> > > > What is needed to make this work is the ability to cope with
> > > > unresolved references in the base dtb, to be resolved as each overlay
> > > > is applied, with runtime checking that each reference is resolved
> > > > before it is used, all of which sounds like a nightmare. Plus, we
> > > > really don't want to have to change the way all our camera and display
> > > > overlays work on all Raspberry Pis just to accommodate somebody's idea
> > > > of how RP1 should be handled.
> > >
> > > Just to be clear, my comments were not there to tell you how RP1 should
> > > work. I just proposed ideas without trying to force anything and I can
> > > fully understand that ideas proposed don't feed your needs.
> > >
> > > Sorry if my approach was misunderstood.
> >
> > I feel I've been misunderstood - I appreciate your ideas.
> >
> > Perhaps it would help if you could outline how you think we could
> > apply your suggestions?
> >
>
> I was thinking about what your mentioned, i.e. the overlay fill the nexus node.
> No sure to understand why the overlay should patch some properties.
> Also where are the unresolved references in that case. The base DT refers to
> the Nexus node.
> The issue will probably be that the translation performed by the nexus node is
> not available until the overlay is applied. The consumer will see errors other
> than PROBE_DEFER when if probes while the overlay is not applied.

The job of the nexus node would be to translate a generic request for
a numbered resource to a specific request for an RP1 resource with
arbitrary properties. The arbitrary properties could be GPIO offsets,
which are board specific, while the node supplying the resource is
provided by the overlay. This means that an entry in the table,
described by a single property, could have contributions from the base
DT and the overlay, which is not possible since overlays overwrite
whole properties.

Perhaps that particular problem could be overcome by creating a
single-entry map, using the map-mask feature to pass through all of
the GPIO offset and flags to the parent, so that the whole table
becomes a proxy for RP1's GPIO controller. Is that what you had in
mind?

> Also, the solution will lead to memory leak at runtime. Indeed, the overlay
> add properties in an already existing node.
> If the overlay is applied by the Kernel itself, this lead to memory leak when
> the overlay is removed.
> Indeed, an overlay can add/remove node without any issue but it cannot
> add/remove properties to/from existing nodes.

Fortunately for me I'm not arguing _for_ the use of an overlay.

> In the case described here, the nexus node is already present in the DT and the
> overlay add/remove properties to/from this existing node.

I think I can see how that could be made to work for GPIOs. It looks
as though the GPIO subsystem is the only one making use of
of_parse_phandle_with_args_map. Interrupts seem to have an open-coded
equivalent, and iommus. What about I2C and PWM?

Phil

Re: [PATCH v6 00/10] Add support for RaspberryPi RP1 PCI device using a DT overlay

[Herve Codina]

On Mon, 17 Feb 2025 17:03:34 +0000
Phil Elwell <phil@raspberrypi.com> wrote:

> Hi Hervé,
> 
> On Mon, 17 Feb 2025 at 15:53, Herve Codina <herve.codina@bootlin.com> wrote:
> >
> > Hi Phil,
> >
> > On Thu, 13 Feb 2025 21:12:43 +0000
> > Phil Elwell <phil@raspberrypi.com> wrote:
> >  
> > > On Thu, 13 Feb 2025, 21:06 Herve Codina, <herve.codina@bootlin.com> wrote:  
> > > >
> > > > Hi Phil,
> > > >
> > > > On Thu, 13 Feb 2025 20:15:06 +0000
> > > > Phil Elwell <phil@raspberrypi.com> wrote:
> > > >  
> > > > > Once more, with plain text, which I'd hoped the Android GMail client
> > > > > would work out for itself.
> > > > >
> > > > > On Thu, 13 Feb 2025, 18:53 Herve Codina, <herve.codina@bootlin.com> wrote:  
> > > > > >
> > > > > > Hi Phil,
> > > > > >
> > > > > > On Thu, 13 Feb 2025 17:57:37 +0000
> > > > > > Phil Elwell <phil@raspberrypi.com> wrote:
> > > > > >  
> > > > > > > On Thu, 13 Feb 2025 at 17:45, Andrew Lunn <andrew@lunn.ch> wrote:  
> > > > > > > >  
> > > > > > > > > > Or do you mean a custom board, which has a CPU, RP1 and the button and
> > > > > > > > > > fan are directly on this custom board? You then want a board DTS which
> > > > > > > > > > includes all these pieces?  
> > > > > > > > >
> > > > > > > > > That depends on whether you count the Raspberry Pi 5 as a custom board.  
> > > > > > > >
> > > > > > > > So you mean the Pi 5 board would itself make use of the resources the
> > > > > > > > RP1 device has? They are not simply connected to headers for plugin
> > > > > > > > boards, but used by the main board? Hence you want to describe them in
> > > > > > > > the board .DTS file.  
> > > > > > >
> > > > > > > That's correct. But even for plug-in devices, those which are on
> > > > > > > non-discoverable buses need overlays to declare them, which causes a
> > > > > > > problem when the overlay application happens before the kernel is
> > > > > > > started.
> > > > > > >  
> > > > > >
> > > > > > Hum, I see.
> > > > > >
> > > > > > We worked on overlay usage on non-discoverable buses wired to a connector
> > > > > > and we did a talk about issues we are facing on at Plumber [0].
> > > > > >
> > > > > > You can also find our big picture in [1] and a last contribution introducing
> > > > > > export-symbols feature in [2]. export-symbols is also under discussion on
> > > > > > some other threads.
> > > > > >
> > > > > > Also, we proposed the i2c bus extensions feature [3] whose goal is to allow
> > > > > > an addon board to add devices on an i2c bus provided by a base board and
> > > > > > wired to an connector the addon board is connected to.
> > > > > >
> > > > > > Maybe in your case, you can decouple resources (gpio, pwm) provided by the
> > > > > > addon board and used by the base board using also nexus node.
> > > > > >
> > > > > > We use a nexus node [4] (not presented at the Plumbers talk because the idea
> > > > > > came during 'out of talk' discussions in Plumbers) in order to allow our
> > > > > > addon board to use resources provided by the base board.
> > > > > >
> > > > > > In your case, if I understood, you are in the other direction but why not
> > > > > > using also a nexus node to decouple and translate resources in this other
> > > > > > direction ?
> > > > > >
> > > > > > Don't know if this idea can help but feel free to ask for some more
> > > > > > information if needed.  
> > > > >
> > > > > Nexus nodes look interesting - I see them as adding a layer of
> > > > > abstraction such that, for example, boards can declare which of their
> > > > > specific resources performs a common function so that clients can
> > > > > treat them all the same. We do the same thing in a limited way by
> > > > > using common labels on nodes, but this goes much further.
> > > > >
> > > > > In the case of Pi 5 and RP1, I imagine you are proposing that the Pi 5
> > > > > dtb declares the connector node and the overlay fills in the content
> > > > > with references to its GPIO controller, PWM controller etc. However, I
> > > > > think the overlay would also have to be board specific because it's
> > > > > not possible to patch part of a property from an overlay, so you'd end
> > > > > up overwriting the GPIO number as well as the controller reference.
> > > > >
> > > > > What is needed to make this work is the ability to cope with
> > > > > unresolved references in the base dtb, to be resolved as each overlay
> > > > > is applied, with runtime checking that each reference is resolved
> > > > > before it is used, all of which sounds like a nightmare. Plus, we
> > > > > really don't want to have to change the way all our camera and display
> > > > > overlays work on all Raspberry Pis just to accommodate somebody's idea
> > > > > of how RP1 should be handled.  
> > > >
> > > > Just to be clear, my comments were not there to tell you how RP1 should
> > > > work. I just proposed ideas without trying to force anything and I can
> > > > fully understand that ideas proposed don't feed your needs.
> > > >
> > > > Sorry if my approach was misunderstood.  
> > >
> > > I feel I've been misunderstood - I appreciate your ideas.
> > >
> > > Perhaps it would help if you could outline how you think we could
> > > apply your suggestions?
> > >  
> >
> > I was thinking about what your mentioned, i.e. the overlay fill the nexus node.
> > No sure to understand why the overlay should patch some properties.
> > Also where are the unresolved references in that case. The base DT refers to
> > the Nexus node.
> > The issue will probably be that the translation performed by the nexus node is
> > not available until the overlay is applied. The consumer will see errors other
> > than PROBE_DEFER when if probes while the overlay is not applied.  
> 
> The job of the nexus node would be to translate a generic request for
> a numbered resource to a specific request for an RP1 resource with
> arbitrary properties. The arbitrary properties could be GPIO offsets,
> which are board specific, while the node supplying the resource is
> provided by the overlay. This means that an entry in the table,
> described by a single property, could have contributions from the base
> DT and the overlay, which is not possible since overlays overwrite
> whole properties.

Hum, I am a bit lost.
Some DT example (base and overlay) could help me to understand.

> 
> Perhaps that particular problem could be overcome by creating a
> single-entry map, using the map-mask feature to pass through all of
> the GPIO offset and flags to the parent, so that the whole table
> becomes a proxy for RP1's GPIO controller. Is that what you had in
> mind?
> 
> > Also, the solution will lead to memory leak at runtime. Indeed, the overlay
> > add properties in an already existing node.
> > If the overlay is applied by the Kernel itself, this lead to memory leak when
> > the overlay is removed.
> > Indeed, an overlay can add/remove node without any issue but it cannot
> > add/remove properties to/from existing nodes.  
> 
> Fortunately for me I'm not arguing _for_ the use of an overlay.
> 
> > In the case described here, the nexus node is already present in the DT and the
> > overlay add/remove properties to/from this existing node.  
> 
> I think I can see how that could be made to work for GPIOs. It looks
> as though the GPIO subsystem is the only one making use of
> of_parse_phandle_with_args_map. Interrupts seem to have an open-coded
> equivalent, and iommus. What about I2C and PWM?

Support for PWM has been recently accepted.
  https://lore.kernel.org/all/ufl4kwrjyp4zid4muvghefevqc6hk3zyvxnsu72fxd4f46fzg6@hufkci2dzjid/

For i2c, nexus node is not suitable.

Nexus node works well when resources are indexed (gpio line in a gpio chip
for instance). For bus controller there is no index.
I mean we never refer a i2c bus controller using <&i2c-ctrl 12>.

For i2c, I proposed i2c bus extension:
  https://lore.kernel.org/all/20250205173918.600037-1-herve.codina@bootlin.com/

Best regards,
Hervé

Re: [PATCH v6 00/10] Add support for RaspberryPi RP1 PCI device using a DT overlay

[Phil Elwell]

On Mon, 17 Feb 2025 at 17:56, Herve Codina <herve.codina@bootlin.com> wrote:
>
> On Mon, 17 Feb 2025 17:03:34 +0000
> Phil Elwell <phil@raspberrypi.com> wrote:
>

<snip>

> > The job of the nexus node would be to translate a generic request for
> > a numbered resource to a specific request for an RP1 resource with
> > arbitrary properties. The arbitrary properties could be GPIO offsets,
> > which are board specific, while the node supplying the resource is
> > provided by the overlay. This means that an entry in the table,
> > described by a single property, could have contributions from the base
> > DT and the overlay, which is not possible since overlays overwrite
> > whole properties.
>
> Hum, I am a bit lost.
> Some DT example (base and overlay) could help me to understand.

I could, but I think it is becoming a distraction.

<snip>

> > I think I can see how that could be made to work for GPIOs. It looks
> > as though the GPIO subsystem is the only one making use of
> > of_parse_phandle_with_args_map. Interrupts seem to have an open-coded
> > equivalent, and iommus. What about I2C and PWM?
>
> Support for PWM has been recently accepted.
>   https://lore.kernel.org/all/ufl4kwrjyp4zid4muvghefevqc6hk3zyvxnsu72fxd4f46fzg6@hufkci2dzjid/
>
> For i2c, nexus node is not suitable.
>
> Nexus node works well when resources are indexed (gpio line in a gpio chip
> for instance). For bus controller there is no index.
> I mean we never refer a i2c bus controller using <&i2c-ctrl 12>.
>
> For i2c, I proposed i2c bus extension:
>   https://lore.kernel.org/all/20250205173918.600037-1-herve.codina@bootlin.com/

I don't see in principle why an address-cells of 0 should cause a
problem - it's a degenerate case, but it's still conceptually valid.

However, we seem to be having to invent a lot of new infrastructure -
some of it already supported by the kernel, some of it not - because
we have made the mistake of using a discoverable bus for a
point-to-point link on a PCB. I don't see how this is fundamentally
different to the USB-attached Ethernet controller on e.g. the Pi 3B.
Despite the fact that the Ethernet interface is discoverable, it has a
Device Tree declaration (see
arch/arm/boot/dts/broadcom/bcm283x-rpi-smsc9514.dtsi) in order that
the firmware can supply a MAC address. Yes, the RP1 DT declaration is
significantly larger, but size shouldn't matter for what seems to be
objections based on tenets.

Phil

Re: [PATCH v6 00/10] Add support for RaspberryPi RP1 PCI device using a DT overlay

[Andrea della Porta]

Hi,

On 16:27 Thu 13 Feb     , Phil Elwell wrote:
> Hi Hervé,
> 
> On Thu, 13 Feb 2025 at 16:14, Herve Codina <herve.codina@bootlin.com> wrote:
> >
> > Hi Phil,
> >
> > On Thu, 13 Feb 2025 15:18:45 +0000
> > Phil Elwell <phil@raspberrypi.com> wrote:
> >
> > > Hi Andrea,
> > >
> > > The problem with this approach (loading an overlay from the RP1 PCIe
> > > driver), and it's one that I have raised with you offline, is that
> > > (unless anyone can prove otherwise) it becomes impossible to create a
> > > Pi 5 DTS file which makes use of the RP1's resources. How do you
> > > declare something as simple as a button wired to an RP1 GPIO, or fan
> > > connected to a PWM output?
> >
> > The driver could be improved in a second step.
> > For instance, it could load the dtbo from user-space using request_firmare()
> > instead of loading the embedded dtbo.
> >
> > >
> > > If this is the preferred route to upstream adoption, I would prefer it
> > > if rp1.dtso could be split in two - an rp1.dtsi similar to what we
> > > have downstream, and an rp1.dtso that #includes it. In this way we can
> > > keep the patching and duplication to a minimum.
> >
> > Indeed, having a rp1.dtsi avoid duplication but how the rp1.dtso in
> > the the kernel sources could include user customization (button, fan, ...)
> > without being modified ?
> > At least we have to '#include <my_rp1_customizations.dtsi>'.
> >
> > Requesting the dtbo from user-space allows to let the user to create
> > its own dtso without the need to modify the one in kernel sources.
> >
> > Does it make sense ?
> 
> I think I understand what you are saying, but at this point the RP1
> overlay would no longer be an RP1 overlay - it would be an
> RP1-and-everything-connected-to-it overlay, which is inherently
> board-specific. Which user-space process do you think would be
> responsible for loading this alternative overlay, choosing carefully
> based on the platform it is running on? Doesn't that place quite a
> burden on all the OS maintainers who up to now have just needed a
> kernel and a bunch of dtb files?
> 
> If it is considered essential that the upstream Pi 5 dts file does not
> include RP1 and its children, then Raspberry Pi are going to have to
> walk a different path until we've seen how that can work. By splitting
> rp1.dtso as I suggested, and perhaps providing an alternative helper
> function that only applies the built-in overlay if the device node
> doesn't already exist, we get to stay as close to upstream as
> possible.
> 
> Phil

So, the problem is twofold: the first is due to the fact that downstream
expects the dtb to be fully declared at fw load time (I'll call that
*monolithic* dtb from now on), the second is about how to represent dependencies
between board dtb and rp1 overlay which arises only when using overlays instead
of a monolithic dtb.

The former issue must be solved first in order for the latter to even exists
(if we don't use overlay, the dependencies are fully exposed in the dtb since
the beginning), so I'll concentrate on the former for now.

There are 3 possible scenarios to be reconciled:


1 - MONOLITHIC DTB

This is the downstream case, where it's advisable to have only one dtb blob
containing everything (rp1 included) loaded by the fw. In this case the
resulting devicetree would looks like:

  axi {
    pcie@120000 {
      rp1_nexus {
        pci-ep-bus@1 {
             ...
        }
      }
    }
  }


2 - RP1 LOADED FROM OVERLAY BY THE FW

In this case the rp1 dt node is loaded from overlay directly by the fw and the 
resulting devicetree is exactly equal to the monolithic dtb scenario.
In order for that overlay to be loaded by fw, just add 'dtoverlay=rp1' in
'config.txt'.


3 - RP1 LOADED FROM OVERLAY AT RUNTIME

Here it's the rp1 driver that loads the overlay at runtime, which is the case
that this patchset originally proposed. The devicetree ends up like this:

  axi {
    pcie@120000 {
      pci@0,0 {
        dev@0,0 {
          pci-ep-bus@1 {
               ...
          }
        }
      }
    }
  }

and this is exepcially useful to cope with the case in which there's no DT
natively used, e.g. on ACPI systems.


In order for all those 3 mentioned scenatios to work, I propose the following
inclusion scheme for for the dts files (the arrow points to the includer):
                   
 
 rp1-pci.dtso         rp1.dtso
     ^                    ^
     |                    |
rp1-common.dtsi ----> rp1-nexus.dtsi ----> bcm2712-rpi-5-b-MONOLITHIC.dts
   
   
where those dts are defined as follows (omitting the internal properties for
clarity sake):


- rp1-common.dtsi ------- // definition of core rp1 and its peripherals, common
			  // for all cases

	pci_ep_bus: pci-ep-bus@1 
	{
		rp1_clocks { };

		rp1_gpio { };

		rp1_eth { };
	};

- rp1-pci.dtso ---------- // ovl linked in the rp1 driver code to be loaded at
			  // runtime from rp1 driver. Only for case 3

	/plugin/;
	fragment@0 {
                target-path="";
                __overlay__ {
			#include "rp1-common.dtsi"
		};
	}

- rp1-nexus.dtsi ------- // adapter to decouple rp1 ranges for non runtime-loaded
		         // overlay case (i.e. only for case 1 and 2)

	rp1_nexus {
		ranges = ...
		
		 #include "rp1-common.dtsi"
	};

- rp1.dtso ------------ // overlay to be loaded by fw (case 2)

	/plugin/;
	&pcie2 {
		#include "rp1-nexus.dtsi"
	};

- bcm2712-rpi-5-b-MONOLITHIC.dts --- // monolithic dtb to avoid any overlay use
				     // (case 1)

	/ {
		... all rpi5 board dts ...
		&pcie2 {
        		#include "rp1-nexus.dtsi"
		};
	};


with only minimal changes to the rp1 driver code, I can confirm that all those
scenarios can coexits and are working fine. Before processding with a new patchset
I'd like to have some thoughts about that, do you think this is a viable approach?

Many thanks,
Andrea

Re: [PATCH v6 00/10] Add support for RaspberryPi RP1 PCI device using a DT overlay

[Phil Elwell]

Hi Andrea,

On Mon, 10 Mar 2025 at 13:58, Andrea della Porta <andrea.porta@suse.com> wrote:
>
> Hi,
>
> On 16:27 Thu 13 Feb     , Phil Elwell wrote:
> > Hi Hervé,
> >
> > On Thu, 13 Feb 2025 at 16:14, Herve Codina <herve.codina@bootlin.com> wrote:
> > >
> > > Hi Phil,
> > >
> > > On Thu, 13 Feb 2025 15:18:45 +0000
> > > Phil Elwell <phil@raspberrypi.com> wrote:
> > >
> > > > Hi Andrea,
> > > >
> > > > The problem with this approach (loading an overlay from the RP1 PCIe
> > > > driver), and it's one that I have raised with you offline, is that
> > > > (unless anyone can prove otherwise) it becomes impossible to create a
> > > > Pi 5 DTS file which makes use of the RP1's resources. How do you
> > > > declare something as simple as a button wired to an RP1 GPIO, or fan
> > > > connected to a PWM output?
> > >
> > > The driver could be improved in a second step.
> > > For instance, it could load the dtbo from user-space using request_firmare()
> > > instead of loading the embedded dtbo.
> > >
> > > >
> > > > If this is the preferred route to upstream adoption, I would prefer it
> > > > if rp1.dtso could be split in two - an rp1.dtsi similar to what we
> > > > have downstream, and an rp1.dtso that #includes it. In this way we can
> > > > keep the patching and duplication to a minimum.
> > >
> > > Indeed, having a rp1.dtsi avoid duplication but how the rp1.dtso in
> > > the the kernel sources could include user customization (button, fan, ...)
> > > without being modified ?
> > > At least we have to '#include <my_rp1_customizations.dtsi>'.
> > >
> > > Requesting the dtbo from user-space allows to let the user to create
> > > its own dtso without the need to modify the one in kernel sources.
> > >
> > > Does it make sense ?
> >
> > I think I understand what you are saying, but at this point the RP1
> > overlay would no longer be an RP1 overlay - it would be an
> > RP1-and-everything-connected-to-it overlay, which is inherently
> > board-specific. Which user-space process do you think would be
> > responsible for loading this alternative overlay, choosing carefully
> > based on the platform it is running on? Doesn't that place quite a
> > burden on all the OS maintainers who up to now have just needed a
> > kernel and a bunch of dtb files?
> >
> > If it is considered essential that the upstream Pi 5 dts file does not
> > include RP1 and its children, then Raspberry Pi are going to have to
> > walk a different path until we've seen how that can work. By splitting
> > rp1.dtso as I suggested, and perhaps providing an alternative helper
> > function that only applies the built-in overlay if the device node
> > doesn't already exist, we get to stay as close to upstream as
> > possible.
> >
> > Phil
>
> So, the problem is twofold: the first is due to the fact that downstream
> expects the dtb to be fully declared at fw load time (I'll call that
> *monolithic* dtb from now on), the second is about how to represent dependencies
> between board dtb and rp1 overlay which arises only when using overlays instead
> of a monolithic dtb.
>
> The former issue must be solved first in order for the latter to even exists
> (if we don't use overlay, the dependencies are fully exposed in the dtb since
> the beginning), so I'll concentrate on the former for now.
>
> There are 3 possible scenarios to be reconciled:
>
>
> 1 - MONOLITHIC DTB
>
> This is the downstream case, where it's advisable to have only one dtb blob
> containing everything (rp1 included) loaded by the fw. In this case the
> resulting devicetree would looks like:
>
>   axi {
>     pcie@120000 {
>       rp1_nexus {
>         pci-ep-bus@1 {
>              ...
>         }
>       }
>     }
>   }

This is indeed our current DT usage for the Pi 5 family - a single DTB
describing each board - although I would like to make it clear that
overlays can then be applied on top of it describing the attached
peripherals.

> 2 - RP1 LOADED FROM OVERLAY BY THE FW
>
> In this case the rp1 dt node is loaded from overlay directly by the fw and the
> resulting devicetree is exactly equal to the monolithic dtb scenario.
> In order for that overlay to be loaded by fw, just add 'dtoverlay=rp1' in
> 'config.txt'.

This halfway house combines the advantages and disadvantages of
scenarios 1 and 3. Compared to scenario 3 it gains support for
applying overlays that make use of interfaces provided by RP1 - I2C,
SPI, UARTs etc. Compared to scenario 1 it loses the ability for the
base DTB to refer to elements of RP1, other than by replacing these
DTB elements with overlays that must be loaded after the RP1 overlay.
As such, we would see that as a backwards step and not use it.

> 3 - RP1 LOADED FROM OVERLAY AT RUNTIME
>
> Here it's the rp1 driver that loads the overlay at runtime, which is the case
> that this patchset originally proposed. The devicetree ends up like this:
>
>   axi {
>     pcie@120000 {
>       pci@0,0 {
>         dev@0,0 {
>           pci-ep-bus@1 {
>                ...
>           }
>         }
>       }
>     }
>   }
>
> and this is exepcially useful to cope with the case in which there's no DT
> natively used, e.g. on ACPI systems.
>
>
> In order for all those 3 mentioned scenatios to work, I propose the following
> inclusion scheme for for the dts files (the arrow points to the includer):
>
>
>  rp1-pci.dtso         rp1.dtso
>      ^                    ^
>      |                    |
> rp1-common.dtsi ----> rp1-nexus.dtsi ----> bcm2712-rpi-5-b-MONOLITHIC.dts
>
>
> where those dts are defined as follows (omitting the internal properties for
> clarity sake):
>
>
> - rp1-common.dtsi ------- // definition of core rp1 and its peripherals, common
>                           // for all cases
>
>         pci_ep_bus: pci-ep-bus@1
>         {
>                 rp1_clocks { };
>
>                 rp1_gpio { };
>
>                 rp1_eth { };
>         };
>
> - rp1-pci.dtso ---------- // ovl linked in the rp1 driver code to be loaded at
>                           // runtime from rp1 driver. Only for case 3
>
>         /plugin/;
>         fragment@0 {
>                 target-path="";
>                 __overlay__ {
>                         #include "rp1-common.dtsi"
>                 };
>         }
>
> - rp1-nexus.dtsi ------- // adapter to decouple rp1 ranges for non runtime-loaded
>                          // overlay case (i.e. only for case 1 and 2)
>
>         rp1_nexus {
>                 ranges = ...
>
>                  #include "rp1-common.dtsi"
>         };
>
> - rp1.dtso ------------ // overlay to be loaded by fw (case 2)
>
>         /plugin/;
>         &pcie2 {
>                 #include "rp1-nexus.dtsi"
>         };
>
> - bcm2712-rpi-5-b-MONOLITHIC.dts --- // monolithic dtb to avoid any overlay use
>                                      // (case 1)
>
>         / {
>                 ... all rpi5 board dts ...
>                 &pcie2 {
>                         #include "rp1-nexus.dtsi"
>                 };
>         };
>
>
> with only minimal changes to the rp1 driver code, I can confirm that all those
> scenarios can coexits and are working fine. Before processding with a new patchset
> I'd like to have some thoughts about that, do you think this is a viable approach?

Thank you for this - the creation of a core RP1 declaration that can
be tailored to multiple applications using different wrappers is
exactly what I had in mind. I agree that your partitioning scheme can
cater for the 3 usage scenarios, but for the reasons outlined above I
think scenario 2 is not useful to us, although it isn't impossible
that U-boot might see things differently; I see no harm in having it
supported, but I wouldn't want anyone to go to unnecessary effort to
make it work.

Phil

Re: [PATCH v6 00/10] Add support for RaspberryPi RP1 PCI device using a DT overlay

[Andrea della Porta]

Hi Phil,

On 14:21 Mon 10 Mar     , Phil Elwell wrote:
> Hi Andrea,
> 

...

> 
> This is indeed our current DT usage for the Pi 5 family - a single DTB
> describing each board - although I would like to make it clear that
> overlays can then be applied on top of it describing the attached
> peripherals.

Indeed. Thanks for specifying that.

> 
> > 2 - RP1 LOADED FROM OVERLAY BY THE FW
> >
> > In this case the rp1 dt node is loaded from overlay directly by the fw and the
> > resulting devicetree is exactly equal to the monolithic dtb scenario.
> > In order for that overlay to be loaded by fw, just add 'dtoverlay=rp1' in
> > 'config.txt'.
> 
> This halfway house combines the advantages and disadvantages of
> scenarios 1 and 3. Compared to scenario 3 it gains support for
> applying overlays that make use of interfaces provided by RP1 - I2C,
> SPI, UARTs etc. Compared to scenario 1 it loses the ability for the
> base DTB to refer to elements of RP1, other than by replacing these
> DTB elements with overlays that must be loaded after the RP1 overlay.
> As such, we would see that as a backwards step and not use it.

This is in fact the second side of the problem I was mentioning: being
able to reference RP1 nodes from base tree (this of course arise only for
case 2 and 3, i.e. in case overlays are used). Some solutions to this have
been added by Herve (nexus nodes) and it seems promising, but I still
don't have a complete and tested solution.
My intention here is to verify if it is worth spending time on figuring
out a solution for the overlay case, since in case we only accept the
monolithic approach we won't use overlay for rp1 at all.

My idea is that we can still provide the dtbs for all three cases for the
user to choose from (so to potentially benefit from all advantages every
single solution provides), and refine the overlay case as we go, since we
can still fallback to the monolithic dtb in case we can't find a viable
solution to the across-overlay referencing issue.  But I'd like a confirm
from upstream too.
I think that this problem should really be addressed sooner or later (I'm
hinting at fpga hw, but lately we're seeing other appliances of this paradigm
arising more often), so maybe this is a good opportunity to lay the ground
for a proper solution that could be generalized for other situations too, but
again, not at the cost of slowing down rpi5 upstream efforts.

> 
> > 3 - RP1 LOADED FROM OVERLAY AT RUNTIME
> >
> > Here it's the rp1 driver that loads the overlay at runtime, which is the case
> > that this patchset originally proposed. The devicetree ends up like this:
> >
> >   axi {
> >     pcie@120000 {
> >       pci@0,0 {
> >         dev@0,0 {

...

> >         / {
> >                 ... all rpi5 board dts ...
> >                 &pcie2 {
> >                         #include "rp1-nexus.dtsi"
> >                 };
> >         };
> >
> >
> > with only minimal changes to the rp1 driver code, I can confirm that all those
> > scenarios can coexits and are working fine. Before processding with a new patchset
> > I'd like to have some thoughts about that, do you think this is a viable approach?
> 
> Thank you for this - the creation of a core RP1 declaration that can
> be tailored to multiple applications using different wrappers is
> exactly what I had in mind. I agree that your partitioning scheme can
> cater for the 3 usage scenarios, but for the reasons outlined above I
> think scenario 2 is not useful to us, although it isn't impossible
> that U-boot might see things differently; I see no harm in having it
> supported, but I wouldn't want anyone to go to unnecessary effort to
> make it work.

In all truth, this is in fact exactly your proposal (we discussed internally with
Dave also), just slightly re-arranged by me.
I agree with you about not wasting time, as I also mentioned above.

Many thanks,
Andrea

> 
> Phil