| Message ID | 20210813030155.23097-1-hongbo.wang@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] arm64: dts: fsl: ls1028a-rdb: Add dts file to choose swp5 as dsa master | expand |
On Fri, Aug 13, 2021 at 11:01:55AM +0800, hongbo.wang@nxp.com wrote: > From: hongbo wang <hongbo.wang@nxp.com> > > some use cases want to use swp4-eno2 link as ordinary data path, > so we can enable swp5 as dsa master, the data from kernel can > be transmitted to eno3, then send to swp5 via internal link, switch will > forward it to swp0-3. > > the data to kernel will come from swp0-3, and received by kernel > via swp5-eno3 link. > new file mode 100644 > index 000000000000..a88396c137a1 > --- /dev/null > +++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb-dsa-swp5-eno3.dts > @@ -0,0 +1,27 @@ > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) > +/* > + * Device Tree file for NXP LS1028A RDB with dsa master swp5-eno3. > + * > + * Copyright 2018-2021 NXP > + * > + * Hongbo Wang <hongbo.wang@nxp.com> > + * > + */ > + > +/dts-v1/; > +#include "fsl-ls1028a-rdb.dts" You will end up with two DT blobs with the same top level compatible. This is going to cause confusion. I suggest you add an additional top level compatible to make it clear this differs from the compatible = "fsl,ls1028a-rdb", "fsl,ls1028a" blob. Andrew
On Fri, Aug 13, 2021 at 01:56:53PM +0000, Hongbo Wang wrote: > > You will end up with two DT blobs with the same top level compatible. This is > > going to cause confusion. I suggest you add an additional top level compatible > > to make it clear this differs from the compatible = "fsl,ls1028a-rdb", > > "fsl,ls1028a" blob. > > > > Andrew > > hi Andrew, > > thanks for comments. > > this "fsl-ls1028a-rdb-dsa-swp5-eno3.dts" is also for fsl-ls1028a-rdb platform, > the only difference with "fsl-ls1028a-rdb.dts" is that it use swp5 as dsa master, not swp4, > and it's based on "fsl-ls1028a-rdb.dts", so I choose this manner, > if "fsl-ls1028a-rdb.dts" has some modification for new version, this file don't need be changed. I tend to agree with Hongbo. What confusion is it going to cause? It is fundamentally the same board, just an Ethernet port stopped having 'status = "disabled"' and another changed role, all inside of the SoC with no externally-visible change. If anything, I think that creating a new top-level compatible for each small change like this would create a bloat-fest of its own. I was going to suggest as an alternative to define a device tree overlay file with the changes in the CPU port assignment, instead of defining a wholly new DTS for the LS1028A reference design board. But I am pretty sure that it is not possible to specify a /delete-property/ inside a device tree overlay file, so that won't actually work.
> On Fri, Aug 13, 2021 at 01:56:53PM +0000, Hongbo Wang wrote: > > > You will end up with two DT blobs with the same top level > > > compatible. This is going to cause confusion. I suggest you add an > > > additional top level compatible to make it clear this differs from > > > the compatible = "fsl,ls1028a-rdb", "fsl,ls1028a" blob. > > > > > > Andrew > > > > hi Andrew, > > > > thanks for comments. > > > > this "fsl-ls1028a-rdb-dsa-swp5-eno3.dts" is also for fsl-ls1028a-rdb > > platform, the only difference with "fsl-ls1028a-rdb.dts" is that it > > use swp5 as dsa master, not swp4, and it's based on > > "fsl-ls1028a-rdb.dts", so I choose this manner, if "fsl-ls1028a-rdb.dts" has > some modification for new version, this file don't need be changed. > > I tend to agree with Hongbo. What confusion is it going to cause? It is > fundamentally the same board, just an Ethernet port stopped having 'status = > "disabled"' and another changed role, all inside of the SoC with no > externally-visible change. If anything, I think that creating a new top-level > compatible for each small change like this would create a bloat-fest of its own. > > I was going to suggest as an alternative to define a device tree overlay file with > the changes in the CPU port assignment, instead of defining a wholly new DTS > for the LS1028A reference design board. But I am pretty sure that it is not > possible to specify a /delete-property/ inside a device tree overlay file, so that > won't actually work. hi Vladimir, if don't specify "/delete-property/" in this dts file, the corresponding dtb will not work well, so I add it to delete 'ethernet' property from mscc_felix_port4 explicitly. thanks, hongbo
> On Mon, Aug 16, 2021 at 06:03:52AM +0000, Hongbo Wang wrote: > > > I was going to suggest as an alternative to define a device tree > > > overlay file with the changes in the CPU port assignment, instead of > > > defining a wholly new DTS for the LS1028A reference design board. > > > But I am pretty sure that it is not possible to specify a > > > /delete-property/ inside a device tree overlay file, so that won't actually > work. > > > > hi Vladimir, > > > > if don't specify "/delete-property/" in this dts file, the > > corresponding dtb will not work well, so I add it to delete 'ethernet' property > from mscc_felix_port4 explicitly. > > Judging by the reply, I am not actually sure you've understood what has been > said. > > I said: > > There is an option to create a device tree overlay: > > https://www.kernel.org/doc/html/latest/devicetree/overlay-notes.html > > We use these for the riser cards on the LS1028A-QDS boards. > > https://source.codeaurora.org/external/qoriq/qoriq-components/linux/tree/ar > ch/arm64/boot/dts/freescale/fsl-ls1028a-qds-13bb.dts?h=LSDK-20.12-V5.4 > > They are included as usual in a U-Boot ITB file: > > / { > images { > /* Base DTB */ > ls1028aqds-dtb { > description = "ls1028aqds-dtb"; > data = > /incbin/("arch/arm64/boot/dts/freescale/fsl-ls1028a-qds.dtb"); > type = "flat_dt"; > arch = "arm64"; > os = "linux"; > compression = "none"; > load = <0x90000000>; > hash@1 { > algo = "crc32"; > }; > }; > /* Overlay */ > fdt@ls1028aqds-13bb { > description = "ls1028aqds-13bb"; > data = > /incbin/("arch/arm64/boot/dts/freescale/fsl-ls1028a-qds-13bb.dtb"); > type = "flat_dt"; > arch = "arm64"; > load = <0x90010000>; > }; > }; > }; > > In U-Boot, you apply the overlay as following: > > tftp $kernel_addr_r boot.itb && bootm > $kernel_addr_r#ls1028aqds#ls1028aqds-13bb > > It would have been nice to have a similar device tree overlay that changes the > DSA master from eno2 to eno3, and for that overlay to be able to be applied > (or not) from U-Boot. > > But it's _not_ possible, because you cannot put the /delete-property/ (that you > need to have) in the .dtbo file. Or if you put it, it will not delete the property > from the base dtb. > > That's all I said. thanks for the detailed explanation, I have got your point. thanks, hongbo
diff --git a/arch/arm64/boot/dts/freescale/Makefile b/arch/arm64/boot/dts/freescale/Makefile index 25806c4924cb..032aaf52079a 100644 --- a/arch/arm64/boot/dts/freescale/Makefile +++ b/arch/arm64/boot/dts/freescale/Makefile @@ -12,6 +12,7 @@ dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls1028a-kontron-sl28-var3-ads2.dtb dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls1028a-kontron-sl28-var4.dtb dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls1028a-qds.dtb dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls1028a-rdb.dtb +dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls1028a-rdb-dsa-swp5-eno3.dtb dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls1043a-qds.dtb dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls1043a-rdb.dtb dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls1046a-frwy.dtb diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb-dsa-swp5-eno3.dts b/arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb-dsa-swp5-eno3.dts new file mode 100644 index 000000000000..a88396c137a1 --- /dev/null +++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb-dsa-swp5-eno3.dts @@ -0,0 +1,27 @@ +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) +/* + * Device Tree file for NXP LS1028A RDB with dsa master swp5-eno3. + * + * Copyright 2018-2021 NXP + * + * Hongbo Wang <hongbo.wang@nxp.com> + * + */ + +/dts-v1/; +#include "fsl-ls1028a-rdb.dts" + +&enetc_port3 { + status = "okay"; +}; + +&mscc_felix_port4 { + label = "swp4"; + /delete-property/ ethernet; + status = "okay"; +}; + +&mscc_felix_port5 { + ethernet = <&enetc_port3>; + status = "okay"; +};