[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e5030f7d-707b-19ac-2e5a-26b0fb08afd9@linaro.org>
Date: Mon, 4 Sep 2023 11:37:06 +0200
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Macpaul Lin <macpaul.lin@...iatek.com>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Matthias Brugger <matthias.bgg@...il.com>,
AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com>,
Frank Wunderlich <frank-w@...lic-files.de>,
Bernhard Rosenkränzer <bero@...libre.com>,
Sean Wang <sean.wang@...iatek.com>, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org
Cc: Bear Wang <bear.wang@...iatek.com>,
Pablo Sun <pablo.sun@...iatek.com>,
Macpaul Lin <macpaul@...il.com>,
Chunfeng Yun <chunfeng.yun@...iatek.com>,
Ben Lok <ben.lok@...iatek.com>
Subject: Re: [PATCH 2/2] arm64: dts: mediatek: add device-tree for Genio 1200
EVK board
On 04/09/2023 11:20, Macpaul Lin wrote:
> Add basic device-tree for the Genio 1200 EVK board. The
> Demo board is made by MediaTek and has a MT8395 SoC (MT8195 family),
> associated with the MT6359 and MT6360 PMICs, and
> the MT7921 connectivity chip.
>
> The IOs available on that board are:
> * 1 USB Type-C connector with DP aux mode support
> * 2 USB Type-A connector with a USB hub
> * 1 micro-USB port for gadget or OTG support
> * 1 full size HDMI RX and 1 full size HDMI TX connector
> * 1 micro SD slot
> * 40 pins header
> * SPI interface header
> * 1 M.2 slot
> * 1 audio jack
> * 1 micro-USB port for serial debug
> * 2 connectors for DSI displays, 1 of the DSI panel is installed
> * 3 connectors for CSI cameras
> * 1 connector for a eDP panel
> * 1 MMC storage
> * 1 Touch Panel (installed DSI display)
> * 1 M.2 slot for 5G dongle
>
> This commit adds basic support in order to be able to boot.
>
> Signed-off-by: Ben Lok <ben.lok@...iatek.com>
> Signed-off-by: Macpaul Lin <macpaul.lin@...iatek.com>
> ---
> arch/arm64/boot/dts/mediatek/Makefile | 1 +
> .../boot/dts/mediatek/genio-1200-evk.dts | 931 ++++++++++++++++++
> 2 files changed, 932 insertions(+)
> create mode 100644 arch/arm64/boot/dts/mediatek/genio-1200-evk.dts
>
> Notes for v1:
> - This dts patch has been checked with 'make dtbs_check W=1', however, some
> warnings are caused by mt8195.dtsi, should be fixed by separate patches.
>
> diff --git a/arch/arm64/boot/dts/mediatek/Makefile b/arch/arm64/boot/dts/mediatek/Makefile
> index c99c3372a4b5..5bf29581f08b 100644
> --- a/arch/arm64/boot/dts/mediatek/Makefile
> +++ b/arch/arm64/boot/dts/mediatek/Makefile
> @@ -1,4 +1,5 @@
> # SPDX-License-Identifier: GPL-2.0
> +dtb-$(CONFIG_ARCH_MEDIATEK) += genio-1200-evk.dtb
> dtb-$(CONFIG_ARCH_MEDIATEK) += mt2712-evb.dtb
> dtb-$(CONFIG_ARCH_MEDIATEK) += mt6755-evb.dtb
> dtb-$(CONFIG_ARCH_MEDIATEK) += mt6779-evb.dtb
> diff --git a/arch/arm64/boot/dts/mediatek/genio-1200-evk.dts b/arch/arm64/boot/dts/mediatek/genio-1200-evk.dts
> new file mode 100644
> index 000000000000..696ad535ac8f
> --- /dev/null
> +++ b/arch/arm64/boot/dts/mediatek/genio-1200-evk.dts
> @@ -0,0 +1,931 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/*
> + * Copyright (C) 2023 MediaTek Inc.
> + * Author: Ben Lok <ben.lok@...iatek.com>
> + * Macpaul Lin <macpaul.lin@...iatek.com>
> + */
> +/dts-v1/;
> +
> +#include "mt8195.dtsi"
> +#include "mt6359.dtsi"
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/input/input.h>
> +#include <dt-bindings/interrupt-controller/irq.h>
> +#include <dt-bindings/pinctrl/mt8195-pinfunc.h>
> +#include <dt-bindings/regulator/mediatek,mt6360-regulator.h>
> +#include <dt-bindings/spmi/spmi.h>
> +#include <dt-bindings/usb/pd.h>
> +
> +/ {
> + model = "MediaTek Genio 1200 EVK-P1V2-EMMC";
> + compatible = "mediatek,mt8395-evk", "mediatek,mt8195";
In the binding patch you said mt8395 is a SoC, so you miss here its
compatible.
> +
> + aliases {
> + serial0 = &uart0;
> + ethernet0 = ð
> + };
> +
> + chosen {
> + stdout-path = "serial0:921600n8";
> + };
> +
> + firmware {
> + optee {
> + compatible = "linaro,optee-tz";
> + method = "smc";
> + };
> + };
> +
> + memory@...00000 {
> + device_type = "memory";
> + reg = <0 0x40000000 0x2 0x00000000>;
> + };
> +
> + reserved-memory {
> + #address-cells = <2>;
> + #size-cells = <2>;
> + ranges;
> +
> + /* 12 MiB reserved for OP-TEE (BL32)
Use Linux coding style comments.
> + can_clk: can-clk {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <20000000>;
> + clock-output-names = "can-clk";
> + };
> +
> + wifi_pwr_fixed_3v3: wifi-pwr-fixed-3v3 {
And this is not a regulator? All other nodes are called regulator-X
> + compatible = "regulator-fixed";
> + regulator-name = "wifi_pwr_fixed_3v3";
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + gpio = <&pio 135 GPIO_ACTIVE_HIGH>;
> + enable-active-high;
> + regulator-always-on;
> + };
> +};
....
> +
> +&i2c6 {
> + clock-frequency = <400000>;
> + pinctrl-0 = <&i2c6_pins>;
> + pinctrl-names = "default";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + status = "okay";
> +
> + mt6360: mt6360@34 {
Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> + compatible = "mediatek,mt6360";
> + reg = <0x34>;
> + interrupts = <128 IRQ_TYPE_EDGE_FALLING>;
> + interrupt-names = "IRQB";
> + interrupt-controller;
> + #interrupt-cells = <1>;
> + pinctrl-0 = <&mt6360_pins>;
> +
> + charger {
> + compatible = "mediatek,mt6360-chg";
> + richtek,vinovp-microvolt = <14500000>;
> +
> + otg_vbus_regulator: usb-otg-vbus-regulator {
> + 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-name = "emi_vdd2";
> + 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-name = "emi_vddq";
> + 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-name = "tp1_p3v0";
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-allowed-modes = <MT6360_OPMODE_NORMAL
> + MT6360_OPMODE_LP>;
> + regulator-always-on;
> + };
> +
> + mt6360_ldo2: ldo2 {
> + regulator-name = "panel1_p1v8";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + regulator-allowed-modes = <MT6360_OPMODE_NORMAL
> + MT6360_OPMODE_LP>;
> + };
> +
> + mt6360_ldo3: ldo3 {
> + regulator-name = "vmc_pmu";
> + regulator-min-microvolt = <1200000>;
> + regulator-max-microvolt = <3600000>;
> + regulator-allowed-modes = <MT6360_OPMODE_NORMAL
> + MT6360_OPMODE_LP>;
> + };
> +
> + mt6360_ldo5: ldo5 {
> + regulator-name = "vmch_pmu";
> + regulator-min-microvolt = <2700000>;
> + regulator-max-microvolt = <3600000>;
> + regulator-allowed-modes = <MT6360_OPMODE_NORMAL
> + MT6360_OPMODE_LP>;
> + };
> +
> + /* This is a measure point, which name is mt6360_ldo1 on schematic */
> + mt6360_ldo6: ldo6 {
> + regulator-name = "mt6360_ldo1";
> + regulator-min-microvolt = <500000>;
> + regulator-max-microvolt = <2100000>;
> + regulator-allowed-modes = <MT6360_OPMODE_NORMAL
> + MT6360_OPMODE_LP>;
> + };
> +
> + mt6360_ldo7: ldo7 {
> + regulator-name = "emi_vmddr_en";
> + regulator-min-microvolt = <500000>;
> + regulator-max-microvolt = <2100000>;
> + regulator-allowed-modes = <MT6360_OPMODE_NORMAL
> + MT6360_OPMODE_LP>;
> + regulator-always-on;
> + };
> + };
> +
Stray blank line
> + };
> +};
> +
> +&spi1 {
> + pinctrl-0 = <&spi1_pins>;
> + pinctrl-names = "default";
> + mediatek,pad-select = <0>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + status = "okay";
> + cs-gpios = <&pio 64 GPIO_ACTIVE_LOW>;
> +
> + can0: can@0 {
> + compatible = "microchip,mcp2518fd";
> + reg = <0>;
> + clocks = <&can_clk>;
> + spi-max-frequency = <20000000>;
> + interrupts-extended = <&pio 16 IRQ_TYPE_LEVEL_LOW>;
> + vdd-supply = <&mt6359_vcn33_2_bt_ldo_reg>;
> + xceiver-supply = <&mt6359_vcn33_2_bt_ldo_reg>;
> + };
> +};
> +
> +&spi2 {
> + pinctrl-0 = <&spi2_pins>;
> + pinctrl-names = "default";
> + mediatek,pad-select = <0>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + status = "okay";
> +};
> +
> +&xhci0 {
> + status = "okay";
> +};
> +
> +&xhci1 {
> + vusb33-supply = <&mt6359_vusb_ldo_reg>;
> + status = "okay";
> +};
> +
> +&xhci2 {
> + vusb33-supply = <&mt6359_vusb_ldo_reg>;
> + status = "okay";
> +};
> +
> +&xhci3 {
> + vusb33-supply = <&mt6359_vusb_ldo_reg>;
> + status = "okay";
> +};
> +
> +&u3port0 {
> + status = "okay";
> +};
> +
You have quite surprising ordering of these overrides...
...
> +
> +&spmi {
> + #address-cells = <2>;
> + #size-cells = <0>;
> +
> + mt6315_6: mt6315@6 {
Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> + compatible = "mediatek,mt6315-regulator";
> + reg = <0x6 SPMI_USID>;
> +
> + regulators {
> + mt6315_6_vbuck1: vbuck1 {
> + regulator-compatible = "vbuck1";
> + regulator-name = "Vbcpu";
> + regulator-min-microvolt = <300000>;
> + regulator-max-microvolt = <1193750>;
> + regulator-enable-ramp-delay = <256>;
> + regulator-allowed-modes = <0 1 2>;
> + regulator-always-on;
> + };
> + };
> + };
> +
> + mt6315_7: mt6315@7 {
Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> + compatible = "mediatek,mt6315-regulator";
> + reg = <0x7 SPMI_USID>;
> +
> + regulators {
Best regards,
Krzysztof
Powered by blists - more mailing lists