[OpenWrt-Devel] [PATCH] bcm53xx: add support for Luxul FullMAC WiFi devices

Mon Apr 6 15:39:25 EDT 2020

Hi,

> -----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)?

> 
> 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.
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.

> 
> 
> >>   	[ -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.

Best

Adrian

> 
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel at lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: openpgp-digital-signature.asc
Type: application/pgp-signature
Size: 834 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/openwrt-devel/attachments/20200406/8d9c6546/attachment.sig>
-------------- next part --------------
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel