| Message ID | 1489158086-9497-1-git-send-email-tharvey@gateworks.com |
|---|---|
| State | Accepted |
| Headers | show |
On 10 March 2017 at 16:01, Tim Harvey <tharvey@gateworks.com> wrote: > The MV88E6176 switch is present on the GW16083 and the GW5904 > > As of a5c32a1f1996f4f75504c4a9abd1c99eaa598df1 these drivers are to be > enabled static in per-target kernels. A standard kernel syntax may be preferred: commit a5c32a1f1996 ("kernel: remove switch driver kmod packages") So does imx6 already work with DSA/switchdev driver? Can you configure switch in /etc/config/network? I didn't track recent progress on this.
On Fri, Mar 10, 2017 at 7:49 AM, Rafał Miłecki <zajec5@gmail.com> wrote: > On 10 March 2017 at 16:01, Tim Harvey <tharvey@gateworks.com> wrote: >> The MV88E6176 switch is present on the GW16083 and the GW5904 >> >> As of a5c32a1f1996f4f75504c4a9abd1c99eaa598df1 these drivers are to be >> enabled static in per-target kernels. > > A standard kernel syntax may be preferred: > commit a5c32a1f1996 ("kernel: remove switch driver kmod packages") I can update the commit log if the patch requires rework. honestly, I'm not all that happy about 'commit a5c32a1f1996 ("kernel: remove switch driver kmod packages")' which removed selecting dsa kernel modules because of perceived bloat but instead this puts bloat into the static kernel of other imx6 boards that dont' have DSA. At least before the modules were selectable. > > So does imx6 already work with DSA/switchdev driver? Can you configure > switch in /etc/config/network? I didn't track recent progress on this. With DSA/switchdev the uplink CPU port must be up first, then the port interfaces can come up. There are two hardware examples of this currently on IMX6 I know of: 1. GW5904 (patch just posted) with IMX6 FEC (eth0) to switch and 4 ports on front panel (lan1, lan2, lan3, lan4). So in this case eth0 must be up first (with no IP config), then lan1-lan4 can be brought up. I'm not sure how to best handle this in /etc/config/network 2. GW5xxx baseboard + GW16083 Ethernet expansion board (patches not posted to LEDE yet). The GW16083 expansion board has an i210 on the PCIe bus as the CPU uplink, 4 phy-less ports to RJ45's, and 2 phy-ports to RJ45/SFP. Various GW5xxx boards a PCIe switch and an expansion connector providing PCI/I2C on a connector to support this. Depending on the baseboard the GW16083 is added to the cpu uplink port may be eth1, or eth2. Tim
On 2017-03-10 17:28, Tim Harvey wrote: > On Fri, Mar 10, 2017 at 7:49 AM, Rafał Miłecki <zajec5@gmail.com> wrote: >> On 10 March 2017 at 16:01, Tim Harvey <tharvey@gateworks.com> wrote: >>> The MV88E6176 switch is present on the GW16083 and the GW5904 >>> >>> As of a5c32a1f1996f4f75504c4a9abd1c99eaa598df1 these drivers are to be >>> enabled static in per-target kernels. >> >> A standard kernel syntax may be preferred: >> commit a5c32a1f1996 ("kernel: remove switch driver kmod packages") > > I can update the commit log if the patch requires rework. > > honestly, I'm not all that happy about 'commit a5c32a1f1996 ("kernel: > remove switch driver kmod packages")' which removed selecting dsa > kernel modules because of perceived bloat but instead this puts bloat > into the static kernel of other imx6 boards that dont' have DSA. At > least before the modules were selectable. Keeping it modular is a bad trade-off, because it was adding bloat to non-imx6 platform kernel images where *no* board was using it. On many of these, bloat is a real factor, as opposed the tiny cosmetic issue that you're describing. I'm pretty sure that no imx6 board is space constrained enough that adding a few kilobytes to the kernel image actually matters in practice. - Felix
On 10 March 2017 at 17:28, Tim Harvey <tharvey@gateworks.com> wrote: > On Fri, Mar 10, 2017 at 7:49 AM, Rafał Miłecki <zajec5@gmail.com> wrote: >> On 10 March 2017 at 16:01, Tim Harvey <tharvey@gateworks.com> wrote: >>> The MV88E6176 switch is present on the GW16083 and the GW5904 >>> >>> As of a5c32a1f1996f4f75504c4a9abd1c99eaa598df1 these drivers are to be >>> enabled static in per-target kernels. >> >> A standard kernel syntax may be preferred: >> commit a5c32a1f1996 ("kernel: remove switch driver kmod packages") > > I can update the commit log if the patch requires rework. That's nothing critical, just a small note for the future. Thanks.
On Fri, Mar 10, 2017 at 8:39 AM, Felix Fietkau <nbd@nbd.name> wrote: > On 2017-03-10 17:28, Tim Harvey wrote: >> On Fri, Mar 10, 2017 at 7:49 AM, Rafał Miłecki <zajec5@gmail.com> wrote: >>> On 10 March 2017 at 16:01, Tim Harvey <tharvey@gateworks.com> wrote: >>>> The MV88E6176 switch is present on the GW16083 and the GW5904 >>>> >>>> As of a5c32a1f1996f4f75504c4a9abd1c99eaa598df1 these drivers are to be >>>> enabled static in per-target kernels. >>> >>> A standard kernel syntax may be preferred: >>> commit a5c32a1f1996 ("kernel: remove switch driver kmod packages") >> >> I can update the commit log if the patch requires rework. >> >> honestly, I'm not all that happy about 'commit a5c32a1f1996 ("kernel: >> remove switch driver kmod packages")' which removed selecting dsa >> kernel modules because of perceived bloat but instead this puts bloat >> into the static kernel of other imx6 boards that dont' have DSA. At >> least before the modules were selectable. > Keeping it modular is a bad trade-off, because it was adding bloat to > non-imx6 platform kernel images where *no* board was using it. On many > of these, bloat is a real factor, as opposed the tiny cosmetic issue > that you're describing. > > I'm pretty sure that no imx6 board is space constrained enough that > adding a few kilobytes to the kernel image actually matters in practice. > Perhaps I'm not understanding as I don't know what you mean by 'cosmetic' issue. Requiring DSA drivers be static in the imx6 kernel means other imx6 boards like wandboard will have that code in their kernel. Making them a configurable module means users have the choice to add that module to their rootfs or leave it out. Is the issue that by default the dsa modules were getting installed on systems without dsa? In that case the 'board profile' (vs 'target') should have enabled those and they should otherwise be defaulted off in menuconfig. Its not a big deal to me at all... like you say this is just a few KB were talking about. It just seemed to me the bloat got shifted to another place is all. Tim
On 2017-03-10 19:58, Tim Harvey wrote: > On Fri, Mar 10, 2017 at 8:39 AM, Felix Fietkau <nbd@nbd.name> wrote: >> On 2017-03-10 17:28, Tim Harvey wrote: >>> On Fri, Mar 10, 2017 at 7:49 AM, Rafał Miłecki <zajec5@gmail.com> wrote: >>>> On 10 March 2017 at 16:01, Tim Harvey <tharvey@gateworks.com> wrote: >>>>> The MV88E6176 switch is present on the GW16083 and the GW5904 >>>>> >>>>> As of a5c32a1f1996f4f75504c4a9abd1c99eaa598df1 these drivers are to be >>>>> enabled static in per-target kernels. >>>> >>>> A standard kernel syntax may be preferred: >>>> commit a5c32a1f1996 ("kernel: remove switch driver kmod packages") >>> >>> I can update the commit log if the patch requires rework. >>> >>> honestly, I'm not all that happy about 'commit a5c32a1f1996 ("kernel: >>> remove switch driver kmod packages")' which removed selecting dsa >>> kernel modules because of perceived bloat but instead this puts bloat >>> into the static kernel of other imx6 boards that dont' have DSA. At >>> least before the modules were selectable. >> Keeping it modular is a bad trade-off, because it was adding bloat to >> non-imx6 platform kernel images where *no* board was using it. On many >> of these, bloat is a real factor, as opposed the tiny cosmetic issue >> that you're describing. >> >> I'm pretty sure that no imx6 board is space constrained enough that >> adding a few kilobytes to the kernel image actually matters in practice. >> > > Perhaps I'm not understanding as I don't know what you mean by > 'cosmetic' issue. Requiring DSA drivers be static in the imx6 kernel > means other imx6 boards like wandboard will have that code in their > kernel. Making them a configurable module means users have the choice > to add that module to their rootfs or leave it out. Is the issue that > by default the dsa modules were getting installed on systems without > dsa? In that case the 'board profile' (vs 'target') should have > enabled those and they should otherwise be defaulted off in > menuconfig. The issue was that enabling DSA as a module selects CONFIG_NET_SWITCHDEV, which is a bool. This causes extra code to be linked into the kernel image, whether the module is installed or not. Since this adds bloat to space sensitive targets, I had to choose between adding dependencies to kmod-dsa, making it selectable only for an arbitrary list of targets, or dropping it entirely and selecting it in the kernel config on targets that need it. I chose the second option, because I didn't see a single case that actually *needed* this to be modular. > Its not a big deal to me at all... like you say this is just a few KB > were talking about. It just seemed to me the bloat got shifted to > another place is all. That is correct. It got shifted from a place where a few kilobytes of bloat actually matters over to a place where it makes no practical difference. - Felix
On Fri, Mar 10, 2017 at 11:05 AM, Felix Fietkau <nbd@nbd.name> wrote: > On 2017-03-10 19:58, Tim Harvey wrote: >> On Fri, Mar 10, 2017 at 8:39 AM, Felix Fietkau <nbd@nbd.name> wrote: >>> On 2017-03-10 17:28, Tim Harvey wrote: >>>> On Fri, Mar 10, 2017 at 7:49 AM, Rafał Miłecki <zajec5@gmail.com> wrote: >>>>> On 10 March 2017 at 16:01, Tim Harvey <tharvey@gateworks.com> wrote: >>>>>> The MV88E6176 switch is present on the GW16083 and the GW5904 >>>>>> >>>>>> As of a5c32a1f1996f4f75504c4a9abd1c99eaa598df1 these drivers are to be >>>>>> enabled static in per-target kernels. >>>>> >>>>> A standard kernel syntax may be preferred: >>>>> commit a5c32a1f1996 ("kernel: remove switch driver kmod packages") >>>> >>>> I can update the commit log if the patch requires rework. >>>> >>>> honestly, I'm not all that happy about 'commit a5c32a1f1996 ("kernel: >>>> remove switch driver kmod packages")' which removed selecting dsa >>>> kernel modules because of perceived bloat but instead this puts bloat >>>> into the static kernel of other imx6 boards that dont' have DSA. At >>>> least before the modules were selectable. >>> Keeping it modular is a bad trade-off, because it was adding bloat to >>> non-imx6 platform kernel images where *no* board was using it. On many >>> of these, bloat is a real factor, as opposed the tiny cosmetic issue >>> that you're describing. >>> >>> I'm pretty sure that no imx6 board is space constrained enough that >>> adding a few kilobytes to the kernel image actually matters in practice. >>> >> >> Perhaps I'm not understanding as I don't know what you mean by >> 'cosmetic' issue. Requiring DSA drivers be static in the imx6 kernel >> means other imx6 boards like wandboard will have that code in their >> kernel. Making them a configurable module means users have the choice >> to add that module to their rootfs or leave it out. Is the issue that >> by default the dsa modules were getting installed on systems without >> dsa? In that case the 'board profile' (vs 'target') should have >> enabled those and they should otherwise be defaulted off in >> menuconfig. > The issue was that enabling DSA as a module selects > CONFIG_NET_SWITCHDEV, which is a bool. This causes extra code to be > linked into the kernel image, whether the module is installed or not. > > Since this adds bloat to space sensitive targets, I had to choose > between adding dependencies to kmod-dsa, making it selectable only for > an arbitrary list of targets, or dropping it entirely and selecting it > in the kernel config on targets that need it. > > I chose the second option, because I didn't see a single case that > actually *needed* this to be modular. > >> Its not a big deal to me at all... like you say this is just a few KB >> were talking about. It just seemed to me the bloat got shifted to >> another place is all. > That is correct. It got shifted from a place where a few kilobytes of > bloat actually matters over to a place where it makes no practical > difference. > > - Felix > Got it - thanks! I'm happy to simply have this patch merged in to enable it on IMX6 targets. Tim
On 03/10/2017 08:28 AM, Tim Harvey wrote: > On Fri, Mar 10, 2017 at 7:49 AM, Rafał Miłecki <zajec5@gmail.com> wrote: >> On 10 March 2017 at 16:01, Tim Harvey <tharvey@gateworks.com> wrote: >>> The MV88E6176 switch is present on the GW16083 and the GW5904 >>> >>> As of a5c32a1f1996f4f75504c4a9abd1c99eaa598df1 these drivers are to be >>> enabled static in per-target kernels. >> >> A standard kernel syntax may be preferred: >> commit a5c32a1f1996 ("kernel: remove switch driver kmod packages") > > I can update the commit log if the patch requires rework. > > honestly, I'm not all that happy about 'commit a5c32a1f1996 ("kernel: > remove switch driver kmod packages")' which removed selecting dsa > kernel modules because of perceived bloat but instead this puts bloat > into the static kernel of other imx6 boards that dont' have DSA. At > least before the modules were selectable. > >> >> So does imx6 already work with DSA/switchdev driver? Can you configure >> switch in /etc/config/network? I didn't track recent progress on this. > > With DSA/switchdev the uplink CPU port must be up first, then the port > interfaces can come up. > > There are two hardware examples of this currently on IMX6 I know of: > > 1. GW5904 (patch just posted) with IMX6 FEC (eth0) to switch and 4 > ports on front panel (lan1, lan2, lan3, lan4). So in this case eth0 > must be up first (with no IP config), then lan1-lan4 can be brought > up. I'm not sure how to best handle this in /etc/config/network > > 2. GW5xxx baseboard + GW16083 Ethernet expansion board (patches not > posted to LEDE yet). The GW16083 expansion board has an i210 on the > PCIe bus as the CPU uplink, 4 phy-less ports to RJ45's, and 2 > phy-ports to RJ45/SFP. Various GW5xxx boards a PCIe switch and an > expansion connector providing PCI/I2C on a connector to support this. > Depending on the baseboard the GW16083 is added to the cpu uplink port > may be eth1, or eth2. Ideally we should keep the same configuration syntax that we already have, and replace swconfig calls with bridge + bridge vlan calls. The key difference with DSA is going to be that there is no "switchX" device that we are going to be referencing, instead, individual ports will be there. For instance, if you previously configured your switch with: - Port 0 through 3, in VLAN 1, untagged - Port 4 in VLAN 2, untagged - CPU port in VLAN 1 tagged and VLAN 2 tagged This will be something like: brctl addbr br0 for port in $(seq 0 4) do brctl addif br0 lan$port bridge vlan add vid 1 pvid untagged dev lan$port done bridge vlan add vid 2 pvid untagged dev lan4 vconfig br0 2 And that should do it. NB: depending on whether the switch has Marvell/Broadcom/QCA tags enabled or not, traffic is received/sent either through br* devices (tagged), or eth* devices (non tagged), that's a bit inconsistent. The way you could tell is by trying to add eth* to a bridge, when it's tagged, it won't be allowed because the DSA hook in the network stack conflicts with bridge packets reception. Or you can just know what is configured by the DSA switch driver.
diff --git a/target/linux/imx6/config-4.9 b/target/linux/imx6/config-4.9 index 9aafa50..b4d23de 100644 --- a/target/linux/imx6/config-4.9 +++ b/target/linux/imx6/config-4.9 @@ -352,8 +352,16 @@ CONFIG_MUTEX_SPIN_ON_OWNER=y CONFIG_MXS_DMA=y CONFIG_NEED_DMA_MAP_STATE=y CONFIG_NEON=y +CONFIG_NET_DSA=y +# CONFIG_B53 is not set +CONFIG_NET_DSA_MV88E6XXX=y +CONFIG_NET_DSA_MV88E6XXX_GLOBAL2=y +# CONFIG_NET_DSA_QCA8K is not set +CONFIG_NET_DSA_TAG_DSA=y +CONFIG_NET_DSA_TAG_EDSA=y CONFIG_NET_FLOW_LIMIT=y CONFIG_NET_PTP_CLASSIFY=y +CONFIG_NET_SWITCHDEV=y CONFIG_NLS=y CONFIG_NLS_CODEPAGE_437=y CONFIG_NO_BOOTMEM=y
The MV88E6176 switch is present on the GW16083 and the GW5904 As of a5c32a1f1996f4f75504c4a9abd1c99eaa598df1 these drivers are to be enabled static in per-target kernels. Signed-off-by: Tim Harvey <tharvey@gateworks.com> --- target/linux/imx6/config-4.9 | 8 ++++++++ 1 file changed, 8 insertions(+)