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 CAD05CCD199 for ; Mon, 20 Oct 2025 14:03:12 +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=FP3wJ4egQPPrPTjBkz89dEMC48T1xKcDeExWX0daaJU=; b=DBlTelqNfEasrpRmTD0dWr97oj 7N41rDv0T4/UCR0NVQUv4RQmIhfoGBY3Pmn+F2aNCdXbmjpL20eD7GlFKw2vqJ5KrtyVM1ZmkcW6f NIGd7tJOoADG/YuFpGtioQ0rnpbVSH/0ovMKIgXZKtI9NStSAfrD0/TF40RrPjj7AX65UOcd7VAZD r1SNbKT7lQzzHsAEKkT3UkX8vMoA7yUZ7JbL6IVmjrGY21ZKxEU0wdNBGMEYSQ9ZmH+da2nKtXN8M jRLDm+eiy8PpAVjEw0eiPM1aF9TxM7kE1O4MogPrk4yb/hdNOlGbMe0+VWClVnsX9JaljjJVrS45V UJ4JUtfw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98.2 #2 (Red Hat Linux)) id 1vAqTa-0000000Dt57-1Xm3; Mon, 20 Oct 2025 14:03:06 +0000 Received: from bali.collaboradmins.com ([148.251.105.195]) by bombadil.infradead.org with esmtps (Exim 4.98.2 #2 (Red Hat Linux)) id 1vAqTX-0000000Dt4K-3rL9; Mon, 20 Oct 2025 14:03:05 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1760968980; bh=wa/LFx+3nxvEsI8+aKge1iAS1QhaGGNlrbnB3buk7SI=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=VDIf1diVrnftwL38ZREdQLDRwHdtKgievZhFbibJhQFutwAztvjM5o0Ofy8B+MuJ1 gbB2fB5zCNc+lFbJoxBbEDZzf1uheBzC+Nlx9sun+NUI/d/KYBq3Ff28STT0H965tq nnrbsAdRosSrONRkoiNeZgNC8PuvSRPgPpkICA0Q54jAzdpR5hhw1tu2Y1cS27zwJX FRg9vM1KZf5TCg1umqLDbOg3lDUSE36e1IEZJfrh77+iTevPfFGOU0jL5YfxqKJ1H/ gzueE+BKPyTjIQVERY1kpaRD5P4l5aKwVvJQygCbM/bzM0GDjLk7n4Ja3+QPhI/Axl 9w4l4cXjKqsWA== Received: from [192.168.1.100] (2-237-20-237.ip236.fastwebnet.it [2.237.20.237]) (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 bali.collaboradmins.com (Postfix) with ESMTPSA id 0AF5A17E0456; Mon, 20 Oct 2025 16:02:59 +0200 (CEST) Message-ID: <8453efd3-630e-4f2c-950d-88a73927cc54@collabora.com> Date: Mon, 20 Oct 2025 16:02:58 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 02/15] arm64: dts: mediatek: mt7981b-openwrt-one: Configure UART0 pinmux To: Daniel Golle Cc: Sjoerd Simons , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Matthias Brugger , Ryder Lee , Jianjun Wang , Bjorn Helgaas , Lorenzo Pieralisi , =?UTF-8?Q?Krzysztof_Wilczy=C5=84ski?= , Manivannan Sadhasivam , Chunfeng Yun , Vinod Koul , Kishon Vijay Abraham I , Lee Jones , Andrew Lunn , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Lorenzo Bianconi , Felix Fietkau , kernel@collabora.com, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, linux-pci@vger.kernel.org, linux-phy@lists.infradead.org, netdev@vger.kernel.org, Bryan Hinton References: <20251016-openwrt-one-network-v1-0-de259719b6f2@collabora.com> <20251016-openwrt-one-network-v1-2-de259719b6f2@collabora.com> <5f430ff9-d701-426a-bf93-5290e6912eb4@collabora.com> <82594ce7-f093-4753-b808-cd234845aed8@collabora.com> From: AngeloGioacchino Del Regno Content-Language: en-US In-Reply-To: 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-20251020_070304_128427_55476EA7 X-CRM114-Status: GOOD ( 39.57 ) 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: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Il 20/10/25 14:28, Daniel Golle ha scritto: > On Mon, Oct 20, 2025 at 12:23:14PM +0200, AngeloGioacchino Del Regno wrote: >> Il 16/10/25 18:37, Daniel Golle ha scritto: >>> On Thu, Oct 16, 2025 at 04:29:14PM +0200, AngeloGioacchino Del Regno wrote: >>>> Il 16/10/25 14:38, Daniel Golle ha scritto: >>>>> On Thu, Oct 16, 2025 at 12:08:38PM +0200, Sjoerd Simons wrote: >>>>>> Add explicit pinctrl configuration for UART0 on the OpenWrt One board, >>>>>> >>>>>> Signed-off-by: Sjoerd Simons >>>>>> --- >>>>>> arch/arm64/boot/dts/mediatek/mt7981b-openwrt-one.dts | 11 +++++++++++ >>>>>> 1 file changed, 11 insertions(+) >>>>>> >>>>>> diff --git a/arch/arm64/boot/dts/mediatek/mt7981b-openwrt-one.dts b/arch/arm64/boot/dts/mediatek/mt7981b-openwrt-one.dts >>>>>> index 968b91f55bb27..f836059d7f475 100644 >>>>>> --- a/arch/arm64/boot/dts/mediatek/mt7981b-openwrt-one.dts >>>>>> +++ b/arch/arm64/boot/dts/mediatek/mt7981b-openwrt-one.dts >>>>>> @@ -22,6 +22,17 @@ memory@40000000 { >>>>>> }; >>>>>> }; >>>>>> +&pio { >>>>>> + uart0_pins: uart0-pins { >>>>>> + mux { >>>>>> + function = "uart"; >>>>>> + groups = "uart0"; >>>>>> + }; >>>>>> + }; >>>>>> +}; >>>>>> + >>>>>> &uart0 { >>>>>> + pinctrl-names = "default"; >>>>>> + pinctrl-0 = <&uart0_pins>; >>>>>> status = "okay"; >>>>>> }; >>>>> >>>>> As there is only a single possible pinctrl configuration for uart0, >>>>> both the pinmux definition as well as the pinctrl properties should go >>>>> into mt7981b.dtsi rather than in the board's dts. >>>> >>>> If there's really one single possible pin configuration for the UART0 pins, >>>> as in, those pins *do not* have a GPIO mode, then yes I agree. >>>> >>>> If those pins can be as well configured as GPIOs, this goes to board DTS. >>> >>> I respectfully disagree and will explain below. >>> >> >> Thanks a lot for taking the time to write all this - explains everything, >> and even too much :) :) >> >> Though, there's something funny here! The following snippet of "main" text >> does explain stuff that is interesting, but that I (not other people, so >> thanks again for saying all this) know already, but..... >> >>> All pinmux pins on the MediaTek platform also allow being configured as >>> GPIOs. However, if you configure those as GPIOs the consequence is that >>> you cannot use UART0 any more at all. So using UART0 at all always >>> implies using exactly those pins, there is no alternative to that. >>> >>> Hence every board with every possible uses of pins 32 and 33 (there is >>> only RX and TX for UART0, RTS/CTS flow-control is not possible) can be >>> represented without needing to configure the pinctrl for uart0 on the >>> board level. There isn't going to be any variation on the board-level >>> when it comes to uart0. Either it is enabled (status = "okay";), and >>> that will always imply using the 'uart0' group in mode 'uart', or, in >>> case any of the two pins of uart0 is used for something else that means >>> uart0 cannot be enabled. Simple as that. >>> >>> Hence there is no need to duplicate that pinctrl settings on each and >>> every board, as controlling the 'status' property on the board-level >>> already gives 100% freedom. >>> >> >> ...all of this is not justifying your point. > > So what is the rule then? I understand the logic of describing the > pins eg. for uart1 only on board-level as there are actual alternatives > regarding the pins to be used, and if also including RTS/CTS pins. > Hence, for uart1, there are several possible pingroups which can be > used. What would be the argument to keep a pinctrl description for > which the SoC doesn't offer any alternatives to be on the board-level? > There is nothing to be decided by the board, literally 0 freedom. > As you described - the BootROM is using those two pins as UART0. Should you want those pins to be used as GPIOs, you'd at least get HW glitches in early boot phases, or you'd render emergency download mode unusable - which is not a good idea, not practical, and also, well, almost a stupid thing to do from the hardware perspective. This means that it is very, very, very unlikely (to the point that it's practically impossible) that those pins can ever be used for anything else that is not *the* one of the two functions that are supported for them (which is UART0 in this case). In this case, adding the pins at the board level would only create unnecessary duplication and nothing else, because, well, noone could possibly ever use those for anything else, again. That's the criteria. If the BootROM didn't use those pins, and those could support both GPIO mode and HW function mode (any: uart0, 1, 2...n, spi, i2c, whatever else), even though it is likely for boards to use them for one specific function, there is nothing that stops a HW engineer to decide to route those elsewhere and use them as a GPIO instead, so that's not a SoC configuration, but rather a HW implementation decision at the PCB level. See it like this (although this is an oversimplified view): - SoC DT describes the SoC (the chip) - in this case the MT7981B chip - Board DT describes decisions that were taken by the HW engineer that developed the PCB on which the MT7981B was placed. Clearly, if there's a board design (usually, a "base project") that has derivatives (for example, a device with eMMC, one with UFS, one with both, one with two SFP, one with one SFP and one soldered ethernet chip on a non-exposed SFP interface, etc) it is ok to have a "board-common" dtsi and specific board variants on top, like it is done with some bananapi and some genio boards. Lots of text here - yet oversimplified. There is much more to say, but I think (and hope) that this is enough to make you understand the main point (of course feel free to throw more questions if what I wrote doesn't fully satisfy you). >> >>> (Sidenote: As even the BootROM already uses those two pins as UART for >>> debug output, >> >> Funny thing is, your side note is what *fully* justifies your disagreement >> and it's also what triggers me to say that you're right, lol :) >> >> Okay then, I am fine with this commit now and I can renew my >> >> Reviewed-by: AngeloGioacchino Del Regno > > Note that the patch you have just added your Reviewed-by:-tag to does > *not* add the uart0 pinctrl on SoC-level but board-level, so different > from what I argued for above. Ewwww I'm doing too may things at once. Pretty crazy days around here :))) >> Did you mean to add Reviewed-by: for that > (which contraticts what you just wrote) or rather to the to-be-submitted > v2 of this series which includes the change to move the uart0 pinctrl > to mt7981b.dtsi? Yeah. Sorry. I repeat then, so that this is clear: you are right, the pinctrl for UART0 on the MT7981B SoC must go to mt7981b.dtsi and *not* to mt7981b-openwrt-one. Cheers, Angelo