[v2,2/2] arm64: dts: mediatek: add Kontron 3.5"-SBC-i1200

  Add basic support for the Kontron 3.5" single board computer featuring a
Mediatek i1200 SoC (MT8395/MT8195).

Signed-off-by: Michael Walle <mwalle@kernel.org>
---
v2:
 - none

 arch/arm64/boot/dts/mediatek/Makefile         |    1 +
 .../mediatek/mt8395-kontron-3-5-sbc-i1200.dts | 1091 +++++++++++++++++
 2 files changed, 1092 insertions(+)
 create mode 100644 arch/arm64/boot/dts/mediatek/mt8395-kontron-3-5-sbc-i1200.dts
  

Il 19/02/24 09:44, Michael Walle ha scritto:
> Add basic support for the Kontron 3.5" single board computer featuring a
> Mediatek i1200 SoC (MT8395/MT8195).
> 
> Signed-off-by: Michael Walle <mwalle@kernel.org>
> ---
> v2:
>   - none
> 
>   arch/arm64/boot/dts/mediatek/Makefile         |    1 +
>   .../mediatek/mt8395-kontron-3-5-sbc-i1200.dts | 1091 +++++++++++++++++
>   2 files changed, 1092 insertions(+)
>   create mode 100644 arch/arm64/boot/dts/mediatek/mt8395-kontron-3-5-sbc-i1200.dts
> 
> diff --git a/arch/arm64/boot/dts/mediatek/Makefile b/arch/arm64/boot/dts/mediatek/Makefile
> index 37b4ca3a87c9..697b6b5de3cb 100644
> --- a/arch/arm64/boot/dts/mediatek/Makefile
> +++ b/arch/arm64/boot/dts/mediatek/Makefile
> @@ -76,5 +76,6 @@ dtb-$(CONFIG_ARCH_MEDIATEK) += mt8195-demo.dtb
>   dtb-$(CONFIG_ARCH_MEDIATEK) += mt8195-evb.dtb
>   dtb-$(CONFIG_ARCH_MEDIATEK) += mt8365-evk.dtb
>   dtb-$(CONFIG_ARCH_MEDIATEK) += mt8395-genio-1200-evk.dtb
> +dtb-$(CONFIG_ARCH_MEDIATEK) += mt8395-kontron-3-5-sbc-i1200.dtb
>   dtb-$(CONFIG_ARCH_MEDIATEK) += mt8395-radxa-nio-12l.dtb
>   dtb-$(CONFIG_ARCH_MEDIATEK) += mt8516-pumpkin.dtb
> diff --git a/arch/arm64/boot/dts/mediatek/mt8395-kontron-3-5-sbc-i1200.dts b/arch/arm64/boot/dts/mediatek/mt8395-kontron-3-5-sbc-i1200.dts
> new file mode 100644
> index 000000000000..0c634bc8776c
> --- /dev/null
> +++ b/arch/arm64/boot/dts/mediatek/mt8395-kontron-3-5-sbc-i1200.dts
> @@ -0,0 +1,1091 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/*
> + * Copyright (C) 2024 Kontron Europe GmbH
> + *
> + * Author: Michael Walle <mwalle@kernel.org>
> + */
> +/dts-v1/;
> +
> +#include "mt8195.dtsi"
> +#include "mt6359.dtsi"
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/input/input.h>
> +#include <dt-bindings/leds/common.h>
> +#include <dt-bindings/pinctrl/mt8195-pinfunc.h>
> +#include <dt-bindings/regulator/mediatek,mt6360-regulator.h>
> +#include <dt-bindings/spmi/spmi.h>
> +
> +/ {
> +	model = "Kontron 3.5\"-SBC-i1200";
> +	compatible = "kontron,3-5-sbc-i1200", "mediatek,mt8395", "mediatek,mt8195";
> +
> +	aliases {
> +		mmc0 = &mmc0;
> +		mmc1 = &mmc1;
> +		serial0 = &uart1;
> +		serial1 = &uart2;
> +		serial2 = &uart3;
> +		serial3 = &uart4;
> +		serial4 = &uart0;
> +	};
> +
> +	chosen {
> +		stdout-path = "serial0:115200n8";
> +	};
> +
> +	firmware {
> +		optee {
> +			compatible = "linaro,optee-tz";
> +			method = "smc";
> +		};
> +	};
> +
> +	gpio-keys {
> +		compatible = "gpio-keys";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&gpio_keys_pins>;
> +
> +		key-0 {
> +			gpios = <&pio 106 GPIO_ACTIVE_LOW>;
> +			label = "volume_up";
> +			linux,code = <KEY_VOLUMEUP>;
> +			wakeup-source;
> +			debounce-interval = <15>;
> +		};
> +	};
> +
> +	leds {
> +		compatible = "gpio-leds";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&led_pins>;
> +
> +		led-0 {
> +			gpios = <&pio 107 GPIO_ACTIVE_HIGH>;
> +			default-state = "keep";
> +			function = LED_FUNCTION_POWER;
> +			color = <LED_COLOR_ID_GREEN>;
> +		};
> +	};
> +
> +	memory@40000000 {
> +		device_type = "memory";
> +		reg = <0 0x40000000 0x0 0x80000000>;
> +	};
> +
> +	reserved-memory {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		/*
> +		 * 12 MiB reserved for OP-TEE (BL32)
> +		 * +-----------------------+ 0x43e0_0000
> +		 * |      SHMEM 2MiB       |
> +		 * +-----------------------+ 0x43c0_0000
> +		 * |        | TA_RAM  8MiB |
> +		 * + TZDRAM +--------------+ 0x4340_0000
> +		 * |        | TEE_RAM 2MiB |
> +		 * +-----------------------+ 0x4320_0000
> +		 */
> +		optee_reserved: optee@43200000 {
> +			no-map;
> +			reg = <0 0x43200000 0 0x00c00000>;
> +		};
> +
> +		scp_mem: memory@50000000 {
> +			compatible = "shared-dma-pool";
> +			reg = <0 0x50000000 0 0x2900000>;
> +			no-map;
> +		};
> +
> +		vpu_mem: memory@53000000 {
> +			compatible = "shared-dma-pool";
> +			reg = <0 0x53000000 0 0x1400000>; /* 20 MB */
> +		};
> +
> +		/* 2 MiB reserved for ARM Trusted Firmware (BL31) */
> +		bl31_secmon_mem: memory@54600000 {
> +			no-map;
> +			reg = <0 0x54600000 0x0 0x200000>;
> +		};
> +
> +		snd_dma_mem: memory@60000000 {
> +			compatible = "shared-dma-pool";
> +			reg = <0 0x60000000 0 0x1100000>;
> +			no-map;
> +		};
> +
> +		apu_mem: memory@62000000 {
> +			compatible = "shared-dma-pool";
> +			reg = <0 0x62000000 0 0x1400000>; /* 20 MB */
> +		};
> +	};
> +
> +	thermal_sensor0: thermal-sensor-0 {
> +		compatible = "generic-adc-thermal";
> +		#thermal-sensor-cells = <0>;
> +		io-channels = <&auxadc 0>;
> +		io-channel-names = "sensor-channel";
> +		temperature-lookup-table = <(-25000) 1474
> +					    (-20000) 1374
> +					    (-15000) 1260
> +					    (-10000) 1134
> +					     (-5000) 1004
> +						   0 874
> +						5000 750
> +					       10000 635
> +					       15000 532
> +					       20000 443
> +					       25000 367
> +					       30000 303
> +					       35000 250
> +					       40000 206
> +					       45000 170
> +					       50000 141
> +					       55000 117
> +					       60000 97
> +					       65000 81
> +					       70000 68
> +					       75000 57
> +					       80000 48
> +					       85000 41
> +					       90000 35
> +					       95000 30
> +					      100000 25
> +					      105000 22
> +					      110000 19
> +					      115000 16
> +					      120000 14
> +					      125000 12
> +					      130000 10
> +					      135000 9
> +					      140000 8
> +					      145000 7
> +					      150000 6>;
> +	};
> +
> +	thermal_sensor1: thermal-sensor-1 {
> +		compatible = "generic-adc-thermal";
> +		#thermal-sensor-cells = <0>;
> +		io-channels = <&auxadc 1>;
> +		io-channel-names = "sensor-channel";
> +		temperature-lookup-table = <(-25000) 1474
> +					    (-20000) 1374
> +					    (-15000) 1260
> +					    (-10000) 1134
> +					     (-5000) 1004
> +						   0 874
> +						5000 750
> +					       10000 635
> +					       15000 532
> +					       20000 443
> +					       25000 367
> +					       30000 303
> +					       35000 250
> +					       40000 206
> +					       45000 170
> +					       50000 141
> +					       55000 117
> +					       60000 97
> +					       65000 81
> +					       70000 68
> +					       75000 57
> +					       80000 48
> +					       85000 41
> +					       90000 35
> +					       95000 30
> +					      100000 25
> +					      105000 22
> +					      110000 19
> +					      115000 16
> +					      120000 14
> +					      125000 12
> +					      130000 10
> +					      135000 9
> +					      140000 8
> +					      145000 7
> +					      150000 6>;
> +	};
> +
> +	thermal_sensor2: thermal-sensor-2 {
> +		compatible = "generic-adc-thermal";
> +		#thermal-sensor-cells = <0>;
> +		io-channels = <&auxadc 2>;
> +		io-channel-names = "sensor-channel";
> +		temperature-lookup-table = <(-25000) 1474
> +					    (-20000) 1374
> +					    (-15000) 1260
> +					    (-10000) 1134
> +					     (-5000) 1004
> +						   0 874
> +						5000 750
> +					       10000 635
> +					       15000 532
> +					       20000 443
> +					       25000 367
> +					       30000 303
> +					       35000 250
> +					       40000 206
> +					       45000 170
> +					       50000 141
> +					       55000 117
> +					       60000 97
> +					       65000 81
> +					       70000 68
> +					       75000 57
> +					       80000 48
> +					       85000 41
> +					       90000 35
> +					       95000 30
> +					      100000 25
> +					      105000 22
> +					      110000 19
> +					      115000 16
> +					      120000 14
> +					      125000 12
> +					      130000 10
> +					      135000 9
> +					      140000 8
> +					      145000 7
> +					      150000 6>;
> +	};
> +};
> +
> +&auxadc {
> +	status = "okay";
> +};
> +
> +&eth {
> +	phy-mode ="rgmii-id";
> +	phy-handle = <&ethernet_phy0>;
> +	snps,reset-gpio = <&pio 93 GPIO_ACTIVE_HIGH>;
> +	snps,reset-delays-us = <0 10000 80000>;

snps,reset-delays-us and snps,reset-gpio are deprecated.

> +	pinctrl-names = "default", "sleep";
> +	pinctrl-0 = <&eth_default_pins>;
> +	pinctrl-1 = <&eth_sleep_pins>;
> +	status = "okay";
> +
> +	mdio {
> +		ethernet_phy0: ethernet-phy@1 {

compatible = "is there any applicable compatible?"
P.S.: if you've got the usual rtl8211f, should be "ethernet-phy-id001c.c916"

reg = <0x1>;
interrupts-extended = <&pio 94 IRQ_TYPE_LEVEL_LOW>;
reset-assert-us = <10000>;
reset-deassert-us = <80000>;
reset-gpios = <&pio 93 GPIO_ACTIVE_HIGH>;


> +			reg = <0x1>;
> +			interrupts-extended = <&pio 94 IRQ_TYPE_LEVEL_LOW>;
> +		};
> +	};
> +};
> +
> +&gpu {
> +	status = "okay";
> +	mali-supply = <&mt6315_7_vbuck1>;
> +};
> +
> +&i2c2 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&i2c2_pins>;
> +	clock-frequency = <400000>;
> +	status = "okay";

