diff mbox series

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

Message ID 20200406182013.86147-1-dan.haab@luxul.com
State Changes Requested
Headers show
Series [OpenWrt-Devel] bcm53xx: add support for Luxul FullMAC WiFi devices | expand

Commit Message

Rip Route April 6, 2020, 6:20 p.m. UTC 
From: Dan Haab <dan.haab@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@legrand.com>
---
 .../bcm53xx/base-files/etc/board.d/02_network | 22 ++++++++++++++++++-
 target/linux/bcm53xx/image/Makefile           | 18 +++++++++++++++
 2 files changed, 39 insertions(+), 1 deletion(-)

Comments

Adrian Schmutzler April 6, 2020, 6:26 p.m. UTC | #1 
Hi,

> -----Original Message-----
> From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org]
> On Behalf Of Dan Haab
> Sent: Montag, 6. April 2020 20:20
> To: openwrt-devel@lists.openwrt.org
> Cc: Dan Haab <dan.haab@legrand.com>
> Subject: [OpenWrt-Devel] [PATCH] bcm53xx: add support for Luxul FullMAC
> WiFi devices
> 
> From: Dan Haab <dan.haab@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@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@eth0"
>  		;;
> +	luxul,xap-1610-v1)
> +		ucidef_add_switch "switch0" \
> +			"0:lan" "1:lan" "5@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@eth0"
> +		;;
>  	phicomm,k3)
>  		ucidef_add_switch "switch0" \
>  			"0:lan" "1:lan" "2:lan" "3:wan" "5@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.

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

Best

Adrian

> +  $(Device/luxul)
> +  DEVICE_MODEL := XWR-3150
> +  DEVICE_PACKAGES := $(BRCMFMAC_4366C0) $(USB3_PACKAGES)
> +  DEVICE_DTS := bcm47094-luxul-xwr-3150-v1
> +  LUXUL_BOARD := XWR-3150
> +endef
> +TARGET_DEVICES += luxul-xwr-3150
> +
>  define Device/netgear
>    DEVICE_VENDOR := NETGEAR
>    IMAGES := chk
> --
> 2.17.1
> 
> 
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Rafał Miłecki April 6, 2020, 7:26 p.m. UTC | #2 
On 06.04.2020 20:26, mail@adrianschmutzler.de wrote:
>> -----Original Message-----
>> From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org]
>> On Behalf Of Dan Haab
>> Sent: Montag, 6. April 2020 20:20
>> To: openwrt-devel@lists.openwrt.org
>> Cc: Dan Haab <dan.haab@legrand.com>
>> Subject: [OpenWrt-Devel] [PATCH] bcm53xx: add support for Luxul FullMAC
>> WiFi devices
>>
>> From: Dan Haab <dan.haab@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@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@eth0"
>>   		;;
>> +	luxul,xap-1610-v1)
>> +		ucidef_add_switch "switch0" \
>> +			"0:lan" "1:lan" "5@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@eth0"
>> +		;;
>>   	phicomm,k3)
>>   		ucidef_add_switch "switch0" \
>>   			"0:lan" "1:lan" "2:lan" "3:wan" "5@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.

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


