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 X-Spam-Level: X-Spam-Status: No, score=-15.3 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_2 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id B4955C433E0 for ; Wed, 3 Mar 2021 18:18:46 +0000 (UTC) Received: from desiato.infradead.org (desiato.infradead.org [90.155.92.199]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id B97F164EEC for ; Wed, 3 Mar 2021 18:18:45 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B97F164EEC Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=desiato.20200630; h=Sender:Content-Transfer-Encoding :Content-Type:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To:Message-ID: Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=CcQVIQ4YTZ6yj7e+ppl9vQHCjJqvPxnM5RfcipGmBig=; b=LX0lb0PLFjysvPH3Yu/oGNAQs 2jIC1pwgNAcBjy0z2cHv85Jtb2XSPLlhsEpV/XSiHNQewsmWhcfG1JErpJHiqF58k88QD23a3u5iC VsLhxrMXEBeg8SpMxgC8c6e7FeVmNlXgepVcb1TbSRuEmTMSUGUVu32F0W9hKSK9e6qmbmriB4TZ6 0xeFo5IghZ2QK7vlGQAyCHPffwPnTO5ANizXGiDUg/p+FgxqK15Ar/c6w0QOEJix7wUlumjzCiWcs tNvy3mr2rRsyrLZPElJrdq7rgkQ+k50pOrR6Mxvs8HRoPFvkngyHI1Eh+GpOXZcKe2zxI0iQHaCK1 kALLutBiw==; Received: from localhost ([::1] helo=desiato.infradead.org) by desiato.infradead.org with esmtp (Exim 4.94 #2 (Red Hat Linux)) id 1lHW2q-005ylK-Je; Wed, 03 Mar 2021 18:16:25 +0000 Received: from casper.infradead.org ([2001:8b0:10b:1236::1]) by desiato.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1lHSzH-005Gfl-K3 for linux-arm-kernel@desiato.infradead.org; Wed, 03 Mar 2021 15:00:31 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=Content-Transfer-Encoding:Content-Type: MIME-Version:References:In-Reply-To:Message-ID:Subject:Cc:To:From:Date:Sender :Reply-To:Content-ID:Content-Description; bh=AQNvKdTh8qfsvp1ajChe/m19P93WmrFnlbfYrcG7IT0=; b=k4sCoV4Ebe7iFH1aJgPBxTLfes 9H/YTA6aqFGQ0J5Xww8Dq0kxaMU+al/J7/uEzo7ohbt14rSK0oyNa6gxRk3BsqzfLTibImFpgplUd SOx2s3WPpvcABubbR9hTL4AjG/CAHYLdFudjoleKpPzknlntBZaMKmSCqkGIgLdq2TkUWpc0RNXMz 6kmEY2GKSZqjqVZFhLJ2kaDM741qzrND2Gz6rO39T7PmXNc6anU5yrhWHzphX+fpkvh33aYjoo1p0 +49QtbzXr+Dmm3iZfSnBnWTU6N4qm2pOAobs9zBI925LEi4ytOAkLjm9L/mLnMUKxXl7Uv5MNRU5+ MIhBx3JQ==; Received: from foss.arm.com ([217.140.110.172]) by casper.infradead.org with esmtp (Exim 4.94 #2 (Red Hat Linux)) id 1lHRFG-002g3v-1w for linux-arm-kernel@lists.infradead.org; Wed, 03 Mar 2021 13:08:54 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 4CD9131B; Wed, 3 Mar 2021 05:08:40 -0800 (PST) Received: from slackpad.fritz.box (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id F025C3F766; Wed, 3 Mar 2021 05:08:38 -0800 (PST) Date: Wed, 3 Mar 2021 13:08:34 +0000 From: Andre Przywara To: Ivan Uvarov Cc: Rob Herring , Maxime Ripard , Chen-Yu Tsai , Jernej Skrabec , linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org, Icenowy Zheng Subject: Re: [draft2 PATCH] ARM: dts: sun8i: r40: add devicetree for FETA40i-C/OKA40i-C Message-ID: <20210303130834.401cc50c@slackpad.fritz.box> In-Reply-To: References: Organization: Arm Ltd. X-Mailer: Claws Mail 3.17.1 (GTK+ 2.24.31; x86_64-slackware-linux-gnu) MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210303_130854_917633_782E7352 X-CRM114-Status: GOOD ( 36.25 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, 2 Mar 2021 13:54:15 +0300 Ivan Uvarov wrote: Hi Ivan, many thanks for spending the time in piecing this together and caring about upstreaming! > From: Ivan Uvarov > > This patch adds support for the Forlinx FETA40i-C SoM and OKA40i-C > devboard[1] that is based on it. The devicetree is split into a .dtsi > which (hopefully) corresponds to the functions of the SoM itself and > a .dts for the devboard. > > [1]:https://linux-sunxi.org/Forlinx_OKA40i-C > > Signed-off-by: Ivan Uvarov > --- > arch/arm/boot/dts/Makefile | 1 + > arch/arm/boot/dts/sun8i-r40-feta40i.dtsi | 68 +++++++ > arch/arm/boot/dts/sun8i-r40-oka40i-c.dts | 238 +++++++++++++++++++++++ > 3 files changed, 307 insertions(+) > create mode 100644 arch/arm/boot/dts/sun8i-r40-feta40i.dtsi > create mode 100644 arch/arm/boot/dts/sun8i-r40-oka40i-c.dts > > diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile > index 8e5d4ab4e7..88aae9de95 100644 > --- a/arch/arm/boot/dts/Makefile > +++ b/arch/arm/boot/dts/Makefile > @@ -1222,6 +1222,7 @@ dtb-$(CONFIG_MACH_SUN8I) += \ > sun8i-r16-nintendo-super-nes-classic.dtb \ > sun8i-r16-parrot.dtb \ > sun8i-r40-bananapi-m2-ultra.dtb \ > + sun8i-r40-oka40i-c.dtb \ > sun8i-s3-elimo-initium.dtb \ > sun8i-s3-lichee-zero-plus.dtb \ > sun8i-s3-pinecube.dtb \ > diff --git a/arch/arm/boot/dts/sun8i-r40-feta40i.dtsi b/arch/arm/boot/dts/sun8i-r40-feta40i.dtsi > new file mode 100644 > index 0000000000..edfb846db1 > --- /dev/null > +++ b/arch/arm/boot/dts/sun8i-r40-feta40i.dtsi > @@ -0,0 +1,68 @@ > +// SPDX-License-Identifier: GPL-2.0+ OR MIT > +// Copyright (C) 2021 Ivan Uvarov > +// Based on the sun8i-r40-bananapi-m2-ultra.dts, which is: > +// Copyright (C) 2017 Chen-Yu Tsai > +// Copyright (C) 2017 Icenowy Zheng > + > +#include "sun8i-r40.dtsi" > + > + > +&i2c0 { > + status =3D "okay"; > + > + axp22x: pmic@34 { > + compatible =3D "x-powers,axp221"; > + reg =3D <0x34>; > + interrupt-parent =3D <&nmi_intc>; > + interrupts =3D <0 IRQ_TYPE_LEVEL_LOW>; > + }; > +}; > +#include "axp22x.dtsi" > + > +&mmc2 { > + vmmc-supply =3D <®_dcdc1>; > + vqmmc-supply =3D <®_aldo2>; > + bus-width =3D <8>; > + non-removable; Should be a TAB here. And I don't know if this is just my client, but someone mangled equal signs into "=3D" everywhere :-( I am trying to ignore this for now ... Can you force pure text email in Thunderbird? Or use git send-email? > + status =3D "okay"; > +}; > + > + > +&pio { > + pinctrl-names =3D "default"; > + pinctrl-0 =3D <&clk_out_a_pin>; > + vcc-pa-supply =3D <®_dcdc1>; > + vcc-pc-supply =3D <®_aldo2>; > + vcc-pd-supply =3D <®_dcdc1>; > + vcc-pf-supply =3D <®_dldo4>; > + vcc-pg-supply =3D <®_dldo1>; > +}; > + > +®_aldo2 { > + regulator-always-on; > + regulator-min-microvolt =3D <1800000>; > + regulator-max-microvolt =3D <2500000>; > + regulator-name =3D "vcc-pa"; > +};//2500000uV reported by kernel I think min and max should be the same in those cases. Do you know what the recommended voltage is? Does the BSP report something? PortA is used for Ethernet, is that using 2.5V as the signalling voltage? > + > +®_dcdc1 { > + regulator-always-on; > + regulator-min-microvolt =3D <3300000>; > + regulator-max-microvolt =3D <3300000>; > + regulator-name =3D "vcc-3v3"; > +}; > + > + > +//I don't know whether these really belong here Since it seems to be used above, for VCC_PG: yes. > +®_dldo1 { > + regulator-always-on; > + regulator-min-microvolt =3D <3300000>; > + regulator-max-microvolt =3D <3300000>; > + regulator-name =3D "vcc-wifi-io"; > +}; > + > +®_dldo4 { > + regulator-always-on; Does this really need to be always on? If it's just for SATA, I'd expect this to be enabled by the driver when needed? As you correctly do in the ahci node below. > + regulator-min-microvolt =3D <2500000>; > + regulator-max-microvolt =3D <2500000>; > + regulator-name =3D "vdd2v5-sata"; Does this end prematurely here? > diff --git a/arch/arm/boot/dts/sun8i-r40-oka40i-c.dts b/arch/arm/boot/dts/sun8i-r40-oka40i-c.dts > new file mode 100644 > index 0000000000..7e47cf633e > --- /dev/null > +++ b/arch/arm/boot/dts/sun8i-r40-oka40i-c.dts > @@ -0,0 +1,238 @@ > +// SPDX-License-Identifier: GPL-2.0+ OR MIT > +// Copyright (C) 2021 Ivan Uvarov > +// Based on the sun8i-r40-bananapi-m2-ultra.dts, which is: > +// Copyright (C) 2017 Chen-Yu Tsai > +// Copyright (C) 2017 Icenowy Zheng > + > +/dts-v1/; > +#include "sun8i-r40-feta40i.dtsi" > + > +#include > + > +/ { > + model =3D "Forlinx OKA40i-C"; > + compatible =3D "forlinx,oka40i-c", "allwinner,sun8i-r40"; > + > + aliases { > + ethernet0 =3D &gmac; > + serial0 =3D &uart0; > + }; > + > + chosen { > + stdout-path =3D "serial0:115200n8"; > + }; > + > + connector { > + compatible =3D "hdmi-connector"; > + type =3D "a"; > + > + port { > + hdmi_con_in: endpoint { > + remote-endpoint =3D <&hdmi_out_con>; > + }; > + }; > + }; > + > + leds { > + compatible =3D "gpio-leds"; > + > + user-led-5 { > + label =3D "oka40i:led5:user"; The current binding uses different properties: function = LED_FUNCTION_xxx; color = ; > + gpios =3D <&pio 7 26 GPIO_ACTIVE_LOW>; I think it's customary to add the GPIO pin spelled out as a comment, so gpios = <&pio 7 26 GPIO_ACTIVE_LOW>; /* PH27 */ > + }; > + > + user-led-6 { > + label =3D "oka40i:led6:user"; > + gpios =3D <&pio 8 15 GPIO_ACTIVE_LOW>; > + }; > + > + }; > + > + reg_vcc5v0: vcc5v0 { > + compatible =3D "regulator-fixed"; > + regulator-name =3D "vcc5v0"; > + regulator-min-microvolt =3D <5000000>; > + regulator-max-microvolt =3D <5000000>; > + //gpio =3D <&pio 7 23 GPIO_ACTIVE_HIGH>; // PH23 Is that regulator switchable? If not, no need for commented lines, just remove them. I don't see PH23 connected to a regulator on the board. > + //enable-active-high; This is redundant anyway this the above GPIO_ACTIVE_HIGH. > + }; > + > + wifi_pwrseq: wifi_pwrseq { > + compatible =3D "mmc-pwrseq-simple"; > + reset-gpios =3D <&pio 1 10 GPIO_ACTIVE_LOW>; // PB10 WIFI_EN > + clocks =3D <&ccu CLK_OUTA>; > + clock-names =3D "ext_clock"; > + }; > +}; > + > +&ahci { > + ahci-supply =3D <®_dldo4>; > + phy-supply =3D <®_eldo2>; > + status =3D "okay"; > +}; > + > +&de { > + status =3D "okay"; > +}; > + > +&ehci1 { > + status =3D "okay"; > +}; > + > +&ehci2 { > + status =3D "okay"; > +}; > + > +&gmac { > + pinctrl-names =3D "default"; > + pinctrl-0 =3D <&gmac_rgmii_pins>; > + phy-handle =3D <&phy1>; > + phy-mode =3D "rgmii-id"; > + phy-supply =3D <®_dcdc1>; > + status =3D "okay"; > +}; > + > +&gmac_mdio { > + phy1: ethernet-phy@1 { > + compatible =3D "ethernet-phy-ieee802.3-c22"; > + reg =3D <1>; > + }; > +}; > + > +&hdmi { > + status =3D "okay"; > +}; > + > +&hdmi_out { > + hdmi_out_con: endpoint { > + remote-endpoint =3D <&hdmi_con_in>; > + }; > +}; > + > + > +&i2c2 { > + status =3D "okay"; > +}; > + > + > +&mmc0 { > + vmmc-supply =3D <®_dcdc1>; > + vqmmc-supply =3D <®_dcdc1>; > + bus-width =3D <4>; > + cd-gpios =3D <&pio 8 11 GPIO_ACTIVE_LOW>; // PI11 > + status =3D "okay"; > +}; > + > +&mmc1 { So this is the SDIO connector on the board, right? Which is just a set of header pins? Not sure we should have it in here, then. > + vmmc-supply =3D <®_dcdc1>; > + vqmmc-supply =3D <®_dcdc1>; > + mmc-pwrseq =3D <&wifi_pwrseq>; > + bus-width =3D <4>; > + status =3D "okay"; > +}; What about MMC3, which is apparently connected to a microSD slot (TF card in the schematic)? > + > +&ohci1 { > + status =3D "okay"; > +}; > + > +&ohci2 { > + status =3D "okay"; > +}; > + > +®_aldo3 { > + regulator-always-on; > + regulator-min-microvolt =3D <3000000>; > + regulator-max-microvolt =3D <3000000>; > + regulator-name =3D "avcc"; > +}; > + > +®_dc1sw { > + regulator-min-microvolt =3D <3300000>; > + regulator-max-microvolt =3D <3300000>; > + regulator-name =3D "vcc-lcd"; > +}; > + > +®_dcdc2 { > + regulator-always-on; > + regulator-min-microvolt =3D <1100000>; > + regulator-max-microvolt =3D <1160000>; > + regulator-name =3D "vdd-cpu"; > +};//1100000uV reported by kernel Again, this should be the one value then, for both min and max. > + > +®_dcdc3 { > + regulator-always-on; > + regulator-min-microvolt =3D <1100000>; > + regulator-max-microvolt =3D <1200000>; > + regulator-name =3D "vdd-sys"; > +};//1100000uV reported by kernel Same here. > + > + > +®_dcdc5 { > + regulator-always-on; > + regulator-min-microvolt =3D <1500000>; > + regulator-max-microvolt =3D <1500000>; > + regulator-name =3D "vcc-dram"; > +}; > + > +®_dldo2 { > + // regulator-always-on; Please, no commented properties. > + regulator-min-microvolt =3D <3300000>; > + regulator-max-microvolt =3D <3300000>; > + regulator-name =3D "vcc-wifi"; > +}; > + > +®_dldo3 { // possibly unneeded If it's unneeded, you can drop it. If it turns out be needed later on, we can always add it. Did the board work without it? Did you try Wifi? We should not add untested features. > + // regulator-always-on; > + regulator-min-microvolt =3D <3300000>; > + regulator-max-microvolt =3D <3300000>; > + regulator-name =3D "vcc-wifi-2"; > +}; > + > +®_eldo2 { > + regulator-always-on; This shouldn't be always on. I guess this AXP pin is connected to the VDD-SATA pin on the SoC, so it just drives the integrated SATA PHY. Which probably means that this node belong into the SoM .dtsi, as the connection is on the SoM. You just reference it from the ahci node here. > + regulator-min-microvolt =3D <1200000>; > + regulator-max-microvolt =3D <1200000>; > + regulator-name =3D "vdd1v2-sata"; > +}; > + > +®_eldo3 { > + regulator-always-on; Again, why always on? Do you know where this is connected to? If this is VCC_PE on the SoC, then I wonder if this should be moved to the SoM .dtsi, with a range between 1.8V and 3.3V, since those are valid input voltages for the VCC_PE pin. And then here you overwrite this with 2.8V, as this is apparently used for the CSI connector on the board, which seems to work on 2.8V. But I am not sure if this is too much, and just having this node here is sufficient. > + regulator-min-microvolt =3D <2800000>; > + regulator-max-microvolt =3D <2800000>; > + regulator-name =3D "vcc-pe"; > +}; > + > +&tcon_tv0 { > + status =3D "okay"; > +}; > + > +&uart0 { > + pinctrl-names =3D "default"; > + pinctrl-0 =3D <&uart0_pb_pins>; > + status =3D "okay"; > +}; > + > +&uart3 { Please add uart3 to the aliases section then. Cheers, Andre > + pinctrl-names =3D "default"; > + pinctrl-0 =3D <&uart3_pg_pins>, <&uart3_rts_cts_pg_pins>; > + uart-has-rtscts; > + status =3D "okay"; > + > + bluetooth { > + compatible =3D "brcm,bcm43438-bt"; > + clocks =3D <&ccu CLK_OUTA>; > + clock-names =3D "lpo"; > + vbat-supply =3D <®_dldo2>; > + vddio-supply =3D <®_dldo1>; > + device-wakeup-gpios =3D <&pio 6 11 GPIO_ACTIVE_HIGH>; /* PG11 */ > + /* TODO host wake line connected to PMIC GPIO pins */ > + shutdown-gpios =3D <&pio 7 12 GPIO_ACTIVE_HIGH>; /* PH12 */ > + max-speed =3D <1500000>; > + }; > +}; > + > +&usbphy { > + usb1_vbus-supply =3D <®_vcc5v0>; > + usb2_vbus-supply =3D <®_vcc5v0>; > + status =3D "okay"; > +}; _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel