| Message ID | 5954e4dccc0e158cf434d2c281ad57120538409b.1724159867.git.andrea.porta@suse.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for RaspberryPi RP1 PCI device using a DT overlay | expand |
On Tue, Aug 20, 2024 at 04:36:10PM +0200, Andrea della Porta wrote: > The RaspberryPi RP1 is ia 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-borad peripherals > by loading a devicetree overlay during driver probe. > 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 ++++++++++++ Do not mix DTS with drivers. These MUST be separate. > 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 > > diff --git a/MAINTAINERS b/MAINTAINERS > index 67f460c36ea1..1359538b76e8 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -19119,9 +19119,11 @@ F: include/uapi/linux/media/raspberrypi/ > 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/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/arch/arm64/boot/dts/broadcom/rp1.dtso b/arch/arm64/boot/dts/broadcom/rp1.dtso > new file mode 100644 > index 000000000000..d80178a278ee > --- /dev/null > +++ b/arch/arm64/boot/dts/broadcom/rp1.dtso > @@ -0,0 +1,152 @@ > +// 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/rp1.h> > +#include <dt-bindings/misc/rp1.h> > + > +/dts-v1/; > +/plugin/; > + > +/ { > + fragment@0 { > + target-path=""; > + __overlay__ { > + #address-cells = <3>; > + #size-cells = <2>; > + > + rp1: rp1@0 { > + compatible = "simple-bus"; > + #address-cells = <2>; > + #size-cells = <2>; > + interrupt-controller; > + interrupt-parent = <&rp1>; > + #interrupt-cells = <2>; > + > + // ranges and dma-ranges must be provided by the includer > + ranges = <0xc0 0x40000000 > + 0x01/*0x02000000*/ 0x00 0x00000000 > + 0x00 0x00400000>; Are you 100% sure you do not have here dtc W=1 warnings? > + > + dma-ranges = > + // inbound RP1 1x_xxxxxxxx -> PCIe 1x_xxxxxxxx > + <0x10 0x00000000 > + 0x43000000 0x10 0x00000000 > + 0x10 0x00000000>; > + > + clk_xosc: clk_xosc { Nope, switch to DTS coding style. > + compatible = "fixed-clock"; > + #clock-cells = <0>; > + clock-output-names = "xosc"; > + clock-frequency = <50000000>; > + }; > + > + macb_pclk: macb_pclk { > + compatible = "fixed-clock"; > + #clock-cells = <0>; > + clock-output-names = "pclk"; > + clock-frequency = <200000000>; > + }; > + > + macb_hclk: macb_hclk { > + compatible = "fixed-clock"; > + #clock-cells = <0>; > + clock-output-names = "hclk"; > + clock-frequency = <200000000>; > + }; > + > + rp1_clocks: clocks@c040018000 { Why do you mix MMIO with non-MMIO nodes? This really does not look correct. > + compatible = "raspberrypi,rp1-clocks"; > + #clock-cells = <1>; > + reg = <0xc0 0x40018000 0x0 0x10038>; Wrong order of properties - see DTS coding style. > + clocks = <&clk_xosc>; > + clock-names = "xosc"; Best regards, Krzysztof
Hi Andrea,
kernel test robot noticed the following build warnings:
[auto build test WARNING on clk/clk-next]
[also build test WARNING on robh/for-next char-misc/char-misc-testing char-misc/char-misc-next char-misc/char-misc-linus linus/master v6.11-rc4 next-20240821]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Andrea-della-Porta/dt-bindings-clock-Add-RaspberryPi-RP1-clock-bindings/20240821-023901
base: https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
patch link: https://lore.kernel.org/r/5954e4dccc0e158cf434d2c281ad57120538409b.1724159867.git.andrea.porta%40suse.com
patch subject: [PATCH 08/11] misc: rp1: RaspberryPi RP1 misc driver
config: sparc-kismet-CONFIG_PCI_DYNAMIC_OF_NODES-CONFIG_MISC_RP1-0-0 (https://download.01.org/0day-ci/archive/20240821/202408212017.eN5JVu0P-lkp@intel.com/config)
reproduce: (https://download.01.org/0day-ci/archive/20240821/202408212017.eN5JVu0P-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408212017.eN5JVu0P-lkp@intel.com/
kismet warnings: (new ones prefixed by >>)
>> kismet: WARNING: unmet direct dependencies detected for PCI_DYNAMIC_OF_NODES when selected by MISC_RP1
WARNING: unmet direct dependencies detected for PCI_DYNAMIC_OF_NODES
Depends on [n]: PCI [=y] && OF_IRQ [=n]
Selected by [y]:
- MISC_RP1 [=y] && PCI [=y] && PCI_QUIRKS [=y]
Hi Andrea, kernel test robot noticed the following build errors: [auto build test ERROR on clk/clk-next] [also build test ERROR on robh/for-next char-misc/char-misc-testing char-misc/char-misc-next char-misc/char-misc-linus linus/master v6.11-rc4 next-20240821] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Andrea-della-Porta/dt-bindings-clock-Add-RaspberryPi-RP1-clock-bindings/20240821-023901 base: https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next patch link: https://lore.kernel.org/r/5954e4dccc0e158cf434d2c281ad57120538409b.1724159867.git.andrea.porta%40suse.com patch subject: [PATCH 08/11] misc: rp1: RaspberryPi RP1 misc driver config: x86_64-randconfig-r133-20240821 (https://download.01.org/0day-ci/archive/20240821/202408212114.i6MFeKR1-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240821/202408212114.i6MFeKR1-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202408212114.i6MFeKR1-lkp@intel.com/ All errors (new ones prefixed by >>): drivers/misc/rp1/rp1-pci.c: In function 'rp1_mask_irq': >> drivers/misc/rp1/rp1-pci.c:98:9: error: implicit declaration of function 'pci_msi_mask_irq'; did you mean 'pci_msix_free_irq'? [-Werror=implicit-function-declaration] 98 | pci_msi_mask_irq(pcie_irqd); | ^~~~~~~~~~~~~~~~ | pci_msix_free_irq drivers/misc/rp1/rp1-pci.c: In function 'rp1_unmask_irq': >> drivers/misc/rp1/rp1-pci.c:106:9: error: implicit declaration of function 'pci_msi_unmask_irq' [-Werror=implicit-function-declaration] 106 | pci_msi_unmask_irq(pcie_irqd); | ^~~~~~~~~~~~~~~~~~ cc1: some warnings being treated as errors vim +98 drivers/misc/rp1/rp1-pci.c 92 93 static void rp1_mask_irq(struct irq_data *irqd) 94 { 95 struct rp1_dev *rp1 = irqd->domain->host_data; 96 struct irq_data *pcie_irqd = rp1->pcie_irqds[irqd->hwirq]; 97 > 98 pci_msi_mask_irq(pcie_irqd); 99 } 100 101 static void rp1_unmask_irq(struct irq_data *irqd) 102 { 103 struct rp1_dev *rp1 = irqd->domain->host_data; 104 struct irq_data *pcie_irqd = rp1->pcie_irqds[irqd->hwirq]; 105 > 106 pci_msi_unmask_irq(pcie_irqd); 107 } 108
On 21/08/2024 10:38, Krzysztof Kozlowski wrote: > On Tue, Aug 20, 2024 at 04:36:10PM +0200, Andrea della Porta wrote: ... >> 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 >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 67f460c36ea1..1359538b76e8 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -19119,9 +19119,11 @@ F: include/uapi/linux/media/raspberrypi/ >> 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/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/arch/arm64/boot/dts/broadcom/rp1.dtso b/arch/arm64/boot/dts/broadcom/rp1.dtso >> new file mode 100644 >> index 000000000000..d80178a278ee >> --- /dev/null >> +++ b/arch/arm64/boot/dts/broadcom/rp1.dtso >> @@ -0,0 +1,152 @@ >> +// 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/rp1.h> >> +#include <dt-bindings/misc/rp1.h> >> + >> +/dts-v1/; >> +/plugin/; >> + >> +/ { >> + fragment@0 { >> + target-path=""; >> + __overlay__ { >> + #address-cells = <3>; >> + #size-cells = <2>; >> + >> + rp1: rp1@0 { >> + compatible = "simple-bus"; >> + #address-cells = <2>; >> + #size-cells = <2>; >> + interrupt-controller; >> + interrupt-parent = <&rp1>; >> + #interrupt-cells = <2>; >> + >> + // ranges and dma-ranges must be provided by the includer >> + ranges = <0xc0 0x40000000 >> + 0x01/*0x02000000*/ 0x00 0x00000000 >> + 0x00 0x00400000>; > > Are you 100% sure you do not have here dtc W=1 warnings? One more thing, I do not see this overlay applied to any target, which means it cannot be tested. You miss entry in Makefile. Best regards, Krzysztof
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
On Tue, Aug 20, 2024 at 04:36:10PM +0200, Andrea della Porta wrote: > The RaspberryPi RP1 is ia PCI multi function device containing s/ia/a/ > peripherals ranging from Ethernet to USB controller, I2C, SPI > and others. Add blank lines between paragraphs. > Implement a bare minimum driver to operate the RP1, leveraging > actual OF based driver implementations for the on-borad peripherals s/on-borad/on-board/ > by loading a devicetree overlay during driver probe. > 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. > +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. s/support for/support/ Add blank lines between paragraphs. Consider wrapping to fit in 80 columns. Current width of 86 seems random. > 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. s/22/24/ ? > +#define RP1_B0_CHIP_ID 0x10001927 > +#define RP1_C0_CHIP_ID 0x20001927 Drop; both unused. > +#define RP1_PLATFORM_ASIC BIT(1) > +#define RP1_PLATFORM_FPGA BIT(0) Drop; both unused. > +#define RP1_SYSCLK_RATE 200000000 > +#define RP1_SYSCLK_FPGA_RATE 60000000 Drop; both unused. > +enum { > + SYSINFO_CHIP_ID_OFFSET = 0, > + SYSINFO_PLATFORM_OFFSET = 4, > +}; Drop; unused. > +/* MSIX CFG registers start at 0x8 */ s/MSIX/MSI-X/ > +#define MSIX_CFG_TEST BIT(1) Unused. > +#define INTSTATL 0x108 > +#define INTSTATH 0x10c Drop; both unused. > +static void dump_bar(struct pci_dev *pdev, unsigned int bar) > +{ > + dev_info(&pdev->dev, > + "bar%d len 0x%llx, start 0x%llx, end 0x%llx, flags, 0x%lx\n", %pR does most of this for you. > +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; > + int ret = 0; > + > + switch (type) { > + case IRQ_TYPE_LEVEL_HIGH: > + dev_dbg(rp1->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: > + ret = -EINVAL; If you "return -EINVAL" directly here, I think you can drop "ret" and just "return 0" below. > + break; > + } > + > + return ret; > +} > +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) { > + 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 ret; if (ret) return ret; ... return 0; would make this easier to read and unindent the normal path. > + rp1->bar1 = pci_iomap(pdev, 1, 0); pcim_iomap() > + if (!rp1->bar1) { > + dev_err(&pdev->dev, "Cannot map PCI bar\n"); s/bar/BAR/ > +#define PCI_VENDOR_ID_RPI 0x1de4 > +#define PCI_DEVICE_ID_RP1_C0 0x0001 Device ID should include "RPI" as well.
Hi Andrea,
kernel test robot noticed the following build warnings:
[auto build test WARNING on clk/clk-next]
[also build test WARNING on robh/for-next char-misc/char-misc-testing char-misc/char-misc-next char-misc/char-misc-linus linus/master v6.11-rc4 next-20240821]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Andrea-della-Porta/dt-bindings-clock-Add-RaspberryPi-RP1-clock-bindings/20240821-023901
base: https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
patch link: https://lore.kernel.org/r/5954e4dccc0e158cf434d2c281ad57120538409b.1724159867.git.andrea.porta%40suse.com
patch subject: [PATCH 08/11] misc: rp1: RaspberryPi RP1 misc driver
config: mips-allyesconfig (https://download.01.org/0day-ci/archive/20240822/202408220150.bmFMT5Bk-lkp@intel.com/config)
compiler: mips-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240822/202408220150.bmFMT5Bk-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408220150.bmFMT5Bk-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from include/linux/device.h:15,
from include/linux/pci.h:37,
from drivers/misc/rp1/rp1-pci.c:18:
drivers/misc/rp1/rp1-pci.c: In function 'dump_bar':
>> drivers/misc/rp1/rp1-pci.c:75:18: warning: format '%llx' expects argument of type 'long long unsigned int', but argument 4 has type 'resource_size_t' {aka 'unsigned int'} [-Wformat=]
75 | "bar%d len 0x%llx, start 0x%llx, end 0x%llx, flags, 0x%lx\n",
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/dev_printk.h:110:30: note: in definition of macro 'dev_printk_index_wrap'
110 | _p_func(dev, fmt, ##__VA_ARGS__); \
| ^~~
include/linux/dev_printk.h:160:58: note: in expansion of macro 'dev_fmt'
160 | dev_printk_index_wrap(_dev_info, KERN_INFO, dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~
drivers/misc/rp1/rp1-pci.c:74:9: note: in expansion of macro 'dev_info'
74 | dev_info(&pdev->dev,
| ^~~~~~~~
drivers/misc/rp1/rp1-pci.c:75:34: note: format string is defined here
75 | "bar%d len 0x%llx, start 0x%llx, end 0x%llx, flags, 0x%lx\n",
| ~~~^
| |
| long long unsigned int
| %x
drivers/misc/rp1/rp1-pci.c:75:18: warning: format '%llx' expects argument of type 'long long unsigned int', but argument 5 has type 'resource_size_t' {aka 'unsigned int'} [-Wformat=]
75 | "bar%d len 0x%llx, start 0x%llx, end 0x%llx, flags, 0x%lx\n",
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/dev_printk.h:110:30: note: in definition of macro 'dev_printk_index_wrap'
110 | _p_func(dev, fmt, ##__VA_ARGS__); \
| ^~~
include/linux/dev_printk.h:160:58: note: in expansion of macro 'dev_fmt'
160 | dev_printk_index_wrap(_dev_info, KERN_INFO, dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~
drivers/misc/rp1/rp1-pci.c:74:9: note: in expansion of macro 'dev_info'
74 | dev_info(&pdev->dev,
| ^~~~~~~~
drivers/misc/rp1/rp1-pci.c:75:48: note: format string is defined here
75 | "bar%d len 0x%llx, start 0x%llx, end 0x%llx, flags, 0x%lx\n",
| ~~~^
| |
| long long unsigned int
| %x
drivers/misc/rp1/rp1-pci.c:75:18: warning: format '%llx' expects argument of type 'long long unsigned int', but argument 6 has type 'resource_size_t' {aka 'unsigned int'} [-Wformat=]
75 | "bar%d len 0x%llx, start 0x%llx, end 0x%llx, flags, 0x%lx\n",
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/dev_printk.h:110:30: note: in definition of macro 'dev_printk_index_wrap'
110 | _p_func(dev, fmt, ##__VA_ARGS__); \
| ^~~
include/linux/dev_printk.h:160:58: note: in expansion of macro 'dev_fmt'
160 | dev_printk_index_wrap(_dev_info, KERN_INFO, dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~
drivers/misc/rp1/rp1-pci.c:74:9: note: in expansion of macro 'dev_info'
74 | dev_info(&pdev->dev,
| ^~~~~~~~
drivers/misc/rp1/rp1-pci.c:75:60: note: format string is defined here
75 | "bar%d len 0x%llx, start 0x%llx, end 0x%llx, flags, 0x%lx\n",
| ~~~^
| |
| long long unsigned int
| %x
vim +75 drivers/misc/rp1/rp1-pci.c
> 18 #include <linux/pci.h>
19 #include <linux/platform_device.h>
20 #include <linux/reset.h>
21
22 #include <dt-bindings/misc/rp1.h>
23
24 #define RP1_B0_CHIP_ID 0x10001927
25 #define RP1_C0_CHIP_ID 0x20001927
26
27 #define RP1_PLATFORM_ASIC BIT(1)
28 #define RP1_PLATFORM_FPGA BIT(0)
29
30 #define RP1_DRIVER_NAME "rp1"
31
32 #define RP1_ACTUAL_IRQS RP1_INT_END
33 #define RP1_IRQS RP1_ACTUAL_IRQS
34 #define RP1_HW_IRQ_MASK GENMASK(5, 0)
35
36 #define RP1_SYSCLK_RATE 200000000
37 #define RP1_SYSCLK_FPGA_RATE 60000000
38
39 enum {
40 SYSINFO_CHIP_ID_OFFSET = 0,
41 SYSINFO_PLATFORM_OFFSET = 4,
42 };
43
44 #define REG_SET 0x800
45 #define REG_CLR 0xc00
46
47 /* MSIX CFG registers start at 0x8 */
48 #define MSIX_CFG(x) (0x8 + (4 * (x)))
49
50 #define MSIX_CFG_IACK_EN BIT(3)
51 #define MSIX_CFG_IACK BIT(2)
52 #define MSIX_CFG_TEST BIT(1)
53 #define MSIX_CFG_ENABLE BIT(0)
54
55 #define INTSTATL 0x108
56 #define INTSTATH 0x10c
57
58 extern char __dtbo_rp1_pci_begin[];
59 extern char __dtbo_rp1_pci_end[];
60
61 struct rp1_dev {
62 struct pci_dev *pdev;
63 struct device *dev;
64 struct clk *sys_clk;
65 struct irq_domain *domain;
66 struct irq_data *pcie_irqds[64];
67 void __iomem *bar1;
68 int ovcs_id;
69 bool level_triggered_irq[RP1_ACTUAL_IRQS];
70 };
71
72 static void dump_bar(struct pci_dev *pdev, unsigned int bar)
73 {
74 dev_info(&pdev->dev,
> 75 "bar%d len 0x%llx, start 0x%llx, end 0x%llx, flags, 0x%lx\n",
76 bar,
77 pci_resource_len(pdev, bar),
78 pci_resource_start(pdev, bar),
79 pci_resource_end(pdev, bar),
80 pci_resource_flags(pdev, bar));
81 }
82
Hi Krzysztof, On 16:20 Wed 21 Aug , Krzysztof Kozlowski wrote: > On 21/08/2024 10:38, Krzysztof Kozlowski wrote: > > On Tue, Aug 20, 2024 at 04:36:10PM +0200, Andrea della Porta wrote: > > ... > > >> 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 > >> > >> diff --git a/MAINTAINERS b/MAINTAINERS > >> index 67f460c36ea1..1359538b76e8 100644 > >> --- a/MAINTAINERS > >> +++ b/MAINTAINERS > >> @@ -19119,9 +19119,11 @@ F: include/uapi/linux/media/raspberrypi/ > >> 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/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/arch/arm64/boot/dts/broadcom/rp1.dtso b/arch/arm64/boot/dts/broadcom/rp1.dtso > >> new file mode 100644 > >> index 000000000000..d80178a278ee > >> --- /dev/null > >> +++ b/arch/arm64/boot/dts/broadcom/rp1.dtso > >> @@ -0,0 +1,152 @@ > >> +// 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/rp1.h> > >> +#include <dt-bindings/misc/rp1.h> > >> + > >> +/dts-v1/; > >> +/plugin/; > >> + > >> +/ { > >> + fragment@0 { > >> + target-path=""; > >> + __overlay__ { > >> + #address-cells = <3>; > >> + #size-cells = <2>; > >> + > >> + rp1: rp1@0 { > >> + compatible = "simple-bus"; > >> + #address-cells = <2>; > >> + #size-cells = <2>; > >> + interrupt-controller; > >> + interrupt-parent = <&rp1>; > >> + #interrupt-cells = <2>; > >> + > >> + // ranges and dma-ranges must be provided by the includer > >> + ranges = <0xc0 0x40000000 > >> + 0x01/*0x02000000*/ 0x00 0x00000000 > >> + 0x00 0x00400000>; > > > > Are you 100% sure you do not have here dtc W=1 warnings? > > One more thing, I do not see this overlay applied to any target, which > means it cannot be tested. You miss entry in Makefile. > The dtso is intended to be built from driver/misc/rp1/Makefile as it will be included in the driver obj: --- /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 and not as part of the dtb system, hence it's m issing in arch/arm64/boot/dts/broadcom/Makefile. On the other hand: #> make W=1 CHECK_DTBS=y broadcom/rp1.dtbo DTC arch/arm64/boot/dts/broadcom/rp1.dtbo arch/arm64/boot/dts/broadcom/rp1.dtso:37.24-42.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/clk_xosc: missing or empty reg/ranges property arch/arm64/boot/dts/broadcom/rp1.dtso:44.26-49.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/macb_pclk: missing or empty reg/ranges property arch/arm64/boot/dts/broadcom/rp1.dtso:51.26-56.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/macb_hclk: missing or empty reg/ranges property arch/arm64/boot/dts/broadcom/rp1.dtso:14.15-173.5: Warning (avoid_unnecessary_addr_size): /fragment@0/__overlay__: unnecessary #address-cells/#size-cells without "ranges", "dma-ranges" or child "reg" property seems to do the checks, unless I'm missing something. Thanks, Andrea > Best regards, > Krzysztof >
On 22/08/2024 16:33, Andrea della Porta wrote: > Hi Krzysztof, > > On 16:20 Wed 21 Aug , Krzysztof Kozlowski wrote: >> On 21/08/2024 10:38, Krzysztof Kozlowski wrote: >>> On Tue, Aug 20, 2024 at 04:36:10PM +0200, Andrea della Porta wrote: >> >> ... >> >>>> 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 >>>> >>>> diff --git a/MAINTAINERS b/MAINTAINERS >>>> index 67f460c36ea1..1359538b76e8 100644 >>>> --- a/MAINTAINERS >>>> +++ b/MAINTAINERS >>>> @@ -19119,9 +19119,11 @@ F: include/uapi/linux/media/raspberrypi/ >>>> 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/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/arch/arm64/boot/dts/broadcom/rp1.dtso b/arch/arm64/boot/dts/broadcom/rp1.dtso >>>> new file mode 100644 >>>> index 000000000000..d80178a278ee >>>> --- /dev/null >>>> +++ b/arch/arm64/boot/dts/broadcom/rp1.dtso >>>> @@ -0,0 +1,152 @@ >>>> +// 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/rp1.h> >>>> +#include <dt-bindings/misc/rp1.h> >>>> + >>>> +/dts-v1/; >>>> +/plugin/; >>>> + >>>> +/ { >>>> + fragment@0 { >>>> + target-path=""; >>>> + __overlay__ { >>>> + #address-cells = <3>; >>>> + #size-cells = <2>; >>>> + >>>> + rp1: rp1@0 { >>>> + compatible = "simple-bus"; >>>> + #address-cells = <2>; >>>> + #size-cells = <2>; >>>> + interrupt-controller; >>>> + interrupt-parent = <&rp1>; >>>> + #interrupt-cells = <2>; >>>> + >>>> + // ranges and dma-ranges must be provided by the includer >>>> + ranges = <0xc0 0x40000000 >>>> + 0x01/*0x02000000*/ 0x00 0x00000000 >>>> + 0x00 0x00400000>; >>> >>> Are you 100% sure you do not have here dtc W=1 warnings? >> >> One more thing, I do not see this overlay applied to any target, which >> means it cannot be tested. You miss entry in Makefile. >> > > The dtso is intended to be built from driver/misc/rp1/Makefile as it will > be included in the driver obj: > > --- /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 > > and not as part of the dtb system, hence it's m issing in > arch/arm64/boot/dts/broadcom/Makefile. > > On the other hand: > > #> make W=1 CHECK_DTBS=y broadcom/rp1.dtbo > DTC arch/arm64/boot/dts/broadcom/rp1.dtbo > arch/arm64/boot/dts/broadcom/rp1.dtso:37.24-42.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/clk_xosc: missing or empty reg/ranges property > arch/arm64/boot/dts/broadcom/rp1.dtso:44.26-49.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/macb_pclk: missing or empty reg/ranges property > arch/arm64/boot/dts/broadcom/rp1.dtso:51.26-56.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/macb_hclk: missing or empty reg/ranges property > arch/arm64/boot/dts/broadcom/rp1.dtso:14.15-173.5: Warning (avoid_unnecessary_addr_size): /fragment@0/__overlay__: unnecessary #address-cells/#size-cells without "ranges", "dma-ranges" or child "reg" property > > seems to do the checks, unless I'm missing something. Yeah, there is still no target which applies the overlay, so no one can tell whether it applies cleanly or not. You can only test single overlay, but it is expected to test each overlay being applied to chosen DTB. Best regards, Krzysztof
Hi Stefan, On 18:20 Wed 21 Aug , Stefan Wahren wrote: > 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. It's true that RP1 can be called an MFD but the reason for not placing it in mfd subsystem are twofold: - these discussions are quite clear about this matter: please see [1] and [2] - the current driver use no mfd API at all This RP1 driver is not currently addressing any aspect of ARM core in the SoC so I would say it should not stay in drivers/soc / either, as also condifirmed by [2] again and [3] (note that Microchip LAN966x is a very close fit to what we have here on RP1). > > 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 dtsi solution you proposed is the one adopted downstream. It has its benefits of course, but there's more. 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 be possibly be reused in other hw implementations. I believe a similar reasoning could be applied to Bootlin's Microchip LAN966x patchset as well, and they also choose to approach the dtb overlay. Plus, a solution that can (althoguh proabbly in teh long run) cope with both DT or ACPI based system has been kindly requested, plase see [4] for details. IMHO the approach proposed from RH et al. of using dtbo for this 'special' kind of drivers makes a lot of sense (see [5]). > > 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. Could we instead just name them with generic GPIO'N' where N is the number of the gpio? Much like many of that pins already are... in this way we don't add a dependency in the board dts to the rp1_gpio node, which is not even there when the main dts is parsed at boot, since the dtbo will be added only on PCI enumeration of the RP1 device. Or even better: since we don't explicitly use the gpio names to address them (e.g. phy-reset-gpios in rp1_eth node is addressing the ETH_RST_N gpio by number), can we just get rid of the gpio-line-names property? Also Bootlin's Microchip gpio node seems to avoid naming them... > > + }; > > + }; > > + }; > > + }; > > +}; > > 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 My intention here is that, even if the code has been heavily modified by me, the core original code is still there so I just wanted to tribute it to the original author. I'll synchronize this with RaspberryPi guys and coem up with a unified solution. Just in case: would multiple MODULE_AUTHOR entries (one with my name and one with original authors name) be accepetd? Many thanks, Andrea References: - [1]: https://lore.kernel.org/all/20240612140208.GC1504919@google.com/ - [2]: https://lore.kernel.org/all/83f7fa09-d0e6-4f36-a27d-cee08979be2a@app.fastmail.com/ - [3]: https://lore.kernel.org/all/2024081356-mutable-everyday-6f9d@gregkh/ - [4]: https://lore.kernel.org/all/ba8cdf39-3ba3-4abc-98f5-d394d6867f95@gmx.net/ - [5]: https://lpc.events/event/17/contributions/1421/attachments/1337/2680/LPC2023%20Non-discoverable%20devices%20in%20PCI.pdf
On 11:55 Wed 21 Aug , Bjorn Helgaas wrote: > On Tue, Aug 20, 2024 at 04:36:10PM +0200, Andrea della Porta wrote: > > The RaspberryPi RP1 is ia PCI multi function device containing > > s/ia/a/ > > > peripherals ranging from Ethernet to USB controller, I2C, SPI > > and others. > > Add blank lines between paragraphs. > > > Implement a bare minimum driver to operate the RP1, leveraging > > actual OF based driver implementations for the on-borad peripherals > > s/on-borad/on-board/ > > > by loading a devicetree overlay during driver probe. > > 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. > > > +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. > > s/support for/support/ > > Add blank lines between paragraphs. Consider wrapping to fit in 80 > columns. Current width of 86 seems random. > > > 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. > > s/22/24/ ? > > > +#define RP1_B0_CHIP_ID 0x10001927 > > +#define RP1_C0_CHIP_ID 0x20001927 > > Drop; both unused. > > > +#define RP1_PLATFORM_ASIC BIT(1) > > +#define RP1_PLATFORM_FPGA BIT(0) > > Drop; both unused. > > > +#define RP1_SYSCLK_RATE 200000000 > > +#define RP1_SYSCLK_FPGA_RATE 60000000 > > Drop; both unused. > > > +enum { > > + SYSINFO_CHIP_ID_OFFSET = 0, > > + SYSINFO_PLATFORM_OFFSET = 4, > > +}; > > Drop; unused. > > > +/* MSIX CFG registers start at 0x8 */ > > s/MSIX/MSI-X/ > > > +#define MSIX_CFG_TEST BIT(1) > > Unused. > > > +#define INTSTATL 0x108 > > +#define INTSTATH 0x10c > > Drop; both unused. > > > +static void dump_bar(struct pci_dev *pdev, unsigned int bar) > > +{ > > + dev_info(&pdev->dev, > > + "bar%d len 0x%llx, start 0x%llx, end 0x%llx, flags, 0x%lx\n", > > %pR does most of this for you. > > > +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; > > + int ret = 0; > > + > > + switch (type) { > > + case IRQ_TYPE_LEVEL_HIGH: > > + dev_dbg(rp1->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: > > + ret = -EINVAL; > > If you "return -EINVAL" directly here, I think you can drop "ret" and > just "return 0" below. > > > + break; > > + } > > + > > + return ret; > > +} > > > +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) { > > + 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 ret; > > if (ret) > return ret; > > ... > return 0; > > would make this easier to read and unindent the normal path. > > > + rp1->bar1 = pci_iomap(pdev, 1, 0); > > pcim_iomap() > > > + if (!rp1->bar1) { > > + dev_err(&pdev->dev, "Cannot map PCI bar\n"); > > s/bar/BAR/ > > > +#define PCI_VENDOR_ID_RPI 0x1de4 > > +#define PCI_DEVICE_ID_RP1_C0 0x0001 > > Device ID should include "RPI" as well. Ack to all suggestions. Fixed in the next release, thanks. Andrea
Hi Andrea, Am 23.08.24 um 11:44 schrieb Andrea della Porta: > Hi Stefan, > > On 18:20 Wed 21 Aug , Stefan Wahren wrote: >> 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. > It's true that RP1 can be called an MFD but the reason for not placing > it in mfd subsystem are twofold: > > - these discussions are quite clear about this matter: please see [1] > and [2] > - the current driver use no mfd API at all > > This RP1 driver is not currently addressing any aspect of ARM core in the > SoC so I would say it should not stay in drivers/soc / either, as also > condifirmed by [2] again and [3] (note that Microchip LAN966x is a very > close fit to what we have here on RP1). thanks i was aware of these discussions. A pointer to them or at least a short statement in the cover letter would be great. > >>> 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 dtsi solution you proposed is the one adopted downstream. It has its > benefits of course, but there's more. > 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 be > possibly be reused in other hw implementations. I believe a similar > reasoning could be applied to Bootlin's Microchip LAN966x patchset as > well, and they also choose to approach the dtb overlay. Could please add this point in the commit message. Doesn't introduce (maintainence) issues in case U-Boot needs a RP1 driver, too? > Plus, a solution that can (althoguh proabbly in teh long run) cope > with both DT or ACPI based system has been kindly requested, plase see [4] > for details. > IMHO the approach proposed from RH et al. of using dtbo for this 'special' > kind of drivers makes a lot of sense (see [5]). > >>> 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. > Could we instead just name them with generic GPIO'N' where N is the number > of the gpio? Much like many of that pins already are... in this way we > don't add a dependency in the board dts to the rp1_gpio node, which is not > even there when the main dts is parsed at boot, since the dtbo will be > added only on PCI enumeration of the RP1 device. I think we should avoid user space incompatibilities with the vendor tree. > Or even better: since we don't explicitly use the gpio names to address > them (e.g. phy-reset-gpios in rp1_eth node is addressing the ETH_RST_N > gpio by number), can we just get rid of the gpio-line-names property? > Also Bootlin's Microchip gpio node seems to avoid naming them... As i said above the gpio lines are for user space, honestly nobody likes to go to cryptic interfaces of gpiochips and gpio numbers. Maybe ETH_RST_N isn't good example because this not interesting from user space. For example RP1_STAT_LED is a better one. Nobody can predict the future use cases of the RP1 and its pins. So i think we should have the flexibilty to specify the GPIOs on the board level for user friendliness. Isn't it possible to specify almost empty rp1 node with the gpio line names for the RPi 5 and apply the rp1 overlay on top? > >>> + }; >>> + }; >>> + }; >>> + }; >>> +}; >>> 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 > My intention here is that, even if the code has been heavily modified by me, > the core original code is still there so I just wanted to tribute it to the > original author. > I'll synchronize this with RaspberryPi guys and coem up with a unified solution. That would be nice to mention in the commit message and add your copyright. > Just in case: would multiple MODULE_AUTHOR entries (one with my name and one > with original authors name) be accepetd? Sure Best regards > > Many thanks, > Andrea > > References: > > - [1]: https://lore.kernel.org/all/20240612140208.GC1504919@google.com/ > - [2]: https://lore.kernel.org/all/83f7fa09-d0e6-4f36-a27d-cee08979be2a@app.fastmail.com/ > - [3]: https://lore.kernel.org/all/2024081356-mutable-everyday-6f9d@gregkh/ > - [4]: https://lore.kernel.org/all/ba8cdf39-3ba3-4abc-98f5-d394d6867f95@gmx.net/ > - [5]: https://lpc.events/event/17/contributions/1421/attachments/1337/2680/LPC2023%20Non-discoverable%20devices%20in%20PCI.pdf
Hi Stefan, On 12:23 Fri 23 Aug , Stefan Wahren wrote: > Hi Andrea, > > Am 23.08.24 um 11:44 schrieb Andrea della Porta: > > Hi Stefan, > > > > On 18:20 Wed 21 Aug , Stefan Wahren wrote: > > > 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. > > It's true that RP1 can be called an MFD but the reason for not placing > > it in mfd subsystem are twofold: > > > > - these discussions are quite clear about this matter: please see [1] > > and [2] > > - the current driver use no mfd API at all > > > > This RP1 driver is not currently addressing any aspect of ARM core in the > > SoC so I would say it should not stay in drivers/soc / either, as also > > condifirmed by [2] again and [3] (note that Microchip LAN966x is a very > > close fit to what we have here on RP1). > thanks i was aware of these discussions. A pointer to them or at least a > short statement in the cover letter would be great. Sure, consider it done. > > > > > > 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 dtsi solution you proposed is the one adopted downstream. It has its > > benefits of course, but there's more. > > 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 be > > possibly be reused in other hw implementations. I believe a similar > > reasoning could be applied to Bootlin's Microchip LAN966x patchset as > > well, and they also choose to approach the dtb overlay. > Could please add this point in the commit message. Doesn't introduce Ack. > (maintainence) issues in case U-Boot needs a RP1 driver, too? Good point. Right now u-boot does not support RP1 nor PCIe (which is a prerequisite for RP1 to work) on Rpi5 and I'm quite sure that it will be so in the near future. Of course I cannot guarantee this will be the case far away in time. Since u-boot is lacking support for RP1 we cannot really produce some test results to check the compatibility versus kernel dtb overlay but we can speculate a little bit about it. AFAIK u-boot would probably place the rp1 node directly under its pcie@12000 node in DT while the dtb overlay will use dynamically created PCI endpoint node (dev@0) as parent for rp1 node. I would say it should work out of the box, the only minor drawback here should be the redundant rp1 node left from u-boot. And maybe some added checks to make sure the driver will be loaded only once from the dtb overlay and not from the u-boot node, but this change is locally to the RP1 linux driver code so it should not impact u-boot in any way. I'm inclined to consider the last one a minor issue. Please do keep in mind that, of course, this is just brainstorming and I cannot give 100% guarantee about that. > > Plus, a solution that can (althoguh proabbly in teh long run) cope > > with both DT or ACPI based system has been kindly requested, plase see [4] > > for details. > > IMHO the approach proposed from RH et al. of using dtbo for this 'special' > > kind of drivers makes a lot of sense (see [5]). > > > > > > 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. > > Could we instead just name them with generic GPIO'N' where N is the number > > of the gpio? Much like many of that pins already are... in this way we > > don't add a dependency in the board dts to the rp1_gpio node, which is not > > even there when the main dts is parsed at boot, since the dtbo will be > > added only on PCI enumeration of the RP1 device. > I think we should avoid user space incompatibilities with the vendor tree. > > Or even better: since we don't explicitly use the gpio names to address > > them (e.g. phy-reset-gpios in rp1_eth node is addressing the ETH_RST_N > > gpio by number), can we just get rid of the gpio-line-names property? > > Also Bootlin's Microchip gpio node seems to avoid naming them... > As i said above the gpio lines are for user space, honestly nobody likes > to go to cryptic interfaces of gpiochips and gpio numbers. > You're right. > Maybe ETH_RST_N isn't good example because this not interesting from > user space. For example RP1_STAT_LED is a better one. Nobody can predict > the future use cases of the RP1 and its pins. So i think we should have > the flexibilty to specify the GPIOs on the board level for user > friendliness. > Agreed. > Isn't it possible to specify almost empty rp1 node with the gpio line > names for the RPi 5 and apply the rp1 overlay on top? Uhm, we can think of something like that, i.e. a secondary dtbo (populated with the gpio-line-names property only) to be added after the PCI enumeration has added the primary dtbo (i.e. the proposed rp1.dtso with gpio-line-names dropped) into devicetree. This implies loading this second dtbo from either: - the RP1 driver itself, since it's the one that is dynamically adding the RP1 node. I would say this is not the cleanest way unless we provide an elegant way to fed the customized dtbo to teh driver itself, but has the advantage that it would surely work and has no side effects that come to mind. - late at boot, directly from userspace. I see 2 problems here: 1) the gpio driver is already probed by the first dtbo so we should have a way to just add teh gpio names to the alredy existing one. Not sure how to accomplish that right now. 2) not sure whether how to prepare the dtbo, it should probably have an empty target-path since the dt parent tree is created dynamically and can be different for each system. This could be also helpful to customize things like the phy, that is external to teh ethernet MAC. I need to do some investigation, but would be helpful if you or others have some preference/objection on the two approaches above. > > > > > > + }; > > > > + }; > > > > + }; > > > > + }; > > > > +}; > > > > 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 > > My intention here is that, even if the code has been heavily modified by me, > > the core original code is still there so I just wanted to tribute it to the > > original author. > > I'll synchronize this with RaspberryPi guys and coem up with a unified solution. > That would be nice to mention in the commit message and add your copyright. Ack. > > Just in case: would multiple MODULE_AUTHOR entries (one with my name and one > > with original authors name) be accepetd? > Sure Many thanks, Andrea > > Best regards > > > > Many thanks, > > Andrea > > > > References: > > > > - [1]: https://lore.kernel.org/all/20240612140208.GC1504919@google.com/ > > - [2]: https://lore.kernel.org/all/83f7fa09-d0e6-4f36-a27d-cee08979be2a@app.fastmail.com/ > > - [3]: https://lore.kernel.org/all/2024081356-mutable-everyday-6f9d@gregkh/ > > - [4]: https://lore.kernel.org/all/ba8cdf39-3ba3-4abc-98f5-d394d6867f95@gmx.net/ > > - [5]: https://lpc.events/event/17/contributions/1421/attachments/1337/2680/LPC2023%20Non-discoverable%20devices%20in%20PCI.pdf >
On Tue, Aug 20, 2024 at 04:36:10PM +0200, Andrea della Porta wrote: > --- a/include/linux/pci_ids.h > +++ b/include/linux/pci_ids.h > @@ -2610,6 +2610,9 @@ > #define PCI_VENDOR_ID_TEKRAM 0x1de1 > #define PCI_DEVICE_ID_TEKRAM_DC290 0xdc29 > > +#define PCI_VENDOR_ID_RPI 0x1de4 > +#define PCI_DEVICE_ID_RP1_C0 0x0001 Minor thing, but please read the top of this file. As you aren't using these values anywhere outside of this one driver, there's no need to add these values to pci_ids.h. Just keep them local to the .c file itself. thanks, greg k-h
Hi Greg, On 09:53 Sat 24 Aug , Greg Kroah-Hartman wrote: > On Tue, Aug 20, 2024 at 04:36:10PM +0200, Andrea della Porta wrote: > > --- a/include/linux/pci_ids.h > > +++ b/include/linux/pci_ids.h > > @@ -2610,6 +2610,9 @@ > > #define PCI_VENDOR_ID_TEKRAM 0x1de1 > > #define PCI_DEVICE_ID_TEKRAM_DC290 0xdc29 > > > > +#define PCI_VENDOR_ID_RPI 0x1de4 > > +#define PCI_DEVICE_ID_RP1_C0 0x0001 > > Minor thing, but please read the top of this file. As you aren't using > these values anywhere outside of this one driver, there's no need to add > these values to pci_ids.h. Just keep them local to the .c file itself. > Thanks, I've read the top part of that file. The reason I've declared those two macroes in pci_ids.h is that I'm using them both in the main driver (rp1-pci.c) and in drivers/pci/quirks.c. I suppose I could move DECLARE_PCI_FIXUP_FINAL() inside rp1-pci.c to keep those two defines local, but judging from the number of entries of DECLARE_PCI_FIXP_FINAL found in quirks.c versus the occurences found in respective driver, I assumed the preferred way was to place it in quirks.c. Many thanks, Andrea > thanks, > > greg k-h
On Mon, Aug 26, 2024 at 11:07:33AM +0200, Andrea della Porta wrote: > Hi Greg, > > On 09:53 Sat 24 Aug , Greg Kroah-Hartman wrote: > > On Tue, Aug 20, 2024 at 04:36:10PM +0200, Andrea della Porta wrote: > > > --- a/include/linux/pci_ids.h > > > +++ b/include/linux/pci_ids.h > > > @@ -2610,6 +2610,9 @@ > > > #define PCI_VENDOR_ID_TEKRAM 0x1de1 > > > #define PCI_DEVICE_ID_TEKRAM_DC290 0xdc29 > > > > > > +#define PCI_VENDOR_ID_RPI 0x1de4 > > > +#define PCI_DEVICE_ID_RP1_C0 0x0001 > > > > Minor thing, but please read the top of this file. As you aren't using > > these values anywhere outside of this one driver, there's no need to add > > these values to pci_ids.h. Just keep them local to the .c file itself. > > > > Thanks, I've read the top part of that file. The reason I've declared those > two macroes in pci_ids.h is that I'm using them both in the > main driver (rp1-pci.c) and in drivers/pci/quirks.c. Ah, missed that, sorry, nevermind. greg k-h
Hi Krzysztof, On 10:38 Wed 21 Aug , Krzysztof Kozlowski wrote: > On Tue, Aug 20, 2024 at 04:36:10PM +0200, Andrea della Porta wrote: > > The RaspberryPi RP1 is ia 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-borad peripherals > > by loading a devicetree overlay during driver probe. > > 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 ++++++++++++ > > Do not mix DTS with drivers. > > These MUST be separate. Separating the dtso from the driver in two different patches would mean that the dtso patch would be ordered before the driver one. This is because the driver embeds the dtbo binary blob inside itself, at build time. So in order to build the driver, the dtso needs to be there also. This is not the standard approach used with 'normal' dtb/dtbo, where the dtb patch is ordered last wrt the driver it refers to. Are you sure you want to proceed in this way? > > > 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 > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 67f460c36ea1..1359538b76e8 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -19119,9 +19119,11 @@ F: include/uapi/linux/media/raspberrypi/ > > 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/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/arch/arm64/boot/dts/broadcom/rp1.dtso b/arch/arm64/boot/dts/broadcom/rp1.dtso > > new file mode 100644 > > index 000000000000..d80178a278ee > > --- /dev/null > > +++ b/arch/arm64/boot/dts/broadcom/rp1.dtso > > @@ -0,0 +1,152 @@ > > +// 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/rp1.h> > > +#include <dt-bindings/misc/rp1.h> > > + > > +/dts-v1/; > > +/plugin/; > > + > > +/ { > > + fragment@0 { > > + target-path=""; > > + __overlay__ { > > + #address-cells = <3>; > > + #size-cells = <2>; > > + > > + rp1: rp1@0 { > > + compatible = "simple-bus"; > > + #address-cells = <2>; > > + #size-cells = <2>; > > + interrupt-controller; > > + interrupt-parent = <&rp1>; > > + #interrupt-cells = <2>; > > + > > + // ranges and dma-ranges must be provided by the includer > > + ranges = <0xc0 0x40000000 > > + 0x01/*0x02000000*/ 0x00 0x00000000 > > + 0x00 0x00400000>; > > Are you 100% sure you do not have here dtc W=1 warnings? the W=1 warnings are: arch/arm64/boot/dts/broadcom/rp1.dtso:37.24-42.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/clk_xosc: missing or empty reg/ranges property arch/arm64/boot/dts/broadcom/rp1.dtso:44.26-49.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/macb_pclk: missing or empty reg/ranges property arch/arm64/boot/dts/broadcom/rp1.dtso:51.26-56.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/macb_hclk: missing or empty reg/ranges property arch/arm64/boot/dts/broadcom/rp1.dtso:14.15-173.5: Warning (avoid_unnecessary_addr_size): /fragment@0/__overlay__: unnecessary #address-cells/#size-cells without "ranges", "dma-ranges" or child "reg" property I don't see anything related to the ranges line you mentioned. > > > + > > + dma-ranges = > > + // inbound RP1 1x_xxxxxxxx -> PCIe 1x_xxxxxxxx > > + <0x10 0x00000000 > > + 0x43000000 0x10 0x00000000 > > + 0x10 0x00000000>; > > + > > + clk_xosc: clk_xosc { > > Nope, switch to DTS coding style. Ack. > > > + compatible = "fixed-clock"; > > + #clock-cells = <0>; > > + clock-output-names = "xosc"; > > + clock-frequency = <50000000>; > > + }; > > + > > + macb_pclk: macb_pclk { > > + compatible = "fixed-clock"; > > + #clock-cells = <0>; > > + clock-output-names = "pclk"; > > + clock-frequency = <200000000>; > > + }; > > + > > + macb_hclk: macb_hclk { > > + compatible = "fixed-clock"; > > + #clock-cells = <0>; > > + clock-output-names = "hclk"; > > + clock-frequency = <200000000>; > > + }; > > + > > + rp1_clocks: clocks@c040018000 { > > Why do you mix MMIO with non-MMIO nodes? This really does not look > correct. > Right. This is already under discussion here: https://lore.kernel.org/all/ZtBzis5CzQMm8loh@apocalypse/ IIUC you proposed to instantiate the non-MMIO nodes (the three clocks) by using CLK_OF_DECLARE. > > + compatible = "raspberrypi,rp1-clocks"; > > + #clock-cells = <1>; > > + reg = <0xc0 0x40018000 0x0 0x10038>; > > Wrong order of properties - see DTS coding style. Ack. Many thanks, Andrea > > > + clocks = <&clk_xosc>; > > + clock-names = "xosc"; > > Best regards, > Krzysztof >
On Fri, Aug 30, 2024 at 03:49:04PM +0200, Andrea della Porta wrote: > Hi Krzysztof, > > On 10:38 Wed 21 Aug , Krzysztof Kozlowski wrote: > > On Tue, Aug 20, 2024 at 04:36:10PM +0200, Andrea della Porta wrote: > > > The RaspberryPi RP1 is ia 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-borad peripherals > > > by loading a devicetree overlay during driver probe. > > > 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 ++++++++++++ > > > > Do not mix DTS with drivers. > > > > These MUST be separate. > > Separating the dtso from the driver in two different patches would mean > that the dtso patch would be ordered before the driver one. This is because > the driver embeds the dtbo binary blob inside itself, at build time. So > in order to build the driver, the dtso needs to be there also. This is not > the standard approach used with 'normal' dtb/dtbo, where the dtb patch is > ordered last wrt the driver it refers to. > Are you sure you want to proceed in this way? It is more about they are logically separate things. The .dtb/dtbo describes the hardware. It should be possible to review that as a standalone thing. The code them implements the binding. It makes no sense to review the code until the binding is correct, because changes to the binding will need changes to the code. Hence, we want the binding first, then the code which implements it. Andrew
On 30/08/2024 15:49, Andrea della Porta wrote: > Hi Krzysztof, > > On 10:38 Wed 21 Aug , Krzysztof Kozlowski wrote: >> On Tue, Aug 20, 2024 at 04:36:10PM +0200, Andrea della Porta wrote: >>> The RaspberryPi RP1 is ia 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-borad peripherals >>> by loading a devicetree overlay during driver probe. >>> 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 ++++++++++++ >> >> Do not mix DTS with drivers. >> >> These MUST be separate. > > Separating the dtso from the driver in two different patches would mean > that the dtso patch would be ordered before the driver one. This is because > the driver embeds the dtbo binary blob inside itself, at build time. So > in order to build the driver, the dtso needs to be there also. This is not Sure, in such case DTS will have to go through the same tree as driver as an exception. Please document it in patch changelog (---). > the standard approach used with 'normal' dtb/dtbo, where the dtb patch is > ordered last wrt the driver it refers to. It's not exactly the "ordered last" that matters, but lack of dependency and going through separate tree and branch - arm-soc/dts. Here there will be an exception how we handle patch, but still DTS is hardware description so should not be combined with driver code. > Are you sure you want to proceed in this way? > >> >>> 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 >>> >>> diff --git a/MAINTAINERS b/MAINTAINERS >>> index 67f460c36ea1..1359538b76e8 100644 >>> --- a/MAINTAINERS >>> +++ b/MAINTAINERS >>> @@ -19119,9 +19119,11 @@ F: include/uapi/linux/media/raspberrypi/ >>> 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/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/arch/arm64/boot/dts/broadcom/rp1.dtso b/arch/arm64/boot/dts/broadcom/rp1.dtso >>> new file mode 100644 >>> index 000000000000..d80178a278ee >>> --- /dev/null >>> +++ b/arch/arm64/boot/dts/broadcom/rp1.dtso >>> @@ -0,0 +1,152 @@ >>> +// 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/rp1.h> >>> +#include <dt-bindings/misc/rp1.h> >>> + >>> +/dts-v1/; >>> +/plugin/; >>> + >>> +/ { >>> + fragment@0 { >>> + target-path=""; >>> + __overlay__ { >>> + #address-cells = <3>; >>> + #size-cells = <2>; >>> + >>> + rp1: rp1@0 { >>> + compatible = "simple-bus"; >>> + #address-cells = <2>; >>> + #size-cells = <2>; >>> + interrupt-controller; >>> + interrupt-parent = <&rp1>; >>> + #interrupt-cells = <2>; >>> + >>> + // ranges and dma-ranges must be provided by the includer >>> + ranges = <0xc0 0x40000000 >>> + 0x01/*0x02000000*/ 0x00 0x00000000 >>> + 0x00 0x00400000>; >> >> Are you 100% sure you do not have here dtc W=1 warnings? > > the W=1 warnings are: > > arch/arm64/boot/dts/broadcom/rp1.dtso:37.24-42.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/clk_xosc: missing or empty reg/ranges property > arch/arm64/boot/dts/broadcom/rp1.dtso:44.26-49.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/macb_pclk: missing or empty reg/ranges property > arch/arm64/boot/dts/broadcom/rp1.dtso:51.26-56.7: Warning (simple_bus_reg): /fragment@0/__overlay__/rp1@0/macb_hclk: missing or empty reg/ranges property > arch/arm64/boot/dts/broadcom/rp1.dtso:14.15-173.5: Warning (avoid_unnecessary_addr_size): /fragment@0/__overlay__: unnecessary #address-cells/#size-cells without "ranges", "dma-ranges" or child "reg" property > > I don't see anything related to the ranges line you mentioned. Hm, indeed, but I would expect warning about unit address not matching ranges/reg. > >> >>> + >>> + dma-ranges = >>> + // inbound RP1 1x_xxxxxxxx -> PCIe 1x_xxxxxxxx >>> + <0x10 0x00000000 >>> + 0x43000000 0x10 0x00000000 >>> + 0x10 0x00000000>; >>> + >>> + clk_xosc: clk_xosc { >> >> Nope, switch to DTS coding style. > > Ack. > >> >>> + compatible = "fixed-clock"; >>> + #clock-cells = <0>; >>> + clock-output-names = "xosc"; >>> + clock-frequency = <50000000>; >>> + }; >>> + >>> + macb_pclk: macb_pclk { >>> + compatible = "fixed-clock"; >>> + #clock-cells = <0>; >>> + clock-output-names = "pclk"; >>> + clock-frequency = <200000000>; >>> + }; >>> + >>> + macb_hclk: macb_hclk { >>> + compatible = "fixed-clock"; >>> + #clock-cells = <0>; >>> + clock-output-names = "hclk"; >>> + clock-frequency = <200000000>; >>> + }; >>> + >>> + rp1_clocks: clocks@c040018000 { >> >> Why do you mix MMIO with non-MMIO nodes? This really does not look >> correct. >> > > Right. This is already under discussion here: > https://lore.kernel.org/all/ZtBzis5CzQMm8loh@apocalypse/ > > IIUC you proposed to instantiate the non-MMIO nodes (the three clocks) by > using CLK_OF_DECLARE. Depends. Where are these clocks? Naming suggests they might not be even part of this device. But if these are part of the device, then why this is not a clock controller (if they are controllable) or even removed (because we do not represent internal clock tree in DTS). Best regards, Krzysztof
On Fri, Aug 23, 2024 at 11:31 AM Andrea della Porta <andrea.porta@suse.com> wrote: > > Hi Stefan, > > On 12:23 Fri 23 Aug , Stefan Wahren wrote: > > Hi Andrea, > > > > Am 23.08.24 um 11:44 schrieb Andrea della Porta: > > > Hi Stefan, > > > > > > On 18:20 Wed 21 Aug , Stefan Wahren wrote: > > > > 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. > > > It's true that RP1 can be called an MFD but the reason for not placing > > > it in mfd subsystem are twofold: > > > > > > - these discussions are quite clear about this matter: please see [1] > > > and [2] > > > - the current driver use no mfd API at all > > > > > > This RP1 driver is not currently addressing any aspect of ARM core in the > > > SoC so I would say it should not stay in drivers/soc / either, as also > > > condifirmed by [2] again and [3] (note that Microchip LAN966x is a very > > > close fit to what we have here on RP1). > > thanks i was aware of these discussions. A pointer to them or at least a > > short statement in the cover letter would be great. > > Sure, consider it done. > > > > > > > > > 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 dtsi solution you proposed is the one adopted downstream. It has its > > > benefits of course, but there's more. > > > 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 be > > > possibly be reused in other hw implementations. I believe a similar > > > reasoning could be applied to Bootlin's Microchip LAN966x patchset as > > > well, and they also choose to approach the dtb overlay. > > Could please add this point in the commit message. Doesn't introduce > > Ack. > > > (maintainence) issues in case U-Boot needs a RP1 driver, too? > > Good point. Right now u-boot does not support RP1 nor PCIe (which is a > prerequisite for RP1 to work) on Rpi5 and I'm quite sure that it will be > so in the near future. Of course I cannot guarantee this will be the case > far away in time. > > Since u-boot is lacking support for RP1 we cannot really produce some test > results to check the compatibility versus kernel dtb overlay but we can > speculate a little bit about it. AFAIK u-boot would probably place the rp1 > node directly under its pcie@12000 node in DT while the dtb overlay will use > dynamically created PCI endpoint node (dev@0) as parent for rp1 node. u-boot could do that and it would not be following the 25+ year old PCI bus bindings. Some things may be argued about as "Linux bindings", but that isn't one of them. Rob
Hi Rob, On 13:27 Fri 30 Aug , Rob Herring wrote: > On Fri, Aug 23, 2024 at 11:31 AM Andrea della Porta > <andrea.porta@suse.com> wrote: > > ... > > > > Since u-boot is lacking support for RP1 we cannot really produce some test > > results to check the compatibility versus kernel dtb overlay but we can > > speculate a little bit about it. AFAIK u-boot would probably place the rp1 > > node directly under its pcie@12000 node in DT while the dtb overlay will use > > dynamically created PCI endpoint node (dev@0) as parent for rp1 node. > > u-boot could do that and it would not be following the 25+ year old > PCI bus bindings. Some things may be argued about as "Linux bindings", > but that isn't one of them. Indeed. It was just speculation, not something I would bet on. Regards, Andrea > > Rob
On 03/09/2024 17:15, Andrea della Porta wrote: >>>>> + >>>>> + rp1_clocks: clocks@c040018000 { >>>> >>>> Why do you mix MMIO with non-MMIO nodes? This really does not look >>>> correct. >>>> >>> >>> Right. This is already under discussion here: >>> https://lore.kernel.org/all/ZtBzis5CzQMm8loh@apocalypse/ >>> >>> IIUC you proposed to instantiate the non-MMIO nodes (the three clocks) by >>> using CLK_OF_DECLARE. >> >> Depends. Where are these clocks? Naming suggests they might not be even >> part of this device. But if these are part of the device, then why this >> is not a clock controller (if they are controllable) or even removed >> (because we do not represent internal clock tree in DTS). > > xosc is a crystal connected to the oscillator input of the RP1, so I would > consider it an external fixed-clock. If we were in the entire dts, I would have > put it in root under /clocks node, but here we're in the dtbo so I'm not sure > where else should I put it. But physically, on which PCB, where is this clock located? > > Regarding pclk and hclk, I'm still trying to understand where they come from. > If they are external clocks (since they are fixed-clock too), they should be > in the same node as xosc. CLK_OF_DECLARE does not seem to fit here because There is no such node as "/clocks" so do not focus on that. That's just placeholder but useless and it is inconsistent with other cases (e.g. regulators). If this is external oscillator then it is not part of RP1 and you cannot put it inside just to satisfy your drivers. > there's no special management of these clocks, so no new clock definition is > needed. > If they are internal tree, I cannot simply get rid of them because rp1_eth node > references these two clocks (see clocks property), so they must be decalred > somewhere. Any hint about this?. > Describe the hardware. Show the diagram or schematics where is which device. Best regards, Krzysztof
Hi Krzysztof, On 18:52 Thu 05 Sep , Krzysztof Kozlowski wrote: > On 05/09/2024 18:33, Andrea della Porta wrote: > > Hi Krzysztof, > > > > On 20:27 Tue 03 Sep , Krzysztof Kozlowski wrote: > >> On 03/09/2024 17:15, Andrea della Porta wrote: > >>>>>>> + > >>>>>>> + rp1_clocks: clocks@c040018000 { > >>>>>> > >>>>>> Why do you mix MMIO with non-MMIO nodes? This really does not look > >>>>>> correct. > >>>>>> > >>>>> > >>>>> Right. This is already under discussion here: > >>>>> https://lore.kernel.org/all/ZtBzis5CzQMm8loh@apocalypse/ > >>>>> > >>>>> IIUC you proposed to instantiate the non-MMIO nodes (the three clocks) by > >>>>> using CLK_OF_DECLARE. > >>>> > >>>> Depends. Where are these clocks? Naming suggests they might not be even > >>>> part of this device. But if these are part of the device, then why this > >>>> is not a clock controller (if they are controllable) or even removed > >>>> (because we do not represent internal clock tree in DTS). > >>> > >>> xosc is a crystal connected to the oscillator input of the RP1, so I would > >>> consider it an external fixed-clock. If we were in the entire dts, I would have > >>> put it in root under /clocks node, but here we're in the dtbo so I'm not sure > >>> where else should I put it. > >> > >> But physically, on which PCB, where is this clock located? > > > > xosc is a crystal, feeding the reference clock oscillator input pins of the RP1, > > please see page 12 of the following document: > > https://datasheets.raspberrypi.com/rp1/rp1-peripherals.pdf > > That's not the answer. Where is it physically located? Please see below. > > > On Rpi5, the PCB is the very same as the one on which the BCM2712 (SoC) and RP1 > > are soldered. Would you consider it external (since the crystal is outside the RP1) > > or internal (since the oscillator feeded by the crystal is inside the RP1)? > > So it is on RPi 5 board? Just like every other SoC and every other > vendor? Then just like every other SoC and every other vendor it is in > board DTS file. Yes it's on the Rpi5 board. These are two separate thing, though: one is where to put it (DTS, DTSO) and another is in what target path relative to root. I was trying to understand the latter. The clock node should be put in the DTBO since we are loading this driver at runtime and we probably don't want to depend on some specific node name to be present in the DTS. This is also true because this driver should possibly work also on ACPI system and on hypothetical PCI card on which the RP1 could be mounted in the future, and in that case a DTS could be not even there. After all, those clocks must be in the immediate proximity to the RP1, and on the same board, which may or may not be the main board as the Rpi5 case. I think that, since this application is a little bit peculiar, maybe some compromises could be legit. > > > > >> > >>> > >>> Regarding pclk and hclk, I'm still trying to understand where they come from. > >>> If they are external clocks (since they are fixed-clock too), they should be > >>> in the same node as xosc. CLK_OF_DECLARE does not seem to fit here because > >> > >> There is no such node as "/clocks" so do not focus on that. That's just > >> placeholder but useless and it is inconsistent with other cases (e.g. > >> regulators). > > > > Fine, I beleve that the root node would be okay then, or some other carefully named > > node in root, if the clock is not internal to any chip. > > > >> > >> If this is external oscillator then it is not part of RP1 and you cannot > >> put it inside just to satisfy your drivers. > > > > Ack. > > > >> > >>> there's no special management of these clocks, so no new clock definition is > >>> needed. > >> > >>> If they are internal tree, I cannot simply get rid of them because rp1_eth node > >>> references these two clocks (see clocks property), so they must be decalred > >>> somewhere. Any hint about this?. > >>> > >> > >> Describe the hardware. Show the diagram or schematics where is which device. > > > > Unfortunately I don't have the documentation (schematics or other info) about > > how these two clocks (pclk and hclk) are arranged, but I'm trying to get > > some insight about that from various sources. While we're waiting for some > > (hopefully) more certain info, I'd like to speculate a bit. I would say that > > they both probably be either external (just like xosc), or generated internally > > to the RP1: > > > > If externals, I would place them in the same position as xosc, so root node > > or some other node under root (eg.: /rp1-clocks) > > Just like /clocks, /rp1-clocks is not better. Neither /rp1-foo-clocks. Right. So in this case, since xosc seems to be on the same level and on the same board of the RP1 and the SoC, and it's also external to the RP1, can I assume that placing xosc node in root is ok? > > I think there is some sort of big misunderstanding here. Is this RP1 > co-processor on the RP board, connected over PCI to Broadcom SoC? Yes. ---------------Rpi5 board--------------------- | | | SoC ==pci bus==> RP1 <== xosc crystal | | | ---------------------------------------------- > > > > > If internals, I would leave them just where they are, i.e. inside the rp1 node > > > > Does it make sense? > > No, because you do not have xosc there, according to my knowledge. Hmmm sorry, not sure what this negation was referring to... I was talking about hclk and pclk, not xosc here. Could you please add some details? Many thanks, Andrea > > Best regards, > Krzysztof >
On 05/09/2024 20:54, Andrea della Porta wrote: > Hi Krzysztof, > > On 18:52 Thu 05 Sep , Krzysztof Kozlowski wrote: >> On 05/09/2024 18:33, Andrea della Porta wrote: >>> Hi Krzysztof, >>> >>> On 20:27 Tue 03 Sep , Krzysztof Kozlowski wrote: >>>> On 03/09/2024 17:15, Andrea della Porta wrote: >>>>>>>>> + >>>>>>>>> + rp1_clocks: clocks@c040018000 { >>>>>>>> >>>>>>>> Why do you mix MMIO with non-MMIO nodes? This really does not look >>>>>>>> correct. >>>>>>>> >>>>>>> >>>>>>> Right. This is already under discussion here: >>>>>>> https://lore.kernel.org/all/ZtBzis5CzQMm8loh@apocalypse/ >>>>>>> >>>>>>> IIUC you proposed to instantiate the non-MMIO nodes (the three clocks) by >>>>>>> using CLK_OF_DECLARE. >>>>>> >>>>>> Depends. Where are these clocks? Naming suggests they might not be even >>>>>> part of this device. But if these are part of the device, then why this >>>>>> is not a clock controller (if they are controllable) or even removed >>>>>> (because we do not represent internal clock tree in DTS). >>>>> >>>>> xosc is a crystal connected to the oscillator input of the RP1, so I would >>>>> consider it an external fixed-clock. If we were in the entire dts, I would have >>>>> put it in root under /clocks node, but here we're in the dtbo so I'm not sure >>>>> where else should I put it. >>>> >>>> But physically, on which PCB, where is this clock located? >>> >>> xosc is a crystal, feeding the reference clock oscillator input pins of the RP1, >>> please see page 12 of the following document: >>> https://datasheets.raspberrypi.com/rp1/rp1-peripherals.pdf >> >> That's not the answer. Where is it physically located? > > Please see below. > >> >>> On Rpi5, the PCB is the very same as the one on which the BCM2712 (SoC) and RP1 >>> are soldered. Would you consider it external (since the crystal is outside the RP1) >>> or internal (since the oscillator feeded by the crystal is inside the RP1)? >> >> So it is on RPi 5 board? Just like every other SoC and every other >> vendor? Then just like every other SoC and every other vendor it is in >> board DTS file. > > Yes it's on the Rpi5 board. These are two separate thing, though: one is where Finally. > to put it (DTS, DTSO) and another is in what target path relative to root. I > was trying to understand the latter. It is already or should be part of DTS, not DTSO. You are duplicating device nodes. > The clock node should be put in the DTBO since we are loading this driver at > runtime and we probably don't want to depend on some specific node name to be > present in the DTS. This is also true because this driver should possibly work > also on ACPI system and on hypothetical PCI card on which the RP1 could be mounted Not really. ACPI and thus DT in such case will take it as clock input. Basically what you need here is to provide clock inputs to this device. It's then firmware independent. > in the future, and in that case a DTS could be not even there. No problem. Whatever firmware mechanism you have, it will provide you clock. > After all, those clocks must be in the immediate proximity to the RP1, and on the > same board, which may or may not be the main board as the Rpi5 case. > I think that, since this application is a little bit peculiar, maybe some > compromises could be legit. Application is not peculiar but completely standard. You have standard PCI device which has some inputs. One of these inputs, maybe on some reserved M.2 pins or whatver connector you have there, is the clock. ... >>> >>> If externals, I would place them in the same position as xosc, so root node >>> or some other node under root (eg.: /rp1-clocks) >> >> Just like /clocks, /rp1-clocks is not better. Neither /rp1-foo-clocks. > > Right. So in this case, since xosc seems to be on the same level and on the same > board of the RP1 and the SoC, and it's also external to the RP1, can I assume that > placing xosc node in root is ok? root node of the DTS yes. Root node of DTSO of course not, because it is not part of DTSO and you are duplicating DTS. It would not even work. That's why you need to apply the overlay to proper target as I asked long time ago. > >> >> I think there is some sort of big misunderstanding here. Is this RP1 >> co-processor on the RP board, connected over PCI to Broadcom SoC? > > Yes. > > ---------------Rpi5 board--------------------- > | | > | SoC ==pci bus==> RP1 <== xosc crystal | > | | > ---------------------------------------------- > >> >>> >>> If internals, I would leave them just where they are, i.e. inside the rp1 node >>> >>> Does it make sense? >> >> No, because you do not have xosc there, according to my knowledge. > > Hmmm sorry, not sure what this negation was referring to... I was talking about > hclk and pclk, not xosc here. Could you please add some details? If considering hclk and pclk, then depends where are they. If they come as inputs, then same as xosc. If they are not, then it is also obvious - we do not represent internal device clocks as fixed clocks in DTS, because it makes absolutely no sense at all. No benefits, no help, nothing at all. It's just device's internal clock. This is again nothing peculiar. Many other devices have some internal stuff. Do we add fixed clocks for these? Fixed regulators? No, of course not. Best regards, Krzysztof
diff --git a/MAINTAINERS b/MAINTAINERS index 67f460c36ea1..1359538b76e8 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -19119,9 +19119,11 @@ F: include/uapi/linux/media/raspberrypi/ 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/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/arch/arm64/boot/dts/broadcom/rp1.dtso b/arch/arm64/boot/dts/broadcom/rp1.dtso new file mode 100644 index 000000000000..d80178a278ee --- /dev/null +++ b/arch/arm64/boot/dts/broadcom/rp1.dtso @@ -0,0 +1,152 @@ +// 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/rp1.h> +#include <dt-bindings/misc/rp1.h> + +/dts-v1/; +/plugin/; + +/ { + fragment@0 { + target-path=""; + __overlay__ { + #address-cells = <3>; + #size-cells = <2>; + + rp1: rp1@0 { + compatible = "simple-bus"; + #address-cells = <2>; + #size-cells = <2>; + interrupt-controller; + interrupt-parent = <&rp1>; + #interrupt-cells = <2>; + + // ranges and dma-ranges must be provided by the includer + ranges = <0xc0 0x40000000 + 0x01/*0x02000000*/ 0x00 0x00000000 + 0x00 0x00400000>; + + dma-ranges = + // inbound RP1 1x_xxxxxxxx -> PCIe 1x_xxxxxxxx + <0x10 0x00000000 + 0x43000000 0x10 0x00000000 + 0x10 0x00000000>; + + clk_xosc: clk_xosc { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-output-names = "xosc"; + clock-frequency = <50000000>; + }; + + macb_pclk: macb_pclk { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-output-names = "pclk"; + clock-frequency = <200000000>; + }; + + macb_hclk: macb_hclk { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-output-names = "hclk"; + clock-frequency = <200000000>; + }; + + 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 + }; + }; + }; + }; +}; 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 void dump_bar(struct pci_dev *pdev, unsigned int bar) +{ + dev_info(&pdev->dev, + "bar%d len 0x%llx, start 0x%llx, end 0x%llx, flags, 0x%lx\n", + bar, + pci_resource_len(pdev, bar), + pci_resource_start(pdev, bar), + pci_resource_end(pdev, bar), + pci_resource_flags(pdev, bar)); +} + +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; + int ret = 0; + + switch (type) { + case IRQ_TYPE_LEVEL_HIGH: + dev_dbg(rp1->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: + ret = -EINVAL; + break; + } + + return ret; +} + +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); + 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) { + 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 ret; +} + +static int rp1_irq_activate(struct irq_domain *d, struct irq_data *irqd, + bool reserve) +{ + struct rp1_dev *rp1 = d->host_data; + struct irq_data *pcie_irqd; + + pcie_irqd = rp1->pcie_irqds[irqd->hwirq]; + msix_cfg_set(rp1, (unsigned int)irqd->hwirq, MSIX_CFG_ENABLE); + + return irq_domain_activate_irq(pcie_irqd, reserve); +} + +static void rp1_irq_deactivate(struct irq_domain *d, struct irq_data *irqd) +{ + struct rp1_dev *rp1 = d->host_data; + struct irq_data *pcie_irqd; + + pcie_irqd = rp1->pcie_irqds[irqd->hwirq]; + msix_cfg_clr(rp1, (unsigned int)irqd->hwirq, MSIX_CFG_ENABLE); + + return irq_domain_deactivate_irq(pcie_irqd); +} + +static const struct irq_domain_ops rp1_domain_ops = { + .xlate = rp1_irq_xlate, + .activate = rp1_irq_activate, + .deactivate = rp1_irq_deactivate, +}; + +static int rp1_probe(struct pci_dev *pdev, const struct pci_device_id *id) +{ + struct device *dev = &pdev->dev; + struct device_node *rp1_node; + struct reset_control *reset; + 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; + rp1->dev = &pdev->dev; + + reset = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL); + if (IS_ERR(reset)) + return PTR_ERR(reset); + reset_control_reset(reset); + + dump_bar(pdev, 0); + dump_bar(pdev, 1); + + 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) { + dev_err(&pdev->dev, "Enabling PCI device has failed: %d", + err); + return err; + } + + rp1->bar1 = pci_iomap(pdev, 1, 0); + if (!rp1->bar1) { + dev_err(&pdev->dev, "Cannot map PCI bar\n"); + return -EIO; + } + + u32 dtbo_size = __dtbo_rp1_pci_end - __dtbo_rp1_pci_begin; + void *dtbo_start = __dtbo_rp1_pci_begin; + + err = of_overlay_fdt_apply(dtbo_start, dtbo_size, &rp1->ovcs_id, rp1_node); + if (err) + goto err_unmap_bar; + + pci_set_master(pdev); + + err = pci_alloc_irq_vectors(pdev, RP1_IRQS, RP1_IRQS, + PCI_IRQ_MSIX); + if (err != RP1_IRQS) { + dev_err(&pdev->dev, "pci_alloc_irq_vectors failed - %d\n", err); + goto err_unload_overlay; + } + + pci_set_drvdata(pdev, rp1); + rp1->domain = irq_domain_add_linear(of_find_node_by_name(NULL, "rp1"), RP1_IRQS, + &rp1_domain_ops, rp1); + + for (i = 0; i < RP1_IRQS; i++) { + int irq = irq_create_mapping(rp1->domain, i); + + if (irq < 0) { + dev_err(&pdev->dev, "failed to create irq mapping\n"); + err = irq; + goto err_unload_overlay; + } + 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_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_unmap_bar: + pci_iounmap(pdev, rp1->bar1); + + 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); + pci_iounmap(pdev, rp1->bar1); + of_overlay_remove(&rp1->ovcs_id); + + clk_unregister(rp1->sys_clk); +} + +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_DESCRIPTION("RP1 wrapper"); +MODULE_LICENSE("GPL"); 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/pci/quirks.c b/drivers/pci/quirks.c index a2ce4e08edf5..8c2e6238535e 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -6245,6 +6245,7 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0xa76e, dpc_log_size); 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_RPI, PCI_DEVICE_ID_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 e388c8b1cbc2..2120f2895bb8 100644 --- a/include/linux/pci_ids.h +++ b/include/linux/pci_ids.h @@ -2610,6 +2610,9 @@ #define PCI_VENDOR_ID_TEKRAM 0x1de1 #define PCI_DEVICE_ID_TEKRAM_DC290 0xdc29 +#define PCI_VENDOR_ID_RPI 0x1de4 +#define PCI_DEVICE_ID_RP1_C0 0x0001 + #define PCI_VENDOR_ID_ALIBABA 0x1ded #define PCI_VENDOR_ID_CXL 0x1e98
The RaspberryPi RP1 is ia 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-borad peripherals by loading a devicetree overlay during driver probe. 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