Message-ID: <aPZr3WMybjTWnn9E@makrotopia.org>
Date: Mon, 20 Oct 2025 18:05:33 +0100
From: Daniel Golle <daniel@...rotopia.org>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
Cc: Sjoerd Simons <sjoerd@...labora.com>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Matthias Brugger <matthias.bgg@...il.com>,
	Ryder Lee <ryder.lee@...iatek.com>,
	Jianjun Wang <jianjun.wang@...iatek.com>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Lorenzo Pieralisi <lpieralisi@...nel.org>,
	Krzysztof Wilczyński <kwilczynski@...nel.org>,
	Manivannan Sadhasivam <mani@...nel.org>,
	Chunfeng Yun <chunfeng.yun@...iatek.com>,
	Vinod Koul <vkoul@...nel.org>,
	Kishon Vijay Abraham I <kishon@...nel.org>,
	Lee Jones <lee@...nel.org>, Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Lorenzo Bianconi <lorenzo@...nel.org>, Felix Fietkau <nbd@....name>,
	kernel@...labora.com, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org, linux-pci@...r.kernel.org,
	linux-phy@...ts.infradead.org, netdev@...r.kernel.org,
	Bryan Hinton <bryan@...anhinton.com>
Subject: Re: [PATCH 02/15] arm64: dts: mediatek: mt7981b-openwrt-one:
 Configure UART0 pinmux

On Mon, Oct 20, 2025 at 04:02:58PM +0200, AngeloGioacchino Del Regno wrote:
> 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 <sjoerd@...labora.com>
> > > > > > > ---
> > > > > > >     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@...00000 {
> > > > > > >     	};
> > > > > > >     };
> > > > > > > +&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.

No, that's not a problem. During reset the pinctrl/gpio controller is always
reset to the default and no matter how the pins were used in Linux before the
reset. Hence debug output and also emrgency download mode always works.
The only disadvantage of use the pins differently is that the bootrom output
on one of them cannot be prevented -- but in case that's not a problem (eg.
because the pin is later used as an input rather than output) it can totally
be done, though it would be stupid as it would render the debug UART unusable.
Yet, I'd consider it a possible choice of a board designer.

> 
> 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.

So this criteria, avoiding unnecessary duplication, is also what I thought and
is very true for the argument I presented before which somehow wasn't what has
convinced you: That using uart0 in any possible way **always** implied using
the uart0 pingroup in uart mode, because there aren't any other pins which can
be used for uart0. In this sense, if uart0 is used at all, it is **not** the
choice of the board designer which pins to use for that -- there simply is only
that one single option.

> 
> 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.

That's exactly my point: There isn't any other option to route uart0 to. Only
those two pins. The other alternative functions of those pins (apart from GPIO)
are rather esoteric debugging features (I2C access to SoC internals).

> 
> 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.

So the choice of a HW engineer regarding uart0 is simply whether uart0
is used or not. If uart0 is used, the HW engineer doesn't have any choice
regarding which pins they would like to use for the uart0 RX and TX lines,
the SoC design dictates exactly one option for that.

> 
> 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 <angelogioacchino.delregno@...labora.com>
> > 
> > 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.

Thank you, that should make it clear to Sjoerd as well (who may skip and ignore
all of our debating :).

Powered by blists - more mailing lists