Re: Re: [PATCH] arm64: dts: sun50i: h5: NanoPI Neo 2: phy-mode rgmii-id

From: "Jernej Škrabec" <jernej.skrabec@gmail.com>
To: Andrew Lunn <andrew@lunn.ch>
Cc: "Maxime Ripard" <mripard@kernel.org>,
	"Chen-Yu Tsai" <wens@csie.org>, "Clément Bœsch" <u@pkh.me>,
	"Willy Liu" <willy.liu@realtek.com>,
	"Rob Herring" <robh+dt@kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	linux-arm-kernel@lists.infradead.org,
	linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: Re: [PATCH] arm64: dts: sun50i: h5: NanoPI Neo 2: phy-mode rgmii-id
Date: Tue, 31 Aug 2021 21:53:50 +0200	[thread overview]
Message-ID: <1746741.3t3T1tWQfl@kista> (raw)
In-Reply-To: <YS1SsqDgGFOvyIT0@lunn.ch>

Hi Andrew!

Dne ponedeljek, 30. avgust 2021 ob 23:50:42 CEST je Andrew Lunn napisal(a):
> On Mon, Aug 30, 2021 at 10:49:37PM +0200, Jernej Škrabec wrote:
> > Hi!
> > 
> > Dne ponedeljek, 30. avgust 2021 ob 17:16:45 CEST je Clément Bœsch 
napisal(a):
> > > Since commit bbc4d71d6354 ("net: phy: realtek: fix rtl8211e rx/tx delay
> > > config") network is broken on the NanoPi Neo 2.
> > > 
> > > This patch changes the phy-mode to use internal delays both for RX and
> > > TX as has been done for other boards affected by the same commit.
> > > 
> > > Fixes: bbc4d71d6354 ("net: phy: realtek: fix rtl8211e rx/tx delay config")
> > 
> > This commit fixes DT issue, so "fixes" tag should be:
> > Fixes: 44a94c7ef989 ("arm64: dts: allwinner: H5: Restore EMAC changes")
> > 
> > Here, a node with wrong phy-mode property was added to NanoPi Neo 2 board 
DT.  
> 
> Hi Jernej
> 
> I would say, it is debatable. The board broke when the driver started
> acting on the value, which is commit bbc4d71d6354. It could be argued
> it was always technically broken, since it has the wrong value, but
> practically, it was not broken. One of the rules for stable is:
> 
>  - It must fix a real bug that bothers people (not a, "This could be a
>    problem..." type thing).
> 
> So i would argue, anything before bbc4d71d6354 does not fulfil stable
> requirements, since it does not bother anybody.

True, but then again, DT is a bit special, since it's not used only by Linux. 
It's shared at least with BSDs and U-Boot, which most likely have independent 
driver implementation. That's why it's good to have DT fixes referencing DT 
related commits. As you said, DT described HW wrong.

Looking at past Allwinner related DT commits regarding this issue, we were not 
totally consistent. Most of them reference commit where emac node was 
introduced in DT. But I also found a couple of commits which indeed have fixes 
tag for bbc4d71d6354.

Best regards,
Jernej

> 
>       Andrew
> 