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

[Stefan Wahren <wahrenst@gmx.net>]

Hi Andrea,

Am 20.08.24 um 16:36 schrieb Andrea della Porta:
> The RaspberryPi RP1 is ia PCI multi function device containing
> peripherals ranging from Ethernet to USB controller, I2C, SPI
> and others.
sorry, i cannot provide you a code review, but just some comments. multi
function device suggests "mfd" subsystem or at least "soc" . I won't
recommend misc driver here.
> Implement a bare minimum driver to operate the RP1, leveraging
> actual OF based driver implementations for the on-borad peripherals
> by loading a devicetree overlay during driver probe.
Can you please explain why this should be a DT overlay? The RP1 is
assembled on the Raspberry Pi 5 PCB. DT overlays are typically for loose
connections like displays or HATs. I think a DTSI just for the RP1 would
fit better and is easier to read.
> The peripherals are accessed by mapping MMIO registers starting
> from PCI BAR1 region.
> As a minimum driver, the peripherals will not be added to the
> dtbo here, but in following patches.
>
> Link: https://datasheets.raspberrypi.com/rp1/rp1-peripherals.pdf
> Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
> ---
>   MAINTAINERS                           |   2 +
>   arch/arm64/boot/dts/broadcom/rp1.dtso | 152 ++++++++++++
>   drivers/misc/Kconfig                  |   1 +
>   drivers/misc/Makefile                 |   1 +
>   drivers/misc/rp1/Kconfig              |  20 ++
>   drivers/misc/rp1/Makefile             |   3 +
>   drivers/misc/rp1/rp1-pci.c            | 333 ++++++++++++++++++++++++++
>   drivers/misc/rp1/rp1-pci.dtso         |   8 +
>   drivers/pci/quirks.c                  |   1 +
>   include/linux/pci_ids.h               |   3 +
>   10 files changed, 524 insertions(+)
>   create mode 100644 arch/arm64/boot/dts/broadcom/rp1.dtso
>   create mode 100644 drivers/misc/rp1/Kconfig
>   create mode 100644 drivers/misc/rp1/Makefile
>   create mode 100644 drivers/misc/rp1/rp1-pci.c
>   create mode 100644 drivers/misc/rp1/rp1-pci.dtso
>
...
> +
> +				rp1_clocks: clocks@c040018000 {
> +					compatible = "raspberrypi,rp1-clocks";
> +					#clock-cells = <1>;
> +					reg = <0xc0 0x40018000 0x0 0x10038>;
> +					clocks = <&clk_xosc>;
> +					clock-names = "xosc";
> +
> +					assigned-clocks = <&rp1_clocks RP1_PLL_SYS_CORE>,
> +							  <&rp1_clocks RP1_PLL_AUDIO_CORE>,
> +							  // RP1_PLL_VIDEO_CORE and dividers are now managed by VEC,DPI drivers
> +							  <&rp1_clocks RP1_PLL_SYS>,
> +							  <&rp1_clocks RP1_PLL_SYS_SEC>,
> +							  <&rp1_clocks RP1_PLL_SYS_PRI_PH>,
> +							  <&rp1_clocks RP1_CLK_ETH_TSU>;
> +
> +					assigned-clock-rates = <1000000000>, // RP1_PLL_SYS_CORE
> +							       <1536000000>, // RP1_PLL_AUDIO_CORE
> +							       <200000000>,  // RP1_PLL_SYS
> +							       <125000000>,  // RP1_PLL_SYS_SEC
> +							       <100000000>,  // RP1_PLL_SYS_PRI_PH
> +							       <50000000>;   // RP1_CLK_ETH_TSU
> +				};
> +
> +				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 = <RP1_INT_IO_BANK0 IRQ_TYPE_LEVEL_HIGH>,
> +						     <RP1_INT_IO_BANK1 IRQ_TYPE_LEVEL_HIGH>,
> +						     <RP1_INT_IO_BANK2 IRQ_TYPE_LEVEL_HIGH>;
> +					gpio-line-names =
> +						"ID_SDA", // GPIO0
> +						"ID_SCL", // GPIO1
> +						"GPIO2", // GPIO2
> +						"GPIO3", // GPIO3
> +						"GPIO4", // GPIO4
> +						"GPIO5", // GPIO5
> +						"GPIO6", // GPIO6
> +						"GPIO7", // GPIO7
> +						"GPIO8", // GPIO8
> +						"GPIO9", // GPIO9
> +						"GPIO10", // GPIO10
> +						"GPIO11", // GPIO11
> +						"GPIO12", // GPIO12
> +						"GPIO13", // GPIO13
> +						"GPIO14", // GPIO14
> +						"GPIO15", // GPIO15
> +						"GPIO16", // GPIO16
> +						"GPIO17", // GPIO17
> +						"GPIO18", // GPIO18
> +						"GPIO19", // GPIO19
> +						"GPIO20", // GPIO20
> +						"GPIO21", // GPIO21
> +						"GPIO22", // GPIO22
> +						"GPIO23", // GPIO23
> +						"GPIO24", // GPIO24
> +						"GPIO25", // GPIO25
> +						"GPIO26", // GPIO26
> +						"GPIO27", // 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
GPIO line names are board specific, so this should go to the Raspberry
Pi 5 file.
> +				};
> +			};
> +		};
> +	};
> +};
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 41c3d2821a78..02405209e6c4 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -618,4 +618,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 c2f990862d2b..84bfa866fbee 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -71,3 +71,4 @@ obj-$(CONFIG_TPS6594_PFSM)	+= tps6594-pfsm.o
>   obj-$(CONFIG_NSM)		+= nsm.o
>   obj-$(CONFIG_MARVELL_CN10K_DPI)	+= mrvl_cn10k_dpi.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..050417ee09ae
> --- /dev/null
> +++ b/drivers/misc/rp1/Kconfig
> @@ -0,0 +1,20 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# RaspberryPi RP1 misc device
> +#
> +
> +config MISC_RP1
> +        tristate "RaspberryPi RP1 PCIe support"
> +        depends on PCI && PCI_QUIRKS
> +        select OF
> +        select OF_OVERLAY
> +        select IRQ_DOMAIN
> +        select PCI_DYNAMIC_OF_NODES
> +        help
> +          Support for 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..e83854b4ed2c
> --- /dev/null
> +++ b/drivers/misc/rp1/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +rp1-pci-objs			:= rp1-pci.o rp1-pci.dtbo.o
> +obj-$(CONFIG_MISC_RP1)		+= rp1-pci.o
> diff --git a/drivers/misc/rp1/rp1-pci.c b/drivers/misc/rp1/rp1-pci.c
> new file mode 100644
> index 000000000000..a6093ba7e19a
> --- /dev/null
> +++ b/drivers/misc/rp1/rp1-pci.c
> @@ -0,0 +1,333 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018-22 Raspberry Pi Ltd.
> + * All rights reserved.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clkdev.h>
> +#include <linux/clk-provider.h>
> +#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 <linux/reset.h>
> +
> +#include <dt-bindings/misc/rp1.h>
> +
> +#define RP1_B0_CHIP_ID		0x10001927
> +#define RP1_C0_CHIP_ID		0x20001927
> +
> +#define RP1_PLATFORM_ASIC	BIT(1)
> +#define RP1_PLATFORM_FPGA	BIT(0)
> +
> +#define RP1_DRIVER_NAME		"rp1"
> +
> +#define RP1_ACTUAL_IRQS		RP1_INT_END
> +#define RP1_IRQS		RP1_ACTUAL_IRQS
> +#define RP1_HW_IRQ_MASK		GENMASK(5, 0)
> +
> +#define RP1_SYSCLK_RATE		200000000
> +#define RP1_SYSCLK_FPGA_RATE	60000000
> +
> +enum {
> +	SYSINFO_CHIP_ID_OFFSET	= 0,
> +	SYSINFO_PLATFORM_OFFSET	= 4,
> +};
> +
> +#define REG_SET			0x800
> +#define REG_CLR			0xc00
> +
> +/* MSIX 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_TEST           BIT(1)
> +#define MSIX_CFG_ENABLE         BIT(0)
> +
> +#define INTSTATL		0x108
> +#define INTSTATH		0x10c
> +
> +extern char __dtbo_rp1_pci_begin[];
> +extern char __dtbo_rp1_pci_end[];
> +
> +struct rp1_dev {
> +	struct pci_dev *pdev;
> +	struct device *dev;
> +	struct clk *sys_clk;
> +	struct irq_domain *domain;
> +	struct irq_data *pcie_irqds[64];
> +	void __iomem *bar1;
> +	int ovcs_id;
> +	bool level_triggered_irq[RP1_ACTUAL_IRQS];
> +};
> +
> +
...
> +
> +static const struct pci_device_id dev_id_table[] = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_RPI, PCI_DEVICE_ID_RP1_C0), },
> +	{ 0, }
> +};
> +
> +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 & Copyright doesn't seem to match with this patch author.
Please clarify/fix