Are i2c2,3,4 exposed as pins somewhere? If they are, can you please put a
comment saying so?

This is not mandatory, but IMO it's nice to have as a kind of documentation.

> +};
> +
> +&i2c3 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&i2c3_pins>;
> +	clock-frequency = <100000>;
> +	status = "okay";
> +};
> +
> +&i2c4 {
> +	clock-frequency = <100000>;
> +	pinctrl-0 = <&i2c4_pins>;
> +	pinctrl-names = "default";
> +	status = "okay";
> +};
> +
> +&i2c6 {
> +	clock-frequency = <400000>;
> +	pinctrl-0 = <&i2c6_pins>;
> +	pinctrl-names = "default";
> +	status = "okay";
> +
> +	mt6360: pmic@34 {
> +		compatible = "mediatek,mt6360";
> +		reg = <0x34>;
> +		interrupt-controller;
> +		interrupts-extended = <&pio 101 IRQ_TYPE_EDGE_FALLING>;
> +		interrupt-names = "IRQB";

#interrupt-cells = <1>;

> +
> +		charger {
> +			compatible = "mediatek,mt6360-chg";
> +			richtek,vinovp-microvolt = <14500000>;
> +
> +			otg_vbus_regulator: usb-otg-vbus-regulator {
> +				regulator-compatible = "usb-otg-vbus";

The regulator-compatible property is deprecated. Please don't use it.

> +				regulator-name = "usb-otg-vbus";
> +				regulator-min-microvolt = <4425000>;
> +				regulator-max-microvolt = <5825000>;
> +			};
> +		};
> +
> +		regulator {
> +			compatible = "mediatek,mt6360-regulator";
> +			LDO_VIN3-supply = <&mt6360_buck2>;
> +
> +			mt6360_buck1: buck1 {
> +				regulator-compatible = "BUCK1";

same here and everywhere else.

> +				regulator-name = "mt6360,buck1";
> +				regulator-min-microvolt = <300000>;
> +				regulator-max-microvolt = <1300000>;
> +				regulator-allowed-modes = <MT6360_OPMODE_NORMAL
> +							   MT6360_OPMODE_LP
> +							   MT6360_OPMODE_ULP>;
> +				regulator-always-on;
> +			};
> +
> +			mt6360_buck2: buck2 {
> +				regulator-compatible = "BUCK2";
> +				regulator-name = "mt6360,buck2";
> +				regulator-min-microvolt = <300000>;
> +				regulator-max-microvolt = <1300000>;
> +				regulator-allowed-modes = <MT6360_OPMODE_NORMAL
> +							   MT6360_OPMODE_LP
> +							   MT6360_OPMODE_ULP>;
> +				regulator-always-on;
> +			};
> +
> +			mt6360_ldo1: ldo1 {
> +				regulator-compatible = "LDO1";
> +				regulator-name = "mt6360,ldo1";
> +				regulator-min-microvolt = <1200000>;
> +				regulator-max-microvolt = <3600000>;
> +				regulator-allowed-modes = <MT6360_OPMODE_NORMAL
> +							   MT6360_OPMODE_LP>;
> +			};
> +
> +			mt6360_ldo2: ldo2 {
> +				regulator-compatible = "LDO2";
> +				regulator-name = "mt6360,ldo2";
> +				regulator-min-microvolt = <1200000>;
> +				regulator-max-microvolt = <3600000>;
> +				regulator-allowed-modes = <MT6360_OPMODE_NORMAL
> +							   MT6360_OPMODE_LP>;
> +			};
> +
> +			mt6360_ldo3: ldo3 {
> +				regulator-compatible = "LDO3";
> +				regulator-name = "mt6360,ldo3";
> +				regulator-min-microvolt = <1200000>;
> +				regulator-max-microvolt = <3600000>;
> +				regulator-allowed-modes = <MT6360_OPMODE_NORMAL
> +							   MT6360_OPMODE_LP>;
> +			};
> +
> +			mt6360_ldo5: ldo5 {
> +				regulator-compatible = "LDO5";
> +				regulator-name = "mt6360,ldo5";
> +				regulator-min-microvolt = <2700000>;
> +				regulator-max-microvolt = <3600000>;
> +				regulator-allowed-modes = <MT6360_OPMODE_NORMAL
> +							   MT6360_OPMODE_LP>;
> +			};
> +
> +			mt6360_ldo6: ldo6 {
> +				regulator-compatible = "LDO6";
> +				regulator-name = "mt6360,ldo6";
> +				regulator-min-microvolt = <500000>;
> +				regulator-max-microvolt = <2100000>;
> +				regulator-allowed-modes = <MT6360_OPMODE_NORMAL
> +							   MT6360_OPMODE_LP>;
> +			};
> +
> +			mt6360_ldo7: ldo7 {
> +				regulator-compatible = "LDO7";
> +				regulator-name = "mt6360,ldo7";
> +				regulator-min-microvolt = <500000>;
> +				regulator-max-microvolt = <2100000>;
> +				regulator-allowed-modes = <MT6360_OPMODE_NORMAL
> +							   MT6360_OPMODE_LP>;
> +				regulator-always-on;
> +			};
> +		};
> +	};
> +};
> +
> +&mmc0 {

This is not wrong, but can we please keep the ordering consistent between boards?

	pinctrl-names = "default", "state_uhs";
	pinctrl-0 = <&mmc0_default_pins>;
	pinctrl-1 = <&mmc0_uhs_pins>;
	bus-width = <8>;
	max-frequency = <200000000>;
	hs400-ds-delay = <0x14c11>;
	cap-mmc-highspeed;
	cap-mmc-hw-reset;
	mmc-hs200-1_8v;
	mmc-hs400-1_8v;
	no-sdio;
	no-sd;
	non-removable;
	vmmc-supply = <&mt6359_vemc_1_ldo_reg>;
	vqmmc-supply = <&mt6359_vufs_ldo_reg>;

> +	pinctrl-names = "default", "state_uhs";
> +	pinctrl-0 = <&mmc0_default_pins>;
> +	pinctrl-1 = <&mmc0_uhs_pins>;
> +	bus-width = <8>;
> +	max-frequency = <200000000>;
> +	cap-mmc-highspeed;
> +	mmc-hs200-1_8v;
> +	mmc-hs400-1_8v;
> +	cap-mmc-hw-reset;
> +	no-sdio;
> +	no-sd;
> +	hs400-ds-delay = <0x14c11>;
> +	vmmc-supply = <&mt6359_vemc_1_ldo_reg>;
> +	vqmmc-supply = <&mt6359_vufs_ldo_reg>;
> +	non-removable;
> +	status = "okay";
> +};
> +
> +&mmc1 {
> +	pinctrl-names = "default", "state_uhs";
> +	pinctrl-0 = <&mmc1_default_pins>;
> +	pinctrl-1 = <&mmc1_uhs_pins>;
> +	cd-gpios = <&pio 129 GPIO_ACTIVE_LOW>;
> +	bus-width = <4>;
> +	max-frequency = <200000000>;
> +	cap-sd-highspeed;
> +	sd-uhs-sdr50;
> +	sd-uhs-sdr104;
> +	vmmc-supply = <&mt6360_ldo5>;
> +	vqmmc-supply = <&mt6360_ldo3>;

Does mmc1 support eMMC and SDIO?

If not, no-mmc; no-sdio;

> +	status = "okay";
> +};
> +
> +&mt6359_vbbck_ldo_reg {
> +	regulator-always-on;
> +};
> +
> +&mt6359_vcore_buck_reg {
> +	regulator-always-on;
> +};
> +
> +&mt6359_vgpu11_buck_reg {
> +	regulator-always-on;
> +};
> +
> +&mt6359_vproc1_buck_reg {
> +	regulator-always-on;
> +};
> +
> +&mt6359_vproc2_buck_reg {
> +	regulator-always-on;
> +};
> +
> +&mt6359_vpu_buck_reg {
> +	regulator-always-on;
> +};
> +
> +&mt6359_vrf12_ldo_reg {
> +	regulator-always-on;
> +};
> +
> +&mt6359_vsram_md_ldo_reg {
> +	regulator-always-on;
> +};
> +
> +&mt6359_vsram_others_ldo_reg {
> +	regulator-always-on;
> +};
> +
> +&nor_flash {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&nor_pins_default>;
> +	status = "okay";
> +
> +	flash@0 {
> +		compatible = "jedec,spi-nor";
> +		reg = <0>;
> +		spi-max-frequency = <52000000>;
> +		spi-rx-bus-width = <2>;
> +		spi-tx-bus-width = <2>;
> +	};
> +};
> +
> +&pcie0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pcie0_pins_default>;
> +	status = "okay";
> +};
> +
> +&pcie1 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pcie1_pins_default>;
> +	status = "okay";
> +};
> +
> +&pciephy {
> +	status = "okay";
> +};
> +
> +&pio {
> +	eth_default_pins: eth-default-pins {
> +		pins-txd {
> +			pinmux = <PINMUX_GPIO77__FUNC_GBE_TXD3>,
> +				 <PINMUX_GPIO78__FUNC_GBE_TXD2>,
> +				 <PINMUX_GPIO79__FUNC_GBE_TXD1>,
> +				 <PINMUX_GPIO80__FUNC_GBE_TXD0>;
> +			drive-strength = <MTK_DRIVE_8mA>;
> +		};
> +
> +		pins-rxd {
> +			pinmux = <PINMUX_GPIO81__FUNC_GBE_RXD3>,
> +				 <PINMUX_GPIO82__FUNC_GBE_RXD2>,
> +				 <PINMUX_GPIO83__FUNC_GBE_RXD1>,
> +				 <PINMUX_GPIO84__FUNC_GBE_RXD0>;
> +		};
> +
> +		pins-cc {
> +			pinmux = <PINMUX_GPIO85__FUNC_GBE_TXC>,
> +				 <PINMUX_GPIO86__FUNC_GBE_RXC>,
> +				 <PINMUX_GPIO87__FUNC_GBE_RXDV>,
> +				 <PINMUX_GPIO88__FUNC_GBE_TXEN>;
> +			drive-strength = <MTK_DRIVE_8mA>;

s/MTK_DRIVE//g
s/mA//g

drive-strength = <8>;

Please, here and everywhere else, for all values - let's stop using those
MTK_DRIVE_(x)mA definitions, they're just defined as (x), where anyway
the drive-strength property is in milliamps by default.

We don't need these definitions.

> +		};
> +
> +		pins-mdio {
> +			pinmux = <PINMUX_GPIO89__FUNC_GBE_MDC>,
> +				 <PINMUX_GPIO90__FUNC_GBE_MDIO>;
> +			input-enable;
> +		};
> +
> +		pins-power {
> +			pinmux = <PINMUX_GPIO91__FUNC_GPIO91>,
> +				 <PINMUX_GPIO92__FUNC_GPIO92>;
> +			output-high;
> +		};
> +
> +		pins-reset {
> +			pinmux = <PINMUX_GPIO93__FUNC_GPIO93>;
> +			output-high;
> +		};
> +
> +		pins-interrupt {
> +			pinmux = <PINMUX_GPIO94__FUNC_GPIO94>;
> +			input-enable;
> +		};
> +	};
> +
> +	eth_sleep_pins: eth-sleep-pins {
> +		pins-txd {
> +			pinmux = <PINMUX_GPIO77__FUNC_GPIO77>,
> +				 <PINMUX_GPIO78__FUNC_GPIO78>,
> +				 <PINMUX_GPIO79__FUNC_GPIO79>,
> +				 <PINMUX_GPIO80__FUNC_GPIO80>;
> +		};
> +		pins-cc {
> +			pinmux = <PINMUX_GPIO85__FUNC_GPIO85>,
> +				 <PINMUX_GPIO88__FUNC_GPIO88>,
> +				 <PINMUX_GPIO87__FUNC_GPIO87>,
> +				 <PINMUX_GPIO86__FUNC_GPIO86>;
> +		};
> +		pins-rxd {
> +			pinmux = <PINMUX_GPIO81__FUNC_GPIO81>,
> +				 <PINMUX_GPIO82__FUNC_GPIO82>,
> +				 <PINMUX_GPIO83__FUNC_GPIO83>,
> +				 <PINMUX_GPIO84__FUNC_GPIO84>;
> +		};
> +		pins-mdio {
> +			pinmux = <PINMUX_GPIO89__FUNC_GPIO89>,
> +				 <PINMUX_GPIO90__FUNC_GPIO90>;
> +			input-disable;
> +			bias-disable;
> +		};
> +	};
> +
> +	gpio_keys_pins: gpio-keys-pins {
> +		pins {
> +			pinmux = <PINMUX_GPIO106__FUNC_GPIO106>;
> +			input-enable;
> +		};
> +	};
> +
> +	i2c2_pins: i2c2-default-pins {
> +		pins-bus {
> +			pinmux = <PINMUX_GPIO12__FUNC_SDA2>,
> +				 <PINMUX_GPIO13__FUNC_SCL2>;
> +			bias-pull-up;
> +		};
> +	};
> +
> +	i2c3_pins: i2c3-pins {
> +		pins {
> +			pinmux = <PINMUX_GPIO14__FUNC_SDA3>,
> +				 <PINMUX_GPIO15__FUNC_SCL3>;
> +			bias-pull-up;
> +		};
> +	};
> +
> +	i2c4_pins: i2c4-pins {
> +		pins {
> +			pinmux = <PINMUX_GPIO16__FUNC_SDA4>,
> +				 <PINMUX_GPIO17__FUNC_SCL4>;
> +			bias-pull-up;
> +		};
> +	};
> +
> +	i2c6_pins: i2c6-pins {
> +		pins {
> +			pinmux = <PINMUX_GPIO25__FUNC_SDA6>,
> +				 <PINMUX_GPIO26__FUNC_SCL6>;
> +			bias-pull-up;
> +		};
> +	};
> +
> +	mmc0_default_pins: mmc0-default-pins {
> +		pins-clk {
> +			pinmux = <PINMUX_GPIO122__FUNC_MSDC0_CLK>;
> +			drive-strength = <MTK_DRIVE_6mA>;

drive-strength = <6>; etc :-)

> +			bias-pull-down = <MTK_PUPD_SET_R1R0_10>;
> +		};
> +
> +		pins-cmd-dat {
> +			pinmux = <PINMUX_GPIO126__FUNC_MSDC0_DAT0>,
> +				 <PINMUX_GPIO125__FUNC_MSDC0_DAT1>,
> +				 <PINMUX_GPIO124__FUNC_MSDC0_DAT2>,
> +				 <PINMUX_GPIO123__FUNC_MSDC0_DAT3>,
> +				 <PINMUX_GPIO119__FUNC_MSDC0_DAT4>,
> +				 <PINMUX_GPIO118__FUNC_MSDC0_DAT5>,
> +				 <PINMUX_GPIO117__FUNC_MSDC0_DAT6>,
> +				 <PINMUX_GPIO116__FUNC_MSDC0_DAT7>,
> +				 <PINMUX_GPIO121__FUNC_MSDC0_CMD>;
> +			input-enable;
> +			drive-strength = <MTK_DRIVE_6mA>;
> +			bias-pull-up = <MTK_PUPD_SET_R1R0_01>;
> +		};
> +
> +		pins-rst {
> +			pinmux = <PINMUX_GPIO120__FUNC_MSDC0_RSTB>;
> +			drive-strength = <MTK_DRIVE_6mA>;
> +			bias-pull-up = <MTK_PUPD_SET_R1R0_01>;
> +		};
> +	};
> +
> +	mmc0_uhs_pins: mmc0-uhs-pins {
> +		pins-clk {
> +			pinmux = <PINMUX_GPIO122__FUNC_MSDC0_CLK>;
> +			drive-strength = <MTK_DRIVE_8mA>;
> +			bias-pull-down = <MTK_PUPD_SET_R1R0_10>;
> +		};
> +
> +		pins-cmd-dat {
> +			pinmux = <PINMUX_GPIO126__FUNC_MSDC0_DAT0>,
> +				 <PINMUX_GPIO125__FUNC_MSDC0_DAT1>,
> +				 <PINMUX_GPIO124__FUNC_MSDC0_DAT2>,
> +				 <PINMUX_GPIO123__FUNC_MSDC0_DAT3>,
> +				 <PINMUX_GPIO119__FUNC_MSDC0_DAT4>,
> +				 <PINMUX_GPIO118__FUNC_MSDC0_DAT5>,
> +				 <PINMUX_GPIO117__FUNC_MSDC0_DAT6>,
> +				 <PINMUX_GPIO116__FUNC_MSDC0_DAT7>,
> +				 <PINMUX_GPIO121__FUNC_MSDC0_CMD>;
> +			input-enable;
> +			drive-strength = <MTK_DRIVE_8mA>;
> +			bias-pull-up = <MTK_PUPD_SET_R1R0_01>;
> +		};
> +
> +		pins-ds {
> +			pinmux = <PINMUX_GPIO127__FUNC_MSDC0_DSL>;
> +			drive-strength = <MTK_DRIVE_8mA>;
> +			bias-pull-down = <MTK_PUPD_SET_R1R0_10>;
> +		};
> +
> +		pins-rst {
> +			pinmux = <PINMUX_GPIO120__FUNC_MSDC0_RSTB>;
> +			drive-strength = <MTK_DRIVE_8mA>;
> +			bias-pull-up = <MTK_PUPD_SET_R1R0_01>;
> +		};
> +	};
> +
> +	mmc1_default_pins: mmc1-default-pins {
> +		pins-clk {
> +			pinmux = <PINMUX_GPIO111__FUNC_MSDC1_CLK>;
> +			drive-strength = <MTK_DRIVE_8mA>;
> +			bias-pull-down = <MTK_PUPD_SET_R1R0_10>;
> +		};
> +
> +		pins-cmd-dat {
> +			pinmux = <PINMUX_GPIO110__FUNC_MSDC1_CMD>,
> +				 <PINMUX_GPIO112__FUNC_MSDC1_DAT0>,
> +				 <PINMUX_GPIO113__FUNC_MSDC1_DAT1>,
> +				 <PINMUX_GPIO114__FUNC_MSDC1_DAT2>,
> +				 <PINMUX_GPIO115__FUNC_MSDC1_DAT3>;
> +			input-enable;
> +			drive-strength = <MTK_DRIVE_8mA>;
> +			bias-pull-up = <MTK_PUPD_SET_R1R0_01>;
> +		};
> +
> +		pins-insert {
> +			pinmux = <PINMUX_GPIO129__FUNC_GPIO129>;
> +			bias-pull-up;
> +		};
> +	};

MMC1 UHS and DEFAULT are exactly the same, you don't need to define those twice.
Here's how we can clean up this block:

	mmc1_default_pins: mmc1-default-pins {
		pins-clk {
			pinmux = <PINMUX_GPIO111__FUNC_MSDC1_CLK>;
			bias-pull-down = <MTK_PUPD_SET_R1R0_10>;
			drive-strength = <8>;
		};

		pins-cmd-dat {
			pinmux = <PINMUX_GPIO110__FUNC_MSDC1_CMD>,
				 <PINMUX_GPIO112__FUNC_MSDC1_DAT0>,
				 <PINMUX_GPIO113__FUNC_MSDC1_DAT1>,
				 <PINMUX_GPIO114__FUNC_MSDC1_DAT2>,
				 <PINMUX_GPIO115__FUNC_MSDC1_DAT3>;
			bias-pull-up = <MTK_PUPD_SET_R1R0_01>;
			drive-strength = <8>;
			input-enable;
		};
	};

	mmc1_pins_detect: mmc1-detect-pins {
		pins-insert {
			pinmux = <PINMUX_GPIO129__FUNC_GPIO129>;
			bias-pull-up;
		};
	};

&mmc1 {
	pinctrl-names = "default", "state_uhs";
	pinctrl-0 = <&mmc1_default_pins>, <&mmc1_pins_detect>;
	pinctrl-1 = <&mmc1_default_pins>;
	....
}


