diff mbox series

[RFC,v1,5/5] arm64: dts: mediatek: Add mt7986 based Bananapi R3 Mini

Message ID 20240505164549.65644-6-linux@fw-web.de (mailing list archive)
State Not Applicable, archived
Headers show
Series Add Bananapi R3 Mini | expand

Commit Message

Frank Wunderlich May 5, 2024, 4:45 p.m. UTC
From: Frank Wunderlich <frank-w@public-files.de>

Add device Tree for Bananapi R3 Mini SBC.

Co-developed-by: Eric Woudstra <ericwouds@gmail.com>
Signed-off-by: Eric Woudstra <ericwouds@gmail.com>
Co-developed-by: Tianling Shen <cnsztl@gmail.com>
Signed-off-by: Tianling Shen <cnsztl@gmail.com>
Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
 arch/arm64/boot/dts/mediatek/Makefile         |   1 +
 .../mediatek/mt7986a-bananapi-bpi-r3-mini.dts | 486 ++++++++++++++++++
 2 files changed, 487 insertions(+)
 create mode 100644 arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3-mini.dts

Comments

Krzysztof Kozlowski May 6, 2024, 8:20 a.m. UTC | #1
On 05/05/2024 18:45, Frank Wunderlich wrote:
> From: Frank Wunderlich <frank-w@public-files.de>
> 
> Add device Tree for Bananapi R3 Mini SBC.
> 
> Co-developed-by: Eric Woudstra <ericwouds@gmail.com>
> Signed-off-by: Eric Woudstra <ericwouds@gmail.com>
> Co-developed-by: Tianling Shen <cnsztl@gmail.com>
> Signed-off-by: Tianling Shen <cnsztl@gmail.com>
> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> ---
>  arch/arm64/boot/dts/mediatek/Makefile         |   1 +
>  .../mediatek/mt7986a-bananapi-bpi-r3-mini.dts | 486 ++++++++++++++++++
>  2 files changed, 487 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3-mini.dts
> 
> diff --git a/arch/arm64/boot/dts/mediatek/Makefile b/arch/arm64/boot/dts/mediatek/Makefile
> index 37b4ca3a87c9..1763b001ab06 100644
> --- a/arch/arm64/boot/dts/mediatek/Makefile
> +++ b/arch/arm64/boot/dts/mediatek/Makefile
> @@ -11,6 +11,7 @@ dtb-$(CONFIG_ARCH_MEDIATEK) += mt7622-bananapi-bpi-r64.dtb
>  dtb-$(CONFIG_ARCH_MEDIATEK) += mt7981b-xiaomi-ax3000t.dtb
>  dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-acelink-ew-7886cax.dtb
>  dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3.dtb
> +dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3-mini.dtb
>  dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3-emmc.dtbo
>  dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3-nand.dtbo
>  dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3-nor.dtbo
> diff --git a/arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3-mini.dts b/arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3-mini.dts
> new file mode 100644
> index 000000000000..c764b4dc4752
> --- /dev/null
> +++ b/arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3-mini.dts
> @@ -0,0 +1,486 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/*
> + * Copyright (C) 2021 MediaTek Inc.
> + * Authors: Frank Wunderlich <frank-w@public-files.de>
> + *          Eric Woudstra <ericwouds@gmail.com>
> + *          Tianling Shen <cnsztl@immortalwrt.org>
> + */
> +
> +/dts-v1/;
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/input/input.h>
> +#include <dt-bindings/leds/common.h>
> +#include <dt-bindings/pinctrl/mt65xx.h>
> +
> +#include "mt7986a.dtsi"
> +
> +/ {
> +	model = "Bananapi BPI-R3 Mini";
> +	chassis-type = "embedded";
> +	compatible = "bananapi,bpi-r3mini", "mediatek,mt7986a";
> +
> +	aliases {
> +		serial0 = &uart0;
> +		ethernet0 = &gmac0;
> +		ethernet1 = &gmac1;
> +	};
> +
> +	chosen {
> +		stdout-path = "serial0:115200n8";
> +	};
> +
> +	dcin: regulator-12vd {

Please use name for all fixed regulators which matches current format
recommendation: 'regulator-[0-9]+v[0-9]+'

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git/commit/?id=b6d4b3500d57370f5b3abf0701c9166b384db976

> +		compatible = "regulator-fixed";
> +		regulator-name = "12vd";
> +		regulator-min-microvolt = <12000000>;
> +		regulator-max-microvolt = <12000000>;
> +		regulator-boot-on;
> +		regulator-always-on;
> +	};
> +
> +	fan: pwm-fan {
> +		compatible = "pwm-fan";
> +		#cooling-cells = <2>;
> +		/* cooling level (0, 1, 2) - pwm inverted */
> +		cooling-levels = <255 96 0>;
> +		pwms = <&pwm 0 10000>;
> +		status = "okay";

Why? Where is it disabled?

> +	};
> +
> +	reg_1p8v: regulator-1p8v {

Please use name for all fixed regulators which matches current format
recommendation: 'regulator-[0-9]+v[0-9]+'

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git/commit/?id=b6d4b3500d57370f5b3abf0701c9166b384db976

In other places as well.



Best regards,
Krzysztof
AngeloGioacchino Del Regno May 6, 2024, 12:48 p.m. UTC | #2
Il 05/05/24 18:45, Frank Wunderlich ha scritto:
> From: Frank Wunderlich <frank-w@public-files.de>
> 
> Add device Tree for Bananapi R3 Mini SBC.
> 
> Co-developed-by: Eric Woudstra <ericwouds@gmail.com>
> Signed-off-by: Eric Woudstra <ericwouds@gmail.com>
> Co-developed-by: Tianling Shen <cnsztl@gmail.com>
> Signed-off-by: Tianling Shen <cnsztl@gmail.com>
> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> ---
>   arch/arm64/boot/dts/mediatek/Makefile         |   1 +
>   .../mediatek/mt7986a-bananapi-bpi-r3-mini.dts | 486 ++++++++++++++++++
>   2 files changed, 487 insertions(+)
>   create mode 100644 arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3-mini.dts
> 
> diff --git a/arch/arm64/boot/dts/mediatek/Makefile b/arch/arm64/boot/dts/mediatek/Makefile
> index 37b4ca3a87c9..1763b001ab06 100644
> --- a/arch/arm64/boot/dts/mediatek/Makefile
> +++ b/arch/arm64/boot/dts/mediatek/Makefile
> @@ -11,6 +11,7 @@ dtb-$(CONFIG_ARCH_MEDIATEK) += mt7622-bananapi-bpi-r64.dtb
>   dtb-$(CONFIG_ARCH_MEDIATEK) += mt7981b-xiaomi-ax3000t.dtb
>   dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-acelink-ew-7886cax.dtb
>   dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3.dtb
> +dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3-mini.dtb
>   dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3-emmc.dtbo
>   dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3-nand.dtbo
>   dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3-nor.dtbo
> diff --git a/arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3-mini.dts b/arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3-mini.dts
> new file mode 100644
> index 000000000000..c764b4dc4752
> --- /dev/null
> +++ b/arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3-mini.dts
> @@ -0,0 +1,486 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/*
> + * Copyright (C) 2021 MediaTek Inc.
> + * Authors: Frank Wunderlich <frank-w@public-files.de>
> + *          Eric Woudstra <ericwouds@gmail.com>
> + *          Tianling Shen <cnsztl@immortalwrt.org>
> + */
> +
> +/dts-v1/;
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/input/input.h>
> +#include <dt-bindings/leds/common.h>
> +#include <dt-bindings/pinctrl/mt65xx.h>
> +
> +#include "mt7986a.dtsi"
> +
> +/ {
> +	model = "Bananapi BPI-R3 Mini";
> +	chassis-type = "embedded";
> +	compatible = "bananapi,bpi-r3mini", "mediatek,mt7986a";
> +
> +	aliases {
> +		serial0 = &uart0;
> +		ethernet0 = &gmac0;
> +		ethernet1 = &gmac1;
> +	};
> +
> +	chosen {
> +		stdout-path = "serial0:115200n8";
> +	};
> +
> +	dcin: regulator-12vd {
> +		compatible = "regulator-fixed";
> +		regulator-name = "12vd";
> +		regulator-min-microvolt = <12000000>;
> +		regulator-max-microvolt = <12000000>;
> +		regulator-boot-on;
> +		regulator-always-on;
> +	};
> +
> +	fan: pwm-fan {
> +		compatible = "pwm-fan";
> +		#cooling-cells = <2>;
> +		/* cooling level (0, 1, 2) - pwm inverted */
> +		cooling-levels = <255 96 0>;

Did you try to actually invert the PWM?

Look for PWM_POLARITY_INVERTED ;-)

> +		pwms = <&pwm 0 10000>;
> +		status = "okay";
> +	};
> +
> +	reg_1p8v: regulator-1p8v {
> +		compatible = "regulator-fixed";
> +		regulator-name = "1.8vd";
> +		regulator-min-microvolt = <1800000>;
> +		regulator-max-microvolt = <1800000>;
> +		regulator-boot-on;
> +		regulator-always-on;
> +		vin-supply = <&dcin>;
> +	};
> +
> +	reg_3p3v: regulator-3p3v {
> +		compatible = "regulator-fixed";
> +		regulator-name = "3.3vd";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		regulator-boot-on;
> +		regulator-always-on;
> +		vin-supply = <&dcin>;
> +	};
> +
> +	usb_vbus: regulator-usb-vbus {
> +		compatible = "regulator-fixed";
> +		regulator-name = "usb_vbus";
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +		gpios = <&pio 20 GPIO_ACTIVE_LOW>;
> +		regulator-boot-on;
> +	};
> +
> +	en8811_a: regulator-phy1 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "phy1";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		gpio = <&pio 16 GPIO_ACTIVE_LOW>;
> +		regulator-always-on;
> +	};
> +
> +	en8811_b: regulator-phy2 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "phy2";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		gpio = <&pio 17 GPIO_ACTIVE_LOW>;
> +		regulator-always-on;
> +	};
> +
> +	leds {
> +		compatible = "gpio-leds";
> +
> +		green_led: led-0 {
> +			color = <LED_COLOR_ID_GREEN>;
> +			function = LED_FUNCTION_POWER;
> +			gpios = <&pio 19 GPIO_ACTIVE_HIGH>;
> +			default-state = "on";
> +		};
> +	};
> +
> +	gpio-keys {
> +		compatible = "gpio-keys";
> +
> +		reset-key {
> +			label = "reset";
> +			linux,code = <KEY_RESTART>;
> +			gpios = <&pio 7 GPIO_ACTIVE_LOW>;
> +		};
> +	};
> +
> +};
> +
> +&cpu_thermal {
> +	cooling-maps {
> +		map0 {
> +			/* active: set fan to cooling level 2 */
> +			cooling-device = <&fan 2 2>;
> +			trip = <&cpu_trip_active_high>;
> +		};
> +
> +		map1 {
> +			/* active: set fan to cooling level 1 */
> +			cooling-device = <&fan 1 1>;
> +			trip = <&cpu_trip_active_med>;
> +		};
> +
> +		map2 {
> +			/* active: set fan to cooling level 0 */
> +			cooling-device = <&fan 0 0>;
> +			trip = <&cpu_trip_active_low>;
> +		};
> +	};
> +};
> +
> +&crypto {
> +	status = "okay";
> +};
> +
> +&eth {
> +	status = "okay";
> +
> +	gmac0: mac@0 {
> +		compatible = "mediatek,eth-mac";
> +		reg = <0>;
> +		phy-mode = "2500base-x";
> +		phy-handle = <&phy14>;
> +	};
> +
> +	gmac1: mac@1 {
> +		compatible = "mediatek,eth-mac";
> +		reg = <1>;
> +		phy-mode = "2500base-x";
> +		phy-handle = <&phy15>;
> +	};
> +
> +	mdio: mdio-bus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +	};
> +};
> +
> +&mmc0 {
> +	pinctrl-names = "default", "state_uhs";
> +	pinctrl-0 = <&mmc0_pins_default>;
> +	pinctrl-1 = <&mmc0_pins_uhs>;
> +	vmmc-supply = <&reg_3p3v>;
> +	vqmmc-supply = <&reg_1p8v>;
> +};
> +
> +
> +&i2c0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&i2c_pins>;
> +	status = "okay";
> +
> +	/* MAC Address EEPROM */
> +	eeprom@50 {
> +		compatible = "atmel,24c02";
> +		reg = <0x50>;
> +
> +		address-width = <8>;
> +		pagesize = <8>;
> +		size = <256>;
> +	};
> +};
> +
> +&mdio {

You can just move all this stuff to where you declare the mdio-bus....

> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	phy14: ethernet-phy@14 {

I say that this is `phy0: ethernet-phy@14` - because this is the first PHY on this
board.

> +		reg = <14>;

Uhm.. doesn't this require the ethernet-phy-id03a2.a411 compatible?

> +		interrupts-extended = <&pio 48 IRQ_TYPE_EDGE_FALLING>;
> +		reset-gpios = <&pio 49 GPIO_ACTIVE_LOW>;
> +		reset-assert-us = <10000>;
> +		reset-deassert-us = <20000>;
> +		phy-mode = "2500base-x";
> +		full-duplex;
> +		pause;
> +		airoha,pnswap-rx;
> +
> +		leds {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			led@0 { /* en8811_a_gpio5 */
> +				reg = <0>;
> +				color = <LED_COLOR_ID_YELLOW>;
> +				function = LED_FUNCTION_LAN;
> +				function-enumerator = <1>;

Why aren't you simply using a label?

> +				default-state = "keep";
> +				linux,default-trigger = "netdev";
> +			};
> +			led@1 { /* en8811_a_gpio4 */
> +				reg = <1>;
> +				color = <LED_COLOR_ID_GREEN>;
> +				function = LED_FUNCTION_LAN;
> +				function-enumerator = <2>;
> +				default-state = "keep";
> +				linux,default-trigger = "netdev";
> +			};
> +		};
> +	};
> +
> +	phy15: ethernet-phy@15 {
> +		reg = <15>;

Same here.

Cheers,
Angelo
Frank Wunderlich May 6, 2024, 4 p.m. UTC | #3
Hi

Thanks for review.

Am 6. Mai 2024 14:48:59 MESZ schrieb AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>:
>Il 05/05/24 18:45, Frank Wunderlich ha scritto:
>> From: Frank Wunderlich <frank-w@public-files.de>
>> 
>> Add device Tree for Bananapi R3 Mini SBC.
>> 
>> Co-developed-by: Eric Woudstra <ericwouds@gmail.com>
>> Signed-off-by: Eric Woudstra <ericwouds@gmail.com>
>> Co-developed-by: Tianling Shen <cnsztl@gmail.com>
>> Signed-off-by: Tianling Shen <cnsztl@gmail.com>
>> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
>> ---
>>   arch/arm64/boot/dts/mediatek/Makefile         |   1 +
>>   .../mediatek/mt7986a-bananapi-bpi-r3-mini.dts | 486 ++++++++++++++++++
>>   2 files changed, 487 insertions(+)
>>   create mode 100644 arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3-mini.dts
>> 
>> diff --git a/arch/arm64/boot/dts/mediatek/Makefile b/arch/arm64/boot/dts/mediatek/Makefile
>> index 37b4ca3a87c9..1763b001ab06 100644
>> --- a/arch/arm64/boot/dts/mediatek/Makefile
>> +++ b/arch/arm64/boot/dts/mediatek/Makefile
>> @@ -11,6 +11,7 @@ dtb-$(CONFIG_ARCH_MEDIATEK) += mt7622-bananapi-bpi-r64.dtb
>>   dtb-$(CONFIG_ARCH_MEDIATEK) += mt7981b-xiaomi-ax3000t.dtb
>>   dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-acelink-ew-7886cax.dtb
>>   dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3.dtb
>> +dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3-mini.dtb
>>   dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3-emmc.dtbo
>>   dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3-nand.dtbo
>>   dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3-nor.dtbo
>> diff --git a/arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3-mini.dts b/arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3-mini.dts
>> new file mode 100644
>> index 000000000000..c764b4dc4752
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3-mini.dts
>> @@ -0,0 +1,486 @@
>> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
>> +/*
>> + * Copyright (C) 2021 MediaTek Inc.
>> + * Authors: Frank Wunderlich <frank-w@public-files.de>
>> + *          Eric Woudstra <ericwouds@gmail.com>
>> + *          Tianling Shen <cnsztl@immortalwrt.org>
>> + */
>> +
>> +/dts-v1/;
>> +
>> +#include <dt-bindings/gpio/gpio.h>
>> +#include <dt-bindings/input/input.h>
>> +#include <dt-bindings/leds/common.h>
>> +#include <dt-bindings/pinctrl/mt65xx.h>
>> +
>> +#include "mt7986a.dtsi"
>> +
>> +/ {
>> +	model = "Bananapi BPI-R3 Mini";
>> +	chassis-type = "embedded";
>> +	compatible = "bananapi,bpi-r3mini", "mediatek,mt7986a";
>> +
>> +	aliases {
>> +		serial0 = &uart0;
>> +		ethernet0 = &gmac0;
>> +		ethernet1 = &gmac1;
>> +	};
>> +
>> +	chosen {
>> +		stdout-path = "serial0:115200n8";
>> +	};
>> +
>> +	dcin: regulator-12vd {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "12vd";
>> +		regulator-min-microvolt = <12000000>;
>> +		regulator-max-microvolt = <12000000>;
>> +		regulator-boot-on;
>> +		regulator-always-on;
>> +	};
>> +
>> +	fan: pwm-fan {
>> +		compatible = "pwm-fan";
>> +		#cooling-cells = <2>;
>> +		/* cooling level (0, 1, 2) - pwm inverted */
>> +		cooling-levels = <255 96 0>;
>
>Did you try to actually invert the PWM?
>
>Look for PWM_POLARITY_INVERTED ;-)

Mtk pwm driver does not support it

https://elixir.bootlin.com/linux/latest/source/drivers/pwm/pwm-mediatek.c#L211

>> +		pwms = <&pwm 0 10000>;
>> +		status = "okay";
>> +	};
>> +
>> +	reg_1p8v: regulator-1p8v {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "1.8vd";
>> +		regulator-min-microvolt = <1800000>;
>> +		regulator-max-microvolt = <1800000>;
>> +		regulator-boot-on;
>> +		regulator-always-on;
>> +		vin-supply = <&dcin>;
>> +	};
>> +
>> +	reg_3p3v: regulator-3p3v {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "3.3vd";
>> +		regulator-min-microvolt = <3300000>;
>> +		regulator-max-microvolt = <3300000>;
>> +		regulator-boot-on;
>> +		regulator-always-on;
>> +		vin-supply = <&dcin>;
>> +	};
>> +
>> +	usb_vbus: regulator-usb-vbus {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "usb_vbus";
>> +		regulator-min-microvolt = <5000000>;
>> +		regulator-max-microvolt = <5000000>;
>> +		gpios = <&pio 20 GPIO_ACTIVE_LOW>;
>> +		regulator-boot-on;
>> +	};
>> +
>> +	en8811_a: regulator-phy1 {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "phy1";
>> +		regulator-min-microvolt = <3300000>;
>> +		regulator-max-microvolt = <3300000>;
>> +		gpio = <&pio 16 GPIO_ACTIVE_LOW>;
>> +		regulator-always-on;
>> +	};
>> +
>> +	en8811_b: regulator-phy2 {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "phy2";
>> +		regulator-min-microvolt = <3300000>;
>> +		regulator-max-microvolt = <3300000>;
>> +		gpio = <&pio 17 GPIO_ACTIVE_LOW>;
>> +		regulator-always-on;
>> +	};
>> +
>> +	leds {
>> +		compatible = "gpio-leds";
>> +
>> +		green_led: led-0 {
>> +			color = <LED_COLOR_ID_GREEN>;
>> +			function = LED_FUNCTION_POWER;
>> +			gpios = <&pio 19 GPIO_ACTIVE_HIGH>;
>> +			default-state = "on";
>> +		};
>> +	};
>> +
>> +	gpio-keys {
>> +		compatible = "gpio-keys";
>> +
>> +		reset-key {
>> +			label = "reset";
>> +			linux,code = <KEY_RESTART>;
>> +			gpios = <&pio 7 GPIO_ACTIVE_LOW>;
>> +		};
>> +	};
>> +
>> +};
>> +
>> +&cpu_thermal {
>> +	cooling-maps {
>> +		map0 {
>> +			/* active: set fan to cooling level 2 */
>> +			cooling-device = <&fan 2 2>;
>> +			trip = <&cpu_trip_active_high>;
>> +		};
>> +
>> +		map1 {
>> +			/* active: set fan to cooling level 1 */
>> +			cooling-device = <&fan 1 1>;
>> +			trip = <&cpu_trip_active_med>;
>> +		};
>> +
>> +		map2 {
>> +			/* active: set fan to cooling level 0 */
>> +			cooling-device = <&fan 0 0>;
>> +			trip = <&cpu_trip_active_low>;
>> +		};
>> +	};
>> +};
>> +
>> +&crypto {
>> +	status = "okay";
>> +};
>> +
>> +&eth {
>> +	status = "okay";
>> +
>> +	gmac0: mac@0 {
>> +		compatible = "mediatek,eth-mac";
>> +		reg = <0>;
>> +		phy-mode = "2500base-x";
>> +		phy-handle = <&phy14>;
>> +	};
>> +
>> +	gmac1: mac@1 {
>> +		compatible = "mediatek,eth-mac";
>> +		reg = <1>;
>> +		phy-mode = "2500base-x";
>> +		phy-handle = <&phy15>;
>> +	};
>> +
>> +	mdio: mdio-bus {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +	};
>> +};
>> +
>> +&mmc0 {
>> +	pinctrl-names = "default", "state_uhs";
>> +	pinctrl-0 = <&mmc0_pins_default>;
>> +	pinctrl-1 = <&mmc0_pins_uhs>;
>> +	vmmc-supply = <&reg_3p3v>;
>> +	vqmmc-supply = <&reg_1p8v>;
>> +};
>> +
>> +
>> +&i2c0 {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&i2c_pins>;
>> +	status = "okay";
>> +
>> +	/* MAC Address EEPROM */
>> +	eeprom@50 {
>> +		compatible = "atmel,24c02";
>> +		reg = <0x50>;
>> +
>> +		address-width = <8>;
>> +		pagesize = <8>;
>> +		size = <256>;
>> +	};
>> +};
>> +
>> +&mdio {
>
>You can just move all this stuff to where you declare the mdio-bus....

Ok,see these 2 lines are already above,so can be dropped here.

>> +	#address-cells = <1>;
>> +	#size-cells = <0>;
>> +
>> +	phy14: ethernet-phy@14 {
>
>I say that this is `phy0: ethernet-phy@14` - because this is the first PHY on this
>board.

Ok,i change this and phy15

>> +		reg = <14>;
>
>Uhm.. doesn't this require the ethernet-phy-id03a2.a411 compatible?

I can add it,but worked without it.

There was a discussion about that and result was we don't need it in board dts,maybe add compatible in binding example.

https://patchwork.kernel.org/project/netdevbpf/patch/20240206194751.1901802-2-ericwouds@gmail.com/#25703356

>> +		interrupts-extended = <&pio 48 IRQ_TYPE_EDGE_FALLING>;
>> +		reset-gpios = <&pio 49 GPIO_ACTIVE_LOW>;
>> +		reset-assert-us = <10000>;
>> +		reset-deassert-us = <20000>;
>> +		phy-mode = "2500base-x";
>> +		full-duplex;
>> +		pause;
>> +		airoha,pnswap-rx;
>> +
>> +		leds {
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +
>> +			led@0 { /* en8811_a_gpio5 */
>> +				reg = <0>;
>> +				color = <LED_COLOR_ID_YELLOW>;
>> +				function = LED_FUNCTION_LAN;
>> +				function-enumerator = <1>;
>
>Why aren't you simply using a label?

You mean the comment? I can add it of course like for regulators.

>> +				default-state = "keep";
>> +				linux,default-trigger = "netdev";
>> +			};
>> +			led@1 { /* en8811_a_gpio4 */
>> +				reg = <1>;
>> +				color = <LED_COLOR_ID_GREEN>;
>> +				function = LED_FUNCTION_LAN;
>> +				function-enumerator = <2>;
>> +				default-state = "keep";
>> +				linux,default-trigger = "netdev";
>> +			};
>> +		};
>> +	};
>> +
>> +	phy15: ethernet-phy@15 {
>> +		reg = <15>;
>
>Same here.
>
>Cheers,
>Angelo
>


regards Frank
Daniel Golle May 6, 2024, 4:12 p.m. UTC | #4
On Mon, May 06, 2024 at 06:00:30PM +0200, Frank Wunderlich wrote:
> Hi
> 
> Thanks for review.
> 
> Am 6. Mai 2024 14:48:59 MESZ schrieb AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>:
> >Il 05/05/24 18:45, Frank Wunderlich ha scritto:
> >> From: Frank Wunderlich <frank-w@public-files.de>
> >> 
> >> Add device Tree for Bananapi R3 Mini SBC.
> >> 
> >> Co-developed-by: Eric Woudstra <ericwouds@gmail.com>
> >> Signed-off-by: Eric Woudstra <ericwouds@gmail.com>
> >> Co-developed-by: Tianling Shen <cnsztl@gmail.com>
> >> Signed-off-by: Tianling Shen <cnsztl@gmail.com>
> >> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> >> ---
> >>   arch/arm64/boot/dts/mediatek/Makefile         |   1 +
> >>   .../mediatek/mt7986a-bananapi-bpi-r3-mini.dts | 486 ++++++++++++++++++
> >>   2 files changed, 487 insertions(+)
> >>   create mode 100644 arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3-mini.dts
> >> 
> >> diff --git a/arch/arm64/boot/dts/mediatek/Makefile b/arch/arm64/boot/dts/mediatek/Makefile
> >> index 37b4ca3a87c9..1763b001ab06 100644
> >> --- a/arch/arm64/boot/dts/mediatek/Makefile
> >> +++ b/arch/arm64/boot/dts/mediatek/Makefile
> >> @@ -11,6 +11,7 @@ dtb-$(CONFIG_ARCH_MEDIATEK) += mt7622-bananapi-bpi-r64.dtb
> >>   dtb-$(CONFIG_ARCH_MEDIATEK) += mt7981b-xiaomi-ax3000t.dtb
> >>   dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-acelink-ew-7886cax.dtb
> >>   dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3.dtb
> >> +dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3-mini.dtb
> >>   dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3-emmc.dtbo
> >>   dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3-nand.dtbo
> >>   dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3-nor.dtbo
> >> diff --git a/arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3-mini.dts b/arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3-mini.dts
> >> new file mode 100644
> >> index 000000000000..c764b4dc4752
> >> --- /dev/null
> >> +++ b/arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3-mini.dts
> >> @@ -0,0 +1,486 @@
> >> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> >> +/*
> >> + * Copyright (C) 2021 MediaTek Inc.
> >> + * Authors: Frank Wunderlich <frank-w@public-files.de>
> >> + *          Eric Woudstra <ericwouds@gmail.com>
> >> + *          Tianling Shen <cnsztl@immortalwrt.org>
> >> + */
> >> +
> >> +/dts-v1/;
> >> +
> >> +#include <dt-bindings/gpio/gpio.h>
> >> +#include <dt-bindings/input/input.h>
> >> +#include <dt-bindings/leds/common.h>
> >> +#include <dt-bindings/pinctrl/mt65xx.h>
> >> +
> >> +#include "mt7986a.dtsi"
> >> +
> >> +/ {
> >> +	model = "Bananapi BPI-R3 Mini";
> >> +	chassis-type = "embedded";
> >> +	compatible = "bananapi,bpi-r3mini", "mediatek,mt7986a";
> >> +
> >> +	aliases {
> >> +		serial0 = &uart0;
> >> +		ethernet0 = &gmac0;
> >> +		ethernet1 = &gmac1;
> >> +	};
> >> +
> >> +	chosen {
> >> +		stdout-path = "serial0:115200n8";
> >> +	};
> >> +
> >> +	dcin: regulator-12vd {
> >> +		compatible = "regulator-fixed";
> >> +		regulator-name = "12vd";
> >> +		regulator-min-microvolt = <12000000>;
> >> +		regulator-max-microvolt = <12000000>;
> >> +		regulator-boot-on;
> >> +		regulator-always-on;
> >> +	};
> >> +
> >> +	fan: pwm-fan {
> >> +		compatible = "pwm-fan";
> >> +		#cooling-cells = <2>;
> >> +		/* cooling level (0, 1, 2) - pwm inverted */
> >> +		cooling-levels = <255 96 0>;
> >
> >Did you try to actually invert the PWM?
> >
> >Look for PWM_POLARITY_INVERTED ;-)
> 
> Mtk pwm driver does not support it
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/pwm/pwm-mediatek.c#L211
> 
> >> +		pwms = <&pwm 0 10000>;
> >> +		status = "okay";
> >> +	};
> >> +
> >> +	reg_1p8v: regulator-1p8v {
> >> +		compatible = "regulator-fixed";
> >> +		regulator-name = "1.8vd";
> >> +		regulator-min-microvolt = <1800000>;
> >> +		regulator-max-microvolt = <1800000>;
> >> +		regulator-boot-on;
> >> +		regulator-always-on;
> >> +		vin-supply = <&dcin>;
> >> +	};
> >> +
> >> +	reg_3p3v: regulator-3p3v {
> >> +		compatible = "regulator-fixed";
> >> +		regulator-name = "3.3vd";
> >> +		regulator-min-microvolt = <3300000>;
> >> +		regulator-max-microvolt = <3300000>;
> >> +		regulator-boot-on;
> >> +		regulator-always-on;
> >> +		vin-supply = <&dcin>;
> >> +	};
> >> +
> >> +	usb_vbus: regulator-usb-vbus {
> >> +		compatible = "regulator-fixed";
> >> +		regulator-name = "usb_vbus";
> >> +		regulator-min-microvolt = <5000000>;
> >> +		regulator-max-microvolt = <5000000>;
> >> +		gpios = <&pio 20 GPIO_ACTIVE_LOW>;
> >> +		regulator-boot-on;
> >> +	};
> >> +
> >> +	en8811_a: regulator-phy1 {
> >> +		compatible = "regulator-fixed";
> >> +		regulator-name = "phy1";
> >> +		regulator-min-microvolt = <3300000>;
> >> +		regulator-max-microvolt = <3300000>;
> >> +		gpio = <&pio 16 GPIO_ACTIVE_LOW>;
> >> +		regulator-always-on;
> >> +	};
> >> +
> >> +	en8811_b: regulator-phy2 {
> >> +		compatible = "regulator-fixed";
> >> +		regulator-name = "phy2";
> >> +		regulator-min-microvolt = <3300000>;
> >> +		regulator-max-microvolt = <3300000>;
> >> +		gpio = <&pio 17 GPIO_ACTIVE_LOW>;
> >> +		regulator-always-on;
> >> +	};
> >> +
> >> +	leds {
> >> +		compatible = "gpio-leds";
> >> +
> >> +		green_led: led-0 {
> >> +			color = <LED_COLOR_ID_GREEN>;
> >> +			function = LED_FUNCTION_POWER;
> >> +			gpios = <&pio 19 GPIO_ACTIVE_HIGH>;
> >> +			default-state = "on";
> >> +		};
> >> +	};
> >> +
> >> +	gpio-keys {
> >> +		compatible = "gpio-keys";
> >> +
> >> +		reset-key {
> >> +			label = "reset";
> >> +			linux,code = <KEY_RESTART>;
> >> +			gpios = <&pio 7 GPIO_ACTIVE_LOW>;
> >> +		};
> >> +	};
> >> +
> >> +};
> >> +
> >> +&cpu_thermal {
> >> +	cooling-maps {
> >> +		map0 {
> >> +			/* active: set fan to cooling level 2 */
> >> +			cooling-device = <&fan 2 2>;
> >> +			trip = <&cpu_trip_active_high>;
> >> +		};
> >> +
> >> +		map1 {
> >> +			/* active: set fan to cooling level 1 */
> >> +			cooling-device = <&fan 1 1>;
> >> +			trip = <&cpu_trip_active_med>;
> >> +		};
> >> +
> >> +		map2 {
> >> +			/* active: set fan to cooling level 0 */
> >> +			cooling-device = <&fan 0 0>;
> >> +			trip = <&cpu_trip_active_low>;
> >> +		};
> >> +	};
> >> +};
> >> +
> >> +&crypto {
> >> +	status = "okay";
> >> +};
> >> +
> >> +&eth {
> >> +	status = "okay";
> >> +
> >> +	gmac0: mac@0 {
> >> +		compatible = "mediatek,eth-mac";
> >> +		reg = <0>;
> >> +		phy-mode = "2500base-x";
> >> +		phy-handle = <&phy14>;
> >> +	};
> >> +
> >> +	gmac1: mac@1 {
> >> +		compatible = "mediatek,eth-mac";
> >> +		reg = <1>;
> >> +		phy-mode = "2500base-x";
> >> +		phy-handle = <&phy15>;
> >> +	};
> >> +
> >> +	mdio: mdio-bus {
> >> +		#address-cells = <1>;
> >> +		#size-cells = <0>;
> >> +	};
> >> +};
> >> +
> >> +&mmc0 {
> >> +	pinctrl-names = "default", "state_uhs";
> >> +	pinctrl-0 = <&mmc0_pins_default>;
> >> +	pinctrl-1 = <&mmc0_pins_uhs>;
> >> +	vmmc-supply = <&reg_3p3v>;
> >> +	vqmmc-supply = <&reg_1p8v>;
> >> +};
> >> +
> >> +
> >> +&i2c0 {
> >> +	pinctrl-names = "default";
> >> +	pinctrl-0 = <&i2c_pins>;
> >> +	status = "okay";
> >> +
> >> +	/* MAC Address EEPROM */
> >> +	eeprom@50 {
> >> +		compatible = "atmel,24c02";
> >> +		reg = <0x50>;
> >> +
> >> +		address-width = <8>;
> >> +		pagesize = <8>;
> >> +		size = <256>;
> >> +	};
> >> +};
> >> +
> >> +&mdio {
> >
> >You can just move all this stuff to where you declare the mdio-bus....
> 
> Ok,see these 2 lines are already above,so can be dropped here.
> 
> >> +	#address-cells = <1>;
> >> +	#size-cells = <0>;
> >> +
> >> +	phy14: ethernet-phy@14 {
> >
> >I say that this is `phy0: ethernet-phy@14` - because this is the first PHY on this
> >board.
> 
> Ok,i change this and phy15
> 
> >> +		reg = <14>;
> >
> >Uhm.. doesn't this require the ethernet-phy-id03a2.a411 compatible?
> 
> I can add it,but worked without it.
> 
> There was a discussion about that and result was we don't need it in board dts,maybe add compatible in binding example.
> 
> https://patchwork.kernel.org/project/netdevbpf/patch/20240206194751.1901802-2-ericwouds@gmail.com/#25703356

I confirm that setting the PHY ID in DT is **not** needed.

The PHY probes fine and it's possible to read the PHY ID even before
firmware has been loaded.

> 
> >> +		interrupts-extended = <&pio 48 IRQ_TYPE_EDGE_FALLING>;
> >> +		reset-gpios = <&pio 49 GPIO_ACTIVE_LOW>;
> >> +		reset-assert-us = <10000>;
> >> +		reset-deassert-us = <20000>;
> >> +		phy-mode = "2500base-x";
> >> +		full-duplex;
> >> +		pause;
> >> +		airoha,pnswap-rx;
> >> +
> >> +		leds {
> >> +			#address-cells = <1>;
> >> +			#size-cells = <0>;
> >> +
> >> +			led@0 { /* en8811_a_gpio5 */
> >> +				reg = <0>;
> >> +				color = <LED_COLOR_ID_YELLOW>;
> >> +				function = LED_FUNCTION_LAN;
> >> +				function-enumerator = <1>;
> >
> >Why aren't you simply using a label?
> 
> You mean the comment? I can add it of course like for regulators.
> 
> >> +				default-state = "keep";
> >> +				linux,default-trigger = "netdev";

Using linux,default-trigger = "netdev" will NOT work as intended,
see also the reply to your other patch where you are adding netdev
trigger to dt-bindings.
If you do this, it will seemingly work, but if you check
/sys/class/leds/foo/hw_control
it will always be 0, and hardware offloading will hence never be used.
Please just set the LED trigger in userspace for now.

> >> +			};
> >> +			led@1 { /* en8811_a_gpio4 */
> >> +				reg = <1>;
> >> +				color = <LED_COLOR_ID_GREEN>;
> >> +				function = LED_FUNCTION_LAN;
> >> +				function-enumerator = <2>;
> >> +				default-state = "keep";
> >> +				linux,default-trigger = "netdev";
> >> +			};
> >> +		};
> >> +	};
> >> +
> >> +	phy15: ethernet-phy@15 {
> >> +		reg = <15>;
> >
> >Same here.
> >
> >Cheers,
> >Angelo
> >
> 
> 
> regards Frank
>
Frank Wunderlich May 6, 2024, 4:40 p.m. UTC | #5
Hi

Thanks for review



> Gesendet: Montag, 06. Mai 2024 um 10:20 Uhr
> Von: "Krzysztof Kozlowski" <krzysztof.kozlowski@linaro.org>
> An: "Frank Wunderlich" <linux@fw-web.de>, "Rob Herring" <robh@kernel.org>, "Krzysztof Kozlowski"
> > +	dcin: regulator-12vd {
>
> Please use name for all fixed regulators which matches current format
> recommendation: 'regulator-[0-9]+v[0-9]+'
>
> https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git/commit/?id=b6d4b3500d57370f5b3abf0701c9166b384db976
>
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "12vd";
> > +		regulator-min-microvolt = <12000000>;
> > +		regulator-max-microvolt = <12000000>;
> > +		regulator-boot-on;
> > +		regulator-always-on;
> > +	};
> > +
> > +	fan: pwm-fan {
> > +		compatible = "pwm-fan";
> > +		#cooling-cells = <2>;
> > +		/* cooling level (0, 1, 2) - pwm inverted */
> > +		cooling-levels = <255 96 0>;
> > +		pwms = <&pwm 0 10000>;
> > +		status = "okay";
>
> Why? Where is it disabled?

you're right, i'll drop it

> > +	};
> > +
> > +	reg_1p8v: regulator-1p8v {
>
> Please use name for all fixed regulators which matches current format
> recommendation: 'regulator-[0-9]+v[0-9]+'
>
> https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git/commit/?id=b6d4b3500d57370f5b3abf0701c9166b384db976
>
> In other places as well.

ok i change it like this (label doesn't matter, right?):

dcin: regulator-12v {
reg_1p8v: regulator-1v8 {
reg_3p3v: regulator-3v3 {
usb_vbus: regulator-5v {


> Best regards,
> Krzysztof

regards Frank
AngeloGioacchino Del Regno May 7, 2024, 1:35 p.m. UTC | #6
Il 06/05/24 18:00, Frank Wunderlich ha scritto:
> Hi
> 
> Thanks for review.
> 
> Am 6. Mai 2024 14:48:59 MESZ schrieb AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>:
>> Il 05/05/24 18:45, Frank Wunderlich ha scritto:
>>> From: Frank Wunderlich <frank-w@public-files.de>
>>>
>>> Add device Tree for Bananapi R3 Mini SBC.
>>>
>>> Co-developed-by: Eric Woudstra <ericwouds@gmail.com>
>>> Signed-off-by: Eric Woudstra <ericwouds@gmail.com>
>>> Co-developed-by: Tianling Shen <cnsztl@gmail.com>
>>> Signed-off-by: Tianling Shen <cnsztl@gmail.com>
>>> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
>>> ---
>>>    arch/arm64/boot/dts/mediatek/Makefile         |   1 +
>>>    .../mediatek/mt7986a-bananapi-bpi-r3-mini.dts | 486 ++++++++++++++++++
>>>    2 files changed, 487 insertions(+)
>>>    create mode 100644 arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3-mini.dts
>>>
>>> diff --git a/arch/arm64/boot/dts/mediatek/Makefile b/arch/arm64/boot/dts/mediatek/Makefile
>>> index 37b4ca3a87c9..1763b001ab06 100644
>>> --- a/arch/arm64/boot/dts/mediatek/Makefile
>>> +++ b/arch/arm64/boot/dts/mediatek/Makefile
>>> @@ -11,6 +11,7 @@ dtb-$(CONFIG_ARCH_MEDIATEK) += mt7622-bananapi-bpi-r64.dtb
>>>    dtb-$(CONFIG_ARCH_MEDIATEK) += mt7981b-xiaomi-ax3000t.dtb
>>>    dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-acelink-ew-7886cax.dtb
>>>    dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3.dtb
>>> +dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3-mini.dtb
>>>    dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3-emmc.dtbo
>>>    dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3-nand.dtbo
>>>    dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3-nor.dtbo
>>> diff --git a/arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3-mini.dts b/arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3-mini.dts
>>> new file mode 100644
>>> index 000000000000..c764b4dc4752
>>> --- /dev/null
>>> +++ b/arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3-mini.dts
>>> @@ -0,0 +1,486 @@
>>> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
>>> +/*
>>> + * Copyright (C) 2021 MediaTek Inc.
>>> + * Authors: Frank Wunderlich <frank-w@public-files.de>
>>> + *          Eric Woudstra <ericwouds@gmail.com>
>>> + *          Tianling Shen <cnsztl@immortalwrt.org>
>>> + */
>>> +
>>> +/dts-v1/;
>>> +
>>> +#include <dt-bindings/gpio/gpio.h>
>>> +#include <dt-bindings/input/input.h>
>>> +#include <dt-bindings/leds/common.h>
>>> +#include <dt-bindings/pinctrl/mt65xx.h>
>>> +
>>> +#include "mt7986a.dtsi"
>>> +
>>> +/ {
>>> +	model = "Bananapi BPI-R3 Mini";
>>> +	chassis-type = "embedded";
>>> +	compatible = "bananapi,bpi-r3mini", "mediatek,mt7986a";
>>> +
>>> +	aliases {
>>> +		serial0 = &uart0;
>>> +		ethernet0 = &gmac0;
>>> +		ethernet1 = &gmac1;
>>> +	};
>>> +
>>> +	chosen {
>>> +		stdout-path = "serial0:115200n8";
>>> +	};
>>> +
>>> +	dcin: regulator-12vd {
>>> +		compatible = "regulator-fixed";
>>> +		regulator-name = "12vd";
>>> +		regulator-min-microvolt = <12000000>;
>>> +		regulator-max-microvolt = <12000000>;
>>> +		regulator-boot-on;
>>> +		regulator-always-on;
>>> +	};
>>> +
>>> +	fan: pwm-fan {
>>> +		compatible = "pwm-fan";
>>> +		#cooling-cells = <2>;
>>> +		/* cooling level (0, 1, 2) - pwm inverted */
>>> +		cooling-levels = <255 96 0>;
>>
>> Did you try to actually invert the PWM?
>>
>> Look for PWM_POLARITY_INVERTED ;-)
> 
> Mtk pwm driver does not support it
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/pwm/pwm-mediatek.c#L211
> 

You're right, sorry - I confused the general purpose PWM controller with the
rather specific DISP_PWM controller (which does support polarity inversion).

It's good - but I'd appreciate if you can please add a comment stating that
the PWM values are inverted in SW because the controller does *not* support
polarity inversion... so that next time someone looks at this will immediately
understand what's going on and why :-)

>>> +		pwms = <&pwm 0 10000>;
>>> +		status = "okay";
>>> +	};
>>> +
>>> +	reg_1p8v: regulator-1p8v {
>>> +		compatible = "regulator-fixed";
>>> +		regulator-name = "1.8vd";
>>> +		regulator-min-microvolt = <1800000>;
>>> +		regulator-max-microvolt = <1800000>;
>>> +		regulator-boot-on;
>>> +		regulator-always-on;
>>> +		vin-supply = <&dcin>;
>>> +	};
>>> +
>>> +	reg_3p3v: regulator-3p3v {
>>> +		compatible = "regulator-fixed";
>>> +		regulator-name = "3.3vd";
>>> +		regulator-min-microvolt = <3300000>;
>>> +		regulator-max-microvolt = <3300000>;
>>> +		regulator-boot-on;
>>> +		regulator-always-on;
>>> +		vin-supply = <&dcin>;
>>> +	};
>>> +
>>> +	usb_vbus: regulator-usb-vbus {
>>> +		compatible = "regulator-fixed";
>>> +		regulator-name = "usb_vbus";
>>> +		regulator-min-microvolt = <5000000>;
>>> +		regulator-max-microvolt = <5000000>;
>>> +		gpios = <&pio 20 GPIO_ACTIVE_LOW>;
>>> +		regulator-boot-on;
>>> +	};
>>> +
>>> +	en8811_a: regulator-phy1 {
>>> +		compatible = "regulator-fixed";
>>> +		regulator-name = "phy1";
>>> +		regulator-min-microvolt = <3300000>;
>>> +		regulator-max-microvolt = <3300000>;
>>> +		gpio = <&pio 16 GPIO_ACTIVE_LOW>;
>>> +		regulator-always-on;
>>> +	};
>>> +
>>> +	en8811_b: regulator-phy2 {
>>> +		compatible = "regulator-fixed";
>>> +		regulator-name = "phy2";
>>> +		regulator-min-microvolt = <3300000>;
>>> +		regulator-max-microvolt = <3300000>;
>>> +		gpio = <&pio 17 GPIO_ACTIVE_LOW>;
>>> +		regulator-always-on;
>>> +	};
>>> +
>>> +	leds {
>>> +		compatible = "gpio-leds";
>>> +
>>> +		green_led: led-0 {
>>> +			color = <LED_COLOR_ID_GREEN>;
>>> +			function = LED_FUNCTION_POWER;
>>> +			gpios = <&pio 19 GPIO_ACTIVE_HIGH>;
>>> +			default-state = "on";
>>> +		};
>>> +	};
>>> +
>>> +	gpio-keys {
>>> +		compatible = "gpio-keys";
>>> +
>>> +		reset-key {
>>> +			label = "reset";
>>> +			linux,code = <KEY_RESTART>;
>>> +			gpios = <&pio 7 GPIO_ACTIVE_LOW>;
>>> +		};
>>> +	};
>>> +
>>> +};
>>> +
>>> +&cpu_thermal {
>>> +	cooling-maps {
>>> +		map0 {
>>> +			/* active: set fan to cooling level 2 */
>>> +			cooling-device = <&fan 2 2>;
>>> +			trip = <&cpu_trip_active_high>;
>>> +		};
>>> +
>>> +		map1 {
>>> +			/* active: set fan to cooling level 1 */
>>> +			cooling-device = <&fan 1 1>;
>>> +			trip = <&cpu_trip_active_med>;
>>> +		};
>>> +
>>> +		map2 {
>>> +			/* active: set fan to cooling level 0 */
>>> +			cooling-device = <&fan 0 0>;
>>> +			trip = <&cpu_trip_active_low>;
>>> +		};
>>> +	};
>>> +};
>>> +
>>> +&crypto {
>>> +	status = "okay";
>>> +};
>>> +
>>> +&eth {
>>> +	status = "okay";
>>> +
>>> +	gmac0: mac@0 {
>>> +		compatible = "mediatek,eth-mac";
>>> +		reg = <0>;
>>> +		phy-mode = "2500base-x";
>>> +		phy-handle = <&phy14>;
>>> +	};
>>> +
>>> +	gmac1: mac@1 {
>>> +		compatible = "mediatek,eth-mac";
>>> +		reg = <1>;
>>> +		phy-mode = "2500base-x";
>>> +		phy-handle = <&phy15>;
>>> +	};
>>> +
>>> +	mdio: mdio-bus {
>>> +		#address-cells = <1>;
>>> +		#size-cells = <0>;
>>> +	};
>>> +};
>>> +
>>> +&mmc0 {
>>> +	pinctrl-names = "default", "state_uhs";
>>> +	pinctrl-0 = <&mmc0_pins_default>;
>>> +	pinctrl-1 = <&mmc0_pins_uhs>;
>>> +	vmmc-supply = <&reg_3p3v>;
>>> +	vqmmc-supply = <&reg_1p8v>;
>>> +};
>>> +
>>> +
>>> +&i2c0 {
>>> +	pinctrl-names = "default";
>>> +	pinctrl-0 = <&i2c_pins>;
>>> +	status = "okay";
>>> +
>>> +	/* MAC Address EEPROM */
>>> +	eeprom@50 {
>>> +		compatible = "atmel,24c02";
>>> +		reg = <0x50>;
>>> +
>>> +		address-width = <8>;
>>> +		pagesize = <8>;
>>> +		size = <256>;
>>> +	};
>>> +};
>>> +
>>> +&mdio {
>>
>> You can just move all this stuff to where you declare the mdio-bus....
> 
> Ok,see these 2 lines are already above,so can be dropped here.
> 
>>> +	#address-cells = <1>;
>>> +	#size-cells = <0>;
>>> +
>>> +	phy14: ethernet-phy@14 {
>>
>> I say that this is `phy0: ethernet-phy@14` - because this is the first PHY on this
>> board.
> 
> Ok,i change this and phy15
> 
>>> +		reg = <14>;
>>
>> Uhm.. doesn't this require the ethernet-phy-id03a2.a411 compatible?
> 
> I can add it,but worked without it.
> 
> There was a discussion about that and result was we don't need it in board dts,maybe add compatible in binding example.
> 
> https://patchwork.kernel.org/project/netdevbpf/patch/20240206194751.1901802-2-ericwouds@gmail.com/#25703356
> 

Ah, okay. Leave it then.

>>> +		interrupts-extended = <&pio 48 IRQ_TYPE_EDGE_FALLING>;
>>> +		reset-gpios = <&pio 49 GPIO_ACTIVE_LOW>;
>>> +		reset-assert-us = <10000>;
>>> +		reset-deassert-us = <20000>;
>>> +		phy-mode = "2500base-x";
>>> +		full-duplex;
>>> +		pause;
>>> +		airoha,pnswap-rx;
>>> +
>>> +		leds {
>>> +			#address-cells = <1>;
>>> +			#size-cells = <0>;
>>> +
>>> +			led@0 { /* en8811_a_gpio5 */
>>> +				reg = <0>;
>>> +				color = <LED_COLOR_ID_YELLOW>;
>>> +				function = LED_FUNCTION_LAN;
>>> +				function-enumerator = <1>;
>>
>> Why aren't you simply using a label?
> 
> You mean the comment? I can add it of course like for regulators.
> 

I mean in place of the function-enumerator... that's practically used to
distinguish between instances, it's not too common to see it, and usually
"label" replaces exactly that - just that, instead of a different number,
it gets a different name with no (usually) meaningless numbers :-)

Cheers!

>>> +				default-state = "keep";
>>> +				linux,default-trigger = "netdev";
>>> +			};
>>> +			led@1 { /* en8811_a_gpio4 */
>>> +				reg = <1>;
>>> +				color = <LED_COLOR_ID_GREEN>;
>>> +				function = LED_FUNCTION_LAN;
>>> +				function-enumerator = <2>;
>>> +				default-state = "keep";
>>> +				linux,default-trigger = "netdev";
>>> +			};
>>> +		};
>>> +	};
>>> +
>>> +	phy15: ethernet-phy@15 {
>>> +		reg = <15>;
>>
>> Same here.
>>
>> Cheers,
>> Angelo
>>
> 
> 
> regards Frank
Frank Wunderlich May 8, 2024, 6:25 p.m. UTC | #7
Hi

> Gesendet: Dienstag, 07. Mai 2024 um 15:35 Uhr
> Von: "AngeloGioacchino Del Regno" <angelogioacchino.delregno@collabora.com>
>
> Il 06/05/24 18:00, Frank Wunderlich ha scritto:

> >>> +	fan: pwm-fan {
> >>> +		compatible = "pwm-fan";
> >>> +		#cooling-cells = <2>;
> >>> +		/* cooling level (0, 1, 2) - pwm inverted */
> >>> +		cooling-levels = <255 96 0>;
> >>
> >> Did you try to actually invert the PWM?
> >>
> >> Look for PWM_POLARITY_INVERTED ;-)
> >
> > Mtk pwm driver does not support it
> >
> > https://elixir.bootlin.com/linux/latest/source/drivers/pwm/pwm-mediatek.c#L211
> >
>
> You're right, sorry - I confused the general purpose PWM controller with the
> rather specific DISP_PWM controller (which does support polarity inversion).
>
> It's good - but I'd appreciate if you can please add a comment stating that
> the PWM values are inverted in SW because the controller does *not* support
> polarity inversion... so that next time someone looks at this will immediately
> understand what's going on and why :-)

so i would change comment like this:

		/* cooling level (0, 1, 2)
		 * signal is inverted on board
		 * mtk pwm driver does not support
		 * PWM_POLARITY_INVERTED */

> >>> +		pwms = <&pwm 0 10000>;
> >>> +		status = "okay";
> >>> +	};
> >>> +
> >>> +	phy14: ethernet-phy@14 {
...
> >>> +		interrupts-extended = <&pio 48 IRQ_TYPE_EDGE_FALLING>;
> >>> +		reset-gpios = <&pio 49 GPIO_ACTIVE_LOW>;
> >>> +		reset-assert-us = <10000>;
> >>> +		reset-deassert-us = <20000>;
> >>> +		phy-mode = "2500base-x";
> >>> +		full-duplex;
> >>> +		pause;
> >>> +		airoha,pnswap-rx;
> >>> +
> >>> +		leds {
> >>> +			#address-cells = <1>;
> >>> +			#size-cells = <0>;
> >>> +
> >>> +			led@0 { /* en8811_a_gpio5 */
> >>> +				reg = <0>;
> >>> +				color = <LED_COLOR_ID_YELLOW>;
> >>> +				function = LED_FUNCTION_LAN;
> >>> +				function-enumerator = <1>;
> >>
> >> Why aren't you simply using a label?
> >
> > You mean the comment? I can add it of course like for regulators.
> >
>
> I mean in place of the function-enumerator... that's practically used to
> distinguish between instances, it's not too common to see it, and usually
> "label" replaces exactly that - just that, instead of a different number,
> it gets a different name with no (usually) meaningless numbers :-)

as far as i understand using label also makes "function" property useless, after discussing
this with eric i would drop both on all 4 places by labels like these:

label = "yellow-lan";
label = "green-lan";
...

not sure if we should drop color property too...

> >>> +				default-state = "keep";
> >>> +				linux,default-trigger = "netdev";
> >>> +			};
> >>> +			led@1 { /* en8811_a_gpio4 */
> >>> +				reg = <1>;
> >>> +				color = <LED_COLOR_ID_GREEN>;
> >>> +				function = LED_FUNCTION_LAN;
> >>> +				function-enumerator = <2>;
> >>> +				default-state = "keep";
> >>> +				linux,default-trigger = "netdev";
> >>> +			};
> >>> +		};
> >>> +	};
> >>> +
> >>> +	phy15: ethernet-phy@15 {
> >>> +		reg = <15>;
> >>
> >> Same here.
> >>
> >> Cheers,
> >> Angelo

regards Frank
AngeloGioacchino Del Regno May 9, 2024, 10:10 a.m. UTC | #8
Il 08/05/24 20:25, Frank Wunderlich ha scritto:
> Hi
> 
>> Gesendet: Dienstag, 07. Mai 2024 um 15:35 Uhr
>> Von: "AngeloGioacchino Del Regno" <angelogioacchino.delregno@collabora.com>
>>
>> Il 06/05/24 18:00, Frank Wunderlich ha scritto:
> 
>>>>> +	fan: pwm-fan {
>>>>> +		compatible = "pwm-fan";
>>>>> +		#cooling-cells = <2>;
>>>>> +		/* cooling level (0, 1, 2) - pwm inverted */
>>>>> +		cooling-levels = <255 96 0>;
>>>>
>>>> Did you try to actually invert the PWM?
>>>>
>>>> Look for PWM_POLARITY_INVERTED ;-)
>>>
>>> Mtk pwm driver does not support it
>>>
>>> https://elixir.bootlin.com/linux/latest/source/drivers/pwm/pwm-mediatek.c#L211
>>>
>>
>> You're right, sorry - I confused the general purpose PWM controller with the
>> rather specific DISP_PWM controller (which does support polarity inversion).
>>
>> It's good - but I'd appreciate if you can please add a comment stating that
>> the PWM values are inverted in SW because the controller does *not* support
>> polarity inversion... so that next time someone looks at this will immediately
>> understand what's going on and why :-)
> 
> so i would change comment like this:
> 
> 		/* cooling level (0, 1, 2)
> 		 * signal is inverted on board
> 		 * mtk pwm driver does not support
> 		 * PWM_POLARITY_INVERTED */
> 

There you go:

/*
  * The signal is inverted on this board and the general purpose
  * PWM HW IP in this SoC does not support polarity inversion.
  */
/* Cooling level < 0  1  2> */
cooling-levels = <255 96 0>;



>>>>> +		pwms = <&pwm 0 10000>;
>>>>> +		status = "okay";
>>>>> +	};
>>>>> +
>>>>> +	phy14: ethernet-phy@14 {
> ...
>>>>> +		interrupts-extended = <&pio 48 IRQ_TYPE_EDGE_FALLING>;
>>>>> +		reset-gpios = <&pio 49 GPIO_ACTIVE_LOW>;
>>>>> +		reset-assert-us = <10000>;
>>>>> +		reset-deassert-us = <20000>;
>>>>> +		phy-mode = "2500base-x";
>>>>> +		full-duplex;
>>>>> +		pause;
>>>>> +		airoha,pnswap-rx;
>>>>> +
>>>>> +		leds {
>>>>> +			#address-cells = <1>;
>>>>> +			#size-cells = <0>;
>>>>> +
>>>>> +			led@0 { /* en8811_a_gpio5 */
>>>>> +				reg = <0>;
>>>>> +				color = <LED_COLOR_ID_YELLOW>;
>>>>> +				function = LED_FUNCTION_LAN;
>>>>> +				function-enumerator = <1>;
>>>>
>>>> Why aren't you simply using a label?
>>>
>>> You mean the comment? I can add it of course like for regulators.
>>>
>>
>> I mean in place of the function-enumerator... that's practically used to
>> distinguish between instances, it's not too common to see it, and usually
>> "label" replaces exactly that - just that, instead of a different number,
>> it gets a different name with no (usually) meaningless numbers :-)
> 
> as far as i understand using label also makes "function" property useless, after discussing
> this with eric i would drop both on all 4 places by labels like these:
> 
> label = "yellow-lan";
> label = "green-lan";
> ...
> 
> not sure if we should drop color property too...
> 

I'm looking at the leds binding (leds/common.yaml) right now.

My suggestion of using 'label' was actually wrong - and your devicetree was
actually right!!! (apart from the default-trigger that may not work)

Infact, the documentation says, in brief:

- function-enumerator is ignored if label is present
- function doesn't say that gets ignored
- color doesn't say that gets ignored
- label says:
   - If not present -> get string from node name
   - function-enumerator ignored
   - This property is deprecated

...but the 'label' binding does not say 'deprecated: true', which is something
that must be fixed!


So, I'm sorry for the confusion, the noise and the useless loss of time around
this - you can keep the LED nodes as they are, and that's a lesson for the future
me reviewing another node like this one.

P.S.: This shouldn't have been a RFC, as the patches are more than RFC quality!!!

Cheers,
Angelo

>>>>> +				default-state = "keep";
>>>>> +				linux,default-trigger = "netdev";
>>>>> +			};
>>>>> +			led@1 { /* en8811_a_gpio4 */
>>>>> +				reg = <1>;
>>>>> +				color = <LED_COLOR_ID_GREEN>;
>>>>> +				function = LED_FUNCTION_LAN;
>>>>> +				function-enumerator = <2>;
>>>>> +				default-state = "keep";
>>>>> +				linux,default-trigger = "netdev";
>>>>> +			};
>>>>> +		};
>>>>> +	};
>>>>> +
>>>>> +	phy15: ethernet-phy@15 {
>>>>> +		reg = <15>;
>>>>
>>>> Same here.
>>>>
>>>> Cheers,
>>>> Angelo
> 
> regards Frank
Frank Wunderlich May 9, 2024, 10:30 a.m. UTC | #9
Am 9. Mai 2024 12:10:59 MESZ schrieb AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>:
>Il 08/05/24 20:25, Frank Wunderlich ha scritto:
>> Hi
>> 
>>> Gesendet: Dienstag, 07. Mai 2024 um 15:35 Uhr
>>> Von: "AngeloGioacchino Del Regno" <angelogioacchino.delregno@collabora.com>
>>> 
>>> Il 06/05/24 18:00, Frank Wunderlich ha scritto:
>> 
>>>>>> +	fan: pwm-fan {
>>>>>> +		compatible = "pwm-fan";
>>>>>> +		#cooling-cells = <2>;
>>>>>> +		/* cooling level (0, 1, 2) - pwm inverted */
>>>>>> +		cooling-levels = <255 96 0>;
>>>>> 
>>>>> Did you try to actually invert the PWM?
>>>>> 
>>>>> Look for PWM_POLARITY_INVERTED ;-)
>>>> 
>>>> Mtk pwm driver does not support it
>>>> 
>>>> https://elixir.bootlin.com/linux/latest/source/drivers/pwm/pwm-mediatek.c#L211
>>>> 
>>> 
>>> You're right, sorry - I confused the general purpose PWM controller with the
>>> rather specific DISP_PWM controller (which does support polarity inversion).
>>> 
>>> It's good - but I'd appreciate if you can please add a comment stating that
>>> the PWM values are inverted in SW because the controller does *not* support
>>> polarity inversion... so that next time someone looks at this will immediately
>>> understand what's going on and why :-)
>> 
>> so i would change comment like this:
>> 
>> 		/* cooling level (0, 1, 2)
>> 		 * signal is inverted on board
>> 		 * mtk pwm driver does not support
>> 		 * PWM_POLARITY_INVERTED */
>> 
>
>There you go:
>
>/*
> * The signal is inverted on this board and the general purpose
> * PWM HW IP in this SoC does not support polarity inversion.
> */
>/* Cooling level < 0  1  2> */
>cooling-levels = <255 96 0>;

Thanks for clearing structure of the comment,but imho actually it is a driver issue (for all mtk SoC). Not sure it is really a hardware limitation. So i would change this to "... and the PWM driver does not support polarity inversion."

>>>>>> +		pwms = <&pwm 0 10000>;
>>>>>> +		status = "okay";
>>>>>> +	};
>>>>>> +
>>>>>> +	phy14: ethernet-phy@14 {
>> ...
>>>>>> +		interrupts-extended = <&pio 48 IRQ_TYPE_EDGE_FALLING>;
>>>>>> +		reset-gpios = <&pio 49 GPIO_ACTIVE_LOW>;
>>>>>> +		reset-assert-us = <10000>;
>>>>>> +		reset-deassert-us = <20000>;
>>>>>> +		phy-mode = "2500base-x";
>>>>>> +		full-duplex;
>>>>>> +		pause;
>>>>>> +		airoha,pnswap-rx;
>>>>>> +
>>>>>> +		leds {
>>>>>> +			#address-cells = <1>;
>>>>>> +			#size-cells = <0>;
>>>>>> +
>>>>>> +			led@0 { /* en8811_a_gpio5 */
>>>>>> +				reg = <0>;
>>>>>> +				color = <LED_COLOR_ID_YELLOW>;
>>>>>> +				function = LED_FUNCTION_LAN;
>>>>>> +				function-enumerator = <1>;
>>>>> 
>>>>> Why aren't you simply using a label?
>>>> 
>>>> You mean the comment? I can add it of course like for regulators.
>>>> 
>>> 
>>> I mean in place of the function-enumerator... that's practically used to
>>> distinguish between instances, it's not too common to see it, and usually
>>> "label" replaces exactly that - just that, instead of a different number,
>>> it gets a different name with no (usually) meaningless numbers :-)
>> 
>> as far as i understand using label also makes "function" property useless, after discussing
>> this with eric i would drop both on all 4 places by labels like these:
>> 
>> label = "yellow-lan";
>> label = "green-lan";
>> ...
>> 
>> not sure if we should drop color property too...
>> 
>
>I'm looking at the leds binding (leds/common.yaml) right now.
>
>My suggestion of using 'label' was actually wrong - and your devicetree was
>actually right!!! (apart from the default-trigger that may not work)
>
>Infact, the documentation says, in brief:
>
>- function-enumerator is ignored if label is present
>- function doesn't say that gets ignored
>- color doesn't say that gets ignored
>- label says:
>  - If not present -> get string from node name
>  - function-enumerator ignored
>  - This property is deprecated
>
>...but the 'label' binding does not say 'deprecated: true', which is something
>that must be fixed!

Ok,i can try to add the property to binding (independ of this series). Imho label was cleaner than function and function-enumerator...

>So, I'm sorry for the confusion, the noise and the useless loss of time around
>this - you can keep the LED nodes as they are, and that's a lesson for the future
>me reviewing another node like this one.

Don't worry, we are all humas...i missed looking in linux-next for the other binding-patches.

>P.S.: This shouldn't have been a RFC, as the patches are more than RFC quality!!!

I sent it as RFC because i had not expected to be merged before next is closed.

>Cheers,
>Angelo
>
>>>>>> +				default-state = "keep";
>>>>>> +				linux,default-trigger = "netdev";
>>>>>> +			};
>>>>>> +			led@1 { /* en8811_a_gpio4 */
>>>>>> +				reg = <1>;
>>>>>> +				color = <LED_COLOR_ID_GREEN>;
>>>>>> +				function = LED_FUNCTION_LAN;
>>>>>> +				function-enumerator = <2>;
>>>>>> +				default-state = "keep";
>>>>>> +				linux,default-trigger = "netdev";
>>>>>> +			};
>>>>>> +		};
>>>>>> +	};
>>>>>> +
>>>>>> +	phy15: ethernet-phy@15 {
>>>>>> +		reg = <15>;


regards Frank
AngeloGioacchino Del Regno May 9, 2024, 10:35 a.m. UTC | #10
Il 09/05/24 12:30, Frank Wunderlich ha scritto:
> Am 9. Mai 2024 12:10:59 MESZ schrieb AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>:
>> Il 08/05/24 20:25, Frank Wunderlich ha scritto:
>>> Hi
>>>
>>>> Gesendet: Dienstag, 07. Mai 2024 um 15:35 Uhr
>>>> Von: "AngeloGioacchino Del Regno" <angelogioacchino.delregno@collabora.com>
>>>>
>>>> Il 06/05/24 18:00, Frank Wunderlich ha scritto:
>>>
>>>>>>> +	fan: pwm-fan {
>>>>>>> +		compatible = "pwm-fan";
>>>>>>> +		#cooling-cells = <2>;
>>>>>>> +		/* cooling level (0, 1, 2) - pwm inverted */
>>>>>>> +		cooling-levels = <255 96 0>;
>>>>>>
>>>>>> Did you try to actually invert the PWM?
>>>>>>
>>>>>> Look for PWM_POLARITY_INVERTED ;-)
>>>>>
>>>>> Mtk pwm driver does not support it
>>>>>
>>>>> https://elixir.bootlin.com/linux/latest/source/drivers/pwm/pwm-mediatek.c#L211
>>>>>
>>>>
>>>> You're right, sorry - I confused the general purpose PWM controller with the
>>>> rather specific DISP_PWM controller (which does support polarity inversion).
>>>>
>>>> It's good - but I'd appreciate if you can please add a comment stating that
>>>> the PWM values are inverted in SW because the controller does *not* support
>>>> polarity inversion... so that next time someone looks at this will immediately
>>>> understand what's going on and why :-)
>>>
>>> so i would change comment like this:
>>>
>>> 		/* cooling level (0, 1, 2)
>>> 		 * signal is inverted on board
>>> 		 * mtk pwm driver does not support
>>> 		 * PWM_POLARITY_INVERTED */
>>>
>>
>> There you go:
>>
>> /*
>> * The signal is inverted on this board and the general purpose
>> * PWM HW IP in this SoC does not support polarity inversion.
>> */
>> /* Cooling level < 0  1  2> */
>> cooling-levels = <255 96 0>;
> 
> Thanks for clearing structure of the comment,but imho actually it is a driver issue (for all mtk SoC). Not sure it is really a hardware limitation. So i would change this to "... and the PWM driver does not support polarity inversion."
> 
>>>>>>> +		pwms = <&pwm 0 10000>;
>>>>>>> +		status = "okay";
>>>>>>> +	};
>>>>>>> +
>>>>>>> +	phy14: ethernet-phy@14 {
>>> ...
>>>>>>> +		interrupts-extended = <&pio 48 IRQ_TYPE_EDGE_FALLING>;
>>>>>>> +		reset-gpios = <&pio 49 GPIO_ACTIVE_LOW>;
>>>>>>> +		reset-assert-us = <10000>;
>>>>>>> +		reset-deassert-us = <20000>;
>>>>>>> +		phy-mode = "2500base-x";
>>>>>>> +		full-duplex;
>>>>>>> +		pause;
>>>>>>> +		airoha,pnswap-rx;
>>>>>>> +
>>>>>>> +		leds {
>>>>>>> +			#address-cells = <1>;
>>>>>>> +			#size-cells = <0>;
>>>>>>> +
>>>>>>> +			led@0 { /* en8811_a_gpio5 */
>>>>>>> +				reg = <0>;
>>>>>>> +				color = <LED_COLOR_ID_YELLOW>;
>>>>>>> +				function = LED_FUNCTION_LAN;
>>>>>>> +				function-enumerator = <1>;
>>>>>>
>>>>>> Why aren't you simply using a label?
>>>>>
>>>>> You mean the comment? I can add it of course like for regulators.
>>>>>
>>>>
>>>> I mean in place of the function-enumerator... that's practically used to
>>>> distinguish between instances, it's not too common to see it, and usually
>>>> "label" replaces exactly that - just that, instead of a different number,
>>>> it gets a different name with no (usually) meaningless numbers :-)
>>>
>>> as far as i understand using label also makes "function" property useless, after discussing
>>> this with eric i would drop both on all 4 places by labels like these:
>>>
>>> label = "yellow-lan";
>>> label = "green-lan";
>>> ...
>>>
>>> not sure if we should drop color property too...
>>>
>>
>> I'm looking at the leds binding (leds/common.yaml) right now.
>>
>> My suggestion of using 'label' was actually wrong - and your devicetree was
>> actually right!!! (apart from the default-trigger that may not work)
>>
>> Infact, the documentation says, in brief:
>>
>> - function-enumerator is ignored if label is present
>> - function doesn't say that gets ignored
>> - color doesn't say that gets ignored
>> - label says:
>>   - If not present -> get string from node name
>>   - function-enumerator ignored
>>   - This property is deprecated
>>
>> ...but the 'label' binding does not say 'deprecated: true', which is something
>> that must be fixed!
> 
> Ok,i can try to add the property to binding (independ of this series). Imho label was cleaner than function and function-enumerator...
> 

Oh I sort of agree with you, I liked the label more, as it's more consistent with
everything else... but oh well. :-)

>> So, I'm sorry for the confusion, the noise and the useless loss of time around
>> this - you can keep the LED nodes as they are, and that's a lesson for the future
>> me reviewing another node like this one.
> 
> Don't worry, we are all humas...i missed looking in linux-next for the other binding-patches.
> 
>> P.S.: This shouldn't have been a RFC, as the patches are more than RFC quality!!!
> 
> I sent it as RFC because i had not expected to be merged before next is closed.
> 

Ah at least from my side, no worries... when I see RFC I generally expect to see
"dubious/head-scratching stuff", not "sub-optimal timing to send a patch" :-P

Cheers!
Angelo

>>
>>>>>>> +				default-state = "keep";
>>>>>>> +				linux,default-trigger = "netdev";
>>>>>>> +			};
>>>>>>> +			led@1 { /* en8811_a_gpio4 */
>>>>>>> +				reg = <1>;
>>>>>>> +				color = <LED_COLOR_ID_GREEN>;
>>>>>>> +				function = LED_FUNCTION_LAN;
>>>>>>> +				function-enumerator = <2>;
>>>>>>> +				default-state = "keep";
>>>>>>> +				linux,default-trigger = "netdev";
>>>>>>> +			};
>>>>>>> +		};
>>>>>>> +	};
>>>>>>> +
>>>>>>> +	phy15: ethernet-phy@15 {
>>>>>>> +		reg = <15>;
> 
> 
> regards Frank
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/mediatek/Makefile b/arch/arm64/boot/dts/mediatek/Makefile
index 37b4ca3a87c9..1763b001ab06 100644
--- a/arch/arm64/boot/dts/mediatek/Makefile
+++ b/arch/arm64/boot/dts/mediatek/Makefile
@@ -11,6 +11,7 @@  dtb-$(CONFIG_ARCH_MEDIATEK) += mt7622-bananapi-bpi-r64.dtb
 dtb-$(CONFIG_ARCH_MEDIATEK) += mt7981b-xiaomi-ax3000t.dtb
 dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-acelink-ew-7886cax.dtb
 dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3.dtb
+dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3-mini.dtb
 dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3-emmc.dtbo
 dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3-nand.dtbo
 dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3-nor.dtbo
diff --git a/arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3-mini.dts b/arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3-mini.dts
new file mode 100644
index 000000000000..c764b4dc4752
--- /dev/null
+++ b/arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3-mini.dts
@@ -0,0 +1,486 @@ 
+// SPDX-License-Identifier: (GPL-2.0 OR MIT)
+/*
+ * Copyright (C) 2021 MediaTek Inc.
+ * Authors: Frank Wunderlich <frank-w@public-files.de>
+ *          Eric Woudstra <ericwouds@gmail.com>
+ *          Tianling Shen <cnsztl@immortalwrt.org>
+ */
+
+/dts-v1/;
+
+#include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/input/input.h>
+#include <dt-bindings/leds/common.h>
+#include <dt-bindings/pinctrl/mt65xx.h>
+
+#include "mt7986a.dtsi"
+
+/ {
+	model = "Bananapi BPI-R3 Mini";
+	chassis-type = "embedded";
+	compatible = "bananapi,bpi-r3mini", "mediatek,mt7986a";
+
+	aliases {
+		serial0 = &uart0;
+		ethernet0 = &gmac0;
+		ethernet1 = &gmac1;
+	};
+
+	chosen {
+		stdout-path = "serial0:115200n8";
+	};
+
+	dcin: regulator-12vd {
+		compatible = "regulator-fixed";
+		regulator-name = "12vd";
+		regulator-min-microvolt = <12000000>;
+		regulator-max-microvolt = <12000000>;
+		regulator-boot-on;
+		regulator-always-on;
+	};
+
+	fan: pwm-fan {
+		compatible = "pwm-fan";
+		#cooling-cells = <2>;
+		/* cooling level (0, 1, 2) - pwm inverted */
+		cooling-levels = <255 96 0>;
+		pwms = <&pwm 0 10000>;
+		status = "okay";
+	};
+
+	reg_1p8v: regulator-1p8v {
+		compatible = "regulator-fixed";
+		regulator-name = "1.8vd";
+		regulator-min-microvolt = <1800000>;
+		regulator-max-microvolt = <1800000>;
+		regulator-boot-on;
+		regulator-always-on;
+		vin-supply = <&dcin>;
+	};
+
+	reg_3p3v: regulator-3p3v {
+		compatible = "regulator-fixed";
+		regulator-name = "3.3vd";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		regulator-boot-on;
+		regulator-always-on;
+		vin-supply = <&dcin>;
+	};
+
+	usb_vbus: regulator-usb-vbus {
+		compatible = "regulator-fixed";
+		regulator-name = "usb_vbus";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		gpios = <&pio 20 GPIO_ACTIVE_LOW>;
+		regulator-boot-on;
+	};
+
+	en8811_a: regulator-phy1 {
+		compatible = "regulator-fixed";
+		regulator-name = "phy1";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		gpio = <&pio 16 GPIO_ACTIVE_LOW>;
+		regulator-always-on;
+	};
+
+	en8811_b: regulator-phy2 {
+		compatible = "regulator-fixed";
+		regulator-name = "phy2";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		gpio = <&pio 17 GPIO_ACTIVE_LOW>;
+		regulator-always-on;
+	};
+
+	leds {
+		compatible = "gpio-leds";
+
+		green_led: led-0 {
+			color = <LED_COLOR_ID_GREEN>;
+			function = LED_FUNCTION_POWER;
+			gpios = <&pio 19 GPIO_ACTIVE_HIGH>;
+			default-state = "on";
+		};
+	};
+
+	gpio-keys {
+		compatible = "gpio-keys";
+
+		reset-key {
+			label = "reset";
+			linux,code = <KEY_RESTART>;
+			gpios = <&pio 7 GPIO_ACTIVE_LOW>;
+		};
+	};
+
+};
+
+&cpu_thermal {
+	cooling-maps {
+		map0 {
+			/* active: set fan to cooling level 2 */
+			cooling-device = <&fan 2 2>;
+			trip = <&cpu_trip_active_high>;
+		};
+
+		map1 {
+			/* active: set fan to cooling level 1 */
+			cooling-device = <&fan 1 1>;
+			trip = <&cpu_trip_active_med>;
+		};
+
+		map2 {
+			/* active: set fan to cooling level 0 */
+			cooling-device = <&fan 0 0>;
+			trip = <&cpu_trip_active_low>;
+		};
+	};
+};
+
+&crypto {
+	status = "okay";
+};
+
+&eth {
+	status = "okay";
+
+	gmac0: mac@0 {
+		compatible = "mediatek,eth-mac";
+		reg = <0>;
+		phy-mode = "2500base-x";
+		phy-handle = <&phy14>;
+	};
+
+	gmac1: mac@1 {
+		compatible = "mediatek,eth-mac";
+		reg = <1>;
+		phy-mode = "2500base-x";
+		phy-handle = <&phy15>;
+	};
+
+	mdio: mdio-bus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+	};
+};
+
+&mmc0 {
+	pinctrl-names = "default", "state_uhs";
+	pinctrl-0 = <&mmc0_pins_default>;
+	pinctrl-1 = <&mmc0_pins_uhs>;
+	vmmc-supply = <&reg_3p3v>;
+	vqmmc-supply = <&reg_1p8v>;
+};
+
+
+&i2c0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&i2c_pins>;
+	status = "okay";
+
+	/* MAC Address EEPROM */
+	eeprom@50 {
+		compatible = "atmel,24c02";
+		reg = <0x50>;
+
+		address-width = <8>;
+		pagesize = <8>;
+		size = <256>;
+	};
+};
+
+&mdio {
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	phy14: ethernet-phy@14 {
+		reg = <14>;
+		interrupts-extended = <&pio 48 IRQ_TYPE_EDGE_FALLING>;
+		reset-gpios = <&pio 49 GPIO_ACTIVE_LOW>;
+		reset-assert-us = <10000>;
+		reset-deassert-us = <20000>;
+		phy-mode = "2500base-x";
+		full-duplex;
+		pause;
+		airoha,pnswap-rx;
+
+		leds {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			led@0 { /* en8811_a_gpio5 */
+				reg = <0>;
+				color = <LED_COLOR_ID_YELLOW>;
+				function = LED_FUNCTION_LAN;
+				function-enumerator = <1>;
+				default-state = "keep";
+				linux,default-trigger = "netdev";
+			};
+			led@1 { /* en8811_a_gpio4 */
+				reg = <1>;
+				color = <LED_COLOR_ID_GREEN>;
+				function = LED_FUNCTION_LAN;
+				function-enumerator = <2>;
+				default-state = "keep";
+				linux,default-trigger = "netdev";
+			};
+		};
+	};
+
+	phy15: ethernet-phy@15 {
+		reg = <15>;
+		interrupts-extended = <&pio 46 IRQ_TYPE_EDGE_FALLING>;
+		reset-gpios = <&pio 47 GPIO_ACTIVE_LOW>;
+		reset-assert-us = <10000>;
+		reset-deassert-us = <20000>;
+		phy-mode = "2500base-x";
+		full-duplex;
+		pause;
+		airoha,pnswap-rx;
+
+		leds {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			led@0 { /* en8811_b_gpio5 */
+				reg = <0>;
+				color = <LED_COLOR_ID_YELLOW>;
+				function = LED_FUNCTION_WAN;
+				function-enumerator = <1>;
+				default-state = "keep";
+				linux,default-trigger = "netdev";
+			};
+			led@1 { /* en8811_b_gpio4 */
+				reg = <1>;
+				color = <LED_COLOR_ID_GREEN>;
+				function = LED_FUNCTION_WAN;
+				function-enumerator = <2>;
+				default-state = "keep";
+				linux,default-trigger = "netdev";
+			};
+		};
+	};
+};
+
+&pcie {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pcie_pins>;
+	status = "okay";
+};
+
+&pcie_phy {
+	status = "okay";
+};
+
+&pio {
+	i2c_pins: i2c-pins {
+		mux {
+			function = "i2c";
+			groups = "i2c";
+		};
+	};
+
+	mmc0_pins_default: mmc0-pins {
+		mux {
+			function = "emmc";
+			groups = "emmc_51";
+		};
+		conf-cmd-dat {
+			pins = "EMMC_DATA_0", "EMMC_DATA_1", "EMMC_DATA_2",
+			       "EMMC_DATA_3", "EMMC_DATA_4", "EMMC_DATA_5",
+			       "EMMC_DATA_6", "EMMC_DATA_7", "EMMC_CMD";
+			input-enable;
+			drive-strength = <4>;
+			bias-pull-up = <MTK_PUPD_SET_R1R0_01>; /* pull-up 10K */
+		};
+		conf-clk {
+			pins = "EMMC_CK";
+			drive-strength = <6>;
+			bias-pull-down = <MTK_PUPD_SET_R1R0_10>; /* pull-down 50K */
+		};
+		conf-ds {
+			pins = "EMMC_DSL";
+			bias-pull-down = <MTK_PUPD_SET_R1R0_10>; /* pull-down 50K */
+		};
+		conf-rst {
+			pins = "EMMC_RSTB";
+			drive-strength = <4>;
+			bias-pull-up = <MTK_PUPD_SET_R1R0_01>; /* pull-up 10K */
+		};
+	};
+
+	mmc0_pins_uhs: mmc0-uhs-pins {
+		mux {
+			function = "emmc";
+			groups = "emmc_51";
+		};
+		conf-cmd-dat {
+			pins = "EMMC_DATA_0", "EMMC_DATA_1", "EMMC_DATA_2",
+			       "EMMC_DATA_3", "EMMC_DATA_4", "EMMC_DATA_5",
+			       "EMMC_DATA_6", "EMMC_DATA_7", "EMMC_CMD";
+			input-enable;
+			drive-strength = <4>;
+			bias-pull-up = <MTK_PUPD_SET_R1R0_01>; /* pull-up 10K */
+		};
+		conf-clk {
+			pins = "EMMC_CK";
+			drive-strength = <6>;
+			bias-pull-down = <MTK_PUPD_SET_R1R0_10>; /* pull-down 50K */
+		};
+		conf-ds {
+			pins = "EMMC_DSL";
+			bias-pull-down = <MTK_PUPD_SET_R1R0_10>; /* pull-down 50K */
+		};
+		conf-rst {
+			pins = "EMMC_RSTB";
+			drive-strength = <4>;
+			bias-pull-up = <MTK_PUPD_SET_R1R0_01>; /* pull-up 10K */
+		};
+	};
+
+	pcie_pins: pcie-pins {
+		mux {
+			function = "pcie";
+			groups = "pcie_clk", "pcie_wake", "pcie_pereset";
+		};
+	};
+
+	pwm_pins: pwm-pins {
+		mux {
+			function = "pwm";
+			groups = "pwm0";
+		};
+	};
+
+	spi_flash_pins: spi-flash-pins {
+		mux {
+			function = "spi";
+			groups = "spi0", "spi0_wp_hold";
+		};
+	};
+
+	usb_ngff_pins: usb-ngff-pins {
+		ngff-gnss-off-conf {
+			pins = "GPIO_6";
+			drive-strength = <8>;
+			mediatek,pull-up-adv = <1>;
+		};
+		ngff-pe-rst-conf {
+			pins = "GPIO_7";
+			drive-strength = <8>;
+			mediatek,pull-up-adv = <1>;
+		};
+		ngff-wwan-off-conf {
+			pins = "GPIO_8";
+			drive-strength = <8>;
+			mediatek,pull-up-adv = <1>;
+		};
+		ngff-pwr-off-conf {
+			pins = "GPIO_9";
+			drive-strength = <8>;
+			mediatek,pull-up-adv = <1>;
+		};
+		ngff-rst-conf {
+			pins = "GPIO_10";
+			drive-strength = <8>;
+			mediatek,pull-up-adv = <1>;
+		};
+		ngff-coex-conf {
+			pins = "SPI1_CS";
+			drive-strength = <8>;
+			mediatek,pull-up-adv = <1>;
+		};
+	};
+
+	wf_2g_5g_pins: wf-2g-5g-pins {
+		mux {
+			function = "wifi";
+			groups = "wf_2g", "wf_5g";
+		};
+		conf {
+			pins = "WF0_HB1", "WF0_HB2", "WF0_HB3", "WF0_HB4",
+			       "WF0_HB0", "WF0_HB0_B", "WF0_HB5", "WF0_HB6",
+			       "WF0_HB7", "WF0_HB8", "WF0_HB9", "WF0_HB10",
+			       "WF0_TOP_CLK", "WF0_TOP_DATA", "WF1_HB1",
+			       "WF1_HB2", "WF1_HB3", "WF1_HB4", "WF1_HB0",
+			       "WF1_HB5", "WF1_HB6", "WF1_HB7", "WF1_HB8",
+			       "WF1_TOP_CLK", "WF1_TOP_DATA";
+			drive-strength = <4>;
+		};
+	};
+
+	wf_dbdc_pins: wf-dbdc-pins {
+		mux {
+			function = "wifi";
+			groups = "wf_dbdc";
+		};
+		conf {
+			pins = "WF0_HB1", "WF0_HB2", "WF0_HB3", "WF0_HB4",
+			       "WF0_HB0", "WF0_HB0_B", "WF0_HB5", "WF0_HB6",
+			       "WF0_HB7", "WF0_HB8", "WF0_HB9", "WF0_HB10",
+			       "WF0_TOP_CLK", "WF0_TOP_DATA", "WF1_HB1",
+			       "WF1_HB2", "WF1_HB3", "WF1_HB4", "WF1_HB0",
+			       "WF1_HB5", "WF1_HB6", "WF1_HB7", "WF1_HB8",
+			       "WF1_TOP_CLK", "WF1_TOP_DATA";
+			drive-strength = <4>;
+		};
+	};
+
+	wf_led_pins: wf-led-pins {
+		mux {
+			function = "led";
+			groups = "wifi_led";
+		};
+	};
+};
+
+&pwm {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pwm_pins>;
+	status = "okay";
+};
+
+&spi0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&spi_flash_pins>;
+	status = "okay";
+};
+
+&ssusb {
+	pinctrl-names = "default";
+	pinctrl-0 = <&usb_ngff_pins>;
+	vusb33-supply = <&reg_3p3v>;
+	vbus-supply = <&usb_vbus>;
+	status = "okay";
+};
+
+&trng {
+	status = "okay";
+};
+
+&uart0 {
+	status = "okay";
+};
+
+&usb_phy {
+	status = "okay";
+};
+
+&watchdog {
+	status = "okay";
+};
+
+&wifi {
+	status = "okay";
+	pinctrl-names = "default", "dbdc";
+	pinctrl-0 = <&wf_2g_5g_pins>, <&wf_led_pins>;
+	pinctrl-1 = <&wf_dbdc_pins>, <&wf_led_pins>;
+
+	led {
+		led-active-low;
+	};
+};
+