From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 5CEF3C46CD2 for ; Wed, 24 Jan 2024 08:36:55 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:In-Reply-To:From:References:Cc:To:Subject:MIME-Version:Date: Message-ID:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=1fF4TNKMYmIvA/Lkm6VrLqFPAo+V/BWnfcJrlRALTTU=; b=PQ0NtTvsneA7sAf4KJJuX/97LH k7+oQyod289ENIu/47INuARubWCE3BibPkCfNNq3edHYshnAZCtkkKKbZtgbY0b6OGtgce3xNlPvs cNiqGPIRhqeiE+watW38MYUoZv/pTslWKh2E81els63CbilSjJjXQLXzefF/kgQbKI2DKpKkMb3rf MJ9VeMtu9AR4AL1+sSCub4XYYGXSGhC0dvLRSgLSLNiAPiOQC3NvXyPJgCihFFoBUgBYNjFTxxjgK R/SwerwMot7A3FOd9NMzK/tBw9uAeTZVlxV2UejyqWbsRMfw2b40ziPXDj5Z5gmulqaE/1RNfaszZ xFiRfY1g==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1rSYkg-0021jj-0W; Wed, 24 Jan 2024 08:36:54 +0000 Received: from madrid.collaboradmins.com ([46.235.227.194]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1rSYkb-0021iy-0H; Wed, 24 Jan 2024 08:36:51 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1706085407; bh=fTubqUrcvjY2fWw2V+v/BpDL1SgAY/HreczUJAoul0k=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=JFqYgUimPr7iWwzyF4uGka27Uj0NQtofHt/v5iZJ81Vx+ymGTbfkoIZkL5SCcLnBn dEQTNBBbR2Jk2pbpi/IjEnkptKIFcBgFAC35SvYUKDcFEgGX9WIEyUN18WEQ9sba7T NSWRA7j+vYbmXBcAzAHD2/5ONfDmmb/qdEVGd314tIQ/lt3nRMSRIdORIMH16LTpAL 94S3fb/mNHEcmYgr3/bUgaii4sTZaCO7kDc3XkZDfBAujQ3ad2AdBuCCpYIOSU6avW e3yL+hhWdkhzQbtwAdQouRE8yzdluAyqpEy9QUqia23hDhqkh5K8McPImR87uN3eUK GMGek0zYpuWyA== Received: from [100.113.186.2] (cola.collaboradmins.com [195.201.22.229]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: kholk11) by madrid.collaboradmins.com (Postfix) with ESMTPSA id 836B73782074; Wed, 24 Jan 2024 08:36:46 +0000 (UTC) Message-ID: Date: Wed, 24 Jan 2024 09:36:45 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 3/3] arm64: dts: mediatek: Add device tree for MT8365-based Pumpkin i350 Content-Language: en-US To: Laurent Pinchart Cc: linux-mediatek@lists.infradead.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Rob Herring , Krzysztof Kozlowski , Conor Dooley , Matthias Brugger , Paul Elder , Fabien Parent , Alexandre Mergnat , Julien Stephan , Suhrid Subramaniam , Ted Larson References: <20231025210342.30995-1-laurent.pinchart@ideasonboard.com> <20231025210342.30995-4-laurent.pinchart@ideasonboard.com> <20240123150236.GJ10679@pendragon.ideasonboard.com> From: AngeloGioacchino Del Regno In-Reply-To: <20240123150236.GJ10679@pendragon.ideasonboard.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240124_003649_385831_C4F17E32 X-CRM114-Status: GOOD ( 31.54 ) X-BeenThere: linux-mediatek@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "Linux-mediatek" Errors-To: linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org Il 23/01/24 16:02, Laurent Pinchart ha scritto: > Hi Angelo, > > On Tue, Jan 23, 2024 at 02:53:58PM +0100, AngeloGioacchino Del Regno wrote: >> Il 25/10/23 23:03, Laurent Pinchart ha scritto: >>> Add a minimal device tree for the Genio i350 Pumpkin development board, >>> which is based on a MediaTek MT8365. >>> >>> The device tree is based on an initial version by Fabien Parent Based >>> written against the MediaTek BSP kernel ([1]). It has been cleaned up, >>> some features have been added (GPIO LEDs, ethernet), and some features >>> removed (audio, camera, display and dual-role USB). Those features will >>> be added back once the corresponding DT bindings and/or drivers become >>> available in the upstream kernel. >>> >>> [1] https://gitlab.com/mediatek/aiot/bsp/linux/-/blob/mtk-v5.15-dev/arch/arm64/boot/dts/mediatek/mt8365-pumpkin.dts >>> >>> Co-developed-by: Fabien Parent >>> Signed-off-by: Fabien Parent >>> Co-developed-by: Paul Elder >>> Signed-off-by: Paul Elder >>> Signed-off-by: Laurent Pinchart >>> --- >>> Changes since v1: >>> >>> - Drop mmc2 alias >>> - Reorder properties of i2c, mmc and usb nodes >>> >>> Changes compared to the BSP version: >>> >>> - Add ethernet controller >>> - Add GPIO LEDs >>> - Add reserved memory region for BL31 >>> - Update board model and compatible >>> - Rename enable-sdio-wakeup to wakeup-source >>> - Drop audio support >>> - Drop display support >>> - Drop dual role USB support >>> - Don't use underscores in node names >>> - Normalize pinmux node names >>> - Remove unneeded labels >>> - Drop unneeded always-on >>> - Drop unused pinmux nodes >>> - Drop camera GPIO hog >>> - Update copyright >>> - Fix formatting >>> - Sort alphabetically >>> --- >>> arch/arm64/boot/dts/mediatek/Makefile | 1 + >>> .../boot/dts/mediatek/mt8365-pumpkin.dts | 541 ++++++++++++++++++ >>> 2 files changed, 542 insertions(+) >>> create mode 100644 arch/arm64/boot/dts/mediatek/mt8365-pumpkin.dts >>> >>> diff --git a/arch/arm64/boot/dts/mediatek/Makefile b/arch/arm64/boot/dts/mediatek/Makefile >>> index c99c3372a4b5..bbc232bdadc4 100644 >>> --- a/arch/arm64/boot/dts/mediatek/Makefile >>> +++ b/arch/arm64/boot/dts/mediatek/Makefile >>> @@ -53,4 +53,5 @@ dtb-$(CONFIG_ARCH_MEDIATEK) += mt8195-cherry-tomato-r3.dtb >>> 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) += mt8365-pumpkin.dtb >>> dtb-$(CONFIG_ARCH_MEDIATEK) += mt8516-pumpkin.dtb >>> diff --git a/arch/arm64/boot/dts/mediatek/mt8365-pumpkin.dts b/arch/arm64/boot/dts/mediatek/mt8365-pumpkin.dts >>> new file mode 100644 >>> index 000000000000..9c75294c9889 >>> --- /dev/null >>> +++ b/arch/arm64/boot/dts/mediatek/mt8365-pumpkin.dts >>> @@ -0,0 +1,541 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +/* >>> + * Copyright (c) 2021 BayLibre, SAS. >>> + * Copyright (c) 2023 Ideas on Board Oy >>> + * >>> + * Author: Fabien Parent >>> + */ >>> + >>> +/dts-v1/; >>> + >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +#include "mt8365.dtsi" >>> +#include "mt6357.dtsi" >>> + >>> +/ { >>> + model = "OLogic Pumpkin i350 EVK"; >>> + compatible = "ologic,pumpkin-i350", "mediatek,mt8365"; >>> + >>> + aliases { >>> + ethernet0 = ðernet_usb; >>> + mmc0 = &mmc0; >>> + mmc1 = &mmc1; >>> + serial0 = &uart0; >>> + }; >>> + >>> + chosen { >>> + stdout-path = "serial0:921600n8"; >>> + }; >>> + >>> + firmware { >>> + optee { >>> + compatible = "linaro,optee-tz"; >>> + method = "smc"; >>> + }; >>> + }; >>> + >>> + leds { >>> + compatible = "gpio-leds"; >>> + >>> + led-red { >>> + gpios = <&gpio_exp 12 GPIO_ACTIVE_HIGH>; >>> + color = ; >>> + function = LED_FUNCTION_HEARTBEAT; >>> + linux,default-trigger = "heartbeat"; >>> + }; >>> + >>> + led-green { >>> + gpios = <&gpio_exp 13 GPIO_ACTIVE_HIGH>; >>> + color = ; >>> + default-state = "off"; >>> + }; >>> + }; >>> + >>> + memory@40000000 { >>> + device_type = "memory"; >>> + reg = <0 0x40000000 0 0x80000000>; >>> + }; >>> + >>> + reserved-memory { >>> + #address-cells = <2>; >>> + #size-cells = <2>; >>> + ranges; >>> + >>> + /* 192 KiB reserved for ARM Trusted Firmware (BL31) */ >>> + secmon@43000000 { >>> + no-map; >>> + reg = <0 0x43000000 0 0x30000>; >> >> reg first, no-map last. >> >>> + }; >>> + >>> + /* >>> + * 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@43200000 { >>> + no-map; >>> + reg = <0 0x43200000 0 0x00c00000>; >> >> same here. >> >>> + }; >>> + }; >>> + >>> + usb_otg_vbus: usb-otg-vbus-regulator { >>> + compatible = "regulator-fixed"; >>> + regulator-name = "otg_vbus"; >>> + regulator-min-microvolt = <5000000>; >>> + regulator-max-microvolt = <5000000>; >>> + gpio = <&pio 25 GPIO_ACTIVE_HIGH>; >>> + enable-active-high; >>> + }; >>> +}; >>> + >>> +&cpu0 { >>> + proc-supply = <&mt6357_vproc_reg>; >>> + sram-supply = <&mt6357_vsram_proc_reg>; >>> +}; >>> + >>> +&cpu1 { >>> + proc-supply = <&mt6357_vproc_reg>; >>> + sram-supply = <&mt6357_vsram_proc_reg>; >>> +}; >>> + >>> +&cpu2 { >>> + proc-supply = <&mt6357_vproc_reg>; >>> + sram-supply = <&mt6357_vsram_proc_reg>; >>> +}; >>> + >>> +&cpu3 { >>> + proc-supply = <&mt6357_vproc_reg>; >>> + sram-supply = <&mt6357_vsram_proc_reg>; >>> +}; >>> + >>> +&i2c0 { >>> + clock-frequency = <100000>; >>> + >>> + pinctrl-names = "default"; >>> + pinctrl-0 = <&i2c0_pins>; >>> + >>> + status = "okay"; >>> +}; >>> + >>> +&i2c1 { >>> + clock-frequency = <100000>; >>> + >>> + pinctrl-names = "default"; >>> + pinctrl-0 = <&i2c1_pins>; >>> + >>> + status = "okay"; >>> + >>> + gpio_exp: gpio@20 { >>> + compatible = "ti,tca6416"; >>> + reg = <0x20>; >>> + reset-gpios = <&pio 78 GPIO_ACTIVE_LOW>; >>> + pinctrl-names = "default"; >>> + pinctrl-0 = <&tca6416_pins>; >>> + >>> + gpio-controller; >> >> Please check Documentation/devicetree/bindings/dts-coding-style.rst > > I assume you mean the "Order of Properties in Device Node" section ? > What part exactly ? > >>> + #gpio-cells = <2>; >>> + }; >>> +}; >>> + >>> +&i2c2 { >>> + clock-frequency = <100000>; >>> + >>> + pinctrl-names = "default"; >>> + pinctrl-0 = <&i2c2_pins>; >>> + >>> + status = "okay"; >>> +}; >>> + >>> +&i2c3 { >>> + clock-frequency = <100000>; >>> + >>> + pinctrl-names = "default"; >>> + pinctrl-0 = <&i2c3_pins>; >>> + >>> + status = "okay"; >>> +}; >>> + >>> +&keypad { >>> + status = "okay"; >> >> status goes last. >> >> Please check Documentation/devicetree/bindings/dts-coding-style.rst >> >>> + >>> + pinctrl-names = "default"; >>> + pinctrl-0 = <&keypad_pins>; >>> + >>> + keypad,num-rows = <2>; >>> + keypad,num-columns = <1>; >>> + mediatek,debounce-us = <32000>; >>> + mediatek,double-keys; >>> + >>> + linux,keymap = >> + MATRIX_KEY(0x01, 0x00, KEY_VOLUMEUP)>; >>> +}; >>> + >>> +/* eMMC */ >>> +&mmc0 { >>> + bus-width = <8>; >>> + max-frequency = <200000000>; >>> + >>> + cap-mmc-highspeed; >>> + cap-mmc-hw-reset; >>> + hs400-ds-delay = <0x12012>; >>> + mmc-hs200-1_8v; >>> + mmc-hs400-1_8v; >>> + no-sd; >>> + no-sdio; >>> + non-removable; >> >> Please order by name. >> >>> + >>> + vmmc-supply = <&mt6357_vemc_reg>; >>> + vqmmc-supply = <&mt6357_vio18_reg>; >>> + >>> + pinctrl-names = "default", "state_uhs"; >>> + pinctrl-0 = <&mmc0_pins_default>; >>> + pinctrl-1 = <&mmc0_pins_uhs>; >>> + >>> + assigned-clocks = <&topckgen CLK_TOP_MSDC50_0_SEL>; >> >> assigned clocks and clock parents go first. >> >>> + assigned-clock-parents = <&topckgen CLK_TOP_MSDCPLL>; >>> + >>> + status = "okay"; >>> +}; >>> + >>> +/* SD card connector */ >>> +&mmc1 { >>> + bus-width = <4>; >>> + max-frequency = <200000000>; >>> + >>> + cap-sd-highspeed; >>> + cd-gpios = <&pio 76 GPIO_ACTIVE_LOW>; >>> + sd-uhs-sdr104; >>> + sd-uhs-sdr50; >> >> Please order by name. >> >>> + >>> + vmmc-supply = <&mt6357_vmch_reg>; >>> + vqmmc-supply = <&mt6357_vmc_reg>; >>> + >>> + pinctrl-names = "default", "state_uhs"; >>> + pinctrl-0 = <&mmc1_pins_default>; >>> + pinctrl-1 = <&mmc1_pins_uhs>; >>> + >>> + status = "okay"; >>> +}; >>> + >>> +/* WiFi module */ >>> +&mmc2 { >>> + bus-width = <4>; >>> + max-frequency = <200000000>; >>> + >>> + cap-sd-highspeed; >>> + cap-sdio-irq; >>> + wakeup-source; >>> + hs400-ds-delay = <0x12012>; >> >> Please order by name. >> >>> + keep-power-in-suspend; >>> + non-removable; >>> + sd-uhs-sdr104; >>> + sd-uhs-sdr25; >>> + sd-uhs-sdr50; >>> + >>> + vmmc-supply = <&mt6357_vemc_reg>; >>> + vqmmc-supply = <&mt6357_vio18_reg>; >>> + >>> + pinctrl-names = "default", "state_uhs"; >>> + pinctrl-0 = <&mmc2_pins_default>; >>> + pinctrl-1 = <&mmc2_pins_uhs>; >>> + >>> + assigned-clocks = <&topckgen CLK_TOP_MSDC50_2_SEL>; >>> + assigned-clock-parents = <&topckgen CLK_TOP_MSDCPLL>; >>> + >>> + status = "okay"; >>> +}; >>> + >>> +&mt6357_pmic { >>> + interrupt-parent = <&pio>; >>> + interrupts = <145 IRQ_TYPE_LEVEL_HIGH>; >>> + interrupt-controller; >>> + #interrupt-cells = <2>; >>> +}; >>> + >>> +&pio { >>> + i2c0_pins: i2c0-pins { >>> + pins { >>> + pinmux = , >>> + ; >>> + mediatek,pull-up-adv = <3>; >>> + mediatek,drive-strength-adv = <00>; >> >> I don't know what 00 is here, and it's not a valid drive strength value in uA. > > I don't know either, I didn't author that part. I'll look it up. > >> Valid values are 125, 250, 500 and 1000 microamps. >> >> P.S.: Here and everywhere else. >> >>> + bias-pull-up; >>> + }; >>> + }; >>> + >>> + >> >> ..snip.. >> >>> + mmc0_pins_uhs: mmc0-uhs-pins{ >>> + clk-pins { >>> + pinmux = ; >>> + drive-strength = ; >>> + bias-pull-down = ; >>> + }; >>> + >>> + cmd-dat-pins { >>> + pinmux = , >>> + , >>> + , >>> + , >>> + , >>> + , >>> + , >>> + , >>> + ; >>> + input-enable; >>> + drive-strength = ; >> >> There's no need to use MTK_DRIVE_xxxx definitions, as they're really just defining >> the same number that you can just clearly write. >> >> In this case, it is >> drive-strength = <10>; >> >> please change it here and everywhere else. > > OK. Looks cleaner indeed. > >>> + bias-pull-up = ; >>> + }; >>> + >>> + ds-pins { >>> + pinmux = ; >>> + drive-strength = ; >>> + bias-pull-down = ; >>> + }; >>> + >>> + rst-pins { >>> + pinmux = ; >>> + drive-strength = ; >>> + bias-pull-up; >>> + }; >>> + }; >>> + >> >> ..snip.. >> >>> + >>> +&usb_host { >>> + #address-cells = <1>; >>> + #size-cells = <0>; >>> + >>> + vusb33-supply = <&mt6357_vusb33_reg>; >>> + >>> + status = "okay"; >>> + >>> + hub@2 { >>> + reg = <2>; >>> + >>> + #address-cells = <1>; >>> + #size-cells = <0>; >>> + >>> + ethernet_usb: ethernet@1 { >> >> Can you please explain why you need this node on a fully discoverable bus? >> Isn't it enough to just let USB auto enumerate this device at boot? > > To let the boot loader fill in the MAC address in DT. Would you like me > to add a comment here ? > This is pretty unusual... so yes please, add a comment explaining why this node is here. I can otherwise already see a future engineer removing this node thinking that it's useless! :-) Also, if this is an internal "non-removable" USB ethernet adapter, please add that note to the comment. Cheers, Angelo >>> + compatible = "usb424,ec00"; >>> + reg = <1>; >>> + }; >>> + }; >>> +}; >