> +
> +	mmc1_uhs_pins: mmc1-uhs-pins {
> +		pins-clk {
> +			pinmux = <PINMUX_GPIO111__FUNC_MSDC1_CLK>;
> +			drive-strength = <MTK_DRIVE_8mA>;
> +			bias-pull-down = <MTK_PUPD_SET_R1R0_10>;
> +		};
> +
> +		pins-cmd-dat {
> +			pinmux = <PINMUX_GPIO110__FUNC_MSDC1_CMD>,
> +				 <PINMUX_GPIO112__FUNC_MSDC1_DAT0>,
> +				 <PINMUX_GPIO113__FUNC_MSDC1_DAT1>,
> +				 <PINMUX_GPIO114__FUNC_MSDC1_DAT2>,
> +				 <PINMUX_GPIO115__FUNC_MSDC1_DAT3>;
> +			input-enable;
> +			drive-strength = <MTK_DRIVE_8mA>;
> +			bias-pull-up = <MTK_PUPD_SET_R1R0_01>;
> +		};
> +	};
> +
> +	nor_pins_default: nor-default-pins {
> +		pins-ck-io {
> +			pinmux = <PINMUX_GPIO142__FUNC_SPINOR_IO0>,
> +				 <PINMUX_GPIO141__FUNC_SPINOR_CK>,
> +				 <PINMUX_GPIO143__FUNC_SPINOR_IO1>;
> +			drive-strength = <6>;
> +			bias-pull-down;
> +		};
> +
> +		pins-cs {
> +			pinmux = <PINMUX_GPIO140__FUNC_SPINOR_CS>;
> +			drive-strength = <6>;
> +			bias-pull-up;
> +		};
> +	};
> +
> +	pcie0_pins_default: pcie0-default-pins {
> +		pins-bus {
> +			pinmux = <PINMUX_GPIO19__FUNC_WAKEN>,
> +				 <PINMUX_GPIO20__FUNC_PERSTN>,
> +				 <PINMUX_GPIO21__FUNC_CLKREQN>;
> +			bias-pull-up;
> +		};
> +	};
> +
> +	pcie1_pins_default: pcie1-default-pins {
> +		pins-bus {
> +			pinmux = <PINMUX_GPIO0__FUNC_PERSTN_1>,
> +				 <PINMUX_GPIO1__FUNC_CLKREQN_1>,
> +				 <PINMUX_GPIO2__FUNC_WAKEN_1>;
> +			bias-pull-up = <MTK_PUPD_SET_R1R0_01>;
> +		};
> +	};
> +
> +	led_pins: led-pins {
> +		pins-power-en {
> +			pinmux = <PINMUX_GPIO107__FUNC_GPIO107>;
> +			output-high;
> +		};
> +	};
> +
> +	spi0_pins: spi0-default-pins {
> +		pins-cs-mosi-clk {
> +			pinmux = <PINMUX_GPIO132__FUNC_SPIM0_CSB>,
> +				 <PINMUX_GPIO134__FUNC_SPIM0_MO>,
> +				 <PINMUX_GPIO133__FUNC_SPIM0_CLK>;
> +			bias-disable;
> +		};
> +
> +		pins-miso {
> +			pinmux = <PINMUX_GPIO135__FUNC_SPIM0_MI>;
> +			bias-pull-down;
> +		};
> +	};
> +
> +	spi1_pins: spi1-default-pins {
> +		pins-cs-mosi-clk {
> +			pinmux = <PINMUX_GPIO136__FUNC_SPIM1_CSB>,
> +				 <PINMUX_GPIO138__FUNC_SPIM1_MO>,
> +				 <PINMUX_GPIO137__FUNC_SPIM1_CLK>;
> +			bias-disable;
> +		};
> +
> +		pins-miso {
> +			pinmux = <PINMUX_GPIO139__FUNC_SPIM1_MI>;
> +			bias-pull-down;
> +		};
> +	};
> +
> +	uart0_pins: uart0-pins {
> +		pins_rx {

No underscores, please.

> +			pinmux = <PINMUX_GPIO99__FUNC_URXD0>;
> +			input-enable;
> +			bias-pull-up;
> +		};
> +
> +		pins_tx {
> +			pinmux = <PINMUX_GPIO98__FUNC_UTXD0>;
> +		};
> +	};
> +
> +	uart1_pins: uart1-pins {
> +		pins_rx {
> +			pinmux = <PINMUX_GPIO103__FUNC_URXD1>;
> +			input-enable;
> +			bias-pull-up;
> +		};
> +
> +		pins_tx {
> +			pinmux = <PINMUX_GPIO102__FUNC_UTXD1>;
> +		};
> +
> +		pins_rts {
> +			pinmux = <PINMUX_GPIO100__FUNC_URTS1>;
> +			output-enable;

Are you really sure that you need output-enable here?!
RTS is not an output buffer....

I don't think you do. Please double check.

> +		};
> +
> +		pins_cts {
> +			pinmux = <PINMUX_GPIO101__FUNC_UCTS1>;
> +			input-enable;
> +		};
> +	};
> +
> +	uart2_pins: uart2-pins {
> +		pins_rx {
> +			pinmux = <PINMUX_GPIO68__FUNC_URXD2>;
> +			input-enable;
> +			bias-pull-up;
> +		};
> +
> +		pins_tx {
> +			pinmux = <PINMUX_GPIO67__FUNC_UTXD2>;
> +		};
> +
> +		pins_rts {
> +			pinmux = <PINMUX_GPIO66__FUNC_URTS2>;
> +			output-enable;
> +		};
> +
> +		pins_cts {
> +			pinmux = <PINMUX_GPIO65__FUNC_UCTS2>;
> +			input-enable;
> +		};
> +	};
> +
> +	uart3_pins: uart3-pins {
> +		pins_rx {
> +			pinmux = <PINMUX_GPIO5__FUNC_URXD3>;
> +			input-enable;
> +			bias-pull-up = <MTK_PUPD_SET_R1R0_01>;
> +		};
> +
> +		pins_tx {
> +			pinmux = <PINMUX_GPIO4__FUNC_UTXD3>;
> +		};
> +	};
> +
> +	uart4_pins: uart4-pins {
> +		pins_rx {
> +			pinmux = <PINMUX_GPIO7__FUNC_URXD4>;
> +			input-enable;
> +			bias-pull-up;
> +		};
> +
> +		pins_tx {
> +			pinmux = <PINMUX_GPIO6__FUNC_UTXD4>;
> +		};
> +	};
> +};
> +
> +&pmic {
> +	interrupts-extended = <&pio 222 IRQ_TYPE_LEVEL_HIGH>;
> +};
> +
> +&scp {
> +	memory-region = <&scp_mem>;
> +	status = "okay";
> +};
> +
> +&spmi {
> +	#address-cells = <2>;
> +	#size-cells = <0>;
> +
> +	mt6315@6 {
> +		compatible = "mediatek,mt6315-regulator";
> +		reg = <0x6 SPMI_USID>;
> +
> +		regulators {
> +			mt6315_6_vbuck1: vbuck1 {
> +				regulator-compatible = "vbuck1";

We should check if we can avoid using regulator-compatible on mt6315 as well...
I honestly never checked if this is possible, but I think it is.

Eventually, if you can check and verify that it's not needed, we should even
remove that from all devicetrees, but that's not entirely relevant for this
patch anyway.

I'm just saying my plans out loud :-)

> +				regulator-name = "Vbcpu";
> +				regulator-min-microvolt = <300000>;
> +				regulator-max-microvolt = <1193750>;
> +				regulator-enable-ramp-delay = <256>;
> +				regulator-ramp-delay = <6250>;
> +				regulator-allowed-modes = <0 1 2>;
> +				regulator-always-on;
> +			};
> +		};
> +	};
> +
> +	mt6315@7 {
> +		compatible = "mediatek,mt6315-regulator";
> +		reg = <0x7 SPMI_USID>;
> +
> +		regulators {
> +			mt6315_7_vbuck1: vbuck1 {
> +				regulator-compatible = "vbuck1";
> +				regulator-name = "Vgpu";
> +				regulator-min-microvolt = <625000>;
> +				regulator-max-microvolt = <1193750>;
> +				regulator-enable-ramp-delay = <256>;
> +				regulator-ramp-delay = <6250>;
> +				regulator-allowed-modes = <0 1 2>;
> +				regulator-always-on;
> +			};
> +		};
> +	};
> +};
> +
> +&spi0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&spi0_pins>;
> +	mediatek,pad-select = <0>;
> +	status = "okay";
> +
> +	tpm: tpm@0 {
> +		compatible = "infineon,slb9670";
> +		reg = <0>;
> +		spi-max-frequency = <18500000>;
> +	};
> +};
> +
> +&spi1 {

Same as before, is this exposed somewhere? If it is, can we please get a comment in
here advertising so?

> +	pinctrl-names = "default";
> +	pinctrl-0 = <&spi1_pins>;
> +	mediatek,pad-select = <0>;
> +	status = "okay";
> +};
> +
> +&thermal_zones {
> +	board0-thermal {

"board0" is a bit generic, is that on purpose?

If this is a fixed placement sensor, you could name this something like

pcb-bottom-thermal pcb-left-thermal skin-thermal board-soc-thermal

etc etc etc :-)

> +		polling-delay = <1000>; /* milliseconds */
> +		polling-delay-passive = <0>; /* milliseconds */
> +		thermal-sensors = <&thermal_sensor0>;
> +
> +		trips {
> +			trip-alert {
> +				temperature = <85000>;
> +				hysteresis = <2000>;
> +				type = "passive";
> +			};
> +
> +			trip-crit {
> +				temperature = <95000>;
> +				hysteresis = <2000>;
> +				type = "critical";
> +			};
> +		};
> +	};
> +
> +	board1-thermal {
> +		polling-delay = <1000>; /* milliseconds */
> +		polling-delay-passive = <0>; /* milliseconds */
> +		thermal-sensors = <&thermal_sensor1>;
> +
> +		trips {
> +			trip-alert {
> +				temperature = <75000>;
> +				hysteresis = <2000>;
> +				type = "passive";
> +			};
> +
> +			trip-crit {
> +				temperature = <85000>;
> +				hysteresis = <2000>;
> +				type = "critical";
> +			};
> +		};
> +	};
> +
> +	board2-thermal {
> +		polling-delay = <1000>; /* milliseconds */
> +		polling-delay-passive = <0>; /* milliseconds */
> +		thermal-sensors = <&thermal_sensor2>;
> +
> +		trips {
> +			trip-alert {
> +				temperature = <75000>;
> +				hysteresis = <2000>;
> +				type = "passive";
> +			};
> +
> +			trip-crit {
> +				temperature = <85000>;
> +				hysteresis = <2000>;
> +				type = "critical";
> +			};
> +		};
> +	};
> +};
> +
> +&uart0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&uart0_pins>;
> +	status = "okay";
> +};
> +
> +&uart1 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&uart1_pins>;
> +	uart-has-rtscts;
> +	status = "okay";
> +};
> +
> +&uart2 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&uart2_pins>;
> +	uart-has-rtscts;
> +	status = "okay";
> +};
> +
> +&uart3 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&uart3_pins>;
> +	status = "okay";
> +};
> +
> +&uart4 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&uart4_pins>;
> +	status = "okay";
> +};
> +
> +/* USB3 */
> +&u3phy0 {
> +	status = "okay";
> +};
> +
> +/* PCIe1/USB2 */
> +&u3phy1 {
> +	status = "okay";
> +};
> +
> +/* USB2 */
> +&u3phy2 {
> +	status = "okay";
> +};
> +
> +/* USB2 */
> +&u3phy3 {
> +	status = "okay";
> +};
> +
> +/* USB3 front port */
> +&xhci0 {

It's not gonna work like this. I recently fixed the USB nodes in MT8195 by adding
MTU3 where necessary...

Check mt8195.dtsi - only one XHCI controller isn't placed behind MTU3, and that is
XHCI1 (11290000), while the others are MTU3.

As far as I can see from this DT, it should now instead look like..

&ssusb0 {
	dr_mode = "host";
	vusb33-supply = <&mt6359_vusb_ldo_reg>;
	status = "okay";
};

&ssusb2 {
	dr_mode = "host";
	vusb33-supply = <&mt6359_vusb_ldo_reg>;
	status = "okay";
};

&ssusb3 {
	dr_mode = "host";
	vusb33-supply = <&mt6359_vusb_ldo_reg>;
	status = "okay";
};

&xhci0 {
	vbus-supply = <&otg_vbus_regulator>;
	status = "okay";
};

&xhci1 {
	vusb33-supply = <&mt6359_vusb_ldo_reg>;

vbus is always supplied by something, as otherwise USB won't work - whether this
is an always-on regulator or a passthrough from external supply this doesn't really
matter - you should model a regulator-fixed that provides the 5V VBUS line.

For example:
	vbus_fixed: regulator-vbus {
		compatible = "regulator-fixed";
		regulator-name = "usb-vbus";
		regulator-always-on;
		regulator-boot-on;
		regulator-min-microvolt = <5000000>;
		regulator-max-microvolt = <5000000>;
	};

P.S.: If the rail has a different name, please use that different name. Obviously
that requires you to have schematics at hand, and I don't know if you do: if you
don't, then that regulator-vbus name is just fine.


	vbus-supply = <&vbus_fixed>;
	mediatek,u3p-dis-msk = <0x1>;
	status = "okay";
};

&xhci2 {
	vbus-supply = <&vbus_fixed>;
	status = "okay";
};

&xhci3 {
	vbus-supply = <&vbus_fixed>;
	status = "okay";
};

Cheers,
Angelo

> +	vusb33-supply = <&mt6359_vusb_ldo_reg>;
> +	vbus-supply = <&otg_vbus_regulator>;
> +	status = "okay";
> +};
> +
> +/* USB2 M.2 Key-B */
> +&xhci1 {
> +	vusb33-supply = <&mt6359_vusb_ldo_reg>;
> +	mediatek,u3p-dis-msk = <0x01>;
> +	status = "okay";
> +};
> +
> +/* USB2 M.2 Key-E */
> +&xhci2 {
> +	vusb33-supply = <&mt6359_vusb_ldo_reg>;
> +	status = "okay";
> +};
> +
> +/* USB2 to on-board usb hub */
> +&xhci3 {
> +	vusb33-supply = <&mt6359_vusb_ldo_reg>;
> +	status = "okay";
> +};
  

