On Tue, Dec 10, 2024 at 01:06:29PM +0100, Christian Marangi wrote:

On Tue, Dec 10, 2024 at 02:53:34AM +0100, Andrew Lunn wrote:

quoted

quoted

+static int an855_phy_restore_page(struct an8855_mfd_priv *priv,
+				  int phy) __must_hold(&priv->bus->mdio_lock)
+{
+	/* Check PHY page only for addr shared with switch */
+	if (phy != priv->switch_addr)
+		return 0;
+
+	/* Don't restore page if it's not set to switch page */
+	if (priv->current_page != FIELD_GET(AN8855_PHY_PAGE,
+					    AN8855_PHY_PAGE_EXTENDED_4))
+		return 0;
+
+	/* Restore page to 0, PHY might change page right after but that
+	 * will be ignored as it won't be a switch page.
+	 */
+	return an8855_mii_set_page(priv, phy, AN8855_PHY_PAGE_STANDARD);
+}

I don't really understand what is going on here. Maybe the commit
message needs expanding, or the function names changing.

Generally, i would expect a save/restore action. Save the current
page, swap to the PHY page, do the PHY access, and then restore to the
saved page.

Idea is to save on extra read/write on subsequent write on the same
page.

Idea here is that PHY will receive most of the read (for status
poll) hence in 90% of the time page will be 0.

And switch will receive read/write only on setup or fdb/vlan access on
configuration so it will receive subsequent write on the same page.
(page 4)

PHY might also need to write on page 1 on setup but never on page 4 as
that is reserved for switch.

Making the read/swap/write/restore adds 2 additional operation that can
really be skipped 90% of the time.

Also curret_page cache is indirectly protected by the mdio lock.

So in short this function makes sure PHY for port 0 is configured on the
right page based on the prev page set.

An alternative way might be assume PHY is always on page 0 and any
switch operation save and restore the page.

Hope it's clear now why this is needed. Is this ok or you prefer the
alternative way? 

Please think about naming. I'm not sure _restore_ is the correct name
for the function. Maybe select. And i suggest adding some comments to
explain what is going on, since it is not obvious from the code in
this file, you probably need to cross reference it with code in the
DSA driver as well.

	Andrew