diff mbox series

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

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

Commit Message

Andrea della Porta Aug. 20, 2024, 2:36 p.m. UTC
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

Comments

Krzysztof Kozlowski Aug. 21, 2024, 8:38 a.m. UTC | #1
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
kernel test robot Aug. 21, 2024, 1:07 p.m. UTC | #2
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]
kernel test robot Aug. 21, 2024, 1:49 p.m. UTC | #3
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
Krzysztof Kozlowski Aug. 21, 2024, 2:20 p.m. UTC | #4
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
Stefan Wahren Aug. 21, 2024, 4:20 p.m. UTC | #5
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
Bjorn Helgaas Aug. 21, 2024, 4:55 p.m. UTC | #6
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.
kernel test robot Aug. 21, 2024, 5:56 p.m. UTC | #7
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
Andrea della Porta Aug. 22, 2024, 2:33 p.m. UTC | #8
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
>
Krzysztof Kozlowski Aug. 22, 2024, 2:46 p.m. UTC | #9
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
Andrea della Porta Aug. 23, 2024, 9:44 a.m. UTC | #10
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
Andrea della Porta Aug. 23, 2024, 10:21 a.m. UTC | #11
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
Stefan Wahren Aug. 23, 2024, 10:23 a.m. UTC | #12
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
Andrea della Porta Aug. 23, 2024, 4:31 p.m. UTC | #13
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
>
Greg Kroah-Hartman Aug. 24, 2024, 1:53 a.m. UTC | #14
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
Andrea della Porta Aug. 26, 2024, 9:07 a.m. UTC | #15
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
Greg Kroah-Hartman Aug. 26, 2024, 9:18 a.m. UTC | #16
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
Andrea della Porta Aug. 30, 2024, 1:49 p.m. UTC | #17
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
>
Andrew Lunn Aug. 30, 2024, 2:21 p.m. UTC | #18
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
Krzysztof Kozlowski Aug. 30, 2024, 4:52 p.m. UTC | #19
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
Rob Herring (Arm) Aug. 30, 2024, 6:27 p.m. UTC | #20
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
Andrea della Porta Sept. 2, 2024, 9:34 a.m. UTC | #21
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
Krzysztof Kozlowski Sept. 3, 2024, 6:27 p.m. UTC | #22
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
Andrea della Porta Sept. 5, 2024, 6:54 p.m. UTC | #23
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
>
Krzysztof Kozlowski Sept. 5, 2024, 9:20 p.m. UTC | #24
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 mbox series

Patch

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