| Message ID | 20240423110238.32148-2-zajec5@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] ARM: dts: BCM5301X: Drop ranges mapping from AXI bus | expand |
On Tue, Apr 23, 2024 at 1:03 PM Rafał Miłecki <zajec5@gmail.com> wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > This fixes: > arch/arm/boot/dts/broadcom/bcm4708-asus-rt-ac56u.dtb: pcie@18012000: 'device_type' is a required property > from schema $id: http://devicetree.org/schemas/pci/pci-bus.yaml# > arch/arm/boot/dts/broadcom/bcm4708-asus-rt-ac56u.dtb: pcie@18012000: 'ranges' is a required property > from schema $id: http://devicetree.org/schemas/pci/pci-bus.yaml# > arch/arm/boot/dts/broadcom/bcm4708-asus-rt-ac56u.dtb: pcie@18013000: 'device_type' is a required property > from schema $id: http://devicetree.org/schemas/pci/pci-bus.yaml# > arch/arm/boot/dts/broadcom/bcm4708-asus-rt-ac56u.dtb: pcie@18013000: 'ranges' is a required property > from schema $id: http://devicetree.org/schemas/pci/pci-bus.yaml# > arch/arm/boot/dts/broadcom/bcm4708-asus-rt-ac56u.dtb: pcie@18014000: 'device_type' is a required property > from schema $id: http://devicetree.org/schemas/pci/pci-bus.yaml# > arch/arm/boot/dts/broadcom/bcm4708-asus-rt-ac56u.dtb: pcie@18014000: 'ranges' is a required property > from schema $id: http://devicetree.org/schemas/pci/pci-bus.yaml# > > Cc: Arınç ÜNAL <arinc.unal@arinc9.com> > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> Looks right to me! Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On 4/23/24 04:02, Rafał Miłecki wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > This fixes: > arch/arm/boot/dts/broadcom/bcm4708-asus-rt-ac56u.dtb: pcie@18012000: 'device_type' is a required property > from schema $id: http://devicetree.org/schemas/pci/pci-bus.yaml# > arch/arm/boot/dts/broadcom/bcm4708-asus-rt-ac56u.dtb: pcie@18012000: 'ranges' is a required property > from schema $id: http://devicetree.org/schemas/pci/pci-bus.yaml# > arch/arm/boot/dts/broadcom/bcm4708-asus-rt-ac56u.dtb: pcie@18013000: 'device_type' is a required property > from schema $id: http://devicetree.org/schemas/pci/pci-bus.yaml# > arch/arm/boot/dts/broadcom/bcm4708-asus-rt-ac56u.dtb: pcie@18013000: 'ranges' is a required property > from schema $id: http://devicetree.org/schemas/pci/pci-bus.yaml# > arch/arm/boot/dts/broadcom/bcm4708-asus-rt-ac56u.dtb: pcie@18014000: 'device_type' is a required property > from schema $id: http://devicetree.org/schemas/pci/pci-bus.yaml# > arch/arm/boot/dts/broadcom/bcm4708-asus-rt-ac56u.dtb: pcie@18014000: 'ranges' is a required property > from schema $id: http://devicetree.org/schemas/pci/pci-bus.yaml# > > Cc: Arınç ÜNAL <arinc.unal@arinc9.com> > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> OK, so this is the rationale for patch #1, because you are adding a 'ranges' property to each PCIe root complex node, and you need the values in the 'ranges' property to be expressed relative to the global address space, and not the axi@18000000 address space, you needed to flatten the axi@18000000 range. Why not just bring those 3 nodes out of the axi@18000000 node into the global address space then? That would greatly limit the amount of changes in patch #1, some of which are just unfortunate noise. From the chip diagram, each PCIe controller has its own separate AXI interface to the NIC 301 AXI fabric, so having 3 independent nodes which are not tied to the axi@18000000 would not be wrong IMHO. Thanks for doing this!
On 23.04.2024 21:03, Florian Fainelli wrote: > On 4/23/24 04:02, Rafał Miłecki wrote: >> From: Rafał Miłecki <rafal@milecki.pl> >> >> This fixes: >> arch/arm/boot/dts/broadcom/bcm4708-asus-rt-ac56u.dtb: pcie@18012000: 'device_type' is a required property >> from schema $id: http://devicetree.org/schemas/pci/pci-bus.yaml# >> arch/arm/boot/dts/broadcom/bcm4708-asus-rt-ac56u.dtb: pcie@18012000: 'ranges' is a required property >> from schema $id: http://devicetree.org/schemas/pci/pci-bus.yaml# >> arch/arm/boot/dts/broadcom/bcm4708-asus-rt-ac56u.dtb: pcie@18013000: 'device_type' is a required property >> from schema $id: http://devicetree.org/schemas/pci/pci-bus.yaml# >> arch/arm/boot/dts/broadcom/bcm4708-asus-rt-ac56u.dtb: pcie@18013000: 'ranges' is a required property >> from schema $id: http://devicetree.org/schemas/pci/pci-bus.yaml# >> arch/arm/boot/dts/broadcom/bcm4708-asus-rt-ac56u.dtb: pcie@18014000: 'device_type' is a required property >> from schema $id: http://devicetree.org/schemas/pci/pci-bus.yaml# >> arch/arm/boot/dts/broadcom/bcm4708-asus-rt-ac56u.dtb: pcie@18014000: 'ranges' is a required property >> from schema $id: http://devicetree.org/schemas/pci/pci-bus.yaml# >> >> Cc: Arınç ÜNAL <arinc.unal@arinc9.com> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > > OK, so this is the rationale for patch #1, because you are adding a 'ranges' property to each PCIe root complex node, and you need the values in the 'ranges' property to be expressed relative to the global address space, and not the axi@18000000 address space, you needed to flatten the axi@18000000 range. > > Why not just bring those 3 nodes out of the axi@18000000 node into the global address space then? That would greatly limit the amount of changes in patch #1, some of which are just unfortunate noise. > > From the chip diagram, each PCIe controller has its own separate AXI interface to the NIC 301 AXI fabric, so having 3 independent nodes which are not tied to the axi@18000000 would not be wrong IMHO. I got impression that memory mapped blocks should preferably go into a "soc" node. It's what I seen in some other platforms and what is also present (thought not directly documented) in the dts-coding-style.rst . We don't have "soc" node but "axi@18000000" seems like our substitute. So I thought we should keep PCIe controllers nodes in axi@ (soc@).
On 4/23/24 23:43, Rafał Miłecki wrote: > On 23.04.2024 21:03, Florian Fainelli wrote: >> On 4/23/24 04:02, Rafał Miłecki wrote: >>> From: Rafał Miłecki <rafal@milecki.pl> >>> >>> This fixes: >>> arch/arm/boot/dts/broadcom/bcm4708-asus-rt-ac56u.dtb: pcie@18012000: >>> 'device_type' is a required property >>> from schema $id: >>> http://devicetree.org/schemas/pci/pci-bus.yaml# >>> arch/arm/boot/dts/broadcom/bcm4708-asus-rt-ac56u.dtb: pcie@18012000: >>> 'ranges' is a required property >>> from schema $id: >>> http://devicetree.org/schemas/pci/pci-bus.yaml# >>> arch/arm/boot/dts/broadcom/bcm4708-asus-rt-ac56u.dtb: pcie@18013000: >>> 'device_type' is a required property >>> from schema $id: >>> http://devicetree.org/schemas/pci/pci-bus.yaml# >>> arch/arm/boot/dts/broadcom/bcm4708-asus-rt-ac56u.dtb: pcie@18013000: >>> 'ranges' is a required property >>> from schema $id: >>> http://devicetree.org/schemas/pci/pci-bus.yaml# >>> arch/arm/boot/dts/broadcom/bcm4708-asus-rt-ac56u.dtb: pcie@18014000: >>> 'device_type' is a required property >>> from schema $id: >>> http://devicetree.org/schemas/pci/pci-bus.yaml# >>> arch/arm/boot/dts/broadcom/bcm4708-asus-rt-ac56u.dtb: pcie@18014000: >>> 'ranges' is a required property >>> from schema $id: >>> http://devicetree.org/schemas/pci/pci-bus.yaml# >>> >>> Cc: Arınç ÜNAL <arinc.unal@arinc9.com> >>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >> >> OK, so this is the rationale for patch #1, because you are adding a >> 'ranges' property to each PCIe root complex node, and you need the >> values in the 'ranges' property to be expressed relative to the global >> address space, and not the axi@18000000 address space, you needed to >> flatten the axi@18000000 range. >> >> Why not just bring those 3 nodes out of the axi@18000000 node into the >> global address space then? That would greatly limit the amount of >> changes in patch #1, some of which are just unfortunate noise. >> >> From the chip diagram, each PCIe controller has its own separate AXI >> interface to the NIC 301 AXI fabric, so having 3 independent nodes >> which are not tied to the axi@18000000 would not be wrong IMHO. > > I got impression that memory mapped blocks should preferably go into a > "soc" node. It's what I seen in some other platforms and what is also > present (thought not directly documented) in the dts-coding-style.rst . > > We don't have "soc" node but "axi@18000000" seems like our substitute. > > So I thought we should keep PCIe controllers nodes in axi@ (soc@). PCIe Host Bridges are sort of special because they are bridges but also have a configuration interface via the MMIO register space. Tying them to a particular bus node in the .dts(i) that accurately describes how they are interfaced to the host CPU might be a tad difficult sometimes. My preference as a maintainer would be to minimize the amount of node(s) being changed and just isolate the 3 PCIe host bridges directly under the root node. Krysztof, any objection to that?
diff --git a/arch/arm/boot/dts/broadcom/bcm-ns.dtsi b/arch/arm/boot/dts/broadcom/bcm-ns.dtsi index 7c8ee2df538f..fac3e9859b3e 100644 --- a/arch/arm/boot/dts/broadcom/bcm-ns.dtsi +++ b/arch/arm/boot/dts/broadcom/bcm-ns.dtsi @@ -182,22 +182,40 @@ chipcommon: chipcommon@18000000 { }; pcie0: pcie@18012000 { + compatible = "brcm,iproc-pcie"; reg = <0x18012000 0x1000>; - + ranges = <0x82000000 0 0x08000000 0x08000000 0 0x08000000>; + interrupt-map-mask = <0 0 0 0>; + interrupt-map = <0 0 0 0 &gic GIC_SPI 126 IRQ_TYPE_LEVEL_HIGH>; + bus-range = <0x00 0xff>; + device_type = "pci"; + #interrupt-cells = <1>; #address-cells = <3>; #size-cells = <2>; }; pcie1: pcie@18013000 { + compatible = "brcm,iproc-pcie"; reg = <0x18013000 0x1000>; - + ranges = <0x82000000 0 0x20000000 0x20000000 0 0x08000000>; + interrupt-map-mask = <0 0 0 0>; + interrupt-map = <0 0 0 0 &gic GIC_SPI 132 IRQ_TYPE_LEVEL_HIGH>; + bus-range = <0x00 0xff>; + device_type = "pci"; + #interrupt-cells = <1>; #address-cells = <3>; #size-cells = <2>; }; pcie2: pcie@18014000 { + compatible = "brcm,iproc-pcie"; reg = <0x18014000 0x1000>; - + ranges = <0x82000000 0 0x28000000 0x28000000 0 0x08000000>; + interrupt-map-mask = <0 0 0 0>; + interrupt-map = <0 0 0 0 &gic GIC_SPI 138 IRQ_TYPE_LEVEL_HIGH>; + bus-range = <0x00 0xff>; + device_type = "pci"; + #interrupt-cells = <1>; #address-cells = <3>; #size-cells = <2>; };