mediatek: add support for BananaPi BPi-R4 Pro 8X#21083
Conversation
github-actions
Bot
added
the
target/mediatek
andrewjlamarche
force-pushed
the
bpi-r4-pro
branch
3 times, most recently
from
212fb50 to
4ed79d2
Compare
|
Hi @andrewjlamarche is this a draft? because the build jobs are failing for missing dtso. two patches look missing... can you describe what is working and what not? Thanks for the efforts. |
|
Yes, there’s a draft indicator near the top. There is still quite a lot missing upstream to make this work. |
|
Thank you for working on openwrt support. The uci network config is wrong. Currently only the port from mt7988 internal switch is working. I have renamed it based on suggestion from @dangowrt. Mainline misses at least
So basicly in mainline it is currently no router yet because of depencies. But you have to add mmc and pcie overlays here and fix the uci config Feel free adding patch from here when keeping author information: |
@frank-w I believe you are talking about this: https://patchwork.kernel.org/project/netdevbpf/list/?series=1028841&state=%2A&archive=both |
Yes this should be base for ethmux But till this is mainline and the mux patches available you could use the downstream version (if accepted by openwrt maintainers). |
@dangowrt could this be acceptable? (using downstream stuff in openwrt temporarely) I believe that the series from maxime will come in kernel 7.0... |
|
I'd prefer the mux to be done via upstream backports of Maxime's upcoming series which should land soon. The downstream hack to have the mux support added into the ethernet and switch drivers is too ugly and may even break things on other boards... Regarding the DSA driver for the 2.5G switch it's ok to temporarily use downstream driver until upstream is ready (I'm working on that now). |
You mean labeling it |
andrewjlamarche
force-pushed
the
bpi-r4-pro
branch
from
4ed79d2 to
89e8ac5
Compare
I'm too,but as you have ports with different speeds, we have choosen this name (port is available on uboot too) and not numbering all ports lanX which would have the effect that lan0-lan3 are 2.5g,lan4 is 1G and lan5 is 10g. I would label only the 2.5g ports lanX,not sure about the combo-ports as wan is directly connected to mac which afaik cannot be renamed on upstream but the lan-combo is also a dsa port so can have a label. Imho you should check resulting image is working before starting pull request :) as the overlays are still missing i'm sure it does not (and their integration needs more work). |
andrewjlamarche
force-pushed
the
bpi-r4-pro
branch
from
89e8ac5 to
bf8e05b
Compare
andrewjlamarche
changed the title
andrewjlamarche
force-pushed
the
bpi-r4-pro
branch
4 times, most recently
from
5a2cbc4 to
7cadf44
Compare
github-actions
Bot
added
the
core packages
|
I do not see adding uboot dts. You reference it in config (with missing dash for at least emmc). Btw. You do not need variants in uboot. Please be aware of sdcard issue. |
|
@frank-w we do need variants of U-Boot because
This all looks correct in this PR. |
im aware :) as I was working on this last night, I noticed it was very late and I decided to push what I had. Will continue. |
andrewjlamarche
force-pushed
the
bpi-r4-pro
branch
6 times, most recently
from
6d4c2ff to
e7cb2e1
Compare
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed both new commits ("uboot-mediatek: add support for BananaPi BPi-R4 Pro 8X" and "mediatek: add support for BananaPi BPi-R4 Pro 8X").
Commit checks
- The "mediatek: add support for BananaPi BPi-R4 Pro 8X" commit message has a thorough Hardware specification section and an adequate Installation section, but is missing the MAC address layout section required by the project's review rules for new device additions. Please add one line per interface (LAN1-4 via MxL switch, LAN5 via MT7530, LAN6 via mxl port 13, WAN via gmac1/sfp2, plus any Wi-Fi cards) describing where each MAC comes from (factory partition + offset, U-Boot env
ethaddr/eth1addr, derived, etc.). The99_fwenv-store-ethaddr.shchange in this PR shows thatethaddrandeth1addrend up in U-Boot env, but the readers cannot tell from the commit message what is assigned to LAN vs WAN. - The commit message Hardware spec line
Switch: MT7988A built-in, MxL86252Cin the PR description disagrees withSwitch: MT7530, MxL86252Cin the commit body. Both are technically correct (MT7988A integrates an MT7530-class switch) but please make them consistent. - The
Installationsection is on the terse side: it references "instructions for BananaPi R4 for details" rather than giving the verbatim bootloader-menu entries / TFTP filename(s) needed. A stranger reading just this commit message cannot flash the device without consulting another commit. Consider expanding it (the references in the LLM review rules — commits986ca4c887f4anda2dcbd79a4— are the recommended bar).
Inline comments cover: an incomplete cherry-pick in 063 (diffstat says 2 files, body has 1), a dtsi-level bootargs that bakes UBI semantics into a multi-medium board, the OpenWrt-only patch 968 mixing openwrt,netdev-name with an upstream-bound patch slot, two stale copyright years in patch 048, and a couple of nits on package selection in filogic.mk.
CI status on head 8bc113038e is all green (kernel patches check, mediatek/filogic build, malta/be, x86/64 build, package checks, test formalities).
Generated by Claude Code
|
This PR needs a rebase? as a matter of fact line 1084 is different between ToT and this. |
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Commit checks
- 81116a5 "mediatek: add support for BananaPi BPi-R4 Pro 8X" — missing required MAC address layout section. The PR wires MAC handling through
99_fwenv-store-ethaddr.sh(useseth0/addressand deriveseth1addras+1), but the commit message does not document where each interface's MAC comes from. Per the new-device rules a stranger should be able to learn from the commit alone where LAN/WAN/2.4G/5G MACs originate.
Generated by Claude Code
| ucidef_set_led_netdev "sfp0" "sfp0" "green:sfp" "sfp0" "link tx rx" | ||
| ;; | ||
| bananapi,bpi-r4-pro-8x) | ||
| ucidef_set_led_netdev "lan5" "lan5" "mt7530-0:00:green:lan" "lan5" "link tx rx" |
There was a problem hiding this comment.
The sysfs LED name mt7530-0:00:green:lan will not match what the kernel registers. In patch 046-v6.19-...add-dts.patch &gsw_phy0_led0 is set to color = <LED_COLOR_ID_YELLOW>; and no function property, so the LED name produced by the PHY LED binding does not include green nor :lan. Either align the DTS (set color = <LED_COLOR_ID_GREEN> and function = LED_FUNCTION_LAN, like other R4 variants do) or update this entry to use the actual color/function produced by the DTS — otherwise the netdev trigger won't bind and the port LED stays inactive.
Generated by Claude Code
| + sfp = <&sfp2>; | ||
| + managed = "in-band-status"; | ||
| + phy-mode = "usxgmii"; | ||
| + openwrt,netdev-name = "wan"; |
There was a problem hiding this comment.
nit: subject/description claim this patch only adds the SFP gpio-hog, but the hunks also enable &gmac1 as the WAN port, add MXL port@13 (lan6) with sfp = <&sfp1>, and set openwrt,netdev-name = "wan". Either split this into separate patches (gmac1/wan setup, lan6/sfp1 port, gpio-hogs) or update the subject/commit message to describe the full scope so future archaeologists can find these bits.
Generated by Claude Code
There was a problem hiding this comment.
regarding this gpio-hog. Considering the 2nd series from Maxime is taking deserved time and a third series would need at least 6months to understand how to implement muxing at runtime (there are no pocs afaik). What if someone wants to use the wan phy and lan SFP? here in this pull request we are forcing everything on sfp. I understand too it is not possible to create a single overlay for each muxing combination (1 sfplan+sfpwan, 2 sfplan+phywan, 3 phylan+sfpwan and 4 phylan+phywan). Am I correct that anyone that needs a special combination will have to just flip the gpio hog to modify the dts and compile by himself the preferred version? @frank-w @andrewjlamarche @dangowrt
|
@dangowrt I put your The first one has a 5G modem and SIM. The 2nd one has 5G modem and WiFi, but I haven't yet gotten the SIM activated by T-Mobile. Working on that this week but they're incredibly slow. Oh, and both have 10G SFP+ LAN modules. Still don't see traffic on the And restarting the network...Replacing the 10G SFP+ with a 1G SFP |
|
Does the branch need to be rebased to |
|
Okay, figured it out. Still had |
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Commit checks
mediatek: add support for BananaPi BPi-R4 Pro 8X— this commit adds a new device (new.dts/.dtsiand adefine Device/bananapi_bpi-r4-pro-8xblock), so per the new-device convention the commit message should carry a MAC address layout section in addition to the Hardware specification and Installation sections it already has. There is no MAC layout described, and the DTS has nonvmem-cells/label-macfor the interfaces (MACs are seeded into U-Boot env frometh0by99_fwenv-store-ethaddr.sh). Please add a short per-interface MAC origin section (LAN/WAN/lan5/lan6/…), usingxx:xx:xxfor unit-specific bytes.- The same commit's spec block lists
Switch: MT7530, MxL86252C, but the PR body listsMT7988A built-in, MxL86252Cand the DTS uses the MT7988 built-in switch (&gsw_*), not an MT7530. Minor, but worth aligning the commit text with the actual hardware.
One inline comment on the lan5 LED definition.
Generated by Claude Code
| ucidef_set_led_netdev "sfp0" "sfp0" "green:sfp" "sfp0" "link tx rx" | ||
| ;; | ||
| bananapi,bpi-r4-pro-8x) | ||
| ucidef_set_led_netdev "lan5" "lan5" "mt7530-0:00:green:lan" "lan5" "link tx rx" |
There was a problem hiding this comment.
The LED sysfs name mt7530-0:00:green:lan likely won't resolve to the actual PHY LED. In the DTS (patch 046-...add-dts.patch), &gsw_phy0_led0 is configured with only color = <LED_COLOR_ID_YELLOW> and status = "okay" — it has no function. The Linux PHY-LED name is built as <device>:<color>:<function>, so this LED ends up named mt7530-0:00:yellow (no :lan), not mt7530-0:00:green:lan.
Sibling devices set both properties consistently and match them in 01_leds (e.g. keenetic-kn-1812 uses color = <LED_COLOR_ID_GREEN> + function = LED_FUNCTION_LAN → ...:green:lan; the bpi-r4 base also uses green + LED_FUNCTION_LAN).
Two mismatches to reconcile: the color (DTS YELLOW vs green here) and the missing function = LED_FUNCTION_LAN in &gsw_phy0_led0. Either align this string to the actual DT color and add function = LED_FUNCTION_LAN to the DT node, so the netdev trigger binds.
Generated by Claude Code
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the two commits adding BPi-R4 Pro 8X support (kernel/DTS + uboot-mediatek). The DTS patches are an upstream backport (v6.19/v7.0/v7.1) plus OpenWrt-specific glue, and the image/board.d/uboot wiring matches the bpi-r4 siblings. Both commit messages match their diffs and the new-device commit carries the required hardware-spec and installation sections. Two minor points inline (one LED-name mismatch worth confirming, one commit-message nit); no blocking issues found.
Generated by Claude Code
| ucidef_set_led_netdev "sfp0" "sfp0" "green:sfp" "sfp0" "link tx rx" | ||
| ;; | ||
| bananapi,bpi-r4-pro-8x) | ||
| ucidef_set_led_netdev "lan5" "lan5" "mt7530-0:00:green:lan" "lan5" "link tx rx" |
There was a problem hiding this comment.
This references the LED sysfs name mt7530-0:00:green:lan, but the DTS defines gsw_phy0_led0 with color = <LED_COLOR_ID_YELLOW> and no function (in 046-...-add-dts.patch).With that color/function, the kernel registers the LED asmt7530-0:00:yellow, not green:lan`, so this netdev trigger won't bind to any LED.
The sibling bpi-r4 sets the matching LED as function = LED_FUNCTION_LAN; color = <LED_COLOR_ID_GREEN>; to produce mt7530-0:00:green:lan (in 184-...-configure-switch-phys-and-leds.patch).Either align the DTSgsw_phy0_led0tocolor = <LED_COLOR_ID_GREEN>+function = LED_FUNCTION_LAN, or change this name to match the actual DTS color (mt7530-0:00:yellow`).
Generated by Claude Code
| ucidef_set_led_netdev "sfp0" "sfp0" "green:sfp" "sfp0" "link tx rx" | ||
| ;; | ||
| bananapi,bpi-r4-pro-8x) | ||
| ucidef_set_led_netdev "lan5" "lan5" "mt7530-0:00:green:lan" "lan5" "link tx rx" |
There was a problem hiding this comment.
nit: The commit message spec header lists Switch: MT7530, MxL86252C, but this board's internal switch is the MT7988A built-in switch (compatible = "mediatek,mt7988-switch" in mt7988a.dtsi), not an MT7530. The PR description body already states it correctly as "MT7988A built-in, MxL86252C". Worth aligning the commit message to avoid confusion (the mt7530-0:00 LED string here is just the kernel's legacy mdio-bus naming, not the actual switch part).
Generated by Claude Code
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Commit checks
- 81116a54 "mediatek: add support for BananaPi BPi-R4 Pro 8X" — this commit adds a new device (new
.dts/.dtsiplus a newdefine Device/bananapi_bpi-r4-pro-8xblock), so per the new-device convention the message should carry three sections; it has Hardware specification and Installation, but is missing the MAC address layout section. The board derives its MACs from the U-Boot environment (99_fwenv-store-ethaddr.shsetsethaddr/eth1addrfrometh0/eth1), so please document where each interface's MAC comes from (one line per interface).
Note: the device-tree patches 046–049/063/094–097 are upstream cherry-picks (v6.19/v7.0/v7.1) so their content was not flagged for in-tree style; the inline notes are on the OpenWrt-specific glue.
Generated by Claude Code
| ucidef_set_led_netdev "sfp0" "sfp0" "green:sfp" "sfp0" "link tx rx" | ||
| ;; | ||
| bananapi,bpi-r4-pro-8x) | ||
| ucidef_set_led_netdev "lan5" "lan5" "mt7530-0:00:green:lan" "lan5" "link tx rx" |
There was a problem hiding this comment.
The LED name here uses green, but the DTS sets this PHY LED to a different color. In 046-...add-dts.patch, &gsw_phy0_led0is defined ascolor = <LED_COLOR_ID_YELLOW>(with nofunction), so the sysfs LED node will carry yellow, not green. By contrast the existing bpi-r4 sets that LED to GREEN and matches it with mt7530-0:00:green:wan. As written, this ucidef_set_led_netdev` line will reference a non-existent LED and the trigger won't be applied. Please align the color (and the function suffix) with the name the DTS actually produces.
Generated by Claude Code
| endef | ||
|
|
||
| define Device/bananapi_bpi-r4-pro-8x | ||
| DEVICE_MODEL := BPi-R4 Pro 8X |
There was a problem hiding this comment.
nit: the commit message's Specification block lists Switch: MT7530, MxL86252C, but the PR description lists Switch: MT7988A built-in, MxL86252C. The MT7988A integrates its own GbE switch (it does not use a discrete MT7530, which is the MT7986/R3-era part). The two descriptions disagree on the same fact; please make them consistent (the PR-description wording looks correct).
Generated by Claude Code
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Commit checks
-
81116a5 "mediatek: add support for BananaPi BPi-R4 Pro 8X" — the spec line
Switch: MT7530, MxL86252Clooks wrong. This MT7988A board has no MT7530; it uses the SoC's built-in switch (&gsw_port0/&gsw_phy0in the dtsi) for the 1G port plus the MxL86252C for the four 2.5G ports. The PR description itself lists this asSwitch: MT7988A built-in, MxL86252C. Please align the commit message with the hardware (and the PR body). -
81116a5 "mediatek: add support for BananaPi BPi-R4 Pro 8X" — this commit adds a new device (new
.dts/.dtsiundertarget/linux/mediatek/.../dts/plus a newdefine Device/bananapi_bpi-r4-pro-8xinimage/filogic.mk), so per the project's new-device commit-message convention it should carry a MAC address layout section (one line per interface — LAN/WAN/SFP/... — stating where each MAC is derived from, usingxx:xx:xxfor the unit-specific bytes). The message currently has Hardware specification and an Installation/flash section but no MAC layout. The99_fwenv-store-ethaddr.shhook this PR touches suggests MACs come from the U-Boot environment (ethaddr/eth1addr); documenting the per-interface derivation would complete the section.
Generated by Claude Code
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the 2 commits at head 81116a5. The DTS additions are upstream-tagged backports (v6.19/v7.0/v7.1) plus OpenWrt-specific switch/SFP/FIT patches, and the board.d / platform.sh / uboot-envtools wiring is consistent with the sibling BPi-R4 entries. Two inline notes below (one missing required commit-message section, one hardware-spec nit); no functional issues found in the diff.
Generated by Claude Code
| endef | ||
|
|
||
| define Device/bananapi_bpi-r4-pro-8x | ||
| DEVICE_MODEL := BPi-R4 Pro 8X |
There was a problem hiding this comment.
The "mediatek: add support for BananaPi BPi-R4 Pro 8X" commit message documents the hardware spec and an installation path, but is missing a MAC address layout section. For a new device, the message should describe where each interface's MAC comes from (one line per LAN/WAN/etc. — e.g. derived from eth0/eth1 sysfs and stored via 99_fwenv-store-ethaddr.sh, since there is no nvmem-cells MAC source in the DTS). Please add that section so the provenance of each MAC is documented.
Generated by Claude Code
| IMAGE/sysupgrade.itb := append-kernel | fit gzip $$(KDIR)/image-$$(firstword $$(DEVICE_DTS)).dtb external-with-rootfs | pad-rootfs | append-metadata | ||
| endef | ||
|
|
||
| define Device/bananapi_bpi-r4-pro-8x |
There was a problem hiding this comment.
nit: the commit message's hardware spec lists Switch: MT7530, MxL86252C, but the MT7988A has no MT7530 — it uses the SoC's built-in gigabit switch (the single 1G port is gsw_port0 on the internal &switch, the 2.5G ports are on the maxlinear,mxl86252 switch). The PR body correctly says "MT7988A built-in, MxL86252C". Please align the commit message with the PR body / actual hardware.
Generated by Claude Code
|
Maybe you could add the base R4Pro-4e dts from my upstream patch too? There come first users of the board |
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 2 new commits (the full PR diff, no prior bot review on record).
Commit checks
- 81116a5 "mediatek: add support for BananaPi BPi-R4 Pro 8X" — for a new-device commit the project rules require a MAC address layout section (one line per interface: where each MAC comes from). The message has "Specification" and "Installation" but no MAC section. MACs here are populated from the U-Boot env (
ethaddr/eth1addrvia99_fwenv-store-ethaddr.sh, read from eth0/eth1); please document that per interface (LAN / WAN / 2.4G / 5G, usingxx:xx:xxfor unit bytes). Minor: the "Installation" path mostly defers to "instructions for BananaPi R4 for details" — a self-contained path (recovery state + verbatim IP/menu strings) would meet the "could a stranger flash this from the message alone" bar. - 81116a5 — the commit lists
Switch: MT7530, MxL86252Cwhile the PR body listsSwitch: MT7988A built-in, MxL86252C. Both refer to the SoC's internal switch (kernel labelmt7530), so this is just a cosmetic body/commit disagreement worth aligning.
The OpenWrt integration (image recipe in target/linux/mediatek/image/filogic.mk, board.d/01_leds + 02_network, platform.sh, uboot-envtools and the uboot-mediatek Makefile) is consistent with the existing BPi-R4 family and the port topology (lan1-4 = MxL switch, lan5 = internal switch port0, lan6 = SFP1 DSA port, wan = gmac1/SFP2) is internally coherent. All overlays referenced by DEVICE_DTS_OVERLAY (emmc, sd, cn13-15, cn18) are generated. The patches-6.18/ DTS patches carry upstream provenance (v6.19/v7.1 subjects, MediaTek/Daniel Golle sign-offs), so style/design there is upstream's domain and not flagged. No code bugs found.
Generated by Claude Code
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Commit checks
-
mediatek: add support for BananaPi BPi-R4 Pro 8X(81116a5): this is the commit introducing the new device (addsmt7988a-bananapi-bpi-r4-pro-8x.dtsanddefine Device/bananapi_bpi-r4-pro-8x), so per the project's new-device rules the message needs a MAC address layout section in addition to the Specification and Installation sections it already has. The message currently has no per-interface MAC layout (LAN/WAN/2.4G/5G...). Since the board derives MACs from the U-Bootethaddr/eth1addrenv (it is added to99_fwenv-store-ethaddr.sh), a short section stating that — one line per interface, usingxx:xx:xxfor unit-specific bytes — would satisfy the requirement. -
uboot-mediatek: add support for BananaPi BPi-R4 Pro 8X(46a3fb2): message matches the changes (Makefile targets + u-boot patch for emmc/sdmmc/snand boot). No issues.
The DTS patches under patches-6.18/ carry upstream provenance (046-v6.19-... etc. via their From <sha>/Subject headers) and the 9xx patches are OpenWrt-local with proper Signed-off-by; their content was treated as upstream/local per the backport rules. No other new issues found in the OpenWrt glue (image Makefile, board.d, platform.sh, envtools) — the new Device/bananapi_bpi-r4-pro-common block follows the existing bananapi_bpi-r4-common conventions (vendor casing, un-wrapped IMAGE_SIZE, artifact pipeline).
Generated by Claude Code
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Commit checks
mediatek: add support for BananaPi BPi-R4 Pro 8X— the commit message has a Specification and an Installation section but is missing the MAC address layout section required for new-device commits (one line per interface saying where each MAC comes from). Since the U-Boot config usesCONFIG_NET_RANDOM_ETHADDR=yand99_fwenv-store-ethaddr.shpersists the generated eth0 MAC into the U-Boot env, a stranger can't tell from the message whether MACs are board-provisioned or randomly generated. Please add a short MAC-layout section documenting this.
Generated by Claude Code
| ucidef_set_led_netdev "sfp0" "sfp0" "green:sfp" "sfp0" "link tx rx" | ||
| ;; | ||
| bananapi,bpi-r4-pro-8x) | ||
| ucidef_set_led_netdev "lan5" "lan5" "mt7530-0:00:green:lan" "lan5" "link tx rx" |
There was a problem hiding this comment.
The LED name mt7530-0:00:green:lan will not match the LED this board actually registers. The DTS for this device (patch 046-v6.19-...add-dts.patch) configures the port-0 PHY LED as:
&gsw_phy0_led0 {
color = <LED_COLOR_ID_YELLOW>;
status = "okay";
};
i.e. color yellow and no function. The kernel builds the LED class device name from devicename + color (+ function), so it will be mt7530-0:00:yellow, not mt7530-0:00:green:lan. The netdev trigger configured here will therefore never bind.
Compare the in-tree bananapi,bpi-r4 entry which uses mt7530-0:00:green:wan because its DTS sets gsw_phy0_led0 { color = <LED_COLOR_ID_GREEN>; function = LED_FUNCTION_WAN; } (
Either align this line with the LED node's actual color/function, or add a matching function (and the intended color) to &gsw_phy0_led0 in the DTS so the resolved name matches.
| ucidef_set_led_netdev "lan5" "lan5" "mt7530-0:00:green:lan" "lan5" "link tx rx" | |
| ucidef_set_led_netdev "lan5" "lan5" "mt7530-0:00:yellow" "lan5" "link tx rx" |
Generated by Claude Code
I think this would probably be best left for a separate PR, no? |
The BPi R4 Pro 8X can boot from microSD, SPI-NAND and eMMC. Add support for each storage option. Co-authored-by: Frank Wunderlich <[email protected]> Signed-off-by: Andrew LaMarche <[email protected]>
The BPi-R4 Pro 8X is a Wi-Fi 7 router board based on the MT7988A SoC. Specification ------------- - SoC: Mediatek MT7988A (4x Cortex-A73 @1.8GHz) - RAM: 8GB DDR4 - Flash: 256MB SPI-NAND, 8GB eMMC, microSD Slot - Switch: MT7530, MxL86252C - PHYs: 2x AS21010P - Ports: 9 ports, 7 usable simultaneously * 1x 1G RJ45 LAN * 4x 2.5G RJ45 LAN * 1x 10G RJ45/SFP Combo * 1x 10G RJ45/SFP Combo - Buttons: Reset, WPS - LEDs: Power - USB: On-board VIA VL822 USB3.2/USB2.0 hub * 1x USB-A 2.0 connector * 1x USB-A 3.2 connector * 3x NGFF-KEYB - PCIe: * 2x mini-PCIe slots with PCIe 3.0 2-lane interface * 2x M.2 M-Key slots with PCIe 3.0 1-lane interface (switchable PCIe signal with M.2 B-Key) - Debug: USB Type-C console port - Power: USB Type-C PD 20V or DC barrel connector Installation ------------ Uncompress *sdcard.img.gz and write to microSD card, eg. using 'dd'. Use bootloader menu on the serial console to install SPI-NAND once installed to SPI-NAND you can used the bootloader menu to install to eMMC. See instructions for BananaPi R4 for details. Co-authored-by: Frank Wunderlich <[email protected]> Signed-off-by: Andrew LaMarche <[email protected]> Signed-off-by: Daniel Golle <[email protected]>
The support itself yes,but it would be great if you can add the dts part as it was part of my original dts patch... currently prepare the other parts Edit: seems you still add dts part (thought you dropped 4e here) |
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Commit checks
-
fdd68b5 "mediatek: add support for BananaPi BPi-R4 Pro 8X" — this commit adds a new device (new
.dtsundertarget/linux/mediatek/.../dts/plus a newdefine Device/bananapi_bpi-r4-pro-8xintarget/linux/mediatek/image/filogic.mk), so per project convention the commit message needs Hardware specification, Flash instructions and a MAC address layout section. The first two are present, but the MAC address layout section is missing. The board derives MACs from the U-Boot env (99_fwenv-store-ethaddr.shseedsethaddr/eth1addrfrometh0), so a short per-interface line (LAN/WAN/2.5G/10G — where each MAC originates) would satisfy the requirement. -
nit: fdd68b5 — the message's hardware spec lists
Switch: MT7530, MxL86252C, but the 1G RJ45 LAN is handled by the MT7988A's built-in gigabit switch (the DTS wires it through&gsw_phy0/&gsw_port0, and the upstream DTS commit describes it as "MT7988 internal switch"), not an external MT7530. The PR body correctly saysMT7988A built-in, MxL86252C. Please reconcile the commit message with the PR body.
Generated by Claude Code
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed both commits in this new-device PR (no prior openwrt-ai review; reviewed at head fdd68b5).
Both commit messages match their diffs. The vendored kernel DTS patches (046–049, 063, 094–097) carry proper upstream commit headers and the OpenWrt-local patches (192, 967, 968) are scoped correctly. Overlay names referenced in filogic.mk (-emmc, -sd, -cn13, -cn14, -cn15, -cn18) all resolve to .dtso files created across the overlay/rework patches. The board.d / platform.sh / uboot-envtools / fwenv additions are consistent with the bpi-r4 siblings, and the U-Boot snand defconfig reusing the -emmc DT matches the existing bpi-r4 convention. One inline note on the missing MAC-address-layout section in the device commit message.
Generated by Claude Code
| endef | ||
|
|
||
| define Device/bananapi_bpi-r4-pro-8x | ||
| DEVICE_MODEL := BPi-R4 Pro 8X |
There was a problem hiding this comment.
This block (plus the new .dts/.dtsi) makes the "mediatek: add support for BananaPi BPi-R4 Pro 8X" commit a new-device commit, so its message is expected to carry a MAC address layout section in addition to the hardware spec and installation sections it already has. The message currently has no per-interface MAC source. The board derives MACs from the U-Boot env (99_fwenv-store-ethaddr.sh seeds ethaddr/eth1addr from eth0), so a short section documenting where LAN/WAN/lanN MACs come from would make the device flashable/diagnosable from the commit alone.
nit: while here, the commit-message spec lists Switch: MT7530, MxL86252C, but the PR body lists Switch: MT7988A built-in, MxL86252C. The MT7988A has an integrated switch (driven by the mt7530 driver) rather than a discrete MT7530 — worth aligning the two so they agree on the same fact.
Generated by Claude Code
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Commit checks
- fdd68b5 "mediatek: add support for BananaPi BPi-R4 Pro 8X" — new-device commit is missing the MAC address layout section. The message has hardware spec + installation, but per project convention a new-device commit should document where each interface's MAC comes from (one line per LAN/WAN/2.5G/SFP...). Here MACs are seeded from the U-Boot environment via
99_fwenv-store-ethaddr.sh, so a short note to that effect would satisfy it. (Minor: the body lists the switch as "MT7988A built-in, MxL86252C" while the commit message says "MT7530, MxL86252C" — harmless, the DTS uses the MT7530-compatible internal gsw.)
Note: the DTS patches (046/047/... carrying upstream v6.19/v7.0/v7.1 tags) are imports of accepted upstream commits targeting kernel ≥6.19; they could not be diffed against the available reference refs (linux v6.18.34 is older), so they were reviewed for OpenWrt-side integration only, not byte-for-byte against upstream.
Generated by Claude Code
| ucidef_set_led_netdev "sfp0" "sfp0" "green:sfp" "sfp0" "link tx rx" | ||
| ;; | ||
| bananapi,bpi-r4-pro-8x) | ||
| ucidef_set_led_netdev "lan5" "lan5" "mt7530-0:00:green:lan" "lan5" "link tx rx" |
There was a problem hiding this comment.
The netdev LED label here uses color green, but the matching PHY LED in the DTS (&gsw_phy0_led0) is defined as color = <LED_COLOR_ID_YELLOW>; (in 046-v6.19-arm64-dts-mediatek-mt7988a-bpi-r4-pro-add-dts.patch). The kernel builds the LED class name from <device>:<color>:<function>, so this LED should appear as mt7530-0:00:yellow:lan, not :green:. Should this be mt7530-0:00:yellow:lan so the trigger actually binds? (The existing bananapi,bpi-r4 entry works because its gsw_phy0_led0 is green.)
Generated by Claude Code
| @@ -0,0 +1,41 @@ | |||
| From e4e6f0c5a4dc238684acef079e792c81d37e3226 Mon Sep 17 00:00:00 2001 | |||
There was a problem hiding this comment.
CI ("Check Kernel patches (mediatek, filogic)") fails on this patch:
target/linux/mediatek/patches-6.18/063-v7.0-fix-incorrect-model-string-in-devicetree-for-bpi-r4-pro.patch
Kernel patches for mediatek/filogic require refresh. (run 'make target/linux/refresh' and force push this pr)
The hunk context/offsets in this patch have drifted (it sits after the other new bpi-r4-pro patches). Regenerate it with make target/linux/refresh (mediatek/filogic selected in .config) and commit the result.
Generated by Claude Code
|
Hmm. Rebased, recompiled, downloaded, sysupgraded and.... I only see 3 NIC's now: |
Check conversation at #23477 |
So we need to update the firmware… but the firmware hasn’t been released? Or cherry-pick this commit into our cloning. |
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the 2 new commits (full PR diff, first automated review).
Commit checks
mediatek: add support for BananaPi BPi-R4 Pro 8X— the Specification block listsSwitch: MT7530, MxL86252C, but this board has no MT7530: LAN1-4/lan6 come from the MaxLinear MXL86252 DSA switch (maxlinear,mxl86252) and lan5 from the MT7988A's built-in switch (&switch). The PR description itself saysMT7988A built-in, MxL86252C. Please correct the commit message to match.mediatek: add support for BananaPi BPi-R4 Pro 8X— new-device commits are expected to document the per-interface MAC address layout (one line per LAN/WAN/2.4G/5G...). This commit has Specification and Installation sections but no MAC layout. Here MACs are populated from the U-Boot env via99_fwenv-store-ethaddr.shrather than a factory partition — a one-line note to that effect would satisfy the requirement; is the omission intentional?
The remaining patch series (upstream-tagged DTS backports v6.19/v7.0/v7.1, U-Boot support, and filogic glue) is internally consistent: the four PCIe overlays (cn13/cn14/cn15/cn18) and emmc/sd overlays are all defined and wired into DEVICE_DTS_OVERLAY, and the lan1-6/wan netdev names line up with 02_network. See the inline comment for the failing CI patch-refresh check.
Generated by Claude Code
| @@ -0,0 +1,41 @@ | |||
| From e4e6f0c5a4dc238684acef079e792c81d37e3226 Mon Sep 17 00:00:00 2001 | |||
There was a problem hiding this comment.
CI "Check Kernel patches (mediatek, filogic)" fails because this patch is not refreshed:
target/linux/mediatek/patches-6.18/063-v7.0-fix-incorrect-model-string-in-devicetree-for-bpi-r4-pro.patch
Kernel patches for mediatek/filogic require refresh. (run 'make target/linux/refresh' and force push this pr)
##[error]Process completed with exit code 1.
make target/linux/refresh (with mediatek/filogic selected in .config) regenerates this patch's hunk headers/context; commit the resulting change to 063 so the patch-check passes.
Generated by Claude Code
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Commit checks
mediatek: add support for BananaPi BPi-R4 Pro 8X— new-device commit; missing the MAC address layout section (see inline note). Hardware spec and installation/flash instructions are present.- Minor: the commit message lists the integrated switch as
MT7530, MxL86252Cwhereas the PR body saysMT7988A built-in, MxL86252C. Both refer to the same MT7988 internal (mt7530-family) switch, so this is just an inconsistency between the two descriptions, not a code issue.
The remaining kernel DTS patches (046/047/048/049/063/094–097) carry upstream From:/Subject:/Signed-off-by: metadata for commits queued for v6.19/v7.0/v7.1 and are backports; their content was not flagged for upstream style. The OpenWrt-local glue (filogic.mk, uboot-mediatek/Makefile, board.d/{01_leds,02_network}, platform.sh, 99_fwenv-store-ethaddr.sh, and the 192/967/968 local patches) is consistent with the sister BPi-R4 entries; no functional issues found. CI status on the head commit is pending with no completed checks.
Generated by Claude Code
| DEVICE_DTS := mt7988a-bananapi-bpi-r4-pro-8x | ||
| DEVICE_DTS_CONFIG := config-mt7988a-bananapi-bpi-r4-pro-8x | ||
| DEVICE_BL2 := comb-4bg | ||
| $(call Device/bananapi_bpi-r4-pro-common) |
There was a problem hiding this comment.
The "mediatek: add support..." commit adds a new device (.dts + define Device/bananapi_bpi-r4-pro-8x), so its message should document where each interface's MAC address comes from. The message currently has Hardware spec and Installation, but no MAC address layout section. Since MACs here are seeded into the U-Boot env (99_fwenv-store-ethaddr.sh sets ethaddr from eth0 and eth1addr), a short per-interface note (LAN / WAN / lan5 / lan6, using xx:xx:xx for unit-specific bytes) would let someone understand the addressing without reading the boot scripts.
Generated by Claude Code
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 2 new commits.
Commit checks
mediatek: add support for BananaPi BPi-R4 Pro 8X— Two issues in the commit message:- The
Switch:line readsMT7530, MxL86252C, but there is no MT7530 on this board. The DTS uses the MT7988A's built-in switch (&gsw_port0, relabelledlan5) plus the external MaxLinearmaxlinear,mxl86252(switch@16). The PR description itself saysMT7988A built-in, MxL86252C. Please correct the commit body to match (MT7988A built-in, MxL86252C). - Per the new-device support requirements, the message is missing the MAC address layout section. Hardware specification and (terse) flash instructions are present, but there is no per-interface MAC origin documented (e.g. where LAN/WAN/lan1-6 MACs come from — factory partition + offset, derived from a label, stored via the
99_fwenv-store-ethaddr.shpath this PR extends, etc.). Add one line per interface usingxx:xx:xxfor the unit-specific bytes.
- The
The uboot-mediatek commit (uboot-mediatek: add support for BananaPi BPi-R4 Pro 8X) is consistent: the three U-Boot/...-{emmc,sdmmc,snand} defines, UBOOT_TARGETS entries, and the configs/mt7988a_bananapi_bpi-r4-pro-8x-*_defconfig files in patch 472 all line up.
Note: the kernel DTS additions (patches 046/047/048/049 and the v7.0/v7.1 fix patches) carry real upstream provenance headers and were not style-reviewed, since those land via the upstream kernel; the model-string fix (063) and mgmt→lan5 rename (094) are correctly included. The edits to existing patches 173/186/188 are context refreshes caused by inserting the new ssusb0 label and Makefile entries, not arbitrary changes.
Generated by Claude Code
20 participants
The BPi-R4 Pro 8X is a Wi-Fi 7 router board based on the MT7988A SoC.
Specification
Installation
Uncompress *sdcard.img.gz and write to microSD card, eg. using 'dd'. Use bootloader menu on the serial console to install SPI-NAND once installed to SPI-NAND you can used the bootloader menu to install to eMMC. See instructions for BananaPi R4 for details.