Hi,

thanks for the extensive review!

On Mon Feb 19, 2024 at 11:00 AM CET, AngeloGioacchino Del Regno wrote:

> > +&eth {
> > +	phy-mode ="rgmii-id";
> > +	phy-handle = <&ethernet_phy0>;
> > +	snps,reset-gpio = <&pio 93 GPIO_ACTIVE_HIGH>;
> > +	snps,reset-delays-us = <0 10000 80000>;
>
> snps,reset-delays-us and snps,reset-gpio are deprecated.
>
> > +	pinctrl-names = "default", "sleep";
> > +	pinctrl-0 = <&eth_default_pins>;
> > +	pinctrl-1 = <&eth_sleep_pins>;
> > +	status = "okay";
> > +
> > +	mdio {
> > +		ethernet_phy0: ethernet-phy@1 {
>
> compatible = "is there any applicable compatible?"
> P.S.: if you've got the usual rtl8211f, should be "ethernet-phy-id001c.c916"

I'd rather not have a compatible here. First, it's auto discoverable
and IIRC it's frowned upon adding any compatible if you ask the PHY
maintainers. And second, if we change the PHY (maybe due to a second
chip shortage or whatever), there is a chance you don't have to
update this in the DT.

> reg = <0x1>;
> interrupts-extended = <&pio 94 IRQ_TYPE_LEVEL_LOW>;
> reset-assert-us = <10000>;
> reset-deassert-us = <80000>;
> reset-gpios = <&pio 93 GPIO_ACTIVE_HIGH>;
>
>
> > +			reg = <0x1>;
> > +			interrupts-extended = <&pio 94 IRQ_TYPE_LEVEL_LOW>;
> > +		};
> > +	};
> > +};
> > +
> > +&gpu {
> > +	status = "okay";
> > +	mali-supply = <&mt6315_7_vbuck1>;
> > +};
> > +
> > +&i2c2 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&i2c2_pins>;
> > +	clock-frequency = <400000>;
> > +	status = "okay";
>
> Are i2c2,3,4 exposed as pins somewhere? If they are, can you please put a
> comment saying so?

This is only a basic device tree. On one i2c controller, there is
the LVDS bridge for example. My plan is to get the support for this
bridge upstream first and then adding the appropriate device nodes
here.

That being said, some are exposed to connectors. I'll add a comment
then.

> > +&mmc1 {
> > +	pinctrl-names = "default", "state_uhs";
> > +	pinctrl-0 = <&mmc1_default_pins>;
> > +	pinctrl-1 = <&mmc1_uhs_pins>;
> > +	cd-gpios = <&pio 129 GPIO_ACTIVE_LOW>;
> > +	bus-width = <4>;
> > +	max-frequency = <200000000>;
> > +	cap-sd-highspeed;
> > +	sd-uhs-sdr50;
> > +	sd-uhs-sdr104;
> > +	vmmc-supply = <&mt6360_ldo5>;
> > +	vqmmc-supply = <&mt6360_ldo3>;
>
> Does mmc1 support eMMC and SDIO?

No eMMC, but I'd guess it will support SDIO as in you can just plug
an SDIO card in the SD slot, right? Oh, it's a micro SD socket. So
uhm, I'm not sure if we should restrict it, though. Someone might
come up with a microsd to sd card adapter. I have one right in front
of me ;)

> If not, no-mmc; no-sdio;

So no-mmc;

> > +			drive-strength = <MTK_DRIVE_8mA>;
>
> s/MTK_DRIVE//g
> s/mA//g
>
> drive-strength = <8>;
>
> Please, here and everywhere else, for all values - let's stop using those
> MTK_DRIVE_(x)mA definitions, they're just defined as (x), where anyway
> the drive-strength property is in milliamps by default.
>
> We don't need these definitions.

