[OpenWrt-Devel] [PATCH] bcm53xx: add support for Luxul FullMAC WiFi devices
Rip Route
riproute at gmail.com
Mon Apr 6 16:51:54 EDT 2020
On Mon, Apr 6, 2020 at 2:03 PM Rafał Miłecki <zajec5 at gmail.com> wrote:
>
> On 06.04.2020 21:39, mail at adrianschmutzler.de wrote:
> >> -----Original Message-----
> >> From: openwrt-devel [mailto:openwrt-devel-bounces at lists.openwrt.org]
> >> On Behalf Of Rafal Milecki
> >> Sent: Montag, 6. April 2020 21:26
> >> To: mail at adrianschmutzler.de; 'Dan Haab' <riproute at gmail.com>; openwrt-
> >> devel at lists.openwrt.org
> >> Cc: 'Dan Haab' <dan.haab at legrand.com>
> >> Subject: Re: [OpenWrt-Devel] [PATCH] bcm53xx: add support for Luxul
> >> FullMAC WiFi devices
> >>
> >> On 06.04.2020 20:26, mail at adrianschmutzler.de wrote:
> >>>> -----Original Message-----
> >>>> From: openwrt-devel [mailto:openwrt-devel-
> >> bounces at lists.openwrt.org]
> >>>> On Behalf Of Dan Haab
> >>>> Sent: Montag, 6. April 2020 20:20
> >>>> To: openwrt-devel at lists.openwrt.org
> >>>> Cc: Dan Haab <dan.haab at legrand.com>
> >>>> Subject: [OpenWrt-Devel] [PATCH] bcm53xx: add support for Luxul
> >>>> FullMAC WiFi devices
> >>>>
> >>>> From: Dan Haab <dan.haab at legrand.com>
> >>>>
> >>>> This prepares support for models XAP-1610 and XWR-3150. Flashing
> >>>> requires using Luxul firmware version:
> >>>> 1) 8.1.0 or newer for XAP-1610
> >>>> 2) 6.4.0 or newer for XWR-3150
> >>>> and uploading firmware using "Firmware Update" web UI page.
> >>>>
> >>>> Signed-off-by: Dan Haab <dan.haab at legrand.com>
> >>>> ---
> >>>> .../bcm53xx/base-files/etc/board.d/02_network | 22
> >>>> ++++++++++++++++++-
> >>>> target/linux/bcm53xx/image/Makefile | 18 +++++++++++++++
> >>>> 2 files changed, 39 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/target/linux/bcm53xx/base-files/etc/board.d/02_network
> >>>> b/target/linux/bcm53xx/base-files/etc/board.d/02_network
> >>>> index f86f12407f..9256cbdc54 100755
> >>>> --- a/target/linux/bcm53xx/base-files/etc/board.d/02_network
> >>>> +++ b/target/linux/bcm53xx/base-files/etc/board.d/02_network
> >>>> @@ -36,6 +36,15 @@ bcm53xx_setup_interfaces()
> >>>> ucidef_add_switch "switch0" \
> >>>> "0:wan" "1:lan:4" "2:lan:3" "3:lan:2" "4:lan:1"
> >>>> "5 at eth0"
> >>>> ;;
> >>>> + luxul,xap-1610-v1)
> >>>> + ucidef_add_switch "switch0" \
> >>>> + "0:lan" "1:lan" "5 at eth0"
> >>>> + ucidef_set_interface_lan "eth0.1" "dhcp"
> >>>> + ;;
> >>>> + luxul,xwr-3150-v1)
> >>>> + ucidef_add_switch "switch0" \
> >>>> + "0:lan:1" "1:lan:2" "2:lan:3" "3:lan:4" "4:wan"
> >>>> "5 at eth0"
> >>>> + ;;
> >>>> phicomm,k3)
> >>>> ucidef_add_switch "switch0" \
> >>>> "0:lan" "1:lan" "2:lan" "3:wan" "5 at eth0"
> >>>> @@ -100,7 +109,18 @@ bcm53xx_setup_macs()
> >>>> esac
> >>>>
> >>>> # If WAN MAC isn't explicitly set, calculate it using base MAC as
> >>>> reference.
> >>>> - [ -z "$wan_macaddr" -a -n "$etXmacaddr" ] &&
> >>>> wan_macaddr=$(macaddr_add "$etXmacaddr" 1)
> >>>> + [ -z "$wan_macaddr" -a -n "$etXmacaddr" ] && {
> >>>> + local offset=1
> >>>> +
> >>>> + case "$board" in
> >>>> + luxul,xwr-3100v1 | \
> >>>> + luxul,xwr-3150-v1)
> >>>> + offset=5
> >>>> + ;;
> >>>> + esac
> >>>> +
> >>>> + wan_macaddr=$(macaddr_add "$etXmacaddr" $offset)
> >>>> + }
> >>>
> >>> This adds another level of nesting. I'd prefer if you just added your
> >>> devices to the case directly above and just use
> >>>
> >>> [ -n "$wan_macaddr" ] || wan_macaddr=$(macaddr_add "$etXmacaddr"
> >> 5)
> >>>
> >>> for them there.
> >>
> >> We cannot do that, because the lower
> >> [ -z "$wan_macaddr" -a -n "$etXmacaddr" ] &&
> >> wan_macaddr=$(macaddr_add "$etXmacaddr" 1) will overwrite what Luxul-
> >> specific one did.
> >
> > No, it won't, because wan_macaddr won't be empty then (checked by -z)?
>
> Right, I missed that!
>
> So the only thing I slightly dislike about having:
> [ -n "$wan_macaddr" ] || wan_macaddr=$(macaddr_add "$etXmacaddr" 5)
> and
> [ -n "$wan_macaddr" ] || wan_macaddr=$(macaddr_add "$etXmacaddr" 1)
>
> is code duplication. I see it screaming: "use variable" ;)
>
>
> >> What about having offset set by device specific code? Like:
> >>
> >>
> >> etXmacaddr=$(nvram get et0macaddr)
> >> offset=1
> >> case "$board" in
> >> asus,rt-ac87u)
> >> etXmacaddr=$(nvram get et1macaddr)
> >> ;;
> >> dlink,dir-885l | \
> >> netgear,r7900 | \
> >> netgear,r8000 | \
> >> netgear,r8500)
> >> etXmacaddr=$(nvram get et2macaddr)
> >> ;;
> >> luxul,foo)
> >> offset=5
> >> ;;
> >> esac
> >
> > That's a matter of taste. I personally don't like the multi-step assignment at all, and would like to just set the wan_macaddr for every case directly as it's done in ath79/ramips. For my taste, there are too many similar variables flying around here, and I would reduce that to lan_macaddr and wan_macaddr somehow.
>
> I was trying to avoid repeating
> offset=1
> or
> wan_macaddr=$(macaddr_add "$etXmacaddr" 1)
> in 99% of switch cases. I guess that's why I have some extra variables
> in the first place - to avoid some code duplication.
>
>
> > However, if I understood your earlier comment on the tidy-up patch correctly, the et.macaddr variables are a concept somehow specific to bcm53xx, and thus my version would not be logic/desirable here.
> >
> > Thus, I leave the decision to you, as I'm not familiar with this target and mainly doing drive-by comments here.
> > Still, you solution here looks tidier than the additional nesting introduced in the initial patch.
>
> Sure & thanks for comments, it's always nice to someone's opinion.
> I like bcm53xx code much more after adding bcm53xx_setup_interfaces() &
> bcm53xx_setup_macs() - thanks!
>
>
> >>>> [ -n "$wan_macaddr" ] && ucidef_set_interface_macaddr "wan"
> >>>> "$wan_macaddr"
> >>>> }
> >>>> diff --git a/target/linux/bcm53xx/image/Makefile
> >>>> b/target/linux/bcm53xx/image/Makefile
> >>>> index 610af03abe..b3ec1e99a2 100644
> >>>> --- a/target/linux/bcm53xx/image/Makefile
> >>>> +++ b/target/linux/bcm53xx/image/Makefile
> >>>> @@ -291,6 +291,15 @@ define Device/luxul-abr-4500 endef
> >>>> TARGET_DEVICES += luxul-abr-4500
> >>>>
> >>>> +define Device/luxul-xap-1610
> >>>> + $(Device/luxul)
> >>>> + DEVICE_MODEL := XAP-1610
> >>>> + DEVICE_PACKAGES := $(BRCMFMAC_4366C0)
> >>>> + IMAGE/lxl := append-rootfs | trx-serial | luxul-lxl
> >>>> + LUXUL_BOARD := XAP-1610
> >>>> +endef
> >>>> +TARGET_DEVICES += luxul-xap-1610
> >>>> +
> >>>> define Device/luxul-xbr-4500
> >>>> $(Device/luxul)
> >>>> DEVICE_MODEL := XBR-4500
> >>>> @@ -299,6 +308,15 @@ define Device/luxul-xbr-4500 endef
> >>>> TARGET_DEVICES += luxul-xbr-4500
> >>>>
> >>>> +define Device/luxul-xwr-3150
> >>>
> >>> Could you add a -v1 here as well?
> >>
> >> I see DTS file has "v1" in its name but does v2 exist at all?
> >>
> >> If there is not v2 and it's not planned right now I'm OK with filename witohut
> >> "v1".
> >
> > If the rest of the patch is correct, the compatible has a -v1 as well (I haven't checked).
> >
> > I'm generally looking for consistency here, but on the other hand the other luxul-x... devices don't have a version suffix.
> > Though, again, this is not my playing ground, so feel free to ignore my comments from the side.
>
> It's always a problem to predict how many versions given device is
> going to have ;)
>
> We have "linksys-ea6300-v1" even though v2 was never developed /
> released.
>
> On ther other have netgear-r8000 has no v1 but later v2 has appeared.
>
> Dan: if Luxul is planning XWR-3150 v2, then plase use v1 in this patch.
> Otherwise I'm OK with skipping v1.
Luxul has no plans to create a "v2" of this device
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
More information about the openwrt-devel
mailing list