Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 21 May 2023 13:28:09 +0300
From:   Vladimir Oltean <olteanv@gmail.com>
To:     Oleksij Rempel <o.rempel@pengutronix.de>
Cc:     Woojung Huh <woojung.huh@microchip.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Arun Ramadoss <arun.ramadoss@microchip.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Simon Horman <simon.horman@corigine.com>,
        "Russell King (Oracle)" <linux@armlinux.org.uk>,
        linux-kernel@vger.kernel.org, Eric Dumazet <edumazet@google.com>,
        Paolo Abeni <pabeni@redhat.com>, kernel@pengutronix.de,
        netdev@vger.kernel.org, Jakub Kicinski <kuba@kernel.org>,
        UNGLinuxDriver@microchip.com,
        "David S. Miller" <davem@davemloft.net>
Subject: Re: [PATCH net-next v4 1/2] net: dsa: microchip: ksz8: Make flow
 control, speed, and duplex on CPU port configurable
Message-ID: <20230521102809.i3o55e4nuuy7dtdu@skbuf>
References: <20230519124700.635041-1-o.rempel@pengutronix.de>
 <20230519124700.635041-2-o.rempel@pengutronix.de>
 <20230519143004.luvz73jiyvnqxk4y@skbuf>
 <20230519185015.GA18246@pengutronix.de>
 <20230519203449.pc5vbfgbfc6rdo6i@skbuf>
 <20230520050317.GC18246@pengutronix.de>
 <20230520151708.24duenxufth4xsh5@skbuf>
 <20230521043841.GA22442@pengutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230521043841.GA22442@pengutronix.de>
Precedence: bulk

On Sun, May 21, 2023 at 06:38:41AM +0200, Oleksij Rempel wrote:
> Looks good, I like the idea with "wacky" registers!
> 
> Would you prefer that I start working on adapting your patch set to the
> KSZ8873? Or should I make a review to move forward the existing patch set?
> 
> Just a heads up, I don't have access to the KSZ87xx series switches, so
> I won't be able to test the changes on these models.
> 
> Let me know what you think and how we should proceed.

If we convert to regfields, the entire driver will need to be converted
(all switch families). I'd say make a best effort full conversion, and
if something breaks on the families which you could not test, surely
someone will jump to correct it. Since your KSZ8873 also has wacky registers
(btw, feel free to rename the concept to something more serious), I think
that not a lot can go wrong with a blind conversion as long as it's tested
on other hardware.

BTW, revisiting, I already found a bug in the conversion (patch 2/4):

+	} else if (mii_sel == bitval[P_RMII_SEL]) {
 		interface = PHY_INTERFACE_MODE_RGMII;
 	} else {
+		ret = ksz_regfields_read(dev, port, RF_RGMII_ID_IG_ENABLE, &ig);
+		if (WARN_ON(ret))
+			return PHY_INTERFACE_MODE_NA;
+
+		ret = ksz_regfields_read(dev, port, RF_RGMII_ID_IG_ENABLE, &eg);
						    ~~~~~~~~~~~~~~~~~~~~~
						    should have been RF_RGMII_ID_EG_ENABLE
+		if (WARN_ON(ret))
+			return PHY_INTERFACE_MODE_NA;
+
 		interface = PHY_INTERFACE_MODE_RGMII;
-		if (data8 & P_RGMII_ID_EG_ENABLE)
+		if (eg)
 			interface = PHY_INTERFACE_MODE_RGMII_TXID;
-		if (data8 & P_RGMII_ID_IG_ENABLE) {
+		if (ig) {
 			interface = PHY_INTERFACE_MODE_RGMII_RXID;
-			if (data8 & P_RGMII_ID_EG_ENABLE)
+			if (eg)
 				interface = PHY_INTERFACE_MODE_RGMII_ID;
 		}
 	}