| Message ID | 20231130191815.2421978-1-robh@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: dts: imx8mm-venice: Fix PCI bus nodes | expand |
Hi Rob, On Thu, Nov 30, 2023 at 4:18 PM Rob Herring <robh@kernel.org> wrote: > > The imx8mm-venice boards PCI bus nodes are a complete mess. The > unit-addresses are wrong. The PCI bridge nodes are incomplete missing > "device_type" and "ranges" and just wrong for "#address-cells" and > "#size-cells" values. > > All of these issues are reported warnings if anyone bothered to pay > attention. Sigh. The warnings are gone in linux-next: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/arch/arm64/boot/dts/freescale?h=next-20231130&id=d61c5068729a76a6183a897bcad4bf26e8d3d674
On Thu, Nov 30, 2023 at 1:28 PM Fabio Estevam <festevam@gmail.com> wrote: > > Hi Rob, > > On Thu, Nov 30, 2023 at 4:18 PM Rob Herring <robh@kernel.org> wrote: > > > > The imx8mm-venice boards PCI bus nodes are a complete mess. The > > unit-addresses are wrong. The PCI bridge nodes are incomplete missing > > "device_type" and "ranges" and just wrong for "#address-cells" and > > "#size-cells" values. > > > > All of these issues are reported warnings if anyone bothered to pay > > attention. Sigh. > > The warnings are gone in linux-next: > > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/arch/arm64/boot/dts/freescale?h=next-20231130&id=d61c5068729a76a6183a897bcad4bf26e8d3d674 Linux-next is wrong. The ethernet device should have a node name of 'ethernet'. The 'pcie' node name and 'device_type = "pci"' is for PCI buses/bridges only. Rob
On Thu, Nov 30, 2023 at 2:33 PM Rob Herring <robh@kernel.org> wrote: > > On Thu, Nov 30, 2023 at 1:28 PM Fabio Estevam <festevam@gmail.com> wrote: > > > > Hi Rob, > > > > On Thu, Nov 30, 2023 at 4:18 PM Rob Herring <robh@kernel.org> wrote: > > > > > > The imx8mm-venice boards PCI bus nodes are a complete mess. The > > > unit-addresses are wrong. The PCI bridge nodes are incomplete missing > > > "device_type" and "ranges" and just wrong for "#address-cells" and > > > "#size-cells" values. > > > > > > All of these issues are reported warnings if anyone bothered to pay > > > attention. Sigh. Rob, Sorry about that. At the time the dt was submitted there were still so many dt warnings it wasn't very clear what was a legitimate issue and the PCI bindings are not that easy to understand. > > > > The warnings are gone in linux-next: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/arch/arm64/boot/dts/freescale?h=next-20231130&id=d61c5068729a76a6183a897bcad4bf26e8d3d674 > > Linux-next is wrong. The ethernet device should have a node name of > 'ethernet'. The 'pcie' node name and 'device_type = "pci"' is for PCI > buses/bridges only. So as Fabio has tried to fix this with a patch that landed in linux-next this patch won't apply. I'll submit one that covers your changes. It's always been unfortunate to have to have this level of detail in a device-tree just to allow boot firmware to populate the mac address of a PCI ethernet device. Best Regards, Tim
On Tue, Dec 5, 2023 at 12:13 PM Tim Harvey <tharvey@gateworks.com> wrote: > > On Thu, Nov 30, 2023 at 2:33 PM Rob Herring <robh@kernel.org> wrote: > > > > On Thu, Nov 30, 2023 at 1:28 PM Fabio Estevam <festevam@gmail.com> wrote: > > > > > > Hi Rob, > > > > > > On Thu, Nov 30, 2023 at 4:18 PM Rob Herring <robh@kernel.org> wrote: > > > > > > > > The imx8mm-venice boards PCI bus nodes are a complete mess. The > > > > unit-addresses are wrong. The PCI bridge nodes are incomplete missing > > > > "device_type" and "ranges" and just wrong for "#address-cells" and > > > > "#size-cells" values. > > > > > > > > All of these issues are reported warnings if anyone bothered to pay > > > > attention. Sigh. > > Rob, > > Sorry about that. At the time the dt was submitted there were still so > many dt warnings it wasn't very clear what was a legitimate issue and > the PCI bindings are not that easy to understand. > > > > > > > The warnings are gone in linux-next: > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/arch/arm64/boot/dts/freescale?h=next-20231130&id=d61c5068729a76a6183a897bcad4bf26e8d3d674 > > > > Linux-next is wrong. The ethernet device should have a node name of > > 'ethernet'. The 'pcie' node name and 'device_type = "pci"' is for PCI > > buses/bridges only. > > So as Fabio has tried to fix this with a patch that landed in > linux-next this patch won't apply. I'll submit one that covers your > changes. Thanks. > It's always been unfortunate to have to have this level of detail in a > device-tree just to allow boot firmware to populate the mac address of > a PCI ethernet device. More unfortunate are incomplete h/w designs lacking the MAC address. :( Not really any way around it I think if you want something that works for any device and any number of devices. Rob
diff --git a/arch/arm64/boot/dts/freescale/imx8mm-venice-gw72xx.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-venice-gw72xx.dtsi index 3a0a10e835a2..ff461b004dc5 100644 --- a/arch/arm64/boot/dts/freescale/imx8mm-venice-gw72xx.dtsi +++ b/arch/arm64/boot/dts/freescale/imx8mm-venice-gw72xx.dtsi @@ -150,25 +150,29 @@ &pcie0 { <&clk IMX8MM_SYS_PLL2_250M>; status = "okay"; - pcie@0,0 { + pcie@0 { + device_type = "pci"; reg = <0x0000 0 0 0 0>; - #address-cells = <1>; - #size-cells = <0>; + #address-cells = <3>; + #size-cells = <2>; + ranges; - pcie@1,0 { + pcie@0 { + device_type = "pci"; reg = <0x0000 0 0 0 0>; - #address-cells = <1>; - #size-cells = <0>; + #address-cells = <3>; + #size-cells = <2>; + ranges; - pcie@2,3 { + pcie@3 { + device_type = "pci"; reg = <0x1800 0 0 0 0>; - #address-cells = <1>; - #size-cells = <0>; + #address-cells = <3>; + #size-cells = <2>; + ranges; - eth1: pcie@5,0 { + eth1: ethernet@0 { reg = <0x0000 0 0 0 0>; - #address-cells = <1>; - #size-cells = <0>; local-mac-address = [00 00 00 00 00 00]; }; diff --git a/arch/arm64/boot/dts/freescale/imx8mm-venice-gw73xx.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-venice-gw73xx.dtsi index d79fe9f62b95..6f5a6d91c95e 100644 --- a/arch/arm64/boot/dts/freescale/imx8mm-venice-gw73xx.dtsi +++ b/arch/arm64/boot/dts/freescale/imx8mm-venice-gw73xx.dtsi @@ -177,25 +177,29 @@ &pcie0 { <&clk IMX8MM_SYS_PLL2_250M>; status = "okay"; - pcie@0,0 { + pcie@0 { + device_type = "pci"; reg = <0x0000 0 0 0 0>; - #address-cells = <1>; - #size-cells = <0>; + #address-cells = <3>; + #size-cells = <2>; + ranges; - pcie@1,0 { + pcie@0 { + device_type = "pci"; reg = <0x0000 0 0 0 0>; - #address-cells = <1>; - #size-cells = <0>; + #address-cells = <3>; + #size-cells = <2>; + ranges; - pcie@2,4 { + pcie@4 { + device_type = "pci"; reg = <0x2000 0 0 0 0>; - #address-cells = <1>; - #size-cells = <0>; + #address-cells = <3>; + #size-cells = <2>; + ranges; - eth1: pcie@6,0 { + eth1: ethernet@0 { reg = <0x0000 0 0 0 0>; - #address-cells = <1>; - #size-cells = <0>; local-mac-address = [00 00 00 00 00 00]; }; diff --git a/arch/arm64/boot/dts/freescale/imx8mm-venice-gw7902.dts b/arch/arm64/boot/dts/freescale/imx8mm-venice-gw7902.dts index 06a394a41d7c..4bb22fdc5d2e 100644 --- a/arch/arm64/boot/dts/freescale/imx8mm-venice-gw7902.dts +++ b/arch/arm64/boot/dts/freescale/imx8mm-venice-gw7902.dts @@ -633,15 +633,15 @@ &pcie0 { <&clk IMX8MM_SYS_PLL2_250M>; status = "okay"; - pcie@0,0 { + pcie@0 { + device_type = "pci"; reg = <0x0000 0 0 0 0>; - #address-cells = <1>; - #size-cells = <0>; + #address-cells = <3>; + #size-cells = <2>; + ranges; - eth1: pcie@1,0 { + eth1: ethernet@0 { reg = <0x0000 0 0 0 0>; - #address-cells = <1>; - #size-cells = <0>; local-mac-address = [00 00 00 00 00 00]; };
The imx8mm-venice boards PCI bus nodes are a complete mess. The unit-addresses are wrong. The PCI bridge nodes are incomplete missing "device_type" and "ranges" and just wrong for "#address-cells" and "#size-cells" values. All of these issues are reported warnings if anyone bothered to pay attention. Sigh. The kernel may have happened to work because it only looks at "reg" to assign the DT nodes to PCI devices. Based on that, I'm assuming "reg" is correctly matching the devices present, and the unit-addresses are wrong. Presumably the bootloader fills in "local-mac-address" with something valid. Hopefully it too uses "reg" rather than the path. Fixes: afb424b99e0f ("arm64: dts: imx8mm-venice*: add PCIe support") Signed-off-by: Rob Herring <robh@kernel.org> --- .../dts/freescale/imx8mm-venice-gw72xx.dtsi | 28 +++++++++++-------- .../dts/freescale/imx8mm-venice-gw73xx.dtsi | 28 +++++++++++-------- .../dts/freescale/imx8mm-venice-gw7902.dts | 12 ++++---- 3 files changed, 38 insertions(+), 30 deletions(-)