Message-ID: <ZmhvqwnOIdpi7EhA@apocalypse>
Date: Tue, 11 Jun 2024 17:39:23 +0200
From: Andrea della Porta <andrea.porta@...e.com>
To: Lee Jones <lee@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Florian Fainelli <florian.fainelli@...adcom.com>,
	Broadcom internal kernel review list <bcm-kernel-feedback-list@...adcom.com>,
	Stefan Wahren <wahrenst@....net>, devicetree@...r.kernel.org,
	linux-rpi-kernel@...ts.infradead.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Bjorn Helgaas <bhelgaas@...gle.com>, linux-pci@...r.kernel.org,
	Dave Ertman <david.m.ertman@...el.com>,
	Lizhi Hou <lizhi.hou@....com>, clement.leger@...tlin.com
Subject: Raspberry Pi5 - RP1 driver - RFC

Hi,
I'm on the verge of reworking the RP1 driver from downstream in order for it to be
in good shape for upstream inclusion.
RP1 is an MFD chipset that acts as a south-bridge PCIe endpoint sporting a pletora
of subdevices (i.e.  Ethernet, USB host controller, I2C, PWM, etc.) whose registers
are all reachable starting from an offset from the BAR address.
The main point here is that while the RP1 as an endpoint itself is discoverable via
usual PCI enumeraiton, the devices it contains are not discoverable and must be
declared e.g. via the devicetree. This is an RFC about the correct approach to use
in integrating the driver and registering the subdevices.


--- CURRENT DOWNSTREAM APPROACH ---

The DTS shows something like this (see [1] and [2]):

pcie {
	compatible = "brcm,bcm2712-pcie";
	#address-cells = <0x03>;
	#size-cells = <0x02>;
	ranges = <0x2000000 0x00 0x00   0x1f 0x00   0x00 0xfffffffc>;
	...

	rp1 {
		compatible = "simple-bus";
		#address-cells = <0x02>;
               	#size-cells = <0x02>;

		ranges = <0xc0 0x40000000   0x2000000 0x00 0x00   0x00 0x400000>;
		...

		serial@...00 {
			compatible = "arm,pl011-axi";
			reg = <0xc0 0x40034000   0x00 0x100>;
			...
		};
	};
};

The PCI bar address here is at CPU physical address 0x1f00000000 and the RP1 driver
probe function calls of_platform_populate() on the 'rp1' node to register the platform
drivers for each subdevices (e.g. in the above example: 'serial@...00').

Pros:
- quite straightforward to implement
- RP1's dts resides in the directory it should (possibly but not necessarily) belong to,
  e.g. somewhere under arch/*/boot/dts/...

Cons:
- the board dts must manually override 'pcie' ranges (in this case 0x1f00000000)
  depending on the BAR address value, while it should be retrieved by reading the PCI
  config register instead
- the probe() function retieves a reference to 'rp1' node via of_find_node_by_name(NULL, 
  "rp1"), harcoding the node name. This is not desirable since the node name is then set
  in stone, or otherwise if the node name needs to be changed it must be changed either
  in the dts *and* in driver code.


--- ROB HERRING, LIZHI HOU (et al.) PROPOSED APPROACH ---

A proposal (see [3]) presented at  LPC advise to create a PCI bridge DT node ('pcie') leveraging
the current OF dynamic infrastructure, adding a dtb overlay ('rpi1' node in the example
above) on top of it during probe and rearranging the 'ranges' mapping dynamically.
This sounds like the correct approach and is somewhat used in at least a couple of drivers
(namely for Alveo U50 card and MicroChip LAN9662 SoC) but AFAIK none of them made their way
to mainline, leaving some doubts about the applicability of this paradigm.

Pros:
- no need to provide the BAR address manually since it will be discovered and automatically
  amended into the 'ranges' property
- no harcoded reference to 'rp1' node since the endpoint node will be reparented to the 'pcie'
  node automatically
- the core OF dynamic infrastructure on which to base these changes are basically already in 
  mainline (see [4] for discussions)

Cons (albeit I'd consider them minor ones):
- CONFIG_PCI_DYNAMIC_OF_NODES must be enabled
- the dtb should probably reside somewhere near the driver source code, e.g. in drivers/mfd/...


--- AUXILIARY BUS APPROACH ---

The thread in [5] seems to advise to use the auxiliary bus for this kind of devices. In this
case, here's the drawbacks:

- as stated in kernel docs for aux bus (cit.) "need a mechanism to connect and provide access
  to a shared object allocated by the auxiliary_device’s registering driver...", and again 
  (cit.) "A key requirement for utilizing the auxiliary bus is that there is no dependency on
  a physical bus...These individual devices split from the core cannot live on the platform
  bus as they are not physical devices that are controlled by DT/ACPI". Those statements are
  of course at the opposite of how RP1 behaves
- subdevices drivers may need rework in order to cope with the auxiliary bus, while we need to
  use the already existing drivers without modifications

so, for all of the above (and probably other cons I'm not aware of right now), the auxiliary bus
does not seems feasible to be used, but I'm mentioning it just because of the discussionin in [5]
that let me wonder whether I may be missing something relevant here.


CONCLUSIONS

All in all, I'd say Rob's approach should be the way to go, any thoughts about it will be
greatly appreciated.

Many thanks,
Andrea della Porta

Link:
- [1]: https://github.com/raspberrypi/linux/blob/rpi-6.6.y/arch/arm/boot/dts/broadcom/rp1.dtsi
- [2]: https://github.com/raspberrypi/linux/blob/rpi-6.6.y/drivers/mfd/rp1.c
- [3]: https://lpc.events/event/17/contributions/1421/attachments/1337/2680/LPC2023%20Non-discoverable%20devices%20in%20PCI.pdf
- [4]: https://lore.kernel.org/lkml/20230419231155.GA899497-robh@kernel.org/t/
- [5]: https://lore.kernel.org/lkml/Y862WTT03%2FJxXUG8@kroah.com/

Powered by blists - more mailing lists