Hi Krzysztof, On 18:52 Fri 30 Aug , Krzysztof Kozlowski wrote: > 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@xxxxxxxx> > >>> --- > >>> 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 (---). Ack. > > > 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. Ack. > > > 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@xxxxxxxx> > >>> 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). 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. 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'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?. Many thanks, Andrea > > Best regards, > Krzysztof >
