Message-ID: <8453efd3-630e-4f2c-950d-88a73927cc54@collabora.com>
Date: Mon, 20 Oct 2025 16:02:58 +0200
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Daniel Golle <daniel@...rotopia.org>
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

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.

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

Cheers,
Angelo

Powered by blists - more mailing lists