Sure, the mt8195-demo was the blueprint for this. So maybe you should
get rid of it there to prevent any copying ;) (btw the same goes for
the regulator-compatible property).

Speaking of pinctrl, I find the R0R1 bias-pull-down values really
hard to grasp. The DT binding documentation didn't really help here.
What is R0 and R1, I presume some resistors which can be enabled.
Also are they in parallel or in series. I'd have assumed, the DT
binding should have hid this by giving the user a choice for the
resistance instead. Also I had a quick search in the RM and
couldn't find anything, I probably looked at the wrong place ;)

> > +	uart1_pins: uart1-pins {
> > +		pins_rx {
> > +			pinmux = <PINMUX_GPIO103__FUNC_URXD1>;
> > +			input-enable;
> > +			bias-pull-up;
> > +		};
> > +
> > +		pins_tx {
> > +			pinmux = <PINMUX_GPIO102__FUNC_UTXD1>;
> > +		};
> > +
> > +		pins_rts {
> > +			pinmux = <PINMUX_GPIO100__FUNC_URTS1>;
> > +			output-enable;
>
> Are you really sure that you need output-enable here?!
> RTS is not an output buffer....
>
> I don't think you do. Please double check.

Ahh, good catch, it's a leftover from mt8183-kukui.dts. There is
probably wrong, too.

> > +		};
> > +
> > +		pins_cts {
> > +			pinmux = <PINMUX_GPIO101__FUNC_UCTS1>;
> > +			input-enable;
> > +		};
> > +	};
> > +


