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

Rafał Miłecki zajec5 at gmail.com
Mon Apr 6 16:03:44 EDT 2020 

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.

_______________________________________________
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