Re: [PATCH net-next v3 2/2] dsa: lan9303: Move to PHYLINK

From: Vladimir Oltean <olteanv@gmail.com>
To: "Russell King (Oracle)" <linux@armlinux.org.uk>
Cc: Jerry Ray <jerry.ray@microchip.com>, Andrew Lunn <andrew@lunn.ch>,
	Florian Fainelli <f.fainelli@gmail.com>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v3 2/2] dsa: lan9303: Move to PHYLINK
Date: Wed, 7 Dec 2022 00:12:38 +0200	[thread overview]
Message-ID: <20221206221238.5p4wv472wwcjowy5@skbuf> (raw)
In-Reply-To: <Y4+vKh8EfA9vtC2B@shell.armlinux.org.uk>

On Tue, Dec 06, 2022 at 09:07:54PM +0000, Russell King (Oracle) wrote:
> > Are you going to explain why modifying this register is no longer needed?
> 
> ... otherwise it is a fixed link, so the PHY is configured for the fixed
> link setting - which I think would end up writing to the an emulation of
> the PHY, and would end up writing the same settings back to the PHY as
> the PHY was already configured.

To be clear, when you say "an emulation of the PHY", are you talking
about the swphy behind the fixed-link, or about the LAN9303_VIRT_PHY_BASE
registers, which correspond to the RevMII Virtual PHY of the switch CPU port?

As far as I can understand the Microchip LAN9303 documentation, the DSA
master can have a phy-handle to the switch node (which
devicetree/bindings/net/dsa/lan9303.txt seems to confirm), and the
switch can pretend it's a PHY when accessed by a switch-unaware
(Generic) PHY driver at the usual PHY MDIO registers. Through the
Virtual PHY feature and registers, it can also pretend it's the "other"
PHY, and this MII_BMCR register of the Virtual PHY can ultimately
autoneg with "itself" and control what the DSA master sees in terms of
reported speed, duplex, and AN complete.

Prior to this change, the driver, when given a DT blob with a fixed-link
on the switch CPU port, would disable BMCR_ANENABLE in the Virtual PHY.
After the change, it would leave things as they are (which is not
necessarily the way things are out of reset). Which way is better?
Does it matter? Is it a stupid question? No clue.

> So, I don't think adjust_link does anything useful, and I think this is
> an entirely appropriate change.

That it may well be, but its presentation is entirely inappropriate.
Andrew has told Jerry before that it's important to split, describe and
justify his changes accordingly, so it's not like the things I'm
complaining about are news to him. Things would go a lot smoother if
Jerry explained his patches better.

Reviewing patches which do stuff that isn't explained in the commit
message reminds me of Forrest Gump. Life is like a box of chocolates,
you never know what you're going to get. That's not how it's supposed to
work, a box of chocolates should contain chocolates, at least here.