| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2FA85806-B703-4F25-B22D-758AB0BBCCE7@public-files.de>
Date: Wed, 05 Nov 2025 14:31:01 +0100
From: Frank Wunderlich <frank-w@...lic-files.de>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
Frank Wunderlich <linux@...web.de>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Linus Walleij <linus.walleij@...aro.org>,
Matthias Brugger <matthias.bgg@...il.com>
CC: Sean Wang <sean.wang@...iatek.com>, Daniel Golle <daniel@...rotopia.org>,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-gpio@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH v1 4/6] arm64: dts: mt7988: Add devicetree for BananaPi R4 Pro
Am 5. November 2025 13:55:43 MEZ schrieb AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>:
>Il 27/10/25 14:28, Frank Wunderlich ha scritto:
>> From: Frank Wunderlich <frank-w@...lic-files.de>
>>
>> Add devicetree for Bpi-R4-Pro.
>
>Thanks for this!
>
>Overall, this looks good - there are just a few small things to fix, but hey,
>I bet you're getting this upstream as early as v2 :-D
>
>>
>> BananaPi R4 Pro is a MT7988A based board which exists in 2 different
>> hardware versions:
>>
>> - 4E: 4 GB RAM and using internal 2.5G Phy for WAN-Combo
>> - 8X: 8 GB RAM and 2x Aeonsemi AS21010P 10G phys
>>
>> common parts:
>>
>> - MediaTek MT7988A Quad-core Arm Corex-A73,1.8GHz processor
>> - 8GB eMMC flash
>> - 256MB SPI-NAND Flash
>> - Micro SD card slot
>> - 1x 10G SFP+ WAN
>> - 1x 10G SFP+ LAN
>> - 4x 2.5G RJ45 LAN (MxL86252C)
>> - 1x 1G RJ45 LAN (MT7988 internal switch)
>> - 2x miniPCIe slots with PCIe3.0 2lane interface for Wi-Fi NIC
>> - 2x M.2 M-KEY slots with PCIe3.0 1lane interface for NVME SSD
>> - 3x M.2 B-KEY slots with USB3.2 for 5G Module (PCIe shared with key-m)
>> - 1x USB3.2 slot
>> - 1x USB2.0 slot
>> - 1x USB TypeC Debug Console
>> - 2x13 PIN Header for expanding application
>>
>> https://docs.banana-pi.org/en/BPI-R4_Pro/BananaPi_BPI-R4_Pro
>>
>> The PCIe is per default in key-m state and can be changed to key-b with
>> the pcie-overlays.
>>
>> Signed-off-by: Frank Wunderlich <frank-w@...lic-files.de>
>> ---
>> arch/arm64/boot/dts/mediatek/Makefile | 4 +
>> .../mt7988a-bananapi-bpi-r4-pro-4e.dts | 16 +
>> .../mt7988a-bananapi-bpi-r4-pro-8x.dts | 16 +
>> .../mediatek/mt7988a-bananapi-bpi-r4-pro.dtsi | 559 ++++++++++++++++++
>> 4 files changed, 595 insertions(+)
>> create mode 100644 arch/arm64/boot/dts/mediatek/mt7988a-bananapi-bpi-r4-pro-4e.dts
>> create mode 100644 arch/arm64/boot/dts/mediatek/mt7988a-bananapi-bpi-r4-pro-8x.dts
>> create mode 100644 arch/arm64/boot/dts/mediatek/mt7988a-bananapi-bpi-r4-pro.dtsi
>>
>> diff --git a/arch/arm64/boot/dts/mediatek/Makefile b/arch/arm64/boot/dts/mediatek/Makefile
>> index a4df4c21399e..8640e5f32d4b 100644
>> --- a/arch/arm64/boot/dts/mediatek/Makefile
>> +++ b/arch/arm64/boot/dts/mediatek/Makefile
>> @@ -24,6 +24,8 @@ dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986b-rfb.dtb
>> dtb-$(CONFIG_ARCH_MEDIATEK) += mt7988a-bananapi-bpi-r4.dtb
>> dtb-$(CONFIG_ARCH_MEDIATEK) += mt7988a-bananapi-bpi-r4-2g5.dtb
>> dtb-$(CONFIG_ARCH_MEDIATEK) += mt7988a-bananapi-bpi-r4-emmc.dtbo
>> +dtb-$(CONFIG_ARCH_MEDIATEK) += mt7988a-bananapi-bpi-r4-pro-4e.dtb
>> +dtb-$(CONFIG_ARCH_MEDIATEK) += mt7988a-bananapi-bpi-r4-pro-8x.dtb
>> dtb-$(CONFIG_ARCH_MEDIATEK) += mt7988a-bananapi-bpi-r4-sd.dtbo
>> dtb-$(CONFIG_ARCH_MEDIATEK) += mt8167-pumpkin.dtb
>> dtb-$(CONFIG_ARCH_MEDIATEK) += mt8173-elm.dtb
>> @@ -111,4 +113,6 @@ DTC_FLAGS_mt7986a-bananapi-bpi-r3 := -@
>> DTC_FLAGS_mt7986a-bananapi-bpi-r3-mini := -@
>> DTC_FLAGS_mt7988a-bananapi-bpi-r4 := -@
>> DTC_FLAGS_mt7988a-bananapi-bpi-r4-2g5 := -@
>> +DTC_FLAGS_mt7988a-bananapi-bpi-r4-pro-4e := -@
>> +DTC_FLAGS_mt7988a-bananapi-bpi-r4-pro-8x := -@
>> DTC_FLAGS_mt8395-radxa-nio-12l := -@
>> diff --git a/arch/arm64/boot/dts/mediatek/mt7988a-bananapi-bpi-r4-pro-4e.dts b/arch/arm64/boot/dts/mediatek/mt7988a-bananapi-bpi-r4-pro-4e.dts
>> new file mode 100644
>> index 000000000000..c7ea6e88c4f4
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/mediatek/mt7988a-bananapi-bpi-r4-pro-4e.dts
>> @@ -0,0 +1,16 @@
>> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
>> +/*
>> + * Copyright (C) 2025 MediaTek Inc.
>> + * Author: Frank Wunderlich <frank-w@...lic-files.de>
>> + */
>> +
>> +/dts-v1/;
>> +
>> +#include "mt7988a-bananapi-bpi-r4-pro.dtsi"
>> +
>> +/ {
>> + model = "Bananapi BPI-R4";
>> + compatible = "bananapi,bpi-r4-pro-4e",
>> + "bananapi,bpi-r4-pro",
>> + "mediatek,mt7988a";
>> +};
>> diff --git a/arch/arm64/boot/dts/mediatek/mt7988a-bananapi-bpi-r4-pro-8x.dts b/arch/arm64/boot/dts/mediatek/mt7988a-bananapi-bpi-r4-pro-8x.dts
>> new file mode 100644
>> index 000000000000..c9a0e69e9dd5
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/mediatek/mt7988a-bananapi-bpi-r4-pro-8x.dts
>> @@ -0,0 +1,16 @@
>> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
>> +/*
>> + * Copyright (C) 2025 MediaTek Inc.
>> + * Author: Frank Wunderlich <frank-w@...lic-files.de>
>> + */
>> +
>> +/dts-v1/;
>> +
>> +#include "mt7988a-bananapi-bpi-r4-pro.dtsi"
>> +
>> +/ {
>> + model = "Bananapi BPI-R4";
>> + compatible = "bananapi,bpi-r4-pro-8x",
>> + "bananapi,bpi-r4-pro",
>> + "mediatek,mt7988a";
>> +};
>> diff --git a/arch/arm64/boot/dts/mediatek/mt7988a-bananapi-bpi-r4-pro.dtsi b/arch/arm64/boot/dts/mediatek/mt7988a-bananapi-bpi-r4-pro.dtsi
>> new file mode 100644
>> index 000000000000..44406e23c042
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/mediatek/mt7988a-bananapi-bpi-r4-pro.dtsi
>> @@ -0,0 +1,559 @@
>> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
>> +/*
>> + * Copyright (C) 2025 MediaTek Inc.
>> + * Author: Sam.Shih <sam.shih@...iatek.com>
>> + * Author: Frank Wunderlich <frank-w@...lic-files.de>
>> + */
>> +
>> +/dts-v1/;
>> +
>> +#include "mt7988a.dtsi"
>> +#include <dt-bindings/gpio/gpio.h>
>> +#include <dt-bindings/input/input.h>
>> +#include <dt-bindings/leds/common.h>
>> +#include <dt-bindings/regulator/richtek,rt5190a-regulator.h>
>> +
>> +/*
>> + * ----------------------------------- Bananapi Bpi R4 Pro PINs ---------------------------------
>> + * | | Function | Function | |
>> + * |----|----------------------------------------|-----------------------------------------|----|
>> + * | 1 | 3V3 | 5V | 2 |
>> + * | 3 | GPIO18/I2C_1_SDA | 5V | 4 |
>> + * | 5 | GPIO17/I2C_1_SCL | GND | 6 |
>> + * | 7 | GPIO62/JTAG_JTRST_N/PWM6/I2P5G_LED1_P0 | GPIO59/JTAG_JTDO/UART2_TX/GBE_LED1_P1 | 8 |
>> + * | 9 | GND | GPIO58/JTAG_JTDI/UART2_RX/GBE_LED1_P0 | 10 |
>> + * | 11 | GPIO81/UART1_TXD | GPIO51/PCM_CLK_I2S_BCLK | 12 |
>> + * | 13 | GPIO80/UART1_RXD | GND | 14 |
>> + * | 15 | GPIO50/PCM_FS_I2S_LRCK | GPIO61/JTAG_JTCLK/UART2_RTS/GBE_LED1_P3 | 16 |
>> + * | 17 | 3v3 | GPIO60/JTAG_JTMS/UART2_CTS/GBE_LED1_P2 | 18 |
>> + * | 19 | GPIO30/SPI1_MOSI | GND | 20 |
>> + * | 21 | GPIO29/SPI1_MISO | GPIO53/PCM_DTX_I2S_DOUT | 22 |
>> + * | 23 | GPIO31/SPI1_CLK | GPIO28/SPI1_CSB | 24 |
>> + * | 25 | GND | GPIO52/PCM_DRX_I2S_DIN | 26 |
>> + * |----|----------------------------------------|-----------------------------------------|----|
>> + */
>
>Nice! Is this a "raspberry pi header" thingy?
>
>Let's say that the header is called "CON89" on the schematics (if unsure, you
>should find that name by looking at the PCB), so a better title would be:
>
>* ------------------------------------- CON89 Pi header pins -----------------------------------
>
>Also this relates to PIO, right? You could move that as a pio node comment,
>either before or inside of your PIO overrides.
>After all, this comment is mapping "header pin number" to "PIO pin number" ;-)
I think i drop it on v2 as the "raspberry like" gpio header is not yet adressed here. Current dts is only first part.
>> +
>> +/ {
>> + aliases {
>> + ethernet0 = &gmac0;
>> + i2c0 = &i2c0;
>> + i2c1 = &i2c1;
>> + i2c2 = &i2c2;
>> + /* PCA9548 (0-0070) provides 4 i2c channels */
>> + i2c3 = &imux0;
>> + i2c4 = &imux1_sfp1;
>> + i2c5 = &imux2_sfp2;
>> + i2c6 = &imux3_wifi;
>> + };
>> +
>> + chosen {
>> + stdout-path = &serial0;
>> + bootargs = "console=ttyS0,115200n1 \
>> + earlycon=uart8250,mmio32,0x11000000";
>
>No, don't use bootargs - besides, earlycon is supposed to be for development
>purposes only, and it *has to* be unnecessary after development, on upstreamed
>devicetrees.
Yes that's already changed in my tree based on comment from krzysztof
>> + };
>> +
>> + fan: pwm-fan {
>> + compatible = "pwm-fan";
>> + /* cooling level (0, 1, 2, 3) : (0% duty, 30% duty, 50% duty, 100% duty) */
>> + cooling-levels = <0 80 128 255>;
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&pwm0_pins>;
>> + pwms = <&pwm 0 50000>;
>> + #cooling-cells = <2>;
>> + };
>> +
>> + gpio-keys {
>> + compatible = "gpio-keys";
>> +
>> + button-reset {
>> + label = "reset";
>> + linux,code = <KEY_RESTART>;
>> + gpios = <&pio 13 GPIO_ACTIVE_LOW>;
>
>Please treat the linux,code property as a vendor property. Means...
>
>label =
>gpios =
>linux,code =
Ok
>> + };
>> +
>> + button-wps {
>> + label = "WPS";
>> + linux,code = <KEY_WPS_BUTTON>;
>> + gpios = <&pio 14 GPIO_ACTIVE_LOW>;
>> + };
>> + };
>> +
>> + gpio-leds {
>> + compatible = "gpio-leds";
>> +
>> + led_red: sys-led-red {
>> + color = <LED_COLOR_ID_RED>;
>> + gpios = <&pca9555 15 GPIO_ACTIVE_HIGH>;
>> + default-state = "on";
>> + };
>> +
>> + led_blue: sys-led-blue {
>> + color = <LED_COLOR_ID_BLUE>;
>> + gpios = <&pca9555 14 GPIO_ACTIVE_HIGH>;
>> + default-state = "on";
>> + };
>> + };
>> +
>> + reg_1p8v: regulator-1p8v {
>> + compatible = "regulator-fixed";
>> + regulator-name = "fixed-1.8V";
>
>Have you got the board schematics?
>
>If so, can you please change the regulator-name to the one from the schematics?
Yes,have to look for it.
>> + regulator-min-microvolt = <1800000>;
>> + regulator-max-microvolt = <1800000>;
>> + regulator-boot-on;
>> + regulator-always-on;
>> + };
>> +
>> + reg_3p3v: regulator-3p3v {
>> + compatible = "regulator-fixed";
>> + regulator-name = "fixed-3.3V";
>> + regulator-min-microvolt = <3300000>;
>> + regulator-max-microvolt = <3300000>;
>> + regulator-boot-on;
>> + regulator-always-on;
>> + };
>> +
>> + /* SFP1 cage (LAN) */
>> + sfp1: sfp1 {
>> + compatible = "sff,sfp";
>> + i2c-bus = <&imux1_sfp1>;
>> + los-gpios = <&pio 70 GPIO_ACTIVE_HIGH>;//change4
>> + mod-def0-gpios = <&pio 69 GPIO_ACTIVE_LOW>; //change3
>> + tx-disable-gpios = <&pio 21 GPIO_ACTIVE_HIGH>; //change1
>
>What does "change4", "change3", "change1" even mean?
>
>Please remove.
Sorry that was my comments from changes between v0 and the v1 now.
>> + maximum-power-milliwatt = <3000>;
>> + };
>> +
>> + /* SFP2 cage (WAN) */
>> + sfp2: sfp2 {
>> + compatible = "sff,sfp";
>> + i2c-bus = <&imux2_sfp2>;
>> + los-gpios = <&pio 2 GPIO_ACTIVE_HIGH>;
>> + mod-def0-gpios = <&pio 1 GPIO_ACTIVE_LOW>;
>> + tx-disable-gpios = <&pio 0 GPIO_ACTIVE_HIGH>;
>> + maximum-power-milliwatt = <3000>;
>> + };
>> +};
>> +
>
>..snip..
>
>> +
>> +&gmac0 {
>> + status = "okay";
>> +};
>> +
>> +&gsw_phy0 {
>> + pinctrl-names = "gbe-led";
>> + pinctrl-0 = <&gbe0_led0_pins>;
>> +};
>> +
>> +&gsw_phy0_led0 {
>> + status = "okay";
>> + color = <LED_COLOR_ID_YELLOW>;
>
>color first, status last
Ok
>> +};
>> +
>
>..snip..
>
>> +
>> +&i2c1 {
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&i2c1_pins>;
>> + status = "okay";
>> +};
>> +
>> +&i2c2 {
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&i2c2_1_pins>;
>> + status = "okay";
>> +
>> + pca9545: i2c-mux@70 {
>> + reg = <0x70>;
>> + compatible = "nxp,pca9545";
>
>compatible goes first. Check dts-coding-style.rst.
Ok,missed this
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + imux0: i2c@0 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + reg = <0>;
>
>reg goes first.
Ok
>> +
>> + pca9555: i2c-gpio-expander@20 {
>> + compatible = "nxp,pca9555";
>
>reg after compatible please
Ok
>> + gpio-controller;
>> + #gpio-cells = <2>;
>> + reg = <0x20>;
>> + };
>> +
>> + rtc@51 {
>> + compatible = "nxp,pcf8563";
>> + reg = <0x51>;
>> + };
>> +
>> + eeprom@57 {
>> + compatible = "atmel,24c02";
>> + reg = <0x57>;
>> + address-width = <8>;
>> + pagesize = <8>;
>> + size = <256>;
>> + };
>> + };
>> +
>> + imux1_sfp1: i2c@1 {
>
>reg first, cells later;
>reg = <1>;
>#address-cells = <1>;
>#size-cells = <0>;
>
>here and everywhere else.
I do
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + reg = <1>;
>> + };
>> +
>> + imux2_sfp2: i2c@2 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + reg = <2>;
>> + };
>> +
>> + imux3_wifi: i2c@3 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + reg = <3>;
>> + };
>> + };
>> +};
>> +
>> +&int_2p5g_phy {
>> + status = "disabled";
>> +};
>> +
>> +/* mPCIe SIM2 (11300000) */
>> +&pcie0 {
>> + status = "okay";
>> +};
>> +
>> +/* mPCIe (11310000 near leds) SIM3 */
>> +&pcie1 {
>> + status = "okay";
>> +};
>> +
>> +/* M.2 (11280000) 1L0 key-m SSD1 CN13 / key-b SIM1 CN15 */
>> +&pcie2 {
>> + status = "okay";
>> +};
>> +
>> +/* M.2 (11290000) 1L1 key-m SSD2 CN14 / key-b SIM2 CN18 */
>> +&pcie3 {
>> + status = "okay";
>> +};
>> +
>> +&pio {
>
>--snip..
>
>> +
>> + mmc0_pins_sdcard: mmc0-sdcard-pins {
>> + mux {
>> + function = "flash";
>> + groups = "sdcard";
>> + };
>> + };
>> +
>> + // 1L0 0=key-b (CN15), 1=key-m (CN13)
>
>Please fix the comments
>/* 1L0 0=key-b (CN15), 1=key-m (CN13) */
>
>> + pcie-2-hog {
>> + gpio-hog;
>> + gpios = <79 GPIO_ACTIVE_HIGH>;
>> + output-high;
>> + };
>
>Extra space here please, and
>
>> + // 1L1 0=key-b (CN18), 1=key-m (CN14)
>
>/* 1L1 0=key-b (CN18), 1=key-m (CN14) */
Ok
>> + pcie-3-hog {
>> + gpio-hog;
>> + gpios = <63 GPIO_ACTIVE_HIGH>;
>> + output-high;
>> + };
>> +
>> + pwm0_pins: pwm0-pins {
>> + mux {
>> + groups = "pwm0";
>> + function = "pwm";
>> + };
>> + };
>> +
>> + spi0_flash_pins: spi0-flash-pins {
>> + mux {
>> + function = "spi";
>> + groups = "spi0", "spi0_wp_hold";
>> + };
>> + };
>> +};
>> +
>
>..snip..
>
>> +
>> +/* back USB */
>> +&ssusb0 {
>> + /* Use U2P only instead of both U3P/U2P due to U3P serdes shared with pcie2 */
>> + phys = <&xphyu2port0 PHY_TYPE_USB2>;
>> + mediatek,u3p-dis-msk = <1>;
>> + /delete-property/ mediatek,p0_speed_fixup;
>
>Where's that property defined, even?
>Am I blind, or it's nowhere to be found (in any dtsi)?
>
>(no, seriously, I'm not sarcastic :P)
Possibly forgot to drop while adding node from sdk dts :p
>> + status = "okay";
>> +};
>> +
>
>Cheers,
>Angelo
Hi Angelo
Thanks for review
regards Frank
Powered by blists - more mailing lists