| 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: <017b2ebb-e6b0-40a2-a6e6-47ced8909f55@collabora.com>
Date: Wed, 5 Nov 2025 13:55:43 +0100
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: 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: Frank Wunderlich <frank-w@...lic-files.de>,
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
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" ;-)
> +
> +/ {
> + 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.
> + };
> +
> + 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 =
> + };
> +
> + 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?
> + 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.
> + 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
> +};
> +
..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.
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + imux0: i2c@0 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0>;
reg goes first.
> +
> + pca9555: i2c-gpio-expander@20 {
> + compatible = "nxp,pca9555";
reg after compatible please
> + 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.
> + #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) */
> + 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)
> + status = "okay";
> +};
> +
Cheers,
Angelo
Powered by blists - more mailing lists