> > +/* USB3 front port */
> > +&xhci0 {
>
> It's not gonna work like this. I recently fixed the USB nodes in MT8195 by adding
> MTU3 where necessary...

Uhm, seems like I've missed that.

> Check mt8195.dtsi - only one XHCI controller isn't placed behind MTU3, and that is
> XHCI1 (11290000), while the others are MTU3.
>
> As far as I can see from this DT, it should now instead look like..
>
> &ssusb0 {
> 	dr_mode = "host";
> 	vusb33-supply = <&mt6359_vusb_ldo_reg>;
> 	status = "okay";
> };
>
> &ssusb2 {
> 	dr_mode = "host";
> 	vusb33-supply = <&mt6359_vusb_ldo_reg>;
> 	status = "okay";
> };
>
> &ssusb3 {
> 	dr_mode = "host";
> 	vusb33-supply = <&mt6359_vusb_ldo_reg>;
> 	status = "okay";
> };
>
> &xhci0 {
> 	vbus-supply = <&otg_vbus_regulator>;
> 	status = "okay";
> };
>
> &xhci1 {
> 	vusb33-supply = <&mt6359_vusb_ldo_reg>;
>
> vbus is always supplied by something, as otherwise USB won't work - whether this
> is an always-on regulator or a passthrough from external supply this doesn't really
> matter - you should model a regulator-fixed that provides the 5V VBUS line.

I don't think this is correct, though. Think of an on-board USB
hub. There only D+/D- are connected (and maybe the USB3.2 SerDes
lanes). Or have a look at the M.2 pinout. There is no Vbus.

Also it seems I need the "mediatek,u3p-dis-msk = <0x01>;". At least
the last time I've tested it. I'll test it again, with and without.
The SerDes Line of the corresponding USB3.2 port is used for PCIe in
this case.

> For example:
> 	vbus_fixed: regulator-vbus {
> 		compatible = "regulator-fixed";
> 		regulator-name = "usb-vbus";
> 		regulator-always-on;
> 		regulator-boot-on;
> 		regulator-min-microvolt = <5000000>;
> 		regulator-max-microvolt = <5000000>;
> 	};

As mentioned above, I don't think this will make sense in my case.

> P.S.: If the rail has a different name, please use that different name. Obviously
> that requires you to have schematics at hand, and I don't know if you do: if you
> don't, then that regulator-vbus name is just fine.

I do have the schematics.

Thanks,
-michael
  

Il 19/02/24 14:59, Michael Walle ha scritto:
> On Mon Feb 19, 2024 at 2:35 PM CET, AngeloGioacchino Del Regno wrote:
>>>> vbus is always supplied by something, as otherwise USB won't work - whether this
>>>> is an always-on regulator or a passthrough from external supply this doesn't really
>>>> matter - you should model a regulator-fixed that provides the 5V VBUS line.
>>>
>>> I don't think this is correct, though. Think of an on-board USB
>>> hub. There only D+/D- are connected (and maybe the USB3.2 SerDes
>>> lanes). Or have a look at the M.2 pinout. There is no Vbus.
>>>
>>
>> Yes but the MediaTek MTU3 and/or controllers do have it ;-)
> 
> .. and ..
> 
>>>> For example:
>>>> 	vbus_fixed: regulator-vbus {
>>>> 		compatible = "regulator-fixed";
>>>> 		regulator-name = "usb-vbus";
>>>> 		regulator-always-on;
>>>> 		regulator-boot-on;
>>>> 		regulator-min-microvolt = <5000000>;
>>>> 		regulator-max-microvolt = <5000000>;
>>>> 	};
>>>
>>> As mentioned above, I don't think this will make sense in my case.
>>>   >> P.S.: If the rail has a different name, please use that different name. Obviously
>>>> that requires you to have schematics at hand, and I don't know if you do: if you
>>>> don't, then that regulator-vbus name is just fine.
>>>
>>> I do have the schematics.
>>
>> In that case, you should model the power tree with the fixed power lines,
>> check mt8195-cherry (and/or cherry-tomato) and radxa-nio-12l; even though
>> those are technically "doing nothing", this is device tree, so it should
>> provide a description of the hardware ... and the board does have fixed
>> power lines.
>> It has at least one: DC-IN (typec, barrel jack or whatever, the board needs
>> power, doesn't it?!).
> 
> Mh, maybe I don't get it. But within the hardware there is simply no
> Vbus. Thus I'd argue it doesn't make sense to have a vbus-supply
> property. Besides, the mediatek,mtu3.yaml binding lists it as
> deprecated anyway and it should rather be on the connector. There,
> it makes perfectly sense (at least if it's a USB connector).
> 
> Thus in my case, the xhci for the front port has a vbus-supply
> property (but it should rather have a connector node, as I've just
> learned). But the internal port which connects to the USB hub
> shouldn't have one.
> 
> 
>    +-----+           +-----+                  +------+
>    |     |<--Dp/Dn-->|     |<------Dp/Dn----->| USB  |
>    | SoC |           |     |                  | Conn |
>    |     |           |     |   +-----+        |      |
>    +-----+           | USB |   | PWR |--Vbus->|      |
>                      | Hub |   | SW  |        +------+
>                      |     |   +-----+
>                      |     |     ^
>                      |     |     | PRTPWR
>                      |     |-----'
>                      +-----+
> 
> "PWR SW" is a power switch, the input (+5V) isn't shown here. The
> power will be enabled by the USB Hub.
> 

Ok then, put the vbus on the connector, where applicable.

If PWR_SW is a physical switch (not detectable), then you can still model it as an
always-on regulator-fixed, as there's no other clean way of doing that.

As for modeling the power tree with the fixed power lines, I mean that you should
add the power rails as described in your schematics.
You will surely have one DC-IN supply, and others. That should be modeled as a
fixed regulator, which outputs to (probably) the PMIC, using it as an input for
its buck regulators.

Again, your PMIC must be getting a voltage from somewhere in order to regulate
that to a different voltage for a different power rail, right? :-)

Regards,
Angelo