>>   	[ -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".
Adrian Schmutzler April 6, 2020, 7:39 p.m. UTC | #3 
Hi,

> -----Original Message-----
> From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org]
> On Behalf Of Rafal Milecki
> Sent: Montag, 6. April 2020 21:26
> To: mail@adrianschmutzler.de; 'Dan Haab' <riproute@gmail.com>; openwrt-
> devel@lists.openwrt.org
> Cc: 'Dan Haab' <dan.haab@legrand.com>
> Subject: Re: [OpenWrt-Devel] [PATCH] bcm53xx: add support for Luxul
> FullMAC WiFi devices
> 
> On 06.04.2020 20:26, mail@adrianschmutzler.de wrote:
> >> -----Original Message-----
> >> From: openwrt-devel [mailto:openwrt-devel-
> bounces@lists.openwrt.org]
> >> On Behalf Of Dan Haab
> >> Sent: Montag, 6. April 2020 20:20
> >> To: openwrt-devel@lists.openwrt.org
> >> Cc: Dan Haab <dan.haab@legrand.com>
> >> Subject: [OpenWrt-Devel] [PATCH] bcm53xx: add support for Luxul
> >> FullMAC WiFi devices
> >>
> >> From: Dan Haab <dan.haab@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@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@eth0"
> >>   		;;
> >> +	luxul,xap-1610-v1)
> >> +		ucidef_add_switch "switch0" \
> >> +			"0:lan" "1:lan" "5@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@eth0"
> >> +		;;
> >>   	phicomm,k3)
> >>   		ucidef_add_switch "switch0" \
> >>   			"0:lan" "1:lan" "2:lan" "3:wan" "5@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@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Rafał Miłecki April 6, 2020, 8:03 p.m. UTC | #4 
On 06.04.2020 21:39, mail@adrianschmutzler.de wrote:
>> -----Original Message-----
>> From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org]
>> On Behalf Of Rafal Milecki
>> Sent: Montag, 6. April 2020 21:26
>> To: mail@adrianschmutzler.de; 'Dan Haab' <riproute@gmail.com>; openwrt-
>> devel@lists.openwrt.org
>> Cc: 'Dan Haab' <dan.haab@legrand.com>
>> Subject: Re: [OpenWrt-Devel] [PATCH] bcm53xx: add support for Luxul
>> FullMAC WiFi devices
>>
>> On 06.04.2020 20:26, mail@adrianschmutzler.de wrote:
>>>> -----Original Message-----
>>>> From: openwrt-devel [mailto:openwrt-devel-
>> bounces@lists.openwrt.org]
>>>> On Behalf Of Dan Haab
>>>> Sent: Montag, 6. April 2020 20:20
>>>> To: openwrt-devel@lists.openwrt.org
>>>> Cc: Dan Haab <dan.haab@legrand.com>
>>>> Subject: [OpenWrt-Devel] [PATCH] bcm53xx: add support for Luxul
>>>> FullMAC WiFi devices
>>>>
>>>> From: Dan Haab <dan.haab@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@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@eth0"
>>>>    		;;
>>>> +	luxul,xap-1610-v1)
>>>> +		ucidef_add_switch "switch0" \
>>>> +			"0:lan" "1:lan" "5@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@eth0"
>>>> +		;;
>>>>    	phicomm,k3)
>>>>    		ucidef_add_switch "switch0" \
>>>>    			"0:lan" "1:lan" "2:lan" "3:wan" "5@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.
Rip Route April 6, 2020, 8:51 p.m. UTC | #5 
On Mon, Apr 6, 2020 at 2:03 PM Rafał Miłecki <zajec5@gmail.com> wrote:
>
> On 06.04.2020 21:39, mail@adrianschmutzler.de wrote:
> >> -----Original Message-----
> >> From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org]
> >> On Behalf Of Rafal Milecki
> >> Sent: Montag, 6. April 2020 21:26
> >> To: mail@adrianschmutzler.de; 'Dan Haab' <riproute@gmail.com>; openwrt-
> >> devel@lists.openwrt.org
> >> Cc: 'Dan Haab' <dan.haab@legrand.com>
> >> Subject: Re: [OpenWrt-Devel] [PATCH] bcm53xx: add support for Luxul
> >> FullMAC WiFi devices
> >>
> >> On 06.04.2020 20:26, mail@adrianschmutzler.de wrote:
> >>>> -----Original Message-----
> >>>> From: openwrt-devel [mailto:openwrt-devel-
> >> bounces@lists.openwrt.org]
> >>>> On Behalf Of Dan Haab
> >>>> Sent: Montag, 6. April 2020 20:20
> >>>> To: openwrt-devel@lists.openwrt.org
> >>>> Cc: Dan Haab <dan.haab@legrand.com>
> >>>> Subject: [OpenWrt-Devel] [PATCH] bcm53xx: add support for Luxul
> >>>> FullMAC WiFi devices
> >>>>
> >>>> From: Dan Haab <dan.haab@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@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@eth0"
> >>>>                    ;;
> >>>> +  luxul,xap-1610-v1)
> >>>> +          ucidef_add_switch "switch0" \
> >>>> +                  "0:lan" "1:lan" "5@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@eth0"
> >>>> +          ;;
> >>>>            phicomm,k3)
> >>>>                    ucidef_add_switch "switch0" \
> >>>>                            "0:lan" "1:lan" "2:lan" "3:wan" "5@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
diff mbox series

Patch

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@eth0"
 		;;
+	luxul,xap-1610-v1)
+		ucidef_add_switch "switch0" \
+			"0:lan" "1:lan" "5@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@eth0"
+		;;
 	phicomm,k3)
 		ucidef_add_switch "switch0" \
 			"0:lan" "1:lan" "2:lan" "3:wan" "5@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)
+	}
 
 	[ -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
+  $(Device/luxul)
+  DEVICE_MODEL := XWR-3150
+  DEVICE_PACKAGES := $(BRCMFMAC_4366C0) $(USB3_PACKAGES)
+  DEVICE_DTS := bcm47094-luxul-xwr-3150-v1
+  LUXUL_BOARD := XWR-3150
+endef
+TARGET_DEVICES += luxul-xwr-3150
+
 define Device/netgear
   DEVICE_VENDOR := NETGEAR
   IMAGES := chk